mbox series

[v6,0/6] CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation

Message ID cover.1603354744.git.pisa@cmp.felk.cvut.cz
Headers show
Series CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation | expand

Message

Pavel Pisa Oct. 22, 2020, 8:36 a.m. UTC
This driver adds support for the CTU CAN FD open-source IP core.
More documentation and core sources at project page
(https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core).
The core integration to Xilinx Zynq system as platform driver
is available (https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top).
Implementation on Intel FPGA based PCI Express board is available
from project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).
The CTU CAN FD core emulation send for review for QEMU mainline.
Development repository for QEMU emulation - ctu-canfd branch of
  https://gitlab.fel.cvut.cz/canbus/qemu-canbus

More about CAN bus related projects used and developed at CTU FEE
on the guidepost page http://canbus.pages.fel.cvut.cz/ .

Martin Jerabek (1):
  can: ctucanfd: add support for CTU CAN FD open-source IP core - bus
    independent part.

Pavel Pisa (5):
  dt-bindings: vendor-prefix: add prefix for the Czech Technical
    University in Prague.
  dt-bindings: net: can: binding for CTU CAN FD open-source IP core.
  can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support.
  can: ctucanfd: CTU CAN FD open-source IP core - platform/SoC support.
  docs: ctucanfd: CTU CAN FD open-source IP core documentation.

The version 6 changes:
  - sent at 2020-10-22
  - the driver has been tested with 5.9 bigendian MIPS kernel
    against QEMU CTU CAN FD model and correct behavior on PCIe
    virtual board for big-endian system passed
  - documentation updated to reflect inclusion of SocketCAN FD
    and CTU CAN FD functional model support into QEMU mainline
  - the integration for Cyclone V 5CSEMA4U23C6 based DE0-Nano-SoC
    Terasic board used for SkodaAuto research projects at our
    university has been clean up by its author (Jaroslav Beran)
    and published
    https://gitlab.fel.cvut.cz/canbus/intel-soc-ctucanfd
  - Xilinx Zynq Microzed MZ_APO based target for automatic test
    updated to Debian 10 base.

The version 5 changes:
  - sent at 2020-08-15
  - correct Kconfig formatting according to Randy Dunlap
  - silence warnings reported by make W=1 C=1 flags.
    Changes suggested by Jakub Kicinski
  - big thanks for core patch review by Pavel Machek
    resulting in more readability and formating updates
  - fix power management errors found by Pavel Machek
  - removed comments from d-t bindings as suggested by Rob Herring
  - selected ctu,ctucanfd-2 as alternative name to ctu,ctucanfd
    which allows to bind to actual major HDL core sources version 2.x
    if for some reason driver adaptation would not work on version
    read from the core
  - line length limit relaxed to 100 characters on some cases
    where it helps to readability

The version 4 changes:
  - sent at 2020-08-04
  - changes summary, 169 non-merge commits, 6 driver,
    32 IP core sources enhancements and fixes, 58 tests
    in master and about additional 30 iso-testbench
    preparation branch.
  - convert device-tree binding documentation to YAML
  - QEMU model of CTU CAN FD IP core and generic extension
    of QEMU CAN bus emulation developed by Jan Charvat.
  - driver tested on QEMU emulated Malta big-endian MIPS
    platform and big endian-support fixed.
  - checkpatch from 5.4 kernel used to cleanup driver formatting
  - header files generated from IP core IP-Xact description
    updated to include protocol exception (pex) field.
    Mechanism to set it from the driver is not provided yet.

