[RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

Hello,

here is v2 of my patches for the C_CAN drivers.

For Michael I have prepared out-of-tree driver sources allowing to
easily build the drivers also for older 3.x kernel versions. More
tester are welcome.

Changes since v1:

- use init callback after renaming it from initram
- use different sets of interface registers for rx and tx (like PCH_CAN)
- use spin_[un]lock_bh to protect the tx objects

Wolfgang Grandegger (7):
  pch_can: add spinlocks to protect tx objects
  c_can: rename callback "initram" to "init" to more general usage
  c_can: use different sets of interface registers for rx and tx
  c_can_pci: introduce board specific PCI bar
  c_can_pci: enable PCI bus master only for MSI
  c_can_pci: add support for PCH CAN on Intel EG20T PCH
  c_can: add spinlock to protect tx and  objects

 drivers/net/can/c_can/c_can.c          |   66 +++++++++++++++++++++-----------
 drivers/net/can/c_can/c_can.h          |    3 +-
 drivers/net/can/c_can/c_can_pci.c      |   56 +++++++++++++++++++++++++--
 drivers/net/can/c_can/c_can_platform.c |    2 +-
 drivers/net/can/pch_can.c              |    9 +++++
 5 files changed, 108 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[RFC v2 1/7] pch_can: add spinlocks to protect tx objects

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/pch_can.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 48b3d62..442a04f 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -182,6 +182,7 @@ struct pch_can_priv {
 	struct napi_struct napi;
 	int tx_obj;	/* Point next Tx Obj index */
 	int use_msi;
+	spinlock_t lock;	/* to protect tx objects */
 };
 
 static const struct can_bittiming_const pch_can_bittiming_const = {
@@ -724,6 +725,8 @@ static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
 	struct net_device_stats *stats = &(priv->ndev->stats);
 	u32 dlc;
 
+	spin_lock_bh(&priv->lock);
+
 	can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1);
 	iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
 		  &priv->regs->ifregs[1].cmask);
@@ -734,6 +737,8 @@ static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
 	stats->tx_packets++;
 	if (int_stat == PCH_TX_OBJ_END)
 		netif_wake_queue(ndev);
+
+	spin_unlock_bh(&priv->lock);
 }
 
 static int pch_can_poll(struct napi_struct *napi, int quota)
@@ -901,6 +906,8 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (can_dropped_invalid_skb(ndev, skb))
 		return NETDEV_TX_OK;
 
+	spin_lock_bh(&priv->lock);
+
 	tx_obj_no = priv->tx_obj;
 	if (priv->tx_obj == PCH_TX_OBJ_END) {
 		if (ioread32(&priv->regs->treq2) & PCH_TREQ2_TX_MASK)
@@ -911,6 +918,8 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 		priv->tx_obj++;
 	}
 
+	spin_unlock_bh(&priv->lock);
+
 	/* Setting the CMASK register. */
 	pch_can_bit_set(&priv->regs->ifregs[1].cmask, PCH_CMASK_ALL);
 
-- 
1.7.9.5

[RFC v2 2/7] c_can: rename callback "initram" to "init" to more general usage

[Wolfgang Grandegger]

The callback "init" is called with the argument "true" when the
device is opened and with "false" when it is closed allowing for
device specific initialization and cleanup, e.g. raminit or reset.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can.c          |   16 ++++++++--------
 drivers/net/can/c_can/c_can.h          |    2 +-
 drivers/net/can/c_can/c_can_platform.c |    2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 5233b8f..3ae356f 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -233,10 +233,10 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
 		pm_runtime_put_sync(priv->device);
 }
 
-static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
+static inline void c_can_init(const struct c_can_priv *priv, bool enable)
 {
-	if (priv->raminit)
-		priv->raminit(priv, enable);
+	if (priv->init)
+		priv->init(priv, enable);
 }
 
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
@@ -1096,7 +1096,7 @@ static int c_can_open(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_pm_runtime_get_sync(priv);
-	c_can_reset_ram(priv, true);
+	c_can_init(priv, true);
 
 	/* open the can device */
 	err = open_candev(dev);
@@ -1125,7 +1125,7 @@ static int c_can_open(struct net_device *dev)
 exit_irq_fail:
 	close_candev(dev);
 exit_open_fail:
-	c_can_reset_ram(priv, false);
+	c_can_init(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 	return err;
 }
@@ -1140,7 +1140,7 @@ static int c_can_close(struct net_device *dev)
 	free_irq(dev->irq, dev);
 	close_candev(dev);
 
-	c_can_reset_ram(priv, false);
+	c_can_init(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;
@@ -1198,7 +1198,7 @@ int c_can_power_down(struct net_device *dev)
 
 	c_can_stop(dev);
 
-	c_can_reset_ram(priv, false);
+	c_can_init(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;
@@ -1217,7 +1217,7 @@ int c_can_power_up(struct net_device *dev)
 	WARN_ON(priv->type != BOSCH_D_CAN);
 
 	c_can_pm_runtime_get_sync(priv);
-	c_can_reset_ram(priv, true);
+	c_can_init(priv, true);
 
 	/* Clear PDR and INIT bits */
 	val = priv->read_reg(priv, C_CAN_CTRL_EX_REG);
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index d2e1c21..4baf3f6 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -171,7 +171,7 @@ struct c_can_priv {
 	enum c_can_dev_id type;
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
-	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	void (*init) (const struct c_can_priv *priv, bool enable);
 };
 
 struct net_device *alloc_c_can_dev(void);
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 0044fd8..5619b9c 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -205,7 +205,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		if (!priv->raminit_ctrlreg || priv->instance < 0)
 			dev_info(&pdev->dev, "control memory is not used for raminit\n");
 		else
-			priv->raminit = c_can_hw_raminit;
+			priv->init = c_can_hw_raminit;
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.7.9.5

Re: [RFC v2 2/7] c_can: rename callback "initram" to "init" to more general usage

[Alexander Stein]

Hello Wolfgang,

On Thursday 29 November 2012 15:39:42, Wolfgang Grandegger wrote:
> The callback "init" is called with the argument "true" when the
> device is opened and with "false" when it is closed allowing for
> device specific initialization and cleanup, e.g. raminit or reset.
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  drivers/net/can/c_can/c_can.c          |   16 ++++++++--------
>  drivers/net/can/c_can/c_can.h          |    2 +-
>  drivers/net/can/c_can/c_can_platform.c |    2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 5233b8f..3ae356f 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -233,10 +233,10 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
>  		pm_runtime_put_sync(priv->device);
>  }
>  
> -static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)

Where does c_can_reset_ram come from? I can't a patch to it in Linus' tree nor in linux-can repository.

Regards,
Alexander

Re: [RFC v2 2/7] c_can: rename callback "initram" to "init" to more general usage

[Wolfgang Grandegger]

On 12/03/2012 03:20 PM, Alexander Stein wrote:
> Hello Wolfgang,
> 
> On Thursday 29 November 2012 15:39:42, Wolfgang Grandegger wrote:
>> The callback "init" is called with the argument "true" when the
>> device is opened and with "false" when it is closed allowing for
>> device specific initialization and cleanup, e.g. raminit or reset.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  drivers/net/can/c_can/c_can.c          |   16 ++++++++--------
>>  drivers/net/can/c_can/c_can.h          |    2 +-
>>  drivers/net/can/c_can/c_can_platform.c |    2 +-
>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 5233b8f..3ae356f 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -233,10 +233,10 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
>>  		pm_runtime_put_sync(priv->device);
>>  }
>>  
>> -static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
> 
> Where does c_can_reset_ram come from? I can't a patch to it in Linus' tree nor in linux-can repository.

It is in the net-next tree and patches have been posted to this list.

commit 52cde85acc23f61b09dd0376c61eb891125c6990
Author: AnilKumar Ch <anilkumar@ti.com>
Date:   Wed Nov 21 11:14:10 2012 +0530

    can: c_can: Add d_can raminit support

    Add D_CAN raminit support to C_CAN driver to enable D_CAN RAM,
    which holds all the message objects during transmission or
    receiving of data. This initialization/de-initialization should
    be done in synchronous with D_CAN clock.

    In case of AM335X-EVM (current user of D_CAN driver) message RAM is
    controlled through control module register for both instances. So
    control module register details is required to initialization or
    de-initialization of message RAM according to instance number.

    Control module memory resource is obtained from D_CAN dt node and
    instance number obtained from device tree aliases node.

    This patch was tested on AM335x-EVM along with pinctrl data addition
    patch, d_can dt aliases addition and control module data addition.
    pinctrl data addition is not added to am335x-evm.dts (only supports
    CPLD profile#0) because d_can1 is supported under CPLD profile#1.

    Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
    [mkl: fix instance for non DT in probe, cleaned up raminit]
    Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Wolfgang.

[RFC v2 3/7] c_can: use different sets of interface registers for rx and tx

[Wolfgang Grandegger]

To avoid conflicts between CPU access to the message RAM we now
use the first set of interface registers for RX and the second
for TX.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can.c |   37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 3ae356f..27e45e6 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -158,6 +158,10 @@
 #define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
 #define RECEIVE_OBJECT_BITS	0x0000ffff
 
+/* message interface used for rx and tx */
+#define IFACE_RX		0
+#define IFACE_TX		1
+
 /* status interrupt */
 #define STATUS_INTERRUPT	0x8000
 
@@ -544,7 +548,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	msg_obj_no = get_tx_next_msg_obj(priv);
 
 	/* prepare message object for transmission */
-	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
+	c_can_write_msg_object(dev, IFACE_TX, frame, msg_obj_no);
 	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
 
 	/*
@@ -606,16 +610,18 @@ static void c_can_configure_msg_objects(struct net_device *dev)
 	int i;
 
 	/* first invalidate all message objects */
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
-		c_can_inval_msg_object(dev, 0, i);
+	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_OBJ_RX_LAST; i++)
+		c_can_inval_msg_object(dev, IFACE_RX, i);
+	for (i = C_CAN_MSG_OBJ_TX_FIRST; i <= C_CAN_MSG_OBJ_TX_LAST; i++)
+		c_can_inval_msg_object(dev, IFACE_TX, i);
 
 	/* 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, 0, i, 0, 0,
+		c_can_setup_receive_object(dev, IFACE_RX, i, 0, 0,
 			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
 
-	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
-			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
+	c_can_setup_receive_object(dev, IFACE_RX, C_CAN_MSG_OBJ_RX_LAST,
+			0, 0, IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
 }
 
 /*
@@ -748,10 +754,10 @@ static void c_can_do_tx(struct net_device *dev)
 			can_get_echo_skb(dev,
 					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
 			stats->tx_bytes += priv->read_reg(priv,
-					C_CAN_IFACE(MSGCTRL_REG, 0))
+					C_CAN_IFACE(MSGCTRL_REG, IFACE_TX))
 					& IF_MCONT_DLC_MASK;
 			stats->tx_packets++;
-			c_can_inval_msg_object(dev, 0, msg_obj_no);
+			c_can_inval_msg_object(dev, IFACE_TX, msg_obj_no);
 		} else {
 			break;
 		}
@@ -801,16 +807,17 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 		 * message object n, we need to handle the same properly.
 		 */
 		if (val & (1 << (msg_obj - 1))) {
-			c_can_object_get(dev, 0, msg_obj, IF_COMM_ALL &
+			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
 					~IF_COMM_TXRQST);
 			msg_ctrl_save = priv->read_reg(priv,
-					C_CAN_IFACE(MSGCTRL_REG, 0));
+					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));
 
 			if (msg_ctrl_save & IF_MCONT_EOB)
 				return num_rx_pkts;
 
 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
-				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
+				c_can_handle_lost_msg_obj(dev, IFACE_RX,
+								msg_obj);
 				num_rx_pkts++;
 				quota--;
 				continue;
@@ -820,19 +827,19 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 				continue;
 
 			/* read the data from the message object */
-			c_can_read_msg_object(dev, 0, msg_ctrl_save);
+			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);
 
 			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
