[PATCH 0/5 resend] can: various fixes to xilinx_can driver

[PATCH 0/5 resend] can: various fixes to xilinx_can driver

[Anssi Hannula]

Hi all,

I submitted a few bugfixes in Feb 2017 ("can: various fixes to
xilinx_can driver") but they were not applied, so here they are again.

I seem to have missed CCing Michal Simek (the maintainer per
MAINTAINERS) last time, but I've included him in the loop now so he can
comment.

The last patch saw several revisions after comments from Marc, the one here
is the same as the last version posted back then.

I've rebased them on top of linux-can/master and tested on Zynq-7000 HW.


Anssi Hannula (5):
      can: xilinx_can: fix device dropping off bus on RX overrun
      can: xilinx_can: fix RX loop if RXNEMP is asserted without RXOK
      can: xilinx_can: fix recovery from error states not being propagated
      can: xilinx_can: only report warning and passive states on state changes
      can: xilinx_can: keep only 1-2 frames in TX FIFO to fix TX accounting

 drivers/net/can/xilinx_can.c | 314 +++++++++++++++++++++++++++------
 1 file changed, 256 insertions(+), 58 deletions(-)

-- 
Anssi Hannula / Bitwise Oy

[PATCH 1/5] can: xilinx_can: fix device dropping off bus on RX overrun

[Anssi Hannula]

The xilinx_can driver performs a software reset when an RX overrun is
detected. This causes the device to enter Configuration mode where no
messages are received or transmitted.

The documentation does not mention any need to perform a reset on an RX
overrun, and testing by inducing an RX overflow also indicated that the
device continues to work just fine without a reset.

Remove the software reset.

Tested with the integrated CAN on Zynq-7000 SoC.

Fixes: b1201e44f50b ("can: xilinx CAN controller support")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Cc: <stable@vger.kernel.org>
---
 drivers/net/can/xilinx_can.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 89aec07..389a960 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -600,7 +600,6 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	if (isr & XCAN_IXR_RXOFLW_MASK) {
 		stats->rx_over_errors++;
 		stats->rx_errors++;
-		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
 		if (skb) {
 			cf->can_id |= CAN_ERR_CRTL;
 			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
-- 
2.8.3

[PATCH 2/5] can: xilinx_can: fix RX loop if RXNEMP is asserted without RXOK

[Anssi Hannula]

If the device gets into a state where RXNEMP (RX FIFO not empty)
interrupt is asserted without RXOK (new frame received successfully)
interrupt being asserted, xcan_rx_poll() will continue to try to clear
RXNEMP without actually reading frames from RX FIFO. If the RX FIFO is
not empty, the interrupt will not be cleared and napi_schedule() will
just be called again.

This situation can occur when:

(a) xcan_rx() returns without reading RX FIFO due to an error condition.
The code tries to clear both RXOK and RXNEMP but RXNEMP will not clear
due to a frame still being in the FIFO. The frame will never be read
from the FIFO as RXOK is no longer set.

(b) A frame is received between xcan_rx_poll() reading interrupt status
and clearing RXOK. RXOK will be cleared, but RXNEMP will again remain
set as the new message is still in the FIFO.

I'm able to trigger case (b) by flooding the bus with frames under load.

There does not seem to be any benefit in using both RXNEMP and RXOK in
the way the driver does, and the polling example in the reference manual
(UG585 v1.10 18.3.7 Read Messages from RxFIFO) also says that either
RXOK or RXNEMP can be used for detecting incoming messages.

Fix the issue and simplify the RX processing by only using RXNEMP
without RXOK.

Tested with the integrated CAN on Zynq-7000 SoC.

Fixes: b1201e44f50b ("can: xilinx CAN controller support")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Cc: <stable@vger.kernel.org>
---
 drivers/net/can/xilinx_can.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 389a960..1bda47a 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -101,7 +101,7 @@ enum xcan_reg {
 #define XCAN_INTR_ALL		(XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |\
 				 XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK | \
 				 XCAN_IXR_RXNEMP_MASK | XCAN_IXR_ERROR_MASK | \
-				 XCAN_IXR_ARBLST_MASK | XCAN_IXR_RXOK_MASK)
+				 XCAN_IXR_ARBLST_MASK)
 
 /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
 #define XCAN_BTR_SJW_SHIFT		7  /* Synchronous jump width */
@@ -708,15 +708,7 @@ static int xcan_rx_poll(struct napi_struct *napi, int quota)
 
 	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) {
-		if (isr & XCAN_IXR_RXOK_MASK) {
-			priv->write_reg(priv, XCAN_ICR_OFFSET,
-				XCAN_IXR_RXOK_MASK);
-			work_done += xcan_rx(ndev);
-		} else {
-			priv->write_reg(priv, XCAN_ICR_OFFSET,
-				XCAN_IXR_RXNEMP_MASK);
-			break;
-		}
+		work_done += xcan_rx(ndev);
 		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_RXNEMP_MASK);
 		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	}
@@ -727,7 +719,7 @@ static int xcan_rx_poll(struct napi_struct *napi, int quota)
 	if (work_done < quota) {
 		napi_complete_done(napi, work_done);
 		ier = priv->read_reg(priv, XCAN_IER_OFFSET);
-		ier |= (XCAN_IXR_RXOK_MASK | XCAN_IXR_RXNEMP_MASK);
+		ier |= XCAN_IXR_RXNEMP_MASK;
 		priv->write_reg(priv, XCAN_IER_OFFSET, ier);
 	}
 	return work_done;
@@ -799,9 +791,9 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id)
 	}
 
 	/* Check for the type of receive interrupt and Processing it */
