[PATCH V2 1/4] can: m_can: update to support CAN FD features

[PATCH V2 1/4] 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>
---
Changes Since V1:
 * 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 | 172 +++++++++++++++++++++++++++++++-----------
 1 file changed, 129 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..664fe30 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,65 @@ 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));
+		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 */
 	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 +429,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 +787,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 +814,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 +844,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 +854,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 +870,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 +883,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 +950,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 +1039,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 +1060,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 V2 2/4] can: m_can: workaround for transmit data less than 4 bytes

[Dong Aisheng]

At least on the i.MX6SX with M_CAN IP version 3.0.x, 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 at imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
[   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root at imx6qdlsolo:~# cansend can0 123#112233
[   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
ChangeLog since v1:
 * 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 664fe30..f47c200 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -902,6 +902,15 @@ static void m_can_chip_config(struct net_device *dev)
 	/* set bittiming params */
 	m_can_set_bittiming(dev);
 
+	/* At least on the i.MX6SX with M_CAN IP version 3.0.x, 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

[PATCH V1 3/4] 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.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 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..fe3be29 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->protocol == htons(ETH_P_CANFD);
+}
+
 /* get data length from can_dlc with sanitized can_dlc */
 u8 can_dlc2len(u8 can_dlc);
 
-- 
1.9.1

[PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode

[Dong Aisheng]

The current code sends all CAN frames on CAN FD format(with BRS or not)
if CAN_CTRLMODE_FD is enabled.
However, even CAN_CTRLMODE_FD is enabled, the can tool may still
send normal frames.
e.g.
ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on
cansend can0 123#112233

Therefore sending normal CAN frame on FD format seems not reasonable
and the CAN FD incapable device may not be able to receive it correctly.

The patch switches the M_CAN operation mode to ISO11898-1 instead of
staying on CAN FD operation mode by writing "11" to CMR bit if find
we're sending a normal can skb.

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f47c200..219c4b7 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1080,10 +1080,14 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 	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;
+		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);
 	}
 
-- 
1.9.1

[PATCH V1 3/4] can: add can_is_canfd_skb() API

[Oliver Hartkopp]

On 05.11.2014 08:58, 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.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>   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..fe3be29 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->protocol == htons(ETH_P_CANFD);

return skb->len == CANFD_MTU;

Please take the length as distinction as we do it in linux/net/can/ too.

You can add my

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

in the updated post then.

Regards,
Oliver

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

On 05.11.2014 08:58, Dong Aisheng wrote:

> @@ -327,41 +357,65 @@ 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);

Yes. That's the way it should look like ;-)

> +	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));
> +		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));

if (dlc & RX_BUF_EDL)
	cf->len = can_dlc2len((id >> 16) & 0x0F);
else
	cf->len = get_can_dlc((id >> 16) & 0x0F);

(..)

> @@ -804,7 +870,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 +883,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);
>

(..)

>
> +	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);
> +	}

Unfortunately No. Here it becomes complicated due to the fact that the 
revision 3.0.x does not support per-frame switching for FD/BRS ...

When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* tells us, that 
the controller is _capable_ to send either CAN or CAN FD frames.

It does not configure the controller into one of these specific settings!

Additionally: AFAIK when writing to the CCCR you have to make sure that 
there's currently no ongoing transfer.

I would suggest the following approach to make the revision 3.0.x act like a 
correct CAN FD capable controller:

- create a helper function to switch FD and BRS while taking care of ongoing 
transmissions

- create a variable that knows the current tx_mode for FD and BRS

When we need to send a CAN frame which doesn't fit the settings in the tx_mode 
we need to switch the CCCR and update the tx_mode variable.

This at least reduces the needed CCCR operations.