The version 3 changes:
  - sent at 2019-12-21
  - adapts device tree bindings documentation according to
    Rob Herring suggestions.
  - the driver has been separated to individual modules for core support,
    PCI bus integration and platform, SoC integration.
  - the FPGA design has been cleaned up and CAN protocol FSM redesigned
    by Ondrej Ille (the core redesign has been reason to pause attempts to driver
    submission)
  - the work from February 2019 on core, test framework and driver
    1601 commits in total, 436 commits in the core sources, 144 commits
    in the driver, 151 documentation, 502 in tests.
  - not all continuous integration tests updated for latest design version yet
    https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/pipelines
  - Zynq hardware in the loop test show no issues for after driver PCI and platform
    separation and latest VHDL sources updates.
  - driver code has been periodically tested on 4.18.5-rt3 and 4.19 long term
    stable kernels.
  - test of the patches before submission is run on 5.4 kernel
  - the core has been integrated by Jaroslav Beran <jara.beran@gmail.com>
    into Intel FPGA based SoC used in the tester developed for Skoda auto
    at Department of Measurement, Faculty of Electrical Engineering,
    Czech Technical University https://meas.fel.cvut.cz/ . He has contributed
    feedback and fixes to the project.

The version 2 sent at 2019-02-27

The version 1 sent at 2019-02-22

Ondrej Ille has prepared the CTU CAN IP Core sources for new release.
We are waiting with it for the driver review, our intention
is to release IP when driver is reviewed and mainlined.

DKMS CTU CAN FD driver build by OpenBuildService to ease integration
into Debian systems when driver is not provided by the distribution

https://build.opensuse.org/package/show/home:ppisa/ctu_can_fd

Jan Charvat <charvj10@fel.cvut.cz> finished work to extend already
mainlined QEMU SJA1000 and SocketCAN support to provide even CAN FD
support and CTU CAN FD core support.

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/-/tree/ctu-canfd

The patches has been sent for review to QEMU mainlining list.

Thanks in advance to all who help us to deliver the project into public.

Thanks to all colleagues, reviewers and other providing feedback,
infrastructure and enthusiasm and motivation for open-source work.

Build infrastructure and hardware is provided by
  Department of Control Engineering,
  Faculty of Electrical Engineering,
  Czech Technical University in Prague
  https://dce.fel.cvut.cz/en

 .../bindings/net/can/ctu,ctucanfd.yaml        |   63 +
 .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
 .../ctu/FSM_TXT_Buffer_user.png               |  Bin 0 -> 174807 bytes
 .../device_drivers/ctu/ctucanfd-driver.rst    |  638 ++++++++++
 drivers/net/can/Kconfig                       |    1 +
 drivers/net/can/Makefile                      |    1 +
 drivers/net/can/ctucanfd/Kconfig              |   35 +
 drivers/net/can/ctucanfd/Makefile             |   13 +
 drivers/net/can/ctucanfd/ctu_can_fd.c         | 1105 +++++++++++++++++
 drivers/net/can/ctucanfd/ctu_can_fd.h         |   87 ++
 drivers/net/can/ctucanfd/ctu_can_fd_frame.h   |  189 +++
 drivers/net/can/ctucanfd/ctu_can_fd_hw.c      |  790 ++++++++++++
 drivers/net/can/ctucanfd/ctu_can_fd_hw.h      |  916 ++++++++++++++
 drivers/net/can/ctucanfd/ctu_can_fd_pci.c     |  314 +++++
 .../net/can/ctucanfd/ctu_can_fd_platform.c    |  142 +++
 drivers/net/can/ctucanfd/ctu_can_fd_regs.h    |  971 +++++++++++++++
 16 files changed, 5267 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
 create mode 100644 Documentation/networking/device_drivers/ctu/FSM_TXT_Buffer_user.png
 create mode 100644 Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst
 create mode 100644 drivers/net/can/ctucanfd/Kconfig
 create mode 100644 drivers/net/can/ctucanfd/Makefile
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd.h
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_frame.h
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_hw.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_hw.h
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_pci.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_platform.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_regs.h

Comments

Pavel Machek Oct. 22, 2020, 8:58 a.m. UTC | #1
On Thu 2020-10-22 10:36:17, Pavel Pisa wrote:
> The device-tree bindings for open-source/open-hardware CAN FD IP core
> designed at the Czech Technical University in Prague.
> 
> CTU CAN FD IP core and other CTU CAN bus related projects
> listing and documentation page
> 
>    http://canbus.pages.fel.cvut.cz/
> 
> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> Reviewed-by: Rob Herring <robh@kernel.org>

