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

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

[Pavel Pisa]

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

-- 
2.20.1

[PATCH v6 1/6] dt-bindings: vendor-prefix: add prefix for the Czech Technical University in Prague.

[Pavel Pisa]

The Czech Technical University in Prague (CTU) is one of
the biggest and oldest (founded 1707) technical universities
in Europe. The abbreviation in Czech language is ČVUT according
to official name in Czech language

  České vysoké učení technické v Praze

The English translation

  The Czech Technical University in Prague

The university pages in English

  https://www.cvut.cz/en

Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 967e78c5ec0a..dedb10f1b250 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -215,6 +215,8 @@ patternProperties:
     description: Hangzhou C-SKY Microsystems Co., Ltd
   "^csq,.*":
     description: Shenzen Chuangsiqi Technology Co.,Ltd.
+  "^ctu,.*":
+    description: Czech Technical University in Prague
   "^cubietech,.*":
     description: Cubietech, Ltd.
   "^cypress,.*":
-- 
2.20.1

[PATCH v6 2/6] dt-bindings: net: can: binding for CTU CAN FD open-source IP core.

[Pavel Pisa]

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>
---
 .../bindings/net/can/ctu,ctucanfd.yaml        | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml

diff --git a/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
new file mode 100644
index 000000000000..5113bb419ec1
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/ctu,ctucanfd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CTU CAN FD Open-source IP Core Device Tree Bindings
+
+description: |
+  Open-source CAN FD IP core developed at the Czech Technical University in Prague
+
+  The core sources and documentation on project page
+    [1] sources : https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core
+    [2] datasheet : https://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf
+
+  Integration in Xilinx Zynq SoC based system together with
+  OpenCores SJA1000 compatible controllers
+    [3] project : https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top
+  Martin Jerabek dimploma thesis with integration and testing
+  framework description
+    [4] PDF : https://dspace.cvut.cz/bitstream/handle/10467/80366/F3-DP-2019-Jerabek-Martin-Jerabek-thesis-2019-canfd.pdf
+
+maintainers:
+  - Pavel Pisa <pisa@cmp.felk.cvut.cz>
+  - Ondrej Ille <ondrej.ille@gmail.com>
+  - Martin Jerabek <martin.jerabek01@gmail.com>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: ctu,ctucanfd-2
+          - const: ctu,ctucanfd
+      - const: ctu,ctucanfd
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description: |
+      phandle of reference clock (100 MHz is appropriate
+      for FPGA implementation on Zynq-7000 system).
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    ctu_can_fd_0: can@43c30000 {
+      compatible = "ctu,ctucanfd";
+      interrupts = <0 30 4>;
+      clocks = <&clkc 15>;
+      reg = <0x43c30000 0x10000>;
+    };
-- 
2.20.1

[PATCH v6 4/6] can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support.

[Pavel Pisa]

PCI bus adaptation for CTU CAN FD open-source IP core.

The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
PCIe board with PiKRON.com designed transceiver riser shield is available
at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .

Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Signed-off-by: Martin Jerabek <martin.jerabek01@gmail.com>
Signed-off-by: Ondrej Ille <ondrej.ille@gmail.com>
---
 drivers/net/can/ctucanfd/Kconfig          |   9 +
 drivers/net/can/ctucanfd/Makefile         |   3 +
 drivers/net/can/ctucanfd/ctu_can_fd_pci.c | 314 ++++++++++++++++++++++
 3 files changed, 326 insertions(+)
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_pci.c

diff --git a/drivers/net/can/ctucanfd/Kconfig b/drivers/net/can/ctucanfd/Kconfig
index b6d47bba7150..fb4d3cda6885 100644
--- a/drivers/net/can/ctucanfd/Kconfig
+++ b/drivers/net/can/ctucanfd/Kconfig
@@ -12,4 +12,13 @@ config CAN_CTUCANFD
 
 if CAN_CTUCANFD
 
+config CAN_CTUCANFD_PCI
+	tristate "CTU CAN-FD IP core PCI/PCIe driver"
+	depends on PCI
+	help
+	  This driver adds PCI/PCIe support for CTU CAN-FD IP core.
+	  The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
+	  PCIe board with PiKRON.com designed transceiver riser shield is available
+	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
+
 endif
diff --git a/drivers/net/can/ctucanfd/Makefile b/drivers/net/can/ctucanfd/Makefile
index 8d47008d0076..eb945260952d 100644
--- a/drivers/net/can/ctucanfd/Makefile
+++ b/drivers/net/can/ctucanfd/Makefile
@@ -5,3 +5,6 @@
 
 obj-$(CONFIG_CAN_CTUCANFD) := ctucanfd.o
 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
diff --git a/drivers/net/can/ctucanfd/ctu_can_fd_pci.c b/drivers/net/can/ctucanfd/ctu_can_fd_pci.c
new file mode 100644
index 000000000000..c4542eac2747
--- /dev/null
+++ b/drivers/net/can/ctucanfd/ctu_can_fd_pci.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*******************************************************************************
+ *
+ * CTU CAN FD IP Core
+ *
+ * Copyright (C) 2015-2018 Ondrej Ille <ondrej.ille@gmail.com> FEE CTU
+ * Copyright (C) 2018-2020 Ondrej Ille <ondrej.ille@gmail.com> self-funded
+ * Copyright (C) 2018-2019 Martin Jerabek <martin.jerabek01@gmail.com> FEE CTU
+ * Copyright (C) 2018-2020 Pavel Pisa <pisa@cmp.felk.cvut.cz> FEE CTU/self-funded
+ *
+ * Project advisors:
+ *     Jiri Novak <jnovak@fel.cvut.cz>
+ *     Pavel Pisa <pisa@cmp.felk.cvut.cz>
+ *
+ * Department of Measurement         (http://meas.fel.cvut.cz/)
+ * Faculty of Electrical Engineering (http://www.fel.cvut.cz)
+ * Czech Technical University        (http://www.cvut.cz/)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ ******************************************************************************/
+
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#include "ctu_can_fd.h"
+
+#define DRV_NAME	"ctucanfd_pci"
+
+#ifndef PCI_DEVICE_DATA
+#define PCI_DEVICE_DATA(vend, dev, data) \
+.vendor = PCI_VENDOR_ID_##vend, \
+.device = PCI_DEVICE_ID_##vend##_##dev, \
+.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0, \
+.driver_data = (kernel_ulong_t)(data)
+#endif
+#ifndef PCI_VENDOR_ID_TEDIA
+#define PCI_VENDOR_ID_TEDIA 0x1760
+#endif
+
+#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD
+#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00
+
+#define CTUCAN_BAR0_CTUCAN_ID 0x0000
+#define CTUCAN_BAR0_CRA_BASE  0x4000
+#define CYCLONE_IV_CRA_A2P_IE (0x0050)
+
+#define CTUCAN_WITHOUT_CTUCAN_ID  0
+#define CTUCAN_WITH_CTUCAN_ID     1
+
+static bool use_msi = 1;
+module_param(use_msi, bool, 0444);
+MODULE_PARM_DESC(use_msi, "PCIe implementation use MSI interrupts. Default: 1 (yes)");
+
+static bool pci_use_second = 1;
+module_param(pci_use_second, bool, 0444);
+MODULE_PARM_DESC(pci_use_second, "Use the second CAN core on PCIe card. Default: 1 (yes)");
+
+struct ctucan_pci_board_data {
+	void __iomem *bar0_base;
+	void __iomem *cra_base;
+	void __iomem *bar1_base;
+	struct list_head ndev_list_head;
+	int use_msi;
+};
+
+static struct ctucan_pci_board_data *ctucan_pci_get_bdata(struct pci_dev *pdev)
+{
+	return (struct ctucan_pci_board_data *)pci_get_drvdata(pdev);
+}
+
+static void ctucan_pci_set_drvdata(struct device *dev,
+				   struct net_device *ndev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct ctucan_priv *priv = netdev_priv(ndev);
+	struct ctucan_pci_board_data *bdata = ctucan_pci_get_bdata(pdev);
+
+	list_add(&priv->peers_on_pdev, &bdata->ndev_list_head);
+	priv->irq_flags = IRQF_SHARED;
+}
+
+/**
+ * ctucan_pci_probe - PCI registration call
+ * @pdev:	Handle to the pci device structure
+ * @ent:	Pointer to the entry from ctucan_pci_tbl
+ *
+ * This function does all the memory allocation and registration for the CAN
+ * device.
+ *
+ * Return: 0 on success and failure value on error
+ */
+static int ctucan_pci_probe(struct pci_dev *pdev,
+			    const struct pci_device_id *ent)
+{
+	struct device	*dev = &pdev->dev;
+	unsigned long driver_data = ent->driver_data;
+	struct ctucan_pci_board_data *bdata;
+	void __iomem *addr;
+	void __iomem *cra_addr;
+	void __iomem *bar0_base;
+	u32 cra_a2p_ie;
+	u32 ctucan_id = 0;
+	int ret;
+	unsigned int ntxbufs;
+	unsigned int num_cores = 1;
+	unsigned int core_i = 0;
+	int irq;
+	int msi_ok = 0;
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		dev_err(dev, "pci_enable_device FAILED\n");
+		goto err;
+	}
+
+	ret = pci_request_regions(pdev, KBUILD_MODNAME);
+	if (ret) {
+		dev_err(dev, "pci_request_regions FAILED\n");
+		goto err_disable_device;
+	}
+
+	if (use_msi) {
+		ret = pci_enable_msi(pdev);
+		if (!ret) {
+			dev_info(dev, "MSI enabled\n");
+			pci_set_master(pdev);
+			msi_ok = 1;
+		}
+	}
+
+	dev_info(dev, "ctucan BAR0 0x%08llx 0x%08llx\n",
+		 (long long)pci_resource_start(pdev, 0),
+		 (long long)pci_resource_len(pdev, 0));
+
+	dev_info(dev, "ctucan BAR1 0x%08llx 0x%08llx\n",
+		 (long long)pci_resource_start(pdev, 1),
+		 (long long)pci_resource_len(pdev, 1));
+
+	addr = pci_iomap(pdev, 1, pci_resource_len(pdev, 1));
+	if (!addr) {
+		dev_err(dev, "PCI BAR 1 cannot be mapped\n");
+		ret = -ENOMEM;
+		goto err_release_regions;
+	}
+
+	/* Cyclone IV PCI Express Control Registers Area */
+	bar0_base = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+	if (!bar0_base) {
+		dev_err(dev, "PCI BAR 0 cannot be mapped\n");
+		ret = -EIO;
+		goto err_pci_iounmap_bar1;
+	}
+
+	if (driver_data == CTUCAN_WITHOUT_CTUCAN_ID) {
+		cra_addr = bar0_base;
+		num_cores = 2;
+	} else {
+		cra_addr = bar0_base + CTUCAN_BAR0_CRA_BASE;
+		ctucan_id = ioread32(bar0_base + CTUCAN_BAR0_CTUCAN_ID);
+		dev_info(dev, "ctucan_id 0x%08lx\n", (unsigned long)ctucan_id);
+		num_cores = ctucan_id & 0xf;
+	}
+
+	irq = pdev->irq;
+
+	ntxbufs = 4;
+
+	bdata = kzalloc(sizeof(*bdata), GFP_KERNEL);
+	if (!bdata) {
+		ret = -ENOMEM;
+		goto err_pci_iounmap_bar0;
+	}
+
+	INIT_LIST_HEAD(&bdata->ndev_list_head);
+	bdata->bar0_base = bar0_base;
+	bdata->cra_base = cra_addr;
+	bdata->bar1_base = addr;
+	bdata->use_msi = msi_ok;
+
+	pci_set_drvdata(pdev, bdata);
+
+	ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 100000000,
+				  0, ctucan_pci_set_drvdata);
+	if (ret < 0)
+		goto err_free_board;
+
+	core_i++;
+
+	while (pci_use_second && (core_i < num_cores)) {
+		addr += 0x4000;
+		ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 100000000,
+					  0, ctucan_pci_set_drvdata);
+		if (ret < 0) {
+			dev_info(dev, "CTU CAN FD core %d initialization failed\n",
+				 core_i);
+			break;
+		}
+		core_i++;
+	}
+
+	/* enable interrupt in
+	 * Avalon-MM to PCI Express Interrupt Enable Register
+	 */
+	cra_a2p_ie = ioread32(cra_addr + CYCLONE_IV_CRA_A2P_IE);
+	dev_info(dev, "cra_a2p_ie 0x%08x\n", cra_a2p_ie);
+	cra_a2p_ie |= 1;
+	iowrite32(cra_a2p_ie, cra_addr + CYCLONE_IV_CRA_A2P_IE);
+	cra_a2p_ie = ioread32(cra_addr + CYCLONE_IV_CRA_A2P_IE);
+	dev_info(dev, "cra_a2p_ie 0x%08x\n", cra_a2p_ie);
+
+	return 0;
+
+err_free_board:
+	pci_set_drvdata(pdev, NULL);
+	kfree(bdata);
+err_pci_iounmap_bar0:
+	pci_iounmap(pdev, cra_addr);
+err_pci_iounmap_bar1:
+	pci_iounmap(pdev, addr);
+err_release_regions:
+	if (msi_ok) {
+		pci_disable_msi(pdev);
+		pci_clear_master(pdev);
+	}
+	pci_release_regions(pdev);
+err_disable_device:
+	pci_disable_device(pdev);
+err:
+	return ret;
+}
+
+/**
+ * ctucan_pci_remove - Unregister the device after releasing the resources
+ * @pdev:	Handle to the pci device structure
+ *
+ * This function frees all the resources allocated to the device.
+ * Return: 0 always
+ */
+static void ctucan_pci_remove(struct pci_dev *pdev)
+{
+	struct net_device *ndev;
+	struct ctucan_priv *priv = NULL;
+	struct ctucan_pci_board_data *bdata = ctucan_pci_get_bdata(pdev);
+
+	dev_dbg(&pdev->dev, "ctucan_remove");
+
+	if (!bdata) {
+		dev_err(&pdev->dev, "%s: no list of devices\n", __func__);
+		return;
+	}
+
+	/* disable interrupt in
+	 * Avalon-MM to PCI Express Interrupt Enable Register
+	 */
+	if (bdata->cra_base)
+		iowrite32(0, bdata->cra_base + CYCLONE_IV_CRA_A2P_IE);
+
+	while ((priv = list_first_entry_or_null(&bdata->ndev_list_head, struct ctucan_priv,
+						peers_on_pdev)) != NULL) {
+		ndev = priv->can.dev;
+
+		unregister_candev(ndev);
+
+		netif_napi_del(&priv->napi);
+
+		list_del_init(&priv->peers_on_pdev);
+		free_candev(ndev);
+	}
+
+	pci_iounmap(pdev, bdata->bar1_base);
+
+	if (bdata->use_msi) {
+		pci_disable_msi(pdev);
+		pci_clear_master(pdev);
+	}
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+
+	pci_iounmap(pdev, bdata->bar0_base);
+
+	pci_set_drvdata(pdev, NULL);
+	kfree(bdata);
+}
+
+static SIMPLE_DEV_PM_OPS(ctucan_pci_pm_ops, ctucan_suspend, ctucan_resume);
+
+static const struct pci_device_id ctucan_pci_tbl[] = {
+	{PCI_DEVICE_DATA(TEDIA, CTUCAN_VER21,
+		CTUCAN_WITH_CTUCAN_ID)},
+	{},
+};
+
+static struct pci_driver ctucan_pci_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = ctucan_pci_tbl,
+	.probe = ctucan_pci_probe,
+	.remove = ctucan_pci_remove,
+	.driver.pm = &ctucan_pci_pm_ops,
+};
+
+module_pci_driver(ctucan_pci_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Pavel Pisa");
+MODULE_DESCRIPTION("CTU CAN FD for PCI bus");
-- 
2.20.1

[PATCH v6 5/6] can: ctucanfd: CTU CAN FD open-source IP core - platform/SoC support.

[Pavel Pisa]

Platform bus adaptation for CTU CAN FD open-source IP core.

The core has been tested together with OpenCores SJA1000
modified to be CAN FD frames tolerant on MicroZed Zynq based
MZ_APO education kits designed by Petr Porazil from PiKRON.com
company. FPGA design

  https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top.

The kit description at the Computer Architectures course pages

  https://cw.fel.cvut.cz/wiki/courses/b35apo/documentation/mz_apo/start .

Kit carrier board and mechanics design source files

  https://gitlab.com/pikron/projects/mz_apo/microzed_apo

The work is documented in Martin Jeřábek's diploma theses
Open-source and Open-hardware CAN FD Protocol Support

  https://dspace.cvut.cz/handle/10467/80366
.

Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Signed-off-by: Martin Jerabek <martin.jerabek01@gmail.com>
Signed-off-by: Ondrej Ille <ondrej.ille@gmail.com>
---
 drivers/net/can/ctucanfd/Kconfig              |  11 ++
 drivers/net/can/ctucanfd/Makefile             |   3 +
 .../net/can/ctucanfd/ctu_can_fd_platform.c    | 142 ++++++++++++++++++
 3 files changed, 156 insertions(+)
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_platform.c

diff --git a/drivers/net/can/ctucanfd/Kconfig b/drivers/net/can/ctucanfd/Kconfig
index fb4d3cda6885..01fcfe5873cc 100644
--- a/drivers/net/can/ctucanfd/Kconfig
+++ 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
+	  The core has been tested together with OpenCores SJA1000
+	  modified to be CAN FD frames tolerant on MicroZed Zynq based
+	  MZ_APO education kits designed by Petr Porazil from PiKRON.com
+	  company. FPGA design https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top.
+	  The kit description at the Computer Architectures course pages
+	  https://cw.fel.cvut.cz/b182/courses/b35apo/documentation/mz_apo/start .
+
 endif
diff --git a/drivers/net/can/ctucanfd/Makefile b/drivers/net/can/ctucanfd/Makefile
index eb945260952d..a77ca72d534e 100644
--- a/drivers/net/can/ctucanfd/Makefile
+++ b/drivers/net/can/ctucanfd/Makefile
@@ -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
diff --git a/drivers/net/can/ctucanfd/ctu_can_fd_platform.c b/drivers/net/can/ctucanfd/ctu_can_fd_platform.c
new file mode 100644
index 000000000000..c35b16b8566b
--- /dev/null
+++ b/drivers/net/can/ctucanfd/ctu_can_fd_platform.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*******************************************************************************
+ *
+ * CTU CAN FD IP Core
+ *
+ * Copyright (C) 2015-2018 Ondrej Ille <ondrej.ille@gmail.com> FEE CTU
+ * Copyright (C) 2018-2020 Ondrej Ille <ondrej.ille@gmail.com> self-funded
+ * Copyright (C) 2018-2019 Martin Jerabek <martin.jerabek01@gmail.com> FEE CTU
+ * Copyright (C) 2018-2020 Pavel Pisa <pisa@cmp.felk.cvut.cz> FEE CTU/self-funded
+ *
+ * Project advisors:
+ *     Jiri Novak <jnovak@fel.cvut.cz>
+ *     Pavel Pisa <pisa@cmp.felk.cvut.cz>
+ *
+ * Department of Measurement         (http://meas.fel.cvut.cz/)
+ * Faculty of Electrical Engineering (http://www.fel.cvut.cz)
+ * Czech Technical University        (http://www.cvut.cz/)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ ******************************************************************************/
+
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include "ctu_can_fd.h"
+
+#define DRV_NAME	"ctucanfd"
+
+static void ctucan_platform_set_drvdata(struct device *dev,
+					struct net_device *ndev)
+{
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+
+	platform_set_drvdata(pdev, ndev);
+}
+
+/**
+ * ctucan_platform_probe - Platform registration call
+ * @pdev:	Handle to the platform device structure
+ *
+ * This function does all the memory allocation and registration for the CAN
+ * device.
+ *
+ * Return: 0 on success and failure value on error
+ */
+static int ctucan_platform_probe(struct platform_device *pdev)
+{
+	struct resource *res; /* IO mem resources */
+	struct device	*dev = &pdev->dev;
+	void __iomem *addr;
+	int ret;
+	unsigned int ntxbufs;
+	int irq;
+
+	/* Get the virtual base address for the device */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(addr)) {
+		dev_err(dev, "Cannot remap address.\n");
+		ret = PTR_ERR(addr);
+		goto err;
+	}
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "Cannot find interrupt.\n");
+		ret = irq;
+		goto err;
+	}
+
+	/* Number of tx bufs might be change in HW for future. If so,
+	 * it will be passed as property via device tree
+	 */
+	ntxbufs = 4;
+	ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 0,
+				  1, ctucan_platform_set_drvdata);
+
+	if (ret < 0)
+		platform_set_drvdata(pdev, NULL);
+
+err:
+	return ret;
+}
+
+/**
+ * ctucan_platform_remove - Unregister the device after releasing the resources
+ * @pdev:	Handle to the platform device structure
+ *
+ * This function frees all the resources allocated to the device.
+ * Return: 0 always
+ */
+static int ctucan_platform_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct ctucan_priv *priv = netdev_priv(ndev);
+
+	netdev_dbg(ndev, "ctucan_remove");
+
+	unregister_candev(ndev);
+	pm_runtime_disable(&pdev->dev);
+	netif_napi_del(&priv->napi);
+	free_candev(ndev);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(ctucan_platform_pm_ops, ctucan_suspend, ctucan_resume);
+
+/* Match table for OF platform binding */
+static const struct of_device_id ctucan_of_match[] = {
+	{ .compatible = "ctu,ctucanfd-2", },
+	{ .compatible = "ctu,ctucanfd", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, ctucan_of_match);
+
+static struct platform_driver ctucanfd_driver = {
+	.probe	= ctucan_platform_probe,
+	.remove	= ctucan_platform_remove,
+	.driver	= {
+		.name = DRV_NAME,
+		.pm = &ctucan_platform_pm_ops,
+		.of_match_table	= ctucan_of_match,
+	},
+};
+
+module_platform_driver(ctucanfd_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Martin Jerabek");
+MODULE_DESCRIPTION("CTU CAN FD for platform");
-- 
2.20.1

Re: [PATCH v6 2/6] dt-bindings: net: can: binding for CTU CAN FD open-source IP core.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

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>

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v6 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 12626 bytes --]

Hi!

> From: Martin Jerabek <martin.jerabek01@gmail.com>
> 
> 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 FGA based PCI Express board is available
> from project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).

Is "FGA" a typo? Yes, it is.

Anyway, following link tells me:

Project 'canbus/pcie-ctu_can_fd' was moved to
'canbus/pcie-ctucanfd'. Please update any links and bookmarks that may
still have the old path. Fixing it in Kconfig is more important.

> +++ b/drivers/net/can/ctucanfd/Kconfig
> @@ -0,0 +1,15 @@

> +if CAN_CTUCANFD
> +
> +endif

Empty -> drop?

> +++ b/drivers/net/can/ctucanfd/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-only

> +++ b/drivers/net/can/ctucanfd/ctu_can_fd.c
> @@ -0,0 +1,1105 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Having Makefile and sources with different licenses is rather unusual.

> +static const char * const ctucan_state_strings[] = {
> +	"CAN_STATE_ERROR_ACTIVE",
> +	"CAN_STATE_ERROR_WARNING",
> +	"CAN_STATE_ERROR_PASSIVE",
> +	"CAN_STATE_BUS_OFF",
> +	"CAN_STATE_STOPPED",
> +	"CAN_STATE_SLEEPING"
> +};

Put this near function that uses this?

> +/**
> + * ctucan_set_bittiming - CAN set bit timing routine
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the driver set bittiming routine.
> + * Return: 0 on success and failure value on error
> + */

> +/**
> + * ctucan_chip_start - This routine starts the driver
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the drivers start routine.
> + *
> + * Return: 0 on success and failure value on error
> + */

Good documentation is nice, but repeating "This routine starts the
driver" in "This is the drivers start routine." is not too helpful.

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

> +	/* Check if the TX buffer is full */
> +	if (unlikely(!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))) {
> +		netif_stop_queue(ndev);
> +		netdev_err(ndev, "BUG!, no TXB free when queue awake!\n");
> +		return NETDEV_TX_BUSY;
> +	}

You call stop_queue() without spinlock...

> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +
> +	ctucan_hw_txt_set_rdy(&priv->p, txb_id);
> +
> +	priv->txb_head++;
> +
> +	/* Check if all TX buffers are full */
> +	if (!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))
> +		netif_stop_queue(ndev);
> +
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);

...and then with spinlock held. One of them is buggy.

> +/**
> + * xcan_rx -  Is called from CAN isr to complete the received
> + *		frame processing
> + * @ndev:	Pointer to net_device structure
> + *
> + * This function is invoked from the CAN isr(poll) to process the Rx frames. It
> + * does minimal processing and invokes "netif_receive_skb" to complete further
> + * processing.
> + * Return: 1 on success and 0 on failure.
> + */

Adapt to usual 0 / -EFOO?

> +	/* Check for Arbitration Lost interrupt */
> +	if (isr.s.ali) {
> +		if (dologerr)
> +			netdev_info(ndev, "  arbitration lost");
> +		priv->can.can_stats.arbitration_lost++;
> +		if (skb) {
> +			cf->can_id |= CAN_ERR_LOSTARB;
> +			cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> +		}
> +	}
> +
> +	/* Check for Bus Error interrupt */
> +	if (isr.s.bei) {
> +		netdev_info(ndev, "  bus error");

Missing "if (dologerr)" here?

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

> +	/* Check for RX FIFO Overflow */
> +	status = ctu_can_get_status(&priv->p);
> +	if (status.s.dor) {
> +		struct net_device_stats *stats = &ndev->stats;
> +		struct can_frame *cf;
> +		struct sk_buff *skb;
> +
> +		netdev_info(ndev, "  rx fifo overflow");

And this goes at different loglevel, which will be confusing?

> +/**
> + * xcan_tx_interrupt - Tx Done Isr
> + * @ndev:	net_device pointer
> + */
> +static void ctucan_tx_interrupt(struct net_device *ndev)

Mismatch between code and docs.

> +	netdev_dbg(ndev, "%s", __func__);

This is inconsistent with other debugging.... and perhaps it is time
to remove this kind of debugging for merge.

> +/**
> + * xcan_interrupt - CAN Isr
> + */
> +static irqreturn_t ctucan_interrupt(int irq, void *dev_id)

Inconsistent.

> +		/* Error interrupts */
> +		if (isr.s.ewli || isr.s.fcsi || isr.s.ali) {
> +			union ctu_can_fd_int_stat ierrmask = { .s = {
> +				  .ewli = 1, .fcsi = 1, .ali = 1, .bei = 1 } };
> +			icr.u32 = isr.u32 & ierrmask.u32;

We normally do bit arithmetics instead of this. 

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

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

And what kind of multi-module stuff are you doing that you need
symbols exported?

> +int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigned int ntxbufs,
> +			unsigned long can_clk_rate, int pm_enable_call,
> +			void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev))
> +{

Splitting/simplifying this somehow would be good.

> +/* Register descriptions: */
> +union ctu_can_fd_frame_form_w {
> +	uint32_t u32;

u32, as you write elsewhere.

> +	struct ctu_can_fd_frame_form_w_s {
> +#ifdef __LITTLE_ENDIAN_BITFIELD
> +  /* FRAME_FORM_W */
> +		uint32_t dlc                     : 4;
> +		uint32_t reserved_4              : 1;
> +		uint32_t rtr                     : 1;
> +		uint32_t ide                     : 1;
> +		uint32_t fdf                     : 1;
> +		uint32_t reserved_8              : 1;
> +		uint32_t brs                     : 1;
> +		uint32_t esi_rsv                 : 1;
> +		uint32_t rwcnt                   : 5;
> +		uint32_t reserved_31_16         : 16;
> +#else

I believe you should simply avoid using bitfields.

> +union ctu_can_fd_timestamp_l_w {
> +	uint32_t u32;
> +	struct ctu_can_fd_timestamp_l_w_s {
> +  /* TIMESTAMP_L_W */
> +		uint32_t time_stamp_31_0        : 32;
> +	} s;
> +};

This is crazy.

> +union ctu_can_fd_data_5_8_w {
> +	uint32_t u32;
> +	struct ctu_can_fd_data_5_8_w_s {
> +#ifdef __LITTLE_ENDIAN_BITFIELD
> +  /* DATA_5_8_W */
> +		uint32_t data_5                  : 8;
> +		uint32_t data_6                  : 8;
> +		uint32_t data_7                  : 8;
> +		uint32_t data_8                  : 8;
> +#else
> +		uint32_t data_8                  : 8;
> +		uint32_t data_7                  : 8;
> +		uint32_t data_6                  : 8;
> +		uint32_t data_5                  : 8;
> +#endif
> +	} s;
> +};

even more crazy.

> +#ifdef __KERNEL__
> +# include <linux/can/dev.h>
> +#else
> +/* The hardware registers mapping and low level layer should build
> + * in userspace to allow development and verification of CTU CAN IP
> + * core VHDL design when loaded into hardware. Debugging hardware
> + * from kernel driver is really difficult, leads to system stucks
> + * by error reporting etc. Testing of exactly the same code
> + * in userspace together with headers generated automatically
> + * generated from from IP-XACT/cactus helps to driver to hardware
> + * and QEMU emulation model consistency keeping.
> + */
> +# include "ctu_can_fd_linux_defs.h"
> +#endif

Please remove non-kernel code for merge.

> +void ctucan_hw_write32(struct ctucan_hw_priv *priv,
> +		       enum ctu_can_fd_can_registers reg, u32 val)
> +{
> +	iowrite32(val, priv->mem_base + reg);
> +}

And get rid of this kind of abstraction layer.

> +// TODO: rename or do not depend on previous value of id

TODO: grep for TODO and C++ comments before attempting merge.

> +static bool ctucan_hw_len_to_dlc(u8 len, u8 *dlc)
> +{
> +	*dlc = can_len2dlc(len);
> +	return true;
> +}

Compared to how well other code is documented... This one is voodoo.

> +bool ctucan_hw_set_ret_limit(struct ctucan_hw_priv *priv, bool enable, u8 limit)
> +{
> +	union ctu_can_fd_mode_settings reg;
> +
> +	if (limit > CTU_CAN_FD_RETR_MAX)
> +		return false;
> +
> +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE);
> +	reg.s.rtrle = enable ? RTRLE_ENABLED : RTRLE_DISABLED;
> +	reg.s.rtrth = limit & 0xF;
> +	priv->write_reg(priv, CTU_CAN_FD_MODE, reg.u32);
> +	return true;
> +}

As elsewhere, I'd suggest 0/-ERRNO.

> +void ctucan_hw_set_mode_reg(struct ctucan_hw_priv *priv,
> +			    const struct can_ctrlmode *mode)
> +{
> +	u32 flags = mode->flags;
> +	union ctu_can_fd_mode_settings reg;
> +
> +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE);

> +	if (mode->mask & CAN_CTRLMODE_LOOPBACK)
> +		reg.s.ilbp = flags & CAN_CTRLMODE_LOOPBACK ?
> +					INT_LOOP_ENABLED : INT_LOOP_DISABLED;

Not sure what is going on here, but having mode->flags in same format
as hardware register would help...?

> +	switch (fnum) {
> +	case CTU_CAN_FD_FILTER_A:
> +		if (reg.s.sfa)
> +			return true;
> +	break;
> +	case CTU_CAN_FD_FILTER_B:
> +		if (reg.s.sfb)
> +			return true;
> +	break;
> +	case CTU_CAN_FD_FILTER_C:
> +		if (reg.s.sfc)
> +			return true;
> +	break;
> +	}

Check indentation of break statemnts, also elsewhere in this file

> +bool ctucan_hw_get_range_filter_support(struct ctucan_hw_priv *priv)
> +{
> +	union ctu_can_fd_filter_control_filter_status reg;
> +
> +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_FILTER_CONTROL);
> +
> +	if (reg.s.sfr)
> +		return true;

return !!reg.s.sfr; ?

> +enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct ctucan_hw_priv
> +							*priv, u8 buf)
...
> +	default:
> +		status = ~0;
> +	}
> +	return (enum ctu_can_fd_tx_status_tx1s)status;
> +}

Is ~0 in the enum?

