[PATCH] can: ti_hecc: use timestamp based rx-offloading

[PATCH] can: ti_hecc: use timestamp based rx-offloading

[Jeroen Hofstee]

As already mentioned in [1] and included in [2], there is an off by one
issue since the high bank is already enabled when the _next_ mailbox to
be read has index 12, so the mailbox being read was 13. The message can
therefore go into mailbox 31 and the driver will be repolled until the
mailbox 12 eventually receives a msg. Or the message might end up in the
12th mailbox, but then it would become disabled after reading it and only
be enabled again in the next "round" after mailbox 13 was read, which can
cause out of order messages, since the lower priority mailboxes can
accept messages in the meantime.

As mentioned in [3] there is a hardware race condition when changing the
CANME register while messages are being received. Even when including a
busy poll on reception, like in [2] there are still overflows and out of
order messages at times, but less then without the busy loop polling.
Unlike what the patch suggests, the polling time is not in the microsecond
range, but takes as long as a current CAN bus reception needs to finish,
so typically more in the fraction of millisecond range. Since the timeout
is in jiffies it won't timeout.

Even with these additional fixes the driver is still not able to provide a
proper FIFO which doesn't drop packages. So change the driver to use
rx-offload and base order on timestamp instead of message box numbers. As
a side affect, this also fixes [4] and [5].

Before this change messages with a single byte counter were dropped /
received out of order at a bitrate of 250kbit/s on an am3517. With this
patch that no longer occurs up to and including 1Mbit/s.

[1] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6
[2] http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2
[3] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5
[4] https://patchwork.ozlabs.org/patch/895956/
[5] https://www.spinics.net/lists/netdev/msg494971.html

Cc: Anant Gole <anantgole@ti.com>
Cc: AnilKumar Ch <anilkumar@ti.com>
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
 drivers/net/can/ti_hecc.c | 189 +++++++++++++---------------------------------
 1 file changed, 53 insertions(+), 136 deletions(-)

diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index db6ea93..fe7ffff 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -5,6 +5,7 @@
  * specs for the same is available at <http://www.ti.com>
  *
  * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
+ * Copyright (C) 2019 Jeroen Hofstee <jhofstee@victronenergy.com>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -34,6 +35,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/can/rx-offload.h>
 
 #define DRV_NAME "ti_hecc"
 #define HECC_MODULE_VERSION     "0.7"
@@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION);
 #define HECC_TX_PRIO_MASK	(MAX_TX_PRIO << HECC_MB_TX_SHIFT)
 #define HECC_TX_MB_MASK		(HECC_MAX_TX_MBOX - 1)
 #define HECC_TX_MASK		((HECC_MAX_TX_MBOX - 1) | HECC_TX_PRIO_MASK)
-#define HECC_TX_MBOX_MASK	(~(BIT(HECC_MAX_TX_MBOX) - 1))
-#define HECC_DEF_NAPI_WEIGHT	HECC_MAX_RX_MBOX
 
 /*
- * Important Note: RX mailbox configuration
- * RX mailboxes are further logically split into two - main and buffer
- * mailboxes. The goal is to get all packets into main mailboxes as
- * driven by mailbox number and receive priority (higher to lower) and
- * buffer mailboxes are used to receive pkts while main mailboxes are being
- * processed. This ensures in-order packet reception.
- *
- * Here are the recommended values for buffer mailbox. Note that RX mailboxes
- * start after TX mailboxes:
- *
- * HECC_MAX_RX_MBOX		HECC_RX_BUFFER_MBOX	No of buffer mailboxes
- * 28				12			8
- * 16				20			4
+ * RX mailbox configuration
+ * The remaining mailboxes are used for reception and are delivered based on
+ * their timestamp, to avoid a hardware race when CANME is changed while
+ * CAN-bus traffix is being received.
  */
 
 #define HECC_MAX_RX_MBOX	(HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX)
-#define HECC_RX_BUFFER_MBOX	12 /* as per table above */
 #define HECC_RX_FIRST_MBOX	(HECC_MAX_MAILBOXES - 1)
-#define HECC_RX_HIGH_MBOX_MASK	(~(BIT(HECC_RX_BUFFER_MBOX) - 1))
 
 /* TI HECC module registers */
 #define HECC_CANME		0x0	/* Mailbox enable */
@@ -123,6 +112,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
 #define HECC_CANMDL		0x8
 #define HECC_CANMDH		0xC
 
+#define HECC_CANMOTS		0x100
+
 #define HECC_SET_REG		0xFFFFFFFF
 #define HECC_CANID_MASK		0x3FF	/* 18 bits mask for extended id's */
 #define HECC_CCE_WAIT_COUNT     100	/* Wait for ~1 sec for CCE bit */
@@ -193,7 +184,7 @@ static const struct can_bittiming_const ti_hecc_bittiming_const = {
 
 struct ti_hecc_priv {
 	struct can_priv can;	/* MUST be first member/field */
-	struct napi_struct napi;
+	struct can_rx_offload offload;
 	struct net_device *ndev;
 	struct clk *clk;
 	void __iomem *base;
@@ -203,7 +194,6 @@ struct ti_hecc_priv {
 	spinlock_t mbx_lock; /* CANME register needs protection */
 	u32 tx_head;
 	u32 tx_tail;
-	u32 rx_next;
 	struct regulator *reg_xceiver;
 };
 
@@ -265,6 +255,11 @@ static inline u32 hecc_get_bit(struct ti_hecc_priv *priv, int reg, u32 bit_mask)
 	return (hecc_read(priv, reg) & bit_mask) ? 1 : 0;
 }
 
+static inline u32 hecc_read_stamp(struct ti_hecc_priv *priv, u32 mbxno)
+{
+	return __raw_readl(priv->hecc_ram + 0x80 + 4 * mbxno);
+}
+
 static int ti_hecc_set_btc(struct ti_hecc_priv *priv)
 {
 	struct can_bittiming *bit_timing = &priv->can.bittiming;
@@ -375,7 +370,6 @@ static void ti_hecc_start(struct net_device *ndev)
 	ti_hecc_reset(ndev);
 
 	priv->tx_head = priv->tx_tail = HECC_TX_MASK;
-	priv->rx_next = HECC_RX_FIRST_MBOX;
 
 	/* Enable local and global acceptance mask registers */
 	hecc_write(priv, HECC_CANGAM, HECC_SET_REG);
@@ -526,21 +520,17 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	return NETDEV_TX_OK;
 }
 
-static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno)
+static inline struct ti_hecc_priv *rx_offload_to_priv(struct can_rx_offload *offload)
 {
-	struct net_device_stats *stats = &priv->ndev->stats;
-	struct can_frame *cf;
-	struct sk_buff *skb;
-	u32 data, mbx_mask;
-	unsigned long flags;
+	return container_of(offload, struct ti_hecc_priv, offload);
+}
 
