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

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

[Venkatraman S]

mmc_execute_hpi should send the HPI command only
once, 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>
CC: Namjae Jeon <linkinjeon@gmail.com>
CC: Jae hoon Chung <jh80.chung@gmail.com>
CC: Chris Ball <cjb@laptop.org>
---
Changes since v1:
	Skip looping over send_status indefinitely
	Correct the interpretation of OUT_OF_INTERRUPT_TIME

 drivers/mmc/core/core.c    |   45 ++++++++++++++++++++++++--------------------
 drivers/mmc/core/mmc_ops.c |    2 +-
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e541efb..027c579 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 {
 	int err;
 	u32 status;
+	unsigned long starttime;
 
 	BUG_ON(!card);
 
@@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 	/*
 	 * 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);
+	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
+		pr_debug("%s: Can't send HPI: Card state=%d\n",
+			mmc_hostname(card->host), R1_CURRENT_STATE(status));
+		err = -EINVAL;
+		goto out;
+	}
+
+	starttime = jiffies;
+	err = mmc_send_hpi_cmd(card, &status);
+	if (err) {
+		pr_debug("%s:HPI could not be sent.err=%d)\n",
+			mmc_hostname(card->host), err);
+		goto out;
+	}
+
+	do {
+		err = mmc_send_status(card, &status);
+
+		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
+			goto out;
+		if (jiffies_to_msecs(jiffies - starttime) >
+			card->ext_csd.out_of_int_time)
+			err = -ETIMEDOUT;
+	} while (!err);
 
-			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));
 
 out:
 	mmc_release_host(card->host);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 69370f4..f2235d6 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -569,7 +569,7 @@ 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;
+	cmd.cmd_timeout_ms = 0;
 
 	err = mmc_wait_for_cmd(card->host, &cmd, 0);
 	if (err) {
-- 
1.7.10.rc2

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

[Namjae Jeon]

2012/4/17 Venkatraman S <svenkatr@ti.com>:
> mmc_execute_hpi should send the HPI command only
> once, 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.
Hi. Venkatraman.
I can not find this words. " the command's completion time is not
dependent on OUT_OF_INTERRUPT_TIME" .
Would you inform me which page and which part you checked in specification ?

Thanks.
>
> 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>
> CC: Namjae Jeon <linkinjeon@gmail.com>
> CC: Jae hoon Chung <jh80.chung@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> Changes since v1:
>        Skip looping over send_status indefinitely
>        Correct the interpretation of OUT_OF_INTERRUPT_TIME
>
>  drivers/mmc/core/core.c    |   45 ++++++++++++++++++++++++--------------------
>  drivers/mmc/core/mmc_ops.c |    2 +-
>  2 files changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e541efb..027c579 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  {
>        int err;
>        u32 status;
> +       unsigned long starttime;
>
>        BUG_ON(!card);
>
> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>        /*
>         * 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);
> +       if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
> +               pr_debug("%s: Can't send HPI: Card state=%d\n",
> +                       mmc_hostname(card->host), R1_CURRENT_STATE(status));
> +               err = -EINVAL;
> +               goto out;
> +       }
> +
> +       starttime = jiffies;
> +       err = mmc_send_hpi_cmd(card, &status);
> +       if (err) {
> +               pr_debug("%s:HPI could not be sent.err=%d)\n",
> +                       mmc_hostname(card->host), err);
> +               goto out;
> +       }
> +
> +       do {
> +               err = mmc_send_status(card, &status);
> +
> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
> +                       goto out;
> +               if (jiffies_to_msecs(jiffies - starttime) >
> +                       card->ext_csd.out_of_int_time)
> +                       err = -ETIMEDOUT;
> +       } while (!err);
>
> -                       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));
>
>  out:
>        mmc_release_host(card->host);
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 69370f4..f2235d6 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -569,7 +569,7 @@ 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;
> +       cmd.cmd_timeout_ms = 0;
>
>        err = mmc_wait_for_cmd(card->host, &cmd, 0);
>        if (err) {
> --
> 1.7.10.rc2
>

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

[Jaehoon Chung]

On 04/18/2012 09:20 AM, Namjae Jeon wrote:

> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>> mmc_execute_hpi should send the HPI command only
>> once, 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.
> Hi. Venkatraman.
> I can not find this words. " the command's completion time is not
> dependent on OUT_OF_INTERRUPT_TIME" .
> Would you inform me which page and which part you checked in specification ?

Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
It's my misunderstanding?

Best Regards,
Jaehoon Chung

> 
> Thanks.
>>
>> 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>
>> CC: Namjae Jeon <linkinjeon@gmail.com>
>> CC: Jae hoon Chung <jh80.chung@gmail.com>
>> CC: Chris Ball <cjb@laptop.org>
>> ---
>> Changes since v1:
>>        Skip looping over send_status indefinitely
>>        Correct the interpretation of OUT_OF_INTERRUPT_TIME
>>
>>  drivers/mmc/core/core.c    |   45 ++++++++++++++++++++++++--------------------
>>  drivers/mmc/core/mmc_ops.c |    2 +-
>>  2 files changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index e541efb..027c579 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>  {
>>        int err;
>>        u32 status;
>> +       unsigned long starttime;
>>
>>        BUG_ON(!card);
>>
>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>        /*
>>         * 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);
>> +       if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>> +               pr_debug("%s: Can't send HPI: Card state=%d\n",
>> +                       mmc_hostname(card->host), R1_CURRENT_STATE(status));
>> +               err = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       starttime = jiffies;
>> +       err = mmc_send_hpi_cmd(card, &status);
>> +       if (err) {
>> +               pr_debug("%s:HPI could not be sent.err=%d)\n",
>> +                       mmc_hostname(card->host), err);
>> +               goto out;
>> +       }
>> +
>> +       do {
>> +               err = mmc_send_status(card, &status);
>> +
>> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>> +                       goto out;
>> +               if (jiffies_to_msecs(jiffies - starttime) >
>> +                       card->ext_csd.out_of_int_time)
>> +                       err = -ETIMEDOUT;
>> +       } while (!err);
>>
>> -                       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));
>>
>>  out:
>>        mmc_release_host(card->host);
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 69370f4..f2235d6 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -569,7 +569,7 @@ 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;
>> +       cmd.cmd_timeout_ms = 0;
>>
>>        err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>        if (err) {
>> --
>> 1.7.10.rc2
>>
> 

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

[Namjae Jeon]

2012/4/18 Jaehoon Chung <jh80.chung@samsung.com>:
> On 04/18/2012 09:20 AM, Namjae Jeon wrote:
>
>> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>>> mmc_execute_hpi should send the HPI command only
>>> once, 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.
>> Hi. Venkatraman.
>> I can not find this words. " the command's completion time is not
>> dependent on OUT_OF_INTERRUPT_TIME" .
>> Would you inform me which page and which part you checked in specification ?
>
> Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
> It's my misunderstanding?
I agree, I also understand with you. But we should hear Venkatraman's
opinion from next reply.
see the spec.
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
HPI command is accepted as a legal command in prg-state. However, only
some commands may be interrupted by HPI. If HPI is received during
commands which are not interruptible, a response is sent but the HPI
command has no effect and the original command is completed normally,
possibly exceeding the OUT_OF_INTERRUPT_TIME timeout.
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
we can know OUT_OF_INTERRUPT_TIME timeout can be exceeded when
commands is not interrupted by HPI.
Also I think that timeout should be operated by host driver as current code.
But I have a question while checking spec.
Currently, when sending HPI command, MMC_RSP_R1 is set to flags.
When setting MMC_RSP_R1B, does host driver set timeout value from mmc core  ?

when I check stop transmission in spec, R1B response should be set in
write cases.
So MMC_RSP_R1B should be set to flags in HPI stop transmission instead
of MMC_RSP_R1 ?
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Abbreviation : STOP_ TRANSMISSION
response : R1/ R1b4
NOTE 4 R1 for read cases and R1b for write cases.
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Thanks.
>
> Best Regards,
> Jaehoon Chung
>
>>
>> Thanks.
>>>
>>> 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>
>>> CC: Namjae Jeon <linkinjeon@gmail.com>
>>> CC: Jae hoon Chung <jh80.chung@gmail.com>
>>> CC: Chris Ball <cjb@laptop.org>
>>> ---
>>> Changes since v1:
>>>        Skip looping over send_status indefinitely
>>>        Correct the interpretation of OUT_OF_INTERRUPT_TIME
>>>
>>>  drivers/mmc/core/core.c    |   45 ++++++++++++++++++++++++--------------------
>>>  drivers/mmc/core/mmc_ops.c |    2 +-
>>>  2 files changed, 26 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index e541efb..027c579 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>  {
>>>        int err;
>>>        u32 status;
>>> +       unsigned long starttime;
>>>
>>>        BUG_ON(!card);
>>>
>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>        /*
>>>         * 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);
>>> +       if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>> +               pr_debug("%s: Can't send HPI: Card state=%d\n",
>>> +                       mmc_hostname(card->host), R1_CURRENT_STATE(status));
>>> +               err = -EINVAL;
>>> +               goto out;
>>> +       }
>>> +
>>> +       starttime = jiffies;
>>> +       err = mmc_send_hpi_cmd(card, &status);
>>> +       if (err) {
>>> +               pr_debug("%s:HPI could not be sent.err=%d)\n",
>>> +                       mmc_hostname(card->host), err);
>>> +               goto out;
>>> +       }
>>> +
>>> +       do {
>>> +               err = mmc_send_status(card, &status);
>>> +
>>> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>> +                       goto out;
>>> +               if (jiffies_to_msecs(jiffies - starttime) >
>>> +                       card->ext_csd.out_of_int_time)
>>> +                       err = -ETIMEDOUT;
>>> +       } while (!err);
>>>
>>> -                       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));
>>>
>>>  out:
>>>        mmc_release_host(card->host);
>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>> index 69370f4..f2235d6 100644
>>> --- a/drivers/mmc/core/mmc_ops.c
>>> +++ b/drivers/mmc/core/mmc_ops.c
>>> @@ -569,7 +569,7 @@ 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;
>>> +       cmd.cmd_timeout_ms = 0;
>>>
>>>        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 v2] mmc: core: Fix the HPI execution sequence

[S, Venkatraman]

On Wed, Apr 18, 2012 at 10:15 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> 2012/4/18 Jaehoon Chung <jh80.chung@samsung.com>:
>> On 04/18/2012 09:20 AM, Namjae Jeon wrote:
>>
>>> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>>>> mmc_execute_hpi should send the HPI command only
>>>> once, 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.
>>> Hi. Venkatraman.
>>> I can not find this words. " the command's completion time is not
>>> dependent on OUT_OF_INTERRUPT_TIME" .
>>> Would you inform me which page and which part you checked in specification ?
>>
>> Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
>> It's my misunderstanding?
> I agree, I also understand with you. But we should hear Venkatraman's
> opinion from next reply.
> see the spec.

That particular line was explicit, *emphasis* mine..
In Section 6.8.2 "OUT_OF_INTERRUPT_TIME defines the maximum time
between the *end* bit of CMD12/13, arg[0]=1 to the DAT0 release of the
device."

Which essentially means the timer should start after the HPI command
has been exchanged, and should normally end when the DAT0 line is
released (in other words, move out of PRG state).
You can see the same definition in Section 7.4.33

The definition in 6.6.23, is partly confusing, for one it uses
OUT_OF_INTERRUPT_BUSY_TIME and not OUT_OF_INTERRUPT_TIME, and other,
it refers to the command being *interrupted by* HPI, not the HPI
command itself.

Let me know if this explains it a bit better.

Best regards,
Venkat.
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> HPI command is accepted as a legal command in prg-state. However, only
> some commands may be interrupted by HPI. If HPI is received during
> commands which are not interruptible, a response is sent but the HPI
> command has no effect and the original command is completed normally,
> possibly exceeding the OUT_OF_INTERRUPT_TIME timeout.
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> we can know OUT_OF_INTERRUPT_TIME timeout can be exceeded when
> commands is not interrupted by HPI.
> Also I think that timeout should be operated by host driver as current code.
> But I have a question while checking spec.
> Currently, when sending HPI command, MMC_RSP_R1 is set to flags.
> When setting MMC_RSP_R1B, does host driver set timeout value from mmc core  ?
>
> when I check stop transmission in spec, R1B response should be set in
> write cases.
> So MMC_RSP_R1B should be set to flags in HPI stop transmission instead
> of MMC_RSP_R1 ?
> -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> Abbreviation : STOP_ TRANSMISSION
> response : R1/ R1b4
> NOTE 4 R1 for read cases and R1b for write cases.
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> Thanks.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> Thanks.
>>>>
>>>> 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>
>>>> CC: Namjae Jeon <linkinjeon@gmail.com>
>>>> CC: Jae hoon Chung <jh80.chung@gmail.com>
>>>> CC: Chris Ball <cjb@laptop.org>
>>>> ---
>>>> Changes since v1:
>>>>        Skip looping over send_status indefinitely
>>>>        Correct the interpretation of OUT_OF_INTERRUPT_TIME
>>>>
>>>>  drivers/mmc/core/core.c    |   45 ++++++++++++++++++++++++--------------------
>>>>  drivers/mmc/core/mmc_ops.c |    2 +-
>>>>  2 files changed, 26 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index e541efb..027c579 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>  {
>>>>        int err;
>>>>        u32 status;
>>>> +       unsigned long starttime;
>>>>
>>>>        BUG_ON(!card);
>>>>
>>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>        /*
>>>>         * 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);
>>>> +       if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>>> +               pr_debug("%s: Can't send HPI: Card state=%d\n",
>>>> +                       mmc_hostname(card->host), R1_CURRENT_STATE(status));
>>>> +               err = -EINVAL;
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       starttime = jiffies;
>>>> +       err = mmc_send_hpi_cmd(card, &status);
>>>> +       if (err) {
>>>> +               pr_debug("%s:HPI could not be sent.err=%d)\n",
>>>> +                       mmc_hostname(card->host), err);
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       do {
>>>> +               err = mmc_send_status(card, &status);
>>>> +
>>>> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>>> +                       goto out;
>>>> +               if (jiffies_to_msecs(jiffies - starttime) >
>>>> +                       card->ext_csd.out_of_int_time)
>>>> +                       err = -ETIMEDOUT;
>>>> +       } while (!err);
>>>>
>>>> -                       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));
>>>>
>>>>  out:
>>>>        mmc_release_host(card->host);
>>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>>> index 69370f4..f2235d6 100644
>>>> --- a/drivers/mmc/core/mmc_ops.c
>>>> +++ b/drivers/mmc/core/mmc_ops.c
>>>> @@ -569,7 +569,7 @@ 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;
>>>> +       cmd.cmd_timeout_ms = 0;
>>>>
>>>>        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 v2] mmc: core: Fix the HPI execution sequence

[Namjae Jeon]

2012/4/18 S, Venkatraman <svenkatr@ti.com>:
> On Wed, Apr 18, 2012 at 10:15 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>> 2012/4/18 Jaehoon Chung <jh80.chung@samsung.com>:
>>> On 04/18/2012 09:20 AM, Namjae Jeon wrote:
>>>
>>>> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>>>>> mmc_execute_hpi should send the HPI command only
>>>>> once, 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.
>>>> Hi. Venkatraman.
>>>> I can not find this words. " the command's completion time is not
>>>> dependent on OUT_OF_INTERRUPT_TIME" .
>>>> Would you inform me which page and which part you checked in specification ?
>>>
>>> Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
>>> It's my misunderstanding?
>> I agree, I also understand with you. But we should hear Venkatraman's
>> opinion from next reply.
>> see the spec.
>
> That particular line was explicit, *emphasis* mine..
> In Section 6.8.2 "OUT_OF_INTERRUPT_TIME defines the maximum time
> between the *end* bit of CMD12/13, arg[0]=1 to the DAT0 release of the
> device."
>
> Which essentially means the timer should start after the HPI command
> has been exchanged, and should normally end when the DAT0 line is
> released (in other words, move out of PRG state).
> You can see the same definition in Section 7.4.33
>
> The definition in 6.6.23, is partly confusing, for one it uses
> OUT_OF_INTERRUPT_BUSY_TIME and not OUT_OF_INTERRUPT_TIME, and other,
> it refers to the command being *interrupted by* HPI, not the HPI
> command itself.
>
> Let me know if this explains it a bit better.
>
> Best regards,
> Venkat.
Hi. Venkat.
You're right. OUT_OF_INTERRUPT_TIME is range between Busy line(Data0)
released by device and end bit of CMD 12,13.
and I would like you change some code in this patch.
I think that break is better than goto. because it is not duplicated loop.
+               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
+                       goto out;

you don't need to initialize cmd_timeout_ms value. because you can see
struct mmc_command cmd = {0}; code.
+       cmd.cmd_timeout_ms = 0;

Thanks.

>> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> HPI command is accepted as a legal command in prg-state. However, only
>> some commands may be interrupted by HPI. If HPI is received during
>> commands which are not interruptible, a response is sent but the HPI
>> command has no effect and the original command is completed normally,
>> possibly exceeding the OUT_OF_INTERRUPT_TIME timeout.
>> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> we can know OUT_OF_INTERRUPT_TIME timeout can be exceeded when
>> commands is not interrupted by HPI.
>> Also I think that timeout should be operated by host driver as current code.
>> But I have a question while checking spec.
>> Currently, when sending HPI command, MMC_RSP_R1 is set to flags.
>> When setting MMC_RSP_R1B, does host driver set timeout value from mmc core  ?
>>
>> when I check stop transmission in spec, R1B response should be set in
>> write cases.
>> So MMC_RSP_R1B should be set to flags in HPI stop transmission instead
>> of MMC_RSP_R1 ?
>> -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> Abbreviation : STOP_ TRANSMISSION
>> response : R1/ R1b4
>> NOTE 4 R1 for read cases and R1b for write cases.
>> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> Thanks.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>> Thanks.
>>>>>
>>>>> 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>
>>>>> CC: Namjae Jeon <linkinjeon@gmail.com>
>>>>> CC: Jae hoon Chung <jh80.chung@gmail.com>
>>>>> CC: Chris Ball <cjb@laptop.org>
>>>>> ---
>>>>> Changes since v1:
>>>>>        Skip looping over send_status indefinitely
>>>>>        Correct the interpretation of OUT_OF_INTERRUPT_TIME
>>>>>
>>>>>  drivers/mmc/core/core.c    |   45 ++++++++++++++++++++++++--------------------
>>>>>  drivers/mmc/core/mmc_ops.c |    2 +-
>>>>>  2 files changed, 26 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>> index e541efb..027c579 100644
>>>>> --- a/drivers/mmc/core/core.c
>>>>> +++ b/drivers/mmc/core/core.c
>>>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>>  {
>>>>>        int err;
>>>>>        u32 status;
>>>>> +       unsigned long starttime;
>>>>>
>>>>>        BUG_ON(!card);
>>>>>
>>>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>>        /*
>>>>>         * 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);
>>>>> +       if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>>>> +               pr_debug("%s: Can't send HPI: Card state=%d\n",
>>>>> +                       mmc_hostname(card->host), R1_CURRENT_STATE(status));
>>>>> +               err = -EINVAL;
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       starttime = jiffies;
>>>>> +       err = mmc_send_hpi_cmd(card, &status);
>>>>> +       if (err) {
>>>>> +               pr_debug("%s:HPI could not be sent.err=%d)\n",
>>>>> +                       mmc_hostname(card->host), err);
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       do {
>>>>> +               err = mmc_send_status(card, &status);
>>>>> +
>>>>> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>>>> +                       goto out;
>>>>> +               if (jiffies_to_msecs(jiffies - starttime) >
>>>>> +                       card->ext_csd.out_of_int_time)
>>>>> +                       err = -ETIMEDOUT;
>>>>> +       } while (!err);
>>>>>
>>>>> -                       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));
>>>>>
>>>>>  out:
>>>>>        mmc_release_host(card->host);
>>>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>>>> index 69370f4..f2235d6 100644
>>>>> --- a/drivers/mmc/core/mmc_ops.c
>>>>> +++ b/drivers/mmc/core/mmc_ops.c
>>>>> @@ -569,7 +569,7 @@ 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;
>>>>> +       cmd.cmd_timeout_ms = 0;
>>>>>
>>>>>        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 v2] mmc: core: Fix the HPI execution sequence

[S, Venkatraman]

On Wed, Apr 18, 2012 at 1:53 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> 2012/4/18 S, Venkatraman <svenkatr@ti.com>:
>> On Wed, Apr 18, 2012 at 10:15 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>>> 2012/4/18 Jaehoon Chung <jh80.chung@samsung.com>:
>>>> On 04/18/2012 09:20 AM, Namjae Jeon wrote:
>>>>
>>>>> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>>>>>> mmc_execute_hpi should send the HPI command only
>>>>>> once, 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.
>>>>> Hi. Venkatraman.
>>>>> I can not find this words. " the command's completion time is not
>>>>> dependent on OUT_OF_INTERRUPT_TIME" .
>>>>> Would you inform me which page and which part you checked in specification ?
>>>>
>>>> Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
>>>> It's my misunderstanding?
>>> I agree, I also understand with you. But we should hear Venkatraman's
>>> opinion from next reply.
>>> see the spec.
>>
>> That particular line was explicit, *emphasis* mine..
>> In Section 6.8.2 "OUT_OF_INTERRUPT_TIME defines the maximum time
>> between the *end* bit of CMD12/13, arg[0]=1 to the DAT0 release of the
>> device."
>>
>> Which essentially means the timer should start after the HPI command
>> has been exchanged, and should normally end when the DAT0 line is
>> released (in other words, move out of PRG state).
>> You can see the same definition in Section 7.4.33
>>
>> The definition in 6.6.23, is partly confusing, for one it uses
>> OUT_OF_INTERRUPT_BUSY_TIME and not OUT_OF_INTERRUPT_TIME, and other,
>> it refers to the command being *interrupted by* HPI, not the HPI
>> command itself.
>>
>> Let me know if this explains it a bit better.
>>
>> Best regards,
>> Venkat.
> Hi. Venkat.
> You're right. OUT_OF_INTERRUPT_TIME is range between Busy line(Data0)
> released by device and end bit of CMD 12,13.
> and I would like you change some code in this patch.
> I think that break is better than goto. because it is not duplicated loop.
> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
> +                       goto out;
>
> you don't need to initialize cmd_timeout_ms value. because you can see
> struct mmc_command cmd = {0}; code.
> +       cmd.cmd_timeout_ms = 0;
>
> Thanks.
>

Great - thanks for your review and feedback. I'll post a new patch
after making the changes you
requested.
Regards,
Venkat.

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

[Jaehoon Chung]

On 04/18/2012 05:42 PM, S, Venkatraman wrote:

> On Wed, Apr 18, 2012 at 1:53 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>> 2012/4/18 S, Venkatraman <svenkatr@ti.com>:
>>> On Wed, Apr 18, 2012 at 10:15 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>>>> 2012/4/18 Jaehoon Chung <jh80.chung@samsung.com>:
>>>>> On 04/18/2012 09:20 AM, Namjae Jeon wrote:
>>>>>
>>>>>> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>>>>>>> mmc_execute_hpi should send the HPI command only
>>>>>>> once, 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.
>>>>>> Hi. Venkatraman.
>>>>>> I can not find this words. " the command's completion time is not
>>>>>> dependent on OUT_OF_INTERRUPT_TIME" .
>>>>>> Would you inform me which page and which part you checked in specification ?
>>>>>
>>>>> Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
>>>>> It's my misunderstanding?
>>>> I agree, I also understand with you. But we should hear Venkatraman's
>>>> opinion from next reply.
>>>> see the spec.
>>>
>>> That particular line was explicit, *emphasis* mine..
>>> In Section 6.8.2 "OUT_OF_INTERRUPT_TIME defines the maximum time
>>> between the *end* bit of CMD12/13, arg[0]=1 to the DAT0 release of the
>>> device."
>>>
>>> Which essentially means the timer should start after the HPI command
>>> has been exchanged, and should normally end when the DAT0 line is
>>> released (in other words, move out of PRG state).
>>> You can see the same definition in Section 7.4.33
>>>
>>> The definition in 6.6.23, is partly confusing, for one it uses
>>> OUT_OF_INTERRUPT_BUSY_TIME and not OUT_OF_INTERRUPT_TIME, and other,
>>> it refers to the command being *interrupted by* HPI, not the HPI
>>> command itself.
>>>
>>> Let me know if this explains it a bit better.
>>>
>>> Best regards,
>>> Venkat.
>> Hi. Venkat.
>> You're right. OUT_OF_INTERRUPT_TIME is range between Busy line(Data0)
>> released by device and end bit of CMD 12,13.
>> and I would like you change some code in this patch.
>> I think that break is better than goto. because it is not duplicated loop.
>> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>> +                       goto out;
>>
>> you don't need to initialize cmd_timeout_ms value. because you can see
>> struct mmc_command cmd = {0}; code.
>> +       cmd.cmd_timeout_ms = 0;
>>
>> Thanks.
>>
> 
> Great - thanks for your review and feedback. I'll post a new patch
> after making the changes you

Thanks for your explanation. it makes sense.
If you resend the patch, i will test with your patch.

Best Regards,
Jaehoon Chung

> requested.
> Regards,
> Venkat.
> --
> 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 v2] mmc: core: Fix the HPI execution sequence

[S, Venkatraman]

On Wed, Apr 18, 2012 at 6:24 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 04/18/2012 05:42 PM, S, Venkatraman wrote:
>
>> On Wed, Apr 18, 2012 at 1:53 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>>> 2012/4/18 S, Venkatraman <svenkatr@ti.com>:
>>>> On Wed, Apr 18, 2012 at 10:15 AM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>>>>> 2012/4/18 Jaehoon Chung <jh80.chung@samsung.com>:
>>>>>> On 04/18/2012 09:20 AM, Namjae Jeon wrote:
>>>>>>
>>>>>>> 2012/4/17 Venkatraman S <svenkatr@ti.com>:
>>>>>>>> mmc_execute_hpi should send the HPI command only
>>>>>>>> once, 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.
>>>>>>> Hi. Venkatraman.
>>>>>>> I can not find this words. " the command's completion time is not
>>>>>>> dependent on OUT_OF_INTERRUPT_TIME" .
>>>>>>> Would you inform me which page and which part you checked in specification ?
>>>>>>
>>>>>> Well, i know that timeout value is used the OUT_OF_INTERRUPT_TIME, when interrupted by HPI.
>>>>>> It's my misunderstanding?
>>>>> I agree, I also understand with you. But we should hear Venkatraman's
>>>>> opinion from next reply.
>>>>> see the spec.
>>>>
>>>> That particular line was explicit, *emphasis* mine..
>>>> In Section 6.8.2 "OUT_OF_INTERRUPT_TIME defines the maximum time
>>>> between the *end* bit of CMD12/13, arg[0]=1 to the DAT0 release of the
>>>> device."
>>>>
>>>> Which essentially means the timer should start after the HPI command
>>>> has been exchanged, and should normally end when the DAT0 line is
>>>> released (in other words, move out of PRG state).
>>>> You can see the same definition in Section 7.4.33
>>>>
>>>> The definition in 6.6.23, is partly confusing, for one it uses
>>>> OUT_OF_INTERRUPT_BUSY_TIME and not OUT_OF_INTERRUPT_TIME, and other,
>>>> it refers to the command being *interrupted by* HPI, not the HPI
>>>> command itself.
>>>>
>>>> Let me know if this explains it a bit better.
>>>>
>>>> Best regards,
>>>> Venkat.
>>> Hi. Venkat.
>>> You're right. OUT_OF_INTERRUPT_TIME is range between Busy line(Data0)
>>> released by device and end bit of CMD 12,13.
>>> and I would like you change some code in this patch.
>>> I think that break is better than goto. because it is not duplicated loop.
>>> +               if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>> +                       goto out;
>>>
>>> you don't need to initialize cmd_timeout_ms value. because you can see
>>> struct mmc_command cmd = {0}; code.
>>> +       cmd.cmd_timeout_ms = 0;
>>>
>>> Thanks.
>>>
>>
>> Great - thanks for your review and feedback. I'll post a new patch
>> after making the changes you
>
> Thanks for your explanation. it makes sense.
> If you resend the patch, i will test with your patch.
>
I've just sent it. Let me know if you require more details.

> Best Regards,
> Jaehoon Chung
>
>> requested.
>> Regards,
>> Venkat.
>> --
>> 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] mmc: core: Fix the HPI execution sequence

[Venkatraman S]

mmc_execute_hpi should send the HPI command only
once, 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>
---
 drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
 drivers/mmc/core/mmc_ops.c |    1 -
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e541efb..4e696e6 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 {
 	int err;
 	u32 status;
+	unsigned long starttime;
 
 	BUG_ON(!card);
 
@@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 	/*
 	 * 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);
+	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
+		pr_debug("%s: Can't send HPI: 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));
+	starttime = jiffies;
+	err = mmc_send_hpi_cmd(card, &status);
+	if (err) {
+		pr_debug("%s:HPI could not be sent.err=%d)\n",
+			mmc_hostname(card->host), 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] mmc: core: Fix the HPI execution sequence

[Namjae Jeon]

2012/4/18 Venkatraman S <svenkatr@ti.com>:
> mmc_execute_hpi should send the HPI command only
> once, 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>
Looks good to me~
Reviewed-by: Namjae Jeon <linkinjeon@gmail.com>
Thanks.
> ---

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

[Jaehoon Chung]

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

On 04/18/2012 11:45 PM, Venkatraman S wrote:

> mmc_execute_hpi should send the HPI command only
> once, 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>
> ---
>  drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
>  drivers/mmc/core/mmc_ops.c |    1 -
>  2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e541efb..4e696e6 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  {
>  	int err;
>  	u32 status;
> +	unsigned long starttime;
>  
>  	BUG_ON(!card);
>  
> @@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  	/*
>  	 * 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);
> +	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
> +		pr_debug("%s: Can't send HPI: 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));
> +	starttime = jiffies;
> +	err = mmc_send_hpi_cmd(card, &status);
> +	if (err) {
> +		pr_debug("%s:HPI could not be sent.err=%d)\n",
> +			mmc_hostname(card->host), 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) {

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

[S, Venkatraman]

On Thu, Apr 19, 2012 at 7:22 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>
Thanks Jaehoon Chung.
BTW, would you consider looking into the foreground HPI series ?

Chris,
  Can you please take this too for 3.5 ?


> On 04/18/2012 11:45 PM, Venkatraman S wrote:
>
>> mmc_execute_hpi should send the HPI command only
>> once, 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>
>> ---
>>  drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
>>  drivers/mmc/core/mmc_ops.c |    1 -
>>  2 files changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index e541efb..4e696e6 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>  {
>>       int err;
>>       u32 status;
>> +     unsigned long starttime;
>>
>>       BUG_ON(!card);
>>
>> @@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>       /*
>>        * 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);
>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>> +             pr_debug("%s: Can't send HPI: 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));
>> +     starttime = jiffies;
>> +     err = mmc_send_hpi_cmd(card, &status);
>> +     if (err) {
>> +             pr_debug("%s:HPI could not be sent.err=%d)\n",
>> +                     mmc_hostname(card->host), 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) {
>
>

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

[Chris Ball]

Hi,

On Wed, Apr 18 2012, Venkatraman S wrote:
> mmc_execute_hpi should send the HPI command only
> once, 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>
> ---
>  drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
>  drivers/mmc/core/mmc_ops.c |    1 -
>  2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e541efb..4e696e6 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  {
>  	int err;
>  	u32 status;
> +	unsigned long starttime;
>  
>  	BUG_ON(!card);
>  
> @@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  	/*
>  	 * 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);
> +	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
> +		pr_debug("%s: Can't send HPI: 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));
> +	starttime = jiffies;
> +	err = mmc_send_hpi_cmd(card, &status);
> +	if (err) {
> +		pr_debug("%s:HPI could not be sent.err=%d)\n",
> +			mmc_hostname(card->host), err);

Minor nit: you have an extra ) at the end, and missing spaces:

                pr_debug("%s: HPI could not be sent, error %d\n",

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

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

[Venkatraman S]

mmc_execute_hpi should send the HPI command only
once, 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>
---
	v1->v2:	Fix formatting inside debug prints
 drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
 drivers/mmc/core/mmc_ops.c |    1 -
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e541efb..d57c8f3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 {
 	int err;
 	u32 status;
+	unsigned long starttime;
 
 	BUG_ON(!card);
 
@@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 	/*
 	 * 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);
+	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
+		pr_debug("%s: Can't send HPI: 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));
+	starttime = jiffies;
+	err = mmc_send_hpi_cmd(card, &status);
+	if (err) {
+		pr_debug("%s:HPI could not be sent. err=%d\n",
+			mmc_hostname(card->host), 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 v2] mmc: core: Fix the HPI execution sequence

[Chris Ball]

Hi,

On Thu, Apr 19 2012, Venkatraman S wrote:
> mmc_execute_hpi should send the HPI command only
> once, 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>
> ---
> 	v1->v2:	Fix formatting inside debug prints
>  drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
>  drivers/mmc/core/mmc_ops.c |    1 -
>  2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e541efb..d57c8f3 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  {
>  	int err;
>  	u32 status;
> +	unsigned long starttime;
>  
>  	BUG_ON(!card);
>  
> @@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  	/*
>  	 * 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);
> +	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
> +		pr_debug("%s: Can't send HPI: 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));
> +	starttime = jiffies;
> +	err = mmc_send_hpi_cmd(card, &status);
> +	if (err) {
> +		pr_debug("%s:HPI could not be sent. err=%d\n",

You're still missing a space in "%s: HPI".

Thanks,

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

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

[S, Venkatraman]

On Thu, Apr 19, 2012 at 7:05 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Thu, Apr 19 2012, Venkatraman S wrote:
>> mmc_execute_hpi should send the HPI command only
>> once, 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>
>> ---
>>       v1->v2: Fix formatting inside debug prints
>>  drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
>>  drivers/mmc/core/mmc_ops.c |    1 -
>>  2 files changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index e541efb..d57c8f3 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>  {
>>       int err;
>>       u32 status;
>> +     unsigned long starttime;
>>
>>       BUG_ON(!card);
>>
>> @@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>       /*
>>        * 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);
>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>> +             pr_debug("%s: Can't send HPI: 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));
>> +     starttime = jiffies;
>> +     err = mmc_send_hpi_cmd(card, &status);
>> +     if (err) {
>> +             pr_debug("%s:HPI could not be sent. err=%d\n",
>
> You're still missing a space in "%s: HPI".
>
Err.. I thought you meant the space between "sent." and "err=".
Will fix.

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

[Venkatraman S]

mmc_execute_hpi should send the HPI command only
once, 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>
---
	v1->v2:	Fix formatting inside debug prints
 drivers/mmc/core/core.c    |   44 ++++++++++++++++++++++++--------------------
 drivers/mmc/core/mmc_ops.c |    1 -
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e541efb..d57c8f3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 {
 	int err;
 	u32 status;
+	unsigned long starttime;
 
 	BUG_ON(!card);
 
@@ -421,27 +422,30 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 	/*
 	 * 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);
+	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
+		pr_debug("%s: Can't send HPI: 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));
+	starttime = jiffies;
+	err = mmc_send_hpi_cmd(card, &status);
+	if (err) {
+		pr_debug("%s: HPI could not be sent. err=%d\n",
+			mmc_hostname(card->host), 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 v2] mmc: core: Fix the HPI execution sequence

[kdorfman]

> mmc_execute_hpi should send the HPI command only
> once, 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>
> CC: Namjae Jeon <linkinjeon@gmail.com>
> CC: Jae hoon Chung <jh80.chung@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> Changes since v1:
> 	Skip looping over send_status indefinitely
> 	Correct the interpretation of OUT_OF_INTERRUPT_TIME
>
>  drivers/mmc/core/core.c    |   45
> ++++++++++++++++++++++++--------------------
>  drivers/mmc/core/mmc_ops.c |    2 +-
>  2 files changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e541efb..027c579 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  {
>  	int err;
>  	u32 status;
> +	unsigned long starttime;
>
>  	BUG_ON(!card);
>
> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>  	/*
>  	 * 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);
> +	if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
> +		pr_debug("%s: Can't send HPI: Card state=%d\n",
> +			mmc_hostname(card->host), R1_CURRENT_STATE(status));
> +		err = -EINVAL;
> +		goto out;
> +	}
You can't return error here, because the card status may be:
0 = Idle 1 = Ready 2 = Ident 3 = Stby 4 = Tran 5 = Data 6 = Rcv 7 = Prg 8
= Dis 9 = Btst 10
Not every value is error here: the card state may be Idle/Ready and then
you do not need to return error.


> +
> +	starttime = jiffies;
> +	err = mmc_send_hpi_cmd(card, &status);
> +	if (err) {
> +		pr_debug("%s:HPI could not be sent.err=%d)\n",
> +			mmc_hostname(card->host), err);
> +		goto out;
> +	}
> +
> +	do {
> +		err = mmc_send_status(card, &status);
> +
> +		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
> +			goto out;
> +		if (jiffies_to_msecs(jiffies - starttime) >
> +			card->ext_csd.out_of_int_time)
> +			err = -ETIMEDOUT;
> +	} while (!err);
>
> -			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));
>
>  out:
>  	mmc_release_host(card->host);
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 69370f4..f2235d6 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -569,7 +569,7 @@ 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;
> +	cmd.cmd_timeout_ms = 0;
>
>  	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
>
Thanks,
Kostya
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

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

[S, Venkatraman]

On Wed, May 9, 2012 at 6:47 PM,  <kdorfman@codeaurora.org> wrote:
>> mmc_execute_hpi should send the HPI command only
>> once, 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>
>> CC: Namjae Jeon <linkinjeon@gmail.com>
>> CC: Jae hoon Chung <jh80.chung@gmail.com>
>> CC: Chris Ball <cjb@laptop.org>
>> ---
>> Changes since v1:
>>       Skip looping over send_status indefinitely
>>       Correct the interpretation of OUT_OF_INTERRUPT_TIME
>>
>>  drivers/mmc/core/core.c    |   45
>> ++++++++++++++++++++++++--------------------
>>  drivers/mmc/core/mmc_ops.c |    2 +-
>>  2 files changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index e541efb..027c579 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>  {
>>       int err;
>>       u32 status;
>> +     unsigned long starttime;
>>
>>       BUG_ON(!card);
>>
>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>       /*
>>        * 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);
>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>> +             pr_debug("%s: Can't send HPI: Card state=%d\n",
>> +                     mmc_hostname(card->host), R1_CURRENT_STATE(status));
>> +             err = -EINVAL;
>> +             goto out;
>> +     }
> You can't return error here, because the card status may be:
> 0 = Idle 1 = Ready 2 = Ident 3 = Stby 4 = Tran 5 = Data 6 = Rcv 7 = Prg 8
> = Dis 9 = Btst 10
> Not every value is error here: the card state may be Idle/Ready and then
> you do not need to return error.
>
Depends. When the intent is to send HPI command, and the current state
is such that it can't be sent,
then a error has to be returned, right ?
OTOH, if the API was "mmc_get_to_transfer_state()", then it would make
sense to not return an error.


>> +
>> +     starttime = jiffies;
>> +     err = mmc_send_hpi_cmd(card, &status);
>> +     if (err) {
>> +             pr_debug("%s:HPI could not be sent.err=%d)\n",
>> +                     mmc_hostname(card->host), err);
>> +             goto out;
>> +     }
>> +
>> +     do {
>> +             err = mmc_send_status(card, &status);
>> +
>> +             if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>> +                     goto out;
>> +             if (jiffies_to_msecs(jiffies - starttime) >
>> +                     card->ext_csd.out_of_int_time)
>> +                     err = -ETIMEDOUT;
>> +     } while (!err);
>>
>> -                     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));
>>
>>  out:
>>       mmc_release_host(card->host);
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 69370f4..f2235d6 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -569,7 +569,7 @@ 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;
>> +     cmd.cmd_timeout_ms = 0;
>>
>>       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
>>
> Thanks,
> Kostya
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>
>
>
>
>

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

[kdorfman]

> On Wed, May 9, 2012 at 6:47 PM,  <kdorfman@codeaurora.org> wrote:

>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>  {
>>>       int err;
>>>       u32 status;
>>> +     unsigned long starttime;
>>>
>>>       BUG_ON(!card);
>>>
>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>       /*
>>>        * 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);
>>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>> +             pr_debug("%s: Can't send HPI: Card state=%d\n",
>>> +                     mmc_hostname(card->host),
>>> R1_CURRENT_STATE(status));
>>> +             err = -EINVAL;
>>> +             goto out;
>>> +     }
>> You can't return error here, because the card status may be:
>> 0 = Idle 1 = Ready 2 = Ident 3 = Stby 4 = Tran 5 = Data 6 = Rcv 7 = Prg
>> 8
>> = Dis 9 = Btst 10
>> Not every value is error here: the card state may be Idle/Ready and then
>> you do not need to return error.
>>
> Depends. When the intent is to send HPI command, and the current state
> is such that it can't be sent,
> then a error has to be returned, right ?
> OTOH, if the API was "mmc_get_to_transfer_state()", then it would make
> sense to not return an error.

I'm working on HPI commmand that stops running BKOPS, when runtime suspend
flow is triggered. Right now, the code is not correct and CMD5 (sleep)
sent to the card without any check for running BKOPS.
In this case card may be in any state. Do you think we need to wait/check
state before calling mmc_interrupt_hpi() function?

>
>
>>> +
>>> +     starttime = jiffies;
>>> +     err = mmc_send_hpi_cmd(card, &status);
>>> +     if (err) {
>>> +             pr_debug("%s:HPI could not be sent.err=%d)\n",
>>> +                     mmc_hostname(card->host), err);
>>> +             goto out;
>>> +     }
>>> +
>>> +     do {
>>> +             err = mmc_send_status(card, &status);
>>> +
>>> +             if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>> +                     goto out;
>>> +             if (jiffies_to_msecs(jiffies - starttime) >
>>> +                     card->ext_csd.out_of_int_time)
I think we need to add some margin time to the out_of_int_time here.

Thanks,
Kostya
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

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

[S, Venkatraman]

On Wed, May 9, 2012 at 7:36 PM,  <kdorfman@codeaurora.org> wrote:
>> On Wed, May 9, 2012 at 6:47 PM,  <kdorfman@codeaurora.org> wrote:
>
>>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>  {
>>>>       int err;
>>>>       u32 status;
>>>> +     unsigned long starttime;
>>>>
>>>>       BUG_ON(!card);
>>>>
>>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>       /*
>>>>        * 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);
>>>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>>> +             pr_debug("%s: Can't send HPI: Card state=%d\n",
>>>> +                     mmc_hostname(card->host),
>>>> R1_CURRENT_STATE(status));
>>>> +             err = -EINVAL;
>>>> +             goto out;
>>>> +     }
>>> You can't return error here, because the card status may be:
>>> 0 = Idle 1 = Ready 2 = Ident 3 = Stby 4 = Tran 5 = Data 6 = Rcv 7 = Prg
>>> 8
>>> = Dis 9 = Btst 10
>>> Not every value is error here: the card state may be Idle/Ready and then
>>> you do not need to return error.
>>>
>> Depends. When the intent is to send HPI command, and the current state
>> is such that it can't be sent,
>> then a error has to be returned, right ?
>> OTOH, if the API was "mmc_get_to_transfer_state()", then it would make
>> sense to not return an error.
>
> I'm working on HPI commmand that stops running BKOPS, when runtime suspend
> flow is triggered. Right now, the code is not correct and CMD5 (sleep)
> sent to the card without any check for running BKOPS.
> In this case card may be in any state. Do you think we need to wait/check
> state before calling mmc_interrupt_hpi() function?
>

It's a completely different issue that I had informed Saugata, who is
doing the rework on
power off notify and sleep command sequence.
Essentially, the card shouldn't be put to sleep once BKOPS_START has
been issued,
and we should need to interrupt with HPI only when a higher layer
request needs to be serviced.

I don't know whether you are interested in handling only urgent BKOPS
or periodic BKOPS,
but we can discuss the design on a separate thread.

>>
>>
>>>> +
>>>> +     starttime = jiffies;
>>>> +     err = mmc_send_hpi_cmd(card, &status);
>>>> +     if (err) {
>>>> +             pr_debug("%s:HPI could not be sent.err=%d)\n",
>>>> +                     mmc_hostname(card->host), err);
>>>> +             goto out;
>>>> +     }
>>>> +
>>>> +     do {
>>>> +             err = mmc_send_status(card, &status);
>>>> +
>>>> +             if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>>> +                     goto out;
>>>> +             if (jiffies_to_msecs(jiffies - starttime) >
>>>> +                     card->ext_csd.out_of_int_time)
> I think we need to add some margin time to the out_of_int_time here.
>
> Thanks,
> Kostya
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>
>

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

[kdorfman]

Hello,

2 comments (see inside):
1. About card state before sending HPI
2. About waiting after sending HPI

> On Wed, May 9, 2012 at 7:36 PM,  <kdorfman@codeaurora.org> wrote:
>>> On Wed, May 9, 2012 at 6:47 PM,  <kdorfman@codeaurora.org> wrote:
>>
>>>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>>  {
>>>>>       int err;
>>>>>       u32 status;
>>>>> +     unsigned long starttime;
>>>>>
>>>>>       BUG_ON(!card);
>>>>>
>>>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>>       /*
>>>>>        * 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);
>>>>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>>>> +             pr_debug("%s: Can't send HPI: Card state=%d\n",
>>>>> +                     mmc_hostname(card->host),
>>>>> R1_CURRENT_STATE(status));
>>>>> +             err = -EINVAL;
>>>>> +             goto out;
>>>>> +     }
>>>> You can't return error here, because the card status may be:
>>>> 0 = Idle 1 = Ready 2 = Ident 3 = Stby 4 = Tran 5 = Data 6 = Rcv 7 =
>>>> Prg
>>>> 8
>>>> = Dis 9 = Btst 10
>>>> Not every value is error here: the card state may be Idle/Ready and
>>>> then
>>>> you do not need to return error.
>>>>
>>> Depends. When the intent is to send HPI command, and the current state
>>> is such that it can't be sent,
>>> then a error has to be returned, right ?
>>> OTOH, if the API was "mmc_get_to_transfer_state()", then it would make
>>> sense to not return an error.

It does't matter what caller flow is, when current operation finished, the
card may be in IDLE state
you do not need to send HPI and it is not error, because it is like to
have successful HPI done.
Probably not only IDLE state is like this (I think of standby state).

> It's a completely different issue that I had informed Saugata, who is
> doing the rework on
> power off notify and sleep command sequence.
> Essentially, the card shouldn't be put to sleep once BKOPS_START has
> been issued,
> and we should need to interrupt with HPI only when a higher layer
> request needs to be serviced.

Once BKOPS started, it will trigger IDLE timer for 5 sec, but BKOPS may
take more time.
This means that card will enter sleep again and CMD5 will fail.

> I don't know whether you are interested in handling only urgent BKOPS
> or periodic BKOPS,
> but we can discuss the design on a separate thread.

Let's do this discussion on newly published thread: "[PATCH v8] mmc:
support BKOPS feature for eMMC"

>
>>>
>>>
>>>>> +
>>>>> +     starttime = jiffies;
>>>>> +     err = mmc_send_hpi_cmd(card, &status);
>>>>> +     if (err) {
>>>>> +             pr_debug("%s:HPI could not be sent.err=%d)\n",
>>>>> +                     mmc_hostname(card->host), err);
>>>>> +             goto out;
>>>>> +     }
>>>>> +
>>>>> +     do {
>>>>> +             err = mmc_send_status(card, &status);
>>>>> +
>>>>> +             if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>>>> +                     goto out;
>>>>> +             if (jiffies_to_msecs(jiffies - starttime) >
>>>>> +                     card->ext_csd.out_of_int_time)
I think we need to add some margin time to the out_of_int_time here.
Because card clock is different from
jiffies.

Thanks,
Kostya
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum