Re: [PATCH v2 2/2] mtd: spi-nor: issi: Add support for is25wx01g

[Nuno Sá]

On Fri, Sep 18, 2026 at 11:07:10AM +0100, Nuno Sá wrote:
> On Fri, Sep 18, 2026 at 11:25:54AM +0200, Michael Walle wrote:
> > On Wed Sep 16, 2026 at 10:44 AM CEST, Nuno Sá wrote:
> > > On Wed, Sep 16, 2026 at 09:14:52AM +0200, Michael Walle wrote:
> > >> On Mon Sep 14, 2026 at 5:31 PM CEST, Nuno Sá wrote:
> > >> > On Mon, Sep 14, 2026 at 04:04:40PM +0200, Michael Walle wrote:
> > >> >> On Mon Sep 14, 2026 at 3:42 PM CEST, Nuno Sá wrote:
> > >> >> > (*): I should note that the command actually failed with -EIO but it
> > >> >> > actually unlocked the chip! And the reason is because the flash as the same
> > >> >> > FSR register than the micron-st flash. So WEL is set to 1 but can only
> > >> >> > be cleared when clearing the FSR register.
> > >> >> 
> > >> >> Why doesn't this affect only the locking operation? WEL polling is
> > >> >> used also during write and erase.
> > >> 
> > >> Sorry I meant WIP.
> > >> 
> > >> > Not sure if I fully understand. But AFAICT, the reason why erase and
> > >> > write is silent is because the default spi_nor_sr_ready() only looks at
> > >> > SR_WIP [1]:
> > >> 
> > >> So usually, the WEL is cleared automatically by whatever needs the
> > >> WEL in the first place, i.e. program or erase, write (status)
> > >> register. That should also be the case for this flash.
> > >> 
> > >> Now for this flash (as well as the st/micron ones), there is one
> > >> peculiarity. Whenever there is an error bit set in the FSR, the WEL
> > >> cannot be cleared by a write disable command. Which we shouldn't
> > >> need anyway because it should be cleared automatically if nothing
> > >> goes wrong.
> > >> 
> > >> Also the write status register won't set the error bits if i read
> > >> the datasheet correctly and it will always disable the WEL, see
> > >> Table 29 ("WRITE REGISTER Operations") in the MT35XU512ABA datasheet
> > >> and Table 8,8 ("WRITE REGISTER Operstaions") in the IS25WX01G
> > >> datasheet.
> > >> 
> > >> > OTOH, on the unlock path we do spi_nor_write_sr1_and_sr2_and_check() and
> > >> > give no special handling to WEL so I imagine that we try to set it as 0
> > >> > but read it as 1 (given that it clears only with FSR) and hence I got
> > >> > the -EIO in [2].
> > >> 
> > >> We do a RMW, so my guess is that it's the other way around. We read
> > >> it as 1, but then after writing the SR, it's 0 (see above). That
> > >> actually assumes, that if the WEL and any error bit in the FSR is
> > >> set, a write status register will clear the WEL anyways. Could you
> > >> debug that so we are sure, this is what actually happens?
> > >
> > > Sure I'll do some debugging on the unlock path. The DS seems a bit
> > > unclear. It also states (for the WRITE DISABLE)
> > >
> > > "...In case of a protection error, WRITE DISABLE will not
> > > clear the bit. Instead, a CLEAR FLAG STATUS REGISTER command must be issued to
> > > clear both flags.
> > > "
> > 
> > Not sure, this contradicts each other. As I read it:
> > 
> >  - Write (status) register will always clear a WEL, the only open
> >    question is, does it also clear it if the protection bit in the
> >    FSR is set
> >  - Write disable won't clear the WEL if the protection bit in the
> >    FSR is set.
> > 
> > > But the truth is that the second unlock I did came without an error.
> > 
> > Which might indicate that a write status will clear the WEL anyway.
> > But then it might also be interesting to see if the PROT bit in FSR
> > is still set. IOW, if a new write enable is sent, a write disable
> > might fail even if there was no actual error.
> > 
> > >
> > >> 
> > >> But the question is who is setting the error bit in the first place.
> > >> And I guess it's the testing sequence for the locking when you try
> > >> to write to a locked range. So you could also actually test the
> > >> locking/unlocking without writing any data to the flash just to see
> > >> if that is the case.
> > >
> > > Pretty sure the above is the case! If you look at other tests after
> > >
> > > "Once we trust the debugfs output we can use it to test various
> > > situations. Check top locking/unlocking (end of the device):"
> > >
> > > Everything worked nicely given we were just doing lock/unlock. The DS is
> > > also clear about this (table 8.11):
> > >
> > > "...When a command is applied to a protected sector, the command is not executed,
> > > the write enable latch bit remains set to 1, and flag status register bits 1 and 4 are set. 
> > > If the operation
> > > "
> > 
> > Ok.
> > 
> > > I also did tested with basically the same code as in micron-st and then
> > > ERASE and PROGRAM commands just return -EIO.
> > 
> > But the unlocking does not return EIO anymore when it's executed
> > successfully?
> 
> Unlock never return EIO because we do micron_st_nor_clear_fsr() followed by
> spi_nor_write_disable() which should clear WEN. So the error is reported
> on the call it should be reported IMO.
> 
> > 
> > >> >> > AFAICT, we should do something similar as micron so the writing to an
> > >> >> > actual protected region fails rather than being silently discarded with
> > >> >> > that status bit set. The question would be how to do it? The code is
> > >> >> > pretty much identical to [1]. The masks, the opcoded... So should we
> > >> >> > somehow handle this in the core (by having some common helper) that
> > >> >> > could be set in .late_init() under a common MFR_FSR flag? Or just keep
> > >> >> > both implementations separate for now?
> > >> >> 
> > >> >> I'd like to keep that out of the core.c, but also like to avoid any
> > >> >> code duplication esp. because there is already handling for the
> > >> >> intel spi controller in there. So maybe move it it into a new
> > >> >> common.c.
> > >> >
> > >> > Also don't like the dup tbh. Could that be a follow up or should it be
> > >> > v3. From the top of my head I could think on a mfr_common.c kind of thing.
> > >> > Don't thing this FSR register is standard?
> > >> 
> > >> Not really.
> > >> 
> > >> But (at least) parts of the datasheets are actually copied verbatim
> > >> between micron and issi, I wonder if we shouldn't just put the ISSI
> > >> part in micron-st.c. (Yes vendor will be wrong, but I plan on
> > >> deprecating that sysfs property anyway).
> > >
> > > Also works for me. Say the word and I can send v3 with this in
> > > micron-st.c.
> > 
> > Yes. But also please verify the our guesses about the root cause of
> > this and what's the actual behavior of the write disable.
> > 
> 
> Ok

