Re: [PATCH] mtd: spinand: Enable QE on all dies

On 26/08/10 09:58AM, Miquel Raynal wrote:
> Hello Han,
> 
> On 07/08/2026 at 18:11:27 -05, [email protected] wrote:
> 
> > From: Han Xu <[email protected]>
> >
> > The QUAD ENABLE (QE) bit is stored in a per-die configuration
> > register on some SPI-NAND devices. When a device contains multiple
> > dies, updating the QE bit only on the currently selected die can
> > leave the remaining dies operating in non-quad mode.
> >  
> > Iterate over all targets and update the QE setting on each die
> > during initialization to ensure consistent quad I/O operation
> > across the entire device.
> >
> > Tested on ISSI IS38SMW04G8B.
> >
> > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support
> > SPI NANDs")
> 
> There was no ISSI device back then, but I guess this can be useful for
> other devices. However this shall be backported, so Cc: stable.

It should be a common issue, I will CC stable in v2 and try to upstream ISSI
drivers.

> 
> > Signed-off-by: Han Xu <[email protected]>
> > ---
> >  drivers/mtd/nand/spi/core.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 35365b67dd8e..744662533f37 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -281,8 +281,26 @@ static int spinand_init_cfg_cache(struct spinand_device *spinand)
> >  static int spinand_init_quad_enable(struct spinand_device *spinand,
> >  				    bool enable)
> >  {
> > -	return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE,
> > -			       enable ? CFG_QUAD_ENABLE : 0);
> > +	struct nand_device *nand = spinand_to_nand(spinand);
> > +	unsigned int target;
> > +	int ret;
> > +
> > +	/*
> > +	 * QE is a per-die setting on some devices. Program each target
> > +	 * individually when enabling or disabling quad I/O mode.
> > +	 */
> > +	for (target = 0; target < nand->memorg.ntargets; target++) {
> > +		ret = spinand_select_target(spinand, target);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = spinand_upd_cfg(spinand, CFG_QUAD_ENABLE,
> > +				      enable ? CFG_QUAD_ENABLE : 0);
> > +		if (ret)
> > +			return ret;
> 
> If there is an issue in the middle of the loop, the chip will become
> unusable. Perhaps we should reset the dies to the !QE state and return
> an error to continue in degraded (single) mode if that happens?


Single-lane is the most basic op - if the QE set_feature write fails, the link
is broken and single-lane won't work anyway. And falling back needs the vendor's
own 1S op stored somewhere, but those variant tables are discarded after probe.
So it likely requires refactoring struct spinand_device to cache them. Given that,
maybe just keep the current return-error implementation.


> 
> Thanks,
> Miquèl