[PATCH 0/2] Add support to Disable the flash quad mode

[PATCH 0/2] Add support to Disable the flash quad mode

[Yicong Yang]

Previously we didn't disable the flash's quad mode when it's removed
Then comes the problem that if we next time load the flash
in SPI/Dual mode, it will not be correctly enabled as the quad enable
bits has not been cleared. I validated the condition on Cypress s25fs128s1.

This series adds the capability to disable the flash's quad mode. And
restore the flash when it's removed in spi_nor_restore().

Yicong Yang (2):
  mtd: spi-nor: Add capability to disable flash quad mode
  mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

 drivers/mtd/spi-nor/core.c | 57 +++++++++++++++++++++++++++++++---------------
 drivers/mtd/spi-nor/core.h | 10 ++++----
 2 files changed, 44 insertions(+), 23 deletions(-)

-- 
2.8.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: spi-nor: Add capability to disable flash quad mode

[Yicong Yang]

Previous we didn't provide a way to disable the flash's quad mode.
Which means we cannot do some cleanup works when to remove or
poweroff the flash, like what set 4-byte address mode does in
spi_nor_restore().

Add the capability to disable the flash quad mode, by introducing
an enable flag in the flash parameters quad_enable() hooks and
related functions.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 55 +++++++++++++++++++++++++++++++---------------
 drivers/mtd/spi-nor/core.h | 10 ++++-----
 2 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0369d98..ad5498f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1907,15 +1907,16 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 }
 
 /**
- * spi_nor_sr1_bit6_quad_enable() - Set the Quad Enable BIT(6) in the Status
- * Register 1.
+ * spi_nor_sr1_bit6_quad_enable() - Set/Unset the Quad Enable BIT(6) in the
+ *                                  Status Register 1.
  * @nor:	pointer to a 'struct spi_nor'
+ * @enable:	true to enter quad mode. false to leave quad mode.
  *
  * Bit 6 of the Status Register 1 is the QE bit for Macronix like QSPI memories.
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor)
+int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
@@ -1923,45 +1924,58 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor)
 	if (ret)
 		return ret;
 
-	if (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)
+	if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
+	    (!enable || !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))
 		return 0;
 
-	nor->bouncebuf[0] |= SR1_QUAD_EN_BIT6;
+	if (enable)
+		nor->bouncebuf[0] |= SR1_QUAD_EN_BIT6;
+	else
+		nor->bouncebuf[0] &= ~SR1_QUAD_EN_BIT6;
 
 	return spi_nor_write_sr1_and_check(nor, nor->bouncebuf[0]);
 }
 
 /**
- * spi_nor_sr2_bit1_quad_enable() - set the Quad Enable BIT(1) in the Status
- * Register 2.
+ * spi_nor_sr2_bit1_quad_enable() - set/unset the Quad Enable BIT(1) in the
+ *                                  Status Register 2.
  * @nor:       pointer to a 'struct spi_nor'.
+ * @enable:	true to enter quad mode. false to leave quad mode.
  *
  * Bit 1 of the Status Register 2 is the QE bit for Spansion like QSPI memories.
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor)
+int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
 	if (nor->flags & SNOR_F_NO_READ_CR)
-		return spi_nor_write_16bit_cr_and_check(nor, SR2_QUAD_EN_BIT1);
+		return spi_nor_write_16bit_cr_and_check(nor,
+						enable ? SR2_QUAD_EN_BIT1 : 0);
 
 	ret = spi_nor_read_cr(nor, nor->bouncebuf);
 	if (ret)
 		return ret;
 
-	if (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)
+	if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) ||
+	    !(enable || (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)))
 		return 0;
 
 	nor->bouncebuf[0] |= SR2_QUAD_EN_BIT1;
 
+	if (enable)
+		nor->bouncebuf[0] |= SR2_QUAD_EN_BIT1;
+	else
+		nor->bouncebuf[0] &= ~SR2_QUAD_EN_BIT1;
+
 	return spi_nor_write_16bit_cr_and_check(nor, nor->bouncebuf[0]);
 }
 
 /**
- * spi_nor_sr2_bit7_quad_enable() - set QE bit in Status Register 2.
+ * spi_nor_sr2_bit7_quad_enable() - set/unset QE bit in Status Register 2.
  * @nor:	pointer to a 'struct spi_nor'
+ * @enable:	true to enter quad mode. false to leave quad mode.
  *
  * Set the Quad Enable (QE) bit in the Status Register 2.
  *
@@ -1971,7 +1985,7 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor)
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
+int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable)
 {
 	u8 *sr2 = nor->bouncebuf;
 	int ret;
@@ -1981,11 +1995,15 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
 	ret = spi_nor_read_sr2(nor, sr2);
 	if (ret)
 		return ret;
-	if (*sr2 & SR2_QUAD_EN_BIT7)
+	if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) ||
+	    !(enable || (*sr2 & SR2_QUAD_EN_BIT7)))
 		return 0;
 
 	/* Update the Quad Enable bit. */
-	*sr2 |= SR2_QUAD_EN_BIT7;
+	if (enable)
+		*sr2 |= SR2_QUAD_EN_BIT7;
+	else
+		*sr2 &= ~SR2_QUAD_EN_BIT7;
 
 	ret = spi_nor_write_sr2(nor, sr2);
 	if (ret)
@@ -2898,12 +2916,13 @@ static int spi_nor_init_params(struct spi_nor *nor)
 }
 
 /**
- * spi_nor_quad_enable() - enable Quad I/O if needed.
+ * spi_nor_quad_enable() - enable/disable Quad I/O if needed.
  * @nor:                pointer to a 'struct spi_nor'
+ * @enable:             true to enable quad mode. false to disable.
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int spi_nor_quad_enable(struct spi_nor *nor)
+static int spi_nor_quad_enable(struct spi_nor *nor, bool enable)
 {
 	if (!nor->params->quad_enable)
 		return 0;
@@ -2912,7 +2931,7 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
 	      spi_nor_get_protocol_width(nor->write_proto) == 4))
 		return 0;
 
-	return nor->params->quad_enable(nor);
+	return nor->params->quad_enable(nor, enable);
 }
 
 /**
@@ -2936,7 +2955,7 @@ static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
 
-	err = spi_nor_quad_enable(nor);
+	err = spi_nor_quad_enable(nor, true);
 	if (err) {
 		dev_dbg(nor->dev, "quad mode not supported\n");
 		return err;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 6f2f6b2..222e0d3 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -198,7 +198,7 @@ struct spi_nor_locking_ops {
  *                      higher index in the array, the higher priority.
  * @erase_map:		the erase map parsed from the SFDP Sector Map Parameter
  *                      Table.
- * @quad_enable:	enables SPI NOR quad mode.
+ * @quad_enable:	enables/disables SPI NOR quad mode.
  * @set_4byte_addr_mode: puts the SPI NOR in 4 byte addressing mode.
  * @convert_addr:	converts an absolute address into something the flash
  *                      will understand. Particularly useful when pagesize is
@@ -219,7 +219,7 @@ struct spi_nor_flash_parameter {
 
 	struct spi_nor_erase_map        erase_map;
 
-	int (*quad_enable)(struct spi_nor *nor);
+	int (*quad_enable)(struct spi_nor *nor, bool enable);
 	int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
 	u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
 	int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
@@ -406,9 +406,9 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
 int spi_nor_wait_till_ready(struct spi_nor *nor);
 int spi_nor_lock_and_prep(struct spi_nor *nor);
 void spi_nor_unlock_and_unprep(struct spi_nor *nor);
-int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
-int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
-int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
+int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable);
+int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable);
+int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable);
 
 int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
-- 
2.8.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: Add capability to disable flash quad mode

[Tudor.Ambarus]

On 6/16/20 4:02 PM, Yicong Yang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Previous we didn't provide a way to disable the flash's quad mode.
> Which means we cannot do some cleanup works when to remove or
> poweroff the flash, like what set 4-byte address mode does in
> spi_nor_restore().
> 
> Add the capability to disable the flash quad mode, by introducing
> an enable flag in the flash parameters quad_enable() hooks and
> related functions.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.c | 55 +++++++++++++++++++++++++++++++---------------
>  drivers/mtd/spi-nor/core.h | 10 ++++-----
>  2 files changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0369d98..ad5498f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1907,15 +1907,16 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  }
> 
>  /**
> - * spi_nor_sr1_bit6_quad_enable() - Set the Quad Enable BIT(6) in the Status
> - * Register 1.
> + * spi_nor_sr1_bit6_quad_enable() - Set/Unset the Quad Enable BIT(6) in the
> + *                                  Status Register 1.
>   * @nor:       pointer to a 'struct spi_nor'
> + * @enable:    true to enter quad mode. false to leave quad mode.

s/enter/enable and s/leave/disable? s/quad mode/Quad Mode
>   *
>   * Bit 6 of the Status Register 1 is the QE bit for Macronix like QSPI memories.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor)
> +int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable)
>  {
>         int ret;
> 
> @@ -1923,45 +1924,58 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor)
>         if (ret)
>                 return ret;
> 
> -       if (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)
> +       if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
> +           (!enable || !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))

there's a bug here, I think that you wanted:
(!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))

>                 return 0;
> 
> -       nor->bouncebuf[0] |= SR1_QUAD_EN_BIT6;
> +       if (enable)
> +               nor->bouncebuf[0] |= SR1_QUAD_EN_BIT6;
> +       else
> +               nor->bouncebuf[0] &= ~SR1_QUAD_EN_BIT6;
> 
>         return spi_nor_write_sr1_and_check(nor, nor->bouncebuf[0]);
>  }
> 
>  /**
> - * spi_nor_sr2_bit1_quad_enable() - set the Quad Enable BIT(1) in the Status
> - * Register 2.
> + * spi_nor_sr2_bit1_quad_enable() - set/unset the Quad Enable BIT(1) in the
> + *                                  Status Register 2.
>   * @nor:       pointer to a 'struct spi_nor'.
> + * @enable:    true to enter quad mode. false to leave quad mode.

ditto

>   *
>   * Bit 1 of the Status Register 2 is the QE bit for Spansion like QSPI memories.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor)
> +int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable)
>  {
>         int ret;
> 
>         if (nor->flags & SNOR_F_NO_READ_CR)
> -               return spi_nor_write_16bit_cr_and_check(nor, SR2_QUAD_EN_BIT1);
> +               return spi_nor_write_16bit_cr_and_check(nor,
> +                                               enable ? SR2_QUAD_EN_BIT1 : 0);
> 
>         ret = spi_nor_read_cr(nor, nor->bouncebuf);
>         if (ret)
>                 return ret;
> 
> -       if (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)
> +       if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) ||
> +           !(enable || (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)))

I am too finding the following line more readable. Would you change it?

(!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))

>                 return 0;
> 
>         nor->bouncebuf[0] |= SR2_QUAD_EN_BIT1;

you forgot to drop this line

> 
> +       if (enable)
> +               nor->bouncebuf[0] |= SR2_QUAD_EN_BIT1;
> +       else
> +               nor->bouncebuf[0] &= ~SR2_QUAD_EN_BIT1;
> +
>         return spi_nor_write_16bit_cr_and_check(nor, nor->bouncebuf[0]);
>  }
> 
>  /**
> - * spi_nor_sr2_bit7_quad_enable() - set QE bit in Status Register 2.
> + * spi_nor_sr2_bit7_quad_enable() - set/unset QE bit in Status Register 2.
>   * @nor:       pointer to a 'struct spi_nor'
> + * @enable:    true to enter quad mode. false to leave quad mode.

ditto

>   *
>   * Set the Quad Enable (QE) bit in the Status Register 2.
>   *
> @@ -1971,7 +1985,7 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
> +int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable)
>  {
>         u8 *sr2 = nor->bouncebuf;
>         int ret;
> @@ -1981,11 +1995,15 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
>         ret = spi_nor_read_sr2(nor, sr2);
>         if (ret)
>                 return ret;
> -       if (*sr2 & SR2_QUAD_EN_BIT7)
> +       if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) ||
> +           !(enable || (*sr2 & SR2_QUAD_EN_BIT7)))

(!enable && !(*sr2 & SR2_QUAD_EN_BIT7))

>                 return 0;
> 
>         /* Update the Quad Enable bit. */
> -       *sr2 |= SR2_QUAD_EN_BIT7;
> +       if (enable)
> +               *sr2 |= SR2_QUAD_EN_BIT7;
> +       else
> +               *sr2 &= ~SR2_QUAD_EN_BIT7;
> 
>         ret = spi_nor_write_sr2(nor, sr2);
>         if (ret)
> @@ -2898,12 +2916,13 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  }
> 
>  /**
> - * spi_nor_quad_enable() - enable Quad I/O if needed.
> + * spi_nor_quad_enable() - enable/disable Quad I/O if needed.
>   * @nor:                pointer to a 'struct spi_nor'
> + * @enable:             true to enable quad mode. false to disable.

ditto

>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_quad_enable(struct spi_nor *nor)
> +static int spi_nor_quad_enable(struct spi_nor *nor, bool enable)
>  {
>         if (!nor->params->quad_enable)
>                 return 0;
> @@ -2912,7 +2931,7 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
>               spi_nor_get_protocol_width(nor->write_proto) == 4))
>                 return 0;
> 
> -       return nor->params->quad_enable(nor);
> +       return nor->params->quad_enable(nor, enable);
>  }
> 
>  /**
> @@ -2936,7 +2955,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  {
>         int err;
> 
> -       err = spi_nor_quad_enable(nor);
> +       err = spi_nor_quad_enable(nor, true);
>         if (err) {
>                 dev_dbg(nor->dev, "quad mode not supported\n");
>                 return err;
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 6f2f6b2..222e0d3 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -198,7 +198,7 @@ struct spi_nor_locking_ops {
>   *                      higher index in the array, the higher priority.
>   * @erase_map:         the erase map parsed from the SFDP Sector Map Parameter
>   *                      Table.
> - * @quad_enable:       enables SPI NOR quad mode.
> + * @quad_enable:       enables/disables SPI NOR quad mode.
>   * @set_4byte_addr_mode: puts the SPI NOR in 4 byte addressing mode.
>   * @convert_addr:      converts an absolute address into something the flash
>   *                      will understand. Particularly useful when pagesize is
> @@ -219,7 +219,7 @@ struct spi_nor_flash_parameter {
> 
>         struct spi_nor_erase_map        erase_map;
> 
> -       int (*quad_enable)(struct spi_nor *nor);
> +       int (*quad_enable)(struct spi_nor *nor, bool enable);
>         int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
>         u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
>         int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
> @@ -406,9 +406,9 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
>  int spi_nor_wait_till_ready(struct spi_nor *nor);
>  int spi_nor_lock_and_prep(struct spi_nor *nor);
>  void spi_nor_unlock_and_unprep(struct spi_nor *nor);
> -int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
> -int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
> -int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
> +int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable);
> +int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable);
> +int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable);
> 
>  int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
>  ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
> --
> 2.8.1
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

If the flash's quad mode is enabled, it'll remain in the quad mode when
it's removed. If we drive the flash next time in SPI/Dual mode, then
problem occurs as the flash's quad enable bit is not cleared.

Disable the quad mode in spi_nor_restore(), the flash will leave
quad mode when remove. This will make sure the flash always enter the
correct mode when loaded.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ad5498f..4c9d88b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3002,6 +3002,8 @@ void spi_nor_restore(struct spi_nor *nor)
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
 	    nor->flags & SNOR_F_BROKEN_RESET)
 		nor->params->set_4byte_addr_mode(nor, false);
+
+	spi_nor_quad_enable(nor, false);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
-- 
2.8.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Tudor.Ambarus]

On 6/16/20 4:02 PM, Yicong Yang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> If the flash's quad mode is enabled, it'll remain in the quad mode when
> it's removed. If we drive the flash next time in SPI/Dual mode, then
> problem occurs as the flash's quad enable bit is not cleared.

Please describe the problems that occur. When QE bit is one the flash
operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are
affected as they change their functionality to IO2 and IO3 when QE
is 1. Is there anything else?

While I find the intention good, there might be some problems here:
1/ w25q jvm variants come with QE "fixed" to 1. This probably means
that QE is not writable, and a writing of QE to zero will be ignored,
but we have to check.
2/S25FS128S: CR1NV[1] can set the default power-on state for the
CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The
logic here complicates a bit, and maybe we'll have to amend the
patch.

> 
> Disable the quad mode in spi_nor_restore(), the flash will leave
> quad mode when remove. This will make sure the flash always enter the
> correct mode when loaded.
s/correct/ Standard/Dual SPI

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Pratyush Yadav]

On 02/07/20 11:02AM, Tudor.Ambarus@microchip.com wrote:
> On 6/16/20 4:02 PM, Yicong Yang wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > If the flash's quad mode is enabled, it'll remain in the quad mode when
> > it's removed. If we drive the flash next time in SPI/Dual mode, then
> > problem occurs as the flash's quad enable bit is not cleared.
> 
> Please describe the problems that occur. When QE bit is one the flash
> operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are
> affected as they change their functionality to IO2 and IO3 when QE
> is 1. Is there anything else?

IIUC if we do anything that introduces a state on the flash, we want to 
clear that state up on restore. That's what we (will) do for 8D mode and 
for 4-byte addressing mode. Does that not apply here?

> While I find the intention good, there might be some problems here:
> 1/ w25q jvm variants come with QE "fixed" to 1. This probably means
> that QE is not writable, and a writing of QE to zero will be ignored,
> but we have to check.

In that case they shouldn't have a quad_enable() hook, no?

> 2/S25FS128S: CR1NV[1] can set the default power-on state for the
> CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The
> logic here complicates a bit, and maybe we'll have to amend the
> patch.
> 
> > 
> > Disable the quad mode in spi_nor_restore(), the flash will leave
> > quad mode when remove. This will make sure the flash always enter the
> > correct mode when loaded.
> s/correct/ Standard/Dual SPI
> 
> Cheers,
> ta

-- 
Regards,
Pratyush Yadav
Texas Instruments India

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Tudor.Ambarus]

On 7/3/20 2:19 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 02/07/20 11:02AM, Tudor.Ambarus@microchip.com wrote:
>> On 6/16/20 4:02 PM, Yicong Yang wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> If the flash's quad mode is enabled, it'll remain in the quad mode when
>>> it's removed. If we drive the flash next time in SPI/Dual mode, then
>>> problem occurs as the flash's quad enable bit is not cleared.
>>
>> Please describe the problems that occur. When QE bit is one the flash
>> operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are
>> affected as they change their functionality to IO2 and IO3 when QE
>> is 1. Is there anything else?
> 
> IIUC if we do anything that introduces a state on the flash, we want to
> clear that state up on restore. That's what we (will) do for 8D mode and

correct

> for 4-byte addressing mode. Does that not apply here?

yes, it does. I've just asked Yicong to describe in the commit message
the problems that he encounters, for better understanding. Standard and
Dual modes should still work with QE = 1. The only problem that I see
is that WP# and RESET#/HOLD# are changing their functionality to IO2
and IO3 when QE is 1. Is there anything else that I miss?
> 
>> While I find the intention good, there might be some problems here:
>> 1/ w25q jvm variants come with QE "fixed" to 1. This probably means
>> that QE is not writable, and a writing of QE to zero will be ignored,
>> but we have to check.
> 
> In that case they shouldn't have a quad_enable() hook, no?

Right. Although this scenario should be a false positive, probably the
write of QE bit is ignored. There is a superfluous write of QE indeed,
but maybe we can live with it.

> 
>> 2/S25FS128S: CR1NV[1] can set the default power-on state for the
>> CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The
>> logic here complicates a bit, and maybe we'll have to amend the
>> patch.
>>

We can come with a patch on top of these for 2/. Yicong, please address
the minor comments and resubmit.

Cheers.

>>>
>>> Disable the quad mode in spi_nor_restore(), the flash will leave
>>> quad mode when remove. This will make sure the flash always enter the
>>> correct mode when loaded.
>> s/correct/ Standard/Dual SPI
>>
>> Cheers,
>> ta
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

Hi,

Thanks for reviewing the patch.


On 2020/7/3 19:52, Tudor.Ambarus@microchip.com wrote:
> On 7/3/20 2:19 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 02/07/20 11:02AM, Tudor.Ambarus@microchip.com wrote:
>>> On 6/16/20 4:02 PM, Yicong Yang wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> If the flash's quad mode is enabled, it'll remain in the quad mode when
>>>> it's removed. If we drive the flash next time in SPI/Dual mode, then
>>>> problem occurs as the flash's quad enable bit is not cleared.
>>> Please describe the problems that occur. When QE bit is one the flash
>>> operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are
>>> affected as they change their functionality to IO2 and IO3 when QE
>>> is 1. Is there anything else?
>> IIUC if we do anything that introduces a state on the flash, we want to
>> clear that state up on restore. That's what we (will) do for 8D mode and
> correct
>
>> for 4-byte addressing mode. Does that not apply here?
> yes, it does. I've just asked Yicong to describe in the commit message
> the problems that he encounters, for better understanding. Standard and
> Dual modes should still work with QE = 1. The only problem that I see
> is that WP# and RESET#/HOLD# are changing their functionality to IO2
> and IO3 when QE is 1. Is there anything else that I miss?

I think I have mixup the issues.
The problem I met is when I load the driver in Quad mode first and reload it in
Standard SPI/Dual mode, and tested the flash's read/write but I got mirrored
data which differs from what I wrote.  But seems the QE bit won't cause this,
(thanks for your illustration and it may not be the same issue) so it seems
improper to mention it here and I'll reword the commit.

(PS: the flash I tested is Cypress s25fs128s1 with spi-hisi-sfc-v3xx controller. )

Thanks,
Yicong

>>> While I find the intention good, there might be some problems here:
>>> 1/ w25q jvm variants come with QE "fixed" to 1. This probably means
>>> that QE is not writable, and a writing of QE to zero will be ignored,
>>> but we have to check.
>> In that case they shouldn't have a quad_enable() hook, no?
> Right. Although this scenario should be a false positive, probably the
> write of QE bit is ignored. There is a superfluous write of QE indeed,
> but maybe we can live with it.
>
>>> 2/S25FS128S: CR1NV[1] can set the default power-on state for the
>>> CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The
>>> logic here complicates a bit, and maybe we'll have to amend the
>>> patch.
>>>
> We can come with a patch on top of these for 2/. Yicong, please address
> the minor comments and resubmit.
> Cheers.
>
>>>> Disable the quad mode in spi_nor_restore(), the flash will leave
>>>> quad mode when remove. This will make sure the flash always enter the
>>>> correct mode when loaded.
>>> s/correct/ Standard/Dual SPI
>>>
>>> Cheers,
>>> ta
>> --
>> Regards,
>> Pratyush Yadav
>> Texas Instruments India
>>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Matthias Weißer]

Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang
<yangyicong@hisilicon.com>:
> If the flash's quad mode is enabled, it'll remain in the quad mode when
> it's removed. If we drive the flash next time in SPI/Dual mode, then
> problem occurs as the flash's quad enable bit is not cleared.

On flash devices with a non-volatile quad enable bit (we use S25FL512S)
this will wear out the quad enable bit as on every boot the bit is
set and reset on shutdown. Or do I miss something here?

> Disable the quad mode in spi_nor_restore(), the flash will leave
> quad mode when remove. This will make sure the flash always enter the
> correct mode when loaded.

We have a system which relies on an enabled quad mode on boot
(bootloader uses quad mode without enabling it) so using this patch
will prevent our device from booting.

Regards,
Matthias

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Pratyush Yadav]

Hi,

On 01/09/20 08:16AM, Matthias Weißer wrote:
> Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang
> <yangyicong@hisilicon.com>:
> > If the flash's quad mode is enabled, it'll remain in the quad mode when
> > it's removed. If we drive the flash next time in SPI/Dual mode, then
> > problem occurs as the flash's quad enable bit is not cleared.
> 
> On flash devices with a non-volatile quad enable bit (we use S25FL512S)
> this will wear out the quad enable bit as on every boot the bit is
> set and reset on shutdown. Or do I miss something here?

Yes, I think it will. If you always want the bit enabled (and it being 
non-volatile suggests that is the intended use), why not set 
nor->params->quad_enable() to NULL? That way it won't be touched at all, 
neither on boot nor on shutdown.
 
> > Disable the quad mode in spi_nor_restore(), the flash will leave
> > quad mode when remove. This will make sure the flash always enter the
> > correct mode when loaded.
> 
> We have a system which relies on an enabled quad mode on boot
> (bootloader uses quad mode without enabling it) so using this patch
> will prevent our device from booting.

Out of curiosity, how does the flash get detected by SPI NOR? It issues 
the Read ID command in 1S-1S-1S mode but the flash is expecting 1S-4S-4S 
commands. Do you have any extra patches applied to make it work?
 
I tried solving this problem for 8D-8D-8D mode but never really found a 
good solution.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Matthias Weißer]

Am Di., 1. Sept. 2020 um 11:48 Uhr schrieb Pratyush Yadav <p.yadav@ti.com>:
> On 01/09/20 08:16AM, Matthias Weißer wrote:
> > Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang
> > <yangyicong@hisilicon.com>:
> > > If the flash's quad mode is enabled, it'll remain in the quad mode when
> > > it's removed. If we drive the flash next time in SPI/Dual mode, then
> > > problem occurs as the flash's quad enable bit is not cleared.
> >
> > On flash devices with a non-volatile quad enable bit (we use S25FL512S)
> > this will wear out the quad enable bit as on every boot the bit is
> > set and reset on shutdown. Or do I miss something here?
>
> Yes, I think it will. If you always want the bit enabled (and it being
> non-volatile suggests that is the intended use), why not set
> nor->params->quad_enable() to NULL? That way it won't be touched at all,
> neither on boot nor on shutdown.

Because we want to use plain mainline kernel without any local patches.

> > > Disable the quad mode in spi_nor_restore(), the flash will leave
> > > quad mode when remove. This will make sure the flash always enter the
> > > correct mode when loaded.
> >
> > We have a system which relies on an enabled quad mode on boot
> > (bootloader uses quad mode without enabling it) so using this patch
> > will prevent our device from booting.
>
> Out of curiosity, how does the flash get detected by SPI NOR? It issues
> the Read ID command in 1S-1S-1S mode but the flash is expecting 1S-4S-4S
> commands. Do you have any extra patches applied to make it work?

The RDID command (0x9F) is always a 1S-1S-1S command regardless of the
QE bit state as e.g. the READ command (0x03). The only reason for the QE
bit is that it remaps the usage of the !WP and !HOLD signals to IO2 and IO3.

The SPI controller (QSPI on imx6 in our case) have to be programmed
accordingly to execute the RDID command in 1S-1S-1S mode but then the
state of the QE bit doesn't matter.

> I tried solving this problem for 8D-8D-8D mode but never really found a
> good solution.

I don't know your hardware but I am sure that should work that way to.

The current way it is done in your patch may wear out the QE bit (and
maybe others in status/configuration register) quite quickly if you
have a device which is booted frequently.

Regards,
Matthias

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Pratyush Yadav]

On 01/09/20 12:08PM, Matthias Weißer wrote:
> Am Di., 1. Sept. 2020 um 11:48 Uhr schrieb Pratyush Yadav <p.yadav@ti.com>:
> > On 01/09/20 08:16AM, Matthias Weißer wrote:
> > > Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang
> > > <yangyicong@hisilicon.com>:
> > > > If the flash's quad mode is enabled, it'll remain in the quad mode when
> > > > it's removed. If we drive the flash next time in SPI/Dual mode, then
> > > > problem occurs as the flash's quad enable bit is not cleared.
> > >
> > > On flash devices with a non-volatile quad enable bit (we use S25FL512S)
> > > this will wear out the quad enable bit as on every boot the bit is
> > > set and reset on shutdown. Or do I miss something here?
> >
> > Yes, I think it will. If you always want the bit enabled (and it being
> > non-volatile suggests that is the intended use), why not set
> > nor->params->quad_enable() to NULL? That way it won't be touched at all,
> > neither on boot nor on shutdown.
> 
> Because we want to use plain mainline kernel without any local patches.

IMO SPI NOR should not touch non-volatile bits at all. They should be 
set by some other software and SPI NOR should only read them. So I can 
see a change like this justifiable for merging into mainline.

The other option would be to put the call to spi_nor_quad_enable() 
behind a check for SNOR_F_BROKEN_RESET, like we do for 4-byte addressing 
mode.

Either way, the patch has already landed in mainline. You will have to 
submit a patch with whichever option you think is better.
 
> > > > Disable the quad mode in spi_nor_restore(), the flash will leave
> > > > quad mode when remove. This will make sure the flash always enter the
> > > > correct mode when loaded.
> > >
> > > We have a system which relies on an enabled quad mode on boot
> > > (bootloader uses quad mode without enabling it) so using this patch
> > > will prevent our device from booting.
> >
> > Out of curiosity, how does the flash get detected by SPI NOR? It issues
> > the Read ID command in 1S-1S-1S mode but the flash is expecting 1S-4S-4S
> > commands. Do you have any extra patches applied to make it work?
> 
> The RDID command (0x9F) is always a 1S-1S-1S command regardless of the
> QE bit state as e.g. the READ command (0x03). The only reason for the QE
> bit is that it remaps the usage of the !WP and !HOLD signals to IO2 and IO3.
> 
> The SPI controller (QSPI on imx6 in our case) have to be programmed
> accordingly to execute the RDID command in 1S-1S-1S mode but then the
> state of the QE bit doesn't matter.
> 
> > I tried solving this problem for 8D-8D-8D mode but never really found a
> > good solution.
> 
> I don't know your hardware but I am sure that should work that way to.

Unfortunately for the hardware I'm using (S28HS512T) once Octal mode is 
enabled RDID is also in 8D-8D-8D mode. So this isn't applicable there. 
Still, thanks for the explanation.
 
> The current way it is done in your patch may wear out the QE bit (and
> maybe others in status/configuration register) quite quickly if you
> have a device which is booted frequently.

This isn't my patch :-)

-- 
Regards,
Pratyush Yadav
Texas Instruments India

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

Hi Matthias,


On 2020/9/1 14:16, Matthias Weißer wrote:
> Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang
> <yangyicong@hisilicon.com>:
>> If the flash's quad mode is enabled, it'll remain in the quad mode when
>> it's removed. If we drive the flash next time in SPI/Dual mode, then
>> problem occurs as the flash's quad enable bit is not cleared.
> On flash devices with a non-volatile quad enable bit (we use S25FL512S)
> this will wear out the quad enable bit as on every boot the bit is
> set and reset on shutdown. Or do I miss something here?
>
>> Disable the quad mode in spi_nor_restore(), the flash will leave
>> quad mode when remove. This will make sure the flash always enter the
>> correct mode when loaded.
> We have a system which relies on an enabled quad mode on boot
> (bootloader uses quad mode without enabling it) so using this patch
> will prevent our device from booting.

For flashes boot with standard spi mode and with quad mode enabled in kernel,
it will stay in quad mode when shutdown and next time boot. This patch intends
to address this issue as we should restore the flash's original state.

For the flashes boot with quad mode, seems we don't `restore` the flash's state
correctly. It's better to record the flash's quad mode state and if it's in quad
mode when boot, we'll not disable it when shutdown or remove.

Regards,
Yicong

>
> Regards,
> Matthias
> .
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

Hi Mathhias and Pratyush,

I've tested the following patch with s25fs128s1.
I left the flash quad enabled before managed by spi-nor driver,
and it'll stay QE after removed. So I think it'll also address the issue
mentioned. Please have a test.

Regards,
Yicong


From 43aa1afa12a10036f722b272a9a39b8c83218f33 Mon Sep 17 00:00:00 2001
From: Yicong Yang <yangyicong@hisilicon.com>
Date: Tue, 1 Sep 2020 22:09:46 +0800
Subject: [PATCH] mtd: spi-nor: don't disable quad mode of flash whose is
 originally enabled

Currently we'll disable the flash's Quad mode when remove/shutdown in
spi_nor_restore(), no matter whether it's Quad enabled or not.
For flashes originally in Quad mode, it'll clear the flash's QE bit
and restore an incorrect state.

Record the flash's original QE state, and don't disable the Quad mode
of these originally Quad enabled flash.

Reported-by: Matthias Weisser <m.weisser.m@gmail.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/mtd/spi-nor/core.c  | 30 +++++++++++++++++++++++-------
 include/linux/mtd/spi-nor.h |  3 +++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 65eff4c..b13b3b3 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1924,8 +1924,11 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable)
     if (ret)
         return ret;
 
-    if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
-        (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))
+    if (enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) {
+        nor->orig_qe_state = true;
+        return 0;
+    }
+    if (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))
         return 0;
 
     if (enable)
@@ -1958,8 +1961,12 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable)
     if (ret)
         return ret;
 
-    if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) ||
-        (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)))
+    if (enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) {
+        nor->orig_qe_state = true;
+        return 0;
+    }
+
+    if (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))
         return 0;
 
     if (enable)
@@ -1993,8 +2000,12 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable)
     ret = spi_nor_read_sr2(nor, sr2);
     if (ret)
         return ret;
-    if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) ||
-        (!enable && !(*sr2 & SR2_QUAD_EN_BIT7)))
+    if (enable && (*sr2 & SR2_QUAD_EN_BIT7)) {
+        nor->orig_qe_state = true;
+        return 0;
+    }
+
+    if (!enable && !(*sr2 & SR2_QUAD_EN_BIT7))
         return 0;
 
     /* Update the Quad Enable bit. */