-	skb = alloc_can_skb(priv->ndev, &cf);
-	if (!skb) {
-		if (printk_ratelimit())
-			netdev_err(priv->ndev,
-				"ti_hecc_rx_pkt: alloc_can_skb() failed\n");
-		return -ENOMEM;
-	}
+static unsigned int ti_hecc_mailbox_read(struct can_rx_offload *offload,
+					 struct can_frame *cf,
+					 u32 *timestamp, unsigned int mbxno)
+{
+	struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
+	u32 data, mbx_mask;
 
 	mbx_mask = BIT(mbxno);
 	data = hecc_read_mbx(priv, mbxno, HECC_CANMID);
@@ -558,100 +548,19 @@ static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno)
 		data = hecc_read_mbx(priv, mbxno, HECC_CANMDH);
 		*(__be32 *)(cf->data + 4) = cpu_to_be32(data);
 	}
-	spin_lock_irqsave(&priv->mbx_lock, flags);
-	hecc_clear_bit(priv, HECC_CANME, mbx_mask);
-	hecc_write(priv, HECC_CANRMP, mbx_mask);
-	/* enable mailbox only if it is part of rx buffer mailboxes */
-	if (priv->rx_next < HECC_RX_BUFFER_MBOX)
-		hecc_set_bit(priv, HECC_CANME, mbx_mask);
-	spin_unlock_irqrestore(&priv->mbx_lock, flags);
 
-	stats->rx_bytes += cf->can_dlc;
-	can_led_event(priv->ndev, CAN_LED_EVENT_RX);
-	netif_receive_skb(skb);
-	stats->rx_packets++;
+	*timestamp = hecc_read_stamp(priv, mbxno);
 
-	return 0;
-}
-
-/*
- * ti_hecc_rx_poll - HECC receive pkts
- *
- * The receive mailboxes start from highest numbered mailbox till last xmit
- * mailbox. On CAN frame reception the hardware places the data into highest
- * numbered mailbox that matches the CAN ID filter. Since all receive mailboxes
- * have same filtering (ALL CAN frames) packets will arrive in the highest
- * available RX mailbox and we need to ensure in-order packet reception.
- *
- * To ensure the packets are received in the right order we logically divide
- * the RX mailboxes into main and buffer mailboxes. Packets are received as per
- * mailbox priotity (higher to lower) in the main bank and once it is full we
- * disable further reception into main mailboxes. While the main mailboxes are
- * processed in NAPI, further packets are received in buffer mailboxes.
- *
- * We maintain a RX next mailbox counter to process packets and once all main
- * mailboxe packets are passed to the upper stack we enable all of them but
- * continue to process packets received in buffer mailboxes. With each packet
- * received from buffer mailbox we enable it immediately so as to handle the
- * overflow from higher mailboxes.
- */
-static int ti_hecc_rx_poll(struct napi_struct *napi, int quota)
-{
-	struct net_device *ndev = napi->dev;
-	struct ti_hecc_priv *priv = netdev_priv(ndev);
-	u32 num_pkts = 0;
-	u32 mbx_mask;
-	unsigned long pending_pkts, flags;
-
-	if (!netif_running(ndev))
-		return 0;
-
-	while ((pending_pkts = hecc_read(priv, HECC_CANRMP)) &&
-		num_pkts < quota) {
-		mbx_mask = BIT(priv->rx_next); /* next rx mailbox to process */
-		if (mbx_mask & pending_pkts) {
-			if (ti_hecc_rx_pkt(priv, priv->rx_next) < 0)
-				return num_pkts;
-			++num_pkts;
-		} else if (priv->rx_next > HECC_RX_BUFFER_MBOX) {
-			break; /* pkt not received yet */
-		}
-		--priv->rx_next;
-		if (priv->rx_next == HECC_RX_BUFFER_MBOX) {
-			/* enable high bank mailboxes */
-			spin_lock_irqsave(&priv->mbx_lock, flags);
-			mbx_mask = hecc_read(priv, HECC_CANME);
-			mbx_mask |= HECC_RX_HIGH_MBOX_MASK;
-			hecc_write(priv, HECC_CANME, mbx_mask);
-			spin_unlock_irqrestore(&priv->mbx_lock, flags);
-		} else if (priv->rx_next == HECC_MAX_TX_MBOX - 1) {
-			priv->rx_next = HECC_RX_FIRST_MBOX;
-			break;
-		}
-	}
-
-	/* Enable packet interrupt if all pkts are handled */
-	if (hecc_read(priv, HECC_CANRMP) == 0) {
-		napi_complete(napi);
-		/* Re-enable RX mailbox interrupts */
-		mbx_mask = hecc_read(priv, HECC_CANMIM);
-		mbx_mask |= HECC_TX_MBOX_MASK;
-		hecc_write(priv, HECC_CANMIM, mbx_mask);
-	} else {
-		/* repoll is done only if whole budget is used */
-		num_pkts = quota;
-	}
-
-	return num_pkts;
+	return 1;
 }
 
 static int ti_hecc_error(struct net_device *ndev, int int_status,
 	int err_status)
 {
 	struct ti_hecc_priv *priv = netdev_priv(ndev);
-	struct net_device_stats *stats = &ndev->stats;
 	struct can_frame *cf;
 	struct sk_buff *skb;
+	u32 timestamp;
 
 	/* propagate the error condition to the can stack */
 	skb = alloc_can_err_skb(ndev, &cf);
@@ -732,9 +641,8 @@ static int ti_hecc_error(struct net_device *ndev, int int_status,
 		}
 	}
 
-	stats->rx_packets++;
-	stats->rx_bytes += cf->can_dlc;
-	netif_rx(skb);
+	timestamp = hecc_read(priv, HECC_CANLNT);
+	can_rx_offload_queue_sorted(&priv->offload, skb, timestamp);
 
 	return 0;
 }
@@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
 	struct net_device *ndev = (struct net_device *)dev_id;
 	struct ti_hecc_priv *priv = netdev_priv(ndev);
 	struct net_device_stats *stats = &ndev->stats;
-	u32 mbxno, mbx_mask, int_status, err_status;
-	unsigned long ack, flags;
+	u32 mbxno, mbx_mask, int_status, err_status, stamp;
+	unsigned long flags, rx_pending;
 
 	int_status = hecc_read(priv,
 		(priv->use_hecc1int) ? HECC_CANGIF1 : HECC_CANGIF0);
