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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Manual PHY dummy cycle adjustments are overwritten and clobbered by subsequent read setups.
- [Medium] SDR PHY tuning fails for valid 1-tap wide windows due to an off-by-one error in the loop condition.
- [Medium] Incorrect parsing of 64-bit addresses for 'spi-phy-pattern-partition'.
--

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

spi: cadence-quadspi: add PHY tuning support

This commit adds an execute_tuning callback for the Cadence QSPI controller
to perform delay-line calibration. It introduces 2D sweeps for DDR protocols
and 1D sweeps for SDR protocols by reading a known data pattern or writing
to the page cache before calibration.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 046fa5d69b0f7..f7cda1bc71393 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1559,10 +1718,1594 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
> +static void cqspi_phy_pre_config(struct cqspi_st *cqspi,
> +				 struct cqspi_flash_pdata *f_pdata,
> +				 const bool bypass)
> +{
[ ... ]
> +	reg = readl(reg_base + CQSPI_REG_RD_INSTR);
> +	dummy = FIELD_GET(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +				  << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> +			  reg);
> +	cqspi->phy_saved_dummy = dummy;
> +	if (dummy)
> +		dummy--;
> +	reg &= ~(CQSPI_REG_RD_INSTR_DUMMY_MASK << CQSPI_REG_RD_INSTR_DUMMY_LSB);
> +	reg |= FIELD_PREP(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +				  << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> +			  dummy);
> +	writel(reg, reg_base + CQSPI_REG_RD_INSTR);

[Severity: High]
Does this modification to CQSPI_REG_RD_INSTR get clobbered during tuning?

During the tuning process, cqspi_phy_check_pattern() executes a read via the
SPI core. This eventually calls cqspi_read_setup(), which recalculates the
dummy cycles from the spi_mem_op and unconditionally overwrites
CQSPI_REG_RD_INSTR:

cqspi_read_setup() {
    ...
    dummy_clk = cqspi_calc_dummy(op);
    if (dummy_clk)
        reg |= (dummy_clk & CQSPI_REG_RD_INSTR_DUMMY_MASK)
               << CQSPI_REG_RD_INSTR_DUMMY_LSB;
    writel(reg, reg_base + CQSPI_REG_RD_INSTR);
    ...
}

Will this overwrite destroy the PHY pipeline adjustment made here before the
tuning read even executes, causing the tuning reads to fail?

> +
> +	cqspi_phy_set_dll_master(cqspi);
> +}
[ ... ]
> +static int cqspi_find_rx_high_sdr(struct cqspi_flash_pdata *f_pdata,
> +				  struct spi_mem *mem, struct 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);

[Severity: Medium]
Does this loop condition prematurely fail for 1-tap wide windows?

If the valid SDR window is exactly 1-tap wide, the upper boundary will be
equal to lowerbound. Because the loop condition is strictly greater-than
(phy->rx > lowerbound), it will exit immediately when reaching lowerbound
without returning success for that tap. Should this be >= lowerbound so
narrow windows are properly detected?

> +
> +	dev_dbg(dev, "Unable to find RX high\n");
> +	return -ENOENT;
> +}
[ ... ]
> @@ -1588,9 +3331,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 truncate 64-bit partition offsets?

If the SPI device specifies #address-cells = <2>, the reg property contains
a 64-bit address. Reading it with of_property_read_u32_index() fetches only
the upper 32 bits (which is 0 for offsets under 4GB). This could lead to the
pattern being read from the wrong flash offset instead of the dedicated
partition.

> +		of_node_put(part_np);
>  	}

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