[PATCH v5 0/3] mtd: spi-nor: keep lock bits if they are non-volatile

[PATCH v5 0/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

I bundled this as a series, because otherwise there will be conflicts
because the "remove global protection flag" patches modify the same lines
as the main patch.

See invdividual patches for the version history.

Michael Walle (3):
  mtd: spi-nor: atmel: remove global protection flag
  mtd: spi-nor: sst: remove global protection flag
  mtd: spi-nor: keep lock bits if they are non-volatile

 drivers/mtd/spi-nor/Kconfig |  42 ++++++++++++
 drivers/mtd/spi-nor/atmel.c | 125 ++++++++++++++++++++++++++++++------
 drivers/mtd/spi-nor/core.c  |  36 +++++++----
 drivers/mtd/spi-nor/core.h  |   8 +++
 drivers/mtd/spi-nor/esmt.c  |   8 ++-
 drivers/mtd/spi-nor/intel.c |   6 +-
 drivers/mtd/spi-nor/sst.c   |  31 ++++-----
 7 files changed, 203 insertions(+), 53 deletions(-)

-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Michael Walle]

This is considered bad for the following reasons:
 (1) We only support the block protection with BPn bits for write
     protection. Not all Atmel parts support this.
 (2) Newly added flash chip will automatically inherit the "has
     locking" support and thus needs to explicitly tested. Better
     be opt-in instead of opt-out.
 (3) There are already supported flashes which doesn't support
     the locking scheme. So I assume this wasn't properly tested
     before adding that chip; which enforces my previous argument
     that locking support should be an opt-in.

Remove the global flag and add individual flags to all flashes which
supports BP locking. In particular the following flashes don't support
the BP scheme:
 - AT26F004
 - AT25SL321
 - AT45DB081D

Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK just
support Global Protection, i.e. not our supported block protection
locking scheme. This is to keep backwards compatibility with the
current "unlock all at boot" mechanism. In particular the following
flashes doesn't have BP bits:
 - AT25DF041A
 - AT25DF321
 - AT25DF321A
 - AT25DF641
 - AT26DF081A
 - AT26DF161A
 - AT26DF321

Signed-off-by: Michael Walle <michael@walle.cc>
---
changes since v4:
 - none

changes since v3/v2/v1:
 - there was no such version because this patch was bundled with another
   patch

changes since RFC:
 - mention the flashes which just support the "Global Unprotect" in the
   commit message

 drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
index 3f5f21a473a6..49d392c6c8bc 100644
--- a/drivers/mtd/spi-nor/atmel.c
+++ b/drivers/mtd/spi-nor/atmel.c
@@ -10,37 +10,27 @@
 
 static const struct flash_info atmel_parts[] = {
 	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
-	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
-	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
+	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },
 
-	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
-	{ "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
-	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
-	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
+	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
 
 	{ "at25sl321",	INFO(0x1f4216, 0, 64 * 1024, 64,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 
 	{ "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },
-	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K) },
-	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K) },
-	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K) },
+	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) },
 
 	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
 };
 
-static void atmel_default_init(struct spi_nor *nor)
-{
-	nor->flags |= SNOR_F_HAS_LOCK;
-}
-
-static const struct spi_nor_fixups atmel_fixups = {
-	.default_init = atmel_default_init,
-};
-
 const struct spi_nor_manufacturer spi_nor_atmel = {
 	.name = "atmel",
 	.parts = atmel_parts,
 	.nparts = ARRAY_SIZE(atmel_parts),
-	.fixups = &atmel_fixups,
 };
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Tudor.Ambarus]

On 10/3/20 6:32 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This is considered bad for the following reasons:
>  (1) We only support the block protection with BPn bits for write
>      protection. Not all Atmel parts support this.
>  (2) Newly added flash chip will automatically inherit the "has
>      locking" support and thus needs to explicitly tested. Better
>      be opt-in instead of opt-out.
>  (3) There are already supported flashes which doesn't support
>      the locking scheme. So I assume this wasn't properly tested
>      before adding that chip; which enforces my previous argument
>      that locking support should be an opt-in.
> 
> Remove the global flag and add individual flags to all flashes which
> supports BP locking. In particular the following flashes don't support
> the BP scheme:
>  - AT26F004
>  - AT25SL321
>  - AT45DB081D
> 
> Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK just
> support Global Protection, i.e. not our supported block protection
> locking scheme. This is to keep backwards compatibility with the
> current "unlock all at boot" mechanism. In particular the following
> flashes doesn't have BP bits:
>  - AT25DF041A
>  - AT25DF321
>  - AT25DF321A
>  - AT25DF641
>  - AT26DF081A
>  - AT26DF161A
>  - AT26DF321
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
> changes since v4:
>  - none
> 
> changes since v3/v2/v1:
>  - there was no such version because this patch was bundled with another
>    patch
> 
> changes since RFC:
>  - mention the flashes which just support the "Global Unprotect" in the
>    commit message
> 
>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
> index 3f5f21a473a6..49d392c6c8bc 100644
> --- a/drivers/mtd/spi-nor/atmel.c
> +++ b/drivers/mtd/spi-nor/atmel.c
> @@ -10,37 +10,27 @@
> 
>  static const struct flash_info atmel_parts[] = {
>         /* Atmel -- some are (confusingly) marketed as "DataFlash" */
> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/587164/ATMELCorporation/AT25FS010/1
BP bits are at bit 2, 3, 5 and 6.
BP0, BP1, BP3, BP4 and WPEN, are nonvolatile cells

> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/587165/ATMELCorporation/AT25FS040/1
BP bits are at bit 2, 3, 4, 5, and 6.
BP0, BP1, BP2, BP3, BP4 are nonvolatile cells

> 
> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/975331/Adesto/AT25DF041A/1
Global Protect/Unprotect using Write SR command:
Global Unlock: write 0x00 to SR
Global Lock: Read SR. If SR.SPRL is 1 write 0xff to SR, else write 0x7f.

Upon device power-up or after a device reset, each Sector Protection
Register will default to the logical “1” state indicating that all
sectors are protected and cannot be programmed or erased.


> +       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/609207/ATMELCorporation/AT25DF321/1
Global Protect/Unprotect same as in at25df041a.

> +       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/829669/Adesto/AT25DF321A/1
Global Protect/Unprotect same as in at25df041a.

> +       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },

https://www.adestotech.com/wp-content/uploads/doc3680.pdf
Global Protect/Unprotect same as in at25df041a.

> 
>         { "at25sl321",  INFO(0x1f4216, 0, 64 * 1024, 64,
>                              SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },

https://www.adestotech.com/wp-content/uploads/AT25SL321_112.pdf
Ok, just hw write protection.

> 
>         { "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },

https://cdn.sos.sk/productdata/08/5e/c7c8063e/at-26-f004-ssu.pdf
OK, never worked, just Individual Sector Protection for Program/Erase Protection

> -       { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K) },
> -       { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K) },
> -       { "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K) },
> +       { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_HAS_LOCK) },

https://www.adestotech.com/wp-content/uploads/doc3600.pdf
Global Protect/Unprotect same as in at25df041a.

> +       { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/562306/ATMELCorporation/AT26DF161/1
Global Protect/Unprotect same as in at25df041a.

> +       { "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/609208/ATMELCorporation/AT26DF321/1
Global Protect/Unprotect same as in at25df041a.

> 
>         { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },

https://datasheetspdf.com/pdf-file/856198/Adesto/AT45DB081D/1
OK. Individual sector protection.

>  };
> 
> -static void atmel_default_init(struct spi_nor *nor)
> -{
> -       nor->flags |= SNOR_F_HAS_LOCK;
> -}
> -
> -static const struct spi_nor_fixups atmel_fixups = {
> -       .default_init = atmel_default_init,
> -};
> -
>  const struct spi_nor_manufacturer spi_nor_atmel = {
>         .name = "atmel",
>         .parts = atmel_parts,
>         .nparts = ARRAY_SIZE(atmel_parts),
> -       .fixups = &atmel_fixups,
>  };
> --
> 2.20.1
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Michael Walle]

Am 2020-11-24 20:09, schrieb Tudor.Ambarus@microchip.com:
> On 10/3/20 6:32 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> This is considered bad for the following reasons:
>>  (1) We only support the block protection with BPn bits for write
>>      protection. Not all Atmel parts support this.
>>  (2) Newly added flash chip will automatically inherit the "has
>>      locking" support and thus needs to explicitly tested. Better
>>      be opt-in instead of opt-out.
>>  (3) There are already supported flashes which doesn't support
>>      the locking scheme. So I assume this wasn't properly tested
>>      before adding that chip; which enforces my previous argument
>>      that locking support should be an opt-in.
>> 
>> Remove the global flag and add individual flags to all flashes which
>> supports BP locking. In particular the following flashes don't support
>> the BP scheme:
>>  - AT26F004
>>  - AT25SL321
>>  - AT45DB081D
>> 
>> Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK 
>> just
>> support Global Protection, i.e. not our supported block protection
>> locking scheme. This is to keep backwards compatibility with the
>> current "unlock all at boot" mechanism. In particular the following
>> flashes doesn't have BP bits:
>>  - AT25DF041A
>>  - AT25DF321
>>  - AT25DF321A
>>  - AT25DF641
>>  - AT26DF081A
>>  - AT26DF161A
>>  - AT26DF321
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
>> ---
>> changes since v4:
>>  - none
>> 
>> changes since v3/v2/v1:
>>  - there was no such version because this patch was bundled with 
>> another
>>    patch
>> 
>> changes since RFC:
>>  - mention the flashes which just support the "Global Unprotect" in 
>> the
>>    commit message
>> 
>>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>> index 3f5f21a473a6..49d392c6c8bc 100644
>> --- a/drivers/mtd/spi-nor/atmel.c
>> +++ b/drivers/mtd/spi-nor/atmel.c
>> @@ -10,37 +10,27 @@
>> 
>>  static const struct flash_info atmel_parts[] = {
>>         /* Atmel -- some are (confusingly) marketed as "DataFlash" */
>> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
>> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
>> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
> 
> https://datasheetspdf.com/pdf-file/587164/ATMELCorporation/AT25FS010/1
> BP bits are at bit 2, 3, 5 and 6.
> BP0, BP1, BP3, BP4 and WPEN, are nonvolatile cells
> 
>> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
> 
> https://datasheetspdf.com/pdf-file/587165/ATMELCorporation/AT25FS040/1
> BP bits are at bit 2, 3, 4, 5, and 6.
> BP0, BP1, BP2, BP3, BP4 are nonvolatile cells
> 
>> 
>> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
>> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
>> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
>> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
>> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
> 
> https://datasheetspdf.com/pdf-file/975331/Adesto/AT25DF041A/1
> Global Protect/Unprotect using Write SR command:
> Global Unlock: write 0x00 to SR
> Global Lock: Read SR. If SR.SPRL is 1 write 0xff to SR, else write 
> 0x7f.

That is not my understanding. Quote:
   To perform a Global Protect, the appropriate WP pin and SPRL
   conditions must be met, and the system must write a logical “1”
   to bits 5, 4, 3, and 2 of the Status Register.

And
   Conversely, to per-form a Global Unprotect, the same WP and SPRL
   conditions must be met but the system must write a logical “0” to
   bits 5, 4, 3, and 2 of the Status Register

Keep in mind that bit 5, 4, 3 and 2 is exactly the
ATMEL_SR_GLOBAL_PROTECT_MASK. The SPRL bit is handled in the unlock()
function. Accoring to table 9.2 you also have to first disable the SPRL
bit and then write the BP bits to zero.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Tudor.Ambarus]

On 11/25/20 8:17 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2020-11-24 20:09, schrieb Tudor.Ambarus@microchip.com:
>> On 10/3/20 6:32 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> This is considered bad for the following reasons:
>>>  (1) We only support the block protection with BPn bits for write
>>>      protection. Not all Atmel parts support this.
>>>  (2) Newly added flash chip will automatically inherit the "has
>>>      locking" support and thus needs to explicitly tested. Better
>>>      be opt-in instead of opt-out.
>>>  (3) There are already supported flashes which doesn't support
>>>      the locking scheme. So I assume this wasn't properly tested
>>>      before adding that chip; which enforces my previous argument
>>>      that locking support should be an opt-in.
>>>
>>> Remove the global flag and add individual flags to all flashes which
>>> supports BP locking. In particular the following flashes don't support
>>> the BP scheme:
>>>  - AT26F004
>>>  - AT25SL321
>>>  - AT45DB081D
>>>
>>> Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK
>>> just
>>> support Global Protection, i.e. not our supported block protection
>>> locking scheme. This is to keep backwards compatibility with the
>>> current "unlock all at boot" mechanism. In particular the following
>>> flashes doesn't have BP bits:
>>>  - AT25DF041A
>>>  - AT25DF321
>>>  - AT25DF321A
>>>  - AT25DF641
>>>  - AT26DF081A
>>>  - AT26DF161A
>>>  - AT26DF321
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>>> ---
>>> changes since v4:
>>>  - none
>>>
>>> changes since v3/v2/v1:
>>>  - there was no such version because this patch was bundled with
>>> another
>>>    patch
>>>
>>> changes since RFC:
>>>  - mention the flashes which just support the "Global Unprotect" in
>>> the
>>>    commit message
>>>
>>>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>>> index 3f5f21a473a6..49d392c6c8bc 100644
>>> --- a/drivers/mtd/spi-nor/atmel.c
>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>> @@ -10,37 +10,27 @@
>>>
>>>  static const struct flash_info atmel_parts[] = {
>>>         /* Atmel -- some are (confusingly) marketed as "DataFlash" */
>>> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
>>> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
>>> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K |
>>> SPI_NOR_HAS_LOCK) },
>>
>> https://datasheetspdf.com/pdf-file/587164/ATMELCorporation/AT25FS010/1
>> BP bits are at bit 2, 3, 5 and 6.
>> BP0, BP1, BP3, BP4 and WPEN, are nonvolatile cells
>>
>>> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K |
>>> SPI_NOR_HAS_LOCK) },
>>
>> https://datasheetspdf.com/pdf-file/587165/ATMELCorporation/AT25FS040/1
>> BP bits are at bit 2, 3, 4, 5, and 6.
>> BP0, BP1, BP2, BP3, BP4 are nonvolatile cells
>>
>>>
>>> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
>>> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
>>> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
>>> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
>>> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K |
>>> SPI_NOR_HAS_LOCK) },
>>
>> https://datasheetspdf.com/pdf-file/975331/Adesto/AT25DF041A/1
>> Global Protect/Unprotect using Write SR command:
>> Global Unlock: write 0x00 to SR
>> Global Lock: Read SR. If SR.SPRL is 1 write 0xff to SR, else write
>> 0x7f.
> 
> That is not my understanding. Quote:
>   To perform a Global Protect, the appropriate WP pin and SPRL
>   conditions must be met, and the system must write a logical “1”
>   to bits 5, 4, 3, and 2 of the Status Register.
> 
> And
>   Conversely, to per-form a Global Unprotect, the same WP and SPRL
>   conditions must be met but the system must write a logical “0” to
>   bits 5, 4, 3, and 2 of the Status Register
> 

Right. I think we are both correct, and we should choose one method
or the other depending on the level of support we want to introduce.
If we want "locking ops", i.e. partial or full lock and unlock of the
flash, we'll go your way. If we want to keep things as they were before
3e0930f109e76, we'll just support the global unlock by writing 0x00 to SR.

Here's what I followed in the datasheet:
'''
Essentially, if the SPRL bit of the Status Register is in the logical “0”
state (Sector Protection Registers are not locked), then writing a 00h
to the Status Register will perform a Global Unprotect without changing
the state of the SPRL bit. Similarly, writing a 7Fh to the Status Register
will perform a Global Protect and keep the SPRL bit in the logical “0” state.
The SPRL bit can, of course, be changed to a logical “1” by writing an FFh
if software-locking or hardware-locking is desired along with the Global Protect.
'''

Also:
'''
If the desire is to only change the SPRL bit without performing a Global Protect
or Global Unprotect, then the system can simply write a 0Fh to the Status Register
to change the SPRL bit from a logical “1” to a logical “0” provided the WP pin is
deasserted.
'''

> Keep in mind that bit 5, 4, 3 and 2 is exactly the
> ATMEL_SR_GLOBAL_PROTECT_MASK. The SPRL bit is handled in the unlock()
> function. Accoring to table 9.2 you also have to first disable the SPRL
> bit and then write the BP bits to zero.

If SPRL is 1 and we want to unlock the entire flash, writing 0x00 to SR would not
suffice. We must set SPRL to zero first, i.e. write 0x0f to SR and then write 0x00
or set the BP bits to 0 in order to unlock all. Looks like spi_nor_sr_unlock()
does not treat SR_SRWD as it should.

Let me know what method you choose, I'll have to go again through the datasheets.
This time should be easier.

> 
> -michael

Thanks for the effort and patience.
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Tudor.Ambarus]

On 11/26/20 2:45 PM, Tudor Ambarus - M18064 wrote:
> On 11/25/20 8:17 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Am 2020-11-24 20:09, schrieb Tudor.Ambarus@microchip.com:
>>> On 10/3/20 6:32 PM, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>> the content is safe
>>>>
>>>> This is considered bad for the following reasons:
>>>>  (1) We only support the block protection with BPn bits for write
>>>>      protection. Not all Atmel parts support this.
>>>>  (2) Newly added flash chip will automatically inherit the "has
>>>>      locking" support and thus needs to explicitly tested. Better
>>>>      be opt-in instead of opt-out.
>>>>  (3) There are already supported flashes which doesn't support
>>>>      the locking scheme. So I assume this wasn't properly tested
>>>>      before adding that chip; which enforces my previous argument
>>>>      that locking support should be an opt-in.
>>>>
>>>> Remove the global flag and add individual flags to all flashes which
>>>> supports BP locking. In particular the following flashes don't support
>>>> the BP scheme:
>>>>  - AT26F004
>>>>  - AT25SL321
>>>>  - AT45DB081D
>>>>
>>>> Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK
>>>> just
>>>> support Global Protection, i.e. not our supported block protection
>>>> locking scheme. This is to keep backwards compatibility with the
>>>> current "unlock all at boot" mechanism. In particular the following
>>>> flashes doesn't have BP bits:
>>>>  - AT25DF041A
>>>>  - AT25DF321
>>>>  - AT25DF321A
>>>>  - AT25DF641
>>>>  - AT26DF081A
>>>>  - AT26DF161A
>>>>  - AT26DF321
>>>>
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>>
>>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>
>>>> ---
>>>> changes since v4:
>>>>  - none
>>>>
>>>> changes since v3/v2/v1:
>>>>  - there was no such version because this patch was bundled with
>>>> another
>>>>    patch
>>>>
>>>> changes since RFC:
>>>>  - mention the flashes which just support the "Global Unprotect" in
>>>> the
>>>>    commit message
>>>>
>>>>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>>>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>>>> index 3f5f21a473a6..49d392c6c8bc 100644
>>>> --- a/drivers/mtd/spi-nor/atmel.c
>>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>>> @@ -10,37 +10,27 @@
>>>>
>>>>  static const struct flash_info atmel_parts[] = {
>>>>         /* Atmel -- some are (confusingly) marketed as "DataFlash" */
>>>> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
>>>> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
>>>> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K |
>>>> SPI_NOR_HAS_LOCK) },
>>>
>>> https://datasheetspdf.com/pdf-file/587164/ATMELCorporation/AT25FS010/1
>>> BP bits are at bit 2, 3, 5 and 6.
>>> BP0, BP1, BP3, BP4 and WPEN, are nonvolatile cells
>>>
>>>> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K |
>>>> SPI_NOR_HAS_LOCK) },
>>>
>>> https://datasheetspdf.com/pdf-file/587165/ATMELCorporation/AT25FS040/1
>>> BP bits are at bit 2, 3, 4, 5, and 6.
>>> BP0, BP1, BP2, BP3, BP4 are nonvolatile cells
>>>
>>>>
>>>> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
>>>> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
>>>> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
>>>> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
>>>> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K |
>>>> SPI_NOR_HAS_LOCK) },
>>>
>>> https://datasheetspdf.com/pdf-file/975331/Adesto/AT25DF041A/1
>>> Global Protect/Unprotect using Write SR command:
>>> Global Unlock: write 0x00 to SR
>>> Global Lock: Read SR. If SR.SPRL is 1 write 0xff to SR, else write
>>> 0x7f.
>>
>> That is not my understanding. Quote:
>>   To perform a Global Protect, the appropriate WP pin and SPRL
>>   conditions must be met, and the system must write a logical “1”
>>   to bits 5, 4, 3, and 2 of the Status Register.
>>
>> And
>>   Conversely, to per-form a Global Unprotect, the same WP and SPRL
>>   conditions must be met but the system must write a logical “0” to
>>   bits 5, 4, 3, and 2 of the Status Register
>>
> 
> Right. I think we are both correct, and we should choose one method
> or the other depending on the level of support we want to introduce.
> If we want "locking ops", i.e. partial or full lock and unlock of the
> flash, we'll go your way. If we want to keep things as they were before
> 3e0930f109e76, we'll just support the global unlock by writing 0x00 to SR.

