Re: [PATCH net-next-2.6 v6 1/1] can: c_can: Added support for Bosch C_CAN controller

[Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 18526 bytes --]

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 <bhupesh.sharma-qxv4g6HH51o@public.gmane.org>
>>> + *
>>> + * Borrowed heavily from the C_CAN driver originally written by:
>>> + * Copyright (C) 2007
>>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
>>> +<s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> + * - Simon Kallweit, intefo AG <simon.kallweit-+G9qxTFKJT/tRgLqZ5aouw@public.gmane.org>
>>> + *
>>> + * TX and RX NAPI implementation has been borrowed from at91 CAN
>>> +driver
>>> + * written by:
>>> + * Copyright
>>> + * (C) 2007 by Hans J. Koch <hjk-vqZO0P4V72/QD6PfKP4TzA@public.gmane.org>
>>> + * (C) 2008, 2009 by Marc Kleine-Budde <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> + *
>>> + * 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 <bhupesh.sharma-qxv4g6HH51o@public.gmane.org>
>>> + *
>>> + * Borrowed heavily from the C_CAN driver originally written by:
>>> + * Copyright (C) 2007
>>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
>>> +<s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> + * - Simon Kallweit, intefo AG <simon.kallweit-+G9qxTFKJT/tRgLqZ5aouw@public.gmane.org>
>>> + *
>>> + * 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   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core