Message-ID: <CACRpkdaKOHoq_8yhBGdvYpkUr=cZM+-XXyotx4GvJN3C1-ADYg@mail.gmail.com>
Date:   Wed, 12 Jul 2023 23:50:11 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Michael Walle <michael@...le.cc>
Cc:     Tudor Ambarus <tudor.ambarus@...aro.org>,
        Pratyush Yadav <pratyush@...nel.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] mtd: spi-nor: Correct flags for Winbond w25q128

Thanks for helping out Michael, I would never get this
right without people like you!

On Wed, Jul 12, 2023 at 9:04 AM Michael Walle <michael@...le.cc> wrote:

> Am 2023-07-12 00:02, schrieb Linus Walleij:
> > The Winbond W25Q128 (actual vendor name W25Q128JV)
>
> Not necessarily see below. Do you know what part numbers is
> written on your flash?

Yes, if I look at it with a looking glass it says
Winbond
25Q128JVF

> > has exactly the same flags as the sibling device
> > w25q128fw. The devices both require unlocking and
> > support dual and quad SPI transport.
> >
> > The actual product naming between devices:
> >
> > 0xef4018: "w25q128"   W25Q128JV-IM/JM
> > 0xef7018: "w25q128fw" W25Q128JV-IN/IQ/JQ
>
> Where do you get that string? from winbond.c?

Yes

> Because,
> then it's incorrect. For 0xef7018 its actually w25q128jv.

No I just confused things, it should be w25q128jv not fw.
But the actual names to the right are from the datasheet,
they are kind of both actually named "jv" :/

> But that being said, Winbond is known to reuse the IDs among its
> flashes. From a quick look at various datasheets:
>
> 0x60 seems to be DW, FW and NW(Q) series
> 0x70 seems to be JV(M)
> 0x80 seems to be NW(M)
> 0x40 seems to be BV, JV(Q), "V" (probably the first [1])
>
> (Q) denotes the fixed quad enable bit.
>
> Now 0x40 are the first ones who where added back in the days. I'm
> not sure, what kind of winbond devices there were and if they
> support dual/quad read.
>
> Normally, you'd use a .fixups (see w25q256_fixups for example) to
> dynamically detect the newer flash type and then refine the flags.
> But because we don't know how the older flashes look like, that
> would be just guessing :/ Although, I've once thought about
> fingerprinting the SFDP tables eg. by some hash. But that would
> assume the SFDP data is not changing a lot on a given device. Not
> sure if that is the case, we just began to collect SFDP tables
> of various devices.
>
> If it turns out that only SPI_NOR_HAS_LOCK and SPI_NOR_HAS_TB
> is needed, I'm leaning towards just adding these flags to the
> w25q128 entry. According to [1] this was already supported
> back in the days.

They are absolutely needed, else I cannot write to the flash.

> > The latter device, "w25q128fw" supports features
> > named DTQ and QPI, otherwise it is the same.
> >
> > Not having the right flags has the annoying side
> > effect that write access does not work.
>
> This should only apply to FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB).
>
> I'd guess your flash supports SFDP, then the NO_SFDP_FLAGS should be
> automatically detected. Could you please dump the SFDP tables
> (described in [2])?

I hope this is correct:

root@...nWrt:/sys/devices/platform/ubus/10001000.spi/spi_master/spi1/spi1.0/spi-nor#
cat jedec_id
ef4018

root@...nWrt:/sys/devices/platform/ubus/10001000.spi/spi_master/spi1/spi1.0/spi-nor#
cat manufacturer
winbond

root@...nWrt:/sys/devices/platform/ubus/10001000.spi/spi_master/spi1/spi1.0/spi-nor#
cat partname
w25q128

root@...nWrt:/sys/devices/platform/ubus/10001000.spi/spi_master/spi1/spi1.0/spi-nor#
hexdump -v -C sfdp
00000000  53 46 44 50 05 01 00 ff  00 05 01 10 80 00 00 ff  |SFDP............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000020  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000030  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000040  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000050  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000060  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000070  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00000080  e5 20 f9 ff ff ff ff 07  44 eb 08 6b 08 3b 42 bb  |. ......D..k.;B.|
00000090  fe ff ff ff ff ff 00 00  ff ff 40 eb 0c 20 0f 52  |..........@.. .R|
000000a0  10 d8 00 00 36 02 a6 00  82 ea 14 c9 e9 63 76 33  |....6........cv3|
000000b0  7a 75 7a 75 f7 a2 d5 5c  19 f7 4d ff e9 30 f8 80  |zuzu...\..M..0..|
000000c0

> As mentioned above, could you try without the DUAL_READ/QUAD_READ flags.

It works fine but I cannot judge if it is faster or slower,
I guess it mostly affects the speed right?

Don't I need to set the PARSE_SFDP macro here, to turn
.parse_sfdp = true?

> You can have a look at the debugfs whether the detected capabilities
> are still the same with and without these flags.

This is with no changes:

root@...nWrt:/sys/kernel/debug/spi-nor/spi1.0# cat capabilities
Supported read modes by the flash
 1S-1S-1S
  opcode        0x03
  mode cycles   0
  dummy cycles  0
 1S-1S-1S (fast read)
  opcode        0x0b
  mode cycles   0
  dummy cycles  8

Supported page program modes by the flash
 1S-1S-1S
  opcode        0x02

This is with PARSE_SFDP:

root@...nWrt:/sys/kernel/debug/spi-nor/spi1.0# cat capabilities
Supported read modes by the flash
 1S-1S-1S
  opcode        0x03
  mode cycles   0
  dummy cycles  0
 1S-1S-1S (fast read)
  opcode        0x0b
  mode cycles   0
  dummy cycles  8
 1S-1S-2S
  opcode        0x3b
  mode cycles   0
  dummy cycles  8
 1S-2S-2S
  opcode        0xbb
  mode cycles   2
  dummy cycles  2
 1S-1S-4S
  opcode        0x6b
  mode cycles   0
  dummy cycles  8
 1S-4S-4S
  opcode        0xeb
  mode cycles   2
  dummy cycles  4
 4S-4S-4S
  opcode        0xeb
  mode cycles   2
  dummy cycles  0

Supported page program modes by the flash
 1S-1S-1S
  opcode        0x02

So indeed it works with SFDP parsing! I'll send an updated patch.

I guess a lot of the chips could actually use this but someone has
to test them on a 1-by-1 basis?

Yours,
Linus Walleij

Powered by blists - more mailing lists