1,2: Acked-by: Pavel Machek <pavel@ucw.cz>
Pavel Machek Oct. 22, 2020, 11:25 a.m. UTC | #2
On Thu 2020-10-22 10:36:21, Pavel Pisa wrote:
> CTU CAN FD IP core documentation based on Martin Jeřábek's diploma theses
> Open-source and Open-hardware CAN FD Protocol Support
> https://dspace.cvut.cz/handle/10467/80366
> .

> ---
>  .../ctu/FSM_TXT_Buffer_user.png               | Bin 0 -> 174807 bytes

Maybe picture should stay on website, somewhere. It is rather big for
kernel sources.

> +About SocketCAN
> +---------------
> +
> +SocketCAN is a standard common interface for CAN devices in the Linux
> +kernel. As the name suggests, the bus is accessed via sockets, similarly
> +to common network devices. The reasoning behind this is in depth
> +described in `Linux SocketCAN <https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/can.rst>`_.
> +In short, it offers a
> +natural way to implement and work with higher layer protocols over CAN,
> +in the same way as, e.g., UDP/IP over Ethernet.

Drop? Or at least link directly to the file in kernel tree?

> +Device probe
> +~~~~~~~~~~~~
> +
> +Before going into detail about the structure of a CAN bus device driver,
> +let's reiterate how the kernel gets to know about the device at all.
> +Some buses, like PCI or PCIe, support device enumeration. That is, when
> +the system boots, it discovers all the devices on the bus and reads
> +their configuration. The kernel identifies the device via its vendor ID
> +and device ID, and if there is a driver registered for this identifier
> +combination, its probe method is invoked to populate the driver's
> +instance for the given hardware. A similar situation goes with USB, only
> +it allows for device hot-plug.
> +
> +The situation is different for peripherals which are directly embedded
> +in the SoC and connected to an internal system bus (AXI, APB, Avalon,
> +and others). These buses do not support enumeration, and thus the kernel
> +has to learn about the devices from elsewhere. This is exactly what the
> +Device Tree was made for.

Dunno. Is it suitable? This is supposed to be ctu-can documentation,
not "how hardware works" docs.

> +Platform device driver
> +^^^^^^^^^^^^^^^^^^^^^^
> +
> +In the case of Zynq, the core is connected via the AXI system bus, which
> +does not have enumeration support, and the device must be specified in
> +Device Tree. This kind of devices is called *platform device* in the
> +kernel and is handled by a *platform device driver*\  [1]_.
> +
> +A platform device driver provides the following things:
> +
> +-  A *probe* function
> +
> +-  A *remove* function
> +
> +-  A table of *compatible* devices that the driver can handle
> +
> +The *probe* function is called exactly once when the device appears (or
> +the driver is loaded, whichever happens later). If there are more
> +devices handled by the same driver, the *probe* function is called for
> +each one of them. Its role is to allocate and initialize resources
> +required for handling the device, as well as set up low-level functions
> +for the platform-independent layer, e.g., *read_reg* and *write_reg*.
> +After that, the driver registers the device to a higher layer, in our
> +case as a *network device*.
> +
> +The *remove* function is called when the device disappears, or the
> +driver is about to be unloaded. It serves to free the resources
> +allocated in *probe* and to unregister the device from higher layers.
> +
> +Finally, the table of *compatible* devices states which devices the
> +driver can handle. The Device Tree entry ``compatible`` is matched
> +against the tables of all *platform drivers*.

And this is "how to write a kernel driver" documentation. Like, why
not, but maybe it does not need to be in kernel tree, and certainly
should be separate from real "what is ctucan and how to use it" docs.

Best regards,
								Pavel
Pavel Machek Oct. 22, 2020, 11:43 a.m. UTC | #3
Hi!