-				c_can_mark_rx_msg_obj(dev, 0,
+				c_can_mark_rx_msg_obj(dev, IFACE_RX,
 						msg_ctrl_save, msg_obj);
 			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
 				/* activate this msg obj */
-				c_can_activate_rx_msg_obj(dev, 0,
+				c_can_activate_rx_msg_obj(dev, IFACE_RX,
 						msg_ctrl_save, msg_obj);
 			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
 				/* activate all lower message objects */
 				c_can_activate_all_lower_rx_msg_obj(dev,
-						0, msg_ctrl_save);
+						IFACE_RX, msg_ctrl_save);
 
 			num_rx_pkts++;
 			quota--;
-- 
1.7.9.5

Re: [RFC v2 3/7] c_can: use different sets of interface registers for rx and tx

[Marc Kleine-Budde]

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

On 11/29/2012 03:39 PM, Wolfgang Grandegger wrote:
> To avoid conflicts between CPU access to the message RAM we now
> use the first set of interface registers for RX and the second
> for TX.
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  drivers/net/can/c_can/c_can.c |   37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 3ae356f..27e45e6 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -158,6 +158,10 @@
>  #define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
>  #define RECEIVE_OBJECT_BITS	0x0000ffff
>  
> +/* message interface used for rx and tx */
> +#define IFACE_RX		0
> +#define IFACE_TX		1

Can you give these defines a common C_CAN_ prefix? And you can make them
an enum and change the signature of the function you're changing below.

Marc

> +
>  /* status interrupt */
>  #define STATUS_INTERRUPT	0x8000
>  
> @@ -544,7 +548,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	msg_obj_no = get_tx_next_msg_obj(priv);
>  
>  	/* prepare message object for transmission */
> -	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> +	c_can_write_msg_object(dev, IFACE_TX, frame, msg_obj_no);
>  	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>  
>  	/*
> @@ -606,16 +610,18 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>  	int i;
>  
>  	/* first invalidate all message objects */
> -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
> -		c_can_inval_msg_object(dev, 0, i);
> +	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_OBJ_RX_LAST; i++)
> +		c_can_inval_msg_object(dev, IFACE_RX, i);
> +	for (i = C_CAN_MSG_OBJ_TX_FIRST; i <= C_CAN_MSG_OBJ_TX_LAST; i++)
> +		c_can_inval_msg_object(dev, IFACE_TX, i);
>  
>  	/* 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, 0, i, 0, 0,
> +		c_can_setup_receive_object(dev, IFACE_RX, i, 0, 0,
>  			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
>  
> -	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> -			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
> +	c_can_setup_receive_object(dev, IFACE_RX, C_CAN_MSG_OBJ_RX_LAST,
> +			0, 0, IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
>  }
>  
>  /*
> @@ -748,10 +754,10 @@ static void c_can_do_tx(struct net_device *dev)
>  			can_get_echo_skb(dev,
>  					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>  			stats->tx_bytes += priv->read_reg(priv,
> -					C_CAN_IFACE(MSGCTRL_REG, 0))
> +					C_CAN_IFACE(MSGCTRL_REG, IFACE_TX))
>  					& IF_MCONT_DLC_MASK;
>  			stats->tx_packets++;
> -			c_can_inval_msg_object(dev, 0, msg_obj_no);
> +			c_can_inval_msg_object(dev, IFACE_TX, msg_obj_no);
>  		} else {
>  			break;
>  		}
> @@ -801,16 +807,17 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  		 * message object n, we need to handle the same properly.
>  		 */
>  		if (val & (1 << (msg_obj - 1))) {
> -			c_can_object_get(dev, 0, msg_obj, IF_COMM_ALL &
> +			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
>  					~IF_COMM_TXRQST);
>  			msg_ctrl_save = priv->read_reg(priv,
> -					C_CAN_IFACE(MSGCTRL_REG, 0));
> +					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));
>  
>  			if (msg_ctrl_save & IF_MCONT_EOB)
>  				return num_rx_pkts;
>  
>  			if (msg_ctrl_save & IF_MCONT_MSGLST) {
> -				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
> +				c_can_handle_lost_msg_obj(dev, IFACE_RX,
> +								msg_obj);
>  				num_rx_pkts++;
>  				quota--;
>  				continue;
> @@ -820,19 +827,19 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  				continue;
>  
>  			/* read the data from the message object */
> -			c_can_read_msg_object(dev, 0, msg_ctrl_save);
> +			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);
>  
>  			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
> -				c_can_mark_rx_msg_obj(dev, 0,
> +				c_can_mark_rx_msg_obj(dev, IFACE_RX,
>  						msg_ctrl_save, msg_obj);
>  			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
>  				/* activate this msg obj */
> -				c_can_activate_rx_msg_obj(dev, 0,
> +				c_can_activate_rx_msg_obj(dev, IFACE_RX,
>  						msg_ctrl_save, msg_obj);
>  			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
>  				/* activate all lower message objects */
>  				c_can_activate_all_lower_rx_msg_obj(dev,
> -						0, msg_ctrl_save);
> +						IFACE_RX, msg_ctrl_save);
>  
>  			num_rx_pkts++;
>  			quota--;
> 


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

Re: [RFC v2 3/7] c_can: use different sets of interface registers for rx and tx

[Wolfgang Grandegger]

On 11/30/2012 09:39 AM, Marc Kleine-Budde wrote:
> On 11/29/2012 03:39 PM, Wolfgang Grandegger wrote:
>> To avoid conflicts between CPU access to the message RAM we now
>> use the first set of interface registers for RX and the second
>> for TX.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  drivers/net/can/c_can/c_can.c |   37 ++++++++++++++++++++++---------------
>>  1 file changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 3ae356f..27e45e6 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -158,6 +158,10 @@
>>  #define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
>>  #define RECEIVE_OBJECT_BITS	0x0000ffff
>>  
>> +/* message interface used for rx and tx */
>> +#define IFACE_RX		0
>> +#define IFACE_TX		1
> 
> Can you give these defines a common C_CAN_ prefix? And you can make them
> an enum and change the signature of the function you're changing below.

OK, will do with the next version. Want to be sure that the driver works
as expected first.

Wolfgang

[RFC v2 4/7] c_can_pci: introduce board specific PCI bar

[Wolfgang Grandegger]

Also fix minor issue with pci_iomap specifying size 0 for
mapping the full range.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can_pci.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 3d7830b..7b73f90 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -31,6 +31,8 @@ struct c_can_pci_data {
 	enum c_can_pci_reg_align reg_align;
 	/* Set the frequency */
 	unsigned int freq;
+	/* PCI bar number */
+	int bar;
 };
 
 /*
@@ -87,7 +89,7 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 	pci_set_master(pdev);
 	pci_enable_msi(pdev);
 
-	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+	addr = pci_iomap(pdev, c_can_pci_data->bar, 0);
 	if (!addr) {
 		dev_err(&pdev->dev,
 			"device has no PCI memory resources, "
@@ -195,6 +197,7 @@ static struct c_can_pci_data c_can_sta2x11= {
 	.type = BOSCH_C_CAN,
 	.reg_align = C_CAN_REG_ALIGN_32,
 	.freq = 52000000, /* 52 Mhz */
+	.bar = 0,
 };
 
 #define C_CAN_ID(_vend, _dev, _driverdata) {		\
-- 
1.7.9.5

Re: [RFC v2 4/7] c_can_pci: introduce board specific PCI bar

[Marc Kleine-Budde]

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

On 11/29/2012 03:39 PM, Wolfgang Grandegger wrote:
> Also fix minor issue with pci_iomap specifying size 0 for
> mapping the full range.

If I understand the code correct, using pci_resource_len(pdev, 0) for
the max len (of BAR 0) instead of 0 isn't a bug, it's just more
complicated than it needs to be. If it really is a bug, I think a
separate patch will be good.

Marc

> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  drivers/net/can/c_can/c_can_pci.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> index 3d7830b..7b73f90 100644
> --- a/drivers/net/can/c_can/c_can_pci.c
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -31,6 +31,8 @@ struct c_can_pci_data {
>  	enum c_can_pci_reg_align reg_align;
>  	/* Set the frequency */
>  	unsigned int freq;
> +	/* PCI bar number */
> +	int bar;
>  };
>  
>  /*
> @@ -87,7 +89,7 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
>  	pci_set_master(pdev);
>  	pci_enable_msi(pdev);
>  
> -	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> +	addr = pci_iomap(pdev, c_can_pci_data->bar, 0);
>  	if (!addr) {
>  		dev_err(&pdev->dev,
>  			"device has no PCI memory resources, "
> @@ -195,6 +197,7 @@ static struct c_can_pci_data c_can_sta2x11= {
>  	.type = BOSCH_C_CAN,
>  	.reg_align = C_CAN_REG_ALIGN_32,
>  	.freq = 52000000, /* 52 Mhz */
> +	.bar = 0,
>  };
>  
>  #define C_CAN_ID(_vend, _dev, _driverdata) {		\
> 


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

Re: [RFC v2 4/7] c_can_pci: introduce board specific PCI bar

[Wolfgang Grandegger]

On 11/30/2012 09:45 AM, Marc Kleine-Budde wrote:
> On 11/29/2012 03:39 PM, Wolfgang Grandegger wrote:
>> Also fix minor issue with pci_iomap specifying size 0 for
>> mapping the full range.
> 
> If I understand the code correct, using pci_resource_len(pdev, 0) for
> the max len (of BAR 0) instead of 0 isn't a bug, it's just more
> complicated than it needs to be. If it really is a bug, I think a
> separate patch will be good.

It's a minor issue, not a bug. Both do the same thing. See:

http://lxr.linux.no/#linux+v3.6.8/lib/pci_iomap.c#L23

Wolfgang.

Re: [RFC v2 4/7] c_can_pci: introduce board specific PCI bar

[Marc Kleine-Budde]

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

On 11/30/2012 10:11 AM, Wolfgang Grandegger wrote:
> On 11/30/2012 09:45 AM, Marc Kleine-Budde wrote:
>> On 11/29/2012 03:39 PM, Wolfgang Grandegger wrote:
>>> Also fix minor issue with pci_iomap specifying size 0 for
>>> mapping the full range.
>>
>> If I understand the code correct, using pci_resource_len(pdev, 0) for
>> the max len (of BAR 0) instead of 0 isn't a bug, it's just more
>> complicated than it needs to be. If it really is a bug, I think a
>> separate patch will be good.
> 
> It's a minor issue, not a bug. Both do the same thing. See:
> 
> http://lxr.linux.no/#linux+v3.6.8/lib/pci_iomap.c#L23

I've looked at the code, too. Just wanted to be sure :)

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

[RFC v2 5/7] c_can_pci: enable PCI bus master only for MSI

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can_pci.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 7b73f90..320e371 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -86,8 +86,11 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 		goto out_disable_device;
 	}
 
-	pci_set_master(pdev);
-	pci_enable_msi(pdev);
+
+	if (!pci_enable_msi(pdev)) {
+		dev_info(&pdev->dev, "MSI enabled\n");
+		pci_set_master(pdev);
+	}
 
 	addr = pci_iomap(pdev, c_can_pci_data->bar, 0);
 	if (!addr) {
-- 
1.7.9.5

Re: [RFC v2 5/7] c_can_pci: enable PCI bus master only for MSI

[Marc Kleine-Budde]

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

On 11/29/2012 03:39 PM, Wolfgang Grandegger wrote:
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  drivers/net/can/c_can/c_can_pci.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> index 7b73f90..320e371 100644
> --- a/drivers/net/can/c_can/c_can_pci.c
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -86,8 +86,11 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
>  		goto out_disable_device;
>  	}
>  
> -	pci_set_master(pdev);
> -	pci_enable_msi(pdev);
> +
> +	if (!pci_enable_msi(pdev)) {

Can you please use an intermediate variable "ret".

ret = pci_enable_msi(pdev);
if (!ret)  {
}

> +		dev_info(&pdev->dev, "MSI enabled\n");
> +		pci_set_master(pdev);
> +	}
>  
>  	addr = pci_iomap(pdev, c_can_pci_data->bar, 0);
>  	if (!addr) {
> 

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

[RFC v2 6/7] c_can_pci: add support for PCH CAN on Intel EG20T PCH

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can_pci.c |   46 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 320e371..2516ea9 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -19,9 +19,13 @@
 
 #include "c_can.h"
 
+#define PCI_DEVICE_ID_PCH_CAN	0x8818
+#define PCH_PCI_SOFT_RESET	0x01fc
+
 enum c_can_pci_reg_align {
 	C_CAN_REG_ALIGN_16,
 	C_CAN_REG_ALIGN_32,
+	C_CAN_REG_32,
 };
 
 struct c_can_pci_data {
@@ -33,6 +37,8 @@ struct c_can_pci_data {
 	unsigned int freq;
 	/* PCI bar number */
 	int bar;
+	/* Callback for reset */
+	void (*init) (const struct c_can_priv *priv, bool enable);
 };
 
 /*
@@ -65,6 +71,29 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
 	writew(val, priv->base + 2 * priv->regs[index]);
 }
 
+static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
+				    enum reg index)
+{
+	return (u16)ioread32(priv->base + 2 * priv->regs[index]);
+}
+
+static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
+				      enum reg index, u16 val)
+{
+	iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
+}
+
+static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)
+{
+	if (enable) {
+		u32 __iomem *addr = priv->base + PCH_PCI_SOFT_RESET;
+
+		/* write to sw reset register */
+		iowrite32(1, addr);
+		iowrite32(0, addr);
+	}
+}
+
 static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 				     const struct pci_device_id *ent)
 {
@@ -86,7 +115,6 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 		goto out_disable_device;
 	}
 
-
 	if (!pci_enable_msi(pdev)) {
 		dev_info(&pdev->dev, "MSI enabled\n");
 		pci_set_master(pdev);
@@ -147,11 +175,17 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
 		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
 		break;
+	case C_CAN_REG_32:
+		priv->read_reg = c_can_pci_read_reg_32bit;
+		priv->write_reg = c_can_pci_write_reg_32bit;
+		break;
 	default:
 		ret = -EINVAL;
 		goto out_free_c_can;
 	}
 
+	priv->init = c_can_pci_data->init;
+
 	ret = register_c_can_dev(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
@@ -203,6 +237,14 @@ static struct c_can_pci_data c_can_sta2x11= {
 	.bar = 0,
 };
 
+static struct c_can_pci_data c_can_pch = {
+	.type = BOSCH_C_CAN,
+	.reg_align = C_CAN_REG_32,
+	.freq = 50000000, /* 50 MHz */
+	.init = c_can_pci_reset_pch,
+	.bar = 1,
+};
+
 #define C_CAN_ID(_vend, _dev, _driverdata) {		\
 	PCI_DEVICE(_vend, _dev),			\
 	.driver_data = (unsigned long)&_driverdata,	\
@@ -210,6 +252,8 @@ static struct c_can_pci_data c_can_sta2x11= {
 static DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
 	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
 		 c_can_sta2x11),
+	C_CAN_ID(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH_CAN,
+		 c_can_pch),
 	{},
 };
 static struct pci_driver c_can_pci_driver = {
-- 
1.7.9.5

[RFC v2 7/7] c_can: add spinlock to protect tx and rx objects

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can.c |   13 +++++++++++++
 drivers/net/can/c_can/c_can.h |    1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 27e45e6..b024665 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,7 @@
 #include <linux/if_ether.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/can.h>
@@ -541,10 +542,13 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	u32 msg_obj_no;
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct can_frame *frame = (struct can_frame *)skb->data;
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
 
+	spin_lock_irqsave(&priv->lock, flags);
+
 	msg_obj_no = get_tx_next_msg_obj(priv);
 
 	/* prepare message object for transmission */
@@ -560,6 +564,8 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
 		netif_stop_queue(dev);
 
+	spin_unlock_irqrestore(&priv->lock, flags);
+
 	return NETDEV_TX_OK;
 }
 
@@ -746,6 +752,9 @@ static void c_can_do_tx(struct net_device *dev)
 	u32 msg_obj_no;
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->lock, flags);
 
 	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
 		msg_obj_no = get_tx_echo_msg_obj(priv);
@@ -767,6 +776,8 @@ static void c_can_do_tx(struct net_device *dev)
 	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);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 /*
@@ -1173,6 +1184,8 @@ struct net_device *alloc_c_can_dev(void)
 					CAN_CTRLMODE_LISTENONLY |
 					CAN_CTRLMODE_BERR_REPORTING;
 
+	spin_lock_init(&priv->lock);
+
 	return dev;
 }
 EXPORT_SYMBOL_GPL(alloc_c_can_dev);
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 4baf3f6..3487d5e 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -172,6 +172,7 @@ struct c_can_priv {
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
 	void (*init) (const struct c_can_priv *priv, bool enable);
+	spinlock_t lock;	/* to protect tx and rx message objects */
 };
 
 struct net_device *alloc_c_can_dev(void);
-- 
1.7.9.5

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Alexander Stein]

Hello Wolfgang and others,

