[PATCH v4 0/2] mmc: core: hw_reset changes

[PATCH v4 0/2] mmc: core: hw_reset changes

[Johan Rudholm]

Make the mmc_hw_reset routines more generic, so we can easily add a
power cycle of SD cards as well. Also simplify the (e)MMC specific
parts of the reset code.

As I don't have an eMMC device myself, much less one with a reset line,
I'd be very happy if someone could help me test the code with an eMMC?

v4:
 - Rebase onto next

v3:
 - Keep mmc_can_reset
 - Always set bus_mode = MMC_BUSMODE_PUSHPULL in mmc_set_initial_state()

v2:
 - Call the new bus_ops member hw_reset instead of power_reset
 - Create mmc_set_initial_state and call it from mmc_mmc_hw_reset
   instead of mmc_power_up
 - Keep "mmc_hw_reset" naming

Johan Rudholm (2):
  mmc: core: turn hw_reset into a bus_ops
  mmc: sd: add hw_reset callback

 drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
 drivers/mmc/core/core.h |    5 ++++
 drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
 drivers/mmc/core/sd.c   |   13 +++++++++++
 4 files changed, 76 insertions(+), 38 deletions(-)

-- 
1.7.2.5

[PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Johan Rudholm]

Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
it from the new bus_ops member hw_reset. This also lets us add code
for resetting SD cards as well.

Signed-off-by: Johan Rudholm <johanru@axis.com>
---
 drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
 drivers/mmc/core/core.h |    5 ++++
 drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9584bff..492b3e5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
 	mmc_host_clk_release(host);
 }
 
-int mmc_can_reset(struct mmc_card *card)
-{
-	u8 rst_n_function;
-
-	if (!mmc_card_mmc(card))
-		return 0;
-	rst_n_function = card->ext_csd.rst_n_function;
-	if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
-		return 0;
-	return 1;
-}
-EXPORT_SYMBOL(mmc_can_reset);
-
+/* Reset card in a bus-specific way */
 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;
+	int ret;
 
-	if (!card)
+	if (!host->card)
 		return -EINVAL;
 
-	if (!mmc_can_reset(card))
+	if (!host->bus_ops || !host->bus_ops->hw_reset ||
+			host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
 		return -EOPNOTSUPP;
 
-	mmc_host_clk_hold(host);
-	mmc_set_clock(host, host->f_init);
+	ret = host->bus_ops->hw_reset(host, check);
 
-	host->ops->hw_reset(host);
-
-	/* If the reset has happened, then a status command will fail */
-	if (check) {
-		u32 status;
-
-		if (!mmc_send_status(card, &status)) {
-			mmc_host_clk_release(host);
-			return -ENOSYS;
-		}
-	}
-
-	/* Set initial state and call mmc_set_ios */
-	mmc_set_initial_state(host);
+	if (check == MMC_HW_RESET_TEST_CARD)
+		return ret;
 
-	mmc_host_clk_release(host);
+	pr_warn("%s: tried to reset card (status %d)\n",
+						mmc_hostname(host), ret);
 
 	return host->bus_ops->power_restore(host);
 }
 
 int mmc_hw_reset(struct mmc_host *host)
 {
-	return mmc_do_hw_reset(host, 0);
+	return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
 }
 EXPORT_SYMBOL(mmc_hw_reset);
 
 int mmc_hw_reset_check(struct mmc_host *host)
 {
-	return mmc_do_hw_reset(host, 1);
+	return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
 }
 EXPORT_SYMBOL(mmc_hw_reset_check);
 
+int mmc_can_reset(struct mmc_card *card)
+{
+	return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
+}
+EXPORT_SYMBOL(mmc_can_reset);
+
 static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 {
 	host->f_init = freq;
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index d76597c..f6e0a52 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -27,6 +27,11 @@ struct mmc_bus_ops {
 	int (*power_restore)(struct mmc_host *);
 	int (*alive)(struct mmc_host *);
 	int (*shutdown)(struct mmc_host *);
+	int (*hw_reset)(struct mmc_host *, int);
+#define MMC_HW_RESET_RESET	0
+#define MMC_HW_RESET_TEST	1
+#define MMC_HW_RESET_CHECK	2
+#define MMC_HW_RESET_TEST_CARD	3
 };
 
 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 02ad792..7ffa3f2 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1821,6 +1821,45 @@ static int mmc_power_restore(struct mmc_host *host)
 	return ret;
 }
 
