[PATCH v2 0/2] Allwinner SoC CAN controller Support

[PATCH v2 0/2] Allwinner SoC CAN controller Support

[Gerhard Bertelsmann]

Hi Marc,

thanks for your feedback. Hopefully this one looks better :-)
 
Allwinner A10/A20 CAN Controller support

[PATCH v2 1/2] Device Tree Binding Documentation
[PATCH v2 2/2] Kernel Module


Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
Tested-by; Gerhard Bertelsmann < info@gerhard-bertelsmann.de >

[PATCH v2 1/2] Allwinner SoC CAN controller Support

[Gerhard Bertelsmann]

Devicetree Bindings documentation

Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >


.../devicetree/bindings/net/can/sunxi_can.txt      |  42 ++
 1 files changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/sunxi_can.txt b/Documentation/devicetree/bindings/net/can/sunxi_can.txt
new file mode 100644
index 0000000..454e92f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/sunxi_can.txt
@@ -0,0 +1,42 @@
+Allwinner A10/A20 CAN controller Device Tree Bindings
+-----------------------------------------------------
+
+Required properties:
+- compatible: "allwinner,sunxican"
+- reg: physical base address and size of the Allwinner A10/A20 CAN register map.
+- interrupts: interrupt specifier for the sole interrupt.
+- clocks: phandle and clock specifier.
+- pinctrl-0: pin control group to be used for this controller.
+- pinctrl-names: must be "default".
+
+
+Example
+-------
+
+SoC common .dtsi file:
+
+	can0_pins_a: can0@0 {
+		allwinner,pins = "PH20","PH21";
+		allwinner,function = "can";
+		allwinner,drive = <0>;
+		allwinner,pull = <0>;
+	};
+
+	can0: can@01c2bc00 {
+		compatible = "allwinner,sunxican";
+		reg = <0x01c2bc00 0x400>;
+		interrupts = <0 26 4>;
+		clocks = <&apb1_gates 4>;
+		status = "disabled";
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+Board specific .dts file:
+
+	can0: can@01c2bc00 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&can0_pins_a>;
+		status = "okay";
+	};
+

Re: [PATCH v2 1/2] Allwinner SoC CAN controller Support

[Marc Kleine-Budde]

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

On 09/03/2015 12:09 PM, Gerhard Bertelsmann wrote:
> Devicetree Bindings documentation
> 
> Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
                                      ^                           ^
nitpick: no space here.

Please format your subject like: "can: add Allwinner SoC CAN controller
Devicetree Bindings documentation".

> 
> 
> .../devicetree/bindings/net/can/sunxi_can.txt      |  42 ++
>  1 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/sunxi_can.txt b/Documentation/devicetree/bindings/net/can/sunxi_can.txt
> new file mode 100644
> index 0000000..454e92f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/sunxi_can.txt
> @@ -0,0 +1,42 @@
> +Allwinner A10/A20 CAN controller Device Tree Bindings
> +-----------------------------------------------------
> +
> +Required properties:
> +- compatible: "allwinner,sunxican"
> +- reg: physical base address and size of the Allwinner A10/A20 CAN register map.
> +- interrupts: interrupt specifier for the sole interrupt.
> +- clocks: phandle and clock specifier.
> +- pinctrl-0: pin control group to be used for this controller.
> +- pinctrl-names: must be "default".
> +
> +
> +Example
> +-------
> +
> +SoC common .dtsi file:
> +
> +	can0_pins_a: can0@0 {
> +		allwinner,pins = "PH20","PH21";
> +		allwinner,function = "can";
> +		allwinner,drive = <0>;
> +		allwinner,pull = <0>;
> +	};
> +	can0: can@01c2bc00 {
> +		compatible = "allwinner,sunxican";
> +		reg = <0x01c2bc00 0x400>;
> +		interrupts = <0 26 4>;
> +		clocks = <&apb1_gates 4>;
> +		status = "disabled";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Why address and size cells?

> +	};
> +
> +Board specific .dts file:
> +
> +	can0: can@01c2bc00 {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&can0_pins_a>;
> +		status = "okay";
> +	};
> +
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[PATCH v2 2/2] Allwinner SoC CAN controller Support

[Gerhard Bertelsmann]

Second try:
 Allwinner A10/A20 CAN Controller support


Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
Tested-by; Gerhard Bertelsmann < info@gerhard-bertelsmann.de >


 drivers/net/can/Kconfig                            |  10 +
 drivers/net/can/Makefile                           |   1 +
 drivers/net/can/sunxi_can.c                        | 832 +++++++++++++++++++++
 3 files changed, 843 insertions(+)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e8c96b8..29b62e6 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -129,6 +129,16 @@ config CAN_RCAR
 	  To compile this driver as a module, choose M here: the module will
 	  be called rcar_can.
 
+config CAN_SUNXI
+	tristate "Allwinner SUNXI CAN controller"
+	depends on MACH_SUN4I || MACH_SUN7I
+	---help---
+	  Say Y here if you want to use CAN controller found on Allwinner
+	  A10/A20 SoCs.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called sunxi_can.
+
 config CAN_XILINXCAN
 	tristate "Xilinx CAN"
 	depends on ARCH_ZYNQ || ARM64 || MICROBLAZE || COMPILE_TEST
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index c533c62..b3e825c 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
 obj-$(CONFIG_PCH_CAN)		+= pch_can.o
 obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
 obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
+obj-$(CONFIG_CAN_SUNXI)		+= sunxi_can.o
 obj-$(CONFIG_CAN_XILINXCAN)	+= xilinx_can.o
 
 subdir-ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/net/can/sunxi_can.c b/drivers/net/can/sunxi_can.c
