[PATCH v1 0/7] fix and improve Micron/SST26* SPI NOR protection handling

[PATCH v1 0/7] fix and improve Micron/SST26* SPI NOR protection handling

[Bernhard Kirchen]

these patches improve the existing SPI NOR flash protection features by
enabling hardware write protection where applicable, by fixing bugs, and
by providing more information on the status of protection. this shall
make using the protection feature more user-friendly and help meeting
user expectations.


Bernhard Kirchen (7):
  command sf: help text format
  sf protect: warn about failed (un)lock operation
  SST26* locking: need to enable write
  write WPEN bit for SST26* devices when locking
  sf protect (un)lock for SST26*: test BPR values
  fix sst26_process_bpr check
  provide "sf protect check" command

 cmd/sf.c                       |  74 ++++++++-----
 drivers/mtd/spi/spi-nor-core.c | 186 ++++++++++++++++++++++++++++-----
 include/linux/mtd/spi-nor.h    |  18 +++-
 include/spi_flash.h            |   8 ++
 4 files changed, 232 insertions(+), 54 deletions(-)

-- 
2.29.2

[PATCH v1 1/7] command sf: help text format

[Bernhard Kirchen]

properly indent the help text and use single quotes consistently to mark
variable parameters.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 cmd/sf.c | 48 ++++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index c0d6a8f8a0..a991ae0d03 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -564,7 +564,7 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	/* The remaining commands require a selected device */
 	if (!flash) {
-		puts("No SPI flash selected. Please run `sf probe'\n");
+		puts("No SPI flash selected. Please run 'sf probe'\n");
 		return 1;
 	}
 
@@ -590,31 +590,27 @@ usage:
 	return CMD_RET_USAGE;
 }
 
-#ifdef CONFIG_CMD_SF_TEST
-#define SF_TEST_HELP "\nsf test offset len		" \
-		"- run a very basic destructive test"
-#else
-#define SF_TEST_HELP
-#endif
-
 U_BOOT_CMD(
-	sf,	5,	1,	do_spi_flash,
+	sf, 5, 1, do_spi_flash,
 	"SPI flash sub-system",
-	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
-	"				  and chip select\n"
-	"sf read addr offset|partition len	- read `len' bytes starting at\n"
-	"				          `offset' or from start of mtd\n"
-	"					  `partition'to memory at `addr'\n"
-	"sf write addr offset|partition len	- write `len' bytes from memory\n"
-	"				          at `addr' to flash at `offset'\n"
-	"					  or to start of mtd `partition'\n"
-	"sf erase offset|partition [+]len	- erase `len' bytes from `offset'\n"
-	"					  or from start of mtd `partition'\n"
-	"					 `+len' round up `len' to block size\n"
-	"sf update addr offset|partition len	- erase and write `len' bytes from memory\n"
-	"					  at `addr' to flash at `offset'\n"
-	"					  or to start of mtd `partition'\n"
-	"sf protect lock/unlock sector len	- protect/unprotect 'len' bytes starting\n"
-	"					  at address 'sector'\n"
-	SF_TEST_HELP
+	   "probe [[bus:]cs] [hz] [mode]        - init flash device on given SPI bus\n"
+	"                                         and chip select\n"
+	"sf read addr offset|partition len      - read 'len' bytes starting at\n"
+	"                                         'offset' or from start of mtd\n"
+	"                                         'partition' to memory at 'addr'\n"
+	"sf write addr offset|partition len     - write 'len' bytes from memory\n"
+	"                                         at 'addr' to flash at 'offset'\n"
+	"                                         or to start of mtd 'partition'\n"
+	"sf erase offset|partition [+]len       - erase 'len' bytes from 'offset'\n"
+	"                                         or from start of mtd 'partition'.\n"
+	"                                         '+len' round up 'len' to block size\n"
+	"sf update addr offset|partition len    - erase and write 'len' bytes from memory\n"
+	"                                         at 'addr' to flash at 'offset'\n"
+	"                                         or to start of mtd 'partition'\n"
+	"sf protect lock|unlock sector len      - protect/unprotect 'len' bytes starting\n"
+	"                                         at address 'sector'\n"
+#ifdef CONFIG_CMD_SF_TEST
+	"sf test offset len                     - run a very basic destructive test\n"
+	"                                         ranging 'len' bytes from 'offset'\n"
+#endif
 );
