[U-Boot] [PATCH] mmc: send CMD0 before CMD1 for some MMC cards

[Jaehoon Chung <jh80.chung@samsung.com>]

On 08/02/2016 04:13 PM, Jaehoon Chung wrote:
> On 08/02/2016 04:03 PM, Yangbo Lu wrote:
>> Hi Jaehoon,
>>
>>
>>> -----Original Message-----
>>> From: Jaehoon Chung [mailto:jh80.chung at samsung.com]
>>> Sent: Thursday, July 28, 2016 4:40 PM
>>> To: Yangbo Lu; Ziyuan Xu; u-boot at lists.denx.de; Tom Rini
>>> Cc: Pantelis Antoniou
>>> Subject: Re: [U-Boot] [PATCH] mmc: send CMD0 before CMD1 for some MMC
>>> cards
>>>
>>> Hi Yangbo,
>>>
>>> On 07/28/2016 11:45 AM, Yangbo Lu wrote:
>>>> Hi Ziyuan and Jaehoon,
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Ziyuan Xu [mailto:xzy.xu at rock-chips.com]
>>>>> Sent: Wednesday, July 27, 2016 9:37 PM
>>>>> To: Jaehoon Chung; Yangbo Lu; u-boot at lists.denx.de; Tom Rini
>>>>> Cc: Pantelis Antoniou
>>>>> Subject: Re: [U-Boot] [PATCH] mmc: send CMD0 before CMD1 for some MMC
>>>>> cards
>>>>>
>>>>>
>>>>>
>>>>> On 2016?07?27? 19:15, Jaehoon Chung wrote:
>>>>>> On 07/27/2016 04:28 PM, Yangbo Lu wrote:
>>>>>>> Hi Tom,
>>>>>>>
>>>>>>> Could you help to assign this mmc patch reviewing to right person?
>>>>>>> It seems no one had reviewed it for almost half year.
>>>>>>>
>>>>>>> And another my mmc patch also needs to be reviewed.
>>>>>>> I submitted in May. Please help.
>>>>>>> http://patchwork.ozlabs.org/patch/624448/
>>>>>>>
>>>>>>>
>>>>>>> Thank you very much.
>>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Yangbo Lu
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yangbo Lu [mailto:yangbo.lu at nxp.com]
>>>>>>>> Sent: Wednesday, March 09, 2016 11:00 AM
>>>>>>>> To: u-boot at lists.denx.de
>>>>>>>> Cc: Pantelis Antoniou; Yangbo Lu
>>>>>>>> Subject: [PATCH] mmc: send CMD0 before CMD1 for some MMC cards
>>>>>>>>
>>>>>>>> When the MMC framework was added in u-boot, the mmc_go_idle was
>>>>>>>> added before mmc_send_op_cond_iter in function mmc_send_op_cond
>>>>>>>> annotating that some cards seemed to need this. Actually, we still
>>>>>>>> need to do this in function mmc_complete_op_cond for those cards.
>>>>>>>> This has been verified on Micron MTFC4GACAECN eMMC chip.
>>>>>> If there is no go_idle(), then what happen?
>>>>>> If you share the information more, i can check the more..
>>>>> Sounds interesting, I also want want to know what happen?
>>>>> It seems like you failed in CMD1? The eMMC device was always in busy
>>>>> device within 1 second?
>>>>
>>>> [Lu Yangbo-B47093] This was an issue which our customer reported and
>>> required us to fix in March.
>>>> They used NXP LS1020A platform and Micron MTFC4GACAECN eMMC, and
>>> reported they had to add CMD0 as below.
>>>> Otherwise it couldn?t read OCR.
>>>>
>>>> static int mmc_complete_op_cond(struct mmc *mmc) {
>>>> 	struct mmc_cmd cmd;
>>>> 	int timeout = 1000;
>>>> 	uint start;
>>>> 	int err;
>>>>
>>>> #if defined (XXX_CHANGED)
>>>> 	// our eMMC chip (Micron MTFC4GACAECN) requires that it be put in
>>> idle mode before
>>>> 	// negociating the operating voltage levels.
>>>> 	mmc_go_idle(mmc);
>>>> #endif
>>>
>>> Well, it seems to fix workaround. mmc_go_idle() means Device Reset.
>>>
>>> mmc_complete_op_cond() function has added for reducing the booting time.
>>> If mmc_go_idle() is added at here, there is no benefit, and it should be
>>> back to old concept.
>>>
>>> I don't agree this patch..now.
>>>
>>
>> [Lu Yangbo-B47093] Did you notice mmc_send_op_cond function? Before mmc_send_op_cond_iter sending CMD1, there always was mmc_go_idle.
>> I don?t know why said 'Some cards seem to need this', but it must fix some issue.
>>
>> static int mmc_send_op_cond(struct mmc *mmc)
>> {
>>         int err, i;
>>
>>         /* Some cards seem to need this */
>>         mmc_go_idle(mmc);
>>
>>         /* Asking to the card its capabilities */
>>         for (i = 0; i < 2; i++) {
>>                 err = mmc_send_op_cond_iter(mmc, i != 0);
>>                 if (err)
>>                         return err;
>>
>>                 /* exit if not busy (flag seems to be inverted) */
>>                 if (mmc->ocr & OCR_BUSY)
>>                         break;
>>         }
>>         mmc->op_cond_pending = 1;
>>         return 0;
>> }
>>
>> Now in mmc_complete_op_cond function, there may be the same issue. Without the mmc_go_idle, mmc_send_op_cond_iter failed to get ocr.
>> Maybe I should move mmc_go_idle just before mmc_send_op_cond_iter, like this.
>>
>> static int mmc_complete_op_cond(struct mmc *mmc)
>> {
>>         struct mmc_cmd cmd;
>>         int timeout = 1000;
>>         uint start;
>>         int err;
>>
>>         mmc->op_cond_pending = 0;
>>         if (!(mmc->ocr & OCR_BUSY)) {
>>                 start = get_timer(0);
>>                 while (1) {
>> 				/* Some cards seem to need this */
>> 				mmc_go_idle(mmc);
>>                         err = mmc_send_op_cond_iter(mmc, 1);
> 
> If you need to add the mmc_go_idle(), then I think this point is right. :)

One more..i don't check in more detail.
Could you consider the below things?

1. located mmc_go_idle() before while()

2. included mmc_go_idle in while()

Best Regards,
Jaehoon Chung

> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> If you think it's not proper, do you have any suggestion?
>> :)
>>
>> Thanks a lot.
>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>>
>>>> I hadn?t reproduce this to get more details about this issue since I
>>> didn?t have one this kind eMMC card that time.
>>>> Thanks.
>>>>
>>>>>>
>>>>>> Best Regards,
>>>>>> Jaehoon Chung
>>>>>>
>>>>>>>> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
>>>>>>>> ---
>>>>>>>>   drivers/mmc/mmc.c | 3 +++
>>>>>>>>   1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
>>>>>>>> ede5d6e..82e3268
>>>>>>>> 100644
>>>>>>>> --- a/drivers/mmc/mmc.c
>>>>>>>> +++ b/drivers/mmc/mmc.c
>>>>>>>> @@ -418,6 +418,9 @@ static int mmc_complete_op_cond(struct mmc *mmc)
>>>>>>>>   	uint start;
>>>>>>>>   	int err;
>>>>>>>>
>>>>>>>> +	/* Some cards seem to need this */
>>>>>>>> +	mmc_go_idle(mmc);
>>>>>>>> +
>>>>>>>>   	mmc->op_cond_pending = 0;
>>>>>>>>   	if (!(mmc->ocr & OCR_BUSY)) {
>>>>>>>>   		start = get_timer(0);
>>>>>>>> --
>>>>>>>> 2.1.0.27.g96db324
>>>>>>> _______________________________________________
>>>>>>> U-Boot mailing list
>>>>>>> U-Boot at lists.denx.de
>>>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> _______________________________________________
>>>>>> U-Boot mailing list
>>>>>> U-Boot at lists.denx.de
>>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>
>>>>
>>
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>