[PATCH] mmc: core: make hw_reset power cycle SD-cards

[PATCH] mmc: core: make hw_reset power cycle SD-cards

[Johan Rudholm]

SD-cards cannot be reset, but they can be power cycled. Power cycling
a buggy SD-card sometimes helps it get back on track.

Signed-off-by: Johan Rudholm <johanru@axis.com>
---
 drivers/mmc/core/core.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d03a080..9171aa9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2250,19 +2250,25 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
 {
 	struct mmc_card *card = host->card;
 
-	if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
-		return -EOPNOTSUPP;
-
 	if (!card)
 		return -EINVAL;
 
-	if (!mmc_can_reset(card))
+	if (!mmc_card_sd(card) && !((host->caps & MMC_CAP_HW_RESET) &&
+							host->ops->hw_reset))
+		return -EOPNOTSUPP;
+
+	if (!mmc_card_sd(card) && !mmc_can_reset(card))
 		return -EOPNOTSUPP;
 
 	mmc_host_clk_hold(host);
 	mmc_set_clock(host, host->f_init);
 
-	host->ops->hw_reset(host);
+	if (mmc_card_sd(card))
+		mmc_power_cycle(host, card->ocr);
+	else
+		host->ops->hw_reset(host);
+
+	pr_warning("%s: reset device\n", mmc_hostname(host));
 
 	/* If the reset has happened, then a status command will fail */
 	if (check) {
-- 
1.7.2.5

Re: [PATCH] mmc: core: make hw_reset power cycle SD-cards

[Ulf Hansson]

On 9 September 2014 16:21, Johan Rudholm <johan.rudholm@axis.com> wrote:
> SD-cards cannot be reset, but they can be power cycled. Power cycling
> a buggy SD-card sometimes helps it get back on track.

Hi Johan,

This feature has been missing for too long - really great that you are
working on this!

>
> Signed-off-by: Johan Rudholm <johanru@axis.com>
> ---
>  drivers/mmc/core/core.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index d03a080..9171aa9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2250,19 +2250,25 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
>  {
>         struct mmc_card *card = host->card;
>
> -       if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
> -               return -EOPNOTSUPP;
> -
>         if (!card)
>                 return -EINVAL;
>
> -       if (!mmc_can_reset(card))
> +       if (!mmc_card_sd(card) && !((host->caps & MMC_CAP_HW_RESET) &&
> +                                                       host->ops->hw_reset))
> +               return -EOPNOTSUPP;
> +
> +       if (!mmc_card_sd(card) && !mmc_can_reset(card))
>                 return -EOPNOTSUPP;
>
>         mmc_host_clk_hold(host);
>         mmc_set_clock(host, host->f_init);
>
> -       host->ops->hw_reset(host);
> +       if (mmc_card_sd(card))
> +               mmc_power_cycle(host, card->ocr);
> +       else
> +               host->ops->hw_reset(host);
> +
> +       pr_warning("%s: reset device\n", mmc_hostname(host));
>
>         /* If the reset has happened, then a status command will fail */
>         if (check) {
> --
> 1.7.2.5
>

Before we decide to go ahead an continue to hack on the current reset
code as your patch does, could you possibly consider to do some more
extensive rework this code?

Here are some of the ideas that I had in mind.

1) Invent a new bus_ops callback for reset. Or maybe we should re-use
the ->power_save|restore ones, since those already exist!?
2) Since those bus_ops are specific for sd/mmc/sdio we shall handle
those related parts in there.
3) Then let's mmc_do_hw_reset invoke the above bus_ops.
4) Remove the mmc_hw_reset_check() API. Currently only used by mmc_test.

What do you think?

Kind regards
Uffe

Re: [PATCH] mmc: core: make hw_reset power cycle SD-cards

[Johan Rudholm]

Hi Ulf,

2014-09-11 13:03 GMT+02:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 9 September 2014 16:21, Johan Rudholm <johan.rudholm@axis.com> wrote:
>> SD-cards cannot be reset, but they can be power cycled. Power cycling
>> a buggy SD-card sometimes helps it get back on track.
>
> Hi Johan,
>
> This feature has been missing for too long - really great that you are
> working on this!
>
>>
>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>> ---
>>  drivers/mmc/core/core.c |   16 +++++++++++-----
>>  1 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index d03a080..9171aa9 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2250,19 +2250,25 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
>>  {
>>         struct mmc_card *card = host->card;
>>
>> -       if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
>> -               return -EOPNOTSUPP;
>> -
>>         if (!card)
>>                 return -EINVAL;
>>
>> -       if (!mmc_can_reset(card))
>> +       if (!mmc_card_sd(card) && !((host->caps & MMC_CAP_HW_RESET) &&
>> +                                                       host->ops->hw_reset))
>> +               return -EOPNOTSUPP;
>> +
>> +       if (!mmc_card_sd(card) && !mmc_can_reset(card))
>>                 return -EOPNOTSUPP;
>>
>>         mmc_host_clk_hold(host);
>>         mmc_set_clock(host, host->f_init);
>>
>> -       host->ops->hw_reset(host);
>> +       if (mmc_card_sd(card))
>> +               mmc_power_cycle(host, card->ocr);
>> +       else
>> +               host->ops->hw_reset(host);
>> +
>> +       pr_warning("%s: reset device\n", mmc_hostname(host));
>>
>>         /* If the reset has happened, then a status command will fail */
>>         if (check) {
>> --
>> 1.7.2.5
>>
>
> Before we decide to go ahead an continue to hack on the current reset
> code as your patch does, could you possibly consider to do some more
> extensive rework this code?

Sure, challenge accepted!

> Here are some of the ideas that I had in mind.
>
> 1) Invent a new bus_ops callback for reset. Or maybe we should re-use
> the ->power_save|restore ones, since those already exist!?
> 2) Since those bus_ops are specific for sd/mmc/sdio we shall handle
> those related parts in there.
> 3) Then let's mmc_do_hw_reset invoke the above bus_ops.
> 4) Remove the mmc_hw_reset_check() API. Currently only used by mmc_test.

Reuse makes some sense, since bus_ops->power_restore is already a part
of the current hw_reset reset sequence. But do you mean like letting
bus_ops->power_save call host_ops->hw_reset? Won't this interfere with
the other use case of power_save?

What's the current status of mmc_power_save_host and
bus_ops->power_save anyway? As far as I can see, they are no longer
used by the MMC framework. Maybe we can throw away mmc_power_save_host
and bus_ops->power_save, and instead add the reset bus_ops? The naming
of the bus_ops will be more fitting this way.

//Johan