Re: [PATCH v7 4/4] mtd: spi-nor: add locking support for is25wp256 device

[Vignesh Raghavendra]

Hi Sagar,

On 03/07/19 12:09 AM, Sagar Shrikant Kadam wrote:
[...]> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	int status_old, status_new, blk_prot;
> +	u8 mask;
> +	u8 shift;
> +	u8 pow, ret, func_reg;
> +	bool use_top;
> +	loff_t lock_len;
> +
> +	if (nor->flags & SNOR_F_HAS_BP3)
> +		mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> +	else
> +		mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
> +	shift = ffs(mask) - 1;
> +
> +	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 = spi_nor_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 = spi_nor_read_fr(nor);

Sorry, I don't understand where func_reg is used? Since TBS OTP, how
does issi_lock() code comprehend that TBS bit value in OTP register and
use_top returned by spi_nor_select_zone() are matching before we go
ahead and program status register.

We should reject locking request if top-bottom calculation does not
match OTP bit. Where is that done?

Regards
Vignesh

> +	/* lock_len: length of region that should end up locked */
> +	if (use_top)
> +		lock_len = nor->mtd.size - ofs;
> +	else
> +		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.
> @@ -4171,6 +4338,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	if (JEDEC_MFR(info) == SNOR_MFR_ISSI &&
>  	    info->flags & SPI_NOR_HAS_LOCK &&
>  	    info->flags & SPI_NOR_HAS_BP3) {
> +		nor->flash_lock = issi_lock;
>  		nor->flash_unlock = issi_unlock;
>  	}
>  
> @@ -4194,6 +4362,8 @@ 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_BP3;
>  
>  	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 f6fa70f..26dbf48 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 */
> 

-- 
Regards
Vignesh