> +	// TODO: use named constants for the command

TODO...

> +// TODO: AL_CAPTURE and ERROR_CAPTURE

...

> +#if defined(__LITTLE_ENDIAN_BITFIELD) == defined(__BIG_ENDIAN_BITFIELD)
> +# error __BIG_ENDIAN_BITFIELD or __LITTLE_ENDIAN_BITFIELD must be defined.
> +#endif

:-).
> +// True if Core is transceiver of current frame
> +#define CTU_CAN_FD_IS_TRANSMITTER(stat) (!!(stat).ts)
> +
> +// True if Core is receiver of current frame
> +#define CTU_CAN_FD_IS_RECEIVER(stat) (!!(stat).s.rxs)

Why not, documentation is nice. But it is in big contrast to other
parts of code where there's no docs at all.

> +struct ctucan_hw_priv;
> +#ifndef ctucan_hw_priv
> +struct ctucan_hw_priv {
> +	void __iomem *mem_base;
> +	u32 (*read_reg)(struct ctucan_hw_priv *priv,
> +			enum ctu_can_fd_can_registers reg);
> +	void (*write_reg)(struct ctucan_hw_priv *priv,
> +			  enum ctu_can_fd_can_registers reg, u32 val);
> +};
> +#endif

Should not be needed in kernel.

> +/**
> + * ctucan_hw_read_rx_word - Reads one word of CAN Frame from RX FIFO Buffer.
> + *
> + * @priv: Private info
> + *
> + * Return: One wword of received frame

Typo 'word'.

> +++ b/drivers/net/can/ctucanfd/ctu_can_fd_regs.h
> @@ -0,0 +1,971 @@
> +
> +/* This file is autogenerated, DO NOT EDIT! */
> +

Yay. How is that supposed to work after merge?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v6 6/6] docs: ctucanfd: CTU CAN FD open-source IP core documentation.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]

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
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v6 4/6] can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

Hi!


> @@ -12,4 +12,13 @@ config CAN_CTUCANFD
>  
>  if CAN_CTUCANFD
>  
> +config CAN_CTUCANFD_PCI
> +	tristate "CTU CAN-FD IP core PCI/PCIe driver"
> +	depends on PCI
> +	help
> +	  This driver adds PCI/PCIe support for CTU CAN-FD IP core.
> +	  The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
> +	  PCIe board with PiKRON.com designed transceiver riser shield is available
> +	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
> +
>  endif

Ok, now the if in the first patch makes sense. It can stay.

And it is separate module, so EXPORT_SYMBOLs make sense. Ok.

> +#ifndef PCI_VENDOR_ID_TEDIA
> +#define PCI_VENDOR_ID_TEDIA 0x1760
> +#endif

> +#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD
> +#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00

These should go elsewhere.

> +static bool use_msi = 1;
> +static bool pci_use_second = 1;

true?

Best regards,
							Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v6 5/6] can: ctucanfd: CTU CAN FD open-source IP core - platform/SoC support.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

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

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v6 6/6] docs: ctucanfd: CTU CAN FD open-source IP core documentation.

[Pavel Pisa]

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/

Re: [PATCH v6 5/6] can: ctucanfd: CTU CAN FD open-source IP core - platform/SoC support.

[Pavel Pisa]

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

Re: [PATCH v6 4/6] can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support.

[Pavel Pisa]

Hello Pavel,

thanks for review.

On Thursday 22 of October 2020 13:39:52 Pavel Machek wrote:
> Hi!
>
> > @@ -12,4 +12,13 @@ config CAN_CTUCANFD
> >
> >  if CAN_CTUCANFD
> >
> > +config CAN_CTUCANFD_PCI
> > +	tristate "CTU CAN-FD IP core PCI/PCIe driver"
> > +	depends on PCI
> > +	help
> > +	  This driver adds PCI/PCIe support for CTU CAN-FD IP core.
> > +	  The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
> > +	  PCIe board with PiKRON.com designed transceiver riser shield is
> > available +	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
> > +
> >  endif
>
> Ok, now the if in the first patch makes sense. It can stay.
>
> And it is separate module, so EXPORT_SYMBOLs make sense. Ok.

Great.

> > +#ifndef PCI_VENDOR_ID_TEDIA
> > +#define PCI_VENDOR_ID_TEDIA 0x1760
> > +#endif
> >
> > +#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD
> > +#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00
>
> These should go elsewhere.

They should propagate somehow from

https://pci-ids.ucw.cz/read/PC/1760/ff00

We have registered them long time ago.
I am not sure what is right mechanism.

> > +#ifndef PCI_VENDOR_ID_TEDIA
> > +#define PCI_VENDOR_ID_TEDIA 0x1760
> > +#endif

So this one should be known to kernel globally, but I would
be happy if driver build even if global process to introduce
define did not proceed end even backports would be required
for long time until kernel including CTU CAN FD propagates
into distributions, and industrial systems distributions
lag often a lot

> > +#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD

We drop this, I hope we have no system running old test
version of the core integration before Tedia offered us
to reserve some IDs (promissed that they would never use them
in future) for us.

> > +#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00

This should propagate into kernel from registry or at least
match registry.

> > +static bool use_msi = 1;
> > +static bool pci_use_second = 1;
>
> true?

Done

Best wishes,

                Pavel

Re: [PATCH v6 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.

[Pavel Pisa]

Hello Pavel,

thanks for review.

For everybody: the amount of code, analyses etc. is really huge.
If you do not have time and consider this discussion as lost of your
time and or badwidth send me a note. I will remove your from the
recipients list and if you think that some lists should be omitted
as well, please give me notice too. Same if you want to receive
only final resolutions, patches when they pass through some
of the lists.

On Thursday 22 of October 2020 13:02:13 Pavel Machek wrote:
> Hi!
>
> > From: Martin Jerabek <martin.jerabek01@gmail.com>
> >
> > 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 FGA based PCI Express board is available from
> > project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).
>
> Is "FGA" a typo? Yes, it is.

Done, thanks for catching.

I really need to force some of other people from the project
and follow up project to read all original Matin Jerabek's
and mine texts. I am blind to many of these typos as I see
same text many times.

> Anyway, following link tells me:
>
> Project 'canbus/pcie-ctu_can_fd' was moved to
> 'canbus/pcie-ctucanfd'. Please update any links and bookmarks that may
> still have the old path. Fixing it in Kconfig is more important.

Done, move is result of some more steps to name unification.

> > +++ b/drivers/net/can/ctucanfd/Kconfig
> > @@ -0,0 +1,15 @@
> >
> > +if CAN_CTUCANFD
> > +
> > +endif
>
> Empty -> drop?

Considering as appropriate after other patches comments read.
If you have other idea for patches series build give me a hint.

> > +++ b/drivers/net/can/ctucanfd/Makefile
> > @@ -0,0 +1,7 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> >
> > +++ b/drivers/net/can/ctucanfd/ctu_can_fd.c
> > @@ -0,0 +1,1105 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
>
> Having Makefile and sources with different licenses is rather unusual.

Makefile changed to GPL-2.0-or-later.
I do not consider use of Linux specific Makefile outside
GPL 2 only Linux kernel tree. But C sources are important
for us even in userspace tests and possible future projects. 

> > +static const char * const ctucan_state_strings[] = {
> > +	"CAN_STATE_ERROR_ACTIVE",
> > +	"CAN_STATE_ERROR_WARNING",
> > +	"CAN_STATE_ERROR_PASSIVE",
> > +	"CAN_STATE_BUS_OFF",
> > +	"CAN_STATE_STOPPED",
> > +	"CAN_STATE_SLEEPING"
> > +};
>
> Put this near function that uses this?

I prefer defines at start of the file, but I agree that in this
case it is different case and used only in ctucan_state_to_str .
I would prefer to move function up after array.

> > +/**
> > + * ctucan_set_bittiming - CAN set bit timing routine
> > + * @ndev:	Pointer to net_device structure
> > + *
> > + * This is the driver set bittiming routine.
> > + * Return: 0 on success and failure value on error
> > + */
> >
> > +/**
> > + * ctucan_chip_start - This routine starts the driver
> > + * @ndev:	Pointer to net_device structure
> > + *
> > + * This is the drivers start routine.
> > + *
> > + * Return: 0 on success and failure value on error
> > + */
>
> Good documentation is nice, but repeating "This routine starts the
> driver" in "This is the drivers start routine." is not too helpful.

OK, initially probably more or less placeholder to add more infomation.
I will remove it.

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

> > +	/* Check if the TX buffer is full */
> > +	if (unlikely(!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))) {
> > +		netif_stop_queue(ndev);
> > +		netdev_err(ndev, "BUG!, no TXB free when queue awake!\n");
> > +		return NETDEV_TX_BUSY;
> > +	}
>
> You call stop_queue() without spinlock...
>
> > +	spin_lock_irqsave(&priv->tx_lock, flags);
> > +
> > +	ctucan_hw_txt_set_rdy(&priv->p, txb_id);
> > +
> > +	priv->txb_head++;
> > +
> > +	/* Check if all TX buffers are full */
> > +	if (!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))
> > +		netif_stop_queue(ndev);
> > +
> > +	spin_unlock_irqrestore(&priv->tx_lock, flags);
>
> ...and then with spinlock held. One of them is buggy.

I did not feel it that way initially. But may it be there is a problem.

I expect that NET core does not invoke net_device_ops::ndo_start_xmit
aka ctucan_start_xmit when previous invocation is in progress still.
I think I have even checked it in the sources.
So we can do check for free message buffer in hardware without
locking. If there is no space we stop further transmit
but then there is something wrong anyway, because when
there was xmit started then there was already free space
in hardware. So generally this is some fatal error in logic
or hardware or driver. I hope that concurrent run of netif_wake_queue
internal functions to ctucan_start_xmit is prevented by NET
core logic and locking. So even if some buffer becomes free
in meanwhile and invokes netif_wake_queue, it waits with
next ctucan_start_xmit invocation till previous one
finishes.

Incorrect concurrence can happen if ctucan_hw_txt_set_rdy
and frame transmission finishes before ctucan_start_xmit
finishes. In such case there appear incorrect sequence
of manipulation with hardware and counting occupied
and free buffers. So buffer counting is protected
by spinlock....

If this is the last empty TX buffer filled by xmit then
further xmit has to be stopped. So we call
netif_stop_queue(ndev); In the theory, it can
be called after spin lock release (if netif_wake_queue
is synchronized). So if mine analysis is right then
I can check HW for empty buffer under spinlock,
keep result and call netif_stop_queue later.
It seems that netif_stop_queue is called in other
drivers with irq locks held. So there should not be
a problem, netif_stop_queue locking is orthogonal
to &priv->tx_lock which protects HW and slots
counting.

But if somebody can check and clarify that my idea
is wrong or confirm, that moving out of spinlock
section is correct way to go, I would be happy.
Many CAN controllers use only single buffer for TX
so they do not solve these conditions. These
which solve more parallel Tx buffers seems to
not prove me wrong according to my reading.

