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

[Mike Frysinger]

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:
> >> --- /dev/null
> >> +++ b/drivers/net/calxedaxgmac.c
> >> 
> >> +	writel(value, dev->iobase + XGMAC_CORE_CONFIG);
> > 
> > you should declare a C struct that represents the hardware's register
> > layout, and then use that rather than iobase+register_offset
> 
> Is that a suggestion or u-boot mandate? Because the Linux version of the
> driver does it the current way already, it's certainly done both ways in
> u-boot drivers already and personally I really don't like structs for
> register offsets.

i think Wolfgang would tell you it's a mandate, and code you see using 
register offsets are the old style that should get updated.  sorry.

> >> +	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 :).

> >> +	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.
-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/efd292c4/attachment.pgp>