Skip to contentSkip to conversation

can: c_can: Another pile of fixes and improvements

PATCH In review Thomas Gleixner 3 participants 26 messages Apr 11, 2014 – Apr 14, 2014 CAN network drivers

lore
Share & tools
lore

Thomas Gleixner

author
#0 lore raw

v2 cover can: c_can: Another pile of fixes and improvements

Changes since V1:

- Slightly modified version of the interrupt reduction patch
- Included the fix for PCH / C_CAN
- Lockless XMIT path
- Further reduction of register access
- Add the missing can.type setup in c_can_pci.c
- A pile of code cleanups.

It would be nice to reduce the register access some more by relying completely on the status interrupt, but it turned out that the TX/RXOK is not reliable enough. So we need to invalidate the message objects in the tx softirq handling.

But the overall change of this series is that the I/O load gets reduced by about 45% according to perf top. Though that PCH thing sucks. The beaglebone manages to almost saturate the bus with short packets at 1Mbit while PCH fails miserably and thats solely related to the miserable I/O performance.

time cangen can0 -g0 -p10 -I5A5 -L0 -x -n 1000000

arm: real 0m51.510s I/O read: ~6% I/O write: 1.5% ~3.5s
x86: real 1m48.533s I/O read: ~29% I/O write: 0.8% ~32 s!!

That's both with HW loopback on, as my PCH does not have a tranceiver. Granted the C_CAN in the PCH needs the double IF transfer to prevent the message loss versus the D_CAN in the ARM chip, but even that taken into account makes a whopping 16s per 1M messages vs. 3.5s on ARM.

w/o loopback the arm I/O read load drops to ~3.5% on the sender side and ~5.5% on the receiver side. The time drops to 50.5s on the transmit side if we do not have to get all the RX packets from HW loopback. On TX we have a ~10us large gap every 16 packets which is caused by the queue stall as we have to wait for the last packet in the "FIFO" to be transferred.

It seems there is a reason why the ATOM perf events do not expose the stalled cpu cycles. But it's easy to figure out. You can compare the CAN load case with some other scenario which has 100% CPU utilization by running

# perf stat -a sleep 60

The interesting part is: insns per cycle

CAN: 0.23 insns per cycle
Other: 0.53 insns per cycle

I don't have comparison numbers for ARM due to not supported perf events, but the perf top numbers and the transfer performance tell a clear story.

There might be room for a few improvements, but I'm running out of cycles and I really want to get the IF3 DMA feature functional on the TI chips, but that seems to be an equally tedious reverse engineering problem as the rest of this.

Thanks,

tglx

---------

 Kconfig     |    7 
 c_can.c     |  662 +++++++++++++++++++++++++++---------------------------------
 c_can.h     |   21 -
 c_can_pci.c |    2 
 4 files changed, 320 insertions(+), 372 deletions(-)
author
#1 lore raw

PATCH v2 1/21 can: c_can_pci: Set the type of the IP core

All type checks in c_can.c are != BOSCH_D_CAN so nobody noticed so far that the pci code does not update the type information.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can_pci.c |    2 ++
 1 file changed, 2 insertions(+)
drivers/net/can/c_can/c_can_pci.c +2 -0
Index: linux-2.6/drivers/net/can/c_can/c_can_pci.c
===================================================================
@@ -132,6 +132,8 @@ static int c_can_pci_probe(struct pci_de
 		goto out_free_c_can;
 	}
 
+	priv->type = c_can_pci_data->type;
+
 	/* Configure access to registers */
 	switch (c_can_pci_data->reg_align) {
 	case C_CAN_REG_ALIGN_32:
author
#2 lore raw

PATCH v2 2/21 can: c_can: Fix startup logic

In reply to Thomas Gleixner

c_can_start() enables interrupts way too early. The first enabling happens when setting the control mode in c_can_chip_config() and then again at the end of the function.

But that happens before napi_enable() and that means that an interrupt which comes in will disable interrupts again and call napi_schedule, which ignores the request and the later napi_enable() is not making thinks work either. So the interface is up with all device interrupts disabled.

Move the device interrupt after napi_enable() and add it to the other callsites of c_can_start() in c_can_set_mode() and c_can_power_up()

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)
drivers/net/can/c_can/c_can.c +17 -18
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -612,30 +612,22 @@ static int c_can_chip_config(struct net_
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	/* enable automatic retransmission */
-	priv->write_reg(priv, C_CAN_CTRL_REG,
-			CONTROL_ENABLE_AR);
+	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR);
 
 	if ((priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) &&
 	    (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)) {
 		/* loopback + silent mode : useful for hot self-test */
-		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
-				CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
-		priv->write_reg(priv, C_CAN_TEST_REG,
-				TEST_LBACK | TEST_SILENT);
+		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST);
+		priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK | TEST_SILENT);
 	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
 		/* loopback mode : useful for self-test function */
-		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
-				CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
+		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST);
 		priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK);
 	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
 		/* silent mode : bus-monitoring mode */
-		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
-				CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
+		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST);
 		priv->write_reg(priv, C_CAN_TEST_REG, TEST_SILENT);
-	} else
-		/* normal mode*/
-		priv->write_reg(priv, C_CAN_CTRL_REG,
-				CONTROL_EIE | CONTROL_SIE | CONTROL_IE);
+	}
 
 	/* configure message objects */
 	c_can_configure_msg_objects(dev);

@@ -662,9 +654,6 @@ static int c_can_start(struct net_device
 	/* reset tx helper pointers */
 	priv->tx_next = priv->tx_echo = 0;
 
-	/* enable status change, error and module interrupts */
-	c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
-
 	return 0;
 }
 

