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

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

[Jungseung Lee]

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

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

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

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

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

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

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

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


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

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

[Jungseung Lee]

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

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c58c27552a74..31a2106e529a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,6 +238,14 @@ struct flash_info {
 					 * status register. Must be used with
 					 * SPI_NOR_HAS_TB.
 					 */
+#define SPI_NOR_4BIT_BP		BIT(17)	/*
+					 * Flash SR has 4 bit fields (BP0-3)
+					 * for block protection.
+					 */
+#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
+					 * BP3 is bit 6 of status register.
+					 * Must be used with SPI_NOR_4BIT_BP.
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
 {
-	return SR_BP2 | SR_BP1 | SR_BP0;
+	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			mask = mask | SR_BP3_BIT6;
+		else
+			mask = mask | SR_BP3_BIT5;
+	}
+
+	return mask;
 }
 
 static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
@@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
 		return SR_TB_BIT5;
 }
 
+static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
+	u8 mask = spi_nor_get_bp_mask(nor);
+	u8 bp = sr & mask;
+
+	if (bp & SR_BP3_BIT6)
+		bp = (bp & ~BIT(6)) | BIT(5);
+
+	bp = bp >> SR_BP_SHIFT;
+
+	return bp;
+}
+
 static int stm_get_min_prot_length(struct spi_nor *nor)
 {
 	int bp_slots, bp_slots_needed;
-	u8 mask = spi_nor_get_bp_mask(nor);
 
-	bp_slots = (mask >> SR_BP_SHIFT) + 1;
+	if (nor->flags & SNOR_F_HAS_4BIT_BP)
+		bp_slots = 1 << 4;
+	else
+		bp_slots = 1 << 3;
 
 	/* Reserved one for "protect none" and one for "protect all". */
 	bp_slots = bp_slots - 2;
@@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 {
 	struct mtd_info *mtd = &nor->mtd;
 	int min_prot_len;
-	u8 mask = spi_nor_get_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_tb_mask(nor);
-	u8 bp = (sr & mask) >> SR_BP_SHIFT;
+	u8 bp = stm_get_bpval_from_sr(nor, sr);
 
 	if (!bp) {
 		/* No protection */
@@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 
 /*
  * Lock a region of the flash. Compatible with ST Micro and similar flash.
- * Supports the block protection bits BP{0,1,2} in the status register
+ * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the status register
  * (SR). Does not support these features found in newer SR bitfields:
  *   - SEC: sector/block protect - only handle SEC=0 (block protect)
  *   - CMP: complement protect - only support CMP=0 (range is not complemented)
@@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
  * Support for the following is provided conditionally for some flash:
  *   - TB: top/bottom protect
  *
- * Sample table portion for 8MB flash (Winbond w25q64fw):
+ * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
  *
  *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
  *  --------------------------------------------------------------------------
@@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
  *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower 1/4
  *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower 1/2
  *
+ * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-3):
+ *
+ *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
+ *  --------------------------------------------------------------------------
+ *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
+ *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper 1/1024
+ *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper 1/512
+ *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper 1/256
+ *   ...
+ *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper 1/4
+ *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper 1/2
+ *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
+ *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
+ *   ...
+ *  ------|-------|-------|-------|-------|---------------|-------------------
+ *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
+ *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower 1/1024
+ *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower 1/512
+ *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower 1/256
+ *   ...
+ *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower 1/4
+ *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower 1/2
+ *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
+ *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
+ *   ...
+ *
  * Returns negative on errors, 0 on success.
  */
 static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
@@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 		min_prot_len = stm_get_min_prot_length(nor);
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 		val = pow << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
 	}
 
 	if (val & ~mask)
@@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
 		val = pow << SR_BP_SHIFT;
 
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
+
 		/* Some power-of-two sizes are not supported */
 		if (val & ~mask)
 			return -EINVAL;
@@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (info->flags & USE_CLSR)
 		nor->flags |= SNOR_F_USE_CLSR;
 
+	if (info->flags & SPI_NOR_4BIT_BP) {
+		nor->flags |= SNOR_F_HAS_4BIT_BP;
+		if (info->flags & SPI_NOR_BP3_SR_BIT6)
+			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
+	}
+
 	if (info->flags & SPI_NOR_NO_ERASE)
 		mtd->flags |= MTD_NO_ERASE;
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index de90724f62f1..0190ed21576a 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -129,7 +129,9 @@
 #define SR_BP1			BIT(3)	/* Block protect 1 */
 #define SR_BP2			BIT(4)	/* Block protect 2 */
 #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
+#define SR_BP3_BIT5		BIT(5)	/* Block protect 3 */
 #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
+#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 /* Spansion/Cypress specific status bits */
 #define SR_E_ERR		BIT(5)
@@ -240,6 +242,8 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_16BIT_SR	= BIT(9),
 	SNOR_F_NO_READ_CR	= BIT(10),
 	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
+	SNOR_F_HAS_4BIT_BP	= BIT(12),
+	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
 
 };
 
-- 
2.17.1


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

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

[Michael Walle]

Hi Jungseung,

sorry for the late review.

Am 2020-03-04 12:07, schrieb Jungseung Lee:
> Currently, we are supporting block protection only for
> flash chips with 3 block protection bits in the SR register.
> This patch enables block protection support for flashes with
> 4 block protection bits(bp0-3).
> 
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 82 ++++++++++++++++++++++++++++++++---
>  include/linux/mtd/spi-nor.h   |  4 ++
>  2 files changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> b/drivers/mtd/spi-nor/spi-nor.c
> index c58c27552a74..31a2106e529a 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -238,6 +238,14 @@ struct flash_info {
>  					 * status register. Must be used with
>  					 * SPI_NOR_HAS_TB.
>  					 */
> +#define SPI_NOR_4BIT_BP		BIT(17)	/*
> +					 * Flash SR has 4 bit fields (BP0-3)
> +					 * for block protection.
> +					 */
> +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
> +					 * BP3 is bit 6 of status register.
> +					 * Must be used with SPI_NOR_4BIT_BP.
> +					 */
> 
>  	/* Part specific fixup hooks. */
>  	const struct spi_nor_fixups *fixups;
> @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd,
> struct erase_info *instr)
> 
>  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>  {
> -	return SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +

can we just use the SR_BP3 eg:

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


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

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

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

Then just keep this.

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

also this.

> 
>  	if (!bp) {
>  		/* No protection */
> @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> *nor, loff_t ofs, uint64_t len,
> 
>  /*
>   * Lock a region of the flash. Compatible with ST Micro and similar 
> flash.
> - * Supports the block protection bits BP{0,1,2} in the status register
> + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the
> status register
>   * (SR). Does not support these features found in newer SR bitfields:
>   *   - SEC: sector/block protect - only handle SEC=0 (block protect)
>   *   - CMP: complement protect - only support CMP=0 (range is not 
> complemented)
> @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> *nor, loff_t ofs, uint64_t len,
>   * Support for the following is provided conditionally for some flash:
>   *   - TB: top/bottom protect
>   *
> - * Sample table portion for 8MB flash (Winbond w25q64fw):
> + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
>   *
>   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected 
> Portion
>   *  
> --------------------------------------------------------------------------
> @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct spi_nor
> *nor, loff_t ofs, uint64_t len,
>   *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower 1/4
>   *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower 1/2
>   *
> + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-3):
> + *
> + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected 
> Portion
> + *  
> --------------------------------------------------------------------------
> + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
> + *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper 
> 1/1024
> + *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper 
> 1/512
> + *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper 
> 1/256
> + *   ...
> + *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper 1/4
> + *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper 1/2
> + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> + *   ...
> + *  
> ------|-------|-------|-------|-------|---------------|-------------------
> + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
> + *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower 
> 1/1024
> + *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower 
> 1/512
> + *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower 
> 1/256
> + *   ...
> + *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower 1/4
> + *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower 1/2
> + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> + *   ...
> + *
>   * Returns negative on errors, 0 on success.
>   */
>  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t
> ofs, uint64_t len)
>  		min_prot_len = stm_get_min_prot_length(nor);
>  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>  		val = pow << SR_BP_SHIFT;
> +
> +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> +			val = (val & ~BIT(5)) | BIT(6);

.. and the use just the following here:

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

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

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

here would be the other way around:

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


>  		/* Some power-of-two sizes are not supported */
>  		if (val & ~mask)
>  			return -EINVAL;
> @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char 
> *name,
>  	if (info->flags & USE_CLSR)
>  		nor->flags |= SNOR_F_USE_CLSR;
> 
> +	if (info->flags & SPI_NOR_4BIT_BP) {
> +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> +	}
> +
>  	if (info->flags & SPI_NOR_NO_ERASE)
>  		mtd->flags |= MTD_NO_ERASE;
> 
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de90724f62f1..0190ed21576a 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -129,7 +129,9 @@
>  #define SR_BP1			BIT(3)	/* Block protect 1 */
>  #define SR_BP2			BIT(4)	/* Block protect 2 */
>  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
> +#define SR_BP3_BIT5		BIT(5)	/* Block protect 3 */

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

-michael

>  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
> +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
>  #define SR_SRWD			BIT(7)	/* SR write protect */
>  /* Spansion/Cypress specific status bits */
>  #define SR_E_ERR		BIT(5)
> @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
>  	SNOR_F_HAS_16BIT_SR	= BIT(9),
>  	SNOR_F_NO_READ_CR	= BIT(10),
>  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> +	SNOR_F_HAS_4BIT_BP	= BIT(12),
> +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> 
>  };

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

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

[Jungseung Lee]

Hi, Michael,

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

Not at all. thanks for your review.

> Am 2020-03-04 12:07, schrieb Jungseung Lee:
> > Currently, we are supporting block protection only for
> > flash chips with 3 block protection bits in the SR register.
> > This patch enables block protection support for flashes with
> > 4 block protection bits(bp0-3).
> > 
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 82
> > ++++++++++++++++++++++++++++++++---
> >  include/linux/mtd/spi-nor.h   |  4 ++
> >  2 files changed, 79 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> > b/drivers/mtd/spi-nor/spi-nor.c
> > index c58c27552a74..31a2106e529a 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -238,6 +238,14 @@ struct flash_info {
> >  					 * status register. Must be
> > used with
> >  					 * SPI_NOR_HAS_TB.
> >  					 */
> > +#define SPI_NOR_4BIT_BP		BIT(17)	/*
> > +					 * Flash SR has 4 bit fields
> > (BP0-3)
> > +					 * for block protection.
> > +					 */
> > +#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
> > +					 * BP3 is bit 6 of status
> > register.
> > +					 * Must be used with
> > SPI_NOR_4BIT_BP.
> > +					 */
> > 
> >  	/* Part specific fixup hooks. */
> >  	const struct spi_nor_fixups *fixups;
> > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
> > *mtd,
> > struct erase_info *instr)
> > 
> >  static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> >  {
> > -	return SR_BP2 | SR_BP1 | SR_BP0;
> > +	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > +
> 
> can we just use the SR_BP3 eg:
> 
> 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> 		mask |= SR_BP3;
> 	return mask;
> 
> 

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

> > +	if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > +		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > +			mask = mask | SR_BP3_BIT6;
> > +		else
> > +			mask = mask | SR_BP3_BIT5;
> > +	}
> > +
> > +	return mask;
> >  }
> > 
> >  static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > spi_nor 
> > *nor)
> >  		return SR_TB_BIT5;
> >  }
> > 
> > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> > +	u8 mask = spi_nor_get_bp_mask(nor);
> > +	u8 bp = sr & mask;
> > +
> > +	if (bp & SR_BP3_BIT6)
> > +		bp = (bp & ~BIT(6)) | BIT(5);
> > +
> > +	bp = bp >> SR_BP_SHIFT;
> > +
> > +	return bp;
> > +}
> 
> Don't convert this. It makes the code really hard to read. See below.
> 
> > +
> >  static int stm_get_min_prot_length(struct spi_nor *nor)
> >  {
> >  	int bp_slots, bp_slots_needed;
> > -	u8 mask = spi_nor_get_bp_mask(nor);
> > 
> > -	bp_slots = (mask >> SR_BP_SHIFT) + 1;
> 
> Then just keep this.
> 
> > +	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > +		bp_slots = 1 << 4;
> > +	else
> > +		bp_slots = 1 << 3;
> > 
> >  	/* Reserved one for "protect none" and one for "protect all".
> > */
> >  	bp_slots = bp_slots - 2;
> > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > spi_nor
> > *nor, u8 sr, loff_t *ofs,
> >  {
> >  	struct mtd_info *mtd = &nor->mtd;
> >  	int min_prot_len;
> > -	u8 mask = spi_nor_get_bp_mask(nor);
> >  	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > -	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > +	u8 bp = stm_get_bpval_from_sr(nor, sr);
> 
> also this.
> 
> > 
> >  	if (!bp) {
> >  		/* No protection */
> > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> > 
> >  /*
> >   * Lock a region of the flash. Compatible with ST Micro and
> > similar 
> > flash.
> > - * Supports the block protection bits BP{0,1,2} in the status
> > register
> > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the
> > status register
> >   * (SR). Does not support these features found in newer SR
> > bitfields:
> >   *   - SEC: sector/block protect - only handle SEC=0 (block
> > protect)
> >   *   - CMP: complement protect - only support CMP=0 (range is not 
> > complemented)
> > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> >   * Support for the following is provided conditionally for some
> > flash:
> >   *   - TB: top/bottom protect
> >   *
> > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
> >   *
> >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > Protected 
> > Portion
> >   *  
> > -----------------------------------------------------------------
> > ---------
> > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> >   *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower
> > 1/4
> >   *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower
> > 1/2
> >   *
> > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
> > 3):
> > + *
> > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > Protected 
> > Portion
> > + *  
> > -----------------------------------------------------------------
> > ---------
> > + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
> > + *    0   |   0   |   0   |   0   |   1   |   64 KB       | Upper 
> > 1/1024
> > + *    0   |   0   |   0   |   1   |   0   |  128 KB       | Upper 
> > 1/512
> > + *    0   |   0   |   0   |   1   |   1   |  256 KB       | Upper 
> > 1/256
> > + *   ...
> > + *    0   |   1   |   0   |   0   |   1   |  16 MB        | Upper
> > 1/4
> > + *    0   |   1   |   0   |   1   |   0   |  32 MB        | Upper
> > 1/2
> > + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> > + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> > + *   ...
> > + *  
> > ------|-------|-------|-------|-------|---------------|----------
> > ---------
> > + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
> > + *    1   |   0   |   0   |   0   |   1   |   64 KB       | Lower 
> > 1/1024
> > + *    1   |   0   |   0   |   1   |   0   |  128 KB       | Lower 
> > 1/512
> > + *    1   |   0   |   0   |   1   |   1   |  256 KB       | Lower 
> > 1/256
> > + *   ...
> > + *    1   |   1   |   0   |   0   |   1   |  16 MB        | Lower
> > 1/4
> > + *    1   |   1   |   0   |   1   |   0   |  32 MB        | Lower
> > 1/2
> > + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> > + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> > + *   ...
> > + *
> >   * Returns negative on errors, 0 on success.
> >   */
> >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
> > loff_t
> > ofs, uint64_t len)
> >  		min_prot_len = stm_get_min_prot_length(nor);
> >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> >  		val = pow << SR_BP_SHIFT;
> > +
> > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > +			val = (val & ~BIT(5)) | BIT(6);
> 
> .. and the use just the following here:
> 
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
> 
> Ie. just use the "normal case" where all BP bits are next to each
> other
> and then fixup the resulting value and shift the SR3 bit if
> necessary.
> This will be much easier to read.
> 

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

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

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

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

Thanks,

> -michael
> 
> >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
> > +#define SR_BP3_BIT6		BIT(6)	/* Block protect 3
> > */
> >  #define SR_SRWD			BIT(7)	/* SR write protect
> > */
> >  /* Spansion/Cypress specific status bits */
> >  #define SR_E_ERR		BIT(5)
> > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
> >  	SNOR_F_NO_READ_CR	= BIT(10),
> >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> > +	SNOR_F_HAS_4BIT_BP	= BIT(12),
> > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > 
> >  };
> 


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

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

[Jungseung Lee]

Hi, Micahel

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

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

Thanks,

> > > 
> > >  	if (!bp) {
> > >  		/* No protection */
> > > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct
> > > spi_nor
> > > *nor, loff_t ofs, uint64_t len,
> > > 
> > >  /*
> > >   * Lock a region of the flash. Compatible with ST Micro and
> > > similar 
> > > flash.
> > > - * Supports the block protection bits BP{0,1,2} in the status
> > > register
> > > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in
> > > the
> > > status register
> > >   * (SR). Does not support these features found in newer SR
> > > bitfields:
> > >   *   - SEC: sector/block protect - only handle SEC=0 (block
> > > protect)
> > >   *   - CMP: complement protect - only support CMP=0 (range is
> > > not 
> > > complemented)
> > > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct
> > > spi_nor
> > > *nor, loff_t ofs, uint64_t len,
> > >   * Support for the following is provided conditionally for some
> > > flash:
> > >   *   - TB: top/bottom protect
> > >   *
> > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-
> > > 2):
> > >   *
> > >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > > Protected 
> > > Portion
> > >   *  
> > > -----------------------------------------------------------------
> > > ---------
> > > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct
> > > spi_nor
> > > *nor, loff_t ofs, uint64_t len,
> > >   *    0   |   1   |   1   |   0   |   1   |  2 MB         |
> > > Lower
> > > 1/4
> > >   *    0   |   1   |   1   |   1   |   0   |  4 MB         |
> > > Lower
> > > 1/2
> > >   *
> > > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
> > > 3):
> > > + *
> > > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > > Protected 
> > > Portion
> > > + *  
> > > -----------------------------------------------------------------
> > > ---------
> > > + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
> > > + *    0   |   0   |   0   |   0   |   1   |   64 KB       |
> > > Upper 
> > > 1/1024
> > > + *    0   |   0   |   0   |   1   |   0   |  128 KB       |
> > > Upper 
> > > 1/512
> > > + *    0   |   0   |   0   |   1   |   1   |  256 KB       |
> > > Upper 
> > > 1/256
> > > + *   ...
> > > + *    0   |   1   |   0   |   0   |   1   |  16 MB        |
> > > Upper
> > > 1/4
> > > + *    0   |   1   |   0   |   1   |   0   |  32 MB        |
> > > Upper
> > > 1/2
> > > + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> > > + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> > > + *   ...
> > > + *  
> > > ------|-------|-------|-------|-------|---------------|----------
> > > ---------
> > > + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
> > > + *    1   |   0   |   0   |   0   |   1   |   64 KB       |
> > > Lower 
> > > 1/1024
> > > + *    1   |   0   |   0   |   1   |   0   |  128 KB       |
> > > Lower 
> > > 1/512
> > > + *    1   |   0   |   0   |   1   |   1   |  256 KB       |
> > > Lower 
> > > 1/256
> > > + *   ...
> > > + *    1   |   1   |   0   |   0   |   1   |  16 MB        |
> > > Lower
> > > 1/4
> > > + *    1   |   1   |   0   |   1   |   0   |  32 MB        |
> > > Lower
> > > 1/2
> > > + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
> > > + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
> > > + *   ...
> > > + *
> > >   * Returns negative on errors, 0 on success.
> > >   */
> > >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t
> > > len)
> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
> > > loff_t
> > > ofs, uint64_t len)
> > >  		min_prot_len = stm_get_min_prot_length(nor);
> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > >  		val = pow << SR_BP_SHIFT;
> > > +
> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > +			val = (val & ~BIT(5)) | BIT(6);
> > 
> > .. and the use just the following here:
> > 
> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> > 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
> > 
> > Ie. just use the "normal case" where all BP bits are next to each
> > other
> > and then fixup the resulting value and shift the SR3 bit if
> > necessary.
> > This will be much easier to read.
> > 
> 
> Yes, I agree. It would be better to minimize this kind of conversion
> in
> one place.
> 
> > >  	}
> > > 
> > >  	if (val & ~mask)
> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> > > loff_t ofs, uint64_t len)
> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > >  		val = pow << SR_BP_SHIFT;
> > > 
> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > +			val = (val & ~BIT(5)) | BIT(6);
> > > +
> > 
> > here would be the other way around:
> > 
> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> > 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
> > 
> > 
> > >  		/* Some power-of-two sizes are not supported */
> > >  		if (val & ~mask)
> > >  			return -EINVAL;
> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
> > > const
> > > char 
> > > *name,
> > >  	if (info->flags & USE_CLSR)
> > >  		nor->flags |= SNOR_F_USE_CLSR;
> > > 
> > > +	if (info->flags & SPI_NOR_4BIT_BP) {
> > > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> > > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > > +	}
> > > +
> > >  	if (info->flags & SPI_NOR_NO_ERASE)
> > >  		mtd->flags |= MTD_NO_ERASE;
> > > 
> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> > > nor.h
> > > index de90724f62f1..0190ed21576a 100644
> > > --- a/include/linux/mtd/spi-nor.h
> > > +++ b/include/linux/mtd/spi-nor.h
> > > @@ -129,7 +129,9 @@
> > >  #define SR_BP1			BIT(3)	/* Block protect
> > > 1
> > > */
> > >  #define SR_BP2			BIT(4)	/* Block protect
> > > 2
> > > */
> > >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom
> > > protect */
> > > +#define SR_BP3_BIT5		BIT(5)	/* Block protect
> > > 3
> > > */
> > 
> > IMHO just SR_BP3. but that is a matter of taste. But it is easier
> > on
> > the eye in the mask = SR_BP3 | SR_BP2 etc case.
> > 
> 
> SR_BP3 would be a more appropriate name if we could set the case with
> all BP bits next to each other as the "normal case."
> 
> I'm going to write patches based on latest spi-nor/next including
> what
> you mentioned.
> 
> Thanks,
> 
> > -michael
> > 
> > >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom
> > > protect */
> > > +#define SR_BP3_BIT6		BIT(6)	/* Block protect
> > > 3
> > > */
> > >  #define SR_SRWD			BIT(7)	/* SR write
> > > protect
> > > */
> > >  /* Spansion/Cypress specific status bits */
> > >  #define SR_E_ERR		BIT(5)
> > > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> > >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
> > >  	SNOR_F_NO_READ_CR	= BIT(10),
> > >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> > > +	SNOR_F_HAS_4BIT_BP	= BIT(12),
> > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > > 
> > >  };
> 
> 


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

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

[Michael Walle]

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

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

What do you think about:

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

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

u8 spi_nor_get_bp_val(u8 sr)
{
     u8 val;

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

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

     return val >> SR_BP_SHIFT;
}

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

     val <<= SR_BP_SHIFT;

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

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

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

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


-michael

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


}


-michael

> 
> Thanks,
> 
>> > >
>> > >  	if (!bp) {
>> > >  		/* No protection */
>> > > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct
>> > > spi_nor
>> > > *nor, loff_t ofs, uint64_t len,
>> > >
>> > >  /*
>> > >   * Lock a region of the flash. Compatible with ST Micro and
>> > > similar
>> > > flash.
>> > > - * Supports the block protection bits BP{0,1,2} in the status
>> > > register
>> > > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in
>> > > the
>> > > status register
>> > >   * (SR). Does not support these features found in newer SR
>> > > bitfields:
>> > >   *   - SEC: sector/block protect - only handle SEC=0 (block
>> > > protect)
>> > >   *   - CMP: complement protect - only support CMP=0 (range is
>> > > not
>> > > complemented)
>> > > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct
>> > > spi_nor
>> > > *nor, loff_t ofs, uint64_t len,
>> > >   * Support for the following is provided conditionally for some
>> > > flash:
>> > >   *   - TB: top/bottom protect
>> > >   *
>> > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
>> > > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-
>> > > 2):
>> > >   *
>> > >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
>> > > Protected
>> > > Portion
>> > >   *
>> > > -----------------------------------------------------------------
>> > > ---------
>> > > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct
>> > > spi_nor
>> > > *nor, loff_t ofs, uint64_t len,
>> > >   *    0   |   1   |   1   |   0   |   1   |  2 MB         |
>> > > Lower
>> > > 1/4
>> > >   *    0   |   1   |   1   |   1   |   0   |  4 MB         |
>> > > Lower
>> > > 1/2
>> > >   *
>> > > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
>> > > 3):
>> > > + *
>> > > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
>> > > Protected
>> > > Portion
>> > > + *
>> > > -----------------------------------------------------------------
>> > > ---------
>> > > + *    0   |   0   |   0   |   0   |   0   |  NONE         | NONE
>> > > + *    0   |   0   |   0   |   0   |   1   |   64 KB       |
>> > > Upper
>> > > 1/1024
>> > > + *    0   |   0   |   0   |   1   |   0   |  128 KB       |
>> > > Upper
>> > > 1/512
>> > > + *    0   |   0   |   0   |   1   |   1   |  256 KB       |
>> > > Upper
>> > > 1/256
>> > > + *   ...
>> > > + *    0   |   1   |   0   |   0   |   1   |  16 MB        |
>> > > Upper
>> > > 1/4
>> > > + *    0   |   1   |   0   |   1   |   0   |  32 MB        |
>> > > Upper
>> > > 1/2
>> > > + *    0   |   1   |   0   |   1   |   1   |  64 MB        | ALL
>> > > + *    0   |   1   |   1   |   0   |   0   |  64 MB        | ALL
>> > > + *   ...
>> > > + *
>> > > ------|-------|-------|-------|-------|---------------|----------
>> > > ---------
>> > > + *    1   |   0   |   0   |   0   |   0   |   NONE        | NONE
>> > > + *    1   |   0   |   0   |   0   |   1   |   64 KB       |
>> > > Lower
>> > > 1/1024
>> > > + *    1   |   0   |   0   |   1   |   0   |  128 KB       |
>> > > Lower
>> > > 1/512
>> > > + *    1   |   0   |   0   |   1   |   1   |  256 KB       |
>> > > Lower
>> > > 1/256
>> > > + *   ...
>> > > + *    1   |   1   |   0   |   0   |   1   |  16 MB        |
>> > > Lower
>> > > 1/4
>> > > + *    1   |   1   |   0   |   1   |   0   |  32 MB        |
>> > > Lower
>> > > 1/2
>> > > + *    1   |   1   |   0   |   1   |   1   |  64 MB        | ALL
>> > > + *    1   |   1   |   1   |   0   |   0   |  64 MB        | ALL
>> > > + *   ...
>> > > + *
>> > >   * Returns negative on errors, 0 on success.
>> > >   */
>> > >  static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>> > > len)
>> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
>> > > loff_t
>> > > ofs, uint64_t len)
>> > >  		min_prot_len = stm_get_min_prot_length(nor);
>> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>> > >  		val = pow << SR_BP_SHIFT;
>> > > +
>> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > +			val = (val & ~BIT(5)) | BIT(6);
>> >
>> > .. and the use just the following here:
>> >
>> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
>> > 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
>> >
>> > Ie. just use the "normal case" where all BP bits are next to each
>> > other
>> > and then fixup the resulting value and shift the SR3 bit if
>> > necessary.
>> > This will be much easier to read.
>> >
>> 
>> Yes, I agree. It would be better to minimize this kind of conversion
>> in
>> one place.
>> 
>> > >  	}
>> > >
>> > >  	if (val & ~mask)
>> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
>> > > loff_t ofs, uint64_t len)
>> > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>> > >  		val = pow << SR_BP_SHIFT;
>> > >
>> > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > +			val = (val & ~BIT(5)) | BIT(6);
>> > > +
>> >
>> > here would be the other way around:
>> >
>> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
>> > 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
>> >
>> >
>> > >  		/* Some power-of-two sizes are not supported */
>> > >  		if (val & ~mask)
>> > >  			return -EINVAL;
>> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
>> > > const
>> > > char
>> > > *name,
>> > >  	if (info->flags & USE_CLSR)
>> > >  		nor->flags |= SNOR_F_USE_CLSR;
>> > >
>> > > +	if (info->flags & SPI_NOR_4BIT_BP) {
>> > > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
>> > > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
>> > > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>> > > +	}
>> > > +
>> > >  	if (info->flags & SPI_NOR_NO_ERASE)
>> > >  		mtd->flags |= MTD_NO_ERASE;
>> > >
>> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
>> > > nor.h
>> > > index de90724f62f1..0190ed21576a 100644
>> > > --- a/include/linux/mtd/spi-nor.h
>> > > +++ b/include/linux/mtd/spi-nor.h
>> > > @@ -129,7 +129,9 @@
>> > >  #define SR_BP1			BIT(3)	/* Block protect
>> > > 1
>> > > */
>> > >  #define SR_BP2			BIT(4)	/* Block protect
>> > > 2
>> > > */
>> > >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom
>> > > protect */
>> > > +#define SR_BP3_BIT5		BIT(5)	/* Block protect
>> > > 3
>> > > */
>> >
>> > IMHO just SR_BP3. but that is a matter of taste. But it is easier
>> > on
>> > the eye in the mask = SR_BP3 | SR_BP2 etc case.
>> >
>> 
>> SR_BP3 would be a more appropriate name if we could set the case with
>> all BP bits next to each other as the "normal case."
>> 
>> I'm going to write patches based on latest spi-nor/next including
>> what
>> you mentioned.
>> 
>> Thanks,
>> 
>> > -michael
>> >
>> > >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom
>> > > protect */
>> > > +#define SR_BP3_BIT6		BIT(6)	/* Block protect
>> > > 3
>> > > */
>> > >  #define SR_SRWD			BIT(7)	/* SR write
>> > > protect
>> > > */
>> > >  /* Spansion/Cypress specific status bits */
>> > >  #define SR_E_ERR		BIT(5)
>> > > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
>> > >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
>> > >  	SNOR_F_NO_READ_CR	= BIT(10),
>> > >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
>> > > +	SNOR_F_HAS_4BIT_BP	= BIT(12),
>> > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
>> > >
>> > >  };
>> 
>> 

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

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

[Jungseung Lee]

Hi,

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

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

I'm going to post new patch soon.

Thanks,

> 
> -michael
> 
> > > > 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > 	if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > 		mask |= SR_BP3;
> > > > 	return mask;
> 
> 
> }
> 
> 
> -michael
> 
> > 
> > Thanks,
> > 
> > > > > 
> > > > >  	if (!bp) {
> > > > >  		/* No protection */
> > > > > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct
> > > > > spi_nor
> > > > > *nor, loff_t ofs, uint64_t len,
> > > > > 
> > > > >  /*
> > > > >   * Lock a region of the flash. Compatible with ST Micro and
> > > > > similar
> > > > > flash.
> > > > > - * Supports the block protection bits BP{0,1,2} in the
> > > > > status
> > > > > register
> > > > > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3}
> > > > > in
> > > > > the
> > > > > status register
> > > > >   * (SR). Does not support these features found in newer SR
> > > > > bitfields:
> > > > >   *   - SEC: sector/block protect - only handle SEC=0 (block
> > > > > protect)
> > > > >   *   - CMP: complement protect - only support CMP=0 (range
> > > > > is
> > > > > not
> > > > > complemented)
> > > > > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct
> > > > > spi_nor
> > > > > *nor, loff_t ofs, uint64_t len,
> > > > >   * Support for the following is provided conditionally for
> > > > > some
> > > > > flash:
> > > > >   *   - TB: top/bottom protect
> > > > >   *
> > > > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > > > > + * Sample table portion for 8MB flash (Winbond w25q64fw /
> > > > > BP0-
> > > > > 2):
> > > > >   *
> > > > >   *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > > > > Protected
> > > > > Portion
> > > > >   *
> > > > > -----------------------------------------------------------
> > > > > ------
> > > > > ---------
> > > > > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct
> > > > > spi_nor
> > > > > *nor, loff_t ofs, uint64_t len,
> > > > >   *    0   |   1   |   1   |   0   |   1   |  2 MB         |
> > > > > Lower
> > > > > 1/4
> > > > >   *    0   |   1   |   1   |   1   |   0   |  4 MB         |
> > > > > Lower
> > > > > 1/2
> > > > >   *
> > > > > + * Sample table portion for 64MB flash (Micron n25q512ax3 /
> > > > > BP0-
> > > > > 3):
> > > > > + *
> > > > > + *   TB   |  BP3  |  BP2  |  BP1  |  BP0  |  Prot Length  |
> > > > > Protected
> > > > > Portion
> > > > > + *
> > > > > -----------------------------------------------------------
> > > > > ------
> > > > > ---------
> > > > > + *    0   |   0   |   0   |   0   |   0   |  NONE         |
> > > > > NONE
> > > > > + *    0   |   0   |   0   |   0   |   1   |   64 KB       |
> > > > > Upper
> > > > > 1/1024
> > > > > + *    0   |   0   |   0   |   1   |   0   |  128 KB       |
> > > > > Upper
> > > > > 1/512
> > > > > + *    0   |   0   |   0   |   1   |   1   |  256 KB       |
> > > > > Upper
> > > > > 1/256
> > > > > + *   ...
> > > > > + *    0   |   1   |   0   |   0   |   1   |  16 MB        |
> > > > > Upper
> > > > > 1/4
> > > > > + *    0   |   1   |   0   |   1   |   0   |  32 MB        |
> > > > > Upper
> > > > > 1/2
> > > > > + *    0   |   1   |   0   |   1   |   1   |  64 MB        |
> > > > > ALL
> > > > > + *    0   |   1   |   1   |   0   |   0   |  64 MB        |
> > > > > ALL
> > > > > + *   ...
> > > > > + *
> > > > > ------|-------|-------|-------|-------|---------------|----
> > > > > ------
> > > > > ---------
> > > > > + *    1   |   0   |   0   |   0   |   0   |   NONE        |
> > > > > NONE
> > > > > + *    1   |   0   |   0   |   0   |   1   |   64 KB       |
> > > > > Lower
> > > > > 1/1024
> > > > > + *    1   |   0   |   0   |   1   |   0   |  128 KB       |
> > > > > Lower
> > > > > 1/512
> > > > > + *    1   |   0   |   0   |   1   |   1   |  256 KB       |
> > > > > Lower
> > > > > 1/256
> > > > > + *   ...
> > > > > + *    1   |   1   |   0   |   0   |   1   |  16 MB        |
> > > > > Lower
> > > > > 1/4
> > > > > + *    1   |   1   |   0   |   1   |   0   |  32 MB        |
> > > > > Lower
> > > > > 1/2
> > > > > + *    1   |   1   |   0   |   1   |   1   |  64 MB        |
> > > > > ALL
> > > > > + *    1   |   1   |   1   |   0   |   0   |  64 MB        |
> > > > > ALL
> > > > > + *   ...
> > > > > + *
> > > > >   * Returns negative on errors, 0 on success.
> > > > >   */
> > > > >  static int stm_lock(struct spi_nor *nor, loff_t ofs,
> > > > > uint64_t
> > > > > len)
> > > > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor
> > > > > *nor,
> > > > > loff_t
> > > > > ofs, uint64_t len)
> > > > >  		min_prot_len = stm_get_min_prot_length(nor);
> > > > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > 1;
> > > > >  		val = pow << SR_BP_SHIFT;
> > > > > +
> > > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > > +			val = (val & ~BIT(5)) | BIT(6);
> > > > 
> > > > .. and the use just the following here:
> > > > 
> > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> > > > 	val = (val & ~SR_BP3) | SR_BP3_BIT6;
> > > > 
> > > > Ie. just use the "normal case" where all BP bits are next to
> > > > each
> > > > other
> > > > and then fixup the resulting value and shift the SR3 bit if
> > > > necessary.
> > > > This will be much easier to read.
> > > > 
> > > 
> > > Yes, I agree. It would be better to minimize this kind of
> > > conversion
> > > in
> > > one place.
> > > 
> > > > >  	}
> > > > > 
> > > > >  	if (val & ~mask)
> > > > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor
> > > > > *nor,
> > > > > loff_t ofs, uint64_t len)
> > > > >  		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > 1;
> > > > >  		val = pow << SR_BP_SHIFT;
> > > > > 
> > > > > +		if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > > +			val = (val & ~BIT(5)) | BIT(6);
> > > > > +
> > > > 
> > > > here would be the other way around:
> > > > 
> > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> > > > 	val = (val & ~SR_BP3_BIT6) | SR_BP3;
> > > > 
> > > > 
> > > > >  		/* Some power-of-two sizes are not supported */
> > > > >  		if (val & ~mask)
> > > > >  			return -EINVAL;
> > > > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
> > > > > const
> > > > > char
> > > > > *name,
> > > > >  	if (info->flags & USE_CLSR)
> > > > >  		nor->flags |= SNOR_F_USE_CLSR;
> > > > > 
> > > > > +	if (info->flags & SPI_NOR_4BIT_BP) {
> > > > > +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> > > > > +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > > > > +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > > > > +	}
> > > > > +
> > > > >  	if (info->flags & SPI_NOR_NO_ERASE)
> > > > >  		mtd->flags |= MTD_NO_ERASE;
> > > > > 
> > > > > diff --git a/include/linux/mtd/spi-nor.h
> > > > > b/include/linux/mtd/spi-
> > > > > nor.h
> > > > > index de90724f62f1..0190ed21576a 100644
> > > > > --- a/include/linux/mtd/spi-nor.h
> > > > > +++ b/include/linux/mtd/spi-nor.h
> > > > > @@ -129,7 +129,9 @@
> > > > >  #define SR_BP1			BIT(3)	/* Block
> > > > > protect
> > > > > 1
> > > > > */
> > > > >  #define SR_BP2			BIT(4)	/* Block
> > > > > protect
> > > > > 2
> > > > > */
> > > > >  #define SR_TB_BIT5		BIT(5)	/* Top/Bottom
> > > > > protect */
> > > > > +#define SR_BP3_BIT5		BIT(5)	/* Block protect
> > > > > 3
> > > > > */
> > > > 
> > > > IMHO just SR_BP3. but that is a matter of taste. But it is
> > > > easier
> > > > on
> > > > the eye in the mask = SR_BP3 | SR_BP2 etc case.
> > > > 
> > > 
> > > SR_BP3 would be a more appropriate name if we could set the case
> > > with
> > > all BP bits next to each other as the "normal case."
> > > 
> > > I'm going to write patches based on latest spi-nor/next including
> > > what
> > > you mentioned.
> > > 
> > > Thanks,
> > > 
> > > > -michael
> > > > 
> > > > >  #define SR_TB_BIT6		BIT(6)	/* Top/Bottom
> > > > > protect */
> > > > > +#define SR_BP3_BIT6		BIT(6)	/* Block protect
> > > > > 3
> > > > > */
> > > > >  #define SR_SRWD			BIT(7)	/* SR write
> > > > > protect
> > > > > */
> > > > >  /* Spansion/Cypress specific status bits */
> > > > >  #define SR_E_ERR		BIT(5)
> > > > > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> > > > >  	SNOR_F_HAS_16BIT_SR	= BIT(9),
> > > > >  	SNOR_F_NO_READ_CR	= BIT(10),
> > > > >  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
> > > > > +	SNOR_F_HAS_4BIT_BP	= BIT(12),
> > > > > +	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
> > > > > 
> > > > >  };
> > > 
> > > 
> 
> 


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

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

[Jungseung Lee]

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

This patch support lock/unlock feature on the flashes.
Tested on n25q512ax3. The other is modified following the datasheet.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 31a2106e529a..e8d1fbe4062f 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2607,12 +2607,17 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
 			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
 			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
-	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024,
+			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
+			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+			       SPI_NOR_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) },
+	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024,
+			       SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
+			       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+			       SPI_NOR_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) },
 	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
 	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
-- 
2.17.1


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

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

[Jungseung Lee]

Hi,

2020-03-06 (금), 20:19 +0800, chenxiang (M):
> Hi Jungseung,
> 
> 在 2020/3/4 19:07, Jungseung Lee 写道:
> > The current mainline locking was restricted and could only be
> > applied
> > to flashes that has 3 block protection bit and fixed locking ratio.
> > 
> > A new method of normalization was reached at the end of the
> > discussion [1].
> > 
> >     (1) - if bp slot is insufficient.
> >     (2) - if bp slot is sufficient.
> > 
> >     if (bp_slots_needed > bp_slots)    // (1)
> >         min_prot_length = sector_size << (bp_slots_needed -
> > bp_slots);
> >     else                               // (2)
> >         min_prot_length = sector_size;
> > 
> > This patch changes block protection handling logic based on
> > min_prot_length.
> > It is suitable for the overall flashes with exception of some
> > corner case
> > and easy to extend and apply for the case of 2bit or 4bit block
> > protection.
> > 
> > [1] 
> > http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
>  
> I have tested the patchset on one of my board (there is micron flash
> n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR, TB
> bit is on bit5 of SR), so
> i modify the code as follows to enable the lock/unlock of n25q128a11.
> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
> SPI_NOR_QUAD_READ) },
> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> +                       SECT_4K | SPI_NOR_QUAD_READ |
> +                       USE_FSR | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
> +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
> 
> There are two issues i met:
> (1) i lock/unlock the full region of the flash, lock is valid, but
> there is error when unlock the flash, i query the status of it is
> unlock (the issue i think it is
> the same as the issue John has reported before 
> https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/),
>  
> i screenshot the log of the operation as follows:
> 

Looks like the unlock operation was actually done (as can be checked
from the following query of the status) but an error is coming with
EIO.

I think another part of sr handling is related with your case. (maybe
SR read back test ?)

If you can dump the sr value & dev_dbg msg, it will be helpful to
define this issue.

> 
> (2) i try to lock part of the flash region such as ./flash_lock
> /dev/mtd0 0xc00000 10, it reports invalid argument,
> and i am not sure whether it is something wrong with my operation.
> 

It is unable to lock such region since the spi flash doesn't support
it. only we can lock it from the top or the bottom.

like this for n25q128a11,

flash_lock /dev/mtd0 0xff0000 0x10
flash_lock /dev/mtd0 0x0 0x10

Note the block count of examples is 0x10 not 10. The locking try with
block count under minimum block protection length will be failed.

Thanks,

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


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

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

[chenxiang (M)]

Hi Jungseung,

在 2020/3/7 16:24, Jungseung Lee 写道:
> Hi,
>
> 2020-03-06 (금), 20:19 +0800, chenxiang (M):
>> Hi Jungseung,
>>
>> 在 2020/3/4 19:07, Jungseung Lee 写道:
>>> The current mainline locking was restricted and could only be
>>> applied
>>> to flashes that has 3 block protection bit and fixed locking ratio.
>>>
>>> A new method of normalization was reached at the end of the
>>> discussion [1].
>>>
>>>      (1) - if bp slot is insufficient.
>>>      (2) - if bp slot is sufficient.
>>>
>>>      if (bp_slots_needed > bp_slots)    // (1)
>>>          min_prot_length = sector_size << (bp_slots_needed -
>>> bp_slots);
>>>      else                               // (2)
>>>          min_prot_length = sector_size;
>>>
>>> This patch changes block protection handling logic based on
>>> min_prot_length.
>>> It is suitable for the overall flashes with exception of some
>>> corner case
>>> and easy to extend and apply for the case of 2bit or 4bit block
>>> protection.
>>>
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
>>   
>> I have tested the patchset on one of my board (there is micron flash
>> n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR, TB
>> bit is on bit5 of SR), so
>> i modify the code as follows to enable the lock/unlock of n25q128a11.
>> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, SECT_4K |
>> SPI_NOR_QUAD_READ) },
>> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
>> +                       SECT_4K | SPI_NOR_QUAD_READ |
>> +                       USE_FSR | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
>> +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
>>
>> There are two issues i met:
>> (1) i lock/unlock the full region of the flash, lock is valid, but
>> there is error when unlock the flash, i query the status of it is
>> unlock (the issue i think it is
>> the same as the issue John has reported before
>> https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/),
>>   
>> i screenshot the log of the operation as follows:
>>
> Looks like the unlock operation was actually done (as can be checked
> from the following query of the status) but an error is coming with
> EIO.
>
> I think another part of sr handling is related with your case. (maybe
> SR read back test ?)

Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL is 
set), but it reads back 0x0 (bit WEL is cleared).

>
> If you can dump the sr value & dev_dbg msg, it will be helpful to
> define this issue.
>
>> (2) i try to lock part of the flash region such as ./flash_lock
>> /dev/mtd0 0xc00000 10, it reports invalid argument,
>> and i am not sure whether it is something wrong with my operation.
>>
> It is unable to lock such region since the spi flash doesn't support
> it. only we can lock it from the top or the bottom.
>
> like this for n25q128a11,
>
> flash_lock /dev/mtd0 0xff0000 0x10
> flash_lock /dev/mtd0 0x0 0x10

Do you mean if lock/unlcok from top,  the address of lock/unlock 
commands should be the address of 255th block (0xff0000), 254th 
block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
If lock/unlock from bottom, the address of lock/unlock commands should 
be always the address of 0th block (0x0)?

>
> Note the block count of examples is 0x10 not 10. The locking try with
> block count under minimum block protection length will be failed.
>
> Thanks,
>
>>> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>>> ---
>>>   drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++--------
>>> ------
>>>   1 file changed, 66 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
>>> nor/spi-nor.c
>>> index caf0c109cca0..c58c27552a74 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct mtd_info
>>> *mtd, struct erase_info *instr)
>>>   	return ret;
>>>   }
>>>   
>>> +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>>> +{
>>> +	return SR_BP2 | SR_BP1 | SR_BP0;
>>> +}
>>> +
>>> +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
>>> +{
>>> +	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>> +		return SR_TB_BIT6;
>>> +	else
>>> +		return SR_TB_BIT5;
>>> +}
>>> +
>>> +static int stm_get_min_prot_length(struct spi_nor *nor)
>>> +{
>>> +	int bp_slots, bp_slots_needed;
>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>> +
>>> +	bp_slots = (mask >> SR_BP_SHIFT) + 1;
>>> +
>>> +	/* Reserved one for "protect none" and one for "protect all".
>>> */
>>> +	bp_slots = bp_slots - 2;
>>> +
>>> +	bp_slots_needed = ilog2(nor->info->n_sectors);
>>> +
>>> +	if (bp_slots_needed > bp_slots)
>>> +		return nor->info->sector_size <<
>>> +			(bp_slots_needed - bp_slots);
>>> +	else
>>> +		return nor->info->sector_size;
>>> +}
>>> +
>>>   static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
>>> loff_t *ofs,
>>>   				 uint64_t *len)
>>>   {
>>>   	struct mtd_info *mtd = &nor->mtd;
>>> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>> -	u8 tb_mask = SR_TB_BIT5;
>>> -	int pow;
>>> +	int min_prot_len;
>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>> +	u8 tb_mask = spi_nor_get_tb_mask(nor);
>>> +	u8 bp = (sr & mask) >> SR_BP_SHIFT;
>>>   
>>> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>> -		tb_mask = SR_TB_BIT6;
>>> -
>>> -	if (!(sr & mask)) {
>>> +	if (!bp) {
>>>   		/* No protection */
>>>   		*ofs = 0;
>>>   		*len = 0;
>>> -	} else {
>>> -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
>>> -		*len = mtd->size >> pow;
>>> -		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>>> -			*ofs = 0;
>>> -		else
>>> -			*ofs = mtd->size - *len;
>>> +		return;
>>>   	}
>>> +
>>> +	min_prot_len = stm_get_min_prot_length(nor);
>>> +	*len = min_prot_len << (bp - 1);
>>> +
>>> +	if (*len > mtd->size)
>>> +		*len = mtd->size;
>>> +
>>> +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>>> +		*ofs = 0;
>>> +	else
>>> +		*ofs = mtd->size - *len;
>>>   }
>>>   
>>>   /*
>>> @@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
>>> loff_t ofs, uint64_t len)
>>>   {
>>>   	struct mtd_info *mtd = &nor->mtd;
>>>   	int ret, status_old, status_new;
>>> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>> -	u8 tb_mask = SR_TB_BIT5;
>>> +	int min_prot_len;
>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>> +	u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>   	u8 pow, val;
>>>   	loff_t lock_len;
>>>   	bool can_be_top = true, can_be_bottom = nor->flags &
>>> SNOR_F_HAS_SR_TB;
>>> @@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor *nor,
>>> loff_t ofs, uint64_t len)
>>>   	else
>>>   		lock_len = ofs + len;
>>>   
>>> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>> -		tb_mask = SR_TB_BIT6;
>>> +	if (lock_len == mtd->size) {
>>> +		val = mask; /* fully locked */
>>> +	} else {
>>> +		min_prot_len = stm_get_min_prot_length(nor);
>>> +		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>>> +		val = pow << SR_BP_SHIFT;
>>> +	}
>>>   
>>> -	/*
>>> -	 * Need smallest pow such that:
>>> -	 *
>>> -	 *   1 / (2^pow) <= (len / size)
>>> -	 *
>>> -	 * so (assuming power-of-2 size) we do:
>>> -	 *
>>> -	 *   pow = ceil(log2(size / len)) = log2(size) -
>>> floor(log2(len))
>>> -	 */
>>> -	pow = ilog2(mtd->size) - ilog2(lock_len);
>>> -	val = mask - (pow << SR_BP_SHIFT);
>>>   	if (val & ~mask)
>>>   		return -EINVAL;
>>>   	/* Don't "lock" with no region! */
>>> @@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor *nor,
>>> loff_t ofs, uint64_t len)
>>>   {
>>>   	struct mtd_info *mtd = &nor->mtd;
>>>   	int ret, status_old, status_new;
>>> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>> -	u8 tb_mask = SR_TB_BIT5;
>>> +	int min_prot_len;
>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>> +	u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>   	u8 pow, val;
>>>   	loff_t lock_len;
>>>   	bool can_be_top = true, can_be_bottom = nor->flags &
>>> SNOR_F_HAS_SR_TB;
>>> @@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor *nor,
>>> loff_t ofs, uint64_t len)
>>>   	else
>>>   		lock_len = ofs;
>>>   
>>> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>> -		tb_mask = SR_TB_BIT6;
>>> -	/*
>>> -	 * Need largest pow such that:
>>> -	 *
>>> -	 *   1 / (2^pow) >= (len / size)
>>> -	 *
>>> -	 * so (assuming power-of-2 size) we do:
>>> -	 *
>>> -	 *   pow = floor(log2(size / len)) = log2(size) -
>>> ceil(log2(len))
>>> -	 */
>>> -	pow = ilog2(mtd->size) - order_base_2(lock_len);
>>>   	if (lock_len == 0) {
>>>   		val = 0; /* fully unlocked */
>>>   	} else {
>>> -		val = mask - (pow << SR_BP_SHIFT);
>>> +		min_prot_len = stm_get_min_prot_length(nor);
>>> +		pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>>> +		val = pow << SR_BP_SHIFT;
>>> +
>>>   		/* Some power-of-two sizes are not supported */
>>>   		if (val & ~mask)
>>>   			return -EINVAL;
>>   
>
> .
>



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

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

[Jungseung Lee]

Hi,

2020-03-09 (Mon), 15:50 +0800, chenxiang (M):
> Hi Jungseung,
> 
> 在 2020/3/7 16:24, Jungseung Lee 写道:
> > Hi,
> > 
> > 2020-03-06 (Fri), 20:19 +0800, chenxiang (M):
> > > Hi Jungseung,
> > > 
> > > 在 2020/3/4 19:07, Jungseung Lee 写道:
> > > > The current mainline locking was restricted and could only be
> > > > applied
> > > > to flashes that has 3 block protection bit and fixed locking
> > > > ratio.
> > > > 
> > > > A new method of normalization was reached at the end of the
> > > > discussion [1].
> > > > 
> > > >      (1) - if bp slot is insufficient.
> > > >      (2) - if bp slot is sufficient.
> > > > 
> > > >      if (bp_slots_needed > bp_slots)    // (1)
> > > >          min_prot_length = sector_size << (bp_slots_needed -
> > > > bp_slots);
> > > >      else                               // (2)
> > > >          min_prot_length = sector_size;
> > > > 
> > > > This patch changes block protection handling logic based on
> > > > min_prot_length.
> > > > It is suitable for the overall flashes with exception of some
> > > > corner case
> > > > and easy to extend and apply for the case of 2bit or 4bit block
> > > > protection.
> > > > 
> > > > [1]
> > > > 
https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
> > > 
> > >   
> > > I have tested the patchset on one of my board (there is micron
> > > flash
> > > n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR,
> > > TB
> > > bit is on bit5 of SR), so
> > > i modify the code as follows to enable the lock/unlock of
> > > n25q128a11.
> > > -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> > > SECT_4K |
> > > SPI_NOR_QUAD_READ) },
> > > +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> > > +                       SECT_4K | SPI_NOR_QUAD_READ |
> > > +                       USE_FSR | SPI_NOR_HAS_LOCK |
> > > SPI_NOR_HAS_TB |
> > > +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
> > > 
> > > There are two issues i met:
> > > (1) i lock/unlock the full region of the flash, lock is valid,
> > > but
> > > there is error when unlock the flash, i query the status of it is
> > > unlock (the issue i think it is
> > > the same as the issue John has reported before
> > > 
https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/
> > > ),
> > >   
> > > i screenshot the log of the operation as follows:
> > > 
> > 
> > Looks like the unlock operation was actually done (as can be
> > checked
> > from the following query of the status) but an error is coming with
> > EIO.
> > 
> > I think another part of sr handling is related with your case.
> > (maybe
> > SR read back test ?)
> 
> Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL
> is 
> set), but it reads back 0x0 (bit WEL is cleared).
> 