-	if (isr & (XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK)) {
+	if (isr & XCAN_IXR_RXNEMP_MASK) {
 		ier = priv->read_reg(priv, XCAN_IER_OFFSET);
-		ier &= ~(XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK);
+		ier &= ~XCAN_IXR_RXNEMP_MASK;
 		priv->write_reg(priv, XCAN_IER_OFFSET, ier);
 		napi_schedule(&priv->napi);
 	}
-- 
2.8.3

[PATCH 3/5] can: xilinx_can: fix recovery from error states not being propagated

[Anssi Hannula]

The xilinx_can driver contains no mechanism for propagating recovery
from CAN_STATE_ERROR_WARNING and CAN_STATE_ERROR_PASSIVE.

Add such a mechanism by factoring the handling of
XCAN_STATE_ERROR_PASSIVE and XCAN_STATE_ERROR_WARNING out of
xcan_err_interrupt and checking for recovery after RX and TX if the
interface is in one of those states.

Tested with the integrated CAN on Zynq-7000 SoC.

Fixes: b1201e44f50b ("can: xilinx CAN controller support")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/xilinx_can.c | 155 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 127 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 1bda47a..763408a 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -2,6 +2,7 @@
  *
  * Copyright (C) 2012 - 2014 Xilinx, Inc.
  * Copyright (C) 2009 PetaLogix. All rights reserved.
+ * Copyright (C) 2017 Sandvik Mining and Construction Oy
  *
  * Description:
  * This driver is developed for Axi CAN IP and for Zynq CANPS Controller.
@@ -530,6 +531,123 @@ static int xcan_rx(struct net_device *ndev)
 }
 
 /**
+ * xcan_current_error_state - Get current error state from HW
+ * @ndev:	Pointer to net_device structure
+ *
+ * Checks the current CAN error state from the HW. Note that this
+ * only checks for ERROR_PASSIVE and ERROR_WARNING.
+ *
+ * Return:
+ * ERROR_PASSIVE or ERROR_WARNING if either is active, ERROR_ACTIVE
+ * otherwise.
+ */
+static enum can_state xcan_current_error_state(struct net_device *ndev)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	u32 status = priv->read_reg(priv, XCAN_SR_OFFSET);
+
+	if ((status & XCAN_SR_ESTAT_MASK) == XCAN_SR_ESTAT_MASK)
+		return CAN_STATE_ERROR_PASSIVE;
+	else if (status & XCAN_SR_ERRWRN_MASK)
+		return CAN_STATE_ERROR_WARNING;
+	else
+		return CAN_STATE_ERROR_ACTIVE;
+}
+
+/**
+ * xcan_set_error_state - Set new CAN error state
+ * @ndev:	Pointer to net_device structure
+ * @new_state:	The new CAN state to be set
+ * @cf:		Error frame to be populated or NULL
+ *
+ * Set new CAN error state for the device, updating statistics and
+ * populating the error frame if given.
+ */
+static void xcan_set_error_state(struct net_device *ndev,
+				 enum can_state new_state,
+				 struct can_frame *cf)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	u32 ecr = priv->read_reg(priv, XCAN_ECR_OFFSET);
+	u32 txerr = ecr & XCAN_ECR_TEC_MASK;
+	u32 rxerr = (ecr & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT;
+
+	priv->can.state = new_state;
+
+	if (cf) {
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[6] = txerr;
+		cf->data[7] = rxerr;
+	}
+
+	switch (new_state) {
+	case CAN_STATE_ERROR_PASSIVE:
+		priv->can.can_stats.error_passive++;
+		if (cf)
+			cf->data[1] = (rxerr > 127) ?
+					CAN_ERR_CRTL_RX_PASSIVE :
+					CAN_ERR_CRTL_TX_PASSIVE;
+		break;
+	case CAN_STATE_ERROR_WARNING:
+		priv->can.can_stats.error_warning++;
+		if (cf)
+			cf->data[1] |= (txerr > rxerr) ?
+					CAN_ERR_CRTL_TX_WARNING :
+					CAN_ERR_CRTL_RX_WARNING;
+		break;
+	case CAN_STATE_ERROR_ACTIVE:
+		if (cf)
+			cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
+		break;
+	default:
+		/* non-ERROR states are handled elsewhere */
+		WARN_ON(1);
+		break;
+	}
+}
+
+/**
+ * xcan_update_error_state_after_rxtx - Update CAN error state after RX/TX
+ * @ndev:	Pointer to net_device structure
+ *
+ * If the device is in a ERROR-WARNING or ERROR-PASSIVE state, check if
+ * the performed RX/TX has caused it to drop to a lesser state and set
+ * the interface state accordingly.
+ */
+static void xcan_update_error_state_after_rxtx(struct net_device *ndev)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	enum can_state old_state = priv->can.state;
+	enum can_state new_state;
+
+	/* changing error state due to successful frame RX/TX can only
+	 * occur from these states
+	 */
+	if (old_state != CAN_STATE_ERROR_WARNING &&
+	    old_state != CAN_STATE_ERROR_PASSIVE)
+		return;
+
+	new_state = xcan_current_error_state(ndev);
+
+	if (new_state != old_state) {
+		struct sk_buff *skb;
+		struct can_frame *cf;
+
+		skb = alloc_can_err_skb(ndev, &cf);
+
+		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
+
+		if (skb) {
+			struct net_device_stats *stats = &ndev->stats;
+
+			stats->rx_packets++;
+			stats->rx_bytes += cf->can_dlc;
+			netif_rx(skb);
+		}
+	}
+}
+
+/**
  * xcan_err_interrupt - error frame Isr
  * @ndev:	net_device pointer
  * @isr:	interrupt status register value
@@ -544,16 +662,12 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	struct net_device_stats *stats = &ndev->stats;
 	struct can_frame *cf;
 	struct sk_buff *skb;
-	u32 err_status, status, txerr = 0, rxerr = 0;
+	u32 err_status;
 
 	skb = alloc_can_err_skb(ndev, &cf);
 
 	err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
 	priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
-	txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
-	rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
-			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
-	status = priv->read_reg(priv, XCAN_SR_OFFSET);
 
 	if (isr & XCAN_IXR_BSOFF_MASK) {
 		priv->can.state = CAN_STATE_BUS_OFF;
@@ -563,28 +677,10 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 		can_bus_off(ndev);
 		if (skb)
 			cf->can_id |= CAN_ERR_BUSOFF;
-	} else if ((status & XCAN_SR_ESTAT_MASK) == XCAN_SR_ESTAT_MASK) {
-		priv->can.state = CAN_STATE_ERROR_PASSIVE;
-		priv->can.can_stats.error_passive++;
-		if (skb) {
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] = (rxerr > 127) ?
-					CAN_ERR_CRTL_RX_PASSIVE :
-					CAN_ERR_CRTL_TX_PASSIVE;
-			cf->data[6] = txerr;
-			cf->data[7] = rxerr;
-		}
-	} else if (status & XCAN_SR_ERRWRN_MASK) {
-		priv->can.state = CAN_STATE_ERROR_WARNING;
-		priv->can.can_stats.error_warning++;
-		if (skb) {
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] |= (txerr > rxerr) ?
-					CAN_ERR_CRTL_TX_WARNING :
-					CAN_ERR_CRTL_RX_WARNING;
-			cf->data[6] = txerr;
-			cf->data[7] = rxerr;
-		}
+	} else {
+		enum can_state new_state = xcan_current_error_state(ndev);
+
+		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
 	}
 
 	/* Check for Arbitration lost interrupt */
@@ -713,8 +809,10 @@ static int xcan_rx_poll(struct napi_struct *napi, int quota)
 		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	}
 
-	if (work_done)
+	if (work_done) {
 		can_led_event(ndev, CAN_LED_EVENT_RX);
+		xcan_update_error_state_after_rxtx(ndev);
+	}
 
 	if (work_done < quota) {
 		napi_complete_done(napi, work_done);
@@ -745,6 +843,7 @@ static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
 		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	}
 	can_led_event(ndev, CAN_LED_EVENT_TX);
+	xcan_update_error_state_after_rxtx(ndev);
 	netif_wake_queue(ndev);
 }
 
-- 
2.8.3

[PATCH 4/5] can: xilinx_can: only report warning and passive states on state changes

[Anssi Hannula]

The xilinx_can driver currently increments error-warning and
error-passive statistics on every error interrupt regardless of whether
the interface was already in the same state. Similarly, the error frame
sent on error interrupts is always sent with
CAN_ERR_CRTL_(RX|TX)_(PASSIVE|WARNING) bit set.

To make the error-warning and error-passive statistics more useful, add
a check to only set the error state when the state has actually been
changed.

Tested with the integrated CAN on Zynq-7000 SoC.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/xilinx_can.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 763408a..862c12bb 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -680,7 +680,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	} else {
 		enum can_state new_state = xcan_current_error_state(ndev);
 
-		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
+		if (new_state != priv->can.state)
+			xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
 	}
 
 	/* Check for Arbitration lost interrupt */
-- 
2.8.3

[PATCH 5/5 v3] can: xilinx_can: keep only 1-2 frames in TX FIFO to fix TX accounting

[Anssi Hannula]

The xilinx_can driver assumes that the TXOK interrupt only clears after
it has been acknowledged as many times as there have been successfully
sent frames.

However, the documentation does not mention such behavior, instead
saying just that the interrupt is cleared when the clear bit is set.

Similarly, testing seems to also suggest that it is immediately cleared
regardless of the amount of frames having been sent. Performing some
heavy TX load and then going back to idle has the tx_head drifting
further away from tx_tail over time, steadily reducing the amount of
frames the driver keeps in the TX FIFO (but not to zero, as the TXOK
interrupt always frees up space for 1 frame from the driver's
perspective, so frames continue to be sent) and delaying the local echo
frames.

The TX FIFO tracking is also otherwise buggy as it does not account for
TX FIFO being cleared after software resets, causing
  BUG!, TX FIFO full when queue awake!
messages to be output.

There does not seem to be any way to accurately track the state of the
TX FIFO for local echo support while using the full TX FIFO.

The Zynq version of the HW (but not the soft-AXI version) has watermark
programming support and with it an additional TX-FIFO-empty interrupt
bit.

Modify the driver to only put 1 frame into TX FIFO at a time on soft-AXI
and 2 frames at a time on Zynq. On Zynq the TXFEMP interrupt bit is used
to detect whether 1 or 2 frames have been sent at interrupt processing
time.

Tested with the integrated CAN on Zynq-7000 SoC. The 1-frame-FIFO mode
was also tested.

An alternative way to solve this would be to drop local echo support but
keep using the full TX FIFO.

v2: Add FIFO space check before TX queue wake with locking to
synchronize with queue stop. This avoids waking the queue when xmit()
had just filled it.

v3: Keep local echo support and reduce the amount of frames in FIFO
instead as suggested by Marc Kleine-Budde.

Fixes: b1201e44f50b ("can: xilinx CAN controller support")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/xilinx_can.c | 139 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 123 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 862c12bb..26754e9 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -26,8 +26,10 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/skbuff.h>
+#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/can/dev.h>
@@ -119,6 +121,7 @@ enum xcan_reg {
 /**
  * struct xcan_priv - This definition define CAN driver instance
  * @can:			CAN private data structure.
+ * @tx_lock:			Lock for synchronizing TX interrupt handling
  * @tx_head:			Tx CAN packets ready to send on the queue
  * @tx_tail:			Tx CAN packets successfully sended on the queue
  * @tx_max:			Maximum number packets the driver can send
@@ -133,6 +136,7 @@ enum xcan_reg {
  */
 struct xcan_priv {
 	struct can_priv can;
+	spinlock_t tx_lock;
 	unsigned int tx_head;
 	unsigned int tx_tail;
 	unsigned int tx_max;
@@ -160,6 +164,11 @@ static const struct can_bittiming_const xcan_bittiming_const = {
 	.brp_inc = 1,
 };
 
+#define XCAN_CAP_WATERMARK	0x0001
+struct xcan_devtype_data {
+	unsigned int caps;
+};
+
 /**
  * xcan_write_reg_le - Write a value to the device register little endian
  * @priv:	Driver private data structure
@@ -239,6 +248,10 @@ static int set_reset_mode(struct net_device *ndev)
 		usleep_range(500, 10000);
 	}
 
+	/* reset clears FIFOs */
+	priv->tx_head = 0;
+	priv->tx_tail = 0;
+
 	return 0;
 }
 
@@ -393,6 +406,7 @@ static int xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	struct net_device_stats *stats = &ndev->stats;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	u32 id, dlc, data[2] = {0, 0};
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(ndev, skb))
 		return NETDEV_TX_OK;
@@ -440,6 +454,9 @@ static int xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
 
 	can_put_echo_skb(skb, ndev, priv->tx_head % priv->tx_max);
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
 	priv->tx_head++;
 
 	/* Write the Frame to Xilinx CAN TX FIFO */
@@ -455,10 +472,16 @@ static int xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		stats->tx_bytes += cf->can_dlc;
 	}
 
+	/* Clear TX-FIFO-empty interrupt for xcan_tx_interrupt() */
+	if (priv->tx_max > 1)
+		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXFEMP_MASK);
+
 	/* Check if the TX buffer is full */
 	if ((priv->tx_head - priv->tx_tail) == priv->tx_max)
 		netif_stop_queue(ndev);
 
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 	return NETDEV_TX_OK;
 }
 
@@ -833,19 +856,71 @@ static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
 {
 	struct xcan_priv *priv = netdev_priv(ndev);
 	struct net_device_stats *stats = &ndev->stats;
+	unsigned int frames_in_fifo;
+	int frames_sent = 1; /* TXOK => at least 1 frame was sent */
+	unsigned long flags;
+	int retries = 0;
+
+	/* Synchronize with xmit as we need to know the exact number
+	 * of frames in the FIFO to stay in sync due to the TXFEMP
+	 * handling.
+	 * This also prevents a race between netif_wake_queue() and
+	 * netif_stop_queue().
+	 */
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
+	frames_in_fifo = priv->tx_head - priv->tx_tail;
 
-	while ((priv->tx_head - priv->tx_tail > 0) &&
-			(isr & XCAN_IXR_TXOK_MASK)) {
+	if (WARN_ON_ONCE(frames_in_fifo == 0)) {
+		/* clear TXOK anyway to avoid getting back here */
 		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+		return;
+	}
+
+	/* Check if 2 frames were sent (TXOK only means that at least 1
+	 * frame was sent).
+	 */
+	if (frames_in_fifo > 1) {
+		WARN_ON(frames_in_fifo > priv->tx_max);
+
+		/* Synchronize TXOK and isr so that after the loop:
+		 * (1) isr variable is up-to-date at least up to TXOK clear
+		 *     time. This avoids us clearing a TXOK of a second frame
+		 *     but not noticing that the FIFO is now empty and thus
+		 *     marking only a single frame as sent.
+		 * (2) No TXOK is left. Having one could mean leaving a
+		 *     stray TXOK as we might process the associated frame
+		 *     via TXFEMP handling as we read TXFEMP *after* TXOK
+		 *     clear to satisfy (1).
+		 */
+		while ((isr & XCAN_IXR_TXOK_MASK) && !WARN_ON(++retries == 100)) {
+			priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
+			isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
+		}
+
+		if (isr & XCAN_IXR_TXFEMP_MASK) {
+			/* nothing in FIFO anymore */
+			frames_sent = frames_in_fifo;
+		}
+	} else {
+		/* single frame in fifo, just clear TXOK */
+		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
+	}
+
+	while (frames_sent--) {
 		can_get_echo_skb(ndev, priv->tx_tail %
 					priv->tx_max);
 		priv->tx_tail++;
 		stats->tx_packets++;
-		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	}
+
+	netif_wake_queue(ndev);
+
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 	can_led_event(ndev, CAN_LED_EVENT_TX);
 	xcan_update_error_state_after_rxtx(ndev);
-	netif_wake_queue(ndev);
 }
 
 /**
@@ -1152,6 +1227,18 @@ static const struct dev_pm_ops xcan_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
 };
 
+static const struct xcan_devtype_data xcan_zynq_data = {
+	.caps = XCAN_CAP_WATERMARK,
+};
+
+/* Match table for OF platform binding */
+static const struct of_device_id xcan_of_match[] = {
+	{ .compatible = "xlnx,zynq-can-1.0", .data = &xcan_zynq_data },
+	{ .compatible = "xlnx,axi-can-1.00.a", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, xcan_of_match);
+
 /**
  * xcan_probe - Platform registration call
  * @pdev:	Handle to the platform device structure
@@ -1166,8 +1253,10 @@ static int xcan_probe(struct platform_device *pdev)
 	struct resource *res; /* IO mem resources */
 	struct net_device *ndev;
 	struct xcan_priv *priv;
+	const struct of_device_id *of_id;
+	int caps = 0;
 	void __iomem *addr;
-	int ret, rx_max, tx_max;
+	int ret, rx_max, tx_max, tx_fifo_depth;
 
 	/* Get the virtual base address for the device */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1177,7 +1266,8 @@ static int xcan_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth", &tx_max);
+	ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
+				   &tx_fifo_depth);
 	if (ret < 0)
 		goto err;
 
@@ -1185,6 +1275,30 @@ static int xcan_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err;
 
+	of_id = of_match_device(xcan_of_match, &pdev->dev);
+	if (of_id) {
+		const struct xcan_devtype_data *devtype_data = of_id->data;
+
+		if (devtype_data)
+			caps = devtype_data->caps;
+	}
+
+	/* There is no way to directly figure out how many frames have been
+	 * sent when the TXOK interrupt is processed. If watermark programming
+	 * is supported, we can have 2 frames in the FIFO and use TXFEMP
+	 * to determine if 1 or 2 frames have been sent.
+	 * Theoretically we should be able to use TXFWMEMP to determine up
+	 * to 3 frames, but it seems that after putting a second frame in the
+	 * FIFO, with watermark at 2 frames, it can happen that TXFWMEMP (less
+	 * than 2 frames in FIFO) is set anyway with no TXOK (a frame was
+	 * sent), which is not a sensible state - possibly TXFWMEMP is not
+	 * completely synchronized with the rest of the bits?
+	 */
+	if (caps & XCAN_CAP_WATERMARK)
+		tx_max = min(tx_fifo_depth, 2);
+	else
+		tx_max = 1;
+
 	/* Create a CAN device instance */
 	ndev = alloc_candev(sizeof(struct xcan_priv), tx_max);
 	if (!ndev)
@@ -1199,6 +1313,7 @@ static int xcan_probe(struct platform_device *pdev)
 					CAN_CTRLMODE_BERR_REPORTING;
 	priv->reg_base = addr;
 	priv->tx_max = tx_max;
+	spin_lock_init(&priv->tx_lock);
 
 	/* Get IRQ for the device */
 	ndev->irq = platform_get_irq(pdev, 0);
@@ -1263,9 +1378,9 @@ static int xcan_probe(struct platform_device *pdev)
 
 	pm_runtime_put(&pdev->dev);
 
-	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
+	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth: actual %d, using %d\n",
 			priv->reg_base, ndev->irq, priv->can.clock.freq,
-			priv->tx_max);
+			tx_fifo_depth, priv->tx_max);
 
 	return 0;
 
@@ -1299,14 +1414,6 @@ static int xcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
-/* Match table for OF platform binding */
-static const struct of_device_id xcan_of_match[] = {
-	{ .compatible = "xlnx,zynq-can-1.0", },
-	{ .compatible = "xlnx,axi-can-1.00.a", },
-	{ /* end of list */ },
-};
-MODULE_DEVICE_TABLE(of, xcan_of_match);
-
 static struct platform_driver xcan_driver = {
 	.probe = xcan_probe,
 	.remove	= xcan_remove,
-- 
2.8.3

Re: [PATCH 4/5] can: xilinx_can: only report warning and passive states on state changes

[Marc Kleine-Budde]

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

On 01/26/2018 03:27 PM, Anssi Hannula wrote:
> The xilinx_can driver currently increments error-warning and
> error-passive statistics on every error interrupt regardless of whether
> the interface was already in the same state. Similarly, the error frame
> sent on error interrupts is always sent with
> CAN_ERR_CRTL_(RX|TX)_(PASSIVE|WARNING) bit set.
> 
> To make the error-warning and error-passive statistics more useful, add
> a check to only set the error state when the state has actually been
> changed.
> 
> Tested with the integrated CAN on Zynq-7000 SoC.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> ---
>  drivers/net/can/xilinx_can.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 763408a..862c12bb 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -680,7 +680,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>  	} else {
>  		enum can_state new_state = xcan_current_error_state(ndev);
>  
> -		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
> +		if (new_state != priv->can.state)
> +			xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
>  	}

