| Message ID | 20230816104245.2676965-1-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width | expand |
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 >
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
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
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
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?
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
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.
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 |
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(-)