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

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

New issues:
- [High] The error path in `cqspi_tune_phy` (when `enable` is true) fails to restore the `CQSPI_REG_READCAPTURE` register, leaving the controller in a corrupted state for subsequent non-PHY reads.
- [High] `cqspi_tune_phy` (enable=false) bails out early on idle wait timeout, leaving the PHY permanently enabled, and its return value is ignored by callers, masking the failure.
- [High] A failed DLL resync leaves `CQSPI_REG_PHY_CONFIG` updated, causing subsequent retries to skip the resync and use an unlocked PHY.

Pre-existing issues:
- [High] The PIO fallback path in `cqspi_memcpy_fromio()` causes data corruption on big-endian architectures due to mismatched I/O accessors.
--

--- Patch [11]: [PATCH v8 11/18] spi: cadence-quadspi: enable PHY for direct reads ---
commit b1257269b9ef8a064e84e36b6359dd1d26e31e0d
Author: Santhosh Kumar K <[email protected]>
Subject: spi: cadence-quadspi: enable PHY for direct reads

This patch adds support for toggling the PHY mode around direct DMA reads in
the Cadence QSPI driver. It introduces cqspi_tune_phy() to configure and
enable the PHY pipeline and reprogram the DLL if necessary. It also adds a
predicate to ensure PHY is only used when the operation frequency matches the
tuned frequency and the transfer is 16-byte aligned.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 99a0ec0e769c5..8903b4cf4f6f5 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1074,6 +1074,102 @@ static int cqspi_resync_dll(struct cqspi_st *cqspi)
>  	return ret;
>  }
>  
> +static int cqspi_tune_phy(struct cqspi_flash_pdata *f_pdata, bool enable)
> +{
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	void __iomem *reg_base = cqspi->iobase;
> +	u32 reg;
> +	u8 dummy;
> +	int ret;
> +
> +	ret = cqspi_wait_idle(cqspi);
> +	if (ret)
> +		return ret;

[Severity: High]
If this cqspi_wait_idle() times out when enable is false, the function returns
early without clearing CQSPI_REG_CONFIG_PHY_EN or restoring the read capture
settings. Does this leave the SPI controller permanently stuck in PHY mode,
which would break subsequent non-PHY communication with the flash?

> +
> +	if (enable) {

[ ... ]

> +		reg = readl(reg_base + CQSPI_REG_PHY_CONFIG);
> +		if (((reg >> CQSPI_REG_PHY_CONFIG_RX_DEL_LSB) &
> +		     CQSPI_REG_PHY_CONFIG_RX_DEL_MASK) != f_pdata->phy_setting.rx ||
> +		    ((reg >> CQSPI_REG_PHY_CONFIG_TX_DEL_LSB) &
> +		     CQSPI_REG_PHY_CONFIG_TX_DEL_MASK) != f_pdata->phy_setting.tx) {
> +			cqspi_set_dll(reg_base, f_pdata->phy_setting.rx,
> +				      f_pdata->phy_setting.tx);
> +			ret = cqspi_resync_dll(cqspi);
> +			if (ret)
> +				return ret;

[Severity: High]
If cqspi_resync_dll() fails, the hardware register CQSPI_REG_PHY_CONFIG
still retains the new settings written by cqspi_set_dll(). On a subsequent
retry, the initial check above will see that the register matches the device's
settings, skipping the resync entirely. Can this lead to operating the PHY
with an unsynced or unlocked DLL?

> +		}
> +
> +		cqspi_readdata_capture(cqspi, !f_pdata->use_dqs, f_pdata->use_dqs,
> +				       f_pdata->phy_setting.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);
> +

[ ... ]

> +		ret = cqspi_wait_idle(cqspi);
> +		if (ret) {
> +			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);
> +			reg = readl(reg_base + CQSPI_REG_RD_INSTR);
> +			reg &= ~(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +				 << CQSPI_REG_RD_INSTR_DUMMY_LSB);
> +			reg |= FIELD_PREP(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +						  << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> +					  cqspi->phy_saved_dummy);
> +			writel(reg, reg_base + CQSPI_REG_RD_INSTR);
> +		}
> +		return ret;

[Severity: High]
This error path rolls back CQSPI_REG_CONFIG and CQSPI_REG_RD_INSTR, but it
seems to miss reverting CQSPI_REG_READCAPTURE, which was modified earlier
via cqspi_readdata_capture(). Since cqspi_configure() optimizes out
READCAPTURE updates unless the clock frequency or chip-select changes, could
this cause subsequent standard reads to use incorrect PHY capture settings and
corrupt data?

> +	}

[ ... ]

> @@ -1629,6 +1733,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) {

[ ... ]

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

[Severity: High]
Is it possible this hides a hardware failure from the SPI subsystem since
the return value is still 0 from the DMA read? The return value of
cqspi_tune_phy(f_pdata, false) is ignored here, so if the DMA read succeeds
but disabling the PHY times out during cqspi_wait_idle(), the operation is
reported as a success despite the controller being left in a broken state.


While reviewing this, I also noticed a pre-existing issue in the driver.

[Severity: High]
This is a pre-existing issue, but the PIO fallback path in
cqspi_memcpy_fromio() appears to mix accessors in a way that breaks
big-endian systems:

drivers/spi/spi-cadence-quadspi.c:cqspi_memcpy_fromio() {
    ...
    while (count >= 4) {
        put_unaligned_le32(__raw_readl(from), to);
        from += 4;
        to += 4;
        count -= 4;
    }
    ...
}

The __raw_readl() performs a native load without byte-swapping, but
put_unaligned_le32() will apply a byteswap before storing on big-endian
hardware. Can this incorrectly reverse the byte order of the stream in 4-byte
chunks during unaligned DMA transfers that fallback to PIO?

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