| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Jan 2011 09:41:21 +0100
From: Wolfgang Grandegger <wg@...ndegger.com>
To: Bhupesh SHARMA <bhupesh.sharma@...com>
CC: "Socketcan-core@...ts.berlios.de" <Socketcan-core@...ts.berlios.de>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Marc Kleine-Budde <mkl@...gutronix.de>,
David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next-2.6 v4 1/1] can: c_can: Added support for Bosch
C_CAN controller
On 01/12/2011 09:38 AM, Bhupesh SHARMA wrote:
> Hi Wolfgang,
>
>> On 01/12/2011 04:30 AM, Bhupesh SHARMA wrote:
>>> Hi Wolfgang,
>>>
>>>> -----Original Message-----
>>>> From: Wolfgang Grandegger [mailto:wg@...ndegger.com]
>>>> Hi Bhupesh,
>>>>
>>>> some final nitpicking as you need to fix Marc remarks anyway...
>>>>
>>>> On 01/11/2011 12:49 PM, Bhupesh Sharma wrote:
>>>>> Bosch C_CAN controller is a full-CAN implementation which 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 patch adds the support for this controller.
>>>>> The following are the design choices made while writing the
>>>> controller
>>>>> driver:
>>>>> 1. Interface Register set IF1 has be used only in the current
>> design.
>>>>> 2. Out of the 32 Message objects available, 16 are kept aside for
>> RX
>>>>> purposes and the rest for TX purposes.
>>>>> 3. NAPI implementation is such that both the TX and RX paths
>> function
>>>>> in polling mode.
>>>>>
>>>>> Changes since V3:
>>>>> 1. Corrected commenting style.
>>>>> 2. Removing *non-required* header files from driver files.
>>>>> 3. Removed extra *non-required* outer brackets globally.
>>>>> 4. Implemented and used a inline routine to check BUSY status.
>>>>> 5. Added a board-specific param *priv* inside the c_can_priv
>> struct.
>>>>>
>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...com>
>>>>
>>>> ...
>>>>> +++ b/drivers/net/can/c_can/c_can.h
>>>>> @@ -0,0 +1,235 @@
>>>>> +/*
>>>>> + * CAN bus driver for Bosch C_CAN controller
>>>>> + *
>>>>> + * Copyright (C) 2010 ST Microelectronics
>>>>> + * Bhupesh Sharma <bhupesh.sharma@...com>
>>>>> + *
>>>>> + * Borrowed heavily from the C_CAN driver originally written by:
>>>>> + * Copyright (C) 2007
>>>>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
>>>> <s.hauer@...gutronix.de>
>>>>> + * - Simon Kallweit, intefo AG <simon.kallweit@...efo.ch>
>>>>> + *
>>>>> + * TX and RX NAPI implementation has been borrowed from at91 CAN
>>>> driver
>>>>> + * written by:
>>>>> + * Copyright
>>>>> + * (C) 2007 by Hans J. Koch <hjk@...utronix.de>
>>>>> + * (C) 2008, 2009 by Marc Kleine-Budde <kernel@...gutronix.de>
>>>>> + *
>>>>> + * 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)
>>>>> +#define STATUS_LEC_MASK 0x07
>>>>> +
>>>>> +/* error counter register */
>>>>> +#define ERR_COUNTER_TEC_MASK 0xff
>>>>> +#define ERR_COUNTER_TEC_SHIFT 0
>>>>> +#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)
>>>>
>>>> s/ERR_COUNTER/ERR_CNT/ to match the member name.
>>>
>>> Ok.
>>>
>>>>> +
>>>>> +/* 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 31
>>>>> +#define C_CAN_MSG_OBJ_RX_NUM 16
>>>>> +#define C_CAN_MSG_OBJ_TX_NUM 16
>>>>> +
>>>>> +#define C_CAN_MSG_OBJ_RX_FIRST 0
>>>>> +#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 8
>>>>> +#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_reg;
>>>>> + 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 intf[2]; /* [0] = IF1 and [1] = IF2 */
>>>>
>>>> I not happy with the name "intf". Sounds more like an interrupt
>> related
>>>> property. Above you already use the prefix IF_ for the corresponding
>>>> macro definitions and somewhere else the name "iface".
>>>
>>> I tried using the name *if* suggested by you here but the compiler
>> will complain
>>> considering it as a usage of if-construct. Do you have a better name
>> that we
>>> can use here?
>>
>> Oh, I was not aware ot that. Sorry for the noise! Then your old
>> "ifregs"
>> or "iface" would be fine, I think. I just see that the pch_can uses
>> "ifregs" as well.
>>
>
> Yes, I get checked the pch_can driver as well. It also uses the name *ifregs*.
> Let's keep the name *ifregs* then.
D'accord.
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