[PATCH v2] mmc: dw_mmc: Don't start commands while busy

[PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Doug Anderson]

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 <dianders@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)) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[addy ke]

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 ff0c0000.dwmmc > unbind
      sleep .5
      echo ff0c0000.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 <dianders@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)) {
> 

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Javier Martinez Canillas]

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 12210000.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 12210000.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 12210000.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 0x00000000
[  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 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.martinez@collabora.co.uk>
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 module that is always powered but it
needs a power sequence involving the reset and enable pins and also
a 32kHz reference clock.

Add a dev node for the SDIO slot and the MMC power sequence provider.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5420-peach-pit.dts | 59 ++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index ec86d9523935..26df041e46e7 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -125,6 +125,14 @@
 			};
 		};
 	};
+
+	mmc1_pwrseq: mmc1_pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		reset-gpios = <&gpx0 1 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */
+			      <&gpx0 0 GPIO_ACTIVE_LOW>; /* WIFI_EN */
+		clocks = <&max77802 MAX77802_CLK_32K_CP>;
+		clock-names = "ext_clock";
+	};
 };
 
 &adc {
@@ -691,6 +699,24 @@
 	bus-width = <8>;
 };
 
+&mmc_1 {
+	status = "okay";
+	num-slots = <1>;
+	broken-cd;
+	cap-sdio-irq;
+	card-detect-delay = <200>;
+	clock-frequency = <400000000>;
+	samsung,dw-mshc-ciu-div = <1>;
+	samsung,dw-mshc-sdr-timing = <0 1>;
+	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>;
+};
+
 &mmc_2 {
 	status = "okay";
 	num-slots = <1>;
@@ -710,6 +736,20 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&mask_tpm_reset>;
 
+	wifi_en: wifi-en {
+		samsung,pins = "gpx0-0";
+		samsung,pin-function = <1>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
+	wifi_rst: wifi-rst {
+		samsung,pins = "gpx0-1";
+		samsung,pin-function = <1>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	max98090_irq: max98090-irq {
 		samsung,pins = "gpx0-2";
 		samsung,pin-function = <0>;
@@ -797,6 +837,25 @@
 	};
 };
 
+&pinctrl_1 {
+	/* Adjust WiFi drive strengths lower for EMI */
+	sd1_clk: sd1-clk {
+		samsung,pin-drv = <2>;
+	};
+
+	sd1_cmd: sd1-cmd {
+		samsung,pin-drv = <2>;
+	};
+
+	sd1_bus4: sd1-bus-width4 {
+		samsung,pin-drv = <2>;
+	};
+
+	sd1_bus8: sd1-bus-width8 {
+		samsung,pin-drv = <2>;
+	};
+};
+
 &pinctrl_2 {
 	pmic_dvs_2: pmic-dvs-2 {
 		samsung,pins = "gpj4-2";
-- 
2.1.3

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Doug Anderson]

Javier,

On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas
<javier.martinez@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 12210000.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 12210000.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 12210000.mmc: Busy; trying anyway

Hmmmmm.  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 0x00000000
> [  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.martinez@collabora.co.uk>
> 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 module that is always powered but it
> needs a power sequence involving the reset and enable pins and also
> a 32kHz reference clock.
>
> Add a dev node for the SDIO slot and the MMC power sequence provider.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/boot/dts/exynos5420-peach-pit.dts | 59 ++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
> index ec86d9523935..26df041e46e7 100644
> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
> @@ -125,6 +125,14 @@
>                         };
>                 };
>         };
> +
> +       mmc1_pwrseq: mmc1_pwrseq {
> +               compatible = "mmc-pwrseq-simple";
> +               reset-gpios = <&gpx0 1 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */
> +                             <&gpx0 0 GPIO_ACTIVE_LOW>; /* WIFI_EN */

Any chance you want "WIFI_EN" to actually be specified as "vmmc" for
the slot?  ...possibly with a 'regulator-enable-ramp-delay' specified?
 I know that with SD Cards on an rk3288 board I needed to make sure
there was a bit of delay after "vqmmc" came up...

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

> +               clocks = <&max77802 MAX77802_CLK_32K_CP>;
> +               clock-names = "ext_clock";
> +       };
>  };
>
>  &adc {
> @@ -691,6 +699,24 @@
>         bus-width = <8>;
>  };
>
> +&mmc_1 {
> +       status = "okay";
> +       num-slots = <1>;
> +       broken-cd;
> +       cap-sdio-irq;
> +       card-detect-delay = <200>;
> +       clock-frequency = <400000000>;
> +       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...

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

> +};
> +
>  &mmc_2 {
>         status = "okay";
>         num-slots = <1>;
> @@ -710,6 +736,20 @@
>         pinctrl-names = "default";
>         pinctrl-0 = <&mask_tpm_reset>;
>
> +       wifi_en: wifi-en {
> +               samsung,pins = "gpx0-0";
> +               samsung,pin-function = <1>;
> +               samsung,pin-pud = <0>;
> +               samsung,pin-drv = <0>;
> +       };
> +
> +       wifi_rst: wifi-rst {
> +               samsung,pins = "gpx0-1";
> +               samsung,pin-function = <1>;
> +               samsung,pin-pud = <0>;
> +               samsung,pin-drv = <0>;
> +       };
> +
>         max98090_irq: max98090-irq {
>                 samsung,pins = "gpx0-2";
>                 samsung,pin-function = <0>;
> @@ -797,6 +837,25 @@
>         };
>  };
>
> +&pinctrl_1 {
> +       /* Adjust WiFi drive strengths lower for EMI */
> +       sd1_clk: sd1-clk {
> +               samsung,pin-drv = <2>;
> +       };
> +
> +       sd1_cmd: sd1-cmd {
> +               samsung,pin-drv = <2>;
> +       };
> +
> +       sd1_bus4: sd1-bus-width4 {
> +               samsung,pin-drv = <2>;
> +       };
> +
> +       sd1_bus8: sd1-bus-width8 {
> +               samsung,pin-drv = <2>;
> +       };
> +};
> +
>  &pinctrl_2 {
>         pmic_dvs_2: pmic-dvs-2 {
>                 samsung,pins = "gpj4-2";
> --
> 2.1.3

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Javier Martinez Canillas]

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.martinez@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 12210000.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 12210000.mmc: Busy; trying anyway
> 
> Hmmmmm.  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 0x00000000
>> [  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.martinez@collabora.co.uk>
>> 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 module that is always powered but it
>> needs a power sequence involving the reset and enable pins and also
>> a 32kHz reference clock.
>>
>> Add a dev node for the SDIO slot and the MMC power sequence provider.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>  arch/arm/boot/dts/exynos5420-peach-pit.dts | 59 ++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> index ec86d9523935..26df041e46e7 100644
>> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
>> @@ -125,6 +125,14 @@
>>                         };
>>                 };
>>         };
>> +
>> +       mmc1_pwrseq: mmc1_pwrseq {
>> +               compatible = "mmc-pwrseq-simple";
>> +               reset-gpios = <&gpx0 1 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */
>> +                             <&gpx0 0 GPIO_ACTIVE_LOW>; /* WIFI_EN */
> 
> Any chance you want "WIFI_EN" to actually be specified as "vmmc" for
> the slot?  ...possibly with a 'regulator-enable-ramp-delay' specified?
>  I know that with SD Cards on an rk3288 board I needed to make sure
> there was a bit of delay after "vqmmc" came up...
>

If that's the case then we would had a bug in the MMC power sequencing
support but I don't think that is needed because there is a mmc_delay(10)
in mmc_power_up() between mmc_pwrseq_pre_power_on() and mmc_set_ios().

But just in case, I tried removing the "reset-gpios" property and adding
"WIFI_EN" as a fixed regulator with a "regulator-enable-ramp-delay" but
the behavior is the same.

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

>> +               clocks = <&max77802 MAX77802_CLK_32K_CP>;
>> +               clock-names = "ext_clock";
>> +       };
>>  };
>>
>>  &adc {
>> @@ -691,6 +699,24 @@
>>         bus-width = <8>;
>>  };
>>
>> +&mmc_1 {
>> +       status = "okay";
>> +       num-slots = <1>;
>> +       broken-cd;
>> +       cap-sdio-irq;
>> +       card-detect-delay = <200>;
>> +       clock-frequency = <400000000>;
>> +       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.

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

Best regards,
Javier

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Doug Anderson]

Javier,

On Tue, Feb 24, 2015 at 3:20 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>> Hmmmmm.  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 = <400000000>;
>>> +       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 (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...

I think the same bug is present in eMMC and SD but possibly the
bootloader inits the pinctrl properly there?


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?

-Doug

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Javier Martinez Canillas]

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

Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

[Javier Martinez Canillas]

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 <dianders@chromium.org>
>

On an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi:

Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier