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

[Michael Walle <michael@walle.cc>]

Am 2020-09-30 12:35, schrieb Vignesh Raghavendra:
[..]
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> changes since v2:
>>  - add Kconfig option to be able to retain legacy behaviour
>>  - rebased the patch due to the spi-nor rewrite
>>  - dropped the Fixes: tag, it doens't make sense after the spi-nor 
>> rewrite
>>  - mention commit 3e0930f109e76 which further modified the unlock
>>    behaviour.
>> 
>> changes since v1:
>>  - completely rewrote patch, the first version used a device tree flag
>> 
>>  drivers/mtd/spi-nor/Kconfig | 35 +++++++++++++++++++++++++++++
>>  drivers/mtd/spi-nor/atmel.c | 24 +++++++++++++-------
>>  drivers/mtd/spi-nor/core.c  | 44 
>> ++++++++++++++++++++++++++++---------
>>  drivers/mtd/spi-nor/core.h  |  6 +++++
>>  drivers/mtd/spi-nor/esmt.c  |  6 ++---
>>  drivers/mtd/spi-nor/intel.c |  6 ++---
>>  drivers/mtd/spi-nor/sst.c   | 21 +++++++++---------
>>  include/linux/mtd/spi-nor.h |  6 +++++
>>  8 files changed, 114 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 6e816eafb312..647de17c81e2 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -24,6 +24,41 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>>  	  Please note that some tools/drivers/filesystems may not work with
>>  	  4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>> 
>> +choice
>> +	prompt "Write protection at boot"
>> +	default MTD_SPI_NOR_WP_DISABLE
> 
> These choice control how BP0-X bits are manipulated on boot. Hence, to
> be consistent should use Block Protection (BP) terminology throughout.
> 
> This would also be inline with most flash datasheets which also use 
> term BP

Where should I mention the BP bits? In the choice or in the help text.
I tried to keep the choice prompt as easy to understand as possible, so 
an
user who doesn't know anything about how the write protection actually 
works
can still make that choice (without first reading a datasheet). Also 
keep in
mind, that there is also the per sector locking. Wouldn't this option 
also
apply to that?

Therefore, I'd mention in the help text, that (currently) the BP bits 
are
affected.

>> +
>> +config MTD_SPI_NOR_WP_DISABLE
>> +	bool "Disable WP on any flashes (legacy behaviour)"
>> +	help
>> +	  This option disables the write protection on any SPI flashes at
>> +	  boot-up.
>> +
>> +	  Don't use this if you intent to use the write protection of your
>> +	  SPI flash. This is only to keep backwards compatibility.
>> +
>> +config MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE
>> +	bool "Disable WP on flashes w/ volatile protection bits"
>> +	help
>> +	  Some SPI flashes have volatile block protection bits, ie. after a
>> +	  power-up or a reset the flash is write protected by default.
>> +
>> +	  This option disables the write protection for these kind of 
>> flashes
>> +	  while keeping it enabled for any other SPI flashes which have
>> +	  non-volatile block protection bits.
>> +
>> +	  If you are unsure, select this option.
>> +
>> +config MTD_SPI_NOR_WP_KEEP
>> +	bool "Keep write protection as is"
>> +	help
>> +	  If you select this option the write protection of any SPI flashes
>> +	  will not be changed. If your flash is write protected or will be
>> +	  automatically write protected after power-up you have to manually
>> +	  unlock it before you are able to write to it.
>> +
>> +endchoice
>> +
>>  source "drivers/mtd/spi-nor/controllers/Kconfig"
>> 
>>  endif # MTD_SPI_NOR
> 
> [...]
> 
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 6f2f6b27173f..9a33c023717f 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
>>  	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
>>  	SNOR_F_HAS_4BIT_BP      = BIT(12),
>>  	SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
>> +	SNOR_F_NEED_UNPROTECT	= BIT(14),
>>  };
>> 
>>  struct spi_nor_read_command {
>> @@ -311,6 +312,11 @@ struct flash_info {
>>  					 * BP3 is bit 6 of status register.
>>  					 * Must be used with SPI_NOR_4BIT_BP.
>>  					 */
>> +#define SPI_NOR_UNPROTECT	BIT(19)	/*
>> +					 * Flash is write-protected after
>> +					 * power-up and needs a global
>> +					 * unprotect.
>> +					 */
>> 
> 
> It would be better to name the flag to indicate BP bits are volatile or
> powers up locked instead of SPI_NOR_UNPROTECT. This makes it easier to
> understand what this flag means wrt flash HW feature. Maybe:
> 
> SPI_NOR_LOCKED_ON_POWER_UP or SPI_NOR_BP_IS_VOLATILE

SPI_NOR_BP_IS_VOLATILE sounds good to me.

-michael