[PATCH net-next v2 0/2] brcmfmac: add new dt entries for SG SDIO settings

Changes in v2: don't check of_property_read_* return values since it
doesn't change the value if property not found.
Suggested by Andrew Lunn.

There are 3 fields in SDIO settings (quirks) to workaround some of
the SG SDIO host particularities, i.e higher align requirements for
SG items.
All coding is done the long time ago, but there is no way to change the
driver behavior without patching the kernel.
Add missing devicetree entries.

Signed-off-by: Alexey Roslyakov <redacted>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index aee6e5937c41..14135752b659 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c

@@ -30,14 +30,20 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	struct device_node *np = dev->of_node;
 	int irq;
 	u32 irqf;
-	u32 val;
 
 	if (!np || bus_type != BRCMF_BUSTYPE_SDIO ||
 	    !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
 		return;
 
-	if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
-		sdio->drive_strength = val;
+	of_property_read_u32(np, "brcm,drive-strength", &sdio->drive_strength);
+
+	sdio->broken_sg_support =
+		of_property_read_bool(np, "brcm,broken-sg-support");
+
+	of_property_read_u16(np, "brcm,sd-head-align", &sdio->sd_head_align);
+
+	of_property_read_u16(np, "brcm,sd-sgentry-align",
+			     &sdio->sd_sgentry_align);
 
 	/* make sure there are interrupts defined in the node */
 	if (!of_find_property(np, "interrupts", NULL))

-- 
2.16.1

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 <redacted>
---
 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.
 
 Example:
 

-- 
2.16.1

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

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

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
[off-list ref] wrote:

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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

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

quoted

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
[off-list ref] wrote:

quoted

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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

-- 
Florian

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:

On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:

quoted

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.

quoted

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.

quoted

quoted

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

quoted

All the best,
   Alex.

On 19 March 2018 at 16:31, Arend van Spriel
[off-list ref] wrote:

quoted

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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 <redacted>
---
   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,
I use RK3288-firefly, bcm4339 (ap6335).

Regards,
  Alex

On 20 March 2018 at 06:16, Arend van Spriel
[off-list ref] wrote:

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:

quoted

On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

All the best,
   Alex.

On 19 March 2018 at 16:31, Arend van Spriel
[off-list ref] wrote:

quoted

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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

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
[off-list ref] wrote:

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:

quoted

On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

All the best,
   Alex.

On 19 March 2018 at 16:31, Arend van Spriel
[off-list ref] wrote:

quoted

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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

On 3/20/2018 8:58 AM, Alexey Roslyakov wrote:

Arend,

quoted

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
[off-list ref] wrote:

quoted

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:

quoted

On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

All the best,
    Alex.

On 19 March 2018 at 16:31, Arend van Spriel
[off-list ref] wrote:

quoted

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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

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
[off-list ref] wrote:

On 3/20/2018 8:58 AM, Alexey Roslyakov wrote:

quoted

Arend,

quoted

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

quoted

Regards,
   Alex

On 20 March 2018 at 06:16, Arend van Spriel
[off-list ref] wrote:

quoted

+ Uffe

On 3/19/2018 6:55 PM, Florian Fainelli wrote:

quoted

On 03/19/2018 07:10 AM, Alexey Roslyakov wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

All the best,
    Alex.

On 19 March 2018 at 16:31, Arend van Spriel
[off-list ref] wrote:

quoted

On 3/19/2018 2:40 AM, Alexey Roslyakov wrote:

quoted

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

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