[PATCH 1/3] mtd: spi-nor: introduce SR_BP_SHIFT define

[PATCH 1/3] mtd: spi-nor: introduce SR_BP_SHIFT define

[Jungseung 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/

[PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Jungseung 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>
---
 drivers/mtd/spi-nor/spi-nor.c | 90 +++++++++++++++++++++++++++++++----
 include/linux/mtd/spi-nor.h   |  8 ++++
 2 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e3da6a8654a8..214f3b733e9b 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;
@@ -1766,24 +1774,48 @@ 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 tb_mask = SR_TB_BIT5, bp;
+	int pow = 0;
 
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
 
-	if (!(sr & mask)) {
+	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;
+	} else {
+		bp = (sr & mask) >> SR_BP_SHIFT;
+	}
+
+	if (!bp) {
 		/* No protection */
 		*ofs = 0;
 		*len = 0;
+		return;
+	}
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		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)
-			*ofs = 0;
-		else
-			*ofs = mtd->size - *len;
+		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;
 }
 
 /*
@@ -1898,6 +1930,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 +1946,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 +2031,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,6 +2050,14 @@ 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) - pow + 1;
+		val = val << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
+		if (val & ~mask)
+			return -EINVAL;
 	} else {
 		val = mask - (pow << SR_BP_SHIFT);
 		/* Some power-of-two sizes are not supported */
@@ -4736,6 +4799,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 +5256,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 +5268,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/

[PATCH 3/3] mtd: spi-nor: support lock/unlock for a few Micron chip s

[Jungseung Lee]

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

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 214f3b733e9b..da285ab56a3d 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2558,12 +2558,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/

[PATCH 1/3] mtd: spi-nor: reimplement block protection handling

[Jungseung Lee]

The current mainline locking was restricted and could only be applied
to flashes that has 3 block protection bit and fixed locking ratio.

A new method of normalization was reached at the end of the discussion [1].

    (1) - if bp slot is insufficient.
    (2) - if bp slot is sufficient.

    if (bp_slots_needed > bp_slots)    // (1)
        min_prot_length = sector_size << (bp_slots_needed - bp_slots);
    else                               // (2)
        min_prot_length = sector_size;

This patch changes block protection handling logic based on min_prot_length.
It is suitable for the overall flashes with exception of some corner case
and easy to extend and apply for the case of 2bit or 4bit block protection.

[1] http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index caf0c109cca0..c58c27552a74 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	return ret;
 }
 
+static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
+{
+	return SR_BP2 | SR_BP1 | SR_BP0;
+}
+
+static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
+{
+	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
+		return SR_TB_BIT6;
+	else
+		return SR_TB_BIT5;
+}
+
+static int stm_get_min_prot_length(struct spi_nor *nor)
+{
+	int bp_slots, bp_slots_needed;
+	u8 mask = spi_nor_get_bp_mask(nor);
+
+	bp_slots = (mask >> SR_BP_SHIFT) + 1;
+
+	/* Reserved one for "protect none" and one for "protect all". */
+	bp_slots = bp_slots - 2;
+
+	bp_slots_needed = ilog2(nor->info->n_sectors);
+
+	if (bp_slots_needed > bp_slots)
+		return nor->info->sector_size <<
+			(bp_slots_needed - bp_slots);
+	else
+		return nor->info->sector_size;
+}
+
 static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 				 uint64_t *len)
 {
 	struct mtd_info *mtd = &nor->mtd;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
-	int pow;
+	int min_prot_len;
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_tb_mask(nor);
+	u8 bp = (sr & mask) >> SR_BP_SHIFT;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
-
-	if (!(sr & mask)) {
+	if (!bp) {
 		/* No protection */
 		*ofs = 0;
 		*len = 0;
-	} else {
-		pow = ((sr & mask) ^ 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;
+		return;
 	}
+
+	min_prot_len = stm_get_min_prot_length(nor);
+	*len = min_prot_len << (bp - 1);
+
+	if (*len > mtd->size)
+		*len = mtd->size;
+
+	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+		*ofs = 0;
+	else
+		*ofs = mtd->size - *len;
 }
 
 /*
@@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
 	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	int min_prot_len;
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_tb_mask(nor);
 	u8 pow, val;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
@@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	else
 		lock_len = ofs + len;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
+	if (lock_len == mtd->size) {
+		val = mask; /* fully locked */
+	} else {
+		min_prot_len = stm_get_min_prot_length(nor);
+		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
+		val = pow << SR_BP_SHIFT;
+	}
 
-	/*
-	 * Need smallest pow such that:
-	 *
-	 *   1 / (2^pow) <= (len / size)
-	 *
-	 * so (assuming power-of-2 size) we do:
-	 *
-	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
-	 */
-	pow = ilog2(mtd->size) - ilog2(lock_len);
-	val = mask - (pow << SR_BP_SHIFT);
 	if (val & ~mask)
 		return -EINVAL;
 	/* Don't "lock" with no region! */
@@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
 	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	int min_prot_len;
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_tb_mask(nor);
 	u8 pow, val;
 	loff_t lock_len;
 	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
@@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	else
 		lock_len = ofs;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
-	/*
-	 * Need largest pow such that:
-	 *
-	 *   1 / (2^pow) >= (len / size)
-	 *
-	 * so (assuming power-of-2 size) we do:
-	 *
-	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len))
-	 */
-	pow = ilog2(mtd->size) - order_base_2(lock_len);
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
-		val = mask - (pow << SR_BP_SHIFT);
+		min_prot_len = stm_get_min_prot_length(nor);
+		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
+		val = pow << SR_BP_SHIFT;
+
 		/* Some power-of-two sizes are not supported */
 		if (val & ~mask)
 			return -EINVAL;
-- 
2.17.1


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

[PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Jungseung 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 flashes with
4 block protection bits(bp0-3).

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c58c27552a74..31a2106e529a 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_4BIT_BP		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_4BIT_BP.
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
 {
-	return SR_BP2 | SR_BP1 | SR_BP0;
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			mask = mask | SR_BP3_BIT6;
+		else
+			mask = mask | SR_BP3_BIT5;
+	}
+
+	return mask;
 }
 
 static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
@@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
 		return SR_TB_BIT5;
 }
 
