On 3/27/20 9:29 PM, Michael Walle wrote:
> Traditionally, linux unlocks the whole flash because there are legacy
> devices which has the write protections bits set by default at startup.
> If you actually want to use the flash protection bits, eg. because there
> is a read-only part for a bootloader, this automatic unlocking is
> harmful. If there is no hardware write protection in place (usually
> called WP#), a startup of the kernel just discards this protection.
>
[...]
> Further, the commit 3e0930f109e76 ("mtd: spi-nor: Rework the disabling of
> block write protection") expanded the unlock_all() feature to ANY flash
> which supports locking.
>
Appreciate the detail commit log.
> Signed-off-by: Michael Walle <mich...@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
> +
> +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
Reset looks fine to me
[...]
Regards
Vignesh
