[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Some MMC cards come to ready state quite quickly, so that the respective
flag appears to be set in mmc_send_op_cond already. In this case trying
to continue polling the card with CMD1 in mmc_complete_op_cond is incorrect
and may lead to unpredictable results. So check the flag before polling
and skip it appropriately.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/mmc/mmc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index d073d79..42af47c 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc)
 	int err;
 
 	mmc->op_cond_pending = 0;
-	start = get_timer(0);
-	do {
-		err = mmc_send_op_cond_iter(mmc, 1);
-		if (err)
-			return err;
-		if (get_timer(start) > timeout)
-			return UNUSABLE_ERR;
-		udelay(100);
-	} while (!(mmc->ocr & OCR_BUSY));
+	if (!(mmc->ocr & OCR_BUSY)) {
+		start = get_timer(0);
+		do {
+			err = mmc_send_op_cond_iter(mmc, 1);
+			if (err)
+				return err;
+			if (get_timer(start) > timeout)
+				return UNUSABLE_ERR;
+			udelay(100);
+		} while (!(mmc->ocr & OCR_BUSY));
+	}
 
 	if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
 		cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
-- 
2.1.0

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Peng.Fan at freescale.com]

Hi, Andrew

There is already a patch to fix this issue.
Patchwork: https://patchwork.ozlabs.org/patch/451775/

Regards,
Peng.

-----Original Message-----
From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Andrew Gabbasov
Sent: Thursday, March 19, 2015 8:44 PM
To: u-boot at lists.denx.de
Subject: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

Some MMC cards come to ready state quite quickly, so that the respective flag appears to be set in mmc_send_op_cond already. In this case trying to continue polling the card with CMD1 in mmc_complete_op_cond is incorrect and may lead to unpredictable results. So check the flag before polling and skip it appropriately.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/mmc/mmc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d073d79..42af47c 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc)
 	int err;
 
 	mmc->op_cond_pending = 0;
-	start = get_timer(0);
-	do {
-		err = mmc_send_op_cond_iter(mmc, 1);
-		if (err)
-			return err;
-		if (get_timer(start) > timeout)
-			return UNUSABLE_ERR;
-		udelay(100);
-	} while (!(mmc->ocr & OCR_BUSY));
+	if (!(mmc->ocr & OCR_BUSY)) {
+		start = get_timer(0);
+		do {
+			err = mmc_send_op_cond_iter(mmc, 1);
+			if (err)
+				return err;
+			if (get_timer(start) > timeout)
+				return UNUSABLE_ERR;
+			udelay(100);
+		} while (!(mmc->ocr & OCR_BUSY));
+	}
 
 	if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
 		cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
--
2.1.0

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

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Hi Peng,

> From: Peng.Fan at freescale.com [Peng.Fan at freescale.com]
> Sent: Friday, March 20, 2015 03:51
> To: Gabbasov, Andrew; u-boot at lists.denx.de
> Subject: RE: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
only if        it is still not ready
> 
> Hi, Andrew
> 
> There is already a patch to fix this issue.
> Patchwork: https://patchwork.ozlabs.org/patch/451775/
> 
> Regards,
> Peng.

Yes, I noticed it just after I sent this series. ;-)
Unfortunately, the patch, that you mention, has some drawback that I
described
in a separate message (in response to the patch). Basically, it leaves the
code
in some not quite correct state.
This series hopefully does not have this drawback and besides fixing the
issue
has some more useful changes in a single complex.

Thanks.

Best regards,
Andrew


-----Original Message-----
From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Andrew
Gabbasov
Sent: Thursday, March 19, 2015 8:44 PM
To: u-boot at lists.denx.de
Subject: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if
it is still not ready

Some MMC cards come to ready state quite quickly, so that the respective
flag appears to be set in mmc_send_op_cond already. In this case trying to
continue polling the card with CMD1 in mmc_complete_op_cond is incorrect and
may lead to unpredictable results. So check the flag before polling and skip
it appropriately.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/mmc/mmc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d073d79..42af47c
100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc)
        int err;

        mmc->op_cond_pending = 0;
-       start = get_timer(0);
-       do {
-               err = mmc_send_op_cond_iter(mmc, 1);
-               if (err)
-                       return err;
-               if (get_timer(start) > timeout)
-                       return UNUSABLE_ERR;
-               udelay(100);
-       } while (!(mmc->ocr & OCR_BUSY));
+       if (!(mmc->ocr & OCR_BUSY)) {
+               start = get_timer(0);
+               do {
+                       err = mmc_send_op_cond_iter(mmc, 1);
+                       if (err)
+                               return err;
+                       if (get_timer(start) > timeout)
+                               return UNUSABLE_ERR;
+                       udelay(100);
+               } while (!(mmc->ocr & OCR_BUSY));
+       }

        if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
                cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
--
2.1.0

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

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Troy Kisky]

On 3/19/2015 6:51 PM, Peng.Fan at freescale.com wrote:
> Hi, Andrew
> 
> There is already a patch to fix this issue.
> Patchwork: https://patchwork.ozlabs.org/patch/451775/
> 
> Regards,
> Peng.
> 
> -----Original Message-----
> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Andrew Gabbasov
> Sent: Thursday, March 19, 2015 8:44 PM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready
> 
> Some MMC cards come to ready state quite quickly, so that the respective flag appears to be set in mmc_send_op_cond already. In this case trying to continue polling the card with CMD1 in mmc_complete_op_cond is incorrect and may lead to unpredictable results. So check the flag before polling and skip it appropriately.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> ---
>  drivers/mmc/mmc.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d073d79..42af47c 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc)
>  	int err;
>  
>  	mmc->op_cond_pending = 0;
> -	start = get_timer(0);
> -	do {
> -		err = mmc_send_op_cond_iter(mmc, 1);
> -		if (err)
> -			return err;
> -		if (get_timer(start) > timeout)
> -			return UNUSABLE_ERR;
> -		udelay(100);
> -	} while (!(mmc->ocr & OCR_BUSY));
> +	if (!(mmc->ocr & OCR_BUSY)) {
> +		start = get_timer(0);
> +		do {
> +			err = mmc_send_op_cond_iter(mmc, 1);
> +			if (err)
> +				return err;
> +			if (get_timer(start) > timeout)
> +				return UNUSABLE_ERR;
> +			udelay(100);
> +		} while (!(mmc->ocr & OCR_BUSY));
> +	}
>  
>  	if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
>  		cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
> --


Here's another patch that solves the problem a little earlier. It has this disadvantage of being
slightly bigger,
though it makes the code look better.


https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca


I'll ack any version as they all seem to solve the problem.

Troy

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Hi Troy,

> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> Sent: Friday, March 20, 2015 9:39 PM
> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
> Cc: Eric Nelson
> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
> only if it is still not ready
> 
> [skipped]
> 
> Here's another patch that solves the problem a little earlier. It has this
> disadvantage of being slightly bigger, though it makes the code look
better.
> 
> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
> 

I have a couple of doubts regarding that patch.

First, my personal taste objects to such duplicating of the code
(I mean setting of version, ocr, rca, etc fields of mmc structure).
If we'll have to change or add anything to these settings, we'll have to
make
the same change in 2 different place, which is error-prone and extremely
inconvenient from maintenance point of view.

Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
register
with a special command for SPI host case (thus setting ocr field
incorrectly),
if the card comes to ready state with the first attempt?

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Troy Kisky]

On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
> Hi Troy,
> 
>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>> Sent: Friday, March 20, 2015 9:39 PM
>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
>> Cc: Eric Nelson
>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
>> only if it is still not ready
>>
>> [skipped]
>>
>> Here's another patch that solves the problem a little earlier. It has this
>> disadvantage of being slightly bigger, though it makes the code look
> better.
>>
>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
>>
> 
> I have a couple of doubts regarding that patch.
> 
> First, my personal taste objects to such duplicating of the code
> (I mean setting of version, ocr, rca, etc fields of mmc structure).
> If we'll have to change or add anything to these settings, we'll have to
> make
> the same change in 2 different place, which is error-prone and extremely
> inconvenient from maintenance point of view.
> 
> Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
> register
> with a special command for SPI host case (thus setting ocr field
> incorrectly),
> if the card comes to ready state with the first attempt?

That's a good argument for a subroutine to be doing that work instead
of in two places.

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Hi Troy,

> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> Sent: Monday, March 23, 2015 11:09 PM
> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
> Cc: Eric Nelson
> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
> only if it is still not ready
> 
> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
> > Hi Troy,
> >
> >> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> >> Sent: Friday, March 20, 2015 9:39 PM
> >> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
> >> Cc: Eric Nelson
> >> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
> >> OCR only if it is still not ready
> >>
> >> [skipped]
> >>
> >> Here's another patch that solves the problem a little earlier. It has
> >> this disadvantage of being slightly bigger, though it makes the code
> >> look
> > better.
> >>
> >> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
> >>
> >
> > I have a couple of doubts regarding that patch.
> >
> > First, my personal taste objects to such duplicating of the code (I
> > mean setting of version, ocr, rca, etc fields of mmc structure).
> > If we'll have to change or add anything to these settings, we'll have
> > to make the same change in 2 different place, which is error-prone and
> > extremely inconvenient from maintenance point of view.
> >
> > Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
> > register with a special command for SPI host case (thus setting ocr
> > field incorrectly), if the card comes to ready state with the first
> > attempt?
> 
> That's a good argument for a subroutine to be doing that work instead of
in two
> places.

In some sense the second function of these two (mmc_complete_op_cond())
is exactly such subroutine ;-) It is doing this work of final settings and
actions
after making OCR polling. Although completing the polling itself
in some cases too.
Actually, it seems that introducing of one more service function would
make the code a little too "chopped" into too small pieces, and also
further less similar to SD (as opposed to MMC) case.

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Troy Kisky]

On 3/24/2015 9:33 AM, Andrew Gabbasov wrote:
> Hi Troy,
> 
>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>> Sent: Monday, March 23, 2015 11:09 PM
>> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
>> Cc: Eric Nelson
>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
>> only if it is still not ready
>>
>> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
>>> Hi Troy,
>>>
>>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>>>> Sent: Friday, March 20, 2015 9:39 PM
>>>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
>>>> Cc: Eric Nelson
>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
>>>> OCR only if it is still not ready
>>>>
>>>> [skipped]
>>>>
>>>> Here's another patch that solves the problem a little earlier. It has
>>>> this disadvantage of being slightly bigger, though it makes the code
>>>> look
>>> better.
>>>>
>>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
>>>>
>>>
>>> I have a couple of doubts regarding that patch.
>>>
>>> First, my personal taste objects to such duplicating of the code (I
>>> mean setting of version, ocr, rca, etc fields of mmc structure).
>>> If we'll have to change or add anything to these settings, we'll have
>>> to make the same change in 2 different place, which is error-prone and
>>> extremely inconvenient from maintenance point of view.
>>>
>>> Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
>>> register with a special command for SPI host case (thus setting ocr
>>> field incorrectly), if the card comes to ready state with the first
>>> attempt?
>>
>> That's a good argument for a subroutine to be doing that work instead of
> in two
>> places.
> 
> In some sense the second function of these two (mmc_complete_op_cond())
> is exactly such subroutine ;-) It is doing this work of final settings and
> actions
> after making OCR polling. Although completing the polling itself
> in some cases too.
> Actually, it seems that introducing of one more service function would
> make the code a little too "chopped" into too small pieces, and also
> further less similar to SD (as opposed to MMC) case.
> 
> Thanks.
> 


I've already said that I'm fine with any patch that fixes the problem, so there is
no need to convince me of anything. I just wanted to show that setting the pending
flag, when the command is actually complete, does not make a lot of sense.

