[U-Boot] [PATCH v3] Retrieve MAC address from EEPROM
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.
P.S. I have gotten in depth/familiar with PATMAN yet :)
======= Changes since v2: * Drop the id byte altogether and just mark it as reserved. The 'index' can be used to indicate the interface instead * Addopt the read_rom_hwaddr hooks * Renamed crc8 tool to gen_ethaddr_crc * Improved the layout EEPROM example * Made a function out of the hwaddress writing function in sunxi_emac so it can be re-used as the write_hwaddr net_ops hook. * No longer handle fdt parameters in board.c
Changes since v1: * Do not CRC the id byte, move it just after the crc byte. One of the reasons I decided to move it after the crc8 was mostly due to mass generation of MAC + CRC combo's where the ID is still unknown. Also not crc-ing the ID means that it is much easier for a user to change it (via the u-boot i2c cmd line or from within linux) without having to worry about the crc. * Add a generator to convert a MAC address from the input to a MAC + CRC8 on the output.
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- drivers/net/designware.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 9e6d726..aa87f30 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) return 0; }
+__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{ + return -ENOSYS; +} + +static int designware_eth_read_rom_hwaddr(struct udevice *dev) +{ + int retval; + struct eth_pdata *pdata = dev_get_platdata(dev); + + retval = dw_board_read_rom_hwaddr(pdata->enetaddr); + if (retval == -ENOSYS) + return 0; + + return retval; +} + static void dw_adjust_link(struct eth_mac_regs *mac_p, struct phy_device *phydev) { @@ -685,6 +702,7 @@ static const struct eth_ops designware_eth_ops = { .free_pkt = designware_eth_free_pkt, .stop = designware_eth_stop, .write_hwaddr = designware_eth_write_hwaddr, + .read_rom_hwaddr = designware_eth_read_rom_hwaddr, };
static int designware_eth_ofdata_to_platdata(struct udevice *dev)
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Hi Oliver,
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
drivers/net/designware.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 9e6d726..aa87f30 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) return 0; }
+__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{
return -ENOSYS;+}
Instead of a weak function I think this should use driver model, with a driver supplied by the board to read this value. It should be possible to supply the 'hardware-address reading' device to any Ethernet driver, not just dwmmc.
+static int designware_eth_read_rom_hwaddr(struct udevice *dev) +{
int retval;struct eth_pdata *pdata = dev_get_platdata(dev);retval = dw_board_read_rom_hwaddr(pdata->enetaddr);if (retval == -ENOSYS)return 0;return retval;+}
static void dw_adjust_link(struct eth_mac_regs *mac_p, struct phy_device *phydev) { @@ -685,6 +702,7 @@ static const struct eth_ops designware_eth_ops = { .free_pkt = designware_eth_free_pkt, .stop = designware_eth_stop, .write_hwaddr = designware_eth_write_hwaddr,
.read_rom_hwaddr = designware_eth_read_rom_hwaddr,};
static int designware_eth_ofdata_to_platdata(struct udevice *dev)
2.10.2
Regards, Simon
Hi Simon,
On Thu, Nov 17, 2016 at 7:13 PM, Simon Glass sjg@chromium.org wrote:
Hi Oliver,
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
drivers/net/designware.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 9e6d726..aa87f30 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) return 0; }
+__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{
return -ENOSYS;+}
Instead of a weak function I think this should use driver model, with a driver supplied by the board to read this value. It should be possible to supply the 'hardware-address reading' device to any Ethernet driver, not just dwmmc.
How do we reconcile something like that with the concern of using the device tree for boards using only Linux bindings, and sharing the device tree with Linux? Linux probably doesn't care about this and so won't have a binding for defining this relationship. This is a fairly generic question. Where have we landed on this?
Thanks, -Joe
Hi Joe,
On 29 November 2016 at 14:24, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Thu, Nov 17, 2016 at 7:13 PM, Simon Glass sjg@chromium.org wrote:
Hi Oliver,
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
drivers/net/designware.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 9e6d726..aa87f30 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) return 0; }
+__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{
return -ENOSYS;+}
Instead of a weak function I think this should use driver model, with a driver supplied by the board to read this value. It should be possible to supply the 'hardware-address reading' device to any Ethernet driver, not just dwmmc.
How do we reconcile something like that with the concern of using the device tree for boards using only Linux bindings, and sharing the device tree with Linux? Linux probably doesn't care about this and so won't have a binding for defining this relationship. This is a fairly generic question. Where have we landed on this?
So far I have not seen something that cannot be solved either as I suggest above or with platform data.
Often you can pass platform data to the driver - e.g. see the end of board_init() in gurnard.c which tells the video driver which LCD to use.
Is there another case? I certainly have ideas but don't want to create something without a solid case.
Regards, Simon
On Tue, Nov 29, 2016 at 3:40 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 29 November 2016 at 14:24, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Thu, Nov 17, 2016 at 7:13 PM, Simon Glass sjg@chromium.org wrote:
Hi Oliver,
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
drivers/net/designware.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 9e6d726..aa87f30 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) return 0; }
+__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{
return -ENOSYS;+}
Instead of a weak function I think this should use driver model, with a driver supplied by the board to read this value. It should be possible to supply the 'hardware-address reading' device to any Ethernet driver, not just dwmmc.
How do we reconcile something like that with the concern of using the device tree for boards using only Linux bindings, and sharing the device tree with Linux? Linux probably doesn't care about this and so won't have a binding for defining this relationship. This is a fairly generic question. Where have we landed on this?
So far I have not seen something that cannot be solved either as I suggest above or with platform data.
Often you can pass platform data to the driver - e.g. see the end of board_init() in gurnard.c which tells the video driver which LCD to use.
Is there another case? I certainly have ideas but don't want to create something without a solid case.
I hadn't understood what pattern you were referring to when you said "with a driver supplied by the board to read this value." I just reviewed the rockchip series that you referred to and I think that pattern works pretty cleanly.
Thanks, -Joe
Currently the mac address is programmed directly in _sunxi_emac_eth_init making it a one time inflexible operation. By moving it into a separate function, we can now use this more flexibly.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- drivers/net/sunxi_emac.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/net/sunxi_emac.c b/drivers/net/sunxi_emac.c index 11cd0ea..99339db 100644 --- a/drivers/net/sunxi_emac.c +++ b/drivers/net/sunxi_emac.c @@ -327,6 +327,20 @@ static void emac_reset(struct emac_eth_dev *priv) udelay(200); }
+static int _sunxi_write_hwaddr(struct emac_eth_dev *priv, u8 *enetaddr) +{ + struct emac_regs *regs = priv->regs; + u32 enetaddr_lo, enetaddr_hi; + + enetaddr_lo = enetaddr[2] | (enetaddr[1] << 8) | (enetaddr[0] << 16); + enetaddr_hi = enetaddr[5] | (enetaddr[4] << 8) | (enetaddr[3] << 16); + + writel(enetaddr_hi, ®s->mac_a1); + writel(enetaddr_lo, ®s->mac_a0); + + return 0; +} + static int _sunxi_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr) { struct emac_regs *regs = priv->regs; @@ -350,10 +364,7 @@ static int _sunxi_emac_eth_init(struct emac_eth_dev *priv, u8 *enetaddr) /* Set up EMAC */ emac_setup(priv);
- writel(enetaddr[0] << 16 | enetaddr[1] << 8 | enetaddr[2], - ®s->mac_a1); - writel(enetaddr[3] << 16 | enetaddr[4] << 8 | enetaddr[5], - ®s->mac_a0); + _sunxi_write_hwaddr(priv, enetaddr);
mdelay(1);
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently the mac address is programmed directly in _sunxi_emac_eth_init making it a one time inflexible operation. By moving it into a separate function, we can now use this more flexibly.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Expose enetaddr writing via net_ops so that it can be hooked into.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- drivers/net/sunxi_emac.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/net/sunxi_emac.c b/drivers/net/sunxi_emac.c index 99339db..dcf832e 100644 --- a/drivers/net/sunxi_emac.c +++ b/drivers/net/sunxi_emac.c @@ -554,6 +554,14 @@ static void sunxi_emac_eth_stop(struct udevice *dev) /* Nothing to do here */ }
+static int sunxi_emac_eth_write_hwaddr(struct udevice *dev) +{ + struct eth_pdata *pdata = dev_get_platdata(dev); + struct emac_eth_dev *priv = dev_get_priv(dev); + + return _sunxi_write_hwaddr(priv, pdata->enetaddr); +} + static int sunxi_emac_eth_probe(struct udevice *dev) { struct eth_pdata *pdata = dev_get_platdata(dev); @@ -570,6 +578,7 @@ static const struct eth_ops sunxi_emac_eth_ops = { .send = sunxi_emac_eth_send, .recv = sunxi_emac_eth_recv, .stop = sunxi_emac_eth_stop, + .write_hwaddr = sunxi_emac_eth_write_hwaddr, };
static int sunxi_emac_eth_ofdata_to_platdata(struct udevice *dev)
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Expose enetaddr writing via net_ops so that it can be hooked into.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- drivers/net/sunxi_emac.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/net/sunxi_emac.c b/drivers/net/sunxi_emac.c index dcf832e..6a6611e 100644 --- a/drivers/net/sunxi_emac.c +++ b/drivers/net/sunxi_emac.c @@ -562,6 +562,23 @@ static int sunxi_emac_eth_write_hwaddr(struct udevice *dev) return _sunxi_write_hwaddr(priv, pdata->enetaddr); }
+__weak int sunxi_board_read_rom_hwaddr(unsigned char *enetaddr) +{ + return -ENOSYS; +} + +static int sunxi_emac_eth_read_rom_hwaddr(struct udevice *dev) +{ + int retval; + struct eth_pdata *pdata = dev_get_platdata(dev); + + retval = sunxi_board_read_rom_hwaddr(pdata->enetaddr); + if (retval == -ENOSYS) + return 0; + + return retval; +} + static int sunxi_emac_eth_probe(struct udevice *dev) { struct eth_pdata *pdata = dev_get_platdata(dev); @@ -579,6 +596,7 @@ static const struct eth_ops sunxi_emac_eth_ops = { .recv = sunxi_emac_eth_recv, .stop = sunxi_emac_eth_stop, .write_hwaddr = sunxi_emac_eth_write_hwaddr, + .read_rom_hwaddr = sunxi_emac_eth_read_rom_hwaddr, };
static int sunxi_emac_eth_ofdata_to_platdata(struct udevice *dev)
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
This patch allows Kconfig to enable and set parameters to make it possible to read the MAC address from an EEPROM. This patch only sets up some environment variables, it is up to the specific boards to actually use these defines.
Besides the various tuneables as to how to access the eeprom (bus, address, addressing mode/length, 2 configurable that are EEPROM generic (e.g. SPI or some other form of access) which are:
NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of the MAC address is. The default is 8 allowing for 8 bytes before the MAC for other purposes (header MAGIC for example).
NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT checksum that should be verified.
Currently only I2C eeproms have been tested and thus only those options are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be just as created.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com --- doc/README.enetaddr | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++ net/Kconfig | 58 ++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 50e4899..0bf291d 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -47,6 +47,91 @@ Correct flow of setting up the MAC address (summarized): Previous behavior had the MAC address always being programmed into hardware in the device's init() function.
+-------- + EEPROM +-------- + +When there is an EEPROM available on a board, but the EEPROM is not large enough +to store the whole environment, it may be desired to store a MAC address in the +onboard EEPROM. Using CONFIG_NET_ETHADDR_EEPROM enables this feature. Depending +on the board, the EEPROM may be connected on various methods, but currently, +only the I2C bus is available via CONFIG_NET_ETHADDR_EEPROM_I2C. + +The following config options are available, +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom is present. +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM, which +defaults to the very common 0x50. Small size EEPROM's generally use single byte +addressing but larger EEPROM's may use double byte addressing, which can be +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN. + +Within the EEPROM, the MAC address can be stored on any arbitrary offset, +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default however, allowing +the first 8 bytes to be used for an optional data, for example a configuration +struct where the mac address is part of. + +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous ARP_HLEN +bytes. Whether to check this CRC8 or not is dependent on +CONFIG_NET_ETHADDR_EEPROM_CRC8. + +To keep things nicely aligned, a final 'reserved' byte is added to the mac +address + crc8 combo. + +A board may want to store more information in its eeprom, using the following +example layout, this can be achieved. + +struct mac_addr { + uint8_t mac[ARP_HLEN]; + uint8_t crc8; + uint8_t reserved; +}; + +struct config_eeprom { + uint32_t magic; + uint8_t version; + uint8_t reserved[2]; + uint8_t mac_cnt; + struct mac_addr[mac_cnt]; +}; + +Filling this in: +struct config_eeprom eeprom = { + .magic = { 'M', 'g', 'i', 'c' }, + .reserved = { 0x00, 0x00 }, + .mac_cnt = 2, + .mac_addr = { + { + .mac = { + 0x01, 0x23, 0x45, + 0x67, 0x89, 0xab, + }, + .crc8 = 0xbe, + .reserved = 0x00, + }, { + .mac = { + 0xba, 0x98, 0x76, + 0x54, 0x32, 0x10, + }, + .crc8 = 0x82, + .reserved = 0x00, + }, + }, +}; + +The eeprom content would look like this. + +00000000 4d 67 69 63 01 00 00 02 01 23 45 67 89 ab be 00 |Mgic.....#Eg....| +00000010 ba 98 76 54 32 10 82 00 |..vT2...| + +Alternativly the i2c-tools can be used as well. + +i2cset I2CBUS 0x50 0x08 0x01 +i2cset I2CBUS 0x50 0x09 0x23 +i2cset I2CBUS 0x50 0x0a 0x45 +i2cset I2CBUS 0x50 0x0b 0x67 +i2cset I2CBUS 0x50 0x0c 0x89 +i2cset I2CBUS 0x50 0x0d 0xab +i2cset I2CBUS 0x50 0x0e 0xbe + ------- Usage ------- diff --git a/net/Kconfig b/net/Kconfig index 414c549..f7ef2b7 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -7,6 +7,64 @@ menuconfig NET
if NET
+config NET_ETHADDR_EEPROM + bool "Get ethaddr from eeprom" + help + Selecting this will try to get the Ethernet address from an onboard + EEPROM and set into the environment if and only if the environment + does currently not already hold a MAC address. For more information + see doc/README.enetaddr. + +config NET_ETHADDR_EEPROM_I2C + depends on NET_ETHADDR_EEPROM + bool "EEPROM on I2C bus" + help + This switch enables checks for an EEPROM on the I2C bus. Naturally + this will only work if there is an actual EEPROM connected on the + I2C bus and the bus and device are properly configured via the + options below. + +config NET_ETHADDR_EEPROM_I2C_BUS + depends on NET_ETHADDR_EEPROM_I2C + int "I2C bus" + default 0 + help + Select the bus on which the EEPROM is present, defaults to bus 0. + +config NET_ETHADDR_EEPROM_I2C_ADDR + depends on NET_ETHADDR_EEPROM_I2C + hex "eeprom address" + default 0x50 + help + Select the address of the eeprom, defaults to address 0x50. + +config NET_ETHADDR_EEPROM_I2C_ADDRLEN + depends on NET_ETHADDR_EEPROM_I2C + int "eeprom address length" + default 1 + help + Number of bytes to be used for the I2C address length. Typically 1, + 2 for large memories, 0 for register type devices with only one + register. + +config NET_ETHADDR_EEPROM_OFFSET + depends on NET_ETHADDR_EEPROM + int "EEPROM offset" + default 8 + help + Select the byte offset of the MAC address within the page, + defaults to byte 8. + +config NET_ETHADDR_EEPROM_CRC8 + depends on NET_ETHADDR_EEPROM + bool "Check CRC8 of MAC" + default y + help + Optionally, it is possible to run a CRC-8-CCITT check on the MAC + address. To do so, the MAC address is stored with a CRC8 byte append. + This option enables the CRC check of the MAC address against the CRC + byte. + config NET_RANDOM_ETHADDR bool "Random ethaddr if unset" select LIB_RAND
With the newly introduced hooks, we can now set the MAC address at the lowest level properly. The user is still free to override it via a u-boot environment variable.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- arch/arm/include/asm/arch-sunxi/sys_proto.h | 8 +++ board/sunxi/board.c | 103 +++++++++++++++++++--------- 2 files changed, 80 insertions(+), 31 deletions(-)
diff --git a/arch/arm/include/asm/arch-sunxi/sys_proto.h b/arch/arm/include/asm/arch-sunxi/sys_proto.h index a373319..5b24b86 100644 --- a/arch/arm/include/asm/arch-sunxi/sys_proto.h +++ b/arch/arm/include/asm/arch-sunxi/sys_proto.h @@ -30,4 +30,12 @@ void eth_init_board(void); static inline void eth_init_board(void) {} #endif
+#if CONFIG_SUNXI_EMAC +int sunxi_board_read_rom_hwaddr(unsigned char *enetaddr); +#endif + +#if defined(CONFIG_SUNXI_GMAC) || defined(CONFIG_ETH_DESIGNWARE) +int dw_board_read_rom_hwaddr(unsigned char *enetaddr); +#endif + #endif diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 5365638..9b56a89 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -21,6 +21,7 @@ #include <asm/arch/gpio.h> #include <asm/arch/mmc.h> #include <asm/arch/spl.h> +#include <asm/arch/sys_proto.h> #include <asm/arch/usb_phy.h> #ifndef CONFIG_ARM64 #include <asm/armv7.h> @@ -584,6 +585,76 @@ void get_board_serial(struct tag_serialnr *serialnr) } #endif
+static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{ + uint8_t mac_addr[ARP_HLEN] = { 0x00 }; + unsigned int sid[4]; + int ret; + + ret = sunxi_get_sid(sid); + if (ret == 0 && sid[0] != 0) { + /* + * The single words 1 - 3 of the SID have quite a few bits + * which are the same on many models, so we take a crc32 + * of all 3 words, to get a more unique value. + * + * Note we only do this on newer SoCs as we cannot change + * the algorithm on older SoCs since those have been using + * fixed mac-addresses based on only using word 3 for a + * long time and changing a fixed mac-address with an + * u-boot update is not good. + */ +#if !defined(CONFIG_MACH_SUN4I) && !defined(CONFIG_MACH_SUN5I) && \ + !defined(CONFIG_MACH_SUN6I) && !defined(CONFIG_MACH_SUN7I) && \ + !defined(CONFIG_MACH_SUN8I_A23) && !defined(CONFIG_MACH_SUN8I_A33) + sid[3] = crc32(0, (unsigned char *)&sid[1], 12); +#endif + + /* Ensure the NIC specific bytes of the mac are not all 0 */ + if ((sid[3] & 0xffffff) == 0) + sid[3] |= 0x800000; + + /* Non OUI / registered MAC address */ + mac_addr[0] = (cnt << 4) | 0x02; + mac_addr[1] = (sid[0] >> 0) & 0xff; + mac_addr[2] = (sid[3] >> 24) & 0xff; + mac_addr[3] = (sid[3] >> 16) & 0xff; + mac_addr[4] = (sid[3] >> 8) & 0xff; + mac_addr[5] = (sid[3] >> 0) & 0xff; + } + + memcpy(enetaddr, mac_addr, ARP_HLEN); +} + +static int sunxi_read_rom_hwaddr(unsigned char *enetaddr) +{ + static unsigned int cnt; + + _sunxi_gen_sid_hwaddr(enetaddr, cnt); + + /* First call, first stored MAC address, increase for next call */ + cnt++; + + if (is_zero_ethaddr(enetaddr)) + return -ENOSYS; + + printf("MAC address: %02X:%02X:%02X:%02X:%02X:%02X\n", + enetaddr[0], enetaddr[1], enetaddr[2], + enetaddr[3], enetaddr[4], enetaddr[5]); + + return 0; +} + +int sunxi_board_read_rom_hwaddr(unsigned char *enetaddr) +{ + return sunxi_read_rom_hwaddr(enetaddr); +} + +int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{ + return sunxi_read_rom_hwaddr(enetaddr); +} + /* * Check the SPL header for the "sunxi" variant. If found: parse values * that might have been passed by the loader ("fel" utility), and update @@ -625,9 +696,7 @@ static void setup_environment(const void *fdt) { char serial_string[17] = { 0 }; unsigned int sid[4]; - uint8_t mac_addr[6]; - char ethaddr[16]; - int i, ret; + int ret;
ret = sunxi_get_sid(sid); if (ret == 0 && sid[0] != 0) { @@ -648,34 +717,6 @@ static void setup_environment(const void *fdt) sid[3] = crc32(0, (unsigned char *)&sid[1], 12); #endif
- /* Ensure the NIC specific bytes of the mac are not all 0 */ - if ((sid[3] & 0xffffff) == 0) - sid[3] |= 0x800000; - - for (i = 0; i < 4; i++) { - sprintf(ethaddr, "ethernet%d", i); - if (!fdt_get_alias(fdt, ethaddr)) - continue; - - if (i == 0) - strcpy(ethaddr, "ethaddr"); - else - sprintf(ethaddr, "eth%daddr", i); - - if (getenv(ethaddr)) - continue; - - /* Non OUI / registered MAC address */ - mac_addr[0] = (i << 4) | 0x02; - mac_addr[1] = (sid[0] >> 0) & 0xff; - mac_addr[2] = (sid[3] >> 24) & 0xff; - mac_addr[3] = (sid[3] >> 16) & 0xff; - mac_addr[4] = (sid[3] >> 8) & 0xff; - mac_addr[5] = (sid[3] >> 0) & 0xff; - - eth_setenv_enetaddr(ethaddr, mac_addr); - } - if (!getenv("serial#")) { snprintf(serial_string, sizeof(serial_string), "%08x%08x", sid[0], sid[3]);
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
With the newly introduced hooks, we can now set the MAC address at the lowest level properly. The user is still free to override it via a u-boot environment variable.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- board/sunxi/board.c | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 9b56a89..71124f4 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -688,15 +688,19 @@ static void parse_spl_header(const uint32_t spl_addr) setenv_hex("fel_scriptaddr", spl->fel_script_address); }
-/* - * Note this function gets called multiple times. - * It must not make any changes to env variables which already exist. - */ -static void setup_environment(const void *fdt) +int misc_init_r(void) { char serial_string[17] = { 0 }; unsigned int sid[4]; - int ret; + __maybe_unused int ret; + + setenv("fel_booted", NULL); + setenv("fel_scriptaddr", NULL); + /* determine if we are running in FEL mode */ + if (!is_boot0_magic(SPL_ADDR + 4)) { /* eGON.BT0 */ + setenv("fel_booted", "1"); + parse_spl_header(SPL_ADDR); + }
ret = sunxi_get_sid(sid); if (ret == 0 && sid[0] != 0) { @@ -717,28 +721,11 @@ static void setup_environment(const void *fdt) sid[3] = crc32(0, (unsigned char *)&sid[1], 12); #endif
- if (!getenv("serial#")) { - snprintf(serial_string, sizeof(serial_string), - "%08x%08x", sid[0], sid[3]); + snprintf(serial_string, sizeof(serial_string), + "%08x%08x", sid[0], sid[3]);
- setenv("serial#", serial_string); - } + setenv("serial#", serial_string); } -} - -int misc_init_r(void) -{ - __maybe_unused int ret; - - setenv("fel_booted", NULL); - setenv("fel_scriptaddr", NULL); - /* determine if we are running in FEL mode */ - if (!is_boot0_magic(SPL_ADDR + 4)) { /* eGON.BT0 */ - setenv("fel_booted", "1"); - parse_spl_header(SPL_ADDR); - } - - setup_environment(gd->fdt_blob);
#ifndef CONFIG_MACH_SUN9I ret = sunxi_usb_phy_probe(); @@ -754,12 +741,6 @@ int ft_board_setup(void *blob, bd_t *bd) { int __maybe_unused r;
- /* - * Call setup_environment again in case the boot fdt has - * ethernet aliases the u-boot copy does not have. - */ - setup_environment(blob); - #ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); if (r)
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
1) It does not do what its commit message says, it only removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
2) "Currently we inject 5 ethernet addresses into the environment", this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
3) The second attempt at setting ethernet addresses in the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
4) We cannot solely rely on the ethernet driver to set mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
5) AFAIK the dt code for passing mac-addresses to the kernel relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
Regards,
Hans
Hey Hans,
I was hopeing and expecting this :)
As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
On 15-11-16 10:26, Hans de Goede wrote:
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
- It does not do what its commit message says, it only
removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
- "Currently we inject 5 ethernet addresses into the environment",
this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?
- The second attempt at setting ethernet addresses in
the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
I agree with you here, but I still don't think this should be board specific
- We cannot solely rely on the ethernet driver to set
mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific
- AFAIK the dt code for passing mac-addresses to the kernel
relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
The new patch series does that, as the net core layer does that.
What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
eth_read_eeprom_hwaddr(dev); first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
// <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env?
} else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).
} else if (is_zero_ethaddr(pdata->enetaddr)) { #ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it.
So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.
I'll post my split-up new series for review after testing it, so we can discuss it in more detail?
Olliver
Regards,
Hans
Hi,
On 15-11-16 11:17, Olliver Schinagl wrote:
Hey Hans,
I was hopeing and expecting this :)
As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
On 15-11-16 10:26, Hans de Goede wrote:
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
- It does not do what its commit message says, it only
removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
- "Currently we inject 5 ethernet addresses into the environment",
this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?
We want to put mac addresses into the fdt, and this is done at a higher level, by existing dt code, which looks at ethernet aliases in dt and then for any which are present, checks the corresponding ethaddr env setting and if set, injects that mac address into the dt-node the alias points to.
What is sunxi specific is setting the environment variables based on the SID. The sunxi specific code also checks the aliases, exactly to avoid the "inject 5 ethernet addresses" thing you are describing, as we don't want to inject ethernet addresses for non existent NICs as that may confuse the user.
- The second attempt at setting ethernet addresses in
the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
I agree with you here, but I still don't think this should be board specific
Instead of doing this through the environment I guess you could have the u-boot dt code which is injecting the MACs into the dt call some board specific callback when there is no MAC set in the environment, and implement a weak stub for this. Then all the sunxi environment mangling code can go away, and sunxi can simply: 1) Try eeprom if present 2) Do the current SID based thing
- We cannot solely rely on the ethernet driver to set
mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific
The problem is that u-boot may not have a driver for one of the NICs at all, so no place to call the rom hook at all.
Anyways I believe this is solved by my suggestion for making the u-boot dt code which injects the MAC call a board specific callback when no ethaddr is set in the env.
- AFAIK the dt code for passing mac-addresses to the kernel
relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
The new patch series does that, as the net core layer does that.
What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
eth_read_eeprom_hwaddr(dev);first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);// <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env?
} else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).
Ok, good I just wanted to make sure that that would still happen.
} else if (is_zero_ethaddr(pdata->enetaddr)) {#ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it.
So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.
I think you're thinking too much about the case where u-boot has an actual driver for the NIC (and that driver gets initialized, what if it gets skipped to speed up the boot?) and not enough about the case where there is no driver but we still want to use u-boot's board specific MAC generation code to provide a fixed MAC address to the kernel.
Regards,
Hans
Hey,
On 15-11-16 11:27, Hans de Goede wrote:
Hi,
On 15-11-16 11:17, Olliver Schinagl wrote:
Hey Hans,
I was hopeing and expecting this :)
As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
On 15-11-16 10:26, Hans de Goede wrote:
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
- It does not do what its commit message says, it only
removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
- "Currently we inject 5 ethernet addresses into the environment",
this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?
We want to put mac addresses into the fdt, and this is done at a higher level, by existing dt code, which looks at ethernet aliases in dt and then for any which are present, checks the corresponding ethaddr env setting and if set, injects that mac address into the dt-node the alias points to.
What is sunxi specific is setting the environment variables based on the SID. The sunxi specific code also checks the aliases, exactly to avoid the "inject 5 ethernet addresses" thing you are describing, as we don't want to inject ethernet addresses for non existent NICs as that may confuse the user.
- The second attempt at setting ethernet addresses in
the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
I agree with you here, but I still don't think this should be board specific
Instead of doing this through the environment I guess you could have the u-boot dt code which is injecting the MACs into the dt call some board specific callback when there is no MAC set in the environment, and implement a weak stub for this. Then all the sunxi environment mangling code can go away, and sunxi can simply:
- Try eeprom if present
- Do the current SID based thing
- We cannot solely rely on the ethernet driver to set
mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific
The problem is that u-boot may not have a driver for one of the NICs at all, so no place to call the rom hook at all.
Anyways I believe this is solved by my suggestion for making the u-boot dt code which injects the MAC call a board specific callback when no ethaddr is set in the env.
- AFAIK the dt code for passing mac-addresses to the kernel
relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
The new patch series does that, as the net core layer does that.
What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
eth_read_eeprom_hwaddr(dev);first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);// <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env?
} else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).
Ok, good I just wanted to make sure that that would still happen.
} else if (is_zero_ethaddr(pdata->enetaddr)) {#ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it.
So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.
I think you're thinking too much about the case where u-boot has an actual driver for the NIC (and that driver gets initialized, what if it gets skipped to speed up the boot?) and not enough about the case where there is no driver but we still want to use u-boot's board specific MAC generation code to provide a fixed MAC address to the kernel.
In my other e-mail contradiciting myself I just found this somewhat out, and that indeed is another usecase worth thinking about yeah. So I'll need to re-think that part too.
And then thinks come to mind 'if there are 5 address in the eeprom, but only 2 drivers, do the drivers get the first two?' ... I guess there needs to be a general agreement on strange cases as such. How are non-driver devices handled. The dts obviously is one method, but I'm sure board manufactures will hate us for forcing board specific dts. They want to just produce boards en-masse and may be kind enough to supply eeprom or MAC-prom's with fixed mac addresses stored there.
I think this is an architectural based decision which deserves its own thread?
Olliver
Regards,
Hans
Hey Hans,
I guess I have to contradict something here ...
On 15-11-16 11:17, Olliver Schinagl wrote:
Hey Hans,
I was hopeing and expecting this :)
As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
On 15-11-16 10:26, Hans de Goede wrote:
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
- It does not do what its commit message says, it only
removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
- "Currently we inject 5 ethernet addresses into the environment",
this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?
- The second attempt at setting ethernet addresses in
the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
I agree with you here, but I still don't think this should be board specific
- We cannot solely rely on the ethernet driver to set
mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific
Of course I didn't realize that the rtl8189etv does not have a u-boot driver, and thus does not get to call its hook and thus nothing sunxi specific will ever be invoked.
So I guess in the case of the rtl8189 we have to figure out something (probably near the same as you did) to make it work.
It does feel somewhat nasty/hackish of course. I would expect that the linux driver sorts this out for itself and not simply assume u-boot supplies this info on non-existing hardware (to u-boot).
I need some time to think this over, so I'm hoping smarter people then me come with great suggestions here :)
But for now I'm leaning to think that, the rtl8189 is special.
So is this a board specific hack, or a fdt net specific hack. I does feel like the fdt bit I described earlier injects extra mac addresses into the environment for these unknown hardware pieces ... But that will need to come from board specific pieces, as the net stack never gets invoked for these ...
I'll stop thinking outloud now and get back to work ;)
olliver
- AFAIK the dt code for passing mac-addresses to the kernel
relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
The new patch series does that, as the net core layer does that.
What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
eth_read_eeprom_hwaddr(dev);first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);// <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env?
} else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).
} else if (is_zero_ethaddr(pdata->enetaddr)) {#ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it.
So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.
I'll post my split-up new series for review after testing it, so we can discuss it in more detail?
Olliver
Regards,
Hans
Hi,
On 15-11-16 11:29, Olliver Schinagl wrote:
Hey Hans,
I guess I have to contradict something here ...
On 15-11-16 11:17, Olliver Schinagl wrote:
Hey Hans,
I was hopeing and expecting this :)
As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
On 15-11-16 10:26, Hans de Goede wrote:
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
- It does not do what its commit message says, it only
removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
- "Currently we inject 5 ethernet addresses into the environment",
this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?
- The second attempt at setting ethernet addresses in
the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
I agree with you here, but I still don't think this should be board specific
- We cannot solely rely on the ethernet driver to set
mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific
Of course I didn't realize that the rtl8189etv does not have a u-boot driver, and thus does not get to call its hook and thus nothing sunxi specific will ever be invoked.
So I guess in the case of the rtl8189 we have to figure out something (probably near the same as you did) to make it work.
It does feel somewhat nasty/hackish of course. I would expect that the linux driver sorts this out for itself and not simply assume u-boot supplies this info on non-existing hardware (to u-boot).
We've the same with e.g. the wobo-i5 which has its emac routed to an other set of pins then which are usually used which u-boot does not support. Yet the kernel emac driver relies on u-boot to set a MAC, or it will fall back to a random-MAC (which is undesirable).
Likewise if someone configures u-boot without network support to make it boot faster, we get the same problem with the emac / gmac driver.
The logic here really goes like this:
1) When u-boot does have network support, and picks a MAC-address that MAC-address should be propagated to the kernel so that it uses the same MAC (this is esp. important with switches which allow only 1 MAC per port as a security setting).
2) 1. means that u-boot has some algorithm to pick a fixed MAC in a SoC specific manner. Since we do not want booting with an u-boot with networking enabled resulting in a different fixed MAC then booting on a u-boot without networking support, this means that this algorithm should be used even when networking support in u-boot is disabled (e.g. the wobo-i5 case).
3) Given 2. it makes sense to simply have u-boot generate MACs for all NICs in the system.
I need some time to think this over, so I'm hoping smarter people then me come with great suggestions here :)
I still think that my suggestion for having fdt_fixup_ethernet() from common/fdt_support.c call a board specific function instead of the "continue" here:
tmp = getenv(mac); if (!tmp) continue;
for (j = 0; j < 6; j++) { mac_addr[j] = tmp ? simple_strtoul(tmp, &end, 16) : 0; if (tmp) tmp = (*end) ? end + 1 : end; }
E.g:
tmp = getenv(mac); if (!tmp) { if (board_get_ethaddr(i, mac_addr) != 0) continue; } else { for (j = 0; j < 6; j++) { mac_addr[j] = tmp ? simple_strtoul(tmp, &end, 16) : 0; if (tmp) tmp = (*end) ? end + 1 : end; } }
Would be a good idea, with a weak default for
int board_get_ethaddr(int index, unsigned char *mac_addr);
Which simply returns -EINVAL;
This will neatly solve all the problems discussed, you could probably even use the same board_get_ethaddr() in both the generic ethernet setup code you've been working in, as well as in fdt_fixup_ethernet()
Regards,
Hans
But for now I'm leaning to think that, the rtl8189 is special.
So is this a board specific hack, or a fdt net specific hack. I does feel like the fdt bit I described earlier injects extra mac addresses into the environment for these unknown hardware pieces ... But that will need to come from board specific pieces, as the net stack never gets invoked for these ...
I'll stop thinking outloud now and get back to work ;)
olliver
- AFAIK the dt code for passing mac-addresses to the kernel
relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
The new patch series does that, as the net core layer does that.
What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
eth_read_eeprom_hwaddr(dev);first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);// <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env?
} else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).
} else if (is_zero_ethaddr(pdata->enetaddr)) {#ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it.
So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.
I'll post my split-up new series for review after testing it, so we can discuss it in more detail?
Olliver
Regards,
Hans
Hey Hans,
On 15-11-16 11:49, Hans de Goede wrote:
Hi,
On 15-11-16 11:29, Olliver Schinagl wrote:
Hey Hans,
I guess I have to contradict something here ...
On 15-11-16 11:17, Olliver Schinagl wrote:
Hey Hans,
I was hopeing and expecting this :)
As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
On 15-11-16 10:26, Hans de Goede wrote:
Hi,
On 15-11-16 04:25, Joe Hershberger wrote:
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
Acked-by: Joe Hershberger joe.hershberger@ni.com
Erm, this patch seems wrong to me: NACK, let me explain:
- It does not do what its commit message says, it only
removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now.
- "Currently we inject 5 ethernet addresses into the environment",
this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi
If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?
- The second attempt at setting ethernet addresses in
the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless
I agree with you here, but I still don't think this should be board specific
- We cannot solely rely on the ethernet driver to set
mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed.
I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific
Of course I didn't realize that the rtl8189etv does not have a u-boot driver, and thus does not get to call its hook and thus nothing sunxi specific will ever be invoked.
So I guess in the case of the rtl8189 we have to figure out something (probably near the same as you did) to make it work.
It does feel somewhat nasty/hackish of course. I would expect that the linux driver sorts this out for itself and not simply assume u-boot supplies this info on non-existing hardware (to u-boot).
We've the same with e.g. the wobo-i5 which has its emac routed to an other set of pins then which are usually used which u-boot does not support. Yet the kernel emac driver relies on u-boot to set a MAC, or it will fall back to a random-MAC (which is undesirable).
Likewise if someone configures u-boot without network support to make it boot faster, we get the same problem with the emac / gmac driver.
The logic here really goes like this:
- When u-boot does have network support, and picks a MAC-address
that MAC-address should be propagated to the kernel so that it uses the same MAC (this is esp. important with switches which allow only 1 MAC per port as a security setting).
- means that u-boot has some algorithm to pick a fixed MAC
in a SoC specific manner. Since we do not want booting with an u-boot with networking enabled resulting in a different fixed MAC then booting on a u-boot without networking support, this means that this algorithm should be used even when networking support in u-boot is disabled (e.g. the wobo-i5 case).
- Given 2. it makes sense to simply have u-boot generate
MACs for all NICs in the system.
I have to digest all you said so far, but
3) makes sense, expect for the 'generate' part :) But I think we mean the same.
From a manufacturing PoV. we have 2 problems. A product with ethernet hardware needs to use registered mac addresses. So you have fixed mac addresses you can use for your product. This needs to be facilitated in some manner.
The MAC needs to be available to the board uniformly, regardless of the used bootloader or OS. E.g. lets say raspberry pi 3 (without going into actual factual details), which can boot windows or linux, both OSes need to get the same MAC address from the board somehow. It is not uncommon to have an EEPROM from this configuration data (even NIC's have eeproms for this).
In any case, a manufacturer of boards wants to program a unique MAC into every board, without having to modify each env/fdt.
The cases you describe are all valid, but or really for hobbist use in small scales. They are likley also not handing out valid MAC addresses, but use the 'for internal use' range.
Of course we want to facilitate both.
So I do agree that u-boot should be responsible for setting the mac of all devices (configured or unconfigured), or 'generate' if you will.
The problem however is 'what are all NIC's in the system'. And again, if there are MAC's in the eeprom, which device gets what eeprom.
Normally, u-boot probes the hardware, based on that you get a fixed order of eth devices and that fixed order decides which MAC to use.
Unless we make the FDT leading here, so that the FDT decides on the probing order.
You may have noticed however, I am in somewhat unfamiliary territory here as to what does u-boot do now, how does the fdt fit in here.
I need some time to think this over, so I'm hoping smarter people then me come with great suggestions here :)
I still think that my suggestion for having fdt_fixup_ethernet() from common/fdt_support.c call a board specific function instead of the "continue" here:
tmp = getenv(mac); if (!tmp) continue; for (j = 0; j < 6; j++) { mac_addr[j] = tmp ? simple_strtoul(tmp, &end,
- : 0; if (tmp) tmp = (*end) ? end + 1 : end; }
E.g:
tmp = getenv(mac); if (!tmp) { if (board_get_ethaddr(i, mac_addr) != 0) continue; } else { for (j = 0; j < 6; j++) { mac_addr[j] = tmp ? simple_strtoul(tmp,&end, 16) : 0; if (tmp) tmp = (*end) ? end + 1 : end; } }
Would be a good idea, with a weak default for
int board_get_ethaddr(int index, unsigned char *mac_addr);
Which simply returns -EINVAL;
This will neatly solve all the problems discussed, you could probably even use the same board_get_ethaddr() in both the generic ethernet setup code you've been working in, as well as in fdt_fixup_ethernet()
Let me digest this all some more and mull it over.
But to summarize, ignoring the ordering of devices for now (which runs havoc in the 'unconfigured' case to speed up booting),
check eeprom for mac if not, call device read rom hook which may be overriden by the board
set to env
let env override hw generated code
check fdt and let the fdt override the env
if fdt has more ethernet devices, add those to the env as well.
Ideally this is where re-ording would take place to match what the fdt says, but we don't know of course what eth0, eth1 and eth2 are bound too... but again, i need to mull and look at the fdt_support bit.
olliver
Regards,
Hans
But for now I'm leaning to think that, the rtl8189 is special.
So is this a board specific hack, or a fdt net specific hack. I does feel like the fdt bit I described earlier injects extra mac addresses into the environment for these unknown hardware pieces ... But that will need to come from board specific pieces, as the net stack never gets invoked for these ...
I'll stop thinking outloud now and get back to work ;)
olliver
- AFAIK the dt code for passing mac-addresses to the kernel
relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable.
The new patch series does that, as the net core layer does that.
What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
eth_read_eeprom_hwaddr(dev);first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);// <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env?
} else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).
} else if (is_zero_ethaddr(pdata->enetaddr)) {#ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it.
So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.
I'll post my split-up new series for review after testing it, so we can discuss it in more detail?
Olliver
Regards,
Hans
This patch uses the newly introduced Kconfig options to use the net_op read_rom_hwaddr to retrieve the MAC from an EEPROM. This will be especially useful for the Olimex OLinuXino series of sunxi boards as they all have an 2k i2c eeprom chip.
The MAC address in the eeprom is ignored (if enabled) if the CRC8 check fails.
This new functionality allows for querying multiple MAC addresses. The first (supported) device being probed gets the first address, the second the second etc. If a generated MAC address is desired, set it to all 0 (and if crc8 is configured also add that) for the adapter.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- board/sunxi/Kconfig | 4 ++++ board/sunxi/board.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig index e1d4ab1..6b8ac99 100644 --- a/board/sunxi/Kconfig +++ b/board/sunxi/Kconfig @@ -414,6 +414,7 @@ config I2C0_ENABLE
config I2C1_ENABLE bool "Enable I2C/TWI controller 1" + default y if (NET_ETHADDR_EEPROM_I2C_BUS = 1) default n select CMD_I2C ---help--- @@ -421,6 +422,7 @@ config I2C1_ENABLE
config I2C2_ENABLE bool "Enable I2C/TWI controller 2" + default y if NET_ETHADDR_EEPROM_I2C_BUS = 2 default n select CMD_I2C ---help--- @@ -428,6 +430,7 @@ config I2C2_ENABLE
if MACH_SUN6I || MACH_SUN7I config I2C3_ENABLE + default y if NET_ETHADDR_EEPROM_I2C_BUS = 3 bool "Enable I2C/TWI controller 3" default n select CMD_I2C @@ -447,6 +450,7 @@ endif
if MACH_SUN7I config I2C4_ENABLE + default y if NET_ETHADDR_EEPROM_I2C_BUS = 4 bool "Enable I2C/TWI controller 4" default n select CMD_I2C diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 71124f4..f1e64cd 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -12,6 +12,7 @@ */
#include <common.h> +#include <i2c.h> #include <mmc.h> #include <axp_pmic.h> #include <asm/arch/clock.h> @@ -31,6 +32,7 @@ #include <crc.h> #include <environment.h> #include <libfdt.h> +#include <linux/crc8.h> #include <nand.h> #include <net.h> #include <sy8106a.h> @@ -626,11 +628,46 @@ static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) memcpy(enetaddr, mac_addr, ARP_HLEN); }
+static void _sunxi_read_rom_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{ + uint8_t eeprom[ARP_HLEN + 1] = { 0x00 }; +#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C) + int old_i2c_bus; + + old_i2c_bus = i2c_get_bus_num(); + if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS) + i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS); + /* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7. */ + if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR, + CONFIG_NET_ETHADDR_EEPROM_OFFSET + (cnt * (ARP_HLEN + 2)), + CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN, + eeprom, ARP_HLEN + 1)) { + i2c_set_bus_num(old_i2c_bus); + puts("Could not read the EEPROM; EEPROM missing?\n"); + return; + } + i2c_set_bus_num(old_i2c_bus); +#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8 + if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) { + puts("CRC error on MAC address from EEPROM.\n"); + return; + } +#endif +#endif + + memcpy(enetaddr, eeprom, ARP_HLEN); +} + static int sunxi_read_rom_hwaddr(unsigned char *enetaddr) { static unsigned int cnt;
- _sunxi_gen_sid_hwaddr(enetaddr, cnt); + _sunxi_read_rom_hwaddr(enetaddr, cnt); + + if (is_zero_ethaddr(enetaddr)) { + _sunxi_gen_sid_hwaddr(enetaddr, cnt); + puts("Serial# based "); + }
/* First call, first stored MAC address, increase for next call */ cnt++;
On 8.11.2016 16:54, Olliver Schinagl wrote:
This patch uses the newly introduced Kconfig options to use the net_op read_rom_hwaddr to retrieve the MAC from an EEPROM. This will be especially useful for the Olimex OLinuXino series of sunxi boards as they all have an 2k i2c eeprom chip.
The MAC address in the eeprom is ignored (if enabled) if the CRC8 check fails.
This new functionality allows for querying multiple MAC addresses. The first (supported) device being probed gets the first address, the second the second etc. If a generated MAC address is desired, set it to all 0 (and if crc8 is configured also add that) for the adapter.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
board/sunxi/Kconfig | 4 ++++ board/sunxi/board.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig index e1d4ab1..6b8ac99 100644 --- a/board/sunxi/Kconfig +++ b/board/sunxi/Kconfig @@ -414,6 +414,7 @@ config I2C0_ENABLE
config I2C1_ENABLE bool "Enable I2C/TWI controller 1"
- default y if (NET_ETHADDR_EEPROM_I2C_BUS = 1) default n select CMD_I2C ---help---
@@ -421,6 +422,7 @@ config I2C1_ENABLE
config I2C2_ENABLE bool "Enable I2C/TWI controller 2"
- default y if NET_ETHADDR_EEPROM_I2C_BUS = 2 default n select CMD_I2C ---help---
@@ -428,6 +430,7 @@ config I2C2_ENABLE
if MACH_SUN6I || MACH_SUN7I config I2C3_ENABLE
- default y if NET_ETHADDR_EEPROM_I2C_BUS = 3 bool "Enable I2C/TWI controller 3" default n select CMD_I2C
@@ -447,6 +450,7 @@ endif
if MACH_SUN7I config I2C4_ENABLE
- default y if NET_ETHADDR_EEPROM_I2C_BUS = 4 bool "Enable I2C/TWI controller 4" default n select CMD_I2C
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 71124f4..f1e64cd 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -12,6 +12,7 @@ */
#include <common.h> +#include <i2c.h> #include <mmc.h> #include <axp_pmic.h> #include <asm/arch/clock.h> @@ -31,6 +32,7 @@ #include <crc.h> #include <environment.h> #include <libfdt.h> +#include <linux/crc8.h> #include <nand.h> #include <net.h> #include <sy8106a.h> @@ -626,11 +628,46 @@ static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) memcpy(enetaddr, mac_addr, ARP_HLEN); }
+static void _sunxi_read_rom_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{
- uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
+#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
- int old_i2c_bus;
- old_i2c_bus = i2c_get_bus_num();
- if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)
i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);- /* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7. */
- if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
CONFIG_NET_ETHADDR_EEPROM_OFFSET + (cnt * (ARP_HLEN + 2)),CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,eeprom, ARP_HLEN + 1)) {i2c_set_bus_num(old_i2c_bus);puts("Could not read the EEPROM; EEPROM missing?\n");return;- }
- i2c_set_bus_num(old_i2c_bus);
+#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
- if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {
puts("CRC error on MAC address from EEPROM.\n");return;- }
+#endif +#endif
- memcpy(enetaddr, eeprom, ARP_HLEN);
+}
ok. I have briefly looked at the whole series and I think that this should be done in the core because this should be shared across all drivers. Because what you have above make in general sense for every board which contain mac address in eeprom. That's why I would create
eeprom_read_rom_etheaddr() in core which will do stuff as you have above and in driver we will simply assign it to read_rom_hwaddr in drivers or by default for all with options to rewrite it. This function will be empty when !NET_ETHADDR_EEPROM.
By this or similar way you open this to all ethernet drivers to read mac just through enabling Kconfig.
IMHO doesn't make sense to c&p the same logic over all ethernet drivers.
Thanks, Michal
Hi Michal,
On 10-11-16 12:51, Michal Simek wrote:
On 8.11.2016 16:54, Olliver Schinagl wrote:
This patch uses the newly introduced Kconfig options to use the net_op read_rom_hwaddr to retrieve the MAC from an EEPROM. This will be especially useful for the Olimex OLinuXino series of sunxi boards as they all have an 2k i2c eeprom chip.
The MAC address in the eeprom is ignored (if enabled) if the CRC8 check fails.
This new functionality allows for querying multiple MAC addresses. The first (supported) device being probed gets the first address, the second the second etc. If a generated MAC address is desired, set it to all 0 (and if crc8 is configured also add that) for the adapter.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
board/sunxi/Kconfig | 4 ++++ board/sunxi/board.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig index e1d4ab1..6b8ac99 100644 --- a/board/sunxi/Kconfig +++ b/board/sunxi/Kconfig @@ -414,6 +414,7 @@ config I2C0_ENABLE
config I2C1_ENABLE bool "Enable I2C/TWI controller 1"
- default y if (NET_ETHADDR_EEPROM_I2C_BUS = 1) default n select CMD_I2C ---help---
@@ -421,6 +422,7 @@ config I2C1_ENABLE
config I2C2_ENABLE bool "Enable I2C/TWI controller 2"
- default y if NET_ETHADDR_EEPROM_I2C_BUS = 2 default n select CMD_I2C ---help---
@@ -428,6 +430,7 @@ config I2C2_ENABLE
if MACH_SUN6I || MACH_SUN7I config I2C3_ENABLE
- default y if NET_ETHADDR_EEPROM_I2C_BUS = 3 bool "Enable I2C/TWI controller 3" default n select CMD_I2C
@@ -447,6 +450,7 @@ endif
if MACH_SUN7I config I2C4_ENABLE
- default y if NET_ETHADDR_EEPROM_I2C_BUS = 4 bool "Enable I2C/TWI controller 4" default n select CMD_I2C
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 71124f4..f1e64cd 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -12,6 +12,7 @@ */
#include <common.h> +#include <i2c.h> #include <mmc.h> #include <axp_pmic.h> #include <asm/arch/clock.h> @@ -31,6 +32,7 @@ #include <crc.h> #include <environment.h> #include <libfdt.h> +#include <linux/crc8.h> #include <nand.h> #include <net.h> #include <sy8106a.h> @@ -626,11 +628,46 @@ static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) memcpy(enetaddr, mac_addr, ARP_HLEN); }
+static void _sunxi_read_rom_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{
- uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
+#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
- int old_i2c_bus;
- old_i2c_bus = i2c_get_bus_num();
- if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)
i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);- /* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7. */
- if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
CONFIG_NET_ETHADDR_EEPROM_OFFSET + (cnt * (ARP_HLEN + 2)),CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,eeprom, ARP_HLEN + 1)) {i2c_set_bus_num(old_i2c_bus);puts("Could not read the EEPROM; EEPROM missing?\n");return;- }
- i2c_set_bus_num(old_i2c_bus);
+#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
- if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {
puts("CRC error on MAC address from EEPROM.\n");return;- }
+#endif +#endif
- memcpy(enetaddr, eeprom, ARP_HLEN);
+}
ok. I have briefly looked at the whole series and I think that this should be done in the core because this should be shared across all drivers. Because what you have above make in general sense for every board which contain mac address in eeprom. That's why I would create
eeprom_read_rom_etheaddr() in core which will do stuff as you have above and in driver we will simply assign it to read_rom_hwaddr in drivers or by default for all with options to rewrite it. This function will be empty when !NET_ETHADDR_EEPROM.
By this or similar way you open this to all ethernet drivers to read mac just through enabling Kconfig.
IMHO doesn't make sense to c&p the same logic over all ethernet drivers.
Initially, I do agree very much. But when I first wrote this last year, there was no other driver yet etc. It is very very generic so maybe make this a weak function up one level, and let the driver override it even?
Makes using the eeprom framework later easier too. I'll cook something up. Good idea!
Olliver
Thanks, Michal
On Thu, Nov 10, 2016 at 6:11 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Hi Michal,
On 10-11-16 12:51, Michal Simek wrote:
On 8.11.2016 16:54, Olliver Schinagl wrote:
This patch uses the newly introduced Kconfig options to use the net_op read_rom_hwaddr to retrieve the MAC from an EEPROM. This will be especially useful for the Olimex OLinuXino series of sunxi boards as they all have an 2k i2c eeprom chip.
The MAC address in the eeprom is ignored (if enabled) if the CRC8 check fails.
This new functionality allows for querying multiple MAC addresses. The first (supported) device being probed gets the first address, the second the second etc. If a generated MAC address is desired, set it to all 0 (and if crc8 is configured also add that) for the adapter.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
board/sunxi/Kconfig | 4 ++++ board/sunxi/board.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig index e1d4ab1..6b8ac99 100644 --- a/board/sunxi/Kconfig +++ b/board/sunxi/Kconfig @@ -414,6 +414,7 @@ config I2C0_ENABLE config I2C1_ENABLE bool "Enable I2C/TWI controller 1"
default y if (NET_ETHADDR_EEPROM_I2C_BUS = 1) default n select CMD_I2C ---help---@@ -421,6 +422,7 @@ config I2C1_ENABLE config I2C2_ENABLE bool "Enable I2C/TWI controller 2"
default y if NET_ETHADDR_EEPROM_I2C_BUS = 2 default n select CMD_I2C ---help---@@ -428,6 +430,7 @@ config I2C2_ENABLE if MACH_SUN6I || MACH_SUN7I config I2C3_ENABLE
default y if NET_ETHADDR_EEPROM_I2C_BUS = 3 bool "Enable I2C/TWI controller 3" default n select CMD_I2C@@ -447,6 +450,7 @@ endif if MACH_SUN7I config I2C4_ENABLE
default y if NET_ETHADDR_EEPROM_I2C_BUS = 4 bool "Enable I2C/TWI controller 4" default n select CMD_I2Cdiff --git a/board/sunxi/board.c b/board/sunxi/board.c index 71124f4..f1e64cd 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -12,6 +12,7 @@ */ #include <common.h> +#include <i2c.h> #include <mmc.h> #include <axp_pmic.h> #include <asm/arch/clock.h> @@ -31,6 +32,7 @@ #include <crc.h> #include <environment.h> #include <libfdt.h> +#include <linux/crc8.h> #include <nand.h> #include <net.h> #include <sy8106a.h> @@ -626,11 +628,46 @@ static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) memcpy(enetaddr, mac_addr, ARP_HLEN); } +static void _sunxi_read_rom_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{
uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };+#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
int old_i2c_bus;old_i2c_bus = i2c_get_bus_num();if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);/* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7. */if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,CONFIG_NET_ETHADDR_EEPROM_OFFSET + (cnt * (ARP_HLEN- 2)),
CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,eeprom, ARP_HLEN + 1)) {i2c_set_bus_num(old_i2c_bus);puts("Could not read the EEPROM; EEPROM missing?\n");return;}i2c_set_bus_num(old_i2c_bus);+#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {puts("CRC error on MAC address from EEPROM.\n");return;}+#endif +#endif
memcpy(enetaddr, eeprom, ARP_HLEN);+}
ok. I have briefly looked at the whole series and I think that this should be done in the core because this should be shared across all drivers. Because what you have above make in general sense for every board which contain mac address in eeprom. That's why I would create
eeprom_read_rom_etheaddr() in core which will do stuff as you have above and in driver we will simply assign it to read_rom_hwaddr in drivers or by default for all with options to rewrite it. This function will be empty when !NET_ETHADDR_EEPROM.
By this or similar way you open this to all ethernet drivers to read mac just through enabling Kconfig.
IMHO doesn't make sense to c&p the same logic over all ethernet drivers.
Initially, I do agree very much. But when I first wrote this last year, there was no other driver yet etc. It is very very generic so maybe make this a weak function up one level, and let the driver override it even?
Makes using the eeprom framework later easier too. I'll cook something up. Good idea!
Do you think it is valuable enough to apply as is and retrofit later, or just wait on this series?
Thanks, -Joe
On 15.11.2016 04:27, Joe Hershberger wrote:
On Thu, Nov 10, 2016 at 6:11 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Hi Michal,
On 10-11-16 12:51, Michal Simek wrote:
On 8.11.2016 16:54, Olliver Schinagl wrote:
This patch uses the newly introduced Kconfig options to use the net_op read_rom_hwaddr to retrieve the MAC from an EEPROM. This will be especially useful for the Olimex OLinuXino series of sunxi boards as they all have an 2k i2c eeprom chip.
The MAC address in the eeprom is ignored (if enabled) if the CRC8 check fails.
This new functionality allows for querying multiple MAC addresses. The first (supported) device being probed gets the first address, the second the second etc. If a generated MAC address is desired, set it to all 0 (and if crc8 is configured also add that) for the adapter.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
board/sunxi/Kconfig | 4 ++++ board/sunxi/board.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig index e1d4ab1..6b8ac99 100644 --- a/board/sunxi/Kconfig +++ b/board/sunxi/Kconfig @@ -414,6 +414,7 @@ config I2C0_ENABLE config I2C1_ENABLE bool "Enable I2C/TWI controller 1"
default y if (NET_ETHADDR_EEPROM_I2C_BUS = 1) default n select CMD_I2C ---help---@@ -421,6 +422,7 @@ config I2C1_ENABLE config I2C2_ENABLE bool "Enable I2C/TWI controller 2"
default y if NET_ETHADDR_EEPROM_I2C_BUS = 2 default n select CMD_I2C ---help---@@ -428,6 +430,7 @@ config I2C2_ENABLE if MACH_SUN6I || MACH_SUN7I config I2C3_ENABLE
default y if NET_ETHADDR_EEPROM_I2C_BUS = 3 bool "Enable I2C/TWI controller 3" default n select CMD_I2C@@ -447,6 +450,7 @@ endif if MACH_SUN7I config I2C4_ENABLE
default y if NET_ETHADDR_EEPROM_I2C_BUS = 4 bool "Enable I2C/TWI controller 4" default n select CMD_I2Cdiff --git a/board/sunxi/board.c b/board/sunxi/board.c index 71124f4..f1e64cd 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -12,6 +12,7 @@ */ #include <common.h> +#include <i2c.h> #include <mmc.h> #include <axp_pmic.h> #include <asm/arch/clock.h> @@ -31,6 +32,7 @@ #include <crc.h> #include <environment.h> #include <libfdt.h> +#include <linux/crc8.h> #include <nand.h> #include <net.h> #include <sy8106a.h> @@ -626,11 +628,46 @@ static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) memcpy(enetaddr, mac_addr, ARP_HLEN); } +static void _sunxi_read_rom_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{
uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };+#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
int old_i2c_bus;old_i2c_bus = i2c_get_bus_num();if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);/* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7. */if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,CONFIG_NET_ETHADDR_EEPROM_OFFSET + (cnt * (ARP_HLEN- 2)),
CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,eeprom, ARP_HLEN + 1)) {i2c_set_bus_num(old_i2c_bus);puts("Could not read the EEPROM; EEPROM missing?\n");return;}i2c_set_bus_num(old_i2c_bus);+#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {puts("CRC error on MAC address from EEPROM.\n");return;}+#endif +#endif
memcpy(enetaddr, eeprom, ARP_HLEN);+}
ok. I have briefly looked at the whole series and I think that this should be done in the core because this should be shared across all drivers. Because what you have above make in general sense for every board which contain mac address in eeprom. That's why I would create
eeprom_read_rom_etheaddr() in core which will do stuff as you have above and in driver we will simply assign it to read_rom_hwaddr in drivers or by default for all with options to rewrite it. This function will be empty when !NET_ETHADDR_EEPROM.
By this or similar way you open this to all ethernet drivers to read mac just through enabling Kconfig.
IMHO doesn't make sense to c&p the same logic over all ethernet drivers.
Initially, I do agree very much. But when I first wrote this last year, there was no other driver yet etc. It is very very generic so maybe make this a weak function up one level, and let the driver override it even?
Makes using the eeprom framework later easier too. I'll cook something up. Good idea!
Do you think it is valuable enough to apply as is and retrofit later, or just wait on this series?
TBH I would split this series to two to get Sunxi part in and this get later.
Thanks, Michal
Hey Joe, Michal,
On 15-11-16 08:22, Michal Simek wrote:
On 15.11.2016 04:27, Joe Hershberger wrote:
On Thu, Nov 10, 2016 at 6:11 AM, Olliver Schinagl oliver@schinagl.nl wrote:
Hi Michal,
On 10-11-16 12:51, Michal Simek wrote:
On 8.11.2016 16:54, Olliver Schinagl wrote:
This patch uses the newly introduced Kconfig options to use the net_op read_rom_hwaddr to retrieve the MAC from an EEPROM. This will be especially useful for the Olimex OLinuXino series of sunxi boards as they all have an 2k i2c eeprom chip.
The MAC address in the eeprom is ignored (if enabled) if the CRC8 check fails.
This new functionality allows for querying multiple MAC addresses. The first (supported) device being probed gets the first address, the second the second etc. If a generated MAC address is desired, set it to all 0 (and if crc8 is configured also add that) for the adapter.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl
board/sunxi/Kconfig | 4 ++++ board/sunxi/board.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig index e1d4ab1..6b8ac99 100644 --- a/board/sunxi/Kconfig +++ b/board/sunxi/Kconfig @@ -414,6 +414,7 @@ config I2C0_ENABLE config I2C1_ENABLE bool "Enable I2C/TWI controller 1"
default y if (NET_ETHADDR_EEPROM_I2C_BUS = 1) default n select CMD_I2C ---help---@@ -421,6 +422,7 @@ config I2C1_ENABLE config I2C2_ENABLE bool "Enable I2C/TWI controller 2"
default y if NET_ETHADDR_EEPROM_I2C_BUS = 2 default n select CMD_I2C ---help---@@ -428,6 +430,7 @@ config I2C2_ENABLE if MACH_SUN6I || MACH_SUN7I config I2C3_ENABLE
default y if NET_ETHADDR_EEPROM_I2C_BUS = 3 bool "Enable I2C/TWI controller 3" default n select CMD_I2C@@ -447,6 +450,7 @@ endif if MACH_SUN7I config I2C4_ENABLE
default y if NET_ETHADDR_EEPROM_I2C_BUS = 4 bool "Enable I2C/TWI controller 4" default n select CMD_I2Cdiff --git a/board/sunxi/board.c b/board/sunxi/board.c index 71124f4..f1e64cd 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -12,6 +12,7 @@ */ #include <common.h> +#include <i2c.h> #include <mmc.h> #include <axp_pmic.h> #include <asm/arch/clock.h> @@ -31,6 +32,7 @@ #include <crc.h> #include <environment.h> #include <libfdt.h> +#include <linux/crc8.h> #include <nand.h> #include <net.h> #include <sy8106a.h> @@ -626,11 +628,46 @@ static void _sunxi_gen_sid_hwaddr(unsigned char *enetaddr, uint8_t cnt) memcpy(enetaddr, mac_addr, ARP_HLEN); } +static void _sunxi_read_rom_hwaddr(unsigned char *enetaddr, uint8_t cnt) +{
uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };+#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
int old_i2c_bus;old_i2c_bus = i2c_get_bus_num();if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);/* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7. */if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,CONFIG_NET_ETHADDR_EEPROM_OFFSET + (cnt * (ARP_HLEN- 2)),
CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,eeprom, ARP_HLEN + 1)) {i2c_set_bus_num(old_i2c_bus);puts("Could not read the EEPROM; EEPROM missing?\n");return;}i2c_set_bus_num(old_i2c_bus);+#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {puts("CRC error on MAC address from EEPROM.\n");return;}+#endif +#endif
memcpy(enetaddr, eeprom, ARP_HLEN);+}
ok. I have briefly looked at the whole series and I think that this should be done in the core because this should be shared across all drivers. Because what you have above make in general sense for every board which contain mac address in eeprom. That's why I would create
eeprom_read_rom_etheaddr() in core which will do stuff as you have above and in driver we will simply assign it to read_rom_hwaddr in drivers or by default for all with options to rewrite it. This function will be empty when !NET_ETHADDR_EEPROM.
By this or similar way you open this to all ethernet drivers to read mac just through enabling Kconfig.
IMHO doesn't make sense to c&p the same logic over all ethernet drivers.
Initially, I do agree very much. But when I first wrote this last year, there was no other driver yet etc. It is very very generic so maybe make this a weak function up one level, and let the driver override it even?
Makes using the eeprom framework later easier too. I'll cook something up. Good idea!
Do you think it is valuable enough to apply as is and retrofit later, or just wait on this series?
TBH I would split this series to two to get Sunxi part in and this get later.
I have both series ready and they just need to be tested. I'll test it hopefully later today, and send the 2 seperate patch series very soon (within a day or so, not a year :p)
Olliver
Thanks, Michal
This patch enables the I2C EEPROM to be probed for a MAC address on the OLinuXino Lime1 and Lime2 boards. Other boards surely qualify as well but where not tested yet.
Signed-off-by: Olliver Schinagl oliver@schinagl.nl --- configs/A10-OLinuXino-Lime_defconfig | 3 +++ configs/A20-OLinuXino-Lime2_defconfig | 3 +++ configs/A20-OLinuXino-Lime_defconfig | 3 +++ configs/A20-OLinuXino_MICRO_defconfig | 3 +++ 4 files changed, 12 insertions(+)
diff --git a/configs/A10-OLinuXino-Lime_defconfig b/configs/A10-OLinuXino-Lime_defconfig index 04b720d..dce33c8 100644 --- a/configs/A10-OLinuXino-Lime_defconfig +++ b/configs/A10-OLinuXino-Lime_defconfig @@ -13,6 +13,9 @@ CONFIG_SPL=y # CONFIG_CMD_IMLS is not set # CONFIG_CMD_FLASH is not set # CONFIG_CMD_FPGA is not set +CONFIG_NET_ETHADDR_EEPROM=y +CONFIG_NET_ETHADDR_EEPROM_I2C=y +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS=1 CONFIG_AXP_ALDO3_VOLT=2800 CONFIG_AXP_ALDO4_VOLT=2800 CONFIG_USB_EHCI_HCD=y diff --git a/configs/A20-OLinuXino-Lime2_defconfig b/configs/A20-OLinuXino-Lime2_defconfig index 4751fe0..f2424c8 100644 --- a/configs/A20-OLinuXino-Lime2_defconfig +++ b/configs/A20-OLinuXino-Lime2_defconfig @@ -18,6 +18,9 @@ CONFIG_CMD_USB_MASS_STORAGE=y CONFIG_DFU_RAM=y CONFIG_RTL8211X_PHY_FORCE_MASTER=y CONFIG_ETH_DESIGNWARE=y +CONFIG_NET_ETHADDR_EEPROM=y +CONFIG_NET_ETHADDR_EEPROM_I2C=y +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS=1 CONFIG_AXP_ALDO3_VOLT=2800 CONFIG_AXP_ALDO4_VOLT=2800 CONFIG_USB_EHCI_HCD=y diff --git a/configs/A20-OLinuXino-Lime_defconfig b/configs/A20-OLinuXino-Lime_defconfig index 024dc2d..0e50d46 100644 --- a/configs/A20-OLinuXino-Lime_defconfig +++ b/configs/A20-OLinuXino-Lime_defconfig @@ -12,6 +12,9 @@ CONFIG_SPL=y # CONFIG_CMD_FLASH is not set # CONFIG_CMD_FPGA is not set CONFIG_ETH_DESIGNWARE=y +CONFIG_NET_ETHADDR_EEPROM=y +CONFIG_NET_ETHADDR_EEPROM_I2C=y +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS=1 CONFIG_AXP_ALDO3_VOLT=2800 CONFIG_AXP_ALDO4_VOLT=2800 CONFIG_USB_EHCI_HCD=y diff --git a/configs/A20-OLinuXino_MICRO_defconfig b/configs/A20-OLinuXino_MICRO_defconfig index 5809345..b70dd2f 100644 --- a/configs/A20-OLinuXino_MICRO_defconfig +++ b/configs/A20-OLinuXino_MICRO_defconfig @@ -15,6 +15,9 @@ CONFIG_SPL=y # CONFIG_CMD_FLASH is not set # CONFIG_CMD_FPGA is not set CONFIG_ETH_DESIGNWARE=y +CONFIG_NET_ETHADDR_EEPROM=y +CONFIG_NET_ETHADDR_EEPROM_I2C=y +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS=1 CONFIG_AXP_ALDO3_VOLT=2800 CONFIG_AXP_ALDO4_VOLT=2800 CONFIG_USB_EHCI_HCD=y
This patch enables crc8 to be used from within the tools directory using u-boot/crc.h.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com Reviewed-by: Joe Hershberger joe.hershberger@ni.com --- include/u-boot/crc.h | 3 +++ tools/Makefile | 1 + 2 files changed, 4 insertions(+)
diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h index 754ac72..6764d58 100644 --- a/include/u-boot/crc.h +++ b/include/u-boot/crc.h @@ -9,6 +9,9 @@ #ifndef _UBOOT_CRC_H #define _UBOOT_CRC_H
+/* lib/crc8.c */ +unsigned int crc8(unsigned int crc_start, const unsigned char *vptr, int len); + /* lib/crc32.c */ uint32_t crc32 (uint32_t, const unsigned char *, uint); uint32_t crc32_wd (uint32_t, const unsigned char *, uint, uint); diff --git a/tools/Makefile b/tools/Makefile index 400588c..06afdb0 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -191,6 +191,7 @@ fdtgrep-objs += $(LIBFDT_OBJS) fdtgrep.o # that won't build on some weird host compiler -- though there are lots of # exceptions for files that aren't complaint. HOSTCFLAGS_crc32.o := -pedantic +HOSTCFLAGS_crc8.o := -pedantic HOSTCFLAGS_md5.o := -pedantic HOSTCFLAGS_sha1.o := -pedantic HOSTCFLAGS_sha256.o := -pedantic
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
This patch enables crc8 to be used from within the tools directory using u-boot/crc.h.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com Reviewed-by: Joe Hershberger joe.hershberger@ni.com
include/u-boot/crc.h | 3 +++ tools/Makefile | 1 + 2 files changed, 4 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
This patch adds a little tool that takes a generic MAC address and generates a CRC byte for it. The output is the full MAC address without any separators, ready written into an EEPROM.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com --- tools/.gitignore | 1 + tools/Makefile | 4 ++++ tools/gen_ethaddr_crc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 tools/gen_ethaddr_crc.c
diff --git a/tools/.gitignore b/tools/.gitignore index cb1e722..0d1f076 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -6,6 +6,7 @@ /fit_check_sign /fit_info /gen_eth_addr +/gen_ethaddr_crc /ifdtool /img2srec /kwboot diff --git a/tools/Makefile b/tools/Makefile index 06afdb0..4879ded 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -43,6 +43,10 @@ envcrc-objs := envcrc.o lib/crc32.o common/env_embedded.o lib/sha1.o hostprogs-$(CONFIG_CMD_NET) += gen_eth_addr HOSTCFLAGS_gen_eth_addr.o := -pedantic
+hostprogs-$(CONFIG_CMD_NET) += gen_ethaddr_crc +gen_ethaddr_crc-objs := gen_ethaddr_crc.o lib/crc8.o +HOSTCFLAGS_gen_ethaddr_crc.o := -pedantic + hostprogs-$(CONFIG_CMD_LOADS) += img2srec HOSTCFLAGS_img2srec.o := -pedantic
diff --git a/tools/gen_ethaddr_crc.c b/tools/gen_ethaddr_crc.c new file mode 100644 index 0000000..9b5bdb0 --- /dev/null +++ b/tools/gen_ethaddr_crc.c @@ -0,0 +1,52 @@ +/* + * (C) Copyright 2016 + * Olliver Schinagl oliver@schinagl.nl + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <ctype.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <u-boot/crc.h> + +#define ARP_HLEN 6 /* Length of hardware address */ + +int main(int argc, char *argv[]) +{ + uint_fast8_t i; + uint8_t mac_addr[ARP_HLEN + 1] = { 0x00 }; + + if (argc < 2) { + puts("Please supply a MAC address."); + return -1; + } + + if (!((strlen(argv[1]) == 12) || (strlen(argv[1]) == 17))) { + puts("Please supply a valid MAC address with optionaly\n" + "dashes (-) or colons (:) as seperators."); + return -1; + } + + i = 0; + while (*argv[1] != '\0') { + char nibble[2] = { 0x00, '\n' }; /* for strtol */ + + nibble[0] = *argv[1]++; + if (isxdigit(nibble[0])) { + if (isupper(nibble[0])) + nibble[0] = tolower(nibble[0]); + mac_addr[i >> 1] |= strtol(nibble, NULL, 16) << ((i % 2) ? 0 : 4) & ((i % 2) ? 0x0f : 0xf0); + i++; + } + } + mac_addr[ARP_HLEN] = crc8(0, mac_addr, ARP_HLEN); + + for (i = 0; i < ARP_HLEN + 1; i++) + printf("%.2x", mac_addr[i]); + putchar('\n'); + + return 0; +}
Hi Olliver, (is it one l or two?)
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
This patch adds a little tool that takes a generic MAC address and generates a CRC byte for it. The output is the full MAC address without any separators, ready written into an EEPROM.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com
tools/.gitignore | 1 + tools/Makefile | 4 ++++ tools/gen_ethaddr_crc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 tools/gen_ethaddr_crc.c
diff --git a/tools/.gitignore b/tools/.gitignore index cb1e722..0d1f076 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -6,6 +6,7 @@ /fit_check_sign /fit_info /gen_eth_addr +/gen_ethaddr_crc /ifdtool /img2srec /kwboot diff --git a/tools/Makefile b/tools/Makefile index 06afdb0..4879ded 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -43,6 +43,10 @@ envcrc-objs := envcrc.o lib/crc32.o common/env_embedded.o lib/sha1.o hostprogs-$(CONFIG_CMD_NET) += gen_eth_addr HOSTCFLAGS_gen_eth_addr.o := -pedantic
+hostprogs-$(CONFIG_CMD_NET) += gen_ethaddr_crc +gen_ethaddr_crc-objs := gen_ethaddr_crc.o lib/crc8.o +HOSTCFLAGS_gen_ethaddr_crc.o := -pedantic
hostprogs-$(CONFIG_CMD_LOADS) += img2srec HOSTCFLAGS_img2srec.o := -pedantic
diff --git a/tools/gen_ethaddr_crc.c b/tools/gen_ethaddr_crc.c new file mode 100644 index 0000000..9b5bdb0 --- /dev/null +++ b/tools/gen_ethaddr_crc.c @@ -0,0 +1,52 @@ +/*
- (C) Copyright 2016
- Olliver Schinagl oliver@schinagl.nl
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <ctype.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <u-boot/crc.h>
+#define ARP_HLEN 6 /* Length of hardware address */
Is there no #define for this in standard headers?
+int main(int argc, char *argv[]) +{
uint_fast8_t i;uint8_t mac_addr[ARP_HLEN + 1] = { 0x00 };
Why do you need to init it?
if (argc < 2) {puts("Please supply a MAC address.");
How about a usage() function which gives generic help as well?
return -1;}if (!((strlen(argv[1]) == 12) || (strlen(argv[1]) == 17))) {puts("Please supply a valid MAC address with optionaly\n"
optionally. But do you mean 'Please supply a valid MAC address with optional - or : separators?'
"dashes (-) or colons (:) as seperators.");
separators.
return -1;}i = 0;while (*argv[1] != '\0') {
How about putting this code in a separate function:
int process_mac(const char *max)
so you don't have to access argv[1] all the time. Also you can return 1 instead of -1 from main() on error.
char nibble[2] = { 0x00, '\n' }; /* for strtol */nibble[0] = *argv[1]++;if (isxdigit(nibble[0])) {if (isupper(nibble[0]))nibble[0] = tolower(nibble[0]);mac_addr[i >> 1] |= strtol(nibble, NULL, 16) << ((i % 2) ? 0 : 4) & ((i % 2) ? 0x0f : 0xf0);
How about a nibble_to_hex() function here? This is strange-looking code. I'm wondering what you are trying to do.
i++;}}mac_addr[ARP_HLEN] = crc8(0, mac_addr, ARP_HLEN);
I suggest putting this in a separate variable...
for (i = 0; i < ARP_HLEN + 1; i++)printf("%.2x", mac_addr[i]);putchar('\n');
and here: printf("%.2x\n", crc)
return 0;+}
2.10.2
Regards, Simon
On Fri, Nov 11, 2016 at 10:18 AM, Simon Glass sjg@chromium.org wrote:
Hi Olliver, (is it one l or two?)
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
This patch adds a little tool that takes a generic MAC address and generates a CRC byte for it. The output is the full MAC address without any separators, ready written into an EEPROM.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com
tools/.gitignore | 1 + tools/Makefile | 4 ++++ tools/gen_ethaddr_crc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 tools/gen_ethaddr_crc.c
diff --git a/tools/.gitignore b/tools/.gitignore index cb1e722..0d1f076 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -6,6 +6,7 @@ /fit_check_sign /fit_info /gen_eth_addr +/gen_ethaddr_crc /ifdtool /img2srec /kwboot diff --git a/tools/Makefile b/tools/Makefile index 06afdb0..4879ded 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -43,6 +43,10 @@ envcrc-objs := envcrc.o lib/crc32.o common/env_embedded.o lib/sha1.o hostprogs-$(CONFIG_CMD_NET) += gen_eth_addr HOSTCFLAGS_gen_eth_addr.o := -pedantic
+hostprogs-$(CONFIG_CMD_NET) += gen_ethaddr_crc +gen_ethaddr_crc-objs := gen_ethaddr_crc.o lib/crc8.o +HOSTCFLAGS_gen_ethaddr_crc.o := -pedantic
hostprogs-$(CONFIG_CMD_LOADS) += img2srec HOSTCFLAGS_img2srec.o := -pedantic
diff --git a/tools/gen_ethaddr_crc.c b/tools/gen_ethaddr_crc.c new file mode 100644 index 0000000..9b5bdb0 --- /dev/null +++ b/tools/gen_ethaddr_crc.c @@ -0,0 +1,52 @@ +/*
- (C) Copyright 2016
- Olliver Schinagl oliver@schinagl.nl
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <ctype.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <u-boot/crc.h>
+#define ARP_HLEN 6 /* Length of hardware address */
Is there no #define for this in standard headers?
There is. ARP_HLEN is defined in include/net.h
Hey Simon, Joe,
On 15-11-16 04:31, Joe Hershberger wrote:
On Fri, Nov 11, 2016 at 10:18 AM, Simon Glass sjg@chromium.org wrote:
Hi Olliver, (is it one l or two?)
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
This patch adds a little tool that takes a generic MAC address and generates a CRC byte for it. The output is the full MAC address without any separators, ready written into an EEPROM.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com
tools/.gitignore | 1 + tools/Makefile | 4 ++++ tools/gen_ethaddr_crc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 tools/gen_ethaddr_crc.c
diff --git a/tools/.gitignore b/tools/.gitignore index cb1e722..0d1f076 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -6,6 +6,7 @@ /fit_check_sign /fit_info /gen_eth_addr +/gen_ethaddr_crc /ifdtool /img2srec /kwboot diff --git a/tools/Makefile b/tools/Makefile index 06afdb0..4879ded 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -43,6 +43,10 @@ envcrc-objs := envcrc.o lib/crc32.o common/env_embedded.o lib/sha1.o hostprogs-$(CONFIG_CMD_NET) += gen_eth_addr HOSTCFLAGS_gen_eth_addr.o := -pedantic
+hostprogs-$(CONFIG_CMD_NET) += gen_ethaddr_crc +gen_ethaddr_crc-objs := gen_ethaddr_crc.o lib/crc8.o +HOSTCFLAGS_gen_ethaddr_crc.o := -pedantic
- hostprogs-$(CONFIG_CMD_LOADS) += img2srec HOSTCFLAGS_img2srec.o := -pedantic
diff --git a/tools/gen_ethaddr_crc.c b/tools/gen_ethaddr_crc.c new file mode 100644 index 0000000..9b5bdb0 --- /dev/null +++ b/tools/gen_ethaddr_crc.c @@ -0,0 +1,52 @@ +/*
- (C) Copyright 2016
- Olliver Schinagl oliver@schinagl.nl
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <ctype.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <u-boot/crc.h>
+#define ARP_HLEN 6 /* Length of hardware address */
Is there no #define for this in standard headers?
There is. ARP_HLEN is defined in include/net.h
Yep, but including net.h then makes net.h very unhappy (lots of missing things, u32 to begin with) then you add those includes and the party continues with lots of other missing includes. I managed to get all those includes sorted at some point, but then using the functions initially suggested by Joe, caused a whole lot of other library and include files to be missing. So I didn't think it was worth the effort.
Thus I suggest, merge as is, and if someone wants to start cleaning it up (by fixing net.h) that would be awesome. The idea for this tool was to be able to quickly generate a crc8 code :)
Hey Simon,
On 11-11-16 17:18, Simon Glass wrote:
Hi Olliver, (is it one l or two?)
On 8 November 2016 at 08:54, Olliver Schinagl oliver@schinagl.nl wrote:
This patch adds a little tool that takes a generic MAC address and generates a CRC byte for it. The output is the full MAC address without any separators, ready written into an EEPROM.
Signed-off-by: Olliver Schinagl o.schinagl@ultimaker.com
tools/.gitignore | 1 + tools/Makefile | 4 ++++ tools/gen_ethaddr_crc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 tools/gen_ethaddr_crc.c
diff --git a/tools/.gitignore b/tools/.gitignore index cb1e722..0d1f076 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -6,6 +6,7 @@ /fit_check_sign /fit_info /gen_eth_addr +/gen_ethaddr_crc /ifdtool /img2srec /kwboot diff --git a/tools/Makefile b/tools/Makefile index 06afdb0..4879ded 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -43,6 +43,10 @@ envcrc-objs := envcrc.o lib/crc32.o common/env_embedded.o lib/sha1.o hostprogs-$(CONFIG_CMD_NET) += gen_eth_addr HOSTCFLAGS_gen_eth_addr.o := -pedantic
+hostprogs-$(CONFIG_CMD_NET) += gen_ethaddr_crc +gen_ethaddr_crc-objs := gen_ethaddr_crc.o lib/crc8.o +HOSTCFLAGS_gen_ethaddr_crc.o := -pedantic
- hostprogs-$(CONFIG_CMD_LOADS) += img2srec HOSTCFLAGS_img2srec.o := -pedantic
diff --git a/tools/gen_ethaddr_crc.c b/tools/gen_ethaddr_crc.c new file mode 100644 index 0000000..9b5bdb0 --- /dev/null +++ b/tools/gen_ethaddr_crc.c @@ -0,0 +1,52 @@ +/*
- (C) Copyright 2016
- Olliver Schinagl oliver@schinagl.nl
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <ctype.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <u-boot/crc.h>
+#define ARP_HLEN 6 /* Length of hardware address */
Is there no #define for this in standard headers?
+int main(int argc, char *argv[]) +{
uint_fast8_t i;uint8_t mac_addr[ARP_HLEN + 1] = { 0x00 };Why do you need to init it?
Because I did it in the net stuff as well, where I used the 'is_zero_mac()'' to indicate things where not setup properly. I guess it can go here ...
if (argc < 2) {puts("Please supply a MAC address.");How about a usage() function which gives generic help as well?
fair point, You caught me on being lazy :) I didn't think of it for such a simple tool. I'll put it in the next version.
return -1;}if (!((strlen(argv[1]) == 12) || (strlen(argv[1]) == 17))) {puts("Please supply a valid MAC address with optionaly\n"optionally. But do you mean 'Please supply a valid MAC address with optional - or : separators?'
"dashes (-) or colons (:) as seperators.");separators.
return -1;}i = 0;while (*argv[1] != '\0') {How about putting this code in a separate function:
int process_mac(const char *max)
so you don't have to access argv[1] all the time. Also you can return 1 instead of -1 from main() on error.
right, no prob.
char nibble[2] = { 0x00, '\n' }; /* for strtol */nibble[0] = *argv[1]++;if (isxdigit(nibble[0])) {if (isupper(nibble[0]))nibble[0] = tolower(nibble[0]);mac_addr[i >> 1] |= strtol(nibble, NULL, 16) << ((i % 2) ? 0 : 4) & ((i % 2) ? 0x0f : 0xf0);How about a nibble_to_hex() function here? This is strange-looking code. I'm wondering what you are trying to do.
It is strange, and we even have a nice function in u-boot I belive, but it's a pain to get to add it to this, hence the manual way. I'll add some doc to the func as well.
i++;}}mac_addr[ARP_HLEN] = crc8(0, mac_addr, ARP_HLEN);I suggest putting this in a separate variable...
for (i = 0; i < ARP_HLEN + 1; i++)printf("%.2x", mac_addr[i]);putchar('\n');and here: printf("%.2x\n", crc)
yeah i do like the separate var for here.
return 0;+}
2.10.2
Regards, Simon
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.
Thanks, Michal
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.
Thanks, Michal
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?
Thanks, Michal
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
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
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)
Thanks, Michal
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
participants (6)
-
Hans de Goede -
Joe Hershberger -
Michal Simek -
Olliver Schinagl -
Olliver Schinagl -
Simon Glass