@@ -3001,7 +3012,12 @@ void spi_nor_restore(struct spi_nor *nor)
         nor->flags & SNOR_F_BROKEN_RESET)
         nor->params->set_4byte_addr_mode(nor, false);
 
-    spi_nor_quad_enable(nor, false);
+    /*
+     * restore the flash's quad mode. if the flash's quad mode is
+     * enabled originally, we'll not disable it.
+     */
+    if (!nor->orig_qe_state)
+        spi_nor_quad_enable(nor, false);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 60bac2c..f343416 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -344,6 +344,8 @@ struct spi_nor_flash_parameter;
  * @read_dummy:        the dummy needed by the read operation
  * @program_opcode:    the program opcode
  * @sst_write_second:    used by the SST write operation
+ * @orig_qe_state:    used to indicate the flash's original Quad mode enable
+ *                      state. True for enabled and false for disabled.
  * @flags:        flag options for the current SPI NOR (SNOR_F_*)
  * @read_proto:        the SPI protocol for read operations
  * @write_proto:    the SPI protocol for write operations
@@ -375,6 +377,7 @@ struct spi_nor {
     enum spi_nor_protocol    write_proto;
     enum spi_nor_protocol    reg_proto;
     bool            sst_write_second;
+    bool            orig_qe_state;
     u32            flags;
 
     const struct spi_nor_controller_ops *controller_ops;
-- 
2.8.1



On 2020/9/1 14:16, Matthias Weißer wrote:
> Am Di., 16. Juni 2020 um 15:03 Uhr schrieb Yicong Yang
> <yangyicong@hisilicon.com>:
>> If the flash's quad mode is enabled, it'll remain in the quad mode when
>> it's removed. If we drive the flash next time in SPI/Dual mode, then
>> problem occurs as the flash's quad enable bit is not cleared.
> On flash devices with a non-volatile quad enable bit (we use S25FL512S)
> this will wear out the quad enable bit as on every boot the bit is
> set and reset on shutdown. Or do I miss something here?
>
>> Disable the quad mode in spi_nor_restore(), the flash will leave
>> quad mode when remove. This will make sure the flash always enter the
>> correct mode when loaded.
> We have a system which relies on an enabled quad mode on boot
> (bootloader uses quad mode without enabling it) so using this patch
> will prevent our device from booting.
>
> Regards,
> Matthias
> .
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Vignesh Raghavendra]

Hi Yicong,

On 9/1/20 7:50 PM, Yicong Yang wrote:
> Hi Mathhias and Pratyush,
> 
> I've tested the following patch with s25fs128s1.
> I left the flash quad enabled before managed by spi-nor driver,
> and it'll stay QE after removed. So I think it'll also address the issue
> mentioned. Please have a test.
> 
> Regards,
> Yicong
> 
> 
> From 43aa1afa12a10036f722b272a9a39b8c83218f33 Mon Sep 17 00:00:00 2001
> From: Yicong Yang <yangyicong@hisilicon.com>
> Date: Tue, 1 Sep 2020 22:09:46 +0800
> Subject: [PATCH] mtd: spi-nor: don't disable quad mode of flash whose is
>  originally enabled
> 
> Currently we'll disable the flash's Quad mode when remove/shutdown in
> spi_nor_restore(), no matter whether it's Quad enabled or not.
> For flashes originally in Quad mode, it'll clear the flash's QE bit
> and restore an incorrect state.
> 
> Record the flash's original QE state, and don't disable the Quad mode
> of these originally Quad enabled flash.
> 

This will break backward compatibility... Imagine a new board being 
flashed from Kernel. Before this series, QE bit would be set at the end of flashing
and  ROM/bootloader (such as the one reported by Matthias) would work fine. 
After this series, QE bit would no longer be set and would most likely
break boot..

