[U-Boot] [PATCH 1/2] net: add Calxeda xgmac driver

[Mike Frysinger]

On Friday 02 December 2011 17:46:39 Rob Herring wrote:
> On 12/02/2011 04:14 PM, Mike Frysinger wrote:
> > On Friday 02 December 2011 17:02:00 Rob Herring wrote:
> >> On 12/02/2011 03:30 PM, Mike Frysinger wrote:
> >>> On Friday 02 December 2011 15:21:48 Rob Herring wrote:
> >>>> +	macaddr[1] = readl(dev->iobase + XGMAC_CORE_MACADDR0HI);
> >>>> +	macaddr[0] = readl(dev->iobase + XGMAC_CORE_MACADDR0LO);
> >>>> +	memcpy(dev->enetaddr, macaddr, 6);
> >>> 
> >>> does the initial mac regs really start off with useful info ?
> >> 
> >> Yes. It contains the only value that will work.
> > 
> > what i mean is that on embedded peripheral blocks, the device powers on
> > with blank register settings and the core needs to program them.  how
> > did your device get a mac address already programmed into it ?  did
> > something run before u-boot and initialize the registers ?  does the
> > hardware block preseed the registers itself by talking to some internal
> > storage ?  certainly the mac address isn't programmed into the hardware
> > block itself :).
> 
> Something else runs and sets it up and u-boot does not have access to it.

isn't this a board-specific assumption ?  shouldn't those lines be protected by 
a CONFIG_xxx ifdef that boards can enable as they see fit ?

> >>>> +	sprintf(enetvar, id ? "eth%daddr" : "ethaddr", id);
> >>>> +	eth_setenv_enetaddr(enetvar, dev->enetaddr);
> >>> 
> >>> NAK: delete this
> >> 
> >> PXE boot needs the MAC address to generate filenames and gets it from
> >> the env. See format_mac_pxe function in common/cmd_pxe.c. Should that be
> >> done differently? The user setting a MAC address on our platform won't
> >> work, so using the env setting as an override is not valid.
> > 
> > device drivers should not be touching the env.  common code takes care of
> > that.  your driver should only be writing dev->enetaddr.
> 
> The common code does not set the env setting. The env setting is
> normally an override of the h/w value and may not even exist. So how
> should the pxe boot command get the MAC address?
> 
> - move setting of ethXaddr env to the highbank board file
> - Have PXE call eth_get_dev and get it directly from struct eth_device.
> 
> The latter is the only way to enable all boards at once.

i vaguely recall someone else mentioning this, but it doesn't change the 
driver logic.  no net driver may touch the env, FIN.  we can talk about 
improving the core/common net code.  but let's do so in a dedicated thread.
-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/20111202/54e86324/attachment.pgp>