Re: [PATCH v11 08/19] spi: cadence-quadspi: add PHY tuning support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] spi: cadence-quadspi: u8 underflow and infinite loop in cqspi_find_rx_high_sdr()
- [High] spi: cadence-quadspi: unclocked MMIO access in cqspi_am654_ospi_execute_tuning()
- [Medium] spi: cadence-quadspi: broken RX bounds and gap search in cqspi_phy_tuning_ddr()
- [Medium] spi: cadence-quadspi: propagate -ENOMEM from cqspi_phy_check_pattern()
--

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

spi: cadence-quadspi: add PHY tuning support

This commit adds delay-line PHY calibration support for AM654-based Cadence
OSPI controllers via the spi_controller_mem_ops execute_tuning callback. It
implements a 2D RX/TX delay sweep for DDR protocols and a 1D RX delay sweep
for SDR protocols using a known tuning pattern.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 4be37081d066c..b0f01621b323c 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1558,10 +1717,1572 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
[ ... ]
> +static int cqspi_phy_check_pattern(struct cqspi_flash_pdata *f_pdata,
> +				   struct spi_mem *mem)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	struct spi_mem_op op;
> +	u8 *read_data;
> +	int ret;
> +
> +	read_data = kmalloc_array(ARRAY_SIZE(phy_tuning_pattern),
> +				  sizeof(phy_tuning_pattern[0]), GFP_KERNEL);
> +	if (!read_data)
> +		return -ENOMEM;
> +
> +	op = f_pdata->phy_read_op;
> +	op.data.buf.in = read_data;
> +	op.data.nbytes = sizeof(phy_tuning_pattern);
> +
> +	ret = ctlr->mem_ops->exec_op(mem, &op);
> +	if (ret)
> +		goto out;
> +
> +	if (memcmp(read_data, phy_tuning_pattern, sizeof(phy_tuning_pattern)))
> +		ret = -EAGAIN;
> +
> +out:
> +	kfree(read_data);
> +	return ret;
> +}
[ ... ]
> +static int cqspi_find_rx_low_ddr(struct cqspi_flash_pdata *f_pdata,
> +				 struct spi_mem *mem, struct cqspi_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);
> +
> +		phy->read_delay++;
> +	} while (phy->read_delay <= CQSPI_PHY_MAX_RD);
> +
> +	dev_dbg(dev, "Unable to find RX low\n");
> +	return -ENOENT;
> +}

[Severity: Medium]
As reported by Sashiko on v10 [1], in drivers/spi/spi-cadence-quadspi.c,
cqspi_phy_check_pattern() allocates read_data with kmalloc_array() on every
call and returns -ENOMEM on allocation failure.

However, the sweep helpers (cqspi_find_rx_low_ddr(), cqspi_find_rx_low_sdr(),
cqspi_find_rx_high_ddr(), cqspi_find_rx_high_sdr(), cqspi_find_tx_low_ddr(),
and cqspi_find_tx_high_ddr()) only check if (!ret) and ignore the specific
error code, continuing to sweep across all remaining delay taps and ultimately
returning -ENOENT instead of -ENOMEM.

Similarly, the void binary-search helpers cqspi_phy_find_gaplow_ddr() and
cqspi_phy_find_gaphigh_ddr() treat any non-zero return from
cqspi_phy_check_pattern() as a pattern mismatch and bisect the search window
in the wrong direction.

Should -ENOMEM be propagated immediately, or could the 128-byte buffer be
allocated once before the tuning sweep?

[ ... ]
> +static int cqspi_find_rx_high_sdr(struct cqspi_flash_pdata *f_pdata,
> +				  struct spi_mem *mem, struct cqspi_phy_setting *phy,
> +				  u8 lowerbound)
> +{
> +	struct device *dev = &f_pdata->cqspi->pdev->dev;
> +	int ret;
> +
> +	phy->rx = CQSPI_PHY_MAX_DELAY;
> +	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--;
> +	} while (phy->rx >= lowerbound);
> +
> +	dev_dbg(dev, "Unable to find RX high\n");
> +	return -ENOENT;
> +}

[Severity: High]
As reported by Sashiko on v10 [2], can phy->rx underflow and cause an infinite
loop in cqspi_find_rx_high_sdr() in drivers/spi/spi-cadence-quadspi.c?

