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

[<Tudor.Ambarus@microchip.com>]

On 4/14/22 12:03, Michael Walle wrote:

Hi!

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

> 
>> @@ -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.

> 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)

When both specified, I set the method.

Cheers,
ta