On Thursday 29 November 2012 15:39:40, Wolfgang Grandegger wrote:
> here is v2 of my patches for the C_CAN drivers.
> 
> For Michael I have prepared out-of-tree driver sources allowing to
> easily build the drivers also for older 3.x kernel versions. More
> tester are welcome.
> 
> Changes since v1:
> 
> - use init callback after renaming it from initram
> - use different sets of interface registers for rx and tx (like PCH_CAN)
> - use spin_[un]lock_bh to protect the tx objects
> 
> Wolfgang Grandegger (7):
>   pch_can: add spinlocks to protect tx objects
>   c_can: rename callback "initram" to "init" to more general usage
>   c_can: use different sets of interface registers for rx and tx
>   c_can_pci: introduce board specific PCI bar
>   c_can_pci: enable PCI bus master only for MSI
>   c_can_pci: add support for PCH CAN on Intel EG20T PCH
>   c_can: add spinlock to protect tx and  objects
> 
>  drivers/net/can/c_can/c_can.c          |   66 
+++++++++++++++++++++-----------
>  drivers/net/can/c_can/c_can.h          |    3 +-
>  drivers/net/can/c_can/c_can_pci.c      |   56 +++++++++++++++++++++++++--
>  drivers/net/can/c_can/c_can_platform.c |    2 +-
>  drivers/net/can/pch_can.c              |    9 +++++
>  5 files changed, 108 insertions(+), 28 deletions(-)

I backported the c_can patches incl. your patchset to v3.0.31 and tested this 
driver on our own custom atom board using the eg20t pch. CAN in general works, 
but if I run my heavy CAN load testcase I get errors sometimes.
This test works as follows: I send a CAN message to 2 other CAN nodes 
configuring some timings (like burst length or time between each can frame) 
and they send 250000 messages each containing a counter. This way I can detect 
any missing or switched message with a high bus load.
If I use the described software state alone it works, but if I run 'watch 
sensors' in a different ssh session, CAN start to misbehave like missing CAN 
frames or switched order. It seems that I2C usage on the PCH influences the 
CAN part also:(
Even worse, if I use the following patch to check if PCI writes were 
successfully, I notices that some writes (or the consecutive read) don't 
succeed. And I also get lots of I2C timeouts waiting for a xfer complete.

Does anybody have an idea what's going wrong here? Is somebody able to see the 
same problems on their hardware?

Wolfgang: Compared to your v8 c_can drivers for Michael, I'm just missing the 
const bitrate table and don't use pci_register_driver, as I have cherry-picked 
the module_pci_driver patches.

Best regards,
Alexander

diff --git a/drivers/net/can/c_can/c_can_pci.c 
b/drivers/net/can/c_can/c_can_pci.c
index 2516ea9..b124ea5 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -80,7 +80,13 @@ static u16 c_can_pci_read_reg_32bit(struct c_can_priv 
*priv,
 static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
                                      enum reg index, u16 val)
 {
+       u16 reg;
        iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
+       reg = c_can_pci_read_reg_32bit(priv, index);
+       if (reg != val)
+       {
+               netdev_err(priv->dev, "write 0x%x to offset 0x%x failed. got: 
0x%x\n", val, 2 * priv->regs[index], reg);
+       }
 }
 
 static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

Hi Alexander,

thanks for testing!. Maybe we deal with more than one problem.

On 12/05/2012 01:09 PM, Alexander Stein wrote:
> Hello Wolfgang and others,
> 
> On Thursday 29 November 2012 15:39:40, Wolfgang Grandegger wrote:
>> here is v2 of my patches for the C_CAN drivers.
>>
>> For Michael I have prepared out-of-tree driver sources allowing to
>> easily build the drivers also for older 3.x kernel versions. More
>> tester are welcome.
>>
>> Changes since v1:
>>
>> - use init callback after renaming it from initram
>> - use different sets of interface registers for rx and tx (like PCH_CAN)
>> - use spin_[un]lock_bh to protect the tx objects
>>
>> Wolfgang Grandegger (7):
>>   pch_can: add spinlocks to protect tx objects
>>   c_can: rename callback "initram" to "init" to more general usage
>>   c_can: use different sets of interface registers for rx and tx
>>   c_can_pci: introduce board specific PCI bar
>>   c_can_pci: enable PCI bus master only for MSI
>>   c_can_pci: add support for PCH CAN on Intel EG20T PCH
>>   c_can: add spinlock to protect tx and  objects
>>
>>  drivers/net/can/c_can/c_can.c          |   66 
> +++++++++++++++++++++-----------
>>  drivers/net/can/c_can/c_can.h          |    3 +-
>>  drivers/net/can/c_can/c_can_pci.c      |   56 +++++++++++++++++++++++++--
>>  drivers/net/can/c_can/c_can_platform.c |    2 +-
>>  drivers/net/can/pch_can.c              |    9 +++++
>>  5 files changed, 108 insertions(+), 28 deletions(-)
> 
> I backported the c_can patches incl. your patchset to v3.0.31 and tested this 
> driver on our own custom atom board using the eg20t pch. CAN in general works, 

The out-of-tree archive may have worked for you as well.

A few general questions to understand your hardware and setup:

- Is this a multi-processor system (SMP)? If not, you may not run into
  tx-not-working-any-more problem. Have you ever realized it?

- Did you see the problems below with the old PCH_CAN driver as well.

- Do the problems show up with the still existing PCH_CAN driver
  (including the "pch_can: add spinlocks to protect tx objects" patch)?

> but if I run my heavy CAN load testcase I get errors sometimes.
> This test works as follows: I send a CAN message to 2 other CAN nodes 
> configuring some timings (like burst length or time between each can frame) 
> and they send 250000 messages each containing a counter. This way I can detect 
> any missing or switched message with a high bus load.
> If I use the described software state alone it works, but if I run 'watch 
> sensors' in a different ssh session, CAN start to misbehave like missing CAN 
> frames or switched order. It seems that I2C usage on the PCH influences the 
> CAN part also:

- When your app sends/writes messages, does it check for errno==ENOBUFS?

- The messages look still ok (not currupted, I mean)?

> Even worse, if I use the following patch to check if PCI writes were 
> successfully, I notices that some writes (or the consecutive read) don't 
> succeed. And I also get lots of I2C timeouts waiting for a xfer complete.

Be careful, there might be some registers changing their values after
writing. Can you show the value read after writing and the register
offset? The influence on the I2C bus looks more like an overload or
hardware problem. What is your CAN interrupt rate?
Do you see this problem with the old PCH_CAN driver as well?

> Does anybody have an idea what's going wrong here? Is somebody able to see the 
> same problems on their hardware?

At least you are the first one reporting this problem.

> Wolfgang: Compared to your v8 c_can drivers for Michael, I'm just missing the 
> const bitrate table and don't use pci_register_driver, as I have cherry-picked 
> the module_pci_driver patches.

What do you mean with "missing the const bitrate table"?

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Alexander Stein]

Hello Wolfgang,

On Wednesday 05 December 2012 13:50:46, Wolfgang Grandegger wrote:
> Hi Alexander,
> 
> thanks for testing!. Maybe we deal with more than one problem.
> 
> On 12/05/2012 01:09 PM, Alexander Stein wrote:
> > Hello Wolfgang and others,
> > 
> > On Thursday 29 November 2012 15:39:40, Wolfgang Grandegger wrote:
> >> here is v2 of my patches for the C_CAN drivers.
> >>
> >> For Michael I have prepared out-of-tree driver sources allowing to
> >> easily build the drivers also for older 3.x kernel versions. More
> >> tester are welcome.
> >>
> >> Changes since v1:
> >>
> >> - use init callback after renaming it from initram
> >> - use different sets of interface registers for rx and tx (like PCH_CAN)
> >> - use spin_[un]lock_bh to protect the tx objects
> >>
> >> Wolfgang Grandegger (7):
> >>   pch_can: add spinlocks to protect tx objects
> >>   c_can: rename callback "initram" to "init" to more general usage
> >>   c_can: use different sets of interface registers for rx and tx
> >>   c_can_pci: introduce board specific PCI bar
> >>   c_can_pci: enable PCI bus master only for MSI
> >>   c_can_pci: add support for PCH CAN on Intel EG20T PCH
> >>   c_can: add spinlock to protect tx and  objects
> >>
> >>  drivers/net/can/c_can/c_can.c          |   66 
> > +++++++++++++++++++++-----------
> >>  drivers/net/can/c_can/c_can.h          |    3 +-
> >>  drivers/net/can/c_can/c_can_pci.c      |   56 
+++++++++++++++++++++++++--
> >>  drivers/net/can/c_can/c_can_platform.c |    2 +-
> >>  drivers/net/can/pch_can.c              |    9 +++++
> >>  5 files changed, 108 insertions(+), 28 deletions(-)
> > 
> > I backported the c_can patches incl. your patchset to v3.0.31 and tested 
this 
> > driver on our own custom atom board using the eg20t pch. CAN in general 
works, 
> 
> The out-of-tree archive may have worked for you as well.
> 
> A few general questions to understand your hardware and setup:
> 
> - Is this a multi-processor system (SMP)? If not, you may not run into
>   tx-not-working-any-more problem. Have you ever realized it?

This is a Intel E660 single core CPU with HT, so it is a SMP system. I'm 
currently not aware that tx is not working anymore.

> - Did you see the problems below with the old PCH_CAN driver as well.
> 
> - Do the problems show up with the still existing PCH_CAN driver
>   (including the "pch_can: add spinlocks to protect tx objects" patch)?

With the current version of pch_can from Linuxs' tree and the named patch I 
get at least some messaged twice.

> > but if I run my heavy CAN load testcase I get errors sometimes.
> > This test works as follows: I send a CAN message to 2 other CAN nodes 
> > configuring some timings (like burst length or time between each can 
frame) 
> > and they send 250000 messages each containing a counter. This way I can 
detect 
> > any missing or switched message with a high bus load.
> > If I use the described software state alone it works, but if I run 'watch 
> > sensors' in a different ssh session, CAN start to misbehave like missing 
CAN 
> > frames or switched order. It seems that I2C usage on the PCH influences 
the 
> > CAN part also:
> 
> - When your app sends/writes messages, does it check for errno==ENOBUFS?

My test application sends only 1 message each test run to start the other 
nodes. It checks ENOBUFS and returns an error in that case. Though I've never 
seen that.

> - The messages look still ok (not currupted, I mean)?

The received frames all look good (despite wrong counter sometimes due to 
wrong order or lost frames).

> > Even worse, if I use the following patch to check if PCI writes were 
> > successfully, I notices that some writes (or the consecutive read) don't 
> > succeed. And I also get lots of I2C timeouts waiting for a xfer complete.
> 
> Be careful, there might be some registers changing their values after
> writing. Can you show the value read after writing and the register
> offset? The influence on the I2C bus looks more like an overload or
> hardware problem. What is your CAN interrupt rate?

I get about 33 interrupts per second on i2c. On a successful run I get 366886 
interrupts for 500000 messages with the c_can driver.

Here are some failed writes to the CAN controller.
[   50.445695] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x4 failed. 
got: 0x10
[   51.043027] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
got: 0x0
[... repeats several times]
[   64.046031] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
got: 0x0
[   64.458286] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. 
got: 0xb8
[   64.811025] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
got: 0x0
and the last one is repeated all the time.

Some times I also get the 16 of the following message:
c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x2c failed. got: 0x2000

> Do you see this problem with the old PCH_CAN driver as well?

With pch_can I get about 254000 interrupts for about 283000 frames.

[ 2422.198378] regs base: f8f5a000
[ 2449.197911] pch_can_bit_clear: clear bit failed: addr: f8f5a038, reg1: 
0x1404, reg2: 0x8, mask 0xa000
[ 2458.302028] pch_can_bit_set: set bit failed: addr: f8f5a000, reg1: 0x80, 
reg2: 0x80, mask 0xe

On the second line you can see that the register isn't written at all (or the 
read failed for some reason).

> > Does anybody have an idea what's going wrong here? Is somebody able to see 
the 
> > same problems on their hardware?
> 
> At least you are the first one reporting this problem.
> 
> > Wolfgang: Compared to your v8 c_can drivers for Michael, I'm just missing 
the 
> > const bitrate table and don't use pci_register_driver, as I have cherry-
picked 
> > the module_pci_driver patches.
> 
> What do you mean with "missing the const bitrate table"?

I do not have cherry-picked commit 194b9a4cb91713ddb60c9f98f7212f6d8cb8e05f.

Best regards,
Alexander

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/05/2012 03:46 PM, Alexander Stein wrote:
> Hello Wolfgang,
> 
> On Wednesday 05 December 2012 13:50:46, Wolfgang Grandegger wrote:
>> Hi Alexander,
>>
>> thanks for testing!. Maybe we deal with more than one problem.
>>
...
>> A few general questions to understand your hardware and setup:
>>
>> - Is this a multi-processor system (SMP)? If not, you may not run into
>>   tx-not-working-any-more problem. Have you ever realized it?
> 
> This is a Intel E660 single core CPU with HT, so it is a SMP system. I'm 
> currently not aware that tx is not working anymore.

OK, your send rate is very low and therefore it's unlikely that you hit
that problem.

>> - Did you see the problems below with the old PCH_CAN driver as well.
>>
>> - Do the problems show up with the still existing PCH_CAN driver
>>   (including the "pch_can: add spinlocks to protect tx objects" patch)?
> 
> With the current version of pch_can from Linuxs' tree and the named patch I 
> get at least some messaged twice.

OK, sounds better but also not good.

>>> but if I run my heavy CAN load testcase I get errors sometimes.
>>> This test works as follows: I send a CAN message to 2 other CAN nodes 
>>> configuring some timings (like burst length or time between each can 
> frame) 
>>> and they send 250000 messages each containing a counter. This way I can 
> detect 
>>> any missing or switched message with a high bus load.
>>> If I use the described software state alone it works, but if I run 'watch 
>>> sensors' in a different ssh session, CAN start to misbehave like missing 
> CAN 
>>> frames or switched order. It seems that I2C usage on the PCH influences 
> the 
>>> CAN part also:
>>
>> - When your app sends/writes messages, does it check for errno==ENOBUFS?
> 
> My test application sends only 1 message each test run to start the other 
> nodes. It checks ENOBUFS and returns an error in that case. Though I've never 
> seen that.

OK, your TX rate it low.

> 
>> - The messages look still ok (not currupted, I mean)?
> 
> The received frames all look good (despite wrong counter sometimes due to 
> wrong order or lost frames).
> 
>>> Even worse, if I use the following patch to check if PCI writes were 
>>> successfully, I notices that some writes (or the consecutive read) don't 
>>> succeed. And I also get lots of I2C timeouts waiting for a xfer complete.
>>
>> Be careful, there might be some registers changing their values after
>> writing. Can you show the value read after writing and the register
>> offset? The influence on the I2C bus looks more like an overload or
>> hardware problem. What is your CAN interrupt rate?
> 
> I get about 33 interrupts per second on i2c. On a successful run I get 366886 
> interrupts for 500000 messages with the c_can driver.

In what time? Is the CAN bus highly loaded.

