[U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[Olliver Schinagl]

This patch-series introduces methods to retrieve the MAC address from an
onboard EEPROM. The series does a few small cleanups at the start, as either
I ran into them while doing this series and fixed them along the way or
actually depended on them. If you want to split the smaller ones off into a
smaller series I understand, but again, I do somewhat depend on them.

A manufacturer wants to produce boards and may even have MAC addresses for
boards. Maintaining unique environments on a per-board basis however is
horrible. Also this data should be very persistent, and not easily deletable
by simply wiping the environment or device tree. Finally there are
chips available on the market with a pre-programmed MAC address chips (proms)
that a board manufacturer wants to use. Because of this, the MAC needs to be
stored be able to read from such an 'external' source.

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

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.

For the added tools, 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. To be able to
replicate these tests the second series (which will be separate from this set)
are needed.

Left TODO:
If U-Boot configures eth0 and eth1 it inserts these values into the environment.
If the fdt then has an ethernet alias for ethernet0, with a MAC address for a
different device, lets say eth2) things will not work at so well.
I guess that leaves some discussion with a separated improvement patch as a
reliable way is needed to match actual u-boot detected devices, with fdt
described devices. E.g. is dev->name the same name to the fdt? Can we blindly
match here?

=======
Changes since v3:
* Split off board specific stuff and only modify the generic functions
* Make reading of an eeprom available to every board. By default this is
  unconfigure and thus should just fall through
* Clean some minor bits up (ARP_HLEN) and use it more generically
* Update the gen_ethaddr_crc as suggested by simon
* Let the fixup_ethernet from fdt_common insert mac addresses to the environment
  for unconfigured devices. There is a small caveat here however as described
  in the TODO above.
* Print the mac address that u-boot assigned to each device.

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/14] net: cosmetic: Do not use magic values for ARP_HLEN

[Olliver Schinagl]

Commit 674bb249825a ("net: cosmetic: Replace magic numbers in arp.c with
constants") introduced a nice define to replace the magic value 6 for
the ethernet hardware address. Replace more hardcoded instances of 6
which really reference the ARP_HLEN (iow the MAC/Hardware/Ethernet
address).

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 common/fdt_support.c |  2 +-
 include/net.h        | 52 +++++++++++++++++++++++++++-------------------------
 net/eth-uclass.c     | 10 +++++-----
 net/eth_legacy.c     | 10 +++++-----
 4 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 0609470..b082662 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -471,7 +471,7 @@ void fdt_fixup_ethernet(void *fdt)
 	char *tmp, *end;
 	char mac[16];
 	const char *path;
-	unsigned char mac_addr[6];
+	unsigned char mac_addr[ARP_HLEN];
 	int offset;
 
 	if (fdt_path_offset(fdt, "/aliases") < 0)
diff --git a/include/net.h b/include/net.h
index 06320c6..8137cf3 100644
--- a/include/net.h
+++ b/include/net.h
@@ -38,6 +38,9 @@
 
 #define PKTALIGN	ARCH_DMA_MINALIGN
 
+/* ARP hardware address length */
+#define ARP_HLEN 6
+
 /* IPv4 addresses are always 32 bits in size */
 struct in_addr {
 	__be32 s_addr;
@@ -90,7 +93,7 @@ enum eth_state_t {
  */
 struct eth_pdata {
 	phys_addr_t iobase;
-	unsigned char enetaddr[6];
+	unsigned char enetaddr[ARP_HLEN];
 	int phy_interface;
 	int max_speed;
 };
@@ -161,7 +164,7 @@ void eth_halt_state_only(void); /* Set passive state */
 #ifndef CONFIG_DM_ETH
 struct eth_device {
 	char name[16];
-	unsigned char enetaddr[6];
+	unsigned char enetaddr[ARP_HLEN];
 	phys_addr_t iobase;
 	int state;
 
@@ -293,9 +296,9 @@ u32 ether_crc(size_t len, unsigned char const *p);
  */
 
 struct ethernet_hdr {
-	u8		et_dest[6];	/* Destination node		*/
-	u8		et_src[6];	/* Source node			*/
-	u16		et_protlen;	/* Protocol or length		*/
+	u8		et_dest[ARP_HLEN];	/* Destination node	*/
+	u8		et_src[ARP_HLEN];	/* Source node		*/
+	u16		et_protlen;		/* Protocol or length	*/
 };
 
 /* Ethernet header size */
@@ -304,16 +307,16 @@ struct ethernet_hdr {
 #define ETH_FCS_LEN	4		/* Octets in the FCS		*/
 
 struct e802_hdr {
-	u8		et_dest[6];	/* Destination node		*/
-	u8		et_src[6];	/* Source node			*/
-	u16		et_protlen;	/* Protocol or length		*/
-	u8		et_dsap;	/* 802 DSAP			*/
-	u8		et_ssap;	/* 802 SSAP			*/
-	u8		et_ctl;		/* 802 control			*/
-	u8		et_snap1;	/* SNAP				*/
+	u8		et_dest[ARP_HLEN];	/* Destination node	*/
+	u8		et_src[ARP_HLEN];	/* Source node		*/
+	u16		et_protlen;		/* Protocol or length	*/
+	u8		et_dsap;		/* 802 DSAP		*/
+	u8		et_ssap;		/* 802 SSAP		*/
+	u8		et_ctl;			/* 802 control		*/
+	u8		et_snap1;		/* SNAP			*/
 	u8		et_snap2;
 	u8		et_snap3;
-	u16		et_prot;	/* 802 protocol			*/
+	u16		et_prot;		/* 802 protocol		*/
 };
 
 /* 802 + SNAP + ethernet header size */
@@ -323,11 +326,11 @@ struct e802_hdr {
  *	Virtual LAN Ethernet header
  */
 struct vlan_ethernet_hdr {
-	u8		vet_dest[6];	/* Destination node		*/
-	u8		vet_src[6];	/* Source node			*/
-	u16		vet_vlan_type;	/* PROT_VLAN			*/
-	u16		vet_tag;	/* TAG of VLAN			*/
-	u16		vet_type;	/* protocol type		*/
+	u8		vet_dest[ARP_HLEN];	/* Destination node	*/
+	u8		vet_src[ARP_HLEN];	/* Source node		*/
+	u16		vet_vlan_type;		/* PROT_VLAN		*/
+	u16		vet_tag;		/* TAG of VLAN		*/
+	u16		vet_type;		/* protocol type	*/
 };
 
 /* VLAN Ethernet header size */
@@ -398,7 +401,6 @@ struct arp_hdr {
 #   define ARP_ETHER	    1		/* Ethernet  hardware address	*/
 	u16		ar_pro;		/* Format of protocol address	*/
 	u8		ar_hln;		/* Length of hardware address	*/
-#   define ARP_HLEN	6
 	u8		ar_pln;		/* Length of protocol address	*/
 #   define ARP_PLEN	4
 	u16		ar_op;		/* Operation			*/
@@ -507,16 +509,16 @@ extern char	net_nis_domain[32];	/* Our IS domain */
 extern char	net_hostname[32];	/* Our hostname */
 extern char	net_root_path[64];	/* Our root path */
 /** END OF BOOTP EXTENTIONS **/
-extern u8		net_ethaddr[6];		/* Our ethernet address */
-extern u8		net_server_ethaddr[6];	/* Boot server enet address */
+extern u8		net_ethaddr[ARP_HLEN];		/* Our ethernet address */
+extern u8		net_server_ethaddr[ARP_HLEN];	/* Boot server enet address */
 extern struct in_addr	net_ip;		/* Our    IP addr (0 = unknown) */
 extern struct in_addr	net_server_ip;	/* Server IP addr (0 = unknown) */
 extern uchar		*net_tx_packet;		/* THE transmit packet */
 extern uchar		*net_rx_packets[PKTBUFSRX]; /* Receive packets */
 extern uchar		*net_rx_packet;		/* Current receive packet */
 extern int		net_rx_packet_len;	/* Current rx packet length */
-extern const u8		net_bcast_ethaddr[6];	/* Ethernet broadcast address */
-extern const u8		net_null_ethaddr[6];
+extern const u8		net_bcast_ethaddr[ARP_HLEN];	/* Ethernet broadcast address */
+extern const u8		net_null_ethaddr[ARP_HLEN];
 
 #define VLAN_NONE	4095			/* untagged */
 #define VLAN_IDMASK	0x0fff			/* mask of valid vlan id */
@@ -555,9 +557,9 @@ extern ushort cdp_appliance_vlan;	/* CDP returned appliance VLAN */
  */
 static inline int is_cdp_packet(const uchar *ethaddr)
 {
-	extern const u8 net_cdp_ethaddr[6];
+	extern const u8 net_cdp_ethaddr[ARP_HLEN];
 
-	return memcmp(ethaddr, net_cdp_ethaddr, 6) == 0;
+	return memcmp(ethaddr, net_cdp_ethaddr, ARP_HLEN) == 0;
 }
 #endif
 
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index a32961e..a9fc6bb 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -230,7 +230,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
 			eth_write_hwaddr(dev);
 			break;
 		case env_op_delete:
-			memset(pdata->enetaddr, 0, 6);
+			memset(pdata->enetaddr, 0, ARP_HLEN);
 		}
 	}
 
@@ -458,7 +458,7 @@ static int eth_post_probe(struct udevice *dev)
 {
 	struct eth_device_priv *priv = dev->uclass_priv;
 	struct eth_pdata *pdata = dev->platdata;
-	unsigned char env_enetaddr[6];
+	unsigned char env_enetaddr[ARP_HLEN];
 
 #if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	struct eth_ops *ops = eth_get_ops(dev);
@@ -497,7 +497,7 @@ static int eth_post_probe(struct udevice *dev)
 	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, 6)) {
+		    memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) {
 			printf("\nWarning: %s MAC addresses don't match:\n",
 			       dev->name);
 			printf("Address in SROM is         %pM\n",
@@ -507,7 +507,7 @@ static int eth_post_probe(struct udevice *dev)
 		}
 
 		/* Override the ROM MAC address */
-		memcpy(pdata->enetaddr, env_enetaddr, 6);
+		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
 	} else if (is_valid_ethaddr(pdata->enetaddr)) {
 		eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
 		printf("\nWarning: %s using MAC address from ROM\n",
@@ -534,7 +534,7 @@ static int eth_pre_remove(struct udevice *dev)
 	eth_get_ops(dev)->stop(dev);
 
 	/* clear the MAC address */
-	memset(pdata->enetaddr, 0, 6);
+	memset(pdata->enetaddr, 0, ARP_HLEN);
 
 	return 0;
 }
diff --git a/net/eth_legacy.c b/net/eth_legacy.c
index d6d7cee..2b2c2de 100644
--- a/net/eth_legacy.c
+++ b/net/eth_legacy.c
@@ -120,7 +120,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
 				eth_parse_enetaddr(value, dev->enetaddr);
 				break;
 			case env_op_delete:
-				memset(dev->enetaddr, 0, 6);
+				memset(dev->enetaddr, 0, ARP_HLEN);
 			}
 		}
 		dev = dev->next;
@@ -133,14 +133,14 @@ U_BOOT_ENV_CALLBACK(ethaddr, on_ethaddr);
 int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 		   int eth_number)
 {
-	unsigned char env_enetaddr[6];
+	unsigned char env_enetaddr[ARP_HLEN];
 	int ret = 0;
 
 	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
 
 	if (!is_zero_ethaddr(env_enetaddr)) {
 		if (!is_zero_ethaddr(dev->enetaddr) &&
-		    memcmp(dev->enetaddr, env_enetaddr, 6)) {
+		    memcmp(dev->enetaddr, env_enetaddr, ARP_HLEN)) {
 			printf("\nWarning: %s MAC addresses don't match:\n",
 			       dev->name);
 			printf("Address in SROM is         %pM\n",
@@ -149,7 +149,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 			       env_enetaddr);
 		}
 
-		memcpy(dev->enetaddr, env_enetaddr, 6);
+		memcpy(dev->enetaddr, env_enetaddr, ARP_HLEN);
 	} else if (is_valid_ethaddr(dev->enetaddr)) {
 		eth_setenv_enetaddr_by_index(base_name, eth_number,
 					     dev->enetaddr);
@@ -298,7 +298,7 @@ int eth_initialize(void)
  */
 int eth_mcast_join(struct in_addr mcast_ip, int join)
 {
-	u8 mcast_mac[6];
+	u8 mcast_mac[ARP_HLEN];
 	if (!eth_current || !eth_current->mcast)
 		return -1;
 	mcast_mac[5] = htonl(mcast_ip.s_addr) & 0xff;
-- 
2.10.2

[U-Boot] [PATCH 02/14] net: cosmetic: Make the MAC address string less magical

[Olliver Schinagl]

In u-boot printf has been extended with the %pM formatter to allow
printing of MAC addresses. However buffers that want to store a MAC
address cannot safely get the size. Add a define for this case so the
string of a MAC address can be reliably obtained.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 include/net.h    | 5 +++++
 net/eth_common.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index 8137cf3..af0558d 100644
--- a/include/net.h
+++ b/include/net.h
@@ -40,6 +40,11 @@
 
 /* ARP hardware address length */
 #define ARP_HLEN 6
+/*
+ * The size of a MAC address in string form, each digit requires two chars
+ * and five separator characters to form '00:00:00:00:00:00'.
+ */
+#define ARP_HLEN_ASCII (ARP_HLEN * 2) + (ARP_HLEN - 1)
 
 /* IPv4 addresses are always 32 bits in size */
 struct in_addr {
diff --git a/net/eth_common.c b/net/eth_common.c
index 2880901..e9d3c66 100644
--- a/net/eth_common.c
+++ b/net/eth_common.c
@@ -32,7 +32,7 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr)
 
 int eth_setenv_enetaddr(const char *name, const uchar *enetaddr)
 {
-	char buf[20];
+	char buf[ARP_HLEN_ASCII + 1];
 
 	sprintf(buf, "%pM", enetaddr);
 
-- 
2.10.2

[U-Boot] [PATCH 03/14] net: cosmetic: Define ethernet name length

[Olliver Schinagl]

There are various places where the ethernet device name is defined to
several different sizes. Lets add a define and start using it.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 include/net.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index af0558d..9cd7870 100644
--- a/include/net.h
+++ b/include/net.h
@@ -168,7 +168,8 @@ void eth_halt_state_only(void); /* Set passive state */
 
 #ifndef CONFIG_DM_ETH
 struct eth_device {
-	char name[16];
+#define ETH_NAME_LEN 16
+	char name[ETH_NAME_LEN];
 	unsigned char enetaddr[ARP_HLEN];
 	phys_addr_t iobase;
 	int state;
-- 
2.10.2

[U-Boot] [PATCH 04/14] net: core: Sanitize get/set operations for enetaddr

[Olliver Schinagl]

In the current net stack, we have a few functions to get and set
the "ethaddr" and "ethNaddr" environment variables, which use magic
values to get and set these environment variables. Remove the magicness
of the buffer by defining it proper and also check the input for its
length.

Additionally use the define in fdt parser where the ethaddr variables
are also used.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 common/fdt_support.c |  2 +-
 include/net.h        |  1 +
 net/eth_common.c     | 12 ++++++++----
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index b082662..89e6e47 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -469,7 +469,7 @@ void fdt_fixup_ethernet(void *fdt)
 {
 	int i, j, prop;
 	char *tmp, *end;
-	char mac[16];
+	char mac[ETH_ENETADDR_LEN];
 	const char *path;
 	unsigned char mac_addr[ARP_HLEN];
 	int offset;
diff --git a/include/net.h b/include/net.h
index 9cd7870..2534913 100644
--- a/include/net.h
+++ b/include/net.h
@@ -243,6 +243,7 @@ void eth_set_current(void);		/* set nterface to ethcur var */
 
 int eth_get_dev_index(void);		/* get the device index */
 void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
+#define ETH_ENETADDR_LEN 32
 int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
 int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
 
diff --git a/net/eth_common.c b/net/eth_common.c
index e9d3c66..079be89 100644
--- a/net/eth_common.c
+++ b/net/eth_common.c
@@ -42,16 +42,20 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr)
 int eth_getenv_enetaddr_by_index(const char *base_name, int index,
 				 uchar *enetaddr)
 {
-	char enetvar[32];
-	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+	char enetvar[ETH_ENETADDR_LEN];
+
+	snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr",
+		 base_name, index);
 	return eth_getenv_enetaddr(enetvar, enetaddr);
 }
 
 int eth_setenv_enetaddr_by_index(const char *base_name, int index,
 				 uchar *enetaddr)
 {
-	char enetvar[32];
-	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+	char enetvar[ETH_ENETADDR_LEN];
+
+	snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr",
+		 base_name, index);
 	return eth_setenv_enetaddr(enetvar, enetaddr);
 }
 
-- 
2.10.2

[U-Boot] [PATCH 05/14] net: core: cosmetic: A MAC address is not limited to SROM

[Olliver Schinagl]

Currently, we print that the MAC from the SROM does not match. It can be
many forms of ROM, so lets drop the S.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 net/eth-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index a9fc6bb..9703bea 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -500,7 +500,7 @@ static int eth_post_probe(struct udevice *dev)
 		    memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) {
 			printf("\nWarning: %s MAC addresses don't match:\n",
 			       dev->name);
-			printf("Address in SROM is         %pM\n",
+			printf("Address in ROM is          %pM\n",
 			       pdata->enetaddr);
 			printf("Address in environment is  %pM\n",
 			       env_enetaddr);
-- 
2.10.2

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Olliver Schinagl]

Currently we print a warning if the MAC address is read from
ROM/Hardware. This should not be concidered a bad or erronous thing. A
MAC address should come either from the hardware (ROM) or may be
set/overriden in the environment. Neither is a warning or error case.

Worthy of a warning is the case where both are set, so the user is
atleast aware something special is happening.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 net/eth-uclass.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 9703bea..aca3f6d 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
 		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
 	} else if (is_valid_ethaddr(pdata->enetaddr)) {
 		eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
-		printf("\nWarning: %s using MAC address from ROM\n",
-		       dev->name);
 	} else if (is_zero_ethaddr(pdata->enetaddr)) {
 #ifdef CONFIG_NET_RANDOM_ETHADDR
 		net_random_ethaddr(pdata->enetaddr);
-- 
2.10.2

[U-Boot] [PATCH 07/14] net: core: Inform the user of the device MAC address

[Olliver Schinagl]

In certain conditions we currently print the MAC address. For example a
warning when a random mac address is in use or a missmatch between HW
and ENV.

If all things went well however (but even if there is a miss-match) we
do not inform the user what the final MAC address of the device is.

Lets print the final MAC address of the device with which it has been
setup.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 net/eth-uclass.c | 9 ++++++---
 net/eth_legacy.c | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index aca3f6d..5c888b8 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -413,11 +413,12 @@ int eth_initialize(void)
 		}
 
 		bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT);
+		putc('\n');
 		do {
-			if (num_devices)
-				printf(", ");
+			struct eth_pdata *pdata = dev->platdata;
 
-			printf("eth%d: %s", dev->seq, dev->name);
+			printf("eth%d:  %s [%pM]\n", dev->seq, dev->name,
+						     pdata->enetaddr);
 
 			if (ethprime && dev == prime_dev)
 				printf(" [PRIME]");
@@ -522,6 +523,8 @@ static int eth_post_probe(struct udevice *dev)
 #endif
 	}
 
+	printf("%s ", dev->name);
+
 	return 0;
 }
 
diff --git a/net/eth_legacy.c b/net/eth_legacy.c
index 2b2c2de..bf4de37 100644
--- a/net/eth_legacy.c
+++ b/net/eth_legacy.c
@@ -178,6 +178,9 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 			       dev->name);
 	}
 
+	printf("%s (eth%d) has MAC address: %pM\n",
+	       dev->name, eth_number, dev->enetaddr);
+
 	return ret;
 }
 
-- 
2.10.2

[U-Boot] [PATCH 08/14] net: core: Add function to check/set MAC locality

[Olliver Schinagl]

Universally administered and locally administered addresses are
distinguished by setting the second-least-significant bit of the first
octet of the address. Having a function to check and set this U/L bit
from a function makes it nice for boards that want to generate their own
mac address to ensure they are locally administered.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 include/net.h | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index 2534913..08f8af8 100644
--- a/include/net.h
+++ b/include/net.h
@@ -772,6 +772,28 @@ static inline int is_multicast_ethaddr(const u8 *addr)
 	return 0x01 & addr[0];
 }
 
+/**
+ * is_local_ethaddr - Determine if the Ethernet address is a locally
+ *		      administered MAC address.
+ * @addr:	Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return true if the address is a locally administered address.
+ */
+static inline int is_local_ethaddr(const u8 *addr)
+{
+	return 0x02 & addr[0];
+}
+
+/**
+ * set_local_ethaddr - Make the supplied Ethernet address a locally
+ * 		       administered one.
+ * @addr:	Pointer to a six-byte array containing the Ethernet address
+ */
+static inline void set_local_ethaddr(u8 *addr)
+{
+	addr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
+}
+
 /*
  * is_broadcast_ethaddr - Determine if the Ethernet address is broadcast
  * @addr: Pointer to a six-byte array containing the Ethernet address
@@ -816,7 +838,7 @@ static inline void net_random_ethaddr(uchar *addr)
 		addr[i] = rand_r(&seed);
 
 	addr[0] &= 0xfe;	/* clear multicast bit */
-	addr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
+	set_local_ethaddr(addr);
 }
 
 /* Convert an IP address to a string */
-- 
2.10.2

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Olliver Schinagl]

This patch allows Kconfig to enable and set parameters to make it
possible to read the MAC address from an EEPROM. The net core layer then
uses this information to read MAC addresses from this EEPROM.

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

The code currently first checks if there is a non-zero MAC address in
the eeprom. If that fails to be the case, the read_rom_hwaddr can be
used by a board to supply the MAC in other ways.

If both these fails, the other code is still in place to query the
environent, which then can be used to override the hardware supplied
data.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/net.h       | 14 ++++++++
 net/Kconfig         | 59 +++++++++++++++++++++++++++++++
 net/eth-uclass.c    |  9 +++--
 net/eth_common.c    | 34 ++++++++++++++++++
 net/eth_legacy.c    |  2 ++
 6 files changed, 214 insertions(+), 3 deletions(-)

diff --git a/doc/README.enetaddr b/doc/README.enetaddr
index 50e4899..89c1f7d 100644
--- a/doc/README.enetaddr
+++ b/doc/README.enetaddr
@@ -47,6 +47,105 @@ 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
+--------
+
+Boards may come with an EEPROM specifically to store configuration bits, such
+as a MAC address. 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 can be used 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...|
+
+This can be done from linux using the i2c-tools:
+
+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
+
+Alternativly this can be done from the u-boot console as:
+
+u-boot> mm.b 0
+00000000: 00 ? 01
+00000001: 23 ? 23
+00000002: 45 ? 45
+00000003: 67 ? 67
+00000004: 89 ? 89
+00000005: ab ? ab
+00000006: be ? be
+00000007: 00 ? q
+i2c dev I2CBUS
+i2c write 0 50 8 7
+i2c md 50 8
+
 -------
  Usage
 -------
diff --git a/include/net.h b/include/net.h
index 08f8af8..e50ab5d 100644
--- a/include/net.h
+++ b/include/net.h
@@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
 int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
 
 /**
+ * eeprom_read_enetaddr() - Read the hardware address from an eeprom
+ *
+ * This function tries to read the MAC address from an eeprom as can be read
+ * in docs/README.enetaddr.
+ *
+ * @index:	index of the interface to get the hwaddr for
+ * @enetaddr:	pointer for the found hwaddr. Needs to be atleast ARP_HLEN
+ * @return:	0 on success, non-zero is error status. Additionally hwaddr
+ *		is set to 00:00:00:00:00. This is also the case if
+ *		CONFIG_NET_ETHADDR_EEPROM is not set.
+ */
+int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
+
+/**
  * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
  *
  * This sets up an environment variable with the given MAC address (@enetaddr).
diff --git a/net/Kconfig b/net/Kconfig
index 414c549..f699e1c 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -7,6 +7,65 @@ 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.
+	  Remember to also make the selected bus available via I2Cn_ENABLE.
+
+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
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 5c888b8..a154a7e 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -491,9 +491,12 @@ static int eth_post_probe(struct udevice *dev)
 
 	priv->state = ETH_STATE_INIT;
 
-	/* Check if the device has a MAC address in ROM */
-	if (eth_get_ops(dev)->read_rom_hwaddr)
-		eth_get_ops(dev)->read_rom_hwaddr(dev);
+	/* Check if the device has a MAC address in EEPROM */
+	eeprom_read_enetaddr(dev->seq, pdata->enetaddr);
+	if (is_zero_ethaddr(pdata->enetaddr))
+		/* Check if the device has a MAC address in ROM */
+		if (eth_get_ops(dev)->read_rom_hwaddr)
+			eth_get_ops(dev)->read_rom_hwaddr(dev);
 
 	eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
 	if (!is_zero_ethaddr(env_enetaddr)) {
diff --git a/net/eth_common.c b/net/eth_common.c
index 079be89..e0d8b62 100644
--- a/net/eth_common.c
+++ b/net/eth_common.c
@@ -8,10 +8,44 @@
 
 #include <common.h>
 #include <dm.h>
+#include <i2c.h>
 #include <miiphy.h>
 #include <net.h>
 #include "eth_internal.h"
 
+int eeprom_read_enetaddr(const int index, unsigned char *enetaddr)
+{
+	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 from the eeprom */
+	if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
+		     CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index * (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 or EEPROM missing on device: ");
+		return -ENOSYS;
+	}
+	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 on device: ");
+		return -EINVAL;
+	}
+#endif
+#endif
+
+	memcpy(enetaddr, eeprom, ARP_HLEN);
+
+	return 0;
+}
+
 void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
 {
 	char *end;
diff --git a/net/eth_legacy.c b/net/eth_legacy.c
index bf4de37..8fb5844 100644
--- a/net/eth_legacy.c
+++ b/net/eth_legacy.c
@@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 	unsigned char env_enetaddr[ARP_HLEN];
 	int ret = 0;
 
+	eeprom_read_enetaddr(eth_number, dev->enetaddr);
+
 	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
 
 	if (!is_zero_ethaddr(env_enetaddr)) {
-- 
2.10.2

[U-Boot] [PATCH 10/14] fdt: fixup_eth: Remove code duplication with a function

[Olliver Schinagl]

In fdt_support.c we use a loop to parse the mac address string from the
fdt blob, net.h has a function for this however, so lets use it.

Also, rename the variable from tmp to something more descriptive.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 common/fdt_support.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 89e6e47..20a3dd9 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -467,8 +467,8 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
 
 void fdt_fixup_ethernet(void *fdt)
 {
-	int i, j, prop;
-	char *tmp, *end;
+	int i, prop;
+	char *fdt_eth_addr;
 	char mac[ETH_ENETADDR_LEN];
 	const char *path;
 	unsigned char mac_addr[ARP_HLEN];
@@ -503,16 +503,11 @@ void fdt_fixup_ethernet(void *fdt)
 			} else {
 				continue;
 			}
-			tmp = getenv(mac);
-			if (!tmp)
+			fdt_eth_addr = getenv(mac);
+			if (!fdt_eth_addr)
 				continue;
 
-			for (j = 0; j < 6; j++) {
-				mac_addr[j] = tmp ?
-					      simple_strtoul(tmp, &end, 16) : 0;
-				if (tmp)
-					tmp = (*end) ? end + 1 : end;
-			}
+			eth_parse_enetaddr(fdt_eth_addr, mac_addr);
 
 			do_fixup_by_path(fdt, path, "mac-address",
 					 &mac_addr, 6, 0);
-- 
2.10.2

[U-Boot] [PATCH 11/14] fdt: eth_fixup: improve error catching/reduce identation

[Olliver Schinagl]

Currently when checking for an error in ethernet aliases in the fdt, we
only check for the error case -1. It is safer to ignore anything < 0.

By rearranging logic a bit we can now also reduce identation.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 common/fdt_support.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 20a3dd9..c34a13c 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -495,20 +495,20 @@ void fdt_fixup_ethernet(void *fdt)
 		path = fdt_getprop_by_offset(fdt, offset, &name, NULL);
 		if (!strncmp(name, "ethernet", len)) {
 			i = trailing_strtol(name);
-			if (i != -1) {
-				if (i == 0)
-					strcpy(mac, "ethaddr");
-				else
-					sprintf(mac, "eth%daddr", i);
-			} else {
+			if (i < 0)
 				continue;
-			}
+
+			if (i == 0)
+				strcpy(mac, "ethaddr");
+			else
+				sprintf(mac, "eth%daddr", i);
+
 			fdt_eth_addr = getenv(mac);
-			if (!fdt_eth_addr)
+			if (fdt_eth_addr)
+				eth_parse_enetaddr(fdt_eth_addr, mac_addr);
+			else
 				continue;
 
-			eth_parse_enetaddr(fdt_eth_addr, mac_addr);
-
 			do_fixup_by_path(fdt, path, "mac-address",
 					 &mac_addr, 6, 0);
 			do_fixup_by_path(fdt, path, "local-mac-address",
-- 
2.10.2

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

This patch adds a method for the board to set the MAC address if the
environment is not yet set. The environment based MAC addresses are not
touched, but if the fdt has an alias set, it is parsed and put into the
environment.

E.g. The environment contains ethaddr and eth1addr, and the fdt contains
an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
is parsed and inserted into the environment as eth2addr.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 common/fdt_support.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index c34a13c..f127392 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
 	return fdt_fixup_memory_banks(blob, &start, &size, 1);
 }
 
+__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
+{
+	return -ENOSYS;
+}
+
 void fdt_fixup_ethernet(void *fdt)
 {
 	int i, prop;
@@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
 			if (fdt_eth_addr)
 				eth_parse_enetaddr(fdt_eth_addr, mac_addr);
 			else
-				continue;
+				if (board_get_enetaddr(i, mac_addr) < 0)
+					continue;
 
 			do_fixup_by_path(fdt, path, "mac-address",
 					 &mac_addr, 6, 0);
-- 
2.10.2

[U-Boot] [PATCH 13/14] 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>
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 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 14/14] 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>
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 tools/.gitignore        |  1 +
 tools/Makefile          |  4 +++
 tools/gen_ethaddr_crc.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 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..fe9896d
--- /dev/null
+++ b/tools/gen_ethaddr_crc.c
@@ -0,0 +1,75 @@
+/*
+ * (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 */
+#define ARP_HLEN_ASCII (ARP_HLEN * 2) + (ARP_HLEN - 1) /* with separators */
+#define ARP_HLEN_LAZY (ARP_HLEN * 2) /* separatorless hardware address length */
+
+uint8_t nibble_to_hex(const char *nibble, bool lo)
+{
+	return (strtol(nibble, NULL, 16) << (lo ? 0 : 4)) & (lo ? 0x0f : 0xf0);
+}
+
+int process_mac(const char *mac_address)
+{
+	uint8_t ethaddr[ARP_HLEN + 1] = { 0x00 };
+	uint_fast8_t i = 0;
+
+	while (*mac_address != '\0') {
+		char nibble[2] = { 0x00, '\n' }; /* for strtol */
+
+		nibble[0] = *mac_address++;
+		if (isxdigit(nibble[0])) {
+			if (isupper(nibble[0]))
+				nibble[0] = tolower(nibble[0]);
+			ethaddr[i >> 1] |= nibble_to_hex(nibble, (i % 2) != 0);
+			i++;
+		}
+	}
+
+	for (i = 0; i < ARP_HLEN; i++)
+		printf("%.2x", ethaddr[i]);
+	printf("%.2x\n", crc8(0, ethaddr, ARP_HLEN));
+
+	return 0;
+}
+
+void print_usage(char *cmdname)
+{
+	printf("Usage: %s <mac_address>\n", cmdname);
+	puts("<mac_address> may be with or without separators.");
+	puts("Valid seperators are ':' and '-'.");
+	puts("<mac_address> digits are in base 16.\n");
+}
+
+int main(int argc, char *argv[])
+{
+	if (argc < 2) {
+		print_usage(argv[0]);
+		return 1;
+	}
+
+	if (!((strlen(argv[1]) == ARP_HLEN_ASCII) || (strlen(argv[1]) == ARP_HLEN_LAZY))) {
+		puts("The MAC address is not valid.\n");
+		print_usage(argv[0]);
+		return 1;
+	}
+
+	if (process_mac(argv[1])) {
+		puts("Failed to calculate the MAC's checksum.");
+		return 1;
+	}
+
+	return 0;
+}
-- 
2.10.2

[U-Boot] [PATCH 00/14] [PATCHv4] Retrieve MAC address from EEPROM

[Olliver Schinagl]

This patch-series introduces methods to retrieve the MAC address from an
onboard EEPROM. The series does a few small cleanups at the start, as either
I ran into them while doing this series and fixed them along the way or
actually depended on them. If you want to split the smaller ones off into a
smaller series I understand, but again, I do somewhat depend on them.

A manufacturer wants to produce boards and may even have MAC addresses for
boards. Maintaining unique environments on a per-board basis however is
horrible. Also this data should be very persistent, and not easily deletable
by simply wiping the environment or device tree. Finally there are
chips available on the market with a pre-programmed MAC address chips 
(proms)
that a board manufacturer wants to use. Because of this, the MAC needs to be
stored be able to read from such an 'external' source.

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

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.

For the added tools, 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. To be 
able to
replicate these tests the second series (which will be separate from 
this set)
are needed.

Left TODO:
If U-Boot configures eth0 and eth1 it inserts these values into the 
environment.
If the fdt then has an ethernet alias for ethernet0, with a MAC address 
for a
different device, lets say eth2) things will not work at so well.
I guess that leaves some discussion with a separated improvement patch as a
reliable way is needed to match actual u-boot detected devices, with fdt
described devices. E.g. is dev->name the same name to the fdt? Can we 
blindly
match here?

=======
Changes since v3:
* Split off board specific stuff and only modify the generic functions
* Make reading of an eeprom available to every board. By default this is
   unconfigure and thus should just fall through
* Clean some minor bits up (ARP_HLEN) and use it more generically
* Update the gen_ethaddr_crc as suggested by simon
* Let the fixup_ethernet from fdt_common insert mac addresses to the 
environment
   for unconfigured devices. There is a small caveat here however as 
described
   in the TODO above.
* Print the mac address that u-boot assigned to each device.

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 06/14] net: core: Using an ethernet address from ROM is not bad

[Michal Simek]

On 25.11.2016 16:30, Olliver Schinagl wrote:
> Currently we print a warning if the MAC address is read from
> ROM/Hardware. This should not be concidered a bad or erronous thing. A
> MAC address should come either from the hardware (ROM) or may be
> set/overriden in the environment. Neither is a warning or error case.
> 
> Worthy of a warning is the case where both are set, so the user is
> atleast aware something special is happening.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  net/eth-uclass.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 9703bea..aca3f6d 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>  		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>  	} else if (is_valid_ethaddr(pdata->enetaddr)) {
>  		eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
> -		printf("\nWarning: %s using MAC address from ROM\n",
> -		       dev->name);
>  	} else if (is_zero_ethaddr(pdata->enetaddr)) {
>  #ifdef CONFIG_NET_RANDOM_ETHADDR
>  		net_random_ethaddr(pdata->enetaddr);
> 

User should be aware if mac is read from ROM or something else.
Is there a way how to read it without this message to be generated?

Thanks,
Michal

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Michal Simek]

