[PATCH V3 1/3] can: add can_is_canfd_skb() API

[PATCH V3 1/3] can: add can_is_canfd_skb() API

[Dong Aisheng]

The CAN device drivers can use it to check if the frame to send is on
CAN FD mode or normal CAN mode.

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
ChangesLog:
 * v1->v2: change to skb->len == CANFD_MTU;
---
 include/linux/can/dev.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6992afc..4c3919c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -99,6 +99,11 @@ inval_skb:
 	return 1;
 }
 
+static inline int can_is_canfd_skb(struct sk_buff *skb)
+{
+	return skb->len == CANFD_MTU;
+}
+
 /* get data length from can_dlc with sanitized can_dlc */
 u8 can_dlc2len(u8 can_dlc);
 
-- 
1.9.1

[PATCH V3 2/3] 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.
1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
   up to 64 bytes payload. It's backward compatible with old 8 bytes
   normal CAN frame.
2) Allocate can frame or canfd frame based on EDL bit
3) Bitrate Switch function is disabled by default and will be enabled
   according to CANFD_BRS bit in cf->flags.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
ChangeLog:
v2->v3:
 * integrate patch 4 into patch 1 in last series(allow to send std frame
   on can fd mode)
 * use suitable API to get cf->len according to RX_BUF_EDL bit
v1->v2:
 * Allocate can frame or canfd frame based on EDL bit
 * Only check and set RTR bit for normal frame (no EDL bit set)
---
 drivers/net/can/m_can/m_can.c | 179 ++++++++++++++++++++++++++++++++----------
 1 file changed, 136 insertions(+), 43 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 6160b9c..eee1533 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,41 +357,68 @@ 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,
-			    u32 rxfs)
+static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 {
+	struct net_device_stats *stats = &dev->stats;
 	struct m_can_priv *priv = netdev_priv(dev);
-	u32 id, fgi;
+	struct canfd_frame *cf;
+	struct sk_buff *skb;
+	u32 id, fgi, dlc;
+	int i;
 
 	/* calculate the fifo get index for where to read data */
 	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
+	dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
+	if (dlc & RX_BUF_EDL)
+		skb = alloc_canfd_skb(dev, &cf);
+	else
+		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
+	if (!skb) {
+		stats->rx_dropped++;
+		return;
+	}
+
 	id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
 	if (id & RX_BUF_XTD)
 		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
 		cf->can_id = (id >> 18) & CAN_SFF_MASK;
 
-	if (id & RX_BUF_RTR) {
+	if (id & RX_BUF_ESI) {
+		cf->flags |= CANFD_ESI;
+		netdev_dbg(dev, "ESI Error\n");
+	}
+
+	if (!(dlc & RX_BUF_EDL) && (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));
+		if (dlc & RX_BUF_EDL)
+			cf->len = can_dlc2len((id >> 16) & 0x0F);
+		else
+			cf->len = get_can_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 */
 	m_can_write(priv, M_CAN_RXF0A, fgi);
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->len;
+
+	netif_receive_skb(skb);
 }
 
 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 pkts = 0;
 	u32 rxfs;
 
@@ -375,18 +432,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);
-		if (!skb) {
-			stats->rx_dropped++;
-			return pkts;
-		}
-
-		m_can_read_fifo(dev, frame, rxfs);
-
-		stats->rx_packets++;
-		stats->rx_bytes += frame->can_dlc;
-
-		netif_receive_skb(skb);
+		m_can_read_fifo(dev, rxfs);
 
 		quota--;
 		pkts++;
@@ -744,10 +790,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 +817,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 +847,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 +857,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 +873,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 +886,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 +953,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 +1042,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 +1063,28 @@ 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 (can_is_canfd_skb(skb)) {
+			if (cf->flags & CANFD_BRS)
+				cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
+			else
+				cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
+		} else {
+			cccr |= CCCR_CMR_CAN << 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

Re: [PATCH V3 2/3] can: m_can: update to support CAN FD features

[Marc Kleine-Budde]

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

On 11/05/2014 02:16 PM, Dong Aisheng wrote:
> 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.
> 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
>    up to 64 bytes payload. It's backward compatible with old 8 bytes
>    normal CAN frame.
> 2) Allocate can frame or canfd frame based on EDL bit
> 3) Bitrate Switch function is disabled by default and will be enabled
>    according to CANFD_BRS bit in cf->flags.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