@@ -681,6 +670,7 @@ static void c_can_stop(struct net_device
 
 static int c_can_set_mode(struct net_device *dev, enum can_mode mode)
 {
+	struct c_can_priv *priv = netdev_priv(dev);
 	int err;
 
 	switch (mode) {

@@ -689,6 +679,8 @@ static int c_can_set_mode(struct net_dev
 		if (err)
 			return err;
 		netif_wake_queue(dev);
+		/* enable status change, error and module interrupts */
+		c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
 		break;
 	default:
 		return -EOPNOTSUPP;

@@ -1184,6 +1176,8 @@ static int c_can_open(struct net_device
 	can_led_event(dev, CAN_LED_EVENT_OPEN);
 
 	napi_enable(&priv->napi);
+	/* enable status change, error and module interrupts */
+	c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
 	netif_start_queue(dev);
 
 	return 0;

@@ -1281,6 +1275,7 @@ int c_can_power_up(struct net_device *de
 	u32 val;
 	unsigned long time_out;
 	struct c_can_priv *priv = netdev_priv(dev);
+	int ret;
 
 	if (!(dev->flags & IFF_UP))
 		return 0;

@@ -1307,7 +1302,11 @@ int c_can_power_up(struct net_device *de
 	if (time_after(jiffies, time_out))
 		return -ETIMEDOUT;
 
-	return c_can_start(dev);
+	ret = c_can_start(dev);
+	if (!ret)
+		c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(c_can_power_up);
 #endif
author
#3 lore raw

PATCH v2 3/21 can: c_can: Make bus off interrupt disable logic work

In reply to Thomas Gleixner

The state change handler is called with device interrupts disabled already. So no point in disabling them again when we enter bus off state.

But what's worse is that we reenable the interrupts at the end of NAPI poll unconditionally. So c_can_start() which is called from the restart timer can trigger interrupts which confuse the hell out of the half reinitialized driver/hw.

Remove the pointless device interrupt disable in the BUS_OFF handler and prevent reenabling the device interrupts at the end of the poll routine when the current state is BUS_OFF.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/net/can/c_can/c_can.c +4 -7
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -954,11 +954,6 @@ static int c_can_handle_state_change(str
 		/* bus-off state */
 		priv->can.state = CAN_STATE_BUS_OFF;
 		cf->can_id |= CAN_ERR_BUSOFF;
-		/*
-		 * disable all interrupts in bus-off mode to ensure that
-		 * the CPU is not hogged down
-		 */
-		c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
 		can_bus_off(dev);
 		break;
 	default:

@@ -1089,6 +1084,7 @@ static int c_can_poll(struct napi_struct
 			netdev_dbg(dev, "entered bus off state\n");
 			work_done += c_can_handle_state_change(dev,
 						C_CAN_BUS_OFF);
+			goto end;
 		}
 
 		/* handle bus recovery events */

@@ -1122,8 +1118,9 @@ static int c_can_poll(struct napi_struct
 end:
 	if (work_done < quota) {
 		napi_complete(napi);
-		/* enable all IRQs */
-		c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
+		/* enable all IRQs if we are not in bus off state */
+		if (priv->can.state != CAN_STATE_BUS_OFF)
+			c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
 	}
 
 	return work_done;
author
#4 lore raw

PATCH v2 4/21 can: c_can: Do not access skb after net_receive_skb()

In reply to Thomas Gleixner

There is no guarantee that the skb is in the same state after calling net_receive_skb(). It might be freed or reused. Not really harmful as its a read access, except you turn on the proper debugging options which catch a use after free.

The whole can subsystem is full of this. Copy and paste ....

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/net/can/c_can/c_can.c +4 -5
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -429,10 +429,10 @@ static int c_can_read_msg_object(struct
 		}
 	}
 
-	netif_receive_skb(skb);
-
 	stats->rx_packets++;
 	stats->rx_bytes += frame->can_dlc;
+
+	netif_receive_skb(skb);
 	return 0;
 }
 

@@ -960,9 +960,9 @@ static int c_can_handle_state_change(str
 		break;
 	}
 
-	netif_receive_skb(skb);
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
+	netif_receive_skb(skb);
 
 	return 1;
 }

@@ -1033,10 +1033,9 @@ static int c_can_handle_bus_err(struct n
 	/* set a `lec` value so that we can check for updates later */
 	priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
 
-	netif_receive_skb(skb);
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
-
+	netif_receive_skb(skb);
 	return 1;
 }
author
#5 lore raw

PATCH v2 5/21 can: c_can: Handle state change correctly

In reply to Thomas Gleixner

If the allocation of an error skb fails, the state change handling returns w/o doing any work. That leaves the interface in a wreckaged state as the internal status is wrong.

Split the interface handling and the skb handling.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)
drivers/net/can/c_can/c_can.c +20 -5
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -914,6 +914,26 @@ static int c_can_handle_state_change(str
 	struct sk_buff *skb;
 	struct can_berr_counter bec;
 
+	switch (error_type) {
+	case C_CAN_ERROR_WARNING:
+		/* error warning state */
+		priv->can.can_stats.error_warning++;
+		priv->can.state = CAN_STATE_ERROR_WARNING;
+		break;
+	case C_CAN_ERROR_PASSIVE:
+		/* error passive state */
+		priv->can.can_stats.error_passive++;
+		priv->can.state = CAN_STATE_ERROR_PASSIVE;
+		break;
+	case C_CAN_BUS_OFF:
+		/* bus-off state */
+		priv->can.state = CAN_STATE_BUS_OFF;
+		can_bus_off(dev);
+		break;
+	default:
+		break;
+	}
+
 	/* propagate the error condition to the CAN stack */
 	skb = alloc_can_err_skb(dev, &cf);
 	if (unlikely(!skb))

@@ -927,8 +947,6 @@ static int c_can_handle_state_change(str
 	switch (error_type) {
 	case C_CAN_ERROR_WARNING:
 		/* error warning state */
-		priv->can.can_stats.error_warning++;
-		priv->can.state = CAN_STATE_ERROR_WARNING;
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[1] = (bec.txerr > bec.rxerr) ?
 			CAN_ERR_CRTL_TX_WARNING :

@@ -939,8 +957,6 @@ static int c_can_handle_state_change(str
 		break;
 	case C_CAN_ERROR_PASSIVE:
 		/* error passive state */
-		priv->can.can_stats.error_passive++;
-		priv->can.state = CAN_STATE_ERROR_PASSIVE;
 		cf->can_id |= CAN_ERR_CRTL;
 		if (rx_err_passive)
 			cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;

@@ -952,7 +968,6 @@ static int c_can_handle_state_change(str
 		break;
 	case C_CAN_BUS_OFF:
 		/* bus-off state */
-		priv->can.state = CAN_STATE_BUS_OFF;
 		cf->can_id |= CAN_ERR_BUSOFF;
 		can_bus_off(dev);
 		break;
author
#6 lore raw

PATCH v2 6/21 can: c_can: Fix berr reporting

In reply to Thomas Gleixner

Reading the LEC type with

  return (mode & ENABLED) && (status & LEC_MASK);

is not guaranteed to return (status & LEC_MASK) if the enabled bit in mode is set. It's guaranteed to return 0 or !=0.

Remove the inline function and call unconditionally into the berr_handling code and return early when the reporting is disabled.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/net/can/c_can/c_can.c +6 -10
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -171,6 +171,7 @@ enum c_can_lec_type {
 	LEC_BIT0_ERROR,
 	LEC_CRC_ERROR,
 	LEC_UNUSED,
+	LEC_MASK = LEC_UNUSED,
 };
 
 /*

@@ -897,12 +898,6 @@ static int c_can_do_rx_poll(struct net_d
 	return pkts;
 }
 
-static inline int c_can_has_and_handle_berr(struct c_can_priv *priv)
-{
-	return (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
-		(priv->current_status & LEC_UNUSED);
-}
-
 static int c_can_handle_state_change(struct net_device *dev,
 				enum c_can_bus_error_types error_type)
 {

@@ -998,6 +993,9 @@ static int c_can_handle_bus_err(struct n
 	if (lec_type == LEC_UNUSED || lec_type == LEC_NO_ERROR)
 		return 0;
 
+	if (!(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
+		return 0;
+
 	/* propagate the error condition to the CAN stack */
 	skb = alloc_can_err_skb(dev, &cf);
 	if (unlikely(!skb))

@@ -1057,7 +1055,6 @@ static int c_can_handle_bus_err(struct n
 static int c_can_poll(struct napi_struct *napi, int quota)
 {
 	u16 irqstatus;
-	int lec_type = 0;
 	int work_done = 0;
 	struct net_device *dev = napi->dev;
 	struct c_can_priv *priv = netdev_priv(dev);

@@ -1116,9 +1113,8 @@ static int c_can_poll(struct napi_struct
 		priv->last_status = priv->current_status;
 
 		/* handle lec errors on the bus */
-		lec_type = c_can_has_and_handle_berr(priv);
-		if (lec_type)
-			work_done += c_can_handle_bus_err(dev, lec_type);
+		work_done += c_can_handle_bus_err(dev,
+					priv->current_status & LEC_MASK);
 	} else if ((irqstatus >= C_CAN_MSG_OBJ_RX_FIRST) &&
 			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST)) {
 		/* handle events corresponding to receive message objects */
author
#7 lore raw

PATCH v2 7/21 can: c_can: Always update error stats

In reply to Thomas Gleixner

If the allocation of the error skb fails, we still want to see the error statistics.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/net/can/c_can/c_can.c +7 -6
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -378,6 +378,9 @@ static int c_can_handle_lost_msg_obj(str
 	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
 	c_can_object_put(dev, iface, objno, IF_COMM_CONTROL);
 
+	stats->rx_errors++;
+	stats->rx_over_errors++;
+
 	/* create an error msg */
 	skb = alloc_can_err_skb(dev, &frame);
 	if (unlikely(!skb))

@@ -385,8 +388,6 @@ static int c_can_handle_lost_msg_obj(str
 
 	frame->can_id |= CAN_ERR_CRTL;
 	frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
-	stats->rx_errors++;
-	stats->rx_over_errors++;
 
 	netif_receive_skb(skb);
 	return 1;

@@ -996,6 +997,10 @@ static int c_can_handle_bus_err(struct n
 	if (!(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
 		return 0;
 
+	/* common for all type of bus errors */
+	priv->can.can_stats.bus_error++;
+	stats->rx_errors++;
+
 	/* propagate the error condition to the CAN stack */
 	skb = alloc_can_err_skb(dev, &cf);
 	if (unlikely(!skb))

@@ -1005,10 +1010,6 @@ static int c_can_handle_bus_err(struct n
 	 * check for 'last error code' which tells us the
 	 * type of the last error to occur on the CAN bus
 	 */
-
-	/* common for all type of bus errors */
-	priv->can.can_stats.bus_error++;
-	stats->rx_errors++;
 	cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 	cf->data[2] |= CAN_ERR_PROT_UNSPEC;
author
#8 lore raw

PATCH v2 8/21 can: c_can: Simplify buffer reenabling

In reply to Thomas Gleixner

Instead of writing to the message object we can simply clear the NewDat bit with the get method.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/net/can/c_can/c_can.c +6 -10
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -108,6 +108,7 @@
 #define IF_COMM_CONTROL		BIT(4)
 #define IF_COMM_CLR_INT_PND	BIT(3)
 #define IF_COMM_TXRQST		BIT(2)
+#define IF_COMM_CLR_NEWDAT	IF_COMM_TXRQST
 #define IF_COMM_DATAA		BIT(1)
 #define IF_COMM_DATAB		BIT(0)
 #define IF_COMM_ALL		(IF_COMM_MASK | IF_COMM_ARB | \

@@ -120,7 +121,7 @@
 				 IF_COMM_DATAA | IF_COMM_DATAB)
 
 /* For the high buffers we clear the interrupt bit and newdat */
-#define IF_COMM_RCV_HIGH	(IF_COMM_RCV_LOW | IF_COMM_TXRQST)
+#define IF_COMM_RCV_HIGH	(IF_COMM_RCV_LOW | IF_COMM_CLR_NEWDAT)
 
 /* IFx arbitration */
 #define IF_ARB_MSGVAL		BIT(15)

@@ -353,17 +354,12 @@ static void c_can_write_msg_object(struc
 }
 
 static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
-						int iface,
-						int ctrl_mask)
+						       int iface)
 {
 	int i;
-	struct c_can_priv *priv = netdev_priv(dev);
 
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_RX_LOW_LAST; i++) {
-		priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
-				ctrl_mask & ~IF_MCONT_NEWDAT);
-		c_can_object_put(dev, iface, i, IF_COMM_CONTROL);
-	}
+	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_RX_LOW_LAST; i++)
+		c_can_object_get(dev, iface, i, IF_COMM_CLR_NEWDAT);
 }
 
 static int c_can_handle_lost_msg_obj(struct net_device *dev,

@@ -829,7 +825,7 @@ static int c_can_read_objects(struct net
 
 		if (obj == C_CAN_MSG_RX_LOW_LAST)
 			/* activate all lower message objects */
-			c_can_activate_all_lower_rx_msg_obj(dev, IF_RX, ctrl);
+			c_can_activate_all_lower_rx_msg_obj(dev, IF_RX);
 
 		pkts++;
 		quota--;
author
#9 lore raw

PATCH v2 9/21 can: c_can: Avoid status register update for D_CAN

In reply to Thomas Gleixner

On D_CAN the RXOK, TXOK and LEC bits are cleared/set on read of the status register. No need to update them.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/net/can/c_can/c_can.c +6 -3
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -1041,7 +1041,8 @@ static int c_can_handle_bus_err(struct n
 	}
 
 	/* set a `lec` value so that we can check for updates later */
-	priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
+	if (priv->type != BOSCH_D_CAN)
+		priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
 
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;

@@ -1066,11 +1067,13 @@ static int c_can_poll(struct napi_struct
 					C_CAN_STS_REG);
 
 		/* handle Tx/Rx events */
-		if (priv->current_status & STATUS_TXOK)
+		if (priv->current_status & STATUS_TXOK &&
+		    priv->type != BOSCH_D_CAN)
 			priv->write_reg(priv, C_CAN_STS_REG,
 					priv->current_status & ~STATUS_TXOK);
 
-		if (priv->current_status & STATUS_RXOK)
+		if (priv->current_status & STATUS_RXOK &&
+		    priv->type != BOSCH_D_CAN)
 			priv->write_reg(priv, C_CAN_STS_REG,
 					priv->current_status & ~STATUS_RXOK);
author
#10 lore raw

PATCH v2 10/21 can: c_can: Get rid of pointless interrupts

In reply to Thomas Gleixner

The driver handles pointlessly TWO interrupts per packet. The reason is that it enables the status interrupt which fires for each rx and tx packet and it enables the per message object interrupts as well.

The status interrupt merily acks or in case of D_CAN ignores the TX/RX state and then the message object interrupt fires.

The message objects interrupts are only useful if all message objects have hardware filters activated.

But we don't have that and its not simple to implement in that driver without rewriting it completely.

So we can ditch the message object interrupts and handle the RX/TX right away from the status interrupt. Instead of TWO we handle ONE.

Note: We must keep the TXIE/RXIE bits in the message buffers because the status interrupt alone is not reliable enough in corner cases.

If we ever have the need for HW filtering, then this code needs a complete overhaul and we can think about it then. For now we prefer a lower interrupt load.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |  122 ++++++++++++++++--------------------------
 drivers/net/can/c_can/c_can.h |    3 -
 2 files changed, 48 insertions(+), 77 deletions(-)
drivers/net/can/c_can/c_can.c +47 -75
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -593,7 +593,7 @@ static void c_can_configure_msg_objects(
 	/* setup receive message objects */
 	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
 		c_can_setup_receive_object(dev, IF_RX, i, 0, 0,
-			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
+					   IF_MCONT_RXIE | IF_MCONT_UMASK);
 
 	c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
 			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);

@@ -649,8 +649,9 @@ static int c_can_start(struct net_device
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	/* reset tx helper pointers */
+	/* reset tx helper pointers and the rx mask */
 	priv->tx_next = priv->tx_echo = 0;
+	priv->rxmasked = 0;
 
 	return 0;
 }

@@ -823,9 +824,13 @@ static int c_can_read_objects(struct net
 		/* read the data from the message object */
 		c_can_read_msg_object(dev, IF_RX, ctrl);
 
-		if (obj == C_CAN_MSG_RX_LOW_LAST)
+		if (obj < C_CAN_MSG_RX_LOW_LAST)
+			priv->rxmasked |= BIT(obj - 1);
+		else if (obj == C_CAN_MSG_RX_LOW_LAST) {
+			priv->rxmasked = 0;
 			/* activate all lower message objects */
 			c_can_activate_all_lower_rx_msg_obj(dev, IF_RX);
+		}
 
 		pkts++;
 		quota--;

@@ -870,7 +875,8 @@ static int c_can_do_rx_poll(struct net_d
 
 	while (quota > 0) {
 		if (!pend) {
-			pend = priv->read_reg(priv, C_CAN_INTPND1_REG);
+			pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
+			pend &= ~priv->rxmasked;
 			if (!pend)
 				break;
 			/*

@@ -1040,10 +1046,6 @@ static int c_can_handle_bus_err(struct n
 		break;
 	}
 
-	/* set a `lec` value so that we can check for updates later */
-	if (priv->type != BOSCH_D_CAN)
-		priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
-
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
 	netif_receive_skb(skb);

@@ -1052,79 +1054,50 @@ static int c_can_handle_bus_err(struct n
 
 static int c_can_poll(struct napi_struct *napi, int quota)
 {
-	u16 irqstatus;
-	int work_done = 0;
 	struct net_device *dev = napi->dev;
 	struct c_can_priv *priv = netdev_priv(dev);
+	u16 curr, last = priv->last_status;
+	int work_done = 0;
 
-	irqstatus = priv->irqstatus;
-	if (!irqstatus)
-		goto end;
-
-	/* status events have the highest priority */
-	if (irqstatus == STATUS_INTERRUPT) {
-		priv->current_status = priv->read_reg(priv,
-					C_CAN_STS_REG);
-
-		/* handle Tx/Rx events */
-		if (priv->current_status & STATUS_TXOK &&
-		    priv->type != BOSCH_D_CAN)
-			priv->write_reg(priv, C_CAN_STS_REG,
-					priv->current_status & ~STATUS_TXOK);
-
-		if (priv->current_status & STATUS_RXOK &&
-		    priv->type != BOSCH_D_CAN)
-			priv->write_reg(priv, C_CAN_STS_REG,
-					priv->current_status & ~STATUS_RXOK);
-
-		/* handle state changes */
-		if ((priv->current_status & STATUS_EWARN) &&
-				(!(priv->last_status & STATUS_EWARN))) {
-			netdev_dbg(dev, "entered error warning state\n");
-			work_done += c_can_handle_state_change(dev,
-						C_CAN_ERROR_WARNING);
-		}
-		if ((priv->current_status & STATUS_EPASS) &&
-				(!(priv->last_status & STATUS_EPASS))) {
-			netdev_dbg(dev, "entered error passive state\n");
-			work_done += c_can_handle_state_change(dev,
-						C_CAN_ERROR_PASSIVE);
-		}
-		if ((priv->current_status & STATUS_BOFF) &&
-				(!(priv->last_status & STATUS_BOFF))) {
-			netdev_dbg(dev, "entered bus off state\n");
-			work_done += c_can_handle_state_change(dev,
-						C_CAN_BUS_OFF);
-			goto end;
-		}
+	priv->last_status = curr = priv->read_reg(priv, C_CAN_STS_REG);
+	/* Ack status on C_CAN. D_CAN is self clearing */
+	if (priv->type != BOSCH_D_CAN)
+		priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
 
-		/* handle bus recovery events */
-		if ((!(priv->current_status & STATUS_BOFF)) &&
-				(priv->last_status & STATUS_BOFF)) {
-			netdev_dbg(dev, "left bus off state\n");
-			priv->can.state = CAN_STATE_ERROR_ACTIVE;
-		}
-		if ((!(priv->current_status & STATUS_EPASS)) &&
-				(priv->last_status & STATUS_EPASS)) {
-			netdev_dbg(dev, "left error passive state\n");
-			priv->can.state = CAN_STATE_ERROR_ACTIVE;
-		}
+	/* handle state changes */
+	if ((curr & STATUS_EWARN) && (!(last & STATUS_EWARN))) {
+		netdev_dbg(dev, "entered error warning state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_ERROR_WARNING);
+	}
 
-		priv->last_status = priv->current_status;
+	if ((curr & STATUS_EPASS) && (!(last & STATUS_EPASS))) {
+		netdev_dbg(dev, "entered error passive state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_ERROR_PASSIVE);
+	}
 
-		/* handle lec errors on the bus */
-		work_done += c_can_handle_bus_err(dev,
-					priv->current_status & LEC_MASK);
-	} else if ((irqstatus >= C_CAN_MSG_OBJ_RX_FIRST) &&
-			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST)) {
-		/* handle events corresponding to receive message objects */
-		work_done += c_can_do_rx_poll(dev, (quota - work_done));
-	} else if ((irqstatus >= C_CAN_MSG_OBJ_TX_FIRST) &&
-			(irqstatus <= C_CAN_MSG_OBJ_TX_LAST)) {
-		/* handle events corresponding to transmit message objects */
-		c_can_do_tx(dev);
+	if ((curr & STATUS_BOFF) && (!(last & STATUS_BOFF))) {
+		netdev_dbg(dev, "entered bus off state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_BUS_OFF);
+		goto end;
 	}
 
+	/* handle bus recovery events */
+	if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
+		netdev_dbg(dev, "left bus off state\n");
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	}
+	if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
+		netdev_dbg(dev, "left error passive state\n");
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	}
+
+	/* handle lec errors on the bus */
+	work_done += c_can_handle_bus_err(dev, curr & LEC_MASK);
+
+	/* Handle Tx/Rx events. We do this unconditionally */
+	work_done += c_can_do_rx_poll(dev, (quota - work_done));
+	c_can_do_tx(dev);
+
 end:
 	if (work_done < quota) {
 		napi_complete(napi);

@@ -1141,8 +1114,7 @@ static irqreturn_t c_can_isr(int irq, vo
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct c_can_priv *priv = netdev_priv(dev);
 
-	priv->irqstatus = priv->read_reg(priv, C_CAN_INT_REG);
-	if (!priv->irqstatus)
+	if (!priv->read_reg(priv, C_CAN_INT_REG))
 		return IRQ_NONE;
 
 	/* disable all interrupts and schedule the NAPI */
drivers/net/can/c_can/c_can.h +1 -2
Index: linux-2.6/drivers/net/can/c_can/c_can.h
===================================================================
@@ -185,7 +185,6 @@ struct c_can_priv {
 	struct device *device;
 	spinlock_t xmit_lock;
 	int tx_object;
-	int current_status;
 	int last_status;
 	u16 (*read_reg) (struct c_can_priv *priv, enum reg index);
 	void (*write_reg) (struct c_can_priv *priv, enum reg index, u16 val);

@@ -195,11 +194,11 @@ struct c_can_priv {
 	unsigned int tx_next;
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
-	u16 irqstatus;
 	enum c_can_dev_id type;
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
 };
author
#11 lore raw

PATCH v2 11/21 can: c_can : Disable rx split as workaround

In reply to Thomas Gleixner

The RX buffer split causes packet loss in the hardware:

What happens is:

RX Packet 1 --> message buffer 1 (newdat bit is not cleared) RX Packet 2 --> message buffer 2 (newdat bit is not cleared) RX Packet 3 --> message buffer 3 (newdat bit is not cleared) RX Packet 4 --> message buffer 4 (newdat bit is not cleared) RX Packet 5 --> message buffer 5 (newdat bit is not cleared) RX Packet 6 --> message buffer 6 (newdat bit is not cleared) RX Packet 7 --> message buffer 7 (newdat bit is not cleared) RX Packet 8 --> message buffer 8 (newdat bit is not cleared)

Clear newdat bit in message buffer 1
Clear newdat bit in message buffer 2
Clear newdat bit in message buffer 3
Clear newdat bit in message buffer 4
Clear newdat bit in message buffer 5
Clear newdat bit in message buffer 6
Clear newdat bit in message buffer 7
Clear newdat bit in message buffer 8

Now if during that clearing of newdat bits, a new message comes in, the HW gets confused and drops it.

It does not matter how many of them you clear. I put a delay between clear of buffer 1 and buffer 2 which was long enough that the message should have been queued either in buffer 1 or buffer 9. But it did not show up anywhere. The next message ended up in buffer 1. So the hardware lost a packet of course without telling it via one of the error handlers.

That does not happen on all clear newdat bit events. I see one of 10k packets dropped in the scenario which allows us to reproduce. But the trace looks always the same.

Not splitting the RX Buffer avoids the packet loss but can cause reordering. It's hard to trigger, but it CAN happen.

With that mode we use the HW as it was probably designed for. We read from the buffer 1 upwards and clear the buffer as we get the message. That's how all microcontrollers use it. So I assume that the way we handle the buffers was never really tested. According to the public documentation it should just work :)

Let the user decide which evil is the lesser one.

[ Oliver Hartkopp: Provided a sane config option and help text and
made me switch to favour potential and unlikely reordering over packet loss ]

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/Kconfig |    7 ++++
 drivers/net/can/c_can/c_can.c |   62 ++++++++++++++++++++++++++++++++----------
 2 files changed, 55 insertions(+), 14 deletions(-)
drivers/net/can/c_can/Kconfig +7 -0
Index: linux-2.6/drivers/net/can/c_can/Kconfig
===================================================================
@@ -14,6 +14,13 @@ config CAN_C_CAN_PLATFORM
 	  SPEAr1310 and SPEAr320 evaluation boards & TI (www.ti.com)
 	  boards like am335x, dm814x, dm813x and dm811x.
 
+config CAN_C_CAN_STRICT_FRAME_ORDERING
+	bool "Force a strict RX CAN frame order (may cause frame loss)"
+	---help---
+	  The RX split buffer prevents packet reordering but can cause packet
+	  loss. Only enable this option when you accept to lose CAN frames
+	  in favour of getting the received CAN frames in the correct order.
+
 config CAN_C_CAN_PCI
 	tristate "Generic PCI Bus based C_CAN/D_CAN driver"
 	depends on PCI
drivers/net/can/c_can/c_can.c +48 -14

105 lines not on this page · raw message

author
#12 lore raw

PATCH v2 12/21 can: c_can": Work around C_CAN RX wreckage

In reply to Thomas Gleixner

Alexander reported that the new optimized handling of the RX fifo causes random packet loss on Intel PCH C_CAN hardware.

After a few fruitless debugging sessions I got hold of a PCH (eg20t) afflicted system. That machine does not have the CAN interface wired up, but it was possible to reproduce the issue with the HW loopback mode.

As Alexander observed correctly, clearing the NewDat flag along with reading out the message buffer causes that issue on C_CAN, while D_CAN handles that correctly.

Instead of restoring the original message buffer handling horror the following workaround solves the issue:

transfer buffer to IF without clearing the NewDat handle the message
clear NewDat bit

That's similar to the original code but conditional for C_CAN.

I really wonder why all user manuals (C_CAN, Intel PCH and some more) recommend to clear the NewDat bit right away. The knows it all Oracle operated by Gurgle does not unearth any useful information either. I simply cannot believe that we are the first to uncover that HW issue.

Reported-and-tested-by Alexander Stein <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   13 ++++++++++---
 drivers/net/can/c_can/c_can.h |    1 +
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/net/can/c_can/c_can.c +10 -3
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -647,6 +647,10 @@ static int c_can_start(struct net_device
 	if (err)
 		return err;
 
+	/* Setup the command for new messages */
+	priv->comm_rcv_high = priv->type != BOSCH_D_CAN ?
+		IF_COMM_RCV_LOW : IF_COMM_RCV_HIGH;
+
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	/* reset tx helper pointers and the rx mask */

@@ -791,14 +795,15 @@ static u32 c_can_adjust_pending(u32 pend
 	return pend & ~((1 << lasts) - 1);
 }
 
-static inline void c_can_rx_object_get(struct net_device *dev, u32 obj)
+static inline void c_can_rx_object_get(struct net_device *dev,
+				       struct c_can_priv *priv, u32 obj)
 {
 #ifdef CONFIG_CAN_C_CAN_STRICT_FRAME_ORDERING
 	if (obj < C_CAN_MSG_RX_LOW_LAST)
 		c_can_object_get(dev, IF_RX, obj, IF_COMM_RCV_LOW);
 	else
 #endif
-		c_can_object_get(dev, IF_RX, obj, IF_COMM_RCV_HIGH);
+		c_can_object_get(dev, IF_RX, obj, priv->comm_rcv_high);
 }
 
 static inline void c_can_rx_finalize(struct net_device *dev,

@@ -813,6 +818,8 @@ static inline void c_can_rx_finalize(str
 		c_can_activate_all_lower_rx_msg_obj(dev, IF_RX);
 	}
 #endif
+	if (priv->type != BOSCH_D_CAN)
+		c_can_object_get(dev, IF_RX, obj, IF_COMM_CLR_NEWDAT);
 }
 
 static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv,

@@ -823,7 +830,7 @@ static int c_can_read_objects(struct net
 	while ((obj = ffs(pend)) && quota > 0) {
 		pend &= ~BIT(obj - 1);
 
-		c_can_rx_object_get(dev, obj);
+		c_can_rx_object_get(dev, priv, obj);
 		ctrl = priv->read_reg(priv, C_CAN_IFACE(MSGCTRL_REG, IF_RX));
 
 		if (ctrl & IF_MCONT_MSGLST) {
drivers/net/can/c_can/c_can.h +1 -0
Index: linux-2.6/drivers/net/can/c_can/c_can.h
===================================================================
@@ -198,6 +198,7 @@ struct c_can_priv {
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	u32 comm_rcv_high;
 	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
 };
#13 lore raw

Re: [patch V2 12/21] can: c_can": Work around C_CAN RX wreckage

I think the " in the subject got there by accident :-)

Alexander

Quoted 99 lines · Thomas Gleixner

Alexander reported that the new optimized handling of the RX fifo causes random packet loss on Intel PCH C_CAN hardware.

After a few fruitless debugging sessions I got hold of a PCH (eg20t) afflicted system. That machine does not have the CAN interface wired up, but it was possible to reproduce the issue with the HW loopback mode.

As Alexander observed correctly, clearing the NewDat flag along with reading out the message buffer causes that issue on C_CAN, while D_CAN handles that correctly.

Instead of restoring the original message buffer handling horror the following workaround solves the issue:

transfer buffer to IF without clearing the NewDat handle the message
clear NewDat bit

That's similar to the original code but conditional for C_CAN.

I really wonder why all user manuals (C_CAN, Intel PCH and some more) recommend to clear the NewDat bit right away. The knows it all Oracle operated by Gurgle does not unearth any useful information either. I simply cannot believe that we are the first to uncover that HW issue.

Reported-and-tested-by Alexander Stein <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   13 ++++++++++---
 drivers/net/can/c_can/c_can.h |    1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================

drivers/net/can/c_can/c_can.c +10 -3
@@ -647,6 +647,10 @@ static int c_can_start(struct net_device
 	if (err)
 		return err;
 
+	/* Setup the command for new messages */
+	priv->comm_rcv_high = priv->type != BOSCH_D_CAN ?
+		IF_COMM_RCV_LOW : IF_COMM_RCV_HIGH;
+
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	/* reset tx helper pointers and the rx mask */

@@ -791,14 +795,15 @@ static u32 c_can_adjust_pending(u32 pend
 	return pend & ~((1 << lasts) - 1);
 }
 
-static inline void c_can_rx_object_get(struct net_device *dev, u32 obj)
+static inline void c_can_rx_object_get(struct net_device *dev,
+				       struct c_can_priv *priv, u32 obj)
 {
 #ifdef CONFIG_CAN_C_CAN_STRICT_FRAME_ORDERING
 	if (obj < C_CAN_MSG_RX_LOW_LAST)
 		c_can_object_get(dev, IF_RX, obj, IF_COMM_RCV_LOW);
 	else
 #endif
-		c_can_object_get(dev, IF_RX, obj, IF_COMM_RCV_HIGH);
+		c_can_object_get(dev, IF_RX, obj, priv->comm_rcv_high);
 }
 
 static inline void c_can_rx_finalize(struct net_device *dev,

@@ -813,6 +818,8 @@ static inline void c_can_rx_finalize(str
 		c_can_activate_all_lower_rx_msg_obj(dev, IF_RX);
 	}
 #endif
+	if (priv->type != BOSCH_D_CAN)
+		c_can_object_get(dev, IF_RX, obj, IF_COMM_CLR_NEWDAT);
 }
 
 static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv,

@@ -823,7 +830,7 @@ static int c_can_read_objects(struct net
 	while ((obj = ffs(pend)) && quota > 0) {
 		pend &= ~BIT(obj - 1);
 
-		c_can_rx_object_get(dev, obj);
+		c_can_rx_object_get(dev, priv, obj);
 		ctrl = priv->read_reg(priv, C_CAN_IFACE(MSGCTRL_REG, IF_RX));
 
 		if (ctrl & IF_MCONT_MSGLST) {

Index: linux-2.6/drivers/net/can/c_can/c_can.h
===================================================================

drivers/net/can/c_can/c_can.h +1 -0
@@ -198,6 +198,7 @@ struct c_can_priv {
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	u32 comm_rcv_high;
 	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
 };
Signature · 12 lines
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: [email protected]
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082
author
#14 lore raw

Re: [patch V2 12/21] can: c_can": Work around C_CAN RX wreckage

Quoted 1 line · Alexander Stein

I think the " in the subject got there by accident :-)

Definitely :)

maintainer
#15 lore raw

Re: [patch V2 12/21] can: c_can": Work around C_CAN RX wreckage

Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Alexander Stein

I think the " in the subject got there by accident :-)

Definitely :)

I'll fix this while applying.

Marc

Signature · 4 lines
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   |
author
#16 lore raw

PATCH v2 13/21 can: c_can: Cleanup irq enable/disable

In reply to Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)
drivers/net/can/c_can/c_can.c +12 -25
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -60,6 +60,8 @@
 #define CONTROL_IE		BIT(1)
 #define CONTROL_INIT		BIT(0)
 
+#define CONTROL_IRQMSK		(CONTROL_EIE | CONTROL_IE | CONTROL_SIE)
+
 /* test register */
 #define TEST_RX			BIT(7)
 #define TEST_TX1		BIT(6)

@@ -146,13 +148,6 @@
 #define IF_RX			0
 #define IF_TX			1
 
-/* status interrupt */
-#define STATUS_INTERRUPT	0x8000
-
-/* global interrupt masks */
-#define ENABLE_ALL_INTERRUPTS	1
-#define DISABLE_ALL_INTERRUPTS	0
-
 /* minimum timeout for checking BUSY status */
 #define MIN_TIMEOUT_VALUE	6
 

@@ -246,18 +241,14 @@ static u32 c_can_read_reg32(struct c_can
 	return val;
 }
 
-static void c_can_enable_all_interrupts(struct c_can_priv *priv,
-						int enable)
+static void c_can_irq_control(struct c_can_priv *priv, bool enable)
 {
-	unsigned int cntrl_save = priv->read_reg(priv,
-						C_CAN_CTRL_REG);
+	u32 ctrl = priv->read_reg(priv,	C_CAN_CTRL_REG) & ~CONTROL_IRQMSK;
 
 	if (enable)
-		cntrl_save |= (CONTROL_SIE | CONTROL_EIE | CONTROL_IE);
-	else
-		cntrl_save &= ~(CONTROL_EIE | CONTROL_IE | CONTROL_SIE);
+		ctrl |= CONTROL_IRQMSK;
 
-	priv->write_reg(priv, C_CAN_CTRL_REG, cntrl_save);
+	priv->write_reg(priv, C_CAN_CTRL_REG, ctrl);
 }
 
 static inline int c_can_msg_obj_is_busy(struct c_can_priv *priv, int iface)

@@ -664,10 +655,7 @@ static void c_can_stop(struct net_device
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
-	/* disable all interrupts */
-	c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
-
-	/* set the state as STOPPED */
+	c_can_irq_control(priv, false);
 	priv->can.state = CAN_STATE_STOPPED;
 }
 

@@ -682,8 +670,7 @@ static int c_can_set_mode(struct net_dev
 		if (err)
 			return err;
 		netif_wake_queue(dev);
-		/* enable status change, error and module interrupts */
-		c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
+		c_can_irq_control(priv, true);
 		break;
 	default:
 		return -EOPNOTSUPP;

@@ -1144,7 +1131,7 @@ end:
 		napi_complete(napi);
 		/* enable all IRQs if we are not in bus off state */
 		if (priv->can.state != CAN_STATE_BUS_OFF)
-			c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
+			c_can_irq_control(priv, true);
 	}
 
 	return work_done;

@@ -1159,7 +1146,7 @@ static irqreturn_t c_can_isr(int irq, vo
 		return IRQ_NONE;
 
 	/* disable all interrupts and schedule the NAPI */
-	c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
+	c_can_irq_control(priv, false);
 	napi_schedule(&priv->napi);
 
 	return IRQ_HANDLED;

@@ -1197,7 +1184,7 @@ static int c_can_open(struct net_device
 
 	napi_enable(&priv->napi);
 	/* enable status change, error and module interrupts */
-	c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
+	c_can_irq_control(priv, true);
 	netif_start_queue(dev);
 
 	return 0;

@@ -1324,7 +1311,7 @@ int c_can_power_up(struct net_device *de
 
 	ret = c_can_start(dev);
 	if (!ret)
-		c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
+		c_can_irq_control(priv, true);
 
 	return ret;
 }
author
#17 lore raw

PATCH v2 14/21 can: c_can: Cleanup c_can_read_msg_object()

In reply to Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
drivers/net/can/c_can/c_can.c +15 -17
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -380,15 +380,13 @@ static int c_can_handle_lost_msg_obj(str
 	return 1;
 }
 
-static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
+static int c_can_read_msg_object(struct net_device *dev, int iface, u32 ctrl)
 {
-	u16 flags, data;
-	int i;
-	unsigned int val;
-	struct c_can_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
-	struct sk_buff *skb;
+	struct c_can_priv *priv = netdev_priv(dev);
 	struct can_frame *frame;
+	struct sk_buff *skb;
+	u32 arb, data;
 
 	skb = alloc_can_skb(dev, &frame);
 	if (!skb) {

@@ -398,21 +396,21 @@ static int c_can_read_msg_object(struct
 
 	frame->can_dlc = get_can_dlc(ctrl & 0x0F);
 
-	flags =	priv->read_reg(priv, C_CAN_IFACE(ARB2_REG, iface));
-	val = priv->read_reg(priv, C_CAN_IFACE(ARB1_REG, iface)) |
-		(flags << 16);
+	arb = priv->read_reg(priv, C_CAN_IFACE(ARB1_REG, iface));
+	arb |= priv->read_reg(priv, C_CAN_IFACE(ARB2_REG, iface)) << 16;
 
-	if (flags & IF_ARB_MSGXTD)
-		frame->can_id = (val & CAN_EFF_MASK) | CAN_EFF_FLAG;
+	if (arb & (IF_ARB_MSGXTD << 16))
+		frame->can_id = (arb & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
-		frame->can_id = (val >> 18) & CAN_SFF_MASK;
+		frame->can_id = (arb >> 18) & CAN_SFF_MASK;
 
-	if (flags & IF_ARB_TRANSMIT)
+	if (arb & (IF_ARB_TRANSMIT << 16)) {
 		frame->can_id |= CAN_RTR_FLAG;
-	else {
-		for (i = 0; i < frame->can_dlc; i += 2) {
-			data = priv->read_reg(priv,
-				C_CAN_IFACE(DATA1_REG, iface) + i / 2);
+	} else {
+		int i, dreg = C_CAN_IFACE(DATA1_REG, iface);
+
+		for (i = 0; i < frame->can_dlc; i += 2, dreg ++) {
+			data = priv->read_reg(priv, dreg);
 			frame->data[i] = data;
 			frame->data[i + 1] = data >> 8;
 		}
author
#18 lore raw

PATCH v2 15/21 can: c_can Cleanup setup of receive buffers

In reply to Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)
drivers/net/can/c_can/c_can.c +18 -22
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -125,6 +125,10 @@
 /* For the high buffers we clear the interrupt bit and newdat */
 #define IF_COMM_RCV_HIGH	(IF_COMM_RCV_LOW | IF_COMM_CLR_NEWDAT)
 
+
+/* Receive setup of message objects */
+#define IF_COMM_RCV_SETUP	(IF_COMM_MASK | IF_COMM_ARB | IF_COMM_CONTROL)
+
 /* IFx arbitration */
 #define IF_ARB_MSGVAL		BIT(15)
 #define IF_ARB_MSGXTD		BIT(14)

@@ -142,6 +146,9 @@
 #define IF_MCONT_EOB		BIT(7)
 #define IF_MCONT_DLC_MASK	0xf
 
+#define IF_MCONT_RCV		(IF_MCONT_RXIE | IF_MCONT_UMASK)
+#define IF_MCONT_RCV_EOB	(IF_MCONT_RCV | IF_MCONT_EOB)
+
 /*
  * Use IF1 for RX and IF2 for TX
  */

@@ -424,30 +431,20 @@ static int c_can_read_msg_object(struct
 }
 
 static void c_can_setup_receive_object(struct net_device *dev, int iface,
-					int objno, unsigned int mask,
-					unsigned int id, unsigned int mcont)
+				       u32 obj, u32 mask, u32 id, u32 mcont)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
-	priv->write_reg(priv, C_CAN_IFACE(MASK1_REG, iface),
-			IFX_WRITE_LOW_16BIT(mask));
-
-	/* According to C_CAN documentation, the reserved bit
-	 * in IFx_MASK2 register is fixed 1
-	 */
-	priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface),
-			IFX_WRITE_HIGH_16BIT(mask) | BIT(13));
-
-	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
-			IFX_WRITE_LOW_16BIT(id));
-	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
-			(IF_ARB_MSGVAL | IFX_WRITE_HIGH_16BIT(id)));
+	mask |= BIT(29);
+	priv->write_reg(priv, C_CAN_IFACE(MASK1_REG, iface), mask);
+	priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface), mask >> 16);
+
+	id |= IF_ARB_MSGVAL << 16;
+	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), id);
+	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), id >> 16);
 
 	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), mcont);
-	c_can_object_put(dev, iface, objno, IF_COMM_ALL & ~IF_COMM_TXRQST);
-
-	netdev_dbg(dev, "obj no:%d, msgval:0x%08x\n", objno,
-			c_can_read_reg32(priv, C_CAN_MSGVAL1_REG));
+	c_can_object_put(dev, iface, obj, IF_COMM_RCV_SETUP);
 }
 
 static void c_can_inval_msg_object(struct net_device *dev, int iface, int objno)

@@ -581,11 +578,10 @@ static void c_can_configure_msg_objects(
 
 	/* setup receive message objects */
 	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
-		c_can_setup_receive_object(dev, IF_RX, i, 0, 0,
-					   IF_MCONT_RXIE | IF_MCONT_UMASK);
+		c_can_setup_receive_object(dev, IF_RX, i, 0, 0, IF_MCONT_RCV);
 
 	c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
-			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
+				   IF_MCONT_RCV_EOB);
 }
 
 /*
author
#19 lore raw

PATCH v2 16/21 can: c_can: Cleanup c_can_inval_msg_object()

In reply to Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/net/can/c_can/c_can.c +5 -5
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -129,6 +129,9 @@
 /* Receive setup of message objects */
 #define IF_COMM_RCV_SETUP	(IF_COMM_MASK | IF_COMM_ARB | IF_COMM_CONTROL)
 
+/* Invalidation of message objects */
+#define IF_COMM_INVAL		(IF_COMM_ARB | IF_COMM_CONTROL)
+
 /* IFx arbitration */
 #define IF_ARB_MSGVAL		BIT(15)
 #define IF_ARB_MSGXTD		BIT(14)

@@ -447,7 +450,7 @@ static void c_can_setup_receive_object(s
 	c_can_object_put(dev, iface, obj, IF_COMM_RCV_SETUP);
 }
 
-static void c_can_inval_msg_object(struct net_device *dev, int iface, int objno)
+static void c_can_inval_msg_object(struct net_device *dev, int iface, int obj)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 

@@ -455,10 +458,7 @@ static void c_can_inval_msg_object(struc
 	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), 0);
 	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), 0);
 
-	c_can_object_put(dev, iface, objno, IF_COMM_ARB | IF_COMM_CONTROL);
-
-	netdev_dbg(dev, "obj no:%d, msgval:0x%08x\n", objno,
-			c_can_read_reg32(priv, C_CAN_MSGVAL1_REG));
+	c_can_object_put(dev, iface, obj, IF_COMM_INVAL);
 }
 
 static inline int c_can_is_next_tx_obj_busy(struct c_can_priv *priv, int objno)
author
#20 lore raw

PATCH v2 17/21 can: c_can: Cleanup c_can_msg_obj_put/get()

In reply to Thomas Gleixner

Sigh!

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   60 +++++++++++-------------------------------
 1 file changed, 16 insertions(+), 44 deletions(-)
drivers/net/can/c_can/c_can.c +16 -44
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -261,61 +261,33 @@ static void c_can_irq_control(struct c_c
 	priv->write_reg(priv, C_CAN_CTRL_REG, ctrl);
 }
 
-static inline int c_can_msg_obj_is_busy(struct c_can_priv *priv, int iface)
+static void c_can_obj_update(struct net_device *dev, int iface, u32 cmd, u32 obj)
 {
-	int count = MIN_TIMEOUT_VALUE;
+	struct c_can_priv *priv = netdev_priv(dev);
+	int cnt, reg = C_CAN_IFACE(COMREQ_REG, iface);
+
+	priv->write_reg(priv, reg + 1, cmd);
+	priv->write_reg(priv, reg, obj);
 
-	while (count && priv->read_reg(priv,
-				C_CAN_IFACE(COMREQ_REG, iface)) &
-				IF_COMR_BUSY) {
-		count--;
+	for (cnt = MIN_TIMEOUT_VALUE; cnt; cnt--) {
+		if (!(priv->read_reg(priv, reg) & IF_COMR_BUSY))
+			return;
 		udelay(1);
 	}
+	netdev_err(dev, "Updating object timed out\n");
 
-	if (!count)
-		return 1;
-
-	return 0;
 }
 
-static inline void c_can_object_get(struct net_device *dev,
-					int iface, int objno, int mask)
+static inline void c_can_object_get(struct net_device *dev, int iface,
+				    u32 obj, u32 cmd)
 {
-	struct c_can_priv *priv = netdev_priv(dev);
-
-	/*
-	 * As per specs, after writting the message object number in the
-	 * IF command request register the transfer b/w interface
-	 * register and message RAM must be complete in 6 CAN-CLK
-	 * period.
-	 */
-	priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
-			IFX_WRITE_LOW_16BIT(mask));
-	priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
-			IFX_WRITE_LOW_16BIT(objno));
-
-	if (c_can_msg_obj_is_busy(priv, iface))
-		netdev_err(dev, "timed out in object get\n");
+	c_can_obj_update(dev, iface, cmd, obj);
 }
 
-static inline void c_can_object_put(struct net_device *dev,
-					int iface, int objno, int mask)
+static inline void c_can_object_put(struct net_device *dev, int iface,
+				    u32 obj, u32 cmd)
 {
-	struct c_can_priv *priv = netdev_priv(dev);
-
-	/*
-	 * As per specs, after writting the message object number in the
-	 * IF command request register the transfer b/w interface
-	 * register and message RAM must be complete in 6 CAN-CLK
-	 * period.
-	 */
-	priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
-			(IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask)));
-	priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
-			IFX_WRITE_LOW_16BIT(objno));
-
-	if (c_can_msg_obj_is_busy(priv, iface))
-		netdev_err(dev, "timed out in object put\n");
+	c_can_obj_update(dev, iface, cmd | IF_COMM_WR, obj);
 }
 
 static void c_can_write_msg_object(struct net_device *dev,
author
#21 lore raw

PATCH v2 18/21 can: c_can: Cleanup c_can_write_msg_object()

In reply to Thomas Gleixner

Remove the MASK from the TX transfer side.

Make the code readable and get rid of the annoying IFX_WRITE_XXX_16BIT macros which are just obfuscating the code.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   51 ++++++++++++++++++++----------------------
 drivers/net/can/c_can/c_can.h |    8 ------
 2 files changed, 25 insertions(+), 34 deletions(-)
drivers/net/can/c_can/c_can.c +25 -26
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -113,9 +113,11 @@
 #define IF_COMM_CLR_NEWDAT	IF_COMM_TXRQST
 #define IF_COMM_DATAA		BIT(1)
 #define IF_COMM_DATAB		BIT(0)
-#define IF_COMM_ALL		(IF_COMM_MASK | IF_COMM_ARB | \
-				IF_COMM_CONTROL | IF_COMM_TXRQST | \
-				IF_COMM_DATAA | IF_COMM_DATAB)
+
+/* TX buffer setup */
+#define IF_COMM_TX		(IF_COMM_ARB | IF_COMM_CONTROL | \
+				 IF_COMM_TXRQST |		 \
+				 IF_COMM_DATAA | IF_COMM_DATAB)
 
 /* For the low buffers we clear the interrupt bit, but keep newdat */
 #define IF_COMM_RCV_LOW		(IF_COMM_MASK | IF_COMM_ARB | \

@@ -152,6 +154,8 @@
 #define IF_MCONT_RCV		(IF_MCONT_RXIE | IF_MCONT_UMASK)
 #define IF_MCONT_RCV_EOB	(IF_MCONT_RCV | IF_MCONT_EOB)
 
+#define IF_MCONT_TX		(IF_MCONT_TXIE | IF_MCONT_EOB)
+
 /*
  * Use IF1 for RX and IF2 for TX
  */

@@ -290,40 +294,35 @@ static inline void c_can_object_put(stru
 	c_can_obj_update(dev, iface, cmd | IF_COMM_WR, obj);
 }
 
-static void c_can_write_msg_object(struct net_device *dev,
-			int iface, struct can_frame *frame, int objno)
+static void c_can_write_msg_object(struct net_device *dev, int iface,
+				   struct can_frame *frame, int obj)
 {
-	int i;
-	u16 flags = 0;
-	unsigned int id;
 	struct c_can_priv *priv = netdev_priv(dev);
+	u16 ctrl = IF_MCONT_TX | frame->can_dlc;
+	u32 arb = IF_ARB_MSGVAL << 16;
+	int i;
+
+	if (frame->can_id & CAN_EFF_FLAG) {
+		arb |= frame->can_id & CAN_EFF_MASK;
+		arb |= IF_ARB_MSGXTD << 16;
+	} else {
+		arb |= (frame->can_id & CAN_SFF_MASK) << 18;
+	}
 
 	if (!(frame->can_id & CAN_RTR_FLAG))
-		flags |= IF_ARB_TRANSMIT;
+		arb |= IF_ARB_TRANSMIT << 16;
 
-	if (frame->can_id & CAN_EFF_FLAG) {
-		id = frame->can_id & CAN_EFF_MASK;
-		flags |= IF_ARB_MSGXTD;
-	} else
-		id = ((frame->can_id & CAN_SFF_MASK) << 18);
-
-	flags |= IF_ARB_MSGVAL;
-
-	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
-				IFX_WRITE_LOW_16BIT(id));
-	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), flags |
-				IFX_WRITE_HIGH_16BIT(id));
+	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), arb);
+	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), arb >> 16);
+
+	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
 
 	for (i = 0; i < frame->can_dlc; i += 2) {
 		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
 				frame->data[i] | (frame->data[i + 1] << 8));
 	}
 
-	/* enable interrupt for this message object */
-	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
-			IF_MCONT_TXIE | IF_MCONT_TXRQST | IF_MCONT_EOB |
-			frame->can_dlc);
-	c_can_object_put(dev, iface, objno, IF_COMM_ALL);
+	c_can_object_put(dev, iface, obj, IF_COMM_TX);
 }
 
 static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
drivers/net/can/c_can/c_can.h +0 -8
Index: linux-2.6/drivers/net/can/c_can/c_can.h
===================================================================
@@ -22,14 +22,6 @@
 #ifndef C_CAN_H
 #define C_CAN_H
 
-/*
- * IFx register masks:
- * allow easy operation on 16-bit registers when the
- * argument is 32-bit instead
- */
-#define IFX_WRITE_LOW_16BIT(x)	((x) & 0xFFFF)
-#define IFX_WRITE_HIGH_16BIT(x)	(((x) & 0xFFFF0000) >> 16)
-
 /* message object split */
 #define C_CAN_NO_OF_OBJECTS	32
 #define C_CAN_MSG_OBJ_RX_NUM	16
author
#22 lore raw

PATCH v2 19/21 can: c_can: Use proper u32 variables in c_can_write_msg_object()

In reply to Thomas Gleixner

Instead of obfuscating the code by artificial 16 bit splits use the proper 32 bit assignments and split the result when writing to the interface.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/net/can/c_can/c_can.c +9 -9
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -135,9 +135,9 @@
 #define IF_COMM_INVAL		(IF_COMM_ARB | IF_COMM_CONTROL)
 
 /* IFx arbitration */
-#define IF_ARB_MSGVAL		BIT(15)
-#define IF_ARB_MSGXTD		BIT(14)
-#define IF_ARB_TRANSMIT		BIT(13)
+#define IF_ARB_MSGVAL		BIT(31)
+#define IF_ARB_MSGXTD		BIT(30)
+#define IF_ARB_TRANSMIT		BIT(29)
 
 /* IFx message control */
 #define IF_MCONT_NEWDAT		BIT(15)

@@ -299,18 +299,18 @@ static void c_can_write_msg_object(struc
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	u16 ctrl = IF_MCONT_TX | frame->can_dlc;
-	u32 arb = IF_ARB_MSGVAL << 16;
+	u32 arb = IF_ARB_MSGVAL;
 	int i;
 
 	if (frame->can_id & CAN_EFF_FLAG) {
 		arb |= frame->can_id & CAN_EFF_MASK;
-		arb |= IF_ARB_MSGXTD << 16;
+		arb |= IF_ARB_MSGXTD;
 	} else {
 		arb |= (frame->can_id & CAN_SFF_MASK) << 18;
 	}
 
 	if (!(frame->can_id & CAN_RTR_FLAG))
-		arb |= IF_ARB_TRANSMIT << 16;
+		arb |= IF_ARB_TRANSMIT;
 
 	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), arb);
 	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), arb >> 16);

@@ -380,12 +380,12 @@ static int c_can_read_msg_object(struct
 	arb = priv->read_reg(priv, C_CAN_IFACE(ARB1_REG, iface));
 	arb |= priv->read_reg(priv, C_CAN_IFACE(ARB2_REG, iface)) << 16;
 
-	if (arb & (IF_ARB_MSGXTD << 16))
+	if (arb & IF_ARB_MSGXTD)
 		frame->can_id = (arb & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
 		frame->can_id = (arb >> 18) & CAN_SFF_MASK;
 
-	if (arb & (IF_ARB_TRANSMIT << 16)) {
+	if (arb & IF_ARB_TRANSMIT) {
 		frame->can_id |= CAN_RTR_FLAG;
 	} else {
 		int i, dreg = C_CAN_IFACE(DATA1_REG, iface);

@@ -413,7 +413,7 @@ static void c_can_setup_receive_object(s
 	priv->write_reg(priv, C_CAN_IFACE(MASK1_REG, iface), mask);
 	priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface), mask >> 16);
 
-	id |= IF_ARB_MSGVAL << 16;
+	id |= IF_ARB_MSGVAL;
 	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), id);
 	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), id >> 16);
author
#23 lore raw

PATCH v2 20/21 can: c_can: Remove tx locking

In reply to Thomas Gleixner

Mark suggested to use one IF for the softirq and the other for the xmit function to avoid the xmit lock.

That requires to write the frame into the interface first, then handle the echo skb and store the dlc before committing the TX request to the message ram.

We use an atomic to handle the active buffers instead of reading the MSGVAL register as thats way faster especially on PCH/x86.

Suggested-by Mark <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |  127 +++++++++++++-----------------------------
 drivers/net/can/c_can/c_can.h |    8 --
 2 files changed, 43 insertions(+), 92 deletions(-)
drivers/net/can/c_can/c_can.c +42 -85
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -237,24 +237,6 @@ static inline void c_can_reset_ram(const
 		priv->raminit(priv, enable);
 }
 
-static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
-{
-	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
-			C_CAN_MSG_OBJ_TX_FIRST;
-}
-
-static inline int get_tx_echo_msg_obj(int txecho)
-{
-	return (txecho & C_CAN_NEXT_MSG_OBJ_MASK) + C_CAN_MSG_OBJ_TX_FIRST;
-}
-
-static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index)
-{
-	u32 val = priv->read_reg(priv, index);
-	val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
-	return val;
-}
-
 static void c_can_irq_control(struct c_can_priv *priv, bool enable)
 {
 	u32 ctrl = priv->read_reg(priv,	C_CAN_CTRL_REG) & ~CONTROL_IRQMSK;

@@ -294,8 +276,8 @@ static inline void c_can_object_put(stru
 	c_can_obj_update(dev, iface, cmd | IF_COMM_WR, obj);
 }
 
-static void c_can_write_msg_object(struct net_device *dev, int iface,
-				   struct can_frame *frame, int obj)
+static void c_can_setup_tx_object(struct net_device *dev, int iface,
+				  struct can_frame *frame, int obj)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	u16 ctrl = IF_MCONT_TX | frame->can_dlc;

@@ -321,8 +303,6 @@ static void c_can_write_msg_object(struc
 		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
 				frame->data[i] | (frame->data[i + 1] << 8));
 	}
