2022-05-10 18:58:59

by Michael Walle

[permalink] [raw]
Subject: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

Up until now, flashes were defined by specifying the JEDEC ID, the
sector size and the number of sectors. This can be read by parsing the
SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
new flashes which have SFDP should use this macro.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 7 +++++--
drivers/mtd/spi-nor/core.h | 9 +++++++++
2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 402b37cdbcea..29329ed0a934 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
* spi_nor_select_uniform_erase() - select optimum uniform erase type
* @map: the erase map of the SPI NOR
* @wanted_size: the erase type size to search for. Contains the value of
- * info->sector_size or of the "small sector" size in case
- * CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
+ * info->sector_size, the "small sector" size in case
+ * CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
+ * there is no information about the sector size. The
+ * latter is the case if the flash parameters are parsed
+ * solely by SFDP.
*
* Once the optimum uniform sector erase command is found, disable all the
* other.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 61886868cd02..fab3038c4f4a 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -563,6 +563,15 @@ struct flash_info {
.n_regions = (_n_regions), \
},

+#define SNOR_ID3(_jedec_id) \
+ .id = { \
+ ((_jedec_id) >> 16) & 0xff, \
+ ((_jedec_id) >> 8) & 0xff, \
+ (_jedec_id) & 0xff, \
+ }, \
+ .id_len = 3, \
+ .parse_sfdp = true, \
+
#define PARSE_SFDP \
.parse_sfdp = true, \

--
2.30.2



2022-06-05 16:38:42

by Tom Fitzhenry

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

In "Add support for the siliconkaiser sk25lp128 chip."[0], I have tested
that this patch works on the sk25lp128.

Thanks!

Tested-by: Tom Fitzhenry <[email protected]>

0. https://lore.kernel.org/all/[email protected]/

2022-07-12 07:45:57

by Pratyush Yadav

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 10/05/22 04:02PM, Michael Walle wrote:
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.
>
> Signed-off-by: Michael Walle <[email protected]>

Reviewed-by: Pratyush Yadav <[email protected]>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

2022-07-15 12:25:41

by Biju Das

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 10/05/22 04:02PM, Michael Walle wrote:
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.
>
> Signed-off-by: Michael Walle <[email protected]>

Tested the macro with Renesas AT25QL128A serial nor flash. So,

Tested-by: Biju Das <[email protected]>

2022-07-19 06:51:43

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 5/10/22 17:02, Michael Walle wrote:

Hi!

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.

I like the idea, but you need to refine it a bit.
Your assumptions are true only for flashes that are compliant with SFDP revB or
later because params->page_size is initialized by querying BFPT DWORD 11. I think
it would be good to specify this in the comment section. Also, I think you introduce
a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not
selected. wanted_size will be zero, will you select an invalid erase type?
Would you please resubmit?

Thanks,
ta

>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 7 +++++--
> drivers/mtd/spi-nor/core.h | 9 +++++++++
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 402b37cdbcea..29329ed0a934 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
> * spi_nor_select_uniform_erase() - select optimum uniform erase type
> * @map: the erase map of the SPI NOR
> * @wanted_size: the erase type size to search for. Contains the value of
> - * info->sector_size or of the "small sector" size in case
> - * CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
> + * info->sector_size, the "small sector" size in case
> + * CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
> + * there is no information about the sector size. The
> + * latter is the case if the flash parameters are parsed
> + * solely by SFDP.
> *
> * Once the optimum uniform sector erase command is found, disable all the
> * other.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 61886868cd02..fab3038c4f4a 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -563,6 +563,15 @@ struct flash_info {
> .n_regions = (_n_regions), \
> },
>
> +#define SNOR_ID3(_jedec_id) \
> + .id = { \
> + ((_jedec_id) >> 16) & 0xff, \
> + ((_jedec_id) >> 8) & 0xff, \
> + (_jedec_id) & 0xff, \
> + }, \
> + .id_len = 3, \
> + .parse_sfdp = true, \
> +
> #define PARSE_SFDP \
> .parse_sfdp = true, \
>
> --
> 2.30.2
>

2022-07-19 07:20:45

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

Am 2022-07-19 07:57, schrieb [email protected]:
> On 5/10/22 17:02, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Up until now, flashes were defined by specifying the JEDEC ID, the
>> sector size and the number of sectors. This can be read by parsing the
>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>> All
>> new flashes which have SFDP should use this macro.
>
> I like the idea, but you need to refine it a bit.
> Your assumptions are true only for flashes that are compliant with SFDP
> revB or
> later because params->page_size is initialized by querying BFPT DWORD
> 11. I think it would be good to specify this in the comment section.

Sure.

> Also, I think you introduce
> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
> is not
> selected. wanted_size will be zero, will you select an invalid erase
> type?

You mean to squeeze [1] into this one? If so, sure.

-michael

[1]
https://lore.kernel.org/linux-mtd/[email protected]/

2022-07-19 07:46:45

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 7/19/22 10:07, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2022-07-19 07:57, schrieb [email protected]:
>> On 5/10/22 17:02, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Up until now, flashes were defined by specifying the JEDEC ID, the
>>> sector size and the number of sectors. This can be read by parsing the
>>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>>> All
>>> new flashes which have SFDP should use this macro.
>>
>> I like the idea, but you need to refine it a bit.
>> Your assumptions are true only for flashes that are compliant with SFDP
>> revB or
>> later because params->page_size is initialized by querying BFPT DWORD
>> 11. I think it would be good to specify this in the comment section.
>
> Sure.
>
>> Also, I think you introduce
>> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>> is not
>> selected. wanted_size will be zero, will you select an invalid erase
>> type?
>
> You mean to squeeze [1] into this one? If so, sure.
>
> -michael
>
> [1]
> https://lore.kernel.org/linux-mtd/[email protected]/

No, these are orthogonal. If you keep wanted_size to zero, then
spi_nor_select_uniform_erase() will return NULL, doesn't it?


Maybe to adapt the code to something like
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 21cefe2864ba..dd6cd852d1ef 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor *nor)
struct spi_nor_erase_map *map = &nor->params->erase_map;
const struct spi_nor_erase_type *erase = NULL;
struct mtd_info *mtd = &nor->mtd;
- u32 wanted_size = nor->info->sector_size;
+ u32 wanted_size = nor->params->sector_size;

and fill nor->params->sector_size even when no SFDP

Also, params->size = (u64)info->sector_size * info->n_sectors; from
spi_nor_init_default_params() becomes superfluous. I would check
the fields that I don't initialize in flash_info with SNOR_ID3
and check how I can mitigate their absence throughout the code.

2022-07-19 08:01:30

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

Am 2022-07-19 09:33, schrieb [email protected]:
> On 7/19/22 10:07, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Am 2022-07-19 07:57, schrieb [email protected]:
>>> On 5/10/22 17:02, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>> know
>>>> the content is safe
>>>>
>>>> Up until now, flashes were defined by specifying the JEDEC ID, the
>>>> sector size and the number of sectors. This can be read by parsing
>>>> the
>>>> SFDP, we don't have to specify it. Thus provide a new macro
>>>> SNOR_ID3()
>>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>>>> All
>>>> new flashes which have SFDP should use this macro.
>>>
>>> I like the idea, but you need to refine it a bit.
>>> Your assumptions are true only for flashes that are compliant with
>>> SFDP
>>> revB or
>>> later because params->page_size is initialized by querying BFPT DWORD
>>> 11. I think it would be good to specify this in the comment section.
>>
>> Sure.
>>
>>> Also, I think you introduce
>>> a bug in spi_nor_select_erase() when
>>> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>> is not
>>> selected. wanted_size will be zero, will you select an invalid erase
>>> type?
>>
>> You mean to squeeze [1] into this one? If so, sure.
>>
>> -michael
>>
>> [1]
>> https://lore.kernel.org/linux-mtd/[email protected]/
>
> No, these are orthogonal. If you keep wanted_size to zero, then
> spi_nor_select_uniform_erase() will return NULL, doesn't it?

No, have a look at the second condition

if (!erase && tested_erase->size)
erase = ..

So it will return the first non-empty slot. Thus it will
only return NULL if all the slots are empty (given the
fix is included).

Actually, I'd have expected that to mask out an erase
type, you clear the corresponding bit in uniform_erase_type,
in which case the for loop in spi_nor_select_uniform_erase()
would have just worked. But apparently there are two differnt
mechanism here to mark an entry as unused, either the size
is zero or the bit is not set. But that is a topic for another
patch.

-michael

> Maybe to adapt the code to something like
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 21cefe2864ba..dd6cd852d1ef 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor
> *nor)
> struct spi_nor_erase_map *map = &nor->params->erase_map;
> const struct spi_nor_erase_type *erase = NULL;
> struct mtd_info *mtd = &nor->mtd;
> - u32 wanted_size = nor->info->sector_size;
> + u32 wanted_size = nor->params->sector_size;
>
> and fill nor->params->sector_size even when no SFDP
>
> Also, params->size = (u64)info->sector_size * info->n_sectors; from
> spi_nor_init_default_params() becomes superfluous. I would check
> the fields that I don't initialize in flash_info with SNOR_ID3
> and check how I can mitigate their absence throughout the code.

2022-07-19 08:39:07

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 7/19/22 10:57, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2022-07-19 09:33, schrieb [email protected]:
>> On 7/19/22 10:07, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-07-19 07:57, schrieb [email protected]:
>>>> On 5/10/22 17:02, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Up until now, flashes were defined by specifying the JEDEC ID, the
>>>>> sector size and the number of sectors. This can be read by parsing
>>>>> the
>>>>> SFDP, we don't have to specify it. Thus provide a new macro
>>>>> SNOR_ID3()
>>>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>>>>> All
>>>>> new flashes which have SFDP should use this macro.
>>>>
>>>> I like the idea, but you need to refine it a bit.
>>>> Your assumptions are true only for flashes that are compliant with
>>>> SFDP
>>>> revB or
>>>> later because params->page_size is initialized by querying BFPT DWORD
>>>> 11. I think it would be good to specify this in the comment section.
>>>
>>> Sure.
>>>
>>>> Also, I think you introduce
>>>> a bug in spi_nor_select_erase() when
>>>> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>>> is not
>>>> selected. wanted_size will be zero, will you select an invalid erase
>>>> type?
>>>
>>> You mean to squeeze [1] into this one? If so, sure.
>>>
>>> -michael
>>>
>>> [1]
>>> https://lore.kernel.org/linux-mtd/[email protected]/
>>
>> No, these are orthogonal. If you keep wanted_size to zero, then
>> spi_nor_select_uniform_erase() will return NULL, doesn't it?
>
> No, have a look at the second condition
>
> if (!erase && tested_erase->size)
>   erase = ..
>
> So it will return the first non-empty slot. Thus it will
> only return NULL if all the slots are empty (given the
> fix is included).
>
> Actually, I'd have expected that to mask out an erase
> type, you clear the corresponding bit in uniform_erase_type,
> in which case the for loop in spi_nor_select_uniform_erase()
> would have just worked. But apparently there are two differnt
> mechanism here to mark an entry as unused, either the size
> is zero or the bit is not set. But that is a topic for another
> patch.


Right, I remember I leaned towards using just the erase mask to mask
out an erase, but I have to reassess this. Here's a patch that is
related and I left behind:
https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/

>

something else that looks wrong:
drivers/mtd/spi-nor/swp.c: return nor->info->sector_size <<
drivers/mtd/spi-nor/swp.c: return nor->info->sector_size;

How do we progress on this? I like the SNOR_ID3 idea, but I think it
should have a different form. Do you want to spend more time on this
or do you think I should invest more time on this?


> -michael
>
>> Maybe to adapt the code to something like
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 21cefe2864ba..dd6cd852d1ef 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor
>> *nor)
>>         struct spi_nor_erase_map *map = &nor->params->erase_map;
>>         const struct spi_nor_erase_type *erase = NULL;
>>         struct mtd_info *mtd = &nor->mtd;
>> -       u32 wanted_size = nor->info->sector_size;
>> +       u32 wanted_size = nor->params->sector_size;
>>
>> and fill nor->params->sector_size even when no SFDP
>>
>> Also, params->size = (u64)info->sector_size * info->n_sectors; from
>> spi_nor_init_default_params() becomes superfluous. I would check
>> the fields that I don't initialize in flash_info with SNOR_ID3
>> and check how I can mitigate their absence throughout the code.

2022-07-28 03:27:36

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 7/19/22 08:57, [email protected] wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 5/10/22 17:02, Michael Walle wrote:
>
> Hi!
>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Up until now, flashes were defined by specifying the JEDEC ID, the
>> sector size and the number of sectors. This can be read by parsing the
>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
>> new flashes which have SFDP should use this macro.
>
> I like the idea, but you need to refine it a bit.
> Your assumptions are true only for flashes that are compliant with SFDP revB or
> later because params->page_size is initialized by querying BFPT DWORD 11. I think
> it would be good to specify this in the comment section. Also, I think you introduce
> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not
> selected. wanted_size will be zero, will you select an invalid erase type?
> Would you please resubmit?
>
> Thanks,
> ta
>
>>
>> Signed-off-by: Michael Walle <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 7 +++++--
>> drivers/mtd/spi-nor/core.h | 9 +++++++++
>> 2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 402b37cdbcea..29329ed0a934 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
>> * spi_nor_select_uniform_erase() - select optimum uniform erase type
>> * @map: the erase map of the SPI NOR
>> * @wanted_size: the erase type size to search for. Contains the value of
>> - * info->sector_size or of the "small sector" size in case
>> - * CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
>> + * info->sector_size, the "small sector" size in case
>> + * CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
>> + * there is no information about the sector size. The
>> + * latter is the case if the flash parameters are parsed
>> + * solely by SFDP.
>> *
>> * Once the optimum uniform sector erase command is found, disable all the
>> * other.
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 61886868cd02..fab3038c4f4a 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -563,6 +563,15 @@ struct flash_info {
>> .n_regions = (_n_regions), \
>> },
>>
>> +#define SNOR_ID3(_jedec_id)

How about SFDP_ID3 and SFDP_ID6 instead?

\
>> + .id = { \
>> + ((_jedec_id) >> 16) & 0xff, \
>> + ((_jedec_id) >> 8) & 0xff, \
>> + (_jedec_id) & 0xff, \
>> + }, \
>> + .id_len = 3, \
>> + .parse_sfdp = true, \
>> +
>> #define PARSE_SFDP \
>> .parse_sfdp = true, \
>>
>> --
>> 2.30.2
>>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


--
Cheers,
ta

2022-07-28 13:20:21

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

>>> +#define SNOR_ID3(_jedec_id)
>
> How about SFDP_ID3 and SFDP_ID6 instead?

Yes, probably a better name. I was also thinking about splitting
the id in vendor, device and additional bytes. But I haven't
thought of the actual implementation that much. Such as:

#define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
#define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>,
<variable aux bytes>)

Couldn't make up a better name than that _FULL for now. Happy to hear
suggestions :)

-michael

2022-07-28 13:33:44

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 7/28/22 16:12, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> +#define SNOR_ID3(_jedec_id)
>>
>> How about SFDP_ID3 and SFDP_ID6 instead?
>
> Yes, probably a better name. I was also thinking about splitting
> the id in vendor, device and additional bytes. But I haven't
> thought of the actual implementation that much. Such as:
>
> #define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
> #define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>,
> <variable aux bytes>)
>
> Couldn't make up a better name than that _FULL for now. Happy to hear
> suggestions :)
>

You mean splitting the ID in manufacturer ID, flash ID and extended flash ID?
I'd like to understand the benefits of splitting this, can you give me an
example? In the past I though about introducing some flash info macros for
families of flashes of the same vendor, it will reduce the number of lines
on flash definition, but not really related.

--
Cheers,
ta

2022-07-28 14:06:42

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

Am 2022-07-28 15:31, schrieb [email protected]:
> On 7/28/22 16:12, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>>>>> +#define SNOR_ID3(_jedec_id)
>>>
>>> How about SFDP_ID3 and SFDP_ID6 instead?
>>
>> Yes, probably a better name. I was also thinking about splitting
>> the id in vendor, device and additional bytes. But I haven't
>> thought of the actual implementation that much. Such as:
>>
>> #define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
>> #define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>,
>> <variable aux bytes>)
>>
>> Couldn't make up a better name than that _FULL for now. Happy to hear
>> suggestions :)
>>
>
> You mean splitting the ID in manufacturer ID, flash ID and extended
> flash ID?
> I'd like to understand the benefits of splitting this, can you give me
> an
> example? In the past I though about introducing some flash info macros
> for
> families of flashes of the same vendor, it will reduce the number of
> lines
> on flash definition, but not really related.

First, why would you combine the vendor and part id into one three byte
field? Isn't it natural to have these as two fields? We know the did is
8 bit and the vid is 16 bit. And there might be N continuation codes. So
putting that all in one value is error prone. See also below for the
is25cd512.
Second, the extended (so maybe SFDP_ID_EXT?) bytes is variable. I've
seen flashes with one additional byte.

If you want to have per vendor convenience macros you could do

#define WINBOND_SFDP_ID(did, ...) SFDP_ID(0xNN, did, __VA_ARGS__)

I don't see much benefit here. OTHO "#define SFDP_VID_WINBOND 0xNN"
might make sense. But then we'd need to come up with support for
continuation codes. So mhhh.

As for the examples:
SFDP_ID(SFDP_VID_WINBOND, 0x6019)
SFDP_ID(SFDP_VID_MICRON, 0xba19, 0x10, 0x44, 0x00)
or maybe
SFDP_ID(SFDP_VID_MICRON, 0xba19, 0x10, 0x44)

Currently we have the is25cd512 which (correctly) uses
continuation codes. So, we'd need have something like
#define SFDP_VID_ISSI_LEGACY 0x9d
#define SFDP_VID_ISSI 1, 0x9d

And preferrably we'd have something like:

SFDP_ID(SFDP_VID_ISSI_LEGACY, 0x4013)
SFDP_ID(SFDD_VID_ISSI, 0x20xx)

(note that "xx" is unknown here.. we are lacking that because
the entry is just using INFO() and I'm too lazy to look up the
datasheet now.)

Now I know that above probably won't compile, but maybe someone
could come up with macros which actually work :)

The following might work, but feels awful like a hack:
#define SFDP_VID_ISSI_LEGACY 0, 0x9d
#define SFDP_VID_ISSI 1, 0x9d

-michael