[PATCH v3 0/5] mtd: spi-nor: Add SR 4bit block protection support

[PATCH v3 0/5] mtd: spi-nor: Add SR 4bit block protection support

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Hi, Jungseung, Michael, Vignesh,

I took Jungseung's v2 and amend it here and there, the description
of the changes are in each patch. You'll notice that there are 2 new
small patches, the first one is needed so that we start the BP3 support
from something that works. The new formula should not change what was
before, and silently fix a bug, so the first patch is to address this.
The second patch is Michael's suggestion split from Jungseung's patch,
for a clearer separation of the fixes.

This was just compiled, not (yet) tested on a flash, I'll test it later
today. So I need you guys to double check the changes, do some testing,
and if all good, let me know.

Jungseung Lee (3):
  mtd: spi-nor: Add new formula for SR block protection handling
  mtd: spi-nor: Add SR 4bit block protection support
  mtd: spi-nor: Enable locking for n25q512ax3/n25q512a

Tudor Ambarus (2):
  mtd: spi-nor: Fix gap in SR block protection locking
  mtd: spi-nor: Set all BP bits to one when lock_len == mtd->size

 drivers/mtd/spi-nor/core.c      | 144 +++++++++++++++++++++-----------
 drivers/mtd/spi-nor/core.h      |  10 +++
 drivers/mtd/spi-nor/micron-st.c |   8 +-
 include/linux/mtd/spi-nor.h     |   2 +
 4 files changed, 113 insertions(+), 51 deletions(-)

-- 
2.23.0

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

[PATCH v3 2/5] mtd: spi-nor: Set all BP bits to one when lock_len == mtd->size

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

When there are more BP settings than needed for defining the protected
areas of the flash memory, most flashes will define the remaining
settings as "protect all", i.e. the equivalent of having all the BP bits
set to one. But there are flashes where the in-between BP values
are undefined (not mentioned), and only the "all bits set" is protecting
the entire memory. One such example is w25q80, where BP[2:0]=0b101 and
0b110 are not defined.

Set all the BP bits to one when lock_len == mtd->size, to treat this
special case.

Suggested-by: Michael Walle <michael@walle.cc>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 36660068bc04..3788a95c0a47 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1660,13 +1660,19 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	 *
 	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1
 	 */
-	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
-	val = mask - (pow << SR_BP_SHIFT);
-	if (val & ~mask)
-		return -EINVAL;
-	/* Don't "lock" with no region! */
-	if (!(val & mask))
-		return -EINVAL;
+	if (lock_len == mtd->size) {
+		val = mask;
+	} else {
+		pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
+		val = mask - (pow << SR_BP_SHIFT);
+
+		if (val & ~mask)
+			return -EINVAL;
+
+		/* Don't "lock" with no region! */
+		if (!(val & mask))
+			return -EINVAL;
+	}
 
 	status_new = (status_old & ~mask & ~tb_mask) | val;
 
-- 
2.23.0

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

[PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Fix the gap for the SR block protection, the BP bits were set with
a +1 value than actually needed. This patch does not change the
behavior of the locking operations, just fixes the protected areas.

On a 16Mbit flash with 64KByte erase sector, the following changed
for the lock operation:

Number of blocks | BP2:0 before | BP2:0 now |
               1 | 010b         | 001b      |
               2 | 110b         | 010b      |
               3 | 110b         | 010b      |
               4 | 100b         | 011b      |
               5 | 100b         | 011b      |
               6 | 100b         | 011b      |
               7 | 100b         | 011b      |
               8 | 101b         | 100b      |
               9 | 101b         | 100b      |
             ... | ...          | ...       |

For the lock operation, if one requests to lock an area that is not
matching the upper boundary of a BP protected area, we round down
the total length and lock less than the user requested, in order to
not lock more than the user actually requested.

For the unlock operation, read the number of blocks column as
"locked all but number of blocks value". On a 16Mbit flash with
64KByte erase sector, the following changed for the lock operation:

Number of blocks | BP2:0 before | BP2:0 now |
               1 | 111b         | 101b      |
             ... | ...          | ...       |
              15 | 111b         | 101b      |
              16 | 110b         | 101b      |
              17 | 110b         | 100b      |
             ... | ...          | ...       |
              24 | 101b         | 100b      |
              25 | 101b         | 011b      |
              26 | 101b         | 011b      |
              27 | 101b         | 011b      |
              28 | 100b         | 011b      |
              29 | 100b         | 010b      |
              30 | 011b         | 010b      |
              31 | 010b         | 001b      |
              32 | 000b         | 000b      |

For the unlock operation, if one requests to unlock an area that is
not matching the upper boundary of a BP protected area, we round up
the total length and unlock more than the user actually requested.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 877557dbda7f..36660068bc04 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	/*
 	 * Need smallest pow such that:
 	 *
-	 *   1 / (2^pow) <= (len / size)
+	 *   1 / ((2^pow) - 1) <= (len / size)
 	 *
 	 * so (assuming power-of-2 size) we do:
 	 *
-	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
+	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1
 	 */
-	pow = ilog2(mtd->size) - ilog2(lock_len);
+	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
 	val = mask - (pow << SR_BP_SHIFT);
 	if (val & ~mask)
 		return -EINVAL;
@@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	/*
 	 * Need largest pow such that:
 	 *
-	 *   1 / (2^pow) >= (len / size)
+	 *   1 / ((2^pow) - 1) >= (len / size)
 	 *
 	 * so (assuming power-of-2 size) we do:
 	 *
-	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len))
+	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1
 	 */
-	pow = ilog2(mtd->size) - order_base_2(lock_len);
+	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1;
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
-- 
2.23.0

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

[PATCH v3 3/5] mtd: spi-nor: Add new formula for SR block protection handling

[Tudor.Ambarus]

From: Jungseung Lee <js07.lee@samsung.com>

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 logic to handle block protection based on min_prot_length.
It is suitable for the overall flashes with exception of some corner cases
(see EON and catalyst) 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>
Reviewed-by: Michael Walle <michael@walle.cc>
Tested-by: Michael Walle <michael@walle.cc>
[ta: - drop spi_nor_get_bp_mask(), spi_nor_get_tb_mask()
- rename spi_nor_get_min_prot_length/spi_nor_get_min_prot_length_sr
- static u64 spi_nor_get_min_prot_length
- unsigned int bp_slots, bp_slots_needed;
- bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2;
- amend commit description]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 72 ++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 3788a95c0a47..c0d186f417d8 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1514,29 +1514,51 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	return ret;
 }
 
