Message-ID: <a8e086ac-a5f8-f422-82e1-758c7da794b4@microchip.com>
Date:   Thu, 26 Nov 2020 12:45:41 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
        <boris.brezillon@...labora.com>
Subject: Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag

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@...rochip.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@...le.cc>
>>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@...rochip.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

Powered by blists - more mailing lists