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

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: 2026年7月27日 17:17
> To: Bough Chen (OSS) <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: 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?

Yes, correct, this should make the code more strong, I will fix it in another patch.

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

This is false positive, MTD layer already guarantee these four flags all be ture for 8D-8D-8D.


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

Yes, should be an issue here, will fix it in another patch.

Regards
Haibo Chen
> 
> --
> Sashiko AI
> review ・ https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Fsashiko.dev%2F%23%2Fpatchset%2F20260727-fspi-clock-v1-1-f4f3be6c3
> 66d%40nxp.com%3Fpart%3D1&data=05%7C02%7Chaibo.chen%40nxp.com%
> 7Ceb50244b6f504bf7260f08deebc0292d%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C639207407710774374%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIk
> FOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=WY6UItpXvk6y
> wF9rfQgFebt4e%2BsIpcqKt261dyGXiiY%3D&reserved=0