CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation

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 <[email protected]>

Reviewed-by Rob Herring <[email protected]>

From: Martin Jerabek <[email protected]>

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

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

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

drivers/net/can/ctucanfd/ctu_can_fd.c new file +4 -0

+	/* Check for Arbitration Lost interrupt */
+	if (isr.s.ali) {
+		if (dologerr)
+			netdev_info(ndev, "  arbitration lost");

+/**
+ * ctucan_close - Driver close routine
+ * @ndev: Pointer to net_device structure
+ *
+ * Return: 0 always
+ */

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +4 -0

+	struct ctu_can_fd_frame_form_w_s {
+#ifdef __LITTLE_ENDIAN_BITFIELD
+  /* FRAME_FORM_W */
+		uint32_t dlc                     : 4;

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +7 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +4 -0

+#ifdef __KERNEL__
+# include <linux/can/dev.h>
+#else
+/* The hardware registers mapping and low level layer should build

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +4 -0

+	switch (fnum) {
+	case CTU_CAN_FD_FILTER_A:
+		if (reg.s.sfa)
+			return true;

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +2 -0

+enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct ctucan_hw_priv
+							*priv, u8 buf)

+// TODO: AL_CAPTURE and ERROR_CAPTURE

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

Hi!

Quoted 10 lines

From: Martin Jerabek <[email protected]>

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.

drivers/net/can/ctucanfd/Kconfig line 15

Quoted 6 lines

+++ b/drivers/net/can/ctucanfd/Kconfig

drivers/net/can/ctucanfd/Kconfig new file +3 -0

@@ -0,0 +1,15 @@
 
+if CAN_CTUCANFD
+
+endif

Empty -> drop?

Review line 1

Quoted 7 lines

+++ b/drivers/net/can/ctucanfd/Makefile

(quoted diff) +3 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 56

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +7 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 191

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

Quoted 11 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 320

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 347

Quoted 11 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +11 -0

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

Quoted 10 lines

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 508

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

+		}
+	}
+
+	/* Check for Bus Error interrupt */
+	if (isr.s.bei) {
+		netdev_info(ndev, "  bus error");

Missing "if (dologerr)" here?

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +4 -0

+static int ctucan_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *ndev = napi->dev;
+	struct ctucan_priv *priv = netdev_priv(ndev);

drivers/net/can/ctucanfd/ctu_can_fd.c line 544

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 558

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 610

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +5 -0

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

Mismatch between code and docs.

drivers/net/can/ctucanfd/ctu_can_fd.c line 619

Quoted 1 line

drivers/net/can/ctucanfd/ctu_can_fd.c new file +1 -0

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

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 729

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +4 -0

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

Inconsistent.

drivers/net/can/ctucanfd/ctu_can_fd.c line 770

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +5 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 802

Quoted 7 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +7 -0

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

Quoted 6 lines

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 984

Quoted 2 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +2 -0

+EXPORT_SYMBOL(ctucan_suspend);
+EXPORT_SYMBOL(ctucan_resume);

_GPL?

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 989

Quoted 4 lines

+int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq,
unsigned int ntxbufs, + unsigned long can_clk_rate, int pm_enable_call,

drivers/net/can/ctucanfd/ctu_can_fd.c new file +2 -0

+			void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev))
+{

Splitting/simplifying this somehow would be good.

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 49

Quoted 3 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +3 -0

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

u32, as you write elsewhere.

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 63

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 125

Quoted 7 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +7 -0

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

Quoted 11 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +11 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 169

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +6 -0

+		uint32_t data_7                  : 8;
+		uint32_t data_6                  : 8;
+		uint32_t data_5                  : 8;
+#endif
+	} s;
+};

even more crazy.

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 44

Quoted 6 lines

+ * in userspace together with headers generated automatically
+ * generated from from IP-XACT/cactus helps to driver to hardware
+ * and QEMU emulation model consistency keeping.
+ */

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +2 -0

+# include "ctu_can_fd_linux_defs.h"
+#endif

Please remove non-kernel code for merge.

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 53

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

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

Quoted 1 line

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

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 118

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

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

Quoted 7 lines

+bool ctucan_hw_set_ret_limit(struct ctucan_hw_priv *priv, bool enable, u8 limit) +{

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

+	union ctu_can_fd_mode_settings reg;
+
+	if (limit > CTU_CAN_FD_RETR_MAX)
+		return false;
+

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 173

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +6 -0

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +7 -0

+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);

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 185

Quoted 3 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +3 -0

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 438

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +6 -0

+	break;
+	case CTU_CAN_FD_FILTER_C:
+		if (reg.s.sfc)
+			return true;
+	break;
+	}

Check indentation of break statemnts, also elsewhere in this file

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 450

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +8 -0

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

Quoted 2 lines

+enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct
ctucan_hw_priv + *priv, u8 buf)

...

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 621

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

+	default:
+		status = ~0;
+	}
+	return (enum ctu_can_fd_tx_status_tx1s)status;
+}

Is ~0 in the enum?

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 658

Quoted 1 line

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +1 -0

+	// TODO: use named constants for the command

TODO...

Quoted 1 line

+// TODO: AL_CAPTURE and ERROR_CAPTURE

Quoted 5 lines

(quoted diff) +2 -0

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

defined. +#endif

:-).

drivers/net/can/ctucanfd/ctu_can_fd_hw.h line 63

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.h new file +5 -0

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

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.h new file +4 -0

+struct ctucan_hw_priv;
+#ifndef ctucan_hw_priv
+struct ctucan_hw_priv {
+	void __iomem *mem_base;

drivers/net/can/ctucanfd/ctu_can_fd_hw.h line 134

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.h new file +6 -0

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

Quoted 6 lines

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 3

Quoted 5 lines

+++ b/drivers/net/can/ctucanfd/ctu_can_fd_regs.h

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +3 -0

@@ -0,0 +1,971 @@
+
+/* This file is autogenerated, DO NOT EDIT! */
+

Yay. How is that supposed to work after merge?

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 <[email protected]> wrote:

Quoted 18 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 1 line

Quoted 1 line

This + * function uses the next available free txbuf and populates

their

Quoted 5 lines

Quoted 4 lines

fields to + * start the transmission.
+ *
+ * Return: %NETDEV_TX_OK on success and failure value on error
+ */

Quoted 4 lines

Quoted 2 lines

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?

Quoted 10 lines

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd.c line 508

Quoted 3 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +3 -0

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

Quoted 13 lines

Quoted 12 lines

drivers/net/can/ctucanfd/ctu_can_fd.c line 544

Quoted 10 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +10 -0

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

Quoted 2 lines

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

Quoted 6 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +7 -0

+   {
+           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);
+   }

Quoted 5 lines

Quoted 1 line

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.

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

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

Quoted 6 lines

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd.c line 984

Quoted 2 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +2 -0

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

Quoted 8 lines

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.

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

Quoted 1 line

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

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.

Hi!

Quoted 10 lines

From: Martin Jerabek <[email protected]>

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.

drivers/net/can/ctucanfd/Kconfig line 15

Quoted 6 lines

+++ b/drivers/net/can/ctucanfd/Kconfig

drivers/net/can/ctucanfd/Kconfig new file +3 -0

@@ -0,0 +1,15 @@
 
+if CAN_CTUCANFD
+
+endif

Empty -> drop?

Review line 1

Quoted 7 lines

+++ b/drivers/net/can/ctucanfd/Makefile

(quoted diff) +3 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 56

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +7 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 191

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

Quoted 11 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 320

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +5 -0

+	spin_lock_irqsave(&priv->tx_lock, flags);
+
+	ctucan_hw_txt_set_rdy(&priv->p, txb_id);
+
+	priv->txb_head++;

drivers/net/can/ctucanfd/ctu_can_fd.c line 347

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

Quoted 10 lines

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 508

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