+static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
+{
+	unsigned int bp_slots, bp_slots_needed;
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	/* Reserved one for "protect none" and one for "protect all". */
+	bp_slots = (mask >> SR_BP_SHIFT) + 1 - 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 spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
 					uint64_t *len)
 {
 	struct mtd_info *mtd = &nor->mtd;
+	u64 min_prot_len;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
-	int pow;
+	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 = spi_nor_get_min_prot_length_sr(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;
 }
 
 /*
@@ -1609,6 +1631,7 @@ static int spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
+	u64 min_prot_len;
 	int ret, status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
@@ -1651,20 +1674,12 @@ static int spi_nor_sr_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;
 
-	/*
-	 * Need smallest pow such that:
-	 *
-	 *   1 / ((2^pow) - 1) <= (len / size)
-	 *
-	 * so (assuming power-of-2 size) we do:
-	 *
-	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1
-	 */
 	if (lock_len == mtd->size) {
 		val = mask;
 	} else {
-		pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
-		val = mask - (pow << SR_BP_SHIFT);
+		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
+		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
+		val = pow << SR_BP_SHIFT;
 
 		if (val & ~mask)
 			return -EINVAL;
@@ -1701,6 +1716,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
 	struct mtd_info *mtd = &nor->mtd;
+	u64 min_prot_len;
 	int ret, status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 tb_mask = SR_TB_BIT5;
@@ -1742,20 +1758,14 @@ static int spi_nor_sr_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;
-	/*
-	 * Need largest pow such that:
-	 *
-	 *   1 / ((2^pow) - 1) >= (len / size)
-	 *
-	 * so (assuming power-of-2 size) we do:
-	 *
-	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1
-	 */
-	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1;
+
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
-		val = mask - (pow << SR_BP_SHIFT);
+		min_prot_len = spi_nor_get_min_prot_length_sr(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.23.0

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

[PATCH v3 4/5] mtd: spi-nor: Add 4bit SR block protection support

[Tudor.Ambarus]

From: Jungseung Lee <js07.lee@samsung.com>

Currently, we are supporting block protection only for flash chips with
3 block protection bits (BP0-2) in the SR register.

Enable block protection support for flashes with 4 block protection bits
(BP0-3).

Add a flash_info flag for flashes that describe 4 block protection bits.
Add another flash_info flag for flashes in which BP3 bit is not adjacent
to the BP0-2 bits.

Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
Reviewed-by: Michael Walle <michael@walle.cc>
Tested-by: Michael Walle <michael@walle.cc>
[ta:
- introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
- drop Micron n25q512ax3 / BP0-3) boilerplate description
- be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael
suggested,
- amend commit description]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c  | 66 +++++++++++++++++++++++++++----------
 drivers/mtd/spi-nor/core.h  | 10 ++++++
 include/linux/mtd/spi-nor.h |  2 ++
 3 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index c0d186f417d8..b70c0b2e0958 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	return ret;
 }
 
+static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor)
+{
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+		return mask | SR_BP3_BIT6;
+
+	if (nor->flags & SNOR_F_HAS_4BIT_BP)
+		return mask | SR_BP3;
+
+	return mask;
+}
+
+static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
 {
 	unsigned int bp_slots, bp_slots_needed;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
 
 	/* Reserved one for "protect none" and one for "protect all". */
-	bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2;
+	bp_slots = (1 << hweight8(mask)) - 2;
 	bp_slots_needed = ilog2(nor->info->n_sectors);
 
 	if (bp_slots_needed > bp_slots)
@@ -1535,12 +1556,14 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
 {
 	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
-	u8 bp = (sr & mask) >> SR_BP_SHIFT;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
+	u8 bp, val = sr & mask;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
+	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
+		val = (val & ~SR_BP3_BIT6) | SR_BP3;
+
+	bp = val >> SR_BP_SHIFT;
 
 	if (!bp) {
 		/* No protection */
@@ -1598,7 +1621,8 @@ static int spi_nor_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)
@@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_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;
@@ -1671,9 +1695,6 @@ static int spi_nor_sr_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;
 	} else {
@@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
+			val = (val & ~SR_BP3) | SR_BP3_BIT6;
+
 		if (val & ~mask)
 			return -EINVAL;
 
@@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_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;
@@ -1756,9 +1780,6 @@ static int spi_nor_sr_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;
-
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
@@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
+			val = (val & ~SR_BP3) | SR_BP3_BIT6;
+
 		/* Some power-of-two sizes are not supported */
 		if (val & ~mask)
 			return -EINVAL;
@@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 3ce826b35ad1..6f2f6b27173f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -24,6 +24,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),
 };
 
 struct spi_nor_read_command {
@@ -301,6 +303,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;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index e656858b50a5..1e2af0ec1f03 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -111,7 +111,9 @@
 #define SR_BP0			BIT(2)	/* Block protect 0 */
 #define SR_BP1			BIT(3)	/* Block protect 1 */
 #define SR_BP2			BIT(4)	/* Block protect 2 */
+#define SR_BP3			BIT(5)	/* Block protect 3 */
 #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
+#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
 #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 /* Spansion/Cypress specific status bits */
-- 
2.23.0

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

[PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support

[Tudor.Ambarus]

From: Jungseung Lee <js07.lee@samsung.com>

Currently, we are supporting block protection only for flash chips with
3 block protection bits (BP0-2) in the SR register.

Enable block protection support for flashes with 4 block protection bits
(BP0-3).

Add a flash_info flag for flashes that describe 4 block protection bits.
Add another flash_info flag for flashes in which BP3 bit is not adjacent
to the BP0-2 bits.

Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
Reviewed-by: Michael Walle <michael@walle.cc>
Tested-by: Michael Walle <michael@walle.cc>
[ta:
- introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
- drop Micron n25q512ax3 / BP0-3) boilerplate description
- be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael
suggested,
- amend commit description]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c  | 66 +++++++++++++++++++++++++++----------
 drivers/mtd/spi-nor/core.h  | 10 ++++++
 include/linux/mtd/spi-nor.h |  2 ++
 3 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index c0d186f417d8..b70c0b2e0958 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	return ret;
 }
 
+static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor)
+{
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+		return mask | SR_BP3_BIT6;
+
+	if (nor->flags & SNOR_F_HAS_4BIT_BP)
+		return mask | SR_BP3;
+
+	return mask;
+}
+
+static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
 {
 	unsigned int bp_slots, bp_slots_needed;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
 
 	/* Reserved one for "protect none" and one for "protect all". */
-	bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2;
+	bp_slots = (1 << hweight8(mask)) - 2;
 	bp_slots_needed = ilog2(nor->info->n_sectors);
 
 	if (bp_slots_needed > bp_slots)
@@ -1535,12 +1556,14 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
 {
 	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
-	u8 bp = (sr & mask) >> SR_BP_SHIFT;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
+	u8 bp, val = sr & mask;
 
-	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
-		tb_mask = SR_TB_BIT6;
+	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
+		val = (val & ~SR_BP3_BIT6) | SR_BP3;
+
+	bp = val >> SR_BP_SHIFT;
 
 	if (!bp) {
 		/* No protection */
@@ -1598,7 +1621,8 @@ static int spi_nor_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)
@@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_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;
@@ -1671,9 +1695,6 @@ static int spi_nor_sr_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;
 	} else {
@@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
+			val = (val & ~SR_BP3) | SR_BP3_BIT6;
+
 		if (val & ~mask)
 			return -EINVAL;
 
@@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	int ret, status_old, status_new;
-	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
+	u8 mask = spi_nor_get_sr_bp_mask(nor);
+	u8 tb_mask = spi_nor_get_sr_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;
@@ -1756,9 +1780,6 @@ static int spi_nor_sr_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;
-
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
@@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
+			val = (val & ~SR_BP3) | SR_BP3_BIT6;
+
 		/* Some power-of-two sizes are not supported */
 		if (val & ~mask)
 			return -EINVAL;
@@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 3ce826b35ad1..6f2f6b27173f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -24,6 +24,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),
 };
 
 struct spi_nor_read_command {
@@ -301,6 +303,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;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index e656858b50a5..1e2af0ec1f03 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -111,7 +111,9 @@
 #define SR_BP0			BIT(2)	/* Block protect 0 */
 #define SR_BP1			BIT(3)	/* Block protect 1 */
 #define SR_BP2			BIT(4)	/* Block protect 2 */
+#define SR_BP3			BIT(5)	/* Block protect 3 */
 #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
+#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
 #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 /* Spansion/Cypress specific status bits */
-- 
2.23.0

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

[PATCH v3 5/5] mtd: spi-nor: Enable locking for n25q512ax3/n25q512a

[Tudor.Ambarus]

From: Jungseung Lee <js07.lee@samsung.com>

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 n25q512ax3. The other is modified following the datasheet.

Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/micron-st.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 3874a62d8b47..6c034b9718e2 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -47,12 +47,16 @@ static const struct flash_info st_parts[] = {
 			       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) },
+			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
+			      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+			      SPI_NOR_4BIT_BP | 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) },
+			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
+			      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+			      SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6) },
 	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048,
 			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
 			      NO_CHIP_ERASE) },
-- 
2.23.0

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

Re: [PATCH v3 4/5] mtd: spi-nor: Add 4bit SR block protection support

[Tudor.Ambarus]

Please ignore this patch, it duplicates " [PATCH v3 4/5] mtd: spi-nor: Add SR 
4bit block protection support"


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

