Re: [PATCH v8 12/18] spi: cadence-quadspi: enable PHY for indirect writes

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PHY is enabled for indirect writes without verifying the operation's actual frequency, causing PHY timing mismatch at lower frequencies.
- [High] Ignoring the return value of `cqspi_wait_idle()` on the success path leaves the PHY enabled if a timeout occurs.
--

--- Patch [12]: [PATCH v8 12/18] spi: cadence-quadspi: enable PHY for indirect writes ---
commit 573d2ac8c7923def59b134920cd9dc6bde273f1c
Author: Santhosh Kumar K <[email protected]>

spi: cadence-quadspi: enable PHY for indirect writes

This commit enables PHY for indirect writes larger than a minimum length
to amortize PHY enable/disable overhead. It threads post_config_max_speed_hz
into the indirect write execution path and attempts to safely disable the
PHY even if the write fails.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1362,18 +1364,32 @@ static int cqspi_write_setup(struct cqspi_flash_pdata *f_pdata,
>  
>  static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
>  					loff_t to_addr, const u8 *txbuf,
> -					const size_t n_tx)
> +					const size_t n_tx,
> +					u32 post_config_max_speed_hz)
>  {
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
>  	struct device *dev = &cqspi->pdev->dev;
>  	void __iomem *reg_base = cqspi->iobase;
>  	unsigned int remaining = n_tx;
>  	unsigned int write_bytes;
> +	bool use_tuned_phy_write;
>  	int ret;
>  
>  	if (!refcount_read(&cqspi->refcount))
>  		return -ENODEV;
>  
> +	/* Use PHY only for large writes at the calibrated rate */
> +	use_tuned_phy_write = n_tx >= CQSPI_PHY_MIN_INDIRECT_WRITE_LEN &&
> +			      f_pdata->use_tuned_phy &&
> +			      f_pdata->phy_write_op.max_freq ==
> +				      post_config_max_speed_hz;

[Severity: High]
Will this apply high-frequency DLL calibrations to lower frequency operations?

The driver dynamically configures the clock frequency per operation via
cqspi_configure() using op->max_freq. However, this condition checks if the
calibrated maximum frequency matches the post-configuration maximum speed,
rather than checking the current operation's actual frequency.

If an indirect write executes at a lower frequency than the calibrated
maximum, wouldn't this condition still evaluate to true and corrupt data
by applying mismatched PHY timing?

> +
> +	if (use_tuned_phy_write) {
> +		ret = cqspi_tune_phy(f_pdata, true);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
>  	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
[ ... ]
> @@ -1452,15 +1468,26 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
>  
>  	cqspi_wait_idle(cqspi);
>  
> +	if (use_tuned_phy_write)
> +		cqspi_tune_phy(f_pdata, false);
> +
>  	return 0;

[Severity: High]
If cqspi_wait_idle() times out here, does it leak the enabled PHY state?

The return value of cqspi_wait_idle() is ignored on this success path. If
a timeout occurs, the subsequent call to cqspi_tune_phy(f_pdata, false)
will immediately fail its internal idle check and fail to disable the PHY.

Since this function still returns 0 (success) in that scenario, the caller
is unaware of the failure, and subsequent non-PHY operations (like polling
the status register) will execute with the PHY unexpectedly active.

Should the return value of cqspi_wait_idle() be checked, or the error
properly handled so the PHY is successfully disabled?

>  
>  failwr:
>  	/* Disable interrupt. */
>  	writel(0, reg_base + CQSPI_REG_IRQMASK);

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