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

["Michael Walle" <mwalle@kernel.org>]

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?

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.

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

-michael