mtd: spi-nor: macronix: workaround for device id re-use

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.

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.

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.

Signed-off-by: Esben Haabendal <esben@geanix.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(-)


---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7

Best regards,

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? 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?

> 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.

-michael

> Signed-off-by: Esben Haabendal <esben@geanix.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(&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",
>
> ---
> base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
> change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7
>
> Best regards,

"Michael Walle" <mwalle@kernel.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@geanix.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(&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",
>>
>> ---
>> base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
>> change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7
>>
>> Best regards,

Hi,

> > 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.

But you (probably) don't care and we also don't care for the
additional speed. So, I'd rather drop that code that is just there
for that ancient EoL flash.

-michael

"Michael Walle" <mwalle@kernel.org> writes:

> Hi,
>
>> > 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.
>
> But you (probably) don't care and we also don't care for the
> additional speed. So, I'd rather drop that code that is just there
> for that ancient EoL flash.

Fine with me. Because you are right, I don't personally care. I don't
have any boards with the ancient 3205d part.

/Esben

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?
>>
>> 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.

> 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.

> That should already be taken care of with the spi-{tx,rx}-bus-width.
> 
> -michael
> 
>> Signed-off-by: Esben Haabendal <esben@geanix.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.

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.

Cheers,
ta

> >> + */
> >> +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.

It's a horrible hack. For example I'm working on a patch to clean up
the spi_nor_set_read_settings() handling. So just throwing any code
into vendor drivers doesn't make it any better in terms of
maintainability. I'd need to touch all the code anyway. In fact it
makes it even worse, because it looks like the manufacturer drivers
are just a dumping ground for bad things. Thus, I'd really have it
handled in a correct way inside the core.

Also, this is not device specific. Let there be two different
flashes with the same ID, but one support SFDP and one doesn't.
Right now, you have to have any of the magic flags (dual, quad,
etc) set to trigger an SFDP parsing. If the flash without SFDP
doesn't support any of these, like in this case, we are screwed.
Hence we might need such a flag also for other flashes.

> 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.

Well, this will break boards with this flash :) And we don't know if
there are any.

-michael

On 6/6/24 14:45, Michael Walle wrote:
>>>> + */
>>>> +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.
> 
> It's a horrible hack. For example I'm working on a patch to clean up
> the spi_nor_set_read_settings() handling. So just throwing any code
> into vendor drivers doesn't make it any better in terms of
> maintainability. I'd need to touch all the code anyway. In fact it
> makes it even worse, because it looks like the manufacturer drivers
> are just a dumping ground for bad things. Thus, I'd really have it
> handled in a correct way inside the core.
> 
> Also, this is not device specific. Let there be two different
> flashes with the same ID, but one support SFDP and one doesn't.
> Right now, you have to have any of the magic flags (dual, quad,
> etc) set to trigger an SFDP parsing. If the flash without SFDP
> doesn't support any of these, like in this case, we are screwed.
> Hence we might need such a flag also for other flashes.

maybe. How many such flashes have you seen in the last 3 years?
> 
>> 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.
> 
> Well, this will break boards with this flash :) And we don't know if
> there are any.

The flash (MX25L3205D) was already deprecated in two iterations by the
manufacturer. First migration being done in 2011 [1]. Having to maintain
all flavors is a pain, thus let's remove support for the old flash. If
anyone complains we can bring it back to life, but let's not complicate
our existence yet.

[1]
https://www.mxic.com.tw/Lists/ApplicationNote/Attachments/1858/AN058-Migrating%20from%20MX25L3205D%20to%20MX25L3206E-1.2.pdf

Tudor Ambarus <tudor.ambarus@linaro.org> 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@geanix.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.
>
> 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

"Michael Walle" <mwalle@kernel.org> writes:

>> >> + */
>> >> +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.
>
> It's a horrible hack. For example I'm working on a patch to clean up
> the spi_nor_set_read_settings() handling. So just throwing any code
> into vendor drivers doesn't make it any better in terms of
> maintainability. I'd need to touch all the code anyway. In fact it
> makes it even worse, because it looks like the manufacturer drivers
> are just a dumping ground for bad things. Thus, I'd really have it
> handled in a correct way inside the core.
>
> Also, this is not device specific. Let there be two different
> flashes with the same ID, but one support SFDP and one doesn't.
> Right now, you have to have any of the magic flags (dual, quad,
> etc) set to trigger an SFDP parsing. If the flash without SFDP
> doesn't support any of these, like in this case, we are screwed.
> Hence we might need such a flag also for other flashes.

Also, it is not very obvious that you can trigger SFDP parsing by
setting these dual/quad bits.  Having an explicit flag to cause this
behvaiour would be much better IMHO.

>> 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.
>
> Well, this will break boards with this flash :) And we don't know if
> there are any.

Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> On 6/6/24 14:45, Michael Walle wrote:
>>>>> + */
>>>>> +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.
>> 
>> It's a horrible hack. For example I'm working on a patch to clean up
>> the spi_nor_set_read_settings() handling. So just throwing any code
>> into vendor drivers doesn't make it any better in terms of
>> maintainability. I'd need to touch all the code anyway. In fact it
>> makes it even worse, because it looks like the manufacturer drivers
>> are just a dumping ground for bad things. Thus, I'd really have it
>> handled in a correct way inside the core.
>> 
>> Also, this is not device specific. Let there be two different
>> flashes with the same ID, but one support SFDP and one doesn't.
>> Right now, you have to have any of the magic flags (dual, quad,
>> etc) set to trigger an SFDP parsing. If the flash without SFDP
>> doesn't support any of these, like in this case, we are screwed.
>> Hence we might need such a flag also for other flashes.
>
> maybe. How many such flashes have you seen in the last 3 years?

How big a procentage of embedded Linux hardware do we have a realistic
chance to know anything about?

>>> 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.
>> 
>> Well, this will break boards with this flash :) And we don't know if
>> there are any.
>
> The flash (MX25L3205D) was already deprecated in two iterations by the
> manufacturer. First migration being done in 2011 [1]. Having to maintain
> all flavors is a pain, thus let's remove support for the old flash. If
> anyone complains we can bring it back to life, but let's not complicate
> our existence yet.
>
> [1]
> https://www.mxic.com.tw/Lists/ApplicationNote/Attachments/1858/AN058-Migrating%20from%20MX25L3205D%20to%20MX25L3206E-1.2.pdf

How should we communicate the removal of support for MX25L3205D?
And should we then rename the entry to "mx23l3206e"?

/Esben