In struct cqspi_phy_setting, rx is a u8, and the lowerbound parameter of
cqspi_find_rx_high_sdr() is also a u8. During SDR tuning,
cqspi_phy_tuning_sdr() first calls cqspi_find_rx_low_sdr(), which scans upward
starting from phy->rx = 0, and then passes rxlow.rx as lowerbound to
cqspi_find_rx_high_sdr():

	rxhigh.read_delay = rxlow.read_delay;
	ret = cqspi_find_rx_high_sdr(f_pdata, mem, &rxhigh, rxlow.rx);

Whenever tap 0 passes the pattern check in cqspi_find_rx_low_sdr(), rxlow.rx
is 0, so lowerbound is 0. If cqspi_phy_apply_setting() or
cqspi_phy_check_pattern() subsequently fails down through phy->rx == 0 in
cqspi_find_rx_high_sdr() (for example due to a marginal pass at tap 0, a
transient read error, or DLL lock timeout), phy->rx-- wraps from 0 to 255 and
the unsigned condition phy->rx >= lowerbound remains true forever while holding
the SPI controller mutexes.

[ ... ]
> +static void cqspi_phy_find_gaplow_ddr(struct cqspi_flash_pdata *f_pdata,
> +				      struct spi_mem *mem,
> +				      struct cqspi_phy_setting *bottomleft,
> +				      struct cqspi_phy_setting *topright,
> +				      struct cqspi_phy_setting *gaplow)
> +{
> +	struct cqspi_phy_setting left, right, mid;
> +	int ret;
> +
> +	left = *bottomleft;
> +	right = *topright;
> +
> +	mid.tx = left.tx + ((right.tx - left.tx) / 2);
> +	mid.rx = left.rx + ((right.rx - left.rx) / 2);
> +	mid.read_delay = left.read_delay;
> +
> +	do {
> +		ret = cqspi_phy_apply_setting(f_pdata, &mid);
> +		if (!ret)
> +			ret = cqspi_phy_check_pattern(f_pdata, mem);
> +
> +		if (ret) {
> +			/* The pattern was not found. Go to the lower half. */
> +			right.tx = mid.tx;
> +			right.rx = mid.rx;
> +
> +			mid.tx = left.tx + ((mid.tx - left.tx) / 2);
> +			mid.rx = left.rx + ((mid.rx - left.rx) / 2);
> +		} else {
> +			/* The pattern was found. Go to the upper half. */
> +			left.tx = mid.tx;
> +			left.rx = mid.rx;
> +
> +			mid.tx = mid.tx + ((right.tx - mid.tx) / 2);
> +			mid.rx = mid.rx + ((right.rx - mid.rx) / 2);
> +		}
> +
> +		/* Break the loop if the window has closed. */
> +	} while ((right.tx - left.tx >= 2) && (right.rx - left.rx >= 2));
> +
> +	*gaplow = mid;
> +}
[ ... ]
> +static int cqspi_phy_tuning_ddr(struct cqspi_flash_pdata *f_pdata,
> +				struct spi_mem *mem)
> +{
[ ... ]
> +	rx_window = rxhigh.rx - rxlow.rx;
> +	txlow.rx = rxlow.rx + (rx_window / 4);
> +	dev_dbg(dev, "Searching for Golden txlow on RX = %d\n", txlow.rx);
> +	txlow.read_delay = CQSPI_PHY_INIT_RD;
> +	ret = cqspi_find_tx_low_ddr(f_pdata, mem, &txlow);
> +	if (ret)
> +		goto out;
[ ... ]
> +		/* 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 = clamp((int)topright.tx - 16,
> +					       0, CQSPI_PHY_MAX_DELAY);
> +			searchpoint.rx = clamp((int)topright.rx -
> +					       (16 * (topright.rx - bottomleft.rx)) /
> +					       (topright.tx - bottomleft.tx),
> +					       0, CQSPI_PHY_MAX_DELAY);
> +		} else {
> +			/* Bottomleft further: Use Region 1, 16 taps inward */
> +			searchpoint = bottomleft;
> +			searchpoint.tx = clamp((int)bottomleft.tx + 16,
> +					       0, CQSPI_PHY_MAX_DELAY);
> +			searchpoint.rx = clamp((int)bottomleft.rx +
> +					       (16 * (topright.rx - bottomleft.rx)) /
> +					       (topright.tx - bottomleft.tx),
> +					       0, CQSPI_PHY_MAX_DELAY);
> +		}
> +	}

