Message-ID: <4BA39C05.8050800@grandegger.com>
Date:	Fri, 19 Mar 2010 16:45:09 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	"Ira W. Snyder" <iws@...o.caltech.edu>
CC:	linux-kernel@...r.kernel.org, socketcan-core@...ts.berlios.de,
	netdev@...r.kernel.org, sameo@...ux.intel.com
Subject: Re: [PATCH 2/3] can: add support for Janz VMOD-ICAN3 Intelligent
 CAN	module

Ira W. Snyder wrote:
> On Fri, Mar 19, 2010 at 10:01:14AM +0100, Wolfgang Grandegger wrote:
>> Hi Ira,
>>
>> we already discussed this patch on the SocketCAN mailing list and there
>> are just a few minor issues and the request to add support for the new
>> "berr-reporting" option, if feasible. See:
>>
>>   commit 52c793f24054f5dc30d228e37e0e19cc8313f086
>>   Author: Wolfgang Grandegger <wg@...ndegger.com>
>>   Date:   Mon Feb 22 22:21:17 2010 +0000
>>
>>     can: netlink support for bus-error reporting and counters
>>     
>>     This patch makes the bus-error reporting configurable and allows to
>>     retrieve the CAN TX and RX bus error counters via netlink interface.
>>     I have added support for the SJA1000. The TX and RX bus error counters
>>     are also copied to the data fields 6..7 of error messages when state
>>     changes are reported.
>>
>> Should not be a big deal.
>>
> 
> I think this patch came along since my last post of the driver. I must
> have missed it. I'll try and add support.

No problem, it's really new. Just just need to enable BEI depending on
CAN_CTRLMODE_BERR_REPORTING.

>> More inline...
>>
>> Ira W. Snyder wrote:
>>> The Janz VMOD-ICAN3 is a MODULbus daughterboard which fits onto any
>>> MODULbus carrier board. It is an intelligent CAN controller with a
>>> microcontroller and associated firmware.
>>>
>>> Signed-off-by: Ira W. Snyder <iws@...o.caltech.edu>
>>> Cc: socketcan-core@...ts.berlios.de
>>> Cc: netdev@...r.kernel.org
>>> ---
>>>  drivers/net/can/Kconfig      |   10 +
>>>  drivers/net/can/Makefile     |    1 +
>>>  drivers/net/can/janz-ican3.c | 1659 ++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 1670 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/net/can/janz-ican3.c
>>>
>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>>> index 05b7517..2c5227c 100644
>>> --- a/drivers/net/can/Kconfig
>>> +++ b/drivers/net/can/Kconfig
>>> @@ -63,6 +63,16 @@ config CAN_BFIN
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called bfin_can.
>>>  
>>> +config CAN_JANZ_ICAN3
>>> +	tristate "Janz VMOD-ICAN3 Intelligent CAN controller"
>>> +	depends on CAN_DEV && MFD_JANZ_CMODIO
>>> +	---help---
>>> +	  Driver for Janz VMOD-ICAN3 Intelligent CAN controller module, which
>>> +	  connects to a MODULbus carrier board.
>>> +
>>> +	  This driver can also be built as a module. If so, the module will be
>>> +	  called janz-ican3.ko.
>>> +
>>>  source "drivers/net/can/mscan/Kconfig"
>>>  
>>>  source "drivers/net/can/sja1000/Kconfig"
>>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
>>> index 7a702f2..9047cd0 100644
>>> --- a/drivers/net/can/Makefile
>>> +++ b/drivers/net/can/Makefile
>>> @@ -15,5 +15,6 @@ obj-$(CONFIG_CAN_AT91)		+= at91_can.o
>>>  obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
>>>  obj-$(CONFIG_CAN_MCP251X)	+= mcp251x.o
>>>  obj-$(CONFIG_CAN_BFIN)		+= bfin_can.o
>>> +obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
>>>  
>>>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
>>> new file mode 100644
>>> index 0000000..94d4995
>>> --- /dev/null
>>> +++ b/drivers/net/can/janz-ican3.c
>> [snip]
>>  +struct ican3_dev {
>>> +
>>> +	/* must be the first member */
>>> +	struct can_priv can;
>>> +
>>> +	/* CAN network device */
>>> +	struct net_device *ndev;
>>> +	struct napi_struct napi;
>>> +
>>> +	/* Device for printing */
>>> +	struct device *dev;
>>> +
>>> +	/* module number */
>>> +	unsigned int num;
>>> +
>>> +	/* base address of registers and IRQ */
>>> +	struct janz_cmodio_onboard_regs __iomem *ctrl;
>>> +	struct ican3_dpm_control *dpmctrl;
>>> +	void __iomem *dpm;
>>> +	int irq;
>>> +
>>> +	/* old and new style host interface */
>>> +	unsigned int iftype;
>>> +	spinlock_t lock;
>> Please describe what the lock is used for.
>>
>>> +	/* new host interface */
>>> +	unsigned int rx_int;
>>> +	unsigned int rx_num;
>>> +	unsigned int tx_num;
>>> +
>>> +	/* fast host interface */
>>> +	unsigned int fastrx_start;
>>> +	unsigned int fastrx_int;
>>> +	unsigned int fastrx_num;
>>> +	unsigned int fasttx_start;
>>> +	unsigned int fasttx_num;
>>> +
>>> +	/* first free DPM page */
>>> +	unsigned int free_page;
>>> +};
>> [snip]
>>> +static void ican3_to_can_frame(struct ican3_dev *mod,
>>> +			       struct ican3_fast_desc *desc,
>>> +			       struct can_frame *cf)
>>> +{
>>> +	if ((desc->command & ICAN3_CAN_TYPE_MASK) == ICAN3_CAN_TYPE_SFF) {
>>> +		dev_dbg(mod->dev, "%s: old frame format\n", __func__);
>> This prints a debug message for every message which is not really
>> useful for the user. Please remove.
>>
>>> +		if (desc->data[1] & ICAN3_SFF_RTR)
>>> +			cf->can_id |= CAN_RTR_FLAG;
>>> +
>>> +		cf->can_id |= desc->data[0] << 3;
>>> +		cf->can_id |= (desc->data[1] & 0xe0) >> 5;
>>> +		cf->can_dlc = desc->data[1] & ICAN3_CAN_DLC_MASK;
>>> +		memcpy(cf->data, &desc->data[2], sizeof(cf->data));
>>> +	} else {
>>> +		dev_dbg(mod->dev, "%s: new frame format\n", __func__);
>> Ditto.
>>
>>> +		cf->can_dlc = desc->data[0] & ICAN3_CAN_DLC_MASK;
>>> +		if (desc->data[0] & ICAN3_EFF_RTR)
>>> +			cf->can_id |= CAN_RTR_FLAG;
>>> +
>>> +		if (desc->data[0] & ICAN3_EFF) {
>>> +			cf->can_id |= CAN_EFF_FLAG;
>>> +			cf->can_id |= desc->data[2] << 21; /* 28-21 */
>>> +			cf->can_id |= desc->data[3] << 13; /* 20-13 */
>>> +			cf->can_id |= desc->data[4] << 5;  /* 12-5  */
>>> +			cf->can_id |= (desc->data[5] & 0xf8) >> 3;
>>> +		} else {
>>> +			cf->can_id |= desc->data[2] << 3;  /* 10-3  */
>>> +			cf->can_id |= desc->data[3] >> 5;  /* 2-0   */
>>> +		}
>>> +
>>> +		memcpy(cf->data, &desc->data[6], sizeof(cf->data));
>>> +	}
>>> +}
>>> +
>>> +static void can_frame_to_ican3(struct ican3_dev *mod,
>>> +			       struct can_frame *cf,
>>> +			       struct ican3_fast_desc *desc)
>>> +{
>>> +	/* clear out any stale data in the descriptor */
>>> +	memset(desc->data, 0, sizeof(desc->data));
>>> +
>>> +	/* we always use the extended format, with the ECHO flag set */
>>> +	desc->command = ICAN3_CAN_TYPE_EFF;
>>> +	desc->data[0] |= cf->can_dlc;
>>> +	desc->data[1] |= ICAN3_ECHO;
>>> +
>>> +	if (cf->can_id & CAN_RTR_FLAG)
>>> +		desc->data[0] |= ICAN3_EFF_RTR;
>>> +
>>> +	/* pack the id into the correct places */
>>> +	if (cf->can_id & CAN_EFF_FLAG) {
>>> +		dev_dbg(mod->dev, "%s: extended frame\n", __func__);
>> Ditto.
>>
>>> +		desc->data[0] |= ICAN3_EFF;
>>> +		desc->data[2] = (cf->can_id & 0x1fe00000) >> 21; /* 28-21 */
>>> +		desc->data[3] = (cf->can_id & 0x001fe000) >> 13; /* 20-13 */
>>> +		desc->data[4] = (cf->can_id & 0x00001fe0) >> 5;  /* 12-5  */
>>> +		desc->data[5] = (cf->can_id & 0x0000001f) << 3;  /* 4-0   */
>>> +	} else {
>>> +		dev_dbg(mod->dev, "%s: standard frame\n", __func__);
>> Ditto.
>>
>>> +		desc->data[2] = (cf->can_id & 0x7F8) >> 3; /* bits 10-3 */
>>> +		desc->data[3] = (cf->can_id & 0x007) << 5; /* bits 2-0  */
>>> +	}
>>> +
>>> +	/* copy the data bits into the descriptor */
>>> +	memcpy(&desc->data[6], cf->data, sizeof(cf->data));
>>> +}
>> [snip]
>>> +/*
>>> + * Handle CAN Event Indication Messages from the firmware
>>> + *
>>> + * The ICAN3 firmware provides the values of some SJA1000 registers when it
>>> + * generates this message. The code below is largely copied from the
>>> + * drivers/net/can/sja1000/sja1000.c file, and adapted as necessary
>>> + */
>>> +static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>>> +{
>>> +	struct net_device *dev = mod->ndev;
>>> +	struct net_device_stats *stats = &dev->stats;
>>> +	enum can_state state = mod->can.state;
>>> +	struct can_frame *cf;
>>> +	struct sk_buff *skb;
>>> +	u8 status, isrc;
>>> +
>>> +	/* we can only handle the SJA1000 part */
>>> +	if (msg->data[1] != CEVTIND_CHIP_SJA1000) {
>>> +		dev_err(mod->dev, "unable to handle errors on non-SJA1000\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	/* check the message length for sanity */
>>> +	if (msg->len < 6) {
>>> +		dev_err(mod->dev, "error message too short\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	skb = alloc_can_err_skb(dev, &cf);
>>> +	if (skb == NULL)
>>> +		return -ENOMEM;
>>> +
>>> +	isrc = msg->data[0];
>>> +	status = msg->data[3];
>>> +
>>> +	/* data overrun interrupt */
>>> +	if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
>> Here and for the other errors below a dev_dbg() would be useful. Please
>> check sja1000.c.
>>
>>> +		cf->can_id |= CAN_ERR_CRTL;
>>> +		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>> +		stats->rx_over_errors++;
>>> +		stats->rx_errors++;
>>> +		dev_info(mod->dev, "%s: overflow frame generated\n", __func__);
>> s/dev_info/dev_dbg/ ?
>>
> 
> Whoops, a leftover from debugging. Will change to dev_dbg().
> 
>>> +	}
>>> +
>>> +	/* error warning interrupt */
>>> +	if (isrc == CEVTIND_EI) {
>>> +		if (status & SR_BS) {
>>> +			state = CAN_STATE_BUS_OFF;
>>> +			cf->can_id |= CAN_ERR_BUSOFF;
>>> +			can_bus_off(dev);
>>> +		} else if (status & SR_ES) {
>>> +			state = CAN_STATE_ERROR_WARNING;
>>> +		} else {
>>> +			state = CAN_STATE_ERROR_ACTIVE;
>>> +		}
>>> +	}
>>> +
>>> +	/* bus error interrupt */
>>> +	if (isrc == CEVTIND_BEI) {
>>> +		u8 ecc = msg->data[2];
>> Add an empty line, please.
>>
>>> +		mod->can.can_stats.bus_error++;
>>> +		stats->rx_errors++;
>>> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +
>>> +		switch (ecc & ECC_MASK) {
>>> +		case ECC_BIT:
>>> +			cf->data[2] |= CAN_ERR_PROT_BIT;
>>> +			break;
>>> +		case ECC_FORM:
>>> +			cf->data[2] |= CAN_ERR_PROT_FORM;
>>> +			break;
>>> +		case ECC_STUFF:
>>> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> +			break;
>>> +		default:
>>> +			cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>>> +			cf->data[3] = ecc & ECC_SEG;
>>> +			break;
>>> +		}
>>> +
>>> +		if ((ecc & ECC_DIR) == 0)
>>> +			cf->data[2] |= CAN_ERR_PROT_TX;
>>> +	}
>>> +
>>> +	if (state != mod->can.state && (state == CAN_STATE_ERROR_WARNING ||
>>> +					state == CAN_STATE_ERROR_PASSIVE)) {
>>> +		u8 rxerr = msg->data[4];
>>> +		u8 txerr = msg->data[5];
>> Ditto.
>>
>>> +		cf->can_id |= CAN_ERR_CRTL;
>>> +		if (state == CAN_STATE_ERROR_WARNING) {
>>> +			mod->can.can_stats.error_warning++;
>>> +			cf->data[1] = (txerr > rxerr) ?
>>> +				CAN_ERR_CRTL_TX_WARNING :
>>> +				CAN_ERR_CRTL_RX_WARNING;
>>> +		} else {
>>> +			mod->can.can_stats.error_passive++;
>>> +			cf->data[1] = (txerr > rxerr) ?
>>> +				CAN_ERR_CRTL_TX_PASSIVE :
>>> +				CAN_ERR_CRTL_RX_PASSIVE;
>>> +		}
>>> +	}
>>> +
>>> +	mod->can.state = state;
>>> +	stats->rx_errors++;
>>> +	stats->rx_bytes += cf->can_dlc;
>>> +	netif_rx(skb);
>>> +	return 0;
>>> +}
>> [snip]
>>> +static irqreturn_t ican3_irq(int irq, void *dev_id)
>>> +{
>>> +	struct ican3_dev *mod = dev_id;
>>> +	u8 stat;
>>> +
>>> +	/*
>>> +	 * The interrupt status register on this device reports interrupts
>>> +	 * as zeroes instead of using ones like most other devices
>>> +	 */
>>> +	stat = ioread8(&mod->ctrl->int_disable) & (1 << mod->num);
>>> +	if (stat == (1 << mod->num))
>>> +		return IRQ_NONE;
>>> +
>>> +	dev_dbg(mod->dev, "IRQ: module %d\n", mod->num);
>> Please remove this dev_dbg() as well.
>>
>> [snip]
>>> +/*
>>> + * Startup an ICAN module, bringing it into fast mode
>>> + */
>>> +static int __devinit ican3_startup_module(struct ican3_dev *mod)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = ican3_reset_module(mod);
>>> +	if (ret) {
>>> +		dev_err(mod->dev, "unable to reset module\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* re-enable interrupts so we can send messages */
>>> +	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
>>> +
>>> +	ret = ican3_msg_connect(mod);
>>> +	if (ret) {
>>> +		dev_err(mod->dev, "unable to connect to module\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ican3_init_new_host_interface(mod);
>>> +	ret = ican3_msg_newhostif(mod);
>>> +	if (ret) {
>>> +		dev_err(mod->dev, "unable to switch to new-style interface\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = ican3_set_termination(mod, true);
>>> +	if (ret) {
>>> +		dev_err(mod->dev, "unable to enable termination\n");
>>> +		return ret;
>>> +	}
>>
>> Could you please allow the user to disable termination, e.g. via module parameter
>> "bus_termination=0" in case he uses a cable with built-in termination.
>>
> 
> What would you think about a sysfs node instead, so it could be changed
> at runtime, on a per-daughtercard basis? Do you think enabling bus
> termination is a good default? IMO, it is a pretty safe default for most
> users.

I have no problem with the default. There should just be a way to
disable termination somehow. A dev_info(dev, "CAN bus termination
enabled") would furthermore make clear, that it's active. Setting
termination via SysFS is the better solution, of course, as it's per
device. It might even be useful to handle this in future in a common way
via CAN_CTRLMODE_BUS_TERMINATION.

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists