[PATCH v3 0/3] Read MAC address through NVMEM for sama7g5ek

[PATCH v3 0/3] Read MAC address through NVMEM for sama7g5ek

[Andrei Simion]

This series proposes to add EEPROM support and reading MAC addresses
through NVMEM (via Devicetree) for sama7g5ek:
- Add in DT bindings document the EEPROM compatibles :
"microchip,24aa025e48" and "microchip,24aa025e64"
- Update to the driver to support "microchip,24aa025e48" and
"microchip,24aa025e64" and adjusting offset for those 24AA025E{48, 64}.
- Added the nodes in devicetree for eeproms where are stored EUI-48 MAC,
and update gmac nodes to read the MAC via devicetree through NVMEM.

------------------------------------------------------------------
v2 -> v3:
* dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64
  - commit subject changed to reference Microchip 24AA025E48/24AA025E64
  - drop the pattern: mac02e4$ and mac02e6$ and a-z from regex
  - add these two devices down at the bottom
  - added Reviewed-by

* eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  - add specific compatible names according with
https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
  - add extended macros to initialize the structure with explicit value for adjoff
  - drop co-developed-by to maintain the commit history
  (chronological order of modifications)

* ARM: dts: at91: at91-sama7g5ek: add EEPROMs
  - change from atmel,24mac02e4 to microchip,24aa025e48 to align with the datasheet
  - drop co-developed-by to maintain the chronological order of the changes

v1 -> v2:
* dt-bindings: eeprom: at24: Add at24,mac02e4 and at24,mac02e6
  - change pattern into "^atmel,(24(c|cs|mac)[a-z0-9]+|spd)$" to keep simpler

* eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  - no change

* ARM: dts: at91: at91-sama7g5ek: add EEPROMs
  - remove unnecessary #address-cells #size-cells

------------------------------------------------------------------
Andrei Simion (1):
  dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64

Claudiu Beznea (2):
  eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  ARM: dts: at91: at91-sama7g5ek: add EEPROMs

 .../devicetree/bindings/eeprom/at24.yaml      |  4 ++
 .../arm/boot/dts/microchip/at91-sama7g5ek.dts | 40 +++++++++++++++++++
 drivers/misc/eeprom/at24.c                    | 28 ++++++++++---
 3 files changed, 67 insertions(+), 5 deletions(-)


base-commit: 642a16ca7994a50d7de85715996a8ce171a5bdfb
-- 
2.34.1

[PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Andrei Simion]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

The EEPROMs could be used only for MAC storage. In this case the
EEPROM areas where MACs resides could be modeled as NVMEM cells
(directly via DT bindings) such that the already available networking
infrastructure to read properly the MAC addresses (via
of_get_mac_address()). The previously available compatibles needs the
offset adjustment probably for compatibility w/ old DT bindings.
Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
for EUI-48 address and "24aa025e64" stands for EUI-64 address.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
[andrei.simion@microchip.com: Add extended macros to initialize the structure
with explicit value to adjusting offset. Add extra description for the commit
message.]
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
---
v2 -> v3:
- add specific compatible names according with
https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
- add extended macros to initialize the structure with explicit value for adjoff
- drop co-developed-by to maintain the commit history
 (chronological order of modifications)

v1 -> v2:
- no change
---
 drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 4bd4f32bcdab..e2ac08f656cf 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -121,20 +121,29 @@ struct at24_chip_data {
 	u32 byte_len;
 	u8 flags;
 	u8 bank_addr_shift;
+	u8 adjoff;
 	void (*read_post)(unsigned int off, char *buf, size_t count);
 };
 
-#define AT24_CHIP_DATA(_name, _len, _flags)				\
+#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao)			\
 	static const struct at24_chip_data _name = {			\
 		.byte_len = _len, .flags = _flags,			\
+		.adjoff = _ao						\
 	}
 
-#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)		\
+#define AT24_CHIP_DATA(_name, _len, _flags)				\
+	AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
+
+#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post)	\
 	static const struct at24_chip_data _name = {			\
 		.byte_len = _len, .flags = _flags,			\
+		.adjoff = _ao,						\
 		.read_post = _read_post,				\
 	}
 
+#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)		\
+	AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
+
 #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift)	\
 	static const struct at24_chip_data _name = {			\
 		.byte_len = _len, .flags = _flags,			\
@@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
 AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
 AT24_CHIP_DATA(at24_data_24cs02, 16,
 	AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
+AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
+	AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
+AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
+	AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
+AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
 	AT24_FLAG_MAC | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
+AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
 	AT24_FLAG_MAC | AT24_FLAG_READONLY);
 /* spd is a 24c02 in memory DIMMs */
 AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
@@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
 	{ "24cs02",	(kernel_ulong_t)&at24_data_24cs02 },
 	{ "24mac402",	(kernel_ulong_t)&at24_data_24mac402 },
 	{ "24mac602",	(kernel_ulong_t)&at24_data_24mac602 },
+	{ "24aa025e48",	(kernel_ulong_t)&at24_data_24aa025e48 },
+	{ "24aa025e64",	(kernel_ulong_t)&at24_data_24aa025e64 },
 	{ "spd",	(kernel_ulong_t)&at24_data_spd },
 	{ "24c02-vaio",	(kernel_ulong_t)&at24_data_24c02_vaio },
 	{ "24c04",	(kernel_ulong_t)&at24_data_24c04 },
@@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
 	{ .compatible = "atmel,24c1024",	.data = &at24_data_24c1024 },
 	{ .compatible = "atmel,24c1025",	.data = &at24_data_24c1025 },
 	{ .compatible = "atmel,24c2048",	.data = &at24_data_24c2048 },
+	{ .compatible = "microchip,24aa025e48",	.data = &at24_data_24aa025e48 },
+	{ .compatible = "microchip,24aa025e64",	.data = &at24_data_24aa025e64 },
 	{ /* END OF LIST */ },
 };
 MODULE_DEVICE_TABLE(of, at24_of_match);
@@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
 	at24->read_post = cdata->read_post;
 	at24->bank_addr_shift = cdata->bank_addr_shift;
 	at24->num_addresses = num_addresses;