I still am unable to understand what is the underlying problem that is 
being addressed here?

You mention addressing issue loading the driver in Quad mode first and reload it in
Standard SPI/Dual mode. But per s25fs128s data sheet:
"
Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP#
becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP#
input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still
function normally but, there is no need to drive the WP# input for those commands when switching between commands using
different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)."

So setting QE bit should have no impact for serial/dual IO modes?

Regards
Vignesh

> Reported-by: Matthias Weisser <m.weisser.m@gmail.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 30 +++++++++++++++++++++++-------
>  include/linux/mtd/spi-nor.h |  3 +++
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 65eff4c..b13b3b3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1924,8 +1924,11 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable)
>      if (ret)
>          return ret;
>  
> -    if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
> -        (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))
> +    if (enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) {
> +        nor->orig_qe_state = true;
> +        return 0;
> +    }
> +    if (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))
>          return 0;
>  
>      if (enable)
> @@ -1958,8 +1961,12 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable)
>      if (ret)
>          return ret;
>  
> -    if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) ||
> -        (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)))
> +    if (enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) {
> +        nor->orig_qe_state = true;
> +        return 0;
> +    }
> +
> +    if (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))
>          return 0;
>  
>      if (enable)
> @@ -1993,8 +2000,12 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable)
>      ret = spi_nor_read_sr2(nor, sr2);
>      if (ret)
>          return ret;
> -    if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) ||
> -        (!enable && !(*sr2 & SR2_QUAD_EN_BIT7)))
> +    if (enable && (*sr2 & SR2_QUAD_EN_BIT7)) {
> +        nor->orig_qe_state = true;
> +        return 0;
> +    }
> +
> +    if (!enable && !(*sr2 & SR2_QUAD_EN_BIT7))
>          return 0;
>  
>      /* Update the Quad Enable bit. */
> @@ -3001,7 +3012,12 @@ void spi_nor_restore(struct spi_nor *nor)
>          nor->flags & SNOR_F_BROKEN_RESET)
>          nor->params->set_4byte_addr_mode(nor, false);
>  
> -    spi_nor_quad_enable(nor, false);
> +    /*
> +     * restore the flash's quad mode. if the flash's quad mode is
> +     * enabled originally, we'll not disable it.
> +     */
> +    if (!nor->orig_qe_state)
> +        spi_nor_quad_enable(nor, false);
>  }
>  EXPORT_SYMBOL_GPL(spi_nor_restore);
>  
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 60bac2c..f343416 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -344,6 +344,8 @@ struct spi_nor_flash_parameter;
>   * @read_dummy:        the dummy needed by the read operation
>   * @program_opcode:    the program opcode
>   * @sst_write_second:    used by the SST write operation
> + * @orig_qe_state:    used to indicate the flash's original Quad mode enable
> + *                      state. True for enabled and false for disabled.
>   * @flags:        flag options for the current SPI NOR (SNOR_F_*)
>   * @read_proto:        the SPI protocol for read operations
>   * @write_proto:    the SPI protocol for write operations
> @@ -375,6 +377,7 @@ struct spi_nor {
>      enum spi_nor_protocol    write_proto;
>      enum spi_nor_protocol    reg_proto;
>      bool            sst_write_second;
> +    bool            orig_qe_state;
>      u32            flags;
>  
>      const struct spi_nor_controller_ops *controller_ops;
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

Hi Vignesh,


On 2020/9/2 15:50, Vignesh Raghavendra wrote:
> Hi Yicong,
>
> On 9/1/20 7:50 PM, Yicong Yang wrote:
>> Hi Mathhias and Pratyush,
>>
>> I've tested the following patch with s25fs128s1.
>> I left the flash quad enabled before managed by spi-nor driver,
>> and it'll stay QE after removed. So I think it'll also address the issue
>> mentioned. Please have a test.
>>
>> Regards,
>> Yicong
>>
>>
>> From 43aa1afa12a10036f722b272a9a39b8c83218f33 Mon Sep 17 00:00:00 2001
>> From: Yicong Yang <yangyicong@hisilicon.com>
>> Date: Tue, 1 Sep 2020 22:09:46 +0800
>> Subject: [PATCH] mtd: spi-nor: don't disable quad mode of flash whose is
>>  originally enabled
>>
>> Currently we'll disable the flash's Quad mode when remove/shutdown in
>> spi_nor_restore(), no matter whether it's Quad enabled or not.
>> For flashes originally in Quad mode, it'll clear the flash's QE bit
>> and restore an incorrect state.
>>
>> Record the flash's original QE state, and don't disable the Quad mode
>> of these originally Quad enabled flash.
>>
> This will break backward compatibility... Imagine a new board being 
> flashed from Kernel. Before this series, QE bit would be set at the end of flashing
> and  ROM/bootloader (such as the one reported by Matthias) would work fine. 
> After this series, QE bit would no longer be set and would most likely
> break boot..
>
> I still am unable to understand what is the underlying problem that is 
> being addressed here?
>
> You mention addressing issue loading the driver in Quad mode first and reload it in
> Standard SPI/Dual mode. But per s25fs128s data sheet:
> "
> Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP#
> becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP#
> input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still
> function normally but, there is no need to drive the WP# input for those commands when switching between commands using
> different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)."
>
> So setting QE bit should have no impact for serial/dual IO modes?

yes. and I reword the commit like below,  as suggested by Tudor and send a v2 patch. This thread is the v1 one.
"If the flash's quad mode is enabled, it'll remain in the quad mode when
it's removed. If we drive the flash next time in Standard/Dual SPI mode,
the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD#
have been switched to IO2 and IO3 and are not restored."

I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash
in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems
the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash
in a wrong state.

Regards
Yicong


>
> Regards
> Vignesh
>
>> Reported-by: Matthias Weisser <m.weisser.m@gmail.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/mtd/spi-nor/core.c  | 30 +++++++++++++++++++++++-------
>>  include/linux/mtd/spi-nor.h |  3 +++
>>  2 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 65eff4c..b13b3b3 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1924,8 +1924,11 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable)
>>      if (ret)
>>          return ret;
>>  
>> -    if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
>> -        (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))
>> +    if (enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) {
>> +        nor->orig_qe_state = true;
>> +        return 0;
>> +    }
>> +    if (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))
>>          return 0;
>>  
>>      if (enable)
>> @@ -1958,8 +1961,12 @@ int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable)
>>      if (ret)
>>          return ret;
>>  
>> -    if ((enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) ||
>> -        (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)))
>> +    if (enable && (nor->bouncebuf[0] & SR2_QUAD_EN_BIT1)) {
>> +        nor->orig_qe_state = true;
>> +        return 0;
>> +    }
>> +
>> +    if (!enable && !(nor->bouncebuf[0] & SR2_QUAD_EN_BIT1))
>>          return 0;
>>  
>>      if (enable)
>> @@ -1993,8 +2000,12 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor, bool enable)
>>      ret = spi_nor_read_sr2(nor, sr2);
>>      if (ret)
>>          return ret;
>> -    if ((enable && (*sr2 & SR2_QUAD_EN_BIT7)) ||
>> -        (!enable && !(*sr2 & SR2_QUAD_EN_BIT7)))
>> +    if (enable && (*sr2 & SR2_QUAD_EN_BIT7)) {
>> +        nor->orig_qe_state = true;
>> +        return 0;
>> +    }
>> +
>> +    if (!enable && !(*sr2 & SR2_QUAD_EN_BIT7))
>>          return 0;
>>  
>>      /* Update the Quad Enable bit. */
>> @@ -3001,7 +3012,12 @@ void spi_nor_restore(struct spi_nor *nor)
>>          nor->flags & SNOR_F_BROKEN_RESET)
>>          nor->params->set_4byte_addr_mode(nor, false);
>>  
>> -    spi_nor_quad_enable(nor, false);
>> +    /*
>> +     * restore the flash's quad mode. if the flash's quad mode is
>> +     * enabled originally, we'll not disable it.
>> +     */
>> +    if (!nor->orig_qe_state)
>> +        spi_nor_quad_enable(nor, false);
>>  }
>>  EXPORT_SYMBOL_GPL(spi_nor_restore);
>>  
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 60bac2c..f343416 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -344,6 +344,8 @@ struct spi_nor_flash_parameter;
>>   * @read_dummy:        the dummy needed by the read operation
>>   * @program_opcode:    the program opcode
>>   * @sst_write_second:    used by the SST write operation
>> + * @orig_qe_state:    used to indicate the flash's original Quad mode enable
>> + *                      state. True for enabled and false for disabled.
>>   * @flags:        flag options for the current SPI NOR (SNOR_F_*)
>>   * @read_proto:        the SPI protocol for read operations
>>   * @write_proto:    the SPI protocol for write operations
>> @@ -375,6 +377,7 @@ struct spi_nor {
>>      enum spi_nor_protocol    write_proto;
>>      enum spi_nor_protocol    reg_proto;
>>      bool            sst_write_second;
>> +    bool            orig_qe_state;
>>      u32            flags;
>>  
>>      const struct spi_nor_controller_ops *controller_ops;
>>
> .
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Vignesh Raghavendra]

