Re: [PATCH v8 14/18] spi: cadence-quadspi: reprogram PHY DLL on runtime resume

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing fallback to non-PHY mode when DLL resync fails during resume, breaking subsequent SPI transfers and causing silent data corruption.
--

--- Patch [14]: [PATCH v8 14/18] spi: cadence-quadspi: reprogram PHY DLL on runtime resume ---
commit d9e9d29f6693cbe5e675d990e2acc004b9dd86eb
Author: Santhosh Kumar K <[email protected]>

spi: cadence-quadspi: reprogram PHY DLL on runtime resume

This commit reprograms the PHY DLL and relocks it during runtime resume
for all calibrated chip selects. It treats a DLL resync failure as a
non-fatal warning so the system can theoretically degrade to non-PHY speed
without blocking the resume path.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index d4181b501d8df..c78d4ce1c930a 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -4030,6 +4030,23 @@ static int cqspi_runtime_resume(struct device *dev)
>  
>  	cqspi->current_cs = -1;
>  	cqspi->sclk = 0;
> +
> +	for (cs = 0; cs < CQSPI_MAX_CHIPSELECT; cs++) {
> +		struct cqspi_flash_pdata *f_pdata = &cqspi->f_pdata[cs];
> +
> +		if (!f_pdata->use_tuned_phy)
> +			continue;
> +
> +		cqspi_phy_set_dll_master(cqspi);
> +
> +		cqspi_set_dll(cqspi->iobase, f_pdata->phy_setting.rx,
> +			      f_pdata->phy_setting.tx);
> +		ret = cqspi_resync_dll(cqspi);
> +		if (ret)
> +			dev_warn(dev, "cs%d: PHY DLL resync failed on resume: %d\n",
> +				 cs, ret);
[Severity: High]
If cqspi_resync_dll() fails, should this also set f_pdata->use_tuned_phy
to false?

The commit message mentions that on failure the system degrades to non-PHY
speed, but without explicitly disabling use_tuned_phy, subsequent transfers
will still attempt to use the PHY.

Because cqspi_set_dll() updates the CQSPI_REG_PHY_CONFIG register, the
global hardware state now matches f_pdata->phy_setting. During the next
transfer, cqspi_tune_phy() will see that the hardware registers match the
desired values and bypass its own cqspi_resync_dll() call:

drivers/spi/spi-cadence-quadspi.c:cqspi_tune_phy() {
    ...
	reg = readl(reg_base + CQSPI_REG_PHY_CONFIG);
	if (((reg >> CQSPI_REG_PHY_CONFIG_RX_DEL_LSB) &
	     CQSPI_REG_PHY_CONFIG_RX_DEL_MASK) != f_pdata->phy_setting.rx ||
	    ((reg >> CQSPI_REG_PHY_CONFIG_TX_DEL_LSB) &
	     CQSPI_REG_PHY_CONFIG_TX_DEL_MASK) != f_pdata->phy_setting.tx) {
		cqspi_set_dll(reg_base, f_pdata->phy_setting.rx,
			      f_pdata->phy_setting.tx);
		ret = cqspi_resync_dll(cqspi);
		if (ret)
			return ret;
	}
    ...
}

Could this cause cqspi_tune_phy() to incorrectly proceed and enable the PHY
with an unlocked DLL, leading to data corruption?

> +	}
> +
>  	return 0;
>  }
>

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