diff mbox

mmc: dw_mmc: fix bug that cause 'Timeout sending command'

Message ID 1423134801-23219-1-git-send-email-addy.ke@rock-chips.com
State New, archived
Headers show

Commit Message

addy ke Feb. 5, 2015, 11:13 a.m. UTC
Because of some uncertain factors, such as worse card or worse hardware,
DAT[3:0](the data lines) may be pulled down by card, and mmc controller
will be in busy state. This should not happend when mmc controller
send command to update card clocks. If this happends, mci_send_cmd will
be failed and we will get 'Timeout sending command', and then system will
be blocked. To avoid this, we need reset mmc controller.

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/mmc/host/dw_mmc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Ulf Hansson Feb. 9, 2015, 4:51 a.m. UTC | #1
On 5 February 2015 at 12:13, Addy Ke <addy.ke@rock-chips.com> wrote:
>
> Because of some uncertain factors, such as worse card or worse hardware,
> DAT[3:0](the data lines) may be pulled down by card, and mmc controller
> will be in busy state. This should not happend when mmc controller
> send command to update card clocks. If this happends, mci_send_cmd will
> be failed and we will get 'Timeout sending command', and then system will
> be blocked. To avoid this, we need reset mmc controller.
>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>


Hi Addy,

Should I consider $subject patch as a better option to the one below?

[PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
https://lkml.org/lkml/2015/1/13/562

Kind regards
Uffe


> ---
>  drivers/mmc/host/dw_mmc.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 4d2e3c2..b1d6dfb 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -100,6 +100,7 @@ struct idmac_desc {
>  };
>  #endif /* CONFIG_MMC_DW_IDMAC */
>
> +static int dw_mci_card_busy(struct mmc_host *mmc);
>  static bool dw_mci_reset(struct dw_mci *host);
>  static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
>
> @@ -888,6 +889,26 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
>                 cmd, arg, cmd_status);
>  }
>
> +static void dw_mci_wait_busy(struct dw_mci_slot *slot)
> +{
> +       struct dw_mci *host = slot->host;
> +       unsigned long timeout = jiffies + msecs_to_jiffies(500);
> +
> +       while (time_before(jiffies, timeout)) {
> +               if (!dw_mci_card_busy(slot->mmc))
> +                       return;
> +       }
> +       dev_err(host->dev, "Data busy (status %#x)\n",
> +               mci_readl(slot->host, STATUS));
> +
> +       /*
> +        * Data busy, this should not happend when mmc controller send command
> +        * to update card clocks in non-volt-switch state. If it happends, we
> +        * should reset controller to avoid getting "Timeout sending command".
> +        */
> +       dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS);
> +}
> +
>  static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>  {
>         struct dw_mci *host = slot->host;
> @@ -899,6 +920,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>         /* We must continue to set bit 28 in CMD until the change is complete */
>         if (host->state == STATE_WAITING_CMD11_DONE)
>                 sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH;
> +       else
> +               dw_mci_wait_busy(slot);
>
>         if (!clock) {
>                 mci_writel(host, CLKENA, 0);
> --
> 1.8.3.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
addy ke Feb. 9, 2015, 6:56 a.m. UTC | #2
On 2015.02.09 12:51, Ulf Hansson wrote:
> On 5 February 2015 at 12:13, Addy Ke <addy.ke@rock-chips.com> wrote:
>> Because of some uncertain factors, such as worse card or worse hardware,
>> DAT[3:0](the data lines) may be pulled down by card, and mmc controller
>> will be in busy state. This should not happend when mmc controller
>> send command to update card clocks. If this happends, mci_send_cmd will
>> be failed and we will get 'Timeout sending command', and then system will
>> be blocked. To avoid this, we need reset mmc controller.
>>
>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
>
> Hi Addy,
>
> Should I consider $subject patch as a better option to the one below?
No:
This patch fix the bug, which can be found by script:
     cd /sys/bus/platform/drivers/dwmmc_rockchip
     for i in $(seq 1 10000); do
       echo "========================" $i
       echo ff0c0000.dwmmc > unbind
       sleep .5
       echo ff0c0000.dwmmc > bind
       sleep 2
     done

> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
This patch is for tuning issue: we should delay until card go to idle 
state, when the previous command return error.
> https://lkml.org/lkml/2015/1/13/562
>
> Kind regards
> Uffe
>
>
>> ---
>>   drivers/mmc/host/dw_mmc.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 4d2e3c2..b1d6dfb 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -100,6 +100,7 @@ struct idmac_desc {
>>   };
>>   #endif /* CONFIG_MMC_DW_IDMAC */
>>
>> +static int dw_mci_card_busy(struct mmc_host *mmc);
>>   static bool dw_mci_reset(struct dw_mci *host);
>>   static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
>>
>> @@ -888,6 +889,26 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
>>                  cmd, arg, cmd_status);
>>   }
>>
>> +static void dw_mci_wait_busy(struct dw_mci_slot *slot)
>> +{
>> +       struct dw_mci *host = slot->host;
>> +       unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> +
>> +       while (time_before(jiffies, timeout)) {
>> +               if (!dw_mci_card_busy(slot->mmc))
>> +                       return;
>> +       }
>> +       dev_err(host->dev, "Data busy (status %#x)\n",
>> +               mci_readl(slot->host, STATUS));
>> +
>> +       /*
>> +        * Data busy, this should not happend when mmc controller send command
>> +        * to update card clocks in non-volt-switch state. If it happends, we
>> +        * should reset controller to avoid getting "Timeout sending command".
>> +        */
>> +       dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS);
>> +}
>> +
>>   static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>   {
>>          struct dw_mci *host = slot->host;
>> @@ -899,6 +920,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>          /* We must continue to set bit 28 in CMD until the change is complete */
>>          if (host->state == STATE_WAITING_CMD11_DONE)
>>                  sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH;
>> +       else
>> +               dw_mci_wait_busy(slot);
>>
>>          if (!clock) {
>>                  mci_writel(host, CLKENA, 0);
>> --
>> 1.8.3.2
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
Jaehoon Chung Feb. 9, 2015, 7:04 a.m. UTC | #3
On 02/09/2015 03:56 PM, Addy wrote:
> 
> 
> On 2015.02.09 12:51, Ulf Hansson wrote:
>> On 5 February 2015 at 12:13, Addy Ke <addy.ke@rock-chips.com> wrote:
>>> Because of some uncertain factors, such as worse card or worse hardware,
>>> DAT[3:0](the data lines) may be pulled down by card, and mmc controller
>>> will be in busy state. This should not happend when mmc controller
>>> send command to update card clocks. If this happends, mci_send_cmd will
>>> be failed and we will get 'Timeout sending command', and then system will
>>> be blocked. To avoid this, we need reset mmc controller.

I know that it needs to check whether card is busy or not, before clock-off.
This patch seems to related with it. right?

Best Regards,
Jaehoon Chung

>>>
>>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
>>
>> Hi Addy,
>>
>> Should I consider $subject patch as a better option to the one below?
> No:
> This patch fix the bug, which can be found by script:
>     cd /sys/bus/platform/drivers/dwmmc_rockchip
>     for i in $(seq 1 10000); do
>       echo "========================" $i
>       echo ff0c0000.dwmmc > unbind
>       sleep .5
>       echo ff0c0000.dwmmc > bind
>       sleep 2
>     done
> 
>> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
> This patch is for tuning issue: we should delay until card go to idle state, when the previous command return error.
>> https://lkml.org/lkml/2015/1/13/562
>>
>> Kind regards
>> Uffe
>>
>>
>>> ---
>>>   drivers/mmc/host/dw_mmc.c | 23 +++++++++++++++++++++++
>>>   1 file changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 4d2e3c2..b1d6dfb 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -100,6 +100,7 @@ struct idmac_desc {
>>>   };
>>>   #endif /* CONFIG_MMC_DW_IDMAC */
>>>
>>> +static int dw_mci_card_busy(struct mmc_host *mmc);
>>>   static bool dw_mci_reset(struct dw_mci *host);
>>>   static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
>>>
>>> @@ -888,6 +889,26 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
>>>                  cmd, arg, cmd_status);
>>>   }
>>>
>>> +static void dw_mci_wait_busy(struct dw_mci_slot *slot)
>>> +{
>>> +       struct dw_mci *host = slot->host;
>>> +       unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>> +
>>> +       while (time_before(jiffies, timeout)) {
>>> +               if (!dw_mci_card_busy(slot->mmc))
>>> +                       return;
>>> +       }
>>> +       dev_err(host->dev, "Data busy (status %#x)\n",
>>> +               mci_readl(slot->host, STATUS));
>>> +
>>> +       /*
>>> +        * Data busy, this should not happend when mmc controller send command
>>> +        * to update card clocks in non-volt-switch state. If it happends, we
>>> +        * should reset controller to avoid getting "Timeout sending command".
>>> +        */
>>> +       dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS);
>>> +}
>>> +
>>>   static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>   {
>>>          struct dw_mci *host = slot->host;
>>> @@ -899,6 +920,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>          /* We must continue to set bit 28 in CMD until the change is complete */
>>>          if (host->state == STATE_WAITING_CMD11_DONE)
>>>                  sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH;
>>> +       else
>>> +               dw_mci_wait_busy(slot);
>>>
>>>          if (!clock) {
>>>                  mci_writel(host, CLKENA, 0);
>>> -- 
>>> 1.8.3.2
>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> 
> 
>
addy ke Feb. 9, 2015, 7:25 a.m. UTC | #4
Addy Ke (2):
  mmc: dw_mmc: fix bug that cause 'Timeout sending command'
  mmc: dw_mmc: Don't start command while data busy

 drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
addy ke Feb. 9, 2015, 9:17 a.m. UTC | #5
On 2015/2/9 15:04, Jaehoon Chung wrote:
> On 02/09/2015 03:56 PM, Addy wrote:
>>
>>
>> On 2015.02.09 12:51, Ulf Hansson wrote:
>>> On 5 February 2015 at 12:13, Addy Ke <addy.ke@rock-chips.com> wrote:
>>>> Because of some uncertain factors, such as worse card or worse hardware,
>>>> DAT[3:0](the data lines) may be pulled down by card, and mmc controller
>>>> will be in busy state. This should not happend when mmc controller
>>>> send command to update card clocks. If this happends, mci_send_cmd will
>>>> be failed and we will get 'Timeout sending command', and then system will
>>>> be blocked. To avoid this, we need reset mmc controller.
> 
> I know that it needs to check whether card is busy or not, before clock-off.
> This patch seems to related with it. right?

Yes, it is.

> 
> Best Regards,
> Jaehoon Chung
> 
>>>>
>>>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
>>>
>>> Hi Addy,
>>>
>>> Should I consider $subject patch as a better option to the one below?
>> No:
>> This patch fix the bug, which can be found by script:
>>     cd /sys/bus/platform/drivers/dwmmc_rockchip
>>     for i in $(seq 1 10000); do
>>       echo "========================" $i
>>       echo ff0c0000.dwmmc > unbind
>>       sleep .5
>>       echo ff0c0000.dwmmc > bind
>>       sleep 2
>>     done
>>
>>> [PATCH] mmc: dw_mmc: rockchip: Add DW_MCI_QUIRK_RETRY_DELAY
>> This patch is for tuning issue: we should delay until card go to idle state, when the previous command return error.
>>> https://lkml.org/lkml/2015/1/13/562
>>>
>>> Kind regards
>>> Uffe
>>>
>>>
>>>> ---
>>>>   drivers/mmc/host/dw_mmc.c | 23 +++++++++++++++++++++++
>>>>   1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 4d2e3c2..b1d6dfb 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -100,6 +100,7 @@ struct idmac_desc {
>>>>   };
>>>>   #endif /* CONFIG_MMC_DW_IDMAC */
>>>>
>>>> +static int dw_mci_card_busy(struct mmc_host *mmc);
>>>>   static bool dw_mci_reset(struct dw_mci *host);
>>>>   static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
>>>>
>>>> @@ -888,6 +889,26 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
>>>>                  cmd, arg, cmd_status);
>>>>   }
>>>>
>>>> +static void dw_mci_wait_busy(struct dw_mci_slot *slot)
>>>> +{
>>>> +       struct dw_mci *host = slot->host;
>>>> +       unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>>> +
>>>> +       while (time_before(jiffies, timeout)) {
>>>> +               if (!dw_mci_card_busy(slot->mmc))
>>>> +                       return;
>>>> +       }
>>>> +       dev_err(host->dev, "Data busy (status %#x)\n",
>>>> +               mci_readl(slot->host, STATUS));
>>>> +
>>>> +       /*
>>>> +        * Data busy, this should not happend when mmc controller send command
>>>> +        * to update card clocks in non-volt-switch state. If it happends, we
>>>> +        * should reset controller to avoid getting "Timeout sending command".
>>>> +        */
>>>> +       dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS);
>>>> +}
>>>> +
>>>>   static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>>   {
>>>>          struct dw_mci *host = slot->host;
>>>> @@ -899,6 +920,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>>>          /* We must continue to set bit 28 in CMD until the change is complete */
>>>>          if (host->state == STATE_WAITING_CMD11_DONE)
>>>>                  sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH;
>>>> +       else
>>>> +               dw_mci_wait_busy(slot);
>>>>
>>>>          if (!clock) {
>>>>                  mci_writel(host, CLKENA, 0);
>>>> -- 
>>>> 1.8.3.2
>>>>
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
>>
> 
> 
> 
>
addy ke Feb. 13, 2015, 11:52 a.m. UTC | #6
patch 1: This patch can fix bug that controller is still data busy after
	 reset all blocks. After this patch, I still get data busy in
	 set_ios().

patch 2: This patch fix bug 'Timeout sending command'. After patch1 and
	 patch2, there is no mmc errors after:
	 cd /sys/bus/platform/drivers/dwmmc_rockchip
	 for i in $(seq 1 10000); do
  		echo "========================" $i
  		echo ff0c0000.dwmmc > unbind
  		sleep .5
  		echo ff0c0000.dwmmc > bind
  		sleep 2
	done
 
patch3: This patch fix bug that there is data busy before sdio send CMD53.
        But This patch is necessary for sd and mmc too.

Addy Ke (3):
  mmc: dw_mmc: update clock after host reach a stable voltage
  mmc: dw_mmc: fix bug that cause 'Timeout sending command'
  mmc: dw_mmc: Don't start command while data busy

 drivers/mmc/host/dw_mmc.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)
addy ke Feb. 14, 2015, 6:17 a.m. UTC | #7
patch 1: This patch can fix bug that controller is still data busy after
	 reset all blocks. After this patch, I still get data busy in
	 set_ios().

patch 2: This patch fix bug 'Timeout sending command'. After patch1 and
	 patch2, there is no mmc errors after:
	 cd /sys/bus/platform/drivers/dwmmc_rockchip
	 for i in $(seq 1 10000); do
  		echo "========================" $i
  		echo ff0c0000.dwmmc > unbind
  		sleep .5
  		echo ff0c0000.dwmmc > bind
  		sleep 2
	done
 
patch3: This patch fix bug that there is data busy before sdio send CMD53.
        But This patch is necessary for sd and mmc too.

Addy Ke (3):
  mmc: dw_mmc: update clock after host reach a stable voltage
  mmc: dw_mmc: fix bug that cause 'Timeout sending command'
  mmc: dw_mmc: Don't start command while data busy

 drivers/mmc/host/dw_mmc.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)
Javier Martinez Canillas Feb. 15, 2015, 11:41 a.m. UTC | #8
Hello Addy,

On Sat, Feb 14, 2015 at 7:17 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
> patch 1: This patch can fix bug that controller is still data busy after
>          reset all blocks. After this patch, I still get data busy in
>          set_ios().
>
> patch 2: This patch fix bug 'Timeout sending command'. After patch1 and
>          patch2, there is no mmc errors after:
>          cd /sys/bus/platform/drivers/dwmmc_rockchip
>          for i in $(seq 1 10000); do
>                 echo "========================" $i
>                 echo ff0c0000.dwmmc > unbind
>                 sleep .5
>                 echo ff0c0000.dwmmc > bind
>                 sleep 2
>         done
>
> patch3: This patch fix bug that there is data busy before sdio send CMD53.
>         But This patch is necessary for sd and mmc too.
>

I faced the same 'Timeout sending command' error when trying to enable
support for the SDIO wifi chip attached to mmc@12210000 (mmc1) on an
Exynos5420 Peach Pit Chromebook. On booting the kernel log shows:

mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status 0x80202000)

0x202000 == SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT so your patch
#2 dw_mci_setup_bus() avoids the mmc comand to time out. However, it
has a side effect since with your series the uSD that in mmc@12220000
(mmc2) fails to be detected and the kernel log shows:

[    5.466432] Waiting for root device /dev/mmcblk1p4...
[  240.169436] INFO: task kworker/u16:1:50 blocked for more than 120 seconds.
[  240.174844]       Not tainted
3.19.0-next-20150211-00006-g045d4aba96ce-dirty #476
[  240.182302] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
[  240.190109] kworker/u16:1   D c04c2710     0    50      2 0x00000000
[  240.196446] Workqueue: kmmcd mmc_rescan
[  240.200249] [<c04c2710>] (__schedule) from [<c04c2ac0>] (schedule+0x34/0x98)
[  240.207290] [<c04c2ac0>] (schedule) from [<c04c6568>]
(schedule_timeout+0x120/0x16c)
[  240.215009] [<c04c6568>] (schedule_timeout) from [<c04c3584>]
(wait_for_common+0xb0/0x154)
[  240.223251] [<c04c3584>] (wait_for_common) from [<c038a5ac>]
(mmc_wait_for_req+0xa0/0x140)
[  240.231492] [<c038a5ac>] (mmc_wait_for_req) from [<c038a6d4>]
(mmc_wait_for_cmd+0x88/0xa8)
[  240.239735] [<c038a6d4>] (mmc_wait_for_cmd) from [<c03905b0>]
(mmc_go_idle+0x78/0xf8)
[  240.247540] [<c03905b0>] (mmc_go_idle) from [<c038c578>]
(mmc_rescan+0x254/0x300)
[  240.255003] [<c038c578>] (mmc_rescan) from [<c00346e8>]
(process_one_work+0x120/0x324)
[  240.262897] [<c00346e8>] (process_one_work) from [<c0034a58>]
(worker_thread+0x138/0x464)
[  240.271048] [<c0034a58>] (worker_thread) from [<c0039070>]
(kthread+0xd8/0xf4)
[  240.278254] [<c0039070>] (kthread) from [<c000e680>]
(ret_from_fork+0x14/0x34)