-- 
2.29.2

[PATCH v1 2/7] sf protect: warn about failed (un)lock operation

[Bernhard Kirchen]

it is not guaranteed that there is a human readable message when the
lock or unlock operation failed. make sure there is a message emitted
by the "sf protect" implementation if the subcommand failed.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 cmd/sf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index a991ae0d03..ecd2918cbc 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -378,7 +378,12 @@ static int do_spi_protect(int argc, char *const argv[])
 
 	ret = spi_flash_protect(flash, start, len, prot);
 
-	return ret == 0 ? 0 : 1;
+	if (ret != 0) {
+		printf("ERROR: %slocking operation failed (%d)\n", (prot ? "" : "un"), ret);
+		return 1;
+	}
+
+	return 0;
 }
 
 #ifdef CONFIG_CMD_SF_TEST
-- 
2.29.2

[PATCH v1 3/7] SST26* locking: need to enable write

[Bernhard Kirchen]

prior to using the WBPR (write block protection register) command to
write new block protection register values, the WREN command must be
sent. otherwise the new values are not applied.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 drivers/mtd/spi/spi-nor-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index e16b0e1462..050aeac3fa 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1091,12 +1091,20 @@ static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lo
 	if (ctl == SST26_CTL_CHECK)
 		return 0;
 
+	ret = write_enable(nor);
+	if (ret < 0)
+		return ret;
+
 	ret = nor->write_reg(nor, SPINOR_OP_WRITE_BPR, bpr_buff, bpr_size);
 	if (ret < 0) {
 		dev_err(nor->dev, "fail to write block-protection register\n");
 		return ret;
 	}
 
+	// ignore return value. even if write disable failed, the actual task
+	// (write block protecton register) was completed successfully.
+	write_disable(nor);
+
 	return 0;
 }
 
-- 
2.29.2

[PATCH v1 4/7] write WPEN bit for SST26* devices when locking

[Bernhard Kirchen]

"sf protect lock" did only protect against accidental writes by
software. it did not lock down the config or block-protection registers
if the WP# pin was deasserted. hardware write protection was never
enabled for these devices.

this change implements setting the WPEN bit and clearing the IOC bit in
SST26* devices' config register if any block is protected by "sf protect
lock" command, thus enabling hardware write protection. this behavior is
similar to the "sf protect lock" implementation for Micron (and
compatible) chips.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 drivers/mtd/spi/spi-nor-core.c | 64 ++++++++++++++++++++++++++++++++--
 include/linux/mtd/spi-nor.h    |  1 +
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 050aeac3fa..10c01cf20e 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -185,7 +185,9 @@ static int read_fsr(struct spi_nor *nor)
  * location. Return the configuration register value.
  * Returns negative if error occurred.
  */