On 25.11.2016 16:30, Olliver Schinagl wrote:
> This patch allows Kconfig to enable and set parameters to make it
> possible to read the MAC address from an EEPROM. The net core layer then
> uses this information to read MAC addresses from this EEPROM.
> 
> 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 and added.
> 
> The code currently first checks if there is a non-zero MAC address in
> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
> used by a board to supply the MAC in other ways.
> 
> If both these fails, the other code is still in place to query the
> environent, which then can be used to override the hardware supplied
> data.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/net.h       | 14 ++++++++
>  net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>  net/eth-uclass.c    |  9 +++--
>  net/eth_common.c    | 34 ++++++++++++++++++
>  net/eth_legacy.c    |  2 ++
>  6 files changed, 214 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index 50e4899..89c1f7d 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -47,6 +47,105 @@ 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
> +--------
> +
> +Boards may come with an EEPROM specifically to store configuration bits, such
> +as a MAC address. 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 can be used 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...|
> +
> +This can be done from linux using the i2c-tools:
> +
> +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
> +
> +Alternativly this can be done from the u-boot console as:
> +
> +u-boot> mm.b 0
> +00000000: 00 ? 01
> +00000001: 23 ? 23
> +00000002: 45 ? 45
> +00000003: 67 ? 67
> +00000004: 89 ? 89
> +00000005: ab ? ab
> +00000006: be ? be
> +00000007: 00 ? q
> +i2c dev I2CBUS
> +i2c write 0 50 8 7
> +i2c md 50 8
> +
>  -------
>   Usage
>  -------
> diff --git a/include/net.h b/include/net.h
> index 08f8af8..e50ab5d 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>  int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>  
>  /**
> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
> + *
> + * This function tries to read the MAC address from an eeprom as can be read
> + * in docs/README.enetaddr.
> + *
> + * @index:	index of the interface to get the hwaddr for
> + * @enetaddr:	pointer for the found hwaddr. Needs to be atleast ARP_HLEN
> + * @return:	0 on success, non-zero is error status. Additionally hwaddr
> + *		is set to 00:00:00:00:00. This is also the case if
> + *		CONFIG_NET_ETHADDR_EEPROM is not set.
> + */
> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
> +
> +/**
>   * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>   *
>   * This sets up an environment variable with the given MAC address (@enetaddr).
> diff --git a/net/Kconfig b/net/Kconfig
> index 414c549..f699e1c 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -7,6 +7,65 @@ 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.
> +	  Remember to also make the selected bus available via I2Cn_ENABLE.
> +
> +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.

I would prefer to all these values to be in hex because i2c commands are
also taking values in hex.

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

Would it be possible to have default n here?
I would guess that more boards don't have this CRC8 sums.

Thanks,
Michal

[U-Boot] [PATCH 00/14] [PATCHv4] Retrieve MAC address from EEPROM

[Michal Simek]

Hi,

On 25.11.2016 16:43, Olliver Schinagl wrote:
> This patch-series introduces methods to retrieve the MAC address from an
> onboard EEPROM. The series does a few small cleanups at the start, as
> either
> I ran into them while doing this series and fixed them along the way or
> actually depended on them. If you want to split the smaller ones off into a
> smaller series I understand, but again, I do somewhat depend on them.
> 
> A manufacturer wants to produce boards and may even have MAC addresses for
> boards. Maintaining unique environments on a per-board basis however is
> horrible. Also this data should be very persistent, and not easily
> deletable
> by simply wiping the environment or device tree. Finally there are
> chips available on the market with a pre-programmed MAC address chips
> (proms)
> that a board manufacturer wants to use. Because of this, the MAC needs
> to be
> stored be able to read from such an 'external' source.
> 
> 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
> address.
> 
> 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.
> 
> For the added tools, 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. To be
> able to
> replicate these tests the second series (which will be separate from
> this set)
> are needed.
> 
> Left TODO:
> If U-Boot configures eth0 and eth1 it inserts these values into the
> environment.
> If the fdt then has an ethernet alias for ethernet0, with a MAC address
> for a
> different device, lets say eth2) things will not work at so well.
> I guess that leaves some discussion with a separated improvement patch as a
> reliable way is needed to match actual u-boot detected devices, with fdt
> described devices. E.g. is dev->name the same name to the fdt? Can we
> blindly
> match here?
> 
> =======
> Changes since v3:
> * Split off board specific stuff and only modify the generic functions
> * Make reading of an eeprom available to every board. By default this is
>   unconfigure and thus should just fall through
> * Clean some minor bits up (ARP_HLEN) and use it more generically
> * Update the gen_ethaddr_crc as suggested by simon
> * Let the fixup_ethernet from fdt_common insert mac addresses to the
> environment
>   for unconfigured devices. There is a small caveat here however as
> described
>   in the TODO above.
> * Print the mac address that u-boot assigned to each device.
> 
> 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.

I have tested this on zcu102 with mac in eeprom and it is working fine
with one mac. I expect you have tested it with more and code looks good
in this sense.
I would personally prefer to know where that mac address is coming from
in output.

Thanks,
Michal

[U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[Igor Grinberg]

Hi Olliver,

On 11/25/16 17:30, Olliver Schinagl wrote:

[...]

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

While reading the above, I'm wondering, have you considered the eeprom layout
feature that we have in: common/eeprom/... ?

The layout feature was actually designed for these tasks, but in a more generic
way then just Ethernet MAC address.

What do you think?


-- 
Regards,
Igor.

[U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[Olliver Schinagl]

On 28-11-16 10:13, Igor Grinberg wrote:
> Hi Olliver,
>
> On 11/25/16 17:30, Olliver Schinagl wrote:
>
> [...]
>
>> 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 address.
>>
>> 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.
> While reading the above, I'm wondering, have you considered the eeprom layout
> feature that we have in: common/eeprom/... ?
Last year, when starting this patch series, this did not really exist in 
so far (iirc).
>
> The layout feature was actually designed for these tasks, but in a more generic
> way then just Ethernet MAC address.
I did see it and I was quite excited. I think a follow up patch should 
switch over. I did not yet use the new eeprom layout thing as I am 
hoping for Maxime's patches to land first, where he makes the whole 
eeprom interfacing more generic.
>
> What do you think?
I'll have to look at the eeprom layout feature a little more in depth, 
the one thing that was a little 'annoying' (from a short quick glance) 
was that the layout was jumping around a bit (eth0, eth1, something 
else, eth2, eth3). But yes, I was quite intrigued herein.

Olliver

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Olliver Schinagl]

On 28-11-16 08:59, Michal Simek wrote:
> On 25.11.2016 16:30, Olliver Schinagl wrote:
>> Currently we print a warning if the MAC address is read from
>> ROM/Hardware. This should not be concidered a bad or erronous thing. A
>> MAC address should come either from the hardware (ROM) or may be
>> set/overriden in the environment. Neither is a warning or error case.
>>
>> Worthy of a warning is the case where both are set, so the user is
>> atleast aware something special is happening.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   net/eth-uclass.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index 9703bea..aca3f6d 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>>   		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>>   	} else if (is_valid_ethaddr(pdata->enetaddr)) {
>>   		eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
>> -		printf("\nWarning: %s using MAC address from ROM\n",
>> -		       dev->name);
>>   	} else if (is_zero_ethaddr(pdata->enetaddr)) {
>>   #ifdef CONFIG_NET_RANDOM_ETHADDR
>>   		net_random_ethaddr(pdata->enetaddr);
>>
> User should be aware if mac is read from ROM or something else.
> Is there a way how to read it without this message to be generated?
I think what we need is a 'source' field here to be printed at the end. 
I do agree the user will want to know WHERE the address came from (and 
what it is). I do think the warning is misplaced here however. There's 
nothing to be warned against.

I'll look into adding the feature that prints the source at the end (as 
detailed as we can).
>
> Thanks,
> Michal
>

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Michal Simek]

On 28.11.2016 11:48, Olliver Schinagl wrote:
> On 28-11-16 08:59, Michal Simek wrote:
>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>> Currently we print a warning if the MAC address is read from
>>> ROM/Hardware. This should not be concidered a bad or erronous thing. A
>>> MAC address should come either from the hardware (ROM) or may be
>>> set/overriden in the environment. Neither is a warning or error case.
>>>
>>> Worthy of a warning is the case where both are set, so the user is
>>> atleast aware something special is happening.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   net/eth-uclass.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>> index 9703bea..aca3f6d 100644
>>> --- a/net/eth-uclass.c
>>> +++ b/net/eth-uclass.c
>>> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>>>           memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>>>       } else if (is_valid_ethaddr(pdata->enetaddr)) {
>>>           eth_setenv_enetaddr_by_index("eth", dev->seq,
>>> pdata->enetaddr);
>>> -        printf("\nWarning: %s using MAC address from ROM\n",
>>> -               dev->name);
>>>       } else if (is_zero_ethaddr(pdata->enetaddr)) {
>>>   #ifdef CONFIG_NET_RANDOM_ETHADDR
>>>           net_random_ethaddr(pdata->enetaddr);
>>>
>> User should be aware if mac is read from ROM or something else.
>> Is there a way how to read it without this message to be generated?
> I think what we need is a 'source' field here to be printed at the end.
> I do agree the user will want to know WHERE the address came from (and
> what it is). I do think the warning is misplaced here however. There's
> nothing to be warned against.
> 
> I'll look into adding the feature that prints the source at the end (as
> detailed as we can).

Maybe enough here to remove that Warning from print message.

Thanks,
Michal

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Olliver Schinagl]

On November 28, 2016 12:06:37 PM CET, Michal Simek <michal.simek@xilinx.com> wrote:
>On 28.11.2016 11:48, Olliver Schinagl wrote:
>> On 28-11-16 08:59, Michal Simek wrote:
>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>> Currently we print a warning if the MAC address is read from
>>>> ROM/Hardware. This should not be concidered a bad or erronous
>thing. A
>>>> MAC address should come either from the hardware (ROM) or may be
>>>> set/overriden in the environment. Neither is a warning or error
>case.
>>>>
>>>> Worthy of a warning is the case where both are set, so the user is
>>>> atleast aware something special is happening.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>> ---
>>>>   net/eth-uclass.c | 2 --
>>>>   1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>>> index 9703bea..aca3f6d 100644
>>>> --- a/net/eth-uclass.c
>>>> +++ b/net/eth-uclass.c
>>>> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>>>>           memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>>>>       } else if (is_valid_ethaddr(pdata->enetaddr)) {
>>>>           eth_setenv_enetaddr_by_index("eth", dev->seq,
>>>> pdata->enetaddr);
>>>> -        printf("\nWarning: %s using MAC address from ROM\n",
>>>> -               dev->name);
>>>>       } else if (is_zero_ethaddr(pdata->enetaddr)) {
>>>>   #ifdef CONFIG_NET_RANDOM_ETHADDR
>>>>           net_random_ethaddr(pdata->enetaddr);
>>>>
>>> User should be aware if mac is read from ROM or something else.
>>> Is there a way how to read it without this message to be generated?
>> I think what we need is a 'source' field here to be printed at the
>end.
>> I do agree the user will want to know WHERE the address came from
>(and
>> what it is). I do think the warning is misplaced here however.
>There's
>> nothing to be warned against.
>> 
>> I'll look into adding the feature that prints the source at the end
>(as
>> detailed as we can).
>
>Maybe enough here to remove that Warning from print message.
Well a later patch in this series does print it per interface.

Hence why my suggestion to add the source/from.

Olliver

>
>Thanks,
>Michal

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Michal Simek]

On 28.11.2016 12:08, Olliver Schinagl wrote:
> 
> 
> On November 28, 2016 12:06:37 PM CET, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 28.11.2016 11:48, Olliver Schinagl wrote:
>>> On 28-11-16 08:59, Michal Simek wrote:
>>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>>> Currently we print a warning if the MAC address is read from
>>>>> ROM/Hardware. This should not be concidered a bad or erronous
>> thing. A
>>>>> MAC address should come either from the hardware (ROM) or may be
>>>>> set/overriden in the environment. Neither is a warning or error
>> case.
>>>>>
>>>>> Worthy of a warning is the case where both are set, so the user is
>>>>> atleast aware something special is happening.
>>>>>
>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>> ---
>>>>>   net/eth-uclass.c | 2 --
>>>>>   1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>>>> index 9703bea..aca3f6d 100644
>>>>> --- a/net/eth-uclass.c
>>>>> +++ b/net/eth-uclass.c
>>>>> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>>>>>           memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>>>>>       } else if (is_valid_ethaddr(pdata->enetaddr)) {
>>>>>           eth_setenv_enetaddr_by_index("eth", dev->seq,
>>>>> pdata->enetaddr);
>>>>> -        printf("\nWarning: %s using MAC address from ROM\n",
>>>>> -               dev->name);
>>>>>       } else if (is_zero_ethaddr(pdata->enetaddr)) {
>>>>>   #ifdef CONFIG_NET_RANDOM_ETHADDR
>>>>>           net_random_ethaddr(pdata->enetaddr);
>>>>>
>>>> User should be aware if mac is read from ROM or something else.
>>>> Is there a way how to read it without this message to be generated?
>>> I think what we need is a 'source' field here to be printed at the
>> end.
>>> I do agree the user will want to know WHERE the address came from
>> (and
>>> what it is). I do think the warning is misplaced here however.
>> There's
>>> nothing to be warned against.
>>>
>>> I'll look into adding the feature that prints the source at the end
>> (as
>>> detailed as we can).
>>
>> Maybe enough here to remove that Warning from print message.
> Well a later patch in this series does print it per interface.
> 
> Hence why my suggestion to add the source/from.

Definitely nice prints and if you add source/from part I will be happy
with it.

Thanks,
Michal

[U-Boot] [PATCHv4] Retrieve MAC address from EEPROM

[Igor Grinberg]

Added Nikita to Cc.

On 11/28/16 12:45, Olliver Schinagl wrote:
> On 28-11-16 10:13, Igor Grinberg wrote:
>> Hi Olliver,
>>
>> On 11/25/16 17:30, Olliver Schinagl wrote:
>>
>> [...]
>>
>>> 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 address.
>>>
>>> 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.
>> While reading the above, I'm wondering, have you considered the eeprom layout
>> feature that we have in: common/eeprom/... ?
> Last year, when starting this patch series, this did not really exist in so far (iirc).
>>
>> The layout feature was actually designed for these tasks, but in a more generic
>> way then just Ethernet MAC address.
> I did see it and I was quite excited. I think a follow up patch should switch over.

Sounds great!

> I did not yet use the new eeprom layout thing as I am hoping for Maxime's patches to
> land first, where he makes the whole eeprom interfacing more generic.

That's interesting. I haven't been around for some time, are there any public patches
already? If so, can you please point where should I look at?

>>
>> What do you think?
> I'll have to look at the eeprom layout feature a little more in depth, the one thing
> that was a little 'annoying' (from a short quick glance) was that the layout was
> jumping around a bit (eth0, eth1, something else, eth2, eth3). But yes,
> I was quite intrigued herein.

Ohh.. you mean compulab layout? I see.
Compulab layout is already out there for 6 years in various devices.
It has gone through several versions, so to be backward compatible (and currently it is,
besides the legacy version), it had to do the "jumping" thing.
It is only last year Nikita has started to facilitate it a bit in upstream.
Anyway, the point is that eeprom layout in u-boot/common/eeprom/... should be generic
as a framework and any vendor/board can define their own layout in vendor/board location.
We are also going to extend the layout framework to provide more in-U-Boot APIs.

-- 
Regards,
Igor.

[U-Boot] [PATCH 01/14] net: cosmetic: Do not use magic values for ARP_HLEN

[Olliver Schinagl]

Just a note to myself mostly, I forgot to this one in the patch series, 
son in v2 I'll add that as well.


diff --git a/net/eth_common.c b/net/eth_common.c
index e0d8b62..57ef821 100644
--- a/net/eth_common.c
+++ b/net/eth_common.c
@@ -51,7 +51,7 @@ void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
         char *end;
         int i;

-       for (i = 0; i < 6; ++i) {
+       for (i = 0; i < ARP_HLEN; ++i) {
                 enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
                 if (addr)
                         addr = (*end) ? end + 1 : end;


On 25-11-16 16:30, Olliver Schinagl wrote:
> Commit 674bb249825a ("net: cosmetic: Replace magic numbers in arp.c with
> constants") introduced a nice define to replace the magic value 6 for
> the ethernet hardware address. Replace more hardcoded instances of 6
> which really reference the ARP_HLEN (iow the MAC/Hardware/Ethernet
> address).
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>   common/fdt_support.c |  2 +-
>   include/net.h        | 52 +++++++++++++++++++++++++++-------------------------
>   net/eth-uclass.c     | 10 +++++-----
>   net/eth_legacy.c     | 10 +++++-----
>   4 files changed, 38 insertions(+), 36 deletions(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 0609470..b082662 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -471,7 +471,7 @@ void fdt_fixup_ethernet(void *fdt)
>   	char *tmp, *end;
>   	char mac[16];
>   	const char *path;
> -	unsigned char mac_addr[6];
> +	unsigned char mac_addr[ARP_HLEN];
>   	int offset;
>   
>   	if (fdt_path_offset(fdt, "/aliases") < 0)
> diff --git a/include/net.h b/include/net.h
> index 06320c6..8137cf3 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -38,6 +38,9 @@
>   
>   #define PKTALIGN	ARCH_DMA_MINALIGN
>   
> +/* ARP hardware address length */
> +#define ARP_HLEN 6
> +
>   /* IPv4 addresses are always 32 bits in size */
>   struct in_addr {
>   	__be32 s_addr;
> @@ -90,7 +93,7 @@ enum eth_state_t {
>    */
>   struct eth_pdata {
>   	phys_addr_t iobase;
> -	unsigned char enetaddr[6];
> +	unsigned char enetaddr[ARP_HLEN];
>   	int phy_interface;
>   	int max_speed;
>   };
> @@ -161,7 +164,7 @@ void eth_halt_state_only(void); /* Set passive state */
>   #ifndef CONFIG_DM_ETH
>   struct eth_device {
>   	char name[16];
> -	unsigned char enetaddr[6];
> +	unsigned char enetaddr[ARP_HLEN];
>   	phys_addr_t iobase;
>   	int state;
>   
> @@ -293,9 +296,9 @@ u32 ether_crc(size_t len, unsigned char const *p);
>    */
>   
>   struct ethernet_hdr {
> -	u8		et_dest[6];	/* Destination node		*/
> -	u8		et_src[6];	/* Source node			*/
> -	u16		et_protlen;	/* Protocol or length		*/
> +	u8		et_dest[ARP_HLEN];	/* Destination node	*/
> +	u8		et_src[ARP_HLEN];	/* Source node		*/
> +	u16		et_protlen;		/* Protocol or length	*/
>   };
>   
>   /* Ethernet header size */
> @@ -304,16 +307,16 @@ struct ethernet_hdr {
>   #define ETH_FCS_LEN	4		/* Octets in the FCS		*/
>   
>   struct e802_hdr {
> -	u8		et_dest[6];	/* Destination node		*/
> -	u8		et_src[6];	/* Source node			*/
> -	u16		et_protlen;	/* Protocol or length		*/
> -	u8		et_dsap;	/* 802 DSAP			*/
> -	u8		et_ssap;	/* 802 SSAP			*/
> -	u8		et_ctl;		/* 802 control			*/
> -	u8		et_snap1;	/* SNAP				*/
> +	u8		et_dest[ARP_HLEN];	/* Destination node	*/
> +	u8		et_src[ARP_HLEN];	/* Source node		*/
> +	u16		et_protlen;		/* Protocol or length	*/
> +	u8		et_dsap;		/* 802 DSAP		*/
> +	u8		et_ssap;		/* 802 SSAP		*/
> +	u8		et_ctl;			/* 802 control		*/
> +	u8		et_snap1;		/* SNAP			*/
>   	u8		et_snap2;
>   	u8		et_snap3;
> -	u16		et_prot;	/* 802 protocol			*/
> +	u16		et_prot;		/* 802 protocol		*/
>   };
>   
>   /* 802 + SNAP + ethernet header size */
> @@ -323,11 +326,11 @@ struct e802_hdr {
>    *	Virtual LAN Ethernet header
>    */
>   struct vlan_ethernet_hdr {
> -	u8		vet_dest[6];	/* Destination node		*/
> -	u8		vet_src[6];	/* Source node			*/
> -	u16		vet_vlan_type;	/* PROT_VLAN			*/
> -	u16		vet_tag;	/* TAG of VLAN			*/
> -	u16		vet_type;	/* protocol type		*/
> +	u8		vet_dest[ARP_HLEN];	/* Destination node	*/
> +	u8		vet_src[ARP_HLEN];	/* Source node		*/
> +	u16		vet_vlan_type;		/* PROT_VLAN		*/
> +	u16		vet_tag;		/* TAG of VLAN		*/
> +	u16		vet_type;		/* protocol type	*/
>   };
>   
>   /* VLAN Ethernet header size */
> @@ -398,7 +401,6 @@ struct arp_hdr {
>   #   define ARP_ETHER	    1		/* Ethernet  hardware address	*/
>   	u16		ar_pro;		/* Format of protocol address	*/
>   	u8		ar_hln;		/* Length of hardware address	*/
> -#   define ARP_HLEN	6
>   	u8		ar_pln;		/* Length of protocol address	*/
>   #   define ARP_PLEN	4
>   	u16		ar_op;		/* Operation			*/
> @@ -507,16 +509,16 @@ extern char	net_nis_domain[32];	/* Our IS domain */
>   extern char	net_hostname[32];	/* Our hostname */
>   extern char	net_root_path[64];	/* Our root path */
>   /** END OF BOOTP EXTENTIONS **/
> -extern u8		net_ethaddr[6];		/* Our ethernet address */
> -extern u8		net_server_ethaddr[6];	/* Boot server enet address */
> +extern u8		net_ethaddr[ARP_HLEN];		/* Our ethernet address */
> +extern u8		net_server_ethaddr[ARP_HLEN];	/* Boot server enet address */
>   extern struct in_addr	net_ip;		/* Our    IP addr (0 = unknown) */
>   extern struct in_addr	net_server_ip;	/* Server IP addr (0 = unknown) */
>   extern uchar		*net_tx_packet;		/* THE transmit packet */
>   extern uchar		*net_rx_packets[PKTBUFSRX]; /* Receive packets */
>   extern uchar		*net_rx_packet;		/* Current receive packet */
>   extern int		net_rx_packet_len;	/* Current rx packet length */
> -extern const u8		net_bcast_ethaddr[6];	/* Ethernet broadcast address */
> -extern const u8		net_null_ethaddr[6];
> +extern const u8		net_bcast_ethaddr[ARP_HLEN];	/* Ethernet broadcast address */
> +extern const u8		net_null_ethaddr[ARP_HLEN];
>   
>   #define VLAN_NONE	4095			/* untagged */
>   #define VLAN_IDMASK	0x0fff			/* mask of valid vlan id */
> @@ -555,9 +557,9 @@ extern ushort cdp_appliance_vlan;	/* CDP returned appliance VLAN */
>    */
>   static inline int is_cdp_packet(const uchar *ethaddr)
>   {
> -	extern const u8 net_cdp_ethaddr[6];
> +	extern const u8 net_cdp_ethaddr[ARP_HLEN];
>   
> -	return memcmp(ethaddr, net_cdp_ethaddr, 6) == 0;
> +	return memcmp(ethaddr, net_cdp_ethaddr, ARP_HLEN) == 0;
>   }
>   #endif
>   
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index a32961e..a9fc6bb 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -230,7 +230,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>   			eth_write_hwaddr(dev);
>   			break;
>   		case env_op_delete:
> -			memset(pdata->enetaddr, 0, 6);
> +			memset(pdata->enetaddr, 0, ARP_HLEN);
>   		}
>   	}
>   
> @@ -458,7 +458,7 @@ static int eth_post_probe(struct udevice *dev)
>   {
>   	struct eth_device_priv *priv = dev->uclass_priv;
>   	struct eth_pdata *pdata = dev->platdata;
> -	unsigned char env_enetaddr[6];
> +	unsigned char env_enetaddr[ARP_HLEN];
>   
>   #if defined(CONFIG_NEEDS_MANUAL_RELOC)
>   	struct eth_ops *ops = eth_get_ops(dev);
> @@ -497,7 +497,7 @@ static int eth_post_probe(struct udevice *dev)
>   	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, 6)) {
> +		    memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) {
>   			printf("\nWarning: %s MAC addresses don't match:\n",
>   			       dev->name);
>   			printf("Address in SROM is         %pM\n",
> @@ -507,7 +507,7 @@ static int eth_post_probe(struct udevice *dev)
>   		}
>   
>   		/* Override the ROM MAC address */
> -		memcpy(pdata->enetaddr, env_enetaddr, 6);
> +		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>   	} else if (is_valid_ethaddr(pdata->enetaddr)) {
>   		eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
>   		printf("\nWarning: %s using MAC address from ROM\n",
> @@ -534,7 +534,7 @@ static int eth_pre_remove(struct udevice *dev)
>   	eth_get_ops(dev)->stop(dev);
>   
>   	/* clear the MAC address */
> -	memset(pdata->enetaddr, 0, 6);
> +	memset(pdata->enetaddr, 0, ARP_HLEN);
>   
>   	return 0;
>   }
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index d6d7cee..2b2c2de 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -120,7 +120,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>   				eth_parse_enetaddr(value, dev->enetaddr);
>   				break;
>   			case env_op_delete:
> -				memset(dev->enetaddr, 0, 6);
> +				memset(dev->enetaddr, 0, ARP_HLEN);
>   			}
>   		}
>   		dev = dev->next;
> @@ -133,14 +133,14 @@ U_BOOT_ENV_CALLBACK(ethaddr, on_ethaddr);
>   int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>   		   int eth_number)
>   {
> -	unsigned char env_enetaddr[6];
> +	unsigned char env_enetaddr[ARP_HLEN];
>   	int ret = 0;
>   
>   	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>   
>   	if (!is_zero_ethaddr(env_enetaddr)) {
>   		if (!is_zero_ethaddr(dev->enetaddr) &&
> -		    memcmp(dev->enetaddr, env_enetaddr, 6)) {
> +		    memcmp(dev->enetaddr, env_enetaddr, ARP_HLEN)) {
>   			printf("\nWarning: %s MAC addresses don't match:\n",
>   			       dev->name);
>   			printf("Address in SROM is         %pM\n",
> @@ -149,7 +149,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>   			       env_enetaddr);
>   		}
>   
> -		memcpy(dev->enetaddr, env_enetaddr, 6);
> +		memcpy(dev->enetaddr, env_enetaddr, ARP_HLEN);
>   	} else if (is_valid_ethaddr(dev->enetaddr)) {
>   		eth_setenv_enetaddr_by_index(base_name, eth_number,
>   					     dev->enetaddr);
> @@ -298,7 +298,7 @@ int eth_initialize(void)
>    */
>   int eth_mcast_join(struct in_addr mcast_ip, int join)
>   {
> -	u8 mcast_mac[6];
> +	u8 mcast_mac[ARP_HLEN];
>   	if (!eth_current || !eth_current->mcast)
>   		return -1;
>   	mcast_mac[5] = htonl(mcast_ip.s_addr) & 0xff;

[U-Boot] [PATCH 01/14] net: cosmetic: Do not use magic values for ARP_HLEN

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Commit 674bb249825a ("net: cosmetic: Replace magic numbers in arp.c with
> constants") introduced a nice define to replace the magic value 6 for
> the ethernet hardware address. Replace more hardcoded instances of 6
> which really reference the ARP_HLEN (iow the MAC/Hardware/Ethernet
> address).
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Olliver Schinagl]

Hey Michal,


On 28-11-16 09:21, Michal Simek wrote:
> On 25.11.2016 16:30, Olliver Schinagl wrote:
>> This patch allows Kconfig to enable and set parameters to make it
>> possible to read the MAC address from an EEPROM. The net core layer then
>> uses this information to read MAC addresses from this EEPROM.
>>
>> 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 and added.
>>
>> The code currently first checks if there is a non-zero MAC address in
>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>> used by a board to supply the MAC in other ways.
>>
>> If both these fails, the other code is still in place to query the
>> environent, which then can be used to override the hardware supplied
>> data.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/net.h       | 14 ++++++++
>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>   net/eth-uclass.c    |  9 +++--
>>   net/eth_common.c    | 34 ++++++++++++++++++
>>   net/eth_legacy.c    |  2 ++
>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>> index 50e4899..89c1f7d 100644
>> --- a/doc/README.enetaddr
>> +++ b/doc/README.enetaddr
>> @@ -47,6 +47,105 @@ 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
>> +--------
>> +
>> +Boards may come with an EEPROM specifically to store configuration bits, such
>> +as a MAC address. 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 can be used 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...|
>> +
>> +This can be done from linux using the i2c-tools:
>> +
>> +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
>> +
>> +Alternativly this can be done from the u-boot console as:
>> +
>> +u-boot> mm.b 0
>> +00000000: 00 ? 01
>> +00000001: 23 ? 23
>> +00000002: 45 ? 45
>> +00000003: 67 ? 67
>> +00000004: 89 ? 89
>> +00000005: ab ? ab
>> +00000006: be ? be
>> +00000007: 00 ? q
>> +i2c dev I2CBUS
>> +i2c write 0 50 8 7
>> +i2c md 50 8
>> +
>>   -------
>>    Usage
>>   -------
>> diff --git a/include/net.h b/include/net.h
>> index 08f8af8..e50ab5d 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>   
>>   /**
>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>> + *
>> + * This function tries to read the MAC address from an eeprom as can be read
>> + * in docs/README.enetaddr.
>> + *
>> + * @index:	index of the interface to get the hwaddr for
>> + * @enetaddr:	pointer for the found hwaddr. Needs to be atleast ARP_HLEN
>> + * @return:	0 on success, non-zero is error status. Additionally hwaddr
>> + *		is set to 00:00:00:00:00. This is also the case if
>> + *		CONFIG_NET_ETHADDR_EEPROM is not set.
>> + */
>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>> +
>> +/**
>>    * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>>    *
>>    * This sets up an environment variable with the given MAC address (@enetaddr).
>> diff --git a/net/Kconfig b/net/Kconfig
>> index 414c549..f699e1c 100644
>> --- a/net/Kconfig
>> +++ b/net/Kconfig
>> @@ -7,6 +7,65 @@ 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.
>> +	  Remember to also make the selected bus available via I2Cn_ENABLE.
>> +
>> +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.
> I would prefer to all these values to be in hex because i2c commands are
> also taking values in hex.
You are completly right and this is indeed my mistake. I will fix it for v2.

Incidently I put them on 0x50 in my own config files for this exact 
reason. I probably did not realise I could make it default to hex :)
>
>> +
>> +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.
>> +
> Would it be possible to have default n here?
> I would guess that more boards don't have this CRC8 sums.
I agree, but most boards will not use this by default yet. If you enable 
this feature for your board, I strongly strongly recommend enabeling 
this feature as well. Thus disable it by user request.

Reason why I strongly recommend to enable it: If i have an unprogrammed 
eeprom, it comes filled with 0xffffffff. Which is interpreted as a 
correct mac address. What if i have random garbage in the eeprom (or a 
user change one bit by accident). I still have a valid mac address. 
Using the crc8 to validate the mac address makes this a lot more safe.

Olliver
>
> Thanks,
> Michal
>
>
>

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Michal Simek]

On 29.11.2016 17:45, Olliver Schinagl wrote:
> Hey Michal,
> 
> 
> On 28-11-16 09:21, Michal Simek wrote:
>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>> This patch allows Kconfig to enable and set parameters to make it
>>> possible to read the MAC address from an EEPROM. The net core layer then
>>> uses this information to read MAC addresses from this EEPROM.
>>>
>>> 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 and added.
>>>
>>> The code currently first checks if there is a non-zero MAC address in
>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>> used by a board to supply the MAC in other ways.
>>>
>>> If both these fails, the other code is still in place to query the
>>> environent, which then can be used to override the hardware supplied
>>> data.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   doc/README.enetaddr | 99
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/net.h       | 14 ++++++++
>>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>   net/eth-uclass.c    |  9 +++--
>>>   net/eth_common.c    | 34 ++++++++++++++++++
>>>   net/eth_legacy.c    |  2 ++
>>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>> index 50e4899..89c1f7d 100644
>>> --- a/doc/README.enetaddr
>>> +++ b/doc/README.enetaddr
>>> @@ -47,6 +47,105 @@ 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
>>> +--------
>>> +
>>> +Boards may come with an EEPROM specifically to store configuration
>>> bits, such
>>> +as a MAC address. 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 can be used 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...|
>>> +
>>> +This can be done from linux using the i2c-tools:
>>> +
>>> +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
>>> +
>>> +Alternativly this can be done from the u-boot console as:
>>> +
>>> +u-boot> mm.b 0
>>> +00000000: 00 ? 01
>>> +00000001: 23 ? 23
>>> +00000002: 45 ? 45
>>> +00000003: 67 ? 67
>>> +00000004: 89 ? 89
>>> +00000005: ab ? ab
>>> +00000006: be ? be
>>> +00000007: 00 ? q
>>> +i2c dev I2CBUS
>>> +i2c write 0 50 8 7
>>> +i2c md 50 8
>>> +
>>>   -------
>>>    Usage
>>>   -------
>>> diff --git a/include/net.h b/include/net.h
>>> index 08f8af8..e50ab5d 100644
>>> --- a/include/net.h
>>> +++ b/include/net.h
>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>> *enetaddr);
>>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>     /**
>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>> + *
>>> + * This function tries to read the MAC address from an eeprom as can
>>> be read
>>> + * in docs/README.enetaddr.
>>> + *
>>> + * @index:    index of the interface to get the hwaddr for
>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>> ARP_HLEN
>>> + * @return:    0 on success, non-zero is error status. Additionally
>>> hwaddr
>>> + *        is set to 00:00:00:00:00. This is also the case if
>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>> + */
>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>> +
>>> +/**
>>>    * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>> variable
>>>    *
>>>    * This sets up an environment variable with the given MAC address
>>> (@enetaddr).
>>> diff --git a/net/Kconfig b/net/Kconfig
>>> index 414c549..f699e1c 100644
>>> --- a/net/Kconfig
>>> +++ b/net/Kconfig
>>> @@ -7,6 +7,65 @@ 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.
>>> +      Remember to also make the selected bus available via I2Cn_ENABLE.
>>> +
>>> +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.
>> I would prefer to all these values to be in hex because i2c commands are
>> also taking values in hex.
> You are completly right and this is indeed my mistake. I will fix it for
> v2.
> 
> Incidently I put them on 0x50 in my own config files for this exact
> reason. I probably did not realise I could make it default to hex :)

ok.

>>
>>> +
>>> +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.
>>> +
>> Would it be possible to have default n here?
>> I would guess that more boards don't have this CRC8 sums.
> I agree, but most boards will not use this by default yet. If you enable
> this feature for your board, I strongly strongly recommend enabeling
> this feature as well. Thus disable it by user request.

Hard to do it for boards in the field. As you see we have this feature
enabled for zcu102 and also zybo board and there is no crc8 sums factory
programmed.


> Reason why I strongly recommend to enable it: If i have an unprogrammed
> eeprom, it comes filled with 0xffffffff. Which is interpreted as a
> correct mac address. What if i have random garbage in the eeprom (or a
> user change one bit by accident). I still have a valid mac address.
> Using the crc8 to validate the mac address makes this a lot more safe.

Isn't 0xffffffff invalid address? I think I saw it as invalid.

Thanks,
Michal

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Olliver Schinagl]

Hey Michal,


On 29-11-16 19:54, Michal Simek wrote:
> On 29.11.2016 17:45, Olliver Schinagl wrote:
>> Hey Michal,
>>
>>
>> On 28-11-16 09:21, Michal Simek wrote:
>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>> This patch allows Kconfig to enable and set parameters to make it
>>>> possible to read the MAC address from an EEPROM. The net core layer then
>>>> uses this information to read MAC addresses from this EEPROM.
>>>>
>>>> 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 and added.
>>>>
>>>> The code currently first checks if there is a non-zero MAC address in
>>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>>> used by a board to supply the MAC in other ways.
>>>>
>>>> If both these fails, the other code is still in place to query the
>>>> environent, which then can be used to override the hardware supplied
>>>> data.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>> ---
>>>>    doc/README.enetaddr | 99
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/net.h       | 14 ++++++++
>>>>    net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>>    net/eth-uclass.c    |  9 +++--
>>>>    net/eth_common.c    | 34 ++++++++++++++++++
>>>>    net/eth_legacy.c    |  2 ++
>>>>    6 files changed, 214 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>>> index 50e4899..89c1f7d 100644
>>>> --- a/doc/README.enetaddr
>>>> +++ b/doc/README.enetaddr
>>>> @@ -47,6 +47,105 @@ 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
>>>> +--------
>>>> +
>>>> +Boards may come with an EEPROM specifically to store configuration
>>>> bits, such
>>>> +as a MAC address. 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 can be used 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...|
>>>> +
>>>> +This can be done from linux using the i2c-tools:
>>>> +
>>>> +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
>>>> +
>>>> +Alternativly this can be done from the u-boot console as:
>>>> +
>>>> +u-boot> mm.b 0
>>>> +00000000: 00 ? 01
>>>> +00000001: 23 ? 23
>>>> +00000002: 45 ? 45
>>>> +00000003: 67 ? 67
>>>> +00000004: 89 ? 89
>>>> +00000005: ab ? ab
>>>> +00000006: be ? be
>>>> +00000007: 00 ? q
>>>> +i2c dev I2CBUS
>>>> +i2c write 0 50 8 7
>>>> +i2c md 50 8
>>>> +
>>>>    -------
>>>>     Usage
>>>>    -------
>>>> diff --git a/include/net.h b/include/net.h
>>>> index 08f8af8..e50ab5d 100644
>>>> --- a/include/net.h
>>>> +++ b/include/net.h
>>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>>> *enetaddr);
>>>>    int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>>      /**
>>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>>> + *
>>>> + * This function tries to read the MAC address from an eeprom as can
>>>> be read
>>>> + * in docs/README.enetaddr.
>>>> + *
>>>> + * @index:    index of the interface to get the hwaddr for
>>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>>> ARP_HLEN
>>>> + * @return:    0 on success, non-zero is error status. Additionally
>>>> hwaddr
>>>> + *        is set to 00:00:00:00:00. This is also the case if
>>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>>> + */
>>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>>> +
>>>> +/**
>>>>     * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>>> variable
>>>>     *
>>>>     * This sets up an environment variable with the given MAC address
>>>> (@enetaddr).
>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>> index 414c549..f699e1c 100644
>>>> --- a/net/Kconfig
>>>> +++ b/net/Kconfig
>>>> @@ -7,6 +7,65 @@ 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.
>>>> +      Remember to also make the selected bus available via I2Cn_ENABLE.
>>>> +
>>>> +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.
>>> I would prefer to all these values to be in hex because i2c commands are
>>> also taking values in hex.
>> You are completly right and this is indeed my mistake. I will fix it for
>> v2.
>>
>> Incidently I put them on 0x50 in my own config files for this exact
>> reason. I probably did not realise I could make it default to hex :)
> ok.
>
>>>> +
>>>> +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.
>>>> +
>>> Would it be possible to have default n here?
>>> I would guess that more boards don't have this CRC8 sums.
>> I agree, but most boards will not use this by default yet. If you enable
>> this feature for your board, I strongly strongly recommend enabeling
>> this feature as well. Thus disable it by user request.
> Hard to do it for boards in the field. As you see we have this feature
> enabled for zcu102 and also zybo board and there is no crc8 sums factory
> programmed.
And that's why it is an option. If you turn on this option in your 
(board)config, you have the option as a board maintainer to set the crc8 
to off, because you have boards in the field already. If I add a new 
board now however, and want to use this feature, it should not be to 
easy to disable it.
>
>> Reason why I strongly recommend to enable it: If i have an unprogrammed
>> eeprom, it comes filled with 0xffffffff. Which is interpreted as a
>> correct mac address. What if i have random garbage in the eeprom (or a
>> user change one bit by accident). I still have a valid mac address.
>> Using the crc8 to validate the mac address makes this a lot more safe.
> Isn't 0xffffffff invalid address? I think I saw it as invalid.
I know 0x000000 is 'invalid' 0xffffffff might be indeed. but random bits 
are valid mac addresses.
>
> Thanks,
> Michal
>

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Michal Simek]

On 30.11.2016 09:10, Olliver Schinagl wrote:
> Hey Michal,
> 
> 
> On 29-11-16 19:54, Michal Simek wrote:
>> On 29.11.2016 17:45, Olliver Schinagl wrote:
>>> Hey Michal,
>>>
>>>
>>> On 28-11-16 09:21, Michal Simek wrote:
>>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>>> This patch allows Kconfig to enable and set parameters to make it
>>>>> possible to read the MAC address from an EEPROM. The net core layer
>>>>> then
>>>>> uses this information to read MAC addresses from this EEPROM.
>>>>>
>>>>> 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 and added.
>>>>>
>>>>> The code currently first checks if there is a non-zero MAC address in
>>>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>>>> used by a board to supply the MAC in other ways.
>>>>>
>>>>> If both these fails, the other code is still in place to query the
>>>>> environent, which then can be used to override the hardware supplied
>>>>> data.
>>>>>
>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>> ---
>>>>>    doc/README.enetaddr | 99
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/net.h       | 14 ++++++++
>>>>>    net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>>>    net/eth-uclass.c    |  9 +++--
>>>>>    net/eth_common.c    | 34 ++++++++++++++++++
>>>>>    net/eth_legacy.c    |  2 ++
>>>>>    6 files changed, 214 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>>>> index 50e4899..89c1f7d 100644
>>>>> --- a/doc/README.enetaddr
>>>>> +++ b/doc/README.enetaddr
>>>>> @@ -47,6 +47,105 @@ 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
>>>>> +--------
>>>>> +
>>>>> +Boards may come with an EEPROM specifically to store configuration
>>>>> bits, such
>>>>> +as a MAC address. 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 can be used 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...|
>>>>> +
>>>>> +This can be done from linux using the i2c-tools:
>>>>> +
>>>>> +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
>>>>> +
>>>>> +Alternativly this can be done from the u-boot console as:
>>>>> +
>>>>> +u-boot> mm.b 0
>>>>> +00000000: 00 ? 01
>>>>> +00000001: 23 ? 23
>>>>> +00000002: 45 ? 45
>>>>> +00000003: 67 ? 67
>>>>> +00000004: 89 ? 89
>>>>> +00000005: ab ? ab
>>>>> +00000006: be ? be
>>>>> +00000007: 00 ? q
>>>>> +i2c dev I2CBUS
>>>>> +i2c write 0 50 8 7
>>>>> +i2c md 50 8
>>>>> +
>>>>>    -------
>>>>>     Usage
>>>>>    -------
>>>>> diff --git a/include/net.h b/include/net.h
>>>>> index 08f8af8..e50ab5d 100644
>>>>> --- a/include/net.h
>>>>> +++ b/include/net.h
>>>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>>>> *enetaddr);
>>>>>    int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>>>      /**
>>>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>>>> + *
>>>>> + * This function tries to read the MAC address from an eeprom as can
>>>>> be read
>>>>> + * in docs/README.enetaddr.
>>>>> + *
>>>>> + * @index:    index of the interface to get the hwaddr for
>>>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>>>> ARP_HLEN
>>>>> + * @return:    0 on success, non-zero is error status. Additionally
>>>>> hwaddr
>>>>> + *        is set to 00:00:00:00:00. This is also the case if
>>>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>>>> + */
>>>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>>>> +
>>>>> +/**
>>>>>     * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>>>> variable
>>>>>     *
>>>>>     * This sets up an environment variable with the given MAC address
>>>>> (@enetaddr).
>>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>>> index 414c549..f699e1c 100644
>>>>> --- a/net/Kconfig
>>>>> +++ b/net/Kconfig
>>>>> @@ -7,6 +7,65 @@ 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.
>>>>> +      Remember to also make the selected bus available via
>>>>> I2Cn_ENABLE.
>>>>> +
>>>>> +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.
>>>> I would prefer to all these values to be in hex because i2c commands
>>>> are
>>>> also taking values in hex.
>>> You are completly right and this is indeed my mistake. I will fix it for
>>> v2.
>>>
>>> Incidently I put them on 0x50 in my own config files for this exact
>>> reason. I probably did not realise I could make it default to hex :)
>> ok.
>>
>>>>> +
>>>>> +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.
>>>>> +
>>>> Would it be possible to have default n here?
>>>> I would guess that more boards don't have this CRC8 sums.
>>> I agree, but most boards will not use this by default yet. If you enable
>>> this feature for your board, I strongly strongly recommend enabeling
>>> this feature as well. Thus disable it by user request.
>> Hard to do it for boards in the field. As you see we have this feature
>> enabled for zcu102 and also zybo board and there is no crc8 sums factory
>> programmed.
> And that's why it is an option. If you turn on this option in your
> (board)config, you have the option as a board maintainer to set the crc8
> to off, because you have boards in the field already. If I add a new
> board now however, and want to use this feature, it should not be to
> easy to disable it.

ok. No problem - will try to do some lobbing to adopt this and I will
disable it for zynq and zynqmp targets.

>>
>>> Reason why I strongly recommend to enable it: If i have an unprogrammed
>>> eeprom, it comes filled with 0xffffffff. Which is interpreted as a
>>> correct mac address. What if i have random garbage in the eeprom (or a
>>> user change one bit by accident). I still have a valid mac address.
>>> Using the crc8 to validate the mac address makes this a lot more safe.
>> Isn't 0xffffffff invalid address? I think I saw it as invalid.
> I know 0x000000 is 'invalid' 0xffffffff might be indeed. but random bits
> are valid mac addresses.

Sure random bits are separate cases. But even random in a lot of cases
can have correct CRC8 too. :-)
But feel free to check 0xff mac address if it is rejected or not.

Thanks,
Michal

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Marcel Ziswiler]

Hi Olliver

On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote:
> This patch adds a method for the board to set the MAC address if the
> environment is not yet set. The environment based MAC addresses are
> not
> touched, but if the fdt has an alias set, it is parsed and put into
> the
> environment.
> 
> E.g. The environment contains ethaddr and eth1addr, and the fdt
> contains
> an ethernet1 nothing happens. If the fdt contains ethernet2 however,
> it
> is parsed and inserted into the environment as eth2addr.

My humble understanding of device tree fixup is that it works the other
way around (e.g. it is the device tree that usually gets fixed up). So
the least I would advice for this patch is to change its naming but
most possibly such code also does not belong into the common
fdt_support implementation.

> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
> ?common/fdt_support.c | 8 +++++++-
> ?1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index c34a13c..f127392 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64
> size)
> ?	return fdt_fixup_memory_banks(blob, &start, &size, 1);
> ?}
> ?
> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
> +{
> +	return -ENOSYS;
> +}
> +
> ?void fdt_fixup_ethernet(void *fdt)
> ?{
> ?	int i, prop;
> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
> ?			if (fdt_eth_addr)
> ?				eth_parse_enetaddr(fdt_eth_addr,
> mac_addr);
> ?			else
> -				continue;
> +				if (board_get_enetaddr(i, mac_addr)
> < 0)
> +					continue;
> ?
> ?			do_fixup_by_path(fdt, path, "mac-address",
> ?					?&mac_addr, 6, 0);

Cheers

Marcel

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[maxime.ripard at free-electrons.com]

On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote:
> Hi Olliver
> 
> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote:
> > This patch adds a method for the board to set the MAC address if the
> > environment is not yet set. The environment based MAC addresses are
> > not
> > touched, but if the fdt has an alias set, it is parsed and put into
> > the
> > environment.
> > 
> > E.g. The environment contains ethaddr and eth1addr, and the fdt
> > contains
> > an ethernet1 nothing happens. If the fdt contains ethernet2 however,
> > it
> > is parsed and inserted into the environment as eth2addr.
> 
> My humble understanding of device tree fixup is that it works the other
> way around (e.g. it is the device tree that usually gets fixed up). So
> the least I would advice for this patch is to change its naming but
> most possibly such code also does not belong into the common
> fdt_support implementation.

I don't really have the context of this patch, but in the DT at least,
you can specify the mac address using the local-mac-address
property. I guess we should honor that too. But I don't really know
how that's related to an alias. If the device is probed and the
property is there, use it, otherwise don't.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161130/d2884d41/attachment.sig>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Hey maime,

Sorry for constantly getting your e-mail address wrong! Sorry!

On 30-11-16 10:12, maxime.ripard at free-electrons.com wrote:
> On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote:
>> Hi Olliver
>>
>> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote:
>>> This patch adds a method for the board to set the MAC address if the
>>> environment is not yet set. The environment based MAC addresses are
>>> not
>>> touched, but if the fdt has an alias set, it is parsed and put into
>>> the
>>> environment.
>>>
>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>> contains
>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however,
>>> it
>>> is parsed and inserted into the environment as eth2addr.
>> My humble understanding of device tree fixup is that it works the other
>> way around (e.g. it is the device tree that usually gets fixed up). So
>> the least I would advice for this patch is to change its naming but
>> most possibly such code also does not belong into the common
>> fdt_support implementation.
First, yes I noticed this as well, the nameing is the wrong way around. 
It took me a few reading times as well. I guess I did not full 
understand Hans's suggestion comment. So there's some work needed here.
> I don't really have the context of this patch, but in the DT at least,
> you can specify the mac address using the local-mac-address
> property. I guess we should honor that too. But I don't really know
> how that's related to an alias. If the device is probed and the
> property is there, use it, otherwise don't.
This came from the sdio_realtek module on some sunxi boards, where the 
device tree has configured it obviously for linux, but u-boot has no 
notion of it. But u-boot IS responsible to generate (the same) MAC 
address for the device. So yes, u-boot inserts a mac address into the 
device-tree for a found alias.

E.g. ethernet2 is an alias without a MAC address configured for it. Thus 
u-boot is used to generate the MAC address for this node and inserts it 
into the dtb. What I haven't double checked (just blindly assumed, which 
is the mother) if this code also inserts the mac into the environment, 
but the kernel only gets this information via the dtb anyway, right? 
Either via the dtb, or via the MAC register bytes where it is stored in 
some drivers.

olliver
>
> Maxime
>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Hans de Goede]

Hi,

On 30-11-16 11:50, Olliver Schinagl wrote:
> Hey maime,
>
> Sorry for constantly getting your e-mail address wrong! Sorry!
>
> On 30-11-16 10:12, maxime.ripard at free-electrons.com wrote:
>> On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote:
>>> Hi Olliver
>>>
>>> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote:
>>>> This patch adds a method for the board to set the MAC address if the
>>>> environment is not yet set. The environment based MAC addresses are
>>>> not
>>>> touched, but if the fdt has an alias set, it is parsed and put into
>>>> the
>>>> environment.
>>>>
>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>> contains
>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however,
>>>> it
>>>> is parsed and inserted into the environment as eth2addr.
>>> My humble understanding of device tree fixup is that it works the other
>>> way around (e.g. it is the device tree that usually gets fixed up). So
>>> the least I would advice for this patch is to change its naming but
>>> most possibly such code also does not belong into the common
>>> fdt_support implementation.
> First, yes I noticed this as well, the nameing is the wrong way around. It took me a few reading times as well. I guess I did not full understand Hans's suggestion comment. So there's some work needed here.
>> I don't really have the context of this patch, but in the DT at least,
>> you can specify the mac address using the local-mac-address
>> property. I guess we should honor that too. But I don't really know
>> how that's related to an alias. If the device is probed and the
>> property is there, use it, otherwise don't.
> This came from the sdio_realtek module on some sunxi boards, where the device tree has configured it obviously for linux, but u-boot has no notion of it. But u-boot IS responsible to generate (the same) MAC address for the device. So yes, u-boot inserts a mac address into the device-tree for a found alias.
>
> E.g. ethernet2 is an alias without a MAC address configured for it. Thus u-boot is used to generate the MAC address for this node and inserts it into the dtb. What I haven't double checked (just blindly assumed, which is the mother) if this code also inserts the mac into the environment, but the kernel only gets this information via the dtb anyway, right? Either via the dtb, or via the MAC register bytes where it is stored in some drivers.

About the setting of the MAC in the environment for devices u-boot
knows nothing about, but which have an alias in the dt, this is
not necessary, it was done in the old code as a way to make the
u-boot fdt code pick up the MAC and inject it into the dtb
passed to the kernel. If this can now happen without setting
it in the env that step can be skipped.

Regards,

Hans

p.s.

I've NOT been following these (and related threads). If anyone
has any questions for me, please send me a direct mail which
does not have "PATCH" in the subject, otherwise I will likely
not seeing as I'm mostly ignoring these threads (sorry -ENOTIME).

[U-Boot] [PATCH 02/14] net: cosmetic: Make the MAC address string less magical

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> In u-boot printf has been extended with the %pM formatter to allow
> printing of MAC addresses. However buffers that want to store a MAC
> address cannot safely get the size. Add a define for this case so the
> string of a MAC address can be reliably obtained.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 03/14] net: cosmetic: Define ethernet name length

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> There are various places where the ethernet device name is defined to
> several different sizes. Lets add a define and start using it.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 04/14] net: core: Sanitize get/set operations for enetaddr

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> In the current net stack, we have a few functions to get and set
> the "ethaddr" and "ethNaddr" environment variables, which use magic
> values to get and set these environment variables. Remove the magicness
> of the buffer by defining it proper and also check the input for its
> length.
>
> Additionally use the define in fdt parser where the ethaddr variables
> are also used.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  common/fdt_support.c |  2 +-
>  include/net.h        |  1 +
>  net/eth_common.c     | 12 ++++++++----
>  3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index b082662..89e6e47 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -469,7 +469,7 @@ void fdt_fixup_ethernet(void *fdt)
>  {
>         int i, j, prop;
>         char *tmp, *end;
> -       char mac[16];
> +       char mac[ETH_ENETADDR_LEN];
>         const char *path;
>         unsigned char mac_addr[ARP_HLEN];
>         int offset;
> diff --git a/include/net.h b/include/net.h
> index 9cd7870..2534913 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -243,6 +243,7 @@ void eth_set_current(void);         /* set nterface to ethcur var */
>
>  int eth_get_dev_index(void);           /* get the device index */
>  void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
> +#define ETH_ENETADDR_LEN 32

I'd prefer a clearer name here.

Maybe ETH_ENETADDR_ENV_NAME_LEN?

>  int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>  int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>
> diff --git a/net/eth_common.c b/net/eth_common.c
> index e9d3c66..079be89 100644
> --- a/net/eth_common.c
> +++ b/net/eth_common.c
> @@ -42,16 +42,20 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr)
>  int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>                                  uchar *enetaddr)
>  {
> -       char enetvar[32];
> -       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +       char enetvar[ETH_ENETADDR_LEN];
> +
> +       snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr",
> +                base_name, index);
>         return eth_getenv_enetaddr(enetvar, enetaddr);
>  }
>
>  int eth_setenv_enetaddr_by_index(const char *base_name, int index,
>                                  uchar *enetaddr)
>  {
> -       char enetvar[32];
> -       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +       char enetvar[ETH_ENETADDR_LEN];
> +
> +       snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr",
> +                base_name, index);
>         return eth_setenv_enetaddr(enetvar, enetaddr);
>  }
>
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 05/14] net: core: cosmetic: A MAC address is not limited to SROM

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Currently, we print that the MAC from the SROM does not match. It can be
> many forms of ROM, so lets drop the S.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Currently we print a warning if the MAC address is read from
> ROM/Hardware. This should not be concidered a bad or erronous thing. A
> MAC address should come either from the hardware (ROM) or may be
> set/overriden in the environment. Neither is a warning or error case.
>
> Worthy of a warning is the case where both are set, so the user is
> atleast aware something special is happening.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  net/eth-uclass.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 9703bea..aca3f6d 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>                 memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>         } else if (is_valid_ethaddr(pdata->enetaddr)) {
>                 eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
> -               printf("\nWarning: %s using MAC address from ROM\n",
> -                      dev->name);

I would prefer to see this go away in the same patch that adds the
source to other prints.

>         } else if (is_zero_ethaddr(pdata->enetaddr)) {
>  #ifdef CONFIG_NET_RANDOM_ETHADDR
>                 net_random_ethaddr(pdata->enetaddr);
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 07/14] net: core: Inform the user of the device MAC address

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> In certain conditions we currently print the MAC address. For example a
> warning when a random mac address is in use or a missmatch between HW
> and ENV.
>
> If all things went well however (but even if there is a miss-match) we
> do not inform the user what the final MAC address of the device is.
>
> Lets print the final MAC address of the device with which it has been
> setup.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  net/eth-uclass.c | 9 ++++++---
>  net/eth_legacy.c | 3 +++
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index aca3f6d..5c888b8 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -413,11 +413,12 @@ int eth_initialize(void)
>                 }
>
>                 bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT);
> +               putc('\n');
>                 do {
> -                       if (num_devices)
> -                               printf(", ");
> +                       struct eth_pdata *pdata = dev->platdata;
>
> -                       printf("eth%d: %s", dev->seq, dev->name);
> +                       printf("eth%d:  %s [%pM]\n", dev->seq, dev->name,
> +                                                    pdata->enetaddr);
>
>                         if (ethprime && dev == prime_dev)
>                                 printf(" [PRIME]");
> @@ -522,6 +523,8 @@ static int eth_post_probe(struct udevice *dev)
>  #endif
>         }
>
> +       printf("%s ", dev->name);
> +

Why this?

Can you send to the list what an example output looks like?

Thanks,
-Joe

>         return 0;
>  }
>
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index 2b2c2de..bf4de37 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -178,6 +178,9 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>                                dev->name);
>         }
>
> +       printf("%s (eth%d) has MAC address: %pM\n",
> +              dev->name, eth_number, dev->enetaddr);
> +
>         return ret;
>  }
>
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 08/14] net: core: Add function to check/set MAC locality

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Universally administered and locally administered addresses are
> distinguished by setting the second-least-significant bit of the first
> octet of the address. Having a function to check and set this U/L bit
> from a function makes it nice for boards that want to generate their own
> mac address to ensure they are locally administered.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Joe Hershberger]

On Wed, Nov 30, 2016 at 2:10 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Michal,
>
>
>
> On 29-11-16 19:54, Michal Simek wrote:
>>
>> On 29.11.2016 17:45, Olliver Schinagl wrote:
>>>
>>> Hey Michal,
>>>
>>>
>>> On 28-11-16 09:21, Michal Simek wrote:
>>>>
>>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>>>
>>>>> This patch allows Kconfig to enable and set parameters to make it
>>>>> possible to read the MAC address from an EEPROM. The net core layer
>>>>> then
>>>>> uses this information to read MAC addresses from this EEPROM.
>>>>>
>>>>> 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 and added.
>>>>>
>>>>> The code currently first checks if there is a non-zero MAC address in
>>>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>>>> used by a board to supply the MAC in other ways.
>>>>>
>>>>> If both these fails, the other code is still in place to query the
>>>>> environent, which then can be used to override the hardware supplied
>>>>> data.
>>>>>
>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>> ---
>>>>>    doc/README.enetaddr | 99
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/net.h       | 14 ++++++++
>>>>>    net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>>>    net/eth-uclass.c    |  9 +++--
>>>>>    net/eth_common.c    | 34 ++++++++++++++++++
>>>>>    net/eth_legacy.c    |  2 ++
>>>>>    6 files changed, 214 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>>>> index 50e4899..89c1f7d 100644
>>>>> --- a/doc/README.enetaddr
>>>>> +++ b/doc/README.enetaddr
>>>>> @@ -47,6 +47,105 @@ 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
>>>>> +--------
>>>>> +
>>>>> +Boards may come with an EEPROM specifically to store configuration
>>>>> bits, such
>>>>> +as a MAC address. 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 can be used 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...|
>>>>> +
>>>>> +This can be done from linux using the i2c-tools:
>>>>> +
>>>>> +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
>>>>> +
>>>>> +Alternativly this can be done from the u-boot console as:
>>>>> +
>>>>> +u-boot> mm.b 0
>>>>> +00000000: 00 ? 01
>>>>> +00000001: 23 ? 23
>>>>> +00000002: 45 ? 45
>>>>> +00000003: 67 ? 67
>>>>> +00000004: 89 ? 89
>>>>> +00000005: ab ? ab
>>>>> +00000006: be ? be
>>>>> +00000007: 00 ? q
>>>>> +i2c dev I2CBUS
>>>>> +i2c write 0 50 8 7
>>>>> +i2c md 50 8
>>>>> +
>>>>>    -------
>>>>>     Usage
>>>>>    -------
>>>>> diff --git a/include/net.h b/include/net.h
>>>>> index 08f8af8..e50ab5d 100644
>>>>> --- a/include/net.h
>>>>> +++ b/include/net.h
>>>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>>>> *enetaddr);
>>>>>    int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>>>      /**
>>>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>>>> + *
>>>>> + * This function tries to read the MAC address from an eeprom as can
>>>>> be read
>>>>> + * in docs/README.enetaddr.
>>>>> + *
>>>>> + * @index:    index of the interface to get the hwaddr for
>>>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>>>> ARP_HLEN
>>>>> + * @return:    0 on success, non-zero is error status. Additionally
>>>>> hwaddr
>>>>> + *        is set to 00:00:00:00:00. This is also the case if
>>>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>>>> + */
>>>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>>>> +
>>>>> +/**
>>>>>     * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>>>> variable
>>>>>     *
>>>>>     * This sets up an environment variable with the given MAC address
>>>>> (@enetaddr).
>>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>>> index 414c549..f699e1c 100644
>>>>> --- a/net/Kconfig
>>>>> +++ b/net/Kconfig
>>>>> @@ -7,6 +7,65 @@ 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.
>>>>> +      Remember to also make the selected bus available via
>>>>> I2Cn_ENABLE.
>>>>> +
>>>>> +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.
>>>>
>>>> I would prefer to all these values to be in hex because i2c commands are
>>>> also taking values in hex.
>>>
>>> You are completly right and this is indeed my mistake. I will fix it for
>>> v2.
>>>
>>> Incidently I put them on 0x50 in my own config files for this exact
>>> reason. I probably did not realise I could make it default to hex :)
>>
>> ok.
>>
>>>>> +
>>>>> +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.
>>>>> +
>>>>
>>>> Would it be possible to have default n here?
>>>> I would guess that more boards don't have this CRC8 sums.
>>>
>>> I agree, but most boards will not use this by default yet. If you enable
>>> this feature for your board, I strongly strongly recommend enabeling
>>> this feature as well. Thus disable it by user request.
>>
>> Hard to do it for boards in the field. As you see we have this feature
>> enabled for zcu102 and also zybo board and there is no crc8 sums factory
>> programmed.
>
> And that's why it is an option. If you turn on this option in your
> (board)config, you have the option as a board maintainer to set the crc8 to
> off, because you have boards in the field already. If I add a new board now
> however, and want to use this feature, it should not be to easy to disable
> it.
>>
>>
>>> Reason why I strongly recommend to enable it: If i have an unprogrammed
>>> eeprom, it comes filled with 0xffffffff. Which is interpreted as a
>>> correct mac address. What if i have random garbage in the eeprom (or a
>>> user change one bit by accident). I still have a valid mac address.
>>> Using the crc8 to validate the mac address makes this a lot more safe.
>>
>> Isn't 0xffffffff invalid address? I think I saw it as invalid.
>
> I know 0x000000 is 'invalid' 0xffffffff might be indeed. but random bits are
> valid mac addresses.

It is perfectly valid, just not as your board's MAC address. It is the
link-wide broadcast address. See is_broadcast_ethaddr()

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> This patch allows Kconfig to enable and set parameters to make it
> possible to read the MAC address from an EEPROM. The net core layer then
> uses this information to read MAC addresses from this EEPROM.
>
> 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 and added.
>
> The code currently first checks if there is a non-zero MAC address in
> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
> used by a board to supply the MAC in other ways.
>
> If both these fails, the other code is still in place to query the
> environent, which then can be used to override the hardware supplied
> data.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/net.h       | 14 ++++++++
>  net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>  net/eth-uclass.c    |  9 +++--
>  net/eth_common.c    | 34 ++++++++++++++++++
>  net/eth_legacy.c    |  2 ++
>  6 files changed, 214 insertions(+), 3 deletions(-)
>
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index 50e4899..89c1f7d 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -47,6 +47,105 @@ 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
> +--------
> +
> +Boards may come with an EEPROM specifically to store configuration bits, such
> +as a MAC address. 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 can be used 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...|
> +
> +This can be done from linux using the i2c-tools:
> +
> +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
> +
> +Alternativly this can be done from the u-boot console as:
> +
> +u-boot> mm.b 0
> +00000000: 00 ? 01
> +00000001: 23 ? 23
> +00000002: 45 ? 45
> +00000003: 67 ? 67
> +00000004: 89 ? 89
> +00000005: ab ? ab
> +00000006: be ? be
> +00000007: 00 ? q
> +i2c dev I2CBUS
> +i2c write 0 50 8 7
> +i2c md 50 8
> +
>  -------
>   Usage
>  -------
> diff --git a/include/net.h b/include/net.h
> index 08f8af8..e50ab5d 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>  int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>
>  /**
> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
> + *
> + * This function tries to read the MAC address from an eeprom as can be read
> + * in docs/README.enetaddr.
> + *
> + * @index:     index of the interface to get the hwaddr for
> + * @enetaddr:  pointer for the found hwaddr. Needs to be atleast ARP_HLEN
> + * @return:    0 on success, non-zero is error status. Additionally hwaddr
> + *             is set to 00:00:00:00:00. This is also the case if
> + *             CONFIG_NET_ETHADDR_EEPROM is not set.
> + */
> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
> +
> +/**
>   * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>   *
>   * This sets up an environment variable with the given MAC address (@enetaddr).
> diff --git a/net/Kconfig b/net/Kconfig
> index 414c549..f699e1c 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -7,6 +7,65 @@ 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.
> +         Remember to also make the selected bus available via I2Cn_ENABLE.
> +
> +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
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 5c888b8..a154a7e 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -491,9 +491,12 @@ static int eth_post_probe(struct udevice *dev)
>
>         priv->state = ETH_STATE_INIT;
>
> -       /* Check if the device has a MAC address in ROM */
> -       if (eth_get_ops(dev)->read_rom_hwaddr)
> -               eth_get_ops(dev)->read_rom_hwaddr(dev);
> +       /* Check if the device has a MAC address in EEPROM */
> +       eeprom_read_enetaddr(dev->seq, pdata->enetaddr);
> +       if (is_zero_ethaddr(pdata->enetaddr))
> +               /* Check if the device has a MAC address in ROM */
> +               if (eth_get_ops(dev)->read_rom_hwaddr)
> +                       eth_get_ops(dev)->read_rom_hwaddr(dev);
>
>         eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>         if (!is_zero_ethaddr(env_enetaddr)) {
> diff --git a/net/eth_common.c b/net/eth_common.c
> index 079be89..e0d8b62 100644
> --- a/net/eth_common.c
> +++ b/net/eth_common.c
> @@ -8,10 +8,44 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <i2c.h>
>  #include <miiphy.h>
>  #include <net.h>
>  #include "eth_internal.h"
>
> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr)
> +{
> +       uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
> +#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)

Since it is easily reasonable that SPI PROM is a likely useful
support, why not keep the layout stuff separate from the I2C stuff so
that it is trivial to plug in a different bus later? It will also make
the code clearer by untangling these.

> +       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 from the eeprom */
> +       if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
> +                    CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index * (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 or EEPROM missing on device: ");
> +               return -ENOSYS;
> +       }
> +       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 on device: ");
> +               return -EINVAL;
> +       }
> +#endif
> +#endif
> +
> +       memcpy(enetaddr, eeprom, ARP_HLEN);
> +
> +       return 0;
> +}
> +
>  void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>  {
>         char *end;
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index bf4de37..8fb5844 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>         unsigned char env_enetaddr[ARP_HLEN];
>         int ret = 0;
>
> +       eeprom_read_enetaddr(eth_number, dev->enetaddr);
> +
>         eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>
>         if (!is_zero_ethaddr(env_enetaddr)) {
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 10/14] fdt: fixup_eth: Remove code duplication with a function

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> In fdt_support.c we use a loop to parse the mac address string from the
> fdt blob, net.h has a function for this however, so lets use it.
>
> Also, rename the variable from tmp to something more descriptive.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 11/14] fdt: eth_fixup: improve error catching/reduce identation

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Currently when checking for an error in ethernet aliases in the fdt, we
> only check for the error case -1. It is safer to ignore anything < 0.
>
> By rearranging logic a bit we can now also reduce identation.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

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

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, 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>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Seems simple enough.

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> This patch adds a method for the board to set the MAC address if the
> environment is not yet set. The environment based MAC addresses are not
> touched, but if the fdt has an alias set, it is parsed and put into the
> environment.
>
> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
> is parsed and inserted into the environment as eth2addr.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  common/fdt_support.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index c34a13c..f127392 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
>         return fdt_fixup_memory_banks(blob, &start, &size, 1);
>  }
>
> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)

Ugh. This collides with a function in board/v38b/ethaddr.c and in
board/intercontrol/digsy_mtc/digsy_mtc.c

Also, it's so generic, and only gets called by the fdt fixup stuff...
This function should probably be named in such a way that its
association with fdt is clear.

> +{
> +       return -ENOSYS;
> +}
> +
>  void fdt_fixup_ethernet(void *fdt)
>  {
>         int i, prop;
> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
>                         if (fdt_eth_addr)
>                                 eth_parse_enetaddr(fdt_eth_addr, mac_addr);
>                         else
> -                               continue;
> +                               if (board_get_enetaddr(i, mac_addr) < 0)
> +                                       continue;
>
>                         do_fixup_by_path(fdt, path, "mac-address",
>                                          &mac_addr, 6, 0);
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Joe Hershberger]

On Wed, Nov 30, 2016 at 4:54 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 30-11-16 11:50, Olliver Schinagl wrote:
>>
>> Hey maime,
>>
>> Sorry for constantly getting your e-mail address wrong! Sorry!
>>
>> On 30-11-16 10:12, maxime.ripard at free-electrons.com wrote:
>>>
>>> On Wed, Nov 30, 2016 at 09:00:51AM +0000, Marcel Ziswiler wrote:
>>>>
>>>> Hi Olliver
>>>>
>>>> On Fri, 2016-11-25 at 16:30 +0100, Olliver Schinagl wrote:
>>>>>
>>>>> This patch adds a method for the board to set the MAC address if the
>>>>> environment is not yet set. The environment based MAC addresses are
>>>>> not
>>>>> touched, but if the fdt has an alias set, it is parsed and put into
>>>>> the
>>>>> environment.
>>>>>
>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>> contains
>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however,
>>>>> it
>>>>> is parsed and inserted into the environment as eth2addr.
>>>>
>>>> My humble understanding of device tree fixup is that it works the other
>>>> way around (e.g. it is the device tree that usually gets fixed up). So
>>>> the least I would advice for this patch is to change its naming but
>>>> most possibly such code also does not belong into the common
>>>> fdt_support implementation.
>>
>> First, yes I noticed this as well, the nameing is the wrong way around. It
>> took me a few reading times as well. I guess I did not full understand
>> Hans's suggestion comment. So there's some work needed here.
>>>
>>> I don't really have the context of this patch, but in the DT at least,
>>> you can specify the mac address using the local-mac-address
>>> property. I guess we should honor that too. But I don't really know
>>> how that's related to an alias. If the device is probed and the
>>> property is there, use it, otherwise don't.
>>
>> This came from the sdio_realtek module on some sunxi boards, where the
>> device tree has configured it obviously for linux, but u-boot has no notion
>> of it. But u-boot IS responsible to generate (the same) MAC address for the
>> device. So yes, u-boot inserts a mac address into the device-tree for a
>> found alias.
>>
>> E.g. ethernet2 is an alias without a MAC address configured for it. Thus
>> u-boot is used to generate the MAC address for this node and inserts it into
>> the dtb. What I haven't double checked (just blindly assumed, which is the
>> mother) if this code also inserts the mac into the environment, but the
>> kernel only gets this information via the dtb anyway, right? Either via the
>> dtb, or via the MAC register bytes where it is stored in some drivers.
>
>
> About the setting of the MAC in the environment for devices u-boot
> knows nothing about, but which have an alias in the dt, this is
> not necessary, it was done in the old code as a way to make the
> u-boot fdt code pick up the MAC and inject it into the dtb
> passed to the kernel. If this can now happen without setting
> it in the env that step can be skipped.

I agree that the env should not be updated.

-Joe

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Simon Glass]

Hi,