new file mode 100644
index 0000000..6e3a5e5
--- /dev/null
+++ b/drivers/net/can/sunxi_can.c
@@ -0,0 +1,832 @@
+/*
+ * sunxi_can.c - CAN bus controller driver for Allwinner SUN4I&SUN7I based SoCs
+ *
+ * Copyright (C) 2013 Peter Chen
+ * Copyright (C) 2015 Gerhard Bertelsmann
+ *
+ * Parts of this software are based on (derived from) the SJA1000 code by:
+ *   Copyright (C) 2014 Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
+ *   Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
+ *   Copyright (C) 2002-2007 Volkswagen Group Electronic Research
+ *   Copyright (C) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
+ *   38106 Braunschweig, GERMANY
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/netdevice.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+#include <linux/can/led.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "sunxi_can"
+#define DRV_MODULE_VERSION "0.94"
+
+/* Registers address */
+#define CAN_BASE0		0x01C2BC00
+#define CAN_MSEL_ADDR		0x0000	/* CAN Mode Select */
+#define CAN_CMD_ADDR            0x0004	/* CAN Command */
+#define CAN_STA_ADDR		0x0008	/* CAN Status */
+#define CAN_INT_ADDR		0x000c	/* CAN Interrupt Flag */
+#define CAN_INTEN_ADDR		0x0010	/* CAN Interrupt Enable */
+#define CAN_BTIME_ADDR		0x0014	/* CAN Bus Timing 0 */
+#define CAN_TEWL_ADDR		0x0018	/* CAN Tx Error Warning Limit */
+#define CAN_ERRC_ADDR		0x001c	/* CAN Error Counter */
+#define CAN_RMCNT_ADDR		0x0020	/* CAN Receive Message Counter */
+#define CAN_RBUFSA_ADDR		0x0024	/* CAN Receive Buffer Start Address */
+#define CAN_BUF0_ADDR		0x0040	/* CAN Tx/Rx Buffer 0 */
+#define CAN_BUF1_ADDR		0x0044	/* CAN Tx/Rx Buffer 1 */
+#define CAN_BUF2_ADDR		0x0048	/* CAN Tx/Rx Buffer 2 */
+#define CAN_BUF3_ADDR		0x004c	/* CAN Tx/Rx Buffer 3 */
+#define CAN_BUF4_ADDR		0x0050	/* CAN Tx/Rx Buffer 4 */
+#define CAN_BUF5_ADDR		0x0054	/* CAN Tx/Rx Buffer 5 */
+#define CAN_BUF6_ADDR		0x0058	/* CAN Tx/Rx Buffer 6 */
+#define CAN_BUF7_ADDR		0x005c	/* CAN Tx/Rx Buffer 7 */
+#define CAN_BUF8_ADDR		0x0060	/* CAN Tx/Rx Buffer 8 */
+#define CAN_BUF9_ADDR		0x0064	/* CAN Tx/Rx Buffer 9 */
+#define CAN_BUF10_ADDR		0x0068	/* CAN Tx/Rx Buffer 10 */
+#define CAN_BUF11_ADDR		0x006c	/* CAN Tx/Rx Buffer 11 */
+#define CAN_BUF12_ADDR		0x0070	/* CAN Tx/Rx Buffer 12 */
+#define CAN_ACPC_ADDR		0x0040	/* CAN Acceptance Code 0 */
+#define CAN_ACPM_ADDR		0x0044	/* CAN Acceptance Mask 0 */
+#define CAN_RBUF_RBACK_START_ADDR	0x0180	/* CAN transmit buffer start */
+#define CAN_RBUF_RBACK_END_ADDR		0x01b0	/* CAN transmit buffer end */
+
+/* Controller Register Description */
+
+/* mode select register (r/w)
+ * offset:0x0000 default:0x0000_0001
+ */
+#define SLEEP_MODE		(0x01 << 4)	/* write in reset mode */
+#define WAKE_UP			(0x00 << 4)
+#define SINGLE_FILTER		(0x01 << 3)	/* write in reset mode */
+#define DUAL_FILTERS		(0x00 << 3)
+#define LOOPBACK_MODE		BIT(2)
+#define LISTEN_ONLY_MODE	BIT(1)
+#define RESET_MODE		BIT(0)
+
+/* command register (w)
+ * offset:0x0004 default:0x0000_0000
+ */
+#define BUS_OFF_REQ		BIT(5)
+#define SELF_RCV_REQ		BIT(4)
+#define CLEAR_OR_FLAG		BIT(3)
+#define RELEASE_RBUF		BIT(2)
+#define ABORT_REQ		BIT(1)
+#define TRANS_REQ		BIT(0)
+
+/* status register (r)
+ * offset:0x0008 default:0x0000_003c
+ */
+#define BIT_ERR			(0x00 << 22)
+#define FORM_ERR		(0x01 << 22)
+#define STUFF_ERR		(0x02 << 22)
+#define OTHER_ERR		(0x03 << 22)
+#define ERR_DIR			BIT(21)
+#define ERR_SEG_CODE		(0x1f << 16)
+#define START			(0x03 << 16)
+#define ID28_21			(0x02 << 16)
+#define ID20_18			(0x06 << 16)
+#define SRTR			(0x04 << 16)
+#define IDE			(0x05 << 16)
+#define ID17_13			(0x07 << 16)
+#define ID12_5			(0x0f << 16)
+#define ID4_0			(0x0e << 16)
+#define RTR			(0x0c << 16)
+#define RB1			(0x0d << 16)
+#define RB0			(0x09 << 16)
+#define DLEN			(0x0b << 16)
+#define DATA_FIELD		(0x0a << 16)
+#define CRC_SEQUENCE		(0x08 << 16)
+#define CRC_DELIMITER		(0x18 << 16)
+#define ACK			(0x19 << 16)
+#define ACK_DELIMITER		(0x1b << 16)
+#define END			(0x1a << 16)
+#define INTERMISSION		(0x12 << 16)
+#define ACTIVE_ERROR		(0x11 << 16)
+#define PASSIVE_ERROR		(0x16 << 16)
+#define TOLERATE_DOMINANT_BITS	(0x13 << 16)
+#define ERROR_DELIMITER		(0x17 << 16)
+#define OVERLOAD		(0x1c << 16)
+#define BUS_OFF			BIT(7)
+#define ERR_STA			BIT(6)
+#define TRANS_BUSY		BIT(5)
+#define RCV_BUSY		BIT(4)
+#define TRANS_OVER		BIT(3)
+#define TBUF_RDY		BIT(2)
+#define DATA_ORUN		BIT(1)
+#define RBUF_RDY		BIT(0)
+
+/* interrupt register (r)
+ * offset:0x000c default:0x0000_0000
+ */
+#define BUS_ERR			BIT(7)
+#define ARB_LOST		BIT(6)
+#define ERR_PASSIVE		BIT(5)
+#define WAKEUP			BIT(4)
+#define DATA_OR			BIT(3)
+#define ERR_WRN			BIT(2)
+#define TBUF_VLD		BIT(1)
+#define RBUF_VLD		BIT(0)
+
+/* interrupt enable register (r/w)
+ * offset:0x0010 default:0x0000_0000
+ */
+#define BERR_IRQ_EN		BIT(7)
+#define ARB_LOST_IRQ_EN		BIT(6)
+#define ERR_PASSIVE_IRQ_EN	BIT(5)
+#define WAKEUP_IRQ_EN		BIT(4)
+#define OR_IRQ_EN		BIT(3)
+#define ERR_WRN_IRQ_EN		BIT(2)
+#define TX_IRQ_EN		BIT(1)
+#define RX_IRQ_EN		BIT(0)
+
+/* error code */
+#define ERR_INRCV		(0x1 << 5)
+#define ERR_INTRANS		(0x0 << 5)
+
+/* filter mode */
+#define FILTER_CLOSE		0
+#define SINGLE_FLTER_MODE	1
+#define DUAL_FILTER_MODE	2
+
+/* max. number of interrupts handled in ISR */
+#define SUNXI_CAN_MAX_IRQ	20
+
+struct sunxican_priv {
+	struct can_priv can;
+	struct sk_buff *echo_skb;
+	void __iomem *base;
+	struct clk *clk;
+	spinlock_t cmdreg_lock;	/* lock for concurrent cmd register writes */
+};
+
+static const struct can_bittiming_const sunxican_bittiming_const = {
+	.name = DRV_NAME,
+	.tseg1_min = 1,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 64,
+	.brp_inc = 1,
+};
+
+static void sunxi_can_write_cmdreg(struct sunxican_priv *priv, u8 val)
+{
+	unsigned long flags;
+
+	 /* The command register needs some locking and time to settle
+	  * the write_reg() operation - especially on SMP systems.
+	  */
+	spin_lock_irqsave(&priv->cmdreg_lock, flags);
+	writel(val, priv->base + CAN_CMD_ADDR);
+	readl(priv->base + CAN_STA_ADDR);
+	spin_unlock_irqrestore(&priv->cmdreg_lock, flags);
+}
+
+static void set_normal_mode(struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	u8 status = readl(priv->base + CAN_MSEL_ADDR);
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if ((status & RESET_MODE) == 0) {
+			priv->can.state = CAN_STATE_ERROR_ACTIVE;
+			/* enable interrupts */
+			if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
+				writel(0xFFFF, priv->base + CAN_INTEN_ADDR);
+			} else {
+				writel(0xFFFF & ~BERR_IRQ_EN,
+				       priv->base + CAN_INTEN_ADDR);
+			}
+
+			if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
+				/* Put device into loopback mode */
+				writel(readl(priv->base + CAN_MSEL_ADDR) |
+				       LOOPBACK_MODE,
+				       priv->base + CAN_MSEL_ADDR);
+			} else if
+				(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+				/* Put device into listen-only mode */
+				writel(readl(priv->base + CAN_MSEL_ADDR) |
+				       LISTEN_ONLY_MODE,
+				       priv->base + CAN_MSEL_ADDR);
+			}
+			return;
+		}
+		/* set chip to normal mode */
+		writel(readl(priv->base + CAN_MSEL_ADDR) & (~RESET_MODE),
+		       priv->base + CAN_MSEL_ADDR);
+		status = readl(priv->base + CAN_MSEL_ADDR);
+	}
+	netdev_err(dev, "setting controller into normal mode failed!\n");
+}
+
+static void set_reset_mode(struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	u8 status = readl(priv->base + CAN_MSEL_ADDR);
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if (status & RESET_MODE) {
+			priv->can.state = CAN_STATE_STOPPED;
+			return;
+		}
+		/* select reset mode */
+		writel(readl(priv->base + CAN_MSEL_ADDR) |
+			RESET_MODE, priv->base + CAN_MSEL_ADDR);
+		status = readl(priv->base + CAN_MSEL_ADDR);
+	}
+	netdev_err(dev, "setting controller into reset mode failed!\n");
+}
+
+static int sunxican_set_bittiming(struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	u32 cfg;
+
+	cfg = ((bt->brp - 1) & 0x3FF) |
+	    (((bt->sjw - 1) & 0x3) << 14) |
+	    (((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) << 16) |
+	    (((bt->phase_seg2 - 1) & 0x7) << 20);
+	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		cfg |= 0x800000;
+
+	netdev_info(dev, "setting BITTIMING=0x%08x\n", cfg);
+
+	/* CAN_BTIME_ADDR only writable in reset mode */
+	set_reset_mode(dev);
+	writel(cfg, priv->base + CAN_BTIME_ADDR);
+	set_normal_mode(dev);
+
+	return 0;
+}
+
+static int sunxican_get_berr_counter(const struct net_device *dev,
+				     struct can_berr_counter *bec)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	u32 errors;
+
+	errors = readl(priv->base + CAN_ERRC_ADDR);
+	bec->txerr = errors & 0x000F;
+	bec->rxerr = (errors >> 16) & 0x000F;
+	return 0;
+}
+
+static void sunxi_can_start(struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+
+	/* leave reset mode */
+	if (priv->can.state != CAN_STATE_STOPPED)
+		set_reset_mode(dev);
+
+	/* set filters - we accept all */
+	writel(0x00000000, priv->base + CAN_ACPC_ADDR);
+	writel(0xffffffff, priv->base + CAN_ACPM_ADDR);
+
+	/* Clear error counters and error code capture */
+	writel(0x0, priv->base + CAN_ERRC_ADDR);
+
+	/* leave reset mode */
+	set_normal_mode(dev);
+}
+
+static int sunxican_set_mode(struct net_device *dev, enum can_mode mode)
+{
+	switch (mode) {
+	case CAN_MODE_START:
+		sunxi_can_start(dev);
+		if (netif_queue_stopped(dev))
+			netif_wake_queue(dev);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+/* transmit a CAN message
+ * message layout in the sk_buff should be like this:
+ * xx xx xx xx         ff         ll 00 11 22 33 44 55 66 77
+ * [ can_id ] [flags] [len] [can data (up to 8 bytes]
+ */
+static int sunxican_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	u8 dlc;
+	canid_t id;
+	u32 temp = 0;
+	int i;
+
+	/* wait buffer ready */
+	while (!(readl(priv->base + CAN_STA_ADDR) & TBUF_RDY))
+		usleep_range(10, 100);
+
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
+
+	netif_stop_queue(dev);
+
+	dlc = cf->can_dlc;
+	id = cf->can_id;
+
+	temp = ((id >> 30) << 6) | dlc;
+	writel(temp, priv->base + CAN_BUF0_ADDR);
+	if (id & CAN_EFF_FLAG) {
+		/* extended frame */
+		writel((id >> 21) & 0xFF, priv->base + CAN_BUF1_ADDR);
+		writel((id >> 13) & 0xFF, priv->base + CAN_BUF2_ADDR);
+		writel((id >> 5)  & 0xFF, priv->base + CAN_BUF3_ADDR);
+		writel((id & 0x1F) << 3,  priv->base + CAN_BUF4_ADDR);
+
+		for (i = 0; i < dlc; i++) {
+			writel(cf->data[i],
+			       priv->base + (CAN_BUF5_ADDR + i * 4));
+		}
+	} else {
+		/* standard frame */
+		writel((id >> 3) & 0xFF, priv->base + CAN_BUF1_ADDR);
+		writel((id & 0x7) << 5, priv->base + CAN_BUF2_ADDR);
+
+		for (i = 0; i < dlc; i++)
+			writel(cf->data[i], priv->base + CAN_BUF3_ADDR + i * 4);
+	}
+	can_put_echo_skb(skb, dev, 0);
+	sunxi_can_write_cmdreg(priv, TRANS_REQ);
+
+	return NETDEV_TX_OK;
+}
+
+static void sunxi_can_rx(struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u8 fi;
+	canid_t id;
+	int i;
+
+	/* create zero'ed CAN frame buffer */
+	skb = alloc_can_skb(dev, &cf);
+	if (!skb)
+		return;
+
+	fi = readl(priv->base + CAN_BUF0_ADDR);
+	cf->can_dlc = get_can_dlc(fi & 0x0F);
+	if (fi >> 7) {
+		/* extended frame format (EFF) */
+		id = (readl(priv->base + CAN_BUF1_ADDR) << 21) |
+		     (readl(priv->base + CAN_BUF2_ADDR) << 13) |
+		     (readl(priv->base + CAN_BUF3_ADDR) << 5)  |
+		    ((readl(priv->base + CAN_BUF4_ADDR) >> 3) & 0x1f);
+		id |= CAN_EFF_FLAG;
+
+		/* remote transmission request ? */
+		if ((fi >> 6) & 0x1) {
+			id |= CAN_RTR_FLAG;
+		} else {
+			for (i = 0; i < cf->can_dlc; i++)
+				cf->data[i] =
+				    readl(priv->base + CAN_BUF5_ADDR + i * 4);
+		}
+	} else {
+		/* standard frame format (SFF) */
+		id = (readl(priv->base + CAN_BUF1_ADDR) << 3) |
+		    ((readl(priv->base + CAN_BUF2_ADDR) >> 5) & 0x7);
+
+		/* remote transmission request ? */
+		if ((fi >> 6) & 0x1) {
+			id |= CAN_RTR_FLAG;
+		} else {
+			for (i = 0; i < cf->can_dlc; i++)
+				cf->data[i] =
+				    readl(priv->base + CAN_BUF3_ADDR + i * 4);
+		}
+	}
+	cf->can_id = id;
+
+	/* release receive buffer */
+	sunxi_can_write_cmdreg(priv, RELEASE_RBUF);
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+	netif_rx(skb);
+
+	can_led_event(dev, CAN_LED_EVENT_RX);
+}
+
+static int sunxi_can_err(struct net_device *dev, u8 isrc, u8 status)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	enum can_state state = priv->can.state;
+	u32 ecc, alc;
+
+	skb = alloc_can_err_skb(dev, &cf);
+	if (!skb)
+		return -ENOMEM;
+
+	if (isrc & DATA_OR) {
+		/* data overrun interrupt */
+		netdev_dbg(dev, "data overrun interrupt\n");
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		stats->rx_over_errors++;
+		stats->rx_errors++;
+		sunxi_can_write_cmdreg(priv, CLEAR_OR_FLAG);	/* clear bit */
+	}
+	if (isrc & ERR_WRN) {
+		/* error warning interrupt */
+		netdev_dbg(dev, "error warning interrupt\n");
+
+		if (status & BUS_OFF) {
+			state = CAN_STATE_BUS_OFF;
+			cf->can_id |= CAN_ERR_BUSOFF;
+			can_bus_off(dev);
+		} else if (status & ERR_STA) {
+			state = CAN_STATE_ERROR_WARNING;
+		} else {
+			state = CAN_STATE_ERROR_ACTIVE;
+		}
+	}
+	if (isrc & BUS_ERR) {
+		/* bus error interrupt */
+		netdev_dbg(dev, "bus error interrupt\n");
+		priv->can.can_stats.bus_error++;
+		stats->rx_errors++;
+
+		ecc = readl(priv->base + CAN_STA_ADDR);
+
+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+		if (ecc & BIT_ERR) {
+			cf->data[2] |= CAN_ERR_PROT_BIT;
+		} else if (ecc & FORM_ERR) {
+			cf->data[2] |= CAN_ERR_PROT_FORM;
+		} else if (ecc & STUFF_ERR) {
+			cf->data[2] |= CAN_ERR_PROT_STUFF;
+		} else {
+			cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+			cf->data[3] = (ecc & ERR_SEG_CODE) >> 16;
+		}
+		/* error occurred during transmission? */
+		if ((ecc & ERR_DIR) == 0)
+			cf->data[2] |= CAN_ERR_PROT_TX;
+	}
+	if (isrc & ERR_PASSIVE) {
+		/* error passive interrupt */
+		netdev_dbg(dev, "error passive interrupt\n");
+		if (status & ERR_STA)
+			state = CAN_STATE_ERROR_PASSIVE;
+		else
+			state = CAN_STATE_ERROR_ACTIVE;
+	}
+	if (isrc & ARB_LOST) {
+		/* arbitration lost interrupt */
+		netdev_dbg(dev, "arbitration lost interrupt\n");
+		alc = readl(priv->base + CAN_STA_ADDR);
+		priv->can.can_stats.arbitration_lost++;
+		stats->tx_errors++;
+		cf->can_id |= CAN_ERR_LOSTARB;
+		cf->data[0] = (alc & 0x1f) >> 8;
+	}
+	if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
+					 state == CAN_STATE_ERROR_PASSIVE)) {
+		u32 errc = readl(priv->base + CAN_ERRC_ADDR);
+		u8 rxerr = (errc >> 16) & 0xFF;
+		u8 txerr = errc & 0xFF;
+
+		cf->can_id |= CAN_ERR_CRTL;
+		if (state == CAN_STATE_ERROR_WARNING) {
+			priv->can.can_stats.error_warning++;
+			cf->data[1] = (txerr > rxerr) ?
+			    CAN_ERR_CRTL_TX_WARNING : CAN_ERR_CRTL_RX_WARNING;
+		} else {
+			priv->can.can_stats.error_passive++;
+			cf->data[1] = (txerr > rxerr) ?
+			    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
+		}
+		cf->data[6] = txerr;
+		cf->data[7] = rxerr;
+	}
+	priv->can.state = state;
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+	netif_rx(skb);
+
+	return 0;
+}
+
+static irqreturn_t sunxi_can_interrupt(int irq, void *dev_id)
+{
+	struct net_device *dev = (struct net_device *)dev_id;
+	struct sunxican_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	u8 isrc, status;
+	int n = 0;
+
+	while ((isrc = readl(priv->base + CAN_INT_ADDR))
+	       && (n < SUNXI_CAN_MAX_IRQ)) {
+		n++;
+		status = readl(priv->base + CAN_STA_ADDR);
+
+		if (isrc & WAKEUP)
+			netdev_warn(dev, "wakeup interrupt\n");
+
+		if (isrc & TBUF_VLD) {
+			/* transmission complete interrupt */
+			stats->tx_bytes +=
+			    readl(priv->base + CAN_RBUF_RBACK_START_ADDR) & 0xf;
+			stats->tx_packets++;
+			can_get_echo_skb(dev, 0);
+			netif_wake_queue(dev);
+			can_led_event(dev, CAN_LED_EVENT_TX);
+		}
+		if (isrc & RBUF_VLD) {
+			/* receive interrupt */
+			while (status & RBUF_RDY) {
+			/* RX buffer is not empty */
+				sunxi_can_rx(dev);
+				status = readl(priv->base + CAN_STA_ADDR);
+			}
+		}
+		if (isrc &
+		    (DATA_OR | ERR_WRN | BUS_ERR | ERR_PASSIVE | ARB_LOST)) {
+			/* error interrupt */
+			if (sunxi_can_err(dev, isrc, status))
+				break;
+		}
+		/* clear the interrupt */
+		writel(isrc, priv->base + CAN_INT_ADDR);
+	}
+	if (n >= SUNXI_CAN_MAX_IRQ)
+		netdev_dbg(dev, "%d messages handled in ISR", n);
+
+	return (n) ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int sunxican_open(struct net_device *dev)
+{
+	int err;
+
+	/* set chip into reset mode */
+	set_reset_mode(dev);
+
+	/* common open */
+	err = open_candev(dev);
+	if (err)
+		return err;
+
+	/* register interrupt handler */
+	if (request_irq
+	    (dev->irq, sunxi_can_interrupt, IRQF_TRIGGER_NONE, dev->name,
+	     (void *)dev)) {
+		netdev_err(dev, "request_irq err: %d\n", err);
+		return -EAGAIN;
+	}
+	sunxican_set_bittiming(dev);
+	sunxi_can_start(dev);
+
+	can_led_event(dev, CAN_LED_EVENT_OPEN);
+
+	netif_start_queue(dev);
+
+	return 0;
+}
+
+static int sunxican_close(struct net_device *dev)
+{
+	netif_stop_queue(dev);
+	set_reset_mode(dev);
+
+	free_irq(dev->irq, (void *)dev);
+
+	close_candev(dev);
+
+	can_led_event(dev, CAN_LED_EVENT_STOP);
+
+	return 0;
+}
+
+static const struct net_device_ops sunxican_netdev_ops = {
+	.ndo_open = sunxican_open,
+	.ndo_stop = sunxican_close,
+	.ndo_start_xmit = sunxican_start_xmit,
+};
+
+static const struct of_device_id sunxican_of_match[] = {
+	{.compatible = "allwinner,sunxican"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, sunxican_of_match);
+
+static const struct platform_device_id sunxican_id_table[] = {
+	{.name = "sunxican"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(platform, sunxican_id_table);
+
+static int sunxican_remove(struct platform_device *pdev)
+{
+	struct net_device *dev = platform_get_drvdata(pdev);
+	struct sunxican_priv *priv = netdev_priv(dev);
+	struct resource *res;
+
+	set_reset_mode(dev);
+	unregister_netdev(dev);
+	iounmap(priv->base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, resource_size(res));
+
+	free_candev(dev);
+
+	return 0;
+}
+
+static int sunxican_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct clk *clk;
+	void __iomem *addr;
+	int err, irq;
+	u32 temp_irqen;
+	struct net_device *dev;
+	struct sunxican_priv *priv;
+
+	clk = clk_get(&pdev->dev, "apb1_can");
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "no clock defined\n");
+		err = -ENODEV;
+		goto exit;
+	}
+	/* turn on clocking for CAN peripheral block */
+	err = clk_prepare_enable(clk);
+	if (err) {
+		dev_err(&pdev->dev, "could not enable clocking (apb1_can)\n");
+		goto exit;
+	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+
+	if (!res || irq <= 0) {
+		dev_err(&pdev->dev, "could not get a valid irq\n");
+		err = -ENODEV;
+		goto exit_put;
+	}
+	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
+		dev_err(&pdev->dev, "could not get io memory resource\n");
+		err = -EBUSY;
+		goto exit_put;
+	}
+	addr = ioremap_nocache(res->start, resource_size(res));
+	if (!addr) {
+		dev_err(&pdev->dev, "could not map io memory\n");
+		err = -ENOMEM;
+		goto exit_release;
+	}
+	dev = alloc_candev(sizeof(struct sunxican_priv), 1);
+
+	if (!dev) {
+		dev_err(&pdev->dev,
+			"could not allocate memory for CAN device\n");
+		err = -ENOMEM;
+		goto exit_iounmap;
+	}
+
+	dev->netdev_ops = &sunxican_netdev_ops;
+	dev->irq = irq;
+	dev->flags |= IFF_ECHO;
+
+	priv = netdev_priv(dev);
+	priv->can.clock.freq = clk_get_rate(clk);
+	priv->can.bittiming_const = &sunxican_bittiming_const;
+	priv->can.do_set_mode = sunxican_set_mode;
+	priv->can.do_get_berr_counter = sunxican_get_berr_counter;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
+	    CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_LOOPBACK |
+	    CAN_CTRLMODE_3_SAMPLES;
+	priv->base = addr;
+	priv->clk = clk;
+	spin_lock_init(&priv->cmdreg_lock);
+
+	/* enable CAN specific interrupts */
+	set_reset_mode(dev);
+	temp_irqen = BERR_IRQ_EN | ERR_PASSIVE_IRQ_EN | OR_IRQ_EN | RX_IRQ_EN;
+	writel(readl(priv->base + CAN_INTEN_ADDR) | temp_irqen,
+	       priv->base + CAN_INTEN_ADDR);
+
+	platform_set_drvdata(pdev, dev);
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	err = register_candev(dev);
+	if (err) {
+		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
+			DRV_NAME, err);
+		goto exit_free;
+	}
+	devm_can_led_init(dev);
+
+	dev_info(&pdev->dev, "device registered (base=%p, irq=%d)\n",
+		 priv->base, dev->irq);
+
+	return 0;
+
+exit_free:
+	free_candev(dev);
+exit_iounmap:
+	iounmap(addr);
+exit_release:
+	release_mem_region(res->start, resource_size(res));
+exit_put:
+	clk_put(clk);
+exit:
+	return err;
+}
+
+static int __maybe_unused sunxi_can_suspend(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct sunxican_priv *priv = netdev_priv(dev);
+
+	if (netif_running(dev)) {
+		netif_stop_queue(dev);
+		netif_device_detach(dev);
+	}
+	priv->can.state = CAN_STATE_SLEEPING;
+
+	return 0;
+}
+
+static int __maybe_unused sunxi_can_resume(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct sunxican_priv *priv = netdev_priv(dev);
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	if (netif_running(dev)) {
+		netif_device_attach(dev);
+		netif_start_queue(dev);
+	}
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(sunxi_can_pm_ops, sunxi_can_suspend, sunxi_can_resume);
+
+static struct platform_driver sunxi_can_driver = {
+	.driver = {
+		   .name = DRV_NAME,
+		   .owner = THIS_MODULE,
+		   .pm = &sunxi_can_pm_ops,
+		   .of_match_table = sunxican_of_match,
+		   },
+	.probe = sunxican_probe,
+	.remove = sunxican_remove,
+	.id_table = sunxican_id_table,
+};
+
+module_platform_driver(sunxi_can_driver);
+
+MODULE_AUTHOR("Peter Chen <xingkongcp@gmail.com>");
+MODULE_AUTHOR("Gerhard Bertelsmann <info@gerhard-bertelsmann.de>");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION(DRV_NAME "CAN driver for Allwinner SoCs (A10/A20)");
+MODULE_VERSION(DRV_MODULE_VERSION);

Re: [PATCH v2 2/2] Allwinner SoC CAN controller Support

[Andri Yngvason]

Quoting Gerhard Bertelsmann (2015-09-03 10:09:30)
> Second try:
>  Allwinner A10/A20 CAN Controller support
> 
> 
> Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
> Tested-by; Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
> 
> 
>  drivers/net/can/Kconfig                            |  10 +
>  drivers/net/can/Makefile                           |   1 +
>  drivers/net/can/sunxi_can.c                        | 832 +++++++++++++++++++++
>  3 files changed, 843 insertions(+)
> 
[...]

Hi Gerhard,

Have you looked at changes made to sja1000 since 2013? It might be prudent to
apply those changes. ;)

See:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/drivers/net/can/sja1000/sja1000.c

In particular, I noticed that this one is not applied:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=215db1856e8313ef8a1d9b64346dc261570012a6

Cheers,
Andri

Re: [PATCH v2 2/2] Allwinner SoC CAN controller Support

[Marc Kleine-Budde]

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

On 09/03/2015 12:09 PM, Gerhard Bertelsmann wrote:
> Second try:
>  Allwinner A10/A20 CAN Controller support
> 
> 
> Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
> Tested-by; Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
> 
> 
>  drivers/net/can/Kconfig                            |  10 +
>  drivers/net/can/Makefile                           |   1 +
>  drivers/net/can/sunxi_can.c                        | 832 +++++++++++++++++++++
>  3 files changed, 843 insertions(+)
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index e8c96b8..29b62e6 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -129,6 +129,16 @@ config CAN_RCAR
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called rcar_can.
>  
> +config CAN_SUNXI
> +	tristate "Allwinner SUNXI CAN controller"
> +	depends on MACH_SUN4I || MACH_SUN7I
> +	---help---
> +	  Say Y here if you want to use CAN controller found on Allwinner
> +	  A10/A20 SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called sunxi_can.
> +
>  config CAN_XILINXCAN
>  	tristate "Xilinx CAN"
>  	depends on ARCH_ZYNQ || ARM64 || MICROBLAZE || COMPILE_TEST
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index c533c62..b3e825c 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
>  obj-$(CONFIG_PCH_CAN)		+= pch_can.o
>  obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
>  obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
> +obj-$(CONFIG_CAN_SUNXI)		+= sunxi_can.o
>  obj-$(CONFIG_CAN_XILINXCAN)	+= xilinx_can.o
>  
>  subdir-ccflags-y += -D__CHECK_ENDIAN__
> diff --git a/drivers/net/can/sunxi_can.c b/drivers/net/can/sunxi_can.c
> new file mode 100644
> index 0000000..6e3a5e5
> --- /dev/null
> +++ b/drivers/net/can/sunxi_can.c
> @@ -0,0 +1,832 @@
> +/*
> + * sunxi_can.c - CAN bus controller driver for Allwinner SUN4I&SUN7I based SoCs
> + *
> + * Copyright (C) 2013 Peter Chen
> + * Copyright (C) 2015 Gerhard Bertelsmann
> + *
> + * Parts of this software are based on (derived from) the SJA1000 code by:
> + *   Copyright (C) 2014 Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
> + *   Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
> + *   Copyright (C) 2002-2007 Volkswagen Group Electronic Research
> + *   Copyright (C) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
> + *   38106 Braunschweig, GERMANY
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the version 2 of the GNU General Public License
> + * as published by the Free Software Foundation
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/led.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "sunxi_can"
> +#define DRV_MODULE_VERSION "0.94"

Where does the version come from? Are you planing to make use of this?

> +
> +/* Registers address */

Please prefix _all_ your defines with a common prefix, e.g. SUNXI_

> +#define CAN_BASE0		0x01C2BC00

Please remove that define, the base address comes from the DT now.

> +#define CAN_MSEL_ADDR		0x0000	/* CAN Mode Select */
> +#define CAN_CMD_ADDR            0x0004	/* CAN Command */
> +#define CAN_STA_ADDR		0x0008	/* CAN Status */
> +#define CAN_INT_ADDR		0x000c	/* CAN Interrupt Flag */
> +#define CAN_INTEN_ADDR		0x0010	/* CAN Interrupt Enable */
> +#define CAN_BTIME_ADDR		0x0014	/* CAN Bus Timing 0 */
> +#define CAN_TEWL_ADDR		0x0018	/* CAN Tx Error Warning Limit */
> +#define CAN_ERRC_ADDR		0x001c	/* CAN Error Counter */
> +#define CAN_RMCNT_ADDR		0x0020	/* CAN Receive Message Counter */
> +#define CAN_RBUFSA_ADDR		0x0024	/* CAN Receive Buffer Start Address */
> +#define CAN_BUF0_ADDR		0x0040	/* CAN Tx/Rx Buffer 0 */
> +#define CAN_BUF1_ADDR		0x0044	/* CAN Tx/Rx Buffer 1 */
> +#define CAN_BUF2_ADDR		0x0048	/* CAN Tx/Rx Buffer 2 */
> +#define CAN_BUF3_ADDR		0x004c	/* CAN Tx/Rx Buffer 3 */
> +#define CAN_BUF4_ADDR		0x0050	/* CAN Tx/Rx Buffer 4 */
> +#define CAN_BUF5_ADDR		0x0054	/* CAN Tx/Rx Buffer 5 */
> +#define CAN_BUF6_ADDR		0x0058	/* CAN Tx/Rx Buffer 6 */
> +#define CAN_BUF7_ADDR		0x005c	/* CAN Tx/Rx Buffer 7 */
> +#define CAN_BUF8_ADDR		0x0060	/* CAN Tx/Rx Buffer 8 */
> +#define CAN_BUF9_ADDR		0x0064	/* CAN Tx/Rx Buffer 9 */
> +#define CAN_BUF10_ADDR		0x0068	/* CAN Tx/Rx Buffer 10 */
> +#define CAN_BUF11_ADDR		0x006c	/* CAN Tx/Rx Buffer 11 */
> +#define CAN_BUF12_ADDR		0x0070	/* CAN Tx/Rx Buffer 12 */
> +#define CAN_ACPC_ADDR		0x0040	/* CAN Acceptance Code 0 */
> +#define CAN_ACPM_ADDR		0x0044	/* CAN Acceptance Mask 0 */
> +#define CAN_RBUF_RBACK_START_ADDR	0x0180	/* CAN transmit buffer start */
> +#define CAN_RBUF_RBACK_END_ADDR		0x01b0	/* CAN transmit buffer end */

Please rename the defines like this:
SUNXI_REG_ACPC, SUNXI_REG_ACPM

> +
> +/* Controller Register Description */
> +
> +/* mode select register (r/w)
> + * offset:0x0000 default:0x0000_0001
> + */
> +#define SLEEP_MODE		(0x01 << 4)	/* write in reset mode */
> +#define WAKE_UP			(0x00 << 4)
> +#define SINGLE_FILTER		(0x01 << 3)	/* write in reset mode */
> +#define DUAL_FILTERS		(0x00 << 3)
> +#define LOOPBACK_MODE		BIT(2)
> +#define LISTEN_ONLY_MODE	BIT(1)
> +#define RESET_MODE		BIT(0)

For these something like: SUNXI_MSEL_SLEEP_MODE, SUNXI_MSEL_WAKE_UP
makes sense.

> +
> +/* command register (w)
> + * offset:0x0004 default:0x0000_0000
> + */
> +#define BUS_OFF_REQ		BIT(5)
> +#define SELF_RCV_REQ		BIT(4)
> +#define CLEAR_OR_FLAG		BIT(3)
> +#define RELEASE_RBUF		BIT(2)
> +#define ABORT_REQ		BIT(1)
> +#define TRANS_REQ		BIT(0)

use here SUNXI_CMD_ as prefix

> +
> +/* status register (r)
> + * offset:0x0008 default:0x0000_003c
> + */
> +#define BIT_ERR			(0x00 << 22)
> +#define FORM_ERR		(0x01 << 22)
> +#define STUFF_ERR		(0x02 << 22)
> +#define OTHER_ERR		(0x03 << 22)
> +#define ERR_DIR			BIT(21)
> +#define ERR_SEG_CODE		(0x1f << 16)
> +#define START			(0x03 << 16)
> +#define ID28_21			(0x02 << 16)
> +#define ID20_18			(0x06 << 16)
> +#define SRTR			(0x04 << 16)
> +#define IDE			(0x05 << 16)
> +#define ID17_13			(0x07 << 16)
> +#define ID12_5			(0x0f << 16)
> +#define ID4_0			(0x0e << 16)
> +#define RTR			(0x0c << 16)
> +#define RB1			(0x0d << 16)
> +#define RB0			(0x09 << 16)
> +#define DLEN			(0x0b << 16)
> +#define DATA_FIELD		(0x0a << 16)
> +#define CRC_SEQUENCE		(0x08 << 16)
> +#define CRC_DELIMITER		(0x18 << 16)
> +#define ACK			(0x19 << 16)
> +#define ACK_DELIMITER		(0x1b << 16)
> +#define END			(0x1a << 16)
> +#define INTERMISSION		(0x12 << 16)
> +#define ACTIVE_ERROR		(0x11 << 16)
> +#define PASSIVE_ERROR		(0x16 << 16)
> +#define TOLERATE_DOMINANT_BITS	(0x13 << 16)
> +#define ERROR_DELIMITER		(0x17 << 16)
> +#define OVERLOAD		(0x1c << 16)
> +#define BUS_OFF			BIT(7)
> +#define ERR_STA			BIT(6)
> +#define TRANS_BUSY		BIT(5)
> +#define RCV_BUSY		BIT(4)
> +#define TRANS_OVER		BIT(3)
> +#define TBUF_RDY		BIT(2)
> +#define DATA_ORUN		BIT(1)
> +#define RBUF_RDY		BIT(0)

SUNXI_STA_
> +
> +/* interrupt register (r)
> + * offset:0x000c default:0x0000_0000
> + */
> +#define BUS_ERR			BIT(7)
> +#define ARB_LOST		BIT(6)
> +#define ERR_PASSIVE		BIT(5)
> +#define WAKEUP			BIT(4)
> +#define DATA_OR			BIT(3)
> +#define ERR_WRN			BIT(2)
> +#define TBUF_VLD		BIT(1)
> +#define RBUF_VLD		BIT(0)

SUNXI_INT_
> +
> +/* interrupt enable register (r/w)
> + * offset:0x0010 default:0x0000_0000
> + */
> +#define BERR_IRQ_EN		BIT(7)
> +#define ARB_LOST_IRQ_EN		BIT(6)
> +#define ERR_PASSIVE_IRQ_EN	BIT(5)
> +#define WAKEUP_IRQ_EN		BIT(4)
> +#define OR_IRQ_EN		BIT(3)
> +#define ERR_WRN_IRQ_EN		BIT(2)
> +#define TX_IRQ_EN		BIT(1)
> +#define RX_IRQ_EN		BIT(0)