By enabling debug I see that the card is detected in dw_mci_get_cd() though.

Alim suggested [0] that dw_mci_wait_busy() should be called in
mci_send_cmd() instead dw_mci_setup_bus() because the controller hangs
when when sending update clock cmd in different cases.

I modified [1] your patch #2 to do what Alim suggested and only with
that patch on top of linux-next I have neither the the "Timeout
sending command" error nor the uSD not getting detected errors. Linux
mounts the rootfs from the uSD and the wifi SDIO device is enumerated
and listed in /sys/bus/sdio/devices/

Does that also solve your issue?

Best regards,
Javier

[0]: https://lkml.org/lkml/2015/2/10/353
[1]: http://paste.debian.net/plain/148794
Jaehoon Chung Feb. 16, 2015, 5:48 a.m. UTC | #9
On 02/15/2015 08:41 PM, Javier Martinez Canillas wrote:
> Hello Addy,
> 
> On Sat, Feb 14, 2015 at 7:17 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
>> patch 1: This patch can fix bug that controller is still data busy after
>>          reset all blocks. After this patch, I still get data busy in
>>          set_ios().
>>
>> patch 2: This patch fix bug 'Timeout sending command'. After patch1 and
>>          patch2, there is no mmc errors after:
>>          cd /sys/bus/platform/drivers/dwmmc_rockchip
>>          for i in $(seq 1 10000); do
>>                 echo "========================" $i
>>                 echo ff0c0000.dwmmc > unbind
>>                 sleep .5
>>                 echo ff0c0000.dwmmc > bind
>>                 sleep 2
>>         done
>>
>> patch3: This patch fix bug that there is data busy before sdio send CMD53.
>>         But This patch is necessary for sd and mmc too.
>>
> 
> I faced the same 'Timeout sending command' error when trying to enable
> support for the SDIO wifi chip attached to mmc@12210000 (mmc1) on an
> Exynos5420 Peach Pit Chromebook. On booting the kernel log shows:
> 
> mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status 0x80202000)
> 
> 0x202000 == SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT so your patch
> #2 dw_mci_setup_bus() avoids the mmc comand to time out. However, it
> has a side effect since with your series the uSD that in mmc@12220000
> (mmc2) fails to be detected and the kernel log shows:
> 
> [    5.466432] Waiting for root device /dev/mmcblk1p4...
> [  240.169436] INFO: task kworker/u16:1:50 blocked for more than 120 seconds.
> [  240.174844]       Not tainted
> 3.19.0-next-20150211-00006-g045d4aba96ce-dirty #476
> [  240.182302] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [  240.190109] kworker/u16:1   D c04c2710     0    50      2 0x00000000
> [  240.196446] Workqueue: kmmcd mmc_rescan
> [  240.200249] [<c04c2710>] (__schedule) from [<c04c2ac0>] (schedule+0x34/0x98)
> [  240.207290] [<c04c2ac0>] (schedule) from [<c04c6568>]
> (schedule_timeout+0x120/0x16c)
> [  240.215009] [<c04c6568>] (schedule_timeout) from [<c04c3584>]
> (wait_for_common+0xb0/0x154)
> [  240.223251] [<c04c3584>] (wait_for_common) from [<c038a5ac>]
> (mmc_wait_for_req+0xa0/0x140)
> [  240.231492] [<c038a5ac>] (mmc_wait_for_req) from [<c038a6d4>]
> (mmc_wait_for_cmd+0x88/0xa8)
> [  240.239735] [<c038a6d4>] (mmc_wait_for_cmd) from [<c03905b0>]
> (mmc_go_idle+0x78/0xf8)
> [  240.247540] [<c03905b0>] (mmc_go_idle) from [<c038c578>]
> (mmc_rescan+0x254/0x300)
> [  240.255003] [<c038c578>] (mmc_rescan) from [<c00346e8>]
> (process_one_work+0x120/0x324)
> [  240.262897] [<c00346e8>] (process_one_work) from [<c0034a58>]
> (worker_thread+0x138/0x464)
> [  240.271048] [<c0034a58>] (worker_thread) from [<c0039070>]
> (kthread+0xd8/0xf4)
> [  240.278254] [<c0039070>] (kthread) from [<c000e680>]
> (ret_from_fork+0x14/0x34)
> 
> 
> By enabling debug I see that the card is detected in dw_mci_get_cd() though.
> 
> Alim suggested [0] that dw_mci_wait_busy() should be called in
> mci_send_cmd() instead dw_mci_setup_bus() because the controller hangs
> when when sending update clock cmd in different cases.
> 
> I modified [1] your patch #2 to do what Alim suggested and only with
> that patch on top of linux-next I have neither the the "Timeout
> sending command" error nor the uSD not getting detected errors. Linux
> mounts the rootfs from the uSD and the wifi SDIO device is enumerated
> and listed in /sys/bus/sdio/devices/

it needs to check when clock value only update.
As Javier and Alim are mentioned, if check whether card is busy or not in setup_bus(),
should be processed unnecessary checking.
(According to TRM, before disabling clock, check whether card is busy or not.)
if my thinking is right, chekcing is located more exactly before mci_writel(host, CLKENA, 0).

And i recommend if CLK_GATE is enabled, clkgate_delay sets to the bigger value than 3.
I'm not sure Javier's issue is same thing..I will check more this.

Best Regards,
Jaehoon Chung

> 
> Does that also solve your issue?
> 
> Best regards,
> Javier
> 
> [0]: https://lkml.org/lkml/2015/2/10/353
> [1]: http://paste.debian.net/plain/148794
>
Javier Martinez Canillas Feb. 16, 2015, 11:09 a.m. UTC | #10
Hello Jaehoon,

On Mon, Feb 16, 2015 at 6:48 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 02/15/2015 08:41 PM, Javier Martinez Canillas wrote:
>> I modified [1] your patch #2 to do what Alim suggested and only with
>> that patch on top of linux-next I have neither the the "Timeout
>> sending command" error nor the uSD not getting detected errors. Linux
>> mounts the rootfs from the uSD and the wifi SDIO device is enumerated
>> and listed in /sys/bus/sdio/devices/
>
> it needs to check when clock value only update.
> As Javier and Alim are mentioned, if check whether card is busy or not in setup_bus(),
> should be processed unnecessary checking.
> (According to TRM, before disabling clock, check whether card is busy or not.)
> if my thinking is right, chekcing is located more exactly before mci_writel(host, CLKENA, 0).
>
> And i recommend if CLK_GATE is enabled, clkgate_delay sets to the bigger value than 3.
> I'm not sure Javier's issue is same thing..I will check more this.
>

Thanks for checking, do you have access to a Peach Pit or Pi
Chromebook to reproduce the issue I reported? Please let me know if
you need any help from me.

> Best Regards,
> Jaehoon Chung
>

Best regards,
Javier
addy ke Feb. 19, 2015, 10:55 a.m. UTC | #11
Hi, Javier and Alim

These days are Spring Festival holiday.
Sorry for late reply.

On 2015/2/15 19:41, Javier Martinez Canillas wrote:
> Hello Addy,
> 
> On Sat, Feb 14, 2015 at 7:17 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
>> patch 1: This patch can fix bug that controller is still data busy after
>>          reset all blocks. After this patch, I still get data busy in
>>          set_ios().
>>
>> patch 2: This patch fix bug 'Timeout sending command'. After patch1 and
>>          patch2, there is no mmc errors after:
>>          cd /sys/bus/platform/drivers/dwmmc_rockchip
>>          for i in $(seq 1 10000); do
>>                 echo "========================" $i
>>                 echo ff0c0000.dwmmc > unbind
>>                 sleep .5
>>                 echo ff0c0000.dwmmc > bind
>>                 sleep 2
>>         done
>>
>> patch3: This patch fix bug that there is data busy before sdio send CMD53.
>>         But This patch is necessary for sd and mmc too.
>>
> 
> I faced the same 'Timeout sending command' error when trying to enable
> support for the SDIO wifi chip attached to mmc@12210000 (mmc1) on an
> Exynos5420 Peach Pit Chromebook. On booting the kernel log shows:
> 
> mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status 0x80202000)
> 
> 0x202000 == SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT so your patch
> #2 dw_mci_setup_bus() avoids the mmc comand to time out. However, it
> has a side effect since with your series the uSD that in mmc@12220000
> (mmc2) fails to be detected and the kernel log shows:
> 
> [    5.466432] Waiting for root device /dev/mmcblk1p4...
> [  240.169436] INFO: task kworker/u16:1:50 blocked for more than 120 seconds.
> [  240.174844]       Not tainted
> 3.19.0-next-20150211-00006-g045d4aba96ce-dirty #476
> [  240.182302] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [  240.190109] kworker/u16:1   D c04c2710     0    50      2 0x00000000
> [  240.196446] Workqueue: kmmcd mmc_rescan
> [  240.200249] [<c04c2710>] (__schedule) from [<c04c2ac0>] (schedule+0x34/0x98)
> [  240.207290] [<c04c2ac0>] (schedule) from [<c04c6568>]
> (schedule_timeout+0x120/0x16c)
> [  240.215009] [<c04c6568>] (schedule_timeout) from [<c04c3584>]
> (wait_for_common+0xb0/0x154)
> [  240.223251] [<c04c3584>] (wait_for_common) from [<c038a5ac>]
> (mmc_wait_for_req+0xa0/0x140)
> [  240.231492] [<c038a5ac>] (mmc_wait_for_req) from [<c038a6d4>]
> (mmc_wait_for_cmd+0x88/0xa8)
> [  240.239735] [<c038a6d4>] (mmc_wait_for_cmd) from [<c03905b0>]
> (mmc_go_idle+0x78/0xf8)
> [  240.247540] [<c03905b0>] (mmc_go_idle) from [<c038c578>]
> (mmc_rescan+0x254/0x300)
> [  240.255003] [<c038c578>] (mmc_rescan) from [<c00346e8>]
> (process_one_work+0x120/0x324)
> [  240.262897] [<c00346e8>] (process_one_work) from [<c0034a58>]
> (worker_thread+0x138/0x464)
> [  240.271048] [<c0034a58>] (worker_thread) from [<c0039070>]
> (kthread+0xd8/0xf4)
> [  240.278254] [<c0039070>] (kthread) from [<c000e680>]
> (ret_from_fork+0x14/0x34)
> 
> 
> By enabling debug I see that the card is detected in dw_mci_get_cd() though.
> 
> Alim suggested [0] that dw_mci_wait_busy() should be called in
> mci_send_cmd() instead dw_mci_setup_bus() because the controller hangs
> when when sending update clock cmd in different cases.
> 
> I modified [1] your patch #2 to do what Alim suggested and only with
> that patch on top of linux-next I have neither the the "Timeout
> sending command" error nor the uSD not getting detected errors. Linux
> mounts the rootfs from the uSD and the wifi SDIO device is enumerated
> and listed in /sys/bus/sdio/devices/
> 
> Does that also solve your issue?

After merge Alim patch,and set re_try 8,
it can pass test by:
cd /sys/bus/platform/drivers/dwmmc_rockchip
for i in $(seq 1 10000); do
  echo "========================" $i
  echo ff0c0000.dwmmc > unbind
  sleep .5
  echo ff0c0000.dwmmc > bind
  sleep 2
done

My card is ADATA UHS-1 card(SDR50).
The maximum retry count is 6.

[ 1146.907596] mmc1: card 59b4 removed
[ 1147.421036] dwmmc_rockchip ff0c0000.dwmmc: Using internal DMA controller.
[ 1147.427827] dwmmc_rockchip ff0c0000.dwmmc: Version ID is 270a
[ 1147.433958] dwmmc_rockchip ff0c0000.dwmmc: DW MMC controller at irq 64, 32 bit host data width, 256 deep fifo
[ 1147.444269] dwmmc_rockchip ff0c0000.dwmmc: Got CD GPIO #221.
[ 1147.450381] dwmmc_rockchip ff0c0000.dwmmc: Got WP GPIO #226.
[ 1147.456046] ff0c0000.dwmmc supply card-external-vcc not found, using dummy regulator
[ 1148.519400] dwmmc_rockchip ff0c0000.dwmmc: Data busy (status 0x206)
[ 1149.019451] dwmmc_rockchip ff0c0000.dwmmc: Data busy (status 0x206)
[ 1149.519382] dwmmc_rockchip ff0c0000.dwmmc: Data busy (status 0x206)
[ 1150.019492] dwmmc_rockchip ff0c0000.dwmmc: Data busy (status 0x206)
[ 1150.519442] dwmmc_rockchip ff0c0000.dwmmc: Data busy (status 0x206)
>>>>>>>>>>>>>>>>>>>>>>>>> if re_try is 5, I still get "Timeout sending command".
[ 1150.525711] mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status 0x80202000)
[ 1150.534723] rockchip-iodomain io-domains.25: Setting to 3300000 done

So re_try must bigger than 6, but I don't known which value is reansonable.
Do you have any idear about it?

This is the patch Alim suggests:
+       int re_try = 8; /* just random for now, 1 re-try should be ok */

-       mci_writel(host, CMDARG, arg);
-       wmb();
-       mci_writel(host, CMD, SDMMC_CMD_START | cmd);
+       while(re_try--) {
+               mci_writel(host, CMDARG, arg);
+               wmb();
+               mci_writel(host, CMD, SDMMC_CMD_START | cmd);

-       while (time_before(jiffies, timeout)) {
-               cmd_status = mci_readl(host, CMD);
-               if (!(cmd_status & SDMMC_CMD_START))
-                       return;
+               while (time_before(jiffies, timeout)) {
+                       cmd_status = mci_readl(host, CMD);
+                       if (!(cmd_status & SDMMC_CMD_START))
+                               return;
+               }
+
+               dw_mci_wait_busy(slot);
> 
> Best regards,
> Javier
> 
> [0]: https://lkml.org/lkml/2015/2/10/353
> [1]: http://paste.debian.net/plain/148794
> 
> 
>
Doug Anderson Feb. 20, 2015, 7:03 p.m. UTC | #12
Hi,

On Fri, Feb 13, 2015 at 10:17 PM, Addy Ke <addy.ke@rock-chips.com> wrote:
> patch 1: This patch can fix bug that controller is still data busy after
>          reset all blocks. After this patch, I still get data busy in
>          set_ios().
>
> patch 2: This patch fix bug 'Timeout sending command'. After patch1 and
>          patch2, there is no mmc errors after:
>          cd /sys/bus/platform/drivers/dwmmc_rockchip
>          for i in $(seq 1 10000); do
>                 echo "========================" $i
>                 echo ff0c0000.dwmmc > unbind
>                 sleep .5
>                 echo ff0c0000.dwmmc > bind
>                 sleep 2
>         done
>
> patch3: This patch fix bug that there is data busy before sdio send CMD53.
>         But This patch is necessary for sd and mmc too.
>
> Addy Ke (3):
>   mmc: dw_mmc: update clock after host reach a stable voltage
>   mmc: dw_mmc: fix bug that cause 'Timeout sending command'
>   mmc: dw_mmc: Don't start command while data busy
>
>  drivers/mmc/host/dw_mmc.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)

A little hard to follow all the patches flying around (so I'll
probably reply a few different places with the same info), but I
believe that all of Addy's patches (with the exception of the one
intended to fix mmc_test which needs to be spun by him for a bugfix)
can be replaced with:

* 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/


In order to avoid further spreading info out among several patches,
I'd request that you don't respond here but instead respond to my
posted patches.  Thanks!

-Doug
diff mbox

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4d2e3c2..b1d6dfb 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -100,6 +100,7 @@  struct idmac_desc {
 };
 #endif /* CONFIG_MMC_DW_IDMAC */
 
+static int dw_mci_card_busy(struct mmc_host *mmc);
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 
@@ -888,6 +889,26 @@  static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
 		cmd, arg, cmd_status);
 }
 
+static void dw_mci_wait_busy(struct dw_mci_slot *slot)
+{
+	struct dw_mci *host = slot->host;
+	unsigned long timeout = jiffies + msecs_to_jiffies(500);
+
+	while (time_before(jiffies, timeout)) {
+		if (!dw_mci_card_busy(slot->mmc))
+			return;
+	}
+	dev_err(host->dev, "Data busy (status %#x)\n",
+		mci_readl(slot->host, STATUS));
+
+	/*
+	 * Data busy, this should not happend when mmc controller send command
+	 * to update card clocks in non-volt-switch state. If it happends, we
+	 * should reset controller to avoid getting "Timeout sending command".
+	 */
+	dw_mci_ctrl_reset(host, SDMMC_CTRL_ALL_RESET_FLAGS);
+}
+
 static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
 {
 	struct dw_mci *host = slot->host;
@@ -899,6 +920,8 @@  static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
 	/* We must continue to set bit 28 in CMD until the change is complete */
 	if (host->state == STATE_WAITING_CMD11_DONE)
 		sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH;
+	else
+		dw_mci_wait_busy(slot);
 
 	if (!clock) {
 		mci_writel(host, CLKENA, 0);