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

[Michael Walle <michael@walle.cc>]

Am 2022-04-21 15:13, schrieb Tudor.Ambarus@microchip.com:

>> If the parsing wouldn't change any runtime parameters we wouldn't
>> have this problem at all. no?
> 
> It doesn't change, _but_ it sets the correct number of address nbytes.

It changes nor->addr_width which might be used for all commands
except the read_sfdp_data(). It changes it before we are entering
the 4 byte mode. Also parse_sfdp changes the opcodes. It seems this
was the reason for the SNOR_F_HAS_4BAIT flag in the first place,
so the core doesn't convert the opcodes again.

>> 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.
> 
> this would mean introducing a nor->params->addr_nbytes, which
> is redundant with SNOR_F_HAS_4BAIT.

So? The SFDP table has both information, I don't see a problem
with that. And I'm not sure they are redunant, I think a flash
can have 4 byte addresses and no 4BAIT table.

And if it would be redundant why do we need that empty
case below..

>> 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 */
>> +       }
>> ..

here.

>> 
>> 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).
> 
> which is good! we prefer using 4b opcodes than entering 4byte address
> mode.

You didn't understand my point. All the assignments of addr_width
are clustered around in the code. Why can't we have them in a common
place. We even have this place already: spi_nor_set_addr_width().
Also you could probably get rid of that "don't change opcodes
if SNOR_F_HAS_4BAIT is set" thingy if the parsing code wouldn't
change the opcodes but returns the parsed ones for the core
to decide what to use. With the benefit of better readability
and lesser bugs.

-michael

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