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

[Yann Vernier]

On Fri, 26 Oct 2012 23:37:28 +0200
Albert ARIBAUD <albert.u.boot at aribaud.net> wrote:

> Hi Yann,
> 
> On Fri, 19 Oct 2012 10:02:09 +0200, Yann Vernier
> <yann.vernier at orsoc.se> wrote:
> 
> > On Thu, 18 Oct 2012 15:55:31 -0500
> > Joe Hershberger <joe.hershberger at gmail.com> wrote:
> > 
> > > Hi Yann,
> > > 
> > > On Fri, Oct 5, 2012 at 7:09 AM, Yann Vernier
> > > <yann.vernier at 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.