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

[Tudor.Ambarus at microchip.com]

On 4/27/22 07:16, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?

yes. I think sometime this week, I'm a little busy right now.

> Or can we go with this one for now?

No, let's implement Michael's suggestion from the start.

Cheers,
ta