-#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
+#if defined(CONFIG_SPI_FLASH_SPANSION) || \
+	defined(CONFIG_SPI_FLASH_WINBOND) || \
+	defined(CONFIG_SPI_FLASH_SST)
 static int read_cr(struct spi_nor *nor)
 {
 	int ret;
@@ -972,6 +974,8 @@ read_err:
 #define SST26_MAX_BPR_REG_LEN		(18 + 1)
 #define SST26_BOUND_REG_SIZE		((32 + SST26_BPR_8K_NUM * 8) * SZ_1K)
 
+static int write_cr(struct spi_nor *nor, u8 value);
+
 enum lock_ctl {
 	SST26_CTL_LOCK,
 	SST26_CTL_UNLOCK,
@@ -994,6 +998,35 @@ static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
 	return false;
 }
 
+static int sst26_hardware_write_protection(struct spi_nor *nor, bool lock)
+{
+	int ret;
+	u8 cr_val;
+
+	cr_val = read_cr(nor);
+	if (cr_val < 0) {
+		dev_err(nor->dev, "fail to read config register value\n");
+		return cr_val;
+	}
+
+	cr_val &= ~CR_WPEN;
+
+	if (lock) {
+		// disallow further writes if WP# pin is not asserted.
+		cr_val |= CR_WPEN;
+		// force quad SPI disabled as otherwise WP# pin works as data line.
+		cr_val &= ~CR_QUAD_EN_SPAN;
+	}
+
+	ret = write_cr(nor, cr_val);
+	if (ret < 0) {
+		dev_err(nor->dev, "fail to configure WP# pin (hardware write protection)\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 /*
  * Lock, unlock or check lock status of the flash region of the flash (depending
  * on the lock_ctl value)
@@ -1101,6 +1134,14 @@ static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lo
 		return ret;
 	}
 
+	// enable hardware write protection iff any protection bit is set
+	for (i = 0; i < bpr_size; ++i)
+		if (bpr_buff[i] != 0)
+			break;
+	ret = sst26_hardware_write_protection(nor, i < bpr_size);
+	if (ret < 0)
+		return ret;
+
 	// ignore return value. even if write disable failed, the actual task
 	// (write block protecton register) was completed successfully.
 	write_disable(nor);
@@ -1339,7 +1380,9 @@ static int macronix_quad_enable(struct spi_nor *nor)
 }
 #endif
 
-#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
+#if defined(CONFIG_SPI_FLASH_SPANSION) || \
+	defined(CONFIG_SPI_FLASH_WINBOND) || \
+	defined(CONFIG_SPI_FLASH_SST)
 /*
  * Write status Register and configuration register with 2 bytes
  * The first byte will be written to the status register, while the
@@ -1369,6 +1412,23 @@ static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
 	return 0;
 }
 
+static int write_cr(struct spi_nor *nor, u8 value)
+{
+	int ret;
+	u8 sr_cr[2];
+
+	ret = read_sr(nor);
+	if (ret < 0)
+		return ret;
+
+	sr_cr[0] = ret;
+	sr_cr[1] = value;
+
+	return write_sr_cr(nor, sr_cr);
+}
+#endif
+
+#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
 /**
  * spansion_read_cr_quad_enable() - set QE bit in Configuration Register.
  * @nor:	pointer to a 'struct spi_nor'
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4a8e19ee4f..ee5a59cf0a 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -151,6 +151,7 @@
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPAN		BIT(1)	/* Spansion Quad I/O */
+#define CR_WPEN			BIT(7)	/* SST26* Write-Protection Pin (WP#) Enable */
 
 /* Status Register 2 bits. */
 #define SR2_QUAD_EN_BIT7	BIT(7)
-- 
2.29.2

[PATCH v1 5/7] sf protect (un)lock for SST26*: test BPR values

[Bernhard Kirchen]

after writing new block protection bits, read back the block protection
bit register and test whether the desired settings were accepted. fail
with error message otherwise.

this adjusts the behavior of "sf protect (un)lock" for SST26* chips to
be similar to the (un)locking behavior of Micron-compatible chips, where
the statur register is read back and checked after writing it with the
new block protection bit setting.

comparing the desired and current values fails if hardware write
protection is enabled (SRWD (Micron) or WPEN (SST26*) bit set), and the
WP# pin is not asserted, as the respective registers are write-protected
in that case.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 drivers/mtd/spi/spi-nor-core.c | 55 +++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 10c01cf20e..b3873aaf6e 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -998,6 +998,47 @@ static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
 	return false;
 }
 
+static int sst26_read_bpr(struct spi_nor *nor, u8 *buff, u32 *len)
+{
+	struct mtd_info *mtd = &nor->mtd;
+	u32 bpr_size;
+	int ret;
+
+	bpr_size = 2 + (mtd->size / SZ_64K / 8);
+
+	if (*len < bpr_size) {
+		dev_err(nor->dev, "block-protection register buffer too small\n");
+		return -EINVAL;
+	}
+
+	ret = nor->read_reg(nor, SPINOR_OP_READ_BPR, buff, bpr_size);
+	if (ret < 0) {
+		dev_err(nor->dev, "failed to read block-protection register\n");
+		return ret;
+	}
+
+	*len = bpr_size;
+	return 0;
+}
+
+static int sst26_check_bpr(struct spi_nor *nor, u8 *expected_buf, u32 expected_len)
+{
+	u8 actual_buf[SST26_MAX_BPR_REG_LEN] = {};
+	u32 actual_len = SST26_MAX_BPR_REG_LEN;
+	int ret;
+
+	ret = sst26_read_bpr(nor, actual_buf, &actual_len);
+	if (ret < 0)
+		return ret;
+
+	if (expected_len != actual_len || memcmp(expected_buf, actual_buf, actual_len)) {
+		dev_err(nor->dev, "device did not accept new block protection bits\n");
+		return -EACCES;
+	}
+
+	return 0;
+}
+
 static int sst26_hardware_write_protection(struct spi_nor *nor, bool lock)
 {
 	int ret;
@@ -1034,7 +1075,7 @@ static int sst26_hardware_write_protection(struct spi_nor *nor, bool lock)
 static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lock_ctl ctl)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	u32 i, bpr_ptr, rptr_64k, lptr_64k, bpr_size;
+	u32 i, bpr_ptr, rptr_64k, lptr_64k, bpr_size = SST26_MAX_BPR_REG_LEN;
 	bool lower_64k = false, upper_64k = false;
 	u8 bpr_buff[SST26_MAX_BPR_REG_LEN] = {};
 	int ret;
@@ -1057,13 +1098,9 @@ static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lo
 	    mtd->size != SZ_8M)
 		return -EINVAL;
 