And it also addresses the intention of your patch
[PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode

Regards,
Oliver

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

[Marc Kleine-Budde]

On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with

Can you add the imx mask revision, too and the exact m_can version (is
available).

> 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 at imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root at imx6qdlsolo:~# cansend can0 123#112233
> [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
> ChangeLog since v1:
>  * 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 664fe30..f47c200 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -902,6 +902,15 @@ static void m_can_chip_config(struct net_device *dev)
>  	/* set bittiming params */
>  	m_can_set_bittiming(dev);
>  
> +	/* At least on the i.MX6SX with M_CAN IP version 3.0.x, 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);
>  }
>  
> 

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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141105/f7b96084/attachment-0001.sig>

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

[Dong Aisheng]

On Wed, Nov 05, 2014 at 11:17:48AM +0100, Marc Kleine-Budde wrote:
> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> > At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
> 
> Can you add the imx mask revision, too and the exact m_can version (is
> available).
> 

What do you mean imx mask revision?

By reading the Core Release Register (CREL), it seems the exact m_can
version is 3.0.1. (CREL = 30130506).

So what about changing to as follows?
"At least on the i.MX6SX with M_CAN IP version 3.0.1 (CREL = 30130506),
an issue with"

Regards
Dong Aisheng

> > 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 at imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> > [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> > root at imx6qdlsolo:~# cansend can0 123#112233
> > [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> > 
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> > ChangeLog since v1:
> >  * 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 | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 664fe30..f47c200 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -902,6 +902,15 @@ static void m_can_chip_config(struct net_device *dev)
> >  	/* set bittiming params */
> >  	m_can_set_bittiming(dev);
> >  
> > +	/* At least on the i.MX6SX with M_CAN IP version 3.0.x, 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);
> >  }
> >  
> > 
> 
> 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   |
> 

[PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode

[Marc Kleine-Budde]

On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> The current code sends all CAN frames on CAN FD format(with BRS or not)
> if CAN_CTRLMODE_FD is enabled.
> However, even CAN_CTRLMODE_FD is enabled, the can tool may still
> send normal frames.
> e.g.
> ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on
> cansend can0 123#112233
> 
> Therefore sending normal CAN frame on FD format seems not reasonable
> and the CAN FD incapable device may not be able to receive it correctly.
> 
> The patch switches the M_CAN operation mode to ISO11898-1 instead of
> staying on CAN FD operation mode by writing "11" to CMR bit if find
> we're sending a normal can skb.

With this patch applied and Olivre's version of 3/4, how does the
application send CAN-FD frames?
1. witch on CAN-FD via "ip fd on"
2. write a struct canfd_frame

Correct?

What happens if:
3. write a struct can_frame

A CAN frame is send?

Oliver are you okay with this behaviour?

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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141105/1a6e3879/attachment-0001.sig>

[PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode

[Oliver Hartkopp]

On 05.11.2014 11:41, Marc Kleine-Budde wrote:
> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
>> The current code sends all CAN frames on CAN FD format(with BRS or not)
>> if CAN_CTRLMODE_FD is enabled.
>> However, even CAN_CTRLMODE_FD is enabled, the can tool may still
>> send normal frames.
>> e.g.
>> ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on
>> cansend can0 123#112233
>>
>> Therefore sending normal CAN frame on FD format seems not reasonable
>> and the CAN FD incapable device may not be able to receive it correctly.
>>
>> The patch switches the M_CAN operation mode to ISO11898-1 instead of
>> staying on CAN FD operation mode by writing "11" to CMR bit if find
>> we're sending a normal can skb.
>
> With this patch applied and Olivre's version of 3/4, how does the
> application send CAN-FD frames?

This patch becomes obsolete when we do it like in my answer of [3/4].

> 1. witch on CAN-FD via "ip fd on"

With

ip link set can0 type can fd on

the netdevice switches to the MTU of CAN FD (72) instead of 16.

This means that this netdevice can handle CAN frames (MTU 16) and CAN FD 
frames (MTU 72).

When you send a standard CAN frame, e.g.

	cansend can0 123#112233

you would get a CAN 2.0 frame (dlc = 3) on the bus.

When you send a CAN FD frame, e.g.

	cansend can0 123##0112233

you would get a CAN FD frame (dlc = 3) on the bus.

With

	cansend can0 123##1112233

you would get a CAN FD frame (dlc = 3) with BRS on the bus.

Whether it is CAN or CAN FD is given by checking skb->len for CAN_MTU of 
CANFD_MTU in the driver.

Regards,
Oliver


> 2. write a struct canfd_frame
>
> Correct?
>
> What happens if:
> 3. write a struct can_frame
>
> A CAN frame is send?
>
> Oliver are you okay with this behaviour?
>
> Marc
>

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> On 05.11.2014 08:58, Dong Aisheng wrote:
> 
> >@@ -327,41 +357,65 @@ 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);
> 
> Yes. That's the way it should look like ;-)
> 
> >+	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));
> >+		cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F));
> 
> if (dlc & RX_BUF_EDL)
> 	cf->len = can_dlc2len((id >> 16) & 0x0F);
> else
> 	cf->len = get_can_dlc((id >> 16) & 0x0F);
> 
> (..)
> 

Correct.
Will change it.

> >@@ -804,7 +870,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 +883,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);
> >
> 
> (..)
> 
> >
> >+	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);
> >+	}
> 
> Unfortunately No. Here it becomes complicated due to the fact that
> the revision 3.0.x does not support per-frame switching for FD/BRS
> ...

I'm not sure i got your point.
>From m_can spec, it allows switch CAN mode by setting CMR bit.

Bits 11:10 CMR[1:0]: CAN Mode Request
A change of the CAN operation mode is requested by writing to this bit field. After change to the
requested operation mode the bit field is reset to ?00? and the status flags FDBS and FDO are set
accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is
retained until it is overwritten by the next mode change request. In case CME = ?01?/?10?/?11? a
change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation
according to ISO11898-1.
00 unchanged
01 Request CAN FD operation
10 Request CAN FD operation with bit rate switching
11 Request CAN operation according ISO11898-1

So what's the difference between this function and the per-frame switching
you mentioned?

> 
> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
> tells us, that the controller is _capable_ to send either CAN or CAN
> FD frames.
> 
> It does not configure the controller into one of these specific settings!
> 
> Additionally: AFAIK when writing to the CCCR you have to make sure
> that there's currently no ongoing transfer.
> 

I don't know about it before.
By searching m_can user manual v302 again, i still did not find this
limitation. Can you point me if you know where it is?

BTW, since we only use one Tx Buffer for transmission currently, so we
should not meet that case that CAN mode is switched during transfer.
So the issue you concern may not happen.

Additionally it looks to me like m_can does allow set CMR bit at runtime
since the mode change will take affect when controller becomes idle.
See below:

"A mode change requested by writing to CCCR.CMR will be executed next
time the CAN protocol controller FSM reaches idle phase between CAN frames.
Upon this event CCCR.CMR is reset to ?00? and the status flags CCCR.FDBS
and CCCR.FDO are set accordingly. In case the requested
CAN operation mode is not enabled, the value written to CCCR.CMR is
retained until it is overwritten by the next mode change request.
Default is CAN operation according to ISO11898-1."

> I would suggest the following approach to make the revision 3.0.x
> act like a correct CAN FD capable controller:
> 
> - create a helper function to switch FD and BRS while taking care of
> ongoing transmissions
> 
> - create a variable that knows the current tx_mode for FD and BRS
> 
> When we need to send a CAN frame which doesn't fit the settings in
> the tx_mode we need to switch the CCCR and update the tx_mode
> variable.
> 
> This at least reduces the needed CCCR operations.
> 

Yes, i can do like that.
But what i see by doing that is only reduces the needed CCCR operations?
Any other benefits i missed?

And the test for current code showed it seemed work well on the Mode
switch among std frame/fd frame/brs frame.

Regards
Dong Aisheng

> And it also addresses the intention of your patch
> [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode
> 
> Regards,
> Oliver
> 

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

[Dong Aisheng]

On Wed, Nov 05, 2014 at 12:32:21PM +0100, Marc Kleine-Budde wrote:
> On 11/05/2014 11:33 AM, Dong Aisheng wrote:
> > On Wed, Nov 05, 2014 at 11:17:48AM +0100, Marc Kleine-Budde wrote:
> >> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
> >>> At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
> >>
> >> Can you add the imx mask revision, too and the exact m_can version (is
> >> available).
> >>
> > 
> > What do you mean imx mask revision?
> 
> In my imx6sdl/imx6q data sheet it's called: Chip Silicon Version
> (USB_ANALOG_DIGPROG), I expect that the imx6sx has a the same register.
> 
> > By reading the Core Release Register (CREL), it seems the exact m_can
> > version is 3.0.1. (CREL = 30130506).
> > 
> > So what about changing to as follows?
> > "At least on the i.MX6SX with M_CAN IP version 3.0.1 (CREL = 30130506),
> > an issue with"
> 
> Yes, please ass the imx silicon revision, too.
> 

Okay, it's i.MX6SX TO1.2. Will add 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   |
> 

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

[Marc Kleine-Budde]

On 11/05/2014 11:33 AM, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 11:17:48AM +0100, Marc Kleine-Budde wrote:
>> On 11/05/2014 08:58 AM, Dong Aisheng wrote:
>>> At least on the i.MX6SX with M_CAN IP version 3.0.x, an issue with
>>
>> Can you add the imx mask revision, too and the exact m_can version (is
>> available).
>>
> 
> What do you mean imx mask revision?

In my imx6sdl/imx6q data sheet it's called: Chip Silicon Version
(USB_ANALOG_DIGPROG), I expect that the imx6sx has a the same register.

> By reading the Core Release Register (CREL), it seems the exact m_can
> version is 3.0.1. (CREL = 30130506).
> 
> So what about changing to as follows?
> "At least on the i.MX6SX with M_CAN IP version 3.0.1 (CREL = 30130506),
> an issue with"

Yes, please ass the imx silicon revision, too.

Marc

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141105/b6ee209c/attachment.sig>

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Varka Bhadram]

Hi Dong Aisheng,

On 11/05/2014 01:28 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>

In these four patches two of them are V1 and other two are V2.

Is there any reason..?

How will you be able to generate like this by git format-patch..?

-- 
Thanks and Regards,
Varka Bhadram.

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Wed, Nov 05, 2014 at 05:05:11PM +0530, Varka Bhadram wrote:
> Hi Dong Aisheng,
> 
> On 11/05/2014 01:28 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>
> 
> In these four patches two of them are V1 and other two are V2.
> 
> Is there any reason..?
> 

Sorry for confusion.
Because some patches are already picked by Macr on the first round.
See below:
http://www.spinics.net/lists/netdev/msg302133.html

> How will you be able to generate like this by git format-patch..?
> 

Generated as v2 and i manually changed the later two to v1 since they
are new.
Not sure if it's suitable to do like that. :-)

Regards
Dong Aisheng

> -- 
> Thanks and Regards,
> Varka Bhadram.
> 

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Wed, Nov 05, 2014 at 02:10:05PM +0100, Oliver Hartkopp wrote:
> On 05.11.2014 12:26, Dong Aisheng wrote:
> >On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> >>On 05.11.2014 08:58, Dong Aisheng wrote:
> 
> 
> >>Unfortunately No. Here it becomes complicated due to the fact that
> >>the revision 3.0.x does not support per-frame switching for FD/BRS
> >>...
> >
> >I'm not sure i got your point.
> > From m_can spec, it allows switch CAN mode by setting CMR bit.
> >
> >Bits 11:10 CMR[1:0]: CAN Mode Request
> >A change of the CAN operation mode is requested by writing to this bit field. After change to the
> >requested operation mode the bit field is reset to ?00? and the status flags FDBS and FDO are set
> >accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is
> >retained until it is overwritten by the next mode change request. In case CME = ?01?/?10?/?11? a
> >change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation
> >according to ISO11898-1.
> >00 unchanged
> >01 Request CAN FD operation
> >10 Request CAN FD operation with bit rate switching
> >11 Request CAN operation according ISO11898-1
> >
> >So what's the difference between this function and the per-frame switching
> >you mentioned?
> >
> >>
> >>When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
> >>tells us, that the controller is _capable_ to send either CAN or CAN
> >>FD frames.
> >>
> >>It does not configure the controller into one of these specific settings!
> >>
> >>Additionally: AFAIK when writing to the CCCR you have to make sure
> >>that there's currently no ongoing transfer.
> >>
> >
> >I don't know about it before.
> >By searching m_can user manual v302 again, i still did not find this
> >limitation. Can you point me if you know where it is?
> >
> >BTW, since we only use one Tx Buffer for transmission currently, so we
> >should not meet that case that CAN mode is switched during transfer.
> >So the issue you concern may not happen.
> 
> Yes. You are right. Having a FIFO with a size of 1 will help here :-)
> 
> >
> >Additionally it looks to me like m_can does allow set CMR bit at runtime
> >since the mode change will take affect when controller becomes idle.
> >See below:
> >
> >"A mode change requested by writing to CCCR.CMR will be executed next
> >time the CAN protocol controller FSM reaches idle phase between CAN frames.
> >Upon this event CCCR.CMR is reset to ?00? and the status flags CCCR.FDBS
> >and CCCR.FDO are set accordingly. In case the requested
> >CAN operation mode is not enabled, the value written to CCCR.CMR is
> >retained until it is overwritten by the next mode change request.
> >Default is CAN operation according to ISO11898-1."
> 
> Ah. I assumed we have to take care to set these bits in the idle state.
> 
> So when thinking about the one and only tx buffer your current
> approach should work indeed. Sorry for my confusion.
> 
> >
> >>I would suggest the following approach to make the revision 3.0.x
> >>act like a correct CAN FD capable controller:
> >>
> >>- create a helper function to switch FD and BRS while taking care of
> >>ongoing transmissions
> >>
> >>- create a variable that knows the current tx_mode for FD and BRS
> >>
> >>When we need to send a CAN frame which doesn't fit the settings in
> >>the tx_mode we need to switch the CCCR and update the tx_mode
> >>variable.
> >>
> >>This at least reduces the needed CCCR operations.
> >>
> >
> >Yes, i can do like that.
> >But what i see by doing that is only reduces the needed CCCR operations?
> >Any other benefits i missed?
> 
> No. I just thought about a function that also takes care of the idle
> phase to switch the bits. But now you made it clear that this is not
> needed - so you can stay on your current implementation: Setting the
> CCCR each time before sending the frame.
> 

Okay

> With the 3.1.x or 3.2.x this code will become obsolete then as the
> EDL and BRS seeting will get extra bits in the Tx Buffer.
> But that's a future point.
> 

Got it.
Will update it in the future when the new IP doc is available.

> >And the test for current code showed it seemed work well on the Mode
> >switch among std frame/fd frame/brs frame.
> 
> Fine.
> 
> Btw. I would suggest to integrate patch [4/4] into [3/4].
> 

Yes, will do it.
Thanks

Regards
Dong Aisheng

> Best regards,
> Oliver
> 

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Wed, Nov 05, 2014 at 02:15:27PM +0100, Oliver Hartkopp wrote:
> Typo
> 
> On 05.11.2014 14:10, Oliver Hartkopp wrote:
> 
> >Btw. I would suggest to integrate patch [4/4] into [3/4].
> 
> into [1/4] of course

Understand! :-)

Regards
Dong Aisheng

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

On 05.11.2014 12:26, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
>> On 05.11.2014 08:58, Dong Aisheng wrote:


>> Unfortunately No. Here it becomes complicated due to the fact that
>> the revision 3.0.x does not support per-frame switching for FD/BRS
>> ...
>
> I'm not sure i got your point.
>  From m_can spec, it allows switch CAN mode by setting CMR bit.
>
> Bits 11:10 CMR[1:0]: CAN Mode Request
> A change of the CAN operation mode is requested by writing to this bit field. After change to the
> requested operation mode the bit field is reset to ?00? and the status flags FDBS and FDO are set
> accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is
> retained until it is overwritten by the next mode change request. In case CME = ?01?/?10?/?11? a
> change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation
> according to ISO11898-1.
> 00 unchanged
> 01 Request CAN FD operation
> 10 Request CAN FD operation with bit rate switching
> 11 Request CAN operation according ISO11898-1
>
> So what's the difference between this function and the per-frame switching
> you mentioned?
>
>>
>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
>> tells us, that the controller is _capable_ to send either CAN or CAN
>> FD frames.
>>
>> It does not configure the controller into one of these specific settings!
>>
>> Additionally: AFAIK when writing to the CCCR you have to make sure
>> that there's currently no ongoing transfer.
>>
>
> I don't know about it before.
> By searching m_can user manual v302 again, i still did not find this
> limitation. Can you point me if you know where it is?
>
> BTW, since we only use one Tx Buffer for transmission currently, so we
> should not meet that case that CAN mode is switched during transfer.
> So the issue you concern may not happen.

Yes. You are right. Having a FIFO with a size of 1 will help here :-)

>
> Additionally it looks to me like m_can does allow set CMR bit at runtime
> since the mode change will take affect when controller becomes idle.
> See below:
>
> "A mode change requested by writing to CCCR.CMR will be executed next
> time the CAN protocol controller FSM reaches idle phase between CAN frames.
> Upon this event CCCR.CMR is reset to ?00? and the status flags CCCR.FDBS
> and CCCR.FDO are set accordingly. In case the requested
> CAN operation mode is not enabled, the value written to CCCR.CMR is
> retained until it is overwritten by the next mode change request.
> Default is CAN operation according to ISO11898-1."

Ah. I assumed we have to take care to set these bits in the idle state.

So when thinking about the one and only tx buffer your current approach should 
work indeed. Sorry for my confusion.

>
>> I would suggest the following approach to make the revision 3.0.x
>> act like a correct CAN FD capable controller:
>>
>> - create a helper function to switch FD and BRS while taking care of
>> ongoing transmissions
>>
>> - create a variable that knows the current tx_mode for FD and BRS
>>
>> When we need to send a CAN frame which doesn't fit the settings in
>> the tx_mode we need to switch the CCCR and update the tx_mode
>> variable.
>>
>> This at least reduces the needed CCCR operations.
>>
>
> Yes, i can do like that.
> But what i see by doing that is only reduces the needed CCCR operations?
> Any other benefits i missed?

No. I just thought about a function that also takes care of the idle phase to 
switch the bits. But now you made it clear that this is not needed - so you 
can stay on your current implementation: Setting the CCCR each time before 
sending the frame.

With the 3.1.x or 3.2.x this code will become obsolete then as the EDL and BRS 
seeting will get extra bits in the Tx Buffer.
But that's a future point.

> And the test for current code showed it seemed work well on the Mode
> switch among std frame/fd frame/brs frame.

Fine.

Btw. I would suggest to integrate patch [4/4] into [3/4].

Best regards,
Oliver

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

Typo

On 05.11.2014 14:10, Oliver Hartkopp wrote:

> Btw. I would suggest to integrate patch [4/4] into [3/4].

into [1/4] of course

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Marc Kleine-Budde]

On 11/05/2014 02:10 PM, Oliver Hartkopp wrote:
> On 05.11.2014 12:26, Dong Aisheng wrote:
>> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
>>> On 05.11.2014 08:58, Dong Aisheng wrote:
> 
> 
>>> Unfortunately No. Here it becomes complicated due to the fact that
>>> the revision 3.0.x does not support per-frame switching for FD/BRS
>>> ...
>>
>> I'm not sure i got your point.
>>  From m_can spec, it allows switch CAN mode by setting CMR bit.
>>
>> Bits 11:10 CMR[1:0]: CAN Mode Request
>> A change of the CAN operation mode is requested by writing to this bit
>> field. After change to the
>> requested operation mode the bit field is reset to ?00? and the status
>> flags FDBS and FDO are set
>> accordingly. In case the requested CAN operation mode is not enabled,
>> the value written to CMR is
>> retained until it is overwritten by the next mode change request. In
>> case CME = ?01?/?10?/?11? a
>> change to CAN operation according to ISO 11898-1 is always possible.
>> Default is CAN operation
>> according to ISO11898-1.
>> 00 unchanged
>> 01 Request CAN FD operation
>> 10 Request CAN FD operation with bit rate switching
>> 11 Request CAN operation according ISO11898-1
>>
>> So what's the difference between this function and the per-frame
>> switching
>> you mentioned?
>>
>>>
>>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
>>> tells us, that the controller is _capable_ to send either CAN or CAN
>>> FD frames.
>>>
>>> It does not configure the controller into one of these specific
>>> settings!
>>>
>>> Additionally: AFAIK when writing to the CCCR you have to make sure
>>> that there's currently no ongoing transfer.
>>>
>>
>> I don't know about it before.
>> By searching m_can user manual v302 again, i still did not find this
>> limitation. Can you point me if you know where it is?
>>
>> BTW, since we only use one Tx Buffer for transmission currently, so we
>> should not meet that case that CAN mode is switched during transfer.
>> So the issue you concern may not happen.
> 
> Yes. You are right. Having a FIFO with a size of 1 will help here :-)

Errrr....sorry...no.

Taking an easy route here but making it x times harder to extend the
driver to make use of the FIFO is not an option.

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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141105/d1e64e16/attachment-0001.sig>

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Dong Aisheng]

On Wed, Nov 05, 2014 at 02:19:22PM +0100, Marc Kleine-Budde wrote:
> On 11/05/2014 02:10 PM, Oliver Hartkopp wrote:
> > On 05.11.2014 12:26, Dong Aisheng wrote:
> >> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> >>> On 05.11.2014 08:58, Dong Aisheng wrote:
> > 
> > 
> >>> Unfortunately No. Here it becomes complicated due to the fact that
> >>> the revision 3.0.x does not support per-frame switching for FD/BRS
> >>> ...
> >>
> >> I'm not sure i got your point.
> >>  From m_can spec, it allows switch CAN mode by setting CMR bit.
> >>
> >> Bits 11:10 CMR[1:0]: CAN Mode Request
> >> A change of the CAN operation mode is requested by writing to this bit
> >> field. After change to the
> >> requested operation mode the bit field is reset to ?00? and the status
> >> flags FDBS and FDO are set
> >> accordingly. In case the requested CAN operation mode is not enabled,
> >> the value written to CMR is
> >> retained until it is overwritten by the next mode change request. In
> >> case CME = ?01?/?10?/?11? a
> >> change to CAN operation according to ISO 11898-1 is always possible.
> >> Default is CAN operation
> >> according to ISO11898-1.
> >> 00 unchanged
> >> 01 Request CAN FD operation
> >> 10 Request CAN FD operation with bit rate switching
> >> 11 Request CAN operation according ISO11898-1
> >>
> >> So what's the difference between this function and the per-frame
> >> switching
> >> you mentioned?
> >>
> >>>
> >>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
> >>> tells us, that the controller is _capable_ to send either CAN or CAN
> >>> FD frames.
> >>>
> >>> It does not configure the controller into one of these specific
> >>> settings!
> >>>
> >>> Additionally: AFAIK when writing to the CCCR you have to make sure
> >>> that there's currently no ongoing transfer.
> >>>
> >>
> >> I don't know about it before.
> >> By searching m_can user manual v302 again, i still did not find this
> >> limitation. Can you point me if you know where it is?
> >>
> >> BTW, since we only use one Tx Buffer for transmission currently, so we
> >> should not meet that case that CAN mode is switched during transfer.
> >> So the issue you concern may not happen.
> > 
> > Yes. You are right. Having a FIFO with a size of 1 will help here :-)
> 
> Errrr....sorry...no.
> 
> Taking an easy route here but making it x times harder to extend the
> driver to make use of the FIFO is not an option.
> 

Hmm, this way is just following the original approach the current driver
used. It's initial work and won't make things complicated.

Extend the driver to use FIFO for TX is another story and based on
my understanding it may be a bit complicated to do CAN FD mode switch on this
case due to hw limitation that the revision 3.0.x does not support per-frame
switching for FD/BRS as Oliver pointed out.
(e.g. how to switch FD MODE for each frame on Tx FIFO?)
Probably that's why the 3.1.x version will add the FD/BRS bit controller
in Tx Buffer to fix this issue.

Anyway, that's future work and we can discuss it when adding FIFO support
for Tx function.

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

[PATCH V2 1/4] can: m_can: update to support CAN FD features

[Oliver Hartkopp]

On 05.11.2014 14:46, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 02:19:22PM +0100, Marc Kleine-Budde wrote:
>> On 11/05/2014 02:10 PM, Oliver Hartkopp wrote:
>>> On 05.11.2014 12:26, Dong Aisheng wrote:
>>>> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
>>>>> On 05.11.2014 08:58, Dong Aisheng wrote:
>>>
>>>
>>>>> Unfortunately No. Here it becomes complicated due to the fact that
>>>>> the revision 3.0.x does not support per-frame switching for FD/BRS
>>>>> ...
>>>>
>>>> I'm not sure i got your point.
>>>>   From m_can spec, it allows switch CAN mode by setting CMR bit.
>>>>
>>>> Bits 11:10 CMR[1:0]: CAN Mode Request
>>>> A change of the CAN operation mode is requested by writing to this bit
>>>> field. After change to the
>>>> requested operation mode the bit field is reset to ?00? and the status
>>>> flags FDBS and FDO are set
>>>> accordingly. In case the requested CAN operation mode is not enabled,
>>>> the value written to CMR is
>>>> retained until it is overwritten by the next mode change request. In
>>>> case CME = ?01?/?10?/?11? a
>>>> change to CAN operation according to ISO 11898-1 is always possible.
>>>> Default is CAN operation
>>>> according to ISO11898-1.
>>>> 00 unchanged
>>>> 01 Request CAN FD operation
>>>> 10 Request CAN FD operation with bit rate switching
>>>> 11 Request CAN operation according ISO11898-1
>>>>
>>>> So what's the difference between this function and the per-frame
>>>> switching
>>>> you mentioned?
>>>>
>>>>>
>>>>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
>>>>> tells us, that the controller is _capable_ to send either CAN or CAN
>>>>> FD frames.
>>>>>
>>>>> It does not configure the controller into one of these specific
>>>>> settings!
>>>>>
>>>>> Additionally: AFAIK when writing to the CCCR you have to make sure
>>>>> that there's currently no ongoing transfer.
>>>>>
>>>>
>>>> I don't know about it before.
>>>> By searching m_can user manual v302 again, i still did not find this
>>>> limitation. Can you point me if you know where it is?
>>>>
>>>> BTW, since we only use one Tx Buffer for transmission currently, so we
>>>> should not meet th
> 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   |
>>
>
>
at case that CAN mode is switched during transfer.
>>>> So the issue you concern may not happen.
>>>
>>> Yes. You are right. Having a FIFO with a size of 1 will help here :-)
>>
>> Errrr....sorry...no.
>>
>> Taking an easy route here but making it x times harder to extend the
>> driver to make use of the FIFO is not an option.
>>
>
> Hmm, this way is just following the original approach the current driver
> used. It's initial work and won't make things complicated.
>
> Extend the driver to use FIFO for TX is another story and based on
> my understanding it may be a bit complicated to do CAN FD mode switch on this
> case due to hw limitation that the revision 3.0.x does not support per-frame
> switching for FD/BRS as Oliver pointed out.
> (e.g. how to switch FD MODE for each frame on Tx FIFO?)
> Probably that's why the 3.1.x version will add the FD/BRS bit controller
> in Tx Buffer to fix this issue.
>
> Anyway, that's future work and we can discuss it when adding FIFO support
> for Tx function.
>

Yes. I have to second this opinion.

I also would like to have a TX FIFO. But due to the limitations of the 3.0.x 
M_CAN I would suggest to prefer a 'correct" CAN FD driver implementation in 
favor of having a TX FIFO which is unusable for mixed CAN frame types.

Let's try the FIFO stuff with the next M_CAN revision.

It's a bit of a SJA1000 for CAN FD :-)

Regards,
Oliver