Are you changing a piece of code, that you've introduced in patch 3? If
so, I'm going to squash these two patches together.

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 4/5] can: xilinx_can: only report warning and passive states on state changes

[Anssi Hannula]

On 29.1.2018 10:35, Marc Kleine-Budde wrote:
> On 01/26/2018 03:27 PM, Anssi Hannula wrote:
>> The xilinx_can driver currently increments error-warning and
>> error-passive statistics on every error interrupt regardless of whether
>> the interface was already in the same state. Similarly, the error frame
>> sent on error interrupts is always sent with
>> CAN_ERR_CRTL_(RX|TX)_(PASSIVE|WARNING) bit set.
>>
>> To make the error-warning and error-passive statistics more useful, add
>> a check to only set the error state when the state has actually been
>> changed.
>>
>> Tested with the integrated CAN on Zynq-7000 SoC.
>>
>> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
>> ---
>>  drivers/net/can/xilinx_can.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>> index 763408a..862c12bb 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -680,7 +680,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>>  	} else {
>>  		enum can_state new_state = xcan_current_error_state(ndev);
>>  
>> -		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
>> +		if (new_state != priv->can.state)
>> +			xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
>>  	}
> Are you changing a piece of code, that you've introduced in patch 3? If
> so, I'm going to squash these two patches together.

Yes, patch 3 keeps the state change reporting behavior (always vs. on
state change) like it was before, only introducing reporting recovery
from error states.

But I'm OK with squashing, as long as the commit message is updated to
say it changes that behavior, too.


-- 
Anssi Hannula / Bitwise Oy
+358503803997

Re: [PATCH 4/5] can: xilinx_can: only report warning and passive states on state changes

[Marc Kleine-Budde]

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

On 01/29/2018 10:45 AM, Anssi Hannula wrote:
> On 29.1.2018 10:35, Marc Kleine-Budde wrote:
>> On 01/26/2018 03:27 PM, Anssi Hannula wrote:
>>> The xilinx_can driver currently increments error-warning and
>>> error-passive statistics on every error interrupt regardless of whether
>>> the interface was already in the same state. Similarly, the error frame
>>> sent on error interrupts is always sent with
>>> CAN_ERR_CRTL_(RX|TX)_(PASSIVE|WARNING) bit set.
>>>
>>> To make the error-warning and error-passive statistics more useful, add
>>> a check to only set the error state when the state has actually been
>>> changed.
>>>
>>> Tested with the integrated CAN on Zynq-7000 SoC.
>>>
>>> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
>>> ---
>>>  drivers/net/can/xilinx_can.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>>> index 763408a..862c12bb 100644
>>> --- a/drivers/net/can/xilinx_can.c
>>> +++ b/drivers/net/can/xilinx_can.c
>>> @@ -680,7 +680,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>>>  	} else {
>>>  		enum can_state new_state = xcan_current_error_state(ndev);
>>>  
>>> -		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
>>> +		if (new_state != priv->can.state)
>>> +			xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
>>>  	}
>> Are you changing a piece of code, that you've introduced in patch 3? If
>> so, I'm going to squash these two patches together.
> 
> Yes, patch 3 keeps the state change reporting behavior (always vs. on
> state change) like it was before, only introducing reporting recovery
> from error states.

OK I see - then better leave it as two separate patches.

> But I'm OK with squashing, as long as the commit message is updated to
> say it changes that behavior, too.

tnx,
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 3/5] can: xilinx_can: fix recovery from error states not being propagated

[Andri Yngvason]

Hello Anssi

Quoting Anssi Hannula (2018-01-26 14:27:21)
> The xilinx_can driver contains no mechanism for propagating recovery
> from CAN_STATE_ERROR_WARNING and CAN_STATE_ERROR_PASSIVE.
> 
> Add such a mechanism by factoring the handling of
> XCAN_STATE_ERROR_PASSIVE and XCAN_STATE_ERROR_WARNING out of
> xcan_err_interrupt and checking for recovery after RX and TX if the
> interface is in one of those states.
> 
> Tested with the integrated CAN on Zynq-7000 SoC.
> 
> Fixes: b1201e44f50b ("can: xilinx CAN controller support")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> ---
>  drivers/net/can/xilinx_can.c | 155 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 127 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 1bda47a..763408a 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
...

There is a function in drivers/net/can/dev.c that does this:
> +
> +/**
> + * xcan_set_error_state - Set new CAN error state
> + * @ndev:      Pointer to net_device structure
> + * @new_state: The new CAN state to be set
> + * @cf:                Error frame to be populated or NULL
> + *
> + * Set new CAN error state for the device, updating statistics and
> + * populating the error frame if given.
> + */
> +static void xcan_set_error_state(struct net_device *ndev,
> +                                enum can_state new_state,
> +                                struct can_frame *cf)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       u32 ecr = priv->read_reg(priv, XCAN_ECR_OFFSET);
> +       u32 txerr = ecr & XCAN_ECR_TEC_MASK;
> +       u32 rxerr = (ecr & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT;
> +
> +       priv->can.state = new_state;
> +
> +       if (cf) {
> +               cf->can_id |= CAN_ERR_CRTL;
> +               cf->data[6] = txerr;
> +               cf->data[7] = rxerr;
> +       }
> +
> +       switch (new_state) {
> +       case CAN_STATE_ERROR_PASSIVE:
> +               priv->can.can_stats.error_passive++;
> +               if (cf)
> +                       cf->data[1] = (rxerr > 127) ?
> +                                       CAN_ERR_CRTL_RX_PASSIVE :
> +                                       CAN_ERR_CRTL_TX_PASSIVE;
> +               break;
> +       case CAN_STATE_ERROR_WARNING:
> +               priv->can.can_stats.error_warning++;
> +               if (cf)
> +                       cf->data[1] |= (txerr > rxerr) ?
> +                                       CAN_ERR_CRTL_TX_WARNING :
> +                                       CAN_ERR_CRTL_RX_WARNING;
> +               break;
> +       case CAN_STATE_ERROR_ACTIVE:
> +               if (cf)
> +                       cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
> +               break;
> +       default:
> +               /* non-ERROR states are handled elsewhere */
> +               WARN_ON(1);
> +               break;
> +       }
> +}
...

It's not a big function, but the idea was to try to make this behaviour
consistent between drivers by putting it into a function.

Regards,
Andri

[PATCH] can: xilinx_can: use can_change_state()

[Anssi Hannula]

Replace some custom code with a call to can_change_state().

This subtly changes the error reporting behavior in some cases.

