Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Michael Walle <michael@walle.cc>]

Am 2022-04-21 14:06, schrieb Tudor.Ambarus@microchip.com:
> On 4/21/22 14:29, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>>> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know the content is safe
>>>> 
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> 
>>>> In 4BAIT parse, keep nor->addr_width because it may be used as
>>>> current address mode in SMPT parse later on.
>>>> 
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 40ba45328975..87603a99938f 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct 
>>>> spi_nor *nor,
>>>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>>  {
>>>>         if (nor->addr_width) {
>>>> -               /* already configured from SFDP */
>>>> +               /*
>>>> +                * Already configured from SFDP. Use an address 
>>>> width of 4 in
>>>> +                * case the device has 4byte opcodes.
>>>> +                */
>>>> +               if (nor->addr_width == 3 && nor->flags & 
>>>> SNOR_F_HAS_4BAIT)
>>>> +                       nor->addr_width = 4;
>>>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>> 
>>> Can we have this instead?
>>> 
>>> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
>>> Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>> Date:   Thu Apr 21 18:40:21 2022 +0900
>>> 
>>>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>>> 
>>>     In 4BAIT parse, keep nor->addr_width because it may be used as
>>>     current address mode in SMPT parse later on.
>> 
>> Mh, I don't know it that is any better, there are places where
>> addr_width is set in parse_bfpt. Why can't we fix the real problem
> 
> which I find it correct. The only thing that worth attention is at
> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, which we're already taken care of. We
> don't change the addr mode at parse time and use SNOR_F_HAS_4BAIT to 
> change
> the number of bytes in the aforementioned case.

It's not about the correctness but that we have a clear sync point
and code flow.

Which parameters can you change in the parse_sfdp() which can't you
change, you'll have to document that and even then it is really hard
to follow. To make thing worse, it turns out, you can sometimes
change addr_width and sometimes it doesn't matter?
If the parsing wouldn't change any runtime parameters we wouldn't
have this problem at all. no?

The parse_sfdp() should only change members of struct
spi_nor_flash_parameters, the caller will then decide if they
should be used and more imporantly *when* they should be used.

Then you can do the sane thing in spi_nor_set_addr_width():
setting the addr_width.

Right now it's:

+static int spi_nor_set_addr_width(struct spi_nor *nor)
+{
+	if (nor->flags & SNOR_F_HAS_4BAIT)
+		nor->addr_width = 4;
+
+	if (nor->addr_width) {
+		/* already configured from SFDP */
..

Sometimes it will set addr_width, sometimes it will not be set
and every once in a while 4byte mode is determined by SFDP but
it is not configured (SNOR_F_HAS_4BAIT).

-michael

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