[U-Boot] [PATCH] ne2000: take MAC address from config if available

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Daniel Mack]

Hi,

the board I'm currently working on has an ASIX AX88796 NE2000 clone but
no EEPROM attached to it. Hence, the get_prom() routine returns zeros
only so the system won't work.

This patch takes the MAC address given by CONFIG_ETHADDR and translates
it to numeric values. This could probably go to some other, more generic
place, but I didn't find any.

Best regards,
Daniel


diff --git a/drivers/net/ne2000_base.c b/drivers/net/ne2000_base.c
index f93f932..f8480a3 100644
--- a/drivers/net/ne2000_base.c
+++ b/drivers/net/ne2000_base.c
@@ -693,6 +693,7 @@ int eth_init(bd_t *bd) {
 
 	nic.base = (u8 *) CONFIG_DRIVER_NE2000_BASE;
 
+#ifndef CONFIG_ETHADDR
 	r = get_prom(dev_addr, nic.base);
 	if (!r)
 		return -1;
@@ -703,6 +704,20 @@ int eth_init(bd_t *bd) {
 		 dev_addr[4], dev_addr[5]) ;
 	PRINTK("Set environment from HW MAC addr = \"%s\"\n", ethaddr);
 	setenv ("ethaddr", ethaddr);
+#else /* CONFIG_ETHADDR */
+#define STR(X) #X
+#define XSTR(X) STR(X)
+	strncpy(ethaddr, XSTR(CONFIG_ETHADDR), sizeof(ethaddr));
+
+	/* replace all colons by NULL characters */
+	for (r = 0; r < strlen(ethaddr); r++)
+		if (ethaddr[r] == ':')
+			ethaddr[r] = '\0';
+	
+	/* convert the string notation */
+	for (r = 0; r < 6; r++)
+		dev_addr[r] = simple_strtol(ethaddr + (r * 3), NULL, 16);
+#endif
 
 	nic.data = nic.base + DP_DATA;
 	nic.tx_buf1 = START_PG;

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Ben Warren]

Hi Daniel,

On Fri, Nov 28, 2008 at 8:16 AM, Daniel Mack <daniel@caiaq.de> wrote:

> Hi,
>
> the board I'm currently working on has an ASIX AX88796 NE2000 clone but
> no EEPROM attached to it. Hence, the get_prom() routine returns zeros
> only so the system won't work.
>
> This patch takes the MAC address given by CONFIG_ETHADDR and translates
> it to numeric values. This could probably go to some other, more generic
> place, but I didn't find any.
>

CONFIG_ETHADDR is bad and shouldn't be used unless you only ever plan on
building one board.  No new board ports will be accepted that define it.
What would be better here, although it's still not very good, would be to
have something like CONFIG_NE2000_NOPROM, and if defined, make a
'getenv("ethaddr")' call and program the hardware with the return value
(with proper error checking, of course).

regards,
Ben

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Daniel Mack]

Hi Ben,

On Fri, Nov 28, 2008 at 09:27:37PM -0800, Ben Warren wrote:
> > the board I'm currently working on has an ASIX AX88796 NE2000 clone but
> > no EEPROM attached to it. Hence, the get_prom() routine returns zeros
> > only so the system won't work.
> >
> > This patch takes the MAC address given by CONFIG_ETHADDR and translates
> > it to numeric values. This could probably go to some other, more generic
> > place, but I didn't find any.
> >
> 
> CONFIG_ETHADDR is bad and shouldn't be used unless you only ever plan on
> building one board.  No new board ports will be accepted that define it.

I agree it's a hack, but in order to boot the board from ethernet, there
is need for something, even if it's not perfect.

> What would be better here, although it's still not very good, would be to
> have something like CONFIG_NE2000_NOPROM, and if defined, make a
> 'getenv("ethaddr")' call and program the hardware with the return value
> (with proper error checking, of course).

I agree. See the patch below.

Thanks,
Daniel

