Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK

[Tudor.Ambarus]

On 10/1/20 5:37 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2020-10-01 16:25, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
>> On 10/1/20 5:12 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2020-10-01 16:06, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
>>>> On 10/1/20 3:28 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 don'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
>>>>>
>>>>> Signed-off-by: Michael Walle <michael@xxxxxxxx>
>>>>> ---
>>>>>  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) },
>>>>
>>>> after a quick look in the datasheets of these flashes, I suspect that
>>>> what we have now in the SPI NOR core for SR locking does not work for
>>>> them. They probably supported just "unlock all", clearing all the
>>>> BP bits. Anyway, different problem.
>>>>>
>>>>> -       { "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) },
>>>>
>>>> this one does not support BP locking:
>>>> https://www.adestotech.com/wp-content/uploads/doc3668.pdf
>>>>
>>>>> +       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> neither this one:
>>>> https://datasheet.octopart.com/AT25DF321-S3U-Atmel-datasheet-8700896.pdf
>>>>
>>>>> +       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> nor this one:
>>>> https://www.adestotech.com/wp-content/uploads/doc3686.pdf
>>>>
>>>>> +       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> nor this one:
>>>> https://www.adestotech.com/wp-content/uploads/doc3680.pdf
>>>>
>>>> I stop here.
>>>
>>> These are all the ones which use the global unlock. I cannot just skip
>>> the HAS_LOCK bit here, because otherwise this patch wouldn't be
>>> backwards
>>> compatibe. Yes I missed that in the commit log, my bad.
>>>
>>
>> No worries.
>>
>> "unlock all at boot" just cleared the SR bits. Clearing the SR bits
>> unlocks
>> these flashes?
> 
> Clearing bits 5,4,3,2, yes (with SPRL=0)

oh, the horror. Those bits are described as Read Only when describing
Status Register. I'll re-read the datasheets.

> 
>   Conversely, to perform 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 first byte of the Status Register.

OK. I see this under "Global unprotect" section. Strange. Will get back to you.

Cheers,
ta
> 
> This will hopefully be cleaned up by my "mtd: spi-nor: keep lock bits if
> they
> are non-volatile" patch.
> 
> -michael