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

[Shawn Lin]

在 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@xxxxxxxxxxxxxxxx>
> > ---
> >   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,
> >   };