[PATCH] mmc:sdhci: handle busy-end interrupt during command

[PATCH] mmc:sdhci: handle busy-end interrupt during command

[Chanho Min]

It is fully legal for a controller to start handling busy-end interrupt
before it has signaled that the command has completed. So make sure
we do things in the proper order, Or it results that command interrupt
is ignored so it can cause unexpected operations. This is founded at some
toshiba emmc with the bellow warning.

"mmc0: Got command interrupt 0x00000001 even though
no command operation was in progress."

This issue has been also reported by Youssef TRIKI:
It is not specific to Toshiba devices, and happens with eMMC devices
as well as SD card which support Auto-CMD12 rather than CMD23.

Also, similar patch is submitted by:
Gwendal Grignou <gwendal@chromium.org>

Signed-off-by: Hankyung Yu <hankyung.yu@lge.com>
Signed-off-by: Chanho Min <chanho.min@lge.com>
Tested-by: Youssef TRIKI <youssef.triki@st.com>
---
 drivers/mmc/host/sdhci.c  |   17 +++++++++++++++--
 include/linux/mmc/sdhci.h |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 47055f3..2383be0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1007,6 +1007,7 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	mod_timer(&host->timer, timeout);
 
 	host->cmd = cmd;
+	host->busy_handle = 0;
 
 	sdhci_prepare_data(host, cmd);
 
@@ -2238,8 +2239,12 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		if (host->cmd->data)
 			DBG("Cannot wait for busy signal when also "
 				"doing a data transfer");
-		else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ))
+		else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ)
+				&& !host->busy_handle) {
+			/* Mark that command complete before busy is ended */
+			host->busy_handle = 1;
 			return;
+		}
 
 		/* The controller does not support the end-of-busy IRQ,
 		 * fall through and take the SDHCI_INT_RESPONSE */
@@ -2302,7 +2307,15 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 */
 		if (host->cmd && (host->cmd->flags & MMC_RSP_BUSY)) {
 			if (intmask & SDHCI_INT_DATA_END) {
-				sdhci_finish_command(host);
+				/*
+				 * Some cards handle busy-end interrupt
+				 * before the command completed, so make
+				 * sure we do things in the proper order.
+				 */
+				if (host->busy_handle)
+					sdhci_finish_command(host);
+				else
+					host->busy_handle = 1;
 				return;
 			}
 		}
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 08abe99..f91085b 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -149,6 +149,7 @@ struct sdhci_host {
 	struct mmc_command *cmd;	/* Current command */
 	struct mmc_data *data;	/* Current data request */
 	unsigned int data_early:1;	/* Data finished before cmd */
+	unsigned int busy_handle:1;	/* Handling the order of Busy-end */
 
 	struct sg_mapping_iter sg_miter;	/* SG state for PIO */
 	unsigned int blocks;	/* remaining PIO blocks */
-- 
1.7.9.5

Re: [PATCH] mmc:sdhci: handle busy-end interrupt during command

[Ulf Hansson]

On 21 August 2014 03:53, Chanho Min <chanho.min@lge.com> wrote:
> It is fully legal for a controller to start handling busy-end interrupt
> before it has signaled that the command has completed. So make sure
> we do things in the proper order, Or it results that command interrupt
> is ignored so it can cause unexpected operations. This is founded at some
> toshiba emmc with the bellow warning.
>
> "mmc0: Got command interrupt 0x00000001 even though
> no command operation was in progress."
>
> This issue has been also reported by Youssef TRIKI:
> It is not specific to Toshiba devices, and happens with eMMC devices
> as well as SD card which support Auto-CMD12 rather than CMD23.
>
> Also, similar patch is submitted by:
> Gwendal Grignou <gwendal@chromium.org>
>
> Signed-off-by: Hankyung Yu <hankyung.yu@lge.com>
> Signed-off-by: Chanho Min <chanho.min@lge.com>
> Tested-by: Youssef TRIKI <youssef.triki@st.com>

Hi Chanho,

This patch has been around for quite some time, sorry for lack of
response. Appreciate if we could get this tested on a few more
variants of sdhci, I am not able to test myself.

I plan to queue this for 3.18, unless someone objects soon.

Kind regards
Uffe

Re: [PATCH] mmc:sdhci: handle busy-end interrupt during command

[Ulf Hansson]

On 21 August 2014 09:26, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 August 2014 03:53, Chanho Min <chanho.min@lge.com> wrote:
>> It is fully legal for a controller to start handling busy-end interrupt
>> before it has signaled that the command has completed. So make sure
>> we do things in the proper order, Or it results that command interrupt
>> is ignored so it can cause unexpected operations. This is founded at some
>> toshiba emmc with the bellow warning.
>>
>> "mmc0: Got command interrupt 0x00000001 even though
>> no command operation was in progress."
>>
>> This issue has been also reported by Youssef TRIKI:
>> It is not specific to Toshiba devices, and happens with eMMC devices
>> as well as SD card which support Auto-CMD12 rather than CMD23.
>>
>> Also, similar patch is submitted by:
>> Gwendal Grignou <gwendal@chromium.org>
>>
>> Signed-off-by: Hankyung Yu <hankyung.yu@lge.com>
>> Signed-off-by: Chanho Min <chanho.min@lge.com>
>> Tested-by: Youssef TRIKI <youssef.triki@st.com>
>
> Hi Chanho,
>
> This patch has been around for quite some time, sorry for lack of
> response. Appreciate if we could get this tested on a few more
> variants of sdhci, I am not able to test myself.
>
> I plan to queue this for 3.18, unless someone objects soon.

Hi Chanho,

I was about to apply this one, but I am hitting a conflict. Even if
wasn't too hard for me to resolve it, I think it might need an extra
round of testing. Could you please re-spin and send a new version?

Kind regards
Uffe

[PATCH] mmc:sdhci:handle busy-end interrupt during command

[Gwendal Grignou]

From: Chanho Min <chanho.min@lge.com>

When controller supports busy-end interrupts, they may send it
before commands complete. If the host sends a new command too early,
it will result in CRC errors.

CMD  : CMD | ,,,, |   RESPONSE   |
DATA :     |    busy    |
                        .        .
                        .        . sdhci_cmd_irq (command interupt)
                        .
                        . sdhci_data_irq ("busy end" interrupt)

Before this patch, if the CPU is very fast, when sdhci_data_irq is
executed, it would complete the command and issue a new one while
CMD line is still driven by the device, resulting in a CRC error.

With this patch, we wait for both interrupts to be received before
completing the command.

Change-Id: I43b7467d59eb133d8c545115b48a5acbc450c2dd
Signed-off-by: Hankyung Yu <hankyung.yu@lge.com>
Signed-off-by: Chanho Min <chanho.min@lge.com>
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---

I reformated your patch. I also fix an issue when SDHCI_QUIRK_NO_BUSY_IRQ
is enabled.

On fast chromebooks with Tohisba eMMC, check the error messages:
 mmc0: Got command interrupt 0x00000001 even though no command operation was in progress.
and
 mmc0: unexpected status 0x800800 after switch
are gone.

 drivers/mmc/host/sdhci.c  | 29 ++++++++++++++++++++++++-----
 include/linux/mmc/sdhci.h |  1 +
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 04f0314..acfb2aa 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1007,6 +1007,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	mod_timer(&host->timer, jiffies + 10 * HZ);
 
 	host->cmd = cmd;
+	host->busy_handle = 0;
 
 	sdhci_prepare_data(host, cmd);
 
@@ -2282,11 +2283,21 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		if (host->cmd->data)
 			DBG("Cannot wait for busy signal when also "
 				"doing a data transfer");
-		else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ))
+		else if (host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) {
+			/*
+			 * The controller does not support the end-of-busy IRQ,
+			 * fall through and take the SDHCI_INT_RESPONSE
+			 */
+		} else if (host->busy_handle == 0) {
+			/* Mark that command complete before busy is ended */
+			host->busy_handle = 1;
 			return;
-
-		/* The controller does not support the end-of-busy IRQ,
-		 * fall through and take the SDHCI_INT_RESPONSE */
+		} else {
+			/*
+			 * We already received end-of-busy IRQ,
+			 * process commnad now.
+			 */
+		}
 	}
 
 	if (intmask & SDHCI_INT_RESPONSE)
@@ -2346,7 +2357,15 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 */
 		if (host->cmd && (host->cmd->flags & MMC_RSP_BUSY)) {
 			if (intmask & SDHCI_INT_DATA_END) {
-				sdhci_finish_command(host);
+				/*
+				 * Some cards handle busy-end interrupt
+				 * before the command completed, so make
+				 * sure we do things in the proper order.
+				 */
+				if (host->busy_handle)
+					sdhci_finish_command(host);
+				else
+					host->busy_handle = 1;
 				return;
 			}
 		}
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 796216c..7fa83aa 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -150,6 +150,7 @@ struct sdhci_host {
 	struct mmc_command *cmd;	/* Current command */
 	struct mmc_data *data;	/* Current data request */
 	unsigned int data_early:1;	/* Data finished before cmd */
+	unsigned int busy_handle:1;	/* Handling the order of Busy-end */
 
 	struct sg_mapping_iter sg_miter;	/* SG state for PIO */
 	unsigned int blocks;	/* remaining PIO blocks */
-- 
2.0.0.526.g5318336

[PATCH] mmc:sdhci: handle busy-end interrupt during command

[Chanho Min]

It is fully legal for a controller to start handling busy-end interrupt
before it has signaled that the command has completed. So make sure
we do things in the proper order, Or it results that command interrupt
is ignored so it can cause unexpected operations. This is founded at some
toshiba emmc with the bellow warning.

"mmc0: Got command interrupt 0x00000001 even though
no command operation was in progress."

Signed-off-by: Hankyung Yu <hankyung.yu@lge.com>
Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 drivers/mmc/host/sdhci.c  |   17 +++++++++++++++--
 include/linux/mmc/sdhci.h |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index bd8a098..21f98e7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1016,6 +1016,7 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	mod_timer(&host->timer, jiffies + 10 * HZ);
 
 	host->cmd = cmd;
+	host->busy_handle = 0;
 
 	sdhci_prepare_data(host, cmd);
 
@@ -2271,8 +2272,12 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		if (host->cmd->data)
 			DBG("Cannot wait for busy signal when also "
 				"doing a data transfer");
-		else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ))
+		else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ)
+				&& !host->busy_handle) {
+			/* Mark that command complete before busy is ended */
+			host->busy_handle = 1;
 			return;
+		}
 
 		/* The controller does not support the end-of-busy IRQ,
 		 * fall through and take the SDHCI_INT_RESPONSE */
@@ -2335,7 +2340,15 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 */
 		if (host->cmd && (host->cmd->flags & MMC_RSP_BUSY)) {
 			if (intmask & SDHCI_INT_DATA_END) {
-				sdhci_finish_command(host);
+				/*
+				 * Some cards handle busy-end interrupt
+				 * before the command completed, so make
+				 * sure we do things in the proper order.
+				 */
+				if (host->busy_handle)
+					sdhci_finish_command(host);
+				else
+					host->busy_handle = 1;
 				return;
 			}
 		}
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 3e781b8..0118020 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -148,6 +148,7 @@ struct sdhci_host {
 	struct mmc_command *cmd;	/* Current command */
 	struct mmc_data *data;	/* Current data request */
 	unsigned int data_early:1;	/* Data finished before cmd */
+	unsigned int busy_handle:1;	/* Handling the order of Busy-end */
 
 	struct sg_mapping_iter sg_miter;	/* SG state for PIO */
 	unsigned int blocks;	/* remaining PIO blocks */
-- 
1.7.9.5