RE: [PATCH v3] mmc: core: Fix the HPI execution sequence

["Subhash Jadavani" <subhashj@codeaurora.org>]

[-- Attachment #1: Type: text/plain, Size: 5149 bytes --]

Hi Venkatraman,

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Venkatraman S
> Sent: Wednesday, June 20, 2012 3:04 PM
> To: cjb@laptop.org
> Cc: linux-mmc@vger.kernel.org; linkinjeon@gmail.com;
> jh80.chung@samsung.com; alex.lemberg@sandisk.com; Venkatraman S; Kostya
> Subject: [PATCH v3] mmc: core: Fix the HPI execution sequence
> 
> mmc_execute_hpi should send the HPI command only once, and only if the
> card is in PRG state.
> 
> According to eMMC spec, the command's completion time is not dependent on
> OUT_OF_INTERRUPT_TIME. Only the transition out of PRG STATE is guarded by
> OUT_OF_INTERRUPT_TIME - which is defined to begin at the end of sending
> the command itself.
> 
> Specify the default timeout for the actual sending of HPI command, and
then
> use OUT_OF_INTERRUPT_TIME to wait for the transition out of PRG state.

I guess this is not the correct interpretation. cmd.cmd_timeout_ms should
specify for how much time host driver should wait for command + busy line
deassertion. So why are you keeping the timeout calculation after the
command is completed?

mmc_send_hpi_cmd() returns means the host controller driver have ensured
that BUSY indication is removed so after mmc_send_hpi_cmd(), if you want to
check if the card is still in PROGRAMMING state, it should be unconditional
check in loop (rather than timeout based) until card is out of programming
state.

To me, mmc_send_hpi_cmd() implementation should not be anything different
than mmc_switch(). In mmc_switch() also, after mmc_wait_for_cmd() returns it
continuously keeps checking if card is programming or not.

Regards,
Subhash

> 
> Reported-by: Alex Lemberg <Alex.Lemberg@sandisk.com>
> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> Reviewed-by: Namjae Jeon <linkinjeon@gmail.com>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> CC: Kostya <kdorfman@codeaurora.org>
> ---
> v2 discussions: http://marc.info/?l=linux-mmc&m=133657255616390&w=2
> 
> v2->v3:
> 	Return gracefully for card idle states (suggested by kdorfman)
> 
>  drivers/mmc/core/core.c    |   57
++++++++++++++++++++++++++-----------------
> -
>  drivers/mmc/core/mmc_ops.c |    1 -
>  2 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> 80ec427..08ed144 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -404,6 +404,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)  {
>  	int err;
>  	u32 status;
> +	unsigned long starttime;
> 
>  	BUG_ON(!card);
> 
> @@ -419,30 +420,40 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  		goto out;
>  	}
> 
> -	/*
> -	 * If the card status is in PRG-state, we can send the HPI command.
> -	 */
> -	if (R1_CURRENT_STATE(status) == R1_STATE_PRG) {
> -		do {
> -			/*
> -			 * We don't know when the HPI command will finish
> -			 * processing, so we need to resend HPI until out
> -			 * of prg-state, and keep checking the card status
> -			 * with SEND_STATUS.  If a timeout error occurs when
> -			 * sending the HPI command, we are already out of
> -			 * prg-state.
> -			 */
> -			err = mmc_send_hpi_cmd(card, &status);
> -			if (err)
> -				pr_debug("%s: abort HPI (%d error)\n",
> -					 mmc_hostname(card->host), err);
> +	switch (R1_CURRENT_STATE(status)) {
> 
> -			err = mmc_send_status(card, &status);
> -			if (err)
> -				break;
> -		} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> -	} else
> -		pr_debug("%s: Left prg-state\n", mmc_hostname(card->host));
> +	case R1_STATE_IDLE:
> +	case R1_STATE_READY:
> +	case R1_STATE_STBY: /* intentional fall through */
> +		/* In idle states, HPI is not needed and the caller
> +		 can issue the next intended command immediately */
> +		goto out;
> +		break;
> +	case R1_STATE_PRG:
> +		break;
> +	default:
> +		/* In all other states, it's illegal to issue HPI */
> +		pr_debug("%s: HPI cannot be sent. Card state=%d\n",
> +			mmc_hostname(card->host),
> R1_CURRENT_STATE(status));
> +		err = -EINVAL;
> +		goto out;
> +		break;
> +	}
> +
> +	starttime = jiffies;
> +	err = mmc_send_hpi_cmd(card, &status);
> +	if (err)
> +		goto out;
> +
> +	do {
> +		err = mmc_send_status(card, &status);
> +
> +		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
> +			break;
> +		if (jiffies_to_msecs(jiffies - starttime) >=
> +			card->ext_csd.out_of_int_time)
> +			err = -ETIMEDOUT;
> +	} while (!err);
> 
>  out:
>  	mmc_release_host(card->host);
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 69370f4..0ed2cc5 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -569,7 +569,6 @@ int mmc_send_hpi_cmd(struct mmc_card *card, u32
> *status)
> 
>  	cmd.opcode = opcode;
>  	cmd.arg = card->rca << 16 | 1;
> -	cmd.cmd_timeout_ms = card->ext_csd.out_of_int_time;
> 
>  	err = mmc_wait_for_cmd(card->host, &cmd, 0);
>  	if (err) {
> --
> 1.7.10.rc2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Type: message/rfc822, Size: 5959 bytes --]

From: "Venkatraman S" <svenkatr@ti.com>
To: <cjb@laptop.org>
Cc: <linux-mmc@vger.kernel.org>, <linkinjeon@gmail.com>, <jh80.chung@samsung.com>, <alex.lemberg@sandisk.com>, "Venkatraman S" <svenkatr@ti.com>, "Kostya" <kdorfman@codeaurora.org>
Subject: [PATCH v3] mmc: core: Fix the HPI execution sequence
Date: Wed, 20 Jun 2012 15:03:54 +0530
Message-ID: <1340184834-12700-1-git-send-email-svenkatr@ti.com>

mmc_execute_hpi should send the HPI command only
once, and only if the card is in PRG state.

According to eMMC spec, the command's completion time is
not dependent on OUT_OF_INTERRUPT_TIME. Only the transition
out of PRG STATE is guarded by OUT_OF_INTERRUPT_TIME - which is
defined to begin at the end of sending the command itself.

Specify the default timeout for the actual sending of HPI
command, and then use OUT_OF_INTERRUPT_TIME to wait for
the transition out of PRG state.

Reported-by: Alex Lemberg <Alex.Lemberg@sandisk.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
Reviewed-by: Namjae Jeon <linkinjeon@gmail.com>
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
CC: Kostya <kdorfman@codeaurora.org>
---
v2 discussions: http://marc.info/?l=linux-mmc&m=133657255616390&w=2

v2->v3: 
	Return gracefully for card idle states (suggested by kdorfman)

 drivers/mmc/core/core.c    |   57
++++++++++++++++++++++++++------------------
 drivers/mmc/core/mmc_ops.c |    1 -
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 80ec427..08ed144 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -404,6 +404,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 {
 	int err;
 	u32 status;
+	unsigned long starttime;
 
 	BUG_ON(!card);
 
@@ -419,30 +420,40 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 		goto out;
 	}
 
-	/*
-	 * If the card status is in PRG-state, we can send the HPI command.
-	 */
-	if (R1_CURRENT_STATE(status) == R1_STATE_PRG) {
-		do {
-			/*
-			 * We don't know when the HPI command will finish
-			 * processing, so we need to resend HPI until out
-			 * of prg-state, and keep checking the card status
-			 * with SEND_STATUS.  If a timeout error occurs when
-			 * sending the HPI command, we are already out of
-			 * prg-state.
-			 */
-			err = mmc_send_hpi_cmd(card, &status);
-			if (err)
-				pr_debug("%s: abort HPI (%d error)\n",
-					 mmc_hostname(card->host), err);
+	switch (R1_CURRENT_STATE(status)) {
 
-			err = mmc_send_status(card, &status);
-			if (err)
-				break;
-		} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
-	} else
-		pr_debug("%s: Left prg-state\n", mmc_hostname(card->host));
+	case R1_STATE_IDLE:
+	case R1_STATE_READY:
+	case R1_STATE_STBY: /* intentional fall through */
+		/* In idle states, HPI is not needed and the caller
+		 can issue the next intended command immediately */
+		goto out;
+		break;
+	case R1_STATE_PRG:
+		break;
+	default:
+		/* In all other states, it's illegal to issue HPI */
+		pr_debug("%s: HPI cannot be sent. Card state=%d\n",
+			mmc_hostname(card->host), R1_CURRENT_STATE(status));
+		err = -EINVAL;
+		goto out;
+		break;
+	}
+
+	starttime = jiffies;
+	err = mmc_send_hpi_cmd(card, &status);
+	if (err)
+		goto out;
+
+	do {
+		err = mmc_send_status(card, &status);
+
+		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
+			break;
+		if (jiffies_to_msecs(jiffies - starttime) >=
+			card->ext_csd.out_of_int_time)
+			err = -ETIMEDOUT;
+	} while (!err);
 
 out:
 	mmc_release_host(card->host);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 69370f4..0ed2cc5 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -569,7 +569,6 @@ int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status)
 
 	cmd.opcode = opcode;
 	cmd.arg = card->rca << 16 | 1;
-	cmd.cmd_timeout_ms = card->ext_csd.out_of_int_time;
 
 	err = mmc_wait_for_cmd(card->host, &cmd, 0);
 	if (err) {
-- 
1.7.10.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html