Re: [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support

[Jungseung Lee]

Hi,

On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote:
> From: Jungseung Lee <js07.lee@samsung.com>
> 
> Currently, we are supporting block protection only for flash chips
> with
> 3 block protection bits (BP0-2) in the SR register.
> 
> Enable block protection support for flashes with 4 block protection
> bits
> (BP0-3).
> 
> Add a flash_info flag for flashes that describe 4 block protection
> bits.
> Add another flash_info flag for flashes in which BP3 bit is not
> adjacent
> to the BP0-2 bits.
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> Reviewed-by: Michael Walle <michael@walle.cc>
> Tested-by: Michael Walle <michael@walle.cc>
> [ta:
> - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
> - drop Micron n25q512ax3 / BP0-3) boilerplate description
> - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael
> suggested,
> - amend commit description]
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 66 +++++++++++++++++++++++++++------
> ----
>  drivers/mtd/spi-nor/core.h  | 10 ++++++
>  include/linux/mtd/spi-nor.h |  2 ++
>  3 files changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c0d186f417d8..b70c0b2e0958 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info
> *mtd, struct erase_info *instr)
>  	return ret;
>  }
>  
> +static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor)
> +{
> +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
> +	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> +		return mask | SR_BP3_BIT6;
> +
> +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> +		return mask | SR_BP3;
> +
> +	return mask;
> +}
> +
> +static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
>  {
>  	unsigned int bp_slots, bp_slots_needed;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
>  
>  	/* Reserved one for "protect none" and one for "protect all".
> */
> -	bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2;
> +	bp_slots = (1 << hweight8(mask)) - 2;
>  	bp_slots_needed = ilog2(nor->info->n_sectors);
>  
>  	if (bp_slots_needed > bp_slots)
> @@ -1535,12 +1556,14 @@ static void
> spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  	u64 min_prot_len;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 tb_mask = SR_TB_BIT5;
> -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> +	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
> +	u8 bp, val = sr & mask;
>  
> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> -		tb_mask = SR_TB_BIT6;
> +	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> +		val = (val & ~SR_BP3_BIT6) | SR_BP3;
> +
> +	bp = val >> SR_BP_SHIFT;
>  
>  	if (!bp) {
>  		/* No protection */
> @@ -1598,7 +1621,8 @@ static int spi_nor_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)
> @@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor,
> loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	u64 min_prot_len;
>  	int ret, status_old, status_new;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 tb_mask = SR_TB_BIT5;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> +	u8 tb_mask = spi_nor_get_sr_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;
> @@ -1671,9 +1695,6 @@ static int spi_nor_sr_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;
>  	} else {
> @@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val &
> SR_BP3)
> +			val = (val & ~SR_BP3) | SR_BP3_BIT6;
> +
>  		if (val & ~mask)
>  			return -EINVAL;
>  
> @@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	u64 min_prot_len;
>  	int ret, status_old, status_new;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 tb_mask = SR_TB_BIT5;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> +	u8 tb_mask = spi_nor_get_sr_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;
> @@ -1756,9 +1780,6 @@ static int spi_nor_sr_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;
> -
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
>  	} else {
> @@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val &
> SR_BP3)
> +			val = (val & ~SR_BP3) | SR_BP3_BIT6;
> +
>  		/* Some power-of-two sizes are not supported */
>  		if (val & ~mask)
>  			return -EINVAL;
> @@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 3ce826b35ad1..6f2f6b27173f 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -24,6 +24,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),
>  };
>  
>  struct spi_nor_read_command {
> @@ -301,6 +303,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;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> nor.h
> index e656858b50a5..1e2af0ec1f03 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -111,7 +111,9 @@
>  #define SR_BP0			BIT(2)	/* Block protect 0 */
>  #define SR_BP1			BIT(3)	/* Block protect 1 */
>  #define SR_BP2			BIT(4)	/* Block protect 2 */
> +#define SR_BP3			BIT(5)	/* Block protect 3 */
>  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
> +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
>  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
>  #define SR_SRWD			BIT(7)	/* SR write protect
> */
>  /* Spansion/Cypress specific status bits */

Tested with a n25q512ax3 (bp0-3) and w25q128 (bp0-2).
It passed all of my test cases.

Tested-by: Jungseung Lee <js07.lee@samsung.com>


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

Re: [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support

[Tudor.Ambarus]

On Monday, March 23, 2020 2:43:09 PM EET Jungseung Lee wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi,

Hi, Jungseung,
> 
> On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote:
> > From: Jungseung Lee <js07.lee@samsung.com>
> > 
> > Currently, we are supporting block protection only for flash chips
> > with
> > 3 block protection bits (BP0-2) in the SR register.
> > 
> > Enable block protection support for flashes with 4 block protection
> > bits
> > (BP0-3).
> > 
> > Add a flash_info flag for flashes that describe 4 block protection
> > bits.
> > Add another flash_info flag for flashes in which BP3 bit is not
> > adjacent
> > to the BP0-2 bits.
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > Reviewed-by: Michael Walle <michael@walle.cc>
> > Tested-by: Michael Walle <michael@walle.cc>
> > [ta:
> > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
from the previous patch set
> > - drop Micron n25q512ax3 / BP0-3) boilerplate description
> > - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael
> > suggested,
> > - amend commit description]

I'll drop these comments when/if applying. Let me know if you're ok with the 
changes that I did. Please do the same for patches 3/5 and 5/5.

> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > ---
> > 
> >  drivers/mtd/spi-nor/core.c  | 66 +++++++++++++++++++++++++++------
> > 
> > ----
> > 
> >  drivers/mtd/spi-nor/core.h  | 10 ++++++
> >  include/linux/mtd/spi-nor.h |  2 ++
> >  3 files changed, 60 insertions(+), 18 deletions(-)

cut

> Tested with a n25q512ax3 (bp0-3) and w25q128 (bp0-2).
> It passed all of my test cases.
> 
> Tested-by: Jungseung Lee <js07.lee@samsung.com>

Great! Since you are the author of the patch, it's not necessary to add your 
T-b tag, but I'll amend the commit description to say that it was tested on 
n25q512ax3 (bp0-3) and w25q128 (bp0-2).

Also, would you please review patches 1 and 2 from the series?

Cheers,
ta


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

Re: [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support

[Jungseung Lee]

Hi,

On Mon, 2020-03-23 at 12:55 +0000, Tudor.Ambarus@microchip.com wrote:
> On Monday, March 23, 2020 2:43:09 PM EET Jungseung Lee wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the
> > content is safe
> > 
> > Hi,
> 
> Hi, Jungseung,
> > 
> > On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com
> > wrote:
> > > From: Jungseung Lee <js07.lee@samsung.com>
> > > 
> > > Currently, we are supporting block protection only for flash
> > > chips
> > > with
> > > 3 block protection bits (BP0-2) in the SR register.
> > > 
> > > Enable block protection support for flashes with 4 block
> > > protection
> > > bits
> > > (BP0-3).
> > > 
> > > Add a flash_info flag for flashes that describe 4 block
> > > protection
> > > bits.
> > > Add another flash_info flag for flashes in which BP3 bit is not
> > > adjacent
> > > to the BP0-2 bits.
> > > 
> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > Reviewed-by: Michael Walle <michael@walle.cc>
> > > Tested-by: Michael Walle <michael@walle.cc>
> > > [ta:
> > > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
> 
> from the previous patch set
> > > - drop Micron n25q512ax3 / BP0-3) boilerplate description
> > > - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as
> > > Michael
> > > suggested,
> > > - amend commit description]
> 
> I'll drop these comments when/if applying. Let me know if you're ok
> with the 
> changes that I did. Please do the same for patches 3/5 and 5/5.
> 

It looks much better. All the parts you modified are those that I
thought were ambiguous. now it's ok.

Thanks,

> > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > > ---
> > > 
> > >  drivers/mtd/spi-nor/core.c  | 66 +++++++++++++++++++++++++++--
> > > ----
> > > 
> > > ----
> > > 
> > >  drivers/mtd/spi-nor/core.h  | 10 ++++++
> > >  include/linux/mtd/spi-nor.h |  2 ++
> > >  3 files changed, 60 insertions(+), 18 deletions(-)
> 
> cut
> 
> > Tested with a n25q512ax3 (bp0-3) and w25q128 (bp0-2).
> > It passed all of my test cases.
> > 
> > Tested-by: Jungseung Lee <js07.lee@samsung.com>
> 
> Great! Since you are the author of the patch, it's not necessary to
> add your 
> T-b tag, but I'll amend the commit description to say that it was
> tested on 
> n25q512ax3 (bp0-3) and w25q128 (bp0-2).
> 
> Also, would you please review patches 1 and 2 from the series?
> 
> Cheers,
> ta
> 
> 


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

Re: [PATCH v3 3/5] mtd: spi-nor: Add new formula for SR block protection handling

[Jungseung Lee]

> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> 
> Sent: Monday, March 23, 2020 6:25 PM
> To: js07.lee@samsung.com; michael@walle.cc; vigneshr@ti.com
> Cc: linux-mtd@lists.infradead.org; Tudor.Ambarus@microchip.com
> Subject: [PATCH v3 3/5] mtd: spi-nor: Add new formula for SR block
> protection handling
> 
> From: Jungseung Lee <js07.lee@samsung.com>
> 
> 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 logic to handle block protection based on
> min_prot_length.
> It is suitable for the overall flashes with exception of some corner
> cases
> (see EON and catalyst) and easy to extend and apply for the case of
> 2bit or
> 4bit block protection.
> 
> [1] https://protect2.fireeye.com/url?k=d62c9c1b-8bf82073-d62d1754-
> 0cc47a3356b2-012ef3655070329a&u=
> http://lists.infradead.org/pipermail/linux-
> mtd/2020-February/093934.html
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> Reviewed-by: Michael Walle <michael@walle.cc>
> Tested-by: Michael Walle <michael@walle.cc>
> [ta: - drop spi_nor_get_bp_mask(), spi_nor_get_tb_mask()
> - rename spi_nor_get_min_prot_length/spi_nor_get_min_prot_length_sr
> - static u64 spi_nor_get_min_prot_length
> - unsigned int bp_slots, bp_slots_needed;
> - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2;
> - amend commit description]

All looks good and it's ok for me.

Thanks,

> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 72 ++++++++++++++++++++++------------
> ----
>  1 file changed, 41 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 3788a95c0a47..c0d186f417d8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1514,29 +1514,51 @@ static int spi_nor_erase(struct mtd_info
> *mtd,
> struct erase_info *instr)
>  	return ret;
>  }
>  
> +static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
> +{
> +	unsigned int bp_slots, bp_slots_needed;
> +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
> +	/* Reserved one for "protect none" and one for "protect all".
> */
> +	bp_slots = (mask >> SR_BP_SHIFT) + 1 - 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 spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr,
> loff_t
> *ofs,
>  					uint64_t *len)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
> +	u64 min_prot_len;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>  	u8 tb_mask = SR_TB_BIT5;
> -	int pow;
> +	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 = spi_nor_get_min_prot_length_sr(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;
>  }
>  
>  /*
> @@ -1609,6 +1631,7 @@ static int spi_nor_is_unlocked_sr(struct
> spi_nor
> *nor, loff_t ofs, uint64_t len,
>  static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t
> len)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
> +	u64 min_prot_len;
>  	int ret, status_old, status_new;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>  	u8 tb_mask = SR_TB_BIT5;
> @@ -1651,20 +1674,12 @@ static int spi_nor_sr_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;
>  
> -	/*
> -	 * Need smallest pow such that:
> -	 *
> -	 *   1 / ((2^pow) - 1) <= (len / size)
> -	 *
> -	 * so (assuming power-of-2 size) we do:
> -	 *
> -	 *   pow = ceil(log2(size / len)) = log2(size) -
> floor(log2(len)) +
> 1
> -	 */
>  	if (lock_len == mtd->size) {
>  		val = mask;
>  	} else {
> -		pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
> -		val = mask - (pow << SR_BP_SHIFT);
> +		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
> +		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> +		val = pow << SR_BP_SHIFT;
>  
>  		if (val & ~mask)
>  			return -EINVAL;
> @@ -1701,6 +1716,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor,
> loff_t ofs, uint64_t len)
>  static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs,
> uint64_t len)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
> +	u64 min_prot_len;
>  	int ret, status_old, status_new;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>  	u8 tb_mask = SR_TB_BIT5;
> @@ -1742,20 +1758,14 @@ static int spi_nor_sr_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;
> -	/*
> -	 * Need largest pow such that:
> -	 *
> -	 *   1 / ((2^pow) - 1) >= (len / size)
> -	 *
> -	 * so (assuming power-of-2 size) we do:
> -	 *
> -	 *   pow = floor(log2(size / len)) = log2(size) -
> ceil(log2(len)) +
> 1
> -	 */
> -	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1;
> +
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
>  	} else {
> -		val = mask - (pow << SR_BP_SHIFT);
> +		min_prot_len = spi_nor_get_min_prot_length_sr(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;


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

Re: [PATCH v3 2/5] mtd: spi-nor: Set all BP bits to one when lock_len == mtd->size

[Jungseung Lee]

On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> When there are more BP settings than needed for defining the
> protected
> areas of the flash memory, most flashes will define the remaining
> settings as "protect all", i.e. the equivalent of having all the BP
> bits
> set to one. But there are flashes where the in-between BP values
> are undefined (not mentioned), and only the "all bits set" is
> protecting
> the entire memory. One such example is w25q80, where BP[2:0]=0b101
> and
> 0b110 are not defined.
> 
> Set all the BP bits to one when lock_len == mtd->size, to treat this
> special case.
> 
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 36660068bc04..3788a95c0a47 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1660,13 +1660,19 @@ static int spi_nor_sr_lock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	 *
>  	 *   pow = ceil(log2(size / len)) = log2(size) -
> floor(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
> -	val = mask - (pow << SR_BP_SHIFT);
> -	if (val & ~mask)
> -		return -EINVAL;
> -	/* Don't "lock" with no region! */
> -	if (!(val & mask))
> -		return -EINVAL;
> +	if (lock_len == mtd->size) {
> +		val = mask;
> +	} else {
> +		pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
> +		val = mask - (pow << SR_BP_SHIFT);
> +
> +		if (val & ~mask)
> +			return -EINVAL;
> +
> +		/* Don't "lock" with no region! */
> +		if (!(val & mask))
> +			return -EINVAL;
> +	}
>  
>  	status_new = (status_old & ~mask & ~tb_mask) | val;
>  

Reviewed-by: Jungseung Lee <js07.lee@samsung.com>


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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Michael Walle]

Hi,

Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Fix the gap for the SR block protection, the BP bits were set with
> a +1 value than actually needed. This patch does not change the
> behavior of the locking operations, just fixes the protected areas.

So instead of rounding up, it does round down now?

> 
> On a 16Mbit flash with 64KByte erase sector, the following changed
> for the lock operation:
> 
> Number of blocks | BP2:0 before | BP2:0 now |
>                1 | 010b         | 001b      |
>                2 | 110b         | 010b      |
>                3 | 110b         | 010b      |
>                4 | 100b         | 011b      |
>                5 | 100b         | 011b      |
>                6 | 100b         | 011b      |
>                7 | 100b         | 011b      |
>                8 | 101b         | 100b      |
>                9 | 101b         | 100b      |
>              ... | ...          | ...       |
> 
> For the lock operation, if one requests to lock an area that is not
> matching the upper boundary of a BP protected area, we round down
> the total length and lock less than the user requested, in order to
> not lock more than the user actually requested.

I don't know if that is really what a user really want. Because you'd
end up with regions which the user believes are locked but are not.
IMHO if you'd have to make a choice I'd prefer to have the remainder
locked. Not the other way around. Esp. since the user explicitly
express to have a region locked.

-michael

> For the unlock operation, read the number of blocks column as
> "locked all but number of blocks value". On a 16Mbit flash with
> 64KByte erase sector, the following changed for the lock operation:
> 
> Number of blocks | BP2:0 before | BP2:0 now |
>                1 | 111b         | 101b      |
>              ... | ...          | ...       |
>               15 | 111b         | 101b      |
>               16 | 110b         | 101b      |
>               17 | 110b         | 100b      |
>              ... | ...          | ...       |
>               24 | 101b         | 100b      |
>               25 | 101b         | 011b      |
>               26 | 101b         | 011b      |
>               27 | 101b         | 011b      |
>               28 | 100b         | 011b      |
>               29 | 100b         | 010b      |
>               30 | 011b         | 010b      |
>               31 | 010b         | 001b      |
>               32 | 000b         | 000b      |
> 
> For the unlock operation, if one requests to unlock an area that is
> not matching the upper boundary of a BP protected area, we round up
> the total length and unlock more than the user actually requested.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 877557dbda7f..36660068bc04 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	/*
>  	 * Need smallest pow such that:
>  	 *
> -	 *   1 / (2^pow) <= (len / size)
> +	 *   1 / ((2^pow) - 1) <= (len / size)
>  	 *
>  	 * so (assuming power-of-2 size) we do:
>  	 *
> -	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
> +	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - ilog2(lock_len);
> +	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
>  	val = mask - (pow << SR_BP_SHIFT);
>  	if (val & ~mask)
>  		return -EINVAL;
> @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	/*
>  	 * Need largest pow such that:
>  	 *
> -	 *   1 / (2^pow) >= (len / size)
> +	 *   1 / ((2^pow) - 1) >= (len / size)
>  	 *
>  	 * so (assuming power-of-2 size) we do:
>  	 *
> -	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len))
> +	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - order_base_2(lock_len);
> +	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1;
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
>  	} else {

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

Re: [PATCH v3 2/5] mtd: spi-nor: Set all BP bits to one when lock_len == mtd->size

[Michael Walle]

Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> When there are more BP settings than needed for defining the protected
> areas of the flash memory, most flashes will define the remaining
> settings as "protect all", i.e. the equivalent of having all the BP 
> bits
> set to one. But there are flashes where the in-between BP values
> are undefined (not mentioned), and only the "all bits set" is 
> protecting
> the entire memory. One such example is w25q80, where BP[2:0]=0b101 and
> 0b110 are not defined.
> 
> Set all the BP bits to one when lock_len == mtd->size, to treat this
> special case.
> 
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Michael Walle <michael@walle.cc>

> ---
>  drivers/mtd/spi-nor/core.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 36660068bc04..3788a95c0a47 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1660,13 +1660,19 @@ static int spi_nor_sr_lock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	 *
>  	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
> -	val = mask - (pow << SR_BP_SHIFT);
> -	if (val & ~mask)
> -		return -EINVAL;
> -	/* Don't "lock" with no region! */
> -	if (!(val & mask))
> -		return -EINVAL;
> +	if (lock_len == mtd->size) {
> +		val = mask;
> +	} else {
> +		pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
> +		val = mask - (pow << SR_BP_SHIFT);
> +
> +		if (val & ~mask)
> +			return -EINVAL;
> +
> +		/* Don't "lock" with no region! */
> +		if (!(val & mask))
> +			return -EINVAL;
> +	}
> 
>  	status_new = (status_old & ~mask & ~tb_mask) | val;

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

Re: [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support

[Michael Walle]

Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> From: Jungseung Lee <js07.lee@samsung.com>
> 
> Currently, we are supporting block protection only for flash chips with
> 3 block protection bits (BP0-2) in the SR register.
> 
> Enable block protection support for flashes with 4 block protection 
> bits
> (BP0-3).
> 
> Add a flash_info flag for flashes that describe 4 block protection 
> bits.
> Add another flash_info flag for flashes in which BP3 bit is not 
> adjacent
> to the BP0-2 bits.
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> Reviewed-by: Michael Walle <michael@walle.cc>
> Tested-by: Michael Walle <michael@walle.cc>
> [ta:
> - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
> - drop Micron n25q512ax3 / BP0-3) boilerplate description

that was actually a comment on my side some time ago. Because the 
current
example isn't really good and lacks the second case (which is added by
this patch).

-michael


> - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael
> suggested,
> - amend commit description]
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 66 +++++++++++++++++++++++++++----------
>  drivers/mtd/spi-nor/core.h  | 10 ++++++
>  include/linux/mtd/spi-nor.h |  2 ++
>  3 files changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c0d186f417d8..b70c0b2e0958 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info *mtd,
> struct erase_info *instr)
>  	return ret;
>  }
> 
> +static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor)
> +{
> +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
> +	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> +		return mask | SR_BP3_BIT6;
> +
> +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> +		return mask | SR_BP3;
> +
> +	return mask;
> +}
> +
> +static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
>  {
>  	unsigned int bp_slots, bp_slots_needed;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> 
>  	/* Reserved one for "protect none" and one for "protect all". */
> -	bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2;
> +	bp_slots = (1 << hweight8(mask)) - 2;
>  	bp_slots_needed = ilog2(nor->info->n_sectors);
> 
>  	if (bp_slots_needed > bp_slots)
> @@ -1535,12 +1556,14 @@ static void spi_nor_get_locked_range_sr(struct
> spi_nor *nor, u8 sr, loff_t *ofs,
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  	u64 min_prot_len;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 tb_mask = SR_TB_BIT5;
> -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> +	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
> +	u8 bp, val = sr & mask;
> 
> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> -		tb_mask = SR_TB_BIT6;
> +	if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> +		val = (val & ~SR_BP3_BIT6) | SR_BP3;
> +
> +	bp = val >> SR_BP_SHIFT;
> 
>  	if (!bp) {
>  		/* No protection */
> @@ -1598,7 +1621,8 @@ static int spi_nor_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)
> @@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor,
> loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	u64 min_prot_len;
>  	int ret, status_old, status_new;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 tb_mask = SR_TB_BIT5;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> +	u8 tb_mask = spi_nor_get_sr_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;
> @@ -1671,9 +1695,6 @@ static int spi_nor_sr_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;
>  	} else {
> @@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> +			val = (val & ~SR_BP3) | SR_BP3_BIT6;
> +
>  		if (val & ~mask)
>  			return -EINVAL;
> 
> @@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	u64 min_prot_len;
>  	int ret, status_old, status_new;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 tb_mask = SR_TB_BIT5;
> +	u8 mask = spi_nor_get_sr_bp_mask(nor);
> +	u8 tb_mask = spi_nor_get_sr_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;
> @@ -1756,9 +1780,6 @@ static int spi_nor_sr_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;
> -
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
>  	} else {
> @@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> +			val = (val & ~SR_BP3) | SR_BP3_BIT6;
> +
>  		/* Some power-of-two sizes are not supported */
>  		if (val & ~mask)
>  			return -EINVAL;
> @@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 3ce826b35ad1..6f2f6b27173f 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -24,6 +24,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),
>  };
> 
>  struct spi_nor_read_command {
> @@ -301,6 +303,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;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index e656858b50a5..1e2af0ec1f03 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -111,7 +111,9 @@
>  #define SR_BP0			BIT(2)	/* Block protect 0 */
>  #define SR_BP1			BIT(3)	/* Block protect 1 */
>  #define SR_BP2			BIT(4)	/* Block protect 2 */
> +#define SR_BP3			BIT(5)	/* Block protect 3 */
>  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
> +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
>  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
>  #define SR_SRWD			BIT(7)	/* SR write protect */
>  /* Spansion/Cypress specific status bits */

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

Re: [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support

[Tudor.Ambarus]

On Monday, March 23, 2020 8:33:19 PM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> > From: Jungseung Lee <js07.lee@samsung.com>
> > 
> > Currently, we are supporting block protection only for flash chips with
> > 3 block protection bits (BP0-2) in the SR register.
> > 
> > Enable block protection support for flashes with 4 block protection
> > bits
> > (BP0-3).
> > 
> > Add a flash_info flag for flashes that describe 4 block protection
> > bits.
> > Add another flash_info flag for flashes in which BP3 bit is not
> > adjacent
> > to the BP0-2 bits.
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > Reviewed-by: Michael Walle <michael@walle.cc>
> > Tested-by: Michael Walle <michael@walle.cc>
> > [ta:
> > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask()
> > - drop Micron n25q512ax3 / BP0-3) boilerplate description
> 
> that was actually a comment on my side some time ago. Because the
> current
> example isn't really good and lacks the second case (which is added by
> this patch).
> 

I didn't like the example that was introduced by Jungseung because of the last 
column, the "Protected Portion" -> it focuses on Upper/Lower 1/pow(2, n). I 
think it is better to replace the "Protected Portion" column with a "Protected 
Block(s)" column (see a winbond datasheet), in order to be in sync with how 
the code looks now.

It's true that the current example has the same problem. Would you care to 
send a patch to replace the current example? (keeping two examples would be 
too much). Or maybe remove the example entirely?

Also, would you please review 1/5 as well? I need an agreement on that before 
applying the series.

Cheers,
ta


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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi,
> 
> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
> > 
> > Fix the gap for the SR block protection, the BP bits were set with
> > a +1 value than actually needed. This patch does not change the
> > behavior of the locking operations, just fixes the protected areas.
> 
> So instead of rounding up, it does round down now?

No. Why do you say that it rounds up? The behavior is not changed, the patch 
merely fix the protected area, which was wrong before. The round down is 
present before this patch.
 
> 
> > On a 16Mbit flash with 64KByte erase sector, the following changed
> > for the lock operation:
> > 
> > Number of blocks | BP2:0 before | BP2:0 now |
> > 
> >                1 | 010b         | 001b      |
> >                2 | 110b         | 010b      |
> >                3 | 110b         | 010b      |
> >                4 | 100b         | 011b      |
> >                5 | 100b         | 011b      |
> >                6 | 100b         | 011b      |
> >                7 | 100b         | 011b      |
> >                8 | 101b         | 100b      |
> >                9 | 101b         | 100b      |
> >              
> >              ... | ...          | ...       |
> > 
> > For the lock operation, if one requests to lock an area that is not
> > matching the upper boundary of a BP protected area, we round down
> > the total length and lock less than the user requested, in order to
> > not lock more than the user actually requested.
> 
> I don't know if that is really what a user really want. Because you'd
> end up with regions which the user believes are locked but are not.

True. I'm thinking of how we can improve this, but it's not in the scope of 
this patch set, because the behavior is not changed.

> IMHO if you'd have to make a choice I'd prefer to have the remainder
> locked. Not the other way around. Esp. since the user explicitly
> express to have a region locked.
> 

We can still talk about this. Please notice that the formula that we want to 
introduce does the same thing as described in this commit message: for 
unaligned locks, it round down the length, and for unaligned unlocks it rounds 
up the length.

I'm waiting for a reply.
ta


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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Michael Walle]

Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com:
> On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Hi,
>> 
>> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
>> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
>> >
>> > Fix the gap for the SR block protection, the BP bits were set with
>> > a +1 value than actually needed. This patch does not change the
>> > behavior of the locking operations, just fixes the protected areas.
>> 
>> So instead of rounding up, it does round down now?
> 
> No. Why do you say that it rounds up? The behavior is not changed, the 
> patch
> merely fix the protected area, which was wrong before. The round down 
> is
> present before this patch.

TBH I don't understand what this patch should do. Could you give an 
example?

>> 
>> > On a 16Mbit flash with 64KByte erase sector, the following changed
>> > for the lock operation:

16MBit is a bad example, because it is broken anyway, isn't it? We use a
32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. a
50/50 split. I haven't seen any issued. Shouldn't it be then completely
locked according this the following example?

>> >
>> > Number of blocks | BP2:0 before | BP2:0 now |
>> >
>> >                1 | 010b         | 001b      |
>> >                2 | 110b         | 010b      |
>> >                3 | 110b         | 010b      |
>> >                4 | 100b         | 011b      |
>> >                5 | 100b         | 011b      |
>> >                6 | 100b         | 011b      |
>> >                7 | 100b         | 011b      |
>> >                8 | 101b         | 100b      |
>> >                9 | 101b         | 100b      |
>> >
>> >              ... | ...          | ...       |
>> >
>> > For the lock operation, if one requests to lock an area that is not
>> > matching the upper boundary of a BP protected area, we round down
>> > the total length and lock less than the user requested, in order to
>> > not lock more than the user actually requested.
>> 
>> I don't know if that is really what a user really want. Because you'd
>> end up with regions which the user believes are locked but are not.
> 
> True. I'm thinking of how we can improve this, but it's not in the 
> scope of
> this patch set, because the behavior is not changed.

ok, agreed,

> 
>> IMHO if you'd have to make a choice I'd prefer to have the remainder
>> locked. Not the other way around. Esp. since the user explicitly
>> express to have a region locked.
>> 
> 
> We can still talk about this. Please notice that the formula that we 
> want to
> introduce does the same thing as described in this commit message: for
> unaligned locks, it round down the length, and for unaligned unlocks it 
> rounds
> up the length.

ok

-michael

> 
> I'm waiting for a reply.
> ta

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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com:
> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> the
> >> content is safe
> >> 
> >> Hi,
> >> 
> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> > 
> >> > Fix the gap for the SR block protection, the BP bits were set with
> >> > a +1 value than actually needed. This patch does not change the
> >> > behavior of the locking operations, just fixes the protected areas.
> >> 
> >> So instead of rounding up, it does round down now?
> > 
> > No. Why do you say that it rounds up? The behavior is not changed, the
> > patch
> > merely fix the protected area, which was wrong before. The round down
> > is
> > present before this patch.
> 
> TBH I don't understand what this patch should do. Could you give an
> example?

sure, let me try to be more explicit.

> 
> >> > On a 16Mbit flash with 64KByte erase sector, the following changed
> 
> >> > for the lock operation:
> 16MBit is a bad example, because it is broken anyway, isn't it? We use a