I'm waiting on Oliver's Ack on this patch, then I'll apply 1+2.

regards,
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 V3 2/3] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

Looks good to me now.

Thanks for your patience.

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

On 05.11.2014 15:31, Marc Kleine-Budde wrote:
> On 11/05/2014 02:16 PM, Dong Aisheng wrote:
>> 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.
>> 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
>>     up to 64 bytes payload. It's backward compatible with old 8 bytes
>>     normal CAN frame.
>> 2) Allocate can frame or canfd frame based on EDL bit
>> 3) Bitrate Switch function is disabled by default and will be enabled
>>     according to CANFD_BRS bit in cf->flags.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>
> I'm waiting on Oliver's Ack on this patch, then I'll apply 1+2.
>
> regards,
> Marc
>

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

[Dong Aisheng]

At least on the i.MX6SX TO1.2 with M_CAN IP version 3.0.1, 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 in open()
function.

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>
---
ChangeLog
v2->v3:
 * add i.MX chip version in issue in commit message
v1->v2:
 * initialize the first 8 bytes of Tx Buffer of Message RAM in open()
   to workaround the issue
---
 drivers/net/can/m_can/m_can.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index eee1533..567cd27 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -905,6 +905,16 @@ static void m_can_chip_config(struct net_device *dev)
 	/* set bittiming params */
 	m_can_set_bittiming(dev);
 
+	/* At least on the i.MX6SX TO1.2 with M_CAN IP version 3.0.1,
+	 * (CREL = 30130506) 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.
+	 */
+	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);
 }
 
-- 
1.9.1

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

[Marc Kleine-Budde]

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

On 11/05/2014 02:16 PM, Dong Aisheng wrote:
> At least on the i.MX6SX TO1.2 with M_CAN IP version 3.0.1, 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 in open()
> function.
> 
> 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>

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

M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Oliver Hartkopp]

Hi all,

just to close this application note relevant point ...

I got an answer from Florian Hartwich (Mr. CAN) from Bosch regarding the bit 
error detection found by Dong Aisheng.

The relevant interrupts IR.BEU or IR.BEC monitor the message RAM:

Bit 21 BEU: Bit Error Uncorrected
Message RAM bit error detected, uncorrected. Controlled by input signal 
m_can_aeim_berr[1] generated by an optional external parity / ECC logic 
attached to the Message RAM. An uncorrected Message RAM bit error sets 
CCCR.INIT to ‘1’. This is done to avoid transmission of corrupted data.

0= No bit error detected when reading from Message RAM
1= Bit error detected, uncorrected (e.g. parity logic)

Bit 20 BEC: Bit Error Corrected
Message RAM bit error detected and corrected. Controlled by input signal 
m_can_aeim_berr[0] generated by an optional external parity / ECC logic 
attached to the Message RAM.

0= No bit error detected when reading from Message RAM
1= Bit error detected and corrected (e.g. ECC)

---

The Message RAM is usually equipped with a parity or ECC functionality.
But RAM cells suffer a hardware reset and can therefore hold arbitrary content 
at startup - including parity and/or ECC bits.

So when you write only the CAN ID and the first four bytes the last four bytes 
remain untouched. Then the M_CAN starts to read in 32bit words from the start 
of the Tx Message element. So it is very likely to trigger the message RAM 
error when reading the uninitialized 32bit word from the last four bytes.

Finally it turns out that an initial writing (with any kind of data) to the 
entire message RAM is mandatory to create valid parity/ECC checksums.

That's it.

Regards,
Oliver

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Wed, Nov 05, 2014 at 07:15:10PM +0100, Oliver Hartkopp wrote:
> Hi all,
> 
> just to close this application note relevant point ...
> 
> I got an answer from Florian Hartwich (Mr. CAN) from Bosch regarding
> the bit error detection found by Dong Aisheng.
> 
> The relevant interrupts IR.BEU or IR.BEC monitor the message RAM:
> 
> Bit 21 BEU: Bit Error Uncorrected
> Message RAM bit error detected, uncorrected. Controlled by input
> signal m_can_aeim_berr[1] generated by an optional external parity /
> ECC logic attached to the Message RAM. An uncorrected Message RAM
> bit error sets CCCR.INIT to ‘1’. This is done to avoid transmission
> of corrupted data.
> 
> 0= No bit error detected when reading from Message RAM
> 1= Bit error detected, uncorrected (e.g. parity logic)
> 
> Bit 20 BEC: Bit Error Corrected
> Message RAM bit error detected and corrected. Controlled by input
> signal m_can_aeim_berr[0] generated by an optional external parity /
> ECC logic attached to the Message RAM.
> 
> 0= No bit error detected when reading from Message RAM
> 1= Bit error detected and corrected (e.g. ECC)
> 
> ---
> 
> The Message RAM is usually equipped with a parity or ECC functionality.
> But RAM cells suffer a hardware reset and can therefore hold
> arbitrary content at startup - including parity and/or ECC bits.
> 
> So when you write only the CAN ID and the first four bytes the last
> four bytes remain untouched. Then the M_CAN starts to read in 32bit
> words from the start of the Tx Message element. So it is very likely
> to trigger the message RAM error when reading the uninitialized
> 32bit word from the last four bytes.
> 
> Finally it turns out that an initial writing (with any kind of data)
> to the entire message RAM is mandatory to create valid parity/ECC
> checksums.
> 
> That's it.
> 

Thanks for sharing this information.
Does it mean this issue is related to the nature of Message RAM and is
supposed to exist on all M_CAN IP versions?

> Regards,
> Oliver
> 

Regards
Dong Aisheng

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Oliver Hartkopp]

On 06.11.2014 02:57, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 07:15:10PM +0100, Oliver Hartkopp wrote:

>> The Message RAM is usually equipped with a parity or ECC functionality.
>> But RAM cells suffer a hardware reset and can therefore hold
>> arbitrary content at startup - including parity and/or ECC bits.
>>
>> So when you write only the CAN ID and the first four bytes the last
>> four bytes remain untouched. Then the M_CAN starts to read in 32bit
>> words from the start of the Tx Message element. So it is very likely
>> to trigger the message RAM error when reading the uninitialized
>> 32bit word from the last four bytes.
>>
>> Finally it turns out that an initial writing (with any kind of data)
>> to the entire message RAM is mandatory to create valid parity/ECC
>> checksums.
>>
>> That's it.
>>
>
> Thanks for sharing this information.
> Does it mean this issue is related to the nature of Message RAM and is
> supposed to exist on all M_CAN IP versions?

 From what I know from the 3.1.x revision there's no change regarding IR.BRU 
and IR.BEC - so I would assume this to stay on all M_CAN IP revisions.

But after some sleep I wonder if this patch [3/3] would need an update too.

Writing to the TX message RAM is obviously no workaround but a valid and 
needed initialization process.

I would tend to make this patch:

---

can: m_can: add missing TX message RAM initialization

The M_CAN message RAM is usually equipped with a parity or ECC functionality.
But RAM cells suffer a hardware reset and can therefore hold arbitrary content 
at startup - including parity and/or ECC bits.

To prevent the M_CAN controller detecting checksum errors when reading 
potentially uninitialized TX message RAM content to transmit CAN frames the TX 
message RAM has to be written with (any kind of) initial data.

---

Then the code should memset() the entire TX FIFO element - and not only the 8 
data bytes we are addressing now.

Maybe it makes sense to send the entire updated patch set (3) again ...

[1/3] can: add can_is_canfd_skb() API
[2/3] can: m_can: update to support CAN FD features
[3/3] can: m_can: add missing message RAM initialization

Are you ok with that?