@@ -769,11 +677,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
 			spin_lock_irqsave(&priv->mbx_lock, flags);
 			hecc_clear_bit(priv, HECC_CANME, mbx_mask);
 			spin_unlock_irqrestore(&priv->mbx_lock, flags);
-			stats->tx_bytes += hecc_read_mbx(priv, mbxno,
-						HECC_CANMCF) & 0xF;
+			stamp = hecc_read_stamp(priv, mbxno);
+			stats->tx_bytes += can_rx_offload_get_echo_skb(&priv->offload,
+									mbxno, stamp);
 			stats->tx_packets++;
 			can_led_event(ndev, CAN_LED_EVENT_TX);
-			can_get_echo_skb(ndev, mbxno);
 			--priv->tx_tail;
 		}
 
@@ -784,12 +692,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
 		((priv->tx_head & HECC_TX_MASK) == HECC_TX_MASK)))
 			netif_wake_queue(ndev);
 
-		/* Disable RX mailbox interrupts and let NAPI reenable them */
-		if (hecc_read(priv, HECC_CANRMP)) {
-			ack = hecc_read(priv, HECC_CANMIM);
-			ack &= BIT(HECC_MAX_TX_MBOX) - 1;
-			hecc_write(priv, HECC_CANMIM, ack);
-			napi_schedule(&priv->napi);
+		/* offload RX mailboxes and let NAPI deliver them */
+		while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
+			can_rx_offload_irq_offload_timestamp(&priv->offload,
+							     rx_pending);
+			hecc_write(priv, HECC_CANRMP, rx_pending);
 		}
 	}
 
@@ -831,7 +738,7 @@ static int ti_hecc_open(struct net_device *ndev)
 	can_led_event(ndev, CAN_LED_EVENT_OPEN);
 
 	ti_hecc_start(ndev);
-	napi_enable(&priv->napi);
+	can_rx_offload_enable(&priv->offload);
 	netif_start_queue(ndev);
 
 	return 0;
@@ -842,7 +749,7 @@ static int ti_hecc_close(struct net_device *ndev)
 	struct ti_hecc_priv *priv = netdev_priv(ndev);
 
 	netif_stop_queue(ndev);
-	napi_disable(&priv->napi);
+	can_rx_offload_disable(&priv->offload);
 	ti_hecc_stop(ndev);
 	free_irq(ndev->irq, ndev);
 	close_candev(ndev);
@@ -962,8 +869,6 @@ static int ti_hecc_probe(struct platform_device *pdev)
 		goto probe_exit_candev;
 	}
 	priv->can.clock.freq = clk_get_rate(priv->clk);
-	netif_napi_add(ndev, &priv->napi, ti_hecc_rx_poll,
-		HECC_DEF_NAPI_WEIGHT);
 
 	err = clk_prepare_enable(priv->clk);
 	if (err) {
@@ -971,10 +876,19 @@ static int ti_hecc_probe(struct platform_device *pdev)
 		goto probe_exit_clk;
 	}
 
+	priv->offload.mailbox_read = ti_hecc_mailbox_read;
+	priv->offload.mb_first = HECC_RX_FIRST_MBOX;
+	priv->offload.mb_last = HECC_MAX_TX_MBOX;
+	err = can_rx_offload_add_timestamp(ndev, &priv->offload);
+	if (err) {
+		dev_err(&pdev->dev, "can_rx_offload_add_timestamp() failed\n");
+		goto probe_exit_clk;
+	}
+
 	err = register_candev(ndev);
 	if (err) {
 		dev_err(&pdev->dev, "register_candev() failed\n");
-		goto probe_exit_clk;
+		goto probe_exit_offload;
 	}
 
 	devm_can_led_init(ndev);
@@ -984,6 +898,8 @@ static int ti_hecc_probe(struct platform_device *pdev)
 
 	return 0;
 
+probe_exit_offload:
+	can_rx_offload_del(&priv->offload);
 probe_exit_clk:
 	clk_put(priv->clk);
 probe_exit_candev:
@@ -1000,6 +916,7 @@ static int ti_hecc_remove(struct platform_device *pdev)
 	unregister_candev(ndev);
 	clk_disable_unprepare(priv->clk);
 	clk_put(priv->clk);
+	can_rx_offload_del(&priv->offload);
 	free_candev(ndev);
 
 	return 0;
-- 
2.7.4

Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 6420 bytes --]

On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
> As already mentioned in [1] and included in [2], there is an off by one
> issue since the high bank is already enabled when the _next_ mailbox to
> be read has index 12, so the mailbox being read was 13. The message can
> therefore go into mailbox 31 and the driver will be repolled until the
> mailbox 12 eventually receives a msg. Or the message might end up in the
> 12th mailbox, but then it would become disabled after reading it and only
> be enabled again in the next "round" after mailbox 13 was read, which can
> cause out of order messages, since the lower priority mailboxes can
> accept messages in the meantime.
> 
> As mentioned in [3] there is a hardware race condition when changing the
> CANME register while messages are being received. Even when including a
> busy poll on reception, like in [2] there are still overflows and out of
> order messages at times, but less then without the busy loop polling.
> Unlike what the patch suggests, the polling time is not in the microsecond
> range, but takes as long as a current CAN bus reception needs to finish,
> so typically more in the fraction of millisecond range. Since the timeout
> is in jiffies it won't timeout.
> 
> Even with these additional fixes the driver is still not able to provide a
> proper FIFO which doesn't drop packages. So change the driver to use
> rx-offload and base order on timestamp instead of message box numbers. As
> a side affect, this also fixes [4] and [5].
> 
> Before this change messages with a single byte counter were dropped /
> received out of order at a bitrate of 250kbit/s on an am3517. With this
> patch that no longer occurs up to and including 1Mbit/s.
> 
> [1] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6
> [2] http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2
> [3] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5
> [4] https://patchwork.ozlabs.org/patch/895956/
> [5] https://www.spinics.net/lists/netdev/msg494971.html
> 
> Cc: Anant Gole <anantgole@ti.com>
> Cc: AnilKumar Ch <anilkumar@ti.com>
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
> ---
>  drivers/net/can/ti_hecc.c | 189 +++++++++++++---------------------------------
>  1 file changed, 53 insertions(+), 136 deletions(-)
> 
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> index db6ea93..fe7ffff 100644
> --- a/drivers/net/can/ti_hecc.c
> +++ b/drivers/net/can/ti_hecc.c
> @@ -5,6 +5,7 @@
>   * specs for the same is available at <http://www.ti.com>
>   *
>   * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
> + * Copyright (C) 2019 Jeroen Hofstee <jhofstee@victronenergy.com>
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License as
> @@ -34,6 +35,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/can/rx-offload.h>
>  
>  #define DRV_NAME "ti_hecc"
>  #define HECC_MODULE_VERSION     "0.7"
> @@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION);
>  #define HECC_TX_PRIO_MASK	(MAX_TX_PRIO << HECC_MB_TX_SHIFT)
>  #define HECC_TX_MB_MASK		(HECC_MAX_TX_MBOX - 1)
>  #define HECC_TX_MASK		((HECC_MAX_TX_MBOX - 1) | HECC_TX_PRIO_MASK)
> -#define HECC_TX_MBOX_MASK	(~(BIT(HECC_MAX_TX_MBOX) - 1))
> -#define HECC_DEF_NAPI_WEIGHT	HECC_MAX_RX_MBOX
>  
>  /*
> - * Important Note: RX mailbox configuration
> - * RX mailboxes are further logically split into two - main and buffer
> - * mailboxes. The goal is to get all packets into main mailboxes as
> - * driven by mailbox number and receive priority (higher to lower) and
> - * buffer mailboxes are used to receive pkts while main mailboxes are being
> - * processed. This ensures in-order packet reception.
> - *
> - * Here are the recommended values for buffer mailbox. Note that RX mailboxes
> - * start after TX mailboxes:
> - *
> - * HECC_MAX_RX_MBOX		HECC_RX_BUFFER_MBOX	No of buffer mailboxes
> - * 28				12			8
> - * 16				20			4
> + * RX mailbox configuration
> + * The remaining mailboxes are used for reception and are delivered based on
> + * their timestamp, to avoid a hardware race when CANME is changed while
> + * CAN-bus traffix is being received.
>   */
>  
>  #define HECC_MAX_RX_MBOX	(HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX)
> -#define HECC_RX_BUFFER_MBOX	12 /* as per table above */
>  #define HECC_RX_FIRST_MBOX	(HECC_MAX_MAILBOXES - 1)
> -#define HECC_RX_HIGH_MBOX_MASK	(~(BIT(HECC_RX_BUFFER_MBOX) - 1))
>  
>  /* TI HECC module registers */
>  #define HECC_CANME		0x0	/* Mailbox enable */
> @@ -123,6 +112,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
>  #define HECC_CANMDL		0x8
>  #define HECC_CANMDH		0xC
>  
> +#define HECC_CANMOTS		0x100

