[PATCH] mtd: spi-nor: Clear Winbond SR3 WPS bit on boot

[Michael Walle]

On Tue Mar 5, 2024 at 5:28 PM CET, Marek Vasut wrote:
> On 3/5/24 4:53 PM, Michael Walle wrote:
> > On Tue Mar 5, 2024 at 4:37 PM CET, Marek Vasut wrote:
> >> On 3/5/24 1:50 PM, Michael Walle wrote:
> >>> On Tue Mar 5, 2024 at 1:31 PM CET, Marek Vasut wrote:
> >>>> On 3/5/24 9:55 AM, Michael Walle wrote:
> >>>>> On Mon Mar 4, 2024 at 5:16 PM CET, Marek Vasut wrote:
> >>>>>> Some Winbond SPI NORs have special SR3 register which is
> >>>>>> used among other things to control whether non-standard
> >>>>>> "Individual Block/Sector Write Protection" (WPS bit)
> >>>>>> locking scheme is activated. This non-standard locking
> >>>>>> scheme is not supported by either U-Boot or Linux SPI
> >>>>>> NOR stack so make sure it is disabled, otherwise the
> >>>>>> SPI NOR may appear locked for no obvious reason.
> >>>>>
> >>>>> I don't think it is a good idea to just disable the WPS bit.
> >>>>> Usually, that bit is non-volatile and the default is not set.
> >>>>
> >>>> Yes, that's right, the bit is non-volatile and should not be set unless
> >>>> the MTD stack can handle it correctly. Currently, neither U-Boot nor
> >>>> Linux does handle the bit at all, instead both attempt to use the
> >>>> standard SPI NOR locking scheme which is also implemented by this SPI
> >>>> NOR model and they both fail to unlock the SPI NOR that way.
> >>>>
> >>>> Note that the SR3 WPS bit only switches between two different locking
> >>>> schemes (unset = standard SPI NOR locking scheme, set = custom winbond
> >>>> locking scheme), setting SR3 WPS does not immediately imply the SPI NOR
> >>>> is locked, rather the opposite. Out of manufacturing, the SPI NOR is
> >>>> unlocked in either locking scheme. Depending on the SR3 WPS bit state,
> >>>> different commands are used to lock and unlock the SPI NOR.
> >>>>
> >>>> I recently ran across a device which had the SR3 WPS bit incorrectly set
> >>>> out of manufacturing of that device (i.e. before it was populated on a
> >>>> board at board manufacturer) and the device was locked using the custom
> >>>> locking scheme. I was not able to unlock or use that device because both
> >>>> U-Boot and Linux tried to use the standard scheme for that purpose.
> >>>
> >>> Still, I don't think it makes any sense, to disable that bit for all
> >>> winbond flashes just because there was one vendor which set it the
> >>> wrong way - or the board manufacturer didn't test it and programmed
> >>> the flash correctly.
> >>
> >> OK
> >>
> >>>> Clearing this SR3 WPS bit fixes that problem, both in U-Boot and in
> >>>> Linux, since Linux that is booted afterward then gets a device that has
> >>>> locking scheme configured in a way that Linux expects and can operate.
> >>>>
> >>>> Better yet, if some old LTS version of the Linux kernel is in use, it
> >>>> will also work correctly, because this patch will configure the SPI NOR
> >>>> locking scheme to what Linux expects it to be, before the SPI NOR is
> >>>> handed over to that old kernel.
> >>>
> >>> Agreed, but it should *not* be done automatically and nor
> >>> unconditionally. Please don't just overwrite any locking bits just
> >>> because there is one flash which have it set.
> >>
> >> The unlock code is not triggered unconditionally, it is protected by
> >>
> >> if (IS_ENABLED(CONFIG_SPI_FLASH_UNLOCK_ALL)...
> >>
> >> Kconfig option, so this can be turned on/off in board config already.
> > 
> > Ahh, OK then :)
> > 
> > But keep in mind that enabling this option is foobar and I've gone
> > lengths to eliminate it in linux. And actually made that option in
> > u-boot.
> > 
> > See linux commit 31ad3eff093c ("mtd: spi-nor: keep lock bits if they
> > are non-volatile").
>
> So, how shall we proceed here?

Regarding this patch, I think it's fine. It will unlock the whole
flash as advertised.

But u-boot should really consider making that a "default n" option.
And most likely adding =y to every existing defconfig out there so
that at least new boards will benefit from it.

> The way I see this, if Linux ever implements this scheme, Linux can set 
> the SR3 WPS bit as needed, it does not matter which way bootloader sets 
> the bit as the protection bits are not cleared when the bit is cleared, 
> they seem to be stored elsewhere.

On each reboot you'd wear out that cell with two erases/writes.
That's another reason why that whole unlocking thing is foobar for
non-volatile bits. For me, non-volatile bits are for provisioning,
so during a normal boot they should not be touched at all. Just
during board manufacturing or because the user actively want to
protect something.

If you clear this bit during the unlock all command, all the locking
bits are cleared anyway. Or do you mean, the individual bits are
still retained?

> But, if Linux starts to use SR3 WPS, then U-Boot should be updated to 
> match, i.e. both projects should agree on the locking scheme, so that 
> there won't be a situation where on the same device, one project uses 
> one scheme, the other project uses another scheme. I think this would 
> technically work, but it would be horrible from the user perspective. 
> And if that happens, both projects should then be updated in lockstep 
> and this UNLOCK_ALL should be disabled for such a setup then.

If you don't touch it, I don't think you have a problem here. u-boot
and linux can support different schemes, as long as the user is
aware of that. I.e. if they want to lock a region and the flash is
configured in a mode which isn't supported in u-boot (or linux) it
should be rejected. There might of course be a command to switch the
scheme if someone want to do so.

-michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 252 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240305/5dc973f5/attachment.sig>