From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olliver Schinagl Date: Thu, 10 Nov 2016 13:31:45 +0100 Subject: [U-Boot] [PATCH v3] Retrieve MAC address from EEPROM In-Reply-To: References: <[PATCH v2 0/5] Retrieve MAC address from EEPROM> <20161108155437.1085-1-oliver@schinagl.nl> <4247d228-92be-a919-bcc1-511009add21a@schinagl.nl> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 at lists.denx.de/msg230179.html > > Thanks, > Michal >