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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Tudor Ambarus <[email protected]>
> ---
> 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
>
Am 2020-11-24 20:09, schrieb [email protected]:
> 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 <[email protected]>
>
> Reviewed-by: Tudor Ambarus <[email protected]>
>
>> ---
>> 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
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 [email protected]:
>> 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 <[email protected]>
>>
>> Reviewed-by: Tudor Ambarus <[email protected]>
>>
>>> ---
>>> 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
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 [email protected]:
>>> 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 <[email protected]>
>>>
>>> Reviewed-by: Tudor Ambarus <[email protected]>
>>>
>>>> ---
>>>> 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.
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 [email protected]:
>> 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 <[email protected]>
>>
>> Reviewed-by: Tudor Ambarus <[email protected]>
>>
>>> ---
>>> 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
Am 2020-11-26 17:42, schrieb [email protected]:
> 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 [email protected]:
>>> 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 <[email protected]>
>>>
>>> Reviewed-by: Tudor Ambarus <[email protected]>
>>>
>>>> ---
>>>> 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