lkml.org 
[lkml]   [2018]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac
    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@broadcom.com> 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@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
    >>>>>
    >>>>>
    >>>>>
    >>>>>
    >>>>>
    >>>>
    >>>>
    >>>
    >>
    >>
    >>
    >



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

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