> > 
> > If you can dump the sr value & dev_dbg msg, it will be helpful to
> > define this issue.
> > 
> > > (2) i try to lock part of the flash region such as ./flash_lock
> > > /dev/mtd0 0xc00000 10, it reports invalid argument,
> > > and i am not sure whether it is something wrong with my
> > > operation.
> > > 
> > 
> > It is unable to lock such region since the spi flash doesn't
> > support
> > it. only we can lock it from the top or the bottom.
> > 
> > like this for n25q128a11,
> > 
> > flash_lock /dev/mtd0 0xff0000 0x10
> > flash_lock /dev/mtd0 0x0 0x10
> 
> Do you mean if lock/unlcok from top,  the address of lock/unlock 
> commands should be the address of 255th block (0xff0000), 254th 
> block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
> If lock/unlock from bottom, the address of lock/unlock commands
> should 
> be always the address of 0th block (0x0)?
> 

I'm not fully understanding the usage of flash_lock, but it would be
better to use such addresses for lock/unlocking to make it under
control.

There are some ambiguous parts to explain that since some lock/unlock
operation is still working well without the addresses.

LOCK 
- Return success if the requested area is already locked.
- If requested area is not fully matched with locking portion of the
flash, lock the some of portion including the request area.

UNLOCK 
 - unlock operation return success if the requested area is already
unlocked.

 - unlock operation try to unlock all portions includes the request
area. (But user will not knowing well)



> > 
> > Note the block count of examples is 0x10 not 10. The locking try
> > with
> > block count under minimum block protection length will be failed.
> > 
> > Thanks,
> > 
> > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > > ---
> > > >   drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++-----
> > > > ---
> > > > ------
> > > >   1 file changed, 66 insertions(+), 44 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > > > nor/spi-nor.c
> > > > index caf0c109cca0..c58c27552a74 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
> > > > mtd_info
> > > > *mtd, struct erase_info *instr)
> > > >   	return ret;
> > > >   }
> > > >   
> > > > +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > > > +{
> > > > +	return SR_BP2 | SR_BP1 | SR_BP0;
> > > > +}
> > > > +
> > > > +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > > +{
> > > > +	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > +		return SR_TB_BIT6;
> > > > +	else
> > > > +		return SR_TB_BIT5;
> > > > +}
> > > > +
> > > > +static int stm_get_min_prot_length(struct spi_nor *nor)
> > > > +{
> > > > +	int bp_slots, bp_slots_needed;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +
> > > > +	bp_slots = (mask >> SR_BP_SHIFT) + 1;
> > > > +
> > > > +	/* Reserved one for "protect none" and one for "protect
> > > > all".
> > > > */
> > > > +	bp_slots = bp_slots - 2;
> > > > +
> > > > +	bp_slots_needed = ilog2(nor->info->n_sectors);
> > > > +
> > > > +	if (bp_slots_needed > bp_slots)
> > > > +		return nor->info->sector_size <<
> > > > +			(bp_slots_needed - bp_slots);
> > > > +	else
> > > > +		return nor->info->sector_size;
> > > > +}
> > > > +
> > > >   static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
> > > > loff_t *ofs,
> > > >   				 uint64_t *len)
> > > >   {
> > > >   	struct mtd_info *mtd = &nor->mtd;
> > > > -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > -	u8 tb_mask = SR_TB_BIT5;
> > > > -	int pow;
> > > > +	int min_prot_len;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > +	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > >   
> > > > -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > -		tb_mask = SR_TB_BIT6;
> > > > -
> > > > -	if (!(sr & mask)) {
> > > > +	if (!bp) {
> > > >   		/* No protection */
> > > >   		*ofs = 0;
> > > >   		*len = 0;
> > > > -	} else {
> > > > -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> > > > -		*len = mtd->size >> pow;
> > > > -		if (nor->flags & SNOR_F_HAS_SR_TB && sr &
> > > > tb_mask)
> > > > -			*ofs = 0;
> > > > -		else
> > > > -			*ofs = mtd->size - *len;
> > > > +		return;
> > > >   	}
> > > > +
> > > > +	min_prot_len = stm_get_min_prot_length(nor);
> > > > +	*len = min_prot_len << (bp - 1);
> > > > +
> > > > +	if (*len > mtd->size)
> > > > +		*len = mtd->size;
> > > > +
> > > > +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> > > > +		*ofs = 0;
> > > > +	else
> > > > +		*ofs = mtd->size - *len;
> > > >   }
> > > >   
> > > >   /*
> > > > @@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   {
> > > >   	struct mtd_info *mtd = &nor->mtd;
> > > >   	int ret, status_old, status_new;
> > > > -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > -	u8 tb_mask = SR_TB_BIT5;
> > > > +	int min_prot_len;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > >   	u8 pow, val;
> > > >   	loff_t lock_len;
> > > >   	bool can_be_top = true, can_be_bottom = nor->flags &
> > > > SNOR_F_HAS_SR_TB;
> > > > @@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   	else
> > > >   		lock_len = ofs + len;
> > > >   
> > > > -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > -		tb_mask = SR_TB_BIT6;
> > > > +	if (lock_len == mtd->size) {
> > > > +		val = mask; /* fully locked */
> > > > +	} else {
> > > > +		min_prot_len = stm_get_min_prot_length(nor);
> > > > +		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > 1;
> > > > +		val = pow << SR_BP_SHIFT;
> > > > +	}
> > > >   
> > > > -	/*
> > > > -	 * Need smallest pow such that:
> > > > -	 *
> > > > -	 *   1 / (2^pow) <= (len / size)
> > > > -	 *
> > > > -	 * so (assuming power-of-2 size) we do:
> > > > -	 *
> > > > -	 *   pow = ceil(log2(size / len)) = log2(size) -
> > > > floor(log2(len))
> > > > -	 */
> > > > -	pow = ilog2(mtd->size) - ilog2(lock_len);
> > > > -	val = mask - (pow << SR_BP_SHIFT);
> > > >   	if (val & ~mask)
> > > >   		return -EINVAL;
> > > >   	/* Don't "lock" with no region! */
> > > > @@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   {
> > > >   	struct mtd_info *mtd = &nor->mtd;
> > > >   	int ret, status_old, status_new;
> > > > -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > -	u8 tb_mask = SR_TB_BIT5;
> > > > +	int min_prot_len;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > >   	u8 pow, val;
> > > >   	loff_t lock_len;
> > > >   	bool can_be_top = true, can_be_bottom = nor->flags &
> > > > SNOR_F_HAS_SR_TB;
> > > > @@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   	else
> > > >   		lock_len = ofs;
> > > >   
> > > > -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > -		tb_mask = SR_TB_BIT6;
> > > > -	/*
> > > > -	 * Need largest pow such that:
> > > > -	 *
> > > > -	 *   1 / (2^pow) >= (len / size)
> > > > -	 *
> > > > -	 * so (assuming power-of-2 size) we do:
> > > > -	 *
> > > > -	 *   pow = floor(log2(size / len)) = log2(size) -
> > > > ceil(log2(len))
> > > > -	 */
> > > > -	pow = ilog2(mtd->size) - order_base_2(lock_len);
> > > >   	if (lock_len == 0) {
> > > >   		val = 0; /* fully unlocked */
> > > >   	} else {
> > > > -		val = mask - (pow << SR_BP_SHIFT);
> > > > +		min_prot_len = stm_get_min_prot_length(nor);
> > > > +		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > 1;
> > > > +		val = pow << SR_BP_SHIFT;
> > > > +
> > > >   		/* Some power-of-two sizes are not supported */
> > > >   		if (val & ~mask)
> > > >   			return -EINVAL;
> > > 
> > >   
> > 
> > .
> > 
> 
> 
> 


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

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

[Jungseung Lee]

Hi,

2020-03-09 (월), 15:50 +0800, chenxiang (M):
> Hi Jungseung,
> 
> 在 2020/3/7 16:24, Jungseung Lee 写道:
> > Hi,
> > 
> > 2020-03-06 (금), 20:19 +0800, chenxiang (M):
> > > Hi Jungseung,
> > > 
> > > 在 2020/3/4 19:07, Jungseung Lee 写道:
> > > > The current mainline locking was restricted and could only be
> > > > applied
> > > > to flashes that has 3 block protection bit and fixed locking
> > > > ratio.
> > > > 
> > > > A new method of normalization was reached at the end of the
> > > > discussion [1].
> > > > 
> > > >      (1) - if bp slot is insufficient.
> > > >      (2) - if bp slot is sufficient.
> > > > 
> > > >      if (bp_slots_needed > bp_slots)    // (1)
> > > >          min_prot_length = sector_size << (bp_slots_needed -
> > > > bp_slots);
> > > >      else                               // (2)
> > > >          min_prot_length = sector_size;
> > > > 
> > > > This patch changes block protection handling logic based on
> > > > min_prot_length.
> > > > It is suitable for the overall flashes with exception of some
> > > > corner case
> > > > and easy to extend and apply for the case of 2bit or 4bit block
> > > > protection.
> > > > 
> > > > [1]
> > > > 
https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
> > > 
> > >   
> > > I have tested the patchset on one of my board (there is micron
> > > flash
> > > n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR,
> > > TB
> > > bit is on bit5 of SR), so
> > > i modify the code as follows to enable the lock/unlock of
> > > n25q128a11.
> > > -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> > > SECT_4K |
> > > SPI_NOR_QUAD_READ) },
> > > +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> > > +                       SECT_4K | SPI_NOR_QUAD_READ |
> > > +                       USE_FSR | SPI_NOR_HAS_LOCK |
> > > SPI_NOR_HAS_TB |
> > > +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
> > > 
> > > There are two issues i met:
> > > (1) i lock/unlock the full region of the flash, lock is valid,
> > > but
> > > there is error when unlock the flash, i query the status of it is
> > > unlock (the issue i think it is
> > > the same as the issue John has reported before
> > > 
https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/
> > > ),
> > >   
> > > i screenshot the log of the operation as follows:
> > > 
> > 
> > Looks like the unlock operation was actually done (as can be
> > checked
> > from the following query of the status) but an error is coming with
> > EIO.
> > 
> > I think another part of sr handling is related with your case.
> > (maybe
> > SR read back test ?)
> 
> Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL
> is 
> set), but it reads back 0x0 (bit WEL is cleared).
> 

I am reviewing tudor's patches and it seems solve your issue
effectively. 
http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html

> > 
> > If you can dump the sr value & dev_dbg msg, it will be helpful to
> > define this issue.
> > 
> > > (2) i try to lock part of the flash region such as ./flash_lock
> > > /dev/mtd0 0xc00000 10, it reports invalid argument,
> > > and i am not sure whether it is something wrong with my
> > > operation.
> > > 
> > 
> > It is unable to lock such region since the spi flash doesn't
> > support
> > it. only we can lock it from the top or the bottom.
> > 
> > like this for n25q128a11,
> > 
> > flash_lock /dev/mtd0 0xff0000 0x10
> > flash_lock /dev/mtd0 0x0 0x10
> 
> Do you mean if lock/unlcok from top,  the address of lock/unlock 
> commands should be the address of 255th block (0xff0000), 254th 
> block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
> If lock/unlock from bottom, the address of lock/unlock commands
> should 
> be always the address of 0th block (0x0)?
> 

I'm not fully understanding the usage of flash_lock, but it would be
better to use such addresses for lock/unlocking to make it under
control.
 
There are some ambiguous parts to explain that since some lock/unlock
operation is still working well without the addresses.

LOCK 
- Return success if the requested area is already locked.
- If requested area is not fully matched with lock portion of the
flash, lock some of the portion including the request area as it could
be.

UNLOCK 
- Return success if the requested area is already unlocked.
- If requested area is not fully matched with lock portion of the
flash, unlock all locked portion including the request area. the
portion would be bigger than requested area.

So, the lock/unlock would be able to work without the addresses. but in
my case I don't use the way because it will makes hard to tracking the
locked area.

Thanks,

> > 
> > Note the block count of examples is 0x10 not 10. The locking try
> > with
> > block count under minimum block protection length will be failed.
> > 
> > Thanks,
> > 
> > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > > ---
> > > >   drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++-----
> > > > ---
> > > > ------
> > > >   1 file changed, 66 insertions(+), 44 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> > > > nor/spi-nor.c
> > > > index caf0c109cca0..c58c27552a74 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
> > > > mtd_info
> > > > *mtd, struct erase_info *instr)
> > > >   	return ret;
> > > >   }
> > > >   
> > > > +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > > > +{
> > > > +	return SR_BP2 | SR_BP1 | SR_BP0;
> > > > +}
> > > > +
> > > > +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > > +{
> > > > +	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > +		return SR_TB_BIT6;
> > > > +	else
> > > > +		return SR_TB_BIT5;
> > > > +}
> > > > +
> > > > +static int stm_get_min_prot_length(struct spi_nor *nor)
> > > > +{
> > > > +	int bp_slots, bp_slots_needed;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +
> > > > +	bp_slots = (mask >> SR_BP_SHIFT) + 1;
> > > > +
> > > > +	/* Reserved one for "protect none" and one for "protect
> > > > all".
> > > > */
> > > > +	bp_slots = bp_slots - 2;
> > > > +
> > > > +	bp_slots_needed = ilog2(nor->info->n_sectors);
> > > > +
> > > > +	if (bp_slots_needed > bp_slots)
> > > > +		return nor->info->sector_size <<
> > > > +			(bp_slots_needed - bp_slots);
> > > > +	else
> > > > +		return nor->info->sector_size;
> > > > +}
> > > > +
> > > >   static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
> > > > loff_t *ofs,
> > > >   				 uint64_t *len)
> > > >   {
> > > >   	struct mtd_info *mtd = &nor->mtd;
> > > > -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > -	u8 tb_mask = SR_TB_BIT5;
> > > > -	int pow;
> > > > +	int min_prot_len;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > +	u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > >   
> > > > -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > -		tb_mask = SR_TB_BIT6;
> > > > -
> > > > -	if (!(sr & mask)) {
> > > > +	if (!bp) {
> > > >   		/* No protection */
> > > >   		*ofs = 0;
> > > >   		*len = 0;
> > > > -	} else {
> > > > -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> > > > -		*len = mtd->size >> pow;
> > > > -		if (nor->flags & SNOR_F_HAS_SR_TB && sr &
> > > > tb_mask)
> > > > -			*ofs = 0;
> > > > -		else
> > > > -			*ofs = mtd->size - *len;
> > > > +		return;
> > > >   	}
> > > > +
> > > > +	min_prot_len = stm_get_min_prot_length(nor);
> > > > +	*len = min_prot_len << (bp - 1);
> > > > +
> > > > +	if (*len > mtd->size)
> > > > +		*len = mtd->size;
> > > > +
> > > > +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> > > > +		*ofs = 0;
> > > > +	else
> > > > +		*ofs = mtd->size - *len;
> > > >   }
> > > >   
> > > >   /*
> > > > @@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   {
> > > >   	struct mtd_info *mtd = &nor->mtd;
> > > >   	int ret, status_old, status_new;
> > > > -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > -	u8 tb_mask = SR_TB_BIT5;
> > > > +	int min_prot_len;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > >   	u8 pow, val;
> > > >   	loff_t lock_len;
> > > >   	bool can_be_top = true, can_be_bottom = nor->flags &
> > > > SNOR_F_HAS_SR_TB;
> > > > @@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   	else
> > > >   		lock_len = ofs + len;
> > > >   
> > > > -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > -		tb_mask = SR_TB_BIT6;
> > > > +	if (lock_len == mtd->size) {
> > > > +		val = mask; /* fully locked */
> > > > +	} else {
> > > > +		min_prot_len = stm_get_min_prot_length(nor);
> > > > +		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > 1;
> > > > +		val = pow << SR_BP_SHIFT;
> > > > +	}
> > > >   
> > > > -	/*
> > > > -	 * Need smallest pow such that:
> > > > -	 *
> > > > -	 *   1 / (2^pow) <= (len / size)
> > > > -	 *
> > > > -	 * so (assuming power-of-2 size) we do:
> > > > -	 *
> > > > -	 *   pow = ceil(log2(size / len)) = log2(size) -
> > > > floor(log2(len))
> > > > -	 */
> > > > -	pow = ilog2(mtd->size) - ilog2(lock_len);
> > > > -	val = mask - (pow << SR_BP_SHIFT);
> > > >   	if (val & ~mask)
> > > >   		return -EINVAL;
> > > >   	/* Don't "lock" with no region! */
> > > > @@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   {
> > > >   	struct mtd_info *mtd = &nor->mtd;
> > > >   	int ret, status_old, status_new;
> > > > -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > -	u8 tb_mask = SR_TB_BIT5;
> > > > +	int min_prot_len;
> > > > +	u8 mask = spi_nor_get_bp_mask(nor);
> > > > +	u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > >   	u8 pow, val;
> > > >   	loff_t lock_len;
> > > >   	bool can_be_top = true, can_be_bottom = nor->flags &
> > > > SNOR_F_HAS_SR_TB;
> > > > @@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor
> > > > *nor,
> > > > loff_t ofs, uint64_t len)
> > > >   	else
> > > >   		lock_len = ofs;
> > > >   
> > > > -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > -		tb_mask = SR_TB_BIT6;
> > > > -	/*
> > > > -	 * Need largest pow such that:
> > > > -	 *
> > > > -	 *   1 / (2^pow) >= (len / size)
> > > > -	 *
> > > > -	 * so (assuming power-of-2 size) we do:
> > > > -	 *
> > > > -	 *   pow = floor(log2(size / len)) = log2(size) -
> > > > ceil(log2(len))
> > > > -	 */
> > > > -	pow = ilog2(mtd->size) - order_base_2(lock_len);
> > > >   	if (lock_len == 0) {
> > > >   		val = 0; /* fully unlocked */
> > > >   	} else {
> > > > -		val = mask - (pow << SR_BP_SHIFT);
> > > > +		min_prot_len = stm_get_min_prot_length(nor);
> > > > +		pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > 1;
> > > > +		val = pow << SR_BP_SHIFT;
> > > > +
> > > >   		/* Some power-of-two sizes are not supported */
> > > >   		if (val & ~mask)
> > > >   			return -EINVAL;
> > > 
> > >   
> > 
> > .
> > 
> 
> 
> 


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

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

[chenxiang (M)]

Hi Jungseung,

在 2020/3/9 19:44, Jungseung Lee 写道:
> Hi,
>
> 2020-03-09 (월), 15:50 +0800, chenxiang (M):
>> Hi Jungseung,
>>
>> 在 2020/3/7 16:24, Jungseung Lee 写道:
>>> Hi,
>>>
>>> 2020-03-06 (금), 20:19 +0800, chenxiang (M):
>>>> Hi Jungseung,
>>>>
>>>> 在 2020/3/4 19:07, Jungseung Lee 写道:
>>>>> The current mainline locking was restricted and could only be
>>>>> applied
>>>>> to flashes that has 3 block protection bit and fixed locking
>>>>> ratio.
>>>>>
>>>>> A new method of normalization was reached at the end of the
>>>>> discussion [1].
>>>>>
>>>>>       (1) - if bp slot is insufficient.
>>>>>       (2) - if bp slot is sufficient.
>>>>>
>>>>>       if (bp_slots_needed > bp_slots)    // (1)
>>>>>           min_prot_length = sector_size << (bp_slots_needed -
>>>>> bp_slots);
>>>>>       else                               // (2)
>>>>>           min_prot_length = sector_size;
>>>>>
>>>>> This patch changes block protection handling logic based on
>>>>> min_prot_length.
>>>>> It is suitable for the overall flashes with exception of some
>>>>> corner case
>>>>> and easy to extend and apply for the case of 2bit or 4bit block
>>>>> protection.
>>>>>
>>>>> [1]
>>>>>
> https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
>>>>    
>>>> I have tested the patchset on one of my board (there is micron
>>>> flash
>>>> n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR,
>>>> TB
>>>> bit is on bit5 of SR), so
>>>> i modify the code as follows to enable the lock/unlock of
>>>> n25q128a11.
>>>> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
>>>> SECT_4K |
>>>> SPI_NOR_QUAD_READ) },
>>>> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
>>>> +                       SECT_4K | SPI_NOR_QUAD_READ |
>>>> +                       USE_FSR | SPI_NOR_HAS_LOCK |
>>>> SPI_NOR_HAS_TB |
>>>> +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
>>>>
>>>> There are two issues i met:
>>>> (1) i lock/unlock the full region of the flash, lock is valid,
>>>> but
>>>> there is error when unlock the flash, i query the status of it is
>>>> unlock (the issue i think it is
>>>> the same as the issue John has reported before
>>>>
> https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/
>>>> ),
>>>>    
>>>> i screenshot the log of the operation as follows:
>>>>
>>> Looks like the unlock operation was actually done (as can be
>>> checked
>>> from the following query of the status) but an error is coming with
>>> EIO.
>>>
>>> I think another part of sr handling is related with your case.
>>> (maybe
>>> SR read back test ?)
>> Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL
>> is
>> set), but it reads back 0x0 (bit WEL is cleared).
>>
> I am reviewing tudor's patches and it seems solve your issue
> effectively.
> http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html