-	at24->offset_adj = at24_get_offset_adj(flags, byte_len);
+	at24->offset_adj = cdata->adjoff ?
+				at24_get_offset_adj(flags, byte_len) : 0;
 	at24->client_regmaps[0] = regmap;
 
 	at24->vcc_reg = devm_regulator_get(dev, "vcc");
-- 
2.34.1

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Bartosz Golaszewski]

On Fri, Jun 28, 2024 at 10:02 AM Andrei Simion
<andrei.simion@microchip.com> wrote:
>
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>
> The EEPROMs could be used only for MAC storage. In this case the
> EEPROM areas where MACs resides could be modeled as NVMEM cells
> (directly via DT bindings) such that the already available networking
> infrastructure to read properly the MAC addresses (via
> of_get_mac_address()). The previously available compatibles needs the
> offset adjustment probably for compatibility w/ old DT bindings.
> Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
> usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
> for EUI-48 address and "24aa025e64" stands for EUI-64 address.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> [andrei.simion@microchip.com: Add extended macros to initialize the structure
> with explicit value to adjusting offset. Add extra description for the commit
> message.]
> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> ---
> v2 -> v3:
> - add specific compatible names according with
> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
> - add extended macros to initialize the structure with explicit value for adjoff
> - drop co-developed-by to maintain the commit history
>  (chronological order of modifications)
>
> v1 -> v2:
> - no change
> ---
>  drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 4bd4f32bcdab..e2ac08f656cf 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -121,20 +121,29 @@ struct at24_chip_data {
>         u32 byte_len;
>         u8 flags;
>         u8 bank_addr_shift;
> +       u8 adjoff;
>         void (*read_post)(unsigned int off, char *buf, size_t count);
>  };
>
> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> +#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao)                    \

Please, don't try to save space on a few letters, call it
AT24_CHIP_DATA_ADJOFF() for better readability.

>         static const struct at24_chip_data _name = {                    \
>                 .byte_len = _len, .flags = _flags,                      \
> +               .adjoff = _ao                                           \
>         }
>
> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> +#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> +       AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
> +
> +#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post)     \
>         static const struct at24_chip_data _name = {                    \
>                 .byte_len = _len, .flags = _flags,                      \
> +               .adjoff = _ao,                                          \
>                 .read_post = _read_post,                                \
>         }
>
> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> +       AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
> +
>  #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift)       \
>         static const struct at24_chip_data _name = {                    \
>                 .byte_len = _len, .flags = _flags,                      \
> @@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
>  AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
>  AT24_CHIP_DATA(at24_data_24cs02, 16,
>         AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> +AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);

And this will not break existing users? I guess you refer to these
changes in your commit message but it's not very clear what you're
doing and why.

> +AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> +AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> +AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>  /* spd is a 24c02 in memory DIMMs */
>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> @@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
> +       { "24aa025e48", (kernel_ulong_t)&at24_data_24aa025e48 },
> +       { "24aa025e64", (kernel_ulong_t)&at24_data_24aa025e64 },
>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
> @@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
>         { .compatible = "atmel,24c1024",        .data = &at24_data_24c1024 },
>         { .compatible = "atmel,24c1025",        .data = &at24_data_24c1025 },
>         { .compatible = "atmel,24c2048",        .data = &at24_data_24c2048 },
> +       { .compatible = "microchip,24aa025e48", .data = &at24_data_24aa025e48 },
> +       { .compatible = "microchip,24aa025e64", .data = &at24_data_24aa025e64 },
>         { /* END OF LIST */ },
>  };
>  MODULE_DEVICE_TABLE(of, at24_of_match);
> @@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
>         at24->read_post = cdata->read_post;
>         at24->bank_addr_shift = cdata->bank_addr_shift;
>         at24->num_addresses = num_addresses;
> -       at24->offset_adj = at24_get_offset_adj(flags, byte_len);
> +       at24->offset_adj = cdata->adjoff ?
> +                               at24_get_offset_adj(flags, byte_len) : 0;
>         at24->client_regmaps[0] = regmap;
>
>         at24->vcc_reg = devm_regulator_get(dev, "vcc");
> --
> 2.34.1
>

Bart

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Andrei.Simion]

On 28.06.2024 11:30, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jun 28, 2024 at 10:02 AM Andrei Simion
> <andrei.simion@microchip.com> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>
>> The EEPROMs could be used only for MAC storage. In this case the
>> EEPROM areas where MACs resides could be modeled as NVMEM cells
>> (directly via DT bindings) such that the already available networking
>> infrastructure to read properly the MAC addresses (via
>> of_get_mac_address()). The previously available compatibles needs the
>> offset adjustment probably for compatibility w/ old DT bindings.
>> Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
>> usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
>> for EUI-48 address and "24aa025e64" stands for EUI-64 address.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> [andrei.simion@microchip.com: Add extended macros to initialize the structure
>> with explicit value to adjusting offset. Add extra description for the commit
>> message.]
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> ---
>> v2 -> v3:
>> - add specific compatible names according with
>> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
>> - add extended macros to initialize the structure with explicit value for adjoff
>> - drop co-developed-by to maintain the commit history
>>  (chronological order of modifications)
>>
>> v1 -> v2:
>> - no change
>> ---
>>  drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index 4bd4f32bcdab..e2ac08f656cf 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -121,20 +121,29 @@ struct at24_chip_data {
>>         u32 byte_len;
>>         u8 flags;
>>         u8 bank_addr_shift;
>> +       u8 adjoff;
>>         void (*read_post)(unsigned int off, char *buf, size_t count);
>>  };
>>
>> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>> +#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao)                    \
> 
> Please, don't try to save space on a few letters, call it
> AT24_CHIP_DATA_ADJOFF() for better readability.
>

I will change in next the version.

