[PATCH 0/3] mtd: spi-nor: introduce optional rdid-dummy-ncycles DT property

[PATCH 0/3] mtd: spi-nor: introduce optional rdid-dummy-ncycles DT property

[Takahiro Kuwano]

There are infineon flashes [1] that require 8 dummy cycles for the
1-1-1 Read ID command. Since the command is not covered by JESD216
or any other standard, introduce an optional "rdid-dummy-ncycles"
DT property to allow flashes to be correctly identified.
Add support for CYRS17B512.

Link: https://www.infineon.com/dgdl/Infineon-CYRS17B512_512_MB_64_MB_SERIAL_NOR_FLASH_SPI_QSPI_3-DataSheet-v07_00-EN.pdf?fileId=8ac78c8c8fc2dd9c01900eee733d45f3 [1]

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Takahiro Kuwano (3):
      dt-bindings: mtd: jedec,spi-nor: add optional rdid-dummy-ncycles
      mtd: spi-nor: use rdid-dummy-ncycles DT property
      mtd: spi-nor: spansion: add support for CYRS17B512

 .../devicetree/bindings/mtd/jedec,spi-nor.yaml          |  6 ++++++
 drivers/mtd/spi-nor/core.c                              |  9 ++++++++-
 drivers/mtd/spi-nor/spansion.c                          | 17 +++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250214-snor-rdid-dummy-ncycles-73575ecc7b8d

Best regards,
-- 
Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

[PATCH 1/3] dt-bindings: mtd: jedec,spi-nor: add optional rdid-dummy-ncycles

[Takahiro Kuwano]

There are infineon flashes [1] that require 8 dummy cycles for the
1-1-1 Read ID command. Since the command is not covered by JESD216
or any other standard, introduce an optional "rdid-dummy-ncycles"
DT property to allow flashes to be correctly identified.

Link: https://www.infineon.com/dgdl/Infineon-CYRS17B512_512_MB_64_MB_SERIAL_NOR_FLASH_SPI_QSPI_3-DataSheet-v07_00-EN.pdf?fileId=8ac78c8c8fc2dd9c01900eee733d45f3 [1]
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 335f8204aa1ebce3d2b4686b2a06d0ea3791667c..25abbe4f5d17f66215fa1af097d14de4a6ef776f 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -63,6 +63,12 @@ properties:
       it can not be detected at runtime. Refer to your chips' datasheet to check
       if this is supported by your chip.
 
+  rdid-dummy-ncycles:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Number of dummy cycles for the 1-1-1 Read ID command, if not zero.
+      The Read ID command is not covered by JESD216 or any other standard.
+
   broken-flash-reset:
     type: boolean
     description:

-- 
2.34.1

[PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Takahiro Kuwano]

There are infineon flashes [1] that require 8 dummy cycles for the
1-1-1 Read ID command. Since the command is not covered by JESD216
or any other standard, get the number of dummy cycles from DT and use
them to correctly identify the flash.

Link: https://www.infineon.com/dgdl/Infineon-CYRS17B512_512_MB_64_MB_SERIAL_NOR_FLASH_SPI_QSPI_3-DataSheet-v07_00-EN.pdf?fileId=8ac78c8c8fc2dd9c01900eee733d45f3 [1]
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi-nor/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 19eb98bd68210f41acd716635c02a8936678a385..6452ae6eecee3325b52cdcc2cc9703355951e0db 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -7,6 +7,7 @@
  * Copyright (C) 2014, Freescale Semiconductor, Inc.
  */
 
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/delay.h>
@@ -16,6 +17,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/spi-nor.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sched/task_stack.h>
@@ -2011,9 +2013,14 @@ static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
 {
 	const struct flash_info *info;
 	u8 *id = nor->bouncebuf;
+	u32 ndummy = 0;
 	int ret;
 
-	ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
+	if (!of_property_read_u32(nor->dev->of_node, "rdid-dummy-ncycles",
+				  &ndummy))
+		ndummy /= BITS_PER_BYTE;
+
+	ret = spi_nor_read_id(nor, 0, ndummy, id, nor->reg_proto);
 	if (ret) {
 		dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
 		return ERR_PTR(ret);

-- 
2.34.1

[PATCH 3/3] mtd: spi-nor: spansion: add support for CYRS17B512

[Takahiro Kuwano]

Add device ID info and fixups to support Infineon CYRS17B512 flash.

Although this flash has untypical features such as dummy cycles in RDID,
inverted erase polarity, larger program page size with automatic page
erase, and larger sector size, it supports basic flash commands
including SFDP.

Link: https://www.infineon.com/dgdl/Infineon-CYRS17B512_512_MB_64_MB_SERIAL_NOR_FLASH_SPI_QSPI_3-DataSheet-v07_00-EN.pdf?fileId=8ac78c8c8fc2dd9c01900eee733d45f3
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Tested on Xilinx Zynq-7000 board and Infineon internal SPI controller.

zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
c1601a

zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
spansion

zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080102ff00080114000300ff84080102500300ff8700011c5803
00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
fffffffffffffffffffffffffffffffffffff7ffe2ffffffff1f48eb086b
fffffffffeffffffffffffffffff48eb142017d800ff00ffa028fdffb73f
84a2e0fb1fc4ffff7a75f7ffffff22f65dfff050f8a10000000000002c00
00000000f6fffffff30600fe21dcffff0000800000000000c0ffc3ebc0ff
c3eb00650090066500b1006501950065019671650494716504d000000000
b02e000088a489aa71650393716503930000000000000000000000000000
0000000000000000000000000000000000000000000000000000716503d4
716503d400002020

zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
a2fec0f47c5aa119e21c3d50a173e2ba  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp

zynq> cat /sys/kernel/debug/spi-nor/spi0.0/capabilities
Supported read modes by the flash
 1S-1S-1S
  opcode        0x13
  mode cycles   0
  dummy cycles  0
 1S-1S-1S (fast read)
  opcode        0x0c
  mode cycles   8
  dummy cycles  8
 1S-1S-4S
  opcode        0x6c
  mode cycles   0
  dummy cycles  8
 1S-4S-4S
  opcode        0xec
  mode cycles   2
  dummy cycles  8
 4S-4S-4S
  opcode        0xec
  mode cycles   2
  dummy cycles  8

Supported page program modes by the flash
 1S-1S-1S
  opcode        0x12
 1S-1S-4S
  opcode        0x34

zynq> cat /sys/kernel/debug/spi-nor/spi0.0/params
name            (null)
id              c1 60 1a 00 00 00
size            64.0 MiB
write size      1
page size       2048
address nbytes  4
flags           4B_OPCODES | HAS_4BAIT | HAS_16BIT_SR | SOFT_RESET

opcodes
 read           0xec
  dummy cycles  10
 erase          0xdc
 program        0x34
 8D extension   repeat

protocols
 read           1S-4S-4S
 write          1S-1S-4S
 register       1S-1S-1S

erase commands
 21 (1.00 MiB) [2]
 dc (8.00 MiB) [3]
 c7 (64.0 MiB)

sector map
 region (in hex)   | erase mask | overlaid
 ------------------+------------+----------
 00000000-03ffffff |     [   3] | no

zynq> mtd_debug info /dev/mtd0
mtd.type = MTD_NORFLASH
mtd.flags = MTD_CAP_NORFLASH
mtd.size = 67108864 (64M)
mtd.erasesize = 8388608 (8M)
mtd.writesize = 1
mtd.oobsize = 0
regions = 0

zynq> ./test_spi_cyrs17b512.sh
8+0 records in
8+0 records out
8388608 bytes (8.0MB) copied, 0.308282 seconds, 26.0MB/s
Erased 8388608 bytes from address 0x00000000 in flash
Copied 8388608 bytes from address 0x00000000 in flash to spi_read
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
0800000
2daeb1f36095b44b318410b3f4e8b5d989dcc7bb023d1426c492dab0a3053e74  spi_read
Copied 8388608 bytes from spi_test to address 0x00000000 in flash
Copied 8388608 bytes from address 0x00000000 in flash to spi_read
8f8842585053d5200d7d80bc766dcf8bbe9f4fea08499c576f67ed631050b6c3  spi_read
8f8842585053d5200d7d80bc766dcf8bbe9f4fea08499c576f67ed631050b6c3  spi_test
Erased 8388608 bytes from address 0x00000000 in flash
Copied 8388608 bytes from address 0x00000000 in flash to spi_read
2daeb1f36095b44b318410b3f4e8b5d989dcc7bb023d1426c492dab0a3053e74  spi_read
8f8842585053d5200d7d80bc766dcf8bbe9f4fea08499c576f67ed631050b6c3  spi_test
---
 drivers/mtd/spi-nor/spansion.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index bf08dbf5e7421f8725a9931e36acaf3f7348db42..5c9588b02b7e61f1b64e5dc61e5c1f976ac58508 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -758,6 +758,18 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
 	.post_bfpt = s25fs_s_nor_post_bfpt_fixups,
 };
 
+static int cyrs17b_late_init(struct spi_nor *nor)
+{
+	/* Fast Read requires mode cycles */
+	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+
+	return 0;
+}
+
+static const struct spi_nor_fixups cyrs17b_fixups = {
+	.late_init = cyrs17b_late_init,
+};
+
 static const struct flash_info spansion_nor_parts[] = {
 	{
 		.id = SNOR_ID(0x01, 0x02, 0x12),
@@ -996,6 +1008,11 @@ static const struct flash_info spansion_nor_parts[] = {
 		.name = "s28hs02gt",
 		.mfr_flags = USE_CLPEF,
 		.fixups = &s28hx_t_fixups,
+	}, {
+		/* cyrs17b512 */
+		.id = SNOR_ID(0xc1, 0x60, 0x1a),
+		.mfr_flags = USE_CLSR,
+		.fixups = &cyrs17b_fixups
 	}, {
 		.id = SNOR_ID(0xef, 0x40, 0x13),
 		.name = "s25fl004k",

-- 
2.34.1

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Rob Herring]

On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
> There are infineon flashes [1] that require 8 dummy cycles for the
> 1-1-1 Read ID command. Since the command is not covered by JESD216
> or any other standard, get the number of dummy cycles from DT and use
> them to correctly identify the flash.

If Read ID fails, then couldn't you just retry with dummy cycles? Or 
would unconditionally adding dummy cycles adversely affect other chips?

Otherwise, add a specific compatible to imply this requirement. Adding 
quirk properties doesn't scale.

Rob

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Michael Walle]

On Thu Mar 20, 2025 at 12:30 AM CET, Rob Herring wrote:
> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
> > There are infineon flashes [1] that require 8 dummy cycles for the
> > 1-1-1 Read ID command. Since the command is not covered by JESD216
> > or any other standard, get the number of dummy cycles from DT and use
> > them to correctly identify the flash.
>
> If Read ID fails, then couldn't you just retry with dummy cycles? Or 
> would unconditionally adding dummy cycles adversely affect other chips?

Yes exactly. IIUC, the first byte of the auto discovery (RDID
command) would return random data, because the output driver of the
flash is disabled. The second byte would then return the
manufacturer id and third and fourth the device id. This makes auto
discovery fundamentally broken with this chip as the first byte
might randomly match any manufacturer within our database.

Takahiro, if you can reach designer of this chip, you might tell
them, that this was not the greatest idea :o

> Otherwise, add a specific compatible to imply this requirement. Adding 
> quirk properties doesn't scale.

Since auto discovery doesn't work at all, you might just add,
"infineon,cyrs17b512" *instead of* "jedec,spi-nor" (because it doesn't
support the usual RDID command").

Alternatively, we could introduce a "generic" compatible which just
uses the standard RDSFDP command instead of RDID for discovery. Rob?
Then we'd need some SFDP fingerprinting mechanism to match the SFDP
to a particular flash type.

-michael

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Tudor Ambarus]

Hi, Rob,

On 3/19/25 11:30 PM, Rob Herring wrote:
> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
>> There are infineon flashes [1] that require 8 dummy cycles for the
>> 1-1-1 Read ID command. Since the command is not covered by JESD216
>> or any other standard, get the number of dummy cycles from DT and use
>> them to correctly identify the flash.
> 
> If Read ID fails, then couldn't you just retry with dummy cycles? Or 

I think Read ID won't fail when the op requires 8 dummy cycles, it
probably just reads garbage on the first 8 cycles, so we risk to wrongly
match other flash IDs.

> would unconditionally adding dummy cycles adversely affect other chips?

Adding 8 dummy cycles to chips that don't need it, would mean ignoring
the first byte of the flash ID, thus we again risk to wrongly match
against other flash IDs.

> 
> Otherwise, add a specific compatible to imply this requirement. Adding 
> quirk properties doesn't scale.

Do you mean a flash name compatible, like "cyrs17b512,spi-nor"? The
problem that I see with that is that we no longer bind against the
generic jedec,spi-nor compatible, so people need to update their DT in
case they use/plug-in a different flash on their board.

Thanks,
ta

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Rob Herring]

On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Rob,
>
> On 3/19/25 11:30 PM, Rob Herring wrote:
> > On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
> >> There are infineon flashes [1] that require 8 dummy cycles for the
> >> 1-1-1 Read ID command. Since the command is not covered by JESD216
> >> or any other standard, get the number of dummy cycles from DT and use
> >> them to correctly identify the flash.
> >
> > If Read ID fails, then couldn't you just retry with dummy cycles? Or
>
> I think Read ID won't fail when the op requires 8 dummy cycles, it
> probably just reads garbage on the first 8 cycles, so we risk to wrongly
> match other flash IDs.
>
> > would unconditionally adding dummy cycles adversely affect other chips?
>
> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
> the first byte of the flash ID, thus we again risk to wrongly match
> against other flash IDs.
>
> >
> > Otherwise, add a specific compatible to imply this requirement. Adding
> > quirk properties doesn't scale.
>
> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?

Yes, but that's not the format of compatible strings.

> The
> problem that I see with that is that we no longer bind against the
> generic jedec,spi-nor compatible, so people need to update their DT in
> case they use/plug-in a different flash on their board.

This chip is clearly *not* compatible with a generic chip.

You have the same problem with a property. Users have to add or remove
the property if the flash changes. Anyone thinking they can use this
chip as a compatible 2nd source is SOL.

Rob

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Tudor Ambarus]