-
-	c_can_object_put(dev, iface, obj, IF_COMM_TX);
 }
 
 static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,

@@ -432,47 +412,38 @@ static void c_can_inval_msg_object(struc
 	c_can_object_put(dev, iface, obj, IF_COMM_INVAL);
 }
 
-static inline int c_can_is_next_tx_obj_busy(struct c_can_priv *priv, int objno)
-{
-	int val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
-
-	/*
-	 * as transmission request register's bit n-1 corresponds to
-	 * message object n, we need to handle the same properly.
-	 */
-	if (val & (1 << (objno - 1)))
-		return 1;
-
-	return 0;
-}
-
 static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
-					struct net_device *dev)
+				    struct net_device *dev)
 {
-	u32 msg_obj_no;
-	struct c_can_priv *priv = netdev_priv(dev);
 	struct can_frame *frame = (struct can_frame *)skb->data;
+	struct c_can_priv *priv = netdev_priv(dev);
+	u32 idx, obj;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
-
-	spin_lock_bh(&priv->xmit_lock);
-	msg_obj_no = get_tx_next_msg_obj(priv);
-
-	/* prepare message object for transmission */
-	c_can_write_msg_object(dev, IF_TX, frame, msg_obj_no);
-	priv->dlc[msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST] = frame->can_dlc;
-	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
-
 	/*
-	 * we have to stop the queue in case of a wrap around or
-	 * if the next TX message object is still in use
+	 * This is not a FIFO. C/D_CAN sends out the buffers
+	 * prioritized. The lowest buffer number wins.
 	 */
-	priv->tx_next++;
-	if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv)) ||
-			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
+	idx = fls(atomic_read(&priv->tx_active));
+	obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+
+	/* If this is the last buffer, stop the xmit queue */
+	if (idx == C_CAN_MSG_OBJ_TX_NUM - 1)
 		netif_stop_queue(dev);
