Re: [PATCH 2/2] mtd: spi-nor: Avoid setting SRWD bit in SR if WP signal not connected

[Michael Walle]

Am 2023-06-15 13:16, schrieb Amit Kumar Mahapatra:
> Setting the status register write disable (SRWD) bit in the status
> register (SR) with WP signal of the flash not connected will configure 
> the
> SR permanently as read-only. If WP signal is not connected, avoid 
> setting
> SRWD bit while writing the SR during flash protection.
>
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>
> ---
>  drivers/mtd/spi-nor/core.c | 3 +++
>  drivers/mtd/spi-nor/core.h | 1 +
>  drivers/mtd/spi-nor/swp.c  | 5 +++--
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0bb0ad14a2fc..81b57c51f41c 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2864,6 +2864,9 @@ static void spi_nor_init_flags(struct spi_nor 
> *nor)
>  	if (flags & NO_CHIP_ERASE)
>  		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>
> +	if (of_property_read_bool(np, "broken-wp"))
> +		nor->flags |= SNOR_F_BROKEN_WP;
> +
>  	if (flags & SPI_NOR_RWW && nor->info->n_banks > 1 &&
>  	    !nor->controller_ops)
>  		nor->flags |= SNOR_F_RWW;
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 4fb5ff09c63a..6ac932eba913 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -132,6 +132,7 @@ enum spi_nor_option_flags {
>  	SNOR_F_SWP_IS_VOLATILE	= BIT(13),
>  	SNOR_F_RWW		= BIT(14),
>  	SNOR_F_ECC		= BIT(15),
> +	SNOR_F_BROKEN_WP	= BIT(16),
>  };
>
>  struct spi_nor_read_command {
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 0ba716e84377..074f3bce2034 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -214,8 +214,9 @@ static int spi_nor_sr_lock(struct spi_nor *nor,
> loff_t ofs, uint64_t len)
>
>  	status_new = (status_old & ~mask & ~tb_mask) | val;
>
> -	/* Disallow further writes if WP pin is asserted */
> -	status_new |= SR_SRWD;
> +	/* Disallow further writes if WP pin is connected */

"is not broken" or similar. Maybe descibe what is broken.
Like I said, this might also be a valid use case.

Thinking more about this, maybe we should make this
configurable. I.e. make it possible to set the
locking region without disabling further writes. Although
I'm not sure how. Right now, we always enable both the
software and hardware write protection. (winbond distiguish
between software and hardware write protection here; software
here means not linux/kernel but just setting the protection
bits without the locking bit). And in the case WP# is tied
to low, one should not use the hardware write protection.

Although I'm not really sure, how to do that in a backwards
compatible way.

-michael