Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 1/3] mtd: spi-nor: introduce SR_BP_SHIFT define
       [not found] <CGME20200113055910epcas1p4f97dfeb465b00d66649d6321cffc7b5a@epcas1p4.samsung.com>
@ 2020-01-13  5:59 ` Jungseung Lee
       [not found]   ` <CGME20200113055910epcas1p377b2618bea2ca860acac2b6f34e2b83e@epcas1p3.samsung.com>
       [not found]   ` <CGME20200113055910epcas1p384c04182e7c643163d659d42fafd01b3@epcas1p3.samsung.com>
  0 siblings, 2 replies; 19+ messages in thread
From: Jungseung Lee @ 2020-01-13  5:59 UTC (permalink / raw)
  To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee

The shift variable of SR_BP is conclusive because the first bit of SR_BP
is fixed on all known flashes.

Introduce SR_BP_SHIFT define and let them used by stm_* functions
to replace ffs operation to get shift value.

Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 11 +++++------
 include/linux/mtd/spi-nor.h   |  2 ++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index addb6319fcbb..e3da6a8654a8 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1767,7 +1767,6 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 	struct mtd_info *mtd = &nor->mtd;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
-	int shift = ffs(mask) - 1;
 	int pow;
 
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
@@ -1778,7 +1777,7 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 		*ofs = 0;
 		*len = 0;
 	} else {
-		pow = ((sr & mask) ^ mask) >> shift;
+		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
 		*len = mtd->size >> pow;
 		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
 			*ofs = 0;
@@ -1860,7 +1859,7 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	int ret, status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
-	u8 shift = ffs(mask) - 1, pow, val;
+	u8 pow, val;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
@@ -1909,7 +1908,7 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
 	 */
 	pow = ilog2(mtd->size) - ilog2(lock_len);
-	val = mask - (pow << shift);
+	val = mask - (pow << SR_BP_SHIFT);
 	if (val & ~mask)
 		return -EINVAL;
 	/* Don't "lock" with no region! */
@@ -1946,7 +1945,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	int ret, status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
-	u8 shift = ffs(mask) - 1, pow, val;
+	u8 pow, val;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
 	bool use_top;
@@ -1997,7 +1996,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
-		val = mask - (pow << shift);
+		val = mask - (pow << SR_BP_SHIFT);
 		/* Some power-of-two sizes are not supported */
 		if (val & ~mask)
 			return -EINVAL;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 7e32adce72f7..541c06d042e8 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -137,6 +137,8 @@
 
 #define SR1_QUAD_EN_BIT6	BIT(6)
 
+#define SR_BP_SHIFT		2
+
 /* Enhanced Volatile Configuration Register bits */
 #define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
 
-- 
2.17.1


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

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

* [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
       [not found]   ` <CGME20200113055910epcas1p377b2618bea2ca860acac2b6f34e2b83e@epcas1p3.samsung.com>
@ 2020-01-13  5:59     ` Jungseung Lee
  2020-01-14 10:49       ` Tudor.Ambarus
  2020-01-22 19:36       ` Michael Walle
  0 siblings, 2 replies; 19+ messages in thread
From: Jungseung Lee @ 2020-01-13  5:59 UTC (permalink / raw)
  To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee

Currently, we are supporting block protection only for
flash chips with 3 block protection bits in the SR register.
This patch enables block protection support for some flash with
4 block protection bits(bp0-3).

Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
v3 :
  Fix wrong ofs calculation on v2 patch
v2 :
  Add sample table portion about 4bit block protection on the comment
  Trivial coding style change

 drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-----
 include/linux/mtd/spi-nor.h   |   8 +++
 2 files changed, 119 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e3da6a8654a8..7e8af6c4fdfa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,6 +238,14 @@ struct flash_info {
 					 * status register. Must be used with
 					 * SPI_NOR_HAS_TB.
 					 */
+#define SPI_NOR_HAS_BP3		BIT(17)	/*
+					 * Flash SR has 4 bit fields (BP0-3)
+					 * for block protection.
+					 */
+#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
+					 * BP3 is bit 6 of status register.
+					 * Must be used with SPI_NOR_HAS_BP3.
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -1767,23 +1775,47 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 	struct mtd_info *mtd = &nor->mtd;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
-	int pow;
+	u8 bp;
+	int pow = 0;
 
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
 
-	if (!(sr & mask)) {
-		/* No protection */
-		*ofs = 0;
-		*len = 0;
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		u8 tmp;
+
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			tmp = sr & (mask | SR_BP3_BIT6);
+		else
+			tmp = sr & (mask | SR_BP3_BIT5);
+
+		if (tmp & SR_BP3_BIT6)
+			tmp = (tmp & ~BIT(6)) | BIT(5);
+
+		bp = tmp >> SR_BP_SHIFT;
+		if (!bp) {
+			*ofs = 0;
+			*len = 0;
+			return;
+		}
+		if (bp <= ilog2(nor->n_sectors))
+			pow = ilog2(nor->n_sectors) + 1 - bp;
 	} else {
-		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
-		*len = mtd->size >> pow;
-		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+		bp = (sr & mask) >> SR_BP_SHIFT;
+		if (!bp) {
 			*ofs = 0;
-		else
-			*ofs = mtd->size - *len;
+			*len = 0;
+			return;
+		}
+		pow = bp ^ (mask >> SR_BP_SHIFT);
 	}
+
+	*len = mtd->size >> pow;
+
+	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+		*ofs = 0;
+	else
+		*ofs = mtd->size - *len;
 }
 
 /*
@@ -1823,7 +1855,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 
 /*
  * Lock a region of the flash. Compatible with ST Micro and similar flash.
- * Supports the block protection bits BP{0,1,2} in the status register
+ * Supports the block protection bits BP{0,1,2,3} in the status register
  * (SR). Does not support these features found in newer SR bitfields:
  *   - SEC: sector/block protect - only handle SEC=0 (block protect)
  *   - CMP: complement protect - only support CMP=0 (range is not complemented)
@@ -1831,7 +1863,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
  * Support for the following is provided conditionally for some flash:
  *   - TB: top/bottom protect
  *
- * Sample table portion for 8MB flash (Winbond w25q64fw):
+ * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
  *
  *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
  *  --------------------------------------------------------------------------
@@ -1851,6 +1883,32 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
  *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower 1/4
  *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower 1/2
  *
+ * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-3):
+ *
+ *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
+ *  --------------------------------------------------------------------------
+ *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
+ *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper 1/1024
+ *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper 1/512
+ *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper 1/256
+ *   ...
+ *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper 1/4
+ *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper 1/2
+ *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
+ *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
+ *   ...
+ *  ------|-------|-------|-------|-------|---------------|-------------------
+ *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
+ *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower 1/1024
+ *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower 1/512
+ *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower 1/256
+ *   ...
+ *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower 1/4
+ *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower 1/2
+ *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
+ *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
+ *   ...
+ *
  * Returns negative on errors, 0 on success.
  */
 static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
@@ -1898,6 +1956,12 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
 
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			mask = mask | SR_BP3_BIT6;
+		else
+			mask = mask | SR_BP3_BIT5;
+	}
 	/*
 	 * Need smallest pow such that:
 	 *
@@ -1908,7 +1972,17 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
 	 */
 	pow = ilog2(mtd->size) - ilog2(lock_len);
-	val = mask - (pow << SR_BP_SHIFT);
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		val = ilog2(nor->n_sectors) + 1 - pow;
+		val = val << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
+	} else {
+		val = mask - (pow << SR_BP_SHIFT);
+	}
+
 	if (val & ~mask)
 		return -EINVAL;
 	/* Don't "lock" with no region! */
@@ -1983,6 +2057,13 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			mask = mask | SR_BP3_BIT6;
+		else
+			mask = mask | SR_BP3_BIT5;
+	}
 	/*
 	 * Need largest pow such that:
 	 *
@@ -1995,13 +2076,20 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	pow = ilog2(mtd->size) - order_base_2(lock_len);
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
+	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		val = ilog2(nor->n_sectors) + 1 - pow;
+		val = val << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
 	} else {
 		val = mask - (pow << SR_BP_SHIFT);
-		/* Some power-of-two sizes are not supported */
-		if (val & ~mask)
-			return -EINVAL;
 	}
 
+	/* Some power-of-two sizes are not supported */
+	if (val & ~mask)
+		return -EINVAL;
+
 	status_new = (status_old & ~mask & ~tb_mask) | val;
 
 	/* Don't protect status register if we're fully unlocked */
@@ -4736,6 +4824,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
 	/* Set SPI NOR sizes. */
 	params->size = (u64)info->sector_size * info->n_sectors;
 	params->page_size = info->page_size;
+	params->n_sectors = info->n_sectors;
 
 	if (!(info->flags & SPI_NOR_NO_FR)) {
 		/* Default to Fast Read for DT and non-DT platform devices. */
@@ -5192,6 +5281,11 @@ 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_HAS_BP3) {
+		nor->flags |= SNOR_F_HAS_SR_BP3;
+		if (info->flags & SPI_NOR_BP3_SR_BIT6)
+			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
+	}
 
 	if (info->flags & SPI_NOR_NO_ERASE)
 		mtd->flags |= MTD_NO_ERASE;
@@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	mtd->dev.parent = dev;
 	nor->page_size = params->page_size;
 	mtd->writebufsize = nor->page_size;
+	nor->n_sectors = params->n_sectors;
 
 	if (of_property_read_bool(np, "broken-flash-reset"))
 		nor->flags |= SNOR_F_BROKEN_RESET;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 541c06d042e8..92d550501daf 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -129,7 +129,9 @@
 #define SR_BP1			BIT(3)	/* Block protect 1 */
 #define SR_BP2			BIT(4)	/* Block protect 2 */
 #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
+#define SR_BP3_BIT5		BIT(5)	/* Block protect 3 */
 #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
+#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 /* Spansion/Cypress specific status bits */
 #define SR_E_ERR		BIT(5)
@@ -248,6 +250,8 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_16BIT_SR	= BIT(9),
 	SNOR_F_NO_READ_CR	= BIT(10),
 	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
+	SNOR_F_HAS_SR_BP3	= BIT(12),
+	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
 
 };
 
@@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
  *
  * @size:		the flash memory density in bytes.
  * @page_size:		the page size of the SPI NOR flash memory.
+ * @n_sectors:		number of sectors
  * @hwcaps:		describes the read and page program hardware
  *			capabilities.
  * @reads:		read capabilities ordered by priority: the higher index
@@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
 struct spi_nor_flash_parameter {
 	u64				size;
 	u32				page_size;
+	u16				n_sectors;
 
 	struct spi_nor_hwcaps		hwcaps;
 	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
@@ -573,6 +579,7 @@ struct flash_info;
  * @bouncebuf_size:	size of the bounce buffer
  * @info:		spi-nor part JDEC MFR id and other info
  * @page_size:		the page size of the SPI NOR
+ * @n_sector:		number of sectors
  * @addr_width:		number of address bytes
  * @erase_opcode:	the opcode for erasing a sector
  * @read_opcode:	the read opcode
@@ -599,6 +606,7 @@ struct spi_nor {
 	size_t			bouncebuf_size;
 	const struct flash_info	*info;
 	u32			page_size;
+	u16			n_sectors;
 	u8			addr_width;
 	u8			erase_opcode;
 	u8			read_opcode;
-- 
2.17.1


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

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

* [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips
       [not found]   ` <CGME20200113055910epcas1p384c04182e7c643163d659d42fafd01b3@epcas1p3.samsung.com>
@ 2020-01-13  5:59     ` Jungseung Lee
  2020-01-13 12:30       ` John Garry
  0 siblings, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-13  5:59 UTC (permalink / raw)
  To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee

Some Micron models are known to have lock/unlock support,
and that also support 4bit block protection (bp0-3).

This patch support lock/unlock feature on the flashes.

Tested on w25q512ax3. The Other is modified following the datasheet.

Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 7e8af6c4fdfa..97a027c38d66 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2583,12 +2583,17 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
 			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
 			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
-	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024,
+			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
+			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
 	{ "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024,
 			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
 			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
-	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
-			      USE_FSR | SPI_NOR_QUAD_READ) },
+	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024,
+			       SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
+			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
 	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
 	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
 	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
-- 
2.17.1


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

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

