[PATCH 1/7] can: m_can: fix possible sleep in napi poll

[PATCH 1/7] can: m_can: fix possible sleep in napi poll

[Dong Aisheng]

The m_can_get_berr_counter function can sleep and it may be called in napi poll function.
Rework it to fix the following warning.
root@imx6qdlsolo:~# cangen can0 -f -L 12 -D 112233445566778899001122
[ 1846.017565] m_can 20e8000.can can0: entered error warning state
[ 1846.023551] ------------[ cut here ]------------
[ 1846.028216] WARNING: CPU: 0 PID: 560 at kernel/locking/mutex.c:867 mutex_trylock+0x218/0x23c()
[ 1846.036889] DEBUG_LOCKS_WARN_ON(in_interrupt())
[ 1846.041263] Modules linked in:
[ 1846.044594] CPU: 0 PID: 560 Comm: cangen Not tainted 3.17.0-rc4-next-20140915-00010-g032d018-dirty #477
[ 1846.054033] Backtrace:
[ 1846.056557] [<80012448>] (dump_backtrace) from [<80012728>] (show_stack+0x18/0x1c)
[ 1846.064180]  r6:809a07ec r5:809a07ec r4:00000000 r3:00000000
[ 1846.069966] [<80012710>] (show_stack) from [<806c9ee0>] (dump_stack+0x8c/0xa4)
[ 1846.077264] [<806c9e54>] (dump_stack) from [<8002aa78>] (warn_slowpath_common+0x70/0x94)
[ 1846.085403]  r6:806cd1b0 r5:00000009 r4:be1d5c20 r3:be07b0c0
[ 1846.091204] [<8002aa08>] (warn_slowpath_common) from [<8002aad4>] (warn_slowpath_fmt+0x38/0x40)
[ 1846.099951]  r8:8119106c r7:80515aa4 r6:be027000 r5:00000001 r4:809d1df4
[ 1846.106830] [<8002aaa0>] (warn_slowpath_fmt) from [<806cd1b0>] (mutex_trylock+0x218/0x23c)
[ 1846.115141]  r3:80851c88 r2:8084fb74
[ 1846.118804] [<806ccf98>] (mutex_trylock) from [<80515aa4>] (clk_prepare_lock+0x14/0xf4)
[ 1846.126859]  r8:00000040 r7:be1d5cec r6:be027000 r5:be255800 r4:be027000
[ 1846.133737] [<80515a90>] (clk_prepare_lock) from [<80517660>] (clk_prepare+0x14/0x2c)
[ 1846.141583]  r5:be255800 r4:be027000
[ 1846.145272] [<8051764c>] (clk_prepare) from [<8041ff14>] (m_can_get_berr_counter+0x20/0xd4)
[ 1846.153672]  r4:be255800 r3:be07b0c0
[ 1846.157325] [<8041fef4>] (m_can_get_berr_counter) from [<80420428>] (m_can_poll+0x310/0x8fc)
[ 1846.165809]  r7:bd4dc540 r6:00000744 r5:11300000 r4:be255800
[ 1846.171590] [<80420118>] (m_can_poll) from [<8056a468>] (net_rx_action+0xcc/0x1b4)
[ 1846.179204]  r10:00000101 r9:be255ebc r8:00000040 r7:be7c3208 r6:8097c100 r5:be7c3200
[ 1846.187192]  r4:0000012c
[ 1846.189779] [<8056a39c>] (net_rx_action) from [<8002deec>] (__do_softirq+0xfc/0x2c4)
[ 1846.197568]  r10:00000101 r9:8097c088 r8:00000003 r7:8097c080 r6:40000001 r5:8097c08c
[ 1846.205559]  r4:00000020
[ 1846.208144] [<8002ddf0>] (__do_softirq) from [<8002e194>] (do_softirq+0x7c/0x88)
[ 1846.215588]  r10:00000000 r9:bd516a60 r8:be18ce00 r7:00000000 r6:be255800 r5:8056c0ec
[ 1846.223578]  r4:60000093
[ 1846.226163] [<8002e118>] (do_softirq) from [<8002e288>] (__local_bh_enable_ip+0xe8/0x10c)
[ 1846.234386]  r4:00000200 r3:be1d4000
[ 1846.238036] [<8002e1a0>] (__local_bh_enable_ip) from [<8056c108>] (__dev_queue_xmit+0x314/0x6b0)
[ 1846.246868]  r6:be255800 r5:bd516a00 r4:00000000 r3:be07b0c0
[ 1846.252645] [<8056bdf4>] (__dev_queue_xmit) from [<8056c4b8>] (dev_queue_xmit+0x14/0x18)

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/m_can/m_can.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 10d571e..8692848 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -481,11 +481,23 @@ static int m_can_handle_lec_err(struct net_device *dev,
 	return 1;
 }
 
+static int __m_can_get_berr_counter(const struct net_device *dev,
+				    struct can_berr_counter *bec)
+{
+	struct m_can_priv *priv = netdev_priv(dev);
+	unsigned int ecr;
+
+	ecr = m_can_read(priv, M_CAN_ECR);
+	bec->rxerr = (ecr & ECR_REC_MASK) >> ECR_REC_SHIFT;
+	bec->txerr = ecr & ECR_TEC_MASK;
+
+	return 0;
+}
+
 static int m_can_get_berr_counter(const struct net_device *dev,
 				  struct can_berr_counter *bec)
 {
 	struct m_can_priv *priv = netdev_priv(dev);
-	unsigned int ecr;
 	int err;
 
 	err = clk_prepare_enable(priv->hclk);
@@ -498,9 +510,7 @@ static int m_can_get_berr_counter(const struct net_device *dev,
 		return err;
 	}
 
-	ecr = m_can_read(priv, M_CAN_ECR);
-	bec->rxerr = (ecr & ECR_REC_MASK) >> ECR_REC_SHIFT;
-	bec->txerr = ecr & ECR_TEC_MASK;
+	__m_can_get_berr_counter(dev, bec);
 
 	clk_disable_unprepare(priv->cclk);
 	clk_disable_unprepare(priv->hclk);
@@ -544,7 +554,7 @@ static int m_can_handle_state_change(struct net_device *dev,
 	if (unlikely(!skb))
 		return 0;
 
-	m_can_get_berr_counter(dev, &bec);
+	__m_can_get_berr_counter(dev, &bec);
 
 	switch (new_state) {
 	case CAN_STATE_ERROR_ACTIVE:
-- 
1.9.1

[PATCH 2/7] can: m_can: fix the incorrect error messages

[Dong Aisheng]

Fix a few error messages.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/m_can/m_can.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 8692848..2784423 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -606,14 +606,14 @@ static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
 
 	if ((psr & PSR_EP) &&
 	    (priv->can.state != CAN_STATE_ERROR_PASSIVE)) {
-		netdev_dbg(dev, "entered error warning state\n");
+		netdev_dbg(dev, "entered error passive state\n");
 		work_done += m_can_handle_state_change(dev,
 						       CAN_STATE_ERROR_PASSIVE);
 	}
 
 	if ((psr & PSR_BO) &&
 	    (priv->can.state != CAN_STATE_BUS_OFF)) {
-		netdev_dbg(dev, "entered error warning state\n");
+		netdev_dbg(dev, "entered error bus off state\n");
 		work_done += m_can_handle_state_change(dev,
 						       CAN_STATE_BUS_OFF);
 	}
@@ -625,7 +625,7 @@ static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)
 {
 	if (irqstatus & IR_WDI)
 		netdev_err(dev, "Message RAM Watchdog event due to missing READY\n");
-	if (irqstatus & IR_BEU)
+	if (irqstatus & IR_ELO)
 		netdev_err(dev, "Error Logging Overflow\n");
 	if (irqstatus & IR_BEU)
 		netdev_err(dev, "Bit Error Uncorrected\n");
-- 
1.9.1

[PATCH 3/7] can: m_can: add .ndo_change_mtu function

[Dong Aisheng]

Use common can_change_mtu function.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/m_can/m_can.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 2784423..e4ef146 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1002,6 +1002,7 @@ 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,
+	.ndo_change_mtu = can_change_mtu,
 };
 
 static int register_m_can_dev(struct net_device *dev)
-- 
1.9.1

[PATCH 4/7] can: m_can: add a bit delay after setting CCCR_INIT bit

[Dong Aisheng]

The spec mentions there may be a delay until the value written to
INIT can be read back due to the synchronization mechanism between the
two clock domains. But it does not indicate the exact clock cycles needed.
The 5us delay is a test value and seems ok.

Without the delay, CCCR.CCE bit may fail to be set and then the
initialization fail sometimes when do repeatly up and down.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/m_can/m_can.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index e4ef146..6160b9c 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -296,6 +296,7 @@ static inline void m_can_config_endisable(const struct m_can_priv *priv,
 	if (enable) {
 		/* enable m_can configuration */
 		m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
+		udelay(5);
 		/* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
 		m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
 	} else {
-- 
1.9.1

[PATCH 5/7] can: clear ctrlmode when close candev

[Dong Aisheng]

Currently priv->ctrlmode is not cleared when close_candev, so next time
the driver will still use this value to set controller even user
does not set any ctrl mode.
e.g.
Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on
Controller will be in loopback mode
Step 2. ip link set can0 down
Step 3. ip link set can0 up type can0 bitrate 1000000
Controller will still be set to loopback mode in driver due to saved
priv->ctrlmode.

This patch clears priv->ctrlmode when the CAN interface is closed,
and set it to correct mode according to next user setting.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 02492d2..1fce485 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -671,6 +671,7 @@ void close_candev(struct net_device *dev)
 
 	del_timer_sync(&priv->restart_timer);
 	can_flush_echo_skb(dev);
+	priv->ctrlmode = 0;
 }
 EXPORT_SYMBOL_GPL(close_candev);
 
-- 
1.9.1

[PATCH 6/7] can: m_can: update to support CAN FD features

[Dong Aisheng]

Bosch M_CAN is CAN FD capable device.
This patch implements the CAN FD features include up to 64 bytes
payload and bitrate switch function.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/m_can/m_can.c | 138 +++++++++++++++++++++++++++++++++---------
 1 file changed, 110 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 6160b9c..219e0e3 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -105,14 +105,36 @@ enum m_can_mram_cfg {
 	MRAM_CFG_NUM,
 };
 
+/* Fast Bit Timing & Prescaler Register (FBTP) */
+#define FBTR_FBRP_MASK		0x1f
+#define FBTR_FBRP_SHIFT		16
+#define FBTR_FTSEG1_SHIFT	8
+#define FBTR_FTSEG1_MASK	(0xf << FBTR_FTSEG1_SHIFT)
+#define FBTR_FTSEG2_SHIFT	4
+#define FBTR_FTSEG2_MASK	(0x7 << FBTR_FTSEG2_SHIFT)
+#define FBTR_FSJW_SHIFT		0
+#define FBTR_FSJW_MASK		0x3
+
 /* Test Register (TEST) */
 #define TEST_LBCK	BIT(4)
 
 /* CC Control Register(CCCR) */
-#define CCCR_TEST	BIT(7)
-#define CCCR_MON	BIT(5)
-#define CCCR_CCE	BIT(1)
-#define CCCR_INIT	BIT(0)
+#define CCCR_TEST		BIT(7)
+#define CCCR_CMR_MASK		0x3
+#define CCCR_CMR_SHIFT		10
+#define CCCR_CMR_CANFD		0x1
+#define CCCR_CMR_CANFD_BRS	0x2
+#define CCCR_CMR_CAN		0x3
+#define CCCR_CME_MASK		0x3
+#define CCCR_CME_SHIFT		8
+#define CCCR_CME_CAN		0
+#define CCCR_CME_CANFD		0x1
+#define CCCR_CME_CANFD_BRS	0x2
+#define CCCR_TEST		BIT(7)
+#define CCCR_MON		BIT(5)
+#define CCCR_CCE		BIT(1)
+#define CCCR_INIT		BIT(0)
+#define CCCR_CANFD		0x10
 
 /* Bit Timing & Prescaler Register (BTP) */
 #define BTR_BRP_MASK		0x3ff
@@ -204,6 +226,7 @@ enum m_can_mram_cfg {
 
 /* Rx Buffer / FIFO Element Size Configuration (RXESC) */
 #define M_CAN_RXESC_8BYTES	0x0
+#define M_CAN_RXESC_64BYTES	0x777
 
 /* Tx Buffer Configuration(TXBC) */
 #define TXBC_NDTB_OFF		16
@@ -211,6 +234,7 @@ enum m_can_mram_cfg {
 
 /* Tx Buffer Element Size Configuration(TXESC) */
 #define TXESC_TBDS_8BYTES	0x0
+#define TXESC_TBDS_64BYTES	0x7
 
 /* Tx Event FIFO Con.guration (TXEFC) */
 #define TXEFC_EFS_OFF		16
@@ -219,11 +243,11 @@ enum m_can_mram_cfg {
 /* 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 RXF0_ELEMENT_SIZE	72
+#define RXF1_ELEMENT_SIZE	72
 #define RXB_ELEMENT_SIZE	16
 #define TXE_ELEMENT_SIZE	8
-#define TXB_ELEMENT_SIZE	16
+#define TXB_ELEMENT_SIZE	72
 
 /* Message RAM Elements */
 #define M_CAN_FIFO_ID		0x0
@@ -231,11 +255,17 @@ enum m_can_mram_cfg {
 #define M_CAN_FIFO_DATA(n)	(0x8 + ((n) << 2))
 
 /* Rx Buffer Element */
+/* R0 */
 #define RX_BUF_ESI		BIT(31)
 #define RX_BUF_XTD		BIT(30)
 #define RX_BUF_RTR		BIT(29)
+/* R1 */
+#define RX_BUF_ANMF		BIT(31)
+#define RX_BUF_EDL		BIT(21)
+#define RX_BUF_BRS		BIT(20)
 
 /* Tx Buffer Element */
+/* R0 */
 #define TX_BUF_XTD		BIT(30)
 #define TX_BUF_RTR		BIT(29)
 
@@ -327,11 +357,12 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
 	m_can_write(priv, M_CAN_ILE, 0x0);
 }
 
-static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
+static void m_can_read_fifo(const struct net_device *dev, struct canfd_frame *cf,
 			    u32 rxfs)
 {
 	struct m_can_priv *priv = netdev_priv(dev);
 	u32 id, fgi;
+	int i;
 
 	/* calculate the fifo get index for where to read data */
 	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
@@ -341,15 +372,23 @@ static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
 	else
 		cf->can_id = (id >> 18) & CAN_SFF_MASK;
 
+	if (id & RX_BUF_ESI) {
+		cf->flags |= CANFD_ESI;
+		netdev_dbg(dev, "ESI Error\n");
+	}
+
 	if (id & RX_BUF_RTR) {
 		cf->can_id |= CAN_RTR_FLAG;
 	} else {
 		id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
-		cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
-		*(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
-							 M_CAN_FIFO_DATA(0));
-		*(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
-							 M_CAN_FIFO_DATA(1));
+		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
+
+		if (id & RX_BUF_BRS)
+			cf->flags |= CANFD_BRS;
+
+		for (i = 0; i < cf->len; i += 4)
+			*(u32 *)(cf->data + i) =
+				m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i / 4));
 	}
 
 	/* acknowledge rx fifo 0 */
@@ -361,7 +400,7 @@ 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;
+	struct canfd_frame *frame;
 	u32 pkts = 0;
 	u32 rxfs;
 
@@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 		if (rxfs & RXFS_RFL)
 			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
 
-		skb = alloc_can_skb(dev, &frame);
+		skb = alloc_canfd_skb(dev, &frame);
 		if (!skb) {
 			stats->rx_dropped++;
 			return pkts;
@@ -384,7 +423,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 		m_can_read_fifo(dev, frame, rxfs);
 
 		stats->rx_packets++;
-		stats->rx_bytes += frame->can_dlc;
+		stats->rx_bytes += frame->len;
 
 		netif_receive_skb(skb);
 
@@ -744,10 +783,23 @@ static const struct can_bittiming_const m_can_bittiming_const = {
 	.brp_inc = 1,
 };
 
+static const struct can_bittiming_const m_can_data_bittiming_const = {
+	.name = KBUILD_MODNAME,
+	.tseg1_min = 2,		/* Time segment 1 = prop_seg + phase_seg1 */
+	.tseg1_max = 16,
+	.tseg2_min = 1,		/* Time segment 2 = phase_seg2 */
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 32,
+	.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;
+	const struct can_bittiming *dbt = &priv->can.data_bittiming;
 	u16 brp, sjw, tseg1, tseg2;
 	u32 reg_btp;
 
@@ -758,7 +810,17 @@ static int m_can_set_bittiming(struct net_device *dev)
 	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);
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		brp = dbt->brp - 1;
+		sjw = dbt->sjw - 1;
+		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
+		tseg2 = dbt->phase_seg2 - 1;
+		reg_btp = (brp << FBTR_FBRP_SHIFT) | (sjw << FBTR_FSJW_SHIFT) |
+				(tseg1 << FBTR_FTSEG1_SHIFT) |
+				(tseg2 << FBTR_FTSEG2_SHIFT);
+		m_can_write(priv, M_CAN_FBTP, reg_btp);
+	}
 
 	return 0;
 }
@@ -778,8 +840,8 @@ static void m_can_chip_config(struct net_device *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);
+	/* RX Buffer/FIFO Element Size 64 bytes data field */
+	m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_64BYTES);
 
 	/* Accept Non-matching Frames Into FIFO 0 */
 	m_can_write(priv, M_CAN_GFC, 0x0);
@@ -788,8 +850,8 @@ static void m_can_chip_config(struct net_device *dev)
 	m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
 		    priv->mcfg[MRAM_TXB].off);
 
-	/* only support 8 bytes firstly */
-	m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
+	/* support 64 bytes payload */
+	m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_64BYTES);
 
 	m_can_write(priv, M_CAN_TXEFC, (1 << TXEFC_EFS_OFF) |
 		    priv->mcfg[MRAM_TXE].off);
@@ -804,7 +866,8 @@ static void m_can_chip_config(struct net_device *dev)
 		    RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
 
 	cccr = m_can_read(priv, M_CAN_CCCR);
-	cccr &= ~(CCCR_TEST | CCCR_MON);
+	cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
+		(CCCR_CME_MASK << CCCR_CME_SHIFT));
 	test = m_can_read(priv, M_CAN_TEST);
 	test &= ~TEST_LBCK;
 
@@ -816,6 +879,9 @@ static void m_can_chip_config(struct net_device *dev)
 		test |= TEST_LBCK;
 	}
 
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
+		cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
+
 	m_can_write(priv, M_CAN_CCCR, cccr);
 	m_can_write(priv, M_CAN_TEST, test);
 
