[U-Boot] [PATCH v3] Retrieve MAC address from EEPROM

[Olliver Schinagl]

On 10-11-16 13:26, Michal Simek wrote:
> On 10.11.2016 13:08, Olliver Schinagl wrote:
>> Hi Michal,
>>
>> On 10-11-16 12:37, Michal Simek wrote:
>>> On 8.11.2016 16:54, Olliver Schinagl wrote:
>>>> This patch-series introduces methods to retrieve the MAC address from an
>>>> onboard EEPROM using the read_rom_hwaddr hook.
>>>>
>>>> The reason we might want to read the MAC address from an EEPROM
>>>> instead of
>>>> storing the entire environment is mostly a size thing. Our default
>>>> environment
>>>> already is bigger then the EEPROM so it is understandable that
>>>> someone might
>>>> not give up the entire eeprom just for the u-boot environment.
>>>> Especially if
>>>> only board specific things might be stored in the eeprom (MAC,
>>>> serial, product
>>>> number etc). Additionally it is a board thing and a MAC address might be
>>>> programmed at the factory before ever seeing any software.
>>>>
>>>> The current idea of the eeprom layout, is to skip the first 8 bytes,
>>>> so that
>>>> other information can be stored there if needed, for example a header
>>>> with some
>>>> magic to identify the EEPROM. Or equivalent purposes.
>>>>
>>>> After those 8 bytes the MAC address follows the first macaddress. The
>>>> macaddress
>>>> is appended by a CRC8 byte and then padded to make for nice 8 bytes.
>>>> Following
>>>> the first macaddress one can store a second, or a third etc etc mac
>>>> addresses.
>>>>
>>>> The CRC8 is optional (via a define) but is strongly recommended to
>>>> have. It
>>>> helps preventing user error and more importantly, checks if the bytes
>>>> read are
>>>> actually a user inserted address. E.g. only writing 1 macaddress into
>>>> the eeprom
>>>> but trying to consume 2.
>>>>
>>>> Hans de Goede and I talked about retrieving the MAC from the EEPROM
>>>> for sunxi
>>>> based boards a while ago, but hopefully this patch makes possible to
>>>> have
>>>> something slightly more generic, even if only the configuration options.
>>>> Additionally the EEPROM layout could be recommended by u-boot as well.
>>>>
>>>> Since the Olimex OLinuXino sunxi boards all seem to have an eeprom, I
>>>> started
>>>> my work on one of these and tested the implementation with one of our
>>>> own real
>>>> MAC addresses.
>>>>
>>>> What still needs disussing I think, is the patches that remove the
>>>> 'setup_environment' function in board.c. I think we have put
>>>> functionality in
>>>> boards.c which does not belong. Injecting ethernet addresses into the
>>>> environment instead of using the net_op hooks as well as parsing the
>>>> fdt to get
>>>> overrides from. I think especially this last point should be done at
>>>> a higher
>>>> level, if possible at all.
>>>>
>>>> I explicitly did not use the wiser eth_validate_ethaddr_str(),
>>>> eth_parse_enetaddr() and the ARP_HLEN define as it was quite painful
>>>> (dependancies) to get these functions into the tools. I would suggest
>>>> to merge
>>>> as is, and if someone wants to improve these simple tools to use
>>>> these functions
>>>> to happily do so.
>>>>
>>>> These patches where tested on Olimex OLinuXino Lime1 (A10/A20), Lime2
>>>> (NAND
>>>> and eMMC) and A20-OLinuXino-MICRO-4G variants and have been in use
>>>> internally on our production systems since v2 of this patch set.
>>>>
>>>> As a recommendation, I would suggest for the zynq to adopt the config
>>>> defines,
>>>> as they are nice and generic and suggest to also implement the 8 byte
>>>> offset,
>>>> crc8 and pad bytes.
>>> Yes, Zynq and ZynqMP is using this feature. I am also aware about using
>>> qspi OTP region for storing information like this.
>> I saw, which triggered me here. What the Znyq currently does it uses its
>> own private CONFIG setting:
>>
>> +int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
>> +{
>> +#if defined(CONFIG_ZYNQ_GEM_EEPROM_ADDR) && \
>> +    defined(CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET) && \
>> +    defined(CONFIG_ZYNQ_EEPROM_BUS)
>> +       i2c_set_bus_num(CONFIG_ZYNQ_EEPROM_BUS);
>> +
>> +       if (eeprom_read(CONFIG_ZYNQ_GEM_EEPROM_ADDR,
>> +                       CONFIG_ZYNQ_GEM_I2C_MAC_OFFSET,
>> +                       ethaddr, 6))
>> +               printf("I2C EEPROM MAC address read failed\n");
>> +#endif
>> +
>> +       return 0;
>> +}
>>
>> which are ZNYQ specific. In my patchset I give them very generic names
>> as they can be used by anybody really.
>>
>> Once Maxime's patches have merged and stabilized, i'd even say to switch
>> over to the eeprom framework.
> Can you give me that link to these patches?
Well [0] is your own patch, so that is easy :) [1] is Maxime's work. But 
with your generic comment, this entire function probably can simply go 
then. The only thing I haven't figured out/thought through yet, if 
eeprom reading fails, we want to fall back to the old board specific 
method. But I think I know what might do there ...

Olliver

[0] http://git.denx.de/?p=u-boot.git;a=commit;h=6919b4bf363574
[1] https://www.mail-archive.com/u-boot@lists.denx.de/msg230179.html
>
> Thanks,
> Michal
>