Re: [PATCH v2 1/3] 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but clock configuration failures in `nxp_fspi_select_mem()` and `nxp_fspi_default_setup()` are silently ignored, leaving the controller unclocked and leading to system crashes upon subsequent register accesses.
--

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

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

This commit corrects and updates the maximum clock rates for SDR and DTR
modes across various supported NXP SoCs. It adds max_sdr_rate and
max_dtr_rate fields to the devtype data and updates the runtime clock
rate selection to respect these SoC-specific limits.

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c

[ ... ]

> @@ -691,10 +757,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);
> -		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 */
>  		reg &= ~FSPI_MCR0_RXCLKSRC(3);
> -		f->max_rate = 66000000;
> +		/*
> +		 * Use the SoC-specific SDR max rate if provided, otherwise
> +		 * fall back to 66MHz (limit from IMX8MN datasheet §3.9.9).
> +		 */
> +		f->max_rate = f->devtype_data->max_sdr_rate ?
> +			      f->devtype_data->max_sdr_rate : 66000000;

[Severity: High]
This is a pre-existing issue, but I noticed that changing the maximum rate
limit could potentially expose a silent failure path if the clock framework
returns an error for these newly configured max limits.

In nxp_fspi_select_mem(), the clock is disabled before configuring the
rate. If clk_set_rate() or nxp_fspi_clk_prep_enable() fails, the function
returns early without propagating the error:

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;
    ...
}

Since nxp_fspi_select_mem() returns void, its caller nxp_fspi_exec_op()
continues executing and accesses registers in nxp_fspi_prepare_lut(), even
though the clock is left disabled:

drivers/spi/spi-nxp-fspi.c:nxp_fspi_exec_op() {
    ...
    nxp_fspi_select_mem(f, mem->spi, op);

    nxp_fspi_prepare_lut(f, op);
    ...
}

Could this unclocked IP block access lead to a system crash (such as an
external abort or kernel panic)?

Additionally, nxp_fspi_probe() seems to completely ignore the return value
of nxp_fspi_default_setup():

drivers/spi/spi-nxp-fspi.c:nxp_fspi_probe() {
    ...
    nxp_fspi_default_setup(f);

    ret = pm_runtime_put_sync(dev);
    ...
}

Should these errors be propagated up the call chain to ensure we avoid
accessing memory-mapped registers when the clock fails to initialize or
enable?

>  	}
>  	fspi_writel(f, reg, f->iobase + FSPI_MCR0);
>  }

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