Re: [PATCH v5 2/6] mtd: spi-nor: Rework the disabling of block write protection

[Vignesh Raghavendra]

On 07-Nov-19 2:11 PM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> 
> spi_nor_unlock() unlocks blocks of memory or the entire flash memory
> array, if requested. clear_sr_bp() unlocks the entire flash memory
> array at boot time. This calls for some unification, clear_sr_bp() is
> just an optimization for the case when the unlock request covers the
> entire flash size.
> 
> Get rid of clear_sr_bp() and introduce spi_nor_unlock_all(), which is
> just a call to spi_nor_unlock() for the entire flash memory array.
> This fixes a bug that was present in spi_nor_spansion_clear_sr_bp().
> When the QE bit was zero, we used the Write Status (01h) command with
> one data byte, which might cleared the Status Register 2. We now always
> use the Write Status (01h) command with two data bytes when
> SNOR_F_HAS_16BIT_SR is set, to avoid clearing the Status Register 2.
> 
> The SNOR_F_NO_READ_CR case is treated as well. When the flash doesn't
> support the CR Read command, we make an assumption about the value of
> the QE bit. In spi_nor_init(), call spi_nor_quad_enable() first, then
> spi_nor_unlock_all(), so that at the spi_nor_unlock_all() time we can
> be sure the QE bit has value one, because of the previous call to
> spi_nor_quad_enable().
> 
> Get rid of the MFR handling and implement specific manufacturer
> default_init() fixup hooks.
> 
> Note that this changes a bit the logic for the SNOR_MFR_ATMEL,
> SNOR_MFR_INTEL and SNOR_MFR_SST cases. Before this patch, the Atmel,
> Intel and SST chips did not set the locking ops, but unlocked the entire
> flash at boot time, while now they are setting the locking ops to
> stm_locking_ops. This should work, since the the disable of the block

Nit: s/the the/the

> protection at the boot time used the same Status Register bits to unlock
> the flash, as in the stm_locking_ops case.
> 
> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> ---

Reviewed-by: Vignesh Raghavendra <vigneshr@xxxxxx>

Regards
Vignesh

