RE: [PATCH v2 2/3] spi: spi-nxp-fspi: enter stop mode before reconfiguring MCR0 and DLL

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: 2026年7月28日 18:23
> To: Bough Chen (OSS) <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH v2 2/3] spi: spi-nxp-fspi: enter stop mode before
> reconfiguring MCR0 and DLL
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] Error paths in nxp_fspi_select_mem() leave the FlexSPI module
> permanently in stop mode.
> --
> 
> --- Patch [2]: [PATCH v2 2/3] spi: spi-nxp-fspi: enter stop mode before
> reconfiguring MCR0 and DLL --- commit
> 4391ccb7c73c0489ac0b6f714a7073bb2b08b3c1
> Author: Haibo Chen <[email protected]>
> 
> spi: spi-nxp-fspi: enter stop mode before reconfiguring MCR0 and DLL
> 
> This patch modifies nxp_fspi_select_mem() to enter stop mode by setting
> MCR0[MDIS] before reconfiguring the RX sample clock source and DLL control
> registers. It then exits stop mode once the reconfiguration is complete
> according to the reference manual.
> 
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 6de6d432cdcb2..a426b8d92193a 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> [ ... ]
> > @@ -896,6 +897,15 @@ static void nxp_fspi_select_mem(struct nxp_fspi
> > *f, struct spi_device *spi,
> >
> >  	dev_dbg(f->dev, "Target device [CS:%x] selected\n",
> > spi_get_chipselect(spi, 0));
> >
> > +	/*
> > +	 * Per the FlexSPI reference manual (initialization sequence), MCR0 and
> > +	 * the DLL control registers should be configured while the module is in
> > +	 * stop mode (MCR0[MDIS] = 1). Enter stop mode before reconfiguring
> the
> > +	 * RX sample clock source and the DLL, then exit stop mode afterwards.
> > +	 */
> > +	reg = fspi_readl(f, f->iobase + FSPI_MCR0);
> > +	fspi_writel(f, reg | FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
> > +
> >  	nxp_fspi_select_rx_sample_clk_source(f, op_is_dtr);
> >  	rate = min(f->max_rate, op->max_freq);
> >
> [ ... ]
> > @@ -928,6 +938,10 @@ static void nxp_fspi_select_mem(struct nxp_fspi *f,
> struct spi_device *spi,
> >  	else
> >  		nxp_fspi_dll_override(f);
> >
> > +	/* Exit stop mode now that MCR0 and the DLL have been reconfigured.
> */
> > +	reg = fspi_readl(f, f->iobase + FSPI_MCR0);
> > +	fspi_writel(f, reg & ~FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
> > +
> 
> [Severity: High]
> Does this leave the FlexSPI module permanently in stop mode if the clock
> configuration fails earlier in this function?
> 
> If clk_set_rate() or nxp_fspi_clk_prep_enable() fails, the code returns early
> without executing this exit block:
> 
>         ret = clk_set_rate(f->clk, rate);
>         if (ret) {
>                 /* ... */
>                 nxp_fspi_clk_prep_enable(f);
>                 return ret;
>         }
> 
> Because the early returns happen before the stop mode is cleared, the
> hardware module is left disabled since FSPI_MCR0_MDIS remains set.
> 
> If this error occurs, a subsequent SPI transfer matching the previous
> successful state could hit the early-return fast path at the top of
> nxp_fspi_select_mem(), skip the hardware reconfiguration, and attempt to
> execute a transfer while the hardware is still disabled.

Reasonable, will fix in next version.

Regards
Haibo Chen
> 
> --
> Sashiko AI
> review ・ https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Fsashiko.dev%2F%23%2Fpatchset%2F20260728-fspi-clock-v2-0-dbe786a4
> a6eb%40nxp.com%3Fpart%3D2&data=05%7C02%7Chaibo.chen%40nxp.com
> %7C2385073e1cfe43f0d54e08deec951073%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C639208322149096149%7CUnknown%7CTWFpbGZ
> sb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIs
> IkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=y1ntFYWNNj
> wcvKdHaJuISdrZY5a1qjm0X2iQKb%2BefJw%3D&reserved=0