Thanks
Troy

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> Sent: Tuesday, March 24, 2015 9:03 PM
> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
> Cc: Eric Nelson
> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
> only if it is still not ready
> 
> On 3/24/2015 9:33 AM, Andrew Gabbasov wrote:
> > Hi Troy,
> >
> >> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> >> Sent: Monday, March 23, 2015 11:09 PM
> >> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
> >> Cc: Eric Nelson
> >> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
> >> OCR only if it is still not ready
> >>
> >> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
> >>> Hi Troy,
> >>>
> >>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> >>>> Sent: Friday, March 20, 2015 9:39 PM
> >>>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
> >>>> Cc: Eric Nelson
> >>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card
> >>>> for OCR only if it is still not ready
> >>>>
> >>>> [skipped]
> >>>>
> >>>> Here's another patch that solves the problem a little earlier. It
> >>>> has this disadvantage of being slightly bigger, though it makes the
> >>>> code look
> >>> better.
> >>>>
> >>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
> >>>>
> >>>
> >>> I have a couple of doubts regarding that patch.
> >>>
> >>> First, my personal taste objects to such duplicating of the code (I
> >>> mean setting of version, ocr, rca, etc fields of mmc structure).
> >>> If we'll have to change or add anything to these settings, we'll
> >>> have to make the same change in 2 different place, which is
> >>> error-prone and extremely inconvenient from maintenance point of view.
> >>>
> >>> Second, what about SPI mode? Doesn't this patch skip retrieving of
> >>> OCR register with a special command for SPI host case (thus setting
> >>> ocr field incorrectly), if the card comes to ready state with the
> >>> first attempt?
> >>
> >> That's a good argument for a subroutine to be doing that work instead
> >> of
> > in two
> >> places.
> >
> > In some sense the second function of these two
> > (mmc_complete_op_cond()) is exactly such subroutine ;-) It is doing
> > this work of final settings and actions after making OCR polling.
> > Although completing the polling itself in some cases too.
> > Actually, it seems that introducing of one more service function would
> > make the code a little too "chopped" into too small pieces, and also
> > further less similar to SD (as opposed to MMC) case.
> >
> > Thanks.
> >
> 
> I've already said that I'm fine with any patch that fixes the problem, so
there is
> no need to convince me of anything. I just wanted to show that setting the
> pending flag, when the command is actually complete, does not make a lot
of
> sense.

I absolutely agree. In fact, this flag in current implementation just
indicates
the branch that the MMC card case is being processed. If the version field,
differing SD and MMC cases, would be set a little earlier, for example,
directly in mmc_start_init(), we could just use !IS_SD() instead of this
pending flag. And thinking further, it's not quite clear why the splitting
of OCR polling was done for MMC case only, but not for SD too.
So, there is still the room for further improving the code, may be even
some reorganizing, but I had to stop somewhere, unless there is
the direct necessity ;-)

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Pantelis Antoniou]

Hi Andrew,

> On Mar 24, 2015, at 21:02 , Andrew Gabbasov <andrew_gabbasov@mentor.com> wrote:
> 
>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>> Sent: Tuesday, March 24, 2015 9:03 PM
>> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
>> Cc: Eric Nelson
>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
>> only if it is still not ready
>> 
>> On 3/24/2015 9:33 AM, Andrew Gabbasov wrote:
>>> Hi Troy,
>>> 
>>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>>>> Sent: Monday, March 23, 2015 11:09 PM
>>>> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
>>>> Cc: Eric Nelson
>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
>>>> OCR only if it is still not ready
>>>> 
>>>> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
>>>>> Hi Troy,
>>>>> 
>>>>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>>>>>> Sent: Friday, March 20, 2015 9:39 PM
>>>>>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
>>>>>> Cc: Eric Nelson
>>>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card
>>>>>> for OCR only if it is still not ready
>>>>>> 
>>>>>> [skipped]
>>>>>> 
>>>>>> Here's another patch that solves the problem a little earlier. It
>>>>>> has this disadvantage of being slightly bigger, though it makes the
>>>>>> code look
>>>>> better.
>>>>>> 
>>>>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
>>>>>> 
>>>>> 
>>>>> I have a couple of doubts regarding that patch.
>>>>> 
>>>>> First, my personal taste objects to such duplicating of the code (I
>>>>> mean setting of version, ocr, rca, etc fields of mmc structure).
>>>>> If we'll have to change or add anything to these settings, we'll
>>>>> have to make the same change in 2 different place, which is
>>>>> error-prone and extremely inconvenient from maintenance point of view.
>>>>> 
>>>>> Second, what about SPI mode? Doesn't this patch skip retrieving of
>>>>> OCR register with a special command for SPI host case (thus setting
>>>>> ocr field incorrectly), if the card comes to ready state with the
>>>>> first attempt?
>>>> 
>>>> That's a good argument for a subroutine to be doing that work instead
>>>> of
>>> in two
>>>> places.
>>> 
>>> In some sense the second function of these two
>>> (mmc_complete_op_cond()) is exactly such subroutine ;-) It is doing
>>> this work of final settings and actions after making OCR polling.
>>> Although completing the polling itself in some cases too.
>>> Actually, it seems that introducing of one more service function would
>>> make the code a little too "chopped" into too small pieces, and also
>>> further less similar to SD (as opposed to MMC) case.
>>> 
>>> Thanks.
>>> 
>> 
>> I've already said that I'm fine with any patch that fixes the problem, so
> there is
>> no need to convince me of anything. I just wanted to show that setting the
>> pending flag, when the command is actually complete, does not make a lot
> of
>> sense.
> 
> I absolutely agree. In fact, this flag in current implementation just
> indicates
> the branch that the MMC card case is being processed. If the version field,
> differing SD and MMC cases, would be set a little earlier, for example,
> directly in mmc_start_init(), we could just use !IS_SD() instead of this
> pending flag. And thinking further, it's not quite clear why the splitting
> of OCR polling was done for MMC case only, but not for SD too.
> So, there is still the room for further improving the code, may be even
> some reorganizing, but I had to stop somewhere, unless there is
> the direct necessity ;-)
> 

Your patch fixes the current problem nicely. There is a lot of cruft around, we?ll
get around looking it at some point in the future (yeah, maybe) :)

Thanks for the good work, it?s fine as it is right now.

> Thanks.
> 
> Best regards,
> Andrew
> 

Regards

? Pantelis

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Pantelis Antoniou]

Hi Andrew,

> On Mar 19, 2015, at 14:44 , Andrew Gabbasov <andrew_gabbasov@mentor.com> wrote:
> 
> Some MMC cards come to ready state quite quickly, so that the respective
> flag appears to be set in mmc_send_op_cond already. In this case trying
> to continue polling the card with CMD1 in mmc_complete_op_cond is incorrect
> and may lead to unpredictable results. So check the flag before polling
> and skip it appropriately.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> ---
> drivers/mmc/mmc.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index d073d79..42af47c 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc)
> 	int err;
> 
> 	mmc->op_cond_pending = 0;
> -	start = get_timer(0);
> -	do {
> -		err = mmc_send_op_cond_iter(mmc, 1);
> -		if (err)
> -			return err;
> -		if (get_timer(start) > timeout)
> -			return UNUSABLE_ERR;
> -		udelay(100);
> -	} while (!(mmc->ocr & OCR_BUSY));
> +	if (!(mmc->ocr & OCR_BUSY)) {
> +		start = get_timer(0);
> +		do {
> +			err = mmc_send_op_cond_iter(mmc, 1);
> +			if (err)
> +				return err;
> +			if (get_timer(start) > timeout)
> +				return UNUSABLE_ERR;
> +			udelay(100);
> +		} while (!(mmc->ocr & OCR_BUSY));
> +	}
> 
> 	if (mmc_host_is_spi(mmc)) { /* read OCR for spi */
> 		cmd.cmdidx = MMC_CMD_SPI_READ_OCR;
> -- 
> 2.1.0
> 


Thanks, Applied.

? Pantelis

PS. Peng, I?m picking up this instead of your patch.