From: <Tudor.Ambarus@microchip.com> To: <michael@walle.cc>, <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org> Cc: <miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>, <boris.brezillon@collabora.com> Subject: Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag Date: Tue, 24 Nov 2020 19:09:09 +0000 [thread overview] Message-ID: <7b5159c1-5457-b43c-2bf2-1a17ed6df34a@microchip.com> (raw) In-Reply-To: <20201003153235.29762-2-michael@walle.cc> 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@walle.cc> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.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. 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 >
WARNING: multiple messages have this Message-ID (diff)
From: <Tudor.Ambarus@microchip.com> To: <michael@walle.cc>, <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org> Cc: richard@nod.at, boris.brezillon@collabora.com, vigneshr@ti.com, miquel.raynal@bootlin.com Subject: Re: [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag Date: Tue, 24 Nov 2020 19:09:09 +0000 [thread overview] Message-ID: <7b5159c1-5457-b43c-2bf2-1a17ed6df34a@microchip.com> (raw) In-Reply-To: <20201003153235.29762-2-michael@walle.cc> 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@walle.cc> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.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. 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 > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-11-24 19:09 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-03 15:32 [PATCH v5 0/3] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 2020-10-03 15:32 ` Michael Walle 2020-10-03 15:32 ` [PATCH v5 1/3] mtd: spi-nor: atmel: remove global protection flag Michael Walle 2020-10-03 15:32 ` Michael Walle 2020-11-24 19:09 ` Tudor.Ambarus [this message] 2020-11-24 19:09 ` Tudor.Ambarus 2020-11-25 18:17 ` Michael Walle 2020-11-25 18:17 ` Michael Walle 2020-11-26 12:45 ` Tudor.Ambarus 2020-11-26 12:45 ` Tudor.Ambarus 2020-11-26 12:59 ` Tudor.Ambarus 2020-11-26 12:59 ` Tudor.Ambarus 2020-11-26 16:42 ` Tudor.Ambarus 2020-11-26 16:42 ` Tudor.Ambarus 2020-11-26 18:44 ` Michael Walle 2020-11-26 18:44 ` Michael Walle 2020-10-03 15:32 ` [PATCH v5 2/3] mtd: spi-nor: sst: " Michael Walle 2020-10-03 15:32 ` Michael Walle 2020-11-24 19:50 ` Tudor.Ambarus 2020-11-24 19:50 ` Tudor.Ambarus 2020-10-03 15:32 ` [PATCH v5 3/3] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 2020-10-03 15:32 ` Michael Walle 2020-11-25 12:21 ` Tudor.Ambarus 2020-11-25 12:21 ` Tudor.Ambarus 2020-11-25 18:52 ` Michael Walle 2020-11-25 18:52 ` Michael Walle 2020-11-26 16:47 ` Tudor.Ambarus 2020-11-26 16:47 ` Tudor.Ambarus 2020-11-26 20:46 ` Michael Walle 2020-11-26 20:46 ` Michael Walle 2020-10-27 22:26 ` [PATCH v5 0/3] " Michael Walle 2020-10-27 22:26 ` Michael Walle 2020-11-10 13:07 ` Vignesh Raghavendra 2020-11-10 13:07 ` Vignesh Raghavendra
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7b5159c1-5457-b43c-2bf2-1a17ed6df34a@microchip.com \ --to=tudor.ambarus@microchip.com \ --cc=boris.brezillon@collabora.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=michael@walle.cc \ --cc=miquel.raynal@bootlin.com \ --cc=richard@nod.at \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.