Re: [PATCH v4 08/16] spi: cadence-quadspi: add PHY tuning support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unchecked dereference of `cqspi->ddata` in `cqspi_mem_op_execute_tuning` when it can be NULL.
- [High] Passing `.rodata` buffer to `spi_mem_exec_op()` violates SPI API DMA constraints.
- [High] Dual-region PHY tuning arithmetic lacks bounds clamping, causing `u8` underflows.
- [High] The tuning loops sweep `read_delay`, but `cqspi_phy_apply_setting` fails to update the hardware register with the new delay.
- [High] PHY tuning modifies shared SPI controller registers without holding the SPI bus lock, creating a race condition with concurrent transfers.
- [High] The PHY tuning sequence performs direct MMIO to controller registers without acquiring a PM runtime reference.
- [Medium] Missing error check in SDR tuning causes invalid read_delay configuration.
- [Medium] The PHY tuning search loops ignore critical hardware timeouts and memory allocation errors, causing the system to needlessly stall.
--

commit a0e116638901b220157d7f4a14b288dc44e8d0fd
Author: Santhosh Kumar K <s-k6@ti.com>
Subject: spi: cadence-quadspi: add PHY tuning support

This commit adds an execute_tuning callback for delay-line calibration on
Cadence QSPI controllers. It implements tuning sweeps for both DDR and SDR
modes. The calibration relies on writing and reading a predefined pattern
to and from flash memory.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index f4efb92c9446a..05e1b7a2f3433 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1559,6 +1640,1678 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
> +static int cqspi_write_pattern_to_cache(struct cqspi_flash_pdata *f_pdata,
> +					struct spi_mem *mem,
> +					struct spi_mem_op *write_op)
> +{
> +	struct device *dev = &f_pdata->cqspi->pdev->dev;
> +	int ret;
> +
> +	write_op->data.nbytes = sizeof(phy_tuning_pattern);
> +	write_op->data.buf.out = phy_tuning_pattern;
> +
> +	ret = spi_mem_exec_op(mem, write_op);

[Severity: High]
Does passing phy_tuning_pattern directly to spi_mem_exec_op violate SPI DMA
constraints? The phy_tuning_pattern array is defined as static const u8 and
resides in the rodata section.

The SPI framework mandates that buffers passed to the SPI core must be
DMA-capable, as fallback paths will attempt to call dma_map_single on them.
Mapping a rodata buffer violates the DMA API and will trigger kernel panics
or warnings when CONFIG_DMA_API_DEBUG is enabled. Could this be copied to a
dynamically allocated bounce buffer first?

[ ... ]
> +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);
> +
> +	ret = cqspi_resync_dll(cqspi);
> +	if (ret)
> +		return ret;
> +
> +	f_pdata->phy_setting.read_delay = phy->read_delay;
> +	return 0;
> +}

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

While cqspi_phy_apply_setting updates the DLL and edge registers, it appears
it never writes the phy->read_delay into the DELAY_LSB field of the
CQSPI_REG_READCAPTURE register. 

Because the internal configuration updates are bypassed when the SPI clock
frequency has not changed, doesn't this mean the hardware read_delay remains
fixed at its initial value throughout the entire tuning sequence?