> > +/**
> > + * xcan_rx -  Is called from CAN isr to complete the received
> > + *		frame processing
> > + * @ndev:	Pointer to net_device structure
> > + *
> > + * This function is invoked from the CAN isr(poll) to process the Rx
> > frames. It + * does minimal processing and invokes "netif_receive_skb" to
> > complete further + * processing.
> > + * Return: 1 on success and 0 on failure.
> > + */
>
> Adapt to usual 0 / -EFOO?

OK

> > +	/* Check for Arbitration Lost interrupt */
> > +	if (isr.s.ali) {
> > +		if (dologerr)
> > +			netdev_info(ndev, "  arbitration lost");
> > +		priv->can.can_stats.arbitration_lost++;
> > +		if (skb) {
> > +			cf->can_id |= CAN_ERR_LOSTARB;
> > +			cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> > +		}
> > +	}
> > +
> > +	/* 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.

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

> > +	/* Check for RX FIFO Overflow */
> > +	status = ctu_can_get_status(&priv->p);
> > +	if (status.s.dor) {
> > +		struct net_device_stats *stats = &ndev->stats;
> > +		struct can_frame *cf;
> > +		struct sk_buff *skb;
> > +
> > +		netdev_info(ndev, "  rx fifo overflow");
>
> And this goes at different loglevel, which will be confusing?

I add prefix there, this condition is more important to be noticed.

> > +/**
> > + * xcan_tx_interrupt - Tx Done Isr
> > + * @ndev:	net_device pointer
> > + */
> > +static void ctucan_tx_interrupt(struct net_device *ndev)
>
> Mismatch between code and docs.

Corrected.

> > +	netdev_dbg(ndev, "%s", __func__);
>
> This is inconsistent with other debugging.... and perhaps it is time
> to remove this kind of debugging for merge.

OK

> > +/**
> > + * xcan_interrupt - CAN Isr
> > + */
> > +static irqreturn_t ctucan_interrupt(int irq, void *dev_id)
>
> Inconsistent.

Corrected

> > +		/* Error interrupts */
> > +		if (isr.s.ewli || isr.s.fcsi || isr.s.ali) {
> > +			union ctu_can_fd_int_stat ierrmask = { .s = {
> > +				  .ewli = 1, .fcsi = 1, .ali = 1, .bei = 1 } };
> > +			icr.u32 = isr.u32 & ierrmask.u32;
>
> We normally do bit arithmetics instead of this.

This is attempt to use only HW definitions generated from registers definition 
in IPXACT format

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.

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

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

> And what kind of multi-module stuff are you doing that you need
> symbols exported?
>
> > +int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq,
> > unsigned int ntxbufs, +			unsigned long can_clk_rate, int pm_enable_call,
> > +			void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev))
> > +{
>
> Splitting/simplifying this somehow would be good.

This is attempt to hide the most of all other functions from bus integration
modules. Prototype is not nice, but it is only one of a few function exported.
The arguments can be passed through structure. May it be better???

> > +/* Register descriptions: */
> > +union ctu_can_fd_frame_form_w {
> > +	uint32_t u32;
>
> u32, as you write elsewhere.

I would be happy, if we can keep uint32_t there.
The header files are generated from IPXACT and are intended to be used
even outside of Linux kernel. I.e. QEMU

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/net/can/ctu_can_fd_regs.h

Easy way to compare these together is great to keep consistency.
Adaptation of generation tools to run in multiple modes would
be overhead. So if the uint32_t type in HW definition
is acceptable for Linux kernel it would save us lot of
headaches and possible errors in the future.

Enforce u32 in all other projects in form visible
from this and other header files is problematic. 

> > +	struct ctu_can_fd_frame_form_w_s {
> > +#ifdef __LITTLE_ENDIAN_BITFIELD
> > +  /* FRAME_FORM_W */
> > +		uint32_t dlc                     : 4;
> > +		uint32_t reserved_4              : 1;
> > +		uint32_t rtr                     : 1;
> > +		uint32_t ide                     : 1;
> > +		uint32_t fdf                     : 1;
> > +		uint32_t reserved_8              : 1;
> > +		uint32_t brs                     : 1;
> > +		uint32_t esi_rsv                 : 1;
> > +		uint32_t rwcnt                   : 5;
> > +		uint32_t reserved_31_16         : 16;
> > +#else
>
> I believe you should simply avoid using bitfields.

I would prefer style I have negotiated with my students for generated
HW description for RTEMS TI TMS570 support

  https://git.rtems.org/rtems/tree/bsps/arm/tms570/include/bsp/ti_herc/reg_dcan.h

but Ondrej Ille argued that the macro names are long and you need more of them
or ?: hack to encode filed position and its length. Which is correct arguments.
There are more Linux kernel files using this style

  grep -r  __LITTLE_ENDIAN_BITFIELD include

so we agreed that it should be acceptable. Big and little endian part
match is guaranteed because files are autogenerated.
Actual access and move to bitfiled typed variable is done
through right IO access functions and offsets are given by defines.
So there are no registers HW access overlay structures used.

> > +union ctu_can_fd_timestamp_l_w {
> > +	uint32_t u32;
> > +	struct ctu_can_fd_timestamp_l_w_s {
> > +  /* TIMESTAMP_L_W */
> > +		uint32_t time_stamp_31_0        : 32;
> > +	} s;
> > +};
>
> This is crazy.

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.

> > +union ctu_can_fd_data_5_8_w {
> > +	uint32_t u32;
> > +	struct ctu_can_fd_data_5_8_w_s {
> > +#ifdef __LITTLE_ENDIAN_BITFIELD
> > +  /* DATA_5_8_W */
> > +		uint32_t data_5                  : 8;
> > +		uint32_t data_6                  : 8;
> > +		uint32_t data_7                  : 8;
> > +		uint32_t data_8                  : 8;
> > +#else
> > +		uint32_t data_8                  : 8;
> > +		uint32_t data_7                  : 8;
> > +		uint32_t data_6                  : 8;
> > +		uint32_t data_5                  : 8;
> > +#endif
> > +	} s;
> > +};
>
> even more crazy.

Data part other then DATA_1_4_W is not referenced from the driver.
And yes, I would like more DATA_0_3_W etc... But question is indexing
in the standards. I have tried to help the project to move into direction
to be sustainable in our small team and enhance consistency.
But we have not resources to start whole
work from scratch again and due to effort into testing, analyses
etc. it would be extremely hard to get into same state again.
And I hope that there are not many really painfull limits for future
enhancements.

The design can be extended for 8 TX frames for examle. Above that there
would be problem with atomic priority management. But this should be
enough for most applications and if sequential stream at some priority
is required then we can combine core with DMA. The same for RX.
I plan DMA there. Actual design with SW controllable priorities
allows us to maintain single TX FIFO but even more dynamic multiqueue
FIFOs. I want to have time to experiment in these directions,
but until the code is mainlined it would be really headache
to keep additional features above base changing according to reviewer
requests. So when there is this really simple minimal base for CAN/CAN FD
functionality agreed upon then we want to develop extensions in more
directions interresting for us and our applications.

> > +#ifdef __KERNEL__
> > +# include <linux/can/dev.h>
> > +#else
> > +/* The hardware registers mapping and low level layer should build
> > + * in userspace to allow development and verification of CTU CAN IP
> > + * core VHDL design when loaded into hardware. Debugging hardware
> > + * from kernel driver is really difficult, leads to system stucks
> > + * by error reporting etc. Testing of exactly the same code
> > + * in userspace together with headers generated automatically
> > + * generated from from IP-XACT/cactus helps to driver to hardware
> > + * and QEMU emulation model consistency keeping.
> > + */
> > +# include "ctu_can_fd_linux_defs.h"
> > +#endif
>
> Please remove non-kernel code for merge.

This is only one line which allows us to run code test in userspace
which is of big help to keep code consistent. This part of kernel
driver sources is used during userspace test of hardware registers
implemented right in each FPGA build run so it checks that SW
and HW matches

  https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top/-/jobs/73445

See the line cantest/test_regtest.py::test_regtest PASSED

If you confirm that it is really required to remove this single
include ifdef then we remove it. But it would break our tooling
and we have to patching during tests or keep in sync two versions
of this C file which has to be prepared manually, it cannot be autogenerated,
because it holds algorithmic description how to access structures
defined by hardware.

> > +void ctucan_hw_write32(struct ctucan_hw_priv *priv,
> > +		       enum ctu_can_fd_can_registers reg, u32 val)
> > +{
> > +	iowrite32(val, priv->mem_base + reg);
> > +}
>
> And get rid of this kind of abstraction layer.

We need big and little endian support. This works and is tiny.
I can try to switch to linux/regmap.h, but it seems to me as a monster
and I do not understand it fully yet.

MCAN uses similar approach, common part

static inline u32 m_can_read(struct m_can_classdev *cdev, enum m_can_reg reg)
{
        return cdev->ops->read_reg(cdev, reg);
}

platform bus specific one

static u32 iomap_read_reg(struct m_can_classdev *cdev, int reg)
{
        struct m_can_plat_priv *priv = cdev->device_data;

        return readl(priv->base + reg);
}

So I think that it is not against actual conventions.

When the CTU CAN FD IP core driver runs on big endian systems
then PCI bus is usually still mapped little endian way but platform
IP mapping is native big endian. Actual driver automatically decides
according to CTU_CAN_FD_DEVICE_ID register read.

> > +// TODO: rename or do not depend on previous value of id
>
> TODO: grep for TODO and C++ comments before attempting merge.

That is my mistake, but some of these are really there for future
development or cleanups, some only remarks. As I have said
I do not consider projects as perfect or finished but as in
state usable for broader community and when format is really
agreed/accepted then we can continue. But I do not want waste
my time to work above code base which has to be changed again
and again. So there are much more TODOs in my head.

> > +static bool ctucan_hw_len_to_dlc(u8 len, u8 *dlc)
> > +{
> > +	*dlc = can_len2dlc(len);
> > +	return true;
> > +}
>
> Compared to how well other code is documented... This one is voodoo.

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.

> > +bool ctucan_hw_set_ret_limit(struct ctucan_hw_priv *priv, bool enable,
> > u8 limit) +{
> > +	union ctu_can_fd_mode_settings reg;
> > +
> > +	if (limit > CTU_CAN_FD_RETR_MAX)
> > +		return false;
> > +
> > +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE);
> > +	reg.s.rtrle = enable ? RTRLE_ENABLED : RTRLE_DISABLED;
> > +	reg.s.rtrth = limit & 0xF;
> > +	priv->write_reg(priv, CTU_CAN_FD_MODE, reg.u32);
> > +	return true;
> > +}
>
> As elsewhere, I'd suggest 0/-ERRNO.

Hmm, generally there is attempt to keep ctu_can_fd_hw.c dependant
only on generated hardware description and independent of OS
specific constructs. It helps its testing in other environments.
Yes, there exists even Windows drier for other SkodaAuto
project and CTU CAN FD integration which runs same ctu_can_fd_hw.c
And it is the only part which directly depends on HW registers
placement. We do not go typical direction of Windows code and then
adaptation for Linux. We try to be OS agnostic in lowest level HW
adaptation and provide minimal well matching driver which does
not take care of generated sources.

> > +void ctucan_hw_set_mode_reg(struct ctucan_hw_priv *priv,
> > +			    const struct can_ctrlmode *mode)
> > +{
> > +	u32 flags = mode->flags;
> > +	union ctu_can_fd_mode_settings reg;
> > +
> > +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE);
> >
> > +	if (mode->mask & CAN_CTRLMODE_LOOPBACK)
> > +		reg.s.ilbp = flags & CAN_CTRLMODE_LOOPBACK ?
> > +					INT_LOOP_ENABLED : INT_LOOP_DISABLED;
> Not sure what is going on here, but having mode->flags in same format
> as hardware register would help...?
>

In this case we prefer to use defines which match Linux SocketCAN
defines (CAN_CTRLMODE_LOOPBACK,...) and translate them to HW specific
bits only inside ctu_can_fd_hw.c to not pollute driver by hardware
specific details. As you can see, we use Linux specific define
and adapt other builds to match Linux convention.

> > +	switch (fnum) {
> > +	case CTU_CAN_FD_FILTER_A:
> > +		if (reg.s.sfa)
> > +			return true;
> > +	break;
> > +	case CTU_CAN_FD_FILTER_B:
> > +		if (reg.s.sfb)
> > +			return true;
> > +	break;
> > +	case CTU_CAN_FD_FILTER_C:
> > +		if (reg.s.sfc)
> > +			return true;
> > +	break;
> > +	}
>
> Check indentation of break statemnts, also elsewhere in this file

OK, I look at examples, I am not sure if I have overlooked checkpatch.pl warnings...
I think that not. So it seems to be acceptabe for some case at least in 4.19 RT.

> > +bool ctucan_hw_get_range_filter_support(struct ctucan_hw_priv *priv)
> > +{
> > +	union ctu_can_fd_filter_control_filter_status reg;
> > +
> > +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_FILTER_CONTROL);
> > +
> > +	if (reg.s.sfr)
> > +		return true;
>
> return !!reg.s.sfr; ?

OK.

> > +enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct
> > ctucan_hw_priv +							*priv, u8 buf)
>
> ...
>
> > +	default:
> > +		status = ~0;
> > +	}
> > +	return (enum ctu_can_fd_tx_status_tx1s)status;
> > +}
>
> Is ~0 in the enum?

This can happen only when there is fundamental internal error
in the code logic, but good catch, I add something to enum. 

> > +	// TODO: use named constants for the command
>
> TODO...

It is intenal mechanism between ctu_can_fd_hw.c and ctu_can_fd_hw.h
with fully documented external API in ctu_can_fd_hw.h .
So it is questionable if to polute user visible header
there by another definitions. May it be we should switch to
type ctu_can_fd_tx_command for command or do other optimization
which I can imagine.

This TODO is Ondrej Ille and Marin Jerabek leftover, so they
should provide their suggestion and intention there as well
as consequences of change for other projects which are
not mine busyness.
   

> > +// TODO: AL_CAPTURE and ERROR_CAPTURE
>

Again colleagues remark for future work. For me, it is important
basic function under GNU/Linux.

>
> > +#if defined(__LITTLE_ENDIAN_BITFIELD) == defined(__BIG_ENDIAN_BITFIELD)
> > +# error __BIG_ENDIAN_BITFIELD or __LITTLE_ENDIAN_BITFIELD must be
> > defined. +#endif
> >
> :-).

To catch problems in another more undermined environments which I am
happy not to take care at all.


> > +// True if Core is transceiver of current frame
> > +#define CTU_CAN_FD_IS_TRANSMITTER(stat) (!!(stat).ts)
> > +
> > +// True if Core is receiver of current frame
> > +#define CTU_CAN_FD_IS_RECEIVER(stat) (!!(stat).s.rxs)
>
> Why not, documentation is nice. But it is in big contrast to other
> parts of code where there's no docs at all.

This is to document things outside of ctu_can_fd_hw.c.
On the other hand actual HW is documented in referenced generated
PDF file so there is not so big need to documents each bit
of ctu_can_fd_hw.c . There is even internal HW docuemntation

  http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/ctu_can_fd_architecture.pdf

so in the fact these parts are documented for those who poke with them
quite well and for regular driver developer the functions of ctu_can_fd_hw.h
should provide all what is needed. And yes you can use referenced documentation
and go deeper

  https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/blob/master/src/memory_registers/memory_registers.vhd

so basically ctu_can_fd_hw.c is interface between those who wants to go deeper,
try to modify HW, do verification etc. and these who do not care about these
HW specific bits and ctu_can_fd_hw.h should be sufficient for driver developers.

All this division to abstraction layers is result of more iterations thoughts and experiments.
There are not many open-source/open-hardware projects of this scale where to learn from
and we are really limited team. Chips Alliance aims this direction now
but CTU CAN FD project predates its founding by many years.

> > +struct ctucan_hw_priv;
> > +#ifndef ctucan_hw_priv
> > +struct ctucan_hw_priv {
> > +	void __iomem *mem_base;
> > +	u32 (*read_reg)(struct ctucan_hw_priv *priv,
> > +			enum ctu_can_fd_can_registers reg);
> > +	void (*write_reg)(struct ctucan_hw_priv *priv,
> > +			  enum ctu_can_fd_can_registers reg, u32 val);
> > +};
> > +#endif
>
> Should not be needed in kernel.

I am really not sure if this is required for some userpace tests or other CTU CAN FD
based projects. It is to colleagues participating in other projects to defend this
ifndef. I think that it is remnant of hacks before I have separated
ctucan_priv and ctucan_hw_priv structures for proper layering.


> > +/**
> > + * ctucan_hw_read_rx_word - Reads one word of CAN Frame from RX FIFO
> > Buffer. + *
> > + * @priv: Private info
> > + *
> > + * Return: One wword of received frame
>
> Typo 'word'.

Thanks.

> > +++ b/drivers/net/can/ctucanfd/ctu_can_fd_regs.h
> > @@ -0,0 +1,971 @@
> > +
> > +/* This file is autogenerated, DO NOT EDIT! */
> > +
>
> Yay. How is that supposed to work after merge?

I think that it is really better to not edit this file directly.
It has to match exactly hardware. So we provide compatible
license which allows you edit, but you have been warned
to not shoot into your own leg.

And for sure you can clone CTU CAN FD repo and experiment and generate
your own core derivative. You can even include tools into Linux
kernel tree or reference them

  https://github.com/Blebowski/Reg_Map_Gen
  https://github.com/Blebowski/ipyxact

same as use complete Kactus2 IP cores editor to manage specification file.

But if somebody wants to do such massive changes without coordination
with us it would be polite to copy driver do different directory under
another name and change "ctu,ctucanfd" in device tree to something
different.

Again thanks for review, I try to follow most of your suggestions
for some I would be really happy if you and other reviewers
try to look at the project as the whole. Linux driver is really
small (but very important) part and some relatively small
and I think not so important changes can result in significant
problems in another parts of the quite large project
and its automatic testing and inject errors without our notice.
Actual setup allows us to run full synthetic and real hardware
tests for each changed line in driver or HDL sources. This is simple
test and each night complex test including simulated jitter on CAN lines etc.
is run. In sum, it takes servers about three hours to do all testing
on multiple projects to provide next day results for the previous
day development step.

Best wishes,

                Pavel

Re: [PATCH v6 5/6] can: ctucanfd: CTU CAN FD open-source IP core - platform/SoC support.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

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

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v6 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.

[Pavel Pisa]

Hello Pavel and others,

thanks for review and suggestions. I have spent weekend
to attempt to resolve the most of the suggestions.

The result is merge request to our projects

  https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/merge_requests/372

I am running test with two CTU CAN FD cores and two SJA1000 FD tollereant
against two CTU CAN FD cores on PCIe board on my home machine
from the morning. Zynq XCANs are not put into the mix because
they are not FD tolerant.

Actual statistics from Zynq system

