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

[Michal Simek]

On 10.11.2016 13:31, Olliver Schinagl wrote:
> 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 ...

Joe recently applied one our patch
http://lists.denx.de/pipermail/u-boot/2016-November/271662.html
which in case of NET_RAMDOM_ETHADDR generates random address.
I don't have in my mind exact calling sequence but I expect when eeprom
read failed then random eth is generated if selected or warning, etc.

Thanks,
Michal