Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

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 v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP
Date: Thu, 11 Jul 2024 13:55:57 +0200	[thread overview]
Message-ID: <87zfqoqh0y.fsf@geanix.com> (raw)
In-Reply-To: <D2ML6RG09T38.RZU3CCH0XD9I@kernel.org> (Michael Walle's message of "Thu, 11 Jul 2024 11:02:39 +0200")

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

>> > I actually had the same concern. But currently there is no
>> > non-deprecated way to handle this case, right?
>> >
>> > Right now we have the following cases:
>> >  (1) pure SFDP parsing
>> >  (2) non-SFDP flashes with static configuration only
>> >  (3) legacy implementation, where the magic flags decide whether we
>> >      use SFDP
>>
>> Actually, in the code we have two variants of 2.
>>
>> (2a) non-SFDP flashes with SPI_NOR_SKIP_SFDP set
>> (2b) non-SFDP flashes without SPI_NOR_SKIP_SFDP and with none of the
>>      DUAL/QUAD/OCTAL read bits set
>
> Isn't (2b) my case (3)? The SPI_NOR_SKIP_SFDP flag was intended to
> be for flashes we know for a fact, there are no SFDP tables.
>
> I'm looking at spi_nor_init_params(). Maybe I'm missing something?

Probably not. I might just be confusing things here.

Your case (3) is conditional. Based on the magic flags checking in
spi_nor_init_params_deprecated(), it is either doing static
configuration only (this is what I tried to redefine as case (2b)) or
parsing SFDP with fallback to static configuration.

The issue I am getting at is that while the 2 different ways to end up
doing static configuration only is almost identical, there is a slight
difference.

But I will be addressing this in a v3 patch of this series.

Coming up shortly, and looking forward to discuss it.

/Esben

>> These almost handled the same way. But
>> spi_nor_manufacturer_init_params() is only called for 2b, and not for
>> 2a.
>>
>> Is this desired behavior, or something that we want to align?
>>
>> > Which case is eventually used depends on the ID of the flash -
>> > assuming there will only be IDs which either fall into (1) *or* (2).
>> > That assumption is clearly wrong :)
>> >
>> > I'd propose a new case in spi_nor_init_params()
>> >  (4) try SFDP with a fallback to the static flags from the
>> >      flash_info db.
>>
>> /Esben

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/