[PATCH] Revert "mmc: sdhci: set to INT_DATA_END when there are data"

Thu May 20 00:03:07 CEST 2021

Dear Andy,

On 5/12/21 7:09 AM, Jaehoon Chung wrote:
> Dear Andy
> 
> On 5/11/21 4:39 PM, Andy.Wu at sony.com wrote:
>> Hi Jaehoon
>>
>>> If you're ok, I will test after reverted the patch on tomorrow, and I will share
>>> result.
>>> Or I will try to reproduce timeout issue on 410c board.
>>
>> Sorry, but is there any update for this comments?
> 
> Sorry for replying too late. I had been doing other things.
> So if you're ok, i will test on my own boards with your patch until this weekend.
> (If possible, i will also check 410c board.)

Reviewed-by: Jaehoon Chung <jh80.chung at samsung.com>

If a problem is occurred again, I will fix again.

Best Regards,
Jaehoon Chung

> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> Best Regards
>> Andy Wu
>>
>>> -----Original Message-----
>>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Jaehoon Chung
>>> Sent: Tuesday, April 6, 2021 7:13 PM
>>> To: Peng Fan <peng.fan at nxp.com>; jh80.chung at gmail.com;
>>> u-boot at lists.denx.de
>>> Subject: Re: [PATCH] Revert "mmc: sdhci: set to INT_DATA_END when there are
>>> data"
>>>
>>> Hi Peng,
>>>
>>> On 4/6/21 7:02 PM, Peng Fan wrote:
>>>>> Subject: RE: [PATCH] Revert "mmc: sdhci: set to INT_DATA_END when
>>>>> there are data"
>>>>>
>>>>> Hi Jaehoon
>>>>>
>>>>>> Did you test on latest u-boot?  v2018.01 was too old version.
>>>>>>
>>>>> Yes, we tested on v2020.04, although there is no such issue, but I
>>>>> think it just depends on call sequence timing.
>>>>>
>>>>>> And if my understanding is right, INT_DATA_END needs to set when
>>>>>> there is a data. If there is no data, it doesn't need to set to it.
>>>>>> Logically, there is no
>>>>> problem, isn't?
>>>>>>
>>>>> If there is no data, but current command is RESPONSE-WITH-BUSY (like
>>>>> CMD6) type, the INT_DATA_END needs set also, refer sdhci spec
>>>>> explanation for INT_DATA_END bit:
>>>>>
>>>>> Transfer Complete
>>>>> This bit indicates stop of transaction on three cases:
>>>>> ...
>>>>> (2) Completion of a command pairing with response-with-busy (R1b,
>>>>> R5b)
>>>>>
>>>>> So, our modification just within if (cmd->resp_type & MMC_RSP_BUSY)
>>>>> judgment.
>>>>
>>>> Jaehoon,
>>>>
>>>> Do you see any issue if revert the patch?
>>>
>>> If you're ok, I will test after reverted the patch on tomorrow, and I will share
>>> result.
>>> Or I will try to reproduce timeout issue on 410c board.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>>
>>>> Thanks,
>>>> Peng.
>>>>
>>>>>
>>>>> Best Regards
>>>>> Andy Wu
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jaehoon Chung <jh80.chung at gmail.com>
>>>>>> Sent: Monday, March 22, 2021 6:03 PM
>>>>>> To: Wu, Andy <Andy.Wu at sony.com>; jh80.chung at samsung.com; Mo,
>>>>> Yuezhang
>>>>>> <Yuezhang.Mo at sony.com>; u-boot at lists.denx.de
>>>>>> Cc: peng.fan at nxp.com; cpgs at samsung.com
>>>>>> Subject: Re: [PATCH] Revert "mmc: sdhci: set to INT_DATA_END when
>>>>>> there are data"
>>>>>>
>>>>>> Hi Andy,
>>>>>>
>>>>>> On 3/18/21 10:59 AM, Andy.Wu at sony.com wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>>> I don't want to revert this commit. Is there any issue without this?
>>>>>>> Without revert commit 17ea3c86, Some board, like Dragonboard 410c
>>>>>>> will meet transfer data timeout error (we used v2018.01):
>>>>>>>
>>>>>>> U-Boot 2018.01 (Nov 26 2020 - 03:31:09 +0000) Qualcomm-DragonBoard
>>>>>>> 410C
>>>>>>>
>>>>>>> DRAM:  986 MiB
>>>>>>> MMC:   sdhci at 07824000: 0, sdhci at 07864000: 1
>>>>>>> sdhci_transfer_data: Transfer data timeout
>>>>>>> mmc_init: -70, time 10645
>>>>>>> *** Warning - No block device, using default environment
>>>>>>>
>>>>>>> And it seems the 17ea3c86 not followed the sdhci specification as
>>>>>>> transfer complete bit should be wait for the BUSY status de-assert.
>>>>>>>
>>>>>>> Kernel side code also wait the transfer complete bit for
>>>>>>> response-with-busy command.
>>>>>>
>>>>>> Did you test on latest u-boot?  v2018.01 was too old version.
>>>>>>
>>>>>> And if my understanding is right, INT_DATA_END needs to set when
>>>>>> there is a data.
>>>>>>
>>>>>> If there is no data, it doesn't need to set to it. Logically, there
>>>>>> is no problem,
>>>>> isn't?
>>>>>>
>>>>>> I will check with QC 410C board for clarifying this problem.
>>>>>>
>>>>>>>
>>>>>>>> Without this patch, some SoCs have timeout error with stop command.
>>>>>>> Sorry, we didn't meet this stop command timeout issue, but I guess
>>>>>>> it maybe another issue, and can be fixed with modification limited
>>>>>>> to stop command, not for all response-with-busy command.
>>>>>>>
>>>>>>> Does the SDHCI_QUIRK_BROKEN_R1B can be used for this case?
>>>>>>
>>>>>> Well, it can be used.
>>>>>>
>>>>>> Best Regards,
>>>>>>
>>>>>> Jaehoon Chung
>>>>>>
>>>>>>>
>>>>>>> Best Regards
>>>>>>> Andy Wu
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Jaehoon
>>>>>>>> Chung
>>>>>>>> Sent: Thursday, March 18, 2021 6:44 AM
>>>>>>>> To: Mo, Yuezhang <Yuezhang.Mo at sony.com>; u-boot at lists.denx.de
>>>>>>>> Cc: peng.fan at nxp.com; cpgs at samsung.com
>>>>>>>> Subject: Re: [PATCH] Revert "mmc: sdhci: set to INT_DATA_END when
>>>>>>>> there are data"
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On 3/17/21 3:44 PM, Yuezhang.Mo at sony.com wrote:
>>>>>>>>> This reverts commit 17ea3c862865c0d704646f67dbf8412f9ff54f59.
>>>>>>>>>
>>>>>>>>> In eMMC specification, for the response-with-busy(R1b, R5b)
>>>>>>>>> command, the DAT0 will driven to LOW as BUSY status, and in sdhci
>>>>>>>>> specification, the transfer complete bit should be wait for BUSY
>>>>>>>>> status de-assert.
>>>>>>>>>
>>>>>>>>> All response-with-busy commands don't contain data, the data
>>>>>>>>> judgement is no need.
>>>>>>>>
>>>>>>>> I don't want to revert this commit. Is there any issue without this?
>>>>>>>> Without this patch, some SoCs have timeout error with stop command.
>>>>>>>>
>>>>>>>> To prevent it, it needs to increase timeout value at that time.
>>>>>>>> (Timeout value can't fix each boards, waste time to find proper
>>>>>>>> value, and be performance degradation.)
>>>>>>>>
>>>>>>>> I didn't test without this patch on latest U-boot.
>>>>>>>> But if there is no critical issue, keep it, plz.
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Jaehoon Chung
>>>>>>>>
>>>>>>>>> Signed-off-by: Yuezhang.Mo <Yuezhang.Mo at sony.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/mmc/sdhci.c | 3 +--
>>>>>>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index
>>>>>>>>> d9ab6a0a83..8568f65b18 100644
>>>>>>>>> --- a/drivers/mmc/sdhci.c
>>>>>>>>> +++ b/drivers/mmc/sdhci.c
>>>>>>>>> @@ -258,8 +258,7 @@ static int sdhci_send_command(struct mmc
>>>>> *mmc,
>>>>>>>> struct mmc_cmd *cmd,
>>>>>>>>>   		flags = SDHCI_CMD_RESP_LONG;
>>>>>>>>>   	else if (cmd->resp_type & MMC_RSP_BUSY) {
>>>>>>>>>   		flags = SDHCI_CMD_RESP_SHORT_BUSY;
>>>>>>>>> -		if (data)
>>>>>>>>> -			mask |= SDHCI_INT_DATA_END;
>>>>>>>>> +		mask |= SDHCI_INT_DATA_END;
>>>>>>>>>   	} else
>>>>>>>>>   		flags = SDHCI_CMD_RESP_SHORT;
>>>>>>>>>
>>>>>>>>>
>>
> 
> 
> 