[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87frtuigo3.fsf@geanix.com>
Date: Mon, 03 Jun 2024 10:17:32 +0200
From: Esben Haabendal <esben@...nix.com>
To: "Michael Walle" <mwalle@...nel.org>
Cc: "Tudor Ambarus" <tudor.ambarus@...aro.org>, "Pratyush Yadav"
<pratyush@...nel.org>, "Miquel Raynal" <miquel.raynal@...tlin.com>,
"Richard Weinberger" <richard@....at>, "Vignesh Raghavendra"
<vigneshr@...com>, <linux-mtd@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, "Rasmus Villemoes"
<rasmus.villemoes@...vas.dk>
Subject: Re: [PATCH] mtd: spi-nor: macronix: workaround for device id re-use
"Michael Walle" <mwalle@...nel.org> writes:
> Hi,
>
> On Fri May 24, 2024 at 12:48 PM CEST, Esben Haabendal wrote:
>> Macronix engineers apparantly do not understand the purpose of having
>> an ID actually identify the chip and its capabilities. Sigh.
>>
>> The original Macronix SPI NOR flash that identifies itself as 0xC22016
>> with RDID was MX25L3205D. This chip does not support SFDP, but does
>> support the 2READ command (1-2-2).
>>
>> When Macronix announced EoL for MX25L3205D, the recommended
>> replacement part was MX25L3206E, which conveniently also identifies
>> itself as 0xC22016. It does not support 2READ, but supports DREAD
>> (1-1-2) instead, and supports SFDP for discovering this.
>>
>> When Macronix announced EoL for MX25L3206E, the recommended
>> replacement part was MX25L3233F, which also identifies itself as
>> 0xC22016. It supports DREAD, 2READ, and the quad modes QREAD (1-1-4)
>> and 4READ (1-4-4). This also support SFDP.
>
> Thanks for collecting all this info!
>
>> So far, all of these chips have been handled the same way by the Linux
>> driver. The SFDP information have not been read, and no dual and quad
>> read modes have been enabled.
>>
>> The trouble begins when we want to enable the faster read modes. The
>> RDID command only return the same 3 bytes for all 3 chips, so that
>> doesn't really help.
>>
>> But we can take advantage of the fact that only the old MX25L3205D
>> chip does not support SFDP, so by triggering the old initialization
>> mechanism where we try to read and parse SFDP, but has a fall-back
>> configuration in place, we can configure all 3 chips to their optimal
>> configurations.
>
> You are (mis)using the quad info bits to trigger an sfdp read,
> correct?
Correct.
> In that case, I'd rather see a new flag in .no_sfdp_flags
> to explicitly trigger the SFDP read. Then your new flash would only
> need this flag and doesn't require the shenanigans with the fixup,
> right?
I fixup would still be required in order to enable 1-2-2 for MX25L3205D,
as it will fail the SFDP read, but actually does support 1-2-2.
>> With this, MX25L3205D will get the faster 2READ command enabled,
>> speading up reads. This should be safe.
>>
>> MX25L3206E will get the faster DREAD command enabled. This should also
>> be safe.
>>
>> MX25L3233F will get all of DREAD, 2READ, QREAD and 4READ enabled. In
>> order for this to actually work, the WP#/SIO2 and HOLD#/SIO3 pins must
>> be correctly wired to the SPI controller.
>
> That should already be taken care of with the spi-{tx,rx}-bus-width.
Yes. Which defaults to 1, which should take care of the backwards
compatibility.
/Esben
>> Signed-off-by: Esben Haabendal <esben@...nix.com>
>> ---
>> I only have access to boards with MX25L3233F flashes, so haven't been
>> able to test the backwards compatibility. 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.
>> ---
>> drivers/mtd/spi-nor/macronix.c | 60 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>> index ea6be95e75a5..c1e64ee3baa3 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -8,6 +8,63 @@
>>
>> #include "core.h"
>>
>> +/*
>> + * There is a whole sequence of chips from Macronix that uses the same device
>> + * id. These are recommended as EoL replacement parts by Macronix, although they
>> + * are only partly software compatible.
>> + *
>> + * Recommended replacement for MX25L3205D was MX25L3206E.
>> + * Recommended replacement for MX25L3206E was MX25L3233F.
>> + *
>> + * MX25L3205D does not support RDSFDP. The other two does.
>> + *
>> + * MX25L3205D supports 1-2-2 (2READ) command.
>> + * MX25L3206E supports 1-1-2 (DREAD) command.
>> + * MX25L3233F supports 1-1-2 (DREAD), 1-2-2 (2READ), 1-1-4 (QREAD), and 1-4-4
>> + * (4READ) commands.
>> + *
>> + * In order to trigger reading optional SFDP configuration, the
>> + * SPI_NOR_DUAL_READ|SPI_NOR_QUAD_READ flags are set, seemingly enabling 1-1-2
>> + * and 1-1-4 for MX25L3205D. The other chips supporting RDSFDP will have the
>> + * correct read commands configured based on SFDP information.
>> + *
>> + * As none of the other will enable 1-1-4 and NOT 1-4-4, so we identify
>> + * MX25L3205D when we see that.
>> + */
>> +static int
>> +mx25l3205d_late_init(struct spi_nor *nor)
>> +{
>> + struct spi_nor_flash_parameter *params = nor->params;
>> +
>> + /* DREAD 2READ QREAD 4READ
>> + * 1-1-2 1-2-2 1-1-4 1-4-4
>> + * Before SFDP parse 1 0 1 0
>> + * 3206e after SFDP parse 1 0 0 0
>> + * 3233f after SFDP parse 1 1 1 1
>> + * 3205d after this func 0 1 0 0
>> + */
>> + if ((params->hwcaps.mask & SNOR_HWCAPS_READ_1_1_4) &&
>> + !(params->hwcaps.mask & SNOR_HWCAPS_READ_1_4_4)) {
>> + /* Should be MX25L3205D */
>> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_2;
>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2],
>> + 0, 0, 0, 0);
>> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_1_1_4;
>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_4],
>> + 0, 0, 0, 0);
>> + params->hwcaps.mask |= SNOR_HWCAPS_READ_1_2_2;
>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_2_2],
>> + 0, 4, SPINOR_OP_READ_1_2_2,
>> + SNOR_PROTO_1_2_2);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct spi_nor_fixups mx25l3205d_fixups = {
>> + .late_init = mx25l3205d_late_init,
>> +};
>> +
>> static int
>> mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>> const struct sfdp_parameter_header *bfpt_header,
>> @@ -61,7 +118,8 @@ static const struct flash_info macronix_nor_parts[] = {
>> .id = SNOR_ID(0xc2, 0x20, 0x16),
>> .name = "mx25l3205d",
>> .size = SZ_4M,
>> - .no_sfdp_flags = SECT_4K,
>> + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>> + .fixups = &mx25l3205d_fixups
>> }, {
>> .id = SNOR_ID(0xc2, 0x20, 0x17),
>> .name = "mx25l6405d",
>>
>> ---
>> base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
>> change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7
>>
>> Best regards,
Powered by blists - more mailing lists