On 3/20/25 2:06 PM, Rob Herring wrote:
> On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Hi, Rob,
>>
>> On 3/19/25 11:30 PM, Rob Herring wrote:
>>> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
>>>> There are infineon flashes [1] that require 8 dummy cycles for the
>>>> 1-1-1 Read ID command. Since the command is not covered by JESD216
>>>> or any other standard, get the number of dummy cycles from DT and use
>>>> them to correctly identify the flash.
>>>
>>> If Read ID fails, then couldn't you just retry with dummy cycles? Or
>>
>> I think Read ID won't fail when the op requires 8 dummy cycles, it
>> probably just reads garbage on the first 8 cycles, so we risk to wrongly
>> match other flash IDs.
>>
>>> would unconditionally adding dummy cycles adversely affect other chips?
>>
>> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
>> the first byte of the flash ID, thus we again risk to wrongly match
>> against other flash IDs.
>>
>>>
>>> Otherwise, add a specific compatible to imply this requirement. Adding
>>> quirk properties doesn't scale.
>>
>> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?
> 
> Yes, but that's not the format of compatible strings.
> 
>> The
>> problem that I see with that is that we no longer bind against the
>> generic jedec,spi-nor compatible, so people need to update their DT in
>> case they use/plug-in a different flash on their board.
> 
> This chip is clearly *not* compatible with a generic chip.