-	bpr_size = 2 + (mtd->size / SZ_64K / 8);
-
-	ret = nor->read_reg(nor, SPINOR_OP_READ_BPR, bpr_buff, bpr_size);
-	if (ret < 0) {
-		dev_err(nor->dev, "fail to read block-protection register\n");
+	ret = sst26_read_bpr(nor, bpr_buff, &bpr_size);
+	if (ret < 0)
 		return ret;
-	}
 
 	rptr_64k = min_t(u32, ofs + len, mtd->size - SST26_BOUND_REG_SIZE);
 	lptr_64k = max_t(u32, ofs, SST26_BOUND_REG_SIZE);
@@ -1134,6 +1171,10 @@ static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lo
 		return ret;
 	}
 
+	ret = sst26_check_bpr(nor, bpr_buff, bpr_size);
+	if (ret < 0)
+		return ret;
+
 	// enable hardware write protection iff any protection bit is set
 	for (i = 0; i < bpr_size; ++i)
 		if (bpr_buff[i] != 0)
-- 
2.29.2

[PATCH v1 6/7] fix sst26_process_bpr check

[Bernhard Kirchen]

the sst26_process_bpr function is supposed to check whether a particular
protection bit is set (write-lock active). the caller always returns
with EACCES when sst26_process_bpr returns true. the previous
implementation (double negation) causes the caller to abort checking
protection bits when the first bit expected to be set is set.

in order to check whether a particular region is (completely)
write-protected, all bits enabling write-protection for all relevant
blocks (blocks within the region) must be set. checking for bits can be
aborted if a required bit is NOT set, i.e., if sst26_process_bpr returns
true. the test whether the region is locked then fails, indicated by
(positive) return value EACCES. checking for bits must continue while
the required bits are found to be set. only if all expected bits were
found to be set, return value 0 indicates that no error occurred and all
relevant bits are set.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 drivers/mtd/spi/spi-nor-core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index b3873aaf6e..0b48e068be 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -982,6 +982,9 @@ enum lock_ctl {
 	SST26_CTL_CHECK
 };
 
+/*
+ * returns false for "no error" and true for "expected protection bit is NOT set"
+ */
 static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
 {
 	switch (ctl) {
@@ -992,7 +995,7 @@ static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
 		cmd[bpr_size - (bit / 8) - 1] &= ~BIT(bit % 8);
 		break;
 	case SST26_CTL_CHECK:
-		return !!(cmd[bpr_size - (bit / 8) - 1] & BIT(bit % 8));
+		return !(cmd[bpr_size - (bit / 8) - 1] & BIT(bit % 8));
 	}
 
 	return false;
@@ -1071,6 +1074,11 @@ static int sst26_hardware_write_protection(struct spi_nor *nor, bool lock)
 /*
  * Lock, unlock or check lock status of the flash region of the flash (depending
  * on the lock_ctl value)
+ *
+ * if lock_ctl == SST26_CTL_CHECK, returns EACCES (positive value) if region is
+ * NOT locked, 0 if region is locked, and negative on errors.
+ *
+ * otherwise returns 0 on success, negative value on error.
  */
 static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lock_ctl ctl)
 {
@@ -1200,10 +1208,6 @@ static int sst26_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	return sst26_lock_ctl(nor, ofs, len, SST26_CTL_LOCK);
 }
 