SUNXI_INTENT_
> +
> +/* error code */
> +#define ERR_INRCV		(0x1 << 5)
> +#define ERR_INTRANS		(0x0 << 5)
> +
> +/* filter mode */
> +#define FILTER_CLOSE		0
> +#define SINGLE_FLTER_MODE	1
> +#define DUAL_FILTER_MODE	2
> +
> +/* max. number of interrupts handled in ISR */
> +#define SUNXI_CAN_MAX_IRQ	20
> +
> +struct sunxican_priv {
> +	struct can_priv can;
> +	struct sk_buff *echo_skb;

not used

> +	void __iomem *base;
> +	struct clk *clk;
> +	spinlock_t cmdreg_lock;	/* lock for concurrent cmd register writes */
> +};
> +
> +static const struct can_bittiming_const sunxican_bittiming_const = {
> +	.name = DRV_NAME,
> +	.tseg1_min = 1,
> +	.tseg1_max = 16,
> +	.tseg2_min = 1,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 1,
> +	.brp_max = 64,
> +	.brp_inc = 1,
> +};
> +
> +static void sunxi_can_write_cmdreg(struct sunxican_priv *priv, u8 val)
> +{
> +	unsigned long flags;
> +
> +	 /* The command register needs some locking and time to settle
> +	  * the write_reg() operation - especially on SMP systems.
> +	  */

Is this needed with your SJA1000 alike IP core?

> +	spin_lock_irqsave(&priv->cmdreg_lock, flags);
> +	writel(val, priv->base + CAN_CMD_ADDR);
> +	readl(priv->base + CAN_STA_ADDR);
> +	spin_unlock_irqrestore(&priv->cmdreg_lock, flags);
> +}
> +
> +static void set_normal_mode(struct net_device *dev)

What about making this function an int and return an error value in case
of an error?

> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	u8 status = readl(priv->base + CAN_MSEL_ADDR);
> +	int i;
> +
> +	for (i = 0; i < 100; i++) {
> +		/* check reset bit */
> +		if ((status & RESET_MODE) == 0) {
> +			priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +			/* enable interrupts */
> +			if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
> +				writel(0xFFFF, priv->base + CAN_INTEN_ADDR);
> +			} else {
> +				writel(0xFFFF & ~BERR_IRQ_EN,
> +				       priv->base + CAN_INTEN_ADDR);
> +			}
> +
> +			if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> +				/* Put device into loopback mode */

That's not loopback mode according to socketcan, that's
CAN_CTRLMODE_PRESUME_ACK.

> +				writel(readl(priv->base + CAN_MSEL_ADDR) |
> +				       LOOPBACK_MODE,
> +				       priv->base + CAN_MSEL_ADDR);
> +			} else if
> +				(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
> +				/* Put device into listen-only mode */
> +				writel(readl(priv->base + CAN_MSEL_ADDR) |
> +				       LISTEN_ONLY_MODE,
> +				       priv->base + CAN_MSEL_ADDR);
> +			}
> +			return;
> +		}
> +		/* set chip to normal mode */
> +		writel(readl(priv->base + CAN_MSEL_ADDR) & (~RESET_MODE),
> +		       priv->base + CAN_MSEL_ADDR);
> +		status = readl(priv->base + CAN_MSEL_ADDR);
> +	}

This loop looks not straight forward. What about:

	while (timeout -- &&
		(readl(priv->base + CAN_MSEL_ADDR) & RESET_MODE)) {
			writel(readl(priv->base)....);
	}

	if (readl(priv->base + CAN_MSEL_ADDR) & RESET_MODE)
		return -ETIMEDOUT;

	/* do the rest */
	return 0;	

> +	netdev_err(dev, "setting controller into normal mode failed!\n");
> +}
> +
> +static void set_reset_mode(struct net_device *dev)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	u8 status = readl(priv->base + CAN_MSEL_ADDR);
> +	int i;
> +
> +	for (i = 0; i < 100; i++) {
> +		/* check reset bit */
> +		if (status & RESET_MODE) {
> +			priv->can.state = CAN_STATE_STOPPED;
> +			return;
> +		}
> +		/* select reset mode */
> +		writel(readl(priv->base + CAN_MSEL_ADDR) |
> +			RESET_MODE, priv->base + CAN_MSEL_ADDR);
> +		status = readl(priv->base + CAN_MSEL_ADDR);
> +	}
> +	netdev_err(dev, "setting controller into reset mode failed!\n");
> +}

same here. move the action out of the loop and return an error.

> +
> +static int sunxican_set_bittiming(struct net_device *dev)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	struct can_bittiming *bt = &priv->can.bittiming;
> +	u32 cfg;
> +
> +	cfg = ((bt->brp - 1) & 0x3FF) |
> +	    (((bt->sjw - 1) & 0x3) << 14) |
> +	    (((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) << 16) |
> +	    (((bt->phase_seg2 - 1) & 0x7) << 20);
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> +		cfg |= 0x800000;
> +
> +	netdev_info(dev, "setting BITTIMING=0x%08x\n", cfg);
> +
> +	/* CAN_BTIME_ADDR only writable in reset mode */
> +	set_reset_mode(dev);
> +	writel(cfg, priv->base + CAN_BTIME_ADDR);
> +	set_normal_mode(dev);

check the return values of set_.._mode() here
> +
> +	return 0;
> +}
> +
> +static int sunxican_get_berr_counter(const struct net_device *dev,
> +				     struct can_berr_counter *bec)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	u32 errors;
> +
> +	errors = readl(priv->base + CAN_ERRC_ADDR);
> +	bec->txerr = errors & 0x000F;
> +	bec->rxerr = (errors >> 16) & 0x000F;
> +	return 0;
> +}
> +
> +static void sunxi_can_start(struct net_device *dev)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +
> +	/* leave reset mode */
> +	if (priv->can.state != CAN_STATE_STOPPED)
> +		set_reset_mode(dev);
> +
> +	/* set filters - we accept all */
> +	writel(0x00000000, priv->base + CAN_ACPC_ADDR);
> +	writel(0xffffffff, priv->base + CAN_ACPM_ADDR);
> +
> +	/* Clear error counters and error code capture */
> +	writel(0x0, priv->base + CAN_ERRC_ADDR);
> +
> +	/* leave reset mode */
> +	set_normal_mode(dev);
> +}
> +
> +static int sunxican_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		sunxi_can_start(dev);
> +		if (netif_queue_stopped(dev))
> +			netif_wake_queue(dev);
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +/* transmit a CAN message
> + * message layout in the sk_buff should be like this:
> + * xx xx xx xx         ff         ll 00 11 22 33 44 55 66 77
> + * [ can_id ] [flags] [len] [can data (up to 8 bytes]
> + */
> +static int sunxican_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	u8 dlc;
> +	canid_t id;
> +	u32 temp = 0;
> +	int i;
> +
> +	/* wait buffer ready */
> +	while (!(readl(priv->base + CAN_STA_ADDR) & TBUF_RDY))
> +		usleep_range(10, 100);

Can you explain why you need this sleep here?

> +
> +	if (can_dropped_invalid_skb(dev, skb))
> +		return NETDEV_TX_OK;
> +
> +	netif_stop_queue(dev);
> +
> +	dlc = cf->can_dlc;
> +	id = cf->can_id;
> +
> +	temp = ((id >> 30) << 6) | dlc;

Please don't reply on the fact that the upper two bits of id match the
upper two bits for temp. Please use an explizid conversion via:
	if (id & ..)
		temp |= BIT_FOO;

> +	writel(temp, priv->base + CAN_BUF0_ADDR);
> +	if (id & CAN_EFF_FLAG) {
> +		/* extended frame */
> +		writel((id >> 21) & 0xFF, priv->base + CAN_BUF1_ADDR);
> +		writel((id >> 13) & 0xFF, priv->base + CAN_BUF2_ADDR);
> +		writel((id >> 5)  & 0xFF, priv->base + CAN_BUF3_ADDR);
> +		writel((id & 0x1F) << 3,  priv->base + CAN_BUF4_ADDR);

Can you make it always first shift then mask or the other way round. see
the original sja1000 xmit() functio, that look pretty neat.

> +
> +		for (i = 0; i < dlc; i++) {
> +			writel(cf->data[i],
> +			       priv->base + (CAN_BUF5_ADDR + i * 4));
> +		}
> +	} else {
> +		/* standard frame */
> +		writel((id >> 3) & 0xFF, priv->base + CAN_BUF1_ADDR);
> +		writel((id & 0x7) << 5, priv->base + CAN_BUF2_ADDR);
> +
> +		for (i = 0; i < dlc; i++)
> +			writel(cf->data[i], priv->base + CAN_BUF3_ADDR + i * 4);
> +	}
> +	can_put_echo_skb(skb, dev, 0);

Do you have support for one_shot or loopback mode here, too. Like the
sja1000?