> Here are some failed writes to the CAN controller.
> [   50.445695] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x4 failed. 
> got: 0x10
> [   51.043027] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
> got: 0x0
> [... repeats several times]
> [   64.046031] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
> got: 0x0
> [   64.458286] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. 
> got: 0xb8
> [   64.811025] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
> got: 0x0
> and the last one is repeated all the time.

That's wired! Writing 0xe to offset 0x0 does re-enable the interrupts at
the end of poll-rx. Disabling the interrupts in the isr does not show
that symptoms. Strange.

> Some times I also get the 16 of the following message:
> c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x2c failed. got: 0x2000


>> Do you see this problem with the old PCH_CAN driver as well?
> 
> With pch_can I get about 254000 interrupts for about 283000 frames.
> 
> [ 2422.198378] regs base: f8f5a000
> [ 2449.197911] pch_can_bit_clear: clear bit failed: addr: f8f5a038, reg1: 
> 0x1404, reg2: 0x8, mask 0xa000
> [ 2458.302028] pch_can_bit_set: set bit failed: addr: f8f5a000, reg1: 0x80, 
> reg2: 0x80, mask 0xe

That's the same thing as with the c_can driver when the interrupts gets
re-enabled.

> On the second line you can see that the register isn't written at all (or the 
> read failed for some reason).

I assume the latter. Could you please retry reading the register until
the correct value shows up. With some timeout, of course.

I checked the PCH ethernet driver and did not find anythings special
accessing registers. Maybe Tomoya has an idea or could tell use somebody
at OKI/LAPIS Semiconductor who could help. Tomoya?

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/05/2012 06:35 PM, Wolfgang Grandegger wrote:
> On 12/05/2012 03:46 PM, Alexander Stein wrote:
>> Hello Wolfgang,
>>
>> On Wednesday 05 December 2012 13:50:46, Wolfgang Grandegger wrote:
>>> Hi Alexander,
>>>
>>> thanks for testing!. Maybe we deal with more than one problem.
>>>
> ...
>>> A few general questions to understand your hardware and setup:
>>>
>>> - Is this a multi-processor system (SMP)? If not, you may not run into
>>>   tx-not-working-any-more problem. Have you ever realized it?
>>
>> This is a Intel E660 single core CPU with HT, so it is a SMP system. I'm 
>> currently not aware that tx is not working anymore.
> 
> OK, your send rate is very low and therefore it's unlikely that you hit
> that problem.
> 
>>> - Did you see the problems below with the old PCH_CAN driver as well.
>>>
>>> - Do the problems show up with the still existing PCH_CAN driver
>>>   (including the "pch_can: add spinlocks to protect tx objects" patch)?
>>
>> With the current version of pch_can from Linuxs' tree and the named patch I 
>> get at least some messaged twice.
> 
> OK, sounds better but also not good.
> 
>>>> but if I run my heavy CAN load testcase I get errors sometimes.
>>>> This test works as follows: I send a CAN message to 2 other CAN nodes 
>>>> configuring some timings (like burst length or time between each can 
>> frame) 
>>>> and they send 250000 messages each containing a counter. This way I can 
>> detect 
>>>> any missing or switched message with a high bus load.
>>>> If I use the described software state alone it works, but if I run 'watch 
>>>> sensors' in a different ssh session, CAN start to misbehave like missing 
>> CAN 
>>>> frames or switched order. It seems that I2C usage on the PCH influences 
>> the 
>>>> CAN part also:
>>>
>>> - When your app sends/writes messages, does it check for errno==ENOBUFS?
>>
>> My test application sends only 1 message each test run to start the other 
>> nodes. It checks ENOBUFS and returns an error in that case. Though I've never 
>> seen that.
> 
> OK, your TX rate it low.
> 
>>
>>> - The messages look still ok (not currupted, I mean)?
>>
>> The received frames all look good (despite wrong counter sometimes due to 
>> wrong order or lost frames).
>>
>>>> Even worse, if I use the following patch to check if PCI writes were 
>>>> successfully, I notices that some writes (or the consecutive read) don't 
>>>> succeed. And I also get lots of I2C timeouts waiting for a xfer complete.
>>>
>>> Be careful, there might be some registers changing their values after
>>> writing. Can you show the value read after writing and the register
>>> offset? The influence on the I2C bus looks more like an overload or
>>> hardware problem. What is your CAN interrupt rate?
>>
>> I get about 33 interrupts per second on i2c. On a successful run I get 366886 
>> interrupts for 500000 messages with the c_can driver.
> 
> In what time? Is the CAN bus highly loaded.
> 
>> Here are some failed writes to the CAN controller.
>> [   50.445695] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x4 failed. 
>> got: 0x10
>> [   51.043027] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
>> got: 0x0
>> [... repeats several times]
>> [   64.046031] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
>> got: 0x0
>> [   64.458286] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. 
>> got: 0xb8
>> [   64.811025] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
>> got: 0x0
>> and the last one is repeated all the time.
> 
> That's wired! Writing 0xe to offset 0x0 does re-enable the interrupts at
> the end of poll-rx. Disabling the interrupts in the isr does not show
> that symptoms. Strange.

The write+read check is racy. The interrupt handler might disable the
interrupts again.

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

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/05/2012 10:52 PM, Marc Kleine-Budde wrote:
> On 12/05/2012 06:35 PM, Wolfgang Grandegger wrote:
>> On 12/05/2012 03:46 PM, Alexander Stein wrote:
>>> Hello Wolfgang,
>>>
>>> On Wednesday 05 December 2012 13:50:46, Wolfgang Grandegger wrote:
>>>> Hi Alexander,
>>>>
>>>> thanks for testing!. Maybe we deal with more than one problem.
>>>>
>> ...
>>>> A few general questions to understand your hardware and setup:
>>>>
>>>> - Is this a multi-processor system (SMP)? If not, you may not run into
>>>>   tx-not-working-any-more problem. Have you ever realized it?
>>>
>>> This is a Intel E660 single core CPU with HT, so it is a SMP system. I'm 
>>> currently not aware that tx is not working anymore.
>>
>> OK, your send rate is very low and therefore it's unlikely that you hit
>> that problem.
>>
>>>> - Did you see the problems below with the old PCH_CAN driver as well.
>>>>
>>>> - Do the problems show up with the still existing PCH_CAN driver
>>>>   (including the "pch_can: add spinlocks to protect tx objects" patch)?
>>>
>>> With the current version of pch_can from Linuxs' tree and the named patch I 
>>> get at least some messaged twice.
>>
>> OK, sounds better but also not good.
>>
>>>>> but if I run my heavy CAN load testcase I get errors sometimes.
>>>>> This test works as follows: I send a CAN message to 2 other CAN nodes 
>>>>> configuring some timings (like burst length or time between each can 
>>> frame) 
>>>>> and they send 250000 messages each containing a counter. This way I can 
>>> detect 
>>>>> any missing or switched message with a high bus load.
>>>>> If I use the described software state alone it works, but if I run 'watch 
>>>>> sensors' in a different ssh session, CAN start to misbehave like missing 
>>> CAN 
>>>>> frames or switched order. It seems that I2C usage on the PCH influences 
>>> the 
>>>>> CAN part also:
>>>>
>>>> - When your app sends/writes messages, does it check for errno==ENOBUFS?
>>>
>>> My test application sends only 1 message each test run to start the other 
>>> nodes. It checks ENOBUFS and returns an error in that case. Though I've never 
>>> seen that.
>>
>> OK, your TX rate it low.
>>
>>>
>>>> - The messages look still ok (not currupted, I mean)?
>>>
>>> The received frames all look good (despite wrong counter sometimes due to 
>>> wrong order or lost frames).
>>>
>>>>> Even worse, if I use the following patch to check if PCI writes were 
>>>>> successfully, I notices that some writes (or the consecutive read) don't 
>>>>> succeed. And I also get lots of I2C timeouts waiting for a xfer complete.
>>>>
>>>> Be careful, there might be some registers changing their values after
>>>> writing. Can you show the value read after writing and the register
>>>> offset? The influence on the I2C bus looks more like an overload or
>>>> hardware problem. What is your CAN interrupt rate?
>>>
>>> I get about 33 interrupts per second on i2c. On a successful run I get 366886 
>>> interrupts for 500000 messages with the c_can driver.
>>
>> In what time? Is the CAN bus highly loaded.
>>
>>> Here are some failed writes to the CAN controller.
>>> [   50.445695] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x4 failed. 
>>> got: 0x10
>>> [   51.043027] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
>>> got: 0x0
>>> [... repeats several times]
>>> [   64.046031] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
>>> got: 0x0
>>> [   64.458286] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. 
>>> got: 0xb8
>>> [   64.811025] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. 
>>> got: 0x0
>>> and the last one is repeated all the time.
>>
>> That's wired! Writing 0xe to offset 0x0 does re-enable the interrupts at
>> the end of poll-rx. Disabling the interrupts in the isr does not show
>> that symptoms. Strange.
> 
> The write+read check is racy. The interrupt handler might disable the
> interrupts again.

Ah, yes, of course. Nothing to worry about then. Sorry for the noise.

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/06/2012 08:09 AM, Wolfgang Grandegger wrote:
>>> That's wired! Writing 0xe to offset 0x0 does re-enable the interrupts at
>>> the end of poll-rx. Disabling the interrupts in the isr does not show
>>> that symptoms. Strange.
>>
>> The write+read check is racy. The interrupt handler might disable the
>> interrupts again.
> 
> Ah, yes, of course. Nothing to worry about then. Sorry for the noise.

However we can add a spinlock around the write+read and the read
function itself for debugging purpose.

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

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/05/2012 06:35 PM, Wolfgang Grandegger wrote:
> On 12/05/2012 03:46 PM, Alexander Stein wrote:
>> Hello Wolfgang,
>>
>> On Wednesday 05 December 2012 13:50:46, Wolfgang Grandegger wrote:
>>> Hi Alexander,
>>>
>>> thanks for testing!. Maybe we deal with more than one problem.
>>>
> ...
>>> A few general questions to understand your hardware and setup:
>>>
>>> - Is this a multi-processor system (SMP)? If not, you may not run into
>>>   tx-not-working-any-more problem. Have you ever realized it?
>>
>> This is a Intel E660 single core CPU with HT, so it is a SMP system. I'm 
>> currently not aware that tx is not working anymore.
> 
> OK, your send rate is very low and therefore it's unlikely that you hit
> that problem.
> 
>>> - Did you see the problems below with the old PCH_CAN driver as well.
>>>
>>> - Do the problems show up with the still existing PCH_CAN driver
>>>   (including the "pch_can: add spinlocks to protect tx objects" patch)?
>>
>> With the current version of pch_can from Linuxs' tree and the named patch I 
>> get at least some messaged twice.
> 
> OK, sounds better but also not good.
> 
>>>> but if I run my heavy CAN load testcase I get errors sometimes.
>>>> This test works as follows: I send a CAN message to 2 other CAN nodes 
>>>> configuring some timings (like burst length or time between each can 
>> frame) 
>>>> and they send 250000 messages each containing a counter. This way I can 
>> detect 
>>>> any missing or switched message with a high bus load.
>>>> If I use the described software state alone it works, but if I run 'watch 
>>>> sensors' in a different ssh session, CAN start to misbehave like missing 
>> CAN 
>>>> frames or switched order. It seems that I2C usage on the PCH influences 
>> the 
>>>> CAN part also:
>>>
>>> - When your app sends/writes messages, does it check for errno==ENOBUFS?
>>
>> My test application sends only 1 message each test run to start the other 
>> nodes. It checks ENOBUFS and returns an error in that case. Though I've never 
>> seen that.
> 
> OK, your TX rate it low.
> 
>>
>>> - The messages look still ok (not currupted, I mean)?
>>
>> The received frames all look good (despite wrong counter sometimes due to 
>> wrong order or lost frames).

Could you show use the sequence of the lost, duplicated and out-of-order
messages in the format:

received-sequence number: sent-sequence-number-in-the-can-data

Maybe we can see a pattern.

Thanks,

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Alexander Stein]

Hello Wolfgang,

On Wednesday 05 December 2012 18:35:25, Wolfgang Grandegger wrote:
> >> - The messages look still ok (not currupted, I mean)?
> > 
> > The received frames all look good (despite wrong counter sometimes due to 
> > wrong order or lost frames).
> > 
> >>> Even worse, if I use the following patch to check if PCI writes were 
> >>> successfully, I notices that some writes (or the consecutive read) don't 
> >>> succeed. And I also get lots of I2C timeouts waiting for a xfer complete.
> >>
> >> Be careful, there might be some registers changing their values after
> >> writing. Can you show the value read after writing and the register
> >> offset? The influence on the I2C bus looks more like an overload or
> >> hardware problem. What is your CAN interrupt rate?
> > 
> > I get about 33 interrupts per second on i2c. On a successful run I get 366886 
> > interrupts for 500000 messages with the c_can driver.
> 
> In what time? Is the CAN bus highly loaded.

Busload is about 14-18% according to canbusload with 1MBit. A complete sucessful run takes about 5 minutes.

> > On the second line you can see that the register isn't written at all (or the 
> > read failed for some reason).
> 
> I assume the latter. Could you please retry reading the register until
> the correct value shows up. With some timeout, of course.

