Topcliff: Update PCH_CAN driver to 2.6.35
Subsystems (3)
PATCH v3 1/1 Topcliff: Update PCH_CAN driver to 2.6.35
Hi Wolfgang and Marc,
We have modified a pretty amount of our driver based on other accepted Socket CAN driver. Additionally, We have reduced the number of lines 3601 to 1444.
Please check below.
Thanks, Ohtake(OKISemi)
---
CAN driver of Topcliff PCH
Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has CAN I/F. This driver enables CAN function.
--- drivers/net/can/Kconfig | 8 + drivers/net/can/Makefile | 1 + drivers/net/can/pch_can.c | 1444 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1453 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pch_can.c
drivers/net/can/Kconfig +8 -0
@@ -73,6 +73,14 @@ config CAN_JANZ_ICAN3 This driver can also be built as a module. If so, the module will be called janz-ican3.ko. +config PCH_CAN + tristate "PCH CAN" + depends on CAN_DEV + ---help--- + This driver is for PCH CAN of Topcliff which is an IOH for x86 + embedded processor. + This driver can access CAN bus. + source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"
drivers/net/can/Makefile +1 -0
@@ -16,5 +16,6 @@ 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 +obj-$(CONFIG_PCH_CAN) += pch_can.o ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
drivers/net/can/pch_can.c new file +1444 -0
1,444 lines not on this page · raw message
Signature · 1 line
1.6.0.6
Hi Ohtake,
Quoted 4 lines · Masayuki Ohtak
Hi Wolfgang and Marc,
We have modified a pretty amount of our driver based on other accepted Socket CAN driver. Additionally, We have reduced the number of lines 3601 to 1444.
Much better now! I need some more time for a full review, though.
Thanks,
Wolfgang.
Hi Ohtake,
here comes my review, sorry for delay.
Quoted 4 lines · Masayuki Ohtak
Hi Wolfgang and Marc,
We have modified a pretty amount of our driver based on other accepted Socket CAN driver. Additionally, We have reduced the number of lines 3601 to 1444.
Much better, but I believe it could be reduced even further.
Quoted 44 lines
Please check below.
Thanks, Ohtake(OKISemi)
---
CAN driver of Topcliff PCHTopcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has CAN I/F. This driver enables CAN function.
Signed-off-by Masayuki Ohtake <[email protected]>--- drivers/net/can/Kconfig | 8 + drivers/net/can/Makefile | 1 + drivers/net/can/pch_can.c | 1444 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1453 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pch_can.cdrivers/net/can/Kconfig +8
-0@@ -73,6 +73,14 @@ config CAN_JANZ_ICAN3 This driver can also be built as a module. If so, the module will be called janz-ican3.ko. +config PCH_CAN + tristate "PCH CAN" + depends on CAN_DEV + ---help--- + This driver is for PCH CAN of Topcliff which is an IOH for x86 + embedded processor. + This driver can access CAN bus. + source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"drivers/net/can/Makefile +0
-0
drivers/net/can/Makefile obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o · line 19
Quoted 4 lines
drivers/net/can/Makefile +1
-0@@ -16,5 +16,6 @@ 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 +obj-$(CONFIG_PCH_CAN) += pch_can.o
Please provide patches against David Millers "net-next-2.6" GIT tree and use the prefix "can: " in your subject next time. See
http://svn.berlios.de/wsvn/socketcan/trunk/README.submitting-patches
for further information.
Quoted 32 lines
drivers/net/can/Makefile +0
-0ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUGdrivers/net/can/pch_can.c new file +25
-0@@ -0,0 +1,1444 @@ +/* +* Copyright (C) 1999 - 2010 Intel Corporation. +* Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; version 2 of the License. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA. +*/ + +#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/sched.h> +#include <linux/pci.h> +#include <linux/init.h>
drivers/net/can/pch_can.c line 35
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/errno.h> +#include <linux/netdevice.h> +#include <linux/skbuff.h> +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> + +#define MAX_BITRATE 0x3e8
Dead code? At least it's not used anywhere.
Quoted 48 lines
drivers/net/can/pch_can.c new file +47
-0+ +#define MAX_MSG_OBJ 32 +#define MSG_OBJ_RX 0/* The receive message object flag. */ +#define MSG_OBJ_TX 1/* The transmit message object flag. */ + +#define ENABLE 1/* The enable flag */ +#define DISABLE 0/* The disable flag */ +#define CAN_CTRL_INIT 0x0001/* The INIT bit of CANCONT register. */ +#define CAN_CTRL_IE 0x0002/* The IE bit of CAN control register */ +#define CAN_CTRL_IE_SIE_EIE 0x000e +#define CAN_CTRL_CCE 0x0040 +#define CAN_CTRL_OPT 0x0080/* The OPT bit of CANCONT register. */ +#define CAN_OPT_SILENT 0x0008/* The Silent bit of CANOPT reg. */ +#define CAN_OPT_LBACK 0x0010/* The LoopBack bit of CANOPT reg. */ +#define CAN_CMASK_RX_TX_SET 0x00f3 +#define CAN_CMASK_RX_TX_GET 0x0073 +#define CAN_CMASK_ALL 0xff +#define CAN_CMASK_RDWR 0x80 +#define CAN_CMASK_ARB 0x20 +#define CAN_CMASK_CTRL 0x10 +#define CAN_CMASK_MASK 0x40 + +#define CAN_IF_MCONT_NEWDAT 0x8000 +#define CAN_IF_MCONT_INTPND 0x2000 +#define CAN_IF_MCONT_UMASK 0x1000 +#define CAN_IF_MCONT_TXIE 0x0800 +#define CAN_IF_MCONT_RXIE 0x0400 +#define CAN_IF_MCONT_RMTEN 0x0200 +#define CAN_IF_MCONT_TXRQXT 0x0100 +#define CAN_IF_MCONT_EOB 0x0080 +#define CAN_IF_MCONT_MSGLOST 0x4000 +#define CAN_MASK2_MDIR_MXTD 0xc000 +#define CAN_ID2_DIR 0x2000 +#define CAN_ID_MSGVAL 0x8000 + +#define CAN_STATUS_INT 0x8000 +#define CAN_IF_CREQ_BUSY 0x8000 +#define CAN_ID2_XTD 0x4000 + +#define CAN_REC 0x00007f00 +#define CAN_TEC 0x000000ff + +#define PCH_RX_OK 0x00000010 +#define PCH_TX_OK 0x00000008 +#define PCH_BUS_OFF 0x00000080 +#define PCH_EWARN 0x00000040 +#define PCH_EPASSIV 0x00000020
drivers/net/can/pch_can.c line 92
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+#define PCH_LEC0 0x00000001 +#define PCH_LEC1 0x00000002 +#define PCH_LEC2 0x00000004 +#define PCH_LEC_ALL (PCH_LEC0 | PCH_LEC1 | PCH_LEC2) +#define PCH_STUF_ERR PCH_LEC0 +#define PCH_FORM_ERR PCH_LEC1 +#define PCH_ACK_ERR (PCH_LEC0 | PCH_LEC1) +#define PCH_BIT1_ERR PCH_LEC2 +#define PCH_BIT0_ERR (PCH_LEC0 | PCH_LEC2) +#define PCH_CRC_ERR (PCH_LEC1 | PCH_LEC2)
enum {
PCH_LEC_STUF_ERR = 0,
PCH_LEC_FORM_ERR, PCH_LEC_ACK_ERR, ... PCH_LEC_ALL };
Seems more appropriate. More comments below.
Quoted 13 lines
drivers/net/can/pch_can.c new file +13
-0+ +/* bit position of certain controller bits. */ +#define BIT_BITT_BRP 0 +#define BIT_BITT_SJW 6 +#define BIT_BITT_TSEG1 8 +#define BIT_BITT_TSEG2 12 +#define BIT_IF1_MCONT_RXIE 10 +#define BIT_IF2_MCONT_TXIE 11 +#define BIT_BRPE_BRPE 6 +#define BIT_ES_TXERRCNT 0 +#define BIT_ES_RXERRCNT 8 +#define MSK_BITT_BRP 0x3f +#define MSK_BITT_SJW 0xc0
drivers/net/can/pch_can.c line 115
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+#define MSK_BITT_TSEG1 0xf00 +#define MSK_BITT_TSEG2 0x7000 +#define MSK_BRPE_BRPE 0x3c0 +#define MSK_BRPE_GET 0x0f +#define MSK_CTRL_IE_SIE_EIE 0x07 +#define MSK_MCONT_TXIE 0x08 +#define MSK_MCONT_RXIE 0x10 +#define PCH_CAN_NO_TX_BUFF 1 +#define PCI_DEVICE_ID_INTEL_PCH1_CAN 0x8818 +#define COUNTER_LIMIT 0xFFFF
Keep alignment?
Quoted 1 line
+#define
PCH_CAN_CLK50000 /* 50MHz */
Please specify it in Hz already here.
drivers/net/can/pch_can.c line 121
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+ +/* Total 32 OBJs */ +#define PCH_RX_OBJ_NUM 1 +#define PCH_TX_OBJ_NUM 1 +#define PCH_OBJ_NUM (PCH_TX_OBJ_NUM + PCH_RX_OBJ_NUM)
Please explain biefly what message object are use for what purpose. Either here or in the initialization code.
drivers/net/can/pch_can.c line 131
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +#define PCH_CAN_ACTIVE 0 +#define PCH_CAN_LISTEN 1 +#define PCH_CAN_STOP 0 +#define PCH_CAN_RUN 1 + +#define PCH_CAN_ENABLE 0 +#define PCH_CAN_DISABLE 1 +#define PCH_CAN_ALL 2 +#define PCH_CAN_NONE 3
The above are used in switch case and should therefore be anonymous enums. I suggested to remove them because I'm not a real friend of the helper functions which are just called *once*.
Quoted 33 lines
drivers/net/can/pch_can.c new file +33
-0+ +struct pch_can_regs { +u32 cont; +u32 stat; +u32 errc; +u32 bitt; +u32 intr; +u32 opt; +u32 brpe; +u32 reserve1; +u32 if1_creq; +u32 if1_cmask; +u32 if1_mask1; +u32 if1_mask2; +u32 if1_id1; +u32 if1_id2; +u32 if1_mcont; +u32 if1_dataa1; +u32 if1_dataa2; +u32 if1_datab1; +u32 if1_datab2; +u32 reserve2; +u32 reserve3[12 ]; +u32 if2_creq; +u32 if2_cmask; +u32 if2_mask1; +u32 if2_mask2; +u32 if2_id1; +u32 if2_id2; +u32 if2_mcont; +u32 if2_dataa1; +u32 if2_dataa2; +u32 if2_datab1;
drivers/net/can/pch_can.c line 174
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+u32 if2_datab2; +u32 reserve4; +u32 reserve5[20 ]; +u32 treq1; +u32 treq2; +u32 reserve6[2 ]; +u32 reserve7[56 ]; +u32 reserve8[3 ]; +u32 srst; +};
Nice.
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+struct pch_can_priv { +struct can_priv can; +void __iomem *base; +unsigned int can_num; +struct pci_dev *dev;
drivers/net/can/pch_can.c line 190
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+unsigned int tx_enable[MAX_MSG_OBJ ]; +unsigned int rx_enable[MAX_MSG_OBJ ]; +unsigned int rx_link[MAX_MSG_OBJ ]; +unsigned int int_enables; +unsigned int int_stat; +unsigned int bus_off_interrupt; +struct net_device *ndev; +spinlock_t msgif_reg_lock;/* Message Interface Registers Access Lock*/ +unsigned int msg_obj[MAX_MSG_OBJ ]; +struct pch_can_regs *regs;
Please add __iomem. Do you need both, regs *and* base?
drivers/net/can/pch_can.c line 194
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+}; + +static struct can_bittiming_const pch_can_bittiming_const = { + .name =KBUILD_MODNAME ,
Not sure what KBUILD_MODNAME is. Should be "pch_can", the name of the driver.
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+ .tseg1_min = 1, + .tseg1_max = 16, + .tseg2_min = 1, + .tseg2_max = 8,
drivers/net/can/pch_can.c line 208
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ .sjw_max = 4, + .brp_min = 1, + .brp_max = 1024, /* 6bit + extended 4bit */ + .brp_inc = 1, +}; + +static const struct pci_device_id pch_can_pcidev_id[]__devinitdata = { + {PCI_DEVICE (PCI_VENDOR_ID_INTEL ,PCI_DEVICE_ID_INTEL_PCH1_CAN )}, + {} +};
Please use DEFINE_PCI_DEVICE_TABLE.
drivers/net/can/pch_can.c line 212
Quoted 3 lines
drivers/net/can/pch_can.c new file +3
-0+static inline void pch_can_bit_set (u32 *addr,u32 mask) +{ +iowrite32 ((ioread32 (addr) | mask), addr);
Outer brackets not needed!
drivers/net/can/pch_can.c line 217
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+} + +static inline void pch_can_bit_clear (u32 *addr,u32 mask) +{ +iowrite32 ((ioread32 (addr) & ~(mask)), addr);
Ditto.
Quoted 12 lines
drivers/net/can/pch_can.c new file +12
-0+} + +static void pch_can_set_run_mode (struct pch_can_priv *priv,u32 mode) +{ +switch (mode) { +case PCH_CAN_RUN : +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_INIT ); +break ; + +case PCH_CAN_STOP : +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_INIT ); +break ;
drivers/net/can/pch_can.c line 239
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +default : +dev_err (&priv->ndev->dev,"%s -> Invalid Mode.\n" , __func__); +break ; + } +} + +static void pch_can_get_run_mode (struct pch_can_priv *priv,u32 *mode) +{ +u32 reg_val =ioread32 (&(priv->regs)->cont);
I don't think you need the brackets around "priv->regs". Therefore I suggest s/&(priv->regs)/&priv->regs/ for the whole file.
drivers/net/can/pch_can.c line 245
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ +if (reg_val &CAN_CTRL_INIT ) + *mode =PCH_CAN_STOP ; +else + *mode =PCH_CAN_RUN ; +}
These are the helper functions I complained about above. And reg_val is not really needed.
Quoted 21 lines
drivers/net/can/pch_can.c new file +21
-0+static void pch_can_set_optmode (struct pch_can_priv *priv) +{ +u32 reg_val =ioread32 (&(priv->regs)->opt); + +if (priv->can.ctrlmode &CAN_CTRLMODE_LISTENONLY ) + reg_val |=CAN_OPT_SILENT ; + +if (priv->can.ctrlmode &CAN_CTRLMODE_LOOPBACK ) + reg_val |=CAN_OPT_LBACK ; + +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_OPT ); +iowrite32 (reg_val, &(priv->regs)->opt); +} + +static void pch_can_set_int_custom (struct pch_can_priv *priv) +{ +/* Clearing the IE, SIE and EIE bits of Can control register. */ +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_IE_SIE_EIE ); + +/* Appropriately setting them. */ + pch_can_bit_set(&(priv->regs)->cont,
drivers/net/can/pch_can.c line 277
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ ((priv->int_enables & MSK_CTRL_IE_SIE_EIE) << 1)); +} + +/* This function retrieves interrupt enabled for the CAN device. */ +static void pch_can_get_int_enables (struct pch_can_priv *priv,u32 *enables) +{ +u32 reg_ctrl_val =ioread32 (&(priv->regs)->cont); + +/* Obtaining the status of IE, SIE and EIE interrupt bits. */ + *enables = ((reg_ctrl_val &CAN_CTRL_IE_SIE_EIE ) >>1 );
Do you really need an extra variable?
Quoted 32 lines
drivers/net/can/pch_can.c new file +32
-0+} + +static void pch_can_set_int_enables (struct pch_can_priv *priv,u32 interrupt_no) +{ +switch (interrupt_no) { +case PCH_CAN_ENABLE : +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_IE ); +break ; + +case PCH_CAN_DISABLE : +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_IE ); +break ; + +case PCH_CAN_ALL : +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_IE_SIE_EIE ); +break ; + +case PCH_CAN_NONE : +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_IE_SIE_EIE ); +break ; + +default : +dev_err (&priv->ndev->dev,"Invalid interrupt number.\n" ); +break ; + } +} + +static void pch_can_check_if1_busy (struct pch_can_priv *priv,u32 num) +{ +u32 counter =COUNTER_LIMIT ; +u32 if1_creq; +
drivers/net/can/pch_can.c line 319
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+iowrite32 (num, &(priv->regs)->if1_creq); +while (counter) { + if1_creq = (ioread32 (&(priv->regs)->if1_creq)) & +CAN_IF_CREQ_BUSY ; +if (!if1_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF1 BUSY Flag is set forever.\n" );
Please use a defined delay for the above timeout. How long does it usually take the bit to toggle? A small delay, e.g. udelay(1) could be fine. This function is called in the time critical path!
Quoted 8 lines
drivers/net/can/pch_can.c new file +8
-0+} + +static void pch_can_check_if2_busy (struct pch_can_priv *priv,u32 num) +{ +u32 counter =COUNTER_LIMIT ; +u32 if2_creq; + +iowrite32 (num, &(priv->regs)->if2_creq);
drivers/net/can/pch_can.c line 337
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+while (counter) { + if2_creq = (ioread32 (&(priv->regs)->if2_creq)) & +CAN_IF_CREQ_BUSY ; +if (!if2_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF2 BUSY Flag is set forever.\n" ); +}
Duplicated code!
drivers/net/can/pch_can.c line 345
Quoted 7 lines
drivers/net/can/pch_can.c new file +7
-0+static void pch_can_set_rx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 set) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +/*Reading the receive buffer data from RAM to Interface1 registers */
Space after /* ?
Quoted 149 lines
drivers/net/can/pch_can.c new file +149
-0+iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buff_num);/* Read from MsgRAN */ + +/* Setting the IF1MASK1 register to access MsgVal and RxIE bits */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if1_cmask)); + +if (set ==ENABLE ) { +/* Setting the MsgVal and RxIE bits */ +pch_can_bit_set (&(priv->regs)->if1_mcont,CAN_IF_MCONT_RXIE ); +pch_can_bit_set (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + + }else if (set ==DISABLE ) { +/* Resetting the MsgVal and RxIE bits */ +pch_can_bit_clear (&(priv->regs)->if1_mcont,CAN_IF_MCONT_RXIE ); +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + } + +pch_can_check_if1_busy (priv, buff_num);/* Write to MsgRAM */ +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_rx_enable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as receivers. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) +pch_can_set_rx_enable (priv, i +1 ,ENABLE ); + } +} + +static void pch_can_rx_disable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as receivers. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) +pch_can_set_rx_enable (priv, (i +1 ),DISABLE ); + } +} + +static void pch_can_set_tx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 set) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +/* Reading the Msg buffer from Message RAM to Interface2 registers. */ +iowrite32 (CAN_CMASK_RX_TX_GET , (&(priv->regs)->if1_cmask)); +pch_can_check_if1_busy (priv, buff_num); + +/* Setting the IF2CMASK register for accessing the +MsgVal and TxIE bits */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if1_cmask)); + +if (set ==ENABLE ) { +/* Setting the MsgVal and TxIE bits */ +pch_can_bit_set (&(priv->regs)->if1_mcont,CAN_IF_MCONT_TXIE ); +pch_can_bit_set (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + }else if (set ==DISABLE ) { +/* Resetting the MsgVal and TxIE bits. */ +pch_can_bit_clear (&(priv->regs)->if1_mcont,CAN_IF_MCONT_TXIE ); +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + } + +pch_can_check_if1_busy (priv, buff_num);/* Write to MsgRAM */ +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_tx_enable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as transmit object. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_set_tx_enable (priv, (i +1 ),ENABLE ); + } +} + +static void pch_can_tx_disable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as transmit object. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_set_tx_enable (priv, (i +1 ),DISABLE ); + } +} + +static void pch_can_get_rx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 *enable) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , (&(priv->regs)->if1_cmask)); +pch_can_check_if1_busy (priv, buff_num); + +if (((ioread32 (&(priv->regs)->if1_id2)) &CAN_ID_MSGVAL ) && + ((ioread32 (&(priv->regs)->if1_mcont)) & +CAN_IF_MCONT_RXIE )) + *enable =ENABLE ; +else + *enable =DISABLE ; +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_get_tx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 *enable) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buff_num); + +if (((ioread32 (&(priv->regs)->if1_id2)) &CAN_ID_MSGVAL ) && + ((ioread32 (&(priv->regs)->if1_mcont)) & +CAN_IF_MCONT_TXIE )) { + *enable =ENABLE ; + }else { + *enable =DISABLE ; + } +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static int pch_can_int_pending (struct pch_can_priv *priv) +{ +return ioread32 (&(priv->regs)->intr) &0xffff ; +} + +static void pch_can_set_rx_buffer_link (struct pch_can_priv *priv, +u32 buffer_num,u32 set) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buffer_num); +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_CTRL ), &(priv->regs)->if1_cmask); +if (set ==ENABLE ) +pch_can_bit_clear (&(priv->regs)->if1_mcont,CAN_IF_MCONT_EOB ); +else
drivers/net/can/pch_can.c line 504
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_bit_set (&(priv->regs)->if1_mcont,CAN_IF_MCONT_EOB ); + +pch_can_check_if1_busy (priv, buffer_num); +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_get_rx_buffer_link (struct pch_can_priv *priv, +u32 buffer_num,u32 *link) +{ +u32 reg_val;
Really needed?
Quoted 9 lines
drivers/net/can/pch_can.c new file +9
-0+unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buffer_num); + + reg_val =ioread32 (&(priv->regs)->if1_mcont); +if (reg_val &CAN_IF_MCONT_EOB ) + *link =DISABLE ;
drivers/net/can/pch_can.c line 523
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+else + *link =ENABLE ; +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_clear_buffers (struct pch_can_priv *priv) +{ +u32 i; +u32 rx_buff_num; +u32 tx_buff_num;
Really needed?
Quoted 68 lines
drivers/net/can/pch_can.c new file +68
-0+ +iowrite32 (CAN_CMASK_RX_TX_SET , &(priv->regs)->if1_cmask); +iowrite32 (CAN_CMASK_RX_TX_SET , &(priv->regs)->if2_cmask); +iowrite32 (0xffff , &(priv->regs)->if1_mask1); +iowrite32 (0xffff , &(priv->regs)->if1_mask2); +iowrite32 (0xffff , &(priv->regs)->if2_mask1); +iowrite32 (0xffff , &(priv->regs)->if2_mask2); + +iowrite32 (0x0 , &(priv->regs)->if1_id1); +iowrite32 (0x0 , &(priv->regs)->if1_id2); +iowrite32 (0x0 , &(priv->regs)->if2_id1); +iowrite32 (0x0 , &(priv->regs)->if2_id2); +iowrite32 (0x0 , &(priv->regs)->if1_mcont); +iowrite32 (0x0 , &(priv->regs)->if2_mcont); +iowrite32 (0x0 , &(priv->regs)->if1_dataa1); +iowrite32 (0x0 , &(priv->regs)->if1_dataa2); +iowrite32 (0x0 , &(priv->regs)->if1_datab1); +iowrite32 (0x0 , &(priv->regs)->if1_datab2); +iowrite32 (0x0 , &(priv->regs)->if2_dataa1); +iowrite32 (0x0 , &(priv->regs)->if2_dataa2); +iowrite32 (0x0 , &(priv->regs)->if2_datab1); +iowrite32 (0x0 , &(priv->regs)->if2_datab2); + +for (i =1 ; i <= (MAX_MSG_OBJ /2 ); i++) { + rx_buff_num =2 * i; + tx_buff_num = (2 * i) -1 ; + +iowrite32 (rx_buff_num, &(priv->regs)->if1_creq); +iowrite32 (tx_buff_num, &(priv->regs)->if2_creq); + +mdelay (10 ); + } +} + +static void pch_can_config_rx_tx_buffers (struct pch_can_priv *priv) +{ +u32 i; +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +/* For accssing MsgVal, ID and EOB bit */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if1_cmask)); +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if2_cmask)); +iowrite32 (0x0 , (&(priv->regs)->if1_id1)); +iowrite32 (0x0 , (&(priv->regs)->if1_id2)); + +/* Resetting DIR bit for reception */ +iowrite32 (0x0 , (&(priv->regs)->if2_id1)); + +/* Setting DIR bit for transmission */ +iowrite32 ((CAN_ID2_DIR | (0x7ff <<2 )), + (&(priv->regs)->if2_id2)); + +/* Setting EOB bit for receiver */ +iowrite32 (CAN_IF_MCONT_EOB , &(priv->regs)->if1_mcont); + +/* Setting EOB bit for transmitter */ +iowrite32 (CAN_IF_MCONT_EOB , (&(priv->regs)->if2_mcont)); + +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) +pch_can_check_if1_busy (priv, i +1 ); +else if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_check_if2_busy (priv, i +1 ); +else +dev_err (&priv->ndev->dev,"Invalid OBJ\n" );
drivers/net/can/pch_can.c line 601
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) { +iowrite32 (CAN_CMASK_RX_TX_GET , + &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, i+1 ); + +pch_can_bit_clear (&(priv->regs)->if1_id2,0x1fff ); +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID2_XTD );
Could'nt it be set just by one call?
Quoted 17 lines
drivers/net/can/pch_can.c new file +17
-0+iowrite32 (0 , (&(priv->regs)->if1_id1)); +pch_can_bit_set (&(priv->regs)->if1_id2,0 ); +pch_can_bit_set (&(priv->regs)->if1_mcont, +CAN_IF_MCONT_UMASK ); +pch_can_bit_set (&(priv->regs)->if2_mcont, +CAN_IF_MCONT_UMASK ); + +iowrite32 (0 , &(priv->regs)->if1_mask1); +pch_can_bit_clear (&(priv->regs)->if1_mask2,0x1fff ); +pch_can_bit_clear (&(priv->regs)->if1_mask2, +CAN_MASK2_MDIR_MXTD ); + +iowrite32 (0 , &(priv->regs)->if2_mask1); +pch_can_bit_clear (&(priv->regs)->if2_mask2,0x1fff ); + +/* Setting CMASK for writing */ + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_MASK |
drivers/net/can/pch_can.c line 628
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ CAN_CMASK_ARB | CAN_CMASK_CTRL), + (&(priv->regs)->if1_cmask)); + +pch_can_check_if1_busy (priv, i+1 ); + } + } +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_open (struct pch_can_priv *priv)
Probably pch_can_init is the better name.
Quoted 11 lines
drivers/net/can/pch_can.c new file +11
-0+{ +/* Stopping the Can device. */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Clearing all the message object buffers. */ +pch_can_clear_buffers (priv); + +/* Configuring the respective message object as either rx/tx object. */ +pch_can_config_rx_tx_buffers (priv); + +/* Enabling all receive objects. */
drivers/net/can/pch_can.c line 649
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_rx_enable_all (priv); + +/* Enabling all transmit objects. */ +pch_can_tx_enable_all (priv); + +/* Enabling the interrupts. */ +pch_can_set_int_enables (priv,PCH_CAN_ALL ); + +/* Setting the CAN to run mode. */ +pch_can_set_run_mode (priv,PCH_CAN_RUN );
Hm, you start the controller here... more later.
Quoted 35 lines
drivers/net/can/pch_can.c new file +35
-0+} + +static void pch_can_release (struct pch_can_priv *priv) +{ +/* Stooping the CAN device. */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Disabling the interrupts. */ +pch_can_set_int_enables (priv,PCH_CAN_NONE ); + +/* Disabling all the receive object. */ +pch_can_rx_disable_all (priv); + +/* Disabling all the transmit object. */ +pch_can_tx_disable_all (priv); +} + +/* This function clears interrupt(s) from the CAN device. */ +static void pch_can_int_clr (struct pch_can_priv *priv,u32 mask) +{ +if (mask ==CAN_STATUS_INT ) { +ioread32 (&(priv->regs)->stat); +return ; + } + +/* Clear interrupt for transmit object */ +if (priv->msg_obj[mask -1 ] ==MSG_OBJ_TX ) { +/* Setting CMASK for clearing interrupts for +frame transmission. */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_CTRL |CAN_CMASK_ARB ), + (&(priv->regs)->if2_cmask)); + +/* Resetting the ID registers. */ +pch_can_bit_set (&(priv->regs)->if2_id2, + (CAN_ID2_DIR | (0x7ff <<2 )));
drivers/net/can/pch_can.c line 694
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+iowrite32 (0x0 , (&(priv->regs)->if2_id1)); + +/* Claring NewDat, TxRqst & IntPnd */ +pch_can_bit_clear (&(priv->regs)->if2_mcont, + (CAN_IF_MCONT_NEWDAT |CAN_IF_MCONT_INTPND | +CAN_IF_MCONT_TXRQXT )); +pch_can_check_if2_busy (priv, mask); + } +/* Clear interrupt for receive object */ +else if (priv->msg_obj[mask -1 ] ==MSG_OBJ_RX ) {
Should be "} else if ..."
Quoted 9 lines
drivers/net/can/pch_can.c new file +9
-0+/* Setting CMASK for clearing the reception interrupts. */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_CTRL |CAN_CMASK_ARB ), + (&(priv->regs)->if2_cmask)); + +/* Clearing the Dir bit. */ +pch_can_bit_clear (&(priv->regs)->if2_id2,CAN_ID2_DIR ); + +/* Clearing NewDat & IntPnd */ + pch_can_bit_clear(&(priv->regs)->if2_mcont,
drivers/net/can/pch_can.c line 713
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ (CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND)); + +pch_can_check_if2_busy (priv, mask); + } +} + +static int pch_can_get_buffer_status (struct pch_can_priv *priv) +{ +u32 reg_treq1; +u32 reg_treq2;
Really needed?
Quoted 30 lines
drivers/net/can/pch_can.c new file +30
-0+ +/* Reading the transmission request registers. */ + reg_treq1 = (ioread32 (&(priv->regs)->treq1) &0xffff ); + reg_treq2 = ((ioread32 (&(priv->regs)->treq2) &0xffff ) <<16 ); + +return reg_treq1 | reg_treq2; +} + +static void pch_can_reset (struct pch_can_priv *priv) +{ +/* write to sw reset register */ +iowrite32 (1 , (&(priv->regs)->srst)); +iowrite32 (0 , (&(priv->regs)->srst)); +} + +static void pch_can_msg_obj (struct net_device *ndev,u32 status) +{ +struct pch_can_priv *priv =netdev_priv (ndev); +u32 reg; +struct sk_buff *skb; +struct can_frame *cf; +canid_t id; +u32 ide; +u32 rtr; +int i, j; +struct net_device_stats *stats = &(priv->ndev->stats); + +/* Reading the messsage object from the Message RAM */ +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if2_cmask); +pch_can_check_if2_busy (priv, status);
drivers/net/can/pch_can.c line 753
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +/* Reading the MCONT register. */ + reg =ioread32 (&(priv->regs)->if2_mcont); + reg &=0xffff ; + +/* If MsgLost bit set. */ +if (reg &CAN_IF_MCONT_MSGLOST ) { +pch_can_bit_clear (&(priv->regs)->if2_mcont, +CAN_IF_MCONT_MSGLOST ); +dev_err (&priv->ndev->dev,"Msg Obj is overwritten.\n" );
That should create an error message as well.
drivers/net/can/pch_can.c line 759
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ } +/* Read the direction bit for determination of remote frame . */ + rtr = (ioread32 ((&(priv->regs)->if2_id2)) &CAN_ID2_DIR ); +/* Clearing interrupts. */ +pch_can_int_clr (priv, status); +/* Hanlde reception interrupt */
Typo!
Quoted 32 lines
drivers/net/can/pch_can.c new file +32
-0+if (priv->msg_obj[status -1 ] ==MSG_OBJ_RX ) { +if (!(reg &CAN_IF_MCONT_NEWDAT )) { +dev_err (&priv->ndev->dev,"MCONT_NEWDAT isn't SET.\n" ); +return ; + } + skb =alloc_can_skb (priv->ndev, &cf); +if (!skb) +return ; + + ide = ((ioread32 (&(priv->regs)->if2_id2)) &CAN_ID2_XTD ) >>14 ; +if (ide) { + id = (ioread32 (&(priv->regs)->if2_id1) &0xffff ); + id |= (((ioread32 (&(priv->regs)->if2_id2)) & +0x1fff ) <<16 ); + cf->can_id = (id &CAN_EFF_MASK ) |CAN_EFF_FLAG ; + }else { + id = (((ioread32 (&(priv->regs)->if2_id2)) & + (CAN_SFF_MASK <<2 )) >>2 ); + cf->can_id = (id &CAN_SFF_MASK ); + } + +if (rtr) { + cf->can_dlc =0 ; + cf->can_id |=CAN_RTR_FLAG ; + }else { + cf->can_dlc = ((ioread32 (&(priv->regs)->if2_mcont)) & +0x0f ); + } + +/* Reading back the data. */ +for (i =0 , j =0 ; i < cf->can_dlc; j++) { + reg =ioread32 (&(priv->regs)->if2_dataa1 + j*4 );
drivers/net/can/pch_can.c line 801
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ cf->data[i++] =cpu_to_le32 (reg &0xff ); +if (i == cf->can_dlc) +break ; + cf->data[i++] =cpu_to_le32 ((reg & (0xff <<8 )) >>8 ); + } +netif_rx (skb); + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; + }else if (priv->msg_obj[status -1 ] ==MSG_OBJ_TX ) { +/* Hanlde transmission interrupt */
Typo!
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+can_get_echo_skb (priv->ndev,0 ); +netif_wake_queue (priv->ndev); + } +} + +static void pch_can_error (struct net_device *ndev,u32 status)
drivers/net/can/pch_can.c line 817
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+{ +struct sk_buff *skb; +struct pch_can_priv *priv =netdev_priv (ndev); +struct can_frame *cf; +u32 errc; +struct net_device_stats *stats = &(priv->ndev->stats); + + skb =alloc_can_err_skb (ndev, &cf); +if (!skb) { +dev_err (&ndev->dev,"%s -> No memory.\n" , __func__);
Please drop the error message.
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+return ; + } + +if (status &PCH_BUS_OFF ) {
drivers/net/can/pch_can.c line 831
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+if (!priv->bus_off_interrupt) { +pch_can_tx_disable_all (priv); +pch_can_rx_disable_all (priv); + + priv->can.state =CAN_STATE_BUS_OFF ; + cf->can_id |=CAN_ERR_BUSOFF ; +can_bus_off (ndev); + + priv->bus_off_interrupt =1 ; +pch_can_set_run_mode (priv,PCH_CAN_RUN );
Hm, you automatically restart the contoller after a bus-off. That's not the intended behaviour. It's up to the user to define how and when the device should recover from bus-off. For further information read
http://lxr.linux.no/#linux+v2.6.35.7/Documentation/networking/can.txt#L767
Quoted 38 lines
drivers/net/can/pch_can.c new file +37
-0+ } + } +/* Warning interrupt. */ +if (status &PCH_EWARN ) { + priv->can.state =CAN_STATE_ERROR_WARNING ; + priv->can.can_stats.error_warning++; + cf->can_id |=CAN_ERR_CRTL ; + errc =ioread32 ((&(priv->regs)->errc)); +if (((errc &CAN_REC ) >>8 ) >96 ) + cf->data[1 ] |=CAN_ERR_CRTL_RX_WARNING ; +if ((errc &CAN_TEC ) >96 ) + cf->data[1 ] |=CAN_ERR_CRTL_TX_WARNING ; +dev_warn (&ndev->dev,"%s -> Warning interrupt.\n" , __func__); + } +/* Error passive interrupt. */ +if (status &PCH_EPASSIV ) { + priv->can.can_stats.error_passive++; + priv->can.state =CAN_STATE_ERROR_PASSIVE ; + cf->can_id |=CAN_ERR_CRTL ; + errc =ioread32 ((&(priv->regs)->errc)); +if (((errc &CAN_REC ) >>8 ) >127 ) + cf->data[1 ] |=CAN_ERR_CRTL_RX_PASSIVE ; +if ((errc &CAN_TEC ) >127 ) + cf->data[1 ] |=CAN_ERR_CRTL_TX_PASSIVE ; +dev_err (&ndev->dev, +"%s -> Error interrupt.\n" , __func__); + } + +if (status &PCH_STUF_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_STUFF ; + +if (status &PCH_FORM_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_FORM ; + +if (status &PCH_ACK_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_ACK |CAN_ERR_PROT_LOC_ACK_DEL ; +
drivers/net/can/pch_can.c line 879
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+if ((status &PCH_BIT1_ERR ) || (status &PCH_BIT0_ERR )) + cf->data[2 ] |=CAN_ERR_PROT_BIT ; + +if (status &PCH_CRC_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_CRC_SEQ | +CAN_ERR_PROT_LOC_CRC_DEL ; + +if (status &PCH_LEC_ALL ) +iowrite32 (status |PCH_LEC_ALL , + &(priv->regs)->stat);
A bit-wise test of the above values is wrong, I believe. Please use the switch statement instead.
drivers/net/can/pch_can.c line 886
Quoted 7 lines
drivers/net/can/pch_can.c new file +7
-0+ + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +netif_rx (skb); +} + +static irqreturn_t pch_can_handler (int irq,void *dev_id)
A better name making clear that it's the interrupt handler would be nice.
Quoted 15 lines
drivers/net/can/pch_can.c new file +15
-0+{ +u32 int_stat; +u32 reg_stat; +struct net_device *ndev = (struct net_device *)dev_id; +struct pch_can_priv *priv =netdev_priv (ndev); + int_stat =pch_can_int_pending (priv); + +if (!int_stat) +return IRQ_NONE ; + +if (int_stat ==CAN_STATUS_INT ) { + reg_stat =ioread32 ((&(priv->regs)->stat)); +if (reg_stat & (PCH_BUS_OFF |PCH_LEC_ALL |PCH_EWARN | +PCH_EPASSIV )) { +if ((reg_stat &PCH_LEC_ALL ) !=PCH_LEC_ALL )
drivers/net/can/pch_can.c line 911
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_error (ndev, reg_stat); + } + +/* Recover from Bus Off */ +if (!reg_stat && priv->bus_off_interrupt) { + priv->bus_off_interrupt =0 ; +pch_can_tx_enable_all (priv); +pch_can_rx_enable_all (priv); + +dev_info (&priv->ndev->dev,"BusOff stage recovered.\n" );
Bogus bus-off handling, more later...
drivers/net/can/pch_can.c line 918
Quoted 7 lines
drivers/net/can/pch_can.c new file +7
-0+ } + +if (reg_stat &PCH_RX_OK ) +pch_can_bit_clear (&(priv->regs)->stat,PCH_RX_OK ); + +if (reg_stat &PCH_TX_OK ) +pch_can_bit_clear (&(priv->regs)->stat,PCH_TX_OK );
Could be done in one call, I think.
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ int_stat =pch_can_int_pending (priv); + } + +if ((int_stat >0 ) && (int_stat <=MAX_MSG_OBJ )) +pch_can_msg_obj (ndev, int_stat); +
drivers/net/can/pch_can.c line 935
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+return IRQ_HANDLED ; +} + +static int pch_set_bittiming (struct net_device *ndev) +{ +struct pch_can_priv *priv =netdev_priv (ndev); +const struct can_bittiming *bt = &priv->can.bittiming; +u32 curr_mode; +u32 reg1;/* CANBIT */ +u32 reg2;/* BEPE */
Why not "u32 canbit" then?
drivers/net/can/pch_can.c line 940
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+u32 brp; + +pch_can_get_run_mode (priv, &curr_mode); +if (curr_mode ==PCH_CAN_RUN ) +pch_can_set_run_mode (priv,PCH_CAN_STOP );
The device is stopped when this function is called. Please remove.
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ +/* Setting the CCE bit for accessing the Can Timing register. */ +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_CCE ); + + brp = (bt->tq) / (1000000 /PCH_CAN_CLK ) -1 ; + reg1 = brp &MSK_BITT_BRP ;
drivers/net/can/pch_can.c line 956
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ reg1 |= (bt->sjw -1 ) <<BIT_BITT_SJW ; + reg1 |= (bt->phase_seg1 + bt->prop_seg -1 ) <<BIT_BITT_TSEG1 ; + reg1 |= (bt->phase_seg2 -1 ) <<BIT_BITT_TSEG2 ; + reg2 = (brp &MSK_BRPE_BRPE ) >>BIT_BRPE_BRPE ; +iowrite32 (reg1, (&(priv->regs)->bitt)); +iowrite32 (reg2, (&(priv->regs)->brpe)); +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_CCE ); + +if (curr_mode ==PCH_CAN_RUN ) +pch_can_set_run_mode (priv,PCH_CAN_RUN );
Ditto.
drivers/net/can/pch_can.c line 970
Quoted 13 lines
drivers/net/can/pch_can.c new file +13
-0+return 0 ; +} + +static void pch_can_start (struct net_device *ndev) +{ +struct pch_can_priv *priv =netdev_priv (ndev); + +if (priv->can.state !=CAN_STATE_STOPPED ) +pch_can_reset (priv); + +pch_set_bittiming (ndev); +pch_can_set_optmode (priv); + priv->can.state =CAN_STATE_ERROR_ACTIVE ;
Hm, where do you really start the controller. I'm missing pch_can_set_run_mode(priv, PCH_CAN_RUN).
Quoted 9 lines
drivers/net/can/pch_can.c new file +9
-0+return ; +} + +static int pch_can_do_set_mode (struct net_device *ndev,enum can_mode mode) +{ +int ret =0 ; + +switch (mode) { +case CAN_MODE_START :
drivers/net/can/pch_can.c line 990
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_start (ndev); +netif_wake_queue (ndev); +break ; +default : + ret = -EOPNOTSUPP ; +break ; + } + +return ret; +}
Note that this function is called when the device will recover from bus-off.
drivers/net/can/pch_can.c line 999
Quoted 8 lines
drivers/net/can/pch_can.c new file +8
-0+static int pch_can_get_state (const struct net_device *ndev, +enum can_state *state) +{ +struct pch_can_priv *priv =netdev_priv (ndev); + + *state = priv->can.state; +return 0 ; +}
There is no need for that function as the driver handles state changes in the interrupt handler.
Quoted 1 line
+static int
pch_open(struct net_device *ndev)
That's confussing! Please use the prefix pch_can throught this file.
drivers/net/can/pch_can.c line 1006
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+{ +struct pch_can_priv *priv =netdev_priv (ndev); +int retval; + +pch_can_open (priv);
This function already starts the controller, which is too *early*.
Quoted 16 lines
drivers/net/can/pch_can.c new file +16
-0+ + retval =pci_enable_msi (priv->dev); +if (retval) { +dev_err (&ndev->dev,"Unable to allocate MSI ret=%d\n" , retval); + goto pci_en_msi_err; + } + +/* Regsitering the interrupt. */ + retval =request_irq (priv->dev->irq, pch_can_handler,IRQF_SHARED , + ndev->name, ndev); +if (retval) { +dev_err (&ndev->dev,"request_irq failed.\n" ); + goto req_irq_err; + } + +/* Assuming that no bus off interrupt. */
drivers/net/can/pch_can.c line 1032
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ priv->bus_off_interrupt =0 ; + +/* Open common can device */ + retval =open_candev (ndev); +if (retval) { +dev_err (ndev->dev.parent,"open_candev() failed %d\n" , retval); + goto err_open_candev; + } + +pch_can_start (ndev);
Thde above function should finally start the controller.
Quoted 64 lines
drivers/net/can/pch_can.c new file +64
-0+netif_start_queue (ndev); + +return 0 ; + +err_open_candev: +free_irq (priv->dev->irq, ndev); +req_irq_err: +pci_disable_msi (priv->dev); +pci_en_msi_err: +pch_can_release (priv); + +return retval; +} + +static int pch_close (struct net_device *ndev) +{ +struct pch_can_priv *priv =netdev_priv (ndev); + +netif_stop_queue (ndev); +pch_can_release (priv); +free_irq (priv->dev->irq, ndev); +pci_disable_msi (priv->dev); +close_candev (ndev); + priv->can.state =CAN_STATE_STOPPED ; +return 0 ; +} + +static int pch_get_free_msg_obj (struct net_device *ndev) +{ +u32 buffer_status =0 ; +u32 tx_disable_counter =0 ; +u32 tx_buffer_avail =0 ; +u32 status; +s32 i; +struct pch_can_priv *priv =netdev_priv (ndev); + +/* Getting the message object status. */ + buffer_status = (u32 )pch_can_get_buffer_status (priv); + +/* Getting the free transmit message object. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if ((priv->msg_obj[i] ==MSG_OBJ_TX )) { +/* Checking whether the object is enabled. */ +pch_can_get_tx_enable (priv, i +1 , &status); +if (status ==ENABLE ) { +if (!((buffer_status >> i) &1 )) { + tx_buffer_avail = (i +1 ); +break ; + } + }else { + tx_disable_counter++; + } + } + } + +/* If no transmit object available. */ +if (!tx_buffer_avail) { +/* If no object is enabled. */ +if ((tx_disable_counter ==PCH_TX_OBJ_NUM )) { +dev_err (&ndev->dev,"All tx buffers are disabled.\n" ); +return -EPERM ; + }else { +dev_err (&ndev->dev,"%s:No tx buf free.\n" , __func__); +return -PCH_CAN_NO_TX_BUFF ;
drivers/net/can/pch_can.c line 1106
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + } +return tx_buffer_avail; +} + +static netdev_tx_t pch_xmit (struct sk_buff *skb,struct net_device *ndev) +{ +canid_t id; +u32 id1 =0 ; +u32 id2 =0 ;
Need these values to be preset?
drivers/net/can/pch_can.c line 1108
Quoted 2 lines
drivers/net/can/pch_can.c new file +2
-0+u32 run_mode; +u32 i, j;
It's common to use type "int" for the usual incrementer value... as you do in other places as well. Please check!
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+unsigned long flags; +struct pch_can_priv *priv =netdev_priv (ndev); +struct can_frame *cf = (struct can_frame *)skb->data; +struct net_device_stats *stats = &ndev->stats; +u32 tx_buffer_avail =0 ;
drivers/net/can/pch_can.c line 1123
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +if (can_dropped_invalid_skb (ndev, skb)) +return NETDEV_TX_OK ; + +/* Getting the current CAN mode. */ +pch_can_get_run_mode (priv, &run_mode); +if (run_mode !=PCH_CAN_RUN ) { +dev_err (&ndev->dev,"CAN stopped on transmit attempt.\n" ); +return -EPERM ; + }
Can this happen? I think this check can be removed. Anyway, -EPERM is not a valid return value for that function.
drivers/net/can/pch_can.c line 1127
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+ + tx_buffer_avail =pch_get_free_msg_obj (ndev); +if (tx_buffer_avail <0 ) +return tx_buffer_avail;
Wrong return value?
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ +/* Attaining the lock. */ +spin_lock_irqsave (&priv->msgif_reg_lock, flags); + +/* Reading the Msg Obj from the Msg RAM to the Interface register. */ +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask);
drivers/net/can/pch_can.c line 1143
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_check_if1_busy (priv, tx_buffer_avail); + +/* Setting the CMASK register. */ +pch_can_bit_set (&(priv->regs)->if1_cmask,CAN_CMASK_ALL ); + +/* If ID extended is set. */ +if (cf->can_id &CAN_EFF_FLAG ) { + id = cf->can_id &CAN_EFF_MASK ; + id1 = id &0xffff ; + id2 = ((id & (0x1fff <<16 )) >>16 ) |CAN_ID2_XTD ;
Please use some more macro definitions for the sake of readability.
Quoted 32 lines
drivers/net/can/pch_can.c new file +32
-0+ }else { + id = cf->can_id &CAN_SFF_MASK ; + id1 =0 ; + id2 = ((id &CAN_SFF_MASK ) <<2 ); + } +pch_can_bit_clear (&(priv->regs)->if1_id1,0xffff ); +pch_can_bit_clear (&(priv->regs)->if1_id2,0x1fff |CAN_ID2_XTD ); +pch_can_bit_set (&(priv->regs)->if1_id1, id1); +pch_can_bit_set (&(priv->regs)->if1_id2, id2); + +/* If remote frame has to be transmitted.. */ +if (cf->can_id &CAN_RTR_FLAG ) +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID2_DIR ); + +for (i =0 , j =0 ; i < cf->can_dlc; j++) { +iowrite32 (le32_to_cpu (cf->data[i++]), + (&(priv->regs)->if1_dataa1) + j*4 ); +if (i == cf->can_dlc) +break ; +iowrite32 (le32_to_cpu (cf->data[i++] <<8 ), + (&(priv->regs)->if1_dataa1) + j*4 ); + } +can_put_echo_skb (skb, ndev,0 ); + +/* Updating the size of the data. */ +pch_can_bit_clear (&(priv->regs)->if1_mcont,0x0f ); +pch_can_bit_set (&(priv->regs)->if1_mcont, cf->can_dlc); + +/* Clearing IntPend, NewDat & TxRqst */ +pch_can_bit_clear (&(priv->regs)->if1_mcont, + (CAN_IF_MCONT_NEWDAT |CAN_IF_MCONT_INTPND | +CAN_IF_MCONT_TXRQXT ));
drivers/net/can/pch_can.c line 1185
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +/* Setting NewDat, TxRqst bits */ +pch_can_bit_set (&(priv->regs)->if1_mcont, + (CAN_IF_MCONT_NEWDAT |CAN_IF_MCONT_TXRQXT )); + +pch_can_check_if1_busy (priv, tx_buffer_avail); +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); + + stats->tx_bytes += cf->can_dlc; + stats->tx_packets++;
That shoould be incremented when the TX done interrupt is handled.
Quoted 40 lines
drivers/net/can/pch_can.c new file +40
-0+ +return NETDEV_TX_OK ; +} + +static const struct net_device_ops pch_can_netdev_ops = { + .ndo_open = pch_open, + .ndo_stop = pch_close, + .ndo_start_xmit = pch_xmit, +}; + +static void __devexit pch_can_remove(struct pci_dev *pdev) +{ +struct net_device *ndev =pci_get_drvdata (pdev); +struct pch_can_priv *priv =netdev_priv (ndev); + +unregister_candev (priv->ndev); +free_candev (priv->ndev); +pci_iounmap (pdev, priv->base); +pci_release_regions (pdev); +pci_disable_device (pdev); +pci_set_drvdata (pdev,NULL ); +pch_can_reset (priv); +} + +#ifdef CONFIG_PM +static int pch_can_suspend (struct pci_dev *pdev,pm_message_t state) +{ +int i;/* Counter variable. */ +int retval;/* Return value. */ +u32 buf_stat;/* Variable for reading the transmit buffer status. */ +u32 counter =0xFFFFFF ; + +struct net_device *dev =pci_get_drvdata (pdev); +struct pch_can_priv *priv =netdev_priv (dev); + +/* Stop the CAN controller */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Indicate that we are aboutto/in suspend */ + priv->can.state =CAN_STATE_SLEEPING ;
drivers/net/can/pch_can.c line 1235
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +/* Waiting for all transmission to complete. */ +while (counter) { + buf_stat =pch_can_get_buffer_status (priv); +if (!buf_stat) +break ; + counter--; + } +if (!counter) +dev_err (&pdev->dev,"%s -> Transmission time out.\n" , __func__);
Timeout without defined delay!
Quoted 90 lines
drivers/net/can/pch_can.c new file +90
-0+/* Save interrupt configuration and then disable them */ +pch_can_get_int_enables (priv, &(priv->int_enables)); +pch_can_set_int_enables (priv,PCH_CAN_DISABLE ); + +/* Save Tx buffer enable state */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_get_tx_enable (priv, (i +1 ), + &(priv->tx_enable[i])); + } + +/* Disable all Transmit buffers */ +pch_can_tx_disable_all (priv); + +/* Save Rx buffer enable state */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) { +pch_can_get_rx_enable (priv, (i +1 ), + &(priv->rx_enable[i])); +pch_can_get_rx_buffer_link (priv, (i +1 ), + &(priv->rx_link[i])); + } + } + +/* Disable all Receive buffers */ +pch_can_rx_disable_all (priv); + retval =pci_save_state (pdev); +if (retval) { +dev_err (&pdev->dev,"pci_save_state failed.\n" ); + }else { +pci_enable_wake (pdev, PCI_D3hot,0 ); +pci_disable_device (pdev); +pci_set_power_state (pdev,pci_choose_state (pdev, state)); + } + +return retval; +} + +static int pch_can_resume (struct pci_dev *pdev) +{ +int i;/* Counter variable. */ +int retval;/* Return variable. */ +struct net_device *dev =pci_get_drvdata (pdev); +struct pch_can_priv *priv =netdev_priv (dev); + +pci_set_power_state (pdev,PCI_D0 ); +pci_restore_state (pdev); + retval =pci_enable_device (pdev); +if (retval) { +dev_err (&pdev->dev,"pci_enable_device failed.\n" ); +return retval; + } + +pci_enable_wake (pdev, PCI_D3hot,0 ); + + priv->can.state =CAN_STATE_ERROR_ACTIVE ; + +/* Disabling all interrupts. */ +pch_can_set_int_enables (priv,PCH_CAN_DISABLE ); + +/* Setting the CAN device in Stop Mode. */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Configuring the transmit and receive buffers. */ +pch_can_config_rx_tx_buffers (priv); + +/* Restore the CAN state */ +pch_set_bittiming (dev); + +/* Listen/Active */ +pch_can_set_optmode (priv); + +/* Enabling the transmit buffer. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) { +pch_can_set_tx_enable (priv, i +1 , + priv->tx_enable[i]); + } + } + +/* Configuring the receive buffer and enabling them. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) { +/* Restore buffer link */ +pch_can_set_rx_buffer_link (priv, i +1 , + priv->rx_link[i]); + +/* Restore buffer enables */ +pch_can_set_rx_enable (priv, i +1 , priv->rx_enable[i]); + }
drivers/net/can/pch_can.c line 1336
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + +/* Enable CAN Interrupts */ +pch_can_set_int_custom (priv); + +/* Restore Run Mode */ +pch_can_set_run_mode (priv,PCH_CAN_RUN ); + +return retval; +}
Are the suspend and resume functions tested?
drivers/net/can/pch_can.c line 1340
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+#else +#define pch_can_suspend NULL +#define pch_can_resume NULL +#endif
Add empty line here
Quoted 33 lines
drivers/net/can/pch_can.c new file +33
-0+static int __devinit pch_can_probe (struct pci_dev *pdev, +const struct pci_device_id *id) +{ +struct net_device *ndev; +struct pch_can_priv *priv; +int rc; +int index; +void __iomem *addr; + + rc =pci_enable_device (pdev); +if (rc) { +dev_err (&pdev->dev,"Failed pci_enable_device %d\n" , rc); + goto probe_exit_endev; + } + + rc =pci_request_regions (pdev,KBUILD_MODNAME ); +if (rc) { +dev_err (&pdev->dev,"Failed pci_request_regions %d\n" , rc); + goto probe_exit_pcireq; + } + + addr =pci_iomap (pdev,1 ,0 ); +if (!addr) { + rc = -EIO ; +dev_err (&pdev->dev,"Failed pci_iomap\n" ); + goto probe_exit_ipmap; + } + + ndev =alloc_candev (sizeof (struct pch_can_priv ),1 ); +if (!ndev) { + rc = -ENOMEM ; +dev_err (&pdev->dev,"Failed alloc_candev\n" ); + goto probe_exit_alloc_candev;
drivers/net/can/pch_can.c line 1383
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + + priv =netdev_priv (ndev); + priv->ndev = ndev; + priv->base = addr; + priv->regs = addr; + priv->dev = pdev; + priv->can.bittiming_const = &pch_can_bittiming_const; + priv->can.do_set_mode = pch_can_do_set_mode; + priv->can.do_get_state = pch_can_get_state;
Not needed! See above.
Could you please also implement do_get_berr_counter().
Quoted 49 lines
drivers/net/can/pch_can.c new file +49
-0+ priv->can.ctrlmode_supported =CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_LOOPBACK ; + ndev->irq = pdev->irq; + ndev->flags |=IFF_ECHO ; + +pci_set_drvdata (pdev, ndev); +SET_NETDEV_DEV (ndev, &pdev->dev); + ndev->netdev_ops = &pch_can_netdev_ops; + + priv->can.clock.freq =PCH_CAN_CLK *1000 ;/* Hz to KHz) */ +for (index =0 ; index <PCH_RX_OBJ_NUM ;) + priv->msg_obj[index++] =MSG_OBJ_RX ; + +for (index = index; index <PCH_OBJ_NUM ;) + priv->msg_obj[index++] =MSG_OBJ_TX ; + + rc =register_candev (ndev); +if (rc) { +dev_err (&pdev->dev,"Failed register_candev %d\n" , rc); + goto probe_exit_reg_candev; + } + +return 0 ; + +probe_exit_reg_candev: +free_candev (ndev); +probe_exit_alloc_candev: +pci_iounmap (pdev, addr); +probe_exit_ipmap: +pci_release_regions (pdev); +probe_exit_pcireq: +pci_disable_device (pdev); +probe_exit_endev: +return rc; +} + +static struct pci_driver pch_can_pcidev = { + .name =KBUILD_MODNAME , + .id_table = pch_can_pcidev_id, + .probe = pch_can_probe, + .remove =__devexit_p (pch_can_remove), + .suspend = pch_can_suspend, + .resume = pch_can_resume, +}; + +static int __init pch_can_pci_init (void ) +{ +return pci_register_driver (&pch_can_pcidev); +}
drivers/net/can/pch_can.c line 1442
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+module_init (pch_can_pci_init); + +static void __exit pch_can_pci_exit(void) +{ +pci_unregister_driver (&pch_can_pcidev); +} +module_exit (pch_can_pci_exit); + +MODULE_DESCRIPTION ("Controller Area Network Driver" ); +MODULE_LICENSE ("GPL" );
GPL v2 ?
drivers/net/can/pch_can.c line 1444
Quoted 2 lines
drivers/net/can/pch_can.c new file +2
-0+MODULE_VERSION ("0.94" ); +MODULE_DEVICE_TABLE (pci, pch_can_pcidev_id);
Please add it below the declaration of pch_can_pcidev_id.
In this driver you are using just *one* RX object. This means that the CPU must handle new messages as quickly as possible otherwise message losses will happen, right?. For sure, this will not make user's happy. Any chance to use more RX objects in FIFO mode?
Thanks,
Wolfgang.
David Miller
maintainerFrom: Wolfgang Grandegger <[email protected]>
Date: Thu, 30 Sep 2010 11:10:55 +0200
Quoted 8 lines · Masayuki Ohtak
drivers/net/can/pch_can.c line 194
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+}; + +static struct can_bittiming_const pch_can_bittiming_const = { + .name =KBUILD_MODNAME ,Not sure what
KBUILD_MODNAMEis. Should be "pch_can", the name of the driver.
That's what KBUILD_MODNAME will be defined to when this gets compiled :-)
Hi Wolfgang Grandegger,
Thank you for your comments.
We will modify and re-post ASAP.
I have a comment about below.
Quoted 4 lines
In this driver you are using just *one* RX object. This means that the CPU must handle new messages as quickly as possible otherwise message losses will happen, right?. For sure, this will not make user's happy. Any chance to use more RX objects in FIFO mode?
In case implementing with FIFO mode,
received packets may be our of order.
Because our CAN register access is slow.
I am confirming our CAN HW spec and the possibility of our-of-order.
Thanks, Ohtake(OKISemi)
---- Original Message -----
From: "Wolfgang Grandegger" <[email protected]>
To: "Masayuki Ohtak" <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>; "Wolfram Sang" <[email protected]>; "Christian Pellegrin" <[email protected]>; "Barry Song" <[email protected]>; "Samuel Ortiz" <sameo-VuQAYsv1563Yd54FQh9/[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; "Tomoya MORINAGA" <morinaga526-ECg8zkTtlr0C6LszWs/[email protected]>; <[email protected]>
Sent: Thursday, September 30, 2010 6:10 PM
Subject: Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
Quoted 1,713 lines
Hi Ohtake,
here comes my review, sorry for delay.
Quoted 4 lines · Masayuki Ohtak
Hi Wolfgang and Marc,
We have modified a pretty amount of our driver based on other accepted Socket CAN driver. Additionally, We have reduced the number of lines 3601 to 1444.
Much better, but I believe it could be reduced even further.
Quoted 44 lines
Please check below.
Thanks, Ohtake(OKISemi)
---
CAN driver of Topcliff PCHTopcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has CAN I/F. This driver enables CAN function.
--- drivers/net/can/Kconfig | 8 + drivers/net/can/Makefile | 1 + drivers/net/can/pch_can.c | 1444 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1453 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pch_can.cdrivers/net/can/Kconfig +8
-0@@ -73,6 +73,14 @@ config CAN_JANZ_ICAN3 This driver can also be built as a module. If so, the module will be called janz-ican3.ko. +config PCH_CAN + tristate "PCH CAN" + depends on CAN_DEV + ---help--- + This driver is for PCH CAN of Topcliff which is an IOH for x86 + embedded processor. + This driver can access CAN bus. + source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"drivers/net/can/Makefile +0
-0drivers/net/can/Makefile obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o · line 19
Quoted 4 lines
drivers/net/can/Makefile +1
-0@@ -16,5 +16,6 @@ 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 +obj-$(CONFIG_PCH_CAN) += pch_can.oPlease provide patches against David Millers "net-next-2.6" GIT tree and use the prefix "can: " in your subject next time. See
http://svn.berlios.de/wsvn/socketcan/trunk/README.submitting-patches
for further information.Quoted 32 lines
drivers/net/can/Makefile +0
-0ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUGdrivers/net/can/pch_can.c new file +25
-0@@ -0,0 +1,1444 @@ +/* +* Copyright (C) 1999 - 2010 Intel Corporation. +* Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; version 2 of the License. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA. +*/ + +#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/sched.h> +#include <linux/pci.h> +#include <linux/init.h> drivers/net/can/pch_can.c line 35
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/errno.h> +#include <linux/netdevice.h> +#include <linux/skbuff.h> +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> + +#define MAX_BITRATE 0x3e8Dead code? At least it's not used anywhere.
Quoted 48 lines
drivers/net/can/pch_can.c new file +47
-0+ +#define MAX_MSG_OBJ 32 +#define MSG_OBJ_RX 0/* The receive message object flag. */ +#define MSG_OBJ_TX 1/* The transmit message object flag. */ + +#define ENABLE 1/* The enable flag */ +#define DISABLE 0/* The disable flag */ +#define CAN_CTRL_INIT 0x0001/* The INIT bit of CANCONT register. */ +#define CAN_CTRL_IE 0x0002/* The IE bit of CAN control register */ +#define CAN_CTRL_IE_SIE_EIE 0x000e +#define CAN_CTRL_CCE 0x0040 +#define CAN_CTRL_OPT 0x0080/* The OPT bit of CANCONT register. */ +#define CAN_OPT_SILENT 0x0008/* The Silent bit of CANOPT reg. */ +#define CAN_OPT_LBACK 0x0010/* The LoopBack bit of CANOPT reg. */ +#define CAN_CMASK_RX_TX_SET 0x00f3 +#define CAN_CMASK_RX_TX_GET 0x0073 +#define CAN_CMASK_ALL 0xff +#define CAN_CMASK_RDWR 0x80 +#define CAN_CMASK_ARB 0x20 +#define CAN_CMASK_CTRL 0x10 +#define CAN_CMASK_MASK 0x40 + +#define CAN_IF_MCONT_NEWDAT 0x8000 +#define CAN_IF_MCONT_INTPND 0x2000 +#define CAN_IF_MCONT_UMASK 0x1000 +#define CAN_IF_MCONT_TXIE 0x0800 +#define CAN_IF_MCONT_RXIE 0x0400 +#define CAN_IF_MCONT_RMTEN 0x0200 +#define CAN_IF_MCONT_TXRQXT 0x0100 +#define CAN_IF_MCONT_EOB 0x0080 +#define CAN_IF_MCONT_MSGLOST 0x4000 +#define CAN_MASK2_MDIR_MXTD 0xc000 +#define CAN_ID2_DIR 0x2000 +#define CAN_ID_MSGVAL 0x8000 + +#define CAN_STATUS_INT 0x8000 +#define CAN_IF_CREQ_BUSY 0x8000 +#define CAN_ID2_XTD 0x4000 + +#define CAN_REC 0x00007f00 +#define CAN_TEC 0x000000ff + +#define PCH_RX_OK 0x00000010 +#define PCH_TX_OK 0x00000008 +#define PCH_BUS_OFF 0x00000080 +#define PCH_EWARN 0x00000040 +#define PCH_EPASSIV 0x00000020drivers/net/can/pch_can.c line 92
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+#define PCH_LEC0 0x00000001 +#define PCH_LEC1 0x00000002 +#define PCH_LEC2 0x00000004 +#define PCH_LEC_ALL (PCH_LEC0 | PCH_LEC1 | PCH_LEC2) +#define PCH_STUF_ERR PCH_LEC0 +#define PCH_FORM_ERR PCH_LEC1 +#define PCH_ACK_ERR (PCH_LEC0 | PCH_LEC1) +#define PCH_BIT1_ERR PCH_LEC2 +#define PCH_BIT0_ERR (PCH_LEC0 | PCH_LEC2) +#define PCH_CRC_ERR (PCH_LEC1 | PCH_LEC2)enum {
PCH_LEC_STUF_ERR= 0,PCH_LEC_FORM_ERR,PCH_LEC_ACK_ERR,
...PCH_LEC_ALL
};Seems more appropriate. More comments below.
Quoted 13 lines
drivers/net/can/pch_can.c new file +13
-0+ +/* bit position of certain controller bits. */ +#define BIT_BITT_BRP 0 +#define BIT_BITT_SJW 6 +#define BIT_BITT_TSEG1 8 +#define BIT_BITT_TSEG2 12 +#define BIT_IF1_MCONT_RXIE 10 +#define BIT_IF2_MCONT_TXIE 11 +#define BIT_BRPE_BRPE 6 +#define BIT_ES_TXERRCNT 0 +#define BIT_ES_RXERRCNT 8 +#define MSK_BITT_BRP 0x3f +#define MSK_BITT_SJW 0xc0drivers/net/can/pch_can.c line 115
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+#define MSK_BITT_TSEG1 0xf00 +#define MSK_BITT_TSEG2 0x7000 +#define MSK_BRPE_BRPE 0x3c0 +#define MSK_BRPE_GET 0x0f +#define MSK_CTRL_IE_SIE_EIE 0x07 +#define MSK_MCONT_TXIE 0x08 +#define MSK_MCONT_RXIE 0x10 +#define PCH_CAN_NO_TX_BUFF 1 +#define PCI_DEVICE_ID_INTEL_PCH1_CAN 0x8818 +#define COUNTER_LIMIT 0xFFFFKeep alignment?
Quoted 1 line
+#define
PCH_CAN_CLK50000 /* 50MHz */Please specify it in Hz already here.
drivers/net/can/pch_can.c line 121
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+ +/* Total 32 OBJs */ +#define PCH_RX_OBJ_NUM 1 +#define PCH_TX_OBJ_NUM 1 +#define PCH_OBJ_NUM (PCH_TX_OBJ_NUM + PCH_RX_OBJ_NUM)Please explain biefly what message object are use for what purpose. Either here or in the initialization code.
drivers/net/can/pch_can.c line 131
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +#define PCH_CAN_ACTIVE 0 +#define PCH_CAN_LISTEN 1 +#define PCH_CAN_STOP 0 +#define PCH_CAN_RUN 1 + +#define PCH_CAN_ENABLE 0 +#define PCH_CAN_DISABLE 1 +#define PCH_CAN_ALL 2 +#define PCH_CAN_NONE 3The above are used in switch case and should therefore be anonymous enums. I suggested to remove them because I'm not a real friend of the helper functions which are just called *once*.
Quoted 33 lines
drivers/net/can/pch_can.c new file +33
-0+ +struct pch_can_regs { +u32 cont; +u32 stat; +u32 errc; +u32 bitt; +u32 intr; +u32 opt; +u32 brpe; +u32 reserve1; +u32 if1_creq; +u32 if1_cmask; +u32 if1_mask1; +u32 if1_mask2; +u32 if1_id1; +u32 if1_id2; +u32 if1_mcont; +u32 if1_dataa1; +u32 if1_dataa2; +u32 if1_datab1; +u32 if1_datab2; +u32 reserve2; +u32 reserve3[12 ]; +u32 if2_creq; +u32 if2_cmask; +u32 if2_mask1; +u32 if2_mask2; +u32 if2_id1; +u32 if2_id2; +u32 if2_mcont; +u32 if2_dataa1; +u32 if2_dataa2; +u32 if2_datab1;drivers/net/can/pch_can.c line 174
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+u32 if2_datab2; +u32 reserve4; +u32 reserve5[20 ]; +u32 treq1; +u32 treq2; +u32 reserve6[2 ]; +u32 reserve7[56 ]; +u32 reserve8[3 ]; +u32 srst; +};Nice.
Quoted 7 lines
+struct
pch_can_priv{
+ structcan_privcan;
+ void__iomem*base;
+ unsigned intcan_num;
+ structpci_dev*dev;
+ unsigned int tx_enable[MAX_MSG_OBJ];
+ unsigned int rx_enable[MAX_MSG_OBJ];Quoted 8 lines
+ unsigned int rx_link[MAX_MSG_OBJ];
+ unsigned intint_enables;
+ unsigned intint_stat;
+ unsigned intbus_off_interrupt;
+ structnet_device*ndev;
+spinlock_tmsgif_reg_lock; /* Message Interface Registers Access Lock*/
+ unsigned int msg_obj[MAX_MSG_OBJ];
+ structpch_can_regs*regs;Please add
__iomem. Do you need both, regs *and* base?drivers/net/can/pch_can.c line 194
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+}; + +static struct can_bittiming_const pch_can_bittiming_const = { + .name =KBUILD_MODNAME ,Not sure what
KBUILD_MODNAMEis. Should be "pch_can", the name of the driver.Quoted 8 lines
+ .tseg1_min = 1,
+ .tseg1_max = 16,
+ .tseg2_min = 1,
+ .tseg2_max = 8,
+ .sjw_max = 4,
+ .brp_min = 1,
+ .brp_max = 1024, /* 6bit + extended 4bit */
+ .brp_inc = 1,drivers/net/can/pch_can.c line 208
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+}; + +static const struct pci_device_id pch_can_pcidev_id[]__devinitdata = { + {PCI_DEVICE (PCI_VENDOR_ID_INTEL ,PCI_DEVICE_ID_INTEL_PCH1_CAN )}, + {} +};Please use
DEFINE_PCI_DEVICE_TABLE.drivers/net/can/pch_can.c line 212
Quoted 3 lines
drivers/net/can/pch_can.c new file +3
-0+static inline void pch_can_bit_set (u32 *addr,u32 mask) +{ +iowrite32 ((ioread32 (addr) | mask), addr);Outer brackets not needed!
drivers/net/can/pch_can.c line 217
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+} + +static inline void pch_can_bit_clear (u32 *addr,u32 mask) +{ +iowrite32 ((ioread32 (addr) & ~(mask)), addr);Ditto.
Quoted 12 lines
drivers/net/can/pch_can.c new file +12
-0+} + +static void pch_can_set_run_mode (struct pch_can_priv *priv,u32 mode) +{ +switch (mode) { +case PCH_CAN_RUN : +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_INIT ); +break ; + +case PCH_CAN_STOP : +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_INIT ); +break ;drivers/net/can/pch_can.c line 239
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +default : +dev_err (&priv->ndev->dev,"%s -> Invalid Mode.\n" , __func__); +break ; + } +} + +static void pch_can_get_run_mode (struct pch_can_priv *priv,u32 *mode) +{ +u32 reg_val =ioread32 (&(priv->regs)->cont);I don't think you need the brackets around "
priv->regs". Therefore I suggest s/&(priv->regs)/&priv->regs/ for the whole file.drivers/net/can/pch_can.c line 245
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ +if (reg_val &CAN_CTRL_INIT ) + *mode =PCH_CAN_STOP ; +else + *mode =PCH_CAN_RUN ; +}These are the helper functions I complained about above. And
reg_valis not really needed.Quoted 21 lines
drivers/net/can/pch_can.c new file +21
-0+static void pch_can_set_optmode (struct pch_can_priv *priv) +{ +u32 reg_val =ioread32 (&(priv->regs)->opt); + +if (priv->can.ctrlmode &CAN_CTRLMODE_LISTENONLY ) + reg_val |=CAN_OPT_SILENT ; + +if (priv->can.ctrlmode &CAN_CTRLMODE_LOOPBACK ) + reg_val |=CAN_OPT_LBACK ; + +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_OPT ); +iowrite32 (reg_val, &(priv->regs)->opt); +} + +static void pch_can_set_int_custom (struct pch_can_priv *priv) +{ +/* Clearing the IE, SIE and EIE bits of Can control register. */ +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_IE_SIE_EIE ); + +/* Appropriately setting them. */ + pch_can_bit_set(&(priv->regs)->cont,drivers/net/can/pch_can.c line 277
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ ((priv->int_enables & MSK_CTRL_IE_SIE_EIE) << 1)); +} + +/* This function retrieves interrupt enabled for the CAN device. */ +static void pch_can_get_int_enables (struct pch_can_priv *priv,u32 *enables) +{ +u32 reg_ctrl_val =ioread32 (&(priv->regs)->cont); + +/* Obtaining the status of IE, SIE and EIE interrupt bits. */ + *enables = ((reg_ctrl_val &CAN_CTRL_IE_SIE_EIE ) >>1 );Do you really need an extra variable?
Quoted 32 lines
drivers/net/can/pch_can.c new file +32
-0+} + +static void pch_can_set_int_enables (struct pch_can_priv *priv,u32 interrupt_no) +{ +switch (interrupt_no) { +case PCH_CAN_ENABLE : +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_IE ); +break ; + +case PCH_CAN_DISABLE : +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_IE ); +break ; + +case PCH_CAN_ALL : +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_IE_SIE_EIE ); +break ; + +case PCH_CAN_NONE : +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_IE_SIE_EIE ); +break ; + +default : +dev_err (&priv->ndev->dev,"Invalid interrupt number.\n" ); +break ; + } +} + +static void pch_can_check_if1_busy (struct pch_can_priv *priv,u32 num) +{ +u32 counter =COUNTER_LIMIT ; +u32 if1_creq; +drivers/net/can/pch_can.c line 319
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+iowrite32 (num, &(priv->regs)->if1_creq); +while (counter) { + if1_creq = (ioread32 (&(priv->regs)->if1_creq)) & +CAN_IF_CREQ_BUSY ; +if (!if1_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF1 BUSY Flag is set forever.\n" );Please use a defined delay for the above timeout. How long does it usually take the bit to toggle? A small delay, e.g. udelay(1) could be fine. This function is called in the time critical path!
Quoted 8 lines
drivers/net/can/pch_can.c new file +8
-0+} + +static void pch_can_check_if2_busy (struct pch_can_priv *priv,u32 num) +{ +u32 counter =COUNTER_LIMIT ; +u32 if2_creq; + +iowrite32 (num, &(priv->regs)->if2_creq);drivers/net/can/pch_can.c line 337
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+while (counter) { + if2_creq = (ioread32 (&(priv->regs)->if2_creq)) & +CAN_IF_CREQ_BUSY ; +if (!if2_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF2 BUSY Flag is set forever.\n" ); +}Duplicated code!
drivers/net/can/pch_can.c line 345
Quoted 7 lines
drivers/net/can/pch_can.c new file +7
-0+static void pch_can_set_rx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 set) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +/*Reading the receive buffer data from RAM to Interface1 registers */ Space after /* ?
Quoted 149 lines
+
iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if1_cmask);
+pch_can_check_if1_busy(priv, buff_num); /* Read from MsgRAN */drivers/net/can/pch_can.c new file +147
-0+ +/* Setting the IF1MASK1 register to access MsgVal and RxIE bits */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if1_cmask)); + +if (set ==ENABLE ) { +/* Setting the MsgVal and RxIE bits */ +pch_can_bit_set (&(priv->regs)->if1_mcont,CAN_IF_MCONT_RXIE ); +pch_can_bit_set (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + + }else if (set ==DISABLE ) { +/* Resetting the MsgVal and RxIE bits */ +pch_can_bit_clear (&(priv->regs)->if1_mcont,CAN_IF_MCONT_RXIE ); +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + } + +pch_can_check_if1_busy (priv, buff_num);/* Write to MsgRAM */ +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_rx_enable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as receivers. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) +pch_can_set_rx_enable (priv, i +1 ,ENABLE ); + } +} + +static void pch_can_rx_disable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as receivers. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) +pch_can_set_rx_enable (priv, (i +1 ),DISABLE ); + } +} + +static void pch_can_set_tx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 set) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +/* Reading the Msg buffer from Message RAM to Interface2 registers. */ +iowrite32 (CAN_CMASK_RX_TX_GET , (&(priv->regs)->if1_cmask)); +pch_can_check_if1_busy (priv, buff_num); + +/* Setting the IF2CMASK register for accessing the +MsgVal and TxIE bits */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if1_cmask)); + +if (set ==ENABLE ) { +/* Setting the MsgVal and TxIE bits */ +pch_can_bit_set (&(priv->regs)->if1_mcont,CAN_IF_MCONT_TXIE ); +pch_can_bit_set (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + }else if (set ==DISABLE ) { +/* Resetting the MsgVal and TxIE bits. */ +pch_can_bit_clear (&(priv->regs)->if1_mcont,CAN_IF_MCONT_TXIE ); +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID_MSGVAL ); + } + +pch_can_check_if1_busy (priv, buff_num);/* Write to MsgRAM */ +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_tx_enable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as transmit object. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_set_tx_enable (priv, (i +1 ),ENABLE ); + } +} + +static void pch_can_tx_disable_all (struct pch_can_priv *priv) +{ +u32 i; + +/* Traversing to obtain the object configured as transmit object. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_set_tx_enable (priv, (i +1 ),DISABLE ); + } +} + +static void pch_can_get_rx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 *enable) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , (&(priv->regs)->if1_cmask)); +pch_can_check_if1_busy (priv, buff_num); + +if (((ioread32 (&(priv->regs)->if1_id2)) &CAN_ID_MSGVAL ) && + ((ioread32 (&(priv->regs)->if1_mcont)) & +CAN_IF_MCONT_RXIE )) + *enable =ENABLE ; +else + *enable =DISABLE ; +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_get_tx_enable (struct pch_can_priv *priv,u32 buff_num, +u32 *enable) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buff_num); + +if (((ioread32 (&(priv->regs)->if1_id2)) &CAN_ID_MSGVAL ) && + ((ioread32 (&(priv->regs)->if1_mcont)) & +CAN_IF_MCONT_TXIE )) { + *enable =ENABLE ; + }else { + *enable =DISABLE ; + } +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static int pch_can_int_pending (struct pch_can_priv *priv) +{ +return ioread32 (&(priv->regs)->intr) &0xffff ; +} + +static void pch_can_set_rx_buffer_link (struct pch_can_priv *priv, +u32 buffer_num,u32 set) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buffer_num); +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_CTRL ), &(priv->regs)->if1_cmask); +if (set ==ENABLE ) +pch_can_bit_clear (&(priv->regs)->if1_mcont,CAN_IF_MCONT_EOB ); +else drivers/net/can/pch_can.c line 504
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_bit_set (&(priv->regs)->if1_mcont,CAN_IF_MCONT_EOB ); + +pch_can_check_if1_busy (priv, buffer_num); +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_get_rx_buffer_link (struct pch_can_priv *priv, +u32 buffer_num,u32 *link) +{ +u32 reg_val;Really needed?
Quoted 9 lines
+ unsigned long flags;
drivers/net/can/pch_can.c new file +8
-0+ +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, buffer_num); + + reg_val =ioread32 (&(priv->regs)->if1_mcont); +if (reg_val &CAN_IF_MCONT_EOB ) + *link =DISABLE ;drivers/net/can/pch_can.c line 523
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+else + *link =ENABLE ; +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_clear_buffers (struct pch_can_priv *priv) +{ +u32 i; +u32 rx_buff_num; +u32 tx_buff_num;Really needed?
Quoted 68 lines
drivers/net/can/pch_can.c new file +68
-0+ +iowrite32 (CAN_CMASK_RX_TX_SET , &(priv->regs)->if1_cmask); +iowrite32 (CAN_CMASK_RX_TX_SET , &(priv->regs)->if2_cmask); +iowrite32 (0xffff , &(priv->regs)->if1_mask1); +iowrite32 (0xffff , &(priv->regs)->if1_mask2); +iowrite32 (0xffff , &(priv->regs)->if2_mask1); +iowrite32 (0xffff , &(priv->regs)->if2_mask2); + +iowrite32 (0x0 , &(priv->regs)->if1_id1); +iowrite32 (0x0 , &(priv->regs)->if1_id2); +iowrite32 (0x0 , &(priv->regs)->if2_id1); +iowrite32 (0x0 , &(priv->regs)->if2_id2); +iowrite32 (0x0 , &(priv->regs)->if1_mcont); +iowrite32 (0x0 , &(priv->regs)->if2_mcont); +iowrite32 (0x0 , &(priv->regs)->if1_dataa1); +iowrite32 (0x0 , &(priv->regs)->if1_dataa2); +iowrite32 (0x0 , &(priv->regs)->if1_datab1); +iowrite32 (0x0 , &(priv->regs)->if1_datab2); +iowrite32 (0x0 , &(priv->regs)->if2_dataa1); +iowrite32 (0x0 , &(priv->regs)->if2_dataa2); +iowrite32 (0x0 , &(priv->regs)->if2_datab1); +iowrite32 (0x0 , &(priv->regs)->if2_datab2); + +for (i =1 ; i <= (MAX_MSG_OBJ /2 ); i++) { + rx_buff_num =2 * i; + tx_buff_num = (2 * i) -1 ; + +iowrite32 (rx_buff_num, &(priv->regs)->if1_creq); +iowrite32 (tx_buff_num, &(priv->regs)->if2_creq); + +mdelay (10 ); + } +} + +static void pch_can_config_rx_tx_buffers (struct pch_can_priv *priv) +{ +u32 i; +unsigned long flags; + +spin_lock_irqsave (&priv->msgif_reg_lock, flags); +/* For accssing MsgVal, ID and EOB bit */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if1_cmask)); +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_ARB |CAN_CMASK_CTRL ), + (&(priv->regs)->if2_cmask)); +iowrite32 (0x0 , (&(priv->regs)->if1_id1)); +iowrite32 (0x0 , (&(priv->regs)->if1_id2)); + +/* Resetting DIR bit for reception */ +iowrite32 (0x0 , (&(priv->regs)->if2_id1)); + +/* Setting DIR bit for transmission */ +iowrite32 ((CAN_ID2_DIR | (0x7ff <<2 )), + (&(priv->regs)->if2_id2)); + +/* Setting EOB bit for receiver */ +iowrite32 (CAN_IF_MCONT_EOB , &(priv->regs)->if1_mcont); + +/* Setting EOB bit for transmitter */ +iowrite32 (CAN_IF_MCONT_EOB , (&(priv->regs)->if2_mcont)); + +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) +pch_can_check_if1_busy (priv, i +1 ); +else if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_check_if2_busy (priv, i +1 ); +else +dev_err (&priv->ndev->dev,"Invalid OBJ\n" );drivers/net/can/pch_can.c line 601
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) { +iowrite32 (CAN_CMASK_RX_TX_GET , + &(priv->regs)->if1_cmask); +pch_can_check_if1_busy (priv, i+1 ); + +pch_can_bit_clear (&(priv->regs)->if1_id2,0x1fff ); +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID2_XTD );Could'nt it be set just by one call?
Quoted 17 lines
+
iowrite32(0, (&(priv->regs)->if1_id1));
+pch_can_bit_set(&(priv->regs)->if1_id2, 0);
+ pch_can_bit_set(&(priv->regs)->if1_mcont,
+CAN_IF_MCONT_UMASK);
+ pch_can_bit_set(&(priv->regs)->if2_mcont,
+CAN_IF_MCONT_UMASK);drivers/net/can/pch_can.c new file +11
-0+ +iowrite32 (0 , &(priv->regs)->if1_mask1); +pch_can_bit_clear (&(priv->regs)->if1_mask2,0x1fff ); +pch_can_bit_clear (&(priv->regs)->if1_mask2, +CAN_MASK2_MDIR_MXTD ); + +iowrite32 (0 , &(priv->regs)->if2_mask1); +pch_can_bit_clear (&(priv->regs)->if2_mask2,0x1fff ); + +/* Setting CMASK for writing */ + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_MASK |drivers/net/can/pch_can.c line 628
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ CAN_CMASK_ARB | CAN_CMASK_CTRL), + (&(priv->regs)->if1_cmask)); + +pch_can_check_if1_busy (priv, i+1 ); + } + } +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); +} + +static void pch_can_open (struct pch_can_priv *priv)Probably
pch_can_initis the better name.Quoted 11 lines
drivers/net/can/pch_can.c new file +11
-0+{ +/* Stopping the Can device. */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Clearing all the message object buffers. */ +pch_can_clear_buffers (priv); + +/* Configuring the respective message object as either rx/tx object. */ +pch_can_config_rx_tx_buffers (priv); + +/* Enabling all receive objects. */ drivers/net/can/pch_can.c line 649
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_rx_enable_all (priv); + +/* Enabling all transmit objects. */ +pch_can_tx_enable_all (priv); + +/* Enabling the interrupts. */ +pch_can_set_int_enables (priv,PCH_CAN_ALL ); + +/* Setting the CAN to run mode. */ +pch_can_set_run_mode (priv,PCH_CAN_RUN );Hm, you start the controller here... more later.
Quoted 35 lines
drivers/net/can/pch_can.c new file +35
-0+} + +static void pch_can_release (struct pch_can_priv *priv) +{ +/* Stooping the CAN device. */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Disabling the interrupts. */ +pch_can_set_int_enables (priv,PCH_CAN_NONE ); + +/* Disabling all the receive object. */ +pch_can_rx_disable_all (priv); + +/* Disabling all the transmit object. */ +pch_can_tx_disable_all (priv); +} + +/* This function clears interrupt(s) from the CAN device. */ +static void pch_can_int_clr (struct pch_can_priv *priv,u32 mask) +{ +if (mask ==CAN_STATUS_INT ) { +ioread32 (&(priv->regs)->stat); +return ; + } + +/* Clear interrupt for transmit object */ +if (priv->msg_obj[mask -1 ] ==MSG_OBJ_TX ) { +/* Setting CMASK for clearing interrupts for +frame transmission. */ +iowrite32 ((CAN_CMASK_RDWR |CAN_CMASK_CTRL |CAN_CMASK_ARB ), + (&(priv->regs)->if2_cmask)); + +/* Resetting the ID registers. */ +pch_can_bit_set (&(priv->regs)->if2_id2, + (CAN_ID2_DIR | (0x7ff <<2 )));drivers/net/can/pch_can.c line 694
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+iowrite32 (0x0 , (&(priv->regs)->if2_id1)); + +/* Claring NewDat, TxRqst & IntPnd */ +pch_can_bit_clear (&(priv->regs)->if2_mcont, + (CAN_IF_MCONT_NEWDAT |CAN_IF_MCONT_INTPND | +CAN_IF_MCONT_TXRQXT )); +pch_can_check_if2_busy (priv, mask); + } +/* Clear interrupt for receive object */ +else if (priv->msg_obj[mask -1 ] ==MSG_OBJ_RX ) {Should be "} else if ..."
Quoted 9 lines
+ /* Setting CMASK for clearing the reception interrupts. */
+ iowrite32((CAN_CMASK_RDWR |CAN_CMASK_CTRL|CAN_CMASK_ARB),drivers/net/can/pch_can.c new file +7
-0+ (&(priv->regs)->if2_cmask)); + +/* Clearing the Dir bit. */ +pch_can_bit_clear (&(priv->regs)->if2_id2,CAN_ID2_DIR ); + +/* Clearing NewDat & IntPnd */ + pch_can_bit_clear(&(priv->regs)->if2_mcont,drivers/net/can/pch_can.c line 713
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ (CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND)); + +pch_can_check_if2_busy (priv, mask); + } +} + +static int pch_can_get_buffer_status (struct pch_can_priv *priv) +{ +u32 reg_treq1; +u32 reg_treq2;Really needed?
Quoted 30 lines
drivers/net/can/pch_can.c new file +30
-0+ +/* Reading the transmission request registers. */ + reg_treq1 = (ioread32 (&(priv->regs)->treq1) &0xffff ); + reg_treq2 = ((ioread32 (&(priv->regs)->treq2) &0xffff ) <<16 ); + +return reg_treq1 | reg_treq2; +} + +static void pch_can_reset (struct pch_can_priv *priv) +{ +/* write to sw reset register */ +iowrite32 (1 , (&(priv->regs)->srst)); +iowrite32 (0 , (&(priv->regs)->srst)); +} + +static void pch_can_msg_obj (struct net_device *ndev,u32 status) +{ +struct pch_can_priv *priv =netdev_priv (ndev); +u32 reg; +struct sk_buff *skb; +struct can_frame *cf; +canid_t id; +u32 ide; +u32 rtr; +int i, j; +struct net_device_stats *stats = &(priv->ndev->stats); + +/* Reading the messsage object from the Message RAM */ +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if2_cmask); +pch_can_check_if2_busy (priv, status);drivers/net/can/pch_can.c line 753
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +/* Reading the MCONT register. */ + reg =ioread32 (&(priv->regs)->if2_mcont); + reg &=0xffff ; + +/* If MsgLost bit set. */ +if (reg &CAN_IF_MCONT_MSGLOST ) { +pch_can_bit_clear (&(priv->regs)->if2_mcont, +CAN_IF_MCONT_MSGLOST ); +dev_err (&priv->ndev->dev,"Msg Obj is overwritten.\n" );That should create an error message as well.
Quoted 6 lines
+ }
+ /* Read the direction bit for determination of remote frame . */
+ rtr = (ioread32((&(priv->regs)->if2_id2)) &CAN_ID2_DIR);
+ /* Clearing interrupts. */
+pch_can_int_clr(priv, status);
+ /* Hanlde reception interrupt */Typo!
Quoted 32 lines
+ if (priv->msg_obj[status - 1] ==
MSG_OBJ_RX) {
+ if (!(reg &CAN_IF_MCONT_NEWDAT)) {
+dev_err(&priv->ndev->dev, "MCONT_NEWDAT isn't SET.\n");
+ return;
+ }
+ skb =alloc_can_skb(priv->ndev, &cf);
+ if (!skb)
+ return;drivers/net/can/pch_can.c new file +24
-0+ + ide = ((ioread32 (&(priv->regs)->if2_id2)) &CAN_ID2_XTD ) >>14 ; +if (ide) { + id = (ioread32 (&(priv->regs)->if2_id1) &0xffff ); + id |= (((ioread32 (&(priv->regs)->if2_id2)) & +0x1fff ) <<16 ); + cf->can_id = (id &CAN_EFF_MASK ) |CAN_EFF_FLAG ; + }else { + id = (((ioread32 (&(priv->regs)->if2_id2)) & + (CAN_SFF_MASK <<2 )) >>2 ); + cf->can_id = (id &CAN_SFF_MASK ); + } + +if (rtr) { + cf->can_dlc =0 ; + cf->can_id |=CAN_RTR_FLAG ; + }else { + cf->can_dlc = ((ioread32 (&(priv->regs)->if2_mcont)) & +0x0f ); + } + +/* Reading back the data. */ +for (i =0 , j =0 ; i < cf->can_dlc; j++) { + reg =ioread32 (&(priv->regs)->if2_dataa1 + j*4 );drivers/net/can/pch_can.c line 801
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ cf->data[i++] =cpu_to_le32 (reg &0xff ); +if (i == cf->can_dlc) +break ; + cf->data[i++] =cpu_to_le32 ((reg & (0xff <<8 )) >>8 ); + } +netif_rx (skb); + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; + }else if (priv->msg_obj[status -1 ] ==MSG_OBJ_TX ) { +/* Hanlde transmission interrupt */ Typo!
Quoted 6 lines
+
can_get_echo_skb(priv->ndev, 0);
+netif_wake_queue(priv->ndev);
+ }drivers/net/can/pch_can.c new file +3
-0+} + +static void pch_can_error (struct net_device *ndev,u32 status)drivers/net/can/pch_can.c line 817
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+{ +struct sk_buff *skb; +struct pch_can_priv *priv =netdev_priv (ndev); +struct can_frame *cf; +u32 errc; +struct net_device_stats *stats = &(priv->ndev->stats); + + skb =alloc_can_err_skb (ndev, &cf); +if (!skb) { +dev_err (&ndev->dev,"%s -> No memory.\n" , __func__);Please drop the error message.
Quoted 4 lines
+ return;
+ }drivers/net/can/pch_can.c new file +2
-0+ +if (status &PCH_BUS_OFF ) {drivers/net/can/pch_can.c line 831
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+if (!priv->bus_off_interrupt) { +pch_can_tx_disable_all (priv); +pch_can_rx_disable_all (priv); + + priv->can.state =CAN_STATE_BUS_OFF ; + cf->can_id |=CAN_ERR_BUSOFF ; +can_bus_off (ndev); + + priv->bus_off_interrupt =1 ; +pch_can_set_run_mode (priv,PCH_CAN_RUN );Hm, you automatically restart the contoller after a bus-off. That's not the intended behaviour. It's up to the user to define how and when the device should recover from bus-off. For further information read
http://lxr.linux.no/#linux+v2.6.35.7/Documentation/networking/can.txt#L767
Quoted 38 lines
+ }
+ }+ /* Warning interrupt. */
+ if (status &PCH_EWARN) {
+priv->can.state=CAN_STATE_ERROR_WARNING;
+ priv->can.can_stats.error_warning++;
+cf->can_id|=CAN_ERR_CRTL;
+ errc = ioread32((&(priv->regs)->errc));
+ if (((errc &CAN_REC) >> 8) > 96)
+ cf->data[1] |=CAN_ERR_CRTL_RX_WARNING;
+ if ((errc &CAN_TEC) > 96)
+ cf->data[1] |=CAN_ERR_CRTL_TX_WARNING;
+dev_warn(&ndev->dev, "%s -> Warning interrupt.\n", __func__);
+ }
+ /* Error passive interrupt. */
+ if (status &PCH_EPASSIV) {
+ priv->can.can_stats.error_passive++;
+priv->can.state=CAN_STATE_ERROR_PASSIVE;
+cf->can_id|=CAN_ERR_CRTL;
+ errc = ioread32((&(priv->regs)->errc));
+ if (((errc &CAN_REC) >> 8) > 127)
+ cf->data[1] |=CAN_ERR_CRTL_RX_PASSIVE;
+ if ((errc &CAN_TEC) > 127)
+ cf->data[1] |=CAN_ERR_CRTL_TX_PASSIVE;
+ dev_err(&ndev->dev,
+ "%s -> Error interrupt.\n",__func__);
+ }drivers/net/can/pch_can.c new file +10
-0+ +if (status &PCH_STUF_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_STUFF ; + +if (status &PCH_FORM_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_FORM ; + +if (status &PCH_ACK_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_ACK |CAN_ERR_PROT_LOC_ACK_DEL ; +drivers/net/can/pch_can.c line 879
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+if ((status &PCH_BIT1_ERR ) || (status &PCH_BIT0_ERR )) + cf->data[2 ] |=CAN_ERR_PROT_BIT ; + +if (status &PCH_CRC_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_CRC_SEQ | +CAN_ERR_PROT_LOC_CRC_DEL ; + +if (status &PCH_LEC_ALL ) +iowrite32 (status |PCH_LEC_ALL , + &(priv->regs)->stat);A bit-wise test of the above values is wrong, I believe. Please use the switch statement instead.
drivers/net/can/pch_can.c line 886
Quoted 7 lines
drivers/net/can/pch_can.c new file +7
-0+ + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +netif_rx (skb); +} + +static irqreturn_t pch_can_handler (int irq,void *dev_id)A better name making clear that it's the interrupt handler would be nice.
Quoted 15 lines
drivers/net/can/pch_can.c new file +15
-0+{ +u32 int_stat; +u32 reg_stat; +struct net_device *ndev = (struct net_device *)dev_id; +struct pch_can_priv *priv =netdev_priv (ndev); + int_stat =pch_can_int_pending (priv); + +if (!int_stat) +return IRQ_NONE ; + +if (int_stat ==CAN_STATUS_INT ) { + reg_stat =ioread32 ((&(priv->regs)->stat)); +if (reg_stat & (PCH_BUS_OFF |PCH_LEC_ALL |PCH_EWARN | +PCH_EPASSIV )) { +if ((reg_stat &PCH_LEC_ALL ) !=PCH_LEC_ALL )drivers/net/can/pch_can.c line 911
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_error (ndev, reg_stat); + } + +/* Recover from Bus Off */ +if (!reg_stat && priv->bus_off_interrupt) { + priv->bus_off_interrupt =0 ; +pch_can_tx_enable_all (priv); +pch_can_rx_enable_all (priv); + +dev_info (&priv->ndev->dev,"BusOff stage recovered.\n" );Bogus bus-off handling, more later...
drivers/net/can/pch_can.c line 918
Quoted 7 lines
+ }
drivers/net/can/pch_can.c new file +6
-0+ +if (reg_stat &PCH_RX_OK ) +pch_can_bit_clear (&(priv->regs)->stat,PCH_RX_OK ); + +if (reg_stat &PCH_TX_OK ) +pch_can_bit_clear (&(priv->regs)->stat,PCH_TX_OK );Could be done in one call, I think.
Quoted 6 lines
+
int_stat=pch_can_int_pending(priv);
+ }drivers/net/can/pch_can.c new file +4
-0+ +if ((int_stat >0 ) && (int_stat <=MAX_MSG_OBJ )) +pch_can_msg_obj (ndev, int_stat); +drivers/net/can/pch_can.c line 935
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+return IRQ_HANDLED ; +} + +static int pch_set_bittiming (struct net_device *ndev) +{ +struct pch_can_priv *priv =netdev_priv (ndev); +const struct can_bittiming *bt = &priv->can.bittiming; +u32 curr_mode; +u32 reg1;/* CANBIT */ +u32 reg2;/* BEPE */ Why not "u32 canbit" then?
Quoted 5 lines
+ u32 brp;
+
+pch_can_get_run_mode(priv, &curr_mode);
+ if (curr_mode==PCH_CAN_RUN)
+pch_can_set_run_mode(priv, PCH_CAN_STOP);The device is stopped when this function is called. Please remove.
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ +/* Setting the CCE bit for accessing the Can Timing register. */ +pch_can_bit_set (&(priv->regs)->cont,CAN_CTRL_CCE ); + + brp = (bt->tq) / (1000000 /PCH_CAN_CLK ) -1 ; + reg1 = brp &MSK_BITT_BRP ;drivers/net/can/pch_can.c line 956
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ reg1 |= (bt->sjw -1 ) <<BIT_BITT_SJW ; + reg1 |= (bt->phase_seg1 + bt->prop_seg -1 ) <<BIT_BITT_TSEG1 ; + reg1 |= (bt->phase_seg2 -1 ) <<BIT_BITT_TSEG2 ; + reg2 = (brp &MSK_BRPE_BRPE ) >>BIT_BRPE_BRPE ; +iowrite32 (reg1, (&(priv->regs)->bitt)); +iowrite32 (reg2, (&(priv->regs)->brpe)); +pch_can_bit_clear (&(priv->regs)->cont,CAN_CTRL_CCE ); + +if (curr_mode ==PCH_CAN_RUN ) +pch_can_set_run_mode (priv,PCH_CAN_RUN );Ditto.
drivers/net/can/pch_can.c line 970
Quoted 13 lines
+ return 0;
drivers/net/can/pch_can.c new file +12
-0+} + +static void pch_can_start (struct net_device *ndev) +{ +struct pch_can_priv *priv =netdev_priv (ndev); + +if (priv->can.state !=CAN_STATE_STOPPED ) +pch_can_reset (priv); + +pch_set_bittiming (ndev); +pch_can_set_optmode (priv); + priv->can.state =CAN_STATE_ERROR_ACTIVE ;Hm, where do you really start the controller. I'm missing
pch_can_set_run_mode(priv, PCH_CAN_RUN).Quoted 9 lines
+ return;
drivers/net/can/pch_can.c new file +8
-0+} + +static int pch_can_do_set_mode (struct net_device *ndev,enum can_mode mode) +{ +int ret =0 ; + +switch (mode) { +case CAN_MODE_START :drivers/net/can/pch_can.c line 990
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_start (ndev); +netif_wake_queue (ndev); +break ; +default : + ret = -EOPNOTSUPP ; +break ; + } + +return ret; +}Note that this function is called when the device will recover from bus-off.
drivers/net/can/pch_can.c line 999
Quoted 8 lines
drivers/net/can/pch_can.c new file +8
-0+static int pch_can_get_state (const struct net_device *ndev, +enum can_state *state) +{ +struct pch_can_priv *priv =netdev_priv (ndev); + + *state = priv->can.state; +return 0 ; +}There is no need for that function as the driver handles state changes in the interrupt handler.
Quoted 1 line
+static int
pch_open(struct net_device *ndev)That's confussing! Please use the prefix
pch_canthrought this file.drivers/net/can/pch_can.c line 1006
Quoted 5 lines
drivers/net/can/pch_can.c new file +5
-0+{ +struct pch_can_priv *priv =netdev_priv (ndev); +int retval; + +pch_can_open (priv);This function already starts the controller, which is too *early*.
Quoted 16 lines
drivers/net/can/pch_can.c new file +16
-0+ + retval =pci_enable_msi (priv->dev); +if (retval) { +dev_err (&ndev->dev,"Unable to allocate MSI ret=%d\n" , retval); + goto pci_en_msi_err; + } + +/* Regsitering the interrupt. */ + retval =request_irq (priv->dev->irq, pch_can_handler,IRQF_SHARED , + ndev->name, ndev); +if (retval) { +dev_err (&ndev->dev,"request_irq failed.\n" ); + goto req_irq_err; + } + +/* Assuming that no bus off interrupt. */ drivers/net/can/pch_can.c line 1032
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ priv->bus_off_interrupt =0 ; + +/* Open common can device */ + retval =open_candev (ndev); +if (retval) { +dev_err (ndev->dev.parent,"open_candev() failed %d\n" , retval); + goto err_open_candev; + } + +pch_can_start (ndev);Thde above function should finally start the controller.
Quoted 64 lines
+
netif_start_queue(ndev);drivers/net/can/pch_can.c new file +63
-0+ +return 0 ; + +err_open_candev: +free_irq (priv->dev->irq, ndev); +req_irq_err: +pci_disable_msi (priv->dev); +pci_en_msi_err: +pch_can_release (priv); + +return retval; +} + +static int pch_close (struct net_device *ndev) +{ +struct pch_can_priv *priv =netdev_priv (ndev); + +netif_stop_queue (ndev); +pch_can_release (priv); +free_irq (priv->dev->irq, ndev); +pci_disable_msi (priv->dev); +close_candev (ndev); + priv->can.state =CAN_STATE_STOPPED ; +return 0 ; +} + +static int pch_get_free_msg_obj (struct net_device *ndev) +{ +u32 buffer_status =0 ; +u32 tx_disable_counter =0 ; +u32 tx_buffer_avail =0 ; +u32 status; +s32 i; +struct pch_can_priv *priv =netdev_priv (ndev); + +/* Getting the message object status. */ + buffer_status = (u32 )pch_can_get_buffer_status (priv); + +/* Getting the free transmit message object. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if ((priv->msg_obj[i] ==MSG_OBJ_TX )) { +/* Checking whether the object is enabled. */ +pch_can_get_tx_enable (priv, i +1 , &status); +if (status ==ENABLE ) { +if (!((buffer_status >> i) &1 )) { + tx_buffer_avail = (i +1 ); +break ; + } + }else { + tx_disable_counter++; + } + } + } + +/* If no transmit object available. */ +if (!tx_buffer_avail) { +/* If no object is enabled. */ +if ((tx_disable_counter ==PCH_TX_OBJ_NUM )) { +dev_err (&ndev->dev,"All tx buffers are disabled.\n" ); +return -EPERM ; + }else { +dev_err (&ndev->dev,"%s:No tx buf free.\n" , __func__); +return -PCH_CAN_NO_TX_BUFF ;drivers/net/can/pch_can.c line 1106
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + } +return tx_buffer_avail; +} + +static netdev_tx_t pch_xmit (struct sk_buff *skb,struct net_device *ndev) +{ +canid_t id; +u32 id1 =0 ; +u32 id2 =0 ;Need these values to be preset?
Quoted 2 lines
+ u32
run_mode;
+ u32 i, j;It's common to use type "int" for the usual incrementer value... as you do in other places as well. Please check!
Quoted 5 lines
+ unsigned long flags;
+ structpch_can_priv*priv =netdev_priv(ndev);
+ structcan_frame*cf = (structcan_frame*)skb->data;
+ structnet_device_stats*stats =&ndev->stats;
+ u32tx_buffer_avail= 0;drivers/net/can/pch_can.c line 1123
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +if (can_dropped_invalid_skb (ndev, skb)) +return NETDEV_TX_OK ; + +/* Getting the current CAN mode. */ +pch_can_get_run_mode (priv, &run_mode); +if (run_mode !=PCH_CAN_RUN ) { +dev_err (&ndev->dev,"CAN stopped on transmit attempt.\n" ); +return -EPERM ; + }Can this happen? I think this check can be removed. Anyway,
-EPERMis not a valid return value for that function.Quoted 4 lines
+
+tx_buffer_avail=pch_get_free_msg_obj(ndev);
+ if (tx_buffer_avail< 0)
+ returntx_buffer_avail;Wrong return value?
Quoted 6 lines
drivers/net/can/pch_can.c new file +6
-0+ +/* Attaining the lock. */ +spin_lock_irqsave (&priv->msgif_reg_lock, flags); + +/* Reading the Msg Obj from the Msg RAM to the Interface register. */ +iowrite32 (CAN_CMASK_RX_TX_GET , &(priv->regs)->if1_cmask);drivers/net/can/pch_can.c line 1143
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+pch_can_check_if1_busy (priv, tx_buffer_avail); + +/* Setting the CMASK register. */ +pch_can_bit_set (&(priv->regs)->if1_cmask,CAN_CMASK_ALL ); + +/* If ID extended is set. */ +if (cf->can_id &CAN_EFF_FLAG ) { + id = cf->can_id &CAN_EFF_MASK ; + id1 = id &0xffff ; + id2 = ((id & (0x1fff <<16 )) >>16 ) |CAN_ID2_XTD ;Please use some more macro definitions for the sake of readability.
Quoted 32 lines
+ } else {
+ id =cf->can_id&CAN_SFF_MASK;
+ id1 = 0;
+ id2 = ((id &CAN_SFF_MASK) << 2);
+ }
+pch_can_bit_clear(&(priv->regs)->if1_id1, 0xffff);
+pch_can_bit_clear(&(priv->regs)->if1_id2, 0x1fff | CAN_ID2_XTD);
+pch_can_bit_set(&(priv->regs)->if1_id1, id1);
+pch_can_bit_set(&(priv->regs)->if1_id2, id2);drivers/net/can/pch_can.c new file +23
-0+ +/* If remote frame has to be transmitted.. */ +if (cf->can_id &CAN_RTR_FLAG ) +pch_can_bit_clear (&(priv->regs)->if1_id2,CAN_ID2_DIR ); + +for (i =0 , j =0 ; i < cf->can_dlc; j++) { +iowrite32 (le32_to_cpu (cf->data[i++]), + (&(priv->regs)->if1_dataa1) + j*4 ); +if (i == cf->can_dlc) +break ; +iowrite32 (le32_to_cpu (cf->data[i++] <<8 ), + (&(priv->regs)->if1_dataa1) + j*4 ); + } +can_put_echo_skb (skb, ndev,0 ); + +/* Updating the size of the data. */ +pch_can_bit_clear (&(priv->regs)->if1_mcont,0x0f ); +pch_can_bit_set (&(priv->regs)->if1_mcont, cf->can_dlc); + +/* Clearing IntPend, NewDat & TxRqst */ +pch_can_bit_clear (&(priv->regs)->if1_mcont, + (CAN_IF_MCONT_NEWDAT |CAN_IF_MCONT_INTPND | +CAN_IF_MCONT_TXRQXT ));drivers/net/can/pch_can.c line 1185
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +/* Setting NewDat, TxRqst bits */ +pch_can_bit_set (&(priv->regs)->if1_mcont, + (CAN_IF_MCONT_NEWDAT |CAN_IF_MCONT_TXRQXT )); + +pch_can_check_if1_busy (priv, tx_buffer_avail); +spin_unlock_irqrestore (&priv->msgif_reg_lock, flags); + + stats->tx_bytes += cf->can_dlc; + stats->tx_packets++;That shoould be incremented when the TX done interrupt is handled.
Quoted 40 lines
drivers/net/can/pch_can.c new file +40
-0+ +return NETDEV_TX_OK ; +} + +static const struct net_device_ops pch_can_netdev_ops = { + .ndo_open = pch_open, + .ndo_stop = pch_close, + .ndo_start_xmit = pch_xmit, +}; + +static void __devexit pch_can_remove(struct pci_dev *pdev) +{ +struct net_device *ndev =pci_get_drvdata (pdev); +struct pch_can_priv *priv =netdev_priv (ndev); + +unregister_candev (priv->ndev); +free_candev (priv->ndev); +pci_iounmap (pdev, priv->base); +pci_release_regions (pdev); +pci_disable_device (pdev); +pci_set_drvdata (pdev,NULL ); +pch_can_reset (priv); +} + +#ifdef CONFIG_PM +static int pch_can_suspend (struct pci_dev *pdev,pm_message_t state) +{ +int i;/* Counter variable. */ +int retval;/* Return value. */ +u32 buf_stat;/* Variable for reading the transmit buffer status. */ +u32 counter =0xFFFFFF ; + +struct net_device *dev =pci_get_drvdata (pdev); +struct pch_can_priv *priv =netdev_priv (dev); + +/* Stop the CAN controller */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Indicate that we are aboutto/in suspend */ + priv->can.state =CAN_STATE_SLEEPING ;drivers/net/can/pch_can.c line 1235
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ +/* Waiting for all transmission to complete. */ +while (counter) { + buf_stat =pch_can_get_buffer_status (priv); +if (!buf_stat) +break ; + counter--; + } +if (!counter) +dev_err (&pdev->dev,"%s -> Transmission time out.\n" , __func__);Timeout without defined delay!
Quoted 90 lines
+ /* Save interrupt configuration and then disable them */
+pch_can_get_int_enables(priv, &(priv->int_enables));
+pch_can_set_int_enables(priv, PCH_CAN_DISABLE);drivers/net/can/pch_can.c new file +87
-0+ +/* Save Tx buffer enable state */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) +pch_can_get_tx_enable (priv, (i +1 ), + &(priv->tx_enable[i])); + } + +/* Disable all Transmit buffers */ +pch_can_tx_disable_all (priv); + +/* Save Rx buffer enable state */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) { +pch_can_get_rx_enable (priv, (i +1 ), + &(priv->rx_enable[i])); +pch_can_get_rx_buffer_link (priv, (i +1 ), + &(priv->rx_link[i])); + } + } + +/* Disable all Receive buffers */ +pch_can_rx_disable_all (priv); + retval =pci_save_state (pdev); +if (retval) { +dev_err (&pdev->dev,"pci_save_state failed.\n" ); + }else { +pci_enable_wake (pdev, PCI_D3hot,0 ); +pci_disable_device (pdev); +pci_set_power_state (pdev,pci_choose_state (pdev, state)); + } + +return retval; +} + +static int pch_can_resume (struct pci_dev *pdev) +{ +int i;/* Counter variable. */ +int retval;/* Return variable. */ +struct net_device *dev =pci_get_drvdata (pdev); +struct pch_can_priv *priv =netdev_priv (dev); + +pci_set_power_state (pdev,PCI_D0 ); +pci_restore_state (pdev); + retval =pci_enable_device (pdev); +if (retval) { +dev_err (&pdev->dev,"pci_enable_device failed.\n" ); +return retval; + } + +pci_enable_wake (pdev, PCI_D3hot,0 ); + + priv->can.state =CAN_STATE_ERROR_ACTIVE ; + +/* Disabling all interrupts. */ +pch_can_set_int_enables (priv,PCH_CAN_DISABLE ); + +/* Setting the CAN device in Stop Mode. */ +pch_can_set_run_mode (priv,PCH_CAN_STOP ); + +/* Configuring the transmit and receive buffers. */ +pch_can_config_rx_tx_buffers (priv); + +/* Restore the CAN state */ +pch_set_bittiming (dev); + +/* Listen/Active */ +pch_can_set_optmode (priv); + +/* Enabling the transmit buffer. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_TX ) { +pch_can_set_tx_enable (priv, i +1 , + priv->tx_enable[i]); + } + } + +/* Configuring the receive buffer and enabling them. */ +for (i =0 ; i <PCH_OBJ_NUM ; i++) { +if (priv->msg_obj[i] ==MSG_OBJ_RX ) { +/* Restore buffer link */ +pch_can_set_rx_buffer_link (priv, i +1 , + priv->rx_link[i]); + +/* Restore buffer enables */ +pch_can_set_rx_enable (priv, i +1 , priv->rx_enable[i]); + }drivers/net/can/pch_can.c line 1336
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + +/* Enable CAN Interrupts */ +pch_can_set_int_custom (priv); + +/* Restore Run Mode */ +pch_can_set_run_mode (priv,PCH_CAN_RUN ); + +return retval; +}Are the suspend and resume functions tested?
drivers/net/can/pch_can.c line 1340
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+#else +#define pch_can_suspend NULL +#define pch_can_resume NULL +#endif Add empty line here
Quoted 33 lines
drivers/net/can/pch_can.c new file +33
-0+static int __devinit pch_can_probe (struct pci_dev *pdev, +const struct pci_device_id *id) +{ +struct net_device *ndev; +struct pch_can_priv *priv; +int rc; +int index; +void __iomem *addr; + + rc =pci_enable_device (pdev); +if (rc) { +dev_err (&pdev->dev,"Failed pci_enable_device %d\n" , rc); + goto probe_exit_endev; + } + + rc =pci_request_regions (pdev,KBUILD_MODNAME ); +if (rc) { +dev_err (&pdev->dev,"Failed pci_request_regions %d\n" , rc); + goto probe_exit_pcireq; + } + + addr =pci_iomap (pdev,1 ,0 ); +if (!addr) { + rc = -EIO ; +dev_err (&pdev->dev,"Failed pci_iomap\n" ); + goto probe_exit_ipmap; + } + + ndev =alloc_candev (sizeof (struct pch_can_priv ),1 ); +if (!ndev) { + rc = -ENOMEM ; +dev_err (&pdev->dev,"Failed alloc_candev\n" ); + goto probe_exit_alloc_candev;drivers/net/can/pch_can.c line 1383
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+ } + + priv =netdev_priv (ndev); + priv->ndev = ndev; + priv->base = addr; + priv->regs = addr; + priv->dev = pdev; + priv->can.bittiming_const = &pch_can_bittiming_const; + priv->can.do_set_mode = pch_can_do_set_mode; + priv->can.do_get_state = pch_can_get_state;Not needed! See above.
Could you please also implement
do_get_berr_counter().Quoted 49 lines
+
priv->can.ctrlmode_supported=CAN_CTRLMODE_LISTENONLY|drivers/net/can/pch_can.c new file +48
-0+CAN_CTRLMODE_LOOPBACK ; + ndev->irq = pdev->irq; + ndev->flags |=IFF_ECHO ; + +pci_set_drvdata (pdev, ndev); +SET_NETDEV_DEV (ndev, &pdev->dev); + ndev->netdev_ops = &pch_can_netdev_ops; + + priv->can.clock.freq =PCH_CAN_CLK *1000 ;/* Hz to KHz) */ +for (index =0 ; index <PCH_RX_OBJ_NUM ;) + priv->msg_obj[index++] =MSG_OBJ_RX ; + +for (index = index; index <PCH_OBJ_NUM ;) + priv->msg_obj[index++] =MSG_OBJ_TX ; + + rc =register_candev (ndev); +if (rc) { +dev_err (&pdev->dev,"Failed register_candev %d\n" , rc); + goto probe_exit_reg_candev; + } + +return 0 ; + +probe_exit_reg_candev: +free_candev (ndev); +probe_exit_alloc_candev: +pci_iounmap (pdev, addr); +probe_exit_ipmap: +pci_release_regions (pdev); +probe_exit_pcireq: +pci_disable_device (pdev); +probe_exit_endev: +return rc; +} + +static struct pci_driver pch_can_pcidev = { + .name =KBUILD_MODNAME , + .id_table = pch_can_pcidev_id, + .probe = pch_can_probe, + .remove =__devexit_p (pch_can_remove), + .suspend = pch_can_suspend, + .resume = pch_can_resume, +}; + +static int __init pch_can_pci_init (void ) +{ +return pci_register_driver (&pch_can_pcidev); +}drivers/net/can/pch_can.c line 1442
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+module_init (pch_can_pci_init); + +static void __exit pch_can_pci_exit(void) +{ +pci_unregister_driver (&pch_can_pcidev); +} +module_exit (pch_can_pci_exit); + +MODULE_DESCRIPTION ("Controller Area Network Driver" ); +MODULE_LICENSE ("GPL" );GPL v2 ?
drivers/net/can/pch_can.c line 1444
Quoted 2 lines
drivers/net/can/pch_can.c new file +2
-0+MODULE_VERSION ("0.94" ); +MODULE_DEVICE_TABLE (pci, pch_can_pcidev_id);Please add it below the declaration of
pch_can_pcidev_id.In this driver you are using just *one* RX object. This means that the CPU must handle new messages as quickly as possible otherwise message losses will happen, right?. For sure, this will not make user's happy. Any chance to use more RX objects in FIFO mode?
Thanks,
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines · Masayuki Ohtake
Hi Wolfgang Grandegger,
Thank you for your comments.
We will modify and re-post ASAP.
Quoted 8 lines
I have a comment about below.
Quoted 4 lines
In this driver you are using just *one* RX object. This means that the CPU must handle new messages as quickly as possible otherwise message losses will happen, right?. For sure, this will not make user's happy. Any chance to use more RX objects in FIFO mode?
In case implementing with FIFO mode,
received packets may be our of order.
Hm, FIFO means "First in First out"! It might be tricky to implement, though.
Quoted 3 lines
Because our CAN register access is slow.
I am confirming our CAN HW spec and the possibility of our-of-order.
I don't understand?
Wolfgang.
Hi Wolfgang,
I could confirm below.
With FIFO mode, it is able to receive packet with in-order. We are now implementing FIFO mode.
Thanks, Ohtake(OKISemi)
----- Original Message -----
From: "Wolfgang Grandegger" <[email protected]>
To: "Masayuki Ohtake" <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>
Cc: <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; "Christian Pellegrin" <[email protected]>; "Tomoya MORINAGA" <morinaga526-ECg8zkTtlr0C6LszWs/[email protected]>; <[email protected]>; "David S. Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>; <[email protected]>; "Samuel Ortiz" <sameo-VuQAYsv1563Yd54FQh9/[email protected]>
Sent: Friday, October 01, 2010 9:40 PM
Subject: Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
Quoted 32 lines · Masayuki Ohtake
Quoted 6 lines
Hi Wolfgang Grandegger,
Thank you for your comments.
We will modify and re-post ASAP.
Quoted 8 lines
I have a comment about below.
Quoted 4 lines
In this driver you are using just *one* RX object. This means that the CPU must handle new messages as quickly as possible otherwise message losses will happen, right?. For sure, this will not make user's happy. Any chance to use more RX objects in FIFO mode?
In case implementing with FIFO mode,
received packets may be our of order.Hm, FIFO means "First in First out"! It might be tricky to implement, though.
Quoted 3 lines
Because our CAN register access is slow.
I am confirming our CAN HW spec and the possibility of our-of-order.
I don't understand?
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde
maintainerQuoted 5 lines · Masayuki Ohtake
Hi Wolfgang,
I could confirm below.
With FIFO mode, it is able to receive packet with in-order. We are now implementing FIFO mode.
If FIFO is working you might also think about NAPI.
cheers, Marc
Signature · 4 lines
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 |
Hi Marc,
Quoted 1 line · Marc Kleine-Budde
If FIFO is working you might also think about NAPI.
I think NAPI isn't necessary for our CAN driver.
NAPI is for high-speed networking.
CAN is NOT high-speed.
In fact, some accepted CAN drivers don't have NAPI.
Thanks, Ohtake(OKISemi)
David Miller
maintainerFrom: "Masayuki Ohtake" <[email protected]>
Date: Tue, 5 Oct 2010 21:09:30 +0900
Quoted 8 lines · Marc Kleine-Budde
Quoted 1 line
If FIFO is working you might also think about NAPI.
I think NAPI isn't necessary for our CAN driver.
NAPI is for high-speed networking.
CAN is NOT high-speed.In fact, some accepted CAN drivers don't have NAPI.
NAPI is not just for performance concerns.
It greatly simplifies the locking since all packet processing paths run only in software interrupt context, never in hardware interrupt context.
We encourage all new drivers that can use NAPI to do so, just because recent driver submissions do not do this doesn't mean we should continue such a mistake ;-)
Hi David,
On Wednesday, October 06, 2010 3:45 AM, David Miller wrote,
Quoted 3 lines
We encourage all new drivers that can use NAPI to do so, just because recent driver submissions do not do this doesn't mean we should continue such a mistake ;-)
I understand NAPI's merit.
But, since we have already implemented without NAPI. If possible, we don't want to use NAPI.
Does your mail mean, for accepting upstream, NAPI is essential for CAN driver ? If essential, we will just do.
Thanks, Ohtake(OKISemi)
----- Original Message -----
From: "David Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>
To: <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>
Cc: <[email protected]>; <sameo-VuQAYsv1563Yd54FQh9/[email protected]>; <[email protected]>; <[email protected]>; <morinaga526-ECg8zkTtlr0C6LszWs/[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Wednesday, October 06, 2010 3:45 AM
Subject: Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
Quoted 26 lines
From: "Masayuki Ohtake" <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>
Date: Tue, 5 Oct 2010 21:09:30 +0900Quoted 8 lines · Marc Kleine-Budde
Quoted 1 line
If FIFO is working you might also think about NAPI.
I think NAPI isn't necessary for our CAN driver.
NAPI is for high-speed networking.
CAN is NOT high-speed.In fact, some accepted CAN drivers don't have NAPI.
NAPI is not just for performance concerns.
It greatly simplifies the locking since all packet processing paths run only in software interrupt context, never in hardware interrupt context.
We encourage all new drivers that can use NAPI to do so, just because recent driver submissions do not do this doesn't mean we should continue such a mistake ;-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller
maintainerFrom: "Masayuki Ohtake" <[email protected]>
Date: Wed, 6 Oct 2010 12:07:15 +0900
Quoted 2 lines
Does your mail mean, for accepting upstream, NAPI is essential for CAN driver ?
It is up to the CAN maintainers :-)
Quoted 7 lines · David Miller
From: "Masayuki Ohtake" <[email protected]>
Date: Wed, 6 Oct 2010 12:07:15 +0900Quoted 2 lines
Does your mail mean, for accepting upstream, NAPI is essential for CAN driver ?
It is up to the CAN maintainers :-)
Well, our SJA1000 reference driver does still not use NAPI. But NAPI is for CAN especially useful to avoid the infamous *bus error irq flooding*, which may hang low end systems if the interrupts are handled in the IRQ context. Ohtake, if your system can handle well such CAN bus error irq storms at 1MB/s, then NAPI is *not* a must to have. Anyway, as you are at it, I also suggest to use NAPI right from the beginning.
Wolfgang.
Hi Wolfgang,
We have implemented our CAN driver with FIFO mode, and
We are testing our CAN driver with FIFO mode.
However, we have found Our CAN hardware spec is different from our anticipated. Our CAN HW FIFO is not common FIFO.
Using FIFO mode, there is possibility received packets are out-of-order.
e.g.
Recv packet-A from NW and set to FIFO.
|A|
Recv packet-B from NW and set to FIFO.
|A|B|
Recv packet-C is about to set to FIFO
|A|B|(C)|
Userspace Copies A from Driver
Userspace Copies B from Driver
| | |(C)|
packet-C set to FIFO (C is not head.)
Recv packet-D from NW(Next packet is set to head)
|D| |C|
Userspace Copies D from Driver
Userspace Copies C from Driver
Userspace raceived packet order is like below
A-B-D-C
So, I think normal-mode is better than FIFO-mode.
I will revert like the following spec.
Rx 1 Message Object
Tx 1 Message Object
Could you agree the above ?
Thanks, Ohtake(OKISemi)
----- Original Message -----
From: "Wolfgang Grandegger" <[email protected]>
To: "David Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>
Cc: <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>; <[email protected]>; <sameo-VuQAYsv1563Yd54FQh9/[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <morinaga526-ECg8zkTtlr0C6LszWs/[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Wednesday, October 06, 2010 6:12 PM
Subject: Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
Quoted 22 lines · David Miller
Quoted 7 lines
From: "Masayuki Ohtake" <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>
Date: Wed, 6 Oct 2010 12:07:15 +0900Quoted 2 lines
Does your mail mean, for accepting upstream, NAPI is essential for CAN driver ?
It is up to the CAN maintainers :-)
Well, our SJA1000 reference driver does still not use NAPI. But NAPI is for CAN especially useful to avoid the infamous *bus error irq flooding*, which may hang low end systems if the interrupts are handled in the IRQ context. Ohtake, if your system can handle well such CAN bus error irq storms at 1MB/s, then NAPI is *not* a must to have. Anyway, as you are at it, I also suggest to use NAPI right from the beginning.
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ohtake,
Quoted 12 lines · Masayuki Ohtake
Hi Wolfgang,
We have implemented our CAN driver with FIFO mode, and
We are testing our CAN driver with FIFO mode.
However, we have found Our CAN hardware spec is different from our anticipated. Our CAN HW FIFO is not common FIFO.
Using FIFO mode, there is possibility received packets are out-of-order.e.g.
Recv packet-A from NW and set to FIFO.
|A|
Quoted 7 lines
Recv packet-B from NW and set to FIFO.
|A|B|Recv packet-C is about to set to FIFO
|A|B|(C)|Userspace Copies A from Driver
OK, let's say the CPU or software starts processing the message FIFO.
Quoted 8 lines
Userspace Copies B from Driver
| | |(C)|packet-C set to FIFO (C is not head.)
Recv packet-D from NW(Next packet is set to head)
|D| |C|Userspace Copies D from Driver
The software could continues searching the FIFO for valid messages. Then it would find C first.
Quoted 3 lines
Userspace Copies C from Driver
Userspace raceived packet order is like below
A-B-D-C
I'm still optimistic that it could be handled properly by software, it might be tricky though.
Quoted 1 line
So, I think normal-mode is better than FIFO-mode.
To be clear. Out-of-order reception is not allowed!
Quoted 5 lines
I will revert like the following spec.
Rx 1 Message Object
Tx 1 Message ObjectCould you agree the above ?
See above. I agree if the software cannot assure in-order reception. But it's not yet obvious to me that it cannot be achieved. I will have a closer look to the manual. Just one RX message object without any further buffering is really bad as message losses are likely to happen.
Wolfgang.
Marc Kleine-Budde
maintainerHello Masayuki,
Quoted 5 lines · Masayuki Ohtake
We have implemented our CAN driver with FIFO mode, and
We are testing our CAN driver with FIFO mode.
However, we have found Our CAN hardware spec is different from our anticipated. Our CAN HW FIFO is not common FIFO.
Using FIFO mode, there is possibility received packets are out-of-order.
It's called a FIFO in the Documentation, but it's not a real FIFO as we all hoped.
Quoted 14 lines
e.g.
Recv packet-A from NW and set to FIFO.
|A|Recv packet-B from NW and set to FIFO.
|A|B|Recv packet-C is about to set to FIFO
|A|B|(C)|Userspace Copies A from Driver
Userspace Copies B from Driver
| | |(C)|
Quoted 8 lines
packet-C set to FIFO (C is not head.)
Recv packet-D from NW(Next packet is set to head)
|D| |C|Userspace Copies D from Driver
Userspace Copies C from Driver
Userspace raceived packet order is like below
A-B-D-C
I just had a small peak at the datasheet. It seems you can implement the same scheme for rx as on the at91, if you enough rx buffers. In the at91 driver I use 8+4 = 12 buffers for rx. Have a look at the driver, the rx path is documented.
The driver uses 8 + 4 rx buffers, the trick is that a buffer in read but _not_ marked as free, if I understand the intel datasheet correctly this is by not resetting the NEWDAT bit. Becasue the driver doesn't free the buffers it knows that the next CAN frame is in the next buffer. After reading the 8th buffer, the first 8 buffers (0-7) are marked as free. Then the driver continues to read buffers 8-11, they might have been filled before marking 0-7 as free. Then the driver continues from the beginning.
Quoted 7 lines
So, I think normal-mode is better than FIFO-mode.
I will revert like the following spec.
Rx 1 Message Object
Tx 1 Message ObjectCould you agree the above ?
ahh common...the hardware doesn't seem that bad :)
cheers, Marc
Signature · 4 lines
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 |
_______________________________________________
Socketcan-core mailing list [email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
Quoted 4 lines · Marc Kleine-Budde
I just had a small peak at the datasheet. It seems you can implement the same scheme for rx as on the at91, if you enough rx buffers. In the at91 driver I use 8+4 = 12 buffers for rx. Have a look at the driver, the rx path is documented.
Thank you for your suggestion.
This is valuable for us.
It seems the at91's Hw benabior similar to ours.
Using this method, we will implement and test.
Thanks, Ohtake(OKISemi)
Marc Kleine-Budde
maintainerQuoted 10 lines · Masayuki Ohtake
Quoted 4 lines · Marc Kleine-Budde
I just had a small peak at the datasheet. It seems you can implement the same scheme for rx as on the at91, if you enough rx buffers. In the at91 driver I use 8+4 = 12 buffers for rx. Have a look at the driver, the rx path is documented.
Thank you for your suggestion.
This is valuable for us.
It seems the at91's Hw benabior similar to ours.
Using this method, we will implement and test.
Feel free to ask if you have questions regarding the algorithm or the code or have some code to review.
cheers, Marc
Signature · 4 lines
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 |
_______________________________________________
Socketcan-core mailing list [email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
Quoted 13 lines · Wolfgang Grandegger
+
iowrite32(num, &(priv->regs)->if2_creq);
+ while (counter) {drivers/net/can/pch_can.c line 337
Quoted 9 lines
+
if2_creq= (ioread32(&(priv->regs)->if2_creq)) &drivers/net/can/pch_can.c new file +8
-0+CAN_IF_CREQ_BUSY ; +if (!if2_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF2 BUSY Flag is set forever.\n" ); +}
Quoted 1 line
Duplicated code!
No.
These are not the same.
Though it is possible to integrate to one function by adding parameter, I think, current two function method is more easily to read.
Quoted 23 lines
Quoted 8 lines
+ if (status &
PCH_STUF_ERR)
+ cf->data[2] |=CAN_ERR_PROT_STUFF;
+
+ if (status &PCH_FORM_ERR)
+ cf->data[2] |=CAN_ERR_PROT_FORM;drivers/net/can/pch_can.c new file +14
-0+ +if (status &PCH_ACK_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_ACK |CAN_ERR_PROT_LOC_ACK_DEL ; + +if ((status &PCH_BIT1_ERR ) || (status &PCH_BIT0_ERR )) + cf->data[2 ] |=CAN_ERR_PROT_BIT ; + +if (status &PCH_CRC_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_CRC_SEQ | +CAN_ERR_PROT_LOC_CRC_DEL ; + +if (status &PCH_LEC_ALL ) +iowrite32 (status |PCH_LEC_ALL , + &(priv->regs)->stat);
Quoted 2 lines
A bit-wise test of the above values is wrong, I believe. Please use the switch statement instead.
The above conditions are not only one time.
I think "switch" is not suitable for the above.
Thus, current "if" processing is better.
Quoted 9 lines
+ u32 brp;
+
+pch_can_get_run_mode(priv, &curr_mode);
+ if (curr_mode==PCH_CAN_RUN)
+pch_can_set_run_mode(priv, PCH_CAN_STOP);The device is stopped when this function is called. Please remove.
No.
The above is necessary.
Because this is our HW specification.
Before setting bitrate, run-mode must be "STOP".
Quoted 9 lines
drivers/net/can/pch_can.c new file +5
-0+static netdev_tx_t pch_xmit (struct sk_buff *skb,struct net_device *ndev) +{ +canid_t id; +u32 id1 =0 ; +u32 id2 =0 ;Need these values to be preset?
These values are not essential.
But these help a engineer to read this code.
Quoted 11 lines
+ /* Enable CAN Interrupts */
+pch_can_set_int_custom(priv);drivers/net/can/pch_can.c new file +6
-0+ +/* Restore Run Mode */ +pch_can_set_run_mode (priv,PCH_CAN_RUN ); + +return retval; +}
Quoted 2 lines
Are the suspend and resume functions tested?
Yes, we tested before.
=========================================
Except the above, we are modifying for your indications.
I will resubmit soon.
Thanks, Ohtake(OKISemi)
Quoted 15 lines · Masayuki Ohtake
Quoted 13 lines · Wolfgang Grandegger
+
iowrite32(num, &(priv->regs)->if2_creq);
+ while (counter) {Quoted 9 lines
+
if2_creq= (ioread32(&(priv->regs)->if2_creq)) &drivers/net/can/pch_can.c new file +8
-0+CAN_IF_CREQ_BUSY ; +if (!if2_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF2 BUSY Flag is set forever.\n" ); +}
Quoted 4 lines
Quoted 1 line
Duplicated code!
No.
These are not the same.
Of course they are not the same. The only difference is the register offset (of if1 or if2). A common function with a pointer to the if register as argument makes sense.
Quoted 2 lines
Though it is possible to integrate to one function by adding parameter, I think, current two function method is more easily to read.
I disagree.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
+ if (status &
PCH_STUF_ERR)
+ cf->data[2] |=CAN_ERR_PROT_STUFF;
+
+ if (status &PCH_FORM_ERR)
+ cf->data[2] |=CAN_ERR_PROT_FORM;
Quoted 14 lines
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+ +if (status &PCH_ACK_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_ACK |CAN_ERR_PROT_LOC_ACK_DEL ; +drivers/net/can/pch_can.c line 879
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+if ((status &PCH_BIT1_ERR ) || (status &PCH_BIT0_ERR )) + cf->data[2 ] |=CAN_ERR_PROT_BIT ; + +if (status &PCH_CRC_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_CRC_SEQ | +CAN_ERR_PROT_LOC_CRC_DEL ; + +if (status &PCH_LEC_ALL ) +iowrite32 (status |PCH_LEC_ALL , + &(priv->regs)->stat);
Well, if status==7 (PCH_LEC_ALL), all of the above conditions are true as well... convinced now?
Quoted 6 lines
Quoted 2 lines
A bit-wise test of the above values is wrong, I believe. Please use the switch statement instead.
The above conditions are not only one time.
I think "switch" is not suitable for the above.
Thus, current "if" processing is better.
I don't understand! The Last Error Code (LEC) can have values from 0 to
7. A "switch" statement is therefore the right choice. Or have I missed something.
Quoted 8 lines
Quoted 8 lines
+ u32 brp;
+
+pch_can_get_run_mode(priv, &curr_mode);
+ if (curr_mode==PCH_CAN_RUN)
+pch_can_set_run_mode(priv, PCH_CAN_STOP);
Quoted 4 lines
Quoted 1 line
The device is stopped when this function is called. Please remove.
No.
The above is necessary.
Yes, because you started the device *too early* in pch_can_open() called by pch_open(). See my other related comments of my previous mail.
Quoted 2 lines
Because this is our HW specification.
Before setting bitrate, run-mode must be "STOP".
I think it can be avoided easily.
Quoted 8 lines
Quoted 8 lines
drivers/net/can/pch_can.c new file +5
-0+static netdev_tx_t pch_xmit (struct sk_buff *skb,struct net_device *ndev) +{ +canid_t id; +u32 id1 =0 ; +u32 id2 =0 ;
Quoted 4 lines
Quoted 1 line
Need these values to be preset?
These values are not essential.
But these help a engineer to read this code.
I disagree.
Quoted 13 lines
Quoted 11 lines
+ /* Enable CAN Interrupts */
+pch_can_set_int_custom(priv);drivers/net/can/pch_can.c new file +6
-0+ +/* Restore Run Mode */ +pch_can_set_run_mode (priv,PCH_CAN_RUN ); + +return retval; +}Are the suspend and resume functions tested?
Yes, we tested before.
Quoted 5 lines
=========================================
Except the above, we are modifying for your indications.
I will resubmit soon.
Thanks,
Wolfgang.
Hi Wolfgang,
Quoted 4 lines · Wolfgang Grandegger
I disagree.
I think it can be avoided easily.
I disagree.
I will modify like you are saying.
Quoted 3 lines
I don't understand! The Last Error Code (LEC) can have values from 0 to
7. A "switch" statement is therefore the right choice. Or have I missed something.
Yes, you are right.
I miss-understood.
Thanks, Ohtake(OKISemi)
----- Original Message -----
From: "Wolfgang Grandegger" <[email protected]>
To: "Masayuki Ohtake" <masa-korg-ECg8zkTtlr0C6LszWs/[email protected]>
Cc: <[email protected]>; "Tomoya MORINAGA" <morinaga526-ECg8zkTtlr0C6LszWs/[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; "Samuel Ortiz" <sameo-VuQAYsv1563Yd54FQh9/[email protected]>; "Barry Song" <[email protected]>; "Christian Pellegrin" <[email protected]>; "Wolfram Sang" <[email protected]>; "David S. Miller" <davem-fT/PcQaiUtIeIZ0/[email protected]>
Sent: Wednesday, October 13, 2010 8:08 PM
Subject: Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
Quoted 130 lines · Masayuki Ohtake
Quoted 15 lines · Wolfgang Grandegger
Quoted 14 lines
+
iowrite32(num, &(priv->regs)->if2_creq);
+ while (counter) {Quoted 9 lines
+
if2_creq= (ioread32(&(priv->regs)->if2_creq)) &drivers/net/can/pch_can.c new file +8
-0+CAN_IF_CREQ_BUSY ; +if (!if2_creq) +break ; + counter--; + } +if (!counter) +dev_err (&priv->ndev->dev,"IF2 BUSY Flag is set forever.\n" ); +}Quoted 4 lines
Quoted 1 line
Duplicated code!
No.
These are not the same.Of course they are not the same. The only difference is the register offset (of if1 or if2). A common function with a pointer to the if register as argument makes sense.
Quoted 2 lines
Though it is possible to integrate to one function by adding parameter, I think, current two function method is more easily to read.
I disagree.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
+ if (status &
PCH_STUF_ERR)
+ cf->data[2] |=CAN_ERR_PROT_STUFF;
+
+ if (status &PCH_FORM_ERR)
+ cf->data[2] |=CAN_ERR_PROT_FORM;Quoted 14 lines
Quoted 4 lines
drivers/net/can/pch_can.c new file +4
-0+ +if (status &PCH_ACK_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_ACK |CAN_ERR_PROT_LOC_ACK_DEL ; +drivers/net/can/pch_can.c line 879
Quoted 10 lines
drivers/net/can/pch_can.c new file +10
-0+if ((status &PCH_BIT1_ERR ) || (status &PCH_BIT0_ERR )) + cf->data[2 ] |=CAN_ERR_PROT_BIT ; + +if (status &PCH_CRC_ERR ) + cf->data[2 ] |=CAN_ERR_PROT_LOC_CRC_SEQ | +CAN_ERR_PROT_LOC_CRC_DEL ; + +if (status &PCH_LEC_ALL ) +iowrite32 (status |PCH_LEC_ALL , + &(priv->regs)->stat);Well, if status==7 (
PCH_LEC_ALL), all of the above conditions are true as well... convinced now?Quoted 6 lines
Quoted 2 lines
A bit-wise test of the above values is wrong, I believe. Please use the switch statement instead.
The above conditions are not only one time.
I think "switch" is not suitable for the above.
Thus, current "if" processing is better.I don't understand! The Last Error Code (LEC) can have values from 0 to
7. A "switch" statement is therefore the right choice. Or have I missed something.Quoted 8 lines
Quoted 8 lines
+ u32 brp;
+
+pch_can_get_run_mode(priv, &curr_mode);
+ if (curr_mode==PCH_CAN_RUN)
+pch_can_set_run_mode(priv, PCH_CAN_STOP);Quoted 4 lines
Quoted 1 line
The device is stopped when this function is called. Please remove.
No.
The above is necessary.Yes, because you started the device *too early* in
pch_can_open()called bypch_open(). See my other related comments of my previous mail.Quoted 2 lines
Because this is our HW specification.
Before setting bitrate, run-mode must be "STOP".I think it can be avoided easily.
Quoted 8 lines
Quoted 8 lines
drivers/net/can/pch_can.c new file +5
-0+static netdev_tx_t pch_xmit (struct sk_buff *skb,struct net_device *ndev) +{ +canid_t id; +u32 id1 =0 ; +u32 id2 =0 ;Quoted 4 lines
Quoted 1 line
Need these values to be preset?
These values are not essential.
But these help a engineer to read this code.I disagree.
Quoted 13 lines
Quoted 11 lines
+ /* Enable CAN Interrupts */
+pch_can_set_int_custom(priv);drivers/net/can/pch_can.c new file +6
-0+ +/* Restore Run Mode */ +pch_can_set_run_mode (priv,PCH_CAN_RUN ); + +return retval; +}Are the suspend and resume functions tested?
Yes, we tested before.
Quoted 5 lines
=========================================
Except the above, we are modifying for your indications.
I will resubmit soon.
Thanks,
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html