mtd: spi-nor: Correct flags for Winbond w25q128

  The Winbond W25Q128 (actual vendor name W25Q128JV)
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

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.

After this patch I can write to the flash on the
Inteno XG6846 router.

Cc: stable@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/spi-nor/winbond.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230711-spi-nor-winbond-w25q128-321a602ee267

Best regards,
  

Hi Linus,

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?

> 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? Because,
then it's incorrect. For 0xef7018 its actually w25q128jv.

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.

> 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])?

> After this patch I can write to the flash on the
> Inteno XG6846 router.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/spi-nor/winbond.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/winbond.c 
> b/drivers/mtd/spi-nor/winbond.c
> index 834d6ba5ce70..a67e1d4206f3 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -121,7 +121,9 @@ static const struct flash_info winbond_nor_parts[] 
> = {
>  	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
> -		NO_SFDP_FLAGS(SECT_4K) },
> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> +		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> +			      SPI_NOR_QUAD_READ) },

As mentioned above, could you try without the DUAL_READ/QUAD_READ flags.
You can have a look at the debugfs whether the detected capabilities
are still the same with and without these flags.

-michael

[1] https://www.elinux.org/images/f/f5/Winbond-w25q32.pdf
[2] 
https://lore.kernel.org/all/4304e19f3399a0a6e856119d01ccabe0@walle.cc/
  

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@walle.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@OpenWrt:/sys/devices/platform/ubus/10001000.spi/spi_master/spi1/spi1.0/spi-nor#
cat jedec_id
ef4018

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

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

root@OpenWrt:/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@OpenWrt:/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@OpenWrt:/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