From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaehoon Chung Date: Tue, 6 Apr 2021 20:13:14 +0900 Subject: [PATCH] Revert "mmc: sdhci: set to INT_DATA_END when there are data" In-Reply-To: References: <1891546521.01616028481381.JavaMail.epsvc@epcpadp4> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> Sent: Monday, March 22, 2021 6:03 PM >>> To: Wu, Andy ; jh80.chung at samsung.com; Mo, >> Yuezhang >>> ; 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 On Behalf Of Jaehoon >>>>> Chung >>>>> Sent: Thursday, March 18, 2021 6:44 AM >>>>> To: Mo, Yuezhang ; 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 >>>>>> --- >>>>>> 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; >>>>>> >>>>>>