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

[Michael Walle <michael@walle.cc>]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/