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

[Tudor.Ambarus at microchip.com]

On 4/21/22 16:01, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-04-21 14:06, schrieb Tudor.Ambarus at microchip.com:
>> On 4/21/22 14:29, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>>> On 4/21/22 12:40, tkuw584924 at gmail.com wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know the content is safe
>>>>>
>>>>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>>
>>>>> In 4BAIT parse, keep nor->addr_width because it may be used as
>>>>> current address mode in SMPT parse later on.
>>>>>
>>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>>>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>> index 40ba45328975..87603a99938f 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct
>>>>> spi_nor *nor,
>>>>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>>>  {
>>>>>         if (nor->addr_width) {
>>>>> -               /* already configured from SFDP */
>>>>> +               /*
>>>>> +                * Already configured from SFDP. Use an address
>>>>> width of 4 in
>>>>> +                * case the device has 4byte opcodes.
>>>>> +                */
>>>>> +               if (nor->addr_width == 3 && nor->flags &
>>>>> SNOR_F_HAS_4BAIT)
>>>>> +                       nor->addr_width = 4;
>>>>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>>>
>>>> Can we have this instead?
>>>>
>>>> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
>>>> Author: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>> Date:   Thu Apr 21 18:40:21 2022 +0900
>>>>
>>>>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>>>>
>>>>     In 4BAIT parse, keep nor->addr_width because it may be used as
>>>>     current address mode in SMPT parse later on.
>>>
>>> Mh, I don't know it that is any better, there are places where
>>> addr_width is set in parse_bfpt. Why can't we fix the real problem
>>
>> which I find it correct. The only thing that worth attention is at
>> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, which we're already taken care of. We
>> don't change the addr mode at parse time and use SNOR_F_HAS_4BAIT to
>> change
>> the number of bytes in the aforementioned case.
> 
> It's not about the correctness but that we have a clear sync point
> and code flow.
> 
> Which parameters can you change in the parse_sfdp() which can't you
> change, you'll have to document that and even then it is really hard
> to follow. To make thing worse, it turns out, you can sometimes
> change addr_width and sometimes it doesn't matter?

no, it matters all the time. nor->addr_width (which I'll rename to
nor->addr_nbytes btw) is a particular case and we shouldn't
generalize it, I think.

nor->addr_width is initialized only in sfdp in:
        /* Number of address bytes. */                                          
        switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {  
        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:                                  
        case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:                                  
                nor->addr_width = 3;                                            
                break;                                                          
                                                                                
        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:                                  
                nor->addr_width = 4;                                            
                break;                                                          
                                                                                
        default:                                                                
                break;                                                          
        }

BFPT_DWORD1_ADDRESS_BYTES_3_ONLY and BFPT_DWORD1_ADDRESS_BYTES_4_ONLY
are clear and we can always set nor->addr_width with them.

For BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 we have the following note in BFPT:
"01b: 3- or 4-Byte addressing (e.g., defaults to 3-Byte mode; enters 4-Byte
mode on command)"

Entering in 4-byte mode and deciding the number of addr nbytes is done after
the SFDP parsing, which we currently done, so we're safe.

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

> 
> -michael