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

[Stephen Warren]

On 10/11/2016 05:24 PM, Joe Hershberger wrote:
> Hi Stephen,
>
> On Tue, Sep 27, 2016 at 12:02 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
>>
>>
>>>> +/* Core registers */
>>>
>>> Why are registers not defined as a struct? That is the common way that
>>> is accepted to represent registers in a driver.
>>
>>
>> It is common, but I find using structs has significant disadvantages, which
>> outweigh any advantages, so I strongly prefer not to use them.
>>
>> Disadvantages of structs:
>>
>> * Drivers often have to deal with different but similar HW variants. Similar
>> enough that it makes sense to use the same driver for them, but different
>> enough that certain registers are placed differently in the memory map. With
>> structs, there's little choice but to use ifdefs to pick a particular HW
>> variant and bake that into the driver. This doesn't work well for drivers
>> that must pick the variant at run-time rather than compile-time, e.g.
>> drivers for USB or PCIe devices, or when you wish to build a single SW image
>> that can run on multiple SoCs.
>
> Is that really the case here or are you just making a broad statement?
> Are registers really moving based on IP configuration? That sounds
> like broken IP / IP generator.

I think we should choose the best techniques and use them globally, even 
if a particular disadvantage of a non-optimal technique isn't relevant 
to a particular device. Thus, I don't think whether I'm making a broad 
statement is too relevant.

I know there are multiple versions of this HW block in existence. I 
don't know the details of the register compatibility between the 
different versions for sure.

There are many broken IPs and IP generators in existence, and I believe 
we need to accomodate that.

>> A common variant on this theme is device with different register layouts due
>> to IP integration issues, such as a UART with byte-wide registers that
>> appear at consecutive byte offsets in some SoCs, but at separate word
>> addresses in other SoCs.
>
> Again, this sounds like a generic argument that doesn't apply here.

I don't believe the distinction is appropriate.

>> This issue is relevant here since the EQoS block is a generic IP block with
>> many options that may have different SoC- or IP-version-specific differences
>> between SoCs.
>
> But simply additive, no? Included features add registers or bitfields.

As far as I know, NVIDIA's register additions to this particular version 
of this particular HW block are purely additions in a well segregated 
register block.

This certainly isn't true for other HW blocks in Tegra though (e.g. 
consider the Synopsys USB2 controller Tegra uses), and I'm sure the same 
applies to many other HW blocks in many other SoCs. I wouldn't be at all 
surprised if some SoC vendor does the same thing to this IP block. 
There's no way to tell until someone appears to integrate driver support 
into U-Boot.

>> * The register space for the EQoS driver is quite sparse, and U-Boot uses a
>> subset of the registers effectively making it even more sparse. Defining a
>> struct to describe this will be a frustrating exercise in calculating the
>> right amount of padding to place into the struct to get the correct offsets.
>> Mistakes will be made and it will be annoying.
>
> It's also organized into a few blocks. It's preferable to group
> registers into a struct that represents each block instead of one huge
> struct.

That will certainly help.

> It also hurts nothing to have registers defined in the struct
> that apply to configurations that may not be enabled in a given
> instance... the switching can be done at the spot where the register
> is accessed. And the accesses don't have to be compile-time options so
> you can have your built-in and your PCIe variants.

So long as there aren't conflicting register definitions, which is 
certainly not guaranteed in any way, what you say is true.

Anyway, I'll rewrite the driver using structs for now in the interests 
of moving it forward. If this causes issue down the road, whomever is 
adding the support for additional devices can worry about how to solve 
any issues that arise then.

>>>> +static int eqos_start_clks_tegra(struct udevice *dev)
>>>
>>> Why is this not in a board file and a separate patch?
>>
>> The clocks (and other resources) that this driver operates on are directly
>> related to the EQoS IP block itself. Hence, it's completely appropriate for
>> this driver to deal with them.
>>
>> Perhaps the name "tegra" is slightly misleading; it refers to "the
>> configuration of the core EQoS block that Tegra happens to have chosen"
>> rather than "something entirely Tegra-specific that is unrelated to this
>> device/driver". However, I'm not sure what name would be better than "tegra"
>> here; the correct name is something like
>> axi_ahb_dma_mtl_core_single_phy_rgmii, but that's far too long for a symbol
>> name. I attempted to describe this in the comment regarding IP configuration
>> at the beginning of the file. Is there a specific shortcoming in that
>> comment that could be changed to address this better?
>
> I think the comment should list the configuration with at least the
> level of detail that you used to create that long symbol name.
>
> Do you anticipate that this set of configurations will with high
> confidence be used in Tegra going forward? Or would it be better to
> call it tegra186?

It's too early to tell. I hope we'll maintain the same configuration in 
any future SoCs, but who knows.

I can rename it all to tegra186 if you want; we can simply call the 
tegra186 functions when running on any future SoCs that are 
backwards-compatible, which would be conceptually similar to the way 
Device Tree compatible values are extended.