[PATCH v2 0/4] mmc: core: hw_reset changes

[PATCH v2 0/4] 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?

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 (4):
  mmc: core: use mmc_send_status to check hw_reset
  mmc: core: group default initial state
  mmc: core: turn hw_reset into a bus_ops
  mmc: sd: add hw_reset callback

 drivers/mmc/card/mmc_test.c |    3 -
 drivers/mmc/core/core.c     |   99 +++++++++++++-----------------------------
 drivers/mmc/core/core.h     |    5 ++
 drivers/mmc/core/mmc.c      |   37 ++++++++++++++++
 drivers/mmc/core/sd.c       |   13 ++++++
 include/linux/mmc/core.h    |    1 -
 6 files changed, 86 insertions(+), 72 deletions(-)

-- 
1.7.2.5

[PATCH v2 1/4] mmc: core: use mmc_send_status to check hw_reset

[Johan Rudholm]

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 7dc0c85..5d215ee 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2265,15 +2265,9 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
 
 	/* If the reset has happened, then a status command will fail */
 	if (check) {
-		struct mmc_command cmd = {0};
-		int err;
+		u32 status;
 
-		cmd.opcode = MMC_SEND_STATUS;
-		if (!mmc_host_is_spi(card->host))
-			cmd.arg = card->rca << 16;
-		cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
-		err = mmc_wait_for_cmd(card->host, &cmd, 0);
-		if (!err) {
+		if (!mmc_send_status(card, &status)) {
 			mmc_host_clk_release(host);
 			return -ENOSYS;
 		}
-- 
1.7.2.5

Re: [PATCH v2 1/4] mmc: core: use mmc_send_status to check hw_reset

[Ulf Hansson]

On 4 November 2014 16:07, Johan Rudholm <johan.rudholm@axis.com> wrote:
> Signed-off-by: Johan Rudholm <johanru@axis.com>

Thanks! Applied for next.

Kind regards
Uffe

> ---
>  drivers/mmc/core/core.c |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 7dc0c85..5d215ee 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2265,15 +2265,9 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
>
>         /* If the reset has happened, then a status command will fail */
>         if (check) {
> -               struct mmc_command cmd = {0};
> -               int err;
> +               u32 status;
>
> -               cmd.opcode = MMC_SEND_STATUS;
> -               if (!mmc_host_is_spi(card->host))
> -                       cmd.arg = card->rca << 16;
> -               cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
> -               err = mmc_wait_for_cmd(card->host, &cmd, 0);
> -               if (!err) {
> +               if (!mmc_send_status(card, &status)) {
>                         mmc_host_clk_release(host);
>                         return -ENOSYS;
>                 }
> --
> 1.7.2.5
>

[PATCH v2 2/4] mmc: core: group default initial state

[Johan Rudholm]

mmc_do_hw_reset, mmc_power_up and mmc_power_off all set the same
group of initial values, simplify by sticking them together.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5d215ee..2c39d26 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1088,6 +1088,24 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
 	mmc_host_clk_release(host);
 }
 
+/*
+ * Set initial state after a power cycle or a hw_reset.
+ */
+void mmc_set_initial_state(struct mmc_host *host)
+{
+	if (mmc_host_is_spi(host)) {
+		host->ios.chip_select = MMC_CS_HIGH;
+		host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
+	} else {
+		host->ios.chip_select = MMC_CS_DONTCARE;
+		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
+	}
+	host->ios.bus_width = MMC_BUS_WIDTH_1;
+	host->ios.timing = MMC_TIMING_LEGACY;
+
+	mmc_set_ios(host);
+}
+
 /**
  * mmc_vdd_to_ocrbitnum - Convert a voltage to the OCR bit number
  * @vdd:	voltage (mV)
@@ -1534,15 +1552,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
 	mmc_host_clk_hold(host);
 
 	host->ios.vdd = fls(ocr) - 1;
-	if (mmc_host_is_spi(host))
-		host->ios.chip_select = MMC_CS_HIGH;
-	else
-		host->ios.chip_select = MMC_CS_DONTCARE;
-	host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
 	host->ios.power_mode = MMC_POWER_UP;
-	host->ios.bus_width = MMC_BUS_WIDTH_1;
-	host->ios.timing = MMC_TIMING_LEGACY;
-	mmc_set_ios(host);
+	/* Set initial state and call mmc_set_ios */
+	mmc_set_initial_state(host);
 
 	/* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
 	if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
@@ -1582,14 +1594,9 @@ void mmc_power_off(struct mmc_host *host)
 	host->ios.clock = 0;
 	host->ios.vdd = 0;
 
-	if (!mmc_host_is_spi(host)) {
-		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
-		host->ios.chip_select = MMC_CS_DONTCARE;
-	}
 	host->ios.power_mode = MMC_POWER_OFF;
-	host->ios.bus_width = MMC_BUS_WIDTH_1;
-	host->ios.timing = MMC_TIMING_LEGACY;
-	mmc_set_ios(host);
+	/* Set initial state and call mmc_set_ios */
+	mmc_set_initial_state(host);
 
 	/*
 	 * Some configurations, such as the 802.11 SDIO card in the OLPC
@@ -2273,16 +2280,8 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
 		}
 	}
 
-	if (mmc_host_is_spi(host)) {
-		host->ios.chip_select = MMC_CS_HIGH;
-		host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
-	} else {
-		host->ios.chip_select = MMC_CS_DONTCARE;
-		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
-	}
-	host->ios.bus_width = MMC_BUS_WIDTH_1;
-	host->ios.timing = MMC_TIMING_LEGACY;
-	mmc_set_ios(host);
+	/* Set initial state and call mmc_set_ios */
+	mmc_set_initial_state(host);
 
 	mmc_host_clk_release(host);
 
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 443a584..d76597c 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -49,6 +49,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
 void mmc_power_up(struct mmc_host *host, u32 ocr);
 void mmc_power_off(struct mmc_host *host);
 void mmc_power_cycle(struct mmc_host *host, u32 ocr);
+void mmc_set_initial_state(struct mmc_host *host);
 
 static inline void mmc_delay(unsigned int ms)
 {
-- 
1.7.2.5

Re: [PATCH v2 2/4] mmc: core: group default initial state

[Ulf Hansson]

On 4 November 2014 16:07, Johan Rudholm <johan.rudholm@axis.com> wrote:
> mmc_do_hw_reset, mmc_power_up and mmc_power_off all set the same
> group of initial values, simplify by sticking them together.

That's not entirely true. This patch will actually change the
behaviour for the "ios.bus_mode", which isn't exactly what the commit
message are telling us. See more below.

I still think this patch is interesting, since apparently the value of
"ios.bus_mode" hasn't been handled consistently. In principle I like
the approach of this patch, but we need to make sure it's working and
following the specs.

Kind regards
Uffe

>
> Signed-off-by: Johan Rudholm <johanru@axis.com>
> ---
>  drivers/mmc/core/core.c |   49 +++++++++++++++++++++++------------------------
>  drivers/mmc/core/core.h |    1 +
>  2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5d215ee..2c39d26 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1088,6 +1088,24 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
>         mmc_host_clk_release(host);
>  }
>
> +/*
> + * Set initial state after a power cycle or a hw_reset.
> + */
> +void mmc_set_initial_state(struct mmc_host *host)
> +{
> +       if (mmc_host_is_spi(host)) {
> +               host->ios.chip_select = MMC_CS_HIGH;
> +               host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
> +       } else {
> +               host->ios.chip_select = MMC_CS_DONTCARE;
> +               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
> +       }
> +       host->ios.bus_width = MMC_BUS_WIDTH_1;
> +       host->ios.timing = MMC_TIMING_LEGACY;
> +
> +       mmc_set_ios(host);
> +}
> +
>  /**
>   * mmc_vdd_to_ocrbitnum - Convert a voltage to the OCR bit number
>   * @vdd:       voltage (mV)
> @@ -1534,15 +1552,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
>         mmc_host_clk_hold(host);
>
>         host->ios.vdd = fls(ocr) - 1;
> -       if (mmc_host_is_spi(host))
> -               host->ios.chip_select = MMC_CS_HIGH;
> -       else
> -               host->ios.chip_select = MMC_CS_DONTCARE;
> -       host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;

Earlier we used always used MMC_BUSMODE_PUSHPULL, now it will depend on "spi".

>         host->ios.power_mode = MMC_POWER_UP;
> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
> -       host->ios.timing = MMC_TIMING_LEGACY;
> -       mmc_set_ios(host);
> +       /* Set initial state and call mmc_set_ios */
> +       mmc_set_initial_state(host);
>
>         /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
>         if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
> @@ -1582,14 +1594,9 @@ void mmc_power_off(struct mmc_host *host)
>         host->ios.clock = 0;
>         host->ios.vdd = 0;
>
> -       if (!mmc_host_is_spi(host)) {
> -               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
> -               host->ios.chip_select = MMC_CS_DONTCARE;

So in here another change in behaviour, right!?

> -       }
>         host->ios.power_mode = MMC_POWER_OFF;
> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
> -       host->ios.timing = MMC_TIMING_LEGACY;
> -       mmc_set_ios(host);
> +       /* Set initial state and call mmc_set_ios */
> +       mmc_set_initial_state(host);
>
>         /*
>          * Some configurations, such as the 802.11 SDIO card in the OLPC
> @@ -2273,16 +2280,8 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
>                 }
>         }
>
> -       if (mmc_host_is_spi(host)) {
> -               host->ios.chip_select = MMC_CS_HIGH;
> -               host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
> -       } else {
> -               host->ios.chip_select = MMC_CS_DONTCARE;
> -               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
> -       }
> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
> -       host->ios.timing = MMC_TIMING_LEGACY;
> -       mmc_set_ios(host);
> +       /* Set initial state and call mmc_set_ios */
> +       mmc_set_initial_state(host);
>
>         mmc_host_clk_release(host);
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 443a584..d76597c 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -49,6 +49,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
>  void mmc_power_up(struct mmc_host *host, u32 ocr);
>  void mmc_power_off(struct mmc_host *host);
>  void mmc_power_cycle(struct mmc_host *host, u32 ocr);
> +void mmc_set_initial_state(struct mmc_host *host);
>
>  static inline void mmc_delay(unsigned int ms)
>  {
> --
> 1.7.2.5
>

Kind regards
Uffe

Re: [PATCH v2 2/4] mmc: core: group default initial state

[Johan Rudholm]

Hi Ulf,

2014-11-05 11:13 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 4 November 2014 16:07, Johan Rudholm <johan.rudholm@axis.com> wrote:
>> mmc_do_hw_reset, mmc_power_up and mmc_power_off all set the same
>> group of initial values, simplify by sticking them together.
>
> That's not entirely true. This patch will actually change the
> behaviour for the "ios.bus_mode", which isn't exactly what the commit
> message are telling us. See more below.
>
> I still think this patch is interesting, since apparently the value of
> "ios.bus_mode" hasn't been handled consistently. In principle I like
> the approach of this patch, but we need to make sure it's working and
> following the specs.

Thank you for pointing this out. I've double-checked the JEDEC 5.01
spec regarding the bus mode in the context of this initial state, see
below for comments.

>
> Kind regards
> Uffe
>
>>
>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>> ---
>>  drivers/mmc/core/core.c |   49 +++++++++++++++++++++++------------------------
>>  drivers/mmc/core/core.h |    1 +
>>  2 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 5d215ee..2c39d26 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1088,6 +1088,24 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
>>         mmc_host_clk_release(host);
>>  }
>>
>> +/*
>> + * Set initial state after a power cycle or a hw_reset.
>> + */
>> +void mmc_set_initial_state(struct mmc_host *host)
>> +{
>> +       if (mmc_host_is_spi(host)) {
>> +               host->ios.chip_select = MMC_CS_HIGH;
>> +               host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
>> +       } else {
>> +               host->ios.chip_select = MMC_CS_DONTCARE;
>> +               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
>> +       }
>> +       host->ios.bus_width = MMC_BUS_WIDTH_1;
>> +       host->ios.timing = MMC_TIMING_LEGACY;
>> +
>> +       mmc_set_ios(host);
>> +}
>> +
>>  /**
>>   * mmc_vdd_to_ocrbitnum - Convert a voltage to the OCR bit number
>>   * @vdd:       voltage (mV)
>> @@ -1534,15 +1552,9 @@ void mmc_power_up(struct mmc_host *host, u32 ocr)
>>         mmc_host_clk_hold(host);
>>
>>         host->ios.vdd = fls(ocr) - 1;
>> -       if (mmc_host_is_spi(host))
>> -               host->ios.chip_select = MMC_CS_HIGH;
>> -       else
>> -               host->ios.chip_select = MMC_CS_DONTCARE;
>> -       host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
>
> Earlier we used always used MMC_BUSMODE_PUSHPULL, now it will depend on "spi".

The JEDEC spec says that in device identification mode the bus should
be open-drain and not push-pull. So this way should be compliant to
the spec, right? Unfortunately, I will require help in testing this on
a proper eMMC.

I can't find anything in the JEDEC or the SD spec about bus mode in
SPI mode though. However, this behavior is not changed by this patch,
so we should be safe.

>>         host->ios.power_mode = MMC_POWER_UP;
>> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
>> -       host->ios.timing = MMC_TIMING_LEGACY;
>> -       mmc_set_ios(host);
>> +       /* Set initial state and call mmc_set_ios */
>> +       mmc_set_initial_state(host);
>>
>>         /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
>>         if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
>> @@ -1582,14 +1594,9 @@ void mmc_power_off(struct mmc_host *host)
>>         host->ios.clock = 0;
>>         host->ios.vdd = 0;
>>
>> -       if (!mmc_host_is_spi(host)) {
>> -               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
>> -               host->ios.chip_select = MMC_CS_DONTCARE;
>
> So in here another change in behaviour, right!?

Ok, so the behavior for SPI is changed in this case, now we explicitly
set the bus+cs modes and before we left them as they were.

Does this matter though, since we're powering off the device and the
bus+cs modes will be set on the subsequent mmc_power_up?

>
>> -       }
>>         host->ios.power_mode = MMC_POWER_OFF;
>> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
>> -       host->ios.timing = MMC_TIMING_LEGACY;
>> -       mmc_set_ios(host);
>> +       /* Set initial state and call mmc_set_ios */
>> +       mmc_set_initial_state(host);
>>
>>         /*
>>          * Some configurations, such as the 802.11 SDIO card in the OLPC
>> @@ -2273,16 +2280,8 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
>>                 }
>>         }
>>
>> -       if (mmc_host_is_spi(host)) {
>> -               host->ios.chip_select = MMC_CS_HIGH;
>> -               host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
>> -       } else {
>> -               host->ios.chip_select = MMC_CS_DONTCARE;
>> -               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
>> -       }
>> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
>> -       host->ios.timing = MMC_TIMING_LEGACY;
>> -       mmc_set_ios(host);
>> +       /* Set initial state and call mmc_set_ios */
>> +       mmc_set_initial_state(host);
>>
>>         mmc_host_clk_release(host);
>>
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index 443a584..d76597c 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -49,6 +49,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
>>  void mmc_power_up(struct mmc_host *host, u32 ocr);
>>  void mmc_power_off(struct mmc_host *host);
>>  void mmc_power_cycle(struct mmc_host *host, u32 ocr);
>> +void mmc_set_initial_state(struct mmc_host *host);
>>
>>  static inline void mmc_delay(unsigned int ms)
>>  {
>> --
>> 1.7.2.5
>>
>
> Kind regards
> Uffe

//Johan

Re: [PATCH v2 2/4] mmc: core: group default initial state

[Ulf Hansson]

[...]

>>
>> Earlier we used always used MMC_BUSMODE_PUSHPULL, now it will depend on "spi".
>
> The JEDEC spec says that in device identification mode the bus should
> be open-drain and not push-pull. So this way should be compliant to
> the spec, right? Unfortunately, I will require help in testing this on
> a proper eMMC.

For MMC yes, but you will break SD/SDIO in this patch.

The default bus mode at initialization needs to be
MMC_BUSMODE_PUSHPULL, since that's required by the SD/SDIO spec.

The MMC cards are being initialized after SD and SDIO, which means a
switch to MMC_BUSMODE_OPENDRAIN is done in mmc_attach_mmc().
Additionally, to handle resume of MMC cards, mmc_init_card() also do a
switch to MMC_BUSMODE_OPENDRAIN.

>
> I can't find anything in the JEDEC or the SD spec about bus mode in
> SPI mode though. However, this behavior is not changed by this patch,
> so we should be safe.

SPI mode has been removed from the eMMC spec.

The SD spec doesn't have open drain mode but do support SPI mode. That
tells me that we should keep using MMC_BUSMODE_PUSHPULL for SPI mode.

>
>>>         host->ios.power_mode = MMC_POWER_UP;
>>> -       host->ios.bus_width = MMC_BUS_WIDTH_1;
>>> -       host->ios.timing = MMC_TIMING_LEGACY;
>>> -       mmc_set_ios(host);
>>> +       /* Set initial state and call mmc_set_ios */
>>> +       mmc_set_initial_state(host);
>>>
>>>         /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
>>>         if (__mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) == 0)
>>> @@ -1582,14 +1594,9 @@ void mmc_power_off(struct mmc_host *host)
>>>         host->ios.clock = 0;
>>>         host->ios.vdd = 0;
>>>
>>> -       if (!mmc_host_is_spi(host)) {
>>> -               host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
>>> -               host->ios.chip_select = MMC_CS_DONTCARE;
>>
>> So in here another change in behaviour, right!?
>
> Ok, so the behavior for SPI is changed in this case, now we explicitly
> set the bus+cs modes and before we left them as they were.
>
> Does this matter though, since we're powering off the device and the
> bus+cs modes will be set on the subsequent mmc_power_up?

That seems reasonable, it shouldn't be an issue.

Kind regards
Uffe

Re: [PATCH v2 2/4] mmc: core: group default initial state

[Johan Rudholm]

Hi Ulf,

2014-11-06 10:38 GMT+01:00 Ulf Hansson <ulf.hansson@linaro.org>:
> [...]
>
>>>
>>> Earlier we used always used MMC_BUSMODE_PUSHPULL, now it will depend on "spi".
>>
>> The JEDEC spec says that in device identification mode the bus should
>> be open-drain and not push-pull. So this way should be compliant to
>> the spec, right? Unfortunately, I will require help in testing this on
>> a proper eMMC.
>
> For MMC yes, but you will break SD/SDIO in this patch.
>
> The default bus mode at initialization needs to be
> MMC_BUSMODE_PUSHPULL, since that's required by the SD/SDIO spec.
>
> The MMC cards are being initialized after SD and SDIO, which means a
> switch to MMC_BUSMODE_OPENDRAIN is done in mmc_attach_mmc().
> Additionally, to handle resume of MMC cards, mmc_init_card() also do a
> switch to MMC_BUSMODE_OPENDRAIN.
>
>>
>> I can't find anything in the JEDEC or the SD spec about bus mode in
>> SPI mode though. However, this behavior is not changed by this patch,
>> so we should be safe.
>
> SPI mode has been removed from the eMMC spec.
>
> The SD spec doesn't have open drain mode but do support SPI mode. That
> tells me that we should keep using MMC_BUSMODE_PUSHPULL for SPI mode.

Thank you for explaining this. So I think we should be OK by always
choosing MMC_BUSMODE_PUSHPULL, because for MMC cards mmc_init_card is
called from mmc_power_restore which is called after the hw_reset. Will
post a v3 of this patch.

//Johan

[PATCH v2 3/4] 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 reseting SD cards as well.

Signed-off-by: Johan Rudholm <johanru@axis.com>
---
 drivers/mmc/card/mmc_test.c |    3 --
 drivers/mmc/core/core.c     |   48 ++++++++----------------------------------
 drivers/mmc/core/core.h     |    4 +++
 drivers/mmc/core/mmc.c      |   37 +++++++++++++++++++++++++++++++++
 include/linux/mmc/core.h    |    1 -
 5 files changed, 50 insertions(+), 43 deletions(-)

diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 0c0fc52..2cd3385 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -2349,9 +2349,6 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
 	if (err != -EOPNOTSUPP)
 		return err;
 
-	if (!mmc_can_reset(card))
-		return RESULT_UNSUP_CARD;
-
 	return RESULT_UNSUP_HOST;
 }
 
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2c39d26..b35f205 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2239,64 +2239,34 @@ 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);
-
-	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);
+	ret = host->bus_ops->hw_reset(host, check);
 
-	mmc_host_clk_release(host);
+	pr_warn("%s: tried to reset card (%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);
 
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index d76597c..918bbe8 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -27,6 +27,10 @@ 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
 };
 
 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 793c6f7..55e1a97 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1773,6 +1773,42 @@ 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;
+
+	if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
+		return -EOPNOTSUPP;
+
+	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)
+		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,
@@ -1783,6 +1819,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,
 };
 
 /*
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index f206e29..d7e02a7 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -182,7 +182,6 @@ extern int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
 			      bool is_rel_write);
 extern int mmc_hw_reset(struct mmc_host *host);
 extern int mmc_hw_reset_check(struct mmc_host *host);
-extern int mmc_can_reset(struct mmc_card *card);
 
 extern void mmc_set_data_timeout(struct mmc_data *, const struct mmc_card *);
 extern unsigned int mmc_align_data_size(struct mmc_card *, unsigned int);
-- 
1.7.2.5

Re: [PATCH v2 3/4] mmc: core: turn hw_reset into a bus_ops

[Adrian Hunter]

On 04/11/14 17:07, Johan Rudholm 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 reseting SD cards as well.
> 
> Signed-off-by: Johan Rudholm <johanru@axis.com>
> ---
>  drivers/mmc/card/mmc_test.c |    3 --
>  drivers/mmc/core/core.c     |   48 ++++++++----------------------------------
>  drivers/mmc/core/core.h     |    4 +++
>  drivers/mmc/core/mmc.c      |   37 +++++++++++++++++++++++++++++++++
>  include/linux/mmc/core.h    |    1 -
>  5 files changed, 50 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
> index 0c0fc52..2cd3385 100644
> --- a/drivers/mmc/card/mmc_test.c
> +++ b/drivers/mmc/card/mmc_test.c
> @@ -2349,9 +2349,6 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>  	if (err != -EOPNOTSUPP)
>  		return err;
>  
> -	if (!mmc_can_reset(card))
> -		return RESULT_UNSUP_CARD;
> -

I would like to retain the ability for mmc_test to distinguish
RESULT_UNSUP_CARD from RESULT_UNSUP_HOST

>  	return RESULT_UNSUP_HOST;
>  }
>  
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 2c39d26..b35f205 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2239,64 +2239,34 @@ 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);
> -
> -	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);
> +	ret = host->bus_ops->hw_reset(host, check);
>  
> -	mmc_host_clk_release(host);
> +	pr_warn("%s: tried to reset card (%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);
>  
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index d76597c..918bbe8 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -27,6 +27,10 @@ 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
>  };
>  
>  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 793c6f7..55e1a97 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1773,6 +1773,42 @@ 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;
> +
> +	if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
> +		return -EOPNOTSUPP;
> +
> +	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)
> +		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,
> @@ -1783,6 +1819,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,
>  };
>  
>  /*
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index f206e29..d7e02a7 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -182,7 +182,6 @@ extern int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
>  			      bool is_rel_write);
>  extern int mmc_hw_reset(struct mmc_host *host);
>  extern int mmc_hw_reset_check(struct mmc_host *host);
> -extern int mmc_can_reset(struct mmc_card *card);
>  
>  extern void mmc_set_data_timeout(struct mmc_data *, const struct mmc_card *);
>  extern unsigned int mmc_align_data_size(struct mmc_card *, unsigned int);
> 

Re: [PATCH v2 3/4] mmc: core: turn hw_reset into a bus_ops

[Johan Rudholm]

Hi Adrian,

2014-11-05 13:10 GMT+01:00 Adrian Hunter <adrian.hunter@intel.com>:
> On 04/11/14 17:07, Johan Rudholm 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 reseting SD cards as well.
>>
>> Signed-off-by: Johan Rudholm <johanru@axis.com>
>> ---
>>  drivers/mmc/card/mmc_test.c |    3 --
>>  drivers/mmc/core/core.c     |   48 ++++++++----------------------------------
>>  drivers/mmc/core/core.h     |    4 +++
>>  drivers/mmc/core/mmc.c      |   37 +++++++++++++++++++++++++++++++++
>>  include/linux/mmc/core.h    |    1 -
>>  5 files changed, 50 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
>> index 0c0fc52..2cd3385 100644
>> --- a/drivers/mmc/card/mmc_test.c
>> +++ b/drivers/mmc/card/mmc_test.c
>> @@ -2349,9 +2349,6 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>>       if (err != -EOPNOTSUPP)
>>               return err;
>>
>> -     if (!mmc_can_reset(card))
>> -             return RESULT_UNSUP_CARD;
>> -
>
> I would like to retain the ability for mmc_test to distinguish
> RESULT_UNSUP_CARD from RESULT_UNSUP_HOST

Ok, coming up in v3, thanks.

//Johan

[PATCH v2 4/4] 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 0c44510..383320e 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1177,6 +1177,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)
+		return 0;
+
+	mmc_power_cycle(host, card->ocr);
+
+	return 0;
+}
+
 static int mmc_sd_power_restore(struct mmc_host *host)
 {
 	int ret;
@@ -1198,6 +1210,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