I'm wrong, please ignore. I mixed BP locking with individual sector protection.
Let me read again.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Tudor.Ambarus]

On 11/25/20 8:17 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2020-11-24 20:09, schrieb Tudor.Ambarus@microchip.com:
>> On 10/3/20 6:32 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> This is considered bad for the following reasons:
>>>  (1) We only support the block protection with BPn bits for write
>>>      protection. Not all Atmel parts support this.
>>>  (2) Newly added flash chip will automatically inherit the "has
>>>      locking" support and thus needs to explicitly tested. Better
>>>      be opt-in instead of opt-out.
>>>  (3) There are already supported flashes which doesn't support
>>>      the locking scheme. So I assume this wasn't properly tested
>>>      before adding that chip; which enforces my previous argument
>>>      that locking support should be an opt-in.
>>>
>>> Remove the global flag and add individual flags to all flashes which
>>> supports BP locking. In particular the following flashes don't support
>>> the BP scheme:
>>>  - AT26F004
>>>  - AT25SL321
>>>  - AT45DB081D
>>>
>>> Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK
>>> just
>>> support Global Protection, i.e. not our supported block protection
>>> locking scheme. This is to keep backwards compatibility with the
>>> current "unlock all at boot" mechanism. In particular the following
>>> flashes doesn't have BP bits:
>>>  - AT25DF041A
>>>  - AT25DF321
>>>  - AT25DF321A
>>>  - AT25DF641
>>>  - AT26DF081A
>>>  - AT26DF161A
>>>  - AT26DF321
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>>> ---
>>> changes since v4:
>>>  - none
>>>
>>> changes since v3/v2/v1:
>>>  - there was no such version because this patch was bundled with
>>> another
>>>    patch
>>>
>>> changes since RFC:
>>>  - mention the flashes which just support the "Global Unprotect" in
>>> the
>>>    commit message
>>>
>>>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>>> index 3f5f21a473a6..49d392c6c8bc 100644
>>> --- a/drivers/mtd/spi-nor/atmel.c
>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>> @@ -10,37 +10,27 @@
>>>
>>>  static const struct flash_info atmel_parts[] = {
>>>         /* Atmel -- some are (confusingly) marketed as "DataFlash" */
>>> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
>>> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
>>> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K |
>>> SPI_NOR_HAS_LOCK) },
>>
>> https://datasheetspdf.com/pdf-file/587164/ATMELCorporation/AT25FS010/1
>> BP bits are at bit 2, 3, 5 and 6.
>> BP0, BP1, BP3, BP4 and WPEN, are nonvolatile cells
>>
>>> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K |
>>> SPI_NOR_HAS_LOCK) },
>>
>> https://datasheetspdf.com/pdf-file/587165/ATMELCorporation/AT25FS040/1
>> BP bits are at bit 2, 3, 4, 5, and 6.
>> BP0, BP1, BP2, BP3, BP4 are nonvolatile cells
>>
>>>
>>> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
>>> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
>>> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
>>> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
>>> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K |
>>> SPI_NOR_HAS_LOCK) },
>>
>> https://datasheetspdf.com/pdf-file/975331/Adesto/AT25DF041A/1
>> Global Protect/Unprotect using Write SR command:
>> Global Unlock: write 0x00 to SR
>> Global Lock: Read SR. If SR.SPRL is 1 write 0xff to SR, else write
>> 0x7f.
> 
> That is not my understanding. Quote:
>   To perform a Global Protect, the appropriate WP pin and SPRL
>   conditions must be met, and the system must write a logical “1”
>   to bits 5, 4, 3, and 2 of the Status Register.
> 
> And
>   Conversely, to per-form a Global Unprotect, the same WP and SPRL
>   conditions must be met but the system must write a logical “0” to
>   bits 5, 4, 3, and 2 of the Status Register
> 
> Keep in mind that bit 5, 4, 3 and 2 is exactly the
> ATMEL_SR_GLOBAL_PROTECT_MASK. The SPRL bit is handled in the unlock()
> function. Accoring to table 9.2 you also have to first disable the SPRL
> bit and then write the BP bits to zero.
> 

We took this on irc, I try to summarize the conclusions:
1/ for global unlock protect we have to first set SPRL to zero, if not already
set, then to set the BP bits to zero
2/ for global lock protect, SPRL and BP bits should be written in one shot
3/ consider WP#: set SPRL to 1 when something is locked, set it to zero
if nothing is locked.
4/ at25fs010 and at25fs040 have a BPn mechanism that uses BP4, similar to
what we have in spi_nor_sr_locking_ops(). We decided that it doesn't worth
to pollute the core function just for these flashes, they will have their
own fixup hook. We can't use the hook introduced in 3/3 because those
flashes are using "individual sector protection", and even if the
"global protect/unprotect feature" is close to writing a 0x0 to SR,
eventually the "individual sector protection" locking mechanism should be
extended to also support individual sector locking.

I hope that I didn't miss anything.
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

[Michael Walle]

Am 2020-11-26 17:42, schrieb Tudor.Ambarus@microchip.com:
> On 11/25/20 8:17 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2020-11-24 20:09, schrieb Tudor.Ambarus@microchip.com:
>>> On 10/3/20 6:32 PM, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> This is considered bad for the following reasons:
>>>>  (1) We only support the block protection with BPn bits for write
>>>>      protection. Not all Atmel parts support this.
>>>>  (2) Newly added flash chip will automatically inherit the "has
>>>>      locking" support and thus needs to explicitly tested. Better
>>>>      be opt-in instead of opt-out.
>>>>  (3) There are already supported flashes which doesn't support
>>>>      the locking scheme. So I assume this wasn't properly tested
>>>>      before adding that chip; which enforces my previous argument
>>>>      that locking support should be an opt-in.
>>>> 
>>>> Remove the global flag and add individual flags to all flashes which
>>>> supports BP locking. In particular the following flashes don't 
>>>> support
>>>> the BP scheme:
>>>>  - AT26F004
>>>>  - AT25SL321
>>>>  - AT45DB081D
>>>> 
>>>> Please note, that some flashes which are marked as SPI_NOR_HAS_LOCK
>>>> just
>>>> support Global Protection, i.e. not our supported block protection
>>>> locking scheme. This is to keep backwards compatibility with the
>>>> current "unlock all at boot" mechanism. In particular the following
>>>> flashes doesn't have BP bits:
>>>>  - AT25DF041A
>>>>  - AT25DF321
>>>>  - AT25DF321A
>>>>  - AT25DF641
>>>>  - AT26DF081A
>>>>  - AT26DF161A
>>>>  - AT26DF321
>>>> 
>>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> 
>>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> 
>>>> ---
>>>> changes since v4:
>>>>  - none
>>>> 
>>>> changes since v3/v2/v1:
>>>>  - there was no such version because this patch was bundled with
>>>> another
>>>>    patch
>>>> 
>>>> changes since RFC:
>>>>  - mention the flashes which just support the "Global Unprotect" in
>>>> the
>>>>    commit message
>>>> 
>>>>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>>>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>>> 
>>>> diff --git a/drivers/mtd/spi-nor/atmel.c 
>>>> b/drivers/mtd/spi-nor/atmel.c
>>>> index 3f5f21a473a6..49d392c6c8bc 100644
>>>> --- a/drivers/mtd/spi-nor/atmel.c
>>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>>> @@ -10,37 +10,27 @@
>>>> 
>>>>  static const struct flash_info atmel_parts[] = {
>>>>         /* Atmel -- some are (confusingly) marketed as "DataFlash" 
>>>> */
>>>> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) 
>>>> },
>>>> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) 
>>>> },
>>>> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K |
>>>> SPI_NOR_HAS_LOCK) },
>>> 
>>> https://datasheetspdf.com/pdf-file/587164/ATMELCorporation/AT25FS010/1
>>> BP bits are at bit 2, 3, 5 and 6.
>>> BP0, BP1, BP3, BP4 and WPEN, are nonvolatile cells
>>> 
>>>> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K |
>>>> SPI_NOR_HAS_LOCK) },
>>> 
>>> https://datasheetspdf.com/pdf-file/587165/ATMELCorporation/AT25FS040/1
>>> BP bits are at bit 2, 3, 4, 5, and 6.
>>> BP0, BP1, BP2, BP3, BP4 are nonvolatile cells
>>> 
>>>> 
>>>> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) 
>>>> },
>>>> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) 
>>>> },
>>>> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) 
>>>> },
>>>> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) 
>>>> },
>>>> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K |
>>>> SPI_NOR_HAS_LOCK) },
>>> 
>>> https://datasheetspdf.com/pdf-file/975331/Adesto/AT25DF041A/1
>>> Global Protect/Unprotect using Write SR command:
>>> Global Unlock: write 0x00 to SR
>>> Global Lock: Read SR. If SR.SPRL is 1 write 0xff to SR, else write
>>> 0x7f.
>> 
>> That is not my understanding. Quote:
>>   To perform a Global Protect, the appropriate WP pin and SPRL
>>   conditions must be met, and the system must write a logical “1”
>>   to bits 5, 4, 3, and 2 of the Status Register.
>> 
>> And
>>   Conversely, to per-form a Global Unprotect, the same WP and SPRL
>>   conditions must be met but the system must write a logical “0” to
>>   bits 5, 4, 3, and 2 of the Status Register
>> 
>> Keep in mind that bit 5, 4, 3 and 2 is exactly the
>> ATMEL_SR_GLOBAL_PROTECT_MASK. The SPRL bit is handled in the unlock()
>> function. Accoring to table 9.2 you also have to first disable the 
>> SPRL
>> bit and then write the BP bits to zero.
>> 
> 
> We took this on irc, I try to summarize the conclusions:
> 1/ for global unlock protect we have to first set SPRL to zero, if not 
> already
> set, then to set the BP bits to zero
> 2/ for global lock protect, SPRL and BP bits should be written in one 
> shot

