| Message ID | 20190610062351.24405-1-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | mtd: spi-nor: use 16-bit WRR command when QE is set on spansion flashes | expand |
Hi Tudor, On 10/06/2019 08:24, Tudor.Ambarus@microchip.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > SPI memory devices from different manufacturers have widely > different configurations for Status, Control and Configuration > registers. JEDEC 216C defines a new map for these common register > bits and their functions, and describes how the individual bits may > be accessed for a specific device. For the JEDEC 216B compliant > flashes, we can partially deduce Status and Configuration registers > functions by inspecting the 16th DWORD of BFPT. Older flashes that > don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A ©11 > SPANSION) let the software decide how to interact with these registers. > > The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking") > uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set > in the bootloader. When this bit is set, only the Write Register > WRR command format with 16 data bits may be used, WRR with 8 bits > is not recognized and hence the error when trying to clear the block > protection bits. > > Fix the above by using 16-bits WRR command when Quad bit is set. > > Backward compatibility should be fine. The newly introduced > spi_nor_spansion_clear_sr_bp() is tightly coupled with the > spansion_quad_enable() function. Both assume that the Write Register > with 16 bits, together with the Read Configuration Register (35h) > instructions are supported. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > Geert, Jonas, > > This patch is compile-tested only. I don't have the flash, I need your > help for testing this. Tested this on my hardware. It works fine in the non-quad case. Tested-by: Jonas Bonn <jonas@norrbonn.se> /Jonas > > Thanks, > ta > > drivers/mtd/spi-nor/spi-nor.c | 116 ++++++++++++++++++++++++++++++++++++++---- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 106 insertions(+), 11 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index c0a8837c0575..af9ac7f09cc2 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1636,6 +1636,92 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor) > return 0; > } > > +/** > + * spi_nor_clear_sr_bp() - clear the Status Register Block Protection bits. > + * @nor: pointer to a 'struct spi_nor' > + * > + * Read-modify-write function that clears the Block Protection bits from the > + * Status Register without affecting other bits. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_clear_sr_bp(struct spi_nor *nor) > +{ > + int ret; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + ret = read_sr(nor); > + if (ret < 0) { > + dev_err(nor->dev, "error while reading status register\n"); > + return ret; > + } > + > + write_enable(nor); > + > + ret = write_sr(nor, ret & ~mask); > + if (ret) { > + dev_err(nor->dev, "write to status register failed\n"); > + return ret; > + } > + > + ret = spi_nor_wait_till_ready(nor); > + if (ret) > + dev_err(nor->dev, "timeout while writing status register\n"); > + return ret; > +} > + > +/** > + * spi_nor_spansion_clear_sr_bp() - clear the Status Register Block Protection > + * bits on spansion flashes. > + * @nor: pointer to a 'struct spi_nor' > + * > + * Read-modify-write function that clears the Block Protection bits from the > + * Status Register without affecting other bits. The function is tightly > + * coupled with the spansion_quad_enable() function. Both assume that the Write > + * Register with 16 bits, together with the Read Configuration Register (35h) > + * instructions are supported > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor) > +{ > + int ret; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + u8 sr_cr[2] = {0}; > + > + /* Check current Quad Enable bit value. */ > + ret = read_cr(nor); > + if (ret < 0) { > + dev_err(nor->dev, > + "error while reading configuration register\n"); > + return ret; > + } > + > + /* > + * When the configuration register QUAD bit CR[1] is 1, only > + * the WRR command format with 16 data bits may be used. > + */ > + if (ret & CR_QUAD_EN_SPAN) { > + sr_cr[1] = ret; > + > + ret = read_sr(nor); > + if (ret < 0) { > + dev_err(nor->dev, > + "error while reading status register\n"); > + return ret; > + } > + sr_cr[0] = ret & ~mask; > + > + ret = write_sr_cr(nor, sr_cr); > + if (ret) > + dev_err(nor->dev, "16-bit write register failed\n"); > + return ret; > + } > + > + /* If quad bit is not set, use 8-bit WRR command. */ > + return spi_nor_clear_sr_bp(nor); > +} > + > /* Used when the "_ext_id" is two bytes at most */ > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ > .id = { \ > @@ -3663,6 +3749,8 @@ static int spi_nor_init_params(struct spi_nor *nor, > default: > /* Kept only for backward compatibility purpose. */ > params->quad_enable = spansion_quad_enable; > + if (nor->clear_sr_bp) > + nor->clear_sr_bp = spi_nor_spansion_clear_sr_bp; > break; > } > > @@ -3915,17 +4003,13 @@ static int spi_nor_init(struct spi_nor *nor) > { > int err; > > - /* > - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > - * with the software protection bits set > - */ > - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || > - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > - JEDEC_MFR(nor->info) == SNOR_MFR_SST || > - nor->info->flags & SPI_NOR_HAS_LOCK) { > - write_enable(nor); > - write_sr(nor, 0); > - spi_nor_wait_till_ready(nor); > + if (nor->clear_sr_bp) { > + err = nor->clear_sr_bp(nor); > + if (err) { > + dev_err(nor->dev, > + "fail to clear block protection bits\n"); > + return err; > + } > } > > if (nor->quad_enable) { > @@ -4050,6 +4134,16 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > if (info->flags & SPI_S3AN) > nor->flags |= SNOR_F_READY_XSR_RDY; > > + /* > + * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > + * with the software protection bits set. > + */ > + if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || > + JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > + JEDEC_MFR(nor->info) == SNOR_MFR_SST || > + nor->info->flags & SPI_NOR_HAS_LOCK) > + nor->clear_sr_bp = spi_nor_clear_sr_bp; > + > /* Parse the Serial Flash Discoverable Parameters table. */ > ret = spi_nor_init_params(nor, ¶ms); > if (ret) > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index b3d360b0ee3d..566bd5010bc8 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -410,6 +410,7 @@ struct spi_nor { > int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); > int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); > int (*quad_enable)(struct spi_nor *nor); > + int (*clear_sr_bp)(struct spi_nor *nor); > > void *priv; > }; >
Hi Tudor, On Mon, Jun 10, 2019 at 8:24 AM <Tudor.Ambarus@microchip.com> wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > SPI memory devices from different manufacturers have widely > different configurations for Status, Control and Configuration > registers. JEDEC 216C defines a new map for these common register > bits and their functions, and describes how the individual bits may > be accessed for a specific device. For the JEDEC 216B compliant > flashes, we can partially deduce Status and Configuration registers > functions by inspecting the 16th DWORD of BFPT. Older flashes that > don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A ©11 > SPANSION) let the software decide how to interact with these registers. > > The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking") > uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set > in the bootloader. When this bit is set, only the Write Register > WRR command format with 16 data bits may be used, WRR with 8 bits > is not recognized and hence the error when trying to clear the block > protection bits. > > Fix the above by using 16-bits WRR command when Quad bit is set. > > Backward compatibility should be fine. The newly introduced > spi_nor_spansion_clear_sr_bp() is tightly coupled with the > spansion_quad_enable() function. Both assume that the Write Register > with 16 bits, together with the Read Configuration Register (35h) > instructions are supported. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > Geert, Jonas, > > This patch is compile-tested only. I don't have the flash, I need your > help for testing this. Thanks, this revives access to the s25fl512s on Koelsch. Fixes: dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region locking") Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>Hi Tudor, Two questions below... > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor) > +{ [...] > + * When the configuration register QUAD bit CR[1] is 1, only > + * the WRR command format with 16 data bits may be used. s/WRR/WRSR/? > + */ > + if (ret & CR_QUAD_EN_SPAN) { > + sr_cr[1] = ret; > + > + ret = read_sr(nor); > + if (ret < 0) { > + dev_err(nor->dev, > + "error while reading status register\n"); > + return ret; > + } > + sr_cr[0] = ret & ~mask; > + > + ret = write_sr_cr(nor, sr_cr); > + if (ret) > + dev_err(nor->dev, "16-bit write register failed\n"); > + return ret; > + } > + > + /* If quad bit is not set, use 8-bit WRR command. */ Likewise. > + return spi_nor_clear_sr_bp(nor); > +} Gr{oetje,eeting}s, Geert
Hi, On 10-Jun-19 11:54 AM, Tudor.Ambarus@microchip.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > SPI memory devices from different manufacturers have widely > different configurations for Status, Control and Configuration > registers. JEDEC 216C defines a new map for these common register > bits and their functions, and describes how the individual bits may > be accessed for a specific device. For the JEDEC 216B compliant > flashes, we can partially deduce Status and Configuration registers > functions by inspecting the 16th DWORD of BFPT. Older flashes that > don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A ©11 > SPANSION) let the software decide how to interact with these registers. > > The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking") > uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set > in the bootloader. When this bit is set, only the Write Register > WRR command format with 16 data bits may be used, WRR with 8 bits > is not recognized and hence the error when trying to clear the block > protection bits. > I see above text in s25fl512s datasheet[1] as well. So the patch is indeed needed: Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com> Also, I was able to reproduce above scenario with TI's K2G EVM with s25fl512s flash and can confirm that this patch fixes the issue: Tested-by: Vignesh Raghavendra <vigneshr@ti.com> Regards Vignesh [1] https://www.cypress.com/file/177971/download 9.3.7 Write Registers > Fix the above by using 16-bits WRR command when Quad bit is set. > > Backward compatibility should be fine. The newly introduced > spi_nor_spansion_clear_sr_bp() is tightly coupled with the > spansion_quad_enable() function. Both assume that the Write Register > with 16 bits, together with the Read Configuration Register (35h) > instructions are supported. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > Geert, Jonas, > > This patch is compile-tested only. I don't have the flash, I need your > help for testing this. > > Thanks, > ta > > drivers/mtd/spi-nor/spi-nor.c | 116 ++++++++++++++++++++++++++++++++++++++---- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 106 insertions(+), 11 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index c0a8837c0575..af9ac7f09cc2 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1636,6 +1636,92 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor) > return 0; > } > > +/** > + * spi_nor_clear_sr_bp() - clear the Status Register Block Protection bits. > + * @nor: pointer to a 'struct spi_nor' > + * > + * Read-modify-write function that clears the Block Protection bits from the > + * Status Register without affecting other bits. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_clear_sr_bp(struct spi_nor *nor) > +{ > + int ret; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + ret = read_sr(nor); > + if (ret < 0) { > + dev_err(nor->dev, "error while reading status register\n"); > + return ret; > + } > + > + write_enable(nor); > + > + ret = write_sr(nor, ret & ~mask); > + if (ret) { > + dev_err(nor->dev, "write to status register failed\n"); > + return ret; > + } > + > + ret = spi_nor_wait_till_ready(nor); > + if (ret) > + dev_err(nor->dev, "timeout while writing status register\n"); > + return ret; > +} > + > +/** > + * spi_nor_spansion_clear_sr_bp() - clear the Status Register Block Protection > + * bits on spansion flashes. > + * @nor: pointer to a 'struct spi_nor' > + * > + * Read-modify-write function that clears the Block Protection bits from the > + * Status Register without affecting other bits. The function is tightly > + * coupled with the spansion_quad_enable() function. Both assume that the Write > + * Register with 16 bits, together with the Read Configuration Register (35h) > + * instructions are supported > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor) > +{ > + int ret; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + u8 sr_cr[2] = {0}; > + > + /* Check current Quad Enable bit value. */ > + ret = read_cr(nor); > + if (ret < 0) { > + dev_err(nor->dev, > + "error while reading configuration register\n"); > + return ret; > + } > + > + /* > + * When the configuration register QUAD bit CR[1] is 1, only > + * the WRR command format with 16 data bits may be used. > + */ > + if (ret & CR_QUAD_EN_SPAN) { > + sr_cr[1] = ret; > + > + ret = read_sr(nor); > + if (ret < 0) { > + dev_err(nor->dev, > + "error while reading status register\n"); > + return ret; > + } > + sr_cr[0] = ret & ~mask; > + > + ret = write_sr_cr(nor, sr_cr); > + if (ret) > + dev_err(nor->dev, "16-bit write register failed\n"); > + return ret; > + } > + > + /* If quad bit is not set, use 8-bit WRR command. */ > + return spi_nor_clear_sr_bp(nor); > +} > + > /* Used when the "_ext_id" is two bytes at most */ > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ > .id = { \ > @@ -3663,6 +3749,8 @@ static int spi_nor_init_params(struct spi_nor *nor, > default: > /* Kept only for backward compatibility purpose. */ > params->quad_enable = spansion_quad_enable; > + if (nor->clear_sr_bp) > + nor->clear_sr_bp = spi_nor_spansion_clear_sr_bp; > break; > } > > @@ -3915,17 +4003,13 @@ static int spi_nor_init(struct spi_nor *nor) > { > int err; > > - /* > - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > - * with the software protection bits set > - */ > - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || > - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > - JEDEC_MFR(nor->info) == SNOR_MFR_SST || > - nor->info->flags & SPI_NOR_HAS_LOCK) { > - write_enable(nor); > - write_sr(nor, 0); > - spi_nor_wait_till_ready(nor); > + if (nor->clear_sr_bp) { > + err = nor->clear_sr_bp(nor); > + if (err) { > + dev_err(nor->dev, > + "fail to clear block protection bits\n"); > + return err; > + } > } > > if (nor->quad_enable) { > @@ -4050,6 +4134,16 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > if (info->flags & SPI_S3AN) > nor->flags |= SNOR_F_READY_XSR_RDY; > > + /* > + * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > + * with the software protection bits set. > + */ > + if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || > + JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > + JEDEC_MFR(nor->info) == SNOR_MFR_SST || > + nor->info->flags & SPI_NOR_HAS_LOCK) > + nor->clear_sr_bp = spi_nor_clear_sr_bp; > + > /* Parse the Serial Flash Discoverable Parameters table. */ > ret = spi_nor_init_params(nor, ¶ms); > if (ret) > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index b3d360b0ee3d..566bd5010bc8 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -410,6 +410,7 @@ struct spi_nor { > int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); > int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); > int (*quad_enable)(struct spi_nor *nor); > + int (*clear_sr_bp)(struct spi_nor *nor); > > void *priv; > }; >
Hi, Geert, On 06/11/2019 11:35 AM, Geert Uytterhoeven wrote: > Hi Tudor, > > On Mon, Jun 10, 2019 at 8:24 AM <Tudor.Ambarus@microchip.com> wrote: >> From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> SPI memory devices from different manufacturers have widely >> different configurations for Status, Control and Configuration >> registers. JEDEC 216C defines a new map for these common register >> bits and their functions, and describes how the individual bits may >> be accessed for a specific device. For the JEDEC 216B compliant >> flashes, we can partially deduce Status and Configuration registers >> functions by inspecting the 16th DWORD of BFPT. Older flashes that >> don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A ©11 >> SPANSION) let the software decide how to interact with these registers. >> >> The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking") >> uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set >> in the bootloader. When this bit is set, only the Write Register >> WRR command format with 16 data bits may be used, WRR with 8 bits >> is not recognized and hence the error when trying to clear the block >> protection bits. >> >> Fix the above by using 16-bits WRR command when Quad bit is set. >> >> Backward compatibility should be fine. The newly introduced >> spi_nor_spansion_clear_sr_bp() is tightly coupled with the >> spansion_quad_enable() function. Both assume that the Write Register >> with 16 bits, together with the Read Configuration Register (35h) >> instructions are supported. >> >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> Geert, Jonas, >> >> This patch is compile-tested only. I don't have the flash, I need your >> help for testing this. > > Thanks, this revives access to the s25fl512s on Koelsch. > > Fixes: dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region locking") I didn't add the Fixes tag because this commit helped us discover a case that has not been taken into consideration before. It didn't introduce a bug, but rather revealed one. However, it's not the time to walk over this thin line, so I'll add it, thanks! > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>Hi Tudor, > > Two questions below... > >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor) >> +{ > > [...] > >> + * When the configuration register QUAD bit CR[1] is 1, only >> + * the WRR command format with 16 data bits may be used. > > s/WRR/WRSR/? S25FL512S named it "Write Registers" command and chose the "WRR" acronym. JESD216D names it "Write Register" command and doesn't suggest an acronym. I'll s/"WRR"/"Write Register command", to use the JESD216D naming and avoid confusion. I also forgot to describe int (*clear_sr_bp), v2 will follow. Will keep the R-b and T-b tags since I'll just update comments. ta
Hi Tudor, On Wed, Jun 19, 2019 at 5:47 PM <Tudor.Ambarus@microchip.com> wrote: > On 06/11/2019 11:35 AM, Geert Uytterhoeven wrote: > > On Mon, Jun 10, 2019 at 8:24 AM <Tudor.Ambarus@microchip.com> wrote: > >> From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> > >> SPI memory devices from different manufacturers have widely > >> different configurations for Status, Control and Configuration > >> registers. JEDEC 216C defines a new map for these common register > >> bits and their functions, and describes how the individual bits may > >> be accessed for a specific device. For the JEDEC 216B compliant > >> flashes, we can partially deduce Status and Configuration registers > >> functions by inspecting the 16th DWORD of BFPT. Older flashes that > >> don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A ©11 > >> SPANSION) let the software decide how to interact with these registers. > >> > >> The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking") > >> uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set > >> in the bootloader. When this bit is set, only the Write Register > >> WRR command format with 16 data bits may be used, WRR with 8 bits > >> is not recognized and hence the error when trying to clear the block > >> protection bits. > >> > >> Fix the above by using 16-bits WRR command when Quad bit is set. > >> > >> Backward compatibility should be fine. The newly introduced > >> spi_nor_spansion_clear_sr_bp() is tightly coupled with the > >> spansion_quad_enable() function. Both assume that the Write Register > >> with 16 bits, together with the Read Configuration Register (35h) > >> instructions are supported. > >> > >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > >> --- > >> Geert, Jonas, > >> > >> This patch is compile-tested only. I don't have the flash, I need your > >> help for testing this. > > > > Thanks, this revives access to the s25fl512s on Koelsch. > > > > Fixes: dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region locking") > > I didn't add the Fixes tag because this commit helped us discover a case that > has not been taken into consideration before. It didn't introduce a bug, but > rather revealed one. However, it's not the time to walk over this thin line, so > I'll add it, thanks! Good. Fixes also serves as an indicator that if dcb4b22eeaf44f91 is backported to stable, the "fix" must be backported, too. > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>Hi Tudor, > > > > Two questions below... > > > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > > > >> +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor) > >> +{ > > > > [...] > > > >> + * When the configuration register QUAD bit CR[1] is 1, only > >> + * the WRR command format with 16 data bits may be used. > > > > s/WRR/WRSR/? > > S25FL512S named it "Write Registers" command and chose the "WRR" acronym. > JESD216D names it "Write Register" command and doesn't suggest an acronym. I'll > s/"WRR"/"Write Register command", to use the JESD216D naming and avoid confusion. > > I also forgot to describe int (*clear_sr_bp), v2 will follow. Will keep the R-b > and T-b tags since I'll just update comments. OK. Thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index c0a8837c0575..af9ac7f09cc2 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1636,6 +1636,92 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor) return 0; } +/** + * spi_nor_clear_sr_bp() - clear the Status Register Block Protection bits. + * @nor: pointer to a 'struct spi_nor' + * + * Read-modify-write function that clears the Block Protection bits from the + * Status Register without affecting other bits. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_clear_sr_bp(struct spi_nor *nor) +{ + int ret; + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + ret = read_sr(nor); + if (ret < 0) { + dev_err(nor->dev, "error while reading status register\n"); + return ret; + } + + write_enable(nor); + + ret = write_sr(nor, ret & ~mask); + if (ret) { + dev_err(nor->dev, "write to status register failed\n"); + return ret; + } + + ret = spi_nor_wait_till_ready(nor); + if (ret) + dev_err(nor->dev, "timeout while writing status register\n"); + return ret; +} + +/** + * spi_nor_spansion_clear_sr_bp() - clear the Status Register Block Protection + * bits on spansion flashes. + * @nor: pointer to a 'struct spi_nor' + * + * Read-modify-write function that clears the Block Protection bits from the + * Status Register without affecting other bits. The function is tightly + * coupled with the spansion_quad_enable() function. Both assume that the Write + * Register with 16 bits, together with the Read Configuration Register (35h) + * instructions are supported + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor) +{ + int ret; + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + u8 sr_cr[2] = {0}; + + /* Check current Quad Enable bit value. */ + ret = read_cr(nor); + if (ret < 0) { + dev_err(nor->dev, + "error while reading configuration register\n"); + return ret; + } + + /* + * When the configuration register QUAD bit CR[1] is 1, only + * the WRR command format with 16 data bits may be used. + */ + if (ret & CR_QUAD_EN_SPAN) { + sr_cr[1] = ret; + + ret = read_sr(nor); + if (ret < 0) { + dev_err(nor->dev, + "error while reading status register\n"); + return ret; + } + sr_cr[0] = ret & ~mask; + + ret = write_sr_cr(nor, sr_cr); + if (ret) + dev_err(nor->dev, "16-bit write register failed\n"); + return ret; + } + + /* If quad bit is not set, use 8-bit WRR command. */ + return spi_nor_clear_sr_bp(nor); +} + /* Used when the "_ext_id" is two bytes at most */ #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ .id = { \ @@ -3663,6 +3749,8 @@ static int spi_nor_init_params(struct spi_nor *nor, default: /* Kept only for backward compatibility purpose. */ params->quad_enable = spansion_quad_enable; + if (nor->clear_sr_bp) + nor->clear_sr_bp = spi_nor_spansion_clear_sr_bp; break; } @@ -3915,17 +4003,13 @@ static int spi_nor_init(struct spi_nor *nor) { int err; - /* - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up - * with the software protection bits set - */ - if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || - JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || - JEDEC_MFR(nor->info) == SNOR_MFR_SST || - nor->info->flags & SPI_NOR_HAS_LOCK) { - write_enable(nor); - write_sr(nor, 0); - spi_nor_wait_till_ready(nor); + if (nor->clear_sr_bp) { + err = nor->clear_sr_bp(nor); + if (err) { + dev_err(nor->dev, + "fail to clear block protection bits\n"); + return err; + } } if (nor->quad_enable) { @@ -4050,6 +4134,16 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (info->flags & SPI_S3AN) nor->flags |= SNOR_F_READY_XSR_RDY; + /* + * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up + * with the software protection bits set. + */ + if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL || + JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || + JEDEC_MFR(nor->info) == SNOR_MFR_SST || + nor->info->flags & SPI_NOR_HAS_LOCK) + nor->clear_sr_bp = spi_nor_clear_sr_bp; + /* Parse the Serial Flash Discoverable Parameters table. */ ret = spi_nor_init_params(nor, ¶ms); if (ret) diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index b3d360b0ee3d..566bd5010bc8 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -410,6 +410,7 @@ struct spi_nor { int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*quad_enable)(struct spi_nor *nor); + int (*clear_sr_bp)(struct spi_nor *nor); void *priv; };