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

[Michael Walle]

Hi Tudor,

> > 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.

Wouldn't it make more sense to just set params->locking_ops for these 
flashes
to different functions? or even provide a spi_nor_global_unprotect_ops 
in
core.c and these flashes will just set them. there is no individual 
sector
range lock for these chips. just a lock all or nothing.

If it is more common and not just one vendor it might also make sense to 
add
a seperate flag which will then set the locking ops to
spi_nor_global_unprotect_ops, also it seems that there have to be two 
writes
to the status register, one to clear SPRL and then one to clear the BP 
bits.
See for example [1] Table 9-2. I'll have to go through the datasheets 
again
to see what flashes just have a global (un)protect.

[1] https://www.adestotech.com/wp-content/uploads/doc3668.pdf

-michael