> +++ b/drivers/net/can/ctucanfd/Kconfig
> @@ -21,4 +21,15 @@ config CAN_CTUCANFD_PCI
>  	  PCIe board with PiKRON.com designed transceiver riser shield is available
>  	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
>  
> +config CAN_CTUCANFD_PLATFORM
> +	tristate "CTU CAN-FD IP core platform (FPGA, SoC) driver"
> +	depends on OF || COMPILE_TEST
> +	help

This is likely wrong, as it can enable config of CAN_CTUCANFD=M,
CAN_CTUCANFD_PLATFORM=y, right?

> @@ -8,3 +8,6 @@ ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o
>  
>  obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o
>  ctucanfd_pci-y := ctu_can_fd_pci.o
> +
> +obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o
> +ctucanfd_platform-y += ctu_can_fd_platform.o

Can you simply add right object files directly?

Best regards,
							Pavel
Pavel Pisa Oct. 22, 2020, 3:24 p.m. UTC | #4
Hello Pavel,

thanks for review.

As for the documentation, my current intention is to keep/maintain
the common driver documentation for CTU CAN FD site
and kernel source. The standalone driver documentation

  http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/driver_doc/ctucanfd-driver.html

when the driver documentation moves to

  https://www.kernel.org/doc/html/latest/

we may consider to drop standalone. But resource are limited
and keeping and maintaining sync between slightly different
files is error prone. I run manual kdiff3 updates
of both RST files because references to sources are different.

On Thursday 22 of October 2020 13:25:40 Pavel Machek wrote:
> On Thu 2020-10-22 10:36:21, Pavel Pisa wrote:
> > CTU CAN FD IP core documentation based on Martin Jeřábek's diploma theses
> > Open-source and Open-hardware CAN FD Protocol Support
> > https://dspace.cvut.cz/handle/10467/80366
> > .
> >
> > ---
> >  .../ctu/FSM_TXT_Buffer_user.png               | Bin 0 -> 174807 bytes
>
> Maybe picture should stay on website, somewhere. It is rather big for
> kernel sources.

My sense is that it is proffered that documentation is self-contained
without embedded references to "untrusted" third party sites.
But we try to do something with it. I try reduce size or switch
to SVG, our actual source is PDF prepared by Ondrej Ille
as part of CTU CAN FD IP core architecture. I probably redraw
image in inscape with little worse graphics style, fonts,
smoothness etc. but in smaller and simpler SVG file size format.
I expect that use of original PDF in vector form would not help much.

> > +About SocketCAN
> > +---------------
> > +
> > +SocketCAN is a standard common interface for CAN devices in the Linux
> > +kernel. As the name suggests, the bus is accessed via sockets, similarly
> > +to common network devices. The reasoning behind this is in depth
> > +described in `Linux SocketCAN
> > <https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Doc
> >umentation/networking/can.rst>`_. +In short, it offers a
> > +natural way to implement and work with higher layer protocols over CAN,
> > +in the same way as, e.g., UDP/IP over Ethernet.
>
> Drop? Or at least link directly to the file in kernel tree?

Yes, this is another place where we need other diversion
between standalone and kernel. I try to learn what is the right
way to cross-reference Linux kernel manuals writtent n RST? 
If you can speedup it by hint/right done example I would be happy.

> > +Device probe
> > +~~~~~~~~~~~~
> > +
> > +Before going into detail about the structure of a CAN bus device driver,
> > +let's reiterate how the kernel gets to know about the device at all.
> > +Some buses, like PCI or PCIe, support device enumeration. That is, when
> > +the system boots, it discovers all the devices on the bus and reads
> > +their configuration. The kernel identifies the device via its vendor ID
> > +and device ID, and if there is a driver registered for this identifier
> > +combination, its probe method is invoked to populate the driver's
> > +instance for the given hardware. A similar situation goes with USB, only
> > +it allows for device hot-plug.
> > +
> > +The situation is different for peripherals which are directly embedded
> > +in the SoC and connected to an internal system bus (AXI, APB, Avalon,
> > +and others). These buses do not support enumeration, and thus the kernel
> > +has to learn about the devices from elsewhere. This is exactly what the
> > +Device Tree was made for.
>
> Dunno. Is it suitable? This is supposed to be ctu-can documentation,
> not "how hardware works" docs.