Regards,
Oliver

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Thu, Nov 06, 2014 at 08:04:17AM +0100, Oliver Hartkopp wrote:
> On 06.11.2014 02:57, Dong Aisheng wrote:
> >On Wed, Nov 05, 2014 at 07:15:10PM +0100, Oliver Hartkopp wrote:
> 
> >>The Message RAM is usually equipped with a parity or ECC functionality.
> >>But RAM cells suffer a hardware reset and can therefore hold
> >>arbitrary content at startup - including parity and/or ECC bits.
> >>
> >>So when you write only the CAN ID and the first four bytes the last
> >>four bytes remain untouched. Then the M_CAN starts to read in 32bit
> >>words from the start of the Tx Message element. So it is very likely
> >>to trigger the message RAM error when reading the uninitialized
> >>32bit word from the last four bytes.
> >>
> >>Finally it turns out that an initial writing (with any kind of data)
> >>to the entire message RAM is mandatory to create valid parity/ECC
> >>checksums.
> >>
> >>That's it.
> >>
> >
> >Thanks for sharing this information.
> >Does it mean this issue is related to the nature of Message RAM and is
> >supposed to exist on all M_CAN IP versions?
> 
> From what I know from the 3.1.x revision there's no change regarding
> IR.BRU and IR.BEC - so I would assume this to stay on all M_CAN IP
> revisions.
> 
> But after some sleep I wonder if this patch [3/3] would need an update too.
> 
> Writing to the TX message RAM is obviously no workaround but a valid
> and needed initialization process.
> 
> I would tend to make this patch:
> 
> ---
> 
> can: m_can: add missing TX message RAM initialization
> 
> The M_CAN message RAM is usually equipped with a parity or ECC functionality.
> But RAM cells suffer a hardware reset and can therefore hold
> arbitrary content at startup - including parity and/or ECC bits.
> 
> To prevent the M_CAN controller detecting checksum errors when
> reading potentially uninitialized TX message RAM content to transmit
> CAN frames the TX message RAM has to be written with (any kind of)
> initial data.
> 

The key point of the issue is that why M_CAN will read potentially uninitialized
TX message RAM content which should not happen?
e.g. for our case of the issue, if we sending a no data frame or a less
than 4 bytes frame, why m_can will read extra 4 bytes uninitialized/unset
data which seems not reasonable?

From IP code logic, it will read full 8 bytes of data no matter how many data
actually to be transfered which is strange.

For sending data over 4 bytes, since the Message RAM content will be filled
with the real data to be transfered so there's no such issue.

> ---
> 
> Then the code should memset() the entire TX FIFO element - and not
> only the 8 data bytes we are addressing now.
> 

Our IC guy told me the issue only happened on transferring a data size
of less than 4 bytes and my test also proved that.
So i'm not sure memset() the entire TX FIFO element is neccesary...

Do you think we could keep the current solution firstly and updated later
if needed?

> Maybe it makes sense to send the entire updated patch set (3) again ...
> 
> [1/3] can: add can_is_canfd_skb() API
> [2/3] can: m_can: update to support CAN FD features
> [3/3] can: m_can: add missing message RAM initialization
> 
> Are you ok with that?
> 
> Regards,
> Oliver
> 

Regards
Dong Aisheng

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Oliver Hartkopp]

On 06.11.2014 09:09, Dong Aisheng wrote:
> On Thu, Nov 06, 2014 at 08:04:17AM +0100, Oliver Hartkopp wrote:


>> To prevent the M_CAN controller detecting checksum errors when
>> reading potentially uninitialized TX message RAM content to transmit
>> CAN frames the TX message RAM has to be written with (any kind of)
>> initial data.
>>
>
> The key point of the issue is that why M_CAN will read potentially uninitialized
> TX message RAM content which should not happen?
> e.g. for our case of the issue, if we sending a no data frame or a less
> than 4 bytes frame, why m_can will read extra 4 bytes uninitialized/unset
> data which seems not reasonable?
>
>  From IP code logic, it will read full 8 bytes of data no matter how many data
> actually to be transfered which is strange.

Yes.

>
> For sending data over 4 bytes, since the Message RAM content will be filled
> with the real data to be transfered so there's no such issue.

E.g. think about the transfer of a CAN FD frame with 32 byte.
When you only fill up content until 28 byte the last four bytes still remain 
uninitialized.

Did you try this 28 byte use-case with an uninitialized TX message RAM ?

cansend can0 123##1001122334566778899AABBCCDDEEFF001122334566778899AABB

To me it looks too risky when we only initialize the first 8 byte.

>
>> ---
>>
>> Then the code should memset() the entire TX FIFO element - and not
>> only the 8 data bytes we are addressing now.
>>
>
> Our IC guy told me the issue only happened on transferring a data size
> of less than 4 bytes and my test also proved that.

'less than'?

So you might try to use 26 bytes too:

cansend can0 123##1001122334566778899AABBCCDDEEFF001122334566778899