can2: flags=193<UP,RUNNING,NOARP>  mtu 16  Open Cores SJA1000 FD tollerant
        unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 10  (UNSPEC)
        RX packets 2431626  bytes 14089057 (13.4 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 329075  bytes 2383757 (2.2 MiB)
        TX errors 65043  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 49

can3: flags=193<UP,RUNNING,NOARP>  mtu 16  Open Cores SJA1000 FD tollerant
        unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 10  (UNSPEC)
        RX packets 2695281  bytes 15459822 (14.7 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 50

can4: flags=193<UP,RUNNING,NOARP>  mtu 72  CTU CAN FD on Zynq
        unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 10  (UNSPEC)
        RX packets 84791228  bytes 1307744193 (1.2 GiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 1189540  bytes 17447286 (16.6 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 47

can5: flags=193<UP,RUNNING,NOARP>  mtu 72  CTU CAN FD on Zynq
        unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 10  (UNSPEC)
        RX packets 81264790  bytes 1273404732 (1.1 GiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 4715979  bytes 51786821 (49.3 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 48

Statistic from my the Intel Core 2 based system, only CTU CAN FD, EMS PCI board
not used, it is not FD tollereant.

can2: flags=193<UP,RUNNING,NOARP>  mtu 72  CTU CAN FD on DB4CGX15
        unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 10  (UNSPEC)
        RX packets 77731204  bytes 1222332122 (1.1 GiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 847131  bytes 13328222 (12.7 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 33

can3: flags=193<UP,RUNNING,NOARP>  mtu 72  CTU CAN FD on DB4CGX15
        unspec 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  txqueuelen 10  (UNSPEC)
        RX packets 1830253  bytes 28749064 (27.4 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 76340684  bytes 1200464054 (1.1 GiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 33

There is no indication of errors in the logs. On the other hand,
massive renames and even some code functional changes to allow
rewrite some parts to be better readable could introduce
some errors.

Automatic systemic tests and builds do passed in the final
version

  https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/pipelines/25070

Same as automatic test at computes at the university

  https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top/-/pipelines/25071

So hopefully nothing is broken but testing for all corner cases
after this update can take weeks.

The response to review notes follows. I will wait for remarks form the community
or at least check for typos from colleagues and then I send v7 series.

Best wishes,

Pavel

On Thursday 22 of October 2020 13:02:13 Pavel Machek wrote:
> Hi!
>
> > From: Martin Jerabek <martin.jerabek01@gmail.com>
> >
> > 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 FGA based PCI Express board is available from
> > project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).
>
> Is "FGA" a typo? Yes, it is.

Fixed

> Anyway, following link tells me:
>
> Project 'canbus/pcie-ctu_can_fd' was moved to
> 'canbus/pcie-ctucanfd'. Please update any links and bookmarks that may
> still have the old path. Fixing it in Kconfig is more important.

Unification of ctu_can_fd -> ctucanfd done for file names, driver paths,
device tree and documentation. Not enough resources to unify HDL sources,
generated files etc. It would break history (blames etc.), testing etc.,
there are more than 4k occurrences.

> > +++ b/drivers/net/can/ctucanfd/Kconfig
> > @@ -0,0 +1,15 @@
> >
> > +if CAN_CTUCANFD
> > +
> > +endif
>
> Empty -> drop?

TODO

> > +++ b/drivers/net/can/ctucanfd/Makefile
> > @@ -0,0 +1,7 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> >
> > +++ b/drivers/net/can/ctucanfd/ctu_can_fd.c
> > @@ -0,0 +1,1105 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
>
> Having Makefile and sources with different licenses is rather unusual.

UNified

> > +static const char * const ctucan_state_strings[] = {
> > +	"CAN_STATE_ERROR_ACTIVE",
> > +	"CAN_STATE_ERROR_WARNING",
> > +	"CAN_STATE_ERROR_PASSIVE",
> > +	"CAN_STATE_BUS_OFF",
> > +	"CAN_STATE_STOPPED",
> > +	"CAN_STATE_SLEEPING"
> > +};
>
> Put this near function that uses this?

ctucan_state_to_str() moved after array, array defined with strict
correspondence of index to text.

> > +/**
> > + * ctucan_set_bittiming - CAN set bit timing routine
> > + * @ndev:	Pointer to net_device structure
> > + *
> > + * This is the driver set bittiming routine.
> > + * Return: 0 on success and failure value on error
> > + */
> >
> > +/**
> > + * ctucan_chip_start - This routine starts the driver
> > + * @ndev:	Pointer to net_device structure
> > + *
> > + * This is the drivers start routine.
> > + *
> > + * Return: 0 on success and failure value on error
> > + */
>
> Good documentation is nice, but repeating "This routine starts the
> driver" in "This is the drivers start routine." is not too helpful.

Included relevant text as well as I have decided to move call
to ctucan_reset() out of ctucan_chip_start() and slightly reorder/clarify
some part of initialization and error handling.

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

Clarified NETDEV_TX_BUSY case and decided to drop frame if there is
problem with format, length or other parameters during attempt
to move frame into hardware.

> > +	/* Check if the TX buffer is full */
> > +	if (unlikely(!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))) {
> > +		netif_stop_queue(ndev);
> > +		netdev_err(ndev, "BUG!, no TXB free when queue awake!\n");
> > +		return NETDEV_TX_BUSY;
> > +	}
>
> You call stop_queue() without spinlock...

I hope that netif_stop_queue can be called from ctucan_start_xmit
without locking and that during ctucan_start_xmit() call, would networking
core postpone finish of netif_wake_queue(ndev) invoked from other CPU
core or kernel thread. I have send longer request for clarification
in previous reply and I would be happy, if somebody can check
that my findings are right. If all that locking in network core work
as I understand it then I would move the second call out of spinlock.
But if there are differences how locking in networking core works
between kernel versions (I have noticed some indication in the past)
then I think that actual solution is correct and more robust.
Even other drivers calls netif_stop_queue from irq lock protected
regions...

> > +	spin_lock_irqsave(&priv->tx_lock, flags);
> > +
> > +	ctucan_hw_txt_set_rdy(&priv->p, txb_id);
> > +
> > +	priv->txb_head++;
> > +
> > +	/* Check if all TX buffers are full */
> > +	if (!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))
> > +		netif_stop_queue(ndev);
> > +
> > +	spin_unlock_irqrestore(&priv->tx_lock, flags);
>
> ...and then with spinlock held. One of them is buggy.

See above.

> > +/**
> > + * xcan_rx -  Is called from CAN isr to complete the received
> > + *		frame processing
> > + * @ndev:	Pointer to net_device structure
> > + *
> > + * This function is invoked from the CAN isr(poll) to process the Rx
> > frames. It + * does minimal processing and invokes "netif_receive_skb" to
> > complete further + * processing.
> > + * Return: 1 on success and 0 on failure.
> > + */
>
> Adapt to usual 0 / -EFOO?

I have clarified that 1 is used to inform that frame has been fully received
and sent to the network layer. 0 inform that there is frame first word read
but SKB cannot be (hopefully temporarily) allocated. -EAGAIN indicate unlikelly
condition that function is called when there is no frame in Rx FIFO, this indicates
HW bug because this cannot happen when Rx FIFO frame count is not zero.

> > +	/* Check for Arbitration Lost interrupt */
> > +	if (isr.s.ali) {
> > +		if (dologerr)
> > +			netdev_info(ndev, "  arbitration lost");
> > +		priv->can.can_stats.arbitration_lost++;
> > +		if (skb) {
> > +			cf->can_id |= CAN_ERR_LOSTARB;
> > +			cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> > +		}
> > +	}
> > +
> > +	/* Check for Bus Error interrupt */
> > +	if (isr.s.bei) {
> > +		netdev_info(ndev, "  bus error");
>
> Missing "if (dologerr)" here?

Bus error is result of series of troubles and should be logged. Bus restart
after bus error is rate limited by driver nad hardware, so the missing 
dologerr is intentional there. I have added \n to all messages and left them
to be considered independent.

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

OK, usesfull during development and only on debug level but removed now.

> > +	/* Check for RX FIFO Overflow */
> > +	status = ctu_can_get_status(&priv->p);
> > +	if (status.s.dor) {
> > +		struct net_device_stats *stats = &ndev->stats;
> > +		struct can_frame *cf;
> > +		struct sk_buff *skb;
> > +
> > +		netdev_info(ndev, "  rx fifo overflow");
>
> And this goes at different loglevel, which will be confusing?

Levels try to be reciprocal to severity and proportional to occurrence
frequency. Can be tuned for sure. Some most flooding are ratelimited.

> > +/**
> > + * xcan_tx_interrupt - Tx Done Isr
> > + * @ndev:	net_device pointer
> > + */
> > +static void ctucan_tx_interrupt(struct net_device *ndev)
>
> Mismatch between code and docs.

Corrected.

> > +	netdev_dbg(ndev, "%s", __func__);
>
> This is inconsistent with other debugging.... and perhaps it is time
> to remove this kind of debugging for merge.

I have removed some and will consider others.
We are far from being production silicon version, so some debugging
helps even "hardware"/HDL development.

It seems that checkpatch pushes me in use of __func__.

WARNING: Prefer using '"%s...", __func__' to using 'ctucan_set_bittiming', this function's name, in a string
#127: FILE: /home/pi/fpga/can-fd/ctu-can-fd/CAN_FD_IP_Core/driver/linux/ctucanfd_base.c:127:
+	netdev_dbg(ndev, "ctucan_set_bittiming\n");

Generally, I am not sure if it is problem to have there many debugs
at NET debug level. Today, when it is possible to control debug
levels on per file basis it should not be so big problem and it can
help to debug code and hardware interaction. It can be removed
any time later.

> > +/**
> > + * xcan_interrupt - CAN Isr
> > + */
> > +static irqreturn_t ctucan_interrupt(int irq, void *dev_id)
>
> Inconsistent.

Corrected.

> > +		/* Error interrupts */
> > +		if (isr.s.ewli || isr.s.fcsi || isr.s.ali) {
> > +			union ctu_can_fd_int_stat ierrmask = { .s = {
> > +				  .ewli = 1, .fcsi = 1, .ali = 1, .bei = 1 } };
> > +			icr.u32 = isr.u32 & ierrmask.u32;
>
> We normally do bit arithmetics instead of this.

As described, my intention is to use only fields and bits mapping
which is directly generated from IPXACT specification
and team decision was to to use structs for bitfields.
So I do not want to add manually introduced defines.
Even that core interface is stabilized at least for version 2.x now,
I want all code to directly reflect HW design changes.

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

Block is to inform, that imask is really local and you do not
need to look for it elsewhere in the function.
But you prefer flat so I move define to start and remove
block. In this case all F are OK, we know that there is nothing
else than interrupts enable, mask bits, and if all should be
stopped in response to error, then there cannot be problem even after
some move of bitfileds in the register.

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

Not critical for me, may be Ondrej Ille has some opinion there.

> And what kind of multi-module stuff are you doing that you need
> symbols exported?

Seems to be understood from followup patches.

> > +int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq,
> > unsigned int ntxbufs, +			unsigned long can_clk_rate, int pm_enable_call,
> > +			void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev))
> > +{
>
> Splitting/simplifying this somehow would be good.

If you confirm my offer to move to structure with options then
I look at it. On the other hand when functions prototype
changes it enforces all users to get update which can bee
better to catch problems than sillent structure fields addition.
If zeroed the first then probably manageable too.

> > +/* Register descriptions: */
> > +union ctu_can_fd_frame_form_w {
> > +	uint32_t u32;
>
> u32, as you write elsewhere.

As I already described these generated files should be used one
to one in QEMU and other tools. There seems to be acceptable
to use uint32_t in the kernel for these cases.
Definition of u32 type in all other cases would lead often
to unwanted pollute of namespace.

> > +	struct ctu_can_fd_frame_form_w_s {
> > +#ifdef __LITTLE_ENDIAN_BITFIELD
> > +  /* FRAME_FORM_W */
> > +		uint32_t dlc                     : 4;
> > +		uint32_t reserved_4              : 1;
> > +		uint32_t rtr                     : 1;
> > +		uint32_t ide                     : 1;
> > +		uint32_t fdf                     : 1;
> > +		uint32_t reserved_8              : 1;
> > +		uint32_t brs                     : 1;
> > +		uint32_t esi_rsv                 : 1;
> > +		uint32_t rwcnt                   : 5;
> > +		uint32_t reserved_31_16         : 16;
> > +#else
>
> I believe you should simply avoid using bitfields.

I have already described that my personal tendency
is similar but in this case big and little part
is kept consistent by generation and it was preferred
solution by colleagues. There are more places where this
style is used in the kernel and we do not use overlay structures
for hardware directly. Value is read by iored/iowrite in all cases
and union/struct works only for local variables to parse
bitfields. It is correct from IO synchronization
and single access rules and it optimizes to same code
as use of defines on local variables.

> > +union ctu_can_fd_timestamp_l_w {
> > +	uint32_t u32;
> > +	struct ctu_can_fd_timestamp_l_w_s {
> > +  /* TIMESTAMP_L_W */
> > +		uint32_t time_stamp_31_0        : 32;
> > +	} s;
> > +};
>
> This is crazy.

Yes, but generated from spec so it is kept consistent.
Adding exception for case that size of field is 32-bits
is possible but when field size changes it would cause
incorrect access.

> > +union ctu_can_fd_data_5_8_w {
> > +	uint32_t u32;
> > +	struct ctu_can_fd_data_5_8_w_s {
> > +#ifdef __LITTLE_ENDIAN_BITFIELD
> > +  /* DATA_5_8_W */
> > +		uint32_t data_5                  : 8;
> > +		uint32_t data_6                  : 8;
> > +		uint32_t data_7                  : 8;
> > +		uint32_t data_8                  : 8;
> > +#else
> > +		uint32_t data_8                  : 8;
> > +		uint32_t data_7                  : 8;
> > +		uint32_t data_6                  : 8;
> > +		uint32_t data_5                  : 8;
> > +#endif
> > +	} s;
> > +};
>
> even more crazy.

This is mainly for documentation where it has place and reason.
It is used only in sense of data area start in the driver.

> > +#ifdef __KERNEL__
> > +# include <linux/can/dev.h>
> > +#else
> > +/* The hardware registers mapping and low level layer should build
> > + * in userspace to allow development and verification of CTU CAN IP
> > + * core VHDL design when loaded into hardware. Debugging hardware
> > + * from kernel driver is really difficult, leads to system stucks
> > + * by error reporting etc. Testing of exactly the same code
> > + * in userspace together with headers generated automatically
> > + * generated from from IP-XACT/cactus helps to driver to hardware
> > + * and QEMU emulation model consistency keeping.
> > + */
> > +# include "ctu_can_fd_linux_defs.h"
> > +#endif
>
> Please remove non-kernel code for merge.

As I tried to describe these mechanism allows to ensure that
there is match between HW access generated registers description
and manual algorithmic part of HW interface and actual implementation
of registers file by different test from userpace run during each
driver or HDL update. I am convinced personally that it worth
to be run and tested. Alternative to #ifdef is file patching 
during userspace build, but we use same files with symliks only
during GitLab runner execution. It would complicate things
on our side and can lead to not catching problem which would cost
really much more than four additional lines. I have added description
for others to understand value of this solution.
If really requested to be removed, then I would follow
requirement but with bad taste what more worthless problems
I have to spent my time. 

> > +void ctucan_hw_write32(struct ctucan_hw_priv *priv,
> > +		       enum ctu_can_fd_can_registers reg, u32 val)
> > +{
> > +	iowrite32(val, priv->mem_base + reg);
> > +}
>
> And get rid of this kind of abstraction layer.

We need to support big endian and little endian mapping
in same driver. I.e. bigendian MIPS with little endian
PCI mapping (that is standard) and big endian for SoC
integration. I really think that linux/regmap.h is
to big monster for this simple purpose. See my previous
analysis and reference to similar conclusions of M_CAN
authors.

> > +// TODO: rename or do not depend on previous value of id
>
> TODO: grep for TODO and C++ comments before attempting merge.

There should not be any C++ comment except SPDX, which seems to be
preffered even in other kernel C files and TODOs. They go after
my colleagues providing basic HW abstraction. I have replaced C++
comments in ctucanfd_hw.h.

Code restructured to resolve this TODO.

> > +static bool ctucan_hw_len_to_dlc(u8 len, u8 *dlc)
> > +{
> > +	*dlc = can_len2dlc(len);
> > +	return true;
> > +}
>
> Compared to how well other code is documented... This one is voodoo.

Wrapper removed, can_len2dlc used directly.

Generally ctucanfd_hw.h/ctucanfd_hw.c provides documented API
to the driver. Who wants to poke with ctucanfd_hw.h/ctucanfd_hw.c
should read HW, registers docs, to which code directly corresponds.

  http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf
  http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/ctu_can_fd_architecture.pdf

> > +bool ctucan_hw_set_ret_limit(struct ctucan_hw_priv *priv, bool enable,
> > u8 limit) +{
> > +	union ctu_can_fd_mode_settings reg;
> > +
> > +	if (limit > CTU_CAN_FD_RETR_MAX)
> > +		return false;
> > +
> > +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE);
> > +	reg.s.rtrle = enable ? RTRLE_ENABLED : RTRLE_DISABLED;
> > +	reg.s.rtrth = limit & 0xF;
> > +	priv->write_reg(priv, CTU_CAN_FD_MODE, reg.u32);
> > +	return true;
> > +}
>
> As elsewhere, I'd suggest 0/-ERRNO.

I would prefer this HW documenting layer without dependency
of concrete systems status reporting mechanisms.
EXXX rule can be followed but at cost of testing, other
systems integration etc.

> > +void ctucan_hw_set_mode_reg(struct ctucan_hw_priv *priv,
> > +			    const struct can_ctrlmode *mode)
> > +{
> > +	u32 flags = mode->flags;
> > +	union ctu_can_fd_mode_settings reg;
> > +
> > +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE);
> >
> > +	if (mode->mask & CAN_CTRLMODE_LOOPBACK)
> > +		reg.s.ilbp = flags & CAN_CTRLMODE_LOOPBACK ?
> > +					INT_LOOP_ENABLED : INT_LOOP_DISABLED;
>
> Not sure what is going on here, but having mode->flags in same format
> as hardware register would help...?

Converts SocketCAN defined code to actual encoding in the HDL

https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/netlink.h#L95

So this is to have external API of HW access functions as close to SocketCAN
as possible.

> > +	switch (fnum) {
> > +	case CTU_CAN_FD_FILTER_A:
> > +		if (reg.s.sfa)
> > +			return true;
> > +	break;
> > +	case CTU_CAN_FD_FILTER_B:
> > +		if (reg.s.sfb)
> > +			return true;
> > +	break;
> > +	case CTU_CAN_FD_FILTER_C:
> > +		if (reg.s.sfc)
> > +			return true;
> > +	break;
> > +	}
>
> Check indentation of break statemnts, also elsewhere in this file

Strange that checkpatch accepts this, but changing.

> > +bool ctucan_hw_get_range_filter_support(struct ctucan_hw_priv *priv)
> > +{
> > +	union ctu_can_fd_filter_control_filter_status reg;
> > +
> > +	reg.u32 = priv->read_reg(priv, CTU_CAN_FD_FILTER_CONTROL);
> > +
> > +	if (reg.s.sfr)
> > +		return true;
>
> return !!reg.s.sfr; ?

Replaced

> > +enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct
> > ctucan_hw_priv +							*priv, u8 buf)
>
> ...
>
> > +	default:
> > +		status = ~0;
> > +	}
> > +	return (enum ctu_can_fd_tx_status_tx1s)status;
> > +}
>
> Is ~0 in the enum?

Hmm enum ctu_can_fd_tx_status_tx1s is generated from IPXACT CTU CAN FD specification.
The ~0 is there to catch fatal problems, to put something, which does not match
any enum value. Changing enum is problematic, it would require to change
generator or specification... It is to catch real problem in the code
implementation. So I am not sure what else I can do there.

> > +	// TODO: use named constants for the command
>
> TODO...
>
> > +// TODO: AL_CAPTURE and ERROR_CAPTURE

Removed, I am not sure what was on the mind of colleagues who has placed this TODO there.

> > +#if defined(__LITTLE_ENDIAN_BITFIELD) == defined(__BIG_ENDIAN_BITFIELD)
> > +# error __BIG_ENDIAN_BITFIELD or __LITTLE_ENDIAN_BITFIELD must be
> > defined. +#endif
> >
> :-).
> :
> > +// True if Core is transceiver of current frame
> > +#define CTU_CAN_FD_IS_TRANSMITTER(stat) (!!(stat).ts)
> > +
> > +// True if Core is receiver of current frame
> > +#define CTU_CAN_FD_IS_RECEIVER(stat) (!!(stat).s.rxs)
>
> Why not, documentation is nice. But it is in big contrast to other
> parts of code where there's no docs at all.

The ctucanfd_hw.h API should be documented for driver implementers.
ctucanfd_hw.c require to read real HW docs.

> > +struct ctucan_hw_priv;
> > +#ifndef ctucan_hw_priv
> > +struct ctucan_hw_priv {
> > +	void __iomem *mem_base;
> > +	u32 (*read_reg)(struct ctucan_hw_priv *priv,
> > +			enum ctu_can_fd_can_registers reg);
> > +	void (*write_reg)(struct ctucan_hw_priv *priv,
> > +			  enum ctu_can_fd_can_registers reg, u32 val);
> > +};
> > +#endif
>
> Should not be needed in kernel.

Old mechanism, actual user space tests can live without it.

> > +/**
> > + * ctucan_hw_read_rx_word - Reads one word of CAN Frame from RX FIFO
> > Buffer. + *
> > + * @priv: Private info
> > + *
> > + * Return: One wword of received frame
>
> Typo 'word'.
>
> > +++ b/drivers/net/can/ctucanfd/ctu_can_fd_regs.h
> > @@ -0,0 +1,971 @@
> > +
> > +/* This file is autogenerated, DO NOT EDIT! */
> > +
>
> Yay. How is that supposed to work after merge?
>
> Best regards,
> 								Pavel

On Thursday 22 of October 2020 13:39:52 Pavel Machek wrote:
> > @@ -12,4 +12,13 @@ config CAN_CTUCANFD
> >
> >  if CAN_CTUCANFD
> >
> > +config CAN_CTUCANFD_PCI
> > +	tristate "CTU CAN-FD IP core PCI/PCIe driver"
> > +	depends on PCI
> > +	help
> > +	  This driver adds PCI/PCIe support for CTU CAN-FD IP core.
> > +	  The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
> > +	  PCIe board with PiKRON.com designed transceiver riser shield is
> > available +	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
> > +
> >  endif
>
> Ok, now the if in the first patch makes sense. It can stay.
>
> And it is separate module, so EXPORT_SYMBOLs make sense. Ok.

I have changed dependency to not use if but
  depends on CAN_CTUCANFD

> > +#ifndef PCI_VENDOR_ID_TEDIA
> > +#define PCI_VENDOR_ID_TEDIA 0x1760
> > +#endif
> >
> > +#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00
>
> These should go elsewhere.

Kept for now, I will adapt to suggestions but I would
prefer to put it independent to allow easy backports
for meanwhile. 

> > +#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD

Test integration not in use removed.

>
> > +static bool use_msi = 1;
> > +static bool pci_use_second = 1;
>
> true?

Changed to true.

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?

Chanegd to depends on

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

I have done rename in many places to fullfill this single line.
Full rename in HDL would require to analyze 4k+ occurrences.

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.

I have invested time to redraw image in Inscape and do more
optimization to reduce 172K to 16K SVG.

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

What is the best way to cross-reference RST documentation in Linux kernel
sources??

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

I would be happy if it stays in our standalone build.
If it is problem for mainline I try to reduce text.
Help, suggestions etc. much appreciated.
Mr. Ille, Mr. Jerabek and others, please help there.
Same with checking for errors.

Thanks for your time (when you reached the end of the discussion),

Pavel

Re: [PATCH v6 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.

[Pavel Pisa]

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