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

[Michael Walle]

On Tue Mar 5, 2024 at 4:37 PM CET, Marek Vasut wrote:
> On 3/5/24 1:50 PM, Michael Walle wrote:
> > Hi Marek,
>
> Hi,
>
> > 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").

-michael

> > This should be either be a board level option or maybe expose some
> > command line interface to let the user change the settings.
> > 
> >>> Thus,
> >>> there is likely someone, probably the manufacturer of the board,
> >>> who intentionally set this bit. Now, with this patch it will get
> >>> disabled *unconditionally*, forever.
> >>
> >> In my case, it is exactly the opposite, the SR3 WPS shouldn't have been
> >> set and the device should not have been locked, but it was and that
> >> confused both U-Boot and Linux.
> >>
> >> I would argue that if the board manufacturer intention was to lock the
> >> SPI NOR, they would have had multiple better options at their disposal,
> >> and those options should have been aligned with the software support:
> >> - nWP pin of the SPI NOR could be tied low to enable WRITE PROTECT
> >> - OTP bits could have been programmed to enable permanent WRITE PROTECT
> >> - standard SPI NOR locking scheme could have been used too
> >>
> >> If they did set SR3 WPS and hoped that U-Boot or Linux would fail to
> >> unlock the SPI NOR using standard locking scheme commands, that is I
> >> think broken design.
> > 
> > There might be software/OS which could handle that correctly. Also,
> > if linux will ever learn to use the new locking scheme
> > unconditionally, u-boot will always mess it up then.
>
> See CONFIG_SPI_FLASH_UNLOCK_ALL above.

-------------- 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/4ba5cbfc/attachment.sig>