I think that this text is fully valid for standalone driver documentation,
I understand that in the kernel tree this can be replaced by references
to right places if we locate them.

> > +Platform device driver
> > +^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +In the case of Zynq, the core is connected via the AXI system bus, which
> > +does not have enumeration support, and the device must be specified in
> > +Device Tree. This kind of devices is called *platform device* in the
> > +kernel and is handled by a *platform device driver*\  [1]_.
> > +
> > +A platform device driver provides the following things:
> > +
> > +-  A *probe* function
> > +
> > +-  A *remove* function
> > +
> > +-  A table of *compatible* devices that the driver can handle
> > +
> > +The *probe* function is called exactly once when the device appears (or
> > +the driver is loaded, whichever happens later). If there are more
> > +devices handled by the same driver, the *probe* function is called for
> > +each one of them. Its role is to allocate and initialize resources
> > +required for handling the device, as well as set up low-level functions
> > +for the platform-independent layer, e.g., *read_reg* and *write_reg*.
> > +After that, the driver registers the device to a higher layer, in our
> > +case as a *network device*.
> > +
> > +The *remove* function is called when the device disappears, or the
> > +driver is about to be unloaded. It serves to free the resources
> > +allocated in *probe* and to unregister the device from higher layers.
> > +
> > +Finally, the table of *compatible* devices states which devices the
> > +driver can handle. The Device Tree entry ``compatible`` is matched
> > +against the tables of all *platform drivers*.
>
> And this is "how to write a kernel driver" documentation. Like, why
> not, but maybe it does not need to be in kernel tree, and certainly
> should be separate from real "what is ctucan and how to use it" docs.

I agree, we try to find way how to separate the things and reference
pieces already found in the kernel. But it probably means massive
diversion of documentation sources which is yet another additional
load during preparation for mainlining. So I probably drop updates
of standalone documentation. Which can be problem for another university
group which builds applications based on the core and needs
to maintain and patch kernels now.

Best wishes,

                Pavel
--
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://dce.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
Pavel Pisa Oct. 22, 2020, 4:06 p.m. UTC | #5
Hello Pavel,

thanks for review.

On Thursday 22 of October 2020 13:43:06 Pavel Machek wrote:
> Hi!
>
> > +++ b/drivers/net/can/ctucanfd/Kconfig
> > @@ -21,4 +21,15 @@ config CAN_CTUCANFD_PCI
> >  	  PCIe board with PiKRON.com designed transceiver riser shield is
> > available at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
> >
> > +config CAN_CTUCANFD_PLATFORM
> > +	tristate "CTU CAN-FD IP core platform (FPGA, SoC) driver"
> > +	depends on OF || COMPILE_TEST
> > +	help
>
> This is likely wrong, as it can enable config of CAN_CTUCANFD=M,
> CAN_CTUCANFD_PLATFORM=y, right?

My original code has not || COMPILE_TEST alternative.

But I have been asked to add it

On Sunday 16 of August 2020 01:28:13 Randy Dunlap wrote:
> Can this be
>         depends on OF || COMPILE_TEST
> ?

I have send discussion later that I am not sure if it is right
but followed suggestion. If there is no other reply now,
I would drop || COMPILE_TEST. I believe that then it is correct
for regular use. I ma not sure about all consequences of COMPILE_TEST
missing.

> > @@ -8,3 +8,6 @@ ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o
> >
> >  obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o
> >  ctucanfd_pci-y := ctu_can_fd_pci.o
> > +
> > +obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o
> > +ctucanfd_platform-y += ctu_can_fd_platform.o
>
> Can you simply add right object files directly?

This is more tough question. We have kept sources
as ctu_can_fd.c, ctu_can_fd_hw.c etc. to produce
final ctucanfd.ko which matches device tree entry etc.
after name simplification now...
So we move from underscores to ctucanfd on more places.
So yes, we can rename ctu_can_fd.c to ctucanfd_drv.c + others
keep final ctucanfd.ko and change to single file based objects
ctucanfd_platform.c and ctucanfd_pci.c

If you think that it worth to be redone, I would do that.
It would disrupt sources history, may it be blames, merging
etc... but I would invest effort into it if asked for. 

Best wishes,

                Pavel
Pavel Machek Oct. 22, 2020, 8:50 p.m. UTC | #6
Hi!

> > > +++ b/drivers/net/can/ctucanfd/Kconfig
> > > @@ -21,4 +21,15 @@ config CAN_CTUCANFD_PCI
> > >  	  PCIe board with PiKRON.com designed transceiver riser shield is
> > > available at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
> > >
> > > +config CAN_CTUCANFD_PLATFORM
> > > +	tristate "CTU CAN-FD IP core platform (FPGA, SoC) driver"
> > > +	depends on OF || COMPILE_TEST
> > > +	help
> >
> > This is likely wrong, as it can enable config of CAN_CTUCANFD=M,
> > CAN_CTUCANFD_PLATFORM=y, right?
> 
> My original code has not || COMPILE_TEST alternative.
> 
> But I have been asked to add it
> 
> On Sunday 16 of August 2020 01:28:13 Randy Dunlap wrote:
> > Can this be
> >         depends on OF || COMPILE_TEST
> > ?
> 
> I have send discussion later that I am not sure if it is right
> but followed suggestion. If there is no other reply now,
> I would drop || COMPILE_TEST. I believe that then it is correct
> for regular use. I ma not sure about all consequences of COMPILE_TEST
> missing.

COMPILE_TEST is not a problem. But you need to make this depend on
main CONFIG_ option to disallow CAN_CTUCANFD=M,
CAN_CTUCANFD_PLATFORM=y combination.

> > > @@ -8,3 +8,6 @@ ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o
> > >
> > >  obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o
> > >  ctucanfd_pci-y := ctu_can_fd_pci.o
> > > +
> > > +obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o
> > > +ctucanfd_platform-y += ctu_can_fd_platform.o
> >
> > Can you simply add right object files directly?
> 
> This is more tough question. We have kept sources
> as ctu_can_fd.c, ctu_can_fd_hw.c etc. to produce
> final ctucanfd.ko which matches device tree entry etc.
> after name simplification now...
> So we move from underscores to ctucanfd on more places.
> So yes, we can rename ctu_can_fd.c to ctucanfd_drv.c + others
> keep final ctucanfd.ko and change to single file based objects
> ctucanfd_platform.c and ctucanfd_pci.c
> 
> If you think that it worth to be redone, I would do that.
> It would disrupt sources history, may it be blames, merging
> etc... but I would invest effort into it if asked for.

git can handle renames. Or you can use the new names for module
names...?

Best regards,
							Pavel
Pavel Pisa Oct. 26, 2020, 4:11 p.m. UTC | #7
Hello Ondrej and others,

On Monday 26 of October 2020 14:38:59 Ondrej Ille wrote:
> Hello Pavel and Pavel,
>
> first of all, Pavel (Machek) thank you for review, we appreciate it.
> We will try to fix as much mistakes as possible. Please, see my comments
> below.
>
> With Regards
> Ondrej
>
> On Thu, Oct 22, 2020 at 10:22 PM Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
...
> > > > +/**
> > > > + * ctucan_start_xmit - Starts the transmission
> > > > + * @skb:   sk_buff pointer that contains data to be Txed
> > > > + * @ndev:  Pointer to net_device structure
> > > > + *
> > > > + * This function is invoked from upper layers to initiate
> >
> > transmission.
> >
> > > > This + * function uses the next available free txbuf and populates
> >
> > their
> >
> > > > fields to + * start the transmission.
> > > > + *
> > > > + * Return: %NETDEV_TX_OK on success and failure value on error
> > > > + */
> > >
> > > Based on other documentation, I'd expect this to return -ESOMETHING on
> > > error, but it returns NETDEV_TX_BUSY.
> >
> > I add information about explicit error/need for postpone type.
>
> Changing description, OK. Pavel Pisa, but why did you change handling of
> insertion
> failure to TXT Buffer to return NETDEV_TX_BUSY and increment tx_dropped?
> Is there some preference on what should the driver return in case of HW
> error?
> Also, couldnt we afford not to check return value of ctucan_hw_insert_frame
> ? Is purpose of
> driver to be fail-safe against HW bug which says "There is TX buffer free
> in Status register", but in reality,
> no TXT Buffer is free?
>
> If we look at when ctu_can_hw_insert_frame returns false, it is when:
>   1. We attempt to insert to non-existent TXT buffer -> Under drivers
> control to do rotation correctly.
>   2. If cfg->len > CAN_FD_MAX_LEN. Couldnt this check be removed?
> CAN_FD_MAX_LEN is
>       defined for Linux, so it is not OS agnostic... Also, is it possible
> that driver will call insert with
>       cf->len > CAN_FD_MAX_LEN?
>   3. When there is HW bug (as mentioned earlier). There are assertions in
> RTL checking this situation
>       will not happend!
> So maybe we dont even need to check return value of this function at all?

I try to follow other drivers.
So if everything is OK then return NETDEV_TX_OK.

If there is no Tx buffer available then return
NETDEV_TX_BUSY. Some retransmit or drop should be handled by
NET core in such case. This situation should not appear
in reality, because Tx queue should be stopped if there is no
free Tx buffer and should not be reenable earlien than
at least one is available. So this situation is bug in
driver logic or NET core.

If the check for CAN FD frame format fails then it is right
to drop SKB and it is handled with NETDEV_TX_OK return
in other drivers as well. Only statistic counter increments.
If the Tx buffer selected by driver s in incorrect state
then it is even more serious bug so alternative is to
stop whole driver and report fatal error.

