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

[Jungseung Lee <js07.lee@samsung.com>]

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/