* Re: [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips
  2020-01-13  5:59     ` [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips Jungseung Lee
@ 2020-01-13 12:30       ` John Garry
  2020-01-13 12:40         ` Jungseung Lee
  2020-01-13 12:45         ` Jungseung Lee
  0 siblings, 2 replies; 19+ messages in thread
From: John Garry @ 2020-01-13 12:30 UTC (permalink / raw)
  To: Jungseung Lee, Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee
  Cc: chenxiang

On 13/01/2020 05:59, Jungseung Lee wrote:
> Some Micron models are known to have lock/unlock support,
> and that also support 4bit block protection (bp0-3).
> 
> This patch support lock/unlock feature on the flashes.
> 
> Tested on w25q512ax3. The Other is modified following the datasheet.
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> ---
>   drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 7e8af6c4fdfa..97a027c38d66 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2583,12 +2583,17 @@ static const struct flash_info spi_nor_ids[] = {
>   	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
>   			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
>   			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> -	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
> +	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024,
> +			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
>   	{ "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024,
>   			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
>   			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
> -			      USE_FSR | SPI_NOR_QUAD_READ) },
> +	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024,
> +			       SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
>   	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>   	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>   	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> 

Hi,

I'd like to test on a n25q128a11.

Seems I just need to add "SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | 		 
SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6" to
n25q128a11 INFO also, right?

Thanks,
John


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

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

* Re: [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips
  2020-01-13 12:30       ` John Garry
@ 2020-01-13 12:40         ` Jungseung Lee
  2020-01-13 12:45         ` Jungseung Lee
  1 sibling, 0 replies; 19+ messages in thread
From: Jungseung Lee @ 2020-01-13 12:40 UTC (permalink / raw)
  To: John Garry, Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee
  Cc: chenxiang

Hi, John

2020-01-13 (Mon), 12:30 +0000, John Garry:
> On 13/01/2020 05:59, Jungseung Lee wrote:
> > Some Micron models are known to have lock/unlock support,
> > and that also support 4bit block protection (bp0-3).
> > 
> > This patch support lock/unlock feature on the flashes.
> > 
> > Tested on w25q512ax3. The Other is modified following the
> > datasheet.
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> >   drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > nor/spi-nor.c
> > index 7e8af6c4fdfa..97a027c38d66 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2583,12 +2583,17 @@ static const struct flash_info
> > spi_nor_ids[] = {
> >   	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
> >   			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> >   			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
> > },
> > -	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
> > USE_FSR | SPI_NOR_QUAD_READ) },
> > +	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024,
> > +			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> > +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6)
> > },
> >   	{ "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024,
> >   			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> >   			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
> > },
> > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
> > -			      USE_FSR | SPI_NOR_QUAD_READ) },
> > +	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024,
> > +			       SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> > +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> > +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6)
> > },
> >   	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K |
> > USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> >   	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K |
> > USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> >   	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> > 
> 
> Hi,
> 
> I'd like to test on a n25q128a11.
> 
Thanks for your interest.

> Seems I just need to add "SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | 
> 	 
> SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6" to
> n25q128a11 INFO also, right?
> 
You are right.

> Thanks,
> John
> 
> 
Best Regards,
Jungseung Lee


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

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

* Re: [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips
  2020-01-13 12:30       ` John Garry
  2020-01-13 12:40         ` Jungseung Lee
@ 2020-01-13 12:45         ` Jungseung Lee
  2020-01-13 13:00           ` John Garry
  1 sibling, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-13 12:45 UTC (permalink / raw)
  To: John Garry, Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee
  Cc: chenxiang

Hi, John

2020-01-13 (Mon), 12:30 +0000, John Garry:
> On 13/01/2020 05:59, Jungseung Lee wrote:
> > Some Micron models are known to have lock/unlock support,
> > and that also support 4bit block protection (bp0-3).
> > 
> > This patch support lock/unlock feature on the flashes.
> > 
> > Tested on w25q512ax3. The Other is modified following the
> > datasheet.
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> >   drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > nor/spi-nor.c
> > index 7e8af6c4fdfa..97a027c38d66 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2583,12 +2583,17 @@ static const struct flash_info
> > spi_nor_ids[] = {
> >   	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
> >   			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> >   			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
> > },
> > -	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
> > USE_FSR | SPI_NOR_QUAD_READ) },
> > +	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024,
> > +			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> > +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6)
> > },
> >   	{ "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024,
> >   			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> >   			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
> > },
> > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
> > -			      USE_FSR | SPI_NOR_QUAD_READ) },
> > +	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024,
> > +			       SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> > +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> > +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6)
> > },
> >   	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K |
> > USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> >   	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K |
> > USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> >   	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> > 
> 
> Hi,
> 
> I'd like to test on a n25q128a11.
> 
Thanks for your interest to my patches.

> Seems I just need to add "SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | 
> 	 
> SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6" to
> n25q128a11 INFO also, right?
You're right.

> Thanks,
> John
> 
> 
Best Regards,
Jungseung Lee


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

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

* Re: [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips
  2020-01-13 12:45         ` Jungseung Lee
@ 2020-01-13 13:00           ` John Garry
  0 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2020-01-13 13:00 UTC (permalink / raw)
  To: Jungseung Lee, Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee
  Cc: chenxiang

On 13/01/2020 12:45, Jungseung Lee wrote:
> Hi, John
> 
> 2020-01-13 (Mon), 12:30 +0000, John Garry:
>> On 13/01/2020 05:59, Jungseung Lee wrote:
>>> Some Micron models are known to have lock/unlock support,
>>> and that also support 4bit block protection (bp0-3).
>>>
>>> This patch support lock/unlock feature on the flashes.
>>>
>>> Tested on w25q512ax3. The Other is modified following the
>>> datasheet.
>>>
>>> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>>> ---
>>>    drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
>>> nor/spi-nor.c
>>> index 7e8af6c4fdfa..97a027c38d66 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -2583,12 +2583,17 @@ static const struct flash_info
>>> spi_nor_ids[] = {
>>>    	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
>>>    			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
>>>    			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
>>> },
>>> -	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
>>> USE_FSR | SPI_NOR_QUAD_READ) },
>>> +	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024,
>>> +			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
>>> +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
>>> +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6)
>>> },
>>>    	{ "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024,
>>>    			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
>>>    			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES)
>>> },
>>> -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
>>> -			      USE_FSR | SPI_NOR_QUAD_READ) },
>>> +	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024,
>>> +			       SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>>> +			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
>>> +			       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6)
>>> },
>>>    	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K |
>>> USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>>    	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K |
>>> USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>>    	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
>>>
>>
>> Hi,
>>
>> I'd like to test on a n25q128a11.
>>
> Thanks for your interest to my patches.
> 
>> Seems I just need to add "SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
>> 	
>> SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6" to
>> n25q128a11 INFO also, right?
> You're right.
> 
>> Thanks,
>> John
>>
>>
> Best Regards,
> Jungseung Lee

OK, great. So from *limited* testing (which you can judge by my response 
time), this seems to work ok:

root@ubuntu:/home/john# sudo flash_lock -l /dev/mtd0
root@ubuntu:/home/john# sudo mtd_debug write /dev/mtd0 0x100000 4096 
dump4096
[  220.310538] spi-nor spi-PRP0001:00: Program operation failed.
[  220.316314] spi-nor spi-PRP0001:00: Attempted to modify a protected 
sector.
file_to_flash: write, size 0x1000, n 0x1000
write(): Input/output error
root@ubuntu:/home/john# sudo flash_lock -u /dev/mtd0
flash_lock: error!: could not unlock device: /dev/mtd0

             error 5 (Input/output error)
root@ubuntu:/home/john# sudo flash_lock -u /dev/mtd0
root@ubuntu:/home/john# sudo mtd_debug write /dev/mtd0 0x100000 4096 
dump4096
Copied 4096 bytes from dump4096 to address 0x00100000 in flash
root@ubuntu:/home/john#

This write to the bottom portion was previously passing (which was 
improper to do so). Note that the flash_lock -u error is a known issue.

I'll test further this/next week if I get a chance.

Thanks,
John



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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-13  5:59     ` [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
@ 2020-01-14 10:49       ` Tudor.Ambarus
  2020-01-17 15:06         ` Jungseung Lee
  2020-01-22 19:36       ` Michael Walle
  1 sibling, 1 reply; 19+ messages in thread
From: Tudor.Ambarus @ 2020-01-14 10:49 UTC (permalink / raw)
  To: js07.lee; +Cc: linux-mtd, vigneshr, js07.lee

Hi, Jungseung,

Thanks for working on this.

On Monday, January 13, 2020 7:59:06 AM EET Jungseung Lee wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Currently, we are supporting block protection only for
> flash chips with 3 block protection bits in the SR register.
> This patch enables block protection support for some flash with
> 4 block protection bits(bp0-3).

Some? Isn't this generic for all the flashes that support BP0-3?

> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> ---
> v3 :
>   Fix wrong ofs calculation on v2 patch
> v2 :
>   Add sample table portion about 4bit block protection on the comment
>   Trivial coding style change
> 
>  drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-----
>  include/linux/mtd/spi-nor.h   |   8 +++
>  2 files changed, 119 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e3da6a8654a8..7e8af6c4fdfa 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -238,6 +238,14 @@ struct flash_info {
>                                          * status register. Must be used
> with * SPI_NOR_HAS_TB.
>                                          */
> +#define SPI_NOR_HAS_BP3                BIT(17) /*
> +                                        * Flash SR has 4 bit fields (BP0-3)
> +                                        * for block protection.
> +                                        */
> +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
> +                                        * BP3 is bit 6 of status register.
> +                                        * Must be used with

Are we safe to replace SPI_NOR_TB_SR_BIT6 and SPI_NOR_BP3_SR_BIT6 with a 
SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a SPI_NOR_SR_BP3_BIT6_TB_BIT5, how 
is more convenient?

Cheers,
ta


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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-14 10:49       ` Tudor.Ambarus
@ 2020-01-17 15:06         ` Jungseung Lee
  2020-01-22 11:42           ` Jungseung Lee
  0 siblings, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-17 15:06 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: linux-mtd, vigneshr, Jungseung Lee

Hi, Tudor,

On Tue, Jan 14, 2020 at 7:49 PM <Tudor.Ambarus@microchip.com> wrote:
>
> Hi, Jungseung,
>
> Thanks for working on this.
>
> On Monday, January 13, 2020 7:59:06 AM EET Jungseung Lee wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Currently, we are supporting block protection only for
> > flash chips with 3 block protection bits in the SR register.
> > This patch enables block protection support for some flash with
> > 4 block protection bits(bp0-3).
>
> Some? Isn't this generic for all the flashes that support BP0-3?
>

This one would be a generic solution to support BP0-3 on Status Register.
From my study, this covers all the flashes listed on spi-nor.c that
have BP0-3 bit on SR.
It looks like I have to change this description.

Note that it is NOT for some flashes that have BP0-3 in another register.
As you know, just like SPI_NOR_HAS_TB did.

> >
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> > v3 :
> >   Fix wrong ofs calculation on v2 patch
> > v2 :
> >   Add sample table portion about 4bit block protection on the comment
> >   Trivial coding style change
> >
> >  drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-----
> >  include/linux/mtd/spi-nor.h   |   8 +++
> >  2 files changed, 119 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index e3da6a8654a8..7e8af6c4fdfa 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -238,6 +238,14 @@ struct flash_info {
> >                                          * status register. Must be used
> > with * SPI_NOR_HAS_TB.
> >                                          */
> > +#define SPI_NOR_HAS_BP3                BIT(17) /*
> > +                                        * Flash SR has 4 bit fields (BP0-3)
> > +                                        * for block protection.
> > +                                        */
> > +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
> > +                                        * BP3 is bit 6 of status register.
> > +                                        * Must be used with
>
> Are we safe to replace SPI_NOR_TB_SR_BIT6 and SPI_NOR_BP3_SR_BIT6 with a
> SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
> is more convenient?
>

Let's think about some flash in which BP0-3 exists in the status
register but TB exists in another register.
for example, mx25u12835f.
I haven't tested yet, but according to the datasheet, I think this
patch can support 4bit block protection for the flash.

In order to embrace the case, how about letting them as It is.
Is there any suggestion?

> Cheers,
> ta
>

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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-17 15:06         ` Jungseung Lee
@ 2020-01-22 11:42           ` Jungseung Lee
  2020-01-22 14:31             ` Tudor.Ambarus
  0 siblings, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-22 11:42 UTC (permalink / raw)
  To: Jungseung Lee, Tudor.Ambarus, js07.lee; +Cc: linux-mtd, vigneshr

Hi, Tudor,

2020-01-18 (Sat), 00:06 +0900, Jungseung Lee:
> Hi, Tudor,
> 
> On Tue, Jan 14, 2020 at 7:49 PM <Tudor.Ambarus@microchip.com> wrote:
> > 
> > Hi, Jungseung,
> > 
> > Thanks for working on this.
> > 
> > On Monday, January 13, 2020 7:59:06 AM EET Jungseung Lee wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the
> > > content is safe
> > > 
> > > Currently, we are supporting block protection only for
> > > flash chips with 3 block protection bits in the SR register.
> > > This patch enables block protection support for some flash with
> > > 4 block protection bits(bp0-3).
> > 
> > Some? Isn't this generic for all the flashes that support BP0-3?
> > 
> 
> This one would be a generic solution to support BP0-3 on Status
> Register.
> From my study, this covers all the flashes listed on spi-nor.c that
> have BP0-3 bit on SR.
> It looks like I have to change this description.
> 
> Note that it is NOT for some flashes that have BP0-3 in another
> register.
> As you know, just like SPI_NOR_HAS_TB did.
> 
> > > 
> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > ---
> > > v3 :
> > >   Fix wrong ofs calculation on v2 patch
> > > v2 :
> > >   Add sample table portion about 4bit block protection on the
> > > comment
> > >   Trivial coding style change
> > > 
> > >  drivers/mtd/spi-nor/spi-nor.c | 127
> > > +++++++++++++++++++++++++++++-----
> > >  include/linux/mtd/spi-nor.h   |   8 +++
> > >  2 files changed, 119 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > > nor/spi-nor.c
> > > index e3da6a8654a8..7e8af6c4fdfa 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -238,6 +238,14 @@ struct flash_info {
> > >                                          * status register. Must
> > > be used
> > > with * SPI_NOR_HAS_TB.
> > >                                          */
> > > +#define SPI_NOR_HAS_BP3                BIT(17) /*
> > > +                                        * Flash SR has 4 bit
> > > fields (BP0-3)
> > > +                                        * for block protection.
> > > +                                        */
> > > +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
> > > +                                        * BP3 is bit 6 of status
> > > register.
> > > +                                        * Must be used with
> > 
> > Are we safe to replace SPI_NOR_TB_SR_BIT6 and SPI_NOR_BP3_SR_BIT6
> > with a
> > SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a
> > SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
> > is more convenient?
> > 
> 
> Let's think about some flash in which BP0-3 exists in the status
> register but TB exists in another register.
> for example, mx25u12835f.
> I haven't tested yet, but according to the datasheet, I think this
> patch can support 4bit block protection for the flash.
> 
> In order to embrace the case, how about letting them as It is.
> Is there any suggestion?
> 

Hmm.. I again have summarized the case of using 4bit block protection.

* Not supporting TB on status register.

- BP3(BIT5)
  SPI_NOR_HAS_BP3

- BP3(BIT6)
  SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6

* Supporting TB on status register.

- TB(BIT6), BP3(BIT5)
  SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_HAS_BP3
  vs
  SPI_NOR_HAS_TB | SPI_NOR_HAS_BP3 | SPI_NOR_SR_TB_BIT6_BP3_BIT5
    
- TB(BIT5), BP3(BIT6)
  SPI_NOR_HAS_TB | SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6
  vs
  SPI_NOR_HAS_TB | SPI_NOR_HAS_BP3 | SPI_NOR_SR_TB_BIT5_BP3_BIT6  
    
SPI_NOR_BP3_SR_BIT6 and SPI_NOR_TB_SR_BIT6 must exist becuase it maybe
used. It would be ok to add some definitions(as you mentioned) to get
same value with SPI_NOR_BP3_SR_BIT6 or SPI_NOR_TB_SR_BIT6 for
readabilty.

But I think there is nothing looks so cool. Any suggestion?

> > Cheers,
> > ta
> > 
> 
> 
Thanks,
Jungseung Lee


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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-22 11:42           ` Jungseung Lee
@ 2020-01-22 14:31             ` Tudor.Ambarus
  2020-01-22 17:14               ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Tudor.Ambarus @ 2020-01-22 14:31 UTC (permalink / raw)
  To: js07.lee, michael, john.garry; +Cc: linux-mtd, vigneshr, js07.lee

Hi, Jungseung,

On Wednesday, January 22, 2020 1:42:00 PM EET Jungseung Lee wrote:

cut

> > > > +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
> > > > +                                        * BP3 is bit 6 of status
> > > > register.
> > > > +                                        * Must be used with
> > > 
> > > Are we safe to replace SPI_NOR_TB_SR_BIT6 and SPI_NOR_BP3_SR_BIT6
> > > with a
> > > SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a
> > > SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
> > > is more convenient?
> > 
> > Let's think about some flash in which BP0-3 exists in the status
> > register but TB exists in another register.
> > for example, mx25u12835f.
> > I haven't tested yet, but according to the datasheet, I think this
> > patch can support 4bit block protection for the flash.
> > 
> > In order to embrace the case, how about letting them as It is.
> > Is there any suggestion?

Ok, this info should go in the commit message, together with details about 
which flashes were tested.

I didn't know that the TB bit can be defined in the Configuration register. 
This means that your suggestion with dedicated macros for BP3 and TB is fine.

I looked a bit over your patches, they are in a pretty good shape. I saw 
something that can be improved on patch 2/3, but I didn't manage to finish the 
review. Your patches are the first on my TODO list, but now I'm a bit busy. I 
hope that I'll come with a complete review by the end of the next week. I'm 
going to do tests on few flashes too, to make sure that BP0-2 was not 
affected.

In the meantime, maybe Michael or John can review/test your patches, they 
showed interest in BP0-3 support.

Cheers,
ta


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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-22 14:31             ` Tudor.Ambarus
@ 2020-01-22 17:14               ` Michael Walle
  2020-01-23  3:59                 ` Jungseung Lee
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-01-22 17:14 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: linux-mtd, john.garry, js07.lee, vigneshr, js07.lee

Am 2020-01-22 15:31, schrieb Tudor.Ambarus@microchip.com:
> Hi, Jungseung,
> 
> On Wednesday, January 22, 2020 1:42:00 PM EET Jungseung Lee wrote:
> 
> cut
> 
>> > > > +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
>> > > > +                                        * BP3 is bit 6 of status
>> > > > register.
>> > > > +                                        * Must be used with
>> > >
>> > > Are we safe to replace SPI_NOR_TB_SR_BIT6 and SPI_NOR_BP3_SR_BIT6
>> > > with a
>> > > SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a
>> > > SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
>> > > is more convenient?
>> >
>> > Let's think about some flash in which BP0-3 exists in the status
>> > register but TB exists in another register.
>> > for example, mx25u12835f.

And like the mx25u3232f, but this bit is only OTP programmable! For now,
I'd only add support for reading the TB bit to for this kind of flashes,
to prevent accidentially program this bit. It would be up to the board
manufacturer how to actually set the bit.

Like having a TB_READ_ONLY flag.

Its also some kind of asymmetrical because you can only set it one way,
eg. you can OTP the flash to be TB=1. Then you can be sure that the 
flash
will always be TB=1. But OTOH you cannot be sure that a TB=0 flash will
always be a TB=0 flash, because you cannot lock that bit.

Any thoughts?

-michael

>> > I haven't tested yet, but according to the datasheet, I think this
>> > patch can support 4bit block protection for the flash.
>> >
>> > In order to embrace the case, how about letting them as It is.
>> > Is there any suggestion?
> 
> Ok, this info should go in the commit message, together with details 
> about
> which flashes were tested.
> 
> I didn't know that the TB bit can be defined in the Configuration 
> register.
> This means that your suggestion with dedicated macros for BP3 and TB is 
> fine.
> 
> I looked a bit over your patches, they are in a pretty good shape. I 
> saw
> something that can be improved on patch 2/3, but I didn't manage to 
> finish the
> review. Your patches are the first on my TODO list, but now I'm a bit 
> busy. I
> hope that I'll come with a complete review by the end of the next week. 
> I'm
> going to do tests on few flashes too, to make sure that BP0-2 was not
> affected.
> 
> In the meantime, maybe Michael or John can review/test your patches, 
> they
> showed interest in BP0-3 support.
> 
> Cheers,
> ta

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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-13  5:59     ` [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
  2020-01-14 10:49       ` Tudor.Ambarus
@ 2020-01-22 19:36       ` Michael Walle
  2020-01-23  6:22         ` Jungseung Lee
  1 sibling, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-01-22 19:36 UTC (permalink / raw)
  To: js07.lee; +Cc: Michael Walle, tudor.ambarus, linux-mtd, vigneshr, js07.lee

Hi,

> Currently, we are supporting block protection only for
> flash chips with 3 block protection bits in the SR register.
> This patch enables block protection support for some flash with
> 4 block protection bits(bp0-3).
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> ---
> v3 :
>   Fix wrong ofs calculation on v2 patch
> v2 :
>   Add sample table portion about 4bit block protection on the comment
>   Trivial coding style change
> 
>  drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-----
>  include/linux/mtd/spi-nor.h   |   8 +++
>  2 files changed, 119 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e3da6a8654a8..7e8af6c4fdfa 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -238,6 +238,14 @@ struct flash_info {
>  					 * status register. Must be used with
>  					 * SPI_NOR_HAS_TB.
>  					 */
> +#define SPI_NOR_HAS_BP3		BIT(17)	/*
> +					 * Flash SR has 4 bit fields (BP0-3)
> +					 * for block protection.
> +					 */
> +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
> +					 * BP3 is bit 6 of status register.
> +					 * Must be used with SPI_NOR_HAS_BP3.
> +					 */
>  
>  	/* Part specific fixup hooks. */
>  	const struct spi_nor_fixups *fixups;
> @@ -1767,23 +1775,47 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  	struct mtd_info *mtd = &nor->mtd;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>  	u8 tb_mask = SR_TB_BIT5;
> -	int pow;
> +	u8 bp;
> +	int pow = 0;
>  
>  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>  		tb_mask = SR_TB_BIT6;
>  
> -	if (!(sr & mask)) {
> -		/* No protection */
> -		*ofs = 0;
> -		*len = 0;
> +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> +		u8 tmp;
> +
> +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> +			tmp = sr & (mask | SR_BP3_BIT6);
> +		else
> +			tmp = sr & (mask | SR_BP3_BIT5);
> +
> +		if (tmp & SR_BP3_BIT6)
> +			tmp = (tmp & ~BIT(6)) | BIT(5);
> +
> +		bp = tmp >> SR_BP_SHIFT;
> +		if (!bp) {
> +			*ofs = 0;
> +			*len = 0;
> +			return;
> +		}
> +		if (bp <= ilog2(nor->n_sectors))
> +			pow = ilog2(nor->n_sectors) + 1 - bp;
>  	} else {
> -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> -		*len = mtd->size >> pow;
> -		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> +		bp = (sr & mask) >> SR_BP_SHIFT;
> +		if (!bp) {
>  			*ofs = 0;
> -		else
> -			*ofs = mtd->size - *len;
> +			*len = 0;
> +			return;
> +		}
> +		pow = bp ^ (mask >> SR_BP_SHIFT);
>  	}
> +
> +	*len = mtd->size >> pow;
> +
> +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> +		*ofs = 0;
> +	else
> +		*ofs = mtd->size - *len;
>  }
>  
>  /*
> @@ -1823,7 +1855,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>  
>  /*
>   * Lock a region of the flash. Compatible with ST Micro and similar flash.
> - * Supports the block protection bits BP{0,1,2} in the status register
> + * Supports the block protection bits BP{0,1,2,3} in the status register
>   * (SR). Does not support these features found in newer SR bitfields:
>   *   - SEC: sector/block protect - only handle SEC=0 (block protect)
>   *   - CMP: complement protect - only support CMP=0 (range is not complemented)
> @@ -1831,7 +1863,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>   * Support for the following is provided conditionally for some flash:
>   *   - TB: top/bottom protect
>   *
> - * Sample table portion for 8MB flash (Winbond w25q64fw):
> + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
>   *
>   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
>   *  --------------------------------------------------------------------------
> @@ -1851,6 +1883,32 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>   *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower 1/4
>   *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower 1/2
>   *
> + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-3):
> + *
> + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
> + *  --------------------------------------------------------------------------
> + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
> + *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper 1/1024
> + *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper 1/512
> + *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper 1/256
> + *   ...
> + *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper 1/4
> + *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper 1/2
> + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> + *   ...
> + *  ------|-------|-------|-------|-------|---------------|-------------------
> + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
> + *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower 1/1024
> + *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower 1/512
> + *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower 1/256
> + *   ...
> + *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower 1/4
> + *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower 1/2
> + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> + *   ...
> + *
>   * Returns negative on errors, 0 on success.
>   */
>  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> @@ -1898,6 +1956,12 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>  		tb_mask = SR_TB_BIT6;
>  
> +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> +			mask = mask | SR_BP3_BIT6;
> +		else
> +			mask = mask | SR_BP3_BIT5;
> +	}
>  	/*
>  	 * Need smallest pow such that:
>  	 *
> @@ -1908,7 +1972,17 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
>  	 */
>  	pow = ilog2(mtd->size) - ilog2(lock_len);
> -	val = mask - (pow << SR_BP_SHIFT);
> +
> +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> +		val = ilog2(nor->n_sectors) + 1 - pow;

Why do you use a new calculation here? As far as I can see, the method is
the same except that is has one bit more. That also raises the question why
n_sectors is now needed?

Can't we just initialize the mask with

mask = SR_BP2 | SR_BP1 | SR_BP0;
if (nor->flags & SNOR_F_HAS_SR_BP3)
    mask |= SR_BP3_BIT5;

do the calculation and checks and then move the SR_BP3_BIT5 to SR_BP3_BIT6
if SNOR_F_HAS_SR_BP3_BIT6 is set.

> +		val = val << SR_BP_SHIFT;
> +
> +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> +			val = (val & ~BIT(5)) | BIT(6);
> +	} else {
> +		val = mask - (pow << SR_BP_SHIFT);
> +	}
> +
>  	if (val & ~mask)
>  		return -EINVAL;
>  	/* Don't "lock" with no region! */
> @@ -1983,6 +2057,13 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  
>  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>  		tb_mask = SR_TB_BIT6;
> +
> +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> +			mask = mask | SR_BP3_BIT6;
> +		else
> +			mask = mask | SR_BP3_BIT5;
> +	}
>  	/*
>  	 * Need largest pow such that:
>  	 *
> @@ -1995,13 +2076,20 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	pow = ilog2(mtd->size) - order_base_2(lock_len);
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
> +	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
> +		val = ilog2(nor->n_sectors) + 1 - pow;
> +		val = val << SR_BP_SHIFT;
> +
> +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> +			val = (val & ~BIT(5)) | BIT(6);
>  	} else {
>  		val = mask - (pow << SR_BP_SHIFT);
> -		/* Some power-of-two sizes are not supported */
> -		if (val & ~mask)
> -			return -EINVAL;
>  	}
>  
> +	/* Some power-of-two sizes are not supported */
> +	if (val & ~mask)
> +		return -EINVAL;
> +
>  	status_new = (status_old & ~mask & ~tb_mask) | val;
>  
>  	/* Don't protect status register if we're fully unlocked */
> @@ -4736,6 +4824,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>  	/* Set SPI NOR sizes. */
>  	params->size = (u64)info->sector_size * info->n_sectors;
>  	params->page_size = info->page_size;
> +	params->n_sectors = info->n_sectors;
>  
>  	if (!(info->flags & SPI_NOR_NO_FR)) {
>  		/* Default to Fast Read for DT and non-DT platform devices. */
> @@ -5192,6 +5281,11 @@ 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_HAS_BP3) {
> +		nor->flags |= SNOR_F_HAS_SR_BP3;
> +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> +	}
>  
>  	if (info->flags & SPI_NOR_NO_ERASE)
>  		mtd->flags |= MTD_NO_ERASE;
> @@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	mtd->dev.parent = dev;
>  	nor->page_size = params->page_size;
>  	mtd->writebufsize = nor->page_size;
> +	nor->n_sectors = params->n_sectors;
>  
>  	if (of_property_read_bool(np, "broken-flash-reset"))
>  		nor->flags |= SNOR_F_BROKEN_RESET;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 541c06d042e8..92d550501daf 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -129,7 +129,9 @@
>  #define SR_BP1			BIT(3)	/* Block protect 1 */
>  #define SR_BP2			BIT(4)	/* Block protect 2 */
>  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
> +#define SR_BP3_BIT5		BIT(5)	/* Block protect 3 */

