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

[Marek Vasut]

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.

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