RE: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

From: Marek Vasut <marex@denx.de>
Sent: Thursday, October 17, 2024 4:01 PM
> On 10/17/24 1:09 PM, Christoph Niedermaier wrote:
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, October 16, 2024 2:16 PM
>>> On 10/16/24 1:57 PM, Christoph Niedermaier wrote:
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: Saturday, October 12, 2024 10:43 PM
>>>>> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>>>>>> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
>>>>>> from multiple sources in the following priority order:
>>>>>>
>>>>>> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
>>>>>> 2) SoC OTP fuses
>>>>>> 3) On-SoM EEPROM
>>>>>>
>>>>>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>>>>>> which contains additional write-lockable page, which can also be
>>>>>> populated with a structure containing ethernet MAC address.
>>>>>>
>>>>>> Add support for parsing the content of this new write-lockable page
>>>>>> and place it between 2) and 3) on the priority list. The new entry is
>>>>>> 2.5) On-SoM EEPROM write-lockable page
>>>>>>
>>>>>> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
>>>>>> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
>>>>>> first. If so, read the entire ID page out, validate it, and determine
>>>>>> whether EEPROM MAC address is populated in it in DH specific format. If
>>>>>> so, use the MAC address. There may be multiple EEPROMs with an ID page
>>>>>> on this platform, always use the first one.
>>>>>>
>>>>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>>>>>> ---
>>>>>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>> Cc: u-boot@dh-electronics.com
>>>>>> ---
>>>>>>     board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>>>>>>     board/dhelectronics/common/dh_common.h        |  23 ++++
>>>>>>     .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>>>>>>     3 files changed, 142 insertions(+)
>>>>>>
>>>>>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>>>>>> index 32c50b4f0f..8ea70fc984 100644
>>>>>> --- a/board/dhelectronics/common/dh_common.c
>>>>>> +++ b/board/dhelectronics/common/dh_common.c
>>>>>> @@ -7,9 +7,22 @@
>>>>>>     #include <dm.h>
>>>>>>     #include <i2c_eeprom.h>
>>>>>>     #include <net.h>
>>>>>> +#include <u-boot/crc.h>
>>>>>>
>>>>>>     #include "dh_common.h"
>>>>>>
>>>>>> +struct eeprom_id_page {
>>>>>> +     u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>>>>>> +     u8      version;        /* 0x10 -- Version 1.0 */
>>>>>> +     u8      data_crc16[2];  /* [1] is MSbyte */
>>>>>> +     u8      header_crc8;
>>>>>> +     u8      mac0[6];
>>>>>> +     u8      mac1[6];
>>>>>> +     u8      item_prefix;    /* H/F is coded in MSbits, Vendor coding starts at LSbits */
>>>>>> +     u8      item_num[3];    /* [2] is MSbyte */
>>>>>> +     u8      serial[9];      /* [8] is MSbyte */
>>>>>> +} __packed;
>>>>>
>>>>> Is __packed needed ?
>>>>
>>>> I want to avoid padding in any case.
>>> Every single field is u8, do you observe any padding ?
>>
>> In this case, I don't expect padding, but if the struct is extended,
>> padding could be possible.
> 
> Please add the padding only once it is really required, ideally the
> design of the structure or its extension(s) would be able to avoid such
> necessity altogether.
> 
>> This struct is for an EEPROM and therefore
>> it should be defined with no padding. I don't want to give the compile
>> the change to add padding if something changed in the struct.
>> What are the disadvantages of keeping __packed here?
> If it is unnecessary, then please remove it, there is no good
> justification for packing a structure which is naturally aligned already.

OK. I will remove it in V2.

Regards
Christoph