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