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

[Alexey Roslyakov]

Arend,

Agreed. Let's dismiss these patches.
Now I'm curious if I can get the information about DMA SG limitations
from MMC layer, I'll try to figure out something.

BTW, my specific setup (with default alignments) triggers kernel panic
(I see brcm_* in backtrace). It's better if I create separate email
chain for the issue.

Thanks,
  Alex.

On 20 March 2018 at 16:03, Arend van Spriel
<arend.vanspriel@xxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxx>
>>>>>>> ---
>>>>>>>
>>>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>>
>



-- 
With best regards,
  Alexey Roslyakov
Email: alexey.roslyakov@xxxxxxxxx