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 <[email protected]> > 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. > 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 <[email protected]> > > Cc: [email protected] > > Cc: [email protected] > > --- > > 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. > [snip] > > +static int __devinit ican3_probe(struct platform_device *pdev) > > +{ > > + struct janz_platform_data *pdata; > > + struct net_device *ndev; > > + struct ican3_dev *mod; > > + struct resource *res; > > + struct device *dev; > > + int ret; > > + > > + pdata = pdev->dev.platform_data; > > + if (!pdata) > > + return -ENXIO; > > + > > + dev_dbg(&pdev->dev, "probe: module number %d\n", pdata->modno); > > + > > + /* save the struct device for printing */ > > + dev = &pdev->dev; > > + > > + /* allocate the CAN device and private data */ > > + ndev = alloc_candev(sizeof(*mod), 0); > > + if (!ndev) { > > + dev_err(dev, "unable to allocate CANdev\n"); > > + ret = -ENOMEM; > > + goto out_return; > > + } > > + > > + platform_set_drvdata(pdev, ndev); > > + mod = netdev_priv(ndev); > > + mod->ndev = ndev; > > + mod->dev = &pdev->dev; > > + mod->num = pdata->modno; > > + netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS); > > + spin_lock_init(&mod->lock); > > + > > + /* the first unallocated page in the DPM is 9 */ > > + mod->free_page = DPM_FREE_START; > > + > > + ndev->netdev_ops = &ican3_netdev_ops; > > + ndev->flags |= IFF_ECHO; > > + SET_NETDEV_DEV(ndev, &pdev->dev); > > + > > + mod->can.clock.freq = 8000000; > > Please use a constant here. > [snip] > > Please fix and resubmit with my: > > "Acked-by: Wolfgang Grandegger <[email protected]>" > > for the SocketCAN part. > Thanks for the review! Ira _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
