[U-Boot] [PATCH] mmc: fix OCR Polling

[U-Boot] [PATCH] mmc: fix OCR Polling

[Andrew Gabbasov]

Hi Peng,

> From: Peng.Fan at freescale.com (Peng Fan)
> Date: Thu, 19 Mar 2015 16:22:46 +0800
> Subject: [U-Boot] [PATCH] mmc: fix OCR Polling
> 
> If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
> state is transfered to Ready state, and there is no need to send
> CMD1 again. Otherwise following CMD1 will recieve no response, or
> timeour error from driver such as fsl_esdhc.c.
> 
> If not into Ready state in previous CMD1, then continue CMD1 command.
> 
> In mmc_complete_op_cond, we use the value mmc->op_cond_response
> from mmc_send_op_cond, since there should be no CMD1 command between
> mmc_send_op_cond and mmc_complete_op_cond
> 
> Before fixing this, uboot log shows:
> "
> CMD_SEND:0
>                 ARG                      0x00000000
>                 MMC_RSP_NONE
> CMD_SEND:8
>                 ARG                      0x000001AA
>                 MMC_RSP_R1,5,6,7         0x18EC1504
> CMD_SEND:55
>                 ARG                      0x00000000
>                 MMC_RSP_R1,5,6,7         0x18EC1504
> CMD_SEND:0
>                 ARG                      0x00000000
>                 MMC_RSP_NONE
> CMD_SEND:1
>                 ARG                      0x00000000
>                 MMC_RSP_R3,4             0x00FF8080
> CMD_SEND:1
>                 ARG                      0x40300000
>                 MMC_RSP_R3,4             0xC0FF8080 --> Already OCR_BUSY
set
> CMD_SEND:1
>                 ARG                      0x40300000
>                 MMC_RSP_R3,4             0x0096850A --> Failed CMD1
> MMC init failed
> "
> 
> Using this patch, this issue is fixed, emmc can be detected correctly.
> 
> 
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
>  drivers/mmc/mmc.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index a13769e..43a9a8a 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -406,14 +406,23 @@ static int mmc_complete_op_cond(struct mmc *mmc)
>  
>      mmc->op_cond_pending = 0;
>      start = get_timer(0);
> -    do {
> +    /*
> +     * If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
> +     * state is transfered to Ready state, and there is no need to
> +     * send CMD1 again. Otherwise following CMD1 will recieve no
response,
> +     * or timeour error from driver such as fsl_esdhc.c.
> +     *
> +     * If not into Ready state in previous CMD1, then continue CMD1
> +     * command.
> +     */
> +    while (!(mmc->op_cond_response & OCR_BUSY)) {
>          err = mmc_send_op_cond_iter(mmc, &cmd, 1);
>          if (err)
>              return err;
>          if (get_timer(start) > timeout)
>              return UNUSABLE_ERR;
>          udelay(100);
> -    } while (!(mmc->op_cond_response & OCR_BUSY));
> +    }
>  
>      if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
>          cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
> -- 
> 1.8.4

After this patch, if the busy flag is indeed already set (so that the loop
body
is not executed), and it is not an SPI case (mmc_host_is_spi(mmc) is false),
the "cmd" structure (which is local to mmc_complete_op_cond() function)
is left uninitialized, and using cmd.response[0] later in the function
becomes
incorrect. And the OCR register value and the high capacity flag may be set
incorrectly.

So, this patch is not enough by itself, it needs to be modified (appended)
in order to get the correct code.

Coincidentally, the same day I submitted a patch series
("[U-Boot] [PATCH 0/6] mmc: Fix OCR polling and splitted initialization"),
that, among other changes, fixes the OCR polling problem too
("[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is
still not ready"),
but in a slightly different way, that hopefully doesn't have the mentioned
issue.

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH] mmc: fix OCR Polling

[Peng Fan]

Hi, Andrew

