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

[Stephen Warren]

On 10/20/2016 01:30 PM, Joe Hershberger wrote:
> 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"?

Perhaps we should make a run-time decision rather than compile-time. 
This will allow a single system to contain multiple instances of the 
EQoS HW block with different behaviour, e.g. 1 embedded in the SoC, and 
1 plug-in PCIe device.

We can assume that for any PCIe device, we can implement write_hwaddr() 
without issue.

For devices instantiated from DT, the SoC-level integration details will 
drive whether there is SW control over the HW block's clock/reset 
signals. We can use struct udevice_id's .data field to provide 
parameters for each different DT compatible value that the driver 
supports, which can indicate how write_hwaddr() should work.

Then, we can implement roughly the following:

write_hwaddr:
     if (!dev->reg_access_ok && !dev->reg_access_always_ok)
         return 0;
     write MAC address registers

That should also allow write_hwaddr to operate correctly if 
start()/stop() are drivers are refactored not to start()/stop() for 
every separate network transaction, since presumably the code that sets 
reg_access_ok will be refactored as part of that.