Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Francois Romieu]

 Darek Marcinkiewicz <reksio@xxxxxxxxxx> :
[...]
> This driver adds support for EtherCAT master module located on CCAT
> FPGA found on Beckhoff CX series industrial PCs. The driver exposes
> EtherCAT master as an ethernet interface.
> 
> EtherCAT is a filedbus protocol defined on top of ethernet and Beckhoff

s/filedbus/fieldbus/ :o)

> CX5020 PCs come with built-in EtherCAT master module located on a FPGA,
> which in turn is connected to a PCI bus.
> 
> Signed-off-by: Dariusz Marcinkiewicz <reksio@xxxxxxxxxx>

I have attached four patches. Patch #3 / 4 contains a bugfix. Remaining
patches are small nits.

The points below are not showstoppers. Your answer will be welcome though.

If the tx_desc_free list can turn empty, I don't see much point for not
using an array as descriptor ring. It's quite common for linux ethernet
drivers.

Be it a list or an array, sizing it as the max count of descriptors that
could fit in the memory window through which packet descriptors _and_
ethernet data are copied seems overkill. It may be an upper bound though.
If you are not sure about a sane default value, ethtool API allows to
change the descriptor ring size.

Given the amount of debug messages, ethtool registers dump support could
be of some value.

I guess that the ASIC and the host CPU need some external polling to
communicate (no IRQ, really ?), whence the hrtimer. It should naturally
fit with NAPI. hrtimer could thus be disabled as soon as some ASIC event
is detected. It should save some hrtimer work when load increases and
the usual Tx lock avoidance patterns would kick in (current locking is
gross).

Did Beckhoff document their FPGA PCI interface ?

-- 
Ueimor