Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
@ 2021-04-02 13:24 Claudiu Beznea
  2021-04-07 18:37 ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Claudiu Beznea @ 2021-04-02 13:24 UTC (permalink / raw)
  To: bgolaszewski, arnd, gregkh; +Cc: linux-i2c, linux-kernel, Claudiu Beznea

Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
the driver to not do offset adjustments.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

Hi Bartosz,

For the previously available compatibles the offset adjustment is done
(probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
is used in setup with macb driver which is calling of_get_mac_address()
to get the proper NVMEM cell in EEPROM where the MAC resides and read
directly from there. We modeled the EEPROM and NVMEM cell in DT as
follows:

&i2cnode {
	// ...
	eeprom0: eeprom0@52 {
		compatible = "atmel,24mac02e4";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x52>;
		pagesize = <16>;
		size = <256>;
		status = "okay";

		eeprom0_eui48: eui48@fa {
			reg = <0xfa 0x6>;
		};
	};
};

&gmac {
	// ...

	nvmem-cells = <&eeprom0_eui48>;
	nvmem-cell-names = "mac-address";

	// ...
};


Let me know if some other approach needs to be taken into account in
at24 driver for this to work.

Thank you,
Claudiu Beznea

 drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 926408b41270..ae2fbcb5e83d 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
 struct at24_chip_data {
 	u32 byte_len;
 	u8 flags;
+	u8 adjoff;
 	void (*read_post)(unsigned int off, char *buf, size_t count);
 };
 
-#define AT24_CHIP_DATA(_name, _len, _flags)				\
+#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)			\
 	static const struct at24_chip_data _name = {			\
-		.byte_len = _len, .flags = _flags,			\
+		.byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
 	}
 
-#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)		\
+#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)	\
 	static const struct at24_chip_data _name = {			\
 		.byte_len = _len, .flags = _flags,			\
+		.adjoff = _adjoff,					\
 		.read_post = _read_post,				\
 	}
 
@@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
 }
 
 /* needs 8 addresses as A0-A2 are ignored */
-AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
+AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
 /* old variants can't be handled with this generic entry! */
-AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
+AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
 AT24_CHIP_DATA(at24_data_24cs01, 16,
-	AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
+	AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
+AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
 AT24_CHIP_DATA(at24_data_24cs02, 16,
-	AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
+	AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
 AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
-	AT24_FLAG_MAC | AT24_FLAG_READONLY);
+	AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
 AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
-	AT24_FLAG_MAC | AT24_FLAG_READONLY);
+	AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
+AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
+	AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
+AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
+	AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
 /* spd is a 24c02 in memory DIMMs */
 AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
-	AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
+	AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
 /* 24c02_vaio is a 24c02 on some Sony laptops */
 AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
-	AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
+	AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
 	at24_read_post_vaio);
-AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
+AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
 AT24_CHIP_DATA(at24_data_24cs04, 16,
-	AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
+	AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
 /* 24rf08 quirk is handled at i2c-core */
-AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
+AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
 AT24_CHIP_DATA(at24_data_24cs08, 16,
-	AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
+	AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
+AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
 AT24_CHIP_DATA(at24_data_24cs16, 16,
-	AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
+	AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
+AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
 AT24_CHIP_DATA(at24_data_24cs32, 16,
-	AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
+	AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
+AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
 AT24_CHIP_DATA(at24_data_24cs64, 16,
-	AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
-AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
-AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
-AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
-AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
-AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
+	AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
+AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
+AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
+AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
+AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
+AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
 /* identical to 24c08 ? */
-AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
+AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
 
 static const struct i2c_device_id at24_ids[] = {
 	{ "24c00",	(kernel_ulong_t)&at24_data_24c00 },
@@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
 	{ "24c02",	(kernel_ulong_t)&at24_data_24c02 },
 	{ "24cs02",	(kernel_ulong_t)&at24_data_24cs02 },
 	{ "24mac402",	(kernel_ulong_t)&at24_data_24mac402 },
+	{ "24mac02e4",	(kernel_ulong_t)&at24_data_24mac02e4 },
 	{ "24mac602",	(kernel_ulong_t)&at24_data_24mac602 },
+	{ "24mac02e6",	(kernel_ulong_t)&at24_data_24mac02e6 },
 	{ "spd",	(kernel_ulong_t)&at24_data_spd },
 	{ "24c02-vaio",	(kernel_ulong_t)&at24_data_24c02_vaio },
 	{ "24c04",	(kernel_ulong_t)&at24_data_24c04 },
@@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
 	{ .compatible = "atmel,24c02",		.data = &at24_data_24c02 },
 	{ .compatible = "atmel,24cs02",		.data = &at24_data_24cs02 },
 	{ .compatible = "atmel,24mac402",	.data = &at24_data_24mac402 },
+	{ .compatible = "atmel,24mac02e4",	.data = &at24_data_24mac02e4 },
 	{ .compatible = "atmel,24mac602",	.data = &at24_data_24mac602 },
+	{ .compatible = "atmel,24mac02e6",	.data = &at24_data_24mac02e6 },
 	{ .compatible = "atmel,spd",		.data = &at24_data_spd },
 	{ .compatible = "atmel,24c04",		.data = &at24_data_24c04 },
 	{ .compatible = "atmel,24cs04",		.data = &at24_data_24cs04 },
@@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
 	at24->flags = flags;
 	at24->read_post = cdata->read_post;
 	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[0].client = client;
 	at24->client[0].regmap = regmap;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  2021-04-02 13:24 [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64} Claudiu Beznea
@ 2021-04-07 18:37 ` Bartosz Golaszewski
  2021-04-12  7:42   ` Claudiu.Beznea
  0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2021-04-07 18:37 UTC (permalink / raw)
  To: Claudiu Beznea; +Cc: Arnd Bergmann, Greg KH, linux-i2c, LKML

On Fri, Apr 2, 2021 at 3:24 PM Claudiu Beznea
<claudiu.beznea@microchip.com> wrote:
>
> Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
> compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
> the driver to not do offset adjustments.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>
> Hi Bartosz,
>
> For the previously available compatibles the offset adjustment is done
> (probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
> is used in setup with macb driver which is calling of_get_mac_address()
> to get the proper NVMEM cell in EEPROM where the MAC resides and read
> directly from there. We modeled the EEPROM and NVMEM cell in DT as
> follows:
>
> &i2cnode {
>         // ...
>         eeprom0: eeprom0@52 {
>                 compatible = "atmel,24mac02e4";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 reg = <0x52>;
>                 pagesize = <16>;
>                 size = <256>;
>                 status = "okay";
>
>                 eeprom0_eui48: eui48@fa {
>                         reg = <0xfa 0x6>;
>                 };
>         };
> };
>
> &gmac {
>         // ...
>
>         nvmem-cells = <&eeprom0_eui48>;
>         nvmem-cell-names = "mac-address";
>
>         // ...
> };
>
>
> Let me know if some other approach needs to be taken into account in
> at24 driver for this to work.
>
> Thank you,
> Claudiu Beznea
>

Hi Claudiu,

First of all: any new compatibles need to go into the DT bindings document.


>  drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 926408b41270..ae2fbcb5e83d 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
>  struct at24_chip_data {
>         u32 byte_len;
>         u8 flags;
> +       u8 adjoff;
>         void (*read_post)(unsigned int off, char *buf, size_t count);
>  };
>
> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> +#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)                   \
>         static const struct at24_chip_data _name = {                    \
> -               .byte_len = _len, .flags = _flags,                      \
> +               .byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
>         }
>
> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)    \
>         static const struct at24_chip_data _name = {                    \
>                 .byte_len = _len, .flags = _flags,                      \
> +               .adjoff = _adjoff,                                      \
>                 .read_post = _read_post,                                \
>         }
>
> @@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
>  }
>
>  /* needs 8 addresses as A0-A2 are ignored */
> -AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
> +AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
>  /* old variants can't be handled with this generic entry! */
> -AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
> +AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
>  AT24_CHIP_DATA(at24_data_24cs01, 16,
> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> +AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
>  AT24_CHIP_DATA(at24_data_24cs02, 16,
> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>  AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>  AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> +AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> +AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
>  /* spd is a 24c02 in memory DIMMs */
>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
>  /* 24c02_vaio is a 24c02 on some Sony laptops */
>  AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
>         at24_read_post_vaio);
> -AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
> +AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
>  AT24_CHIP_DATA(at24_data_24cs04, 16,
> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>  /* 24rf08 quirk is handled at i2c-core */
> -AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
> +AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
>  AT24_CHIP_DATA(at24_data_24cs08, 16,
> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> +AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
>  AT24_CHIP_DATA(at24_data_24cs16, 16,
> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> +AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
>  AT24_CHIP_DATA(at24_data_24cs32, 16,
> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> +AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
>  AT24_CHIP_DATA(at24_data_24cs64, 16,
> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> -AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
> -AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
> -AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
> -AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
> -AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> +AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
> +AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
> +AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
> +AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
> +AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
>  /* identical to 24c08 ? */
> -AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
> +AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
>
>  static const struct i2c_device_id at24_ids[] = {
>         { "24c00",      (kernel_ulong_t)&at24_data_24c00 },
> @@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
>         { "24c02",      (kernel_ulong_t)&at24_data_24c02 },
>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
> +       { "24mac02e4",  (kernel_ulong_t)&at24_data_24mac02e4 },
>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
> +       { "24mac02e6",  (kernel_ulong_t)&at24_data_24mac02e6 },
>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
> @@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
>         { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
>         { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
>         { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
> +       { .compatible = "atmel,24mac02e4",      .data = &at24_data_24mac02e4 },
>         { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
> +       { .compatible = "atmel,24mac02e6",      .data = &at24_data_24mac02e6 },
>         { .compatible = "atmel,spd",            .data = &at24_data_spd },
>         { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
>         { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
> @@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
>         at24->flags = flags;
>         at24->read_post = cdata->read_post;
>         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[0].client = client;
>         at24->client[0].regmap = regmap;
>
> --
> 2.25.1
>

What is the problem you're trying to solve? The MAC area is accessible
on a different device address. The variants with serial and MAC areas
are meant to be instantiated as devices separate from the main
writeable area and you can then create an nvmem cell that will take up
the whole MAC.

Or does your model keep the MAC in the same block that's used for
writing? In which case just using a regular compatible that matches
the size and creating the nvmem cell at the right offset should be
enough, right?

Am I missing something?

Bartosz

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  2021-04-07 18:37 ` Bartosz Golaszewski
@ 2021-04-12  7:42   ` Claudiu.Beznea
  2021-04-12 18:29     ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Claudiu.Beznea @ 2021-04-12  7:42 UTC (permalink / raw)
  To: bgolaszewski; +Cc: arnd, gregkh, linux-i2c, linux-kernel

On 07.04.2021 21:37, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Apr 2, 2021 at 3:24 PM Claudiu Beznea
> <claudiu.beznea@microchip.com> wrote:
>>
>> Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
>> compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
>> the driver to not do offset adjustments.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>
>> Hi Bartosz,
>>
>> For the previously available compatibles the offset adjustment is done
>> (probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
>> is used in setup with macb driver which is calling of_get_mac_address()
>> to get the proper NVMEM cell in EEPROM where the MAC resides and read
>> directly from there. We modeled the EEPROM and NVMEM cell in DT as
>> follows:
>>
>> &i2cnode {
>>         // ...
>>         eeprom0: eeprom0@52 {
>>                 compatible = "atmel,24mac02e4";
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>                 reg = <0x52>;
>>                 pagesize = <16>;
>>                 size = <256>;
>>                 status = "okay";
>>
>>                 eeprom0_eui48: eui48@fa {
>>                         reg = <0xfa 0x6>;
>>                 };
>>         };
>> };
>>
>> &gmac {
>>         // ...
>>
>>         nvmem-cells = <&eeprom0_eui48>;
>>         nvmem-cell-names = "mac-address";
>>
>>         // ...
>> };
>>
>>
>> Let me know if some other approach needs to be taken into account in
>> at24 driver for this to work.
>>
>> Thank you,
>> Claudiu Beznea
>>
> 
> Hi Claudiu,
> 
> First of all: any new compatibles need to go into the DT bindings document.

Agree! I missed this.

> 
> 
>>  drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
>>  1 file changed, 40 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index 926408b41270..ae2fbcb5e83d 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
>>  struct at24_chip_data {
>>         u32 byte_len;
>>         u8 flags;
>> +       u8 adjoff;
>>         void (*read_post)(unsigned int off, char *buf, size_t count);
>>  };
>>
>> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>> +#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)                   \
>>         static const struct at24_chip_data _name = {                    \
>> -               .byte_len = _len, .flags = _flags,                      \
>> +               .byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
>>         }
>>
>> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)    \
>>         static const struct at24_chip_data _name = {                    \
>>                 .byte_len = _len, .flags = _flags,                      \
>> +               .adjoff = _adjoff,                                      \
>>                 .read_post = _read_post,                                \
>>         }
>>
>> @@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
>>  }
>>
>>  /* needs 8 addresses as A0-A2 are ignored */
>> -AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
>> +AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
>>  /* old variants can't be handled with this generic entry! */
>> -AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
>> +AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
>>  AT24_CHIP_DATA(at24_data_24cs01, 16,
>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>> +AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
>>  AT24_CHIP_DATA(at24_data_24cs02, 16,
>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>  AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
>> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>  AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
>> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>> +AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
>> +AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
>>  /* spd is a 24c02 in memory DIMMs */
>>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
>> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
>> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
>>  /* 24c02_vaio is a 24c02 on some Sony laptops */
>>  AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
>> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
>> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
>>         at24_read_post_vaio);
>> -AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
>> +AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
>>  AT24_CHIP_DATA(at24_data_24cs04, 16,
>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>  /* 24rf08 quirk is handled at i2c-core */
>> -AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
>> +AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
>>  AT24_CHIP_DATA(at24_data_24cs08, 16,
>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>> +AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
>>  AT24_CHIP_DATA(at24_data_24cs16, 16,
>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>> +AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
>>  AT24_CHIP_DATA(at24_data_24cs32, 16,
>> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
>> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>> +AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
>>  AT24_CHIP_DATA(at24_data_24cs64, 16,
>> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>> -AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
>> -AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
>> -AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
>> -AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
>> -AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
>> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>> +AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
>> +AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
>> +AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
>> +AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
>> +AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
>>  /* identical to 24c08 ? */
>> -AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
>> +AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
>>
>>  static const struct i2c_device_id at24_ids[] = {
>>         { "24c00",      (kernel_ulong_t)&at24_data_24c00 },
>> @@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
>>         { "24c02",      (kernel_ulong_t)&at24_data_24c02 },
>>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
>> +       { "24mac02e4",  (kernel_ulong_t)&at24_data_24mac02e4 },
>>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
>> +       { "24mac02e6",  (kernel_ulong_t)&at24_data_24mac02e6 },
>>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
>> @@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
>>         { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
>>         { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
>>         { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
>> +       { .compatible = "atmel,24mac02e4",      .data = &at24_data_24mac02e4 },
>>         { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
>> +       { .compatible = "atmel,24mac02e6",      .data = &at24_data_24mac02e6 },
>>         { .compatible = "atmel,spd",            .data = &at24_data_spd },
>>         { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
>>         { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
>> @@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
>>         at24->flags = flags;
>>         at24->read_post = cdata->read_post;
>>         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[0].client = client;
>>         at24->client[0].regmap = regmap;
>>
>> --
>> 2.25.1
>>
> 
> What is the problem you're trying to solve?

I wanted to instantiate a NVMEM cell with proper offset and size via DT and
make whatever Ethernet driver aware of this NVMEM cell via DT bindings, as
bellow:

&i2cnode {
	// ...
	eeprom0: eeprom0@52 {
		compatible = "atmel,24mac02e4";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x52>;
		pagesize = <16>;
		size = <256>;
		status = "okay";

		eeprom0_eui48: eui48@fa {
			reg = <0xfa 0x6>;
		};
	};
};

&gmac {
	// ...

	nvmem-cells = <&eeprom0_eui48>;
	nvmem-cell-names = "mac-address";

	// ...
};

By adding this new compatible and changing the driver to not adjust the
NVMEM cell offset for it but make it use the one provided in DT I was
thinking this may scale for any future EEPROM storing MAC addresses at any
offsets (because these would be provided via DT and not adjusted by driver).

I the current at24 driver I haven't managed to find a compatible in the
driver that mach the offset 0xfa (used to store MAC address on case of
24AA025E48).

> The MAC area is accessible
> on a different device address. The variants with serial and MAC areas
> are meant to be instantiated as devices separate from the main
> writeable area and you can then create an nvmem cell that will take up
> the whole MAC.
> 
> Or does your model keep the MAC in the same block that's used for
> writing? In which case just using a regular compatible that matches
> the size and creating the nvmem cell at the right offset should be
> enough, right?

Meaning describing the NVMEM cell in DT with an offset such that the driver
adjustment +/- that offset to lead to the offset described in EEPROM datasheet?

> 
> Am I missing something?



> 
> Bartosz
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  2021-04-12  7:42   ` Claudiu.Beznea
@ 2021-04-12 18:29     ` Bartosz Golaszewski
  2021-04-13 11:03       ` Claudiu.Beznea
  0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2021-04-12 18:29 UTC (permalink / raw)
  To: Claudiu Beznea; +Cc: Arnd Bergmann, Greg KH, linux-i2c, LKML

On Mon, Apr 12, 2021 at 9:42 AM <Claudiu.Beznea@microchip.com> wrote:
>
> On 07.04.2021 21:37, Bartosz Golaszewski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Fri, Apr 2, 2021 at 3:24 PM Claudiu Beznea
> > <claudiu.beznea@microchip.com> wrote:
> >>
> >> Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
> >> compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
> >> the driver to not do offset adjustments.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >> ---
> >>
> >> Hi Bartosz,
> >>
> >> For the previously available compatibles the offset adjustment is done
> >> (probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
> >> is used in setup with macb driver which is calling of_get_mac_address()
> >> to get the proper NVMEM cell in EEPROM where the MAC resides and read
> >> directly from there. We modeled the EEPROM and NVMEM cell in DT as
> >> follows:
> >>
> >> &i2cnode {
> >>         // ...
> >>         eeprom0: eeprom0@52 {
> >>                 compatible = "atmel,24mac02e4";

Can you point me to the datasheet for this model, google only directs
me to this very email.

From the device tree it looks as if it was just a regular 24c02 EEPROM
with MAC hard-coded at 250-255 bytes, is that right?

Bartosz

> >>                 #address-cells = <1>;
> >>                 #size-cells = <0>;
> >>                 reg = <0x52>;
> >>                 pagesize = <16>;
> >>                 size = <256>;
> >>                 status = "okay";
> >>
> >>                 eeprom0_eui48: eui48@fa {
> >>                         reg = <0xfa 0x6>;
> >>                 };
> >>         };
> >> };
> >>
> >> &gmac {
> >>         // ...
> >>
> >>         nvmem-cells = <&eeprom0_eui48>;
> >>         nvmem-cell-names = "mac-address";
> >>
> >>         // ...
> >> };
> >>
> >>
> >> Let me know if some other approach needs to be taken into account in
> >> at24 driver for this to work.
> >>
> >> Thank you,
> >> Claudiu Beznea
> >>
> >
> > Hi Claudiu,
> >
> > First of all: any new compatibles need to go into the DT bindings document.
>
> Agree! I missed this.
>
> >
> >
> >>  drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
> >>  1 file changed, 40 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >> index 926408b41270..ae2fbcb5e83d 100644
> >> --- a/drivers/misc/eeprom/at24.c
> >> +++ b/drivers/misc/eeprom/at24.c
> >> @@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
> >>  struct at24_chip_data {
> >>         u32 byte_len;
> >>         u8 flags;
> >> +       u8 adjoff;
> >>         void (*read_post)(unsigned int off, char *buf, size_t count);
> >>  };
> >>
> >> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> >> +#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)                   \
> >>         static const struct at24_chip_data _name = {                    \
> >> -               .byte_len = _len, .flags = _flags,                      \
> >> +               .byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
> >>         }
> >>
> >> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> >> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)    \
> >>         static const struct at24_chip_data _name = {                    \
> >>                 .byte_len = _len, .flags = _flags,                      \
> >> +               .adjoff = _adjoff,                                      \
> >>                 .read_post = _read_post,                                \
> >>         }
> >>
> >> @@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
> >>  }
> >>
> >>  /* needs 8 addresses as A0-A2 are ignored */
> >> -AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
> >> +AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
> >>  /* old variants can't be handled with this generic entry! */
> >> -AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs01, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs02, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>  AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> >> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >>  AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> >> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >> +AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
> >> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> >>  /* spd is a 24c02 in memory DIMMs */
> >>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> >> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
> >> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
> >>  /* 24c02_vaio is a 24c02 on some Sony laptops */
> >>  AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
> >> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
> >> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
> >>         at24_read_post_vaio);
> >> -AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs04, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>  /* 24rf08 quirk is handled at i2c-core */
> >> -AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs08, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs16, 16,
> >> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
> >> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs32, 16,
> >> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
> >> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
> >>  AT24_CHIP_DATA(at24_data_24cs64, 16,
> >> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >> -AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
> >> -AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
> >> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >> +AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
> >> +AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
> >>  /* identical to 24c08 ? */
> >> -AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
> >> +AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
> >>
> >>  static const struct i2c_device_id at24_ids[] = {
> >>         { "24c00",      (kernel_ulong_t)&at24_data_24c00 },
> >> @@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
> >>         { "24c02",      (kernel_ulong_t)&at24_data_24c02 },
> >>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
> >>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
> >> +       { "24mac02e4",  (kernel_ulong_t)&at24_data_24mac02e4 },
> >>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
> >> +       { "24mac02e6",  (kernel_ulong_t)&at24_data_24mac02e6 },
> >>         { "spd",        (kernel_ulong_t)&at24_data_spd },
> >>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
> >>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
> >> @@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
> >>         { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
> >>         { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
> >>         { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
> >> +       { .compatible = "atmel,24mac02e4",      .data = &at24_data_24mac02e4 },
> >>         { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
> >> +       { .compatible = "atmel,24mac02e6",      .data = &at24_data_24mac02e6 },
> >>         { .compatible = "atmel,spd",            .data = &at24_data_spd },
> >>         { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
> >>         { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
> >> @@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
> >>         at24->flags = flags;
> >>         at24->read_post = cdata->read_post;
> >>         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[0].client = client;
> >>         at24->client[0].regmap = regmap;
> >>
> >> --
> >> 2.25.1
> >>
> >
> > What is the problem you're trying to solve?
>
> I wanted to instantiate a NVMEM cell with proper offset and size via DT and
> make whatever Ethernet driver aware of this NVMEM cell via DT bindings, as
> bellow:
>
> &i2cnode {
>         // ...
>         eeprom0: eeprom0@52 {
>                 compatible = "atmel,24mac02e4";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 reg = <0x52>;
>                 pagesize = <16>;
>                 size = <256>;
>                 status = "okay";
>
>                 eeprom0_eui48: eui48@fa {
>                         reg = <0xfa 0x6>;
>                 };
>         };
> };
>
> &gmac {
>         // ...
>
>         nvmem-cells = <&eeprom0_eui48>;
>         nvmem-cell-names = "mac-address";
>
>         // ...
> };
>
> By adding this new compatible and changing the driver to not adjust the
> NVMEM cell offset for it but make it use the one provided in DT I was
> thinking this may scale for any future EEPROM storing MAC addresses at any
> offsets (because these would be provided via DT and not adjusted by driver).
>
> I the current at24 driver I haven't managed to find a compatible in the
> driver that mach the offset 0xfa (used to store MAC address on case of
> 24AA025E48).
>
> > The MAC area is accessible
> > on a different device address. The variants with serial and MAC areas
> > are meant to be instantiated as devices separate from the main
> > writeable area and you can then create an nvmem cell that will take up
> > the whole MAC.
> >
> > Or does your model keep the MAC in the same block that's used for
> > writing? In which case just using a regular compatible that matches
> > the size and creating the nvmem cell at the right offset should be
> > enough, right?
>
> Meaning describing the NVMEM cell in DT with an offset such that the driver
> adjustment +/- that offset to lead to the offset described in EEPROM datasheet?
>
> >
> > Am I missing something?
>
>
>
> >
> > Bartosz
> >
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  2021-04-12 18:29     ` Bartosz Golaszewski
@ 2021-04-13 11:03       ` Claudiu.Beznea
  2021-04-16 14:13         ` Bartosz Golaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Claudiu.Beznea @ 2021-04-13 11:03 UTC (permalink / raw)
  To: bgolaszewski; +Cc: arnd, gregkh, linux-i2c, linux-kernel

On 12.04.2021 21:29, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Apr 12, 2021 at 9:42 AM <Claudiu.Beznea@microchip.com> wrote:
>>
>> On 07.04.2021 21:37, Bartosz Golaszewski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Fri, Apr 2, 2021 at 3:24 PM Claudiu Beznea
>>> <claudiu.beznea@microchip.com> wrote:
>>>>
>>>> Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
>>>> compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
>>>> the driver to not do offset adjustments.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> ---
>>>>
>>>> Hi Bartosz,
>>>>
>>>> For the previously available compatibles the offset adjustment is done
>>>> (probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
>>>> is used in setup with macb driver which is calling of_get_mac_address()
>>>> to get the proper NVMEM cell in EEPROM where the MAC resides and read
>>>> directly from there. We modeled the EEPROM and NVMEM cell in DT as
>>>> follows:
>>>>
>>>> &i2cnode {
>>>>         // ...
>>>>         eeprom0: eeprom0@52 {
>>>>                 compatible = "atmel,24mac02e4";
> 
> Can you point me to the datasheet for this model, google only directs
> me to this very email.

This is the datasheet:
https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf

>
>>From the device tree it looks as if it was just a regular 24c02 EEPROM
> with MAC hard-coded at 250-255 bytes, is that right?

Yes, the MAC is hard-coded at 250. But using "24c02" compatible will
involve the offset adjustment in the driver (let me know if I missed
something).

Thank you,
Claudiu

> 
> Bartosz
> 
>>>>                 #address-cells = <1>;
>>>>                 #size-cells = <0>;
>>>>                 reg = <0x52>;
>>>>                 pagesize = <16>;
>>>>                 size = <256>;
>>>>                 status = "okay";
>>>>
>>>>                 eeprom0_eui48: eui48@fa {
>>>>                         reg = <0xfa 0x6>;
>>>>                 };
>>>>         };
>>>> };
>>>>
>>>> &gmac {
>>>>         // ...
>>>>
>>>>         nvmem-cells = <&eeprom0_eui48>;
>>>>         nvmem-cell-names = "mac-address";
>>>>
>>>>         // ...
>>>> };
>>>>
>>>>
>>>> Let me know if some other approach needs to be taken into account in
>>>> at24 driver for this to work.
>>>>
>>>> Thank you,
>>>> Claudiu Beznea
>>>>
>>>
>>> Hi Claudiu,
>>>
>>> First of all: any new compatibles need to go into the DT bindings document.
>>
>> Agree! I missed this.
>>
>>>
>>>
>>>>  drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
>>>>  1 file changed, 40 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>> index 926408b41270..ae2fbcb5e83d 100644
>>>> --- a/drivers/misc/eeprom/at24.c
>>>> +++ b/drivers/misc/eeprom/at24.c
>>>> @@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
>>>>  struct at24_chip_data {
>>>>         u32 byte_len;
>>>>         u8 flags;
>>>> +       u8 adjoff;
>>>>         void (*read_post)(unsigned int off, char *buf, size_t count);
>>>>  };
>>>>
>>>> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>>>> +#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)                   \
>>>>         static const struct at24_chip_data _name = {                    \
>>>> -               .byte_len = _len, .flags = _flags,                      \
>>>> +               .byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
>>>>         }
>>>>
>>>> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>>>> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)    \
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> +               .adjoff = _adjoff,                                      \
>>>>                 .read_post = _read_post,                                \
>>>>         }
>>>>
>>>> @@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
>>>>  }
>>>>
>>>>  /* needs 8 addresses as A0-A2 are ignored */
>>>> -AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
>>>> +AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
>>>>  /* old variants can't be handled with this generic entry! */
>>>> -AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs01, 16,
>>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
>>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs02, 16,
>>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>>  AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
>>>> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>>>  AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
>>>> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>>> +AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
>>>> +AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
>>>>  /* spd is a 24c02 in memory DIMMs */
>>>>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
>>>> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
>>>> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
>>>>  /* 24c02_vaio is a 24c02 on some Sony laptops */
>>>>  AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
>>>> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
>>>> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
>>>>         at24_read_post_vaio);
>>>> -AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs04, 16,
>>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>>  /* 24rf08 quirk is handled at i2c-core */
>>>> -AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs08, 16,
>>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
>>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs16, 16,
>>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
>>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs32, 16,
>>>> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
>>>> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs64, 16,
>>>> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
>>>> -AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
>>>> -AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
>>>> -AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
>>>> -AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
>>>> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
>>>> +AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
>>>>  /* identical to 24c08 ? */
>>>> -AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
>>>> +AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
>>>>
>>>>  static const struct i2c_device_id at24_ids[] = {
>>>>         { "24c00",      (kernel_ulong_t)&at24_data_24c00 },
>>>> @@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
>>>>         { "24c02",      (kernel_ulong_t)&at24_data_24c02 },
>>>>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>>>>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
>>>> +       { "24mac02e4",  (kernel_ulong_t)&at24_data_24mac02e4 },
>>>>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
>>>> +       { "24mac02e6",  (kernel_ulong_t)&at24_data_24mac02e6 },
>>>>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>>>>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>>>>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
>>>> @@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
>>>>         { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
>>>>         { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
>>>>         { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
>>>> +       { .compatible = "atmel,24mac02e4",      .data = &at24_data_24mac02e4 },
>>>>         { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
>>>> +       { .compatible = "atmel,24mac02e6",      .data = &at24_data_24mac02e6 },
>>>>         { .compatible = "atmel,spd",            .data = &at24_data_spd },
>>>>         { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
>>>>         { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
>>>> @@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
>>>>         at24->flags = flags;
>>>>         at24->read_post = cdata->read_post;
>>>>         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[0].client = client;
>>>>         at24->client[0].regmap = regmap;
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>
>>> What is the problem you're trying to solve?
>>
>> I wanted to instantiate a NVMEM cell with proper offset and size via DT and
>> make whatever Ethernet driver aware of this NVMEM cell via DT bindings, as
>> bellow:
>>
>> &i2cnode {
>>         // ...
>>         eeprom0: eeprom0@52 {
>>                 compatible = "atmel,24mac02e4";
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>                 reg = <0x52>;
>>                 pagesize = <16>;
>>                 size = <256>;
>>                 status = "okay";
>>
>>                 eeprom0_eui48: eui48@fa {
>>                         reg = <0xfa 0x6>;
>>                 };
>>         };
>> };
>>
>> &gmac {
>>         // ...
>>
>>         nvmem-cells = <&eeprom0_eui48>;
>>         nvmem-cell-names = "mac-address";
>>
>>         // ...
>> };
>>
>> By adding this new compatible and changing the driver to not adjust the
>> NVMEM cell offset for it but make it use the one provided in DT I was
>> thinking this may scale for any future EEPROM storing MAC addresses at any
>> offsets (because these would be provided via DT and not adjusted by driver).
>>
>> I the current at24 driver I haven't managed to find a compatible in the
>> driver that mach the offset 0xfa (used to store MAC address on case of
>> 24AA025E48).
>>
>>> The MAC area is accessible
>>> on a different device address. The variants with serial and MAC areas
>>> are meant to be instantiated as devices separate from the main
>>> writeable area and you can then create an nvmem cell that will take up
>>> the whole MAC.
>>>
>>> Or does your model keep the MAC in the same block that's used for
>>> writing? In which case just using a regular compatible that matches
>>> the size and creating the nvmem cell at the right offset should be
>>> enough, right?
>>
>> Meaning describing the NVMEM cell in DT with an offset such that the driver
>> adjustment +/- that offset to lead to the offset described in EEPROM datasheet?
>>
>>>
>>> Am I missing something?
>>
>>
>>
>>>
>>> Bartosz
>>>
>>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}
  2021-04-13 11:03       ` Claudiu.Beznea
@ 2021-04-16 14:13         ` Bartosz Golaszewski
  0 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2021-04-16 14:13 UTC (permalink / raw)
  To: Claudiu Beznea; +Cc: Arnd Bergmann, Greg KH, linux-i2c, LKML

On Tue, Apr 13, 2021 at 1:03 PM <Claudiu.Beznea@microchip.com> wrote:
>
> On 12.04.2021 21:29, Bartosz Golaszewski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Mon, Apr 12, 2021 at 9:42 AM <Claudiu.Beznea@microchip.com> wrote:
> >>
> >> On 07.04.2021 21:37, Bartosz Golaszewski wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On Fri, Apr 2, 2021 at 3:24 PM Claudiu Beznea
> >>> <claudiu.beznea@microchip.com> wrote:
> >>>>
> >>>> Some 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()). Add "atmel,24mac02e4", "atmel,24mac02e4"
> >>>> compatible for the usage w/ 24AA025E{48, 64} type of EEPROMs and adapt
> >>>> the driver to not do offset adjustments.
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >>>> ---
> >>>>
> >>>> Hi Bartosz,
> >>>>
> >>>> For the previously available compatibles the offset adjustment is done
> >>>> (probably for compatibility w/ old DT bindings?). In my scenario 24AA025E48
> >>>> is used in setup with macb driver which is calling of_get_mac_address()
> >>>> to get the proper NVMEM cell in EEPROM where the MAC resides and read
> >>>> directly from there. We modeled the EEPROM and NVMEM cell in DT as
> >>>> follows:
> >>>>
> >>>> &i2cnode {
> >>>>         // ...
> >>>>         eeprom0: eeprom0@52 {
> >>>>                 compatible = "atmel,24mac02e4";
> >
> > Can you point me to the datasheet for this model, google only directs
> > me to this very email.
>
> This is the datasheet:
> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
>
> >
> >>From the device tree it looks as if it was just a regular 24c02 EEPROM
> > with MAC hard-coded at 250-255 bytes, is that right?
>
> Yes, the MAC is hard-coded at 250. But using "24c02" compatible will
> involve the offset adjustment in the driver (let me know if I missed
> something).
>

Something seems to be wrong. There's no offset adjustment for "24c02".
Have you tried running i2cdump on the EEPROM's address? Do you see the
MAC?

Bartosz

> Thank you,
> Claudiu
>
> >
> > Bartosz
> >
> >>>>                 #address-cells = <1>;
> >>>>                 #size-cells = <0>;
> >>>>                 reg = <0x52>;
> >>>>                 pagesize = <16>;
> >>>>                 size = <256>;
> >>>>                 status = "okay";
> >>>>
> >>>>                 eeprom0_eui48: eui48@fa {
> >>>>                         reg = <0xfa 0x6>;
> >>>>                 };
> >>>>         };
> >>>> };
> >>>>
> >>>> &gmac {
> >>>>         // ...
> >>>>
> >>>>         nvmem-cells = <&eeprom0_eui48>;
> >>>>         nvmem-cell-names = "mac-address";
> >>>>
> >>>>         // ...
> >>>> };
> >>>>
> >>>>
> >>>> Let me know if some other approach needs to be taken into account in
> >>>> at24 driver for this to work.
> >>>>
> >>>> Thank you,
> >>>> Claudiu Beznea
> >>>>
> >>>
> >>> Hi Claudiu,
> >>>
> >>> First of all: any new compatibles need to go into the DT bindings document.
> >>
> >> Agree! I missed this.
> >>
> >>>
> >>>
> >>>>  drivers/misc/eeprom/at24.c | 69 ++++++++++++++++++++++----------------
> >>>>  1 file changed, 40 insertions(+), 29 deletions(-)
> >>>>
> >>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >>>> index 926408b41270..ae2fbcb5e83d 100644
> >>>> --- a/drivers/misc/eeprom/at24.c
> >>>> +++ b/drivers/misc/eeprom/at24.c
> >>>> @@ -123,17 +123,19 @@ MODULE_PARM_DESC(at24_write_timeout, "Time (in ms) to try writes (default 25)");
> >>>>  struct at24_chip_data {
> >>>>         u32 byte_len;
> >>>>         u8 flags;
> >>>> +       u8 adjoff;
> >>>>         void (*read_post)(unsigned int off, char *buf, size_t count);
> >>>>  };
> >>>>
> >>>> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
> >>>> +#define AT24_CHIP_DATA(_name, _len, _flags, _adjoff)                   \
> >>>>         static const struct at24_chip_data _name = {                    \
> >>>> -               .byte_len = _len, .flags = _flags,                      \
> >>>> +               .byte_len = _len, .flags = _flags, .adjoff = _adjoff, \
> >>>>         }
> >>>>
> >>>> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
> >>>> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _adjoff, _read_post)    \
> >>>>         static const struct at24_chip_data _name = {                    \
> >>>>                 .byte_len = _len, .flags = _flags,                      \
> >>>> +               .adjoff = _adjoff,                                      \
> >>>>                 .read_post = _read_post,                                \
> >>>>         }
> >>>>
> >>>> @@ -158,48 +160,52 @@ static void at24_read_post_vaio(unsigned int off, char *buf, size_t count)
> >>>>  }
> >>>>
> >>>>  /* needs 8 addresses as A0-A2 are ignored */
> >>>> -AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR);
> >>>> +AT24_CHIP_DATA(at24_data_24c00, 128 / 8, AT24_FLAG_TAKE8ADDR, 0);
> >>>>  /* old variants can't be handled with this generic entry! */
> >>>> -AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c01, 1024 / 8, 0, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs01, 16,
> >>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> -AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
> >>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs02, 16,
> >>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
> >>>> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >>>>  AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
> >>>> -       AT24_FLAG_MAC | AT24_FLAG_READONLY);
> >>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
> >>>> +AT24_CHIP_DATA(at24_data_24mac02e4, 48 / 8,
> >>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24mac02e6, 64 / 8,
> >>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 0);
> >>>>  /* spd is a 24c02 in memory DIMMs */
> >>>>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
> >>>> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO);
> >>>> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0);
> >>>>  /* 24c02_vaio is a 24c02 on some Sony laptops */
> >>>>  AT24_CHIP_DATA_CB(at24_data_24c02_vaio, 2048 / 8,
> >>>> -       AT24_FLAG_READONLY | AT24_FLAG_IRUGO,
> >>>> +       AT24_FLAG_READONLY | AT24_FLAG_IRUGO, 0,
> >>>>         at24_read_post_vaio);
> >>>> -AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c04, 4096 / 8, 0, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs04, 16,
> >>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>>  /* 24rf08 quirk is handled at i2c-core */
> >>>> -AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c08, 8192 / 8, 0, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs08, 16,
> >>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> -AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0);
> >>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c16, 16384 / 8, 0, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs16, 16,
> >>>> -       AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> -AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16);
> >>>> +       AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c32, 32768 / 8, AT24_FLAG_ADDR16, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs32, 16,
> >>>> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> -AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16);
> >>>> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c64, 65536 / 8, AT24_FLAG_ADDR16, 0);
> >>>>  AT24_CHIP_DATA(at24_data_24cs64, 16,
> >>>> -       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
> >>>> -AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16);
> >>>> -AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16);
> >>>> -AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16);
> >>>> -AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16);
> >>>> -AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16);
> >>>> +       AT24_FLAG_ADDR16 | AT24_FLAG_SERIAL | AT24_FLAG_READONLY, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c128, 131072 / 8, AT24_FLAG_ADDR16, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c256, 262144 / 8, AT24_FLAG_ADDR16, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c512, 524288 / 8, AT24_FLAG_ADDR16, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c1024, 1048576 / 8, AT24_FLAG_ADDR16, 0);
> >>>> +AT24_CHIP_DATA(at24_data_24c2048, 2097152 / 8, AT24_FLAG_ADDR16, 0);
> >>>>  /* identical to 24c08 ? */
> >>>> -AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0);
> >>>> +AT24_CHIP_DATA(at24_data_INT3499, 8192 / 8, 0, 0);
> >>>>
> >>>>  static const struct i2c_device_id at24_ids[] = {
> >>>>         { "24c00",      (kernel_ulong_t)&at24_data_24c00 },
> >>>> @@ -208,7 +214,9 @@ static const struct i2c_device_id at24_ids[] = {
> >>>>         { "24c02",      (kernel_ulong_t)&at24_data_24c02 },
> >>>>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
> >>>>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
> >>>> +       { "24mac02e4",  (kernel_ulong_t)&at24_data_24mac02e4 },
> >>>>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
> >>>> +       { "24mac02e6",  (kernel_ulong_t)&at24_data_24mac02e6 },
> >>>>         { "spd",        (kernel_ulong_t)&at24_data_spd },
> >>>>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
> >>>>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
> >>>> @@ -238,7 +246,9 @@ static const struct of_device_id at24_of_match[] = {
> >>>>         { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
> >>>>         { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
> >>>>         { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
> >>>> +       { .compatible = "atmel,24mac02e4",      .data = &at24_data_24mac02e4 },
> >>>>         { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
> >>>> +       { .compatible = "atmel,24mac02e6",      .data = &at24_data_24mac02e6 },
> >>>>         { .compatible = "atmel,spd",            .data = &at24_data_spd },
> >>>>         { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
> >>>>         { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
> >>>> @@ -690,7 +700,8 @@ static int at24_probe(struct i2c_client *client)
> >>>>         at24->flags = flags;
> >>>>         at24->read_post = cdata->read_post;
> >>>>         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[0].client = client;
> >>>>         at24->client[0].regmap = regmap;
> >>>>
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>> What is the problem you're trying to solve?
> >>
> >> I wanted to instantiate a NVMEM cell with proper offset and size via DT and
> >> make whatever Ethernet driver aware of this NVMEM cell via DT bindings, as
> >> bellow:
> >>
> >> &i2cnode {
> >>         // ...
> >>         eeprom0: eeprom0@52 {
> >>                 compatible = "atmel,24mac02e4";
> >>                 #address-cells = <1>;
> >>                 #size-cells = <0>;
> >>                 reg = <0x52>;
> >>                 pagesize = <16>;
> >>                 size = <256>;
> >>                 status = "okay";
> >>
> >>                 eeprom0_eui48: eui48@fa {
> >>                         reg = <0xfa 0x6>;
> >>                 };
> >>         };
> >> };
> >>
> >> &gmac {
> >>         // ...
> >>
> >>         nvmem-cells = <&eeprom0_eui48>;
> >>         nvmem-cell-names = "mac-address";
> >>
> >>         // ...
> >> };
> >>
> >> By adding this new compatible and changing the driver to not adjust the
> >> NVMEM cell offset for it but make it use the one provided in DT I was
> >> thinking this may scale for any future EEPROM storing MAC addresses at any
> >> offsets (because these would be provided via DT and not adjusted by driver).
> >>
> >> I the current at24 driver I haven't managed to find a compatible in the
> >> driver that mach the offset 0xfa (used to store MAC address on case of
> >> 24AA025E48).
> >>
> >>> The MAC area is accessible
> >>> on a different device address. The variants with serial and MAC areas
> >>> are meant to be instantiated as devices separate from the main
> >>> writeable area and you can then create an nvmem cell that will take up
> >>> the whole MAC.
> >>>
> >>> Or does your model keep the MAC in the same block that's used for
> >>> writing? In which case just using a regular compatible that matches
> >>> the size and creating the nvmem cell at the right offset should be
> >>> enough, right?
> >>
> >> Meaning describing the NVMEM cell in DT with an offset such that the driver
> >> adjustment +/- that offset to lead to the offset described in EEPROM datasheet?
> >>
> >>>
> >>> Am I missing something?
> >>
> >>
> >>
> >>>
> >>> Bartosz
> >>>
> >>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 13:24 [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64} Claudiu Beznea
2021-04-07 18:37 ` Bartosz Golaszewski
2021-04-12  7:42   ` Claudiu.Beznea
2021-04-12 18:29     ` Bartosz Golaszewski
2021-04-13 11:03       ` Claudiu.Beznea
2021-04-16 14:13         ` Bartosz Golaszewski

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git