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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PHY tuning is incorrectly executed after the indirect write operation has been started, causing hardware hangs and a guaranteed timeout.
- [High] A failure during PHY enablement permanently corrupts the controller's dummy cycle configuration due to asymmetric error handling.
- [High] Restoring PHY state before canceling a pending indirect write causes secondary timeouts in the error handler.
--

--- Patch [12]: [PATCH v5 12/17] spi: cadence-quadspi: enable PHY for indirect writes ---
commit ee250e92bd8277b7a8635c4c209796570036798e
Author: Santhosh Kumar K <s-k6@ti.com>

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.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 35a4000739449..542ad331ff167 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1377,6 +1381,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 this execute PHY tuning after the indirect write operation has already
started? Modifying global controller configuration while an indirect
operation is active could violate hardware sequencing rules. 

Since the controller is actively waiting for FIFO data, would the call chain
cqspi_tune_phy()->cqspi_wait_idle() time out and fail the write?

>  	while (remaining > 0) {
>  		size_t write_words, mod_bytes;
[ ... ]
> @@ -1425,9 +1441,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]
If cqspi_tune_phy(f_pdata, true) fails to enable the PHY and returns an
error, it returns early and avoids decrementing the dummy cycle count. 

Would jumping to failwr here and unconditionally calling
cqspi_tune_phy(f_pdata, false) end up incrementing the dummy cycle count
without a prior decrement? This could permanently corrupt the controller's
dummy cycle configuration for subsequent read operations.

[Severity: High]
Is cqspi_tune_phy() called here before the pending indirect write is canceled
in the error handler? 

If cqspi_tune_phy() calls cqspi_wait_idle(), could this cause a secondary
timeout in the error handler since the indirect write hasn't been canceled
yet?

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

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