diff mbox

[U-Boot,03/11] arm: ks8695eth: Use MAC address from environment

Message ID 1349438998-10954-4-git-send-email-yann.vernier@orsoc.se
State Changes Requested
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Yann Vernier Oct. 5, 2012, 12:09 p.m. UTC 
Removed board specific MAC reading code from driver.
Should move the reading to the cm4008/cm41xx board code.
---
 drivers/net/ks8695eth.c |   38 +++++++++-----------------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

Comments

Albert ARIBAUD Oct. 18, 2012, 7 p.m. UTC | #1 
Hi Yann,

On Fri,  5 Oct 2012 14:09:50 +0200, Yann Vernier
<yann.vernier@orsoc.se> wrote:

> Removed board specific MAC reading code from driver.
> Should move the reading to the cm4008/cm41xx board code.
> ---
>  drivers/net/ks8695eth.c |   38 +++++++++-----------------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
> index b4904b6..2dda7ab 100644
> --- a/drivers/net/ks8695eth.c
> +++ b/drivers/net/ks8695eth.c
> @@ -71,30 +71,13 @@ volatile uint8_t ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
>  
>  /****************************************************************************/
>  
> -/*
> - *	Ideally we want to use the MAC address stored in flash.
> - *	But we do some sanity checks in case they are not present
> - *	first.
> - */
> -unsigned char eth_mac[] = {
> -	0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
> -};
> -
> -void ks8695_getmac(void)
> +static int ks8695_set_mac_address(struct eth_device *dev)
>  {
> -	unsigned char *fp;
> -	int i;
> -
> -	/* Check if flash MAC is valid */
> -	fp = (unsigned char *) 0x0201c000;
> -	for (i = 0; (i < 6); i++) {
> -		if ((fp[i] != 0) && (fp[i] != 0xff))
> -			break;
> -	}
> -
> -	/* If we found a valid looking MAC address then use it */
> -	if (i < 6)
> -		memcpy(&eth_mac[0], fp, 6);
> +	/* Set MAC address */
> +	ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] | (dev->enetaddr[4] << 8) |
> +		(dev->enetaddr[3] << 16) | (dev->enetaddr[2] << 24)));
> +	ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] | (dev->enetaddr[0] << 8)));
> +	return 0;
>  }
>  
>  /****************************************************************************/
> @@ -109,12 +92,8 @@ static int ks8695_eth_init(struct eth_device *dev, bd_t *bd)
>  	ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
>  	ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
>  
> -	ks8695_getmac();
> -
>  	/* Set MAC address */
> -	ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] | (eth_mac[4] << 8) |
> -		(eth_mac[3] << 16) | (eth_mac[2] << 24)));
> -	ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] | (eth_mac[0] << 8)));
> +	ks8695_set_mac_address(dev);
>  
>  	/* Turn the 4 port switch on */
>  	i = ks8695_read(KS8695_SWITCH_CTRL0);
> @@ -150,7 +129,7 @@ static int ks8695_eth_init(struct eth_device *dev, bd_t *bd)
>  	ks8695_write(KS8695_LAN_DMA_RX, 0x71);
>  	ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
>  
> -	printf("KS8695 ETHERNET: %pM\n", eth_mac);
> +	printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
>  	return 0;
>  }
>  
> @@ -234,6 +213,7 @@ int ks8695_eth_initialize(void)
>  	dev->halt = ks8695_eth_halt;
>  	dev->send = ks8695_eth_send;
>  	dev->recv = ks8695_eth_recv;
> +	dev->write_hwaddr = ks8695_set_mac_address;
>  	strcpy(dev->name, "ks8695eth");
>  
>  	eth_register(dev);

Cc:ing Joe as the network custodian for ack'ing the patch (although I'll
take it along with the whole series).

Amicalement,
Joe Hershberger Oct. 18, 2012, 8:55 p.m. UTC | #2 
Hi Yann,

On Fri, Oct 5, 2012 at 7:09 AM, Yann Vernier <yann.vernier@orsoc.se> wrote:
> Removed board specific MAC reading code from driver.
> Should move the reading to the cm4008/cm41xx board code.
> ---
>  drivers/net/ks8695eth.c |   38 +++++++++-----------------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
> index b4904b6..2dda7ab 100644
> --- a/drivers/net/ks8695eth.c
> +++ b/drivers/net/ks8695eth.c
> @@ -71,30 +71,13 @@ volatile uint8_t ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
>
>  /****************************************************************************/
>
> -/*
> - *     Ideally we want to use the MAC address stored in flash.
> - *     But we do some sanity checks in case they are not present
> - *     first.
> - */
> -unsigned char eth_mac[] = {
> -       0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
> -};
> -
> -void ks8695_getmac(void)
> +static int ks8695_set_mac_address(struct eth_device *dev)
>  {
> -       unsigned char *fp;
> -       int i;
> -
> -       /* Check if flash MAC is valid */
> -       fp = (unsigned char *) 0x0201c000;
> -       for (i = 0; (i < 6); i++) {
> -               if ((fp[i] != 0) && (fp[i] != 0xff))
> -                       break;
> -       }
> -
> -       /* If we found a valid looking MAC address then use it */
> -       if (i < 6)
> -               memcpy(&eth_mac[0], fp, 6);
> +       /* Set MAC address */
> +       ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] | (dev->enetaddr[4] << 8) |
> +               (dev->enetaddr[3] << 16) | (dev->enetaddr[2] << 24)));
> +       ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] | (dev->enetaddr[0] << 8)));
> +       return 0;
>  }
>
>  /****************************************************************************/
> @@ -109,12 +92,8 @@ static int ks8695_eth_init(struct eth_device *dev, bd_t *bd)
>         ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
>         ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
>
> -       ks8695_getmac();
> -
>         /* Set MAC address */
> -       ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] | (eth_mac[4] << 8) |
> -               (eth_mac[3] << 16) | (eth_mac[2] << 24)));
> -       ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] | (eth_mac[0] << 8)));
> +       ks8695_set_mac_address(dev);

Why do you set the MAC address here?  It should be set for you by the
network infrastructure without this call.

>
>         /* Turn the 4 port switch on */
>         i = ks8695_read(KS8695_SWITCH_CTRL0);
> @@ -150,7 +129,7 @@ static int ks8695_eth_init(struct eth_device *dev, bd_t *bd)
>         ks8695_write(KS8695_LAN_DMA_RX, 0x71);
>         ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
>
> -       printf("KS8695 ETHERNET: %pM\n", eth_mac);
> +       printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
>         return 0;
>  }
>
> @@ -234,6 +213,7 @@ int ks8695_eth_initialize(void)
>         dev->halt = ks8695_eth_halt;
>         dev->send = ks8695_eth_send;
>         dev->recv = ks8695_eth_recv;
> +       dev->write_hwaddr = ks8695_set_mac_address;
>         strcpy(dev->name, "ks8695eth");
>
>         eth_register(dev);
> --
> 1.7.10.4

-Joe
Yann Vernier Oct. 19, 2012, 8:02 a.m. UTC | #3 
On Thu, 18 Oct 2012 15:55:31 -0500
Joe Hershberger <joe.hershberger@gmail.com> wrote:

> Hi Yann,
> 
> On Fri, Oct 5, 2012 at 7:09 AM, Yann Vernier <yann.vernier@orsoc.se>
> wrote:
> > Removed board specific MAC reading code from driver.
> > Should move the reading to the cm4008/cm41xx board code.
> > ---
> >  drivers/net/ks8695eth.c |   38
> > +++++++++----------------------------- 1 file changed, 9
> > insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
> > index b4904b6..2dda7ab 100644
> > --- a/drivers/net/ks8695eth.c
> > +++ b/drivers/net/ks8695eth.c
> > @@ -71,30 +71,13 @@ volatile uint8_t
> > ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
> >
> >  /****************************************************************************/
> >
> > -/*
> > - *     Ideally we want to use the MAC address stored in flash.
> > - *     But we do some sanity checks in case they are not present
> > - *     first.
> > - */
> > -unsigned char eth_mac[] = {
> > -       0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
> > -};
> > -
> > -void ks8695_getmac(void)
> > +static int ks8695_set_mac_address(struct eth_device *dev)
> >  {
> > -       unsigned char *fp;
> > -       int i;
> > -
> > -       /* Check if flash MAC is valid */
> > -       fp = (unsigned char *) 0x0201c000;
> > -       for (i = 0; (i < 6); i++) {
> > -               if ((fp[i] != 0) && (fp[i] != 0xff))
> > -                       break;
> > -       }
> > -
> > -       /* If we found a valid looking MAC address then use it */
> > -       if (i < 6)
> > -               memcpy(&eth_mac[0], fp, 6);
> > +       /* Set MAC address */
> > +       ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] |
> > (dev->enetaddr[4] << 8) |
> > +               (dev->enetaddr[3] << 16) | (dev->enetaddr[2] <<
> > 24)));
> > +       ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] |
> > (dev->enetaddr[0] << 8)));
> > +       return 0;
> >  }
> >
> >  /****************************************************************************/
> > @@ -109,12 +92,8 @@ static int ks8695_eth_init(struct eth_device
> > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
> >         ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
> >
> > -       ks8695_getmac();
> > -
> >         /* Set MAC address */
> > -       ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] | (eth_mac[4]
> > << 8) |
> > -               (eth_mac[3] << 16) | (eth_mac[2] << 24)));
> > -       ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] | (eth_mac[0]
> > << 8)));
> > +       ks8695_set_mac_address(dev);
> 
> Why do you set the MAC address here?  It should be set for you by the
> network infrastructure without this call.

Simply because I was not aware of this at the time. Before the changes
here the generic infrastructure could not do it, as write_hwaddr was
unimplemented, and I was just trying to preserve function. The main
reason I started poking at the network driver was that it loaded the
MAC from a hardcoded address in ROM, obviously board specific (this is
the magic number in [PATCH 11/11] arm: cm4008, cm41xx: read MAC address
from flash). It also has a bug where it stops working after one command
(i.e. can't tftp twice), which I have not tracked down as yet. 

> >         /* Turn the 4 port switch on */
> >         i = ks8695_read(KS8695_SWITCH_CTRL0);
> > @@ -150,7 +129,7 @@ static int ks8695_eth_init(struct eth_device
> > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_RX, 0x71);
> >         ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
> >
> > -       printf("KS8695 ETHERNET: %pM\n", eth_mac);
> > +       printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
> >         return 0;
> >  }
> >
> > @@ -234,6 +213,7 @@ int ks8695_eth_initialize(void)
> >         dev->halt = ks8695_eth_halt;
> >         dev->send = ks8695_eth_send;
> >         dev->recv = ks8695_eth_recv;
> > +       dev->write_hwaddr = ks8695_set_mac_address;
> >         strcpy(dev->name, "ks8695eth");
> >
> >         eth_register(dev);
> > --
> > 1.7.10.4
> 
> -Joe
Albert ARIBAUD Oct. 26, 2012, 9:37 p.m. UTC | #4 
Hi Yann,

On Fri, 19 Oct 2012 10:02:09 +0200, Yann Vernier
<yann.vernier@orsoc.se> wrote:

> On Thu, 18 Oct 2012 15:55:31 -0500
> Joe Hershberger <joe.hershberger@gmail.com> wrote:
> 
> > Hi Yann,
> > 
> > On Fri, Oct 5, 2012 at 7:09 AM, Yann Vernier <yann.vernier@orsoc.se>
> > wrote:
> > > Removed board specific MAC reading code from driver.
> > > Should move the reading to the cm4008/cm41xx board code.
> > > ---
> > >  drivers/net/ks8695eth.c |   38
> > > +++++++++----------------------------- 1 file changed, 9
> > > insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
> > > index b4904b6..2dda7ab 100644
> > > --- a/drivers/net/ks8695eth.c
> > > +++ b/drivers/net/ks8695eth.c
> > > @@ -71,30 +71,13 @@ volatile uint8_t
> > > ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
> > >
> > >  /****************************************************************************/
> > >
> > > -/*
> > > - *     Ideally we want to use the MAC address stored in flash.
> > > - *     But we do some sanity checks in case they are not present
> > > - *     first.
> > > - */
> > > -unsigned char eth_mac[] = {
> > > -       0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
> > > -};
> > > -
> > > -void ks8695_getmac(void)
> > > +static int ks8695_set_mac_address(struct eth_device *dev)
> > >  {
> > > -       unsigned char *fp;
> > > -       int i;
> > > -
> > > -       /* Check if flash MAC is valid */
> > > -       fp = (unsigned char *) 0x0201c000;
> > > -       for (i = 0; (i < 6); i++) {
> > > -               if ((fp[i] != 0) && (fp[i] != 0xff))
> > > -                       break;
> > > -       }
> > > -
> > > -       /* If we found a valid looking MAC address then use it */
> > > -       if (i < 6)
> > > -               memcpy(&eth_mac[0], fp, 6);
> > > +       /* Set MAC address */
> > > +       ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] |
> > > (dev->enetaddr[4] << 8) |
> > > +               (dev->enetaddr[3] << 16) | (dev->enetaddr[2] <<
> > > 24)));
> > > +       ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] |
> > > (dev->enetaddr[0] << 8)));
> > > +       return 0;
> > >  }
> > >
> > >  /****************************************************************************/
> > > @@ -109,12 +92,8 @@ static int ks8695_eth_init(struct eth_device
> > > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
> > >         ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
> > >
> > > -       ks8695_getmac();
> > > -
> > >         /* Set MAC address */
> > > -       ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] | (eth_mac[4]
> > > << 8) |
> > > -               (eth_mac[3] << 16) | (eth_mac[2] << 24)));
> > > -       ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] | (eth_mac[0]
> > > << 8)));
> > > +       ks8695_set_mac_address(dev);
> > 
> > Why do you set the MAC address here?  It should be set for you by the
> > network infrastructure without this call.
> 
> Simply because I was not aware of this at the time. Before the changes
> here the generic infrastructure could not do it, as write_hwaddr was
> unimplemented, and I was just trying to preserve function. The main
> reason I started poking at the network driver was that it loaded the
> MAC from a hardcoded address in ROM, obviously board specific (this is
> the magic number in [PATCH 11/11] arm: cm4008, cm41xx: read MAC address
> from flash). It also has a bug where it stops working after one command
> (i.e. can't tftp twice), which I have not tracked down as yet. 
> 
> > >         /* Turn the 4 port switch on */
> > >         i = ks8695_read(KS8695_SWITCH_CTRL0);
> > > @@ -150,7 +129,7 @@ static int ks8695_eth_init(struct eth_device
> > > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_RX, 0x71);
> > >         ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
> > >
> > > -       printf("KS8695 ETHERNET: %pM\n", eth_mac);
> > > +       printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
> > >         return 0;
> > >  }
> > >
> > > @@ -234,6 +213,7 @@ int ks8695_eth_initialize(void)
> > >         dev->halt = ks8695_eth_halt;
> > >         dev->send = ks8695_eth_send;
> > >         dev->recv = ks8695_eth_recv;
> > > +       dev->write_hwaddr = ks8695_set_mac_address;
> > >         strcpy(dev->name, "ks8695eth");
> > >
> > >         eth_register(dev);
> > > --
> > > 1.7.10.4
> > 
> > -Joe

What is the status for this patch? Are you going to drop it or submit a
new version?

Amicalement,
Yann Vernier Nov. 19, 2012, 11:42 a.m. UTC | #5 
On Fri, 26 Oct 2012 23:37:28 +0200
Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:

> Hi Yann,
> 
> On Fri, 19 Oct 2012 10:02:09 +0200, Yann Vernier
> <yann.vernier@orsoc.se> wrote:
> 
> > On Thu, 18 Oct 2012 15:55:31 -0500
> > Joe Hershberger <joe.hershberger@gmail.com> wrote:
> > 
> > > Hi Yann,
> > > 
> > > On Fri, Oct 5, 2012 at 7:09 AM, Yann Vernier
> > > <yann.vernier@orsoc.se> wrote:
> > > > Removed board specific MAC reading code from driver.
> > > > Should move the reading to the cm4008/cm41xx board code.
> > > > ---
> > > >  drivers/net/ks8695eth.c |   38
> > > > +++++++++----------------------------- 1 file changed, 9
> > > > insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
> > > > index b4904b6..2dda7ab 100644
> > > > --- a/drivers/net/ks8695eth.c
> > > > +++ b/drivers/net/ks8695eth.c
> > > > @@ -71,30 +71,13 @@ volatile uint8_t
> > > > ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
> > > >
> > > >  /****************************************************************************/
> > > >
> > > > -/*
> > > > - *     Ideally we want to use the MAC address stored in flash.
> > > > - *     But we do some sanity checks in case they are not
> > > > present
> > > > - *     first.
> > > > - */
> > > > -unsigned char eth_mac[] = {
> > > > -       0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
> > > > -};
> > > > -
> > > > -void ks8695_getmac(void)
> > > > +static int ks8695_set_mac_address(struct eth_device *dev)
> > > >  {
> > > > -       unsigned char *fp;
> > > > -       int i;
> > > > -
> > > > -       /* Check if flash MAC is valid */
> > > > -       fp = (unsigned char *) 0x0201c000;
> > > > -       for (i = 0; (i < 6); i++) {
> > > > -               if ((fp[i] != 0) && (fp[i] != 0xff))
> > > > -                       break;
> > > > -       }
> > > > -
> > > > -       /* If we found a valid looking MAC address then use it
> > > > */
> > > > -       if (i < 6)
> > > > -               memcpy(&eth_mac[0], fp, 6);
> > > > +       /* Set MAC address */
> > > > +       ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] |
> > > > (dev->enetaddr[4] << 8) |
> > > > +               (dev->enetaddr[3] << 16) | (dev->enetaddr[2] <<
> > > > 24)));
> > > > +       ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] |
> > > > (dev->enetaddr[0] << 8)));
> > > > +       return 0;
> > > >  }
> > > >
> > > >  /****************************************************************************/
> > > > @@ -109,12 +92,8 @@ static int ks8695_eth_init(struct eth_device
> > > > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
> > > >         ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
> > > >
> > > > -       ks8695_getmac();
> > > > -
> > > >         /* Set MAC address */
> > > > -       ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] |
> > > > (eth_mac[4] << 8) |
> > > > -               (eth_mac[3] << 16) | (eth_mac[2] << 24)));
> > > > -       ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] |
> > > > (eth_mac[0] << 8)));
> > > > +       ks8695_set_mac_address(dev);
> > > 
> > > Why do you set the MAC address here?  It should be set for you by
> > > the network infrastructure without this call.
> > 
> > Simply because I was not aware of this at the time. Before the
> > changes here the generic infrastructure could not do it, as
> > write_hwaddr was unimplemented, and I was just trying to preserve
> > function. The main reason I started poking at the network driver
> > was that it loaded the MAC from a hardcoded address in ROM,
> > obviously board specific (this is the magic number in [PATCH 11/11]
> > arm: cm4008, cm41xx: read MAC address from flash). It also has a
> > bug where it stops working after one command (i.e. can't tftp
> > twice), which I have not tracked down as yet. 
> > 
> > > >         /* Turn the 4 port switch on */
> > > >         i = ks8695_read(KS8695_SWITCH_CTRL0);
> > > > @@ -150,7 +129,7 @@ static int ks8695_eth_init(struct eth_device
> > > > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_RX, 0x71);
> > > >         ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
> > > >
> > > > -       printf("KS8695 ETHERNET: %pM\n", eth_mac);
> > > > +       printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
> > > >         return 0;
> > > >  }
> > > >
> > > > @@ -234,6 +213,7 @@ int ks8695_eth_initialize(void)
> > > >         dev->halt = ks8695_eth_halt;
> > > >         dev->send = ks8695_eth_send;
> > > >         dev->recv = ks8695_eth_recv;
> > > > +       dev->write_hwaddr = ks8695_set_mac_address;
> > > >         strcpy(dev->name, "ks8695eth");
> > > >
> > > >         eth_register(dev);
> > > > --
> > > > 1.7.10.4
> > > 
> > > -Joe
> 
> What is the status for this patch? Are you going to drop it or submit
> a new version?
> 
> Amicalement,

After some testing, I can conclude that removing the call to
set_mac_address actually breaks this code. This is because the first
section of ks8695_eth_init sets a reset bit, which resets all register
values including the MAC address. The debug message shows that this has
at some point been in an eth_reset() subroutine. The same reset bit is
set in ks8695_eth_halt(), which is why Linux does not see the MAC
address set by u-boot. 

At this point, I do not know of a better approach than the patch as
submitted. Advice is welcome.
Joe Hershberger Dec. 1, 2012, 7:23 p.m. UTC | #6 
Hi Yann,

On Mon, Nov 19, 2012 at 5:42 AM, Yann Vernier <yann.vernier@orsoc.se> wrote:
> On Fri, 26 Oct 2012 23:37:28 +0200
> Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
>
>> Hi Yann,
>>
>> On Fri, 19 Oct 2012 10:02:09 +0200, Yann Vernier
>> <yann.vernier@orsoc.se> wrote:
>>
>> > On Thu, 18 Oct 2012 15:55:31 -0500
>> > Joe Hershberger <joe.hershberger@gmail.com> wrote:
>> >
>> > > Hi Yann,
>> > >
>> > > On Fri, Oct 5, 2012 at 7:09 AM, Yann Vernier
>> > > <yann.vernier@orsoc.se> wrote:
>> > > > Removed board specific MAC reading code from driver.
>> > > > Should move the reading to the cm4008/cm41xx board code.
>> > > > ---
>> > > >  drivers/net/ks8695eth.c |   38
>> > > > +++++++++----------------------------- 1 file changed, 9
>> > > > insertions(+), 29 deletions(-)
>> > > >
>> > > > diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
>> > > > index b4904b6..2dda7ab 100644
>> > > > --- a/drivers/net/ks8695eth.c
>> > > > +++ b/drivers/net/ks8695eth.c
>> > > > @@ -71,30 +71,13 @@ volatile uint8_t
>> > > > ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
>> > > >
>> > > >  /****************************************************************************/
>> > > >
>> > > > -/*
>> > > > - *     Ideally we want to use the MAC address stored in flash.
>> > > > - *     But we do some sanity checks in case they are not
>> > > > present
>> > > > - *     first.
>> > > > - */
>> > > > -unsigned char eth_mac[] = {
>> > > > -       0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
>> > > > -};
>> > > > -
>> > > > -void ks8695_getmac(void)
>> > > > +static int ks8695_set_mac_address(struct eth_device *dev)
>> > > >  {
>> > > > -       unsigned char *fp;
>> > > > -       int i;
>> > > > -
>> > > > -       /* Check if flash MAC is valid */
>> > > > -       fp = (unsigned char *) 0x0201c000;
>> > > > -       for (i = 0; (i < 6); i++) {
>> > > > -               if ((fp[i] != 0) && (fp[i] != 0xff))
>> > > > -                       break;
>> > > > -       }
>> > > > -
>> > > > -       /* If we found a valid looking MAC address then use it
>> > > > */
>> > > > -       if (i < 6)
>> > > > -               memcpy(&eth_mac[0], fp, 6);
>> > > > +       /* Set MAC address */
>> > > > +       ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] |
>> > > > (dev->enetaddr[4] << 8) |
>> > > > +               (dev->enetaddr[3] << 16) | (dev->enetaddr[2] <<
>> > > > 24)));
>> > > > +       ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] |
>> > > > (dev->enetaddr[0] << 8)));
>> > > > +       return 0;
>> > > >  }
>> > > >
>> > > >  /****************************************************************************/
>> > > > @@ -109,12 +92,8 @@ static int ks8695_eth_init(struct eth_device
>> > > > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
>> > > >         ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
>> > > >
>> > > > -       ks8695_getmac();
>> > > > -
>> > > >         /* Set MAC address */
>> > > > -       ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] |
>> > > > (eth_mac[4] << 8) |
>> > > > -               (eth_mac[3] << 16) | (eth_mac[2] << 24)));
>> > > > -       ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] |
>> > > > (eth_mac[0] << 8)));
>> > > > +       ks8695_set_mac_address(dev);
>> > >
>> > > Why do you set the MAC address here?  It should be set for you by
>> > > the network infrastructure without this call.
>> >
>> > Simply because I was not aware of this at the time. Before the
>> > changes here the generic infrastructure could not do it, as
>> > write_hwaddr was unimplemented, and I was just trying to preserve
>> > function. The main reason I started poking at the network driver
>> > was that it loaded the MAC from a hardcoded address in ROM,
>> > obviously board specific (this is the magic number in [PATCH 11/11]
>> > arm: cm4008, cm41xx: read MAC address from flash). It also has a
>> > bug where it stops working after one command (i.e. can't tftp
>> > twice), which I have not tracked down as yet.
>> >
>> > > >         /* Turn the 4 port switch on */
>> > > >         i = ks8695_read(KS8695_SWITCH_CTRL0);
>> > > > @@ -150,7 +129,7 @@ static int ks8695_eth_init(struct eth_device
>> > > > *dev, bd_t *bd) ks8695_write(KS8695_LAN_DMA_RX, 0x71);
>> > > >         ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
>> > > >
>> > > > -       printf("KS8695 ETHERNET: %pM\n", eth_mac);
>> > > > +       printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
>> > > >         return 0;
>> > > >  }
>> > > >
>> > > > @@ -234,6 +213,7 @@ int ks8695_eth_initialize(void)
>> > > >         dev->halt = ks8695_eth_halt;
>> > > >         dev->send = ks8695_eth_send;
>> > > >         dev->recv = ks8695_eth_recv;
>> > > > +       dev->write_hwaddr = ks8695_set_mac_address;
>> > > >         strcpy(dev->name, "ks8695eth");
>> > > >
>> > > >         eth_register(dev);
>> > > > --
>> > > > 1.7.10.4
>> > >
>> > > -Joe
>>
>> What is the status for this patch? Are you going to drop it or submit
>> a new version?
>>
>> Amicalement,
>
> After some testing, I can conclude that removing the call to
> set_mac_address actually breaks this code. This is because the first
> section of ks8695_eth_init sets a reset bit, which resets all register
> values including the MAC address. The debug message shows that this has
> at some point been in an eth_reset() subroutine. The same reset bit is
> set in ks8695_eth_halt(), which is why Linux does not see the MAC
> address set by u-boot.
>
> At this point, I do not know of a better approach than the patch as
> submitted. Advice is welcome.

