Re: [PATCH v11 11/19] spi: cadence-quadspi: enable PHY for direct reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi: cadence-quadspi: state leak on disable timeout in cqspi_tune_phy_read()
- [High] spi: cadence-quadspi: silent base-clock fallback during PHY tuning in cqspi_read()
- [High] spi: cadence-quadspi: missing READCAPTURE_EDGE bit in cqspi_tune_phy_read()
--

commit feb139507791ba27ec4dfd5612d622bfad619500
Author: Santhosh Kumar K <s-k6@ti.com>

spi: cadence-quadspi: enable PHY for direct reads

This commit enables PHY mode around direct-mode DMA reads when PHY tuning has
succeeded and the operation requests the post-config clock rate. It also adds
fallback logic to drop the clock to the base rate when a read cannot use the
tuned PHY.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index c324451a9a285..55c446f4245af 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -1075,6 +1076,105 @@ static int cqspi_resync_dll(struct cqspi_st *cqspi)

[ ... ]

> +static int cqspi_tune_phy_read(struct cqspi_flash_pdata *f_pdata, bool enable)
> +{
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	void __iomem *reg_base = cqspi->iobase;
> +	u32 reg;
> +	u8 dummy;
> +	int ret;
> +
> +	/* Wait for idle before touching CONFIG/RD_INSTR either direction. */
> +	ret = cqspi_wait_idle(cqspi);
> +	if (ret)
> +		return ret;
> +
> +	if (!enable) {
> +		cqspi_phy_restore_read_config(f_pdata);
> +		return 0;
> +	}

[Severity: High]
As reported by Sashiko on v10 [1], when cqspi_direct_read_execute() calls
cqspi_tune_phy_read(f_pdata, false) after cqspi_direct_read_dma() and
cqspi_wait_idle() times out, returning early before the !enable check skips
cqspi_phy_restore_read_config().

Doesn't this leave CQSPI_REG_CONFIG_PHY_EN and CQSPI_REG_CONFIG_PHY_PIPELINE
asserted in CQSPI_REG_CONFIG, CQSPI_REG_READCAPTURE programmed with the PHY
capture settings, and the decremented dummy cycle count in CQSPI_REG_RD_INSTR
for subsequent untuned transfers?

> +
> +	ret = cqspi_phy_reprogram_dll(f_pdata);
> +	if (ret)
> +		return ret;
> +
> +	cqspi_readdata_capture(cqspi, !f_pdata->use_dqs, f_pdata->use_dqs,
> +			       f_pdata->phy_setting.read_delay);

[Severity: High]
During PHY calibration, cqspi_phy_apply_setting() sets
BIT(CQSPI_REG_READCAPTURE_EDGE_LSB) in CQSPI_REG_READCAPTURE before each
pattern read:

drivers/spi/spi-cadence-quadspi.c:cqspi_phy_apply_setting() {
	reg = readl(cqspi->iobase + CQSPI_REG_READCAPTURE);
	reg |= BIT(CQSPI_REG_READCAPTURE_EDGE_LSB);
	writel(reg, cqspi->iobase + CQSPI_REG_READCAPTURE);
	...
}

When calibration finishes, cqspi_phy_post_config() clears that bit to restore
the non-PHY state:

drivers/spi/spi-cadence-quadspi.c:cqspi_phy_post_config() {
	...
	reg = readl(reg_base + CQSPI_REG_READCAPTURE);
	reg &= ~BIT(CQSPI_REG_READCAPTURE_EDGE_LSB);
	writel(reg, reg_base + CQSPI_REG_READCAPTURE);
	...
}

However, cqspi_tune_phy_read() and cqspi_readdata_capture() never set or
restore BIT(CQSPI_REG_READCAPTURE_EDGE_LSB). Won't all runtime PHY direct
reads execute with CQSPI_REG_READCAPTURE_EDGE_LSB cleared, even though the
PHY delay settings were calibrated with that bit set?

[ ... ]

> @@ -1622,26 +1730,66 @@ static void cqspi_memcpy_fromio(const struct spi_mem_op *op, void *to,
>  	memcpy_fromio(to, from, count);
>  }
>  
> +static int cqspi_direct_read_fallback(struct cqspi_flash_pdata *f_pdata,
> +				      const struct spi_mem_op *op,
> +				      unsigned long max_speed_hz,
> +				      u32 post_config_max_speed_hz)
> +{
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	loff_t from = op->addr.val;
> +	size_t len = op->data.nbytes;
> +	u_char *buf = op->data.buf.in;
> +	unsigned long configured_sclk = cqspi->sclk;
> +	bool reclock = op->max_freq == post_config_max_speed_hz &&
> +		       configured_sclk != max_speed_hz;
> +	int ret = 0;
> +
> +	/* Drop to the safe rate for this untuned transfer; PHY isn't used. */
> +	if (reclock)
> +		cqspi_configure(f_pdata, max_speed_hz);
> +
> +	if (!cqspi->rx_chan || !virt_addr_valid(buf) ||
> +	    len < CQSPI_PHY_MIN_DIRECT_READ_LEN)
> +		cqspi_memcpy_fromio(op, buf, cqspi->ahb_base + from, len);
> +	else
> +		ret = cqspi_direct_read_dma(f_pdata, buf, from, len);
> +
> +	if (reclock)
> +		cqspi_configure(f_pdata, configured_sclk);
> +
> +	return ret;
> +}
> +
>  static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>  				     const struct spi_mem_op *op,
> +				     unsigned long max_speed_hz,
>  				     u32 post_config_max_speed_hz)
>  {
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
>  	loff_t from = op->addr.val;
>  	size_t len = op->data.nbytes;
>  	u_char *buf = op->data.buf.in;
> +	int ret;
>  
> -	if (!cqspi->rx_chan || !virt_addr_valid(buf) ||
> -	    len < CQSPI_PHY_MIN_DIRECT_READ_LEN) {
> -		cqspi_memcpy_fromio(op, buf, cqspi->ahb_base + from, len);
> -		return 0;
> -	}
> +	if (!cqspi_use_tuned_phy(f_pdata, op, post_config_max_speed_hz) ||
> +	    !cqspi->rx_chan || !virt_addr_valid(buf) ||
> +	    len < CQSPI_PHY_MIN_DIRECT_READ_LEN ||
> +	    !IS_ALIGNED(from, 16) || !IS_ALIGNED(from + len, 16))
> +		return cqspi_direct_read_fallback(f_pdata, op, max_speed_hz,
> +						  post_config_max_speed_hz);

[Severity: High]
During PHY calibration via cqspi_am654_ospi_execute_tuning(), pattern
verification reads go through cqspi_phy_check_pattern() -> cqspi_exec_mem_op()
-> cqspi_mem_process() -> cqspi_read() -> cqspi_direct_read_execute().

If DMA is not available (!cqspi->rx_chan), direct mode is disabled
(!cqspi->use_direct_mode), or f_pdata->phy_read_op.addr.val is not 16-byte
aligned or exceeds cqspi->ahb_size, won't every calibration read silently fall
back to cqspi_direct_read_fallback() (or the indirect read path in
cqspi_read()) and drop the clock from post_config_max_speed_hz to
max_speed_hz without running cqspi_tune_phy_read()?

That would evaluate the entire PHY delay sweep at the slow base clock rate,
falsely report tuning success, and promote read_op->max_freq and
write_op->max_freq to post_config_max_speed_hz with uncalibrated tap settings.
Should cqspi_am654_ospi_execute_tuning() verify that direct DMA reads can be
used for the calibration pattern before starting tuning, or should fallback
reads be rejected while cqspi->phy_tuning_active is set?

[1] https://lore.kernel.org/r/20260929063529.1235417-12-s-k6@ti.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105105832.2134313-1-s-k6@ti.com?part=11