Message-Id: <D8LSAUU0358V.2H1D7QXB9WBOF@kernel.org>
Date: Fri, 21 Mar 2025 09:00:18 +0100
From: "Michael Walle" <mwalle@...nel.org>
To: "Tudor Ambarus" <tudor.ambarus@...aro.org>, "Rob Herring"
 <robh@...nel.org>
Cc: "Takahiro Kuwano" <tkuw584924@...il.com>, "Pratyush Yadav"
 <pratyush@...nel.org>, "Miquel Raynal" <miquel.raynal@...tlin.com>,
 "Richard Weinberger" <richard@....at>, "Vignesh Raghavendra"
 <vigneshr@...com>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, <linux-mtd@...ts.infradead.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Bacem
 Daassi" <Bacem.Daassi@...ineon.com>, "Takahiro Kuwano"
 <Takahiro.Kuwano@...ineon.com>, "Mark Brown" <broonie@...nel.org>
Subject: Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

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

Powered by blists - more mailing lists