[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Mike Rapoport]

Signed-off-by: Mike Rapoport <mike@compulab.co.il>

  drivers/dm9000x.c |   14 ++++++++++++++
  include/net.h     |   39 +++++++++++++++++++++++++++++++++++++++
  2 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/drivers/dm9000x.c b/drivers/dm9000x.c
index 6877076..6486699 100644
--- a/drivers/dm9000x.c
+++ b/drivers/dm9000x.c
@@ -302,6 +302,20 @@ eth_init(bd_t * bd)
  	/* Set Node address */
  	for (i = 0; i < 6; i++)
  		((u16 *) bd->bi_enetaddr)[i] = read_srom_word(i);
+
+	if (!is_valid_ether_addr(bd->bi_enetaddr)) {
+		/* try reading from environment */
+		u8 i;
+		char *s, *e;
+		s = getenv ("ethaddr");
+		for (i = 0; i < 6; ++i) {
+			bd->bi_enetaddr[i] = s ?
+				simple_strtoul (s, &e, 16) : 0;
+			if (s)
+				s = (*e) ? e + 1 : e;
+		}
+	}
+
  	printf("MAC: %02x:%02x:%02x:%02x:%02x:%02x\n", bd->bi_enetaddr[0],
  	       bd->bi_enetaddr[1], bd->bi_enetaddr[2], bd->bi_enetaddr[3],
  	       bd->bi_enetaddr[4], bd->bi_enetaddr[5]);
diff --git a/include/net.h b/include/net.h
index 461e038..34967e4 100644
--- a/include/net.h
+++ b/include/net.h
@@ -435,6 +435,45 @@ static inline void NetCopyLong(ulong *to, ulong *from)
  	memcpy((void*)to, (void*)from, sizeof(ulong));
  }

+/**
+ * is_zero_ether_addr - Determine if give Ethernet address is all zeros.
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return true if the address is all zeroes.
+ */
+static inline int is_zero_ether_addr(const u8 *addr)
+{
+	return !(addr[0] | addr[1] | addr[2] | addr[3] | addr[4] | addr[5]);
+}
+
+/**
+ * is_multicast_ether_addr - Determine if the Ethernet address is a multicast.
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return true if the address is a multicast address.
+ * By definition the broadcast address is also a multicast address.
+ */
+static inline int is_multicast_ether_addr(const u8 *addr)
+{
+	return (0x01 & addr[0]);
+}
+
+/**
+ * is_valid_ether_addr - Determine if the given Ethernet address is valid
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
+ * a multicast address, and is not FF:FF:FF:FF:FF:FF.
+ *
+ * Return true if the address is valid.
+ */
+static inline int is_valid_ether_addr(const u8 *addr)
+{
+	/* FF:FF:FF:FF:FF:FF is a multicast address so we don't need to
+	 * explicitly check for it here. */
+	return !is_multicast_ether_addr(addr) && !is_zero_ether_addr(addr);
+}
+
  /* Convert an IP address to a string */
  extern void	ip_to_string (IPaddr_t x, char *s);

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Wolfgang Denk]

In message <46B84A47.3030908@compulab.co.il> you wrote:
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>

Patch does not apply to current u-boot (neither -testing nor
mainline). Please fix and resubmit:

error: patch failed: drivers/dm9000x.c:302
error: drivers/dm9000x.c: patch does not apply
error: patch failed: include/net.h:435
error: include/net.h: patch does not apply
Using index info to reconstruct a base tree...
error: patch failed: drivers/dm9000x.c:302
error: drivers/dm9000x.c: patch does not apply
error: patch failed: include/net.h:435
error: include/net.h: patch does not apply
Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Cannot fall back to three-way merge.
Patch failed at 0001.


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
Computers are not intelligent.  They only think they are.

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Wolfgang Denk]

In message <46B84A47.3030908@compulab.co.il> you wrote:
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>

Ditto:

error: patch failed: include/asm-arm/arch-pxa/pxa-regs.h:150
error: include/asm-arm/arch-pxa/pxa-regs.h: patch does not apply
Using index info to reconstruct a base tree...
error: patch failed: include/asm-arm/arch-pxa/pxa-regs.h:150
error: include/asm-arm/arch-pxa/pxa-regs.h: patch does not apply
Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Cannot fall back to three-way merge.
Patch failed at 0001.


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
It became apparent that one reason why the Ice Giants were  known  as
the  Ice  Giants  was  because they were, well, giants. The other was
that they were made of ice.              -Terry Pratchett, _Sourcery_

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Mike Rapoport]

