Re: [PATCH v8 08/18] spi: cadence-quadspi: add PHY tuning support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The DDR PHY tuning search loops modify the TX loop variable after a successful pattern match but before exiting the loop, resulting in an incorrect TX coordinate being recorded.
- [High] The `execute_tuning` callback modifies controller registers and executes SPI transfers without acquiring the SPI bus lock or maintaining a continuous PM runtime reference.
- [Medium] Device tree `reg` property parsing incorrectly assumes a 32-bit offset.
--

--- Patch [8]: [PATCH v8 08/18] spi: cadence-quadspi: add PHY tuning support ---
commit dcb00c8fe3b55241aea74077e15b54f0457e2f13
Author: Santhosh Kumar K <[email protected]>

spi: cadence-quadspi: add PHY tuning support

This commit adds an execute_tuning callback for delay-line calibration in
Cadence QSPI controllers to enable high-speed operation. It performs sweeps
of rx_delay and tx_delay pairs to find the optimal operating point. The
feature is currently enabled for am654-based OSPI controllers.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 046fa5d69b0f7..5651320426b14 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1559,10 +1717,1590 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
>  	return spi_mem_default_supports_op(mem, op);
>  }
>  
> +static int cqspi_write_pattern_to_cache(struct cqspi_flash_pdata *f_pdata,
> +					struct spi_mem *mem,
> +					const struct spi_mem_op *write_op)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	struct device *dev = &f_pdata->cqspi->pdev->dev;
> +	struct spi_mem_op op = *write_op;
> +	int ret;
> +
> +	op.max_freq = mem->spi->max_speed_hz;
> +	op.data.nbytes = sizeof(phy_tuning_pattern);
> +	op.data.buf.out = phy_tuning_pattern;
> +
> +	ret = ctlr->mem_ops->exec_op(mem, &op);

[Severity: High]
Does this bypass the SPI bus lock? By directly invoking the unlocked
ctlr->mem_ops->exec_op() callback instead of the standard spi_mem_exec_op(),
could concurrent transfers to other chip selects interleave and corrupt the
tuning configuration?

> +	if (ret) {
> +		dev_err(dev, "Failed to write PHY pattern to cache: %d\n", ret);
> +		return ret;
> +	}

[ ... ]

> +static int cqspi_phy_tuning_ddr(struct cqspi_flash_pdata *f_pdata,
> +				struct spi_mem *mem)
> +{

[ ... ]

> +	/* Primary rxlow: Sweep TX window to find valid RX lower bound */
> +
> +	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;

[Severity: High]
Will this modify the TX loop variable even after a successful pattern match?

When cqspi_find_rx_low_ddr() returns 0 (success), the loop condition evaluates
to false and terminates. However, rxlow.tx is unconditionally incremented here
before exiting. 

Does this mean the selected TX baseline is shifted by 4 taps from the optimal
value, degrading calibration accuracy?

> +	} while (ret && rxlow.tx <= CQSPI_PHY_TX_LOOKUP_LOW_END);
> +	if (ret)
> +		goto out;

[ ... ]

> +static int cqspi_am654_ospi_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);
> +	struct cqspi_flash_pdata *f_pdata;

[ ... ]

> +	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);

[Severity: High]
Are hardware registers modified here without holding a PM runtime reference?

The spi_mem_execute_tuning() helper does not acquire the controller's PM
runtime reference or the SPI bus lock. Calling cqspi_phy_pre_config() performs
raw memory-mapped register accesses (like CQSPI_REG_CONFIG) without calling
pm_runtime_resume_and_get() first. 

Could this cause a synchronous bus fault if the hardware is runtime-suspended?

> +		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);
> +	}

[ ... ]

> @@ -1588,9 +3326,12 @@ static int cqspi_of_get_flash_pdata(struct platform_device *pdev,
>  		return -ENXIO;
>  	}
>  
> -	if (of_property_read_u32(np, "spi-max-frequency", &f_pdata->clk_rate)) {
> -		dev_err(&pdev->dev, "couldn't determine spi-max-frequency\n");
> -		return -ENXIO;
> +	part_np = of_parse_phandle(np, "spi-phy-pattern-partition", 0);
> +	if (part_np) {
> +		f_pdata->phy_pattern_partition_valid =
> +			!of_property_read_u32_index(part_np, "reg", 0,
> +						    &f_pdata->phy_pattern_partition_offset);

[Severity: Medium]
Will this property read fail on platforms where the device tree uses 64-bit
addresses? 

Using of_property_read_u32_index() for an address just grabs the first 32-bit
cell, which will evaluate to 0 if #address-cells = <2> is set. 

Would it be safer to use of_property_read_u64() or standard DT address helpers
so the tuning pattern offset is parsed correctly?

> +		of_node_put(part_np);
>  	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8