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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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
> -----Original Message-----
> From: Rizvi, Mohammad Faiz Abbas [mailto:[email protected]]
> Sent: Tuesday, March 12, 2019 7:35 PM
> To: Hunter, Adrian <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; [email protected]
> 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 <[email protected]>
> >> ---
> >> 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.