Re: [PATCH v9 11/18] spi: cadence-quadspi: enable PHY for direct reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bypassing PHY for unaligned or short direct reads at high frequency causes silent data corruption.
- [High] Modifying QSPI configuration registers before the controller is idle causes hardware race conditions.
--

commit 478fb069d36d7f959d00975130c0e1c3988aa09c
Author: Santhosh Kumar K <s-k6@ti.com>

spi: cadence-quadspi: enable PHY for direct reads

This commit adds support for toggling PHY mode around DMA transfers in the
Cadence QSPI driver. It introduces cqspi_tune_phy() to manage the PHY DLL
settings and configuration, and falls back to non-PHY reads for unaligned or
small transfers.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 58bca15dbd6ec..d38a0253e4bda 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1075,6 +1075,109 @@ static int cqspi_tune_phy(struct cqspi_flash_pdata *f_pdata, bool enable)

[ ... ]

> +	/* Disable path: execute unconditionally regardless of idle state. */
> +	cqspi_readdata_capture(cqspi, !cqspi->rclk_en, false,
> +			       f_pdata->read_delay);
> +
> +	reg = readl(reg_base + CQSPI_REG_CONFIG);
> +	reg &= ~(CQSPI_REG_CONFIG_PHY_EN |
> +		 CQSPI_REG_CONFIG_PHY_PIPELINE);
> +	writel(reg, reg_base + CQSPI_REG_CONFIG);

[Severity: High]
Does modifying active configuration registers before waiting for the controller
to become idle cause a hardware race condition?

When a successful direct read completes its DMA transfer, cqspi_tune_phy() is
called to disable the PHY. Because the QSPI prefetches data in direct mode,
the SPI bus may still be actively receiving data when the DMA finishes,
potentially violating IP requirements if the controller is not idle.

[ ... ]

> @@ -1630,6 +1741,7 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>  	loff_t from = op->addr.val;
>  	size_t len = op->data.nbytes;
>  	u_char *buf = op->data.buf.in;
> +	int ret;
>  
>  	if (!cqspi->rx_chan || !virt_addr_valid(buf) ||
>  	    len < CQSPI_PHY_MIN_DIRECT_READ_LEN) {
>  		cqspi_memcpy_fromio(op, buf, cqspi->ahb_base + from, len);
>  		return 0;
>  	}

[Severity: High]
Can bypassing the PHY for short direct reads at high frequencies cause silent
data corruption?

If a subsystem issues a short read that falls back to cqspi_memcpy_fromio(),
the read will execute at the high post_config_max_speed_hz (e.g., 200MHz DTR)
but without DQS or PHY DLL timing, capturing garbage data.

> -	return cqspi_direct_read_dma(f_pdata, buf, from, len);
> +
> +	if (!cqspi_use_tuned_phy(f_pdata, op, post_config_max_speed_hz))
> +		return cqspi_direct_read_dma(f_pdata, buf, from, len);
> +
> +	if (!IS_ALIGNED(from, 16) || !IS_ALIGNED(from + len, 16))
> +		return cqspi_direct_read_dma(f_pdata, buf, from, len);

[Severity: High]
Does this unaligned fallback also risk silent data corruption for the same
reason? 

Similar to short reads, falling back to cqspi_direct_read_dma() without the
PHY enabled means the controller executes the read at high speed without the
tuned PHY data-capture timing.

> +	ret = cqspi_tune_phy(f_pdata, true);
> +	if (ret)
> +		return ret;
> +	ret = cqspi_direct_read_dma(f_pdata, buf, from, len);
> +	ret = cqspi_tune_phy(f_pdata, false) ?: ret;

The race condition mentioned earlier triggers here. The DMA transfer completes
and immediately calls cqspi_tune_phy(..., false) without waiting for idle.

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