Re: [PATCH v6 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 and configuration are illegally executed while the indirect write controller is already active, causing a guaranteed timeout and hardware state corruption.
- [High] Incorrect teardown order on the `failwr` error path causes a secondary 500ms timeout by waiting for the controller to become idle while it is still hung.
- [Medium] Reusing `cqspi_tune_phy()` for writes underflows the dummy cycle count, injecting 31 unintended dummy cycles.
--

--- Patch [12]: [PATCH v6 12/17] spi: cadence-quadspi: enable PHY for indirect writes ---
commit 37f2245caf607b9e33d418045174172a49f40271
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 1KB that run at the
calibrated post-config frequency. It threads the max speed parameter into
the write execution path to match the read path's frequency guard.

> 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);

[Severity: High]
Does this call to cqspi_tune_phy() happen after the hardware state machine
has already been started?

In cqspi_indirect_write_execute(), the CQSPI_REG_INDIRECTWR_START_MASK bit
is written to CQSPI_REG_INDIRECTWR just before this block. This activates
the state machine, making the controller busy as it waits for AHB data. 

When cqspi_tune_phy() is called here, it eventually calls cqspi_wait_idle().
Will this create a deadlock where the CPU waits 500ms for the controller
to become idle, while the controller waits for the CPU to push data?

[Severity: Medium]
Does reusing cqspi_tune_phy() for writes cause an underflow of the dummy 
cycle count?

The cqspi_tune_phy() function unconditionally decrements the dummy cycle count:

drivers/spi/spi-cadence-quadspi.c:cqspi_tune_phy() {
    ...
    dummy = FIELD_GET(CQSPI_REG_RD_INSTR_DUMMY_MASK
                      << CQSPI_REG_RD_INSTR_DUMMY_LSB,
                      reg);
    dummy--;
    ...
}

Since indirect writes typically have 0 dummy cycles, does this unsigned
decrement underflow to 255 and overwrite CQSPI_REG_RD_INSTR with 31 
unintended dummy cycles?

> +		if (ret)
> +			goto failwr;
> +	}
> +
>  	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]
Will this teardown order cause a secondary timeout during error recovery?

When the initial cqspi_tune_phy() call times out, execution jumps to the
failwr label. At this point, cqspi_tune_phy(..., false) is called before 
the active indirect write is cancelled via CQSPI_REG_INDIRECTWR_CANCEL_MASK.

Since the controller is still actively waiting for data, does the mandatory 
cqspi_wait_idle() call inside the PHY disable sequence also time out, 
stalling the CPU for another 500ms?

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

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