can: janz-ican3: add support for CAL/CANopen firmware

in our department we are using some older Janz ICAN3-modules in our dekstop pcs. There we have slightly different carrier boards than the janz-cmodio supported in the kernel sources, called CAN-PCI2 with two submodules. But the pci configuration regions are identical. So extending the supported pci devices to the corresponding device ids is sufficient to get the drivers working. Great.

I have two minor issues:
* The old ICAN3-modules with firmware 1.28 need more then 250ms for the restart after reset. I've increased the timeout to 500ms.

* The janz_ican3 module uses the raw can services of the Janz-firmware, this means firmware must be ICANOS/2. Our ICAN3-modules are equipped with CAL/CANopen-firmware, so I must use the appropriate commands for the layer management services. My proposal: the driver detects the firmware after module reset and selects the commands matching the firmware. This affects the bus on/off-command (ican3_set_bus_state) and the configuration of the bittiming (ican3_set_bittiming). For better diagnostics the detected

firmware string is presented as sysfs attribute (fwinfo).

Currently my system works great. Are there any possibilities to get my changes into the linux kernel? Here are my changes based on a linux-3.19.6 vanilla kernel. Is this the right place for my request, is there any maintainer I can ask? I would be grateful for any help.

---
 drivers/mfd/janz-cmodio.c    |    4 +
 drivers/net/can/janz-ican3.c |   97 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 84 insertions(+), 17 deletions(-)

diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/drivers/mfd/janz-cmodio.c linux-3.19.me/drivers/mfd/janz-cmodio.c

drivers/mfd/janz-cmodio.c +4 -0

@@ -267,6 +267,10 @@
 static const struct pci_device_id cmodio_pci_ids[] = {
 	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0101 },
 	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0100 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0201 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0202 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0201 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0202 },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, cmodio_pci_ids);

diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/drivers/net/can/janz-ican3.c linux-3.19.me/drivers/net/can/janz-ican3.c

drivers/net/can/janz-ican3.c +1 -0

@@ -83,6 +83,7 @@
 #define MSG_COFFREQ		0x42
 #define MSG_CONREQ		0x43
 #define MSG_CCONFREQ		0x47
+#define MSG_LMTS		0xb4
 
 /*
  * Janz ICAN3 CAN Inquiry Message Types

drivers/net/can/janz-ican3.c +21 -0

+
 	/* old and new style host interface */
 	unsigned int iftype;
 

@@ -270,6 +275,26 @@
 }
 
 /*
+ * This routine was stolen from drivers/net/can/sja1000/sja1000.c
+ *
+ * The bittiming register command for the ICAN3 just sets the bit timing
+ * registers on the SJA1000 chip directly
+ */
+static inline void ican3_calc_bittiming(struct ican3_dev *mod, u8 *btr0, u8 *btr1)
+{
+	struct can_bittiming *bt = &mod->can.bittiming;
+
+	if (!btr0 || !btr1)
+		return;
+
+	*btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
+	*btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
+		(((bt->phase_seg2 - 1) & 0x7) << 4);
+	if (mod->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		*btr1 |= 0x80;
+}
+
+/*
  * ICAN3 "old-style" host interface
  */
 

@@ -751,12 +776,37 @@
 static int ican3_set_bus_state(struct ican3_dev *mod, bool on)

drivers/net/can/janz-ican3.c +18 -1

+		msg.spec = MSG_LMTS;
+		if (on) {
+			ican3_calc_bittiming(mod, &btr0, &btr1);
+			msg.len = cpu_to_le16(4);
+			msg.data[0] = 0;
+			msg.data[1] = 0;
+			msg.data[2] = btr0;
+			msg.data[3] = btr1;
+		} else {
+			msg.len = cpu_to_le16(2);
+			msg.data[0] = 1;
+			msg.data[1] = 0;
+		}
+
+		return ican3_send_msg(mod, &msg);
+	}
+	return -ENOTSUPP;
 }
 
 static int ican3_set_termination(struct ican3_dev *mod, bool on)

@@ -1401,7 +1451,7 @@
 			return 0;
 
 		msleep(10);
-	} while (time_before(jiffies, start + HZ / 4));
+	} while (time_before(jiffies, start + HZ / 2));
 
 	netdev_err(mod->ndev, "failed to reset CAN module\n");
 	return -ETIMEDOUT;

@@ -1426,6 +1476,15 @@
 		return ret;
 	}

drivers/net/can/janz-ican3.c +17 -14

 	/* re-enable interrupts so we can send messages */
 	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
 

@@ -1614,24 +1673,17 @@
 	.brp_inc = 1,
 };
 
-/*
- * This routine was stolen from drivers/net/can/sja1000/sja1000.c
- *
- * The bittiming register command for the ICAN3 just sets the bit timing
- * registers on the SJA1000 chip directly
- */
 static int ican3_set_bittiming(struct net_device *ndev)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