Wolfgang Denk wrote:
> In message <46B84A47.3030908@compulab.co.il> you wrote:
>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> 
> Patch does not apply to current u-boot (neither -testing nor
> mainline). Please fix and resubmit:
> 
> error: patch failed: drivers/dm9000x.c:302
> error: drivers/dm9000x.c: patch does not apply
> error: patch failed: include/net.h:435
> error: include/net.h: patch does not apply
> Using index info to reconstruct a base tree...
> error: patch failed: drivers/dm9000x.c:302
> error: drivers/dm9000x.c: patch does not apply
> error: patch failed: include/net.h:435
> error: include/net.h: patch does not apply
> Did you hand edit your patch?
> It does not apply to blobs recorded in its index.
> Cannot fall back to three-way merge.
> Patch failed at 0001.

I've just cloned the mainline HEAD, and created the patch from there.
This time the patch attached to avoid Thunderbird intervention.

> Best regards,
> 
> Wolfgang Denk
> 


-- 
Sincerely yours,
Mike.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dm9000.patch
Type: text/x-patch
Size: 2425 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070807/ab6da7e9/attachment.bin 

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Ben Warren]

Hi Mike,

Mike Rapoport wrote:
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>
>   drivers/dm9000x.c |   14 ++++++++++++++
>   include/net.h     |   39 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 53 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/dm9000x.c b/drivers/dm9000x.c
> index 6877076..6486699 100644
> --- a/drivers/dm9000x.c
> +++ b/drivers/dm9000x.c
> @@ -302,6 +302,20 @@ eth_init(bd_t * bd)
>   	/* Set Node address */
>   	for (i = 0; i < 6; i++)
>   		((u16 *) bd->bi_enetaddr)[i] = read_srom_word(i);
> +
> +	if (!is_valid_ether_addr(bd->bi_enetaddr)) {
> +		/* try reading from environment */
> +		u8 i;
> +		char *s, *e;
> +		s = getenv ("ethaddr");
>   
Could this ever be other than the first Ethernet controller on a board? 
If so, 'ethaddr' won't cut it.
> +		for (i = 0; i < 6; ++i) {
> +			bd->bi_enetaddr[i] = s ?
> +				simple_strtoul (s, &e, 16) : 0;
> +			if (s)
> +				s = (*e) ? e + 1 : e;
> +		}
> +	}
>   
While this is probably fine, it looks scary to me. My little brain is 
going to need a few more passes...
> +
>   	printf("MAC: %02x:%02x:%02x:%02x:%02x:%02x\n", bd->bi_enetaddr[0],
>   	       bd->bi_enetaddr[1], bd->bi_enetaddr[2], bd->bi_enetaddr[3],
>   	       bd->bi_enetaddr[4], bd->bi_enetaddr[5]);
> diff --git a/include/net.h b/include/net.h
> index 461e038..34967e4 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -435,6 +435,45 @@ static inline void NetCopyLong(ulong *to, ulong *from)
>   	memcpy((void*)to, (void*)from, sizeof(ulong));
>   }
>
> +/**
> + * is_zero_ether_addr - Determine if give Ethernet address is all zeros.
> + * @addr: Pointer to a six-byte array containing the Ethernet address
> + *
> + * Return true if the address is all zeroes.
> + */
> +static inline int is_zero_ether_addr(const u8 *addr)
> +{
> +	return !(addr[0] | addr[1] | addr[2] | addr[3] | addr[4] | addr[5]);
> +}
> +
> +/**
> + * is_multicast_ether_addr - Determine if the Ethernet address is a multicast.
> + * @addr: Pointer to a six-byte array containing the Ethernet address
> + *
> + * Return true if the address is a multicast address.
> + * By definition the broadcast address is also a multicast address.
> + */
> +static inline int is_multicast_ether_addr(const u8 *addr)
> +{
> +	return (0x01 & addr[0]);
> +}
> +
> +/**
> + * is_valid_ether_addr - Determine if the given Ethernet address is valid
> + * @addr: Pointer to a six-byte array containing the Ethernet address
> + *
> + * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
> + * a multicast address, and is not FF:FF:FF:FF:FF:FF.
> + *
> + * Return true if the address is valid.
> + */
> +static inline int is_valid_ether_addr(const u8 *addr)
> +{
>   
Please choose a better name for this function. While multicast addresses 
are poor choices for source, they're perfectly fine for destination.

thanks,
Ben

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Mike Rapoport]

Ben Warren wrote:
> Hi Mike,
> 
> Mike Rapoport wrote:

>> +		s = getenv ("ethaddr");
>>   
> Could this ever be other than the first Ethernet controller on a board? 
> If so, 'ethaddr' won't cut it.

If a board has more than one DM9000, the driver in its current state does not 
support it anyway.