-	spin_unlock_bh(&priv->xmit_lock);
+	/*
+	 * Store the message in the interface so we can call
+	 * can_put_echo_skb(). We must do this before we enable
+	 * transmit as we might race against do_tx().
+	 */
+	c_can_setup_tx_object(dev, IF_TX, frame, obj);
+	priv->dlc[idx] = frame->can_dlc;
+	can_put_echo_skb(skb, dev, idx);
+
+	/* Update the active bits */
+	atomic_add((1 << idx), &priv->tx_active);
+	/* Start transmission */
+	c_can_object_put(dev, IF_TX, obj, IF_COMM_TX);
 
 	return NETDEV_TX_OK;
 }

@@ -589,6 +560,10 @@ static int c_can_chip_config(struct net_
 	/* set a `lec` value so that we can check for updates later */
 	priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
 
+	/* Clear all internal status */
+	atomic_set(&priv->tx_active, 0);
+	priv->rxmasked = 0;
+
 	/* set bittiming params */
 	return c_can_set_bittiming(dev);
 }

@@ -609,10 +584,6 @@ static int c_can_start(struct net_device
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	/* reset tx helper pointers and the rx mask */
-	priv->tx_next = priv->tx_echo = 0;
-	priv->rxmasked = 0;
-
 	return 0;
 }
 