> +	sunxi_can_write_cmdreg(priv, TRANS_REQ);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static void sunxi_can_rx(struct net_device *dev)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u8 fi;
> +	canid_t id;
> +	int i;
> +
> +	/* create zero'ed CAN frame buffer */
> +	skb = alloc_can_skb(dev, &cf);
> +	if (!skb)
> +		return;
> +
> +	fi = readl(priv->base + CAN_BUF0_ADDR);
> +	cf->can_dlc = get_can_dlc(fi & 0x0F);
> +	if (fi >> 7) {
> +		/* extended frame format (EFF) */
> +		id = (readl(priv->base + CAN_BUF1_ADDR) << 21) |
> +		     (readl(priv->base + CAN_BUF2_ADDR) << 13) |
> +		     (readl(priv->base + CAN_BUF3_ADDR) << 5)  |
> +		    ((readl(priv->base + CAN_BUF4_ADDR) >> 3) & 0x1f);
> +		id |= CAN_EFF_FLAG;
> +
> +		/* remote transmission request ? */
> +		if ((fi >> 6) & 0x1) {
> +			id |= CAN_RTR_FLAG;
> +		} else {
> +			for (i = 0; i < cf->can_dlc; i++)
> +				cf->data[i] =
> +				    readl(priv->base + CAN_BUF5_ADDR + i * 4);
> +		}
> +	} else {
> +		/* standard frame format (SFF) */
> +		id = (readl(priv->base + CAN_BUF1_ADDR) << 3) |
> +		    ((readl(priv->base + CAN_BUF2_ADDR) >> 5) & 0x7);
> +
> +		/* remote transmission request ? */
> +		if ((fi >> 6) & 0x1) {
> +			id |= CAN_RTR_FLAG;
> +		} else {
> +			for (i = 0; i < cf->can_dlc; i++)
> +				cf->data[i] =
> +				    readl(priv->base + CAN_BUF3_ADDR + i * 4);
> +		}
> +	}
> +	cf->can_id = id;
> +
> +	/* release receive buffer */
> +	sunxi_can_write_cmdreg(priv, RELEASE_RBUF);
> +
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +	netif_rx(skb);
> +
> +	can_led_event(dev, CAN_LED_EVENT_RX);
> +}
> +
> +static int sunxi_can_err(struct net_device *dev, u8 isrc, u8 status)
> +{
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	enum can_state state = priv->can.state;
> +	u32 ecc, alc;
> +
> +	skb = alloc_can_err_skb(dev, &cf);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	if (isrc & DATA_OR) {
> +		/* data overrun interrupt */
> +		netdev_dbg(dev, "data overrun interrupt\n");
> +		cf->can_id |= CAN_ERR_CRTL;
> +		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +		stats->rx_over_errors++;
> +		stats->rx_errors++;
> +		sunxi_can_write_cmdreg(priv, CLEAR_OR_FLAG);	/* clear bit */
> +	}
> +	if (isrc & ERR_WRN) {
> +		/* error warning interrupt */
> +		netdev_dbg(dev, "error warning interrupt\n");
> +
> +		if (status & BUS_OFF) {
> +			state = CAN_STATE_BUS_OFF;
> +			cf->can_id |= CAN_ERR_BUSOFF;
> +			can_bus_off(dev);
> +		} else if (status & ERR_STA) {
> +			state = CAN_STATE_ERROR_WARNING;
> +		} else {
> +			state = CAN_STATE_ERROR_ACTIVE;
> +		}
> +	}
> +	if (isrc & BUS_ERR) {
> +		/* bus error interrupt */
> +		netdev_dbg(dev, "bus error interrupt\n");
> +		priv->can.can_stats.bus_error++;
> +		stats->rx_errors++;
> +
> +		ecc = readl(priv->base + CAN_STA_ADDR);
> +
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> +		if (ecc & BIT_ERR) {
> +			cf->data[2] |= CAN_ERR_PROT_BIT;
> +		} else if (ecc & FORM_ERR) {
> +			cf->data[2] |= CAN_ERR_PROT_FORM;
> +		} else if (ecc & STUFF_ERR) {
> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
> +		} else {
> +			cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> +			cf->data[3] = (ecc & ERR_SEG_CODE) >> 16;
> +		}
> +		/* error occurred during transmission? */
> +		if ((ecc & ERR_DIR) == 0)
> +			cf->data[2] |= CAN_ERR_PROT_TX;
> +	}
> +	if (isrc & ERR_PASSIVE) {
> +		/* error passive interrupt */
> +		netdev_dbg(dev, "error passive interrupt\n");
> +		if (status & ERR_STA)
> +			state = CAN_STATE_ERROR_PASSIVE;
> +		else
> +			state = CAN_STATE_ERROR_ACTIVE;
> +	}
> +	if (isrc & ARB_LOST) {
> +		/* arbitration lost interrupt */
> +		netdev_dbg(dev, "arbitration lost interrupt\n");
> +		alc = readl(priv->base + CAN_STA_ADDR);
> +		priv->can.can_stats.arbitration_lost++;
> +		stats->tx_errors++;
> +		cf->can_id |= CAN_ERR_LOSTARB;
> +		cf->data[0] = (alc & 0x1f) >> 8;
> +	}
> +	if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
> +					 state == CAN_STATE_ERROR_PASSIVE)) {
> +		u32 errc = readl(priv->base + CAN_ERRC_ADDR);
> +		u8 rxerr = (errc >> 16) & 0xFF;
> +		u8 txerr = errc & 0xFF;
> +
> +		cf->can_id |= CAN_ERR_CRTL;
> +		if (state == CAN_STATE_ERROR_WARNING) {
> +			priv->can.can_stats.error_warning++;
> +			cf->data[1] = (txerr > rxerr) ?
> +			    CAN_ERR_CRTL_TX_WARNING : CAN_ERR_CRTL_RX_WARNING;
> +		} else {
> +			priv->can.can_stats.error_passive++;
> +			cf->data[1] = (txerr > rxerr) ?
> +			    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
> +		}
> +		cf->data[6] = txerr;
> +		cf->data[7] = rxerr;
> +	}
> +	priv->can.state = state;
> +
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +	netif_rx(skb);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t sunxi_can_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *dev = (struct net_device *)dev_id;
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	u8 isrc, status;
> +	int n = 0;
> +
> +	while ((isrc = readl(priv->base + CAN_INT_ADDR))
> +	       && (n < SUNXI_CAN_MAX_IRQ)) {
> +		n++;
> +		status = readl(priv->base + CAN_STA_ADDR);
> +
> +		if (isrc & WAKEUP)
> +			netdev_warn(dev, "wakeup interrupt\n");
> +
> +		if (isrc & TBUF_VLD) {
> +			/* transmission complete interrupt */
> +			stats->tx_bytes +=
> +			    readl(priv->base + CAN_RBUF_RBACK_START_ADDR) & 0xf;
> +			stats->tx_packets++;
> +			can_get_echo_skb(dev, 0);
> +			netif_wake_queue(dev);
> +			can_led_event(dev, CAN_LED_EVENT_TX);
> +		}
> +		if (isrc & RBUF_VLD) {
> +			/* receive interrupt */
> +			while (status & RBUF_RDY) {
> +			/* RX buffer is not empty */
> +				sunxi_can_rx(dev);
> +				status = readl(priv->base + CAN_STA_ADDR);
> +			}
> +		}
> +		if (isrc &
> +		    (DATA_OR | ERR_WRN | BUS_ERR | ERR_PASSIVE | ARB_LOST)) {
> +			/* error interrupt */
> +			if (sunxi_can_err(dev, isrc, status))
> +				break;
> +		}
> +		/* clear the interrupt */
> +		writel(isrc, priv->base + CAN_INT_ADDR);
> +	}
> +	if (n >= SUNXI_CAN_MAX_IRQ)
> +		netdev_dbg(dev, "%d messages handled in ISR", n);
> +
> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static int sunxican_open(struct net_device *dev)
> +{
> +	int err;
> +
> +	/* set chip into reset mode */
> +	set_reset_mode(dev);
> +
> +	/* common open */
> +	err = open_candev(dev);
> +	if (err)
> +		return err;
> +
> +	/* register interrupt handler */

please make it:
	err = request_irq();
	if (err) {
	}
> +	if (request_irq
> +	    (dev->irq, sunxi_can_interrupt, IRQF_TRIGGER_NONE, dev->name,
                                            ^^^^^^^^^^^^^^^^^
Please make it a IRQF_SHARED.


> +	     (void *)dev)) {
             ^^^^^^^^
cast not needed
> +		netdev_err(dev, "request_irq err: %d\n", err);
close_candev() missing
> +		return -EAGAIN;

> +	}
> +	sunxican_set_bittiming(dev);
> +	sunxi_can_start(dev);
> +
> +	can_led_event(dev, CAN_LED_EVENT_OPEN);
> +
> +	netif_start_queue(dev);
> +
> +	return 0;
> +}
> +
> +static int sunxican_close(struct net_device *dev)
> +{
> +	netif_stop_queue(dev);
> +	set_reset_mode(dev);
> +
> +	free_irq(dev->irq, (void *)dev);
> +
> +	close_candev(dev);
> +
> +	can_led_event(dev, CAN_LED_EVENT_STOP);
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops sunxican_netdev_ops = {
> +	.ndo_open = sunxican_open,
> +	.ndo_stop = sunxican_close,
> +	.ndo_start_xmit = sunxican_start_xmit,
> +};
> +
> +static const struct of_device_id sunxican_of_match[] = {
> +	{.compatible = "allwinner,sunxican"},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxican_of_match);
> +
> +static const struct platform_device_id sunxican_id_table[] = {
> +	{.name = "sunxican"},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(platform, sunxican_id_table);
> +
> +static int sunxican_remove(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +	struct resource *res;
> +
> +	set_reset_mode(dev);
> +	unregister_netdev(dev);
> +	iounmap(priv->base);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(res->start, resource_size(res));
> +
> +	free_candev(dev);
> +
> +	return 0;
> +}
> +
> +static int sunxican_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct clk *clk;
> +	void __iomem *addr;
> +	int err, irq;
> +	u32 temp_irqen;
> +	struct net_device *dev;
> +	struct sunxican_priv *priv;
> +
> +	clk = clk_get(&pdev->dev, "apb1_can");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "no clock defined\n");
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +	/* turn on clocking for CAN peripheral block */
> +	err = clk_prepare_enable(clk);

Please move the clock handling into the open() function. Or better think
about implementing runtimepm.

> +	if (err) {
> +		dev_err(&pdev->dev, "could not enable clocking (apb1_can)\n");
> +		goto exit;
> +	}
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	irq = platform_get_irq(pdev, 0);
> +
> +	if (!res || irq <= 0) {
> +		dev_err(&pdev->dev, "could not get a valid irq\n");
> +		err = -ENODEV;
> +		goto exit_put;
> +	}
> +	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
> +		dev_err(&pdev->dev, "could not get io memory resource\n");
> +		err = -EBUSY;
> +		goto exit_put;
> +	}
> +	addr = ioremap_nocache(res->start, resource_size(res));
> +	if (!addr) {
> +		dev_err(&pdev->dev, "could not map io memory\n");
> +		err = -ENOMEM;
> +		goto exit_release;
> +	}
> +	dev = alloc_candev(sizeof(struct sunxican_priv), 1);
> +
> +	if (!dev) {
> +		dev_err(&pdev->dev,
> +			"could not allocate memory for CAN device\n");
> +		err = -ENOMEM;
> +		goto exit_iounmap;
> +	}
> +
> +	dev->netdev_ops = &sunxican_netdev_ops;
> +	dev->irq = irq;
> +	dev->flags |= IFF_ECHO;
> +
> +	priv = netdev_priv(dev);
> +	priv->can.clock.freq = clk_get_rate(clk);
> +	priv->can.bittiming_const = &sunxican_bittiming_const;
> +	priv->can.do_set_mode = sunxican_set_mode;
> +	priv->can.do_get_berr_counter = sunxican_get_berr_counter;
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
> +	    CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_LOOPBACK |
> +	    CAN_CTRLMODE_3_SAMPLES;
> +	priv->base = addr;
> +	priv->clk = clk;
> +	spin_lock_init(&priv->cmdreg_lock);
> +
> +	/* enable CAN specific interrupts */
> +	set_reset_mode(dev);
> +	temp_irqen = BERR_IRQ_EN | ERR_PASSIVE_IRQ_EN | OR_IRQ_EN | RX_IRQ_EN;
> +	writel(readl(priv->base + CAN_INTEN_ADDR) | temp_irqen,
> +	       priv->base + CAN_INTEN_ADDR);
> +
> +	platform_set_drvdata(pdev, dev);
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	err = register_candev(dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> +			DRV_NAME, err);
> +		goto exit_free;
> +	}
> +	devm_can_led_init(dev);
> +
> +	dev_info(&pdev->dev, "device registered (base=%p, irq=%d)\n",
> +		 priv->base, dev->irq);
> +
> +	return 0;
> +
> +exit_free:
> +	free_candev(dev);
> +exit_iounmap:
> +	iounmap(addr);
> +exit_release:
> +	release_mem_region(res->start, resource_size(res));
> +exit_put:
> +	clk_put(clk);
> +exit:
> +	return err;
> +}
> +
> +static int __maybe_unused sunxi_can_suspend(struct device *device)
> +{
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +
> +	if (netif_running(dev)) {
> +		netif_stop_queue(dev);
> +		netif_device_detach(dev);
> +	}
> +	priv->can.state = CAN_STATE_SLEEPING;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused sunxi_can_resume(struct device *device)
> +{
> +	struct net_device *dev = dev_get_drvdata(device);
> +	struct sunxican_priv *priv = netdev_priv(dev);
> +
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +	if (netif_running(dev)) {
> +		netif_device_attach(dev);
> +		netif_start_queue(dev);
> +	}
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(sunxi_can_pm_ops, sunxi_can_suspend, sunxi_can_resume);
> +
> +static struct platform_driver sunxi_can_driver = {
> +	.driver = {
> +		   .name = DRV_NAME,

Please remove the .name.

> +		   .owner = THIS_MODULE,

Please indent with just two tabs here.

> +		   .pm = &sunxi_can_pm_ops,
> +		   .of_match_table = sunxican_of_match,
> +		   },
> +	.probe = sunxican_probe,
> +	.remove = sunxican_remove,
> +	.id_table = sunxican_id_table,
> +};
> +
> +module_platform_driver(sunxi_can_driver);
> +
> +MODULE_AUTHOR("Peter Chen <xingkongcp@gmail.com>");
> +MODULE_AUTHOR("Gerhard Bertelsmann <info@gerhard-bertelsmann.de>");

Oh nice, I didn't know that you can use two MODULE_AUTHOR lines.

> +MODULE_LICENSE("Dual BSD/GPL");
                   ^^^^^^^^^^^^
The header does only state GPL
> +MODULE_DESCRIPTION(DRV_NAME "CAN driver for Allwinner SoCs (A10/A20)");
> +MODULE_VERSION(DRV_MODULE_VERSION);

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH v2 2/2] Allwinner SoC CAN controller Support

[Gerhard Bertelsmann]

Hi Marc,

thanks for your remarks - comments below.

Am 2015-09-08 12:51, schrieb Marc Kleine-Budde:
> On 09/03/2015 12:09 PM, Gerhard Bertelsmann wrote:
>> Second try:
>>  Allwinner A10/A20 CAN Controller support
>> 
>> 
>> Signed-off-by: Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
>> Tested-by; Gerhard Bertelsmann < info@gerhard-bertelsmann.de >
>> 
>> 
>>  drivers/net/can/Kconfig                            |  10 +
>>  drivers/net/can/Makefile                           |   1 +
>>  drivers/net/can/sunxi_can.c                        | 832 
>> +++++++++++++++++++++
>>  3 files changed, 843 insertions(+)
>> 
>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>> index e8c96b8..29b62e6 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -129,6 +129,16 @@ config CAN_RCAR
>>  	  To compile this driver as a module, choose M here: the module will
>>  	  be called rcar_can.
>> 
>> +config CAN_SUNXI
>> +	tristate "Allwinner SUNXI CAN controller"
>> +	depends on MACH_SUN4I || MACH_SUN7I
>> +	---help---
>> +	  Say Y here if you want to use CAN controller found on Allwinner
>> +	  A10/A20 SoCs.
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called sunxi_can.
>> +
>>  config CAN_XILINXCAN
>>  	tristate "Xilinx CAN"
>>  	depends on ARCH_ZYNQ || ARM64 || MICROBLAZE || COMPILE_TEST
>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
>> index c533c62..b3e825c 100644
>> --- a/drivers/net/can/Makefile
>> +++ b/drivers/net/can/Makefile
>> @@ -27,6 +27,7 @@ obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
>>  obj-$(CONFIG_PCH_CAN)		+= pch_can.o
>>  obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
>>  obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
>> +obj-$(CONFIG_CAN_SUNXI)		+= sunxi_can.o
>>  obj-$(CONFIG_CAN_XILINXCAN)	+= xilinx_can.o
>> 
>>  subdir-ccflags-y += -D__CHECK_ENDIAN__
>> diff --git a/drivers/net/can/sunxi_can.c b/drivers/net/can/sunxi_can.c
>> new file mode 100644
>> index 0000000..6e3a5e5
>> --- /dev/null
>> +++ b/drivers/net/can/sunxi_can.c
>> @@ -0,0 +1,832 @@
>> +/*
>> + * sunxi_can.c - CAN bus controller driver for Allwinner SUN4I&SUN7I 
>> based SoCs
>> + *
>> + * Copyright (C) 2013 Peter Chen
>> + * Copyright (C) 2015 Gerhard Bertelsmann
>> + *
>> + * Parts of this software are based on (derived from) the SJA1000 
>> code by:
>> + *   Copyright (C) 2014 Oliver Hartkopp 
>> <oliver.hartkopp@volkswagen.de>
>> + *   Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
>> + *   Copyright (C) 2002-2007 Volkswagen Group Electronic Research
>> + *   Copyright (C) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
>> + *   38106 Braunschweig, GERMANY
>> + *
>> + * This program is free software; you can redistribute it and/or 
>> modify
>> + * it under the terms of the version 2 of the GNU General Public 
>> License
>> + * as published by the Free Software Foundation
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see 
>> <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#include <linux/netdevice.h>
>> +#include <linux/can.h>
>> +#include <linux/can/dev.h>
>> +#include <linux/can/error.h>
>> +#include <linux/can/led.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define DRV_NAME "sunxi_can"
>> +#define DRV_MODULE_VERSION "0.94"
> 
> Where does the version come from? Are you planing to make use of this?

my personally version numbering. with 1.0 the code is for the wild :-)

> 
>> +
>> +/* Registers address */
> 
> Please prefix _all_ your defines with a common prefix, e.g. SUNXI_
> 
>> +#define CAN_BASE0		0x01C2BC00
> 
> Please remove that define, the base address comes from the DT now.

fixed - in next version, but I personally like to see which IO address
is the code talking about. If you aren't aware of the address, you have
need some time to find it in the docs.

> 
>> +#define CAN_MSEL_ADDR		0x0000	/* CAN Mode Select */
>> +#define CAN_CMD_ADDR            0x0004	/* CAN Command */
>> +#define CAN_STA_ADDR		0x0008	/* CAN Status */
>> +#define CAN_INT_ADDR		0x000c	/* CAN Interrupt Flag */
>> +#define CAN_INTEN_ADDR		0x0010	/* CAN Interrupt Enable */
>> +#define CAN_BTIME_ADDR		0x0014	/* CAN Bus Timing 0 */
>> +#define CAN_TEWL_ADDR		0x0018	/* CAN Tx Error Warning Limit */
>> +#define CAN_ERRC_ADDR		0x001c	/* CAN Error Counter */
>> +#define CAN_RMCNT_ADDR		0x0020	/* CAN Receive Message Counter */
>> +#define CAN_RBUFSA_ADDR		0x0024	/* CAN Receive Buffer Start Address 
>> */
>> +#define CAN_BUF0_ADDR		0x0040	/* CAN Tx/Rx Buffer 0 */
>> +#define CAN_BUF1_ADDR		0x0044	/* CAN Tx/Rx Buffer 1 */
>> +#define CAN_BUF2_ADDR		0x0048	/* CAN Tx/Rx Buffer 2 */
>> +#define CAN_BUF3_ADDR		0x004c	/* CAN Tx/Rx Buffer 3 */
>> +#define CAN_BUF4_ADDR		0x0050	/* CAN Tx/Rx Buffer 4 */
>> +#define CAN_BUF5_ADDR		0x0054	/* CAN Tx/Rx Buffer 5 */
>> +#define CAN_BUF6_ADDR		0x0058	/* CAN Tx/Rx Buffer 6 */
>> +#define CAN_BUF7_ADDR		0x005c	/* CAN Tx/Rx Buffer 7 */
>> +#define CAN_BUF8_ADDR		0x0060	/* CAN Tx/Rx Buffer 8 */
>> +#define CAN_BUF9_ADDR		0x0064	/* CAN Tx/Rx Buffer 9 */
>> +#define CAN_BUF10_ADDR		0x0068	/* CAN Tx/Rx Buffer 10 */
>> +#define CAN_BUF11_ADDR		0x006c	/* CAN Tx/Rx Buffer 11 */
>> +#define CAN_BUF12_ADDR		0x0070	/* CAN Tx/Rx Buffer 12 */
>> +#define CAN_ACPC_ADDR		0x0040	/* CAN Acceptance Code 0 */
>> +#define CAN_ACPM_ADDR		0x0044	/* CAN Acceptance Mask 0 */
>> +#define CAN_RBUF_RBACK_START_ADDR	0x0180	/* CAN transmit buffer start 
>> */
>> +#define CAN_RBUF_RBACK_END_ADDR		0x01b0	/* CAN transmit buffer end */
> 
> Please rename the defines like this:
> SUNXI_REG_ACPC, SUNXI_REG_ACPM

fixed - in next version

> 
>> +
>> +/* Controller Register Description */
>> +
>> +/* mode select register (r/w)
>> + * offset:0x0000 default:0x0000_0001
>> + */
>> +#define SLEEP_MODE		(0x01 << 4)	/* write in reset mode */
>> +#define WAKE_UP			(0x00 << 4)
>> +#define SINGLE_FILTER		(0x01 << 3)	/* write in reset mode */
>> +#define DUAL_FILTERS		(0x00 << 3)
>> +#define LOOPBACK_MODE		BIT(2)
>> +#define LISTEN_ONLY_MODE	BIT(1)
>> +#define RESET_MODE		BIT(0)
> 
> For these something like: SUNXI_MSEL_SLEEP_MODE, SUNXI_MSEL_WAKE_UP
> makes sense.

fixed - in next version

> 
>> +
>> +/* command register (w)
>> + * offset:0x0004 default:0x0000_0000
>> + */
>> +#define BUS_OFF_REQ		BIT(5)
>> +#define SELF_RCV_REQ		BIT(4)
>> +#define CLEAR_OR_FLAG		BIT(3)
>> +#define RELEASE_RBUF		BIT(2)
>> +#define ABORT_REQ		BIT(1)
>> +#define TRANS_REQ		BIT(0)
> 
> use here SUNXI_CMD_ as prefix

fixed - in next version

> 
>> +
>> +/* status register (r)
>> + * offset:0x0008 default:0x0000_003c
>> + */
>> +#define BIT_ERR			(0x00 << 22)
>> +#define FORM_ERR		(0x01 << 22)
>> +#define STUFF_ERR		(0x02 << 22)
>> +#define OTHER_ERR		(0x03 << 22)
>> +#define ERR_DIR			BIT(21)
>> +#define ERR_SEG_CODE		(0x1f << 16)
>> +#define START			(0x03 << 16)
>> +#define ID28_21			(0x02 << 16)
>> +#define ID20_18			(0x06 << 16)
>> +#define SRTR			(0x04 << 16)
>> +#define IDE			(0x05 << 16)
>> +#define ID17_13			(0x07 << 16)
>> +#define ID12_5			(0x0f << 16)
>> +#define ID4_0			(0x0e << 16)
>> +#define RTR			(0x0c << 16)
>> +#define RB1			(0x0d << 16)
>> +#define RB0			(0x09 << 16)
>> +#define DLEN			(0x0b << 16)
>> +#define DATA_FIELD		(0x0a << 16)
>> +#define CRC_SEQUENCE		(0x08 << 16)
>> +#define CRC_DELIMITER		(0x18 << 16)
>> +#define ACK			(0x19 << 16)
>> +#define ACK_DELIMITER		(0x1b << 16)
>> +#define END			(0x1a << 16)
>> +#define INTERMISSION		(0x12 << 16)
>> +#define ACTIVE_ERROR		(0x11 << 16)
>> +#define PASSIVE_ERROR		(0x16 << 16)
>> +#define TOLERATE_DOMINANT_BITS	(0x13 << 16)
>> +#define ERROR_DELIMITER		(0x17 << 16)
>> +#define OVERLOAD		(0x1c << 16)
>> +#define BUS_OFF			BIT(7)
>> +#define ERR_STA			BIT(6)
>> +#define TRANS_BUSY		BIT(5)
>> +#define RCV_BUSY		BIT(4)
>> +#define TRANS_OVER		BIT(3)
>> +#define TBUF_RDY		BIT(2)
>> +#define DATA_ORUN		BIT(1)
>> +#define RBUF_RDY		BIT(0)
> 
> SUNXI_STA_

fixed - in next version

>> +
>> +/* interrupt register (r)
>> + * offset:0x000c default:0x0000_0000
>> + */
>> +#define BUS_ERR			BIT(7)
>> +#define ARB_LOST		BIT(6)
>> +#define ERR_PASSIVE		BIT(5)
>> +#define WAKEUP			BIT(4)
>> +#define DATA_OR			BIT(3)
>> +#define ERR_WRN			BIT(2)
>> +#define TBUF_VLD		BIT(1)
>> +#define RBUF_VLD		BIT(0)
> 
> SUNXI_INT_

fixed - in next version

>> +
>> +/* interrupt enable register (r/w)
>> + * offset:0x0010 default:0x0000_0000
>> + */
>> +#define BERR_IRQ_EN		BIT(7)
>> +#define ARB_LOST_IRQ_EN		BIT(6)
>> +#define ERR_PASSIVE_IRQ_EN	BIT(5)
>> +#define WAKEUP_IRQ_EN		BIT(4)
>> +#define OR_IRQ_EN		BIT(3)
>> +#define ERR_WRN_IRQ_EN		BIT(2)
>> +#define TX_IRQ_EN		BIT(1)
>> +#define RX_IRQ_EN		BIT(0)
> 
> SUNXI_INTENT_

fixed - in next version

>> +
>> +/* error code */
>> +#define ERR_INRCV		(0x1 << 5)
>> +#define ERR_INTRANS		(0x0 << 5)
>> +
>> +/* filter mode */
>> +#define FILTER_CLOSE		0
>> +#define SINGLE_FLTER_MODE	1
>> +#define DUAL_FILTER_MODE	2
>> +
>> +/* max. number of interrupts handled in ISR */
>> +#define SUNXI_CAN_MAX_IRQ	20
>> +
>> +struct sunxican_priv {
>> +	struct can_priv can;
>> +	struct sk_buff *echo_skb;
> 
> not used

already fixed

> 
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	spinlock_t cmdreg_lock;	/* lock for concurrent cmd register writes 
>> */
>> +};
>> +
>> +static const struct can_bittiming_const sunxican_bittiming_const = {
>> +	.name = DRV_NAME,
>> +	.tseg1_min = 1,
>> +	.tseg1_max = 16,
>> +	.tseg2_min = 1,
>> +	.tseg2_max = 8,
>> +	.sjw_max = 4,
>> +	.brp_min = 1,
>> +	.brp_max = 64,
>> +	.brp_inc = 1,
>> +};
>> +
>> +static void sunxi_can_write_cmdreg(struct sunxican_priv *priv, u8 
>> val)
>> +{
>> +	unsigned long flags;
>> +
>> +	 /* The command register needs some locking and time to settle
>> +	  * the write_reg() operation - especially on SMP systems.
>> +	  */
> 
> Is this needed with your SJA1000 alike IP core?

IMHO the Allwinner CAN IP isn't a real SJA1000. It's like a SJA1000 -
not more. The extra reading doesn't hurt IMHO ...

> 
>> +	spin_lock_irqsave(&priv->cmdreg_lock, flags);
>> +	writel(val, priv->base + CAN_CMD_ADDR);
>> +	readl(priv->base + CAN_STA_ADDR);
>> +	spin_unlock_irqrestore(&priv->cmdreg_lock, flags);
>> +}
>> +
>> +static void set_normal_mode(struct net_device *dev)
> 
> What about making this function an int and return an error value in 
> case
> of an error?

I will check this

> 
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	u8 status = readl(priv->base + CAN_MSEL_ADDR);
>> +	int i;
>> +
>> +	for (i = 0; i < 100; i++) {
>> +		/* check reset bit */
>> +		if ((status & RESET_MODE) == 0) {
>> +			priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +			/* enable interrupts */
>> +			if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
>> +				writel(0xFFFF, priv->base + CAN_INTEN_ADDR);
>> +			} else {
>> +				writel(0xFFFF & ~BERR_IRQ_EN,
>> +				       priv->base + CAN_INTEN_ADDR);
>> +			}
>> +
>> +			if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>> +				/* Put device into loopback mode */
> 
> That's not loopback mode according to socketcan, that's
> CAN_CTRLMODE_PRESUME_ACK.

isn't CAN_CTRLMODE_LOOPBACK loopback mode ? May I can't see the obvious 
here ...

> 
>> +				writel(readl(priv->base + CAN_MSEL_ADDR) |
>> +				       LOOPBACK_MODE,
>> +				       priv->base + CAN_MSEL_ADDR);
>> +			} else if
>> +				(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
>> +				/* Put device into listen-only mode */
>> +				writel(readl(priv->base + CAN_MSEL_ADDR) |
>> +				       LISTEN_ONLY_MODE,
>> +				       priv->base + CAN_MSEL_ADDR);
>> +			}
>> +			return;
>> +		}
>> +		/* set chip to normal mode */
>> +		writel(readl(priv->base + CAN_MSEL_ADDR) & (~RESET_MODE),
>> +		       priv->base + CAN_MSEL_ADDR);
>> +		status = readl(priv->base + CAN_MSEL_ADDR);
>> +	}
> 
> This loop looks not straight forward. What about:
> 
> 	while (timeout -- &&
> 		(readl(priv->base + CAN_MSEL_ADDR) & RESET_MODE)) {
> 			writel(readl(priv->base)....);
> 	}
> 
> 	if (readl(priv->base + CAN_MSEL_ADDR) & RESET_MODE)
> 		return -ETIMEDOUT;
> 
> 	/* do the rest */
> 	return 0;
> 

