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

[Joe Hershberger]

Hi Stephen,

On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>
>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren at wwwdotorg.org>
>> wrote:
>>>
>>> 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.
>>
>>
>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>> to get a default mac addr that can be overridden with the env.
>
>
> If the EQoS HW module contained the interface to this EEPROM, such that all
> instances of the HW module always accessed the EEPROM in the same way and
> the layout of data in the EEPROM was fixed by the HW module, then yes.
>
> However, the EqoS HW module doesn't define any mechanism for non-volatile
> MAC address storage; only the runtime registers. So, we can't implement
> read_rom_hwaddr() inside the EQoS driver unfortunately.

OK.

>> The
>> write_hwaddr is about what the mac uses to filter for limiting packet
>> ingress. One reason to support it as an op is so that when the env var
>> for the mac address is changed, the mac filter in the hw is also
>> updated.
>
>
> I believe that every time the Ethernet device is used, the start() op is
> called first, followed by packet transfer, followed by the stop() op. If
> start() always programs the MAC address, the driver will always end up using
> the value requested by the user.

That may be. I still don't understand the reluctance to implement it.
When the behavior of start is restructured, it will be one fewer place
to be forced to retrofit.

>>> 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.
>>
>>
>> That's what the comment I quoted in my last mail is directly
>> addressing. It does make that data valid.
>
>
> Resetting the device zeros out the MAC address value in the runtime
> registers. Perhaps we mean different things by "make the data valid", but
> resetting the device certainly doesn't load/activate any useful MAC address.

The point that comment was making was to initialize those registers in
all cases, so as to make them valid. Of course resetting undoes that.
At least some of the devices I've worked with use those registers to
communicate the MAC address from U-Boot to kernel. If that is not true
of this device, so be it.

>>> Thus SW after U-Boot won't rely on the MAC
>>> address register values,
>>
>>
>> And yet it does in most cases, hence the comment I quoted in the last
>> mail.
>
>
> This certainly isn't true for this HW at least, and I believe aside from any
> platform-specific hacks is true fairly generally across other Ethernet
> devices too. I've checked:

OK

> a) The mainline kernel's EQoS driver.
>
> b) The Synopsis-supplied EQoS driver as used in NVIDIA's downstream Tegra
> kernel.

Is this different from a) for some reason?

> In both cases, the driver retrieves the desired MAC address from sources
> other than the EQoS registers (i.e. device tree, or a system-specific
> user-space application which sets the MAC address before enabling the
> interface), and unconditionally programs that value into the EQoS runtime
> registers.
>
> I also talked to the only user of the mainline Linux EQoS driver, and he
> also is of the opinion that we can't rely on transferring the MAC address
> between U-Boot (or any FW/...) and Linux using the EQoS registers.

I think you can choose to rely on it and make it work. It's done by
others, but does not have to be if you choose to go another way.

If you were to rely on it the way others do, then you would be able to
isolate the knowledge about how to determine what the MAC is in a
single location in U-Boot and be able to share that logic between
U-Boot and Linux, since U-Boot is the only one that needs to know
about the EEPROM (for instance).

-Joe