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

Rob Herring
2011-12-02T22:46:39Z

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