Hi Pantelis,
From: Pantelis Antoniou [mailto:pantelis.antoniou@gmail.com] Sent: Friday, March 27, 2015 9:32 PM To: Gabbasov, Andrew Cc: Peng Fan; u-boot@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@freescale.com] Sent: Saturday, March 21, 2015 1:34 PM To: Gabbasov, Andrew; u-boot@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