[PATCH 0/2] D_CAN RX buffer size improvements

[PATCH 0/2] D_CAN RX buffer size improvements

[Andrejs Cainikovs]

Re-sending entire patchset due to missed cover letter, sorry.

This patchset introduces support for 64 D_CAN message objects with an option of
unequal split between RX/TX.

The rationale behind this is that there are lots of frame loss on higher bus
speeds. Below are test results from my custom Sitara AM3352 based board:

  Sender: timeout 15m cangen can0 -g 0 -i x
  Target: candump can0,0~0,#FFFFFFFF -td -c -d -e

  * Without patches:
    - 15 minute RX test, 500kbps
    - 16 RX / 16 TX message objects
    - 77 received frames lost out of 4649415

  * With patches applied:
    - 15 hours RX test, 500kbps
    - 56 RX / 8 TX message objects
    - 41 received frames lost out of 279303376

Please note, I do not have ability to test pure C_CAN, so it is left untested.

---

Andrejs Cainikovs (2):
  can: c_can: support 64 message objects for D_CAN
  can: c_can: configurable amount of D_CAN RX objects

 drivers/net/can/c_can/Kconfig | 20 ++++++++++
 drivers/net/can/c_can/c_can.c | 93 +++++++++++++++++++++++++++----------------
 drivers/net/can/c_can/c_can.h | 20 +++++++---
 3 files changed, 94 insertions(+), 39 deletions(-)

---
2.11.0

[PATCH 1/2] can: c_can: support 64 message objects for D_CAN

[Andrejs Cainikovs]

D_CAN supports up to 128 message objects, comparing to 32 on C_CAN.
However, some CPUs with D_CAN controller have their own limits:
TI AM335x Sitara CPU, for example, supports max of 64 message objects.

This patch extends max D_CAN message objects up to 64.

Signed-off-by: Andrejs Cainikovs <andrejs.cainikovs@netmodule.com>
---
 drivers/net/can/c_can/Kconfig | 12 ++++++++++++
 drivers/net/can/c_can/c_can.c | 42 ++++++++++++++++++++++--------------------
 drivers/net/can/c_can/c_can.h | 20 ++++++++++++++++----
 3 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
index 61ffc12d8fd8..6c1ada7291df 100644
--- a/drivers/net/can/c_can/Kconfig
+++ b/drivers/net/can/c_can/Kconfig
@@ -20,4 +20,16 @@ config CAN_C_CAN_PCI
 	---help---
 	  This driver adds support for the C_CAN/D_CAN chips connected
 	  to the PCI bus.
+
+config CAN_C_CAN_DCAN_64_MSG_OBJECTS
+	bool "Use 64 message objects for D_CAN"
+	default n
+	---help---
+	  D_CAN supports up to 128 message objects, comparing to 32 on
+	  C_CAN. However, some CPUs with D_CAN controller have their
+	  own limits: TI AM335x Sitara CPU, for example, supports max
+	  of 64 message objects.
+	  Enabling this option extends max D_CAN message objects up to
+	  64.
+
 endif
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 606b7d8ffe13..5d695b89b459 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -352,15 +352,6 @@ static void c_can_setup_tx_object(struct net_device *dev, int iface,
 	}
 }
 
-static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
-						       int iface)
-{
-	int i;
-
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_RX_LOW_LAST; i++)
-		c_can_object_get(dev, iface, i, IF_COMM_CLR_NEWDAT);
-}
-
 static int c_can_handle_lost_msg_obj(struct net_device *dev,
 				     int iface, int objno, u32 ctrl)
 {
@@ -706,7 +697,16 @@ static void c_can_do_tx(struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 	u32 idx, obj, pkts = 0, bytes = 0, pend, clr;
 
-	clr = pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
+#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+	if (priv->type == BOSCH_D_CAN) {
+		pend = priv->read_reg32(priv, C_CAN_INTPND3_REG);
+	} else {
+#endif
+		pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
+#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+	}
+#endif
+	clr = pend;
 
 	while ((idx = ffs(pend))) {
 		idx--;
@@ -817,7 +817,17 @@ static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv,
 
 static inline u32 c_can_get_pending(struct c_can_priv *priv)
 {
-	u32 pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
+	u32 pend;
+
+#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+	if (priv->type == BOSCH_D_CAN) {
+		pend = priv->read_reg32(priv, C_CAN_NEWDAT1_REG);
+	} else {
+#endif
+		pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
+#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+	}
+#endif
 
 	return pend;
 }
@@ -828,8 +838,7 @@ static inline u32 c_can_get_pending(struct c_can_priv *priv)
  * c_can core saves a received CAN message into the first free message
  * object it finds free (starting with the lowest). Bits NEWDAT and
  * INTPND are set for this message object indicating that a new message
- * has arrived. To work-around this issue, we keep two groups of message
- * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
+ * has arrived.
  *
  * We clear the newdat bit right away.
  *
@@ -840,13 +849,6 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 	struct c_can_priv *priv = netdev_priv(dev);
 	u32 pkts = 0, pend = 0, toread, n;
 
-	/*
-	 * It is faster to read only one 16bit register. This is only possible
-	 * for a maximum number of 16 objects.
-	 */
-	BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16,
-			"Implementation does not support more message objects than 16");
-
 	while (quota > 0) {
 		if (!pend) {
 			pend = c_can_get_pending(priv);
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 8acdc7fa4792..e44b686a70a2 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -23,9 +23,15 @@
 #define C_CAN_H
 
 /* message object split */
+
+#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+#define C_CAN_NO_OF_OBJECTS	64
+#else
 #define C_CAN_NO_OF_OBJECTS	32
-#define C_CAN_MSG_OBJ_RX_NUM	16
-#define C_CAN_MSG_OBJ_TX_NUM	16
+#endif
+
+#define C_CAN_MSG_OBJ_TX_NUM	(C_CAN_NO_OF_OBJECTS >> 1)
+#define C_CAN_MSG_OBJ_RX_NUM	(C_CAN_NO_OF_OBJECTS - C_CAN_MSG_OBJ_TX_NUM)
 
 #define C_CAN_MSG_OBJ_RX_FIRST	1
 #define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
@@ -35,9 +41,11 @@
 #define C_CAN_MSG_OBJ_TX_LAST	(C_CAN_MSG_OBJ_TX_FIRST + \
 				C_CAN_MSG_OBJ_TX_NUM - 1)
 
-#define C_CAN_MSG_OBJ_RX_SPLIT	9
-#define C_CAN_MSG_RX_LOW_LAST	(C_CAN_MSG_OBJ_RX_SPLIT - 1)
+#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+#define RECEIVE_OBJECT_BITS	0xffffffff
+#else
 #define RECEIVE_OBJECT_BITS	0x0000ffff
+#endif
 
 enum reg {
 	C_CAN_CTRL_REG = 0,
@@ -76,6 +84,8 @@ enum reg {
 	C_CAN_NEWDAT2_REG,
 	C_CAN_INTPND1_REG,
 	C_CAN_INTPND2_REG,
+	C_CAN_INTPND3_REG,
+	C_CAN_INTPND4_REG,
 	C_CAN_MSGVAL1_REG,
 	C_CAN_MSGVAL2_REG,
 	C_CAN_FUNCTION_REG,
@@ -137,6 +147,8 @@ static const u16 reg_map_d_can[] = {
 	[C_CAN_NEWDAT2_REG]	= 0x9E,
 	[C_CAN_INTPND1_REG]	= 0xB0,
 	[C_CAN_INTPND2_REG]	= 0xB2,
+	[C_CAN_INTPND3_REG]	= 0xB4,
+	[C_CAN_INTPND4_REG]	= 0xB6,
 	[C_CAN_MSGVAL1_REG]	= 0xC4,
 	[C_CAN_MSGVAL2_REG]	= 0xC6,
 	[C_CAN_IF1_COMREQ_REG]	= 0x100,
-- 
2.11.0

[PATCH 2/2] can: c_can: configurable amount of D_CAN RX objects

[Andrejs Cainikovs]

Make number of D_CAN RX message objects configurable. This will allow
having bigger (or smaller) RX buffer instead of 50/50 split for RX/TX.

Signed-off-by: Andrejs Cainikovs <andrejs.cainikovs@netmodule.com>
---
 drivers/net/can/c_can/Kconfig |  8 ++++++
 drivers/net/can/c_can/c_can.c | 64 +++++++++++++++++++++++++++++--------------
 drivers/net/can/c_can/c_can.h | 16 +++++------
 3 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
index 6c1ada7291df..949d2d12d71e 100644
--- a/drivers/net/can/c_can/Kconfig
+++ b/drivers/net/can/c_can/Kconfig
@@ -32,4 +32,12 @@ config CAN_C_CAN_DCAN_64_MSG_OBJECTS
 	  Enabling this option extends max D_CAN message objects up to
 	  64.
 
+config CAN_C_CAN_DCAN_RX_MSG_OBJECTS
+	int "Specify amount of D_CAN RX message objects"
+	depends on CAN_C_CAN_DCAN_64_MSG_OBJECTS
+	default 32
+	---help---
+	  Use specific number of message objects for RX, instead of
+	  50/50 split between RX/TX.
+
 endif
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 5d695b89b459..675bc223e222 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -208,6 +208,26 @@ static const struct can_bittiming_const c_can_bittiming_const = {
 	.brp_inc = 1,
 };
 
+static inline u64 c_can_get_mask(int bits)
+{
+	return ((u64)1 << bits) - 1;
+}
+
+static inline int c_can_ffs64(u64 x)
+{
+	int b;
+
+	b = ffs(x);
+
+	if (!b) {
+		b = ffs(x >> 32);
+		if (b)
+			b += 32;
+	}
+
+	return b;
+}
+
 static inline void c_can_pm_runtime_enable(const struct c_can_priv *priv)
 {
 	if (priv->device)
@@ -695,24 +715,23 @@ static void c_can_do_tx(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
-	u32 idx, obj, pkts = 0, bytes = 0, pend, clr;
+	u32 idx, obj, pkts = 0, bytes = 0;
+	u64 pend, clr;
 
+	/* Mask interrupt pending bits */
+	pend = priv->read_reg32(priv, C_CAN_INTPND1_REG);
 #ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
 	if (priv->type == BOSCH_D_CAN) {
-		pend = priv->read_reg32(priv, C_CAN_INTPND3_REG);
-	} else {
-#endif
-		pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
-#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
+		pend |= (u64)priv->read_reg32(priv, C_CAN_INTPND3_REG) << 32;
 	}
 #endif
-	clr = pend;
+	pend &= ~c_can_get_mask(C_CAN_MSG_OBJ_RX_NUM);
+	clr = pend >> C_CAN_MSG_OBJ_RX_NUM;
 
-	while ((idx = ffs(pend))) {
-		idx--;
-		pend &= ~(1 << idx);
-		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+	while ((obj = c_can_ffs64(pend))) {
+		pend &= ~((u64)1 << (obj - 1));
 		c_can_inval_tx_object(dev, IF_RX, obj);
+		idx = obj - C_CAN_MSG_OBJ_TX_FIRST;
 		can_get_echo_skb(dev, idx);
 		bytes += priv->dlc[idx];
 		pkts++;
@@ -736,19 +755,19 @@ static void c_can_do_tx(struct net_device *dev)
  * raced with the hardware or failed to readout all upper
  * objects in the last run due to quota limit.
  */
-static u32 c_can_adjust_pending(u32 pend)
+static u64 c_can_adjust_pending(u64 pend)
 {
-	u32 weight, lasts;
+	u64 weight, lasts;
 
-	if (pend == RECEIVE_OBJECT_BITS)
+	if (pend == c_can_get_mask(C_CAN_MSG_OBJ_RX_NUM))
 		return pend;
 
 	/*
 	 * If the last set bit is larger than the number of pending
 	 * bits we have a gap.
 	 */
-	weight = hweight32(pend);
-	lasts = fls(pend);
+	weight = hweight64(pend);
+	lasts = fls64(pend);
 
 	/* If the bits are linear, nothing to do */
 	if (lasts == weight)
@@ -777,11 +796,11 @@ static inline void c_can_rx_finalize(struct net_device *dev,
 }
 
 static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv,
-			      u32 pend, int quota)
+			      u64 pend, int quota)
 {
 	u32 pkts = 0, ctrl, obj;
 
-	while ((obj = ffs(pend)) && quota > 0) {
+	while ((obj = c_can_ffs64(pend)) && quota > 0) {
 		pend &= ~BIT(obj - 1);
 
 		c_can_rx_object_get(dev, priv, obj);
@@ -815,13 +834,15 @@ static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv,
 	return pkts;
 }
 
-static inline u32 c_can_get_pending(struct c_can_priv *priv)
+static inline u64 c_can_get_pending(struct c_can_priv *priv)
 {
-	u32 pend;
+	u64 pend;
 
 #ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
 	if (priv->type == BOSCH_D_CAN) {
 		pend = priv->read_reg32(priv, C_CAN_NEWDAT1_REG);
+		pend |= (u64)priv->read_reg32(priv, C_CAN_NEWDAT3_REG) << 32;
+		pend &= c_can_get_mask(C_CAN_MSG_OBJ_RX_NUM);
 	} else {
 #endif
 		pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
@@ -847,7 +868,8 @@ static inline u32 c_can_get_pending(struct c_can_priv *priv)
 static int c_can_do_rx_poll(struct net_device *dev, int quota)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
-	u32 pkts = 0, pend = 0, toread, n;
+	u32 pkts = 0, n;
+	u64 pend = 0, toread;
 
 	while (quota > 0) {
 		if (!pend) {
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index e44b686a70a2..4a0759ee249d 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -26,12 +26,12 @@
 
 #ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
 #define C_CAN_NO_OF_OBJECTS	64
+#define C_CAN_MSG_OBJ_RX_NUM	CONFIG_CAN_C_CAN_DCAN_RX_MSG_OBJECTS
 #else
 #define C_CAN_NO_OF_OBJECTS	32
+#define C_CAN_MSG_OBJ_RX_NUM	16
 #endif
-
-#define C_CAN_MSG_OBJ_TX_NUM	(C_CAN_NO_OF_OBJECTS >> 1)
-#define C_CAN_MSG_OBJ_RX_NUM	(C_CAN_NO_OF_OBJECTS - C_CAN_MSG_OBJ_TX_NUM)
+#define C_CAN_MSG_OBJ_TX_NUM	(C_CAN_NO_OF_OBJECTS - C_CAN_MSG_OBJ_RX_NUM)
 
 #define C_CAN_MSG_OBJ_RX_FIRST	1
 #define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
@@ -41,12 +41,6 @@
 #define C_CAN_MSG_OBJ_TX_LAST	(C_CAN_MSG_OBJ_TX_FIRST + \
 				C_CAN_MSG_OBJ_TX_NUM - 1)
 
-#ifdef CONFIG_CAN_C_CAN_DCAN_64_MSG_OBJECTS
-#define RECEIVE_OBJECT_BITS	0xffffffff
-#else
-#define RECEIVE_OBJECT_BITS	0x0000ffff
-#endif
-
 enum reg {
 	C_CAN_CTRL_REG = 0,
 	C_CAN_CTRL_EX_REG,
@@ -82,6 +76,8 @@ enum reg {
 	C_CAN_TXRQST2_REG,
 	C_CAN_NEWDAT1_REG,
 	C_CAN_NEWDAT2_REG,
+	C_CAN_NEWDAT3_REG,
+	C_CAN_NEWDAT4_REG,
 	C_CAN_INTPND1_REG,
 	C_CAN_INTPND2_REG,
 	C_CAN_INTPND3_REG,
@@ -145,6 +141,8 @@ static const u16 reg_map_d_can[] = {
 	[C_CAN_TXRQST2_REG]	= 0x8A,
 	[C_CAN_NEWDAT1_REG]	= 0x9C,
 	[C_CAN_NEWDAT2_REG]	= 0x9E,
+	[C_CAN_NEWDAT3_REG]	= 0xA0,
+	[C_CAN_NEWDAT4_REG]	= 0xA2,
 	[C_CAN_INTPND1_REG]	= 0xB0,
 	[C_CAN_INTPND2_REG]	= 0xB2,
 	[C_CAN_INTPND3_REG]	= 0xB4,
-- 
2.11.0

Re: [PATCH 0/2] D_CAN RX buffer size improvements

[Ezequiel Garcia]

Hi everyone,

This series was recently brought to my attention, in connection to a
long-standing
packet drop issue that we had on a Sitara-based product.

I haven't tested this personally, but I've been notified that this was
backported
to the v4.19 kernel, and the packet drop was fixed.

It seems the series never managed to get upstreamed,
but I think this should be resurrected and merged (probably with after
some cleanup/review).

Thanks,
Ezequiel

On Fri, 8 Feb 2019 at 10:31, Andrejs Cainikovs
<Andrejs.Cainikovs@netmodule.com> wrote:
>
> Re-sending entire patchset due to missed cover letter, sorry.
>
> This patchset introduces support for 64 D_CAN message objects with an option of
> unequal split between RX/TX.
>
> The rationale behind this is that there are lots of frame loss on higher bus
> speeds. Below are test results from my custom Sitara AM3352 based board:
>
>   Sender: timeout 15m cangen can0 -g 0 -i x
>   Target: candump can0,0~0,#FFFFFFFF -td -c -d -e
>
>   * Without patches:
>     - 15 minute RX test, 500kbps
>     - 16 RX / 16 TX message objects
>     - 77 received frames lost out of 4649415
>
>   * With patches applied:
>     - 15 hours RX test, 500kbps
>     - 56 RX / 8 TX message objects
>     - 41 received frames lost out of 279303376
>
> Please note, I do not have ability to test pure C_CAN, so it is left untested.
>
> ---
>
> Andrejs Cainikovs (2):
>   can: c_can: support 64 message objects for D_CAN
>   can: c_can: configurable amount of D_CAN RX objects
>
>  drivers/net/can/c_can/Kconfig | 20 ++++++++++
>  drivers/net/can/c_can/c_can.c | 93 +++++++++++++++++++++++++++----------------
>  drivers/net/can/c_can/c_can.h | 20 +++++++---
>  3 files changed, 94 insertions(+), 39 deletions(-)
>
> ---
> 2.11.0
>

Re: [PATCH 0/2] D_CAN RX buffer size improvements

[Andrejs Cainikovs]

Hi Ezequiel,

Sorry for a late reply. I'm the author of this patch set, and I will 
have a look at this after I obtain the hardware. I hope this is still 
relevant.

Best regards,
Andrejs.

  > Hi everyone,
  >
  > This series was recently brought to my attention, in connection to a
long-standing packet drop issue that we had on a Sitara-based product.
  >
  > I haven't tested this personally, but I've been notified that this
was backported to the v4.19 kernel, and the packet drop was fixed.
  >
  > It seems the series never managed to get upstreamed, but I think this
should be resurrected and merged (probably with after some cleanup/review).
  >
  > Thanks,
  > Ezequiel

Re: [PATCH 0/2] D_CAN RX buffer size improvements

[Marc Kleine-Budde]

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

On 06.08.2021 12:16:26, Andrejs Cainikovs wrote:
> Sorry for a late reply. I'm the author of this patch set, and I will
> have a look at this after I obtain the hardware. I hope this is still
> relevant.

Dario (Cc'ed) created a proper patch series to support 64 message
objects. The series has been mainlined in:

https://git.kernel.org/linus/132f2d45fb2302a582aef617ea766f3fa52a084c

Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] D_CAN RX buffer size improvements

[Ezequiel Garcia]

(Adding Federico and Max)

On Fri, 2021-08-06 at 12:36 +0200, Marc Kleine-Budde wrote:
> On 06.08.2021 12:16:26, Andrejs Cainikovs wrote:
> > Sorry for a late reply. I'm the author of this patch set, and I will
> > have a look at this after I obtain the hardware. I hope this is still
> > relevant.
> 
> Dario (Cc'ed) created a proper patch series to support 64 message
> objects. The series has been mainlined in:
> 
> https://git.kernel.org/linus/132f2d45fb2302a582aef617ea766f3fa52a084c
> 

Ah, that's really great news.

Thanks a lot Marc and Dario.
-- 
Kindly,
Ezequiel