This patch adds CONFIG_NE2000_NOPROM as configuration variable and reads
the 'ethaddr' setting from environment to set up the adapter.

Signed-off-by: Daniel Mack <daniel@caiaq.de>

diff --git a/drivers/net/ne2000_base.c b/drivers/net/ne2000_base.c
index f93f932..86c1380 100644
--- a/drivers/net/ne2000_base.c
+++ b/drivers/net/ne2000_base.c
@@ -669,7 +669,6 @@ void uboot_push_tx_done(int key, int val) {
 int eth_init(bd_t *bd) {
 	int r;
 	u8 dev_addr[6];
-	char ethaddr[20];
 
 	PRINTK("### eth_init\n");
 
@@ -693,16 +692,35 @@ int eth_init(bd_t *bd) {
 
 	nic.base = (u8 *) CONFIG_DRIVER_NE2000_BASE;
 
-	r = get_prom(dev_addr, nic.base);
-	if (!r)
-		return -1;
+#ifndef CONFIG_NE2000_NOPROM
+	{
+		char ethaddr[20];
+		r = get_prom(dev_addr, nic.base);
+		if (!r)
+			return -1;
+
+		sprintf (ethaddr, "%02X:%02X:%02X:%02X:%02X:%02X",
+			 dev_addr[0], dev_addr[1],
+			 dev_addr[2], dev_addr[3],
+			 dev_addr[4], dev_addr[5]) ;
+		PRINTK("Set environment from HW MAC addr = \"%s\"\n", ethaddr);
+		setenv ("ethaddr", ethaddr);
+	}
+#else /* CONFIG_NE2000_NOPROM */
+	{
+		char *s = getenv("ethaddr");
 
-	sprintf (ethaddr, "%02X:%02X:%02X:%02X:%02X:%02X",
-		 dev_addr[0], dev_addr[1],
-		 dev_addr[2], dev_addr[3],
-		 dev_addr[4], dev_addr[5]) ;
-	PRINTK("Set environment from HW MAC addr = \"%s\"\n", ethaddr);
-	setenv ("ethaddr", ethaddr);
+		if (!s) {
+			printf ("CONFIG_NE2000_NOPROM set but no "
+				"ethaddr given in environment.\n");
+			return -1;
+		}
+
+		/* convert the string notation */
+		for (r = 0; r < 6; r++)
+			dev_addr[r] = simple_strtol(s + (r * 3), NULL, 16);
+	}
+#endif
 
 	nic.data = nic.base + DP_DATA;
 	nic.tx_buf1 = START_PG;

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Daniel Mack]

This adds CONFIG_NE2000_NOPROM. If set, the ethernet MAC address is taken
from the environment variable 'ethaddr' and the NIC is configured
accordingly. Needed for boards that don't have an EEPROM to store this
setting permanently.

Signed-off-by: Daniel Mack <daniel@caiaq.de>

---
 ne2000_base.c |   38 ++++++++++++++++++++++++++++----------
  1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ne2000_base.c b/drivers/net/ne2000_base.c
index f93f932..86c1380 100644
--- a/drivers/net/ne2000_base.c
+++ b/drivers/net/ne2000_base.c
@@ -669,7 +669,6 @@ void uboot_push_tx_done(int key, int val) {
 int eth_init(bd_t *bd) {
 	int r;
 	u8 dev_addr[6];
-	char ethaddr[20];
 
 	PRINTK("### eth_init\n");
 
@@ -693,16 +692,35 @@ int eth_init(bd_t *bd) {
 
 	nic.base = (u8 *) CONFIG_DRIVER_NE2000_BASE;
 
-	r = get_prom(dev_addr, nic.base);
-	if (!r)
-		return -1;
+#ifndef CONFIG_NE2000_NOPROM
+	{
+		char ethaddr[20];
+		r = get_prom(dev_addr, nic.base);
+		if (!r)
+			return -1;
+
+		sprintf (ethaddr, "%02X:%02X:%02X:%02X:%02X:%02X",
+			 dev_addr[0], dev_addr[1],
+			 dev_addr[2], dev_addr[3],
+			 dev_addr[4], dev_addr[5]) ;
+		PRINTK("Set environment from HW MAC addr = \"%s\"\n", ethaddr);
+		setenv ("ethaddr", ethaddr);
+	}
+#else /* CONFIG_NE2000_NOPROM */
+	{
+		char *s = getenv("ethaddr");
 
-	sprintf (ethaddr, "%02X:%02X:%02X:%02X:%02X:%02X",
-		 dev_addr[0], dev_addr[1],
-		 dev_addr[2], dev_addr[3],
-		 dev_addr[4], dev_addr[5]) ;
-	PRINTK("Set environment from HW MAC addr = \"%s\"\n", ethaddr);
-	setenv ("ethaddr", ethaddr);
+		if (!s) {
+			printf ("CONFIG_NE2000_NOPROM set but no "
+				"ethaddr given in environment.\n");
+			return -1;
+		}
+
+		/* convert the string notation */
+		for (r = 0; r < 6; r++)
+			dev_addr[r] = simple_strtol(s + (r * 3), NULL, 16);
+	}
+#endif
 
 	nic.data = nic.base + DP_DATA;
 	nic.tx_buf1 = START_PG;

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Wolfgang Denk]

Dear Daniel Mack,

In message <20081203010154.GE2012@buzzloop.caiaq.de> you wrote:
> This adds CONFIG_NE2000_NOPROM. If set, the ethernet MAC address is taken
> from the environment variable 'ethaddr' and the NIC is configured
> accordingly. Needed for boards that don't have an EEPROM to store this
> setting permanently.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> 
> ---
>  ne2000_base.c |   38 ++++++++++++++++++++++++++++----------
>   1 file changed, 28 insertions(+), 10 deletions(-)

Why do we need a special CONFIG_ option? is it not possible to detect
(by software) that there is no EEPROM available, and auto-adjust?

Note that there is existing policy how to handle the  situation  that
we have both a MAC address stored in some other storeage (like EEPROM
on  the  NIC)  and  in  the  "ethaddr"  environment variable (see for
example drivers/net/cs8900.c). In the end, the code shall always only
rely on the U-Boot environment settings.

The system will test if MAC addresses are set in EEPROM and in the
envrionment; then it should behave as follows:

* If there is no MAC address in the EEPROM, then do nothing, i. e.
  rely on any previously existing envrionment settings.

* If there is a MAC address in the EEPROM, and ther eis no
  corresponding "ethaddr" environment variable defined, then
  initialize (setenv()) this variable with the value read from the
  EEPROM.

* If there is a MAC address in the EEPROM, and there is a value in
  the "ethaddr" environment variable, and both are identical, we use
  that value and everything is fine.

* If there is a MAC address in the EEPROM, and there is a value in
  the "ethaddr" environment variable, and both are different, we
  issue a warning "Warning: MAC addresses don't match: ... HW MAC
  address: ... "ethaddr" value: ..." and then use the value from the
  ENVIRONMENT.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There was no difference between  the  behavior  of  a  god  and  the
operations of pure chance..."   - Thomas Pynchon, _Gravity's Rainbow_

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Daniel Mack]

Hi Wolfgang,

On Sun, Dec 14, 2008 at 12:12:24PM +0100, Wolfgang Denk wrote:
> > This adds CONFIG_NE2000_NOPROM. If set, the ethernet MAC address is taken
> > from the environment variable 'ethaddr' and the NIC is configured
> > accordingly. Needed for boards that don't have an EEPROM to store this
> > setting permanently.
> > 
> > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > 
> > ---
> >  ne2000_base.c |   38 ++++++++++++++++++++++++++++----------
> >   1 file changed, 28 insertions(+), 10 deletions(-)
> 
> Why do we need a special CONFIG_ option? is it not possible to detect
> (by software) that there is no EEPROM available, and auto-adjust?

Hmm, what's bad about a special config variable? I agree that in
general, auto-probing is a good thing, but in U-Boot with its per-board
configuration file concept, I don't see much advantage. It is very
unlikely that a certain board is produced with and without an EEPROM.

> Note that there is existing policy how to handle the  situation  that
> we have both a MAC address stored in some other storeage (like EEPROM
> on  the  NIC)  and  in  the  "ethaddr"  environment variable (see for
> example drivers/net/cs8900.c). In the end, the code shall always only
> rely on the U-Boot environment settings.

I can send a new version of this patch following this, but I wonder
why such a logic has to be implemented in the ethernet drivers and did
not find a well-defined place in some kind of layer all drivers make use
of? Wouldn't it make more sense to put some effort in this direction?

Best regards,
Daniel

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Daniel Mack]

Picking up an acient thread,

On Sun, Dec 14, 2008 at 12:39:10PM +0100, Daniel Mack wrote:
> On Sun, Dec 14, 2008 at 12:12:24PM +0100, Wolfgang Denk wrote:
> > > This adds CONFIG_NE2000_NOPROM. If set, the ethernet MAC address is taken
> > > from the environment variable 'ethaddr' and the NIC is configured
> > > accordingly. Needed for boards that don't have an EEPROM to store this
> > > setting permanently.
> > > 
> > > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > > 
> > > ---
> > >  ne2000_base.c |   38 ++++++++++++++++++++++++++++----------
> > >   1 file changed, 28 insertions(+), 10 deletions(-)
> > 
> > Why do we need a special CONFIG_ option? is it not possible to detect
> > (by software) that there is no EEPROM available, and auto-adjust?
> 
> Hmm, what's bad about a special config variable? I agree that in
> general, auto-probing is a good thing, but in U-Boot with its per-board
> configuration file concept, I don't see much advantage. It is very
> unlikely that a certain board is produced with and without an EEPROM.
> 
> > Note that there is existing policy how to handle the  situation  that
> > we have both a MAC address stored in some other storeage (like EEPROM
> > on  the  NIC)  and  in  the  "ethaddr"  environment variable (see for
> > example drivers/net/cs8900.c). In the end, the code shall always only
> > rely on the U-Boot environment settings.
> 
> I can send a new version of this patch following this, but I wonder
> why such a logic has to be implemented in the ethernet drivers and did
> not find a well-defined place in some kind of layer all drivers make use
> of? Wouldn't it make more sense to put some effort in this direction?

Is there any reason why there was no reply to this anymore suddenly? Did
it sound like too much work for anyone to do it the right way? I'll get
back to this project soon and will need to address the same problems
agains like many weeks ago, and unfortunately, nothing happened to all
the issues I pointed out back then, not a single patch I sent in has been
taken.

Daniel

[U-Boot] [PATCH] ne2000: take MAC address from config if available

[Wolfgang Denk]

Dear Daniel,

In message <20090127130912.GA25722@buzzloop.caiaq.de> you wrote:
> 
> > I can send a new version of this patch following this, but I wonder
> > why such a logic has to be implemented in the ethernet drivers and did
> > not find a well-defined place in some kind of layer all drivers make use
> > of? Wouldn't it make more sense to put some effort in this direction?
> 
> Is there any reason why there was no reply to this anymore suddenly? Did
> it sound like too much work for anyone to do it the right way? I'll get

Well, let me answer with a quote from my signature list:

There are three ways to get something  done:  do  it  yourself,  hire
someone, or forbid your kids to do it.

> back to this project soon and will need to address the same problems
> agains like many weeks ago, and unfortunately, nothing happened to all
> the issues I pointed out back then, not a single patch I sent in has been
> taken.

You know the reasons why they were rejected. As for the "nothing
happened" part - see signature below :-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Business is like a wheelbarrow. Nothing ever happens until you  start
pushing.