| Message ID | 20190320071605.4289-3-jonas@norrbonn.se |
|---|---|
| State | Accepted |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | spi-nor block protection | expand |
On 03/20/2019 09:16 AM, Jonas Bonn wrote: > External E-Mail > > > Both the BP[0-2] bits and the TBPROT bit are supported on this chip. > Tested and verified on a Cypress s25fl512s. > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 0cc0ef4fbf3a..9abac9e326a1 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { > SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, > + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, > { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, > { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, >
On 03/20/2019 09:16 AM, Jonas Bonn wrote: > Both the BP[0-2] bits and the TBPROT bit are supported on this chip. > Tested and verified on a Cypress s25fl512s. > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > --- > drivers/mtd/spi-nor/spi-nor.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Applied to http://git.infradead.org/linux-mtd.git, spi-nor/next. Thanks.
Hi Jonas, On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: > Both the BP[0-2] bits and the TBPROT bit are supported on this chip. > Tested and verified on a Cypress s25fl512s. > > Signed-off-by: Jonas Bonn <jonas@norrbonn.se> This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region locking") in mtd/next. > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { > SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, > + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail probing. Before/after: -m25p80 spi0.0: s25fl512s (65536 Kbytes) -3 fixed-partitions partitions found on MTD device spi0.0 -Creating 3 MTD partitions on "spi0.0": -0x000000000000-0x000000080000 : "loader" -0x000000080000-0x000000600000 : "user" -0x000000600000-0x000004000000 : "flash" +m25p80 spi0.0: Erase Error occurred +m25p80 spi0.0: Erase Error occurred +m25p80 spi0.0: timeout while writing configuration register +m25p80 spi0.0: quad mode not supported +m25p80: probe of spi0.0 failed with error -5 FLASH chip is SPANSION FL512SAIFG1 311QQ063 A ©11 SPANSION JEDEC id bytes: 01 02 20 4d 00 80 Gr{oetje,eeting}s, Geert
Hi, Geert, On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote: > External E-Mail > > > Hi Jonas, > > On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: >> Both the BP[0-2] bits and the TBPROT bit are supported on this chip. >> Tested and verified on a Cypress s25fl512s. >> >> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > > This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region > locking") in mtd/next. > >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { >> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, >> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, >> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, > > Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail > probing. > > Before/after: > > -m25p80 spi0.0: s25fl512s (65536 Kbytes) > -3 fixed-partitions partitions found on MTD device spi0.0 > -Creating 3 MTD partitions on "spi0.0": > -0x000000000000-0x000000080000 : "loader" > -0x000000080000-0x000000600000 : "user" > -0x000000600000-0x000004000000 : "flash" > +m25p80 spi0.0: Erase Error occurred > +m25p80 spi0.0: Erase Error occurred > +m25p80 spi0.0: timeout while writing configuration register > +m25p80 spi0.0: quad mode not supported > +m25p80: probe of spi0.0 failed with error -5 > > FLASH chip is SPANSION FL512SAIFG1 311QQ063 A ©11 SPANSION > JEDEC id bytes: 01 02 20 4d 00 80 That's curious. Did you revert this patch and probe was ok? Are you sure it is not related to the recent changes on spi-rspi.c? Cheers, ta
Hi Tudor, On Tue, May 7, 2019 at 12:42 PM <Tudor.Ambarus@microchip.com> wrote: > On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote: > > On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: > >> Both the BP[0-2] bits and the TBPROT bit are supported on this chip. > >> Tested and verified on a Cypress s25fl512s. > >> > >> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > > > > This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region > > locking") in mtd/next. > > > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { > >> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > >> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > >> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > >> - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > >> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, > >> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > >> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, > > > > Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail > > probing. > > > > Before/after: > > > > -m25p80 spi0.0: s25fl512s (65536 Kbytes) > > -3 fixed-partitions partitions found on MTD device spi0.0 > > -Creating 3 MTD partitions on "spi0.0": > > -0x000000000000-0x000000080000 : "loader" > > -0x000000080000-0x000000600000 : "user" > > -0x000000600000-0x000004000000 : "flash" > > +m25p80 spi0.0: Erase Error occurred > > +m25p80 spi0.0: Erase Error occurred > > +m25p80 spi0.0: timeout while writing configuration register > > +m25p80 spi0.0: quad mode not supported > > +m25p80: probe of spi0.0 failed with error -5 > > > > FLASH chip is SPANSION FL512SAIFG1 311QQ063 A ©11 SPANSION > > JEDEC id bytes: 01 02 20 4d 00 80 > > That's curious. Did you revert this patch and probe was ok? Are you sure it is Yes, a revert fixes the the probe. I had tried all combinations of the 2 newly set flags, and only SPI_NOR_HAS_LOCK is a problem. > not related to the recent changes on spi-rspi.c? These changes affect QSPI transfers only. Gr{oetje,eeting}s, Geert
Hi Geert, On 07/05/2019 12:50, Geert Uytterhoeven wrote: > Hi Tudor, > > On Tue, May 7, 2019 at 12:42 PM <Tudor.Ambarus@microchip.com> wrote: >> On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote: >>> On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: >>>> Both the BP[0-2] bits and the TBPROT bit are supported on this chip. >>>> Tested and verified on a Cypress s25fl512s. >>>> >>>> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> >>> >>> This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region >>> locking") in mtd/next. >>> >>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>> @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { >>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, >>>> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>> - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, >>>> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >>>> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, >>> >>> Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail >>> probing. >>> >>> Before/after: >>> >>> -m25p80 spi0.0: s25fl512s (65536 Kbytes) >>> -3 fixed-partitions partitions found on MTD device spi0.0 >>> -Creating 3 MTD partitions on "spi0.0": >>> -0x000000000000-0x000000080000 : "loader" >>> -0x000000080000-0x000000600000 : "user" >>> -0x000000600000-0x000004000000 : "flash" >>> +m25p80 spi0.0: Erase Error occurred >>> +m25p80 spi0.0: Erase Error occurred >>> +m25p80 spi0.0: timeout while writing configuration register >>> +m25p80 spi0.0: quad mode not supported >>> +m25p80: probe of spi0.0 failed with error -5 >>> In drivers/mtd/spi-nor/spi-nor.c you have: 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->quad_enable) { err = nor->quad_enable(nor); if (err) { dev_err(nor->dev, "quad mode not supported\n"); return err; } } This is the only meaningful thing that I can see may have changed with this flag. We now have an additional write_enable before quad_enable. What happens if you swap those two blocks above so that quad_enable is called first? If it's not that, I can't see how the extra flags can have any effect. Regards, Jonas
Hi Jonas, On Tue, May 7, 2019 at 1:14 PM Jonas Bonn <jonas@norrbonn.se> wrote: > On 07/05/2019 12:50, Geert Uytterhoeven wrote: > > On Tue, May 7, 2019 at 12:42 PM <Tudor.Ambarus@microchip.com> wrote: > >> On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote: > >>> On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: > >>>> Both the BP[0-2] bits and the TBPROT bit are supported on this chip. > >>>> Tested and verified on a Cypress s25fl512s. > >>>> > >>>> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> > >>> > >>> This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region > >>> locking") in mtd/next. > >>> > >>>> --- a/drivers/mtd/spi-nor/spi-nor.c > >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c > >>>> @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { > >>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > >>>> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, > >>>> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > >>>> - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, > >>>> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, > >>>> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > >>>> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, > >>> > >>> Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail > >>> probing. > >>> > >>> Before/after: > >>> > >>> -m25p80 spi0.0: s25fl512s (65536 Kbytes) > >>> -3 fixed-partitions partitions found on MTD device spi0.0 > >>> -Creating 3 MTD partitions on "spi0.0": > >>> -0x000000000000-0x000000080000 : "loader" > >>> -0x000000080000-0x000000600000 : "user" > >>> -0x000000600000-0x000004000000 : "flash" > >>> +m25p80 spi0.0: Erase Error occurred > >>> +m25p80 spi0.0: Erase Error occurred > >>> +m25p80 spi0.0: timeout while writing configuration register > >>> +m25p80 spi0.0: quad mode not supported > >>> +m25p80: probe of spi0.0 failed with error -5 > >>> > > In drivers/mtd/spi-nor/spi-nor.c you have: > > 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); With more debug prints: m25p80 spi0.0: spi_nor_init:3925: write_enable() returned 0 > write_sr(nor, 0); m25p80 spi0.0: spi_nor_init:3927: write_sr() returned 0 > spi_nor_wait_till_ready(nor); m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 ... m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff m25p80 spi0.0: Erase Error occurred m25p80 spi0.0: spi_nor_init:3929: spi_nor_wait_till_ready() returned -5 > } > > if (nor->quad_enable) { > err = nor->quad_enable(nor); m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff m25p80 spi0.0: Erase Error occurred m25p80 spi0.0: timeout while writing configuration register m25p80 spi0.0: spi_nor_init:3937: spansion_quad_enable() returned -5 > if (err) { > dev_err(nor->dev, "quad mode not supported\n"); > return err; > } > } > > This is the only meaningful thing that I can see may have changed with > this flag. We now have an additional write_enable before quad_enable. > What happens if you swap those two blocks above so that quad_enable is > called first? With the two blocks swapped: m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x0 m25p80 spi0.0: spi_nor_init:3919: spansion_quad_enable() returned 0 m25p80 spi0.0: spi_nor_init:3937: write_enable() returned 0 m25p80 spi0.0: spi_nor_init:3939: write_sr() returned 0 m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 ... m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff m25p80 spi0.0: Erase Error occurred m25p80 spi0.0: spi_nor_init:3941: spi_nor_wait_till_ready() returned -5 m25p80 spi0.0: s25fl512s (65536 Kbytes) 3 fixed-partitions partitions found on MTD device spi0.0 Creating 3 MTD partitions on "spi0.0": 0x000000000000-0x000000080000 : "loader" 0x000000080000-0x000000600000 : "user" 0x000000600000-0x000004000000 : "flash" Note that spi_nor_wait_till_ready() still fails. While the device (which contains the boot loader) now appears to be detected fine, reading returns bogus repetitive data, for all partitions: # hd /dev/mtd0 | head 00000000 33 33 33 33 33 33 33 33 33 33 3b bb ff ff ff ff |3333333333;.....| 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| * 00001000 33 33 33 33 33 33 33 33 33 33 3b bb ff ff ff ff |3333333333;.....| 00001010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| * If I remove the call to "write_sr(nor, 0)", everything works as before, regardless of swapping the blocks. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Geert, On 05/07/2019 03:52 PM, Geert Uytterhoeven wrote: > External E-Mail > > > Hi Jonas, > > On Tue, May 7, 2019 at 1:14 PM Jonas Bonn <jonas@norrbonn.se> wrote: >> On 07/05/2019 12:50, Geert Uytterhoeven wrote: >>> On Tue, May 7, 2019 at 12:42 PM <Tudor.Ambarus@microchip.com> wrote: >>>> On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote: >>>>> On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: >>>>>> Both the BP[0-2] bits and the TBPROT bit are supported on this chip. >>>>>> Tested and verified on a Cypress s25fl512s. >>>>>> >>>>>> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> >>>>> >>>>> This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region >>>>> locking") in mtd/next. >>>>> >>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>>>> @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { >>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>>>> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, >>>>>> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>>>> - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>>>> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, >>>>>> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >>>>>> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, >>>>> >>>>> Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail >>>>> probing. >>>>> >>>>> Before/after: >>>>> >>>>> -m25p80 spi0.0: s25fl512s (65536 Kbytes) >>>>> -3 fixed-partitions partitions found on MTD device spi0.0 >>>>> -Creating 3 MTD partitions on "spi0.0": >>>>> -0x000000000000-0x000000080000 : "loader" >>>>> -0x000000080000-0x000000600000 : "user" >>>>> -0x000000600000-0x000004000000 : "flash" >>>>> +m25p80 spi0.0: Erase Error occurred >>>>> +m25p80 spi0.0: Erase Error occurred >>>>> +m25p80 spi0.0: timeout while writing configuration register >>>>> +m25p80 spi0.0: quad mode not supported >>>>> +m25p80: probe of spi0.0 failed with error -5 >>>>> >> >> In drivers/mtd/spi-nor/spi-nor.c you have: >> >> 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); > > With more debug prints: > > m25p80 spi0.0: spi_nor_init:3925: write_enable() returned 0 > >> write_sr(nor, 0); > > m25p80 spi0.0: spi_nor_init:3927: write_sr() returned 0 > >> spi_nor_wait_till_ready(nor); > > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 > ... > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff > m25p80 spi0.0: Erase Error occurred > m25p80 spi0.0: spi_nor_init:3929: spi_nor_wait_till_ready() returned -5 > >> } >> >> if (nor->quad_enable) { >> err = nor->quad_enable(nor); > > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff > m25p80 spi0.0: Erase Error occurred > m25p80 spi0.0: timeout while writing configuration register > m25p80 spi0.0: spi_nor_init:3937: spansion_quad_enable() returned -5 > >> if (err) { >> dev_err(nor->dev, "quad mode not supported\n"); >> return err; >> } >> } >> >> This is the only meaningful thing that I can see may have changed with >> this flag. We now have an additional write_enable before quad_enable. >> What happens if you swap those two blocks above so that quad_enable is >> called first? > > With the two blocks swapped: > > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x0 > m25p80 spi0.0: spi_nor_init:3919: spansion_quad_enable() returned 0 > m25p80 spi0.0: spi_nor_init:3937: write_enable() returned 0 > m25p80 spi0.0: spi_nor_init:3939: write_sr() returned 0 > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 > ... > m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff > m25p80 spi0.0: Erase Error occurred > m25p80 spi0.0: spi_nor_init:3941: spi_nor_wait_till_ready() returned -5 > m25p80 spi0.0: s25fl512s (65536 Kbytes) > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000080000 : "loader" > 0x000000080000-0x000000600000 : "user" > 0x000000600000-0x000004000000 : "flash" > > Note that spi_nor_wait_till_ready() still fails. > > While the device (which contains the boot loader) now appears to be > detected fine, reading returns bogus repetitive data, for all partitions: > > # hd /dev/mtd0 | head > 00000000 33 33 33 33 33 33 33 33 33 33 3b bb ff ff ff ff > |3333333333;.....| > 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > |................| > * > 00001000 33 33 33 33 33 33 33 33 33 33 3b bb ff ff ff ff > |3333333333;.....| > 00001010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > |................| > * > > If I remove the call to "write_sr(nor, 0)", everything works as > before, regardless > of swapping the blocks. > Can you try this one? diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 73172d7f512b..b94a6eaaaca5 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1551,6 +1551,7 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) u8 sr_cr[2]; int ret; + dev_err(dev, "%s\n", __FUNCTION__); /* Check current Quad Enable bit value. */ ret = read_cr(nor); if (ret < 0) { @@ -3911,6 +3912,12 @@ static int spi_nor_setup(struct spi_nor *nor, static int spi_nor_init(struct spi_nor *nor) { int err; + u8 val; + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + /* Check current Quad Enable bit value. */ + val = read_cr(nor); + dev_err(nor->dev, "%s val = %02x\n", val); /* * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up @@ -3921,7 +3928,7 @@ static int spi_nor_init(struct spi_nor *nor) JEDEC_MFR(nor->info) == SNOR_MFR_SST || nor->info->flags & SPI_NOR_HAS_LOCK) { write_enable(nor); - write_sr(nor, 0); + write_sr(nor, val & ~mask); spi_nor_wait_till_ready(nor); }
On 05/07/2019 04:15 PM, Tudor Ambarus wrote: > Geert, > > On 05/07/2019 03:52 PM, Geert Uytterhoeven wrote: >> External E-Mail >> >> >> Hi Jonas, >> >> On Tue, May 7, 2019 at 1:14 PM Jonas Bonn <jonas@norrbonn.se> wrote: >>> On 07/05/2019 12:50, Geert Uytterhoeven wrote: >>>> On Tue, May 7, 2019 at 12:42 PM <Tudor.Ambarus@microchip.com> wrote: >>>>> On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote: >>>>>> On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@norrbonn.se> wrote: >>>>>>> Both the BP[0-2] bits and the TBPROT bit are supported on this chip. >>>>>>> Tested and verified on a Cypress s25fl512s. >>>>>>> >>>>>>> Signed-off-by: Jonas Bonn <jonas@norrbonn.se> >>>>>> >>>>>> This is now commit dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region >>>>>> locking") in mtd/next. >>>>>> >>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>>>>> @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { >>>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>>>>> { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, >>>>>>> { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>>>>> - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, >>>>>>> + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, >>>>>>> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >>>>>>> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, >>>>>> >>>>>> Setting SPI_NOR_HAS_LOCK causes the QSPI FLASH on r8a7791/koelsch to fail >>>>>> probing. >>>>>> >>>>>> Before/after: >>>>>> >>>>>> -m25p80 spi0.0: s25fl512s (65536 Kbytes) >>>>>> -3 fixed-partitions partitions found on MTD device spi0.0 >>>>>> -Creating 3 MTD partitions on "spi0.0": >>>>>> -0x000000000000-0x000000080000 : "loader" >>>>>> -0x000000080000-0x000000600000 : "user" >>>>>> -0x000000600000-0x000004000000 : "flash" >>>>>> +m25p80 spi0.0: Erase Error occurred >>>>>> +m25p80 spi0.0: Erase Error occurred >>>>>> +m25p80 spi0.0: timeout while writing configuration register >>>>>> +m25p80 spi0.0: quad mode not supported >>>>>> +m25p80: probe of spi0.0 failed with error -5 >>>>>> >>> >>> In drivers/mtd/spi-nor/spi-nor.c you have: >>> >>> 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); >> >> With more debug prints: >> >> m25p80 spi0.0: spi_nor_init:3925: write_enable() returned 0 >> >>> write_sr(nor, 0); >> >> m25p80 spi0.0: spi_nor_init:3927: write_sr() returned 0 >> >>> spi_nor_wait_till_ready(nor); >> >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 >> ... >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff >> m25p80 spi0.0: Erase Error occurred >> m25p80 spi0.0: spi_nor_init:3929: spi_nor_wait_till_ready() returned -5 >> >>> } >>> >>> if (nor->quad_enable) { >>> err = nor->quad_enable(nor); >> >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff >> m25p80 spi0.0: Erase Error occurred >> m25p80 spi0.0: timeout while writing configuration register >> m25p80 spi0.0: spi_nor_init:3937: spansion_quad_enable() returned -5 >> >>> if (err) { >>> dev_err(nor->dev, "quad mode not supported\n"); >>> return err; >>> } >>> } >>> >>> This is the only meaningful thing that I can see may have changed with >>> this flag. We now have an additional write_enable before quad_enable. >>> What happens if you swap those two blocks above so that quad_enable is >>> called first? >> >> With the two blocks swapped: >> >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x0 >> m25p80 spi0.0: spi_nor_init:3919: spansion_quad_enable() returned 0 >> m25p80 spi0.0: spi_nor_init:3937: write_enable() returned 0 >> m25p80 spi0.0: spi_nor_init:3939: write_sr() returned 0 >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0x3 >> ... >> m25p80 spi0.0: spi_nor_sr_ready:533: sr = 0xff >> m25p80 spi0.0: Erase Error occurred >> m25p80 spi0.0: spi_nor_init:3941: spi_nor_wait_till_ready() returned -5 >> m25p80 spi0.0: s25fl512s (65536 Kbytes) >> 3 fixed-partitions partitions found on MTD device spi0.0 >> Creating 3 MTD partitions on "spi0.0": >> 0x000000000000-0x000000080000 : "loader" >> 0x000000080000-0x000000600000 : "user" >> 0x000000600000-0x000004000000 : "flash" >> >> Note that spi_nor_wait_till_ready() still fails. >> >> While the device (which contains the boot loader) now appears to be >> detected fine, reading returns bogus repetitive data, for all partitions: >> >> # hd /dev/mtd0 | head >> 00000000 33 33 33 33 33 33 33 33 33 33 3b bb ff ff ff ff >> |3333333333;.....| >> 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >> |................| >> * >> 00001000 33 33 33 33 33 33 33 33 33 33 3b bb ff ff ff ff >> |3333333333;.....| >> 00001010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >> |................| >> * >> >> If I remove the call to "write_sr(nor, 0)", everything works as >> before, regardless >> of swapping the blocks. >> > > Can you try this one? > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 73172d7f512b..b94a6eaaaca5 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1551,6 +1551,7 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) > u8 sr_cr[2]; > int ret; > > + dev_err(dev, "%s\n", __FUNCTION__); > /* Check current Quad Enable bit value. */ > ret = read_cr(nor); > if (ret < 0) { > @@ -3911,6 +3912,12 @@ static int spi_nor_setup(struct spi_nor *nor, > static int spi_nor_init(struct spi_nor *nor) > { > int err; > + u8 val; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + /* Check current Quad Enable bit value. */ > + val = read_cr(nor); > + dev_err(nor->dev, "%s val = %02x\n", val); dev_err(nor->dev, "%s val = %02x\n", __FUNCTION__, val); > > /* > * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > @@ -3921,7 +3928,7 @@ static int spi_nor_init(struct spi_nor *nor) > JEDEC_MFR(nor->info) == SNOR_MFR_SST || > nor->info->flags & SPI_NOR_HAS_LOCK) { > write_enable(nor); > - write_sr(nor, 0); > + write_sr(nor, val & ~mask); > spi_nor_wait_till_ready(nor); > } > >
On 05/07/2019 04:15 PM, Tudor Ambarus wrote: > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 73172d7f512b..b94a6eaaaca5 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1551,6 +1551,7 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) > u8 sr_cr[2]; > int ret; > > + dev_err(dev, "%s\n", __FUNCTION__); > /* Check current Quad Enable bit value. */ > ret = read_cr(nor); > if (ret < 0) { > @@ -3911,6 +3912,12 @@ static int spi_nor_setup(struct spi_nor *nor, > static int spi_nor_init(struct spi_nor *nor) > { > int err; > + u8 val; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + /* Check current Quad Enable bit value. */ > + val = read_cr(nor); this should have been: val = read_sr(nor); sorry. > + dev_err(nor->dev, "%s val = %02x\n", val); and here dev_err(nor->dev, "%s val = %02x\n", __FUNCTION__, val); > > /* > * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > @@ -3921,7 +3928,7 @@ static int spi_nor_init(struct spi_nor *nor) > JEDEC_MFR(nor->info) == SNOR_MFR_SST || > nor->info->flags & SPI_NOR_HAS_LOCK) { > write_enable(nor); > - write_sr(nor, 0); > + write_sr(nor, val & ~mask); > spi_nor_wait_till_ready(nor); > } >
Hi Tudor, On Tue, May 7, 2019 at 3:25 PM <Tudor.Ambarus@microchip.com> wrote: > On 05/07/2019 04:15 PM, Tudor Ambarus wrote: > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 73172d7f512b..b94a6eaaaca5 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -1551,6 +1551,7 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) > > u8 sr_cr[2]; > > int ret; > > > > + dev_err(dev, "%s\n", __FUNCTION__); This one isn't triggered. > > /* Check current Quad Enable bit value. */ > > ret = read_cr(nor); > > if (ret < 0) { > > @@ -3911,6 +3912,12 @@ static int spi_nor_setup(struct spi_nor *nor, > > static int spi_nor_init(struct spi_nor *nor) > > { > > int err; > > + u8 val; > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > + > > + /* Check current Quad Enable bit value. */ > > + val = read_cr(nor); > this should have been: > val = read_sr(nor); > sorry. > > > + dev_err(nor->dev, "%s val = %02x\n", val); > and here > dev_err(nor->dev, "%s val = %02x\n", __FUNCTION__, val); m25p80 spi0.0: spi_nor_init val = 00 so the masking trick doesn't help :-( Gr{oetje,eeting}s, Geert
Geert,
Would you run this debug patch on top of mtd/next? I dumped the SR and CR before
and after the operations in cause.
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 73172d7f512b..20d0fdb1dc92 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -22,6 +22,8 @@
#include <linux/spi/flash.h>
#include <linux/mtd/spi-nor.h>
+#define DEBUG
+
/* Define max times to check status register before we give up. */
/*
@@ -200,7 +202,7 @@ struct sfdp_header {
* register does not modify status register 2.
* - 101b: QE is bit 1 of status register 2. Status register 1 is read using
* Read Status instruction 05h. Status register2 is read using
- * instruction 35h. QE is set via Writ Status instruction 01h with
+ * instruction 35h. QE is set via Write Status instruction 01h with
* two data bytes where bit 1 of the second byte is one.
* [...]
*/
@@ -2795,8 +2797,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
return err;
/* Fix endianness of the BFPT DWORDs. */
- for (i = 0; i < BFPT_DWORD_MAX; i++)
+ for (i = 0; i < BFPT_DWORD_MAX; i++) {
bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+ dev_dbg(nor->dev, "bfpt.dwords[%d] = %08x\n", i + 1,
+ bfpt.dwords[i]);
+ }
/* Number of address bytes. */
switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
@@ -3532,8 +3537,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
}
err = spi_nor_parse_bfpt(nor, bfpt_header, params);
- if (err)
+ if (err) {
+ dev_dbg(dev, "failed to parse BFPT: err = %d\n", err);
goto exit;
+ }
/* Parse optional parameter tables. */
for (i = 0; i < header.nph; i++) {
@@ -3576,6 +3583,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
struct spi_nor_erase_map *map = &nor->erase_map;
const struct flash_info *info = nor->info;
u8 i, erase_mask;
+ int ret;
/* Set legacy flash parameters as default. */
memset(params, 0, sizeof(*params));
@@ -3681,12 +3689,15 @@ static int spi_nor_init_params(struct spi_nor *nor,
memcpy(&sfdp_params, params, sizeof(sfdp_params));
memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
- if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
+ ret = spi_nor_parse_sfdp(nor, &sfdp_params);
+ if (ret) {
nor->addr_width = 0;
nor->flags &= ~SNOR_F_4B_OPCODES;
/* restore previous erase map */
memcpy(&nor->erase_map, &prev_map,
sizeof(nor->erase_map));
+ dev_dbg(nor->dev, "%s sfdp parse failed, ret =%d\n",
+ __FUNCTION__, ret);
} else {
memcpy(params, &sfdp_params, sizeof(*params));
}
@@ -3908,9 +3919,67 @@ static int spi_nor_setup(struct spi_nor *nor,
return 0;
}
+static int spi_nor_dump_sr_cr(struct spi_nor *nor)
+{
+ int ret = read_sr(nor);
+
+ dev_dbg(nor->dev, "SR = %08x\n", ret);
+ if (ret < 0) {
+ dev_err(nor->dev, "error while reading status register\n");
+ return ret;
+ }
+
+ ret = read_cr(nor);
+ dev_dbg(nor->dev, "CR = %08x\n", ret);
+ if (ret < 0) {
+ dev_err(nor->dev, "error while reading configuration register\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int spi_nor_clear_block_protection(struct spi_nor *nor)
+{
+ int ret;
+ u8 val, mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+ ret = spi_nor_dump_sr_cr(nor);
+ if (ret)
+ return ret;
+
+ /* clear just the BP bits */
+ ret = read_sr(nor);
+ if (ret < 0) {
+ dev_err(nor->dev, "error while reading status register\n");
+ return ret;
+ }
+ val = ret;
+
+ ret = write_enable(nor);
+ if (ret < 0) {
+ dev_err(nor->dev, "error on write enable, err = %d\n", ret);
+ return ret;
+ }
+
+ ret = write_sr(nor, val & ~mask);
+ if (ret < 0) {
+ dev_err(nor->dev, "error on write_sr, err = %d\n", ret);
+ return ret;
+ }
+
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret) {
+ dev_err(nor->dev, "timeout while writing SR, ret = %d\n", ret);
+ return spi_nor_dump_sr_cr(nor);
+ }
+
+ return 0;
+}
+
static int spi_nor_init(struct spi_nor *nor)
{
- int err;
+ int err = 0, quad_err;
/*
* Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
@@ -3919,18 +3988,38 @@ static int spi_nor_init(struct spi_nor *nor)
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);
+ nor->info->flags & SPI_NOR_HAS_LOCK)
+ /*
+ * this should be done only on demand, not for every flash that
+ * has SPI_NOR_HAS_LOCK set
+ */
+ err = spi_nor_clear_block_protection(nor);
+ if (err) {
+ dev_err(nor->dev, "clearing BP bits failed, err = %d\n", err);
+ return err;
}
if (nor->quad_enable) {
- err = nor->quad_enable(nor);
+ dev_dbg(nor->dev, "SR and CR before quad_enable:\n");
+
+ err = spi_nor_dump_sr_cr(nor);
if (err) {
- dev_err(nor->dev, "quad mode not supported\n");
+ dev_err(nor->dev, "dump_sr_cr before quad enable fail, err = %d\n", err);
return err;
}
+
+ quad_err = nor->quad_enable(nor);
+ dev_dbg(nor->dev, "SR and CR after quad_enable:\n");
+ err = spi_nor_dump_sr_cr(nor);
+ if (err) {
+ dev_err(nor->dev, "dump_sr_cr after quad enable fail, err = %d\n", err);
+ return err;
+ }
+
+ if (quad_err) {
+ dev_err(nor->dev, "quad mode not supported, err = %d\n", quad_err);
+ return quad_err;
+ }
}
if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
Hi Tudor, On Wed, May 8, 2019 at 12:44 PM <Tudor.Ambarus@microchip.com> wrote: > Would you run this debug patch on top of mtd/next? I dumped the SR and CR before > and after the operations in cause. Thanks, giving it a try... > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 73172d7f512b..20d0fdb1dc92 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -22,6 +22,8 @@ > #include <linux/spi/flash.h> > #include <linux/mtd/spi-nor.h> > > +#define DEBUG Should be moved to the top of the file, before dev_dbg() is defined. Result is: m25p80 spi0.0: bfpt.dwords[1] = ffffffff m25p80 spi0.0: bfpt.dwords[2] = ffffffff m25p80 spi0.0: bfpt.dwords[3] = ffffffff m25p80 spi0.0: bfpt.dwords[4] = ffffffff m25p80 spi0.0: bfpt.dwords[5] = ffffffff m25p80 spi0.0: bfpt.dwords[6] = ffffffff m25p80 spi0.0: bfpt.dwords[7] = ffffffff m25p80 spi0.0: bfpt.dwords[8] = ffffffff m25p80 spi0.0: bfpt.dwords[9] = ffffffff m25p80 spi0.0: bfpt.dwords[10] = 00000000 m25p80 spi0.0: bfpt.dwords[11] = 00000000 m25p80 spi0.0: bfpt.dwords[12] = 00000000 m25p80 spi0.0: bfpt.dwords[13] = 00000000 m25p80 spi0.0: bfpt.dwords[14] = 00000000 m25p80 spi0.0: bfpt.dwords[15] = 00000000 m25p80 spi0.0: bfpt.dwords[16] = 00000000 m25p80 spi0.0: failed to parse BFPT: err = -22 m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 m25p80 spi0.0: SR = 00000000 m25p80 spi0.0: CR = 00000002 m25p80 spi0.0: Erase Error occurred m25p80 spi0.0: timeout while writing SR, ret = -5 m25p80 spi0.0: SR = 000000ff m25p80 spi0.0: CR = 000000ff m25p80 spi0.0: SR and CR before quad_enable: m25p80 spi0.0: SR = 000000ff m25p80 spi0.0: CR = 000000ff m25p80 spi0.0: Erase Error occurred m25p80 spi0.0: timeout while writing configuration register m25p80 spi0.0: SR and CR after quad_enable: m25p80 spi0.0: SR = 000000ff m25p80 spi0.0: CR = 000000ff m25p80 spi0.0: quad mode not supported, err = -5 m25p80: probe of spi0.0 failed with error -5 Gr{oetje,eeting}s, Geert
Hi, Geert, On 05/08/2019 04:11 PM, Geert Uytterhoeven wrote: > External E-Mail > > > Hi Tudor, > > On Wed, May 8, 2019 at 12:44 PM <Tudor.Ambarus@microchip.com> wrote: >> Would you run this debug patch on top of mtd/next? I dumped the SR and CR before >> and after the operations in cause. > > Thanks, giving it a try... > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 73172d7f512b..20d0fdb1dc92 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -22,6 +22,8 @@ >> #include <linux/spi/flash.h> >> #include <linux/mtd/spi-nor.h> >> >> +#define DEBUG > > Should be moved to the top of the file, before dev_dbg() is defined. > > Result is: > > m25p80 spi0.0: bfpt.dwords[1] = ffffffff > m25p80 spi0.0: bfpt.dwords[2] = ffffffff > m25p80 spi0.0: bfpt.dwords[3] = ffffffff > m25p80 spi0.0: bfpt.dwords[4] = ffffffff > m25p80 spi0.0: bfpt.dwords[5] = ffffffff > m25p80 spi0.0: bfpt.dwords[6] = ffffffff > m25p80 spi0.0: bfpt.dwords[7] = ffffffff > m25p80 spi0.0: bfpt.dwords[8] = ffffffff > m25p80 spi0.0: bfpt.dwords[9] = ffffffff > m25p80 spi0.0: bfpt.dwords[10] = 00000000 > m25p80 spi0.0: bfpt.dwords[11] = 00000000 > m25p80 spi0.0: bfpt.dwords[12] = 00000000 > m25p80 spi0.0: bfpt.dwords[13] = 00000000 > m25p80 spi0.0: bfpt.dwords[14] = 00000000 > m25p80 spi0.0: bfpt.dwords[15] = 00000000 > m25p80 spi0.0: bfpt.dwords[16] = 00000000 > m25p80 spi0.0: failed to parse BFPT: err = -22 > m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 > m25p80 spi0.0: SR = 00000000 > m25p80 spi0.0: CR = 00000002 > m25p80 spi0.0: Erase Error occurred > m25p80 spi0.0: timeout while writing SR, ret = -5 > m25p80 spi0.0: SR = 000000ff > m25p80 spi0.0: CR = 000000ff ff can mean that the lines are "in air", maybe the flash resets when we write_sr(nor, 0)? How about adding a delay here to let the flash reset? SR=0 and CR=2 after reset, both write_sr(nor, 0) and quad_enable can be avoided -> read SR, clear BP bits only if they are set to 1, read CR -> set Quad Enable bit only when it's zero. Cheers, ta > m25p80 spi0.0: SR and CR before quad_enable: > m25p80 spi0.0: SR = 000000ff > m25p80 spi0.0: CR = 000000ff > m25p80 spi0.0: Erase Error occurred > m25p80 spi0.0: timeout while writing configuration register > m25p80 spi0.0: SR and CR after quad_enable: > m25p80 spi0.0: SR = 000000ff > m25p80 spi0.0: CR = 000000ff > m25p80 spi0.0: quad mode not supported, err = -5 > m25p80: probe of spi0.0 failed with error -5 > > Gr{oetje,eeting}s, > > Geert >
Hi Tudor, On Wed, May 8, 2019 at 4:23 PM <Tudor.Ambarus@microchip.com> wrote: > On 05/08/2019 04:11 PM, Geert Uytterhoeven wrote: > > On Wed, May 8, 2019 at 12:44 PM <Tudor.Ambarus@microchip.com> wrote: > >> Would you run this debug patch on top of mtd/next? I dumped the SR and CR before > >> and after the operations in cause. > > > > Thanks, giving it a try... > > > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > >> index 73172d7f512b..20d0fdb1dc92 100644 > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -22,6 +22,8 @@ > >> #include <linux/spi/flash.h> > >> #include <linux/mtd/spi-nor.h> > >> > >> +#define DEBUG > > > > Should be moved to the top of the file, before dev_dbg() is defined. > > > > Result is: > > > > m25p80 spi0.0: bfpt.dwords[1] = ffffffff > > m25p80 spi0.0: bfpt.dwords[2] = ffffffff > > m25p80 spi0.0: bfpt.dwords[3] = ffffffff > > m25p80 spi0.0: bfpt.dwords[4] = ffffffff > > m25p80 spi0.0: bfpt.dwords[5] = ffffffff > > m25p80 spi0.0: bfpt.dwords[6] = ffffffff > > m25p80 spi0.0: bfpt.dwords[7] = ffffffff > > m25p80 spi0.0: bfpt.dwords[8] = ffffffff > > m25p80 spi0.0: bfpt.dwords[9] = ffffffff > > m25p80 spi0.0: bfpt.dwords[10] = 00000000 > > m25p80 spi0.0: bfpt.dwords[11] = 00000000 > > m25p80 spi0.0: bfpt.dwords[12] = 00000000 > > m25p80 spi0.0: bfpt.dwords[13] = 00000000 > > m25p80 spi0.0: bfpt.dwords[14] = 00000000 > > m25p80 spi0.0: bfpt.dwords[15] = 00000000 > > m25p80 spi0.0: bfpt.dwords[16] = 00000000 > > m25p80 spi0.0: failed to parse BFPT: err = -22 > > m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 > > m25p80 spi0.0: SR = 00000000 > > m25p80 spi0.0: CR = 00000002 > > m25p80 spi0.0: Erase Error occurred > > m25p80 spi0.0: timeout while writing SR, ret = -5 > > m25p80 spi0.0: SR = 000000ff > > m25p80 spi0.0: CR = 000000ff > > ff can mean that the lines are "in air", maybe the flash resets when we > write_sr(nor, 0)? How about adding a delay here to let the flash reset? No difference after adding msleep(1000). Gr{oetje,eeting}s, Geert
Hi, Geert, On 05/08/2019 08:00 PM, Geert Uytterhoeven wrote: > External E-Mail > > > Hi Tudor,> > On Wed, May 8, 2019 at 4:23 PM <Tudor.Ambarus@microchip.com> wrote: >> On 05/08/2019 04:11 PM, Geert Uytterhoeven wrote: >>> On Wed, May 8, 2019 at 12:44 PM <Tudor.Ambarus@microchip.com> wrote: >>>> Would you run this debug patch on top of mtd/next? I dumped the SR and CR before >>>> and after the operations in cause. >>> >>> Thanks, giving it a try... >>> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>>> index 73172d7f512b..20d0fdb1dc92 100644 >>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>> @@ -22,6 +22,8 @@ >>>> #include <linux/spi/flash.h> >>>> #include <linux/mtd/spi-nor.h> >>>> >>>> +#define DEBUG >>> >>> Should be moved to the top of the file, before dev_dbg() is defined. >>> >>> Result is: >>> >>> m25p80 spi0.0: bfpt.dwords[1] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[2] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[3] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[4] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[5] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[6] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[7] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[8] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[9] = ffffffff >>> m25p80 spi0.0: bfpt.dwords[10] = 00000000 >>> m25p80 spi0.0: bfpt.dwords[11] = 00000000 >>> m25p80 spi0.0: bfpt.dwords[12] = 00000000 >>> m25p80 spi0.0: bfpt.dwords[13] = 00000000 >>> m25p80 spi0.0: bfpt.dwords[14] = 00000000 >>> m25p80 spi0.0: bfpt.dwords[15] = 00000000 >>> m25p80 spi0.0: bfpt.dwords[16] = 00000000 >>> m25p80 spi0.0: failed to parse BFPT: err = -22 >>> m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 >>> m25p80 spi0.0: SR = 00000000 >>> m25p80 spi0.0: CR = 00000002 >>> m25p80 spi0.0: Erase Error occurred >>> m25p80 spi0.0: timeout while writing SR, ret = -5 >>> m25p80 spi0.0: SR = 000000ff >>> m25p80 spi0.0: CR = 000000ff >> >> ff can mean that the lines are "in air", maybe the flash resets when we >> write_sr(nor, 0)? How about adding a delay here to let the flash reset? > > No difference after adding msleep(1000). > When the configuration register QUAD bit CR[1] is 1, only the WRR command format with 16 data bits may be used, WRR with 8 bits is not recognized and hence the FFs. You probably set quad bit in u-boot, while others don't. We can verify this assumption with the patch form below. Can you try it? diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 73172d7f512b..0d636eab3ebf 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -6,6 +6,7 @@ * Copyright (C) 2005, Intec Automation Inc. * Copyright (C) 2014, Freescale Semiconductor, Inc. */ +#define DEBUG #include <linux/err.h> #include <linux/errno.h> @@ -200,7 +201,7 @@ struct sfdp_header { * register does not modify status register 2. * - 101b: QE is bit 1 of status register 2. Status register 1 is read using * Read Status instruction 05h. Status register2 is read using - * instruction 35h. QE is set via Writ Status instruction 01h with + * instruction 35h. QE is set via Write Status instruction 01h with * two data bytes where bit 1 of the second byte is one. * [...] */ @@ -2795,8 +2796,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, return err; /* Fix endianness of the BFPT DWORDs. */ - for (i = 0; i < BFPT_DWORD_MAX; i++) + for (i = 0; i < BFPT_DWORD_MAX; i++) { bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]); + dev_dbg(nor->dev, "bfpt.dwords[%d] = %08x\n", i + 1, + bfpt.dwords[i]); + } /* Number of address bytes. */ switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { @@ -3532,8 +3536,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor, } err = spi_nor_parse_bfpt(nor, bfpt_header, params); - if (err) + if (err) { + dev_dbg(dev, "failed to parse BFPT: err = %d\n", err); goto exit; + } /* Parse optional parameter tables. */ for (i = 0; i < header.nph; i++) { @@ -3576,6 +3582,7 @@ static int spi_nor_init_params(struct spi_nor *nor, struct spi_nor_erase_map *map = &nor->erase_map; const struct flash_info *info = nor->info; u8 i, erase_mask; + int ret; /* Set legacy flash parameters as default. */ memset(params, 0, sizeof(*params)); @@ -3681,12 +3688,15 @@ static int spi_nor_init_params(struct spi_nor *nor, memcpy(&sfdp_params, params, sizeof(sfdp_params)); memcpy(&prev_map, &nor->erase_map, sizeof(prev_map)); - if (spi_nor_parse_sfdp(nor, &sfdp_params)) { + ret = spi_nor_parse_sfdp(nor, &sfdp_params); + if (ret) { nor->addr_width = 0; nor->flags &= ~SNOR_F_4B_OPCODES; /* restore previous erase map */ memcpy(&nor->erase_map, &prev_map, sizeof(nor->erase_map)); + dev_dbg(nor->dev, "%s sfdp parse failed, ret =%d\n", + __FUNCTION__, ret); } else { memcpy(params, &sfdp_params, sizeof(*params)); } @@ -3908,9 +3918,86 @@ static int spi_nor_setup(struct spi_nor *nor, return 0; } +static int spi_nor_dump_sr_cr(struct spi_nor *nor) +{ + int ret = read_sr(nor); + + dev_dbg(nor->dev, "SR = %08x\n", ret); + if (ret < 0) { + dev_err(nor->dev, "error while reading status register\n"); + return ret; + } + + ret = read_cr(nor); + dev_dbg(nor->dev, "CR = %08x\n", ret); + if (ret < 0) { + dev_err(nor->dev, "error while reading configuration register\n"); + return ret; + } + + return 0; +} + +static int spi_nor_clear_block_protection(struct spi_nor *nor) +{ + int ret; + u8 sr, cr, sr_cr[2] = {0}; + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + ret = read_cr(nor); + dev_dbg(nor->dev, "CR = %08x\n", ret); + if (ret < 0) { + dev_err(nor->dev, "error while reading CR\n"); + return ret; + } + cr = ret; + + if (cr & CR_QUAD_EN_SPAN) { + /* disable quad if already set, must do it with 16-bit WRR */ + ret = write_sr_cr(nor, sr_cr); + if (ret) { + dev_err(nor->dev, "error diasbling quad mode\n"); + return ret; + } + + /* read back and check it */ + ret = read_cr(nor); + if (ret >= 0 && (ret & CR_QUAD_EN_SPAN)) { + dev_err(nor->dev, "Spansion Quad bit not cleared\n"); + return -EINVAL; + } + } + + /* Clear BP bits with 8-bit WRR */ + ret = read_sr(nor); + dev_dbg(nor->dev, "SR = %08x\n", ret); + if (ret < 0) { + dev_err(nor->dev, "error while reading SR\n"); + return ret; + } + sr = ret; + + ret = write_enable(nor); + if (ret < 0) { + dev_err(nor->dev, "error on write enable, err = %d\n", ret); + return ret; + } + + ret = write_sr(nor, sr & ~mask); + if (ret < 0) { + dev_err(nor->dev, "error on write_sr, err = %d\n", ret); + return ret; + } + + ret = spi_nor_wait_till_ready(nor); + if (ret) + dev_err(nor->dev, "timeout while writing SR, ret = %d\n", ret); + return spi_nor_dump_sr_cr(nor); +} + static int spi_nor_init(struct spi_nor *nor) { - int err; + int err = 0, quad_err; /* * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up @@ -3919,18 +4006,38 @@ static int spi_nor_init(struct spi_nor *nor) 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); + nor->info->flags & SPI_NOR_HAS_LOCK) + /* + * this should be done only on demand, not for every flash that + * has SPI_NOR_HAS_LOCK set + */ + err = spi_nor_clear_block_protection(nor); + if (err) { + dev_err(nor->dev, "clearing BP bits failed, err = %d\n", err); + return err; } if (nor->quad_enable) { - err = nor->quad_enable(nor); + dev_dbg(nor->dev, "SR and CR before quad_enable:\n"); + + err = spi_nor_dump_sr_cr(nor); if (err) { - dev_err(nor->dev, "quad mode not supported\n"); + dev_err(nor->dev, "dump_sr_cr before quad enable fail, err = %d\n", err); return err; } + + quad_err = nor->quad_enable(nor); + dev_dbg(nor->dev, "SR and CR after quad_enable:\n"); + err = spi_nor_dump_sr_cr(nor); + if (err) { + dev_err(nor->dev, "dump_sr_cr after quad enable fail, err = %d\n", err); + return err; + } + + if (quad_err) { + dev_err(nor->dev, "quad mode not supported, err = %d\n", quad_err); + return quad_err; + } } if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
Hi Tudor, On Thu, May 9, 2019 at 8:56 AM <Tudor.Ambarus@microchip.com> wrote: > When the configuration register QUAD bit CR[1] is 1, only the WRR command format > with 16 data bits may be used, WRR with 8 bits is not recognized and hence the > FFs. You probably set quad bit in u-boot, while others don't. We can verify this > assumption with the patch form below. Can you try it? Thanks! > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > +static int spi_nor_clear_block_protection(struct spi_nor *nor) > +{ > + int ret; > + u8 sr, cr, sr_cr[2] = {0}; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + ret = read_cr(nor); > + dev_dbg(nor->dev, "CR = %08x\n", ret); > + if (ret < 0) { > + dev_err(nor->dev, "error while reading CR\n"); > + return ret; > + } > + cr = ret; > + > + if (cr & CR_QUAD_EN_SPAN) { > + /* disable quad if already set, must do it with 16-bit WRR */ > + ret = write_sr_cr(nor, sr_cr); > + if (ret) { > + dev_err(nor->dev, "error diasbling quad mode\n"); disabling > + return ret; > + } renesas_spi e6b10000.spi: DMA available renesas_spi e6b10000.spi: registered master spi0 spi spi0.0: setup mode 3, 8 bits/w, 30000000 Hz max --> 0 m25p80 spi0.0: bfpt.dwords[1] = ffffffff m25p80 spi0.0: bfpt.dwords[2] = ffffffff m25p80 spi0.0: bfpt.dwords[3] = ffffffff m25p80 spi0.0: bfpt.dwords[4] = ffffffff m25p80 spi0.0: bfpt.dwords[5] = ffffffff m25p80 spi0.0: bfpt.dwords[6] = ffffffff m25p80 spi0.0: bfpt.dwords[7] = ffffffff m25p80 spi0.0: bfpt.dwords[8] = ffffffff m25p80 spi0.0: bfpt.dwords[9] = ffffffff m25p80 spi0.0: bfpt.dwords[10] = 00000000 m25p80 spi0.0: bfpt.dwords[11] = 00000000 m25p80 spi0.0: bfpt.dwords[12] = 00000000 m25p80 spi0.0: bfpt.dwords[13] = 00000000 m25p80 spi0.0: bfpt.dwords[14] = 00000000 m25p80 spi0.0: bfpt.dwords[15] = 00000000 m25p80 spi0.0: bfpt.dwords[16] = 00000000 m25p80 spi0.0: failed to parse BFPT: err = -22 m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 m25p80 spi0.0: SR and CR before quad_enable: m25p80 spi0.0: SR = 00000000 m25p80 spi0.0: CR = 00000002 m25p80 spi0.0: SR and CR after quad_enable: m25p80 spi0.0: SR = 00000000 m25p80 spi0.0: CR = 00000002 m25p80 spi0.0: s25fl512s (65536 Kbytes) m25p80 spi0.0: mtd .name = spi0.0, .size = 0x4000000 (64MiB), .erasesize = 0x00040000 (256KiB) .numeraseregions = 0 3 fixed-partitions partitions found on MTD device spi0.0 Creating 3 MTD partitions on "spi0.0": 0x000000000000-0x000000080000 : "loader" 0x000000080000-0x000000600000 : "user" 0x000000600000-0x000004000000 : "flash" renesas_spi e6b10000.spi: registered child spi0.0 renesas_spi e6b10000.spi: probed And /dev/mtd0 reading works fine. Thanks! Gr{oetje,eeting}s, Geert
Hi, Geert, On 05/09/2019 12:11 PM, Geert Uytterhoeven wrote: > External E-Mail > > > Hi Tudor, > > On Thu, May 9, 2019 at 8:56 AM <Tudor.Ambarus@microchip.com> wrote: >> When the configuration register QUAD bit CR[1] is 1, only the WRR command format >> with 16 data bits may be used, WRR with 8 bits is not recognized and hence the >> FFs. You probably set quad bit in u-boot, while others don't. We can verify this >> assumption with the patch form below. Can you try it? > > Thanks! > >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> +static int spi_nor_clear_block_protection(struct spi_nor *nor) >> +{ >> + int ret; >> + u8 sr, cr, sr_cr[2] = {0}; >> + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; >> + >> + ret = read_cr(nor); >> + dev_dbg(nor->dev, "CR = %08x\n", ret); >> + if (ret < 0) { >> + dev_err(nor->dev, "error while reading CR\n"); >> + return ret; >> + } >> + cr = ret; >> + >> + if (cr & CR_QUAD_EN_SPAN) { >> + /* disable quad if already set, must do it with 16-bit WRR */ >> + ret = write_sr_cr(nor, sr_cr); >> + if (ret) { >> + dev_err(nor->dev, "error diasbling quad mode\n"); > > disabling > >> + return ret; >> + } > > renesas_spi e6b10000.spi: DMA available > renesas_spi e6b10000.spi: registered master spi0 > spi spi0.0: setup mode 3, 8 bits/w, 30000000 Hz max --> 0 > m25p80 spi0.0: bfpt.dwords[1] = ffffffff > m25p80 spi0.0: bfpt.dwords[2] = ffffffff > m25p80 spi0.0: bfpt.dwords[3] = ffffffff > m25p80 spi0.0: bfpt.dwords[4] = ffffffff > m25p80 spi0.0: bfpt.dwords[5] = ffffffff > m25p80 spi0.0: bfpt.dwords[6] = ffffffff > m25p80 spi0.0: bfpt.dwords[7] = ffffffff > m25p80 spi0.0: bfpt.dwords[8] = ffffffff > m25p80 spi0.0: bfpt.dwords[9] = ffffffff > m25p80 spi0.0: bfpt.dwords[10] = 00000000 > m25p80 spi0.0: bfpt.dwords[11] = 00000000 > m25p80 spi0.0: bfpt.dwords[12] = 00000000 > m25p80 spi0.0: bfpt.dwords[13] = 00000000 > m25p80 spi0.0: bfpt.dwords[14] = 00000000 > m25p80 spi0.0: bfpt.dwords[15] = 00000000 > m25p80 spi0.0: bfpt.dwords[16] = 00000000 > m25p80 spi0.0: failed to parse BFPT: err = -22 > m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 > m25p80 spi0.0: SR and CR before quad_enable: > m25p80 spi0.0: SR = 00000000 > m25p80 spi0.0: CR = 00000002 > m25p80 spi0.0: SR and CR after quad_enable: > m25p80 spi0.0: SR = 00000000 > m25p80 spi0.0: CR = 00000002 > m25p80 spi0.0: s25fl512s (65536 Kbytes) > m25p80 spi0.0: mtd .name = spi0.0, .size = 0x4000000 (64MiB), > .erasesize = 0x00040000 (256KiB) .numeraseregions = 0 > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000080000 : "loader" > 0x000000080000-0x000000600000 : "user" > 0x000000600000-0x000004000000 : "flash" > renesas_spi e6b10000.spi: registered child spi0.0 > renesas_spi e6b10000.spi: probed > > And /dev/mtd0 reading works fine. > Thanks! > I'm glad that it worked, thanks for the help. I'll do a patch to fix this case, but probably it will qualify for -next. Is -next ok for you? Cheers, ta
Hi Tudor, On Thu, May 9, 2019 at 12:31 PM <Tudor.Ambarus@microchip.com> wrote: > On 05/09/2019 12:11 PM, Geert Uytterhoeven wrote: > > On Thu, May 9, 2019 at 8:56 AM <Tudor.Ambarus@microchip.com> wrote: > >> When the configuration register QUAD bit CR[1] is 1, only the WRR command format > >> with 16 data bits may be used, WRR with 8 bits is not recognized and hence the > >> FFs. You probably set quad bit in u-boot, while others don't. We can verify this > >> assumption with the patch form below. Can you try it? > > > > And /dev/mtd0 reading works fine. > > Thanks! > > > > I'm glad that it worked, thanks for the help. I'll do a patch to fix this case, > but probably it will qualify for -next. Is -next ok for you? Given the issue is present only in -next, fixing it in -next is fine for me. Thanks! Gr{oetje,eeting}s, Geert
Hi Tudor, On 09/05/19 4:01 PM, Tudor.Ambarus@microchip.com wrote: [...] >> >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >> >>> +static int spi_nor_clear_block_protection(struct spi_nor *nor) >>> +{ >>> + int ret; >>> + u8 sr, cr, sr_cr[2] = {0}; >>> + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; >>> + >>> + ret = read_cr(nor); >>> + dev_dbg(nor->dev, "CR = %08x\n", ret); >>> + if (ret < 0) { >>> + dev_err(nor->dev, "error while reading CR\n"); >>> + return ret; >>> + } >>> + cr = ret; >>> + >>> + if (cr & CR_QUAD_EN_SPAN) { >>> + /* disable quad if already set, must do it with 16-bit WRR */ >>> + ret = write_sr_cr(nor, sr_cr); >>> + if (ret) { >>> + dev_err(nor->dev, "error diasbling quad mode\n"); >> >> disabling >> >>> + return ret; >>> + } >> >> renesas_spi e6b10000.spi: DMA available >> renesas_spi e6b10000.spi: registered master spi0 >> spi spi0.0: setup mode 3, 8 bits/w, 30000000 Hz max --> 0 >> m25p80 spi0.0: bfpt.dwords[1] = ffffffff >> m25p80 spi0.0: bfpt.dwords[2] = ffffffff >> m25p80 spi0.0: bfpt.dwords[3] = ffffffff >> m25p80 spi0.0: bfpt.dwords[4] = ffffffff >> m25p80 spi0.0: bfpt.dwords[5] = ffffffff >> m25p80 spi0.0: bfpt.dwords[6] = ffffffff >> m25p80 spi0.0: bfpt.dwords[7] = ffffffff >> m25p80 spi0.0: bfpt.dwords[8] = ffffffff >> m25p80 spi0.0: bfpt.dwords[9] = ffffffff >> m25p80 spi0.0: bfpt.dwords[10] = 00000000 >> m25p80 spi0.0: bfpt.dwords[11] = 00000000 >> m25p80 spi0.0: bfpt.dwords[12] = 00000000 >> m25p80 spi0.0: bfpt.dwords[13] = 00000000 >> m25p80 spi0.0: bfpt.dwords[14] = 00000000 >> m25p80 spi0.0: bfpt.dwords[15] = 00000000 >> m25p80 spi0.0: bfpt.dwords[16] = 00000000 >> m25p80 spi0.0: failed to parse BFPT: err = -22 >> m25p80 spi0.0: spi_nor_init_params sfdp parse failed, ret =-22 >> m25p80 spi0.0: SR and CR before quad_enable: >> m25p80 spi0.0: SR = 00000000 >> m25p80 spi0.0: CR = 00000002 >> m25p80 spi0.0: SR and CR after quad_enable: >> m25p80 spi0.0: SR = 00000000 >> m25p80 spi0.0: CR = 00000002 >> m25p80 spi0.0: s25fl512s (65536 Kbytes) >> m25p80 spi0.0: mtd .name = spi0.0, .size = 0x4000000 (64MiB), >> .erasesize = 0x00040000 (256KiB) .numeraseregions = 0 >> 3 fixed-partitions partitions found on MTD device spi0.0 >> Creating 3 MTD partitions on "spi0.0": >> 0x000000000000-0x000000080000 : "loader" >> 0x000000080000-0x000000600000 : "user" >> 0x000000600000-0x000004000000 : "flash" >> renesas_spi e6b10000.spi: registered child spi0.0 >> renesas_spi e6b10000.spi: probed >> >> And /dev/mtd0 reading works fine. >> Thanks! >> > > I'm glad that it worked, thanks for the help. I'll do a patch to fix this case, > but probably it will qualify for -next. Is -next ok for you? I think this fix should be forwarded to v5.2-rc1 (or -rc2 at least) as patch in question ("spi-nor: s25fl512s supports region locking") is part of SPI NOR pull request for v5.2-rc1 and therefore would end up in mainline v5.2-rc1 during the merge window.
Hi, Geert, On 05/09/2019 02:12 PM, Geert Uytterhoeven wrote: > External E-Mail > > > Hi Tudor, > > On Thu, May 9, 2019 at 12:31 PM <Tudor.Ambarus@microchip.com> wrote: >> On 05/09/2019 12:11 PM, Geert Uytterhoeven wrote: >>> On Thu, May 9, 2019 at 8:56 AM <Tudor.Ambarus@microchip.com> wrote: >>>> When the configuration register QUAD bit CR[1] is 1, only the WRR command format >>>> with 16 data bits may be used, WRR with 8 bits is not recognized and hence the >>>> FFs. You probably set quad bit in u-boot, while others don't. We can verify this >>>> assumption with the patch form below. Can you try it? >>> >>> And /dev/mtd0 reading works fine. >>> Thanks! >>> >> >> I'm glad that it worked, thanks for the help. I'll do a patch to fix this case, >> but probably it will qualify for -next. Is -next ok for you? > > Given the issue is present only in -next, fixing it in -next is fine for me. > Thanks! > I've started working to squash the bug discovered by this patch. spi-nor flashes from different manufacturers have widely different configurations for status and configuration registers. I have a work in progress patch, backward compatibility requirements increased code complexity. I'll be out of office and will return on 3rd of June. Probably I will not finish it today, this is to inform you (and others) that I'll be inactive next week. Cheers, ta
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 0cc0ef4fbf3a..9abac9e326a1 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1898,7 +1898,9 @@ static const struct flash_info spi_nor_ids[] = { SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) }, { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) },
Both the BP[0-2] bits and the TBPROT bit are supported on this chip. Tested and verified on a Cypress s25fl512s. Signed-off-by: Jonas Bonn <jonas@norrbonn.se> --- drivers/mtd/spi-nor/spi-nor.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)