On 9/2/20 3:42 PM, Yicong Yang wrote:
> Hi Vignesh,
> 
> 
> On 2020/9/2 15:50, Vignesh Raghavendra wrote:
>> Hi Yicong,
>>
>> On 9/1/20 7:50 PM, Yicong Yang wrote:
>>> Hi Mathhias and Pratyush,
>>>
[...]
>>>
>> This will break backward compatibility... Imagine a new board being 
>> flashed from Kernel. Before this series, QE bit would be set at the end of flashing
>> and  ROM/bootloader (such as the one reported by Matthias) would work fine. 
>> After this series, QE bit would no longer be set and would most likely
>> break boot..
>>
>> I still am unable to understand what is the underlying problem that is 
>> being addressed here?
>>
>> You mention addressing issue loading the driver in Quad mode first and reload it in
>> Standard SPI/Dual mode. But per s25fs128s data sheet:
>> "
>> Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP#
>> becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP#
>> input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still
>> function normally but, there is no need to drive the WP# input for those commands when switching between commands using
>> different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)."
>>
>> So setting QE bit should have no impact for serial/dual IO modes?
> 
> yes. and I reword the commit like below,  as suggested by Tudor and send a v2 patch. This thread is the v1 one.
> "If the flash's quad mode is enabled, it'll remain in the quad mode when
> it's removed. If we drive the flash next time in Standard/Dual SPI mode,
> the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD#
> have been switched to IO2 and IO3 and are not restored."
> 
> I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash
> in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems
> the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash
> in a wrong state.
> 

How do you load driver in Quad mode first and then reload in Single/Dual  
mode later on? What is the use case?

I don't think relying on WP# and RESET#/HOLD# functionality for a QSPI 
flash is right thing to do as these lines would act as data lines in 
Quad mode and thus WP# wont really protect contents of the flash 
when in Quad mode.

Also, below patch is not fool proof even for hypothetical case that you are
trying to solve: 
Consider a scenario of kernel crash or hard reset, then there will be no 
chance to call spi_nor_restore() and you would end up with QE bit set.. 
Upon reboot, kernel will find that QE bit and will simply restore the same
back on shutdown.

Given the fact that setting and unsetting NV bit causes wearing of this 
rather important bit and also breaks backward compatibility of tools 
that expect Kernel to set QE bit on flashing, I suggest reverting these patches:

cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()
be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode


Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

On 2020/9/3 13:59, Vignesh Raghavendra wrote:
>
> On 9/2/20 3:42 PM, Yicong Yang wrote:
>> Hi Vignesh,
>>
>>
>> On 2020/9/2 15:50, Vignesh Raghavendra wrote:
>>> Hi Yicong,
>>>
>>> On 9/1/20 7:50 PM, Yicong Yang wrote:
>>>> Hi Mathhias and Pratyush,
>>>>
> [...]
>>> This will break backward compatibility... Imagine a new board being 
>>> flashed from Kernel. Before this series, QE bit would be set at the end of flashing
>>> and  ROM/bootloader (such as the one reported by Matthias) would work fine. 
>>> After this series, QE bit would no longer be set and would most likely
>>> break boot..
>>>
>>> I still am unable to understand what is the underlying problem that is 
>>> being addressed here?
>>>
>>> You mention addressing issue loading the driver in Quad mode first and reload it in
>>> Standard SPI/Dual mode. But per s25fs128s data sheet:
>>> "
>>> Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP#
>>> becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP#
>>> input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still
>>> function normally but, there is no need to drive the WP# input for those commands when switching between commands using
>>> different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)."
>>>
>>> So setting QE bit should have no impact for serial/dual IO modes?
>> yes. and I reword the commit like below,  as suggested by Tudor and send a v2 patch. This thread is the v1 one.
>> "If the flash's quad mode is enabled, it'll remain in the quad mode when
>> it's removed. If we drive the flash next time in Standard/Dual SPI mode,
>> the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD#
>> have been switched to IO2 and IO3 and are not restored."
>>
>> I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash
>> in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems
>> the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash
>> in a wrong state.
>>
> How do you load driver in Quad mode first and then reload in Single/Dual  
> mode later on? What is the use case?

we use module paramters to indicate the bus width in a private version of our driver,
yet this hasn't been upstreamed.

>
> I don't think relying on WP# and RESET#/HOLD# functionality for a QSPI 
> flash is right thing to do as these lines would act as data lines in 
> Quad mode and thus WP# wont really protect contents of the flash 
> when in Quad mode.
>
> Also, below patch is not fool proof even for hypothetical case that you are
> trying to solve: 
> Consider a scenario of kernel crash or hard reset, then there will be no 
> chance to call spi_nor_restore() and you would end up with QE bit set.. 
> Upon reboot, kernel will find that QE bit and will simply restore the same
> back on shutdown.

well this make sense to me.

>
> Given the fact that setting and unsetting NV bit causes wearing of this 
> rather important bit and also breaks backward compatibility of tools 
> that expect Kernel to set QE bit on flashing, I suggest reverting these patches:
>
> cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()
> be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode

I've send the revert patches. You may found at :
https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/

but I still have something uncertain, I think we should avoid setting the non-volatile bits
in spi-nor driver, should we?

Regards
Yicong

>
>
> Regards
> Vignesh
> .
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Matthias Weißer]