[Severity: Medium]
In drivers/spi/spi-cadence-quadspi.c, does cqspi_phy_tuning_ddr() handle the
case where rxhigh.rx is smaller than or close to rxlow.rx across different
read_delay values?

cqspi_find_rx_low_ddr() searches phy->rx in 0..40 across read_delay 1..4,
while cqspi_find_rx_high_ddr() searches phy->rx in 24..127 across read_delay
4..1. Because those RX ranges overlap in 24..40 and rxlow and rxhigh can be
found at different read_delay values, rxhigh.rx can be less than or nearly
equal to rxlow.rx:

1. In cqspi_phy_tuning_ddr(), rx_window = rxhigh.rx - rxlow.rx becomes
   negative when rxhigh.rx < rxlow.rx, which steps txlow.rx = rxlow.rx +
   (rx_window / 4) and backuppoint.rx = rxlow.rx + ((rx_window * 3) / 4)
   below rxlow.rx.

2. In cqspi_phy_find_gaplow_ddr() and cqspi_phy_find_gaphigh_ddr(), the loop
   condition is:

	} while ((right.tx - left.tx >= 2) && (right.rx - left.rx >= 2));

   Because both axes are combined with &&, the binary search stops as soon as
   the narrower axis span drops below 2 (or after a single iteration if
   topright.rx <= bottomleft.rx + 1), leaving the wider axis unsearched.

3. In the final point calculation in cqspi_phy_tuning_ddr(), when
   topright.rx < bottomleft.rx, (topright.rx - bottomleft.rx) is negative,
   which pushes searchpoint.rx outward past the valid RX boundary (above
   topright.rx for Region 2 or below bottomleft.rx for Region 1) instead of
   inward.

[ ... ]
> +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);
> +	}
> +
> +	if (ret)
> +		dev_warn(dev, "tuning failed: %d\n", ret);
> +
> +	cqspi_phy_post_config(cqspi, f_pdata->read_delay);
> +
> +out:
> +	if (!ret) {
> +		read_op->max_freq = mem->spi->post_config_max_speed_hz;
> +		if (write_op) {
> +			write_op->max_freq = mem->spi->post_config_max_speed_hz;
> +			f_pdata->phy_write_op.max_freq =
> +				mem->spi->post_config_max_speed_hz;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +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 || !cqspi->ddata->execute_tuning)
> +		return -EOPNOTSUPP;
> +
> +	return cqspi->ddata->execute_tuning(mem, read_op, write_op);
> +}

[Severity: High]
Can cqspi_am654_ospi_execute_tuning() access MMIO registers while the
controller is runtime-suspended?

In drivers/spi/spi-cadence-quadspi.c, cqspi_probe() enables runtime PM with
autosuspend on &pdev->dev without setting host->auto_runtime_pm. As a result,
spi_mem_access_start() does not wake the controller, and cqspi_exec_mem_op()
acquires and releases runtime PM (and cqspi->inflight_ops) around each
individual memory operation:

	ret = pm_runtime_resume_and_get(dev);
	if (ret) {
		dev_err(&mem->spi->dev, "resume failed with %d\n", ret);
		goto dec_inflight_refcount;
	}

	ret = cqspi_mem_process(mem, op);

	pm_runtime_put_autosuspend(dev);

Unlike cqspi_exec_mem_op(), cqspi_mem_op_execute_tuning() does not hold a
runtime PM reference or an inflight_ops reference across the tuning sequence.
When cqspi_write_pattern_to_cache() or cqspi_phy_check_pattern() returns from
ctlr->mem_ops->exec_op(), the runtime PM usage count drops back to zero, so
direct MMIO accesses in cqspi_phy_pre_config(), cqspi_phy_apply_setting(),
cqspi_set_dll(), cqspi_resync_dll(), and cqspi_phy_post_config() run without
an active runtime PM reference. If autosuspend triggers between operations,
cqspi_runtime_suspend() can gate the clocks during MMIO access or
cqspi_runtime_resume() can reinitialize the controller mid-calibration.

Should cqspi_mem_op_execute_tuning() hold pm_runtime_resume_and_get() and
cqspi->inflight_ops across the execute_tuning callback in the same way as
cqspi_exec_mem_op()?

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

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