I think it is compatible. The chip defines the SFDP (serial flash
discoverable parameters) tables. At probe time we parse those tables and
initialize the flash based on them.

We don't even care about the chip ID, if all the flash parameters can be
discovered via SFDP. Unfortunately these tables do not describe all the
flash capabilities (block protection being one). Or worse, manufacturers
mangle these tables.

So vendors need to identify chips to either fix those tables via some
quirks after the parsing is done, or to specify support that's not
covered by those tables.

For basic ops, flashes that get the SFDP tables right, don't even need a
flash entry defined, we don't care about their ID, we just initialize
the flash solely based on SFDP.

In this particular case, this flash needs identification to fix some
wrong SFDP field, it corrects just the mode cycles for the FAST READ
command. All the other commands seem fine according to patch 3/3.

> 
> You have the same problem with a property. Users have to add or remove

True. It's the same problem. Even if we specify the dummy cycles via a
property, the next plugged-in flash will use those. We can of course
fallback to the SFDP only init if the ID doesn't match any flash entry,
but the problem is the same.

> the property if the flash changes. Anyone thinking they can use this
> chip as a compatible 2nd source is SOL.
> 

I think the property vs compatible decision resumes at whether we
consider that the dummy cycles requirement for Read ID is/will be
generic or not.

I noticed that with higher frequencies or protocol modes (e.g, octal
DTR), flashes tend to require more dummy cycles. I think with time,
we'll have more flashes with such requirement. Takahiro can jump in and
tell if it's already the case with IFX.

Thus instead of having lots of new compatibles for this, I lean towards
having this property. I'm still open for the compatible idea, I just
wanted to explain better where we are.

Thanks,
ta

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Michael Walle]

Hi,

> >>>> There are infineon flashes [1] that require 8 dummy cycles for the
> >>>> 1-1-1 Read ID command. Since the command is not covered by JESD216
> >>>> or any other standard, get the number of dummy cycles from DT and use
> >>>> them to correctly identify the flash.
> >>>
> >>> If Read ID fails, then couldn't you just retry with dummy cycles? Or
> >>
> >> I think Read ID won't fail when the op requires 8 dummy cycles, it
> >> probably just reads garbage on the first 8 cycles, so we risk to wrongly
> >> match other flash IDs.
> >>
> >>> would unconditionally adding dummy cycles adversely affect other chips?
> >>
> >> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
> >> the first byte of the flash ID, thus we again risk to wrongly match
> >> against other flash IDs.
> >>
> >>>
> >>> Otherwise, add a specific compatible to imply this requirement. Adding
> >>> quirk properties doesn't scale.
> >>
> >> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?
> > 
> > Yes, but that's not the format of compatible strings.
> > 
> >> The
> >> problem that I see with that is that we no longer bind against the
> >> generic jedec,spi-nor compatible, so people need to update their DT in
> >> case they use/plug-in a different flash on their board.
> > 
> > This chip is clearly *not* compatible with a generic chip.
>
> I think it is compatible. The chip defines the SFDP (serial flash
> discoverable parameters) tables. At probe time we parse those tables and
> initialize the flash based on them.

I disagree. It's not compatible with "jedec,spi-nor", which is
defined as

      SPI NOR flashes compatible with the JEDEC SFDP standard or which may be
      identified with the READ ID opcode (0x9F) do not deserve a specific
      compatible. They should instead only be matched against the generic
      "jedec,spi-nor" compatible.

The first part was recently added and is a bit misleading. The old
definition was:

      Must also include "jedec,spi-nor" for any SPI NOR flash that can be
      identified by the JEDEC READ ID opcode (0x9F).

See my first reply, on how to possibly fix this mess (new
compatible if accepted, just use RDSFDP sequence which is backed by
the standard and do some fingerprinting).

FWIW, a new (or rather different) compatible is needed because we
cannot distinguish between random data returned during the dummy
cycles and a proper manufacturer id. So there is no way we could fix
this in the core itself.

At least if we keep the logic as it is for now, if we use RDSFDP to
fingerprint first and then fall back to RDID, we could get away with
the old compatible.

> We don't even care about the chip ID, if all the flash parameters can be
> discovered via SFDP. Unfortunately these tables do not describe all the
> flash capabilities (block protection being one). Or worse, manufacturers
> mangle these tables.
>
> So vendors need to identify chips to either fix those tables via some
> quirks after the parsing is done, or to specify support that's not
> covered by those tables.
>
> For basic ops, flashes that get the SFDP tables right, don't even need a
> flash entry defined, we don't care about their ID, we just initialize
> the flash solely based on SFDP.
>
> In this particular case, this flash needs identification to fix some
> wrong SFDP field, it corrects just the mode cycles for the FAST READ
> command. All the other commands seem fine according to patch 3/3.
>
> > 
> > You have the same problem with a property. Users have to add or remove
>
> True. It's the same problem. Even if we specify the dummy cycles via a
> property, the next plugged-in flash will use those. We can of course
> fallback to the SFDP only init if the ID doesn't match any flash entry,
> but the problem is the same.
>
> > the property if the flash changes. Anyone thinking they can use this
> > chip as a compatible 2nd source is SOL.
> > 
>
> I think the property vs compatible decision resumes at whether we
> consider that the dummy cycles requirement for Read ID is/will be
> generic or not.

It is not generic. Because it will break autodetection. And that is
the whole purpose of this. Adding that property means, we can just
autodetect flashes within this 'group'. And personally, I think this
is a bad precedent.

> I noticed that with higher frequencies or protocol modes (e.g, octal
> DTR), flashes tend to require more dummy cycles. I think with time,
> we'll have more flashes with such requirement. Takahiro can jump in and
> tell if it's already the case with IFX.

But hopefully not with RDID. Again this doesn't play nice with other
flashes (or all flashes for now). Instead of adding random delay
cycles one should rather define a max clock speed for this opcode.

-michael

> Thus instead of having lots of new compatibles for this, I lean towards
> having this property. I'm still open for the compatible idea, I just
> wanted to explain better where we are.
>
> Thanks,
> ta

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Takahiro Kuwano]

On 3/21/2025 12:45 AM, Tudor Ambarus wrote:
> 
> 
> On 3/20/25 2:06 PM, Rob Herring wrote:
>> On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>> Hi, Rob,
>>>
>>> On 3/19/25 11:30 PM, Rob Herring wrote:
>>>> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
>>>>> There are infineon flashes [1] that require 8 dummy cycles for the
>>>>> 1-1-1 Read ID command. Since the command is not covered by JESD216
>>>>> or any other standard, get the number of dummy cycles from DT and use
>>>>> them to correctly identify the flash.
>>>>
>>>> If Read ID fails, then couldn't you just retry with dummy cycles? Or
>>>
>>> I think Read ID won't fail when the op requires 8 dummy cycles, it
>>> probably just reads garbage on the first 8 cycles, so we risk to wrongly
>>> match other flash IDs.
>>>
>>>> would unconditionally adding dummy cycles adversely affect other chips?
>>>
>>> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
>>> the first byte of the flash ID, thus we again risk to wrongly match
>>> against other flash IDs.
>>>
>>>>
>>>> Otherwise, add a specific compatible to imply this requirement. Adding
>>>> quirk properties doesn't scale.
>>>
>>> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?
>>
>> Yes, but that's not the format of compatible strings.
>>
>>> The
>>> problem that I see with that is that we no longer bind against the
>>> generic jedec,spi-nor compatible, so people need to update their DT in
>>> case they use/plug-in a different flash on their board.
>>
>> This chip is clearly *not* compatible with a generic chip.
> 
> I think it is compatible. The chip defines the SFDP (serial flash
> discoverable parameters) tables. At probe time we parse those tables and
> initialize the flash based on them.
> 
> We don't even care about the chip ID, if all the flash parameters can be
> discovered via SFDP. Unfortunately these tables do not describe all the
> flash capabilities (block protection being one). Or worse, manufacturers
> mangle these tables.
> 
> So vendors need to identify chips to either fix those tables via some
> quirks after the parsing is done, or to specify support that's not
> covered by those tables.
> 
> For basic ops, flashes that get the SFDP tables right, don't even need a
> flash entry defined, we don't care about their ID, we just initialize
> the flash solely based on SFDP.
> 
> In this particular case, this flash needs identification to fix some
> wrong SFDP field, it corrects just the mode cycles for the FAST READ
> command. All the other commands seem fine according to patch 3/3.
> 
>>
>> You have the same problem with a property. Users have to add or remove
> 
> True. It's the same problem. Even if we specify the dummy cycles via a
> property, the next plugged-in flash will use those. We can of course
> fallback to the SFDP only init if the ID doesn't match any flash entry,
> but the problem is the same.
> 
>> the property if the flash changes. Anyone thinking they can use this
>> chip as a compatible 2nd source is SOL.
>>
> 
> I think the property vs compatible decision resumes at whether we
> consider that the dummy cycles requirement for Read ID is/will be
> generic or not.
> 
> I noticed that with higher frequencies or protocol modes (e.g, octal
> DTR), flashes tend to require more dummy cycles. I think with time,
> we'll have more flashes with such requirement. Takahiro can jump in and
> tell if it's already the case with IFX.
> 
Infineon SEMPER flash families (S25Hx-T/S28Hx-T) requires dummy cycles in
Read ID, depending on Configuration Register setting. That is to support
higher frequencies. By factory default dummy is 0 in 1S-1S-1S and it works
up to 133MHz. Users can change the setting to support higher frequencies.

> Thus instead of having lots of new compatibles for this, I lean towards
> having this property. I'm still open for the compatible idea, I just
> wanted to explain better where we are.
> 
> Thanks,
> ta
> 

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Tudor Ambarus]

Hi, Michael,

Sorry, I somehow missed your replies.

On 3/21/25 8:00 AM, Michael Walle wrote:

cut

>>>> The
>>>> problem that I see with that is that we no longer bind against the
>>>> generic jedec,spi-nor compatible, so people need to update their DT in
>>>> case they use/plug-in a different flash on their board.
>>>
>>> This chip is clearly *not* compatible with a generic chip.
>>
>> I think it is compatible. The chip defines the SFDP (serial flash
>> discoverable parameters) tables. At probe time we parse those tables and
>> initialize the flash based on them.
> 
> I disagree. It's not compatible with "jedec,spi-nor", which is
> defined as
> 