It's actually 0x80

> +
>  #define HECC_SET_REG		0xFFFFFFFF
>  #define HECC_CANID_MASK		0x3FF	/* 18 bits mask for extended id's */
>  #define HECC_CCE_WAIT_COUNT     100	/* Wait for ~1 sec for CCE bit */
> @@ -193,7 +184,7 @@ static const struct can_bittiming_const ti_hecc_bittiming_const = {
>  
>  struct ti_hecc_priv {
>  	struct can_priv can;	/* MUST be first member/field */
> -	struct napi_struct napi;
> +	struct can_rx_offload offload;
>  	struct net_device *ndev;
>  	struct clk *clk;
>  	void __iomem *base;
> @@ -203,7 +194,6 @@ struct ti_hecc_priv {
>  	spinlock_t mbx_lock; /* CANME register needs protection */
>  	u32 tx_head;
>  	u32 tx_tail;
> -	u32 rx_next;
>  	struct regulator *reg_xceiver;
>  };
>  
> @@ -265,6 +255,11 @@ static inline u32 hecc_get_bit(struct ti_hecc_priv *priv, int reg, u32 bit_mask)
>  	return (hecc_read(priv, reg) & bit_mask) ? 1 : 0;
>  }
>  
> +static inline u32 hecc_read_stamp(struct ti_hecc_priv *priv, u32 mbxno)
> +{
> +	return __raw_readl(priv->hecc_ram + 0x80 + 4 * mbxno);

I've changed this function to use HECC_CANMOTS.

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

Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

[Jeroen Hofstee]

Hello Marc,

On 7/24/19 10:26 AM, Marc Kleine-Budde wrote:
> On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
>> As already mentioned in [1] and included in [2], there is an off by one
>> issue since the high bank is already enabled when the _next_ mailbox to
>> be read has index 12, so the mailbox being read was 13. The message can
>> therefore go into mailbox 31 and the driver will be repolled until the
>> mailbox 12 eventually receives a msg. Or the message might end up in the
>> 12th mailbox, but then it would become disabled after reading it and only
>> be enabled again in the next "round" after mailbox 13 was read, which can
>> cause out of order messages, since the lower priority mailboxes can
>> accept messages in the meantime.
>>
>> As mentioned in [3] there is a hardware race condition when changing the
>> CANME register while messages are being received. Even when including a
>> busy poll on reception, like in [2] there are still overflows and out of
>> order messages at times, but less then without the busy loop polling.
>> Unlike what the patch suggests, the polling time is not in the microsecond
>> range, but takes as long as a current CAN bus reception needs to finish,
>> so typically more in the fraction of millisecond range. Since the timeout
>> is in jiffies it won't timeout.
>>
>> Even with these additional fixes the driver is still not able to provide a
>> proper FIFO which doesn't drop packages. So change the driver to use
>> rx-offload and base order on timestamp instead of message box numbers. As
>> a side affect, this also fixes [4] and [5].
>>
>> Before this change messages with a single byte counter were dropped /
>> received out of order at a bitrate of 250kbit/s on an am3517. With this
>> patch that no longer occurs up to and including 1Mbit/s.
>>
>> [1] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6
>> [2] http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2
>> [3] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5
>> [4] https://patchwork.ozlabs.org/patch/895956/
>> [5] https://www.spinics.net/lists/netdev/msg494971.html
>>
>> Cc: Anant Gole <anantgole@ti.com>
>> Cc: AnilKumar Ch <anilkumar@ti.com>
>> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
>> ---
>>   drivers/net/can/ti_hecc.c | 189 +++++++++++++---------------------------------
>>   1 file changed, 53 insertions(+), 136 deletions(-)
>>
>> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
>> index db6ea93..fe7ffff 100644
>> --- a/drivers/net/can/ti_hecc.c
>> +++ b/drivers/net/can/ti_hecc.c
>> @@ -5,6 +5,7 @@
>>    * specs for the same is available at <http://www.ti.com>
>>    *
>>    * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
>> + * Copyright (C) 2019 Jeroen Hofstee <jhofstee@victronenergy.com>
>>    *
>>    * This program is free software; you can redistribute it and/or
>>    * modify it under the terms of the GNU General Public License as
>> @@ -34,6 +35,7 @@
>>   #include <linux/can/dev.h>
>>   #include <linux/can/error.h>
>>   #include <linux/can/led.h>
>> +#include <linux/can/rx-offload.h>
>>   
>>   #define DRV_NAME "ti_hecc"
>>   #define HECC_MODULE_VERSION     "0.7"
>> @@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION);
>>   #define HECC_TX_PRIO_MASK	(MAX_TX_PRIO << HECC_MB_TX_SHIFT)
>>   #define HECC_TX_MB_MASK		(HECC_MAX_TX_MBOX - 1)
>>   #define HECC_TX_MASK		((HECC_MAX_TX_MBOX - 1) | HECC_TX_PRIO_MASK)
>> -#define HECC_TX_MBOX_MASK	(~(BIT(HECC_MAX_TX_MBOX) - 1))
>> -#define HECC_DEF_NAPI_WEIGHT	HECC_MAX_RX_MBOX
>>   
>>   /*
>> - * Important Note: RX mailbox configuration
>> - * RX mailboxes are further logically split into two - main and buffer
>> - * mailboxes. The goal is to get all packets into main mailboxes as
>> - * driven by mailbox number and receive priority (higher to lower) and
>> - * buffer mailboxes are used to receive pkts while main mailboxes are being
>> - * processed. This ensures in-order packet reception.
>> - *
>> - * Here are the recommended values for buffer mailbox. Note that RX mailboxes
>> - * start after TX mailboxes:
>> - *
>> - * HECC_MAX_RX_MBOX		HECC_RX_BUFFER_MBOX	No of buffer mailboxes
>> - * 28				12			8
>> - * 16				20			4
>> + * RX mailbox configuration
>> + * The remaining mailboxes are used for reception and are delivered based on
>> + * their timestamp, to avoid a hardware race when CANME is changed while
>> + * CAN-bus traffix is being received.
>>    */
>>   
>>   #define HECC_MAX_RX_MBOX	(HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX)
>> -#define HECC_RX_BUFFER_MBOX	12 /* as per table above */
>>   #define HECC_RX_FIRST_MBOX	(HECC_MAX_MAILBOXES - 1)
>> -#define HECC_RX_HIGH_MBOX_MASK	(~(BIT(HECC_RX_BUFFER_MBOX) - 1))
>>   
>>   /* TI HECC module registers */
>>   #define HECC_CANME		0x0	/* Mailbox enable */
>> @@ -123,6 +112,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
>>   #define HECC_CANMDL		0x8
>>   #define HECC_CANMDH		0xC
>>   
>> +#define HECC_CANMOTS		0x100
> It's actually 0x80
>
>> +
>>   #define HECC_SET_REG		0xFFFFFFFF
>>   #define HECC_CANID_MASK		0x3FF	/* 18 bits mask for extended id's */
>>   #define HECC_CCE_WAIT_COUNT     100	/* Wait for ~1 sec for CCE bit */
>> @@ -193,7 +184,7 @@ static const struct can_bittiming_const ti_hecc_bittiming_const = {
>>   
>>   struct ti_hecc_priv {
>>   	struct can_priv can;	/* MUST be first member/field */
>> -	struct napi_struct napi;
>> +	struct can_rx_offload offload;
>>   	struct net_device *ndev;
>>   	struct clk *clk;
>>   	void __iomem *base;
>> @@ -203,7 +194,6 @@ struct ti_hecc_priv {
>>   	spinlock_t mbx_lock; /* CANME register needs protection */
>>   	u32 tx_head;
>>   	u32 tx_tail;
>> -	u32 rx_next;
>>   	struct regulator *reg_xceiver;
>>   };
>>   
>> @@ -265,6 +255,11 @@ static inline u32 hecc_get_bit(struct ti_hecc_priv *priv, int reg, u32 bit_mask)
>>   	return (hecc_read(priv, reg) & bit_mask) ? 1 : 0;
>>   }
>>   
>> +static inline u32 hecc_read_stamp(struct ti_hecc_priv *priv, u32 mbxno)
>> +{
>> +	return __raw_readl(priv->hecc_ram + 0x80 + 4 * mbxno);
> I've changed this function to use HECC_CANMOTS.
>

That is correct. For completeness the HECC_CANMOTS wasn't
even used in the original patch, so there is no functional difference.

Thanks,

Jeroen

Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

[Saeed Mahameed]

On Mon, 2019-04-29 at 12:03 +0000, Jeroen Hofstee wrote:
> As already mentioned in [1] and included in [2], there is an off by
> one
> issue since the high bank is already enabled when the _next_ mailbox
> to
> be read has index 12, so the mailbox being read was 13. The message
> can
> therefore go into mailbox 31 and the driver will be repolled until
> the
> mailbox 12 eventually receives a msg. Or the message might end up in
> the
> 12th mailbox, but then it would become disabled after reading it and
> only
> be enabled again in the next "round" after mailbox 13 was read, which
> can
> cause out of order messages, since the lower priority mailboxes can
> accept messages in the meantime.
> 
> As mentioned in [3] there is a hardware race condition when changing
> the
> CANME register while messages are being received. Even when including
> a
> busy poll on reception, like in [2] there are still overflows and out
> of
> order messages at times, but less then without the busy loop polling.
> Unlike what the patch suggests, the polling time is not in the
> microsecond
> range, but takes as long as a current CAN bus reception needs to
> finish,
> so typically more in the fraction of millisecond range. Since the
> timeout
> is in jiffies it won't timeout.
> 
> Even with these additional fixes the driver is still not able to
> provide a
> proper FIFO which doesn't drop packages. So change the driver to use
> rx-offload and base order on timestamp instead of message box
> numbers. As
> a side affect, this also fixes [4] and [5].
> 
> Before this change messages with a single byte counter were dropped /
> received out of order at a bitrate of 250kbit/s on an am3517. With
> this
> patch that no longer occurs up to and including 1Mbit/s.
> 
> [1] 
> https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6
> [2] 
> http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2
> [3] 
> https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5
> [4] https://patchwork.ozlabs.org/patch/895956/
> [5] https://www.spinics.net/lists/netdev/msg494971.html
> 
> Cc: Anant Gole <anantgole@ti.com>
> Cc: AnilKumar Ch <anilkumar@ti.com>
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
> ---
>  drivers/net/can/ti_hecc.c | 189 +++++++++++++-----------------------
> ----------
>  1 file changed, 53 insertions(+), 136 deletions(-)
> 
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> index db6ea93..fe7ffff 100644
> --- a/drivers/net/can/ti_hecc.c
> +++ b/drivers/net/can/ti_hecc.c
> @@ -5,6 +5,7 @@
>   * specs for the same is available at <http://www.ti.com>
>   *
>   * Copyright (C) 2009 Texas Instruments Incorporated - 
> http://www.ti.com/
> + * Copyright (C) 2019 Jeroen Hofstee <jhofstee@victronenergy.com>
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License as
> @@ -34,6 +35,7 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
> +#include <linux/can/rx-offload.h>
>  
>  #define DRV_NAME "ti_hecc"
>  #define HECC_MODULE_VERSION     "0.7"
> @@ -63,29 +65,16 @@ MODULE_VERSION(HECC_MODULE_VERSION);
>  #define HECC_TX_PRIO_MASK	(MAX_TX_PRIO << HECC_MB_TX_SHIFT)
>  #define HECC_TX_MB_MASK		(HECC_MAX_TX_MBOX - 1)
>  #define HECC_TX_MASK		((HECC_MAX_TX_MBOX - 1) |
> HECC_TX_PRIO_MASK)
> -#define HECC_TX_MBOX_MASK	(~(BIT(HECC_MAX_TX_MBOX) - 1))
> -#define HECC_DEF_NAPI_WEIGHT	HECC_MAX_RX_MBOX
>  
>  /*
> - * Important Note: RX mailbox configuration
> - * RX mailboxes are further logically split into two - main and
> buffer
> - * mailboxes. The goal is to get all packets into main mailboxes as
> - * driven by mailbox number and receive priority (higher to lower)
> and
> - * buffer mailboxes are used to receive pkts while main mailboxes
> are being
> - * processed. This ensures in-order packet reception.
> - *
> - * Here are the recommended values for buffer mailbox. Note that RX
> mailboxes
> - * start after TX mailboxes:
> - *
> - * HECC_MAX_RX_MBOX		HECC_RX_BUFFER_MBOX	No of buffer
> mailboxes
> - * 28				12			8
> - * 16				20			4
> + * RX mailbox configuration
> + * The remaining mailboxes are used for reception and are delivered
> based on
> + * their timestamp, to avoid a hardware race when CANME is changed
> while
> + * CAN-bus traffix is being received.
>   */
>  
>  #define HECC_MAX_RX_MBOX	(HECC_MAX_MAILBOXES - HECC_MAX_TX_MBOX)
> -#define HECC_RX_BUFFER_MBOX	12 /* as per table above */
>  #define HECC_RX_FIRST_MBOX	(HECC_MAX_MAILBOXES - 1)
> -#define HECC_RX_HIGH_MBOX_MASK	(~(BIT(HECC_RX_BUFFER_MBOX) -
> 1))
>  
>  /* TI HECC module registers */
>  #define HECC_CANME		0x0	/* Mailbox enable */
> @@ -123,6 +112,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
>  #define HECC_CANMDL		0x8
>  #define HECC_CANMDH		0xC
>  
> +#define HECC_CANMOTS		0x100
> +
>  #define HECC_SET_REG		0xFFFFFFFF
>  #define HECC_CANID_MASK		0x3FF	/* 18 bits mask for
> extended id's */
>  #define HECC_CCE_WAIT_COUNT     100	/* Wait for ~1 sec for CCE bit
> */
> @@ -193,7 +184,7 @@ static const struct can_bittiming_const
> ti_hecc_bittiming_const = {
>  
>  struct ti_hecc_priv {
>  	struct can_priv can;	/* MUST be first member/field */
> -	struct napi_struct napi;
> +	struct can_rx_offload offload;
>  	struct net_device *ndev;
>  	struct clk *clk;
>  	void __iomem *base;
> @@ -203,7 +194,6 @@ struct ti_hecc_priv {
>  	spinlock_t mbx_lock; /* CANME register needs protection */
>  	u32 tx_head;
>  	u32 tx_tail;
> -	u32 rx_next;
>  	struct regulator *reg_xceiver;
>  };
>  
> @@ -265,6 +255,11 @@ static inline u32 hecc_get_bit(struct
> ti_hecc_priv *priv, int reg, u32 bit_mask)
>  	return (hecc_read(priv, reg) & bit_mask) ? 1 : 0;
>  }
>  
> +static inline u32 hecc_read_stamp(struct ti_hecc_priv *priv, u32
> mbxno)
> +{
> +	return __raw_readl(priv->hecc_ram + 0x80 + 4 * mbxno);
> +}
> +
>  static int ti_hecc_set_btc(struct ti_hecc_priv *priv)
>  {
>  	struct can_bittiming *bit_timing = &priv->can.bittiming;
> @@ -375,7 +370,6 @@ static void ti_hecc_start(struct net_device
> *ndev)
>  	ti_hecc_reset(ndev);
>  
>  	priv->tx_head = priv->tx_tail = HECC_TX_MASK;
> -	priv->rx_next = HECC_RX_FIRST_MBOX;
>  
>  	/* Enable local and global acceptance mask registers */
>  	hecc_write(priv, HECC_CANGAM, HECC_SET_REG);
> @@ -526,21 +520,17 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff
> *skb, struct net_device *ndev)
>  	return NETDEV_TX_OK;
>  }
>  
> -static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno)
> +static inline struct ti_hecc_priv *rx_offload_to_priv(struct
> can_rx_offload *offload)
>  {
> -	struct net_device_stats *stats = &priv->ndev->stats;
> -	struct can_frame *cf;
> -	struct sk_buff *skb;
> -	u32 data, mbx_mask;
> -	unsigned long flags;
> +	return container_of(offload, struct ti_hecc_priv, offload);
> +}
>  
> -	skb = alloc_can_skb(priv->ndev, &cf);
> -	if (!skb) {
> -		if (printk_ratelimit())
> -			netdev_err(priv->ndev,
> -				"ti_hecc_rx_pkt: alloc_can_skb()
> failed\n");
> -		return -ENOMEM;
> -	}
> +static unsigned int ti_hecc_mailbox_read(struct can_rx_offload
> *offload,
> +					 struct can_frame *cf,
> +					 u32 *timestamp, unsigned int
> mbxno)
> +{
> +	struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
> +	u32 data, mbx_mask;
>  
>  	mbx_mask = BIT(mbxno);
>  	data = hecc_read_mbx(priv, mbxno, HECC_CANMID);
> @@ -558,100 +548,19 @@ static int ti_hecc_rx_pkt(struct ti_hecc_priv
> *priv, int mbxno)
>  		data = hecc_read_mbx(priv, mbxno, HECC_CANMDH);
>  		*(__be32 *)(cf->data + 4) = cpu_to_be32(data);
>  	}
> -	spin_lock_irqsave(&priv->mbx_lock, flags);
> -	hecc_clear_bit(priv, HECC_CANME, mbx_mask);
> -	hecc_write(priv, HECC_CANRMP, mbx_mask);
> -	/* enable mailbox only if it is part of rx buffer mailboxes */
> -	if (priv->rx_next < HECC_RX_BUFFER_MBOX)
> -		hecc_set_bit(priv, HECC_CANME, mbx_mask);
> -	spin_unlock_irqrestore(&priv->mbx_lock, flags);
>  
> -	stats->rx_bytes += cf->can_dlc;
> -	can_led_event(priv->ndev, CAN_LED_EVENT_RX);
> -	netif_receive_skb(skb);
> -	stats->rx_packets++;
> +	*timestamp = hecc_read_stamp(priv, mbxno);
>  
> -	return 0;
> -}
> -
> -/*
> - * ti_hecc_rx_poll - HECC receive pkts
> - *
> - * The receive mailboxes start from highest numbered mailbox till
> last xmit
> - * mailbox. On CAN frame reception the hardware places the data into
> highest
> - * numbered mailbox that matches the CAN ID filter. Since all
> receive mailboxes
> - * have same filtering (ALL CAN frames) packets will arrive in the
> highest
> - * available RX mailbox and we need to ensure in-order packet
> reception.
> - *
> - * To ensure the packets are received in the right order we
> logically divide
> - * the RX mailboxes into main and buffer mailboxes. Packets are
> received as per
> - * mailbox priotity (higher to lower) in the main bank and once it
> is full we
> - * disable further reception into main mailboxes. While the main
> mailboxes are
> - * processed in NAPI, further packets are received in buffer
> mailboxes.
> - *
> - * We maintain a RX next mailbox counter to process packets and once
> all main
> - * mailboxe packets are passed to the upper stack we enable all of
> them but
> - * continue to process packets received in buffer mailboxes. With
> each packet
> - * received from buffer mailbox we enable it immediately so as to
> handle the
> - * overflow from higher mailboxes.
> - */
> -static int ti_hecc_rx_poll(struct napi_struct *napi, int quota)
> -{
> -	struct net_device *ndev = napi->dev;
> -	struct ti_hecc_priv *priv = netdev_priv(ndev);
> -	u32 num_pkts = 0;
> -	u32 mbx_mask;
> -	unsigned long pending_pkts, flags;
> -
> -	if (!netif_running(ndev))
> -		return 0;
> -
> -	while ((pending_pkts = hecc_read(priv, HECC_CANRMP)) &&
> -		num_pkts < quota) {
> -		mbx_mask = BIT(priv->rx_next); /* next rx mailbox to
> process */
> -		if (mbx_mask & pending_pkts) {
> -			if (ti_hecc_rx_pkt(priv, priv->rx_next) < 0)
> -				return num_pkts;
> -			++num_pkts;
> -		} else if (priv->rx_next > HECC_RX_BUFFER_MBOX) {
> -			break; /* pkt not received yet */
> -		}
> -		--priv->rx_next;
> -		if (priv->rx_next == HECC_RX_BUFFER_MBOX) {
> -			/* enable high bank mailboxes */
> -			spin_lock_irqsave(&priv->mbx_lock, flags);
> -			mbx_mask = hecc_read(priv, HECC_CANME);
> -			mbx_mask |= HECC_RX_HIGH_MBOX_MASK;
> -			hecc_write(priv, HECC_CANME, mbx_mask);
> -			spin_unlock_irqrestore(&priv->mbx_lock, flags);
> -		} else if (priv->rx_next == HECC_MAX_TX_MBOX - 1) {
> -			priv->rx_next = HECC_RX_FIRST_MBOX;
> -			break;
> -		}
> -	}
> -
> -	/* Enable packet interrupt if all pkts are handled */
> -	if (hecc_read(priv, HECC_CANRMP) == 0) {
> -		napi_complete(napi);
> -		/* Re-enable RX mailbox interrupts */
> -		mbx_mask = hecc_read(priv, HECC_CANMIM);
> -		mbx_mask |= HECC_TX_MBOX_MASK;
> -		hecc_write(priv, HECC_CANMIM, mbx_mask);
> -	} else {
> -		/* repoll is done only if whole budget is used */
> -		num_pkts = quota;
> -	}
> -
> -	return num_pkts;
> +	return 1;
>  }
>  
>  static int ti_hecc_error(struct net_device *ndev, int int_status,
>  	int err_status)
>  {
>  	struct ti_hecc_priv *priv = netdev_priv(ndev);
> -	struct net_device_stats *stats = &ndev->stats;
>  	struct can_frame *cf;
>  	struct sk_buff *skb;
> +	u32 timestamp;
>  
>  	/* propagate the error condition to the can stack */
>  	skb = alloc_can_err_skb(ndev, &cf);
> @@ -732,9 +641,8 @@ static int ti_hecc_error(struct net_device *ndev,
> int int_status,
>  		}
>  	}
>  
> -	stats->rx_packets++;
> -	stats->rx_bytes += cf->can_dlc;
> -	netif_rx(skb);
> +	timestamp = hecc_read(priv, HECC_CANLNT);
> +	can_rx_offload_queue_sorted(&priv->offload, skb, timestamp);
>  
>  	return 0;
>  }
> @@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq,
> void *dev_id)
>  	struct net_device *ndev = (struct net_device *)dev_id;
>  	struct ti_hecc_priv *priv = netdev_priv(ndev);
>  	struct net_device_stats *stats = &ndev->stats;
> -	u32 mbxno, mbx_mask, int_status, err_status;
> -	unsigned long ack, flags;
> +	u32 mbxno, mbx_mask, int_status, err_status, stamp;

