[U-Boot] [PATCH v5 08/12] SPEAr : Support for HW mac id read/write from i2c mem

[Ben Warren]

Hi Vipin,

Sorry for chiming in so late in the review process.

Vipin KUMAR wrote:
> This patch adds the  support to read and write mac id from i2c
> memory.
> For reading:
> 	if (env contains ethaddr)
> 		pick env ethaddr
> 	else
> 		pick ethaddr from i2c memory
> For writing:
> 	chip_config ethaddr XX:XX:XX:XX:XX:XX writes the mac id
> 	in i2c memory
>
> Signed-off-by: Vipin <vipin.kumar at st.com>
> ---
>  board/spear/common/spr_misc.c         |   68 ++++++++++++++++++++++++++++++++-
>  include/asm-arm/arch-spear/spr_defs.h |    8 ++++
>  2 files changed, 75 insertions(+), 1 deletions(-)
>
> diff --git a/board/spear/common/spr_misc.c b/board/spear/common/spr_misc.c
> index dfa3ece..204ccf2 100755
> --- a/board/spear/common/spr_misc.c
> +++ b/board/spear/common/spr_misc.c
> @@ -67,6 +67,12 @@ int dram_init(void)
>  
>  int misc_init_r(void)
>  {
> +#if defined(CONFIG_CMD_NET)
> +	uchar mac_id[6];
> +
> +	if (!eth_getenv_enetaddr("ethaddr", mac_id) && !i2c_read_mac(mac_id))
> +		eth_setenv_enetaddr("ethaddr", mac_id);
>   
It's really not a good idea to programatically set an environment 
variable like this.  Also, there's already a well-defined and documented 
protocol for  how MAC addresses should be read and the precedence 
between EEPROM storage and environment storage.  Please find a way to 
fit into that model.  Since you don't have network support yet, I have 
no idea which network controller you're using, and recommend waiting 
until this is really needed.
> +#endif
>  	setenv("verify", "n");
>  
>  #if defined(CONFIG_SPEAR_USBTTY)
> @@ -101,12 +107,54 @@ int spear_board_init(ulong mach_type)
>  	return 0;
>  }
>  
> +static int i2c_read_mac(uchar *buffer)
> +{
> +	u8 buf[2];
> +
> +	i2c_read(CONFIG_I2C_CHIPADDRESS, MAGIC_OFF, 1, buf, MAGIC_LEN);
> +
> +	/* Check if mac in i2c memory is valid */
> +	if ((buf[0] == MAGIC_BYTE0) && (buf[1] == MAGIC_BYTE1)) {
> +		/* Valid mac address is saved in i2c eeprom */
> +		i2c_read(CONFIG_I2C_CHIPADDRESS, MAC_OFF, 1, buffer, MAC_LEN);
> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +static int write_mac(uchar *mac)
> +{
> +	u8 buf[2];
> +
> +	buf[0] = (u8)MAGIC_BYTE0;
> +	buf[1] = (u8)MAGIC_BYTE1;
> +	i2c_write(CONFIG_I2C_CHIPADDRESS, MAGIC_OFF, 1, buf, MAGIC_LEN);
> +
> +	buf[0] = (u8)~MAGIC_BYTE0;
> +	buf[1] = (u8)~MAGIC_BYTE1;
> +
> +	i2c_read(CONFIG_I2C_CHIPADDRESS, MAGIC_OFF, 1, buf, MAGIC_LEN);
> +
> +	/* check if valid MAC address is saved in I2C EEPROM or not? */
> +	if ((buf[0] == MAGIC_BYTE0) && (buf[1] == MAGIC_BYTE1)) {
> +		i2c_write(CONFIG_I2C_CHIPADDRESS, MAC_OFF, 1, mac, MAC_LEN);
> +		puts("I2C EEPROM written with mac address \n");
> +		return 0;
> +	}
> +
> +	puts("I2C EEPROM writing failed \n");
> +	return -1;
> +}
> +
>  int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  {
>  	void (*sram_setfreq) (unsigned int, unsigned int);
>  	struct chip_data *chip = &chip_data;
>  	unsigned char mac[6];
> -	unsigned int frequency;
> +	unsigned int reg, frequency;
> +	char *s, *e;
> +	char i2c_mac[20];
>  
>  	if ((argc > 3) || (argc < 2)) {
>  		cmd_usage(cmdtp);
> @@ -137,6 +185,17 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  		}
>  
>  		return 0;
> +	} else if (!strcmp(argv[1], "ethaddr")) {
> +
> +		s = argv[2];
> +		for (reg = 0; reg < 6; ++reg) {
> +			mac[reg] = s ? simple_strtoul(s, &e, 16) : 0;
> +			if (s)
> +				s = (*e) ? e + 1 : e;
> +		}
> +		write_mac(mac);
> +
> +		return 0;
>  	} else if (!strcmp(argv[1], "print")) {
>  
>  		if (chip->cpufreq == -1)
> @@ -156,6 +215,13 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  		else
>  			printf("DDR Type    = Not Known\n");
>  
> +		if (!i2c_read_mac(mac)) {
> +			sprintf(i2c_mac, "%pM", mac);
> +			printf("Ethaddr (from i2c mem) = %s\n", i2c_mac);
>   
Maybe I'm missing some context, but why's the sprintf needed?
> +		} else {
> +			printf("Ethaddr (from i2c mem) = Not set\n");
> +		}
> +
>  		printf("Xloader Rev = %s\n", chip->version);
>  
>  		return 0;
> diff --git a/include/asm-arm/arch-spear/spr_defs.h b/include/asm-arm/arch-spear/spr_defs.h
> index 9dde54a..fa8412c 100644
> --- a/include/asm-arm/arch-spear/spr_defs.h
> +++ b/include/asm-arm/arch-spear/spr_defs.h
> @@ -35,4 +35,12 @@ struct chip_data {
>  	uchar version[32];
>  };
>  
> +/* HW mac id in i2c memory definitions */
> +#define MAGIC_OFF	0x0
> +#define MAGIC_LEN	0x2
> +#define MAGIC_BYTE0	0x55
> +#define MAGIC_BYTE1	0xAA
> +#define MAC_OFF		0x2
> +#define MAC_LEN		0x6
> +
>  #endif
>   
regards,
Ben