Received-SPF: pass (google.com: domain of linux-kernel+bounces-204787-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: 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 v2 1/2] mtd: spi-nor: core: add flag for doing optional
 SFDP
In-Reply-To: <48719b0f-1a7f-47f9-948a-c981a0a29b41@linaro.org> (Tudor
	Ambarus's message of "Thu, 6 Jun 2024 15:52:35 +0100")
References: <20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com>
	<20240603-macronix-mx25l3205d-fixups-v2-1-ff98da26835c@geanix.com>
	<a379a411-2c9e-4d9d-aa8f-4c4f3463cc27@linaro.org>
	<D1SZKLZBDDBA.1Z7ZD4UEOX05F@kernel.org>
	<48719b0f-1a7f-47f9-948a-c981a0a29b41@linaro.org>
Date: Thu, 06 Jun 2024 19:20:52 +0200
Message-ID: <87zfry9edn.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:59, Michael Walle wrote:
>> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>>> On 6/3/24 14:09, Esben Haabendal wrote:
>>>> A dedicated flag for triggering call to
>>>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>>>> and parse, with fallback to legacy flash parameters, without having dual,
>>>> quad or octal parameters set in the legacy flash parameters.
>>>>
>>>> With this, spi-nor flash parts without SFDP that is replaced with a
>>>> different flash NOR flash part that does have SFDP, but shares the same
>>>> manufacturer and device ID is easily handled.
>>>>
>>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>>>  drivers/mtd/spi-nor/core.h | 1 +
>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 3e1f1913536b..1c4d66fc993b 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>>>  
>>>>  	spi_nor_manufacturer_init_params(nor);
>>>>  
>>>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>>>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>>
>>> I don't like that we update deprecated methods. The solution though is
>>> elegant.
>> 
>> 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
>> 
>> 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.
>> 
>
> that's not that bad, but I would avoid doing it if it's not common. You
> also have to update the core a bit, you can't use no_sfdp_flags &
> TRY_SFDP, it's misleading. Does it worth it?
>
> I won't oppose too much, but to me it feels that we're trying to keep
> alive a dead man.

Which dead man are you referring to? The legacy implementation or the
old non-SFDP Macronix part?

/Esben