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.
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.
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.
* 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.
* 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.
>> +/*
>> + * 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.
>> +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?
>> +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.
>> +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.
I note that there are other existing network drivers that set the MAC
address in start(); see rtl8169.c for example.
>> +int eqos_send(struct udevice *dev, void *packet, int length)
>> +{
>> + struct eqos_priv *eqos = dev_get_priv(dev);
>> + u32 *tx_desc;
>
> Please make this a struct.
IIRC I'd avoided that because there are a variety of different formats
based on the type, which will involve a mess of unions. I'll take a look
at the details and convert them if it doesn't turn out massively
horrible; they're small enough that it sounds reasonable to do so on the
surface.