@@ -671,42 +642,29 @@ static int c_can_get_berr_counter(const
 	return err;
 }
 
-/*
- * priv->tx_echo holds the number of the oldest can_frame put for
- * transmission into the hardware, but not yet ACKed by the CAN tx
- * complete IRQ.
- *
- * We iterate from priv->tx_echo to priv->tx_next and check if the
- * packet has been transmitted, echo it back to the CAN framework.
- * If we discover a not yet transmitted packet, stop looking for more.
- */
 static void c_can_do_tx(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
-	u32 val, obj, pkts = 0, bytes = 0;
+	u32 idx, obj, pkts = 0, bytes = 0, pend, clr;
 
-	spin_lock_bh(&priv->xmit_lock);
+	clr = pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
 
-	for (; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
-		obj = get_tx_echo_msg_obj(priv->tx_echo);
-		val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
-
-		if (val & (1 << (obj - 1)))
-			break;
-
-		can_get_echo_skb(dev, obj - C_CAN_MSG_OBJ_TX_FIRST);
-		bytes += priv->dlc[obj - C_CAN_MSG_OBJ_TX_FIRST];
+	while ((idx = ffs(pend))) {
+		idx--;
+		pend &= ~(1 << idx);
+		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+		c_can_inval_msg_object(dev, IF_RX, obj);
+		can_get_echo_skb(dev, idx);
+		bytes += priv->dlc[idx];
 		pkts++;
-		c_can_inval_msg_object(dev, IF_TX, obj);
 	}
 
-	/* restart queue if wrap-up or if queue stalled on last pkt */
-	if (((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) != 0) ||
-			((priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
-		netif_wake_queue(dev);
+	/* Clear the bits in the tx_active mask */
+	atomic_sub(clr, &priv->tx_active);
 
-	spin_unlock_bh(&priv->xmit_lock);
+	if (clr & (1 << (C_CAN_MSG_OBJ_TX_NUM - 1)))
+		netif_wake_queue(dev);
 
 	if (pkts) {
 		stats->tx_bytes += bytes;

@@ -1192,7 +1150,6 @@ struct net_device *alloc_c_can_dev(void)
 		return NULL;
 
 	priv = netdev_priv(dev);
-	spin_lock_init(&priv->xmit_lock);
 	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
 
 	priv->dev = dev;
drivers/net/can/c_can/c_can.h +1 -7
Index: linux-2.6/drivers/net/can/c_can/c_can.h
===================================================================
@@ -37,8 +37,6 @@
 
 #define C_CAN_MSG_OBJ_RX_SPLIT	9
 #define C_CAN_MSG_RX_LOW_LAST	(C_CAN_MSG_OBJ_RX_SPLIT - 1)
-
-#define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
 #define RECEIVE_OBJECT_BITS	0x0000ffff
 
 enum reg {

@@ -175,16 +173,12 @@ struct c_can_priv {
 	struct napi_struct napi;
 	struct net_device *dev;
 	struct device *device;
-	spinlock_t xmit_lock;
-	int tx_object;
+	atomic_t tx_active;
 	int last_status;
 	u16 (*read_reg) (struct c_can_priv *priv, enum reg index);
 	void (*write_reg) (struct c_can_priv *priv, enum reg index, u16 val);
 	void __iomem *base;
 	const u16 *regs;
-	unsigned long irq_flags; /* for request_irq() */
-	unsigned int tx_next;
-	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
 	enum c_can_dev_id type;
 	u32 __iomem *raminit_ctrlreg;
author
#24 lore raw

PATCH v2 21/21 can: c_can: Speed up tx buffer invalidation

In reply to Thomas Gleixner

It's suffcient to kill the TXIE bit in the message control register even if the documentation of C and D CAN says that it's not allowed to do that while MSGVAL is set. Reality tells a different story and this change gives us another 2% of CPU back for not waiting on I/O.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   54 ++++++++++++++++++++++++++++++------------
 drivers/net/can/c_can/c_can.h |    1 
 2 files changed, 40 insertions(+), 15 deletions(-)
drivers/net/can/c_can/c_can.c +39 -15
Index: linux-2.6/drivers/net/can/c_can/c_can.c
===================================================================
@@ -276,11 +276,34 @@ static inline void c_can_object_put(stru
 	c_can_obj_update(dev, iface, cmd | IF_COMM_WR, obj);
 }
 
+/*
+ * Note: According to documentation clearing TXIE while MSGVAL is set
+ * is not allowed, but works nicely on C/DCAN. And that lowers the I/O
+ * load significantly.
+ */
+static void c_can_inval_tx_object(struct net_device *dev, int iface, int obj)
+{
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), 0);
+	c_can_object_put(dev, iface, obj, IF_COMM_INVAL);
+}
+
+static void c_can_inval_msg_object(struct net_device *dev, int iface, int obj)
+{
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), 0);
+	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), 0);
+	c_can_inval_tx_object(dev, iface, obj);
+}
+
 static void c_can_setup_tx_object(struct net_device *dev, int iface,
-				  struct can_frame *frame, int obj)
+				  struct can_frame *frame, int idx)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	u16 ctrl = IF_MCONT_TX | frame->can_dlc;
+	bool rtr = frame->can_id & CAN_RTR_FLAG;
 	u32 arb = IF_ARB_MSGVAL;
 	int i;
 

@@ -291,9 +314,20 @@ static void c_can_setup_tx_object(struct
 		arb |= (frame->can_id & CAN_SFF_MASK) << 18;
 	}
 
-	if (!(frame->can_id & CAN_RTR_FLAG))
+	if (!rtr)
 		arb |= IF_ARB_TRANSMIT;
 
+	/*
+	 * If we change the DIR bit, we need to invalidate the buffer
+	 * first, i.e. clear the MSGVAL flag in the arbiter.
+	 */
+	if (rtr != (bool)test_bit(idx, &priv->tx_dir)) {
+		u32 obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+
+		c_can_inval_msg_object(dev, iface, obj);
+		change_bit(idx, &priv->tx_dir);
+	}
+
 	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), arb);
 	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), arb >> 16);
 