I notices having all error events printed on serial console using pch_uart driver has negative effect (I guess one problem causes another one), so I setup 'dmesg -n1' to reduce serial load before the test.
Running the test with just one heartbeat triggered LED set using I2C the test runs without errors. I only see
Lots of 'c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x2c failed. got: 0x2000' at the beginning. It seems this (reserved?) bit is always 1 no matter what we write.
But things start to break when running the test while running 'watch sensors' (sensors queries several temperature sensors via I2C) in a seconds ssh session.
First off the driver error messages (omitting the messages as written above):
[  384.466217] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  384.466630] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  394.754389] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xac
[  394.754532] c_can_pci 0000:02:0c.3: can0: write 0x6 to offset 0x20 failed. got: 0x20
[  394.754798] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x98
[  429.851053] c_can_pci 0000:02:0c.3: can0: write 0x5 to offset 0x20 failed. got: 0x8
[  440.129098] c_can_pci 0000:02:0c.3: can0: write 0x9404 to offset 0x38 failed. got: 0x8
[  440.129496] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x388
[  440.129765] c_can_pci 0000:02:0c.3: can0: write 0x9404 to offset 0x38 failed. got: 0x8
[  440.130441] c_can_pci 0000:02:0c.3: can0: write 0xa to offset 0x20 failed. got: 0x8
[  460.742395] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x8
[  460.742662] c_can_pci 0000:02:0c.3: can0: write 0x90 to offset 0x24 failed. got: 0xa8
[  460.742789] c_can_pci 0000:02:0c.3: can0: write 0x4 to offset 0x20 failed. got: 0x8
[  460.742805] c_can_pci 0000:02:0c.3: can0: write 0x4 to offset 0x20 failed. got: 0x18
[  460.744112] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  471.025367] c_can_pci 0000:02:0c.3: can0: write 0x9 to offset 0x20 failed. got: 0x18
[  493.714037] c_can_pci 0000:02:0c.3: can0: write 0xa to offset 0x20 failed. got: 0x18
[  493.714296] c_can_pci 0000:02:0c.3: can0: write 0xa to offset 0x20 failed. got: 0x8
[  493.714976] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  493.715788] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x388
[  493.716243] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x8
[  547.301509] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x388
[  547.302181] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x88
[  557.602229] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
[  557.602501] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x28
[  557.602773] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa8
[  567.901158] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x8
[  567.901558] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x388
[  567.901821] c_can_pci 0000:02:0c.3: can0: write 0xb to offset 0x20 failed. got: 0x8
[  569.962867] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa0
[  572.024462] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  572.024731] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  576.150457] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  576.150730] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  576.151463] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  576.151716] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  576.151987] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  578.222888] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  578.224252] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  580.284468] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  580.284741] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  580.285969] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  580.286226] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  582.348458] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  582.348730] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  582.348878] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa0
[  582.349494] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
[  582.350354] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x88
[  584.410064] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  584.410455] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  584.410725] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  586.478397] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  586.478662] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  588.551484] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x98
[  588.552464] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  588.552720] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  588.552991] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  590.624880] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  592.695613] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  592.696377] c_can_pci 0000:02:0c.3: can0: write 0x1 to offset 0x20 failed. got: 0x8
[  594.759455] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  594.759727] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  594.760696] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  596.821041] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xac
[  596.821433] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x88
[  596.822423] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
[  596.822683] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x28
[  598.901376] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  598.901517] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x20
[  600.962052] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  600.962728] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  603.050703] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  603.050967] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  605.146593] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  605.146855] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  607.207808] c_can_pci 0000:02:0c.3: can0: write 0x7 to offset 0x20 failed. got: 0x8
[  607.209239] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  609.287327] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x98
[  609.287589] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  611.349466] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  611.349721] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  611.349993] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  613.411182] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa0
[  613.411451] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  613.411714] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  613.413185] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  615.473479] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  615.473739] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  617.547060] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  617.547452] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  617.547723] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  619.610167] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x388
[  619.610897] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x84
[  621.674433] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  621.674827] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x88
[  623.736136] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  623.736389] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  623.736661] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  625.797476] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  625.798001] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa8
[  625.798721] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  627.878566] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  627.878831] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  629.979482] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  629.979735] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  632.053216] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa0
[  632.053486] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  632.053739] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  632.055326] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  634.116321] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  634.116730] c_can_pci 0000:02:0c.3: can0: write 0x1404 to offset 0x38 failed. got: 0x388
[  638.241495] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  638.241757] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  638.243102] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa8
[  640.303328] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  642.386078] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  642.386342] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  644.498061] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  644.498329] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa8
[  644.498476] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  644.498729] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  644.499000] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  646.662057] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  646.663432] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
[  648.726341] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  648.726475] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  648.726742] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  648.728303] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  650.900856] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
[  650.901394] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xac
[  650.901538] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa0
[  655.024046] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  655.024316] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa8
[  655.024465] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  655.024999] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  657.189061] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  657.189453] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x88
[  657.190518] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  657.190787] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  659.251734] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  659.251875] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa0
[  659.252407] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
[  663.484251] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  663.484516] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  665.544773] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  665.546173] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa0
[  665.546443] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  665.546699] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  665.546845] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa0
[  667.608301] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  669.671155] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  671.732041] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  671.732311] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  671.732434] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  671.732450] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  671.732708] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  671.732978] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  671.733784] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  673.794056] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xac
[  673.794317] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  673.794441] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  673.794457] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  673.794715] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  673.794853] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x20
[  673.796066] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  673.796463] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x88
[  675.856075] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  675.856466] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  675.856736] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  677.918058] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  680.001185] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x18
[  680.001444] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  682.060763] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa8
[  682.061048] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xac
[  682.062648] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[  682.062787] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x20
[  684.138124] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  686.199499] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  686.199895] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa0
[  688.261491] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  688.262607] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  688.262879] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  688.263001] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  690.323491] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  690.323754] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  692.384770] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x98
[  692.386036] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xac
[  692.386445] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa8
[  692.386692] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  692.386839] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa0
[  694.447059] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xac
[  694.447320] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  694.447443] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  694.447459] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  694.447715] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  694.447985] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  694.449055] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x28
[  694.449327] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  694.449450] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0x88
[  696.508763] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
[  696.509050] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xac
[  696.509586] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0xa8
[  696.509706] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  696.509971] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  696.510954] c_can_pci 0000:02:0c.3: can0: write 0x10 to offset 0x20 failed. got: 0x8
[  696.511223] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa0
[  698.571318] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0xa8
[  698.572681] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xa8
[  700.646608] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
[  700.647000] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88

As you can see, sometimes it needs several write retries to succeed. Nevertheless my test application detects also some problems:
> Error on MSG ID 0x252. Got counter 96480 and expected 96466
> Error on MSG ID 0x251. Got counter 96480 and expected 96466
> Error on MSG ID 0x252. Got counter 101706 and expected 101696
> Error on MSG ID 0x251. Got counter 101706 and expected 101696
Here were messages missed/dropped for both CAN-IDs.
> Error on MSG ID 0x251. Got counter 108673 and expected 108672
> Error on MSG ID 0x251. Got counter 108672 and expected 108674
> Error on MSG ID 0x251. Got counter 108674 and expected 108673
                                     ^^^^^^
Here you can see the CAN frame with counter 108673 is read before 108672.
> Error on MSG ID 0x251. Got counter 117488 and expected 117487
> Error on MSG ID 0x251. Got counter 117487 and expected 117489
> Error on MSG ID 0x251. Got counter 117489 and expected 117488
                                     ^^^^^^
Here you can see the CAN frame with counter 117488 is read before 117487.
> Error on MSG ID 0x251. Got counter 142297 and expected 142296
> Error on MSG ID 0x251. Got counter 142296 and expected 142298
> Error on MSG ID 0x251. Got counter 142298 and expected 142297
Same again.
> Error on MSG ID 0x251. Got counter 147932 and expected 147924
> Error on MSG ID 0x251. Got counter 147936 and expected 147933
> Error on MSG ID 0x252. Got counter 147936 and expected 147924
> Error on MSG ID 0x251. Got counter 165268 and expected 165267
> Error on MSG ID 0x251. Got counter 218560 and expected 218558
> Error on MSG ID 0x252. Got counter 218560 and expected 218558
> Error on MSG ID 0x252. Got counter 231076 and expected 231075
> Error on MSG ID 0x251. Got counter 231077 and expected 231076
> Error on MSG ID 0x251. Got counter 241959 and expected 241958
Messages missed/dropped again.

Below is the patch for c_can.

Best regards,
Alexander

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index d63aaa3..da9bbc0 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -1186,6 +1186,7 @@ struct net_device *alloc_c_can_dev(void)
                                        CAN_CTRLMODE_BERR_REPORTING;
 
        spin_lock_init(&priv->lock);
+       spin_lock_init(&priv->testlock);
 
        return dev;
 }
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 3487d5e..2b58b75 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -173,6 +173,7 @@ struct c_can_priv {
        unsigned int instance;
        void (*init) (const struct c_can_priv *priv, bool enable);
        spinlock_t lock;        /* to protect tx and rx message objects */
+       spinlock_t testlock;    /* to protect tx and rx message objects */
 };
 
 struct net_device *alloc_c_can_dev(void);
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 2516ea9..0ac4d43 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -74,13 +74,37 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
 static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
                                    enum reg index)
 {
-       return (u16)ioread32(priv->base + 2 * priv->regs[index]);
+       unsigned long flags;
+       u16 reg;
+
+       spin_lock_irqsave(&priv->testlock, flags);
+       reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
+       spin_unlock_irqrestore(&priv->testlock, flags);
+
+       return reg;
 }
 
 static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
                                      enum reg index, u16 val)
 {
-       iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
+       u16 reg;
+       unsigned long flags;
+       int retries;
+
+       retries = 0;
+
+       spin_lock_irqsave(&priv->testlock, flags);
+
+       do
+       {
+               iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
+               reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
+               if (reg != val)
+               {
+                       netdev_err(priv->dev, "write 0x%x to offset 0x%x failed. got: 0x%x\n", val, 2 * priv->regs[index], reg);
+               }
+       } while ((reg != val) && (retries++ < 20));
+       spin_unlock_irqrestore(&priv->testlock, flags);
 }
 
 static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/06/2012 02:38 PM, Alexander Stein wrote:
>>> On the second line you can see that the register isn't written at all (or the 
>>> read failed for some reason).
>>
>> I assume the latter. Could you please retry reading the register until
>> the correct value shows up. With some timeout, of course.
> 
> I notices having all error events printed on serial console using
> pch_uart driver has negative effect (I guess one problem causes
> another one), so I setup 'dmesg -n1' to reduce serial load before the
> test. Running the test with just one heartbeat triggered LED set
> using I2C the test runs without errors. I only see Lots of 'c_can_pci
> 0000:02:0c.3: can0: write 0x0 to offset 0x2c failed. got: 0x2000' at
> the beginning. It seems this (reserved?) bit is always 1 no matter
> what we write. But things start to break when running the test while
> running 'watch sensors' (sensors queries several temperature sensors
> via I2C) in a seconds ssh session. First off the driver error
> messages (omitting the messages as written above):

Hmmm, maybe there is a hardware problem, that would not be the first one
we've seen in CAN hardware.

> [  384.466217] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc
[...]
> [  700.647000] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88

Your spin lock patches look good. I think it's time to get in contact
with your Intel support and try to get through to the hardware people
that can look into the VHDL code.

> As you can see, sometimes it needs several write retries to succeed.
> Nevertheless my test application detects also some problems:

>> Error on MSG ID 0x252. Got counter 96480 and expected 96466
>> Error on MSG ID 0x251. Got counter 96480 and expected 96466
>> Error on MSG ID 0x252. Got counter 101706 and expected 101696
>> Error on MSG ID 0x251. Got counter 101706 and expected 101696
> Here were messages missed/dropped for both CAN-IDs.

Drops can happen at the hardware level, rx-overflow, or in the socket
queueing in linux. Look in the driver if it reports rx-overflows
properly and look at the stats. There also is a flag that indicates
frame drop at the linux socket level, but I don't remember it.

>> Error on MSG ID 0x251. Got counter 108673 and expected 108672
>> Error on MSG ID 0x251. Got counter 108672 and expected 108674
>> Error on MSG ID 0x251. Got counter 108674 and expected 108673
>                                      ^^^^^^
> Here you can see the CAN frame with counter 108673 is read before 108672.

This is a typical out-of-sequence reception with a
RX-goes-into-first-free-mailbox hardware. I've implemented the algorithm
used in the at91 and fixed the one for the ti_hecc.

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

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/06/2012 02:38 PM, Alexander Stein wrote:
> Hello Wolfgang,
> 
> On Wednesday 05 December 2012 18:35:25, Wolfgang Grandegger wrote:
>>>> - The messages look still ok (not currupted, I mean)?
>>>
>>> The received frames all look good (despite wrong counter sometimes due to 
>>> wrong order or lost frames).
>>>
>>>>> Even worse, if I use the following patch to check if PCI writes were 
>>>>> successfully, I notices that some writes (or the consecutive read) don't 
>>>>> succeed. And I also get lots of I2C timeouts waiting for a xfer complete.
>>>>
>>>> Be careful, there might be some registers changing their values after
>>>> writing. Can you show the value read after writing and the register
>>>> offset? The influence on the I2C bus looks more like an overload or
>>>> hardware problem. What is your CAN interrupt rate?
>>>
>>> I get about 33 interrupts per second on i2c. On a successful run I get 366886 
>>> interrupts for 500000 messages with the c_can driver.
>>
>> In what time? Is the CAN bus highly loaded.
> 
> Busload is about 14-18% according to canbusload with 1MBit. A complete sucessful run takes about 5 minutes.
> 
>>> On the second line you can see that the register isn't written at all (or the 
>>> read failed for some reason).
>>
>> I assume the latter. Could you please retry reading the register until
>> the correct value shows up. With some timeout, of course.
> 
> I notices having all error events printed on serial console using pch_uart driver has negative effect (I guess one problem causes another one), so I setup 'dmesg -n1' to reduce serial load before the test.
> Running the test with just one heartbeat triggered LED set using I2C the test runs without errors. I only see
> Lots of 'c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x2c failed. got: 0x2000' at the beginning. It seems this (reserved?) bit is always 1 no matter what we write.

There are reserved bit! From the manual:

 "Reserved: This bit is reserved for future expansion. Only “0” is
  accepted as the write data to the reserved bit. When “1” is
  written, the operation is not guaranteed."

  When reading the reserved bit of the IFmMASK2 register (m = 1), a “1”
  is read. Write a “1” for write.

Well, I don't fully understand but it's clear that we always read "1!.

> But things start to break when running the test while running 'watch sensors' (sensors queries several temperature sensors via I2C) in a seconds ssh session.
> First off the driver error messages (omitting the messages as written above):
> [  384.466217] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xbc


Hm...

> [  384.466630] c_can_pci 0000:02:0c.3: can0: write 0x73 to offset 0x24 failed. got: 0xb8
...
> [  700.646608] c_can_pci 0000:02:0c.3: can0: write 0xe to offset 0x0 failed. got: 0x28
> [  700.647000] c_can_pci 0000:02:0c.3: can0: write 0x0 to offset 0x0 failed. got: 0x88
> 
> As you can see, sometimes it needs several write retries to succeed. Nevertheless my test application detects also some problems:

OK, obviously the real value needs some time to show up when there is
other activity on the bus. This hardware is really special. We need to
contact the hardware developers.

>> Error on MSG ID 0x252. Got counter 96480 and expected 96466
>> Error on MSG ID 0x251. Got counter 96480 and expected 96466
>> Error on MSG ID 0x252. Got counter 101706 and expected 101696
>> Error on MSG ID 0x251. Got counter 101706 and expected 101696
> Here were messages missed/dropped for both CAN-IDs.

It is interesting that the same number of messages are missing for both
IDs... which come from different CAN nodes, right?

Do you see dropped messages with "ip -d -s link show canX"?
Did you check if the protocol layer did drop messages. You can
use "candump -d" to find that out.

>> Error on MSG ID 0x251. Got counter 108673 and expected 108672
>> Error on MSG ID 0x251. Got counter 108672 and expected 108674
>> Error on MSG ID 0x251. Got counter 108674 and expected 108673
>                                      ^^^^^^
> Here you can see the CAN frame with counter 108673 is read before 108672.

