Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug, On 02/20/2015 09:31 PM, Doug Anderson wrote: > We've seen problems on some WiFi modules where we seem to send a CMD53 > (which requires the data lines) while the module is asserting busy. > We shouldn't do that. > > The Designware Databook says that before issuing a new data transfer > command we should check for busy, so that's what we'll do. > > We'll leverage the existing dw_mmc knowledge about whether it should > wait for the previous command to finish to know whether we should > check for busy before sending the command. This means we won't end up > incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which > don't use the data line. > > Note that this also has the advantage of making sure that we don't > change the clock while the card is busy, too. > > Signed-off-by: Doug Anderson > On an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi: Tested-by: Javier Martinez Canillas Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug,
On 02/25/2015 06:43 AM, Doug Anderson wrote:
>
> OK, so looking through things I _think_ I found another difference
> that _might_ matter...
>
Yes it does! when adding the "sd1_bus1" the slot pinctrl I do have
the WiFi module working, thanks a lot for your help!
> Upstream (arch/arm/boot/dts/exynos5420-pinctrl.dtsi):
> sd1_bus1: sd1-bus-width1 {
> samsung,pins = "gpc1-3";
> ...
> };
>
> sd1_bus4: sd1-bus-width4 {
> samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6";
> ...
>};
>
> sd1_bus8: sd1-bus-width8 {
> samsung,pins = "gpd1-4", "gpd1-5", "gpd1-6", "gpd1-7";
> ...
>};
>
> Upsttream (arch/arm/boot/dts/exynos5420-peach-pit.dts) -- your patch:
>pinctrl-0 = <_clk>, <_cmd>, <_int>, <_bus4>,
><_bus8>, <_en>, <_rst>;
>
> Downstream (arch/arm/boot/dts/exynos542x-pinctrl.dtsi):
> sd1_bus1: sd1-bus-width1 {
> samsung,pins = "gpc1-3";
> ...
>};
>
> sd1_bus4: sd1-bus-width4 {
> samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", "gpc1-6";
> ...
>};
>
> sd1_bus8: sd1-bus-width8 {
> samsung,pins = "gpd1-4", "gpd1-5", "gpd1-6", "gpd1-7";
> ...
>};
>
> Downstream (arch/arm/boot/dts/exynos542x-peach.dtsi):
> pinctrl-0 = <_clk _cmd _int _bus4 _bus8>;
>
>
> Notice the difference? You need to add "sd1_bus1" to the pinctrl for
> upstream. The upstream DTS makes more sense. I think I remember
> discussing this in the past (finding the conversation on the lists is
> left as an exercise to the reader) and you can in fact see that the
> upstream 5250 pinctrl file is like the downstream 5420 pinctrl...
>
Yeah, I didn't notice that there was an inconsistency in the pinctrl
defines in mainline around the different SoCs. So for 5250 you need:
* only sd1_bus1 for bus-width = <1>
* only sd1_bus4 for bus-width = <4>
* sd1_bus4 + sd1_bus8 for bus-width = <8>
and for 5440 you need:
* only sd1_bus1 for bus-width = <1>
* only sd1_bus1 + sd1_bus4 for bus-width = <4>
* sd1_bus1 + sd1_bus4 + sd1_bus8 for bus-width = <8>
Is true that 5440 is at least more consistent in the sense that you
have to add all the pins but IMHO this approach is very error prone.
I would preferred if sd1_busN included all pins needed for width N
so you only had to define a single pinctrl group but for now I'll
include "sd1_bus1" to fix the SDIO WiFi issue.
> I think the same bug is present in eMMC and SD but possibly the
> bootloader inits the pinctrl properly there?
>
Correct, I'll fix those too so the kernel does not rely on the boot
loader to setup those pins.
>
> Crossing my fingers that's your bug, but I can't say for sure why
> adding a tons of resets would somehow make it better?
>
It is, thanks a lot again for finding this issue. I feel very dumb for
not realizing there was a difference in the included pinctrl definition.
> -Doug
>
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug, On 02/20/2015 09:31 PM, Doug Anderson wrote: We've seen problems on some WiFi modules where we seem to send a CMD53 (which requires the data lines) while the module is asserting busy. We shouldn't do that. The Designware Databook says that before issuing a new data transfer command we should check for busy, so that's what we'll do. We'll leverage the existing dw_mmc knowledge about whether it should wait for the previous command to finish to know whether we should check for busy before sending the command. This means we won't end up incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which don't use the data line. Note that this also has the advantage of making sure that we don't change the clock while the card is busy, too. Signed-off-by: Doug Anderson diand...@chromium.org On an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi: Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk Best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug,
On 02/25/2015 06:43 AM, Doug Anderson wrote:
OK, so looking through things I _think_ I found another difference
that _might_ matter...
Yes it does! when adding the sd1_bus1 the slot pinctrl I do have
the WiFi module working, thanks a lot for your help!
Upstream (arch/arm/boot/dts/exynos5420-pinctrl.dtsi):
sd1_bus1: sd1-bus-width1 {
samsung,pins = gpc1-3;
...
};
sd1_bus4: sd1-bus-width4 {
samsung,pins = gpc1-4, gpc1-5, gpc1-6;
...
};
sd1_bus8: sd1-bus-width8 {
samsung,pins = gpd1-4, gpd1-5, gpd1-6, gpd1-7;
...
};
Upsttream (arch/arm/boot/dts/exynos5420-peach-pit.dts) -- your patch:
pinctrl-0 = sd1_clk, sd1_cmd, sd1_int, sd1_bus4,
sd1_bus8, wifi_en, wifi_rst;
Downstream (arch/arm/boot/dts/exynos542x-pinctrl.dtsi):
sd1_bus1: sd1-bus-width1 {
samsung,pins = gpc1-3;
...
};
sd1_bus4: sd1-bus-width4 {
samsung,pins = gpc1-3, gpc1-4, gpc1-5, gpc1-6;
...
};
sd1_bus8: sd1-bus-width8 {
samsung,pins = gpd1-4, gpd1-5, gpd1-6, gpd1-7;
...
};
Downstream (arch/arm/boot/dts/exynos542x-peach.dtsi):
pinctrl-0 = sd1_clk sd1_cmd sd1_int sd1_bus4 sd1_bus8;
Notice the difference? You need to add sd1_bus1 to the pinctrl for
upstream. The upstream DTS makes more sense. I think I remember
discussing this in the past (finding the conversation on the lists is
left as an exercise to the reader) and you can in fact see that the
upstream 5250 pinctrl file is like the downstream 5420 pinctrl...
Yeah, I didn't notice that there was an inconsistency in the pinctrl
defines in mainline around the different SoCs. So for 5250 you need:
* only sd1_bus1 for bus-width = 1
* only sd1_bus4 for bus-width = 4
* sd1_bus4 + sd1_bus8 for bus-width = 8
and for 5440 you need:
* only sd1_bus1 for bus-width = 1
* only sd1_bus1 + sd1_bus4 for bus-width = 4
* sd1_bus1 + sd1_bus4 + sd1_bus8 for bus-width = 8
Is true that 5440 is at least more consistent in the sense that you
have to add all the pins but IMHO this approach is very error prone.
I would preferred if sd1_busN included all pins needed for width N
so you only had to define a single pinctrl group but for now I'll
include sd1_bus1 to fix the SDIO WiFi issue.
I think the same bug is present in eMMC and SD but possibly the
bootloader inits the pinctrl properly there?
Correct, I'll fix those too so the kernel does not rely on the boot
loader to setup those pins.
Crossing my fingers that's your bug, but I can't say for sure why
adding a tons of resets would somehow make it better?
It is, thanks a lot again for finding this issue. I feel very dumb for
not realizing there was a difference in the included pinctrl definition.
-Doug
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Javier,
On Tue, Feb 24, 2015 at 3:20 AM, Javier Martinez Canillas
wrote:
>> Hm. In the cases I was seeing I didn't need the "reset" since the
>> "SDMMC_CMD_UPD_CLK" was the one from dw_mci_set_ios() and my patch:
>>
>> * mmc: dw_mmc: Give a good reset after we give power
>> https://patchwork.kernel.org/patch/5858281/
>>
>> ...gave the needed reset after vqmmc power was applied. Then dw_mmc
>> never got wedged and didn't need the reset to get it unwedged. In
>> your care you're getting called from dw_mci_init_card(). That should
>> happen _after_ dw_mci_set_ios() as far as I know. Can you put a
>> printout in the reset in dw_mci_set_ios() and make sure it runs?
>>
>
> Added a printout in dw_mci_set_ios() and I see that the card is reset
> at MMC_POWER_ON. So you are right that it gets wedged somehow between
> dw_mci_set_ios() and dw_mci_init_card().
Actually, if your code needs 3 resets then maybe there's something else wrong...
> This only happens when the slot is attached to a SDIO device though
> since the controller neither gets busy nor a command timeouts for
> the eMMC and uSD.
It sounds like there's something else going on here.
>> How many resets do you need before things work? If just one then
>> something must be happening between the initial "set_ios" and the
>
> The card is reseted 3 times to make it "work", that is to avoid being
> blocked constantly with "Timeout sending command" errors. But as I said
> before, even when the reset in dw_mci_wait_while_busy(), the firmware
> fails to load into the WiFi module so is not in the best state.
3 times? ...but that doesn't make a lot of sense to me. Is it simply
the delay that makes it work, or do you actually need the 3 resets?
Is anything else happening between all the resets? I guess you keep
trying to send the command and resetting between? Hrmmm...
Seems like there has got to be something else going on in this case...
>> BTW: IIRC the "WIFI_RSTn" ended up being totally unused. It was used
>> in earlier revs of the board that had a different rev of the WiFi
>> chip...
>>
>
> You are right, I removed WIFI_RSTn and the SDIO devices are still enumerated.
I just checked. WIFI_RSTn goes to the 3G connector which (as far as I
know) was never hooked up to anything. ...so yeah, you can safely
leave that out.
>>> +_1 {
>>> + status = "okay";
>>> + num-slots = <1>;
>>> + broken-cd;
>>> + cap-sdio-irq;
>>> + card-detect-delay = <200>;
>>> + clock-frequency = <4>;
>>> + samsung,dw-mshc-ciu-div = <1>;
>>> + samsung,dw-mshc-sdr-timing = <0 1>;
>>
>> Just for kicks, does this help if you do:
>>
>> ciu-div = 3
>> sdr-timing = 2 3
>>
>> ...I know we have ciu-div = 1 downstream, but we also have tuning...
>>
>
> This makes it even worst since with those values the boot hangs after a
> "Timeout sending command" error even with the reset in wait while busy.
I don't have a pit or pi hooked up on my desk right now, but I will
try to give it a shot soon and see what it ends up at. ...of course
you might still be at 400kHz mode in which case the divs just have to
not be totally wrong, I think...
>>> + samsung,dw-mshc-ddr-timing = <0 2>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <_clk>, <_cmd>, <_int>, <_bus4>,
>>> + <_bus8>, <_en>, <_rst>;
>>> + bus-width = <4>;
>>> + cap-sd-highspeed;
>>> + mmc-pwrseq = <_pwrseq>;
>>
>> Should you be specifying a "vqmmc-supply" here? I know that we don't
>> change voltages for WiFi (starts at 1.8V and ends up at 1.8V) but
>> still might be good to specify it...
>>
>
> Sure, I added a "vqmmc-supply = <_reg>" here. It does not have an
> effect though but is true that is better to specify it.
Yeah, makes sense.
---
OK, so looking through things I _think_ I found another difference
that _might_ matter...
Upstream (arch/arm/boot/dts/exynos5420-pinctrl.dtsi):
sd1_bus1: sd1-bus-width1 {
samsung,pins = "gpc1-3";
...
};
sd1_bus4: sd1-bus-width4 {
samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6";
...
};
sd1_bus8: sd1-bus-width8 {
samsung,pins = "gpd1-4", "gpd1-5", "gpd1-6", "gpd1-7";
...
};
Upsttream (arch/arm/boot/dts/exynos5420-peach-pit.dts) -- your patch:
pinctrl-0 = <_clk>, <_cmd>, <_int>, <_bus4>,
<_bus8>, <_en>, <_rst>;
Downstream (arch/arm/boot/dts/exynos542x-pinctrl.dtsi):
sd1_bus1: sd1-bus-width1 {
samsung,pins = "gpc1-3";
...
};
sd1_bus4: sd1-bus-width4 {
samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", "gpc1-6";
...
};
sd1_bus8: sd1-bus-width8 {
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug, On 02/23/2015 08:45 PM, Doug Anderson wrote: > On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas > wrote: >> >> Addy's "mmc: dw_mmc: fix bug that cause 'Timeout sending command" [0] patch >> reset the controller if it was busy for more than 500ms while your patch >> doesn't. I don't mean that your patch is not correct, I'm just mentioning >> because calling dw_mci_ctrl_reset() does makes the command to succeed the >> next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices: >> >> [5.892124] dwmmc_exynos 1221.mmc: Busy; trying anyway >> [5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0 >> status 0x0) >> [5.913885] mmc2: new high speed SDIO card at address 0001 >> [6.582133] dwmmc_exynos 1221.mmc: Busy; trying anyway > > Hm. In the cases I was seeing I didn't need the "reset" since the > "SDMMC_CMD_UPD_CLK" was the one from dw_mci_set_ios() and my patch: > > * mmc: dw_mmc: Give a good reset after we give power > https://patchwork.kernel.org/patch/5858281/ > > ...gave the needed reset after vqmmc power was applied. Then dw_mmc > never got wedged and didn't need the reset to get it unwedged. In > your care you're getting called from dw_mci_init_card(). That should > happen _after_ dw_mci_set_ios() as far as I know. Can you put a > printout in the reset in dw_mci_set_ios() and make sure it runs? > Added a printout in dw_mci_set_ios() and I see that the card is reset at MMC_POWER_ON. So you are right that it gets wedged somehow between dw_mci_set_ios() and dw_mci_init_card(). This only happens when the slot is attached to a SDIO device though since the controller neither gets busy nor a command timeouts for the eMMC and uSD. > How many resets do you need before things work? If just one then > something must be happening between the initial "set_ios" and the The card is reseted 3 times to make it "work", that is to avoid being blocked constantly with "Timeout sending command" errors. But as I said before, even when the reset in dw_mci_wait_while_busy(), the firmware fails to load into the WiFi module so is not in the best state. > "init_card". Any chance you could figure out what that is? If it > needs multiple resets then it seems like something is fishy... > Sure, I'll investigate more what happens between dw_mci_set_ios() and dw_mci_init_card(). Thanks for the suggestion. > >> but that reset may not be right since the WiFi chip does not work because >> the firmware later fails to load: >> >> [ 240.273352] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables >> this message. >> [ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x >> [ 240.287487] Workqueue: events request_firmware_work_func >> [ 240.292763] [] (__schedule) from [] >> (schedule+0x34/0x98) >> [ 240.299815] [] (schedule) from [] >> (schedule_timeout+0x120/0x16c) >> [ 240.307537] [] (schedule_timeout) from [] >> (wait_for_common+0xb0/0x154) >> [ 240.315783] [] (wait_for_common) from [] >> (mmc_wait_for_req+0xa0/0x140) >> [ 240.324020] [] (mmc_wait_for_req) from [] >> (mmc_io_rw_extended+0x304/0x34c) >> [ 240.332607] [] (mmc_io_rw_extended) from [] >> (sdio_io_rw_ext_helper+0x138/0x1a4) >> [ 240.341630] [] (sdio_io_rw_ext_helper) from [] >> (sdio_writesb+0x20/0x28) >> [ 240.349962] [] (sdio_writesb) from [] >> (mwifiex_write_data_sync+0x4c/0x80) >> [ 240.358460] [] (mwifiex_write_data_sync) from [] >> (mwifiex_prog_fw_w_helper+0x1b0/0x2c4) >> [ 240.368176] [] (mwifiex_prog_fw_w_helper) from [] >> (mwifiex_dnld_fw+0x58/0x10c) >> [ 240.377127] [] (mwifiex_dnld_fw) from [] >> (mwifiex_fw_dpc+0x264/0x408) >> [ 240.385263] [] (mwifiex_fw_dpc) from [] >> (request_firmware_work_func+0x30/0x50) >> [ 240.394200] [] (request_firmware_work_func) from [] >> (process_one_work+0x120/0x324) >> [ 240.403482] [] (process_one_work) from [] >> (worker_thread+0x138/0x464) >> [ 240.411635] [] (worker_thread) from [] >> (kthread+0xd8/0xf4) >> [ 240.418837] [] (kthread) from [] >> (ret_from_fork+0x14/0x34) >> >> The DTS changes on top of linux-next to add WiFi support is [1] in case you >> can >> find something that is wrong with my patch. > > I still haven't had time to try using the new MMC power sequencing :( > so I can't say for sure, but one thought below... > > >> I also checked that the external reference clock for the WiFi module is >> enabled >> correctly according to /sys/kernel/debug/clk/clk_summary and also by looking >> at the value in the max77802 i2c register address that enables that clock. >> >> Any hints will be highly appreciated. >> >> Thanks a lot and best regards, >> Javier >> >> [0]: https://lkml.org/lkml/2015/2/5/222 >> [1]: >> From ced0974472d109019e1ee551a6dd08f16ae5cfc2 Mon Sep 17 00:00:00 2001 >> From: Javier Martinez Canillas >> Date: Mon, 23 Feb 2014 15:42:15 +0100 >> Subject: [PATCH] ARM: dts: Add Peach Pit board WiFi support >> >> Peach boards have an SDIO WiFi
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug, On 02/23/2015 08:45 PM, Doug Anderson wrote: On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas javier.marti...@collabora.co.uk wrote: Addy's mmc: dw_mmc: fix bug that cause 'Timeout sending command [0] patch reset the controller if it was busy for more than 500ms while your patch doesn't. I don't mean that your patch is not correct, I'm just mentioning because calling dw_mci_ctrl_reset() does makes the command to succeed the next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices: [5.892124] dwmmc_exynos 1221.mmc: Busy; trying anyway [5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0 status 0x0) [5.913885] mmc2: new high speed SDIO card at address 0001 [6.582133] dwmmc_exynos 1221.mmc: Busy; trying anyway Hm. In the cases I was seeing I didn't need the reset since the SDMMC_CMD_UPD_CLK was the one from dw_mci_set_ios() and my patch: * mmc: dw_mmc: Give a good reset after we give power https://patchwork.kernel.org/patch/5858281/ ...gave the needed reset after vqmmc power was applied. Then dw_mmc never got wedged and didn't need the reset to get it unwedged. In your care you're getting called from dw_mci_init_card(). That should happen _after_ dw_mci_set_ios() as far as I know. Can you put a printout in the reset in dw_mci_set_ios() and make sure it runs? Added a printout in dw_mci_set_ios() and I see that the card is reset at MMC_POWER_ON. So you are right that it gets wedged somehow between dw_mci_set_ios() and dw_mci_init_card(). This only happens when the slot is attached to a SDIO device though since the controller neither gets busy nor a command timeouts for the eMMC and uSD. How many resets do you need before things work? If just one then something must be happening between the initial set_ios and the The card is reseted 3 times to make it work, that is to avoid being blocked constantly with Timeout sending command errors. But as I said before, even when the reset in dw_mci_wait_while_busy(), the firmware fails to load into the WiFi module so is not in the best state. init_card. Any chance you could figure out what that is? If it needs multiple resets then it seems like something is fishy... Sure, I'll investigate more what happens between dw_mci_set_ios() and dw_mci_init_card(). Thanks for the suggestion. but that reset may not be right since the WiFi chip does not work because the firmware later fails to load: [ 240.273352] echo 0 /proc/sys/kernel/hung_task_timeout_secs disables this message. [ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x [ 240.287487] Workqueue: events request_firmware_work_func [ 240.292763] [c04b3340] (__schedule) from [c04b36f0] (schedule+0x34/0x98) [ 240.299815] [c04b36f0] (schedule) from [c04b7198] (schedule_timeout+0x120/0x16c) [ 240.307537] [c04b7198] (schedule_timeout) from [c04b41b4] (wait_for_common+0xb0/0x154) [ 240.315783] [c04b41b4] (wait_for_common) from [c037abb8] (mmc_wait_for_req+0xa0/0x140) [ 240.324020] [c037abb8] (mmc_wait_for_req) from [c0384b04] (mmc_io_rw_extended+0x304/0x34c) [ 240.332607] [c0384b04] (mmc_io_rw_extended) from [c0385a90] (sdio_io_rw_ext_helper+0x138/0x1a4) [ 240.341630] [c0385a90] (sdio_io_rw_ext_helper) from [c0385b1c] (sdio_writesb+0x20/0x28) [ 240.349962] [c0385b1c] (sdio_writesb) from [c02e60d4] (mwifiex_write_data_sync+0x4c/0x80) [ 240.358460] [c02e60d4] (mwifiex_write_data_sync) from [c02e68e4] (mwifiex_prog_fw_w_helper+0x1b0/0x2c4) [ 240.368176] [c02e68e4] (mwifiex_prog_fw_w_helper) from [c02c7d6c] (mwifiex_dnld_fw+0x58/0x10c) [ 240.377127] [c02c7d6c] (mwifiex_dnld_fw) from [c02c5f10] (mwifiex_fw_dpc+0x264/0x408) [ 240.385263] [c02c5f10] (mwifiex_fw_dpc) from [c0291b28] (request_firmware_work_func+0x30/0x50) [ 240.394200] [c0291b28] (request_firmware_work_func) from [c0032ae8] (process_one_work+0x120/0x324) [ 240.403482] [c0032ae8] (process_one_work) from [c0032e58] (worker_thread+0x138/0x464) [ 240.411635] [c0032e58] (worker_thread) from [c0037470] (kthread+0xd8/0xf4) [ 240.418837] [c0037470] (kthread) from [c000e680] (ret_from_fork+0x14/0x34) The DTS changes on top of linux-next to add WiFi support is [1] in case you can find something that is wrong with my patch. I still haven't had time to try using the new MMC power sequencing :( so I can't say for sure, but one thought below... I also checked that the external reference clock for the WiFi module is enabled correctly according to /sys/kernel/debug/clk/clk_summary and also by looking at the value in the max77802 i2c register address that enables that clock. Any hints will be highly appreciated. Thanks a lot and best regards, Javier [0]: https://lkml.org/lkml/2015/2/5/222 [1]: From ced0974472d109019e1ee551a6dd08f16ae5cfc2 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas javier.marti...@collabora.co.uk
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Javier,
On Tue, Feb 24, 2015 at 3:20 AM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
Hm. In the cases I was seeing I didn't need the reset since the
SDMMC_CMD_UPD_CLK was the one from dw_mci_set_ios() and my patch:
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
...gave the needed reset after vqmmc power was applied. Then dw_mmc
never got wedged and didn't need the reset to get it unwedged. In
your care you're getting called from dw_mci_init_card(). That should
happen _after_ dw_mci_set_ios() as far as I know. Can you put a
printout in the reset in dw_mci_set_ios() and make sure it runs?
Added a printout in dw_mci_set_ios() and I see that the card is reset
at MMC_POWER_ON. So you are right that it gets wedged somehow between
dw_mci_set_ios() and dw_mci_init_card().
Actually, if your code needs 3 resets then maybe there's something else wrong...
This only happens when the slot is attached to a SDIO device though
since the controller neither gets busy nor a command timeouts for
the eMMC and uSD.
It sounds like there's something else going on here.
How many resets do you need before things work? If just one then
something must be happening between the initial set_ios and the
The card is reseted 3 times to make it work, that is to avoid being
blocked constantly with Timeout sending command errors. But as I said
before, even when the reset in dw_mci_wait_while_busy(), the firmware
fails to load into the WiFi module so is not in the best state.
3 times? ...but that doesn't make a lot of sense to me. Is it simply
the delay that makes it work, or do you actually need the 3 resets?
Is anything else happening between all the resets? I guess you keep
trying to send the command and resetting between? Hrmmm...
Seems like there has got to be something else going on in this case...
BTW: IIRC the WIFI_RSTn ended up being totally unused. It was used
in earlier revs of the board that had a different rev of the WiFi
chip...
You are right, I removed WIFI_RSTn and the SDIO devices are still enumerated.
I just checked. WIFI_RSTn goes to the 3G connector which (as far as I
know) was never hooked up to anything. ...so yeah, you can safely
leave that out.
+mmc_1 {
+ status = okay;
+ num-slots = 1;
+ broken-cd;
+ cap-sdio-irq;
+ card-detect-delay = 200;
+ clock-frequency = 4;
+ samsung,dw-mshc-ciu-div = 1;
+ samsung,dw-mshc-sdr-timing = 0 1;
Just for kicks, does this help if you do:
ciu-div = 3
sdr-timing = 2 3
...I know we have ciu-div = 1 downstream, but we also have tuning...
This makes it even worst since with those values the boot hangs after a
Timeout sending command error even with the reset in wait while busy.
I don't have a pit or pi hooked up on my desk right now, but I will
try to give it a shot soon and see what it ends up at. ...of course
you might still be at 400kHz mode in which case the divs just have to
not be totally wrong, I think...
+ samsung,dw-mshc-ddr-timing = 0 2;
+ pinctrl-names = default;
+ pinctrl-0 = sd1_clk, sd1_cmd, sd1_int, sd1_bus4,
+ sd1_bus8, wifi_en, wifi_rst;
+ bus-width = 4;
+ cap-sd-highspeed;
+ mmc-pwrseq = mmc1_pwrseq;
Should you be specifying a vqmmc-supply here? I know that we don't
change voltages for WiFi (starts at 1.8V and ends up at 1.8V) but
still might be good to specify it...
Sure, I added a vqmmc-supply = buck10_reg here. It does not have an
effect though but is true that is better to specify it.
Yeah, makes sense.
---
OK, so looking through things I _think_ I found another difference
that _might_ matter...
Upstream (arch/arm/boot/dts/exynos5420-pinctrl.dtsi):
sd1_bus1: sd1-bus-width1 {
samsung,pins = gpc1-3;
...
};
sd1_bus4: sd1-bus-width4 {
samsung,pins = gpc1-4, gpc1-5, gpc1-6;
...
};
sd1_bus8: sd1-bus-width8 {
samsung,pins = gpd1-4, gpd1-5, gpd1-6, gpd1-7;
...
};
Upsttream (arch/arm/boot/dts/exynos5420-peach-pit.dts) -- your patch:
pinctrl-0 = sd1_clk, sd1_cmd, sd1_int, sd1_bus4,
sd1_bus8, wifi_en, wifi_rst;
Downstream (arch/arm/boot/dts/exynos542x-pinctrl.dtsi):
sd1_bus1: sd1-bus-width1 {
samsung,pins = gpc1-3;
...
};
sd1_bus4: sd1-bus-width4 {
samsung,pins = gpc1-3, gpc1-4, gpc1-5, gpc1-6;
...
};
sd1_bus8: sd1-bus-width8 {
samsung,pins = gpd1-4, gpd1-5, gpd1-6, gpd1-7;
...
};
Downstream
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Javier,
On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas
wrote:
> Hello Doug,
>
> On 02/20/2015 09:31 PM, Doug Anderson wrote:
>> We've seen problems on some WiFi modules where we seem to send a CMD53
>> (which requires the data lines) while the module is asserting busy.
>> We shouldn't do that.
>>
>> The Designware Databook says that before issuing a new data transfer
>> command we should check for busy, so that's what we'll do.
>>
>
> I tried $subject along with patches:
>
> * mmc: dw_mmc: Make sure we only adjust the clock when power is on
> https://patchwork.kernel.org/patch/5858261/
>
> * mmc: dw_mmc: Give a good reset after we give power
> https://patchwork.kernel.org/patch/5858281/
>
> but unfortunately these don't solve the "Timeout sending command" error
> that I got when trying to enable the WiFi module on the Peach Pit and Pi:
>
> [5.332103] dwmmc_exynos 1221.mmc: Busy; trying anyway
> [5.336110] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
> status 0x0)
>
>> We'll leverage the existing dw_mmc knowledge about whether it should
>> wait for the previous command to finish to know whether we should
>> check for busy before sending the command. This means we won't end up
>> incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which
>> don't use the data line.
>>
>> Note that this also has the advantage of making sure that we don't
>> change the clock while the card is busy, too.
>>
>
> The timeout happens in this case:
>
> mmc_rescan()
> mmc_attach_sdio()
> mmc_sdio_init_card()
> dw_mci_init_card()
> mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>SDMMC_CMD_PRV_DAT_WAIT, 0);
>
> which should be covered by your patch since the SDMMC_CMD_PRV_DAT_WAIT flag
> is set but SDMMC_STATUS_BUSY bit is not cleared and mci_send_cmd() timeouts
> when sending the command to the controller.
>
>>
>> +static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
>> +{
>> + unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> +
>> + /*
>> + * Databook says that before issuing a new data transfer command
>> + * we need to check to see if the card is busy. Data transfer commands
>> + * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that.
>> + *
>> + * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is
>> + * expected.
>> + */
>> + if ((cmd_flags & SDMMC_CMD_PRV_DAT_WAIT) &&
>> + !(cmd_flags & SDMMC_CMD_VOLT_SWITCH)) {
>> + while (mci_readl(host, STATUS) & SDMMC_STATUS_BUSY) {
>> + if (time_after(jiffies, timeout)) {
>> + /* Command will fail; we'll pass error then */
>> + dev_err(host->dev, "Busy; trying anyway\n");
>
> Addy's "mmc: dw_mmc: fix bug that cause 'Timeout sending command" [0] patch
> reset the controller if it was busy for more than 500ms while your patch
> doesn't. I don't mean that your patch is not correct, I'm just mentioning
> because calling dw_mci_ctrl_reset() does makes the command to succeed the
> next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices:
>
> [5.892124] dwmmc_exynos 1221.mmc: Busy; trying anyway
> [5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
> status 0x0)
> [5.913885] mmc2: new high speed SDIO card at address 0001
> [6.582133] dwmmc_exynos 1221.mmc: Busy; trying anyway
Hm. In the cases I was seeing I didn't need the "reset" since the
"SDMMC_CMD_UPD_CLK" was the one from dw_mci_set_ios() and my patch:
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
...gave the needed reset after vqmmc power was applied. Then dw_mmc
never got wedged and didn't need the reset to get it unwedged. In
your care you're getting called from dw_mci_init_card(). That should
happen _after_ dw_mci_set_ios() as far as I know. Can you put a
printout in the reset in dw_mci_set_ios() and make sure it runs?
How many resets do you need before things work? If just one then
something must be happening between the initial "set_ios" and the
"init_card". Any chance you could figure out what that is? If it
needs multiple resets then it seems like something is fishy...
> but that reset may not be right since the WiFi chip does not work because
> the firmware later fails to load:
>
> [ 240.273352] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
> this message.
> [ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x
> [ 240.287487] Workqueue: events request_firmware_work_func
> [ 240.292763] [] (__schedule) from []
> (schedule+0x34/0x98)
> [ 240.299815] [] (schedule) from []
> (schedule_timeout+0x120/0x16c)
> [ 240.307537] [] (schedule_timeout) from []
> (wait_for_common+0xb0/0x154)
> [ 240.315783]
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug,
On 02/20/2015 09:31 PM, Doug Anderson wrote:
> We've seen problems on some WiFi modules where we seem to send a CMD53
> (which requires the data lines) while the module is asserting busy.
> We shouldn't do that.
>
> The Designware Databook says that before issuing a new data transfer
> command we should check for busy, so that's what we'll do.
>
I tried $subject along with patches:
* mmc: dw_mmc: Make sure we only adjust the clock when power is on
https://patchwork.kernel.org/patch/5858261/
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
but unfortunately these don't solve the "Timeout sending command" error
that I got when trying to enable the WiFi module on the Peach Pit and Pi:
[5.332103] dwmmc_exynos 1221.mmc: Busy; trying anyway
[5.336110] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
status 0x0)
> We'll leverage the existing dw_mmc knowledge about whether it should
> wait for the previous command to finish to know whether we should
> check for busy before sending the command. This means we won't end up
> incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which
> don't use the data line.
>
> Note that this also has the advantage of making sure that we don't
> change the clock while the card is busy, too.
>
The timeout happens in this case:
mmc_rescan()
mmc_attach_sdio()
mmc_sdio_init_card()
dw_mci_init_card()
mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
SDMMC_CMD_PRV_DAT_WAIT, 0);
which should be covered by your patch since the SDMMC_CMD_PRV_DAT_WAIT flag
is set but SDMMC_STATUS_BUSY bit is not cleared and mci_send_cmd() timeouts
when sending the command to the controller.
>
> +static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(500);
> +
> + /*
> + * Databook says that before issuing a new data transfer command
> + * we need to check to see if the card is busy. Data transfer commands
> + * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that.
> + *
> + * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is
> + * expected.
> + */
> + if ((cmd_flags & SDMMC_CMD_PRV_DAT_WAIT) &&
> + !(cmd_flags & SDMMC_CMD_VOLT_SWITCH)) {
> + while (mci_readl(host, STATUS) & SDMMC_STATUS_BUSY) {
> + if (time_after(jiffies, timeout)) {
> + /* Command will fail; we'll pass error then */
> + dev_err(host->dev, "Busy; trying anyway\n");
Addy's "mmc: dw_mmc: fix bug that cause 'Timeout sending command" [0] patch
reset the controller if it was busy for more than 500ms while your patch
doesn't. I don't mean that your patch is not correct, I'm just mentioning
because calling dw_mci_ctrl_reset() does makes the command to succeed the
next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices:
[5.892124] dwmmc_exynos 1221.mmc: Busy; trying anyway
[5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
status 0x0)
[5.913885] mmc2: new high speed SDIO card at address 0001
[6.582133] dwmmc_exynos 1221.mmc: Busy; trying anyway
but that reset may not be right since the WiFi chip does not work because
the firmware later fails to load:
[ 240.273352] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
[ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x
[ 240.287487] Workqueue: events request_firmware_work_func
[ 240.292763] [] (__schedule) from [] (schedule+0x34/0x98)
[ 240.299815] [] (schedule) from []
(schedule_timeout+0x120/0x16c)
[ 240.307537] [] (schedule_timeout) from []
(wait_for_common+0xb0/0x154)
[ 240.315783] [] (wait_for_common) from []
(mmc_wait_for_req+0xa0/0x140)
[ 240.324020] [] (mmc_wait_for_req) from []
(mmc_io_rw_extended+0x304/0x34c)
[ 240.332607] [] (mmc_io_rw_extended) from []
(sdio_io_rw_ext_helper+0x138/0x1a4)
[ 240.341630] [] (sdio_io_rw_ext_helper) from []
(sdio_writesb+0x20/0x28)
[ 240.349962] [] (sdio_writesb) from []
(mwifiex_write_data_sync+0x4c/0x80)
[ 240.358460] [] (mwifiex_write_data_sync) from []
(mwifiex_prog_fw_w_helper+0x1b0/0x2c4)
[ 240.368176] [] (mwifiex_prog_fw_w_helper) from []
(mwifiex_dnld_fw+0x58/0x10c)
[ 240.377127] [] (mwifiex_dnld_fw) from []
(mwifiex_fw_dpc+0x264/0x408)
[ 240.385263] [] (mwifiex_fw_dpc) from []
(request_firmware_work_func+0x30/0x50)
[ 240.394200] [] (request_firmware_work_func) from []
(process_one_work+0x120/0x324)
[ 240.403482] [] (process_one_work) from []
(worker_thread+0x138/0x464)
[ 240.411635] [] (worker_thread) from []
(kthread+0xd8/0xf4)
[ 240.418837] [] (kthread) from []
(ret_from_fork+0x14/0x34)
The DTS changes on top of linux-next to add WiFi
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hello Doug,
On 02/20/2015 09:31 PM, Doug Anderson wrote:
We've seen problems on some WiFi modules where we seem to send a CMD53
(which requires the data lines) while the module is asserting busy.
We shouldn't do that.
The Designware Databook says that before issuing a new data transfer
command we should check for busy, so that's what we'll do.
I tried $subject along with patches:
* mmc: dw_mmc: Make sure we only adjust the clock when power is on
https://patchwork.kernel.org/patch/5858261/
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
but unfortunately these don't solve the Timeout sending command error
that I got when trying to enable the WiFi module on the Peach Pit and Pi:
[5.332103] dwmmc_exynos 1221.mmc: Busy; trying anyway
[5.336110] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
status 0x0)
We'll leverage the existing dw_mmc knowledge about whether it should
wait for the previous command to finish to know whether we should
check for busy before sending the command. This means we won't end up
incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which
don't use the data line.
Note that this also has the advantage of making sure that we don't
change the clock while the card is busy, too.
The timeout happens in this case:
mmc_rescan()
mmc_attach_sdio()
mmc_sdio_init_card()
dw_mci_init_card()
mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
SDMMC_CMD_PRV_DAT_WAIT, 0);
which should be covered by your patch since the SDMMC_CMD_PRV_DAT_WAIT flag
is set but SDMMC_STATUS_BUSY bit is not cleared and mci_send_cmd() timeouts
when sending the command to the controller.
+static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(500);
+
+ /*
+ * Databook says that before issuing a new data transfer command
+ * we need to check to see if the card is busy. Data transfer commands
+ * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that.
+ *
+ * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is
+ * expected.
+ */
+ if ((cmd_flags SDMMC_CMD_PRV_DAT_WAIT)
+ !(cmd_flags SDMMC_CMD_VOLT_SWITCH)) {
+ while (mci_readl(host, STATUS) SDMMC_STATUS_BUSY) {
+ if (time_after(jiffies, timeout)) {
+ /* Command will fail; we'll pass error then */
+ dev_err(host-dev, Busy; trying anyway\n);
Addy's mmc: dw_mmc: fix bug that cause 'Timeout sending command [0] patch
reset the controller if it was busy for more than 500ms while your patch
doesn't. I don't mean that your patch is not correct, I'm just mentioning
because calling dw_mci_ctrl_reset() does makes the command to succeed the
next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices:
[5.892124] dwmmc_exynos 1221.mmc: Busy; trying anyway
[5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
status 0x0)
[5.913885] mmc2: new high speed SDIO card at address 0001
[6.582133] dwmmc_exynos 1221.mmc: Busy; trying anyway
but that reset may not be right since the WiFi chip does not work because
the firmware later fails to load:
[ 240.273352] echo 0 /proc/sys/kernel/hung_task_timeout_secs disables this
message.
[ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x
[ 240.287487] Workqueue: events request_firmware_work_func
[ 240.292763] [c04b3340] (__schedule) from [c04b36f0] (schedule+0x34/0x98)
[ 240.299815] [c04b36f0] (schedule) from [c04b7198]
(schedule_timeout+0x120/0x16c)
[ 240.307537] [c04b7198] (schedule_timeout) from [c04b41b4]
(wait_for_common+0xb0/0x154)
[ 240.315783] [c04b41b4] (wait_for_common) from [c037abb8]
(mmc_wait_for_req+0xa0/0x140)
[ 240.324020] [c037abb8] (mmc_wait_for_req) from [c0384b04]
(mmc_io_rw_extended+0x304/0x34c)
[ 240.332607] [c0384b04] (mmc_io_rw_extended) from [c0385a90]
(sdio_io_rw_ext_helper+0x138/0x1a4)
[ 240.341630] [c0385a90] (sdio_io_rw_ext_helper) from [c0385b1c]
(sdio_writesb+0x20/0x28)
[ 240.349962] [c0385b1c] (sdio_writesb) from [c02e60d4]
(mwifiex_write_data_sync+0x4c/0x80)
[ 240.358460] [c02e60d4] (mwifiex_write_data_sync) from [c02e68e4]
(mwifiex_prog_fw_w_helper+0x1b0/0x2c4)
[ 240.368176] [c02e68e4] (mwifiex_prog_fw_w_helper) from [c02c7d6c]
(mwifiex_dnld_fw+0x58/0x10c)
[ 240.377127] [c02c7d6c] (mwifiex_dnld_fw) from [c02c5f10]
(mwifiex_fw_dpc+0x264/0x408)
[ 240.385263] [c02c5f10] (mwifiex_fw_dpc) from [c0291b28]
(request_firmware_work_func+0x30/0x50)
[ 240.394200] [c0291b28] (request_firmware_work_func) from [c0032ae8]
(process_one_work+0x120/0x324)
[ 240.403482] [c0032ae8] (process_one_work) from [c0032e58]
(worker_thread+0x138/0x464)
[
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Javier,
On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas
javier.marti...@collabora.co.uk wrote:
Hello Doug,
On 02/20/2015 09:31 PM, Doug Anderson wrote:
We've seen problems on some WiFi modules where we seem to send a CMD53
(which requires the data lines) while the module is asserting busy.
We shouldn't do that.
The Designware Databook says that before issuing a new data transfer
command we should check for busy, so that's what we'll do.
I tried $subject along with patches:
* mmc: dw_mmc: Make sure we only adjust the clock when power is on
https://patchwork.kernel.org/patch/5858261/
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
but unfortunately these don't solve the Timeout sending command error
that I got when trying to enable the WiFi module on the Peach Pit and Pi:
[5.332103] dwmmc_exynos 1221.mmc: Busy; trying anyway
[5.336110] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
status 0x0)
We'll leverage the existing dw_mmc knowledge about whether it should
wait for the previous command to finish to know whether we should
check for busy before sending the command. This means we won't end up
incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which
don't use the data line.
Note that this also has the advantage of making sure that we don't
change the clock while the card is busy, too.
The timeout happens in this case:
mmc_rescan()
mmc_attach_sdio()
mmc_sdio_init_card()
dw_mci_init_card()
mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
SDMMC_CMD_PRV_DAT_WAIT, 0);
which should be covered by your patch since the SDMMC_CMD_PRV_DAT_WAIT flag
is set but SDMMC_STATUS_BUSY bit is not cleared and mci_send_cmd() timeouts
when sending the command to the controller.
+static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(500);
+
+ /*
+ * Databook says that before issuing a new data transfer command
+ * we need to check to see if the card is busy. Data transfer commands
+ * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that.
+ *
+ * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is
+ * expected.
+ */
+ if ((cmd_flags SDMMC_CMD_PRV_DAT_WAIT)
+ !(cmd_flags SDMMC_CMD_VOLT_SWITCH)) {
+ while (mci_readl(host, STATUS) SDMMC_STATUS_BUSY) {
+ if (time_after(jiffies, timeout)) {
+ /* Command will fail; we'll pass error then */
+ dev_err(host-dev, Busy; trying anyway\n);
Addy's mmc: dw_mmc: fix bug that cause 'Timeout sending command [0] patch
reset the controller if it was busy for more than 500ms while your patch
doesn't. I don't mean that your patch is not correct, I'm just mentioning
because calling dw_mci_ctrl_reset() does makes the command to succeed the
next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices:
[5.892124] dwmmc_exynos 1221.mmc: Busy; trying anyway
[5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0
status 0x0)
[5.913885] mmc2: new high speed SDIO card at address 0001
[6.582133] dwmmc_exynos 1221.mmc: Busy; trying anyway
Hm. In the cases I was seeing I didn't need the reset since the
SDMMC_CMD_UPD_CLK was the one from dw_mci_set_ios() and my patch:
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
...gave the needed reset after vqmmc power was applied. Then dw_mmc
never got wedged and didn't need the reset to get it unwedged. In
your care you're getting called from dw_mci_init_card(). That should
happen _after_ dw_mci_set_ios() as far as I know. Can you put a
printout in the reset in dw_mci_set_ios() and make sure it runs?
How many resets do you need before things work? If just one then
something must be happening between the initial set_ios and the
init_card. Any chance you could figure out what that is? If it
needs multiple resets then it seems like something is fishy...
but that reset may not be right since the WiFi chip does not work because
the firmware later fails to load:
[ 240.273352] echo 0 /proc/sys/kernel/hung_task_timeout_secs disables
this message.
[ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x
[ 240.287487] Workqueue: events request_firmware_work_func
[ 240.292763] [c04b3340] (__schedule) from [c04b36f0]
(schedule+0x34/0x98)
[ 240.299815] [c04b36f0] (schedule) from [c04b7198]
(schedule_timeout+0x120/0x16c)
[ 240.307537] [c04b7198] (schedule_timeout) from [c04b41b4]
(wait_for_common+0xb0/0x154)
[ 240.315783] [c04b41b4] (wait_for_common) from [c037abb8]
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hi, Doug
I have two cards which are easy to get "Timeout sending command":
one is ADATA uhs-1 SDR50, another is Kingston uhs-1 SDR104.
After merge three patches:
--
* mmc: dw_mmc: Don't start commands while busy
https://patchwork.kernel.org/patch/5858221/
* mmc: dw_mmc: Make sure we only adjust the clock when power is on
https://patchwork.kernel.org/patch/5858261/
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
-
Both of them can pass bind/unbind test, thank you.
test script:
cd /sys/bus/platform/drivers/dwmmc_rockchip
for i in $(seq 1 5000); do
echo "" $i
echo ff0c.dwmmc > unbind
sleep .5
echo ff0c.dwmmc > bind
sleep 2
done
On 2015/2/21 04:31, Doug Anderson wrote:
> We've seen problems on some WiFi modules where we seem to send a CMD53
> (which requires the data lines) while the module is asserting busy.
> We shouldn't do that.
>
> The Designware Databook says that before issuing a new data transfer
> command we should check for busy, so that's what we'll do.
>
> We'll leverage the existing dw_mmc knowledge about whether it should
> wait for the previous command to finish to know whether we should
> check for busy before sending the command. This means we won't end up
> incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which
> don't use the data line.
>
> Note that this also has the advantage of making sure that we don't
> change the clock while the card is busy, too.
>
> Signed-off-by: Doug Anderson
> ---
> Changes in v2:
> - Also check for busy in in mci_send_cmd()
>
> drivers/mmc/host/dw_mmc.c | 28
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 4d2e3c2..7733c5c 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -102,6 +102,7 @@ struct idmac_desc {
>
> static bool dw_mci_reset(struct dw_mci *host);
> static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
> +static int dw_mci_card_busy(struct mmc_host *mmc);
>
> #if defined(CONFIG_DEBUG_FS)
> static int dw_mci_req_show(struct seq_file *s, void *v)
> @@ -335,6 +336,31 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host,
> struct mmc_command *cmd)
> return cmdr;
> }
>
> +static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(500);
> +
> + /*
> + * Databook says that before issuing a new data transfer command
> + * we need to check to see if the card is busy. Data transfer commands
> + * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that.
> + *
> + * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is
> + * expected.
> + */
> + if ((cmd_flags & SDMMC_CMD_PRV_DAT_WAIT) &&
> + !(cmd_flags & SDMMC_CMD_VOLT_SWITCH)) {
> + while (mci_readl(host, STATUS) & SDMMC_STATUS_BUSY) {
> + if (time_after(jiffies, timeout)) {
> + /* Command will fail; we'll pass error then */
> + dev_err(host->dev, "Busy; trying anyway\n");
> + break;
> + }
> + udelay(10);
> + }
> + }
> +}
> +
> static void dw_mci_start_command(struct dw_mci *host,
>struct mmc_command *cmd, u32 cmd_flags)
> {
> @@ -345,6 +371,7 @@ static void dw_mci_start_command(struct dw_mci *host,
>
> mci_writel(host, CMDARG, cmd->arg);
> wmb();
> + dw_mci_wait_while_busy(host, cmd_flags);
>
> mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
> }
> @@ -876,6 +903,7 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32
> cmd, u32 arg)
>
> mci_writel(host, CMDARG, arg);
> wmb();
> + dw_mci_wait_while_busy(host, cmd);
> mci_writel(host, CMD, SDMMC_CMD_START | cmd);
>
> while (time_before(jiffies, timeout)) {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy
Hi, Doug
I have two cards which are easy to get Timeout sending command:
one is ADATA uhs-1 SDR50, another is Kingston uhs-1 SDR104.
After merge three patches:
--
* mmc: dw_mmc: Don't start commands while busy
https://patchwork.kernel.org/patch/5858221/
* mmc: dw_mmc: Make sure we only adjust the clock when power is on
https://patchwork.kernel.org/patch/5858261/
* mmc: dw_mmc: Give a good reset after we give power
https://patchwork.kernel.org/patch/5858281/
-
Both of them can pass bind/unbind test, thank you.
test script:
cd /sys/bus/platform/drivers/dwmmc_rockchip
for i in $(seq 1 5000); do
echo $i
echo ff0c.dwmmc unbind
sleep .5
echo ff0c.dwmmc bind
sleep 2
done
On 2015/2/21 04:31, Doug Anderson wrote:
We've seen problems on some WiFi modules where we seem to send a CMD53
(which requires the data lines) while the module is asserting busy.
We shouldn't do that.
The Designware Databook says that before issuing a new data transfer
command we should check for busy, so that's what we'll do.
We'll leverage the existing dw_mmc knowledge about whether it should
wait for the previous command to finish to know whether we should
check for busy before sending the command. This means we won't end up
incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which
don't use the data line.
Note that this also has the advantage of making sure that we don't
change the clock while the card is busy, too.
Signed-off-by: Doug Anderson diand...@chromium.org
---
Changes in v2:
- Also check for busy in in mci_send_cmd()
drivers/mmc/host/dw_mmc.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4d2e3c2..7733c5c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -102,6 +102,7 @@ struct idmac_desc {
static bool dw_mci_reset(struct dw_mci *host);
static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
+static int dw_mci_card_busy(struct mmc_host *mmc);
#if defined(CONFIG_DEBUG_FS)
static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -335,6 +336,31 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host,
struct mmc_command *cmd)
return cmdr;
}
+static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(500);
+
+ /*
+ * Databook says that before issuing a new data transfer command
+ * we need to check to see if the card is busy. Data transfer commands
+ * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that.
+ *
+ * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is
+ * expected.
+ */
+ if ((cmd_flags SDMMC_CMD_PRV_DAT_WAIT)
+ !(cmd_flags SDMMC_CMD_VOLT_SWITCH)) {
+ while (mci_readl(host, STATUS) SDMMC_STATUS_BUSY) {
+ if (time_after(jiffies, timeout)) {
+ /* Command will fail; we'll pass error then */
+ dev_err(host-dev, Busy; trying anyway\n);
+ break;
+ }
+ udelay(10);
+ }
+ }
+}
+
static void dw_mci_start_command(struct dw_mci *host,
struct mmc_command *cmd, u32 cmd_flags)
{
@@ -345,6 +371,7 @@ static void dw_mci_start_command(struct dw_mci *host,
mci_writel(host, CMDARG, cmd-arg);
wmb();
+ dw_mci_wait_while_busy(host, cmd_flags);
mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
}
@@ -876,6 +903,7 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32
cmd, u32 arg)
mci_writel(host, CMDARG, arg);
wmb();
+ dw_mci_wait_while_busy(host, cmd);
mci_writel(host, CMD, SDMMC_CMD_START | cmd);
while (time_before(jiffies, timeout)) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
