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

[Esben Haabendal <esben@geanix.com>]

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