Re: [PATCH] mtd: spi-nor: macronix: workaround for device id re-use

[Esben Haabendal]

Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> writes:

> On 6/3/24 08:25, Michael Walle wrote:
>> 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).
>
> and it lacks support for 1-1-2?

Yes.

>>> 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,
>
> right, not ideal.
>
>> 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
>
> I hate to update the core for vendor's madness.

Me too. But on the other hand, it would be ashame to not support such
common parts.

>> need this flag and doesn't require the shenanigans with the fixup,
>> right?
>> 
>>> 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.
>> 
> don't add superfluous info. we already know how quad works.

Noted. And I already did drop that part in v2.

>> That should already be taken care of with the spi-{tx,rx}-bus-width.
>> 
>> -michael
>> 
>>> Signed-off-by: Esben Haabendal <esben@xxxxxxxxxx>
>>> ---
>>> 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.
>
> I find this description more clear than the commit message. I've written
> some questions for the commit message, then I removed them once I read
> this description.
>
>>> + */
>>> +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(&params->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(&params->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(&params->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",
>>>
>
> If all support 1-1-2, (seems MX25L3205D doesn't), then we may have a
> change to don't update the core.
>
> Frankly I don't care too much about what happens in the manufacturer
> drivers, but I do care about the core and to not extend it with . This
> patch is not too heavy to be unmaintainable and shows clear where the
> problem is, we can keep this as well.
>
> Other option that I'd like you to consider is whether we just remove
> support for MX25L3205D, thus the entry altogether, and instead rely on
> SFDP to set everything.

I won't mind for the hardware I am involved with. But on the
other hand, I personally don't think it is right to cause problems for
anyone upgrading the kernel to boards using MX25L3205D. But I will leave
that to you maintainers, as you both have to bear the maintance burden
and will be the ones to get the blame if the change upsets someone :)

/Esben