Message-ID: <CAPB_pELazUPccKa72_m7vb80Z7wLRO+PpgfeY-afDHSgg4eVNg@mail.gmail.com>
Date: Wed, 24 Sep 2025 11:00:02 +0200
From: Maarten Zanders <maarten@...ders.be>
To: Michael Walle <mwalle@...nel.org>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>, Pratyush Yadav <pratyush@...nel.org>, 
	Miquel Raynal <miquel.raynal@...tlin.com>, Richard Weinberger <richard@....at>, 
	Vignesh Raghavendra <vigneshr@...com>, Boris Brezillon <bbrezillon@...nel.org>, linux-mtd@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, chengminglin@...c.com.tw
Subject: Re: [PATCH 2/2] mtd: spi-nor: macronix: use RDCR opcode 0x15

Hi Michael,

> Why isn't that also true for this device? It supports SFDP. Does it
> have a wrong value there?

You're right. I started working on this issue in an older kernel and
didn't check the full error path again on the most recent version. I
noted that the CR opcode was still wrong and went ahead forward
porting my patches without checking the erroneous behavior in the
latest kernel. My bad!

My particular part (MX25L12833F) has been working (by doing 8 bit SR
writes) since 947c86e481a0 ("mtd: spi-nor: macronix: Drop the
redundant flash info fields", 2025-04-07). This ensures that SFDP data
is read and behavior after that is OK. Before that commit, the SFDP
data wouldn't be read because the .size was filled in (and before that
because of .no_sfdp_flags). That in turn triggered the 16 bit writes.

> But I'm also not convinced that we should fix it that way. I just
> had a look at a random macronix flash MX25L12805D and it doesn't
> have that opcode. Thus, just adding that to all the macronix flashes
> doesn't make much sense. But it also doesn't seem to have a WRSR
> command that takes 16bits.. and the core unconditonally set
> SNOR_F_HAS_16BIT_SR. Hum.

Yes. That part (MX25L12805D) has the same ID code whilst it is not
supporting SFDP, RDCR or 16 bit SR writes (according to the
datasheet).
With the current flash info & logic in core.c, it will no longer work
at all as spi_nor_parse_sfdp() fails.

Consider a different example: 8M devices MX25L6433F, MX25L6436F and
MX25L6473F. The ID for these is 0xC22017. Flash info for this contains
a .size field (probably because of the legacy MX25L6405D) so SFDP will
not be parsed and we're falling back on the defaults - so it will do
16 bit SR writes. CR will get corrupted due to wrong CR read opcode.

So I believe this first problem boils down to the same ID representing
both flashes with and without SFDP. If we want to keep supporting the
old non-SFDP devices, the .size should be filled in for those ID's. Or
we drop support for them altogether and make SFDP a hard requirement
(solving the other issues in one go). But it should be consistent
across the different sizes.

> So maybe just clear the SNOR_F_HAS_16BIT_SR or add SNOR_F_NO_READ_CR
> for the macronix flashes by default as a fix. Not sure what's better
> here.

SNOR_F_NO_READ_CR doesn't help: this will write all 0's to the CR in a
16 bit SR write, which is not the default state of some parts
mentioned earlier.
Clearing SNOR_F_HAS_16BIT_SR could indeed be a solution for letting
these parts work properly in this non-sfdp mode. But we probably
shouldn't do it for *all* Macronix flashes?

> Then on top of that you might add the RDCR opcode, although
> I'm not sure for what it is used then.

There wouldn't be a real use until someone starts actually
implementing the features in the Macronix CR (like top/bottom SWP). Or
untill someone else is changing SNOR_F_HAS_16BIT_SR logic due to
additional SFDP/BFPT parsing. Which I still consider a risk due to the
weak link.

> > Fixes: 10526d85e4c6 ("mtd: spi-nor: Move Macronix bits out of core.c")
>
> I doubt that this is the correct Fixes tag as this only moves code
> around.

Essentially, I meant 'since the beginning of macronix introduction'.
In such a case, should we dig further through file renames & stale
LTS's?

Thanks for your input,
Maarten

>
> In any case, there seems to be another issue with your flash and the
> SFDP tables.
>
> -michael
>
> [1] https://docs.kernel.org/driver-api/mtd/spi-nor.html
>
> > Signed-off-by: Maarten Zanders <maarten@...ders.be>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 1 +
> >  include/linux/mtd/spi-nor.h    | 3 +++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > index e97f5cbd9aad..de3f3d963f86 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -322,6 +322,7 @@ static int macronix_nor_late_init(struct spi_nor *nor)
> >       if (!nor->params->set_4byte_addr_mode)
> >               nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b;
> >       nor->params->set_octal_dtr = macronix_nor_set_octal_dtr;
> > +     nor->params->rdcr_opcode = SPINOR_OP_RDCR_MX;
> >
> >       return 0;
> >  }
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index cdcfe0fd2e7d..e35405b126c2 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -92,6 +92,9 @@
> >  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
> >  #define SPINOR_OP_WD_EVCR      0x61    /* Write EVCR register */
> >
> > +/* Used for Macronix flashes only. */
> > +#define SPINOR_OP_RDCR_MX    0x15    /* Read configuration register */
> > +
> >  /* Used for GigaDevices and Winbond flashes. */
> >  #define SPINOR_OP_ESECR              0x44    /* Erase Security registers */
> >  #define SPINOR_OP_PSECR              0x42    /* Program Security registers */
>

Powered by blists - more mailing lists