it's not.

> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. a
> 50/50 split. I haven't seen any issued. Shouldn't it be then completely
> locked according this the following example?

I don't follow.

The table from below was generated for the S25FL116K 16 Mbit flash. BTW, one 
has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the locking. 
When you have a 4k sector erase, the locking is simply wrong, but this is 
another topic.

> 
> >> > Number of blocks | BP2:0 before | BP2:0 now |
> >> > 
> >> >                1 | 010b         | 001b      |

- number of blocks is how many blocks you want to lock. One would do for one 
block:
    flash_lock /dev/mtd 0 1
i.e. lock a single erase block starting from offset 0.

- "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 1" 
before this patch

- "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" using 
this patch

So before this patch, the lock operation was bad, because it locked 2 blocks 
instead of one.

> >> >                2 | 110b         | 010b      |

- lock 2 erase blocks starting from offset 0. Results before this patch, and 
after this patch. Continue the logic on the following lines.

oops there's a typo in column 2, sorry. The value in column 2 should have been 
011b.

So before this patch, when one requested to lock 2 block starting from offset 
0, we would obtain 4 blocks locked, and he should have obtained just 2.

The scope of this patch is to first fix the locking ops, so that we can 
introduce a more generic formula that gives the same results as before 
introducing it. Without this patch, the new formula will silently fix the bug 
that is described here.

> >> >                3 | 110b         | 010b      |
		^ typo s/110b/011b

rest of the examples are good.

Cheers,
ta

> >> >                4 | 100b         | 011b      |
> >> >                5 | 100b         | 011b      |
> >> >                6 | 100b         | 011b      |
> >> >                7 | 100b         | 011b      |
> >> >                8 | 101b         | 100b      |
> >> >                9 | 101b         | 100b      |
> >> >              
> >> >              ... | ...          | ...       |
> >> > 
> >> > For the lock operation, if one requests to lock an area that is not
> >> > matching the upper boundary of a BP protected area, we round down
> >> > the total length and lock less than the user requested, in order to
> >> > not lock more than the user actually requested.



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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Michael Walle]

Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com:
> On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com:
>> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
>> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> >> the
>> >> content is safe
>> >>
>> >> Hi,
>> >>
>> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
>> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
>> >> >
>> >> > Fix the gap for the SR block protection, the BP bits were set with
>> >> > a +1 value than actually needed. This patch does not change the
>> >> > behavior of the locking operations, just fixes the protected areas.
>> >>
>> >> So instead of rounding up, it does round down now?
>> >
>> > No. Why do you say that it rounds up? The behavior is not changed, the
>> > patch
>> > merely fix the protected area, which was wrong before. The round down
>> > is
>> > present before this patch.
>> 
>> TBH I don't understand what this patch should do. Could you give an
>> example?
> 
> sure, let me try to be more explicit.
> 
>> 
>> >> > On a 16Mbit flash with 64KByte erase sector, the following changed
>> 
>> >> > for the lock operation:
>> 16MBit is a bad example, because it is broken anyway, isn't it? We use 
>> a
> 
> it's not.

If I'm not mistaken this falls into the same category like the new 4bits 
BP
flashes, because there are more slots free than needed. Ie. the last one
"protect all" is either 110b or 111b and thus don't work with the old
formula. This was actually my reason why there is no new formula for the
4 bits BP flashes; but the current one is not working with flashes 
<32Mbit.
See the old long thread.


> 
>> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. 
>> a
>> 50/50 split. I haven't seen any issued. Shouldn't it be then 
>> completely
>> locked according this the following example?
> 
> I don't follow.

We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on 
our
boards to lock the first half of our 4MiB flash.

> The table from below was generated for the S25FL116K 16 Mbit flash. 
> BTW, one
> has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the 
> locking.
> When you have a 4k sector erase, the locking is simply wrong, but this 
> is
> another topic.

it should work with that too if you convert the number to the smaller 
sectors,
ie multiply by 16; But yeah the cli tool has a broken interface. It 
should
accept both offset and length in bytes; not one one in bytes and one in 
sectors,
where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS.


>> 
>> >> > Number of blocks | BP2:0 before | BP2:0 now |
>> >> >
>> >> >                1 | 010b         | 001b      |
> 
> - number of blocks is how many blocks you want to lock. One would do 
> for one
> block:
>     flash_lock /dev/mtd 0 1
> i.e. lock a single erase block starting from offset 0.
> 
> - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 
> 1"
> before this patch


Without your patch applied it works like expected:

[    1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes)
# flash_lock -l /dev/mtd1 0 1
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
a4

A4 is 1010_0100, ie BP[2:0] = 001b and TB=1

# flash_lock -u /dev/mtd1 0 64
# flash_lock -l /dev/mtd1 0 32
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
b8


With this patch applied:

# flash_lock -u /dev/mtd1 0 64
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
00
# flash_lock -l /dev/mtd1 0 1
flash_lock: error!: could not lock device: /dev/mtd1

             error 22 (Invalid argument)
# flash_lock -l /dev/mtd1 0 2
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
a4

which is wrong, isn't it?

-michael

> 
> - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" 
> using
> this patch
> 
> So before this patch, the lock operation was bad, because it locked 2 
> blocks
> instead of one.
> 
>> >> >                2 | 110b         | 010b      |
> 
> - lock 2 erase blocks starting from offset 0. Results before this 
> patch, and
> after this patch. Continue the logic on the following lines.
> 
> oops there's a typo in column 2, sorry. The value in column 2 should 
> have been
> 011b.
> 
> So before this patch, when one requested to lock 2 block starting from 
> offset
> 0, we would obtain 4 blocks locked, and he should have obtained just 2.
> 
> The scope of this patch is to first fix the locking ops, so that we can
> introduce a more generic formula that gives the same results as before
> introducing it. Without this patch, the new formula will silently fix 
> the bug
> that is described here.
> 
>> >> >                3 | 110b         | 010b      |
> 		^ typo s/110b/011b
> 
> rest of the examples are good.
> 
> Cheers,
> ta
> 
>> >> >                4 | 100b         | 011b      |
>> >> >                5 | 100b         | 011b      |
>> >> >                6 | 100b         | 011b      |
>> >> >                7 | 100b         | 011b      |
>> >> >                8 | 101b         | 100b      |
>> >> >                9 | 101b         | 100b      |
>> >> >
>> >> >              ... | ...          | ...       |
>> >> >
>> >> > For the lock operation, if one requests to lock an area that is not
>> >> > matching the upper boundary of a BP protected area, we round down
>> >> > the total length and lock less than the user requested, in order to
>> >> > not lock more than the user actually requested.

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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com:
> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> the
> >> content is safe
> >> 
> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com:
> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> >> the
> >> >> content is safe
> >> >> 
> >> >> Hi,
> >> >> 
> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> >> > 
> >> >> > Fix the gap for the SR block protection, the BP bits were set with
> >> >> > a +1 value than actually needed. This patch does not change the
> >> >> > behavior of the locking operations, just fixes the protected areas.
> >> >> 
> >> >> So instead of rounding up, it does round down now?
> >> > 
> >> > No. Why do you say that it rounds up? The behavior is not changed, the
> >> > patch
> >> > merely fix the protected area, which was wrong before. The round down
> >> > is
> >> > present before this patch.
> >> 
> >> TBH I don't understand what this patch should do. Could you give an
> >> example?
> > 
> > sure, let me try to be more explicit.
> > 
> >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed
> >> 
> >> >> > for the lock operation:
> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use
> >> a
> > 
> > it's not.
> 
> If I'm not mistaken this falls into the same category like the new 4bits
> BP
> flashes, because there are more slots free than needed. Ie. the last one
> "protect all" is either 110b or 111b and thus don't work with the old
> formula. This was actually my reason why there is no new formula for the
> 4 bits BP flashes; but the current one is not working with flashes
> <32Mbit.
> See the old long thread.
> 
> >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg.
> >> a
> >> 50/50 split. I haven't seen any issued. Shouldn't it be then
> >> completely
> >> locked according this the following example?
> > 
> > I don't follow.
> 
> We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on
> our
> boards to lock the first half of our 4MiB flash.
> 
> > The table from below was generated for the S25FL116K 16 Mbit flash.
> > BTW, one
> > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the
> > locking.
> > When you have a 4k sector erase, the locking is simply wrong, but this
> > is
> > another topic.
> 
> it should work with that too if you convert the number to the smaller
> sectors,
> ie multiply by 16; But yeah the cli tool has a broken interface. It
> should
> accept both offset and length in bytes; not one one in bytes and one in
> sectors,
> where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS.
> 
> >> >> > Number of blocks | BP2:0 before | BP2:0 now |
> >> >> > 
> >> >> >                1 | 010b         | 001b      |
> > 
> > - number of blocks is how many blocks you want to lock. One would do
> > for one
> > 
> > block:
> >     flash_lock /dev/mtd 0 1
> > 
> > i.e. lock a single erase block starting from offset 0.
> > 
> > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0
> > 1"
> > before this patch
> 
> Without your patch applied it works like expected:
> 
> [    1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes)
> # flash_lock -l /dev/mtd1 0 1
> # cat
> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
> a4
> 
> A4 is 1010_0100, ie BP[2:0] = 001b and TB=1
> 

what happens if you request flash_lock -l /dev/mtd1 0 3?

> # flash_lock -u /dev/mtd1 0 64
> # flash_lock -l /dev/mtd1 0 32
> # cat
> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
> b8
> 
> 
> With this patch applied:
> 
> # flash_lock -u /dev/mtd1 0 64
> # cat
> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
> 00
> # flash_lock -l /dev/mtd1 0 1
> flash_lock: error!: could not lock device: /dev/mtd1
> 
>              error 22 (Invalid argument)

I'm wondering what was the reason for the -EINVAL.

> # flash_lock -l /dev/mtd1 0 2
> # cat
> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
> a4
> 
> which is wrong, isn't it?
> 
Looks so. You should have obtained, 0xa8, right? Will recheck tomorrow 
morning.

Thanks for testing this! I don't have a 32Mbit flash ...

Cheers,
ta
> 
> > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1"
> > using
> > this patch
> > 
> > So before this patch, the lock operation was bad, because it locked 2
> > blocks
> > instead of one.
> > 
> >> >> >                2 | 110b         | 010b      |
> > 
> > - lock 2 erase blocks starting from offset 0. Results before this
> > patch, and
> > after this patch. Continue the logic on the following lines.
> > 
> > oops there's a typo in column 2, sorry. The value in column 2 should
> > have been
> > 011b.
> > 
> > So before this patch, when one requested to lock 2 block starting from
> > offset
> > 0, we would obtain 4 blocks locked, and he should have obtained just 2.
> > 
> > The scope of this patch is to first fix the locking ops, so that we can
> > introduce a more generic formula that gives the same results as before
> > introducing it. Without this patch, the new formula will silently fix
> > the bug
> > that is described here.
> > 
> >> >> >                3 | 110b         | 010b      |
> >               
> >               ^ typo s/110b/011b
> > 
> > rest of the examples are good.
> > 
> > Cheers,
> > ta
> > 
> >> >> >                4 | 100b         | 011b      |
> >> >> >                5 | 100b         | 011b      |
> >> >> >                6 | 100b         | 011b      |
> >> >> >                7 | 100b         | 011b      |
> >> >> >                8 | 101b         | 100b      |
> >> >> >                9 | 101b         | 100b      |
> >> >> >              
> >> >> >              ... | ...          | ...       |
> >> >> > 
> >> >> > For the lock operation, if one requests to lock an area that is not
> >> >> > matching the upper boundary of a BP protected area, we round down
> >> >> > the total length and lock less than the user requested, in order to
> >> >> > not lock more than the user actually requested.




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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

On Monday, March 23, 2020 11:30:44 PM EET Tudor Ambarus wrote:
> > With this patch applied:
> > 
> > # flash_lock -u /dev/mtd1 0 64
> > # cat
> > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
> > 00
> > # flash_lock -l /dev/mtd1 0 1
> > flash_lock: error!: could not lock device: /dev/mtd1
> > 
> > error 22 (Invalid argument)
> 
> I'm wondering what was the reason for the -EINVAL.

Probably because the BP bits would have value zero.



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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Michael Walle]

Am 2020-03-23 22:30, schrieb Tudor.Ambarus@microchip.com:
> On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com:
>> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote:
>> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> >> the
>> >> content is safe
>> >>
>> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com:
>> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
>> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> >> >> the
>> >> >> content is safe
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
>> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
>> >> >> >
>> >> >> > Fix the gap for the SR block protection, the BP bits were set with
>> >> >> > a +1 value than actually needed. This patch does not change the
>> >> >> > behavior of the locking operations, just fixes the protected areas.
>> >> >>
>> >> >> So instead of rounding up, it does round down now?
>> >> >
>> >> > No. Why do you say that it rounds up? The behavior is not changed, the
>> >> > patch
>> >> > merely fix the protected area, which was wrong before. The round down
>> >> > is
>> >> > present before this patch.
>> >>
>> >> TBH I don't understand what this patch should do. Could you give an
>> >> example?
>> >
>> > sure, let me try to be more explicit.
>> >
>> >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed
>> >>
>> >> >> > for the lock operation:
>> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use
>> >> a
>> >
>> > it's not.
>> 
>> If I'm not mistaken this falls into the same category like the new 
>> 4bits
>> BP
>> flashes, because there are more slots free than needed. Ie. the last 
>> one
>> "protect all" is either 110b or 111b and thus don't work with the old
>> formula. This was actually my reason why there is no new formula for 
>> the
>> 4 bits BP flashes; but the current one is not working with flashes
>> <32Mbit.
>> See the old long thread.
>> 
>> >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg.
>> >> a
>> >> 50/50 split. I haven't seen any issued. Shouldn't it be then
>> >> completely
>> >> locked according this the following example?
>> >
>> > I don't follow.
>> 
>> We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on
>> our
>> boards to lock the first half of our 4MiB flash.
>> 
>> > The table from below was generated for the S25FL116K 16 Mbit flash.
>> > BTW, one
>> > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the
>> > locking.
>> > When you have a 4k sector erase, the locking is simply wrong, but this
>> > is
>> > another topic.
>> 
>> it should work with that too if you convert the number to the smaller
>> sectors,
>> ie multiply by 16; But yeah the cli tool has a broken interface. It
>> should
>> accept both offset and length in bytes; not one one in bytes and one 
>> in
>> sectors,
>> where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS.
>> 
>> >> >> > Number of blocks | BP2:0 before | BP2:0 now |
>> >> >> >
>> >> >> >                1 | 010b         | 001b      |
>> >
>> > - number of blocks is how many blocks you want to lock. One would do
>> > for one
>> >
>> > block:
>> >     flash_lock /dev/mtd 0 1
>> >
>> > i.e. lock a single erase block starting from offset 0.
>> >
>> > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0
>> > 1"
>> > before this patch
>> 
>> Without your patch applied it works like expected:
>> 
>> [    1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes)
>> # flash_lock -l /dev/mtd1 0 1
>> # cat
>> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
>> a4
>> 
>> A4 is 1010_0100, ie BP[2:0] = 001b and TB=1
>> 
> 
> what happens if you request flash_lock -l /dev/mtd1 0 3?

with your patch applied:

# flash_lock -u /dev/mtd1 0 64
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
00
# flash_lock -l /dev/mtd1 0 3
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
a4


without it:

# flash_lock -u /dev/mtd1 0 64
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
00
# flash_lock -l /dev/mtd1 0 3
# cat 
/sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
a8


>> # flash_lock -u /dev/mtd1 0 64
>> # flash_lock -l /dev/mtd1 0 32
>> # cat
>> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
>> b8
>> 
>> 
>> With this patch applied:
>> 
>> # flash_lock -u /dev/mtd1 0 64
>> # cat
>> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
>> 00
>> # flash_lock -l /dev/mtd1 0 1
>> flash_lock: error!: could not lock device: /dev/mtd1
>> 
>>              error 22 (Invalid argument)
> 
> I'm wondering what was the reason for the -EINVAL.
> 
>> # flash_lock -l /dev/mtd1 0 2
>> # cat
>> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg
>> a4
>> 
>> which is wrong, isn't it?
>> 
> Looks so. You should have obtained, 0xa8, right?

correct, BP should be 010b for the first two sectors.

> Will recheck tomorrow
> morning.
> 
> Thanks for testing this! I don't have a 32Mbit flash ...

You should be able to reproduce it with every flash >=32Mbit which has
3 BP bits.

-michael