indeed, that's looking bad. I will fix it in next version

>> +	netdev_err(dev, "setting controller into normal mode failed!\n");
>> +}
>> +
>> +static void set_reset_mode(struct net_device *dev)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	u8 status = readl(priv->base + CAN_MSEL_ADDR);
>> +	int i;
>> +
>> +	for (i = 0; i < 100; i++) {
>> +		/* check reset bit */
>> +		if (status & RESET_MODE) {
>> +			priv->can.state = CAN_STATE_STOPPED;
>> +			return;
>> +		}
>> +		/* select reset mode */
>> +		writel(readl(priv->base + CAN_MSEL_ADDR) |
>> +			RESET_MODE, priv->base + CAN_MSEL_ADDR);
>> +		status = readl(priv->base + CAN_MSEL_ADDR);
>> +	}
>> +	netdev_err(dev, "setting controller into reset mode failed!\n");
>> +}
> 
> same here. move the action out of the loop and return an error.

will be fixed in next version

> 
>> +
>> +static int sunxican_set_bittiming(struct net_device *dev)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	struct can_bittiming *bt = &priv->can.bittiming;
>> +	u32 cfg;
>> +
>> +	cfg = ((bt->brp - 1) & 0x3FF) |
>> +	    (((bt->sjw - 1) & 0x3) << 14) |
>> +	    (((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) << 16) |
>> +	    (((bt->phase_seg2 - 1) & 0x7) << 20);
>> +	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>> +		cfg |= 0x800000;
>> +
>> +	netdev_info(dev, "setting BITTIMING=0x%08x\n", cfg);
>> +
>> +	/* CAN_BTIME_ADDR only writable in reset mode */
>> +	set_reset_mode(dev);
>> +	writel(cfg, priv->base + CAN_BTIME_ADDR);
>> +	set_normal_mode(dev);
> 
> check the return values of set_.._mode() here

I will check this ...

>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxican_get_berr_counter(const struct net_device *dev,
>> +				     struct can_berr_counter *bec)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	u32 errors;
>> +
>> +	errors = readl(priv->base + CAN_ERRC_ADDR);
>> +	bec->txerr = errors & 0x000F;
>> +	bec->rxerr = (errors >> 16) & 0x000F;
>> +	return 0;
>> +}
>> +
>> +static void sunxi_can_start(struct net_device *dev)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +
>> +	/* leave reset mode */
>> +	if (priv->can.state != CAN_STATE_STOPPED)
>> +		set_reset_mode(dev);
>> +
>> +	/* set filters - we accept all */
>> +	writel(0x00000000, priv->base + CAN_ACPC_ADDR);
>> +	writel(0xffffffff, priv->base + CAN_ACPM_ADDR);
>> +
>> +	/* Clear error counters and error code capture */
>> +	writel(0x0, priv->base + CAN_ERRC_ADDR);
>> +
>> +	/* leave reset mode */
>> +	set_normal_mode(dev);
>> +}
>> +
>> +static int sunxican_set_mode(struct net_device *dev, enum can_mode 
>> mode)
>> +{
>> +	switch (mode) {
>> +	case CAN_MODE_START:
>> +		sunxi_can_start(dev);
>> +		if (netif_queue_stopped(dev))
>> +			netif_wake_queue(dev);
>> +		break;
>> +
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/* transmit a CAN message
>> + * message layout in the sk_buff should be like this:
>> + * xx xx xx xx         ff         ll 00 11 22 33 44 55 66 77
>> + * [ can_id ] [flags] [len] [can data (up to 8 bytes]
>> + */
>> +static int sunxican_start_xmit(struct sk_buff *skb, struct net_device 
>> *dev)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	struct can_frame *cf = (struct can_frame *)skb->data;
>> +	u8 dlc;
>> +	canid_t id;
>> +	u32 temp = 0;
>> +	int i;
>> +
>> +	/* wait buffer ready */
>> +	while (!(readl(priv->base + CAN_STA_ADDR) & TBUF_RDY))
>> +		usleep_range(10, 100);
> 
> Can you explain why you need this sleep here?

IMHO its needed. How could you guarantee that the previous transmit is 
finished
otherwise ?

> 
>> +
>> +	if (can_dropped_invalid_skb(dev, skb))
>> +		return NETDEV_TX_OK;
>> +
>> +	netif_stop_queue(dev);
>> +
>> +	dlc = cf->can_dlc;
>> +	id = cf->can_id;
>> +
>> +	temp = ((id >> 30) << 6) | dlc;
> 
> Please don't reply on the fact that the upper two bits of id match the
> upper two bits for temp. Please use an explizid conversion via:
> 	if (id & ..)
> 		temp |= BIT_FOO;

fixed - in next version

> 
>> +	writel(temp, priv->base + CAN_BUF0_ADDR);
>> +	if (id & CAN_EFF_FLAG) {
>> +		/* extended frame */
>> +		writel((id >> 21) & 0xFF, priv->base + CAN_BUF1_ADDR);
>> +		writel((id >> 13) & 0xFF, priv->base + CAN_BUF2_ADDR);
>> +		writel((id >> 5)  & 0xFF, priv->base + CAN_BUF3_ADDR);
>> +		writel((id & 0x1F) << 3,  priv->base + CAN_BUF4_ADDR);
> 
> Can you make it always first shift then mask or the other way round. 
> see
> the original sja1000 xmit() functio, that look pretty neat.

fixed - in next version. I like shifting first and mask afterwards.
In fact, it's using less soucre code chars ;-)

> 
>> +
>> +		for (i = 0; i < dlc; i++) {
>> +			writel(cf->data[i],
>> +			       priv->base + (CAN_BUF5_ADDR + i * 4));
>> +		}
>> +	} else {
>> +		/* standard frame */
>> +		writel((id >> 3) & 0xFF, priv->base + CAN_BUF1_ADDR);
>> +		writel((id & 0x7) << 5, priv->base + CAN_BUF2_ADDR);
>> +
>> +		for (i = 0; i < dlc; i++)
>> +			writel(cf->data[i], priv->base + CAN_BUF3_ADDR + i * 4);
>> +	}
>> +	can_put_echo_skb(skb, dev, 0);
> 
> Do you have support for one_shot or loopback mode here, too. Like the
> sja1000?

Hmm - will have a look at this ...

> 
>> +	sunxi_can_write_cmdreg(priv, TRANS_REQ);
>> +
>> +	return NETDEV_TX_OK;
>> +}
>> +
>> +static void sunxi_can_rx(struct net_device *dev)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	struct can_frame *cf;
>> +	struct sk_buff *skb;
>> +	u8 fi;
>> +	canid_t id;
>> +	int i;
>> +
>> +	/* create zero'ed CAN frame buffer */
>> +	skb = alloc_can_skb(dev, &cf);
>> +	if (!skb)
>> +		return;
>> +
>> +	fi = readl(priv->base + CAN_BUF0_ADDR);
>> +	cf->can_dlc = get_can_dlc(fi & 0x0F);
>> +	if (fi >> 7) {
>> +		/* extended frame format (EFF) */
>> +		id = (readl(priv->base + CAN_BUF1_ADDR) << 21) |
>> +		     (readl(priv->base + CAN_BUF2_ADDR) << 13) |
>> +		     (readl(priv->base + CAN_BUF3_ADDR) << 5)  |
>> +		    ((readl(priv->base + CAN_BUF4_ADDR) >> 3) & 0x1f);
>> +		id |= CAN_EFF_FLAG;
>> +
>> +		/* remote transmission request ? */
>> +		if ((fi >> 6) & 0x1) {
>> +			id |= CAN_RTR_FLAG;
>> +		} else {
>> +			for (i = 0; i < cf->can_dlc; i++)
>> +				cf->data[i] =
>> +				    readl(priv->base + CAN_BUF5_ADDR + i * 4);
>> +		}
>> +	} else {
>> +		/* standard frame format (SFF) */
>> +		id = (readl(priv->base + CAN_BUF1_ADDR) << 3) |
>> +		    ((readl(priv->base + CAN_BUF2_ADDR) >> 5) & 0x7);
>> +
>> +		/* remote transmission request ? */
>> +		if ((fi >> 6) & 0x1) {
>> +			id |= CAN_RTR_FLAG;
>> +		} else {
>> +			for (i = 0; i < cf->can_dlc; i++)
>> +				cf->data[i] =
>> +				    readl(priv->base + CAN_BUF3_ADDR + i * 4);
>> +		}
>> +	}
>> +	cf->can_id = id;
>> +
>> +	/* release receive buffer */
>> +	sunxi_can_write_cmdreg(priv, RELEASE_RBUF);
>> +
>> +	stats->rx_packets++;
>> +	stats->rx_bytes += cf->can_dlc;
>> +	netif_rx(skb);
>> +
>> +	can_led_event(dev, CAN_LED_EVENT_RX);
>> +}
>> +
>> +static int sunxi_can_err(struct net_device *dev, u8 isrc, u8 status)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	struct can_frame *cf;
>> +	struct sk_buff *skb;
>> +	enum can_state state = priv->can.state;
>> +	u32 ecc, alc;
>> +
>> +	skb = alloc_can_err_skb(dev, &cf);
>> +	if (!skb)
>> +		return -ENOMEM;
>> +
>> +	if (isrc & DATA_OR) {
>> +		/* data overrun interrupt */
>> +		netdev_dbg(dev, "data overrun interrupt\n");
>> +		cf->can_id |= CAN_ERR_CRTL;
>> +		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>> +		stats->rx_over_errors++;
>> +		stats->rx_errors++;
>> +		sunxi_can_write_cmdreg(priv, CLEAR_OR_FLAG);	/* clear bit */
>> +	}
>> +	if (isrc & ERR_WRN) {
>> +		/* error warning interrupt */
>> +		netdev_dbg(dev, "error warning interrupt\n");
>> +
>> +		if (status & BUS_OFF) {
>> +			state = CAN_STATE_BUS_OFF;
>> +			cf->can_id |= CAN_ERR_BUSOFF;
>> +			can_bus_off(dev);
>> +		} else if (status & ERR_STA) {
>> +			state = CAN_STATE_ERROR_WARNING;
>> +		} else {
>> +			state = CAN_STATE_ERROR_ACTIVE;
>> +		}
>> +	}
>> +	if (isrc & BUS_ERR) {
>> +		/* bus error interrupt */
>> +		netdev_dbg(dev, "bus error interrupt\n");
>> +		priv->can.can_stats.bus_error++;
>> +		stats->rx_errors++;
>> +
>> +		ecc = readl(priv->base + CAN_STA_ADDR);
>> +
>> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>> +
>> +		if (ecc & BIT_ERR) {
>> +			cf->data[2] |= CAN_ERR_PROT_BIT;
>> +		} else if (ecc & FORM_ERR) {
>> +			cf->data[2] |= CAN_ERR_PROT_FORM;
>> +		} else if (ecc & STUFF_ERR) {
>> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
>> +		} else {
>> +			cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>> +			cf->data[3] = (ecc & ERR_SEG_CODE) >> 16;
>> +		}
>> +		/* error occurred during transmission? */
>> +		if ((ecc & ERR_DIR) == 0)
>> +			cf->data[2] |= CAN_ERR_PROT_TX;
>> +	}
>> +	if (isrc & ERR_PASSIVE) {
>> +		/* error passive interrupt */
>> +		netdev_dbg(dev, "error passive interrupt\n");
>> +		if (status & ERR_STA)
>> +			state = CAN_STATE_ERROR_PASSIVE;
>> +		else
>> +			state = CAN_STATE_ERROR_ACTIVE;
>> +	}
>> +	if (isrc & ARB_LOST) {
>> +		/* arbitration lost interrupt */
>> +		netdev_dbg(dev, "arbitration lost interrupt\n");
>> +		alc = readl(priv->base + CAN_STA_ADDR);
>> +		priv->can.can_stats.arbitration_lost++;
>> +		stats->tx_errors++;
>> +		cf->can_id |= CAN_ERR_LOSTARB;
>> +		cf->data[0] = (alc & 0x1f) >> 8;
>> +	}
>> +	if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
>> +					 state == CAN_STATE_ERROR_PASSIVE)) {
>> +		u32 errc = readl(priv->base + CAN_ERRC_ADDR);
>> +		u8 rxerr = (errc >> 16) & 0xFF;
>> +		u8 txerr = errc & 0xFF;
>> +
>> +		cf->can_id |= CAN_ERR_CRTL;
>> +		if (state == CAN_STATE_ERROR_WARNING) {
>> +			priv->can.can_stats.error_warning++;
>> +			cf->data[1] = (txerr > rxerr) ?
>> +			    CAN_ERR_CRTL_TX_WARNING : CAN_ERR_CRTL_RX_WARNING;
>> +		} else {
>> +			priv->can.can_stats.error_passive++;
>> +			cf->data[1] = (txerr > rxerr) ?
>> +			    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
>> +		}
>> +		cf->data[6] = txerr;
>> +		cf->data[7] = rxerr;
>> +	}
>> +	priv->can.state = state;
>> +
>> +	stats->rx_packets++;
>> +	stats->rx_bytes += cf->can_dlc;
>> +	netif_rx(skb);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t sunxi_can_interrupt(int irq, void *dev_id)
>> +{
>> +	struct net_device *dev = (struct net_device *)dev_id;
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	u8 isrc, status;
>> +	int n = 0;
>> +
>> +	while ((isrc = readl(priv->base + CAN_INT_ADDR))
>> +	       && (n < SUNXI_CAN_MAX_IRQ)) {
>> +		n++;
>> +		status = readl(priv->base + CAN_STA_ADDR);
>> +
>> +		if (isrc & WAKEUP)
>> +			netdev_warn(dev, "wakeup interrupt\n");
>> +
>> +		if (isrc & TBUF_VLD) {
>> +			/* transmission complete interrupt */
>> +			stats->tx_bytes +=
>> +			    readl(priv->base + CAN_RBUF_RBACK_START_ADDR) & 0xf;
>> +			stats->tx_packets++;
>> +			can_get_echo_skb(dev, 0);
>> +			netif_wake_queue(dev);
>> +			can_led_event(dev, CAN_LED_EVENT_TX);
>> +		}
>> +		if (isrc & RBUF_VLD) {
>> +			/* receive interrupt */
>> +			while (status & RBUF_RDY) {
>> +			/* RX buffer is not empty */
>> +				sunxi_can_rx(dev);
>> +				status = readl(priv->base + CAN_STA_ADDR);
>> +			}
>> +		}
>> +		if (isrc &
>> +		    (DATA_OR | ERR_WRN | BUS_ERR | ERR_PASSIVE | ARB_LOST)) {
>> +			/* error interrupt */
>> +			if (sunxi_can_err(dev, isrc, status))
>> +				break;
>> +		}
>> +		/* clear the interrupt */
>> +		writel(isrc, priv->base + CAN_INT_ADDR);
>> +	}
>> +	if (n >= SUNXI_CAN_MAX_IRQ)
>> +		netdev_dbg(dev, "%d messages handled in ISR", n);
>> +
>> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +
>> +static int sunxican_open(struct net_device *dev)
>> +{
>> +	int err;
>> +
>> +	/* set chip into reset mode */
>> +	set_reset_mode(dev);
>> +
>> +	/* common open */
>> +	err = open_candev(dev);
>> +	if (err)
>> +		return err;
>> +
>> +	/* register interrupt handler */
> 
> please make it:
> 	err = request_irq();
> 	if (err) {
> 	}

fixed - in next version


>> +	if (request_irq
>> +	    (dev->irq, sunxi_can_interrupt, IRQF_TRIGGER_NONE, dev->name,
>                                             ^^^^^^^^^^^^^^^^^
> Please make it a IRQF_SHARED.
> 
> 
>> +	     (void *)dev)) {
>              ^^^^^^^^
> cast not needed

fixed - in next version. BTW: the sja1000 in linux-next has also this 
cast.

>> +		netdev_err(dev, "request_irq err: %d\n", err);
> close_candev() missing

fixed - in next version

>> +		return -EAGAIN;
> 
>> +	}
>> +	sunxican_set_bittiming(dev);
>> +	sunxi_can_start(dev);
>> +
>> +	can_led_event(dev, CAN_LED_EVENT_OPEN);
>> +
>> +	netif_start_queue(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxican_close(struct net_device *dev)
>> +{
>> +	netif_stop_queue(dev);
>> +	set_reset_mode(dev);
>> +
>> +	free_irq(dev->irq, (void *)dev);
>> +
>> +	close_candev(dev);
>> +
>> +	can_led_event(dev, CAN_LED_EVENT_STOP);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct net_device_ops sunxican_netdev_ops = {
>> +	.ndo_open = sunxican_open,
>> +	.ndo_stop = sunxican_close,
>> +	.ndo_start_xmit = sunxican_start_xmit,
>> +};
>> +
>> +static const struct of_device_id sunxican_of_match[] = {
>> +	{.compatible = "allwinner,sunxican"},
>> +	{},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, sunxican_of_match);
>> +
>> +static const struct platform_device_id sunxican_id_table[] = {
>> +	{.name = "sunxican"},
>> +	{},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(platform, sunxican_id_table);
>> +
>> +static int sunxican_remove(struct platform_device *pdev)
>> +{
>> +	struct net_device *dev = platform_get_drvdata(pdev);
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	struct resource *res;
>> +
>> +	set_reset_mode(dev);
>> +	unregister_netdev(dev);
>> +	iounmap(priv->base);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	release_mem_region(res->start, resource_size(res));
>> +
>> +	free_candev(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxican_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	struct clk *clk;
>> +	void __iomem *addr;
>> +	int err, irq;
>> +	u32 temp_irqen;
>> +	struct net_device *dev;
>> +	struct sunxican_priv *priv;
>> +
>> +	clk = clk_get(&pdev->dev, "apb1_can");
>> +	if (IS_ERR(clk)) {
>> +		dev_err(&pdev->dev, "no clock defined\n");
>> +		err = -ENODEV;
>> +		goto exit;
>> +	}
>> +	/* turn on clocking for CAN peripheral block */
>> +	err = clk_prepare_enable(clk);
> 
> Please move the clock handling into the open() function. Or better 
> think
> about implementing runtimepm.

Why ? It feels like the right place. The CAN core clock starts when the 
module is
loaded.

> 
>> +	if (err) {
>> +		dev_err(&pdev->dev, "could not enable clocking (apb1_can)\n");
>> +		goto exit;
>> +	}
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	irq = platform_get_irq(pdev, 0);
>> +
>> +	if (!res || irq <= 0) {
>> +		dev_err(&pdev->dev, "could not get a valid irq\n");
>> +		err = -ENODEV;
>> +		goto exit_put;
>> +	}
>> +	if (!request_mem_region(res->start, resource_size(res), pdev->name)) 
>> {
>> +		dev_err(&pdev->dev, "could not get io memory resource\n");
>> +		err = -EBUSY;
>> +		goto exit_put;
>> +	}
>> +	addr = ioremap_nocache(res->start, resource_size(res));
>> +	if (!addr) {
>> +		dev_err(&pdev->dev, "could not map io memory\n");
>> +		err = -ENOMEM;
>> +		goto exit_release;
>> +	}
>> +	dev = alloc_candev(sizeof(struct sunxican_priv), 1);
>> +
>> +	if (!dev) {
>> +		dev_err(&pdev->dev,
>> +			"could not allocate memory for CAN device\n");
>> +		err = -ENOMEM;
>> +		goto exit_iounmap;
>> +	}
>> +
>> +	dev->netdev_ops = &sunxican_netdev_ops;
>> +	dev->irq = irq;
>> +	dev->flags |= IFF_ECHO;
>> +
>> +	priv = netdev_priv(dev);
>> +	priv->can.clock.freq = clk_get_rate(clk);
>> +	priv->can.bittiming_const = &sunxican_bittiming_const;
>> +	priv->can.do_set_mode = sunxican_set_mode;
>> +	priv->can.do_get_berr_counter = sunxican_get_berr_counter;
>> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
>> +	    CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_LOOPBACK |
>> +	    CAN_CTRLMODE_3_SAMPLES;
>> +	priv->base = addr;
>> +	priv->clk = clk;
>> +	spin_lock_init(&priv->cmdreg_lock);
>> +
>> +	/* enable CAN specific interrupts */
>> +	set_reset_mode(dev);
>> +	temp_irqen = BERR_IRQ_EN | ERR_PASSIVE_IRQ_EN | OR_IRQ_EN | 
>> RX_IRQ_EN;
>> +	writel(readl(priv->base + CAN_INTEN_ADDR) | temp_irqen,
>> +	       priv->base + CAN_INTEN_ADDR);
>> +
>> +	platform_set_drvdata(pdev, dev);
>> +	SET_NETDEV_DEV(dev, &pdev->dev);
>> +
>> +	err = register_candev(dev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
>> +			DRV_NAME, err);
>> +		goto exit_free;
>> +	}
>> +	devm_can_led_init(dev);
>> +
>> +	dev_info(&pdev->dev, "device registered (base=%p, irq=%d)\n",
>> +		 priv->base, dev->irq);
>> +
>> +	return 0;
>> +
>> +exit_free:
>> +	free_candev(dev);
>> +exit_iounmap:
>> +	iounmap(addr);
>> +exit_release:
>> +	release_mem_region(res->start, resource_size(res));
>> +exit_put:
>> +	clk_put(clk);
>> +exit:
>> +	return err;
>> +}
>> +
>> +static int __maybe_unused sunxi_can_suspend(struct device *device)
>> +{
>> +	struct net_device *dev = dev_get_drvdata(device);
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +
>> +	if (netif_running(dev)) {
>> +		netif_stop_queue(dev);
>> +		netif_device_detach(dev);
>> +	}
>> +	priv->can.state = CAN_STATE_SLEEPING;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused sunxi_can_resume(struct device *device)
>> +{
>> +	struct net_device *dev = dev_get_drvdata(device);
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +
>> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +	if (netif_running(dev)) {
>> +		netif_device_attach(dev);
>> +		netif_start_queue(dev);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(sunxi_can_pm_ops, sunxi_can_suspend, 
>> sunxi_can_resume);
>> +
>> +static struct platform_driver sunxi_can_driver = {
>> +	.driver = {
>> +		   .name = DRV_NAME,
> 
> Please remove the .name.

Without it, the kernel oops

> 
>> +		   .owner = THIS_MODULE,
> 
> Please indent with just two tabs here.
> 
>> +		   .pm = &sunxi_can_pm_ops,
>> +		   .of_match_table = sunxican_of_match,
>> +		   },
>> +	.probe = sunxican_probe,
>> +	.remove = sunxican_remove,
>> +	.id_table = sunxican_id_table,
>> +};
>> +
>> +module_platform_driver(sunxi_can_driver);
>> +
>> +MODULE_AUTHOR("Peter Chen <xingkongcp@gmail.com>");
>> +MODULE_AUTHOR("Gerhard Bertelsmann <info@gerhard-bertelsmann.de>");
> 
> Oh nice, I didn't know that you can use two MODULE_AUTHOR lines.
> 
>> +MODULE_LICENSE("Dual BSD/GPL");
>                    ^^^^^^^^^^^^
> The header does only state GPL

fixed - in next version

>> +MODULE_DESCRIPTION(DRV_NAME "CAN driver for Allwinner SoCs 
>> (A10/A20)");
>> +MODULE_VERSION(DRV_MODULE_VERSION);
> 
> Marc


Gerd

Re: [PATCH v2 2/2] Allwinner SoC CAN controller Support

[Marc Kleine-Budde]

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

On 09/10/2015 08:29 AM, Gerhard Bertelsmann wrote:

>>> +
>>> +/* Registers address */
>>
>> Please prefix _all_ your defines with a common prefix, e.g. SUNXI_
>>
>>> +#define CAN_BASE0		0x01C2BC00
>>
>> Please remove that define, the base address comes from the DT now.
> 
> fixed - in next version, but I personally like to see which IO address
> is the code talking about. If you aren't aware of the address, you have
> need some time to find it in the docs.

This might make sense now, where there only a single instance of this
core on 2 SoC. But in general it's of no use, as the base address comes
from the DT, have a look at the flexcan core, it's supported on more
then 5 SoCs, with more than one cores on some SoCs. This define is not
used it's redundant information just look at the DT, that's where the
base address comes from.

[...]

>>> +static void sunxi_can_write_cmdreg(struct sunxican_priv *priv, u8 
>>> val)
>>> +{
>>> +	unsigned long flags;
>>> +
>>> +	 /* The command register needs some locking and time to settle
>>> +	  * the write_reg() operation - especially on SMP systems.
>>> +	  */
>>
>> Is this needed with your SJA1000 alike IP core?
> 
> IMHO the Allwinner CAN IP isn't a real SJA1000. It's like a SJA1000 -
> not more. The extra reading doesn't hurt IMHO ...

just performance - keep it just to be on the save side.

[...]

>>> +{
>>> +	struct sunxican_priv *priv = netdev_priv(dev);
>>> +	u8 status = readl(priv->base + CAN_MSEL_ADDR);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < 100; i++) {
>>> +		/* check reset bit */
>>> +		if ((status & RESET_MODE) == 0) {
>>> +			priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> +			/* enable interrupts */
>>> +			if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
>>> +				writel(0xFFFF, priv->base + CAN_INTEN_ADDR);
>>> +			} else {
>>> +				writel(0xFFFF & ~BERR_IRQ_EN,
>>> +				       priv->base + CAN_INTEN_ADDR);
>>> +			}
>>> +
>>> +			if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>>> +				/* Put device into loopback mode */
>>
>> That's not loopback mode according to socketcan, that's
>> CAN_CTRLMODE_PRESUME_ACK.
> 
> isn't CAN_CTRLMODE_LOOPBACK loopback mode ? May I can't see the obvious 
> here ...

No - look at the datsheet - and the original sja1000 driver

CAN_CTRLMODE_LOOPBACK is command register bit 4
And it means you receive the frame (via the transceiver) you are just
sending.

CAN_CTRLMODE_PRESUME_ACK means the frame is send successfully, even if
there is no other CAN station on the bus. The other station is supposed
to send an ACK to signal correct reception, if there is no ACK the frame
is repeated. According to the datasheet that is what mode selection
register bit 2 does.

>>> +				writel(readl(priv->base + CAN_MSEL_ADDR) |
>>> +				       LOOPBACK_MODE,
>>> +				       priv->base + CAN_MSEL_ADDR);
>>> +			} else if
>>> +				(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
>>> +				/* Put device into listen-only mode */
>>> +				writel(readl(priv->base + CAN_MSEL_ADDR) |
>>> +				       LISTEN_ONLY_MODE,
>>> +				       priv->base + CAN_MSEL_ADDR);
>>> +			}
>>> +			return;
>>> +		}
>>> +		/* set chip to normal mode */
>>> +		writel(readl(priv->base + CAN_MSEL_ADDR) & (~RESET_MODE),
>>> +		       priv->base + CAN_MSEL_ADDR);
>>> +		status = readl(priv->base + CAN_MSEL_ADDR);
>>> +	}

[...]

>>> +/* transmit a CAN message
>>> + * message layout in the sk_buff should be like this:
>>> + * xx xx xx xx         ff         ll 00 11 22 33 44 55 66 77
>>> + * [ can_id ] [flags] [len] [can data (up to 8 bytes]
>>> + */
>>> +static int sunxican_start_xmit(struct sk_buff *skb, struct net_device 
>>> *dev)
>>> +{
>>> +	struct sunxican_priv *priv = netdev_priv(dev);
>>> +	struct can_frame *cf = (struct can_frame *)skb->data;
>>> +	u8 dlc;
>>> +	canid_t id;
>>> +	u32 temp = 0;
>>> +	int i;
>>> +
>>> +	/* wait buffer ready */
>>> +	while (!(readl(priv->base + CAN_STA_ADDR) & TBUF_RDY))
>>> +		usleep_range(10, 100);
>>
>> Can you explain why you need this sleep here?
> 
> IMHO its needed. How could you guarantee that the previous transmit is 
> finished
> otherwise ?

Nope - this driver already implements proper flow control.... see below:

>>> +
>>> +	if (can_dropped_invalid_skb(dev, skb))
>>> +		return NETDEV_TX_OK;
>>> +
>>> +	netif_stop_queue(dev);

The core has just one transmit buffer. Right?
So when sending a CAN frame the send queue from the networking stack
into the driver is stopped. In the interrupt handler, when a TX-complete
interrupt occurs the queue is started again.

For testing purposes you can add the following:

	if (!(readl(priv->base + CAN_STA_ADDR) & TBUF_RDY))
		printf("%s: xmit while buffer not ready\n", __func__);

>>> +
>>> +	dlc = cf->can_dlc;
>>> +	id = cf->can_id;
>>> +
>>> +	temp = ((id >> 30) << 6) | dlc;
>>
>> Please don't reply on the fact that the upper two bits of id match the
>> upper two bits for temp. Please use an explizid conversion via:
>> 	if (id & ..)
>> 		temp |= BIT_FOO;
> 
> fixed - in next version
> 
>>
>>> +	writel(temp, priv->base + CAN_BUF0_ADDR);
>>> +	if (id & CAN_EFF_FLAG) {
>>> +		/* extended frame */
>>> +		writel((id >> 21) & 0xFF, priv->base + CAN_BUF1_ADDR);
>>> +		writel((id >> 13) & 0xFF, priv->base + CAN_BUF2_ADDR);
>>> +		writel((id >> 5)  & 0xFF, priv->base + CAN_BUF3_ADDR);
>>> +		writel((id & 0x1F) << 3,  priv->base + CAN_BUF4_ADDR);
>>
>> Can you make it always first shift then mask or the other way round. 
>> see
>> the original sja1000 xmit() functio, that look pretty neat.
> 
> fixed - in next version. I like shifting first and mask afterwards.
> In fact, it's using less soucre code chars ;-)

Do as you like, but make it the same for all assignments :) Or better
copy the code from the original sja1000 driver.

> 
>>
>>> +
>>> +		for (i = 0; i < dlc; i++) {
>>> +			writel(cf->data[i],
>>> +			       priv->base + (CAN_BUF5_ADDR + i * 4));
>>> +		}
>>> +	} else {
>>> +		/* standard frame */
>>> +		writel((id >> 3) & 0xFF, priv->base + CAN_BUF1_ADDR);
>>> +		writel((id & 0x7) << 5, priv->base + CAN_BUF2_ADDR);
>>> +
>>> +		for (i = 0; i < dlc; i++)
>>> +			writel(cf->data[i], priv->base + CAN_BUF3_ADDR + i * 4);
>>> +	}
>>> +	can_put_echo_skb(skb, dev, 0);
>>
>> Do you have support for one_shot or loopback mode here, too. Like the
>> sja1000?
> 
> Hmm - will have a look at this ...

See discussion about loopback above (and the original sja100 driver).

[...]

>>> +	if (request_irq
>>> +	    (dev->irq, sunxi_can_interrupt, IRQF_TRIGGER_NONE, dev->name,
>>                                             ^^^^^^^^^^^^^^^^^
>> Please make it a IRQF_SHARED.
>>
>>
>>> +	     (void *)dev)) {
>>              ^^^^^^^^
>> cast not needed
> 
> fixed - in next version. BTW: the sja1000 in linux-next has also this 
> cast.

But we want to do better, don't we. (send patches if you like.)

>>> +	clk = clk_get(&pdev->dev, "apb1_can");
>>> +	if (IS_ERR(clk)) {
>>> +		dev_err(&pdev->dev, "no clock defined\n");
>>> +		err = -ENODEV;
>>> +		goto exit;
>>> +	}
>>> +	/* turn on clocking for CAN peripheral block */
>>> +	err = clk_prepare_enable(clk);
>>
>> Please move the clock handling into the open() function. Or better 
>> think
>> about implementing runtimepm.
> 
> Why ? It feels like the right place. The CAN core clock starts when the 
> module is
> loaded.

Naively thinking yes, but why turn on the clocks when you load the
module? Just increases power consumption but serves no purpose. So move
prepare_enable and disable_unprepare into open/close. We have to take
care about get_berrcounter, though.

>>> +static SIMPLE_DEV_PM_OPS(sunxi_can_pm_ops, sunxi_can_suspend, 
>>> sunxi_can_resume);

BTW: have you tested suspend and resume?

>>> +
>>> +static struct platform_driver sunxi_can_driver = {
>>> +	.driver = {
>>> +		   .name = DRV_NAME,
>>
>> Please remove the .name.
> 
> Without it, the kernel oops

Doh! But there was one assignment here, that was not needed...I'll try
to remember which one it was.

>>
>>> +		   .owner = THIS_MODULE,

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH v2 2/2] Allwinner SoC CAN controller Support

[Mirza Krak]

2015-09-10 10:17 GMT+02:00 Marc Kleine-Budde <mkl@pengutronix.de>:
> On 09/10/2015 08:29 AM, Gerhard Bertelsmann wrote:
>>>> +
>>>> +static struct platform_driver sunxi_can_driver = {
>>>> +   .driver = {
>>>> +              .name = DRV_NAME,
>>>
>>> Please remove the .name.
>>
>> Without it, the kernel oops
>
> Doh! But there was one assignment here, that was not needed...I'll try
> to remember which one it was.
>

I believe

    .owner = THIS_MODULE,

is not needed.


-- 
Med Vänliga Hälsningar / Best Regards

*******************************************************************
Mirza Krak
Host Mobility AB
mirza.krak@hostmobility.com
Anders Personsgatan 12, 416 64 Göteborg
Sweden
http://www.hostmobility.com
Direct: +46 31 31 32 704
Phone: +46 31 31 32 700
Fax: +46 31 80 67 51
Mobile: +46 730 28 06 22
*******************************************************************

Re: [PATCH v2 2/2] Allwinner SoC CAN controller Support

[Marc Kleine-Budde]

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

On 09/10/2015 10:29 AM, Mirza Krak wrote:
> 2015-09-10 10:17 GMT+02:00 Marc Kleine-Budde <mkl@pengutronix.de>:
>> On 09/10/2015 08:29 AM, Gerhard Bertelsmann wrote:
>>>>> +
>>>>> +static struct platform_driver sunxi_can_driver = {
>>>>> +   .driver = {
>>>>> +              .name = DRV_NAME,
>>>>
>>>> Please remove the .name.
>>>
>>> Without it, the kernel oops
>>
>> Doh! But there was one assignment here, that was not needed...I'll try
>> to remember which one it was.
>>
> 
> I believe
> 
>     .owner = THIS_MODULE,
> 
> is not needed.

Makes sense.

Thanks,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]