cut

> 
> See my first reply, on how to possibly fix this mess (new
> compatible if accepted, just use RDSFDP sequence which is backed by
> the standard and do some fingerprinting).
>

this won't work unless there's a unique parameter or ID in the sfdp or
vendors tables, which I doubt. Takahiro to confirm.

> FWIW, a new (or rather different) compatible is needed because we
> cannot distinguish between random data returned during the dummy
> cycles and a proper manufacturer id. So there is no way we could fix
> this in the core itself.

Yes, I agree, new compatible it is then.

cut

>> I think the property vs compatible decision resumes at whether we
>> consider that the dummy cycles requirement for Read ID is/will be
>> generic or not.
> 
> It is not generic. Because it will break autodetection. And that is
> the whole purpose of this. Adding that property means, we can just
> autodetect flashes within this 'group'. And personally, I think this
> is a bad precedent.
> 

yes, I agree.

>> I noticed that with higher frequencies or protocol modes (e.g, octal
>> DTR), flashes tend to require more dummy cycles. I think with time,
>> we'll have more flashes with such requirement. Takahiro can jump in and
>> tell if it's already the case with IFX.
> 
> But hopefully not with RDID. Again this doesn't play nice with other
> flashes (or all flashes for now). Instead of adding random delay
> cycles one should rather define a max clock speed for this opcode.

This could work, yes. But not for this flash. Or maybe encourage vendors
to either contribute and enlarge the SFDP database or define their own
vendor tables for all the flash properties that are not covered yet.
It's strange how Block Protection is not yet covered by SFDP after all
these years.

Thanks,
ta

Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Takahiro Kuwano]

On 3/26/2025 11:44 PM, Tudor Ambarus wrote:
> Hi, Michael,
> 
> Sorry, I somehow missed your replies.
> 
> On 3/21/25 8:00 AM, Michael Walle wrote:
> 
> cut
> 
>>>>> The
>>>>> problem that I see with that is that we no longer bind against the
>>>>> generic jedec,spi-nor compatible, so people need to update their DT in
>>>>> case they use/plug-in a different flash on their board.
>>>>
>>>> This chip is clearly *not* compatible with a generic chip.
>>>
>>> I think it is compatible. The chip defines the SFDP (serial flash
>>> discoverable parameters) tables. At probe time we parse those tables and
>>> initialize the flash based on them.
>>
>> I disagree. It's not compatible with "jedec,spi-nor", which is
>> defined as
>>
> 
> cut
> 
>>
>> See my first reply, on how to possibly fix this mess (new
>> compatible if accepted, just use RDSFDP sequence which is backed by
>> the standard and do some fingerprinting).
>>
> 
> this won't work unless there's a unique parameter or ID in the sfdp or
> vendors tables, which I doubt. Takahiro to confirm.
> 
No, cyrs17b doesn't have it.

>> FWIW, a new (or rather different) compatible is needed because we
>> cannot distinguish between random data returned during the dummy
>> cycles and a proper manufacturer id. So there is no way we could fix
>> this in the core itself.
> 
> Yes, I agree, new compatible it is then.
> 
> cut
> 
>>> I think the property vs compatible decision resumes at whether we
>>> consider that the dummy cycles requirement for Read ID is/will be
>>> generic or not.
>>
>> It is not generic. Because it will break autodetection. And that is
>> the whole purpose of this. Adding that property means, we can just
>> autodetect flashes within this 'group'. And personally, I think this
>> is a bad precedent.
>>
> 
> yes, I agree.
> 
>>> I noticed that with higher frequencies or protocol modes (e.g, octal
>>> DTR), flashes tend to require more dummy cycles. I think with time,
>>> we'll have more flashes with such requirement. Takahiro can jump in and
>>> tell if it's already the case with IFX.
>>
>> But hopefully not with RDID. Again this doesn't play nice with other
>> flashes (or all flashes for now). Instead of adding random delay
>> cycles one should rather define a max clock speed for this opcode.
> 
> This could work, yes. But not for this flash. Or maybe encourage vendors
> to either contribute and enlarge the SFDP database or define their own
> vendor tables for all the flash properties that are not covered yet.
> It's strange how Block Protection is not yet covered by SFDP after all
> these years.
> 
> Thanks,
> ta