@@ -880,11 +946,13 @@ static struct net_device *alloc_m_can_dev(void)
 
 	priv->dev = dev;
 	priv->can.bittiming_const = &m_can_bittiming_const;
+	priv->can.data_bittiming_const = &m_can_data_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;
+					CAN_CTRLMODE_BERR_REPORTING |
+					CAN_CTRLMODE_FD;
 
 	return dev;
 }
@@ -967,8 +1035,9 @@ 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 *cf = (struct can_frame *)skb->data;
-	u32 id;
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	u32 id, cccr;
+	int i;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
@@ -987,11 +1056,24 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 
 	/* message ram configuration */
 	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
-	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, cf->can_dlc << 16);
-	m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
-	m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
+	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
+
+	for (i = 0; i < cf->len; i += 4)
+		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
+				 *(u32 *)(cf->data + i));
+
 	can_put_echo_skb(skb, dev, 0);
 
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+		cccr = m_can_read(priv, M_CAN_CCCR);
+		cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
+		if (cf->flags & CANFD_BRS
+			cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
+		else
+			cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
+		m_can_write(priv, M_CAN_CCCR, cccr);
+	}
+
 	/* enable first TX buffer to start transfer  */
 	m_can_write(priv, M_CAN_TXBTIE, 0x1);
 	m_can_write(priv, M_CAN_TXBAR, 0x1);
-- 
1.9.1

[PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

We meet an IC issue that we have to write the full 8 bytes (whatever
value for the second word) in Message RAM to avoid bit error for transmit
data less than 4 bytes.

Without the workaround, we can easily see the following errors:
root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
[   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@imx6qdlsolo:~# cansend can0 123#112233
[   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/net/can/m_can/m_can.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 219e0e3..f2d9ebe 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1058,10 +1058,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
 	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
 
-	for (i = 0; i < cf->len; i += 4)
+	for (i = 0; i < cf->len; i += 4) {
 		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
 				 *(u32 *)(cf->data + i));
 
+		/* FIXME: we meet an IC issue that we have to write the full 8
+		 * bytes (whatever value for the second word) in Message RAM to
+		 * avoid bit error for transmit data less than 4 bytes
+		 */
+		if (cf->len <= 4)
+			m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4 + 1),
+					 0x0);
+	}
+
 	can_put_echo_skb(skb, dev, 0);
 
 	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
-- 
1.9.1

Re: [PATCH 6/7] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

Hello Dong,

thanks for your update to support CAN FD with the 3.0.x M_CAN IP core.

AFAIK from the last CAN in Automation (CiA) Plugfest which took place in
Nuremberg yesterday, there are two more IP cores on the way:

v3.0.1 / v3.0.2 (the current spec from the Bosch website)

v3.1.0 which will support per-frame CAN/CANFD switching in the tx path
(the FDF/(former)EDL bit and the BRS bit appear in the TX buffer element at
the bit position you know from reading the RX FIFO element)

v3.2.0 which will support the final(?) ISO specification with a bitstuffing
counter before the CRC in the frame on the wire.

So first I would suggest to check the core release register (CREL) to be
version 3.0.x and quit the driver initialization if it doesn't fit this
version. I would suggest to provide a separate patch for that check. What
about printing the core release version into the kernel log at driver
initialization time?

Regarding the CAN FD support in this patch I have some remarks in the text ...

On 10/29/2014 11:45 AM, Dong Aisheng wrote:

>  /* Rx Buffer Element */
> +/* R0 */
>  #define RX_BUF_ESI		BIT(31)
>  #define RX_BUF_XTD		BIT(30)
>  #define RX_BUF_RTR		BIT(29)
> +/* R1 */
> +#define RX_BUF_ANMF		BIT(31)
> +#define RX_BUF_EDL		BIT(21)
> +#define RX_BUF_BRS		BIT(20)
>  
>  /* Tx Buffer Element */
> +/* R0 */
>  #define TX_BUF_XTD		BIT(30)
>  #define TX_BUF_RTR		BIT(29)
>  
> @@ -327,11 +357,12 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
>  	m_can_write(priv, M_CAN_ILE, 0x0);
>  }
>  
> -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> +static void m_can_read_fifo(const struct net_device *dev, struct canfd_frame *cf,
>  			    u32 rxfs)
>  {
>  	struct m_can_priv *priv = netdev_priv(dev);
>  	u32 id, fgi;
> +	int i;
>  
>  	/* calculate the fifo get index for where to read data */
>  	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> @@ -341,15 +372,23 @@ static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
>  	else
>  		cf->can_id = (id >> 18) & CAN_SFF_MASK;
>  
> +	if (id & RX_BUF_ESI) {
> +		cf->flags |= CANFD_ESI;
> +		netdev_dbg(dev, "ESI Error\n");
> +	}
> +
>  	if (id & RX_BUF_RTR) {
>  		cf->can_id |= CAN_RTR_FLAG;

When RX_BUF_EDL is set you should not check for RX_BUF_RTR as RTR is not
allowed for CAN FD.

>  	} else {
>  		id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> -		cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
> -		*(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
> -							 M_CAN_FIFO_DATA(0));
> -		*(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
> -							 M_CAN_FIFO_DATA(1));
> +		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
> +
> +		if (id & RX_BUF_BRS)
> +			cf->flags |= CANFD_BRS;
> +
> +		for (i = 0; i < cf->len; i += 4)
> +			*(u32 *)(cf->data + i) =
> +				m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i / 4));
>  	}
>  
>  	/* acknowledge rx fifo 0 */
> @@ -361,7 +400,7 @@ 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;
> +	struct canfd_frame *frame;
>  	u32 pkts = 0;
>  	u32 rxfs;
>  
> @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>  		if (rxfs & RXFS_RFL)
>  			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>  
> -		skb = alloc_can_skb(dev, &frame);
> +		skb = alloc_canfd_skb(dev, &frame);

You are *always* allocating CAN FD frames now?

Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN
FD frame.

Of course you can use the struct canfd_frame in m_can_read_fifo() as the
layout of the struct can_frame is identical when filled with 'normal' CAN
frame content.

But you need to distinguish whether it is a CAN or CAN FD frame when
allocating the skb based on the RX_BUF_EDL value.