Reverse xmas tree.

Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4699 bytes --]

On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
> As already mentioned in [1] and included in [2], there is an off by one
> issue since the high bank is already enabled when the _next_ mailbox to
> be read has index 12, so the mailbox being read was 13. The message can
> therefore go into mailbox 31 and the driver will be repolled until the
> mailbox 12 eventually receives a msg. Or the message might end up in the
> 12th mailbox, but then it would become disabled after reading it and only
> be enabled again in the next "round" after mailbox 13 was read, which can
> cause out of order messages, since the lower priority mailboxes can
> accept messages in the meantime.
> 
> As mentioned in [3] there is a hardware race condition when changing the
> CANME register while messages are being received. Even when including a
> busy poll on reception, like in [2] there are still overflows and out of
> order messages at times, but less then without the busy loop polling.
> Unlike what the patch suggests, the polling time is not in the microsecond
> range, but takes as long as a current CAN bus reception needs to finish,
> so typically more in the fraction of millisecond range. Since the timeout
> is in jiffies it won't timeout.
> 
> Even with these additional fixes the driver is still not able to provide a
> proper FIFO which doesn't drop packages. So change the driver to use
> rx-offload and base order on timestamp instead of message box numbers. As
> a side affect, this also fixes [4] and [5].
> 
> Before this change messages with a single byte counter were dropped /
> received out of order at a bitrate of 250kbit/s on an am3517. With this
> patch that no longer occurs up to and including 1Mbit/s.
> 
> [1] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post6
> [2] http://arago-project.org/git/projects/?p=linux-omap3.git;a=commit;h=02346892777f07245de4d5af692513ebd852dcb2
> [3] https://linux-can.vger.kernel.narkive.com/zgO9inVi/patch-can-ti-hecc-fix-rx-wrong-sequence-issue#post5
> [4] https://patchwork.ozlabs.org/patch/895956/
> [5] https://www.spinics.net/lists/netdev/msg494971.html
> 
> Cc: Anant Gole <anantgole@ti.com>
> Cc: AnilKumar Ch <anilkumar@ti.com>
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>

