Hi Wolfgang, Thanks for your inputs. Please see my comments below:
> > Signed-off-by: Bhupesh Sharma <[email protected]> > > Please truncate to the usual < 80 lines for readability. I am myself not a big supporter of < 80 lines being mandatory, especially if it is in a place like this. Please see Linus' comments on the same here: http://www.linux-archive.org/device-mapper-development/295901-drop-80-character-limit-checkpatch-pl.html > > > > Index: bosch_ccan.c > > =================================================================== > > --- bosch_ccan.c (revision 0) > > +++ bosch_ccan.c (revision 0) > > @@ -0,0 +1,858 @@ > > +/* > > + * drivers/net/can/bosch_ccan.c > > + * > > + * CAN bus driver for Bosch CCAN controller > > + * > > + * Copyright (C) 2010 ST Microelectronics > > + * Bhupesh Sharma <[email protected]> > > + * > > + * Borrowed heavily from the CCAN driver originally written by: > > + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix > <[email protected]> > > + * - Simon Kallweit, intefo AG <[email protected]> > > + * which can be viewed here: > > + * http://svn.berlios.de/svnroot/repos/socketcan/trunk/kernel/2.6/ > > + * drivers/net/can/old/ccan/ccan.c > > + * > > + * Bosch CCAN controller is compliant to CAN protocol version 2.0 > part A and B. > > + * Bosch CCAN user manual can be obtained from: > > + * http://www.semiconductors.bosch.de/pdf/Users_Manual_C_CAN.pdf > > + * > > + * TODO: > > + * - Add support for IF2 in Basic mode. > > + * > > + * This file is licensed under the terms of the GNU General Public > > + * License version 2. This program is licensed "as is" without any > > + * warranty of any kind, whether express or implied. > > + */ > > See may comments on patch 1/4. Ack to adding 'Original Copyright' message in V2 and removing the 'drivers/net/can/..' representation. However, I have seen URL links in a number of kernel drivers at this place (for e.g., see 'ti_hecc.c' CAN driver). > > +#include <linux/kernel.h> > > +#include <linux/version.h> > > +#include <linux/module.h> > > +#include <linux/interrupt.h> > > +#include <linux/delay.h> > > +#include <linux/netdevice.h> > > +#include <linux/if_arp.h> > > +#include <linux/if_ether.h> > > +#include <linux/list.h> > > +#include <linux/delay.h> > > +#include <linux/workqueue.h> > > +#include <linux/io.h> > > + > > +#include <socketcan/can.h> > > +#include <socketcan/can/dev.h> > > +#include <socketcan/can/error.h> > > + > > +#include "bosch_ccan.h" > > + > > +#define DRV_NAME "bosch_ccan" > > + > > +static struct can_bittiming_const bosch_ccan_bittiming_const = { > > + .name = DRV_NAME, > > + .tseg1_min = 2, /* Time segment 1 = prop_seg + > phase_seg1 */ > > + .tseg1_max = 16, > > + .tseg2_min = 1, /* Time segment 2 = phase_seg2 */ > > + .tseg2_max = 8, > > + .sjw_max = 4, > > + .brp_min = 1, > > + .brp_max = 1024, /* 6-bit BRP field + 4-bit BRPE > field*/ > > + .brp_inc = 1, > > +}; > > + > > +static u32 bosch_ccan_read_reg32(struct net_device *dev, enum > ccan_regs reg) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + u32 val = priv->read_reg(priv, reg); > > + val |= ((u32) priv->read_reg(priv, reg + 2)) << 16; > > + > > + return val; > > +} > > + > > +static void bosch_ccan_write_reg32(struct net_device *dev, enum > ccan_regs reg, > > + u32 val) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + priv->write_reg(priv, reg, val & 0xffff); > > + priv->write_reg(priv, reg + 2, val >> 16); > > +} > > + > > +static inline void bosch_ccan_object_get(struct net_device *dev, > > + int iface, int objno, int > mask) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + priv->write_reg(priv, CAN_IF_COMM(iface), mask); > > + priv->write_reg(priv, CAN_IF_COMR(iface), objno + 1); > > + while (priv->read_reg(priv, CAN_IF_COMR(iface)) & > IF_COMR_BUSY) > > + dev_info(dev->dev.parent, "busy in object get\n"); > > Hm, please use a proper timeout handling with a defined delay. Ack. Suggested by Mark as well. Now adding 'wait_for_completion_timout' here. > > +} > > + > > +static inline void bosch_ccan_object_put(struct net_device *dev, > > + int iface, int objno, int > mask) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + priv->write_reg(priv, CAN_IF_COMM(iface), IF_COMM_WR | mask); > > + priv->write_reg(priv, CAN_IF_COMR(iface), objno + 1); > > + while (priv->read_reg(priv, CAN_IF_COMR(iface)) & > IF_COMR_BUSY) > > + dev_info(dev->dev.parent, "busy in object put\n"); > > ditto Ditto > > +} > > + > > +int bosch_ccan_write_object(struct net_device *dev, > > + int iface, struct can_frame *frame, int > objno) > > +{ > > + unsigned int val; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + if (frame->can_id & CAN_EFF_FLAG) > > + val = IF_ARB_MSGXTD | (frame->can_id & CAN_EFF_MASK); > > + else > > + val = ((frame->can_id & CAN_SFF_MASK) << 18); > > + > > + if (!(frame->can_id & CAN_RTR_FLAG)) > > + val |= IF_ARB_TRANSMIT; > > + > > + val |= IF_ARB_MSGVAL; > > + bosch_ccan_write_reg32(dev, CAN_IF_ARB(iface), val); > > + > > + memcpy(&val, &frame->data[0], 4); > > + bosch_ccan_write_reg32(dev, CAN_IF_DATAA(iface), val); > > + memcpy(&val, &frame->data[4], 4); > > + bosch_ccan_write_reg32(dev, CAN_IF_DATAB(iface), val); > > + priv->write_reg(priv, CAN_IF_MCONT(iface), > > + IF_MCONT_TXIE | IF_MCONT_TXRQST | > IF_MCONT_EOB | > > + (frame->can_dlc & 0xf)); > > This seems very inefficient. Also you only need to set can_dlc bytes. Ack. Suggested by Mark as well. Now removing 'memcpy' and will also checking that no endianess issues are caused by this. > > + bosch_ccan_object_put(dev, 0, objno, IF_COMM_ALL); > > + > > + return 0; > > +} > > + > > +int bosch_ccan_read_object(struct net_device *dev, int iface, int > objno) > > +{ > > + unsigned int val, ctrl, data; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) > > + struct net_device_stats *stats = can_get_stats(dev); > > +#else > > + struct net_device_stats *stats = &dev->stats; > > +#endif > > + struct sk_buff *skb; > > + struct can_frame *frame; > > + > > + skb = dev_alloc_skb(sizeof(struct can_frame)); > > + if (skb == NULL) { > > + dev_err(dev->dev.parent, > > + "failed to allocate skb for read > object\n"); > > + return -ENOMEM; > > + } > > + > > + skb->dev = dev; > > + bosch_ccan_object_get(dev, 0, objno, IF_COMM_ALL & > ~IF_COMM_TXRQST); > > + frame = (struct can_frame *)skb_put(skb, sizeof(struct > can_frame)); > > + > > + ctrl = priv->read_reg(priv, CAN_IF_MCONT(iface)); > > + if (ctrl & IF_MCONT_MSGLST) { > > + stats->rx_errors++; > > + dev_info(dev->dev.parent, "msg lost in buffer %d\n", > objno); > > + } > > + > > + frame->can_dlc = ctrl & 0xf; > > + val = bosch_ccan_read_reg32(dev, CAN_IF_ARB(iface)); > > + data = bosch_ccan_read_reg32(dev, CAN_IF_DATAA(iface)); > > + memcpy(&frame->data[0], &data, 4); > > + data = bosch_ccan_read_reg32(dev, CAN_IF_DATAB(iface)); > > + memcpy(&frame->data[4], &data, 4); > > Dito. Also, I smell endianess issus. Ditto > > + if (val & IF_ARB_MSGXTD) > > + frame->can_id = (val & CAN_EFF_MASK) | CAN_EFF_FLAG; > > + else > > + frame->can_id = (val >> 18) & CAN_SFF_MASK; > > + > > + if (val & IF_ARB_TRANSMIT) > > + frame->can_id |= CAN_RTR_FLAG; > > + > > + priv->write_reg(priv, CAN_IF_MCONT(iface), ctrl & > > + ~(IF_MCONT_MSGLST | IF_MCONT_INTPND | > IF_MCONT_NEWDAT)); > > + > > + bosch_ccan_object_put(dev, 0, objno, IF_COMM_CONTROL); > > + > > + skb->protocol = __constant_htons(ETH_P_CAN); > > + netif_rx(skb); > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) > > + dev->last_rx = jiffies; > > +#endif > > + > > + stats->rx_packets++; > > + stats->rx_bytes += frame->can_dlc; > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_setup_receive_object(struct net_device *dev, > int iface, > > + int objno, unsigned int mask, > > + unsigned int id, unsigned int > mcont) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + bosch_ccan_write_reg32(dev, CAN_IF_MASK(iface), mask); > > + bosch_ccan_write_reg32(dev, CAN_IF_ARB(iface), IF_ARB_MSGVAL > | id); > > + > > + priv->write_reg(priv, CAN_IF_MCONT(iface), mcont); > > + bosch_ccan_object_put(dev, 0, objno, IF_COMM_ALL & > ~IF_COMM_TXRQST); > > + > > + dev_dbg(dev->dev.parent, "obj no:%d, msgval:0x%08x\n", objno, > > + bosch_ccan_read_reg32(dev, CAN_MSGVAL)); > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_inval_object(struct net_device *dev, int > iface, int objno) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + bosch_ccan_write_reg32(dev, CAN_IF_ARB(iface), 0); > > + priv->write_reg(priv, CAN_IF_MCONT(iface), 0); > > + bosch_ccan_object_put(dev, 0, objno, IF_COMM_ARB | > IF_COMM_CONTROL); > > + > > + dev_dbg(dev->dev.parent, "obj no:%d, msgval:0x%08x\n", objno, > > + bosch_ccan_read_reg32(dev, CAN_MSGVAL)); > > + > > + return 0; > > +} > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) > > +static int bosch_ccan_start_xmit(struct sk_buff *skb, > > + struct net_device *dev) > > +#else > > +static netdev_tx_t bosch_ccan_start_xmit(struct sk_buff *skb, > > + struct net_device *dev) > > +#endif > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + struct can_frame *frame = (struct can_frame *)skb->data; > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) > > + struct net_device_stats *stats = can_get_stats(dev); > > +#else > > + struct net_device_stats *stats = &dev->stats; > > +#endif > > + > > + netif_stop_queue(dev); > > + > > + bosch_ccan_write_object(dev, 0, frame, priv->tx_object); > > + priv->tx_object++; > > Is this variable really useful? In my view it is helpful for debugging. > > + > > + stats->tx_bytes += frame->can_dlc; > > Should be set when TX is done. Ack. Will be reflected in V2. > > + dev->trans_start = jiffies; > > + can_put_echo_skb(skb, dev, 0); > > + > > + return NETDEV_TX_OK; > > +} > > + > > +static int bosch_ccan_set_bittiming(struct net_device *dev) > > +{ > > + unsigned int reg_btr, reg_brpe, ctrl_save; > > + u8 brp, brpe, sjw, tseg1, tseg2; > > + u32 ten_bit_brp; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + const struct can_bittiming *bt = &priv->can.bittiming; > > + > > + /* ccan provides a 6-bit brp and 4-bit brpe fields */ > > + ten_bit_brp = bt->brp - 1; > > + brp = ten_bit_brp & BTR_BRP_MASK; > > + brpe = ten_bit_brp >> 6; > > + > > + sjw = bt->sjw - 1; > > + tseg1 = bt->prop_seg + bt->phase_seg1 - 1; > > + tseg2 = bt->phase_seg2 - 1; > > + > > + reg_btr = (brp) | > > + ((sjw << BTR_SJW_SHIFT) & BTR_SJW_MASK) | > > + ((tseg1 << BTR_TSEG1_SHIFT) & BTR_TSEG1_MASK) | > > + ((tseg2 << BTR_TSEG2_SHIFT) & BTR_TSEG2_MASK); > > + > > + reg_brpe = brpe & BRP_EXT_BRPE_MASK; > > + > > + dev_dbg(dev->dev.parent, > > + "brp = %d, brpe = %d, sjw = %d, seg1 = %d, > seg2 = %d\n", > > + brp, brpe, sjw, tseg1, tseg2); > > + dev_dbg(dev->dev.parent, "setting BTR to %04x\n", reg_btr); > > + dev_dbg(dev->dev.parent, "setting BRPE to %04x\n", reg_brpe); > > + > > + ctrl_save = priv->read_reg(priv, CAN_CONTROL); > > + priv->write_reg(priv, CAN_CONTROL, > > + ctrl_save | CONTROL_CCE | CONTROL_INIT); > > + priv->write_reg(priv, CAN_BTR, reg_btr); > > + priv->write_reg(priv, CAN_BRP_EXT, reg_brpe); > > + priv->write_reg(priv, CAN_CONTROL, ctrl_save); > > + > > + return 0; > > +} > > + > > +/* > > + * Configure CCAN auto-retransmission: > > + * CCAN provides means to disable automatic retransmission of > > + * frames to allow CCAN to work within a TTCAN environment. > > + * One must be careful about the different bevaior of TxRqst and > > + * NewDat in the case automatic retransmssion is disabled. > > + * See user guide document for details. > > + */ > > +static int bosch_ccan_auto_retransmission_config(struct net_device > *dev, > > + enum > bosch_ccan_auto_tx_config mode) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + switch (mode) { > > + case CCAN_ENABLE_AUTO_RE_TRANSMIT: > > + priv->write_reg(priv, CAN_CONTROL, > CONTROL_ENABLE_AR); > > + break; > > + case CCAN_DISABLE_AUTO_RE_TRANSMIT: > > + priv->write_reg(priv, CAN_CONTROL, > CONTROL_DISABLE_AR); > > + break; > > + default: > > + return -EOPNOTSUPP; > > + } > > + return 0; > > +} > > + > > +/* > > + * Configure CCAN operating mode > > + */ > > +static int bosch_ccan_set_operating_mode(struct net_device *dev, > > + enum bosch_ccan_operating_mode mode) > > +{ > > + unsigned int cntrl_reg = 0; > > + unsigned int test_reg = 0; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + switch (mode) { > > + case CCAN_NORMAL_MODE: > > + cntrl_reg = (CONTROL_EIE | CONTROL_SIE | CONTROL_IE); > > + break; > > + case CCAN_BASIC_MODE: > > + /* basic mode : CCAN runs without the message RAM */ > > + cntrl_reg = (CONTROL_EIE | CONTROL_SIE | > > + CONTROL_IE | CONTROL_TEST); > > + test_reg = TEST_BASIC; > > + break; > > + case CCAN_LOOPBACK_MODE: > > + /* loopback mode : useful for self-test function */ > > + cntrl_reg = (CONTROL_EIE | CONTROL_SIE | > > + CONTROL_IE | CONTROL_TEST); > > + test_reg = TEST_LBACK; > > + break; > > + case CCAN_LOOPBACK_WITH_SILENT_MODE: > > + /* loopback + silent mode : useful for hot self-test > */ > > + cntrl_reg = (CONTROL_EIE | CONTROL_SIE | > > + CONTROL_IE | CONTROL_TEST); > > + test_reg = (TEST_LBACK | TEST_SILENT); > > + break; > > + case CCAN_SILENT_MODE: > > + /* silent mode : bus-monitoring mode */ > > + cntrl_reg = (CONTROL_EIE | CONTROL_SIE | > > + CONTROL_IE | CONTROL_TEST); > > + test_reg = TEST_SILENT; > > + break; > > Of these only CCAN_NORMAL_MODE is used. Please use priv->crtlmode to > handle more of them and remove the unused cases. Please elaborate this. I have use CCAN_LOOPBACK_MODE as well during debugging the driver. These modes are especially successful in debugging or testing the PCB's for non-functioning CAN interfaces. > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + priv->write_reg(priv, CAN_CONTROL, cntrl_reg); > > + > > + /* set test mode only when we do not want NORMAL mode */ > > + if (test_reg) > > + priv->write_reg(priv, CAN_TEST, test_reg); > > + > > + return 0; > > +} > > + > > +/* > > + * Configure CCAN message objects for Tx and Rx purposes: > > + * CCAN provides a total of 32 message objects that can be > configured > > + * either for Tx or Rx purposes. This configuration may vary as per > the > > + * system design. Here by default 16 message objects are kept aside > for > > + * Tx purposes and 16 for Rx purposes. See user guide document for > details. > > + */ > > +static int bosch_ccan_configure_msg_objects(struct net_device *dev) > > +{ > > + int i; > > + > > + /* setup message objects */ > > + for (i = 0; i <= MAX_OBJECT; i++) > > + bosch_ccan_inval_object(dev, 0, i); > > + > > + for (i = MAX_TRANSMIT_OBJECT + 1; i < MAX_OBJECT; i++) > > + bosch_ccan_setup_receive_object(dev, 0, i, 0, 0, > > + IF_MCONT_RXIE | > IF_MCONT_UMASK); > > + > > + bosch_ccan_setup_receive_object(dev, 0, MAX_OBJECT, 0, 0, > IF_MCONT_EOB | > > + IF_MCONT_RXIE | > IF_MCONT_UMASK); > > + > > + return 0; > > +} > > + > > +/* > > + * Configure CCAN chip: > > + * - enable/disable auto-retransmission > > + * - set operating mode > > + * - configure message objects > > + */ > > +static int bosch_ccan_chip_config(struct net_device *dev) > > +{ > > + int err; > > + > > + /* enable automatic retransmission */ > > + err = bosch_ccan_auto_retransmission_config(dev, > > + > CCAN_ENABLE_AUTO_RE_TRANSMIT); > > + if (err) > > + return err; > > + > > + /* enable normal operating mode */ > > + err = bosch_ccan_set_operating_mode(dev, CCAN_NORMAL_MODE); > > + if (err) > > + return err; > > + > > + /* configure message objects */ > > + bosch_ccan_configure_msg_objects(dev); > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_start(struct net_device *dev) > > +{ > > + int err; > > + unsigned int cntrl_save; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + /* enable status change, error and module interrupts */ > > + cntrl_save = priv->read_reg(priv, CAN_CONTROL); > > + cntrl_save = (cntrl_save | (CONTROL_SIE | CONTROL_EIE | > CONTROL_IE)); > > "|=" ? Ack. Will be reflected in V2. > > + priv->write_reg(priv, CAN_CONTROL, cntrl_save); > > + > > + /* basic ccan configuration */ > > + err = bosch_ccan_chip_config(dev); > > + if (err) > > + return err; > > + > > + priv->tx_object = 0; > > + priv->can.state = CAN_STATE_ERROR_ACTIVE; > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_stop(struct net_device *dev) > > +{ > > + unsigned int cntrl_save; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + /* disable all interrupts */ > > + cntrl_save = priv->read_reg(priv, CAN_CONTROL); > > + cntrl_save = ((((cntrl_save & ~CONTROL_EIE) & ~CONTROL_IE) & > > + ~CONTROL_SIE)); > > "&=" ? Ack. Will be reflected in V2. > > + priv->write_reg(priv, CAN_CONTROL, cntrl_save); > > + > > + priv->can.state = CAN_STATE_STOPPED; > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_set_mode(struct net_device *dev, enum can_mode > mode) > > +{ > > + switch (mode) { > > + case CAN_MODE_START: > > + bosch_ccan_start(dev); > > + if (netif_queue_stopped(dev)) > > + netif_wake_queue(dev); > > + dev_info(dev->dev.parent, > > + "bosch ccan CAN_MODE_START > requested\n"); > > + break; > > How is bus-off recovery supposed to work? Bus-Off recovery works by providing the command: # ip link set canX type can restart This causes 'bosch_ccan_start' to be called which enables the interrupts and allows a change of Bus Off bit in Status Register which is properly handled by ISR. Already checked the bus-off recovery on SPEAr320 platform. > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_get_state(const struct net_device *dev, > > + enum can_state *state) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + *state = priv->can.state; > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_err(struct net_device *dev, > > + enum bosch_ccan_bus_error_types > error_type, > > + int lec_type) > > +{ > > + unsigned int reg_err_counter = 0; > > + unsigned int rx_err_passive = 0; > > + unsigned int rx_err_counter = 0; > > + unsigned int tx_err_counter = 0; > > I do not see a need for pre-setting the values above. Ack. V2 will implement do_get_berr_counter and use it as suggested by you and Mark. > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) > > + struct net_device_stats *stats = can_get_stats(dev); > > +#else > > + struct net_device_stats *stats = &dev->stats; > > +#endif > > + struct can_frame *cf; > > + struct sk_buff *skb; > > + > > + /* propogate the error condition to the CAN stack */ > > + skb = dev_alloc_skb(sizeof(struct can_frame)); > > + if (skb == NULL) > > + return -ENOMEM; > > + > > + skb->dev = dev; > > + skb->protocol = htons(ETH_P_CAN); > > + cf = (struct can_frame *)skb_put(skb, sizeof(struct > can_frame)); > > + memset(cf, 0, sizeof(struct can_frame)); > > + cf->can_id = CAN_ERR_FLAG; > > + cf->can_dlc = CAN_ERR_DLC; > > Please use alloc_can_skb(), Ack. V2 will reflect this. > > + > > + reg_err_counter = priv->read_reg(priv, CAN_ERROR); > > + rx_err_counter = ((reg_err_counter & ERR_COUNTER_REC_MASK) >> > > + ERR_COUNTER_REC_SHIFT); > > + tx_err_counter = (reg_err_counter & ERR_COUNTER_TEC_MASK); > > + rx_err_passive = ((reg_err_counter & ERR_COUNTER_RP_MASK) >> > > + ERR_COUNTER_RP_SHIFT); > > + > > + if (error_type & CCAN_ERROR_WARNING) { > > + /* error warning state */ > > + priv->can.can_stats.error_warning++; > > + priv->can.state = CAN_STATE_ERROR_WARNING; > > + cf->can_id |= CAN_ERR_CRTL; > > + if (rx_err_counter > 96) > > + cf->data[1] = CAN_ERR_CRTL_RX_WARNING; > > + if (tx_err_counter > 96) > > + cf->data[1] = CAN_ERR_CRTL_TX_WARNING; > > + } > > + if (error_type & CCAN_ERROR_PASSIVE) { > > + /* error passive state */ > > + priv->can.can_stats.error_passive++; > > + priv->can.state = CAN_STATE_ERROR_PASSIVE; > > + cf->can_id |= CAN_ERR_CRTL; > > + if (rx_err_passive) > > + cf->data[1] = CAN_ERR_CRTL_RX_PASSIVE; > > + if (tx_err_counter > 127) > > + cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE; > > + } > > + if (error_type & CCAN_BUS_OFF) { > > + /* bus-off state */ > > + priv->can.state = CAN_STATE_BUS_OFF; > > + cf->can_id |= CAN_ERR_BUSOFF; > > + /* disable all interrupts in bus-off mode to ensure > that > > + * the CPU is not hogged down > > + */ > > + priv->write_reg(priv, CAN_CONTROL, 0); > > + can_bus_off(dev); > > + } > > + > > + /* check for 'last error code' which tells us the > > + * type of the last error to occur on the CAN bus > > + */ > > + if (lec_type) { > > + /* common for all type of bus errors */ > > + priv->can.can_stats.bus_error++; > > + stats->rx_errors++; > > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > > + cf->data[2] |= CAN_ERR_PROT_UNSPEC; > > + > > + if (lec_type & LEC_STUFF_ERROR) { > > + dev_info(dev->dev.parent, "stuff error\n"); > > + cf->data[2] |= CAN_ERR_PROT_STUFF; > > + } > > + if (lec_type & LEC_FORM_ERROR) { > > + dev_info(dev->dev.parent, "form error\n"); > > + cf->data[2] |= CAN_ERR_PROT_FORM; > > + } > > + if (lec_type & LEC_ACK_ERROR) { > > + dev_info(dev->dev.parent, "ack error\n"); > > + cf->data[2] |= (CAN_ERR_PROT_LOC_ACK | > > + CAN_ERR_PROT_LOC_ACK_DEL); > > + } > > + if (lec_type & LEC_BIT1_ERROR) { > > + dev_info(dev->dev.parent, "bit1 error\n"); > > + cf->data[2] |= CAN_ERR_PROT_BIT1; > > + } > > + if (lec_type & LEC_BIT0_ERROR) { > > + dev_info(dev->dev.parent, "bit0 error\n"); > > + cf->data[2] |= CAN_ERR_PROT_BIT0; > > + } > > + if (lec_type & LEC_CRC_ERROR) { > > + dev_info(dev->dev.parent, "CRC error\n"); > > + cf->data[2] |= (CAN_ERR_PROT_LOC_CRC_SEQ | > > + CAN_ERR_PROT_LOC_CRC_DEL); > > + } > > + } > > + > > + netif_rx(skb); > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) > > + dev->last_rx = jiffies; > > +#endif > > + > > + stats->rx_packets++; > > + stats->rx_bytes += cf->can_dlc; > > + > > + return 0; > > +} > > + > > +static int bosch_ccan_do_status_irq(struct net_device *dev) > > +{ > > + int ret, lec_type = 0; > > + enum bosch_ccan_bus_error_types error_type = CCAN_NO_ERROR; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + priv->current_status = priv->read_reg(priv, CAN_STATUS); > > + > > + /* handle Tx/Rx events */ > > + if (priv->current_status & STATUS_TXOK) { > > + dev_dbg(dev->dev.parent, > > + "Trasmitted a msg successfully\n"); > > + priv->write_reg(priv, CAN_STATUS, > > + (priv->current_status & > ~STATUS_TXOK)); > > Please restrict dev_dbg() to the one useful for the real user. Printing > a message for every message sent is not really usefull. Ack. V2 will reflect this. > > + } > > + if (priv->current_status & STATUS_RXOK) { > > + dev_dbg(dev->dev.parent, > > + "Received a msg successfully\n"); > > + priv->write_reg(priv, CAN_STATUS, > > + (priv->current_status & > ~STATUS_RXOK)); > > + } > > Dito. Ditto > > + /* handle bus error events */ > > + if (priv->current_status & STATUS_EWARN) { > > + dev_info(dev->dev.parent, > > + "entered error warning state\n"); > > + error_type = CCAN_ERROR_WARNING; > > + } > > + if ((priv->current_status & STATUS_EPASS) && > > + (!(priv->last_status & STATUS_EPASS))) { > > + dev_info(dev->dev.parent, > > + "entered error passive state\n"); > > + error_type = CCAN_ERROR_PASSIVE; > > + } > > + if ((priv->current_status & STATUS_BOFF) && > > + (!(priv->last_status & STATUS_BOFF))) { > > + dev_info(dev->dev.parent, > > + "entered bus off state\n"); > > + error_type = CCAN_BUS_OFF; > > + } > > But you can keep these. Ok. :-) > > + if (priv->current_status & STATUS_LEC_MASK) > > + lec_type = (priv->current_status & STATUS_LEC_MASK); > > + > > + /* handle bus recovery events */ > > + if ((!(priv->current_status & STATUS_EPASS)) && > > + (priv->last_status & STATUS_EPASS)) { > > + dev_info(dev->dev.parent, > > + "left error passive state\n"); > > + priv->can.state = CAN_STATE_ERROR_ACTIVE; > > + } > > + if ((!(priv->current_status & STATUS_BOFF)) && > > + (priv->last_status & STATUS_BOFF)) { > > + dev_info(dev->dev.parent, > > + "left bus off state\n"); > > + priv->can.state = CAN_STATE_ERROR_ACTIVE; > > + } > > + > > + priv->last_status = priv->current_status; > > + > > + /* handle error on the bus */ > > + if (error_type != CCAN_NO_ERROR) { > > + ret = bosch_ccan_err(dev, error_type, lec_type); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void bosch_ccan_do_object_irq(struct net_device *dev, u16 > irqstatus) > > +{ > > + int i; > > + u32 val; > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) > > + struct net_device_stats *stats = can_get_stats(dev); > > +#else > > + struct net_device_stats *stats = &dev->stats; > > +#endif > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + if (irqstatus > MAX_TRANSMIT_OBJECT) { > > + val = bosch_ccan_read_reg32(dev, CAN_NEWDAT); > > + while (val & RECEIVE_OBJECT_BITS) { > > + for (i = MAX_TRANSMIT_OBJECT + 1; i <= > MAX_OBJECT; i++) > > + if (val & (1 << i)) > > + bosch_ccan_read_object(dev, > 0, i); > > + > > + val = bosch_ccan_read_reg32(dev, CAN_NEWDAT); > > + } > > + } else { > > + bosch_ccan_inval_object(dev, 0, irqstatus - 1); > > + val = bosch_ccan_read_reg32(dev, CAN_TXRQST); > > + if (!val) { > > + can_get_echo_skb(dev, 0); > > + priv->tx_object--; > > + stats->tx_packets++; > > + netif_wake_queue(dev); > > + } > > + } > > +} > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,19) > > +static irqreturn_t bosch_ccan_isr(int irq, void *dev_id, struct > pt_regs *regs) > > +#else > > +static irqreturn_t bosch_ccan_isr(int irq, void *dev_id) > > +#endif > > +{ > > + u16 irqstatus; > > + int ret; > > + struct net_device *dev = (struct net_device *)dev_id; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + irqstatus = priv->read_reg(priv, CAN_IR); > > + > > + if (!irqstatus) > > + return IRQ_NONE; > > + while (irqstatus) { > > + if (irqstatus == 0x8000) { > > Please use an appropriate macro definition. Ack. V2 will reflect this. > > + ret = bosch_ccan_do_status_irq(dev); > > + if (ret < 0) { > > + dev_err(dev->dev.parent, > > + "do_status_irq > failed\n"); > > + goto exit; > > + } > > + } else > > + bosch_ccan_do_object_irq(dev, irqstatus); > > + > > + irqstatus = priv->read_reg(priv, CAN_IR); > > + } > > + > > +exit: > > + return IRQ_HANDLED; > > +} > > + > > +static int bosch_ccan_open(struct net_device *dev) > > +{ > > + int err; > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + /* open the can device */ > > + err = open_candev(dev); > > + if (err) { > > + dev_err(dev->dev.parent, "failed to open can > device\n"); > > + return err; > > + } > > + > > + /* register interrupt handler */ > > + err = request_irq(dev->irq, &bosch_ccan_isr, priv->irq_flags, > dev->name, > > + (void *)dev); > > + if (err < 0) { > > + dev_err(dev->dev.parent, "failed to attach > interrupt\n"); > > + goto exit_irq_fail; > > + } > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) > > + /* clear statistics */ > > + memset(&priv->can.net_stats, 0, sizeof(priv->can.net_stats)); > > +#endif > > + > > + /* start the ccan controller */ > > + err = bosch_ccan_start(dev); > > + if (err) > > + goto exit_start_fail; > > + > > + netif_start_queue(dev); > > + > > + return 0; > > + > > +exit_start_fail: > > + free_irq(dev->irq, dev); > > +exit_irq_fail: > > + close_candev(dev); > > + return err; > > +} > > + > > +static int bosch_ccan_close(struct net_device *dev) > > +{ > > + netif_stop_queue(dev); > > + bosch_ccan_stop(dev); > > + free_irq(dev->irq, dev); > > + close_candev(dev); > > + > > + return 0; > > +} > > + > > +struct net_device *alloc_bosch_ccandev(int sizeof_priv) > > +{ > > + struct net_device *dev; > > + struct bosch_ccan_priv *priv; > > + > > +#if ((LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32)) > ||(LINUX_VERSION_CODE > KERNEL_VERSION(2,6,32))) > > + dev = alloc_candev(sizeof(struct bosch_ccan_priv) + > sizeof_priv, > > + BOSCH_CCAN_ECHO_SKB_MAX); > > +#else > > + dev = alloc_candev(sizeof(struct bosch_ccan_priv) + > sizeof_priv); > > +#endif > > + if (!dev) > > + return NULL; > > + > > + priv = netdev_priv(dev); > > + > > + priv->dev = dev; > > + priv->can.bittiming_const = &bosch_ccan_bittiming_const; > > + priv->can.do_set_bittiming = bosch_ccan_set_bittiming; > > + priv->can.do_get_state = bosch_ccan_get_state; > > + priv->can.do_set_mode = bosch_ccan_set_mode; > > Please also implement the "get_berr_counter" callback. Please see above. > > + return dev; > > +} > > +EXPORT_SYMBOL(alloc_bosch_ccandev); > > + > > +void free_bosch_ccandev(struct net_device *dev) > > +{ > > + free_candev(dev); > > +} > > +EXPORT_SYMBOL(free_bosch_ccandev); > > + > > +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28) > > +static const struct net_device_ops bosch_ccan_netdev_ops = { > > + .ndo_open = bosch_ccan_open, > > + .ndo_stop = bosch_ccan_close, > > + .ndo_start_xmit = bosch_ccan_start_xmit, > > +}; > > +#endif > > + > > +int register_bosch_ccandev(struct net_device *dev) > > +{ > > + dev->flags |= IFF_ECHO; /* we support local echo */ > > +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28) > > + dev->netdev_ops = &bosch_ccan_netdev_ops; > > +#else > > + dev->open = bosch_ccan_open; > > + dev->stop = bosch_ccan_close; > > + dev->hard_start_xmit = bosch_ccan_start_xmit; > > +#endif > > + > > + return register_candev(dev); > > +} > > +EXPORT_SYMBOL(register_bosch_ccandev); > > + > > +void unregister_bosch_ccandev(struct net_device *dev) > > +{ > > + struct bosch_ccan_priv *priv = netdev_priv(dev); > > + > > + /* disable all interrupts */ > > + priv->write_reg(priv, CAN_CONTROL, 0); > > + > > + unregister_candev(dev); > > +} > > +EXPORT_SYMBOL(unregister_bosch_ccandev); > > + > > +MODULE_AUTHOR("Bhupesh Sharma <[email protected]>"); > > +MODULE_LICENSE("GPL"); > > +MODULE_DESCRIPTION("CAN bus driver for Bosch CCAN controller"); > > > > Property changes on: bosch_ccan.c > > ___________________________________________________________________ > > Name: svn:executable > > + * > > > > Index: bosch_ccan.h > > =================================================================== > > --- bosch_ccan.h (revision 0) > > +++ bosch_ccan.h (revision 0) > > @@ -0,0 +1,207 @@ > > +/* > > + * drivers/net/can/bosch_ccan.h > > + * > > + * CAN bus driver definitions for Bosch CCAN controller > > + * > > + * Copyright (C) 2010 ST Microelectronics > > + * Bhupesh Sharma <[email protected]> > > + * > > + * Borrowed heavily from the CCAN driver originally written by: > > + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix > <[email protected]> > > + * - Simon Kallweit, intefo AG <[email protected]> > > + * which can be viewed here: > > + * http://svn.berlios.de/svnroot/repos/socketcan/trunk/kernel/2.6/ > > + * drivers/net/can/old/ccan/ccan.c > > + * > > + * Bosch CCAN controller is compliant to CAN protocol version 2.0 > part A and B. > > + * Bosch CCAN user manual can be obtained from: > > + * http://www.semiconductors.bosch.de/pdf/Users_Manual_C_CAN.pdf > > + * > > + * This file is licensed under the terms of the GNU General Public > > + * License version 2. This program is licensed "as is" without any > > + * warranty of any kind, whether express or implied. > > + */ > > See abocve. Ditto as is bosch_ccan.c > > +#ifndef __CCAN_H__ > > +#define __CCAN_H__ > > + > > +#include <linux/can.h> > > +#include <linux/clk.h> > > +#include <linux/platform_device.h> > > + > > +#define BOSCH_CCAN_ECHO_SKB_MAX 1 > > + > > +/* ccan register offsets */ > > +enum ccan_regs { > > + CAN_CONTROL = 0x00, > > + CAN_STATUS = 0x02, > > + CAN_ERROR = 0x04, > > + CAN_BTR = 0x06, > > + CAN_IR = 0x08, > > + CAN_TEST = 0x0a, > > + CAN_BRP_EXT = 0x0c, > > + CAN_IF1 = 0x10, > > + CAN_IF2 = 0x40, > > + CAN_TXRQST = 0x80, /* 32bit */ > > + CAN_NEWDAT = 0x90, /* 32bit */ > > + CAN_INTPND = 0xa0, /* 32bit */ > > + CAN_MSGVAL = 0xb0, /* 32bit */ > > +}; > > It's unusual to define register offsets as enumeration. Ok. I will check which can be a better way here and try to introduce the same in V2. > > +#define CAN_IF_COMR(x) (CAN_IF1 + (x) * 0x30 + 0x00) > > +#define CAN_IF_COMM(x) (CAN_IF1 + (x) * 0x30 + 0x02) > > +#define CAN_IF_MASK(x) (CAN_IF1 + (x) * 0x30 + 0x04) /* > 32bit */ > > +#define CAN_IF_ARB(x) (CAN_IF1 + (x) * 0x30 + 0x08) /* > 32bit */ > > +#define CAN_IF_MCONT(x) (CAN_IF1 + (x) * 0x30 + 0x0c) > > +#define CAN_IF_DATAA(x) (CAN_IF1 + (x) * 0x30 + 0x0e) > /* 32bit */ > > +#define CAN_IF_DATAB(x) (CAN_IF1 + (x) * 0x30 + 0x12) > /* 32bit */ > > Hm, I don't like these macros. Is there a better way to handle message > offsets? Either with "static inlines" or even better by using a struct > to define the register layout. Ok. I will check which can be a better way here and try to introduce the same in V2. > > +/* control register */ > > +#define CONTROL_TEST (1<<7) > > +#define CONTROL_CCE (1<<6) > > +#define CONTROL_DISABLE_AR (1<<5) > > +#define CONTROL_ENABLE_AR (0<<5) > > +#define CONTROL_EIE (1<<3) > > +#define CONTROL_SIE (1<<2) > > +#define CONTROL_IE (1<<1) > > +#define CONTROL_INIT (1<<0) > > + > > +/* test register */ > > +#define TEST_RX (1<<7) > > +#define TEST_TX1 (1<<6) > > +#define TEST_TX2 (1<<5) > > +#define TEST_LBACK (1<<4) > > +#define TEST_SILENT (1<<3) > > +#define TEST_BASIC (1<<2) > > + > > +/* status register */ > > +#define STATUS_BOFF (1<<7) > > +#define STATUS_EWARN (1<<6) > > +#define STATUS_EPASS (1<<5) > > +#define STATUS_RXOK (1<<4) > > +#define STATUS_TXOK (1<<3) > > +#define STATUS_LEC_MASK 0x07 > > +#define LEC_STUFF_ERROR 1 > > +#define LEC_FORM_ERROR 2 > > +#define LEC_ACK_ERROR 3 > > +#define LEC_BIT1_ERROR 4 > > +#define LEC_BIT0_ERROR 5 > > +#define LEC_CRC_ERROR 6 > > + > > +/* error counter register */ > > +#define ERR_COUNTER_TEC_MASK 0xff > > +#define ERR_COUNTER_TEC_SHIFT 0x0 > > +#define ERR_COUNTER_REC_SHIFT 8 > > +#define ERR_COUNTER_REC_MASK (0x7f<<ERR_COUNTER_REC_SHIFT) > > +#define ERR_COUNTER_RP_SHIFT 15 > > +#define ERR_COUNTER_RP_MASK (0x1<<ERR_COUNTER_RP_SHIFT) > > + > > +/* bit-timing register */ > > +#define BTR_BRP_MASK 0x3f > > +#define BTR_BRP_SHIFT 0 > > +#define BTR_SJW_SHIFT 6 > > +#define BTR_SJW_MASK (0x3<<BTR_SJW_SHIFT) > > +#define BTR_TSEG1_SHIFT 8 > > +#define BTR_TSEG1_MASK (0xf<<BTR_TSEG1_SHIFT) > > +#define BTR_TSEG2_SHIFT 12 > > +#define BTR_TSEG2_MASK (0x7<<BTR_TSEG2_SHIFT) > > + > > +/* brp extension register */ > > +#define BRP_EXT_BRPE_MASK 0x0f > > +#define BRP_EXT_BRPE_SHIFT 0 > > + > > +/* IFx command request */ > > +#define IF_COMR_BUSY (1<<15) > > + > > +/* IFx command mask */ > > +#define IF_COMM_WR (1<<7) > > +#define IF_COMM_MASK (1<<6) > > +#define IF_COMM_ARB (1<<5) > > +#define IF_COMM_CONTROL (1<<4) > > +#define IF_COMM_CLR_INT_PND (1<<3) > > +#define IF_COMM_TXRQST (1<<2) > > +#define IF_COMM_DATAA (1<<1) > > +#define IF_COMM_DATAB (1<<0) > > +#define IF_COMM_ALL (IF_COMM_MASK | IF_COMM_ARB | \ > > + IF_COMM_CONTROL | IF_COMM_TXRQST | \ > > + IF_COMM_DATAA | IF_COMM_DATAB) > > + > > +/* IFx arbitration */ > > +#define IF_ARB_MSGVAL (1<<31) > > +#define IF_ARB_MSGXTD (1<<30) > > +#define IF_ARB_TRANSMIT (1<<29) > > + > > +/* IFx message control */ > > +#define IF_MCONT_NEWDAT (1<<15) > > +#define IF_MCONT_MSGLST (1<<14) > > +#define IF_MCONT_INTPND (1<<13) > > +#define IF_MCONT_UMASK (1<<12) > > +#define IF_MCONT_TXIE (1<<11) > > +#define IF_MCONT_RXIE (1<<10) > > +#define IF_MCONT_RMTEN (1<<9) > > +#define IF_MCONT_TXRQST (1<<8) > > +#define IF_MCONT_EOB (1<<7) > > + > > +/* message object */ > > +#define MAX_OBJECT 31 > > +#define MAX_TRANSMIT_OBJECT 15 > > +#define RECEIVE_OBJECT_BITS 0xffff0000 > > + > > +/* > > + * CCAN operating modes: > > + * Support is available for default as well as test operating modes. > > + * Normal mode will generally be used as a default mode in most > cases, > > + * however, various test modes may be useful in specific use-cases. > > + */ > > +enum bosch_ccan_operating_mode { > > + CCAN_NORMAL_MODE = 0, > > + CCAN_BASIC_MODE, > > + CCAN_LOOPBACK_MODE, > > + CCAN_LOOPBACK_WITH_SILENT_MODE, > > + CCAN_SILENT_MODE > > +}; > > I do not see a need for another enumeration o the type. We already have > the CAN_CTRLMODE_*. Yes. But I don't see CAN_CTRLMODE_* capturing all the operating mode types supported by Bosch CCAN. > > +/* > > + * Automatic Retransmssion compliance with ISO11898, 6.3.3 Recovery > Management: > > + * Support is available for enabling automatic retransmission of > frames > > + * (default behavior) as well as disabling the same for TTCAN > > + * environments. > > + */ > > +enum bosch_ccan_auto_tx_config { > > + CCAN_ENABLE_AUTO_RE_TRANSMIT = 0, > > + CCAN_DISABLE_AUTO_RE_TRANSMIT > > +}; > > + > > +/* > > + * CCAN error types: > > + * Bus errors (BUS_OFF, ERROR_WARNING, ERROR_PASSIVE) are supported > > + */ > > +enum bosch_ccan_bus_error_types { > > + CCAN_NO_ERROR = 0, > > + CCAN_BUS_OFF, > > + CCAN_ERROR_WARNING, > > + CCAN_ERROR_PASSIVE > > +}; > > Ditto. Also CCAN_ERROR_PASSIVE=3 and you use "error_type & > CCAN_ERROR_PASSIVE" in your code. Ditto. > > +/* CCAN private data structure */ > > +struct bosch_ccan_priv { > > + struct can_priv can; /* must be the first member */ > > + struct net_device *dev; > > + int tx_object; > > + int current_status; > > + int last_status; > > + u16 (*read_reg) (const struct bosch_ccan_priv *priv, > > + enum ccan_regs reg); > > + void (*write_reg) (const struct bosch_ccan_priv *priv, > > + enum ccan_regs reg, u16 val); > > + void __iomem *reg_base; /* ioremap'ed address to registers > */ > > + unsigned long irq_flags; /* for request_irq() */ > > + struct clk *clk; > > +}; > > + > > +struct net_device *alloc_bosch_ccandev(int sizeof_priv); > > +void free_bosch_ccandev(struct net_device *dev); > > +int register_bosch_ccandev(struct net_device *dev); > > +void unregister_bosch_ccandev(struct net_device *dev); > > + > > +#endif /* __CCAN_H__ */ > > Wolfgang. Regards, Bhupesh _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
