[PATCH v2 0/2] Fixes for command errors during tuning

[PATCH v2 0/2] Fixes for command errors during tuning

[Faiz Abbas]

These patches fix the following error message in dra7xx boards:

[4.833198] mmc1: Got data interrupt 0x00000002 even though no data
operation was in progress.

Tested with 100 times boot tests on dra71x-evm, dra72x-evm and
dra7xx-evm.

v2:
Added EXPORT_SYMBOL_GPL for sdhci_cmd_err and sdhci_send_command to fix
errors when built as module.

Faiz Abbas (2):
  mmc: sdhci: Add platform_cmd_err() to sdhci_ops
  mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning

 drivers/mmc/host/sdhci-omap.c | 24 +++++++++++++++++++++
 drivers/mmc/host/sdhci.c      | 40 +++++++++++++++++++++++------------
 drivers/mmc/host/sdhci.h      |  4 ++++
 3 files changed, 54 insertions(+), 14 deletions(-)

-- 
2.19.2

[PATCH v2 1/2] mmc: sdhci: Add platform_cmd_err() to sdhci_ops

[Faiz Abbas]

Some platforms might need a custom method for handling command error
interrupts. Add a callback to sdhci_ops to facilitate the same. Move
default command error handling to its own non-static function so it can
be called from platform drivers. Also make sdhci_finish_command()
non-static.

Fixes: 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
callback")
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci.c | 40 ++++++++++++++++++++++++++--------------
 drivers/mmc/host/sdhci.h |  4 ++++
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a8141ff9be03..ff60b1830896 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1445,7 +1445,7 @@ static void sdhci_read_rsp_136(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 }
 
-static void sdhci_finish_command(struct sdhci_host *host)
+void sdhci_finish_command(struct sdhci_host *host)
 {
 	struct mmc_command *cmd = host->cmd;
 
@@ -1495,6 +1495,8 @@ static void sdhci_finish_command(struct sdhci_host *host)
 			sdhci_finish_mrq(host, cmd->mrq);
 	}
 }
+EXPORT_SYMBOL_GPL(sdhci_finish_command);
+
 
 static u16 sdhci_get_preset_value(struct sdhci_host *host)
 {
@@ -2780,6 +2782,26 @@ static void sdhci_timeout_data_timer(struct timer_list *t)
  *                                                                           *
 \*****************************************************************************/
 
+void sdhci_cmd_err(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
+{
+		if (intmask & SDHCI_INT_TIMEOUT)
+			host->cmd->error = -ETIMEDOUT;
+		else
+			host->cmd->error = -EILSEQ;
+
+		/* Treat data command CRC error the same as data CRC error */
+		if (host->cmd->data &&
+		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
+		     SDHCI_INT_CRC) {
+			host->cmd = NULL;
+			*intmask_p |= SDHCI_INT_DATA_CRC;
+			return;
+		}
+
+		sdhci_finish_mrq(host, host->cmd->mrq);
+}
+EXPORT_SYMBOL_GPL(sdhci_cmd_err);
+
 static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
 {
 	/* Handle auto-CMD12 error */
@@ -2813,21 +2835,11 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
 
 	if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
 		       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
-		if (intmask & SDHCI_INT_TIMEOUT)
-			host->cmd->error = -ETIMEDOUT;
+		if (host->ops->platform_cmd_err)
+			host->ops->platform_cmd_err(host, intmask, intmask_p);
 		else
-			host->cmd->error = -EILSEQ;
-
-		/* Treat data command CRC error the same as data CRC error */
-		if (host->cmd->data &&
-		    (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
-		     SDHCI_INT_CRC) {
-			host->cmd = NULL;
-			*intmask_p |= SDHCI_INT_DATA_CRC;
-			return;
-		}
+			sdhci_cmd_err(host, intmask, intmask_p);
 
-		sdhci_finish_mrq(host, host->cmd->mrq);
 		return;
 	}
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 01002cba1359..ca427d8efc29 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -645,6 +645,8 @@ struct sdhci_ops {
 	void	(*voltage_switch)(struct sdhci_host *host);
 	void	(*adma_write_desc)(struct sdhci_host *host, void **desc,
 				   dma_addr_t addr, int len, unsigned int cmd);
+	void	(*platform_cmd_err)(struct sdhci_host *host, u32 intmask,
+				    u32 *intmask_p);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
@@ -798,5 +800,7 @@ void sdhci_start_tuning(struct sdhci_host *host);
 void sdhci_end_tuning(struct sdhci_host *host);
 void sdhci_reset_tuning(struct sdhci_host *host);
 void sdhci_send_tuning(struct sdhci_host *host, u32 opcode);
+void sdhci_cmd_err(struct sdhci_host *host, u32 intmask, u32 *intmask_p);
+void sdhci_finish_command(struct sdhci_host *host);
 
 #endif /* __SDHCI_HW_H */
-- 
2.19.2

[PATCH v2 2/2] mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning

[Faiz Abbas]

commit 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
callback") skips data resets during tuning operation. Because of this,
a data error or data finish interrupt might still arrive after a command
error has been handled and the mrq ended. This ends up with a "mmc0: Got
data interrupt 0x00000002 even though no data operation was in progress"
error message.

Fix this by adding a platform specific callback for command errors. Mark
the mrq as a failure but wait for a data interrupt instead of calling
finish_mrq().

Fixes: 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
callback")
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci-omap.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index b1a66ca3821a..67b361a403bc 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -797,6 +797,29 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
 	sdhci_reset(host, mask);
 }
 
+void sdhci_omap_cmd_err(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
+
+	if (omap_host->is_tuning) {
+		/*
+		 * Since we are not resetting data lines during tuning
+		 * operation, data error or data complete interrupts
+		 * might still arrive. Mark this request as a failure
+		 * but still wait for the data interrupt
+		 */
+		if (intmask & SDHCI_INT_TIMEOUT)
+			host->cmd->error = -ETIMEDOUT;
+		else
+			host->cmd->error = -EILSEQ;
+
+		sdhci_finish_command(host);
+	} else {
+		sdhci_cmd_err(host, intmask, intmask_p);
+	}
+}
+
 static struct sdhci_ops sdhci_omap_ops = {
 	.set_clock = sdhci_omap_set_clock,
 	.set_power = sdhci_omap_set_power,
@@ -807,6 +830,7 @@ static struct sdhci_ops sdhci_omap_ops = {
 	.platform_send_init_74_clocks = sdhci_omap_init_74_clocks,
 	.reset = sdhci_omap_reset,
 	.set_uhs_signaling = sdhci_omap_set_uhs_signaling,
+	.platform_cmd_err = sdhci_omap_cmd_err,
 };
 
 static int sdhci_omap_set_capabilities(struct sdhci_omap_host *omap_host)
-- 
2.19.2

Re: [PATCH v2 0/2] Fixes for command errors during tuning

[Faiz Abbas]

Hi,

On 01/03/19 2:08 PM, Faiz Abbas wrote:
> These patches fix the following error message in dra7xx boards:
> 
> [4.833198] mmc1: Got data interrupt 0x00000002 even though no data
> operation was in progress.
> 
> Tested with 100 times boot tests on dra71x-evm, dra72x-evm and
> dra7xx-evm.
> 
> v2:
> Added EXPORT_SYMBOL_GPL for sdhci_cmd_err and sdhci_send_command to fix
> errors when built as module.
> 
> Faiz Abbas (2):
>   mmc: sdhci: Add platform_cmd_err() to sdhci_ops
>   mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning
> 
>  drivers/mmc/host/sdhci-omap.c | 24 +++++++++++++++++++++
>  drivers/mmc/host/sdhci.c      | 40 +++++++++++++++++++++++------------
>  drivers/mmc/host/sdhci.h      |  4 ++++
>  3 files changed, 54 insertions(+), 14 deletions(-)
>

Gentle ping.

Thanks,
Faiz

Re: [PATCH v2 2/2] mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning

[Adrian Hunter]

On 1/03/19 10:38 AM, Faiz Abbas wrote:
> commit 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
> callback") skips data resets during tuning operation. Because of this,
> a data error or data finish interrupt might still arrive after a command
> error has been handled and the mrq ended. This ends up with a "mmc0: Got
> data interrupt 0x00000002 even though no data operation was in progress"
> error message.
> 
> Fix this by adding a platform specific callback for command errors. Mark
> the mrq as a failure but wait for a data interrupt instead of calling
> finish_mrq().
> 
> Fixes: 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
> callback")
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/sdhci-omap.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index b1a66ca3821a..67b361a403bc 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -797,6 +797,29 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> +void sdhci_omap_cmd_err(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	if (omap_host->is_tuning) {
> +		/*
> +		 * Since we are not resetting data lines during tuning
> +		 * operation, data error or data complete interrupts
> +		 * might still arrive. Mark this request as a failure
> +		 * but still wait for the data interrupt
> +		 */
> +		if (intmask & SDHCI_INT_TIMEOUT)
> +			host->cmd->error = -ETIMEDOUT;
> +		else
> +			host->cmd->error = -EILSEQ;
> +
> +		sdhci_finish_command(host);
> +	} else {
> +		sdhci_cmd_err(host, intmask, intmask_p);
> +	}
> +}

Could this be done by the existing ->irq() callback? i.e. mask the error
bits in intmask (have to write them back to SDHCI_INT_STATUS also) but set
cmd->error.

Re: [PATCH v2 2/2] mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning

[Rizvi, Mohammad Faiz Abbas]

Hi Adrian,

On 3/6/2019 5:39 PM, Adrian Hunter wrote:
> On 1/03/19 10:38 AM, Faiz Abbas wrote:
>> commit 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
>> callback") skips data resets during tuning operation. Because of this,
>> a data error or data finish interrupt might still arrive after a command
>> error has been handled and the mrq ended. This ends up with a "mmc0: Got
>> data interrupt 0x00000002 even though no data operation was in progress"
>> error message.
>>
>> Fix this by adding a platform specific callback for command errors. Mark
>> the mrq as a failure but wait for a data interrupt instead of calling
>> finish_mrq().
>>
>> Fixes: 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
>> callback")
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>>  drivers/mmc/host/sdhci-omap.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
>> index b1a66ca3821a..67b361a403bc 100644
>> --- a/drivers/mmc/host/sdhci-omap.c
>> +++ b/drivers/mmc/host/sdhci-omap.c
>> @@ -797,6 +797,29 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>>  	sdhci_reset(host, mask);
>>  }
>>  
>> +void sdhci_omap_cmd_err(struct sdhci_host *host, u32 intmask, u32 *intmask_p)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	if (omap_host->is_tuning) {
>> +		/*
>> +		 * Since we are not resetting data lines during tuning
>> +		 * operation, data error or data complete interrupts
>> +		 * might still arrive. Mark this request as a failure
>> +		 * but still wait for the data interrupt
>> +		 */
>> +		if (intmask & SDHCI_INT_TIMEOUT)
>> +			host->cmd->error = -ETIMEDOUT;
>> +		else
>> +			host->cmd->error = -EILSEQ;
>> +
>> +		sdhci_finish_command(host);
>> +	} else {
>> +		sdhci_cmd_err(host, intmask, intmask_p);
>> +	}
>> +}
> 
> Could this be done by the existing ->irq() callback? i.e. mask the error
> bits in intmask (have to write them back to SDHCI_INT_STATUS also) but set
> cmd->error.
> 

It is possible but I really don't want the callback to be unnecessarily
called for every single interrupt that happens. I think we should only
use the callback in the case of an actual error interrupt :-)

Thanks,
Faiz

RE: [PATCH v2 2/2] mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning

[Hunter, Adrian]

> -----Original Message-----
> From: Rizvi, Mohammad Faiz Abbas [mailto:faiz_abbas@ti.com]
> Sent: Tuesday, March 12, 2019 7:35 PM
> To: Hunter, Adrian <adrian.hunter@intel.com>; linux-
> kernel@vger.kernel.org; linux-mmc@vger.kernel.org; linux-
> omap@vger.kernel.org
> Cc: ulf.hansson@linaro.org; kishon@ti.com
> Subject: Re: [PATCH v2 2/2] mmc: sdhci-omap: Don't finish_mrq() on a
> command error during tuning
> 
> Hi Adrian,
> 
> On 3/6/2019 5:39 PM, Adrian Hunter wrote:
> > On 1/03/19 10:38 AM, Faiz Abbas wrote:
> >> commit 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
> >> callback") skips data resets during tuning operation. Because of
> >> this, a data error or data finish interrupt might still arrive after
> >> a command error has been handled and the mrq ended. This ends up with
> >> a "mmc0: Got data interrupt 0x00000002 even though no data operation
> was in progress"
> >> error message.
> >>
> >> Fix this by adding a platform specific callback for command errors.
> >> Mark the mrq as a failure but wait for a data interrupt instead of
> >> calling finish_mrq().
> >>
> >> Fixes: 5b0d62108b46 ("mmc: sdhci-omap: Add platform specific reset
> >> callback")
> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >> ---
> >>  drivers/mmc/host/sdhci-omap.c | 24 ++++++++++++++++++++++++
> >>  1 file changed, 24 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-omap.c
> >> b/drivers/mmc/host/sdhci-omap.c index b1a66ca3821a..67b361a403bc
> >> 100644
> >> --- a/drivers/mmc/host/sdhci-omap.c
> >> +++ b/drivers/mmc/host/sdhci-omap.c
> >> @@ -797,6 +797,29 @@ void sdhci_omap_reset(struct sdhci_host *host,
> u8 mask)
> >>  	sdhci_reset(host, mask);
> >>  }
> >>
> >> +void sdhci_omap_cmd_err(struct sdhci_host *host, u32 intmask, u32
> >> +*intmask_p) {
> >> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +	struct sdhci_omap_host *omap_host =
> sdhci_pltfm_priv(pltfm_host);
> >> +
> >> +	if (omap_host->is_tuning) {
> >> +		/*
> >> +		 * Since we are not resetting data lines during tuning
> >> +		 * operation, data error or data complete interrupts
> >> +		 * might still arrive. Mark this request as a failure
> >> +		 * but still wait for the data interrupt
> >> +		 */
> >> +		if (intmask & SDHCI_INT_TIMEOUT)
> >> +			host->cmd->error = -ETIMEDOUT;
> >> +		else
> >> +			host->cmd->error = -EILSEQ;
> >> +
> >> +		sdhci_finish_command(host);
> >> +	} else {
> >> +		sdhci_cmd_err(host, intmask, intmask_p);
> >> +	}
> >> +}
> >
> > Could this be done by the existing ->irq() callback? i.e. mask the
> > error bits in intmask (have to write them back to SDHCI_INT_STATUS
> > also) but set
> > cmd->error.
> >
> 
> It is possible but I really don't want the callback to be unnecessarily called for
> every single interrupt that happens. I think we should only use the callback in
> the case of an actual error interrupt :-)

You mean for performance reasons?  I would have thought it would be
only a handful of cycles to call into a function, find nothing to do, and return.
That should be negligible compared to the rest of the interrupt handler.
If that is the concern, then it might be worth measuring it.