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

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

[Venkatraman S]

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

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

[Chris Ball]

Hi, trivial style comments,

On Wed, Jun 20 2012, Venkatraman S wrote:
>	switch (R1_CURRENT_STATE(status)) {
>

No newline here.

>	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;

This might just be personal style, but I don't think of this as
"intentional fall through" -- you're just matching multiple cases.

Case fall-through would be if the code inside the cases didn't call
goto or break at the end and kept going into the next code block,
which doesn't happen here.  Here's an elegant use of fall-through:
   http://chris.printf.net/btrfs_parse_size.txt

I don't think you should bother using break after you've already
called goto.

Also, please use:
/*
 * comment 1
 * comment 2
 */

for multiline comments instead of

/* comment 1
 comment 2 */
 
Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

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

[S, Venkatraman]

On Wed, Jun 20, 2012 at 10:31 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi, trivial style comments,
>
> On Wed, Jun 20 2012, Venkatraman S wrote:
>>       switch (R1_CURRENT_STATE(status)) {
>>
>
> No newline here.
>
>>       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;
>
> This might just be personal style, but I don't think of this as
> "intentional fall through" -- you're just matching multiple cases.
>
> Case fall-through would be if the code inside the cases didn't call
> goto or break at the end and kept going into the next code block,
> which doesn't happen here.  Here's an elegant use of fall-through:
>   http://chris.printf.net/btrfs_parse_size.txt

That's a cool trick !!

>
> I don't think you should bother using break after you've already
> called goto.
>
Thanks - I was circumspect about the goto & break thing myself.
> Also, please use:
> /*
>  * comment 1
>  * comment 2
>  */
>
> for multiline comments instead of
>
> /* comment 1
>  comment 2 */
>
I'll fix these and send a patch now.

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

[Subhash Jadavani]

[-- 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

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

[S, Venkatraman]

On Wed, Jun 20, 2012 at 11:29 PM, Subhash Jadavani
<subhashj@codeaurora.org> wrote:
> 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?
>
If what you stay is correct, the card should be in PRG state after
mmc_send_hpi_cmd()
returns.

I understood and observed it as opposite. HPI cmd returns immediately,
and the device takes
OUT_OF_INTERRUPT_TIME time to come out of PRG state.

My experiments / tests confirm that behavior. It takes a few ms (and several
SEND_STATUS messages later), after HPI, for the card to be out of PRG state.

> 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

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

[Subhash Jadavani]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of S, Venkatraman
> Sent: Thursday, June 21, 2012 12:55 PM
> To: Subhash Jadavani
> Cc: cjb@laptop.org; linux-mmc@vger.kernel.org; linkinjeon@gmail.com;
> jh80.chung@samsung.com; alex.lemberg@sandisk.com; Kostya
> Subject: Re: [PATCH v3] mmc: core: Fix the HPI execution sequence
> 
> On Wed, Jun 20, 2012 at 11:29 PM, Subhash Jadavani
> <subhashj@codeaurora.org> wrote:
> > 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?
> >
> If what you stay is correct, the card should be in PRG state after
> mmc_send_hpi_cmd()
> returns.

> 
> I understood and observed it as opposite. HPI cmd returns immediately, and
the
> device takes OUT_OF_INTERRUPT_TIME time to come out of PRG state.
> 
> My experiments / tests confirm that behavior. It takes a few ms (and
several
> SEND_STATUS messages later), after HPI, for the card to be out of PRG
state.

What is the HPI command in your case? CMD12 or CMD13? For command CMD12, we
set the R1B flag which would tell the host driver to wait for the DAT0 line
to be deasserted (if host controller support the automatic detection of DAT0
de assertion).
But HPI command is CMD13 then yes, driver will not wait for DAT0 line de
assertion.

Yes, so to support both CMD12 and CMD13 as HPI command, we can have 2
options:
1. 
	- Set the R1B flag for CMD13 as well so controller waits for the
PROG_DONE.
	- Once mmc_send_hpi_cmd() returns, you can poll (without any
timeout) until the card is out of programming state.

2.
	- don't set the R1B flag for CMD13
	- Once mmc_send_hpi_cmd() returns, if HPI command is CMD12, you can
poll (without any timeout) until the card is out of programming state. But
if HPI command is CMD13, poll until card is out of programming state with
timeout.

But in both of the above case, if host driver do not support automatic
detection of DAT0 de assertion, we will have to send multiple CMD13
(send_status) before card comes out of programming state but yes, in case
card gets stuck in programming state forever then we will end up sending
CMD13 forever. In this case timeout based wait is fine. So if you want to
take of case where card is stuck in programming state forever, your
implementation is fine. Is this your intention here? I have few comments on
your code below.
	
Regards,
Subhash

> 
> > 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)

When you are comparing snapshot of 2 different jiffies, you should be using
time_after() or time_before() macros others this will not take case of
jiffies wraparound.

> >> +                     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
> --
> 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

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

[S, Venkatraman]

On Thu, Jun 21, 2012 at 4:38 PM, Subhash Jadavani
<subhashj@codeaurora.org> wrote:
>
>
>> -----Original Message-----
>> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
>> owner@vger.kernel.org] On Behalf Of S, Venkatraman
>> Sent: Thursday, June 21, 2012 12:55 PM
>> To: Subhash Jadavani
>> Cc: cjb@laptop.org; linux-mmc@vger.kernel.org; linkinjeon@gmail.com;
>> jh80.chung@samsung.com; alex.lemberg@sandisk.com; Kostya
>> Subject: Re: [PATCH v3] mmc: core: Fix the HPI execution sequence
>>
>> On Wed, Jun 20, 2012 at 11:29 PM, Subhash Jadavani
>> <subhashj@codeaurora.org> wrote:
>> > 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?
>> >
>> If what you stay is correct, the card should be in PRG state after
>> mmc_send_hpi_cmd()
>> returns.
>
>>
>> I understood and observed it as opposite. HPI cmd returns immediately, and
> the
>> device takes OUT_OF_INTERRUPT_TIME time to come out of PRG state.
>>
>> My experiments / tests confirm that behavior. It takes a few ms (and
> several
>> SEND_STATUS messages later), after HPI, for the card to be out of PRG
> state.
>
> What is the HPI command in your case? CMD12 or CMD13? For command CMD12, we
> set the R1B flag which would tell the host driver to wait for the DAT0 line
> to be deasserted (if host controller support the automatic detection of DAT0
> de assertion).
> But HPI command is CMD13 then yes, driver will not wait for DAT0 line de
> assertion.
>
Yes, it has CMD13 - and it makes sense.

> Yes, so to support both CMD12 and CMD13 as HPI command, we can have 2
> options:
> 1.
>        - Set the R1B flag for CMD13 as well so controller waits for the
> PROG_DONE.
>        - Once mmc_send_hpi_cmd() returns, you can poll (without any
> timeout) until the card is out of programming state.
>
> 2.
>        - don't set the R1B flag for CMD13
>        - Once mmc_send_hpi_cmd() returns, if HPI command is CMD12, you can
> poll (without any timeout) until the card is out of programming state. But
> if HPI command is CMD13, poll until card is out of programming state with
> timeout.
>
> But in both of the above case, if host driver do not support automatic
> detection of DAT0 de assertion, we will have to send multiple CMD13
> (send_status) before card comes out of programming state but yes, in case

I think that's the case with OMAP, and I assumed it to be common.

> card gets stuck in programming state forever then we will end up sending
> CMD13 forever. In this case timeout based wait is fine. So if you want to
> take of case where card is stuck in programming state forever, your
> implementation is fine. Is this your intention here? I have few comments on
> your code below.
>
Yes - that was atleast the intention. I didn't assume DAT0 de assertation was
auto detectable, and even otherwise, PRG state was to be polled later
anyway.

> Regards,
> Subhash
>
>>
>> > 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)
>
> When you are comparing snapshot of 2 different jiffies, you should be using
> time_after() or time_before() macros others this will not take case of
> jiffies wraparound.

Ok- I'll send out an updated version.

[PATCH v4] mmc: core: Fix the HPI execution sequence

[Venkatraman S]

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>
---
v3->v4: Fix coding style issues as suggested by Chris Ball.
	Use time_after() for safe time diff calculations as
	 suggested by Subash Jadavani.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 80ec427..0d4bb19 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 prg_wait;
 
 	BUG_ON(!card);
 
@@ -419,30 +420,38 @@ 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)) {
+	case R1_STATE_IDLE:
+	case R1_STATE_READY:
+	case R1_STATE_STBY:
+		/*
+		 * In idle states, HPI is not needed and the caller
+		 * can issue the next intended command immediately
+		 */
+		goto out;
+	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;
+	}
 
-			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));
+	prg_wait = jiffies + msecs_to_jiffies(card->ext_csd.out_of_int_time);
+	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 (time_after(jiffies, prg_wait))
+			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

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

[Subhash Jadavani]

One comment inline below:

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Venkatraman S
> Sent: Thursday, June 21, 2012 7:43 PM
> To: cjb@laptop.org
> Cc: linux-mmc@vger.kernel.org; linkinjeon@gmail.com;
> jh80.chung@samsung.com; alex.lemberg@sandisk.com;
> subashj@codeaurora.org; Venkatraman S; Kostya
> Subject: [PATCH v4] 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.
> 
> 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>
> ---
> v3->v4: Fix coding style issues as suggested by Chris Ball.
> 	Use time_after() for safe time diff calculations as
> 	 suggested by Subash Jadavani.
> 
>  drivers/mmc/core/core.c    |   55
++++++++++++++++++++++++++-----------------
> -
>  drivers/mmc/core/mmc_ops.c |    1 -
>  2 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> 80ec427..0d4bb19 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 prg_wait;
> 
>  	BUG_ON(!card);
> 
> @@ -419,30 +420,38 @@ 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)) {
> +	case R1_STATE_IDLE:
> +	case R1_STATE_READY:
> +	case R1_STATE_STBY:
> +		/*
> +		 * In idle states, HPI is not needed and the caller
> +		 * can issue the next intended command immediately
> +		 */
> +		goto out;
> +	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;
> +	}
> 
> -			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));
> +	prg_wait = jiffies +
msecs_to_jiffies(card->ext_csd.out_of_int_time);

