[PATCH 0/3] can: at91_can: fix for errata 50.2.6.3 & 50.3.5.3

[PATCH 0/3] can: at91_can: fix for errata 50.2.6.3 & 50.3.5.3

[Marc Kleine-Budde]

Hello,

as promised I've implemented the proposed workaround for the errata
50.2.6.3 & 50.3.5.3:
"Contents of Mailbox 0 can be sent Even if Mailbox is Disabled"

This means under high bus load it can happen that the mailbox 0 is send
to the bus. And it does happen, even with the mainline driver where
Mailbox 0 is a receive mailbox. The errata proposes not to use mailbox 0
and load it with an unused can_id that will not disturb the bus.

The first patch cleans up the driver without any functional changes, so
that the mailbox 0 can be disabled in the second patch. The third patch
adds a sysfs parameter to the driver, so that the identifier of mailbox 0
can configured.

This series applies to net-2.6/master. It has been tested on a ronetix pm9263
board against a PCI-SJA1000 card with the canfdtest utility.

regards, Marc

---

The following changes since commit b11a25aaeccc29d5090d1ce9776af20e3ee99ab9:

  qlcnic: change module parameter permissions (2011-01-10 13:34:55 -0800)

are available in the git repository at:
  git://git.pengutronix.de/git/mkl/linux-2.6.git can/at91_can-for-net-2.6

Marc Kleine-Budde (3):
      can: at91_can: clean up usage of AT91_MB_RX_FIRST and AT91_MB_RX_NUM
      can: at91_can: don't use mailbox 0
      can: at91_can: make can_id of mailbox 0 configurable

 drivers/net/can/at91_can.c |  138 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 26 deletions(-)

[PATCH 1/3] can: at91_can: clean up usage of AT91_MB_RX_FIRST and AT91_MB_RX_NUM

[Marc Kleine-Budde]

This patch cleans up the usage of two macros which specify the mailbox
usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
number of RX mailboxes. The current driver uses these variables in an
unclean way; assuming that AT91_MB_RX_FIRST is 0;

This patch cleans up the usage of these macros, no longer assuming
AT91_MB_RX_FIRST == 0.

Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/net/can/at91_can.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 7ef83d0..892c3d8 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -2,7 +2,7 @@
  * at91_can.c - CAN network driver for AT91 SoC CAN controller
  *
  * (C) 2007 by Hans J. Koch <hjk-vqZO0P4V72/QD6PfKP4TzA@public.gmane.org>
- * (C) 2008, 2009, 2010 by Marc Kleine-Budde <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ * (C) 2008, 2009, 2010, 2011 by Marc Kleine-Budde <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  *
  * This software may be distributed under the terms of the GNU General
  * Public License ("GPL") version 2 as distributed in the 'COPYING'
@@ -55,7 +55,8 @@
 #define AT91_MB_RX_MASK(i)	((1 << (i)) - 1)
 #define AT91_MB_RX_SPLIT	8
 #define AT91_MB_RX_LOW_LAST	(AT91_MB_RX_SPLIT - 1)
-#define AT91_MB_RX_LOW_MASK	(AT91_MB_RX_MASK(AT91_MB_RX_SPLIT))
+#define AT91_MB_RX_LOW_MASK	(AT91_MB_RX_MASK(AT91_MB_RX_SPLIT) & \
+				 ~AT91_MB_RX_MASK(AT91_MB_RX_FIRST))
 
 #define AT91_MB_TX_NUM		(1 << AT91_MB_TX_SHIFT)
 #define AT91_MB_TX_FIRST	(AT91_MB_RX_LAST + 1)
@@ -254,7 +255,8 @@ static void at91_setup_mailboxes(struct net_device *dev)
 		set_mb_mode_prio(priv, i, AT91_MB_MODE_TX, 0);
 
 	/* Reset tx and rx helper pointers */
-	priv->tx_next = priv->tx_echo = priv->rx_next = 0;
+	priv->tx_next = priv->tx_echo = 0;
+	priv->rx_next = AT91_MB_RX_FIRST;
 }
 
 static int at91_set_bittiming(struct net_device *dev)
@@ -590,10 +592,10 @@ static int at91_poll_rx(struct net_device *dev, int quota)
 			"order of incoming frames cannot be guaranteed\n");
 
  again:
-	for (mb = find_next_bit(addr, AT91_MB_RX_NUM, priv->rx_next);
-	     mb < AT91_MB_RX_NUM && quota > 0;
+	for (mb = find_next_bit(addr, AT91_MB_RX_LAST + 1, priv->rx_next);
+	     mb < AT91_MB_RX_LAST + 1 && quota > 0;
 	     reg_sr = at91_read(priv, AT91_SR),
-	     mb = find_next_bit(addr, AT91_MB_RX_NUM, ++priv->rx_next)) {
+	     mb = find_next_bit(addr, AT91_MB_RX_LAST + 1, ++priv->rx_next)) {
 		at91_read_msg(dev, mb);
 
 		/* reactivate mailboxes */
@@ -610,8 +612,8 @@ static int at91_poll_rx(struct net_device *dev, int quota)
 
 	/* upper group completed, look again in lower */
 	if (priv->rx_next > AT91_MB_RX_LOW_LAST &&
-	    quota > 0 && mb >= AT91_MB_RX_NUM) {
-		priv->rx_next = 0;
+	    quota > 0 && mb > AT91_MB_RX_LAST) {
+		priv->rx_next = AT91_MB_RX_FIRST;
 		goto again;
 	}
 
-- 
1.7.2.3

[PATCH 2/3] can: at91_can: don't use mailbox 0

[Marc Kleine-Budde]

Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
"AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
0 may be send under certain conditions (even if disabled or in rx mode).

The workaround in the errata suggests not to use the mailbox and load it
with a unused identifier.

This patch implements the first part of the workaround, it updates
AT91_MB_RX_NUM and AT91_MB_RX_FIRST (and the inline documentation)
so that mailbox 0 stays unused.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/at91_can.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 892c3d8..16e45a5 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -40,16 +40,16 @@
 
 #include <mach/board.h>
 
-#define AT91_NAPI_WEIGHT	12
+#define AT91_NAPI_WEIGHT	11
 
 /*
  * RX/TX Mailbox split
  * don't dare to touch
  */
-#define AT91_MB_RX_NUM		12
+#define AT91_MB_RX_NUM		11
 #define AT91_MB_TX_SHIFT	2
 
-#define AT91_MB_RX_FIRST	0
+#define AT91_MB_RX_FIRST	1
 #define AT91_MB_RX_LAST		(AT91_MB_RX_FIRST + AT91_MB_RX_NUM - 1)
 
 #define AT91_MB_RX_MASK(i)	((1 << (i)) - 1)
@@ -236,10 +236,14 @@ static void at91_setup_mailboxes(struct net_device *dev)
 	unsigned int i;
 
 	/*
-	 * The first 12 mailboxes are used as a reception FIFO. The
-	 * last mailbox is configured with overwrite option. The
-	 * overwrite flag indicates a FIFO overflow.
+	 * Due to a chip bug (errata 50.2.6.3 & 50.3.5.3) the first
+	 * mailbox is disabled. The next 11 mailboxes are used as a
+	 * reception FIFO. The last mailbox is configured with
+	 * overwrite option. The overwrite flag indicates a FIFO
+	 * overflow.
 	 */
+	for (i = 0; i < AT91_MB_RX_FIRST; i++)
+		set_mb_mode(priv, i, AT91_MB_MODE_DISABLED);
 	for (i = AT91_MB_RX_FIRST; i < AT91_MB_RX_LAST; i++)
 		set_mb_mode(priv, i, AT91_MB_MODE_RX);
 	set_mb_mode(priv, AT91_MB_RX_LAST, AT91_MB_MODE_RX_OVRWR);
@@ -541,27 +545,31 @@ static void at91_read_msg(struct net_device *dev, unsigned int mb)
  *
  * Theory of Operation:
  *
- * 12 of the 16 mailboxes on the chip are reserved for RX. we split
- * them into 2 groups. The lower group holds 8 and upper 4 mailboxes.
+ * 11 of the 16 mailboxes on the chip are reserved for RX. we split
+ * them into 2 groups. The lower group holds 7 and upper 4 mailboxes.
  *
  * Like it or not, but the chip always saves a received CAN message
  * into the first free mailbox it finds (starting with the
  * lowest). This makes it very difficult to read the messages in the
  * right order from the chip. This is how we work around that problem:
  *
- * The first message goes into mb nr. 0 and issues an interrupt. All
+ * The first message goes into mb nr. 1 and issues an interrupt. All
  * rx ints are disabled in the interrupt handler and a napi poll is
  * scheduled. We read the mailbox, but do _not_ reenable the mb (to
  * receive another message).
  *
  *    lower mbxs      upper
- *   ______^______    __^__
- *  /             \  /     \
+ *     ____^______    __^__
+ *    /           \  /     \
  * +-+-+-+-+-+-+-+-++-+-+-+-+
- * |x|x|x|x|x|x|x|x|| | | | |
+ * | |x|x|x|x|x|x|x|| | | | |
  * +-+-+-+-+-+-+-+-++-+-+-+-+
  *  0 0 0 0 0 0  0 0 0 0 1 1  \ mail
  *  0 1 2 3 4 5  6 7 8 9 0 1  / box
+ *  ^
+ *  |
+ *   \
+ *     unused, due to chip bug
  *
  * The variable priv->rx_next points to the next mailbox to read a
  * message from. As long we're in the lower mailboxes we just read the
-- 
1.7.2.3

[PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Marc Kleine-Budde]

Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
"AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
0 may be send under certain conditions (even if disabled or in rx mode).

The workaround in the errata suggests not to use the mailbox and load it
with a unused identifier.

This patch implements the second part of the workaround. A sysfs entry
"mb0_id" is introduced. While the interface is down it can be used to
configure the can_id of mailbox 0. The default value id 0x7ff.

In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
to the can_id. Example:

- standard id 0x7ff:
echo 0x7ff      > /sys/class/net/can0/mb0_id

- extended if 0x1fffffff:
echo 0x9fffffff > /sys/class/net/can0/mb0_id

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/at91_can.c |   90 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 16e45a5..2532b96 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/platform_device.h>
+#include <linux/rtnetlink.h>
 #include <linux/skbuff.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
@@ -169,6 +170,8 @@ struct at91_priv {
 
 	struct clk		*clk;
 	struct at91_can_data	*pdata;
+
+	canid_t			mb0_id;
 };
 
 static struct can_bittiming_const at91_bittiming_const = {
@@ -221,6 +224,18 @@ static inline void set_mb_mode(const struct at91_priv *priv, unsigned int mb,
 	set_mb_mode_prio(priv, mb, mode, 0);
 }
 
+static inline u32 at91_can_id_to_reg_mid(canid_t can_id)
+{
+	u32 reg_mid;
+
+	if (can_id & CAN_EFF_FLAG)
+		reg_mid = (can_id & CAN_EFF_MASK) | AT91_MID_MIDE;
+	else
+		reg_mid = (can_id & CAN_SFF_MASK) << 18;
+
+	return reg_mid;
+}
+
 /*
  * Swtich transceiver on or off
  */
@@ -234,6 +249,7 @@ static void at91_setup_mailboxes(struct net_device *dev)
 {
 	struct at91_priv *priv = netdev_priv(dev);
 	unsigned int i;
+	u32 reg_mid;
 
 	/*
 	 * Due to a chip bug (errata 50.2.6.3 & 50.3.5.3) the first
@@ -242,8 +258,13 @@ static void at91_setup_mailboxes(struct net_device *dev)
 	 * overwrite option. The overwrite flag indicates a FIFO
 	 * overflow.
 	 */
-	for (i = 0; i < AT91_MB_RX_FIRST; i++)
+	reg_mid = at91_can_id_to_reg_mid(priv->mb0_id);
+	for (i = 0; i < AT91_MB_RX_FIRST; i++) {
 		set_mb_mode(priv, i, AT91_MB_MODE_DISABLED);
+		at91_write(priv, AT91_MID(i), reg_mid);
+		at91_write(priv, AT91_MCR(i), 0x0);	/* clear dlc */
+	}
+
 	for (i = AT91_MB_RX_FIRST; i < AT91_MB_RX_LAST; i++)
 		set_mb_mode(priv, i, AT91_MB_MODE_RX);
 	set_mb_mode(priv, AT91_MB_RX_LAST, AT91_MB_MODE_RX_OVRWR);
@@ -378,12 +399,7 @@ static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		netdev_err(dev, "BUG! TX buffer full when queue awake!\n");
 		return NETDEV_TX_BUSY;
 	}
-
-	if (cf->can_id & CAN_EFF_FLAG)
-		reg_mid = (cf->can_id & CAN_EFF_MASK) | AT91_MID_MIDE;
-	else
-		reg_mid = (cf->can_id & CAN_SFF_MASK) << 18;
-
+	reg_mid = at91_can_id_to_reg_mid(cf->can_id);
 	reg_mcr = ((cf->can_id & CAN_RTR_FLAG) ? AT91_MCR_MRTR : 0) |
 		(cf->can_dlc << 16) | AT91_MCR_MTCR;
 
@@ -1047,6 +1063,64 @@ static const struct net_device_ops at91_netdev_ops = {
 	.ndo_start_xmit	= at91_start_xmit,
 };
 
+static ssize_t at91_sysfs_show_mb0_id(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct at91_priv *priv = netdev_priv(to_net_dev(dev));
+
+	if (priv->mb0_id & CAN_EFF_FLAG)
+		return snprintf(buf, PAGE_SIZE, "0x%08x\n", priv->mb0_id);
+	else
+		return snprintf(buf, PAGE_SIZE, "0x%03x\n", priv->mb0_id);
+}
+
+static ssize_t at91_sysfs_set_mb0_id(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct net_device *ndev = to_net_dev(dev);
+	struct at91_priv *priv = netdev_priv(ndev);
+	unsigned long can_id;
+	ssize_t ret;
+	int err;
+
+	rtnl_lock();
+
+	if (ndev->flags & IFF_UP) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	err = strict_strtoul(buf, 0, &can_id);
+	if (err) {
+		ret = err;
+		goto out;
+	}
+
+	if (can_id & CAN_EFF_FLAG)
+		can_id &= CAN_EFF_MASK | CAN_EFF_FLAG;
+	else
+		can_id &= CAN_SFF_MASK;
+
+	priv->mb0_id = can_id;
+	ret = count;
+
+ out:
+	rtnl_unlock();
+	return ret;
+}
+
+static DEVICE_ATTR(mb0_id, S_IWUGO | S_IRUGO,
+	at91_sysfs_show_mb0_id, at91_sysfs_set_mb0_id);
+
+static struct attribute *at91_sysfs_attrs[] = {
+	&dev_attr_mb0_id.attr,
+	NULL,
+};
+
+static struct attribute_group at91_sysfs_attr_group = {
+	.attrs = at91_sysfs_attrs,
+};
+
 static int __devinit at91_can_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
@@ -1092,6 +1166,7 @@ static int __devinit at91_can_probe(struct platform_device *pdev)
 	dev->netdev_ops	= &at91_netdev_ops;
 	dev->irq = irq;
 	dev->flags |= IFF_ECHO;
+	dev->sysfs_groups[0] = &at91_sysfs_attr_group;
 
 	priv = netdev_priv(dev);
 	priv->can.clock.freq = clk_get_rate(clk);
@@ -1103,6 +1178,7 @@ static int __devinit at91_can_probe(struct platform_device *pdev)
 	priv->dev = dev;
 	priv->clk = clk;
 	priv->pdata = pdev->dev.platform_data;
+	priv->mb0_id = 0x7ff;
 
 	netif_napi_add(dev, &priv->napi, at91_poll, AT91_NAPI_WEIGHT);
 
-- 
1.7.2.3

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Wolfgang Grandegger]

On 01/11/2011 11:28 AM, Marc Kleine-Budde wrote:
> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
> 0 may be send under certain conditions (even if disabled or in rx mode).
> 
> The workaround in the errata suggests not to use the mailbox and load it
> with a unused identifier.
> 
> This patch implements the second part of the workaround. A sysfs entry
> "mb0_id" is introduced. While the interface is down it can be used to
> configure the can_id of mailbox 0. The default value id 0x7ff.
> 
> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
> to the can_id. Example:
> 
> - standard id 0x7ff:
> echo 0x7ff      > /sys/class/net/can0/mb0_id
> 
> - extended if 0x1fffffff:
> echo 0x9fffffff > /sys/class/net/can0/mb0_id

As this is a device specific property, I think it should go into
/sys/class/net/can0/device/.

Wolfgang.

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Marc Kleine-Budde]

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

On 01/11/2011 12:45 PM, Wolfgang Grandegger wrote:
> On 01/11/2011 11:28 AM, Marc Kleine-Budde wrote:
>> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
>> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
>> 0 may be send under certain conditions (even if disabled or in rx mode).
>>
>> The workaround in the errata suggests not to use the mailbox and load it
>> with a unused identifier.
>>
>> This patch implements the second part of the workaround. A sysfs entry
>> "mb0_id" is introduced. While the interface is down it can be used to
>> configure the can_id of mailbox 0. The default value id 0x7ff.
>>
>> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
>> to the can_id. Example:
>>
>> - standard id 0x7ff:
>> echo 0x7ff      > /sys/class/net/can0/mb0_id
>>
>> - extended if 0x1fffffff:
              ^^
I've fixed the typo on my git repo. I'll send an updated series later.

>> echo 0x9fffffff > /sys/class/net/can0/mb0_id
> 
> As this is a device specific property, I think it should go into
> /sys/class/net/can0/device/.

The attribute goes autoamtically to /sys/class/net/can0 if you add it to
the driver via:

+       dev->sysfs_groups[0] = &at91_sysfs_attr_group;

I've copied this from the janz-ican3 driver[1].

regards, Marc

[1] http://lxr.linux.no/linux+v2.6.37/drivers/net/can/janz-ican3.c#L1682

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Wolfram Sang]

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

On Tue, Jan 11, 2011 at 11:28:08AM +0100, Marc Kleine-Budde wrote:
> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
> 0 may be send under certain conditions (even if disabled or in rx mode).
> 
> The workaround in the errata suggests not to use the mailbox and load it
> with a unused identifier.
> 
> This patch implements the second part of the workaround. A sysfs entry
> "mb0_id" is introduced. While the interface is down it can be used to
> configure the can_id of mailbox 0. The default value id 0x7ff.
> 
> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
> to the can_id. Example:
> 
> - standard id 0x7ff:
> echo 0x7ff      > /sys/class/net/can0/mb0_id
> 
> - extended if 0x1fffffff:
> echo 0x9fffffff > /sys/class/net/can0/mb0_id

Maybe put something like this also in Documentation/ABI/testing? Perhaps
a bit more explicit: Can this interface be used? Should it be used? Must
it be used?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Marc Kleine-Budde]

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

On 01/11/2011 12:57 PM, Wolfram Sang wrote:
> On Tue, Jan 11, 2011 at 11:28:08AM +0100, Marc Kleine-Budde wrote:
>> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
>> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
>> 0 may be send under certain conditions (even if disabled or in rx mode).
>>
>> The workaround in the errata suggests not to use the mailbox and load it
>> with a unused identifier.
>>
>> This patch implements the second part of the workaround. A sysfs entry
>> "mb0_id" is introduced. While the interface is down it can be used to
>> configure the can_id of mailbox 0. The default value id 0x7ff.
>>
>> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
>> to the can_id. Example:
>>
>> - standard id 0x7ff:
>> echo 0x7ff      > /sys/class/net/can0/mb0_id
>>
>> - extended if 0x1fffffff:
>> echo 0x9fffffff > /sys/class/net/can0/mb0_id
> 
> Maybe put something like this also in Documentation/ABI/testing? Perhaps

Good point.

> a bit more explicit: Can this interface be used? Should it be used? Must
> it be used?

It can be used if your aren't satisfied with with default id of 0x7ff.

regards, 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Wolfgang Grandegger]

On 01/11/2011 12:56 PM, Marc Kleine-Budde wrote:
> On 01/11/2011 12:45 PM, Wolfgang Grandegger wrote:
>> On 01/11/2011 11:28 AM, Marc Kleine-Budde wrote:
>>> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
>>> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
>>> 0 may be send under certain conditions (even if disabled or in rx mode).
>>>
>>> The workaround in the errata suggests not to use the mailbox and load it
>>> with a unused identifier.
>>>
>>> This patch implements the second part of the workaround. A sysfs entry
>>> "mb0_id" is introduced. While the interface is down it can be used to
>>> configure the can_id of mailbox 0. The default value id 0x7ff.
>>>
>>> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
>>> to the can_id. Example:
>>>
>>> - standard id 0x7ff:
>>> echo 0x7ff      > /sys/class/net/can0/mb0_id
>>>
>>> - extended if 0x1fffffff:
>               ^^
> I've fixed the typo on my git repo. I'll send an updated series later.
> 
>>> echo 0x9fffffff > /sys/class/net/can0/mb0_id
>>
>> As this is a device specific property, I think it should go into
>> /sys/class/net/can0/device/.
> 
> The attribute goes autoamtically to /sys/class/net/can0 if you add it to
> the driver via:
> 
> +       dev->sysfs_groups[0] = &at91_sysfs_attr_group;
> 
> I've copied this from the janz-ican3 driver[1].

Oh, I missed that. And also the Softing driver does it that way :-(. The
member has the comment:

  /* space for optional device, statistics, and wireless sysfs groups */
  const struct attribute_group *sysfs_groups[4];

Therefore it seems to be legal to use it for device specific properties.

Wolfgang.

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Marc Kleine-Budde]

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

On 01/11/2011 01:27 PM, Wolfgang Grandegger wrote:
> On 01/11/2011 12:56 PM, Marc Kleine-Budde wrote:
>> On 01/11/2011 12:45 PM, Wolfgang Grandegger wrote:
>>> On 01/11/2011 11:28 AM, Marc Kleine-Budde wrote:
>>>> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
>>>> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
>>>> 0 may be send under certain conditions (even if disabled or in rx mode).
>>>>
>>>> The workaround in the errata suggests not to use the mailbox and load it
>>>> with a unused identifier.
>>>>
>>>> This patch implements the second part of the workaround. A sysfs entry
>>>> "mb0_id" is introduced. While the interface is down it can be used to
>>>> configure the can_id of mailbox 0. The default value id 0x7ff.
>>>>
>>>> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
>>>> to the can_id. Example:
>>>>
>>>> - standard id 0x7ff:
>>>> echo 0x7ff      > /sys/class/net/can0/mb0_id
>>>>
>>>> - extended if 0x1fffffff:
>>               ^^
>> I've fixed the typo on my git repo. I'll send an updated series later.
>>
>>>> echo 0x9fffffff > /sys/class/net/can0/mb0_id
>>>
>>> As this is a device specific property, I think it should go into
>>> /sys/class/net/can0/device/.
>>
>> The attribute goes autoamtically to /sys/class/net/can0 if you add it to
>> the driver via:
>>
>> +       dev->sysfs_groups[0] = &at91_sysfs_attr_group;
>>
>> I've copied this from the janz-ican3 driver[1].
> 
> Oh, I missed that. And also the Softing driver does it that way :-(. The
> member has the comment:
> 
>   /* space for optional device, statistics, and wireless sysfs groups */
>   const struct attribute_group *sysfs_groups[4];
> 
> Therefore it seems to be legal to use it for device specific properties.

I'm not really happy with these sysfs approach, but it's quick
implemented. Is device specific rtnetlink an option here?

cheers, 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Wolfgang Grandegger]

On 01/11/2011 01:33 PM, Marc Kleine-Budde wrote:
> On 01/11/2011 01:27 PM, Wolfgang Grandegger wrote:
>> On 01/11/2011 12:56 PM, Marc Kleine-Budde wrote:
>>> On 01/11/2011 12:45 PM, Wolfgang Grandegger wrote:
>>>> On 01/11/2011 11:28 AM, Marc Kleine-Budde wrote:
>>>>> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
>>>>> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
>>>>> 0 may be send under certain conditions (even if disabled or in rx mode).
>>>>>
>>>>> The workaround in the errata suggests not to use the mailbox and load it
>>>>> with a unused identifier.
>>>>>
>>>>> This patch implements the second part of the workaround. A sysfs entry
>>>>> "mb0_id" is introduced. While the interface is down it can be used to
>>>>> configure the can_id of mailbox 0. The default value id 0x7ff.
>>>>>
>>>>> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
>>>>> to the can_id. Example:
>>>>>
>>>>> - standard id 0x7ff:
>>>>> echo 0x7ff      > /sys/class/net/can0/mb0_id
>>>>>
>>>>> - extended if 0x1fffffff:
>>>               ^^
>>> I've fixed the typo on my git repo. I'll send an updated series later.
>>>
>>>>> echo 0x9fffffff > /sys/class/net/can0/mb0_id
>>>>
>>>> As this is a device specific property, I think it should go into
>>>> /sys/class/net/can0/device/.
>>>
>>> The attribute goes autoamtically to /sys/class/net/can0 if you add it to
>>> the driver via:
>>>
>>> +       dev->sysfs_groups[0] = &at91_sysfs_attr_group;
>>>
>>> I've copied this from the janz-ican3 driver[1].
>>
>> Oh, I missed that. And also the Softing driver does it that way :-(. The
>> member has the comment:
>>
>>   /* space for optional device, statistics, and wireless sysfs groups */
>>   const struct attribute_group *sysfs_groups[4];
>>
>> Therefore it seems to be legal to use it for device specific properties.
> 
> I'm not really happy with these sysfs approach, but it's quick
> implemented. Is device specific rtnetlink an option here?

That's toooooooo heavy, I think. I personally would just use
device_create_file for that purpose. But let's keep using sysfs_groups[]
if nobody else complains.

Wolfgang.

Wolfgang.

Re: [PATCH 3/3] can: at91_can: make can_id of mailbox 0 configurable

[Kurt Van Dijck]

On Tue, Jan 11, 2011 at 02:28:36PM +0100, Wolfgang Grandegger wrote:
> 
> On 01/11/2011 01:33 PM, Marc Kleine-Budde wrote:
> > On 01/11/2011 01:27 PM, Wolfgang Grandegger wrote:
> >> On 01/11/2011 12:56 PM, Marc Kleine-Budde wrote:
> >>> On 01/11/2011 12:45 PM, Wolfgang Grandegger wrote:
> >>>> On 01/11/2011 11:28 AM, Marc Kleine-Budde wrote:
> >>>>> Due to a chip bug (errata 50.2.6.3 & 50.3.5.3 in
> >>>>> "AT91SAM9263 Preliminary 6249H-ATARM-27-Jul-09") the contents of mailbox
> >>>>> 0 may be send under certain conditions (even if disabled or in rx mode).
> >>>>>
> >>>>> The workaround in the errata suggests not to use the mailbox and load it
> >>>>> with a unused identifier.
> >>>>>
> >>>>> This patch implements the second part of the workaround. A sysfs entry
> >>>>> "mb0_id" is introduced. While the interface is down it can be used to
> >>>>> configure the can_id of mailbox 0. The default value id 0x7ff.
> >>>>>
> >>>>> In order to use an extended can_id add the CAN_EFF_FLAG (0x80000000U)
> >>>>> to the can_id. Example:
> >>>>>
> >>>>> - standard id 0x7ff:
> >>>>> echo 0x7ff      > /sys/class/net/can0/mb0_id
> >>>>>
> >>>>> - extended if 0x1fffffff:
> >>>               ^^
> >>> I've fixed the typo on my git repo. I'll send an updated series later.
> >>>
> >>>>> echo 0x9fffffff > /sys/class/net/can0/mb0_id
> >>>>
> >>>> As this is a device specific property, I think it should go into
> >>>> /sys/class/net/can0/device/.
> >>>
> >>> The attribute goes autoamtically to /sys/class/net/can0 if you add it to
> >>> the driver via:
> >>>
> >>> +       dev->sysfs_groups[0] = &at91_sysfs_attr_group;
> >>>
> >>> I've copied this from the janz-ican3 driver[1].
> >>
> >> Oh, I missed that. And also the Softing driver does it that way :-(. The
> >> member has the comment:
> >>
> >>   /* space for optional device, statistics, and wireless sysfs groups */
> >>   const struct attribute_group *sysfs_groups[4];
> >>
> >> Therefore it seems to be legal to use it for device specific properties.
> > 
> > I'm not really happy with these sysfs approach, but it's quick
> > implemented. Is device specific rtnetlink an option here?
> 
> That's toooooooo heavy, I think. I personally would just use
> device_create_file for that purpose. But let's keep using sysfs_groups[]
> if nobody else complains.

sysfs_groups[0] has the advantage that it's ready during the uevent
(ie. for use in udev). device_create_file() may get online after the uevent...

Kurt