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

From: Jonas Bonn <jonas@norrbonn.se>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>
Cc: MTD Maling List <linux-mtd@lists.infradead.org>,
	Marek Vasut <marek.vasut+renesas@gmail.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v4 2/3] spi-nor: s25fl512s supports region locking
Date: Tue, 7 May 2019 13:13:55 +0200	[thread overview]
Message-ID: <fac5fa6d-95e9-cfb0-4d5a-6b16d4470190@norrbonn.se> (raw)
In-Reply-To: <CAMuHMdU83vLeVSqMZuJwR4yd382mau-OE1saMAOC2+6HodsHvg@mail.gmail.com>

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