[PATCH 0/3] brcmfmac: sdio: Deal better w/ transmission errors waking from sleep

[PATCH 0/3] brcmfmac: sdio: Deal better w/ transmission errors waking from sleep

[Douglas Anderson]

This series attempts to deal better with the expected transmission
errors that we get when waking up the SDIO-based WiFi on
rk3288-veyron-minnie, rk3288-veyron-speedy, and rk3288-veyron-mickey.

Some details about those errors can be found in
<https://crbug.com/960222>, but to summarize it here: if we try to
send the wakeup command to the WiFi card at the same time it has
decided to wake up itself then it will behave badly on the SDIO bus.
This can cause timeouts or CRC errors.

When I tested on 4.19 and 4.20 these CRC errors can be seen to cause
re-tuning.  Since I am currently developing on 4.19 this was the
original problem I attempted to solve.

On mainline it turns out that you don't see the retuning errors but
you see tons of spam about timeouts trying to wakeup from sleep.  I
tracked down the commit that was causing that and have partially
reverted it here.  I have no real knowledge about Broadcom WiFi, but
the commit that was causing problems sounds (from the descriptioin) to
be a hack commit penalizing all Broadcom WiFi users because of a bug
in a Cypress SD controller.  I will let others comment if this is
truly the case and, if so, what the right solution should be.


Douglas Anderson (3):
  brcmfmac: re-enable command decode in sdio_aos for BRCM 4354
  mmc: core: API for temporarily disabling auto-retuning due to errors
  brcmfmac: sdio: Disable auto-tuning around commands expected to fail

 drivers/mmc/core/core.c                       | 27 +++++++++++++++++--
 .../broadcom/brcm80211/brcmfmac/sdio.c        |  6 +++--
 include/linux/mmc/core.h                      |  2 ++
 include/linux/mmc/host.h                      |  1 +
 4 files changed, 32 insertions(+), 4 deletions(-)

-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Douglas Anderson]

In commit 29f6589140a1 ("brcmfmac: disable command decode in
sdio_aos") we disabled something called "command decode in sdio_aos"
for a whole bunch of Broadcom SDIO WiFi parts.

After that patch landed I find that my kernel log on
rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
  brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110

This seems to happen every time the Broadcom WiFi transitions out of
sleep mode.  Reverting the part of the commit that affects the WiFi on
my boards fixes the problem for me, so that's what this patch does.

Note that, in general, the justification in the original commit seemed
a little weak.  It looked like someone was testing on a SD card
controller that would sometimes die if there were CRC errors on the
bus.  This used to happen back in early days of dw_mmc (the controller
on my boards), but we fixed it.  Disabling a feature on all boards
just because one SD card controller is broken seems bad.  ...so
instead of just this patch possibly the right thing to do is to fully
revert the original commit.

Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 22b73da42822..3fd2d58a3c88 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3378,8 +3378,7 @@ static bool brcmf_sdio_aos_no_decode(struct brcmf_sdio *bus)
 	if (bus->ci->chip == CY_CC_43012_CHIP_ID ||
 	    bus->ci->chip == CY_CC_4373_CHIP_ID ||
 	    bus->ci->chip == BRCM_CC_4339_CHIP_ID ||
-	    bus->ci->chip == BRCM_CC_4345_CHIP_ID ||
-	    bus->ci->chip == BRCM_CC_4354_CHIP_ID)
+	    bus->ci->chip == BRCM_CC_4345_CHIP_ID)
 		return true;
 	else
 		return false;
-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH 3/3] brcmfmac: sdio: Disable auto-tuning around commands expected to fail

[Douglas Anderson]

There are certain cases, notably when transitioning between sleep and
active state, when Broadcom SDIO WiFi cards will produce errors on the
SDIO bus.  This is evident from the source code where you can see that
we try commands in a loop until we either get success or we've tried
too many times.  The comment in the code reinforces this by saying
"just one write attempt may fail"

Unfortunately these failures sometimes end up causing an "-EILSEQ"
back to the core which triggers a retuning of the SDIO card and that
blocks all traffic to the card until it's done.

Let's disable retuning around the commands we expect might fail.

Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 3fd2d58a3c88..c09bb8965487 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -27,6 +27,7 @@
 #include <linux/mmc/sdio_ids.h>
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/core.h>
 #include <linux/semaphore.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
@@ -708,6 +709,7 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
 		bmask = SBSDIO_FUNC1_SLEEPCSR_KSO_MASK;
 	}
 
