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

Rob Herring
2011-12-02T22:02:00Z

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