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

From: Claudiu.Beznea
Date: Mon Apr 12 2021 - 03:42:38 EST

Next message: Pratik Rajesh Sampat: "[RFC v4 0/2] CPU-Idle latency selftest framework"
Previous message: Tero Kristo: "Re: [PATCH 0/2] fdt: translate address if #size-cells = <0>"
Next in thread: Bartosz Golaszewski: "Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Pratik Rajesh Sampat: "[RFC v4 0/2] CPU-Idle latency selftest framework"
Previous message: Tero Kristo: "Re: [PATCH 0/2] fdt: translate address if #size-cells = <0>"
Next in thread: Bartosz Golaszewski: "Re: [PATCH] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]