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

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

[-- Attachment #1: Type: text/plain, Size: 5807 bytes --]

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?

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

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]