| Message ID | 20240304161607.82759-1-marex@denx.de |
|---|---|
| State | New |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: spi-nor: Clear Winbond SR3 WPS bit on boot | expand |
[+ linux-mtd ] Hi Marek, 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. 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. -michael > W25Q16DW, but the W25Q16DW does not implement the SR3 WPS bit. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Hai Pham <hai.pham.ud@renesas.com> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Johann Neuhauser <jneuhauser@dh-electronics.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > Cc: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com> > Cc: Vignesh R <vigneshr@ti.com> > --- > drivers/mtd/spi/spi-nor-core.c | 47 ++++++++++++++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 5 ++++ > 2 files changed, 52 insertions(+) > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > index 9a1801ba93d..68dee57258d 100644 > --- a/drivers/mtd/spi/spi-nor-core.c > +++ b/drivers/mtd/spi/spi-nor-core.c > @@ -544,6 +544,24 @@ static int read_cr(struct spi_nor *nor) > } > #endif > > +/** > + * read_sr3() - Read status register 3 unique to newer Winbond flashes > + * @nor: pointer to a 'struct spi_nor' > + */ > +static int read_sr3(struct spi_nor *nor) > +{ > + int ret; > + u8 val; > + > + ret = nor->read_reg(nor, SPINOR_OP_RDSR3, &val, 1); > + if (ret < 0) { > + dev_dbg(nor->dev, "error %d reading SR3\n", ret); > + return ret; > + } > + > + return val; > +} > + > /* > * Write status register 1 byte > * Returns negative if error occurred. > @@ -554,6 +572,17 @@ static int write_sr(struct spi_nor *nor, u8 val) > return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1); > } > > +/** > + * write_sr3() - Write status register 3 unique to newer Winbond flashes > + * @nor: pointer to a 'struct spi_nor' > + * @val: value to be written into SR3 > + */ > +static int write_sr3(struct spi_nor *nor, u8 val) > +{ > + nor->cmd_buf[0] = val; > + return nor->write_reg(nor, SPINOR_OP_WRSR3, nor->cmd_buf, 1); > +} > + > /* > * Set write enable latch with Write Enable command. > * Returns negative if error occurred. > @@ -3890,6 +3919,24 @@ static int spi_nor_init(struct spi_nor *nor) > write_enable(nor); > write_sr(nor, 0); > spi_nor_wait_till_ready(nor); > + > + /* > + * 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. > + */ > + if (JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) { > + err = read_sr3(nor); > + if (err > 0 && err & SR3_WPS) { > + write_enable(nor); > + write_sr3(nor, err & ~SR3_WPS); > + write_disable(nor); > + } > + } > } > > if (nor->quad_enable) { > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 2861b73edbc..ceb32e3906f 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -45,6 +45,8 @@ > #define SPINOR_OP_WRSR 0x01 /* Write status register 1 byte */ > #define SPINOR_OP_RDSR2 0x3f /* Read status register 2 */ > #define SPINOR_OP_WRSR2 0x3e /* Write status register 2 */ > +#define SPINOR_OP_RDSR3 0x15 /* Read status register 3 */ > +#define SPINOR_OP_WRSR3 0x11 /* Write status register 3 */ > #define SPINOR_OP_READ 0x03 /* Read data bytes (low frequency) */ > #define SPINOR_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ > #define SPINOR_OP_READ_1_1_2 0x3b /* Read data bytes (Dual Output SPI) */ > @@ -185,6 +187,9 @@ > /* Status Register 2 bits. */ > #define SR2_QUAD_EN_BIT7 BIT(7) > > +/* Status Register 3 bits. */ > +#define SR3_WPS BIT(2) > + > /* For Cypress flash. */ > #define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */ > #define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
On 3/5/24 9:55 AM, Michael Walle wrote: > [+ linux-mtd ] > > Hi Marek, Hi, > 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. 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. > 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.
Hi Marek, 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. > 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. 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. -michael
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.
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.
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: >>> 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"). So, how shall we proceed here? 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. 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.
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
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. > 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 . In other words, there should be no writes into the non-volatile bits, unless U-Boot and Linux disagree on the locking scheme, in which case writes are unavoidable (if you want unlock to work in both projects). > 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). >> 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. That is why I wrote that it is technically possible, but probably not a good idea because it is inconsistent and therefore error prone.
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? > > 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. > 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". > > 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 :) -michael > >> 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. > > That is why I wrote that it is technically possible, but probably not a > good idea because it is inconsistent and therefore error prone.
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.
Hi, On Wed Mar 6, 2024 at 3:56 AM CET, Marek Vasut wrote: > > 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. Yes, that's what I've meant with "the unlock all works as advertised" and your patch is fine. -michael
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 9a1801ba93d..68dee57258d 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -544,6 +544,24 @@ static int read_cr(struct spi_nor *nor) } #endif +/** + * read_sr3() - Read status register 3 unique to newer Winbond flashes + * @nor: pointer to a 'struct spi_nor' + */ +static int read_sr3(struct spi_nor *nor) +{ + int ret; + u8 val; + + ret = nor->read_reg(nor, SPINOR_OP_RDSR3, &val, 1); + if (ret < 0) { + dev_dbg(nor->dev, "error %d reading SR3\n", ret); + return ret; + } + + return val; +} + /* * Write status register 1 byte * Returns negative if error occurred. @@ -554,6 +572,17 @@ static int write_sr(struct spi_nor *nor, u8 val) return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1); } +/** + * write_sr3() - Write status register 3 unique to newer Winbond flashes + * @nor: pointer to a 'struct spi_nor' + * @val: value to be written into SR3 + */ +static int write_sr3(struct spi_nor *nor, u8 val) +{ + nor->cmd_buf[0] = val; + return nor->write_reg(nor, SPINOR_OP_WRSR3, nor->cmd_buf, 1); +} + /* * Set write enable latch with Write Enable command. * Returns negative if error occurred. @@ -3890,6 +3919,24 @@ static int spi_nor_init(struct spi_nor *nor) write_enable(nor); write_sr(nor, 0); spi_nor_wait_till_ready(nor); + + /* + * 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. + */ + if (JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) { + err = read_sr3(nor); + if (err > 0 && err & SR3_WPS) { + write_enable(nor); + write_sr3(nor, err & ~SR3_WPS); + write_disable(nor); + } + } } if (nor->quad_enable) { diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 2861b73edbc..ceb32e3906f 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -45,6 +45,8 @@ #define SPINOR_OP_WRSR 0x01 /* Write status register 1 byte */ #define SPINOR_OP_RDSR2 0x3f /* Read status register 2 */ #define SPINOR_OP_WRSR2 0x3e /* Write status register 2 */ +#define SPINOR_OP_RDSR3 0x15 /* Read status register 3 */ +#define SPINOR_OP_WRSR3 0x11 /* Write status register 3 */ #define SPINOR_OP_READ 0x03 /* Read data bytes (low frequency) */ #define SPINOR_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ #define SPINOR_OP_READ_1_1_2 0x3b /* Read data bytes (Dual Output SPI) */ @@ -185,6 +187,9 @@ /* Status Register 2 bits. */ #define SR2_QUAD_EN_BIT7 BIT(7) +/* Status Register 3 bits. */ +#define SR3_WPS BIT(2) + /* For Cypress flash. */ #define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */ #define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
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. This SR3 WPS appears e.g. on W25Q16FW which has the same ID as W25Q16DW, but the W25Q16DW does not implement the SR3 WPS bit. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Hai Pham <hai.pham.ud@renesas.com> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Johann Neuhauser <jneuhauser@dh-electronics.com> Cc: Simon Glass <sjg@chromium.org> Cc: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> Cc: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com> Cc: Vignesh R <vigneshr@ti.com> --- drivers/mtd/spi/spi-nor-core.c | 47 ++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 5 ++++ 2 files changed, 52 insertions(+)