On 3/20/2015 3:19 PM, Andrew Gabbasov wrote:
> Hi Peng,
>
>> From: Peng.Fan at freescale.com (Peng Fan)
>> Date: Thu, 19 Mar 2015 16:22:46 +0800
>> Subject: [U-Boot] [PATCH] mmc: fix OCR Polling
>>
>> If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
>> state is transfered to Ready state, and there is no need to send
>> CMD1 again. Otherwise following CMD1 will recieve no response, or
>> timeour error from driver such as fsl_esdhc.c.
>>
>> If not into Ready state in previous CMD1, then continue CMD1 command.
>>
>> In mmc_complete_op_cond, we use the value mmc->op_cond_response
>> from mmc_send_op_cond, since there should be no CMD1 command between
>> mmc_send_op_cond and mmc_complete_op_cond
>>
>> Before fixing this, uboot log shows:
>> "
>> CMD_SEND:0
>>                  ARG                      0x00000000
>>                  MMC_RSP_NONE
>> CMD_SEND:8
>>                  ARG                      0x000001AA
>>                  MMC_RSP_R1,5,6,7         0x18EC1504
>> CMD_SEND:55
>>                  ARG                      0x00000000
>>                  MMC_RSP_R1,5,6,7         0x18EC1504
>> CMD_SEND:0
>>                  ARG                      0x00000000
>>                  MMC_RSP_NONE
>> CMD_SEND:1
>>                  ARG                      0x00000000
>>                  MMC_RSP_R3,4             0x00FF8080
>> CMD_SEND:1
>>                  ARG                      0x40300000
>>                  MMC_RSP_R3,4             0xC0FF8080 --> Already OCR_BUSY
> set
>> CMD_SEND:1
>>                  ARG                      0x40300000
>>                  MMC_RSP_R3,4             0x0096850A --> Failed CMD1
>> MMC init failed
>> "
>>
>> Using this patch, this issue is fixed, emmc can be detected correctly.
>>
>>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> ---
>>   drivers/mmc/mmc.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index a13769e..43a9a8a 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -406,14 +406,23 @@ static int mmc_complete_op_cond(struct mmc *mmc)
>>   
>>       mmc->op_cond_pending = 0;
>>       start = get_timer(0);
>> -    do {
>> +    /*
>> +     * If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
>> +     * state is transfered to Ready state, and there is no need to
>> +     * send CMD1 again. Otherwise following CMD1 will recieve no
> response,
>> +     * or timeour error from driver such as fsl_esdhc.c.
>> +     *
>> +     * If not into Ready state in previous CMD1, then continue CMD1
>> +     * command.
>> +     */
>> +    while (!(mmc->op_cond_response & OCR_BUSY)) {
>>           err = mmc_send_op_cond_iter(mmc, &cmd, 1);
>>           if (err)
>>               return err;
>>           if (get_timer(start) > timeout)
>>               return UNUSABLE_ERR;
>>           udelay(100);
>> -    } while (!(mmc->op_cond_response & OCR_BUSY));
>> +    }
>>   
>>       if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
>>           cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
>> -- 
>> 1.8.4
> After this patch, if the busy flag is indeed already set (so that the loop
> body
> is not executed), and it is not an SPI case (mmc_host_is_spi(mmc) is false),
> the "cmd" structure (which is local to mmc_complete_op_cond() function)
> is left uninitialized, and using cmd.response[0] later in the function
> becomes
> incorrect. And the OCR register value and the high capacity flag may be set
> incorrectly.
Yeah. you are right.
Maybe the following piece of code should be added to replace mmc->ocr = 
cmd.response[0]:
"
if (mmc_host_is_spi(mmc))
     mmc->ocr = cmd.response[0];
else
     mmc->ocr = mmc->op_cond_response;
"
Thanks for correcting me.
>
> So, this patch is not enough by itself, it needs to be modified (appended)
> in order to get the correct code.
>
> Coincidentally, the same day I submitted a patch series
> ("[U-Boot] [PATCH 0/6] mmc: Fix OCR polling and splitted initialization"),
> that, among other changes, fixes the OCR polling problem too
> ("[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is
> still not ready"),
> but in a slightly different way, that hopefully doesn't have the mentioned
> issue.
Your's does not have such issue:)
> Thanks.
>
> Best regards,
> Andrew
>
>
> .
>
Thanks
Peng.

[U-Boot] [PATCH] mmc: fix OCR Polling

[Andrew Gabbasov]

Hi Peng,

> From: Peng Fan [mailto:Peng.Fan at freescale.com]
> Sent: Saturday, March 21, 2015 1:34 PM
> To: Gabbasov, Andrew; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] mmc: fix OCR Polling
> 
> [skipped]
> 
> > After this patch, if the busy flag is indeed already set (so that the
> > loop body is not executed), and it is not an SPI case
> > (mmc_host_is_spi(mmc) is false), the "cmd" structure (which is local
> > to mmc_complete_op_cond() function) is left uninitialized, and using
> > cmd.response[0] later in the function becomes incorrect. And the OCR
> > register value and the high capacity flag may be set incorrectly.
> Yeah. you are right.
> Maybe the following piece of code should be added to replace mmc->ocr =
> cmd.response[0]:
> "
> if (mmc_host_is_spi(mmc))
>      mmc->ocr = cmd.response[0];
> else
>      mmc->ocr = mmc->op_cond_response;
> "
> Thanks for correcting me.

Well, there can be several ways to correct that. The easiest would be
something
similar to what you propose, but, just to avoid extra "if", we could add
    mmc->op_cond_response = cmd.response[0];
to the end of existing "if(mmc_host_is_spi(mmc))" and change
    mmc->ocr = cmd.response[0];
to
    mmc->ocr = mmc->op_cond_response;
at the end of function. Since op_cond_response should be already set from
the function beginning, this can be used immediately.

And, going further, since op_cond_response is actually the same contents
as mmc->ocr, we could combine them and use mmc->ocr at once, from
the beginning of polling loops. This is a little more complex, but makes
the code cleaner. This is what is done in one of other patches in my series
;-)

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH] mmc: fix OCR Polling

[Pantelis Antoniou]

Hi Andrew, Peng,

> On Mar 23, 2015, at 01:23 , Andrew Gabbasov <andrew_gabbasov@mentor.com> wrote:
> 
> Hi Peng,
> 
>> From: Peng Fan [mailto:Peng.Fan at freescale.com]
>> Sent: Saturday, March 21, 2015 1:34 PM
>> To: Gabbasov, Andrew; u-boot at lists.denx.de
>> Subject: Re: [U-Boot] [PATCH] mmc: fix OCR Polling
>> 
>> [skipped]
>> 
>>> After this patch, if the busy flag is indeed already set (so that the
>>> loop body is not executed), and it is not an SPI case
>>> (mmc_host_is_spi(mmc) is false), the "cmd" structure (which is local
>>> to mmc_complete_op_cond() function) is left uninitialized, and using
>>> cmd.response[0] later in the function becomes incorrect. And the OCR
>>> register value and the high capacity flag may be set incorrectly.
>> Yeah. you are right.
>> Maybe the following piece of code should be added to replace mmc->ocr =
>> cmd.response[0]:
>> "
>> if (mmc_host_is_spi(mmc))
>>     mmc->ocr = cmd.response[0];
>> else
>>     mmc->ocr = mmc->op_cond_response;
>> "
>> Thanks for correcting me.
> 
> Well, there can be several ways to correct that. The easiest would be
> something
> similar to what you propose, but, just to avoid extra "if", we could add
>    mmc->op_cond_response = cmd.response[0];
> to the end of existing "if(mmc_host_is_spi(mmc))" and change
>    mmc->ocr = cmd.response[0];
> to
>    mmc->ocr = mmc->op_cond_response;
> at the end of function. Since op_cond_response should be already set from
> the function beginning, this can be used immediately.
> 
> And, going further, since op_cond_response is actually the same contents
> as mmc->ocr, we could combine them and use mmc->ocr at once, from
> the beginning of polling loops. This is a little more complex, but makes
> the code cleaner. This is what is done in one of other patches in my series
> ;-)
> 

