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

[Jaehoon Chung]

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.)

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;
>>>>>>>>
>>>>>>>>
>