[U-Boot] [PATCH 2/2] net: add driver for Synopsys Ethernet QoS device

[Stephen Warren]

On 10/11/2016 04:48 PM, Joe Hershberger wrote:
> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for sending this! I have some comments below.
>>>
>>> Cheers,
>>> -Joe
>>>
>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren at wwwdotorg.org>
>>> wrote:
>>>>
>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>
>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>> supported by the existing designware.c driver. The IP supports many
>>>> options for bus type, clocking/reset structure, and feature list. This
>>>> driver currently supports the specific configuration used in NVIDIA's
>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>> easily, as explained in the source.
>>
>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>
>>> +static int eqos_start(struct udevice *dev)
>>
>>>> +       /* Update the MAC address */
>>>> +       val = (plat->enetaddr[5] << 8) |
>>>> +               (plat->enetaddr[4]);
>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>> +       val = (plat->enetaddr[3] << 24) |
>>>> +               (plat->enetaddr[2] << 16) |
>>>> +               (plat->enetaddr[1] << 8) |
>>>> +               (plat->enetaddr[0]);
>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>
>>>
>>> This should be implemented in write_hwaddr() op.
>>
>> That op is never called because this driver is only instantiated by device
>> tree. Since this code can't be skipped, it can't be moved to that op.
>
> I don't understand what you're saying here. That op is called in
> eth_initialize() on every device found in device tree.

Oh, so it is. I must have screwed up my tracing before.

Anyway, I still don't believe using write_hwaddr() is correct for this 
HW. It's marked optional in include/net.h; it would be implemented in 
cases where the MAC address should be passed to subsequent SW in 
Ethernet controller registers. That's not the case here. The master 
location for the MAC address is in an unrelated EEPROM that all drivers 
must read. Resetting the device (as any driver would do to guarantee 
correct operation no matter what SW ran before), or never initializing 
it in the first place (as is the case now without any driver in U-Boot) 
means that those registers cannot be assumed to contain valid data. Thus 
SW after U-Boot won't rely on the MAC address register values, and hence 
there's no need to implement .write_hwaddr() separately, since that's 
the only reason it exists separately according to the comments in 
include/net.h.