Re: [PATCH 2/2] mtd: spi-nor: refine Spansion S25FL512S ID

From: <Tudor.Ambarus@microchip.com>
To: <geert@linux-m68k.org>, <sergei.shtylyov@cogentembedded.com>
Cc: bbrezillon@kernel.org, richard@nod.at,
	linux-renesas-soc@vger.kernel.org, marek.vasut@gmail.com,
	linux-mtd@lists.infradead.org, computersforpeace@gmail.com,
	dwmw2@infradead.org
Subject: Re: [PATCH 2/2] mtd: spi-nor: refine Spansion S25FL512S ID
Date: Wed, 6 Mar 2019 10:59:17 +0000	[thread overview]
Message-ID: <0e132c00-fb0c-c535-0f23-0cfa19a9a99e@microchip.com> (raw)
In-Reply-To: <CAMuHMdVjvbcK21r9ajp9xKEGx=VHbXkrSB0ob2EjR8+dmNVdPw@mail.gmail.com>

Hi, Geert,

On 03/05/2019 04:05 PM, Geert Uytterhoeven wrote:
> Hi Sergei,
> 
> On Wed, Jan 16, 2019 at 6:53 PM Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
>> Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip
>> family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also
>> covering S25FS512S and now that we have added 6-byte ID for that chip, we
>> can convert S25FL512S to using a proper 6-byte ID as well...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> This is now commit a2126b0a010905e5 ("mtd: spi-nor: refine Spansion
> S25FL512S ID"), and turns out to cause a regression on r8a7791/koelsch.
> Dmesg diff 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: unrecognized JEDEC id bytes: 01, 02, 20
> 
> As the (old) U-Boot on my Koelsch keeps many module clocks enabled, I
> typically merge in my topic/renesas-debug branch, which makes sure all
> non-critical module clocks are disabled early during boot, to catch
> drivers not properly implementing Runtime PM.
> 
> However, it turns out this has some impact on JEDEC ID detection:
>   - When module clocks are left untouched, spi_nor_read_id() reads
>     0x01:0x02:0x20:0x4d:0x00:0x80.
>   - When my debug code has disabled module clocks during early boot,
>     The last byte is 0x00.
> 
> Before the above commit, only the first 5 bytes were compared, and the
> last byte was ignored, thus not causing problems.
> When comparing all 6 bytes, detection fails if the last byte is 0x00.
> 

We'll have to understand why just the 6th byte is different.

> I believe mainline U-Boot for R-Car Gen2 boards doesn't keep the QSPI
> module clock enabled, so this commit may breaks those boards.

Who provides the flash's input clock and at what frequency?

Cheers,
ta

> 
> To be investigated more (e.g. with a logic analyzer)...
> 
>> --- linux-mtd.orig/drivers/mtd/spi-nor/spi-nor.c
>> +++ linux-mtd/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1887,8 +1887,8 @@ static const struct flash_info spi_nor_i
>>         { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>>         { "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) },
>>         { "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> -       { "s25fl512s",  INFO(0x010220, 0x4d00, 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) },
>>         { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> 
> 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
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/