Previously, if both RX and TX error counters indicated the same state,
and the status register agreed:
- if overall state is PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE
- if overall state is WARNING, report CAN_ERR_CRTL_TX_WARNING or
  CAN_ERR_CRTL_RX_WARNING depending on which counter is higher.

Previously, if status registers indicated a different state than the
worst state indicated by the RX/TX error counters (I have not observed
this to happen, though):
- if status register says PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE if
  RX error counter indicates PASSIVE as well, otherwise report
  CAN_ERR_CRTL_TX_PASSIVE
- if status register says WARNING, report CAN_ERR_CRTL_TX_WARNING
  or CAN_ERR_CRTL_RX_WARNING depending on which counter is higher.

After this commit, in both of the above scenarios:
- if overall state is PASSIVE, report
  CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE
- if overall state is WARNING, report
  CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING.

Tested with the integrated CAN on Zynq-7000 SoC.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---

Andri Yngvason wrote:
> There is a function in drivers/net/can/dev.c that does this:
[...]
> It's not a big function, but the idea was to try to make this behaviour
> consistent between drivers by putting it into a function.

I didn't use it because I didn't have separate TX and RX states
available, and if I calculated them based on RX/TX error counters the
behavior would still be subtly different.

So I opted to just keep the previous error frame generation code as-is
to make sure the only behavior changed was the recovery from error
states being fixed, like the commit message said.

But here is a patch that makes it use can_change_state(), on top of the
posted patchset.

If wanted, I can squash this one into patch 3 (and 4), though I'd prefer
it to be kept separate as it introduces subtle behavior changes
unrelated to the actual fix in patch 3.


 drivers/net/can/xilinx_can.c | 63 +++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 26754e9..9525016 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -578,6 +578,27 @@ static enum can_state xcan_current_error_state(struct net_device *ndev)
 }
 
 /**
+ * xcan_errcounter_to_state - Get state based on error counter
+ * @err:	RX/TX error counter
+ *
+ * Get the CAN error state based on just the error counter. This is useful
+ * for determining RX and TX states separately as we only get a single global
+ * state from the device.
+ *
+ * Return:
+ * The CAN state based on the error counter.
+ */
+static enum can_state xcan_errcounter_to_state(u32 err)
+{
+	if (err >= 128)
+		return CAN_STATE_ERROR_PASSIVE;
+	else if (err >= 96)
+		return CAN_STATE_ERROR_WARNING;
+
+	return CAN_STATE_ERROR_ACTIVE;
+}
+
+/**
  * xcan_set_error_state - Set new CAN error state
  * @ndev:	Pointer to net_device structure
  * @new_state:	The new CAN state to be set
@@ -595,38 +616,26 @@ static void xcan_set_error_state(struct net_device *ndev,
 	u32 txerr = ecr & XCAN_ECR_TEC_MASK;
 	u32 rxerr = (ecr & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT;
 
-	priv->can.state = new_state;
+	enum can_state tx_state = xcan_errcounter_to_state(txerr);
+	enum can_state rx_state = xcan_errcounter_to_state(rxerr);
+
+	/* non-ERROR states are handled elsewhere */
+	if (WARN_ON(new_state > CAN_STATE_ERROR_PASSIVE))
+		return;
+
+	if (max(rx_state, tx_state) != new_state) {
+		/* hw has a different idea of the state, just set both
+		 * tx and rx states to the same value
+		 */
+		rx_state = tx_state = new_state;
+	}
+
+	can_change_state(ndev, cf, tx_state, rx_state);
 
 	if (cf) {
-		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
 	}