This is the other way around from the datasheet:
https://www.adestotech.com/wp-content/uploads/doc3668.pdf

   When changing the SPRL bit to a logical “1” from a logical “0”, it
   is also possible to perform a Global Protect or Global Unprotect at
   the same time by writing the appropriate values into bits 5, 4, 3,
   and 2 of the Status Register.

Doing Global Protect and setting SPRL=1 at the same time is also 
possible,
see Table 9-2. That is pretty clear.

Therefore, we could do both lock and unlock in one step. But one thing I
didn't consider is that it may be possible that clearing will fail if 
WP#
is asserted. The current patch will check that and report an error. I'd
like to keep that.

> 3/ consider WP#: set SPRL to 1 when something is locked, set it to zero
> if nothing is locked.

Ack. This follows the behavior of the current locking mechanism for 
flashes
with BP bits.

> 4/ at25fs010 and at25fs040 have a BPn mechanism that uses BP4, similar 
> to
> what we have in spi_nor_sr_locking_ops(). We decided that it doesn't 
> worth
> to pollute the core function just for these flashes, they will have 
> their
> own fixup hook. We can't use the hook introduced in 3/3 because those
> flashes are using "individual sector protection", and even if the
> "global protect/unprotect feature" is close to writing a 0x0 to SR,
> eventually the "individual sector protection" locking mechanism should 
> be
> extended to also support individual sector locking.

Ack

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 2/3] mtd: spi-nor: sst: remove global protection flag

[Michael Walle]

This is considered bad for the following reasons:
 (1) We only support the block protection with BPn bits for write
     protection. Not all SST parts support this.
 (2) Newly added flash chip will automatically inherit the "has
     locking" support and thus needs to explicitly tested. Better
     be opt-in instead of opt-out.
 (3) There are already supported flashes which doesn't support
     the locking scheme. So I assume this wasn't properly tested
     before adding that chip; which enforces my previous argument
     that locking support should be an opt-in.

Remove the global flag and add individual flags to all flashes
which supports BP locking. In particular the following flashes
don't support the BP scheme:
 - SST26VF016B
 - SST26WF016B
 - SST26VF064B

Signed-off-by: Michael Walle <michael@walle.cc>
---
changes since v4:
 - none

changes since v3/v2/v1:
 - there was no such version because this patch was bundled with another
   patch

changes since RFC:
 - none

 drivers/mtd/spi-nor/sst.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index e0af6d25d573..8b169fa4102a 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -11,26 +11,26 @@
 static const struct flash_info sst_parts[] = {
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
 	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
-			      SECT_4K | SST_WRITE) },
-	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
-			      SECT_4K | SST_WRITE) },
-	{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K) },
-	{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+	{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16,
-			      SECT_4K | SST_WRITE) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
 	{ "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,
 			      SECT_4K | SPI_NOR_DUAL_READ |
 			      SPI_NOR_QUAD_READ) },
@@ -127,11 +127,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	return ret;
 }
 
-static void sst_default_init(struct spi_nor *nor)
-{
-	nor->flags |= SNOR_F_HAS_LOCK;
-}
-
 static void sst_post_sfdp_fixups(struct spi_nor *nor)
 {
 	if (nor->info->flags & SST_WRITE)
@@ -139,7 +134,6 @@ static void sst_post_sfdp_fixups(struct spi_nor *nor)
 }
 
 static const struct spi_nor_fixups sst_fixups = {
-	.default_init = sst_default_init,
 	.post_sfdp = sst_post_sfdp_fixups,
 };
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 2/3] mtd: spi-nor: sst: remove global protection flag

[Tudor.Ambarus]

On 10/3/20 6:32 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This is considered bad for the following reasons:
>  (1) We only support the block protection with BPn bits for write
>      protection. Not all SST parts support this.
>  (2) Newly added flash chip will automatically inherit the "has
>      locking" support and thus needs to explicitly tested. Better
>      be opt-in instead of opt-out.
>  (3) There are already supported flashes which doesn't support
>      the locking scheme. So I assume this wasn't properly tested
>      before adding that chip; which enforces my previous argument
>      that locking support should be an opt-in.
> 
> Remove the global flag and add individual flags to all flashes
> which supports BP locking. In particular the following flashes
> don't support the BP scheme:
>  - SST26VF016B
>  - SST26WF016B
>  - SST26VF064B
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
> changes since v4:
>  - none
> 
> changes since v3/v2/v1:
>  - there was no such version because this patch was bundled with another
>    patch
> 
> changes since RFC:
>  - none
> 
>  drivers/mtd/spi-nor/sst.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index e0af6d25d573..8b169fa4102a 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -11,26 +11,26 @@
>  static const struct flash_info sst_parts[] = {
>         /* SST -- large erase sizes are "overlays", "sectors" are 4K */
>         { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/DS20005051D.pdf
BP GENMASK(2, 5), volatile, after power up BP0-2 set to 1, BP3 set to 0.
BP3 is "don't care".
SR.BPL BIT(7) volatile, default to 0. When 1 - BPs are read-only bits,
when 0 - BPs are r/w

>         { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/20005045C.pdf
same as in sst25vf040b

>         { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/20005044C.pdf
same as in sst25vf040b


>         { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
> -                             SECT_4K | SST_WRITE) },
> -       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/20005071B.pdf
same as in sst25vf040b

> +       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/devicedoc/20005036c.pdf
BP GENMASK(2, 5), volatile, after power up all set to 1.
SR.BPL BIT(7) volatile, default to 0. When 1 - BPs are read-only bits,
when 0 - BPs are r/w

>         { "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/20005016C.pdf
BP GENMASK(2, 4), volatile, after power up all set to 1.
SR.BPL BIT(7) volatile, default to 0. When 1 - BPs are read-only bits,
when 0 - BPs are r/w

>         { "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

same datasheet and characteristics as in sst25wf512

>         { "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
> -                             SECT_4K | SST_WRITE) },
> -       { "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K) },
> -       { "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

same datasheet and characteristics as in sst25wf512

> +       { "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/20005139F.pdf
BP0 BIT(2), BP1 BIT(3), TB BIT(5), BPL BIT(7) all non-volatile
SR.BPL BIT(7): when 1 - BPs are read-only bits, when 0 - BPs are r/w

> +       { "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/20005193D.pdf
BP0 BIT(2), BP1 BIT(3), BP2 BIT(4), TB BIT(5), BPL BIT(7) all non-volatile
SR.BPL BIT(7): when 1 - BPs are read-only bits, when 0 - BPs are r/w

>         { "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

same datasheet and characteristics as in sst25wf512

>         { "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16,
> -                             SECT_4K | SST_WRITE) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },

https://ww1.microchip.com/downloads/en/DeviceDoc/25024A.pdf
same as in sst25vf040b

>         { "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,

https://ww1.microchip.com/downloads/en/DeviceDoc/20005013D.pdf
OK. sst26wf016b, SST26VF016B, SST26VF064B support individual block protection
and global unlock command.

>                               SECT_4K | SPI_NOR_DUAL_READ |
>                               SPI_NOR_QUAD_READ) },
> @@ -127,11 +127,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
>         return ret;
>  }
> 
> -static void sst_default_init(struct spi_nor *nor)
> -{
> -       nor->flags |= SNOR_F_HAS_LOCK;
> -}
> -
>  static void sst_post_sfdp_fixups(struct spi_nor *nor)
>  {
>         if (nor->info->flags & SST_WRITE)
> @@ -139,7 +134,6 @@ static void sst_post_sfdp_fixups(struct spi_nor *nor)
>  }
> 
>  static const struct spi_nor_fixups sst_fixups = {
> -       .default_init = sst_default_init,
>         .post_sfdp = sst_post_sfdp_fixups,
>  };
> 
> --
> 2.20.1
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Traditionally, linux unlocks the whole flash because there are legacy
devices which has the write protections bits set by default at startup.
If you actually want to use the flash protection bits, eg. because there
is a read-only part for a bootloader, this automatic unlocking is
harmful. If there is no hardware write protection in place (usually
called WP#), a startup of the kernel just discards this protection.

I've gone through the datasheets of all the flashes (except the Intel
ones where I could not find any datasheet nor reference) which supports
the unlocking feature and looked how the sector protection was
implemented. The currently supported flashes can be divided into the
following two categories:
 (1) block protection bits are non-volatile. Thus they keep their values
     at reset and power-cycle
 (2) flashes where these bits are volatile. After reset or power-cycle,
     the whole memory array is protected.
     (a) some devices needs a special "Global Unprotect" command, eg.
         the Atmel AT25DF041A.
     (b) some devices require to clear the BPn bits in the status
         register.

Due to the reasons above, we do not want to clear the bits for flashes
which belong to category (1). Fortunately for us, only Atmel flashes
fall into category (2a). Implement the "Global Protect" and "Global
Unprotect" commands for these. For (2b) we can use normal block
protection locking scheme.

This patch adds a new flag to indicate the case (2). Only if we have
such a flash we unlock the whole flash array. To be backwards compatible
it also introduces a kernel configuration option which restores the
complete legacy behavior ("Disable write protection on any flashes").
Hopefully, this will clean up "unlock the entire flash for legacy
devices" once and for all.

For reference here are the actually commits which introduced the legacy
behaviour (and extended the behaviour to other chip manufacturers):

commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx {16,32,64}0S33B SPI flash chips")
commit ea60658a08f8f ("mtd: m25p80: disable SST software protection bits by default")
commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails to be copied to")

Actually, this might also fix handling of the Atmel AT25DF flashes,
because the original commit 7228982442365 ("[MTD] m25p80: fix bug -
ATmel spi flash fails to be copied to") was writing a 0 to the status
register, which is a "Global Unprotect". This might not be the case in
the current code which only handles the block protection bits BP2, BP1
and BP0. Thus, it depends on the current contents of the status register
if this unlock actually corresponds to a "Global Unprotect" command. In
the worst case, the current code might leave the AT25DF flashes in a
write protected state.

The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when QE
is set on spansion flashes") changed that behaviour by just clearing BP2
to BP0 instead of writing a 0 to the status register.

Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling
of block write protection") expanded the unlock_all() feature to ANY
flash which supports locking.

Signed-off-by: Michael Walle <michael@walle.cc>
---
changes since v4:
 - made atmel_global_protection_default_init() static, spotted by
   lkp@intel.com

changes since v3:
 - now defaulting to MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE, suggested by Vignesh
 - restored the original spi_nor_unlock_all(), instead add individual
   locking ops for the "Global Protect" scheme in atmel.c. This was tested
   partly with the AT25SL321 (for the test I added the fixups to this
   flash).
 - renamed SPI_NOR_UNPROTECT to SPI_NOR_WP_IS_VOLATILE. Suggested by
   Vingesh, although I've renamed it to a more general "WP_IS_VOLATILE"
   because either the BP bits or the individual sector locks might be
   volatile.
 - add mention of both block protection bits and "Global Unprotect" command
   in the Kconfig help text.

changes since v2:
 - add Kconfig option to be able to retain legacy behaviour
 - rebased the patch due to the spi-nor rewrite
 - dropped the Fixes: tag, it doens't make sense after the spi-nor rewrite
 - mention commit 3e0930f109e76 which further modified the unlock
   behaviour.

changes since v1:
 - completely rewrote patch, the first version used a device tree flag

 drivers/mtd/spi-nor/Kconfig |  42 ++++++++++++++
 drivers/mtd/spi-nor/atmel.c | 111 +++++++++++++++++++++++++++++++++---
 drivers/mtd/spi-nor/core.c  |  36 ++++++++----
 drivers/mtd/spi-nor/core.h  |   8 +++
 drivers/mtd/spi-nor/esmt.c  |   8 ++-
 drivers/mtd/spi-nor/intel.c |   6 +-
 drivers/mtd/spi-nor/sst.c   |  21 +++----
 7 files changed, 199 insertions(+), 33 deletions(-)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index ffc4b380f2b1..11e6658ee85d 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -24,6 +24,48 @@ config MTD_SPI_NOR_USE_4K_SECTORS
 	  Please note that some tools/drivers/filesystems may not work with
 	  4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
 
+choice
+	prompt "Write protection at boot"
+	default MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE
+
+config MTD_SPI_NOR_WP_DISABLE
+	bool "Disable WP on any flashes (legacy behaviour)"
+	help
+	  This option disables the write protection on any SPI flashes at
+	  boot-up.
+
+	  Depending on the flash chip this either clears the block protection
+	  bits or does a "Global Unprotect" command.
+
+	  Don't use this if you intent to use the write protection of your
+	  SPI flash. This is only to keep backwards compatibility.
+
+config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE
+	bool "Disable WP on flashes w/ volatile protection bits"
+	help
+	  Some SPI flashes have volatile block protection bits, ie. after a
+	  power-up or a reset the flash is write protected by default.
+
+	  This option disables the write protection for these kind of flashes
+	  while keeping it enabled for any other SPI flashes which have
+	  non-volatile write protection bits.
+
+	  If the write protection will be disabled depending on the flash
+	  either the block protection bits are cleared or a "Global Unprotect"
+	  command is issued.
+
+	  If you are unsure, select this option.
+
+config MTD_SPI_NOR_WP_KEEP
+	bool "Keep write protection as is"
+	help
+	  If you select this option the write protection of any SPI flashes
+	  will not be changed. If your flash is write protected or will be
+	  automatically write protected after power-up you have to manually
+	  unlock it before you are able to write to it.
+
+endchoice
+
 source "drivers/mtd/spi-nor/controllers/Kconfig"
 
 endif # MTD_SPI_NOR
diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
index 49d392c6c8bc..19665095aeab 100644
--- a/drivers/mtd/spi-nor/atmel.c
+++ b/drivers/mtd/spi-nor/atmel.c
@@ -8,23 +8,120 @@
 
 #include "core.h"
 
+#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2)
+
+/**
+ * atmel_set_global_protection - Do a Global Protect or Unprotect command
+ * @nor:	pointer to 'struct spi_nor'
+ * @ofs:	offset in bytes
+ * @len:	len in bytes
+ * @is_protect:	if true do a Global Protect otherwise it is a Global Unprotect
+ *
+ * Return: 0 on success, -error otherwise.
+ */
+static int atmel_set_global_protection(struct spi_nor *nor, loff_t ofs,
+				       uint64_t len, bool is_protect)
+{
+	int ret;
+	u8 sr;
+
+	/* We only support locking the whole flash array */
+	if (ofs || len != nor->params->size)
+		return -EINVAL;
+
+	ret = spi_nor_read_sr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+	sr = nor->bouncebuf[0];
+
+	/* SRWD bit needs to be cleared, otherwise the protection doesn't change */
+	if (sr & SR_SRWD) {
+		sr &= ~SR_SRWD;
+		ret = spi_nor_write_sr_and_check(nor, sr);
+		if (ret) {
+			dev_err(nor->dev, "unable to clear SRWD bit, WP# asserted?\n");
+			return ret;
+		}
+	}
+
+	if (is_protect)
+		sr |= ATMEL_SR_GLOBAL_PROTECT_MASK;
+	else
+		sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK;
+
+	return spi_nor_write_sr_and_check(nor, sr);
+}
+
+static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	return atmel_set_global_protection(nor, ofs, len, true);
+}
+
+static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	return atmel_set_global_protection(nor, ofs, len, false);
+}
+
+static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	int ret;
+
+	if (ofs >= nor->params->size || (ofs + len) > nor->params->size)
+		return -EINVAL;
+
+	ret = spi_nor_read_sr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+
+	return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == ATMEL_SR_GLOBAL_PROTECT_MASK);
+}
+
+static const struct spi_nor_locking_ops atmel_global_protection_ops = {
+	.lock = atmel_global_protect,
+	.unlock = atmel_global_unprotect,
+	.is_locked = atmel_is_global_protected,
+};
+
+static void atmel_global_protection_default_init(struct spi_nor *nor)
+{
+	nor->params->locking_ops = &atmel_global_protection_ops;
+}
+
+static const struct spi_nor_fixups atmel_global_protection_fixups = {
+	.default_init = atmel_global_protection_default_init,
+};
+
 static const struct flash_info atmel_parts[] = {
 	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
 	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K | SPI_NOR_HAS_LOCK) },
 	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },
 
-	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
+	{ "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
+	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
+	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
 
 	{ "at25sl321",	INFO(0x1f4216, 0, 64 * 1024, 64,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 
 	{ "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },
-	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
+	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
+	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64,
+			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
+			.fixups = &atmel_global_protection_fixups },
 
 	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
 };
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 2add4a01fce2..c40a7c1490d7 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -276,7 +276,7 @@ int spi_nor_write_disable(struct spi_nor *nor)
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
+int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 {
 	int ret;
 
@@ -906,7 +906,7 @@ static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1)
+int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1)
 {
 	if (nor->flags & SNOR_F_HAS_16BIT_SR)
 		return spi_nor_write_16bit_sr_and_check(nor, sr1);
@@ -2919,15 +2919,14 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
  * spi_nor_unlock_all() - Unlocks the entire flash memory array.
  * @nor:	pointer to a 'struct spi_nor'.
  *
- * Some SPI NOR flashes are write protected by default after a power-on reset
- * cycle, in order to avoid inadvertent writes during power-up. Backward
- * compatibility imposes to unlock the entire flash memory array at power-up
- * by default.
+ * Return: 0 on success, -errno otherwise.
  */
 static int spi_nor_unlock_all(struct spi_nor *nor)
 {
-	if (nor->flags & SNOR_F_HAS_LOCK)
+	if (nor->flags & SNOR_F_HAS_LOCK) {
+		dev_dbg(nor->dev, "unprotecting entire flash\n");
 		return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
+	}
 
 	return 0;
 }
@@ -2942,10 +2941,23 @@ static int spi_nor_init(struct spi_nor *nor)
 		return err;
 	}
 
-	err = spi_nor_unlock_all(nor);
-	if (err) {
-		dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
-		return err;
+	/*
+	 * Some SPI NOR flashes are write protected by default after a power-on
+	 * reset cycle, in order to avoid inadvertent writes during power-up.
+	 * Backward compatibility imposes to unlock the entire flash memory
+	 * array at power-up by default. Depending on the kernel configuration
+	 * (1) we do nothing, (2) we unlock the entire flash in any case or (3)
+	 * just do it actually powers up write-protected. The latter is
+	 * indicated by SNOR_F_WP_IS_VOLATILE.
+	 */
+	if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) ||
+	    (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) &&
+	     nor->flags & SNOR_F_WP_IS_VOLATILE)) {
+		err = spi_nor_unlock_all(nor);
+		if (err) {
+			dev_err(nor->dev, "Failed to unlock the entire flash memory array\n");
+			return err;
+		}
 	}
 
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
@@ -3170,6 +3182,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
 	if (info->flags & USE_CLSR)
 		nor->flags |= SNOR_F_USE_CLSR;
+	if (info->flags & SPI_NOR_WP_IS_VOLATILE)
+		nor->flags |= SNOR_F_WP_IS_VOLATILE;
 
 	if (info->flags & SPI_NOR_4BIT_BP) {
 		nor->flags |= SNOR_F_HAS_4BIT_BP;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 6f2f6b27173f..99dd2e14142a 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -26,6 +26,7 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
 	SNOR_F_HAS_4BIT_BP      = BIT(12),
 	SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
+	SNOR_F_WP_IS_VOLATILE	= BIT(14),
 };
 
 struct spi_nor_read_command {
@@ -311,6 +312,11 @@ struct flash_info {
 					 * BP3 is bit 6 of status register.
 					 * Must be used with SPI_NOR_4BIT_BP.
 					 */
+#define SPI_NOR_WP_IS_VOLATILE	BIT(19)	/*
+					 * Flash has volatile write protection
+					 * bits. Usually these will power-up in
+					 * a write-protected state.
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -409,6 +415,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor);
 int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
 int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
 int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
+int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
+int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
 
 int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c
index c93170008118..c2ebf29d95f2 100644
--- a/drivers/mtd/spi-nor/esmt.c
+++ b/drivers/mtd/spi-nor/esmt.c
@@ -11,9 +11,13 @@
 static const struct flash_info esmt_parts[] = {
 	/* ESMT */
 	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64,
-			   SECT_4K | SPI_NOR_HAS_LOCK) },
+			   SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64,
-			   SECT_4K | SPI_NOR_HAS_LOCK) },
+			   SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
+	/*
+	 * According to the datasheet the BPn bits are non-volatile, whereas
+	 * they are volatile for the smaller f25l32qa.
+	 */
 	{ "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128,
 			   SECT_4K | SPI_NOR_HAS_LOCK) },
 };
diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c
index d8196f101368..c45ea0ad01f0 100644
--- a/drivers/mtd/spi-nor/intel.c
+++ b/drivers/mtd/spi-nor/intel.c
@@ -10,9 +10,9 @@
 
 static const struct flash_info intel_parts[] = {
 	/* Intel/Numonyx -- xxxs33b */
-	{ "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
-	{ "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
-	{ "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
+	{ "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, SPI_NOR_WP_IS_VOLATILE) },
+	{ "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, SPI_NOR_WP_IS_VOLATILE) },
+	{ "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_WP_IS_VOLATILE) },
 };
 
 static void intel_default_init(struct spi_nor *nor)
diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 8b169fa4102a..5e4450877d66 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -11,26 +11,27 @@
 static const struct flash_info sst_parts[] = {
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
 	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
-	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
+	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
+			      SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_HAS_LOCK) },
 	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
+			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
 	{ "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,
 			      SECT_4K | SPI_NOR_DUAL_READ |
 			      SPI_NOR_QUAD_READ) },
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

On 10/3/20 6:32 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Traditionally, linux unlocks the whole flash because there are legacy
> devices which has the write protections bits set by default at startup.
> If you actually want to use the flash protection bits, eg. because there
> is a read-only part for a bootloader, this automatic unlocking is
> harmful. If there is no hardware write protection in place (usually
> called WP#), a startup of the kernel just discards this protection.
> 
> I've gone through the datasheets of all the flashes (except the Intel
> ones where I could not find any datasheet nor reference) which supports
> the unlocking feature and looked how the sector protection was
> implemented. The currently supported flashes can be divided into the
> following two categories:
>  (1) block protection bits are non-volatile. Thus they keep their values
>      at reset and power-cycle
>  (2) flashes where these bits are volatile. After reset or power-cycle,
>      the whole memory array is protected.
>      (a) some devices needs a special "Global Unprotect" command, eg.
>          the Atmel AT25DF041A.
>      (b) some devices require to clear the BPn bits in the status
>          register.
> 
> Due to the reasons above, we do not want to clear the bits for flashes
> which belong to category (1). Fortunately for us, only Atmel flashes
> fall into category (2a). Implement the "Global Protect" and "Global
> Unprotect" commands for these. For (2b) we can use normal block
> protection locking scheme.
> 
> This patch adds a new flag to indicate the case (2). Only if we have
> such a flash we unlock the whole flash array. To be backwards compatible
> it also introduces a kernel configuration option which restores the
> complete legacy behavior ("Disable write protection on any flashes").
> Hopefully, this will clean up "unlock the entire flash for legacy
> devices" once and for all.
> 
> For reference here are the actually commits which introduced the legacy
> behaviour (and extended the behaviour to other chip manufacturers):
> 
> commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx {16,32,64}0S33B SPI flash chips")
> commit ea60658a08f8f ("mtd: m25p80: disable SST software protection bits by default")
> commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails to be copied to")
> 
> Actually, this might also fix handling of the Atmel AT25DF flashes,
> because the original commit 7228982442365 ("[MTD] m25p80: fix bug -
> ATmel spi flash fails to be copied to") was writing a 0 to the status
> register, which is a "Global Unprotect". This might not be the case in
> the current code which only handles the block protection bits BP2, BP1
> and BP0. Thus, it depends on the current contents of the status register
> if this unlock actually corresponds to a "Global Unprotect" command. In
> the worst case, the current code might leave the AT25DF flashes in a
> write protected state.
> 
> The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when QE
> is set on spansion flashes") changed that behaviour by just clearing BP2
> to BP0 instead of writing a 0 to the status register.
> 
> Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling
> of block write protection") expanded the unlock_all() feature to ANY
> flash which supports locking.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> changes since v4:
>  - made atmel_global_protection_default_init() static, spotted by
>    lkp@intel.com
> 
> changes since v3:
>  - now defaulting to MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE, suggested by Vignesh
>  - restored the original spi_nor_unlock_all(), instead add individual
>    locking ops for the "Global Protect" scheme in atmel.c. This was tested
>    partly with the AT25SL321 (for the test I added the fixups to this
>    flash).
>  - renamed SPI_NOR_UNPROTECT to SPI_NOR_WP_IS_VOLATILE. Suggested by
>    Vingesh, although I've renamed it to a more general "WP_IS_VOLATILE"
>    because either the BP bits or the individual sector locks might be
>    volatile.
>  - add mention of both block protection bits and "Global Unprotect" command
>    in the Kconfig help text.
> 
> changes since v2:
>  - add Kconfig option to be able to retain legacy behaviour
>  - rebased the patch due to the spi-nor rewrite
>  - dropped the Fixes: tag, it doens't make sense after the spi-nor rewrite
>  - mention commit 3e0930f109e76 which further modified the unlock
>    behaviour.
> 
> changes since v1:
>  - completely rewrote patch, the first version used a device tree flag
> 
>  drivers/mtd/spi-nor/Kconfig |  42 ++++++++++++++
>  drivers/mtd/spi-nor/atmel.c | 111 +++++++++++++++++++++++++++++++++---
>  drivers/mtd/spi-nor/core.c  |  36 ++++++++----
>  drivers/mtd/spi-nor/core.h  |   8 +++
>  drivers/mtd/spi-nor/esmt.c  |   8 ++-
>  drivers/mtd/spi-nor/intel.c |   6 +-
>  drivers/mtd/spi-nor/sst.c   |  21 +++----
>  7 files changed, 199 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index ffc4b380f2b1..11e6658ee85d 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -24,6 +24,48 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>           Please note that some tools/drivers/filesystems may not work with
>           4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
> 
> +choice
> +       prompt "Write protection at boot"
> +       default MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE
> +
> +config MTD_SPI_NOR_WP_DISABLE
> +       bool "Disable WP on any flashes (legacy behaviour)"
> +       help
> +         This option disables the write protection on any SPI flashes at
> +         boot-up.
> +
> +         Depending on the flash chip this either clears the block protection
> +         bits or does a "Global Unprotect" command.
> +
> +         Don't use this if you intent to use the write protection of your
> +         SPI flash. This is only to keep backwards compatibility.
> +
> +config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE
> +       bool "Disable WP on flashes w/ volatile protection bits"
> +       help
> +         Some SPI flashes have volatile block protection bits, ie. after a
> +         power-up or a reset the flash is write protected by default.
> +
> +         This option disables the write protection for these kind of flashes
> +         while keeping it enabled for any other SPI flashes which have
> +         non-volatile write protection bits.
> +
> +         If the write protection will be disabled depending on the flash
> +         either the block protection bits are cleared or a "Global Unprotect"
> +         command is issued.
> +
> +         If you are unsure, select this option.
> +
> +config MTD_SPI_NOR_WP_KEEP
> +       bool "Keep write protection as is"
> +       help
> +         If you select this option the write protection of any SPI flashes
> +         will not be changed. If your flash is write protected or will be
> +         automatically write protected after power-up you have to manually
> +         unlock it before you are able to write to it.
> +
> +endchoice
> +
>  source "drivers/mtd/spi-nor/controllers/Kconfig"
> 
>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
> index 49d392c6c8bc..19665095aeab 100644
> --- a/drivers/mtd/spi-nor/atmel.c
> +++ b/drivers/mtd/spi-nor/atmel.c
> @@ -8,23 +8,120 @@
> 
>  #include "core.h"
> 
> +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2)
> +
> +/**
> + * atmel_set_global_protection - Do a Global Protect or Unprotect command
> + * @nor:       pointer to 'struct spi_nor'
> + * @ofs:       offset in bytes
> + * @len:       len in bytes
> + * @is_protect:        if true do a Global Protect otherwise it is a Global Unprotect
> + *
> + * Return: 0 on success, -error otherwise.
> + */
> +static int atmel_set_global_protection(struct spi_nor *nor, loff_t ofs,
> +                                      uint64_t len, bool is_protect)
> +{
> +       int ret;
> +       u8 sr;
> +
> +       /* We only support locking the whole flash array */
> +       if (ofs || len != nor->params->size)
> +               return -EINVAL;
> +
> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
> +       if (ret)
> +               return ret;
> +       sr = nor->bouncebuf[0];
> +
> +       /* SRWD bit needs to be cleared, otherwise the protection doesn't change */
> +       if (sr & SR_SRWD) {
> +               sr &= ~SR_SRWD;
> +               ret = spi_nor_write_sr_and_check(nor, sr);
> +               if (ret) {
> +                       dev_err(nor->dev, "unable to clear SRWD bit, WP# asserted?\n");
> +                       return ret;
> +               }
> +       }
> +
> +       if (is_protect)
> +               sr |= ATMEL_SR_GLOBAL_PROTECT_MASK;
> +       else
> +               sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK;
> +
> +       return spi_nor_write_sr_and_check(nor, sr);
> +}
> +
> +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +       return atmel_set_global_protection(nor, ofs, len, true);
> +}
> +
> +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +       return atmel_set_global_protection(nor, ofs, len, false);
> +}
> +
> +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +       int ret;
> +
> +       if (ofs >= nor->params->size || (ofs + len) > nor->params->size)
> +               return -EINVAL;
> +
> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
> +       if (ret)
> +               return ret;
> +
> +       return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == ATMEL_SR_GLOBAL_PROTECT_MASK);
> +}
> +
> +static const struct spi_nor_locking_ops atmel_global_protection_ops = {
> +       .lock = atmel_global_protect,
> +       .unlock = atmel_global_unprotect,
> +       .is_locked = atmel_is_global_protected,
> +};

Skimming through my notes in 1/3, I'm guessing this will not work for any
of the atmel flashes that we currently have.

Can we instead return -EOPNOTSUPP for .is_locked and .lock and just write a 0x00 to SR
for the .unlock method?

