Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
@ 2020-10-01 12:28 Michael Walle
  2020-10-01 12:28 ` [RFC PATCH 2/2] mtd: spi-nor: sst: " Michael Walle
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Michael Walle @ 2020-10-01 12:28 UTC (permalink / raw)
  To: Tudor Ambarus, linux-mtd, linux-arm-kernel, linux-kernel
  Cc: Richard Weinberger, Michael Walle, Vignesh Raghavendra, Miquel Raynal

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@walle.cc>
---
 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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [RFC PATCH 2/2] mtd: spi-nor: sst: remove global SNOR_F_HAS_LOCK
  2020-10-01 12:28 [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK Michael Walle
@ 2020-10-01 12:28 ` Michael Walle
  2020-10-01 13:21 ` [RFC PATCH 1/2] mtd: spi-nor: atmel: " Tudor.Ambarus
  2020-10-01 14:06 ` Tudor.Ambarus
  2 siblings, 0 replies; 8+ messages in thread
From: Michael Walle @ 2020-10-01 12:28 UTC (permalink / raw)
  To: Tudor Ambarus, linux-mtd, linux-arm-kernel, linux-kernel
  Cc: Richard Weinberger, Michael Walle, Vignesh Raghavendra, Miquel Raynal

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 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:
 - SST26VF016B
 - SST26WF016B
 - SST26VF064B

Signed-off-by: Michael Walle <michael@walle.cc>
---
 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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
  2020-10-01 12:28 [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK Michael Walle
  2020-10-01 12:28 ` [RFC PATCH 2/2] mtd: spi-nor: sst: " Michael Walle
@ 2020-10-01 13:21 ` Tudor.Ambarus
  2020-10-01 14:06 ` Tudor.Ambarus
  2 siblings, 0 replies; 8+ messages in thread
From: Tudor.Ambarus @ 2020-10-01 13:21 UTC (permalink / raw)
  To: michael, linux-mtd, linux-arm-kernel, linux-kernel
  Cc: richard, vigneshr, miquel.raynal

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
> 

I like the idea. Thanks for the effort. Will check all those datasheets
and get back to you.

> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
  2020-10-01 12:28 [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK Michael Walle
  2020-10-01 12:28 ` [RFC PATCH 2/2] mtd: spi-nor: sst: " Michael Walle
  2020-10-01 13:21 ` [RFC PATCH 1/2] mtd: spi-nor: atmel: " Tudor.Ambarus
@ 2020-10-01 14:06 ` Tudor.Ambarus
  2020-10-01 14:12   ` Michael Walle
  2 siblings, 1 reply; 8+ messages in thread
From: Tudor.Ambarus @ 2020-10-01 14:06 UTC (permalink / raw)
  To: michael, linux-mtd, linux-arm-kernel, linux-kernel
  Cc: richard, vigneshr, miquel.raynal

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@walle.cc>
> ---
>  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
> 

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
  2020-10-01 14:06 ` Tudor.Ambarus
@ 2020-10-01 14:12   ` Michael Walle
  2020-10-01 14:25     ` Tudor.Ambarus
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Walle @ 2020-10-01 14:12 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: vigneshr, richard, linux-kernel, linux-mtd, miquel.raynal,
	linux-arm-kernel

Am 2020-10-01 16:06, schrieb Tudor.Ambarus@microchip.com:
> 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@walle.cc>
>> ---
>>  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.

-michael

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
  2020-10-01 14:12   ` Michael Walle
@ 2020-10-01 14:25     ` Tudor.Ambarus
  2020-10-01 14:37       ` Michael Walle
  0 siblings, 1 reply; 8+ messages in thread
From: Tudor.Ambarus @ 2020-10-01 14:25 UTC (permalink / raw)
  To: michael
  Cc: vigneshr, richard, linux-kernel, linux-mtd, miquel.raynal,
	linux-arm-kernel

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@microchip.com:
>> 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@walle.cc>
>>> ---
>>>  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?

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
  2020-10-01 14:25     ` Tudor.Ambarus
@ 2020-10-01 14:37       ` Michael Walle
  2020-10-01 15:25         ` Tudor.Ambarus
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Walle @ 2020-10-01 14:37 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: vigneshr, richard, linux-kernel, linux-mtd, miquel.raynal,
	linux-arm-kernel

Am 2020-10-01 16:25, schrieb Tudor.Ambarus@microchip.com:
> 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@microchip.com:
>>> 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@walle.cc>
>>>> ---
>>>>  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)

   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.

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

-michael

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK
  2020-10-01 14:37       ` Michael Walle
@ 2020-10-01 15:25         ` Tudor.Ambarus
  0 siblings, 0 replies; 8+ messages in thread
From: Tudor.Ambarus @ 2020-10-01 15:25 UTC (permalink / raw)
  To: michael
  Cc: vigneshr, richard, linux-kernel, linux-mtd, miquel.raynal,
	linux-arm-kernel

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@microchip.com:
>> 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@microchip.com:
>>>> 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@walle.cc>
>>>>> ---
>>>>>  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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 12:28 [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS_LOCK Michael Walle
2020-10-01 12:28 ` [RFC PATCH 2/2] mtd: spi-nor: sst: " Michael Walle
2020-10-01 13:21 ` [RFC PATCH 1/2] mtd: spi-nor: atmel: " Tudor.Ambarus
2020-10-01 14:06 ` Tudor.Ambarus
2020-10-01 14:12   ` Michael Walle
2020-10-01 14:25     ` Tudor.Ambarus
2020-10-01 14:37       ` Michael Walle
2020-10-01 15:25         ` Tudor.Ambarus

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git