diff mbox

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

Message ID 20161125153032.14617-10-oliver@schinagl.nl
State Changes Requested
Delegated to: Joe Hershberger
Headers show

Commit Message

Olliver Schinagl Nov. 25, 2016, 3:30 p.m. UTC
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(-)

Comments

Michal Simek Nov. 28, 2016, 8:21 a.m. UTC | #1
On 25.11.2016 16:30, Olliver Schinagl wrote:
> This patch allows Kconfig to enable and set parameters to make it
> possible to read the MAC address from an EEPROM. The net core layer then
> uses this information to read MAC addresses from this EEPROM.
> 
> Besides the various tuneables as to how to access the eeprom (bus,
> address, addressing mode/length, 2 configurable that are EEPROM generic
> (e.g. SPI or some other form of access) which are:
> 
> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
> for other purposes (header MAGIC for example).
> 
> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
> checksum that should be verified.
> 
> Currently only I2C eeproms have been tested and thus only those options
> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
> just as created and added.
> 
> The code currently first checks if there is a non-zero MAC address in
> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
> used by a board to supply the MAC in other ways.
> 
> If both these fails, the other code is still in place to query the
> environent, which then can be used to override the hardware supplied
> data.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/net.h       | 14 ++++++++
>  net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>  net/eth-uclass.c    |  9 +++--
>  net/eth_common.c    | 34 ++++++++++++++++++
>  net/eth_legacy.c    |  2 ++
>  6 files changed, 214 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index 50e4899..89c1f7d 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address (summarized):
>  Previous behavior had the MAC address always being programmed into hardware
>  in the device's init() function.
>  
> +--------
> + EEPROM
> +--------
> +
> +Boards may come with an EEPROM specifically to store configuration bits, such
> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this feature.
> +Depending on the board, the EEPROM may be connected on various methods, but
> +currently, only the I2C bus can be used via CONFIG_NET_ETHADDR_EEPROM_I2C.
> +
> +The following config options are available,
> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom is present.
> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM, which
> +defaults to the very common 0x50. Small size EEPROM's generally use single byte
> +addressing but larger EEPROM's may use double byte addressing, which can be
> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
> +
> +Within the EEPROM, the MAC address can be stored on any arbitrary offset,
> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default however, allowing
> +the first 8 bytes to be used for an optional data, for example a configuration
> +struct where the mac address is part of.
> +
> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous ARP_HLEN
> +bytes. Whether to check this CRC8 or not is dependent on
> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
> +
> +To keep things nicely aligned, a final 'reserved' byte is added to the mac
> +address + crc8 combo.
> +
> +A board may want to store more information in its eeprom, using the following
> +example layout, this can be achieved.
> +
> +struct mac_addr {
> +	uint8_t mac[ARP_HLEN];
> +	uint8_t crc8;
> +	uint8_t reserved;
> +};
> +
> +struct config_eeprom {
> +	uint32_t magic;
> +	uint8_t version;
> +	uint8_t reserved[2];
> +	uint8_t mac_cnt;
> +	struct mac_addr[mac_cnt];
> +};
> +
> +Filling this in:
> +struct config_eeprom eeprom = {
> +	.magic = { 'M', 'g', 'i', 'c' },
> +	.reserved = { 0x00, 0x00 },
> +	.mac_cnt = 2,
> +	.mac_addr = {
> +		{
> +			.mac = {
> +				0x01, 0x23, 0x45,
> +				0x67, 0x89, 0xab,
> +			},
> +			.crc8 = 0xbe,
> +			.reserved = 0x00,
> +		}, {
> +			.mac = {
> +				0xba, 0x98, 0x76,
> +				0x54, 0x32, 0x10,
> +			},
> +			.crc8 = 0x82,
> +			.reserved = 0x00,
> +		},
> +	},
> +};
> +
> +The eeprom content would look like this.
> +
> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00 |Mgic.....#Eg....|
> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
> +
> +This can be done from linux using the i2c-tools:
> +
> +i2cset I2CBUS 0x50 0x08 0x01
> +i2cset I2CBUS 0x50 0x09 0x23
> +i2cset I2CBUS 0x50 0x0a 0x45
> +i2cset I2CBUS 0x50 0x0b 0x67
> +i2cset I2CBUS 0x50 0x0c 0x89
> +i2cset I2CBUS 0x50 0x0d 0xab
> +i2cset I2CBUS 0x50 0x0e 0xbe
> +
> +Alternativly this can be done from the u-boot console as:
> +
> +u-boot> mm.b 0
> +00000000: 00 ? 01
> +00000001: 23 ? 23
> +00000002: 45 ? 45
> +00000003: 67 ? 67
> +00000004: 89 ? 89
> +00000005: ab ? ab
> +00000006: be ? be
> +00000007: 00 ? q
> +i2c dev I2CBUS
> +i2c write 0 50 8 7
> +i2c md 50 8
> +
>  -------
>   Usage
>  -------
> diff --git a/include/net.h b/include/net.h
> index 08f8af8..e50ab5d 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>  int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>  
>  /**
> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
> + *
> + * This function tries to read the MAC address from an eeprom as can be read
> + * in docs/README.enetaddr.
> + *
> + * @index:	index of the interface to get the hwaddr for
> + * @enetaddr:	pointer for the found hwaddr. Needs to be atleast ARP_HLEN
> + * @return:	0 on success, non-zero is error status. Additionally hwaddr
> + *		is set to 00:00:00:00:00. This is also the case if
> + *		CONFIG_NET_ETHADDR_EEPROM is not set.
> + */
> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
> +
> +/**
>   * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>   *
>   * This sets up an environment variable with the given MAC address (@enetaddr).
> diff --git a/net/Kconfig b/net/Kconfig
> index 414c549..f699e1c 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -7,6 +7,65 @@ menuconfig NET
>  
>  if NET
>  
> +config NET_ETHADDR_EEPROM
> +	bool "Get ethaddr from eeprom"
> +	help
> +	  Selecting this will try to get the Ethernet address from an onboard
> +	  EEPROM and set into the environment if and only if the environment
> +	  does currently not already hold a MAC address. For more information
> +	  see doc/README.enetaddr.
> +
> +config NET_ETHADDR_EEPROM_I2C
> +	depends on NET_ETHADDR_EEPROM
> +	bool "EEPROM on I2C bus"
> +	help
> +	  This switch enables checks for an EEPROM on the I2C bus. Naturally
> +	  this will only work if there is an actual EEPROM connected on the
> +	  I2C bus and the bus and device are properly configured via the
> +	  options below.
> +
> +config NET_ETHADDR_EEPROM_I2C_BUS
> +	depends on NET_ETHADDR_EEPROM_I2C
> +	int "I2C bus"
> +	default 0
> +	help
> +	  Select the bus on which the EEPROM is present, defaults to bus 0.
> +	  Remember to also make the selected bus available via I2Cn_ENABLE.
> +
> +config NET_ETHADDR_EEPROM_I2C_ADDR
> +	depends on NET_ETHADDR_EEPROM_I2C
> +	hex "EEPROM address"
> +	default 0x50
> +	help
> +	  Select the address of the EEPROM, defaults to address 0x50.
> +
> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
> +	depends on NET_ETHADDR_EEPROM_I2C
> +	int "EEPROM address length"
> +	default 1
> +	help
> +	  Number of bytes to be used for the I2C address length. Typically 1,
> +	  2 for large memories, 0 for register type devices with only one
> +	  register.
> +
> +config NET_ETHADDR_EEPROM_OFFSET
> +	depends on NET_ETHADDR_EEPROM
> +	int "EEPROM offset"
> +	default 8
> +	help
> +	  Select the byte offset of the MAC address within the page,
> +	  defaults to byte 8.

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

> +
> +config NET_ETHADDR_EEPROM_CRC8
> +	depends on NET_ETHADDR_EEPROM
> +	bool "Check CRC8 of MAC"
> +	default y
> +	help
> +	  Optionally, it is possible to run a CRC-8-CCITT check on the MAC
> +	  address. To do so, the MAC address is stored with a CRC8 byte append.
> +	  This option enables the CRC check of the MAC address against the CRC
> +	  byte.
> +

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

Thanks,
Michal
Olliver Schinagl Nov. 29, 2016, 4:45 p.m. UTC | #2
Hey Michal,


On 28-11-16 09:21, Michal Simek wrote:
> On 25.11.2016 16:30, Olliver Schinagl wrote:
>> This patch allows Kconfig to enable and set parameters to make it
>> possible to read the MAC address from an EEPROM. The net core layer then
>> uses this information to read MAC addresses from this EEPROM.
>>
>> Besides the various tuneables as to how to access the eeprom (bus,
>> address, addressing mode/length, 2 configurable that are EEPROM generic
>> (e.g. SPI or some other form of access) which are:
>>
>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
>> for other purposes (header MAGIC for example).
>>
>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
>> checksum that should be verified.
>>
>> Currently only I2C eeproms have been tested and thus only those options
>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>> just as created and added.
>>
>> The code currently first checks if there is a non-zero MAC address in
>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>> used by a board to supply the MAC in other ways.
>>
>> If both these fails, the other code is still in place to query the
>> environent, which then can be used to override the hardware supplied
>> data.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/net.h       | 14 ++++++++
>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>   net/eth-uclass.c    |  9 +++--
>>   net/eth_common.c    | 34 ++++++++++++++++++
>>   net/eth_legacy.c    |  2 ++
>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>> index 50e4899..89c1f7d 100644
>> --- a/doc/README.enetaddr
>> +++ b/doc/README.enetaddr
>> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address (summarized):
>>   Previous behavior had the MAC address always being programmed into hardware
>>   in the device's init() function.
>>   
>> +--------
>> + EEPROM
>> +--------
>> +
>> +Boards may come with an EEPROM specifically to store configuration bits, such
>> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this feature.
>> +Depending on the board, the EEPROM may be connected on various methods, but
>> +currently, only the I2C bus can be used via CONFIG_NET_ETHADDR_EEPROM_I2C.
>> +
>> +The following config options are available,
>> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom is present.
>> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM, which
>> +defaults to the very common 0x50. Small size EEPROM's generally use single byte
>> +addressing but larger EEPROM's may use double byte addressing, which can be
>> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
>> +
>> +Within the EEPROM, the MAC address can be stored on any arbitrary offset,
>> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default however, allowing
>> +the first 8 bytes to be used for an optional data, for example a configuration
>> +struct where the mac address is part of.
>> +
>> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous ARP_HLEN
>> +bytes. Whether to check this CRC8 or not is dependent on
>> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
>> +
>> +To keep things nicely aligned, a final 'reserved' byte is added to the mac
>> +address + crc8 combo.
>> +
>> +A board may want to store more information in its eeprom, using the following
>> +example layout, this can be achieved.
>> +
>> +struct mac_addr {
>> +	uint8_t mac[ARP_HLEN];
>> +	uint8_t crc8;
>> +	uint8_t reserved;
>> +};
>> +
>> +struct config_eeprom {
>> +	uint32_t magic;
>> +	uint8_t version;
>> +	uint8_t reserved[2];
>> +	uint8_t mac_cnt;
>> +	struct mac_addr[mac_cnt];
>> +};
>> +
>> +Filling this in:
>> +struct config_eeprom eeprom = {
>> +	.magic = { 'M', 'g', 'i', 'c' },
>> +	.reserved = { 0x00, 0x00 },
>> +	.mac_cnt = 2,
>> +	.mac_addr = {
>> +		{
>> +			.mac = {
>> +				0x01, 0x23, 0x45,
>> +				0x67, 0x89, 0xab,
>> +			},
>> +			.crc8 = 0xbe,
>> +			.reserved = 0x00,
>> +		}, {
>> +			.mac = {
>> +				0xba, 0x98, 0x76,
>> +				0x54, 0x32, 0x10,
>> +			},
>> +			.crc8 = 0x82,
>> +			.reserved = 0x00,
>> +		},
>> +	},
>> +};
>> +
>> +The eeprom content would look like this.
>> +
>> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00 |Mgic.....#Eg....|
>> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
>> +
>> +This can be done from linux using the i2c-tools:
>> +
>> +i2cset I2CBUS 0x50 0x08 0x01
>> +i2cset I2CBUS 0x50 0x09 0x23
>> +i2cset I2CBUS 0x50 0x0a 0x45
>> +i2cset I2CBUS 0x50 0x0b 0x67
>> +i2cset I2CBUS 0x50 0x0c 0x89
>> +i2cset I2CBUS 0x50 0x0d 0xab
>> +i2cset I2CBUS 0x50 0x0e 0xbe
>> +
>> +Alternativly this can be done from the u-boot console as:
>> +
>> +u-boot> mm.b 0
>> +00000000: 00 ? 01
>> +00000001: 23 ? 23
>> +00000002: 45 ? 45
>> +00000003: 67 ? 67
>> +00000004: 89 ? 89
>> +00000005: ab ? ab
>> +00000006: be ? be
>> +00000007: 00 ? q
>> +i2c dev I2CBUS
>> +i2c write 0 50 8 7
>> +i2c md 50 8
>> +
>>   -------
>>    Usage
>>   -------
>> diff --git a/include/net.h b/include/net.h
>> index 08f8af8..e50ab5d 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>   
>>   /**
>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>> + *
>> + * This function tries to read the MAC address from an eeprom as can be read
>> + * in docs/README.enetaddr.
>> + *
>> + * @index:	index of the interface to get the hwaddr for
>> + * @enetaddr:	pointer for the found hwaddr. Needs to be atleast ARP_HLEN
>> + * @return:	0 on success, non-zero is error status. Additionally hwaddr
>> + *		is set to 00:00:00:00:00. This is also the case if
>> + *		CONFIG_NET_ETHADDR_EEPROM is not set.
>> + */
>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>> +
>> +/**
>>    * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>>    *
>>    * This sets up an environment variable with the given MAC address (@enetaddr).
>> diff --git a/net/Kconfig b/net/Kconfig
>> index 414c549..f699e1c 100644
>> --- a/net/Kconfig
>> +++ b/net/Kconfig
>> @@ -7,6 +7,65 @@ menuconfig NET
>>   
>>   if NET
>>   
>> +config NET_ETHADDR_EEPROM
>> +	bool "Get ethaddr from eeprom"
>> +	help
>> +	  Selecting this will try to get the Ethernet address from an onboard
>> +	  EEPROM and set into the environment if and only if the environment
>> +	  does currently not already hold a MAC address. For more information
>> +	  see doc/README.enetaddr.
>> +
>> +config NET_ETHADDR_EEPROM_I2C
>> +	depends on NET_ETHADDR_EEPROM
>> +	bool "EEPROM on I2C bus"
>> +	help
>> +	  This switch enables checks for an EEPROM on the I2C bus. Naturally
>> +	  this will only work if there is an actual EEPROM connected on the
>> +	  I2C bus and the bus and device are properly configured via the
>> +	  options below.
>> +
>> +config NET_ETHADDR_EEPROM_I2C_BUS
>> +	depends on NET_ETHADDR_EEPROM_I2C
>> +	int "I2C bus"
>> +	default 0
>> +	help
>> +	  Select the bus on which the EEPROM is present, defaults to bus 0.
>> +	  Remember to also make the selected bus available via I2Cn_ENABLE.
>> +
>> +config NET_ETHADDR_EEPROM_I2C_ADDR
>> +	depends on NET_ETHADDR_EEPROM_I2C
>> +	hex "EEPROM address"
>> +	default 0x50
>> +	help
>> +	  Select the address of the EEPROM, defaults to address 0x50.
>> +
>> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
>> +	depends on NET_ETHADDR_EEPROM_I2C
>> +	int "EEPROM address length"
>> +	default 1
>> +	help
>> +	  Number of bytes to be used for the I2C address length. Typically 1,
>> +	  2 for large memories, 0 for register type devices with only one
>> +	  register.
>> +
>> +config NET_ETHADDR_EEPROM_OFFSET
>> +	depends on NET_ETHADDR_EEPROM
>> +	int "EEPROM offset"
>> +	default 8
>> +	help
>> +	  Select the byte offset of the MAC address within the page,
>> +	  defaults to byte 8.
> I would prefer to all these values to be in hex because i2c commands are
> also taking values in hex.
You are completly right and this is indeed my mistake. I will fix it for v2.

Incidently I put them on 0x50 in my own config files for this exact 
reason. I probably did not realise I could make it default to hex :)
>
>> +
>> +config NET_ETHADDR_EEPROM_CRC8
>> +	depends on NET_ETHADDR_EEPROM
>> +	bool "Check CRC8 of MAC"
>> +	default y
>> +	help
>> +	  Optionally, it is possible to run a CRC-8-CCITT check on the MAC
>> +	  address. To do so, the MAC address is stored with a CRC8 byte append.
>> +	  This option enables the CRC check of the MAC address against the CRC
>> +	  byte.
>> +
> Would it be possible to have default n here?
> I would guess that more boards don't have this CRC8 sums.
I agree, but most boards will not use this by default yet. If you enable 
this feature for your board, I strongly strongly recommend enabeling 
this feature as well. Thus disable it by user request.

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

Olliver
>
> Thanks,
> Michal
>
>
>
Michal Simek Nov. 29, 2016, 6:54 p.m. UTC | #3
On 29.11.2016 17:45, Olliver Schinagl wrote:
> Hey Michal,
> 
> 
> On 28-11-16 09:21, Michal Simek wrote:
>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>> This patch allows Kconfig to enable and set parameters to make it
>>> possible to read the MAC address from an EEPROM. The net core layer then
>>> uses this information to read MAC addresses from this EEPROM.
>>>
>>> Besides the various tuneables as to how to access the eeprom (bus,
>>> address, addressing mode/length, 2 configurable that are EEPROM generic
>>> (e.g. SPI or some other form of access) which are:
>>>
>>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>>> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
>>> for other purposes (header MAGIC for example).
>>>
>>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
>>> checksum that should be verified.
>>>
>>> Currently only I2C eeproms have been tested and thus only those options
>>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>>> just as created and added.
>>>
>>> The code currently first checks if there is a non-zero MAC address in
>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>> used by a board to supply the MAC in other ways.
>>>
>>> If both these fails, the other code is still in place to query the
>>> environent, which then can be used to override the hardware supplied
>>> data.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   doc/README.enetaddr | 99
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/net.h       | 14 ++++++++
>>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>   net/eth-uclass.c    |  9 +++--
>>>   net/eth_common.c    | 34 ++++++++++++++++++
>>>   net/eth_legacy.c    |  2 ++
>>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>> index 50e4899..89c1f7d 100644
>>> --- a/doc/README.enetaddr
>>> +++ b/doc/README.enetaddr
>>> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address
>>> (summarized):
>>>   Previous behavior had the MAC address always being programmed into
>>> hardware
>>>   in the device's init() function.
>>>   +--------
>>> + EEPROM
>>> +--------
>>> +
>>> +Boards may come with an EEPROM specifically to store configuration
>>> bits, such
>>> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this feature.
>>> +Depending on the board, the EEPROM may be connected on various
>>> methods, but
>>> +currently, only the I2C bus can be used via
>>> CONFIG_NET_ETHADDR_EEPROM_I2C.
>>> +
>>> +The following config options are available,
>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom
>>> is present.
>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM,
>>> which
>>> +defaults to the very common 0x50. Small size EEPROM's generally use
>>> single byte
>>> +addressing but larger EEPROM's may use double byte addressing, which
>>> can be
>>> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
>>> +
>>> +Within the EEPROM, the MAC address can be stored on any arbitrary
>>> offset,
>>> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default
>>> however, allowing
>>> +the first 8 bytes to be used for an optional data, for example a
>>> configuration
>>> +struct where the mac address is part of.
>>> +
>>> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous
>>> ARP_HLEN
>>> +bytes. Whether to check this CRC8 or not is dependent on
>>> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
>>> +
>>> +To keep things nicely aligned, a final 'reserved' byte is added to
>>> the mac
>>> +address + crc8 combo.
>>> +
>>> +A board may want to store more information in its eeprom, using the
>>> following
>>> +example layout, this can be achieved.
>>> +
>>> +struct mac_addr {
>>> +    uint8_t mac[ARP_HLEN];
>>> +    uint8_t crc8;
>>> +    uint8_t reserved;
>>> +};
>>> +
>>> +struct config_eeprom {
>>> +    uint32_t magic;
>>> +    uint8_t version;
>>> +    uint8_t reserved[2];
>>> +    uint8_t mac_cnt;
>>> +    struct mac_addr[mac_cnt];
>>> +};
>>> +
>>> +Filling this in:
>>> +struct config_eeprom eeprom = {
>>> +    .magic = { 'M', 'g', 'i', 'c' },
>>> +    .reserved = { 0x00, 0x00 },
>>> +    .mac_cnt = 2,
>>> +    .mac_addr = {
>>> +        {
>>> +            .mac = {
>>> +                0x01, 0x23, 0x45,
>>> +                0x67, 0x89, 0xab,
>>> +            },
>>> +            .crc8 = 0xbe,
>>> +            .reserved = 0x00,
>>> +        }, {
>>> +            .mac = {
>>> +                0xba, 0x98, 0x76,
>>> +                0x54, 0x32, 0x10,
>>> +            },
>>> +            .crc8 = 0x82,
>>> +            .reserved = 0x00,
>>> +        },
>>> +    },
>>> +};
>>> +
>>> +The eeprom content would look like this.
>>> +
>>> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00
>>> |Mgic.....#Eg....|
>>> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
>>> +
>>> +This can be done from linux using the i2c-tools:
>>> +
>>> +i2cset I2CBUS 0x50 0x08 0x01
>>> +i2cset I2CBUS 0x50 0x09 0x23
>>> +i2cset I2CBUS 0x50 0x0a 0x45
>>> +i2cset I2CBUS 0x50 0x0b 0x67
>>> +i2cset I2CBUS 0x50 0x0c 0x89
>>> +i2cset I2CBUS 0x50 0x0d 0xab
>>> +i2cset I2CBUS 0x50 0x0e 0xbe
>>> +
>>> +Alternativly this can be done from the u-boot console as:
>>> +
>>> +u-boot> mm.b 0
>>> +00000000: 00 ? 01
>>> +00000001: 23 ? 23
>>> +00000002: 45 ? 45
>>> +00000003: 67 ? 67
>>> +00000004: 89 ? 89
>>> +00000005: ab ? ab
>>> +00000006: be ? be
>>> +00000007: 00 ? q
>>> +i2c dev I2CBUS
>>> +i2c write 0 50 8 7
>>> +i2c md 50 8
>>> +
>>>   -------
>>>    Usage
>>>   -------
>>> diff --git a/include/net.h b/include/net.h
>>> index 08f8af8..e50ab5d 100644
>>> --- a/include/net.h
>>> +++ b/include/net.h
>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>> *enetaddr);
>>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>     /**
>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>> + *
>>> + * This function tries to read the MAC address from an eeprom as can
>>> be read
>>> + * in docs/README.enetaddr.
>>> + *
>>> + * @index:    index of the interface to get the hwaddr for
>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>> ARP_HLEN
>>> + * @return:    0 on success, non-zero is error status. Additionally
>>> hwaddr
>>> + *        is set to 00:00:00:00:00. This is also the case if
>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>> + */
>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>> +
>>> +/**
>>>    * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>> variable
>>>    *
>>>    * This sets up an environment variable with the given MAC address
>>> (@enetaddr).
>>> diff --git a/net/Kconfig b/net/Kconfig
>>> index 414c549..f699e1c 100644
>>> --- a/net/Kconfig
>>> +++ b/net/Kconfig
>>> @@ -7,6 +7,65 @@ menuconfig NET
>>>     if NET
>>>   +config NET_ETHADDR_EEPROM
>>> +    bool "Get ethaddr from eeprom"
>>> +    help
>>> +      Selecting this will try to get the Ethernet address from an
>>> onboard
>>> +      EEPROM and set into the environment if and only if the
>>> environment
>>> +      does currently not already hold a MAC address. For more
>>> information
>>> +      see doc/README.enetaddr.
>>> +
>>> +config NET_ETHADDR_EEPROM_I2C
>>> +    depends on NET_ETHADDR_EEPROM
>>> +    bool "EEPROM on I2C bus"
>>> +    help
>>> +      This switch enables checks for an EEPROM on the I2C bus.
>>> Naturally
>>> +      this will only work if there is an actual EEPROM connected on the
>>> +      I2C bus and the bus and device are properly configured via the
>>> +      options below.
>>> +
>>> +config NET_ETHADDR_EEPROM_I2C_BUS
>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>> +    int "I2C bus"
>>> +    default 0
>>> +    help
>>> +      Select the bus on which the EEPROM is present, defaults to bus 0.
>>> +      Remember to also make the selected bus available via I2Cn_ENABLE.
>>> +
>>> +config NET_ETHADDR_EEPROM_I2C_ADDR
>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>> +    hex "EEPROM address"
>>> +    default 0x50
>>> +    help
>>> +      Select the address of the EEPROM, defaults to address 0x50.
>>> +
>>> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>> +    int "EEPROM address length"
>>> +    default 1
>>> +    help
>>> +      Number of bytes to be used for the I2C address length.
>>> Typically 1,
>>> +      2 for large memories, 0 for register type devices with only one
>>> +      register.
>>> +
>>> +config NET_ETHADDR_EEPROM_OFFSET
>>> +    depends on NET_ETHADDR_EEPROM
>>> +    int "EEPROM offset"
>>> +    default 8
>>> +    help
>>> +      Select the byte offset of the MAC address within the page,
>>> +      defaults to byte 8.
>> I would prefer to all these values to be in hex because i2c commands are
>> also taking values in hex.
> You are completly right and this is indeed my mistake. I will fix it for
> v2.
> 
> Incidently I put them on 0x50 in my own config files for this exact
> reason. I probably did not realise I could make it default to hex :)

ok.

>>
>>> +
>>> +config NET_ETHADDR_EEPROM_CRC8
>>> +    depends on NET_ETHADDR_EEPROM
>>> +    bool "Check CRC8 of MAC"
>>> +    default y
>>> +    help
>>> +      Optionally, it is possible to run a CRC-8-CCITT check on the MAC
>>> +      address. To do so, the MAC address is stored with a CRC8 byte
>>> append.
>>> +      This option enables the CRC check of the MAC address against
>>> the CRC
>>> +      byte.
>>> +
>> Would it be possible to have default n here?
>> I would guess that more boards don't have this CRC8 sums.
> I agree, but most boards will not use this by default yet. If you enable
> this feature for your board, I strongly strongly recommend enabeling
> this feature as well. Thus disable it by user request.

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


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

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

Thanks,
Michal
Olliver Schinagl Nov. 30, 2016, 8:10 a.m. UTC | #4
Hey Michal,


On 29-11-16 19:54, Michal Simek wrote:
> On 29.11.2016 17:45, Olliver Schinagl wrote:
>> Hey Michal,
>>
>>
>> On 28-11-16 09:21, Michal Simek wrote:
>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>> This patch allows Kconfig to enable and set parameters to make it
>>>> possible to read the MAC address from an EEPROM. The net core layer then
>>>> uses this information to read MAC addresses from this EEPROM.
>>>>
>>>> Besides the various tuneables as to how to access the eeprom (bus,
>>>> address, addressing mode/length, 2 configurable that are EEPROM generic
>>>> (e.g. SPI or some other form of access) which are:
>>>>
>>>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>>>> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
>>>> for other purposes (header MAGIC for example).
>>>>
>>>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
>>>> checksum that should be verified.
>>>>
>>>> Currently only I2C eeproms have been tested and thus only those options
>>>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>>>> just as created and added.
>>>>
>>>> The code currently first checks if there is a non-zero MAC address in
>>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>>> used by a board to supply the MAC in other ways.
>>>>
>>>> If both these fails, the other code is still in place to query the
>>>> environent, which then can be used to override the hardware supplied
>>>> data.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>> ---
>>>>    doc/README.enetaddr | 99
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/net.h       | 14 ++++++++
>>>>    net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>>    net/eth-uclass.c    |  9 +++--
>>>>    net/eth_common.c    | 34 ++++++++++++++++++
>>>>    net/eth_legacy.c    |  2 ++
>>>>    6 files changed, 214 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>>> index 50e4899..89c1f7d 100644
>>>> --- a/doc/README.enetaddr
>>>> +++ b/doc/README.enetaddr
>>>> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address
>>>> (summarized):
>>>>    Previous behavior had the MAC address always being programmed into
>>>> hardware
>>>>    in the device's init() function.
>>>>    +--------
>>>> + EEPROM
>>>> +--------
>>>> +
>>>> +Boards may come with an EEPROM specifically to store configuration
>>>> bits, such
>>>> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this feature.
>>>> +Depending on the board, the EEPROM may be connected on various
>>>> methods, but
>>>> +currently, only the I2C bus can be used via
>>>> CONFIG_NET_ETHADDR_EEPROM_I2C.
>>>> +
>>>> +The following config options are available,
>>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom
>>>> is present.
>>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM,
>>>> which
>>>> +defaults to the very common 0x50. Small size EEPROM's generally use
>>>> single byte
>>>> +addressing but larger EEPROM's may use double byte addressing, which
>>>> can be
>>>> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
>>>> +
>>>> +Within the EEPROM, the MAC address can be stored on any arbitrary
>>>> offset,
>>>> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default
>>>> however, allowing
>>>> +the first 8 bytes to be used for an optional data, for example a
>>>> configuration
>>>> +struct where the mac address is part of.
>>>> +
>>>> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous
>>>> ARP_HLEN
>>>> +bytes. Whether to check this CRC8 or not is dependent on
>>>> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
>>>> +
>>>> +To keep things nicely aligned, a final 'reserved' byte is added to
>>>> the mac
>>>> +address + crc8 combo.
>>>> +
>>>> +A board may want to store more information in its eeprom, using the
>>>> following
>>>> +example layout, this can be achieved.
>>>> +
>>>> +struct mac_addr {
>>>> +    uint8_t mac[ARP_HLEN];
>>>> +    uint8_t crc8;
>>>> +    uint8_t reserved;
>>>> +};
>>>> +
>>>> +struct config_eeprom {
>>>> +    uint32_t magic;
>>>> +    uint8_t version;
>>>> +    uint8_t reserved[2];
>>>> +    uint8_t mac_cnt;
>>>> +    struct mac_addr[mac_cnt];
>>>> +};
>>>> +
>>>> +Filling this in:
>>>> +struct config_eeprom eeprom = {
>>>> +    .magic = { 'M', 'g', 'i', 'c' },
>>>> +    .reserved = { 0x00, 0x00 },
>>>> +    .mac_cnt = 2,
>>>> +    .mac_addr = {
>>>> +        {
>>>> +            .mac = {
>>>> +                0x01, 0x23, 0x45,
>>>> +                0x67, 0x89, 0xab,
>>>> +            },
>>>> +            .crc8 = 0xbe,
>>>> +            .reserved = 0x00,
>>>> +        }, {
>>>> +            .mac = {
>>>> +                0xba, 0x98, 0x76,
>>>> +                0x54, 0x32, 0x10,
>>>> +            },
>>>> +            .crc8 = 0x82,
>>>> +            .reserved = 0x00,
>>>> +        },
>>>> +    },
>>>> +};
>>>> +
>>>> +The eeprom content would look like this.
>>>> +
>>>> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00
>>>> |Mgic.....#Eg....|
>>>> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
>>>> +
>>>> +This can be done from linux using the i2c-tools:
>>>> +
>>>> +i2cset I2CBUS 0x50 0x08 0x01
>>>> +i2cset I2CBUS 0x50 0x09 0x23
>>>> +i2cset I2CBUS 0x50 0x0a 0x45
>>>> +i2cset I2CBUS 0x50 0x0b 0x67
>>>> +i2cset I2CBUS 0x50 0x0c 0x89
>>>> +i2cset I2CBUS 0x50 0x0d 0xab
>>>> +i2cset I2CBUS 0x50 0x0e 0xbe
>>>> +
>>>> +Alternativly this can be done from the u-boot console as:
>>>> +
>>>> +u-boot> mm.b 0
>>>> +00000000: 00 ? 01
>>>> +00000001: 23 ? 23
>>>> +00000002: 45 ? 45
>>>> +00000003: 67 ? 67
>>>> +00000004: 89 ? 89
>>>> +00000005: ab ? ab
>>>> +00000006: be ? be
>>>> +00000007: 00 ? q
>>>> +i2c dev I2CBUS
>>>> +i2c write 0 50 8 7
>>>> +i2c md 50 8
>>>> +
>>>>    -------
>>>>     Usage
>>>>    -------
>>>> diff --git a/include/net.h b/include/net.h
>>>> index 08f8af8..e50ab5d 100644
>>>> --- a/include/net.h
>>>> +++ b/include/net.h
>>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>>> *enetaddr);
>>>>    int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>>      /**
>>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>>> + *
>>>> + * This function tries to read the MAC address from an eeprom as can
>>>> be read
>>>> + * in docs/README.enetaddr.
>>>> + *
>>>> + * @index:    index of the interface to get the hwaddr for
>>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>>> ARP_HLEN
>>>> + * @return:    0 on success, non-zero is error status. Additionally
>>>> hwaddr
>>>> + *        is set to 00:00:00:00:00. This is also the case if
>>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>>> + */
>>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>>> +
>>>> +/**
>>>>     * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>>> variable
>>>>     *
>>>>     * This sets up an environment variable with the given MAC address
>>>> (@enetaddr).
>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>> index 414c549..f699e1c 100644
>>>> --- a/net/Kconfig
>>>> +++ b/net/Kconfig
>>>> @@ -7,6 +7,65 @@ menuconfig NET
>>>>      if NET
>>>>    +config NET_ETHADDR_EEPROM
>>>> +    bool "Get ethaddr from eeprom"
>>>> +    help
>>>> +      Selecting this will try to get the Ethernet address from an
>>>> onboard
>>>> +      EEPROM and set into the environment if and only if the
>>>> environment
>>>> +      does currently not already hold a MAC address. For more
>>>> information
>>>> +      see doc/README.enetaddr.
>>>> +
>>>> +config NET_ETHADDR_EEPROM_I2C
>>>> +    depends on NET_ETHADDR_EEPROM
>>>> +    bool "EEPROM on I2C bus"
>>>> +    help
>>>> +      This switch enables checks for an EEPROM on the I2C bus.
>>>> Naturally
>>>> +      this will only work if there is an actual EEPROM connected on the
>>>> +      I2C bus and the bus and device are properly configured via the
>>>> +      options below.
>>>> +
>>>> +config NET_ETHADDR_EEPROM_I2C_BUS
>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>> +    int "I2C bus"
>>>> +    default 0
>>>> +    help
>>>> +      Select the bus on which the EEPROM is present, defaults to bus 0.
>>>> +      Remember to also make the selected bus available via I2Cn_ENABLE.
>>>> +
>>>> +config NET_ETHADDR_EEPROM_I2C_ADDR
>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>> +    hex "EEPROM address"
>>>> +    default 0x50
>>>> +    help
>>>> +      Select the address of the EEPROM, defaults to address 0x50.
>>>> +
>>>> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>> +    int "EEPROM address length"
>>>> +    default 1
>>>> +    help
>>>> +      Number of bytes to be used for the I2C address length.
>>>> Typically 1,
>>>> +      2 for large memories, 0 for register type devices with only one
>>>> +      register.
>>>> +
>>>> +config NET_ETHADDR_EEPROM_OFFSET
>>>> +    depends on NET_ETHADDR_EEPROM
>>>> +    int "EEPROM offset"
>>>> +    default 8
>>>> +    help
>>>> +      Select the byte offset of the MAC address within the page,
>>>> +      defaults to byte 8.
>>> I would prefer to all these values to be in hex because i2c commands are
>>> also taking values in hex.
>> You are completly right and this is indeed my mistake. I will fix it for
>> v2.
>>
>> Incidently I put them on 0x50 in my own config files for this exact
>> reason. I probably did not realise I could make it default to hex :)
> ok.
>
>>>> +
>>>> +config NET_ETHADDR_EEPROM_CRC8
>>>> +    depends on NET_ETHADDR_EEPROM
>>>> +    bool "Check CRC8 of MAC"
>>>> +    default y
>>>> +    help
>>>> +      Optionally, it is possible to run a CRC-8-CCITT check on the MAC
>>>> +      address. To do so, the MAC address is stored with a CRC8 byte
>>>> append.
>>>> +      This option enables the CRC check of the MAC address against
>>>> the CRC
>>>> +      byte.
>>>> +
>>> Would it be possible to have default n here?
>>> I would guess that more boards don't have this CRC8 sums.
>> I agree, but most boards will not use this by default yet. If you enable
>> this feature for your board, I strongly strongly recommend enabeling
>> this feature as well. Thus disable it by user request.
> Hard to do it for boards in the field. As you see we have this feature
> enabled for zcu102 and also zybo board and there is no crc8 sums factory
> programmed.
And that's why it is an option. If you turn on this option in your 
(board)config, you have the option as a board maintainer to set the crc8 
to off, because you have boards in the field already. If I add a new 
board now however, and want to use this feature, it should not be to 
easy to disable it.
>
>> Reason why I strongly recommend to enable it: If i have an unprogrammed
>> eeprom, it comes filled with 0xffffffff. Which is interpreted as a
>> correct mac address. What if i have random garbage in the eeprom (or a
>> user change one bit by accident). I still have a valid mac address.
>> Using the crc8 to validate the mac address makes this a lot more safe.
> Isn't 0xffffffff invalid address? I think I saw it as invalid.
I know 0x000000 is 'invalid' 0xffffffff might be indeed. but random bits 
are valid mac addresses.
>
> Thanks,
> Michal
>
Michal Simek Nov. 30, 2016, 8:31 a.m. UTC | #5
On 30.11.2016 09:10, Olliver Schinagl wrote:
> Hey Michal,
> 
> 
> On 29-11-16 19:54, Michal Simek wrote:
>> On 29.11.2016 17:45, Olliver Schinagl wrote:
>>> Hey Michal,
>>>
>>>
>>> On 28-11-16 09:21, Michal Simek wrote:
>>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>>> This patch allows Kconfig to enable and set parameters to make it
>>>>> possible to read the MAC address from an EEPROM. The net core layer
>>>>> then
>>>>> uses this information to read MAC addresses from this EEPROM.
>>>>>
>>>>> Besides the various tuneables as to how to access the eeprom (bus,
>>>>> address, addressing mode/length, 2 configurable that are EEPROM
>>>>> generic
>>>>> (e.g. SPI or some other form of access) which are:
>>>>>
>>>>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>>>>> the MAC address is. The default is 8 allowing for 8 bytes before
>>>>> the MAC
>>>>> for other purposes (header MAGIC for example).
>>>>>
>>>>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a
>>>>> CRC8-CCIT
>>>>> checksum that should be verified.
>>>>>
>>>>> Currently only I2C eeproms have been tested and thus only those
>>>>> options
>>>>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>>>>> just as created and added.
>>>>>
>>>>> The code currently first checks if there is a non-zero MAC address in
>>>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>>>> used by a board to supply the MAC in other ways.
>>>>>
>>>>> If both these fails, the other code is still in place to query the
>>>>> environent, which then can be used to override the hardware supplied
>>>>> data.
>>>>>
>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>> ---
>>>>>    doc/README.enetaddr | 99
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/net.h       | 14 ++++++++
>>>>>    net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>>>    net/eth-uclass.c    |  9 +++--
>>>>>    net/eth_common.c    | 34 ++++++++++++++++++
>>>>>    net/eth_legacy.c    |  2 ++
>>>>>    6 files changed, 214 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>>>> index 50e4899..89c1f7d 100644
>>>>> --- a/doc/README.enetaddr
>>>>> +++ b/doc/README.enetaddr
>>>>> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address
>>>>> (summarized):
>>>>>    Previous behavior had the MAC address always being programmed into
>>>>> hardware
>>>>>    in the device's init() function.
>>>>>    +--------
>>>>> + EEPROM
>>>>> +--------
>>>>> +
>>>>> +Boards may come with an EEPROM specifically to store configuration
>>>>> bits, such
>>>>> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this
>>>>> feature.
>>>>> +Depending on the board, the EEPROM may be connected on various
>>>>> methods, but
>>>>> +currently, only the I2C bus can be used via
>>>>> CONFIG_NET_ETHADDR_EEPROM_I2C.
>>>>> +
>>>>> +The following config options are available,
>>>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom
>>>>> is present.
>>>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM,
>>>>> which
>>>>> +defaults to the very common 0x50. Small size EEPROM's generally use
>>>>> single byte
>>>>> +addressing but larger EEPROM's may use double byte addressing, which
>>>>> can be
>>>>> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
>>>>> +
>>>>> +Within the EEPROM, the MAC address can be stored on any arbitrary
>>>>> offset,
>>>>> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default
>>>>> however, allowing
>>>>> +the first 8 bytes to be used for an optional data, for example a
>>>>> configuration
>>>>> +struct where the mac address is part of.
>>>>> +
>>>>> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous
>>>>> ARP_HLEN
>>>>> +bytes. Whether to check this CRC8 or not is dependent on
>>>>> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
>>>>> +
>>>>> +To keep things nicely aligned, a final 'reserved' byte is added to
>>>>> the mac
>>>>> +address + crc8 combo.
>>>>> +
>>>>> +A board may want to store more information in its eeprom, using the
>>>>> following
>>>>> +example layout, this can be achieved.
>>>>> +
>>>>> +struct mac_addr {
>>>>> +    uint8_t mac[ARP_HLEN];
>>>>> +    uint8_t crc8;
>>>>> +    uint8_t reserved;
>>>>> +};
>>>>> +
>>>>> +struct config_eeprom {
>>>>> +    uint32_t magic;
>>>>> +    uint8_t version;
>>>>> +    uint8_t reserved[2];
>>>>> +    uint8_t mac_cnt;
>>>>> +    struct mac_addr[mac_cnt];
>>>>> +};
>>>>> +
>>>>> +Filling this in:
>>>>> +struct config_eeprom eeprom = {
>>>>> +    .magic = { 'M', 'g', 'i', 'c' },
>>>>> +    .reserved = { 0x00, 0x00 },
>>>>> +    .mac_cnt = 2,
>>>>> +    .mac_addr = {
>>>>> +        {
>>>>> +            .mac = {
>>>>> +                0x01, 0x23, 0x45,
>>>>> +                0x67, 0x89, 0xab,
>>>>> +            },
>>>>> +            .crc8 = 0xbe,
>>>>> +            .reserved = 0x00,
>>>>> +        }, {
>>>>> +            .mac = {
>>>>> +                0xba, 0x98, 0x76,
>>>>> +                0x54, 0x32, 0x10,
>>>>> +            },
>>>>> +            .crc8 = 0x82,
>>>>> +            .reserved = 0x00,
>>>>> +        },
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +The eeprom content would look like this.
>>>>> +
>>>>> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00
>>>>> |Mgic.....#Eg....|
>>>>> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
>>>>> +
>>>>> +This can be done from linux using the i2c-tools:
>>>>> +
>>>>> +i2cset I2CBUS 0x50 0x08 0x01
>>>>> +i2cset I2CBUS 0x50 0x09 0x23
>>>>> +i2cset I2CBUS 0x50 0x0a 0x45
>>>>> +i2cset I2CBUS 0x50 0x0b 0x67
>>>>> +i2cset I2CBUS 0x50 0x0c 0x89
>>>>> +i2cset I2CBUS 0x50 0x0d 0xab
>>>>> +i2cset I2CBUS 0x50 0x0e 0xbe
>>>>> +
>>>>> +Alternativly this can be done from the u-boot console as:
>>>>> +
>>>>> +u-boot> mm.b 0
>>>>> +00000000: 00 ? 01
>>>>> +00000001: 23 ? 23
>>>>> +00000002: 45 ? 45
>>>>> +00000003: 67 ? 67
>>>>> +00000004: 89 ? 89
>>>>> +00000005: ab ? ab
>>>>> +00000006: be ? be
>>>>> +00000007: 00 ? q
>>>>> +i2c dev I2CBUS
>>>>> +i2c write 0 50 8 7
>>>>> +i2c md 50 8
>>>>> +
>>>>>    -------
>>>>>     Usage
>>>>>    -------
>>>>> diff --git a/include/net.h b/include/net.h
>>>>> index 08f8af8..e50ab5d 100644
>>>>> --- a/include/net.h
>>>>> +++ b/include/net.h
>>>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>>>> *enetaddr);
>>>>>    int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>>>      /**
>>>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>>>> + *
>>>>> + * This function tries to read the MAC address from an eeprom as can
>>>>> be read
>>>>> + * in docs/README.enetaddr.
>>>>> + *
>>>>> + * @index:    index of the interface to get the hwaddr for
>>>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>>>> ARP_HLEN
>>>>> + * @return:    0 on success, non-zero is error status. Additionally
>>>>> hwaddr
>>>>> + *        is set to 00:00:00:00:00. This is also the case if
>>>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>>>> + */
>>>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>>>> +
>>>>> +/**
>>>>>     * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>>>> variable
>>>>>     *
>>>>>     * This sets up an environment variable with the given MAC address
>>>>> (@enetaddr).
>>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>>> index 414c549..f699e1c 100644
>>>>> --- a/net/Kconfig
>>>>> +++ b/net/Kconfig
>>>>> @@ -7,6 +7,65 @@ menuconfig NET
>>>>>      if NET
>>>>>    +config NET_ETHADDR_EEPROM
>>>>> +    bool "Get ethaddr from eeprom"
>>>>> +    help
>>>>> +      Selecting this will try to get the Ethernet address from an
>>>>> onboard
>>>>> +      EEPROM and set into the environment if and only if the
>>>>> environment
>>>>> +      does currently not already hold a MAC address. For more
>>>>> information
>>>>> +      see doc/README.enetaddr.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C
>>>>> +    depends on NET_ETHADDR_EEPROM
>>>>> +    bool "EEPROM on I2C bus"
>>>>> +    help
>>>>> +      This switch enables checks for an EEPROM on the I2C bus.
>>>>> Naturally
>>>>> +      this will only work if there is an actual EEPROM connected
>>>>> on the
>>>>> +      I2C bus and the bus and device are properly configured via the
>>>>> +      options below.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C_BUS
>>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>>> +    int "I2C bus"
>>>>> +    default 0
>>>>> +    help
>>>>> +      Select the bus on which the EEPROM is present, defaults to
>>>>> bus 0.
>>>>> +      Remember to also make the selected bus available via
>>>>> I2Cn_ENABLE.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C_ADDR
>>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>>> +    hex "EEPROM address"
>>>>> +    default 0x50
>>>>> +    help
>>>>> +      Select the address of the EEPROM, defaults to address 0x50.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
>>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>>> +    int "EEPROM address length"
>>>>> +    default 1
>>>>> +    help
>>>>> +      Number of bytes to be used for the I2C address length.
>>>>> Typically 1,
>>>>> +      2 for large memories, 0 for register type devices with only one
>>>>> +      register.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_OFFSET
>>>>> +    depends on NET_ETHADDR_EEPROM
>>>>> +    int "EEPROM offset"
>>>>> +    default 8
>>>>> +    help
>>>>> +      Select the byte offset of the MAC address within the page,
>>>>> +      defaults to byte 8.
>>>> I would prefer to all these values to be in hex because i2c commands
>>>> are
>>>> also taking values in hex.
>>> You are completly right and this is indeed my mistake. I will fix it for
>>> v2.
>>>
>>> Incidently I put them on 0x50 in my own config files for this exact
>>> reason. I probably did not realise I could make it default to hex :)
>> ok.
>>
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_CRC8
>>>>> +    depends on NET_ETHADDR_EEPROM
>>>>> +    bool "Check CRC8 of MAC"
>>>>> +    default y
>>>>> +    help
>>>>> +      Optionally, it is possible to run a CRC-8-CCITT check on the
>>>>> MAC
>>>>> +      address. To do so, the MAC address is stored with a CRC8 byte
>>>>> append.
>>>>> +      This option enables the CRC check of the MAC address against
>>>>> the CRC
>>>>> +      byte.
>>>>> +
>>>> Would it be possible to have default n here?
>>>> I would guess that more boards don't have this CRC8 sums.
>>> I agree, but most boards will not use this by default yet. If you enable
>>> this feature for your board, I strongly strongly recommend enabeling
>>> this feature as well. Thus disable it by user request.
>> Hard to do it for boards in the field. As you see we have this feature
>> enabled for zcu102 and also zybo board and there is no crc8 sums factory
>> programmed.
> And that's why it is an option. If you turn on this option in your
> (board)config, you have the option as a board maintainer to set the crc8
> to off, because you have boards in the field already. If I add a new
> board now however, and want to use this feature, it should not be to
> easy to disable it.

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

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

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

Thanks,
Michal
Joe Hershberger Nov. 30, 2016, 7:52 p.m. UTC | #6
On Wed, Nov 30, 2016 at 2:10 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Michal,
>
>
>
> On 29-11-16 19:54, Michal Simek wrote:
>>
>> On 29.11.2016 17:45, Olliver Schinagl wrote:
>>>
>>> Hey Michal,
>>>
>>>
>>> On 28-11-16 09:21, Michal Simek wrote:
>>>>
>>>> On 25.11.2016 16:30, Olliver Schinagl wrote:
>>>>>
>>>>> This patch allows Kconfig to enable and set parameters to make it
>>>>> possible to read the MAC address from an EEPROM. The net core layer
>>>>> then
>>>>> uses this information to read MAC addresses from this EEPROM.
>>>>>
>>>>> Besides the various tuneables as to how to access the eeprom (bus,
>>>>> address, addressing mode/length, 2 configurable that are EEPROM generic
>>>>> (e.g. SPI or some other form of access) which are:
>>>>>
>>>>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>>>>> the MAC address is. The default is 8 allowing for 8 bytes before the
>>>>> MAC
>>>>> for other purposes (header MAGIC for example).
>>>>>
>>>>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a
>>>>> CRC8-CCIT
>>>>> checksum that should be verified.
>>>>>
>>>>> Currently only I2C eeproms have been tested and thus only those options
>>>>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>>>>> just as created and added.
>>>>>
>>>>> The code currently first checks if there is a non-zero MAC address in
>>>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>>>> used by a board to supply the MAC in other ways.
>>>>>
>>>>> If both these fails, the other code is still in place to query the
>>>>> environent, which then can be used to override the hardware supplied
>>>>> data.
>>>>>
>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>>> ---
>>>>>    doc/README.enetaddr | 99
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/net.h       | 14 ++++++++
>>>>>    net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>>>    net/eth-uclass.c    |  9 +++--
>>>>>    net/eth_common.c    | 34 ++++++++++++++++++
>>>>>    net/eth_legacy.c    |  2 ++
>>>>>    6 files changed, 214 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>>>>> index 50e4899..89c1f7d 100644
>>>>> --- a/doc/README.enetaddr
>>>>> +++ b/doc/README.enetaddr
>>>>> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address
>>>>> (summarized):
>>>>>    Previous behavior had the MAC address always being programmed into
>>>>> hardware
>>>>>    in the device's init() function.
>>>>>    +--------
>>>>> + EEPROM
>>>>> +--------
>>>>> +
>>>>> +Boards may come with an EEPROM specifically to store configuration
>>>>> bits, such
>>>>> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this
>>>>> feature.
>>>>> +Depending on the board, the EEPROM may be connected on various
>>>>> methods, but
>>>>> +currently, only the I2C bus can be used via
>>>>> CONFIG_NET_ETHADDR_EEPROM_I2C.
>>>>> +
>>>>> +The following config options are available,
>>>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom
>>>>> is present.
>>>>> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM,
>>>>> which
>>>>> +defaults to the very common 0x50. Small size EEPROM's generally use
>>>>> single byte
>>>>> +addressing but larger EEPROM's may use double byte addressing, which
>>>>> can be
>>>>> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
>>>>> +
>>>>> +Within the EEPROM, the MAC address can be stored on any arbitrary
>>>>> offset,
>>>>> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default
>>>>> however, allowing
>>>>> +the first 8 bytes to be used for an optional data, for example a
>>>>> configuration
>>>>> +struct where the mac address is part of.
>>>>> +
>>>>> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous
>>>>> ARP_HLEN
>>>>> +bytes. Whether to check this CRC8 or not is dependent on
>>>>> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
>>>>> +
>>>>> +To keep things nicely aligned, a final 'reserved' byte is added to
>>>>> the mac
>>>>> +address + crc8 combo.
>>>>> +
>>>>> +A board may want to store more information in its eeprom, using the
>>>>> following
>>>>> +example layout, this can be achieved.
>>>>> +
>>>>> +struct mac_addr {
>>>>> +    uint8_t mac[ARP_HLEN];
>>>>> +    uint8_t crc8;
>>>>> +    uint8_t reserved;
>>>>> +};
>>>>> +
>>>>> +struct config_eeprom {
>>>>> +    uint32_t magic;
>>>>> +    uint8_t version;
>>>>> +    uint8_t reserved[2];
>>>>> +    uint8_t mac_cnt;
>>>>> +    struct mac_addr[mac_cnt];
>>>>> +};
>>>>> +
>>>>> +Filling this in:
>>>>> +struct config_eeprom eeprom = {
>>>>> +    .magic = { 'M', 'g', 'i', 'c' },
>>>>> +    .reserved = { 0x00, 0x00 },
>>>>> +    .mac_cnt = 2,
>>>>> +    .mac_addr = {
>>>>> +        {
>>>>> +            .mac = {
>>>>> +                0x01, 0x23, 0x45,
>>>>> +                0x67, 0x89, 0xab,
>>>>> +            },
>>>>> +            .crc8 = 0xbe,
>>>>> +            .reserved = 0x00,
>>>>> +        }, {
>>>>> +            .mac = {
>>>>> +                0xba, 0x98, 0x76,
>>>>> +                0x54, 0x32, 0x10,
>>>>> +            },
>>>>> +            .crc8 = 0x82,
>>>>> +            .reserved = 0x00,
>>>>> +        },
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +The eeprom content would look like this.
>>>>> +
>>>>> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00
>>>>> |Mgic.....#Eg....|
>>>>> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
>>>>> +
>>>>> +This can be done from linux using the i2c-tools:
>>>>> +
>>>>> +i2cset I2CBUS 0x50 0x08 0x01
>>>>> +i2cset I2CBUS 0x50 0x09 0x23
>>>>> +i2cset I2CBUS 0x50 0x0a 0x45
>>>>> +i2cset I2CBUS 0x50 0x0b 0x67
>>>>> +i2cset I2CBUS 0x50 0x0c 0x89
>>>>> +i2cset I2CBUS 0x50 0x0d 0xab
>>>>> +i2cset I2CBUS 0x50 0x0e 0xbe
>>>>> +
>>>>> +Alternativly this can be done from the u-boot console as:
>>>>> +
>>>>> +u-boot> mm.b 0
>>>>> +00000000: 00 ? 01
>>>>> +00000001: 23 ? 23
>>>>> +00000002: 45 ? 45
>>>>> +00000003: 67 ? 67
>>>>> +00000004: 89 ? 89
>>>>> +00000005: ab ? ab
>>>>> +00000006: be ? be
>>>>> +00000007: 00 ? q
>>>>> +i2c dev I2CBUS
>>>>> +i2c write 0 50 8 7
>>>>> +i2c md 50 8
>>>>> +
>>>>>    -------
>>>>>     Usage
>>>>>    -------
>>>>> diff --git a/include/net.h b/include/net.h
>>>>> index 08f8af8..e50ab5d 100644
>>>>> --- a/include/net.h
>>>>> +++ b/include/net.h
>>>>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar
>>>>> *enetaddr);
>>>>>    int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>>>>      /**
>>>>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>>>>> + *
>>>>> + * This function tries to read the MAC address from an eeprom as can
>>>>> be read
>>>>> + * in docs/README.enetaddr.
>>>>> + *
>>>>> + * @index:    index of the interface to get the hwaddr for
>>>>> + * @enetaddr:    pointer for the found hwaddr. Needs to be atleast
>>>>> ARP_HLEN
>>>>> + * @return:    0 on success, non-zero is error status. Additionally
>>>>> hwaddr
>>>>> + *        is set to 00:00:00:00:00. This is also the case if
>>>>> + *        CONFIG_NET_ETHADDR_EEPROM is not set.
>>>>> + */
>>>>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>>>>> +
>>>>> +/**
>>>>>     * eth_setenv_enetaddr_by_index() - set the MAC address environment
>>>>> variable
>>>>>     *
>>>>>     * This sets up an environment variable with the given MAC address
>>>>> (@enetaddr).
>>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>>> index 414c549..f699e1c 100644
>>>>> --- a/net/Kconfig
>>>>> +++ b/net/Kconfig
>>>>> @@ -7,6 +7,65 @@ menuconfig NET
>>>>>      if NET
>>>>>    +config NET_ETHADDR_EEPROM
>>>>> +    bool "Get ethaddr from eeprom"
>>>>> +    help
>>>>> +      Selecting this will try to get the Ethernet address from an
>>>>> onboard
>>>>> +      EEPROM and set into the environment if and only if the
>>>>> environment
>>>>> +      does currently not already hold a MAC address. For more
>>>>> information
>>>>> +      see doc/README.enetaddr.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C
>>>>> +    depends on NET_ETHADDR_EEPROM
>>>>> +    bool "EEPROM on I2C bus"
>>>>> +    help
>>>>> +      This switch enables checks for an EEPROM on the I2C bus.
>>>>> Naturally
>>>>> +      this will only work if there is an actual EEPROM connected on
>>>>> the
>>>>> +      I2C bus and the bus and device are properly configured via the
>>>>> +      options below.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C_BUS
>>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>>> +    int "I2C bus"
>>>>> +    default 0
>>>>> +    help
>>>>> +      Select the bus on which the EEPROM is present, defaults to bus
>>>>> 0.
>>>>> +      Remember to also make the selected bus available via
>>>>> I2Cn_ENABLE.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C_ADDR
>>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>>> +    hex "EEPROM address"
>>>>> +    default 0x50
>>>>> +    help
>>>>> +      Select the address of the EEPROM, defaults to address 0x50.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
>>>>> +    depends on NET_ETHADDR_EEPROM_I2C
>>>>> +    int "EEPROM address length"
>>>>> +    default 1
>>>>> +    help
>>>>> +      Number of bytes to be used for the I2C address length.
>>>>> Typically 1,
>>>>> +      2 for large memories, 0 for register type devices with only one
>>>>> +      register.
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_OFFSET
>>>>> +    depends on NET_ETHADDR_EEPROM
>>>>> +    int "EEPROM offset"
>>>>> +    default 8
>>>>> +    help
>>>>> +      Select the byte offset of the MAC address within the page,
>>>>> +      defaults to byte 8.
>>>>
>>>> I would prefer to all these values to be in hex because i2c commands are
>>>> also taking values in hex.
>>>
>>> You are completly right and this is indeed my mistake. I will fix it for
>>> v2.
>>>
>>> Incidently I put them on 0x50 in my own config files for this exact
>>> reason. I probably did not realise I could make it default to hex :)
>>
>> ok.
>>
>>>>> +
>>>>> +config NET_ETHADDR_EEPROM_CRC8
>>>>> +    depends on NET_ETHADDR_EEPROM
>>>>> +    bool "Check CRC8 of MAC"
>>>>> +    default y
>>>>> +    help
>>>>> +      Optionally, it is possible to run a CRC-8-CCITT check on the MAC
>>>>> +      address. To do so, the MAC address is stored with a CRC8 byte
>>>>> append.
>>>>> +      This option enables the CRC check of the MAC address against
>>>>> the CRC
>>>>> +      byte.
>>>>> +
>>>>
>>>> Would it be possible to have default n here?
>>>> I would guess that more boards don't have this CRC8 sums.
>>>
>>> I agree, but most boards will not use this by default yet. If you enable
>>> this feature for your board, I strongly strongly recommend enabeling
>>> this feature as well. Thus disable it by user request.
>>
>> Hard to do it for boards in the field. As you see we have this feature
>> enabled for zcu102 and also zybo board and there is no crc8 sums factory
>> programmed.
>
> And that's why it is an option. If you turn on this option in your
> (board)config, you have the option as a board maintainer to set the crc8 to
> off, because you have boards in the field already. If I add a new board now
> however, and want to use this feature, it should not be to easy to disable
> it.
>>
>>
>>> Reason why I strongly recommend to enable it: If i have an unprogrammed
>>> eeprom, it comes filled with 0xffffffff. Which is interpreted as a
>>> correct mac address. What if i have random garbage in the eeprom (or a
>>> user change one bit by accident). I still have a valid mac address.
>>> Using the crc8 to validate the mac address makes this a lot more safe.
>>
>> Isn't 0xffffffff invalid address? I think I saw it as invalid.
>
> I know 0x000000 is 'invalid' 0xffffffff might be indeed. but random bits are
> valid mac addresses.

It is perfectly valid, just not as your board's MAC address. It is the
link-wide broadcast address. See is_broadcast_ethaddr()
Joe Hershberger Nov. 30, 2016, 8 p.m. UTC | #7
On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> This patch allows Kconfig to enable and set parameters to make it
> possible to read the MAC address from an EEPROM. The net core layer then
> uses this information to read MAC addresses from this EEPROM.
>
> Besides the various tuneables as to how to access the eeprom (bus,
> address, addressing mode/length, 2 configurable that are EEPROM generic
> (e.g. SPI or some other form of access) which are:
>
> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
> for other purposes (header MAGIC for example).
>
> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
> checksum that should be verified.
>
> Currently only I2C eeproms have been tested and thus only those options
> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
> just as created and added.
>
> The code currently first checks if there is a non-zero MAC address in
> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
> used by a board to supply the MAC in other ways.
>
> If both these fails, the other code is still in place to query the
> environent, which then can be used to override the hardware supplied
> data.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/net.h       | 14 ++++++++
>  net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>  net/eth-uclass.c    |  9 +++--
>  net/eth_common.c    | 34 ++++++++++++++++++
>  net/eth_legacy.c    |  2 ++
>  6 files changed, 214 insertions(+), 3 deletions(-)
>
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index 50e4899..89c1f7d 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address (summarized):
>  Previous behavior had the MAC address always being programmed into hardware
>  in the device's init() function.
>
> +--------
> + EEPROM
> +--------
> +
> +Boards may come with an EEPROM specifically to store configuration bits, such
> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this feature.
> +Depending on the board, the EEPROM may be connected on various methods, but
> +currently, only the I2C bus can be used via CONFIG_NET_ETHADDR_EEPROM_I2C.
> +
> +The following config options are available,
> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom is present.
> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM, which
> +defaults to the very common 0x50. Small size EEPROM's generally use single byte
> +addressing but larger EEPROM's may use double byte addressing, which can be
> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
> +
> +Within the EEPROM, the MAC address can be stored on any arbitrary offset,
> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default however, allowing
> +the first 8 bytes to be used for an optional data, for example a configuration
> +struct where the mac address is part of.
> +
> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous ARP_HLEN
> +bytes. Whether to check this CRC8 or not is dependent on
> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
> +
> +To keep things nicely aligned, a final 'reserved' byte is added to the mac
> +address + crc8 combo.
> +
> +A board may want to store more information in its eeprom, using the following
> +example layout, this can be achieved.
> +
> +struct mac_addr {
> +       uint8_t mac[ARP_HLEN];
> +       uint8_t crc8;
> +       uint8_t reserved;
> +};
> +
> +struct config_eeprom {
> +       uint32_t magic;
> +       uint8_t version;
> +       uint8_t reserved[2];
> +       uint8_t mac_cnt;
> +       struct mac_addr[mac_cnt];
> +};
> +
> +Filling this in:
> +struct config_eeprom eeprom = {
> +       .magic = { 'M', 'g', 'i', 'c' },
> +       .reserved = { 0x00, 0x00 },
> +       .mac_cnt = 2,
> +       .mac_addr = {
> +               {
> +                       .mac = {
> +                               0x01, 0x23, 0x45,
> +                               0x67, 0x89, 0xab,
> +                       },
> +                       .crc8 = 0xbe,
> +                       .reserved = 0x00,
> +               }, {
> +                       .mac = {
> +                               0xba, 0x98, 0x76,
> +                               0x54, 0x32, 0x10,
> +                       },
> +                       .crc8 = 0x82,
> +                       .reserved = 0x00,
> +               },
> +       },
> +};
> +
> +The eeprom content would look like this.
> +
> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00 |Mgic.....#Eg....|
> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
> +
> +This can be done from linux using the i2c-tools:
> +
> +i2cset I2CBUS 0x50 0x08 0x01
> +i2cset I2CBUS 0x50 0x09 0x23
> +i2cset I2CBUS 0x50 0x0a 0x45
> +i2cset I2CBUS 0x50 0x0b 0x67
> +i2cset I2CBUS 0x50 0x0c 0x89
> +i2cset I2CBUS 0x50 0x0d 0xab
> +i2cset I2CBUS 0x50 0x0e 0xbe
> +
> +Alternativly this can be done from the u-boot console as:
> +
> +u-boot> mm.b 0
> +00000000: 00 ? 01
> +00000001: 23 ? 23
> +00000002: 45 ? 45
> +00000003: 67 ? 67
> +00000004: 89 ? 89
> +00000005: ab ? ab
> +00000006: be ? be
> +00000007: 00 ? q
> +i2c dev I2CBUS
> +i2c write 0 50 8 7
> +i2c md 50 8
> +
>  -------
>   Usage
>  -------
> diff --git a/include/net.h b/include/net.h
> index 08f8af8..e50ab5d 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>  int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>
>  /**
> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
> + *
> + * This function tries to read the MAC address from an eeprom as can be read
> + * in docs/README.enetaddr.
> + *
> + * @index:     index of the interface to get the hwaddr for
> + * @enetaddr:  pointer for the found hwaddr. Needs to be atleast ARP_HLEN
> + * @return:    0 on success, non-zero is error status. Additionally hwaddr
> + *             is set to 00:00:00:00:00. This is also the case if
> + *             CONFIG_NET_ETHADDR_EEPROM is not set.
> + */
> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
> +
> +/**
>   * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>   *
>   * This sets up an environment variable with the given MAC address (@enetaddr).
> diff --git a/net/Kconfig b/net/Kconfig
> index 414c549..f699e1c 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -7,6 +7,65 @@ menuconfig NET
>
>  if NET
>
> +config NET_ETHADDR_EEPROM
> +       bool "Get ethaddr from eeprom"
> +       help
> +         Selecting this will try to get the Ethernet address from an onboard
> +         EEPROM and set into the environment if and only if the environment
> +         does currently not already hold a MAC address. For more information
> +         see doc/README.enetaddr.
> +
> +config NET_ETHADDR_EEPROM_I2C
> +       depends on NET_ETHADDR_EEPROM
> +       bool "EEPROM on I2C bus"
> +       help
> +         This switch enables checks for an EEPROM on the I2C bus. Naturally
> +         this will only work if there is an actual EEPROM connected on the
> +         I2C bus and the bus and device are properly configured via the
> +         options below.
> +
> +config NET_ETHADDR_EEPROM_I2C_BUS
> +       depends on NET_ETHADDR_EEPROM_I2C
> +       int "I2C bus"
> +       default 0
> +       help
> +         Select the bus on which the EEPROM is present, defaults to bus 0.
> +         Remember to also make the selected bus available via I2Cn_ENABLE.
> +
> +config NET_ETHADDR_EEPROM_I2C_ADDR
> +       depends on NET_ETHADDR_EEPROM_I2C
> +       hex "EEPROM address"
> +       default 0x50
> +       help
> +         Select the address of the EEPROM, defaults to address 0x50.
> +
> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
> +       depends on NET_ETHADDR_EEPROM_I2C
> +       int "EEPROM address length"
> +       default 1
> +       help
> +         Number of bytes to be used for the I2C address length. Typically 1,
> +         2 for large memories, 0 for register type devices with only one
> +         register.
> +
> +config NET_ETHADDR_EEPROM_OFFSET
> +       depends on NET_ETHADDR_EEPROM
> +       int "EEPROM offset"
> +       default 8
> +       help
> +         Select the byte offset of the MAC address within the page,
> +         defaults to byte 8.
> +
> +config NET_ETHADDR_EEPROM_CRC8
> +       depends on NET_ETHADDR_EEPROM
> +       bool "Check CRC8 of MAC"
> +       default y
> +       help
> +         Optionally, it is possible to run a CRC-8-CCITT check on the MAC
> +         address. To do so, the MAC address is stored with a CRC8 byte append.
> +         This option enables the CRC check of the MAC address against the CRC
> +         byte.
> +
>  config NET_RANDOM_ETHADDR
>         bool "Random ethaddr if unset"
>         select LIB_RAND
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 5c888b8..a154a7e 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -491,9 +491,12 @@ static int eth_post_probe(struct udevice *dev)
>
>         priv->state = ETH_STATE_INIT;
>
> -       /* Check if the device has a MAC address in ROM */
> -       if (eth_get_ops(dev)->read_rom_hwaddr)
> -               eth_get_ops(dev)->read_rom_hwaddr(dev);
> +       /* Check if the device has a MAC address in EEPROM */
> +       eeprom_read_enetaddr(dev->seq, pdata->enetaddr);
> +       if (is_zero_ethaddr(pdata->enetaddr))
> +               /* Check if the device has a MAC address in ROM */
> +               if (eth_get_ops(dev)->read_rom_hwaddr)
> +                       eth_get_ops(dev)->read_rom_hwaddr(dev);
>
>         eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>         if (!is_zero_ethaddr(env_enetaddr)) {
> diff --git a/net/eth_common.c b/net/eth_common.c
> index 079be89..e0d8b62 100644
> --- a/net/eth_common.c
> +++ b/net/eth_common.c
> @@ -8,10 +8,44 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <i2c.h>
>  #include <miiphy.h>
>  #include <net.h>
>  #include "eth_internal.h"
>
> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr)
> +{
> +       uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
> +#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)

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

> +       int old_i2c_bus;
> +
> +       old_i2c_bus = i2c_get_bus_num();
> +       if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)
> +               i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);
> +       /* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7 from the eeprom */
> +       if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
> +                    CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index * (ARP_HLEN + 2)),
> +                    CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,
> +                    eeprom, ARP_HLEN + 1)) {
> +               i2c_set_bus_num(old_i2c_bus);
> +               puts("Could not read the EEPROM or EEPROM missing on device: ");
> +               return -ENOSYS;
> +       }
> +       i2c_set_bus_num(old_i2c_bus);
> +
> +#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
> +       if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {
> +               puts("CRC error on MAC address from EEPROM on device: ");
> +               return -EINVAL;
> +       }
> +#endif
> +#endif
> +
> +       memcpy(enetaddr, eeprom, ARP_HLEN);
> +
> +       return 0;
> +}
> +
>  void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>  {
>         char *end;
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index bf4de37..8fb5844 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>         unsigned char env_enetaddr[ARP_HLEN];
>         int ret = 0;
>
> +       eeprom_read_enetaddr(eth_number, dev->enetaddr);
> +
>         eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>
>         if (!is_zero_ethaddr(env_enetaddr)) {
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Olliver Schinagl Dec. 2, 2016, 10:12 a.m. UTC | #8
Hey Joe,


On 30-11-16 21:00, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> This patch allows Kconfig to enable and set parameters to make it
>> possible to read the MAC address from an EEPROM. The net core layer then
>> uses this information to read MAC addresses from this EEPROM.
>>
>> Besides the various tuneables as to how to access the eeprom (bus,
>> address, addressing mode/length, 2 configurable that are EEPROM generic
>> (e.g. SPI or some other form of access) which are:
>>
>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
>> for other purposes (header MAGIC for example).
>>
>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
>> checksum that should be verified.
>>
>> Currently only I2C eeproms have been tested and thus only those options
>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>> just as created and added.
>>
>> The code currently first checks if there is a non-zero MAC address in
>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>> used by a board to supply the MAC in other ways.
>>
>> If both these fails, the other code is still in place to query the
>> environent, which then can be used to override the hardware supplied
>> data.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   doc/README.enetaddr | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/net.h       | 14 ++++++++
>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>   net/eth-uclass.c    |  9 +++--
>>   net/eth_common.c    | 34 ++++++++++++++++++
>>   net/eth_legacy.c    |  2 ++
>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
>> index 50e4899..89c1f7d 100644
>> --- a/doc/README.enetaddr
>> +++ b/doc/README.enetaddr
>> @@ -47,6 +47,105 @@ Correct flow of setting up the MAC address (summarized):
>>   Previous behavior had the MAC address always being programmed into hardware
>>   in the device's init() function.
>>
>> +--------
>> + EEPROM
>> +--------
>> +
>> +Boards may come with an EEPROM specifically to store configuration bits, such
>> +as a MAC address. Using CONFIG_NET_ETHADDR_EEPROM enables this feature.
>> +Depending on the board, the EEPROM may be connected on various methods, but
>> +currently, only the I2C bus can be used via CONFIG_NET_ETHADDR_EEPROM_I2C.
>> +
>> +The following config options are available,
>> +CONFIG_NET_ETHADDR_EEPROM_I2C_BUS is the I2C bus on which the eeprom is present.
>> +CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR sets the address of the EEPROM, which
>> +defaults to the very common 0x50. Small size EEPROM's generally use single byte
>> +addressing but larger EEPROM's may use double byte addressing, which can be
>> +configured using CONFIG_NET_ETHADDR_EEPROM_ADDRLEN.
>> +
>> +Within the EEPROM, the MAC address can be stored on any arbitrary offset,
>> +CONFIG_NET_ETHADDR_EEPROM_OFFSET sets this to 8 as a default however, allowing
>> +the first 8 bytes to be used for an optional data, for example a configuration
>> +struct where the mac address is part of.
>> +
>> +Appending the 6 (ARP_HLEN) bytes is a CRC8 byte over the previous ARP_HLEN
>> +bytes. Whether to check this CRC8 or not is dependent on
>> +CONFIG_NET_ETHADDR_EEPROM_CRC8.
>> +
>> +To keep things nicely aligned, a final 'reserved' byte is added to the mac
>> +address + crc8 combo.
>> +
>> +A board may want to store more information in its eeprom, using the following
>> +example layout, this can be achieved.
>> +
>> +struct mac_addr {
>> +       uint8_t mac[ARP_HLEN];
>> +       uint8_t crc8;
>> +       uint8_t reserved;
>> +};
>> +
>> +struct config_eeprom {
>> +       uint32_t magic;
>> +       uint8_t version;
>> +       uint8_t reserved[2];
>> +       uint8_t mac_cnt;
>> +       struct mac_addr[mac_cnt];
>> +};
>> +
>> +Filling this in:
>> +struct config_eeprom eeprom = {
>> +       .magic = { 'M', 'g', 'i', 'c' },
>> +       .reserved = { 0x00, 0x00 },
>> +       .mac_cnt = 2,
>> +       .mac_addr = {
>> +               {
>> +                       .mac = {
>> +                               0x01, 0x23, 0x45,
>> +                               0x67, 0x89, 0xab,
>> +                       },
>> +                       .crc8 = 0xbe,
>> +                       .reserved = 0x00,
>> +               }, {
>> +                       .mac = {
>> +                               0xba, 0x98, 0x76,
>> +                               0x54, 0x32, 0x10,
>> +                       },
>> +                       .crc8 = 0x82,
>> +                       .reserved = 0x00,
>> +               },
>> +       },
>> +};
>> +
>> +The eeprom content would look like this.
>> +
>> +00000000  4d 67 69 63 01 00 00 02  01 23 45 67 89 ab be 00 |Mgic.....#Eg....|
>> +00000010  ba 98 76 54 32 10 82 00                          |..vT2...|
>> +
>> +This can be done from linux using the i2c-tools:
>> +
>> +i2cset I2CBUS 0x50 0x08 0x01
>> +i2cset I2CBUS 0x50 0x09 0x23
>> +i2cset I2CBUS 0x50 0x0a 0x45
>> +i2cset I2CBUS 0x50 0x0b 0x67
>> +i2cset I2CBUS 0x50 0x0c 0x89
>> +i2cset I2CBUS 0x50 0x0d 0xab
>> +i2cset I2CBUS 0x50 0x0e 0xbe
>> +
>> +Alternativly this can be done from the u-boot console as:
>> +
>> +u-boot> mm.b 0
>> +00000000: 00 ? 01
>> +00000001: 23 ? 23
>> +00000002: 45 ? 45
>> +00000003: 67 ? 67
>> +00000004: 89 ? 89
>> +00000005: ab ? ab
>> +00000006: be ? be
>> +00000007: 00 ? q
>> +i2c dev I2CBUS
>> +i2c write 0 50 8 7
>> +i2c md 50 8
>> +
>>   -------
>>    Usage
>>   -------
>> diff --git a/include/net.h b/include/net.h
>> index 08f8af8..e50ab5d 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -248,6 +248,20 @@ int eth_getenv_enetaddr(const char *name, uchar *enetaddr);
>>   int eth_setenv_enetaddr(const char *name, const uchar *enetaddr);
>>
>>   /**
>> + * eeprom_read_enetaddr() - Read the hardware address from an eeprom
>> + *
>> + * This function tries to read the MAC address from an eeprom as can be read
>> + * in docs/README.enetaddr.
>> + *
>> + * @index:     index of the interface to get the hwaddr for
>> + * @enetaddr:  pointer for the found hwaddr. Needs to be atleast ARP_HLEN
>> + * @return:    0 on success, non-zero is error status. Additionally hwaddr
>> + *             is set to 00:00:00:00:00. This is also the case if
>> + *             CONFIG_NET_ETHADDR_EEPROM is not set.
>> + */
>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr);
>> +
>> +/**
>>    * eth_setenv_enetaddr_by_index() - set the MAC address environment variable
>>    *
>>    * This sets up an environment variable with the given MAC address (@enetaddr).
>> diff --git a/net/Kconfig b/net/Kconfig
>> index 414c549..f699e1c 100644
>> --- a/net/Kconfig
>> +++ b/net/Kconfig
>> @@ -7,6 +7,65 @@ menuconfig NET
>>
>>   if NET
>>
>> +config NET_ETHADDR_EEPROM
>> +       bool "Get ethaddr from eeprom"
>> +       help
>> +         Selecting this will try to get the Ethernet address from an onboard
>> +         EEPROM and set into the environment if and only if the environment
>> +         does currently not already hold a MAC address. For more information
>> +         see doc/README.enetaddr.
>> +
>> +config NET_ETHADDR_EEPROM_I2C
>> +       depends on NET_ETHADDR_EEPROM
>> +       bool "EEPROM on I2C bus"
>> +       help
>> +         This switch enables checks for an EEPROM on the I2C bus. Naturally
>> +         this will only work if there is an actual EEPROM connected on the
>> +         I2C bus and the bus and device are properly configured via the
>> +         options below.
>> +
>> +config NET_ETHADDR_EEPROM_I2C_BUS
>> +       depends on NET_ETHADDR_EEPROM_I2C
>> +       int "I2C bus"
>> +       default 0
>> +       help
>> +         Select the bus on which the EEPROM is present, defaults to bus 0.
>> +         Remember to also make the selected bus available via I2Cn_ENABLE.
>> +
>> +config NET_ETHADDR_EEPROM_I2C_ADDR
>> +       depends on NET_ETHADDR_EEPROM_I2C
>> +       hex "EEPROM address"
>> +       default 0x50
>> +       help
>> +         Select the address of the EEPROM, defaults to address 0x50.
>> +
>> +config NET_ETHADDR_EEPROM_I2C_ADDRLEN
>> +       depends on NET_ETHADDR_EEPROM_I2C
>> +       int "EEPROM address length"
>> +       default 1
>> +       help
>> +         Number of bytes to be used for the I2C address length. Typically 1,
>> +         2 for large memories, 0 for register type devices with only one
>> +         register.
>> +
>> +config NET_ETHADDR_EEPROM_OFFSET
>> +       depends on NET_ETHADDR_EEPROM
>> +       int "EEPROM offset"
>> +       default 8
>> +       help
>> +         Select the byte offset of the MAC address within the page,
>> +         defaults to byte 8.
>> +
>> +config NET_ETHADDR_EEPROM_CRC8
>> +       depends on NET_ETHADDR_EEPROM
>> +       bool "Check CRC8 of MAC"
>> +       default y
>> +       help
>> +         Optionally, it is possible to run a CRC-8-CCITT check on the MAC
>> +         address. To do so, the MAC address is stored with a CRC8 byte append.
>> +         This option enables the CRC check of the MAC address against the CRC
>> +         byte.
>> +
>>   config NET_RANDOM_ETHADDR
>>          bool "Random ethaddr if unset"
>>          select LIB_RAND
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index 5c888b8..a154a7e 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -491,9 +491,12 @@ static int eth_post_probe(struct udevice *dev)
>>
>>          priv->state = ETH_STATE_INIT;
>>
>> -       /* Check if the device has a MAC address in ROM */
>> -       if (eth_get_ops(dev)->read_rom_hwaddr)
>> -               eth_get_ops(dev)->read_rom_hwaddr(dev);
>> +       /* Check if the device has a MAC address in EEPROM */
>> +       eeprom_read_enetaddr(dev->seq, pdata->enetaddr);
>> +       if (is_zero_ethaddr(pdata->enetaddr))
>> +               /* Check if the device has a MAC address in ROM */
>> +               if (eth_get_ops(dev)->read_rom_hwaddr)
>> +                       eth_get_ops(dev)->read_rom_hwaddr(dev);
>>
>>          eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>>          if (!is_zero_ethaddr(env_enetaddr)) {
>> diff --git a/net/eth_common.c b/net/eth_common.c
>> index 079be89..e0d8b62 100644
>> --- a/net/eth_common.c
>> +++ b/net/eth_common.c
>> @@ -8,10 +8,44 @@
>>
>>   #include <common.h>
>>   #include <dm.h>
>> +#include <i2c.h>
>>   #include <miiphy.h>
>>   #include <net.h>
>>   #include "eth_internal.h"
>>
>> +int eeprom_read_enetaddr(const int index, unsigned char *enetaddr)
>> +{
>> +       uint8_t eeprom[ARP_HLEN + 1] = { 0x00 };
>> +#if defined(CONFIG_NET_ETHADDR_EEPROM) && defined(CONFIG_NET_ETHADDR_EEPROM_I2C)
> Since it is easily reasonable that SPI PROM is a likely useful
> support, why not keep the layout stuff separate from the I2C stuff so
> that it is trivial to plug in a different bus later? It will also make
> the code clearer by untangling these.

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

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

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

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

Olliver
>
>> +       int old_i2c_bus;
>> +
>> +       old_i2c_bus = i2c_get_bus_num();
>> +       if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)
>> +               i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);
>> +       /* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7 from the eeprom */
>> +       if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
>> +                    CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index * (ARP_HLEN + 2)),
>> +                    CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,
>> +                    eeprom, ARP_HLEN + 1)) {
>> +               i2c_set_bus_num(old_i2c_bus);
>> +               puts("Could not read the EEPROM or EEPROM missing on device: ");
>> +               return -ENOSYS;
>> +       }
>> +       i2c_set_bus_num(old_i2c_bus);
>> +
>> +#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
>> +       if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {
>> +               puts("CRC error on MAC address from EEPROM on device: ");
>> +               return -EINVAL;
>> +       }
>> +#endif
>> +#endif
>> +
>> +       memcpy(enetaddr, eeprom, ARP_HLEN);
>> +
>> +       return 0;
>> +}
>> +
>>   void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>>   {
>>          char *end;
>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>> index bf4de37..8fb5844 100644
>> --- a/net/eth_legacy.c
>> +++ b/net/eth_legacy.c
>> @@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>>          unsigned char env_enetaddr[ARP_HLEN];
>>          int ret = 0;
>>
>> +       eeprom_read_enetaddr(eth_number, dev->enetaddr);
>> +
>>          eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>>
>>          if (!is_zero_ethaddr(env_enetaddr)) {
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
Joe Hershberger Dec. 9, 2016, 6:11 p.m. UTC | #9
On Fri, Dec 2, 2016 at 4:12 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Joe,
>
>
>
> On 30-11-16 21:00, Joe Hershberger wrote:
>>
>> On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl>
>> wrote:
>>>
>>> This patch allows Kconfig to enable and set parameters to make it
>>> possible to read the MAC address from an EEPROM. The net core layer then
>>> uses this information to read MAC addresses from this EEPROM.
>>>
>>> Besides the various tuneables as to how to access the eeprom (bus,
>>> address, addressing mode/length, 2 configurable that are EEPROM generic
>>> (e.g. SPI or some other form of access) which are:
>>>
>>> NET_ETHADDR_EEPROM_OFFSET, indicating where in the EEPROM the start of
>>> the MAC address is. The default is 8 allowing for 8 bytes before the MAC
>>> for other purposes (header MAGIC for example).
>>>
>>> NET_ETHADDR_EEPROM_CRC8, indicating the MAC is appended with a CRC8-CCIT
>>> checksum that should be verified.
>>>
>>> Currently only I2C eeproms have been tested and thus only those options
>>> are available, but shouldn't be a limit. NET_ETHADDR_EEPROM_SPI can be
>>> just as created and added.
>>>
>>> The code currently first checks if there is a non-zero MAC address in
>>> the eeprom. If that fails to be the case, the read_rom_hwaddr can be
>>> used by a board to supply the MAC in other ways.
>>>
>>> If both these fails, the other code is still in place to query the
>>> environent, which then can be used to override the hardware supplied
>>> data.
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   doc/README.enetaddr | 99
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/net.h       | 14 ++++++++
>>>   net/Kconfig         | 59 +++++++++++++++++++++++++++++++
>>>   net/eth-uclass.c    |  9 +++--
>>>   net/eth_common.c    | 34 ++++++++++++++++++
>>>   net/eth_legacy.c    |  2 ++
>>>   6 files changed, 214 insertions(+), 3 deletions(-)
>>>

...

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

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

> Olliver
>
>>
>>> +       int old_i2c_bus;
>>> +
>>> +       old_i2c_bus = i2c_get_bus_num();
>>> +       if (old_i2c_bus != CONFIG_NET_ETHADDR_EEPROM_I2C_BUS)
>>> +               i2c_set_bus_num(CONFIG_NET_ETHADDR_EEPROM_I2C_BUS);
>>> +       /* Skip in blocks of 8 (ARP + CRC8 + pad), but read 7 from the
>>> eeprom */
>>> +       if (i2c_read(CONFIG_NET_ETHADDR_EEPROM_I2C_ADDR,
>>> +                    CONFIG_NET_ETHADDR_EEPROM_OFFSET + (index *
>>> (ARP_HLEN + 2)),
>>> +                    CONFIG_NET_ETHADDR_EEPROM_I2C_ADDRLEN,
>>> +                    eeprom, ARP_HLEN + 1)) {
>>> +               i2c_set_bus_num(old_i2c_bus);
>>> +               puts("Could not read the EEPROM or EEPROM missing on
>>> device: ");
>>> +               return -ENOSYS;
>>> +       }
>>> +       i2c_set_bus_num(old_i2c_bus);
>>> +
>>> +#ifdef CONFIG_NET_ETHADDR_EEPROM_CRC8
>>> +       if (crc8(0, eeprom, ARP_HLEN) != eeprom[ARP_HLEN]) {
>>> +               puts("CRC error on MAC address from EEPROM on device: ");
>>> +               return -EINVAL;
>>> +       }
>>> +#endif
>>> +#endif
>>> +
>>> +       memcpy(enetaddr, eeprom, ARP_HLEN);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>>>   {
>>>          char *end;
>>> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
>>> index bf4de37..8fb5844 100644
>>> --- a/net/eth_legacy.c
>>> +++ b/net/eth_legacy.c
>>> @@ -136,6 +136,8 @@ int eth_write_hwaddr(struct eth_device *dev, const
>>> char *base_name,
>>>          unsigned char env_enetaddr[ARP_HLEN];
>>>          int ret = 0;
>>>
>>> +       eeprom_read_enetaddr(eth_number, dev->enetaddr);
>>> +
>>>          eth_getenv_enetaddr_by_index(base_name, eth_number,
>>> env_enetaddr);
>>>
>>>          if (!is_zero_ethaddr(env_enetaddr)) {
>>> --
>>> 2.10.2
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
diff mbox

Patch

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