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

[Darek Marcinkiewicz]

 On Sat, May 03, 2014 at 01:40:29PM +0200, Francois Romieu wrote:
> 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)
> 
Ops :) Fixed in next revision of the patch.
> > 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.
> 
Thank you. I am attaching 2 of thse to this repsonse - the other two 
are no longer relevant due to the changes I made into the driver.
One of the attached patches is slightly modfied by simply having one hunk
removed (that hunk was applying to the code that was removed in next rev 
of the driver). Not sure how to proceed with those patches, shall I simply
sent out these patches to this list as a separate messages?

> 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.
> 
I have switched the code to use array of descriptors. Yep, lists do
not make too much sense here.

> 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.
> 
I have changed the code to use much modest value - it is set to be of the
size of the fifo now. I think that this value is much better, but of course
having this configurable would be even better.

> 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).
> 
No, there is really no interrupt, hence the timer. Also on this device I wouldn't 
expect any bursts of data. What happens here, during regular operation of the 
device, is a periodic exchange of (few) ethernet packets between host cpu and
terminals attached to the EtherCAT bus. As for the locking on the tx path,
I have removed that completely on receiving path. I simply didn't know 
that it is such a big no-no here :)

> Did Beckhoff document their FPGA PCI interface ?
> 

Yes. It took a lot of nagging but I got some documentation from them, together
with a sample code showing how to use dma interface and generally interact with
the device.


Thank you!
-- 
DM