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

[<Tudor.Ambarus@microchip.com>]

On 4/21/22 16:42, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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

no, it _sets_ the nor->addr_width. nor->addr_width is initialized only
in BFPT, where BFPT is present. No change done.

> 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.

right, but this is not something that we are addressing right now.
> 
> 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

set in bfpt and then at updated at flash init. It's not spread
throughout the code.

> 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.
> 

I think I understood you from the beginning. Both approaches are fine
IMO, but seems that you care about yours, so let's implement your
suggestion. Takahiro, will you handle it, or do you want me to do it?

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