-/*
- * Returns EACCES (positive value) if region is locked, 0 if region is unlocked,
- * and negative on errors.
- */
 static int sst26_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	/*
-- 
2.29.2

[PATCH v1 7/7] provide "sf protect check" command

[Bernhard Kirchen]

this change exposes checking the protection mechanism of SPI NOR flash
chips to the CLI. it expands what was already there to communicate not
only the protection state of a particular region, but also whether or
not the hardware write protection mechanism of the chip is enabled.

the new command works for Micron (and compatible) SPI NOR flash chips as
well as the SST26* series of SPI NOR flash chips.

the changes were tested on proprietary boards using a M25P16 and a
SST26VF016B.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 cmd/sf.c                       | 23 +++++++++++++++--
 drivers/mtd/spi/spi-nor-core.c | 45 +++++++++++++++++++++++++---------
 include/linux/mtd/spi-nor.h    | 17 ++++++++++++-
 include/spi_flash.h            |  8 ++++++
 4 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index ecd2918cbc..664442813d 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -355,6 +355,7 @@ static int do_spi_protect(int argc, char *const argv[])
 	int ret = 0;
 	loff_t start, len;
 	bool prot = false;
+	struct lock_info info;
 
 	if (argc != 4)
 		return -1;
@@ -369,6 +370,24 @@ static int do_spi_protect(int argc, char *const argv[])
 		return 1;
 	}
 
+	if (strcmp(argv[1], "check") == 0) {
+		info.ofs = start;
+		info.len = len;
+		ret = spi_flash_is_protected(flash, &info);
+		if (ret != 0) {
+			printf("ERROR: operation failed (%d)\n", ret);
+			return 1;
+		}
+
+		printf("region [0x%06x, 0x%06x] is %sfully write-protected\n",
+		       info.ofs, (info.ofs + info.len - 1),
+		       (info.is_locked ? "" : "NOT "));
+		printf("hardware write protection is %sactive\n",
+		       (info.hw_protection ? "" : "NOT "));
+
+		return 0;
+	}
+
 	if (strcmp(argv[1], "lock") == 0)
 		prot = true;
 	else if (strcmp(argv[1], "unlock") == 0)
@@ -612,8 +631,8 @@ U_BOOT_CMD(
 	"sf update addr offset|partition len    - erase and write 'len' bytes from memory\n"
 	"                                         at 'addr' to flash at 'offset'\n"
 	"                                         or to start of mtd 'partition'\n"
-	"sf protect lock|unlock sector len      - protect/unprotect 'len' bytes starting\n"
-	"                                         at address 'sector'\n"
+	"sf protect lock|unlock|check offs len  - (un)protect or check protection of 'len'\n"
+	"                                         bytes starting at address 'offs'\n"
 #ifdef CONFIG_CMD_SF_TEST
 	"sf test offset len                     - run a very basic destructive test\n"
 	"                                         ranging 'len' bytes from 'offset'\n"
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 0b48e068be..f77f22684d 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -869,18 +869,25 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
  * Check if a region of the flash is (completely) locked. See stm_lock() for
  * more info.
  *
- * Returns 1 if entire region is locked, 0 if any portion is unlocked, and
- * negative on errors.
+ * Updates struct lock_info in-place to reflect protection state.
  */
-static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
+static int stm_is_locked(struct spi_nor *nor, struct lock_info *info)
 {
-	int status;
+	int status, ret;
 
 	status = read_sr(nor);
 	if (status < 0)
 		return status;
 
-	return stm_is_locked_sr(nor, ofs, len, status);
+	info->hw_protection = ((status & SR_SRWD) > 0);
+
+	ret = stm_is_locked_sr(nor, info->ofs, info->len, status);
+	if (ret < 0)
+		return ret;
+
+	info->is_locked = (ret > 0);
+
+	return 0;
 }
 #endif /* CONFIG_SPI_FLASH_STMICRO */
 
@@ -1208,18 +1215,32 @@ static int sst26_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	return sst26_lock_ctl(nor, ofs, len, SST26_CTL_LOCK);
 }
 