It seems to me you should not be resetting, or if you must then I
guess what you have here is best.  In that case, though, you should
also be writing the MAC address in halt.  It doesn't sound like good
behavior that if you use Ethernet in u-boot, that you get no MAC
address in Linux.

-Joe
diff mbox

Patch

diff --git a/drivers/net/ks8695eth.c b/drivers/net/ks8695eth.c
index b4904b6..2dda7ab 100644
--- a/drivers/net/ks8695eth.c
+++ b/drivers/net/ks8695eth.c
@@ -71,30 +71,13 @@  volatile uint8_t ks8695_bufs[BUFSIZE*(TXDESCS+RXDESCS)] __attribute__((aligned(2
 
 /****************************************************************************/
 
-/*
- *	Ideally we want to use the MAC address stored in flash.
- *	But we do some sanity checks in case they are not present
- *	first.
- */
-unsigned char eth_mac[] = {
-	0x00, 0x13, 0xc6, 0x00, 0x00, 0x00
-};
-
-void ks8695_getmac(void)
+static int ks8695_set_mac_address(struct eth_device *dev)
 {
-	unsigned char *fp;
-	int i;
-
-	/* Check if flash MAC is valid */
-	fp = (unsigned char *) 0x0201c000;
-	for (i = 0; (i < 6); i++) {
-		if ((fp[i] != 0) && (fp[i] != 0xff))
-			break;
-	}
-
-	/* If we found a valid looking MAC address then use it */
-	if (i < 6)
-		memcpy(&eth_mac[0], fp, 6);
+	/* Set MAC address */
+	ks8695_write(KS8695_LAN_MAC_LOW, (dev->enetaddr[5] | (dev->enetaddr[4] << 8) |
+		(dev->enetaddr[3] << 16) | (dev->enetaddr[2] << 24)));
+	ks8695_write(KS8695_LAN_MAC_HIGH, (dev->enetaddr[1] | (dev->enetaddr[0] << 8)));
+	return 0;
 }
 
 /****************************************************************************/
@@ -109,12 +92,8 @@  static int ks8695_eth_init(struct eth_device *dev, bd_t *bd)
 	ks8695_write(KS8695_LAN_DMA_TX, 0x80000000);
 	ks8695_write(KS8695_LAN_DMA_RX, 0x80000000);
 
-	ks8695_getmac();
-
 	/* Set MAC address */
-	ks8695_write(KS8695_LAN_MAC_LOW, (eth_mac[5] | (eth_mac[4] << 8) |
-		(eth_mac[3] << 16) | (eth_mac[2] << 24)));
-	ks8695_write(KS8695_LAN_MAC_HIGH, (eth_mac[1] | (eth_mac[0] << 8)));
+	ks8695_set_mac_address(dev);
 
 	/* Turn the 4 port switch on */
 	i = ks8695_read(KS8695_SWITCH_CTRL0);
@@ -150,7 +129,7 @@  static int ks8695_eth_init(struct eth_device *dev, bd_t *bd)
 	ks8695_write(KS8695_LAN_DMA_RX, 0x71);
 	ks8695_write(KS8695_LAN_DMA_RX_START, 0x1);
 
-	printf("KS8695 ETHERNET: %pM\n", eth_mac);
+	printf("KS8695 ETHERNET: %pM\n", dev->enetaddr);
 	return 0;
 }
 
@@ -234,6 +213,7 @@  int ks8695_eth_initialize(void)
 	dev->halt = ks8695_eth_halt;
 	dev->send = ks8695_eth_send;
 	dev->recv = ks8695_eth_recv;
+	dev->write_hwaddr = ks8695_set_mac_address;
 	strcpy(dev->name, "ks8695eth");
 
 	eth_register(dev);