Blackfin: bfin_mac: hook up new write_hwaddr function

Signed-off-by Mike Frysinger<[email protected]>

---
  drivers/net/bfin_mac.c |   28 +++++++++++++++-------------
  drivers/net/bfin_mac.h |    3 +--
  2 files changed, 16 insertions(+), 15 deletions(-)

drivers/net/bfin_mac.c +14 -0

index ec45b63..720e126 100644
@@ -106,6 +106,7 @@ int bfin_EMAC_initialize(bd_t *bis)
  	dev->halt = bfin_EMAC_halt;
  	dev->send = bfin_EMAC_send;
  	dev->recv = bfin_EMAC_recv;
+	dev->write_hwaddr = bfin_EMAC_setup_addr;
 
  	eth_register(dev);
 

@@ -303,6 +304,19 @@ static int bfin_miiphy_init(struct eth_device *dev, int *opmode)
  	return 0;
  }
 
+static int bfin_EMAC_setup_addr(struct eth_device *dev)
+{
+	*pEMAC_ADDRLO =
+		dev->enetaddr[0] |
+		dev->enetaddr[1]<<  8 |
+		dev->enetaddr[2]<<  16 |
+		dev->enetaddr[3]<<  24;
+	*pEMAC_ADDRHI =
+		dev->enetaddr[4] |
+		dev->enetaddr[5]<<  8;
+	return 0;
+}
+
  static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd)
  {
  	u32 opmode;

drivers/net/bfin_mac.c static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd) · line 335

Quoted 2 lines · Mike Frysinger

drivers/net/bfin_mac.c +1 -1

-	bfin_EMAC_setup_addr(dev->enetaddr);
+	bfin_EMAC_setup_addr(dev);

Are you sure you still want to program it on every init() call? There's nothing wrong with that, BTW...

Quoted 8 lines · On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:

drivers/net/bfin_mac.c static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd) · line 335

Quoted 3 lines · Mike Frysinger

drivers/net/bfin_mac.c +1 -1

-	bfin_EMAC_setup_addr(dev->enetaddr);
+	bfin_EMAC_setup_addr(dev);

Are you sure you still want to program it on every init() call? There's nothing wrong with that, BTW...

that's the correct documented behavior ... otherwise, there's no way of syncing the env to the hardware. this is init(), not initialize(). -mike

Quoted 8 lines · Mike Frysinger

Quoted 6 lines · On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:

drivers/net/bfin_mac.c static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd) · line 335

Quoted 2 lines · Mike Frysinger

drivers/net/bfin_mac.c +1 -1

-	bfin_EMAC_setup_addr(dev->enetaddr);
+	bfin_EMAC_setup_addr(dev);

Are you sure you still want to program it on every init() call? There's nothing wrong with that, BTW...

Quoted 2 lines

that's the correct documented behavior ... otherwise, there's no way of syncing the env to the hardware. this is init(), not initialize().

Hardware will already be sync'd to the env, since write_hwaddr() now automatically gets called after initialize(). The only thing you gain from having this call in init() is to program hardware on interfaces that were skipped due to the 'ethmacskip' environment variable being set.

Quoted 12 lines · On Tuesday 27 April 2010 14:46:34 Ben Warren wrote:

Quoted 10 lines · Mike Frysinger

Quoted 8 lines · On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:

drivers/net/bfin_mac.c static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd) · line 335

Quoted 3 lines · Mike Frysinger

drivers/net/bfin_mac.c +1 -1

-	bfin_EMAC_setup_addr(dev->enetaddr);
+	bfin_EMAC_setup_addr(dev);

Are you sure you still want to program it on every init() call? There's nothing wrong with that, BTW...

Quoted 8 lines

Quoted 3 lines

that's the correct documented behavior ... otherwise, there's no way of syncing the env to the hardware. this is init(), not initialize().

Hardware will already be sync'd to the env, since write_hwaddr() now automatically gets called after initialize(). The only thing you gain from having this call in init() is to program hardware on interfaces that were skipped due to the 'ethmacskip' environment variable being set.

your v4 patch writes the env to the hardware in the initialize() step, not the init() step. if someone changes ethaddr on the fly, the common net funcs sync env->eth_device, but it does not call the new write_hwaddr() func. perhaps you intended to make that change, but it currently doesnt do it. -mike

Quoted 5 lines · Mike Frysinger

your v4 patch writes the env to the hardware in the initialize() step, not the init() step. if someone changes ethaddr on the fly, the common net funcs sync env->eth_device, but it does not call the new write_hwaddr() func. perhaps you intended to make that change, but it currently doesnt do it.

I see. I think this logic could use some rework. It seems to me that we should only worry about sync'ing the device that we're trying to use. If its address in the environment is different from the one in the device struct, we could sync and program, instead of doing all of them. Does that make sense?

if the new direction is to have the OS booted with the current correct MAC address programmed regardless of any net funcs being called, then the current logic in eth_initialize() will probably need to be replicated back into cmd_nvedit (see commit 56b555a644). then the logic in eth_init() here can be dropped, and drivers themselves can lose the MAC programming in their init() funcs.

however, considering the number of drivers and thrashing this policy is causing, and the intent for this to be a stop gap measure, i'm not sure we should change the init() func in drivers to stop programming the MAC address. otherwise we're going to have to do this same thing yet again but in the reverse direction.

Quoted 6 lines

if the new direction is to have the OS booted with the current correct MAC address programmed regardless of any net funcs being called, then the current logic in eth_initialize() will probably need to be replicated back into cmd_nvedit (see commit 56b555a644). then the logic in eth_init() here can be dropped, and drivers themselves can lose the MAC programming in their init() funcs.

For the direction we have taken, I consider this to be a completion of the path. Actually I did not realize that we had such a mac-programming in "setenv" at all and that Mike explicitely took it out, but I realized that we certainly would need such a thing. I considered it to be much easier hoewever once we have such a method of the netdevice.

Quoted 5 lines

however, considering the number of drivers and thrashing this policy is causing, and the intent for this to be a stop gap measure, i'm not sure we should change the init() func in drivers to stop programming the MAC address. otherwise we're going to have to do this same thing yet again but in the reverse direction.

We are aiming at a level of consitency which we never had before, so I expect some changes. They are quite small however and the amount of ongoing patches to adopt the new policy encourage me to be optimistic here ;)