You could add some offset/mask to the counter data of MSG ID 0x252 to
see if they get mixed up.

>> Error on MSG ID 0x251. Got counter 117488 and expected 117487
>> Error on MSG ID 0x251. Got counter 117487 and expected 117489
>> Error on MSG ID 0x251. Got counter 117489 and expected 117488
>                                      ^^^^^^
> Here you can see the CAN frame with counter 117488 is read before 117487.
>> Error on MSG ID 0x251. Got counter 142297 and expected 142296
>> Error on MSG ID 0x251. Got counter 142296 and expected 142298
>> Error on MSG ID 0x251. Got counter 142298 and expected 142297
> Same again.
>> Error on MSG ID 0x251. Got counter 147932 and expected 147924
>> Error on MSG ID 0x251. Got counter 147936 and expected 147933
>> Error on MSG ID 0x252. Got counter 147936 and expected 147924
>> Error on MSG ID 0x251. Got counter 165268 and expected 165267
>> Error on MSG ID 0x251. Got counter 218560 and expected 218558
>> Error on MSG ID 0x252. Got counter 218560 and expected 218558
>> Error on MSG ID 0x252. Got counter 231076 and expected 231075
>> Error on MSG ID 0x251. Got counter 231077 and expected 231076
>> Error on MSG ID 0x251. Got counter 241959 and expected 241958
> Messages missed/dropped again.
> 
> Below is the patch for c_can.
> 
> Best regards,
> Alexander
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index d63aaa3..da9bbc0 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1186,6 +1186,7 @@ struct net_device *alloc_c_can_dev(void)
>                                         CAN_CTRLMODE_BERR_REPORTING;
>  
>         spin_lock_init(&priv->lock);
> +       spin_lock_init(&priv->testlock);
>  
>         return dev;
>  }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 3487d5e..2b58b75 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -173,6 +173,7 @@ struct c_can_priv {
>         unsigned int instance;
>         void (*init) (const struct c_can_priv *priv, bool enable);
>         spinlock_t lock;        /* to protect tx and rx message objects */
> +       spinlock_t testlock;    /* to protect tx and rx message objects */
>  };
>  
>  struct net_device *alloc_c_can_dev(void);
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> index 2516ea9..0ac4d43 100644
> --- a/drivers/net/can/c_can/c_can_pci.c
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -74,13 +74,37 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
>  static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
>                                     enum reg index)
>  {
> -       return (u16)ioread32(priv->base + 2 * priv->regs[index]);
> +       unsigned long flags;
> +       u16 reg;
> +
> +       spin_lock_irqsave(&priv->testlock, flags);
> +       reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
> +       spin_unlock_irqrestore(&priv->testlock, flags);
> +
> +       return reg;
>  }
>  
>  static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
>                                       enum reg index, u16 val)
>  {
> -       iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
> +       u16 reg;
> +       unsigned long flags;
> +       int retries;
> +
> +       retries = 0;
> +
> +       spin_lock_irqsave(&priv->testlock, flags);
> +
> +       do
> +       {
> +               iowrite32((u32)val, priv->base + 2 * priv->regs[index]);

I think it's enough to write the message once.

> +               reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
> +               if (reg != val)
> +               {
> +                       netdev_err(priv->dev, "write 0x%x to offset 0x%x failed. got: 0x%x\n", val, 2 * priv->regs[index], reg);
> +               }
> +       } while ((reg != val) && (retries++ < 20));
> +       spin_unlock_irqrestore(&priv->testlock, flags);
>  }
>  
>  static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/06/2012 03:31 PM, Wolfgang Grandegger wrote:
>>  
>>  static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
>>                                       enum reg index, u16 val)
>>  {
>> -       iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
>> +       u16 reg;
>> +       unsigned long flags;
>> +       int retries;
>> +
>> +       retries = 0;
>> +
>> +       spin_lock_irqsave(&priv->testlock, flags);
>> +
>> +       do
>> +       {
>> +               iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
> 
> I think it's enough to write the message once.

It's a different test, but also very interesting.

> 
>> +               reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
>> +               if (reg != val)
>> +               {
>> +                       netdev_err(priv->dev, "write 0x%x to offset 0x%x failed. got: 0x%x\n", val, 2 * priv->regs[index], reg);
>> +               }
>> +       } while ((reg != val) && (retries++ < 20));
>> +       spin_unlock_irqrestore(&priv->testlock, flags);
>>  }
>>  
>>  static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)> 

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

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Alexander Stein]

Hello Wolfgang,

On Thursday 06 December 2012 15:31:20, Wolfgang Grandegger wrote:
> > diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> > index 2516ea9..0ac4d43 100644
> > --- a/drivers/net/can/c_can/c_can_pci.c
> > +++ b/drivers/net/can/c_can/c_can_pci.c
> > @@ -74,13 +74,37 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
> >  static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
> >                                     enum reg index)
> >  {
> > -       return (u16)ioread32(priv->base + 2 * priv->regs[index]);
> > +       unsigned long flags;
> > +       u16 reg;
> > +
> > +       spin_lock_irqsave(&priv->testlock, flags);
> > +       reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
> > +       spin_unlock_irqrestore(&priv->testlock, flags);
> > +
> > +       return reg;
> >  }
> >  
> >  static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
> >                                       enum reg index, u16 val)
> >  {
> > -       iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
> > +       u16 reg;
> > +       unsigned long flags;
> > +       int retries;
> > +
> > +       retries = 0;
> > +
> > +       spin_lock_irqsave(&priv->testlock, flags);
> > +
> > +       do
> > +       {
> > +               iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
> 
> I think it's enough to write the message once.

I can reply to this one immediatly. Not it is not sufficent to write once. This was my 1st implementation (iowrite32 just above the do) and noticed in case the write doesn't succeed I won't succeed until all ioread32 have passed. IMO I must write again if it failed before.

Alexander

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/06/2012 03:56 PM, Alexander Stein wrote:
> Hello Wolfgang,
> 
> On Thursday 06 December 2012 15:31:20, Wolfgang Grandegger wrote:
>>> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
>>> index 2516ea9..0ac4d43 100644
>>> --- a/drivers/net/can/c_can/c_can_pci.c
>>> +++ b/drivers/net/can/c_can/c_can_pci.c
>>> @@ -74,13 +74,37 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
>>>  static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
>>>                                     enum reg index)
>>>  {
>>> -       return (u16)ioread32(priv->base + 2 * priv->regs[index]);
>>> +       unsigned long flags;
>>> +       u16 reg;
>>> +
>>> +       spin_lock_irqsave(&priv->testlock, flags);
>>> +       reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
>>> +       spin_unlock_irqrestore(&priv->testlock, flags);
>>> +
>>> +       return reg;
>>>  }
>>>  
>>>  static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
>>>                                       enum reg index, u16 val)
>>>  {
>>> -       iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
>>> +       u16 reg;
>>> +       unsigned long flags;
>>> +       int retries;
>>> +
>>> +       retries = 0;
>>> +
>>> +       spin_lock_irqsave(&priv->testlock, flags);
>>> +
>>> +       do
>>> +       {
>>> +               iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
>>
>> I think it's enough to write the message once.
> 
> I can reply to this one immediatly. Not it is not sufficent to write once. This was my 1st implementation (iowrite32 just above the do) and noticed in case the write doesn't succeed I won't succeed until all ioread32 have passed. IMO I must write again if it failed before.

Puh, even worse. Then I'm really surprised that the driver continues to
work more or less properly. Anyway, sometime it takes up to 500+ us for
the good value to show up. If we have a real write-read sequence to the
same register, everything can happen. Looks like broken hardware.

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/06/2012 04:15 PM, Wolfgang Grandegger wrote:
> On 12/06/2012 03:56 PM, Alexander Stein wrote:
>> Hello Wolfgang,
>>
>> On Thursday 06 December 2012 15:31:20, Wolfgang Grandegger wrote:
>>>> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
>>>> index 2516ea9..0ac4d43 100644
>>>> --- a/drivers/net/can/c_can/c_can_pci.c
>>>> +++ b/drivers/net/can/c_can/c_can_pci.c
>>>> @@ -74,13 +74,37 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
>>>>  static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
>>>>                                     enum reg index)
>>>>  {
>>>> -       return (u16)ioread32(priv->base + 2 * priv->regs[index]);
>>>> +       unsigned long flags;
>>>> +       u16 reg;
>>>> +
>>>> +       spin_lock_irqsave(&priv->testlock, flags);
>>>> +       reg = (u16)ioread32(priv->base + 2 * priv->regs[index]);
>>>> +       spin_unlock_irqrestore(&priv->testlock, flags);
>>>> +
>>>> +       return reg;
>>>>  }
>>>>  
>>>>  static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
>>>>                                       enum reg index, u16 val)
>>>>  {
>>>> -       iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
>>>> +       u16 reg;
>>>> +       unsigned long flags;
>>>> +       int retries;
>>>> +
>>>> +       retries = 0;
>>>> +
>>>> +       spin_lock_irqsave(&priv->testlock, flags);
>>>> +
>>>> +       do
>>>> +       {
>>>> +               iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
>>>
>>> I think it's enough to write the message once.
>>
>> I can reply to this one immediatly. Not it is not sufficent to write once. This was my 1st implementation (iowrite32 just above the do) and noticed in case the write doesn't succeed I won't succeed until all ioread32 have passed. IMO I must write again if it failed before.
> 
> Puh, even worse. Then I'm really surprised that the driver continues to
> work more or less properly. Anyway, sometime it takes up to 500+ us for
> the good value to show up. If we have a real write-read sequence to the
> same register, everything can happen. Looks like broken hardware.

Could you show the output of "lspci -vv" of your system?

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Alexander Stein]

Hello Wolfgang,

On Thursday 06 December 2012 16:27:55, Wolfgang Grandegger wrote:
> >>> I think it's enough to write the message once.
> >>
> >> I can reply to this one immediatly. Not it is not sufficent to write once. This was my 1st implementation (iowrite32 just above the do) and noticed in case the write doesn't succeed I won't succeed until all ioread32 have passed. IMO I must write again if it failed before.
> > 
> > Puh, even worse. Then I'm really surprised that the driver continues to
> > work more or less properly. Anyway, sometime it takes up to 500+ us for
> > the good value to show up. If we have a real write-read sequence to the
> > same register, everything can happen. Looks like broken hardware.
> 
> Could you show the output of "lspci -vv" of your system?

Here we go,

Alexander

00:00.0 Host bridge: Intel Corporation Device 4114 (rev 05)
	Subsystem: Intel Corporation Device 4108
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0

00:01.0 Host bridge: Intel Corporation Device 8183 (rev 02)
	Subsystem: Intel Corporation Device 8186
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0

00:02.0 Display controller: Intel Corporation Device 4108 (rev 05)
	Subsystem: Intel Corporation Device 4108
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin A routed to IRQ 255
	Region 0: Memory at a0200000 (32-bit, non-prefetchable) [disabled] [size=1M]
	Region 1: I/O ports at 4008 [disabled] [size=8]
	Region 2: Memory at 90000000 (32-bit, non-prefetchable) [disabled] [size=256M]
	Region 3: Memory at a03c0000 (32-bit, non-prefetchable) [disabled] [size=256K]
	Capabilities: [d0] Power Management version 2
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [b0] Vendor Specific Information <?>
	Capabilities: [90] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000

00:03.0 Multimedia video controller: Intel Corporation Device 8182 (rev 02)
	Subsystem: Intel Corporation Device 8186
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin A routed to IRQ 255
	Region 0: Memory at a0300000 (32-bit, non-prefetchable) [disabled] [size=512K]
	Region 1: I/O ports at 4000 [disabled] [size=8]
	Region 2: Memory at 80000000 (32-bit, non-prefetchable) [disabled] [size=256M]
	Region 3: Memory at a0380000 (32-bit, non-prefetchable) [disabled] [size=256K]
	Capabilities: [d0] Power Management version 2
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [b0] Vendor Specific Information <?>
	Capabilities: [90] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000

00:17.0 PCI bridge: Intel Corporation Device 8184 (rev 02) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Bus: primary=00, secondary=01, subordinate=02, sec-latency=0
	I/O behind bridge: 00003000-00003fff
	Memory behind bridge: a0100000-a01fffff
	Prefetchable memory behind bridge: 40000000-401fffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Express (v1) Root Port (Slot-), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
			ExtTag- RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
		LnkCap:	Port #1, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <256ns, L1 <4us
			ClockPM+ Surprise- LLActRep+ BwNot-
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
		RootCap: CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
	Capabilities: [90] Subsystem: Intel Corporation Device 8186
	Capabilities: [a0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [100] Virtual Channel <?>
	Kernel driver in use: pcieport

00:18.0 PCI bridge: Intel Corporation Device 8185 (rev 02) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Bus: primary=00, secondary=03, subordinate=03, sec-latency=0
	I/O behind bridge: 00002000-00002fff
	Memory behind bridge: a0000000-a00fffff
	Prefetchable memory behind bridge: 40200000-403fffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Express (v1) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
			ExtTag- RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
		LnkCap:	Port #2, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <256ns, L1 <4us
			ClockPM+ Surprise- LLActRep+ BwNot-
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surpise+
			Slot #  2, PowerLimit 10.000000; Interlock- NoCompl-
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt+ HPIrq+ LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
			Changed: MRL- PresDet- LinkState+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
		RootCap: CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
	Capabilities: [90] Subsystem: Intel Corporation Device 8186
	Capabilities: [a0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [100] Virtual Channel <?>
	Kernel driver in use: pcieport

00:1b.0 Audio device: Intel Corporation System Controller Hub (SCH Poulsbo) HD Audio Controller (rev 02)
	Subsystem: Intel Corporation Device 8186
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at a0400000 (64-bit, non-prefetchable) [size=16K]
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [70] Express (v1) Root Complex Integrated Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag- RBE- FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
		LnkCap:	Port #0, Speed unknown, Width x0, ASPM unknown, Latency L0 <64ns, L1 <1us
			ClockPM- Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM L0s Enabled; Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed unknown, Width x0, TrErr- Train- SlotClk- DLActive- BWMgmt- ABWMgmt-
	Capabilities: [100] Virtual Channel <?>
	Capabilities: [130] Root Complex Link <?>
	Kernel driver in use: HDA Intel

00:1f.0 ISA bridge: Intel Corporation Device 8186 (rev 02)
	Subsystem: Intel Corporation Device 8186
	Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Kernel driver in use: lpc_sch

01:00.0 PCI bridge: Intel Corporation Device 8800 (rev 01) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Bus: primary=01, secondary=02, subordinate=02, sec-latency=0
	I/O behind bridge: 00003000-00003fff
	Memory behind bridge: a0100000-a01fffff
	Prefetchable memory behind bridge: 0000000040000000-00000000400fffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	Expansion ROM at 40100000 [disabled] [size=64K]
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2- AuxCurrent=0mA PME(D0+,D1+,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [70] Express (v2) PCI/PCI-X Bridge, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- BrConfRtry-
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr+ UncorrErr- FatalErr- UnsuppReq+ AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <512ns, L1 <64us
			ClockPM- Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM Disabled; Disabled- Retrain- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
		DevCap2: Completion Timeout: Not Supported, TimeoutDis-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
		LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-, Selectable De-emphasis: -6dB
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB

02:00.0 Class ff00: Intel Corporation Device 8801 (rev 01)
	Subsystem: Intel Corporation Device 8801
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Region 1: Memory at a0102000 (32-bit, non-prefetchable) [size=2K]
	Expansion ROM at 40000000 [disabled] [size=128K]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_phub

02:00.1 Ethernet controller: Intel Corporation Device 8802 (rev 02)
	Subsystem: Intel Corporation Device 8802
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 55
	Region 0: I/O ports at 3020 [size=32]
	Region 1: Memory at a0107000 (32-bit, non-prefetchable) [size=512]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 41d1
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_gbe
	Kernel modules: pch_gbe

02:00.2 Class ff00: Intel Corporation Device 8803 (rev 01)
	Subsystem: Intel Corporation Device 8803
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 40
	Region 1: Memory at a0114000 (32-bit, non-prefetchable) [size=64]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4149
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_gpio

02:02.0 USB Controller: Intel Corporation Device 8804 (rev 02) (prog-if 10 [OHCI])
	Subsystem: Intel Corporation Device 8804
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 19
	Region 0: Memory at a0113000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

02:02.1 USB Controller: Intel Corporation Device 8805 (rev 02) (prog-if 10 [OHCI])
	Subsystem: Intel Corporation Device 8805
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 19
	Region 0: Memory at a0112000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

02:02.2 USB Controller: Intel Corporation Device 8806 (rev 02) (prog-if 10 [OHCI])
	Subsystem: Intel Corporation Device 8806
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 19
	Region 0: Memory at a0111000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

02:02.3 USB Controller: Intel Corporation Device 8807 (rev 02) (prog-if 20 [EHCI])
	Subsystem: Intel Corporation Device 8807
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 19
	Region 0: Memory at a0110000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ehci_hcd

02:02.4 USB Controller: Intel Corporation Device 8808 (rev 02) (prog-if fe [USB Device])
	Subsystem: Intel Corporation Device 8808
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin B routed to IRQ 255
	Region 1: Memory at a0100000 (32-bit, non-prefetchable) [disabled] [size=8K]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-

02:04.0 SD Host controller: Intel Corporation Device 8809 (rev 01) (prog-if 01)
	Subsystem: Intel Corporation Device 8809
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin C routed to IRQ 49
	Region 0: Memory at a0106000 (32-bit, non-prefetchable) [size=512]
	Capabilities: [80] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 41a1
	Capabilities: [90] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: sdhci-pci

02:04.1 SD Host controller: Intel Corporation Device 880a (rev 01) (prog-if 01)
	Subsystem: Intel Corporation Device 880a
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin C routed to IRQ 50
	Region 0: Memory at a0105000 (32-bit, non-prefetchable) [size=512]
	Capabilities: [80] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 41a9
	Capabilities: [90] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: sdhci-pci

02:06.0 SATA controller: Intel Corporation Device 880b (rev 02) (prog-if 01 [AHCI 1.0])
	Subsystem: Intel Corporation Device 880b
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin D routed to IRQ 47
	Region 4: I/O ports at 3000 [size=32]
	Region 5: Memory at a0103000 (32-bit, non-prefetchable) [size=1K]
	Expansion ROM at 40020000 [disabled] [size=128K]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4191
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [60] SATA HBA <?>
	Kernel driver in use: ahci

02:08.0 USB Controller: Intel Corporation Device 880c (rev 02) (prog-if 10 [OHCI])
	Subsystem: Intel Corporation Device 880c
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at a010f000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

02:08.1 USB Controller: Intel Corporation Device 880d (rev 02) (prog-if 10 [OHCI])
	Subsystem: Intel Corporation Device 880d
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at a010e000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

02:08.2 USB Controller: Intel Corporation Device 880e (rev 02) (prog-if 10 [OHCI])
	Subsystem: Intel Corporation Device 880e
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at a010d000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

02:08.3 USB Controller: Intel Corporation Device 880f (rev 02) (prog-if 20 [EHCI])
	Subsystem: Intel Corporation Device 880f
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at a010c000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ehci_hcd

02:0a.0 Class ff00: Intel Corporation Device 8810
	Subsystem: Intel Corporation Device 8810
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 41
	Region 1: Memory at a010b000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4151
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [60] #00 [0000]
	Kernel driver in use: pch-dma

02:0a.1 Serial controller: Intel Corporation Device 8811 (rev 01) (prog-if 02 [16550])
	Subsystem: Intel Corporation Device 8811
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 43
	Region 0: I/O ports at 3058 [size=8]
	Region 1: Memory at a0119000 (32-bit, non-prefetchable) [size=16]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4169
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_uart

02:0a.2 Serial controller: Intel Corporation Device 8812 (prog-if 02 [16550])
	Subsystem: Intel Corporation Device 8812
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 44
	Region 0: I/O ports at 3050 [size=8]
	Region 1: Memory at a0118000 (32-bit, non-prefetchable) [size=16]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4171
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_uart

02:0a.3 Serial controller: Intel Corporation Device 8813 (prog-if 02 [16550])
	Subsystem: Intel Corporation Device 8813
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 45
	Region 0: I/O ports at 3048 [size=8]
	Region 1: Memory at a0117000 (32-bit, non-prefetchable) [size=16]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4179
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_uart

02:0a.4 Serial controller: Intel Corporation Device 8814 (prog-if 02 [16550])
	Subsystem: Intel Corporation Device 8814
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 46
	Region 0: I/O ports at 3040 [size=8]
	Region 1: Memory at a0116000 (32-bit, non-prefetchable) [size=16]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4181
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_uart

02:0c.0 Class ff00: Intel Corporation Device 8815
	Subsystem: Intel Corporation Device 8815
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin C routed to IRQ 42
	Region 1: Memory at a010a000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4161
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [60] #00 [0000]
	Kernel driver in use: pch-dma

02:0c.1 Serial bus controller [0c80]: Intel Corporation Device 8816
	Subsystem: Intel Corporation Device 8816
	Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin C routed to IRQ 48
	Region 1: Memory at a0115000 (32-bit, non-prefetchable) [size=32]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 4199
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pch_spi

02:0c.2 Serial bus controller [0c80]: Intel Corporation Device 8817
	Subsystem: Intel Corporation Device 8817
	Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin C routed to IRQ 18
	Region 1: Memory at a0109000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: i2c_eg20t

02:0c.3 CANBUS: Intel Corporation Device 8818
	Subsystem: Intel Corporation Device 8818
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin C routed to IRQ 51
	Region 1: Memory at a0104000 (32-bit, non-prefetchable) [size=512]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable+
		Address: fee0300c  Data: 41b1
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: c_can_pci
	Kernel modules: c_can_pci

02:0c.4 Class ff00: Intel Corporation Device 8819 (rev 01)
	Subsystem: Intel Corporation Device 8819
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin C routed to IRQ 255
	Region 1: Memory at a0108000 (32-bit, non-prefetchable) [disabled] [size=256]
	Capabilities: [40] MSI: Mask- 64bit- Count=1/1 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-

03:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network Connection
	Subsystem: Intel Corporation Device 0000
	Physical Slot: 2
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 17
	Region 0: Memory at a0000000 (32-bit, non-prefetchable) [size=128K]
	Region 2: I/O ports at 2000 [disabled] [size=32]
	Region 3: Memory at a0020000 (32-bit, non-prefetchable) [size=16K]
	Expansion ROM at 40200000 [disabled] [size=2K]
	Capabilities: [c8] Power Management version 2
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=1 PME-
	Capabilities: [d0] MSI: Mask- 64bit+ Count=1/1 Enable-
		Address: 0000000000000000  Data: 0000
	Capabilities: [e0] Express (v1) Endpoint, MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
		DevCtl:	Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr+ UncorrErr- FatalErr- UnsuppReq+ AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <2us, L1 <64us
			ClockPM- Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: [a0] MSI-X: Enable+ Mask- TabSize=5
		Vector table: BAR=3 offset=00000000
		PBA: BAR=3 offset=00002000
	Capabilities: [100] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
		AERCap:	First Error Pointer: 00, GenCap- CGenEn- ChkCap- ChkEn-
	Capabilities: [140] Device Serial Number f6-01-f8-ff-ff-c2-50-00
	Kernel driver in use: e1000e
	Kernel modules: e1000e

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Alexander Stein]

Hello Wolfgang,

On Thursday 06 December 2012 15:31:20, Wolfgang Grandegger wrote:
> >> Error on MSG ID 0x252. Got counter 96480 and expected 96466
> >> Error on MSG ID 0x251. Got counter 96480 and expected 96466
> >> Error on MSG ID 0x252. Got counter 101706 and expected 101696
> >> Error on MSG ID 0x251. Got counter 101706 and expected 101696
> > Here were messages missed/dropped for both CAN-IDs.
> 
> It is interesting that the same number of messages are missing for both
> IDs... which come from different CAN nodes, right?

Yep, node 1 sends 0x251 and node 2 sends 0x252.

> Do you see dropped messages with "ip -d -s link show canX"?
> Did you check if the protocol layer did drop messages. You can
> use "candump -d" to find that out.

My test application says:
Error on MSG ID 0x251. Got counter 8442 and expected 8427
Error on MSG ID 0x252. Got counter 8442 and expected 8426
Error on MSG ID 0x251. Got counter 8794 and expected 8792
Error on MSG ID 0x252. Got counter 8794 and expected 8791
Message counters: 0x251: 249982, 0x252: 249980

ip -s -d link show can0:
3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN qlen 10
    link/can 
    can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0 
    bitrate 1000000 sample-point 0.700 
    tq 100 prop-seg 3 phase-seg1 3 phase-seg2 3 sjw 1
    c_can: tseg1 2..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp-inc 1
    clock 50000000
    re-started bus-errors arbit-lost error-warn error-pass bus-off
    0          0          0          0          0          0         
    RX: bytes  packets  errors  dropped overrun mcast   
    1999980    501239   0       0       0       0      
    TX: bytes  packets  errors  dropped carrier collsns 
    8          1        0       0       0       0

candump -d can0,0~0,#ffffffff didn't show anything. I used the filter to reduce the load from that ssh session.

On Thursday 06 December 2012 15:02:39, Marc Kleine-Budde wrote:
> >> Error on MSG ID 0x251. Got counter 108673 and expected 108672
> >> Error on MSG ID 0x251. Got counter 108672 and expected 108674
> >> Error on MSG ID 0x251. Got counter 108674 and expected 108673
> >                                      ^^^^^^
> > Here you can see the CAN frame with counter 108673 is read before 108672.
> 
> You could add some offset/mask to the counter data of MSG ID 0x252 to
> see if they get mixed up.

Yep, nice idea. But I've no idea whether I get the possibility to modify the firmware for that.

> This is a typical out-of-sequence reception with a
> RX-goes-into-first-free-mailbox hardware. I've implemented the algorithm
> used in the at91 and fixed the one for the ti_hecc.

Marc, do you mean out-of-sequence reception with such mailbox types can happen even with the algorithm used in c_can and at91?

Best regards,
Alexander

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/06/2012 06:14 PM, Alexander Stein wrote:
>> This is a typical out-of-sequence reception with a 
>> RX-goes-into-first-free-mailbox hardware. I've implemented the
>> algorithm used in the at91 and fixed the one for the ti_hecc.
> 
> Marc, do you mean out-of-sequence reception with such mailbox types
> can happen even with the algorithm used in c_can and at91?

It can happen, if the algorithm isn't implemented properly and adopted
to the needs of the hardware. On the unmodified ti_hecc we see this
out-of-sequence problems, too.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/07/2012 12:34 AM, Marc Kleine-Budde wrote:
> On 12/06/2012 06:14 PM, Alexander Stein wrote:
>>> This is a typical out-of-sequence reception with a 
>>> RX-goes-into-first-free-mailbox hardware. I've implemented the
>>> algorithm used in the at91 and fixed the one for the ti_hecc.
>>
>> Marc, do you mean out-of-sequence reception with such mailbox types
>> can happen even with the algorithm used in c_can and at91?
> 
> It can happen, if the algorithm isn't implemented properly and adopted
> to the needs of the hardware. On the unmodified ti_hecc we see this
> out-of-sequence problems, too.

Yep, that's the tricky part and the pch_can and c_can use a different
approach to solve the problem. Let's concentrate on the c_can driver.
Key-point is that the controller does put the message into the FIFO
object with the lowest number. Here is the relevant code:

/*
 * theory of operation:
 *
 * c_can core saves a received CAN message into the first free message
 * object it finds free (starting with the lowest). Bits NEWDAT and
 * INTPND are set for this message object indicating that a new message
 * has arrived. To work-around this issue, we keep two groups of message
 * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
 *
 * To ensure in-order frame reception we use the following
 * approach while re-activating a message object to receive further
 * frames:
 * - if the current message object number is lower than
 *   C_CAN_MSG_RX_LOW_LAST, do not clear the NEWDAT bit while clearing
 *   the INTPND bit.
 * - if the current message object number is equal to
 *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of all lower
 *   receive message objects.
 * - if the current message object number is greater than
 *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of
 *   only this message object.
 */
static int c_can_do_rx_poll(struct net_device *dev, int quota)
{
	u32 num_rx_pkts = 0;
	unsigned int msg_obj, msg_ctrl_save;
	struct c_can_priv *priv = netdev_priv(dev);
	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);

	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
			val = c_can_read_reg32(priv, C_CAN_INTPND1_REG),
			msg_obj++) {
		/*
		 * as interrupt pending register's bit n-1 corresponds to
		 * message object n, we need to handle the same properly.
		 */
		if (val & (1 << (msg_obj - 1))) {
			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
					~IF_COMM_TXRQST);
			msg_ctrl_save = priv->read_reg(priv,
					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));

			if (msg_ctrl_save & IF_MCONT_EOB)
				return num_rx_pkts;

			if (msg_ctrl_save & IF_MCONT_MSGLST) {
				c_can_handle_lost_msg_obj(dev, IFACE_RX,
								msg_obj);
				num_rx_pkts++;
				quota--;
				continue;
			}

			if (!(msg_ctrl_save & IF_MCONT_NEWDAT))
				continue;

			/* read the data from the message object */
			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);

			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
				c_can_mark_rx_msg_obj(dev, IFACE_RX,
						msg_ctrl_save, msg_obj);
			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
				/* activate this msg obj */
				c_can_activate_rx_msg_obj(dev, IFACE_RX,
						msg_ctrl_save, msg_obj);
			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
				/* activate all lower message objects */
				c_can_activate_all_lower_rx_msg_obj(dev,
						IFACE_RX, msg_ctrl_save);

			num_rx_pkts++;
			quota--;
		}
	}

	return num_rx_pkts;
}


I see a problem if the current message object number is greater than
C_CAN_MSG_RX_LOW_LAST, which is #8. Lets assume the FIFO filled up to
message number #8 when we read INTPND1_REG. Before the we call
c_can_activate_all_lower_rx_msg_obj(), another message is received
into #9. Then we call c_can_activate_all_lower_rx_msg_obj() and shortly
after another messages is received into #0. The next time we enter
this function it will read first #1 and then #9, which is the wrong
order.

Wolfgang.

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/07/2012 10:26 AM, Wolfgang Grandegger wrote:
> On 12/07/2012 12:34 AM, Marc Kleine-Budde wrote:
>> On 12/06/2012 06:14 PM, Alexander Stein wrote:
>>>> This is a typical out-of-sequence reception with a 
>>>> RX-goes-into-first-free-mailbox hardware. I've implemented the
>>>> algorithm used in the at91 and fixed the one for the ti_hecc.
>>>
>>> Marc, do you mean out-of-sequence reception with such mailbox types
>>> can happen even with the algorithm used in c_can and at91?
>>
>> It can happen, if the algorithm isn't implemented properly and adopted
>> to the needs of the hardware. On the unmodified ti_hecc we see this
>> out-of-sequence problems, too.
> 
> Yep, that's the tricky part and the pch_can and c_can use a different
> approach to solve the problem. Let's concentrate on the c_can driver.
> Key-point is that the controller does put the message into the FIFO
> object with the lowest number. Here is the relevant code:
> 
> /*
>  * theory of operation:
>  *
>  * c_can core saves a received CAN message into the first free message
>  * object it finds free (starting with the lowest). Bits NEWDAT and
>  * INTPND are set for this message object indicating that a new message
>  * has arrived. To work-around this issue, we keep two groups of message
>  * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
>  *
>  * To ensure in-order frame reception we use the following
>  * approach while re-activating a message object to receive further
>  * frames:
>  * - if the current message object number is lower than
>  *   C_CAN_MSG_RX_LOW_LAST, do not clear the NEWDAT bit while clearing
>  *   the INTPND bit.
>  * - if the current message object number is equal to
>  *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of all lower
>  *   receive message objects.
>  * - if the current message object number is greater than
>  *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of
>  *   only this message object.
>  */
> static int c_can_do_rx_poll(struct net_device *dev, int quota)
> {
> 	u32 num_rx_pkts = 0;
> 	unsigned int msg_obj, msg_ctrl_save;
> 	struct c_can_priv *priv = netdev_priv(dev);
> 	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);
> 
> 	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
> 			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
> 			val = c_can_read_reg32(priv, C_CAN_INTPND1_REG),
> 			msg_obj++) {
> 		/*
> 		 * as interrupt pending register's bit n-1 corresponds to
> 		 * message object n, we need to handle the same properly.
> 		 */
> 		if (val & (1 << (msg_obj - 1))) {
> 			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
> 					~IF_COMM_TXRQST);
> 			msg_ctrl_save = priv->read_reg(priv,
> 					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));
> 
> 			if (msg_ctrl_save & IF_MCONT_EOB)
> 				return num_rx_pkts;
> 
> 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
> 				c_can_handle_lost_msg_obj(dev, IFACE_RX,
> 								msg_obj);
> 				num_rx_pkts++;
> 				quota--;
> 				continue;
> 			}
> 
> 			if (!(msg_ctrl_save & IF_MCONT_NEWDAT))
> 				continue;
> 
> 			/* read the data from the message object */
> 			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);
> 
> 			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
> 				c_can_mark_rx_msg_obj(dev, IFACE_RX,
> 						msg_ctrl_save, msg_obj);
> 			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
> 				/* activate this msg obj */
> 				c_can_activate_rx_msg_obj(dev, IFACE_RX,
> 						msg_ctrl_save, msg_obj);
> 			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
> 				/* activate all lower message objects */
> 				c_can_activate_all_lower_rx_msg_obj(dev,
> 						IFACE_RX, msg_ctrl_save);
> 
> 			num_rx_pkts++;
> 			quota--;
> 		}
> 	}
> 
> 	return num_rx_pkts;
> }
> 
> 
> I see a problem if the current message object number is greater than
> C_CAN_MSG_RX_LOW_LAST, which is #8. Lets assume the FIFO filled up to
> message number #8 when we read INTPND1_REG. Before the we call
> c_can_activate_all_lower_rx_msg_obj(), another message is received
> into #9. Then we call c_can_activate_all_lower_rx_msg_obj() and shortly
> after another messages is received into #0. The next time we enter
> this function it will read first #1 and then #9, which is the wrong
> order.

This is the my reworked version of the ti_hecc poll loop.
Note: the ti_hecc fills the mailboxes from highest number to lowest, not
lowest to highest like the c_can.

>         do {

It's not a for loop starting at the first mailbox. It always works on
the the next one....

>                 reg_rmp = hecc_read(priv, HECC_CANRMP) & priv->rx_active;
>                 if (!(reg_rmp & BIT(priv->rx_next))) {
>                         /*
>                          * Wrap around only if:
>                          * - we are in the lower group and
>                          * - there is a CAN frame in the first mailbox
>                          *   of the high group.
>                          */

...and only wraps around to the first one under certain circumstances.

>                         if ((priv->rx_next <= HECC_RX_BUFFER_MBOX) &&
>                             (reg_rmp & BIT(HECC_RX_FIRST_MBOX)))
>                                 priv->rx_next = HECC_RX_FIRST_MBOX;
>                         else
>                                 break;
>                 }
>                 mb = priv->rx_next--;
> 
>                 /* disable mailbox */
>                 priv->rx_active &= ~BIT(mb);
> 
>                 ti_hecc_rx_pkt(priv, mb);
> 
>                 /* enable mailboxes */
>                 if (mb == HECC_RX_BUFFER_MBOX) {
>                         hecc_write(priv, HECC_CANRMP, HECC_RX_HIGH_MBOX_MASK);
>                         priv->rx_active |= HECC_RX_HIGH_MBOX_MASK;
>                 } else if (mb == HECC_RX_FIRST_MBOX) {
>                         hecc_write(priv, HECC_CANRMP, HECC_RX_LOW_MBOX_MASK);
>                         priv->rx_active |= HECC_RX_LOW_MBOX_MASK;
>                 }

The enabling of the mailboxes is delayed compared to the original algorithm.

> 
>                 received++;
>                 quota--;
>         } while (quota);

We have at least 3 drivers with the same algorithm. I'm thinking if it
is possible to create a library helper for this.

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

RE: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Bhupesh SHARMA]

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Friday, December 07, 2012 3:26 PM
> To: Wolfgang Grandegger
> Cc: Alexander Stein; linux-can@vger.kernel.org; Bhupesh SHARMA;
> tomoya.rohm@gmail.com
> Subject: Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to
> c_can
> 
> On 12/07/2012 10:26 AM, Wolfgang Grandegger wrote:
> > On 12/07/2012 12:34 AM, Marc Kleine-Budde wrote:
> >> On 12/06/2012 06:14 PM, Alexander Stein wrote:
> >>>> This is a typical out-of-sequence reception with a
> >>>> RX-goes-into-first-free-mailbox hardware. I've implemented the
> >>>> algorithm used in the at91 and fixed the one for the ti_hecc.
> >>>
> >>> Marc, do you mean out-of-sequence reception with such mailbox types
> >>> can happen even with the algorithm used in c_can and at91?
> >>
> >> It can happen, if the algorithm isn't implemented properly and
> >> adopted to the needs of the hardware. On the unmodified ti_hecc we
> >> see this out-of-sequence problems, too.
> >
> > Yep, that's the tricky part and the pch_can and c_can use a different
> > approach to solve the problem. Let's concentrate on the c_can driver.
> > Key-point is that the controller does put the message into the FIFO
> > object with the lowest number. Here is the relevant code:
> >
> > /*
> >  * theory of operation:
> >  *
> >  * c_can core saves a received CAN message into the first free message
> >  * object it finds free (starting with the lowest). Bits NEWDAT and
> >  * INTPND are set for this message object indicating that a new
> > message
> >  * has arrived. To work-around this issue, we keep two groups of
> > message
> >  * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
> >  *
> >  * To ensure in-order frame reception we use the following
> >  * approach while re-activating a message object to receive further
> >  * frames:
> >  * - if the current message object number is lower than
> >  *   C_CAN_MSG_RX_LOW_LAST, do not clear the NEWDAT bit while
> clearing
> >  *   the INTPND bit.
> >  * - if the current message object number is equal to
> >  *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of all lower
> >  *   receive message objects.
> >  * - if the current message object number is greater than
> >  *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of
> >  *   only this message object.
> >  */
> > static int c_can_do_rx_poll(struct net_device *dev, int quota) {
> > 	u32 num_rx_pkts = 0;
> > 	unsigned int msg_obj, msg_ctrl_save;
> > 	struct c_can_priv *priv = netdev_priv(dev);
> > 	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);
> >
> > 	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
> > 			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota >
> 0;
> > 			val = c_can_read_reg32(priv,
> C_CAN_INTPND1_REG),
> > 			msg_obj++) {
> > 		/*
> > 		 * as interrupt pending register's bit n-1 corresponds to
> > 		 * message object n, we need to handle the same properly.
> > 		 */
> > 		if (val & (1 << (msg_obj - 1))) {
> > 			c_can_object_get(dev, IFACE_RX, msg_obj,
> IF_COMM_ALL &
> > 					~IF_COMM_TXRQST);
> > 			msg_ctrl_save = priv->read_reg(priv,
> > 					C_CAN_IFACE(MSGCTRL_REG,
> IFACE_RX));
> >
> > 			if (msg_ctrl_save & IF_MCONT_EOB)
> > 				return num_rx_pkts;
> >
> > 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
> > 				c_can_handle_lost_msg_obj(dev, IFACE_RX,
> > 								msg_obj);
> > 				num_rx_pkts++;
> > 				quota--;
> > 				continue;
> > 			}
> >
> > 			if (!(msg_ctrl_save & IF_MCONT_NEWDAT))
> > 				continue;
> >
> > 			/* read the data from the message object */
> > 			c_can_read_msg_object(dev, IFACE_RX,
> msg_ctrl_save);
> >
> > 			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
> > 				c_can_mark_rx_msg_obj(dev, IFACE_RX,
> > 						msg_ctrl_save, msg_obj);
> > 			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
> > 				/* activate this msg obj */
> > 				c_can_activate_rx_msg_obj(dev, IFACE_RX,
> > 						msg_ctrl_save, msg_obj);
> > 			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
> > 				/* activate all lower message objects */
> > 				c_can_activate_all_lower_rx_msg_obj(dev,
> > 						IFACE_RX, msg_ctrl_save);
> >
> > 			num_rx_pkts++;
> > 			quota--;
> > 		}
> > 	}
> >
> > 	return num_rx_pkts;
> > }
> >
> >
> > I see a problem if the current message object number is greater than
> > C_CAN_MSG_RX_LOW_LAST, which is #8. Lets assume the FIFO filled up to
> > message number #8 when we read INTPND1_REG. Before the we call
> > c_can_activate_all_lower_rx_msg_obj(), another message is received
> > into #9. Then we call c_can_activate_all_lower_rx_msg_obj() and
> > shortly after another messages is received into #0. The next time we
> > enter this function it will read first #1 and then #9, which is the
> > wrong order.
> 
> This is the my reworked version of the ti_hecc poll loop.
> Note: the ti_hecc fills the mailboxes from highest number to lowest, not
> lowest to highest like the c_can.
> 
> >         do {
> 
> It's not a for loop starting at the first mailbox. It always works on the the next
> one....
> 
> >                 reg_rmp = hecc_read(priv, HECC_CANRMP) & priv->rx_active;
> >                 if (!(reg_rmp & BIT(priv->rx_next))) {
> >                         /*
> >                          * Wrap around only if:
> >                          * - we are in the lower group and
> >                          * - there is a CAN frame in the first mailbox
> >                          *   of the high group.
> >                          */
> 
> ...and only wraps around to the first one under certain circumstances.
> 
> >                         if ((priv->rx_next <= HECC_RX_BUFFER_MBOX) &&
> >                             (reg_rmp & BIT(HECC_RX_FIRST_MBOX)))
> >                                 priv->rx_next = HECC_RX_FIRST_MBOX;
> >                         else
> >                                 break;
> >                 }
> >                 mb = priv->rx_next--;
> >
> >                 /* disable mailbox */
> >                 priv->rx_active &= ~BIT(mb);
> >
> >                 ti_hecc_rx_pkt(priv, mb);
> >
> >                 /* enable mailboxes */
> >                 if (mb == HECC_RX_BUFFER_MBOX) {
> >                         hecc_write(priv, HECC_CANRMP,
> HECC_RX_HIGH_MBOX_MASK);
> >                         priv->rx_active |= HECC_RX_HIGH_MBOX_MASK;
> >                 } else if (mb == HECC_RX_FIRST_MBOX) {
> >                         hecc_write(priv, HECC_CANRMP,
> HECC_RX_LOW_MBOX_MASK);
> >                         priv->rx_active |= HECC_RX_LOW_MBOX_MASK;
> >                 }
> 
> The enabling of the mailboxes is delayed compared to the original algorithm.
> 
> >
> >                 received++;
> >                 quota--;
> >         } while (quota);
> 
> We have at least 3 drivers with the same algorithm. I'm thinking if it is
> possible to create a library helper for this.
> 

A library helper seems sensible, but don't forget that different cores have different priority schemes (higher to lower and lower to higher) 
when they service RX packets, so the library helper should also accommodate this.

Regards,
Bhupesh

Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Marc Kleine-Budde]

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

On 12/07/2012 11:00 AM, Bhupesh SHARMA wrote:
>> We have at least 3 drivers with the same algorithm. I'm thinking if
>> it is possible to create a library helper for this.
>> 
> 
> A library helper seems sensible, but don't forget that different
> cores have different priority schemes (higher to lower and lower to
> higher) when they service RX packets, so the library helper should
> also accommodate this.

Sure. There are some quite sophisticated abstractions in the kernel,
like irq chips, that I should look at.

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