[PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Hsin-Yi Wang]

gd25lq64c has Quad Enable Requirement flag parsed as
BFPT_DWORD15_QER_SR2_BIT1_BUGGY in BFPT, even if spi-{rx/tx}-bus-width
set as non QUAD, eg. 0, 1, 2... Thus quad_enable will not be NULL and
quad enable (QE) bit will be set to 1 by default. According to
datasheet[1], if QE bit is enabled, WP pin will become IO pin and the
system can't use write protection feature, and it's also not recommended
to set QE bit to 1[1].

Add a post_bfpt fixup that reads spi-rx-bus-width to remove quad_enable
if the width is set to below QUAD mode.

[1]
https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00012-GD25LQ64C-Rev3.4.pdf
page 13

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/mtd/spi-nor/gigadevice.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/gigadevice.c b/drivers/mtd/spi-nor/gigadevice.c
index d57ddaf1525b3..8ea89e1858f9b 100644
--- a/drivers/mtd/spi-nor/gigadevice.c
+++ b/drivers/mtd/spi-nor/gigadevice.c
@@ -33,6 +33,31 @@ static const struct spi_nor_fixups gd25q256_fixups = {
 	.post_bfpt = gd25q256_post_bfpt,
 };
 
+static int
+gd25lq64c_post_bfpt(struct spi_nor *nor,
+		    const struct sfdp_parameter_header *bfpt_header,
+		    const struct sfdp_bfpt *bfpt)
+{
+	struct device_node *np = spi_nor_get_flash_node(nor);
+	u32 value;
+
+	/*
+	 * Even if spi-{tx,rx}-bus-width is set to DUAL mode, due to the QER
+	 * flag parsed from BFPT is BFPT_DWORD15_QER_SR2_BIT1_BUGGY, so the
+	 * quad_enable will be set and QE bit set to 1.
+	 */
+	if (!of_property_read_u32(np, "spi-rx-bus-width", &value)) {
+		if (value <= 2)
+			nor->params->quad_enable = NULL;
+	}
+
+	return 0;
+}
+
+static struct spi_nor_fixups gd25lq64c_fixups = {
+	.post_bfpt = gd25lq64c_post_bfpt,
+};
+
 static const struct flash_info gigadevice_nor_parts[] = {
 	{ "gd25q16", INFO(0xc84015, 0, 64 * 1024,  32)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
@@ -53,7 +78,8 @@ static const struct flash_info gigadevice_nor_parts[] = {
 	{ "gd25lq64c", INFO(0xc86017, 0, 64 * 1024, 128)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
+			      SPI_NOR_QUAD_READ)
+		.fixups = &gd25lq64c_fixups },
 	{ "gd25lq128d", INFO(0xc86018, 0, 64 * 1024, 256)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
-- 
2.41.0.694.ge786442a9b-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2,2/2] arm64: dts: mediatek: mt8183: set bus rx width to disable quad mode

[Hsin-Yi Wang]

Some of the SKUs are using gigadevice gd25lq64c flash chip. The chip
default enables quad mode, which results in the write protect pin set to
IO pin. In mt8183 kukui, we won't use quad enable for all SKUs, so apply
the property to disable spi nor's quad mode.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
index 6ce16a265e053..ef472b522f2e7 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
@@ -877,6 +877,7 @@ w25q64dw: flash@0 {
 		compatible = "winbond,w25q64dw", "jedec,spi-nor";
 		reg = <0>;
 		spi-max-frequency = <25000000>;
+		spi-rx-bus-width = <2>;
 	};
 };
 
-- 
2.41.0.694.ge786442a9b-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,2/2] arm64: dts: mediatek: mt8183: set bus rx width to disable quad mode

[Hsin-Yi Wang]

On Wed, Aug 16, 2023 at 6:42 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some of the SKUs are using gigadevice gd25lq64c flash chip. The chip
> default enables quad mode, which results in the write protect pin set to
> IO pin. In mt8183 kukui, we won't use quad enable for all SKUs, so apply
> the property to disable spi nor's quad mode.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
change log:
v1->v2: use existing property.
v1 link: https://lore.kernel.org/lkml/20230815154412.713846-1-hsinyi@chromium.org/
---
>  arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
> index 6ce16a265e053..ef472b522f2e7 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
> @@ -877,6 +877,7 @@ w25q64dw: flash@0 {
>                 compatible = "winbond,w25q64dw", "jedec,spi-nor";
>                 reg = <0>;
>                 spi-max-frequency = <25000000>;
> +               spi-rx-bus-width = <2>;
>         };
>  };
>
> --
> 2.41.0.694.ge786442a9b-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,2/2] arm64: dts: mediatek: mt8183: set bus rx width to disable quad mode