-
-	switch (new_state) {
-	case CAN_STATE_ERROR_PASSIVE:
-		priv->can.can_stats.error_passive++;
-		if (cf)
-			cf->data[1] = (rxerr > 127) ?
-					CAN_ERR_CRTL_RX_PASSIVE :
-					CAN_ERR_CRTL_TX_PASSIVE;
-		break;
-	case CAN_STATE_ERROR_WARNING:
-		priv->can.can_stats.error_warning++;
-		if (cf)
-			cf->data[1] |= (txerr > rxerr) ?
-					CAN_ERR_CRTL_TX_WARNING :
-					CAN_ERR_CRTL_RX_WARNING;
-		break;
-	case CAN_STATE_ERROR_ACTIVE:
-		if (cf)
-			cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
-		break;
-	default:
-		/* non-ERROR states are handled elsewhere */
-		WARN_ON(1);
-		break;
-	}
 }
 
 /**
-- 
2.8.3

Re: [PATCH] can: xilinx_can: use can_change_state()

[Andri Yngvason]

Hi Anssi

I wasn't sure you knew about can_change_state(). I didn't mean to say that you
_should_ use it, just that you could.

Quoting Anssi Hannula (2018-01-29 16:49:53)
> Replace some custom code with a call to can_change_state().
> 
> This subtly changes the error reporting behavior in some cases.
> 
> Previously, if both RX and TX error counters indicated the same state,
> and the status register agreed:
> - if overall state is PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE
> - if overall state is WARNING, report CAN_ERR_CRTL_TX_WARNING or
>   CAN_ERR_CRTL_RX_WARNING depending on which counter is higher.
If you look at sja1000.c, you'll see that it does this:
tx_state = txerr >= rxerr ? state : 0
rx_state = txerr <= rxerr ? state : 0;
can_change_state(dev, cf, tx_state, rx_state);

Maybe something similar could work for this?
> 
> Previously, if status registers indicated a different state than the
> worst state indicated by the RX/TX error counters (I have not observed
> this to happen, though):
> - if status register says PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE if
>   RX error counter indicates PASSIVE as well, otherwise report
>   CAN_ERR_CRTL_TX_PASSIVE
> - if status register says WARNING, report CAN_ERR_CRTL_TX_WARNING
>   or CAN_ERR_CRTL_RX_WARNING depending on which counter is higher.
> 
> After this commit, in both of the above scenarios:
> - if overall state is PASSIVE, report
>   CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE
> - if overall state is WARNING, report
>   CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING.
> 
> Tested with the integrated CAN on Zynq-7000 SoC.
> 

Anyway, the use of this API function is a somewhat superfluous issue, so I think
we shouldn't worry too much about it.

Best regards,
Andri

Re: [PATCH] can: xilinx_can: use can_change_state()

[Anssi Hannula]

On 29.1.2018 19:50, Andri Yngvason wrote:
> Hi Anssi
Hi!

> I wasn't sure you knew about can_change_state(). I didn't mean to say that you
> _should_ use it, just that you could.

OK :)

> Quoting Anssi Hannula (2018-01-29 16:49:53)
>> Replace some custom code with a call to can_change_state().
>>
>> This subtly changes the error reporting behavior in some cases.
>>
>> Previously, if both RX and TX error counters indicated the same state,
>> and the status register agreed:
>> - if overall state is PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE
>> - if overall state is WARNING, report CAN_ERR_CRTL_TX_WARNING or
>>   CAN_ERR_CRTL_RX_WARNING depending on which counter is higher.
> If you look at sja1000.c, you'll see that it does this:
> tx_state = txerr >= rxerr ? state : 0
> rx_state = txerr <= rxerr ? state : 0;
> can_change_state(dev, cf, tx_state, rx_state);
>
> Maybe something similar could work for this?

That would work, though with that we don't get the
(CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING) error anymore when
both RX and TX are at warning level (and same for passive), but just one
of them. But probably not many users care (just guessing here, though)
so maybe it is worth the large simplification.

The current xilinx_can code does not produce such an error either, after
all, it just similarly picks one of RX/TX based on error counters.

I'll try your suggestion today or tomorrow.

>> Previously, if status registers indicated a different state than the
>> worst state indicated by the RX/TX error counters (I have not observed
>> this to happen, though):
>> - if status register says PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE if
>>   RX error counter indicates PASSIVE as well, otherwise report
>>   CAN_ERR_CRTL_TX_PASSIVE
>> - if status register says WARNING, report CAN_ERR_CRTL_TX_WARNING
>>   or CAN_ERR_CRTL_RX_WARNING depending on which counter is higher.
>>
>> After this commit, in both of the above scenarios:
>> - if overall state is PASSIVE, report
>>   CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE
>> - if overall state is WARNING, report
>>   CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING.
>>
>> Tested with the integrated CAN on Zynq-7000 SoC.
>>
> Anyway, the use of this API function is a somewhat superfluous issue, so I think
> we shouldn't worry too much about it.
>

-- 
Anssi Hannula / Bitwise Oy
+358503803997

[PATCH v2] can: xilinx_can: use can_change_state()

[Anssi Hannula]

Replace some custom code with a call to can_change_state().

This subtly changes the error reporting behavior when both RX and TX
error counters indicate the same state.

Previously, if both RX and TX counters indicated the same state:
- if overall state is PASSIVE, report CAN_ERR_CRTL_RX_PASSIVE
- if overall state is WARNING, report CAN_ERR_CRTL_TX_WARNING or
  CAN_ERR_CRTL_RX_WARNING depending on which counter is higher,
  or CAN_ERR_CRTL_RX_WARNING if the counters have the same value.

After this commit:
- report RX_* or TX_* depending on which counter is higher, or both if
  the counters have exactly the same value.

This behavior is consistent with many other CAN drivers that use this
same code pattern.

Tested with the integrated CAN on Zynq-7000 SoC.

v2: Simplify resolving states as suggested by Andri Yngvason.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---

Here's the simplified version of the patch, again on top of the previous
patchset.


 drivers/net/can/xilinx_can.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 26754e9..3348003 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -594,39 +594,19 @@ static void xcan_set_error_state(struct net_device *ndev,
 	u32 ecr = priv->read_reg(priv, XCAN_ECR_OFFSET);
 	u32 txerr = ecr & XCAN_ECR_TEC_MASK;
 	u32 rxerr = (ecr & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT;
+	enum can_state tx_state = txerr >= rxerr ? new_state : 0;
+	enum can_state rx_state = txerr <= rxerr ? new_state : 0;
 
-	priv->can.state = new_state;
+	/* non-ERROR states are handled elsewhere */
+	if (WARN_ON(new_state > CAN_STATE_ERROR_PASSIVE))
+		return;
+
+	can_change_state(ndev, cf, tx_state, rx_state);
 
 	if (cf) {
-		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
 	}
