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

You are right that __cqhci_enable() does not touch CQHCI_SSC1. It only
programs CQHCI_CFG, CQHCI_TDLBA/TDLBAU, CQHCI_SSC2 and CQHCI_CTL, so
today the CIT write lands just before the first request and the default
value in the idle window is harmless.

I agree the value belongs in struct cqhci_host and should be programmed
from __cqhci_enable() when it is non-zero, as you suggested.

> 
> Going forward, it would probably be simpler to put 'cit' in
> struct cqhci_host and write it, if it is not 0, in __cqhci_enable()
> e.g.
> 
> diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
> index a466cd88b043..d5f8b427e1f8 100644
> --- a/drivers/mmc/host/cqhci-core.c
> +++ b/drivers/mmc/host/cqhci-core.c
> @@ -279,6 +279,14 @@ static void __cqhci_enable(struct cqhci_host *cq_host)
>   	cqhci_writel(cq_host, upper_32_bits(cq_host->desc_dma_base),
>   		     CQHCI_TDLBAU);
>   
> +	if (cq_host->cit) {
> +		u32 ssc1 = cqhci_readl(cq_host, CQHCI_SSC1);
> +
> +		ssc1 &= ~CQHCI_SSC1_CIT_MASK;
> +		ssc1 |= FIELD_PREP(CQHCI_SSC1_CIT_MASK, cq_host->cit);
> +		cqhci_writel(cq_host, ssc1, CQHCI_SSC1);
> +	}
> +
>   	cqhci_writel(cq_host, cq_host->rca, CQHCI_SSC2);
>   
>   	cqhci_set_irqs(cq_host, 0);
> 
>> +	if (ret)
>> +		dev_warn_once(mmc_dev(mmc),
>> +			      "failed to set CQE command idle time: %d\n", ret);
>> +}
>> +
>> +static const struct cqhci_host_ops starfive_jhb100_cqhci_ops = {
>> +	.enable			= dwcmshc_sdhci_cqe_enable,
>> +	.disable		= sdhci_cqe_disable,
>> +	.pre_enable		= dwcmshc_starfive_jhb100_pre_enable,
>> +	.dumpregs		= dwcmshc_cqhci_dumpregs,
>> +	.set_tran_desc		= dwcmshc_set_tran_desc,
>> +};
>> +
>>   static int dwcmshc_starfive_jhb100_init(struct device *dev, struct sdhci_host *host,
>>   					struct dwcmshc_priv *dwc_priv)
>>   {
>> @@ -2412,6 +2433,7 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_jhb100_pdata = {
>>   		.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>>   		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>   	},
>> +	.cqhci_host_ops = &starfive_jhb100_cqhci_ops,
>>   	.init = dwcmshc_starfive_jhb100_init,
>>   	.deinit = dwcmshc_starfive_jhb100_deinit,
>>   };
> 
>