From: Doug Anderson <dianders@chromium.org> Date: 2019-05-28 15:52:12
Hi,
On Tue, May 28, 2019 at 5:18 AM Kalle Valo [off-list ref] wrote:
Douglas Anderson [off-list ref] wrote:
quoted
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
From: Arend Van Spriel <arend.vanspriel@broadcom.com> Date: 2019-05-28 16:09:24
On May 28, 2019 5:52:10 PM Doug Anderson [off-list ref] wrote:
Hi,
On Tue, May 28, 2019 at 5:18 AM Kalle Valo [off-list ref] wrote:
quoted
Douglas Anderson [off-list ref] wrote:
quoted
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/
quoted
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
From: Arend Van Spriel <arend.vanspriel@broadcom.com> Date: 2019-05-28 16:11:07
On May 28, 2019 6:09:21 PM Arend Van Spriel [off-list ref]
wrote:
On May 28, 2019 5:52:10 PM Doug Anderson [off-list ref] wrote:
quoted
Hi,
On Tue, May 28, 2019 at 5:18 AM Kalle Valo [off-list ref] wrote:
quoted
Douglas Anderson [off-list ref] wrote:
quoted
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/
quoted
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:
On May 28, 2019 6:09:21 PM Arend Van Spriel
[off-list ref] wrote:
quoted
On May 28, 2019 5:52:10 PM Doug Anderson [off-list ref] wrote:
quoted
Hi,
On Tue, May 28, 2019 at 5:18 AM Kalle Valo [off-list ref] wrote:
quoted
Douglas Anderson [off-list ref] wrote:
quoted
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
quoted
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
quoted
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
quoted
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
quoted
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
quoted
quoted
quoted
quoted
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/
quoted
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:
From: Doug Anderson <dianders@chromium.org> Date: 2019-06-04 16:08:28
Hi,
On Mon, Jun 3, 2019 at 8:20 PM Wright Feng [off-list ref] wrote:
On 2019/5/29 上午 12:11, Arend Van Spriel wrote:
quoted
On May 28, 2019 6:09:21 PM Arend Van Spriel
[off-list ref] wrote:
quoted
On May 28, 2019 5:52:10 PM Doug Anderson [off-list ref] wrote:
quoted
Hi,
On Tue, May 28, 2019 at 5:18 AM Kalle Valo [off-list ref] wrote:
quoted
Douglas Anderson [off-list ref] wrote:
quoted
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
quoted
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
quoted
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
quoted
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
quoted
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
From: Arend Van Spriel <arend.vanspriel@broadcom.com> Date: 2019-06-04 16:49:06
On June 4, 2019 6:01:26 PM Doug Anderson [off-list ref] wrote:
Hi,
On Mon, Jun 3, 2019 at 8:20 PM Wright Feng [off-list ref] wrote:
quoted
On 2019/5/29 上午 12:11, Arend Van Spriel wrote:
quoted
On May 28, 2019 6:09:21 PM Arend Van Spriel
[off-list ref] wrote:
quoted
On May 28, 2019 5:52:10 PM Doug Anderson [off-list ref] wrote:
quoted
Hi,
On Tue, May 28, 2019 at 5:18 AM Kalle Valo [off-list ref] wrote:
quoted
Douglas Anderson [off-list ref] wrote:
quoted
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
quoted
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
quoted
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
quoted
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
quoted
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?
quoted
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?