| Message ID | 20180319014032.9394-3-alexey.roslyakov@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | brcmfmac: add new dt entries for SG SDIO settings | expand |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
>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.
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
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. > 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... > >> 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. > > 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 > > >
+ 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 >> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arend, I use RK3288-firefly, bcm4339 (ap6335). 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 >>> >>> >>> >>> >> >> >
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 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 >>> >>> >>> >>> >> >> >
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 >>>> >>>> >>>> >>>> >>> >>> >> > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >> >> >> >
Arend van Spriel <arend.vanspriel@broadcom.com> writes: >>> 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. BTW, last year we were discussing something similar (I mean related to alignment requirements) with ath10k SDIO patches and at the time the patch submitter was proposing to have a bounce buffer in ath10k to workaround that. I don't remember the details anymore, they are on the ath10k mailing list archive if anyone is curious to know, but I would not be surprised if they are similar as here. So there might be a need to solve this in a generic way (but not sure of course as I haven't checked the details).
On 20 March 2018 at 10:55, Kalle Valo <kvalo@codeaurora.org> wrote: > Arend van Spriel <arend.vanspriel@broadcom.com> writes: > >>>> 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. > > BTW, last year we were discussing something similar (I mean related to > alignment requirements) with ath10k SDIO patches and at the time the > patch submitter was proposing to have a bounce buffer in ath10k to > workaround that. I don't remember the details anymore, they are on the > ath10k mailing list archive if anyone is curious to know, but I would > not be surprised if they are similar as here. So there might be a need > to solve this in a generic way (but not sure of course as I haven't > checked the details). I re-call something about these as well, here are the patches. Perhaps I should pick some of them up... https://patchwork.kernel.org/patch/10123137/ https://patchwork.kernel.org/patch/10123139/ https://patchwork.kernel.org/patch/10123141/ https://patchwork.kernel.org/patch/10123143/ Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson <ulf.hansson@linaro.org> writes: > On 20 March 2018 at 10:55, Kalle Valo <kvalo@codeaurora.org> wrote: >> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >> >>>>> 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. >> >> BTW, last year we were discussing something similar (I mean related to >> alignment requirements) with ath10k SDIO patches and at the time the >> patch submitter was proposing to have a bounce buffer in ath10k to >> workaround that. I don't remember the details anymore, they are on the >> ath10k mailing list archive if anyone is curious to know, but I would >> not be surprised if they are similar as here. So there might be a need >> to solve this in a generic way (but not sure of course as I haven't >> checked the details). > > I re-call something about these as well, here are the patches. Perhaps > I should pick some of them up... > > https://patchwork.kernel.org/patch/10123137/ > https://patchwork.kernel.org/patch/10123139/ > https://patchwork.kernel.org/patch/10123141/ > https://patchwork.kernel.org/patch/10123143/ Actually I was talking about a different patch, found it now: ath10k_sdio: DMA bounce buffers for read write https://patchwork.kernel.org/patch/9979543/
On 5 April 2018 at 15:10, Kalle Valo <kvalo@codeaurora.org> wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> On 20 March 2018 at 10:55, Kalle Valo <kvalo@codeaurora.org> wrote: >>> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >>> >>>>>> 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. >>> >>> BTW, last year we were discussing something similar (I mean related to >>> alignment requirements) with ath10k SDIO patches and at the time the >>> patch submitter was proposing to have a bounce buffer in ath10k to >>> workaround that. I don't remember the details anymore, they are on the >>> ath10k mailing list archive if anyone is curious to know, but I would >>> not be surprised if they are similar as here. So there might be a need >>> to solve this in a generic way (but not sure of course as I haven't >>> checked the details). >> >> I re-call something about these as well, here are the patches. Perhaps >> I should pick some of them up... >> >> https://patchwork.kernel.org/patch/10123137/ >> https://patchwork.kernel.org/patch/10123139/ >> https://patchwork.kernel.org/patch/10123141/ >> https://patchwork.kernel.org/patch/10123143/ > > Actually I was talking about a different patch, found it now: > > ath10k_sdio: DMA bounce buffers for read write > > https://patchwork.kernel.org/patch/9979543/ Ah, yes. This is about buffer alignment, particularly when using DMA. Normally there should be no constraint on the alignment, if the mmc/sdio controller driver would implement a fallback mechanism from DMA to PIO mode, in case the buffer can't be used for DMA. However, I know about cases where simply PIO doesn't work because of broken HW and in many cases the mmc drivers don't implement the fallback to PIO even if they could. Moreover, it seems reasonable to anyway have a way for mmc driver to inform upper layers about DMA buffer alignment constraints, as to be able to use DMA as long as possible. Thoughts? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/5/2018 3:10 PM, Kalle Valo wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> On 20 March 2018 at 10:55, Kalle Valo <kvalo@codeaurora.org> wrote: >>> Arend van Spriel <arend.vanspriel@broadcom.com> writes: >>> >>>>>> 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. >>> >>> BTW, last year we were discussing something similar (I mean related to >>> alignment requirements) with ath10k SDIO patches and at the time the >>> patch submitter was proposing to have a bounce buffer in ath10k to >>> workaround that. I don't remember the details anymore, they are on the >>> ath10k mailing list archive if anyone is curious to know, but I would >>> not be surprised if they are similar as here. So there might be a need >>> to solve this in a generic way (but not sure of course as I haven't >>> checked the details). >> >> I re-call something about these as well, here are the patches. Perhaps >> I should pick some of them up... >> >> https://patchwork.kernel.org/patch/10123137/ >> https://patchwork.kernel.org/patch/10123139/ >> https://patchwork.kernel.org/patch/10123141/ >> https://patchwork.kernel.org/patch/10123143/ > > Actually I was talking about a different patch, found it now: > > ath10k_sdio: DMA bounce buffers for read write > > https://patchwork.kernel.org/patch/9979543/ The patches Uffe mentions are related to this. In brcmfmac we simply implemented functionality to create a scatter list and send it through the MMC stack using SDIO CMD53. It is in the creation of the scatter list that these alignment properties are needed. Moving the whole functionality to the MMC stack will also move those properties into their right context, ie. SDIO host controller. Our broker_sg_support property is basically doing the bounce buffer thing if I understand it correctly. Regards, Arend -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Example:
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(+)