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

[<Tudor.Ambarus@microchip.com>]

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);
>         }
>  
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/