On 25 November 2016 at 08:30, Olliver Schinagl <oliver@schinagl.nl> wrote:
> This patch adds a method for the board to set the MAC address if the
> environment is not yet set. The environment based MAC addresses are not
> touched, but if the fdt has an alias set, it is parsed and put into the
> environment.
>
> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
> is parsed and inserted into the environment as eth2addr.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  common/fdt_support.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index c34a13c..f127392 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
>         return fdt_fixup_memory_banks(blob, &start, &size, 1);
>  }
>
> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
> +{
> +       return -ENOSYS;
> +}
> +
>  void fdt_fixup_ethernet(void *fdt)
>  {
>         int i, prop;
> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
>                         if (fdt_eth_addr)
>                                 eth_parse_enetaddr(fdt_eth_addr, mac_addr);
>                         else
> -                               continue;
> +                               if (board_get_enetaddr(i, mac_addr) < 0)
> +                                       continue;
>
>                         do_fixup_by_path(fdt, path, "mac-address",
>                                          &mac_addr, 6, 0);
> --
> 2.10.2
>

Much as I don't want to pin this on any one patch, but I feel that our
DT fixup stuff is a bit out of control. We have so many functions that
do this, and they are called from various places. At some point we
need to think about the infrastructure.

IMO we should move to a linker list approach a bit like SPL did
recently (SPL_LOAD_IMAGE_METHOD). Then boards and subsystems can
register (at build-time) a fixup routine and they all get called one
after the other.

We could also (later) add run-time support for registering fixups,
that drivers could use.

Regards,
Simon

[U-Boot] [PATCH 06/14] net: core: Using an ethernet address from ROM is not bad

[Olliver Schinagl]

Hey Joe,


On 30-11-16 20:23, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> Currently we print a warning if the MAC address is read from
>> ROM/Hardware. This should not be concidered a bad or erronous thing. A
>> MAC address should come either from the hardware (ROM) or may be
>> set/overriden in the environment. Neither is a warning or error case.
>>
>> Worthy of a warning is the case where both are set, so the user is
>> atleast aware something special is happening.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   net/eth-uclass.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index 9703bea..aca3f6d 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -510,8 +510,6 @@ static int eth_post_probe(struct udevice *dev)
>>                  memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>>          } else if (is_valid_ethaddr(pdata->enetaddr)) {
>>                  eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
>> -               printf("\nWarning: %s using MAC address from ROM\n",
>> -                      dev->name);
> I would prefer to see this go away in the same patch that adds the
> source to other prints.
Great minds! in v2 (which I'm testing right now) this has been turned 
into one patch.
>>          } else if (is_zero_ethaddr(pdata->enetaddr)) {
>>   #ifdef CONFIG_NET_RANDOM_ETHADDR
>>                  net_random_ethaddr(pdata->enetaddr);
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 07/14] net: core: Inform the user of the device MAC address

[Olliver Schinagl]

Hey Joe,


On 30-11-16 20:29, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> In certain conditions we currently print the MAC address. For example a
>> warning when a random mac address is in use or a missmatch between HW
>> and ENV.
>>
>> If all things went well however (but even if there is a miss-match) we
>> do not inform the user what the final MAC address of the device is.
>>
>> Lets print the final MAC address of the device with which it has been
>> setup.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   net/eth-uclass.c | 9 ++++++---
>>   net/eth_legacy.c | 3 +++
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index aca3f6d..5c888b8 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -413,11 +413,12 @@ int eth_initialize(void)
>>                  }
>>
>>                  bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT);
>> +               putc('\n');
>>                  do {
>> -                       if (num_devices)
>> -                               printf(", ");
>> +                       struct eth_pdata *pdata = dev->platdata;
>>
>> -                       printf("eth%d: %s", dev->seq, dev->name);
>> +                       printf("eth%d:  %s [%pM]\n", dev->seq, dev->name,
>> +                                                    pdata->enetaddr);
>>
>>                          if (ethprime && dev == prime_dev)
>>                                  printf(" [PRIME]");
>> @@ -522,6 +523,8 @@ static int eth_post_probe(struct udevice *dev)
>>   #endif
>>          }
>>
>> +       printf("%s ", dev->name);
>> +
> Why this?
>
> Can you send to the list what an example output looks like?

Absolutly. Right now I have this, with the v2 work I'm doing to print 
the mac source.

Net:   ethernet at 01c50000 <other etnernet devices>
eth0:  ethernet at 01c50000 [cc:bd:d3:00:01:c6] (EEPROM)
eth1: <other_ethernet_device1> [<other_mac>] (<other_source>)


If there is an error or warning from the net layer (during probe, during 
init etc)
we get something like

Net:   CRC error on MAC address from EEPROM on device: ethernet at 01c50000 
<other warning> on device: <other_device>
eth0:  ethernet at 01c50000 [02:4b:04:42:12:31] (driver)

(where driver is the read_rom_hwaddr hook a driver supplies).

I did not go over all error messages to print it as pretty. So in that 
case it will be
Net:   Some error. ethernet at 01c50000

Olliver
>
> Thanks,
> -Joe
>
>>          return 0;
>>   }
>>
>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>> index 2b2c2de..bf4de37 100644
>> --- a/net/eth_legacy.c
>> +++ b/net/eth_legacy.c
>> @@ -178,6 +178,9 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>>                                 dev->name);
>>          }
>>
>> +       printf("%s (eth%d) has MAC address: %pM\n",
>> +              dev->name, eth_number, dev->enetaddr);
>> +
>>          return ret;
>>   }
>>
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Olliver Schinagl]

Hey Joe,


On 30-11-16 21:00, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> This patch allows Kconfig to enable and set parameters to make it
>> possible to read the MAC address from an EEPROM. The net core layer then
>> uses this information to read MAC addresses from this EEPROM.
>>
>> 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 and added.
>>
>> The code currently first checks if there is a non-zero MAC address in
>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>> used by a board to supply the MAC in other ways.
>>
>> If both these fails, the other code is still in place to query the
>> environent, which then can be used to override the hardware supplied
>> data.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/net.h       | 14 ++++++++
>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>   net/eth-uclass.c    |  9 +++--
>>   net/eth_common.c    | 34 ++++++++++++++++++
>>   net/eth_legacy.c    |  2 ++
>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>> index 50e4899..89c1f7d 100644
>> --- a/doc/README.enetaddr
>> +++ b/doc/README.enetaddr
>> @@ -47,6 +47,105 @@ 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
>> +--------
>> +
>> +Boards may come with an EEPROM specifically to store configuration bits, such
>> +as a MAC address. 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 can be used 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...|
>> +
>> +This can be done from linux using the i2c-tools:
>> +
>> +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
>> +
>> +Alternativly this can be done from the u-boot console as:
>> +
>> +u-boot> mm.b 0
>> +00000000: 00 ? 01
>> +00000001: 23 ? 23
>> +00000002: 45 ? 45
>> +00000003: 67 ? 67
>> +00000004: 89 ? 89
>> +00000005: ab ? ab
>> +00000006: be ? be
>> +00000007: 00 ? q
>> +i2c dev I2CBUS
>> +i2c write 0 50 8 7
>> +i2c md 50 8
>> +
>>   -------
>>    Usage
>>   -------
>> diff --git a/include/net.h b/include/net.h
>> index 08f8af8..e50ab5d 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>
>>   /**
>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>> + *
>> + * This function tries to read the MAC address from an eeprom as can be read
>> + * in docs/README.enetaddr.
>> + *
>> + * @index:     index of the interface to get the hwaddr for
>> + * @enetaddr:  pointer for the found hwaddr. Needs to be atleast ARP_HLEN
>> + * @return:    0 on success, non-zero is error status. Additionally hwaddr
>> + *             is set to 00:00:00:00:00. This is also the case if
>> + *             CONFIG_NET_ETHADDR_EEPROM is not set.
>> + */
>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>> +
>> +/**
>>    * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>>    *
>>    * This sets up an environment variable with the given MAC address (@enetaddr).
>> diff --git a/net/Kconfig b/net/Kconfig
>> index 414c549..f699e1c 100644
>> --- a/net/Kconfig
>> +++ b/net/Kconfig
>> @@ -7,6 +7,65 @@ 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.
>> +         Remember to also make the selected bus available via I2Cn_ENABLE.
>> +
>> +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
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index 5c888b8..a154a7e 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -491,9 +491,12 @@ static int eth_post_probe(struct udevice *dev)
>>
>>          priv->state = ETH_STATE_INIT;
>>
>> -       /* Check if the device has a MAC address in ROM */
>> -       if (eth_get_ops(dev)->read_rom_hwaddr)
>> -               eth_get_ops(dev)->read_rom_hwaddr(dev);
>> +       /* Check if the device has a MAC address in EEPROM */
>> +       eeprom_read_enetaddr(dev->seq, pdata->enetaddr);
>> +       if (is_zero_ethaddr(pdata->enetaddr))
>> +               /* Check if the device has a MAC address in ROM */
>> +               if (eth_get_ops(dev)->read_rom_hwaddr)
>> +                       eth_get_ops(dev)->read_rom_hwaddr(dev);
>>
>>          eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>>          if (!is_zero_ethaddr(env_enetaddr)) {
>> diff --git a/net/eth_common.c b/net/eth_common.c
>> index 079be89..e0d8b62 100644
>> --- a/net/eth_common.c
>> +++ b/net/eth_common.c
>> @@ -8,10 +8,44 @@
>>
>>   #include <common.h>
>>   #include <dm.h>
>> +#include <i2c.h>
>>   #include <miiphy.h>
>>   #include <net.h>
>>   #include "eth_internal.h"
>>
>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr)
>> +{
>> +       uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
>> +#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
> Since it is easily reasonable that SPI PROM is a likely useful
> support, why not keep the layout stuff separate from the I2C stuff so
> that it is trivial to plug in a different bus later? It will also make
> the code clearer by untangling these.

I strongly agree, but I recommend a follow up patch series (and thus 
merge this as is for now) to use Maxime's EEPROM framework patches. So 
then this gets replaced by simple read from eeprom.

So yes, I have contemplated in splitting it up now and have a simply 
read_from_i2c() kind of function, I figured this gets solved elsewhere 
anyway.

Additionally, the layout stuff would ideally be replaced by Igor (i 
think it was) eeprom layout framework (if those two combine) which 
solves both problems in one go.

Or you want to see it split now as the other is a bad plan (tm)?

Olliver
>
>> +       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 from the eeprom */
>> +       if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
>> +                    CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index * (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 or EEPROM missing on device: ");
>> +               return -ENOSYS;
>> +       }
>> +       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 on device: ");
>> +               return -EINVAL;
>> +       }
>> +#endif
>> +#endif
>> +
>> +       memcpy(enetaddr, eeprom, ARP_HLEN);
>> +
>> +       return 0;
>> +}
>> +
>>   void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>>   {
>>          char *end;
>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>> index bf4de37..8fb5844 100644
>> --- a/net/eth_legacy.c
>> +++ b/net/eth_legacy.c
>> @@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>>          unsigned char env_enetaddr[ARP_HLEN];
>>          int ret = 0;
>>
>> +       eeprom_read_enetaddr(eth_number, dev->enetaddr);
>> +
>>          eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>>
>>          if (!is_zero_ethaddr(env_enetaddr)) {
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Hey Joe,


On 30-11-16 21:40, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> This patch adds a method for the board to set the MAC address if the
>> environment is not yet set. The environment based MAC addresses are not
>> touched, but if the fdt has an alias set, it is parsed and put into the
>> environment.
>>
>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
>> is parsed and inserted into the environment as eth2addr.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   common/fdt_support.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index c34a13c..f127392 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
>>          return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>   }
>>
>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
> Ugh. This collides with a function in board/v38b/ethaddr.c and in
> board/intercontrol/digsy_mtc/digsy_mtc.c
>
> Also, it's so generic, and only gets called by the fdt fixup stuff...
> This function should probably be named in such a way that its
> association with fdt is clear.
I did not notice that, sorry! But naming suggestions are welcome :)

Right now, I use it in two unrelated spots however.

from the fdt as seen above and in a subclass driver (which will come up 
for review) as suggested by Simon.

There I do:

+static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
+{
+       struct eth_pdata *pdata = dev_get_platdata(dev);
+
+       return board_get_enetaddr(dev->seq, pdata->enetaddr);
+}
+
  const struct eth_ops sunxi_gmac_eth_ops = {
         .start                  = designware_eth_start,
         .send                   = designware_eth_send,
@@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
         .free_pkt               = designware_eth_free_pkt,
         .stop                   = designware_eth_stop,
         .write_hwaddr           = designware_eth_write_hwaddr,
+       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
  };

which is completly unrelated to the fdt.

So naming suggestion or overal suggestion how to handle this nice and 
generically?

Based from the name however I would think that all board_get_enetaddr's 
work the same however so should have been interchangeable? Or was that 
silly thinking?

Olliver

>
>> +{
>> +       return -ENOSYS;
>> +}
>> +
>>   void fdt_fixup_ethernet(void *fdt)
>>   {
>>          int i, prop;
>> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
>>                          if (fdt_eth_addr)
>>                                  eth_parse_enetaddr(fdt_eth_addr, mac_addr);
>>                          else
>> -                               continue;
>> +                               if (board_get_enetaddr(i, mac_addr) < 0)
>> +                                       continue;
>>
>>                          do_fixup_by_path(fdt, path, "mac-address",
>>                                           &mac_addr, 6, 0);
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 04/14] net: core: Sanitize get/set operations for enetaddr

[Olliver Schinagl]

Hey Joe,


On 30-11-16 20:20, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> In the current net stack, we have a few functions to get and set
>> the "ethaddr" and "ethNaddr" environment variables, which use magic
>> values to get and set these environment variables. Remove the magicness
>> of the buffer by defining it proper and also check the input for its
>> length.
>>
>> Additionally use the define in fdt parser where the ethaddr variables
>> are also used.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   common/fdt_support.c |  2 +-
>>   include/net.h        |  1 +
>>   net/eth_common.c     | 12 ++++++++----
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index b082662..89e6e47 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -469,7 +469,7 @@ void fdt_fixup_ethernet(void *fdt)
>>   {
>>          int i, j, prop;
>>          char *tmp, *end;
>> -       char mac[16];
>> +       char mac[ETH_ENETADDR_LEN];
>>          const char *path;
>>          unsigned char mac_addr[ARP_HLEN];
>>          int offset;
>> diff --git a/include/net.h b/include/net.h
>> index 9cd7870..2534913 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -243,6 +243,7 @@ void eth_set_current(void);         /* set nterface to ethcur var */
>>
>>   int eth_get_dev_index(void);           /* get the device index */
>>   void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
>> +#define ETH_ENETADDR_LEN 32
> I'd prefer a clearer name here.
I'd agree, but
>
> Maybe ETH_ENETADDR_ENV_NAME_LEN?
I do find it to be quite a long name. Which makes the whole 80 charcters 
per line a thing.

But done :)
>
>>   int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>
>> diff --git a/net/eth_common.c b/net/eth_common.c
>> index e9d3c66..079be89 100644
>> --- a/net/eth_common.c
>> +++ b/net/eth_common.c
>> @@ -42,16 +42,20 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr)
>>   int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>>                                   uchar *enetaddr)
>>   {
>> -       char enetvar[32];
>> -       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
>> +       char enetvar[ETH_ENETADDR_LEN];
>> +
>> +       snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr",
>> +                base_name, index);
>>          return eth_getenv_enetaddr(enetvar, enetaddr);
>>   }
>>
>>   int eth_setenv_enetaddr_by_index(const char *base_name, int index,
>>                                   uchar *enetaddr)
>>   {
>> -       char enetvar[32];
>> -       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
>> +       char enetvar[ETH_ENETADDR_LEN];
>> +
>> +       snprintf(enetvar, ETH_ENETADDR_LEN, index ? "%s%daddr" : "%saddr",
>> +                base_name, index);
>>          return eth_setenv_enetaddr(enetvar, enetaddr);
>>   }
>>
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Simon Glass]

Hi Oliver,

On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Joe,
>
>
>
> On 30-11-16 21:40, Joe Hershberger wrote:
>>
>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>> wrote:
>>>
>>> This patch adds a method for the board to set the MAC address if the
>>> environment is not yet set. The environment based MAC addresses are not
>>> touched, but if the fdt has an alias set, it is parsed and put into the
>>> environment.
>>>
>>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
>>> is parsed and inserted into the environment as eth2addr.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   common/fdt_support.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>> index c34a13c..f127392 100644
>>> --- a/common/fdt_support.c
>>> +++ b/common/fdt_support.c
>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64
>>> size)
>>>          return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>>   }
>>>
>>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
>>
>> Ugh. This collides with a function in board/v38b/ethaddr.c and in
>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>
>> Also, it's so generic, and only gets called by the fdt fixup stuff...
>> This function should probably be named in such a way that its
>> association with fdt is clear.
>
> I did not notice that, sorry! But naming suggestions are welcome :)
>
> Right now, I use it in two unrelated spots however.
>
> from the fdt as seen above and in a subclass driver (which will come up for
> review) as suggested by Simon.
>
> There I do:
>
> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
> +{
> +       struct eth_pdata *pdata = dev_get_platdata(dev);
> +
> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
> +}
> +
>  const struct eth_ops sunxi_gmac_eth_ops = {
>         .start                  = designware_eth_start,
>         .send                   = designware_eth_send,
> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>         .free_pkt               = designware_eth_free_pkt,
>         .stop                   = designware_eth_stop,
>         .write_hwaddr           = designware_eth_write_hwaddr,
> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>  };
>
> which is completly unrelated to the fdt.
>
> So naming suggestion or overal suggestion how to handle this nice and
> generically?
>
> Based from the name however I would think that all board_get_enetaddr's work
> the same however so should have been interchangeable? Or was that silly
> thinking?

Would it be possible to use a name without 'board' in it? I think this
/ hope is actually sunxi-specific code, not board-specific?

Regards,
Simon

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Hey Simon,


On 05-12-16 07:24, Simon Glass wrote:
> Hi Oliver,
>
> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> Hey Joe,
>>
>>
>>
>> On 30-11-16 21:40, Joe Hershberger wrote:
>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>>> wrote:
>>>> This patch adds a method for the board to set the MAC address if the
>>>> environment is not yet set. The environment based MAC addresses are not
>>>> touched, but if the fdt has an alias set, it is parsed and put into the
>>>> environment.
>>>>
>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
>>>> is parsed and inserted into the environment as eth2addr.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>> ---
>>>>    common/fdt_support.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>> index c34a13c..f127392 100644
>>>> --- a/common/fdt_support.c
>>>> +++ b/common/fdt_support.c
>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64
>>>> size)
>>>>           return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>>>    }
>>>>
>>>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in
>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>
>>> Also, it's so generic, and only gets called by the fdt fixup stuff...
>>> This function should probably be named in such a way that its
>>> association with fdt is clear.
>> I did not notice that, sorry! But naming suggestions are welcome :)
>>
>> Right now, I use it in two unrelated spots however.
>>
>> from the fdt as seen above and in a subclass driver (which will come up for
>> review) as suggested by Simon.
>>
>> There I do:
>>
>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>> +{
>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>> +
>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>> +}
>> +
>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>          .start                  = designware_eth_start,
>>          .send                   = designware_eth_send,
>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>          .free_pkt               = designware_eth_free_pkt,
>>          .stop                   = designware_eth_stop,
>>          .write_hwaddr           = designware_eth_write_hwaddr,
>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>   };
>>
>> which is completly unrelated to the fdt.
>>
>> So naming suggestion or overal suggestion how to handle this nice and
>> generically?
>>
>> Based from the name however I would think that all board_get_enetaddr's work
>> the same however so should have been interchangeable? Or was that silly
>> thinking?
> Would it be possible to use a name without 'board' in it? I think this
> / hope is actually sunxi-specific code, not board-specific?
You are actually correct, we take the serial number of the SoC (sunxi 
specific) and generate a serial/MAC from it. So nothing to do with the 
board. So I can just name it sunxi_gen_enetaddr(). Would that be then 
(much) better?

The reason why I went to 'board' with my mind, is because a) the 
original mac gen code and b) the location was in board/sunxi/board.c. I 
think it is thus also sensible to move it out of board/sunxi/board.c as 
indeed, it has nothing to do with board(s).

Olliver
>
> Regards,
> Simon

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Simon Glass]

Hi Oliver,

On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Simon,
>
>
>
> On 05-12-16 07:24, Simon Glass wrote:
>>
>> Hi Oliver,
>>
>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl> wrote:
>>>
>>> Hey Joe,
>>>
>>>
>>>
>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>
>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>>>> wrote:
>>>>>
>>>>> This patch adds a method for the board to set the MAC address if the
>>>>> environment is not yet set. The environment based MAC addresses are not
>>>>> touched, but if the fdt has an alias set, it is parsed and put into the
>>>>> environment.
>>>>>
>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>> contains
>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>
>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>> ---
>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>> index c34a13c..f127392 100644
>>>>> --- a/common/fdt_support.c
>>>>> +++ b/common/fdt_support.c
>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64
>>>>> size)
>>>>>           return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>>>>    }
>>>>>
>>>>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
>>>>
>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in
>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>
>>>> Also, it's so generic, and only gets called by the fdt fixup stuff...
>>>> This function should probably be named in such a way that its
>>>> association with fdt is clear.
>>>
>>> I did not notice that, sorry! But naming suggestions are welcome :)
>>>
>>> Right now, I use it in two unrelated spots however.
>>>
>>> from the fdt as seen above and in a subclass driver (which will come up
>>> for
>>> review) as suggested by Simon.
>>>
>>> There I do:
>>>
>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>> +{
>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>> +
>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>> +}
>>> +
>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>          .start                  = designware_eth_start,
>>>          .send                   = designware_eth_send,
>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>          .free_pkt               = designware_eth_free_pkt,
>>>          .stop                   = designware_eth_stop,
>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>   };
>>>
>>> which is completly unrelated to the fdt.
>>>
>>> So naming suggestion or overal suggestion how to handle this nice and
>>> generically?
>>>
>>> Based from the name however I would think that all board_get_enetaddr's
>>> work
>>> the same however so should have been interchangeable? Or was that silly
>>> thinking?
>>
>> Would it be possible to use a name without 'board' in it? I think this
>> / hope is actually sunxi-specific code, not board-specific?
>
> You are actually correct, we take the serial number of the SoC (sunxi
> specific) and generate a serial/MAC from it. So nothing to do with the
> board. So I can just name it sunxi_gen_enetaddr(). Would that be then (much)
> better?
>
> The reason why I went to 'board' with my mind, is because a) the original
> mac gen code and b) the location was in board/sunxi/board.c. I think it is
> thus also sensible to move it out of board/sunxi/board.c as indeed, it has
> nothing to do with board(s).

That sounds good to me - and you should be able to call it directly
from the driver and avoid any weak functions, right?

Regards,
Simon

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> wrote:
>Hi Oliver,
>
>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl>
>wrote:
>> Hey Simon,
>>
>>
>>
>> On 05-12-16 07:24, Simon Glass wrote:
>>>
>>> Hi Oliver,
>>>
>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl>
>wrote:
>>>>
>>>> Hey Joe,
>>>>
>>>>
>>>>
>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>
>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
><oliver@schinagl.nl>
>>>>> wrote:
>>>>>>
>>>>>> This patch adds a method for the board to set the MAC address if
>the
>>>>>> environment is not yet set. The environment based MAC addresses
>are not
>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>into the
>>>>>> environment.
>>>>>>
>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>> contains
>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>however, it
>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>
>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>>> ---
>>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>> index c34a13c..f127392 100644
>>>>>> --- a/common/fdt_support.c
>>>>>> +++ b/common/fdt_support.c
>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start,
>u64
>>>>>> size)
>>>>>>           return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>>>>>    }
>>>>>>
>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>*mac_addr)
>>>>>
>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in
>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>
>>>>> Also, it's so generic, and only gets called by the fdt fixup
>stuff...
>>>>> This function should probably be named in such a way that its
>>>>> association with fdt is clear.
>>>>
>>>> I did not notice that, sorry! But naming suggestions are welcome :)
>>>>
>>>> Right now, I use it in two unrelated spots however.
>>>>
>>>> from the fdt as seen above and in a subclass driver (which will
>come up
>>>> for
>>>> review) as suggested by Simon.
>>>>
>>>> There I do:
>>>>
>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>> +{
>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>> +
>>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>> +}
>>>> +
>>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>>          .start                  = designware_eth_start,
>>>>          .send                   = designware_eth_send,
>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>          .free_pkt               = designware_eth_free_pkt,
>>>>          .stop                   = designware_eth_stop,
>>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>>   };
>>>>
>>>> which is completly unrelated to the fdt.
>>>>
>>>> So naming suggestion or overal suggestion how to handle this nice
>and
>>>> generically?
>>>>
>>>> Based from the name however I would think that all
>board_get_enetaddr's
>>>> work
>>>> the same however so should have been interchangeable? Or was that
>silly
>>>> thinking?
>>>
>>> Would it be possible to use a name without 'board' in it? I think
>this
>>> / hope is actually sunxi-specific code, not board-specific?
>>
>> You are actually correct, we take the serial number of the SoC (sunxi
>> specific) and generate a serial/MAC from it. So nothing to do with
>the
>> board. So I can just name it sunxi_gen_enetaddr(). Would that be then
>(much)
>> better?
>>
>> The reason why I went to 'board' with my mind, is because a) the
>original
>> mac gen code and b) the location was in board/sunxi/board.c. I think
>it is
>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>it has
>> nothing to do with board(s).
>
>That sounds good to me - and you should be able to call it directly
>from the driver and avoid any weak functions, right?
The subclass driver can, the fdt fixup however still needs a weak fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() i think.)
>
>Regards,
>Simon

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Simon Glass]

Hi Oliver,

On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl> wrote:
>
>
> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org> wrote:
>>Hi Oliver,
>>
>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl>
>>wrote:
>>> Hey Simon,
>>>
>>>
>>>
>>> On 05-12-16 07:24, Simon Glass wrote:
>>>>
>>>> Hi Oliver,
>>>>
>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl>
>>wrote:
>>>>>
>>>>> Hey Joe,
>>>>>
>>>>>
>>>>>
>>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>>
>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
>><oliver@schinagl.nl>
>>>>>> wrote:
>>>>>>>
>>>>>>> This patch adds a method for the board to set the MAC address if
>>the
>>>>>>> environment is not yet set. The environment based MAC addresses
>>are not
>>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>>into the
>>>>>>> environment.
>>>>>>>
>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>>> contains
>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>>however, it
>>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>>
>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>>>> ---
>>>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>> index c34a13c..f127392 100644
>>>>>>> --- a/common/fdt_support.c
>>>>>>> +++ b/common/fdt_support.c
>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start,
>>u64
>>>>>>> size)
>>>>>>>           return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>>>>>>    }
>>>>>>>
>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>>*mac_addr)
>>>>>>
>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and in
>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>>
>>>>>> Also, it's so generic, and only gets called by the fdt fixup
>>stuff...
>>>>>> This function should probably be named in such a way that its
>>>>>> association with fdt is clear.
>>>>>
>>>>> I did not notice that, sorry! But naming suggestions are welcome :)
>>>>>
>>>>> Right now, I use it in two unrelated spots however.
>>>>>
>>>>> from the fdt as seen above and in a subclass driver (which will
>>come up
>>>>> for
>>>>> review) as suggested by Simon.
>>>>>
>>>>> There I do:
>>>>>
>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>>> +{
>>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>> +
>>>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>>> +}
>>>>> +
>>>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>          .start                  = designware_eth_start,
>>>>>          .send                   = designware_eth_send,
>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>          .free_pkt               = designware_eth_free_pkt,
>>>>>          .stop                   = designware_eth_stop,
>>>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>>>   };
>>>>>
>>>>> which is completly unrelated to the fdt.
>>>>>
>>>>> So naming suggestion or overal suggestion how to handle this nice
>>and
>>>>> generically?
>>>>>
>>>>> Based from the name however I would think that all
>>board_get_enetaddr's
>>>>> work
>>>>> the same however so should have been interchangeable? Or was that
>>silly
>>>>> thinking?
>>>>
>>>> Would it be possible to use a name without 'board' in it? I think
>>this
>>>> / hope is actually sunxi-specific code, not board-specific?
>>>
>>> You are actually correct, we take the serial number of the SoC (sunxi
>>> specific) and generate a serial/MAC from it. So nothing to do with
>>the
>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be then
>>(much)
>>> better?
>>>
>>> The reason why I went to 'board' with my mind, is because a) the
>>original
>>> mac gen code and b) the location was in board/sunxi/board.c. I think
>>it is
>>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>>it has
>>> nothing to do with board(s).
>>
>>That sounds good to me - and you should be able to call it directly
>>from the driver and avoid any weak functions, right?
> The subclass driver can, the fdt fixup however still needs a weak fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr() i think.)

OK - I feel that the fdt fixups need a bit of thought. At the moment
it is all pretty ad-hoc.

Regards,
Simon

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Hey simon

On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote:
>Hi Oliver,
>
>On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl>
>wrote:
>>
>>
>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org>
>wrote:
>>>Hi Oliver,
>>>
>>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl>
>>>wrote:
>>>> Hey Simon,
>>>>
>>>>
>>>>
>>>> On 05-12-16 07:24, Simon Glass wrote:
>>>>>
>>>>> Hi Oliver,
>>>>>
>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl>
>>>wrote:
>>>>>>
>>>>>> Hey Joe,
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>>>
>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
>>><oliver@schinagl.nl>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> This patch adds a method for the board to set the MAC address
>if
>>>the
>>>>>>>> environment is not yet set. The environment based MAC addresses
>>>are not
>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>>>into the
>>>>>>>> environment.
>>>>>>>>
>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>>>> contains
>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>>>however, it
>>>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>>>
>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>>>>> ---
>>>>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>> index c34a13c..f127392 100644
>>>>>>>> --- a/common/fdt_support.c
>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64
>start,
>>>u64
>>>>>>>> size)
>>>>>>>>           return fdt_fixup_memory_banks(blob, &start, &size,
>1);
>>>>>>>>    }
>>>>>>>>
>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>>>*mac_addr)
>>>>>>>
>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and
>in
>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>>>
>>>>>>> Also, it's so generic, and only gets called by the fdt fixup
>>>stuff...
>>>>>>> This function should probably be named in such a way that its
>>>>>>> association with fdt is clear.
>>>>>>
>>>>>> I did not notice that, sorry! But naming suggestions are welcome
>:)
>>>>>>
>>>>>> Right now, I use it in two unrelated spots however.
>>>>>>
>>>>>> from the fdt as seen above and in a subclass driver (which will
>>>come up
>>>>>> for
>>>>>> review) as suggested by Simon.
>>>>>>
>>>>>> There I do:
>>>>>>
>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>>>> +{
>>>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>> +
>>>>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>>>> +}
>>>>>> +
>>>>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>          .start                  = designware_eth_start,
>>>>>>          .send                   = designware_eth_send,
>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>          .free_pkt               = designware_eth_free_pkt,
>>>>>>          .stop                   = designware_eth_stop,
>>>>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>>>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>>>>   };
>>>>>>
>>>>>> which is completly unrelated to the fdt.
>>>>>>
>>>>>> So naming suggestion or overal suggestion how to handle this nice
>>>and
>>>>>> generically?
>>>>>>
>>>>>> Based from the name however I would think that all
>>>board_get_enetaddr's
>>>>>> work
>>>>>> the same however so should have been interchangeable? Or was that
>>>silly
>>>>>> thinking?
>>>>>
>>>>> Would it be possible to use a name without 'board' in it? I think
>>>this
>>>>> / hope is actually sunxi-specific code, not board-specific?
>>>>
>>>> You are actually correct, we take the serial number of the SoC
>(sunxi
>>>> specific) and generate a serial/MAC from it. So nothing to do with
>>>the
>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be
>then
>>>(much)
>>>> better?
>>>>
>>>> The reason why I went to 'board' with my mind, is because a) the
>>>original
>>>> mac gen code and b) the location was in board/sunxi/board.c. I
>think
>>>it is
>>>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>>>it has
>>>> nothing to do with board(s).
>>>
>>>That sounds good to me - and you should be able to call it directly
>>>from the driver and avoid any weak functions, right?
>> The subclass driver can, the fdt fixup however still needs a weak
>fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr()
>i think.)
>
>OK - I feel that the fdt fixups need a bit of thought. At the moment
>it is all pretty ad-hoc.

Yeah i think i agree, but i guess thats a separate cleanup step generally, no?

>
>Regards,
>Simon

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[U-Boot] [PATCH 09/14] net: Add ability to set MAC address via EEPROM

[Joe Hershberger]

On Fri, Dec 2, 2016 at 4:12 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Joe,
>
>
>
> On 30-11-16 21:00, Joe Hershberger wrote:
>>
>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>> wrote:
>>>
>>> This patch allows Kconfig to enable and set parameters to make it
>>> possible to read the MAC address from an EEPROM. The net core layer then
>>> uses this information to read MAC addresses from this EEPROM.
>>>
>>> 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 and added.
>>>
>>> The code currently first checks if there is a non-zero MAC address in
>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>> used by a board to supply the MAC in other ways.
>>>
>>> If both these fails, the other code is still in place to query the
>>> environent, which then can be used to override the hardware supplied
>>> data.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   doc/README.enetaddr | 99
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/net.h       | 14 ++++++++
>>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>   net/eth-uclass.c    |  9 +++--
>>>   net/eth_common.c    | 34 ++++++++++++++++++
>>>   net/eth_legacy.c    |  2 ++
>>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>>

...

>>> diff --git a/net/eth_common.c b/net/eth_common.c
>>> index 079be89..e0d8b62 100644
>>> --- a/net/eth_common.c
>>> +++ b/net/eth_common.c
>>> @@ -8,10 +8,44 @@
>>>
>>>   #include <common.h>
>>>   #include <dm.h>
>>> +#include <i2c.h>
>>>   #include <miiphy.h>
>>>   #include <net.h>
>>>   #include "eth_internal.h"
>>>
>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr)
>>> +{
>>> +       uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
>>> +#if defined(CONFIG_NET_ETHADDR_EEPROM) &&
>>> defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
>>
>> Since it is easily reasonable that SPI PROM is a likely useful
>> support, why not keep the layout stuff separate from the I2C stuff so
>> that it is trivial to plug in a different bus later? It will also make
>> the code clearer by untangling these.
>
>
> I strongly agree, but I recommend a follow up patch series (and thus merge
> this as is for now) to use Maxime's EEPROM framework patches. So then this
> gets replaced by simple read from eeprom.
>
> So yes, I have contemplated in splitting it up now and have a simply
> read_from_i2c() kind of function, I figured this gets solved elsewhere
> anyway.
>
> Additionally, the layout stuff would ideally be replaced by Igor (i think it
> was) eeprom layout framework (if those two combine) which solves both
> problems in one go.
>
> Or you want to see it split now as the other is a bad plan (tm)?

It's fine to wait if there is a plan going forward with a dependency
that might make this throw-away work.

> Olliver
>
>>
>>> +       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 from the
>>> eeprom */
>>> +       if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
>>> +                    CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index *
>>> (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 or EEPROM missing on
>>> device: ");
>>> +               return -ENOSYS;
>>> +       }
>>> +       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 on device: ");
>>> +               return -EINVAL;
>>> +       }
>>> +#endif
>>> +#endif
>>> +
>>> +       memcpy(enetaddr, eeprom, ARP_HLEN);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>>>   {
>>>          char *end;
>>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>>> index bf4de37..8fb5844 100644
>>> --- a/net/eth_legacy.c
>>> +++ b/net/eth_legacy.c
>>> @@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const
>>> char *base_name,
>>>          unsigned char env_enetaddr[ARP_HLEN];
>>>          int ret = 0;
>>>
>>> +       eeprom_read_enetaddr(eth_number, dev->enetaddr);
>>> +
>>>          eth_getenv_enetaddr_by_index(base_name, eth_number,
>>> env_enetaddr);
>>>
>>>          if (!is_zero_ethaddr(env_enetaddr)) {
>>> --
>>> 2.10.2
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>
>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Simon Glass]

Hi Oliver,

On 9 December 2016 at 02:25, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey simon
>
> On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote:
>>Hi Oliver,
>>
>>On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl>
>>wrote:
>>>
>>>
>>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org>
>>wrote:
>>>>Hi Oliver,
>>>>
>>>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl>
>>>>wrote:
>>>>> Hey Simon,
>>>>>
>>>>>
>>>>>
>>>>> On 05-12-16 07:24, Simon Glass wrote:
>>>>>>
>>>>>> Hi Oliver,
>>>>>>
>>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl>
>>>>wrote:
>>>>>>>
>>>>>>> Hey Joe,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>>>>
>>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
>>>><oliver@schinagl.nl>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> This patch adds a method for the board to set the MAC address
>>if
>>>>the
>>>>>>>>> environment is not yet set. The environment based MAC addresses
>>>>are not
>>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>>>>into the
>>>>>>>>> environment.
>>>>>>>>>
>>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>>>>> contains
>>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>>>>however, it
>>>>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>>>>>> ---
>>>>>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>> index c34a13c..f127392 100644
>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64
>>start,
>>>>u64
>>>>>>>>> size)
>>>>>>>>>           return fdt_fixup_memory_banks(blob, &start, &size,
>>1);
>>>>>>>>>    }
>>>>>>>>>
>>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>>>>*mac_addr)
>>>>>>>>
>>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and
>>in
>>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>>>>
>>>>>>>> Also, it's so generic, and only gets called by the fdt fixup
>>>>stuff...
>>>>>>>> This function should probably be named in such a way that its
>>>>>>>> association with fdt is clear.
>>>>>>>
>>>>>>> I did not notice that, sorry! But naming suggestions are welcome
>>:)
>>>>>>>
>>>>>>> Right now, I use it in two unrelated spots however.
>>>>>>>
>>>>>>> from the fdt as seen above and in a subclass driver (which will
>>>>come up
>>>>>>> for
>>>>>>> review) as suggested by Simon.
>>>>>>>
>>>>>>> There I do:
>>>>>>>
>>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>>>>> +{
>>>>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>>> +
>>>>>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>>>>> +}
>>>>>>> +
>>>>>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>          .start                  = designware_eth_start,
>>>>>>>          .send                   = designware_eth_send,
>>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>          .free_pkt               = designware_eth_free_pkt,
>>>>>>>          .stop                   = designware_eth_stop,
>>>>>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>>>>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>>>>>   };
>>>>>>>
>>>>>>> which is completly unrelated to the fdt.
>>>>>>>
>>>>>>> So naming suggestion or overal suggestion how to handle this nice
>>>>and
>>>>>>> generically?
>>>>>>>
>>>>>>> Based from the name however I would think that all
>>>>board_get_enetaddr's
>>>>>>> work
>>>>>>> the same however so should have been interchangeable? Or was that
>>>>silly
>>>>>>> thinking?
>>>>>>
>>>>>> Would it be possible to use a name without 'board' in it? I think
>>>>this
>>>>>> / hope is actually sunxi-specific code, not board-specific?
>>>>>
>>>>> You are actually correct, we take the serial number of the SoC
>>(sunxi
>>>>> specific) and generate a serial/MAC from it. So nothing to do with
>>>>the
>>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be
>>then
>>>>(much)
>>>>> better?
>>>>>
>>>>> The reason why I went to 'board' with my mind, is because a) the
>>>>original
>>>>> mac gen code and b) the location was in board/sunxi/board.c. I
>>think
>>>>it is
>>>>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>>>>it has
>>>>> nothing to do with board(s).
>>>>
>>>>That sounds good to me - and you should be able to call it directly
>>>>from the driver and avoid any weak functions, right?
>>> The subclass driver can, the fdt fixup however still needs a weak
>>fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr()
>>i think.)
>>
>>OK - I feel that the fdt fixups need a bit of thought. At the moment
>>it is all pretty ad-hoc.
>
> Yeah i think i agree, but i guess thats a separate cleanup step generally, no?

OK - are you able to take a look at this?

Regards,
Simon

[U-Boot] [PATCH 07/14] net: core: Inform the user of the device MAC address

[Joe Hershberger]

Hi Olliver,

On Fri, Dec 2, 2016 at 4:08 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Joe,
>
>
>
> On 30-11-16 20:29, Joe Hershberger wrote:
>>
>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>> wrote:
>>>
>>> In certain conditions we currently print the MAC address. For example a
>>> warning when a random mac address is in use or a missmatch between HW
>>> and ENV.
>>>
>>> If all things went well however (but even if there is a miss-match) we
>>> do not inform the user what the final MAC address of the device is.
>>>
>>> Lets print the final MAC address of the device with which it has been
>>> setup.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   net/eth-uclass.c | 9 ++++++---
>>>   net/eth_legacy.c | 3 +++
>>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>> index aca3f6d..5c888b8 100644
>>> --- a/net/eth-uclass.c
>>> +++ b/net/eth-uclass.c
>>> @@ -413,11 +413,12 @@ int eth_initialize(void)
>>>                  }
>>>
>>>                  bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT);
>>> +               putc('\n');
>>>                  do {
>>> -                       if (num_devices)
>>> -                               printf(", ");
>>> +                       struct eth_pdata *pdata = dev->platdata;
>>>
>>> -                       printf("eth%d: %s", dev->seq, dev->name);
>>> +                       printf("eth%d:  %s [%pM]\n", dev->seq, dev->name,
>>> +                                                    pdata->enetaddr);
>>>
>>>                          if (ethprime && dev == prime_dev)
>>>                                  printf(" [PRIME]");
>>> @@ -522,6 +523,8 @@ static int eth_post_probe(struct udevice *dev)
>>>   #endif
>>>          }
>>>
>>> +       printf("%s ", dev->name);
>>> +
>>
>> Why this?
>>
>> Can you send to the list what an example output looks like?
>
>
> Absolutly. Right now I have this, with the v2 work I'm doing to print the
> mac source.

Was there a v2?

> Net:   ethernet at 01c50000 <other etnernet devices>
> eth0:  ethernet at 01c50000 [cc:bd:d3:00:01:c6] (EEPROM)
> eth1: <other_ethernet_device1> [<other_mac>] (<other_source>)
>
>
> If there is an error or warning from the net layer (during probe, during
> init etc)
> we get something like
>
> Net:   CRC error on MAC address from EEPROM on device: ethernet at 01c50000
> <other warning> on device: <other_device>
> eth0:  ethernet at 01c50000 [02:4b:04:42:12:31] (driver)
>
> (where driver is the read_rom_hwaddr hook a driver supplies).
>
> I did not go over all error messages to print it as pretty. So in that case
> it will be
> Net:   Some error. ethernet at 01c50000
>
> Olliver
>
>>
>> Thanks,
>> -Joe
>>
>>>          return 0;
>>>   }
>>>
>>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>>> index 2b2c2de..bf4de37 100644
>>> --- a/net/eth_legacy.c
>>> +++ b/net/eth_legacy.c
>>> @@ -178,6 +178,9 @@ int eth_write_hwaddr(struct eth_device *dev, const
>>> char *base_name,
>>>                                 dev->name);
>>>          }
>>>
>>> +       printf("%s (eth%d) has MAC address: %pM\n",
>>> +              dev->name, eth_number, dev->enetaddr);
>>> +
>>>          return ret;
>>>   }
>>>
>>> --
>>> 2.10.2
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>
>

[U-Boot] net: cosmetic: Do not use magic values for ARP_HLEN

[Joe Hershberger]

Hi oliver at schinagl.nl,

https://patchwork.ozlabs.org/patch/699274/ was applied to u-boot-net.git.

Thanks!
-Joe

[U-Boot] net: cosmetic: Make the MAC address string less magical

[Joe Hershberger]

Hi oliver at schinagl.nl,

https://patchwork.ozlabs.org/patch/699273/ was applied to u-boot-net.git.

Thanks!
-Joe

[U-Boot] net: cosmetic: Define ethernet name length

[Joe Hershberger]

Hi oliver at schinagl.nl,

https://patchwork.ozlabs.org/patch/699278/ was applied to u-boot-net.git.

Thanks!
-Joe

[U-Boot] net: core: cosmetic: A MAC address is not limited to SROM

[Joe Hershberger]

Hi oliver at schinagl.nl,

https://patchwork.ozlabs.org/patch/699283/ was applied to u-boot-net.git.

Thanks!
-Joe

[U-Boot] tools: Allow crc8 to be used

[Joe Hershberger]

Hi oliver at schinagl.nl,

https://patchwork.ozlabs.org/patch/699401/ was applied to u-boot-net.git.

Thanks!
-Joe

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

[Joe Hershberger]

Hi oliver at schinagl.nl,

https://patchwork.ozlabs.org/patch/699397/ was applied to u-boot-net.git.

Thanks!
-Joe

[U-Boot] [PATCH 07/14] net: core: Inform the user of the device MAC address

[Olliver Schinagl]

Hey Joe,

On 02-02-17 18:52, Joe Hershberger wrote:
> Hi Olliver,
>
> On Fri, Dec 2, 2016 at 4:08 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> Hey Joe,
>>
>>
>>
>> On 30-11-16 20:29, Joe Hershberger wrote:
>>>
>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>>> wrote:
>>>>
>>>> In certain conditions we currently print the MAC address. For example a
>>>> warning when a random mac address is in use or a missmatch between HW
>>>> and ENV.
>>>>
>>>> If all things went well however (but even if there is a miss-match) we
>>>> do not inform the user what the final MAC address of the device is.
>>>>
>>>> Lets print the final MAC address of the device with which it has been
>>>> setup.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>> ---
>>>>   net/eth-uclass.c | 9 ++++++---
>>>>   net/eth_legacy.c | 3 +++
>>>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>>> index aca3f6d..5c888b8 100644
>>>> --- a/net/eth-uclass.c
>>>> +++ b/net/eth-uclass.c
>>>> @@ -413,11 +413,12 @@ int eth_initialize(void)
>>>>                  }
>>>>
>>>>                  bootstage_mark(BOOTSTAGE_ID_NET_ETH_INIT);
>>>> +               putc('\n');
>>>>                  do {
>>>> -                       if (num_devices)
>>>> -                               printf(", ");
>>>> +                       struct eth_pdata *pdata = dev->platdata;
>>>>
>>>> -                       printf("eth%d: %s", dev->seq, dev->name);
>>>> +                       printf("eth%d:  %s [%pM]\n", dev->seq, dev->name,
>>>> +                                                    pdata->enetaddr);
>>>>
>>>>                          if (ethprime && dev == prime_dev)
>>>>                                  printf(" [PRIME]");
>>>> @@ -522,6 +523,8 @@ static int eth_post_probe(struct udevice *dev)
>>>>   #endif
>>>>          }
>>>>
>>>> +       printf("%s ", dev->name);
>>>> +
>>>
>>> Why this?
>>>
>>> Can you send to the list what an example output looks like?
>>
>>
>> Absolutly. Right now I have this, with the v2 work I'm doing to print the
>> mac source.
>
> Was there a v2?
I think so, I will go over my mailbox and my local commits to double 
check and send the v2 (or v3) if I missed it.
>
>> Net:   ethernet at 01c50000 <other etnernet devices>
>> eth0:  ethernet at 01c50000 [cc:bd:d3:00:01:c6] (EEPROM)
>> eth1: <other_ethernet_device1> [<other_mac>] (<other_source>)
>>
>>
>> If there is an error or warning from the net layer (during probe, during
>> init etc)
>> we get something like
>>
>> Net:   CRC error on MAC address from EEPROM on device: ethernet at 01c50000
>> <other warning> on device: <other_device>
>> eth0:  ethernet at 01c50000 [02:4b:04:42:12:31] (driver)
>>
>> (where driver is the read_rom_hwaddr hook a driver supplies).
>>
>> I did not go over all error messages to print it as pretty. So in that case
>> it will be
>> Net:   Some error. ethernet at 01c50000
>>
>> Olliver
>>
>>>
>>> Thanks,
>>> -Joe
>>>
>>>>          return 0;
>>>>   }
>>>>
>>>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>>>> index 2b2c2de..bf4de37 100644
>>>> --- a/net/eth_legacy.c
>>>> +++ b/net/eth_legacy.c
>>>> @@ -178,6 +178,9 @@ int eth_write_hwaddr(struct eth_device *dev, const
>>>> char *base_name,
>>>>                                 dev->name);
>>>>          }
>>>>
>>>> +       printf("%s (eth%d) has MAC address: %pM\n",
>>>> +              dev->name, eth_number, dev->enetaddr);
>>>> +
>>>>          return ret;
>>>>   }
>>>>
>>>> --
>>>> 2.10.2
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Joe Hershberger]

Hi Oliver,

On Sun, Dec 11, 2016 at 3:27 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Oliver,
>
> On 9 December 2016 at 02:25, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> Hey simon
>>
>> On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote:
>>>Hi Oliver,
>>>
>>>On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl>
>>>wrote:
>>>>
>>>>
>>>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org>
>>>wrote:
>>>>>Hi Oliver,
>>>>>
>>>>>On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl>
>>>>>wrote:
>>>>>> Hey Simon,
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 05-12-16 07:24, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi Oliver,
>>>>>>>
>>>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl>
>>>>>wrote:
>>>>>>>>
>>>>>>>> Hey Joe,
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>>>>>
>>>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
>>>>><oliver@schinagl.nl>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> This patch adds a method for the board to set the MAC address
>>>if
>>>>>the
>>>>>>>>>> environment is not yet set. The environment based MAC addresses
>>>>>are not
>>>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>>>>>into the
>>>>>>>>>> environment.
>>>>>>>>>>
>>>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>>>>>> contains
>>>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>>>>>however, it
>>>>>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>>>>>>> ---
>>>>>>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>>> index c34a13c..f127392 100644
>>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64
>>>start,
>>>>>u64
>>>>>>>>>> size)
>>>>>>>>>>           return fdt_fixup_memory_banks(blob, &start, &size,
>>>1);
>>>>>>>>>>    }
>>>>>>>>>>
>>>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>>>>>*mac_addr)
>>>>>>>>>
>>>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and
>>>in
>>>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>>>>>
>>>>>>>>> Also, it's so generic, and only gets called by the fdt fixup
>>>>>stuff...
>>>>>>>>> This function should probably be named in such a way that its
>>>>>>>>> association with fdt is clear.
>>>>>>>>
>>>>>>>> I did not notice that, sorry! But naming suggestions are welcome
>>>:)
>>>>>>>>
>>>>>>>> Right now, I use it in two unrelated spots however.
>>>>>>>>
>>>>>>>> from the fdt as seen above and in a subclass driver (which will
>>>>>come up
>>>>>>>> for
>>>>>>>> review) as suggested by Simon.
>>>>>>>>
>>>>>>>> There I do:
>>>>>>>>
>>>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>>>>>> +{
>>>>>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>>>> +
>>>>>>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>>          .start                  = designware_eth_start,
>>>>>>>>          .send                   = designware_eth_send,
>>>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>>          .free_pkt               = designware_eth_free_pkt,
>>>>>>>>          .stop                   = designware_eth_stop,
>>>>>>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>>>>>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>>>>>>   };
>>>>>>>>
>>>>>>>> which is completly unrelated to the fdt.
>>>>>>>>
>>>>>>>> So naming suggestion or overal suggestion how to handle this nice
>>>>>and
>>>>>>>> generically?
>>>>>>>>
>>>>>>>> Based from the name however I would think that all
>>>>>board_get_enetaddr's
>>>>>>>> work
>>>>>>>> the same however so should have been interchangeable? Or was that
>>>>>silly
>>>>>>>> thinking?
>>>>>>>
>>>>>>> Would it be possible to use a name without 'board' in it? I think
>>>>>this
>>>>>>> / hope is actually sunxi-specific code, not board-specific?
>>>>>>
>>>>>> You are actually correct, we take the serial number of the SoC
>>>(sunxi
>>>>>> specific) and generate a serial/MAC from it. So nothing to do with
>>>>>the
>>>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be
>>>then
>>>>>(much)
>>>>>> better?
>>>>>>
>>>>>> The reason why I went to 'board' with my mind, is because a) the
>>>>>original
>>>>>> mac gen code and b) the location was in board/sunxi/board.c. I
>>>think
>>>>>it is
>>>>>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>>>>>it has
>>>>>> nothing to do with board(s).
>>>>>
>>>>>That sounds good to me - and you should be able to call it directly
>>>>>from the driver and avoid any weak functions, right?
>>>> The subclass driver can, the fdt fixup however still needs a weak
>>>fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr()
>>>i think.)
>>>
>>>OK - I feel that the fdt fixups need a bit of thought. At the moment
>>>it is all pretty ad-hoc.
>>
>> Yeah i think i agree, but i guess thats a separate cleanup step generally, no?
>
> OK - are you able to take a look at this?

Any update on this or any of the other patches in your series that I
did not apply last release? I was expecting a v2 to address some
things such as this patch.

Thanks!
-Joe

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Hey Joe,

On 26-03-17 16:10, Joe Hershberger wrote:
> Hi Oliver,
>
> On Sun, Dec 11, 2016 at 3:27 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Oliver,
>>
>> On 9 December 2016 at 02:25, Olliver Schinagl <oliver@schinagl.nl> wrote:
>>> Hey simon
>>>
>>> On December 8, 2016 11:21:32 PM CET, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Oliver,
>>>>
>>>> On 7 December 2016 at 02:26, Olliver Schinagl <oliver@schinagl.nl>
>>>> wrote:
>>>>>
>>>>>
>>>>> On December 7, 2016 4:47:23 AM CET, Simon Glass <sjg@chromium.org>
>>>> wrote:
>>>>>> Hi Oliver,
>>>>>>
>>>>>> On 5 December 2016 at 03:28, Olliver Schinagl <oliver@schinagl.nl>
>>>>>> wrote:
>>>>>>> Hey Simon,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 05-12-16 07:24, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Hi Oliver,
>>>>>>>>
>>>>>>>> On 2 December 2016 at 03:16, Olliver Schinagl <oliver@schinagl.nl>
>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hey Joe,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 30-11-16 21:40, Joe Hershberger wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl
>>>>>> <oliver@schinagl.nl>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> This patch adds a method for the board to set the MAC address
>>>> if
>>>>>> the
>>>>>>>>>>> environment is not yet set. The environment based MAC addresses
>>>>>> are not
>>>>>>>>>>> touched, but if the fdt has an alias set, it is parsed and put
>>>>>> into the
>>>>>>>>>>> environment.
>>>>>>>>>>>
>>>>>>>>>>> E.g. The environment contains ethaddr and eth1addr, and the fdt
>>>>>>>>>>> contains
>>>>>>>>>>> an ethernet1 nothing happens. If the fdt contains ethernet2
>>>>>> however, it
>>>>>>>>>>> is parsed and inserted into the environment as eth2addr.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>>>>>>>> ---
>>>>>>>>>>>    common/fdt_support.c | 8 +++++++-
>>>>>>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>>>>>>>> index c34a13c..f127392 100644
>>>>>>>>>>> --- a/common/fdt_support.c
>>>>>>>>>>> +++ b/common/fdt_support.c
>>>>>>>>>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64
>>>> start,
>>>>>> u64
>>>>>>>>>>> size)
>>>>>>>>>>>           return fdt_fixup_memory_banks(blob, &start, &size,
>>>> 1);
>>>>>>>>>>>    }
>>>>>>>>>>>
>>>>>>>>>>> +__weak int board_get_enetaddr(const int i, unsigned char
>>>>>> *mac_addr)
>>>>>>>>>>
>>>>>>>>>> Ugh. This collides with a function in board/v38b/ethaddr.c and
>>>> in
>>>>>>>>>> board/intercontrol/digsy_mtc/digsy_mtc.c
>>>>>>>>>>
>>>>>>>>>> Also, it's so generic, and only gets called by the fdt fixup
>>>>>> stuff...
>>>>>>>>>> This function should probably be named in such a way that its
>>>>>>>>>> association with fdt is clear.
>>>>>>>>>
>>>>>>>>> I did not notice that, sorry! But naming suggestions are welcome
>>>> :)
>>>>>>>>>
>>>>>>>>> Right now, I use it in two unrelated spots however.
>>>>>>>>>
>>>>>>>>> from the fdt as seen above and in a subclass driver (which will
>>>>>> come up
>>>>>>>>> for
>>>>>>>>> review) as suggested by Simon.
>>>>>>>>>
>>>>>>>>> There I do:
>>>>>>>>>
>>>>>>>>> +static int sunxi_gmac_eth_read_rom_hwaddr(struct udevice *dev)
>>>>>>>>> +{
>>>>>>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>>>>> +
>>>>>>>>> +       return board_get_enetaddr(dev->seq, pdata->enetaddr);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>   const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>>>          .start                  = designware_eth_start,
>>>>>>>>>          .send                   = designware_eth_send,
>>>>>>>>> @@ -102,6 +110,7 @@ const struct eth_ops sunxi_gmac_eth_ops = {
>>>>>>>>>          .free_pkt               = designware_eth_free_pkt,
>>>>>>>>>          .stop                   = designware_eth_stop,
>>>>>>>>>          .write_hwaddr           = designware_eth_write_hwaddr,
>>>>>>>>> +       .read_rom_hwaddr        = sunxi_gmac_eth_read_rom_hwaddr,
>>>>>>>>>   };
>>>>>>>>>
>>>>>>>>> which is completly unrelated to the fdt.
>>>>>>>>>
>>>>>>>>> So naming suggestion or overal suggestion how to handle this nice
>>>>>> and
>>>>>>>>> generically?
>>>>>>>>>
>>>>>>>>> Based from the name however I would think that all
>>>>>> board_get_enetaddr's
>>>>>>>>> work
>>>>>>>>> the same however so should have been interchangeable? Or was that
>>>>>> silly
>>>>>>>>> thinking?
>>>>>>>>
>>>>>>>> Would it be possible to use a name without 'board' in it? I think
>>>>>> this
>>>>>>>> / hope is actually sunxi-specific code, not board-specific?
>>>>>>>
>>>>>>> You are actually correct, we take the serial number of the SoC
>>>> (sunxi
>>>>>>> specific) and generate a serial/MAC from it. So nothing to do with
>>>>>> the
>>>>>>> board. So I can just name it sunxi_gen_enetaddr(). Would that be
>>>> then
>>>>>> (much)
>>>>>>> better?
>>>>>>>
>>>>>>> The reason why I went to 'board' with my mind, is because a) the
>>>>>> original
>>>>>>> mac gen code and b) the location was in board/sunxi/board.c. I
>>>> think
>>>>>> it is
>>>>>>> thus also sensible to move it out of board/sunxi/board.c as indeed,
>>>>>> it has
>>>>>>> nothing to do with board(s).
>>>>>>
>>>>>> That sounds good to me - and you should be able to call it directly
>>>>> >from the driver and avoid any weak functions, right?
>>>>> The subclass driver can, the fdt fixup however still needs a weak
>>>> fdt_get_enetaddr()? (Which in our case calls then sunxi_get_enetaddr()
>>>> i think.)
>>>>
>>>> OK - I feel that the fdt fixups need a bit of thought. At the moment
>>>> it is all pretty ad-hoc.
>>>
>>> Yeah i think i agree, but i guess thats a separate cleanup step generally, no?
>>
>> OK - are you able to take a look at this?
>
> Any update on this or any of the other patches in your series that I
> did not apply last release? I was expecting a v2 to address some
> things such as this patch.

I have a few after rebasing u-boot-net/master and I just started 
yesterday to get back up to speed. I'll double check all review comments 
and send a v2 with the remaining patches. Sorry for taking so long here!

Olliver

>
> Thanks!
> -Joe
>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Hey Simon,

On 01-12-16 03:20, Simon Glass wrote:
> Hi,
>
> On 25 November 2016 at 08:30, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> This patch adds a method for the board to set the MAC address if the
>> environment is not yet set. The environment based MAC addresses are not
>> touched, but if the fdt has an alias set, it is parsed and put into the
>> environment.
>>
>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
>> is parsed and inserted into the environment as eth2addr.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>  common/fdt_support.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index c34a13c..f127392 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
>>         return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>  }
>>
>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
>> +{
>> +       return -ENOSYS;
>> +}
>> +
>>  void fdt_fixup_ethernet(void *fdt)
>>  {
>>         int i, prop;
>> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
>>                         if (fdt_eth_addr)
>>                                 eth_parse_enetaddr(fdt_eth_addr, mac_addr);
>>                         else
>> -                               continue;
>> +                               if (board_get_enetaddr(i, mac_addr) < 0)
>> +                                       continue;
>>
>>                         do_fixup_by_path(fdt, path, "mac-address",
>>                                          &mac_addr, 6, 0);
>> --
>> 2.10.2
>>
>
> Much as I don't want to pin this on any one patch, but I feel that our
> DT fixup stuff is a bit out of control. We have so many functions that
> do this, and they are called from various places. At some point we
> need to think about the infrastructure.
>
> IMO we should move to a linker list approach a bit like SPL did
> recently (SPL_LOAD_IMAGE_METHOD). Then boards and subsystems can
> register (at build-time) a fixup routine and they all get called one
> after the other.
>
> We could also (later) add run-time support for registering fixups,
> that drivers could use.

We kinda left this out in the cold last time (or I did?) and I just sent 
my v5 of this patch-series (without this one).

The gist from what I remember here was, to not introduce a weak function 
here, and have boards go back-forth to other subsystems. I belive one 
suggestion was to use a more specific name, lets say sunxi_get_hwaddr() 
for example and use that. Quite obviously this being the 
common/fdt_support, it would have to be generic.

So I'm still at loss as to how to handle this here.

Some background here, what the idea was I belive, that certain network 
adapters rely on the fdt to supply the MAC address to the driver, and so 
u-boot injects the mac address into the fdt.

We currently hack this together I belive, by generating environment 
variables for non-existant devices (if the fdt has ethernet aliases) and 
when doing this bit of code, inject the generated environment variables 
into the fdt.

I can't rewrite/fixup the fdt stuff, but am also not sure what an 
acceptable solution for this would be. Granted we really are hacking 
this into u-boot but I suppose we cannot escape this for legacy reasons. 
Devices expect the same generated mac address as before.

Again, bar rewriting this stuff, what can we do as the current code in 
board/sunxi/board.c is just as ugly a wart.

Olliver

>
> Regards,
> Simon
>

[U-Boot] [PATCH 12/14] fdt: eth_fixup: Add hook for board to override MAC

[Olliver Schinagl]

Resend  with maxime's correct e-mail address and Jagain added to CC

On 10-04-17 17:46, Olliver Schinagl wrote:
> Hey Simon,
>
> On 01-12-16 03:20, Simon Glass wrote:
>> Hi,
>>
>> On 25 November 2016 at 08:30, Olliver Schinagl <oliver@schinagl.nl>
>> wrote:
>>> This patch adds a method for the board to set the MAC address if the
>>> environment is not yet set. The environment based MAC addresses are not
>>> touched, but if the fdt has an alias set, it is parsed and put into the
>>> environment.
>>>
>>> E.g. The environment contains ethaddr and eth1addr, and the fdt contains
>>> an ethernet1 nothing happens. If the fdt contains ethernet2 however, it
>>> is parsed and inserted into the environment as eth2addr.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>  common/fdt_support.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>> index c34a13c..f127392 100644
>>> --- a/common/fdt_support.c
>>> +++ b/common/fdt_support.c
>>> @@ -465,6 +465,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64
>>> size)
>>>         return fdt_fixup_memory_banks(blob, &start, &size, 1);
>>>  }
>>>
>>> +__weak int board_get_enetaddr(const int i, unsigned char *mac_addr)
>>> +{
>>> +       return -ENOSYS;
>>> +}
>>> +
>>>  void fdt_fixup_ethernet(void *fdt)
>>>  {
>>>         int i, prop;
>>> @@ -507,7 +512,8 @@ void fdt_fixup_ethernet(void *fdt)
>>>                         if (fdt_eth_addr)
>>>                                 eth_parse_enetaddr(fdt_eth_addr,
>>> mac_addr);
>>>                         else
>>> -                               continue;
>>> +                               if (board_get_enetaddr(i, mac_addr) < 0)
>>> +                                       continue;
>>>
>>>                         do_fixup_by_path(fdt, path, "mac-address",
>>>                                          &mac_addr, 6, 0);
>>> --
>>> 2.10.2
>>>
>>
>> Much as I don't want to pin this on any one patch, but I feel that our
>> DT fixup stuff is a bit out of control. We have so many functions that
>> do this, and they are called from various places. At some point we
>> need to think about the infrastructure.
>>
>> IMO we should move to a linker list approach a bit like SPL did
>> recently (SPL_LOAD_IMAGE_METHOD). Then boards and subsystems can
>> register (at build-time) a fixup routine and they all get called one
>> after the other.
>>
>> We could also (later) add run-time support for registering fixups,
>> that drivers could use.
>
> We kinda left this out in the cold last time (or I did?) and I just sent
> my v5 of this patch-series (without this one).
>
> The gist from what I remember here was, to not introduce a weak function
> here, and have boards go back-forth to other subsystems. I belive one
> suggestion was to use a more specific name, lets say sunxi_get_hwaddr()
> for example and use that. Quite obviously this being the
> common/fdt_support, it would have to be generic.
>
> So I'm still at loss as to how to handle this here.
>
> Some background here, what the idea was I belive, that certain network
> adapters rely on the fdt to supply the MAC address to the driver, and so
> u-boot injects the mac address into the fdt.
>
> We currently hack this together I belive, by generating environment
> variables for non-existant devices (if the fdt has ethernet aliases) and
> when doing this bit of code, inject the generated environment variables
> into the fdt.
>
> I can't rewrite/fixup the fdt stuff, but am also not sure what an
> acceptable solution for this would be. Granted we really are hacking
> this into u-boot but I suppose we cannot escape this for legacy reasons.
> Devices expect the same generated mac address as before.
>
> Again, bar rewriting this stuff, what can we do as the current code in
> board/sunxi/board.c is just as ugly a wart.
>
> Olliver
>
>>
>> Regards,
>> Simon
>>