Re: [PATCH v9 11/18] spi: cadence-quadspi: enable PHY for direct reads
From: sashiko-bot@kernel.org
Date: 2026-08-25 17:32:37
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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