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

[Joe Hershberger]

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.

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

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

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

> * Structs don't translate well to interactive debugging. It's common to
> read/write registers using md/mw U-Boot shell commands. With structs, you
> need to either (a) use some other data source for the register offsets, (b)
> manually add up field sizes (c) write a comment indicating the offset next
> to each field. (a) and (b) are both annoying, and by the time you've done
> (c) (which is quite complicated in the face of ifdefs) you may as well have
> simply used #defines in the first place.

(a) is not so bad, but not ideal.  I agree that (b) is annoying. I
don't agree that you have to ifdef it, see above. I think (c) is a bit
nicer than (a), but either are fine. I think the resulting code that
is accessing the registers looks more concise and readable than a
defined constant that is going to need to contain all of the context /
scope in its name for disambiguation in the global namespace. A
register name in a struct is scoped to that block and a local variable
can be much shorter such that you don't have to keep repeating the
same text throughout the function.

>>> +/*
>>> + * Warn if the cache-line size is larger than the descriptor size. In
>>> such
>>> + * cases the driver will likely fail because the CPU needs to flush the
>>> cache
>>> + * when requeuing RX buffers, therefore descriptors written by the
>>> hardware
>>> + * may be discarded.
>>> + *
>>> + * This can be fixed by defining CONFIG_SYS_NONCACHED_MEMORY which will
>>> cause
>>> + * the driver to allocate descriptors from a pool of non-cached memory.
>>> + */
>>> +#if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN
>>> +#if !defined(CONFIG_SYS_NONCACHED_MEMORY) && \
>>> +       !defined(CONFIG_SYS_DCACHE_OFF) && !defined(CONFIG_X86)
>>
>>
>> Please include an explanation for why x86 is special.
>
>
> Sure. Do note though that this exact text already exists in other U-Boot
> drivers.

That's unfortunate. At least someone can find the explanation here
after you add it.

>>> +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?

>>> +static int eqos_calibrate_pads_tegra(struct udevice *dev)
>
>
> This function is the only part that's truly Tegra-specific, rather than
> specific to the standard IP configuration Tegra happens to use. However, I
> still believe it's entirely appropriate for this driver to include the logic
> to control these registers; they are part of the Ethernet IP block itself,
> even if NVIDIA added them as customer registers. The registers can't be
> controlled from any board/... file without very tight co-ordination with the
> driver; they aren't something that a board/... file could set up once at
> boot time in readiness for this driver to run later. The only way we could
> separate out this function from the driver would be to provide hook/callback
> functions from this driver to board logic. I don't believe that would make
> the code any cleaner. For one thing, it would spread related code across
> multiple locations making it hard to find. Another issue would be
> determining when to call the hook/callback functions for a specific device;
> they may only be applicable to a single instance of the device in a
> multi-device system. It's entirely possible someone could place an EQoS IP
> block into a PCI device, and then you'd potentially need a separate hook
> implementation for the in-SoC and the on-PCI instance. The driver can work
> that out reasonably easily itself based on how it's instantiated, but
> finding and distinguishing the different external implementations of the
> hook functions would be challenging.

OK, I'm sold. The registers are part of this IP block, then the code
should be in this driver. Thanks.

>>> +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.
>
>
> Does the network core code guarantee that start() is called before
> write_hwaddr() (so that clocks are active) and write_hwaddr() is called
> before any packets are sent/received? If so, I can move this.

start() is not called first, it's called after. But it can also be
called other times later, so it makes sense to register it even if you
call it again in start().

Maybe I need to add a flag that defers this until after start.
However, for most boards, it is undesirable to require start. From
net/eth-uclass.c:

        /*
         * Devices need to write the hwaddr even if not started so that Linux
         * will have access to the hwaddr that u-boot stored for the device.
         * This is accomplished by attempting to probe each device and calling
         * their write_hwaddr() operation.
         */

It sounds like you have a peculiar situation where you aren't starting
your peripheral clocks in SPL or some early board init, so you can't
even access registers around probe time. Is that really required?

> I note that there are other existing network drivers that set the MAC
> address in start(); see rtl8169.c for example.

I believe that the reset that is done in the rtl8169 driver clears the
MAC address, so it needs to be set on each start, but that is not that
common. The real question is why those drivers need to reset the
hardware for every network operation instead of just in probe or so.
Realistically, the network stack needs to be rearchitected to allow
multiple active MACs simultaneously. Along with that would be the
elimination of the nuclear option of resetting the IP before every
operation.

Thanks,
-Joe