Re: [PATCH v4 2/3] spi-nor: s25fl512s supports region locking

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Jonas,

On Tue, May 7, 2019 at 1:14 PM Jonas Bonn <jonas@xxxxxxxxxxx> wrote:
> On 07/05/2019 12:50, Geert Uytterhoeven wrote:
> > On Tue, May 7, 2019 at 12:42 PM <Tudor.Ambarus@xxxxxxxxxxxxx> wrote:
> >> On 05/07/2019 12:53 PM, Geert Uytterhoeven wrote:
> >>> On Wed, Mar 20, 2019 at 8:16 AM Jonas Bonn <jonas@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxxxxxx

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/