>> +		for (i = 0; i < 6; ++i) {
>> +	}
>>   
> While this is probably fine, it looks scary to me. My little brain is 
> going to need a few more passes...
>> +
>>   	printf("MAC: %02x:%02x:%02x:%02x:%02x:%02x\n", bd->bi_enetaddr[0],
>>   	       bd->bi_enetaddr[1], bd->bi_enetaddr[2], bd->bi_enetaddr[3],
>>   	       bd->bi_enetaddr[4], bd->bi_enetaddr[5]);

>> +/**
>> + * is_valid_ether_addr - Determine if the given Ethernet address is valid
>> + * @addr: Pointer to a six-byte array containing the Ethernet address
>> + *
>> + * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
>> + * a multicast address, and is not FF:FF:FF:FF:FF:FF.
>> + *
>> + * Return true if the address is valid.
>> + */
>> +static inline int is_valid_ether_addr(const u8 *addr)
>> +{
>>   
> Please choose a better name for this function. While multicast addresses 
> are poor choices for source, they're perfectly fine for destination.

What about 'is_valid_ether_src_addr'?

> thanks,
> Ben
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
> 


-- 
Sincerely yours,
Mike.

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Wolfgang Denk]

In message <46B9563B.1010807@compulab.co.il> you wrote:
>
> > Could this ever be other than the first Ethernet controller on a board? 
> > If so, 'ethaddr' won't cut it.
> 
> If a board has more than one DM9000, the driver in its current state does not 
> support it anyway.

Assume the board has an XXX Ethernet controller as eth0, and a DM9000
as eth1 ?

> > Please choose a better name for this function. While multicast addresses 
> > are poor choices for source, they're perfectly fine for destination.
> 
> What about 'is_valid_ether_src_addr'?

That's a very long name which is difficult to read, difficult to type,
and makes for very long source code lines...

And I will probably have to look up the definition of  this  function
each  time  I read the code. You use this function exactly once, so I
suggest to NOT define it at all and write down what it does:

	if (!is_multicast_ether_addr(addr) && !is_zero_ether_addr(addr)) ...

This is IMHO much better to read and to understand.

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
The more complex the mind, the greater the need for the simplicity of
play.
	-- Kirk, "Shore Leave", stardate 3025.8

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Mike Rapoport]

Wolfgang Denk wrote:
> In message <46B9563B.1010807@compulab.co.il> you wrote:
>>> Could this ever be other than the first Ethernet controller on a board? 
>>> If so, 'ethaddr' won't cut it.
>> If a board has more than one DM9000, the driver in its current state does not 
>> support it anyway.
> 
> Assume the board has an XXX Ethernet controller as eth0, and a DM9000
> as eth1 ?

The only solution I can think of in this case is adding some
#define CONFIG_DM9000_ETHER_INDEX
in the board config
and approptiate '#if' clauses in the driver.
If it's Ok with you I'll do it and resend the patch.
Still, the configuration you mention seems very theoretical for me and I can 
hardly imagine someone designing a board with such configuration.

>>> Please choose a better name for this function. While multicast addresses 
>>> are poor choices for source, they're perfectly fine for destination.
>> What about 'is_valid_ether_src_addr'?
> 
> That's a very long name which is difficult to read, difficult to type,
> and makes for very long source code lines...
> 
> And I will probably have to look up the definition of  this  function
> each  time  I read the code. You use this function exactly once, so I
> suggest to NOT define it at all and write down what it does:
> 
> 	if (!is_multicast_ether_addr(addr) && !is_zero_ether_addr(addr)) ...
> 
> This is IMHO much better to read and to understand.

I agree and I'll fix it.

> Best regards,
> 
> Wolfgang Denk
> 


-- 
Sincerely yours,
Mike.

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Ben Warren]

Mike Rapoport wrote:
> Wolfgang Denk wrote:
>> In message <46B9563B.1010807@compulab.co.il> you wrote:
>>>> Could this ever be other than the first Ethernet controller on a 
>>>> board? If so, 'ethaddr' won't cut it.
>>> If a board has more than one DM9000, the driver in its current state 
>>> does not support it anyway.
>>
>> Assume the board has an XXX Ethernet controller as eth0, and a DM9000
>> as eth1 ?
>
> The only solution I can think of in this case is adding some
> #define CONFIG_DM9000_ETHER_INDEX
> in the board config
> and approptiate '#if' clauses in the driver.
> If it's Ok with you I'll do it and resend the patch.
> Still, the configuration you mention seems very theoretical for me and 
> I can hardly imagine someone designing a board with such configuration.
>
This may not be necessary, since this controller isn't CONFIG_NET_MULTI 
compatible, although it should be.  Yeah, I know you pointed this out 
already.  Sorry...

This type of configuration is not at all theoretical.  While it's hardly 
a shining example of hardware design, we have a board that uses both 
TSECs on an MPC8349 for high speed traffic, but we needed another 10/100 
interface for control purposes.  We put a cheap MAC/PHY chip on the 
local bus to meet this need.  It wasn't a DM9000, but it could have been.

