Re: [PATCH v5 08/17] spi: cadence-quadspi: add PHY tuning support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware `read_delay` is neither applied during PHY tuning sweeps nor preserved after tuning succeeds.
- [High] The variable `ret` is returned uninitialized if tuning is skipped due to a missing pattern partition offset.
- [High] The `execute_tuning` callback modifies shared controller hardware registers without holding the SPI bus lock.
- [Medium] Missing clamping in dual-region PHY tuning calculation causes integer wrap-around.
- [Medium] Unconditional loop increment corrupts the successful TX delay value during RX low boundary search.
--

--- Patch [8]: [PATCH v5 08/17] spi: cadence-quadspi: add PHY tuning support ---
commit 60d05f5aa8117400f4e61a7bebf88a390815be00
Author: Santhosh Kumar K <s-k6@ti.com>

spi: cadence-quadspi: add PHY tuning support

This commit introduces delay-line PHY calibration for Cadence QSPI controllers
to enable high-speed DDR/SDR operation. It adds an execute_tuning callback
that sweeps through RX and TX delay parameters and tests them using a known
data pattern.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 046fa5d69b0f7..22a5c88bbd8dc 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -2048,15 +2048,22 @@ static int cqspi_phy_apply_setting(struct cqspi_flash_pdata *f_pdata,
>  				   struct phy_setting *phy)
>  {
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
>  	unsigned int reg;
>  	int ret;
>  
>  	reg = readl(cqspi->iobase + CQSPI_REG_READCAPTURE);
>  	reg |= BIT(CQSPI_REG_READCAPTURE_EDGE_LSB);
>  	writel(reg, cqspi->iobase + CQSPI_REG_READCAPTURE);
>  
>  	cqspi_set_dll(cqspi->iobase, phy->rx, phy->tx);

[Severity: High]
Does this execute_tuning process run without the SPI bus lock?

Since cqspi_phy_apply_setting() modifies global hardware registers directly,
and the caller subsequently invokes spi_mem_exec_op() to verify the pattern,
could a concurrent transaction on another chip select interfere and cause
register data races?

>  
>  	ret = cqspi_resync_dll(cqspi);
>  	if (ret)
>  		return ret;
>  
>  	f_pdata->phy_setting.rx = phy->rx;
>  	f_pdata->phy_setting.tx = phy->tx;
>  	f_pdata->phy_setting.read_delay = phy->read_delay;
>  	return 0;
>  }

[Severity: High]
Is the new read_delay value actually programmed into the hardware during
the tuning sweeps?

While cqspi_phy_apply_setting() updates f_pdata->phy_setting.read_delay,
it doesn't appear to write this value to the CQSPI_REG_READCAPTURE
register, meaning all reads during the sweep might still be using the
static initial read delay.

[ ... ]
> @@ -2314,6 +2314,12 @@ static int cqspi_phy_tuning_ddr(struct cqspi_flash_pdata *f_pdata,
>  	rxlow.tx = CQSPI_PHY_TX_LOOKUP_LOW_START;
>  	do {
>  		dev_dbg(dev, "Searching for Golden Primary rxlow on TX = %d\n",
>  			rxlow.tx);
>  		rxlow.read_delay = CQSPI_PHY_INIT_RD;
>  		ret = cqspi_find_rx_low_ddr(f_pdata, mem, &rxlow);
>  		rxlow.tx += CQSPI_PHY_DDR_SEARCH_STEP;
>  	} while (ret && rxlow.tx <= CQSPI_PHY_TX_LOOKUP_LOW_END);

[Severity: Medium]
Will this unconditionally increment rxlow.tx even when the pattern search
succeeds?

If ret == 0, the loop breaks, but rxlow.tx has already been incremented by
CQSPI_PHY_DDR_SEARCH_STEP. This seems to skew the successful TX boundary
by 4 taps and could corrupt the later midpoint calculation.

[ ... ]
> @@ -3037,6 +3037,10 @@ static int cqspi_phy_tuning_ddr(struct cqspi_flash_pdata *f_pdata,
>  		if ((abs(gaplow.tx - bottomleft.tx) +
>  		     abs(gaplow.rx - bottomleft.rx)) <
>  		    (abs(gaphigh.tx - topright.tx) +
>  		     abs(gaphigh.rx - topright.rx))) {
>  			/* Topright further: Use Region 2, 16 taps inward */
>  			searchpoint = topright;
>  			searchpoint.tx -= 16;
>  			searchpoint.rx -= (16 * (topright.rx - bottomleft.rx)) /
>  					  (topright.tx - bottomleft.tx);

[Severity: Medium]
Could this calculation cause an integer underflow on the 8-bit unsigned
rx and tx values?

If the resulting coordinate drops below zero, it looks like it would
wrap around and apply a very large, incorrect delay to the PHY. Should
this use clamping similar to the single-region temperature compensation
path above?

[ ... ]
> @@ -3237,6 +3237,10 @@ static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
>  			dev_warn(dev,
>  				 "failed to write pattern to cache: %d, skipping tuning\n",
>  				 ret);
>  			goto out;
>  		}
>  
>  		f_pdata->phy_write_op = *write_op;
>  	} else {
>  		if (!f_pdata->phy_pattern_partition_offset) {
>  			dev_warn(dev, "pattern partition not found, skipping tuning\n");
>  			goto out;
>  		}

[Severity: High]
Does this path return an uninitialized stack variable?

The local variable ret is declared but not initialized. If write_op
is NULL and the spi-phy-pattern-partition property is missing, taking
this goto will return whatever garbage is on the stack.

[ ... ]
> @@ -3274,6 +3274,8 @@ static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
>  		ret = cqspi_phy_tuning_sdr(f_pdata, mem);
>  	}
>  
>  	if (ret)
>  		dev_warn(dev, "tuning failed: %d\n", ret);
>  
>  	cqspi_phy_post_config(cqspi, f_pdata->read_delay);

[Severity: High]
Does this discard the tuned read delay?

cqspi_phy_post_config() is called with f_pdata->read_delay (the static
DT value) instead of f_pdata->phy_setting.read_delay. This seems to
overwrite the hardware with the untuned delay, dropping the results of
the calibration.

Also, should cqspi_configure() be updated to check use_tuned_phy and
apply f_pdata->phy_setting instead of unconditionally using
f_pdata->read_delay during context switches? Without this, the DLL
parameters might be lost on a CS switch.

> +
> +out:
> +	/*
> +	 * On success, write back the validated maximum speed into the caller's
> +	 * op templates so that those specific ops bypass the cap in subsequent
> +	 * exec_op calls.
> +	 */

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