Received-SPF: pass (google.com: domain of linux-kernel+bounces-204470-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <9e304860-b4e9-4193-9bb3-ebcc3611032f@linaro.org>
Date: Thu, 6 Jun 2024 15:27:34 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mtd: spi-nor: macronix: workaround for device id re-use
To: Michael Walle <mwalle@kernel.org>, Esben Haabendal <esben@geanix.com>,
 Pratyush Yadav <pratyush@kernel.org>,
 Miquel Raynal <miquel.raynal@bootlin.com>,
 Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
 Rasmus Villemoes <rasmus.villemoes@prevas.dk>
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>
Content-Language: en-US
From: Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <D1SZA4ZDM06P.CJC0EQ9ULA04@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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