[PATCH 0/6] mmc: Some misc patches

[PATCH 0/6] mmc: Some misc patches

[Adrian Hunter]

Hi

Here are some miscellaneous changes.


Adrian Hunter (6):
      mmc: Fix use of wrong device in mmc_gpiod_free_cd()
      mmc: Fix incorrect warning when setting 0 Hz via debugfs
      mmc: It is not an error for the card to be removed while suspended
      mmc: block: Fix error recovery stop cmd timeout calculation
      mmc: block: Fix SD card stop cmd response type
      mmc: sdhci: Transfer Complete has higher priority than Data Timeout Error

 drivers/mmc/card/block.c     | 30 ++++++++++++++++++++++++------
 drivers/mmc/core/core.c      |  2 +-
 drivers/mmc/core/mmc.c       |  2 +-
 drivers/mmc/core/sd.c        |  2 +-
 drivers/mmc/core/slot-gpio.c |  2 +-
 drivers/mmc/host/sdhci.c     | 13 +++++++------
 6 files changed, 35 insertions(+), 16 deletions(-)


Regards
Adrian

[PATCH 1/6] mmc: Fix use of wrong device in mmc_gpiod_free_cd()

[Adrian Hunter]

mmc_gpiod_free_cd() is paired with mmc_gpiod_request_cd()
and both must reference the same device which is the
actual host controller device not the mmc_host class
device.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/slot-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index e3fce44..06616cd 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -392,7 +392,7 @@ void mmc_gpiod_free_cd(struct mmc_host *host)
 		host->slot.cd_irq = -EINVAL;
 	}
 
-	devm_gpiod_put(&host->class_dev, ctx->cd_gpio);
+	devm_gpiod_put(host->parent, ctx->cd_gpio);
 
 	ctx->cd_gpio = NULL;
 }
-- 
1.8.3.2

[PATCH 2/6] mmc: Fix incorrect warning when setting 0 Hz via debugfs

[Adrian Hunter]

It is possible to turn off the card clock by setting
the frequency to zero via debugfs e.g.

	echo 0 > /sys/kernel/debug/mmc0/clock

However that produces an incorrect warning that is
designed to warn if the frequency is below the minimum
operating frequency.  So correct the warning.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e2e1dd4..16aa7bd 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -995,7 +995,7 @@ void mmc_set_chip_select(struct mmc_host *host, int mode)
  */
 static void __mmc_set_clock(struct mmc_host *host, unsigned int hz)
 {
-	WARN_ON(hz < host->f_min);
+	WARN_ON(hz && hz < host->f_min);
 
 	if (hz > host->f_max)
 		hz = host->f_max;
-- 
1.8.3.2

[PATCH 3/6] mmc: It is not an error for the card to be removed while suspended

[Adrian Hunter]

A removable card can be removed while it is runtime suspended.
Do not print an error message.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 2 +-
 drivers/mmc/core/sd.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index ce11d89..1d827eb 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1804,7 +1804,7 @@ static int mmc_runtime_resume(struct mmc_host *host)
 		return 0;
 
 	err = _mmc_resume(host);
-	if (err)
+	if (err && (err != -ENOMEDIUM || (host->caps & MMC_CAP_NONREMOVABLE)))
 		pr_err("%s: error %d doing aggessive resume\n",
 			mmc_hostname(host), err);
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 2591388..28089b3 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1178,7 +1178,7 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
 		return 0;
 
 	err = _mmc_sd_resume(host);
-	if (err)
+	if (err && (err != -ENOMEDIUM || (host->caps & MMC_CAP_NONREMOVABLE)))
 		pr_err("%s: error %d doing aggessive resume\n",
 			mmc_hostname(host), err);
 
-- 
1.8.3.2

[PATCH 4/6] mmc: block: Fix error recovery stop cmd timeout calculation

[Adrian Hunter]

Data timeout has two components: one in nanoseconds
and one in clock cycles.  Clock cycles were not being
added when using the data timeout for the error
recovery stop cmd timeout.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/card/block.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 5d27997..c3770dd 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -945,9 +945,19 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
 	 */
 	if (R1_CURRENT_STATE(status) == R1_STATE_DATA ||
 	    R1_CURRENT_STATE(status) == R1_STATE_RCV) {
-		err = send_stop(card,
-			DIV_ROUND_UP(brq->data.timeout_ns, 1000000),
-			req, gen_err, &stop_status);
+		unsigned int timeout_ms, clock;
+
+		timeout_ms = DIV_ROUND_UP(brq->data.timeout_ns, 1000000);
+		if (brq->data.timeout_clks) {
+			clock = card->host->actual_clock;
+			if (!clock)
+				clock = card->host->ios.clock;
+			if (clock) {
+				clock = DIV_ROUND_UP(clock, 1000);
+				timeout_ms += brq->data.timeout_clks / clock;
+			}
+		}
+		err = send_stop(card, timeout_ms, req, gen_err, &stop_status);
 		if (err) {
 			pr_err("%s: error %d sending stop command\n",
 			       req->rq_disk->disk_name, err);
-- 
1.8.3.2

[PATCH 5/6] mmc: block: Fix SD card stop cmd response type

[Adrian Hunter]

Nowhere in the SD Association Specifications does
it state that the stop command has an R1 response
type.  It is always R1B.  Change accordingly.

Note that, for SD cards, this puts the situation back
to what it was prior to commit
bcc3e1726d827c2d6f62f0e0e7bbc99eed7ad925.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/card/block.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c3770dd..0736efb 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -801,6 +801,9 @@ static int send_stop(struct mmc_card *card, unsigned int timeout_ms,
 	if (host->max_busy_timeout && (timeout_ms > host->max_busy_timeout))
 		use_r1b_resp = false;
 
+	if (!mmc_card_mmc(card))
+		use_r1b_resp = true;
+
 	cmd.opcode = MMC_STOP_TRANSMISSION;
 	if (use_r1b_resp) {
 		cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
@@ -1436,9 +1439,14 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	if (rq_data_dir(req) == READ) {
 		brq->cmd.opcode = readcmd;
 		brq->data.flags |= MMC_DATA_READ;
-		if (brq->mrq.stop)
-			brq->stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
-					MMC_CMD_AC;
+		if (brq->mrq.stop) {
+			if (mmc_card_mmc(card))
+				brq->stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
+						  MMC_CMD_AC;
+			else
+				brq->stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1B |
+						  MMC_CMD_AC;
+		}
 	} else {
 		brq->cmd.opcode = writecmd;
 		brq->data.flags |= MMC_DATA_WRITE;
-- 
1.8.3.2

[PATCH 6/6] mmc: sdhci: Transfer Complete has higher priority than Data Timeout Error

[Adrian Hunter]

According to the SD Host Controller Standard Specification:

"...Transfer Complete has higher priority than Data Timeout
Error. If both bits are set to 1, execution of a command
can be considered to be completed."

Adjust the checking of SDHCI_INT_DATA_TIMEOUT and
SDHCI_INT_DATA_END (Transfer Complete) accordingly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7481bd8..7a8586f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2329,11 +2329,6 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 * above in sdhci_cmd_irq().
 		 */
 		if (host->cmd && (host->cmd->flags & MMC_RSP_BUSY)) {
-			if (intmask & SDHCI_INT_DATA_TIMEOUT) {
-				host->cmd->error = -ETIMEDOUT;
-				tasklet_schedule(&host->finish_tasklet);
-				return;
-			}
 			if (intmask & SDHCI_INT_DATA_END) {
 				/*
 				 * Some cards handle busy-end interrupt
@@ -2346,6 +2341,11 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 					host->busy_handle = 1;
 				return;
 			}
+			if (intmask & SDHCI_INT_DATA_TIMEOUT) {
+				host->cmd->error = -ETIMEDOUT;
+				tasklet_schedule(&host->finish_tasklet);
+				return;
+			}
 		}
 
 		pr_err("%s: Got data interrupt 0x%08x even "
@@ -2356,7 +2356,8 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		return;
 	}
 
-	if (intmask & SDHCI_INT_DATA_TIMEOUT)
+	if ((intmask & SDHCI_INT_DATA_TIMEOUT) &&
+	    !(intmask & SDHCI_INT_DATA_END))
 		host->data->error = -ETIMEDOUT;
 	else if (intmask & SDHCI_INT_DATA_END_BIT)
 		host->data->error = -EILSEQ;
-- 
1.8.3.2

Re: [PATCH 3/6] mmc: It is not an error for the card to be removed while suspended

[Ulf Hansson]

On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> A removable card can be removed while it is runtime suspended.
> Do not print an error message.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/mmc.c | 2 +-
>  drivers/mmc/core/sd.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index ce11d89..1d827eb 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1804,7 +1804,7 @@ static int mmc_runtime_resume(struct mmc_host *host)
>                 return 0;
>
>         err = _mmc_resume(host);
> -       if (err)
> +       if (err && (err != -ENOMEDIUM || (host->caps & MMC_CAP_NONREMOVABLE)))

The check for NONREMOVABLE cap shouldn't be needed!? I mean -ENOMEDIUM
can't be set for such devices anyway.

>                 pr_err("%s: error %d doing aggessive resume\n",
>                         mmc_hostname(host), err);
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 2591388..28089b3 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1178,7 +1178,7 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
>                 return 0;
>
>         err = _mmc_sd_resume(host);
> -       if (err)
> +       if (err && (err != -ENOMEDIUM || (host->caps & MMC_CAP_NONREMOVABLE)))

Same comment as above.

>                 pr_err("%s: error %d doing aggessive resume\n",
>                         mmc_hostname(host), err);
>
> --
> 1.8.3.2
>

Kind regards
Uffe

Re: [PATCH 4/6] mmc: block: Fix error recovery stop cmd timeout calculation

[Ulf Hansson]

On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Data timeout has two components: one in nanoseconds
> and one in clock cycles.  Clock cycles were not being
> added when using the data timeout for the error
> recovery stop cmd timeout.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/card/block.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 5d27997..c3770dd 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -945,9 +945,19 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
>          */
>         if (R1_CURRENT_STATE(status) == R1_STATE_DATA ||
>             R1_CURRENT_STATE(status) == R1_STATE_RCV) {
> -               err = send_stop(card,
> -                       DIV_ROUND_UP(brq->data.timeout_ns, 1000000),
> -                       req, gen_err, &stop_status);
> +               unsigned int timeout_ms, clock;
> +
> +               timeout_ms = DIV_ROUND_UP(brq->data.timeout_ns, 1000000);
> +               if (brq->data.timeout_clks) {
> +                       clock = card->host->actual_clock;
> +                       if (!clock)
> +                               clock = card->host->ios.clock;
> +                       if (clock) {
> +                               clock = DIV_ROUND_UP(clock, 1000);
> +                               timeout_ms += brq->data.timeout_clks / clock;
> +                       }

This pieces of code seems like it should be provided from
mmc/core/core.c. Could we maybe even have it close to
mmc_set_data_timeout(), since it somewhat related, and thus export a
new function for it?

Kind regards
Uffe

> +               }
> +               err = send_stop(card, timeout_ms, req, gen_err, &stop_status);
>                 if (err) {
>                         pr_err("%s: error %d sending stop command\n",
>                                req->rq_disk->disk_name, err);
> --
> 1.8.3.2
>

Re: [PATCH 5/6] mmc: block: Fix SD card stop cmd response type

[Ulf Hansson]

On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Nowhere in the SD Association Specifications does
> it state that the stop command has an R1 response
> type.  It is always R1B.  Change accordingly.

It depends on how detailed you read the spec. :-)

First, R1B is the same as R1, but with optional busy signalling on DAT0.

Just reading the table listing CMDS their related response types,
confirms what you are saying. CMD12 has an R1B.

Though, going into the details of the "Timing" section where this is
clearly visualized in diagrams, you realize there are no busy
signalling associated with a DATA READ, only for DATA WRITE. It is
also indicated in earlier sections of the spec when "DATA READ/WRITE
sequences are described", but I think the "Timing" section describes
this the best.

Kind regards
Uffe

Re: [PATCH 3/6] mmc: It is not an error for the card to be removed while suspended

[Adrian Hunter]

On 25/09/2014 11:27 a.m., Ulf Hansson wrote:
> On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> A removable card can be removed while it is runtime suspended.
>> Do not print an error message.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>   drivers/mmc/core/mmc.c | 2 +-
>>   drivers/mmc/core/sd.c  | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index ce11d89..1d827eb 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1804,7 +1804,7 @@ static int mmc_runtime_resume(struct mmc_host *host)
>>                  return 0;
>>
>>          err = _mmc_resume(host);
>> -       if (err)
>> +       if (err && (err != -ENOMEDIUM || (host->caps & MMC_CAP_NONREMOVABLE)))
>
> The check for NONREMOVABLE cap shouldn't be needed!? I mean -ENOMEDIUM
> can't be set for such devices anyway.

So it would be a bug if it did return -ENOMEDIUM so we should
definitely print the error message, which is what the code does.

>
>>                  pr_err("%s: error %d doing aggessive resume\n",
>>                          mmc_hostname(host), err);
>>
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 2591388..28089b3 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -1178,7 +1178,7 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
>>                  return 0;
>>
>>          err = _mmc_sd_resume(host);
>> -       if (err)
>> +       if (err && (err != -ENOMEDIUM || (host->caps & MMC_CAP_NONREMOVABLE)))
>
> Same comment as above.
>
>>                  pr_err("%s: error %d doing aggessive resume\n",
>>                          mmc_hostname(host), err);
>>
>> --
>> 1.8.3.2
>>
>
> Kind regards
> Uffe
>

Re: [PATCH 1/6] mmc: Fix use of wrong device in mmc_gpiod_free_cd()

[Ulf Hansson]

On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> mmc_gpiod_free_cd() is paired with mmc_gpiod_request_cd()
> and both must reference the same device which is the
> actual host controller device not the mmc_host class
> device.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks! Applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/core/slot-gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index e3fce44..06616cd 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -392,7 +392,7 @@ void mmc_gpiod_free_cd(struct mmc_host *host)
>                 host->slot.cd_irq = -EINVAL;
>         }
>
> -       devm_gpiod_put(&host->class_dev, ctx->cd_gpio);
> +       devm_gpiod_put(host->parent, ctx->cd_gpio);
>
>         ctx->cd_gpio = NULL;
>  }
> --
> 1.8.3.2
>

Re: [PATCH 2/6] mmc: Fix incorrect warning when setting 0 Hz via debugfs

[Ulf Hansson]

On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> It is possible to turn off the card clock by setting
> the frequency to zero via debugfs e.g.
>
>         echo 0 > /sys/kernel/debug/mmc0/clock
>
> However that produces an incorrect warning that is
> designed to warn if the frequency is below the minimum
> operating frequency.  So correct the warning.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks! Applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/core/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e2e1dd4..16aa7bd 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -995,7 +995,7 @@ void mmc_set_chip_select(struct mmc_host *host, int mode)
>   */
>  static void __mmc_set_clock(struct mmc_host *host, unsigned int hz)
>  {
> -       WARN_ON(hz < host->f_min);
> +       WARN_ON(hz && hz < host->f_min);
>
>         if (hz > host->f_max)
>                 hz = host->f_max;
> --
> 1.8.3.2
>

Re: [PATCH 5/6] mmc: block: Fix SD card stop cmd response type

[Adrian Hunter]

On 25/09/14 12:20, Ulf Hansson wrote:
> On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Nowhere in the SD Association Specifications does
>> it state that the stop command has an R1 response
>> type.  It is always R1B.  Change accordingly.
> 
> It depends on how detailed you read the spec. :-)
> 
> First, R1B is the same as R1, but with optional busy signalling on DAT0.

Not exactly:

"R1b is identical to R1 with an optional busy signal
transmitted on the data line. The card may become
busy after receiving these commands based on its
state prior to the command reception. The Host shall
check for busy at the response. Refer to Section 4.12.3
for a detailed description and timing diagrams."

Note it says "The Host shall check for busy at the response."
It does not say "The Host is not affected"

> 
> Just reading the table listing CMDS their related response types,
> confirms what you are saying. CMD12 has an R1B.

Which is explicit and definitive.

> Though, going into the details of the "Timing" section where this is
> clearly visualized in diagrams, you realize there are no busy
> signalling associated with a DATA READ, only for DATA WRITE. It is
> also indicated in earlier sections of the spec when "DATA READ/WRITE
> sequences are described", but I think the "Timing" section describes
> this the best.

You are looking at it only from the point of view of the card.  The host
controller can expect that CMD12/R1b is the only valid combination
because that is what the specification defines.  You can't know what
the host controller will do if you tell it to do CMD12/R1 because that
is outside the spec.

Re: [PATCH 5/6] mmc: block: Fix SD card stop cmd response type

[Ulf Hansson]

On 30 September 2014 13:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 25/09/14 12:20, Ulf Hansson wrote:
>> On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Nowhere in the SD Association Specifications does
>>> it state that the stop command has an R1 response
>>> type.  It is always R1B.  Change accordingly.
>>
>> It depends on how detailed you read the spec. :-)
>>
>> First, R1B is the same as R1, but with optional busy signalling on DAT0.
>
> Not exactly:
>
> "R1b is identical to R1 with an optional busy signal
> transmitted on the data line. The card may become
> busy after receiving these commands based on its
> state prior to the command reception. The Host shall
> check for busy at the response. Refer to Section 4.12.3
> for a detailed description and timing diagrams."
>
> Note it says "The Host shall check for busy at the response."
> It does not say "The Host is not affected"

Sorry, I was a bit unclear. I was referring to the format of the response.

>
>>
>> Just reading the table listing CMDS their related response types,
>> confirms what you are saying. CMD12 has an R1B.
>
> Which is explicit and definitive.
>
>> Though, going into the details of the "Timing" section where this is
>> clearly visualized in diagrams, you realize there are no busy
>> signalling associated with a DATA READ, only for DATA WRITE. It is
>> also indicated in earlier sections of the spec when "DATA READ/WRITE
>> sequences are described", but I think the "Timing" section describes
>> this the best.
>
> You are looking at it only from the point of view of the card. The host
> controller can expect that CMD12/R1b is the only valid combination
> because that is what the specification defines.  You can't know what
> the host controller will do if you tell it to do CMD12/R1 because that
> is outside the spec.
>

It doesn't matter from what point of view we look at it. It's all
about the details of the spec, which tells us there are no busy
signalling involved with a READ. HW designers of MMC controllers
should know this as well.

Unless you really are fixing a regression here, I can't see the need
for your patch.

Kind regards
Uffe

Re: [PATCH 5/6] mmc: block: Fix SD card stop cmd response type

[Adrian Hunter]

On 30/09/14 15:09, Ulf Hansson wrote:
> On 30 September 2014 13:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 25/09/14 12:20, Ulf Hansson wrote:
>>> On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Nowhere in the SD Association Specifications does
>>>> it state that the stop command has an R1 response
>>>> type.  It is always R1B.  Change accordingly.
>>>
>>> It depends on how detailed you read the spec. :-)
>>>
>>> First, R1B is the same as R1, but with optional busy signalling on DAT0.
>>
>> Not exactly:
>>
>> "R1b is identical to R1 with an optional busy signal
>> transmitted on the data line. The card may become
>> busy after receiving these commands based on its
>> state prior to the command reception. The Host shall
>> check for busy at the response. Refer to Section 4.12.3
>> for a detailed description and timing diagrams."
>>
>> Note it says "The Host shall check for busy at the response."
>> It does not say "The Host is not affected"
> 
> Sorry, I was a bit unclear. I was referring to the format of the response.
> 
>>
>>>
>>> Just reading the table listing CMDS their related response types,
>>> confirms what you are saying. CMD12 has an R1B.
>>
>> Which is explicit and definitive.
>>
>>> Though, going into the details of the "Timing" section where this is
>>> clearly visualized in diagrams, you realize there are no busy
>>> signalling associated with a DATA READ, only for DATA WRITE. It is
>>> also indicated in earlier sections of the spec when "DATA READ/WRITE
>>> sequences are described", but I think the "Timing" section describes
>>> this the best.
>>
>> You are looking at it only from the point of view of the card. The host
>> controller can expect that CMD12/R1b is the only valid combination
>> because that is what the specification defines.  You can't know what
>> the host controller will do if you tell it to do CMD12/R1 because that
>> is outside the spec.
>>
> 
> It doesn't matter from what point of view we look at it. It's all
> about the details of the spec, which tells us there are no busy
> signalling involved with a READ. HW designers of MMC controllers
> should know this as well.

HW designers may well choose to follow the spec.

> 
> Unless you really are fixing a regression here, I can't see the need
> for your patch.

No, I have a host controller that wants to give a TC interrupt on CMD12
which is correct if the response type is R1b but incorrect if the
response type is R1.  However I am anyway fixing that with a quirk because
theoretically MMC is affected too - although not in practice because it uses
CMD23 instead of CMD12.

That got me thinking that we really ought to follow the spec and use R1b.

Re: [PATCH 5/6] mmc: block: Fix SD card stop cmd response type

[Ulf Hansson]

On 30 September 2014 14:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 30/09/14 15:09, Ulf Hansson wrote:
>> On 30 September 2014 13:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 25/09/14 12:20, Ulf Hansson wrote:
>>>> On 23 September 2014 22:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> Nowhere in the SD Association Specifications does
>>>>> it state that the stop command has an R1 response
>>>>> type.  It is always R1B.  Change accordingly.
>>>>
>>>> It depends on how detailed you read the spec. :-)
>>>>
>>>> First, R1B is the same as R1, but with optional busy signalling on DAT0.
>>>
>>> Not exactly:
>>>
>>> "R1b is identical to R1 with an optional busy signal
>>> transmitted on the data line. The card may become
>>> busy after receiving these commands based on its
>>> state prior to the command reception. The Host shall
>>> check for busy at the response. Refer to Section 4.12.3
>>> for a detailed description and timing diagrams."
>>>
>>> Note it says "The Host shall check for busy at the response."
>>> It does not say "The Host is not affected"
>>
>> Sorry, I was a bit unclear. I was referring to the format of the response.
>>
>>>
>>>>
>>>> Just reading the table listing CMDS their related response types,
>>>> confirms what you are saying. CMD12 has an R1B.
>>>
>>> Which is explicit and definitive.
>>>
>>>> Though, going into the details of the "Timing" section where this is
>>>> clearly visualized in diagrams, you realize there are no busy
>>>> signalling associated with a DATA READ, only for DATA WRITE. It is
>>>> also indicated in earlier sections of the spec when "DATA READ/WRITE
>>>> sequences are described", but I think the "Timing" section describes
>>>> this the best.
>>>
>>> You are looking at it only from the point of view of the card. The host
>>> controller can expect that CMD12/R1b is the only valid combination
>>> because that is what the specification defines.  You can't know what
>>> the host controller will do if you tell it to do CMD12/R1 because that
>>> is outside the spec.
>>>
>>
>> It doesn't matter from what point of view we look at it. It's all
>> about the details of the spec, which tells us there are no busy
>> signalling involved with a READ. HW designers of MMC controllers
>> should know this as well.
>
> HW designers may well choose to follow the spec.
>
>>
>> Unless you really are fixing a regression here, I can't see the need
>> for your patch.
>
> No, I have a host controller that wants to give a TC interrupt on CMD12
> which is correct if the response type is R1b but incorrect if the
> response type is R1.  However I am anyway fixing that with a quirk because
> theoretically MMC is affected too - although not in practice because it uses
> CMD23 instead of CMD12.
>
> That got me thinking that we really ought to follow the spec and use R1b.

I will certainly keep this in mind. Likely a similar situation will
occur when I fixup mmc erase/discard.

In principle host drivers must pay attention to MMC_RSP_R1B and act
accordingly both if set and not set. I suppose you will add some extra
quirks around that in your driver.

Anyway, thanks for the discussion, it was useful!

Kind regards
Uffe