* [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile @ 2020-11-26 20:26 Michael Walle 2020-11-26 20:26 ` [PATCH v6 1/5] mtd: spi-nor: atmel: remove global protection flag Michael Walle ` (4 more replies) 0 siblings, 5 replies; 15+ messages in thread From: Michael Walle @ 2020-11-26 20:26 UTC (permalink / raw) To: linux-mtd, linux-kernel Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Boris Brezillon, Michael Walle I bundled this as a series, because otherwise there will be conflicts because the "remove global protection flag" patches modify the same lines as the main patch. See invdividual patches for the version history. Michael Walle (5): mtd: spi-nor: atmel: remove global protection flag mtd: spi-nor: sst: remove global protection flag mtd: spi-nor: intel: remove global protection flag mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 mtd: spi-nor: keep lock bits if they are non-volatile drivers/mtd/spi-nor/Kconfig | 42 ++++++++ drivers/mtd/spi-nor/atmel.c | 190 ++++++++++++++++++++++++++++++++---- drivers/mtd/spi-nor/core.c | 38 +++++--- drivers/mtd/spi-nor/core.h | 9 ++ drivers/mtd/spi-nor/esmt.c | 2 +- drivers/mtd/spi-nor/intel.c | 19 ++-- drivers/mtd/spi-nor/sst.c | 31 +++--- 7 files changed, 268 insertions(+), 63 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 1/5] mtd: spi-nor: atmel: remove global protection flag 2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle @ 2020-11-26 20:26 ` Michael Walle 2020-11-26 20:26 ` [PATCH v6 2/5] mtd: spi-nor: sst: " Michael Walle ` (3 subsequent siblings) 4 siblings, 0 replies; 15+ messages in thread From: Michael Walle @ 2020-11-26 20:26 UTC (permalink / raw) To: linux-mtd, linux-kernel Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Boris Brezillon, Michael Walle 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 v5: - none 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) }, + { "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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v6 2/5] mtd: spi-nor: sst: remove global protection flag 2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 2020-11-26 20:26 ` [PATCH v6 1/5] mtd: spi-nor: atmel: remove global protection flag Michael Walle @ 2020-11-26 20:26 ` Michael Walle 2020-11-26 20:26 ` [PATCH v6 3/5] mtd: spi-nor: intel: " Michael Walle ` (2 subsequent siblings) 4 siblings, 0 replies; 15+ messages in thread From: Michael Walle @ 2020-11-26 20:26 UTC (permalink / raw) To: linux-mtd, linux-kernel Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Boris Brezillon, Michael Walle 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 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: - SST26VF016B - SST26WF016B - SST26VF064B Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- changes since v5: - none 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: - none 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v6 3/5] mtd: spi-nor: intel: remove global protection flag 2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 2020-11-26 20:26 ` [PATCH v6 1/5] mtd: spi-nor: atmel: remove global protection flag Michael Walle 2020-11-26 20:26 ` [PATCH v6 2/5] mtd: spi-nor: sst: " Michael Walle @ 2020-11-26 20:26 ` Michael Walle 2020-11-27 9:07 ` Tudor.Ambarus 2020-11-26 20:26 ` [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 Michael Walle 2020-11-26 20:26 ` [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 4 siblings, 1 reply; 15+ messages in thread From: Michael Walle @ 2020-11-26 20:26 UTC (permalink / raw) To: linux-mtd, linux-kernel Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Boris Brezillon, Michael Walle For the Atmel and SST parts this flag was already moved to individual flash parts because it is considered bad esp. because newer flash chips will automatically inherit the "has locking" support. While this won't likely be the case for the Intel parts, we do it for consistency reasons. Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v5 - new patch drivers/mtd/spi-nor/intel.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c index d8196f101368..6c31bef3fc60 100644 --- a/drivers/mtd/spi-nor/intel.c +++ b/drivers/mtd/spi-nor/intel.c @@ -10,23 +10,13 @@ static const struct flash_info intel_parts[] = { /* Intel/Numonyx -- xxxs33b */ - { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, 0) }, - { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, 0) }, - { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, 0) }, -}; - -static void intel_default_init(struct spi_nor *nor) -{ - nor->flags |= SNOR_F_HAS_LOCK; -} - -static const struct spi_nor_fixups intel_fixups = { - .default_init = intel_default_init, + { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, SPI_NOR_HAS_LOCK) }, + { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, SPI_NOR_HAS_LOCK) }, + { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_HAS_LOCK) }, }; const struct spi_nor_manufacturer spi_nor_intel = { .name = "intel", .parts = intel_parts, .nparts = ARRAY_SIZE(intel_parts), - .fixups = &intel_fixups, }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/5] mtd: spi-nor: intel: remove global protection flag 2020-11-26 20:26 ` [PATCH v6 3/5] mtd: spi-nor: intel: " Michael Walle @ 2020-11-27 9:07 ` Tudor.Ambarus 0 siblings, 0 replies; 15+ messages in thread From: Tudor.Ambarus @ 2020-11-27 9:07 UTC (permalink / raw) To: michael, linux-mtd, linux-kernel Cc: miquel.raynal, richard, vigneshr, boris.brezillon On 11/26/20 10:26 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > For the Atmel and SST parts this flag was already moved to individual > flash parts because it is considered bad esp. because newer flash chips > will automatically inherit the "has locking" support. While this won't > likely be the case for the Intel parts, we do it for consistency > reasons. > > Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > changes since v5 > - new patch > > drivers/mtd/spi-nor/intel.c | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c > index d8196f101368..6c31bef3fc60 100644 > --- a/drivers/mtd/spi-nor/intel.c > +++ b/drivers/mtd/spi-nor/intel.c > @@ -10,23 +10,13 @@ > > static const struct flash_info intel_parts[] = { > /* Intel/Numonyx -- xxxs33b */ > - { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, 0) }, > - { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, 0) }, > - { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, 0) }, > -}; > - > -static void intel_default_init(struct spi_nor *nor) > -{ > - nor->flags |= SNOR_F_HAS_LOCK; > -} > - > -static const struct spi_nor_fixups intel_fixups = { > - .default_init = intel_default_init, > + { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, SPI_NOR_HAS_LOCK) }, > + { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, SPI_NOR_HAS_LOCK) }, > + { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_HAS_LOCK) }, > }; > > const struct spi_nor_manufacturer spi_nor_intel = { > .name = "intel", > .parts = intel_parts, > .nparts = ARRAY_SIZE(intel_parts), > - .fixups = &intel_fixups, > }; > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle ` (2 preceding siblings ...) 2020-11-26 20:26 ` [PATCH v6 3/5] mtd: spi-nor: intel: " Michael Walle @ 2020-11-26 20:26 ` Michael Walle 2020-11-28 8:25 ` Tudor.Ambarus 2020-11-26 20:26 ` [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 4 siblings, 1 reply; 15+ messages in thread From: Michael Walle @ 2020-11-26 20:26 UTC (permalink / raw) To: linux-mtd, linux-kernel Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Boris Brezillon, Michael Walle These flashes have some weird BP bits mapping which aren't supported in the current locking code. Just add a simple unlock op to unprotect the entire flash array which is needed for legacy behavior. Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v5 - new patch drivers/mtd/spi-nor/atmel.c | 53 +++++++++++++++++++++++++++++++++++-- drivers/mtd/spi-nor/core.c | 2 +- drivers/mtd/spi-nor/core.h | 1 + 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c index 49d392c6c8bc..fe6a4653823d 100644 --- a/drivers/mtd/spi-nor/atmel.c +++ b/drivers/mtd/spi-nor/atmel.c @@ -8,10 +8,59 @@ #include "core.h" +/* + * The Atmel AT25FS010/AT25FS040 parts have some weird configuration for the + * block protection bits. We don't support them. But legacy behaviour in linux + * is to unlock the whole flash array on startup. Therefore, we have to support + * exactly this operation. + */ +static int atmel_at25fs_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + return -EOPNOTSUPP; +} + +static int atmel_at25fs_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + /* We only support unlocking the whole flash array */ + if (ofs || len != nor->params->size) + return -EINVAL; + + /* + * Write 0x00 to the status register to try to disable the write + * protection. This will fail if SRWD (the datasheet calls it WPEN) is + * set. But there is nothing we can do. + */ + nor->bouncebuf[0] = 0; + + return spi_nor_write_sr(nor, nor->bouncebuf, 1); +} + +static int atmel_at25fs_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + return -EOPNOTSUPP; +} + +static const struct spi_nor_locking_ops atmel_at25fs_locking_ops = { + .lock = atmel_at25fs_lock, + .unlock = atmel_at25fs_unlock, + .is_locked = atmel_at25fs_is_locked, +}; + +static void atmel_at25fs_default_init(struct spi_nor *nor) +{ + nor->params->locking_ops = &atmel_at25fs_locking_ops; +} + +static const struct spi_nor_fixups atmel_at25fs_fixups = { + .default_init = atmel_at25fs_default_init, +}; + static const struct flash_info atmel_parts[] = { /* Atmel -- some are (confusingly) marketed as "DataFlash" */ - { "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) }, + { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK) + .fixups = &atmel_at25fs_fixups }, + { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK) + .fixups = &atmel_at25fs_fixups }, { "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) }, diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 5bee7c8da4dc..8c06a28a90de 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -861,7 +861,7 @@ int spi_nor_wait_till_ready(struct spi_nor *nor) * * Return: 0 on success, -errno otherwise. */ -static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) +int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) { int ret; diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 0a775a7b5606..16b350e1d865 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -430,6 +430,7 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); +int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len); int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr); ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 2020-11-26 20:26 ` [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 Michael Walle @ 2020-11-28 8:25 ` Tudor.Ambarus 2020-11-30 14:16 ` Michael Walle 0 siblings, 1 reply; 15+ messages in thread From: Tudor.Ambarus @ 2020-11-28 8:25 UTC (permalink / raw) To: michael, linux-mtd, linux-kernel Cc: miquel.raynal, richard, vigneshr, boris.brezillon On 11/26/20 10:26 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > These flashes have some weird BP bits mapping which aren't supported in > the current locking code. Just add a simple unlock op to unprotect the > entire flash array which is needed for legacy behavior. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > changes since v5 > - new patch > > drivers/mtd/spi-nor/atmel.c | 53 +++++++++++++++++++++++++++++++++++-- > drivers/mtd/spi-nor/core.c | 2 +- > drivers/mtd/spi-nor/core.h | 1 + > 3 files changed, 53 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c > index 49d392c6c8bc..fe6a4653823d 100644 > --- a/drivers/mtd/spi-nor/atmel.c > +++ b/drivers/mtd/spi-nor/atmel.c > @@ -8,10 +8,59 @@ > > #include "core.h" > > +/* > + * The Atmel AT25FS010/AT25FS040 parts have some weird configuration for the > + * block protection bits. We don't support them. But legacy behaviour in linux > + * is to unlock the whole flash array on startup. Therefore, we have to support > + * exactly this operation. > + */ > +static int atmel_at25fs_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) > +{ > + return -EOPNOTSUPP; > +} > + > +static int atmel_at25fs_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) > +{ > + /* We only support unlocking the whole flash array */ > + if (ofs || len != nor->params->size) > + return -EINVAL; > + > + /* > + * Write 0x00 to the status register to try to disable the write > + * protection. This will fail if SRWD (the datasheet calls it WPEN) is > + * set. But there is nothing we can do. > + */ can't we do the same as you did in 5/5? + ret = spi_nor_read_sr(nor, nor->bouncebuf); + if (ret) + return ret; + + sr = nor->bouncebuf[0]; + + if (sr & SR_SRWD) { + sr &= ~SR_SRWD; + ret = spi_nor_write_sr_and_check(nor, sr); + if (ret) + return ret; + } If SRWD is set to 1, we first try to set it to 0. If WP# is asserted, we will catch this in spi_nor_write_sr_and_check() > + nor->bouncebuf[0] = 0; > + > + return spi_nor_write_sr(nor, nor->bouncebuf, 1); and then you can use spi_nor_write_sr_and_check here > +} > + > +static int atmel_at25fs_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len) > +{ > + return -EOPNOTSUPP; > +} > + > +static const struct spi_nor_locking_ops atmel_at25fs_locking_ops = { > + .lock = atmel_at25fs_lock, > + .unlock = atmel_at25fs_unlock, > + .is_locked = atmel_at25fs_is_locked, > +}; > + > +static void atmel_at25fs_default_init(struct spi_nor *nor) > +{ > + nor->params->locking_ops = &atmel_at25fs_locking_ops; > +} > + > +static const struct spi_nor_fixups atmel_at25fs_fixups = { > + .default_init = atmel_at25fs_default_init, > +}; > + > static const struct flash_info atmel_parts[] = { > /* Atmel -- some are (confusingly) marketed as "DataFlash" */ > - { "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) }, > + { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK) > + .fixups = &atmel_at25fs_fixups }, > + { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK) > + .fixups = &atmel_at25fs_fixups }, > > { "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) }, > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 5bee7c8da4dc..8c06a28a90de 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -861,7 +861,7 @@ int spi_nor_wait_till_ready(struct spi_nor *nor) > * > * Return: 0 on success, -errno otherwise. > */ > -static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) > +int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) > { > int ret; > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 0a775a7b5606..16b350e1d865 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -430,6 +430,7 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); > +int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len); > > int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr); > ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 2020-11-28 8:25 ` Tudor.Ambarus @ 2020-11-30 14:16 ` Michael Walle 2020-12-02 10:32 ` Tudor.Ambarus 0 siblings, 1 reply; 15+ messages in thread From: Michael Walle @ 2020-11-30 14:16 UTC (permalink / raw) To: Tudor.Ambarus Cc: linux-mtd, linux-kernel, miquel.raynal, richard, vigneshr, boris.brezillon Am 2020-11-28 09:25, schrieb Tudor.Ambarus@microchip.com: > On 11/26/20 10:26 PM, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> These flashes have some weird BP bits mapping which aren't supported >> in >> the current locking code. Just add a simple unlock op to unprotect the >> entire flash array which is needed for legacy behavior. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> changes since v5 >> - new patch >> >> drivers/mtd/spi-nor/atmel.c | 53 >> +++++++++++++++++++++++++++++++++++-- >> drivers/mtd/spi-nor/core.c | 2 +- >> drivers/mtd/spi-nor/core.h | 1 + >> 3 files changed, 53 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c >> index 49d392c6c8bc..fe6a4653823d 100644 >> --- a/drivers/mtd/spi-nor/atmel.c >> +++ b/drivers/mtd/spi-nor/atmel.c >> @@ -8,10 +8,59 @@ >> >> #include "core.h" >> >> +/* >> + * The Atmel AT25FS010/AT25FS040 parts have some weird configuration >> for the >> + * block protection bits. We don't support them. But legacy behaviour >> in linux >> + * is to unlock the whole flash array on startup. Therefore, we have >> to support >> + * exactly this operation. >> + */ >> +static int atmel_at25fs_lock(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +static int atmel_at25fs_unlock(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + /* We only support unlocking the whole flash array */ >> + if (ofs || len != nor->params->size) >> + return -EINVAL; >> + >> + /* >> + * Write 0x00 to the status register to try to disable the >> write >> + * protection. This will fail if SRWD (the datasheet calls it >> WPEN) is >> + * set. But there is nothing we can do. >> + */ > > can't we do the same as you did in 5/5? Sure, but - assuming it is only used for the legacy unlock all operation - the outcome will be the same. It will either keep being locked or all will be unlocked. That being said, I can also change it to the same as the global_unprotect(). I don't have any option on that other than this is simpler. -michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 2020-11-30 14:16 ` Michael Walle @ 2020-12-02 10:32 ` Tudor.Ambarus 0 siblings, 0 replies; 15+ messages in thread From: Tudor.Ambarus @ 2020-12-02 10:32 UTC (permalink / raw) To: michael Cc: linux-mtd, linux-kernel, miquel.raynal, richard, vigneshr, boris.brezillon On 11/30/20 4:16 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2020-11-28 09:25, schrieb Tudor.Ambarus@microchip.com: >> On 11/26/20 10:26 PM, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> These flashes have some weird BP bits mapping which aren't supported >>> in >>> the current locking code. Just add a simple unlock op to unprotect the >>> entire flash array which is needed for legacy behavior. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> changes since v5 >>> - new patch >>> >>> drivers/mtd/spi-nor/atmel.c | 53 >>> +++++++++++++++++++++++++++++++++++-- >>> drivers/mtd/spi-nor/core.c | 2 +- >>> drivers/mtd/spi-nor/core.h | 1 + >>> 3 files changed, 53 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c >>> index 49d392c6c8bc..fe6a4653823d 100644 >>> --- a/drivers/mtd/spi-nor/atmel.c >>> +++ b/drivers/mtd/spi-nor/atmel.c >>> @@ -8,10 +8,59 @@ >>> >>> #include "core.h" >>> >>> +/* >>> + * The Atmel AT25FS010/AT25FS040 parts have some weird configuration >>> for the >>> + * block protection bits. We don't support them. But legacy behaviour >>> in linux >>> + * is to unlock the whole flash array on startup. Therefore, we have >>> to support >>> + * exactly this operation. >>> + */ >>> +static int atmel_at25fs_lock(struct spi_nor *nor, loff_t ofs, >>> uint64_t len) >>> +{ >>> + return -EOPNOTSUPP; >>> +} >>> + >>> +static int atmel_at25fs_unlock(struct spi_nor *nor, loff_t ofs, >>> uint64_t len) >>> +{ >>> + /* We only support unlocking the whole flash array */ >>> + if (ofs || len != nor->params->size) >>> + return -EINVAL; >>> + >>> + /* >>> + * Write 0x00 to the status register to try to disable the >>> write >>> + * protection. This will fail if SRWD (the datasheet calls it >>> WPEN) is >>> + * set. But there is nothing we can do. >>> + */ >> >> can't we do the same as you did in 5/5? > > Sure, but - assuming it is only used for the legacy unlock all operation > - the > outcome will be the same. It will either keep being locked or all will > be > unlocked. In case WP# is asserted, we'll catch this via the dev_dbg message in spi_nor_write_sr_and_check() when trying to clear the SRWD bit. We will have an idea of what's going on, instead of a silent fail to unlock. > > That being said, I can also change it to the same as the > global_unprotect(). > I don't have any option on that other than this is simpler. > > -michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile 2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle ` (3 preceding siblings ...) 2020-11-26 20:26 ` [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 Michael Walle @ 2020-11-26 20:26 ` Michael Walle 2020-11-28 10:17 ` Tudor.Ambarus 4 siblings, 1 reply; 15+ messages in thread From: Michael Walle @ 2020-11-26 20:26 UTC (permalink / raw) To: linux-mtd, linux-kernel Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Boris Brezillon, Michael Walle Traditionally, linux unlocks the whole flash because there are legacy devices which has the write protections bits set by default at startup. If you actually want to use the flash protection bits, eg. because there is a read-only part for a bootloader, this automatic unlocking is harmful. If there is no hardware write protection in place (usually called WP#), a startup of the kernel just discards this protection. I've gone through the datasheets of all the flashes (except the Intel ones where I could not find any datasheet nor reference) which supports the unlocking feature and looked how the sector protection was implemented. The currently supported flashes can be divided into the following two categories: (1) block protection bits are non-volatile. Thus they keep their values at reset and power-cycle (2) flashes where these bits are volatile. After reset or power-cycle, the whole memory array is protected. (a) some devices needs a special "Global Unprotect" command, eg. the Atmel AT25DF041A. (b) some devices require to clear the BPn bits in the status register. Due to the reasons above, we do not want to clear the bits for flashes which belong to category (1). Fortunately for us, only Atmel flashes fall into category (2a). Implement the "Global Protect" and "Global Unprotect" commands for these. For (2b) we can use normal block protection locking scheme. This patch adds a new flag to indicate the case (2). Only if we have such a flash we unlock the whole flash array. To be backwards compatible it also introduces a kernel configuration option which restores the complete legacy behavior ("Disable write protection on any flashes"). Hopefully, this will clean up "unlock the entire flash for legacy devices" once and for all. For reference here are the actually commits which introduced the legacy behaviour (and extended the behaviour to other chip manufacturers): commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx {16,32,64}0S33B SPI flash chips") commit ea60658a08f8f ("mtd: m25p80: disable SST software protection bits by default") commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails to be copied to") Actually, this might also fix handling of the Atmel AT25DF flashes, because the original commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails to be copied to") was writing a 0 to the status register, which is a "Global Unprotect". This might not be the case in the current code which only handles the block protection bits BP2, BP1 and BP0. Thus, it depends on the current contents of the status register if this unlock actually corresponds to a "Global Unprotect" command. In the worst case, the current code might leave the AT25DF flashes in a write protected state. The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when QE is set on spansion flashes") changed that behaviour by just clearing BP2 to BP0 instead of writing a 0 to the status register. Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling of block write protection") expanded the unlock_all() feature to ANY flash which supports locking. Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v5: - also set SRWD bit for the "Global Protect" command - use spi_nor_write_sr() instead of spi_nor_write_sr_and_check() to send the "Global Protect" or "Global Unprotect" command - mark ESMT F25L32QA as non-volatile as indicated in a newer datasheet revision - rebased to latest tree changes since v4: - made atmel_global_protection_default_init() static, spotted by lkp@intel.com changes since v3: - now defaulting to MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE, suggested by Vignesh - restored the original spi_nor_unlock_all(), instead add individual locking ops for the "Global Protect" scheme in atmel.c. This was tested partly with the AT25SL321 (for the test I added the fixups to this flash). - renamed SPI_NOR_UNPROTECT to SPI_NOR_WP_IS_VOLATILE. Suggested by Vingesh, although I've renamed it to a more general "WP_IS_VOLATILE" because either the BP bits or the individual sector locks might be volatile. - add mention of both block protection bits and "Global Unprotect" command in the Kconfig help text. changes since v2: - add Kconfig option to be able to retain legacy behaviour - rebased the patch due to the spi-nor rewrite - dropped the Fixes: tag, it doens't make sense after the spi-nor rewrite - mention commit 3e0930f109e76 which further modified the unlock behaviour. changes since v1: - completely rewrote patch, the first version used a device tree flag drivers/mtd/spi-nor/Kconfig | 42 ++++++++++++ drivers/mtd/spi-nor/atmel.c | 127 ++++++++++++++++++++++++++++++++++-- drivers/mtd/spi-nor/core.c | 36 ++++++---- drivers/mtd/spi-nor/core.h | 8 +++ drivers/mtd/spi-nor/esmt.c | 2 +- drivers/mtd/spi-nor/intel.c | 9 ++- drivers/mtd/spi-nor/sst.c | 21 +++--- 7 files changed, 213 insertions(+), 32 deletions(-) diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig index ffc4b380f2b1..11e6658ee85d 100644 --- a/drivers/mtd/spi-nor/Kconfig +++ b/drivers/mtd/spi-nor/Kconfig @@ -24,6 +24,48 @@ config MTD_SPI_NOR_USE_4K_SECTORS Please note that some tools/drivers/filesystems may not work with 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum). +choice + prompt "Write protection at boot" + default MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE + +config MTD_SPI_NOR_WP_DISABLE + bool "Disable WP on any flashes (legacy behaviour)" + help + This option disables the write protection on any SPI flashes at + boot-up. + + Depending on the flash chip this either clears the block protection + bits or does a "Global Unprotect" command. + + Don't use this if you intent to use the write protection of your + SPI flash. This is only to keep backwards compatibility. + +config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE + bool "Disable WP on flashes w/ volatile protection bits" + help + Some SPI flashes have volatile block protection bits, ie. after a + power-up or a reset the flash is write protected by default. + + This option disables the write protection for these kind of flashes + while keeping it enabled for any other SPI flashes which have + non-volatile write protection bits. + + If the write protection will be disabled depending on the flash + either the block protection bits are cleared or a "Global Unprotect" + command is issued. + + If you are unsure, select this option. + +config MTD_SPI_NOR_WP_KEEP + bool "Keep write protection as is" + help + If you select this option the write protection of any SPI flashes + will not be changed. If your flash is write protected or will be + automatically write protected after power-up you have to manually + unlock it before you are able to write to it. + +endchoice + source "drivers/mtd/spi-nor/controllers/Kconfig" endif # MTD_SPI_NOR diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c index fe6a4653823d..215df7c4272b 100644 --- a/drivers/mtd/spi-nor/atmel.c +++ b/drivers/mtd/spi-nor/atmel.c @@ -8,6 +8,8 @@ #include "core.h" +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2) + /* * The Atmel AT25FS010/AT25FS040 parts have some weird configuration for the * block protection bits. We don't support them. But legacy behaviour in linux @@ -55,6 +57,103 @@ static const struct spi_nor_fixups atmel_at25fs_fixups = { .default_init = atmel_at25fs_default_init, }; +/** + * atmel_set_global_protection - Do a Global Protect or Unprotect command + * @nor: pointer to 'struct spi_nor' + * @ofs: offset in bytes + * @len: len in bytes + * @is_protect: if true do a Global Protect otherwise it is a Global Unprotect + * + * Return: 0 on success, -error otherwise. + */ +static int atmel_set_global_protection(struct spi_nor *nor, loff_t ofs, + uint64_t len, bool is_protect) +{ + int ret; + u8 sr; + + /* We only support locking the whole flash array */ + if (ofs || len != nor->params->size) + return -EINVAL; + + ret = spi_nor_read_sr(nor, nor->bouncebuf); + if (ret) + return ret; + sr = nor->bouncebuf[0]; + + /* SRWD bit needs to be cleared, otherwise the protection doesn't change */ + if (sr & SR_SRWD) { + sr &= ~SR_SRWD; + ret = spi_nor_write_sr_and_check(nor, sr); + if (ret) { + dev_err(nor->dev, "unable to clear SRWD bit, WP# asserted?\n"); + return ret; + } + } + + if (is_protect) { + sr |= ATMEL_SR_GLOBAL_PROTECT_MASK; + /* + * Set the SRWD bit again as soon as we are protecting + * anything. This will ensure that the WP# pin is working + * correctly. By doing this we also behave the same as + * spi_nor_sr_lock(), which sets SRWD if any block protection + * is active. + */ + sr |= SR_SRWD; + } else { + sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK; + } + + nor->bouncebuf[0] = sr; + + /* + * We cannot use the spi_nor_write_sr_and_check() because this command + * isn't really setting any bits, instead it is an pseudo command for + * "Global Unprotect" or "Global Protect" + */ + return spi_nor_write_sr(nor, nor->bouncebuf, 1); +} + +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + return atmel_set_global_protection(nor, ofs, len, true); +} + +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + return atmel_set_global_protection(nor, ofs, len, false); +} + +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + int ret; + + if (ofs >= nor->params->size || (ofs + len) > nor->params->size) + return -EINVAL; + + ret = spi_nor_read_sr(nor, nor->bouncebuf); + if (ret) + return ret; + + return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == ATMEL_SR_GLOBAL_PROTECT_MASK); +} + +static const struct spi_nor_locking_ops atmel_global_protection_ops = { + .lock = atmel_global_protect, + .unlock = atmel_global_unprotect, + .is_locked = atmel_is_global_protected, +}; + +static void atmel_global_protection_default_init(struct spi_nor *nor) +{ + nor->params->locking_ops = &atmel_global_protection_ops; +} + +static const struct spi_nor_fixups atmel_global_protection_fixups = { + .default_init = atmel_global_protection_default_init, +}; + static const struct flash_info atmel_parts[] = { /* Atmel -- some are (confusingly) marketed as "DataFlash" */ { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK) @@ -62,18 +161,32 @@ static const struct flash_info atmel_parts[] = { { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK) .fixups = &atmel_at25fs_fixups }, - { "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) }, + { "at25df041a", INFO(0x1f4401, 0, 64 * 1024, 8, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, + { "at25df321", INFO(0x1f4700, 0, 64 * 1024, 64, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, + { "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, + { "at25df641", INFO(0x1f4800, 0, 64 * 1024, 128, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, { "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 | 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) }, + { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, + { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, + { "at26df321", INFO(0x1f4700, 0, 64 * 1024, 64, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) + .fixups = &atmel_global_protection_fixups }, { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, }; diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 8c06a28a90de..8354ce0c8810 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -377,7 +377,7 @@ int spi_nor_write_disable(struct spi_nor *nor) * * Return: 0 on success, -errno otherwise. */ -static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) { int ret; @@ -1049,7 +1049,7 @@ static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr) * * Return: 0 on success, -errno otherwise. */ -static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) { if (nor->flags & SNOR_F_HAS_16BIT_SR) return spi_nor_write_16bit_sr_and_check(nor, sr1); @@ -3124,15 +3124,14 @@ static int spi_nor_quad_enable(struct spi_nor *nor) * spi_nor_unlock_all() - Unlocks the entire flash memory array. * @nor: pointer to a 'struct spi_nor'. * - * Some SPI NOR flashes are write protected by default after a power-on reset - * cycle, in order to avoid inadvertent writes during power-up. Backward - * compatibility imposes to unlock the entire flash memory array at power-up - * by default. + * Return: 0 on success, -errno otherwise. */ static int spi_nor_unlock_all(struct spi_nor *nor) { - if (nor->flags & SNOR_F_HAS_LOCK) + if (nor->flags & SNOR_F_HAS_LOCK) { + dev_dbg(nor->dev, "unprotecting entire flash\n"); return spi_nor_unlock(&nor->mtd, 0, nor->params->size); + } return 0; } @@ -3153,10 +3152,23 @@ static int spi_nor_init(struct spi_nor *nor) return err; } - err = spi_nor_unlock_all(nor); - if (err) { - dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n"); - return err; + /* + * Some SPI NOR flashes are write protected by default after a power-on + * reset cycle, in order to avoid inadvertent writes during power-up. + * Backward compatibility imposes to unlock the entire flash memory + * array at power-up by default. Depending on the kernel configuration + * (1) we do nothing, (2) we unlock the entire flash in any case or (3) + * just do it actually powers up write-protected. The latter is + * indicated by SNOR_F_WP_IS_VOLATILE. + */ + if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) || + (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) && + nor->flags & SNOR_F_WP_IS_VOLATILE)) { + err = spi_nor_unlock_all(nor); + if (err) { + dev_err(nor->dev, "Failed to unlock the entire flash memory array\n"); + return err; + } } if (nor->addr_width == 4 && @@ -3456,6 +3468,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; if (info->flags & USE_CLSR) nor->flags |= SNOR_F_USE_CLSR; + if (info->flags & SPI_NOR_WP_IS_VOLATILE) + nor->flags |= SNOR_F_WP_IS_VOLATILE; if (info->flags & SPI_NOR_4BIT_BP) { nor->flags |= SNOR_F_HAS_4BIT_BP; diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 16b350e1d865..6bf86c2994c8 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -28,6 +28,7 @@ enum spi_nor_option_flags { SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), SNOR_F_IO_MODE_EN_VOLATILE = BIT(14), SNOR_F_SOFT_RESET = BIT(15), + SNOR_F_WP_IS_VOLATILE = BIT(16), }; struct spi_nor_read_command { @@ -329,6 +330,11 @@ struct flash_info { * available I/O mode via a * volatile bit. */ +#define SPI_NOR_WP_IS_VOLATILE BIT(22) /* + * Flash has volatile write protection + * bits. Usually these will power-up in + * a write-protected state. + */ /* Part specific fixup hooks. */ const struct spi_nor_fixups *fixups; @@ -430,7 +436,9 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len); +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1); int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr); ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c index c93170008118..d970f6da27c2 100644 --- a/drivers/mtd/spi-nor/esmt.c +++ b/drivers/mtd/spi-nor/esmt.c @@ -11,7 +11,7 @@ static const struct flash_info esmt_parts[] = { /* ESMT */ { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, - SECT_4K | SPI_NOR_HAS_LOCK) }, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_LOCK) }, { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128, diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c index 6c31bef3fc60..0bb1e3fd9702 100644 --- a/drivers/mtd/spi-nor/intel.c +++ b/drivers/mtd/spi-nor/intel.c @@ -10,9 +10,12 @@ static const struct flash_info intel_parts[] = { /* Intel/Numonyx -- xxxs33b */ - { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, SPI_NOR_HAS_LOCK) }, - { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, SPI_NOR_HAS_LOCK) }, - { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_HAS_LOCK) }, + { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, + SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, + { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, + SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, + { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, + SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, }; const struct spi_nor_manufacturer spi_nor_intel = { diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c index 8b169fa4102a..5e4450877d66 100644 --- a/drivers/mtd/spi-nor/sst.c +++ b/drivers/mtd/spi-nor/sst.c @@ -11,26 +11,27 @@ 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 | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, - { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, + { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25wf512", INFO(0xbf2501, 0, 64 * 1024, 1, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25wf010", INFO(0xbf2502, 0, 64 * 1024, 2, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25wf020", INFO(0xbf2503, 0, 64 * 1024, 4, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "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 | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst25wf080", INFO(0xbf2505, 0, 64 * 1024, 16, - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, { "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile 2020-11-26 20:26 ` [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle @ 2020-11-28 10:17 ` Tudor.Ambarus 2020-11-30 14:38 ` Michael Walle 0 siblings, 1 reply; 15+ messages in thread From: Tudor.Ambarus @ 2020-11-28 10:17 UTC (permalink / raw) To: michael, linux-mtd, linux-kernel Cc: miquel.raynal, richard, vigneshr, boris.brezillon On 11/26/20 10:26 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Traditionally, linux unlocks the whole flash because there are legacy > devices which has the write protections bits set by default at startup. > If you actually want to use the flash protection bits, eg. because there > is a read-only part for a bootloader, this automatic unlocking is > harmful. If there is no hardware write protection in place (usually > called WP#), a startup of the kernel just discards this protection. > > I've gone through the datasheets of all the flashes (except the Intel > ones where I could not find any datasheet nor reference) which supports > the unlocking feature and looked how the sector protection was > implemented. The currently supported flashes can be divided into the > following two categories: > (1) block protection bits are non-volatile. Thus they keep their values > at reset and power-cycle > (2) flashes where these bits are volatile. After reset or power-cycle, > the whole memory array is protected. > (a) some devices needs a special "Global Unprotect" command, eg. > the Atmel AT25DF041A. > (b) some devices require to clear the BPn bits in the status > register. > > Due to the reasons above, we do not want to clear the bits for flashes > which belong to category (1). Fortunately for us, only Atmel flashes > fall into category (2a). Implement the "Global Protect" and "Global > Unprotect" commands for these. For (2b) we can use normal block > protection locking scheme. > > This patch adds a new flag to indicate the case (2). Only if we have > such a flash we unlock the whole flash array. To be backwards compatible > it also introduces a kernel configuration option which restores the > complete legacy behavior ("Disable write protection on any flashes"). > Hopefully, this will clean up "unlock the entire flash for legacy > devices" once and for all. > > For reference here are the actually commits which introduced the legacy > behaviour (and extended the behaviour to other chip manufacturers): typo: behavior > > commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx {16,32,64}0S33B SPI flash chips") > commit ea60658a08f8f ("mtd: m25p80: disable SST software protection bits by default") > commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails to be copied to") > > Actually, this might also fix handling of the Atmel AT25DF flashes, > because the original commit 7228982442365 ("[MTD] m25p80: fix bug - > ATmel spi flash fails to be copied to") was writing a 0 to the status > register, which is a "Global Unprotect". This might not be the case in > the current code which only handles the block protection bits BP2, BP1 > and BP0. Thus, it depends on the current contents of the status register > if this unlock actually corresponds to a "Global Unprotect" command. In > the worst case, the current code might leave the AT25DF flashes in a > write protected state. > > The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when QE > is set on spansion flashes") changed that behaviour by just clearing BP2 > to BP0 instead of writing a 0 to the status register. > > Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling > of block write protection") expanded the unlock_all() feature to ANY > flash which supports locking. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > changes since v5: > - also set SRWD bit for the "Global Protect" command > - use spi_nor_write_sr() instead of spi_nor_write_sr_and_check() to send > the "Global Protect" or "Global Unprotect" command > - mark ESMT F25L32QA as non-volatile as indicated in a newer datasheet > revision > - rebased to latest tree > > changes since v4: > - made atmel_global_protection_default_init() static, spotted by > lkp@intel.com > > changes since v3: > - now defaulting to MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE, suggested by Vignesh > - restored the original spi_nor_unlock_all(), instead add individual > locking ops for the "Global Protect" scheme in atmel.c. This was tested > partly with the AT25SL321 (for the test I added the fixups to this > flash). > - renamed SPI_NOR_UNPROTECT to SPI_NOR_WP_IS_VOLATILE. Suggested by > Vingesh, although I've renamed it to a more general "WP_IS_VOLATILE" > because either the BP bits or the individual sector locks might be > volatile. > - add mention of both block protection bits and "Global Unprotect" command > in the Kconfig help text. > > changes since v2: > - add Kconfig option to be able to retain legacy behaviour > - rebased the patch due to the spi-nor rewrite > - dropped the Fixes: tag, it doens't make sense after the spi-nor rewrite > - mention commit 3e0930f109e76 which further modified the unlock > behaviour. > > changes since v1: > - completely rewrote patch, the first version used a device tree flag > > drivers/mtd/spi-nor/Kconfig | 42 ++++++++++++ > drivers/mtd/spi-nor/atmel.c | 127 ++++++++++++++++++++++++++++++++++-- > drivers/mtd/spi-nor/core.c | 36 ++++++---- > drivers/mtd/spi-nor/core.h | 8 +++ > drivers/mtd/spi-nor/esmt.c | 2 +- > drivers/mtd/spi-nor/intel.c | 9 ++- > drivers/mtd/spi-nor/sst.c | 21 +++--- > 7 files changed, 213 insertions(+), 32 deletions(-) > > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig > index ffc4b380f2b1..11e6658ee85d 100644 > --- a/drivers/mtd/spi-nor/Kconfig > +++ b/drivers/mtd/spi-nor/Kconfig > @@ -24,6 +24,48 @@ config MTD_SPI_NOR_USE_4K_SECTORS > Please note that some tools/drivers/filesystems may not work with > 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum). > > +choice > + prompt "Write protection at boot" > + default MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE > + > +config MTD_SPI_NOR_WP_DISABLE Maybe it's just me, but when I see WP, I think about the WP# signal, which is somehow related. I think I would prefer to use SWP instead, which comes from Software Write Protection, which should be good for both the BPn protection and for the Individual Sector Protection with its Global Lock and Unlock. I won't stall the series just for this, so do as you prefer. > + bool "Disable WP on any flashes (legacy behaviour)" typo: behavior > + help > + This option disables the write protection on any SPI flashes at If you'll choose SWP, you have to update description here and there. For example s/write protection/software write protection. > + boot-up. > + > + Depending on the flash chip this either clears the block protection > + bits or does a "Global Unprotect" command. > + > + Don't use this if you intent to use the write protection of your > + SPI flash. This is only to keep backwards compatibility. > + > +config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE > + bool "Disable WP on flashes w/ volatile protection bits" > + help > + Some SPI flashes have volatile block protection bits, ie. after a > + power-up or a reset the flash is write protected by default. > + > + This option disables the write protection for these kind of flashes > + while keeping it enabled for any other SPI flashes which have > + non-volatile write protection bits. > + > + If the write protection will be disabled depending on the flash > + either the block protection bits are cleared or a "Global Unprotect" > + command is issued. > + > + If you are unsure, select this option. > + > +config MTD_SPI_NOR_WP_KEEP > + bool "Keep write protection as is" > + help > + If you select this option the write protection of any SPI flashes > + will not be changed. If your flash is write protected or will be > + automatically write protected after power-up you have to manually > + unlock it before you are able to write to it. > + > +endchoice > + > source "drivers/mtd/spi-nor/controllers/Kconfig" > > endif # MTD_SPI_NOR > diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c > index fe6a4653823d..215df7c4272b 100644 > --- a/drivers/mtd/spi-nor/atmel.c > +++ b/drivers/mtd/spi-nor/atmel.c > @@ -8,6 +8,8 @@ > > #include "core.h" > > +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2) > + > /* > * The Atmel AT25FS010/AT25FS040 parts have some weird configuration for the > * block protection bits. We don't support them. But legacy behaviour in linux > @@ -55,6 +57,103 @@ static const struct spi_nor_fixups atmel_at25fs_fixups = { > .default_init = atmel_at25fs_default_init, > }; > > +/** > + * atmel_set_global_protection - Do a Global Protect or Unprotect command > + * @nor: pointer to 'struct spi_nor' > + * @ofs: offset in bytes > + * @len: len in bytes > + * @is_protect: if true do a Global Protect otherwise it is a Global Unprotect > + * > + * Return: 0 on success, -error otherwise. > + */ > +static int atmel_set_global_protection(struct spi_nor *nor, loff_t ofs, > + uint64_t len, bool is_protect) > +{ > + int ret; > + u8 sr; > + > + /* We only support locking the whole flash array */ > + if (ofs || len != nor->params->size) > + return -EINVAL; > + > + ret = spi_nor_read_sr(nor, nor->bouncebuf); > + if (ret) > + return ret; maybe a new line in between. > + sr = nor->bouncebuf[0]; > + > + /* SRWD bit needs to be cleared, otherwise the protection doesn't change */ > + if (sr & SR_SRWD) { > + sr &= ~SR_SRWD; > + ret = spi_nor_write_sr_and_check(nor, sr); > + if (ret) { > + dev_err(nor->dev, "unable to clear SRWD bit, WP# asserted?\n"); spi_nor_write_sr_and_check() already prints a dev_dbg(). If you find a second message useful, you should use dev_dbg for low level info. > + return ret; > + } > + } > + > + if (is_protect) { > + sr |= ATMEL_SR_GLOBAL_PROTECT_MASK; > + /* > + * Set the SRWD bit again as soon as we are protecting > + * anything. This will ensure that the WP# pin is working > + * correctly. By doing this we also behave the same as > + * spi_nor_sr_lock(), which sets SRWD if any block protection > + * is active. > + */ > + sr |= SR_SRWD; > + } else { > + sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK; > + } > + > + nor->bouncebuf[0] = sr; > + > + /* > + * We cannot use the spi_nor_write_sr_and_check() because this command > + * isn't really setting any bits, instead it is an pseudo command for > + * "Global Unprotect" or "Global Protect" > + */ > + return spi_nor_write_sr(nor, nor->bouncebuf, 1); > +} > + > +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, uint64_t len) > +{ > + return atmel_set_global_protection(nor, ofs, len, true); > +} > + > +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, uint64_t len) > +{ > + return atmel_set_global_protection(nor, ofs, len, false); > +} > + > +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, uint64_t len) > +{ > + int ret; > + > + if (ofs >= nor->params->size || (ofs + len) > nor->params->size) > + return -EINVAL; > + > + ret = spi_nor_read_sr(nor, nor->bouncebuf); > + if (ret) > + return ret; > + > + return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == ATMEL_SR_GLOBAL_PROTECT_MASK); > +} > + > +static const struct spi_nor_locking_ops atmel_global_protection_ops = { > + .lock = atmel_global_protect, > + .unlock = atmel_global_unprotect, > + .is_locked = atmel_is_global_protected, > +}; > + > +static void atmel_global_protection_default_init(struct spi_nor *nor) > +{ > + nor->params->locking_ops = &atmel_global_protection_ops; > +} > + > +static const struct spi_nor_fixups atmel_global_protection_fixups = { > + .default_init = atmel_global_protection_default_init, > +}; > + > static const struct flash_info atmel_parts[] = { > /* Atmel -- some are (confusingly) marketed as "DataFlash" */ > { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK) > @@ -62,18 +161,32 @@ static const struct flash_info atmel_parts[] = { > { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK) > .fixups = &atmel_at25fs_fixups }, > > - { "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) }, > + { "at25df041a", INFO(0x1f4401, 0, 64 * 1024, 8, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > + { "at25df321", INFO(0x1f4700, 0, 64 * 1024, 64, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > + { "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > + { "at25df641", INFO(0x1f4800, 0, 64 * 1024, 128, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > > { "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 | 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) }, > + { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > + { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > + { "at26df321", INFO(0x1f4700, 0, 64 * 1024, 64, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) > + .fixups = &atmel_global_protection_fixups }, > > { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, > }; > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 8c06a28a90de..8354ce0c8810 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -377,7 +377,7 @@ int spi_nor_write_disable(struct spi_nor *nor) > * > * Return: 0 on success, -errno otherwise. > */ > -static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) > { > int ret; > > @@ -1049,7 +1049,7 @@ static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr) > * > * Return: 0 on success, -errno otherwise. > */ > -static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) > +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) > { > if (nor->flags & SNOR_F_HAS_16BIT_SR) > return spi_nor_write_16bit_sr_and_check(nor, sr1); > @@ -3124,15 +3124,14 @@ static int spi_nor_quad_enable(struct spi_nor *nor) > * spi_nor_unlock_all() - Unlocks the entire flash memory array. > * @nor: pointer to a 'struct spi_nor'. > * > - * Some SPI NOR flashes are write protected by default after a power-on reset > - * cycle, in order to avoid inadvertent writes during power-up. Backward > - * compatibility imposes to unlock the entire flash memory array at power-up > - * by default. > + * Return: 0 on success, -errno otherwise. > */ > static int spi_nor_unlock_all(struct spi_nor *nor) > { > - if (nor->flags & SNOR_F_HAS_LOCK) > + if (nor->flags & SNOR_F_HAS_LOCK) { > + dev_dbg(nor->dev, "unprotecting entire flash\n"); > return spi_nor_unlock(&nor->mtd, 0, nor->params->size); > + } > > return 0; > } > @@ -3153,10 +3152,23 @@ static int spi_nor_init(struct spi_nor *nor) > return err; > } > > - err = spi_nor_unlock_all(nor); > - if (err) { > - dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n"); > - return err; > + /* > + * Some SPI NOR flashes are write protected by default after a power-on > + * reset cycle, in order to avoid inadvertent writes during power-up. > + * Backward compatibility imposes to unlock the entire flash memory > + * array at power-up by default. Depending on the kernel configuration > + * (1) we do nothing, (2) we unlock the entire flash in any case or (3) > + * just do it actually powers up write-protected. The latter is do it if it actually powers up How about: (1) do nothing, (2) always unlock the entire flash array, (3) unlock the entire flash array only when the software protection bits are volatile. > + * indicated by SNOR_F_WP_IS_VOLATILE. > + */ > + if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) || > + (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) && > + nor->flags & SNOR_F_WP_IS_VOLATILE)) { > + err = spi_nor_unlock_all(nor); > + if (err) { > + dev_err(nor->dev, "Failed to unlock the entire flash memory array\n"); dev_dbg for low level info > + return err; > + } > } > > if (nor->addr_width == 4 && > @@ -3456,6 +3468,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; > if (info->flags & USE_CLSR) > nor->flags |= SNOR_F_USE_CLSR; > + if (info->flags & SPI_NOR_WP_IS_VOLATILE) > + nor->flags |= SNOR_F_WP_IS_VOLATILE; > > if (info->flags & SPI_NOR_4BIT_BP) { > nor->flags |= SNOR_F_HAS_4BIT_BP; > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 16b350e1d865..6bf86c2994c8 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -28,6 +28,7 @@ enum spi_nor_option_flags { > SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), > SNOR_F_IO_MODE_EN_VOLATILE = BIT(14), > SNOR_F_SOFT_RESET = BIT(15), > + SNOR_F_WP_IS_VOLATILE = BIT(16), > }; > > struct spi_nor_read_command { > @@ -329,6 +330,11 @@ struct flash_info { > * available I/O mode via a > * volatile bit. > */ > +#define SPI_NOR_WP_IS_VOLATILE BIT(22) /* > + * Flash has volatile write protection > + * bits. Usually these will power-up in > + * a write-protected state. > + */ > > /* Part specific fixup hooks. */ > const struct spi_nor_fixups *fixups; > @@ -430,7 +436,9 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); > +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); > int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len); > +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1); > > int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr); > ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, > diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c > index c93170008118..d970f6da27c2 100644 > --- a/drivers/mtd/spi-nor/esmt.c > +++ b/drivers/mtd/spi-nor/esmt.c > @@ -11,7 +11,7 @@ > static const struct flash_info esmt_parts[] = { > /* ESMT */ > { "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, > - SECT_4K | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64, > SECT_4K | SPI_NOR_HAS_LOCK) }, > { "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128, > diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c > index 6c31bef3fc60..0bb1e3fd9702 100644 > --- a/drivers/mtd/spi-nor/intel.c > +++ b/drivers/mtd/spi-nor/intel.c > @@ -10,9 +10,12 @@ > > static const struct flash_info intel_parts[] = { > /* Intel/Numonyx -- xxxs33b */ > - { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, SPI_NOR_HAS_LOCK) }, > - { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, SPI_NOR_HAS_LOCK) }, > - { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_HAS_LOCK) }, > + { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, > + SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > + { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, > + SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > + { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, > + SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > }; > > const struct spi_nor_manufacturer spi_nor_intel = { > diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c > index 8b169fa4102a..5e4450877d66 100644 > --- a/drivers/mtd/spi-nor/sst.c > +++ b/drivers/mtd/spi-nor/sst.c > @@ -11,26 +11,27 @@ > 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 | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > - { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > + { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, > + SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25wf512", INFO(0xbf2501, 0, 64 * 1024, 1, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25wf010", INFO(0xbf2502, 0, 64 * 1024, 2, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25wf020", INFO(0xbf2503, 0, 64 * 1024, 4, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "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 | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst25wf080", INFO(0xbf2505, 0, 64 * 1024, 16, > - SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_WP_IS_VOLATILE) }, > { "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32, > SECT_4K | SPI_NOR_DUAL_READ | > SPI_NOR_QUAD_READ) }, > -- > 2.20.1 > Thanks, ta ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile 2020-11-28 10:17 ` Tudor.Ambarus @ 2020-11-30 14:38 ` Michael Walle 2020-12-02 11:10 ` Tudor.Ambarus 0 siblings, 1 reply; 15+ messages in thread From: Michael Walle @ 2020-11-30 14:38 UTC (permalink / raw) To: Tudor.Ambarus Cc: linux-mtd, linux-kernel, miquel.raynal, richard, vigneshr, boris.brezillon Am 2020-11-28 11:17, schrieb Tudor.Ambarus@microchip.com: > On 11/26/20 10:26 PM, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Traditionally, linux unlocks the whole flash because there are legacy >> devices which has the write protections bits set by default at >> startup. >> If you actually want to use the flash protection bits, eg. because >> there >> is a read-only part for a bootloader, this automatic unlocking is >> harmful. If there is no hardware write protection in place (usually >> called WP#), a startup of the kernel just discards this protection. >> >> I've gone through the datasheets of all the flashes (except the Intel >> ones where I could not find any datasheet nor reference) which >> supports >> the unlocking feature and looked how the sector protection was >> implemented. The currently supported flashes can be divided into the >> following two categories: >> (1) block protection bits are non-volatile. Thus they keep their >> values >> at reset and power-cycle >> (2) flashes where these bits are volatile. After reset or >> power-cycle, >> the whole memory array is protected. >> (a) some devices needs a special "Global Unprotect" command, eg. >> the Atmel AT25DF041A. >> (b) some devices require to clear the BPn bits in the status >> register. >> >> Due to the reasons above, we do not want to clear the bits for flashes >> which belong to category (1). Fortunately for us, only Atmel flashes >> fall into category (2a). Implement the "Global Protect" and "Global >> Unprotect" commands for these. For (2b) we can use normal block >> protection locking scheme. >> >> This patch adds a new flag to indicate the case (2). Only if we have >> such a flash we unlock the whole flash array. To be backwards >> compatible >> it also introduces a kernel configuration option which restores the >> complete legacy behavior ("Disable write protection on any flashes"). >> Hopefully, this will clean up "unlock the entire flash for legacy >> devices" once and for all. >> >> For reference here are the actually commits which introduced the >> legacy >> behaviour (and extended the behaviour to other chip manufacturers): > > typo: behavior > >> >> commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx >> {16,32,64}0S33B SPI flash chips") >> commit ea60658a08f8f ("mtd: m25p80: disable SST software protection >> bits by default") >> commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails >> to be copied to") >> >> Actually, this might also fix handling of the Atmel AT25DF flashes, >> because the original commit 7228982442365 ("[MTD] m25p80: fix bug - >> ATmel spi flash fails to be copied to") was writing a 0 to the status >> register, which is a "Global Unprotect". This might not be the case in >> the current code which only handles the block protection bits BP2, BP1 >> and BP0. Thus, it depends on the current contents of the status >> register >> if this unlock actually corresponds to a "Global Unprotect" command. >> In >> the worst case, the current code might leave the AT25DF flashes in a >> write protected state. >> >> The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when >> QE >> is set on spansion flashes") changed that behaviour by just clearing >> BP2 >> to BP0 instead of writing a 0 to the status register. >> >> Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling >> of block write protection") expanded the unlock_all() feature to ANY >> flash which supports locking. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> changes since v5: >> - also set SRWD bit for the "Global Protect" command >> - use spi_nor_write_sr() instead of spi_nor_write_sr_and_check() to >> send >> the "Global Protect" or "Global Unprotect" command >> - mark ESMT F25L32QA as non-volatile as indicated in a newer >> datasheet >> revision >> - rebased to latest tree >> >> changes since v4: >> - made atmel_global_protection_default_init() static, spotted by >> lkp@intel.com >> >> changes since v3: >> - now defaulting to MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE, suggested by >> Vignesh >> - restored the original spi_nor_unlock_all(), instead add individual >> locking ops for the "Global Protect" scheme in atmel.c. This was >> tested >> partly with the AT25SL321 (for the test I added the fixups to this >> flash). >> - renamed SPI_NOR_UNPROTECT to SPI_NOR_WP_IS_VOLATILE. Suggested by >> Vingesh, although I've renamed it to a more general >> "WP_IS_VOLATILE" >> because either the BP bits or the individual sector locks might be >> volatile. >> - add mention of both block protection bits and "Global Unprotect" >> command >> in the Kconfig help text. >> >> changes since v2: >> - add Kconfig option to be able to retain legacy behaviour >> - rebased the patch due to the spi-nor rewrite >> - dropped the Fixes: tag, it doens't make sense after the spi-nor >> rewrite >> - mention commit 3e0930f109e76 which further modified the unlock >> behaviour. >> >> changes since v1: >> - completely rewrote patch, the first version used a device tree flag >> >> drivers/mtd/spi-nor/Kconfig | 42 ++++++++++++ >> drivers/mtd/spi-nor/atmel.c | 127 >> ++++++++++++++++++++++++++++++++++-- >> drivers/mtd/spi-nor/core.c | 36 ++++++---- >> drivers/mtd/spi-nor/core.h | 8 +++ >> drivers/mtd/spi-nor/esmt.c | 2 +- >> drivers/mtd/spi-nor/intel.c | 9 ++- >> drivers/mtd/spi-nor/sst.c | 21 +++--- >> 7 files changed, 213 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >> index ffc4b380f2b1..11e6658ee85d 100644 >> --- a/drivers/mtd/spi-nor/Kconfig >> +++ b/drivers/mtd/spi-nor/Kconfig >> @@ -24,6 +24,48 @@ config MTD_SPI_NOR_USE_4K_SECTORS >> Please note that some tools/drivers/filesystems may not work >> with >> 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum). >> >> +choice >> + prompt "Write protection at boot" >> + default MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE >> + >> +config MTD_SPI_NOR_WP_DISABLE > > Maybe it's just me, but when I see WP, I think about the WP# signal, > which is > somehow related. I think I would prefer to use SWP instead, which > comes from > Software Write Protection, which should be good for both the BPn > protection > and for the Individual Sector Protection with its Global Lock and > Unlock. I don't know either. Somehow the SWP will become a true hw write protection with the WP# pin. But I tend to agree, I'll change it to SWP. > > I won't stall the series just for this, so do as you prefer. > >> + bool "Disable WP on any flashes (legacy behaviour)" > > typo: behavior Just out of curiosity, is kernel doc strict American English? > >> + help >> + This option disables the write protection on any SPI flashes >> at > > If you'll choose SWP, you have to update description here and there. > For > example s/write protection/software write protection. > >> + boot-up. >> + >> + Depending on the flash chip this either clears the block >> protection >> + bits or does a "Global Unprotect" command. >> + >> + Don't use this if you intent to use the write protection of >> your >> + SPI flash. This is only to keep backwards compatibility. >> + >> +config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE >> + bool "Disable WP on flashes w/ volatile protection bits" >> + help >> + Some SPI flashes have volatile block protection bits, ie. >> after a >> + power-up or a reset the flash is write protected by default. >> + >> + This option disables the write protection for these kind of >> flashes >> + while keeping it enabled for any other SPI flashes which >> have >> + non-volatile write protection bits. >> + >> + If the write protection will be disabled depending on the >> flash >> + either the block protection bits are cleared or a "Global >> Unprotect" >> + command is issued. >> + >> + If you are unsure, select this option. >> + >> +config MTD_SPI_NOR_WP_KEEP >> + bool "Keep write protection as is" >> + help >> + If you select this option the write protection of any SPI >> flashes >> + will not be changed. If your flash is write protected or >> will be >> + automatically write protected after power-up you have to >> manually >> + unlock it before you are able to write to it. >> + >> +endchoice >> + >> source "drivers/mtd/spi-nor/controllers/Kconfig" >> >> endif # MTD_SPI_NOR >> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c >> index fe6a4653823d..215df7c4272b 100644 >> --- a/drivers/mtd/spi-nor/atmel.c >> +++ b/drivers/mtd/spi-nor/atmel.c >> @@ -8,6 +8,8 @@ >> >> #include "core.h" >> >> +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2) >> + >> /* >> * The Atmel AT25FS010/AT25FS040 parts have some weird configuration >> for the >> * block protection bits. We don't support them. But legacy behaviour >> in linux >> @@ -55,6 +57,103 @@ static const struct spi_nor_fixups >> atmel_at25fs_fixups = { >> .default_init = atmel_at25fs_default_init, >> }; >> >> +/** >> + * atmel_set_global_protection - Do a Global Protect or Unprotect >> command >> + * @nor: pointer to 'struct spi_nor' >> + * @ofs: offset in bytes >> + * @len: len in bytes >> + * @is_protect: if true do a Global Protect otherwise it is a >> Global Unprotect >> + * >> + * Return: 0 on success, -error otherwise. >> + */ >> +static int atmel_set_global_protection(struct spi_nor *nor, loff_t >> ofs, >> + uint64_t len, bool is_protect) >> +{ >> + int ret; >> + u8 sr; >> + >> + /* We only support locking the whole flash array */ >> + if (ofs || len != nor->params->size) >> + return -EINVAL; >> + >> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >> + if (ret) >> + return ret; > > maybe a new line in between. ok >> + sr = nor->bouncebuf[0]; >> + >> + /* SRWD bit needs to be cleared, otherwise the protection >> doesn't change */ >> + if (sr & SR_SRWD) { >> + sr &= ~SR_SRWD; >> + ret = spi_nor_write_sr_and_check(nor, sr); >> + if (ret) { >> + dev_err(nor->dev, "unable to clear SRWD bit, >> WP# asserted?\n"); > > spi_nor_write_sr_and_check() already prints a dev_dbg(). If you find a > second message > useful, you should use dev_dbg for low level info. The intention for this was to make the user aware the reason why the unlock might not work. The reason I chose dev_err() was that its unlikely that John Doe will have debug enabled. But I already came up with another reason why this is bad: Everytime the kernel will start an unlock all might raise that error. Therefore, I guess the kernel is the wrong place for that. > >> + return ret; >> + } >> + } >> + >> + if (is_protect) { >> + sr |= ATMEL_SR_GLOBAL_PROTECT_MASK; >> + /* >> + * Set the SRWD bit again as soon as we are protecting >> + * anything. This will ensure that the WP# pin is >> working >> + * correctly. By doing this we also behave the same as >> + * spi_nor_sr_lock(), which sets SRWD if any block >> protection >> + * is active. >> + */ >> + sr |= SR_SRWD; >> + } else { >> + sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK; >> + } >> + >> + nor->bouncebuf[0] = sr; >> + >> + /* >> + * We cannot use the spi_nor_write_sr_and_check() because this >> command >> + * isn't really setting any bits, instead it is an pseudo >> command for >> + * "Global Unprotect" or "Global Protect" >> + */ >> + return spi_nor_write_sr(nor, nor->bouncebuf, 1); >> +} >> + >> +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + return atmel_set_global_protection(nor, ofs, len, true); >> +} >> + >> +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + return atmel_set_global_protection(nor, ofs, len, false); >> +} >> + >> +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, >> uint64_t len) >> +{ >> + int ret; >> + >> + if (ofs >= nor->params->size || (ofs + len) > >> nor->params->size) >> + return -EINVAL; >> + >> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >> + if (ret) >> + return ret; >> + >> + return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == >> ATMEL_SR_GLOBAL_PROTECT_MASK); >> +} >> + >> +static const struct spi_nor_locking_ops atmel_global_protection_ops = >> { >> + .lock = atmel_global_protect, >> + .unlock = atmel_global_unprotect, >> + .is_locked = atmel_is_global_protected, >> +}; >> + >> +static void atmel_global_protection_default_init(struct spi_nor *nor) >> +{ >> + nor->params->locking_ops = &atmel_global_protection_ops; >> +} >> + >> +static const struct spi_nor_fixups atmel_global_protection_fixups = { >> + .default_init = atmel_global_protection_default_init, >> +}; >> + >> static const struct flash_info atmel_parts[] = { >> /* Atmel -- some are (confusingly) marketed as "DataFlash" */ >> { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | >> SPI_NOR_HAS_LOCK) >> @@ -62,18 +161,32 @@ static const struct flash_info atmel_parts[] = { >> { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | >> SPI_NOR_HAS_LOCK) >> .fixups = &atmel_at25fs_fixups }, >> >> - { "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) }, >> + { "at25df041a", INFO(0x1f4401, 0, 64 * 1024, 8, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> + { "at25df321", INFO(0x1f4700, 0, 64 * 1024, 64, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> + { "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> + { "at25df641", INFO(0x1f4800, 0, 64 * 1024, 128, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> >> { "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 | >> 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) }, >> + { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> + { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> + { "at26df321", INFO(0x1f4700, 0, 64 * 1024, 64, >> + SECT_4K | SPI_NOR_HAS_LOCK | >> SPI_NOR_WP_IS_VOLATILE) >> + .fixups = &atmel_global_protection_fixups }, >> >> { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, >> }; >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 8c06a28a90de..8354ce0c8810 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -377,7 +377,7 @@ int spi_nor_write_disable(struct spi_nor *nor) >> * >> * Return: 0 on success, -errno otherwise. >> */ >> -static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) >> +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) >> { >> int ret; >> >> @@ -1049,7 +1049,7 @@ static int >> spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr) >> * >> * Return: 0 on success, -errno otherwise. >> */ >> -static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) >> +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) >> { >> if (nor->flags & SNOR_F_HAS_16BIT_SR) >> return spi_nor_write_16bit_sr_and_check(nor, sr1); >> @@ -3124,15 +3124,14 @@ static int spi_nor_quad_enable(struct spi_nor >> *nor) >> * spi_nor_unlock_all() - Unlocks the entire flash memory array. >> * @nor: pointer to a 'struct spi_nor'. >> * >> - * Some SPI NOR flashes are write protected by default after a >> power-on reset >> - * cycle, in order to avoid inadvertent writes during power-up. >> Backward >> - * compatibility imposes to unlock the entire flash memory array at >> power-up >> - * by default. >> + * Return: 0 on success, -errno otherwise. >> */ >> static int spi_nor_unlock_all(struct spi_nor *nor) >> { >> - if (nor->flags & SNOR_F_HAS_LOCK) >> + if (nor->flags & SNOR_F_HAS_LOCK) { >> + dev_dbg(nor->dev, "unprotecting entire flash\n"); >> return spi_nor_unlock(&nor->mtd, 0, >> nor->params->size); >> + } >> >> return 0; >> } >> @@ -3153,10 +3152,23 @@ static int spi_nor_init(struct spi_nor *nor) >> return err; >> } >> >> - err = spi_nor_unlock_all(nor); >> - if (err) { >> - dev_dbg(nor->dev, "Failed to unlock the entire flash >> memory array\n"); >> - return err; >> + /* >> + * Some SPI NOR flashes are write protected by default after a >> power-on >> + * reset cycle, in order to avoid inadvertent writes during >> power-up. >> + * Backward compatibility imposes to unlock the entire flash >> memory >> + * array at power-up by default. Depending on the kernel >> configuration >> + * (1) we do nothing, (2) we unlock the entire flash in any >> case or (3) >> + * just do it actually powers up write-protected. The latter >> is > > do it if it actually powers up > > How about: (1) do nothing, (2) always unlock the entire flash array, > (3) unlock > the entire flash array only when the software protection bits are > volatile. ok >> + * indicated by SNOR_F_WP_IS_VOLATILE. >> + */ >> + if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) || >> + (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) && >> + nor->flags & SNOR_F_WP_IS_VOLATILE)) { >> + err = spi_nor_unlock_all(nor); >> + if (err) { >> + dev_err(nor->dev, "Failed to unlock the entire >> flash memory array\n"); > > dev_dbg for low level info Is this low level info or an actual error? Which raises the question: should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus should all the spi_nor_init fail of this? Or should it rather be a soft error? Also I don't know how spi_nor_sr_unlock() will behave. >> + return err; >> + } >> } >> [..] -michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile 2020-11-30 14:38 ` Michael Walle @ 2020-12-02 11:10 ` Tudor.Ambarus 2020-12-02 11:25 ` Michael Walle 0 siblings, 1 reply; 15+ messages in thread From: Tudor.Ambarus @ 2020-12-02 11:10 UTC (permalink / raw) To: michael Cc: linux-mtd, linux-kernel, miquel.raynal, richard, vigneshr, boris.brezillon On 11/30/20 4:38 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2020-11-28 11:17, schrieb Tudor.Ambarus@microchip.com: >> On 11/26/20 10:26 PM, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Traditionally, linux unlocks the whole flash because there are legacy >>> devices which has the write protections bits set by default at >>> startup. >>> If you actually want to use the flash protection bits, eg. because >>> there >>> is a read-only part for a bootloader, this automatic unlocking is >>> harmful. If there is no hardware write protection in place (usually >>> called WP#), a startup of the kernel just discards this protection. >>> >>> I've gone through the datasheets of all the flashes (except the Intel >>> ones where I could not find any datasheet nor reference) which >>> supports >>> the unlocking feature and looked how the sector protection was >>> implemented. The currently supported flashes can be divided into the >>> following two categories: >>> (1) block protection bits are non-volatile. Thus they keep their >>> values >>> at reset and power-cycle >>> (2) flashes where these bits are volatile. After reset or >>> power-cycle, >>> the whole memory array is protected. >>> (a) some devices needs a special "Global Unprotect" command, eg. >>> the Atmel AT25DF041A. >>> (b) some devices require to clear the BPn bits in the status >>> register. >>> >>> Due to the reasons above, we do not want to clear the bits for flashes >>> which belong to category (1). Fortunately for us, only Atmel flashes >>> fall into category (2a). Implement the "Global Protect" and "Global >>> Unprotect" commands for these. For (2b) we can use normal block >>> protection locking scheme. >>> >>> This patch adds a new flag to indicate the case (2). Only if we have >>> such a flash we unlock the whole flash array. To be backwards >>> compatible >>> it also introduces a kernel configuration option which restores the >>> complete legacy behavior ("Disable write protection on any flashes"). >>> Hopefully, this will clean up "unlock the entire flash for legacy >>> devices" once and for all. >>> >>> For reference here are the actually commits which introduced the >>> legacy >>> behaviour (and extended the behaviour to other chip manufacturers): >> >> typo: behavior >> >>> >>> commit f80e521c916cb ("mtd: m25p80: add support for the Intel/Numonyx >>> {16,32,64}0S33B SPI flash chips") >>> commit ea60658a08f8f ("mtd: m25p80: disable SST software protection >>> bits by default") >>> commit 7228982442365 ("[MTD] m25p80: fix bug - ATmel spi flash fails >>> to be copied to") >>> >>> Actually, this might also fix handling of the Atmel AT25DF flashes, >>> because the original commit 7228982442365 ("[MTD] m25p80: fix bug - >>> ATmel spi flash fails to be copied to") was writing a 0 to the status >>> register, which is a "Global Unprotect". This might not be the case in >>> the current code which only handles the block protection bits BP2, BP1 >>> and BP0. Thus, it depends on the current contents of the status >>> register >>> if this unlock actually corresponds to a "Global Unprotect" command. >>> In >>> the worst case, the current code might leave the AT25DF flashes in a >>> write protected state. >>> >>> The commit 191f5c2ed4b6f ("mtd: spi-nor: use 16-bit WRR command when >>> QE >>> is set on spansion flashes") changed that behaviour by just clearing >>> BP2 >>> to BP0 instead of writing a 0 to the status register. >>> >>> Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling >>> of block write protection") expanded the unlock_all() feature to ANY >>> flash which supports locking. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> changes since v5: >>> - also set SRWD bit for the "Global Protect" command >>> - use spi_nor_write_sr() instead of spi_nor_write_sr_and_check() to >>> send >>> the "Global Protect" or "Global Unprotect" command >>> - mark ESMT F25L32QA as non-volatile as indicated in a newer >>> datasheet >>> revision >>> - rebased to latest tree >>> >>> changes since v4: >>> - made atmel_global_protection_default_init() static, spotted by >>> lkp@intel.com >>> >>> changes since v3: >>> - now defaulting to MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE, suggested by >>> Vignesh >>> - restored the original spi_nor_unlock_all(), instead add individual >>> locking ops for the "Global Protect" scheme in atmel.c. This was >>> tested >>> partly with the AT25SL321 (for the test I added the fixups to this >>> flash). >>> - renamed SPI_NOR_UNPROTECT to SPI_NOR_WP_IS_VOLATILE. Suggested by >>> Vingesh, although I've renamed it to a more general >>> "WP_IS_VOLATILE" >>> because either the BP bits or the individual sector locks might be >>> volatile. >>> - add mention of both block protection bits and "Global Unprotect" >>> command >>> in the Kconfig help text. >>> >>> changes since v2: >>> - add Kconfig option to be able to retain legacy behaviour >>> - rebased the patch due to the spi-nor rewrite >>> - dropped the Fixes: tag, it doens't make sense after the spi-nor >>> rewrite >>> - mention commit 3e0930f109e76 which further modified the unlock >>> behaviour. >>> >>> changes since v1: >>> - completely rewrote patch, the first version used a device tree flag >>> >>> drivers/mtd/spi-nor/Kconfig | 42 ++++++++++++ >>> drivers/mtd/spi-nor/atmel.c | 127 >>> ++++++++++++++++++++++++++++++++++-- >>> drivers/mtd/spi-nor/core.c | 36 ++++++---- >>> drivers/mtd/spi-nor/core.h | 8 +++ >>> drivers/mtd/spi-nor/esmt.c | 2 +- >>> drivers/mtd/spi-nor/intel.c | 9 ++- >>> drivers/mtd/spi-nor/sst.c | 21 +++--- >>> 7 files changed, 213 insertions(+), 32 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>> index ffc4b380f2b1..11e6658ee85d 100644 >>> --- a/drivers/mtd/spi-nor/Kconfig >>> +++ b/drivers/mtd/spi-nor/Kconfig >>> @@ -24,6 +24,48 @@ config MTD_SPI_NOR_USE_4K_SECTORS >>> Please note that some tools/drivers/filesystems may not work >>> with >>> 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum). >>> >>> +choice >>> + prompt "Write protection at boot" >>> + default MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE >>> + >>> +config MTD_SPI_NOR_WP_DISABLE >> >> Maybe it's just me, but when I see WP, I think about the WP# signal, >> which is >> somehow related. I think I would prefer to use SWP instead, which >> comes from >> Software Write Protection, which should be good for both the BPn >> protection >> and for the Individual Sector Protection with its Global Lock and >> Unlock. > > I don't know either. Somehow the SWP will become a true hw write > protection > with the WP# pin. But I tend to agree, I'll change it to SWP. > >> >> I won't stall the series just for this, so do as you prefer. >> >>> + bool "Disable WP on any flashes (legacy behaviour)" >> >> typo: behavior > > Just out of curiosity, is kernel doc strict American English? :) I don't know. I wasn't aware that behaviour is the UK English variant. It's the same as in color/colour. I see both variants on a short grep, but the US variant is predominant. Do as you prefer. > >> >>> + help >>> + This option disables the write protection on any SPI flashes >>> at >> >> If you'll choose SWP, you have to update description here and there. >> For >> example s/write protection/software write protection. >> >>> + boot-up. >>> + >>> + Depending on the flash chip this either clears the block >>> protection >>> + bits or does a "Global Unprotect" command. >>> + >>> + Don't use this if you intent to use the write protection of >>> your >>> + SPI flash. This is only to keep backwards compatibility. >>> + >>> +config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE >>> + bool "Disable WP on flashes w/ volatile protection bits" >>> + help >>> + Some SPI flashes have volatile block protection bits, ie. >>> after a >>> + power-up or a reset the flash is write protected by default. >>> + >>> + This option disables the write protection for these kind of >>> flashes >>> + while keeping it enabled for any other SPI flashes which >>> have >>> + non-volatile write protection bits. >>> + >>> + If the write protection will be disabled depending on the >>> flash >>> + either the block protection bits are cleared or a "Global >>> Unprotect" >>> + command is issued. >>> + >>> + If you are unsure, select this option. >>> + >>> +config MTD_SPI_NOR_WP_KEEP >>> + bool "Keep write protection as is" >>> + help >>> + If you select this option the write protection of any SPI >>> flashes >>> + will not be changed. If your flash is write protected or >>> will be >>> + automatically write protected after power-up you have to >>> manually >>> + unlock it before you are able to write to it. >>> + >>> +endchoice >>> + >>> source "drivers/mtd/spi-nor/controllers/Kconfig" >>> >>> endif # MTD_SPI_NOR >>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c >>> index fe6a4653823d..215df7c4272b 100644 >>> --- a/drivers/mtd/spi-nor/atmel.c >>> +++ b/drivers/mtd/spi-nor/atmel.c >>> @@ -8,6 +8,8 @@ >>> >>> #include "core.h" >>> >>> +#define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2) >>> + >>> /* >>> * The Atmel AT25FS010/AT25FS040 parts have some weird configuration >>> for the >>> * block protection bits. We don't support them. But legacy behaviour >>> in linux >>> @@ -55,6 +57,103 @@ static const struct spi_nor_fixups >>> atmel_at25fs_fixups = { >>> .default_init = atmel_at25fs_default_init, >>> }; >>> >>> +/** >>> + * atmel_set_global_protection - Do a Global Protect or Unprotect >>> command >>> + * @nor: pointer to 'struct spi_nor' >>> + * @ofs: offset in bytes >>> + * @len: len in bytes >>> + * @is_protect: if true do a Global Protect otherwise it is a >>> Global Unprotect >>> + * >>> + * Return: 0 on success, -error otherwise. >>> + */ >>> +static int atmel_set_global_protection(struct spi_nor *nor, loff_t >>> ofs, >>> + uint64_t len, bool is_protect) >>> +{ >>> + int ret; >>> + u8 sr; >>> + >>> + /* We only support locking the whole flash array */ >>> + if (ofs || len != nor->params->size) >>> + return -EINVAL; >>> + >>> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >>> + if (ret) >>> + return ret; >> >> maybe a new line in between. > > ok > >>> + sr = nor->bouncebuf[0]; >>> + >>> + /* SRWD bit needs to be cleared, otherwise the protection >>> doesn't change */ >>> + if (sr & SR_SRWD) { >>> + sr &= ~SR_SRWD; >>> + ret = spi_nor_write_sr_and_check(nor, sr); >>> + if (ret) { >>> + dev_err(nor->dev, "unable to clear SRWD bit, >>> WP# asserted?\n"); >> >> spi_nor_write_sr_and_check() already prints a dev_dbg(). If you find a >> second message >> useful, you should use dev_dbg for low level info. > > The intention for this was to make the user aware the reason why the > unlock > might not work. The reason I chose dev_err() was that its unlikely that > John > Doe will have debug enabled. But I already came up with another reason > why > this is bad: Everytime the kernel will start an unlock all might raise > that > error. Therefore, I guess the kernel is the wrong place for that. > >> >>> + return ret; >>> + } >>> + } >>> + >>> + if (is_protect) { >>> + sr |= ATMEL_SR_GLOBAL_PROTECT_MASK; >>> + /* >>> + * Set the SRWD bit again as soon as we are protecting >>> + * anything. This will ensure that the WP# pin is >>> working >>> + * correctly. By doing this we also behave the same as >>> + * spi_nor_sr_lock(), which sets SRWD if any block >>> protection >>> + * is active. >>> + */ >>> + sr |= SR_SRWD; >>> + } else { >>> + sr &= ~ATMEL_SR_GLOBAL_PROTECT_MASK; >>> + } >>> + >>> + nor->bouncebuf[0] = sr; >>> + >>> + /* >>> + * We cannot use the spi_nor_write_sr_and_check() because this >>> command >>> + * isn't really setting any bits, instead it is an pseudo >>> command for >>> + * "Global Unprotect" or "Global Protect" >>> + */ >>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1); >>> +} >>> + >>> +static int atmel_global_protect(struct spi_nor *nor, loff_t ofs, >>> uint64_t len) >>> +{ >>> + return atmel_set_global_protection(nor, ofs, len, true); >>> +} >>> + >>> +static int atmel_global_unprotect(struct spi_nor *nor, loff_t ofs, >>> uint64_t len) >>> +{ >>> + return atmel_set_global_protection(nor, ofs, len, false); >>> +} >>> + >>> +static int atmel_is_global_protected(struct spi_nor *nor, loff_t ofs, >>> uint64_t len) >>> +{ >>> + int ret; >>> + >>> + if (ofs >= nor->params->size || (ofs + len) > >>> nor->params->size) >>> + return -EINVAL; >>> + >>> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >>> + if (ret) >>> + return ret; >>> + >>> + return ((nor->bouncebuf[0] & ATMEL_SR_GLOBAL_PROTECT_MASK) == >>> ATMEL_SR_GLOBAL_PROTECT_MASK); >>> +} >>> + >>> +static const struct spi_nor_locking_ops atmel_global_protection_ops = >>> { >>> + .lock = atmel_global_protect, >>> + .unlock = atmel_global_unprotect, >>> + .is_locked = atmel_is_global_protected, >>> +}; >>> + >>> +static void atmel_global_protection_default_init(struct spi_nor *nor) >>> +{ >>> + nor->params->locking_ops = &atmel_global_protection_ops; >>> +} >>> + >>> +static const struct spi_nor_fixups atmel_global_protection_fixups = { >>> + .default_init = atmel_global_protection_default_init, >>> +}; >>> + >>> static const struct flash_info atmel_parts[] = { >>> /* Atmel -- some are (confusingly) marketed as "DataFlash" */ >>> { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K | >>> SPI_NOR_HAS_LOCK) >>> @@ -62,18 +161,32 @@ static const struct flash_info atmel_parts[] = { >>> { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K | >>> SPI_NOR_HAS_LOCK) >>> .fixups = &atmel_at25fs_fixups }, >>> >>> - { "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) }, >>> + { "at25df041a", INFO(0x1f4401, 0, 64 * 1024, 8, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> + { "at25df321", INFO(0x1f4700, 0, 64 * 1024, 64, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> + { "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> + { "at25df641", INFO(0x1f4800, 0, 64 * 1024, 128, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> >>> { "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 | >>> 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) }, >>> + { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> + { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> + { "at26df321", INFO(0x1f4700, 0, 64 * 1024, 64, >>> + SECT_4K | SPI_NOR_HAS_LOCK | >>> SPI_NOR_WP_IS_VOLATILE) >>> + .fixups = &atmel_global_protection_fixups }, >>> >>> { "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) }, >>> }; >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> index 8c06a28a90de..8354ce0c8810 100644 >>> --- a/drivers/mtd/spi-nor/core.c >>> +++ b/drivers/mtd/spi-nor/core.c >>> @@ -377,7 +377,7 @@ int spi_nor_write_disable(struct spi_nor *nor) >>> * >>> * Return: 0 on success, -errno otherwise. >>> */ >>> -static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) >>> +int spi_nor_read_sr(struct spi_nor *nor, u8 *sr) >>> { >>> int ret; >>> >>> @@ -1049,7 +1049,7 @@ static int >>> spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr) >>> * >>> * Return: 0 on success, -errno otherwise. >>> */ >>> -static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) >>> +int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) >>> { >>> if (nor->flags & SNOR_F_HAS_16BIT_SR) >>> return spi_nor_write_16bit_sr_and_check(nor, sr1); >>> @@ -3124,15 +3124,14 @@ static int spi_nor_quad_enable(struct spi_nor >>> *nor) >>> * spi_nor_unlock_all() - Unlocks the entire flash memory array. >>> * @nor: pointer to a 'struct spi_nor'. >>> * >>> - * Some SPI NOR flashes are write protected by default after a >>> power-on reset >>> - * cycle, in order to avoid inadvertent writes during power-up. >>> Backward >>> - * compatibility imposes to unlock the entire flash memory array at >>> power-up >>> - * by default. >>> + * Return: 0 on success, -errno otherwise. >>> */ >>> static int spi_nor_unlock_all(struct spi_nor *nor) >>> { >>> - if (nor->flags & SNOR_F_HAS_LOCK) >>> + if (nor->flags & SNOR_F_HAS_LOCK) { >>> + dev_dbg(nor->dev, "unprotecting entire flash\n"); >>> return spi_nor_unlock(&nor->mtd, 0, >>> nor->params->size); >>> + } >>> >>> return 0; >>> } >>> @@ -3153,10 +3152,23 @@ static int spi_nor_init(struct spi_nor *nor) >>> return err; >>> } >>> >>> - err = spi_nor_unlock_all(nor); >>> - if (err) { >>> - dev_dbg(nor->dev, "Failed to unlock the entire flash >>> memory array\n"); >>> - return err; >>> + /* >>> + * Some SPI NOR flashes are write protected by default after a >>> power-on >>> + * reset cycle, in order to avoid inadvertent writes during >>> power-up. >>> + * Backward compatibility imposes to unlock the entire flash >>> memory >>> + * array at power-up by default. Depending on the kernel >>> configuration >>> + * (1) we do nothing, (2) we unlock the entire flash in any >>> case or (3) >>> + * just do it actually powers up write-protected. The latter >>> is >> >> do it if it actually powers up >> >> How about: (1) do nothing, (2) always unlock the entire flash array, >> (3) unlock >> the entire flash array only when the software protection bits are >> volatile. > > ok > >>> + * indicated by SNOR_F_WP_IS_VOLATILE. >>> + */ >>> + if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) || >>> + (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) && >>> + nor->flags & SNOR_F_WP_IS_VOLATILE)) { >>> + err = spi_nor_unlock_all(nor); >>> + if (err) { >>> + dev_err(nor->dev, "Failed to unlock the entire >>> flash memory array\n"); >> >> dev_dbg for low level info > > Is this low level info or an actual error? Which raises the question: > should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus > should all the spi_nor_init fail of this? Or should it rather be a yes, it should, because the flash will not work as expected/requested. What most users care about is "my dev is not working properly". The low level info about the why, should be revealed when activating the debug traces. > soft error? > > Also I don't know how spi_nor_sr_unlock() will behave. > >>> + return err; >>> + } >>> } >>> > [..] > > -michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile 2020-12-02 11:10 ` Tudor.Ambarus @ 2020-12-02 11:25 ` Michael Walle 2020-12-02 14:09 ` Tudor.Ambarus 0 siblings, 1 reply; 15+ messages in thread From: Michael Walle @ 2020-12-02 11:25 UTC (permalink / raw) To: Tudor.Ambarus Cc: linux-mtd, linux-kernel, miquel.raynal, richard, vigneshr, boris.brezillon Am 2020-12-02 12:10, schrieb Tudor.Ambarus@microchip.com: > On 11/30/20 4:38 PM, Michael Walle wrote: [..] >>>> + * indicated by SNOR_F_WP_IS_VOLATILE. >>>> + */ >>>> + if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) || >>>> + (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) >>>> && >>>> + nor->flags & SNOR_F_WP_IS_VOLATILE)) { >>>> + err = spi_nor_unlock_all(nor); >>>> + if (err) { >>>> + dev_err(nor->dev, "Failed to unlock the >>>> entire >>>> flash memory array\n"); >>> >>> dev_dbg for low level info >> >> Is this low level info or an actual error? Which raises the question: >> should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus >> should all the spi_nor_init fail of this? Or should it rather be a > > yes, it should, because the flash will not work as expected/requested. One counterargument: take our sl28 board, it has a hardware write-protected SPI flash. It actually works right now because the write_sr_and_check() doesn't work as intended and doesn't check what is written. So if you'd fix that (and these changes would be backported to the stable trees), you'd basically break spi-nor on these boards. And this _must_ be the case for all boards which are actually using (hard- or sofware) write-protection. That is the only way write-protection makes sense prior to this patch series. Because linux will happily unlock every flash on startup. Therefore, the hardware write protection is the only measure against this. -michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile 2020-12-02 11:25 ` Michael Walle @ 2020-12-02 14:09 ` Tudor.Ambarus 0 siblings, 0 replies; 15+ messages in thread From: Tudor.Ambarus @ 2020-12-02 14:09 UTC (permalink / raw) To: michael Cc: linux-mtd, linux-kernel, miquel.raynal, richard, vigneshr, boris.brezillon On 12/2/20 1:25 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2020-12-02 12:10, schrieb Tudor.Ambarus@microchip.com: >> On 11/30/20 4:38 PM, Michael Walle wrote: > [..] >>>>> + * indicated by SNOR_F_WP_IS_VOLATILE. >>>>> + */ >>>>> + if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) || >>>>> + (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) >>>>> && >>>>> + nor->flags & SNOR_F_WP_IS_VOLATILE)) { >>>>> + err = spi_nor_unlock_all(nor); >>>>> + if (err) { >>>>> + dev_err(nor->dev, "Failed to unlock the >>>>> entire >>>>> flash memory array\n"); >>>> >>>> dev_dbg for low level info >>> >>> Is this low level info or an actual error? Which raises the question: >>> should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus >>> should all the spi_nor_init fail of this? Or should it rather be a >> >> yes, it should, because the flash will not work as expected/requested. > > One counterargument: take our sl28 board, it has a hardware > write-protected > SPI flash. It actually works right now because the write_sr_and_check() > doesn't work as intended and doesn't check what is written. So if you'd > fix that (and these changes would be backported to the stable trees), > you'd > basically break spi-nor on these boards. And this _must_ be the case for > all boards which are actually using (hard- or sofware) write-protection. > That is the only way write-protection makes sense prior to this patch > series. Because linux will happily unlock every flash on startup. > Therefore, > the hardware write protection is the only measure against this. > I see. If WP# is asserted, spi_nor_unlock_all() would fail and would stop the execution. One can avoid the fail by choosing MTD_SPI_NOR_SWP_KEEP, but that would not be backward compatible. Having in mind that in the past the unlock all was just a zero written to SR, without checking if the write was successful, I would now say that your proposal with the soft error if fair. Even if writes and erases will not work in case WP# is asserted, we would at least let users do reads. The writes and erases problems would be caught when enabling the debug traces. So please go ahead and print just a dev_dbg when spi_nor_unlock_all() fails, without stopping the execution. Cheers, ta ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-12-02 14:10 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-26 20:26 [PATCH v6 0/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 2020-11-26 20:26 ` [PATCH v6 1/5] mtd: spi-nor: atmel: remove global protection flag Michael Walle 2020-11-26 20:26 ` [PATCH v6 2/5] mtd: spi-nor: sst: " Michael Walle 2020-11-26 20:26 ` [PATCH v6 3/5] mtd: spi-nor: intel: " Michael Walle 2020-11-27 9:07 ` Tudor.Ambarus 2020-11-26 20:26 ` [PATCH v6 4/5] mtd: spi-nor: atmel: Fix unlock_all() for AT25FS010/040 Michael Walle 2020-11-28 8:25 ` Tudor.Ambarus 2020-11-30 14:16 ` Michael Walle 2020-12-02 10:32 ` Tudor.Ambarus 2020-11-26 20:26 ` [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile Michael Walle 2020-11-28 10:17 ` Tudor.Ambarus 2020-11-30 14:38 ` Michael Walle 2020-12-02 11:10 ` Tudor.Ambarus 2020-12-02 11:25 ` Michael Walle 2020-12-02 14:09 ` Tudor.Ambarus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).