According to commit text, you should be calculating timeout duration after
the HPI command is sent to card. So this prg_wait calculation should be
moved after mmc_send_hpi_cmd().

> +	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 (time_after(jiffies, prg_wait))
> +			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

[PATCH v5] mmc: core: Fix the HPI execution sequence

[Venkatraman S]

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>
---
v4->v5: Start time calculation after the HPI command has been sent

v3->v4: Fix coding style issues as suggested by Chris Ball.
       Use time_after() for safe time diff calculations as
        suggested by Subash Jadavani.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 80ec427..f11de40 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 prg_wait;
 
 	BUG_ON(!card);
 
@@ -419,30 +420,38 @@ 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)) {
+	case R1_STATE_IDLE:
+	case R1_STATE_READY:
+	case R1_STATE_STBY:
+		/*
+		 * In idle states, HPI is not needed and the caller
+		 * can issue the next intended command immediately
+		 */
+		goto out;
+	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;
+	}
 
-			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));
+	err = mmc_send_hpi_cmd(card, &status);
+	if (err)
+		goto out;
+
+	prg_wait = jiffies + msecs_to_jiffies(card->ext_csd.out_of_int_time);
+	do {
+		err = mmc_send_status(card, &status);
+
+		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
+			break;
+		if (time_after(jiffies, prg_wait))
+			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

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

[Subhash Jadavani]

Thanks Venkatraman. Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

Regards,
Subhash

> -----Original Message-----
> From: Venkatraman S [mailto:svenkatr@ti.com]
> Sent: Friday, June 22, 2012 11:43 AM
> To: cjb@laptop.org
> Cc: linux-mmc@vger.kernel.org; linkinjeon@gmail.com;
> jh80.chung@samsung.com; alex.lemberg@sandisk.com;
> subhashj@codeaurora.org; Venkatraman S; Kostya
> Subject: [PATCH v5] 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.
> 
> 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>
> ---
> v4->v5: Start time calculation after the HPI command has been sent
> 
> v3->v4: Fix coding style issues as suggested by Chris Ball.
>        Use time_after() for safe time diff calculations as
>         suggested by Subash Jadavani.
> 
>  drivers/mmc/core/core.c    |   55
++++++++++++++++++++++++++-----------------
> -
>  drivers/mmc/core/mmc_ops.c |    1 -
>  2 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> 80ec427..f11de40 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 prg_wait;
> 
>  	BUG_ON(!card);
> 
> @@ -419,30 +420,38 @@ 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)) {
> +	case R1_STATE_IDLE:
> +	case R1_STATE_READY:
> +	case R1_STATE_STBY:
> +		/*
> +		 * In idle states, HPI is not needed and the caller
> +		 * can issue the next intended command immediately
> +		 */
> +		goto out;
> +	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;
> +	}
> 
> -			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));
> +	err = mmc_send_hpi_cmd(card, &status);
> +	if (err)
> +		goto out;
> +
> +	prg_wait = jiffies +
msecs_to_jiffies(card->ext_csd.out_of_int_time);
> +	do {
> +		err = mmc_send_status(card, &status);
> +
> +		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
> +			break;
> +		if (time_after(jiffies, prg_wait))
> +			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

Re: [PATCH v5] mmc: core: Fix the HPI execution sequence

[S, Venkatraman]

On Fri, Jun 22, 2012 at 7:01 PM, Subhash Jadavani
<subhashj@codeaurora.org> wrote:
> Thanks Venkatraman. Looks good to me.
> Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>
>
Thanks Subhash.
Chris,
   Can you consider including this for 3.6 ?

Thanks and regards,
Venkat.

Re: [PATCH v5] mmc: core: Fix the HPI execution sequence

[S, Venkatraman]

On Thu, Jun 28, 2012 at 1:12 PM, S, Venkatraman <svenkatr@ti.com> wrote:
> On Fri, Jun 22, 2012 at 7:01 PM, Subhash Jadavani
> <subhashj@codeaurora.org> wrote:
>> Thanks Venkatraman. Looks good to me.
>> Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>
>>
> Thanks Subhash.
> Chris,
>   Can you consider including this for 3.6 ?
>
Oops!! - I see that it's already in mmc-next.
Many thanks !

> Thanks and regards,
> Venkat.