maybe just name it SR_BP3? would also be more consistent with the proposal
above.

>  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
> +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
>  #define SR_SRWD			BIT(7)	/* SR write protect */
>  /* Spansion/Cypress specific status bits */
>  #define SR_E_ERR		BIT(5)
> @@ -248,6 +250,8 @@ enum spi_nor_option_flags {
>  	SNOR_F_HAS_16BIT_SR	= BIT(9),
>  	SNOR_F_NO_READ_CR	= BIT(10),
>  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> +	SNOR_F_HAS_SR_BP3	= BIT(12),
> +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
>  
>  };
>  
> @@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
>   *
>   * @size:		the flash memory density in bytes.
>   * @page_size:		the page size of the SPI NOR flash memory.
> + * @n_sectors:		number of sectors
>   * @hwcaps:		describes the read and page program hardware
>   *			capabilities.
>   * @reads:		read capabilities ordered by priority: the higher index
> @@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
>  struct spi_nor_flash_parameter {
>  	u64				size;
>  	u32				page_size;
> +	u16				n_sectors;
>  
>  	struct spi_nor_hwcaps		hwcaps;
>  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
> @@ -573,6 +579,7 @@ struct flash_info;
>   * @bouncebuf_size:	size of the bounce buffer
>   * @info:		spi-nor part JDEC MFR id and other info
>   * @page_size:		the page size of the SPI NOR
> + * @n_sector:		number of sectors
>   * @addr_width:		number of address bytes
>   * @erase_opcode:	the opcode for erasing a sector
>   * @read_opcode:	the read opcode
> @@ -599,6 +606,7 @@ struct spi_nor {
>  	size_t			bouncebuf_size;
>  	const struct flash_info	*info;
>  	u32			page_size;
> +	u16			n_sectors;
>  	u8			addr_width;
>  	u8			erase_opcode;
>  	u8			read_opcode;
> -- 
> 2.17.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-22 17:14               ` Michael Walle
@ 2020-01-23  3:59                 ` Jungseung Lee
  2020-01-23  8:15                   ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-23  3:59 UTC (permalink / raw)
  To: Michael Walle, Tudor.Ambarus; +Cc: john.garry, linux-mtd, vigneshr, js07.lee

Hi, Michael

2020-01-22 (Wed), 18:14 +0100, Michael Walle:
> Am 2020-01-22 15:31, schrieb Tudor.Ambarus@microchip.com:
> > Hi, Jungseung,
> > 
> > On Wednesday, January 22, 2020 1:42:00 PM EET Jungseung Lee wrote:
> > 
> > cut
> > 
> > > > > > +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
> > > > > > +                                        * BP3 is bit 6 of
> > > > > > status
> > > > > > register.
> > > > > > +                                        * Must be used
> > > > > > with
> > > > > 
> > > > > Are we safe to replace SPI_NOR_TB_SR_BIT6 and
> > > > > SPI_NOR_BP3_SR_BIT6
> > > > > with a
> > > > > SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a
> > > > > SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
> > > > > is more convenient?
> > > > 
> > > > Let's think about some flash in which BP0-3 exists in the
> > > > status
> > > > register but TB exists in another register.
> > > > for example, mx25u12835f.
> 
> And like the mx25u3232f, but this bit is only OTP programmable! For
> now,
> I'd only add support for reading the TB bit to for this kind of
> flashes,
> to prevent accidentially program this bit. It would be up to the
> board
> manufacturer how to actually set the bit.
> 
> Like having a TB_READ_ONLY flag.
> 
> Its also some kind of asymmetrical because you can only set it one
> way,
> eg. you can OTP the flash to be TB=1. Then you can be sure that the 
> flash
> will always be TB=1. But OTOH you cannot be sure that a TB=0 flash
> will
> always be a TB=0 flash, because you cannot lock that bit.
> 
> Any thoughts?
> 

Actually, I didn't get the chance to take a look at some flash that has
TB bit in configuration register. Currently mainline kernel just care
flashes that has TB bit in SR and SPI_NOR_HAS_TB can be set only such
flashes(as you could see in comment). It seems neccessary to add
another functions and flag for mx25u3232f case.
Is there any flash that has OTP programmable TB bit in SR?

Tudor and me were saying that there is some flash that has not TB bit
in *SR* even if it has BP3 bit in SR. So it seems unnecessary to make
correlated flag like SPI_NOR_SR_TB_BIT6_BP3_BIT5 for convenience.

> -michael
> 
> > > > I haven't tested yet, but according to the datasheet, I think
> > > > this
> > > > patch can support 4bit block protection for the flash.
> > > > 
> > > > In order to embrace the case, how about letting them as It is.
> > > > Is there any suggestion?
> > 
> > Ok, this info should go in the commit message, together with
> > details 
> > about
> > which flashes were tested.
> > 
> > I didn't know that the TB bit can be defined in the Configuration 
> > register.
> > This means that your suggestion with dedicated macros for BP3 and
> > TB is 
> > fine.
> > 
> > I looked a bit over your patches, they are in a pretty good shape.
> > I 
> > saw
> > something that can be improved on patch 2/3, but I didn't manage
> > to 
> > finish the
> > review. Your patches are the first on my TODO list, but now I'm a
> > bit 
> > busy. I
> > hope that I'll come with a complete review by the end of the next
> > week. 
> > I'm
> > going to do tests on few flashes too, to make sure that BP0-2 was
> > not
> > affected.
> > 
> > In the meantime, maybe Michael or John can review/test your
> > patches, 
> > they
> > showed interest in BP0-3 support.
> > 
> > Cheers,
> > ta
> 
> 
Thanks,
Jungseung Lee


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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-22 19:36       ` Michael Walle
@ 2020-01-23  6:22         ` Jungseung Lee
  2020-01-23  8:10           ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-23  6:22 UTC (permalink / raw)
  To: Michael Walle, js07.lee; +Cc: tudor.ambarus, linux-mtd, vigneshr, js07.lee

Hi, Michael

2020-01-22 (Wed), 20:36 +0100, Michael Walle:
> Hi,
> 
> > Currently, we are supporting block protection only for
> > flash chips with 3 block protection bits in the SR register.
> > This patch enables block protection support for some flash with
> > 4 block protection bits(bp0-3).
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> > v3 :
> >   Fix wrong ofs calculation on v2 patch
> > v2 :
> >   Add sample table portion about 4bit block protection on the
> > comment
> >   Trivial coding style change
> > 
> >  drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-
> > ----
> >  include/linux/mtd/spi-nor.h   |   8 +++
> >  2 files changed, 119 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > nor/spi-nor.c
> > index e3da6a8654a8..7e8af6c4fdfa 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -238,6 +238,14 @@ struct flash_info {
> >  					 * status register. Must be
> > used with
> >  					 * SPI_NOR_HAS_TB.
> >  					 */
> > +#define SPI_NOR_HAS_BP3		BIT(17)	/*
> > +					 * Flash SR has 4 bit fields
> > (BP0-3)
> > +					 * for block protection.
> > +					 */
> > +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
> > +					 * BP3 is bit 6 of status
> > register.
> > +					 * Must be used with
> > SPI_NOR_HAS_BP3.
> > +					 */
> >  
> >  	/* Part specific fixup hooks. */
> >  	const struct spi_nor_fixups *fixups;
> > @@ -1767,23 +1775,47 @@ static void stm_get_locked_range(struct
> > spi_nor *nor, u8 sr, loff_t *ofs,
> >  	struct mtd_info *mtd = &nor->mtd;
> >  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> >  	u8 tb_mask = SR_TB_BIT5;
> > -	int pow;
> > +	u8 bp;
> > +	int pow = 0;
> >  
> >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >  		tb_mask = SR_TB_BIT6;
> >  
> > -	if (!(sr & mask)) {
> > -		/* No protection */
> > -		*ofs = 0;
> > -		*len = 0;
> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > +		u8 tmp;
> > +
> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > +			tmp = sr & (mask | SR_BP3_BIT6);
> > +		else
> > +			tmp = sr & (mask | SR_BP3_BIT5);
> > +
> > +		if (tmp & SR_BP3_BIT6)
> > +			tmp = (tmp & ~BIT(6)) | BIT(5);
> > +
> > +		bp = tmp >> SR_BP_SHIFT;
> > +		if (!bp) {
> > +			*ofs = 0;
> > +			*len = 0;
> > +			return;
> > +		}
> > +		if (bp <= ilog2(nor->n_sectors))
> > +			pow = ilog2(nor->n_sectors) + 1 - bp;
> >  	} else {
> > -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> > -		*len = mtd->size >> pow;
> > -		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> > +		bp = (sr & mask) >> SR_BP_SHIFT;
> > +		if (!bp) {
> >  			*ofs = 0;
> > -		else
> > -			*ofs = mtd->size - *len;
> > +			*len = 0;
> > +			return;
> > +		}
> > +		pow = bp ^ (mask >> SR_BP_SHIFT);
> >  	}
> > +
> > +	*len = mtd->size >> pow;
> > +
> > +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> > +		*ofs = 0;
> > +	else
> > +		*ofs = mtd->size - *len;
> >  }
> >  
> >  /*
> > @@ -1823,7 +1855,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> >  
> >  /*
> >   * Lock a region of the flash. Compatible with ST Micro and
> > similar flash.
> > - * Supports the block protection bits BP{0,1,2} in the status
> > register
> > + * Supports the block protection bits BP{0,1,2,3} in the status
> > register
> >   * (SR). Does not support these features found in newer SR
> > bitfields:
> >   *   - SEC: sector/block protect - only handle SEC=0 (block
> > protect)
> >   *   - CMP: complement protect - only support CMP=0 (range is not
> > complemented)
> > @@ -1831,7 +1863,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> >   * Support for the following is provided conditionally for some
> > flash:
> >   *   - TB: top/bottom protect
> >   *
> > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
> >   *
> >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > Protected Portion
> >   *  ------------------------------------------------------------
> > --------------
> > @@ -1851,6 +1883,32 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> >   *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower
> > 1/4
> >   *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower
> > 1/2
> >   *
> > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
> > 3):
> > + *
> > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > Protected Portion
> > + *  ------------------------------------------------------------
> > --------------
> > + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
> > + *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper
> > 1/1024
> > + *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper
> > 1/512
> > + *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper
> > 1/256
> > + *   ...
> > + *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper
> > 1/4
> > + *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper
> > 1/2
> > + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> > + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> > + *   ...
> > + *  ------|-------|-------|-------|-------|---------------|-------
> > ------------
> > + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
> > + *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower
> > 1/1024
> > + *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower
> > 1/512
> > + *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower
> > 1/256
> > + *   ...
> > + *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower
> > 1/4
> > + *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower
> > 1/2
> > + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> > + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> > + *   ...
> > + *
> >   * Returns negative on errors, 0 on success.
> >   */
> >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > @@ -1898,6 +1956,12 @@ static int stm_lock(struct spi_nor *nor,
> > loff_t ofs, uint64_t len)
> >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >  		tb_mask = SR_TB_BIT6;
> >  
> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > +			mask = mask | SR_BP3_BIT6;
> > +		else
> > +			mask = mask | SR_BP3_BIT5;
> > +	}
> >  	/*
> >  	 * Need smallest pow such that:
> >  	 *
> > @@ -1908,7 +1972,17 @@ static int stm_lock(struct spi_nor *nor,
> > loff_t ofs, uint64_t len)
> >  	 *   pow = ceil(log2(size / len)) = log2(size) -
> > floor(log2(len))
> >  	 */
> >  	pow = ilog2(mtd->size) - ilog2(lock_len);
> > -	val = mask - (pow << SR_BP_SHIFT);
> > +
> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > +		val = ilog2(nor->n_sectors) + 1 - pow;
> 
> Why do you use a new calculation here? As far as I can see, the
> method is
> the same except that is has one bit more. That also raises the
> question why
> n_sectors is now needed?
> 
> Can't we just initialize the mask with
> 
> mask = SR_BP2 | SR_BP1 | SR_BP0;
> if (nor->flags & SNOR_F_HAS_SR_BP3)
>     mask |= SR_BP3_BIT5;
> 
> do the calculation and checks and then move the SR_BP3_BIT5 to
> SR_BP3_BIT6
> if SNOR_F_HAS_SR_BP3_BIT6 is set.
> 

For most of flashes that supporting BP0-2, the smallest protected
portion is fixed as 1/64 and it can be properly handled by existing
calculation. (Actually it's not fully generic, see flashes like
w25q40bw or m25p80. Of course, it doesn't have SPI_NOR_HAS_LOCK flag
even though it has BP0-2 bit in SR)

We need new calculation method for 4bit block protection and for making
it more generic, I choose n_sectors.

On all the flashes I checked, n_sectors is proper value for getting
block protected portion.

		density	portion	n_sectors
W25M512JV	64MB	1/512	512
N25Q128A	16MB	1/256	256	
N25Q512A	64MB	1/1024	1024
MT25QL02GCBB	256MB	1/4096	4096

> > +		val = val << SR_BP_SHIFT;
> > +
> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > +			val = (val & ~BIT(5)) | BIT(6);
> > +	} else {
> > +		val = mask - (pow << SR_BP_SHIFT);
> > +	}
> > +
> >  	if (val & ~mask)
> >  		return -EINVAL;
> >  	/* Don't "lock" with no region! */
> > @@ -1983,6 +2057,13 @@ static int stm_unlock(struct spi_nor *nor,
> > loff_t ofs, uint64_t len)
> >  
> >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >  		tb_mask = SR_TB_BIT6;
> > +
> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > +			mask = mask | SR_BP3_BIT6;
> > +		else
> > +			mask = mask | SR_BP3_BIT5;
> > +	}
> >  	/*
> >  	 * Need largest pow such that:
> >  	 *
> > @@ -1995,13 +2076,20 @@ static int stm_unlock(struct spi_nor *nor,
> > loff_t ofs, uint64_t len)
> >  	pow = ilog2(mtd->size) - order_base_2(lock_len);
> >  	if (lock_len == 0) {
> >  		val = 0; /* fully unlocked */
> > +	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > +		val = ilog2(nor->n_sectors) + 1 - pow;
> > +		val = val << SR_BP_SHIFT;
> > +
> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > +			val = (val & ~BIT(5)) | BIT(6);
> >  	} else {
> >  		val = mask - (pow << SR_BP_SHIFT);
> > -		/* Some power-of-two sizes are not supported */
> > -		if (val & ~mask)
> > -			return -EINVAL;
> >  	}
> >  
> > +	/* Some power-of-two sizes are not supported */
> > +	if (val & ~mask)
> > +		return -EINVAL;
> > +
> >  	status_new = (status_old & ~mask & ~tb_mask) | val;
> >  
> >  	/* Don't protect status register if we're fully unlocked */
> > @@ -4736,6 +4824,7 @@ static void spi_nor_info_init_params(struct
> > spi_nor *nor)
> >  	/* Set SPI NOR sizes. */
> >  	params->size = (u64)info->sector_size * info->n_sectors;
> >  	params->page_size = info->page_size;
> > +	params->n_sectors = info->n_sectors;
> >  
> >  	if (!(info->flags & SPI_NOR_NO_FR)) {
> >  		/* Default to Fast Read for DT and non-DT platform
> > devices. */
> > @@ -5192,6 +5281,11 @@ 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_HAS_BP3) {
> > +		nor->flags |= SNOR_F_HAS_SR_BP3;
> > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > +	}
> >  
> >  	if (info->flags & SPI_NOR_NO_ERASE)
> >  		mtd->flags |= MTD_NO_ERASE;
> > @@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor *nor, const
> > char *name,
> >  	mtd->dev.parent = dev;
> >  	nor->page_size = params->page_size;
> >  	mtd->writebufsize = nor->page_size;
> > +	nor->n_sectors = params->n_sectors;
> >  
> >  	if (of_property_read_bool(np, "broken-flash-reset"))
> >  		nor->flags |= SNOR_F_BROKEN_RESET;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> > nor.h
> > index 541c06d042e8..92d550501daf 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -129,7 +129,9 @@
> >  #define SR_BP1			BIT(3)	/* Block protect 1
> > */
> >  #define SR_BP2			BIT(4)	/* Block protect 2
> > */
> >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
> > +#define SR_BP3_BIT5		BIT(5)	/* Block protect 3
> > */
> 
> maybe just name it SR_BP3? would also be more consistent with the
> proposal
> above.
> 
> >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
> > +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3
> > */
> >  #define SR_SRWD			BIT(7)	/* SR write protect
> > */
> >  /* Spansion/Cypress specific status bits */
> >  #define SR_E_ERR		BIT(5)
> > @@ -248,6 +250,8 @@ enum spi_nor_option_flags {
> >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
> >  	SNOR_F_NO_READ_CR	= BIT(10),
> >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> > +	SNOR_F_HAS_SR_BP3	= BIT(12),
> > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> >  
> >  };
> >  
> > @@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
> >   *
> >   * @size:		the flash memory density in bytes.
> >   * @page_size:		the page size of the SPI NOR flash
> > memory.
> > + * @n_sectors:		number of sectors
> >   * @hwcaps:		describes the read and page program
> > hardware
> >   *			capabilities.
> >   * @reads:		read capabilities ordered by priority: the
> > higher index
> > @@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
> >  struct spi_nor_flash_parameter {
> >  	u64				size;
> >  	u32				page_size;
> > +	u16				n_sectors;
> >  
> >  	struct spi_nor_hwcaps		hwcaps;
> >  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
> > @@ -573,6 +579,7 @@ struct flash_info;
> >   * @bouncebuf_size:	size of the bounce buffer
> >   * @info:		spi-nor part JDEC MFR id and other info
> >   * @page_size:		the page size of the SPI NOR
> > + * @n_sector:		number of sectors
> >   * @addr_width:		number of address bytes
> >   * @erase_opcode:	the opcode for erasing a sector
> >   * @read_opcode:	the read opcode
> > @@ -599,6 +606,7 @@ struct spi_nor {
> >  	size_t			bouncebuf_size;
> >  	const struct flash_info	*info;
> >  	u32			page_size;
> > +	u16			n_sectors;
> >  	u8			addr_width;
> >  	u8			erase_opcode;
> >  	u8			read_opcode;
> > -- 
> > 2.17.1
> > 
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > 
https://protect2.fireeye.com/url?k=06b6dd5d-5b7d5a63-06b75612-0cc47a31309a-83164929001f7741&u=http://lists.infradead.org/mailman/listinfo/linux-mtd/
> > 
> 
> 


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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-23  6:22         ` Jungseung Lee
@ 2020-01-23  8:10           ` Michael Walle
  2020-01-23  8:53             ` Jungseung Lee
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-01-23  8:10 UTC (permalink / raw)
  To: Jungseung Lee; +Cc: tudor.ambarus, linux-mtd, vigneshr, js07.lee

Hi Jungseung,

Am 2020-01-23 07:22, schrieb Jungseung Lee:
> Hi, Michael
> 
> 2020-01-22 (Wed), 20:36 +0100, Michael Walle:
>> Hi,
>> 
>> > Currently, we are supporting block protection only for
>> > flash chips with 3 block protection bits in the SR register.
>> > This patch enables block protection support for some flash with
>> > 4 block protection bits(bp0-3).
>> >
>> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>> > ---
>> > v3 :
>> >   Fix wrong ofs calculation on v2 patch
>> > v2 :
>> >   Add sample table portion about 4bit block protection on the
>> > comment
>> >   Trivial coding style change
>> >
>> >  drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-
>> > ----
>> >  include/linux/mtd/spi-nor.h   |   8 +++
>> >  2 files changed, 119 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
>> > nor/spi-nor.c
>> > index e3da6a8654a8..7e8af6c4fdfa 100644
>> > --- a/drivers/mtd/spi-nor/spi-nor.c
>> > +++ b/drivers/mtd/spi-nor/spi-nor.c
>> > @@ -238,6 +238,14 @@ struct flash_info {
>> >  					 * status register. Must be
>> > used with
>> >  					 * SPI_NOR_HAS_TB.
>> >  					 */
>> > +#define SPI_NOR_HAS_BP3		BIT(17)	/*
>> > +					 * Flash SR has 4 bit fields
>> > (BP0-3)
>> > +					 * for block protection.
>> > +					 */
>> > +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
>> > +					 * BP3 is bit 6 of status
>> > register.
>> > +					 * Must be used with
>> > SPI_NOR_HAS_BP3.
>> > +					 */
>> >
>> >  	/* Part specific fixup hooks. */
>> >  	const struct spi_nor_fixups *fixups;
>> > @@ -1767,23 +1775,47 @@ static void stm_get_locked_range(struct
>> > spi_nor *nor, u8 sr, loff_t *ofs,
>> >  	struct mtd_info *mtd = &nor->mtd;
>> >  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> >  	u8 tb_mask = SR_TB_BIT5;
>> > -	int pow;
>> > +	u8 bp;
>> > +	int pow = 0;
>> >
>> >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>> >  		tb_mask = SR_TB_BIT6;
>> >
>> > -	if (!(sr & mask)) {
>> > -		/* No protection */
>> > -		*ofs = 0;
>> > -		*len = 0;
>> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > +		u8 tmp;
>> > +
>> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > +			tmp = sr & (mask | SR_BP3_BIT6);
>> > +		else
>> > +			tmp = sr & (mask | SR_BP3_BIT5);
>> > +
>> > +		if (tmp & SR_BP3_BIT6)
>> > +			tmp = (tmp & ~BIT(6)) | BIT(5);
>> > +
>> > +		bp = tmp >> SR_BP_SHIFT;
>> > +		if (!bp) {
>> > +			*ofs = 0;
>> > +			*len = 0;
>> > +			return;
>> > +		}
>> > +		if (bp <= ilog2(nor->n_sectors))
>> > +			pow = ilog2(nor->n_sectors) + 1 - bp;
>> >  	} else {
>> > -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
>> > -		*len = mtd->size >> pow;
>> > -		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>> > +		bp = (sr & mask) >> SR_BP_SHIFT;
>> > +		if (!bp) {
>> >  			*ofs = 0;
>> > -		else
>> > -			*ofs = mtd->size - *len;
>> > +			*len = 0;
>> > +			return;
>> > +		}
>> > +		pow = bp ^ (mask >> SR_BP_SHIFT);
>> >  	}
>> > +
>> > +	*len = mtd->size >> pow;
>> > +
>> > +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>> > +		*ofs = 0;
>> > +	else
>> > +		*ofs = mtd->size - *len;
>> >  }
>> >
>> >  /*
>> > @@ -1823,7 +1855,7 @@ static int stm_is_unlocked_sr(struct spi_nor
>> > *nor, loff_t ofs, uint64_t len,
>> >
>> >  /*
>> >   * Lock a region of the flash. Compatible with ST Micro and
>> > similar flash.
>> > - * Supports the block protection bits BP{0,1,2} in the status
>> > register
>> > + * Supports the block protection bits BP{0,1,2,3} in the status
>> > register
>> >   * (SR). Does not support these features found in newer SR
>> > bitfields:
>> >   *   - SEC: sector/block protect - only handle SEC=0 (block
>> > protect)
>> >   *   - CMP: complement protect - only support CMP=0 (range is not
>> > complemented)
>> > @@ -1831,7 +1863,7 @@ static int stm_is_unlocked_sr(struct spi_nor
>> > *nor, loff_t ofs, uint64_t len,
>> >   * Support for the following is provided conditionally for some
>> > flash:
>> >   *   - TB: top/bottom protect
>> >   *
>> > - * Sample table portion for 8MB flash (Winbond w25q64fw):
>> > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
>> >   *
>> >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
>> > Protected Portion
>> >   *  ------------------------------------------------------------
>> > --------------
>> > @@ -1851,6 +1883,32 @@ static int stm_is_unlocked_sr(struct spi_nor
>> > *nor, loff_t ofs, uint64_t len,
>> >   *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower
>> > 1/4
>> >   *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower
>> > 1/2
>> >   *
>> > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
>> > 3):
>> > + *
>> > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
>> > Protected Portion
>> > + *  ------------------------------------------------------------
>> > --------------
>> > + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
>> > + *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper
>> > 1/1024
>> > + *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper
>> > 1/512
>> > + *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper
>> > 1/256
>> > + *   ...
>> > + *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper
>> > 1/4
>> > + *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper
>> > 1/2
>> > + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
>> > + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
>> > + *   ...
>> > + *  ------|-------|-------|-------|-------|---------------|-------
>> > ------------
>> > + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
>> > + *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower
>> > 1/1024
>> > + *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower
>> > 1/512
>> > + *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower
>> > 1/256
>> > + *   ...
>> > + *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower
>> > 1/4
>> > + *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower
>> > 1/2
>> > + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
>> > + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
>> > + *   ...
>> > + *
>> >   * Returns negative on errors, 0 on success.
>> >   */
>> >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> > @@ -1898,6 +1956,12 @@ static int stm_lock(struct spi_nor *nor,
>> > loff_t ofs, uint64_t len)
>> >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>> >  		tb_mask = SR_TB_BIT6;
>> >
>> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > +			mask = mask | SR_BP3_BIT6;
>> > +		else
>> > +			mask = mask | SR_BP3_BIT5;
>> > +	}
>> >  	/*
>> >  	 * Need smallest pow such that:
>> >  	 *
>> > @@ -1908,7 +1972,17 @@ static int stm_lock(struct spi_nor *nor,
>> > loff_t ofs, uint64_t len)
>> >  	 *   pow = ceil(log2(size / len)) = log2(size) -
>> > floor(log2(len))
>> >  	 */
>> >  	pow = ilog2(mtd->size) - ilog2(lock_len);
>> > -	val = mask - (pow << SR_BP_SHIFT);
>> > +
>> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > +		val = ilog2(nor->n_sectors) + 1 - pow;
>> 
>> Why do you use a new calculation here? As far as I can see, the
>> method is
>> the same except that is has one bit more. That also raises the
>> question why
>> n_sectors is now needed?
>> 
>> Can't we just initialize the mask with
>> 
>> mask = SR_BP2 | SR_BP1 | SR_BP0;
>> if (nor->flags & SNOR_F_HAS_SR_BP3)
>>     mask |= SR_BP3_BIT5;
>> 
>> do the calculation and checks and then move the SR_BP3_BIT5 to
>> SR_BP3_BIT6
>> if SNOR_F_HAS_SR_BP3_BIT6 is set.
>> 
> 
> For most of flashes that supporting BP0-2, the smallest protected
> portion is fixed as 1/64
> and it can be properly handled by existing
> calculation. (Actually it's not fully generic, see flashes like
> w25q40bw or m25p80. Of course, it doesn't have SPI_NOR_HAS_LOCK flag
> even though it has BP0-2 bit in SR)

No. The rules are always the same wether there are three or four BP
bits (the example in stm_lock() has not enough information on this):

(1) the first setting (besides 0) protects one sector. The second
     protects 2, the third 4 and so on. eg 2^N
(2) the last setting is _always_ protect all, just like the '0' setting
     is always protect none.
(3) if there is an overflow because there are no more free slots for
     further settings (for 3 bits with flashes > 32MBit, for 4
     bits if should be flashes > 16GBit), the first entry will be
     discarded (eg the very first is the "just one sector" entry).

This is true for all flashes which uses this kind of setting, have a
look at the m25p80 or w25q40bw, these are no exception. It is just the
notation "lower 1/64" which is only true for flashes which either
overflows in (3) or fill all entries (eg. with 3bits that would be the
32Mbit version).

So for the 3 bit case the following flashes are border cases:
  - 16mbit (less settings than slots)
  - 32mbit (number of settings and free slots match)
  - 64mbit (more settings than slots, first setting is discarded)

That being said, I suspect all the 16mbit flashes (and below) which have
the _LOCK bit set are broken, because the entries has to be shifted. 
I'll
look into that later. This is the same "issue" you have with the 4 bits.
So if this is fixed, you should not need another formula for the 4 bit
case.

> We need new calculation method for 4bit block protection and for making
> it more generic, I choose n_sectors.
> 
> On all the flashes I checked, n_sectors is proper value for getting
> block protected portion.
> 
> 		density	portion	n_sectors
> W25M512JV	64MB	1/512	512
> N25Q128A	16MB	1/256	256
> N25Q512A	64MB	1/1024	1024
> MT25QL02GCBB	256MB	1/4096	4096

The rules above apply to these flashes, too. Could you double check 
that?

-michael

> 
>> > +		val = val << SR_BP_SHIFT;
>> > +
>> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > +			val = (val & ~BIT(5)) | BIT(6);
>> > +	} else {
>> > +		val = mask - (pow << SR_BP_SHIFT);
>> > +	}
>> > +
>> >  	if (val & ~mask)
>> >  		return -EINVAL;
>> >  	/* Don't "lock" with no region! */
>> > @@ -1983,6 +2057,13 @@ static int stm_unlock(struct spi_nor *nor,
>> > loff_t ofs, uint64_t len)
>> >
>> >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>> >  		tb_mask = SR_TB_BIT6;
>> > +
>> > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > +			mask = mask | SR_BP3_BIT6;
>> > +		else
>> > +			mask = mask | SR_BP3_BIT5;
>> > +	}
>> >  	/*
>> >  	 * Need largest pow such that:
>> >  	 *
>> > @@ -1995,13 +2076,20 @@ static int stm_unlock(struct spi_nor *nor,
>> > loff_t ofs, uint64_t len)
>> >  	pow = ilog2(mtd->size) - order_base_2(lock_len);
>> >  	if (lock_len == 0) {
>> >  		val = 0; /* fully unlocked */
>> > +	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > +		val = ilog2(nor->n_sectors) + 1 - pow;
>> > +		val = val << SR_BP_SHIFT;
>> > +
>> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > +			val = (val & ~BIT(5)) | BIT(6);
>> >  	} else {
>> >  		val = mask - (pow << SR_BP_SHIFT);
>> > -		/* Some power-of-two sizes are not supported */
>> > -		if (val & ~mask)
>> > -			return -EINVAL;
>> >  	}
>> >
>> > +	/* Some power-of-two sizes are not supported */
>> > +	if (val & ~mask)
>> > +		return -EINVAL;
>> > +
>> >  	status_new = (status_old & ~mask & ~tb_mask) | val;
>> >
>> >  	/* Don't protect status register if we're fully unlocked */
>> > @@ -4736,6 +4824,7 @@ static void spi_nor_info_init_params(struct
>> > spi_nor *nor)
>> >  	/* Set SPI NOR sizes. */
>> >  	params->size = (u64)info->sector_size * info->n_sectors;
>> >  	params->page_size = info->page_size;
>> > +	params->n_sectors = info->n_sectors;
>> >
>> >  	if (!(info->flags & SPI_NOR_NO_FR)) {
>> >  		/* Default to Fast Read for DT and non-DT platform
>> > devices. */
>> > @@ -5192,6 +5281,11 @@ 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_HAS_BP3) {
>> > +		nor->flags |= SNOR_F_HAS_SR_BP3;
>> > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
>> > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>> > +	}
>> >
>> >  	if (info->flags & SPI_NOR_NO_ERASE)
>> >  		mtd->flags |= MTD_NO_ERASE;
>> > @@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor *nor, const
>> > char *name,
>> >  	mtd->dev.parent = dev;
>> >  	nor->page_size = params->page_size;
>> >  	mtd->writebufsize = nor->page_size;
>> > +	nor->n_sectors = params->n_sectors;
>> >
>> >  	if (of_property_read_bool(np, "broken-flash-reset"))
>> >  		nor->flags |= SNOR_F_BROKEN_RESET;
>> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
>> > nor.h
>> > index 541c06d042e8..92d550501daf 100644
>> > --- a/include/linux/mtd/spi-nor.h
>> > +++ b/include/linux/mtd/spi-nor.h
>> > @@ -129,7 +129,9 @@
>> >  #define SR_BP1			BIT(3)	/* Block protect 1
>> > */
>> >  #define SR_BP2			BIT(4)	/* Block protect 2
>> > */
>> >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
>> > +#define SR_BP3_BIT5		BIT(5)	/* Block protect 3
>> > */
>> 
>> maybe just name it SR_BP3? would also be more consistent with the
>> proposal
>> above.
>> 
>> >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
>> > +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3
>> > */
>> >  #define SR_SRWD			BIT(7)	/* SR write protect
>> > */
>> >  /* Spansion/Cypress specific status bits */
>> >  #define SR_E_ERR		BIT(5)
>> > @@ -248,6 +250,8 @@ enum spi_nor_option_flags {
>> >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
>> >  	SNOR_F_NO_READ_CR	= BIT(10),
>> >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
>> > +	SNOR_F_HAS_SR_BP3	= BIT(12),
>> > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
>> >
>> >  };
>> >
>> > @@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
>> >   *
>> >   * @size:		the flash memory density in bytes.
>> >   * @page_size:		the page size of the SPI NOR flash
>> > memory.
>> > + * @n_sectors:		number of sectors
>> >   * @hwcaps:		describes the read and page program
>> > hardware
>> >   *			capabilities.
>> >   * @reads:		read capabilities ordered by priority: the
>> > higher index
>> > @@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
>> >  struct spi_nor_flash_parameter {
>> >  	u64				size;
>> >  	u32				page_size;
>> > +	u16				n_sectors;
>> >
>> >  	struct spi_nor_hwcaps		hwcaps;
>> >  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
>> > @@ -573,6 +579,7 @@ struct flash_info;
>> >   * @bouncebuf_size:	size of the bounce buffer
>> >   * @info:		spi-nor part JDEC MFR id and other info
>> >   * @page_size:		the page size of the SPI NOR
>> > + * @n_sector:		number of sectors
>> >   * @addr_width:		number of address bytes
>> >   * @erase_opcode:	the opcode for erasing a sector
>> >   * @read_opcode:	the read opcode
>> > @@ -599,6 +606,7 @@ struct spi_nor {
>> >  	size_t			bouncebuf_size;
>> >  	const struct flash_info	*info;
>> >  	u32			page_size;
>> > +	u16			n_sectors;
>> >  	u8			addr_width;
>> >  	u8			erase_opcode;
>> >  	u8			read_opcode;
>> > --
>> > 2.17.1
>> >
>> >
>> > ______________________________________________________
>> > Linux MTD discussion mailing list
>> >
> https://protect2.fireeye.com/url?k=06b6dd5d-5b7d5a63-06b75612-0cc47a31309a-83164929001f7741&u=http://lists.infradead.org/mailman/listinfo/linux-mtd/
>> >
>> 
>> 

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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-23  3:59                 ` Jungseung Lee
@ 2020-01-23  8:15                   ` Michael Walle
  0 siblings, 0 replies; 19+ messages in thread
From: Michael Walle @ 2020-01-23  8:15 UTC (permalink / raw)
  To: Jungseung Lee; +Cc: linux-mtd, john.garry, js07.lee, vigneshr, Tudor.Ambarus

Am 2020-01-23 04:59, schrieb Jungseung Lee:
> Hi, Michael
> 
> 2020-01-22 (Wed), 18:14 +0100, Michael Walle:
>> Am 2020-01-22 15:31, schrieb Tudor.Ambarus@microchip.com:
>> > Hi, Jungseung,
>> >
>> > On Wednesday, January 22, 2020 1:42:00 PM EET Jungseung Lee wrote:
>> >
>> > cut
>> >
>> > > > > > +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
>> > > > > > +                                        * BP3 is bit 6 of
>> > > > > > status
>> > > > > > register.
>> > > > > > +                                        * Must be used
>> > > > > > with
>> > > > >
>> > > > > Are we safe to replace SPI_NOR_TB_SR_BIT6 and
>> > > > > SPI_NOR_BP3_SR_BIT6
>> > > > > with a
>> > > > > SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a
>> > > > > SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
>> > > > > is more convenient?
>> > > >
>> > > > Let's think about some flash in which BP0-3 exists in the
>> > > > status
>> > > > register but TB exists in another register.
>> > > > for example, mx25u12835f.
>> 
>> And like the mx25u3232f, but this bit is only OTP programmable! For
>> now,
>> I'd only add support for reading the TB bit to for this kind of
>> flashes,
>> to prevent accidentially program this bit. It would be up to the
>> board
>> manufacturer how to actually set the bit.
>> 
>> Like having a TB_READ_ONLY flag.
>> 
>> Its also some kind of asymmetrical because you can only set it one
>> way,
>> eg. you can OTP the flash to be TB=1. Then you can be sure that the
>> flash
>> will always be TB=1. But OTOH you cannot be sure that a TB=0 flash
>> will
>> always be a TB=0 flash, because you cannot lock that bit.
>> 
>> Any thoughts?
>> 
> 
> Actually, I didn't get the chance to take a look at some flash that has
> TB bit in configuration register. Currently mainline kernel just care
> flashes that has TB bit in SR and SPI_NOR_HAS_TB can be set only such
> flashes(as you could see in comment). It seems neccessary to add
> another functions and flag for mx25u3232f case.
> Is there any flash that has OTP programmable TB bit in SR?

Not that I'm aware of. My intention wasn't to have anything changed in
this patch series regarding these flashes, just to ask if anyone else
has ideas how they would solve the mx25u3232f (or mx25u12835f) case.

-michael

> Tudor and me were saying that there is some flash that has not TB bit
> in *SR* even if it has BP3 bit in SR. So it seems unnecessary to make
> correlated flag like SPI_NOR_SR_TB_BIT6_BP3_BIT5 for convenience.
> 
>> -michael
>> 
>> > > > I haven't tested yet, but according to the datasheet, I think
>> > > > this
>> > > > patch can support 4bit block protection for the flash.
>> > > >
>> > > > In order to embrace the case, how about letting them as It is.
>> > > > Is there any suggestion?
>> >
>> > Ok, this info should go in the commit message, together with
>> > details
>> > about
>> > which flashes were tested.
>> >
>> > I didn't know that the TB bit can be defined in the Configuration
>> > register.
>> > This means that your suggestion with dedicated macros for BP3 and
>> > TB is
>> > fine.
>> >
>> > I looked a bit over your patches, they are in a pretty good shape.
>> > I
>> > saw
>> > something that can be improved on patch 2/3, but I didn't manage
>> > to
>> > finish the
>> > review. Your patches are the first on my TODO list, but now I'm a
>> > bit
>> > busy. I
>> > hope that I'll come with a complete review by the end of the next
>> > week.
>> > I'm
>> > going to do tests on few flashes too, to make sure that BP0-2 was
>> > not
>> > affected.
>> >
>> > In the meantime, maybe Michael or John can review/test your
>> > patches,
>> > they
>> > showed interest in BP0-3 support.
>> >
>> > Cheers,
>> > ta
>> 
>> 
> Thanks,
> Jungseung Lee

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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-23  8:10           ` Michael Walle
@ 2020-01-23  8:53             ` Jungseung Lee
  2020-01-23  9:31               ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Jungseung Lee @ 2020-01-23  8:53 UTC (permalink / raw)
  To: Michael Walle; +Cc: tudor.ambarus, linux-mtd, vigneshr, js07.lee

Hi, Michael

2020-01-23 (Thu), 09:10 +0100, Michael Walle:
> Hi Jungseung,
> 
> Am 2020-01-23 07:22, schrieb Jungseung Lee:
> > Hi, Michael
> > 
> > 2020-01-22 (Wed), 20:36 +0100, Michael Walle:
> > > Hi,
> > > 
> > > > Currently, we are supporting block protection only for
> > > > flash chips with 3 block protection bits in the SR register.
> > > > This patch enables block protection support for some flash with
> > > > 4 block protection bits(bp0-3).
> > > > 
> > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > > ---
> > > > v3 :
> > > >   Fix wrong ofs calculation on v2 patch
> > > > v2 :
> > > >   Add sample table portion about 4bit block protection on the
> > > > comment
> > > >   Trivial coding style change
> > > > 
> > > >  drivers/mtd/spi-nor/spi-nor.c | 127
> > > > +++++++++++++++++++++++++++++-
> > > > ----
> > > >  include/linux/mtd/spi-nor.h   |   8 +++
> > > >  2 files changed, 119 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > > > nor/spi-nor.c
> > > > index e3da6a8654a8..7e8af6c4fdfa 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -238,6 +238,14 @@ struct flash_info {
> > > >  					 * status register.
> > > > Must be
> > > > used with
> > > >  					 * SPI_NOR_HAS_TB.
> > > >  					 */
> > > > +#define SPI_NOR_HAS_BP3		BIT(17)	/*
> > > > +					 * Flash SR has 4 bit
> > > > fields
> > > > (BP0-3)
> > > > +					 * for block
> > > > protection.
> > > > +					 */
> > > > +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
> > > > +					 * BP3 is bit 6 of
> > > > status
> > > > register.
> > > > +					 * Must be used with
> > > > SPI_NOR_HAS_BP3.
> > > > +					 */
> > > > 
> > > >  	/* Part specific fixup hooks. */
> > > >  	const struct spi_nor_fixups *fixups;
> > > > @@ -1767,23 +1775,47 @@ static void stm_get_locked_range(struct
> > > > spi_nor *nor, u8 sr, loff_t *ofs,
> > > >  	struct mtd_info *mtd = &nor->mtd;
> > > >  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > >  	u8 tb_mask = SR_TB_BIT5;
> > > > -	int pow;
> > > > +	u8 bp;
> > > > +	int pow = 0;
> > > > 
> > > >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > >  		tb_mask = SR_TB_BIT6;
> > > > 
> > > > -	if (!(sr & mask)) {
> > > > -		/* No protection */
> > > > -		*ofs = 0;
> > > > -		*len = 0;
> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > +		u8 tmp;
> > > > +
> > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > > +			tmp = sr & (mask | SR_BP3_BIT6);
> > > > +		else
> > > > +			tmp = sr & (mask | SR_BP3_BIT5);
> > > > +
> > > > +		if (tmp & SR_BP3_BIT6)
> > > > +			tmp = (tmp & ~BIT(6)) | BIT(5);
> > > > +
> > > > +		bp = tmp >> SR_BP_SHIFT;
> > > > +		if (!bp) {
> > > > +			*ofs = 0;
> > > > +			*len = 0;
> > > > +			return;
> > > > +		}
> > > > +		if (bp <= ilog2(nor->n_sectors))
> > > > +			pow = ilog2(nor->n_sectors) + 1 - bp;
> > > >  	} else {
> > > > -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> > > > -		*len = mtd->size >> pow;
> > > > -		if (nor->flags & SNOR_F_HAS_SR_TB && sr &
> > > > tb_mask)
> > > > +		bp = (sr & mask) >> SR_BP_SHIFT;
> > > > +		if (!bp) {
> > > >  			*ofs = 0;
> > > > -		else
> > > > -			*ofs = mtd->size - *len;
> > > > +			*len = 0;
> > > > +			return;
> > > > +		}
> > > > +		pow = bp ^ (mask >> SR_BP_SHIFT);
> > > >  	}
> > > > +
> > > > +	*len = mtd->size >> pow;
> > > > +
> > > > +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> > > > +		*ofs = 0;
> > > > +	else
> > > > +		*ofs = mtd->size - *len;
> > > >  }
> > > > 
> > > >  /*
> > > > @@ -1823,7 +1855,7 @@ static int stm_is_unlocked_sr(struct
> > > > spi_nor
> > > > *nor, loff_t ofs, uint64_t len,
> > > > 
> > > >  /*
> > > >   * Lock a region of the flash. Compatible with ST Micro and
> > > > similar flash.
> > > > - * Supports the block protection bits BP{0,1,2} in the status
> > > > register
> > > > + * Supports the block protection bits BP{0,1,2,3} in the
> > > > status
> > > > register
> > > >   * (SR). Does not support these features found in newer SR
> > > > bitfields:
> > > >   *   - SEC: sector/block protect - only handle SEC=0 (block
> > > > protect)
> > > >   *   - CMP: complement protect - only support CMP=0 (range is
> > > > not
> > > > complemented)
> > > > @@ -1831,7 +1863,7 @@ static int stm_is_unlocked_sr(struct
> > > > spi_nor
> > > > *nor, loff_t ofs, uint64_t len,
> > > >   * Support for the following is provided conditionally for
> > > > some
> > > > flash:
> > > >   *   - TB: top/bottom protect
> > > >   *
> > > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > > > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-
> > > > 2):
> > > >   *
> > > >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > > > Protected Portion
> > > >   *  --------------------------------------------------------
> > > > ----
> > > > --------------
> > > > @@ -1851,6 +1883,32 @@ static int stm_is_unlocked_sr(struct
> > > > spi_nor
> > > > *nor, loff_t ofs, uint64_t len,
> > > >   *    0   |   1   |   1   |   0   |   1   |  2 MB         |
> > > > Lower
> > > > 1/4
> > > >   *    0   |   1   |   1   |   1   |   0   |  4 MB         |
> > > > Lower
> > > > 1/2
> > > >   *
> > > > + * Sample table portion for 64MB flash (Micron n25q512ax3 /
> > > > BP0-
> > > > 3):
> > > > + *
> > > > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > > > Protected Portion
> > > > + *  --------------------------------------------------------
> > > > ----
> > > > --------------
> > > > + *    0   |   0   |   0   |   0   |   0   |  NONE         |
> > > > NONE
> > > > + *    0   |   0   |   0   |   0   |   1   |   64 KB       |
> > > > Upper
> > > > 1/1024
> > > > + *    0   |   0   |   0   |   1   |   0   |  128 KB       |
> > > > Upper
> > > > 1/512
> > > > + *    0   |   0   |   0   |   1   |   1   |  256 KB       |
> > > > Upper
> > > > 1/256
> > > > + *   ...
> > > > + *    0   |   1   |   0   |   0   |   1   |  16 MB        |
> > > > Upper
> > > > 1/4
> > > > + *    0   |   1   |   0   |   1   |   0   |  32 MB        |
> > > > Upper
> > > > 1/2
> > > > + *    0   |   1   |   0   |   1   |   1   |  64 MB        |
> > > > ALL
> > > > + *    0   |   1   |   1   |   0   |   0   |  64 MB        |
> > > > ALL
> > > > + *   ...
> > > > + *  ------|-------|-------|-------|-------|---------------|---
> > > > ----
> > > > ------------
> > > > + *    1   |   0   |   0   |   0   |   0   |   NONE        |
> > > > NONE
> > > > + *    1   |   0   |   0   |   0   |   1   |   64 KB       |
> > > > Lower
> > > > 1/1024
> > > > + *    1   |   0   |   0   |   1   |   0   |  128 KB       |
> > > > Lower
> > > > 1/512
> > > > + *    1   |   0   |   0   |   1   |   1   |  256 KB       |
> > > > Lower
> > > > 1/256
> > > > + *   ...
> > > > + *    1   |   1   |   0   |   0   |   1   |  16 MB        |
> > > > Lower
> > > > 1/4
> > > > + *    1   |   1   |   0   |   1   |   0   |  32 MB        |
> > > > Lower
> > > > 1/2
> > > > + *    1   |   1   |   0   |   1   |   1   |  64 MB        |
> > > > ALL
> > > > + *    1   |   1   |   1   |   0   |   0   |  64 MB        |
> > > > ALL
> > > > + *   ...
> > > > + *
> > > >   * Returns negative on errors, 0 on success.
> > > >   */
> > > >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t
> > > > len)
> > > > @@ -1898,6 +1956,12 @@ static int stm_lock(struct spi_nor *nor,
> > > > loff_t ofs, uint64_t len)
> > > >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > >  		tb_mask = SR_TB_BIT6;
> > > > 
> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > > +			mask = mask | SR_BP3_BIT6;
> > > > +		else
> > > > +			mask = mask | SR_BP3_BIT5;
> > > > +	}
> > > >  	/*
> > > >  	 * Need smallest pow such that:
> > > >  	 *
> > > > @@ -1908,7 +1972,17 @@ static int stm_lock(struct spi_nor *nor,
> > > > loff_t ofs, uint64_t len)
> > > >  	 *   pow = ceil(log2(size / len)) = log2(size) -
> > > > floor(log2(len))
> > > >  	 */
> > > >  	pow = ilog2(mtd->size) - ilog2(lock_len);
> > > > -	val = mask - (pow << SR_BP_SHIFT);
> > > > +
> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > +		val = ilog2(nor->n_sectors) + 1 - pow;
> > > 
> > > Why do you use a new calculation here? As far as I can see, the
> > > method is
> > > the same except that is has one bit more. That also raises the
> > > question why
> > > n_sectors is now needed?
> > > 
> > > Can't we just initialize the mask with
> > > 
> > > mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > if (nor->flags & SNOR_F_HAS_SR_BP3)
> > >     mask |= SR_BP3_BIT5;
> > > 
> > > do the calculation and checks and then move the SR_BP3_BIT5 to
> > > SR_BP3_BIT6
> > > if SNOR_F_HAS_SR_BP3_BIT6 is set.
> > > 
> > 
> > For most of flashes that supporting BP0-2, the smallest protected
> > portion is fixed as 1/64
> > and it can be properly handled by existing
> > calculation. (Actually it's not fully generic, see flashes like
> > w25q40bw or m25p80. Of course, it doesn't have SPI_NOR_HAS_LOCK
> > flag
> > even though it has BP0-2 bit in SR)
> 
> No. The rules are always the same wether there are three or four BP
> bits (the example in stm_lock() has not enough information on this):
> 
> (1) the first setting (besides 0) protects one sector. The second
>      protects 2, the third 4 and so on. eg 2^N
> (2) the last setting is _always_ protect all, just like the '0'
> setting
>      is always protect none.
> (3) if there is an overflow because there are no more free slots for
>      further settings (for 3 bits with flashes > 32MBit, for 4
>      bits if should be flashes > 16GBit), the first entry will be
>      discarded (eg the very first is the "just one sector" entry).
> 
> This is true for all flashes which uses this kind of setting, have a
> look at the m25p80 or w25q40bw, these are no exception. It is just
> the
> notation "lower 1/64" which is only true for flashes which either
> overflows in (3) or fill all entries (eg. with 3bits that would be
> the
> 32Mbit version).
> 

Looks like you noticed that we need new calculation method that would
be based on n_sectors :). Rule (1) is NOT true for some flashes
supporting BP0-2 and that's why I said that smallest protected portion
is fixed as '1/64' for these flashes.

See this one.

W25Q20EW	256KB	1/4 ... = 64KB		BP2
W25Q128JV	16MB	1/64 ... = 256KB	BP2 <--
S25FL132K	4MB	1/64 ... = 64KB		BP2 <--
S25FL164K	8MB	
1/64 ... = 128KB	BP2 <--
W25Q256JV	32MB	1/512 ... =
64KB	BP3
S25FL128L	16MB	1/256 ... = 64KB	BP3
S25FL256L	32MB	1/512 ... = 64KB	BP3

In current BP implementation, block protection is just working for some
flashes that has smallest protected portion as '1/64'.

We need new fomula based on n_sectors for BP3 at least.

> So for the 3 bit case the following flashes are border cases:
>   - 16mbit (less settings than slots)
>   - 32mbit (number of settings and free slots match)
>   - 64mbit (more settings than slots, first setting is discarded)
> 
> That being said, I suspect all the 16mbit flashes (and below) which
> have
> the _LOCK bit set are broken, because the entries has to be shifted. 
> I'll
> look into that later. This is the same "issue" you have with the 4
> bits.
> So if this is fixed, you should not need another formula for the 4
> bit
> case.
> 
> > We need new calculation method for 4bit block protection and for
> > making
> > it more generic, I choose n_sectors.
> > 
> > On all the flashes I checked, n_sectors is proper value for getting
> > block protected portion.
> > 
> > 		density	portion	n_sectors
> > W25M512JV	64MB	1/512	512
> > N25Q128A	16MB	1/256	256
> > N25Q512A	64MB	1/1024	1024
> > MT25QL02GCBB	256MB	1/4096	4096
> 
> The rules above apply to these flashes, too. Could you double check 
> that?
> 
> -michael
> 
> > 
> > > > +		val = val << SR_BP_SHIFT;
> > > > +
> > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > +			val = (val & ~BIT(5)) | BIT(6);
> > > > +	} else {
> > > > +		val = mask - (pow << SR_BP_SHIFT);
> > > > +	}
> > > > +
> > > >  	if (val & ~mask)
> > > >  		return -EINVAL;
> > > >  	/* Don't "lock" with no region! */
> > > > @@ -1983,6 +2057,13 @@ static int stm_unlock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > > 
> > > >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > >  		tb_mask = SR_TB_BIT6;
> > > > +
> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > > +			mask = mask | SR_BP3_BIT6;
> > > > +		else
> > > > +			mask = mask | SR_BP3_BIT5;
> > > > +	}
> > > >  	/*
> > > >  	 * Need largest pow such that:
> > > >  	 *
> > > > @@ -1995,13 +2076,20 @@ static int stm_unlock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >  	pow = ilog2(mtd->size) - order_base_2(lock_len);
> > > >  	if (lock_len == 0) {
> > > >  		val = 0; /* fully unlocked */
> > > > +	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > +		val = ilog2(nor->n_sectors) + 1 - pow;
> > > > +		val = val << SR_BP_SHIFT;
> > > > +
> > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > +			val = (val & ~BIT(5)) | BIT(6);
> > > >  	} else {
> > > >  		val = mask - (pow << SR_BP_SHIFT);
> > > > -		/* Some power-of-two sizes are not supported */
> > > > -		if (val & ~mask)
> > > > -			return -EINVAL;
> > > >  	}
> > > > 
> > > > +	/* Some power-of-two sizes are not supported */
> > > > +	if (val & ~mask)
> > > > +		return -EINVAL;
> > > > +
> > > >  	status_new = (status_old & ~mask & ~tb_mask) | val;
> > > > 
> > > >  	/* Don't protect status register if we're fully
> > > > unlocked */
> > > > @@ -4736,6 +4824,7 @@ static void
> > > > spi_nor_info_init_params(struct
> > > > spi_nor *nor)
> > > >  	/* Set SPI NOR sizes. */
> > > >  	params->size = (u64)info->sector_size * info-
> > > > >n_sectors;
> > > >  	params->page_size = info->page_size;
> > > > +	params->n_sectors = info->n_sectors;
> > > > 
> > > >  	if (!(info->flags & SPI_NOR_NO_FR)) {
> > > >  		/* Default to Fast Read for DT and non-DT
> > > > platform
> > > > devices. */
> > > > @@ -5192,6 +5281,11 @@ 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_HAS_BP3) {
> > > > +		nor->flags |= SNOR_F_HAS_SR_BP3;
> > > > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > > > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > > > +	}
> > > > 
> > > >  	if (info->flags & SPI_NOR_NO_ERASE)
> > > >  		mtd->flags |= MTD_NO_ERASE;
> > > > @@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor *nor,
> > > > const
> > > > char *name,
> > > >  	mtd->dev.parent = dev;
> > > >  	nor->page_size = params->page_size;
> > > >  	mtd->writebufsize = nor->page_size;
> > > > +	nor->n_sectors = params->n_sectors;
> > > > 
> > > >  	if (of_property_read_bool(np, "broken-flash-reset"))
> > > >  		nor->flags |= SNOR_F_BROKEN_RESET;
> > > > diff --git a/include/linux/mtd/spi-nor.h
> > > > b/include/linux/mtd/spi-
> > > > nor.h
> > > > index 541c06d042e8..92d550501daf 100644
> > > > --- a/include/linux/mtd/spi-nor.h
> > > > +++ b/include/linux/mtd/spi-nor.h
> > > > @@ -129,7 +129,9 @@
> > > >  #define SR_BP1			BIT(3)	/* Block protect 1
> > > > */
> > > >  #define SR_BP2			BIT(4)	/* Block protect 2
> > > > */
> > > >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect
> > > > */
> > > > +#define SR_BP3_BIT5		BIT(5)	/* Block protect 3
> > > > */
> > > 
> > > maybe just name it SR_BP3? would also be more consistent with the
> > > proposal
> > > above.
> > > 
> > > >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect
> > > > */
> > > > +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3
> > > > */
> > > >  #define SR_SRWD			BIT(7)	/* SR write
> > > > protect
> > > > */
> > > >  /* Spansion/Cypress specific status bits */
> > > >  #define SR_E_ERR		BIT(5)
> > > > @@ -248,6 +250,8 @@ enum spi_nor_option_flags {
> > > >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
> > > >  	SNOR_F_NO_READ_CR	= BIT(10),
> > > >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> > > > +	SNOR_F_HAS_SR_BP3	= BIT(12),
> > > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > > > 
> > > >  };
> > > > 
> > > > @@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
> > > >   *
> > > >   * @size:		the flash memory density in bytes.
> > > >   * @page_size:		the page size of the SPI NOR flash
> > > > memory.
> > > > + * @n_sectors:		number of sectors
> > > >   * @hwcaps:		describes the read and page program
> > > > hardware
> > > >   *			capabilities.
> > > >   * @reads:		read capabilities ordered by priority:
> > > > the
> > > > higher index
> > > > @@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
> > > >  struct spi_nor_flash_parameter {
> > > >  	u64				size;
> > > >  	u32				page_size;
> > > > +	u16				n_sectors;
> > > > 
> > > >  	struct spi_nor_hwcaps		hwcaps;
> > > >  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX
> > > > ];
> > > > @@ -573,6 +579,7 @@ struct flash_info;
> > > >   * @bouncebuf_size:	size of the bounce buffer
> > > >   * @info:		spi-nor part JDEC MFR id and other info
> > > >   * @page_size:		the page size of the SPI NOR
> > > > + * @n_sector:		number of sectors
> > > >   * @addr_width:		number of address bytes
> > > >   * @erase_opcode:	the opcode for erasing a sector
> > > >   * @read_opcode:	the read opcode
> > > > @@ -599,6 +606,7 @@ struct spi_nor {
> > > >  	size_t			bouncebuf_size;
> > > >  	const struct flash_info	*info;
> > > >  	u32			page_size;
> > > > +	u16			n_sectors;
> > > >  	u8			addr_width;
> > > >  	u8			erase_opcode;
> > > >  	u8			read_opcode;
> > > > --
> > > > 2.17.1
> > > > 
> > > > 
> > > > ______________________________________________________
> > > > Linux MTD discussion mailing list
> > > > 
> > 
> > 
https://protect2.fireeye.com/url?k=06b6dd5d-5b7d5a63-06b75612-0cc47a31309a-83164929001f7741&u=http://lists.infradead.org/mailman/listinfo/linux-mtd/
> > > > 
> > > 
> > > 
> 
> 


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

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

* Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
  2020-01-23  8:53             ` Jungseung Lee
@ 2020-01-23  9:31               ` Michael Walle
  0 siblings, 0 replies; 19+ messages in thread
From: Michael Walle @ 2020-01-23  9:31 UTC (permalink / raw)
  To: Jungseung Lee; +Cc: tudor.ambarus, linux-mtd, vigneshr, js07.lee

Am 2020-01-23 09:53, schrieb Jungseung Lee:
> Hi, Michael
> 
> 2020-01-23 (Thu), 09:10 +0100, Michael Walle:
>> Hi Jungseung,
>> 
>> Am 2020-01-23 07:22, schrieb Jungseung Lee:
>> > Hi, Michael
>> >
>> > 2020-01-22 (Wed), 20:36 +0100, Michael Walle:
>> > > Hi,
>> > >
>> > > > Currently, we are supporting block protection only for
>> > > > flash chips with 3 block protection bits in the SR register.
>> > > > This patch enables block protection support for some flash with
>> > > > 4 block protection bits(bp0-3).
>> > > >
>> > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>> > > > ---
>> > > > v3 :
>> > > >   Fix wrong ofs calculation on v2 patch
>> > > > v2 :
>> > > >   Add sample table portion about 4bit block protection on the
>> > > > comment
>> > > >   Trivial coding style change
>> > > >
>> > > >  drivers/mtd/spi-nor/spi-nor.c | 127
>> > > > +++++++++++++++++++++++++++++-
>> > > > ----
>> > > >  include/linux/mtd/spi-nor.h   |   8 +++
>> > > >  2 files changed, 119 insertions(+), 16 deletions(-)
>> > > >
>> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
>> > > > nor/spi-nor.c
>> > > > index e3da6a8654a8..7e8af6c4fdfa 100644
>> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
>> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
>> > > > @@ -238,6 +238,14 @@ struct flash_info {
>> > > >  					 * status register.
>> > > > Must be
>> > > > used with
>> > > >  					 * SPI_NOR_HAS_TB.
>> > > >  					 */
>> > > > +#define SPI_NOR_HAS_BP3		BIT(17)	/*
>> > > > +					 * Flash SR has 4 bit
>> > > > fields
>> > > > (BP0-3)
>> > > > +					 * for block
>> > > > protection.
>> > > > +					 */
>> > > > +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
>> > > > +					 * BP3 is bit 6 of
>> > > > status
>> > > > register.
>> > > > +					 * Must be used with
>> > > > SPI_NOR_HAS_BP3.
>> > > > +					 */
>> > > >
>> > > >  	/* Part specific fixup hooks. */
>> > > >  	const struct spi_nor_fixups *fixups;
>> > > > @@ -1767,23 +1775,47 @@ static void stm_get_locked_range(struct
>> > > > spi_nor *nor, u8 sr, loff_t *ofs,
>> > > >  	struct mtd_info *mtd = &nor->mtd;
>> > > >  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > > >  	u8 tb_mask = SR_TB_BIT5;
>> > > > -	int pow;
>> > > > +	u8 bp;
>> > > > +	int pow = 0;
>> > > >
>> > > >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>> > > >  		tb_mask = SR_TB_BIT6;
>> > > >
>> > > > -	if (!(sr & mask)) {
>> > > > -		/* No protection */
>> > > > -		*ofs = 0;
>> > > > -		*len = 0;
>> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > > > +		u8 tmp;
>> > > > +
>> > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > > > +			tmp = sr & (mask | SR_BP3_BIT6);
>> > > > +		else
>> > > > +			tmp = sr & (mask | SR_BP3_BIT5);
>> > > > +
>> > > > +		if (tmp & SR_BP3_BIT6)
>> > > > +			tmp = (tmp & ~BIT(6)) | BIT(5);
>> > > > +
>> > > > +		bp = tmp >> SR_BP_SHIFT;
>> > > > +		if (!bp) {
>> > > > +			*ofs = 0;
>> > > > +			*len = 0;
>> > > > +			return;
>> > > > +		}
>> > > > +		if (bp <= ilog2(nor->n_sectors))
>> > > > +			pow = ilog2(nor->n_sectors) + 1 - bp;
>> > > >  	} else {
>> > > > -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
>> > > > -		*len = mtd->size >> pow;
>> > > > -		if (nor->flags & SNOR_F_HAS_SR_TB && sr &
>> > > > tb_mask)
>> > > > +		bp = (sr & mask) >> SR_BP_SHIFT;
>> > > > +		if (!bp) {
>> > > >  			*ofs = 0;
>> > > > -		else
>> > > > -			*ofs = mtd->size - *len;
>> > > > +			*len = 0;
>> > > > +			return;
>> > > > +		}
>> > > > +		pow = bp ^ (mask >> SR_BP_SHIFT);
>> > > >  	}
>> > > > +
>> > > > +	*len = mtd->size >> pow;
>> > > > +
>> > > > +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>> > > > +		*ofs = 0;
>> > > > +	else
>> > > > +		*ofs = mtd->size - *len;
>> > > >  }
>> > > >
>> > > >  /*
>> > > > @@ -1823,7 +1855,7 @@ static int stm_is_unlocked_sr(struct
>> > > > spi_nor
>> > > > *nor, loff_t ofs, uint64_t len,
>> > > >
>> > > >  /*
>> > > >   * Lock a region of the flash. Compatible with ST Micro and
>> > > > similar flash.
>> > > > - * Supports the block protection bits BP{0,1,2} in the status
>> > > > register
>> > > > + * Supports the block protection bits BP{0,1,2,3} in the
>> > > > status
>> > > > register
>> > > >   * (SR). Does not support these features found in newer SR
>> > > > bitfields:
>> > > >   *   - SEC: sector/block protect - only handle SEC=0 (block
>> > > > protect)
>> > > >   *   - CMP: complement protect - only support CMP=0 (range is
>> > > > not
>> > > > complemented)
>> > > > @@ -1831,7 +1863,7 @@ static int stm_is_unlocked_sr(struct
>> > > > spi_nor
>> > > > *nor, loff_t ofs, uint64_t len,
>> > > >   * Support for the following is provided conditionally for
>> > > > some
>> > > > flash:
>> > > >   *   - TB: top/bottom protect
>> > > >   *
>> > > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
>> > > > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-
>> > > > 2):
>> > > >   *
>> > > >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
>> > > > Protected Portion
>> > > >   *  --------------------------------------------------------
>> > > > ----
>> > > > --------------
>> > > > @@ -1851,6 +1883,32 @@ static int stm_is_unlocked_sr(struct
>> > > > spi_nor
>> > > > *nor, loff_t ofs, uint64_t len,
>> > > >   *    0   |   1   |   1   |   0   |   1   |  2 MB         |
>> > > > Lower
>> > > > 1/4
>> > > >   *    0   |   1   |   1   |   1   |   0   |  4 MB         |
>> > > > Lower
>> > > > 1/2
>> > > >   *
>> > > > + * Sample table portion for 64MB flash (Micron n25q512ax3 /
>> > > > BP0-
>> > > > 3):
>> > > > + *
>> > > > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
>> > > > Protected Portion
>> > > > + *  --------------------------------------------------------
>> > > > ----
>> > > > --------------
>> > > > + *    0   |   0   |   0   |   0   |   0   |  NONE         |
>> > > > NONE
>> > > > + *    0   |   0   |   0   |   0   |   1   |   64 KB       |
>> > > > Upper
>> > > > 1/1024
>> > > > + *    0   |   0   |   0   |   1   |   0   |  128 KB       |
>> > > > Upper
>> > > > 1/512
>> > > > + *    0   |   0   |   0   |   1   |   1   |  256 KB       |
>> > > > Upper
>> > > > 1/256
>> > > > + *   ...
>> > > > + *    0   |   1   |   0   |   0   |   1   |  16 MB        |
>> > > > Upper
>> > > > 1/4
>> > > > + *    0   |   1   |   0   |   1   |   0   |  32 MB        |
>> > > > Upper
>> > > > 1/2
>> > > > + *    0   |   1   |   0   |   1   |   1   |  64 MB        |
>> > > > ALL
>> > > > + *    0   |   1   |   1   |   0   |   0   |  64 MB        |
>> > > > ALL
>> > > > + *   ...
>> > > > + *  ------|-------|-------|-------|-------|---------------|---
>> > > > ----
>> > > > ------------
>> > > > + *    1   |   0   |   0   |   0   |   0   |   NONE        |
>> > > > NONE
>> > > > + *    1   |   0   |   0   |   0   |   1   |   64 KB       |
>> > > > Lower
>> > > > 1/1024
>> > > > + *    1   |   0   |   0   |   1   |   0   |  128 KB       |
>> > > > Lower
>> > > > 1/512
>> > > > + *    1   |   0   |   0   |   1   |   1   |  256 KB       |
>> > > > Lower
>> > > > 1/256
>> > > > + *   ...
>> > > > + *    1   |   1   |   0   |   0   |   1   |  16 MB        |
>> > > > Lower
>> > > > 1/4
>> > > > + *    1   |   1   |   0   |   1   |   0   |  32 MB        |
>> > > > Lower
>> > > > 1/2
>> > > > + *    1   |   1   |   0   |   1   |   1   |  64 MB        |
>> > > > ALL
>> > > > + *    1   |   1   |   1   |   0   |   0   |  64 MB        |
>> > > > ALL
>> > > > + *   ...
>> > > > + *
>> > > >   * Returns negative on errors, 0 on success.
>> > > >   */
>> > > >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>> > > > len)
>> > > > @@ -1898,6 +1956,12 @@ static int stm_lock(struct spi_nor *nor,
>> > > > loff_t ofs, uint64_t len)
>> > > >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>> > > >  		tb_mask = SR_TB_BIT6;
>> > > >
>> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > > > +			mask = mask | SR_BP3_BIT6;
>> > > > +		else
>> > > > +			mask = mask | SR_BP3_BIT5;
>> > > > +	}
>> > > >  	/*
>> > > >  	 * Need smallest pow such that:
>> > > >  	 *
>> > > > @@ -1908,7 +1972,17 @@ static int stm_lock(struct spi_nor *nor,
>> > > > loff_t ofs, uint64_t len)
>> > > >  	 *   pow = ceil(log2(size / len)) = log2(size) -
>> > > > floor(log2(len))
>> > > >  	 */
>> > > >  	pow = ilog2(mtd->size) - ilog2(lock_len);
>> > > > -	val = mask - (pow << SR_BP_SHIFT);
>> > > > +
>> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > > > +		val = ilog2(nor->n_sectors) + 1 - pow;
>> > >
>> > > Why do you use a new calculation here? As far as I can see, the
>> > > method is
>> > > the same except that is has one bit more. That also raises the
>> > > question why
>> > > n_sectors is now needed?
>> > >
>> > > Can't we just initialize the mask with
>> > >
>> > > mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > > if (nor->flags & SNOR_F_HAS_SR_BP3)
>> > >     mask |= SR_BP3_BIT5;
>> > >
>> > > do the calculation and checks and then move the SR_BP3_BIT5 to
>> > > SR_BP3_BIT6
>> > > if SNOR_F_HAS_SR_BP3_BIT6 is set.
>> > >
>> >
>> > For most of flashes that supporting BP0-2, the smallest protected
>> > portion is fixed as 1/64
>> > and it can be properly handled by existing
>> > calculation. (Actually it's not fully generic, see flashes like
>> > w25q40bw or m25p80. Of course, it doesn't have SPI_NOR_HAS_LOCK
>> > flag
>> > even though it has BP0-2 bit in SR)
>> 
>> No. The rules are always the same wether there are three or four BP
>> bits (the example in stm_lock() has not enough information on this):
>> 
>> (1) the first setting (besides 0) protects one sector. The second
>>      protects 2, the third 4 and so on. eg 2^N
>> (2) the last setting is _always_ protect all, just like the '0'
>> setting
>>      is always protect none.
>> (3) if there is an overflow because there are no more free slots for
>>      further settings (for 3 bits with flashes > 32MBit, for 4
>>      bits if should be flashes > 16GBit), the first entry will be
>>      discarded (eg the very first is the "just one sector" entry).
>> 
>> This is true for all flashes which uses this kind of setting, have a
>> look at the m25p80 or w25q40bw, these are no exception. It is just
>> the
>> notation "lower 1/64" which is only true for flashes which either
>> overflows in (3) or fill all entries (eg. with 3bits that would be
>> the
>> 32Mbit version).
>> 
> 
> Looks like you noticed that we need new calculation method that would
> be based on n_sectors :).

No it will work without that (if I'm not mistaken). Give me some time
and I'll post a patch.

> Rule (1) is NOT true for some flashes
> supporting BP0-2 and that's why I said that smallest protected portion
> is fixed as '1/64' for these flashes.

No, you have to apply rule (3). (1) is only the starting point. It is 
kind
of a sliding window.

> See this one.
> 
> W25Q20EW	256KB	1/4 ... = 64KB		BP2
> W25Q128JV	16MB	1/64 ... = 256KB	BP2 <--
> S25FL132K	4MB	1/64 ... = 64KB		BP2 <--
> S25FL164K	8MB
> 1/64 ... = 128KB	BP2 <--

All these flashes need (3) to be applied, thus (1) doesn't apply
anymore.

Let me give you an example for the 64MBit case, the settings would be:

0 sectors (corresponds to protect none)
1 sector
2 sectors
4 sectors
8 sectors
16 sectors
32 sectors
64 sectors
128 sectors (corresponds to protect all)

Unfortunately, we have only 8 slots (because 3 BP bits), therefore we 
have
to discard some setting. According to rule (2) 0 is always "protect 
none"
and 7 is always "protect all". Thus we have 6 settings left. According 
to
rule (3) we discard the first ones. In this case, this is the "1 sector"
setting. Thus we end up with the following possible settings:

0 sectors (corresponds to protect none)
2 sectors
4 sectors
8 sectors
16 sectors
32 sectors
64 sectors
128 sectors (corresponds to protect all)

If you have a 128Mbit flash, the next setting that would be discarded is
"2 sectors". Thus it would start with:

0 sectors (corresponds to protect none)
4 sectors
[..]
256 sectors (corresponds to protect all)


Another example W25Q20EW, following possible settings:

0 sectors (corresponds to protect none)
1 sector
2 sectors
4 sectors (corresponds to protect all)

We now have less settings then our 8 possible slots. So this is where
rule (1) applies, because according to that the "1 sector" setting is
the first possible setting besides 0.

And this also applies to the 4 bit protection bits.



> W25Q256JV	32MB	1/512 ... =
> 64KB	BP3
> S25FL128L	16MB	1/256 ... = 64KB	BP3
> S25FL256L	32MB	1/512 ... = 64KB	BP3
> 
> In current BP implementation, block protection is just working for some
> flashes that has smallest protected portion as '1/64'.

No its currently working for all except flashes smaller than 32Mbit.
Applied to the 4 bits, this would mean "it works for all except flashes
smaller than 8Gbit" which are practially all. As I said, this is a bug
and once this bug is fixed, there should be no difference between 3
and 4 bits.

-michael

> 
> We need new fomula based on n_sectors for BP3 at least.

No they are the same, but yes there is a bug in the current
implementation.

-michael

> 
>> So for the 3 bit case the following flashes are border cases:
>>   - 16mbit (less settings than slots)
>>   - 32mbit (number of settings and free slots match)
>>   - 64mbit (more settings than slots, first setting is discarded)
>> 
>> That being said, I suspect all the 16mbit flashes (and below) which
>> have
>> the _LOCK bit set are broken, because the entries has to be shifted.
>> I'll
>> look into that later. This is the same "issue" you have with the 4
>> bits.
>> So if this is fixed, you should not need another formula for the 4
>> bit
>> case.
>> 
>> > We need new calculation method for 4bit block protection and for
>> > making
>> > it more generic, I choose n_sectors.
>> >
>> > On all the flashes I checked, n_sectors is proper value for getting
>> > block protected portion.
>> >
>> > 		density	portion	n_sectors
>> > W25M512JV	64MB	1/512	512
>> > N25Q128A	16MB	1/256	256
>> > N25Q512A	64MB	1/1024	1024
>> > MT25QL02GCBB	256MB	1/4096	4096
>> 
>> The rules above apply to these flashes, too. Could you double check
>> that?
>> 
>> -michael
>> 
>> >
>> > > > +		val = val << SR_BP_SHIFT;
>> > > > +
>> > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > > +			val = (val & ~BIT(5)) | BIT(6);
>> > > > +	} else {
>> > > > +		val = mask - (pow << SR_BP_SHIFT);
>> > > > +	}
>> > > > +
>> > > >  	if (val & ~mask)
>> > > >  		return -EINVAL;
>> > > >  	/* Don't "lock" with no region! */
>> > > > @@ -1983,6 +2057,13 @@ static int stm_unlock(struct spi_nor
>> > > > *nor,
>> > > > loff_t ofs, uint64_t len)
>> > > >
>> > > >  	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>> > > >  		tb_mask = SR_TB_BIT6;
>> > > > +
>> > > > +	if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > > > +			mask = mask | SR_BP3_BIT6;
>> > > > +		else
>> > > > +			mask = mask | SR_BP3_BIT5;
>> > > > +	}
>> > > >  	/*
>> > > >  	 * Need largest pow such that:
>> > > >  	 *
>> > > > @@ -1995,13 +2076,20 @@ static int stm_unlock(struct spi_nor
>> > > > *nor,
>> > > > loff_t ofs, uint64_t len)
>> > > >  	pow = ilog2(mtd->size) - order_base_2(lock_len);
>> > > >  	if (lock_len == 0) {
>> > > >  		val = 0; /* fully unlocked */
>> > > > +	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> > > > +		val = ilog2(nor->n_sectors) + 1 - pow;
>> > > > +		val = val << SR_BP_SHIFT;
>> > > > +
>> > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > > +			val = (val & ~BIT(5)) | BIT(6);
>> > > >  	} else {
>> > > >  		val = mask - (pow << SR_BP_SHIFT);
>> > > > -		/* Some power-of-two sizes are not supported */
>> > > > -		if (val & ~mask)
>> > > > -			return -EINVAL;
>> > > >  	}
>> > > >
>> > > > +	/* Some power-of-two sizes are not supported */
>> > > > +	if (val & ~mask)
>> > > > +		return -EINVAL;
>> > > > +
>> > > >  	status_new = (status_old & ~mask & ~tb_mask) | val;
>> > > >
>> > > >  	/* Don't protect status register if we're fully
>> > > > unlocked */
>> > > > @@ -4736,6 +4824,7 @@ static void
>> > > > spi_nor_info_init_params(struct
>> > > > spi_nor *nor)
>> > > >  	/* Set SPI NOR sizes. */
>> > > >  	params->size = (u64)info->sector_size * info-
>> > > > >n_sectors;
>> > > >  	params->page_size = info->page_size;
>> > > > +	params->n_sectors = info->n_sectors;
>> > > >
>> > > >  	if (!(info->flags & SPI_NOR_NO_FR)) {
>> > > >  		/* Default to Fast Read for DT and non-DT
>> > > > platform
>> > > > devices. */
>> > > > @@ -5192,6 +5281,11 @@ 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_HAS_BP3) {
>> > > > +		nor->flags |= SNOR_F_HAS_SR_BP3;
>> > > > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
>> > > > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>> > > > +	}
>> > > >
>> > > >  	if (info->flags & SPI_NOR_NO_ERASE)
>> > > >  		mtd->flags |= MTD_NO_ERASE;
>> > > > @@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor *nor,
>> > > > const
>> > > > char *name,
>> > > >  	mtd->dev.parent = dev;
>> > > >  	nor->page_size = params->page_size;
>> > > >  	mtd->writebufsize = nor->page_size;
>> > > > +	nor->n_sectors = params->n_sectors;
>> > > >
>> > > >  	if (of_property_read_bool(np, "broken-flash-reset"))
>> > > >  		nor->flags |= SNOR_F_BROKEN_RESET;
>> > > > diff --git a/include/linux/mtd/spi-nor.h
>> > > > b/include/linux/mtd/spi-
>> > > > nor.h
>> > > > index 541c06d042e8..92d550501daf 100644
>> > > > --- a/include/linux/mtd/spi-nor.h
>> > > > +++ b/include/linux/mtd/spi-nor.h
>> > > > @@ -129,7 +129,9 @@
>> > > >  #define SR_BP1			BIT(3)	/* Block protect 1
>> > > > */
>> > > >  #define SR_BP2			BIT(4)	/* Block protect 2
>> > > > */
>> > > >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect
>> > > > */
>> > > > +#define SR_BP3_BIT5		BIT(5)	/* Block protect 3
>> > > > */
>> > >
>> > > maybe just name it SR_BP3? would also be more consistent with the
>> > > proposal
>> > > above.
>> > >
>> > > >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect
>> > > > */
>> > > > +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3
>> > > > */
>> > > >  #define SR_SRWD			BIT(7)	/* SR write
>> > > > protect
>> > > > */
>> > > >  /* Spansion/Cypress specific status bits */
>> > > >  #define SR_E_ERR		BIT(5)
>> > > > @@ -248,6 +250,8 @@ enum spi_nor_option_flags {
>> > > >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
>> > > >  	SNOR_F_NO_READ_CR	= BIT(10),
>> > > >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
>> > > > +	SNOR_F_HAS_SR_BP3	= BIT(12),
>> > > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
>> > > >
>> > > >  };
>> > > >
>> > > > @@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
>> > > >   *
>> > > >   * @size:		the flash memory density in bytes.
>> > > >   * @page_size:		the page size of the SPI NOR flash
>> > > > memory.
>> > > > + * @n_sectors:		number of sectors
>> > > >   * @hwcaps:		describes the read and page program
>> > > > hardware
>> > > >   *			capabilities.
>> > > >   * @reads:		read capabilities ordered by priority:
>> > > > the
>> > > > higher index
>> > > > @@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
>> > > >  struct spi_nor_flash_parameter {
>> > > >  	u64				size;
>> > > >  	u32				page_size;
>> > > > +	u16				n_sectors;
>> > > >
>> > > >  	struct spi_nor_hwcaps		hwcaps;
>> > > >  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX
>> > > > ];
>> > > > @@ -573,6 +579,7 @@ struct flash_info;
>> > > >   * @bouncebuf_size:	size of the bounce buffer
>> > > >   * @info:		spi-nor part JDEC MFR id and other info
>> > > >   * @page_size:		the page size of the SPI NOR
>> > > > + * @n_sector:		number of sectors
>> > > >   * @addr_width:		number of address bytes
>> > > >   * @erase_opcode:	the opcode for erasing a sector
>> > > >   * @read_opcode:	the read opcode
>> > > > @@ -599,6 +606,7 @@ struct spi_nor {
>> > > >  	size_t			bouncebuf_size;
>> > > >  	const struct flash_info	*info;
>> > > >  	u32			page_size;
>> > > > +	u16			n_sectors;
>> > > >  	u8			addr_width;
>> > > >  	u8			erase_opcode;
>> > > >  	u8			read_opcode;
>> > > > --
>> > > > 2.17.1
>> > > >
>> > > >
>> > > > ______________________________________________________
>> > > > Linux MTD discussion mailing list
>> > > >
>> >
>> >
> https://protect2.fireeye.com/url?k=06b6dd5d-5b7d5a63-06b75612-0cc47a31309a-83164929001f7741&u=http://lists.infradead.org/mailman/listinfo/linux-mtd/
>> > > >
>> > >
>> > >
>> 
>> 

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

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

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200113055910epcas1p4f97dfeb465b00d66649d6321cffc7b5a@epcas1p4.samsung.com>
2020-01-13  5:59 ` [PATCH v3 1/3] mtd: spi-nor: introduce SR_BP_SHIFT define Jungseung Lee
     [not found]   ` <CGME20200113055910epcas1p377b2618bea2ca860acac2b6f34e2b83e@epcas1p3.samsung.com>
2020-01-13  5:59     ` [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
2020-01-14 10:49       ` Tudor.Ambarus
2020-01-17 15:06         ` Jungseung Lee
2020-01-22 11:42           ` Jungseung Lee
2020-01-22 14:31             ` Tudor.Ambarus
2020-01-22 17:14               ` Michael Walle
2020-01-23  3:59                 ` Jungseung Lee
2020-01-23  8:15                   ` Michael Walle
2020-01-22 19:36       ` Michael Walle
2020-01-23  6:22         ` Jungseung Lee
2020-01-23  8:10           ` Michael Walle
2020-01-23  8:53             ` Jungseung Lee
2020-01-23  9:31               ` Michael Walle
     [not found]   ` <CGME20200113055910epcas1p384c04182e7c643163d659d42fafd01b3@epcas1p3.samsung.com>
2020-01-13  5:59     ` [PATCH v3 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips Jungseung Lee
2020-01-13 12:30       ` John Garry
2020-01-13 12:40         ` Jungseung Lee
2020-01-13 12:45         ` Jungseung Lee
2020-01-13 13:00           ` John Garry

Linux-mtd Archive on lore.kernel.org

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

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

Example config snippet for mirrors

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


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