regards,
Ben

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Wolfgang Denk]

In message <46B88FDD.4090708@compulab.co.il> you wrote:
>
> I've just cloned the mainline HEAD, and created the patch from there.
> This time the patch attached to avoid Thunderbird intervention.

Applied, thanks.

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
...though his invention worked superbly -- his theory was a crock  of
sewage from beginning to end.         - Vernor Vinge, "The Peace War"

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Wolfgang Denk]

In message <20070809210451.E4756353428@atlas.denx.de> you wrote:
> In message <46B88FDD.4090708@compulab.co.il> you wrote:
> >
> > I've just cloned the mainline HEAD, and created the patch from there.
> > This time the patch attached to avoid Thunderbird intervention.
> 
> Applied, thanks.

Oops. There was later discussion on this, and probably a better patch
coming?

Undone. Sorry for the confusion.

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
Lispers are among  the  best  grads  of  the  Sweep-It-Under-Someone-
Else's-Carpet  School of Simulated Simplicity. [Was that sufficiently
incendiary? :-)]  - Larry Wall in <1992Jan10.201804.11926@netlabs.com

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Mike Rapoport]

Wolfgang Denk wrote:
> In message <20070809210451.E4756353428@atlas.denx.de> you wrote:
>> In message <46B88FDD.4090708@compulab.co.il> you wrote:
>>> I've just cloned the mainline HEAD, and created the patch from there.
>>> This time the patch attached to avoid Thunderbird intervention.
>> Applied, thanks.
> 
> Oops. There was later discussion on this, and probably a better patch
> coming?

Following the comments of the discussion, below is the updated patch.

> Undone. Sorry for the confusion.
> 
> Best regards,
> 
> Wolfgang Denk
> 

Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
  drivers/dm9000x.c |   15 +++++++++++++++
  include/net.h     |   23 +++++++++++++++++++++++
  2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/dm9000x.c b/drivers/dm9000x.c
index 6877076..78acb09 100644
--- a/drivers/dm9000x.c
+++ b/drivers/dm9000x.c
@@ -302,6 +302,21 @@ eth_init(bd_t * bd)
  	/* Set Node address */
  	for (i = 0; i < 6; i++)
  		((u16 *) bd->bi_enetaddr)[i] = read_srom_word(i);
+
+	if (!is_zero_ether_addr(bd->bi_enetaddr) &&
+	    !is_mutlicast_ether_addr(bd->bi_enetaddr)) {
+		/* try reading from environment */
+		u8 i;
+		char *s, *e;
+		s = getenv ("ethaddr");
+		for (i = 0; i < 6; ++i) {
+			bd->bi_enetaddr[i] = s ?
+				simple_strtoul (s, &e, 16) : 0;
+			if (s)
+				s = (*e) ? e + 1 : e;
+		}
+	}
+
  	printf("MAC: %02x:%02x:%02x:%02x:%02x:%02x\n", bd->bi_enetaddr[0],
  	       bd->bi_enetaddr[1], bd->bi_enetaddr[2], bd->bi_enetaddr[3],
  	       bd->bi_enetaddr[4], bd->bi_enetaddr[5]);
diff --git a/include/net.h b/include/net.h
index 9671948..aa58e33 100644
--- a/include/net.h
+++ b/include/net.h
@@ -435,6 +435,29 @@ static inline void NetCopyLong(ulong *to, ulong *from)
  	memcpy((void*)to, (void*)from, sizeof(ulong));
  }

+/**
+ * is_zero_ether_addr - Determine if give Ethernet address is all zeros.
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return true if the address is all zeroes.
+ */
+static inline int is_zero_ether_addr(const u8 *addr)
+{
+	return !(addr[0] | addr[1] | addr[2] | addr[3] | addr[4] | addr[5]);
+}
+
+/**
+ * is_multicast_ether_addr - Determine if the Ethernet address is a multicast.
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return true if the address is a multicast address.
+ * By definition the broadcast address is also a multicast address.
+ */
+static inline int is_multicast_ether_addr(const u8 *addr)
+{
+	return (0x01 & addr[0]);
+}
+
  /* Convert an IP address to a string */
  extern void	ip_to_string (IPaddr_t x, char *s);

-- 

[U-Boot-Users] [PATCH] Add ability to take MAC address from the environment to DM9000 driver

[Ben Warren]

Wolfgang/Mike,

Mike Rapoport wrote:
> Wolfgang Denk wrote:
>   
>> In message <20070809210451.E4756353428@atlas.denx.de> you wrote:
>>     
>>> In message <46B88FDD.4090708@compulab.co.il> you wrote:
I'm currently pulling in a few net-related patches and will include this 
one. Expect a pull request some time today or early tomorrow.

regards,
Ben