On 3/5/24 10:41 PM, Michael Walle wrote:
On Tue Mar 5, 2024 at 7:54 PM CET, Marek Vasut wrote:
On 3/5/24 5:55 PM, Michael Walle wrote:
[...]
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.
This change won't unlock the flash, it will switch to the supported
locking scheme, one which can then be used to unlock the flash.
I can't follow you here. The code is added right below the
write_sr(0), which will clear all the BP bits, thus unlocking
everything if WPS=0. Therefore, no locking will be enabled anymore
afterwards. What did I miss?
Ah, I think you didn't miss anything. Toggling the SR3 WPS does not
clear any lock bits (BP or page ones), it only selects the locking
scheme. The SR0 write does clear lock bits (BP ones) using the standard
locking scheme.
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.
Yes, I agree with that.
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.
That is what happens here, the write to clear the bit is triggered only
if the bit is set , so OK .
And if Linux wants to use the new locking scheme, then the bootloader
should ideally be updated to match, i.e. SPI_FLASH_UNLOCK_ALL=n etc, so
that is also OK .
I'd argue if one wants to use the locking at all, you have to set
UNLOCK_ALL=n. Otherwise, the bootloader might come alone and just
clear your locking bits again. Clearing the WPS bit there is just
one more thing which IMHO doesn't make much sense.
On the other hand, if UNLOCK_ALL=y is supposed to work and reset
locking, then the SR3 WPS bit has to be configured to select the
standard SPI NOR locking scheme, so the locking can actually be reset
using that scheme.
In other words, there should be no writes into the non-volatile bits,
unless U-Boot and Linux disagree on the locking scheme,
Agreed.
in which case
writes are unavoidable (if you want unlock to work in both projects).
But this should only happen if the user want to change the locking
at all. u-boot should not just do "oh this bit is set, I'm clearing
it" during SPI flash probe. Again, I'm not caring much if this is
guarded by the UNLOCK_ALL=y, because u-boot is already doing "oh the
BP bits are set, lets clear em".
It is guarded, yes.
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?
The lock bits themselves are retained, this SR3 WPS only selects which
of those bits take effect, whether the SR ones (standard locking scheme)
or the per-page ones (winbond custom locking scheme).
Ok. So switching back to WPS=1 might come with some suprises :)
Yes, the bad kind.
