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

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

[Andreas Gröger]

Hello all,

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.

Kind regards
Andreas

---
 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
--- linux-3.19.6/drivers/mfd/janz-cmodio.c	2015-04-29 10:30:26.000000000 +0200
+++ linux-3.19.me/drivers/mfd/janz-cmodio.c	2015-04-30 19:24:44.729193534 +0200
@@ -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
--- linux-3.19.6/drivers/net/can/janz-ican3.c	2015-04-29 10:30:26.000000000 +0200
+++ linux-3.19.me/drivers/net/can/janz-ican3.c	2015-04-30 19:54:20.223146411 +0200
@@ -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
@@ -215,6 +216,10 @@
 	struct completion buserror_comp;
 	struct can_berr_counter bec;
 
+	/* firmware type: 0=ICANOS, 1=CAN/CALopen */
+	unsigned int fwtype;
+	char fwinfo[0x20];
+
 	/* 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)
 {
 	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 */
+	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) {
+		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;
 	}
 
+	/* detect firmware */
+	memcpy_fromio(mod->fwinfo, mod->dpm + 0x60, sizeof(mod->fwinfo) - 1);
+	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;
+
 	/* 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:

Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Marc Kleine-Budde]

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

On 05/01/2015 08:31 PM, Andreas Gröger wrote:
> 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.

> * 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.

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

> 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

> ---
>  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
> --- linux-3.19.6/drivers/mfd/janz-cmodio.c	2015-04-29 10:30:26.000000000 +0200
> +++ linux-3.19.me/drivers/mfd/janz-cmodio.c	2015-04-30 19:24:44.729193534 +0200
> @@ -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
> --- linux-3.19.6/drivers/net/can/janz-ican3.c	2015-04-29 10:30:26.000000000 +0200
> +++ linux-3.19.me/drivers/net/can/janz-ican3.c	2015-04-30 19:54:20.223146411 +0200
> @@ -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
> @@ -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,
};

> +	char fwinfo[0x20];

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

> +
>  	/* 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)
>  {
>  	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
> +	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

> +		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;
>  	}
>  
> +	/* detect firmware */
> +	memcpy_fromio(mod->fwinfo, mod->dpm + 0x60, sizeof(mod->fwinfo) - 1);
> +	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?

>  	/* 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Ira W. Snyder]

On Fri, May 01, 2015 at 10:21:57PM +0200, Marc Kleine-Budde wrote:
> On 05/01/2015 08:31 PM, Andreas Gröger wrote:
> > 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.
> 
> > * 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.
> 
> > 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.
> 
> > 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!

Acked-by: Ira W. Snyder <ira.snyder@gmail.com>

Thanks,
Ira

> 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
> 
> > ---
> >  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
> > --- linux-3.19.6/drivers/mfd/janz-cmodio.c	2015-04-29 10:30:26.000000000 +0200
> > +++ linux-3.19.me/drivers/mfd/janz-cmodio.c	2015-04-30 19:24:44.729193534 +0200
> > @@ -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
> > --- linux-3.19.6/drivers/net/can/janz-ican3.c	2015-04-29 10:30:26.000000000 +0200
> > +++ linux-3.19.me/drivers/net/can/janz-ican3.c	2015-04-30 19:54:20.223146411 +0200
> > @@ -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
> > @@ -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,
> };
> 
> > +	char fwinfo[0x20];
> 
> Nitpick, I'd prefer a plain decimal 32 instead. Where does the length
> come from?
> 
> > +
> >  	/* 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)
> >  {
> >  	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
> > +	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
> 
> > +		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;
> >  	}
> >  
> > +	/* detect firmware */
> > +	memcpy_fromio(mod->fwinfo, mod->dpm + 0x60, sizeof(mod->fwinfo) - 1);

Please create a #define for the 0x60 here. Using the name from the
manual is fine. Look at MSYNC_PEER, MSYNC_LOCL, etc. for examples.

> > +	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?
> 
> >  	/* 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 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   |
> 

Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Marc Kleine-Budde]

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

On 05/02/2015 06:40 PM, Ira W. Snyder wrote:
> On Fri, May 01, 2015 at 10:21:57PM +0200, Marc Kleine-Budde wrote:
>> On 05/01/2015 08:31 PM, Andreas Gröger wrote:
>>> 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.
>>
>>> * 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.
>>
>>> 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.
>>
>>> 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.

Good catch.

> Great job on your first patch!

ACK.

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

Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Andreas Gröger]

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 <andreas24groeger@google.com>
---
 Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio |    8 
 Documentation/ABI/testing/sysfs-class-net-janz-ican3        |   21 ++
 drivers/mfd/janz-cmodio.c                                   |    4 
 drivers/net/can/janz-ican3.c                                |  125 ++++++++----
 4 files changed, 121 insertions(+), 37 deletions(-)

diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio linux-3.19.me/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio
--- linux-3.19.6/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio	1970-01-01 01:00:00.000000000 +0100
+++ linux-3.19.me/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio	2015-05-04 15:44:33.056595249 +0200
@@ -0,0 +1,8 @@
+What:		/sys/bus/pci/drivers/janz-cmodio/.../modulbus_number
+Date:		May 2010
+KernelVersion:	2.6.35
+Contact:	Ira W. Snyder <ira.snyder@gmail.com>
+Description:
+		Value representing the HEX switch S2 of the janz carrier board CMOD-IO or CAN-PCI2
+
+		Read-only: value of the configuration switch (0..15)
diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/Documentation/ABI/testing/sysfs-class-net-janz-ican3 linux-3.19.me/Documentation/ABI/testing/sysfs-class-net-janz-ican3
--- linux-3.19.6/Documentation/ABI/testing/sysfs-class-net-janz-ican3	1970-01-01 01:00:00.000000000 +0100
+++ linux-3.19.me/Documentation/ABI/testing/sysfs-class-net-janz-ican3	2015-05-04 17:38:38.527615571 +0200
@@ -0,0 +1,21 @@
+What:		/sys/class/net/<iface>/termination
+Date:		May 2010
+KernelVersion:	2.6.35
+Contact:	Ira W. Snyder <ira.snyder@gmail.com>
+Description:
+		Value representing the can bus termination
+
+		Default: 1 (termination active)
+		Reading: get actual termination state
+		Writing: set actual termination state (0=no termination, 1=termination active)
+
+What:		/sys/class/net/<iface>/fwinfo
+Date:		May 2015
+KernelVersion:	3.19
+Contact:	Andreas Gröger <andreas24groeger@gmail.com>
+Description:
+		Firmware stamp of ican3 module
+		Read-only: 32 byte string identification of the ICAN3 module
+		(known values: "JANZ-ICAN3 ICANOS 1.xx", "JANZ-ICAN3 CAL/CANopen 1.xx")
+
+
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
--- linux-3.19.6/drivers/mfd/janz-cmodio.c	2015-04-29 10:30:26.000000000 +0200
+++ linux-3.19.me/drivers/mfd/janz-cmodio.c	2015-05-04 17:34:00.044635384 +0200
@@ -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
--- linux-3.19.6/drivers/net/can/janz-ican3.c	2015-04-29 10:30:26.000000000 +0200
+++ linux-3.19.me/drivers/net/can/janz-ican3.c	2015-05-04 17:34:00.044635384 +0200
@@ -40,6 +40,7 @@
 #define MSYNC_PEER		0x00		/* ICAN only */
 #define MSYNC_LOCL		0x01		/* host only */
 #define TARGET_RUNNING		0x02
+#define FIRMWARE_STAMP		0x60		/* big endian firmware stamp */
 
 #define MSYNC_RB0		0x01
 #define MSYNC_RB1		0x02
@@ -83,6 +84,7 @@
 #define MSG_COFFREQ		0x42
 #define MSG_CONREQ		0x43
 #define MSG_CCONFREQ		0x47
+#define MSG_LMTS		0xb4
 
 /*
  * Janz ICAN3 CAN Inquiry Message Types
@@ -165,6 +167,12 @@
 /* SJA1000 Clock Input */
 #define ICAN3_CAN_CLOCK		8000000
 
+/* Janz ICAN3 firmware types */
+enum ican3_fwtype {
+	ICAN3_FWTYPE_ICANOS,
+	ICAN3_FWTYPE_CAL_CANOPEN
+};
+
 /* Driver Name */
 #define DRV_NAME "janz-ican3"
 
@@ -215,6 +223,10 @@
 	struct completion buserror_comp;
 	struct can_berr_counter bec;
 
+	/* firmware type */
+	enum ican3_fwtype fwtype;
+	char fwinfo[32];
+
 	/* old and new style host interface */
 	unsigned int iftype;
 
@@ -750,13 +762,61 @@
  */
 static int ican3_set_bus_state(struct ican3_dev *mod, bool on)
 {
+	struct can_bittiming *bt = &mod->can.bittiming;
 	struct ican3_msg msg;
+	u8 btr0, btr1;
+	int res;
 
-	memset(&msg, 0, sizeof(msg));
-	msg.spec = on ? MSG_CONREQ : MSG_COFFREQ;
-	msg.len = cpu_to_le16(0);
+	/* This algorithm 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                                */
+	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;
 
-	return ican3_send_msg(mod, &msg);
+	if (mod->fwtype == ICAN3_FWTYPE_ICANOS) {
+		if (on) {
+			/* set bittiming */
+			memset(&msg, 0, sizeof(msg));
+			msg.spec = MSG_CBTRREQ;
+			msg.len = cpu_to_le16(4);
+			msg.data[0] = 0x00;
+			msg.data[1] = 0x00;
+			msg.data[2] = btr0;
+			msg.data[3] = btr1;
+
+			res = ican3_send_msg(mod, &msg);
+			if (res)
+				return res;
+		}
+
+		/* can-on/off request */
+		memset(&msg, 0, sizeof(msg));
+		msg.spec = on ? MSG_CONREQ : MSG_COFFREQ;
+		msg.len = cpu_to_le16(0);
+
+		return ican3_send_msg(mod, &msg);
+
+	} else if (mod->fwtype == ICAN3_FWTYPE_CAL_CANOPEN) {
+		memset(&msg, 0, sizeof(msg));
+		msg.spec = MSG_LMTS;
+		if (on) {
+			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 +1461,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 +1486,17 @@
 		return ret;
 	}
 
+	/* detect firmware */
+	memcpy_fromio(mod->fwinfo, mod->dpm + FIRMWARE_STAMP, sizeof(mod->fwinfo) - 1);
+	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 = ICAN3_FWTYPE_CAL_CANOPEN;
+	else
+		mod->fwtype = ICAN3_FWTYPE_ICANOS;
+
 	/* re-enable interrupts so we can send messages */
 	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
 
@@ -1614,36 +1685,6 @@
 	.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;
-
-	memset(&msg, 0, sizeof(msg));
-	msg.spec = MSG_CBTRREQ;
-	msg.len = cpu_to_le16(4);
-	msg.data[0] = 0x00;
-	msg.data[1] = 0x00;
-	msg.data[2] = btr0;
-	msg.data[3] = btr1;
-
-	return ican3_send_msg(mod, &msg);
-}
-
 static int ican3_set_mode(struct net_device *ndev, enum can_mode mode)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
@@ -1731,11 +1772,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,
 };
 
@@ -1795,7 +1847,6 @@
 
 	mod->can.clock.freq = ICAN3_CAN_CLOCK;
 	mod->can.bittiming_const = &ican3_bittiming_const;
-	mod->can.do_set_bittiming = ican3_set_bittiming;
 	mod->can.do_set_mode = ican3_set_mode;
 	mod->can.do_get_berr_counter = ican3_get_berr_counter;
 	mod->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES
@@ -1867,7 +1918,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:

Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Andreas Gröger]

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 <andreas24groeger@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio |    8 
 Documentation/ABI/testing/sysfs-class-net-janz-ican3        |   21 ++
 drivers/mfd/janz-cmodio.c                                   |    4 
 drivers/net/can/janz-ican3.c                                |  125 ++++++++----
 4 files changed, 121 insertions(+), 37 deletions(-)

diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio linux-3.19.me/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio
--- linux-3.19.6/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio	1970-01-01 01:00:00.000000000 +0100
+++ linux-3.19.me/Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio	2015-05-04 15:44:33.056595249 +0200
@@ -0,0 +1,8 @@
+What:		/sys/bus/pci/drivers/janz-cmodio/.../modulbus_number
+Date:		May 2010
+KernelVersion:	2.6.35
+Contact:	Ira W. Snyder <ira.snyder@gmail.com>
+Description:
+		Value representing the HEX switch S2 of the janz carrier board CMOD-IO or CAN-PCI2
+
+		Read-only: value of the configuration switch (0..15)
diff -uNr -X linux-3.19.me/Documentation/dontdiff linux-3.19.6/Documentation/ABI/testing/sysfs-class-net-janz-ican3 linux-3.19.me/Documentation/ABI/testing/sysfs-class-net-janz-ican3
--- linux-3.19.6/Documentation/ABI/testing/sysfs-class-net-janz-ican3	1970-01-01 01:00:00.000000000 +0100
+++ linux-3.19.me/Documentation/ABI/testing/sysfs-class-net-janz-ican3	2015-05-04 17:38:38.527615571 +0200
@@ -0,0 +1,21 @@
+What:		/sys/class/net/<iface>/termination
+Date:		May 2010
+KernelVersion:	2.6.35
+Contact:	Ira W. Snyder <ira.snyder@gmail.com>
+Description:
+		Value representing the can bus termination
+
+		Default: 1 (termination active)
+		Reading: get actual termination state
+		Writing: set actual termination state (0=no termination, 1=termination active)
+
+What:		/sys/class/net/<iface>/fwinfo
+Date:		May 2015
+KernelVersion:	3.19
+Contact:	Andreas Gröger <andreas24groeger@gmail.com>
+Description:
+		Firmware stamp of ican3 module
+		Read-only: 32 byte string identification of the ICAN3 module
+		(known values: "JANZ-ICAN3 ICANOS 1.xx", "JANZ-ICAN3 CAL/CANopen 1.xx")
+
+
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
--- linux-3.19.6/drivers/mfd/janz-cmodio.c	2015-04-29 10:30:26.000000000 +0200
+++ linux-3.19.me/drivers/mfd/janz-cmodio.c	2015-05-04 17:34:00.044635384 +0200
@@ -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
--- linux-3.19.6/drivers/net/can/janz-ican3.c	2015-04-29 10:30:26.000000000 +0200
+++ linux-3.19.me/drivers/net/can/janz-ican3.c	2015-05-04 17:34:00.044635384 +0200
@@ -40,6 +40,7 @@
 #define MSYNC_PEER		0x00		/* ICAN only */
 #define MSYNC_LOCL		0x01		/* host only */
 #define TARGET_RUNNING		0x02
+#define FIRMWARE_STAMP		0x60		/* big endian firmware stamp */
 
 #define MSYNC_RB0		0x01
 #define MSYNC_RB1		0x02
@@ -83,6 +84,7 @@
 #define MSG_COFFREQ		0x42
 #define MSG_CONREQ		0x43
 #define MSG_CCONFREQ		0x47
+#define MSG_LMTS		0xb4
 
 /*
  * Janz ICAN3 CAN Inquiry Message Types
@@ -165,6 +167,12 @@
 /* SJA1000 Clock Input */
 #define ICAN3_CAN_CLOCK		8000000
 
+/* Janz ICAN3 firmware types */
+enum ican3_fwtype {
+	ICAN3_FWTYPE_ICANOS,
+	ICAN3_FWTYPE_CAL_CANOPEN
+};
+
 /* Driver Name */
 #define DRV_NAME "janz-ican3"
 
@@ -215,6 +223,10 @@
 	struct completion buserror_comp;
 	struct can_berr_counter bec;
 
+	/* firmware type */
+	enum ican3_fwtype fwtype;
+	char fwinfo[32];
+
 	/* old and new style host interface */
 	unsigned int iftype;
 
@@ -750,13 +762,61 @@
  */
 static int ican3_set_bus_state(struct ican3_dev *mod, bool on)
 {
+	struct can_bittiming *bt = &mod->can.bittiming;
 	struct ican3_msg msg;
+	u8 btr0, btr1;
+	int res;
 
-	memset(&msg, 0, sizeof(msg));
-	msg.spec = on ? MSG_CONREQ : MSG_COFFREQ;
-	msg.len = cpu_to_le16(0);
+	/* This algorithm 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                                */
+	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;
 
-	return ican3_send_msg(mod, &msg);
+	if (mod->fwtype == ICAN3_FWTYPE_ICANOS) {
+		if (on) {
+			/* set bittiming */
+			memset(&msg, 0, sizeof(msg));
+			msg.spec = MSG_CBTRREQ;
+			msg.len = cpu_to_le16(4);
+			msg.data[0] = 0x00;
+			msg.data[1] = 0x00;
+			msg.data[2] = btr0;
+			msg.data[3] = btr1;
+
+			res = ican3_send_msg(mod, &msg);
+			if (res)
+				return res;
+		}
+
+		/* can-on/off request */
+		memset(&msg, 0, sizeof(msg));
+		msg.spec = on ? MSG_CONREQ : MSG_COFFREQ;
+		msg.len = cpu_to_le16(0);
+
+		return ican3_send_msg(mod, &msg);
+
+	} else if (mod->fwtype == ICAN3_FWTYPE_CAL_CANOPEN) {
+		memset(&msg, 0, sizeof(msg));
+		msg.spec = MSG_LMTS;
+		if (on) {
+			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 +1461,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 +1486,17 @@
 		return ret;
 	}
 
+	/* detect firmware */
+	memcpy_fromio(mod->fwinfo, mod->dpm + FIRMWARE_STAMP, sizeof(mod->fwinfo) - 1);
+	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 = ICAN3_FWTYPE_CAL_CANOPEN;
+	else
+		mod->fwtype = ICAN3_FWTYPE_ICANOS;
+
 	/* re-enable interrupts so we can send messages */
 	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
 
@@ -1614,36 +1685,6 @@
 	.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;
-
-	memset(&msg, 0, sizeof(msg));
-	msg.spec = MSG_CBTRREQ;
-	msg.len = cpu_to_le16(4);
-	msg.data[0] = 0x00;
-	msg.data[1] = 0x00;
-	msg.data[2] = btr0;
-	msg.data[3] = btr1;
-
-	return ican3_send_msg(mod, &msg);
-}
-
 static int ican3_set_mode(struct net_device *ndev, enum can_mode mode)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
@@ -1731,11 +1772,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,
 };
 
@@ -1795,7 +1847,6 @@
 
 	mod->can.clock.freq = ICAN3_CAN_CLOCK;
 	mod->can.bittiming_const = &ican3_bittiming_const;
-	mod->can.do_set_bittiming = ican3_set_bittiming;
 	mod->can.do_set_mode = ican3_set_mode;
 	mod->can.do_get_berr_counter = ican3_get_berr_counter;
 	mod->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES
@@ -1867,7 +1918,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:

Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Marc Kleine-Budde]

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

On 05/05/2015 08:08 PM, Andreas Gröger wrote:
> 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 <andreas24groeger@gmail.com>

Applied to can-next. I've split the patch, first the existing sysfs
entries are documented, the second one updates the driver.

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