> > > > +   /* Check for Bus Error interrupt */
> > > > +   if (isr.s.bei) {
> > > > +           netdev_info(ndev, "  bus error");
> > >
> > > Missing "if (dologerr)" here?
> >
> > Intention was to left this one to appear without rate limit, it is really
> > indication of bigger problem. But on the other hand without dologerr
> > would be quite short/unclear, but does not overflow the log buffers...
> > We would discuss what to do with my colleagues, suggestions welcomed.
>
> I agree with adding "dologerror" check here. It is true that arbitration
> lost is not really an
> error, and Bus error (error frame), therefore Bus error has higher
> "severity". Could we
> maybe do it that both have "dologerr" condition, but arbitration lost uses
> "netdev_dbg"?

Arbitration lost should not be reported nor generate interrupt
for usual can application setup.

> > > > +static int ctucan_rx_poll(struct napi_struct *napi, int quota)
> > > > +{
> > > > +   struct net_device *ndev = napi->dev;
> > > > +   struct ctucan_priv *priv = netdev_priv(ndev);
> > > > +   int work_done = 0;
> > > > +   union ctu_can_fd_status status;
> > > > +   u32 framecnt;
> > > > +
> > > > +   framecnt = ctucan_hw_get_rx_frame_count(&priv->p);
> > > > +   netdev_dbg(ndev, "rx_poll: %d frames in RX FIFO", framecnt);
> > >
> > > This will be rather noisy, right?
> >
> > It has use to debug during development but may be it should be removed
> > or controlled by other option.
>
> Maybe again suppress by "net_ratelimit" ?

I have removed this one and report only errors.

...
> > https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/blob/master/spec/CTU
> >/ip/CAN_FD_IP_Core/2.1/CAN_FD_IP_Core.2.1.xml
> >
> > Which I consider as good option which should be preserved.
> > I prefer to have only singe source of infomation
> > which is kept with rest in automatic sync.
>
> We are really trying only to use bitfields generated in ctu_can_fd_regs.
> Whether it is bitfield or
> mask, is up to debate, but we always use generated values. Linux driver is
> only one part of it
> all. The golden source (IP-XACT) is propagated to RTL, 2 different TB
> packages, constant definitions,
> generated documentation. This is the only way how to keep register map
> consistent with limited
> developer resources we have. If we corrupt this rule, we end up with 4
> different representations
> of register maps:
>    1. What Testebench thinks
>    2. What is in RTL
>    3. What is in documentation
>    4. What driver sees.
> and then we will never put it back together again...

I think that it is even important example for others. And there is not listed
use of the generated header files for functional emulation in QEMU.
I have even plan to use generator to prepare RO/RW mask for QEMU 
and IP block emulation skeleton for QEMU automatically. This part
has not been implemented during last bachelor thesis. That part
was written manually but it is in QEMU mainline now.

If there is strong preferences for macros than bitfields
we add macros generation alternative.

> > > > +   {
> > > > +           union ctu_can_fd_int_stat imask;
> > > > +
> > > > +           imask.u32 = 0xffffffff;
> > > > +           ctucan_hw_int_ena_clr(&priv->p, imask);
> > > > +           ctucan_hw_int_mask_set(&priv->p, imask);
> > > > +   }
> > >
> > > More like this. Plus avoid block here...?
> >
> > Blocks is to document that imask is really local for these
> > two lines, no need to look for it elsewhere in the function.
> > But I can move declaration to start of the function.
>
> I would also remove blocks here.

Flattened

> > > > +/**
> > > > + * ctucan_close - Driver close routine
> > > > + * @ndev:  Pointer to net_device structure
> > > > + *
> > > > + * Return: 0 always
> > > > + */
> > >
> > > You see, this is better. No need to say "Driver close routine"
> > > twice. Now, make the rest consistent :-).
> > >
> > > > +EXPORT_SYMBOL(ctucan_suspend);
> > > > +EXPORT_SYMBOL(ctucan_resume);
> > >
> > > _GPL?
> >
> > Should we be so strict??? Ondrej Ille can provide his opinion there.
>
> Is it really necessary? If yes, then we can change it.

I see no reason that it is necessary. Without GPL some vendor can come
with CTU CAN FD integration on such platform, where integration
code stays unavailable to users. But common driver part has to be made 
available. We can prevent such use of the driver code, but I see
no big wind there or financial income instrument.

...

> > Hmmm, we can add special rules to tools to skip some special cases
> > but actual files exactly math what is in documentation and VHDL
> > sources and registers implementation. See page 61 / PDF 67 of
> >
> >   http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf
> >
> > Yes there is still space for improvements but we need to have
> > acceptable base for already running applications.
>
> Point taken. These are indeed ridiculous. I was thinking about adding some
> skip rule
> to the register map generator, but I am out of IP-XACT fields to represent
> this thing.
> Maybe I can use vendor extensions do to hide it? Or custom switch? Anyway,
> the
> same thing needs to be resolved if HW design has dedicated test registers
> which
> are for debug. I will think about some solution.

I personally prefer rules without exception. Defined 32-bit fields over
whole register are harmless and if narrowed or reorganized later then
in can help.

....

> > OK, I have invested lot of time after Marin Jerabek's submission of
> > diploma theses to make code really documented etc.. I add there something
> > even that it is really simple use of can_len2dlc. May it be, we can use
> > that directly. It is Linux specific, but clean.
>
> Using can_len2dlc seems as right option for me...

Done

> > > > +// TODO: AL_CAPTURE and ERROR_CAPTURE
> >
> > Again colleagues remark for future work. For me, it is important
> > basic function under GNU/Linux.
>
> Again, CAN be removed and moved to Issue tracker on Gitlab.

Please add issue, I have removed this one already.

> It is basically the same topic as above. We need to generate everything
> from single
> source, otherwise we are not able to keep all the targets (RTL, TB, driver,
> docs)
> consistent.

Best wishes,

Pavel