This does seem like a case where a simple accessor structure would help until
we figure out how to process.

Something like mmc_get_ocr() as a private API perhaps?

> Thanks.
> 
> Best regards,
> Andrew
> 

Regards

? Pantelis

> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] mmc: fix OCR Polling

[Andrew Gabbasov]

Hi Pantelis,

> From: Pantelis Antoniou [mailto:pantelis.antoniou at gmail.com]
> Sent: Friday, March 27, 2015 9:32 PM
> To: Gabbasov, Andrew
> Cc: Peng Fan; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] mmc: fix OCR Polling
> 
> Hi Andrew, Peng,
> 
> > On Mar 23, 2015, at 01:23 , Andrew Gabbasov
> <andrew_gabbasov@mentor.com> wrote:
> >
> > Hi Peng,
> >
> >> From: Peng Fan [mailto:Peng.Fan at freescale.com]
> >> Sent: Saturday, March 21, 2015 1:34 PM
> >> To: Gabbasov, Andrew; u-boot at lists.denx.de
> >> Subject: Re: [U-Boot] [PATCH] mmc: fix OCR Polling
> >>
> >> [skipped]
> >>
> >>> After this patch, if the busy flag is indeed already set (so that the
> >>> loop body is not executed), and it is not an SPI case
> >>> (mmc_host_is_spi(mmc) is false), the "cmd" structure (which is local
> >>> to mmc_complete_op_cond() function) is left uninitialized, and using
> >>> cmd.response[0] later in the function becomes incorrect. And the OCR
> >>> register value and the high capacity flag may be set incorrectly.
> >> Yeah. you are right.
> >> Maybe the following piece of code should be added to replace mmc->ocr =
> >> cmd.response[0]:
> >> "
> >> if (mmc_host_is_spi(mmc))
> >>     mmc->ocr = cmd.response[0];
> >> else
> >>     mmc->ocr = mmc->op_cond_response;
> >> "
> >> Thanks for correcting me.
> >
> > Well, there can be several ways to correct that. The easiest would be
> > something
> > similar to what you propose, but, just to avoid extra "if", we could add
> >    mmc->op_cond_response = cmd.response[0];
> > to the end of existing "if(mmc_host_is_spi(mmc))" and change
> >    mmc->ocr = cmd.response[0];
> > to
> >    mmc->ocr = mmc->op_cond_response;
> > at the end of function. Since op_cond_response should be already set from
> > the function beginning, this can be used immediately.
> >
> > And, going further, since op_cond_response is actually the same contents
> > as mmc->ocr, we could combine them and use mmc->ocr at once, from
> > the beginning of polling loops. This is a little more complex, but makes
> > the code cleaner. This is what is done in one of other patches in my series
> > ;-)
> >
> 
> This does seem like a case where a simple accessor structure would help until
> we figure out how to process.
> 
> Something like mmc_get_ocr() as a private API perhaps?

Actually I don't see any need to introduce a new accessor or any other API here.
Currently the 2 fields are used to store exactly the same data (OCR, which
is a response to send_op_cond command). But the 'op_cond_response' is used
while OCR polling and 'ocr' is filled in after its completion.
The correct solution from my point of view is to just use a single field (ocr)
from the very beginning. I have already submitted this solution in one
of my patches ("[PATCH 2/6] mmc: Avoid extra duplicate entry in mmc device structure"
in "[PATCH 0/6] mmc: Fix OCR polling and splitted initialization" series).

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH] mmc: fix OCR Polling

[Pantelis Antoniou]

Hi Peng,

