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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: cadence-quadspi: missing clock fallback in cqspi_write() without PHY
--

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

spi: cadence-quadspi: enable PHY for indirect writes

This commit enables PHY mode for indirect writes of at least 1 KiB when the
operation runs at the calibrated post-configuration clock rate. It adds
cqspi_tune_phy_write() to toggle PHY around the indirect write transfer and
passes post_config_max_speed_hz from cqspi_mem_process() through
cqspi_write().

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 55c446f4245af..806bf49e814d4 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -1602,12 +1653,14 @@ static bool cqspi_use_tuned_phy(struct cqspi_flash_pdata *f_pdata,
>  }
>  
>  static ssize_t cqspi_write(struct cqspi_flash_pdata *f_pdata,
> -			   const struct spi_mem_op *op)
> +			   const struct spi_mem_op *op,
> +			   u32 post_config_max_speed_hz)
>  {
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
>  	loff_t to = op->addr.val;
>  	size_t len = op->data.nbytes;
>  	const u_char *buf = op->data.buf.out;
> +	bool use_phy;
>  	int ret;
>  
>  	ret = cqspi_write_setup(f_pdata, op);
> @@ -1629,7 +1682,9 @@ static ssize_t cqspi_write(struct cqspi_flash_pdata *f_pdata,
>  		return cqspi_wait_idle(cqspi);
>  	}
>  
> -	return cqspi_indirect_write_execute(f_pdata, to, buf, len);
> +	use_phy = len >= CQSPI_PHY_MIN_INDIRECT_WRITE_LEN &&
> +		  cqspi_use_tuned_phy(f_pdata, op, post_config_max_speed_hz);
> +	return cqspi_indirect_write_execute(f_pdata, to, buf, len, use_phy);
>  }

[Severity: High]
When a write operation has op->max_freq == post_config_max_speed_hz and
f_pdata->use_tuned_phy is true, cqspi_mem_process() configures the controller
clock to post_config_max_speed_hz before calling cqspi_write():

drivers/spi/spi-cadence-quadspi.c:cqspi_mem_process() {
	...
	/* Tuning inactive, or STIG (no PHY handling of its own): use safe rate. */
	if (sclk == mem->spi->post_config_max_speed_hz &&
	    (!f_pdata->use_tuned_phy || use_stig))
		sclk = mem->spi->max_speed_hz;

	cqspi_configure(f_pdata, sclk);
	...
	if (!op->addr.nbytes || !op->data.buf.out)
		return cqspi_command_write(f_pdata, op);

	return cqspi_write(f_pdata, op, mem->spi->post_config_max_speed_hz);
}

In cqspi_write(), if len < CQSPI_PHY_MIN_INDIRECT_WRITE_LEN (or if the
direct-mode write branch is taken, or cqspi_command_write() is called in
cqspi_mem_process()), PHY mode is not enabled. Unlike cqspi_read() and
cqspi_direct_read_fallback(), which call cqspi_configure(f_pdata,
max_speed_hz) when PHY is not used, the write path never drops the clock back
down to mem->spi->max_speed_hz.

Won't this cause writes shorter than 1 KiB (as well as direct-mode or STIG
writes at post_config_max_speed_hz) to run at the tuned post-configuration
clock rate without PHY delay-line compensation?

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/