>  		if (!skb) {
>  			stats->rx_dropped++;
>  			return pkts;
> @@ -384,7 +423,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>  		m_can_read_fifo(dev, frame, rxfs);
>  
>  		stats->rx_packets++;
> -		stats->rx_bytes += frame->can_dlc;
> +		stats->rx_bytes += frame->len;
>  
>  		netif_receive_skb(skb);
>  

The rest of your entire patch set looks very good from my perspective.

Best regards,
Oliver

Re: [PATCH 6/7] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Wed, Oct 29, 2014 at 08:21:28PM +0100, Oliver Hartkopp wrote:
> Hello Dong,
> 
> thanks for your update to support CAN FD with the 3.0.x M_CAN IP core.
> 
> AFAIK from the last CAN in Automation (CiA) Plugfest which took place in
> Nuremberg yesterday, there are two more IP cores on the way:
> 
> v3.0.1 / v3.0.2 (the current spec from the Bosch website)
> 
> v3.1.0 which will support per-frame CAN/CANFD switching in the tx path
> (the FDF/(former)EDL bit and the BRS bit appear in the TX buffer element at
> the bit position you know from reading the RX FIFO element)
> 
> v3.2.0 which will support the final(?) ISO specification with a bitstuffing
> counter before the CRC in the frame on the wire.
> 

Sounds good.

> So first I would suggest to check the core release register (CREL) to be
> version 3.0.x and quit the driver initialization if it doesn't fit this
> version. I would suggest to provide a separate patch for that check. What
> about printing the core release version into the kernel log at driver
> initialization time?
> 

One question is that if v3.1.0 and v3.2.0 will be backward compatible with
v3.0.1, if yes, how about let the driver still work for them instead of
simply quit?
And then we can add new features according new released IP version.

> Regarding the CAN FD support in this patch I have some remarks in the text ...
> 
> On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> 
> >  /* Rx Buffer Element */
> > +/* R0 */
> >  #define RX_BUF_ESI		BIT(31)
> >  #define RX_BUF_XTD		BIT(30)
> >  #define RX_BUF_RTR		BIT(29)
> > +/* R1 */
> > +#define RX_BUF_ANMF		BIT(31)
> > +#define RX_BUF_EDL		BIT(21)
> > +#define RX_BUF_BRS		BIT(20)
> >  
> >  /* Tx Buffer Element */
> > +/* R0 */
> >  #define TX_BUF_XTD		BIT(30)
> >  #define TX_BUF_RTR		BIT(29)
> >  
> > @@ -327,11 +357,12 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
> >  	m_can_write(priv, M_CAN_ILE, 0x0);
> >  }
> >  
> > -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> > +static void m_can_read_fifo(const struct net_device *dev, struct canfd_frame *cf,
> >  			    u32 rxfs)
> >  {
> >  	struct m_can_priv *priv = netdev_priv(dev);
> >  	u32 id, fgi;
> > +	int i;
> >  
> >  	/* calculate the fifo get index for where to read data */
> >  	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> > @@ -341,15 +372,23 @@ static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> >  	else
> >  		cf->can_id = (id >> 18) & CAN_SFF_MASK;
> >  
> > +	if (id & RX_BUF_ESI) {
> > +		cf->flags |= CANFD_ESI;
> > +		netdev_dbg(dev, "ESI Error\n");
> > +	}
> > +
> >  	if (id & RX_BUF_RTR) {
> >  		cf->can_id |= CAN_RTR_FLAG;
> 
> When RX_BUF_EDL is set you should not check for RX_BUF_RTR as RTR is not
> allowed for CAN FD.
> 

Right, will change it.

> >  	} else {
> >  		id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> > -		cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
> > -		*(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
> > -							 M_CAN_FIFO_DATA(0));
> > -		*(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
> > -							 M_CAN_FIFO_DATA(1));
> > +		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
> > +
> > +		if (id & RX_BUF_BRS)
> > +			cf->flags |= CANFD_BRS;
> > +
> > +		for (i = 0; i < cf->len; i += 4)
> > +			*(u32 *)(cf->data + i) =
> > +				m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i / 4));
> >  	}
> >  
> >  	/* acknowledge rx fifo 0 */
> > @@ -361,7 +400,7 @@ 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;
> > +	struct canfd_frame *frame;
> >  	u32 pkts = 0;
> >  	u32 rxfs;
> >  
> > @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >  		if (rxfs & RXFS_RFL)
> >  			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >  
> > -		skb = alloc_can_skb(dev, &frame);
> > +		skb = alloc_canfd_skb(dev, &frame);
> 
> You are *always* allocating CAN FD frames now?
> 

Yes, currently it is.
The test seemed ok using CAN FD frames even receive normal frame.

The issue i know is that candump seemed can not recognize remote frame reported
by the driver.
Not sure if it's caused by canfd_frame used.
Will test and check.

> Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN
> FD frame.
> 
> Of course you can use the struct canfd_frame in m_can_read_fifo() as the
> layout of the struct can_frame is identical when filled with 'normal' CAN
> frame content.
> 
> But you need to distinguish whether it is a CAN or CAN FD frame when
> allocating the skb based on the RX_BUF_EDL value.
> 

Yes, i think it's good to do that.
One obvious benefit is it saves memory at least.

> >  		if (!skb) {
> >  			stats->rx_dropped++;
> >  			return pkts;
> > @@ -384,7 +423,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >  		m_can_read_fifo(dev, frame, rxfs);
> >  
> >  		stats->rx_packets++;
> > -		stats->rx_bytes += frame->can_dlc;
> > +		stats->rx_bytes += frame->len;
> >  
> >  		netif_receive_skb(skb);
> >  
> 
> The rest of your entire patch set looks very good from my perspective.
> 

Thanks for the review. :-)

> Best regards,
> Oliver
> 
> 

Regards
Dong Aisheng

Re: [PATCH 6/7] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

On 10/30/2014 03:42 AM, Dong Aisheng wrote:
> On Wed, Oct 29, 2014 at 08:21:28PM +0100, Oliver Hartkopp wrote:

>> So first I would suggest to check the core release register (CREL) to be
>> version 3.0.x and quit the driver initialization if it doesn't fit this
>> version. I would suggest to provide a separate patch for that check. What
>> about printing the core release version into the kernel log at driver
>> initialization time?
>>
> 
> One question is that if v3.1.0 and v3.2.0 will be backward compatible with
> v3.0.1, if yes, how about let the driver still work for them instead of
> simply quit?

There are several important differences between 3.0.x and 3.1.x.
E.g. the CCCR, BTP, PSR and others are changed and a register for the
transmitter delay compensation is added.

I assume from 3.1.x to 3.2.x the controller registers will only change in
small details as the main update will be on the wire and not in the functionality.

> And then we can add new features according new released IP version.

Yes. We probably can wait for 3.[12].x to become available before adding the
special code that behaves according the core release register content.

>>> @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>>>  		if (rxfs & RXFS_RFL)
>>>  			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>>>  
>>> -		skb = alloc_can_skb(dev, &frame);
>>> +		skb = alloc_canfd_skb(dev, &frame);
>>
>> You are *always* allocating CAN FD frames now?
>>
> 
> Yes, currently it is.
> The test seemed ok using CAN FD frames even receive normal frame.

When you put CAN frame content into a CAN FD skb it becomes a CAN FD frame -
which is wrong.

CAN 2.0 frame (EDL is clear) -> alloc_can_skb()
CAN FD frame (EDL is set) -> alloc_canfd_skb()

You can have a CAN FD frame with a DLC of 8, which does *not* mean that you
have a CAN 2.0 frame.

> The issue i know is that candump seemed can not recognize remote frame reported
> by the driver.

Do you use the latest candump from

	https://gitorious.org/linux-can/can-utils/
??

The latest candump switches the CAN_RAW socket into the mode to accept both
CAN *and* CAN FD frames and displays the frames correctly.

> Not sure if it's caused by canfd_frame used.

Yes. CAN FD frames do not have a RTR bit.

> Will test and check.
> 
>> Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN
>> FD frame.
>>
>> Of course you can use the struct canfd_frame in m_can_read_fifo() as the
>> layout of the struct can_frame is identical when filled with 'normal' CAN
>> frame content.
>>
>> But you need to distinguish whether it is a CAN or CAN FD frame when
>> allocating the skb based on the RX_BUF_EDL value.
>>
> 
> Yes, i think it's good to do that.
> One obvious benefit is it saves memory at least.

The main point is that CAN frames and CAN FD frames are separated by this
(MTU) length information. It's not about saving memory.
A CAN FD frame with DLC 8 still has 64 data bytes inside it's data structure.

Regards,
Oliver

Re: [PATCH 1/7] can: m_can: fix possible sleep in napi poll

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> The m_can_get_berr_counter function can sleep and it may be called in napi poll function.
> Rework it to fix the following warning.

Applied to can/master.

Thanks, Marc

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


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

Re: [PATCH 2/7] can: m_can: fix the incorrect error messages

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> Fix a few error messages.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

Applied to can/master.

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


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

Re: [PATCH 5/7] can: clear ctrlmode when close candev

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> Currently priv->ctrlmode is not cleared when close_candev, so next time
> the driver will still use this value to set controller even user
> does not set any ctrl mode.
> e.g.
> Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on
> Controller will be in loopback mode
> Step 2. ip link set can0 down
> Step 3. ip link set can0 up type can0 bitrate 1000000
> Controller will still be set to loopback mode in driver due to saved
> priv->ctrlmode.
> 
> This patch clears priv->ctrlmode when the CAN interface is closed,
> and set it to correct mode according to next user setting.

Oliver, what do you think of this patch? It will introduce a subtle
change to the userspace.

Marc

> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/net/can/dev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 02492d2..1fce485 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -671,6 +671,7 @@ void close_candev(struct net_device *dev)
>  
>  	del_timer_sync(&priv->restart_timer);
>  	can_flush_echo_skb(dev);
> +	priv->ctrlmode = 0;
>  }
>  EXPORT_SYMBOL_GPL(close_candev);
>  
> 


-- 
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: 819 bytes --]

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> We meet an IC issue that we have to write the full 8 bytes (whatever
> value for the second word) in Message RAM to avoid bit error for transmit
> data less than 4 bytes.

Is this a SoC or a m_can problem? Are all versions of the SoC/m_can
affected? Is there a m_can version register somewhere?

