Re: [PATCH v10 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:
- [Medium] spi: cadence-quadspi: modify CONFIG before controller idle in cqspi_tune_phy_write()
--

commit 4f09f6b26f85c3cbe5dbc4f013c4a13b3d48d1d9
Author: Santhosh Kumar K <s-k6@ti.com>

spi: cadence-quadspi: enable PHY for indirect writes

This commit enables PHY for indirect writes of at least a minimum length,
adding cqspi_tune_phy_write() to toggle PHY mode based on calibrated rate
checks. It ensures indirect writes are cancelled before disabling the PHY
in the error path.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index ad99e72578337..2752e7cce16cf 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1175,6 +1178,37 @@ static int cqspi_tune_phy_read(struct cqspi_flash_pdata *f_pdata, bool enable)
>  	return ret;
>  }
>  
> +static int cqspi_tune_phy_write(struct cqspi_flash_pdata *f_pdata, bool enable)
> +{
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	void __iomem *reg_base = cqspi->iobase;
> +	unsigned int reg;
> +	int ret;
> +
> +	if (!enable) {
> +		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);
> +		return cqspi_wait_idle(cqspi);
> +	}

[Severity: Medium]
Is it safe to modify CQSPI_REG_CONFIG here before ensuring the controller
is idle? In the error path of cqspi_indirect_write_execute(), this is called
with enable=false immediately after cancelling the write. If the controller
is still busy waiting for AHB data, could modifying this register lead to
undefined hardware behavior and potentially leave the PHY enabled?

[ ... ]
> @@ -1367,7 +1401,7 @@ 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, bool use_phy)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does the remainder byte
handling in this function (and cqspi_indirect_read_execute) corrupt partial
transfers on big-endian systems?

For the bulk of the transfer, the stream accessors (iowrite32_rep /
ioread32_rep) are used, which preserve byte order. However, for the
remainder bytes:

		if (mod_bytes) {
			unsigned int temp = 0xFFFFFFFF;

			memcpy(&temp, txbuf, mod_bytes);
			iowrite32(temp, cqspi->ahb_base);
			txbuf += mod_bytes;
		}

Will using iowrite32() (and ioread32() for reads) cause the remainder bytes
to be unexpectedly byte-swapped on big-endian hardware since they use
register accessors instead of stream accessors?

[ ... ]

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