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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Enabling PHY after starting the indirect write operation guarantees a deadlock and hardware timeout, while the error path compounds the issue with a secondary timeout.

Pre-existing issues:
- [High] Missing PHY disable cleanup when `cqspi_tune_phy(true)` fails during direct read.
--

commit 58e38b1c2aa32f1b8542b6e43dd5365e97bd7dce
Author: Santhosh Kumar K <s-k6@ti.com>
Subject: spi: cadence-quadspi: enable PHY for indirect writes

Enable PHY for indirect writes of at least CQSPI_PHY_MIN_INDIRECT_WRITE_LEN
bytes. PHY is activated only when tuning completed successfully and the
write op runs at the calibrated post-config frequency, matching the same
frequency guard used by the read path.

Thread max_speed_hz from cqspi_mem_process() through cqspi_write() into
cqspi_indirect_write_execute() for the frequency check.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 057381e56a7fd..f4cf97a909fc5 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -1073,13 +1242,15 @@ 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))

[ ... ]

> @@ -1115,6 +1286,18 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
>  	if (cqspi->apb_ahb_hazard)
>  		readl(reg_base + CQSPI_REG_INDIRECTWR);
>  
> +	/* 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;
> +
> +	if (use_tuned_phy_write) {
> +		ret = cqspi_tune_phy(f_pdata, true);
> +		if (ret)
> +			goto failwr;
> +	}

[Severity: High]
Does calling cqspi_tune_phy() here guarantee a timeout?

Since cqspi_tune_phy() internally calls cqspi_wait_idle(), and this is
executed after writing CQSPI_REG_INDIRECTWR_START_MASK to start the indirect
write controller, the controller is already busy waiting for data on the AHB
interface.

Will this cause cqspi_wait_idle() to deterministically poll for 500ms and
return -ETIMEDOUT?

>  	while (remaining > 0) {
>  		size_t write_words, mod_bytes;

[ ... ]

> @@ -1163,9 +1346,15 @@ 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;
>  
>  failwr:
> +	if (use_tuned_phy_write)
> +		cqspi_tune_phy(f_pdata, false);
> +

[Severity: High]
Could this error path cause a secondary timeout?

Since the indirect write operation is not yet canceled when cqspi_tune_phy()
is called, the controller is still busy.

When cqspi_tune_phy() calls cqspi_wait_idle(), wouldn't it time out again
for another 500ms before we write CQSPI_REG_INDIRECTWR_CANCEL_MASK below?

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


This is a pre-existing issue, but I noticed a similar cleanup problem in
cqspi_direct_read_execute():

drivers/spi/spi-cadence-quadspi.c:cqspi_direct_read_execute() {
    ...
	if (len_aligned) {
		ret = cqspi_tune_phy(f_pdata, true);
		if (ret)
			return ret;
		ret = cqspi_direct_read_dma(f_pdata, buf, from_aligned,
					    len_aligned);
		cqspi_tune_phy(f_pdata, false);
    ...
}

[Severity: High]
If cqspi_tune_phy(f_pdata, true) fails, it returns directly without calling
cqspi_tune_phy(f_pdata, false).

Since cqspi_tune_phy(true) modifies the hardware state by enabling the PHY
in CQSPI_REG_CONFIG and decrementing the read dummy cycles in
CQSPI_REG_RD_INSTR before checking for idle, doesn't returning here leave
the QSPI controller with the PHY partially enabled and an incorrect number
of dummy cycles?

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