Re: [PATCH v2 1/2] mmc: cqhci: Add support for setting command idle time in nanoseconds

[Adrian Hunter <adrian.hunter@intel.com>]

On 05/11/2026 12:04, Changhuang Liang wrote:
> Hi, Adrian
> 
> Thanks for the review.
> 
>> On 04/11/2026 13:26, Changhuang Liang wrote:
>>> Add a helper that derives the internal timer clock frequency (ITCF) of
>>> the CQE from the ITCFMUL and ITCFVAL fields of the CQHCI capabilities
>>> register, and export cqhci_set_cmd_idle_time() to let host drivers
>>> program the Send Status Command Idle Timer (CIT) in CQHCI_SSC1 using a
>>> value in nanoseconds.
>>>
>>> The CIT defines how long the bus has to be idle, in internal timer
>>> clock cycles, before the CQE issues CMD13 to poll the Queue Status
>> Register.
>>> Its reset value is controller specific and can be far from optimal, so
>>> host drivers may need to tune it for their platform. Doing the
>>> conversion in the core avoids every host driver re-implementing the
>>> ITCFMUL/ITCFVAL decoding.
>>
>> Should probably have asked this before, but why calculate it at runtime?  If
>> you know the hardware, then you already know what the value is going to be?
>>
> 
> For convenient unified use later, different SoCs may have different ITCFMUL and
> ITCFVAL values, which could lead to different platforms writing different CIT values 
> to CQHCI_SSC1—even though the CIT expressed in nanoseconds may be the same. 
> Once this has been validated on more platforms, it may be possible to unify on a 
> nanosecond-based CIT and thereby get rid of the .cqhci_cit_ns field in patch2.
> 
> Best Regards,
> Changhuang
> 
>>>
>>> The requested time is rounded up to whole clock cycles so that the
>>> programmed idle time is never shorter than requested. The helper
>>> returns -EOPNOTSUPP when the controller does not report a usable ITCF
>>> and -ERANGE when the result does not fit the 16-bit CIT field, so
>>> callers can report the failure instead of silently running with the
>>> reset default.
>>>
>>> Record the cit value in cqhci_set_cmd_idle_time(), and if cit is
>>> non-zero, program it into the register on every call to __cqhci_enable().
>>>
>>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>>> ---
>>>  drivers/mmc/host/cqhci-core.c | 64
>> +++++++++++++++++++++++++++++++++++
>>>  drivers/mmc/host/cqhci.h      |  8 +++++
>>>  2 files changed, 72 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/cqhci-core.c
>>> b/drivers/mmc/host/cqhci-core.c index a466cd88b043..bfa5d8b79145
>>> 100644
>>> --- a/drivers/mmc/host/cqhci-core.c
>>> +++ b/drivers/mmc/host/cqhci-core.c
>>> @@ -12,6 +12,7 @@
>>>  #include <linux/scatterlist.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/ktime.h>
>>> +#include <linux/units.h>
>>>
>>>  #include <linux/mmc/mmc.h>
>>>  #include <linux/mmc/host.h>
>>> @@ -279,6 +280,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);
>>> @@ -1163,6 +1172,59 @@ struct cqhci_host *cqhci_pltfm_init(struct
>>> platform_device *pdev)  }  EXPORT_SYMBOL(cqhci_pltfm_init);
>>>
>>> +static u64 cqhci_itcfmul_to_hz(u32 itcfmul) {
>>> +	/* CQHCI_CAP.ITCFMUL: 0 = 1 kHz, 1 = 10 kHz, ..., 4 = 10 MHz, others
>> reserved */
>>> +	if (itcfmul > 4)
>>> +		return 0;
>>> +
>>> +	return HZ_PER_KHZ * int_pow(10, itcfmul); }
>>> +
>>> +static void cqhci_itcf_init(struct cqhci_host *cq_host) {
>>> +	u32 cap = cqhci_readl(cq_host, CQHCI_CAP);
>>> +	u64 itcf;
>>> +
>>> +	itcf = cqhci_itcfmul_to_hz(CQHCI_ITCFMUL(cap)) * CQHCI_ITCFVAL(cap);
>>> +	if (!itcf || itcf > U32_MAX) {
>>> +		pr_debug("%s: cqhci: internal timer clock frequency not available
>> (CAP 0x%08x)\n",
>>> +			 mmc_hostname(cq_host->mmc), cap);
>>> +		return;
>>> +	}
>>> +
>>> +	cq_host->itcf_hz = itcf;
>>> +}
>>> +
>>> +/**
>>> + * cqhci_set_cmd_idle_time - program the Send Status Command Idle
>>> +Timer (CIT)
>>> + * @cq_host: CQHCI host
>>> + * @ns: idle time in nanoseconds
>>> + *
>>> + * Set the number of internal timer clock cycles the CQE waits on an
>>> +idle bus
>>> + * before issuing CMD13 to poll the Queue Status Register. The cycle
>>> +count is
>>> + * rounded up so that the resulting idle time is never shorter than @ns.
>>> + *
>>> + * Return: 0 on success, -EOPNOTSUPP if the controller does not
>>> +report its
>>> + * internal timer clock frequency, or -ERANGE if @ns does not fit the CIT
>> field.
>>> + */
>>> +int cqhci_set_cmd_idle_time(struct cqhci_host *cq_host, u32 ns) {

Let's just add cit_ns (and cit) to struct cqhci_host and then
calculate cit during init if cit_ns != 0.  Users can just set
cit_ns or cit directly


>>> +	u64 cit;
>>> +
>>> +	if (!cq_host->itcf_hz)
>>> +		return -EOPNOTSUPP;

This can be pr_warn(), don't return an error.  Users are expected to know
their hardware, and it is not fatal.

>>> +
>>> +	cit = DIV_ROUND_UP_ULL((u64)cq_host->itcf_hz * ns, NSEC_PER_SEC);
>>> +	if (!cit || cit > FIELD_MAX(CQHCI_SSC1_CIT_MASK))
>>> +		return -ERANGE;

Ditto.

>>> +
>>> +	cq_host->cit = cit;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(cqhci_set_cmd_idle_time);
>>> +
>>>  static unsigned int cqhci_ver_major(struct cqhci_host *cq_host)  {
>>>  	return CQHCI_VER_MAJOR(cqhci_readl(cq_host, CQHCI_VER)); @@
>> -1207,6
>>> +1269,8 @@ int cqhci_init(struct cqhci_host *cq_host, struct mmc_host
>> *mmc,
>>>  		goto out_err;
>>>  	}
>>>
>>> +	cqhci_itcf_init(cq_host);

Calculate cit from cit_ns if cit_ns != 0.  No need to store itcf_hz.

>>> +
>>>  	spin_lock_init(&cq_host->lock);
>>>
>>>  	init_completion(&cq_host->halt_comp);
>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h index
>>> 8fbbc48c3f85..e6840a8f4798 100644
>>> --- a/drivers/mmc/host/cqhci.h
>>> +++ b/drivers/mmc/host/cqhci.h
>>> @@ -26,6 +26,8 @@
>>>  #define CQHCI_CAP_CS			0x10000000 /* Crypto Support */
>>>  #define CQHCI_CAP_ITCFMUL		GENMASK(15, 12)
>>>  #define CQHCI_ITCFMUL(x)		FIELD_GET(CQHCI_CAP_ITCFMUL, (x))
>>> +#define CQHCI_CAP_ITCFVAL		GENMASK(9, 0)
>>> +#define CQHCI_ITCFVAL(x)		FIELD_GET(CQHCI_CAP_ITCFVAL, (x))
>>>
>>>  /* configuration */
>>>  #define CQHCI_CFG			0x08
>>> @@ -234,6 +236,11 @@ struct cqhci_host {
>>>  	int num_slots;
>>>  	int qcnt;
>>>
>>> +	/* Internal Timer Clock Frequency (Hz), 0 if not reported by CQHCI_CAP
>> */
>>> +	u32 itcf_hz;
>>> +	/* Send Status Command Idle Timer in ITCF cycles, 0 keeps the default */
>>> +	u16 cit;
>>> +
>>>  	u32 dcmd_slot;
>>>  	u32 caps;
>>>  #define CQHCI_TASK_DESC_SZ_128		0x1
>>> @@ -318,6 +325,7 @@ struct platform_device;  irqreturn_t
>>> cqhci_irq(struct mmc_host *mmc, int cmd_error, int data_error);  int
>>> cqhci_init(struct cqhci_host *cq_host, struct mmc_host *mmc, bool
>>> dma64);  struct cqhci_host *cqhci_pltfm_init(struct platform_device
>>> *pdev);
>>> +int cqhci_set_cmd_idle_time(struct cqhci_host *cq_host, u32 ns);
>>>  int cqhci_deactivate(struct mmc_host *mmc);  void
>>> cqhci_set_tran_desc(u8 *desc, dma_addr_t addr, int len, bool end, bool
>>> dma64);  static inline int cqhci_suspend(struct mmc_host *mmc)
>