| Message ID | 20191102112316.20715-14-tudor.ambarus@microchip.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: Quad Enable and (un)lock methods | expand |
On 02-Nov-19 4:53 PM, Tudor.Ambarus@microchip.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Make sure that when doing a lock() or an unlock() operation we don't clear > the QE bit from Status Register 2. > > JESD216 revB or later offers information about the *default* Status > Register commands to use (see BFPT DWORDS[15], bits 22:20). In this > standard, Status Register 1 refers to the first data byte transferred on a > Read Status (05h) or Write Status (01h) command. Status register 2 refers > to the byte read using instruction 35h. Status register 2 is the second > byte transferred in a Write Status (01h) command. > > Industry naming and definitions of these Status Registers may differ. > The definitions are described in JESD216B, BFPT DWORDS[15], bits 22:20. > There are cases in which writing only one byte to the Status Register 1 > has the side-effect of clearing Status Register 2 and implicitly the Quad > Enable bit. This side-effect is hit just by the > BFPT_DWORD15_QER_SR2_BIT1_BUGGY and BFPT_DWORD15_QER_SR2_BIT1 cases. > But I see that 1 byte SR1 write still happens as part of spi_nor_clear_sr_bp() until patch 20/20. So is this only a partial fix? Should this patch be rearranged to appear along with 20/20? > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 120 ++++++++++++++++++++++++++++++++++++++++-- > include/linux/mtd/spi-nor.h | 3 ++ > 2 files changed, 118 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 725dab241271..f96bc80c0ed1 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -959,12 +959,19 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) > return spi_nor_wait_till_ready(nor); > } > [...] > +/** > * spi_nor_write_sr2() - Write the Status Register 2 using the > * SPINOR_OP_WRSR2 (3eh) command. > * @nor: pointer to 'struct spi_nor'. > @@ -3634,19 +3723,38 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > break; > > case BFPT_DWORD15_QER_SR2_BIT1_BUGGY: > + /* > + * Writing only one byte to the Status Register has the > + * side-effect of clearing Status Register 2. > + */ > case BFPT_DWORD15_QER_SR2_BIT1_NO_RD: > + /* > + * Read Configuration Register (35h) instruction is not > + * supported. > + */ > + nor->flags |= SNOR_F_HAS_16BIT_SR | SNOR_F_NO_READ_CR; Since SNOR_F_HAS_16BIT_SR is set by default in spi_nor_info_init_params(), no need to set the flag here again > params->quad_enable = spansion_no_read_cr_quad_enable; > break; > > case BFPT_DWORD15_QER_SR1_BIT6: > + nor->flags &= ~SNOR_F_HAS_16BIT_SR; > params->quad_enable = macronix_quad_enable; > break; > > case BFPT_DWORD15_QER_SR2_BIT7: > + nor->flags &= ~SNOR_F_HAS_16BIT_SR; > params->quad_enable = sr2_bit7_quad_enable; > break; > > case BFPT_DWORD15_QER_SR2_BIT1: > + /* > + * JESD216 rev B or later does not specify if writing only one > + * byte to the Status Register clears or not the Status > + * Register 2, so let's be cautious and keep the default > + * assumption of a 16-bit Write Status (01h) command. > + */ > + nor->flags |= SNOR_F_HAS_16BIT_SR; > + Same here... > params->quad_enable = spansion_read_cr_quad_enable; > break; > > @@ -4613,6 +4721,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > params->quad_enable = spansion_read_cr_quad_enable; > params->set_4byte = spansion_set_4byte; > params->setup = spi_nor_default_setup; > + /* Default to 16-bit Write Status (01h) Command */ > + nor->flags |= SNOR_F_HAS_16BIT_SR; > > /* Set SPI NOR sizes. */ > params->size = (u64)info->sector_size * info->n_sectors; > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index d1d736d3c8ab..d6ec55cc6d97 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -243,6 +243,9 @@ enum spi_nor_option_flags { > SNOR_F_4B_OPCODES = BIT(6), > SNOR_F_HAS_4BAIT = BIT(7), > SNOR_F_HAS_LOCK = BIT(8), > + SNOR_F_HAS_16BIT_SR = BIT(9), > + SNOR_F_NO_READ_CR = BIT(10), > + > }; > > /** >
On 11/05/2019 07:07 PM, Vignesh Raghavendra wrote: > On 02-Nov-19 4:53 PM, Tudor.Ambarus@microchip.com wrote: >> From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> Make sure that when doing a lock() or an unlock() operation we don't clear >> the QE bit from Status Register 2. >> >> JESD216 revB or later offers information about the *default* Status >> Register commands to use (see BFPT DWORDS[15], bits 22:20). In this >> standard, Status Register 1 refers to the first data byte transferred on a >> Read Status (05h) or Write Status (01h) command. Status register 2 refers >> to the byte read using instruction 35h. Status register 2 is the second >> byte transferred in a Write Status (01h) command. >> >> Industry naming and definitions of these Status Registers may differ. >> The definitions are described in JESD216B, BFPT DWORDS[15], bits 22:20. >> There are cases in which writing only one byte to the Status Register 1 >> has the side-effect of clearing Status Register 2 and implicitly the Quad >> Enable bit. This side-effect is hit just by the >> BFPT_DWORD15_QER_SR2_BIT1_BUGGY and BFPT_DWORD15_QER_SR2_BIT1 cases. >> > But I see that 1 byte SR1 write still happens as part of > spi_nor_clear_sr_bp() until patch 20/20. So is this only a partial fix? Fixing spi_nor_clear_sr_bp() would mean to add dead code that will be removed anyway with patch 20/20. This patch fixes the clearing of the QE bit, while in 20/20 the QE bit is already zero when the one byte SR1 write is used, so the quad mode is not affected. 20/20 fixes indirectly the clearing of all the bits from SR2 but QE bit, because it's already zero. I would say it's not a partial fix, but a different bug. There are different angles to look at this, I chose the modifying of the quad mode angle. Given the two arguments from above (avoid adding dead code and changing of quad mode approach), I would prefer to keep things as they are. But I get your approach too, so if you still want yours, I can do it. Please let me know. > Should this patch be rearranged to appear along with 20/20? Not necessarily (different bugs) but I can bring 20/20 immediately after this one if you want. > > >> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 120 ++++++++++++++++++++++++++++++++++++++++-- >> include/linux/mtd/spi-nor.h | 3 ++ >> 2 files changed, 118 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 725dab241271..f96bc80c0ed1 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -959,12 +959,19 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) >> return spi_nor_wait_till_ready(nor); >> } >> > [...] >> +/** >> * spi_nor_write_sr2() - Write the Status Register 2 using the >> * SPINOR_OP_WRSR2 (3eh) command. >> * @nor: pointer to 'struct spi_nor'. >> @@ -3634,19 +3723,38 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, >> break; >> >> case BFPT_DWORD15_QER_SR2_BIT1_BUGGY: >> + /* >> + * Writing only one byte to the Status Register has the >> + * side-effect of clearing Status Register 2. >> + */ >> case BFPT_DWORD15_QER_SR2_BIT1_NO_RD: >> + /* >> + * Read Configuration Register (35h) instruction is not >> + * supported. >> + */ >> + nor->flags |= SNOR_F_HAS_16BIT_SR | SNOR_F_NO_READ_CR; > Since SNOR_F_HAS_16BIT_SR is set by default in > spi_nor_info_init_params(), no need to set the flag here again > I did this on purpose. I set SNOR_F_HAS_16BIT_SR here based on SFDP standard, I want to indicate where the standard requires the 16 bit SR write . spi_nor_info_init_params() initializes data based on info, but that data can be overwritten (even with the same data) when parsing SFDP. Thanks, ta
Hi, On 06/11/19 2:03 PM, Tudor.Ambarus@microchip.com wrote: > > > On 11/05/2019 07:07 PM, Vignesh Raghavendra wrote: >> On 02-Nov-19 4:53 PM, Tudor.Ambarus@microchip.com wrote: >>> From: Tudor Ambarus <tudor.ambarus@microchip.com> >>> >>> Make sure that when doing a lock() or an unlock() operation we don't clear >>> the QE bit from Status Register 2. >>> >>> JESD216 revB or later offers information about the *default* Status >>> Register commands to use (see BFPT DWORDS[15], bits 22:20). In this >>> standard, Status Register 1 refers to the first data byte transferred on a >>> Read Status (05h) or Write Status (01h) command. Status register 2 refers >>> to the byte read using instruction 35h. Status register 2 is the second >>> byte transferred in a Write Status (01h) command. >>> >>> Industry naming and definitions of these Status Registers may differ. >>> The definitions are described in JESD216B, BFPT DWORDS[15], bits 22:20. >>> There are cases in which writing only one byte to the Status Register 1 >>> has the side-effect of clearing Status Register 2 and implicitly the Quad >>> Enable bit. This side-effect is hit just by the >>> BFPT_DWORD15_QER_SR2_BIT1_BUGGY and BFPT_DWORD15_QER_SR2_BIT1 cases. >>> >> But I see that 1 byte SR1 write still happens as part of >> spi_nor_clear_sr_bp() until patch 20/20. So is this only a partial fix? > > Fixing spi_nor_clear_sr_bp() would mean to add dead code that will be removed > anyway with patch 20/20. This patch fixes the clearing of the QE bit, while in > 20/20 the QE bit is already zero when the one byte SR1 write is used, so the > quad mode is not affected. 20/20 fixes indirectly the clearing of all the bits > from SR2 but QE bit, because it's already zero. I would say it's not a partial > fix, but a different bug. > I was not suggesting on fixing up spi_nor_clear_sr_bp(), but pointing out that single byte writes SR1 can happen until patch 20/20. But now I see that these patches are fixing related but different bugs. > There are different angles to look at this, I chose the modifying of the quad > mode angle. Given the two arguments from above (avoid adding dead code and > changing of quad mode approach), I would prefer to keep things as they are. Ok, sounds fine, no need to change... > But I get your approach too, so if you still want yours, I can do it. Please let me > know. > >> Should this patch be rearranged to appear along with 20/20? > > Not necessarily (different bugs) but I can bring 20/20 immediately after this > one if you want. > >> >> >>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 120 ++++++++++++++++++++++++++++++++++++++++-- >>> include/linux/mtd/spi-nor.h | 3 ++ >>> 2 files changed, 118 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 725dab241271..f96bc80c0ed1 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -959,12 +959,19 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) >>> return spi_nor_wait_till_ready(nor); >>> } >>> >> [...] >>> +/** >>> * spi_nor_write_sr2() - Write the Status Register 2 using the >>> * SPINOR_OP_WRSR2 (3eh) command. >>> * @nor: pointer to 'struct spi_nor'. >>> @@ -3634,19 +3723,38 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, >>> break; >>> >>> case BFPT_DWORD15_QER_SR2_BIT1_BUGGY: >>> + /* >>> + * Writing only one byte to the Status Register has the >>> + * side-effect of clearing Status Register 2. >>> + */ >>> case BFPT_DWORD15_QER_SR2_BIT1_NO_RD: >>> + /* >>> + * Read Configuration Register (35h) instruction is not >>> + * supported. >>> + */ >>> + nor->flags |= SNOR_F_HAS_16BIT_SR | SNOR_F_NO_READ_CR; >> Since SNOR_F_HAS_16BIT_SR is set by default in >> spi_nor_info_init_params(), no need to set the flag here again >> > > I did this on purpose. I set SNOR_F_HAS_16BIT_SR here based on SFDP standard, I > want to indicate where the standard requires the 16 bit SR write . > spi_nor_info_init_params() initializes data based on info, but that data can be > overwritten (even with the same data) when parsing SFDP. > Alright. Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com> Regards Vignesh
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 725dab241271..f96bc80c0ed1 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -959,12 +959,19 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) return spi_nor_wait_till_ready(nor); } -/* Write status register and ensure bits in mask match written values */ -static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 status_new) +/** + * spi_nor_write_sr1_and_check() - Write one byte to the Status Register 1 and + * ensure that the byte written match the received value. + * @nor: pointer to a 'struct spi_nor'. + * @sr1: byte value to be written to the Status Register. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_write_sr1_and_check(struct spi_nor *nor, u8 sr1) { int ret; - nor->bouncebuf[0] = status_new; + nor->bouncebuf[0] = sr1; ret = spi_nor_write_sr(nor, nor->bouncebuf, 1); if (ret) @@ -974,8 +981,73 @@ static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 status_new) if (ret) return ret; - if (nor->bouncebuf[0] != status_new) { - dev_dbg(nor->dev, "SR: read back test failed\n"); + if (nor->bouncebuf[0] != sr1) { + dev_dbg(nor->dev, "SR1: read back test failed\n"); + return -EIO; + } + + return 0; +} + +/** + * spi_nor_write_16bit_sr_and_check() - Write the Status Register 1 and the + * Status Register 2 in one shot. Ensure that the byte written in the Status + * Register 1 match the received value, and that the 16-bit Write did not + * affect what was already in the Status Register 2. + * @nor: pointer to a 'struct spi_nor'. + * @sr1: byte value to be written to the Status Register 1. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1) +{ + int ret; + u8 *sr_cr = nor->bouncebuf; + u8 cr_written; + + /* Make sure we don't overwrite the contents of Status Register 2. */ + if (!(nor->flags & SNOR_F_NO_READ_CR)) { + ret = spi_nor_read_cr(nor, &sr_cr[1]); + if (ret) + return ret; + } 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] = CR_QUAD_EN_SPAN; + } else { + sr_cr[1] = 0; + } + + sr_cr[0] = sr1; + + ret = spi_nor_write_sr(nor, sr_cr, 2); + if (ret) + return ret; + + if (nor->flags & SNOR_F_NO_READ_CR) + return 0; + + cr_written = sr_cr[1]; + + ret = spi_nor_read_cr(nor, &sr_cr[1]); + if (ret) + return ret; + + if (cr_written != sr_cr[1]) { + dev_dbg(nor->dev, "CR: read back test failed\n"); return -EIO; } @@ -983,6 +1055,23 @@ static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 status_new) } /** + * spi_nor_write_sr_and_check() - Write the Status Register 1 and ensure that + * the byte written match the received value without affecting other bits in the + * Status Register 1 and 2. + * @nor: pointer to a 'struct spi_nor'. + * @sr1: byte value to be written to the Status Register. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1) +{ + if (nor->flags & SNOR_F_HAS_16BIT_SR) + return spi_nor_write_16bit_sr_and_check(nor, sr1); + + return spi_nor_write_sr1_and_check(nor, sr1); +} + +/** * spi_nor_write_sr2() - Write the Status Register 2 using the * SPINOR_OP_WRSR2 (3eh) command. * @nor: pointer to 'struct spi_nor'. @@ -3634,19 +3723,38 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, break; case BFPT_DWORD15_QER_SR2_BIT1_BUGGY: + /* + * Writing only one byte to the Status Register has the + * side-effect of clearing Status Register 2. + */ case BFPT_DWORD15_QER_SR2_BIT1_NO_RD: + /* + * Read Configuration Register (35h) instruction is not + * supported. + */ + nor->flags |= SNOR_F_HAS_16BIT_SR | SNOR_F_NO_READ_CR; params->quad_enable = spansion_no_read_cr_quad_enable; break; case BFPT_DWORD15_QER_SR1_BIT6: + nor->flags &= ~SNOR_F_HAS_16BIT_SR; params->quad_enable = macronix_quad_enable; break; case BFPT_DWORD15_QER_SR2_BIT7: + nor->flags &= ~SNOR_F_HAS_16BIT_SR; params->quad_enable = sr2_bit7_quad_enable; break; case BFPT_DWORD15_QER_SR2_BIT1: + /* + * JESD216 rev B or later does not specify if writing only one + * byte to the Status Register clears or not the Status + * Register 2, so let's be cautious and keep the default + * assumption of a 16-bit Write Status (01h) command. + */ + nor->flags |= SNOR_F_HAS_16BIT_SR; + params->quad_enable = spansion_read_cr_quad_enable; break; @@ -4613,6 +4721,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor) params->quad_enable = spansion_read_cr_quad_enable; params->set_4byte = spansion_set_4byte; params->setup = spi_nor_default_setup; + /* Default to 16-bit Write Status (01h) Command */ + nor->flags |= SNOR_F_HAS_16BIT_SR; /* Set SPI NOR sizes. */ params->size = (u64)info->sector_size * info->n_sectors; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index d1d736d3c8ab..d6ec55cc6d97 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -243,6 +243,9 @@ enum spi_nor_option_flags { SNOR_F_4B_OPCODES = BIT(6), SNOR_F_HAS_4BAIT = BIT(7), SNOR_F_HAS_LOCK = BIT(8), + SNOR_F_HAS_16BIT_SR = BIT(9), + SNOR_F_NO_READ_CR = BIT(10), + }; /**