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

[Joe Hershberger]

Hi Yann,

On Mon, Nov 19, 2012 at 5:42 AM, Yann Vernier <yann.vernier at orsoc.se> wrote:
> 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.

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