Re: [PATCH v3 1/3] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods

[Tudor.Ambarus]

On 4/14/22 12:51, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>> index a5211543d30d..2e40eba3744d 100644
>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>> @@ -401,6 +401,93 @@ static void
>>>> spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>>>       }
>>>>  }
>>>>
>>>> +/**
>>>> + * spansion_set_4byte_addr_mode() - Set 4-byte address mode for
>>>> Spansion
>>>> + * flashes.
>>>> + * @nor:     pointer to 'struct spi_nor'.
>>>> + * @enable:  true to enter the 4-byte address mode, false to exit
>>>> the
>>>> 4-byte
>>>> + *           address mode.
>>>> + *
>>>> + * Return: 0 on success, -errno otherwise.
>>>> + */
>>>> +int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>>>
>>> Mh, so now some callback functions are in the core like the quad
>>> enable
>>> methods and some are in sfdp.c. I think there should be only the
>>> parsing
>>> in sfdp.c and all the callback methods should be in core.c; as they
>>> are
>>> potentially used by the vendor modules.
>>
>> All set_4byte_addr_mode methods are defined in sfdp.c and declared in
>> sfdp.h.
>> I kept the methods defined in sfdp.c because SFDP defines their
>> behavior/how
>> they work. Vendors already have access to all these methods when
>> including
>> core.h (which includes sfdp.h). No need to move them to core, as they
>> are
>> SFDP specific.
> 
> The same is true for the quad enable method and they reside in core.c.
> Again, I think sfdp.c should be about the parsing, not the flash
> handling
> itself. (And sfdp.h should be the equivalent to the spec in terms of
> constants). I mean SFDP will eventually cover everything, so will you
> move
> all methods over to sfdp.c? All these methods can be used with flashes
> without SFDP.
> 
> SFDP just collects all the different methods used by flash manufacturers
> and put them into a table. I don't see how SFDP is a spec where they
> specify
> a paricular method and all the flash manufacturer pick that up. I think
> it
> is the other way around, a flash manufacturer does something
> proprietary and then it eventually ends up in the SFDP spec.

It doesn't matter who was first to define the method. What matters is that
the method is backed-up by a standard. The JEDEC standard "[..] provides a
consistent method of describing the functional and feature capabilities of
serial flash devices [...]".

So if the functional method is described by the standard, let's use the
"standard" methods, and keep them in the standard's code.

> 
>>>> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor
>>>> *nor,
>>>>               break;
>>>>       }
>>>>
>>>> +     switch (bfpt.dwords[BFPT_DWORD(16)] &
>>>> BFPT_DWORD16_4B_ADDR_MODE_MASK)
>>>> {
>>>
>>> I was wondering why this is encoded as single bits and not as an
>>> enumeration. To me it looks like it is intended to support
>>
>> because I parse 2 bits and check if both the enter and the exit methods
>> of
>> the 4b addr mode are specified.
> 
> No, I'm only speaking of either the entry or the exit mask. See
> below.
> 
>>> different methods at the same time. Thus I think there might be
>>> multiple bits set in each entry and exit mask at once. The spec
>>> lists all the entries as x_xxx1, x_xx1x, ..
>>>
>>>> +     case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
>>> .. then this will only match if exactly these two bits are set.
>>>
>>
>> these 2 bits are:
>> drivers/mtd/spi-nor/sfdp.h:#define BFPT_DWORD16_4B_ADDR_MODE_BRWR
>>                  \
>> drivers/mtd/spi-nor/sfdp.h-     (BFPT_DWORD16_EN4B_BRWR |
>> BFPT_DWORD16_EX4B_BRWR)
> 
> I know this are two bits, but IMHO there can be multiple bits
> set in *each* of these masks. Eg. the enter mask could be
> 0b00011 which would indicate that both the first and the second
> enter method is supported.
> (I'd expect that the exit mask will then be 0b00011, too).

I see. I can't contradict you here. How about:

u32 dword;

dword = bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK;
if ((dword & BFPT_DWORD16_4B_ADDR_MODE_BRWR) == BFPT_DWORD16_4B_ADDR_MODE_BRWR)
	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
else if ((dword & BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B) == BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B)
	params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
...
else
	dev_dbg(nor->dev, "BFPT: 4-Byte Address Mode method is not recognized or not implemented\n");

The first method hit will be used, regardless of how many methods are supported.

thanks,
ta