-	struct can_bittiming *bt = &mod->can.bittiming;
 	struct ican3_msg msg;
 	u8 btr0, btr1;
 
-	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
-	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
-		(((bt->phase_seg2 - 1) & 0x7) << 4);
-	if (mod->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
-		btr1 |= 0x80;
+	/* with CAL firmware bittiming is set in ican3_set_bus_state  */
+	if (mod->fwtype == 1)
+		return 0;
+
+	ican3_calc_bittiming(mod, &btr0, &btr1);
 
 	memset(&msg, 0, sizeof(msg));
 	msg.spec = MSG_CBTRREQ;

@@ -1731,11 +1783,22 @@
 	return count;
 }
 
+static ssize_t ican3_sysfs_show_fwinfo(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct ican3_dev *mod = netdev_priv(to_net_dev(dev));
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", mod->fwinfo);
+}
+
 static DEVICE_ATTR(termination, S_IWUSR | S_IRUGO, ican3_sysfs_show_term,
 						   ican3_sysfs_set_term);
+static DEVICE_ATTR(fwinfo, S_IRUSR | S_IRUGO, ican3_sysfs_show_fwinfo, NULL);
 
 static struct attribute *ican3_sysfs_attrs[] = {
 	&dev_attr_termination.attr,
+	&dev_attr_fwinfo.attr,
 	NULL,
 };
 

@@ -1867,7 +1930,7 @@
 		goto out_free_irq;
 	}
 
-	dev_info(dev, "module %d: registered CAN device\n", pdata->modno);
+	netdev_info(mod->ndev, "module %d: registered CAN device\n", pdata->modno);
 	return 0;
 
 out_free_irq:
--

To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Andreas Gröger

in our department we are using some older Janz ICAN3-modules in our dekstop pcs. There we have slightly different carrier boards than the janz-cmodio supported in the kernel sources, called CAN-PCI2 with two submodules. But the pci configuration regions are identical. So extending the supported pci devices to the corresponding device ids is sufficient to get the drivers working. Great.

I have two minor issues:
* The old ICAN3-modules with firmware 1.28 need more then 250ms for the restart after reset. I've increased the timeout to 500ms.

Should be no problem, as it's a timeout.

Quoted 8 lines

* The janz_ican3 module uses the raw can services of the Janz-firmware, this means firmware must be ICANOS/2. Our ICAN3-modules are equipped with CAL/CANopen-firmware, so I must use the appropriate commands for the layer management services. My proposal: the driver detects the firmware after module reset and selects the commands matching the firmware. This affects the bus on/off-command (ican3_set_bus_state) and the configuration of the bittiming (ican3_set_bittiming). For better diagnostics the detected

Is it possible to move the set_bittiming for both firmwares into the ican3_set_bus_state() function? For various reasons I don't like the fact that the bitrate is written into the hardware while the interface is down.

Quoted 1 line

firmware string is presented as sysfs attribute (fwinfo).

Please document the new sysfs entry, see Documentation/ABI/testing/sysfs-platform-at91 as an example. Please be so kind and document the already existing termination attribute, too.

Quoted 4 lines

Currently my system works great. Are there any possibilities to get my changes into the linux kernel? Here are my changes based on a linux-3.19.6 vanilla kernel. Is this the right place for my request, is there any maintainer I can ask? I would be grateful for any help.

Ira, can you have a look at the changes and give your Acked-by?

The changes look quite good, some minor nitpicks inline. In the next patch iteration add add your S-o-b, see [1].

[1]
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L407

Quoted 31 lines

---
 drivers/mfd/janz-cmodio.c    |    4 +
 drivers/net/can/janz-ican3.c |   97 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 84 insertions(+), 17 deletions(-)

diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/drivers/mfd/janz-cmodio.c linux-3.19.me/drivers/mfd/janz-cmodio.c

drivers/mfd/janz-cmodio.c +4 -0

@@ -267,6 +267,10 @@
 static const struct pci_device_id cmodio_pci_ids[] = {
 	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0101 },
 	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0100 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0201 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0202 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0201 },
+	{ PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0202 },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, cmodio_pci_ids);

diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/drivers/net/can/janz-ican3.c linux-3.19.me/drivers/net/can/janz-ican3.c

drivers/net/can/janz-ican3.c +1 -0

@@ -83,6 +83,7 @@
 #define MSG_COFFREQ		0x42
 #define MSG_CONREQ		0x43
 #define MSG_CCONFREQ		0x47
+#define MSG_LMTS		0xb4
 
 /*
  * Janz ICAN3 CAN Inquiry Message Types

drivers/net/can/janz-ican3.c line 220

Quoted 5 lines

drivers/net/can/janz-ican3.c +2 -0

@@ -215,6 +216,10 @@
 	struct completion buserror_comp;
 	struct can_berr_counter bec;
 
+	/* firmware type: 0=ICANOS, 1=CAN/CALopen */
+	unsigned int fwtype;

please create an enum, make it readable, so that you don't need the comments anymore :) Something like this:

enum ican3_fwtype {
ICAN3_FWTYPE_ICANOS, // or RAW
ICAN3_FWTYPE_CALOPEN,
};

drivers/net/can/janz-ican3.c line 221

Quoted 1 line

drivers/net/can/janz-ican3.c +1 -0

+	char fwinfo[0x20];

Nitpick, I'd prefer a plain decimal 32 instead. Where does the length come from?

Quoted 33 lines

drivers/net/can/janz-ican3.c +21 -0

+
 	/* old and new style host interface */
 	unsigned int iftype;
 

@@ -270,6 +275,26 @@
 }
 
 /*
+ * This routine was stolen from drivers/net/can/sja1000/sja1000.c
+ *
+ * The bittiming register command for the ICAN3 just sets the bit timing
+ * registers on the SJA1000 chip directly
+ */
+static inline void ican3_calc_bittiming(struct ican3_dev *mod, u8 *btr0, u8 *btr1)
+{
+	struct can_bittiming *bt = &mod->can.bittiming;
+
+	if (!btr0 || !btr1)
+		return;
+
+	*btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
+	*btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
+		(((bt->phase_seg2 - 1) & 0x7) << 4);
+	if (mod->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		*btr1 |= 0x80;
+}
+
+/*
  * ICAN3 "old-style" host interface
  */
 

@@ -751,12 +776,37 @@
 static int ican3_set_bus_state(struct ican3_dev *mod, bool on)

drivers/net/can/janz-ican3.c line 783

Quoted 10 lines

drivers/net/can/janz-ican3.c +2 -3

 {
 	struct ican3_msg msg;
+	u8 btr0, btr1;
 
 	memset(&msg, 0, sizeof(msg));
-	msg.spec = on ? MSG_CONREQ : MSG_COFFREQ;
-	msg.len = cpu_to_le16(0);
 
-	return ican3_send_msg(mod, &msg);
+	/* raw CAN firmware */

make use of the enum

drivers/net/can/janz-ican3.c line 792

Quoted 9 lines

drivers/net/can/janz-ican3.c +9 -0

+	if (mod->fwtype == 0) {
+		msg.spec = on ? MSG_CONREQ : MSG_COFFREQ;
+		msg.len = cpu_to_le16(0);
+
+		return ican3_send_msg(mod, &msg);
+	}
+
+	/* CAL firmware */
+	if (mod->fwtype == 1) {

else if + use enum

Quoted 27 lines

drivers/net/can/janz-ican3.c +18 -1

+		msg.spec = MSG_LMTS;
+		if (on) {
+			ican3_calc_bittiming(mod, &btr0, &btr1);
+			msg.len = cpu_to_le16(4);
+			msg.data[0] = 0;
+			msg.data[1] = 0;
+			msg.data[2] = btr0;
+			msg.data[3] = btr1;
+		} else {
+			msg.len = cpu_to_le16(2);
+			msg.data[0] = 1;
+			msg.data[1] = 0;
+		}
+
+		return ican3_send_msg(mod, &msg);
+	}
+	return -ENOTSUPP;
 }
 
 static int ican3_set_termination(struct ican3_dev *mod, bool on)

@@ -1401,7 +1451,7 @@
 			return 0;
 
 		msleep(10);
-	} while (time_before(jiffies, start + HZ / 4));
+	} while (time_before(jiffies, start + HZ / 2));
 

drivers/net/can/janz-ican3.c line 1480

Quoted 8 lines

drivers/net/can/janz-ican3.c +2 -0

 	netdev_err(mod->ndev, "failed to reset CAN module\n");
 	return -ETIMEDOUT;

@@ -1426,6 +1476,15 @@
 		return ret;
 	}
 
+	/* detect firmware */
+	memcpy_fromio(mod->fwinfo, mod->dpm + 0x60, sizeof(mod->fwinfo) - 1);

drivers/net/can/janz-ican3.c line 1487

Quoted 7 lines

drivers/net/can/janz-ican3.c +7 -0

+	if (strncmp(mod->fwinfo, "JANZ-ICAN3", 10)) {
+		netdev_err(mod->ndev, "ICAN3 not detected (found %s)\n", mod->fwinfo);
+		return -ENODEV;
+	}
+	if (strstr(mod->fwinfo, "CAL/CANopen"))
+		mod->fwtype = 1;
+

Is there a saner way of identifying one or the other card than doing these string comparisons?

Quoted 66 lines

drivers/net/can/janz-ican3.c +17 -14

 	/* re-enable interrupts so we can send messages */
 	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
 

@@ -1614,24 +1673,17 @@
 	.brp_inc = 1,
 };
 
-/*
- * This routine was stolen from drivers/net/can/sja1000/sja1000.c
- *
- * The bittiming register command for the ICAN3 just sets the bit timing
- * registers on the SJA1000 chip directly
- */
 static int ican3_set_bittiming(struct net_device *ndev)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
-	struct can_bittiming *bt = &mod->can.bittiming;
 	struct ican3_msg msg;
 	u8 btr0, btr1;
 
-	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
-	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
-		(((bt->phase_seg2 - 1) & 0x7) << 4);
-	if (mod->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
-		btr1 |= 0x80;
+	/* with CAL firmware bittiming is set in ican3_set_bus_state  */
+	if (mod->fwtype == 1)
+		return 0;
+
+	ican3_calc_bittiming(mod, &btr0, &btr1);
 
 	memset(&msg, 0, sizeof(msg));
 	msg.spec = MSG_CBTRREQ;

@@ -1731,11 +1783,22 @@
 	return count;
 }
 
+static ssize_t ican3_sysfs_show_fwinfo(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct ican3_dev *mod = netdev_priv(to_net_dev(dev));
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", mod->fwinfo);
+}
+
 static DEVICE_ATTR(termination, S_IWUSR | S_IRUGO, ican3_sysfs_show_term,
 						   ican3_sysfs_set_term);
+static DEVICE_ATTR(fwinfo, S_IRUSR | S_IRUGO, ican3_sysfs_show_fwinfo, NULL);
 
 static struct attribute *ican3_sysfs_attrs[] = {
 	&dev_attr_termination.attr,
+	&dev_attr_fwinfo.attr,
 	NULL,
 };
 

@@ -1867,7 +1930,7 @@
 		goto out_free_irq;
 	}
 
-	dev_info(dev, "module %d: registered CAN device\n", pdata->modno);
+	netdev_info(mod->ndev, "module %d: registered CAN device\n", pdata->modno);
 	return 0;
 
 out_free_irq:
--

Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marc

Signature · 4 lines

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

Quoted 34 lines · Marc Kleine-Budde

Quoted 10 lines · Andreas Gröger

in our department we are using some older Janz ICAN3-modules in our dekstop pcs. There we have slightly different carrier boards than the janz-cmodio supported in the kernel sources, called CAN-PCI2 with two submodules. But the pci configuration regions are identical. So extending the supported pci devices to the corresponding device ids is sufficient to get the drivers working. Great.

I have two minor issues:
* The old ICAN3-modules with firmware 1.28 need more then 250ms for the restart after reset. I've increased the timeout to 500ms.

Should be no problem, as it's a timeout.

Quoted 8 lines

* The janz_ican3 module uses the raw can services of the Janz-firmware, this means firmware must be ICANOS/2. Our ICAN3-modules are equipped with CAL/CANopen-firmware, so I must use the appropriate commands for the layer management services. My proposal: the driver detects the firmware after module reset and selects the commands matching the firmware. This affects the bus on/off-command (ican3_set_bus_state) and the configuration of the bittiming (ican3_set_bittiming). For better diagnostics the detected

Is it possible to move the set_bittiming for both firmwares into the ican3_set_bus_state() function? For various reasons I don't like the fact that the bitrate is written into the hardware while the interface is down.

Quoted 1 line

firmware string is presented as sysfs attribute (fwinfo).

Please document the new sysfs entry, see Documentation/ABI/testing/sysfs-platform-at91 as an example. Please be so kind and document the already existing termination attribute, too.

Quoted 7 lines

Quoted 4 lines

Currently my system works great. Are there any possibilities to get my changes into the linux kernel? Here are my changes based on a linux-3.19.6 vanilla kernel. Is this the right place for my request, is there any maintainer I can ask? I would be grateful for any help.

Ira, can you have a look at the changes and give your Acked-by?

Hi Andreas, Hi Marc,

The changes look great to me. I no longer work for Caltech, and so I don't have access to the hardware to test the changes. They are very self contained, and so are unlikely to cause any problems.

I have one more nitpick in addition to Marc's, which I added inline below.

Great job on your first patch!

sorry, my S-o-b was invalid.
---
Hi Marc, Hi Ira,

thank you very much for the positive and constructive responses, seems to be the right way.

String comparison for firmware identification: there is another way to retrieve the id and version (IDVERS-request), but it is more complicated. You have to await the response and the result is a longer string about 200 bytes. Janz recommends this firmware stamp for module identification.

Bittiming in ican3_set_bus_state: this affects the driver with ICANOS-firmware. So I asked Janz for the ICANOS-firmware to be able to test. I tested both firmware-variants with different baudrates:
went as expected. Hope I met your needs.

Kind regards, Andreas

Signed-off-by Andreas Gröger <[email protected]>