>>         static const struct at24_chip_data _name = {                    \
>>                 .byte_len = _len, .flags = _flags,                      \
>> +               .adjoff = _ao                                           \
>>         }
>>
>> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>> +#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>> +       AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
>> +
>> +#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post)     \
>>         static const struct at24_chip_data _name = {                    \
>>                 .byte_len = _len, .flags = _flags,                      \
>> +               .adjoff = _ao,                                          \
>>                 .read_post = _read_post,                                \
>>         }
>>
>> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>> +       AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
>> +
>>  #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift)       \
>>         static const struct at24_chip_data _name = {                    \
>>                 .byte_len = _len, .flags = _flags,                      \
>> @@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
>>  AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
>>  AT24_CHIP_DATA(at24_data_24cs02, 16,
>>         AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
>> +AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> 
> And this will not break existing users? I guess you refer to these
> changes in your commit message but it's not very clear what you're
> doing and why.
> 

For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
So, indeed, it is an entanglement in logic.
To keep the implementation as it is: 
adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0. 

I think that is enough not to break the existing users. What are your thoughts?

Best Regards,
Andrei Simion

>> +AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>> +AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
>>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
>> +AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
>>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>  /* spd is a 24c02 in memory DIMMs */
>>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
>> @@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
>>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
>>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
>> +       { "24aa025e48", (kernel_ulong_t)&at24_data_24aa025e48 },
>> +       { "24aa025e64", (kernel_ulong_t)&at24_data_24aa025e64 },
>>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
>> @@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
>>         { .compatible = "atmel,24c1024",        .data = &at24_data_24c1024 },
>>         { .compatible = "atmel,24c1025",        .data = &at24_data_24c1025 },
>>         { .compatible = "atmel,24c2048",        .data = &at24_data_24c2048 },
>> +       { .compatible = "microchip,24aa025e48", .data = &at24_data_24aa025e48 },
>> +       { .compatible = "microchip,24aa025e64", .data = &at24_data_24aa025e64 },
>>         { /* END OF LIST */ },
>>  };
>>  MODULE_DEVICE_TABLE(of, at24_of_match);
>> @@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
>>         at24->read_post = cdata->read_post;
>>         at24->bank_addr_shift = cdata->bank_addr_shift;
>>         at24->num_addresses = num_addresses;
>> -       at24->offset_adj = at24_get_offset_adj(flags, byte_len);
>> +       at24->offset_adj = cdata->adjoff ?
>> +                               at24_get_offset_adj(flags, byte_len) : 0;
>>         at24->client_regmaps[0] = regmap;
>>
>>         at24->vcc_reg = devm_regulator_get(dev, "vcc");
>> --
>> 2.34.1
>>
> 
> Bart

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Bartosz Golaszewski]

On Fri, Jun 28, 2024 at 4:17 PM <Andrei.Simion@microchip.com> wrote:
>
> On 28.06.2024 11:30, Bartosz Golaszewski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Fri, Jun 28, 2024 at 10:02 AM Andrei Simion
> > <andrei.simion@microchip.com> wrote:
> >>
> >> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> >>
> >> The EEPROMs could be used only for MAC storage. In this case the
> >> EEPROM areas where MACs resides could be modeled as NVMEM cells
> >> (directly via DT bindings) such that the already available networking
> >> infrastructure to read properly the MAC addresses (via
> >> of_get_mac_address()). The previously available compatibles needs the
> >> offset adjustment probably for compatibility w/ old DT bindings.
> >> Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
> >> usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
> >> for EUI-48 address and "24aa025e64" stands for EUI-64 address.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >> [andrei.simion@microchip.com: Add extended macros to initialize the structure
> >> with explicit value to adjusting offset. Add extra description for the commit
> >> message.]
> >> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> >> ---
> >> v2 -> v3:
> >> - add specific compatible names according with
> >> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
> >> - add extended macros to initialize the structure with explicit value for adjoff
> >> - drop co-developed-by to maintain the commit history
> >>  (chronological order of modifications)
> >>
> >> v1 -> v2:
> >> - no change
> >> ---
> >>  drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
> >>  1 file changed, 23 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >> index 4bd4f32bcdab..e2ac08f656cf 100644
> >> --- a/drivers/misc/eeprom/at24.c
> >> +++ b/drivers/misc/eeprom/at24.c
> >> @@ -121,20 +121,29 @@ struct at24_chip_data {
> >>         u32 byte_len;
> >>         u8 flags;
> >>         u8 bank_addr_shift;
> >> +       u8 adjoff;
> >>         void (*read_post)(unsigned int off, char *buf, size_t count);
> >>  };
> >>
> >> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> >> +#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao)                    \
> >
> > Please, don't try to save space on a few letters, call it
> > AT24_CHIP_DATA_ADJOFF() for better readability.
> >
>
> I will change in next the version.
>
> >>         static const struct at24_chip_data _name = {                    \
> >>                 .byte_len = _len, .flags = _flags,                      \
> >> +               .adjoff = _ao                                           \
> >>         }
> >>
> >> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> >> +#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> >> +       AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
> >> +
> >> +#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post)     \
> >>         static const struct at24_chip_data _name = {                    \
> >>                 .byte_len = _len, .flags = _flags,                      \
> >> +               .adjoff = _ao,                                          \
> >>                 .read_post = _read_post,                                \
> >>         }
> >>
> >> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> >> +       AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
> >> +
> >>  #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift)       \
> >>         static const struct at24_chip_data _name = {                    \
> >>                 .byte_len = _len, .flags = _flags,                      \
> >> @@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
> >>  AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs02, 16,
> >>         AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> >> +AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >
> > And this will not break existing users? I guess you refer to these
> > changes in your commit message but it's not very clear what you're
> > doing and why.
> >
>
> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
> So, indeed, it is an entanglement in logic.
> To keep the implementation as it is:
> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
>
> I think that is enough not to break the existing users. What are your thoughts?
>

Wait... is the adjoff field effectively a boolean? Why u8?

Bart

> Best Regards,
> Andrei Simion
>
> >> +AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >> +AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
> >>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> >> +AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
> >>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >>  /* spd is a 24c02 in memory DIMMs */
> >>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> >> @@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
> >>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
> >>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
> >>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
> >> +       { "24aa025e48", (kernel_ulong_t)&at24_data_24aa025e48 },
> >> +       { "24aa025e64", (kernel_ulong_t)&at24_data_24aa025e64 },
> >>         { "spd",        (kernel_ulong_t)&at24_data_spd },
> >>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
> >>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
> >> @@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
> >>         { .compatible = "atmel,24c1024",        .data = &at24_data_24c1024 },
> >>         { .compatible = "atmel,24c1025",        .data = &at24_data_24c1025 },
> >>         { .compatible = "atmel,24c2048",        .data = &at24_data_24c2048 },
> >> +       { .compatible = "microchip,24aa025e48", .data = &at24_data_24aa025e48 },
> >> +       { .compatible = "microchip,24aa025e64", .data = &at24_data_24aa025e64 },
> >>         { /* END OF LIST */ },
> >>  };
> >>  MODULE_DEVICE_TABLE(of, at24_of_match);
> >> @@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
> >>         at24->read_post = cdata->read_post;
> >>         at24->bank_addr_shift = cdata->bank_addr_shift;
> >>         at24->num_addresses = num_addresses;
> >> -       at24->offset_adj = at24_get_offset_adj(flags, byte_len);
> >> +       at24->offset_adj = cdata->adjoff ?
> >> +                               at24_get_offset_adj(flags, byte_len) : 0;
> >>         at24->client_regmaps[0] = regmap;
> >>
> >>         at24->vcc_reg = devm_regulator_get(dev, "vcc");
> >> --
> >> 2.34.1
> >>
> >
> > Bart

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Andrei.Simion]

On 28.06.2024 17:46, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jun 28, 2024 at 4:17 PM <Andrei.Simion@microchip.com> wrote:
>>
>> On 28.06.2024 11:30, Bartosz Golaszewski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Fri, Jun 28, 2024 at 10:02 AM Andrei Simion
>>> <andrei.simion@microchip.com> wrote:
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>
>>>> The EEPROMs could be used only for MAC storage. In this case the
>>>> EEPROM areas where MACs resides could be modeled as NVMEM cells
>>>> (directly via DT bindings) such that the already available networking
>>>> infrastructure to read properly the MAC addresses (via
>>>> of_get_mac_address()). The previously available compatibles needs the
>>>> offset adjustment probably for compatibility w/ old DT bindings.
>>>> Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
>>>> usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
>>>> for EUI-48 address and "24aa025e64" stands for EUI-64 address.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> [andrei.simion@microchip.com: Add extended macros to initialize the structure
>>>> with explicit value to adjusting offset. Add extra description for the commit
>>>> message.]
>>>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>>>> ---
>>>> v2 -> v3:
>>>> - add specific compatible names according with
>>>> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
>>>> - add extended macros to initialize the structure with explicit value for adjoff
>>>> - drop co-developed-by to maintain the commit history
>>>>  (chronological order of modifications)
>>>>
>>>> v1 -> v2:
>>>> - no change
>>>> ---
>>>>  drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
>>>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>> index 4bd4f32bcdab..e2ac08f656cf 100644
>>>> --- a/drivers/misc/eeprom/at24.c
>>>> +++ b/drivers/misc/eeprom/at24.c
>>>> @@ -121,20 +121,29 @@ struct at24_chip_data {
>>>>         u32 byte_len;
>>>>         u8 flags;
>>>>         u8 bank_addr_shift;
>>>> +       u8 adjoff;
>>>>         void (*read_post)(unsigned int off, char *buf, size_t count);
>>>>  };
>>>>
>>>> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>>>> +#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao)                    \
>>>
>>> Please, don't try to save space on a few letters, call it
>>> AT24_CHIP_DATA_ADJOFF() for better readability.
>>>
>>
>> I will change in next the version.
>>
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> +               .adjoff = _ao                                           \
>>>>         }
>>>>
>>>> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>>>> +#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>>>> +       AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
>>>> +
>>>> +#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post)     \
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> +               .adjoff = _ao,                                          \
>>>>                 .read_post = _read_post,                                \
>>>>         }
>>>>
>>>> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>>>> +       AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
>>>> +
>>>>  #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift)       \
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> @@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
>>>>  AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs02, 16,
>>>>         AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
>>>> +AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>>
>>> And this will not break existing users? I guess you refer to these
>>> changes in your commit message but it's not very clear what you're
>>> doing and why.
>>>
>>
>> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
>> So, indeed, it is an entanglement in logic.
>> To keep the implementation as it is:
>> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
>>
>> I think that is enough not to break the existing users. What are your thoughts?
>>
> 
> Wait... is the adjoff field effectively a boolean? Why u8?
> 

struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
Yes, adjoff needs to be treated as a boolean. I will change it in the next version.

Best Regards,
Andrei

> Bart
> 
>> Best Regards,
>> Andrei Simion
>>
>>>> +AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>>> +AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
>>>>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
>>>> +AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
>>>>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>>>  /* spd is a 24c02 in memory DIMMs */
>>>>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
>>>> @@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
>>>>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>>>>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
>>>>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
>>>> +       { "24aa025e48", (kernel_ulong_t)&at24_data_24aa025e48 },
>>>> +       { "24aa025e64", (kernel_ulong_t)&at24_data_24aa025e64 },
>>>>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>>>>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>>>>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
>>>> @@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
>>>>         { .compatible = "atmel,24c1024",        .data = &at24_data_24c1024 },
>>>>         { .compatible = "atmel,24c1025",        .data = &at24_data_24c1025 },
>>>>         { .compatible = "atmel,24c2048",        .data = &at24_data_24c2048 },
>>>> +       { .compatible = "microchip,24aa025e48", .data = &at24_data_24aa025e48 },
>>>> +       { .compatible = "microchip,24aa025e64", .data = &at24_data_24aa025e64 },
>>>>         { /* END OF LIST */ },
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, at24_of_match);
>>>> @@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
>>>>         at24->read_post = cdata->read_post;
>>>>         at24->bank_addr_shift = cdata->bank_addr_shift;
>>>>         at24->num_addresses = num_addresses;
>>>> -       at24->offset_adj = at24_get_offset_adj(flags, byte_len);
>>>> +       at24->offset_adj = cdata->adjoff ?
>>>> +                               at24_get_offset_adj(flags, byte_len) : 0;
>>>>         at24->client_regmaps[0] = regmap;
>>>>
>>>>         at24->vcc_reg = devm_regulator_get(dev, "vcc");
>>>> --
>>>> 2.34.1
>>>>
>>>
>>> Bart

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Bartosz Golaszewski]

On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote:
>
> >>
> >> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
> >> So, indeed, it is an entanglement in logic.
> >> To keep the implementation as it is:
> >> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
> >>
> >> I think that is enough not to break the existing users. What are your thoughts?
> >>
> >
> > Wait... is the adjoff field effectively a boolean? Why u8?
> >
>
> struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
> Yes, adjoff needs to be treated as a boolean. I will change it in the next version.
>

No, wait. Why can't you just do:

AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY);

and avoid this whole new macro variant entirely?

Bart

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Andrei.Simion]

On 01.07.2024 11:46, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote:
>>
>>>>
>>>> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
>>>> So, indeed, it is an entanglement in logic.
>>>> To keep the implementation as it is:
>>>> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
>>>>
>>>> I think that is enough not to break the existing users. What are your thoughts?
>>>>
>>>
>>> Wait... is the adjoff field effectively a boolean? Why u8?
>>>
>>
>> struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
>> Yes, adjoff needs to be treated as a boolean. I will change it in the next version.
>>
> 
> No, wait. Why can't you just do:
> 
> AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY);
> 
> and avoid this whole new macro variant entirely?
> 

just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY):
# hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
00000000  ff ff ff ff ff ff                                 |......|
00000006
# hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
00000000  ff ff ff ff ff ff                                 |......|
00000006

with this patch (adjoff false and new macro)
# hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
00000000  04 91 62 [the rest bytes]                                 |..b...|
00000006
# hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
00000000  04 91 62 [the rest bytes]                                 |..b..m|
00000006
#

> Bart

-- 
Andrei Simion
MPU32 Engineer|Microchip Technology Inc.

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Bartosz Golaszewski]

On Mon, Jul 1, 2024 at 12:20 PM <Andrei.Simion@microchip.com> wrote:
>
> On 01.07.2024 11:46, Bartosz Golaszewski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote:
> >>
> >>>>
> >>>> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
> >>>> So, indeed, it is an entanglement in logic.
> >>>> To keep the implementation as it is:
> >>>> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
> >>>>
> >>>> I think that is enough not to break the existing users. What are your thoughts?
> >>>>
> >>>
> >>> Wait... is the adjoff field effectively a boolean? Why u8?
> >>>
> >>
> >> struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
> >> Yes, adjoff needs to be treated as a boolean. I will change it in the next version.
> >>
> >
> > No, wait. Why can't you just do:
> >
> > AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY);
> >
> > and avoid this whole new macro variant entirely?
> >
>
> just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY):
> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
> 00000000  ff ff ff ff ff ff                                 |......|
> 00000006
> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
> 00000000  ff ff ff ff ff ff                                 |......|
> 00000006
>
> with this patch (adjoff false and new macro)
> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
> 00000000  04 91 62 [the rest bytes]                                 |..b...|
> 00000006
> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
> 00000000  04 91 62 [the rest bytes]                                 |..b..m|
> 00000006
> #
>

Ok, but your goal is for at24_get_offset_adj() to return 0, isn't it?
This is what line

at24->offset_adj = cdata->adjoff ? at24_get_offset_adj(flags, byte_len) : 0;

is effectively achieving. What's the difference between this patch and
the solution I'm proposing? Isn't the offset_adj field 0 in both
cases? Is there any other difference I'm not seeing?

Because I still think we can avoid all this churn.

Bart

Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[Andrei.Simion]

On 01.07.2024 14:16, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Jul 1, 2024 at 12:20 PM <Andrei.Simion@microchip.com> wrote:
>>
>> On 01.07.2024 11:46, Bartosz Golaszewski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote:
>>>>
>>>>>>
>>>>>> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
>>>>>> So, indeed, it is an entanglement in logic.
>>>>>> To keep the implementation as it is:
>>>>>> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
>>>>>>
>>>>>> I think that is enough not to break the existing users. What are your thoughts?
>>>>>>
>>>>>
>>>>> Wait... is the adjoff field effectively a boolean? Why u8?
>>>>>
>>>>
>>>> struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
>>>> Yes, adjoff needs to be treated as a boolean. I will change it in the next version.
>>>>
>>>
>>> No, wait. Why can't you just do:
>>>
>>> AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY);
>>>
>>> and avoid this whole new macro variant entirely?
>>>
>>
>> just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY):
>> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
>> 00000000  ff ff ff ff ff ff                                 |......|
>> 00000006
>> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
>> 00000000  ff ff ff ff ff ff                                 |......|
>> 00000006
>>
>> with this patch (adjoff false and new macro)
>> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0
>> 00000000  04 91 62 [the rest bytes]                                 |..b...|
>> 00000006
>> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0
>> 00000000  04 91 62 [the rest bytes]                                 |..b..m|
>> 00000006
>> #
>>
> 
> Ok, but your goal is for at24_get_offset_adj() to return 0, isn't it?
> This is what line
> 
> at24->offset_adj = cdata->adjoff ? at24_get_offset_adj(flags, byte_len) : 0;
> 
> is effectively achieving. What's the difference between this patch and
> the solution I'm proposing? Isn't the offset_adj field 0 in both
> cases? Is there any other difference I'm not seeing?
> 
> Because I still think we can avoid all this churn.
> 

I've rechecked what you said and see the function implementation at24_get_offset_adj(flags, byte_len) and made a mistake.
I didn't see that you said only AT24_FLAG_READONLY. (Sorry for the wrong output)
Ok, if I put only AT24_FLAG_READONLY then at24_get_offset_adj(flags,  byte_len)  returns 0 -> I've got in both 'cells/eui48@fa\,0' the MAC address.

Best Regards,
Andrei

> Bart

[PATCH v3 2/3] ARM: dts: at91: at91-sama7g5ek: add EEPROMs

[Andrei Simion]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add EEPROMs and nvmem-layout to describe eui48 mac address region.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
[andrei.simion@microchip.com: add nvmem-layout to describe eui48 mac region
align compatible name with datasheet]
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
---
v2 -> v3:
- change from atmel,24mac02e4 to microchip,24aa025e48 to align with the datasheet
- drop co-developed-by to maintain the chronological order of the changes

v1 -> v2:
- remove unnecessary #address-cells #size-cells
---
 .../arm/boot/dts/microchip/at91-sama7g5ek.dts | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts b/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts
index 20b2497657ae..40f4480e298b 100644
--- a/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts
+++ b/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts
@@ -403,6 +403,42 @@ i2c8: i2c@600 {
 		i2c-digital-filter;
 		i2c-digital-filter-width-ns = <35>;
 		status = "okay";
+
+		eeprom0: eeprom@52 {
+			compatible = "microchip,24aa025e48";
+			reg = <0x52>;
+			size = <256>;
+			pagesize = <16>;
+			vcc-supply = <&vdd_3v3>;
+
+			nvmem-layout {
+				compatible = "fixed-layout";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				eeprom0_eui48: eui48@fa {
+					reg = <0xfa 0x6>;
+				};
+			};
+		};
+
+		eeprom1: eeprom@53 {
+			compatible = "microchip,24aa025e48";
+			reg = <0x53>;
+			size = <256>;
+			pagesize = <16>;
+			vcc-supply = <&vdd_3v3>;
+
+			nvmem-layout {
+				compatible = "fixed-layout";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				eeprom1_eui48: eui48@fa {
+					reg = <0xfa 0x6>;
+				};
+			};
+		};
 	};
 };
 
@@ -440,6 +476,8 @@ &pinctrl_gmac0_mdio_default
 		     &pinctrl_gmac0_txck_default
 		     &pinctrl_gmac0_phy_irq>;
 	phy-mode = "rgmii-id";
+	nvmem-cells = <&eeprom0_eui48>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 
 	ethernet-phy@7 {
@@ -457,6 +495,8 @@ &gmac1 {
 		     &pinctrl_gmac1_mdio_default
 		     &pinctrl_gmac1_phy_irq>;
 	phy-mode = "rmii";
+	nvmem-cells = <&eeprom1_eui48>;
+	nvmem-cell-names = "mac-address";
 	status = "okay"; /* Conflict with pdmc0. */
 
 	ethernet-phy@0 {
-- 
2.34.1

[PATCH v3 3/3] dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64

[Andrei Simion]

Add compatible for Microchip 24AA025E48/24AA025E64 EEPROMs.

Reviewed-by: Connor Dooley <conor.dooley@microchip.com>
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
---
v2 -> v3:
- commit subject changed to reference Microchip 24AA025E48/24AA025E64
- drop the pattern: mac02e4$ and mac02e6$ and a-z from regex
- add these two devices down at the bottom
- added Reviewed-by

v1 -> v2:
- change pattern into "^atmel,(24(c|cs|mac)[a-z0-9]+|spd)$" to keep simpler
---
 Documentation/devicetree/bindings/eeprom/at24.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
index 3c36cd0510de..699c2bbc16f5 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.yaml
+++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
@@ -132,6 +132,10 @@ properties:
               - renesas,r1ex24128
               - samsung,s524ad0xd1
           - const: atmel,24c128
+      - items:
+          - const: microchip,24aa025e48
+      - items:
+          - const: microchip,24aa025e64
       - pattern: '^atmel,24c(32|64)d-wl$' # Actual vendor is st
 
   label:
-- 
2.34.1

Re: [PATCH v3 3/3] dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]

Hey,

On Fri, Jun 28, 2024 at 11:01:46AM +0300, Andrei Simion wrote:
> Add compatible for Microchip 24AA025E48/24AA025E64 EEPROMs.
> 
> Reviewed-by: Connor Dooley <conor.dooley@microchip.com>
                 ^^             ^
There's no way that I provided a tag with my name spelt incorrectly
given I use a macro to insert them. Please copy-paste tags or use b4
to pick them up, rather than type them out yourself.

> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> ---
> v2 -> v3:
> - commit subject changed to reference Microchip 24AA025E48/24AA025E64
> - drop the pattern: mac02e4$ and mac02e6$ and a-z from regex
> - add these two devices down at the bottom
> - added Reviewed-by
> 
> v1 -> v2:
> - change pattern into "^atmel,(24(c|cs|mac)[a-z0-9]+|spd)$" to keep simpler
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> index 3c36cd0510de..699c2bbc16f5 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -132,6 +132,10 @@ properties:
>                - renesas,r1ex24128
>                - samsung,s524ad0xd1
>            - const: atmel,24c128
> +      - items:
> +          - const: microchip,24aa025e48
> +      - items:
> +          - const: microchip,24aa025e64

I don't think this patch works, the schema has a select in it that only
matches ^atmel,(24(c|cs|mac)[0-9]+|spd)$. You either need to have these
fall back to an existing compatible (iff actually compatible) or else do
something like:
diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
index 699c2bbc16f5..4d46b8c5439d 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.yaml
+++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
@@ -18,7 +18,9 @@ select:
   properties:
     compatible:
       contains:
-        pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
+        anyOf:
+          - pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
+          - enum: ["microchip,24aa025e48", "microchip,24aa025e64"]

Thanks,
Conor.

>        - pattern: '^atmel,24c(32|64)d-wl$' # Actual vendor is st
>  
>    label:
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/3] dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64

[Andrei.Simion]

On 28.06.2024 12:09, Conor Dooley wrote:
> Hey,
> 
> On Fri, Jun 28, 2024 at 11:01:46AM +0300, Andrei Simion wrote:
>> Add compatible for Microchip 24AA025E48/24AA025E64 EEPROMs.
>>
>> Reviewed-by: Connor Dooley <conor.dooley@microchip.com>
>                  ^^             ^
> There's no way that I provided a tag with my name spelt incorrectly
> given I use a macro to insert them. Please copy-paste tags or use b4
> to pick them up, rather than type them out yourself.
> 
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> ---
>> v2 -> v3:
>> - commit subject changed to reference Microchip 24AA025E48/24AA025E64
>> - drop the pattern: mac02e4$ and mac02e6$ and a-z from regex
>> - add these two devices down at the bottom
>> - added Reviewed-by
>>
>> v1 -> v2:
>> - change pattern into "^atmel,(24(c|cs|mac)[a-z0-9]+|spd)$" to keep simpler
>> ---
>>  Documentation/devicetree/bindings/eeprom/at24.yaml | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
>> index 3c36cd0510de..699c2bbc16f5 100644
>> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
>> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
>> @@ -132,6 +132,10 @@ properties:
>>                - renesas,r1ex24128
>>                - samsung,s524ad0xd1
>>            - const: atmel,24c128
>> +      - items:
>> +          - const: microchip,24aa025e48
>> +      - items:
>> +          - const: microchip,24aa025e64
> 
> I don't think this patch works, the schema has a select in it that only
> matches ^atmel,(24(c|cs|mac)[0-9]+|spd)$. You either need to have these
> fall back to an existing compatible (iff actually compatible) or else do
> something like:
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> index 699c2bbc16f5..4d46b8c5439d 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -18,7 +18,9 @@ select:
>    properties:
>      compatible:
>        contains:
> -        pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
> +        anyOf:
> +          - pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
> +          - enum: ["microchip,24aa025e48", "microchip,24aa025e64"]
> 
> Thanks,
> Conor.
> 

If I use your solution (and drop both items: const) -> it is selected the compatible but does not match anything.

What do you think about :

diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
index 699c2bbc16f5..bfaf7eac658a 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.yaml
+++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
@@ -18,7 +18,7 @@ select:
   properties:
     compatible:
       contains:
-        pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
+        pattern: "^(atmel|microchip),(24(aa|c|cs|mac)[0-9]+|spd)$"
   required:
     - compatible
 
@@ -37,9 +37,13 @@ properties:
       - allOf:
           - minItems: 1
             items:
-              - pattern: "^(atmel|catalyst|microchip|nxp|ramtron|renesas|rohm|st),(24(c|cs|lc|mac)[0-9]+|spd)$"
+              - pattern: "^(atmel|catalyst|microchip|nxp|ramtron|renesas|rohm|st),(24(aa|c|cs|lc|mac)[0-9]+|spd)$"
               - pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
           - oneOf:
+              - items:
+                  pattern: aa025e48$
+              - items:
+                  pattern: aa025e64$
               - items:
                   pattern: c00$
               - items:

Best Regards,
Andrei 


>>        - pattern: '^atmel,24c(32|64)d-wl$' # Actual vendor is st
>>  
>>    label:
>> -- 
>> 2.34.1
>>

Re: [PATCH v3 3/3] dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2785 bytes --]

On Mon, Jul 01, 2024 at 02:37:57PM +0000, Andrei.Simion@microchip.com wrote:
> On 28.06.2024 12:09, Conor Dooley wrote:
> > Hey,
> > 
> > On Fri, Jun 28, 2024 at 11:01:46AM +0300, Andrei Simion wrote:
> >> Add compatible for Microchip 24AA025E48/24AA025E64 EEPROMs.
> >>
> >> Reviewed-by: Connor Dooley <conor.dooley@microchip.com>
> >                  ^^             ^
> > There's no way that I provided a tag with my name spelt incorrectly
> > given I use a macro to insert them. Please copy-paste tags or use b4
> > to pick them up, rather than type them out yourself.
> > 
> >> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> >> ---
> >> v2 -> v3:
> >> - commit subject changed to reference Microchip 24AA025E48/24AA025E64
> >> - drop the pattern: mac02e4$ and mac02e6$ and a-z from regex
> >> - add these two devices down at the bottom
> >> - added Reviewed-by
> >>
> >> v1 -> v2:
> >> - change pattern into "^atmel,(24(c|cs|mac)[a-z0-9]+|spd)$" to keep simpler
> >> ---
> >>  Documentation/devicetree/bindings/eeprom/at24.yaml | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> >> index 3c36cd0510de..699c2bbc16f5 100644
> >> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> >> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> >> @@ -132,6 +132,10 @@ properties:
> >>                - renesas,r1ex24128
> >>                - samsung,s524ad0xd1
> >>            - const: atmel,24c128
> >> +      - items:
> >> +          - const: microchip,24aa025e48
> >> +      - items:
> >> +          - const: microchip,24aa025e64
> > 
> > I don't think this patch works, the schema has a select in it that only
> > matches ^atmel,(24(c|cs|mac)[0-9]+|spd)$. You either need to have these
> > fall back to an existing compatible (iff actually compatible) or else do
> > something like:
> > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > index 699c2bbc16f5..4d46b8c5439d 100644
> > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > @@ -18,7 +18,9 @@ select:
> >    properties:
> >      compatible:
> >        contains:
> > -        pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
> > +        anyOf:
> > +          - pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
> > +          - enum: ["microchip,24aa025e48", "microchip,24aa025e64"]
> 
> If I use your solution (and drop both items: const) -> it is selected the compatible but does not match anything.

My intention was that you added my diff to your existing patch.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/3] dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64

[Andrei.Simion]

