[U-Boot] [PATCHv4] Retrieve MAC address from EEPROM
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.
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;
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.
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
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);@@ -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); }@@ -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);@@ -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;
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
Hi oliver@schinagl.nl,
https://patchwork.ozlabs.org/patch/699274/ was applied to u-boot-net.git.
Thanks! -Joe
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);
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
Hi oliver@schinagl.nl,
https://patchwork.ozlabs.org/patch/699273/ was applied to u-boot-net.git.
Thanks! -Joe
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;
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
Hi oliver@schinagl.nl,
https://patchwork.ozlabs.org/patch/699278/ was applied to u-boot-net.git.
Thanks! -Joe
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); }
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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);
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
Hi oliver@schinagl.nl,
https://patchwork.ozlabs.org/patch/699283/ was applied to u-boot-net.git.
Thanks! -Joe
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);
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);#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
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);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
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)) {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
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)) {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
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)) {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
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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; }
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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@01c50000 <other etnernet devices> eth0: ethernet@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@01c50000 <other warning> on device: <other_device> eth0: ethernet@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@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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@01c50000 <other etnernet devices> eth0: ethernet@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@01c50000 <other warning> on device: <other_device> eth0: ethernet@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@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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@01c50000 <other etnernet devices> eth0: ethernet@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@01c50000 <other warning> on device: <other_device> eth0: ethernet@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@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 */
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
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)) {
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 onboardEEPROM and set into the environment if and only if the environmentdoes currently not already hold a MAC address. For more informationsee 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. Naturallythis will only work if there is an actual EEPROM connected on theI2C bus and the bus and device are properly configured via theoptions 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 oneregister.+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 MACaddress. 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 CRCbyte.
Would it be possible to have default n here? I would guess that more boards don't have this CRC8 sums.
Thanks, Michal
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 onboardEEPROM and set into the environment if and only if the environmentdoes currently not already hold a MAC address. For more informationsee 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. Naturallythis will only work if there is an actual EEPROM connected on theI2C bus and the bus and device are properly configured via theoptions 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 oneregister.+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 MACaddress. 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 CRCbyte.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
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 anonboard
EEPROM and set into the environment if and only if theenvironment
does currently not already hold a MAC address. For moreinformation
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 theI2C bus and the bus and device are properly configured via theoptions 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 oneregister.+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 MACaddress. To do so, the MAC address is stored with a CRC8 byteappend.
This option enables the CRC check of the MAC address againstthe 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
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 anonboard
EEPROM and set into the environment if and only if theenvironment
does currently not already hold a MAC address. For moreinformation
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 theI2C bus and the bus and device are properly configured via theoptions 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 oneregister.+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 MACaddress. To do so, the MAC address is stored with a CRC8 byteappend.
This option enables the CRC check of the MAC address againstthe 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
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 anonboard
EEPROM and set into the environment if and only if theenvironment
does currently not already hold a MAC address. For moreinformation
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 connectedon the
I2C bus and the bus and device are properly configured via theoptions 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 tobus 0.
Remember to also make the selected bus available viaI2Cn_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 oneregister.+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 theMAC
address. To do so, the MAC address is stored with a CRC8 byteappend.
This option enables the CRC check of the MAC address againstthe 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
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 anonboard
EEPROM and set into the environment if and only if theenvironment
does currently not already hold a MAC address. For moreinformation
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 onthe
I2C bus and the bus and device are properly configured via theoptions 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
Remember to also make the selected bus available viaI2Cn_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 oneregister.+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 MACaddress. To do so, the MAC address is stored with a CRC8 byteappend.
This option enables the CRC check of the MAC address againstthe 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()
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"helpSelecting this will try to get the Ethernet address from an onboardEEPROM and set into the environment if and only if the environmentdoes currently not already hold a MAC address. For more informationsee doc/README.enetaddr.+config NET_ETHADDR_EEPROM_I2C
depends on NET_ETHADDR_EEPROMbool "EEPROM on I2C bus"helpThis switch enables checks for an EEPROM on the I2C bus. Naturallythis will only work if there is an actual EEPROM connected on theI2C bus and the bus and device are properly configured via theoptions below.+config NET_ETHADDR_EEPROM_I2C_BUS
depends on NET_ETHADDR_EEPROM_I2Cint "I2C bus"default 0helpSelect 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_I2Chex "EEPROM address"default 0x50helpSelect the address of the EEPROM, defaults to address 0x50.+config NET_ETHADDR_EEPROM_I2C_ADDRLEN
depends on NET_ETHADDR_EEPROM_I2Cint "EEPROM address length"default 1helpNumber of bytes to be used for the I2C address length. Typically 1,2 for large memories, 0 for register type devices with only oneregister.+config NET_ETHADDR_EEPROM_OFFSET
depends on NET_ETHADDR_EEPROMint "EEPROM offset"default 8helpSelect the byte offset of the MAC address within the page,defaults to byte 8.+config NET_ETHADDR_EEPROM_CRC8
depends on NET_ETHADDR_EEPROMbool "Check CRC8 of MAC"default yhelpOptionally, it is possible to run a CRC-8-CCITT check on the MACaddress. 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 CRCbyte.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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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"helpSelecting this will try to get the Ethernet address from an onboardEEPROM and set into the environment if and only if the environmentdoes currently not already hold a MAC address. For more informationsee doc/README.enetaddr.+config NET_ETHADDR_EEPROM_I2C
depends on NET_ETHADDR_EEPROMbool "EEPROM on I2C bus"helpThis switch enables checks for an EEPROM on the I2C bus. Naturallythis will only work if there is an actual EEPROM connected on theI2C bus and the bus and device are properly configured via theoptions below.+config NET_ETHADDR_EEPROM_I2C_BUS
depends on NET_ETHADDR_EEPROM_I2Cint "I2C bus"default 0helpSelect 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_I2Chex "EEPROM address"default 0x50helpSelect the address of the EEPROM, defaults to address 0x50.+config NET_ETHADDR_EEPROM_I2C_ADDRLEN
depends on NET_ETHADDR_EEPROM_I2Cint "EEPROM address length"default 1helpNumber of bytes to be used for the I2C address length. Typically 1,2 for large memories, 0 for register type devices with only oneregister.+config NET_ETHADDR_EEPROM_OFFSET
depends on NET_ETHADDR_EEPROMint "EEPROM offset"default 8helpSelect the byte offset of the MAC address within the page,defaults to byte 8.+config NET_ETHADDR_EEPROM_CRC8
depends on NET_ETHADDR_EEPROMbool "Check CRC8 of MAC"default yhelpOptionally, it is possible to run a CRC-8-CCITT check on the MACaddress. 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 CRCbyte.- 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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 theeeprom */
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 ondevice: ");
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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);
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
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",
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
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);
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
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
Hey maime,
Sorry for constantly getting your e-mail address wrong! Sorry!
On 30-11-16 10:12, maxime.ripard@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
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@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).
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@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
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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
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
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
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
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
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
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
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
> 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
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
>> 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
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
>>> 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
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
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
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
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
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
Hi oliver@schinagl.nl,
https://patchwork.ozlabs.org/patch/699401/ was applied to u-boot-net.git.
Thanks! -Joe
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; +}
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
Hi oliver@schinagl.nl,
https://patchwork.ozlabs.org/patch/699397/ was applied to u-boot-net.git.
Thanks! -Joe
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?
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
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.
participants (10)
-
Hans de Goede -
Igor Grinberg -
Joe Hershberger -
Joe Hershberger -
Marcel Ziswiler -
maxime.ripardīŧ free-electrons.com -
Michal Simek -
Olliver Schinagl -
Olliver Schinagl -
Simon Glass