Message-Id: <DD10GE4EOCD7.CPTN7198QFUV@kernel.org>
Date: Wed, 24 Sep 2025 13:57:34 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: "Maarten Zanders" <maarten@...ders.be>
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 Maarten, Hi Cheng,

On Wed Sep 24, 2025 at 11:00 AM CEST, Maarten Zanders wrote:
> > 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.

A missing size only *mandates* a configuration by SFDP. But that's
not true the other way around. If there is a size, SFDP might still
be evaluated and will overwrite any static configuration. But for
your flash, that's not the case, because of legacy behavior this is
only done for flashes which are multi i/o, see
spi_nor_init_params_deprecated(). Sigh. What a mess.

But in any case, commit 947c86e481a0 ("mtd: spi-nor: macronix: Drop the
redundant flash info fields") is clearly wrong as it will drop
support for older flashes which doesn't feature SFDP. Cheng can you
look into that please?

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

Yes! I fully agree.

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

Yes, but again not because of the populated .size but because it
doesn't have any multi i/o flags set.

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

Honestly, Macronix is know for duplicating flash id with flashes
incompatible with each other. I have mixed feelings about reverting
the commit mentioned above. On one hand, it takes the very easy way
to just brush off support for older flashes without even mentioning
it. On the other hand, it seems that only Guenter Roeck noticed.

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

Mh? You've said:

  Other Macronix parts avoid this issue because their SFDP data
  specifies that CR is not read (BFPT_DWORD15_QER_SR2_BIT1_NO_RD),
  and the driver assumes CR defaults to all zeroes which matches the
  hardware register.

Also isn't that the same behavior as with the SFDP? But I agree,
that if the clearing the SNOR_F_HAS_16BIT_SR is probably better.

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

Well as I said it's a mess right now. Moving forward we should
probably have a static configuration and try to parse via SFDP even
on non-SFDP flashes. Pratyush, any opinions?

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

Probably, the patches won't be backported automatically anyway
because of the conflicts. But it might be a good argument to have a
(manual) backported fix.

-michael

Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)

Powered by blists - more mailing lists