Received-SPF: pass (google.com: domain of linux-kernel+bounces-205714-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <1d683b33-16db-4d81-92cb-d98e35b87cba@linaro.org>
Date: Fri, 7 Jun 2024 10:22:55 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional
 SFDP
To: Esben Haabendal <esben@geanix.com>, Michael Walle <mwalle@kernel.org>
Cc: 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>
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>
 <D1T10API5U80.1OKB56YTFGMTN@kernel.org> <874ja6aszh.fsf@geanix.com>
Content-Language: en-US
From: Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <874ja6aszh.fsf@geanix.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 6/6/24 18:20, Esben Haabendal wrote:
> "Michael Walle" <mwalle@kernel.org> writes:
> 
>> On Thu Jun 6, 2024 at 4:52 PM CEST, Tudor Ambarus wrote:
>>> 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?
>>
>> IMHO no_sfdp_flags is the correct place (maybe TRY_SFDP is wrong,
>> maybe SFDP_FALLBACK?)
> 
> TRY_SFDP might not be the best choice. But SFDP_FALLBACK sounds to me
> like it is fallback _to_ SFDP, so rather counter-intuitive.
> 
>> because the flash is first treated like in
>> case (2). Then SFDP is tried based on that flag.
> 
> It is first treated like in case (2), and then tried for case (1),
> falling back to the result from case (2) if/when case (1) fails.
> 
>> Is it worth it? I
>> don't know, Esben is doing the development here ;) So up to him.
> 
> I am not sure exactly how it should look like, but I do like the idea
> proposed above, case (4). It is easier to describe and understand than
> the current legacy implementation.
> 
>>> I won't oppose too much, but to me it feels that we're trying to keep
>>> alive a dead man.
>>
>> Maybe, but we'd have a readily solution if we face a similar
>> problem in the future. I'm really not sure, how many flashes there
>> are, but I think these magic bits (which tells the legacy
>> implementation to try SFDP) will mask quite a few of these.
>> I.e. in an ideal world where we could finally drop case (3) and
>> you'd need to split the flashes between case (1) or (2), I think
>> there will be quite some in (4).
> 
> I like this. Judging by the way Macronix is handling this particular
> chip, I strongly assume that there are several other examples of this
> for other Macronix parts. Of-course, as long as the original part using
> the particular flash id supported SFDP, and all later flashes using the
> same id also does, none of this is needed.
> 

okay, let's implement 4/