-static int sst26_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
+static int sst26_is_locked(struct spi_nor *nor, struct lock_info *info)
 {
+	int ret;
+
 	/*
 	 * is_locked function is used for check before reading or erasing flash
-	 * region, so offset and length might be not 64k allighned, so adjust
-	 * them to be 64k allighned as sst26_lock_ctl works only with 64k
-	 * allighned regions.
+	 * region, so offset and length might be not 64k aligned, so adjust
+	 * them to be 64k aligned as sst26_lock_ctl works only with 64k
+	 * aligned regions.
 	 */
-	ofs -= ofs & (SZ_64K - 1);
-	len = len & (SZ_64K - 1) ? (len & ~(SZ_64K - 1)) + SZ_64K : len;
+	info->ofs -= info->ofs & (SZ_64K - 1);
+	info->len = info->len & (SZ_64K - 1) ? (info->len & ~(SZ_64K - 1)) + SZ_64K : info->len;
 
-	return sst26_lock_ctl(nor, ofs, len, SST26_CTL_CHECK);
+	ret = sst26_lock_ctl(nor, info->ofs, info->len, SST26_CTL_CHECK);
+	if (ret < 0)
+		return ret;
+
+	info->is_locked = (ret == 0);
+
+	ret = read_cr(nor);
+	if (ret < 0)
+		return ret;
+
+	info->hw_protection = ((ret & (CR_WPEN | CR_QUAD_EN_SPAN)) == CR_WPEN);
+
+	return 0;
 }
 
 static int sst_write_byteprogram(struct spi_nor *nor, loff_t to, size_t len,
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index ee5a59cf0a..bfd503b1b2 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -256,6 +256,21 @@ enum spi_nor_option_flags {
  */
 struct flash_info;
 
+/**
+ * struct lock_info - structure describing flash protection
+ * @ofs:            start address of region
+ * @len:            length of region
+ * @is_locked:      true if the region is write-protected (locked)
+ * @hw_protection:  true if the write-protection is backed by hardware, e.g.,
+ *                  the lock can only be removed by setting WP# pin high
+ */
+struct lock_info {
+	u32 ofs;
+	u32 len;
+	bool is_locked;
+	bool hw_protection;
+};
+
 /*
  * TODO: Remove, once all users of spi_flash interface are moved to MTD
  *
@@ -344,7 +359,7 @@ struct spi_nor {
 
 	int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
-	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
+	int (*flash_is_locked)(struct spi_nor *nor, struct lock_info *lock);
 	int (*quad_enable)(struct spi_nor *nor);
 
 	void *priv;
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 85cae32cc7..09a4fdad1c 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -179,4 +179,12 @@ static inline int spi_flash_protect(struct spi_flash *flash, u32 ofs, u32 len,
 		return flash->flash_unlock(flash, ofs, len);
 }
 
+static inline int spi_flash_is_protected(struct spi_flash *flash, struct lock_info *info)
+{
+	if (!flash->flash_is_locked)
+		return -EOPNOTSUPP;
+
+	return flash->flash_is_locked(flash, info);
+}
+
 #endif /* _SPI_FLASH_H_ */
-- 
2.29.2

[PATCH v1 1/7] command sf: help text format

[Simon Glass]

On Thu, 28 Jan 2021 at 09:19, Bernhard Kirchen <schlimmchen@gmail.com> wrote:
>
> properly indent the help text and use single quotes consistently to mark
> variable parameters.
>
> Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
> ---
>
>  cmd/sf.c | 48 ++++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v1 2/7] sf protect: warn about failed (un)lock operation

[Simon Glass]

On Thu, 28 Jan 2021 at 09:19, Bernhard Kirchen <schlimmchen@gmail.com> wrote:
>
> it is not guaranteed that there is a human readable message when the
> lock or unlock operation failed. make sure there is a message emitted
> by the "sf protect" implementation if the subcommand failed.
>
> Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
> ---
>
>  cmd/sf.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v1 7/7] provide "sf protect check" command

[Simon Glass]

Hi,

On Thu, 28 Jan 2021 at 09:19, Bernhard Kirchen <schlimmchen@gmail.com> wrote:
>
> this change exposes checking the protection mechanism of SPI NOR flash
> chips to the CLI. it expands what was already there to communicate not
> only the protection state of a particular region, but also whether or
> not the hardware write protection mechanism of the chip is enabled.
>
> the new command works for Micron (and compatible) SPI NOR flash chips as
> well as the SST26* series of SPI NOR flash chips.
>
> the changes were tested on proprietary boards using a M25P16 and a
> SST26VF016B.
>
> Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
> ---
>
>  cmd/sf.c                       | 23 +++++++++++++++--
>  drivers/mtd/spi/spi-nor-core.c | 45 +++++++++++++++++++++++++---------
>  include/linux/mtd/spi-nor.h    | 17 ++++++++++++-
>  include/spi_flash.h            |  8 ++++++
>  4 files changed, 78 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Can we get a sandbox test for this in test/dm/sf.c ?