+static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 bp = sr & mask;
+
+	if (bp & SR_BP3_BIT6)
+		bp = (bp & ~BIT(6)) | BIT(5);
+
+	bp = bp >> SR_BP_SHIFT;
+
+	return bp;
+}
+
 static int stm_get_min_prot_length(struct spi_nor *nor)
 {
 	int bp_slots, bp_slots_needed;
-	u8 mask = spi_nor_get_bp_mask(nor);
 
-	bp_slots = (mask >> SR_BP_SHIFT) + 1;
+	if (nor->flags & SNOR_F_HAS_4BIT_BP)
+		bp_slots = 1 << 4;
+	else
+		bp_slots = 1 << 3;
 
 	/* Reserved one for "protect none" and one for "protect all". */
 	bp_slots = bp_slots - 2;
@@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 {
 	struct mtd_info *mtd = &nor->mtd;
 	int min_prot_len;
-	u8 mask = spi_nor_get_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_tb_mask(nor);
-	u8 bp = (sr & mask) >> SR_BP_SHIFT;
+	u8 bp = stm_get_bpval_from_sr(nor, sr);
 
 	if (!bp) {
 		/* No protection */
@@ -1881,7 +1911,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}/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)
@@ -1889,7 +1919,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
  *  --------------------------------------------------------------------------
@@ -1909,6 +1939,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)
@@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 		min_prot_len = stm_get_min_prot_length(nor);
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 		val = pow << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
 	}
 
 	if (val & ~mask)
@@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 		val = pow << SR_BP_SHIFT;
 
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
+
 		/* Some power-of-two sizes are not supported */
 		if (val & ~mask)
 			return -EINVAL;
@@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (info->flags & USE_CLSR)
 		nor->flags |= SNOR_F_USE_CLSR;
 
+	if (info->flags & SPI_NOR_4BIT_BP) {
+		nor->flags |= SNOR_F_HAS_4BIT_BP;
+		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;
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index de90724f62f1..0190ed21576a 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)
@@ -240,6 +242,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_4BIT_BP	= BIT(12),
+	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
 
 };
 
-- 
2.17.1


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

Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Michael Walle]

Hi Jungseung,

sorry for the late review.

Am 2020-03-04 12:07, schrieb Jungseung 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 flashes with
> 4 block protection bits(bp0-3).
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 82 ++++++++++++++++++++++++++++++++---
>  include/linux/mtd/spi-nor.h   |  4 ++
>  2 files changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> b/drivers/mtd/spi-nor/spi-nor.c
> index c58c27552a74..31a2106e529a 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_4BIT_BP		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_4BIT_BP.
> +					 */
> 
>  	/* Part specific fixup hooks. */
>  	const struct spi_nor_fixups *fixups;
> @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd,
> struct erase_info *instr)
> 
>  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>  {
> -	return SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +

can we just use the SR_BP3 eg:

	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
	if (nor->flags & SNOR_F_HAS_4BIT_BP)
		mask |= SR_BP3;
	return mask;


> +	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> +			mask = mask | SR_BP3_BIT6;
> +		else
> +			mask = mask | SR_BP3_BIT5;
> +	}
> +
> +	return mask;
>  }
> 
>  static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor 
> *nor)
>  		return SR_TB_BIT5;
>  }
> 
> +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> +	u8 mask = spi_nor_get_bp_mask(nor);
> +	u8 bp = sr & mask;
> +
> +	if (bp & SR_BP3_BIT6)
> +		bp = (bp & ~BIT(6)) | BIT(5);
> +
> +	bp = bp >> SR_BP_SHIFT;
> +
> +	return bp;
> +}

Don't convert this. It makes the code really hard to read. See below.

> +
>  static int stm_get_min_prot_length(struct spi_nor *nor)
>  {
>  	int bp_slots, bp_slots_needed;
> -	u8 mask = spi_nor_get_bp_mask(nor);
> 
> -	bp_slots = (mask >> SR_BP_SHIFT) + 1;

Then just keep this.

> +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> +		bp_slots = 1 << 4;
> +	else
> +		bp_slots = 1 << 3;
> 
>  	/* Reserved one for "protect none" and one for "protect all". */
>  	bp_slots = bp_slots - 2;
> @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor
> *nor, u8 sr, loff_t *ofs,
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  	int min_prot_len;
> -	u8 mask = spi_nor_get_bp_mask(nor);
>  	u8 tb_mask = spi_nor_get_tb_mask(nor);
> -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> +	u8 bp = stm_get_bpval_from_sr(nor, sr);

also this.

> 
>  	if (!bp) {
>  		/* No protection */
> @@ -1881,7 +1911,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}/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)
> @@ -1889,7 +1919,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
>   *  
> --------------------------------------------------------------------------
> @@ -1909,6 +1939,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)
> @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t
> ofs, uint64_t len)
>  		min_prot_len = stm_get_min_prot_length(nor);
>  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>  		val = pow << SR_BP_SHIFT;
> +
> +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> +			val = (val & ~BIT(5)) | BIT(6);

.. and the use just the following here:

if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
	val = (val & ~SR_BP3) | SR_BP3_BIT6;

Ie. just use the "normal case" where all BP bits are next to each other
and then fixup the resulting value and shift the SR3 bit if necessary.
This will be much easier to read.

>  	}
> 
>  	if (val & ~mask)
> @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> loff_t ofs, uint64_t len)
>  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>  		val = pow << SR_BP_SHIFT;
> 
> +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> +			val = (val & ~BIT(5)) | BIT(6);
> +

here would be the other way around:

if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
	val = (val & ~SR_BP3_BIT6) | SR_BP3;


>  		/* Some power-of-two sizes are not supported */
>  		if (val & ~mask)
>  			return -EINVAL;
> @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char 
> *name,
>  	if (info->flags & USE_CLSR)
>  		nor->flags |= SNOR_F_USE_CLSR;
> 
> +	if (info->flags & SPI_NOR_4BIT_BP) {
> +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> +		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;
> 
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de90724f62f1..0190ed21576a 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 */

IMHO just SR_BP3. but that is a matter of taste. But it is easier on
the eye in the mask = SR_BP3 | SR_BP2 etc case.

-michael

>  #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)
> @@ -240,6 +242,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_4BIT_BP	= BIT(12),
> +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> 
>  };

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

Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Jungseung Lee]

Hi, Michael,

On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
> Hi Jungseung,
> 
> sorry for the late review.
> 

Not at all. thanks for your review.

> Am 2020-03-04 12:07, schrieb Jungseung 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 flashes with
> > 4 block protection bits(bp0-3).
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 82
> > ++++++++++++++++++++++++++++++++---
> >  include/linux/mtd/spi-nor.h   |  4 ++
> >  2 files changed, 79 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> > b/drivers/mtd/spi-nor/spi-nor.c
> > index c58c27552a74..31a2106e529a 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_4BIT_BP		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_4BIT_BP.
> > +					 */
> > 
> >  	/* Part specific fixup hooks. */
> >  	const struct spi_nor_fixups *fixups;
> > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
> > *mtd,
> > struct erase_info *instr)
> > 
> >  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> >  {
> > -	return SR_BP2 | SR_BP1 | SR_BP0;
> > +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > +
> 
> can we just use the SR_BP3 eg:
> 
> 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> 		mask |= SR_BP3;
> 	return mask;
> 
> 

I'd prefer this one too if we can, but there are some places to need
the real mask value. It is also used in other places such as
spi_nor_sr_lock/unlock.

> > +	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > +			mask = mask | SR_BP3_BIT6;
> > +		else
> > +			mask = mask | SR_BP3_BIT5;
> > +	}
> > +
> > +	return mask;
> >  }
> > 
> >  static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > spi_nor 
> > *nor)
> >  		return SR_TB_BIT5;
> >  }
> > 
> > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> > +	u8 mask = spi_nor_get_bp_mask(nor);
> > +	u8 bp = sr & mask;
> > +
> > +	if (bp & SR_BP3_BIT6)
> > +		bp = (bp & ~BIT(6)) | BIT(5);
> > +
> > +	bp = bp >> SR_BP_SHIFT;
> > +
> > +	return bp;
> > +}
> 
> Don't convert this. It makes the code really hard to read. See below.
> 
> > +
> >  static int stm_get_min_prot_length(struct spi_nor *nor)
> >  {
> >  	int bp_slots, bp_slots_needed;
> > -	u8 mask = spi_nor_get_bp_mask(nor);
> > 
> > -	bp_slots = (mask >> SR_BP_SHIFT) + 1;
> 
> Then just keep this.
> 
> > +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > +		bp_slots = 1 << 4;
> > +	else
> > +		bp_slots = 1 << 3;
> > 
> >  	/* Reserved one for "protect none" and one for "protect all".
> > */
> >  	bp_slots = bp_slots - 2;
> > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > spi_nor
> > *nor, u8 sr, loff_t *ofs,
> >  {
> >  	struct mtd_info *mtd = &nor->mtd;
> >  	int min_prot_len;
> > -	u8 mask = spi_nor_get_bp_mask(nor);
> >  	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > +	u8 bp = stm_get_bpval_from_sr(nor, sr);
> 
> also this.
> 
> > 
> >  	if (!bp) {
> >  		/* No protection */
> > @@ -1881,7 +1911,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}/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)
> > @@ -1889,7 +1919,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
> >   *  
> > -----------------------------------------------------------------
> > ---------
> > @@ -1909,6 +1939,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)
> > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
> > loff_t
> > ofs, uint64_t len)
> >  		min_prot_len = stm_get_min_prot_length(nor);
> >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> >  		val = pow << SR_BP_SHIFT;
> > +
> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > +			val = (val & ~BIT(5)) | BIT(6);
> 
> .. and the use just the following here:
> 
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
> 
> Ie. just use the "normal case" where all BP bits are next to each
> other
> and then fixup the resulting value and shift the SR3 bit if
> necessary.
> This will be much easier to read.
> 

Yes, I agree. It would be better to minimize this kind of conversion in
one place.

> >  	}
> > 
> >  	if (val & ~mask)
> > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> > loff_t ofs, uint64_t len)
> >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> >  		val = pow << SR_BP_SHIFT;
> > 
> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > +			val = (val & ~BIT(5)) | BIT(6);
> > +
> 
> here would be the other way around:
> 
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
> 
> 
> >  		/* Some power-of-two sizes are not supported */
> >  		if (val & ~mask)
> >  			return -EINVAL;
> > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const
> > char 
> > *name,
> >  	if (info->flags & USE_CLSR)
> >  		nor->flags |= SNOR_F_USE_CLSR;
> > 
> > +	if (info->flags & SPI_NOR_4BIT_BP) {
> > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> > +		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;
> > 
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> > nor.h
> > index de90724f62f1..0190ed21576a 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
> > */
> 
> IMHO just SR_BP3. but that is a matter of taste. But it is easier on
> the eye in the mask = SR_BP3 | SR_BP2 etc case.
> 

SR_BP3 would be a more appropriate name if we could set the case with
all BP bits next to each other as the "normal case."

I'm going to write patches based on latest spi-nor/next including what
you mentioned.

Thanks,

> -michael
> 
> >  #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)
> > @@ -240,6 +242,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_4BIT_BP	= BIT(12),
> > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > 
> >  };
> 


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

Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Jungseung Lee]

Hi, Micahel

On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote:
> Hi, Michael,
> 
> On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
> > Hi Jungseung,
> > 
> > sorry for the late review.
> > 
> 
> Not at all. thanks for your review.
> 
> > Am 2020-03-04 12:07, schrieb Jungseung 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 flashes with
> > > 4 block protection bits(bp0-3).
> > > 
> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > ---
> > >  drivers/mtd/spi-nor/spi-nor.c | 82
> > > ++++++++++++++++++++++++++++++++---
> > >  include/linux/mtd/spi-nor.h   |  4 ++
> > >  2 files changed, 79 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> > > b/drivers/mtd/spi-nor/spi-nor.c
> > > index c58c27552a74..31a2106e529a 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_4BIT_BP		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_4BIT_BP.
> > > +					 */
> > > 
> > >  	/* Part specific fixup hooks. */
> > >  	const struct spi_nor_fixups *fixups;
> > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
> > > *mtd,
> > > struct erase_info *instr)
> > > 
> > >  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > >  {
> > > -	return SR_BP2 | SR_BP1 | SR_BP0;
> > > +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > +
> > 
> > can we just use the SR_BP3 eg:
> > 
> > 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > 		mask |= SR_BP3;
> > 	return mask;
> > 
> > 
> 
> I'd prefer this one too if we can, but there are some places to need
> the real mask value. It is also used in other places such as
> spi_nor_sr_lock/unlock.
> 
> > > +	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > +			mask = mask | SR_BP3_BIT6;
> > > +		else
> > > +			mask = mask | SR_BP3_BIT5;
> > > +	}
> > > +
> > > +	return mask;
> > >  }
> > > 
> > >  static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > > spi_nor 
> > > *nor)
> > >  		return SR_TB_BIT5;
> > >  }
> > > 
> > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > +	u8 bp = sr & mask;
> > > +
> > > +	if (bp & SR_BP3_BIT6)
> > > +		bp = (bp & ~BIT(6)) | BIT(5);
> > > +
> > > +	bp = bp >> SR_BP_SHIFT;
> > > +
> > > +	return bp;
> > > +}
> > 
> > Don't convert this. It makes the code really hard to read. See
> > below.
> > 
> > > +
> > >  static int stm_get_min_prot_length(struct spi_nor *nor)
> > >  {
> > >  	int bp_slots, bp_slots_needed;
> > > -	u8 mask = spi_nor_get_bp_mask(nor);
> > > 
> > > -	bp_slots = (mask >> SR_BP_SHIFT) + 1;
> > 
> > Then just keep this.
> > 
> > > +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > +		bp_slots = 1 << 4;
> > > +	else
> > > +		bp_slots = 1 << 3;
> > > 
> > >  	/* Reserved one for "protect none" and one for "protect all".
> > > */
> > >  	bp_slots = bp_slots - 2;
> > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > > spi_nor
> > > *nor, u8 sr, loff_t *ofs,
> > >  {
> > >  	struct mtd_info *mtd = &nor->mtd;
> > >  	int min_prot_len;
> > > -	u8 mask = spi_nor_get_bp_mask(nor);
> > >  	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > +	u8 bp = stm_get_bpval_from_sr(nor, sr);
> > 
> > also this.
> > 

hmm, it looks like we still need some convertion here to get the exact
bp value.

Thanks,

> > > 
> > >  	if (!bp) {
> > >  		/* No protection */
> > > @@ -1881,7 +1911,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}/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)
> > > @@ -1889,7 +1919,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
> > >   *  
> > > -----------------------------------------------------------------
> > > ---------
> > > @@ -1909,6 +1939,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)
> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
> > > loff_t
> > > ofs, uint64_t len)
> > >  		min_prot_len = stm_get_min_prot_length(nor);
> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > >  		val = pow << SR_BP_SHIFT;
> > > +
> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > +			val = (val & ~BIT(5)) | BIT(6);
> > 
> > .. and the use just the following here:
> > 
> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> > 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
> > 
> > Ie. just use the "normal case" where all BP bits are next to each
> > other
> > and then fixup the resulting value and shift the SR3 bit if
> > necessary.
> > This will be much easier to read.
> > 
> 
> Yes, I agree. It would be better to minimize this kind of conversion
> in
> one place.
> 
> > >  	}
> > > 
> > >  	if (val & ~mask)
> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> > > loff_t ofs, uint64_t len)
> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > >  		val = pow << SR_BP_SHIFT;
> > > 
> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > +			val = (val & ~BIT(5)) | BIT(6);
> > > +
> > 
> > here would be the other way around:
> > 
> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> > 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
> > 
> > 
> > >  		/* Some power-of-two sizes are not supported */
> > >  		if (val & ~mask)
> > >  			return -EINVAL;
> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
> > > const
> > > char 
> > > *name,
> > >  	if (info->flags & USE_CLSR)
> > >  		nor->flags |= SNOR_F_USE_CLSR;
> > > 
> > > +	if (info->flags & SPI_NOR_4BIT_BP) {
> > > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> > > +		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;
> > > 
> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> > > nor.h
> > > index de90724f62f1..0190ed21576a 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
> > > */
> > 
> > IMHO just SR_BP3. but that is a matter of taste. But it is easier
> > on
> > the eye in the mask = SR_BP3 | SR_BP2 etc case.
> > 
> 
> SR_BP3 would be a more appropriate name if we could set the case with
> all BP bits next to each other as the "normal case."
> 
> I'm going to write patches based on latest spi-nor/next including
> what
> you mentioned.
> 
> Thanks,
> 
> > -michael
> > 
> > >  #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)
> > > @@ -240,6 +242,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_4BIT_BP	= BIT(12),
> > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > > 
> > >  };
> 
> 


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

Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Michael Walle]

