Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

+ James and Yong from Cypress.

Hi, Jonas, James, Yong,

On 01/30/2019 12:07 AM, Jonas Bonn wrote:
> The status register bit SRWD (status register write disable) is
> described in many words in the datasheets but effectively boils down to:
> 
> i) if set, respect WP# when trying to change protection bits;
> ii) if unset, ignore WP# when trying to change protection bits
> 
> In short, the bit determines whether the WP# signal is honored or not.
> 
> It's difficult to imagine the use-case where the WP# is connected and
> asserted but the user doesn't want to respect its setting.  As such,
> this patch sets the SRWD bit unconditionally so that the WP# is _always_
> respected; hardware that doesn't care about WP# normally won't even have
> it connected.
> 

Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place.
Do you know why Cypress made this bit configurable and didn't enable it by default?

Cheers,
ta

> Tested on a Cypress s25fl512s.  With this patch, the WP# is always
> respected, irregardless of whether any flash protection bits are set.
> 
> Signed-off-by: Jonas Bonn <jonas@xxxxxxxxxxx>
> CC: Marek Vasut <marek.vasut@xxxxxxxxx>
> CC: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> CC: Brian Norris <computersforpeace@xxxxxxxxx>
> CC: Boris Brezillon <bbrezillon@xxxxxxxxxx>
> CC: Richard Weinberger <richard@xxxxxx>
> CC: linux-mtd@xxxxxxxxxxxxxxxxxxx
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 13a5055e5f3f..4c8ce2b90838 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  
>  	status_new = (status_old & ~mask & ~SR_TB) | val;
>  
> -	/* Disallow further writes if WP pin is asserted */
> -	status_new |= SR_SRWD;
> -
>  	if (!use_top)
>  		status_new |= SR_TB;
>  
> @@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  
>  	status_new = (status_old & ~mask & ~SR_TB) | val;
>  
> -	/* Don't protect status register if we're fully unlocked */
> -	if (lock_len == 0)
> -		status_new &= ~SR_SRWD;
> -
>  	if (!use_top)
>  		status_new |= SR_TB;
>  
> @@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor,
>  static int spi_nor_init(struct spi_nor *nor)
>  {
>  	int err;
> +	int sr;
>  
>  	/*
>  	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> @@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor)
>  		set_4byte(nor, true);
>  	}
>  
> -	return 0;
> +	/* Always respect the WP# (write-protect) input */
> +	sr = read_sr(nor);
> +	if (sr < 0) {
> +		dev_err(nor->dev, "error while reading status register\n");
> +		return -EINVAL;
> +	}
> +	sr |= SR_SRWD;
> +	return write_sr_and_check(nor, sr, SR_SRWD);
>  }
>  
>  /* mtd resume handler */
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/