Received-SPF: pass (google.com: domain of linux-kernel+bounces-204817-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Esben Haabendal <esben@geanix.com>
To: "Michael Walle" <mwalle@kernel.org>
Cc: "Tudor Ambarus" <tudor.ambarus@linaro.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: <D1SZA4ZDM06P.CJC0EQ9ULA04@kernel.org> (Michael Walle's message
	of "Thu, 06 Jun 2024 15:45:20 +0200")
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>
Date: Thu, 06 Jun 2024 19:35:06 +0200
Message-ID: <87msny9dpx.fsf@geanix.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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