@@ -401,17 +435,6 @@ static void c_can_setup_receive_object(s
 	c_can_object_put(dev, iface, obj, IF_COMM_RCV_SETUP);
 }
 
-static void c_can_inval_msg_object(struct net_device *dev, int iface, int obj)
-{
-	struct c_can_priv *priv = netdev_priv(dev);
-
-	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), 0);
-	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), 0);
-	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), 0);
-
-	c_can_object_put(dev, iface, obj, IF_COMM_INVAL);
-}
-
 static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {

@@ -436,7 +459,7 @@ static netdev_tx_t c_can_start_xmit(stru
 	 * can_put_echo_skb(). We must do this before we enable
 	 * transmit as we might race against do_tx().
 	 */
-	c_can_setup_tx_object(dev, IF_TX, frame, obj);
+	c_can_setup_tx_object(dev, IF_TX, frame, idx);
 	priv->dlc[idx] = frame->can_dlc;
 	can_put_echo_skb(skb, dev, idx);
 

@@ -563,6 +586,7 @@ static int c_can_chip_config(struct net_
 	/* Clear all internal status */
 	atomic_set(&priv->tx_active, 0);
 	priv->rxmasked = 0;
+	priv->tx_dir = 0;
 
 	/* set bittiming params */
 	return c_can_set_bittiming(dev);

@@ -654,7 +678,7 @@ static void c_can_do_tx(struct net_devic
 		idx--;
 		pend &= ~(1 << idx);
 		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
-		c_can_inval_msg_object(dev, IF_RX, obj);
+		c_can_inval_tx_object(dev, IF_RX, obj);
 		can_get_echo_skb(dev, idx);
 		bytes += priv->dlc[idx];
 		pkts++;
drivers/net/can/c_can/c_can.h +1 -0
Index: linux-2.6/drivers/net/can/c_can/c_can.h
===================================================================
@@ -174,6 +174,7 @@ struct c_can_priv {
 	struct net_device *dev;
 	struct device *device;
 	atomic_t tx_active;
+	unsigned long tx_dir;
 	int last_status;
 	u16 (*read_reg) (struct c_can_priv *priv, enum reg index);
 	void (*write_reg) (struct c_can_priv *priv, enum reg index, u16 val);
Tested-by on the v2 series
#25 lore raw
In reply to Thomas Gleixner
Quoted 50 lines · Thomas Gleixner

Changes since V1:

- Slightly modified version of the interrupt reduction patch
- Included the fix for PCH / C_CAN
- Lockless XMIT path
- Further reduction of register access
- Add the missing can.type setup in c_can_pci.c
- A pile of code cleanups.

It would be nice to reduce the register access some more by relying completely on the status interrupt, but it turned out that the TX/RXOK is not reliable enough. So we need to invalidate the message objects in the tx softirq handling.

But the overall change of this series is that the I/O load gets reduced by about 45% according to perf top. Though that PCH thing sucks. The beaglebone manages to almost saturate the bus with short packets at 1Mbit while PCH fails miserably and thats solely related to the miserable I/O performance.

time cangen can0 -g0 -p10 -I5A5 -L0 -x -n 1000000

arm: real 0m51.510s I/O read: ~6% I/O write: 1.5% ~3.5s
x86: real 1m48.533s I/O read: ~29% I/O write: 0.8% ~32 s!!

That's both with HW loopback on, as my PCH does not have a tranceiver. Granted the C_CAN in the PCH needs the double IF transfer to prevent the message loss versus the D_CAN in the ARM chip, but even that taken into account makes a whopping 16s per 1M messages vs. 3.5s on ARM.

w/o loopback the arm I/O read load drops to ~3.5% on the sender side and ~5.5% on the receiver side. The time drops to 50.5s on the transmit side if we do not have to get all the RX packets from HW loopback. On TX we have a ~10us large gap every 16 packets which is caused by the queue stall as we have to wait for the last packet in the "FIFO" to be transferred.

It seems there is a reason why the ATOM perf events do not expose the stalled cpu cycles. But it's easy to figure out. You can compare the CAN load case with some other scenario which has 100% CPU utilization by running

# perf stat -a sleep 60

The interesting part is: insns per cycle

CAN: 0.23 insns per cycle
Other: 0.53 insns per cycle

Quoted 8 lines

I don't have comparison numbers for ARM due to not supported perf events, but the perf top numbers and the transfer performance tell a clear story.

There might be room for a few improvements, but I'm running out of cycles and I really want to get the IF3 DMA feature functional on the TI chips, but that seems to be an equally tedious reverse engineering problem as the rest of this.

Run this patchset on top of linux-can-fixes-for-3.15-20140401 on idle system and with running iperf and I2C:
idle: 10 runs with 2 x 250'000 frames each, _no_ losts or swaps at all
load: 10 runs with 2 x 250'000 frames each, _no_ losts or swaps at all
\o/

CONFIG_CAN_C_CAN_STRICT_FRAME_ORDERING is not set. Maybe we can drop it now?

Despite that:

Tested-by Alexander Stein <[email protected]>

Thanks a lot and best regards,
Alexander

Signature · 12 lines
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: [email protected]
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082