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

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 4/21/2022 10:56 PM, Tudor.Ambarus@microchip.com wrote:
> 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?
> 
I want you to do it, please.

> Thanks,
> ta

Thank you!

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