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

[Andy.Wu at sony.com]

Hi All

Since patch also reviewed by Jaehoon, how about merge it into mainline?

Best Regards
Andy Wu

> -----Original Message-----
> From: Jaehoon Chung <jh80.chung at samsung.com>
> Sent: Thursday, May 20, 2021 6:03 AM
> To: Wu, Andy <Andy.Wu at sony.com>; peng.fan at nxp.com;
> jh80.chung at gmail.com; u-boot at lists.denx.de
> Cc: CPGS <cpgs at samsung.com>
> Subject: Re: [PATCH] Revert "mmc: sdhci: set to INT_DATA_END when there are
> data"
> 
> 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;
> >>>>>>>>>
> >>>>>>>>>
> >>
> >
> >
> >
>