On 10.11.2016 13:43, Olliver Schinagl wrote: > On 10-11-16 13:37, Michal Simek wrote: >> 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. > In the case of sunxi, we generate a MAC address based off the CPU serial > number and device ID, which is more predictable and doesn't change > compared to the random MAC. The random MAC would then in turn be a > fallback if we still have a 00:00:00:00:00 ethernet address. > > So 3 steps, check EEPROM (i2c only for now, SPI etc can be added later > with the eeprom uclass) > call board specific hook > if empty, generate random (if configured)
yep - not a problem with it. Thanks, Michal _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot