Re: [PATCH] mmc: sdhci: fix up CMD12 sending

Re: [PATCH] mmc: sdhci: fix up CMD12 sending

[Adrian Hunter]

On 14/11/19 1:18 pm, Yangbo Lu wrote:
> The STOP command is disabled for multiple blocks r/w commands
> with auto CMD12, when start to send. However, if there is data
> error, software still needs to send CMD12 according to SD spec.
> This patch is to allow software CMD12 sending for this case.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

Sorry for the delay.  This looks good to me.  Sending a STOP command
on the error path in the auto-CMD12 case should be fine whether it has
been sent already or not.

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 09cdbe8..3041c39 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1326,12 +1326,12 @@ static void sdhci_finish_data(struct sdhci_host *host)
>  
>  	/*
>  	 * Need to send CMD12 if -
> -	 * a) open-ended multiblock transfer (no CMD23)
> +	 * a) open-ended multiblock transfer not using auto CMD12 (no CMD23)
>  	 * b) error in multiblock transfer
>  	 */
>  	if (data->stop &&
> -	    (data->error ||
> -	     !data->mrq->sbc)) {
> +	    ((!data->mrq->sbc && !sdhci_auto_cmd12(host, data->mrq)) ||
> +	     data->error)) {
>  		/*
>  		 * 'cap_cmd_during_tfr' request must not use the command line
>  		 * after mmc_command_done() has been called. It is upper layer's
> @@ -1825,17 +1825,6 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  	sdhci_led_activate(host);
>  
> -	/*
> -	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> -	 * requests if Auto-CMD12 is enabled.
> -	 */
> -	if (sdhci_auto_cmd12(host, mrq)) {
> -		if (mrq->stop) {
> -			mrq->data->stop = NULL;
> -			mrq->stop = NULL;
> -		}
> -	}
> -
>  	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
>  		mrq->cmd->error = -ENOMEDIUM;
>  		sdhci_finish_mrq(host, mrq);
> 

Re: [PATCH] mmc: sdhci: fix up CMD12 sending

[Ulf Hansson]

On Thu, 14 Nov 2019 at 12:18, Yangbo Lu <yangbo.lu@nxp.com> wrote:
>
> The STOP command is disabled for multiple blocks r/w commands
> with auto CMD12, when start to send. However, if there is data
> error, software still needs to send CMD12 according to SD spec.
> This patch is to allow software CMD12 sending for this case.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

Applied for next, thanks!

Let's see how things go, then we can decide whether to add stable tag as well.

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 09cdbe8..3041c39 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1326,12 +1326,12 @@ static void sdhci_finish_data(struct sdhci_host *host)
>
>         /*
>          * Need to send CMD12 if -
> -        * a) open-ended multiblock transfer (no CMD23)
> +        * a) open-ended multiblock transfer not using auto CMD12 (no CMD23)
>          * b) error in multiblock transfer
>          */
>         if (data->stop &&
> -           (data->error ||
> -            !data->mrq->sbc)) {
> +           ((!data->mrq->sbc && !sdhci_auto_cmd12(host, data->mrq)) ||
> +            data->error)) {
>                 /*
>                  * 'cap_cmd_during_tfr' request must not use the command line
>                  * after mmc_command_done() has been called. It is upper layer's
> @@ -1825,17 +1825,6 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>
>         sdhci_led_activate(host);
>
> -       /*
> -        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> -        * requests if Auto-CMD12 is enabled.
> -        */
> -       if (sdhci_auto_cmd12(host, mrq)) {
> -               if (mrq->stop) {
> -                       mrq->data->stop = NULL;
> -                       mrq->stop = NULL;
> -               }
> -       }
> -
>         if (!present || host->flags & SDHCI_DEVICE_DEAD) {
>                 mrq->cmd->error = -ENOMEDIUM;
>                 sdhci_finish_mrq(host, mrq);
> --
> 2.7.4
>

RE: [PATCH] mmc: sdhci: fix up CMD12 sending

[Y.b. Lu]

Hi Uffe and Adrian,

Back again on this topic. Actually we are trying to make the error recovery work after data error of CMD18 in linux-4.14.
With this patch, when CMD18 data error got, manual CMD12 would be sent. And then went into mmc_blk_cmd_recovery(). (Should be mmc_blk_mq_rw_recovery() in latest linux-5.5-rc2.)
In mmc_blk_cmd_recovery(), re-tuning would fail before sending CMD13 on our specific board.
This may be some issue related to specific eMMC card we are investigating.

The above is just background introduction, and you may not care about that:)
I'd like to have some queries on CMD12 usage in MMC driver.
1. It seems CMD12 is always not using ABORT type for sending in sdhci.c. The SDHCI_CMD_ABORTCMD hasn't been used. Is this issue?
2. In block.c, CMD12 uses R1 response for data reading and R1B response for writing. Is it ok to use R1 response for SD? The SD spec mentions only R1B response for CMD12.

Appreciate your helps.
Thanks.

Best regards,
Yangbo Lu

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Tuesday, December 10, 2019 5:52 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: linux-mmc@vger.kernel.org; Adrian Hunter <adrian.hunter@intel.com>
> Subject: Re: [PATCH] mmc: sdhci: fix up CMD12 sending
> 
> On Thu, 14 Nov 2019 at 12:18, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> >
> > The STOP command is disabled for multiple blocks r/w commands
> > with auto CMD12, when start to send. However, if there is data
> > error, software still needs to send CMD12 according to SD spec.
> > This patch is to allow software CMD12 sending for this case.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> 
> Applied for next, thanks!
> 
> Let's see how things go, then we can decide whether to add stable tag as well.
> 
> Kind regards
> Uffe
> 
> 
> > ---
> >  drivers/mmc/host/sdhci.c | 17 +++--------------
> >  1 file changed, 3 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 09cdbe8..3041c39 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1326,12 +1326,12 @@ static void sdhci_finish_data(struct sdhci_host
> *host)
> >
> >         /*
> >          * Need to send CMD12 if -
> > -        * a) open-ended multiblock transfer (no CMD23)
> > +        * a) open-ended multiblock transfer not using auto CMD12 (no
> CMD23)
> >          * b) error in multiblock transfer
> >          */
> >         if (data->stop &&
> > -           (data->error ||
> > -            !data->mrq->sbc)) {
> > +           ((!data->mrq->sbc && !sdhci_auto_cmd12(host, data->mrq)) ||
> > +            data->error)) {
> >                 /*
> >                  * 'cap_cmd_during_tfr' request must not use the
> command line
> >                  * after mmc_command_done() has been called. It is
> upper layer's
> > @@ -1825,17 +1825,6 @@ void sdhci_request(struct mmc_host *mmc,
> struct mmc_request *mrq)
> >
> >         sdhci_led_activate(host);
> >
> > -       /*
> > -        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> > -        * requests if Auto-CMD12 is enabled.
> > -        */
> > -       if (sdhci_auto_cmd12(host, mrq)) {
> > -               if (mrq->stop) {
> > -                       mrq->data->stop = NULL;
> > -                       mrq->stop = NULL;
> > -               }
> > -       }
> > -
> >         if (!present || host->flags & SDHCI_DEVICE_DEAD) {
> >                 mrq->cmd->error = -ENOMEDIUM;
> >                 sdhci_finish_mrq(host, mrq);
> > --
> > 2.7.4
> >

Re: [PATCH] mmc: sdhci: fix up CMD12 sending

[Ulf Hansson]

On Wed, 8 Jan 2020 at 10:37, Y.b. Lu <yangbo.lu@nxp.com> wrote:
>
> Hi Uffe and Adrian,
>
> Back again on this topic. Actually we are trying to make the error recovery work after data error of CMD18 in linux-4.14.
> With this patch, when CMD18 data error got, manual CMD12 would be sent. And then went into mmc_blk_cmd_recovery(). (Should be mmc_blk_mq_rw_recovery() in latest linux-5.5-rc2.)
> In mmc_blk_cmd_recovery(), re-tuning would fail before sending CMD13 on our specific board.
> This may be some issue related to specific eMMC card we are investigating.
>
> The above is just background introduction, and you may not care about that:)
> I'd like to have some queries on CMD12 usage in MMC driver.
> 1. It seems CMD12 is always not using ABORT type for sending in sdhci.c. The SDHCI_CMD_ABORTCMD hasn't been used. Is this issue?

I defer that question to Adrian.

> 2. In block.c, CMD12 uses R1 response for data reading and R1B response for writing. Is it ok to use R1 response for SD? The SD spec mentions only R1B response for CMD12.

I think the specs isn't that clear on this. In this case, the R1B, is
an R1 with an *optional* busy signaling on DAT0, unless it has been
changed lately.

Additionally, as far as can tell, there have been no reports about
problems with the current approach for "reads". Are you saying there
is?

[...]

Kind regards
Uffe

Re: [PATCH] mmc: sdhci: fix up CMD12 sending

[Adrian Hunter]

On 16/01/20 5:36 pm, Ulf Hansson wrote:
> On Wed, 8 Jan 2020 at 10:37, Y.b. Lu <yangbo.lu@nxp.com> wrote:
>>
>> Hi Uffe and Adrian,
>>
>> Back again on this topic. Actually we are trying to make the error recovery work after data error of CMD18 in linux-4.14.
>> With this patch, when CMD18 data error got, manual CMD12 would be sent. And then went into mmc_blk_cmd_recovery(). (Should be mmc_blk_mq_rw_recovery() in latest linux-5.5-rc2.)
>> In mmc_blk_cmd_recovery(), re-tuning would fail before sending CMD13 on our specific board.
>> This may be some issue related to specific eMMC card we are investigating.
>>
>> The above is just background introduction, and you may not care about that:)
>> I'd like to have some queries on CMD12 usage in MMC driver.
>> 1. It seems CMD12 is always not using ABORT type for sending in sdhci.c. The SDHCI_CMD_ABORTCMD hasn't been used. Is this issue?
> 

AFAIK it is not an issue, but support for it can be added if you need it.

> 
>> 2. In block.c, CMD12 uses R1 response for data reading and R1B response for writing. Is it ok to use R1 response for SD? The SD spec mentions only R1B response for CMD12.
> 
> I think the specs isn't that clear on this. In this case, the R1B, is
> an R1 with an *optional* busy signaling on DAT0, unless it has been
> changed lately.

Also SDHCI offers SDHCI_QUIRK2_STOP_WITH_TC to turn all CMD12 responses into R1B

> 
> Additionally, as far as can tell, there have been no reports about
> problems with the current approach for "reads". Are you saying there
> is?
> 
> [...]
> 
> Kind regards
> Uffe
>