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

[Rob Herring <robherring2@gmail.com>]

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

Something else runs and sets it up and u-boot does not have access to it.

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

Rob