> 
> Cheers,
> ta
>> 
>> > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1"
>> > using
>> > this patch
>> >
>> > So before this patch, the lock operation was bad, because it locked 2
>> > blocks
>> > instead of one.
>> >
>> >> >> >                2 | 110b         | 010b      |
>> >
>> > - lock 2 erase blocks starting from offset 0. Results before this
>> > patch, and
>> > after this patch. Continue the logic on the following lines.
>> >
>> > oops there's a typo in column 2, sorry. The value in column 2 should
>> > have been
>> > 011b.
>> >
>> > So before this patch, when one requested to lock 2 block starting from
>> > offset
>> > 0, we would obtain 4 blocks locked, and he should have obtained just 2.
>> >
>> > The scope of this patch is to first fix the locking ops, so that we can
>> > introduce a more generic formula that gives the same results as before
>> > introducing it. Without this patch, the new formula will silently fix
>> > the bug
>> > that is described here.
>> >
>> >> >> >                3 | 110b         | 010b      |
>> >
>> >               ^ typo s/110b/011b
>> >
>> > rest of the examples are good.
>> >
>> > Cheers,
>> > ta
>> >
>> >> >> >                4 | 100b         | 011b      |
>> >> >> >                5 | 100b         | 011b      |
>> >> >> >                6 | 100b         | 011b      |
>> >> >> >                7 | 100b         | 011b      |
>> >> >> >                8 | 101b         | 100b      |
>> >> >> >                9 | 101b         | 100b      |
>> >> >> >
>> >> >> >              ... | ...          | ...       |
>> >> >> >
>> >> >> > For the lock operation, if one requests to lock an area that is not
>> >> >> > matching the upper boundary of a BP protected area, we round down
>> >> >> > the total length and lock less than the user requested, in order to
>> >> >> > not lock more than the user actually requested.

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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Jungseung Lee]

Hi, Tudor,

On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Fix the gap for the SR block protection, the BP bits were set with
> a +1 value than actually needed. This patch does not change the
> behavior of the locking operations, just fixes the protected areas.
> 
> On a 16Mbit flash with 64KByte erase sector, the following changed
> for the lock operation:
> 
> Number of blocks | BP2:0 before | BP2:0 now |
>                1 | 010b         | 001b      |
>                2 | 110b         | 010b      |
>                3 | 110b         | 010b      |
>                4 | 100b         | 011b      |
>                5 | 100b         | 011b      |
>                6 | 100b         | 011b      |
>                7 | 100b         | 011b      |
>                8 | 101b         | 100b      |
>                9 | 101b         | 100b      |
>              ... | ...          | ...       |
> 
> For the lock operation, if one requests to lock an area that is not
> matching the upper boundary of a BP protected area, we round down
> the total length and lock less than the user requested, in order to
> not lock more than the user actually requested.
> 
> For the unlock operation, read the number of blocks column as
> "locked all but number of blocks value". On a 16Mbit flash with
> 64KByte erase sector, the following changed for the lock operation:
> 
> Number of blocks | BP2:0 before | BP2:0 now |
>                1 | 111b         | 101b      |
>              ... | ...          | ...       |
>               15 | 111b         | 101b      |
>               16 | 110b         | 101b      |
>               17 | 110b         | 100b      |
>              ... | ...          | ...       |
>               24 | 101b         | 100b      |
>               25 | 101b         | 011b      |
>               26 | 101b         | 011b      |
>               27 | 101b         | 011b      |
>               28 | 100b         | 011b      |
>               29 | 100b         | 010b      |
>               30 | 011b         | 010b      |
>               31 | 010b         | 001b      |
>               32 | 000b         | 000b      |
> 
> For the unlock operation, if one requests to unlock an area that is
> not matching the upper boundary of a BP protected area, we round up
> the total length and unlock more than the user actually requested.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 877557dbda7f..36660068bc04 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	/*
>  	 * Need smallest pow such that:
>  	 *
> -	 *   1 / (2^pow) <= (len / size)
> +	 *   1 / ((2^pow) - 1) <= (len / size)
>  	 *
>  	 * so (assuming power-of-2 size) we do:
>  	 *
> -	 *   pow = ceil(log2(size / len)) = log2(size) -
> floor(log2(len))
> +	 *   pow = ceil(log2(size / len)) = log2(size) -
> floor(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - ilog2(lock_len);
> +	pow = ilog2(mtd->size) - ilog2(lock_len) + 1;
>  	val = mask - (pow << SR_BP_SHIFT);
>  	if (val & ~mask)
>  		return -EINVAL;

    (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;

I think that current mainline implementation is only valid for flashes
in case (1). (for BP0-2 : 1/64, 1/32 ...) Isn't it?

This is current implementaion.
	pow = ilog2(mtd->size) - order_base_2(lock_len)
	val = mask - (pow << SR_BP_SHIFT);

1/64 6 = 110b -> 001b
1/32 5 = 101b -> 010b
1/16 4 = 100b -> 011b
1/8  3 = 011b -> 100b
1/4  2 = 010b -> 101b
1/2  1 = 001b -> 110b
ALL  0 = 000b -> 111b

It is handling case (1) admirably. In other cases, however, the
situation would be different.


A 16Mbit flash with 64KByte erase sector(which you mentioned on this
patch) should get 32 erase sectors and seems that it's smallest
protected portion is 1/32.

So supporting the flash, it needs some modification as you did.

	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1
	val = mask - (pow << SR_BP_SHIFT);

1/64 6 = 110b 111b -> 000b (execption case)
1/32 5 = 101b 110b -> 001b
1/16 4 = 100b 101b -> 010b
1/8  3 = 011b 100b -> 011b
1/4  2 = 010b 011b -> 100b
1/2  1 = 001b 010b -> 101b
ALL  0 = 000b 001b -> 110b

It would works for the flash, but it will conflicts with flashes in
case (1) what has already been applied on mainline.

And there are too various flashes that has different protected portions
to handle with the above.

Btw, the description on this patch is exactly what I had been looking
for before and seems it's very useful.

Thanks,


> @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor
> *nor, loff_t ofs, uint64_t len)
>  	/*
>  	 * Need largest pow such that:
>  	 *
> -	 *   1 / (2^pow) >= (len / size)
> +	 *   1 / ((2^pow) - 1) >= (len / size)
>  	 *
>  	 * so (assuming power-of-2 size) we do:
>  	 *
> -	 *   pow = floor(log2(size / len)) = log2(size) -
> ceil(log2(len))
> +	 *   pow = floor(log2(size / len)) = log2(size) -
> ceil(log2(len)) + 1
>  	 */
> -	pow = ilog2(mtd->size) - order_base_2(lock_len);
> +	pow = ilog2(mtd->size) - order_base_2(lock_len) + 1;
>  	if (lock_len == 0) {
>  		val = 0; /* fully unlocked */
>  	} else {


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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

On Tuesday, March 24, 2020 12:35:30 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> Am 2020-03-23 22:30, schrieb Tudor.Ambarus@microchip.com:
> > On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> the
> >> content is safe
> >> 
> >> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com:
> >> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote:
> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> >> the
> >> >> content is safe
> >> >> 
> >> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com:
> >> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote:
> >> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >> >> >> know
> >> >> >> the
> >> >> >> content is safe
> >> >> >> 
> >> >> >> Hi,
> >> >> >> 
> >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com:
> >> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> >> >> > 
> >> >> >> > Fix the gap for the SR block protection, the BP bits were set
> >> >> >> > with
> >> >> >> > a +1 value than actually needed. This patch does not change the
> >> >> >> > behavior of the locking operations, just fixes the protected
> >> >> >> > areas.
> >> >> >> 
> >> >> >> So instead of rounding up, it does round down now?
> >> >> > 
> >> >> > No. Why do you say that it rounds up? The behavior is not changed,
> >> >> > the
> >> >> > patch
> >> >> > merely fix the protected area, which was wrong before. The round
> >> >> > down
> >> >> > is
> >> >> > present before this patch.
> >> >> 
> >> >> TBH I don't understand what this patch should do. Could you give an
> >> >> example?
> >> > 
> >> > sure, let me try to be more explicit.
> >> > 
> >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following
> >> >> >> > changed
> >> >> 
> >> >> >> > for the lock operation:
> >> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use
> >> >> a
> >> > 
> >> > it's not.
> >> 
> >> If I'm not mistaken this falls into the same category like the new
> >> 4bits
> >> BP
> >> flashes, because there are more slots free than needed. Ie. the last
> >> one
> >> "protect all" is either 110b or 111b and thus don't work with the old
> >> formula. This was actually my reason why there is no new formula for
> >> the
> >> 4 bits BP flashes; but the current one is not working with flashes
> >> <32Mbit.
> >> See the old long thread.

You're right, I was trying to fix a dead horse. 16Mbit BP0:2 flashes are fixed 
with the generic formula. I'm going to respin without this patch.

Thanks!


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

Re: [PATCH v3 1/5] mtd: spi-nor: Fix gap in SR block protection locking

[Tudor.Ambarus]

On Monday, March 23, 2020 11:24:33 AM EET Tudor Ambarus - M18064 wrote:
> For the unlock operation, read the number of blocks column as
> "locked all but number of blocks value". On a 16Mbit flash with
> 64KByte erase sector, the following changed for the lock operation:
                                                                                         ^
typo s/lock/unlock




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