mbox series

[v3,00/15] mtd: spi-nor: macronix: workaround for device id re-use

Message ID 20240711-macronix-mx25l3205d-fixups-v3-0-99353461dd2d@geanix.com (mailing list archive)
Headers show
Series mtd: spi-nor: macronix: workaround for device id re-use | expand

Message

Esben Haabendal July 11, 2024, 1 p.m. UTC
Following up to various discussions, this series have now been
modified so that it gets rid of the old deprecated approach
for detecting when to do optional SFDP parsing.

Before these changes, spi-nor flashes were handled in 4 different
ways:

(1) SFDP only [size==0]

(2a) static config only [size!=0 && no_sfdp_flags & SPI_NOR_SKIP_SFDP]

(2b) static config only
       [size!=0 &&
        !(no_sfdp_flags & (SPI_NOR_SKIP_SFDP | 
           SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
           SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]

(3) SFDP with fallback to static config
       [size!=0 &&
        !(no_sfdp_flags & SPI_NOR_SKIP_SFDP) &&
        (no_sfdp_flags & SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
           SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]

Cases (2a) and (2b) have been handled slightly different, with
manufacturer and flash_info ->default_init() hooks being called in
case (2b) but not in case (2a).

With this series, that is changed to this simpler approach instead:

(1) SFDP only [size==0]

(2) static config only
      [size!=0 && !(no_sfdp_flags & SPI_NOR_TRY_SFDP)]

(3) SFDP with fallback to static config
      [size!=0 && (no_sfdp_flags & SPI_NOR_TRY_SFDP)]

Existing struct flash_info entries are modified, so that all those
that was case (2a) or (2b) are now case (2), and those that were (1)
and (3) are still (1) and (3).

As a consequence, the SPI_NOR_SKIP_SFDP flag is no more, and all
drivers that have been doing optional SFDP is now marked explicitly to
do that using the SPI_NOR_TRY_SFDP.

It is possible that some of these flashes does not really need to try
SFDP parsing, but as I am unable to test such changes, I will have to
leave that up to someone with access to hardware to do that. The
patches for doing that will be trivial, it is only a matter of testing.

As for the name of the flag, I guess that is still up for discussion.
I think SPI_NOR_TRY_SFDP explains pretty well what is the purpose, but
I am not really that attached to it.

As for the change in macronix.c for the mx25l3205d flash_info entry,
I only have access to boards with MX25L3233F flashes, so haven't been
able to test the backwards compatibility with the old MX25L3205D and
MX25L3206E flashes sharing flash ID with MX25L3233F. If anybody has
boards with MX25L3205D and/or MX25L3206E, please help test this patch.
Keep an eye for read performance regression.

It is worth nothing that both MX25L3205D and MX25L3206E are
end-of-life, and is unavailable from Macronix, so any new boards
featuring a Macronix flash with this ID will likely be using
MX25L3233F.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v3:
- Revised patch 1 in preparation for implementation of the new way of
  detecting when to do optiona SFDP, as described above.
- Added patch 3 that aligns the handling of default_init() hooks to be
  the same for case (2a) and case (2b) described above.
- Added patches for all spi-nor flash drivers to apply the
  SPI_NOR_TRY_SFDP flag instead of relying on the "magic flags"
  detection.
- Added patch dropping the deprecated "magic flags" code. This depends
  on the merging of all the patches applying SPI_NOR_TRY_SFDP to
  flash_info entries.
- Added patch dropping the SPI_NOR_SKIP_SFDP flag.
- Link to v2: https://lore.kernel.org/r/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com

Changes in v2:
- Added new flag (SPI_NOR_TRY_SFDP) to spi-nor core to allow trying
  SFDP and fallback to legacy parameters without having to specify
  DUAL/QUAD parameters.
- Rewrite macronix to use SPI_NOR_TRY_SFDP flag.
- Use with the ancient EoL MX25L3205D chip will not get speed
  increase, but stay at 1-bit mode as it is now.
- Link to v1: https://lore.kernel.org/r/20240524-macronix-mx25l3205d-fixups-v1-1-ee152e56afb3@geanix.com

---
Esben Haabendal (15):
      mtd: spi-nor: core: add flag for doing optional SFDP parsing
      mtd: spi-nor: macronix: enable quad/dual speed for mx25l3205d chips
      mtd: spi-nor: Align default_init() handling for SPI_NOR_SKIP_SFDP
      mtd: spi-nor: atmel: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: eon: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: gigadevice: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: issi: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: macronix: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: micron-st: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: spansion: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: sst: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: winbond: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: xmc: Use new SPI_NOR_TRY_SFDP flag
      mtd: spi-nor: Drop deprecated mechanism for optional SFDP parsing
      mtd: spi-nor: spansion: Drop redundant SPI_NOR_SKIP_SFDP flag

 drivers/mtd/spi-nor/atmel.c      |  2 +-
 drivers/mtd/spi-nor/core.c       | 48 ++++++++++++----------------------------
 drivers/mtd/spi-nor/core.h       | 19 +++++++++++++---
 drivers/mtd/spi-nor/eon.c        |  6 ++---
 drivers/mtd/spi-nor/gigadevice.c | 16 +++++++-------
 drivers/mtd/spi-nor/issi.c       | 18 +++++++--------
 drivers/mtd/spi-nor/macronix.c   | 26 +++++++++++-----------
 drivers/mtd/spi-nor/micron-st.c  | 41 +++++++++++++++++-----------------
 drivers/mtd/spi-nor/spansion.c   | 46 +++++++++++++++++++-------------------
 drivers/mtd/spi-nor/sst.c        |  6 ++---
 drivers/mtd/spi-nor/winbond.c    | 32 +++++++++++++--------------
 drivers/mtd/spi-nor/xmc.c        |  4 ++--
 12 files changed, 129 insertions(+), 135 deletions(-)
---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7

Best regards,

Comments

Michael Walle July 12, 2024, 9:55 a.m. UTC | #1
Hi,

On Thu Jul 11, 2024 at 3:00 PM CEST, Esben Haabendal wrote:
> As a consequence, the SPI_NOR_SKIP_SFDP flag is no more, and all
> drivers that have been doing optional SFDP is now marked explicitly to
> do that using the SPI_NOR_TRY_SFDP.

First, I haven't looked at your patchset at the moment. But I'd like
to take the opportunity to discuss the following (and excuse me that
I didn't had this idea before all your work on this).

First, I'd like to see it the other way around, that is, doing SFDP
by default and let the driver opt-out instead of opt-in. This will
also align with the current "SFDP only driver", i.e. if a flash is
not known we try SFDP anyway. Going forward, I'd say this is also
the sane behavior and we don't have to add any flags if someone want
to add support for an (older) flash with the same ID but without
SFDP. With the current approach, we'd have to add the TRY_SFDP flag.

Now we might play it safe and add that SPI_NOR_SKIP_SFDP to any
flash which doesn't do the SFDP parsing (because it has size != 0
and not any of the magic flags set) - or we might just go ahead and
do the probing first for *any* flashes. Yes we might issue an
unsupported opcode, but we already do that with the generic SFDP
flash driver. So no big deal maybe (?). Vendors where we know for a
fact that they don't have any SFDP (Everspin I'm looking at you),
would of course set the SKIP_SFDP flag.

-michael
Esben Haabendal Sept. 26, 2024, 7:56 a.m. UTC | #2
"Michael Walle" <mwalle@kernel.org> writes:

> Hi,
>
> On Thu Jul 11, 2024 at 3:00 PM CEST, Esben Haabendal wrote:
>> As a consequence, the SPI_NOR_SKIP_SFDP flag is no more, and all
>> drivers that have been doing optional SFDP is now marked explicitly to
>> do that using the SPI_NOR_TRY_SFDP.
>
> First, I haven't looked at your patchset at the moment. But I'd like
> to take the opportunity to discuss the following (and excuse me that
> I didn't had this idea before all your work on this).
>
> First, I'd like to see it the other way around, that is, doing SFDP
> by default and let the driver opt-out instead of opt-in. This will
> also align with the current "SFDP only driver", i.e. if a flash is
> not known we try SFDP anyway. Going forward, I'd say this is also
> the sane behavior and we don't have to add any flags if someone want
> to add support for an (older) flash with the same ID but without
> SFDP. With the current approach, we'd have to add the TRY_SFDP flag.
>
> Now we might play it safe and add that SPI_NOR_SKIP_SFDP to any
> flash which doesn't do the SFDP parsing (because it has size != 0
> and not any of the magic flags set) - or we might just go ahead and
> do the probing first for *any* flashes. Yes we might issue an
> unsupported opcode, but we already do that with the generic SFDP
> flash driver. So no big deal maybe (?). Vendors where we know for a
> fact that they don't have any SFDP (Everspin I'm looking at you),
> would of course set the SKIP_SFDP flag.

I like your idea.

Did you discuss this with Tudor?

On 9/23/24 7:04 PM, Tudor Ambarus wrote:
>>> * Always read Macronix chips SFDP, as Macronix replaced all old chips
>>> in the Manufacture table.
>>
>> I'll NACK it unless you prove that the old flavors of flashes are not
>> used anymore in the kernel.
>
> Even if you can prove that the older flashes are not used in the kernel
> anymore, we can't just switch to parsing SFDP, because we have seen in
> the past flashes with wrong SFDP data that made the flashes misbehave.
> The recommended way is to update just the flashes that you can test.

Does it make sense if I work on a patch implementing the proposal you
put forward, or is it possible to discuss it further before doing that
work?

If it will certainly be NACK'ed, I might as well try to find a different
approach.

/Esben
Tudor Ambarus Sept. 26, 2024, 10:08 a.m. UTC | #3
Hiya, Esben,

On 7/11/24 2:00 PM, Esben Haabendal wrote:
> Following up to various discussions, this series have now been
> modified so that it gets rid of the old deprecated approach
> for detecting when to do optional SFDP parsing.
> 
> Before these changes, spi-nor flashes were handled in 4 different
> ways:
> 

I'm adding a bit of extra context on each point you made. All your
points contain static init, you missed spi_nor_init_default_params()

There's a 0/ case where we have indeed just SFDP initialized flashes,
and that is the generic flash driver.

0/ SFDP only, generic flash driver

> (1) SFDP only [size==0]

1/ static + SFDP

spi_nor_init_default_params(nor);
spi_nor_parse_sfdp();

> 
> (2a) static config only [size!=0 && no_sfdp_flags & SPI_NOR_SKIP_SFDP]

2a/ is
spi_nor_init_default_params(nor);
spi_nor_no_sfdp_init_params(nor);

> 
> (2b) static config only
>        [size!=0 &&
>         !(no_sfdp_flags & (SPI_NOR_SKIP_SFDP | 
>            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]

2b/ is
spi_nor_init_default_params(nor);
spi_nor_init_params_deprecated(nor); //where parse SFDP is not called
	spi_nor_no_sfdp_init_params(nor);
	spi_nor_manufacturer_init_params(nor);>
> (3) SFDP with fallback to static config
>        [size!=0 &&
>         !(no_sfdp_flags & SPI_NOR_SKIP_SFDP) &&
>         (no_sfdp_flags & SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
> 

3/ is 2b/ with parse SFDP called and rollback mechanism
spi_nor_init_default_params(nor);
spi_nor_init_params_deprecated(nor);
	spi_nor_no_sfdp_init_params(nor);
	spi_nor_manufacturer_init_params(nor);
	spi_nor_sfdp_init_params_deprecated(nor);

All cases from above are followed by a call to spi_nor_late_init_params().

> Cases (2a) and (2b) have been handled slightly different, with
> manufacturer and flash_info ->default_init() hooks being called in
> case (2b) but not in case (2a).

default_init() was a mistake and we shall remove it and replace it with
late_init(). The challenge is to do that without affecting backwards
compatibility. But let's move this aside for the moment
> 
> With this series, that is changed to this simpler approach instead:
> 
> (1) SFDP only [size==0]
> 
> (2) static config only
>       [size!=0 && !(no_sfdp_flags & SPI_NOR_TRY_SFDP)]
> 
> (3) SFDP with fallback to static config
>       [size!=0 && (no_sfdp_flags & SPI_NOR_TRY_SFDP)]
> 
> Existing struct flash_info entries are modified, so that all those
> that was case (2a) or (2b) are now case (2), and those that were (1)
> and (3) are still (1) and (3).

We indeed want 2a/ and 2b/ to be squashed, ideally by removing the
default_init() hook.

And if we really want SFDP-only init, we shall not call
spi_nor_init_default_params() in this case.

Your patches do not apply on latest SPI NOR, I tried locally. Can you
point me to a branch of yours where I can see them? No need to resend.

Thanks,
ta
Tudor Ambarus Sept. 26, 2024, 10:47 a.m. UTC | #4
Hi, Esben,

Thank you for the perseverance :D

On 9/26/24 8:56 AM, Esben Haabendal wrote:
> "Michael Walle" <mwalle@kernel.org> writes:
> 
>> Hi,
>>
>> On Thu Jul 11, 2024 at 3:00 PM CEST, Esben Haabendal wrote:
>>> As a consequence, the SPI_NOR_SKIP_SFDP flag is no more, and all
>>> drivers that have been doing optional SFDP is now marked explicitly to
>>> do that using the SPI_NOR_TRY_SFDP.
>>
>> First, I haven't looked at your patchset at the moment. But I'd like
>> to take the opportunity to discuss the following (and excuse me that
>> I didn't had this idea before all your work on this).
>>
>> First, I'd like to see it the other way around, that is, doing SFDP
>> by default and let the driver opt-out instead of opt-in. This will
>> also align with the current "SFDP only driver", i.e. if a flash is
>> not known we try SFDP anyway. Going forward, I'd say this is also
>> the sane behavior and we don't have to add any flags if someone want
>> to add support for an (older) flash with the same ID but without
>> SFDP. With the current approach, we'd have to add the TRY_SFDP flag.
>>
>> Now we might play it safe and add that SPI_NOR_SKIP_SFDP to any
>> flash which doesn't do the SFDP parsing (because it has size != 0
>> and not any of the magic flags set) - or we might just go ahead and
>> do the probing first for *any* flashes. Yes we might issue an
>> unsupported opcode, but we already do that with the generic SFDP
>> flash driver. So no big deal maybe (?). Vendors where we know for a
>> fact that they don't have any SFDP (Everspin I'm looking at you),
>> would of course set the SKIP_SFDP flag.

Issuing RDSFDP for flashes that don't support it shouldn't be too bad
indeed, it's not recommended, but it shall be fine. What I'm worried
about is flashes with wrong SFDP data (see all the SFDP fixup hooks that
we have). So my suggestion is to play it safe unless one of you guys
step up and commit that will fix or help fix each bug that results from
this.

I'd like you to also consider the SFDP versions and how many parameters
they are exposing. I can't tell exactly, but if I remember correctly,
rev A had 9 dwords for BFPT, revB 16, and revC and other more. If we
consider static init folowed by SFDP with rollback option, point 3/ in
your cover letter, are we sure that all the parameters that are
initialized before parsing SFDP are overwritten at SFDP parsing time? If
yes, we shall be fine.

Esben, to speed up the things on both ends, I recommend you for v4 to
draft just a core patch if you care, then you can handle the vendor
drivers. Or send us some pseudocode and we can talk on that.

Thanks,
ta
Tudor Ambarus Sept. 26, 2024, 10:52 a.m. UTC | #5
On 9/26/24 11:47 AM, Tudor Ambarus wrote:
> Hi, Esben,
> 
> Thank you for the perseverance :D
> 
> On 9/26/24 8:56 AM, Esben Haabendal wrote:
>> "Michael Walle" <mwalle@kernel.org> writes:
>>
>>> Hi,
>>>
>>> On Thu Jul 11, 2024 at 3:00 PM CEST, Esben Haabendal wrote:
>>>> As a consequence, the SPI_NOR_SKIP_SFDP flag is no more, and all
>>>> drivers that have been doing optional SFDP is now marked explicitly to
>>>> do that using the SPI_NOR_TRY_SFDP.
>>>
>>> First, I haven't looked at your patchset at the moment. But I'd like
>>> to take the opportunity to discuss the following (and excuse me that
>>> I didn't had this idea before all your work on this).
>>>
>>> First, I'd like to see it the other way around, that is, doing SFDP
>>> by default and let the driver opt-out instead of opt-in. This will
>>> also align with the current "SFDP only driver", i.e. if a flash is
>>> not known we try SFDP anyway. Going forward, I'd say this is also
>>> the sane behavior and we don't have to add any flags if someone want
>>> to add support for an (older) flash with the same ID but without
>>> SFDP. With the current approach, we'd have to add the TRY_SFDP flag.
>>>
>>> Now we might play it safe and add that SPI_NOR_SKIP_SFDP to any
>>> flash which doesn't do the SFDP parsing (because it has size != 0
>>> and not any of the magic flags set) - or we might just go ahead and
>>> do the probing first for *any* flashes. Yes we might issue an
>>> unsupported opcode, but we already do that with the generic SFDP
>>> flash driver. So no big deal maybe (?). Vendors where we know for a
>>> fact that they don't have any SFDP (Everspin I'm looking at you),
>>> would of course set the SKIP_SFDP flag.
> 
> Issuing RDSFDP for flashes that don't support it shouldn't be too bad
> indeed, it's not recommended, but it shall be fine. What I'm worried
> about is flashes with wrong SFDP data (see all the SFDP fixup hooks that
> we have). So my suggestion is to play it safe unless one of you guys
> step up and commit that will fix or help fix each bug that results from
> this.
> 
> I'd like you to also consider the SFDP versions and how many parameters
> they are exposing. I can't tell exactly, but if I remember correctly,
> rev A had 9 dwords for BFPT, revB 16, and revC and other more. If we
> consider static init folowed by SFDP with rollback option, point 3/ in
> your cover letter, are we sure that all the parameters that are
> initialized before parsing SFDP are overwritten at SFDP parsing time? If
> yes, we shall be fine.
> 
> Esben, to speed up the things on both ends, I recommend you for v4 to
> draft just a core patch if you care, then you can handle the vendor
> drivers. Or send us some pseudocode and we can talk on that.
> 

To summarize, I like the idea too, as far as we can keep backward
compatibility.
Esben Haabendal Sept. 26, 2024, 11:01 a.m. UTC | #6
Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> Your patches do not apply on latest SPI NOR, I tried locally. Can you
> point me to a branch of yours where I can see them? No need to resend.

Sure. I have rebased to spi-nor/next and pushed the b4:
https://github.com/esben/linux/tree/b4/macronix-mx25l3205d-fixups

/Esben
Esben Haabendal Sept. 26, 2024, 11:35 a.m. UTC | #7
Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> Hiya, Esben,
>
> On 7/11/24 2:00 PM, Esben Haabendal wrote:
>> Following up to various discussions, this series have now been
>> modified so that it gets rid of the old deprecated approach
>> for detecting when to do optional SFDP parsing.
>> 
>> Before these changes, spi-nor flashes were handled in 4 different
>> ways:
>> 
>
> I'm adding a bit of extra context on each point you made. All your
> points contain static init, you missed spi_nor_init_default_params()

I might be even more confused now :) Let me try to understand...

> There's a 0/ case where we have indeed just SFDP initialized flashes,
> and that is the generic flash driver.
>
> 0/ SFDP only, generic flash driver

Ok. So this is when spi_nor_detect() falls back to
spi_nor_generic_flash, right?

>> (1) SFDP only [size==0]
>
> 1/ static + SFDP

So we have found a matching flash_info, and it has size==0. Right?

> spi_nor_init_default_params(nor);

I guess we can assume that flash_info in this case will always have
something set (other than .name and .size=0)?

> spi_nor_parse_sfdp();

>> (2a) static config only [size!=0 && no_sfdp_flags & SPI_NOR_SKIP_SFDP]
>
> 2a/ is
> spi_nor_init_default_params(nor);
> spi_nor_no_sfdp_init_params(nor);

Got it.

>> (2b) static config only
>>        [size!=0 &&
>>         !(no_sfdp_flags & (SPI_NOR_SKIP_SFDP | 
>>            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
>
> 2b/ is
> spi_nor_init_default_params(nor);
> spi_nor_init_params_deprecated(nor); //where parse SFDP is not called
> 	spi_nor_no_sfdp_init_params(nor);
> 	spi_nor_manufacturer_init_params(nor);>

Got it.

>> (3) SFDP with fallback to static config
>>        [size!=0 &&
>>         !(no_sfdp_flags & SPI_NOR_SKIP_SFDP) &&
>>         (no_sfdp_flags & SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
>> 
>
> 3/ is 2b/ with parse SFDP called and rollback mechanism
> spi_nor_init_default_params(nor);
> spi_nor_init_params_deprecated(nor);
> 	spi_nor_no_sfdp_init_params(nor);
> 	spi_nor_manufacturer_init_params(nor);
> 	spi_nor_sfdp_init_params_deprecated(nor);
>
> All cases from above are followed by a call to spi_nor_late_init_params().

Got it.

Should I move your annotations to the cover letter? If I find it
helpful, I think it might also be helpful to somebody else :)

>> Cases (2a) and (2b) have been handled slightly different, with
>> manufacturer and flash_info ->default_init() hooks being called in
>> case (2b) but not in case (2a).
>
> default_init() was a mistake and we shall remove it and replace it with
> late_init(). The challenge is to do that without affecting backwards
> compatibility. But let's move this aside for the moment
>> 
>> With this series, that is changed to this simpler approach instead:
>> 
>> (1) SFDP only [size==0]
>> 
>> (2) static config only
>>       [size!=0 && !(no_sfdp_flags & SPI_NOR_TRY_SFDP)]
>> 
>> (3) SFDP with fallback to static config
>>       [size!=0 && (no_sfdp_flags & SPI_NOR_TRY_SFDP)]
>> 
>> Existing struct flash_info entries are modified, so that all those
>> that was case (2a) or (2b) are now case (2), and those that were (1)
>> and (3) are still (1) and (3).
>
> We indeed want 2a/ and 2b/ to be squashed, ideally by removing the
> default_init() hook.

But you think we should not remove default_init() hook as part of this
series?

> And if we really want SFDP-only init, we shall not call
> spi_nor_init_default_params() in this case.

So move spi_nor_init_default_params() into the if (spi_nor_needs_sfdp())
else section?
I have pushed a fixup commit doing this to my branch. Should I amend
patch 1/15 with it?

/Esben
Esben Haabendal Sept. 26, 2024, 12:18 p.m. UTC | #8
Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> Hi, Esben,
>
> Thank you for the perseverance :D
>
> On 9/26/24 8:56 AM, Esben Haabendal wrote:
>> "Michael Walle" <mwalle@kernel.org> writes:
>> 
>>> Hi,
>>>
>>> On Thu Jul 11, 2024 at 3:00 PM CEST, Esben Haabendal wrote:
>>>> As a consequence, the SPI_NOR_SKIP_SFDP flag is no more, and all
>>>> drivers that have been doing optional SFDP is now marked explicitly to
>>>> do that using the SPI_NOR_TRY_SFDP.
>>>
>>> First, I haven't looked at your patchset at the moment. But I'd like
>>> to take the opportunity to discuss the following (and excuse me that
>>> I didn't had this idea before all your work on this).
>>>
>>> First, I'd like to see it the other way around, that is, doing SFDP
>>> by default and let the driver opt-out instead of opt-in. This will
>>> also align with the current "SFDP only driver", i.e. if a flash is
>>> not known we try SFDP anyway. Going forward, I'd say this is also
>>> the sane behavior and we don't have to add any flags if someone want
>>> to add support for an (older) flash with the same ID but without
>>> SFDP. With the current approach, we'd have to add the TRY_SFDP flag.
>>>
>>> Now we might play it safe and add that SPI_NOR_SKIP_SFDP to any
>>> flash which doesn't do the SFDP parsing (because it has size != 0
>>> and not any of the magic flags set) - or we might just go ahead and
>>> do the probing first for *any* flashes. Yes we might issue an
>>> unsupported opcode, but we already do that with the generic SFDP
>>> flash driver. So no big deal maybe (?). Vendors where we know for a
>>> fact that they don't have any SFDP (Everspin I'm looking at you),
>>> would of course set the SKIP_SFDP flag.
>
> Issuing RDSFDP for flashes that don't support it shouldn't be too bad
> indeed, it's not recommended, but it shall be fine. What I'm worried
> about is flashes with wrong SFDP data (see all the SFDP fixup hooks that
> we have). So my suggestion is to play it safe unless one of you guys
> step up and commit that will fix or help fix each bug that results from
> this.

Sounds like a good idea. I for one will not put my head on the table
like that :D

> I'd like you to also consider the SFDP versions and how many parameters
> they are exposing. I can't tell exactly, but if I remember correctly,
> rev A had 9 dwords for BFPT, revB 16, and revC and other more. If we
> consider static init folowed by SFDP with rollback option, point 3/ in
> your cover letter, are we sure that all the parameters that are
> initialized before parsing SFDP are overwritten at SFDP parsing time? If
> yes, we shall be fine.

How can we be sure of that?

But if you want to be really sure that no static configuraion is left
after SFDP parsing, why are we handing it a copy of nor->params that was
initialized from static configuration?

Why not call spi_nor_parse_sfdp() with nor->params bing basically
uninitialized?

And then, only if spi_nor_parse_sfdp() fails, look into flash_info
static configuration?

> Esben, to speed up the things on both ends, I recommend you for v4 to
> draft just a core patch if you care, then you can handle the vendor
> drivers. Or send us some pseudocode and we can talk on that.

I will see what I can do.

I guess some kind of RFC patch of the core changes for v4 might be a
good next step.

/Esben