Yes, it solves my issue.

>
>>> If you can dump the sr value & dev_dbg msg, it will be helpful to
>>> define this issue.
>>>
>>>> (2) i try to lock part of the flash region such as ./flash_lock
>>>> /dev/mtd0 0xc00000 10, it reports invalid argument,
>>>> and i am not sure whether it is something wrong with my
>>>> operation.
>>>>
>>> It is unable to lock such region since the spi flash doesn't
>>> support
>>> it. only we can lock it from the top or the bottom.
>>>
>>> like this for n25q128a11,
>>>
>>> flash_lock /dev/mtd0 0xff0000 0x10
>>> flash_lock /dev/mtd0 0x0 0x10
>> Do you mean if lock/unlcok from top,  the address of lock/unlock
>> commands should be the address of 255th block (0xff0000), 254th
>> block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
>> If lock/unlock from bottom, the address of lock/unlock commands
>> should
>> be always the address of 0th block (0x0)?
>>
> I'm not fully understanding the usage of flash_lock, but it would be
> better to use such addresses for lock/unlocking to make it under
> control.
>   
> There are some ambiguous parts to explain that since some lock/unlock
> operation is still working well without the addresses.
>
> LOCK
> - Return success if the requested area is already locked.
> - If requested area is not fully matched with lock portion of the
> flash, lock some of the portion including the request area as it could
> be.
>
> UNLOCK
> - Return success if the requested area is already unlocked.
> - If requested area is not fully matched with lock portion of the
> flash, unlock all locked portion including the request area. the
> portion would be bigger than requested area.

Thanks for you info.
I have tested above situations of lock and unlock, and still have a 
question about it:
For unlock function, as you said, it will unlock all the locked portion 
including the request area which would be bigger than requested area if 
requested area is not fully matched with lock portion of the flash.
But for lock function, it seem not lock some of portion including the 
request area as it could be, and it seems require the total locked area 
must be matched with
some portion of the flash (it seems not allow hole between those regions).

For example, 16MB in my envirnment, i do as follows:
- lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched 
with lock portion of the flash (BP3~0 = 0001, TB=0)
- lock [0xc00000, 0xff0000] or [0xc00000, 0xff1000]   -> it also matched 
with lock portion of the flash (BP3~0 = 0111, TB=0)
but if do it as follows:
- lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched 
with lock portion of the flash (BP3~0 = 0001, TB=0)
- lock [0xc00000, 0xc10000]   -> it will report invalid argument at the 
second time, in my thought it would lock [0xc00000, 0x1000000] which 
will including those two regions

>
> So, the lock/unlock would be able to work without the addresses. but in
> my case I don't use the way because it will makes hard to tracking the
> locked area.
>
> Thanks,
>
>>> Note the block count of examples is 0x10 not 10. The locking try
>>> with
>>> block count under minimum block protection length will be failed.
>>>
>>> Thanks,
>>>
>>>>> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>>>>> ---
>>>>>    drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++-----
>>>>> ---
>>>>> ------
>>>>>    1 file changed, 66 insertions(+), 44 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
>>>>> nor/spi-nor.c
>>>>> index caf0c109cca0..c58c27552a74 100644
>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>> @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
>>>>> mtd_info
>>>>> *mtd, struct erase_info *instr)
>>>>>    	return ret;
>>>>>    }
>>>>>    
>>>>> +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>>>>> +{
>>>>> +	return SR_BP2 | SR_BP1 | SR_BP0;
>>>>> +}
>>>>> +
>>>>> +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
>>>>> +{
>>>>> +	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>> +		return SR_TB_BIT6;
>>>>> +	else
>>>>> +		return SR_TB_BIT5;
>>>>> +}
>>>>> +
>>>>> +static int stm_get_min_prot_length(struct spi_nor *nor)
>>>>> +{
>>>>> +	int bp_slots, bp_slots_needed;
>>>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>>>> +
>>>>> +	bp_slots = (mask >> SR_BP_SHIFT) + 1;
>>>>> +
>>>>> +	/* Reserved one for "protect none" and one for "protect
>>>>> all".
>>>>> */
>>>>> +	bp_slots = bp_slots - 2;
>>>>> +
>>>>> +	bp_slots_needed = ilog2(nor->info->n_sectors);
>>>>> +
>>>>> +	if (bp_slots_needed > bp_slots)
>>>>> +		return nor->info->sector_size <<
>>>>> +			(bp_slots_needed - bp_slots);
>>>>> +	else
>>>>> +		return nor->info->sector_size;
>>>>> +}
>>>>> +
>>>>>    static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
>>>>> loff_t *ofs,
>>>>>    				 uint64_t *len)
>>>>>    {
>>>>>    	struct mtd_info *mtd = &nor->mtd;
>>>>> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>>>> -	u8 tb_mask = SR_TB_BIT5;
>>>>> -	int pow;
>>>>> +	int min_prot_len;
>>>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>>>> +	u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>>> +	u8 bp = (sr & mask) >> SR_BP_SHIFT;
>>>>>    
>>>>> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>> -		tb_mask = SR_TB_BIT6;
>>>>> -
>>>>> -	if (!(sr & mask)) {
>>>>> +	if (!bp) {
>>>>>    		/* No protection */
>>>>>    		*ofs = 0;
>>>>>    		*len = 0;
>>>>> -	} else {
>>>>> -		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
>>>>> -		*len = mtd->size >> pow;
>>>>> -		if (nor->flags & SNOR_F_HAS_SR_TB && sr &
>>>>> tb_mask)
>>>>> -			*ofs = 0;
>>>>> -		else
>>>>> -			*ofs = mtd->size - *len;
>>>>> +		return;
>>>>>    	}
>>>>> +
>>>>> +	min_prot_len = stm_get_min_prot_length(nor);
>>>>> +	*len = min_prot_len << (bp - 1);
>>>>> +
>>>>> +	if (*len > mtd->size)
>>>>> +		*len = mtd->size;
>>>>> +
>>>>> +	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>>>>> +		*ofs = 0;
>>>>> +	else
>>>>> +		*ofs = mtd->size - *len;
>>>>>    }
>>>>>    
>>>>>    /*
>>>>> @@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
>>>>> loff_t ofs, uint64_t len)
>>>>>    {
>>>>>    	struct mtd_info *mtd = &nor->mtd;
>>>>>    	int ret, status_old, status_new;
>>>>> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>>>> -	u8 tb_mask = SR_TB_BIT5;
>>>>> +	int min_prot_len;
>>>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>>>> +	u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>>>    	u8 pow, val;
>>>>>    	loff_t lock_len;
>>>>>    	bool can_be_top = true, can_be_bottom = nor->flags &
>>>>> SNOR_F_HAS_SR_TB;
>>>>> @@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor
>>>>> *nor,
>>>>> loff_t ofs, uint64_t len)
>>>>>    	else
>>>>>    		lock_len = ofs + len;
>>>>>    
>>>>> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>> -		tb_mask = SR_TB_BIT6;
>>>>> +	if (lock_len == mtd->size) {
>>>>> +		val = mask; /* fully locked */
>>>>> +	} else {
>>>>> +		min_prot_len = stm_get_min_prot_length(nor);
>>>>> +		pow = ilog2(lock_len) - ilog2(min_prot_len) +
>>>>> 1;
>>>>> +		val = pow << SR_BP_SHIFT;
>>>>> +	}
>>>>>    
>>>>> -	/*
>>>>> -	 * Need smallest pow such that:
>>>>> -	 *
>>>>> -	 *   1 / (2^pow) <= (len / size)
>>>>> -	 *
>>>>> -	 * so (assuming power-of-2 size) we do:
>>>>> -	 *
>>>>> -	 *   pow = ceil(log2(size / len)) = log2(size) -
>>>>> floor(log2(len))
>>>>> -	 */
>>>>> -	pow = ilog2(mtd->size) - ilog2(lock_len);
>>>>> -	val = mask - (pow << SR_BP_SHIFT);
>>>>>    	if (val & ~mask)
>>>>>    		return -EINVAL;
>>>>>    	/* Don't "lock" with no region! */
>>>>> @@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor
>>>>> *nor,
>>>>> loff_t ofs, uint64_t len)
>>>>>    {
>>>>>    	struct mtd_info *mtd = &nor->mtd;
>>>>>    	int ret, status_old, status_new;
>>>>> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>>>> -	u8 tb_mask = SR_TB_BIT5;
>>>>> +	int min_prot_len;
>>>>> +	u8 mask = spi_nor_get_bp_mask(nor);
>>>>> +	u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>>>    	u8 pow, val;
>>>>>    	loff_t lock_len;
>>>>>    	bool can_be_top = true, can_be_bottom = nor->flags &
>>>>> SNOR_F_HAS_SR_TB;
>>>>> @@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor
>>>>> *nor,
>>>>> loff_t ofs, uint64_t len)
>>>>>    	else
>>>>>    		lock_len = ofs;
>>>>>    
>>>>> -	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>> -		tb_mask = SR_TB_BIT6;
>>>>> -	/*
>>>>> -	 * Need largest pow such that:
>>>>> -	 *
>>>>> -	 *   1 / (2^pow) >= (len / size)
>>>>> -	 *
>>>>> -	 * so (assuming power-of-2 size) we do:
>>>>> -	 *
>>>>> -	 *   pow = floor(log2(size / len)) = log2(size) -
>>>>> ceil(log2(len))
>>>>> -	 */
>>>>> -	pow = ilog2(mtd->size) - order_base_2(lock_len);
>>>>>    	if (lock_len == 0) {
>>>>>    		val = 0; /* fully unlocked */
>>>>>    	} else {
>>>>> -		val = mask - (pow << SR_BP_SHIFT);
>>>>> +		min_prot_len = stm_get_min_prot_length(nor);
>>>>> +		pow = ilog2(lock_len) - ilog2(min_prot_len) +
>>>>> 1;
>>>>> +		val = pow << SR_BP_SHIFT;
>>>>> +
>>>>>    		/* Some power-of-two sizes are not supported */
>>>>>    		if (val & ~mask)
>>>>>    			return -EINVAL;
>>>>    
>>> .
>>>
>>
>>
>
> .
>



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

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

[Jungseung Lee]

Hi, chenxiang,

On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@hisilicon.com> wrote:
>
> Hi Jungseung,
>
> 在 2020/3/9 19:44, Jungseung Lee 写道:
> > Hi,
> >
> > 2020-03-09 (월), 15:50 +0800, chenxiang (M):
> >> Hi Jungseung,
> >>
> >> 在 2020/3/7 16:24, Jungseung Lee 写道:
> >>> Hi,
> >>>
> >>> 2020-03-06 (금), 20:19 +0800, chenxiang (M):
> >>>> Hi Jungseung,
> >>>>
> >>>> 在 2020/3/4 19:07, Jungseung Lee 写道:
> >>>>> The current mainline locking was restricted and could only be
> >>>>> applied
> >>>>> to flashes that has 3 block protection bit and fixed locking
> >>>>> ratio.
> >>>>>
> >>>>> A new method of normalization was reached at the end of the
> >>>>> discussion [1].
> >>>>>
> >>>>>       (1) - if bp slot is insufficient.
> >>>>>       (2) - if bp slot is sufficient.
> >>>>>
> >>>>>       if (bp_slots_needed > bp_slots)    // (1)
> >>>>>           min_prot_length = sector_size << (bp_slots_needed -
> >>>>> bp_slots);
> >>>>>       else                               // (2)
> >>>>>           min_prot_length = sector_size;
> >>>>>
> >>>>> This patch changes block protection handling logic based on
> >>>>> min_prot_length.
> >>>>> It is suitable for the overall flashes with exception of some
> >>>>> corner case
> >>>>> and easy to extend and apply for the case of 2bit or 4bit block
> >>>>> protection.
> >>>>>
> >>>>> [1]
> >>>>>
> > https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
> >>>>
> >>>> I have tested the patchset on one of my board (there is micron
> >>>> flash
> >>>> n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR,
> >>>> TB
> >>>> bit is on bit5 of SR), so
> >>>> i modify the code as follows to enable the lock/unlock of
> >>>> n25q128a11.
> >>>> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> >>>> SECT_4K |
> >>>> SPI_NOR_QUAD_READ) },
> >>>> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
> >>>> +                       SECT_4K | SPI_NOR_QUAD_READ |
> >>>> +                       USE_FSR | SPI_NOR_HAS_LOCK |
> >>>> SPI_NOR_HAS_TB |
> >>>> +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
> >>>>
> >>>> There are two issues i met:
> >>>> (1) i lock/unlock the full region of the flash, lock is valid,
> >>>> but
> >>>> there is error when unlock the flash, i query the status of it is
> >>>> unlock (the issue i think it is
> >>>> the same as the issue John has reported before
> >>>>
> > https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/
> >>>> ),
> >>>>
> >>>> i screenshot the log of the operation as follows:
> >>>>
> >>> Looks like the unlock operation was actually done (as can be
> >>> checked
> >>> from the following query of the status) but an error is coming with
> >>> EIO.
> >>>
> >>> I think another part of sr handling is related with your case.
> >>> (maybe
> >>> SR read back test ?)
> >> Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL
> >> is
> >> set), but it reads back 0x0 (bit WEL is cleared).
> >>
> > I am reviewing tudor's patches and it seems solve your issue
> > effectively.
> > http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html
>
> Yes, it solves my issue.
>
> >
> >>> If you can dump the sr value & dev_dbg msg, it will be helpful to
> >>> define this issue.
> >>>
> >>>> (2) i try to lock part of the flash region such as ./flash_lock
> >>>> /dev/mtd0 0xc00000 10, it reports invalid argument,
> >>>> and i am not sure whether it is something wrong with my
> >>>> operation.
> >>>>
> >>> It is unable to lock such region since the spi flash doesn't
> >>> support
> >>> it. only we can lock it from the top or the bottom.
> >>>
> >>> like this for n25q128a11,
> >>>
> >>> flash_lock /dev/mtd0 0xff0000 0x10
> >>> flash_lock /dev/mtd0 0x0 0x10
> >> Do you mean if lock/unlcok from top,  the address of lock/unlock
> >> commands should be the address of 255th block (0xff0000), 254th
> >> block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
> >> If lock/unlock from bottom, the address of lock/unlock commands
> >> should
> >> be always the address of 0th block (0x0)?
> >>
> > I'm not fully understanding the usage of flash_lock, but it would be
> > better to use such addresses for lock/unlocking to make it under
> > control.
> >
> > There are some ambiguous parts to explain that since some lock/unlock
> > operation is still working well without the addresses.
> >
> > LOCK
> > - Return success if the requested area is already locked.
> > - If requested area is not fully matched with lock portion of the
> > flash, lock some of the portion including the request area as it could
> > be.
> >
> > UNLOCK
> > - Return success if the requested area is already unlocked.
> > - If requested area is not fully matched with lock portion of the
> > flash, unlock all locked portion including the request area. the
> > portion would be bigger than requested area.
>
> Thanks for you info.
> I have tested above situations of lock and unlock, and still have a
> question about it:
> For unlock function, as you said, it will unlock all the locked portion
> including the request area which would be bigger than requested area if
> requested area is not fully matched with lock portion of the flash.
> But for lock function, it seem not lock some of portion including the
> request area as it could be, and it seems require the total locked area
> must be matched with
> some portion of the flash (it seems not allow hole between those regions).
>

