Hello On 02/09/2011 11:54 AM, Bhupesh SHARMA wrote: [...] >> The driver looks quite good, some comments inline, most of them >> nitpicking and or style related. >> >> Have a look at the netif_stop_queue(). In the at91 driver there are two >> possibilities that to stop the queue. First the next tx mailbox is >> still in use, second we have a wrap around. But your hardware is a bit >> different. Anyways a second look doesn't harm. > > As the Tx/Rx path of my driver are based on at91 driver, I have earlier > gone through the possibility of stopping the tx queue in the two cases as > you mentioned above :) > > As per at91 specs, MRDY=0 signifies: > "Mailbox data registers cannot be read/written by the software application" > > But, after reading the Bosch C_CAN specs Transmission Request > Register(TxRqst)'s > bits if set to 1, signify that the transmission of this Message Object is > requested and is not yet done. If you agree we can add a check against the > same here. > Please do go through the Bosch C_CAN specs for details: > http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/ > c_can/users_manual_c_can.pdf
Better save then sorry, add the check. > >>> --- >>> Changes since V5: >>> 1. Seperated the state change and bus error handling paths. >>> 2. Added logic to write LEC value to 0x7 from CPU to check for >> updates >>> later. >>> 3. Corrected the ERROR_WARNING handling logic to correctly send error >>> frames on the bus. >>> >>> drivers/net/can/Kconfig | 2 + >>> drivers/net/can/Makefile | 1 + >>> drivers/net/can/c_can/Kconfig | 15 + >>> drivers/net/can/c_can/Makefile | 8 + >>> drivers/net/can/c_can/c_can.c | 993 >> ++++++++++++++++++++++++++++++++ >>> drivers/net/can/c_can/c_can.h | 230 ++++++++ >>> drivers/net/can/c_can/c_can_platform.c | 207 +++++++ >>> 7 files changed, 1456 insertions(+), 0 deletions(-) create mode >>> 100644 drivers/net/can/c_can/Kconfig create mode 100644 >>> drivers/net/can/c_can/Makefile create mode 100644 >>> drivers/net/can/c_can/c_can.c create mode 100644 >>> drivers/net/can/c_can/c_can.h create mode 100644 >>> drivers/net/can/c_can/c_can_platform.c >>> >>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index >>> 5dec456..1d699e3 100644 >>> --- a/drivers/net/can/Kconfig >>> +++ b/drivers/net/can/Kconfig >>> @@ -115,6 +115,8 @@ source "drivers/net/can/mscan/Kconfig" >>> >>> source "drivers/net/can/sja1000/Kconfig" >>> >>> +source "drivers/net/can/c_can/Kconfig" >>> + >>> source "drivers/net/can/usb/Kconfig" >>> >>> source "drivers/net/can/softing/Kconfig" >>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile >> index >>> 53c82a7..24ebfe8 100644 >>> --- a/drivers/net/can/Makefile >>> +++ b/drivers/net/can/Makefile >>> @@ -13,6 +13,7 @@ obj-y += softing/ >>> >>> obj-$(CONFIG_CAN_SJA1000) += sja1000/ >>> obj-$(CONFIG_CAN_MSCAN) += mscan/ >>> +obj-$(CONFIG_CAN_C_CAN) += c_can/ >>> obj-$(CONFIG_CAN_AT91) += at91_can.o >>> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o >>> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o >>> diff --git a/drivers/net/can/c_can/Kconfig >>> b/drivers/net/can/c_can/Kconfig new file mode 100644 index >>> 0000000..ffb9773 >>> --- /dev/null >>> +++ b/drivers/net/can/c_can/Kconfig >>> @@ -0,0 +1,15 @@ >>> +menuconfig CAN_C_CAN >>> + tristate "Bosch C_CAN devices" >>> + depends on CAN_DEV && HAS_IOMEM >>> + >>> +if CAN_C_CAN >>> + >>> +config CAN_C_CAN_PLATFORM >>> + tristate "Generic Platform Bus based C_CAN driver" >>> + ---help--- >>> + This driver adds support for the C_CAN chips connected to >>> + the "platform bus" (Linux abstraction for directly to the >>> + processor attached devices) which can be found on various >>> + boards from ST Microelectronics (http://www.st.com) >>> + like the SPEAr1310 and SPEAr320 evaluation boards. >>> +endif >>> diff --git a/drivers/net/can/c_can/Makefile >>> b/drivers/net/can/c_can/Makefile new file mode 100644 index >>> 0000000..9273f6d >>> --- /dev/null >>> +++ b/drivers/net/can/c_can/Makefile >>> @@ -0,0 +1,8 @@ >>> +# >>> +# Makefile for the Bosch C_CAN controller drivers. >>> +# >>> + >>> +obj-$(CONFIG_CAN_C_CAN) += c_can.o >>> +obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o >>> + >>> +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG >>> diff --git a/drivers/net/can/c_can/c_can.c >>> b/drivers/net/can/c_can/c_can.c new file mode 100644 index >>> 0000000..7ef4aa9 >>> --- /dev/null >>> +++ b/drivers/net/can/c_can/c_can.c >>> @@ -0,0 +1,993 @@ >>> +/* >>> + * CAN bus driver for Bosch C_CAN controller >>> + * >>> + * Copyright (C) 2010 ST Microelectronics >>> + * Bhupesh Sharma <[email protected]> >>> + * >>> + * Borrowed heavily from the C_CAN driver originally written by: >>> + * Copyright (C) 2007 >>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix >>> +<[email protected]> >>> + * - Simon Kallweit, intefo AG <[email protected]> >>> + * >>> + * TX and RX NAPI implementation has been borrowed from at91 CAN >>> +driver >>> + * written by: >>> + * Copyright >>> + * (C) 2007 by Hans J. Koch <[email protected]> >>> + * (C) 2008, 2009 by Marc Kleine-Budde <[email protected]> >>> + * >>> + * Bosch C_CAN controller is compliant to CAN protocol version 2.0 >> part A and B. >>> + * Bosch C_CAN user manual can be obtained from: >>> + * >> http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/c_can/ >>> + * 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. >>> + */ >>> + >>> +#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/io.h> >>> + >>> +#include <linux/can.h> >>> +#include <linux/can/dev.h> >>> +#include <linux/can/error.h> >>> + >>> +#include "c_can.h" >>> + >>> +static struct can_bittiming_const c_can_bittiming_const = { >>> + .name = KBUILD_MODNAME, >>> + .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 inline int get_tx_next_msg_obj(const struct c_can_priv *priv) >>> +{ >>> + return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) + >>> + C_CAN_MSG_OBJ_TX_FIRST; >>> +} >>> + >>> +static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv) >>> +{ >>> + return (priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) + >>> + C_CAN_MSG_OBJ_TX_FIRST; >>> +} >>> + >>> +static u32 c_can_read_reg32(struct c_can_priv *priv, void *reg) { >>> + u32 val = priv->read_reg(priv, reg); >>> + val |= ((u32) priv->read_reg(priv, reg + 2)) << 16; >>> + return val; >>> +} >>> + >>> +static void c_can_enable_all_interrupts(struct c_can_priv *priv, >>> + int enable) >>> +{ >>> + unsigned int cntrl_save = priv->read_reg(priv, >>> + &priv->regs->control); >>> + >>> + if (enable) >>> + cntrl_save |= (CONTROL_SIE | CONTROL_EIE | CONTROL_IE); >>> + else >>> + cntrl_save &= ~(CONTROL_EIE | CONTROL_IE | CONTROL_SIE); >>> + >>> + priv->write_reg(priv, &priv->regs->control, cntrl_save); } >>> + >>> +static inline int c_can_check_busy_status(struct c_can_priv *priv, >>> +int iface) { >>> + int count = MIN_TIMEOUT_VALUE; >>> + >>> + while (count && priv->read_reg(priv, >>> + &priv->regs->ifregs[iface].com_req) & >>> + IF_COMR_BUSY) { >>> + count--; >>> + udelay(1); >>> + } >>> + >>> + return count; >> >> it's an unusual return value...maybe return 0 on success and -EBUSY >> otherwise? > > Hmm.. this will add the checking MIN_TIMEOUT_VALUE against 0 here, You mean check "count" against 0 here... > instead of "c_can_object_get" and "c_can_object_put" routines. > If you persist we can add the same in V7 though.. :) We have a function "c_can_check_busy_status()". What does it return? The name doesn't tell me. I think it would be more clear if you just rename the function to "c_can_is_busy()" or "c_can_object_is_busy()". The you can use it like this: if (c_can_is_busy()) { printk("mailbox still busy!\n"); return -EWHATEVER; } > >>> +} >>> + >>> +static inline void c_can_object_get(struct net_device *dev, >>> + int iface, int objno, int mask) >>> +{ >>> + int ret; >>> + struct c_can_priv *priv = netdev_priv(dev); >>> + >>> + /* >>> + * As per specs, after writting the message object number in the >>> + * IF command request register the transfer b/w interface >>> + * register and message RAM must be complete in 6 CAN-CLK >>> + * period. >>> + */ >>> + priv->write_reg(priv, &priv->regs->ifregs[iface].com_mask, >>> + IFX_WRITE_LOW_16BIT(mask)); >>> + priv->write_reg(priv, &priv->regs->ifregs[iface].com_req, >>> + IFX_WRITE_LOW_16BIT(objno)); >>> + >>> + ret = c_can_check_busy_status(priv, iface); >>> + if (!ret) >>> + netdev_err(dev, "timed out in object get\n"); } There's no error handling for the object is busy case.... [...] >>> diff --git a/drivers/net/can/c_can/c_can.h >>> b/drivers/net/can/c_can/c_can.h new file mode 100644 index >>> 0000000..bd094e6 >>> --- /dev/null >>> +++ b/drivers/net/can/c_can/c_can.h >>> @@ -0,0 +1,230 @@ >>> +/* >>> + * CAN bus driver for Bosch C_CAN controller >>> + * >>> + * Copyright (C) 2010 ST Microelectronics >>> + * Bhupesh Sharma <[email protected]> >>> + * >>> + * Borrowed heavily from the C_CAN driver originally written by: >>> + * Copyright (C) 2007 >>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix >>> +<[email protected]> >>> + * - Simon Kallweit, intefo AG <[email protected]> >>> + * >>> + * Bosch C_CAN controller is compliant to CAN protocol version 2.0 >> part A and B. >>> + * Bosch C_CAN user manual can be obtained from: >>> + * >> http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/c_can/ >>> + * 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. >>> + */ >>> + >>> +#ifndef C_CAN_H >>> +#define C_CAN_H >>> + >>> +/* control register */ >>> +#define CONTROL_TEST BIT(7) >>> +#define CONTROL_CCE BIT(6) >>> +#define CONTROL_DISABLE_AR BIT(5) >>> +#define CONTROL_ENABLE_AR (0 << 5) >>> +#define CONTROL_EIE BIT(3) >>> +#define CONTROL_SIE BIT(2) >>> +#define CONTROL_IE BIT(1) >>> +#define CONTROL_INIT BIT(0) >>> + >>> +/* test register */ >>> +#define TEST_RX BIT(7) >>> +#define TEST_TX1 BIT(6) >>> +#define TEST_TX2 BIT(5) >>> +#define TEST_LBACK BIT(4) >>> +#define TEST_SILENT BIT(3) >>> +#define TEST_BASIC BIT(2) >>> + >>> +/* status register */ >>> +#define STATUS_BOFF BIT(7) >>> +#define STATUS_EWARN BIT(6) >>> +#define STATUS_EPASS BIT(5) >>> +#define STATUS_RXOK BIT(4) >>> +#define STATUS_TXOK BIT(3) >>> + >>> +/* error counter register */ >>> +#define ERR_CNT_TEC_MASK 0xff >>> +#define ERR_CNT_TEC_SHIFT 0 >>> +#define ERR_CNT_REC_SHIFT 8 >>> +#define ERR_CNT_REC_MASK (0x7f << ERR_CNT_REC_SHIFT) >>> +#define ERR_CNT_RP_SHIFT 15 >>> +#define ERR_CNT_RP_MASK (0x1 << ERR_CNT_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 BIT(15) >>> + >>> +/* IFx command mask */ >>> +#define IF_COMM_WR BIT(7) >>> +#define IF_COMM_MASK BIT(6) >>> +#define IF_COMM_ARB BIT(5) >>> +#define IF_COMM_CONTROL BIT(4) >>> +#define IF_COMM_CLR_INT_PND BIT(3) >>> +#define IF_COMM_TXRQST BIT(2) >>> +#define IF_COMM_DATAA BIT(1) >>> +#define IF_COMM_DATAB BIT(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 BIT(15) >>> +#define IF_ARB_MSGXTD BIT(14) >>> +#define IF_ARB_TRANSMIT BIT(13) >>> + >>> +/* IFx message control */ >>> +#define IF_MCONT_NEWDAT BIT(15) >>> +#define IF_MCONT_MSGLST BIT(14) >>> +#define IF_MCONT_CLR_MSGLST (0 << 14) >>> +#define IF_MCONT_INTPND BIT(13) >>> +#define IF_MCONT_UMASK BIT(12) >>> +#define IF_MCONT_TXIE BIT(11) >>> +#define IF_MCONT_RXIE BIT(10) >>> +#define IF_MCONT_RMTEN BIT(9) >>> +#define IF_MCONT_TXRQST BIT(8) >>> +#define IF_MCONT_EOB BIT(7) >>> +#define IF_MCONT_DLC_MASK 0xf >>> + >>> +/* >>> + * IFx register masks: >>> + * allow easy operation on 16-bit registers when the >>> + * argument is 32-bit instead >>> + */ >>> +#define IFX_WRITE_LOW_16BIT(x) ((x) & 0xFFFF) >>> +#define IFX_WRITE_HIGH_16BIT(x) (((x) & 0xFFFF0000) >> 16) >>> + >>> +/* message object split */ >>> +#define C_CAN_NO_OF_OBJECTS 32 >>> +#define C_CAN_MSG_OBJ_RX_NUM 16 >>> +#define C_CAN_MSG_OBJ_TX_NUM 16 >>> + >>> +#define C_CAN_MSG_OBJ_RX_FIRST 1 >>> +#define C_CAN_MSG_OBJ_RX_LAST (C_CAN_MSG_OBJ_RX_FIRST + \ >>> + C_CAN_MSG_OBJ_RX_NUM - 1) >>> + >>> +#define C_CAN_MSG_OBJ_TX_FIRST (C_CAN_MSG_OBJ_RX_LAST + 1) >>> +#define C_CAN_MSG_OBJ_TX_LAST (C_CAN_MSG_OBJ_TX_FIRST + \ >>> + C_CAN_MSG_OBJ_TX_NUM - 1) >>> + >>> +#define C_CAN_MSG_OBJ_RX_SPLIT 9 >>> +#define C_CAN_MSG_RX_LOW_LAST (C_CAN_MSG_OBJ_RX_SPLIT - 1) >>> + >>> +#define C_CAN_NEXT_MSG_OBJ_MASK (C_CAN_MSG_OBJ_TX_NUM - 1) >>> +#define RECEIVE_OBJECT_BITS 0x0000ffff >>> + >>> +/* status interrupt */ >>> +#define STATUS_INTERRUPT 0x8000 >>> + >>> +/* global interrupt masks */ >>> +#define ENABLE_ALL_INTERRUPTS 1 >>> +#define DISABLE_ALL_INTERRUPTS 0 >>> + >>> +/* minimum timeout for checking BUSY status */ >>> +#define MIN_TIMEOUT_VALUE 6 >>> + >>> +/* napi related */ >>> +#define C_CAN_NAPI_WEIGHT C_CAN_MSG_OBJ_RX_NUM >>> + >>> +/* c_can IF registers */ >>> +struct c_can_if_regs { >>> + u16 com_req; >>> + u16 com_mask; >>> + u16 mask1; >>> + u16 mask2; >>> + u16 arb1; >>> + u16 arb2; >>> + u16 msg_cntrl; >>> + u16 data[4]; >>> + u16 _reserved[13]; >>> +}; >>> + >>> +/* c_can hardware registers */ >>> +struct c_can_regs { >>> + u16 control; >>> + u16 status; >>> + u16 err_cnt; >>> + u16 btr; >>> + u16 interrupt; >>> + u16 test; >>> + u16 brp_ext; >>> + u16 _reserved1; >>> + struct c_can_if_regs ifregs[2]; /* [0] = IF1 and [1] = IF2 */ >>> + u16 _reserved2[8]; >>> + u16 txrqst1; >>> + u16 txrqst2; >>> + u16 _reserved3[6]; >>> + u16 newdat1; >>> + u16 newdat2; >>> + u16 _reserved4[6]; >>> + u16 intpnd1; >>> + u16 intpnd2; >>> + u16 _reserved5[6]; >>> + u16 msgval1; >>> + u16 msgval2; >>> + u16 _reserved6[6]; >>> +}; >>> + >>> +/* c_can lec values */ >>> +enum c_can_lec_type { >>> + LEC_NO_ERROR = 0, >>> + LEC_STUFF_ERROR, >>> + LEC_FORM_ERROR, >>> + LEC_ACK_ERROR, >>> + LEC_BIT1_ERROR, >>> + LEC_BIT0_ERROR, >>> + LEC_CRC_ERROR, >>> + LEC_UNUSED, >>> +}; >>> + >>> +/* >>> + * c_can error types: >>> + * Bus errors (BUS_OFF, ERROR_WARNING, ERROR_PASSIVE) are supported >>> +*/ enum c_can_bus_error_types { >>> + C_CAN_NO_ERROR = 0, >>> + C_CAN_BUS_OFF, >>> + C_CAN_ERROR_WARNING, >>> + C_CAN_ERROR_PASSIVE, >>> +}; >> >> nitpick: are the defines, enums and structs needed in more than one c >> file? If not, please move them into the c-file where they are used. > > Well most of the strcuts/defines are useful in both `c_can.c` and > `c_can_platform.c`, but I will explore how to separate the rest in > the respective c-files. But inititally we agreed to a *sja1000* like > approach and hence this placement in h-file. Yes, your're right. But keeping only in one .c or .h file used things out of the .h file is a good rule for cleaner code :) 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 |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
