Re: [PATCH] spi: spi-nxp-fspi: add per-SoC SDR/DTR clock rate limits for all supported SoCs

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

Pre-existing issues:
- [High] Missing clock re-enable on error path in `nxp_fspi_select_mem` leads to synchronous aborts.
- [High] FlexSPI `MCR0` register is modified while the module is enabled.
- [High] Flawed detection of DTR mode for operations missing optional phases (e.g., commands with no data).
--

--- Patch [1]: [PATCH] spi: spi-nxp-fspi: add per-SoC SDR/DTR clock rate limits for all supported SoCs ---
commit 45fd7def019be2b4fb5ec79c57cfe9cfd93dcfb4
Author: Haibo Chen <[email protected]>

spi: spi-nxp-fspi: add per-SoC SDR/DTR clock rate limits for all supported SoCs

This patch introduces per-SoC clock rate limits for both SDR and DTR modes
by adding max_sdr_rate and max_dtr_rate to nxp_fspi_devtype_data. It updates
nxp_fspi_select_rx_sample_clk_source() to use these SoC-specific limits instead
of a global 166MHz cap.

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index d94a2a7b98d44..1159f5fc8a3a6 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c

[ ... ]

> @@ -691,10 +755,20 @@ static void nxp_fspi_select_rx_sample_clk_source(struct nxp_fspi *f,
>  	reg = fspi_readl(f, f->iobase + FSPI_MCR0);
>  	if (op_is_dtr) {
>  		reg |= FSPI_MCR0_RXCLKSRC(3);

[Severity: High]
This is a pre-existing issue, but modifying the FlexSPI MCR0 register while
the module is enabled can lead to unpredictable IP behavior. 

Does this code need to assert the MDIS bit (disable the module) before
modifying MCR0, as hardware state machines might ignore register writes or
corrupt their internal state if changed on the fly?

> -		f->max_rate = 166000000;
> +		/*
> +		 * Use the SoC-specific DTR max rate if provided, otherwise
> +		 * fall back to 166MHz (limit from IMX8MN datasheet §3.9.9).
> +		 */
> +		f->max_rate = f->devtype_data->max_dtr_rate ?
> +			      f->devtype_data->max_dtr_rate : 166000000;
>  	} else {	/*select mode 0 */

[ ... ]


While reviewing this patch, I noticed a couple of other pre-existing issues
in nxp_fspi_select_mem() within the same file:

drivers/spi/spi-nxp-fspi.c:nxp_fspi_select_mem() {
    ...
	/* flexspi only support one DTR mode: 8D-8D-8D */
	bool op_is_dtr = op->cmd.dtr && op->addr.dtr && op->dummy.dtr && op->data.dtr;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this boolean check strictly require
all four phases to have the DTR flag set?

For an 8D-8D-8D command missing an optional phase (e.g., Read Status Register
missing an address phase), spi-mem sets nbytes=0 and dtr=0 for that phase.
This would evaluate to false, causing valid DTR operations to fall back to
SDR sample clock and half the target frequency.

drivers/spi/spi-nxp-fspi.c:nxp_fspi_select_mem() {
    ...
	nxp_fspi_clk_disable_unprep(f);

	ret = clk_set_rate(f->clk, rate);
	if (ret)
		return;

	ret = nxp_fspi_clk_prep_enable(f);
	if (ret)
		return;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code leave the FlexSPI clock
disabled on the error path?

If clk_set_rate() or nxp_fspi_clk_prep_enable() fails, the function returns
early without restoring the clock.

Since it returns void, the caller is unaware of the failure and might
immediately access hardware registers, which could lead to synchronous
external aborts.

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