On 01.07.2024 17:47, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 
> ForwardedMessage.eml
> 
> Subject:
> Re: [PATCH v3 3/3] dt-bindings: eeprom: at24: Add Microchip 24AA025E48/24AA025E64
> From:
> Conor Dooley <conor@kernel.org>
> Date:
> 01.07.2024, 17:47
> 
> To:
> Andrei.Simion@microchip.com
> CC:
> Conor.Dooley@microchip.com, brgl@bgdev.pl, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, Nicolas.Ferre@microchip.com, alexandre.belloni@bootlin.com, claudiu.beznea@tuxon.dev, arnd@arndb.de, gregkh@linuxfoundation.org, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> 
> 
> 
> On Mon, Jul 01, 2024 at 02:37:57PM +0000, Andrei.Simion@microchip.com wrote:
>> On 28.06.2024 12:09, Conor Dooley wrote:
>>> Hey,
>>>
>>> On Fri, Jun 28, 2024 at 11:01:46AM +0300, Andrei Simion wrote:
>>>> Add compatible for Microchip 24AA025E48/24AA025E64 EEPROMs.
>>>>
>>>> Reviewed-by: Connor Dooley <conor.dooley@microchip.com>
>>>                  ^^             ^
>>> There's no way that I provided a tag with my name spelt incorrectly
>>> given I use a macro to insert them. Please copy-paste tags or use b4
>>> to pick them up, rather than type them out yourself.
>>>
>>>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>>>> ---
>>>> v2 -> v3:
>>>> - commit subject changed to reference Microchip 24AA025E48/24AA025E64
>>>> - drop the pattern: mac02e4$ and mac02e6$ and a-z from regex
>>>> - add these two devices down at the bottom
>>>> - added Reviewed-by
>>>>
>>>> v1 -> v2:
>>>> - change pattern into "^atmel,(24(c|cs|mac)[a-z0-9]+|spd)$" to keep simpler
>>>> ---
>>>>  Documentation/devicetree/bindings/eeprom/at24.yaml | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
>>>> index 3c36cd0510de..699c2bbc16f5 100644
>>>> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
>>>> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
>>>> @@ -132,6 +132,10 @@ properties:
>>>>                - renesas,r1ex24128
>>>>                - samsung,s524ad0xd1
>>>>            - const: atmel,24c128
>>>> +      - items:
>>>> +          - const: microchip,24aa025e48
>>>> +      - items:
>>>> +          - const: microchip,24aa025e64
>>> I don't think this patch works, the schema has a select in it that only
>>> matches ^atmel,(24(c|cs|mac)[0-9]+|spd)$. You either need to have these
>>> fall back to an existing compatible (iff actually compatible) or else do
>>> something like:
>>> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
>>> index 699c2bbc16f5..4d46b8c5439d 100644
>>> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
>>> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
>>> @@ -18,7 +18,9 @@ select:
>>>    properties:
>>>      compatible:
>>>        contains:
>>> -        pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
>>> +        anyOf:
>>> +          - pattern: "^atmel,(24(c|cs|mac)[0-9]+|spd)$"
>>> +          - enum: ["microchip,24aa025e48", "microchip,24aa025e64"]
>> If I use your solution (and drop both items: const) -> it is selected the compatible but does not match anything.
> My intention was that you added my diff to your existing patch.
>

I also tested this scenario, it is OK. 
Thank you for your clarifications.

Andrei

> Thanks,
> Conor.

Re: [PATCH v3 0/3] Read MAC address through NVMEM for sama7g5ek

[Arnd Bergmann]

On Fri, Jun 28, 2024, at 10:01, Andrei Simion wrote:
> This series proposes to add EEPROM support and reading MAC addresses
> through NVMEM (via Devicetree) for sama7g5ek:
> - Add in DT bindings document the EEPROM compatibles :
> "microchip,24aa025e48" and "microchip,24aa025e64"
> - Update to the driver to support "microchip,24aa025e48" and
> "microchip,24aa025e64" and adjusting offset for those 24AA025E{48, 64}.
> - Added the nodes in devicetree for eeproms where are stored EUI-48 MAC,
> and update gmac nodes to read the MAC via devicetree through NVMEM.

Can you add an explanation about what this is good for?

Do you need to work around broken boot loaders that cannot be
updated and that happen to store the MAC address in the EEPROM,
or are you proposing this as a generic solution that board
developers should actually use?

As far as I can tell, even with this logic in place, users
are better off just having the boot loader read the EEPROM
and storing the MAC address in the in-memory dtb as we do
on other platforms.

      Arnd

Re: [PATCH v3 0/3] Read MAC address through NVMEM for sama7g5ek

[Andrei.Simion]

On 28.06.2024 11:29, Arnd Bergmann wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jun 28, 2024, at 10:01, Andrei Simion wrote:
>> This series proposes to add EEPROM support and reading MAC addresses
>> through NVMEM (via Devicetree) for sama7g5ek:
>> - Add in DT bindings document the EEPROM compatibles :
>> "microchip,24aa025e48" and "microchip,24aa025e64"
>> - Update to the driver to support "microchip,24aa025e48" and
>> "microchip,24aa025e64" and adjusting offset for those 24AA025E{48, 64}.
>> - Added the nodes in devicetree for eeproms where are stored EUI-48 MAC,
>> and update gmac nodes to read the MAC via devicetree through NVMEM.
> 
> Can you add an explanation about what this is good for?
> 
> Do you need to work around broken boot loaders that cannot be
> updated and that happen to store the MAC address in the EEPROM,
> or are you proposing this as a generic solution that board
> developers should actually use?
> 
> As far as I can tell, even with this logic in place, users
> are better off just having the boot loader read the EEPROM
> and storing the MAC address in the in-memory dtb as we do
> on other platforms.
> 
>       Arnd


Our boot chain is ROM BOOT -> AT91Bootstrap -> U-Boot -> Linux Kernel. U-Boot is the stage where we set up the MAC address.
We can skip U-Boot and use the following boot chain ROM BOOT -> AT91Boostrap -> Linux Kernel. 
This patch set is useful for this scenario and also for redundancy (if something related with NET/EEPROM fails in U-Boot).

Best Regards,
Andrei Simion

Re: [PATCH v3 0/3] Read MAC address through NVMEM for sama7g5ek

[Arnd Bergmann]

On Fri, Jun 28, 2024, at 16:06, Andrei.Simion@microchip.com wrote:
> On 28.06.2024 11:29, Arnd Bergmann wrote:
>> 
>> As far as I can tell, even with this logic in place, users
>> are better off just having the boot loader read the EEPROM
>> and storing the MAC address in the in-memory dtb as we do
>> on other platforms.
>
> Our boot chain is ROM BOOT -> AT91Bootstrap -> U-Boot -> Linux Kernel. 
> U-Boot is the stage where we set up the MAC address.
> We can skip U-Boot and use the following boot chain ROM BOOT -> 
> AT91Boostrap -> Linux Kernel. 

Right, I can see how that is useful. Can you add that description
in the patch?

> This patch set is useful for this scenario and also for redundancy (if 
> something related with NET/EEPROM fails in U-Boot).

Not sure if redundancy is what we want the boot loader level ;-)

    Arnd