Date:   Tue, 18 Jun 2019 09:54:55 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     Sagar Shrikant Kadam <sagar.kadam@...ive.com>,
        <marek.vasut@...il.com>, <tudor.ambarus@...rochip.com>,
        <dwmw2@...radead.org>, <computersforpeace@...il.com>,
        <miquel.raynal@...tlin.com>, <richard@....at>,
        <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-riscv@...ts.infradead.org>
CC:     <palmer@...ive.com>, <aou@...s.berkeley.edu>,
        <paul.walmsley@...ive.com>, <wesley@...ive.com>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH v5 3/3] mtd: spi-nor: add locking support for is25xxxxx
 device

+Uwe who had interest in 4bit block protection support

On 12-Jun-19 4:17 PM, Sagar Shrikant Kadam wrote:
> Implement a locking scheme for ISSI devices based on stm_lock mechanism.
> The is25xxxxx  devices have 4 bits for selecting the range of blocks to
> be locked/protected from erase/write operations and function register
> gives feasibility to select TOP / Bottom area for protection.
> Added opcodes to read and write function registers.
> 
> The current implementation enables block protection as per the table
> defined into datasheet for is25wp256 device having erase size of 0x1000.
> ISSI and stm devices differ in terms of TBS (Top/Bottom area protection)
> bits. In case of issi this is in Function register and is OTP memory, so
> once FR bits are programmed  cannot be modified.
> 

I am not a fan of modifying/setting OTP bits are they are irreversible
and change the expectation of other SWs in the system such as
bootloader. See comments further down the patch....

> Some common code from stm_lock/unlock implementation is extracted so that
> it can be re-used for issi devices. The locking scheme has been tested on
> HiFive Unleashed board, having is25wp256 flash memory.
> 

Have you tested lock/unlock on non ISSI device with this series?

> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@...ive.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 291 ++++++++++++++++++++++++++++++++++--------
>  include/linux/mtd/spi-nor.h   |   5 +
>  2 files changed, 245 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index b7c6261..9281ec0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -288,6 +288,45 @@ struct flash_info {
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
>  
> +/**
> + * read_fr() -read function register
> + * @nor: pointer to a 'struct spi_nor'.
> + *
> + * ISSI devices have top/bottom area protection bits selection into function
> + * reg.The bits in FR are OTP.So once it's written, it cannot be changed.
> + *
> + * Return: Value in function register or Negative if error.
> + */
> +static int read_fr(struct spi_nor *nor)

Please prefix spi_nor_ (spi_nor_read_fr()) to all generic functions that
you are adding in this patch

> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = nor->read_reg(nor, SPINOR_OP_RDFR, &val, 1);
> +	if (ret < 0) {
> +		pr_err("error %d reading FR\n", (int) ret);

dev_err() and no need to cast 'ret' to int

> +		return ret;
> +	}
> +
> +	return val;
> +}
> +
> +/**
> + * write_fr() -Write function register
> + * @nor: pointer to a 'struct spi_nor'.
> + *
> + * ISSI devices have top/bottom area selection protection bits into function
> + * reg whereas other devices have the TBS bit into Status Register.
s/into/in

> + * The bits in FR are OTP.So once it's written, it cannot be changed.
> + *
> + * Return: Negative if error
> + */
> +static int write_fr(struct spi_nor *nor, u8 val)
> +{
> +	nor->cmd_buf[0] = val;
> +	return nor->write_reg(nor, SPINOR_OP_WRFR, nor->cmd_buf, 1);
> +}
> +
>  /*
>   * Read the status register, returning its value in the location
>   * Return the status register value.
> @@ -1088,10 +1127,17 @@ 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;
> -	int shift = ffs(mask) - 1;
> +	u8 mask = 0;
> +	int shift = 0;
>  	int pow;
>  
> +	if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI)
> +		mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;

Does all ISSI flashes support SR_BP3?

Irrespective of that this isn't generic enough. There are non ISSI
flashes with BP3. Please add a flag or field to flash_info struct to
identify flashes with BP3 bit and then use combination of the flag and
MFR ID to select suitable lock/unlock mechanism


> +	else
> +		mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
> +	shift = ffs(mask) - 1;
> +
>  	if (!(sr & mask)) {
>  		/* No protection */
>  		*ofs = 0;
> @@ -1099,10 +1145,19 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
>  	} else {
>  		pow = ((sr & mask) ^ mask) >> shift;
>  		*len = mtd->size >> pow;
> -		if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
> -			*ofs = 0;
> -		else
> -			*ofs = mtd->size - *len;
> +
> +		if (JEDEC_MFR(nor->info) == SNOR_MFR_ISSI) {
> +			if (nor->flags & SNOR_F_HAS_SR_TB &&
> +					(read_fsr(nor) & FR_TB))
> +				*ofs = 0;
> +			else
> +				*ofs = mtd->size - *len;
> +		} else {
> +			if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
> +				*ofs = 0;
> +			else
> +				*ofs = mtd->size - *len;
> +		}
>  	}
>  }
>  
> @@ -1129,18 +1184,108 @@ static int stm_check_lock_status_sr(struct spi_nor *nor, loff_t ofs, uint64_t le
>  		return (ofs >= lock_offs + lock_len) || (ofs + len <= lock_offs);
>  }
>  
> -static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> +/*
> + * check if memory region is locked
> + *
> + * Returns false if region is locked 0 otherwise.
> + */
> +static int fl_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>  			    u8 sr)
>  {
>  	return stm_check_lock_status_sr(nor, ofs, len, sr, true);
>  }
>  
> -static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> +/*
> + * check if memory region is unlocked
> + *
> + * Returns false if region is locked 0 otherwise.
> + */
> +static int fl_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
>  			      u8 sr)
>  {
>  	return stm_check_lock_status_sr(nor, ofs, len, sr, false);
>  }
>  
> +/**
> + * flash_select_zone() - Select TOP area or bottom area to lock/unlock
> + * @nor: pointer to a 'struct spi_nor'.
> + * @ofs: offset from which to lock memory.
> + * @len: number of bytes to unlock.
> + * @sr: status register
> + * @tb: pointer to top/bottom bool used in caller function
> + * @op: zone selection is for lock/unlock operation. 1: lock 0:unlock
> + *
> + * Select the top area / bottom area paattern to protect memory blocks.

s/paattern/pattern

> + *
> + * Returns negative on errors, 0 on success.
> + */
> +static int fl_select_zone(struct spi_nor *nor, loff_t ofs, uint64_t len,
> +				u8 sr, bool *tb, bool op)
> +{
> +	int retval;
> +	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
> +
> +	if (op) {
> +		/* Select for lock zone operation */
> +
> +		/*
> +		 * If nothing in our range is unlocked, we don't need
> +		 * to do anything.
> +		 */
> +		if (fl_is_locked_sr(nor, ofs, len, sr))
> +			return 0;
> +
> +		/*
> +		 * If anything below us is unlocked, we can't use 'bottom'
> +		 * protection.
> +		 */
> +		if (!fl_is_locked_sr(nor, 0, ofs, sr))
> +			can_be_bottom = false;
> +
> +		/*
> +		 * If anything above us is unlocked, we can't use 'top'
> +		 * protection.
> +		 */
> +		if (!fl_is_locked_sr(nor, ofs + len,
> +					nor->mtd.size - (ofs + len), sr))
> +			can_be_top = false;
> +	} else {
> +		/* Select unlock zone */
> +
> +		/*
> +		 * If nothing in our range is locked, we don't need to
> +		 * do anything.
> +		 */
> +		if (fl_is_unlocked_sr(nor, ofs, len, sr))
> +			return 0;
> +
> +		/*
> +		 * If anything below us is locked, we can't use 'top'
> +		 * protection
> +		 */
> +		if (!fl_is_unlocked_sr(nor, 0, ofs, sr))
> +			can_be_top = false;
> +
> +		/*
> +		 * If anything above us is locked, we can't use 'bottom'
> +		 * protection
> +		 */
> +		if (!fl_is_unlocked_sr(nor, ofs + len,
> +					nor->mtd.size - (ofs + len), sr))
> +			can_be_bottom = false;
> +	}
> +
> +	if (!can_be_bottom && !can_be_top)
> +		retval = -EINVAL;
> +	else {
> +		/* Prefer top, if both are valid */
> +		*tb = can_be_top;
> +		retval = 1;
> +	}
> +
> +	return retval;
> +}
> +
>  /*
>   * 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
> @@ -1178,33 +1323,19 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	int status_old, status_new;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 shift = ffs(mask) - 1, pow, val;
> +	u8 shift = ffs(mask) - 1, pow, val, ret;
>  	loff_t lock_len;
> -	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
>  	bool use_top;
>  
>  	status_old = read_sr(nor);
>  	if (status_old < 0)
>  		return status_old;
>  
> -	/* If nothing in our range is unlocked, we don't need to do anything */
> -	if (stm_is_locked_sr(nor, ofs, len, status_old))
> +	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
> +	if (!ret)
>  		return 0;
> -
> -	/* If anything below us is unlocked, we can't use 'bottom' protection */
> -	if (!stm_is_locked_sr(nor, 0, ofs, status_old))
> -		can_be_bottom = false;
> -
> -	/* If anything above us is unlocked, we can't use 'top' protection */
> -	if (!stm_is_locked_sr(nor, ofs + len, mtd->size - (ofs + len),
> -				status_old))
> -		can_be_top = false;
> -
> -	if (!can_be_bottom && !can_be_top)
> -		return -EINVAL;
> -
> -	/* Prefer top, if both are valid */
> -	use_top = can_be_top;
> +	else if (ret < 0)
> +		return ret;
>  
>  	/* lock_len: length of region that should end up locked */
>  	if (use_top)
> @@ -1258,35 +1389,21 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	struct mtd_info *mtd = &nor->mtd;
>  	int status_old, status_new;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 shift = ffs(mask) - 1, pow, val;
> +	u8 shift = ffs(mask) - 1, pow, val, ret;
>  	loff_t lock_len;
> -	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
>  	bool use_top;
>  
>  	status_old = read_sr(nor);
>  	if (status_old < 0)
>  		return status_old;
>  
> -	/* If nothing in our range is locked, we don't need to do anything */
> -	if (stm_is_unlocked_sr(nor, ofs, len, status_old))
> +	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 0);
> +	if (!ret)
>  		return 0;
> +	else if (ret < 0)
> +		return ret;
>  
> -	/* If anything below us is locked, we can't use 'top' protection */
> -	if (!stm_is_unlocked_sr(nor, 0, ofs, status_old))
> -		can_be_top = false;
> -
> -	/* If anything above us is locked, we can't use 'bottom' protection */
> -	if (!stm_is_unlocked_sr(nor, ofs + len, mtd->size - (ofs + len),
> -				status_old))
> -		can_be_bottom = false;
> -
> -	if (!can_be_bottom && !can_be_top)
> -		return -EINVAL;
> -
> -	/* Prefer top, if both are valid */
> -	use_top = can_be_top;
> -
> -	/* lock_len: length of region that should remain locked */
> +	/* lock_len: length of region that should end up locked */
>  	if (use_top)
>  		lock_len = mtd->size - (ofs + len);
>  	else
> @@ -1338,7 +1455,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>   * Returns 1 if entire region is locked, 0 if any portion is unlocked, and
>   * negative on errors.
>   */
> -static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +static int fl_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  {
>  	int status;
>  
> @@ -1346,7 +1463,7 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	if (status < 0)
>  		return status;
>  
> -	return stm_is_locked_sr(nor, ofs, len, status);
> +	return fl_is_locked_sr(nor, ofs, len, status);
>  }
>  
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> @@ -1461,6 +1578,77 @@ static int macronix_quad_enable(struct spi_nor *nor)
>  }
>  
>  /**
> + * issi_lock() - set BP[0123] write-protection.
> + * @nor: pointer to a 'struct spi_nor'.
> + * @ofs: offset from which to lock memory.
> + * @len: number of bytes to unlock.
> + *
> + * Lock a region of the flash.Implementation is based on stm_lock
> + * Supports the block protection bits BP{0,1,2,3} in the status register
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	int status_old, status_new, blk_prot;
> +	u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 shift = ffs(mask) - 1;
> +	u8 pow, ret, func_reg;
> +	bool use_top;
> +	loff_t lock_len;
> +
> +	status_old = read_sr(nor);
> +
> +	/* if status reg is Write protected don't update bit protection */
> +	if (status_old & SR_SRWD) {
> +		dev_err(nor->dev,
> +			"SR is Write Protected,can't update BP bits...\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = fl_select_zone(nor, ofs, len, status_old, &use_top, 1);
> +	if (!ret)
> +		/* Older protected blocks include the new requested block's */
> +		return 0;
> +	else if (ret < 0)
> +		return ret;
> +
> +	func_reg = read_fr(nor);
> +	/* lock_len: length of region that should end up locked */
> +	if (use_top) {
> +		/* Update Function register to use TOP area */
> +		if ((func_reg >> 1) & 0x1) {
> +			/* Currently bootom selected change to top */
> +			func_reg ^= FR_TB;
> +			write_fr(nor, func_reg);
> +		}

IIUC, since this FR_TB OTP bit is initially 0 and now reads 1, implies
that OTP bit has already been programmed once. So is clearing the bit
possible?

I think this lock/unlock mechanism needs a bit more thought.
One solution would be to not modify OTP bit and return error in all
cases when locking a region requested by user is not possible (for a
default scheme).

Regards
Vignesh

> +		lock_len = nor->mtd.size - ofs;
> +	} else {
> +
> +		/* Update Function register to use bottom area */
> +		if (!((func_reg >> 1) & 0x1)) {
> +			/*Currently top is selected, change to bottom */
> +			func_reg ^= FR_TB;
> +			write_fr(nor, func_reg);
> +		}
> +		lock_len = ofs + len;
> +	}
> +
> +	pow = order_base_2(lock_len);
> +	blk_prot = mask & (((pow+1) & 0xf)<<shift);
> +	if (lock_len <= 0) {
> +		dev_err(nor->dev, "invalid Length to protect");
> +		return -EINVAL;
> +	}
> +
> +	status_new = status_old | blk_prot;
> +	if (status_old == status_new)
> +		return 0;
> +
> +	return write_sr_and_check(nor, status_new, mask);
> +}
> +
> +/**
>   * issi_unlock() - clear BP[0123] write-protection.
>   * @nor: pointer to a 'struct spi_nor'.
>   * @ofs: offset from which to unlock memory.
> @@ -1879,7 +2067,7 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
>  			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 1024,
>  			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -			SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK)
> +			SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>  	},
>  
>  	/* Macronix */
> @@ -4120,12 +4308,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	    info->flags & SPI_NOR_HAS_LOCK) {
>  		nor->flash_lock = stm_lock;
>  		nor->flash_unlock = stm_unlock;
> -		nor->flash_is_locked = stm_is_locked;
> +		nor->flash_is_locked = fl_is_locked;
>  	}
>  
>  	/* NOR protection support for ISSI chips */
>  	if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
>  	    info->flags & SPI_NOR_HAS_LOCK) {
> +		nor->flash_lock = issi_lock;
>  		nor->flash_unlock = issi_unlock;
>  
>  	}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 9a7d719..a15d012 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -40,6 +40,8 @@
>  #define SPINOR_OP_RDSR		0x05	/* Read status register */
>  #define SPINOR_OP_WRSR		0x01	/* Write status register 1 byte */
>  #define SPINOR_OP_RDSR2		0x3f	/* Read status register 2 */
> +#define SPINOR_OP_RDFR		0x48	/* Read Function register */
> +#define SPINOR_OP_WRFR		0x42	/* Write Function register 1 byte */
>  #define SPINOR_OP_WRSR2		0x3e	/* Write status register 2 */
>  #define SPINOR_OP_READ		0x03	/* Read data bytes (low frequency) */
>  #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
> @@ -139,6 +141,9 @@
>  /* Enhanced Volatile Configuration Register bits */
>  #define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
>  
> +/*Function register bit */
> +#define FR_TB			BIT(1)	/*ISSI: Top/Bottom protect */
> +
>  /* Flag Status Register bits */
>  #define FSR_READY		BIT(7)	/* Device status, 0 = Busy, 1 = Ready */
>  #define FSR_E_ERR		BIT(5)	/* Erase operation status */
> 

Powered by blists - more mailing lists