[...]

> @@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>  	struct net_device *ndev = (struct net_device *)dev_id;
>  	struct ti_hecc_priv *priv = netdev_priv(ndev);
>  	struct net_device_stats *stats = &ndev->stats;
> -	u32 mbxno, mbx_mask, int_status, err_status;
> -	unsigned long ack, flags;
> +	u32 mbxno, mbx_mask, int_status, err_status, stamp;
> +	unsigned long flags, rx_pending;
>  
>  	int_status = hecc_read(priv,
>  		(priv->use_hecc1int) ? HECC_CANGIF1 : HECC_CANGIF0);
> @@ -769,11 +677,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>  			spin_lock_irqsave(&priv->mbx_lock, flags);
>  			hecc_clear_bit(priv, HECC_CANME, mbx_mask);
>  			spin_unlock_irqrestore(&priv->mbx_lock, flags);
> -			stats->tx_bytes += hecc_read_mbx(priv, mbxno,
> -						HECC_CANMCF) & 0xF;
> +			stamp = hecc_read_stamp(priv, mbxno);
> +			stats->tx_bytes += can_rx_offload_get_echo_skb(&priv->offload,
> +									mbxno, stamp);
>  			stats->tx_packets++;
>  			can_led_event(ndev, CAN_LED_EVENT_TX);
> -			can_get_echo_skb(ndev, mbxno);
>  			--priv->tx_tail;
>  		}
>  
> @@ -784,12 +692,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>  		((priv->tx_head & HECC_TX_MASK) == HECC_TX_MASK)))
>  			netif_wake_queue(ndev);
>  
> -		/* Disable RX mailbox interrupts and let NAPI reenable them */
> -		if (hecc_read(priv, HECC_CANRMP)) {
> -			ack = hecc_read(priv, HECC_CANMIM);
> -			ack &= BIT(HECC_MAX_TX_MBOX) - 1;
> -			hecc_write(priv, HECC_CANMIM, ack);
> -			napi_schedule(&priv->napi);
> +		/* offload RX mailboxes and let NAPI deliver them */
> +		while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
> +			can_rx_offload_irq_offload_timestamp(&priv->offload,
> +							     rx_pending);
> +			hecc_write(priv, HECC_CANRMP, rx_pending);

Can prepare a patch to move the RMP writing into the mailbox_read()
function. This makes the mailbox available a bit earlier and works
better for memory pressure situations, where no skb can be allocated.

>  		}
>  	}

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

Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

