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, the flashes in (2a)
and (2b) are compatible with each other in a sense that the "Global
Unprotect" command will clear the block protection bits in all the (2b)
flashes.
This patch adds a new flag to indicate the case (2). Only if we have
such a flash we perform a "Global Unprotect". To be backwards compatible
it also introduces a kernel configuration option which is by default
set to "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 <[email protected]>
---
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 | 35 +++++++++++++++++++++++++++++
drivers/mtd/spi-nor/atmel.c | 24 +++++++++++++-------
drivers/mtd/spi-nor/core.c | 44 ++++++++++++++++++++++++++++---------
drivers/mtd/spi-nor/core.h | 6 +++++
drivers/mtd/spi-nor/esmt.c | 6 ++---
drivers/mtd/spi-nor/intel.c | 6 ++---
drivers/mtd/spi-nor/sst.c | 21 +++++++++---------
include/linux/mtd/spi-nor.h | 6 +++++
8 files changed, 114 insertions(+), 34 deletions(-)
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 6e816eafb312..647de17c81e2 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -24,6 +24,41 @@ 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
+
+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.
+
+ 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 block protection bits.
+
+ 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 3f5f21a473a6..bad6736f5a47 100644
--- a/drivers/mtd/spi-nor/atmel.c
+++ b/drivers/mtd/spi-nor/atmel.c
@@ -13,18 +13,26 @@ static const struct flash_info atmel_parts[] = {
{ "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
{ "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
- { "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_UNPROTECT) },
+ { "at25df321", INFO(0x1f4700, 0, 64 * 1024, 64,
+ SECT_4K | SPI_NOR_UNPROTECT) },
+ { "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64,
+ SECT_4K | SPI_NOR_UNPROTECT) },
+ { "at25df641", INFO(0x1f4800, 0, 64 * 1024, 128,
+ SECT_4K | SPI_NOR_UNPROTECT) },
{ "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) },
+ { "at26f004", INFO(0x1f0400, 0, 64 * 1024, 8,
+ SECT_4K | SPI_NOR_UNPROTECT) },
+ { "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16,
+ SECT_4K | SPI_NOR_UNPROTECT) },
+ { "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32,
+ SECT_4K | SPI_NOR_UNPROTECT) },
+ { "at26df321", INFO(0x1f4700, 0, 64 * 1024, 64,
+ SECT_4K | SPI_NOR_UNPROTECT) },
{ "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 cc68ea84318e..fd1c36d70a13 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
}
/**
- * spi_nor_unlock_all() - Unlocks the entire flash memory array.
+ * spi_nor_global_unprotect() - Perform a global unprotect of the memory area.
* @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.
+ * by default. Do it only for flashes where the block protection bits
+ * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
+ *
+ * We cannot use spi_nor_unlock(nor->params.size) here because there are
+ * legacy devices (eg. AT25DF041A) which need a "global unprotect" command.
+ * This is done by writing 0b0x0000xx to the status register. This will also
+ * work for all other flashes which have these bits mapped to BP0 to BP3.
+ * The top most bit is ususally some kind of lock bit for the block
+ * protection bits.
*/
-static int spi_nor_unlock_all(struct spi_nor *nor)
+static int spi_nor_global_unprotect(struct spi_nor *nor)
{
- if (nor->flags & SNOR_F_HAS_LOCK)
- return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
+ int ret;
- return 0;
+ dev_dbg(nor->dev, "unprotecting entire flash\n");
+ ret = spi_nor_read_sr(nor, nor->bouncebuf);
+ if (ret)
+ return ret;
+
+ nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
+
+ /*
+ * Don't use spi_nor_write_sr1_and_check() because writing the status
+ * register might fail if the flash is hardware write protected.
+ */
+ return spi_nor_write_sr(nor, nor->bouncebuf, 1);
}
static int spi_nor_init(struct spi_nor *nor)
@@ -2942,10 +2960,14 @@ 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;
+ if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) ||
+ (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE) &&
+ nor->flags & SNOR_F_NEED_UNPROTECT)) {
+ err = spi_nor_global_unprotect(nor);
+ if (err) {
+ dev_err(nor->dev, "global unprotect failed\n");
+ return err;
+ }
}
if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
@@ -3170,6 +3192,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_UNPROTECT)
+ nor->flags |= SNOR_F_NEED_UNPROTECT;
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 6f2f6b27173f..9a33c023717f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -26,6 +26,7 @@ enum spi_nor_option_flags {
SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
SNOR_F_HAS_4BIT_BP = BIT(12),
SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
+ SNOR_F_NEED_UNPROTECT = BIT(14),
};
struct spi_nor_read_command {
@@ -311,6 +312,11 @@ struct flash_info {
* BP3 is bit 6 of status register.
* Must be used with SPI_NOR_4BIT_BP.
*/
+#define SPI_NOR_UNPROTECT BIT(19) /*
+ * Flash is write-protected after
+ * power-up and needs a global
+ * unprotect.
+ */
/* Part specific fixup hooks. */
const struct spi_nor_fixups *fixups;
diff --git a/drivers/mtd/spi-nor/esmt.c b/drivers/mtd/spi-nor/esmt.c
index c93170008118..f4b603b418ae 100644
--- a/drivers/mtd/spi-nor/esmt.c
+++ b/drivers/mtd/spi-nor/esmt.c
@@ -11,11 +11,11 @@
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_UNPROTECT) },
{ "f25l32qa", INFO(0x8c4116, 0, 64 * 1024, 64,
- SECT_4K | SPI_NOR_HAS_LOCK) },
+ SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_UNPROTECT) },
{ "f25l64qa", INFO(0x8c4117, 0, 64 * 1024, 128,
- SECT_4K | SPI_NOR_HAS_LOCK) },
+ SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_UNPROTECT) },
};
const struct spi_nor_manufacturer spi_nor_esmt = {
diff --git a/drivers/mtd/spi-nor/intel.c b/drivers/mtd/spi-nor/intel.c
index d8196f101368..001cafeb1b12 100644
--- a/drivers/mtd/spi-nor/intel.c
+++ b/drivers/mtd/spi-nor/intel.c
@@ -10,9 +10,9 @@
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) },
+ { "160s33b", INFO(0x898911, 0, 64 * 1024, 32, SPI_NOR_UNPROTECT) },
+ { "320s33b", INFO(0x898912, 0, 64 * 1024, 64, SPI_NOR_UNPROTECT) },
+ { "640s33b", INFO(0x898913, 0, 64 * 1024, 128, SPI_NOR_UNPROTECT) },
};
static void intel_default_init(struct spi_nor *nor)
diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index e0af6d25d573..5e0412caf40d 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) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "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_UNPROTECT) },
+ { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
+ SECT_4K | SPI_NOR_UNPROTECT) },
{ "sst25wf512", INFO(0xbf2501, 0, 64 * 1024, 1,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst25wf010", INFO(0xbf2502, 0, 64 * 1024, 2,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst25wf020", INFO(0xbf2503, 0, 64 * 1024, 4,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024, 4, SECT_4K) },
{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024, 8, SECT_4K) },
{ "sst25wf040", INFO(0xbf2504, 0, 64 * 1024, 8,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst25wf080", INFO(0xbf2505, 0, 64 * 1024, 16,
- SECT_4K | SST_WRITE) },
+ SECT_4K | SST_WRITE | SPI_NOR_UNPROTECT) },
{ "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,
SECT_4K | SPI_NOR_DUAL_READ |
SPI_NOR_QUAD_READ) },
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1e2af0ec1f03..f0a27cf8536b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -124,6 +124,12 @@
#define SR_BP_SHIFT 2
+/*
+ * Global unprotect is performed by writing the 0b0x0000xx to the status
+ * register.
+ */
+#define SR_GLOBAL_UNPROTECT_MASK 0xbc
+
/* Enhanced Volatile Configuration Register bits */
#define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */
--
2.20.1
On 3/27/20 9:29 PM, Michael Walle wrote:
> 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.
>
[...]
> 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.
>
Appreciate the detail commit log.
> Signed-off-by: Michael Walle <[email protected]>
> ---
> 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 | 35 +++++++++++++++++++++++++++++
> drivers/mtd/spi-nor/atmel.c | 24 +++++++++++++-------
> drivers/mtd/spi-nor/core.c | 44 ++++++++++++++++++++++++++++---------
> drivers/mtd/spi-nor/core.h | 6 +++++
> drivers/mtd/spi-nor/esmt.c | 6 ++---
> drivers/mtd/spi-nor/intel.c | 6 ++---
> drivers/mtd/spi-nor/sst.c | 21 +++++++++---------
> include/linux/mtd/spi-nor.h | 6 +++++
> 8 files changed, 114 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 6e816eafb312..647de17c81e2 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -24,6 +24,41 @@ 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
These choice control how BP0-X bits are manipulated on boot. Hence, to
be consistent should use Block Protection (BP) terminology throughout.
This would also be inline with most flash datasheets which also use term BP
> +
> +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.
> +
> + 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 block protection bits.
> +
> + 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/core.h b/drivers/mtd/spi-nor/core.h
> index 6f2f6b27173f..9a33c023717f 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
> SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
> SNOR_F_HAS_4BIT_BP = BIT(12),
> SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
> + SNOR_F_NEED_UNPROTECT = BIT(14),
> };
>
> struct spi_nor_read_command {
> @@ -311,6 +312,11 @@ struct flash_info {
> * BP3 is bit 6 of status register.
> * Must be used with SPI_NOR_4BIT_BP.
> */
> +#define SPI_NOR_UNPROTECT BIT(19) /*
> + * Flash is write-protected after
> + * power-up and needs a global
> + * unprotect.
> + */
>
It would be better to name the flag to indicate BP bits are volatile or
powers up locked instead of SPI_NOR_UNPROTECT. This makes it easier to
understand what this flag means wrt flash HW feature. Maybe:
SPI_NOR_LOCKED_ON_POWER_UP or SPI_NOR_BP_IS_VOLATILE
Reset looks fine to me
[...]
Regards
Vignesh
Hi, Michael,
PLease accept my apologies for the long delay.
I do agree with Vignesh's comments. Few others below.
On 3/27/20 5:59 PM, Michael Walle wrote:
[cut]
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc68ea84318e..fd1c36d70a13 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
> }
>
> /**
> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
> + * spi_nor_global_unprotect() - Perform a global unprotect of the memory area.
> * @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.
> + * by default. Do it only for flashes where the block protection bits
> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
> + *
> + * We cannot use spi_nor_unlock(nor->params.size) here because there are
> + * legacy devices (eg. AT25DF041A) which need a "global unprotect" command.
> + * This is done by writing 0b0x0000xx to the status register. This will also
> + * work for all other flashes which have these bits mapped to BP0 to BP3.
> + * The top most bit is ususally some kind of lock bit for the block
> + * protection bits.
> */
> -static int spi_nor_unlock_all(struct spi_nor *nor)
> +static int spi_nor_global_unprotect(struct spi_nor *nor)
> {
> - if (nor->flags & SNOR_F_HAS_LOCK)
> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
> + int ret;
>
> - return 0;
> + dev_dbg(nor->dev, "unprotecting entire flash\n");
> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
> + if (ret)
> + return ret;
> +
> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
> +
> + /*
> + * Don't use spi_nor_write_sr1_and_check() because writing the status
> + * register might fail if the flash is hardware write protected.
> + */
> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
> }
This won't work for all the flashes. You use a GENMASK(5, 2) to clear
the Status Register even for BP0-2 flashes and you end up clearing BIT(5)
which can lead to side effects.
We should instead introduce a nor->params->locking_ops->global_unlock() hook
for the flashes that have special opcodes that unlock all the flash blocks,
or for the flashes that deviate from the "clear just your BP bits" rule.
you can keep the call to spi_nor_unlock(&nor->mtd, 0, nor->params->size);
and in spi_nor_unlock() do:
if (len == nor->params->size && nor->params->locking_ops->global_unlock)
ret = nor->params->locking_ops->global_unlock(nor)
else
ret = nor->params->locking_ops->unlock(nor, ofs, len);
Cheers,
ta
Hi Tudor,
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index cc68ea84318e..fd1c36d70a13 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct spi_nor
>> *nor)
>> }
>>
>> /**
>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>> + * spi_nor_global_unprotect() - Perform a global unprotect of the
>> memory area.
>> * @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.
>> + * by default. Do it only for flashes where the block protection bits
>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
>> + *
>> + * We cannot use spi_nor_unlock(nor->params.size) here because there
>> are
>> + * legacy devices (eg. AT25DF041A) which need a "global unprotect"
>> command.
>> + * This is done by writing 0b0x0000xx to the status register. This
>> will also
>> + * work for all other flashes which have these bits mapped to BP0 to
>> BP3.
>> + * The top most bit is ususally some kind of lock bit for the block
>> + * protection bits.
>> */
>> -static int spi_nor_unlock_all(struct spi_nor *nor)
>> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>> {
>> - if (nor->flags & SNOR_F_HAS_LOCK)
>> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
>> + int ret;
>>
>> - return 0;
>> + dev_dbg(nor->dev, "unprotecting entire flash\n");
>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> + if (ret)
>> + return ret;
>> +
>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
>> +
>> + /*
>> + * Don't use spi_nor_write_sr1_and_check() because writing the
>> status
>> + * register might fail if the flash is hardware write protected.
>> + */
>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>> }
>
> This won't work for all the flashes. You use a GENMASK(5, 2) to clear
> the Status Register even for BP0-2 flashes and you end up clearing
> BIT(5)
> which can lead to side effects.
>
> We should instead introduce a nor->params->locking_ops->global_unlock()
> hook
> for the flashes that have special opcodes that unlock all the flash
> blocks,
> or for the flashes that deviate from the "clear just your BP bits"
> rule.
Wouldn't it make more sense to just set params->locking_ops for these
flashes
to different functions? or even provide a spi_nor_global_unprotect_ops
in
core.c and these flashes will just set them. there is no individual
sector
range lock for these chips. just a lock all or nothing.
If it is more common and not just one vendor it might also make sense to
add
a seperate flag which will then set the locking ops to
spi_nor_global_unprotect_ops, also it seems that there have to be two
writes
to the status register, one to clear SPRL and then one to clear the BP
bits.
See for example [1] Table 9-2. I'll have to go through the datasheets
again
to see what flashes just have a global (un)protect.
[1] https://www.adestotech.com/wp-content/uploads/doc3668.pdf
-michael
Am 2020-09-30 12:35, schrieb Vignesh Raghavendra:
[..]
>> Signed-off-by: Michael Walle <[email protected]>
>> ---
>> 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 | 35 +++++++++++++++++++++++++++++
>> drivers/mtd/spi-nor/atmel.c | 24 +++++++++++++-------
>> drivers/mtd/spi-nor/core.c | 44
>> ++++++++++++++++++++++++++++---------
>> drivers/mtd/spi-nor/core.h | 6 +++++
>> drivers/mtd/spi-nor/esmt.c | 6 ++---
>> drivers/mtd/spi-nor/intel.c | 6 ++---
>> drivers/mtd/spi-nor/sst.c | 21 +++++++++---------
>> include/linux/mtd/spi-nor.h | 6 +++++
>> 8 files changed, 114 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 6e816eafb312..647de17c81e2 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -24,6 +24,41 @@ 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
>
> These choice control how BP0-X bits are manipulated on boot. Hence, to
> be consistent should use Block Protection (BP) terminology throughout.
>
> This would also be inline with most flash datasheets which also use
> term BP
Where should I mention the BP bits? In the choice or in the help text.
I tried to keep the choice prompt as easy to understand as possible, so
an
user who doesn't know anything about how the write protection actually
works
can still make that choice (without first reading a datasheet). Also
keep in
mind, that there is also the per sector locking. Wouldn't this option
also
apply to that?
Therefore, I'd mention in the help text, that (currently) the BP bits
are
affected.
>> +
>> +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.
>> +
>> + 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 block protection bits.
>> +
>> + 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/core.h b/drivers/mtd/spi-nor/core.h
>> index 6f2f6b27173f..9a33c023717f 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
>> SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
>> SNOR_F_HAS_4BIT_BP = BIT(12),
>> SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
>> + SNOR_F_NEED_UNPROTECT = BIT(14),
>> };
>>
>> struct spi_nor_read_command {
>> @@ -311,6 +312,11 @@ struct flash_info {
>> * BP3 is bit 6 of status register.
>> * Must be used with SPI_NOR_4BIT_BP.
>> */
>> +#define SPI_NOR_UNPROTECT BIT(19) /*
>> + * Flash is write-protected after
>> + * power-up and needs a global
>> + * unprotect.
>> + */
>>
>
> It would be better to name the flag to indicate BP bits are volatile or
> powers up locked instead of SPI_NOR_UNPROTECT. This makes it easier to
> understand what this flag means wrt flash HW feature. Maybe:
>
> SPI_NOR_LOCKED_ON_POWER_UP or SPI_NOR_BP_IS_VOLATILE
SPI_NOR_BP_IS_VOLATILE sounds good to me.
-michael
On 10/1/20 1:38 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Tudor,
Hi, Michael,
>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index cc68ea84318e..fd1c36d70a13 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct spi_nor
>>> *nor)
>>> }
>>>
>>> /**
>>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>>> + * spi_nor_global_unprotect() - Perform a global unprotect of the
>>> memory area.
>>> * @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.
>>> + * by default. Do it only for flashes where the block protection bits
>>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
>>> + *
>>> + * We cannot use spi_nor_unlock(nor->params.size) here because there
>>> are
>>> + * legacy devices (eg. AT25DF041A) which need a "global unprotect"
>>> command.
>>> + * This is done by writing 0b0x0000xx to the status register. This
>>> will also
>>> + * work for all other flashes which have these bits mapped to BP0 to
>>> BP3.
>>> + * The top most bit is ususally some kind of lock bit for the block
>>> + * protection bits.
>>> */
>>> -static int spi_nor_unlock_all(struct spi_nor *nor)
>>> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>>> {
>>> - if (nor->flags & SNOR_F_HAS_LOCK)
>>> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
>>> + int ret;
>>>
>>> - return 0;
>>> + dev_dbg(nor->dev, "unprotecting entire flash\n");
>>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
>>> +
>>> + /*
>>> + * Don't use spi_nor_write_sr1_and_check() because writing the
>>> status
>>> + * register might fail if the flash is hardware write protected.
>>> + */
>>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>>> }
>>
>> This won't work for all the flashes. You use a GENMASK(5, 2) to clear
>> the Status Register even for BP0-2 flashes and you end up clearing
>> BIT(5)
>> which can lead to side effects.
>>
>> We should instead introduce a nor->params->locking_ops->global_unlock()
>> hook
>> for the flashes that have special opcodes that unlock all the flash
>> blocks,
>> or for the flashes that deviate from the "clear just your BP bits"
>> rule.
>
> Wouldn't it make more sense to just set params->locking_ops for these
> flashes
> to different functions? or even provide a spi_nor_global_unprotect_ops
> in
> core.c and these flashes will just set them. there is no individual
> sector
> range lock for these chips. just a lock all or nothing.
I like the idea of having all locking related functions placed in a single
place, thus the global_unlock() should be inside locking_ops struct.
You can update params->locking_ops. If we have vendor specific locking_ops
we can set them via:
nor->manufacturer->fixups->default_init(nor);
or if they are flash specific, with a smaller granularity, via:
nor->info->fixups->default_init(nor);
>
> If it is more common and not just one vendor it might also make sense to
> add
> a seperate flag which will then set the locking ops to
> spi_nor_global_unprotect_ops, also it seems that there have to be two
I don't like that we have so many flags, so I try to avoid introducing new
ones as best as I can. Checking for null pointer should suffice.
> writes
> to the status register, one to clear SPRL and then one to clear the BP
> bits.
SPR is pretty common, also available in Winbond and Micron. I see that
it is a non-volatile bit for these vendors. NV bits are of concern
because you can wear them down with multiple set/clear ops. We have to
think how we'll deal with NV bits in general in SPI NOR.
> See for example [1] Table 9-2. I'll have to go through the datasheets
> again
> to see what flashes just have a global (un)protect.
Yes, sorry for that, my bad. I can help if you want, I can go through
datasheets to.
Let's not introduce the global unlock ops commands yet, let's just
solve the backward compatibility issues with the existing code. We can
add new opcodes later on.
Also, I can scratch a patch with the global_unlock() hook if you prefer,
so that we can talk on code.
Cheers,
ta
>
> [1] https://www.adestotech.com/wp-content/uploads/doc3668.pdf
>
> -michael
Hi Tudor,
Am 2020-10-01 09:07, schrieb [email protected]:
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index cc68ea84318e..fd1c36d70a13 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct
>>>> spi_nor
>>>> *nor)
>>>> }
>>>>
>>>> /**
>>>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>>>> + * spi_nor_global_unprotect() - Perform a global unprotect of the
>>>> memory area.
>>>> * @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.
>>>> + * by default. Do it only for flashes where the block protection
>>>> bits
>>>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
>>>> + *
>>>> + * We cannot use spi_nor_unlock(nor->params.size) here because
>>>> there
>>>> are
>>>> + * legacy devices (eg. AT25DF041A) which need a "global unprotect"
>>>> command.
>>>> + * This is done by writing 0b0x0000xx to the status register. This
>>>> will also
>>>> + * work for all other flashes which have these bits mapped to BP0
>>>> to
>>>> BP3.
>>>> + * The top most bit is ususally some kind of lock bit for the block
>>>> + * protection bits.
>>>> */
>>>> -static int spi_nor_unlock_all(struct spi_nor *nor)
>>>> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>>>> {
>>>> - if (nor->flags & SNOR_F_HAS_LOCK)
>>>> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
>>>> + int ret;
>>>>
>>>> - return 0;
>>>> + dev_dbg(nor->dev, "unprotecting entire flash\n");
>>>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
>>>> +
>>>> + /*
>>>> + * Don't use spi_nor_write_sr1_and_check() because writing the
>>>> status
>>>> + * register might fail if the flash is hardware write
>>>> protected.
>>>> + */
>>>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>>>> }
>>>
>>> This won't work for all the flashes. You use a GENMASK(5, 2) to clear
>>> the Status Register even for BP0-2 flashes and you end up clearing
>>> BIT(5)
>>> which can lead to side effects.
>>>
>>> We should instead introduce a
>>> nor->params->locking_ops->global_unlock()
>>> hook
>>> for the flashes that have special opcodes that unlock all the flash
>>> blocks,
>>> or for the flashes that deviate from the "clear just your BP bits"
>>> rule.
>>
>> Wouldn't it make more sense to just set params->locking_ops for these
>> flashes
>> to different functions? or even provide a spi_nor_global_unprotect_ops
>> in
>> core.c and these flashes will just set them. there is no individual
>> sector
>> range lock for these chips. just a lock all or nothing.
>
> I like the idea of having all locking related functions placed in a
> single
> place, thus the global_unlock() should be inside locking_ops struct.
My point was that this global unlock shouldn't be a special case for the
current spi_nor_unlock() but just another "how to unlock the flash"
function
and thus should replace the original unlock op. For example, it is also
likely
that you need a special global lock (i.e. write all 1's).
static int spi_nor_global_unlock()
{
write_sr(0); /* actually it will be a read-modify write */
}
static int spi_nor_global_lock()
{
write_sr(0x1c);
}
static int spi_nor_is_global_locked()
{
return read_sr() & 0x1c;
}
const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
.lock = spi_nor_global_unlock,
.unlock = spi_nor_global_lock,
.is_locked = spi_nor_is_global_locked,
};
Having the spi_nor_unlock decide what op to choose introduces just
another indirection. Esp. if you think about having support for
individual sector protection which also needs new ops. Btw. to me
it seems that "global (un)lock" is almost always used for the
individual sector protection scheme, i.e. like a shortcut to allow all
sectors be unlocked at once.
> You can update params->locking_ops. If we have vendor specific
> locking_ops
> we can set them via:
> nor->manufacturer->fixups->default_init(nor);
> or if they are flash specific, with a smaller granularity, via:
> nor->info->fixups->default_init(nor);
ok.
>> If it is more common and not just one vendor it might also make sense
>> to
>> add
>> a seperate flag which will then set the locking ops to
>> spi_nor_global_unprotect_ops, also it seems that there have to be two
>
> I don't like that we have so many flags, so I try to avoid introducing
> new
> ones as best as I can. Checking for null pointer should suffice.
Yeah, I already guessed that this would be the answer ;)
-michael
On 10/1/20 4:21 AM, Michael Walle wrote:
> Am 2020-09-30 12:35, schrieb Vignesh Raghavendra:
> [..]
>>> Signed-off-by: Michael Walle <[email protected]>
>>> ---
>>> 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 | 35 +++++++++++++++++++++++++++++
>>> drivers/mtd/spi-nor/atmel.c | 24 +++++++++++++-------
>>> drivers/mtd/spi-nor/core.c | 44 ++++++++++++++++++++++++++++---------
>>> drivers/mtd/spi-nor/core.h | 6 +++++
>>> drivers/mtd/spi-nor/esmt.c | 6 ++---
>>> drivers/mtd/spi-nor/intel.c | 6 ++---
>>> drivers/mtd/spi-nor/sst.c | 21 +++++++++---------
>>> include/linux/mtd/spi-nor.h | 6 +++++
>>> 8 files changed, 114 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>>> index 6e816eafb312..647de17c81e2 100644
>>> --- a/drivers/mtd/spi-nor/Kconfig
>>> +++ b/drivers/mtd/spi-nor/Kconfig
>>> @@ -24,6 +24,41 @@ 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
>>
>> These choice control how BP0-X bits are manipulated on boot. Hence, to
>> be consistent should use Block Protection (BP) terminology throughout.
>>
>> This would also be inline with most flash datasheets which also use
>> term BP
>
> Where should I mention the BP bits? In the choice or in the help text.
choice prompt and in help text.
> I tried to keep the choice prompt as easy to understand as possible, so an
> user who doesn't know anything about how the write protection actually
> works
> can still make that choice (without first reading a datasheet). Also
> keep in
> mind, that there is also the per sector locking. Wouldn't this option also
> apply to that?
>
But, there is more than one way of achieving write protection in
addition to BP bits such as Advanced Sector Protection which may be
added in future. So we should be as specific as possible while making it
easy for user.
How about
prompt "Write Protection (Legacy Block Protection based) at boot"
> Therefore, I'd mention in the help text, that (currently) the BP bits are
> affected.
>
>>> +
>>> +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.
>>> +
>>> + 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 block protection bits.
>>> +
>>> + 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/core.h b/drivers/mtd/spi-nor/core.h
>>> index 6f2f6b27173f..9a33c023717f 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
>>> SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
>>> SNOR_F_HAS_4BIT_BP = BIT(12),
>>> SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
>>> + SNOR_F_NEED_UNPROTECT = BIT(14),
>>> };
>>>
>>> struct spi_nor_read_command {
>>> @@ -311,6 +312,11 @@ struct flash_info {
>>> * BP3 is bit 6 of status register.
>>> * Must be used with SPI_NOR_4BIT_BP.
>>> */
>>> +#define SPI_NOR_UNPROTECT BIT(19) /*
>>> + * Flash is write-protected after
>>> + * power-up and needs a global
>>> + * unprotect.
>>> + */
>>>
>>
>> It would be better to name the flag to indicate BP bits are volatile or
>> powers up locked instead of SPI_NOR_UNPROTECT. This makes it easier to
>> understand what this flag means wrt flash HW feature. Maybe:
>>
>> SPI_NOR_LOCKED_ON_POWER_UP or SPI_NOR_BP_IS_VOLATILE
>
> SPI_NOR_BP_IS_VOLATILE sounds good to me.
>
> -michael
On 10/1/20 10:38 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Tudor,
Hi, Michael,
>
> Am 2020-10-01 09:07, schrieb [email protected]:
>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>> index cc68ea84318e..fd1c36d70a13 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct
>>>>> spi_nor
>>>>> *nor)
>>>>> }
>>>>>
>>>>> /**
>>>>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>>>>> + * spi_nor_global_unprotect() - Perform a global unprotect of the
>>>>> memory area.
>>>>> * @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.
>>>>> + * by default. Do it only for flashes where the block protection
>>>>> bits
>>>>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
>>>>> + *
>>>>> + * We cannot use spi_nor_unlock(nor->params.size) here because
>>>>> there
>>>>> are
>>>>> + * legacy devices (eg. AT25DF041A) which need a "global unprotect"
>>>>> command.
>>>>> + * This is done by writing 0b0x0000xx to the status register. This
>>>>> will also
>>>>> + * work for all other flashes which have these bits mapped to BP0
>>>>> to
>>>>> BP3.
>>>>> + * The top most bit is ususally some kind of lock bit for the block
>>>>> + * protection bits.
>>>>> */
>>>>> -static int spi_nor_unlock_all(struct spi_nor *nor)
>>>>> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>>>>> {
>>>>> - if (nor->flags & SNOR_F_HAS_LOCK)
>>>>> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
>>>>> + int ret;
>>>>>
>>>>> - return 0;
>>>>> + dev_dbg(nor->dev, "unprotecting entire flash\n");
>>>>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
>>>>> +
>>>>> + /*
>>>>> + * Don't use spi_nor_write_sr1_and_check() because writing the
>>>>> status
>>>>> + * register might fail if the flash is hardware write
>>>>> protected.
>>>>> + */
>>>>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>>>>> }
>>>>
>>>> This won't work for all the flashes. You use a GENMASK(5, 2) to clear
>>>> the Status Register even for BP0-2 flashes and you end up clearing
>>>> BIT(5)
>>>> which can lead to side effects.
>>>>
>>>> We should instead introduce a
>>>> nor->params->locking_ops->global_unlock()
>>>> hook
>>>> for the flashes that have special opcodes that unlock all the flash
>>>> blocks,
>>>> or for the flashes that deviate from the "clear just your BP bits"
>>>> rule.
>>>
>>> Wouldn't it make more sense to just set params->locking_ops for these
>>> flashes
>>> to different functions? or even provide a spi_nor_global_unprotect_ops
>>> in
>>> core.c and these flashes will just set them. there is no individual
>>> sector
>>> range lock for these chips. just a lock all or nothing.
>>
>> I like the idea of having all locking related functions placed in a
>> single
>> place, thus the global_unlock() should be inside locking_ops struct.
>
> My point was that this global unlock shouldn't be a special case for the
> current spi_nor_unlock() but just another "how to unlock the flash"
> function
> and thus should replace the original unlock op. For example, it is also
> likely
> that you need a special global lock (i.e. write all 1's).
>
> static int spi_nor_global_unlock()
> {
> write_sr(0); /* actually it will be a read-modify write */
> }
>
> static int spi_nor_global_lock()
> {
> write_sr(0x1c);
> }
>
> static int spi_nor_is_global_locked()
> {
> return read_sr() & 0x1c;
> }
>
> const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
> .lock = spi_nor_global_unlock,
> .unlock = spi_nor_global_lock,
> .is_locked = spi_nor_is_global_locked,
> };
Meh, this would be valid only if the flash supports _just_ global (un)lock,
without supporting locking on a smaller granularity. Otherwise, people will
get lazy and just add support for global (unlock) without introducing
software for smaller granularity locking, which would be a pity.
If there's such a case, those functions should be manufacturer/flash specific.
>
> Having the spi_nor_unlock decide what op to choose introduces just
> another indirection. Esp. if you think about having support for
> individual sector protection which also needs new ops. Btw. to me
> it seems that "global (un)lock" is almost always used for the
> individual sector protection scheme, i.e. like a shortcut to allow all
> sectors be unlocked at once.
>
Probably yes, the global unlock command is tied to individual block locking,
will have to check. And yes, a global unlock command should offer a single
command cycle that unlocks the entire memory array. It should be preferred
when one wants to unlock the entire flash.
Cheers,
ta
Hi Tudor,
Am 2020-10-01 13:46, schrieb [email protected]:
>> Am 2020-10-01 09:07, schrieb [email protected]:
>>>>>> diff --git a/drivers/mtd/spi-nor/core.c
>>>>>> b/drivers/mtd/spi-nor/core.c
>>>>>> index cc68ea84318e..fd1c36d70a13 100644
>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct
>>>>>> spi_nor
>>>>>> *nor)
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>>>>>> + * spi_nor_global_unprotect() - Perform a global unprotect of the
>>>>>> memory area.
>>>>>> * @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.
>>>>>> + * by default. Do it only for flashes where the block protection
>>>>>> bits
>>>>>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
>>>>>> + *
>>>>>> + * We cannot use spi_nor_unlock(nor->params.size) here because
>>>>>> there
>>>>>> are
>>>>>> + * legacy devices (eg. AT25DF041A) which need a "global
>>>>>> unprotect"
>>>>>> command.
>>>>>> + * This is done by writing 0b0x0000xx to the status register.
>>>>>> This
>>>>>> will also
>>>>>> + * work for all other flashes which have these bits mapped to BP0
>>>>>> to
>>>>>> BP3.
>>>>>> + * The top most bit is ususally some kind of lock bit for the
>>>>>> block
>>>>>> + * protection bits.
>>>>>> */
>>>>>> -static int spi_nor_unlock_all(struct spi_nor *nor)
>>>>>> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>>>>>> {
>>>>>> - if (nor->flags & SNOR_F_HAS_LOCK)
>>>>>> - return spi_nor_unlock(&nor->mtd, 0,
>>>>>> nor->params->size);
>>>>>> + int ret;
>>>>>>
>>>>>> - return 0;
>>>>>> + dev_dbg(nor->dev, "unprotecting entire flash\n");
>>>>>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
>>>>>> +
>>>>>> + /*
>>>>>> + * Don't use spi_nor_write_sr1_and_check() because writing
>>>>>> the
>>>>>> status
>>>>>> + * register might fail if the flash is hardware write
>>>>>> protected.
>>>>>> + */
>>>>>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>>>>>> }
>>>>>
>>>>> This won't work for all the flashes. You use a GENMASK(5, 2) to
>>>>> clear
>>>>> the Status Register even for BP0-2 flashes and you end up clearing
>>>>> BIT(5)
>>>>> which can lead to side effects.
>>>>>
>>>>> We should instead introduce a
>>>>> nor->params->locking_ops->global_unlock()
>>>>> hook
>>>>> for the flashes that have special opcodes that unlock all the flash
>>>>> blocks,
>>>>> or for the flashes that deviate from the "clear just your BP bits"
>>>>> rule.
>>>>
>>>> Wouldn't it make more sense to just set params->locking_ops for
>>>> these
>>>> flashes
>>>> to different functions? or even provide a
>>>> spi_nor_global_unprotect_ops
>>>> in
>>>> core.c and these flashes will just set them. there is no individual
>>>> sector
>>>> range lock for these chips. just a lock all or nothing.
>>>
>>> I like the idea of having all locking related functions placed in a
>>> single
>>> place, thus the global_unlock() should be inside locking_ops struct.
>>
>> My point was that this global unlock shouldn't be a special case for
>> the
>> current spi_nor_unlock() but just another "how to unlock the flash"
>> function
>> and thus should replace the original unlock op. For example, it is
>> also
>> likely
>> that you need a special global lock (i.e. write all 1's).
>>
>> static int spi_nor_global_unlock()
>> {
>> write_sr(0); /* actually it will be a read-modify write */
>> }
>>
>> static int spi_nor_global_lock()
>> {
>> write_sr(0x1c);
>> }
>>
>> static int spi_nor_is_global_locked()
>> {
>> return read_sr() & 0x1c;
>> }
>>
>> const struct spi_nor_locking_ops spi_nor_sr_locking_ops = {
>> .lock = spi_nor_global_unlock,
>> .unlock = spi_nor_global_lock,
>> .is_locked = spi_nor_is_global_locked,
>> };
>
> Meh, this would be valid only if the flash supports _just_ global
> (un)lock,
> without supporting locking on a smaller granularity.
Most (all?) of these flashes will support the per-sector protection,
which
we doesn't support. So yes, this is currently a shortcut for the flashes
which were unlocked by default in the past, but doesn't fall into the BP
bits category.
It doesn't feel right to add this as a special case to the current
spi_nor_lock(), which is really a spi_nor_bp_lock(). And this locking
has nothing to do with BP bits. If there would be support for per-sector
protection, then this would end up there. But there isn't.
> Otherwise, people will
> get lazy and just add support for global (unlock) without introducing
> software for smaller granularity locking, which would be a pity.
I can't relate to this. If people need per-sector locking, there will
eventually be code. If people just need a protect all or nothing, well
then.
> If there's such a case, those functions should be manufacturer/flash
> specific.
Lucky me, this is (at the moment) just Atmel. So is it acceptable,
if I'll put this global unlock ops into atmel.c? I mean eventually, this
will be replaced by proper per-sector locking. It is just there to
support
the flashes which was already unlocked that way in the past. In terms
of backward compatibility there won't be more flashes which need this
global unlock.
But then this relates to the question how I should name the menuconfig
choices. Because these flashes doesn't have the block protection bits.
-michael