Am 2020-03-17 12:35, schrieb Jungseung Lee:
> Hi, Micahel
> 
> On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote:
>> Hi, Michael,
>> 
>> On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
>> > Hi Jungseung,
>> >
>> > sorry for the late review.
>> >
>> 
>> Not at all. thanks for your review.
>> 
>> > Am 2020-03-04 12:07, schrieb Jungseung 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 flashes with
>> > > 4 block protection bits(bp0-3).
>> > >
>> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>> > > ---
>> > >  drivers/mtd/spi-nor/spi-nor.c | 82
>> > > ++++++++++++++++++++++++++++++++---
>> > >  include/linux/mtd/spi-nor.h   |  4 ++
>> > >  2 files changed, 79 insertions(+), 7 deletions(-)
>> > >
>> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
>> > > b/drivers/mtd/spi-nor/spi-nor.c
>> > > index c58c27552a74..31a2106e529a 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_4BIT_BP		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_4BIT_BP.
>> > > +					 */
>> > >
>> > >  	/* Part specific fixup hooks. */
>> > >  	const struct spi_nor_fixups *fixups;
>> > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
>> > > *mtd,
>> > > struct erase_info *instr)
>> > >
>> > >  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>> > >  {
>> > > -	return SR_BP2 | SR_BP1 | SR_BP0;
>> > > +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > > +
>> >
>> > can we just use the SR_BP3 eg:
>> >
>> > 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
>> > 		mask |= SR_BP3;
>> > 	return mask;
>> >
>> >
>> 
>> I'd prefer this one too if we can, but there are some places to need
>> the real mask value. It is also used in other places such as
>> spi_nor_sr_lock/unlock.
>> 
>> > > +	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
>> > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > > +			mask = mask | SR_BP3_BIT6;
>> > > +		else
>> > > +			mask = mask | SR_BP3_BIT5;
>> > > +	}
>> > > +
>> > > +	return mask;
>> > >  }
>> > >
>> > >  static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
>> > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
>> > > spi_nor
>> > > *nor)
>> > >  		return SR_TB_BIT5;
>> > >  }
>> > >
>> > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
>> > > +	u8 mask = spi_nor_get_bp_mask(nor);
>> > > +	u8 bp = sr & mask;
>> > > +
>> > > +	if (bp & SR_BP3_BIT6)
>> > > +		bp = (bp & ~BIT(6)) | BIT(5);
>> > > +
>> > > +	bp = bp >> SR_BP_SHIFT;
>> > > +
>> > > +	return bp;
>> > > +}
>> >
>> > Don't convert this. It makes the code really hard to read. See
>> > below.
>> >
>> > > +
>> > >  static int stm_get_min_prot_length(struct spi_nor *nor)
>> > >  {
>> > >  	int bp_slots, bp_slots_needed;
>> > > -	u8 mask = spi_nor_get_bp_mask(nor);
>> > >
>> > > -	bp_slots = (mask >> SR_BP_SHIFT) + 1;
>> >
>> > Then just keep this.
>> >
>> > > +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
>> > > +		bp_slots = 1 << 4;
>> > > +	else
>> > > +		bp_slots = 1 << 3;
>> > >
>> > >  	/* Reserved one for "protect none" and one for "protect all".
>> > > */
>> > >  	bp_slots = bp_slots - 2;
>> > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
>> > > spi_nor
>> > > *nor, u8 sr, loff_t *ofs,
>> > >  {
>> > >  	struct mtd_info *mtd = &nor->mtd;
>> > >  	int min_prot_len;
>> > > -	u8 mask = spi_nor_get_bp_mask(nor);
>> > >  	u8 tb_mask = spi_nor_get_tb_mask(nor);
>> > > -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
>> > > +	u8 bp = stm_get_bpval_from_sr(nor, sr);
>> >
>> > also this.
>> >
> 
> hmm, it looks like we still need some convertion here to get the exact
> bp value.

OK I see. What has confused me before was that some "fixups" were done
in helper functions whereas others where done in the actual
stm_lock/stm_unlock().

What do you think about:

(1) read the BP bits, if they are not consecutive move them around and
     normalize the value, eg. fix them up to be
         SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0  >> SR_BP_SHIFT
(2) do the operations on these bits, eg this should not shift the
     value by SR_BP_SHIFT anymore. This would be done in (3)
(3) convert it back to the representation the flash would need it.

So there could be a function spi_nor_get_bp_val() for (1) and
spi_nor_set_bp_val() for (3).

u8 spi_nor_get_bp_val(u8 sr)
{
     u8 val;

     val = sr & (SR_BP2 | SR_BP1 | SR_BP0);

     if (nor->flags & SNOR_F_HAS_4BIT_BP)
         if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && sr & SR_BP3_BIT6)
             val |= SR_BP3;
         else
             val |= sr & SR_BP3

     return val >> SR_BP_SHIFT;
}

void spi_nor_set_bp_val(u8 val, u8 *sr)
{
     u8 bp3_bit;

     val <<= SR_BP_SHIFT;

     /* clear and set common bits */
     *sr &= ~(SR_BP2 | SR_BP1 | SR_BP0);
     *sr |= val & (SR_BP2 | SR_BP1 | SR_BP0)

     /* BP3 is special because it may be on different bits */
     if (nor->flags & SNOR_F_HAS_4BIT_BP) {
         if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
             bp3_bit = SR_BP3_BIT6;
         else
             bp3_bit = SR_BP3;

         if (val & SR_BP3)
             *sr |= bp3_bit;
         else
             *sr &= ~bp3_bit;
     }
}

This way we'd have all the fixups in one place.


-michael

>> > 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
>> > 		mask |= SR_BP3;
>> > 	return mask;


}


-michael

> 
> Thanks,
> 
>> > >
>> > >  	if (!bp) {
>> > >  		/* No protection */
>> > > @@ -1881,7 +1911,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}/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)
>> > > @@ -1889,7 +1919,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
>> > >   *
>> > > -----------------------------------------------------------------
>> > > ---------
>> > > @@ -1909,6 +1939,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)
>> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
>> > > loff_t
>> > > ofs, uint64_t len)
>> > >  		min_prot_len = stm_get_min_prot_length(nor);
>> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>> > >  		val = pow << SR_BP_SHIFT;
>> > > +
>> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > +			val = (val & ~BIT(5)) | BIT(6);
>> >
>> > .. and the use just the following here:
>> >
>> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
>> > 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
>> >
>> > Ie. just use the "normal case" where all BP bits are next to each
>> > other
>> > and then fixup the resulting value and shift the SR3 bit if
>> > necessary.
>> > This will be much easier to read.
>> >
>> 
>> Yes, I agree. It would be better to minimize this kind of conversion
>> in
>> one place.
>> 
>> > >  	}
>> > >
>> > >  	if (val & ~mask)
>> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
>> > > loff_t ofs, uint64_t len)
>> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>> > >  		val = pow << SR_BP_SHIFT;
>> > >
>> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > +			val = (val & ~BIT(5)) | BIT(6);
>> > > +
>> >
>> > here would be the other way around:
>> >
>> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
>> > 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
>> >
>> >
>> > >  		/* Some power-of-two sizes are not supported */
>> > >  		if (val & ~mask)
>> > >  			return -EINVAL;
>> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
>> > > const
>> > > char
>> > > *name,
>> > >  	if (info->flags & USE_CLSR)
>> > >  		nor->flags |= SNOR_F_USE_CLSR;
>> > >
>> > > +	if (info->flags & SPI_NOR_4BIT_BP) {
>> > > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
>> > > +		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;
>> > >
>> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
>> > > nor.h
>> > > index de90724f62f1..0190ed21576a 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
>> > > */
>> >
>> > IMHO just SR_BP3. but that is a matter of taste. But it is easier
>> > on
>> > the eye in the mask = SR_BP3 | SR_BP2 etc case.
>> >
>> 
>> SR_BP3 would be a more appropriate name if we could set the case with
>> all BP bits next to each other as the "normal case."
>> 
>> I'm going to write patches based on latest spi-nor/next including
>> what
>> you mentioned.
>> 
>> Thanks,
>> 
>> > -michael
>> >
>> > >  #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)
>> > > @@ -240,6 +242,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_4BIT_BP	= BIT(12),
>> > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
>> > >
>> > >  };
>> 
>> 

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

Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support