Alright! I finally got the time to some tests and the unlock error is
actually as you expected:

flash_lock -l /dev/mtd4
dd if=/dev/urandom of=./spi_test2 bs=1M count=2
mtd_debug write /dev/mtd4 0 2097152 spi_test2
flash_lock -u /dev/mtd4
[ 3147.464690] spi-nor spi6.0: SR: 0x5e -> 0x02
[ 3147.469699] spi-nor spi6.0: SR1: read back test (0x00 != 0x02)
flash_lock: error!: could not unlock device: /dev/mtd4

            error 5 (Input/output error)

So yeah, the write enable is left as 1 and we try to write it but read
back 0 (so write_status does cleans it).

The below logs also reply to your FSR bits question:

[  155.737250] spi-nor spi6.0: Reading FSR (0x92) before write SR
[  155.738385] spi-nor spi6.0: Reading FSR (0x92) after write SR

Now, one interesting thing I found is that the datasheet is not very
accurate because the below test passes just fine:

flash_lock -l /dev/mtd4
mtd_debug erase /dev/mtd4 0 2097152
flash_lock -u /dev/mtd4

Debugging the above I found out that write disable actually clears the
write enable bit. And given that spi_nor_erase() calls
spi_nor_write_disable() we do not see -EIO in unlock.

As for FSR, they remain set until we clear them.

So, what would be your preference? Some common.c (or mfr_common.c) with
the issi/micron shared code or should I just move the new issi part to
micron-st.c and use the code already there?

- Nuno Sá

> 
> - Nuno Sá
> 
> > -michael
> 
> 
> 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/