> So i'm not sure memset() the entire TX FIFO element is neccesary...

It's no big deal - so we should be defensive here.
And memset() is not working as Marc pointed out in another mail.

So we would need to loop with

	m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i), 0x0);

>
> Do you think we could keep the current solution firstly and updated later
> if needed?

No :-)

I would like to have all data bytes to be written at startup.

Regards,
Oliver

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Marc Kleine-Budde]

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

On 11/06/2014 01:33 PM, Oliver Hartkopp wrote:
>> So i'm not sure memset() the entire TX FIFO element is neccesary...
> 
> It's no big deal - so we should be defensive here.
> And memset() is not working as Marc pointed out in another mail.
> 
> So we would need to loop with
> 
>     m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i), 0x0);
> 
>>
>> Do you think we could keep the current solution firstly and updated later
>> if needed?
> 
> No :-)
> 
> I would like to have all data bytes to be written at startup.

Me, too. As this happens only once during ifconfig up it should not hurt
performance, either send an incremental or new patch. I'll sort it out.

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: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Thu, Nov 06, 2014 at 05:22:17PM +0100, Oliver Hartkopp wrote:
> Hello all, (reduced the CC list a bit)
> 
> thankfully Mr. Hartwich from Bosch followed the mail threads I forwarded to
> him. It turns out that there's only ONE message RAM containing TX Buffers, RX
> Buffers and all the acceptance filter elements.
> 
> Due to our discussion the next M_CAN user manual will contain the attached
> application note (see marked lines in attached PDF).
> 
> Mr Hartwich wrote:
> 
> ---8<--- snip!
> 
> The RAM initialization issue is not limited to Tx Buffers, all RAM words need
> an initialization before they are read. If e.g. the CPU reads an uninitialized
> Rx Buffer before the M_CAN has stored a message into the buffer, that may
> trigger a RAM access error.
> 
> There is only one single Message RAM attached to the M_CAN,  which buffers
> are found where is configured via the registers SIDFC.FLSSA, XIDFC.FLESA,
> RXF0C.F0SA, RXF1C.F1SA, TXBC.TBSA, TXEFC.EFSA. The size of the area needed
> e.g. for the Tx Buffers depends on the number of Buffers and on the size of
> the buffers. Larger buffers are needed for the longer FD frames.
> 
> The M_CAN does not check whether the RAM areas overlap, so this check needs to
> be done in software. It is also possible that several M_CANs share one single
> RAM, so the overlap-check needs to consider also that case.
> 
> RAMs usually do not have a hardware-reset, so the RAM-cells, including the
> parity/ECC-bits (if exist) may be at invalid values. Any write to a RAM word
> will set the parity/ECC-bits to valid values. Therefore the entire RAM should
> be initialized by software after power-on, e.g. (simplistic):
> 
> for (i=RAM_start; i<=RAM_end; i++) RAM[i] = 0x00000000;
> 
> The size of the RAM may differ in different M_CAN implementations, the
> RAM is not part of the M_CAN IP module. There may be RAMs with parity,
> with ECC, or without protection. There may even be RAMs with hardware-reset.
> 
> ---8<--- snip!
> 
> So the recommendation is to initialize the entire message RAM (see PDF).
> 

Thanks for this information.
By the description i agree that it's reasonable and more safe to initialize
the entire message RAM.

Regards
Dong Aisheng

> Regards,
> Oliver
> 

new M_CAN IP rev 3.2.x documentation available - was: Re: M_CAN message RAM initialization AppNote

[Oliver Hartkopp]

Hi all,

the new M_CAN IP rev 3.2.x documentation is available:

http://www.bosch-semiconductors.de/en/ubk_semiconductors/ip_modules_3/produkttabelle_ip_modules/m_can_1/m_can.html

http://www.bosch-semiconductors.de/media/pdf_1/ipmodules_1/m_can_m_ttcan/mcan_users_manual_v320.pdf

One of the changes is the fact that the new Tx Buffer Element (see page 49) has a FDF and BRS bit.
So the switching between CAN and CAN FD is done per message without fiddling the configuration registers each time.

Best regards,
Oliver


On 06.11.2014 17:22, Oliver Hartkopp wrote:
> Hello all, (reduced the CC list a bit)
> 
> thankfully Mr. Hartwich from Bosch followed the mail threads I forwarded to
> him. It turns out that there's only ONE message RAM containing TX Buffers, RX
> Buffers and all the acceptance filter elements.
> 
> Due to our discussion the next M_CAN user manual will contain the attached
> application note (see marked lines in attached PDF).
> 
> Mr Hartwich wrote:
> 
> ---8<--- snip!
> 
> The RAM initialization issue is not limited to Tx Buffers, all RAM words need
> an initialization before they are read. If e.g. the CPU reads an uninitialized
> Rx Buffer before the M_CAN has stored a message into the buffer, that may
> trigger a RAM access error.
> 
> There is only one single Message RAM attached to the M_CAN,  which buffers
> are found where is configured via the registers SIDFC.FLSSA, XIDFC.FLESA,
> RXF0C.F0SA, RXF1C.F1SA, TXBC.TBSA, TXEFC.EFSA. The size of the area needed
> e.g. for the Tx Buffers depends on the number of Buffers and on the size of
> the buffers. Larger buffers are needed for the longer FD frames.
> 
> The M_CAN does not check whether the RAM areas overlap, so this check needs to
> be done in software. It is also possible that several M_CANs share one single
> RAM, so the overlap-check needs to consider also that case.
> 
> RAMs usually do not have a hardware-reset, so the RAM-cells, including the
> parity/ECC-bits (if exist) may be at invalid values. Any write to a RAM word
> will set the parity/ECC-bits to valid values. Therefore the entire RAM should
> be initialized by software after power-on, e.g. (simplistic):
> 
> for (i=RAM_start; i<=RAM_end; i++) RAM[i] = 0x00000000;
> 
> The size of the RAM may differ in different M_CAN implementations, the
> RAM is not part of the M_CAN IP module. There may be RAMs with parity,
> with ECC, or without protection. There may even be RAMs with hardware-reset.
> 
> ---8<--- snip!
> 
> So the recommendation is to initialize the entire message RAM (see PDF).
> 
> Regards,
> Oliver
> 

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Thu, Nov 06, 2014 at 01:47:20PM +0100, Marc Kleine-Budde wrote:
> On 11/06/2014 01:33 PM, Oliver Hartkopp wrote:
> >> So i'm not sure memset() the entire TX FIFO element is neccesary...
> > 
> > It's no big deal - so we should be defensive here.
> > And memset() is not working as Marc pointed out in another mail.
> > 
> > So we would need to loop with
> > 
> >     m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i), 0x0);
> > 
> >>
> >> Do you think we could keep the current solution firstly and updated later
> >> if needed?
> > 
> > No :-)
> > 
> > I would like to have all data bytes to be written at startup.
> 
> Me, too. As this happens only once during ifconfig up it should not hurt
> performance, either send an incremental or new patch. I'll sort it out.
> 

I will send a new patch for this.

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: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

On Thu, Nov 06, 2014 at 01:33:56PM +0100, Oliver Hartkopp wrote:
> On 06.11.2014 09:09, Dong Aisheng wrote:
> >On Thu, Nov 06, 2014 at 08:04:17AM +0100, Oliver Hartkopp wrote:
> 
> 
> >>To prevent the M_CAN controller detecting checksum errors when
> >>reading potentially uninitialized TX message RAM content to transmit
> >>CAN frames the TX message RAM has to be written with (any kind of)
> >>initial data.
> >>
> >
> >The key point of the issue is that why M_CAN will read potentially uninitialized
> >TX message RAM content which should not happen?
> >e.g. for our case of the issue, if we sending a no data frame or a less
> >than 4 bytes frame, why m_can will read extra 4 bytes uninitialized/unset
> >data which seems not reasonable?
> >
> > From IP code logic, it will read full 8 bytes of data no matter how many data
> >actually to be transfered which is strange.
> 
> Yes.
> 
> >
> >For sending data over 4 bytes, since the Message RAM content will be filled
> >with the real data to be transfered so there's no such issue.
> 
> E.g. think about the transfer of a CAN FD frame with 32 byte.
> When you only fill up content until 28 byte the last four bytes
> still remain uninitialized.
> 
> Did you try this 28 byte use-case with an uninitialized TX message RAM ?
> 
> cansend can0 123##1001122334566778899AABBCCDDEEFF001122334566778899AABB
> 
> To me it looks too risky when we only initialize the first 8 byte.
> 

I tried 28 byte case with two MX6SX SDB board and it worked.
See below:
Tx side:
root@imx6sxsabresd:~# cansend can0 123##1001122334566778899AABBC566778899AABB334
Rx side:
root@imx6sxsabresd:~# candump -x can0
  can0  RX B -  123  [32]  00 11 22 33 45 66 77 88 99 AA BB CC DD EE FF 00 11 22 33 45 66 77 88 99 AA BB 00 00 00 00 00 00

I think this is mainly because the driver will ensure to write
the full 32 bytes to Message RAM even we only fill up content of
28 bytes. The remain 4 bytes written to M_RAM are default 0.
This seems avoid the possibility of reading uninitialized TX message RAM
for transfer.

The code is done as follows:
for (i = 0; i < cf->len; i += 4)
        m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
                         *(u32 *)(cf->data + i));
cf->len will be rounded to 32 in cansend.

> >
> >>---
> >>
> >>Then the code should memset() the entire TX FIFO element - and not
> >>only the 8 data bytes we are addressing now.
> >>
> >
> >Our IC guy told me the issue only happened on transferring a data size
> >of less than 4 bytes and my test also proved that.
> 
> 'less than'?
> 

As i said before, from IP code logic, M_CAN will read extra data bytes
from TX buffer only for sending data less than 4 bytes.
e.g.
cansnd can0 123#
cansnd can0 123#112233
Both case will read the full 8 byte from TX buffer even it sends no data
and a 3 bytes data.

But
cansnd can0 123#1122334455
it read 8 bytes
cansnd can0 123##1112233445566778899001122
it read 12 bytes.
No extra uninitialized data read.

> So you might try to use 26 bytes too:
> 
> cansend can0 123##1001122334566778899AABBCCDDEEFF001122334566778899
> 
> 

It works too.

> >So i'm not sure memset() the entire TX FIFO element is neccesary...
> 
> It's no big deal - so we should be defensive here.
> And memset() is not working as Marc pointed out in another mail.
> 
> So we would need to loop with
> 
> 	m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i), 0x0);
> 

This simple loop may not work.
m_can_fifo_write is only for Tx Buffer.

Since Message RAM may be shared, we may want to initialize each part of
Message RAM used by this M_CAN controller.
Something like follows in probe() function:

/* initialize the entire Message RAM in use to avoid possible
* ECC/parity checksum errors when reading an uninitialized buffer
*/
start = priv->mcfg[MRAM_SIDF].off;
end = priv->mcfg[MRAM_TXB].off +
priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
for (i = start; i < end; i += 4)
        writel(0x0, priv->mram_base + i);

I will send a updated patch for this.

Regards
Dong Aisheng

> >
> >Do you think we could keep the current solution firstly and updated later
> >if needed?
> 
> No :-)
> 
> I would like to have all data bytes to be written at startup.
> 
> Regards,
> Oliver
> 

Re: M_CAN message RAM initialization AppNote - was: Re: [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes

[Marc Kleine-Budde]

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

On 11/06/2014 08:04 AM, Oliver Hartkopp wrote:
> On 06.11.2014 02:57, Dong Aisheng wrote:
>> On Wed, Nov 05, 2014 at 07:15:10PM +0100, Oliver Hartkopp wrote:
> 
>>> The Message RAM is usually equipped with a parity or ECC functionality.
>>> But RAM cells suffer a hardware reset and can therefore hold
>>> arbitrary content at startup - including parity and/or ECC bits.
>>>
>>> So when you write only the CAN ID and the first four bytes the last
>>> four bytes remain untouched. Then the M_CAN starts to read in 32bit
>>> words from the start of the Tx Message element. So it is very likely
>>> to trigger the message RAM error when reading the uninitialized
>>> 32bit word from the last four bytes.
>>>
>>> Finally it turns out that an initial writing (with any kind of data)
>>> to the entire message RAM is mandatory to create valid parity/ECC
>>> checksums.
>>>
>>> That's it.
>>>
>>
>> Thanks for sharing this information.
>> Does it mean this issue is related to the nature of Message RAM and is
>> supposed to exist on all M_CAN IP versions?
> 
> From what I know from the 3.1.x revision there's no change regarding
> IR.BRU and IR.BEC - so I would assume this to stay on all M_CAN IP
> revisions.
> 
> But after some sleep I wonder if this patch [3/3] would need an update too.
> 
> Writing to the TX message RAM is obviously no workaround but a valid and
> needed initialization process.
> 
> I would tend to make this patch:
> 
> ---
> 
> can: m_can: add missing TX message RAM initialization
> 
> The M_CAN message RAM is usually equipped with a parity or ECC
> functionality.
> But RAM cells suffer a hardware reset and can therefore hold arbitrary
> content at startup - including parity and/or ECC bits.
> 
> To prevent the M_CAN controller detecting checksum errors when reading
> potentially uninitialized TX message RAM content to transmit CAN frames
> the TX message RAM has to be written with (any kind of) initial data.
> 
> ---
> 
> Then the code should memset() the entire TX FIFO element - and not only
> the 8 data bytes we are addressing now.

No literal memset() as this is iomem

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 V3 1/3] can: add can_is_canfd_skb() API

[Eric Dumazet]

On Wed, 2014-11-05 at 21:16 +0800, Dong Aisheng wrote:
> The CAN device drivers can use it to check if the frame to send is on
> CAN FD mode or normal CAN mode.
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
> ChangesLog:
>  * v1->v2: change to skb->len == CANFD_MTU;
> ---
>  include/linux/can/dev.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 6992afc..4c3919c 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -99,6 +99,11 @@ inval_skb:
>  	return 1;
>  }
>  

This looks a bit strange to assume that skb->len == magical_value is CAN
FD. A comment would be nice.

> +static inline int can_is_canfd_skb(struct sk_buff *skb)

static inline bool can_is_canfd_skb(const struct sk_buff *skb)

> +{
> +	return skb->len == CANFD_MTU;
> +}
> +
>  /* get data length from can_dlc with sanitized can_dlc */
>  u8 can_dlc2len(u8 can_dlc);
>  

Re: [PATCH V3 1/3] can: add can_is_canfd_skb() API

[Oliver Hartkopp]

On 05.11.2014 17:22, Eric Dumazet wrote:
> On Wed, 2014-11-05 at 21:16 +0800, Dong Aisheng wrote:

>
> This looks a bit strange to assume that skb->len == magical_value is CAN
> FD. A comment would be nice.
>

Yes. Due to exactly two types of struct can(fd)_frame which can be contained 
in a skb the skbs are distinguished by the length which can be either CAN_MTU 
or CANFD_MTU.

>> +static inline int can_is_canfd_skb(struct sk_buff *skb)
>
> static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>

ok.

>> +{

What about:

	/* the CAN specific type of skb is identified by its data length */

>> +	return skb->len == CANFD_MTU;
>> +}
>> +
>>   /* get data length from can_dlc with sanitized can_dlc */
>>   u8 can_dlc2len(u8 can_dlc);

Regards,
Oliver

Re: [PATCH V3 1/3] can: add can_is_canfd_skb() API

[Dong Aisheng]

On Wed, Nov 05, 2014 at 06:33:09PM +0100, Oliver Hartkopp wrote:
> On 05.11.2014 17:22, Eric Dumazet wrote:
> >On Wed, 2014-11-05 at 21:16 +0800, Dong Aisheng wrote:
> 
> >
> >This looks a bit strange to assume that skb->len == magical_value is CAN
> >FD. A comment would be nice.
> >
> 
> Yes. Due to exactly two types of struct can(fd)_frame which can be
> contained in a skb the skbs are distinguished by the length which
> can be either CAN_MTU or CANFD_MTU.
> 
> >>+static inline int can_is_canfd_skb(struct sk_buff *skb)
> >
> >static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> >
> 
> ok.
> 

Got it.

> >>+{
> 
> What about:
> 
> 	/* the CAN specific type of skb is identified by its data length */
> 

Looks good to me.
I will send a updated version with these changes.

> >>+	return skb->len == CANFD_MTU;
> >>+}
> >>+
> >>  /* get data length from can_dlc with sanitized can_dlc */
> >>  u8 can_dlc2len(u8 can_dlc);
> 
> Regards,
> Oliver
>

Regards
Dong Aisheng