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

["Ira W. Snyder" <ira.snyder@gmail.com>]

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