[PATCH v4 0/2] Fix command errors during tuning

[PATCH v4 0/2] Fix 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.

v4:
Fixed commit description for patch 1.
Added SDHCI_INT_TIMEOUT to CMD_ERR_MASK in patch 2.

v3:
Removed the command error specific callback and using the already
existing sdhci_irq callback instead. No measurable drop in performance.

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: Export sdhci_finish_command()
  mmc: sdhci-omap: Don't finish_mrq() on a command error during tuning

 drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c      |  3 ++-
 drivers/mmc/host/sdhci.h      |  1 +
 3 files changed, 40 insertions(+), 1 deletion(-)

-- 
2.19.2

[PATCH v4 1/2] mmc: sdhci: Export sdhci_finish_command()

[Faiz Abbas]

Make sdhci_finish_command() public so that it can be called from platform
drivers.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci.c | 3 ++-
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a8141ff9be03..76d36e13a011 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,7 @@ 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)
 {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 01002cba1359..49b133babf47 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -798,5 +798,6 @@ 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_finish_command(struct sdhci_host *host);
 
 #endif /* __SDHCI_HW_H */
-- 
2.19.2

[PATCH v4 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 sdhci_irq. Mark the
mrq as a failure but wait for a data interrupt instead of calling
finish_mrq().

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index 5bbed477c9b1..9da2ff9ede8b 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -797,6 +797,42 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
 	sdhci_reset(host, mask);
 }
 
+#define CMD_ERR_MASK (SDHCI_INT_CRC | SDHCI_INT_END_BIT | SDHCI_INT_INDEX |\
+		      SDHCI_INT_TIMEOUT)
+#define CMD_MASK (CMD_ERR_MASK | SDHCI_INT_RESPONSE)
+
+static irqreturn_t sdhci_omap_irq(struct sdhci_host *host, u32 intmask)
+{
+	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 && (intmask & CMD_ERR_MASK)) {
+
+		/*
+		 * 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);
+
+		/*
+		 * Sometimes command error interrupts and command complete
+		 * interrupt will arrive together. Clear all command related
+		 * interrupts here.
+		 */
+		sdhci_writel(host, intmask & CMD_MASK, SDHCI_INT_STATUS);
+		intmask &= ~CMD_MASK;
+	}
+
+	return intmask;
+}
+
 static struct sdhci_ops sdhci_omap_ops = {
 	.set_clock = sdhci_omap_set_clock,
 	.set_power = sdhci_omap_set_power,
@@ -807,6 +843,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,
+	.irq = sdhci_omap_irq,
 };
 
 static int sdhci_omap_set_capabilities(struct sdhci_omap_host *omap_host)
-- 
2.19.2

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

[Adrian Hunter]

On 26/03/19 1:00 PM, 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 sdhci_irq. Mark the
> mrq as a failure but wait for a data interrupt instead of calling
> finish_mrq().
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 5bbed477c9b1..9da2ff9ede8b 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -797,6 +797,42 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> +#define CMD_ERR_MASK (SDHCI_INT_CRC | SDHCI_INT_END_BIT | SDHCI_INT_INDEX |\
> +		      SDHCI_INT_TIMEOUT)
> +#define CMD_MASK (CMD_ERR_MASK | SDHCI_INT_RESPONSE)
> +
> +static irqreturn_t sdhci_omap_irq(struct sdhci_host *host, u32 intmask)
> +{
> +	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 && (intmask & CMD_ERR_MASK)) {
> +
> +		/*
> +		 * 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);

Would it be possible to set SDHCI_INT_RESPONSE instead of calling
sdhci_finish_command() directly?

> +
> +		/*
> +		 * Sometimes command error interrupts and command complete
> +		 * interrupt will arrive together. Clear all command related
> +		 * interrupts here.
> +		 */
> +		sdhci_writel(host, intmask & CMD_MASK, SDHCI_INT_STATUS);
> +		intmask &= ~CMD_MASK;
> +	}
> +
> +	return intmask;
> +}
> +
>  static struct sdhci_ops sdhci_omap_ops = {
>  	.set_clock = sdhci_omap_set_clock,
>  	.set_power = sdhci_omap_set_power,
> @@ -807,6 +843,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,
> +	.irq = sdhci_omap_irq,
>  };
>  
>  static int sdhci_omap_set_capabilities(struct sdhci_omap_host *omap_host)
> 

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

[Faiz Abbas]

Hi Adrian,