>  drivers/mtd/spi-nor/spi-nor.c | 140 +++++++++++++++---------------------------
>  include/linux/mtd/spi-nor.h   |   3 -
>  2 files changed, 50 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d696334f25f0..06aac894ee6d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2185,74 +2185,6 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
>  	return 0;
>  }
>  
> -/**
> - * spi_nor_clear_sr_bp() - clear the Status Register Block Protection bits.
> - * @nor:        pointer to a 'struct spi_nor'
> - *
> - * Read-modify-write function that clears the Block Protection bits from the
> - * Status Register without affecting other bits.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -static int spi_nor_clear_sr_bp(struct spi_nor *nor)
> -{
> -	int ret;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -
> -	ret = spi_nor_read_sr(nor, nor->bouncebuf);
> -	if (ret)
> -		return ret;
> -
> -	nor->bouncebuf[0] &= ~mask;
> -
> -	return spi_nor_write_sr(nor, nor->bouncebuf, 1);
> -}
> -
> -/**
> - * spi_nor_spansion_clear_sr_bp() - clear the Status Register Block Protection
> - * bits on spansion flashes.
> - * @nor:        pointer to a 'struct spi_nor'
> - *
> - * Read-modify-write function that clears the Block Protection bits from the
> - * Status Register without affecting other bits. The function is tightly
> - * coupled with the spansion_read_cr_quad_enable() function. Both assume that
> - * the Write Register with 16 bits, together with the Read Configuration
> - * Register (35h) instructions are supported.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor)
> -{
> -	int ret;
> -	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> -	u8 *sr_cr =  nor->bouncebuf;
> -
> -	/* Check current Quad Enable bit value. */
> -	ret = spi_nor_read_cr(nor, &sr_cr[1]);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * When the configuration register Quad Enable bit is one, only the
> -	 * Write Status (01h) command with two data bytes may be used.
> -	 */
> -	if (sr_cr[1] & CR_QUAD_EN_SPAN) {
> -		ret = spi_nor_read_sr(nor, sr_cr);
> -		if (ret)
> -			return ret;
> -
> -		sr_cr[0] &= ~mask;
> -
> -		return spi_nor_write_sr(nor, sr_cr, 2);
> -	}
> -
> -	/*
> -	 * If the Quad Enable bit is zero, use the Write Status (01h) command
> -	 * with one data byte.
> -	 */
> -	return spi_nor_clear_sr_bp(nor);
> -}
> -
>  /* Used when the "_ext_id" is two bytes at most */
>  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
>  		.id = {							\
> @@ -4634,12 +4566,27 @@ static int spi_nor_setup(struct spi_nor *nor,
>  	return nor->params.setup(nor, hwcaps);
>  }
>  
> +static void atmel_set_default_init(struct spi_nor *nor)
> +{
> +	nor->flags |= SNOR_F_HAS_LOCK;
> +}
> +
> +static void intel_set_default_init(struct spi_nor *nor)
> +{
> +	nor->flags |= SNOR_F_HAS_LOCK;
> +}
> +
>  static void macronix_set_default_init(struct spi_nor *nor)
>  {
>  	nor->params.quad_enable = macronix_quad_enable;
>  	nor->params.set_4byte = macronix_set_4byte;
>  }
>  
> +static void sst_set_default_init(struct spi_nor *nor)
> +{
> +	nor->flags |= SNOR_F_HAS_LOCK;
> +}
> +
>  static void st_micron_set_default_init(struct spi_nor *nor)
>  {
>  	nor->flags |= SNOR_F_HAS_LOCK;
> @@ -4661,6 +4608,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  {
>  	/* Init flash parameters based on MFR */
>  	switch (JEDEC_MFR(nor->info)) {
> +	case SNOR_MFR_ATMEL:
> +		atmel_set_default_init(nor);
> +		break;
> +
> +	case SNOR_MFR_INTEL:
> +		intel_set_default_init(nor);
> +		break;
> +
>  	case SNOR_MFR_MACRONIX:
>  		macronix_set_default_init(nor);
>  		break;
> @@ -4670,6 +4625,10 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  		st_micron_set_default_init(nor);
>  		break;
>  
> +	case SNOR_MFR_SST:
> +		sst_set_default_init(nor);
> +		break;
> +
>  	case SNOR_MFR_WINBOND:
>  		winbond_set_default_init(nor);
>  		break;
> @@ -4930,21 +4889,26 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
>  	return nor->params.quad_enable(nor);
>  }
>  
> -static int spi_nor_init(struct spi_nor *nor)
> +/**
> + * spi_nor_unlock_all() - Unlocks the entire flash memory array.
> + * @nor:	pointer to a 'struct spi_nor'.
> + *
> + * Some SPI NOR flashes are write protected by default after a power-on reset
> + * cycle, in order to avoid inadvertent writes during power-up. Backward
> + * compatibility imposes to unlock the entire flash memory array at power-up
> + * by default.
> + */
> +static int spi_nor_unlock_all(struct spi_nor *nor)
>  {
> -	int err;
> +	if (nor->flags & SNOR_F_HAS_LOCK)
> +		return spi_nor_unlock(&nor->mtd, 0, nor->params.size);
>  
> -	if (nor->clear_sr_bp) {
> -		if (nor->params.quad_enable == spansion_read_cr_quad_enable)
> -			nor->clear_sr_bp = spi_nor_spansion_clear_sr_bp;
> +	return 0;
> +}
>  
> -		err = nor->clear_sr_bp(nor);
> -		if (err) {
> -			dev_dbg(nor->dev,
> -				"fail to clear block protection bits\n");
> -			return err;
> -		}
> -	}
> +static int spi_nor_init(struct spi_nor *nor)
> +{
> +	int err;
>  
>  	err = spi_nor_quad_enable(nor);
>  	if (err) {
> @@ -4952,6 +4916,12 @@ static int spi_nor_init(struct spi_nor *nor)
>  		return err;
>  	}
>  
> +	err = spi_nor_unlock_all(nor);
> +	if (err) {
> +		dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
> +		return err;
> +	}
> +
>  	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
>  		/*
>  		 * If the RESET# pin isn't hooked up properly, or the system
> @@ -5134,16 +5104,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	if (info->flags & SPI_NOR_HAS_LOCK)
>  		nor->flags |= SNOR_F_HAS_LOCK;
>  
> -	/*
> -	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> -	 * with the software protection bits set.
> -	 */
> -	if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL ||
> -	    JEDEC_MFR(nor->info) == SNOR_MFR_INTEL ||
> -	    JEDEC_MFR(nor->info) == SNOR_MFR_SST ||
> -	    nor->info->flags & SPI_NOR_HAS_LOCK)
> -		nor->clear_sr_bp = spi_nor_clear_sr_bp;
> -
>  	/* Init flash parameters based on flash_info struct and SFDP */
>  	spi_nor_init_params(nor);
>  
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index d6ec55cc6d97..11daecc5a83d 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -581,8 +581,6 @@ struct flash_info;
>   * @write_proto:	the SPI protocol for write operations
>   * @reg_proto		the SPI protocol for read_reg/write_reg/erase operations
>   * @controller_ops:	SPI NOR controller driver specific operations.
> - * @clear_sr_bp:	[FLASH-SPECIFIC] clears the Block Protection Bits from
> - *			the SPI NOR Status Register.
>   * @params:		[FLASH-SPECIFIC] SPI-NOR flash parameters and settings.
>   *                      The structure includes legacy flash parameters and
>   *                      settings that can be overwritten by the spi_nor_fixups
> @@ -611,7 +609,6 @@ struct spi_nor {
>  
>  	const struct spi_nor_controller_ops *controller_ops;
>  
> -	int (*clear_sr_bp)(struct spi_nor *nor);
>  	struct spi_nor_flash_parameter params;
>  
>  	void *priv;
>