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

["Nuno Sá" <nuno.sa@analog.com>]

On Wed, Sep 16, 2026 at 09:14:52AM +0200, Michael Walle wrote:
> Hi,
> 
> 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.
"

But the truth is that the second unlock I did came without an 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
"

I also did tested with basically the same code as in micron-st and then
ERASE and PROGRAM commands just return -EIO.

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

- Nuno Sá

> 
> -michael