Patchwork [U-Boot,2/9] mvgbe: Support preserving the existing MAC address

login
register
mail settings
Submitter Michael Spang
Date March 17, 2011, 7:46 p.m.
Message ID <1300391223-11879-3-git-send-email-mspang@csclub.uwaterloo.ca>
Download mbox | patch
Permalink /patch/87418/
State Changes Requested
Headers show

Comments

Michael Spang - March 17, 2011, 7:46 p.m.
The MVGBE driver either gets the MAC from the environment, or invents
one. This allows the driver to leave the existing address alone in
case it is initialized before U-Boot starts.

Signed-off-by: Michael Spang <mspang@csclub.uwaterloo.ca>
---
 drivers/net/mvgbe.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
Wolfgang Denk - April 24, 2011, 11:50 p.m.
Dear Michael Spang,

In message <1300391223-11879-3-git-send-email-mspang@csclub.uwaterloo.ca> you wrote:
> The MVGBE driver either gets the MAC from the environment, or invents
> one. This allows the driver to leave the existing address alone in
> case it is initialized before U-Boot starts.

Who or what would be doing that?

>  		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
> +#if defined(CONFIG_PRESERVE_LOCAL_MAC)
> +			port_uc_addr_get(dmvgbe->regs, dmvgbe->dev.enetaddr);
> +#else

For consistency, should this not be 

	port_uc_addr_get(dmvgbe->regs, dev->enetaddr);

?


>  			/* Generate Private MAC addr if not set */
>  			dev->enetaddr[0] = 0x02;
>  			dev->enetaddr[1] = 0x50;
> @@ -734,6 +753,7 @@ error1:
>  			dev->enetaddr[4] = get_random_hex();
>  			dev->enetaddr[5] = get_random_hex();
>  #endif
> +#endif
>  			eth_setenv_enetaddr(s, dev->enetaddr);
>  		}

And please add documentation for the new CONFIG_PRESERVE_LOCAL_MAC to
the README.

Best regards,

Wolfgang Denk
Tabi Timur-B04825 - April 25, 2011, 11:37 a.m.
On Sun, Apr 24, 2011 at 6:50 PM, Wolfgang Denk <wd@denx.de> wrote:

> And please add documentation for the new CONFIG_PRESERVE_LOCAL_MAC to
> the README.

We have something similar already on Freescale parts, but it does
sometimes cause problems.  If the environment ethaddr is already set,
it is left alone.  Otherwise, the MAC address is read from EEPROM.

So we could use CONFIG_PRESERVE_LOCAL_MAC to alter this behavior (i.e.
always use the EEPROM).  However, the current default is already
CONFIG_PRESERVE_LOCAL_MAC, so can we change this to something like
CONFIG_OVERRIDE_LOCAL_MAC?
Michael Spang - April 26, 2011, 4:20 a.m.
On Sun, Apr 24, 2011 at 7:50 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Michael Spang,
>
> In message <1300391223-11879-3-git-send-email-mspang@csclub.uwaterloo.ca> you wrote:
>> The MVGBE driver either gets the MAC from the environment, or invents
>> one. This allows the driver to leave the existing address alone in
>> case it is initialized before U-Boot starts.
>
> Who or what would be doing that?

The manufacturer's bootloader runs first, and cannot easily be
replaced. So U-Boot is loaded second, after the MAC is already set.

>
>>               while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
>> +#if defined(CONFIG_PRESERVE_LOCAL_MAC)
>> +                     port_uc_addr_get(dmvgbe->regs, dmvgbe->dev.enetaddr);
>> +#else
>
> For consistency, should this not be
>
>        port_uc_addr_get(dmvgbe->regs, dev->enetaddr);
>
> ?

Yes.

>
>
>>                       /* Generate Private MAC addr if not set */
>>                       dev->enetaddr[0] = 0x02;
>>                       dev->enetaddr[1] = 0x50;
>> @@ -734,6 +753,7 @@ error1:
>>                       dev->enetaddr[4] = get_random_hex();
>>                       dev->enetaddr[5] = get_random_hex();
>>  #endif
>> +#endif
>>                       eth_setenv_enetaddr(s, dev->enetaddr);
>>               }
>
> And please add documentation for the new CONFIG_PRESERVE_LOCAL_MAC to
> the README.

Ok I'll add:

               CONFIG_PRESERVE_LOCAL_MAC

               If no MAC address is set in the environment, then
               preserve the ethernet interface's current MAC address.
               Used if the MAC is configured before U-Boot is loaded.

Thanks,
Michael
Michael Spang - April 26, 2011, 4:23 a.m.
On Mon, Apr 25, 2011 at 7:37 AM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Sun, Apr 24, 2011 at 6:50 PM, Wolfgang Denk <wd@denx.de> wrote:
>
>> And please add documentation for the new CONFIG_PRESERVE_LOCAL_MAC to
>> the README.
>
> We have something similar already on Freescale parts, but it does
> sometimes cause problems.  If the environment ethaddr is already set,
> it is left alone.  Otherwise, the MAC address is read from EEPROM.
>
> So we could use CONFIG_PRESERVE_LOCAL_MAC to alter this behavior (i.e.
> always use the EEPROM).  However, the current default is already
> CONFIG_PRESERVE_LOCAL_MAC, so can we change this to something like
> CONFIG_OVERRIDE_LOCAL_MAC?

I don't think the option you want is the same. If there's a MAC in the
environment, I want to use it. Otherwise, I want U-Boot to leave the
MAC alone because the manufacturer assigned MAC was written to the
interface's registers before U-Boot started.

Michael
Mike Frysinger - April 30, 2011, 4:17 a.m.
On Tuesday, April 26, 2011 00:23:47 Michael Spang wrote:
> On Mon, Apr 25, 2011 at 7:37 AM, Tabi Timur-B04825 wrote:
> > On Sun, Apr 24, 2011 at 6:50 PM, Wolfgang Denk wrote:
> >> And please add documentation for the new CONFIG_PRESERVE_LOCAL_MAC to
> >> the README.
> > 
> > We have something similar already on Freescale parts, but it does
> > sometimes cause problems.  If the environment ethaddr is already set,
> > it is left alone.  Otherwise, the MAC address is read from EEPROM.
> > 
> > So we could use CONFIG_PRESERVE_LOCAL_MAC to alter this behavior (i.e.
> > always use the EEPROM).  However, the current default is already
> > CONFIG_PRESERVE_LOCAL_MAC, so can we change this to something like
> > CONFIG_OVERRIDE_LOCAL_MAC?
> 
> I don't think the option you want is the same. If there's a MAC in the
> environment, I want to use it. Otherwise, I want U-Boot to leave the
> MAC alone because the manufacturer assigned MAC was written to the
> interface's registers before U-Boot started.

so implement this in your board file in misc_init_r or board_eth_init.  have 
the code do something like:
	uchar enetaddr[6];
	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
		/* ... read current MAC out of the driver's registers ... */
		eth_setenv_enetaddr("ethaddr", enetaddr);
	}

then you dont need ugly config hacks in random drivers
-mike
Tabi Timur-B04825 - April 30, 2011, 2:34 p.m.
Mike Frysinger wrote:
> so implement this in your board file in misc_init_r or board_eth_init.  have
> the code do something like:
> 	uchar enetaddr[6];
> 	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
> 		/* ... read current MAC out of the driver's registers ... */
> 		eth_setenv_enetaddr("ethaddr", enetaddr);
> 	}
>
> then you dont need ugly config hacks in random drivers

This is a feature that could be applied to the e1000 drivers.  The current situation is a mess.  Some drivers ignore the environment, some of them always use it.  This should probably be standardized.
Mike Frysinger - May 1, 2011, 5:40 a.m.
On Sat, Apr 30, 2011 at 10:34, Tabi Timur-B04825 wrote:
> Mike Frysinger wrote:
>> so implement this in your board file in misc_init_r or board_eth_init.  have
>> the code do something like:
>>       uchar enetaddr[6];
>>       if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
>>               /* ... read current MAC out of the driver's registers ... */
>>               eth_setenv_enetaddr("ethaddr", enetaddr);
>>       }
>>
>> then you dont need ugly config hacks in random drivers
>
> This is a feature that could be applied to the e1000 drivers.  The current situation is a mess.  Some drivers ignore the environment, some of them always use it.  This should probably be standardized.

it is standardized already.  no driver should be touching the
environment.  it should only ever use its own eth_device->enetaddr.
the common eth code already takes care of syncing the env and that
member.

also, please fix your e-mailer to properly wrap long lines.
-mike

Patch

diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
index c701f43..bab55b3 100644
--- a/drivers/net/mvgbe.c
+++ b/drivers/net/mvgbe.c
@@ -380,6 +380,22 @@  static void port_uc_addr_set(struct mvgbe_registers *regs, u8 * p_addr)
 }
 
 /*
+ * port_uc_addr_get - This function gets the port unicast address.
+ */
+static void port_uc_addr_get(struct mvgbe_registers *regs, u8 * p_addr)
+{
+	u32 mac_l = MVGBE_REG_RD(regs->macal);
+	u32 mac_h = MVGBE_REG_RD(regs->macah);
+
+	p_addr[0] = (mac_h >> 24);
+	p_addr[1] = (mac_h >> 16);
+	p_addr[2] = (mac_h >> 8);
+	p_addr[3] = (mac_h >> 0);
+	p_addr[4] = (mac_l >> 8);
+	p_addr[5] = (mac_l >> 0);
+}
+
+/*
  * mvgbe_init_rx_desc_ring - Curve a Rx chain desc list and buffer in memory.
  */
 static void mvgbe_init_rx_desc_ring(struct mvgbe_device *dmvgbe)
@@ -719,6 +735,9 @@  error1:
 		}
 
 		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+#if defined(CONFIG_PRESERVE_LOCAL_MAC)
+			port_uc_addr_get(dmvgbe->regs, dmvgbe->dev.enetaddr);
+#else
 			/* Generate Private MAC addr if not set */
 			dev->enetaddr[0] = 0x02;
 			dev->enetaddr[1] = 0x50;
@@ -734,6 +753,7 @@  error1:
 			dev->enetaddr[4] = get_random_hex();
 			dev->enetaddr[5] = get_random_hex();
 #endif
+#endif
 			eth_setenv_enetaddr(s, dev->enetaddr);
 		}