On 27/03/19 4:45 PM, Adrian Hunter wrote:
> On 26/03/19 1:00 PM, 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 sdhci_irq. Mark the
>> mrq as a failure but wait for a data interrupt instead of calling
>> finish_mrq().
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>>  drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
>> index 5bbed477c9b1..9da2ff9ede8b 100644
>> --- a/drivers/mmc/host/sdhci-omap.c
>> +++ b/drivers/mmc/host/sdhci-omap.c
>> @@ -797,6 +797,42 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>>  	sdhci_reset(host, mask);
>>  }
>>  
>> +#define CMD_ERR_MASK (SDHCI_INT_CRC | SDHCI_INT_END_BIT | SDHCI_INT_INDEX |\
>> +		      SDHCI_INT_TIMEOUT)
>> +#define CMD_MASK (CMD_ERR_MASK | SDHCI_INT_RESPONSE)
>> +
>> +static irqreturn_t sdhci_omap_irq(struct sdhci_host *host, u32 intmask)
>> +{
>> +	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 && (intmask & CMD_ERR_MASK)) {
>> +
>> +		/*
>> +		 * 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);
> 
> Would it be possible to set SDHCI_INT_RESPONSE instead of calling
> sdhci_finish_command() directly?
>

It should be possible. But what we are trying to do won't be very clear
from the code. Ideally an interrupt handler should not set any interrupt
bits, just handle and clear them. Also, setting the RESPONSE bit even
when there has been no response seems wrong and makes the code more
convoluted.

Thanks,
Faiz

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

[Adrian Hunter]

On 27/03/19 1:47 PM, Faiz Abbas wrote:
> Hi Adrian,
> 
> On 27/03/19 4:45 PM, Adrian Hunter wrote:
>> On 26/03/19 1:00 PM, 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 sdhci_irq. Mark the
>>> mrq as a failure but wait for a data interrupt instead of calling
>>> finish_mrq().
>>>
>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>> ---
>>>  drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
>>> index 5bbed477c9b1..9da2ff9ede8b 100644
>>> --- a/drivers/mmc/host/sdhci-omap.c
>>> +++ b/drivers/mmc/host/sdhci-omap.c
>>> @@ -797,6 +797,42 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>>>  	sdhci_reset(host, mask);
>>>  }
>>>  
>>> +#define CMD_ERR_MASK (SDHCI_INT_CRC | SDHCI_INT_END_BIT | SDHCI_INT_INDEX |\
>>> +		      SDHCI_INT_TIMEOUT)
>>> +#define CMD_MASK (CMD_ERR_MASK | SDHCI_INT_RESPONSE)
>>> +
>>> +static irqreturn_t sdhci_omap_irq(struct sdhci_host *host, u32 intmask)
>>> +{
>>> +	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 && (intmask & CMD_ERR_MASK)) {
>>> +
>>> +		/*
>>> +		 * 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);
>>
>> Would it be possible to set SDHCI_INT_RESPONSE instead of calling
>> sdhci_finish_command() directly?
>>
> 
> It should be possible. But what we are trying to do won't be very clear
> from the code. Ideally an interrupt handler should not set any interrupt
> bits, just handle and clear them. Also, setting the RESPONSE bit even
> when there has been no response seems wrong and makes the code more
> convoluted.

Well, I am not in favour of exporting sdhci_finish_command().
So I had a look and it is not obvious why you need to call it.
What about something this:

	if (omap_host->is_tuning && host->cmd && !host->data_early &&
	    (intmask & CMD_ERR_MASK)) {

		if (intmask & SDHCI_INT_TIMEOUT)
			host->cmd->error = -ETIMEDOUT;
		else
			host->cmd->error = -EILSEQ;

		host->cmd = NULL;

		sdhci_writel(host, intmask & CMD_MASK, SDHCI_INT_STATUS);
		intmask &= ~CMD_MASK;
	}

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

[Faiz Abbas]

Hi Adrian,

On 28/03/19 1:43 PM, Adrian Hunter wrote:
> On 27/03/19 1:47 PM, Faiz Abbas wrote:
>> Hi Adrian,
>>
>> On 27/03/19 4:45 PM, Adrian Hunter wrote:
>>> On 26/03/19 1:00 PM, 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 sdhci_irq. Mark the
>>>> mrq as a failure but wait for a data interrupt instead of calling
>>>> finish_mrq().
>>>>
>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-omap.c | 37 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
>>>> index 5bbed477c9b1..9da2ff9ede8b 100644
>>>> --- a/drivers/mmc/host/sdhci-omap.c
>>>> +++ b/drivers/mmc/host/sdhci-omap.c
>>>> @@ -797,6 +797,42 @@ void sdhci_omap_reset(struct sdhci_host *host, u8 mask)
>>>>  	sdhci_reset(host, mask);
>>>>  }
>>>>  
>>>> +#define CMD_ERR_MASK (SDHCI_INT_CRC | SDHCI_INT_END_BIT | SDHCI_INT_INDEX |\
>>>> +		      SDHCI_INT_TIMEOUT)
>>>> +#define CMD_MASK (CMD_ERR_MASK | SDHCI_INT_RESPONSE)
>>>> +
>>>> +static irqreturn_t sdhci_omap_irq(struct sdhci_host *host, u32 intmask)
>>>> +{
>>>> +	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 && (intmask & CMD_ERR_MASK)) {
>>>> +
>>>> +		/*
>>>> +		 * 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);
>>>
>>> Would it be possible to set SDHCI_INT_RESPONSE instead of calling
>>> sdhci_finish_command() directly?
>>>
>>
>> It should be possible. But what we are trying to do won't be very clear
>> from the code. Ideally an interrupt handler should not set any interrupt
>> bits, just handle and clear them. Also, setting the RESPONSE bit even
>> when there has been no response seems wrong and makes the code more
>> convoluted.
> 
> Well, I am not in favour of exporting sdhci_finish_command().
> So I had a look and it is not obvious why you need to call it.
> What about something this:
> 
> 	if (omap_host->is_tuning && host->cmd && !host->data_early &&
> 	    (intmask & CMD_ERR_MASK)) {
> 
> 		if (intmask & SDHCI_INT_TIMEOUT)
> 			host->cmd->error = -ETIMEDOUT;
> 		else
> 			host->cmd->error = -EILSEQ;
> 
> 		host->cmd = NULL;
> 
> 		sdhci_writel(host, intmask & CMD_MASK, SDHCI_INT_STATUS);
> 		intmask &= ~CMD_MASK;
> 	}
> 

Yeah. I guess host->cmd = NULL was the only thing that was being done
sdhci_finish_command(). This works with 100 time boot tests for me. Will
post next version.

Thanks,
Faiz