lkml.org 
[lkml]   [2018]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac
From
Date
On 3/20/2018 8:58 AM, Alexey Roslyakov wrote:
> Arend,
>
>> Also I am not sure if the broken-sg-support is still needed. We added that for omap_hsmmc, but that has since changed to scatter-gather emulation so it might not be needed anymore.
> I can confirm it doesn't impact wifi performance in case of rk3288+ap6335.
>
> But I still have to set settings->bus.sdio.sd_head_align = 4 and
> settings->bus.sdio.sd_sgentry_align = 512, otherwise
> brcmf_sdiod_sglist_rw fails:
>
> 974.888452] net_ratelimit: 8 callbacks suppressed
> [ 974.888457] brcmfmac: brcmf_sdiod_sglist_rw: CMD53 sg block read failed -84
> [ 974.901527] brcmfmac: brcmf_sdio_rxglom: glom read of 6144 bytes failed: -5
> [ 974.910248] brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame
> [ 975.018041] brcmfmac: brcmf_sdiod_sglist_rw: CMD53 sg block read failed -84
> [ 975.025833] brcmfmac: brcmf_sdio_rxglom: glom read of 6144 bytes failed: -5
> [ 975.033634] brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame
> [ 975.033924] dwmmc_rockchip ff0d0000.dwmmc: Unexpected command
> timeout, state 0
> [ 975.049209] brcmfmac: brcmf_sdio_readframes: RXHEADER FAILED: -84
> [ 975.056034] brcmfmac: brcmf_sdio_rxfail: abort command, terminate
> frame, send NAK
> [ 975.068367] brcmfmac: brcmf_sdio_hdparse: HW header length too long
> [ 975.075379] brcmfmac: brcmf_sdio_rxfail: terminate frame

Hi Alex,

Thanks for checking. In your case I think you do not need sd_head_align
as it will default to either 4 or 8:

#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
#define ALIGNMENT 8
#else
#define ALIGNMENT 4
#endif

I am not saying you should not be needing this. When it comes to DT
people are often tempted to accommodate a driver solution especially
when such a solution is already in place. However, DT is a hardware
description and these do not describe the wifi device. They are
applicable to the wifi device because it is a limitation of the host
controller and as such should be described in the DT binding of the host
controller.

Regards,
Arend

> Regards,
> Alex
>
> On 20 March 2018 at 06:16, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> + Uffe
>>
>> On 3/19/2018 6:55 PM, Florian Fainelli wrote:
>>>
>>> On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:
>>>>
>>>> Hi Arend,
>>>> I appreciate your response. In my opinion, it has nothing to do with
>>>> SDIO host, because it defines "quirks" in the driver itself.
>>>
>>>
>>> It is not clear to me from your patch series whether the problem is that:
>>>
>>> - the SDIO device has a specific alignment requirements, which would be
>>> either a SDIO device driver limitation/issue or maybe the underlying
>>> hardware device/firmware requiring that
>>>
>>> - the SDIO host controller used is not capable of coping nicely with
>>> these said limitations
>>>
>>> It seems to me like what you are doing here is a) applicable to possibly
>>> more SDIO devices and host combinations, and b) should likely be done at
>>> the layer between the host and device, such that it is available to more
>>> combinations.
>>
>>
>> Indeed. That was my thought exactly and I can not imagine Uffe would push
>> back on that reasoning.
>>
>>>> If I get it right, you mean something like this:
>>>>
>>>> mmc3: mmc@1c12000 {
>>>> ...
>>>> broken-sg-support;
>>>> sd-head-align = 4;
>>>> sd-sgentry-align = 512;
>>>>
>>>> brcmf: wifi@1 {
>>>> ...
>>>> };
>>>> };
>>>>
>>>> Where dt: bindings documentation for these entries should reside?
>>>> In generic MMC bindings? Well, this is the very special case and
>>>> mmc-linux maintainer will unlikely to accept these changes.
>>>> Also, extra kernel code modification might be required. It could make
>>>> quite trivial change much more complex.
>>>
>>>
>>> If the MMC maintainers are not copied on this patch series, it will
>>> likely be hard for them to identify this patch series and chime in...
>>
>>
>> The main question is whether this is indeed a "very special case" as Alexey
>> claims it to be or that it is likely to be applicable to other device and
>> host combinations as you are suggesting.
>>
>> If these properties are imposed by the host or host controller it would make
>> sense to have these in the mmc bindings.
>>
>>>>
>>>>> Also I am not sure if the broken-sg-support is still needed. We added
>>>>> that for omap_hsmmc, but that has since changed to scatter-gather emulation
>>>>> so it might not be needed anymore.
>>>>
>>>>
>>>> I've experienced the problem with rk3288 (dw-mmc host) and sdio
>>>> settings like above solved it.
>>>> Frankly, I haven't investigated any deeper which one of the settings
>>>> helped in my case yet...
>>>> I will try to get rid of broken-sg-support first and let you know if
>>>> it does make any difference.
>>
>>
>> Are you using some chromebook. I have some lying around here so I could also
>> look into it. What broadcom chipset do you have?
>>
>> Regards,
>> Arend
>>
>>
>>>> All the best,
>>>> Alex.
>>>>
>>>> On 19 March 2018 at 16:31, Arend van Spriel
>>>> <arend.vanspriel@broadcom.com> wrote:
>>>>>
>>>>> On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:
>>>>>>
>>>>>>
>>>>>> In case if the host has higher align requirements for SG items, allow
>>>>>> setting device-specific aligns for scatterlist items.
>>>>>>
>>>>>> Signed-off-by: Alexey Roslyakov <alexey.roslyakov@gmail.com>
>>>>>> ---
>>>>>> Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>>>>>> | 5
>>>>>> +++++
>>>>>> 1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>>>>>> b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>>>>>> index 86602f264dce..187b8c1b52a7 100644
>>>>>> ---
>>>>>> a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>>>>>> +++
>>>>>> b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>>>>>> @@ -17,6 +17,11 @@ Optional properties:
>>>>>> When not specified the device will use in-band SDIO
>>>>>> interrupts.
>>>>>> - interrupt-names : name of the out-of-band interrupt, which must
>>>>>> be
>>>>>> set
>>>>>> to "host-wake".
>>>>>> + - brcm,broken-sg-support : boolean flag to indicate that the SDIO
>>>>>> host
>>>>>> + controller has higher align requirement than 32 bytes for each
>>>>>> + scatterlist item.
>>>>>> + - brcm,sd-head-align : alignment requirement for start of data
>>>>>> buffer.
>>>>>> + - brcm,sd-sgentry-align : length alignment requirement for each sg
>>>>>> entry.
>>>>>
>>>>>
>>>>>
>>>>> Hi Alexey,
>>>>>
>>>>> Thanks for the patch. However, the problem with these is that they are
>>>>> characterizing the host controller and not the wireless device. So from
>>>>> device tree perspective , which is to describe the hardware, these
>>>>> properties should be SDIO host controller properties. Also I am not sure
>>>>> if
>>>>> the broken-sg-support is still needed. We added that for omap_hsmmc, but
>>>>> that has since changed to scatter-gather emulation so it might not be
>>>>> needed
>>>>> anymore.
>>>>>
>>>>> Regards,
>>>>> Arend
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
>

\
 
 \ /
  Last update: 2018-03-20 10:03    [W:0.265 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site