* [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
@ 2014-06-27 10:00 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-06-27 10:00 UTC (permalink / raw)
To: linux-can; +Cc: netdev, wg, mkl, devicetree
The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller.
For TX, only one dedicated tx buffer is used for sending data.
For RX, RXFIFO 0 is used for receiving data to avoid overflow.
Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
Due to the message ram can be shared by multi m_can instances
and the fifo element is configurable which is SoC dependant,
the design is to parse the message ram related configuration data from device
tree rather than hardcode define it in driver which can make the message
ram sharing fully transparent to M_CAN controller driver,
then we can gain better driver maintainability and future features upgrade.
M_CAN also supports CANFD protocol features like data payload up to 64 bytes
and bitrate switch at runtime, however, this patch still does not add the
support for these features.
Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
.../devicetree/bindings/net/can/m_can.txt | 29 +
drivers/net/can/Kconfig | 5 +
drivers/net/can/Makefile | 1 +
drivers/net/can/m_can.c | 867 ++++++++++++++++++++
4 files changed, 902 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
new file mode 100644
index 0000000..2b22d02
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -0,0 +1,29 @@
+Bosch MCAN controller Device Tree Bindings
+-------------------------------------------------
+
+Required properties:
+- compatible : Should be "bosch,m_can" for M_CAN controllers
+- reg : physical base address and size of the M_CAN
+ registers map and message ram
+- interrupts : property with a value describing the interrupt
+ number
+- clocks : clocks used by controller
+- mram-cfg : message ram configuration data, the format is
+ <offset sidf_elems xidf_elems rxf1_elems rxb_elems txe_elems txb_elems>
+ The 'offset' is the address offset inside the message ram. This is usually set
+ if you're using the shared message ram while the other part is used by another
+ m_can controller.
+ The left cell are all the number of each elements inside the message ram.
+ Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
+ for each elements definition.
+
+Example:
+canfd1: canfd@020e8000 {
+ compatible = "bosch,m_can";
+ reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
+ reg-names = "canfd", "message_ram";
+ interrupts = <0 114 0x04>;
+ clocks = <&clks IMX6SX_CLK_CANFD>;
+ mram-cfg = <0x0 0 0 32 32 32 0 1>;
+ status = "disabled";
+};
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 4168822..3fd1c4a 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -137,6 +137,11 @@ config CAN_XILINXCAN
Xilinx CAN driver. This driver supports both soft AXI CAN IP and
Zynq CANPS IP.
+config CAN_M_CAN
+ tristate "Bosch M_CAN devices"
+ ---help---
+ Say Y here if you want to support for Bosch M_CAN controller.
+
source "drivers/net/can/mscan/Kconfig"
source "drivers/net/can/sja1000/Kconfig"
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 1697f22..69dee2c 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -17,6 +17,7 @@ obj-y += softing/
obj-$(CONFIG_CAN_SJA1000) += sja1000/
obj-$(CONFIG_CAN_MSCAN) += mscan/
obj-$(CONFIG_CAN_C_CAN) += c_can/
+obj-$(CONFIG_CAN_M_CAN) += m_can.o
obj-$(CONFIG_CAN_CC770) += cc770/
obj-$(CONFIG_CAN_AT91) += at91_can.o
obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
diff --git a/drivers/net/can/m_can.c b/drivers/net/can/m_can.c
new file mode 100644
index 0000000..61e9a8e
--- /dev/null
+++ b/drivers/net/can/m_can.c
@@ -0,0 +1,867 @@
+/*
+ * CAN bus driver for Bosch M_CAN controller
+ *
+ * Copyright (C) 2014 Freescale Semiconductor, Inc.
+ * Dong Aisheng <b29396@freescale.com>
+ *
+ * Bosch M_CAN user manual can be obtained from:
+ * http://www.bosch-semiconductors.de/media/pdf_1/ipmodules_1/m_can/
+ * mcan_users_manual_v302.pdf
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/if_arp.h>
+#include <linux/if_ether.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+
+#include <linux/can/dev.h>
+
+/* napi related */
+#define M_CAN_NAPI_WEIGHT 64
+
+/* registers definition */
+enum m_can_reg {
+ M_CAN_CREL = 0x0,
+ M_CAN_ENDN = 0x4,
+ M_CAN_CUST = 0x8,
+ M_CAN_FBTP = 0xc,
+ M_CAN_TEST = 0x10,
+ M_CAN_RWD = 0x14,
+ M_CAN_CCCR = 0x18,
+ M_CAN_BTP = 0x1c,
+ M_CAN_TSCC = 0x20,
+ M_CAN_TSCV = 0x24,
+ M_CAN_TOCC = 0x28,
+ M_CAN_TOCV = 0x2c,
+ M_CAN_ECR = 0x40,
+ M_CAN_PSR = 0x44,
+ M_CAN_IR = 0x50,
+ M_CAN_IE = 0x54,
+ M_CAN_ILS = 0x58,
+ M_CAN_ILE = 0x5c,
+ M_CAN_GFC = 0x80,
+ M_CAN_SIDFC = 0x84,
+ M_CAN_XIDFC = 0x88,
+ M_CAN_XIDAM = 0x90,
+ M_CAN_HPMS = 0x94,
+ M_CAN_NDAT1 = 0x98,
+ M_CAN_NDAT2 = 0x9c,
+ M_CAN_RXF0C = 0xa0,
+ M_CAN_RXF0S = 0xa4,
+ M_CAN_RXF0A = 0xa8,
+ M_CAN_RXBC = 0xac,
+ M_CAN_RXF1C = 0xb0,
+ M_CAN_RXF1S = 0xb4,
+ M_CAN_RXF1A = 0xb8,
+ M_CAN_RXESC = 0xbc,
+ M_CAN_TXBC = 0xc0,
+ M_CAN_TXFQS = 0xc4,
+ M_CAN_TXESC = 0xc8,
+ M_CAN_TXBRP = 0xcc,
+ M_CAN_TXBAR = 0xd0,
+ M_CAN_TXBCR = 0xd4,
+ M_CAN_TXBTO = 0xd8,
+ M_CAN_TXBCF = 0xdc,
+ M_CAN_TXBTIE = 0xe0,
+ M_CAN_TXBCIE = 0xe4,
+ M_CAN_TXEFC = 0xf0,
+ M_CAN_TXEFS = 0xf4,
+ M_CAN_TXEFA = 0xf8,
+};
+
+/* CC Control Register(CCCR) */
+#define CCCR_CCE BIT(1)
+#define CCCR_INIT BIT(0)
+
+/* Bit Timing & Prescaler Register (BTP) */
+#define BTR_BRP_MASK 0x3ff
+#define BTR_BRP_SHIFT 16
+#define BTR_TSEG1_SHIFT 8
+#define BTR_TSEG1_MASK (0x3f << BTR_TSEG1_SHIFT)
+#define BTR_TSEG2_SHIFT 4
+#define BTR_TSEG2_MASK (0xf << BTR_TSEG2_SHIFT)
+#define BTR_SJW_SHIFT 0
+#define BTR_SJW_MASK 0xf
+
+/* Interrupt Register(IR) */
+#define IR_ALL_INT 0xffffffff
+#define IR_STE BIT(31)
+#define IR_FOE BIT(30)
+#define IR_ACKE BIT(29)
+#define IR_BE BIT(28)
+#define IR_CRCE BIT(27)
+#define IR_WDI BIT(26)
+#define IR_BO BIT(25)
+#define IR_EW BIT(24)
+#define IR_EP BIT(23)
+#define IR_ELO BIT(22)
+#define IR_BEU BIT(21)
+#define IR_BEC BIT(20)
+#define IR_DRX BIT(19)
+#define IR_TOO BIT(18)
+#define IR_MRAF BIT(17)
+#define IR_TSW BIT(16)
+#define IR_TEFL BIT(15)
+#define IR_TEFF BIT(14)
+#define IR_TEFW BIT(13)
+#define IR_TEFN BIT(12)
+#define IR_TFE BIT(11)
+#define IR_TCF BIT(10)
+#define IR_TC BIT(9)
+#define IR_HPM BIT(8)
+#define IR_RF1L BIT(7)
+#define IR_RF1F BIT(6)
+#define IR_RF1W BIT(5)
+#define IR_RF1N BIT(4)
+#define IR_RF0L BIT(3)
+#define IR_RF0F BIT(2)
+#define IR_RF0W BIT(1)
+#define IR_RF0N BIT(0)
+#define IR_ERR_ALL (IR_STE | IR_FOE | IR_ACKE | IR_BE | IR_CRCE | \
+ IR_WDI | IR_BO | IR_EW | IR_EP | IR_ELO | IR_BEU | \
+ IR_BEC | IR_TOO | IR_MRAF | IR_TSW | IR_TEFL | IR_RF1L | \
+ IR_RF0L)
+
+/* Rx FIFO 0/1 Configuration (RXF0C/RXF1C) */
+#define RXFC_FWM_OFF 24
+#define RXFC_FWM_MASK 0x7f
+#define RXFC_FWM_1 (1 << RXFC_FWM_OFF)
+#define RXFC_FS_OFF 16
+#define RXFC_FS_MASK 0x7f
+
+/* Rx FIFO 0/1 Status (RXF0S/RXF1S) */
+#define RXFS_RFL BIT(25)
+#define RXFS_FF BIT(24)
+#define RXFS_FPI_OFF 16
+#define RXFS_FPI_MASK 0x3f0000
+#define RXFS_FGI_OFF 8
+#define RXFS_FGI_MASK 0x3f00
+#define RXFS_FFL_MASK 0x7f
+
+/* Tx Buffer Configuration(TXBC) */
+#define TXBC_NDTB_OFF 16
+#define TXBC_NDTB_MASK 0x3f
+
+/* Tx Buffer Element Size Configuration(TXESC) */
+#define TXESC_TBDS_8BYTES 0x0
+/* Tx Buffer Element */
+#define TX_BUF_XTD BIT(30)
+#define TX_BUF_RTR BIT(29)
+
+/* Rx Buffer Element Size Configuration(TXESC) */
+#define M_CAN_RXESC_8BYTES 0x0
+/* Tx Buffer Element */
+#define RX_BUF_ESI BIT(31)
+#define RX_BUF_XTD BIT(30)
+#define RX_BUF_RTR BIT(29)
+
+/* Message RAM Configuration (in bytes) */
+#define SIDF_ELEMENT_SIZE 4
+#define XIDF_ELEMENT_SIZE 8
+#define RXF0_ELEMENT_SIZE 16
+#define RXF1_ELEMENT_SIZE 16
+#define RXB_ELEMENT_SIZE 16
+#define TXE_ELEMENT_SIZE 8
+#define TXB_ELEMENT_SIZE 16
+
+/* m_can private data structure */
+struct m_can_priv {
+ struct can_priv can; /* must be the first member */
+ struct napi_struct napi;
+ struct net_device *dev;
+ struct device *device;
+ struct clk *clk;
+ void __iomem *base;
+ u32 irqstatus;
+
+ /* message ram configuration */
+ void __iomem *mram_base;
+ u32 mram_off;
+ u32 sidf_elems;
+ u32 sidf_off;
+ u32 xidf_elems;
+ u32 xidf_off;
+ u32 rxf0_elems;
+ u32 rxf0_off;
+ u32 rxf1_elems;
+ u32 rxf1_off;
+ u32 rxb_elems;
+ u32 rxb_off;
+ u32 txe_elems;
+ u32 txe_off;
+ u32 txb_elems;
+ u32 txb_off;
+};
+
+static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)
+{
+ return readl(priv->base + reg);
+}
+
+static inline void m_can_write(const struct m_can_priv *priv,
+ enum m_can_reg reg, u32 val)
+{
+ writel(val, priv->base + reg);
+}
+
+static inline void m_can_config_endisable(const struct m_can_priv *priv,
+ bool enable)
+{
+ u32 cccr = m_can_read(priv, M_CAN_CCCR);
+ u32 timeout = 10, val;
+
+ if (enable) {
+ /* enable m_can configuration */
+ m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
+ /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
+ m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
+ } else {
+ m_can_write(priv, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
+ }
+
+ /* there's a delay for module initialization */
+ val = enable ? CCCR_INIT | CCCR_CCE : 0;
+ while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE))
+ != val) {
+ if (timeout == 0) {
+ netdev_warn(priv->dev, "Failed to init module\n");
+ return;
+ }
+ timeout--;
+ udelay(1);
+ }
+}
+
+static void m_can_enable_all_interrupts(struct m_can_priv *priv, bool enable)
+{
+ m_can_write(priv, M_CAN_ILE, enable ? 0x00000003 : 0x0);
+}
+
+static int m_can_do_rx_poll(struct net_device *dev, int quota)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+ struct net_device_stats *stats = &dev->stats;
+ struct sk_buff *skb;
+ struct can_frame *frame;
+ u32 rxfs, flags, fgi;
+ u32 num_rx_pkts = 0;
+
+ rxfs = m_can_read(priv, M_CAN_RXF0S);
+ if (!(rxfs & RXFS_FFL_MASK)) {
+ netdev_dbg(dev, "no messages in fifo0\n");
+ return 0;
+ }
+
+ while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
+ netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
+ if (rxfs & RXFS_RFL)
+ netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
+
+ skb = alloc_can_skb(dev, &frame);
+ if (!skb) {
+ stats->rx_dropped++;
+ return -ENOMEM;
+ }
+
+ fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
+ flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
+ if (flags & RX_BUF_XTD)
+ frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
+ else
+ frame->can_id = (flags >> 18) & CAN_SFF_MASK;
+ netdev_dbg(dev, "R0 0x%x\n", flags);
+
+ if (flags & RX_BUF_RTR) {
+ frame->can_id |= CAN_RTR_FLAG;
+ } else {
+ flags = readl(priv->mram_base +
+ priv->rxf0_off + fgi * 16 + 0x4);
+ frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
+ netdev_dbg(dev, "R1 0x%x\n", flags);
+
+ *(u32 *)(frame->data + 0) = readl(priv->mram_base +
+ priv->rxf0_off + fgi * 16 + 0x8);
+ *(u32 *)(frame->data + 4) = readl(priv->mram_base +
+ priv->rxf0_off + fgi * 16 + 0xC);
+ netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
+ netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
+ }
+
+ /* acknowledge rx fifo 0 */
+ m_can_write(priv, M_CAN_RXF0A, fgi);
+
+ netif_receive_skb(skb);
+ netdev_dbg(dev, "new packet received\n");
+
+ stats->rx_packets++;
+ stats->rx_bytes += frame->can_dlc;
+
+ can_led_event(dev, CAN_LED_EVENT_RX);
+
+ quota--;
+ num_rx_pkts++;
+ rxfs = m_can_read(priv, M_CAN_RXF0S);
+ };
+
+ return num_rx_pkts;
+}
+
+static int m_can_poll(struct napi_struct *napi, int quota)
+{
+ struct net_device *dev = napi->dev;
+ struct m_can_priv *priv = netdev_priv(dev);
+ u32 work_done = 0;
+ u32 irqstatus;
+
+ irqstatus = m_can_read(priv, M_CAN_IR);
+ if (irqstatus)
+ netdev_dbg(dev, "irqstatus updated! new 0x%x old 0x%x rxf0s 0x%x\n",
+ irqstatus, priv->irqstatus,
+ m_can_read(priv, M_CAN_RXF0S));
+
+ irqstatus = irqstatus | priv->irqstatus;
+ if (!irqstatus)
+ goto end;
+
+ if (irqstatus & IR_RF0N)
+ /* handle events corresponding to receive message objects */
+ work_done += m_can_do_rx_poll(dev, (quota - work_done));
+
+ if (work_done < quota) {
+ napi_complete(napi);
+ /* enable all IRQs */
+ m_can_enable_all_interrupts(priv, true);
+ }
+
+end:
+ return work_done;
+}
+
+static irqreturn_t m_can_isr(int irq, void *dev_id)
+{
+ struct net_device *dev = (struct net_device *)dev_id;
+ struct m_can_priv *priv = netdev_priv(dev);
+ struct net_device_stats *stats = &dev->stats;
+ u32 ir;
+
+ ir = m_can_read(priv, M_CAN_IR);
+ if (!ir)
+ return IRQ_NONE;
+
+ netdev_dbg(dev, "ir 0x%x rxfs0 0x%x\n", ir,
+ m_can_read(priv, M_CAN_RXF0S));
+
+ /* ACK all irqs */
+ if (ir & IR_ALL_INT)
+ m_can_write(priv, M_CAN_IR, ir);
+
+ if (ir & IR_ERR_ALL) {
+ netdev_dbg(dev, "bus error\n");
+ /* TODO: handle bus error */
+ }
+
+ /* save irqstatus for later using */
+ priv->irqstatus = ir;
+
+ /*
+ * schedule NAPI in case of
+ * - rx IRQ
+ * - state change IRQ(TODO)
+ * - bus error IRQ and bus error reporting (TODO)
+ */
+ if (ir & IR_RF0N) {
+ m_can_enable_all_interrupts(priv, false);
+ napi_schedule(&priv->napi);
+ }
+
+ /* FIFO overflow */
+ if (ir & IR_RF0L) {
+ dev->stats.rx_over_errors++;
+ dev->stats.rx_errors++;
+ }
+
+ /* transmission complete interrupt */
+ if (ir & IR_TC) {
+ netdev_dbg(dev, "tx complete\n");
+ stats->tx_bytes += can_get_echo_skb(dev, 0);
+ stats->tx_packets++;
+ can_led_event(dev, CAN_LED_EVENT_TX);
+ netif_wake_queue(dev);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static const struct can_bittiming_const m_can_bittiming_const = {
+ .name = KBUILD_MODNAME,
+ .tseg1_min = 2, /* Time segment 1 = prop_seg + phase_seg1 */
+ .tseg1_max = 64,
+ .tseg2_min = 1, /* Time segment 2 = phase_seg2 */
+ .tseg2_max = 16,
+ .sjw_max = 16,
+ .brp_min = 1,
+ .brp_max = 1024,
+ .brp_inc = 1,
+};
+
+static int m_can_set_bittiming(struct net_device *dev)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+ const struct can_bittiming *bt = &priv->can.bittiming;
+ u16 brp, sjw, tseg1, tseg2;
+ u32 reg_btp;
+
+ brp = bt->brp - 1;
+ sjw = bt->sjw - 1;
+ tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
+ tseg2 = bt->phase_seg2 - 1;
+ reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
+ (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
+ m_can_write(priv, M_CAN_BTP, reg_btp);
+ netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp);
+
+ return 0;
+}
+
+/*
+ * Configure M_CAN chip:
+ * - set rx buffer/fifo element size
+ * - configure rx fifo
+ * - accept non-matching frame into fifo 0
+ * - configure tx buffer
+ * - setup bittiming
+ * - TODO:
+ * 1) other working modes support like monitor, loopback...
+ * 2) lec error status report enable
+ */
+static void m_can_chip_config(struct net_device *dev)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+
+ m_can_config_endisable(priv, true);
+
+ /* RX Buffer/FIFO Element Size 8 bytes data field */
+ m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES);
+
+ /* Accept Non-matching Frames Into FIFO 0 */
+ m_can_write(priv, M_CAN_GFC, 0x0);
+
+ /* only support one Tx Buffer currently */
+ m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
+ (priv->mram_off + priv->txb_off));
+
+ /* only support 8 bytes firstly */
+ m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
+
+ m_can_write(priv, M_CAN_TXEFC, 0x00010000 |
+ (priv->mram_off + priv->txe_off));
+
+ /* rx fifo configuration, blocking mode, fifo size 1 */
+ m_can_write(priv, M_CAN_RXF0C, (priv->rxf0_elems << RXFC_FS_OFF) |
+ RXFC_FWM_1 | (priv->mram_off + priv->rxf0_off));
+
+ m_can_write(priv, M_CAN_RXF1C, (priv->rxf1_elems << RXFC_FS_OFF) |
+ RXFC_FWM_1 | (priv->mram_off + priv->rxf1_off));
+
+ /* enable all interrupts */
+ m_can_write(priv, M_CAN_IR, IR_ALL_INT);
+ m_can_write(priv, M_CAN_IE, IR_ALL_INT);
+ m_can_write(priv, M_CAN_ILS, 0xFFFF0000);
+
+ /* set bittiming params */
+ m_can_set_bittiming(dev);
+
+ m_can_config_endisable(priv, false);
+}
+
+static void m_can_start(struct net_device *dev)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+
+ /* basic m_can configuration */
+ m_can_chip_config(dev);
+
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+ /* enable status change, error and module interrupts */
+ m_can_enable_all_interrupts(priv, true);
+}
+
+static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
+{
+ switch (mode) {
+ case CAN_MODE_START:
+ m_can_start(dev);
+ netif_wake_queue(dev);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int m_can_get_berr_counter(const struct net_device *dev,
+ struct can_berr_counter *bec)
+{
+ /* TODO */
+
+ return 0;
+}
+
+static void free_m_can_dev(struct net_device *dev)
+{
+ free_candev(dev);
+}
+
+static struct net_device *alloc_m_can_dev(void)
+{
+ struct net_device *dev;
+ struct m_can_priv *priv;
+
+ dev = alloc_candev(sizeof(struct m_can_priv), 1);
+ if (!dev)
+ return NULL;
+
+ priv = netdev_priv(dev);
+ netif_napi_add(dev, &priv->napi, m_can_poll, M_CAN_NAPI_WEIGHT);
+
+ priv->dev = dev;
+ priv->can.bittiming_const = &m_can_bittiming_const;
+ priv->can.do_set_mode = m_can_set_mode;
+ priv->can.do_get_berr_counter = m_can_get_berr_counter;
+ priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+ CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_BERR_REPORTING;
+
+ return dev;
+}
+
+static int m_can_open(struct net_device *dev)
+{
+ int err;
+ struct m_can_priv *priv = netdev_priv(dev);
+
+ clk_prepare_enable(priv->clk);
+
+ /* open the can device */
+ err = open_candev(dev);
+ if (err) {
+ netdev_err(dev, "failed to open can device\n");
+ goto exit_open_fail;
+ }
+
+ /* register interrupt handler */
+ err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
+ dev);
+ if (err < 0) {
+ netdev_err(dev, "failed to request interrupt\n");
+ goto exit_irq_fail;
+ }
+
+ /* start the m_can controller */
+ m_can_start(dev);
+
+ can_led_event(dev, CAN_LED_EVENT_OPEN);
+ napi_enable(&priv->napi);
+ netif_start_queue(dev);
+
+ return 0;
+
+exit_irq_fail:
+ close_candev(dev);
+exit_open_fail:
+ clk_disable_unprepare(priv->clk);
+ return err;
+}
+
+static void m_can_stop(struct net_device *dev)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+
+ /* disable all interrupts */
+ m_can_enable_all_interrupts(priv, false);
+
+ /* set the state as STOPPED */
+ priv->can.state = CAN_STATE_STOPPED;
+}
+
+static int m_can_close(struct net_device *dev)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+
+ netif_stop_queue(dev);
+ napi_disable(&priv->napi);
+ m_can_stop(dev);
+ free_irq(dev->irq, dev);
+ close_candev(dev);
+ can_led_event(dev, CAN_LED_EVENT_STOP);
+
+ return 0;
+}
+
+static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ struct m_can_priv *priv = netdev_priv(dev);
+ struct can_frame *frame = (struct can_frame *)skb->data;
+ u32 flags = 0, id;
+
+ if (can_dropped_invalid_skb(dev, skb))
+ return NETDEV_TX_OK;
+
+ netif_stop_queue(dev);
+
+ if (frame->can_id & CAN_RTR_FLAG)
+ flags |= TX_BUF_RTR;
+
+ if (frame->can_id & CAN_EFF_FLAG) {
+ id = frame->can_id & CAN_EFF_MASK;
+ flags |= TX_BUF_XTD;
+ } else {
+ id = ((frame->can_id & CAN_SFF_MASK) << 18);
+ }
+
+ /* message ram configuration */
+ writel(id | flags,
+ priv->mram_base + priv->mram_off + priv->txb_off);
+ writel(frame->can_dlc << 16,
+ priv->mram_base + priv->mram_off + priv->txb_off + 0x4);
+ writel(*(u32 *)(frame->data + 0),
+ priv->mram_base + priv->mram_off + priv->txb_off + 0x8);
+ writel(*(u32 *)(frame->data + 4),
+ priv->mram_base + priv->mram_off + priv->txb_off + 0xc);
+
+ can_put_echo_skb(skb, dev, 0);
+
+ /* enable first TX buffer to start transfer */
+ m_can_write(priv, M_CAN_TXBTIE, 0x00000001);
+ m_can_write(priv, M_CAN_TXBAR, 0x00000001);
+
+ return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops m_can_netdev_ops = {
+ .ndo_open = m_can_open,
+ .ndo_stop = m_can_close,
+ .ndo_start_xmit = m_can_start_xmit,
+};
+
+static int register_m_can_dev(struct net_device *dev)
+{
+ dev->flags |= IFF_ECHO; /* we support local echo */
+ dev->netdev_ops = &m_can_netdev_ops;
+
+ return register_candev(dev);
+}
+
+static const struct of_device_id m_can_of_table[] = {
+ { .compatible = "bosch,m_can", .data = NULL },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, m_can_of_table);
+
+static int m_can_of_parse_mram(struct platform_device *pdev,
+ struct m_can_priv *priv)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct resource *res;
+ void __iomem *addr;
+ u32 out_val[8];
+ int ret;
+
+ /* message ram could be shared */
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
+ if (!res)
+ return -ENODEV;
+
+ addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+ if (!addr)
+ return -ENODEV;
+
+ /* get message ram configuration */
+ ret = of_property_read_u32_array(np, "mram-cfg",
+ out_val, sizeof(out_val) / 4);
+ if (ret) {
+ dev_err(&pdev->dev, "can not get message ram configuration\n");
+ return -ENODEV;
+ }
+
+ priv->mram_base = addr;
+ priv->mram_off = out_val[0];
+ priv->sidf_elems = out_val[1];
+ priv->sidf_off = priv->mram_off;
+ priv->xidf_elems = out_val[2];
+ priv->xidf_off = priv->sidf_off + priv->sidf_elems * SIDF_ELEMENT_SIZE;
+ priv->rxf0_elems = out_val[3] & RXFC_FS_MASK;
+ priv->rxf0_off = priv->xidf_off + priv->xidf_elems * XIDF_ELEMENT_SIZE;
+ priv->rxf1_elems = out_val[4] & RXFC_FS_MASK;
+ priv->rxf1_off = priv->rxf0_off + priv->rxf0_elems * RXF0_ELEMENT_SIZE;
+ priv->rxb_elems = out_val[5];
+ priv->rxb_off = priv->rxf1_off + priv->rxf1_elems * RXF1_ELEMENT_SIZE;
+ priv->txe_elems = out_val[6];
+ priv->txe_off = priv->rxb_off + priv->rxb_elems * RXB_ELEMENT_SIZE;
+ priv->txb_elems = out_val[7] & TXBC_NDTB_MASK;
+ priv->txb_off = priv->txe_off + priv->txe_elems * TXE_ELEMENT_SIZE;
+
+ dev_dbg(&pdev->dev, "mram_base =%p mram_off =0x%x "
+ "sidf %d xidf %d rxf0 %d rxf1 %d rxb %d txe %d txb %d\n",
+ priv->mram_base, priv->mram_off, priv->sidf_elems,
+ priv->xidf_elems, priv->rxf0_elems, priv->rxf1_elems,
+ priv->rxb_elems, priv->txe_elems, priv->txb_elems);
+
+ return 0;
+}
+
+static int m_can_plat_probe(struct platform_device *pdev)
+{
+ struct net_device *dev;
+ struct m_can_priv *priv;
+ struct pinctrl *pinctrl;
+ struct resource *res;
+ void __iomem *addr;
+ struct clk *clk;
+ int irq, ret;
+
+ pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+ if (IS_ERR(pinctrl))
+ dev_warn(&pdev->dev,
+ "failed to configure pins from driver\n");
+
+ clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(clk)) {
+ dev_err(&pdev->dev, "no clock find\n");
+ return PTR_ERR(clk);
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ irq = platform_get_irq(pdev, 0);
+ if (IS_ERR(addr) || irq < 0)
+ return -EINVAL;
+
+ /* allocate the m_can device */
+ dev = alloc_m_can_dev();
+ if (!dev)
+ return -ENOMEM;
+
+ priv = netdev_priv(dev);
+ dev->irq = irq;
+ priv->base = addr;
+ priv->device = &pdev->dev;
+ priv->clk = clk;
+ priv->can.clock.freq = clk_get_rate(clk);
+
+ ret = m_can_of_parse_mram(pdev, priv);
+ if (ret)
+ goto failed_free_dev;
+
+ platform_set_drvdata(pdev, dev);
+ SET_NETDEV_DEV(dev, &pdev->dev);
+
+ ret = register_m_can_dev(dev);
+ if (ret) {
+ dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
+ KBUILD_MODNAME, ret);
+ goto failed_free_dev;
+ }
+
+ devm_can_led_init(dev);
+
+ dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
+ KBUILD_MODNAME, priv->base, dev->irq);
+
+ return 0;
+
+failed_free_dev:
+ free_m_can_dev(dev);
+ return ret;
+}
+
+#ifdef CONFIG_PM
+static int m_can_suspend(struct device *dev)
+{
+ struct net_device *ndev = dev_get_drvdata(dev);
+ struct m_can_priv *priv = netdev_priv(ndev);
+
+ if (netif_running(ndev)) {
+ netif_stop_queue(ndev);
+ netif_device_detach(ndev);
+ }
+
+ /* TODO: enter low power */
+
+ priv->can.state = CAN_STATE_SLEEPING;
+
+ return 0;
+}
+
+static int m_can_resume(struct device *dev)
+{
+ struct net_device *ndev = dev_get_drvdata(dev);
+ struct m_can_priv *priv = netdev_priv(ndev);
+
+ /* TODO: exit low power */
+
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+ if (netif_running(ndev)) {
+ netif_device_attach(ndev);
+ netif_start_queue(ndev);
+ }
+
+ return 0;
+}
+#endif
+
+static void unregister_m_can_dev(struct net_device *dev)
+{
+ unregister_candev(dev);
+}
+
+static int m_can_plat_remove(struct platform_device *pdev)
+{
+ struct net_device *dev = platform_get_drvdata(pdev);
+
+ unregister_m_can_dev(dev);
+ platform_set_drvdata(pdev, NULL);
+
+ free_m_can_dev(dev);
+
+ return 0;
+}
+
+static const struct dev_pm_ops m_can_pmops = {
+ SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
+};
+
+static struct platform_driver m_can_plat_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(m_can_of_table),
+ .pm = &m_can_pmops,
+ },
+ .probe = m_can_plat_probe,
+ .remove = m_can_plat_remove,
+};
+
+module_platform_driver(m_can_plat_driver);
+
+MODULE_AUTHOR("Dong Aisheng <b29396@freescale.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
--
1.7.8
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-27 10:00 ` Dong Aisheng
(?)
@ 2014-06-27 12:35 ` Mark Rutland
2014-06-30 8:03 ` Dong Aisheng
-1 siblings, 1 reply; 35+ messages in thread
From: Mark Rutland @ 2014-06-27 12:35 UTC (permalink / raw)
To: Dong Aisheng; +Cc: linux-can, netdev, wg, mkl, devicetree
On Fri, Jun 27, 2014 at 11:00:44AM +0100, Dong Aisheng wrote:
> The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller.
> For TX, only one dedicated tx buffer is used for sending data.
> For RX, RXFIFO 0 is used for receiving data to avoid overflow.
> Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
>
> Due to the message ram can be shared by multi m_can instances
> and the fifo element is configurable which is SoC dependant,
> the design is to parse the message ram related configuration data from device
> tree rather than hardcode define it in driver which can make the message
> ram sharing fully transparent to M_CAN controller driver,
> then we can gain better driver maintainability and future features upgrade.
>
> M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> and bitrate switch at runtime, however, this patch still does not add the
> support for these features.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
> .../devicetree/bindings/net/can/m_can.txt | 29 +
> drivers/net/can/Kconfig | 5 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/m_can.c | 867 ++++++++++++++++++++
> 4 files changed, 902 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> new file mode 100644
> index 0000000..2b22d02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -0,0 +1,29 @@
> +Bosch MCAN controller Device Tree Bindings
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : Should be "bosch,m_can" for M_CAN controllers
In general we use '-' rather than '_' in compatible strings...
> +- reg : physical base address and size of the M_CAN
> + registers map and message ram
> +- interrupts : property with a value describing the interrupt
> + number
We know this is a property, and we know it has a value.
The important detail is _which_ interrupt this represents. Does the
M_CAN block have a single output interrupt?
> +- clocks : clocks used by controller
How many?
Which clock inputs on the M_CAN to they feed into?
> +- mram-cfg : message ram configuration data, the format is
> + <offset sidf_elems xidf_elems rxf1_elems rxb_elems txe_elems txb_elems>
This is far too complicated for a single property.
> + The 'offset' is the address offset inside the message ram. This is usually set
> + if you're using the shared message ram while the other part is used by another
> + m_can controller.
It sounds like what we actually need to describe is the message RAM and
how M_CAN instances relate to it.
> + The left cell are all the number of each elements inside the message ram.
Pardon?
> + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> + for each elements definition.
This is far too complicated, and far too low-level. I want to see a
better description of what this is any why thie is necessary.
> +
> +Example:
> +canfd1: canfd@020e8000 {
> + compatible = "bosch,m_can";
> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> + reg-names = "canfd", "message_ram";
The use of reg-names was not described.
> + interrupts = <0 114 0x04>;
> + clocks = <&clks IMX6SX_CLK_CANFD>;
> + mram-cfg = <0x0 0 0 32 32 32 0 1>;
> + status = "disabled";
Any reason for a disabled status in the example?
[...]
> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> + priv->rxf0_off + fgi * 16 + 0x8);
> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> + priv->rxf0_off + fgi * 16 + 0xC);
This doesn't look endian-clean.
How are the registers laid out? Are they a string of bytes than can be
accessed in 4-byte chunks, or are they a string of 4-byte values?
[...]
> +static int m_can_get_berr_counter(const struct net_device *dev,
> + struct can_berr_counter *bec)
> +{
> + /* TODO */
> +
> + return 0;
Still to be done?
[...]
> +static int m_can_of_parse_mram(struct platform_device *pdev,
> + struct m_can_priv *priv)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + void __iomem *addr;
> + u32 out_val[8];
Why 8? Use a #define here...
> + int ret;
> +
> + /* message ram could be shared */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> + if (!res)
> + return -ENODEV;
> +
> + addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> + if (!addr)
> + return -ENODEV;
> +
> + /* get message ram configuration */
> + ret = of_property_read_u32_array(np, "mram-cfg",
> + out_val, sizeof(out_val) / 4);
...and use it here too.
That said, I want a better description of this property as I mentioned
previously.
[...]
> +static int m_can_plat_probe(struct platform_device *pdev)
> +{
> + struct net_device *dev;
> + struct m_can_priv *priv;
> + struct pinctrl *pinctrl;
> + struct resource *res;
> + void __iomem *addr;
> + struct clk *clk;
> + int irq, ret;
> +
> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> + if (IS_ERR(pinctrl))
> + dev_warn(&pdev->dev,
> + "failed to configure pins from driver\n");
Doesn't this need pinctrl properties in the DT?
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "no clock find\n");
> + return PTR_ERR(clk);
> + }
So just the one clock?
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
This wasn't in the binding.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-27 12:35 ` Mark Rutland
@ 2014-06-30 8:03 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-06-30 8:03 UTC (permalink / raw)
To: Mark Rutland; +Cc: linux-can, netdev, wg, mkl, devicetree
Hi Mark,
First of all, thanks for the quick and carefully review.
On Fri, Jun 27, 2014 at 01:35:45PM +0100, Mark Rutland wrote:
> On Fri, Jun 27, 2014 at 11:00:44AM +0100, Dong Aisheng wrote:
> > The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller.
> > For TX, only one dedicated tx buffer is used for sending data.
> > For RX, RXFIFO 0 is used for receiving data to avoid overflow.
> > Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
> >
> > Due to the message ram can be shared by multi m_can instances
> > and the fifo element is configurable which is SoC dependant,
> > the design is to parse the message ram related configuration data from device
> > tree rather than hardcode define it in driver which can make the message
> > ram sharing fully transparent to M_CAN controller driver,
> > then we can gain better driver maintainability and future features upgrade.
> >
> > M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> > and bitrate switch at runtime, however, this patch still does not add the
> > support for these features.
> >
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> > .../devicetree/bindings/net/can/m_can.txt | 29 +
> > drivers/net/can/Kconfig | 5 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/m_can.c | 867 ++++++++++++++++++++
> > 4 files changed, 902 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> > new file mode 100644
> > index 0000000..2b22d02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> > @@ -0,0 +1,29 @@
> > +Bosch MCAN controller Device Tree Bindings
> > +-------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Should be "bosch,m_can" for M_CAN controllers
>
> In general we use '-' rather than '_' in compatible strings...
>
First i'm following the exist c_can.txt rule.
Second M_CAN is the M_CAN user manual defined name, i'm not sure change
to bosch,m-can is a good idea.
> > +- reg : physical base address and size of the M_CAN
> > + registers map and message ram
> > +- interrupts : property with a value describing the interrupt
> > + number
>
> We know this is a property, and we know it has a value.
>
> The important detail is _which_ interrupt this represents. Does the
> M_CAN block have a single output interrupt?
>
The m_can module provides two interrupt lines. Interrupts can be routed
either to m_can_int0 or to m_can_int1. By default all interrupts are
routed to interrupt line m_can_int0. By programming ILE.EINT0 and ILE.EINT1
the interrupt lines can be enabled or disabled separately.
However, whether these two interrupts line are connected to two separate
interrupts to interrupt controller depends on SoC design.
For iMX6SX it has a single interrupt number of canfd1(m_can) which is 146,
since both m_can_int0 and m_can_int1 are connected to interrupt 146.
For other SoCs, it may have two interrupt numbers for one m_can instance.
Thus this property could be one value or two.
The current driver only parsed one interrupt line.
Probably we could add description as follows:
- interrupts : Should be the interrupt number shared by m_can_int0
and m_can_int1. If two numbers, the first one is
m_can_int0 and the second one is m_can_int1.
For driver, we still only parse m_can_int0 and route all interrupts
to m_can_int0 to make things a bit simple.
> > +- clocks : clocks used by controller
>
> How many?
>
> Which clock inputs on the M_CAN to they feed into?
>
Yes, M_CAN could be Dual Clock Sources, host clock m_can_hclk and
can clock m_can_cclk. I will add them here.
> > +- mram-cfg : message ram configuration data, the format is
> > + <offset sidf_elems xidf_elems rxf1_elems rxb_elems txe_elems txb_elems>
>
> This is far too complicated for a single property.
>
> > + The 'offset' is the address offset inside the message ram. This is usually set
> > + if you're using the shared message ram while the other part is used by another
> > + m_can controller.
>
> It sounds like what we actually need to describe is the message RAM and
> how M_CAN instances relate to it.
>
> > + The left cell are all the number of each elements inside the message ram.
>
> Pardon?
>
> > + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> > + for each elements definition.
>
> This is far too complicated, and far too low-level. I want to see a
> better description of what this is any why thie is necessary.
>
Yes, this is a bit complicated.
The main reason for adding this property is that message ram can be shared
by multiple m_can instances according to m_can user manual.
For a SOC design, each m_can instance may use the shared message ram
or has its own private message ram.
e.g. For iMX6SX, there's 16K message ram shared by m_can0 and m_can1.
If sharing, we don't know how many instances share it and how the message ram
are shared using by multi m_can.
It's not good to hardcode these information in driver.
Thus i introduced a property called mram-cfg to let driver to parse this
information from device tree. Then controller driver could be fully transparently
to the ram sharing mechanism.
For iMX6SX, we assigned the first 8K for m_can0 and the left 8K for m_can1 to use.
This is done by offset value in mram-cfg property.
Due to each fifo/buffer elements in message ram can also be configurable, the driver
also needs this information calculate the address to know where to put/read
the correct data, e.g. rx data or tx data and etc.
So we need extra cells to let user to define the elements number of each fifo/buffers.
According to user manual, the elements include:
11-bit Filter 0-128 elements / 0-128 words
29-bit Filter 0-64 elements / 0-128 words
Rx FIFO 0 0-64 elements / 0-1152 words
Rx FIFO 1 0-64 elements / 0-1152 words
Rx Buffers 0-64 elements / 0-1152 words
Tx Event FIFO 0-32 elements / 0-64 words
Tx Buffers 0-32 elements / 0-576 words
Maybe i could add more information in the binding doc to describe it to make it
more easy to understand.
> > +
> > +Example:
> > +canfd1: canfd@020e8000 {
> > + compatible = "bosch,m_can";
> > + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> > + reg-names = "canfd", "message_ram";
>
> The use of reg-names was not described.
>
Yes, will add the description in 'Required properties'.
Driver can find the correct resource by the name.
> > + interrupts = <0 114 0x04>;
> > + clocks = <&clks IMX6SX_CLK_CANFD>;
> > + mram-cfg = <0x0 0 0 32 32 32 0 1>;
> > + status = "disabled";
>
> Any reason for a disabled status in the example?
>
No any special reason, just because the example given is usually in SoC dtsi
and disabled by default then it is enabled it board dts later.
Is it a problem?
I could remove it if you really want.
> [...]
>
> > + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0x8);
> > + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0xC);
>
> This doesn't look endian-clean.
>
Yes, i guess the m_can could also be used in big endian system.
Due to i only have littel endian chip on hands, i just used little endian
in the initial driver.
Big endian could be added later if if there's a chip to verify.
> How are the registers laid out? Are they a string of bytes than can be
> accessed in 4-byte chunks, or are they a string of 4-byte values?
>
The register is laid out as follows and can be accessed in 4-byte chunks.
R2 DB3[7:0] DB2[7:0] DB1[7:0] DB0[7:0]
R3 DB7[7:0] DB6[7:0] DB5[7:0] DB4[7:0]
...
Rn DBm[7:0] DBm-1[7:0] DBm-2[7:0] DBm-3[7:0]
> [...]
>
> > +static int m_can_get_berr_counter(const struct net_device *dev,
> > + struct can_berr_counter *bec)
> > +{
> > + /* TODO */
> > +
> > + return 0;
>
> Still to be done?
>
Yes, most error handling code are implemented in second patch.
> [...]
>
> > +static int m_can_of_parse_mram(struct platform_device *pdev,
> > + struct m_can_priv *priv)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + void __iomem *addr;
> > + u32 out_val[8];
>
> Why 8? Use a #define here...
>
It's according to mram-cfg cells number.
Seems change to #define is better, will do it.
> > + int ret;
> > +
> > + /* message ram could be shared */
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> > + if (!res)
> > + return -ENODEV;
> > +
> > + addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > + if (!addr)
> > + return -ENODEV;
> > +
> > + /* get message ram configuration */
> > + ret = of_property_read_u32_array(np, "mram-cfg",
> > + out_val, sizeof(out_val) / 4);
>
> ...and use it here too.
>
> That said, I want a better description of this property as I mentioned
> previously.
>
> [...]
>
> > +static int m_can_plat_probe(struct platform_device *pdev)
> > +{
> > + struct net_device *dev;
> > + struct m_can_priv *priv;
> > + struct pinctrl *pinctrl;
> > + struct resource *res;
> > + void __iomem *addr;
> > + struct clk *clk;
> > + int irq, ret;
> > +
> > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > + if (IS_ERR(pinctrl))
> > + dev_warn(&pdev->dev,
> > + "failed to configure pins from driver\n");
>
> Doesn't this need pinctrl properties in the DT?
I will remove it to let pinctrl code in driver core to handle it.
>
> > + clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(clk)) {
> > + dev_err(&pdev->dev, "no clock find\n");
> > + return PTR_ERR(clk);
> > + }
>
> So just the one clock?
>
For iMX6SX, it is only one clock.
But M_CAN user manual defines two clocks.
I could add them here.
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
>
> This wasn't in the binding.
Yes, will add it in the binding description.
>
> Thanks,
> Mark.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-27 10:00 ` Dong Aisheng
(?)
(?)
@ 2014-06-27 18:03 ` Oliver Hartkopp
2014-06-30 8:26 ` Dong Aisheng
-1 siblings, 1 reply; 35+ messages in thread
From: Oliver Hartkopp @ 2014-06-27 18:03 UTC (permalink / raw)
To: Dong Aisheng, linux-can; +Cc: netdev, wg, mkl, devicetree
Hello Dong,
some general remarks from my side ...
On 27.06.2014 12:00, Dong Aisheng wrote:
>
> M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> and bitrate switch at runtime, however, this patch still does not add the
> support for these features.
What is the reason for not supporting CAN FD?
The infrastructure is ready for it since Linux 3.15.
http://www.can-newsletter.org/engineering/standardization/140513_can-fd-linux-tools-and-driver-infrastructure_peak_vw/
For details see my commits for Linux 3.15.
> + The left cell are all the number of each elements inside the message ram.
> + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
^^^
typo.
> + for each elements definition.
> +
> +Example:
> +canfd1: canfd@020e8000 {
^^^^^^ ^^^^^
There's no reason to name this canfd. The fact that the controller supports
CAN FD is provided by priv->ctrlmode_supported and the CAN_CTRLMODE_FD bit.
Just write
can1: can@020e8000 {
> + compatible = "bosch,m_can";
> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> + reg-names = "canfd", "message_ram";
^^^^^
dito.
> + interrupts = <0 114 0x04>;
> + clocks = <&clks IMX6SX_CLK_CANFD>;
^^^^^
dito.
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -137,6 +137,11 @@ config CAN_XILINXCAN
> Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> Zynq CANPS IP.
>
> +config CAN_M_CAN
> + tristate "Bosch M_CAN devices"
> + ---help---
> + Say Y here if you want to support for Bosch M_CAN controller.
> +
source "drivers/net/can/m_can/Kconfig"
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 1697f22..69dee2c 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -17,6 +17,7 @@ obj-y += softing/
> obj-$(CONFIG_CAN_SJA1000) += sja1000/
> obj-$(CONFIG_CAN_MSCAN) += mscan/
> obj-$(CONFIG_CAN_C_CAN) += c_can/
> +obj-$(CONFIG_CAN_M_CAN) += m_can.o
Please create a new m_can directory and a Kconfig in this directory analogue
to the c_can IP core approach.
> obj-$(CONFIG_CAN_CC770) += cc770/
> obj-$(CONFIG_CAN_AT91) += at91_can.o
> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
Thanks for your contribution,
Oliver
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-27 18:03 ` Oliver Hartkopp
@ 2014-06-30 8:26 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-06-30 8:26 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, netdev, wg, mkl, devicetree
On Fri, Jun 27, 2014 at 08:03:20PM +0200, Oliver Hartkopp wrote:
> Hello Dong,
>
> some general remarks from my side ...
>
> On 27.06.2014 12:00, Dong Aisheng wrote:
> >
> > M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> > and bitrate switch at runtime, however, this patch still does not add the
> > support for these features.
>
> What is the reason for not supporting CAN FD?
> The infrastructure is ready for it since Linux 3.15.
>
> http://www.can-newsletter.org/engineering/standardization/140513_can-fd-linux-tools-and-driver-infrastructure_peak_vw/
>
> For details see my commits for Linux 3.15.
>
Thanks for the information.
Of course i will add CAN FD support.
Mainly because the driver is just newly written these days, CAN FD feature
is still under development. :-)
> > + The left cell are all the number of each elements inside the message ram.
> > + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> ^^^
> typo.
>
Got it, thanks.
> > + for each elements definition.
> > +
> > +Example:
> > +canfd1: canfd@020e8000 {
> ^^^^^^ ^^^^^
>
> There's no reason to name this canfd. The fact that the controller supports
> CAN FD is provided by priv->ctrlmode_supported and the CAN_CTRLMODE_FD bit.
>
Just because mx6sx spec calling it CANFD at many places.
e.g.
Interrupts:
146 CANFD1 CANFD1 interrupt request
147 CANFD2 CANFD2 interrupt request
Memory Map:
020F_0000 020F_3FFF CANFD2 16 KB
020E_8000 020E_BFFF CANFD1 16 KB
So i used canfd firstly.
CCM:
CANFD
ips_clk can_clk_root CCGR1[CG15] (canfd_clk_enable)
m_can_0_cclk can_clk_root CCGR1[CG15] (canfd_clk_enable)
m_can_0_ips_clk can_clk_root CCGR1[CG15] (canfd_clk_enable)
> Just write
>
> can1: can@020e8000 {
>
I'm ok with this style.
Maybe the following is better:
m_can1: can@020e8000 {
> > + compatible = "bosch,m_can";
> > + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> > + reg-names = "canfd", "message_ram";
> ^^^^^
> dito.
>
How about m_can?
Since it's IP driver, not depends on how SoC naming it.
> > + interrupts = <0 114 0x04>;
> > + clocks = <&clks IMX6SX_CLK_CANFD>;
> ^^^^^
> dito.
>
Not for this one, because imx6sx spec calling it CANFD in Clock
chaptor.
We want to align with our spec since it's arch code.
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -137,6 +137,11 @@ config CAN_XILINXCAN
> > Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> > Zynq CANPS IP.
> >
> > +config CAN_M_CAN
> > + tristate "Bosch M_CAN devices"
> > + ---help---
> > + Say Y here if you want to support for Bosch M_CAN controller.
> > +
>
> source "drivers/net/can/m_can/Kconfig"
>
> > source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index 1697f22..69dee2c 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -17,6 +17,7 @@ obj-y += softing/
> > obj-$(CONFIG_CAN_SJA1000) += sja1000/
> > obj-$(CONFIG_CAN_MSCAN) += mscan/
> > obj-$(CONFIG_CAN_C_CAN) += c_can/
> > +obj-$(CONFIG_CAN_M_CAN) += m_can.o
>
> Please create a new m_can directory and a Kconfig in this directory analogue
> to the c_can IP core approach.
>
Got it, thanks.
> > obj-$(CONFIG_CAN_CC770) += cc770/
> > obj-$(CONFIG_CAN_AT91) += at91_can.o
> > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
>
> Thanks for your contribution,
> Oliver
>
Thanks for the review.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
@ 2014-06-30 8:26 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-06-30 8:26 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, netdev, wg, mkl, devicetree
On Fri, Jun 27, 2014 at 08:03:20PM +0200, Oliver Hartkopp wrote:
> Hello Dong,
>
> some general remarks from my side ...
>
> On 27.06.2014 12:00, Dong Aisheng wrote:
> >
> > M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> > and bitrate switch at runtime, however, this patch still does not add the
> > support for these features.
>
> What is the reason for not supporting CAN FD?
> The infrastructure is ready for it since Linux 3.15.
>
> http://www.can-newsletter.org/engineering/standardization/140513_can-fd-linux-tools-and-driver-infrastructure_peak_vw/
>
> For details see my commits for Linux 3.15.
>
Thanks for the information.
Of course i will add CAN FD support.
Mainly because the driver is just newly written these days, CAN FD feature
is still under development. :-)
> > + The left cell are all the number of each elements inside the message ram.
> > + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> ^^^
> typo.
>
Got it, thanks.
> > + for each elements definition.
> > +
> > +Example:
> > +canfd1: canfd@020e8000 {
> ^^^^^^ ^^^^^
>
> There's no reason to name this canfd. The fact that the controller supports
> CAN FD is provided by priv->ctrlmode_supported and the CAN_CTRLMODE_FD bit.
>
Just because mx6sx spec calling it CANFD at many places.
e.g.
Interrupts:
146 CANFD1 CANFD1 interrupt request
147 CANFD2 CANFD2 interrupt request
Memory Map:
020F_0000 020F_3FFF CANFD2 16 KB
020E_8000 020E_BFFF CANFD1 16 KB
So i used canfd firstly.
CCM:
CANFD
ips_clk can_clk_root CCGR1[CG15] (canfd_clk_enable)
m_can_0_cclk can_clk_root CCGR1[CG15] (canfd_clk_enable)
m_can_0_ips_clk can_clk_root CCGR1[CG15] (canfd_clk_enable)
> Just write
>
> can1: can@020e8000 {
>
I'm ok with this style.
Maybe the following is better:
m_can1: can@020e8000 {
> > + compatible = "bosch,m_can";
> > + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> > + reg-names = "canfd", "message_ram";
> ^^^^^
> dito.
>
How about m_can?
Since it's IP driver, not depends on how SoC naming it.
> > + interrupts = <0 114 0x04>;
> > + clocks = <&clks IMX6SX_CLK_CANFD>;
> ^^^^^
> dito.
>
Not for this one, because imx6sx spec calling it CANFD in Clock
chaptor.
We want to align with our spec since it's arch code.
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -137,6 +137,11 @@ config CAN_XILINXCAN
> > Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> > Zynq CANPS IP.
> >
> > +config CAN_M_CAN
> > + tristate "Bosch M_CAN devices"
> > + ---help---
> > + Say Y here if you want to support for Bosch M_CAN controller.
> > +
>
> source "drivers/net/can/m_can/Kconfig"
>
> > source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index 1697f22..69dee2c 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -17,6 +17,7 @@ obj-y += softing/
> > obj-$(CONFIG_CAN_SJA1000) += sja1000/
> > obj-$(CONFIG_CAN_MSCAN) += mscan/
> > obj-$(CONFIG_CAN_C_CAN) += c_can/
> > +obj-$(CONFIG_CAN_M_CAN) += m_can.o
>
> Please create a new m_can directory and a Kconfig in this directory analogue
> to the c_can IP core approach.
>
Got it, thanks.
> > obj-$(CONFIG_CAN_CC770) += cc770/
> > obj-$(CONFIG_CAN_AT91) += at91_can.o
> > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
>
> Thanks for your contribution,
> Oliver
>
Thanks for the review.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-30 8:26 ` Dong Aisheng
(?)
@ 2014-07-02 17:54 ` Oliver Hartkopp
2014-07-02 19:13 ` Marc Kleine-Budde
-1 siblings, 1 reply; 35+ messages in thread
From: Oliver Hartkopp @ 2014-07-02 17:54 UTC (permalink / raw)
To: mkl; +Cc: Dong Aisheng, linux-can, wg, devicetree
Hello Marc,
I'm not really familiar with the naming concept in device trees.
What is your opinion about the remarks below?
Regards,
Oliver
On 30.06.2014 10:26, Dong Aisheng wrote:
> On Fri, Jun 27, 2014 at 08:03:20PM +0200, Oliver Hartkopp wrote:
>>> + for each elements definition.
>>> +
>>> +Example:
>>> +canfd1: canfd@020e8000 {
>> ^^^^^^ ^^^^^
>>
>> There's no reason to name this canfd. The fact that the controller supports
>> CAN FD is provided by priv->ctrlmode_supported and the CAN_CTRLMODE_FD bit.
>>
>
> Just because mx6sx spec calling it CANFD at many places.
> e.g.
> Interrupts:
> 146 CANFD1 CANFD1 interrupt request
> 147 CANFD2 CANFD2 interrupt request
> Memory Map:
> 020F_0000 020F_3FFF CANFD2 16 KB
> 020E_8000 020E_BFFF CANFD1 16 KB
> So i used canfd firstly.
> CCM:
> CANFD
> ips_clk can_clk_root CCGR1[CG15] (canfd_clk_enable)
> m_can_0_cclk can_clk_root CCGR1[CG15] (canfd_clk_enable)
> m_can_0_ips_clk can_clk_root CCGR1[CG15] (canfd_clk_enable)
>
>> Just write
>>
>> can1: can@020e8000 {
>>
>
> I'm ok with this style.
> Maybe the following is better:
> m_can1: can@020e8000 {
>
>>> + compatible = "bosch,m_can";
>>> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
>>> + reg-names = "canfd", "message_ram";
>> ^^^^^
>> dito.
>>
> How about m_can?
> Since it's IP driver, not depends on how SoC naming it.
>
>>> + interrupts = <0 114 0x04>;
>>> + clocks = <&clks IMX6SX_CLK_CANFD>;
>> ^^^^^
>> dito.
>>
>
> Not for this one, because imx6sx spec calling it CANFD in Clock
> chaptor.
> We want to align with our spec since it's arch code.
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-02 17:54 ` Oliver Hartkopp
@ 2014-07-02 19:13 ` Marc Kleine-Budde
2014-07-03 3:48 ` Dong Aisheng
0 siblings, 1 reply; 35+ messages in thread
From: Marc Kleine-Budde @ 2014-07-02 19:13 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Dong Aisheng, linux-can, wg, devicetree
[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]
On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
> I'm not really familiar with the naming concept in device trees.
>
> What is your opinion about the remarks below?
The entries in the DT, at least on freescale baords, follow the naming
scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
can1: can@43f88000 { ... }
can2: can@43f8c000 { ... }
And on the mx28, its:
can0: can@80032000 { ... }
can1: can@80034000 { ... }
Because the imx25 datasheet uses a "1" based counting scheme, while the
imx28 uses a "0" based one.
So it's best practise to follow the naming and numbering scheme of the
hardware reference manual.....and if you have access to the
documentation of the m_can core, use clock names of the m_can core for
the clock-names property.
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: 242 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-02 19:13 ` Marc Kleine-Budde
@ 2014-07-03 3:48 ` Dong Aisheng
2014-07-03 7:12 ` Marc Kleine-Budde
0 siblings, 1 reply; 35+ messages in thread
From: Dong Aisheng @ 2014-07-03 3:48 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Oliver Hartkopp, linux-can, wg, devicetree
On Wed, Jul 02, 2014 at 09:13:07PM +0200, Marc Kleine-Budde wrote:
> On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
> > I'm not really familiar with the naming concept in device trees.
> >
> > What is your opinion about the remarks below?
>
> The entries in the DT, at least on freescale baords, follow the naming
> scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
>
> can1: can@43f88000 { ... }
> can2: can@43f8c000 { ... }
>
> And on the mx28, its:
>
> can0: can@80032000 { ... }
> can1: can@80034000 { ... }
>
> Because the imx25 datasheet uses a "1" based counting scheme, while the
> imx28 uses a "0" based one.
>
> So it's best practise to follow the naming and numbering scheme of the
> hardware reference manual.....and if you have access to the
> documentation of the m_can core, use clock names of the m_can core for
> the clock-names property.
>
Based on my knowledge, device tree allows define phandle name according to
the real device name of HW according spec while the device node name should
be general(e.g can@80032000 rather than flexcan@80032000).
For imx6sx, there are already following entries in
arch/arm/boot/dts/imx6sx.dtsi
flexcan1: can@02090000 {...}
flexcan2: can@02094000 {...}
So i'd prefer to define as:
m_can1: canfd@020e8000 {...}
m_can2: canfd@020f0000 {...}
One problem is there're can alias already.
aliases {
can0 = &flexcan1;
can1 = &flexcan2;
...
}
I'm not sure adding can2&can3 for mcan is properly:
aliases {
can0 = &flexcan1;
can1 = &flexcan2;
can2 = &m_can1;
can3 = &m_can2;
...
}
Since the m_can driver does not need to use aliases,
so i will not add them.
Regards
Dong Aisheng
> Marc
> --
> Pengutronix e.K. | Marc Klein-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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-03 3:48 ` Dong Aisheng
@ 2014-07-03 7:12 ` Marc Kleine-Budde
2014-07-03 8:48 ` Dong Aisheng
0 siblings, 1 reply; 35+ messages in thread
From: Marc Kleine-Budde @ 2014-07-03 7:12 UTC (permalink / raw)
To: Dong Aisheng; +Cc: Oliver Hartkopp, linux-can, wg, devicetree
[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]
On 07/03/2014 05:48 AM, Dong Aisheng wrote:
> On Wed, Jul 02, 2014 at 09:13:07PM +0200, Marc Kleine-Budde wrote:
>> On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
>>> I'm not really familiar with the naming concept in device trees.
>>>
>>> What is your opinion about the remarks below?
>>
>> The entries in the DT, at least on freescale baords, follow the naming
>> scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
>>
>> can1: can@43f88000 { ... }
>> can2: can@43f8c000 { ... }
>>
>> And on the mx28, its:
>>
>> can0: can@80032000 { ... }
>> can1: can@80034000 { ... }
>>
>> Because the imx25 datasheet uses a "1" based counting scheme, while the
>> imx28 uses a "0" based one.
>>
>> So it's best practise to follow the naming and numbering scheme of the
>> hardware reference manual.....and if you have access to the
>> documentation of the m_can core, use clock names of the m_can core for
>> the clock-names property.
>>
>
> Based on my knowledge, device tree allows define phandle name according to
> the real device name of HW according spec while the device node name should
> be general(e.g can@80032000 rather than flexcan@80032000).
> For imx6sx, there are already following entries in
> arch/arm/boot/dts/imx6sx.dtsi
> flexcan1: can@02090000 {...}
> flexcan2: can@02094000 {...}
> So i'd prefer to define as:
> m_can1: canfd@020e8000 {...}
> m_can2: canfd@020f0000 {...}
>
>
> One problem is there're can alias already.
> aliases {
> can0 = &flexcan1;
> can1 = &flexcan2;
> ...
> }
> I'm not sure adding can2&can3 for mcan is properly:
> aliases {
> can0 = &flexcan1;
> can1 = &flexcan2;
> can2 = &m_can1;
> can3 = &m_can2;
> ...
> }
> Since the m_can driver does not need to use aliases,
> so i will not add them.
IMHO It's fine too add the can{2,3} aliases to m_can, too.
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: 242 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-03 7:12 ` Marc Kleine-Budde
@ 2014-07-03 8:48 ` Dong Aisheng
[not found] ` <20140703084803.GA11012-KgLukfWpBlCctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
0 siblings, 1 reply; 35+ messages in thread
From: Dong Aisheng @ 2014-07-03 8:48 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Oliver Hartkopp, linux-can, wg, devicetree
On Thu, Jul 03, 2014 at 09:12:49AM +0200, Marc Kleine-Budde wrote:
> On 07/03/2014 05:48 AM, Dong Aisheng wrote:
> > On Wed, Jul 02, 2014 at 09:13:07PM +0200, Marc Kleine-Budde wrote:
> >> On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
> >>> I'm not really familiar with the naming concept in device trees.
> >>>
> >>> What is your opinion about the remarks below?
> >>
> >> The entries in the DT, at least on freescale baords, follow the naming
> >> scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
> >>
> >> can1: can@43f88000 { ... }
> >> can2: can@43f8c000 { ... }
> >>
> >> And on the mx28, its:
> >>
> >> can0: can@80032000 { ... }
> >> can1: can@80034000 { ... }
> >>
> >> Because the imx25 datasheet uses a "1" based counting scheme, while the
> >> imx28 uses a "0" based one.
> >>
> >> So it's best practise to follow the naming and numbering scheme of the
> >> hardware reference manual.....and if you have access to the
> >> documentation of the m_can core, use clock names of the m_can core for
> >> the clock-names property.
> >>
> >
> > Based on my knowledge, device tree allows define phandle name according to
> > the real device name of HW according spec while the device node name should
> > be general(e.g can@80032000 rather than flexcan@80032000).
> > For imx6sx, there are already following entries in
> > arch/arm/boot/dts/imx6sx.dtsi
> > flexcan1: can@02090000 {...}
> > flexcan2: can@02094000 {...}
> > So i'd prefer to define as:
> > m_can1: canfd@020e8000 {...}
> > m_can2: canfd@020f0000 {...}
> >
> >
> > One problem is there're can alias already.
> > aliases {
> > can0 = &flexcan1;
> > can1 = &flexcan2;
> > ...
> > }
> > I'm not sure adding can2&can3 for mcan is properly:
> > aliases {
> > can0 = &flexcan1;
> > can1 = &flexcan2;
> > can2 = &m_can1;
> > can3 = &m_can2;
> > ...
> > }
> > Since the m_can driver does not need to use aliases,
> > so i will not add them.
>
> IMHO It's fine too add the can{2,3} aliases to m_can, too.
>
I think the main problem for doing this way is that the meaning of id
return by of_alias_get_id may be not persistent.
e.g
For MX6SX
aliases {
can0 = &flexcan1;
can1 = &flexcan2;
can2 = &m_can1;
can3 = &m_can2;
...
}
For other platform, it could be:
aliases {
can0 = &m_can1;
can1 = &m_can2;
...
}
It's hard for driver to use.
And actually the M_CAN driver does not need to use the alias.
So i wonder if it makes sense to add the alias for m_can devices
like that.
Regards
Dong Aisheng
> 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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-27 10:00 ` Dong Aisheng
` (2 preceding siblings ...)
(?)
@ 2014-07-01 10:29 ` Marc Kleine-Budde
2014-07-02 6:20 ` Dong Aisheng
-1 siblings, 1 reply; 35+ messages in thread
From: Marc Kleine-Budde @ 2014-07-01 10:29 UTC (permalink / raw)
To: Dong Aisheng, linux-can; +Cc: netdev, wg, devicetree
[-- Attachment #1: Type: text/plain, Size: 30388 bytes --]
On 06/27/2014 12:00 PM, Dong Aisheng wrote:
> The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller.
> For TX, only one dedicated tx buffer is used for sending data.
> For RX, RXFIFO 0 is used for receiving data to avoid overflow.
> Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
>
> Due to the message ram can be shared by multi m_can instances
> and the fifo element is configurable which is SoC dependant,
> the design is to parse the message ram related configuration data from device
> tree rather than hardcode define it in driver which can make the message
> ram sharing fully transparent to M_CAN controller driver,
> then we can gain better driver maintainability and future features upgrade.
>
> M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> and bitrate switch at runtime, however, this patch still does not add the
> support for these features.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
> .../devicetree/bindings/net/can/m_can.txt | 29 +
> drivers/net/can/Kconfig | 5 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/m_can.c | 867 ++++++++++++++++++++
> 4 files changed, 902 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> new file mode 100644
> index 0000000..2b22d02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -0,0 +1,29 @@
> +Bosch MCAN controller Device Tree Bindings
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : Should be "bosch,m_can" for M_CAN controllers
> +- reg : physical base address and size of the M_CAN
> + registers map and message ram
> +- interrupts : property with a value describing the interrupt
> + number
> +- clocks : clocks used by controller
> +- mram-cfg : message ram configuration data, the format is
> + <offset sidf_elems xidf_elems rxf1_elems rxb_elems txe_elems txb_elems>
> + The 'offset' is the address offset inside the message ram. This is usually set
> + if you're using the shared message ram while the other part is used by another
> + m_can controller.
> + The left cell are all the number of each elements inside the message ram.
> + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> + for each elements definition.
> +
> +Example:
> +canfd1: canfd@020e8000 {
> + compatible = "bosch,m_can";
> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> + reg-names = "canfd", "message_ram";
> + interrupts = <0 114 0x04>;
> + clocks = <&clks IMX6SX_CLK_CANFD>;
> + mram-cfg = <0x0 0 0 32 32 32 0 1>;
> + status = "disabled";
> +};
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 4168822..3fd1c4a 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -137,6 +137,11 @@ config CAN_XILINXCAN
> Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> Zynq CANPS IP.
>
> +config CAN_M_CAN
> + tristate "Bosch M_CAN devices"
> + ---help---
> + Say Y here if you want to support for Bosch M_CAN controller.
> +
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 1697f22..69dee2c 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -17,6 +17,7 @@ obj-y += softing/
> obj-$(CONFIG_CAN_SJA1000) += sja1000/
> obj-$(CONFIG_CAN_MSCAN) += mscan/
> obj-$(CONFIG_CAN_C_CAN) += c_can/
> +obj-$(CONFIG_CAN_M_CAN) += m_can.o
> obj-$(CONFIG_CAN_CC770) += cc770/
> obj-$(CONFIG_CAN_AT91) += at91_can.o
> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> diff --git a/drivers/net/can/m_can.c b/drivers/net/can/m_can.c
> new file mode 100644
> index 0000000..61e9a8e
> --- /dev/null
> +++ b/drivers/net/can/m_can.c
> @@ -0,0 +1,867 @@
> +/*
> + * CAN bus driver for Bosch M_CAN controller
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Dong Aisheng <b29396@freescale.com>
> + *
> + * Bosch M_CAN user manual can be obtained from:
> + * http://www.bosch-semiconductors.de/media/pdf_1/ipmodules_1/m_can/
> + * mcan_users_manual_v302.pdf
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/if_arp.h>
> +#include <linux/if_ether.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/can/dev.h>
> +
> +/* napi related */
> +#define M_CAN_NAPI_WEIGHT 64
> +
> +/* registers definition */
> +enum m_can_reg {
> + M_CAN_CREL = 0x0,
> + M_CAN_ENDN = 0x4,
> + M_CAN_CUST = 0x8,
> + M_CAN_FBTP = 0xc,
> + M_CAN_TEST = 0x10,
> + M_CAN_RWD = 0x14,
> + M_CAN_CCCR = 0x18,
> + M_CAN_BTP = 0x1c,
> + M_CAN_TSCC = 0x20,
> + M_CAN_TSCV = 0x24,
> + M_CAN_TOCC = 0x28,
> + M_CAN_TOCV = 0x2c,
> + M_CAN_ECR = 0x40,
> + M_CAN_PSR = 0x44,
> + M_CAN_IR = 0x50,
> + M_CAN_IE = 0x54,
> + M_CAN_ILS = 0x58,
> + M_CAN_ILE = 0x5c,
> + M_CAN_GFC = 0x80,
> + M_CAN_SIDFC = 0x84,
> + M_CAN_XIDFC = 0x88,
> + M_CAN_XIDAM = 0x90,
> + M_CAN_HPMS = 0x94,
> + M_CAN_NDAT1 = 0x98,
> + M_CAN_NDAT2 = 0x9c,
> + M_CAN_RXF0C = 0xa0,
> + M_CAN_RXF0S = 0xa4,
> + M_CAN_RXF0A = 0xa8,
> + M_CAN_RXBC = 0xac,
> + M_CAN_RXF1C = 0xb0,
> + M_CAN_RXF1S = 0xb4,
> + M_CAN_RXF1A = 0xb8,
> + M_CAN_RXESC = 0xbc,
> + M_CAN_TXBC = 0xc0,
> + M_CAN_TXFQS = 0xc4,
> + M_CAN_TXESC = 0xc8,
> + M_CAN_TXBRP = 0xcc,
> + M_CAN_TXBAR = 0xd0,
> + M_CAN_TXBCR = 0xd4,
> + M_CAN_TXBTO = 0xd8,
> + M_CAN_TXBCF = 0xdc,
> + M_CAN_TXBTIE = 0xe0,
> + M_CAN_TXBCIE = 0xe4,
> + M_CAN_TXEFC = 0xf0,
> + M_CAN_TXEFS = 0xf4,
> + M_CAN_TXEFA = 0xf8,
> +};
> +
> +/* CC Control Register(CCCR) */
> +#define CCCR_CCE BIT(1)
> +#define CCCR_INIT BIT(0)
> +
> +/* Bit Timing & Prescaler Register (BTP) */
> +#define BTR_BRP_MASK 0x3ff
> +#define BTR_BRP_SHIFT 16
> +#define BTR_TSEG1_SHIFT 8
> +#define BTR_TSEG1_MASK (0x3f << BTR_TSEG1_SHIFT)
> +#define BTR_TSEG2_SHIFT 4
> +#define BTR_TSEG2_MASK (0xf << BTR_TSEG2_SHIFT)
> +#define BTR_SJW_SHIFT 0
> +#define BTR_SJW_MASK 0xf
> +
> +/* Interrupt Register(IR) */
> +#define IR_ALL_INT 0xffffffff
> +#define IR_STE BIT(31)
> +#define IR_FOE BIT(30)
> +#define IR_ACKE BIT(29)
> +#define IR_BE BIT(28)
> +#define IR_CRCE BIT(27)
> +#define IR_WDI BIT(26)
> +#define IR_BO BIT(25)
> +#define IR_EW BIT(24)
> +#define IR_EP BIT(23)
> +#define IR_ELO BIT(22)
> +#define IR_BEU BIT(21)
> +#define IR_BEC BIT(20)
> +#define IR_DRX BIT(19)
> +#define IR_TOO BIT(18)
> +#define IR_MRAF BIT(17)
> +#define IR_TSW BIT(16)
> +#define IR_TEFL BIT(15)
> +#define IR_TEFF BIT(14)
> +#define IR_TEFW BIT(13)
> +#define IR_TEFN BIT(12)
> +#define IR_TFE BIT(11)
> +#define IR_TCF BIT(10)
> +#define IR_TC BIT(9)
> +#define IR_HPM BIT(8)
> +#define IR_RF1L BIT(7)
> +#define IR_RF1F BIT(6)
> +#define IR_RF1W BIT(5)
> +#define IR_RF1N BIT(4)
> +#define IR_RF0L BIT(3)
> +#define IR_RF0F BIT(2)
> +#define IR_RF0W BIT(1)
> +#define IR_RF0N BIT(0)
> +#define IR_ERR_ALL (IR_STE | IR_FOE | IR_ACKE | IR_BE | IR_CRCE | \
> + IR_WDI | IR_BO | IR_EW | IR_EP | IR_ELO | IR_BEU | \
> + IR_BEC | IR_TOO | IR_MRAF | IR_TSW | IR_TEFL | IR_RF1L | \
> + IR_RF0L)
> +
> +/* Rx FIFO 0/1 Configuration (RXF0C/RXF1C) */
> +#define RXFC_FWM_OFF 24
> +#define RXFC_FWM_MASK 0x7f
> +#define RXFC_FWM_1 (1 << RXFC_FWM_OFF)
> +#define RXFC_FS_OFF 16
> +#define RXFC_FS_MASK 0x7f
> +
> +/* Rx FIFO 0/1 Status (RXF0S/RXF1S) */
> +#define RXFS_RFL BIT(25)
> +#define RXFS_FF BIT(24)
> +#define RXFS_FPI_OFF 16
> +#define RXFS_FPI_MASK 0x3f0000
> +#define RXFS_FGI_OFF 8
> +#define RXFS_FGI_MASK 0x3f00
> +#define RXFS_FFL_MASK 0x7f
> +
> +/* Tx Buffer Configuration(TXBC) */
> +#define TXBC_NDTB_OFF 16
> +#define TXBC_NDTB_MASK 0x3f
> +
> +/* Tx Buffer Element Size Configuration(TXESC) */
> +#define TXESC_TBDS_8BYTES 0x0
> +/* Tx Buffer Element */
> +#define TX_BUF_XTD BIT(30)
> +#define TX_BUF_RTR BIT(29)
> +
> +/* Rx Buffer Element Size Configuration(TXESC) */
> +#define M_CAN_RXESC_8BYTES 0x0
> +/* Tx Buffer Element */
> +#define RX_BUF_ESI BIT(31)
> +#define RX_BUF_XTD BIT(30)
> +#define RX_BUF_RTR BIT(29)
> +
> +/* Message RAM Configuration (in bytes) */
> +#define SIDF_ELEMENT_SIZE 4
> +#define XIDF_ELEMENT_SIZE 8
> +#define RXF0_ELEMENT_SIZE 16
> +#define RXF1_ELEMENT_SIZE 16
> +#define RXB_ELEMENT_SIZE 16
> +#define TXE_ELEMENT_SIZE 8
> +#define TXB_ELEMENT_SIZE 16
> +
> +/* m_can private data structure */
> +struct m_can_priv {
> + struct can_priv can; /* must be the first member */
> + struct napi_struct napi;
> + struct net_device *dev;
> + struct device *device;
> + struct clk *clk;
> + void __iomem *base;
> + u32 irqstatus;
> +
> + /* message ram configuration */
> + void __iomem *mram_base;
> + u32 mram_off;
> + u32 sidf_elems;
> + u32 sidf_off;
> + u32 xidf_elems;
> + u32 xidf_off;
> + u32 rxf0_elems;
> + u32 rxf0_off;
> + u32 rxf1_elems;
> + u32 rxf1_off;
> + u32 rxb_elems;
> + u32 rxb_off;
> + u32 txe_elems;
> + u32 txe_off;
> + u32 txb_elems;
> + u32 txb_off;
> +};
> +
> +static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)
> +{
> + return readl(priv->base + reg);
> +}
> +
> +static inline void m_can_write(const struct m_can_priv *priv,
> + enum m_can_reg reg, u32 val)
> +{
> + writel(val, priv->base + reg);
> +}
> +
> +static inline void m_can_config_endisable(const struct m_can_priv *priv,
> + bool enable)
> +{
> + u32 cccr = m_can_read(priv, M_CAN_CCCR);
> + u32 timeout = 10, val;
> +
> + if (enable) {
> + /* enable m_can configuration */
> + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
> + /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
> + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
> + } else {
> + m_can_write(priv, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
> + }
> +
> + /* there's a delay for module initialization */
> + val = enable ? CCCR_INIT | CCCR_CCE : 0;
nitpick:
If you declare:
u32 val = 0;
you can use a more readable:
if (enable)
val = CCCR_INIT | CCCR_CCE;
> + while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE))
> + != val) {
> + if (timeout == 0) {
> + netdev_warn(priv->dev, "Failed to init module\n");
> + return;
> + }
> + timeout--;
> + udelay(1);
> + }
> +}
> +
> +static void m_can_enable_all_interrupts(struct m_can_priv *priv, bool enable)
> +{
> + m_can_write(priv, M_CAN_ILE, enable ? 0x00000003 : 0x0);
> +}
Can you make this two functions, I personally don't like to call a
function: *_enable_* if is actually disabled something. Please make use
of the defines instead of using 0x3.
> +
> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> + struct net_device_stats *stats = &dev->stats;
> + struct sk_buff *skb;
> + struct can_frame *frame;
> + u32 rxfs, flags, fgi;
> + u32 num_rx_pkts = 0;
> +
> + rxfs = m_can_read(priv, M_CAN_RXF0S);
> + if (!(rxfs & RXFS_FFL_MASK)) {
> + netdev_dbg(dev, "no messages in fifo0\n");
> + return 0;
> + }
> +
> + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
Please remove the netdev_dbg(), once the driver is stable it should be
of no use.
> + if (rxfs & RXFS_RFL)
> + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
What does that mean? Can you still rx the message if it's lost?
> +
> + skb = alloc_can_skb(dev, &frame);
> + if (!skb) {
> + stats->rx_dropped++;
> + return -ENOMEM;
Have a look at the user of m_can_do_rx_poll() and how it makes use of
the return value.
> + }
> +
> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
where it's not a fifo at all.
> + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
> + if (flags & RX_BUF_XTD)
> + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> + else
> + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
> + netdev_dbg(dev, "R0 0x%x\n", flags);
please remove dbg
> +
> + if (flags & RX_BUF_RTR) {
> + frame->can_id |= CAN_RTR_FLAG;
> + } else {
> + flags = readl(priv->mram_base +
> + priv->rxf0_off + fgi * 16 + 0x4);
> + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> + netdev_dbg(dev, "R1 0x%x\n", flags);
please remove
> +
> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> + priv->rxf0_off + fgi * 16 + 0x8);
> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> + priv->rxf0_off + fgi * 16 + 0xC);
can you create a wrapper function to hide the pointer arithmetics here?
Somethig like m_can_read_fifo()
> + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
> + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
> + }
> +
> + /* acknowledge rx fifo 0 */
> + m_can_write(priv, M_CAN_RXF0A, fgi);
> +
> + netif_receive_skb(skb);
> + netdev_dbg(dev, "new packet received\n");
> +
> + stats->rx_packets++;
> + stats->rx_bytes += frame->can_dlc;
Please move the stats handling in front of netif_receive_skb() as the
skb and thus frame is not a valid pointer anymore.
> +
> + can_led_event(dev, CAN_LED_EVENT_RX);
Please move out of the loop so that it is just called once (if a CAN
frame is rx'ed) per m_can_do_rx_poll().
> +
> + quota--;
> + num_rx_pkts++;
> + rxfs = m_can_read(priv, M_CAN_RXF0S);
> + };
> +
> + return num_rx_pkts;
> +}
> +
> +static int m_can_poll(struct napi_struct *napi, int quota)
> +{
> + struct net_device *dev = napi->dev;
> + struct m_can_priv *priv = netdev_priv(dev);
> + u32 work_done = 0;
Pleas make work_done a int, as it is retuned by this funtcion.
> + u32 irqstatus;
> +
> + irqstatus = m_can_read(priv, M_CAN_IR);
> + if (irqstatus)
> + netdev_dbg(dev, "irqstatus updated! new 0x%x old 0x%x rxf0s 0x%x\n",
> + irqstatus, priv->irqstatus,
> + m_can_read(priv, M_CAN_RXF0S));
Please remove debug.
> +
> + irqstatus = irqstatus | priv->irqstatus;
> + if (!irqstatus)
> + goto end;
> +
> + if (irqstatus & IR_RF0N)
> + /* handle events corresponding to receive message objects */
> + work_done += m_can_do_rx_poll(dev, (quota - work_done));
> +
> + if (work_done < quota) {
> + napi_complete(napi);
> + /* enable all IRQs */
> + m_can_enable_all_interrupts(priv, true);
> + }
> +
> +end:
> + return work_done;
> +}
> +
> +static irqreturn_t m_can_isr(int irq, void *dev_id)
> +{
> + struct net_device *dev = (struct net_device *)dev_id;
> + struct m_can_priv *priv = netdev_priv(dev);
> + struct net_device_stats *stats = &dev->stats;
> + u32 ir;
> +
> + ir = m_can_read(priv, M_CAN_IR);
> + if (!ir)
> + return IRQ_NONE;
> +
> + netdev_dbg(dev, "ir 0x%x rxfs0 0x%x\n", ir,
> + m_can_read(priv, M_CAN_RXF0S));
please remove
> +
> + /* ACK all irqs */
> + if (ir & IR_ALL_INT)
> + m_can_write(priv, M_CAN_IR, ir);
> +
> + if (ir & IR_ERR_ALL) {
> + netdev_dbg(dev, "bus error\n");
> + /* TODO: handle bus error */
> + }
> +
> + /* save irqstatus for later using */
> + priv->irqstatus = ir;
> +
> + /*
> + * schedule NAPI in case of
> + * - rx IRQ
> + * - state change IRQ(TODO)
> + * - bus error IRQ and bus error reporting (TODO)
> + */
> + if (ir & IR_RF0N) {
> + m_can_enable_all_interrupts(priv, false);
> + napi_schedule(&priv->napi);
> + }
> +
> + /* FIFO overflow */
> + if (ir & IR_RF0L) {
> + dev->stats.rx_over_errors++;
> + dev->stats.rx_errors++;
> + }
> +
> + /* transmission complete interrupt */
> + if (ir & IR_TC) {
> + netdev_dbg(dev, "tx complete\n");
> + stats->tx_bytes += can_get_echo_skb(dev, 0);
> + stats->tx_packets++;
> + can_led_event(dev, CAN_LED_EVENT_TX);
> + netif_wake_queue(dev);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static const struct can_bittiming_const m_can_bittiming_const = {
> + .name = KBUILD_MODNAME,
> + .tseg1_min = 2, /* Time segment 1 = prop_seg + phase_seg1 */
> + .tseg1_max = 64,
> + .tseg2_min = 1, /* Time segment 2 = phase_seg2 */
> + .tseg2_max = 16,
> + .sjw_max = 16,
> + .brp_min = 1,
> + .brp_max = 1024,
> + .brp_inc = 1,
> +};
> +
> +static int m_can_set_bittiming(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> + const struct can_bittiming *bt = &priv->can.bittiming;
> + u16 brp, sjw, tseg1, tseg2;
> + u32 reg_btp;
> +
> + brp = bt->brp - 1;
> + sjw = bt->sjw - 1;
> + tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
> + tseg2 = bt->phase_seg2 - 1;
> + reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
> + (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
> + m_can_write(priv, M_CAN_BTP, reg_btp);
> + netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp);
> +
> + return 0;
> +}
> +
> +/*
> + * Configure M_CAN chip:
> + * - set rx buffer/fifo element size
> + * - configure rx fifo
> + * - accept non-matching frame into fifo 0
> + * - configure tx buffer
> + * - setup bittiming
> + * - TODO:
> + * 1) other working modes support like monitor, loopback...
> + * 2) lec error status report enable
> + */
> +static void m_can_chip_config(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + m_can_config_endisable(priv, true);
> +
> + /* RX Buffer/FIFO Element Size 8 bytes data field */
> + m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES);
> +
> + /* Accept Non-matching Frames Into FIFO 0 */
> + m_can_write(priv, M_CAN_GFC, 0x0);
> +
> + /* only support one Tx Buffer currently */
> + m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
> + (priv->mram_off + priv->txb_off));
> +
> + /* only support 8 bytes firstly */
> + m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
> +
> + m_can_write(priv, M_CAN_TXEFC, 0x00010000 |
> + (priv->mram_off + priv->txe_off));
> +
> + /* rx fifo configuration, blocking mode, fifo size 1 */
> + m_can_write(priv, M_CAN_RXF0C, (priv->rxf0_elems << RXFC_FS_OFF) |
> + RXFC_FWM_1 | (priv->mram_off + priv->rxf0_off));
> +
> + m_can_write(priv, M_CAN_RXF1C, (priv->rxf1_elems << RXFC_FS_OFF) |
> + RXFC_FWM_1 | (priv->mram_off + priv->rxf1_off));
> +
> + /* enable all interrupts */
> + m_can_write(priv, M_CAN_IR, IR_ALL_INT);
> + m_can_write(priv, M_CAN_IE, IR_ALL_INT);
> + m_can_write(priv, M_CAN_ILS, 0xFFFF0000);
^^^^^^^^^^
A define would be nice.
> +
> + /* set bittiming params */
> + m_can_set_bittiming(dev);
> +
> + m_can_config_endisable(priv, false);
> +}
> +
> +static void m_can_start(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + /* basic m_can configuration */
> + m_can_chip_config(dev);
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + /* enable status change, error and module interrupts */
> + m_can_enable_all_interrupts(priv, true);
> +}
> +
> +static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> + switch (mode) {
> + case CAN_MODE_START:
> + m_can_start(dev);
> + netif_wake_queue(dev);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int m_can_get_berr_counter(const struct net_device *dev,
> + struct can_berr_counter *bec)
> +{
> + /* TODO */
> +
> + return 0;
> +}
> +
> +static void free_m_can_dev(struct net_device *dev)
> +{
> + free_candev(dev);
> +}
> +
> +static struct net_device *alloc_m_can_dev(void)
> +{
> + struct net_device *dev;
> + struct m_can_priv *priv;
> +
> + dev = alloc_candev(sizeof(struct m_can_priv), 1);
> + if (!dev)
> + return NULL;
> +
> + priv = netdev_priv(dev);
> + netif_napi_add(dev, &priv->napi, m_can_poll, M_CAN_NAPI_WEIGHT);
> +
> + priv->dev = dev;
> + priv->can.bittiming_const = &m_can_bittiming_const;
> + priv->can.do_set_mode = m_can_set_mode;
> + priv->can.do_get_berr_counter = m_can_get_berr_counter;
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> + CAN_CTRLMODE_LISTENONLY |
> + CAN_CTRLMODE_BERR_REPORTING;
Are you taking care of all ctrlmodes you claim to support here?
> +
> + return dev;
> +}
> +
> +static int m_can_open(struct net_device *dev)
> +{
> + int err;
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + clk_prepare_enable(priv->clk);
> +
> + /* open the can device */
> + err = open_candev(dev);
> + if (err) {
> + netdev_err(dev, "failed to open can device\n");
> + goto exit_open_fail;
> + }
> +
> + /* register interrupt handler */
> + err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> + dev);
> + if (err < 0) {
> + netdev_err(dev, "failed to request interrupt\n");
> + goto exit_irq_fail;
> + }
> +
> + /* start the m_can controller */
> + m_can_start(dev);
> +
> + can_led_event(dev, CAN_LED_EVENT_OPEN);
> + napi_enable(&priv->napi);
> + netif_start_queue(dev);
> +
> + return 0;
> +
> +exit_irq_fail:
> + close_candev(dev);
> +exit_open_fail:
> + clk_disable_unprepare(priv->clk);
> + return err;
> +}
> +
> +static void m_can_stop(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + /* disable all interrupts */
> + m_can_enable_all_interrupts(priv, false);
> +
> + /* set the state as STOPPED */
> + priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +static int m_can_close(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + netif_stop_queue(dev);
> + napi_disable(&priv->napi);
> + m_can_stop(dev);
> + free_irq(dev->irq, dev);
> + close_candev(dev);
> + can_led_event(dev, CAN_LED_EVENT_STOP);
> +
> + return 0;
> +}
> +
> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> + struct can_frame *frame = (struct can_frame *)skb->data;
> + u32 flags = 0, id;
> +
> + if (can_dropped_invalid_skb(dev, skb))
> + return NETDEV_TX_OK;
> +
> + netif_stop_queue(dev);
> +
> + if (frame->can_id & CAN_RTR_FLAG)
> + flags |= TX_BUF_RTR;
> +
> + if (frame->can_id & CAN_EFF_FLAG) {
> + id = frame->can_id & CAN_EFF_MASK;
> + flags |= TX_BUF_XTD;
> + } else {
> + id = ((frame->can_id & CAN_SFF_MASK) << 18);
> + }
> +
> + /* message ram configuration */
> + writel(id | flags,
> + priv->mram_base + priv->mram_off + priv->txb_off);
> + writel(frame->can_dlc << 16,
> + priv->mram_base + priv->mram_off + priv->txb_off + 0x4);
> + writel(*(u32 *)(frame->data + 0),
> + priv->mram_base + priv->mram_off + priv->txb_off + 0x8);
> + writel(*(u32 *)(frame->data + 4),
> + priv->mram_base + priv->mram_off + priv->txb_off + 0xc);
Here an inline function to hide the details of pointer arithmetics would
be nice.
> +
> + can_put_echo_skb(skb, dev, 0);
> +
> + /* enable first TX buffer to start transfer */
> + m_can_write(priv, M_CAN_TXBTIE, 0x00000001);
> + m_can_write(priv, M_CAN_TXBAR, 0x00000001);
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops m_can_netdev_ops = {
> + .ndo_open = m_can_open,
> + .ndo_stop = m_can_close,
> + .ndo_start_xmit = m_can_start_xmit,
> +};
> +
> +static int register_m_can_dev(struct net_device *dev)
> +{
> + dev->flags |= IFF_ECHO; /* we support local echo */
> + dev->netdev_ops = &m_can_netdev_ops;
> +
> + return register_candev(dev);
> +}
> +
> +static const struct of_device_id m_can_of_table[] = {
> + { .compatible = "bosch,m_can", .data = NULL },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, m_can_of_table);
> +
> +static int m_can_of_parse_mram(struct platform_device *pdev,
> + struct m_can_priv *priv)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + void __iomem *addr;
> + u32 out_val[8];
> + int ret;
> +
> + /* message ram could be shared */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> + if (!res)
> + return -ENODEV;
> +
> + addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> + if (!addr)
> + return -ENODEV;
> +
> + /* get message ram configuration */
> + ret = of_property_read_u32_array(np, "mram-cfg",
> + out_val, sizeof(out_val) / 4);
> + if (ret) {
> + dev_err(&pdev->dev, "can not get message ram configuration\n");
> + return -ENODEV;
> + }
> +
> + priv->mram_base = addr;
> + priv->mram_off = out_val[0];
> + priv->sidf_elems = out_val[1];
> + priv->sidf_off = priv->mram_off;
> + priv->xidf_elems = out_val[2];
> + priv->xidf_off = priv->sidf_off + priv->sidf_elems * SIDF_ELEMENT_SIZE;
> + priv->rxf0_elems = out_val[3] & RXFC_FS_MASK;
> + priv->rxf0_off = priv->xidf_off + priv->xidf_elems * XIDF_ELEMENT_SIZE;
> + priv->rxf1_elems = out_val[4] & RXFC_FS_MASK;
> + priv->rxf1_off = priv->rxf0_off + priv->rxf0_elems * RXF0_ELEMENT_SIZE;
> + priv->rxb_elems = out_val[5];
> + priv->rxb_off = priv->rxf1_off + priv->rxf1_elems * RXF1_ELEMENT_SIZE;
> + priv->txe_elems = out_val[6];
> + priv->txe_off = priv->rxb_off + priv->rxb_elems * RXB_ELEMENT_SIZE;
> + priv->txb_elems = out_val[7] & TXBC_NDTB_MASK;
> + priv->txb_off = priv->txe_off + priv->txe_elems * TXE_ELEMENT_SIZE;
> +
> + dev_dbg(&pdev->dev, "mram_base =%p mram_off =0x%x "
> + "sidf %d xidf %d rxf0 %d rxf1 %d rxb %d txe %d txb %d\n",
> + priv->mram_base, priv->mram_off, priv->sidf_elems,
> + priv->xidf_elems, priv->rxf0_elems, priv->rxf1_elems,
> + priv->rxb_elems, priv->txe_elems, priv->txb_elems);
> +
> + return 0;
> +}
> +
> +static int m_can_plat_probe(struct platform_device *pdev)
> +{
> + struct net_device *dev;
> + struct m_can_priv *priv;
> + struct pinctrl *pinctrl;
> + struct resource *res;
> + void __iomem *addr;
> + struct clk *clk;
> + int irq, ret;
> +
> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> + if (IS_ERR(pinctrl))
> + dev_warn(&pdev->dev,
> + "failed to configure pins from driver\n");
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "no clock find\n");
> + return PTR_ERR(clk);
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + irq = platform_get_irq(pdev, 0);
> + if (IS_ERR(addr) || irq < 0)
> + return -EINVAL;
> +
> + /* allocate the m_can device */
> + dev = alloc_m_can_dev();
> + if (!dev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(dev);
> + dev->irq = irq;
> + priv->base = addr;
> + priv->device = &pdev->dev;
> + priv->clk = clk;
> + priv->can.clock.freq = clk_get_rate(clk);
> +
> + ret = m_can_of_parse_mram(pdev, priv);
> + if (ret)
> + goto failed_free_dev;
> +
> + platform_set_drvdata(pdev, dev);
> + SET_NETDEV_DEV(dev, &pdev->dev);
> +
> + ret = register_m_can_dev(dev);
> + if (ret) {
> + dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> + KBUILD_MODNAME, ret);
> + goto failed_free_dev;
> + }
> +
> + devm_can_led_init(dev);
> +
> + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> + KBUILD_MODNAME, priv->base, dev->irq);
> +
> + return 0;
> +
> +failed_free_dev:
> + free_m_can_dev(dev);
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM
please remove and use __maybe_unused unstead
> +static int m_can_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct m_can_priv *priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev)) {
> + netif_stop_queue(ndev);
> + netif_device_detach(ndev);
> + }
> +
> + /* TODO: enter low power */
> +
> + priv->can.state = CAN_STATE_SLEEPING;
> +
> + return 0;
> +}
> +
> +static int m_can_resume(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct m_can_priv *priv = netdev_priv(ndev);
> +
> + /* TODO: exit low power */
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + if (netif_running(ndev)) {
> + netif_device_attach(ndev);
> + netif_start_queue(ndev);
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static void unregister_m_can_dev(struct net_device *dev)
> +{
> + unregister_candev(dev);
> +}
> +
> +static int m_can_plat_remove(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + unregister_m_can_dev(dev);
> + platform_set_drvdata(pdev, NULL);
> +
> + free_m_can_dev(dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops m_can_pmops = {
> + SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
> +};
> +
> +static struct platform_driver m_can_plat_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(m_can_of_table),
> + .pm = &m_can_pmops,
> + },
> + .probe = m_can_plat_probe,
> + .remove = m_can_plat_remove,
> +};
> +
> +module_platform_driver(m_can_plat_driver);
> +
> +MODULE_AUTHOR("Dong Aisheng <b29396@freescale.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
>
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: 242 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-01 10:29 ` Marc Kleine-Budde
@ 2014-07-02 6:20 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-07-02 6:20 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, wg, devicetree
Hi Marc,
On Tue, Jul 01, 2014 at 12:29:17PM +0200, Marc Kleine-Budde wrote:
> On 06/27/2014 12:00 PM, Dong Aisheng wrote:
> > The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller.
> > For TX, only one dedicated tx buffer is used for sending data.
> > For RX, RXFIFO 0 is used for receiving data to avoid overflow.
> > Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
> >
> > Due to the message ram can be shared by multi m_can instances
> > and the fifo element is configurable which is SoC dependant,
> > the design is to parse the message ram related configuration data from device
> > tree rather than hardcode define it in driver which can make the message
> > ram sharing fully transparent to M_CAN controller driver,
> > then we can gain better driver maintainability and future features upgrade.
> >
> > M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> > and bitrate switch at runtime, however, this patch still does not add the
> > support for these features.
> >
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> > .../devicetree/bindings/net/can/m_can.txt | 29 +
> > drivers/net/can/Kconfig | 5 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/m_can.c | 867 ++++++++++++++++++++
> > 4 files changed, 902 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> > new file mode 100644
> > index 0000000..2b22d02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> > @@ -0,0 +1,29 @@
> > +Bosch MCAN controller Device Tree Bindings
> > +-------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Should be "bosch,m_can" for M_CAN controllers
> > +- reg : physical base address and size of the M_CAN
> > + registers map and message ram
> > +- interrupts : property with a value describing the interrupt
> > + number
> > +- clocks : clocks used by controller
> > +- mram-cfg : message ram configuration data, the format is
> > + <offset sidf_elems xidf_elems rxf1_elems rxb_elems txe_elems txb_elems>
> > + The 'offset' is the address offset inside the message ram. This is usually set
> > + if you're using the shared message ram while the other part is used by another
> > + m_can controller.
> > + The left cell are all the number of each elements inside the message ram.
> > + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> > + for each elements definition.
> > +
> > +Example:
> > +canfd1: canfd@020e8000 {
> > + compatible = "bosch,m_can";
> > + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> > + reg-names = "canfd", "message_ram";
> > + interrupts = <0 114 0x04>;
> > + clocks = <&clks IMX6SX_CLK_CANFD>;
> > + mram-cfg = <0x0 0 0 32 32 32 0 1>;
> > + status = "disabled";
> > +};
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index 4168822..3fd1c4a 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -137,6 +137,11 @@ config CAN_XILINXCAN
> > Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> > Zynq CANPS IP.
> >
> > +config CAN_M_CAN
> > + tristate "Bosch M_CAN devices"
> > + ---help---
> > + Say Y here if you want to support for Bosch M_CAN controller.
> > +
> > source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index 1697f22..69dee2c 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -17,6 +17,7 @@ obj-y += softing/
> > obj-$(CONFIG_CAN_SJA1000) += sja1000/
> > obj-$(CONFIG_CAN_MSCAN) += mscan/
> > obj-$(CONFIG_CAN_C_CAN) += c_can/
> > +obj-$(CONFIG_CAN_M_CAN) += m_can.o
> > obj-$(CONFIG_CAN_CC770) += cc770/
> > obj-$(CONFIG_CAN_AT91) += at91_can.o
> > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> > diff --git a/drivers/net/can/m_can.c b/drivers/net/can/m_can.c
> > new file mode 100644
> > index 0000000..61e9a8e
> > --- /dev/null
> > +++ b/drivers/net/can/m_can.c
> > @@ -0,0 +1,867 @@
> > +/*
> > + * CAN bus driver for Bosch M_CAN controller
> > + *
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > + * Dong Aisheng <b29396@freescale.com>
> > + *
> > + * Bosch M_CAN user manual can be obtained from:
> > + * http://www.bosch-semiconductors.de/media/pdf_1/ipmodules_1/m_can/
> > + * mcan_users_manual_v302.pdf
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/if_arp.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/can/dev.h>
> > +
> > +/* napi related */
> > +#define M_CAN_NAPI_WEIGHT 64
> > +
> > +/* registers definition */
> > +enum m_can_reg {
> > + M_CAN_CREL = 0x0,
> > + M_CAN_ENDN = 0x4,
> > + M_CAN_CUST = 0x8,
> > + M_CAN_FBTP = 0xc,
> > + M_CAN_TEST = 0x10,
> > + M_CAN_RWD = 0x14,
> > + M_CAN_CCCR = 0x18,
> > + M_CAN_BTP = 0x1c,
> > + M_CAN_TSCC = 0x20,
> > + M_CAN_TSCV = 0x24,
> > + M_CAN_TOCC = 0x28,
> > + M_CAN_TOCV = 0x2c,
> > + M_CAN_ECR = 0x40,
> > + M_CAN_PSR = 0x44,
> > + M_CAN_IR = 0x50,
> > + M_CAN_IE = 0x54,
> > + M_CAN_ILS = 0x58,
> > + M_CAN_ILE = 0x5c,
> > + M_CAN_GFC = 0x80,
> > + M_CAN_SIDFC = 0x84,
> > + M_CAN_XIDFC = 0x88,
> > + M_CAN_XIDAM = 0x90,
> > + M_CAN_HPMS = 0x94,
> > + M_CAN_NDAT1 = 0x98,
> > + M_CAN_NDAT2 = 0x9c,
> > + M_CAN_RXF0C = 0xa0,
> > + M_CAN_RXF0S = 0xa4,
> > + M_CAN_RXF0A = 0xa8,
> > + M_CAN_RXBC = 0xac,
> > + M_CAN_RXF1C = 0xb0,
> > + M_CAN_RXF1S = 0xb4,
> > + M_CAN_RXF1A = 0xb8,
> > + M_CAN_RXESC = 0xbc,
> > + M_CAN_TXBC = 0xc0,
> > + M_CAN_TXFQS = 0xc4,
> > + M_CAN_TXESC = 0xc8,
> > + M_CAN_TXBRP = 0xcc,
> > + M_CAN_TXBAR = 0xd0,
> > + M_CAN_TXBCR = 0xd4,
> > + M_CAN_TXBTO = 0xd8,
> > + M_CAN_TXBCF = 0xdc,
> > + M_CAN_TXBTIE = 0xe0,
> > + M_CAN_TXBCIE = 0xe4,
> > + M_CAN_TXEFC = 0xf0,
> > + M_CAN_TXEFS = 0xf4,
> > + M_CAN_TXEFA = 0xf8,
> > +};
> > +
> > +/* CC Control Register(CCCR) */
> > +#define CCCR_CCE BIT(1)
> > +#define CCCR_INIT BIT(0)
> > +
> > +/* Bit Timing & Prescaler Register (BTP) */
> > +#define BTR_BRP_MASK 0x3ff
> > +#define BTR_BRP_SHIFT 16
> > +#define BTR_TSEG1_SHIFT 8
> > +#define BTR_TSEG1_MASK (0x3f << BTR_TSEG1_SHIFT)
> > +#define BTR_TSEG2_SHIFT 4
> > +#define BTR_TSEG2_MASK (0xf << BTR_TSEG2_SHIFT)
> > +#define BTR_SJW_SHIFT 0
> > +#define BTR_SJW_MASK 0xf
> > +
> > +/* Interrupt Register(IR) */
> > +#define IR_ALL_INT 0xffffffff
> > +#define IR_STE BIT(31)
> > +#define IR_FOE BIT(30)
> > +#define IR_ACKE BIT(29)
> > +#define IR_BE BIT(28)
> > +#define IR_CRCE BIT(27)
> > +#define IR_WDI BIT(26)
> > +#define IR_BO BIT(25)
> > +#define IR_EW BIT(24)
> > +#define IR_EP BIT(23)
> > +#define IR_ELO BIT(22)
> > +#define IR_BEU BIT(21)
> > +#define IR_BEC BIT(20)
> > +#define IR_DRX BIT(19)
> > +#define IR_TOO BIT(18)
> > +#define IR_MRAF BIT(17)
> > +#define IR_TSW BIT(16)
> > +#define IR_TEFL BIT(15)
> > +#define IR_TEFF BIT(14)
> > +#define IR_TEFW BIT(13)
> > +#define IR_TEFN BIT(12)
> > +#define IR_TFE BIT(11)
> > +#define IR_TCF BIT(10)
> > +#define IR_TC BIT(9)
> > +#define IR_HPM BIT(8)
> > +#define IR_RF1L BIT(7)
> > +#define IR_RF1F BIT(6)
> > +#define IR_RF1W BIT(5)
> > +#define IR_RF1N BIT(4)
> > +#define IR_RF0L BIT(3)
> > +#define IR_RF0F BIT(2)
> > +#define IR_RF0W BIT(1)
> > +#define IR_RF0N BIT(0)
> > +#define IR_ERR_ALL (IR_STE | IR_FOE | IR_ACKE | IR_BE | IR_CRCE | \
> > + IR_WDI | IR_BO | IR_EW | IR_EP | IR_ELO | IR_BEU | \
> > + IR_BEC | IR_TOO | IR_MRAF | IR_TSW | IR_TEFL | IR_RF1L | \
> > + IR_RF0L)
> > +
> > +/* Rx FIFO 0/1 Configuration (RXF0C/RXF1C) */
> > +#define RXFC_FWM_OFF 24
> > +#define RXFC_FWM_MASK 0x7f
> > +#define RXFC_FWM_1 (1 << RXFC_FWM_OFF)
> > +#define RXFC_FS_OFF 16
> > +#define RXFC_FS_MASK 0x7f
> > +
> > +/* Rx FIFO 0/1 Status (RXF0S/RXF1S) */
> > +#define RXFS_RFL BIT(25)
> > +#define RXFS_FF BIT(24)
> > +#define RXFS_FPI_OFF 16
> > +#define RXFS_FPI_MASK 0x3f0000
> > +#define RXFS_FGI_OFF 8
> > +#define RXFS_FGI_MASK 0x3f00
> > +#define RXFS_FFL_MASK 0x7f
> > +
> > +/* Tx Buffer Configuration(TXBC) */
> > +#define TXBC_NDTB_OFF 16
> > +#define TXBC_NDTB_MASK 0x3f
> > +
> > +/* Tx Buffer Element Size Configuration(TXESC) */
> > +#define TXESC_TBDS_8BYTES 0x0
> > +/* Tx Buffer Element */
> > +#define TX_BUF_XTD BIT(30)
> > +#define TX_BUF_RTR BIT(29)
> > +
> > +/* Rx Buffer Element Size Configuration(TXESC) */
> > +#define M_CAN_RXESC_8BYTES 0x0
> > +/* Tx Buffer Element */
> > +#define RX_BUF_ESI BIT(31)
> > +#define RX_BUF_XTD BIT(30)
> > +#define RX_BUF_RTR BIT(29)
> > +
> > +/* Message RAM Configuration (in bytes) */
> > +#define SIDF_ELEMENT_SIZE 4
> > +#define XIDF_ELEMENT_SIZE 8
> > +#define RXF0_ELEMENT_SIZE 16
> > +#define RXF1_ELEMENT_SIZE 16
> > +#define RXB_ELEMENT_SIZE 16
> > +#define TXE_ELEMENT_SIZE 8
> > +#define TXB_ELEMENT_SIZE 16
> > +
> > +/* m_can private data structure */
> > +struct m_can_priv {
> > + struct can_priv can; /* must be the first member */
> > + struct napi_struct napi;
> > + struct net_device *dev;
> > + struct device *device;
> > + struct clk *clk;
> > + void __iomem *base;
> > + u32 irqstatus;
> > +
> > + /* message ram configuration */
> > + void __iomem *mram_base;
> > + u32 mram_off;
> > + u32 sidf_elems;
> > + u32 sidf_off;
> > + u32 xidf_elems;
> > + u32 xidf_off;
> > + u32 rxf0_elems;
> > + u32 rxf0_off;
> > + u32 rxf1_elems;
> > + u32 rxf1_off;
> > + u32 rxb_elems;
> > + u32 rxb_off;
> > + u32 txe_elems;
> > + u32 txe_off;
> > + u32 txb_elems;
> > + u32 txb_off;
> > +};
> > +
> > +static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)
> > +{
> > + return readl(priv->base + reg);
> > +}
> > +
> > +static inline void m_can_write(const struct m_can_priv *priv,
> > + enum m_can_reg reg, u32 val)
> > +{
> > + writel(val, priv->base + reg);
> > +}
> > +
> > +static inline void m_can_config_endisable(const struct m_can_priv *priv,
> > + bool enable)
> > +{
> > + u32 cccr = m_can_read(priv, M_CAN_CCCR);
> > + u32 timeout = 10, val;
> > +
> > + if (enable) {
> > + /* enable m_can configuration */
> > + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
> > + /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
> > + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
> > + } else {
> > + m_can_write(priv, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
> > + }
> > +
> > + /* there's a delay for module initialization */
> > + val = enable ? CCCR_INIT | CCCR_CCE : 0;
>
> nitpick:
>
> If you declare:
>
> u32 val = 0;
>
> you can use a more readable:
>
> if (enable)
> val = CCCR_INIT | CCCR_CCE;
>
Right, will change to that.
> > + while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE))
> > + != val) {
> > + if (timeout == 0) {
> > + netdev_warn(priv->dev, "Failed to init module\n");
> > + return;
> > + }
> > + timeout--;
> > + udelay(1);
> > + }
> > +}
> > +
> > +static void m_can_enable_all_interrupts(struct m_can_priv *priv, bool enable)
> > +{
> > + m_can_write(priv, M_CAN_ILE, enable ? 0x00000003 : 0x0);
> > +}
>
> Can you make this two functions, I personally don't like to call a
> function: *_enable_* if is actually disabled something. Please make use
> of the defines instead of using 0x3.
>
Got it.
> > +
> > +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + struct net_device_stats *stats = &dev->stats;
> > + struct sk_buff *skb;
> > + struct can_frame *frame;
> > + u32 rxfs, flags, fgi;
> > + u32 num_rx_pkts = 0;
> > +
> > + rxfs = m_can_read(priv, M_CAN_RXF0S);
> > + if (!(rxfs & RXFS_FFL_MASK)) {
> > + netdev_dbg(dev, "no messages in fifo0\n");
> > + return 0;
> > + }
> > +
> > + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> > + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
>
> Please remove the netdev_dbg(), once the driver is stable it should be
> of no use.
>
Got it.
> > + if (rxfs & RXFS_RFL)
> > + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>
> What does that mean? Can you still rx the message if it's lost?
>
It just warns that there's a message lost, but there are still other
message in fifo to receive.
> > +
> > + skb = alloc_can_skb(dev, &frame);
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return -ENOMEM;
>
> Have a look at the user of m_can_do_rx_poll() and how it makes use of
> the return value.
>
Right, thanks for spotting it.
> > + }
> > +
> > + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>
> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
> where it's not a fifo at all.
>
Yes, it is real fifo in the message ram.
> > + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
> > + if (flags & RX_BUF_XTD)
> > + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> > + else
> > + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
> > + netdev_dbg(dev, "R0 0x%x\n", flags);
>
> please remove dbg
> > +
> > + if (flags & RX_BUF_RTR) {
> > + frame->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + flags = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0x4);
> > + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> > + netdev_dbg(dev, "R1 0x%x\n", flags);
>
> please remove
>
> > +
> > + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0x8);
> > + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0xC);
>
>
> can you create a wrapper function to hide the pointer arithmetics here?
> Somethig like m_can_read_fifo()
>
Yes, i could make a wrapper function for it.
> > + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
> > + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
> > + }
> > +
> > + /* acknowledge rx fifo 0 */
> > + m_can_write(priv, M_CAN_RXF0A, fgi);
> > +
> > + netif_receive_skb(skb);
> > + netdev_dbg(dev, "new packet received\n");
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += frame->can_dlc;
>
> Please move the stats handling in front of netif_receive_skb() as the
> skb and thus frame is not a valid pointer anymore.
>
Good catch!
Will change it.
> > +
> > + can_led_event(dev, CAN_LED_EVENT_RX);
>
> Please move out of the loop so that it is just called once (if a CAN
> frame is rx'ed) per m_can_do_rx_poll().
>
Why that?
The purpose is calling it for each new packet received.
> > +
> > + quota--;
> > + num_rx_pkts++;
> > + rxfs = m_can_read(priv, M_CAN_RXF0S);
> > + };
> > +
> > + return num_rx_pkts;
> > +}
> > +
> > +static int m_can_poll(struct napi_struct *napi, int quota)
> > +{
> > + struct net_device *dev = napi->dev;
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + u32 work_done = 0;
>
> Pleas make work_done a int, as it is retuned by this funtcion.
>
Right
> > + u32 irqstatus;
> > +
> > + irqstatus = m_can_read(priv, M_CAN_IR);
> > + if (irqstatus)
> > + netdev_dbg(dev, "irqstatus updated! new 0x%x old 0x%x rxf0s 0x%x\n",
> > + irqstatus, priv->irqstatus,
> > + m_can_read(priv, M_CAN_RXF0S));
>
> Please remove debug.
>
Got it.
> > +
> > + irqstatus = irqstatus | priv->irqstatus;
> > + if (!irqstatus)
> > + goto end;
> > +
> > + if (irqstatus & IR_RF0N)
> > + /* handle events corresponding to receive message objects */
> > + work_done += m_can_do_rx_poll(dev, (quota - work_done));
> > +
> > + if (work_done < quota) {
> > + napi_complete(napi);
> > + /* enable all IRQs */
> > + m_can_enable_all_interrupts(priv, true);
> > + }
> > +
> > +end:
> > + return work_done;
> > +}
> > +
> > +static irqreturn_t m_can_isr(int irq, void *dev_id)
> > +{
> > + struct net_device *dev = (struct net_device *)dev_id;
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + struct net_device_stats *stats = &dev->stats;
> > + u32 ir;
> > +
> > + ir = m_can_read(priv, M_CAN_IR);
> > + if (!ir)
> > + return IRQ_NONE;
> > +
> > + netdev_dbg(dev, "ir 0x%x rxfs0 0x%x\n", ir,
> > + m_can_read(priv, M_CAN_RXF0S));
>
> please remove
>
Got.
> > +
> > + /* ACK all irqs */
> > + if (ir & IR_ALL_INT)
> > + m_can_write(priv, M_CAN_IR, ir);
> > +
> > + if (ir & IR_ERR_ALL) {
> > + netdev_dbg(dev, "bus error\n");
> > + /* TODO: handle bus error */
> > + }
> > +
> > + /* save irqstatus for later using */
> > + priv->irqstatus = ir;
> > +
> > + /*
> > + * schedule NAPI in case of
> > + * - rx IRQ
> > + * - state change IRQ(TODO)
> > + * - bus error IRQ and bus error reporting (TODO)
> > + */
> > + if (ir & IR_RF0N) {
> > + m_can_enable_all_interrupts(priv, false);
> > + napi_schedule(&priv->napi);
> > + }
> > +
> > + /* FIFO overflow */
> > + if (ir & IR_RF0L) {
> > + dev->stats.rx_over_errors++;
> > + dev->stats.rx_errors++;
> > + }
> > +
> > + /* transmission complete interrupt */
> > + if (ir & IR_TC) {
> > + netdev_dbg(dev, "tx complete\n");
> > + stats->tx_bytes += can_get_echo_skb(dev, 0);
> > + stats->tx_packets++;
> > + can_led_event(dev, CAN_LED_EVENT_TX);
> > + netif_wake_queue(dev);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct can_bittiming_const m_can_bittiming_const = {
> > + .name = KBUILD_MODNAME,
> > + .tseg1_min = 2, /* Time segment 1 = prop_seg + phase_seg1 */
> > + .tseg1_max = 64,
> > + .tseg2_min = 1, /* Time segment 2 = phase_seg2 */
> > + .tseg2_max = 16,
> > + .sjw_max = 16,
> > + .brp_min = 1,
> > + .brp_max = 1024,
> > + .brp_inc = 1,
> > +};
> > +
> > +static int m_can_set_bittiming(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + const struct can_bittiming *bt = &priv->can.bittiming;
> > + u16 brp, sjw, tseg1, tseg2;
> > + u32 reg_btp;
> > +
> > + brp = bt->brp - 1;
> > + sjw = bt->sjw - 1;
> > + tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
> > + tseg2 = bt->phase_seg2 - 1;
> > + reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
> > + (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
> > + m_can_write(priv, M_CAN_BTP, reg_btp);
> > + netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Configure M_CAN chip:
> > + * - set rx buffer/fifo element size
> > + * - configure rx fifo
> > + * - accept non-matching frame into fifo 0
> > + * - configure tx buffer
> > + * - setup bittiming
> > + * - TODO:
> > + * 1) other working modes support like monitor, loopback...
> > + * 2) lec error status report enable
> > + */
> > +static void m_can_chip_config(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + m_can_config_endisable(priv, true);
> > +
> > + /* RX Buffer/FIFO Element Size 8 bytes data field */
> > + m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES);
> > +
> > + /* Accept Non-matching Frames Into FIFO 0 */
> > + m_can_write(priv, M_CAN_GFC, 0x0);
> > +
> > + /* only support one Tx Buffer currently */
> > + m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
> > + (priv->mram_off + priv->txb_off));
> > +
> > + /* only support 8 bytes firstly */
> > + m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
> > +
> > + m_can_write(priv, M_CAN_TXEFC, 0x00010000 |
> > + (priv->mram_off + priv->txe_off));
> > +
> > + /* rx fifo configuration, blocking mode, fifo size 1 */
> > + m_can_write(priv, M_CAN_RXF0C, (priv->rxf0_elems << RXFC_FS_OFF) |
> > + RXFC_FWM_1 | (priv->mram_off + priv->rxf0_off));
> > +
> > + m_can_write(priv, M_CAN_RXF1C, (priv->rxf1_elems << RXFC_FS_OFF) |
> > + RXFC_FWM_1 | (priv->mram_off + priv->rxf1_off));
> > +
> > + /* enable all interrupts */
> > + m_can_write(priv, M_CAN_IR, IR_ALL_INT);
> > + m_can_write(priv, M_CAN_IE, IR_ALL_INT);
> > + m_can_write(priv, M_CAN_ILS, 0xFFFF0000);
> ^^^^^^^^^^
>
> A define would be nice.
>
Will add it.
> > +
> > + /* set bittiming params */
> > + m_can_set_bittiming(dev);
> > +
> > + m_can_config_endisable(priv, false);
> > +}
> > +
> > +static void m_can_start(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + /* basic m_can configuration */
> > + m_can_chip_config(dev);
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + /* enable status change, error and module interrupts */
> > + m_can_enable_all_interrupts(priv, true);
> > +}
> > +
> > +static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
> > +{
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + m_can_start(dev);
> > + netif_wake_queue(dev);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int m_can_get_berr_counter(const struct net_device *dev,
> > + struct can_berr_counter *bec)
> > +{
> > + /* TODO */
> > +
> > + return 0;
> > +}
> > +
> > +static void free_m_can_dev(struct net_device *dev)
> > +{
> > + free_candev(dev);
> > +}
> > +
> > +static struct net_device *alloc_m_can_dev(void)
> > +{
> > + struct net_device *dev;
> > + struct m_can_priv *priv;
> > +
> > + dev = alloc_candev(sizeof(struct m_can_priv), 1);
> > + if (!dev)
> > + return NULL;
> > +
> > + priv = netdev_priv(dev);
> > + netif_napi_add(dev, &priv->napi, m_can_poll, M_CAN_NAPI_WEIGHT);
> > +
> > + priv->dev = dev;
> > + priv->can.bittiming_const = &m_can_bittiming_const;
> > + priv->can.do_set_mode = m_can_set_mode;
> > + priv->can.do_get_berr_counter = m_can_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY |
> > + CAN_CTRLMODE_BERR_REPORTING;
>
> Are you taking care of all ctrlmodes you claim to support here?
>
Yes, those features are marked as TODO in this patch and implemented
in the following two patches in this seris.
> > +
> > + return dev;
> > +}
> > +
> > +static int m_can_open(struct net_device *dev)
> > +{
> > + int err;
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + clk_prepare_enable(priv->clk);
> > +
> > + /* open the can device */
> > + err = open_candev(dev);
> > + if (err) {
> > + netdev_err(dev, "failed to open can device\n");
> > + goto exit_open_fail;
> > + }
> > +
> > + /* register interrupt handler */
> > + err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> > + dev);
> > + if (err < 0) {
> > + netdev_err(dev, "failed to request interrupt\n");
> > + goto exit_irq_fail;
> > + }
> > +
> > + /* start the m_can controller */
> > + m_can_start(dev);
> > +
> > + can_led_event(dev, CAN_LED_EVENT_OPEN);
> > + napi_enable(&priv->napi);
> > + netif_start_queue(dev);
> > +
> > + return 0;
> > +
> > +exit_irq_fail:
> > + close_candev(dev);
> > +exit_open_fail:
> > + clk_disable_unprepare(priv->clk);
> > + return err;
> > +}
> > +
> > +static void m_can_stop(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + /* disable all interrupts */
> > + m_can_enable_all_interrupts(priv, false);
> > +
> > + /* set the state as STOPPED */
> > + priv->can.state = CAN_STATE_STOPPED;
> > +}
> > +
> > +static int m_can_close(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + netif_stop_queue(dev);
> > + napi_disable(&priv->napi);
> > + m_can_stop(dev);
> > + free_irq(dev->irq, dev);
> > + close_candev(dev);
> > + can_led_event(dev, CAN_LED_EVENT_STOP);
> > +
> > + return 0;
> > +}
> > +
> > +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> > + struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + struct can_frame *frame = (struct can_frame *)skb->data;
> > + u32 flags = 0, id;
> > +
> > + if (can_dropped_invalid_skb(dev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + netif_stop_queue(dev);
> > +
> > + if (frame->can_id & CAN_RTR_FLAG)
> > + flags |= TX_BUF_RTR;
> > +
> > + if (frame->can_id & CAN_EFF_FLAG) {
> > + id = frame->can_id & CAN_EFF_MASK;
> > + flags |= TX_BUF_XTD;
> > + } else {
> > + id = ((frame->can_id & CAN_SFF_MASK) << 18);
> > + }
> > +
> > + /* message ram configuration */
> > + writel(id | flags,
> > + priv->mram_base + priv->mram_off + priv->txb_off);
> > + writel(frame->can_dlc << 16,
> > + priv->mram_base + priv->mram_off + priv->txb_off + 0x4);
> > + writel(*(u32 *)(frame->data + 0),
> > + priv->mram_base + priv->mram_off + priv->txb_off + 0x8);
> > + writel(*(u32 *)(frame->data + 4),
> > + priv->mram_base + priv->mram_off + priv->txb_off + 0xc);
>
> Here an inline function to hide the details of pointer arithmetics would
> be nice.
>
Yes, will try to do it in v2.
> > +
> > + can_put_echo_skb(skb, dev, 0);
> > +
> > + /* enable first TX buffer to start transfer */
> > + m_can_write(priv, M_CAN_TXBTIE, 0x00000001);
> > + m_can_write(priv, M_CAN_TXBAR, 0x00000001);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops m_can_netdev_ops = {
> > + .ndo_open = m_can_open,
> > + .ndo_stop = m_can_close,
> > + .ndo_start_xmit = m_can_start_xmit,
> > +};
> > +
> > +static int register_m_can_dev(struct net_device *dev)
> > +{
> > + dev->flags |= IFF_ECHO; /* we support local echo */
> > + dev->netdev_ops = &m_can_netdev_ops;
> > +
> > + return register_candev(dev);
> > +}
> > +
> > +static const struct of_device_id m_can_of_table[] = {
> > + { .compatible = "bosch,m_can", .data = NULL },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, m_can_of_table);
> > +
> > +static int m_can_of_parse_mram(struct platform_device *pdev,
> > + struct m_can_priv *priv)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + void __iomem *addr;
> > + u32 out_val[8];
> > + int ret;
> > +
> > + /* message ram could be shared */
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> > + if (!res)
> > + return -ENODEV;
> > +
> > + addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > + if (!addr)
> > + return -ENODEV;
> > +
> > + /* get message ram configuration */
> > + ret = of_property_read_u32_array(np, "mram-cfg",
> > + out_val, sizeof(out_val) / 4);
> > + if (ret) {
> > + dev_err(&pdev->dev, "can not get message ram configuration\n");
> > + return -ENODEV;
> > + }
> > +
> > + priv->mram_base = addr;
> > + priv->mram_off = out_val[0];
> > + priv->sidf_elems = out_val[1];
> > + priv->sidf_off = priv->mram_off;
> > + priv->xidf_elems = out_val[2];
> > + priv->xidf_off = priv->sidf_off + priv->sidf_elems * SIDF_ELEMENT_SIZE;
> > + priv->rxf0_elems = out_val[3] & RXFC_FS_MASK;
> > + priv->rxf0_off = priv->xidf_off + priv->xidf_elems * XIDF_ELEMENT_SIZE;
> > + priv->rxf1_elems = out_val[4] & RXFC_FS_MASK;
> > + priv->rxf1_off = priv->rxf0_off + priv->rxf0_elems * RXF0_ELEMENT_SIZE;
> > + priv->rxb_elems = out_val[5];
> > + priv->rxb_off = priv->rxf1_off + priv->rxf1_elems * RXF1_ELEMENT_SIZE;
> > + priv->txe_elems = out_val[6];
> > + priv->txe_off = priv->rxb_off + priv->rxb_elems * RXB_ELEMENT_SIZE;
> > + priv->txb_elems = out_val[7] & TXBC_NDTB_MASK;
> > + priv->txb_off = priv->txe_off + priv->txe_elems * TXE_ELEMENT_SIZE;
> > +
> > + dev_dbg(&pdev->dev, "mram_base =%p mram_off =0x%x "
> > + "sidf %d xidf %d rxf0 %d rxf1 %d rxb %d txe %d txb %d\n",
> > + priv->mram_base, priv->mram_off, priv->sidf_elems,
> > + priv->xidf_elems, priv->rxf0_elems, priv->rxf1_elems,
> > + priv->rxb_elems, priv->txe_elems, priv->txb_elems);
> > +
> > + return 0;
> > +}
> > +
> > +static int m_can_plat_probe(struct platform_device *pdev)
> > +{
> > + struct net_device *dev;
> > + struct m_can_priv *priv;
> > + struct pinctrl *pinctrl;
> > + struct resource *res;
> > + void __iomem *addr;
> > + struct clk *clk;
> > + int irq, ret;
> > +
> > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > + if (IS_ERR(pinctrl))
> > + dev_warn(&pdev->dev,
> > + "failed to configure pins from driver\n");
> > +
> > + clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(clk)) {
> > + dev_err(&pdev->dev, "no clock find\n");
> > + return PTR_ERR(clk);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
> > + addr = devm_ioremap_resource(&pdev->dev, res);
> > + irq = platform_get_irq(pdev, 0);
> > + if (IS_ERR(addr) || irq < 0)
> > + return -EINVAL;
> > +
> > + /* allocate the m_can device */
> > + dev = alloc_m_can_dev();
> > + if (!dev)
> > + return -ENOMEM;
> > +
> > + priv = netdev_priv(dev);
> > + dev->irq = irq;
> > + priv->base = addr;
> > + priv->device = &pdev->dev;
> > + priv->clk = clk;
> > + priv->can.clock.freq = clk_get_rate(clk);
> > +
> > + ret = m_can_of_parse_mram(pdev, priv);
> > + if (ret)
> > + goto failed_free_dev;
> > +
> > + platform_set_drvdata(pdev, dev);
> > + SET_NETDEV_DEV(dev, &pdev->dev);
> > +
> > + ret = register_m_can_dev(dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> > + KBUILD_MODNAME, ret);
> > + goto failed_free_dev;
> > + }
> > +
> > + devm_can_led_init(dev);
> > +
> > + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> > + KBUILD_MODNAME, priv->base, dev->irq);
> > +
> > + return 0;
> > +
> > +failed_free_dev:
> > + free_m_can_dev(dev);
> > + return ret;
> > +}
> > +
> > +#ifdef CONFIG_PM
>
> please remove and use __maybe_unused unstead
>
Right, will change.
> > +static int m_can_suspend(struct device *dev)
> > +{
> > + struct net_device *ndev = dev_get_drvdata(dev);
> > + struct m_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (netif_running(ndev)) {
> > + netif_stop_queue(ndev);
> > + netif_device_detach(ndev);
> > + }
> > +
> > + /* TODO: enter low power */
> > +
> > + priv->can.state = CAN_STATE_SLEEPING;
> > +
> > + return 0;
> > +}
> > +
> > +static int m_can_resume(struct device *dev)
> > +{
> > + struct net_device *ndev = dev_get_drvdata(dev);
> > + struct m_can_priv *priv = netdev_priv(ndev);
> > +
> > + /* TODO: exit low power */
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + if (netif_running(ndev)) {
> > + netif_device_attach(ndev);
> > + netif_start_queue(ndev);
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static void unregister_m_can_dev(struct net_device *dev)
> > +{
> > + unregister_candev(dev);
> > +}
> > +
> > +static int m_can_plat_remove(struct platform_device *pdev)
> > +{
> > + struct net_device *dev = platform_get_drvdata(pdev);
> > +
> > + unregister_m_can_dev(dev);
> > + platform_set_drvdata(pdev, NULL);
> > +
> > + free_m_can_dev(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops m_can_pmops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
> > +};
> > +
> > +static struct platform_driver m_can_plat_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(m_can_of_table),
> > + .pm = &m_can_pmops,
> > + },
> > + .probe = m_can_plat_probe,
> > + .remove = m_can_plat_remove,
> > +};
> > +
> > +module_platform_driver(m_can_plat_driver);
> > +
> > +MODULE_AUTHOR("Dong Aisheng <b29396@freescale.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
> >
>
> Marc
>
Thanks for the review.
Regards
Dong Aisheng
> --
> 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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
@ 2014-07-02 6:20 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-07-02 6:20 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, wg, devicetree
Hi Marc,
On Tue, Jul 01, 2014 at 12:29:17PM +0200, Marc Kleine-Budde wrote:
> On 06/27/2014 12:00 PM, Dong Aisheng wrote:
> > The patch adds the basic CAN TX/RX function support for Bosch M_CAN controller.
> > For TX, only one dedicated tx buffer is used for sending data.
> > For RX, RXFIFO 0 is used for receiving data to avoid overflow.
> > Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
> >
> > Due to the message ram can be shared by multi m_can instances
> > and the fifo element is configurable which is SoC dependant,
> > the design is to parse the message ram related configuration data from device
> > tree rather than hardcode define it in driver which can make the message
> > ram sharing fully transparent to M_CAN controller driver,
> > then we can gain better driver maintainability and future features upgrade.
> >
> > M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> > and bitrate switch at runtime, however, this patch still does not add the
> > support for these features.
> >
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> > .../devicetree/bindings/net/can/m_can.txt | 29 +
> > drivers/net/can/Kconfig | 5 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/m_can.c | 867 ++++++++++++++++++++
> > 4 files changed, 902 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> > new file mode 100644
> > index 0000000..2b22d02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> > @@ -0,0 +1,29 @@
> > +Bosch MCAN controller Device Tree Bindings
> > +-------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Should be "bosch,m_can" for M_CAN controllers
> > +- reg : physical base address and size of the M_CAN
> > + registers map and message ram
> > +- interrupts : property with a value describing the interrupt
> > + number
> > +- clocks : clocks used by controller
> > +- mram-cfg : message ram configuration data, the format is
> > + <offset sidf_elems xidf_elems rxf1_elems rxb_elems txe_elems txb_elems>
> > + The 'offset' is the address offset inside the message ram. This is usually set
> > + if you're using the shared message ram while the other part is used by another
> > + m_can controller.
> > + The left cell are all the number of each elements inside the message ram.
> > + Please refer to 2.4.1 Message RAM Con.guration in Bosch M_CAN user mannual
> > + for each elements definition.
> > +
> > +Example:
> > +canfd1: canfd@020e8000 {
> > + compatible = "bosch,m_can";
> > + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> > + reg-names = "canfd", "message_ram";
> > + interrupts = <0 114 0x04>;
> > + clocks = <&clks IMX6SX_CLK_CANFD>;
> > + mram-cfg = <0x0 0 0 32 32 32 0 1>;
> > + status = "disabled";
> > +};
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index 4168822..3fd1c4a 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -137,6 +137,11 @@ config CAN_XILINXCAN
> > Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> > Zynq CANPS IP.
> >
> > +config CAN_M_CAN
> > + tristate "Bosch M_CAN devices"
> > + ---help---
> > + Say Y here if you want to support for Bosch M_CAN controller.
> > +
> > source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index 1697f22..69dee2c 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -17,6 +17,7 @@ obj-y += softing/
> > obj-$(CONFIG_CAN_SJA1000) += sja1000/
> > obj-$(CONFIG_CAN_MSCAN) += mscan/
> > obj-$(CONFIG_CAN_C_CAN) += c_can/
> > +obj-$(CONFIG_CAN_M_CAN) += m_can.o
> > obj-$(CONFIG_CAN_CC770) += cc770/
> > obj-$(CONFIG_CAN_AT91) += at91_can.o
> > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> > diff --git a/drivers/net/can/m_can.c b/drivers/net/can/m_can.c
> > new file mode 100644
> > index 0000000..61e9a8e
> > --- /dev/null
> > +++ b/drivers/net/can/m_can.c
> > @@ -0,0 +1,867 @@
> > +/*
> > + * CAN bus driver for Bosch M_CAN controller
> > + *
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > + * Dong Aisheng <b29396@freescale.com>
> > + *
> > + * Bosch M_CAN user manual can be obtained from:
> > + * http://www.bosch-semiconductors.de/media/pdf_1/ipmodules_1/m_can/
> > + * mcan_users_manual_v302.pdf
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/if_arp.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/can/dev.h>
> > +
> > +/* napi related */
> > +#define M_CAN_NAPI_WEIGHT 64
> > +
> > +/* registers definition */
> > +enum m_can_reg {
> > + M_CAN_CREL = 0x0,
> > + M_CAN_ENDN = 0x4,
> > + M_CAN_CUST = 0x8,
> > + M_CAN_FBTP = 0xc,
> > + M_CAN_TEST = 0x10,
> > + M_CAN_RWD = 0x14,
> > + M_CAN_CCCR = 0x18,
> > + M_CAN_BTP = 0x1c,
> > + M_CAN_TSCC = 0x20,
> > + M_CAN_TSCV = 0x24,
> > + M_CAN_TOCC = 0x28,
> > + M_CAN_TOCV = 0x2c,
> > + M_CAN_ECR = 0x40,
> > + M_CAN_PSR = 0x44,
> > + M_CAN_IR = 0x50,
> > + M_CAN_IE = 0x54,
> > + M_CAN_ILS = 0x58,
> > + M_CAN_ILE = 0x5c,
> > + M_CAN_GFC = 0x80,
> > + M_CAN_SIDFC = 0x84,
> > + M_CAN_XIDFC = 0x88,
> > + M_CAN_XIDAM = 0x90,
> > + M_CAN_HPMS = 0x94,
> > + M_CAN_NDAT1 = 0x98,
> > + M_CAN_NDAT2 = 0x9c,
> > + M_CAN_RXF0C = 0xa0,
> > + M_CAN_RXF0S = 0xa4,
> > + M_CAN_RXF0A = 0xa8,
> > + M_CAN_RXBC = 0xac,
> > + M_CAN_RXF1C = 0xb0,
> > + M_CAN_RXF1S = 0xb4,
> > + M_CAN_RXF1A = 0xb8,
> > + M_CAN_RXESC = 0xbc,
> > + M_CAN_TXBC = 0xc0,
> > + M_CAN_TXFQS = 0xc4,
> > + M_CAN_TXESC = 0xc8,
> > + M_CAN_TXBRP = 0xcc,
> > + M_CAN_TXBAR = 0xd0,
> > + M_CAN_TXBCR = 0xd4,
> > + M_CAN_TXBTO = 0xd8,
> > + M_CAN_TXBCF = 0xdc,
> > + M_CAN_TXBTIE = 0xe0,
> > + M_CAN_TXBCIE = 0xe4,
> > + M_CAN_TXEFC = 0xf0,
> > + M_CAN_TXEFS = 0xf4,
> > + M_CAN_TXEFA = 0xf8,
> > +};
> > +
> > +/* CC Control Register(CCCR) */
> > +#define CCCR_CCE BIT(1)
> > +#define CCCR_INIT BIT(0)
> > +
> > +/* Bit Timing & Prescaler Register (BTP) */
> > +#define BTR_BRP_MASK 0x3ff
> > +#define BTR_BRP_SHIFT 16
> > +#define BTR_TSEG1_SHIFT 8
> > +#define BTR_TSEG1_MASK (0x3f << BTR_TSEG1_SHIFT)
> > +#define BTR_TSEG2_SHIFT 4
> > +#define BTR_TSEG2_MASK (0xf << BTR_TSEG2_SHIFT)
> > +#define BTR_SJW_SHIFT 0
> > +#define BTR_SJW_MASK 0xf
> > +
> > +/* Interrupt Register(IR) */
> > +#define IR_ALL_INT 0xffffffff
> > +#define IR_STE BIT(31)
> > +#define IR_FOE BIT(30)
> > +#define IR_ACKE BIT(29)
> > +#define IR_BE BIT(28)
> > +#define IR_CRCE BIT(27)
> > +#define IR_WDI BIT(26)
> > +#define IR_BO BIT(25)
> > +#define IR_EW BIT(24)
> > +#define IR_EP BIT(23)
> > +#define IR_ELO BIT(22)
> > +#define IR_BEU BIT(21)
> > +#define IR_BEC BIT(20)
> > +#define IR_DRX BIT(19)
> > +#define IR_TOO BIT(18)
> > +#define IR_MRAF BIT(17)
> > +#define IR_TSW BIT(16)
> > +#define IR_TEFL BIT(15)
> > +#define IR_TEFF BIT(14)
> > +#define IR_TEFW BIT(13)
> > +#define IR_TEFN BIT(12)
> > +#define IR_TFE BIT(11)
> > +#define IR_TCF BIT(10)
> > +#define IR_TC BIT(9)
> > +#define IR_HPM BIT(8)
> > +#define IR_RF1L BIT(7)
> > +#define IR_RF1F BIT(6)
> > +#define IR_RF1W BIT(5)
> > +#define IR_RF1N BIT(4)
> > +#define IR_RF0L BIT(3)
> > +#define IR_RF0F BIT(2)
> > +#define IR_RF0W BIT(1)
> > +#define IR_RF0N BIT(0)
> > +#define IR_ERR_ALL (IR_STE | IR_FOE | IR_ACKE | IR_BE | IR_CRCE | \
> > + IR_WDI | IR_BO | IR_EW | IR_EP | IR_ELO | IR_BEU | \
> > + IR_BEC | IR_TOO | IR_MRAF | IR_TSW | IR_TEFL | IR_RF1L | \
> > + IR_RF0L)
> > +
> > +/* Rx FIFO 0/1 Configuration (RXF0C/RXF1C) */
> > +#define RXFC_FWM_OFF 24
> > +#define RXFC_FWM_MASK 0x7f
> > +#define RXFC_FWM_1 (1 << RXFC_FWM_OFF)
> > +#define RXFC_FS_OFF 16
> > +#define RXFC_FS_MASK 0x7f
> > +
> > +/* Rx FIFO 0/1 Status (RXF0S/RXF1S) */
> > +#define RXFS_RFL BIT(25)
> > +#define RXFS_FF BIT(24)
> > +#define RXFS_FPI_OFF 16
> > +#define RXFS_FPI_MASK 0x3f0000
> > +#define RXFS_FGI_OFF 8
> > +#define RXFS_FGI_MASK 0x3f00
> > +#define RXFS_FFL_MASK 0x7f
> > +
> > +/* Tx Buffer Configuration(TXBC) */
> > +#define TXBC_NDTB_OFF 16
> > +#define TXBC_NDTB_MASK 0x3f
> > +
> > +/* Tx Buffer Element Size Configuration(TXESC) */
> > +#define TXESC_TBDS_8BYTES 0x0
> > +/* Tx Buffer Element */
> > +#define TX_BUF_XTD BIT(30)
> > +#define TX_BUF_RTR BIT(29)
> > +
> > +/* Rx Buffer Element Size Configuration(TXESC) */
> > +#define M_CAN_RXESC_8BYTES 0x0
> > +/* Tx Buffer Element */
> > +#define RX_BUF_ESI BIT(31)
> > +#define RX_BUF_XTD BIT(30)
> > +#define RX_BUF_RTR BIT(29)
> > +
> > +/* Message RAM Configuration (in bytes) */
> > +#define SIDF_ELEMENT_SIZE 4
> > +#define XIDF_ELEMENT_SIZE 8
> > +#define RXF0_ELEMENT_SIZE 16
> > +#define RXF1_ELEMENT_SIZE 16
> > +#define RXB_ELEMENT_SIZE 16
> > +#define TXE_ELEMENT_SIZE 8
> > +#define TXB_ELEMENT_SIZE 16
> > +
> > +/* m_can private data structure */
> > +struct m_can_priv {
> > + struct can_priv can; /* must be the first member */
> > + struct napi_struct napi;
> > + struct net_device *dev;
> > + struct device *device;
> > + struct clk *clk;
> > + void __iomem *base;
> > + u32 irqstatus;
> > +
> > + /* message ram configuration */
> > + void __iomem *mram_base;
> > + u32 mram_off;
> > + u32 sidf_elems;
> > + u32 sidf_off;
> > + u32 xidf_elems;
> > + u32 xidf_off;
> > + u32 rxf0_elems;
> > + u32 rxf0_off;
> > + u32 rxf1_elems;
> > + u32 rxf1_off;
> > + u32 rxb_elems;
> > + u32 rxb_off;
> > + u32 txe_elems;
> > + u32 txe_off;
> > + u32 txb_elems;
> > + u32 txb_off;
> > +};
> > +
> > +static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)
> > +{
> > + return readl(priv->base + reg);
> > +}
> > +
> > +static inline void m_can_write(const struct m_can_priv *priv,
> > + enum m_can_reg reg, u32 val)
> > +{
> > + writel(val, priv->base + reg);
> > +}
> > +
> > +static inline void m_can_config_endisable(const struct m_can_priv *priv,
> > + bool enable)
> > +{
> > + u32 cccr = m_can_read(priv, M_CAN_CCCR);
> > + u32 timeout = 10, val;
> > +
> > + if (enable) {
> > + /* enable m_can configuration */
> > + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
> > + /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
> > + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
> > + } else {
> > + m_can_write(priv, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
> > + }
> > +
> > + /* there's a delay for module initialization */
> > + val = enable ? CCCR_INIT | CCCR_CCE : 0;
>
> nitpick:
>
> If you declare:
>
> u32 val = 0;
>
> you can use a more readable:
>
> if (enable)
> val = CCCR_INIT | CCCR_CCE;
>
Right, will change to that.
> > + while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE))
> > + != val) {
> > + if (timeout == 0) {
> > + netdev_warn(priv->dev, "Failed to init module\n");
> > + return;
> > + }
> > + timeout--;
> > + udelay(1);
> > + }
> > +}
> > +
> > +static void m_can_enable_all_interrupts(struct m_can_priv *priv, bool enable)
> > +{
> > + m_can_write(priv, M_CAN_ILE, enable ? 0x00000003 : 0x0);
> > +}
>
> Can you make this two functions, I personally don't like to call a
> function: *_enable_* if is actually disabled something. Please make use
> of the defines instead of using 0x3.
>
Got it.
> > +
> > +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + struct net_device_stats *stats = &dev->stats;
> > + struct sk_buff *skb;
> > + struct can_frame *frame;
> > + u32 rxfs, flags, fgi;
> > + u32 num_rx_pkts = 0;
> > +
> > + rxfs = m_can_read(priv, M_CAN_RXF0S);
> > + if (!(rxfs & RXFS_FFL_MASK)) {
> > + netdev_dbg(dev, "no messages in fifo0\n");
> > + return 0;
> > + }
> > +
> > + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> > + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
>
> Please remove the netdev_dbg(), once the driver is stable it should be
> of no use.
>
Got it.
> > + if (rxfs & RXFS_RFL)
> > + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>
> What does that mean? Can you still rx the message if it's lost?
>
It just warns that there's a message lost, but there are still other
message in fifo to receive.
> > +
> > + skb = alloc_can_skb(dev, &frame);
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return -ENOMEM;
>
> Have a look at the user of m_can_do_rx_poll() and how it makes use of
> the return value.
>
Right, thanks for spotting it.
> > + }
> > +
> > + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>
> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
> where it's not a fifo at all.
>
Yes, it is real fifo in the message ram.
> > + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
> > + if (flags & RX_BUF_XTD)
> > + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> > + else
> > + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
> > + netdev_dbg(dev, "R0 0x%x\n", flags);
>
> please remove dbg
> > +
> > + if (flags & RX_BUF_RTR) {
> > + frame->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + flags = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0x4);
> > + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> > + netdev_dbg(dev, "R1 0x%x\n", flags);
>
> please remove
>
> > +
> > + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0x8);
> > + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> > + priv->rxf0_off + fgi * 16 + 0xC);
>
>
> can you create a wrapper function to hide the pointer arithmetics here?
> Somethig like m_can_read_fifo()
>
Yes, i could make a wrapper function for it.
> > + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
> > + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
> > + }
> > +
> > + /* acknowledge rx fifo 0 */
> > + m_can_write(priv, M_CAN_RXF0A, fgi);
> > +
> > + netif_receive_skb(skb);
> > + netdev_dbg(dev, "new packet received\n");
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += frame->can_dlc;
>
> Please move the stats handling in front of netif_receive_skb() as the
> skb and thus frame is not a valid pointer anymore.
>
Good catch!
Will change it.
> > +
> > + can_led_event(dev, CAN_LED_EVENT_RX);
>
> Please move out of the loop so that it is just called once (if a CAN
> frame is rx'ed) per m_can_do_rx_poll().
>
Why that?
The purpose is calling it for each new packet received.
> > +
> > + quota--;
> > + num_rx_pkts++;
> > + rxfs = m_can_read(priv, M_CAN_RXF0S);
> > + };
> > +
> > + return num_rx_pkts;
> > +}
> > +
> > +static int m_can_poll(struct napi_struct *napi, int quota)
> > +{
> > + struct net_device *dev = napi->dev;
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + u32 work_done = 0;
>
> Pleas make work_done a int, as it is retuned by this funtcion.
>
Right
> > + u32 irqstatus;
> > +
> > + irqstatus = m_can_read(priv, M_CAN_IR);
> > + if (irqstatus)
> > + netdev_dbg(dev, "irqstatus updated! new 0x%x old 0x%x rxf0s 0x%x\n",
> > + irqstatus, priv->irqstatus,
> > + m_can_read(priv, M_CAN_RXF0S));
>
> Please remove debug.
>
Got it.
> > +
> > + irqstatus = irqstatus | priv->irqstatus;
> > + if (!irqstatus)
> > + goto end;
> > +
> > + if (irqstatus & IR_RF0N)
> > + /* handle events corresponding to receive message objects */
> > + work_done += m_can_do_rx_poll(dev, (quota - work_done));
> > +
> > + if (work_done < quota) {
> > + napi_complete(napi);
> > + /* enable all IRQs */
> > + m_can_enable_all_interrupts(priv, true);
> > + }
> > +
> > +end:
> > + return work_done;
> > +}
> > +
> > +static irqreturn_t m_can_isr(int irq, void *dev_id)
> > +{
> > + struct net_device *dev = (struct net_device *)dev_id;
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + struct net_device_stats *stats = &dev->stats;
> > + u32 ir;
> > +
> > + ir = m_can_read(priv, M_CAN_IR);
> > + if (!ir)
> > + return IRQ_NONE;
> > +
> > + netdev_dbg(dev, "ir 0x%x rxfs0 0x%x\n", ir,
> > + m_can_read(priv, M_CAN_RXF0S));
>
> please remove
>
Got.
> > +
> > + /* ACK all irqs */
> > + if (ir & IR_ALL_INT)
> > + m_can_write(priv, M_CAN_IR, ir);
> > +
> > + if (ir & IR_ERR_ALL) {
> > + netdev_dbg(dev, "bus error\n");
> > + /* TODO: handle bus error */
> > + }
> > +
> > + /* save irqstatus for later using */
> > + priv->irqstatus = ir;
> > +
> > + /*
> > + * schedule NAPI in case of
> > + * - rx IRQ
> > + * - state change IRQ(TODO)
> > + * - bus error IRQ and bus error reporting (TODO)
> > + */
> > + if (ir & IR_RF0N) {
> > + m_can_enable_all_interrupts(priv, false);
> > + napi_schedule(&priv->napi);
> > + }
> > +
> > + /* FIFO overflow */
> > + if (ir & IR_RF0L) {
> > + dev->stats.rx_over_errors++;
> > + dev->stats.rx_errors++;
> > + }
> > +
> > + /* transmission complete interrupt */
> > + if (ir & IR_TC) {
> > + netdev_dbg(dev, "tx complete\n");
> > + stats->tx_bytes += can_get_echo_skb(dev, 0);
> > + stats->tx_packets++;
> > + can_led_event(dev, CAN_LED_EVENT_TX);
> > + netif_wake_queue(dev);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct can_bittiming_const m_can_bittiming_const = {
> > + .name = KBUILD_MODNAME,
> > + .tseg1_min = 2, /* Time segment 1 = prop_seg + phase_seg1 */
> > + .tseg1_max = 64,
> > + .tseg2_min = 1, /* Time segment 2 = phase_seg2 */
> > + .tseg2_max = 16,
> > + .sjw_max = 16,
> > + .brp_min = 1,
> > + .brp_max = 1024,
> > + .brp_inc = 1,
> > +};
> > +
> > +static int m_can_set_bittiming(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + const struct can_bittiming *bt = &priv->can.bittiming;
> > + u16 brp, sjw, tseg1, tseg2;
> > + u32 reg_btp;
> > +
> > + brp = bt->brp - 1;
> > + sjw = bt->sjw - 1;
> > + tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
> > + tseg2 = bt->phase_seg2 - 1;
> > + reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
> > + (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
> > + m_can_write(priv, M_CAN_BTP, reg_btp);
> > + netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Configure M_CAN chip:
> > + * - set rx buffer/fifo element size
> > + * - configure rx fifo
> > + * - accept non-matching frame into fifo 0
> > + * - configure tx buffer
> > + * - setup bittiming
> > + * - TODO:
> > + * 1) other working modes support like monitor, loopback...
> > + * 2) lec error status report enable
> > + */
> > +static void m_can_chip_config(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + m_can_config_endisable(priv, true);
> > +
> > + /* RX Buffer/FIFO Element Size 8 bytes data field */
> > + m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES);
> > +
> > + /* Accept Non-matching Frames Into FIFO 0 */
> > + m_can_write(priv, M_CAN_GFC, 0x0);
> > +
> > + /* only support one Tx Buffer currently */
> > + m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
> > + (priv->mram_off + priv->txb_off));
> > +
> > + /* only support 8 bytes firstly */
> > + m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
> > +
> > + m_can_write(priv, M_CAN_TXEFC, 0x00010000 |
> > + (priv->mram_off + priv->txe_off));
> > +
> > + /* rx fifo configuration, blocking mode, fifo size 1 */
> > + m_can_write(priv, M_CAN_RXF0C, (priv->rxf0_elems << RXFC_FS_OFF) |
> > + RXFC_FWM_1 | (priv->mram_off + priv->rxf0_off));
> > +
> > + m_can_write(priv, M_CAN_RXF1C, (priv->rxf1_elems << RXFC_FS_OFF) |
> > + RXFC_FWM_1 | (priv->mram_off + priv->rxf1_off));
> > +
> > + /* enable all interrupts */
> > + m_can_write(priv, M_CAN_IR, IR_ALL_INT);
> > + m_can_write(priv, M_CAN_IE, IR_ALL_INT);
> > + m_can_write(priv, M_CAN_ILS, 0xFFFF0000);
> ^^^^^^^^^^
>
> A define would be nice.
>
Will add it.
> > +
> > + /* set bittiming params */
> > + m_can_set_bittiming(dev);
> > +
> > + m_can_config_endisable(priv, false);
> > +}
> > +
> > +static void m_can_start(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + /* basic m_can configuration */
> > + m_can_chip_config(dev);
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + /* enable status change, error and module interrupts */
> > + m_can_enable_all_interrupts(priv, true);
> > +}
> > +
> > +static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
> > +{
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + m_can_start(dev);
> > + netif_wake_queue(dev);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int m_can_get_berr_counter(const struct net_device *dev,
> > + struct can_berr_counter *bec)
> > +{
> > + /* TODO */
> > +
> > + return 0;
> > +}
> > +
> > +static void free_m_can_dev(struct net_device *dev)
> > +{
> > + free_candev(dev);
> > +}
> > +
> > +static struct net_device *alloc_m_can_dev(void)
> > +{
> > + struct net_device *dev;
> > + struct m_can_priv *priv;
> > +
> > + dev = alloc_candev(sizeof(struct m_can_priv), 1);
> > + if (!dev)
> > + return NULL;
> > +
> > + priv = netdev_priv(dev);
> > + netif_napi_add(dev, &priv->napi, m_can_poll, M_CAN_NAPI_WEIGHT);
> > +
> > + priv->dev = dev;
> > + priv->can.bittiming_const = &m_can_bittiming_const;
> > + priv->can.do_set_mode = m_can_set_mode;
> > + priv->can.do_get_berr_counter = m_can_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY |
> > + CAN_CTRLMODE_BERR_REPORTING;
>
> Are you taking care of all ctrlmodes you claim to support here?
>
Yes, those features are marked as TODO in this patch and implemented
in the following two patches in this seris.
> > +
> > + return dev;
> > +}
> > +
> > +static int m_can_open(struct net_device *dev)
> > +{
> > + int err;
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + clk_prepare_enable(priv->clk);
> > +
> > + /* open the can device */
> > + err = open_candev(dev);
> > + if (err) {
> > + netdev_err(dev, "failed to open can device\n");
> > + goto exit_open_fail;
> > + }
> > +
> > + /* register interrupt handler */
> > + err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> > + dev);
> > + if (err < 0) {
> > + netdev_err(dev, "failed to request interrupt\n");
> > + goto exit_irq_fail;
> > + }
> > +
> > + /* start the m_can controller */
> > + m_can_start(dev);
> > +
> > + can_led_event(dev, CAN_LED_EVENT_OPEN);
> > + napi_enable(&priv->napi);
> > + netif_start_queue(dev);
> > +
> > + return 0;
> > +
> > +exit_irq_fail:
> > + close_candev(dev);
> > +exit_open_fail:
> > + clk_disable_unprepare(priv->clk);
> > + return err;
> > +}
> > +
> > +static void m_can_stop(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + /* disable all interrupts */
> > + m_can_enable_all_interrupts(priv, false);
> > +
> > + /* set the state as STOPPED */
> > + priv->can.state = CAN_STATE_STOPPED;
> > +}
> > +
> > +static int m_can_close(struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > +
> > + netif_stop_queue(dev);
> > + napi_disable(&priv->napi);
> > + m_can_stop(dev);
> > + free_irq(dev->irq, dev);
> > + close_candev(dev);
> > + can_led_event(dev, CAN_LED_EVENT_STOP);
> > +
> > + return 0;
> > +}
> > +
> > +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> > + struct net_device *dev)
> > +{
> > + struct m_can_priv *priv = netdev_priv(dev);
> > + struct can_frame *frame = (struct can_frame *)skb->data;
> > + u32 flags = 0, id;
> > +
> > + if (can_dropped_invalid_skb(dev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + netif_stop_queue(dev);
> > +
> > + if (frame->can_id & CAN_RTR_FLAG)
> > + flags |= TX_BUF_RTR;
> > +
> > + if (frame->can_id & CAN_EFF_FLAG) {
> > + id = frame->can_id & CAN_EFF_MASK;
> > + flags |= TX_BUF_XTD;
> > + } else {
> > + id = ((frame->can_id & CAN_SFF_MASK) << 18);
> > + }
> > +
> > + /* message ram configuration */
> > + writel(id | flags,
> > + priv->mram_base + priv->mram_off + priv->txb_off);
> > + writel(frame->can_dlc << 16,
> > + priv->mram_base + priv->mram_off + priv->txb_off + 0x4);
> > + writel(*(u32 *)(frame->data + 0),
> > + priv->mram_base + priv->mram_off + priv->txb_off + 0x8);
> > + writel(*(u32 *)(frame->data + 4),
> > + priv->mram_base + priv->mram_off + priv->txb_off + 0xc);
>
> Here an inline function to hide the details of pointer arithmetics would
> be nice.
>
Yes, will try to do it in v2.
> > +
> > + can_put_echo_skb(skb, dev, 0);
> > +
> > + /* enable first TX buffer to start transfer */
> > + m_can_write(priv, M_CAN_TXBTIE, 0x00000001);
> > + m_can_write(priv, M_CAN_TXBAR, 0x00000001);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops m_can_netdev_ops = {
> > + .ndo_open = m_can_open,
> > + .ndo_stop = m_can_close,
> > + .ndo_start_xmit = m_can_start_xmit,
> > +};
> > +
> > +static int register_m_can_dev(struct net_device *dev)
> > +{
> > + dev->flags |= IFF_ECHO; /* we support local echo */
> > + dev->netdev_ops = &m_can_netdev_ops;
> > +
> > + return register_candev(dev);
> > +}
> > +
> > +static const struct of_device_id m_can_of_table[] = {
> > + { .compatible = "bosch,m_can", .data = NULL },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, m_can_of_table);
> > +
> > +static int m_can_of_parse_mram(struct platform_device *pdev,
> > + struct m_can_priv *priv)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + void __iomem *addr;
> > + u32 out_val[8];
> > + int ret;
> > +
> > + /* message ram could be shared */
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> > + if (!res)
> > + return -ENODEV;
> > +
> > + addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > + if (!addr)
> > + return -ENODEV;
> > +
> > + /* get message ram configuration */
> > + ret = of_property_read_u32_array(np, "mram-cfg",
> > + out_val, sizeof(out_val) / 4);
> > + if (ret) {
> > + dev_err(&pdev->dev, "can not get message ram configuration\n");
> > + return -ENODEV;
> > + }
> > +
> > + priv->mram_base = addr;
> > + priv->mram_off = out_val[0];
> > + priv->sidf_elems = out_val[1];
> > + priv->sidf_off = priv->mram_off;
> > + priv->xidf_elems = out_val[2];
> > + priv->xidf_off = priv->sidf_off + priv->sidf_elems * SIDF_ELEMENT_SIZE;
> > + priv->rxf0_elems = out_val[3] & RXFC_FS_MASK;
> > + priv->rxf0_off = priv->xidf_off + priv->xidf_elems * XIDF_ELEMENT_SIZE;
> > + priv->rxf1_elems = out_val[4] & RXFC_FS_MASK;
> > + priv->rxf1_off = priv->rxf0_off + priv->rxf0_elems * RXF0_ELEMENT_SIZE;
> > + priv->rxb_elems = out_val[5];
> > + priv->rxb_off = priv->rxf1_off + priv->rxf1_elems * RXF1_ELEMENT_SIZE;
> > + priv->txe_elems = out_val[6];
> > + priv->txe_off = priv->rxb_off + priv->rxb_elems * RXB_ELEMENT_SIZE;
> > + priv->txb_elems = out_val[7] & TXBC_NDTB_MASK;
> > + priv->txb_off = priv->txe_off + priv->txe_elems * TXE_ELEMENT_SIZE;
> > +
> > + dev_dbg(&pdev->dev, "mram_base =%p mram_off =0x%x "
> > + "sidf %d xidf %d rxf0 %d rxf1 %d rxb %d txe %d txb %d\n",
> > + priv->mram_base, priv->mram_off, priv->sidf_elems,
> > + priv->xidf_elems, priv->rxf0_elems, priv->rxf1_elems,
> > + priv->rxb_elems, priv->txe_elems, priv->txb_elems);
> > +
> > + return 0;
> > +}
> > +
> > +static int m_can_plat_probe(struct platform_device *pdev)
> > +{
> > + struct net_device *dev;
> > + struct m_can_priv *priv;
> > + struct pinctrl *pinctrl;
> > + struct resource *res;
> > + void __iomem *addr;
> > + struct clk *clk;
> > + int irq, ret;
> > +
> > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > + if (IS_ERR(pinctrl))
> > + dev_warn(&pdev->dev,
> > + "failed to configure pins from driver\n");
> > +
> > + clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(clk)) {
> > + dev_err(&pdev->dev, "no clock find\n");
> > + return PTR_ERR(clk);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
> > + addr = devm_ioremap_resource(&pdev->dev, res);
> > + irq = platform_get_irq(pdev, 0);
> > + if (IS_ERR(addr) || irq < 0)
> > + return -EINVAL;
> > +
> > + /* allocate the m_can device */
> > + dev = alloc_m_can_dev();
> > + if (!dev)
> > + return -ENOMEM;
> > +
> > + priv = netdev_priv(dev);
> > + dev->irq = irq;
> > + priv->base = addr;
> > + priv->device = &pdev->dev;
> > + priv->clk = clk;
> > + priv->can.clock.freq = clk_get_rate(clk);
> > +
> > + ret = m_can_of_parse_mram(pdev, priv);
> > + if (ret)
> > + goto failed_free_dev;
> > +
> > + platform_set_drvdata(pdev, dev);
> > + SET_NETDEV_DEV(dev, &pdev->dev);
> > +
> > + ret = register_m_can_dev(dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> > + KBUILD_MODNAME, ret);
> > + goto failed_free_dev;
> > + }
> > +
> > + devm_can_led_init(dev);
> > +
> > + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> > + KBUILD_MODNAME, priv->base, dev->irq);
> > +
> > + return 0;
> > +
> > +failed_free_dev:
> > + free_m_can_dev(dev);
> > + return ret;
> > +}
> > +
> > +#ifdef CONFIG_PM
>
> please remove and use __maybe_unused unstead
>
Right, will change.
> > +static int m_can_suspend(struct device *dev)
> > +{
> > + struct net_device *ndev = dev_get_drvdata(dev);
> > + struct m_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (netif_running(ndev)) {
> > + netif_stop_queue(ndev);
> > + netif_device_detach(ndev);
> > + }
> > +
> > + /* TODO: enter low power */
> > +
> > + priv->can.state = CAN_STATE_SLEEPING;
> > +
> > + return 0;
> > +}
> > +
> > +static int m_can_resume(struct device *dev)
> > +{
> > + struct net_device *ndev = dev_get_drvdata(dev);
> > + struct m_can_priv *priv = netdev_priv(ndev);
> > +
> > + /* TODO: exit low power */
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + if (netif_running(ndev)) {
> > + netif_device_attach(ndev);
> > + netif_start_queue(ndev);
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static void unregister_m_can_dev(struct net_device *dev)
> > +{
> > + unregister_candev(dev);
> > +}
> > +
> > +static int m_can_plat_remove(struct platform_device *pdev)
> > +{
> > + struct net_device *dev = platform_get_drvdata(pdev);
> > +
> > + unregister_m_can_dev(dev);
> > + platform_set_drvdata(pdev, NULL);
> > +
> > + free_m_can_dev(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops m_can_pmops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
> > +};
> > +
> > +static struct platform_driver m_can_plat_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(m_can_of_table),
> > + .pm = &m_can_pmops,
> > + },
> > + .probe = m_can_plat_probe,
> > + .remove = m_can_plat_remove,
> > +};
> > +
> > +module_platform_driver(m_can_plat_driver);
> > +
> > +MODULE_AUTHOR("Dong Aisheng <b29396@freescale.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
> >
>
> Marc
>
Thanks for the review.
Regards
Dong Aisheng
> --
> 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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-02 6:20 ` Dong Aisheng
(?)
@ 2014-07-02 7:57 ` Marc Kleine-Budde
2014-07-02 6:33 ` Dong Aisheng
-1 siblings, 1 reply; 35+ messages in thread
From: Marc Kleine-Budde @ 2014-07-02 7:57 UTC (permalink / raw)
To: Dong Aisheng; +Cc: linux-can, netdev, wg, devicetree
[-- Attachment #1: Type: text/plain, Size: 3791 bytes --]
On 07/02/2014 08:20 AM, Dong Aisheng wrote:
[...]
>>> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
>>> +{
>>> + struct m_can_priv *priv = netdev_priv(dev);
>>> + struct net_device_stats *stats = &dev->stats;
>>> + struct sk_buff *skb;
>>> + struct can_frame *frame;
>>> + u32 rxfs, flags, fgi;
>>> + u32 num_rx_pkts = 0;
>>> +
>>> + rxfs = m_can_read(priv, M_CAN_RXF0S);
>>> + if (!(rxfs & RXFS_FFL_MASK)) {
>>> + netdev_dbg(dev, "no messages in fifo0\n");
>>> + return 0;
>>> + }
>>> +
>>> + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
>>> + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
>>
>> Please remove the netdev_dbg(), once the driver is stable it should be
>> of no use.
>>
>
> Got it.
>
>>> + if (rxfs & RXFS_RFL)
>>> + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>>
>> What does that mean? Can you still rx the message if it's lost?
>>
>
> It just warns that there's a message lost, but there are still other
> message in fifo to receive.
>
>>> +
>>> + skb = alloc_can_skb(dev, &frame);
>>> + if (!skb) {
>>> + stats->rx_dropped++;
>>> + return -ENOMEM;
>>
>> Have a look at the user of m_can_do_rx_poll() and how it makes use of
>> the return value.
>>
>
> Right, thanks for spotting it.
>
>>> + }
>>> +
>>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>>
>> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
>> where it's not a fifo at all.
>>
>
> Yes, it is real fifo in the message ram.
>
>>> + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
>>> + if (flags & RX_BUF_XTD)
>>> + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
>>> + else
>>> + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
>>> + netdev_dbg(dev, "R0 0x%x\n", flags);
>>
>> please remove dbg
>>> +
>>> + if (flags & RX_BUF_RTR) {
>>> + frame->can_id |= CAN_RTR_FLAG;
>>> + } else {
>>> + flags = readl(priv->mram_base +
>>> + priv->rxf0_off + fgi * 16 + 0x4);
>>> + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
>>> + netdev_dbg(dev, "R1 0x%x\n", flags);
>>
>> please remove
>>
>>> +
>>> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
>>> + priv->rxf0_off + fgi * 16 + 0x8);
>>> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
>>> + priv->rxf0_off + fgi * 16 + 0xC);
>>
>>
>> can you create a wrapper function to hide the pointer arithmetics here?
>> Somethig like m_can_read_fifo()
>>
>
> Yes, i could make a wrapper function for it.
>
>>> + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
>>> + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
>>> + }
>>> +
>>> + /* acknowledge rx fifo 0 */
>>> + m_can_write(priv, M_CAN_RXF0A, fgi);
>>> +
>>> + netif_receive_skb(skb);
>>> + netdev_dbg(dev, "new packet received\n");
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += frame->can_dlc;
>>
>> Please move the stats handling in front of netif_receive_skb() as the
>> skb and thus frame is not a valid pointer anymore.
>>
>
> Good catch!
> Will change it.
>
>>> +
>>> + can_led_event(dev, CAN_LED_EVENT_RX);
>>
>> Please move out of the loop so that it is just called once (if a CAN
>> frame is rx'ed) per m_can_do_rx_poll().
> Why that?
> The purpose is calling it for each new packet received.
It will only trigger LED blinking, and tglx pointed out, that we don't
need the overhead of calling it for every CAN frame.
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: 242 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-07-02 7:57 ` Marc Kleine-Budde
@ 2014-07-02 6:33 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-07-02 6:33 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, wg, devicetree
On Wed, Jul 02, 2014 at 09:57:50AM +0200, Marc Kleine-Budde wrote:
> On 07/02/2014 08:20 AM, Dong Aisheng wrote:
> [...]
>
> >>> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >>> +{
> >>> + struct m_can_priv *priv = netdev_priv(dev);
> >>> + struct net_device_stats *stats = &dev->stats;
> >>> + struct sk_buff *skb;
> >>> + struct can_frame *frame;
> >>> + u32 rxfs, flags, fgi;
> >>> + u32 num_rx_pkts = 0;
> >>> +
> >>> + rxfs = m_can_read(priv, M_CAN_RXF0S);
> >>> + if (!(rxfs & RXFS_FFL_MASK)) {
> >>> + netdev_dbg(dev, "no messages in fifo0\n");
> >>> + return 0;
> >>> + }
> >>> +
> >>> + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> >>> + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
> >>
> >> Please remove the netdev_dbg(), once the driver is stable it should be
> >> of no use.
> >>
> >
> > Got it.
> >
> >>> + if (rxfs & RXFS_RFL)
> >>> + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >>
> >> What does that mean? Can you still rx the message if it's lost?
> >>
> >
> > It just warns that there's a message lost, but there are still other
> > message in fifo to receive.
> >
> >>> +
> >>> + skb = alloc_can_skb(dev, &frame);
> >>> + if (!skb) {
> >>> + stats->rx_dropped++;
> >>> + return -ENOMEM;
> >>
> >> Have a look at the user of m_can_do_rx_poll() and how it makes use of
> >> the return value.
> >>
> >
> > Right, thanks for spotting it.
> >
> >>> + }
> >>> +
> >>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> >>
> >> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
> >> where it's not a fifo at all.
> >>
> >
> > Yes, it is real fifo in the message ram.
> >
> >>> + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
> >>> + if (flags & RX_BUF_XTD)
> >>> + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> >>> + else
> >>> + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
> >>> + netdev_dbg(dev, "R0 0x%x\n", flags);
> >>
> >> please remove dbg
> >>> +
> >>> + if (flags & RX_BUF_RTR) {
> >>> + frame->can_id |= CAN_RTR_FLAG;
> >>> + } else {
> >>> + flags = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0x4);
> >>> + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> >>> + netdev_dbg(dev, "R1 0x%x\n", flags);
> >>
> >> please remove
> >>
> >>> +
> >>> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0x8);
> >>> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0xC);
> >>
> >>
> >> can you create a wrapper function to hide the pointer arithmetics here?
> >> Somethig like m_can_read_fifo()
> >>
> >
> > Yes, i could make a wrapper function for it.
> >
> >>> + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
> >>> + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
> >>> + }
> >>> +
> >>> + /* acknowledge rx fifo 0 */
> >>> + m_can_write(priv, M_CAN_RXF0A, fgi);
> >>> +
> >>> + netif_receive_skb(skb);
> >>> + netdev_dbg(dev, "new packet received\n");
> >>> +
> >>> + stats->rx_packets++;
> >>> + stats->rx_bytes += frame->can_dlc;
> >>
> >> Please move the stats handling in front of netif_receive_skb() as the
> >> skb and thus frame is not a valid pointer anymore.
> >>
> >
> > Good catch!
> > Will change it.
> >
> >>> +
> >>> + can_led_event(dev, CAN_LED_EVENT_RX);
> >>
> >> Please move out of the loop so that it is just called once (if a CAN
> >> frame is rx'ed) per m_can_do_rx_poll().
>
> > Why that?
> > The purpose is calling it for each new packet received.
>
> It will only trigger LED blinking, and tglx pointed out, that we don't
> need the overhead of calling it for every CAN frame.
>
Okay, got it, thanks for this information.
Regards
Dong Aisheng
> 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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
@ 2014-07-02 6:33 ` Dong Aisheng
0 siblings, 0 replies; 35+ messages in thread
From: Dong Aisheng @ 2014-07-02 6:33 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, wg, devicetree
On Wed, Jul 02, 2014 at 09:57:50AM +0200, Marc Kleine-Budde wrote:
> On 07/02/2014 08:20 AM, Dong Aisheng wrote:
> [...]
>
> >>> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >>> +{
> >>> + struct m_can_priv *priv = netdev_priv(dev);
> >>> + struct net_device_stats *stats = &dev->stats;
> >>> + struct sk_buff *skb;
> >>> + struct can_frame *frame;
> >>> + u32 rxfs, flags, fgi;
> >>> + u32 num_rx_pkts = 0;
> >>> +
> >>> + rxfs = m_can_read(priv, M_CAN_RXF0S);
> >>> + if (!(rxfs & RXFS_FFL_MASK)) {
> >>> + netdev_dbg(dev, "no messages in fifo0\n");
> >>> + return 0;
> >>> + }
> >>> +
> >>> + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> >>> + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
> >>
> >> Please remove the netdev_dbg(), once the driver is stable it should be
> >> of no use.
> >>
> >
> > Got it.
> >
> >>> + if (rxfs & RXFS_RFL)
> >>> + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >>
> >> What does that mean? Can you still rx the message if it's lost?
> >>
> >
> > It just warns that there's a message lost, but there are still other
> > message in fifo to receive.
> >
> >>> +
> >>> + skb = alloc_can_skb(dev, &frame);
> >>> + if (!skb) {
> >>> + stats->rx_dropped++;
> >>> + return -ENOMEM;
> >>
> >> Have a look at the user of m_can_do_rx_poll() and how it makes use of
> >> the return value.
> >>
> >
> > Right, thanks for spotting it.
> >
> >>> + }
> >>> +
> >>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> >>
> >> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
> >> where it's not a fifo at all.
> >>
> >
> > Yes, it is real fifo in the message ram.
> >
> >>> + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
> >>> + if (flags & RX_BUF_XTD)
> >>> + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> >>> + else
> >>> + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
> >>> + netdev_dbg(dev, "R0 0x%x\n", flags);
> >>
> >> please remove dbg
> >>> +
> >>> + if (flags & RX_BUF_RTR) {
> >>> + frame->can_id |= CAN_RTR_FLAG;
> >>> + } else {
> >>> + flags = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0x4);
> >>> + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> >>> + netdev_dbg(dev, "R1 0x%x\n", flags);
> >>
> >> please remove
> >>
> >>> +
> >>> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0x8);
> >>> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0xC);
> >>
> >>
> >> can you create a wrapper function to hide the pointer arithmetics here?
> >> Somethig like m_can_read_fifo()
> >>
> >
> > Yes, i could make a wrapper function for it.
> >
> >>> + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
> >>> + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
> >>> + }
> >>> +
> >>> + /* acknowledge rx fifo 0 */
> >>> + m_can_write(priv, M_CAN_RXF0A, fgi);
> >>> +
> >>> + netif_receive_skb(skb);
> >>> + netdev_dbg(dev, "new packet received\n");
> >>> +
> >>> + stats->rx_packets++;
> >>> + stats->rx_bytes += frame->can_dlc;
> >>
> >> Please move the stats handling in front of netif_receive_skb() as the
> >> skb and thus frame is not a valid pointer anymore.
> >>
> >
> > Good catch!
> > Will change it.
> >
> >>> +
> >>> + can_led_event(dev, CAN_LED_EVENT_RX);
> >>
> >> Please move out of the loop so that it is just called once (if a CAN
> >> frame is rx'ed) per m_can_do_rx_poll().
>
> > Why that?
> > The purpose is calling it for each new packet received.
>
> It will only trigger LED blinking, and tglx pointed out, that we don't
> need the overhead of calling it for every CAN frame.
>
Okay, got it, thanks for this information.
Regards
Dong Aisheng
> 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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
2014-06-27 10:00 ` Dong Aisheng
` (3 preceding siblings ...)
(?)
@ 2014-07-01 10:33 ` Marc Kleine-Budde
-1 siblings, 0 replies; 35+ messages in thread
From: Marc Kleine-Budde @ 2014-07-01 10:33 UTC (permalink / raw)
To: Dong Aisheng, linux-can; +Cc: netdev, wg, devicetree
[-- Attachment #1: Type: text/plain, Size: 684 bytes --]
On 06/27/2014 12:00 PM, Dong Aisheng wrote:
> +static int m_can_close(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + netif_stop_queue(dev);
> + napi_disable(&priv->napi);
> + m_can_stop(dev);
> + free_irq(dev->irq, dev);
> + close_candev(dev);
> + can_led_event(dev, CAN_LED_EVENT_STOP);
You forgot to turn of the clock.
> +
> + return 0;
> +}
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: 242 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread