Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Esben Haabendal <esben@geanix.com>]

Erez <erezgeva2@gmail.com> writes:

> On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@kernel.org> wrote:
>> On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
>> > Yes, I think we should.
>> >
>> > Reading the specification provided publicly by Macronix.
>> > For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c
>> > All of them have a new version or a new chip with the same JEDEC ID
>> > that supports SFDP.
>> > There are 2 chips that Macronix does not provide spec. in public.
>> > I can ask Macronix technical support on these 2 chips.
>>
>> We don't add flashes we cannot test.
>
> I did not suggest adding anything new.
> I refer to the list of chips we already have in drivers/mtd/spi-nor/macronix.c
> I presume someone tested them before adding them to the list in the past.
> And probably the old chip did not have the SFDP table back then.
>
> What I checked with the chip specifications is that all Macronix chips
> since 2010 have SFDP.
>
> The situation today is that all Macronix chips that are NOT in the
> Macronix table work based on the SFDP table.
> But new chips that use a JEDEC found in the Macronix table, skip the
> SFDP table and use the setting of the old chip.

Not entirely true.

Those that entries in the Macronix table that has SPI_NOR_DUAL_READ
and/or SPI_NOR_QUAD_READ in no_sfdp_flags is caught by
the magic flags matching in spi_nor_init_params_deprecated() and will
have spi_nor_parse_sfdp() called from
spi_nor_sfdp_init_params_deprecated(). So flashes reusing ID for these
will have the SFDP tables parsed.

The rest of the entries in the Macronix table is not so lucky. When a
replacement chip (with the same ID) is used, it will not be configured
with the values found in SFDP table.

> So I suggest we read the SFDP table for all Macronix chips.

Based on their strategy of re-using flash ID, I think this is a sane
approach.

> Old Macronix chips that do not have SFDP will use the setting from the
> Macronix table. i.e backward compatible.
> While new chips which do have an SFDP table will work with the new
> setting we find in the table.

Yes, if we apply the new SPI_NOR_TRY_SFDP flag to the matching table
entries.

> Of course, we might have issues in parsing the SFDP table itself.
> So we fix them as developers report and send chip ID and part number
> with the SFDP table content.
> I do not see the point of "hiding" with the old setting.
> Anyhow, as we do not like the IDs table and keep it for backward-compatible,
> so it only makes sense we should use the SFDP table as much as possible.
>
> My check was to ensure Tudor that all Macronix chips have SFDP whether
> they are in the IDs table or not and we are not wasting a no-op on a
> chip which can not have an SFDP table.
>
> All I suggest is we add the new 'SPI_NOR_TRY_SFDP' flag, to all
> Macronix chips.. Which will try to read the SFDP to any Macronix chip.

Makes sense. But obviously comes with a risk as we won't be able to
test all chips for regression when doing that.

/Esben