Re: [PATCH v1 2/2] mmc: sdhci-of-dwcmshc: Set CQHCI command idle time for StarFive JHB100

[Shawn Lin <shawn.lin@rock-chips.com>]

在 2026/11/04 星期三 15:29, Adrian Hunter 写道:
> On 04/11/2026 03:40, Shawn Lin wrote:
>> 在 2026/10/31 星期六 0:45, Adrian Hunter 写道:
>>> + Shawn
>>>
>>> On 14/10/2026 15:12, Changhuang Liang wrote:
>>>> Add a cqhci_host_ops for the StarFive JHB100 platform that sets the
>>>> CQHCI command idle time (CIT) to 320 ns during pre_enable.
>>>>
>>>> Setting the command idle time improves eMMC performance on this
>>>> platform. Before/after dd results:
>>>>
>>>> before:
>>>> dd if=/dev/mmcblk0 of=/dev/null bs=1M count=800
>>>> 838860800 bytes (839 MB, 800 MiB) copied, 9.38511 s, 89.4 MB/s
>>>> dd if=/dev/zero of=/dev/mmcblk0 bs=1M count=800
>>>> 838860800 bytes (839 MB, 800 MiB) copied, 12.2127 s, 68.7 MB/s
>>>>
>>>> after:
>>>> dd if=/dev/mmcblk0 of=/dev/null bs=1M count=800
>>>> 838860800 bytes (839 MB, 800 MiB) copied, 3.19472 s, 263 MB/s
>>>> dd if=/dev/zero of=/dev/mmcblk0 bs=1M count=800
>>>> 838860800 bytes (839 MB, 800 MiB) copied, 10.7587 s, 78.0 MB/s
>>>>
>>>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci-of-dwcmshc.c | 22 ++++++++++++++++++++++
>>>>    1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> index 249055f14162..36628b44f78b 100644
>>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> @@ -283,6 +283,8 @@
>>>>    #define PHY_DELAY_CODE_SD        0x55
>>>>    #define PHY_DELAY_CODE_SDIO        0x29
>>>>    +/* CIT for the JHB100 SoC */
>>>> +#define JHB100_CQHCI_CIT_NS    320
>>>>    struct rk35xx_priv {
>>>>        struct reset_control *reset;
>>>>        u8 txclk_tapnum;
>>>> @@ -2340,6 +2342,25 @@ static const struct sdhci_ops starfive_jhb100_ops = {
>>>>        .platform_execute_tuning = th1520_execute_tuning,
>>>>    };
>>>>    +static void dwcmshc_starfive_jhb100_pre_enable(struct mmc_host *mmc)
>>>> +{
>>>> +    struct cqhci_host *cq_host = mmc->cqe_private;
>>>> +    int ret;
>>>> +
>>>> +    ret = cqhci_set_cmd_idle_time(cq_host, JHB100_CQHCI_CIT_NS);
>>>
>>> Currently doing this from ->pre_enable() leaves a gap.  It needs:
>>>
>>> diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
>>> index a466cd88b043..e1bf6d50e8eb 100644
>>> --- a/drivers/mmc/host/cqhci-core.c
>>> +++ b/drivers/mmc/host/cqhci-core.c
>>> @@ -283,6 +283,9 @@ static void __cqhci_enable(struct cqhci_host *cq_host)
>>>          cqhci_set_irqs(cq_host, 0);
>>>    +    if (cq_host->ops->pre_enable)
>>> +        cq_host->ops->pre_enable(mmc);
>>> +
>>>        cqcfg |= CQHCI_ENABLE;
>>>          cqhci_writel(cq_host, cqcfg, CQHCI_CFG);
>>>
>>> which is probably:
>>>
>>> Fixes: 79cf71c0b177 ("mmc: sdhci-of-dwcmshc: reduce CIT for better performance")
>>>
>>> Shawn?
>>>
>>
>> The window between CQHCI_ENABLE and the first request has no queued
>> commands for the idle timer to apply to, and pre_enable() re-runs on
>> every cqe_off -> cqe_on transition (recovery, resume), so the value gets
>> rewritten even if the registers were reset in between.
> 
> Might be worth checking.
> 
> At card init, cqhci_enable() calls __cqhci_enable(), which sets mmc->cqe_on =
> true (cqhci-core.c) and never calls ->pre_enable(). cqhci_request() only calls
> ->pre_enable() when !mmc->cqe_on (cqhci-core.c). So the first request skips it,
> and so do all the requests after it. CIT is first written only after something
> runs a non-CQE command, because mmc_start_request() then calls cqe_off()
> (core.c). Examples: a CMD6 partition switch, a discard, an ioctl, or a flush
> without DCMD. Until that happens, real CQE I/O runs with the default CIT.
> 

Indeed. If Changhuang is going to configure cit in __cqhci_enable, I
could prepare a patch on top to improve it for rockchip platforms.

> For Resume: cqhci_suspend() → cqhci_deactivate() → __cqhci_disable() clears both
> cqe_on and activated. The first request after resume takes the !activated path
> into __cqhci_enable(), which sets cqe_on = true again. The !cqe_on branch is
> then skipped and ->pre_enable() is not called.
> 
> RESET_ALL: same problem. rk35xx_sdhci_reset() on SDHCI_RESET_ALL calls
> cqhci_deactivate() and also asserts the reset_control, so SSC1 is certainly back
> to its default. CIT then stays at the default after resume until the next
> cqe_off.
> 
>