> On Mar 19, 2015, at 10:22 , Peng Fan <Peng.Fan@freescale.com> wrote:
> 
> If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
> state is transfered to Ready state, and there is no need to send
> CMD1 again. Otherwise following CMD1 will recieve no response, or
> timeour error from driver such as fsl_esdhc.c.
> 
> If not into Ready state in previous CMD1, then continue CMD1 command.
> 
> In mmc_complete_op_cond, we use the value mmc->op_cond_response
> from mmc_send_op_cond, since there should be no CMD1 command between
> mmc_send_op_cond and mmc_complete_op_cond
> 
> Before fixing this, uboot log shows:
> "
> CMD_SEND:0
>                ARG                      0x00000000
>                MMC_RSP_NONE
> CMD_SEND:8
>                ARG                      0x000001AA
>                MMC_RSP_R1,5,6,7         0x18EC1504
> CMD_SEND:55
>                ARG                      0x00000000
>                MMC_RSP_R1,5,6,7         0x18EC1504
> CMD_SEND:0
>                ARG                      0x00000000
>                MMC_RSP_NONE
> CMD_SEND:1
>                ARG                      0x00000000
>                MMC_RSP_R3,4             0x00FF8080
> CMD_SEND:1
>                ARG                      0x40300000
>                MMC_RSP_R3,4             0xC0FF8080 --> Already OCR_BUSY set
> CMD_SEND:1
>                ARG                      0x40300000
>                MMC_RSP_R3,4             0x0096850A --> Failed CMD1
> MMC init failed
> "
> 
> Using this patch, this issue is fixed, emmc can be detected correctly.
> 
> 
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
> drivers/mmc/mmc.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
> 


Thanks for the patch, but Andrew?s patchset is more complete.

I?ll pick up his fix instead.

Regards

? Pantelis

[U-Boot] [PATCH] mmc: fix OCR Polling

[Peng Fan]

If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
state is transfered to Ready state, and there is no need to send
CMD1 again. Otherwise following CMD1 will recieve no response, or
timeour error from driver such as fsl_esdhc.c.

If not into Ready state in previous CMD1, then continue CMD1 command.

In mmc_complete_op_cond, we use the value mmc->op_cond_response
from mmc_send_op_cond, since there should be no CMD1 command between
mmc_send_op_cond and mmc_complete_op_cond

Before fixing this, uboot log shows:
"
CMD_SEND:0
                ARG                      0x00000000
                MMC_RSP_NONE
CMD_SEND:8
                ARG                      0x000001AA
                MMC_RSP_R1,5,6,7         0x18EC1504
CMD_SEND:55
                ARG                      0x00000000
                MMC_RSP_R1,5,6,7         0x18EC1504
CMD_SEND:0
                ARG                      0x00000000
                MMC_RSP_NONE
CMD_SEND:1
                ARG                      0x00000000
                MMC_RSP_R3,4             0x00FF8080
CMD_SEND:1
                ARG                      0x40300000
                MMC_RSP_R3,4             0xC0FF8080 --> Already OCR_BUSY set
CMD_SEND:1
                ARG                      0x40300000
                MMC_RSP_R3,4             0x0096850A --> Failed CMD1
MMC init failed
"

Using this patch, this issue is fixed, emmc can be detected correctly.


Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
---
 drivers/mmc/mmc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index a13769e..43a9a8a 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -406,14 +406,23 @@ static int mmc_complete_op_cond(struct mmc *mmc)
 
 	mmc->op_cond_pending = 0;
 	start = get_timer(0);
-	do {
+	/*
+	 * If in mmc_send_op_cond, OCR_BUSY is set in CMD1's response, then
+	 * state is transfered to Ready state, and there is no need to
+	 * send CMD1 again. Otherwise following CMD1 will recieve no response,
+	 * or timeour error from driver such as fsl_esdhc.c.
+	 *
+	 * If not into Ready state in previous CMD1, then continue CMD1
+	 * command.
+	 */
+	while (!(mmc->op_cond_response & OCR_BUSY)) {
 		err = mmc_send_op_cond_iter(mmc, &cmd, 1);
 		if (err)
 			return err;
 		if (get_timer(start) > timeout)
 			return UNUSABLE_ERR;
 		udelay(100);
-	} while (!(mmc->op_cond_response & OCR_BUSY));
+	}
 
 	if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
 		cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
-- 
1.8.4