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

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

[Olliver Schinagl]

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.

[U-Boot] [PATCH 01/11] net: dw: Add read_rom_hwaddr net_op hook

[Olliver Schinagl]

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)
-- 
2.10.2

[U-Boot] [PATCH 02/11] net: sunxi-emac: Write HW address via function

[Olliver Schinagl]

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, &regs->mac_a1);
+	writel(enetaddr_lo, &regs->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],
-	       &regs->mac_a1);
-	writel(enetaddr[3] << 16 | enetaddr[4] << 8 | enetaddr[5],
-	       &regs->mac_a0);
+	_sunxi_write_hwaddr(priv, enetaddr);
 
 	mdelay(1);
 
-- 
2.10.2

[U-Boot] [PATCH 03/11] net: sunxi-emac: Add write_hwaddr net_op hook

[Olliver Schinagl]

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)
-- 
2.10.2

[U-Boot] [PATCH 04/11] net: sunxi-emac: Add read_rom_hwaddr net_op hook

[Olliver Schinagl]

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)
-- 
2.10.2

[U-Boot] [PATCH 05/11] net: Add ability to set MAC address via EEPROM to Kconfig

[Olliver Schinagl]

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
-- 
2.10.2

[U-Boot] [PATCH 06/11] arm: sunxi: Use read_rom_hwaddr() to obtain MAC address

[Olliver Schinagl]

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]);
-- 
2.10.2

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Olliver Schinagl]

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)
-- 
2.10.2

[U-Boot] [PATCH 08/11] net: sunxi: Allow sunxi boards to set the MAC from an EEPROM

[Olliver Schinagl]

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++;
-- 
2.10.2

[U-Boot] [PATCH 09/11] net: sunxi: Enable eeprom on OLinuXino Lime boards

[Olliver Schinagl]

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
-- 
2.10.2

[U-Boot] [PATCH 10/11] tools: Allow crc8 to be used

[Olliver Schinagl]

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
-- 
2.10.2

[U-Boot] [PATCH 11/11] tools: Add tool to add crc8 to a mac address

[Olliver Schinagl]

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;
+}
-- 
2.10.2

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

[Michal Simek]

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

[U-Boot] [PATCH 08/11] net: sunxi: Allow sunxi boards to set the MAC from an EEPROM

[Michal Simek]

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

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

[Olliver Schinagl]

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
>
>

[U-Boot] [PATCH 08/11] net: sunxi: Allow sunxi boards to set the MAC from an EEPROM

[Olliver Schinagl]

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
>
>
>

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

[Michal Simek]

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

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

[Olliver Schinagl]

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

Olliver

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

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

[Michal Simek]

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

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

Thanks,
Michal

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

[Olliver Schinagl]

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
>
>

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

[Michal Simek]

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

yep - not a problem with it.

Thanks,
Michal

[U-Boot] [PATCH 10/11] tools: Allow crc8 to be used

[Simon Glass]

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>

[U-Boot] [PATCH 11/11] tools: Add tool to add crc8 to a mac address

[Simon Glass]

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

[U-Boot] [PATCH 01/11] net: dw: Add read_rom_hwaddr net_op hook

[Joe Hershberger]

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>

[U-Boot] [PATCH 02/11] net: sunxi-emac: Write HW address via function

[Joe Hershberger]

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>

[U-Boot] [PATCH 03/11] net: sunxi-emac: Add write_hwaddr net_op hook

[Joe Hershberger]

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>

[U-Boot] [PATCH 04/11] net: sunxi-emac: Add read_rom_hwaddr net_op hook

[Joe Hershberger]

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>

[U-Boot] [PATCH 06/11] arm: sunxi: Use read_rom_hwaddr() to obtain MAC address

[Joe Hershberger]

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>

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Joe Hershberger]

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>

[U-Boot] [PATCH 08/11] net: sunxi: Allow sunxi boards to set the MAC from an EEPROM

[Joe Hershberger]

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_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!

Do you think it is valuable enough to apply as is and retrofit later,
or just wait on this series?

Thanks,
-Joe

[U-Boot] [PATCH 11/11] tools: Add tool to add crc8 to a mac address

[Joe Hershberger]

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

[U-Boot] [PATCH 08/11] net: sunxi: Allow sunxi boards to set the MAC from an EEPROM

[Michal Simek]

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_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!
> 
> 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

[U-Boot] [PATCH 08/11] net: sunxi: Allow sunxi boards to set the MAC from an EEPROM

[Olliver Schinagl]

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_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!
>> 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
>
>

[U-Boot] [PATCH 11/11] tools: Add tool to add crc8 to a mac address

[Olliver Schinagl]

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 :)

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Hans de Goede]

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

[U-Boot] [PATCH 11/11] tools: Add tool to add crc8 to a mac address

[Olliver Schinagl]

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

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Olliver Schinagl]

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:
>
> 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
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?
>
> 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
I agree with you here, but I still don't think this should be board specific
>
> 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.
I agree, and I'll fix that in my new patch series proper by making 
rtl8189etv also read rom hook which IS board specific
>
> 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.
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
>

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Hans de Goede]

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:
>>
>> 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
> 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.

>> 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
> 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

>> 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.
> 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.

>> 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.
> 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

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Olliver Schinagl]

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:
>>
>> 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
> 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?
>>
>> 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
> I agree with you here, but I still don't think this should be board
> specific
>>
>> 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.
> 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

>>
>> 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.
> 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
>>
>

-- 
Met vriendelijke groeten, Kind regards, ??????

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Olliver Schinagl]

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:
>>>
>>> 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
>> 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.
>
>>> 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
>> 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
>
>>> 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.
>> 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.
>
>>> 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.
>> 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

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Hans de Goede]

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:
>>>
>>> 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
>> 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?
>>>
>>> 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
>> I agree with you here, but I still don't think this should be board
>> specific
>>>
>>> 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.
>> 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
>
>>>
>>> 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.
>> 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
>>>
>>
>

[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Olliver Schinagl]

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:
>>>>
>>>> 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
>>> 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?
>>>>
>>>> 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
>>> I agree with you here, but I still don't think this should be board
>>> specific
>>>>
>>>> 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.
>>> 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 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,
> 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()

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
>>
>>>>
>>>> 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.
>>> 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
>>>>
>>>
>>

-- 
Met vriendelijke groeten, Kind regards, ??????

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

[U-Boot] [PATCH 01/11] net: dw: Add read_rom_hwaddr net_op hook

[Simon Glass]

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

[U-Boot] [PATCH 01/11] net: dw: Add read_rom_hwaddr net_op hook

[Joe Hershberger]

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

[U-Boot] [PATCH 01/11] net: dw: Add read_rom_hwaddr net_op hook

[Simon Glass]

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

[U-Boot] [PATCH 01/11] net: dw: Add read_rom_hwaddr net_op hook

[Joe Hershberger]

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