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

[Takahiro Kuwano]

Hi Tudor,

On 4/21/2022 11:26 PM, Takahiro Kuwano wrote:
> 
> 
> On 4/21/2022 10:56 PM, Tudor.Ambarus at 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 at 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.
> 
Do you plan to help on another implementation shortly?
Or can we go with this one for now?

Best Regards,
Takahiro