-
-	switch (new_state) {
-	case CAN_STATE_ERROR_PASSIVE:
-		priv->can.can_stats.error_passive++;
-		if (cf)
-			cf->data[1] = (rxerr > 127) ?
-					CAN_ERR_CRTL_RX_PASSIVE :
-					CAN_ERR_CRTL_TX_PASSIVE;
-		break;
-	case CAN_STATE_ERROR_WARNING:
-		priv->can.can_stats.error_warning++;
-		if (cf)
-			cf->data[1] |= (txerr > rxerr) ?
-					CAN_ERR_CRTL_TX_WARNING :
-					CAN_ERR_CRTL_RX_WARNING;
-		break;
-	case CAN_STATE_ERROR_ACTIVE:
-		if (cf)
-			cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
-		break;
-	default:
-		/* non-ERROR states are handled elsewhere */
-		WARN_ON(1);
-		break;
-	}
 }
 
 /**
-- 
2.8.3

Re: [PATCH 0/5 resend] can: various fixes to xilinx_can driver

[Marc Kleine-Budde]

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

On 01/26/2018 03:27 PM, Anssi Hannula wrote:
> Hi all,
> 
> I submitted a few bugfixes in Feb 2017 ("can: various fixes to
> xilinx_can driver") but they were not applied, so here they are again.
> 
> I seem to have missed CCing Michal Simek (the maintainer per
> MAINTAINERS) last time, but I've included him in the loop now so he can
> comment.
> 
> The last patch saw several revisions after comments from Marc, the one here
> is the same as the last version posted back then.
> 
> I've rebased them on top of linux-can/master and tested on Zynq-7000 HW.

I've taken patches 1-3 to linux-can. Patches 3, 4, [PATCH v2] can:
xilinx_can: use can_change_state(), 5 to linux-can-next.

Once net/master will be merged to net-next/master, patch 3 will fall
away and I'll send a pull reqeust upstream.

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

[PATCH 3/5] can: xilinx_can: fix recovery from error states not being propagated

[Anssi Hannula]

The xilinx_can driver contains no mechanism for propagating recovery
from CAN_STATE_ERROR_WARNING and CAN_STATE_ERROR_PASSIVE.

Add such a mechanism by factoring the handling of
XCAN_STATE_ERROR_PASSIVE and XCAN_STATE_ERROR_WARNING out of
xcan_err_interrupt and checking for recovery after RX and TX if the
interface is in one of those states.

Tested with the integrated CAN on Zynq-7000 SoC.

Fixes: b1201e44f50b ("can: xilinx CAN controller support")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/xilinx_can.c | 155 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 127 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index b23bbfb..e02fabf 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -2,6 +2,7 @@
  *
  * Copyright (C) 2012 - 2014 Xilinx, Inc.
  * Copyright (C) 2009 PetaLogix. All rights reserved.
+ * Copyright (C) 2017 Sandvik Mining and Construction Oy
  *
  * Description:
  * This driver is developed for Axi CAN IP and for Zynq CANPS Controller.
@@ -530,6 +531,123 @@ static int xcan_rx(struct net_device *ndev)
 }
 
 /**
+ * xcan_current_error_state - Get current error state from HW
+ * @ndev:	Pointer to net_device structure
+ *
+ * Checks the current CAN error state from the HW. Note that this
+ * only checks for ERROR_PASSIVE and ERROR_WARNING.
+ *
+ * Return:
+ * ERROR_PASSIVE or ERROR_WARNING if either is active, ERROR_ACTIVE
+ * otherwise.
+ */
+static enum can_state xcan_current_error_state(struct net_device *ndev)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	u32 status = priv->read_reg(priv, XCAN_SR_OFFSET);
+
+	if ((status & XCAN_SR_ESTAT_MASK) == XCAN_SR_ESTAT_MASK)
+		return CAN_STATE_ERROR_PASSIVE;
+	else if (status & XCAN_SR_ERRWRN_MASK)
+		return CAN_STATE_ERROR_WARNING;
+	else
+		return CAN_STATE_ERROR_ACTIVE;
+}
+
+/**
+ * xcan_set_error_state - Set new CAN error state
+ * @ndev:	Pointer to net_device structure
+ * @new_state:	The new CAN state to be set
+ * @cf:		Error frame to be populated or NULL
+ *
+ * Set new CAN error state for the device, updating statistics and
+ * populating the error frame if given.
+ */
+static void xcan_set_error_state(struct net_device *ndev,
+				 enum can_state new_state,
+				 struct can_frame *cf)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	u32 ecr = priv->read_reg(priv, XCAN_ECR_OFFSET);
+	u32 txerr = ecr & XCAN_ECR_TEC_MASK;
+	u32 rxerr = (ecr & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT;
+
+	priv->can.state = new_state;
+
+	if (cf) {
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[6] = txerr;
+		cf->data[7] = rxerr;
+	}
+
+	switch (new_state) {
+	case CAN_STATE_ERROR_PASSIVE:
+		priv->can.can_stats.error_passive++;
+		if (cf)
+			cf->data[1] = (rxerr > 127) ?
+					CAN_ERR_CRTL_RX_PASSIVE :
+					CAN_ERR_CRTL_TX_PASSIVE;
+		break;
+	case CAN_STATE_ERROR_WARNING:
+		priv->can.can_stats.error_warning++;
+		if (cf)
+			cf->data[1] |= (txerr > rxerr) ?
+					CAN_ERR_CRTL_TX_WARNING :
+					CAN_ERR_CRTL_RX_WARNING;
+		break;
+	case CAN_STATE_ERROR_ACTIVE:
+		if (cf)
+			cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
+		break;
+	default:
+		/* non-ERROR states are handled elsewhere */
+		WARN_ON(1);
+		break;
+	}
+}
+
+/**
+ * xcan_update_error_state_after_rxtx - Update CAN error state after RX/TX
+ * @ndev:	Pointer to net_device structure
+ *
+ * If the device is in a ERROR-WARNING or ERROR-PASSIVE state, check if
+ * the performed RX/TX has caused it to drop to a lesser state and set
+ * the interface state accordingly.
+ */
+static void xcan_update_error_state_after_rxtx(struct net_device *ndev)
+{
+	struct xcan_priv *priv = netdev_priv(ndev);
+	enum can_state old_state = priv->can.state;
+	enum can_state new_state;
+
+	/* changing error state due to successful frame RX/TX can only
+	 * occur from these states
+	 */
+	if (old_state != CAN_STATE_ERROR_WARNING &&
+	    old_state != CAN_STATE_ERROR_PASSIVE)
+		return;
+
+	new_state = xcan_current_error_state(ndev);
+
+	if (new_state != old_state) {
+		struct sk_buff *skb;
+		struct can_frame *cf;
+
+		skb = alloc_can_err_skb(ndev, &cf);
+
+		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
+
+		if (skb) {
+			struct net_device_stats *stats = &ndev->stats;
+
+			stats->rx_packets++;
+			stats->rx_bytes += cf->can_dlc;
+			netif_rx(skb);
+		}
+	}
+}
+
+/**
  * xcan_err_interrupt - error frame Isr
  * @ndev:	net_device pointer
  * @isr:	interrupt status register value
@@ -544,16 +662,12 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 	struct net_device_stats *stats = &ndev->stats;
 	struct can_frame *cf;
 	struct sk_buff *skb;
-	u32 err_status, status, txerr = 0, rxerr = 0;
+	u32 err_status;
 
 	skb = alloc_can_err_skb(ndev, &cf);
 
 	err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
 	priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
-	txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
-	rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
-			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
-	status = priv->read_reg(priv, XCAN_SR_OFFSET);
 
 	if (isr & XCAN_IXR_BSOFF_MASK) {
 		priv->can.state = CAN_STATE_BUS_OFF;
@@ -563,28 +677,10 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 		can_bus_off(ndev);
 		if (skb)
 			cf->can_id |= CAN_ERR_BUSOFF;
-	} else if ((status & XCAN_SR_ESTAT_MASK) == XCAN_SR_ESTAT_MASK) {
-		priv->can.state = CAN_STATE_ERROR_PASSIVE;
-		priv->can.can_stats.error_passive++;
-		if (skb) {
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] = (rxerr > 127) ?
-					CAN_ERR_CRTL_RX_PASSIVE :
-					CAN_ERR_CRTL_TX_PASSIVE;
-			cf->data[6] = txerr;
-			cf->data[7] = rxerr;
-		}
-	} else if (status & XCAN_SR_ERRWRN_MASK) {
-		priv->can.state = CAN_STATE_ERROR_WARNING;
-		priv->can.can_stats.error_warning++;
-		if (skb) {
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] |= (txerr > rxerr) ?
-					CAN_ERR_CRTL_TX_WARNING :
-					CAN_ERR_CRTL_RX_WARNING;
-			cf->data[6] = txerr;
-			cf->data[7] = rxerr;
-		}
+	} else {
+		enum can_state new_state = xcan_current_error_state(ndev);
+
+		xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
 	}
 
 	/* Check for Arbitration lost interrupt */
@@ -713,8 +809,10 @@ static int xcan_rx_poll(struct napi_struct *napi, int quota)
 		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	}
 
-	if (work_done)
+	if (work_done) {
 		can_led_event(ndev, CAN_LED_EVENT_RX);
+		xcan_update_error_state_after_rxtx(ndev);
+	}
 
 	if (work_done < quota) {
 		napi_complete(napi);
@@ -745,6 +843,7 @@ static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
 		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
 	}
 	can_led_event(ndev, CAN_LED_EVENT_TX);
+	xcan_update_error_state_after_rxtx(ndev);
 	netif_wake_queue(ndev);
 }
 
-- 
2.8.3