Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Tue, 18 Jul 2023 08:25:39 +0200
From:   Michael Walle <michael@walle.cc>
To:     Tudor Ambarus <tudor.ambarus@linaro.org>
Cc:     Linus Walleij <linus.walleij@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,
        stable@vger.kernel.org
Subject: Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128
In-Reply-To: <d99d87e7-47ba-d6fe-735f-16de2a2ec280@linaro.org>
References: <20230712-spi-nor-winbond-w25q128-v2-1-50c9f1d58d6c@linaro.org>
 <f00fa2ae-6d4a-90cb-3724-2bedb96cb4fb@linaro.org>
 <0525440a652854a2a575256cd07d3559@walle.cc>
 <d99d87e7-47ba-d6fe-735f-16de2a2ec280@linaro.org>
Message-ID: <e642ff27fd0bc0f1f0e293972145f680@walle.cc>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

>>> while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
>>> parameters shall be discovered at run-time, so we prepare to get rid 
>>> of
>>> explicitly setting them sooner or later.
>> 
>> This is an entry matching various flash families from Winbond, see my
>> reply in v1. I'm not sure we should remove these as we could break the
>> older ones, which might or might not have SFDP tables. We don't know.
> 
> I'd take the risk and break the older ones if there are some that don't
> define SFDP indeed, just to handle the conflict properly. We can't
> encourage code based on assumptions otherwise we'll get back to the
> knotted spi-nor code that we tried to untie in the last years.

Wait a minute, now I'm the more conservative one of the both of
us? (:

Jokes aside, basically you are saying that if there are two flashes
with the same id, one supports JEDEC one doesn't, we can brake the
latter one.

>>>> -        NO_SFDP_FLAGS(SECT_4K) },
>> 
>> Thus, I'd also keep this one.
>> 
> 
> Keeping this one does not have the effect that you want as SECT_4K is
> used in spi_nor_no_sfdp_init_params() which is not called when
> PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if
> bugs will be reported, I commit I'll fix them in the same release 
> cycle.

Mhh, I should have been more curious to why Linus needed the PARSE_SFDP
flag in the first place. Taking a closer look, that is because in the
legacy behavior, the SFDP is only read if the chip supports dual or
quad read (whatever the rationale was for that).

Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled.
If the chip doesn't support the SFDP is will just fail probing.

As I'm reading the code right now, for the new behavior
it is either
  * expect the flash supports SFDP, if not, probe fails
  * expect the flash to don't support SFDP, no SFDP is ever read at all

Shouldn't we handle the third case in the new behavior, too:
  * start with the static data we have and try reading SFDP to 
amend/correct it.

If not, will you accept breakage for future flashes, too? Looking at 
winbond.c
for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. 
supports
SFDP nowadays and most of them only have SECT_4K set.

> If both of you agree, I'll amend Linus's v4 patch when applying.

So it would be the first chip without flags at all? Then we could
drop the entry entirely :) And if we do this, then we should also
drop all the other entries for the newer winbond flashes.

If you decide to break older flashes, then I'd prefer to also drop
the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).

-michael