Yes it is. The spi flash consequently controls the region that will be
locked through only one bp value on sr register.
I wrote only some of the patterns I checked in the current mainline
code, and frankly, I don't know if even this is always right in all
combinations.

Thanks,

> For example, 16MB in my envirnment, i do as follows:
> - lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched
> with lock portion of the flash (BP3~0 = 0001, TB=0)
> - lock [0xc00000, 0xff0000] or [0xc00000, 0xff1000]   -> it also matched
> with lock portion of the flash (BP3~0 = 0111, TB=0)
> but if do it as follows:
> - lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched
> with lock portion of the flash (BP3~0 = 0001, TB=0)
> - lock [0xc00000, 0xc10000]   -> it will report invalid argument at the
> second time, in my thought it would lock [0xc00000, 0x1000000] which
> will including those two regions
>
> >
> > So, the lock/unlock would be able to work without the addresses. but in
> > my case I don't use the way because it will makes hard to tracking the
> > locked area.
> >
> > Thanks,
> >
> >>> Note the block count of examples is 0x10 not 10. The locking try
> >>> with
> >>> block count under minimum block protection length will be failed.
> >>>
> >>> Thanks,
> >>>
> >>>>> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> >>>>> ---
> >>>>>    drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++-----
> >>>>> ---
> >>>>> ------
> >>>>>    1 file changed, 66 insertions(+), 44 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
> >>>>> nor/spi-nor.c
> >>>>> index caf0c109cca0..c58c27552a74 100644
> >>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>>> @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
> >>>>> mtd_info
> >>>>> *mtd, struct erase_info *instr)
> >>>>>           return ret;
> >>>>>    }
> >>>>>
> >>>>> +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> >>>>> +{
> >>>>> + return SR_BP2 | SR_BP1 | SR_BP0;
> >>>>> +}
> >>>>> +
> >>>>> +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> >>>>> +{
> >>>>> + if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >>>>> +         return SR_TB_BIT6;
> >>>>> + else
> >>>>> +         return SR_TB_BIT5;
> >>>>> +}
> >>>>> +
> >>>>> +static int stm_get_min_prot_length(struct spi_nor *nor)
> >>>>> +{
> >>>>> + int bp_slots, bp_slots_needed;
> >>>>> + u8 mask = spi_nor_get_bp_mask(nor);
> >>>>> +
> >>>>> + bp_slots = (mask >> SR_BP_SHIFT) + 1;
> >>>>> +
> >>>>> + /* Reserved one for "protect none" and one for "protect
> >>>>> all".
> >>>>> */
> >>>>> + bp_slots = bp_slots - 2;
> >>>>> +
> >>>>> + bp_slots_needed = ilog2(nor->info->n_sectors);
> >>>>> +
> >>>>> + if (bp_slots_needed > bp_slots)
> >>>>> +         return nor->info->sector_size <<
> >>>>> +                 (bp_slots_needed - bp_slots);
> >>>>> + else
> >>>>> +         return nor->info->sector_size;
> >>>>> +}
> >>>>> +
> >>>>>    static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
> >>>>> loff_t *ofs,
> >>>>>                                    uint64_t *len)
> >>>>>    {
> >>>>>           struct mtd_info *mtd = &nor->mtd;
> >>>>> - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> >>>>> - u8 tb_mask = SR_TB_BIT5;
> >>>>> - int pow;
> >>>>> + int min_prot_len;
> >>>>> + u8 mask = spi_nor_get_bp_mask(nor);
> >>>>> + u8 tb_mask = spi_nor_get_tb_mask(nor);
> >>>>> + u8 bp = (sr & mask) >> SR_BP_SHIFT;
> >>>>>
> >>>>> - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >>>>> -         tb_mask = SR_TB_BIT6;
> >>>>> -
> >>>>> - if (!(sr & mask)) {
> >>>>> + if (!bp) {
> >>>>>                   /* No protection */
> >>>>>                   *ofs = 0;
> >>>>>                   *len = 0;
> >>>>> - } else {
> >>>>> -         pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> >>>>> -         *len = mtd->size >> pow;
> >>>>> -         if (nor->flags & SNOR_F_HAS_SR_TB && sr &
> >>>>> tb_mask)
> >>>>> -                 *ofs = 0;
> >>>>> -         else
> >>>>> -                 *ofs = mtd->size - *len;
> >>>>> +         return;
> >>>>>           }
> >>>>> +
> >>>>> + min_prot_len = stm_get_min_prot_length(nor);
> >>>>> + *len = min_prot_len << (bp - 1);
> >>>>> +
> >>>>> + if (*len > mtd->size)
> >>>>> +         *len = mtd->size;
> >>>>> +
> >>>>> + if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> >>>>> +         *ofs = 0;
> >>>>> + else
> >>>>> +         *ofs = mtd->size - *len;
> >>>>>    }
> >>>>>
> >>>>>    /*
> >>>>> @@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
> >>>>> loff_t ofs, uint64_t len)
> >>>>>    {
> >>>>>           struct mtd_info *mtd = &nor->mtd;
> >>>>>           int ret, status_old, status_new;
> >>>>> - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> >>>>> - u8 tb_mask = SR_TB_BIT5;
> >>>>> + int min_prot_len;
> >>>>> + u8 mask = spi_nor_get_bp_mask(nor);
> >>>>> + u8 tb_mask = spi_nor_get_tb_mask(nor);
> >>>>>           u8 pow, val;
> >>>>>           loff_t lock_len;
> >>>>>           bool can_be_top = true, can_be_bottom = nor->flags &
> >>>>> SNOR_F_HAS_SR_TB;
> >>>>> @@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor
> >>>>> *nor,
> >>>>> loff_t ofs, uint64_t len)
> >>>>>           else
> >>>>>                   lock_len = ofs + len;
> >>>>>
> >>>>> - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >>>>> -         tb_mask = SR_TB_BIT6;
> >>>>> + if (lock_len == mtd->size) {
> >>>>> +         val = mask; /* fully locked */
> >>>>> + } else {
> >>>>> +         min_prot_len = stm_get_min_prot_length(nor);
> >>>>> +         pow = ilog2(lock_len) - ilog2(min_prot_len) +
> >>>>> 1;
> >>>>> +         val = pow << SR_BP_SHIFT;
> >>>>> + }
> >>>>>
> >>>>> - /*
> >>>>> -  * Need smallest pow such that:
> >>>>> -  *
> >>>>> -  *   1 / (2^pow) <= (len / size)
> >>>>> -  *
> >>>>> -  * so (assuming power-of-2 size) we do:
> >>>>> -  *
> >>>>> -  *   pow = ceil(log2(size / len)) = log2(size) -
> >>>>> floor(log2(len))
> >>>>> -  */
> >>>>> - pow = ilog2(mtd->size) - ilog2(lock_len);
> >>>>> - val = mask - (pow << SR_BP_SHIFT);
> >>>>>           if (val & ~mask)
> >>>>>                   return -EINVAL;
> >>>>>           /* Don't "lock" with no region! */
> >>>>> @@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor
> >>>>> *nor,
> >>>>> loff_t ofs, uint64_t len)
> >>>>>    {
> >>>>>           struct mtd_info *mtd = &nor->mtd;
> >>>>>           int ret, status_old, status_new;
> >>>>> - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> >>>>> - u8 tb_mask = SR_TB_BIT5;
> >>>>> + int min_prot_len;
> >>>>> + u8 mask = spi_nor_get_bp_mask(nor);
> >>>>> + u8 tb_mask = spi_nor_get_tb_mask(nor);
> >>>>>           u8 pow, val;
> >>>>>           loff_t lock_len;
> >>>>>           bool can_be_top = true, can_be_bottom = nor->flags &
> >>>>> SNOR_F_HAS_SR_TB;
> >>>>> @@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor
> >>>>> *nor,
> >>>>> loff_t ofs, uint64_t len)
> >>>>>           else
> >>>>>                   lock_len = ofs;
> >>>>>
> >>>>> - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> >>>>> -         tb_mask = SR_TB_BIT6;
> >>>>> - /*
> >>>>> -  * Need largest pow such that:
> >>>>> -  *
> >>>>> -  *   1 / (2^pow) >= (len / size)
> >>>>> -  *
> >>>>> -  * so (assuming power-of-2 size) we do:
> >>>>> -  *
> >>>>> -  *   pow = floor(log2(size / len)) = log2(size) -
> >>>>> ceil(log2(len))
> >>>>> -  */
> >>>>> - pow = ilog2(mtd->size) - order_base_2(lock_len);
> >>>>>           if (lock_len == 0) {
> >>>>>                   val = 0; /* fully unlocked */
> >>>>>           } else {
> >>>>> -         val = mask - (pow << SR_BP_SHIFT);
> >>>>> +         min_prot_len = stm_get_min_prot_length(nor);
> >>>>> +         pow = ilog2(lock_len) - ilog2(min_prot_len) +
> >>>>> 1;
> >>>>> +         val = pow << SR_BP_SHIFT;
> >>>>> +
> >>>>>                   /* Some power-of-two sizes are not supported */
> >>>>>                   if (val & ~mask)
> >>>>>                           return -EINVAL;
> >>>>
> >>> .
> >>>
> >>
> >>
> >
> > .
> >
>
>

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

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

[chenxiang (M)]

Hi Jungseung,

在 2020/3/14 21:50, Jungseung Lee 写道:
> Hi, chenxiang,
>
> On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@hisilicon.com> wrote:
>> Hi Jungseung,
>>
>> 在 2020/3/9 19:44, Jungseung Lee 写道:
>>> Hi,
>>>
>>> 2020-03-09 (월), 15:50 +0800, chenxiang (M):
>>>> Hi Jungseung,
>>>>
>>>> 在 2020/3/7 16:24, Jungseung Lee 写道:
>>>>> Hi,
>>>>>
>>>>> 2020-03-06 (금), 20:19 +0800, chenxiang (M):
>>>>>> Hi Jungseung,
>>>>>>
>>>>>> 在 2020/3/4 19:07, Jungseung Lee 写道:
>>>>>>> The current mainline locking was restricted and could only be
>>>>>>> applied
>>>>>>> to flashes that has 3 block protection bit and fixed locking
>>>>>>> ratio.
>>>>>>>
>>>>>>> A new method of normalization was reached at the end of the
>>>>>>> discussion [1].
>>>>>>>
>>>>>>>        (1) - if bp slot is insufficient.
>>>>>>>        (2) - if bp slot is sufficient.
>>>>>>>
>>>>>>>        if (bp_slots_needed > bp_slots)    // (1)
>>>>>>>            min_prot_length = sector_size << (bp_slots_needed -
>>>>>>> bp_slots);
>>>>>>>        else                               // (2)
>>>>>>>            min_prot_length = sector_size;
>>>>>>>
>>>>>>> This patch changes block protection handling logic based on
>>>>>>> min_prot_length.
>>>>>>> It is suitable for the overall flashes with exception of some
>>>>>>> corner case
>>>>>>> and easy to extend and apply for the case of 2bit or 4bit block
>>>>>>> protection.
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>> https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
>>>>>> I have tested the patchset on one of my board (there is micron
>>>>>> flash
>>>>>> n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR,
>>>>>> TB
>>>>>> bit is on bit5 of SR), so
>>>>>> i modify the code as follows to enable the lock/unlock of
>>>>>> n25q128a11.
>>>>>> -       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
>>>>>> SECT_4K |
>>>>>> SPI_NOR_QUAD_READ) },
>>>>>> +       { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256,
>>>>>> +                       SECT_4K | SPI_NOR_QUAD_READ |
>>>>>> +                       USE_FSR | SPI_NOR_HAS_LOCK |
>>>>>> SPI_NOR_HAS_TB |
>>>>>> +                       SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
>>>>>>
>>>>>> There are two issues i met:
>>>>>> (1) i lock/unlock the full region of the flash, lock is valid,
>>>>>> but
>>>>>> there is error when unlock the flash, i query the status of it is
>>>>>> unlock (the issue i think it is
>>>>>> the same as the issue John has reported before
>>>>>>
>>> https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/
>>>>>> ),
>>>>>>
>>>>>> i screenshot the log of the operation as follows:
>>>>>>
>>>>> Looks like the unlock operation was actually done (as can be
>>>>> checked
>>>>> from the following query of the status) but an error is coming with
>>>>> EIO.
>>>>>
>>>>> I think another part of sr handling is related with your case.
>>>>> (maybe
>>>>> SR read back test ?)
>>>> Yes,  it is the issue of SR read back test:  it writes 0X2 (bit WEL
>>>> is
>>>> set), but it reads back 0x0 (bit WEL is cleared).
>>>>
>>> I am reviewing tudor's patches and it seems solve your issue
>>> effectively.
>>> http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html
>> Yes, it solves my issue.
>>
>>>>> If you can dump the sr value & dev_dbg msg, it will be helpful to
>>>>> define this issue.
>>>>>
>>>>>> (2) i try to lock part of the flash region such as ./flash_lock
>>>>>> /dev/mtd0 0xc00000 10, it reports invalid argument,
>>>>>> and i am not sure whether it is something wrong with my
>>>>>> operation.
>>>>>>
>>>>> It is unable to lock such region since the spi flash doesn't
>>>>> support
>>>>> it. only we can lock it from the top or the bottom.
>>>>>
>>>>> like this for n25q128a11,
>>>>>
>>>>> flash_lock /dev/mtd0 0xff0000 0x10
>>>>> flash_lock /dev/mtd0 0x0 0x10
>>>> Do you mean if lock/unlcok from top,  the address of lock/unlock
>>>> commands should be the address of 255th block (0xff0000), 254th
>>>> block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
>>>> If lock/unlock from bottom, the address of lock/unlock commands
>>>> should
>>>> be always the address of 0th block (0x0)?
>>>>
>>> I'm not fully understanding the usage of flash_lock, but it would be
>>> better to use such addresses for lock/unlocking to make it under
>>> control.
>>>
>>> There are some ambiguous parts to explain that since some lock/unlock
>>> operation is still working well without the addresses.
>>>
>>> LOCK
>>> - Return success if the requested area is already locked.
>>> - If requested area is not fully matched with lock portion of the
>>> flash, lock some of the portion including the request area as it could
>>> be.
>>>
>>> UNLOCK
>>> - Return success if the requested area is already unlocked.
>>> - If requested area is not fully matched with lock portion of the
>>> flash, unlock all locked portion including the request area. the
>>> portion would be bigger than requested area.
>> Thanks for you info.
>> I have tested above situations of lock and unlock, and still have a
>> question about it:
>> For unlock function, as you said, it will unlock all the locked portion
>> including the request area which would be bigger than requested area if
>> requested area is not fully matched with lock portion of the flash.
>> But for lock function, it seem not lock some of portion including the
>> request area as it could be, and it seems require the total locked area
>> must be matched with
>> some portion of the flash (it seems not allow hole between those regions).
>>
> Yes it is. The spi flash consequently controls the region that will be
> locked through only one bp value on sr register.
> I wrote only some of the patterns I checked in the current mainline
> code, and frankly, I don't know if even this is always right in all
> combinations.

Ok, thanks.
So i have tested those patchset + (enabled n25q128a11 private patch) on 
flash n25q128a11, and it is ok, so you can add : Tested-by: Xiang Chen 
<chenxiang66@hisilicon.com>.
If there would be next version, i will test them also.

> Thanks,
>
>> For example, 16MB in my envirnment, i do as follows:
>> - lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched
>> with lock portion of the flash (BP3~0 = 0001, TB=0)
>> - lock [0xc00000, 0xff0000] or [0xc00000, 0xff1000]   -> it also matched
>> with lock portion of the flash (BP3~0 = 0111, TB=0)
>> but if do it as follows:
>> - lock [0xff0000, 0x1000000] which is the 255th block   -> it is matched
>> with lock portion of the flash (BP3~0 = 0001, TB=0)
>> - lock [0xc00000, 0xc10000]   -> it will report invalid argument at the
>> second time, in my thought it would lock [0xc00000, 0x1000000] which
>> will including those two regions
>>
>>> So, the lock/unlock would be able to work without the addresses. but in
>>> my case I don't use the way because it will makes hard to tracking the
>>> locked area.
>>>
>>> Thanks,
>>>
>>>>> Note the block count of examples is 0x10 not 10. The locking try
>>>>> with
>>>>> block count under minimum block protection length will be failed.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>>>>>>> ---
>>>>>>>     drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++-----
>>>>>>> ---
>>>>>>> ------
>>>>>>>     1 file changed, 66 insertions(+), 44 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
>>>>>>> nor/spi-nor.c
>>>>>>> index caf0c109cca0..c58c27552a74 100644
>>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>>>> @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
>>>>>>> mtd_info
>>>>>>> *mtd, struct erase_info *instr)
>>>>>>>            return ret;
>>>>>>>     }
>>>>>>>
>>>>>>> +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>>>>>>> +{
>>>>>>> + return SR_BP2 | SR_BP1 | SR_BP0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
>>>>>>> +{
>>>>>>> + if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>>>> +         return SR_TB_BIT6;
>>>>>>> + else
>>>>>>> +         return SR_TB_BIT5;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int stm_get_min_prot_length(struct spi_nor *nor)
>>>>>>> +{
>>>>>>> + int bp_slots, bp_slots_needed;
>>>>>>> + u8 mask = spi_nor_get_bp_mask(nor);
>>>>>>> +
>>>>>>> + bp_slots = (mask >> SR_BP_SHIFT) + 1;
>>>>>>> +
>>>>>>> + /* Reserved one for "protect none" and one for "protect
>>>>>>> all".
>>>>>>> */
>>>>>>> + bp_slots = bp_slots - 2;
>>>>>>> +
>>>>>>> + bp_slots_needed = ilog2(nor->info->n_sectors);
>>>>>>> +
>>>>>>> + if (bp_slots_needed > bp_slots)
>>>>>>> +         return nor->info->sector_size <<
>>>>>>> +                 (bp_slots_needed - bp_slots);
>>>>>>> + else
>>>>>>> +         return nor->info->sector_size;
>>>>>>> +}
>>>>>>> +
>>>>>>>     static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
>>>>>>> loff_t *ofs,
>>>>>>>                                     uint64_t *len)
>>>>>>>     {
>>>>>>>            struct mtd_info *mtd = &nor->mtd;
>>>>>>> - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>>>>>> - u8 tb_mask = SR_TB_BIT5;
>>>>>>> - int pow;
>>>>>>> + int min_prot_len;
>>>>>>> + u8 mask = spi_nor_get_bp_mask(nor);
>>>>>>> + u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>>>>> + u8 bp = (sr & mask) >> SR_BP_SHIFT;
>>>>>>>
>>>>>>> - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>>>> -         tb_mask = SR_TB_BIT6;
>>>>>>> -
>>>>>>> - if (!(sr & mask)) {
>>>>>>> + if (!bp) {
>>>>>>>                    /* No protection */
>>>>>>>                    *ofs = 0;
>>>>>>>                    *len = 0;
>>>>>>> - } else {
>>>>>>> -         pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
>>>>>>> -         *len = mtd->size >> pow;
>>>>>>> -         if (nor->flags & SNOR_F_HAS_SR_TB && sr &
>>>>>>> tb_mask)
>>>>>>> -                 *ofs = 0;
>>>>>>> -         else
>>>>>>> -                 *ofs = mtd->size - *len;
>>>>>>> +         return;
>>>>>>>            }
>>>>>>> +
>>>>>>> + min_prot_len = stm_get_min_prot_length(nor);
>>>>>>> + *len = min_prot_len << (bp - 1);
>>>>>>> +
>>>>>>> + if (*len > mtd->size)
>>>>>>> +         *len = mtd->size;
>>>>>>> +
>>>>>>> + if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
>>>>>>> +         *ofs = 0;
>>>>>>> + else
>>>>>>> +         *ofs = mtd->size - *len;
>>>>>>>     }
>>>>>>>
>>>>>>>     /*
>>>>>>> @@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
>>>>>>> loff_t ofs, uint64_t len)
>>>>>>>     {
>>>>>>>            struct mtd_info *mtd = &nor->mtd;
>>>>>>>            int ret, status_old, status_new;
>>>>>>> - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>>>>>> - u8 tb_mask = SR_TB_BIT5;
>>>>>>> + int min_prot_len;
>>>>>>> + u8 mask = spi_nor_get_bp_mask(nor);
>>>>>>> + u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>>>>>            u8 pow, val;
>>>>>>>            loff_t lock_len;
>>>>>>>            bool can_be_top = true, can_be_bottom = nor->flags &
>>>>>>> SNOR_F_HAS_SR_TB;
>>>>>>> @@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor
>>>>>>> *nor,
>>>>>>> loff_t ofs, uint64_t len)
>>>>>>>            else
>>>>>>>                    lock_len = ofs + len;
>>>>>>>
>>>>>>> - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>>>> -         tb_mask = SR_TB_BIT6;
>>>>>>> + if (lock_len == mtd->size) {
>>>>>>> +         val = mask; /* fully locked */
>>>>>>> + } else {
>>>>>>> +         min_prot_len = stm_get_min_prot_length(nor);
>>>>>>> +         pow = ilog2(lock_len) - ilog2(min_prot_len) +
>>>>>>> 1;
>>>>>>> +         val = pow << SR_BP_SHIFT;
>>>>>>> + }
>>>>>>>
>>>>>>> - /*
>>>>>>> -  * Need smallest pow such that:
>>>>>>> -  *
>>>>>>> -  *   1 / (2^pow) <= (len / size)
>>>>>>> -  *
>>>>>>> -  * so (assuming power-of-2 size) we do:
>>>>>>> -  *
>>>>>>> -  *   pow = ceil(log2(size / len)) = log2(size) -
>>>>>>> floor(log2(len))
>>>>>>> -  */
>>>>>>> - pow = ilog2(mtd->size) - ilog2(lock_len);
>>>>>>> - val = mask - (pow << SR_BP_SHIFT);
>>>>>>>            if (val & ~mask)
>>>>>>>                    return -EINVAL;
>>>>>>>            /* Don't "lock" with no region! */
>>>>>>> @@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor
>>>>>>> *nor,
>>>>>>> loff_t ofs, uint64_t len)
>>>>>>>     {
>>>>>>>            struct mtd_info *mtd = &nor->mtd;
>>>>>>>            int ret, status_old, status_new;
>>>>>>> - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>>>>>>> - u8 tb_mask = SR_TB_BIT5;
>>>>>>> + int min_prot_len;
>>>>>>> + u8 mask = spi_nor_get_bp_mask(nor);
>>>>>>> + u8 tb_mask = spi_nor_get_tb_mask(nor);
>>>>>>>            u8 pow, val;
>>>>>>>            loff_t lock_len;
>>>>>>>            bool can_be_top = true, can_be_bottom = nor->flags &
>>>>>>> SNOR_F_HAS_SR_TB;
>>>>>>> @@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor
>>>>>>> *nor,
>>>>>>> loff_t ofs, uint64_t len)
>>>>>>>            else
>>>>>>>                    lock_len = ofs;
>>>>>>>
>>>>>>> - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
>>>>>>> -         tb_mask = SR_TB_BIT6;
>>>>>>> - /*
>>>>>>> -  * Need largest pow such that:
>>>>>>> -  *
>>>>>>> -  *   1 / (2^pow) >= (len / size)
>>>>>>> -  *
>>>>>>> -  * so (assuming power-of-2 size) we do:
>>>>>>> -  *
>>>>>>> -  *   pow = floor(log2(size / len)) = log2(size) -
>>>>>>> ceil(log2(len))
>>>>>>> -  */
>>>>>>> - pow = ilog2(mtd->size) - order_base_2(lock_len);
>>>>>>>            if (lock_len == 0) {
>>>>>>>                    val = 0; /* fully unlocked */
>>>>>>>            } else {
>>>>>>> -         val = mask - (pow << SR_BP_SHIFT);
>>>>>>> +         min_prot_len = stm_get_min_prot_length(nor);
>>>>>>> +         pow = ilog2(lock_len) - ilog2(min_prot_len) +
>>>>>>> 1;
>>>>>>> +         val = pow << SR_BP_SHIFT;
>>>>>>> +
>>>>>>>                    /* Some power-of-two sizes are not supported */
>>>>>>>                    if (val & ~mask)
>>>>>>>                            return -EINVAL;
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
> .
>



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

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

[Jungseung Lee]

Hi,

On Mon, 2020-03-16 at 15:21 +0800, chenxiang (M) wrote:
> Hi Jungseung,
> 
> 在 2020/3/14 21:50, Jungseung Lee 写道:
> > Hi, chenxiang,
> > 
> > On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <
> > chenxiang66@hisilicon.com> wrote:
> > > Hi Jungseung,
> > > 
> > > 在 2020/3/9 19:44, Jungseung Lee 写道:
> > > > Hi,
> > > > 
> > > > 2020-03-09 (월), 15:50 +0800, chenxiang (M):
> > > > > Hi Jungseung,
> > > > > 
> > > > > 在 2020/3/7 16:24, Jungseung Lee 写道:
> > > > > > Hi,
> > > > > > 
> > > > > > 2020-03-06 (금), 20:19 +0800, chenxiang (M):
> > > > > > > Hi Jungseung,
> > > > > > > 
> > > > > > > 在 2020/3/4 19:07, Jungseung Lee 写道:
> > > > > > > > The current mainline locking was restricted and could
> > > > > > > > only be
> > > > > > > > applied
> > > > > > > > to flashes that has 3 block protection bit and fixed
> > > > > > > > locking
> > > > > > > > ratio.
> > > > > > > > 
> > > > > > > > A new method of normalization was reached at the end of
> > > > > > > > the
> > > > > > > > discussion [1].
> > > > > > > > 
> > > > > > > >        (1) - if bp slot is insufficient.
> > > > > > > >        (2) - if bp slot is sufficient.
> > > > > > > > 
> > > > > > > >        if (bp_slots_needed > bp_slots)    // (1)
> > > > > > > >            min_prot_length = sector_size <<
> > > > > > > > (bp_slots_needed -
> > > > > > > > bp_slots);
> > > > > > > >        else                               // (2)
> > > > > > > >            min_prot_length = sector_size;
> > > > > > > > 
> > > > > > > > This patch changes block protection handling logic
> > > > > > > > based on
> > > > > > > > min_prot_length.
> > > > > > > > It is suitable for the overall flashes with exception
> > > > > > > > of some
> > > > > > > > corner case
> > > > > > > > and easy to extend and apply for the case of 2bit or
> > > > > > > > 4bit block
> > > > > > > > protection.
> > > > > > > > 
> > > > > > > > [1]
> > > > > > > > 
> > > > 
> > > > 
https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
> > > > > > > I have tested the patchset on one of my board (there is
> > > > > > > micron
> > > > > > > flash
> > > > > > > n25q128a11 which supports 4bit BP, and also bp3 is on
> > > > > > > bit6 of SR,
> > > > > > > TB
> > > > > > > bit is on bit5 of SR), so
> > > > > > > i modify the code as follows to enable the lock/unlock of
> > > > > > > n25q128a11.
> > > > > > > -       { "n25q128a11",  INFO(0x20bb18, 0, 64 *
> > > > > > > 1024,  256,
> > > > > > > SECT_4K |
> > > > > > > SPI_NOR_QUAD_READ) },
> > > > > > > +       { "n25q128a11",  INFO(0x20bb18, 0, 64 *
> > > > > > > 1024,  256,
> > > > > > > +                       SECT_4K | SPI_NOR_QUAD_READ |
> > > > > > > +                       USE_FSR | SPI_NOR_HAS_LOCK |
> > > > > > > SPI_NOR_HAS_TB |
> > > > > > > +                       SPI_NOR_HAS_BP3 |
> > > > > > > SPI_NOR_BP3_SR_BIT6) },
> > > > > > > 
> > > > > > > There are two issues i met:
> > > > > > > (1) i lock/unlock the full region of the flash, lock is
> > > > > > > valid,
> > > > > > > but
> > > > > > > there is error when unlock the flash, i query the status
> > > > > > > of it is
> > > > > > > unlock (the issue i think it is
> > > > > > > the same as the issue John has reported before
> > > > > > > 
> > > > 
> > > > 
https://protect2.fireeye.com/url?k=ed8659ca-b0544ec3-ed87d285-0cc47a31cdf8-aa60cbf507f7bb2c&u=https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@microchip.com/
> > > > > > > ),
> > > > > > > 
> > > > > > > i screenshot the log of the operation as follows:
> > > > > > > 
> > > > > > 
> > > > > > Looks like the unlock operation was actually done (as can
> > > > > > be
> > > > > > checked
> > > > > > from the following query of the status) but an error is
> > > > > > coming with
> > > > > > EIO.
> > > > > > 
> > > > > > I think another part of sr handling is related with your
> > > > > > case.
> > > > > > (maybe
> > > > > > SR read back test ?)
> > > > > 
> > > > > Yes,  it is the issue of SR read back test:  it writes 0X2
> > > > > (bit WEL
> > > > > is
> > > > > set), but it reads back 0x0 (bit WEL is cleared).
> > > > > 
> > > > 
> > > > I am reviewing tudor's patches and it seems solve your issue
> > > > effectively.
> > > > 
https://protect2.fireeye.com/url?k=a6aef5a7-fb7ce2ae-a6af7ee8-0cc47a31cdf8-1b34841aa21abc3e&u=http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html
> > > 
> > > Yes, it solves my issue.
> > > 
> > > > > > If you can dump the sr value & dev_dbg msg, it will be
> > > > > > helpful to
> > > > > > define this issue.
> > > > > > 
> > > > > > > (2) i try to lock part of the flash region such as
> > > > > > > ./flash_lock
> > > > > > > /dev/mtd0 0xc00000 10, it reports invalid argument,
> > > > > > > and i am not sure whether it is something wrong with my
> > > > > > > operation.
> > > > > > > 
> > > > > > 
> > > > > > It is unable to lock such region since the spi flash
> > > > > > doesn't
> > > > > > support
> > > > > > it. only we can lock it from the top or the bottom.
> > > > > > 
> > > > > > like this for n25q128a11,
> > > > > > 
> > > > > > flash_lock /dev/mtd0 0xff0000 0x10
> > > > > > flash_lock /dev/mtd0 0x0 0x10
> > > > > 
> > > > > Do you mean if lock/unlcok from top,  the address of
> > > > > lock/unlock
> > > > > commands should be the address of 255th block (0xff0000),
> > > > > 254th
> > > > > block(0xfe0000), 252nd block(0xfc0000), ...., 128th block
> > > > > (0x800000)?
> > > > > If lock/unlock from bottom, the address of lock/unlock
> > > > > commands
> > > > > should
> > > > > be always the address of 0th block (0x0)?
> > > > > 
> > > > 
> > > > I'm not fully understanding the usage of flash_lock, but it
> > > > would be
> > > > better to use such addresses for lock/unlocking to make it
> > > > under
> > > > control.
> > > > 
> > > > There are some ambiguous parts to explain that since some
> > > > lock/unlock
> > > > operation is still working well without the addresses.
> > > > 
> > > > LOCK
> > > > - Return success if the requested area is already locked.
> > > > - If requested area is not fully matched with lock portion of
> > > > the
> > > > flash, lock some of the portion including the request area as
> > > > it could
> > > > be.
> > > > 
> > > > UNLOCK
> > > > - Return success if the requested area is already unlocked.
> > > > - If requested area is not fully matched with lock portion of
> > > > the
> > > > flash, unlock all locked portion including the request area.
> > > > the
> > > > portion would be bigger than requested area.
> > > 
> > > Thanks for you info.
> > > I have tested above situations of lock and unlock, and still have
> > > a
> > > question about it:
> > > For unlock function, as you said, it will unlock all the locked
> > > portion
> > > including the request area which would be bigger than requested
> > > area if
> > > requested area is not fully matched with lock portion of the
> > > flash.
> > > But for lock function, it seem not lock some of portion including
> > > the
> > > request area as it could be, and it seems require the total
> > > locked area
> > > must be matched with
> > > some portion of the flash (it seems not allow hole between those
> > > regions).
> > > 
> > 
> > Yes it is. The spi flash consequently controls the region that will
> > be
> > locked through only one bp value on sr register.
> > I wrote only some of the patterns I checked in the current mainline
> > code, and frankly, I don't know if even this is always right in all
> > combinations.
> 
> Ok, thanks.
> So i have tested those patchset + (enabled n25q128a11 private patch)
> on 
> flash n25q128a11, and it is ok, so you can add : Tested-by: Xiang
> Chen 
> <chenxiang66@hisilicon.com>.
> If there would be next version, i will test them also.
> 

Good, I'll post new version by the end of the day.

Thanks,

> > Thanks,
> > 
> > > For example, 16MB in my envirnment, i do as follows:
> > > - lock [0xff0000, 0x1000000] which is the 255th block   -> it is
> > > matched
> > > with lock portion of the flash (BP3~0 = 0001, TB=0)
> > > - lock [0xc00000, 0xff0000] or [0xc00000, 0xff1000]   -> it also
> > > matched
> > > with lock portion of the flash (BP3~0 = 0111, TB=0)
> > > but if do it as follows:
> > > - lock [0xff0000, 0x1000000] which is the 255th block   -> it is
> > > matched
> > > with lock portion of the flash (BP3~0 = 0001, TB=0)
> > > - lock [0xc00000, 0xc10000]   -> it will report invalid argument
> > > at the
> > > second time, in my thought it would lock [0xc00000, 0x1000000]
> > > which
> > > will including those two regions
> > > 
> > > > So, the lock/unlock would be able to work without the
> > > > addresses. but in
> > > > my case I don't use the way because it will makes hard to
> > > > tracking the
> > > > locked area.
> > > > 
> > > > Thanks,
> > > > 
> > > > > > Note the block count of examples is 0x10 not 10. The
> > > > > > locking try
> > > > > > with
> > > > > > block count under minimum block protection length will be
> > > > > > failed.
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > > > > > > ---
> > > > > > > >     drivers/mtd/spi-nor/spi-nor.c | 110
> > > > > > > > ++++++++++++++++++++-----
> > > > > > > > ---
> > > > > > > > ------
> > > > > > > >     1 file changed, 66 insertions(+), 44 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > > > > > b/drivers/mtd/spi-
> > > > > > > > nor/spi-nor.c
> > > > > > > > index caf0c109cca0..c58c27552a74 100644
> > > > > > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > > > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > > > > > @@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
> > > > > > > > mtd_info
> > > > > > > > *mtd, struct erase_info *instr)
> > > > > > > >            return ret;
> > > > > > > >     }
> > > > > > > > 
> > > > > > > > +static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > > > > > > > +{
> > > > > > > > + return SR_BP2 | SR_BP1 | SR_BP0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > > > > > > +{
> > > > > > > > + if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > > > > > +         return SR_TB_BIT6;
> > > > > > > > + else
> > > > > > > > +         return SR_TB_BIT5;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int stm_get_min_prot_length(struct spi_nor
> > > > > > > > *nor)
> > > > > > > > +{
> > > > > > > > + int bp_slots, bp_slots_needed;
> > > > > > > > + u8 mask = spi_nor_get_bp_mask(nor);
> > > > > > > > +
> > > > > > > > + bp_slots = (mask >> SR_BP_SHIFT) + 1;
> > > > > > > > +
> > > > > > > > + /* Reserved one for "protect none" and one for
> > > > > > > > "protect
> > > > > > > > all".
> > > > > > > > */
> > > > > > > > + bp_slots = bp_slots - 2;
> > > > > > > > +
> > > > > > > > + bp_slots_needed = ilog2(nor->info->n_sectors);
> > > > > > > > +
> > > > > > > > + if (bp_slots_needed > bp_slots)
> > > > > > > > +         return nor->info->sector_size <<
> > > > > > > > +                 (bp_slots_needed - bp_slots);
> > > > > > > > + else
> > > > > > > > +         return nor->info->sector_size;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >     static void stm_get_locked_range(struct spi_nor
> > > > > > > > *nor, u8 sr,
> > > > > > > > loff_t *ofs,
> > > > > > > >                                     uint64_t *len)
> > > > > > > >     {
> > > > > > > >            struct mtd_info *mtd = &nor->mtd;
> > > > > > > > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > > > > - u8 tb_mask = SR_TB_BIT5;
> > > > > > > > - int pow;
> > > > > > > > + int min_prot_len;
> > > > > > > > + u8 mask = spi_nor_get_bp_mask(nor);
> > > > > > > > + u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > > > > > + u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > > > > > > 
> > > > > > > > - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > > > > > -         tb_mask = SR_TB_BIT6;
> > > > > > > > -
> > > > > > > > - if (!(sr & mask)) {
> > > > > > > > + if (!bp) {
> > > > > > > >                    /* No protection */
> > > > > > > >                    *ofs = 0;
> > > > > > > >                    *len = 0;
> > > > > > > > - } else {
> > > > > > > > -         pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
> > > > > > > > -         *len = mtd->size >> pow;
> > > > > > > > -         if (nor->flags & SNOR_F_HAS_SR_TB && sr &
> > > > > > > > tb_mask)
> > > > > > > > -                 *ofs = 0;
> > > > > > > > -         else
> > > > > > > > -                 *ofs = mtd->size - *len;
> > > > > > > > +         return;
> > > > > > > >            }
> > > > > > > > +
> > > > > > > > + min_prot_len = stm_get_min_prot_length(nor);
> > > > > > > > + *len = min_prot_len << (bp - 1);
> > > > > > > > +
> > > > > > > > + if (*len > mtd->size)
> > > > > > > > +         *len = mtd->size;
> > > > > > > > +
> > > > > > > > + if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
> > > > > > > > +         *ofs = 0;
> > > > > > > > + else
> > > > > > > > +         *ofs = mtd->size - *len;
> > > > > > > >     }
> > > > > > > > 
> > > > > > > >     /*
> > > > > > > > @@ -1880,8 +1915,9 @@ static int stm_lock(struct
> > > > > > > > spi_nor *nor,
> > > > > > > > loff_t ofs, uint64_t len)
> > > > > > > >     {
> > > > > > > >            struct mtd_info *mtd = &nor->mtd;
> > > > > > > >            int ret, status_old, status_new;
> > > > > > > > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > > > > - u8 tb_mask = SR_TB_BIT5;
> > > > > > > > + int min_prot_len;
> > > > > > > > + u8 mask = spi_nor_get_bp_mask(nor);
> > > > > > > > + u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > > > > >            u8 pow, val;
> > > > > > > >            loff_t lock_len;
> > > > > > > >            bool can_be_top = true, can_be_bottom = nor-
> > > > > > > > >flags &
> > > > > > > > SNOR_F_HAS_SR_TB;
> > > > > > > > @@ -1918,20 +1954,14 @@ static int stm_lock(struct
> > > > > > > > spi_nor
> > > > > > > > *nor,
> > > > > > > > loff_t ofs, uint64_t len)
> > > > > > > >            else
> > > > > > > >                    lock_len = ofs + len;
> > > > > > > > 
> > > > > > > > - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > > > > > -         tb_mask = SR_TB_BIT6;
> > > > > > > > + if (lock_len == mtd->size) {
> > > > > > > > +         val = mask; /* fully locked */
> > > > > > > > + } else {
> > > > > > > > +         min_prot_len = stm_get_min_prot_length(nor);
> > > > > > > > +         pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > > > > 1;
> > > > > > > > +         val = pow << SR_BP_SHIFT;
> > > > > > > > + }
> > > > > > > > 
> > > > > > > > - /*
> > > > > > > > -  * Need smallest pow such that:
> > > > > > > > -  *
> > > > > > > > -  *   1 / (2^pow) <= (len / size)
> > > > > > > > -  *
> > > > > > > > -  * so (assuming power-of-2 size) we do:
> > > > > > > > -  *
> > > > > > > > -  *   pow = ceil(log2(size / len)) = log2(size) -
> > > > > > > > floor(log2(len))
> > > > > > > > -  */
> > > > > > > > - pow = ilog2(mtd->size) - ilog2(lock_len);
> > > > > > > > - val = mask - (pow << SR_BP_SHIFT);
> > > > > > > >            if (val & ~mask)
> > > > > > > >                    return -EINVAL;
> > > > > > > >            /* Don't "lock" with no region! */
> > > > > > > > @@ -1966,8 +1996,9 @@ static int stm_unlock(struct
> > > > > > > > spi_nor
> > > > > > > > *nor,
> > > > > > > > loff_t ofs, uint64_t len)
> > > > > > > >     {
> > > > > > > >            struct mtd_info *mtd = &nor->mtd;
> > > > > > > >            int ret, status_old, status_new;
> > > > > > > > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > > > > - u8 tb_mask = SR_TB_BIT5;
> > > > > > > > + int min_prot_len;
> > > > > > > > + u8 mask = spi_nor_get_bp_mask(nor);
> > > > > > > > + u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > > > > >            u8 pow, val;
> > > > > > > >            loff_t lock_len;
> > > > > > > >            bool can_be_top = true, can_be_bottom = nor-
> > > > > > > > >flags &
> > > > > > > > SNOR_F_HAS_SR_TB;
> > > > > > > > @@ -2004,22 +2035,13 @@ static int stm_unlock(struct
> > > > > > > > spi_nor
> > > > > > > > *nor,
> > > > > > > > loff_t ofs, uint64_t len)
> > > > > > > >            else
> > > > > > > >                    lock_len = ofs;
> > > > > > > > 
> > > > > > > > - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > > > > > -         tb_mask = SR_TB_BIT6;
> > > > > > > > - /*
> > > > > > > > -  * Need largest pow such that:
> > > > > > > > -  *
> > > > > > > > -  *   1 / (2^pow) >= (len / size)
> > > > > > > > -  *
> > > > > > > > -  * so (assuming power-of-2 size) we do:
> > > > > > > > -  *
> > > > > > > > -  *   pow = floor(log2(size / len)) = log2(size) -
> > > > > > > > ceil(log2(len))
> > > > > > > > -  */
> > > > > > > > - pow = ilog2(mtd->size) - order_base_2(lock_len);
> > > > > > > >            if (lock_len == 0) {
> > > > > > > >                    val = 0; /* fully unlocked */
> > > > > > > >            } else {
> > > > > > > > -         val = mask - (pow << SR_BP_SHIFT);
> > > > > > > > +         min_prot_len = stm_get_min_prot_length(nor);
> > > > > > > > +         pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > > > > 1;
> > > > > > > > +         val = pow << SR_BP_SHIFT;
> > > > > > > > +
> > > > > > > >                    /* Some power-of-two sizes are not
> > > > > > > > supported */
> > > > > > > >                    if (val & ~mask)
> > > > > > > >                            return -EINVAL;
> > > > > > 
> > > > > > .
> > > > > > 
> > > > 
> > > > .
> > > > 
> > 
> > .
> > 
> 
> 
> 


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

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

[Tudor.Ambarus]

Hi, Jungseung,

I'm going to respin your patches on top on the what will be the manufacturer 
move v2 series, https://patchwork.ozlabs.org/cover/1247794/, I want both 
included for next.

On Wednesday, March 4, 2020 1:07:58 PM EET Jungseung Lee wrote:
> The current mainline locking was restricted and could only be applied
> to flashes that has 3 block protection bit and fixed locking ratio.
> 
> A new method of normalization was reached at the end of the discussion [1].
> 
>     (1) - if bp slot is insufficient.
>     (2) - if bp slot is sufficient.
> 
>     if (bp_slots_needed > bp_slots)    // (1)
>         min_prot_length = sector_size << (bp_slots_needed - bp_slots);
>     else                               // (2)
>         min_prot_length = sector_size;
> 
> This patch changes block protection handling logic based on min_prot_length.
> It is suitable for the overall flashes with exception of some corner case

What corner case, do you refer to EON? Are you aware of other corner cases? We 
should be more precise, for easier review and understanding.

Cheers,
ta

> 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




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

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

[Jungseung Lee]

Hi, Tudor,

On Sat, Mar 14, 2020 at 12:22 AM <Tudor.Ambarus@microchip.com> wrote:
>
> Hi, Jungseung,
>
> I'm going to respin your patches on top on the what will be the manufacturer
> move v2 series, https://patchwork.ozlabs.org/cover/1247794/, I want both
> included for next.
>
> On Wednesday, March 4, 2020 1:07:58 PM EET Jungseung Lee wrote:
> > The current mainline locking was restricted and could only be applied
> > to flashes that has 3 block protection bit and fixed locking ratio.
> >
> > A new method of normalization was reached at the end of the discussion [1].
> >
> >     (1) - if bp slot is insufficient.
> >     (2) - if bp slot is sufficient.
> >
> >     if (bp_slots_needed > bp_slots)    // (1)
> >         min_prot_length = sector_size << (bp_slots_needed - bp_slots);
> >     else                               // (2)
> >         min_prot_length = sector_size;
> >
> > This patch changes block protection handling logic based on min_prot_length.
> > It is suitable for the overall flashes with exception of some corner case
>
> What corner case, do you refer to EON? Are you aware of other corner cases? We
> should be more precise, for easier review and understanding.
>

Yes, that is eon. eon is the only corner case I've ever seen in 3 bit
and 4 bit block protection.

In 2 bit block protection case, a significant number of flash fully
use available bp_slots (2 bit = 4) regardless of the above rule.
That is the case with microchip, catalyst and more..

Of course, none of the 2 bit block protection flash has been set to
lockable so far.

Thanks,

> Cheers,
> ta
>
> > 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
>
>
>

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

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

[Jungseung Lee]

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

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

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

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


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

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

[Jungseung Lee]

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

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e3da6a8654a8..214f3b733e9b 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,6 +238,14 @@ struct flash_info {
 					 * status register. Must be used with
 					 * SPI_NOR_HAS_TB.
 					 */
+#define SPI_NOR_HAS_BP3		BIT(17)	/*
+					 * Flash SR has 4 bit fields (BP0-3)
+					 * for block protection.
+					 */
+#define SPI_NOR_BP3_SR_BIT6	BIT(18)	/*
+					 * BP3 is bit 6 of status register.
+					 * Must be used with SPI_NOR_HAS_BP3.
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -1766,24 +1774,48 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 {
 	struct mtd_info *mtd = &nor->mtd;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
-	u8 tb_mask = SR_TB_BIT5;
-	int pow;
+	u8 tb_mask = SR_TB_BIT5, bp;
+	int pow = 0;
 
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
 
-	if (!(sr & mask)) {
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		u8 tmp;
+
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			tmp = sr & (mask | SR_BP3_BIT6);
+		else
+			tmp = sr & (mask | SR_BP3_BIT5);
+
+		if (tmp & SR_BP3_BIT6)
+			tmp = (tmp & ~BIT(6)) | BIT(5);
+
+		bp = tmp >> SR_BP_SHIFT;
+	} else {
+		bp = (sr & mask) >> SR_BP_SHIFT;
+	}
+
+	if (!bp) {
 		/* No protection */
 		*ofs = 0;
 		*len = 0;
+		return;
+	}
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		if (bp <= ilog2(nor->n_sectors))
+			pow = ilog2(nor->n_sectors) + 1 - bp;
 	} else {
-		pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
-		*len = mtd->size >> pow;
-		if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
-			*ofs = 0;
-		else
-			*ofs = mtd->size - *len;
+		pow = bp ^ (mask >> SR_BP_SHIFT);
 	}
+
+	*len = mtd->size >> pow;
+
+	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+		*ofs = 0;
+	else
+		*ofs = mtd->size - *len;
 }
 
 /*
@@ -1898,6 +1930,12 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
 
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			mask = mask | SR_BP3_BIT6;
+		else
+			mask = mask | SR_BP3_BIT5;
+	}
 	/*
 	 * Need smallest pow such that:
 	 *
@@ -1908,7 +1946,17 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
 	 */
 	pow = ilog2(mtd->size) - ilog2(lock_len);
-	val = mask - (pow << SR_BP_SHIFT);
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		val = ilog2(nor->n_sectors) + 1 - pow;
+		val = val << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
+	} else {
+		val = mask - (pow << SR_BP_SHIFT);
+	}
+
 	if (val & ~mask)
 		return -EINVAL;
 	/* Don't "lock" with no region! */
@@ -1983,6 +2031,13 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
 		tb_mask = SR_TB_BIT6;
+
+	if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+			mask = mask | SR_BP3_BIT6;
+		else
+			mask = mask | SR_BP3_BIT5;
+	}
 	/*
 	 * Need largest pow such that:
 	 *
@@ -1995,6 +2050,14 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	pow = ilog2(mtd->size) - order_base_2(lock_len);
 	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
+	} else if (nor->flags & SNOR_F_HAS_SR_BP3) {
+		val = ilog2(nor->n_sectors) - pow + 1;
+		val = val << SR_BP_SHIFT;
+
+		if (val & BIT(5) && mask & SR_BP3_BIT6)
+			val = (val & ~BIT(5)) | BIT(6);
+		if (val & ~mask)
+			return -EINVAL;
 	} else {
 		val = mask - (pow << SR_BP_SHIFT);
 		/* Some power-of-two sizes are not supported */
@@ -4736,6 +4799,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
 	/* Set SPI NOR sizes. */
 	params->size = (u64)info->sector_size * info->n_sectors;
 	params->page_size = info->page_size;
+	params->n_sectors = info->n_sectors;
 
 	if (!(info->flags & SPI_NOR_NO_FR)) {
 		/* Default to Fast Read for DT and non-DT platform devices. */
@@ -5192,6 +5256,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
 	if (info->flags & USE_CLSR)
 		nor->flags |= SNOR_F_USE_CLSR;
+	if (info->flags & SPI_NOR_HAS_BP3) {
+		nor->flags |= SNOR_F_HAS_SR_BP3;
+		if (info->flags & SPI_NOR_BP3_SR_BIT6)
+			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
+	}
 
 	if (info->flags & SPI_NOR_NO_ERASE)
 		mtd->flags |= MTD_NO_ERASE;
@@ -5199,6 +5268,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	mtd->dev.parent = dev;
 	nor->page_size = params->page_size;
 	mtd->writebufsize = nor->page_size;
+	nor->n_sectors = params->n_sectors;
 
 	if (of_property_read_bool(np, "broken-flash-reset"))
 		nor->flags |= SNOR_F_BROKEN_RESET;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 541c06d042e8..92d550501daf 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -129,7 +129,9 @@
 #define SR_BP1			BIT(3)	/* Block protect 1 */
 #define SR_BP2			BIT(4)	/* Block protect 2 */
 #define SR_TB_BIT5		BIT(5)	/* Top/Bottom protect */
+#define SR_BP3_BIT5		BIT(5)	/* Block protect 3 */
 #define SR_TB_BIT6		BIT(6)	/* Top/Bottom protect */
+#define SR_BP3_BIT6		BIT(6)	/* Block protect 3 */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 /* Spansion/Cypress specific status bits */
 #define SR_E_ERR		BIT(5)
@@ -248,6 +250,8 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_16BIT_SR	= BIT(9),
 	SNOR_F_NO_READ_CR	= BIT(10),
 	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
+	SNOR_F_HAS_SR_BP3	= BIT(12),
+	SNOR_F_HAS_SR_BP3_BIT6	= BIT(13),
 
 };
 
@@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
  *
  * @size:		the flash memory density in bytes.
  * @page_size:		the page size of the SPI NOR flash memory.
+ * @n_sectors:		number of sectors
  * @hwcaps:		describes the read and page program hardware
  *			capabilities.
  * @reads:		read capabilities ordered by priority: the higher index
@@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
 struct spi_nor_flash_parameter {
 	u64				size;
 	u32				page_size;
+	u16				n_sectors;
 
 	struct spi_nor_hwcaps		hwcaps;
 	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
@@ -573,6 +579,7 @@ struct flash_info;
  * @bouncebuf_size:	size of the bounce buffer
  * @info:		spi-nor part JDEC MFR id and other info
  * @page_size:		the page size of the SPI NOR
+ * @n_sector:		number of sectors
  * @addr_width:		number of address bytes
  * @erase_opcode:	the opcode for erasing a sector
  * @read_opcode:	the read opcode
@@ -599,6 +606,7 @@ struct spi_nor {
 	size_t			bouncebuf_size;
 	const struct flash_info	*info;
 	u32			page_size;
+	u16			n_sectors;
 	u8			addr_width;
 	u8			erase_opcode;
 	u8			read_opcode;
-- 
2.17.1


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