Received-SPF: pass (google.com: domain of linux-kernel+bounces-204823-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Esben Haabendal <esben@geanix.com>
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: Michael Walle <mwalle@kernel.org>,  Pratyush Yadav
 <pratyush@kernel.org>,  Miquel Raynal <miquel.raynal@bootlin.com>,
  Richard Weinberger <richard@nod.at>,  Vignesh Raghavendra
 <vigneshr@ti.com>,  linux-mtd@lists.infradead.org,
  linux-kernel@vger.kernel.org,  Rasmus Villemoes
 <rasmus.villemoes@prevas.dk>
Subject: Re: [PATCH] mtd: spi-nor: macronix: workaround for device id re-use
In-Reply-To: <9e304860-b4e9-4193-9bb3-ebcc3611032f@linaro.org> (Tudor
	Ambarus's message of "Thu, 6 Jun 2024 15:27:34 +0100")
References: <20240524-macronix-mx25l3205d-fixups-v1-1-ee152e56afb3@geanix.com>
	<D1Q7BU6PJ356.1CTXPUZE8U6XX@kernel.org>
	<8513a828-6669-4bf3-91d3-799771866f32@linaro.org>
	<D1SZA4ZDM06P.CJC0EQ9ULA04@kernel.org>
	<9e304860-b4e9-4193-9bb3-ebcc3611032f@linaro.org>
Date: Thu, 06 Jun 2024 19:36:58 +0200
Message-ID: <87ikym9dmt.fsf@geanix.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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