Re: [PATCH v3 3/3] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Pratyush Yadav <p.yadav@ti.com>]

On 20/05/21 05:58PM, Michael Walle wrote:
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
[...]
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index ec0c1b33f7cc..2dc315b6bffc 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -111,6 +111,34 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>  	return ret ?: written;
>  }
>  
> +/**
> + * spi_nor_otp_erase_secr() - erase a security register
> + * @nor:        pointer to 'struct spi_nor'
> + * @addr:       offset of the security register to be erased
> + *
> + * Erase a security register by using the SPINOR_OP_ESECR command. This method
> + * is used on GigaDevice and Winbond flashes to erase OTP data.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
> +{
> +	u8 erase_opcode = nor->erase_opcode;
> +	int ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	nor->erase_opcode = SPINOR_OP_ESECR;
> +	ret = spi_nor_erase_sector(nor, addr);
> +	nor->erase_opcode = erase_opcode;
> +	if (ret)
> +		return ret;
> +
> +	return spi_nor_wait_till_ready(nor);

The datasheet for W25Q32JW says in Section 8.2.29:
  
  The Security Register Lock Bits (LB3-1) in the Status Register-2 can 
  be used to OTP protect the security registers. Once a lock bit is set 
  to 1, the corresponding security register will be permanently locked,  
  Erase Security Register instruction to that register will be ignored

So if the region is locked, the flash will happily accept the erase and 
simply do nothing. So will the program. So when the OTP region is locked 
and someone does an erase-program cycle, they will think their data went 
through even though it was simply thrown away by the flash.

I think you should check that bit before doing these operations to make 
sure it is actually allowed. If it isn't, return an error code (-EPERM 
maybe).

> +}
> +
>  static int spi_nor_otp_lock_bit_cr(unsigned int region)
>  {
>  	static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
[...]

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.