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

[Tudor.Ambarus]

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.

> 
>         { "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
>