[Jeroen Hofstee]

Hello Marc,

On 7/26/19 1:48 PM, Marc Kleine-Budde wrote:
> On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
>
>> @@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>>   	struct net_device *ndev = (struct net_device *)dev_id;
>>   	struct ti_hecc_priv *priv = netdev_priv(ndev);
>>   	struct net_device_stats *stats = &ndev->stats;
>> -	u32 mbxno, mbx_mask, int_status, err_status;
>> -	unsigned long ack, flags;
>> +	u32 mbxno, mbx_mask, int_status, err_status, stamp;
>> +	unsigned long flags, rx_pending;
>>   
>>   	int_status = hecc_read(priv,
>>   		(priv->use_hecc1int) ? HECC_CANGIF1 : HECC_CANGIF0);
>> @@ -769,11 +677,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>>   			spin_lock_irqsave(&priv->mbx_lock, flags);
>>   			hecc_clear_bit(priv, HECC_CANME, mbx_mask);
>>   			spin_unlock_irqrestore(&priv->mbx_lock, flags);
>> -			stats->tx_bytes += hecc_read_mbx(priv, mbxno,
>> -						HECC_CANMCF) & 0xF;
>> +			stamp = hecc_read_stamp(priv, mbxno);
>> +			stats->tx_bytes += can_rx_offload_get_echo_skb(&priv->offload,
>> +									mbxno, stamp);
>>   			stats->tx_packets++;
>>   			can_led_event(ndev, CAN_LED_EVENT_TX);
>> -			can_get_echo_skb(ndev, mbxno);
>>   			--priv->tx_tail;
>>   		}
>>   
>> @@ -784,12 +692,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>>   		((priv->tx_head & HECC_TX_MASK) == HECC_TX_MASK)))
>>   			netif_wake_queue(ndev);
>>   
>> -		/* Disable RX mailbox interrupts and let NAPI reenable them */
>> -		if (hecc_read(priv, HECC_CANRMP)) {
>> -			ack = hecc_read(priv, HECC_CANMIM);
>> -			ack &= BIT(HECC_MAX_TX_MBOX) - 1;
>> -			hecc_write(priv, HECC_CANMIM, ack);
>> -			napi_schedule(&priv->napi);
>> +		/* offload RX mailboxes and let NAPI deliver them */
>> +		while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
>> +			can_rx_offload_irq_offload_timestamp(&priv->offload,
>> +							     rx_pending);
>> +			hecc_write(priv, HECC_CANRMP, rx_pending);
> Can prepare a patch to move the RMP writing into the mailbox_read()
> function. This makes the mailbox available a bit earlier and works
> better for memory pressure situations, where no skb can be allocated.


For my understanding, is there any other reason for alloc_can_skb,
to fail, besides being out of memory. I couldn't easily find an other
limit enforced on it.

If it is actually _moved_, as you suggested, it does loose the ability to
handle the newly received messages while the messages are read
in the same interrupt, so it needs to interrupt again. That will work,
but seems a bit a silly thing to do.

Perhaps we can do both? Mark the mailbox as available in
mailbox_read, so it is available as soon as possible and clear
them all in the irq (under the assumption that alloc_can_skb
failure means real big trouble, why would we want to keep the
old messages around and eventually ignore the new messages?).

Another question, not related to this patch, but this driver..
Most of the times the irq handles 1 or sometimes 2 messages.
Do you happen to know if it is possible to optionally delay the irq
a bit in the millisecond range, so more work can be done in a single
interrupt? Since there are now 28 rx hardware mailboxes available,
it shouldn't run out easily.

And as last, I guess you want a patch which applies to
linux-can-next/testing, right?

With kind regards,

Jeroen