Re: [PATCH v3] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

Hi, Michael,

PLease accept my apologies for the long delay.

I do agree with Vignesh's comments. Few others below.

On 3/27/20 5:59 PM, Michael Walle wrote:

[cut]

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc68ea84318e..fd1c36d70a13 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
> + * spi_nor_global_unprotect() - Perform a global unprotect of the memory area.
>   * @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.
> + * by default. Do it only for flashes where the block protection bits
> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
> + *
> + * We cannot use spi_nor_unlock(nor->params.size) here because there are
> + * legacy devices (eg. AT25DF041A) which need a "global unprotect" command.
> + * This is done by writing 0b0x0000xx to the status register. This will also
> + * work for all other flashes which have these bits mapped to BP0 to BP3.
> + * The top most bit is ususally some kind of lock bit for the block
> + * protection bits.
>   */
> -static int spi_nor_unlock_all(struct spi_nor *nor)
> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>  {
> -	if (nor->flags & SNOR_F_HAS_LOCK)
> -		return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
> +	int ret;
>  
> -	return 0;
> +	dev_dbg(nor->dev, "unprotecting entire flash\n");
> +	ret = spi_nor_read_sr(nor, nor->bouncebuf);
> +	if (ret)
> +		return ret;
> +
> +	nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
> +
> +	/*
> +	 * Don't use spi_nor_write_sr1_and_check() because writing the status
> +	 * register might fail if the flash is hardware write protected.
> +	 */
> +	return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>  }

This won't work for all the flashes. You use a GENMASK(5, 2) to clear
the Status Register even for BP0-2 flashes and you end up clearing BIT(5)
which can lead to side effects.

We should instead introduce a nor->params->locking_ops->global_unlock() hook
for the flashes that have special opcodes that unlock all the flash blocks,
or for the flashes that deviate from the "clear just your BP bits" rule.

you can keep the call to spi_nor_unlock(&nor->mtd, 0, nor->params->size);
and in spi_nor_unlock() do:

if (len == nor->params->size && nor->params->locking_ops->global_unlock)
	ret = nor->params->locking_ops->global_unlock(nor)
else
	ret = nor->params->locking_ops->unlock(nor, ofs, len);

Cheers,
ta