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

[Michael Walle <michael@walle.cc>]

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/