+	mmc_expect_errors_begin(bus->sdiodev->func1->card->host);
 	do {
 		/* reliable KSO bit set/clr:
 		 * the sdiod sleep write access is synced to PMU 32khz clk
@@ -730,6 +732,7 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
 				   &err);
 
 	} while (try_cnt++ < MAX_KSO_ATTEMPTS);
+	mmc_expect_errors_end(bus->sdiodev->func1->card->host);
 
 	if (try_cnt > 2)
 		brcmf_dbg(SDIO, "try_cnt=%d rd_val=0x%x err=%d\n", try_cnt,
-- 
2.21.0.1020.gf2820cf01a-goog

RE: [PATCH 0/3] brcmfmac: sdio: Deal better w/ transmission errors waking from sleep

[Avri Altman]

> 
> This series attempts to deal better with the expected transmission
> errors that we get when waking up the SDIO-based WiFi on
> rk3288-veyron-minnie, rk3288-veyron-speedy, and rk3288-veyron-mickey.
> 
> Some details about those errors can be found in
> <https://crbug.com/960222>, but to summarize it here: if we try to
> send the wakeup command to the WiFi card at the same time it has
> decided to wake up itself then it will behave badly on the SDIO bus.
> This can cause timeouts or CRC errors.
Wake-up itself: as part of a WoWlan, or d0i3?
Looks like this calls for a wifi driver fix, and not WA in the mmc driver.

Thanks,
Avri

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Arend Van Spriel]

On 5/18/2019 12:54 AM, Douglas Anderson wrote:
> In commit 29f6589140a1 ("brcmfmac: disable command decode in
> sdio_aos") we disabled something called "command decode in sdio_aos"
> for a whole bunch of Broadcom SDIO WiFi parts.
> 
> After that patch landed I find that my kernel log on
> rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>    brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
> 
> This seems to happen every time the Broadcom WiFi transitions out of
> sleep mode.  Reverting the part of the commit that affects the WiFi on
> my boards fixes the problem for me, so that's what this patch does.

This sounds very similar to the issue we had during integration of wifi 
on rk3288 chromebooks years ago.

> Note that, in general, the justification in the original commit seemed
> a little weak.  It looked like someone was testing on a SD card
> controller that would sometimes die if there were CRC errors on the
> bus.  This used to happen back in early days of dw_mmc (the controller
> on my boards), but we fixed it.  Disabling a feature on all boards
> just because one SD card controller is broken seems bad.  ...so
> instead of just this patch possibly the right thing to do is to fully
> revert the original commit.

I am leaning towards a full revert, but let's wait for more background info.

Regards,
Arend

Re: [PATCH 0/3] brcmfmac: sdio: Deal better w/ transmission errors waking from sleep

[Arend Van Spriel]

On 5/18/2019 12:54 AM, Douglas Anderson wrote:
> This series attempts to deal better with the expected transmission
> errors that we get when waking up the SDIO-based WiFi on
> rk3288-veyron-minnie, rk3288-veyron-speedy, and rk3288-veyron-mickey.
> 
> Some details about those errors can be found in
> <https://crbug.com/960222>, but to summarize it here: if we try to
> send the wakeup command to the WiFi card at the same time it has
> decided to wake up itself then it will behave badly on the SDIO bus.
> This can cause timeouts or CRC errors.
> 
> When I tested on 4.19 and 4.20 these CRC errors can be seen to cause
> re-tuning.  Since I am currently developing on 4.19 this was the
> original problem I attempted to solve.
> 
> On mainline it turns out that you don't see the retuning errors but
> you see tons of spam about timeouts trying to wakeup from sleep.  I
> tracked down the commit that was causing that and have partially
> reverted it here.  I have no real knowledge about Broadcom WiFi, but
> the commit that was causing problems sounds (from the descriptioin) to
> be a hack commit penalizing all Broadcom WiFi users because of a bug
> in a Cypress SD controller.  I will let others comment if this is
> truly the case and, if so, what the right solution should be.

Let me give a bit of background. The brcmfmac driver implements its own 
runtime-pm like functionality, ie. if the driver is idle for some time 
it will put the device in a low-power state. When it does that it powers 
down several cores in the chip among which the SDIO core. However, the 
SDIO bus used be very bad at handling devices that do that so instead it 
has the Always-On-Station (AOS) block take over the SDIO core in 
handling the bus. Default is will send a R1 response, but only for CMD52 
(and CMD14 but no host is using that cruft). In noCmdDecode it does not 
respond and simply wakes up the SDIO core, which takes over again. 
Because it does not respond timeouts (-110) are kinda expected in this mode.

Regards,
Arend

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Doug Anderson]

Hi,

On Mon, May 20, 2019 at 1:09 AM Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 5/18/2019 12:54 AM, Douglas Anderson wrote:
> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
> > sdio_aos") we disabled something called "command decode in sdio_aos"
> > for a whole bunch of Broadcom SDIO WiFi parts.
> >
> > After that patch landed I find that my kernel log on
> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
> >    brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
> >
> > This seems to happen every time the Broadcom WiFi transitions out of
> > sleep mode.  Reverting the part of the commit that affects the WiFi on
> > my boards fixes the problem for me, so that's what this patch does.
>
> This sounds very similar to the issue we had during integration of wifi
> on rk3288 chromebooks years ago.

I'm working on those same Chromebooks.  ;-)  I'm working on trying to
make them well on newer kernels.

...but I guess you're saying that the problem faced by the people who
wanted commit 29f6589140a1 ("brcmfmac: disable command decode in
sdio_aos") are similar to the problems we saw in the past on those
Chromebooks.  I'd tend to agree.  In general it's difficult to get a
SD Host Controller to be fully robust in the fact of any/all errors on
the bus.  While dw_mmc is pretty robust these days I'm assuming that
some other host controllers aren't.


> > Note that, in general, the justification in the original commit seemed
> > a little weak.  It looked like someone was testing on a SD card
> > controller that would sometimes die if there were CRC errors on the
> > bus.  This used to happen back in early days of dw_mmc (the controller
> > on my boards), but we fixed it.  Disabling a feature on all boards
> > just because one SD card controller is broken seems bad.  ...so
> > instead of just this patch possibly the right thing to do is to fully
> > revert the original commit.
>
> I am leaning towards a full revert, but let's wait for more background info.

I'd be fine with a full revert too.  Presumably that will break
someone but maybe they need to come up with a better solution?

-Doug

Re: [PATCH 0/3] brcmfmac: sdio: Deal better w/ transmission errors waking from sleep

[Brian Norris]

On Sat, May 18, 2019 at 03:09:44PM +0000, Avri Altman wrote:
> > 
> > This series attempts to deal better with the expected transmission
> > errors that we get when waking up the SDIO-based WiFi on
> > rk3288-veyron-minnie, rk3288-veyron-speedy, and rk3288-veyron-mickey.
> > 
> > Some details about those errors can be found in
> > <https://crbug.com/960222>, but to summarize it here: if we try to
> > send the wakeup command to the WiFi card at the same time it has
> > decided to wake up itself then it will behave badly on the SDIO bus.
> > This can cause timeouts or CRC errors.
> Wake-up itself: as part of a WoWlan, or d0i3?

Neither, IIUC. (It's definitely not WoWLAN, and D0i3 sounds like an
Intel thing.)

I believe it's a Broadcom-specific mode. See also Arend's response to
this thread:

http://lkml.kernel.org/linux-wireless/8c3fa57a-3843-947c-ec6b-a6144ccde1e9@broadcom.com

> Looks like this calls for a wifi driver fix, and not WA in the mmc driver.

Basically asked and answered in patch 2's thread:

https://lkml.kernel.org/lkml/20190520085201.GA1021@kunai/

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Kalle Valo]

Douglas Anderson <dianders@chromium.org> wrote:

> In commit 29f6589140a1 ("brcmfmac: disable command decode in
> sdio_aos") we disabled something called "command decode in sdio_aos"
> for a whole bunch of Broadcom SDIO WiFi parts.
> 
> After that patch landed I find that my kernel log on
> rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
> 
> This seems to happen every time the Broadcom WiFi transitions out of
> sleep mode.  Reverting the part of the commit that affects the WiFi on
> my boards fixes the problem for me, so that's what this patch does.
> 
> Note that, in general, the justification in the original commit seemed
> a little weak.  It looked like someone was testing on a SD card
> controller that would sometimes die if there were CRC errors on the
> bus.  This used to happen back in early days of dw_mmc (the controller
> on my boards), but we fixed it.  Disabling a feature on all boards
> just because one SD card controller is broken seems bad.  ...so
> instead of just this patch possibly the right thing to do is to fully
> revert the original commit.
> 
> Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

I don't see patch 2 in patchwork and I assume discussion continues.
Please resend if/when I need to apply something.

2 patches set to Changes Requested.

10948785 [1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354
10948777 [3/3] brcmfmac: sdio: Disable auto-tuning around commands expected to fail

-- 
https://patchwork.kernel.org/patch/10948785/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Doug Anderson]

Hi,

On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Douglas Anderson <dianders@chromium.org> wrote:
>
> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
> > sdio_aos") we disabled something called "command decode in sdio_aos"
> > for a whole bunch of Broadcom SDIO WiFi parts.
> >
> > After that patch landed I find that my kernel log on
> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
> >
> > This seems to happen every time the Broadcom WiFi transitions out of
> > sleep mode.  Reverting the part of the commit that affects the WiFi on
> > my boards fixes the problem for me, so that's what this patch does.
> >
> > Note that, in general, the justification in the original commit seemed
> > a little weak.  It looked like someone was testing on a SD card
> > controller that would sometimes die if there were CRC errors on the
> > bus.  This used to happen back in early days of dw_mmc (the controller
> > on my boards), but we fixed it.  Disabling a feature on all boards
> > just because one SD card controller is broken seems bad.  ...so
> > instead of just this patch possibly the right thing to do is to fully
> > revert the original commit.
> >
> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> I don't see patch 2 in patchwork and I assume discussion continues.

Apologies.  I made sure to CC you individually on all the patches but
didn't think about the fact that you use patchwork to manage and so
didn't ensure all patches made it to all lists (by default each patch
gets recipients individually from get_maintainer).  I'll make sure to
fix for patch set #2.  If you want to see all the patches, you can at
least find them on lore.kernel.org linked from the cover:

https://lore.kernel.org/patchwork/cover/1075373/


> Please resend if/when I need to apply something.
>
> 2 patches set to Changes Requested.
>
> 10948785 [1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

As per Arend I'll change patch #1 to a full revert instead of a
partial revert.  Arend: please yell if you want otherwise.

-Doug

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Arend Van Spriel]

On May 28, 2019 5:52:10 PM Doug Anderson <dianders@chromium.org> wrote:

> Hi,
>
> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Douglas Anderson <dianders@chromium.org> wrote:
>>
>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>> > for a whole bunch of Broadcom SDIO WiFi parts.
>> >
>> > After that patch landed I find that my kernel log on
>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
>> >
>> > This seems to happen every time the Broadcom WiFi transitions out of
>> > sleep mode.  Reverting the part of the commit that affects the WiFi on
>> > my boards fixes the problem for me, so that's what this patch does.
>> >
>> > Note that, in general, the justification in the original commit seemed
>> > a little weak.  It looked like someone was testing on a SD card
>> > controller that would sometimes die if there were CRC errors on the
>> > bus.  This used to happen back in early days of dw_mmc (the controller
>> > on my boards), but we fixed it.  Disabling a feature on all boards
>> > just because one SD card controller is broken seems bad.  ...so
>> > instead of just this patch possibly the right thing to do is to fully
>> > revert the original commit.
>> >
>> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>
>> I don't see patch 2 in patchwork and I assume discussion continues.
>
> Apologies.  I made sure to CC you individually on all the patches but
> didn't think about the fact that you use patchwork to manage and so
> didn't ensure all patches made it to all lists (by default each patch
> gets recipients individually from get_maintainer).  I'll make sure to
> fix for patch set #2.  If you want to see all the patches, you can at
> least find them on lore.kernel.org linked from the cover:
>
> https://lore.kernel.org/patchwork/cover/1075373/
>
>
>> Please resend if/when I need to apply something.
>>
>> 2 patches set to Changes Requested.
>>
>> 10948785 [1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354
>
> As per Arend I'll change patch #1 to a full revert instead of a
> partial revert.  Arend: please yell if you want otherwise.

No yelling here. If any it is expected from Cypress. Maybe good to add them 
specifically in Cc:

Regards,
Arend

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Arend Van Spriel]

On May 28, 2019 6:09:21 PM Arend Van Spriel <arend.vanspriel@broadcom.com> 
wrote:

> On May 28, 2019 5:52:10 PM Doug Anderson <dianders@chromium.org> wrote:
>
>> Hi,
>>
>> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>>
>>> Douglas Anderson <dianders@chromium.org> wrote:
>>>
>>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>>> > for a whole bunch of Broadcom SDIO WiFi parts.
>>> >
>>> > After that patch landed I find that my kernel log on
>>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>>> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
>>> >
>>> > This seems to happen every time the Broadcom WiFi transitions out of
>>> > sleep mode.  Reverting the part of the commit that affects the WiFi on
>>> > my boards fixes the problem for me, so that's what this patch does.
>>> >
>>> > Note that, in general, the justification in the original commit seemed
>>> > a little weak.  It looked like someone was testing on a SD card
>>> > controller that would sometimes die if there were CRC errors on the
>>> > bus.  This used to happen back in early days of dw_mmc (the controller
>>> > on my boards), but we fixed it.  Disabling a feature on all boards
>>> > just because one SD card controller is broken seems bad.  ...so
>>> > instead of just this patch possibly the right thing to do is to fully
>>> > revert the original commit.
>>> >
>>> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
>>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>
>>> I don't see patch 2 in patchwork and I assume discussion continues.
>>
>> Apologies.  I made sure to CC you individually on all the patches but
>> didn't think about the fact that you use patchwork to manage and so
>> didn't ensure all patches made it to all lists (by default each patch
>> gets recipients individually from get_maintainer).  I'll make sure to
>> fix for patch set #2.  If you want to see all the patches, you can at
>> least find them on lore.kernel.org linked from the cover:
>>
>> https://lore.kernel.org/patchwork/cover/1075373/
>>
>>
>>> Please resend if/when I need to apply something.
>>>
>>> 2 patches set to Changes Requested.
>>>
>>> 10948785 [1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354
>>
>> As per Arend I'll change patch #1 to a full revert instead of a
>> partial revert.  Arend: please yell if you want otherwise.
>
> No yelling here. If any it is expected from Cypress. Maybe good to add them
> specifically in Cc:

Of the revert patch that is.

Gr. AvS

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Kalle Valo]

Doug Anderson <dianders@chromium.org> writes:

> Hi,
>
> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Douglas Anderson <dianders@chromium.org> wrote:
>>
>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>> > for a whole bunch of Broadcom SDIO WiFi parts.
>> >
>> > After that patch landed I find that my kernel log on
>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
>> >
>> > This seems to happen every time the Broadcom WiFi transitions out of
>> > sleep mode.  Reverting the part of the commit that affects the WiFi on
>> > my boards fixes the problem for me, so that's what this patch does.
>> >
>> > Note that, in general, the justification in the original commit seemed
>> > a little weak.  It looked like someone was testing on a SD card
>> > controller that would sometimes die if there were CRC errors on the
>> > bus.  This used to happen back in early days of dw_mmc (the controller
>> > on my boards), but we fixed it.  Disabling a feature on all boards
>> > just because one SD card controller is broken seems bad.  ...so
>> > instead of just this patch possibly the right thing to do is to fully
>> > revert the original commit.
>> >
>> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>
>> I don't see patch 2 in patchwork and I assume discussion continues.
>
> Apologies.  I made sure to CC you individually on all the patches but
> didn't think about the fact that you use patchwork to manage and so
> didn't ensure all patches made it to all lists (by default each patch
> gets recipients individually from get_maintainer).  I'll make sure to
> fix for patch set #2.  If you want to see all the patches, you can at
> least find them on lore.kernel.org linked from the cover:
>
> https://lore.kernel.org/patchwork/cover/1075373/

No worries, I had the thread on my email but was just too busy to check.
So I instead wrote down my thought process so that somebode can correct
me in case I have misunderstood. I usually do that when it's not clear
what the next action should be.

-- 
Kalle Valo

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Wright Feng]

On 2019/5/29 上午 12:11, Arend Van Spriel wrote:
> On May 28, 2019 6:09:21 PM Arend Van Spriel 
> <arend.vanspriel@broadcom.com> wrote:
> 
>> On May 28, 2019 5:52:10 PM Doug Anderson <dianders@chromium.org> wrote:
>>
>>> Hi,
>>>
>>> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>>>
>>>> Douglas Anderson <dianders@chromium.org> wrote:
>>>>
>>>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>>>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>>>> > for a whole bunch of Broadcom SDIO WiFi parts.
>>>> >
>>>> > After that patch landed I find that my kernel log on
>>>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>>>> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep 
>>>> state -110
>>>> >
>>>> > This seems to happen every time the Broadcom WiFi transitions out of
>>>> > sleep mode.  Reverting the part of the commit that affects the 
>>>> WiFi on
>>>> > my boards fixes the problem for me, so that's what this patch does.
>>>> >
>>>> > Note that, in general, the justification in the original commit 
>>>> seemed
>>>> > a little weak.  It looked like someone was testing on a SD card
>>>> > controller that would sometimes die if there were CRC errors on the
>>>> > bus.  This used to happen back in early days of dw_mmc (the 
>>>> controller
>>>> > on my boards), but we fixed it.  Disabling a feature on all boards
>>>> > just because one SD card controller is broken seems bad.  ...so
>>>> > instead of just this patch possibly the right thing to do is to fully
>>>> > revert the original commit.
>>>> >
Since the commit 29f6589140a1 ("brcmfmac: disable command decode in 
sdio_aos") causes the regression on other SD card controller, it is 
better to revert it as you mentioned.
Actually, without the commit, we hit MMC timeout(-110) and hanged 
instead of CRC error in our test. Would you please share the analysis of 
dw_mmc issue which you fixed? We'd like to compare whether we got the 
same issue.

Regards,
Wright

>>>> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
>>>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>
>>>> I don't see patch 2 in patchwork and I assume discussion continues.
>>>
>>> Apologies.  I made sure to CC you individually on all the patches but
>>> didn't think about the fact that you use patchwork to manage and so
>>> didn't ensure all patches made it to all lists (by default each patch
>>> gets recipients individually from get_maintainer).  I'll make sure to
>>> fix for patch set #2.  If you want to see all the patches, you can at
>>> least find them on lore.kernel.org linked from the cover:
>>>
>>> https://lore.kernel.org/patchwork/cover/1075373/
>>>
>>>
>>>> Please resend if/when I need to apply something.
>>>>
>>>> 2 patches set to Changes Requested.
>>>>
>>>> 10948785 [1/3] brcmfmac: re-enable command decode in sdio_aos for 
>>>> BRCM 4354
>>>
>>> As per Arend I'll change patch #1 to a full revert instead of a
>>> partial revert.  Arend: please yell if you want otherwise.
>>
>> No yelling here. If any it is expected from Cypress. Maybe good to add 
>> them
>> specifically in Cc:
> 
> Of the revert patch that is.
> 
> Gr. AvS
> 
> 

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Doug Anderson]

Hi,

On Mon, Jun 3, 2019 at 8:20 PM Wright Feng <Wright.Feng@cypress.com> wrote:
>
> On 2019/5/29 上午 12:11, Arend Van Spriel wrote:
> > On May 28, 2019 6:09:21 PM Arend Van Spriel
> > <arend.vanspriel@broadcom.com> wrote:
> >
> >> On May 28, 2019 5:52:10 PM Doug Anderson <dianders@chromium.org> wrote:
> >>
> >>> Hi,
> >>>
> >>> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> >>>>
> >>>> Douglas Anderson <dianders@chromium.org> wrote:
> >>>>
> >>>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
> >>>> > sdio_aos") we disabled something called "command decode in sdio_aos"
> >>>> > for a whole bunch of Broadcom SDIO WiFi parts.
> >>>> >
> >>>> > After that patch landed I find that my kernel log on
> >>>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
> >>>> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep
> >>>> state -110
> >>>> >
> >>>> > This seems to happen every time the Broadcom WiFi transitions out of
> >>>> > sleep mode.  Reverting the part of the commit that affects the
> >>>> WiFi on
> >>>> > my boards fixes the problem for me, so that's what this patch does.
> >>>> >
> >>>> > Note that, in general, the justification in the original commit
> >>>> seemed
> >>>> > a little weak.  It looked like someone was testing on a SD card
> >>>> > controller that would sometimes die if there were CRC errors on the
> >>>> > bus.  This used to happen back in early days of dw_mmc (the
> >>>> controller
> >>>> > on my boards), but we fixed it.  Disabling a feature on all boards
> >>>> > just because one SD card controller is broken seems bad.  ...so
> >>>> > instead of just this patch possibly the right thing to do is to fully
> >>>> > revert the original commit.
> >>>> >
> Since the commit 29f6589140a1 ("brcmfmac: disable command decode in
> sdio_aos") causes the regression on other SD card controller, it is
> better to revert it as you mentioned.
> Actually, without the commit, we hit MMC timeout(-110) and hanged
> instead of CRC error in our test.

Any chance I can convince you to provide some official tags like
Reviewed-by or Tested-by on the revert?

> Would you please share the analysis of
> dw_mmc issue which you fixed? We'd like to compare whether we got the
> same issue.

I'm not sure there's any single magic commit I can point to.  When I
started working on dw_mmc it was terrible at handling error cases and
would often crash / hang / stop all future communication upon certain
classes or efforts.  There were dozens of problems we've had to fix
over the years.  These problems showed up when we started supporting
HS200 / UHS since the tuning phase really stress the error handling of
the host controller.

I searched and by the time we were supporting Broadcom SDIO cards the
error handling was already pretty good.  ...but if we hadn't already
made the error handling more robust for UHS tuning then we would have
definitely hit these types of problems.  The only problem I remember
having to solve in dw_mmc that was unique to Broadcom was commit
0bdbd0e88cf6 ("mmc: dw_mmc: Don't start commands while busy").  Any
chance that could be what you're hitting?

What host controller are you having problems with?

-Doug

Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354

[Arend Van Spriel]

On June 4, 2019 6:01:26 PM Doug Anderson <dianders@chromium.org> wrote:

> Hi,
>
> On Mon, Jun 3, 2019 at 8:20 PM Wright Feng <Wright.Feng@cypress.com> wrote:
>>
>> On 2019/5/29 上午 12:11, Arend Van Spriel wrote:
>> > On May 28, 2019 6:09:21 PM Arend Van Spriel
>> > <arend.vanspriel@broadcom.com> wrote:
>> >
>> >> On May 28, 2019 5:52:10 PM Doug Anderson <dianders@chromium.org> wrote:
>> >>
>> >>> Hi,
>> >>>
>> >>> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>> >>>>
>> >>>> Douglas Anderson <dianders@chromium.org> wrote:
>> >>>>
>> >>>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>> >>>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>> >>>> > for a whole bunch of Broadcom SDIO WiFi parts.
>> >>>> >
>> >>>> > After that patch landed I find that my kernel log on
>> >>>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>> >>>> >   brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep
>> >>>> state -110
>> >>>> >
>> >>>> > This seems to happen every time the Broadcom WiFi transitions out of
>> >>>> > sleep mode.  Reverting the part of the commit that affects the
>> >>>> WiFi on
>> >>>> > my boards fixes the problem for me, so that's what this patch does.
>> >>>> >
>> >>>> > Note that, in general, the justification in the original commit
>> >>>> seemed
>> >>>> > a little weak.  It looked like someone was testing on a SD card
>> >>>> > controller that would sometimes die if there were CRC errors on the
>> >>>> > bus.  This used to happen back in early days of dw_mmc (the
>> >>>> controller
>> >>>> > on my boards), but we fixed it.  Disabling a feature on all boards
>> >>>> > just because one SD card controller is broken seems bad.  ...so
>> >>>> > instead of just this patch possibly the right thing to do is to fully
>> >>>> > revert the original commit.
>> >>>> >
>> Since the commit 29f6589140a1 ("brcmfmac: disable command decode in
>> sdio_aos") causes the regression on other SD card controller, it is
>> better to revert it as you mentioned.
>> Actually, without the commit, we hit MMC timeout(-110) and hanged
>> instead of CRC error in our test.
>
> Any chance I can convince you to provide some official tags like
> Reviewed-by or Tested-by on the revert?
>
>> Would you please share the analysis of
>> dw_mmc issue which you fixed? We'd like to compare whether we got the
>> same issue.
>
> I'm not sure there's any single magic commit I can point to.  When I
> started working on dw_mmc it was terrible at handling error cases and
> would often crash / hang / stop all future communication upon certain
> classes or efforts.  There were dozens of problems we've had to fix
> over the years.  These problems showed up when we started supporting
> HS200 / UHS since the tuning phase really stress the error handling of
> the host controller.
>
> I searched and by the time we were supporting Broadcom SDIO cards the
> error handling was already pretty good.  ...but if we hadn't already
> made the error handling more robust for UHS tuning then we would have
> definitely hit these types of problems.  The only problem I remember
> having to solve in dw_mmc that was unique to Broadcom was commit
> 0bdbd0e88cf6 ("mmc: dw_mmc: Don't start commands while busy").  Any
> chance that could be what you're hitting?

That is indeed an issue I recall resulting in -110 errors.

> What host controller are you having problems with?

Knowing that will be a good start.

Regards,
Arend