[Doug Anderson]

Hi,

On Wed, Aug 16, 2023 at 3:43 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some of the SKUs are using gigadevice gd25lq64c flash chip. The chip
> default enables quad mode, which results in the write protect pin set to
> IO pin. In mt8183 kukui, we won't use quad enable for all SKUs, so apply
> the property to disable spi nor's quad mode.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
> index 6ce16a265e053..ef472b522f2e7 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
> @@ -877,6 +877,7 @@ w25q64dw: flash@0 {
>                 compatible = "winbond,w25q64dw", "jedec,spi-nor";
>                 reg = <0>;
>                 spi-max-frequency = <25000000>;
> +               spi-rx-bus-width = <2>;

This feels wrong to me. Is your controller actually capable of "dual
SPI"? If so, why wasn't the rx-bus-width specified before? ...and if
you're truly capable of "dual SPI" then why aren't you also setting
the tx-bus-width?

My best guess (I can look up the schematic if needed) is that you're
actually _single_ lane, not dual lane SPI. Thus, a more accurate
description would probably be:

spi-rx-bus-width = <1>;
spi-tx-bus-width = <1>;

...but... I think that the default of rx/tx bus width isn't specified
is "1". Thus I think you should drop this patch.

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Hsin-Yi Wang]

On Wed, Aug 16, 2023 at 6:42 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> gd25lq64c has Quad Enable Requirement flag parsed as
> BFPT_DWORD15_QER_SR2_BIT1_BUGGY in BFPT, even if spi-{rx/tx}-bus-width
> set as non QUAD, eg. 0, 1, 2... Thus quad_enable will not be NULL and
> quad enable (QE) bit will be set to 1 by default. According to
> datasheet[1], if QE bit is enabled, WP pin will become IO pin and the
> system can't use write protection feature, and it's also not recommended
> to set QE bit to 1[1].
>
> Add a post_bfpt fixup that reads spi-rx-bus-width to remove quad_enable
> if the width is set to below QUAD mode.
>
> [1]
> https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00012-GD25LQ64C-Rev3.4.pdf
> page 13
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
change log:
v1->v2: read bus width property in driver instead of creating a
duplicate dt property.
v1 link: https://lore.kernel.org/lkml/20230815154412.713846-1-hsinyi@chromium.org/
---
>  drivers/mtd/spi-nor/gigadevice.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/gigadevice.c b/drivers/mtd/spi-nor/gigadevice.c
> index d57ddaf1525b3..8ea89e1858f9b 100644
> --- a/drivers/mtd/spi-nor/gigadevice.c
> +++ b/drivers/mtd/spi-nor/gigadevice.c
> @@ -33,6 +33,31 @@ static const struct spi_nor_fixups gd25q256_fixups = {
>         .post_bfpt = gd25q256_post_bfpt,
>  };
>
> +static int
> +gd25lq64c_post_bfpt(struct spi_nor *nor,
> +                   const struct sfdp_parameter_header *bfpt_header,
> +                   const struct sfdp_bfpt *bfpt)
> +{
> +       struct device_node *np = spi_nor_get_flash_node(nor);
> +       u32 value;
> +
> +       /*
> +        * Even if spi-{tx,rx}-bus-width is set to DUAL mode, due to the QER
> +        * flag parsed from BFPT is BFPT_DWORD15_QER_SR2_BIT1_BUGGY, so the
> +        * quad_enable will be set and QE bit set to 1.
> +        */
> +       if (!of_property_read_u32(np, "spi-rx-bus-width", &value)) {
> +               if (value <= 2)
> +                       nor->params->quad_enable = NULL;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct spi_nor_fixups gd25lq64c_fixups = {
> +       .post_bfpt = gd25lq64c_post_bfpt,
> +};
> +
>  static const struct flash_info gigadevice_nor_parts[] = {
>         { "gd25q16", INFO(0xc84015, 0, 64 * 1024,  32)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> @@ -53,7 +78,8 @@ static const struct flash_info gigadevice_nor_parts[] = {
>         { "gd25lq64c", INFO(0xc86017, 0, 64 * 1024, 128)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> -                             SPI_NOR_QUAD_READ) },
> +                             SPI_NOR_QUAD_READ)
> +               .fixups = &gd25lq64c_fixups },
>         { "gd25lq128d", INFO(0xc86018, 0, 64 * 1024, 256)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> --
> 2.41.0.694.ge786442a9b-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Michael Walle]

Am 2023-08-16 12:38, schrieb Hsin-Yi Wang:
> gd25lq64c has Quad Enable Requirement flag parsed as
> BFPT_DWORD15_QER_SR2_BIT1_BUGGY in BFPT, even if spi-{rx/tx}-bus-width
> set as non QUAD, eg. 0, 1, 2... Thus quad_enable will not be NULL and
> quad enable (QE) bit will be set to 1 by default. According to
> datasheet[1], if QE bit is enabled, WP pin will become IO pin and the
> system can't use write protection feature, and it's also not 
> recommended
> to set QE bit to 1[1].
> 
> Add a post_bfpt fixup that reads spi-rx-bus-width to remove quad_enable
> if the width is set to below QUAD mode.
> 
> [1]
> https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00012-GD25LQ64C-Rev3.4.pdf
> page 13
> 
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/mtd/spi-nor/gigadevice.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/gigadevice.c 
> b/drivers/mtd/spi-nor/gigadevice.c
> index d57ddaf1525b3..8ea89e1858f9b 100644
> --- a/drivers/mtd/spi-nor/gigadevice.c
> +++ b/drivers/mtd/spi-nor/gigadevice.c
> @@ -33,6 +33,31 @@ static const struct spi_nor_fixups gd25q256_fixups = 
> {
>  	.post_bfpt = gd25q256_post_bfpt,
>  };
> 
> +static int
> +gd25lq64c_post_bfpt(struct spi_nor *nor,
> +		    const struct sfdp_parameter_header *bfpt_header,
> +		    const struct sfdp_bfpt *bfpt)
> +{
> +	struct device_node *np = spi_nor_get_flash_node(nor);
> +	u32 value;
> +
> +	/*
> +	 * Even if spi-{tx,rx}-bus-width is set to DUAL mode, due to the QER
> +	 * flag parsed from BFPT is BFPT_DWORD15_QER_SR2_BIT1_BUGGY, so the
> +	 * quad_enable will be set and QE bit set to 1.
> +	 */
> +	if (!of_property_read_u32(np, "spi-rx-bus-width", &value)) {
> +		if (value <= 2)
> +			nor->params->quad_enable = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct spi_nor_fixups gd25lq64c_fixups = {
> +	.post_bfpt = gd25lq64c_post_bfpt,

No. Please fix it in the core and not just for this part. To me it seems
like a fundamental problem and that commit 39d1e3340c73 ("mtd: spi-nor:
Fix clearing of QE bit on lock()/unlock()") is broken in that regard.
Tudor?

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Tudor Ambarus]

On 8/16/23 12:51, Michael Walle wrote:
> Am 2023-08-16 12:38, schrieb Hsin-Yi Wang:
>> gd25lq64c has Quad Enable Requirement flag parsed as
>> BFPT_DWORD15_QER_SR2_BIT1_BUGGY in BFPT, even if spi-{rx/tx}-bus-width
>> set as non QUAD, eg. 0, 1, 2... Thus quad_enable will not be NULL and
>> quad enable (QE) bit will be set to 1 by default. According to
>> datasheet[1], if QE bit is enabled, WP pin will become IO pin and the
>> system can't use write protection feature, and it's also not recommended
>> to set QE bit to 1[1].
>>
>> Add a post_bfpt fixup that reads spi-rx-bus-width to remove quad_enable
>> if the width is set to below QUAD mode.
>>
>> [1]
>> https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00012-GD25LQ64C-Rev3.4.pdf
>> page 13
>>
>> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>> ---
>>  drivers/mtd/spi-nor/gigadevice.c | 28 +++++++++++++++++++++++++++-
>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/gigadevice.c b/drivers/mtd/spi-nor/gigadevice.c
>> index d57ddaf1525b3..8ea89e1858f9b 100644
>> --- a/drivers/mtd/spi-nor/gigadevice.c
>> +++ b/drivers/mtd/spi-nor/gigadevice.c
>> @@ -33,6 +33,31 @@ static const struct spi_nor_fixups gd25q256_fixups = {
>>      .post_bfpt = gd25q256_post_bfpt,
>>  };
>>
>> +static int
>> +gd25lq64c_post_bfpt(struct spi_nor *nor,
>> +            const struct sfdp_parameter_header *bfpt_header,
>> +            const struct sfdp_bfpt *bfpt)
>> +{
>> +    struct device_node *np = spi_nor_get_flash_node(nor);
>> +    u32 value;
>> +
>> +    /*
>> +     * Even if spi-{tx,rx}-bus-width is set to DUAL mode, due to the QER
>> +     * flag parsed from BFPT is BFPT_DWORD15_QER_SR2_BIT1_BUGGY, so the
>> +     * quad_enable will be set and QE bit set to 1.
>> +     */
>> +    if (!of_property_read_u32(np, "spi-rx-bus-width", &value)) {
>> +        if (value <= 2)
>> +            nor->params->quad_enable = NULL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static struct spi_nor_fixups gd25lq64c_fixups = {
>> +    .post_bfpt = gd25lq64c_post_bfpt,
> 
> No. Please fix it in the core and not just for this part. To me it seems

The core seems fine to me. We already adjust the hw caps by keeping just the
hardware capabilities supported by both the SPI controller and the flash,
see spi_nor_spimem_adjust_hwcaps(). If you set spi-rx-bus-width = <2>; 
(spi_nor_get_protocol_width(nor->read_proto) will be 2, thus the quad enable
method will not be called. Are you sure you don't have the quad enable bit
set by the bootloaders? Please add some prints and check whether the
quad_enable method is called or not.

> like a fundamental problem and that commit 39d1e3340c73 ("mtd: spi-nor:
> Fix clearing of QE bit on lock()/unlock()") is broken in that regard.

what's wrong with the mentioned commit?

Cheers,
ta
> Tudor?
> 
> -michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Michael Walle]

Hi,

>> like a fundamental problem and that commit 39d1e3340c73 ("mtd: 
>> spi-nor:
>> Fix clearing of QE bit on lock()/unlock()") is broken in that regard.
> 
> what's wrong with the mentioned commit?

         } else if (nor->params->quad_enable) {
                 /*
                  * If the Status Register 2 Read command (35h) is not
                  * supported, we should at least be sure we don't
                  * change the value of the SR2 Quad Enable bit.
                  *
                  * We can safely assume that when the Quad Enable method 
is
                  * set, the value of the QE bit is one, as a consequence 
of the
                  * nor->params->quad_enable() call.
                  *
                  * We can safely assume that the Quad Enable bit is 
present in
                  * the Status Register 2 at BIT(1). According to the 
JESD216
                  * revB standard, BFPT DWORDS[15], bits 22:20, the 
16-bit
                  * Write Status (01h) command is available just for the 
cases
                  * in which the QE bit is described in SR2 at BIT(1).
                  */
                 sr_cr[1] = SR2_QUAD_EN_BIT1;
         } else {
                 sr_cr[1] = 0;
         }

"We can safely assume that when the Quad Enable method..". We cannot, if 
we
don't have 4 I/O lines. The quad_enable is just the op how to do it, but 
not
*if* can do it. It seems to be missing the same check as the
spi_nor_quad_enable(). But I'm not sure if it's that simple.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Tudor Ambarus]

On 8/16/23 13:22, Michael Walle wrote:
> Hi,
> 
>>> like a fundamental problem and that commit 39d1e3340c73 ("mtd: spi-nor:
>>> Fix clearing of QE bit on lock()/unlock()") is broken in that regard.
>>
>> what's wrong with the mentioned commit?
> 
>         } else if (nor->params->quad_enable) {
>                 /*
>                  * If the Status Register 2 Read command (35h) is not
>                  * supported, we should at least be sure we don't
>                  * change the value of the SR2 Quad Enable bit.
>                  *
>                  * We can safely assume that when the Quad Enable method is
>                  * set, the value of the QE bit is one, as a consequence of the
>                  * nor->params->quad_enable() call.
>                  *
>                  * We can safely assume that the Quad Enable bit is present in
>                  * the Status Register 2 at BIT(1). According to the JESD216
>                  * revB standard, BFPT DWORDS[15], bits 22:20, the 16-bit
>                  * Write Status (01h) command is available just for the cases
>                  * in which the QE bit is described in SR2 at BIT(1).
>                  */
>                 sr_cr[1] = SR2_QUAD_EN_BIT1;
>         } else {
>                 sr_cr[1] = 0;
>         }
> 
> "We can safely assume that when the Quad Enable method..". We cannot, if we
> don't have 4 I/O lines. The quad_enable is just the op how to do it, but not
> *if* can do it. It seems to be missing the same check as the
> spi_nor_quad_enable(). But I'm not sure if it's that simple.
> 

I see. Then extending the if condition should do the trick, as
spi_nor_write_16bit_sr_and_check() is called after setup. Something
like:

if (spi_nor_get_protocol_width(nor->read_proto) == 4 &&
    spi_nor_get_protocol_width(nor->write_proto) == 4 &&
    nor->params->quad_enable)

Is this what Hsin-Yi is hitting?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Michael Walle]

Hi,

>>>> like a fundamental problem and that commit 39d1e3340c73 ("mtd: 
>>>> spi-nor:
>>>> Fix clearing of QE bit on lock()/unlock()") is broken in that 
>>>> regard.
>>> 
>>> what's wrong with the mentioned commit?
>> 
>>         } else if (nor->params->quad_enable) {
>>                 /*
>>                  * If the Status Register 2 Read command (35h) is not
>>                  * supported, we should at least be sure we don't
>>                  * change the value of the SR2 Quad Enable bit.
>>                  *
>>                  * We can safely assume that when the Quad Enable 
>> method is
>>                  * set, the value of the QE bit is one, as a 
>> consequence of the
>>                  * nor->params->quad_enable() call.
>>                  *
>>                  * We can safely assume that the Quad Enable bit is 
>> present in
>>                  * the Status Register 2 at BIT(1). According to the 
>> JESD216
>>                  * revB standard, BFPT DWORDS[15], bits 22:20, the 
>> 16-bit
>>                  * Write Status (01h) command is available just for 
>> the cases
>>                  * in which the QE bit is described in SR2 at BIT(1).
>>                  */
>>                 sr_cr[1] = SR2_QUAD_EN_BIT1;
>>         } else {
>>                 sr_cr[1] = 0;
>>         }
>> 
>> "We can safely assume that when the Quad Enable method..". We cannot, 
>> if we
>> don't have 4 I/O lines. The quad_enable is just the op how to do it, 
>> but not
>> *if* can do it. It seems to be missing the same check as the
>> spi_nor_quad_enable(). But I'm not sure if it's that simple.
>> 
> 
> I see. Then extending the if condition should do the trick, as
> spi_nor_write_16bit_sr_and_check() is called after setup. Something
> like:
> 
> if (spi_nor_get_protocol_width(nor->read_proto) == 4 &&
>     spi_nor_get_protocol_width(nor->write_proto) == 4 &&
>     nor->params->quad_enable)
> 
> Is this what Hsin-Yi is hitting?

Hopefully :)

-michael


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Hsin-Yi Wang]

On Wed, Aug 16, 2023 at 8:34 PM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 8/16/23 13:22, Michael Walle wrote:
> > Hi,
> >
> >>> like a fundamental problem and that commit 39d1e3340c73 ("mtd: spi-nor:
> >>> Fix clearing of QE bit on lock()/unlock()") is broken in that regard.
> >>
> >> what's wrong with the mentioned commit?
> >
> >         } else if (nor->params->quad_enable) {
> >                 /*
> >                  * If the Status Register 2 Read command (35h) is not
> >                  * supported, we should at least be sure we don't
> >                  * change the value of the SR2 Quad Enable bit.
> >                  *
> >                  * We can safely assume that when the Quad Enable method is
> >                  * set, the value of the QE bit is one, as a consequence of the
> >                  * nor->params->quad_enable() call.
> >                  *
> >                  * We can safely assume that the Quad Enable bit is present in
> >                  * the Status Register 2 at BIT(1). According to the JESD216
> >                  * revB standard, BFPT DWORDS[15], bits 22:20, the 16-bit
> >                  * Write Status (01h) command is available just for the cases
> >                  * in which the QE bit is described in SR2 at BIT(1).
> >                  */
> >                 sr_cr[1] = SR2_QUAD_EN_BIT1;
> >         } else {
> >                 sr_cr[1] = 0;
> >         }
> >
> > "We can safely assume that when the Quad Enable method..". We cannot, if we
> > don't have 4 I/O lines. The quad_enable is just the op how to do it, but not
> > *if* can do it. It seems to be missing the same check as the
> > spi_nor_quad_enable(). But I'm not sure if it's that simple.
> >
>
> I see. Then extending the if condition should do the trick, as
> spi_nor_write_16bit_sr_and_check() is called after setup. Something
> like:
>
> if (spi_nor_get_protocol_width(nor->read_proto) == 4 &&
>     spi_nor_get_protocol_width(nor->write_proto) == 4 &&
>     nor->params->quad_enable)
>
> Is this what Hsin-Yi is hitting?

Yes, it is. Adding these checks can solve the issue. The read out
width for both read and write is 1, which matches the default bus
width.

Thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel