[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Mike Frysinger]

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   28 +++++++++++++++-------------
 drivers/net/bfin_mac.h |    3 +--
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index ec45b63..720e126 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -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;
@@ -318,7 +332,7 @@ static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd)
 		return -1;
 
 	/* Initialize EMAC address */
-	bfin_EMAC_setup_addr(dev->enetaddr);
+	bfin_EMAC_setup_addr(dev);
 
 	/* Initialize TX and RX buffer */
 	for (i = 0; i < PKTBUFSRX; i++) {
@@ -376,18 +390,6 @@ static void bfin_EMAC_halt(struct eth_device *dev)
 
 }
 
-void bfin_EMAC_setup_addr(uchar *enetaddr)
-{
-	*pEMAC_ADDRLO =
-		enetaddr[0] |
-		enetaddr[1] << 8 |
-		enetaddr[2] << 16 |
-		enetaddr[3] << 24;
-	*pEMAC_ADDRHI =
-		enetaddr[4] |
-		enetaddr[5] << 8;
-}
-
 ADI_ETHER_BUFFER *SetupRxBuffer(int no)
 {
 	ADI_ETHER_FRAME_BUFFER *frmbuf;
diff --git a/drivers/net/bfin_mac.h b/drivers/net/bfin_mac.h
index 8f467a3..c731c17 100644
--- a/drivers/net/bfin_mac.h
+++ b/drivers/net/bfin_mac.h
@@ -60,7 +60,6 @@ static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd);
 static void bfin_EMAC_halt(struct eth_device *dev);
 static int bfin_EMAC_send(struct eth_device *dev, volatile void *packet, int length);
 static int bfin_EMAC_recv(struct eth_device *dev);
-
-void bfin_EMAC_setup_addr(uchar *enetaddr);
+static int bfin_EMAC_setup_addr(struct eth_device *dev);
 
 #endif
-- 
1.7.0.4

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Ben Warren]

Hi Mike,

On 4/27/2010 11:15 AM, Mike Frysinger wrote:
> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
> ---
>   drivers/net/bfin_mac.c |   28 +++++++++++++++-------------
>   drivers/net/bfin_mac.h |    3 +--
>   2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
> index ec45b63..720e126 100644
> --- a/drivers/net/bfin_mac.c
> +++ b/drivers/net/bfin_mac.c
> @@ -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;
> @@ -318,7 +332,7 @@ static int bfin_EMAC_init(struct eth_device *dev, bd_t *bd)
>   		return -1;
>
>   	/* Initialize EMAC address */
> -	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...

regards,
Ben

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Mike Frysinger]

On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:
> On 4/27/2010 11:15 AM, Mike Frysinger wrote:
> > -	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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100427/e6381e33/attachment.pgp 

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Ben Warren]

On 4/27/2010 11:42 AM, Mike Frysinger wrote:
> On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:
>    
>> On 4/27/2010 11:15 AM, Mike Frysinger wrote:
>>      
>>> -	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
>    
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.

regards,
Ben

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Mike Frysinger]

On Tuesday 27 April 2010 14:46:34 Ben Warren wrote:
> On 4/27/2010 11:42 AM, Mike Frysinger wrote:
> > On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:
> >> On 4/27/2010 11:15 AM, Mike Frysinger wrote:
> >>> -	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().
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100427/f48b75f3/attachment.pgp 

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Ben Warren]

On 4/27/2010 12:32 PM, Mike Frysinger wrote:
> On Tuesday 27 April 2010 14:46:34 Ben Warren wrote:
>    
>> On 4/27/2010 11:42 AM, Mike Frysinger wrote:
>>      
>>> On Tuesday 27 April 2010 14:33:09 Ben Warren wrote:
>>>        
>>>> On 4/27/2010 11:15 AM, Mike Frysinger wrote:
>>>>          
>>>>> -	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().
>>>        
>> 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
>    
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?

I'll take your patch and maybe next release we can work on improving this.

regards,
Ben

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Mike Frysinger]

On Tuesday 27 April 2010 16:07:21 Ben Warren wrote:
> On 4/27/2010 12:32 PM, Mike Frysinger wrote:
> > 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?

the reason it's written the way it is currently is due to the list of eth 
drivers sometimes being used as a linked list and other times as an indexed 
array.  the two loops could probably be merged in eth_init(), but i dont think 
this overall will accomplish the new intended behavior.

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.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100427/154faf06/attachment.pgp 

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Detlev Zundel]

Hi Mike and Ben,

> 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.

> 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 ;)

Cheers
  Detlev

-- 
We can forgive a man for making a useful thing as long as he does not
admire it.  The only excuse  for making  a useless  thing is that one
admires it intensely.
                                    --- Oscar Wilde
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] Blackfin: bfin_mac: hook up new write_hwaddr function

[Mike Frysinger]

On Friday 30 April 2010 08:40:21 Detlev Zundel wrote:
> > 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.

you seem to be confused by what that code actually did.  it never programmed 
the mac in the hardware, it synced the eth_device and the environment.  the 
behavior we have today is exactly the same before that series of commits i 
referenced.  i simply standardized things across the board and unified heavily 
duplicated behavior.  what i'm talking about adding here is different from 
what used to be there ... it's just that the mechanism is the same.

> > 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 ;)

there's going to be one patch for every driver.  just because only a few 
people have submitted patches for drivers they're interested in doesnt mean 
the overall thrashing is small.  a quick grep counts about 35 drivers that 
need updating.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100430/f9098326/attachment.pgp