+		}
+	}
+
+	/* Check for Bus Error interrupt */
+	if (isr.s.bei) {
+		netdev_info(ndev, "  bus error");

Missing "if (dologerr)" here?

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +4 -0

+static int ctucan_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *ndev = napi->dev;
+	struct ctucan_priv *priv = netdev_priv(ndev);

drivers/net/can/ctucanfd/ctu_can_fd.c line 544

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 558

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 610

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +5 -0

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

Mismatch between code and docs.

drivers/net/can/ctucanfd/ctu_can_fd.c line 619

Quoted 1 line

drivers/net/can/ctucanfd/ctu_can_fd.c new file +1 -0

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

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 729

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +4 -0

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

Inconsistent.

drivers/net/can/ctucanfd/ctu_can_fd.c line 770

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +5 -0

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 802

Quoted 7 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +7 -0

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

Quoted 6 lines

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 984

Quoted 2 lines

drivers/net/can/ctucanfd/ctu_can_fd.c new file +2 -0

+EXPORT_SYMBOL(ctucan_suspend);
+EXPORT_SYMBOL(ctucan_resume);

_GPL?

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

drivers/net/can/ctucanfd/ctu_can_fd.c line 989

Quoted 4 lines

+int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq,
unsigned int ntxbufs, + unsigned long can_clk_rate, int pm_enable_call,

drivers/net/can/ctucanfd/ctu_can_fd.c new file +2 -0

+			void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev))
+{

Splitting/simplifying this somehow would be good.

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 49

Quoted 3 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +3 -0

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

u32, as you write elsewhere.

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 63

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +6 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 125

Quoted 7 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +7 -0

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

Quoted 11 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +11 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 169

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +6 -0

+		uint32_t data_7                  : 8;
+		uint32_t data_6                  : 8;
+		uint32_t data_5                  : 8;
+#endif
+	} s;
+};

even more crazy.

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 44

Quoted 6 lines

+ * in userspace together with headers generated automatically
+ * generated from from IP-XACT/cactus helps to driver to hardware
+ * and QEMU emulation model consistency keeping.
+ */

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +2 -0

+# include "ctu_can_fd_linux_defs.h"
+#endif

Please remove non-kernel code for merge.

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 53

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

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

Quoted 1 line

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

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 118

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

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

Quoted 7 lines

+bool ctucan_hw_set_ret_limit(struct ctucan_hw_priv *priv, bool enable, u8 limit) +{

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

+	union ctu_can_fd_mode_settings reg;
+
+	if (limit > CTU_CAN_FD_RETR_MAX)
+		return false;
+

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 173

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +6 -0

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +7 -0

+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);

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 185

Quoted 3 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +3 -0

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

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +8 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 438

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +6 -0

+	break;
+	case CTU_CAN_FD_FILTER_C:
+		if (reg.s.sfc)
+			return true;
+	break;
+	}

Check indentation of break statemnts, also elsewhere in this file

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 450

Quoted 8 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +8 -0

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

Quoted 2 lines

+enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct
ctucan_hw_priv + *priv, u8 buf)

...

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 621

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +5 -0

+	default:
+		status = ~0;
+	}
+	return (enum ctu_can_fd_tx_status_tx1s)status;
+}

Is ~0 in the enum?

drivers/net/can/ctucanfd/ctu_can_fd_hw.c line 658

Quoted 1 line

drivers/net/can/ctucanfd/ctu_can_fd_hw.c new file +1 -0

+	// TODO: use named constants for the command

TODO...

Quoted 1 line

+// TODO: AL_CAPTURE and ERROR_CAPTURE

Quoted 4 lines

(quoted diff) +2 -0

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

defined. +#endif

:-).
:

drivers/net/can/ctucanfd/ctu_can_fd_hw.h line 63

Quoted 5 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.h new file +5 -0

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

Quoted 4 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.h new file +4 -0

