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

[Joe Hershberger]

On Thu, Oct 20, 2016 at 2:19 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

Maybe we should just make it a config option to select between the two
behaviors. Or is there already something that says "This system is
structured" vs "This system is a SoC / all bets are off"?

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