mmc: Continue polling MMC card for OCR only if it is still not ready

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

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

Hi Troy,

Quoted 11 lines

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.

Quoted 3 lines

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?

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

Quoted 27 lines · Andrew Gabbasov

Hi Troy,

Quoted 7 lines

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

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 4 lines

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

places.

Hi Troy,

Quoted 35 lines

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

Quoted 27 lines · Andrew Gabbasov

Hi Troy,

Quoted 7 lines

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

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 6 lines

Quoted 4 lines

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

Quoted 1 line

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.

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

Quoted 55 lines · Andrew Gabbasov

Hi Troy,

Quoted 35 lines

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

Quoted 27 lines · Andrew Gabbasov

Hi Troy,

Quoted 7 lines

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

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 2 lines

Thanks.

I've already said that I'm fine with any patch that fixes the problem, so

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

sense.

Quoted 64 lines

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

Quoted 55 lines · Andrew Gabbasov

Hi Troy,

Quoted 35 lines

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

Quoted 27 lines · Andrew Gabbasov

Hi Troy,

Quoted 7 lines

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

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

Thanks.

I've already said that I'm fine with any patch that fixes the problem, so

there is

Quoted 2 lines

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

Quoted 1 line

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