Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac

[Arend van Spriel <arend.vanspriel@broadcom.com>]

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
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
>