Am Fr., 4. Sept. 2020 um 09:55 Uhr schrieb Yicong Yang
<yangyicong@hisilicon.com>:
> > Given the fact that setting and unsetting NV bit causes wearing of this
> > rather important bit and also breaks backward compatibility of tools
> > that expect Kernel to set QE bit on flashing, I suggest reverting these patches:
> >
> > cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()
> > be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode
>
> I've send the revert patches. You may found at :
> https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/

Thanks. I think this is currently the way to go.

> but I still have something uncertain, I think we should avoid setting the non-volatile bits
> in spi-nor driver, should we?

Why not? If quad mode should be used the QE bit has to be set. There is no
other way to enable quad mode (on the devices I am aware of).

Regards,
Matthias

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Matthias Weißer]

Hi Vignesh

Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang
<yangyicong@hisilicon.com>:
> I've tested the following patch with s25fs128s1.
> I left the flash quad enabled before managed by spi-nor driver,
> and it'll stay QE after removed. So I think it'll also address the issue
> mentioned. Please have a test.

Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware
and your patch on top of it fixes that. Therefore

Tested-by: Matthias Weisser <m.weisser.m@gmail.com>

But I am still concerned about wearing out nonvolatile QE bits (not in my
case, but there may be others) which may also brick hardware after a lot of
boots (by set and reset QE bit on every boot).

So, from my limited point of view, your patch fixes my problem but overall
I think the original approach should be thought-out a bit more.

Regards,
Matthias

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

Hi Matthias,


On 2020/9/2 20:15, Matthias Weißer wrote:
> Hi Vignesh
>
> Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang
> <yangyicong@hisilicon.com>:
>> I've tested the following patch with s25fs128s1.
>> I left the flash quad enabled before managed by spi-nor driver,
>> and it'll stay QE after removed. So I think it'll also address the issue
>> mentioned. Please have a test.
> Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware
> and your patch on top of it fixes that. Therefore
>
> Tested-by: Matthias Weisser <m.weisser.m@gmail.com>
>
> But I am still concerned about wearing out nonvolatile QE bits (not in my
> case, but there may be others) which may also brick hardware after a lot of
> boots (by set and reset QE bit on every boot).
>
> So, from my limited point of view, your patch fixes my problem but overall
> I think the original approach should be thought-out a bit more.

Thanks for testing the patch.

The driver will try to set the QE bit when 1) in ->quad_enable() if QE bit is not set and
2) previously without this patch in spi_nor_restore() to try to disable the flash quad mode.

With this patch, if the QE bit is originally set the driver will only read and record it and
will not disable the quad mode in spi_nor_restore(). What the driver will do is to read
the bit, without trying to set or reset it. So I think it will not wear out the those nonvolatile
QE bits.

Regards,
Yicong


>
> Regards,
> Matthias
> .
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Matthias Weißer]

Am Do., 3. Sept. 2020 um 05:04 Uhr schrieb Yicong Yang
<yangyicong@hisilicon.com>:
>
> Hi Matthias,
> On 2020/9/2 20:15, Matthias Weißer wrote:
> > Hi Vignesh
> >
> > Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang
> > <yangyicong@hisilicon.com>:
> >> I've tested the following patch with s25fs128s1.
> >> I left the flash quad enabled before managed by spi-nor driver,
> >> and it'll stay QE after removed. So I think it'll also address the issue
> >> mentioned. Please have a test.
> > Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware
> > and your patch on top of it fixes that. Therefore
> >
> > Tested-by: Matthias Weisser <m.weisser.m@gmail.com>
> >
> > But I am still concerned about wearing out nonvolatile QE bits (not in my
> > case, but there may be others) which may also brick hardware after a lot of
> > boots (by set and reset QE bit on every boot).
> >
> > So, from my limited point of view, your patch fixes my problem but overall
> > I think the original approach should be thought-out a bit more.
>
> Thanks for testing the patch.
>
> The driver will try to set the QE bit when 1) in ->quad_enable() if QE bit is not set and
> 2) previously without this patch in spi_nor_restore() to try to disable the flash quad mode.
>
> With this patch, if the QE bit is originally set the driver will only read and record it and
> will not disable the quad mode in spi_nor_restore(). What the driver will do is to read
> the bit, without trying to set or reset it. So I think it will not wear out the those nonvolatile
> QE bits.

I think it will. Expect a flash with the QE bit not set. Kernel boots, set the
bit which will cause an erase cycle (setting it from 0 to 1). On shutdown the
bit is reset (transition from 1 to 0) which will cause a program cycle. So,
every boot -> shutdown cycle causes at least one program-erase cycle which will
wear out that area of the flash.

See also this quote from the datasheet of F25FL512S:
"Non-volatile bits have the same cycling (erase and program) endurance
as the main
flash array."

I still think the reset of the QE (or any other non-volatile) bit on
shutdown should
be prevented and only allowed by explicitly enable it (e.g. per system in DT or
per flash in flash_info struct).

Regards,
Matthias

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

On 2020/9/3 13:33, Matthias Weißer wrote:
> Am Do., 3. Sept. 2020 um 05:04 Uhr schrieb Yicong Yang
> <yangyicong@hisilicon.com>:
>> Hi Matthias,
>> On 2020/9/2 20:15, Matthias Weißer wrote:
>>> Hi Vignesh
>>>
>>> Am Di., 1. Sept. 2020 um 16:20 Uhr schrieb Yicong Yang
>>> <yangyicong@hisilicon.com>:
>>>> I've tested the following patch with s25fs128s1.
>>>> I left the flash quad enabled before managed by spi-nor driver,
>>>> and it'll stay QE after removed. So I think it'll also address the issue
>>>> mentioned. Please have a test.
>>> Thanks for the patch. I can confirm that current 5.9-rc3 bricks my hardware
>>> and your patch on top of it fixes that. Therefore
>>>
>>> Tested-by: Matthias Weisser <m.weisser.m@gmail.com>
>>>
>>> But I am still concerned about wearing out nonvolatile QE bits (not in my
>>> case, but there may be others) which may also brick hardware after a lot of
>>> boots (by set and reset QE bit on every boot).
>>>
>>> So, from my limited point of view, your patch fixes my problem but overall
>>> I think the original approach should be thought-out a bit more.
>> Thanks for testing the patch.
>>
>> The driver will try to set the QE bit when 1) in ->quad_enable() if QE bit is not set and
>> 2) previously without this patch in spi_nor_restore() to try to disable the flash quad mode.
>>
>> With this patch, if the QE bit is originally set the driver will only read and record it and
>> will not disable the quad mode in spi_nor_restore(). What the driver will do is to read
>> the bit, without trying to set or reset it. So I think it will not wear out the those nonvolatile
>> QE bits.
> I think it will. Expect a flash with the QE bit not set. Kernel boots, set the
> bit which will cause an erase cycle (setting it from 0 to 1). On shutdown the
> bit is reset (transition from 1 to 0) which will cause a program cycle. So,
> every boot -> shutdown cycle causes at least one program-erase cycle which will
> wear out that area of the flash.
>
> See also this quote from the datasheet of F25FL512S:
> "Non-volatile bits have the same cycling (erase and program) endurance
> as the main
> flash array."
>
> I still think the reset of the QE (or any other non-volatile) bit on
> shutdown should
> be prevented and only allowed by explicitly enable it (e.g. per system in DT or
> per flash in flash_info struct).

you may find the revert patches at:
https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/

thanks for the illustration.

Regards,
Yicong

>
> Regards,
> Matthias
> .
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/