> +
> +static void atmel_global_protection_default_init(struct spi_nor *nor)
> +{
> +       nor->params->locking_ops = &atmel_global_protection_ops;
> +}
> +
> +static const struct spi_nor_fixups atmel_global_protection_fixups = {
> +       .default_init = atmel_global_protection_default_init,
> +};
> +
>  static const struct flash_info atmel_parts[] = {
>         /* Atmel -- some are (confusingly) marketed as "DataFlash" */
>         { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K | SPI_NOR_HAS_LOCK) },
>         { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },
> 
> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K | SPI_NOR_HAS_LOCK) },
> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },
> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_HAS_LOCK) },
> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> +       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> +       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> +       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> 
>         { "at25sl321",  INFO(0x1f4216, 0, 64 * 1024, 64,
>                              SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> 
>         { "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },
> -       { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_HAS_LOCK) },
> -       { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_LOCK) },
> -       { "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) },
> +       { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> +       { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> +       { "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64,
> +                            SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE)
> +                       .fixups = &atmel_global_protection_fixups },
> 
>         { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
>  };
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 2add4a01fce2..c40a7c1490d7 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -276,7 +276,7 @@ int spi_nor_write_disable(struct spi_nor *nor)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
> +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>  {
>         int ret;
> 
> @@ -906,7 +906,7 @@ static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1)
> +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1)
>  {
>         if (nor->flags & SNOR_F_HAS_16BIT_SR)
>                 return spi_nor_write_16bit_sr_and_check(nor, sr1);
> @@ -2919,15 +2919,14 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
>   * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>   * @nor:       pointer to a 'struct spi_nor'.
>   *
> - * Some SPI NOR flashes are write protected by default after a power-on reset
> - * cycle, in order to avoid inadvertent writes during power-up. Backward
> - * compatibility imposes to unlock the entire flash memory array at power-up
> - * by default.
> + * Return: 0 on success, -errno otherwise.
>   */
>  static int spi_nor_unlock_all(struct spi_nor *nor)
>  {
> -       if (nor->flags & SNOR_F_HAS_LOCK)
> +       if (nor->flags & SNOR_F_HAS_LOCK) {
> +               dev_dbg(nor->dev, "unprotecting entire flash\n");
>                 return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
> +       }
> 
>         return 0;
>  }
> @@ -2942,10 +2941,23 @@ static int spi_nor_init(struct spi_nor *nor)
>                 return err;
>         }
> 
> -       err = spi_nor_unlock_all(nor);
> -       if (err) {
> -               dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
> -               return err;
> +       /*
> +        * Some SPI NOR flashes are write protected by default after a power-on
> +        * reset cycle, in order to avoid inadvertent writes during power-up.
> +        * Backward compatibility imposes to unlock the entire flash memory
> +        * array at power-up by default. Depending on the kernel configuration
> +        * (1) we do nothing, (2) we unlock the entire flash in any case or (3)
> +        * just do it actually powers up write-protected. The latter is
> +        * indicated by SNOR_F_WP_IS_VOLATILE.
> +        */
> +       if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) ||
> +           (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) &&
> +            nor->flags & SNOR_F_WP_IS_VOLATILE)) {
> +               err = spi_nor_unlock_all(nor);
> +               if (err) {
> +                       dev_err(nor->dev, "Failed to unlock the entire flash memory array\n");
> +                       return err;
> +               }
>         }
> 
>         if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> @@ -3170,6 +3182,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>                 nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>         if (info->flags & USE_CLSR)
>                 nor->flags |= SNOR_F_USE_CLSR;
> +       if (info->flags & SPI_NOR_WP_IS_VOLATILE)
> +               nor->flags |= SNOR_F_WP_IS_VOLATILE;
> 
>         if (info->flags & SPI_NOR_4BIT_BP) {
>                 nor->flags |= SNOR_F_HAS_4BIT_BP;
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 6f2f6b27173f..99dd2e14142a 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
>         SNOR_F_HAS_SR_TB_BIT6   = BIT(11),
>         SNOR_F_HAS_4BIT_BP      = BIT(12),
>         SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
> +       SNOR_F_WP_IS_VOLATILE   = BIT(14),
>  };
> 
>  struct spi_nor_read_command {
> @@ -311,6 +312,11 @@ struct flash_info {
>                                          * BP3 is bit 6 of status register.
>                                          * Must be used with SPI_NOR_4BIT_BP.
>                                          */
> +#define SPI_NOR_WP_IS_VOLATILE BIT(19) /*
> +                                        * Flash has volatile write protection
> +                                        * bits. Usually these will power-up in
> +                                        * a write-protected state.
> +                                        */
> 
>         /* Part specific fixup hooks. */
>         const struct spi_nor_fixups *fixups;
> @@ -409,6 +415,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor);
>  int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
>  int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
>  int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
> +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
> +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
> 
>  int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
>  ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
> diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c
> index c93170008118..c2ebf29d95f2 100644
> --- a/drivers/mtd/spi-nor/esmt.c
> +++ b/drivers/mtd/spi-nor/esmt.c
> @@ -11,9 +11,13 @@
>  static const struct flash_info esmt_parts[] = {
>         /* ESMT */
>         { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64,
> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
> +                          SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },

https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32PA.pdf
BP GENMASK(4,2), volatile, ok

>         { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64,
> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
> +                          SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },

https://datasheetspdf.com/pdf-file/796196/ESMT/F25L32QA/1
Datasheet states that "BP0~3, QE and BPL bits are non-volatile."
At the same time, it says: "After power-up, BP3, BP2, BP1 and BP0 bits
are set to 0."

Maybe factory default setting for BPn is 0? Let's treat them as NV, as in
f25l64qa.

Do we need BP3?

> +       /*
> +        * According to the datasheet the BPn bits are non-volatile, whereas
> +        * they are volatile for the smaller f25l32qa.
> +        */
>         { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128,
>                            SECT_4K | SPI_NOR_HAS_LOCK) },

https://datasheetspdf.com/pdf-file/967488/EliteSemiconductor/F25L64QA/1
BP GENMASK(5, 2), non-volatile.

BP3?


>  };
> diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c
> index d8196f101368..c45ea0ad01f0 100644
> --- a/drivers/mtd/spi-nor/intel.c
> +++ b/drivers/mtd/spi-nor/intel.c
> @@ -10,9 +10,9 @@
> 
>  static const struct flash_info intel_parts[] = {
>         /* Intel/Numonyx -- xxxs33b */
> -       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
> -       { "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
> -       { "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
> +       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, SPI_NOR_WP_IS_VOLATILE) },
> +       { "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, SPI_NOR_WP_IS_VOLATILE) },
> +       { "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_WP_IS_VOLATILE) },

http://www.datasheet.hk/view_download.php?id=1561787&file=0282\qh25f016s33b8_649107.pdf
BP GENMASK(4,2) all volatile, all set to 1 at power-up.

You can add SPI_NOR_HAS_LOCK to each flash, and get rid of the manufacturer
generalization.

>  };
> 
>  static void intel_default_init(struct spi_nor *nor)
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 8b169fa4102a..5e4450877d66 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -11,26 +11,27 @@
>  static const struct flash_info sst_parts[] = {
>         /* SST -- large erase sizes are "overlays", "sectors" are 4K */
>         { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> -       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
> +       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
> +                             SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },

Looks like BP3 is needed here.

>         { "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | SPI_NOR_HAS_LOCK) },
>         { "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_HAS_LOCK) },

These two flashes have just two BP bits located at bit 2 and 3. Probably will work.

>         { "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16,
> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) },
> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) },
>         { "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,
>                               SECT_4K | SPI_NOR_DUAL_READ |
>                               SPI_NOR_QUAD_READ) },
> --
> 2.20.1
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Am 2020-11-25 13:21, schrieb Tudor.Ambarus@microchip.com:
[..]
>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>> index 49d392c6c8bc..19665095aeab 100644
>> --- a/drivers/mtd/spi-nor/atmel.c
>> +++ b/drivers/mtd/spi-nor/atmel.c
>> @@ -8,23 +8,120 @@
>> 
>>  #include "core.h"
>> 
>> +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2)
>> +
>> +/**
>> + * atmel_set_global_protection - Do a Global Protect or Unprotect 
>> command
>> + * @nor:       pointer to 'struct spi_nor'
>> + * @ofs:       offset in bytes
>> + * @len:       len in bytes
>> + * @is_protect:        if true do a Global Protect otherwise it is a 
>> Global Unprotect
>> + *
>> + * Return: 0 on success, -error otherwise.
>> + */
>> +static int atmel_set_global_protection(struct spi_nor *nor, loff_t 
>> ofs,
>> +                                      uint64_t len, bool is_protect)
>> +{
>> +       int ret;
>> +       u8 sr;
>> +
>> +       /* We only support locking the whole flash array */
>> +       if (ofs || len != nor->params->size)
>> +               return -EINVAL;
>> +
>> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> +       if (ret)
>> +               return ret;
>> +       sr = nor->bouncebuf[0];
>> +
>> +       /* SRWD bit needs to be cleared, otherwise the protection 
>> doesn't change */
>> +       if (sr & SR_SRWD) {
>> +               sr &= ~SR_SRWD;
>> +               ret = spi_nor_write_sr_and_check(nor, sr);
>> +               if (ret) {
>> +                       dev_err(nor->dev, "unable to clear SRWD bit, 
>> WP# asserted?\n");
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       if (is_protect)
>> +               sr |= ATMEL_SR_GLOBAL_PROTECT_MASK;
>> +       else
>> +               sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK;
>> +
>> +       return spi_nor_write_sr_and_check(nor, sr);
>> +}
>> +
>> +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, 
>> uint64_t len)
>> +{
>> +       return atmel_set_global_protection(nor, ofs, len, true);
>> +}
>> +
>> +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, 
>> uint64_t len)
>> +{
>> +       return atmel_set_global_protection(nor, ofs, len, false);
>> +}
>> +
>> +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, 
>> uint64_t len)
>> +{
>> +       int ret;
>> +
>> +       if (ofs >= nor->params->size || (ofs + len) > 
>> nor->params->size)
>> +               return -EINVAL;
>> +
>> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == 
>> ATMEL_SR_GLOBAL_PROTECT_MASK);
>> +}
>> +
>> +static const struct spi_nor_locking_ops atmel_global_protection_ops = 
>> {
>> +       .lock = atmel_global_protect,
>> +       .unlock = atmel_global_unprotect,
>> +       .is_locked = atmel_is_global_protected,
>> +};
> 
> Skimming through my notes in 1/3, I'm guessing this will not work for 
> any
> of the atmel flashes that we currently have.
> 
> Can we instead return -EOPNOTSUPP for .is_locked and .lock and just
> write a 0x00 to SR
> for the .unlock method?

As mentioned in my previous reply to 1/3 I don't think it is enough to
just write 0 because you first have to disable the SPRL.

For the other ops, for what atmel flash do you think this won't work?

All the flashes which uses these ops were marked as "Global
Protect/Unprotect same as in at25df041a." by you. So I think it should
work on all of these. Or am I missing something here?

[..]

>> diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c
>> index c93170008118..c2ebf29d95f2 100644
>> --- a/drivers/mtd/spi-nor/esmt.c
>> +++ b/drivers/mtd/spi-nor/esmt.c
>> @@ -11,9 +11,13 @@
>>  static const struct flash_info esmt_parts[] = {
>>         /* ESMT */
>>         { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64,
>> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
>> +                          SECT_4K | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32PA.pdf
> BP GENMASK(4,2), volatile, ok
> 
>>         { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64,
>> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
>> +                          SECT_4K | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> https://datasheetspdf.com/pdf-file/796196/ESMT/F25L32QA/1
> Datasheet states that "BP0~3, QE and BPL bits are non-volatile."
> At the same time, it says: "After power-up, BP3, BP2, BP1 and BP0 bits
> are set to 0."

Mhh I had this datasheet:
https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32QA.pdf

In that one they are volatile.. but yours is a newer version. So I
guess the flashes with the PA suffix have volatile BP and the QA ones
have the non-volatile version.

> Maybe factory default setting for BPn is 0? Let's treat them as NV, as 
> in
> f25l64qa.

Yes will fix it.

> Do we need BP3?

Rather the top bottom bit. But that is outside of the scope of this 
patch.
And as per your rule, as I don't have this particular flash I cannot 
test
and thus couldn't add the TB bit (technically). But if you like I can do
another patch (outside of this series and after it is applied) which 
will
add the TB bit flag.

> 
>> +       /*
>> +        * According to the datasheet the BPn bits are non-volatile, 
>> whereas
>> +        * they are volatile for the smaller f25l32qa.
>> +        */
>>         { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128,
>>                            SECT_4K | SPI_NOR_HAS_LOCK) },
> 
> https://datasheetspdf.com/pdf-file/967488/EliteSemiconductor/F25L64QA/1
> BP GENMASK(5, 2), non-volatile.
> 
> BP3?

Same as F25L32QA.

> 
> 
>>  };
>> diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c
>> index d8196f101368..c45ea0ad01f0 100644
>> --- a/drivers/mtd/spi-nor/intel.c
>> +++ b/drivers/mtd/spi-nor/intel.c
>> @@ -10,9 +10,9 @@
>> 
>>  static const struct flash_info intel_parts[] = {
>>         /* Intel/Numonyx -- xxxs33b */
>> -       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
>> -       { "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
>> -       { "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
>> +       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 
>> SPI_NOR_WP_IS_VOLATILE) },
>> +       { "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 
>> SPI_NOR_WP_IS_VOLATILE) },
>> +       { "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> http://www.datasheet.hk/view_download.php?id=1561787&file=0282\qh25f016s33b8_649107.pdf
> BP GENMASK(4,2) all volatile, all set to 1 at power-up.
> 
> You can add SPI_NOR_HAS_LOCK to each flash, and get rid of the 
> manufacturer
> generalization.

ok

> 
>>  };
>> 
>>  static void intel_default_init(struct spi_nor *nor)
>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>> index 8b169fa4102a..5e4450877d66 100644
>> --- a/drivers/mtd/spi-nor/sst.c
>> +++ b/drivers/mtd/spi-nor/sst.c
>> @@ -11,26 +11,27 @@
>>  static const struct flash_info sst_parts[] = {
>>         /* SST -- large erase sizes are "overlays", "sectors" are 4K 
>> */
>>         { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> -       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>> +       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
>> +                             SECT_4K | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
> 
> Looks like BP3 is needed here.

https://ww1.microchip.com/downloads/en/DeviceDoc/20005036C.pdf

agreed. But again cannot test it. Would add it as a seperate patch
to this series. (or leave it like it is)

> 
>>         { "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>> },
>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | 
>> SPI_NOR_WP_IS_VOLATILE) },
>>         { "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
>>         { "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | 
>> SPI_NOR_HAS_LOCK) },
> 
> These two flashes have just two BP bits located at bit 2 and 3.
> Probably will work.

Mhh? What datasheet were you looking at? There are three BPs:
https://ww1.microchip.com/downloads/en/DeviceDoc/SST25WF040B-4-Mbit-1.8V-SPI-Serial-Flash-Data-Sheet-DS20005193E.pdf

Ahh here are the tables which only inidicate two. But there are three.
https://ww1.microchip.com/downloads/en/DeviceDoc/20005016C.pdf

And yes since the rework of the BP bits algorithm this should work
as expected. Its just because the flash is too small to actually fill
up all the BP bits.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

On 11/25/20 8:52 PM, Michael Walle wrote:
>> Looks like BP3 is needed here.
> 
> https://ww1.microchip.com/downloads/en/DeviceDoc/20005036C.pdf
> 
> agreed. But again cannot test it. Would add it as a seperate patch
> to this series. (or leave it like it is)

Separate patch for the TB/BP3 bits is fine. We should add a fixes tag
for the patch that introduced the bug:
commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling of block write protection")

cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Am 2020-11-25 19:52, schrieb Michael Walle:
> Am 2020-11-25 13:21, schrieb Tudor.Ambarus@microchip.com:

[..]

>>> diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c
>>> index c93170008118..c2ebf29d95f2 100644
>>> --- a/drivers/mtd/spi-nor/esmt.c
>>> +++ b/drivers/mtd/spi-nor/esmt.c
>>> @@ -11,9 +11,13 @@
>>>  static const struct flash_info esmt_parts[] = {
>>>         /* ESMT */
>>>         { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64,
>>> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
>>> +                          SECT_4K | SPI_NOR_HAS_LOCK | 
>>> SPI_NOR_WP_IS_VOLATILE) },
>> 
>> https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32PA.pdf
>> BP GENMASK(4,2), volatile, ok
>> 
>>>         { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64,
>>> -                          SECT_4K | SPI_NOR_HAS_LOCK) },
>>> +                          SECT_4K | SPI_NOR_HAS_LOCK | 
>>> SPI_NOR_WP_IS_VOLATILE) },
>> 
>> https://datasheetspdf.com/pdf-file/796196/ESMT/F25L32QA/1
>> Datasheet states that "BP0~3, QE and BPL bits are non-volatile."
>> At the same time, it says: "After power-up, BP3, BP2, BP1 and BP0 bits
>> are set to 0."
> 
> Mhh I had this datasheet:
> https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32QA.pdf
> 
> In that one they are volatile.. but yours is a newer version. So I
> guess the flashes with the PA suffix have volatile BP and the QA ones
> have the non-volatile version.
> 
>> Maybe factory default setting for BPn is 0? Let's treat them as NV, as 
>> in
>> f25l64qa.
> 
> Yes will fix it.
> 
>> Do we need BP3?
> 
> Rather the top bottom bit. But that is outside of the scope of this 
> patch.
> And as per your rule, as I don't have this particular flash I cannot 
> test
> and thus couldn't add the TB bit (technically). But if you like I can 
> do
> another patch (outside of this series and after it is applied) which 
> will
> add the TB bit flag.

I've had a closer look at this. The top/bottom behavior is different
to that what we support in spi_nor_sr_lock(). But on the upside, the
current code is correct; it just doesn't support the TB bit. So we can
only protect addresses starting from the top. No changes needed here.

>> 
>>> +       /*
>>> +        * According to the datasheet the BPn bits are non-volatile, 
>>> whereas
>>> +        * they are volatile for the smaller f25l32qa.
>>> +        */
>>>         { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128,
>>>                            SECT_4K | SPI_NOR_HAS_LOCK) },
>> 
>> https://datasheetspdf.com/pdf-file/967488/EliteSemiconductor/F25L64QA/1
>> BP GENMASK(5, 2), non-volatile.
>> 
>> BP3?
> 
> Same as F25L32QA.

[..]

>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>> index 8b169fa4102a..5e4450877d66 100644
>>> --- a/drivers/mtd/spi-nor/sst.c
>>> +++ b/drivers/mtd/spi-nor/sst.c
>>> @@ -11,26 +11,27 @@
>>>  static const struct flash_info sst_parts[] = {
>>>         /* SST -- large erase sizes are "overlays", "sectors" are 4K 
>>> */
>>>         { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>>         { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>>         { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>>         { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> -       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | 
>>> SPI_NOR_HAS_LOCK) },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>> +       { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
>>> +                             SECT_4K | SPI_NOR_HAS_LOCK | 
>>> SPI_NOR_WP_IS_VOLATILE) },
>> 
>> Looks like BP3 is needed here.
> 
> https://ww1.microchip.com/downloads/en/DeviceDoc/20005036C.pdf
> 
> agreed. But again cannot test it. Would add it as a seperate patch
> to this series. (or leave it like it is)

I'll look at this tomorrow.

-michael

>> 
>>>         { "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>>         { "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>>         { "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
>>> -                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) 
>>> },
>>> +                             SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK 
>>> | SPI_NOR_WP_IS_VOLATILE) },
>>>         { "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | 
>>> SPI_NOR_HAS_LOCK) },
>>>         { "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | 
>>> SPI_NOR_HAS_LOCK) },
>> 
>> These two flashes have just two BP bits located at bit 2 and 3.
>> Probably will work.
> 
> Mhh? What datasheet were you looking at? There are three BPs:
> https://ww1.microchip.com/downloads/en/DeviceDoc/SST25WF040B-4-Mbit-1.8V-SPI-Serial-Flash-Data-Sheet-DS20005193E.pdf
> 
> Ahh here are the tables which only inidicate two. But there are three.
> https://ww1.microchip.com/downloads/en/DeviceDoc/20005016C.pdf
> 
> And yes since the rework of the BP bits algorithm this should work
> as expected. Its just because the flash is too small to actually fill
> up all the BP bits.
> 
> -michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 0/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle]

Am 2020-10-03 17:32, schrieb Michael Walle:
> I bundled this as a series, because otherwise there will be conflicts
> because the "remove global protection flag" patches modify the same 
> lines
> as the main patch.
> 
> See invdividual patches for the version history.

any news here?

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 0/3] mtd: spi-nor: keep lock bits if they are non-volatile

[Vignesh Raghavendra]

On 10/28/20 3:56 AM, Michael Walle wrote:
> Am 2020-10-03 17:32, schrieb Michael Walle:
>> I bundled this as a series, because otherwise there will be conflicts
>> because the "remove global protection flag" patches modify the same lines
>> as the main patch.
>>
>> See invdividual patches for the version history.
> 
> any news here?
> 

LGTM, thanks!... Will wait for Tudor's Ack before applying.

Regards
Vignesh



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/