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

[Rob Herring <robherring2@gmail.com>]

Mike,

Thanks for your quick review.

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.

...

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

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

Rob