> Without the workaround, we can easily see the following errors:
> root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root@imx6qdlsolo:~# cansend can0 123#112233
> [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/net/can/m_can/m_can.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 219e0e3..f2d9ebe 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1058,10 +1058,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
>  
> -	for (i = 0; i < cf->len; i += 4)
> +	for (i = 0; i < cf->len; i += 4) {
>  		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
>  				 *(u32 *)(cf->data + i));
>  
> +		/* FIXME: we meet an IC issue that we have to write the full 8

FIXME usually indicates that the driver needs some work here. Just
describe your hardware bug, you might add a reference to an errata if
available, though.

> +		 * bytes (whatever value for the second word) in Message RAM to
> +		 * avoid bit error for transmit data less than 4 bytes
> +		 */
> +		if (cf->len <= 4)
> +			m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4 + 1),
> +					 0x0);

This workaround doesn't handle the dlc == 0 case, your error description
isn't completely if this is a problem, too.

It should be possible to change the for loop to go always to 8, or
simply unroll the loop:

/* errata description goes here */
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));

> +	}
> +
>  	can_put_echo_skb(skb, dev, 0);
>  
>  	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> 

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: 819 bytes --]

Re: [PATCH 3/7] can: m_can: add .ndo_change_mtu function

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> Use common can_change_mtu function.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

Applied to can/master.

Thanks,
Marc

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


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

Re: [PATCH 4/7] can: m_can: add a bit delay after setting CCCR_INIT bit

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> The spec mentions there may be a delay until the value written to
> INIT can be read back due to the synchronization mechanism between the
> two clock domains. But it does not indicate the exact clock cycles needed.
> The 5us delay is a test value and seems ok.
> 
> Without the delay, CCCR.CCE bit may fail to be set and then the
> initialization fail sometimes when do repeatly up and down.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

Applied to can/master.

Thanks,
Marc

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


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

Re: [PATCH 1/7] can: m_can: fix possible sleep in napi poll

[Marc Kleine-Budde]

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

Hey Dong Aisheng,

the state of the patches are:

1: can/master + stable
2: can/mster
3: can/mster
4: can/master + stable
5: waiting for Oliver's opinion due to the user space change
6: waiting for your input
7: waiting for your input

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: 819 bytes --]

Re: [PATCH 5/7] can: clear ctrlmode when close candev

[Oliver Hartkopp]

On 11/03/2014 05:28 PM, Marc Kleine-Budde wrote:
> On 10/29/2014 11:45 AM, Dong Aisheng wrote:
>> Currently priv->ctrlmode is not cleared when close_candev, so next time
>> the driver will still use this value to set controller even user
>> does not set any ctrl mode.
>> e.g.
>> Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on
>> Controller will be in loopback mode
>> Step 2. ip link set can0 down
>> Step 3. ip link set can0 up type can0 bitrate 1000000
>> Controller will still be set to loopback mode in driver due to saved
>> priv->ctrlmode.
>>
>> This patch clears priv->ctrlmode when the CAN interface is closed,
>> and set it to correct mode according to next user setting.
> 
> Oliver, what do you think of this patch? It will introduce a subtle
> change to the userspace.

Hi Marc,

indeed the current policy is that ifdown/ifup doesn't change any setting.
When we start to clear priv->ctrlmode when setting the interface down the
question arises whether the bitrate(s) has/have to be wiped too.

Setting the interface down/up can be done with ip or ifconfig without changing
the controller settings.

So IMO we can not assume that people always use the 'all-in-one'

	ip link set can0 up type can0 bitrate 1000000

command as given in the example above.

You might also do it like this:

	ip link set can0 up type can0 bitrate 1000000 loopback on
	ip link set can0 down
	ip link set can0 type can0 loopback off
	ip link set can0 up

Finally I would vote to reject this patch to stay consistent.

When users like to fiddle with options like loopback they need to take care
about their 'expert' settings too.

Regards,
Oliver


> 
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/net/can/dev.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 02492d2..1fce485 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -671,6 +671,7 @@ void close_candev(struct net_device *dev)
>>  
>>  	del_timer_sync(&priv->restart_timer);
>>  	can_flush_echo_skb(dev);
>> +	priv->ctrlmode = 0;
>>  }
>>  EXPORT_SYMBOL_GPL(close_candev);
>>  
>>
> 
> 

Re: [PATCH 1/7] can: m_can: fix possible sleep in napi poll

[Oliver Hartkopp]

Hello Marc,

you can omit the 'stable' tag as this driver just emerged in 3.18 - which is
currently in 3.18-rc3 state.

If possible I would suggest to push all 7 patches via can/master as it is
pretty bad to say "M_CAN was introduced in 3.18 but M_CAN with CAN FD is
working since 3.19".

This will produce confusion as the M_CAN core has built-in CAN FD support.

Regards,
Oliver

On 11/03/2014 06:02 PM, Marc Kleine-Budde wrote:
> Hey Dong Aisheng,
> 
> the state of the patches are:
> 
> 1: can/master + stable
> 2: can/mster
> 3: can/mster
> 4: can/master + stable
> 5: waiting for Oliver's opinion due to the user space change
> 6: waiting for your input
> 7: waiting for your input
> 
> Marc
> 

Re: [PATCH 5/7] can: clear ctrlmode when close candev

[Marc Kleine-Budde]

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

On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> Currently priv->ctrlmode is not cleared when close_candev, so next time
> the driver will still use this value to set controller even user
> does not set any ctrl mode.
> e.g.
> Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on
> Controller will be in loopback mode
> Step 2. ip link set can0 down
> Step 3. ip link set can0 up type can0 bitrate 1000000
> Controller will still be set to loopback mode in driver due to saved
> priv->ctrlmode.
> 
> This patch clears priv->ctrlmode when the CAN interface is closed,
> and set it to correct mode according to next user setting.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

NACK, as discussed with Oliver.

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: 819 bytes --]

Re: [PATCH 1/7] can: m_can: fix possible sleep in napi poll

[Marc Kleine-Budde]

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

On 11/03/2014 07:37 PM, Oliver Hartkopp wrote:
> you can omit the 'stable' tag as this driver just emerged in 3.18 - which is
> currently in 3.18-rc3 state.

Thanks! fixed.

> If possible I would suggest to push all 7 patches via can/master as it is
> pretty bad to say "M_CAN was introduced in 3.18 but M_CAN with CAN FD is
> working since 3.19".

Sorry, probably not, as CAN FD is a new feature. However we may talk to
David, as the m_can driver was introduced in 3.18 anyways, so it's
unlikely to break anything with this patch.

> This will produce confusion as the M_CAN core has built-in CAN FD support.

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: 819 bytes --]

Re: [PATCH 1/7] can: m_can: fix possible sleep in napi poll

[Oliver Hartkopp]

On 11/03/2014 09:49 PM, Marc Kleine-Budde wrote:
> On 11/03/2014 07:37 PM, Oliver Hartkopp wrote:

>> If possible I would suggest to push all 7 patches via can/master as it is
>> pretty bad to say "M_CAN was introduced in 3.18 but M_CAN with CAN FD is
>> working since 3.19".
> 
> Sorry, probably not, as CAN FD is a new feature. However we may talk to
> David, as the m_can driver was introduced in 3.18 anyways, so it's
> unlikely to break anything with this patch.
> 

Yes. It's just to be 'feature complete' for the FD enabled M_CAN driver.
Hopefully it's ok for Dave ...

Thanks,
Oliver

Re: [PATCH 6/7] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Thu, Oct 30, 2014 at 09:32:49PM +0100, Oliver Hartkopp wrote:
> 
> On 10/30/2014 03:42 AM, Dong Aisheng wrote:
> > On Wed, Oct 29, 2014 at 08:21:28PM +0100, Oliver Hartkopp wrote:
> 
> >> So first I would suggest to check the core release register (CREL) to be
> >> version 3.0.x and quit the driver initialization if it doesn't fit this
> >> version. I would suggest to provide a separate patch for that check. What
> >> about printing the core release version into the kernel log at driver
> >> initialization time?
> >>
> > 
> > One question is that if v3.1.0 and v3.2.0 will be backward compatible with
> > v3.0.1, if yes, how about let the driver still work for them instead of
> > simply quit?
> 
> There are several important differences between 3.0.x and 3.1.x.
> E.g. the CCCR, BTP, PSR and others are changed and a register for the
> transmitter delay compensation is added.
> 
> I assume from 3.1.x to 3.2.x the controller registers will only change in
> small details as the main update will be on the wire and not in the functionality.
> 
> > And then we can add new features according new released IP version.
> 
> Yes. We probably can wait for 3.[12].x to become available before adding the
> special code that behaves according the core release register content.
> 

Okay

> >>> @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >>>  		if (rxfs & RXFS_RFL)
> >>>  			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >>>  
> >>> -		skb = alloc_can_skb(dev, &frame);
> >>> +		skb = alloc_canfd_skb(dev, &frame);
> >>
> >> You are *always* allocating CAN FD frames now?
> >>
> > 
> > Yes, currently it is.
> > The test seemed ok using CAN FD frames even receive normal frame.
> 
> When you put CAN frame content into a CAN FD skb it becomes a CAN FD frame -
> which is wrong.
> 
> CAN 2.0 frame (EDL is clear) -> alloc_can_skb()
> CAN FD frame (EDL is set) -> alloc_canfd_skb()
> 
> You can have a CAN FD frame with a DLC of 8, which does *not* mean that you
> have a CAN 2.0 frame.
> 
> > The issue i know is that candump seemed can not recognize remote frame reported
> > by the driver.
> 
> Do you use the latest candump from
> 

