Hi!

From: Martin Jerabek <redacted>

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.

+if CAN_CTUCANFD
+
+endif

Empty -> drop?

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

Yay. How is that supposed to work after merge?

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