+struct ctucan_hw_priv;
+#ifndef ctucan_hw_priv
+struct ctucan_hw_priv {
+	void __iomem *mem_base;

drivers/net/can/ctucanfd/ctu_can_fd_hw.h line 134

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_hw.h new file +6 -0

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

Quoted 6 lines

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

drivers/net/can/ctucanfd/ctu_can_fd_frame.h line 3

Quoted 5 lines

+++ b/drivers/net/can/ctucanfd/ctu_can_fd_regs.h

drivers/net/can/ctucanfd/ctu_can_fd_frame.h new file +3 -0

@@ -0,0 +1,971 @@
+
+/* This file is autogenerated, DO NOT EDIT! */
+

Yay. How is that supposed to work after merge?

Best regards,
Pavel

Quoted 11 lines

(quoted diff) +7 -0

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

Quoted 3 lines

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.

Review

Quoted 5 lines

(quoted diff) +4 -0

+#ifndef PCI_VENDOR_ID_TEDIA
+#define PCI_VENDOR_ID_TEDIA 0x1760
+#endif
 
+#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00

These should go elsewhere.

Quoted 1 line

+#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST 0xCAFD

Review

Quoted 3 lines

(quoted diff) +2 -0

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

true?

Hi!

Quoted 5 lines

+++ b/drivers/net/can/ctucanfd/Kconfig

(quoted diff) +0 -0

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

Review

Quoted 4 lines

(quoted diff) +4 -0

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

Review ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o · line 13

Quoted 7 lines

(quoted diff) +3 -0

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

Quoted 7 lines · Pavel Pisa

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.

Quoted 5 lines

(quoted diff) +5 -0

+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

Review

Quoted 6 lines

(quoted diff) +2 -0

+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

(quoted diff) +2 -0

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

Quoted 2 lines

+Device probe
+~~~~~~~~~~~~

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

drivers/net/can/ctucanfd/Kconfig +0 -0

@@ -12,4 +12,13 @@ config CAN_CTUCANFD
 
 if CAN_CTUCANFD
 

Hi!

Quoted 11 lines

drivers/net/can/ctucanfd/Kconfig +7 -0

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

Quoted 3 lines

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.

drivers/net/can/ctucanfd/ctu_can_fd_pci.c line 49

Quoted 6 lines

drivers/net/can/ctucanfd/ctu_can_fd_pci.c new file +5 -0

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

drivers/net/can/ctucanfd/ctu_can_fd_pci.c line 46

Quoted 3 lines

drivers/net/can/ctucanfd/ctu_can_fd_pci.c new file +3 -0

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

Quoted 1 line

+#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST 0xCAFD

Quoted 1 line

+#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00

drivers/net/can/ctucanfd/ctu_can_fd_pci.c line 62

Quoted 2 lines

drivers/net/can/ctucanfd/ctu_can_fd_pci.c new file +2 -0

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

true?

Hi!

Quoted 5 lines

+++ b/drivers/net/can/ctucanfd/Kconfig

(quoted diff) +0 -0

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

drivers/net/can/ctucanfd/Kconfig config CAN_CTUCANFD_PCI · line 27

Quoted 4 lines

drivers/net/can/ctucanfd/Kconfig +4 -0

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

Can this be

        depends on OF || COMPILE_TEST

?

drivers/net/can/ctucanfd/Makefile ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o · line 13

Quoted 7 lines

drivers/net/can/ctucanfd/Makefile +3 -0

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

Quoted 5 lines

Quoted 5 lines

+++ b/drivers/net/can/ctucanfd/Kconfig

(quoted diff) +0 -0

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

Quoted 7 lines

drivers/net/can/ctucanfd/Kconfig config CAN_CTUCANFD_PCI · line 27

Quoted 4 lines

drivers/net/can/ctucanfd/Kconfig +4 -0

+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

Quoted 3 lines · 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.

Quoted 9 lines

drivers/net/can/ctucanfd/Makefile ctucanfd-y := ctu_can_fd.o ctu_can_fd_hw.o · line 13

Quoted 7 lines

drivers/net/can/ctucanfd/Makefile +3 -0

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

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

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +8 -0

+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

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +22 -0

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

Quoted 7 lines · Pavel Pisa

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.

Quoted 5 lines

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +5 -0

+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

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst line 46

Quoted 6 lines

(quoted diff) +2 -0

+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

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +2 -0

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

Quoted 13 lines

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +13 -0

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

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst line 65

Quoted 5 lines

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +5 -0

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

Quoted 28 lines

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +28 -0

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

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst line 137

Quoted 4 lines

Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst new file +4 -0

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