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

[Stephen Warren]

On 10/20/2016 12:48 PM, Simon Glass wrote:
> Hi,
>
> On 19 October 2016 at 15:14, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 10/19/2016 12:29 PM, Joe Hershberger wrote:
>>>
>>> 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:
>>>>>>>>>
>>>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren
>>>>>>>>> <swarren at wwwdotorg.org> wrote:
>>>>>>>>>>
>>>>>>>>>> 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.
>>
>> ...
>>>>>>>>>>
>>>>>>>>>> +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.
>>
>> ...
>>>>>>
>>>>>> 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.
>>
>>
>> I don't want to implement it because it can't work.
>>
>> write_hwaddr() is called before start() is called. At that point, clocks to
>> the EQoS HW are not running and the EQoS HW is in reset, and hence it cannot
>> accept any register accesses; attempting any accesses will hang the bus and
>> CPU.
>>
>> These are the possible solutions:
>>
>> a) Don't implement write_hwaddr()
>>
>> b) Make write_hwaddr() turn on the clock and clear the reset, program the
>> register, then reset the device and assert the reset. Re-asserting reset is
>> required so that setting the MAC address doesn't leave the clock running
>> even when the device isn't in use.This is pointless since the written value
>> will not last beyond the end of the function.
>>
>> c) Make probe() start the clock and clear the reset. Then write_hwaddr() can
>> successfully write the MAC address registers at any time. This would waste
>> power running the clock to the device when it's not in use. Also, Simon
>> Glass continually asks that U-Boot not initialize HW that the user hasn't
>> attempted to use. I believe turning on the clocks in probe() violates this.
>
> Not quite...or at least if I did I was mistaken. Of course we should
> limit init in probe() to what is necessary, but it is the bind()
> method which must not touch hardware.
>
> It is fine to turn clocks on in probe if you want to.

Even for a device that the user never ultimately makes use of? If so, 
this might be a reasonable solution. It feels like U-Boot should turn 
off the clocks before booting an OS though so that the overall system 
state isn't any different between the cases where this driver is present 
and enabled vs not. With the clocks manipulated by start()/stop() we 
already do this. If the clocks are enabled in probe() instead, this 
won't be the case.

> However I am wondering whether we have something wrong in the Ethernet
> uclass interface. Should we move the MAC setting to after start(), or
> similar?

That's option d right below.

Joe's objection to this is that for some hardware, downstream OSs expect 
the MAC address to be programmed into controller registers before the OS 
starts. This required write_hwaddr() to be called even if the user 
doesn't actually make use of the Ethernet device, and hence in cases 
where start()/stop() are never called. This is probably more common for 
e.g. PCI devices where the bus imposes a certain system structure 
including the ability to access device registers immediately after boot 
without manual clock programming, rather than SoC-based designs where 
all bets are off regarding consistency, and system configuration data is 
more typically passed by either out-of-band configuration data 
structures like DT, or via entirely custom mechanisms.

>> d) Modify the network core to only call write_hwaddr() between the device's
>> start() and stop() functions. I haven't looked into this at all, but I
>> imagine it will have a fairly significant impact across many parts of the
>> network core and other drivers.
>>
>>> When the behavior of start is restructured, it will be one fewer place
>>> to be forced to retrofit.
>>
>> I believe that the extra work required to refactor write_hwaddr() will be
>> miniscule compared to splitting probe(), start(), and stop() up into
>> separate functions anyway.
>>
>> ...
>>>>
>>>> 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?
>>
>>
>> Yes. Synopsis supplies a non-mainlined Linux driver for the EQoS HW. For
>> whatever reason, we're using that in our downstream kernel.
>>
>>>> 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).
>>
>>
>> The knowledge is already isolated; the mainline Linux kernel will take the
>> MAC address from device tree. This is the standard mechanism (for systems
>> that boot using DT) that works across all Ethernet devices.
>
> Regards,
> Simon
>