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

[Shawn Lin]

在 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@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.
>
> 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.