Regards,
Simon

[PATCH v1 2/7] sf protect: warn about failed (un)lock operation

[Jagan Teki]

On Thu, Jan 28, 2021 at 9:49 PM Bernhard Kirchen <schlimmchen@gmail.com> wrote:
>
> it is not guaranteed that there is a human readable message when the
> lock or unlock operation failed. make sure there is a message emitted
> by the "sf protect" implementation if the subcommand failed.
>
> Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
> ---

The E-mail address of the patch author and Signed-off-by seems to be
different. please fix it in v2. Seems to be for all patches in the
series.

[PATCH v1 1/7] command sf: help text format

[Simon Glass]

On Thu, 28 Jan 2021 at 09:29, Bernhard Kirchen <schlimmchen@gmail.com> wrote:
>
> properly indent the help text and use single quotes consistently to mark
> variable parameters.
>
> Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
> ---
>
>  cmd/sf.c | 48 ++++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 26 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v1 1/7] command sf: help text format

[Bernhard Kirchen]

properly indent the help text and use single quotes consistently to mark
variable parameters.

Signed-off-by: Bernhard Kirchen <bernhard.kirchen@mbconnectline.com>
---

 cmd/sf.c | 48 ++++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index c0d6a8f8a0..a991ae0d03 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -564,7 +564,7 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	/* The remaining commands require a selected device */
 	if (!flash) {
-		puts("No SPI flash selected. Please run `sf probe'\n");
+		puts("No SPI flash selected. Please run 'sf probe'\n");
 		return 1;
 	}
 
@@ -590,31 +590,27 @@ usage:
 	return CMD_RET_USAGE;
 }
 
-#ifdef CONFIG_CMD_SF_TEST
-#define SF_TEST_HELP "\nsf test offset len		" \
-		"- run a very basic destructive test"
-#else
-#define SF_TEST_HELP
-#endif
-
 U_BOOT_CMD(
-	sf,	5,	1,	do_spi_flash,
+	sf, 5, 1, do_spi_flash,
 	"SPI flash sub-system",
-	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
-	"				  and chip select\n"
-	"sf read addr offset|partition len	- read `len' bytes starting at\n"
-	"				          `offset' or from start of mtd\n"
-	"					  `partition'to memory at `addr'\n"
-	"sf write addr offset|partition len	- write `len' bytes from memory\n"
-	"				          at `addr' to flash at `offset'\n"
-	"					  or to start of mtd `partition'\n"
-	"sf erase offset|partition [+]len	- erase `len' bytes from `offset'\n"
-	"					  or from start of mtd `partition'\n"
-	"					 `+len' round up `len' to block size\n"
-	"sf update addr offset|partition len	- erase and write `len' bytes from memory\n"
-	"					  at `addr' to flash at `offset'\n"
-	"					  or to start of mtd `partition'\n"
-	"sf protect lock/unlock sector len	- protect/unprotect 'len' bytes starting\n"
-	"					  at address 'sector'\n"
-	SF_TEST_HELP
+	   "probe [[bus:]cs] [hz] [mode]        - init flash device on given SPI bus\n"
+	"                                         and chip select\n"
+	"sf read addr offset|partition len      - read 'len' bytes starting at\n"
+	"                                         'offset' or from start of mtd\n"
+	"                                         'partition' to memory at 'addr'\n"
+	"sf write addr offset|partition len     - write 'len' bytes from memory\n"
+	"                                         at 'addr' to flash at 'offset'\n"
+	"                                         or to start of mtd 'partition'\n"
+	"sf erase offset|partition [+]len       - erase 'len' bytes from 'offset'\n"
+	"                                         or from start of mtd 'partition'.\n"
+	"                                         '+len' round up 'len' to block size\n"
+	"sf update addr offset|partition len    - erase and write 'len' bytes from memory\n"
+	"                                         at 'addr' to flash at 'offset'\n"
+	"                                         or to start of mtd 'partition'\n"
+	"sf protect lock|unlock sector len      - protect/unprotect 'len' bytes starting\n"
+	"                                         at address 'sector'\n"
+#ifdef CONFIG_CMD_SF_TEST
+	"sf test offset len                     - run a very basic destructive test\n"
+	"                                         ranging 'len' bytes from 'offset'\n"
+#endif
 );
-- 
2.29.2