Patchwork [U-Boot,03/25] SPEAr: Place ethaddr write and read within CONFIG_CMD_NET

login
register
mail settings
Submitter Amit Virdi
Date March 7, 2012, 12:03 p.m.
Message ID <1331121854-20494-4-git-send-email-amit.virdi@st.com>
Download mbox | patch
Permalink /patch/145212/
State Accepted
Commit 8026b1e42f533f14115bb629efeaaedec6eaf23b
Delegated to: Stefan Roese
Headers show

Comments

Amit Virdi - March 7, 2012, 12:03 p.m.
From: Vipin KUMAR <vipin.kumar@st.com>

ethaddr can be optionally read from i2c memory. So, chip_config command supports
reading/writing hw mac id into i2c memory. Placing this code within
CONFIG_CMD_NET as this would only be needed when network interface is configured

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
Signed-off-by: Amit Virdi <amit.virdi@st.com>
---
 board/spear/common/spr_misc.c |   29 +++++++++++++++++++++--------
 doc/README.spear              |    8 ++++++++
 2 files changed, 29 insertions(+), 8 deletions(-)
Stefan Roese - March 7, 2012, 1:21 p.m.
On Wednesday 07 March 2012 13:03:52 Amit Virdi wrote:
> From: Vipin KUMAR <vipin.kumar@st.com>
> 
> ethaddr can be optionally read from i2c memory. So, chip_config command
> supports reading/writing hw mac id into i2c memory. Placing this code
> within CONFIG_CMD_NET as this would only be needed when network interface
> is configured
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> Signed-off-by: Amit Virdi <amit.virdi@st.com>

I'm not really sure, why you have this special ethaddr handling in I2C prom 
here at all. Isn't it enough to have the MAC address (ethaddr) stored in the 
U-Boot environment (NOR, NAND, etc)? Why do you need this additional I2C 
stuff? Could you please explain this?

Thanks,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Mike Frysinger - March 7, 2012, 1:29 p.m.
On Wednesday 07 March 2012 08:21:08 Stefan Roese wrote:
> On Wednesday 07 March 2012 13:03:52 Amit Virdi wrote:
> > From: Vipin KUMAR <vipin.kumar@st.com>
> > 
> > ethaddr can be optionally read from i2c memory. So, chip_config command
> > supports reading/writing hw mac id into i2c memory. Placing this code
> > within CONFIG_CMD_NET as this would only be needed when network interface
> > is configured
> > 
> > Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> > Signed-off-by: Amit Virdi <amit.virdi@st.com>
> 
> I'm not really sure, why you have this special ethaddr handling in I2C prom
> here at all. Isn't it enough to have the MAC address (ethaddr) stored in
> the U-Boot environment (NOR, NAND, etc)? Why do you need this additional
> I2C stuff? Could you please explain this?

if the board is coming from the factory manufactured like this, then it makes 
sense to me to support it.  after all, development boards aren't sold as "only 
runs u-boot".  if i got a dev board and installed u-boot on it myself, i'd 
really prefer it to automatically look up the mac if it's stored locally.

that said, this looks like a debug command, so not sure if i would even bother 
making it depend on CONFIG_CMD_NET if there's no real runtime overhead ...
-mike
Amit Virdi - March 26, 2012, 11:23 a.m.
Hi Stefan,

Sorry for this delayed response, I'm back to office after almost 2 weeks.

On 3/7/2012 6:51 PM, Stefan Roese wrote:
> On Wednesday 07 March 2012 13:03:52 Amit Virdi wrote:
>> From: Vipin KUMAR<vipin.kumar@st.com>
>>
>> ethaddr can be optionally read from i2c memory. So, chip_config command
>> supports reading/writing hw mac id into i2c memory. Placing this code
>> within CONFIG_CMD_NET as this would only be needed when network interface
>> is configured
>>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> Signed-off-by: Amit Virdi<amit.virdi@st.com>
>
> I'm not really sure, why you have this special ethaddr handling in I2C prom
> here at all. Isn't it enough to have the MAC address (ethaddr) stored in the
> U-Boot environment (NOR, NAND, etc)? Why do you need this additional I2C
> stuff? Could you please explain this?
>

Generally MAC addresses are stored in EEPROMs due to two reasons:
1. Special set of command sequences are required to erase data from an 
EEPROM
2. EEPROMs are cheaper

As a result, board manufacturers provide an EEPROM memory that contains 
MAC address and other vital board parameters. In such case, NAND/NOR 
memories may not have ethaddr stored. On SPEAr, I2C is used to access 
EEPROM. So this patch is required. Please let me know if I understood 
your question correctly and, hence, gave the right answer.

Thanks n Regards
Amit Virdi
Wolfgang Denk - March 26, 2012, 1:15 p.m.
Dear Amit Virdi,

In message <4F7051AF.9080905@st.com> you wrote:
> 
> Generally MAC addresses are stored in EEPROMs due to two reasons:
> 1. Special set of command sequences are required to erase data from an 
> EEPROM
> 2. EEPROMs are cheaper

Hm... With U-Boot, an EEPROM is just an additional wart, which costs
_extra_ money.

Best regards,

Wolfgang Denk

Patch

diff --git a/board/spear/common/spr_misc.c b/board/spear/common/spr_misc.c
index be96c15..e2918ff 100644
--- a/board/spear/common/spr_misc.c
+++ b/board/spear/common/spr_misc.c
@@ -36,6 +36,10 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#if defined(CONFIG_CMD_NET)
+static int i2c_read_mac(uchar *buffer);
+#endif
+
 int dram_init(void)
 {
 	/* Store complete RAM size and return */
@@ -136,6 +140,7 @@  int spear_board_init(ulong mach_type)
 	return 0;
 }
 
+#if defined(CONFIG_CMD_NET)
 static int i2c_read_mac(uchar *buffer)
 {
 	u8 buf[2];
@@ -172,17 +177,18 @@  static int write_mac(uchar *mac)
 		return 0;
 	}
 
-	puts("I2C EEPROM writing failed \n");
+	puts("I2C EEPROM writing failed\n");
 	return -1;
 }
+#endif
 
 int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	void (*sram_setfreq) (unsigned int, unsigned int);
+	unsigned int frequency;
+#if defined(CONFIG_CMD_NET)
 	unsigned char mac[6];
-	unsigned int reg, frequency;
-	char *s, *e;
-	char i2c_mac[20];
+#endif
 
 	if ((argc > 3) || (argc < 2))
 		return cmd_usage(cmdtp);
@@ -207,9 +213,12 @@  int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		}
 
 		return 0;
+
+#if defined(CONFIG_CMD_NET)
 	} else if (!strcmp(argv[1], "ethaddr")) {
 
-		s = argv[2];
+		u32 reg;
+		char *e, *s = argv[2];
 		for (reg = 0; reg < 6; ++reg) {
 			mac[reg] = s ? simple_strtoul(s, &e, 16) : 0;
 			if (s)
@@ -218,14 +227,15 @@  int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		write_mac(mac);
 
 		return 0;
+#endif
 	} else if (!strcmp(argv[1], "print")) {
+#if defined(CONFIG_CMD_NET)
 		if (!i2c_read_mac(mac)) {
-			sprintf(i2c_mac, "%pM", mac);
-			printf("Ethaddr (from i2c mem) = %s\n", i2c_mac);
+			printf("Ethaddr (from i2c mem) = %pM\n", mac);
 		} else {
 			printf("Ethaddr (from i2c mem) = Not set\n");
 		}
-
+#endif
 		return 0;
 	}
 
@@ -235,4 +245,7 @@  int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 U_BOOT_CMD(chip_config, 3, 1, do_chip_config,
 	   "configure chip",
 	   "chip_config cpufreq/ddrfreq frequency\n"
+#if defined(CONFIG_CMD_NET)
+	   "chip_config ethaddr XX:XX:XX:XX:XX:XX\n"
+#endif
 	   "chip_config print");
diff --git a/doc/README.spear b/doc/README.spear
index a8b1052..a6ff7fd 100644
--- a/doc/README.spear
+++ b/doc/README.spear
@@ -46,3 +46,11 @@  Further options
 	make FLASH=PNOR (supported by SPEAr310 and SPEAr320)
 	- This option generates a uboot image that supports emi controller for
 	CFI compliant parallel NOR flash
+
+Mac id storage and retrieval in spear platforms
+
+Please read doc/README.enetaddr for the implementation guidelines for mac id
+usage. Basically, environment has precedence over board specific storage. The
+ethaddr beeing used for the network interface is always taken only from
+environment variables. Although, we can check the mac id programmed in i2c
+memory by using chip_config command