+static int mmc_mmc_hw_reset(struct mmc_host *host, int test)
+{
+	struct mmc_card *card = host->card;
+	u8 rst_n_function;
+
+	rst_n_function = card->ext_csd.rst_n_function;
+	if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
+		return -EOPNOTSUPP;
+
+	if (test == MMC_HW_RESET_TEST_CARD)
+		return 0;
+
+	if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
+		return -EOPNOTSUPP;
+
+	if (test == MMC_HW_RESET_TEST)
+		return 0;
+
+	mmc_host_clk_hold(host);
+	mmc_set_clock(host, host->f_init);
+
+	host->ops->hw_reset(host);
+
+	if (test == MMC_HW_RESET_CHECK) {
+		u32 status;
+
+		if (!mmc_send_status(card, &status)) {
+			mmc_host_clk_release(host);
+			return -ENOSYS;
+		}
+	}
+
+	mmc_set_initial_state(host);
+
+	mmc_host_clk_release(host);
+
+	return 0;
+}
+
 static const struct mmc_bus_ops mmc_ops = {
 	.remove = mmc_remove,
 	.detect = mmc_detect,
@@ -1831,6 +1870,7 @@ static const struct mmc_bus_ops mmc_ops = {
 	.power_restore = mmc_power_restore,
 	.alive = mmc_alive,
 	.shutdown = mmc_shutdown,
+	.hw_reset = mmc_mmc_hw_reset,
 };
 
 /*
-- 
1.7.2.5

[PATCH v4 2/2] mmc: sd: add hw_reset callback

[Johan Rudholm]

Enable power cycle and re-initialization of SD cards via the hw_reset
bus_ops. Power cycling a buggy SD card sometimes helps it get back on
track.

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

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index d90a6de..a7fa124 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1180,6 +1180,18 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
 	return 0;
 }
 
+static int mmc_sd_hw_reset(struct mmc_host *host, int test)
+{
+	struct mmc_card *card = host->card;
+
+	if (test == MMC_HW_RESET_TEST || test == MMC_HW_RESET_TEST_CARD)
+		return 0;
+
+	mmc_power_cycle(host, card->ocr);
+
+	return 0;
+}
+
 static int mmc_sd_power_restore(struct mmc_host *host)
 {
 	int ret;
@@ -1201,6 +1213,7 @@ static const struct mmc_bus_ops mmc_sd_ops = {
 	.power_restore = mmc_sd_power_restore,
 	.alive = mmc_sd_alive,
 	.shutdown = mmc_sd_suspend,
+	.hw_reset = mmc_sd_hw_reset,
 };
 
 /*
-- 
1.7.2.5

Re: [PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Ulf Hansson]

On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@axis.com> wrote:
> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
> it from the new bus_ops member hw_reset. This also lets us add code
> for resetting SD cards as well.
>
> Signed-off-by: Johan Rudholm <johanru@axis.com>
> ---
>  drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
>  drivers/mmc/core/core.h |    5 ++++
>  drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 9584bff..492b3e5 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>         mmc_host_clk_release(host);
>  }
>
> -int mmc_can_reset(struct mmc_card *card)
> -{
> -       u8 rst_n_function;
> -
> -       if (!mmc_card_mmc(card))
> -               return 0;
> -       rst_n_function = card->ext_csd.rst_n_function;
> -       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
> -               return 0;
> -       return 1;
> -}
> -EXPORT_SYMBOL(mmc_can_reset);

Isn't the mmc_can_reset() function used from mmc_test.c?

> -
> +/* Reset card in a bus-specific way */
>  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;
> +       int ret;
>
> -       if (!card)
> +       if (!host->card)
>                 return -EINVAL;
>
> -       if (!mmc_can_reset(card))

You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.

Well, if you would executed this code with the host claimed and from
the mmc block layer, you can be sure the bus_ops to exist. Now, since
this is an exported function, I think you need to be more safe to also
do the mmc_bus_get|put().

> +       if (!host->bus_ops || !host->bus_ops->hw_reset ||
> +                       host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>                 return -EOPNOTSUPP;
>
> -       mmc_host_clk_hold(host);
> -       mmc_set_clock(host, host->f_init);
> +       ret = host->bus_ops->hw_reset(host, check);

What's going on here? You are invoking the ->hw_reset() callback
twice. That seems odd.

>
> -       host->ops->hw_reset(host);
> -
> -       /* If the reset has happened, then a status command will fail */
> -       if (check) {
> -               u32 status;
> -
> -               if (!mmc_send_status(card, &status)) {
> -                       mmc_host_clk_release(host);
> -                       return -ENOSYS;
> -               }
> -       }
> -
> -       /* Set initial state and call mmc_set_ios */
> -       mmc_set_initial_state(host);
> +       if (check == MMC_HW_RESET_TEST_CARD)
> +               return ret;
>
> -       mmc_host_clk_release(host);
> +       pr_warn("%s: tried to reset card (status %d)\n",
> +                                               mmc_hostname(host), ret);
>
>         return host->bus_ops->power_restore(host);
>  }
>
>  int mmc_hw_reset(struct mmc_host *host)
>  {
> -       return mmc_do_hw_reset(host, 0);
> +       return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>  }
>  EXPORT_SYMBOL(mmc_hw_reset);
>
>  int mmc_hw_reset_check(struct mmc_host *host)
>  {
> -       return mmc_do_hw_reset(host, 1);
> +       return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>  }
>  EXPORT_SYMBOL(mmc_hw_reset_check);
>
> +int mmc_can_reset(struct mmc_card *card)
> +{
> +       return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
> +}
> +EXPORT_SYMBOL(mmc_can_reset);
> +
>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>  {
>         host->f_init = freq;
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index d76597c..f6e0a52 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>         int (*power_restore)(struct mmc_host *);
>         int (*alive)(struct mmc_host *);
>         int (*shutdown)(struct mmc_host *);
> +       int (*hw_reset)(struct mmc_host *, int);
> +#define MMC_HW_RESET_RESET     0
> +#define MMC_HW_RESET_TEST      1
> +#define MMC_HW_RESET_CHECK     2
> +#define MMC_HW_RESET_TEST_CARD 3

Urgh. Is there a way to remove all this? I just don't like all these options.

In fact, I would prefer to have none of them.

>  };
>
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 02ad792..7ffa3f2 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1821,6 +1821,45 @@ static int mmc_power_restore(struct mmc_host *host)
>         return ret;
>  }
>
> +static int mmc_mmc_hw_reset(struct mmc_host *host, int test)
> +{
> +       struct mmc_card *card = host->card;
> +       u8 rst_n_function;
> +
> +       rst_n_function = card->ext_csd.rst_n_function;
> +       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
> +               return -EOPNOTSUPP;
> +
> +       if (test == MMC_HW_RESET_TEST_CARD)
> +               return 0;
> +
> +       if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
> +               return -EOPNOTSUPP;
> +
> +       if (test == MMC_HW_RESET_TEST)
> +               return 0;
> +
> +       mmc_host_clk_hold(host);
> +       mmc_set_clock(host, host->f_init);
> +
> +       host->ops->hw_reset(host);
> +
> +       if (test == MMC_HW_RESET_CHECK) {
> +               u32 status;
> +
> +               if (!mmc_send_status(card, &status)) {
> +                       mmc_host_clk_release(host);
> +                       return -ENOSYS;
> +               }
> +       }
> +
> +       mmc_set_initial_state(host);
> +
> +       mmc_host_clk_release(host);
> +
> +       return 0;
> +}
> +
>  static const struct mmc_bus_ops mmc_ops = {
>         .remove = mmc_remove,
>         .detect = mmc_detect,
> @@ -1831,6 +1870,7 @@ static const struct mmc_bus_ops mmc_ops = {
>         .power_restore = mmc_power_restore,
>         .alive = mmc_alive,
>         .shutdown = mmc_shutdown,
> +       .hw_reset = mmc_mmc_hw_reset,
>  };
>
>  /*
> --
> 1.7.2.5
>

Kind regards
Uffe

Re: [PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Johan Rudholm]

Hi Ulf,

2014-11-25 14:48 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@axis.com> wrote:
>> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
>> it from the new bus_ops member hw_reset. This also lets us add code
>> for resetting SD cards as well.
>>
>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>> ---
>>  drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
>>  drivers/mmc/core/core.h |    5 ++++
>>  drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
>>  3 files changed, 63 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 9584bff..492b3e5 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>>         mmc_host_clk_release(host);
>>  }
>>
>> -int mmc_can_reset(struct mmc_card *card)
>> -{
>> -       u8 rst_n_function;
>> -
>> -       if (!mmc_card_mmc(card))
>> -               return 0;
>> -       rst_n_function = card->ext_csd.rst_n_function;
>> -       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
>> -               return 0;
>> -       return 1;
>> -}
>> -EXPORT_SYMBOL(mmc_can_reset);
>
> Isn't the mmc_can_reset() function used from mmc_test.c?

Yes. Maybe we should move this stuff to mmc_test instead. We could
also move the code that checks if the reset worked or not to mmc_test,
since this is the only place where the check is performed.

>> -
>> +/* Reset card in a bus-specific way */
>>  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;
>> +       int ret;
>>
>> -       if (!card)
>> +       if (!host->card)
>>                 return -EINVAL;
>>
>> -       if (!mmc_can_reset(card))
>
> You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.
>
> Well, if you would executed this code with the host claimed and from
> the mmc block layer, you can be sure the bus_ops to exist. Now, since
> this is an exported function, I think you need to be more safe to also
> do the mmc_bus_get|put().

Got it, thanks.

>> +       if (!host->bus_ops || !host->bus_ops->hw_reset ||
>> +                       host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>>                 return -EOPNOTSUPP;
>>
>> -       mmc_host_clk_hold(host);
>> -       mmc_set_clock(host, host->f_init);
>> +       ret = host->bus_ops->hw_reset(host, check);
>
> What's going on here? You are invoking the ->hw_reset() callback
> twice. That seems odd.
>>
>> -       host->ops->hw_reset(host);
>> -
>> -       /* If the reset has happened, then a status command will fail */
>> -       if (check) {
>> -               u32 status;
>> -
>> -               if (!mmc_send_status(card, &status)) {
>> -                       mmc_host_clk_release(host);
>> -                       return -ENOSYS;
>> -               }
>> -       }
>> -
>> -       /* Set initial state and call mmc_set_ios */
>> -       mmc_set_initial_state(host);
>> +       if (check == MMC_HW_RESET_TEST_CARD)
>> +               return ret;
>>
>> -       mmc_host_clk_release(host);
>> +       pr_warn("%s: tried to reset card (status %d)\n",
>> +                                               mmc_hostname(host), ret);
>>
>>         return host->bus_ops->power_restore(host);
>>  }
>>
>>  int mmc_hw_reset(struct mmc_host *host)
>>  {
>> -       return mmc_do_hw_reset(host, 0);
>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>>  }
>>  EXPORT_SYMBOL(mmc_hw_reset);
>>
>>  int mmc_hw_reset_check(struct mmc_host *host)
>>  {
>> -       return mmc_do_hw_reset(host, 1);
>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>>  }
>>  EXPORT_SYMBOL(mmc_hw_reset_check);
>>
>> +int mmc_can_reset(struct mmc_card *card)
>> +{
>> +       return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
>> +}
>> +EXPORT_SYMBOL(mmc_can_reset);
>> +
>>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>>  {
>>         host->f_init = freq;
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index d76597c..f6e0a52 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>>         int (*power_restore)(struct mmc_host *);
>>         int (*alive)(struct mmc_host *);
>>         int (*shutdown)(struct mmc_host *);
>> +       int (*hw_reset)(struct mmc_host *, int);
>> +#define MMC_HW_RESET_RESET     0
>> +#define MMC_HW_RESET_TEST      1
>> +#define MMC_HW_RESET_CHECK     2
>> +#define MMC_HW_RESET_TEST_CARD 3
>
> Urgh. Is there a way to remove all this? I just don't like all these options.
>
> In fact, I would prefer to have none of them.

If we move the test and check functionality to mmc_test, I think we
can avoid these. What do you think of this approach?

//Johan

Re: [PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Ulf Hansson]

On 27 November 2014 at 10:05, Johan Rudholm <johan.rudholm@axis.com> wrote:
> Hi Ulf,
>
> 2014-11-25 14:48 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
>> On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@axis.com> wrote:
>>> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
>>> it from the new bus_ops member hw_reset. This also lets us add code
>>> for resetting SD cards as well.
>>>
>>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>>> ---
>>>  drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
>>>  drivers/mmc/core/core.h |    5 ++++
>>>  drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
>>>  3 files changed, 63 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 9584bff..492b3e5 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>>>         mmc_host_clk_release(host);
>>>  }
>>>
>>> -int mmc_can_reset(struct mmc_card *card)
>>> -{
>>> -       u8 rst_n_function;
>>> -
>>> -       if (!mmc_card_mmc(card))
>>> -               return 0;
>>> -       rst_n_function = card->ext_csd.rst_n_function;
>>> -       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
>>> -               return 0;
>>> -       return 1;
>>> -}
>>> -EXPORT_SYMBOL(mmc_can_reset);
>>
>> Isn't the mmc_can_reset() function used from mmc_test.c?
>
> Yes. Maybe we should move this stuff to mmc_test instead. We could
> also move the code that checks if the reset worked or not to mmc_test,
> since this is the only place where the check is performed.
>
>>> -
>>> +/* Reset card in a bus-specific way */
>>>  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;
>>> +       int ret;
>>>
>>> -       if (!card)
>>> +       if (!host->card)
>>>                 return -EINVAL;
>>>
>>> -       if (!mmc_can_reset(card))
>>
>> You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.
>>
>> Well, if you would executed this code with the host claimed and from
>> the mmc block layer, you can be sure the bus_ops to exist. Now, since
>> this is an exported function, I think you need to be more safe to also
>> do the mmc_bus_get|put().
>
> Got it, thanks.
>
>>> +       if (!host->bus_ops || !host->bus_ops->hw_reset ||
>>> +                       host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>>>                 return -EOPNOTSUPP;
>>>
>>> -       mmc_host_clk_hold(host);
>>> -       mmc_set_clock(host, host->f_init);
>>> +       ret = host->bus_ops->hw_reset(host, check);
>>
>> What's going on here? You are invoking the ->hw_reset() callback
>> twice. That seems odd.
>>>
>>> -       host->ops->hw_reset(host);
>>> -
>>> -       /* If the reset has happened, then a status command will fail */
>>> -       if (check) {
>>> -               u32 status;
>>> -
>>> -               if (!mmc_send_status(card, &status)) {
>>> -                       mmc_host_clk_release(host);
>>> -                       return -ENOSYS;
>>> -               }
>>> -       }
>>> -
>>> -       /* Set initial state and call mmc_set_ios */
>>> -       mmc_set_initial_state(host);
>>> +       if (check == MMC_HW_RESET_TEST_CARD)
>>> +               return ret;
>>>
>>> -       mmc_host_clk_release(host);
>>> +       pr_warn("%s: tried to reset card (status %d)\n",
>>> +                                               mmc_hostname(host), ret);
>>>
>>>         return host->bus_ops->power_restore(host);
>>>  }
>>>
>>>  int mmc_hw_reset(struct mmc_host *host)
>>>  {
>>> -       return mmc_do_hw_reset(host, 0);
>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>>>  }
>>>  EXPORT_SYMBOL(mmc_hw_reset);
>>>
>>>  int mmc_hw_reset_check(struct mmc_host *host)
>>>  {
>>> -       return mmc_do_hw_reset(host, 1);
>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>>>  }
>>>  EXPORT_SYMBOL(mmc_hw_reset_check);
>>>
>>> +int mmc_can_reset(struct mmc_card *card)
>>> +{
>>> +       return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
>>> +}
>>> +EXPORT_SYMBOL(mmc_can_reset);
>>> +
>>>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>>>  {
>>>         host->f_init = freq;
>>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>>> index d76597c..f6e0a52 100644
>>> --- a/drivers/mmc/core/core.h
>>> +++ b/drivers/mmc/core/core.h
>>> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>>>         int (*power_restore)(struct mmc_host *);
>>>         int (*alive)(struct mmc_host *);
>>>         int (*shutdown)(struct mmc_host *);
>>> +       int (*hw_reset)(struct mmc_host *, int);
>>> +#define MMC_HW_RESET_RESET     0
>>> +#define MMC_HW_RESET_TEST      1
>>> +#define MMC_HW_RESET_CHECK     2
>>> +#define MMC_HW_RESET_TEST_CARD 3
>>
>> Urgh. Is there a way to remove all this? I just don't like all these options.
>>
>> In fact, I would prefer to have none of them.
>
> If we move the test and check functionality to mmc_test, I think we
> can avoid these. What do you think of this approach?

I like that approach.

In principle I think we should have only one API for hardware reset,
typically that should be mmc_hw_reset(). Then, convert
mmc_test_hw_reset() into invoking the mmc_hw_reset() API and let it
handle further tests by itself.

Kind regards
Uffe

Re: [PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Johan Rudholm]

2014-11-27 10:50 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 27 November 2014 at 10:05, Johan Rudholm <johan.rudholm@axis.com> wrote:
>> Hi Ulf,
>>
>> 2014-11-25 14:48 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>> On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@axis.com> wrote:
>>>> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
>>>> it from the new bus_ops member hw_reset. This also lets us add code
>>>> for resetting SD cards as well.
>>>>
>>>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>>>> ---
>>>>  drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
>>>>  drivers/mmc/core/core.h |    5 ++++
>>>>  drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
>>>>  3 files changed, 63 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 9584bff..492b3e5 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>>>>         mmc_host_clk_release(host);
>>>>  }
>>>>
>>>> -int mmc_can_reset(struct mmc_card *card)
>>>> -{
>>>> -       u8 rst_n_function;
>>>> -
>>>> -       if (!mmc_card_mmc(card))
>>>> -               return 0;
>>>> -       rst_n_function = card->ext_csd.rst_n_function;
>>>> -       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
>>>> -               return 0;
>>>> -       return 1;
>>>> -}
>>>> -EXPORT_SYMBOL(mmc_can_reset);
>>>
>>> Isn't the mmc_can_reset() function used from mmc_test.c?
>>
>> Yes. Maybe we should move this stuff to mmc_test instead. We could
>> also move the code that checks if the reset worked or not to mmc_test,
>> since this is the only place where the check is performed.
>>
>>>> -
>>>> +/* Reset card in a bus-specific way */
>>>>  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;
>>>> +       int ret;
>>>>
>>>> -       if (!card)
>>>> +       if (!host->card)
>>>>                 return -EINVAL;
>>>>
>>>> -       if (!mmc_can_reset(card))
>>>
>>> You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.
>>>
>>> Well, if you would executed this code with the host claimed and from
>>> the mmc block layer, you can be sure the bus_ops to exist. Now, since
>>> this is an exported function, I think you need to be more safe to also
>>> do the mmc_bus_get|put().
>>
>> Got it, thanks.
>>
>>>> +       if (!host->bus_ops || !host->bus_ops->hw_reset ||
>>>> +                       host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>>>>                 return -EOPNOTSUPP;
>>>>
>>>> -       mmc_host_clk_hold(host);
>>>> -       mmc_set_clock(host, host->f_init);
>>>> +       ret = host->bus_ops->hw_reset(host, check);
>>>
>>> What's going on here? You are invoking the ->hw_reset() callback
>>> twice. That seems odd.
>>>>
>>>> -       host->ops->hw_reset(host);
>>>> -
>>>> -       /* If the reset has happened, then a status command will fail */
>>>> -       if (check) {
>>>> -               u32 status;
>>>> -
>>>> -               if (!mmc_send_status(card, &status)) {
>>>> -                       mmc_host_clk_release(host);
>>>> -                       return -ENOSYS;
>>>> -               }
>>>> -       }
>>>> -
>>>> -       /* Set initial state and call mmc_set_ios */
>>>> -       mmc_set_initial_state(host);
>>>> +       if (check == MMC_HW_RESET_TEST_CARD)
>>>> +               return ret;
>>>>
>>>> -       mmc_host_clk_release(host);
>>>> +       pr_warn("%s: tried to reset card (status %d)\n",
>>>> +                                               mmc_hostname(host), ret);
>>>>
>>>>         return host->bus_ops->power_restore(host);
>>>>  }
>>>>
>>>>  int mmc_hw_reset(struct mmc_host *host)
>>>>  {
>>>> -       return mmc_do_hw_reset(host, 0);
>>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>>>>  }
>>>>  EXPORT_SYMBOL(mmc_hw_reset);
>>>>
>>>>  int mmc_hw_reset_check(struct mmc_host *host)
>>>>  {
>>>> -       return mmc_do_hw_reset(host, 1);
>>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>>>>  }
>>>>  EXPORT_SYMBOL(mmc_hw_reset_check);
>>>>
>>>> +int mmc_can_reset(struct mmc_card *card)
>>>> +{
>>>> +       return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
>>>> +}
>>>> +EXPORT_SYMBOL(mmc_can_reset);
>>>> +
>>>>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>>>>  {
>>>>         host->f_init = freq;
>>>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>>>> index d76597c..f6e0a52 100644
>>>> --- a/drivers/mmc/core/core.h
>>>> +++ b/drivers/mmc/core/core.h
>>>> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>>>>         int (*power_restore)(struct mmc_host *);
>>>>         int (*alive)(struct mmc_host *);
>>>>         int (*shutdown)(struct mmc_host *);
>>>> +       int (*hw_reset)(struct mmc_host *, int);
>>>> +#define MMC_HW_RESET_RESET     0
>>>> +#define MMC_HW_RESET_TEST      1
>>>> +#define MMC_HW_RESET_CHECK     2
>>>> +#define MMC_HW_RESET_TEST_CARD 3
>>>
>>> Urgh. Is there a way to remove all this? I just don't like all these options.
>>>
>>> In fact, I would prefer to have none of them.
>>
>> If we move the test and check functionality to mmc_test, I think we
>> can avoid these. What do you think of this approach?
>
> I like that approach.
>
> In principle I think we should have only one API for hardware reset,
> typically that should be mmc_hw_reset(). Then, convert
> mmc_test_hw_reset() into invoking the mmc_hw_reset() API and let it
> handle further tests by itself.

The trouble is that mmc_test_hw_reset() needs to check if the reset
was a success after calling host_ops->hw_reset, but before calling
mmc_set_initial_values and bus_ops->power_restore, so some provisions
for this need to be done in mmc_hw_reset. I'll soon send up a new
patchset, please let me know if I'm on the right track.

BR, Johan

Re: [PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Ulf Hansson]

On 28 November 2014 at 16:54, Johan Rudholm <johan.rudholm@axis.com> wrote:
> 2014-11-27 10:50 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
>> On 27 November 2014 at 10:05, Johan Rudholm <johan.rudholm@axis.com> wrote:
>>> Hi Ulf,
>>>
>>> 2014-11-25 14:48 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>>> On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@axis.com> wrote:
>>>>> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
>>>>> it from the new bus_ops member hw_reset. This also lets us add code
>>>>> for resetting SD cards as well.
>>>>>
>>>>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>>>>> ---
>>>>>  drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
>>>>>  drivers/mmc/core/core.h |    5 ++++
>>>>>  drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 63 insertions(+), 38 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>> index 9584bff..492b3e5 100644
>>>>> --- a/drivers/mmc/core/core.c
>>>>> +++ b/drivers/mmc/core/core.c
>>>>> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>>>>>         mmc_host_clk_release(host);
>>>>>  }
>>>>>
>>>>> -int mmc_can_reset(struct mmc_card *card)
>>>>> -{
>>>>> -       u8 rst_n_function;
>>>>> -
>>>>> -       if (!mmc_card_mmc(card))
>>>>> -               return 0;
>>>>> -       rst_n_function = card->ext_csd.rst_n_function;
>>>>> -       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
>>>>> -               return 0;
>>>>> -       return 1;
>>>>> -}
>>>>> -EXPORT_SYMBOL(mmc_can_reset);
>>>>
>>>> Isn't the mmc_can_reset() function used from mmc_test.c?
>>>
>>> Yes. Maybe we should move this stuff to mmc_test instead. We could
>>> also move the code that checks if the reset worked or not to mmc_test,
>>> since this is the only place where the check is performed.
>>>
>>>>> -
>>>>> +/* Reset card in a bus-specific way */
>>>>>  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;
>>>>> +       int ret;
>>>>>
>>>>> -       if (!card)
>>>>> +       if (!host->card)
>>>>>                 return -EINVAL;
>>>>>
>>>>> -       if (!mmc_can_reset(card))
>>>>
>>>> You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.
>>>>
>>>> Well, if you would executed this code with the host claimed and from
>>>> the mmc block layer, you can be sure the bus_ops to exist. Now, since
>>>> this is an exported function, I think you need to be more safe to also
>>>> do the mmc_bus_get|put().
>>>
>>> Got it, thanks.
>>>
>>>>> +       if (!host->bus_ops || !host->bus_ops->hw_reset ||
>>>>> +                       host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>>>>>                 return -EOPNOTSUPP;
>>>>>
>>>>> -       mmc_host_clk_hold(host);
>>>>> -       mmc_set_clock(host, host->f_init);
>>>>> +       ret = host->bus_ops->hw_reset(host, check);
>>>>
>>>> What's going on here? You are invoking the ->hw_reset() callback
>>>> twice. That seems odd.
>>>>>
>>>>> -       host->ops->hw_reset(host);
>>>>> -
>>>>> -       /* If the reset has happened, then a status command will fail */
>>>>> -       if (check) {
>>>>> -               u32 status;
>>>>> -
>>>>> -               if (!mmc_send_status(card, &status)) {
>>>>> -                       mmc_host_clk_release(host);
>>>>> -                       return -ENOSYS;
>>>>> -               }
>>>>> -       }
>>>>> -
>>>>> -       /* Set initial state and call mmc_set_ios */
>>>>> -       mmc_set_initial_state(host);
>>>>> +       if (check == MMC_HW_RESET_TEST_CARD)
>>>>> +               return ret;
>>>>>
>>>>> -       mmc_host_clk_release(host);
>>>>> +       pr_warn("%s: tried to reset card (status %d)\n",
>>>>> +                                               mmc_hostname(host), ret);
>>>>>
>>>>>         return host->bus_ops->power_restore(host);
>>>>>  }
>>>>>
>>>>>  int mmc_hw_reset(struct mmc_host *host)
>>>>>  {
>>>>> -       return mmc_do_hw_reset(host, 0);
>>>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>>>>>  }
>>>>>  EXPORT_SYMBOL(mmc_hw_reset);
>>>>>
>>>>>  int mmc_hw_reset_check(struct mmc_host *host)
>>>>>  {
>>>>> -       return mmc_do_hw_reset(host, 1);
>>>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>>>>>  }
>>>>>  EXPORT_SYMBOL(mmc_hw_reset_check);
>>>>>
>>>>> +int mmc_can_reset(struct mmc_card *card)
>>>>> +{
>>>>> +       return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
>>>>> +}
>>>>> +EXPORT_SYMBOL(mmc_can_reset);
>>>>> +
>>>>>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>>>>>  {
>>>>>         host->f_init = freq;
>>>>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>>>>> index d76597c..f6e0a52 100644
>>>>> --- a/drivers/mmc/core/core.h
>>>>> +++ b/drivers/mmc/core/core.h
>>>>> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>>>>>         int (*power_restore)(struct mmc_host *);
>>>>>         int (*alive)(struct mmc_host *);
>>>>>         int (*shutdown)(struct mmc_host *);
>>>>> +       int (*hw_reset)(struct mmc_host *, int);
>>>>> +#define MMC_HW_RESET_RESET     0
>>>>> +#define MMC_HW_RESET_TEST      1
>>>>> +#define MMC_HW_RESET_CHECK     2
>>>>> +#define MMC_HW_RESET_TEST_CARD 3
>>>>
>>>> Urgh. Is there a way to remove all this? I just don't like all these options.
>>>>
>>>> In fact, I would prefer to have none of them.
>>>
>>> If we move the test and check functionality to mmc_test, I think we
>>> can avoid these. What do you think of this approach?
>>
>> I like that approach.
>>
>> In principle I think we should have only one API for hardware reset,
>> typically that should be mmc_hw_reset(). Then, convert
>> mmc_test_hw_reset() into invoking the mmc_hw_reset() API and let it
>> handle further tests by itself.
>
> The trouble is that mmc_test_hw_reset() needs to check if the reset
> was a success after calling host_ops->hw_reset, but before calling

How about always do the CMD13 check when host_ops->hw_reset() exist
and returns suceess? The error handling for CMD13 check could be print
an error. It shouldn't prevent us from proceeding with the rest of
power cycle operations.

> mmc_set_initial_values and bus_ops->power_restore, so some provisions
> for this need to be done in mmc_hw_reset. I'll soon send up a new
> patchset, please let me know if I'm on the right track.
>

Kind regards
Uffe

Re: [PATCH v4 1/2] mmc: core: turn hw_reset into a bus_ops

[Johan Rudholm]

Hi Ulf,

2014-12-01 11:50 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 28 November 2014 at 16:54, Johan Rudholm <johan.rudholm@axis.com> wrote:
>> 2014-11-27 10:50 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>> On 27 November 2014 at 10:05, Johan Rudholm <johan.rudholm@axis.com> wrote:
>>>> Hi Ulf,
>>>>
>>>> 2014-11-25 14:48 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>>>> On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@axis.com> wrote:
>>>>>> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
>>>>>> it from the new bus_ops member hw_reset. This also lets us add code
>>>>>> for resetting SD cards as well.
>>>>>>
>>>>>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>>>>>> ---
>>>>>>  drivers/mmc/core/core.c |   56 +++++++++++++++-------------------------------
>>>>>>  drivers/mmc/core/core.h |    5 ++++
>>>>>>  drivers/mmc/core/mmc.c  |   40 +++++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 63 insertions(+), 38 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>> index 9584bff..492b3e5 100644
>>>>>> --- a/drivers/mmc/core/core.c
>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>>>>>>         mmc_host_clk_release(host);
>>>>>>  }
>>>>>>
>>>>>> -int mmc_can_reset(struct mmc_card *card)
>>>>>> -{
>>>>>> -       u8 rst_n_function;
>>>>>> -
>>>>>> -       if (!mmc_card_mmc(card))
>>>>>> -               return 0;
>>>>>> -       rst_n_function = card->ext_csd.rst_n_function;
>>>>>> -       if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
>>>>>> -               return 0;
>>>>>> -       return 1;
>>>>>> -}
>>>>>> -EXPORT_SYMBOL(mmc_can_reset);
>>>>>
>>>>> Isn't the mmc_can_reset() function used from mmc_test.c?
>>>>
>>>> Yes. Maybe we should move this stuff to mmc_test instead. We could
>>>> also move the code that checks if the reset worked or not to mmc_test,
>>>> since this is the only place where the check is performed.
>>>>
>>>>>> -
>>>>>> +/* Reset card in a bus-specific way */
>>>>>>  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;
>>>>>> +       int ret;
>>>>>>
>>>>>> -       if (!card)
>>>>>> +       if (!host->card)
>>>>>>                 return -EINVAL;
>>>>>>
>>>>>> -       if (!mmc_can_reset(card))
>>>>>
>>>>> You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.
>>>>>
>>>>> Well, if you would executed this code with the host claimed and from
>>>>> the mmc block layer, you can be sure the bus_ops to exist. Now, since
>>>>> this is an exported function, I think you need to be more safe to also
>>>>> do the mmc_bus_get|put().
>>>>
>>>> Got it, thanks.
>>>>
>>>>>> +       if (!host->bus_ops || !host->bus_ops->hw_reset ||
>>>>>> +                       host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>>>>>>                 return -EOPNOTSUPP;
>>>>>>
>>>>>> -       mmc_host_clk_hold(host);
>>>>>> -       mmc_set_clock(host, host->f_init);
>>>>>> +       ret = host->bus_ops->hw_reset(host, check);
>>>>>
>>>>> What's going on here? You are invoking the ->hw_reset() callback
>>>>> twice. That seems odd.
>>>>>>
>>>>>> -       host->ops->hw_reset(host);
>>>>>> -
>>>>>> -       /* If the reset has happened, then a status command will fail */
>>>>>> -       if (check) {
>>>>>> -               u32 status;
>>>>>> -
>>>>>> -               if (!mmc_send_status(card, &status)) {
>>>>>> -                       mmc_host_clk_release(host);
>>>>>> -                       return -ENOSYS;
>>>>>> -               }
>>>>>> -       }
>>>>>> -
>>>>>> -       /* Set initial state and call mmc_set_ios */
>>>>>> -       mmc_set_initial_state(host);
>>>>>> +       if (check == MMC_HW_RESET_TEST_CARD)
>>>>>> +               return ret;
>>>>>>
>>>>>> -       mmc_host_clk_release(host);
>>>>>> +       pr_warn("%s: tried to reset card (status %d)\n",
>>>>>> +                                               mmc_hostname(host), ret);
>>>>>>
>>>>>>         return host->bus_ops->power_restore(host);
>>>>>>  }
>>>>>>
>>>>>>  int mmc_hw_reset(struct mmc_host *host)
>>>>>>  {
>>>>>> -       return mmc_do_hw_reset(host, 0);
>>>>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>>>>>>  }
>>>>>>  EXPORT_SYMBOL(mmc_hw_reset);
>>>>>>
>>>>>>  int mmc_hw_reset_check(struct mmc_host *host)
>>>>>>  {
>>>>>> -       return mmc_do_hw_reset(host, 1);
>>>>>> +       return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>>>>>>  }
>>>>>>  EXPORT_SYMBOL(mmc_hw_reset_check);
>>>>>>
>>>>>> +int mmc_can_reset(struct mmc_card *card)
>>>>>> +{
>>>>>> +       return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(mmc_can_reset);
>>>>>> +
>>>>>>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>>>>>>  {
>>>>>>         host->f_init = freq;
>>>>>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>>>>>> index d76597c..f6e0a52 100644
>>>>>> --- a/drivers/mmc/core/core.h
>>>>>> +++ b/drivers/mmc/core/core.h
>>>>>> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>>>>>>         int (*power_restore)(struct mmc_host *);
>>>>>>         int (*alive)(struct mmc_host *);
>>>>>>         int (*shutdown)(struct mmc_host *);
>>>>>> +       int (*hw_reset)(struct mmc_host *, int);
>>>>>> +#define MMC_HW_RESET_RESET     0
>>>>>> +#define MMC_HW_RESET_TEST      1
>>>>>> +#define MMC_HW_RESET_CHECK     2
>>>>>> +#define MMC_HW_RESET_TEST_CARD 3
>>>>>
>>>>> Urgh. Is there a way to remove all this? I just don't like all these options.
>>>>>
>>>>> In fact, I would prefer to have none of them.
>>>>
>>>> If we move the test and check functionality to mmc_test, I think we
>>>> can avoid these. What do you think of this approach?
>>>
>>> I like that approach.
>>>
>>> In principle I think we should have only one API for hardware reset,
>>> typically that should be mmc_hw_reset(). Then, convert
>>> mmc_test_hw_reset() into invoking the mmc_hw_reset() API and let it
>>> handle further tests by itself.
>>
>> The trouble is that mmc_test_hw_reset() needs to check if the reset
>> was a success after calling host_ops->hw_reset, but before calling
>
> How about always do the CMD13 check when host_ops->hw_reset() exist
> and returns suceess? The error handling for CMD13 check could be print
> an error. It shouldn't prevent us from proceeding with the rest of
> power cycle operations.

Sorry for having dropped this conversation for so long. I've thought
about the best approach for a while now and I'll soon send out a patch
set with your suggestions incorporated.

Kind regards, Johan