[ ... ]
> +static int cqspi_find_rx_low_ddr(struct cqspi_flash_pdata *f_pdata,
> +				 struct spi_mem *mem, struct phy_setting *phy)
> +{
> +	struct device *dev = &f_pdata->cqspi->pdev->dev;
> +	int ret;
> +
> +	do {
> +		phy->rx = CQSPI_PHY_RX_LOW_SEARCH_START;
> +		do {
> +			ret = cqspi_phy_apply_setting(f_pdata, phy);
> +			if (!ret) {
> +				ret = cqspi_phy_check_pattern(f_pdata, mem);
> +				if (!ret)
> +					return 0;
> +			}
> +
> +			phy->rx += CQSPI_PHY_DDR_SEARCH_STEP;
> +		} while (phy->rx <= CQSPI_PHY_RX_LOW_SEARCH_END);

[Severity: Medium]
Will this loop stall the boot process needlessly if a fatal error occurs?

If cqspi_phy_apply_setting returns -ETIMEDOUT because the DLL failed to
lock, or cqspi_phy_check_pattern returns -ENOMEM due to an allocation error,
the loop checks if (!ret) and then blindly continues to the next coordinate.

Since these operations wait up to 600us on each iteration, ignoring these
environmental errors instead of failing fast seems to cause a significant
stall. Should we check for specific fatal error codes and abort?

[ ... ]
> +		/* Compare Manhattan distances: choose corner furthest from gap */
> +		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: High]
Can this proportional adjustment underflow the u8 searchpoint.rx variable?

When resolving the dual-region tuning target, if the calculated adjustment
exceeds topright.rx (e.g. if topright.rx is 24 and the adjustment evaluates
to 27), the u8 variable will severely underflow and wrap to 253.

This wrapped value is then passed into cqspi_set_dll where the 0x7F mask
would truncate it to an arbitrary out-of-bounds point, corrupting the
calibration. Should there be a clamp applied here?

[ ... ]
> +static int cqspi_phy_tuning_sdr(struct cqspi_flash_pdata *f_pdata,
> +				struct spi_mem *mem)
> +{
[ ... ]
> +	do {
> +		ret = cqspi_find_rx_low_sdr(f_pdata, mem, &rxlow);
> +
> +		if (ret)
> +			rxlow.read_delay++;
> +	} while (ret && rxlow.read_delay <= CQSPI_PHY_MAX_RD);
> +
> +	/* Find rxhigh: Decrement from RX=127 at same read_delay */
> +
[ ... ]
> +
> +	rxhigh.read_delay = rxlow.read_delay;
> +	ret = cqspi_find_rx_high_sdr(f_pdata, mem, &rxhigh, rxlow.rx);

[Severity: Medium]
Is there a missing error check after the first search loop?

Unlike the DDR implementation, if the loop exceeds CQSPI_PHY_MAX_RD without
finding rxlow, it exits with a non-zero ret. However, the code proceeds
directly to searching rxhigh with an out-of-bounds rxlow.read_delay (5) and
an invalid rxlow.rx (128). This causes out-of-bounds values to be applied to
the hardware configuration. Could we add an if (ret) goto out; check here?

[ ... ]
> +static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
> +					   struct spi_mem_op *read_op,
> +					   struct spi_mem_op *write_op)
> +{
[ ... ]
> +	if (read_op->cmd.dtr || read_op->addr.dtr || read_op->dummy.dtr ||
> +	    read_op->data.dtr) {
> +		f_pdata->use_dqs = true;
> +		cqspi_phy_pre_config(cqspi, f_pdata, false);
> +		ret = cqspi_phy_tuning_ddr(f_pdata, mem);
> +	} else {
> +		f_pdata->use_dqs = false;
> +		cqspi_phy_pre_config(cqspi, f_pdata, true);
> +		ret = cqspi_phy_tuning_sdr(f_pdata, mem);
> +	}

[Severity: High]
Are we adequately protecting concurrent transfers and power management state
during these tuning calls?

The spi_mem_execute_tuning framework API does not acquire the SPI
controller's bus_lock_mutex. Consequently, this callback runs lock-free. It
performs raw read-modify-write operations on shared controller registers
like CQSPI_REG_CONFIG and unconditionally disables the controller via
cqspi_resync_dll(). If another SPI device on the same bus initiates a
concurrent transfer, its transfer could be aborted or corrupted by these
unguarded register writes.

[Severity: High]
Additionally, the tuning sequence accesses the controller's MMIO registers
directly but does not acquire an active PM runtime reference with
pm_runtime_resume_and_get(). If the SPI controller is suspended by
auto-suspend immediately after an inner spi_mem_exec_op returns, subsequent
manual MMIO accesses might hit unclocked hardware and trigger synchronous
external aborts.

[ ... ]
> +static int cqspi_mem_op_execute_tuning(struct spi_mem *mem,
> +				       struct spi_mem_op *read_op,
> +				       struct spi_mem_op *write_op)
> +{
> +	struct cqspi_st *cqspi =
> +		spi_controller_get_devdata(mem->spi->controller);
> +
> +	if (!cqspi->ddata->execute_tuning)
> +		return -EOPNOTSUPP;
> +
> +	return cqspi->ddata->execute_tuning(mem, read_op, write_op);
> +}

[Severity: High]
Can cqspi->ddata be NULL here?

The execute_tuning callback is unconditionally added to cqspi_mem_ops. In
this driver, cqspi->ddata is populated via of_device_get_match_data(), which
can return NULL for generic fallbacks or manual device instantiation. If
ddata is NULL, dereferencing cqspi->ddata->execute_tuning will trigger a
kernel panic when the SPI core attempts to execute tuning.

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