From: Andrew Gabbasov <andrew_gabbasov@mentor.com>
Date: Mon, 30 Mar 2015 14:15:31 +0300
Subject: [U-Boot] [PATCH] mmc: fix OCR Polling
In-Reply-To: <7F19964E-04B9-4626-945C-869B3A268D3B@gmail.com>
References: <000001d062de$30a89240$91f9b6c0$@mentor.com>
 <550D48FE.1040205@freescale.com> <000101d06542$a2a45530$e7ecff90$@mentor.com>
 <7F19964E-04B9-4626-945C-869B3A268D3B@gmail.com>
Message-ID: <000001d06ada$d6252e00$826f8a00$@mentor.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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