Yes, i'm using latest candump.

> 	https://gitorious.org/linux-can/can-utils/
> ??
> 
> The latest candump switches the CAN_RAW socket into the mode to accept both
> CAN *and* CAN FD frames and displays the frames correctly.
> 
> > Not sure if it's caused by canfd_frame used.
> 
> Yes. CAN FD frames do not have a RTR bit.
> 

You're right.
It's indeed caused by using the CAN FD frames to receive RTR frame.
After switch to normal frame, candump showed it well.

> > Will test and check.
> > 
> >> Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN
> >> FD frame.
> >>
> >> Of course you can use the struct canfd_frame in m_can_read_fifo() as the
> >> layout of the struct can_frame is identical when filled with 'normal' CAN
> >> frame content.
> >>
> >> But you need to distinguish whether it is a CAN or CAN FD frame when
> >> allocating the skb based on the RX_BUF_EDL value.
> >>
> > 
> > Yes, i think it's good to do that.
> > One obvious benefit is it saves memory at least.
> 
> The main point is that CAN frames and CAN FD frames are separated by this
> (MTU) length information. It's not about saving memory.
> A CAN FD frame with DLC 8 still has 64 data bytes inside it's data structure.
> 

For normal can frame, the CAN_MAX_DLEN is 8 while CANFD_MAX_DLEN is 64.
So i meant using struct canfd_frame to receive normal frame is a bit waste memory.
And besides, actually it's wrong as you already indicated.
I will send out the updated patch with this changed in v2 soon.
Thanks for pointing out this.

Regards
Dong Aisheng

> Regards,
> Oliver

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Mon, Nov 03, 2014 at 05:48:17PM +0100, Marc Kleine-Budde wrote:
> On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> > We meet an IC issue that we have to write the full 8 bytes (whatever
> > value for the second word) in Message RAM to avoid bit error for transmit
> > data less than 4 bytes.
> 
> Is this a SoC or a m_can problem? Are all versions of the SoC/m_can
> affected? Is there a m_can version register somewhere?
> 

I'm still not sure it's SoC or m_can problem.
Our IC guys ran the simulation code and found this issue.
But due to some reasons, it may be very slow for they to investigate
and get the conclusion.

> > Without the workaround, we can easily see the following errors:
> > root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> > [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> > root@imx6qdlsolo:~# cansend can0 123#112233
> > [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> > 
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 219e0e3..f2d9ebe 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1058,10 +1058,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> >  	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
> >  	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
> >  
> > -	for (i = 0; i < cf->len; i += 4)
> > +	for (i = 0; i < cf->len; i += 4) {
> >  		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
> >  				 *(u32 *)(cf->data + i));
> >  
> > +		/* FIXME: we meet an IC issue that we have to write the full 8
> 
> FIXME usually indicates that the driver needs some work here. Just
> describe your hardware bug, you might add a reference to an errata if
> available, though.
> 

We don't have an errata for it now.
Because i'm not sure this is the final workaround and also not sure if other
SoC vendors having the same issue, so i used FIXME here firstly.
Since the code is harmless, so i wish we could put it here first
until we find evidence no need for other SoC or only belong to specific
IP version.

> > +		 * bytes (whatever value for the second word) in Message RAM to
> > +		 * avoid bit error for transmit data less than 4 bytes
> > +		 */
> > +		if (cf->len <= 4)
> > +			m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4 + 1),
> > +					 0x0);
> 
> This workaround doesn't handle the dlc == 0 case, your error description
> isn't completely if this is a problem, too.
> 

You're right.
I just checked the dlc == 0 case also had such issue and it also needs
the extra 8 bytes write to avoid such issue.

BTW the issue only happened on the first time when you send a frame with no
data(dlc == 0) at the first time.
e.g.
root@imx6sxsabresd:~# ip link set can0 up type can bitrate 1000000
[   62.326014] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@imx6sxsabresd:~# cansend can0 123#R
[   69.233645] m_can 20e8000.can can0: Bit Error Uncorrected
[   69.239167] m_can 20e8000.can can0: Bit Error Corrected

If we send a frame success first (e.g. 5 bytes data), it will not fail
again even you send no data frame (dlc == 0) later.

The former failure of sending data less than 4 bytes is similar.

Looks like the first 8 bytes of message ram has to be initialised
for the first using.

> It should be possible to change the for loop to go always to 8, or
> simply unroll the loop:
> 
> /* errata description goes here */
> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
> 

Yes, i tried to fix it as follows.

/* FIXME: we meet an IC issue that we have to write the full 8
 * bytes (whatever value for the second word) in Message RAM to
 * avoid bit error for transmit data less than 4 bytes
 */
if (cf->len <= 4) {
        m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0),
                         *(u32 *)(cf->data + 0));
        m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1),
                         *(u32 *)(cf->data + 4));
} else {
        for (i = 0; i < cf->len; i += 4)
                m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
                                 *(u32 *)(cf->data + i));

Will update the patch.

Regards
Dong Aisheng

> > +	}
> > +
> >  	can_put_echo_skb(skb, dev, 0);
> >  
> >  	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > 
> 
> 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   |
> 

Re: [PATCH 5/7] can: clear ctrlmode when close candev

[Dong Aisheng]

On Mon, Nov 03, 2014 at 09:47:22PM +0100, Marc Kleine-Budde wrote:
> On 10/29/2014 11:45 AM, Dong Aisheng wrote:
> > Currently priv->ctrlmode is not cleared when close_candev, so next time
> > the driver will still use this value to set controller even user
> > does not set any ctrl mode.
> > e.g.
> > Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on
> > Controller will be in loopback mode
> > Step 2. ip link set can0 down
> > Step 3. ip link set can0 up type can0 bitrate 1000000
> > Controller will still be set to loopback mode in driver due to saved
> > priv->ctrlmode.
> > 
> > This patch clears priv->ctrlmode when the CAN interface is closed,
> > and set it to correct mode according to next user setting.
> > 
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> 
> NACK, as discussed with Oliver.
> 

Okay, i'm fine with it.

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

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Marc Kleine-Budde]

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

On 11/04/2014 09:25 AM, Dong Aisheng wrote:
>>> We meet an IC issue that we have to write the full 8 bytes (whatever
>>> value for the second word) in Message RAM to avoid bit error for transmit
>>> data less than 4 bytes.
>>
>> Is this a SoC or a m_can problem? Are all versions of the SoC/m_can
>> affected? Is there a m_can version register somewhere?

> I'm still not sure it's SoC or m_can problem.
> Our IC guys ran the simulation code and found this issue.
> But due to some reasons, it may be very slow for they to investigate
> and get the conclusion.

Let's hope they will find the root cause of this problem.

>>> Without the workaround, we can easily see the following errors:
>>> root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
>>> [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
>>> root@imx6qdlsolo:~# cansend can0 123#112233
>>> [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
>>>
>>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>>> ---
>>>  drivers/net/can/m_can/m_can.c | 11 ++++++++++-
>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>> index 219e0e3..f2d9ebe 100644
>>> --- a/drivers/net/can/m_can/m_can.c
>>> +++ b/drivers/net/can/m_can/m_can.c
>>> @@ -1058,10 +1058,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
>>>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
>>>  
>>> -	for (i = 0; i < cf->len; i += 4)
>>> +	for (i = 0; i < cf->len; i += 4) {
>>>  		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
>>>  				 *(u32 *)(cf->data + i));
>>>  
>>> +		/* FIXME: we meet an IC issue that we have to write the full 8
>>
>> FIXME usually indicates that the driver needs some work here. Just
>> describe your hardware bug, you might add a reference to an errata if
>> available, though.
>
> We don't have an errata for it now.
> Because i'm not sure this is the final workaround and also not sure if other
> SoC vendors having the same issue, so i used FIXME here firstly.
> Since the code is harmless, so i wish we could put it here first
> until we find evidence no need for other SoC or only belong to specific
> IP version.

It's better to write this in the comment than a FIXME, which is much
harder to interpret....

>>> +		 * bytes (whatever value for the second word) in Message RAM to
>>> +		 * avoid bit error for transmit data less than 4 bytes
>>> +		 */
>>> +		if (cf->len <= 4)
>>> +			m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4 + 1),
>>> +					 0x0);
>>
>> This workaround doesn't handle the dlc == 0 case, your error description
>> isn't completely if this is a problem, too.

> You're right.
> I just checked the dlc == 0 case also had such issue and it also needs
> the extra 8 bytes write to avoid such issue.
> 
> BTW the issue only happened on the first time when you send a frame with no
> data(dlc == 0) at the first time.
> e.g.
> root@imx6sxsabresd:~# ip link set can0 up type can bitrate 1000000
> [   62.326014] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root@imx6sxsabresd:~# cansend can0 123#R
> [   69.233645] m_can 20e8000.can can0: Bit Error Uncorrected
> [   69.239167] m_can 20e8000.can can0: Bit Error Corrected
> 
> If we send a frame success first (e.g. 5 bytes data), it will not fail
> again even you send no data frame (dlc == 0) later.
> 
> The former failure of sending data less than 4 bytes is similar.
> 
> Looks like the first 8 bytes of message ram has to be initialised
> for the first using.

What about putting

/* errata description goes here */
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);

into the open() function? Can you ask the hardware colleges if this is a
functional workaround.

>> It should be possible to change the for loop to go always to 8, or
>> simply unroll the loop:
>>
>> /* errata description goes here */
>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
>>
> 
> Yes, i tried to fix it as follows.
> 
> /* FIXME: we meet an IC issue that we have to write the full 8
>  * bytes (whatever value for the second word) in Message RAM to
>  * avoid bit error for transmit data less than 4 bytes
>  */
> if (cf->len <= 4) {
>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0),
>                          *(u32 *)(cf->data + 0));
>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1),
>                          *(u32 *)(cf->data + 4));
> } else {
>         for (i = 0; i < cf->len; i += 4)
>                 m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
>                                  *(u32 *)(cf->data + i));
> 
> Will update the patch.

Both branches of the above if are doing the same thing, I think you can
replace the while if ... else ... for with this:

/* errata description goes here */
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));

However if writing to DATA(0) and DATA(1) once in the open() function is
enough this code should stay as it is.

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: 819 bytes --]

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Tue, Nov 04, 2014 at 10:22:16AM +0100, Marc Kleine-Budde wrote:
> On 11/04/2014 09:25 AM, Dong Aisheng wrote:
> >>> We meet an IC issue that we have to write the full 8 bytes (whatever
> >>> value for the second word) in Message RAM to avoid bit error for transmit
> >>> data less than 4 bytes.
> >>
> >> Is this a SoC or a m_can problem? Are all versions of the SoC/m_can
> >> affected? Is there a m_can version register somewhere?
> 
> > I'm still not sure it's SoC or m_can problem.
> > Our IC guys ran the simulation code and found this issue.
> > But due to some reasons, it may be very slow for they to investigate
> > and get the conclusion.
> 
> Let's hope they will find the root cause of this problem.
> 
> >>> Without the workaround, we can easily see the following errors:
> >>> root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> >>> [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> >>> root@imx6qdlsolo:~# cansend can0 123#112233
> >>> [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> >>>
> >>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> >>> ---
> >>>  drivers/net/can/m_can/m_can.c | 11 ++++++++++-
> >>>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> >>> index 219e0e3..f2d9ebe 100644
> >>> --- a/drivers/net/can/m_can/m_can.c
> >>> +++ b/drivers/net/can/m_can/m_can.c
> >>> @@ -1058,10 +1058,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> >>>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
> >>>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
> >>>  
> >>> -	for (i = 0; i < cf->len; i += 4)
> >>> +	for (i = 0; i < cf->len; i += 4) {
> >>>  		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
> >>>  				 *(u32 *)(cf->data + i));
> >>>  
> >>> +		/* FIXME: we meet an IC issue that we have to write the full 8
> >>
> >> FIXME usually indicates that the driver needs some work here. Just
> >> describe your hardware bug, you might add a reference to an errata if
> >> available, though.
> >
> > We don't have an errata for it now.
> > Because i'm not sure this is the final workaround and also not sure if other
> > SoC vendors having the same issue, so i used FIXME here firstly.
> > Since the code is harmless, so i wish we could put it here first
> > until we find evidence no need for other SoC or only belong to specific
> > IP version.
> 
> It's better to write this in the comment than a FIXME, which is much
> harder to interpret....
> 
> >>> +		 * bytes (whatever value for the second word) in Message RAM to
> >>> +		 * avoid bit error for transmit data less than 4 bytes
> >>> +		 */
> >>> +		if (cf->len <= 4)
> >>> +			m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4 + 1),
> >>> +					 0x0);
> >>
> >> This workaround doesn't handle the dlc == 0 case, your error description
> >> isn't completely if this is a problem, too.
> 
> > You're right.
> > I just checked the dlc == 0 case also had such issue and it also needs
> > the extra 8 bytes write to avoid such issue.
> > 
> > BTW the issue only happened on the first time when you send a frame with no
> > data(dlc == 0) at the first time.
> > e.g.
> > root@imx6sxsabresd:~# ip link set can0 up type can bitrate 1000000
> > [   62.326014] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> > root@imx6sxsabresd:~# cansend can0 123#R
> > [   69.233645] m_can 20e8000.can can0: Bit Error Uncorrected
> > [   69.239167] m_can 20e8000.can can0: Bit Error Corrected
> > 
> > If we send a frame success first (e.g. 5 bytes data), it will not fail
> > again even you send no data frame (dlc == 0) later.
> > 
> > The former failure of sending data less than 4 bytes is similar.
> > 
> > Looks like the first 8 bytes of message ram has to be initialised
> > for the first using.
> 
> What about putting
> 
> /* errata description goes here */
> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
> 
> into the open() function? Can you ask the hardware colleges if this is a
> functional workaround.
> 
> >> It should be possible to change the for loop to go always to 8, or
> >> simply unroll the loop:
> >>
> >> /* errata description goes here */
> >> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
> >> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
> >>
> > 
> > Yes, i tried to fix it as follows.
> > 
> > /* FIXME: we meet an IC issue that we have to write the full 8
> >  * bytes (whatever value for the second word) in Message RAM to
> >  * avoid bit error for transmit data less than 4 bytes
> >  */
> > if (cf->len <= 4) {
> >         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0),
> >                          *(u32 *)(cf->data + 0));
> >         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1),
> >                          *(u32 *)(cf->data + 4));
> > } else {
> >         for (i = 0; i < cf->len; i += 4)
> >                 m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
> >                                  *(u32 *)(cf->data + i));
> > 
> > Will update the patch.
> 
> Both branches of the above if are doing the same thing, I think you can
> replace the while if ... else ... for with this:
> 

Not the same thing.
The later one will cover payload up to 64 bytes.

> /* errata description goes here */
> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
> 
> However if writing to DATA(0) and DATA(1) once in the open() function is
> enough this code should stay as it is.

I tried put them into open() function and the quick test showed it worked.

Do you think it's ok to put things into open() function for this issue
as follows?

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 065e4f1..ca55988 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -901,6 +901,15 @@ static void m_can_chip_config(struct net_device *dev)
        /* set bittiming params */
        m_can_set_bittiming(dev);

+       /* We meet an IC issue that we have to write the full 8
+        * bytes (whatever value for the second word) in Message RAM to
+        * avoid bit error for transmit data less than 4 bytes at the first
+        * time. By initializing the first 8 bytes of tx buffer before using
+        * it can avoid such issue.
+        */
+       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
+       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
+
        m_can_config_endisable(priv, false);
 }

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

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Marc Kleine-Budde]

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

On 11/04/2014 10:27 AM, Dong Aisheng wrote:
>>>> It should be possible to change the for loop to go always to 8, or
>>>> simply unroll the loop:
>>>>
>>>> /* errata description goes here */
>>>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
>>>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
>>>>
>>>
>>> Yes, i tried to fix it as follows.
>>>
>>> /* FIXME: we meet an IC issue that we have to write the full 8
>>>  * bytes (whatever value for the second word) in Message RAM to
>>>  * avoid bit error for transmit data less than 4 bytes
>>>  */
>>> if (cf->len <= 4) {
>>>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0),
>>>                          *(u32 *)(cf->data + 0));
>>>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1),
>>>                          *(u32 *)(cf->data + 4));
>>> } else {
>>>         for (i = 0; i < cf->len; i += 4)
>>>                 m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
>>>                                  *(u32 *)(cf->data + i));
>>>
>>> Will update the patch.
>>
>> Both branches of the above if are doing the same thing, I think you can
>> replace the while if ... else ... for with this:
>>
> 
> Not the same thing.
> The later one will cover payload up to 64 bytes.

Doh! I'm not used to CAN-FD, yet :) However, I'll apply this fix before
adding the CAN-FD support.

>> /* errata description goes here */
>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
>>
>> However if writing to DATA(0) and DATA(1) once in the open() function is
>> enough this code should stay as it is.
> 
> I tried put them into open() function and the quick test showed it worked.
> 
> Do you think it's ok to put things into open() function for this issue
> as follows?
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 065e4f1..ca55988 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -901,6 +901,15 @@ static void m_can_chip_config(struct net_device *dev)
>         /* set bittiming params */
>         m_can_set_bittiming(dev);
> 
> +       /* We meet an IC issue that we have to write the full 8

At least on the *insert SoC name here*, an issue with the Message RAM
was discovered. Sending CAN frames with dlc less than 4 bytes will lead
to bit errors, when the first 8 bytes of the Message RAM have not been
initialized (i.e. written to). To work around this issue, the first 8
bytes are initialized here.

> +        * bytes (whatever value for the second word) in Message RAM to
> +        * avoid bit error for transmit data less than 4 bytes at the first
> +        * time. By initializing the first 8 bytes of tx buffer before using
> +        * it can avoid such issue.
> +        */
> +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
> +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
> +
>         m_can_config_endisable(priv, false);
>  }

Can you trigger the issue when sending CAN-FD frames with dlc > 8 && dlc
< 64?

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: 819 bytes --]

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Oliver Hartkopp]

On 04.11.2014 11:33, Marc Kleine-Budde wrote:
> On 11/04/2014 10:27 AM, Dong Aisheng wrote:


>> +       /* We meet an IC issue that we have to write the full 8
>
> At least on the *insert SoC name here*, an issue with the Message RAM
> was discovered. Sending CAN frames with dlc less than 4 bytes will lead
> to bit errors, when the first 8 bytes of the Message RAM have not been
> initialized (i.e. written to). To work around this issue, the first 8
> bytes are initialized here.

Yes. Also put the current IP revision (3.0.x) into the comment.
Did inform the Bosch guys from this issue - or is it already in some errata sheet?

>
>> +        * bytes (whatever value for the second word) in Message RAM to
>> +        * avoid bit error for transmit data less than 4 bytes at the first
>> +        * time. By initializing the first 8 bytes of tx buffer before using
>> +        * it can avoid such issue.
>> +        */
>> +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
>> +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
>> +
>>          m_can_config_endisable(priv, false);
>>   }
>
> Can you trigger the issue when sending CAN-FD frames with dlc > 8 && dlc
> < 64?

Just a nitpick:

DLC can just be 0 .. 15

and the length (struct canfd_frame.len) can be from 0 .. 64

See:

http://lxr.free-electrons.com/source/include/uapi/linux/can.h#L83

That's the reason for all these helpers

http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L36

that hide the evil "DLC" from userspace now and make 'len' a usable loop 
variable as we were able to use the former dlc for classic CAN :-)

Regards,
Oliver

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Marc Kleine-Budde]

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

On 11/04/2014 02:13 PM, Oliver Hartkopp wrote:
> 
> 
> On 04.11.2014 11:33, Marc Kleine-Budde wrote:
>> On 11/04/2014 10:27 AM, Dong Aisheng wrote:
> 
> 
>>> +       /* We meet an IC issue that we have to write the full 8
>>
>> At least on the *insert SoC name here*, an issue with the Message RAM
>> was discovered. Sending CAN frames with dlc less than 4 bytes will lead
>> to bit errors, when the first 8 bytes of the Message RAM have not been
>> initialized (i.e. written to). To work around this issue, the first 8
>> bytes are initialized here.
> 
> Yes. Also put the current IP revision (3.0.x) into the comment.

Good idea - also add the SoC's mask revision.

> Did inform the Bosch guys from this issue - or is it already in some
> errata sheet?
> 
>>
>>> +        * bytes (whatever value for the second word) in Message RAM to
>>> +        * avoid bit error for transmit data less than 4 bytes at the
>>> first
>>> +        * time. By initializing the first 8 bytes of tx buffer
>>> before using
>>> +        * it can avoid such issue.
>>> +        */
>>> +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
>>> +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
>>> +
>>>          m_can_config_endisable(priv, false);
>>>   }
>>
>> Can you trigger the issue when sending CAN-FD frames with dlc > 8 && dlc
>> < 64?
> 
> Just a nitpick:
> 
> DLC can just be 0 .. 15

Correct, I was talking about the length

> and the length (struct canfd_frame.len) can be from 0 .. 64
> 
> See:
> 
> http://lxr.free-electrons.com/source/include/uapi/linux/can.h#L83
> 
> That's the reason for all these helpers
> 
> http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L36
> 
> that hide the evil "DLC" from userspace now and make 'len' a usable loop
> variable as we were able to use the former dlc for classic CAN :-)

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: 819 bytes --]

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Tue, Nov 04, 2014 at 11:33:09AM +0100, Marc Kleine-Budde wrote:
> On 11/04/2014 10:27 AM, Dong Aisheng wrote:
> >>>> It should be possible to change the for loop to go always to 8, or
> >>>> simply unroll the loop:
> >>>>
> >>>> /* errata description goes here */
> >>>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
> >>>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
> >>>>
> >>>
> >>> Yes, i tried to fix it as follows.
> >>>
> >>> /* FIXME: we meet an IC issue that we have to write the full 8
> >>>  * bytes (whatever value for the second word) in Message RAM to
> >>>  * avoid bit error for transmit data less than 4 bytes
> >>>  */
> >>> if (cf->len <= 4) {
> >>>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0),
> >>>                          *(u32 *)(cf->data + 0));
> >>>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1),
> >>>                          *(u32 *)(cf->data + 4));
> >>> } else {
> >>>         for (i = 0; i < cf->len; i += 4)
> >>>                 m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
> >>>                                  *(u32 *)(cf->data + i));
> >>>
> >>> Will update the patch.
> >>
> >> Both branches of the above if are doing the same thing, I think you can
> >> replace the while if ... else ... for with this:
> >>
> > 
> > Not the same thing.
> > The later one will cover payload up to 64 bytes.
> 
> Doh! I'm not used to CAN-FD, yet :) However, I'll apply this fix before
> adding the CAN-FD support.
> 
> >> /* errata description goes here */
> >> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
> >> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
> >>
> >> However if writing to DATA(0) and DATA(1) once in the open() function is
> >> enough this code should stay as it is.
> > 
> > I tried put them into open() function and the quick test showed it worked.
> > 
> > Do you think it's ok to put things into open() function for this issue
> > as follows?
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 065e4f1..ca55988 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -901,6 +901,15 @@ static void m_can_chip_config(struct net_device *dev)
> >         /* set bittiming params */
> >         m_can_set_bittiming(dev);
> > 
> > +       /* We meet an IC issue that we have to write the full 8
> 
> At least on the *insert SoC name here*, an issue with the Message RAM
> was discovered. Sending CAN frames with dlc less than 4 bytes will lead
> to bit errors, when the first 8 bytes of the Message RAM have not been
> initialized (i.e. written to). To work around this issue, the first 8
> bytes are initialized here.
> 

Looks good.
Will do like that.

> > +        * bytes (whatever value for the second word) in Message RAM to
> > +        * avoid bit error for transmit data less than 4 bytes at the first
> > +        * time. By initializing the first 8 bytes of tx buffer before using
> > +        * it can avoid such issue.
> > +        */
> > +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
> > +       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
> > +
> >         m_can_config_endisable(priv, false);
> >  }
> 
> Can you trigger the issue when sending CAN-FD frames with dlc > 8 && dlc
> < 64?
> 

No, i did not see the issue with dlc > 8 && dlc < 64.

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

Re: [PATCH 7/7] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Tue, Nov 04, 2014 at 02:13:30PM +0100, Oliver Hartkopp wrote:
> 
> 
> On 04.11.2014 11:33, Marc Kleine-Budde wrote:
> >On 11/04/2014 10:27 AM, Dong Aisheng wrote:
> 
> 
> >>+       /* We meet an IC issue that we have to write the full 8
> >
> >At least on the *insert SoC name here*, an issue with the Message RAM
> >was discovered. Sending CAN frames with dlc less than 4 bytes will lead
> >to bit errors, when the first 8 bytes of the Message RAM have not been
> >initialized (i.e. written to). To work around this issue, the first 8
> >bytes are initialized here.
> 
> Yes. Also put the current IP revision (3.0.x) into the comment.
> Did inform the Bosch guys from this issue - or is it already in some errata sheet?
> 

Good idea, will add it.
I will try if we can talk Bosch guys about this issue.

Regards
Dong Aisheng

> >
> >>+        * bytes (whatever value for the second word) in Message RAM to
> >>+        * avoid bit error for transmit data less than 4 bytes at the first
> >>+        * time. By initializing the first 8 bytes of tx buffer before using
> >>+        * it can avoid such issue.
> >>+        */
> >>+       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
> >>+       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
> >>+
> >>         m_can_config_endisable(priv, false);
> >>  }
> >
> >Can you trigger the issue when sending CAN-FD frames with dlc > 8 && dlc
> >< 64?
> 
> Just a nitpick:
> 
> DLC can just be 0 .. 15
> 
> and the length (struct canfd_frame.len) can be from 0 .. 64
> 
> See:
> 
> http://lxr.free-electrons.com/source/include/uapi/linux/can.h#L83
> 
> That's the reason for all these helpers
> 
> http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L36
> 
> that hide the evil "DLC" from userspace now and make 'len' a usable
> loop variable as we were able to use the former dlc for classic CAN
> :-)
> 
> Regards,
> Oliver
>