[PATCH 0/2] mtd: spi-nor: Avoid setting SRWD bit in SR

[PATCH 0/2] mtd: spi-nor: Avoid setting SRWD bit in SR

[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. To avoid this a boolean type DT property
"broken-wp" is introduced. If this property is defined, the spi-nor doesn't
set the SRWD bit in SR while performing flash protection operation.
---
BRANCH: for-next
---
Amit Kumar Mahapatra (2):
  dt-bindings: mtd: jedec, spi-nor: Add DT property to avoid setting
    SRWD bit in status register
  mtd: spi-nor: Avoid setting SRWD bit in SR if WP signal not connected

 .../devicetree/bindings/mtd/jedec,spi-nor.yaml      | 13 +++++++++++++
 drivers/mtd/spi-nor/core.c                          |  3 +++
 drivers/mtd/spi-nor/core.h                          |  1 +
 drivers/mtd/spi-nor/swp.c                           |  5 +++--
 4 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] dt-bindings: mtd: jedec, spi-nor: Add DT property to avoid setting SRWD bit in status register

[Amit Kumar Mahapatra]

If the WP signal of the flash device is not connected and the software sets
the status register write disable (SRWD) bit in the status register then
thestatus register permanently becomes read-only. To avoid this added a new
boolean DT property "broken-wp". If WP signal is not connected, then this
property should be set in the DT to avoid setting the SRWD during status
register write operation.

Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
---
 .../devicetree/bindings/mtd/jedec,spi-nor.yaml      | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 89959e5c47ba..a509d34f14b2 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -70,6 +70,19 @@ properties:
       be used on such systems, to denote the absence of a reliable reset
       mechanism.
 
+  broken-wp:
+    type: boolean
+    description:
+      The SRWD bit in status register, combined with the WP signal provides
+      hardware data protection for the device. When the SRWD bit is set to 1,
+      and the WP signal is driven LOW, the status register nonvolatile bits
+      become read-only and the WRITE STATUS REGISTER operation will not execute.
+      The only way to exit this hardware-protected mode is to drive WP HIGH. But
+      if the WP signal of the flash device is not connected then status register
+      permanently becomes read-only as the SRWD bit cannot be reset. This boolean
+      flag can be used on such systems in which WP signal is not connected, to
+      avoid setting the SRWD bit while writing the status register.
+
   reset-gpios:
     description:
       A GPIO line connected to the RESET (active low) signal of the device.
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: mtd: jedec, spi-nor: Add DT property to avoid setting SRWD bit in status register

[Michael Walle]

Am 2023-06-15 13:16, schrieb Amit Kumar Mahapatra:
> If the WP signal of the flash device is not connected and the software 
> sets
> the status register write disable (SRWD) bit in the status register 
> then
> thestatus register permanently becomes read-only. To avoid this added a 
> new
> boolean DT property "broken-wp". If WP signal is not connected, then 
> this
> property should be set in the DT to avoid setting the SRWD during 
> status
> register write operation.
> 
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> ---
>  .../devicetree/bindings/mtd/jedec,spi-nor.yaml      | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> index 89959e5c47ba..a509d34f14b2 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> @@ -70,6 +70,19 @@ properties:
>        be used on such systems, to denote the absence of a reliable 
> reset
>        mechanism.
> 
> +  broken-wp:
> +    type: boolean
> +    description:
> +      The SRWD bit in status register, combined with the WP signal 
> provides
> +      hardware data protection for the device. When the SRWD bit is 
> set to 1,
> +      and the WP signal is driven LOW, the status register nonvolatile 
> bits
> +      become read-only and the WRITE STATUS REGISTER operation will
> not execute.
> +      The only way to exit this hardware-protected mode is to drive
> WP HIGH. But
> +      if the WP signal of the flash device is not connected then
> status register
> +      permanently becomes read-only as the SRWD bit cannot be reset.
> This boolean
> +      flag can be used on such systems in which WP signal is not 
> connected, to
> +      avoid setting the SRWD bit while writing the status register.

FWIW, this is also a valid use case: have the WP# pin tied to low, the
OEM will program the flash and then enable locking making the flash
permanently write protected. IWO, if the pin is hard strapped to low,
it is not always broken. You might add that to the description.

-michael

Re: [PATCH 1/2] dt-bindings: mtd: jedec, spi-nor: Add DT property to avoid setting SRWD bit in status register

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]

On Thu, Jun 15, 2023 at 04:46:48PM +0530, Amit Kumar Mahapatra wrote:
> If the WP signal of the flash device is not connected and the software sets
> the status register write disable (SRWD) bit in the status register then
> thestatus register permanently becomes read-only. To avoid this added a new
> boolean DT property "broken-wp". If WP signal is not connected, then this
> property should be set in the DT to avoid setting the SRWD during status
> register write operation.
> 
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> ---
>  .../devicetree/bindings/mtd/jedec,spi-nor.yaml      | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> index 89959e5c47ba..a509d34f14b2 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> @@ -70,6 +70,19 @@ properties:
>        be used on such systems, to denote the absence of a reliable reset
>        mechanism.
>  
> +  broken-wp:
> +    type: boolean
> +    description:
> +      The SRWD bit in status register, combined with the WP signal provides

Should the first use of SRWD be spelt out as you did in your commit
message?

> +      The SRWD bit in status register, combined with the WP signal provides
                                                                     ^
nit: missing a comma here I think.

> +      hardware data protection for the device. When the SRWD bit is set to 1,
> +      and the WP signal is driven LOW, the status register nonvolatile bits
> +      become read-only and the WRITE STATUS REGISTER operation will not execute.
> +      The only way to exit this hardware-protected mode is to drive WP HIGH. But
> +      if the WP signal of the flash device is not connected then status register
> +      permanently becomes read-only as the SRWD bit cannot be reset. This boolean
> +      flag can be used on such systems in which WP signal is not connected, to

nit: s/such//

Otherwise, seems reasonable to me & the detail in the description is
nice.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> +      avoid setting the SRWD bit while writing the status register.
> +
>    reset-gpios:
>      description:
>        A GPIO line connected to the RESET (active low) signal of the device.
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[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@amd.com>
---
 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 */
+	if (!(nor->flags & SNOR_F_BROKEN_WP))
+		status_new |= SR_SRWD;
 
 	if (!use_top)
 		status_new |= tb_mask;
-- 
2.17.1

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@amd.com>
> ---
>  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