[Jungseung Lee]

Hi,

On Tue, 2020-03-17 at 15:52 +0100, Michael Walle wrote:
> Am 2020-03-17 12:35, schrieb Jungseung Lee:
> > Hi, Micahel
> > 
> > On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote:
> > > Hi, Michael,
> > > 
> > > On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
> > > > Hi Jungseung,
> > > > 
> > > > sorry for the late review.
> > > > 
> > > 
> > > Not at all. thanks for your review.
> > > 
> > > > Am 2020-03-04 12:07, schrieb Jungseung 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 flashes with
> > > > > 4 block protection bits(bp0-3).
> > > > > 
> > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > > > ---
> > > > >  drivers/mtd/spi-nor/spi-nor.c | 82
> > > > > ++++++++++++++++++++++++++++++++---
> > > > >  include/linux/mtd/spi-nor.h   |  4 ++
> > > > >  2 files changed, 79 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > > b/drivers/mtd/spi-nor/spi-nor.c
> > > > > index c58c27552a74..31a2106e529a 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_4BIT_BP		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_4BIT_BP.
> > > > > +					 */
> > > > > 
> > > > >  	/* Part specific fixup hooks. */
> > > > >  	const struct spi_nor_fixups *fixups;
> > > > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct
> > > > > mtd_info
> > > > > *mtd,
> > > > > struct erase_info *instr)
> > > > > 
> > > > >  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > > > >  {
> > > > > -	return SR_BP2 | SR_BP1 | SR_BP0;
> > > > > +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > +
> > > > 
> > > > can we just use the SR_BP3 eg:
> > > > 
> > > > 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > 		mask |= SR_BP3;
> > > > 	return mask;
> > > > 
> > > > 
> > > 
> > > I'd prefer this one too if we can, but there are some places to
> > > need
> > > the real mask value. It is also used in other places such as
> > > spi_nor_sr_lock/unlock.
> > > 
> > > > > +	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > > > > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > > > +			mask = mask | SR_BP3_BIT6;
> > > > > +		else
> > > > > +			mask = mask | SR_BP3_BIT5;
> > > > > +	}
> > > > > +
> > > > > +	return mask;
> > > > >  }
> > > > > 
> > > > >  static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > > > > spi_nor
> > > > > *nor)
> > > > >  		return SR_TB_BIT5;
> > > > >  }
> > > > > 
> > > > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr)
> > > > > {
> > > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > > +	u8 bp = sr & mask;
> > > > > +
> > > > > +	if (bp & SR_BP3_BIT6)
> > > > > +		bp = (bp & ~BIT(6)) | BIT(5);
> > > > > +
> > > > > +	bp = bp >> SR_BP_SHIFT;
> > > > > +
> > > > > +	return bp;
> > > > > +}
> > > > 
> > > > Don't convert this. It makes the code really hard to read. See
> > > > below.
> > > > 
> > > > > +
> > > > >  static int stm_get_min_prot_length(struct spi_nor *nor)
> > > > >  {
> > > > >  	int bp_slots, bp_slots_needed;
> > > > > -	u8 mask = spi_nor_get_bp_mask(nor);
> > > > > 
> > > > > -	bp_slots = (mask >> SR_BP_SHIFT) + 1;
> > > > 
> > > > Then just keep this.
> > > > 
> > > > > +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > > +		bp_slots = 1 << 4;
> > > > > +	else
> > > > > +		bp_slots = 1 << 3;
> > > > > 
> > > > >  	/* Reserved one for "protect none" and one for "protect
> > > > > all".
> > > > > */
> > > > >  	bp_slots = bp_slots - 2;
> > > > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > > > > spi_nor
> > > > > *nor, u8 sr, loff_t *ofs,
> > > > >  {
> > > > >  	struct mtd_info *mtd = &nor->mtd;
> > > > >  	int min_prot_len;
> > > > > -	u8 mask = spi_nor_get_bp_mask(nor);
> > > > >  	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > > -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > > > +	u8 bp = stm_get_bpval_from_sr(nor, sr);
> > > > 
> > > > also this.
> > > > 
> > 
> > hmm, it looks like we still need some convertion here to get the
> > exact
> > bp value.
> 
> OK I see. What has confused me before was that some "fixups" were
> done
> in helper functions whereas others where done in the actual
> stm_lock/stm_unlock().
> 
> What do you think about:
> 
> (1) read the BP bits, if they are not consecutive move them around
> and
>      normalize the value, eg. fix them up to be
>          SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0  >> SR_BP_SHIFT
> (2) do the operations on these bits, eg this should not shift the
>      value by SR_BP_SHIFT anymore. This would be done in (3)
> (3) convert it back to the representation the flash would need it.
> 
> So there could be a function spi_nor_get_bp_val() for (1) and
> spi_nor_set_bp_val() for (3).
> 
> u8 spi_nor_get_bp_val(u8 sr)
> {
>      u8 val;
> 
>      val = sr & (SR_BP2 | SR_BP1 | SR_BP0);
> 
>      if (nor->flags & SNOR_F_HAS_4BIT_BP)
>          if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && sr & SR_BP3_BIT6)
>              val |= SR_BP3;
>          else
>              val |= sr & SR_BP3
> 
>      return val >> SR_BP_SHIFT;
> }
> 
> void spi_nor_set_bp_val(u8 val, u8 *sr)
> {
>      u8 bp3_bit;
> 
>      val <<= SR_BP_SHIFT;
> 
>      /* clear and set common bits */
>      *sr &= ~(SR_BP2 | SR_BP1 | SR_BP0);
>      *sr |= val & (SR_BP2 | SR_BP1 | SR_BP0)
> 
>      /* BP3 is special because it may be on different bits */
>      if (nor->flags & SNOR_F_HAS_4BIT_BP) {
>          if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>              bp3_bit = SR_BP3_BIT6;
>          else
>              bp3_bit = SR_BP3;
> 
>          if (val & SR_BP3)
>              *sr |= bp3_bit;
>          else
>              *sr &= ~bp3_bit;
>      }
> }
> 
> This way we'd have all the fixups in one place.
> 

It's good suggestion, but if there are no additional use cases, it
would be good idea to place them within without using helper function.

I'm going to post new patch soon.

Thanks,

> 
> -michael
> 
> > > > 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > 		mask |= SR_BP3;
> > > > 	return mask;
> 
> 
> }
> 
> 
> -michael
> 
> > 
> > Thanks,
> > 
> > > > > 
> > > > >  	if (!bp) {
> > > > >  		/* No protection */
> > > > > @@ -1881,7 +1911,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}/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)
> > > > > @@ -1889,7 +1919,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
> > > > >   *
> > > > > -----------------------------------------------------------
> > > > > ------
> > > > > ---------
> > > > > @@ -1909,6 +1939,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)
> > > > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor
> > > > > *nor,
> > > > > loff_t
> > > > > ofs, uint64_t len)
> > > > >  		min_prot_len = stm_get_min_prot_length(nor);
> > > > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > 1;
> > > > >  		val = pow << SR_BP_SHIFT;
> > > > > +
> > > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > > +			val = (val & ~BIT(5)) | BIT(6);
> > > > 
> > > > .. and the use just the following here:
> > > > 
> > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> > > > 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
> > > > 
> > > > Ie. just use the "normal case" where all BP bits are next to
> > > > each
> > > > other
> > > > and then fixup the resulting value and shift the SR3 bit if
> > > > necessary.
> > > > This will be much easier to read.
> > > > 
> > > 
> > > Yes, I agree. It would be better to minimize this kind of
> > > conversion
> > > in
> > > one place.
> > > 
> > > > >  	}
> > > > > 
> > > > >  	if (val & ~mask)
> > > > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor
> > > > > *nor,
> > > > > loff_t ofs, uint64_t len)
> > > > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > 1;
> > > > >  		val = pow << SR_BP_SHIFT;
> > > > > 
> > > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > > +			val = (val & ~BIT(5)) | BIT(6);
> > > > > +
> > > > 
> > > > here would be the other way around:
> > > > 
> > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> > > > 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
> > > > 
> > > > 
> > > > >  		/* Some power-of-two sizes are not supported */
> > > > >  		if (val & ~mask)
> > > > >  			return -EINVAL;
> > > > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
> > > > > const
> > > > > char
> > > > > *name,
> > > > >  	if (info->flags & USE_CLSR)
> > > > >  		nor->flags |= SNOR_F_USE_CLSR;
> > > > > 
> > > > > +	if (info->flags & SPI_NOR_4BIT_BP) {
> > > > > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> > > > > +		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;
> > > > > 
> > > > > diff --git a/include/linux/mtd/spi-nor.h
> > > > > b/include/linux/mtd/spi-
> > > > > nor.h
> > > > > index de90724f62f1..0190ed21576a 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
> > > > > */
> > > > 
> > > > IMHO just SR_BP3. but that is a matter of taste. But it is
> > > > easier
> > > > on
> > > > the eye in the mask = SR_BP3 | SR_BP2 etc case.
> > > > 
> > > 
> > > SR_BP3 would be a more appropriate name if we could set the case
> > > with
> > > all BP bits next to each other as the "normal case."
> > > 
> > > I'm going to write patches based on latest spi-nor/next including
> > > what
> > > you mentioned.
> > > 
> > > Thanks,
> > > 
> > > > -michael
> > > > 
> > > > >  #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)
> > > > > @@ -240,6 +242,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_4BIT_BP	= BIT(12),
> > > > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > > > > 
> > > > >  };
> > > 
> > > 
> 
> 


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