pruss CAN driver.
Subsystems (3)
v4 cover pruss CAN driver.
This patch adds a SocketCAN complaint CAN Network driver implemented on the Programmable Real Time Unit (PRUSS) available on da8xx processors from Texas Instruments.
It utilizes both available PRUs on da8xx for concurrent Rx/Tx processing and load sharing. PRU0 is used as Rx and PRU1 as Tx.
This controller also supports ID filtering at the device level. Eight different ID can be programmed into the device through the sysfs interface exported by the driver.
version 4:
=========
* added NAPI
* added sysfs entry for programming mailbox IDs
* added bittiming interface from the kernel
* removed unused function
* improved coding style
* removed unwanted files
* merged required files into the driver
* code cleanup as per review comments
NOTE: The versioning is being maintained in sequence with the complete set
of patches submitted for the PRUSS.
Subhasish Ghosh (1):
can: add pruss CAN driver.
drivers/net/can/Kconfig | 7 + drivers/net/can/Makefile | 1 + drivers/net/can/pruss_can.c | 1074 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1082 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pruss_can.c
Signature · 1 line
1.7.2.3
PATCH v4 1/1 can: add pruss CAN driver.
This patch adds support for the CAN device emulated on PRUSS.
--- drivers/net/can/Kconfig | 7 + drivers/net/can/Makefile | 1 + drivers/net/can/pruss_can.c | 1074 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1082 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pruss_can.c
drivers/net/can/Kconfig +7 -0
@@ -111,6 +111,13 @@ config PCH_CAN embedded processor. This driver can access CAN bus. +config CAN_TI_DA8XX_PRU + depends on CAN_DEV && ARCH_DAVINCI && ARCH_DAVINCI_DA850 + tristate "PRU based CAN emulation for DA8XX" + ---help--- + Enable this to emulate a CAN controller on the PRU of DA8XX. + If not sure, mark N + source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"
drivers/net/can/Makefile +1 -0
@@ -15,6 +15,7 @@ obj-$(CONFIG_CAN_SJA1000) += sja1000/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_AT91) += at91_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o +obj-$(CONFIG_CAN_TI_DA8XX_PRU) += pruss_can.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o obj-$(CONFIG_CAN_BFIN) += bfin_can.o obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
drivers/net/can/pruss_can.c new file +1074 -0
1,074 lines not on this page · raw message
Signature · 1 line
1.7.2.3
A message that is not in the archive (sent off-list or not yet received): 1303474267-6344-2-git-send-email-subhasish-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/@public.gmane.org
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 1 line · Subhasish Ghosh
This patch adds support for the CAN device emulated on PRUSS.
After commenting the code inline, some remarks:
- Your tx path looks broken, see commits inline
- Please setup a proper struct to describe your register layout, make use of arrays for rx and tx
- don't use u32, s32 for not hardware related variables like return codes and loop counter.
- the routines that load and save the can data bytes from/into your mailbox look way to complicated. Please write down the layout so that we can think of a elegant way to do it
- a lot of functions unconditionally return 0, make them void if no error can happen
- think about using managed devices, that would simplify the probe and release function
Quoted 14 lines
Signed-off-by Subhasish Ghosh <[email protected]>--- drivers/net/can/Kconfig | 7 + drivers/net/can/Makefile | 1 + drivers/net/can/pruss_can.c | 1074 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1082 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pruss_can.cdrivers/net/can/Kconfig +0
-0
drivers/net/can/Kconfig config PCH_CAN · line 119
Quoted 9 lines
drivers/net/can/Kconfig +6
-0@@ -111,6 +111,13 @@ config PCH_CAN embedded processor. This driver can access CAN bus. +config CAN_TI_DA8XX_PRU + depends on CAN_DEV && ARCH_DAVINCI && ARCH_DAVINCI_DA850 + tristate "PRU based CAN emulation for DA8XX" + ---help--- + Enable this to emulate a CAN controller on the PRU of DA8XX. + If not sure, mark N
Please indent the help text with 1 tab and 2 spaces
Quoted 62 lines
drivers/net/can/Kconfig +1
-0+ source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"drivers/net/can/Makefile +1
-0@@ -15,6 +15,7 @@ obj-$(CONFIG_CAN_SJA1000) += sja1000/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_AT91) += at91_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o +obj-$(CONFIG_CAN_TI_DA8XX_PRU) += pruss_can.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o obj-$(CONFIG_CAN_BFIN) += bfin_can.o obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.odrivers/net/can/pruss_can.c new file +40
-0@@ -0,0 +1,1074 @@ +/* +* TI DA8XX PRU CAN Emulation device driver +* Author: subhasish at mistralsolutions.com +* +* This driver supports TI's PRU CAN Emulation and the +* specs for the same is available at <http://www.ti.com> +* +* Copyright (C) 2010, 2011 Texas Instruments Incorporated +* +* 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. +* +* This program is distributed as is WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/bitops.h> +#include <linux/interrupt.h> +#include <linux/errno.h> +#include <linux/netdevice.h> +#include <linux/skbuff.h> +#include <linux/platform_device.h> +#include <linux/firmware.h> +#include <linux/clk.h> +#include <linux/types.h> +#include <linux/sysfs.h> +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> +#include <linux/mfd/pruss.h> + +#define PRUSS_CAN_RX_PRU_0 PRUSS_NUM0 +#define PRUSS_CAN_TX_PRU_1 PRUSS_NUM1 +#define PRUSS_CAN_TX_SYS_EVT 34
drivers/net/can/pruss_can.c line 50
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+#define PRUSS_CAN_RX_SYS_EVT 33 +#define PRUSS_CAN_ARM2DSP_SYS_EVT 32 +#define PRUSS_CAN_DELAY_LOOP_LENGTH 2 +#define PRUSS_CAN_TIMER_SETUP_DELAY 14 +#define PRUSS_CAN_GPIO_SETUP_DELAY 150 +#define PRUSS_CAN_TX_GBL_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x04) +#define PRUSS_CAN_TX_INTR_MASK_REG (PRUSS_PRU1_BASE_ADDRESS + 0x08) +#define PRUSS_CAN_TX_INTR_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x0C) +#define PRUSS_CAN_TX_MBOX0_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x10) +#define PRUSS_CAN_TX_ERR_CNTR_REG (PRUSS_PRU1_BASE_ADDRESS + 0x30)
I think Wolfgang and me have asked you to describe the register layout with a struct.
Quoted 32 lines
drivers/net/can/pruss_can.c new file +32
-0+#define PRUSS_CAN_TX_INT_STAT 0x2 +#define PRUSS_CAN_MBOX_OFFSET 0x40 +#define PRUSS_CAN_MBOX_SIZE 0x10 +#define PRUSS_CAN_MBOX_STAT_OFFSET 0x10 +#define PRUSS_CAN_MBOX_STAT_SIZE 0x04 +#define PRUSS_CAN_GBL_STAT_REG_VAL 0x00000040 +#define PRUSS_CAN_INTR_MASK_REG_VAL 0x00004000 +#define PRUSS_CAN_TIMING_VAL_TX (PRUSS_PRU1_BASE_ADDRESS + 0xC0) +#define PRUSS_CAN_TIMING_SETUP (PRUSS_PRU1_BASE_ADDRESS + 0xC4) +#define PRUSS_CAN_RX_GBL_STAT_REG (PRUSS_PRU0_BASE_ADDRESS + 0x04) +#define PRUSS_CAN_RX_INTR_MASK_REG (PRUSS_PRU0_BASE_ADDRESS + 0x08) +#define PRUSS_CAN_RX_INTR_STAT_REG (PRUSS_PRU0_BASE_ADDRESS + 0x0C) +#define PRUSS_CAN_RX_ERR_CNTR_REG (PRUSS_PRU0_BASE_ADDRESS + 0x34) +#define PRUSS_CAN_TIMING_VAL_RX (PRUSS_PRU0_BASE_ADDRESS + 0xD0) +#define PRUSS_CAN_BIT_TIMING_VAL_RX (PRUSS_PRU0_BASE_ADDRESS + 0xD4) +#define PRUSS_CAN_ID_MAP (PRUSS_PRU0_BASE_ADDRESS + 0xF0) +#define PRUSS_CAN_ERROR_ACTIVE 128 +#define PRUSS_CAN_GBL_STAT_REG_MASK 0x1F +#define PRUSS_CAN_GBL_STAT_REG_SET_TX 0x80 +#define PRUSS_CAN_GBL_STAT_REG_SET_RX 0x40 +#define PRUSS_CAN_GBL_STAT_REG_STOP_TX 0x7F +#define PRUSS_CAN_GBL_STAT_REG_STOP_RX 0xBF +#define PRUSS_CAN_SET_TX_REQ 0x00000080 +#define PRUSS_CAN_STD_FRAME_MASK 18 +#define PRUSS_CAN_START 1 +#define PRUSS_CAN_MB_MIN 0 +#define PRUSS_CAN_MB_MAX 7 +#define PRUSS_CAN_MID_IDE BIT(29) +#define PRUSS_CAN_ISR_BIT_CCI BIT(15) +#define PRUSS_CAN_ISR_BIT_ESI BIT(14) +#define PRUSS_CAN_ISR_BIT_SRDI BIT(13) +#define PRUSS_CAN_ISR_BIT_RRI BIT(8)
drivers/net/can/pruss_can.c line 92
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+#define PRUSS_CAN_GSR_BIT_EPM BIT(4) +#define PRUSS_CAN_GSR_BIT_BFM BIT(3) +#define PRUSS_CAN_RTR_BUFF_NUM 8 +#define PRUSS_DEF_NAPI_WEIGHT 8 +#define PRUSS_CAN_DRV_NAME "da8xx_pruss_can" +#define PRUSS_CAN_DRV_DESC "TI PRU CAN Controller Driver v1.0" + +enum can_tx_dir { +TRANSMIT =1 , +RECEIVE
please add a "," after RECEIVE
drivers/net/can/pruss_can.c line 95
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+}; + +struct can_mbox {
I suggest to add a namespace to these structs, e.g. pru_can_mbox. Same comment applies to the other structs.
drivers/net/can/pruss_can.c line 96
Quoted 1 line
drivers/net/can/pruss_can.c new file +1
-0+canid_t can_id;
You write this struct into the hardware, don't you? So you should not use kernel internal types to describe your hardware layout. Think about declaring this struct __packed__
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+u8 data[8 ]; +u16 datalength; +u16 crc; +}; + +struct can_emu_cntx { +enum can_tx_dir txdir; +struct can_mbox mbox; +u32 mboxno; +u8 pruno; +u32 gbl_stat; +u32 intr_stat; +u32 mbox_stat;
drivers/net/can/pruss_can.c line 119
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+}; + +struct can_emu_priv { +struct can_priv can; +struct napi_struct napi; +struct net_device *ndev; +struct device *dev; +struct clk *clk_timer; +struct can_emu_cntx can_tx_cntx; +struct can_emu_cntx can_rx_cntx;
I have not looked at the rest of the code, but it smells that you should make this an array of two cntx.
Quoted 14 lines
drivers/net/can/pruss_can.c new file +14
-0+unsigned int clk_freq_pru; +unsigned int trx_irq; +unsigned int tx_head; +unsigned int tx_tail; +unsigned int tx_next; +unsigned int mbx_id[8 ]; +}; + +static struct can_bittiming_const pru_can_bittiming_const = { + .name =PRUSS_CAN_DRV_NAME , + .tseg1_min =1 , + .tseg1_max =16 , + .tseg2_min =1 , + .tseg2_max =8 ,
drivers/net/can/pruss_can.c line 143
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ .sjw_max = 4, + .brp_min = 1, + .brp_max = 256, + .brp_inc = 1, +}; + +static int pru_can_mbox_write (struct device *dev, +struct can_emu_cntx *emu_cntx) +{ +u32 offset =0 ;
^^^^^
not needed
drivers/net/can/pruss_can.c line 156
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+ + offset =PRUSS_PRU1_BASE_ADDRESS +PRUSS_CAN_MBOX_OFFSET + + (PRUSS_CAN_MBOX_SIZE * emu_cntx->mboxno); + +pruss_writel_multi (dev, offset, (u32 *) &(emu_cntx->mbox),4 ); + +return 0 ; +} + +static int pru_can_mbox_read (struct device *dev, +struct can_emu_cntx *emu_cntx) +{ +u32 offset =0 ;
dito
drivers/net/can/pruss_can.c line 161
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ + offset =PRUSS_PRU0_BASE_ADDRESS +PRUSS_CAN_MBOX_OFFSET + + (PRUSS_CAN_MBOX_SIZE * emu_cntx->mboxno); + +pruss_readl_multi (dev, offset, (u32 *) &emu_cntx->mbox,4 );
where does this "4" come from? consider using sizeof()
drivers/net/can/pruss_can.c line 163
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ +return 0 ;
why do you return 0 here? pruss_readl_multi is not void, although it always returns 0, too. consider make all void.
drivers/net/can/pruss_can.c line 170
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+} + +static int pru_can_rx_id_set (struct device *dev,u32 nodeid,u32 mboxno) +{ +pruss_writel (dev, (PRUSS_CAN_ID_MAP + (mboxno *4 )), nodeid); + +return 0 ;
consider making this a void function.
drivers/net/can/pruss_can.c line 181
Quoted 11 lines
drivers/net/can/pruss_can.c new file +11
-0+} + +static int pru_can_intr_stat_get (struct device *dev, +struct can_emu_cntx *emu_cntx) +{ +if (emu_cntx->pruno ==PRUCORE_1 ) +pruss_readl (dev,PRUSS_CAN_TX_INTR_STAT_REG , + &emu_cntx->intr_stat); +else if (emu_cntx->pruno ==PRUCORE_0 ) +pruss_readl (dev,PRUSS_CAN_RX_INTR_STAT_REG , + &emu_cntx->intr_stat);
If you describe the register layout with a struct with an array containing with rx and tx registers you can get rid of the if..else.. use emu_cntx->pruno as index to the array.
drivers/net/can/pruss_can.c line 183
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+else +return -EINVAL ;
It's an internally used function, if emu_cntx->pruno is bogous here you've got really big problems. I think it's save to remove this. Then this function would become a void function.
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ +return 0 ; +} + +static int pru_can_gbl_stat_get (struct device *dev, +struct can_emu_cntx *emu_cntx)
drivers/net/can/pruss_can.c line 199
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+{ +if (emu_cntx->pruno ==PRUCORE_1 ) +pruss_readl (dev,PRUSS_CAN_TX_GBL_STAT_REG , + &emu_cntx->gbl_stat); +else if (emu_cntx->pruno ==PRUCORE_0 ) +pruss_readl (dev,PRUSS_CAN_RX_GBL_STAT_REG , + &emu_cntx->gbl_stat); +else +return -EINVAL ; +return 0 ;
Same comments apply here, too.
Quoted 18 lines
drivers/net/can/pruss_can.c new file +18
-0+} + +static int pru_can_tx_mode_set (struct device *dev,bool transfer_flag, +enum can_tx_dir ecan_trx) +{ +u32 value; + +if (ecan_trx ==TRANSMIT ) { +pruss_readl (dev,PRUSS_CAN_RX_GBL_STAT_REG , &value); +if (transfer_flag) { + value &=PRUSS_CAN_GBL_STAT_REG_MASK ; + value |=PRUSS_CAN_GBL_STAT_REG_SET_TX ; + }else + value &=PRUSS_CAN_GBL_STAT_REG_STOP_TX ; + +pruss_writel (dev,PRUSS_CAN_RX_GBL_STAT_REG , value); +pruss_writel (dev,PRUSS_CAN_TX_GBL_STAT_REG , value); + }else if (ecan_trx ==RECEIVE ) {
drivers/net/can/pruss_can.c line 227
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+pruss_readl (dev,PRUSS_CAN_RX_GBL_STAT_REG , &value); +if (transfer_flag) { + value &=PRUSS_CAN_GBL_STAT_REG_MASK ; + value |=PRUSS_CAN_GBL_STAT_REG_SET_RX ; + }else + value &=PRUSS_CAN_GBL_STAT_REG_STOP_RX ; + +pruss_writel (dev,PRUSS_CAN_RX_GBL_STAT_REG , value); +pruss_writel (dev,PRUSS_CAN_TX_GBL_STAT_REG , value); + }else
Same comments apply here, too.
drivers/net/can/pruss_can.c line 187
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+return -EINVAL ; + +return 0 ; +} +
is this array const?
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static u32 pruss_intc_init[19 ][3 ] = { + {PRUSS_INTC_POLARITY0 ,PRU_INTC_REGMAP_MASK ,0xFFFFFFFF }, + {PRUSS_INTC_POLARITY1 ,PRU_INTC_REGMAP_MASK ,0xFFFFFFFF }, + {PRUSS_INTC_TYPE0 ,PRU_INTC_REGMAP_MASK ,0x1C000000 }, + {PRUSS_INTC_TYPE1 ,PRU_INTC_REGMAP_MASK ,0 }, + {PRUSS_INTC_GLBLEN ,0 ,1 }, + {PRUSS_INTC_HOSTMAP0 ,PRU_INTC_REGMAP_MASK ,0x03020100 }, + {PRUSS_INTC_HOSTMAP1 ,PRU_INTC_REGMAP_MASK ,0x07060504 }, + {PRUSS_INTC_HOSTMAP2 ,PRU_INTC_REGMAP_MASK ,0x0000908 }, + {PRUSS_INTC_CHANMAP0 ,PRU_INTC_REGMAP_MASK ,0 },
drivers/net/can/pruss_can.c line 252
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ {PRUSS_INTC_CHANMAP8, PRU_INTC_REGMAP_MASK, 0x00020200}, + {PRUSS_INTC_STATIDXCLR, 0, 32}, + {PRUSS_INTC_STATIDXCLR, 0, 19}, + {PRUSS_INTC_ENIDXSET, 0, 19}, + {PRUSS_INTC_STATIDXCLR, 0, 18}, + {PRUSS_INTC_ENIDXSET, 0, 18}, + {PRUSS_INTC_STATIDXCLR, 0, 34}, + {PRUSS_INTC_ENIDXSET, 0, 34}, + {PRUSS_INTC_ENIDXSET, 0, 32}, + {PRUSS_INTC_HOSTINTEN, 0, 5}
please add ","
drivers/net/can/pruss_can.c line 258
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+}; + +static int pru_can_emu_init (struct device *dev,u32 pruclock) +{ +u32 value[8 ] = {[0 ...7 ] =1 }; +u32 i;
we usually use plain ints as a for-loop variable.
drivers/net/can/pruss_can.c line 261
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+ +/* PRU Internal Registers Initializations */ +pruss_writel_multi (dev,PRUSS_CAN_TX_MBOX0_STAT_REG , value,8 );
use sizeof(), or ARRAY_SIZE
drivers/net/can/pruss_can.c line 265
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+ + *value =PRUSS_CAN_GBL_STAT_REG_VAL ; +pruss_writel (dev,PRUSS_CAN_TX_GBL_STAT_REG , value[0 ]); +pruss_writel (dev,PRUSS_CAN_RX_GBL_STAT_REG , value[0 ]);
why not:pruss_writel(dev, PRUSS_CAN_TX_GBL_STAT_REG, PRUSS_CAN_GBL_STAT_REG_VAL);
drivers/net/can/pruss_can.c line 271
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ + *value =PRUSS_CAN_INTR_MASK_REG_VAL ; +pruss_writel (dev,PRUSS_CAN_TX_INTR_MASK_REG , value[0 ]); +pruss_writel (dev,PRUSS_CAN_RX_INTR_MASK_REG , value[0 ]); + +for (i =0 ; i <19 ; i++)
ARRAY_SIZE
drivers/net/can/pruss_can.c line 276
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ (i <12 ) ?pruss_rmwl (dev, pruss_intc_init[i][0 ], + pruss_intc_init[i][1 ], + pruss_intc_init[i][2 ]) : +pruss_idx_writel (dev, pruss_intc_init[i][0 ], + pruss_intc_init[i][2 ]);
if..else here, please
or put the stuff into two arrays
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+return 0 ; +} + +static void pru_can_emu_exit (struct device *dev)
drivers/net/can/pruss_can.c line 290
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+{ +pruss_disable (dev,PRUSS_CAN_RX_PRU_0 ); +pruss_disable (dev,PRUSS_CAN_TX_PRU_1 ); +} + +static int pru_can_tx (struct device *dev,u8 mboxnumber,u8 pruno) +{ +u32 value =0 ; + +if (PRUCORE_1 == pruno) {
we usually write it the other way round...:
if (pruno == PRUCORE_1)
drivers/net/can/pruss_can.c line 295
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ value =PRUSS_CAN_SET_TX_REQ ; +pruss_writel (dev, ((PRUSS_PRU1_BASE_ADDRESS + + (PRUSS_CAN_MBOX_STAT_OFFSET + + (PRUSS_CAN_MBOX_STAT_SIZE * mboxnumber))) + &0xFFFF ), value);
don't use value, use PRUSS_CAN_SET_TX_REQ directly
drivers/net/can/pruss_can.c line 304
Quoted 9 lines
drivers/net/can/pruss_can.c new file +9
-0+ }else if (PRUCORE_0 == pruno) { +pruss_readl (dev,PRUSS_CAN_RX_INTR_STAT_REG , &value); + value = value & ~(1 << mboxnumber); +pruss_writel (dev,PRUSS_CAN_RX_INTR_STAT_REG , value); + value =0 ; +pruss_writel (dev, ((PRUSS_PRU0_BASE_ADDRESS + + (PRUSS_CAN_MBOX_STAT_OFFSET + + (PRUSS_CAN_MBOX_STAT_SIZE * mboxnumber))) + &0xFFFF ), value);
same here
drivers/net/can/pruss_can.c line 306
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ }else +return -EINVAL ;
trust your own code, get rid of the -EINVAL, make this a void function.
drivers/net/can/pruss_can.c line 315
Quoted 9 lines
drivers/net/can/pruss_can.c new file +9
-0+return 0 ; +} + +static int pru_can_reset_tx (struct device *dev) +{ +pruss_idx_writel (dev,PRUSS_INTC_STATIDXCLR ,PRUSS_CAN_ARM2DSP_SYS_EVT ); +pruss_idx_writel (dev,PRUSS_INTC_ENIDXSET ,PRUSS_CAN_ARM2DSP_SYS_EVT ); +pruss_idx_writel (dev,PRUSS_INTC_STATIDXSET ,PRUSS_CAN_ARM2DSP_SYS_EVT ); +return 0 ;
void function?
drivers/net/can/pruss_can.c line 320
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+} + +static void pru_can_mask_ints (struct device *dev,u32 trx,bool enable) +{ +u32 value =0 ;
not needed
drivers/net/can/pruss_can.c line 323
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+ + value = (trx ==PRUSS_CAN_TX_PRU_1 ) ? +PRUSS_CAN_TX_SYS_EVT :PRUSS_CAN_RX_SYS_EVT ;
use a struct with arrays for the register description
drivers/net/can/pruss_can.c line 325
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ enable ?pruss_idx_writel (dev,PRUSS_INTC_ENIDXSET , value) : +pruss_idx_writel (dev,PRUSS_INTC_ENIDXCLR , value);
if..else
drivers/net/can/pruss_can.c line 330
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+} + +static unsigned int pru_can_get_error_cnt (struct device *dev,u8 pruno) +{ +u32 value =0 ;
not needed
drivers/net/can/pruss_can.c line 337
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ +if (pruno ==PRUSS_CAN_RX_PRU_0 ) +pruss_readl (dev,PRUSS_CAN_RX_ERR_CNTR_REG , &value); +else if (pruno ==PRUSS_CAN_TX_PRU_1 ) +pruss_readl (dev,PRUSS_CAN_TX_ERR_CNTR_REG , &value); +else +return -EINVAL ;
remove the -EINVAL
drivers/net/can/pruss_can.c line 344
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ +return value; +} + +static unsigned int pru_can_get_intc_status (struct device *dev) +{ +u32 value =0 ;
not needed
drivers/net/can/pruss_can.c line 352
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ +pruss_readl (dev,PRUSS_INTC_STATCLRINT1 , &value); +return value; +} + +static void pru_can_clr_intc_status (struct device *dev,u32 trx) +{ +u32 value =0 ;
dito
drivers/net/can/pruss_can.c line 323
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+ + value = (trx ==PRUSS_CAN_TX_PRU_1 ) ? +PRUSS_CAN_TX_SYS_EVT :PRUSS_CAN_RX_SYS_EVT ;
use a struct + array for the resiter desc
drivers/net/can/pruss_can.c line 363
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+pruss_idx_writel (dev,PRUSS_INTC_STATIDXCLR , value); +} + +static int pru_can_get_state (const struct net_device *ndev, +enum can_state *state) +{ +struct can_emu_priv *priv =netdev_priv (ndev); + *state = priv->can.state;
we don't implemnt this function anymore..
Quoted 31 lines
drivers/net/can/pruss_can.c new file +31
-0+ +return 0 ; +} + +static int pru_can_set_bittiming (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct can_bittiming *bt = &priv->can.bittiming; +u32 value; + + value = priv->can.clock.freq / bt->bitrate; +pruss_writel (priv->dev,PRUSS_CAN_TIMING_VAL_TX , value); +pruss_writel (priv->dev,PRUSS_CAN_BIT_TIMING_VAL_RX , value); + + value = (bt->phase_seg2 + bt->phase_seg1 + + bt->prop_seg +1 ) * bt->brp; + value = (value >>1 ) -PRUSS_CAN_TIMER_SETUP_DELAY ; + value = (value <<16 ) | value; +pruss_writel (priv->dev,PRUSS_CAN_TIMING_VAL_RX , value); + + value = (PRUSS_CAN_GPIO_SETUP_DELAY * + (priv->clk_freq_pru /1000000 ) /1000 ) / +PRUSS_CAN_DELAY_LOOP_LENGTH ; + +pruss_writel (priv->dev,PRUSS_CAN_TIMING_SETUP , value); +return 0 ; +} + +static void pru_can_stop (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev);
drivers/net/can/pruss_can.c line 404
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , false); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +pru_can_reset_tx (priv->dev); + priv->can.state =CAN_STATE_STOPPED ; +} + +/* +* This is to just set the can state to ERROR_ACTIVE +* ip link set canX up type can bitrate 125000
fix the comment
Quoted 42 lines
drivers/net/can/pruss_can.c new file +42
-0+*/ +static void pru_can_start (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); + +if (priv->can.state !=CAN_STATE_STOPPED ) +pru_can_stop (ndev); + +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , true); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , true); + +pru_can_gbl_stat_get (priv->dev, &priv->can_tx_cntx); +pru_can_gbl_stat_get (priv->dev, &priv->can_rx_cntx); + +if (PRUSS_CAN_GSR_BIT_EPM & priv->can_tx_cntx.gbl_stat) + priv->can.state =CAN_STATE_ERROR_PASSIVE ; +else if (PRUSS_CAN_GSR_BIT_BFM & priv->can_tx_cntx.gbl_stat) + priv->can.state =CAN_STATE_BUS_OFF ; +else + priv->can.state =CAN_STATE_ERROR_ACTIVE ; +} + +static int pru_can_set_mode (struct net_device *ndev,enum can_mode mode) +{ +int ret =0 ; + +switch (mode) { +case CAN_MODE_START : +pru_can_start (ndev); +netif_wake_queue (ndev); +break ; +default : + ret = -EOPNOTSUPP ; +break ; + } +return ret; +} + +static netdev_tx_t pru_can_start_xmit (struct sk_buff *skb, +struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev);
drivers/net/can/pruss_can.c line 456
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+struct can_frame *cf = (struct can_frame *)skb->data; +int count; +u8 *data = cf->data; +u8 dlc = cf->can_dlc; +u8 *pdata =NULL ; + +if (can_dropped_invalid_skb (ndev, skb)) +return NETDEV_TX_OK ; + +netif_stop_queue (ndev);
why do you stop the queue unconditionally here?
drivers/net/can/pruss_can.c line 469
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+if (cf->can_id &CAN_EFF_FLAG )/* Extended frame format */ + priv->can_tx_cntx.mbox.can_id = + (cf->can_id &CAN_EFF_MASK ) |PRUSS_CAN_MID_IDE ; +else /* Standard frame format */ + priv->can_tx_cntx.mbox.can_id = + (cf->can_id &CAN_SFF_MASK ) <<PRUSS_CAN_STD_FRAME_MASK ; + +if (cf->can_id &CAN_RTR_FLAG )/* Remote transmission request */ + priv->can_tx_cntx.mbox.can_id |=CAN_RTR_FLAG ; + + pdata = &priv->can_tx_cntx.mbox.data[0 ] + (dlc -1 ); +for (count =0 ; count < (u8 ) dlc; count++) + *pdata-- = *data++;
What does this loop do? endianess conversion? Please don't open code this.
drivers/net/can/pruss_can.c line 471
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ + priv->can_tx_cntx.mbox.datalength = (u16 ) dlc;
no need to cast
drivers/net/can/pruss_can.c line 480
Quoted 9 lines
drivers/net/can/pruss_can.c new file +9
-0+ priv->can_tx_cntx.mbox.crc =0 ; +/* +* search for the next available mbx +* if the next mbx is busy, then try the next + 1 +* do this until the head is reached. +* if still unable to tx, stop accepting any packets +* if able to tx and the head is reached, then reset next to tail, i.e mbx0 +* if head is not reached, then just point to the next mbx +*/
indention, please
Your tx algorithm looks fishy. You always use can_get_echo_skb(ndev, 0). This means you can have only 1 can frame on the fly. But you say you look for a free mailbox. You have to tx mailboxes in a defined order, otherwise your hardware/firmware is broken. If your hardware transmits frames in order, you always know which one will be the next free mailbox. You have a power of 2 number of mailboxes, you can simply apply a mask to get to the real mailbox number. No need for manual wrap around. Have a look at the at91_can tx sheme.
Activate the tx_interrupt, putting a can frame into a mailbox, stop the tx_queue if there are no free mailboxes, or in case of a wrap around. Reenable the tx_queue in the tx_complete interrupt handler.
drivers/net/can/pruss_can.c line 484
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+for (; priv->tx_next <= priv->tx_head; priv->tx_next++) { + priv->can_tx_cntx.mboxno = priv->tx_next; +if (-1 ==pru_can_mbox_write (priv->dev, + &priv->can_tx_cntx)) {
this function will always return 0.
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+if (priv->tx_next == priv->tx_head) { + priv->tx_next = priv->tx_tail; +netif_stop_queue (ndev);/* IF stalled */ +dev_err (priv->dev, +"%s: no tx mbx available" , __func__); +return NETDEV_TX_BUSY ; + }else
drivers/net/can/pruss_can.c line 501
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+continue ; + }else { +/* set transmit request */ +pru_can_tx (priv->dev, priv->tx_next, +PRUSS_CAN_TX_PRU_1 ); +pru_can_tx_mode_set (priv->dev, false,RECEIVE ); +pru_can_tx_mode_set (priv->dev, true,TRANSMIT ); +pru_can_reset_tx (priv->dev); + priv->tx_next++; +can_put_echo_skb (skb, ndev,0 );
^^^
see comment above
Quoted 20 lines
drivers/net/can/pruss_can.c new file +20
-0+break ; + } + } +if (priv->tx_next > priv->tx_head) + priv->tx_next = priv->tx_tail; + +return NETDEV_TX_OK ; +} + +static int pru_can_rx (struct net_device *ndev,u32 mbxno) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; +u8 *data =NULL ; +u8 *pdata =NULL ; +int count =0 ; + + skb =alloc_can_skb (ndev, &cf);
drivers/net/can/pruss_can.c line 531
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+if (!skb) { +if (printk_ratelimit ()) +dev_err (priv->dev, +"alloc_can_skb() failed\n" ); +return -ENOMEM ; + } + data = cf->data; +/* get payload */ + priv->can_rx_cntx.mboxno = mbxno; +if (pru_can_mbox_read (priv->dev, &priv->can_rx_cntx)) {
function always returns 0!
drivers/net/can/pruss_can.c line 539
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+dev_err (priv->dev,"failed to get data from mailbox\n" ); +return -EAGAIN ; + } +/* give ownweship to pru */ +pru_can_tx (priv->dev, mbxno,PRUSS_CAN_RX_PRU_0 ); + +/* get data length code */ + cf->can_dlc =get_can_dlc (priv->can_rx_cntx.mbox.datalength &0xF );
This looks to complicated. Please state how the individual can bytes are placed in the mailbox, so that we can think of a simpler way to do this.
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+if (cf->can_dlc <=4 ) { + pdata = &priv->can_rx_cntx.mbox.data[4 ] + (4 - cf->can_dlc); +for (count =0 ; count < cf->can_dlc; count++) + *data++ = *pdata++; + }else { + pdata = &priv->can_rx_cntx.mbox.data[4 ]; +for (count =0 ; count <4 ; count++)
drivers/net/can/pruss_can.c line 556
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ *data++ = *pdata++; + pdata = &priv->can_rx_cntx.mbox.data[3 ] - (cf->can_dlc -5 ); +for (count =0 ; count < cf->can_dlc -4 ; count++) + *data++ = *pdata++; + } + +/* get id extended or std */ +if (priv->can_rx_cntx.mbox.can_id &PRUSS_CAN_MID_IDE ) + cf->can_id = (priv->can_rx_cntx.mbox.can_id &CAN_EFF_MASK ) + |CAN_EFF_FLAG ;
the usual way is to write the "|" at the end of the line.
drivers/net/can/pruss_can.c line 562
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+else + cf->can_id = (priv->can_rx_cntx.mbox.can_id >>18 ) + &CAN_SFF_MASK ; + +if (priv->can_rx_cntx.mbox.can_id &CAN_RTR_FLAG ) + cf->can_id |=CAN_RTR_FLAG ;
please don't copy any data to the can frame in case if an RTR message.
Quoted 112 lines
drivers/net/can/pruss_can.c new file +112
-0+ + stats->rx_bytes += cf->can_dlc; +netif_receive_skb (skb); + stats->rx_packets++; +return 0 ; +} + +static int pru_can_err (struct net_device *ndev,int int_status, +int err_status) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; +u32 tx_err_cnt, rx_err_cnt; + + skb =alloc_can_err_skb (ndev, &cf); +if (!skb) { +if (printk_ratelimit ()) +dev_err (priv->dev, +"alloc_can_err_skb() failed\n" ); +return -ENOMEM ; + } + +if (err_status &PRUSS_CAN_GSR_BIT_EPM ) {/* error passive int */ + priv->can.state =CAN_STATE_ERROR_PASSIVE ; + ++priv->can.can_stats.error_passive; + cf->can_id |=CAN_ERR_CRTL ; + tx_err_cnt =pru_can_get_error_cnt (priv->dev, +PRUSS_CAN_TX_PRU_1 ); + rx_err_cnt =pru_can_get_error_cnt (priv->dev, +PRUSS_CAN_RX_PRU_0 ); +if (tx_err_cnt >PRUSS_CAN_ERROR_ACTIVE -1 ) + cf->data[1 ] |=CAN_ERR_CRTL_TX_PASSIVE ; +if (rx_err_cnt >PRUSS_CAN_ERROR_ACTIVE -1 ) + cf->data[1 ] |=CAN_ERR_CRTL_RX_PASSIVE ; + +dev_dbg (priv->ndev->dev.parent,"Error passive interrupt\n" ); + } + +if (err_status &PRUSS_CAN_GSR_BIT_BFM ) { + priv->can.state =CAN_STATE_BUS_OFF ; + cf->can_id |=CAN_ERR_BUSOFF ; +/* +* Disable all interrupts in bus-off to avoid int hog +* this should be handled by the pru +*/ +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , false); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +can_bus_off (ndev); +dev_dbg (priv->ndev->dev.parent,"Bus off mode\n" ); + } + +netif_rx (skb); + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +return 0 ; +} + +static int pru_can_rx_poll (struct napi_struct *napi,int quota) +{ +struct net_device *ndev = napi->dev; +struct can_emu_priv *priv =netdev_priv (ndev); +u32 bit_set, mbxno =0 ; +u32 num_pkts =0 ; + +if (!netif_running (ndev)) +return 0 ; + +do { +/* rx int sys_evt -> 33 */ +pru_can_clr_intc_status (priv->dev,PRUSS_CAN_RX_PRU_0 ); +if (pru_can_intr_stat_get (priv->dev, &priv->can_rx_cntx)) +return num_pkts; + +if (PRUSS_CAN_ISR_BIT_RRI & priv->can_rx_cntx.intr_stat) { + mbxno =PRUSS_CAN_RTR_BUFF_NUM ; +pru_can_rx (ndev, mbxno); + num_pkts++; + }else { +/* Extract the mboxno from the status */ + bit_set =fls (priv->can_rx_cntx.intr_stat &0xFF ); +if (bit_set) { + num_pkts++; + mbxno = bit_set -1 ; +if (PRUSS_CAN_ISR_BIT_ESI & priv->can_rx_cntx. + intr_stat) { +pru_can_gbl_stat_get (priv->dev, + &priv->can_rx_cntx); +pru_can_err (ndev, + priv->can_rx_cntx.intr_stat, + priv->can_rx_cntx.gbl_stat); + }else +pru_can_rx (ndev, mbxno); + }else +break ; + } + }while (((PRUSS_CAN_TX_INT_STAT &pru_can_get_intc_status (priv->dev)) + && (num_pkts < quota))); + +/* Enable packet interrupt if all pkts are handled */ +if (!(PRUSS_CAN_TX_INT_STAT &pru_can_get_intc_status (priv->dev))) { +napi_complete (napi); +/* Re-enable RX mailbox interrupts */ +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , true); + } +return num_pkts; +} + +static irqreturn_t pru_tx_can_intr (int irq,void *dev_id) +{ +struct net_device *ndev = dev_id;
drivers/net/can/pruss_can.c line 684
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +u32 bit_set, mbxno; + +pru_can_intr_stat_get (priv->dev, &priv->can_tx_cntx); +if ((PRUSS_CAN_ISR_BIT_CCI & priv->can_tx_cntx.intr_stat) + || (PRUSS_CAN_ISR_BIT_SRDI & priv->can_tx_cntx.intr_stat)) { +dev_dbg (priv->ndev->dev.parent,"tx_int_status = 0x%X\n" , + priv->can_tx_cntx.intr_stat); +can_free_echo_skb (ndev,0 );
^^^
make no sense if using multiple tx mailboxes
drivers/net/can/pruss_can.c line 690
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ }else { + bit_set =fls (priv->can_tx_cntx.intr_stat &0xFF ); +if (!bit_set) { +dev_err (priv->dev,"%s: invalid mailbox number\n" , + __func__); +can_free_echo_skb (ndev,0 );
^^^^
drivers/net/can/pruss_can.c line 703
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+ }else { + mbxno = bit_set -1 ; +if (PRUSS_CAN_ISR_BIT_ESI & priv->can_tx_cntx. + intr_stat) { +/* read gsr and ack pru */ +pru_can_gbl_stat_get (priv->dev, + &priv->can_tx_cntx); +pru_can_err (ndev, priv->can_tx_cntx.intr_stat, + priv->can_tx_cntx.gbl_stat); + }else { + stats->tx_packets++; +/* stats->tx_bytes += dlc; */ +/*can_get_echo_skb(ndev, 0);*/
??
drivers/net/can/pruss_can.c line 708
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ } + } + } +netif_wake_queue (ndev); +can_get_echo_skb (ndev,0 );
again?
drivers/net/can/pruss_can.c line 713
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+pru_can_tx_mode_set (priv->dev, true,RECEIVE ); +return IRQ_HANDLED ; +} + +static irqreturn_t pru_rx_can_intr (int irq,void *dev_id)
why is this function calles rx_can_intr it's a generic interrupt function..
drivers/net/can/pruss_can.c line 724
Quoted 11 lines
drivers/net/can/pruss_can.c new file +11
-0+{ +struct net_device *ndev = dev_id; +struct can_emu_priv *priv =netdev_priv (ndev); +u32 intc_status =0 ; + + intc_status =pru_can_get_intc_status (priv->dev); + +/* tx int sys_evt -> 34 */ +if (intc_status &4 ) { +pru_can_clr_intc_status (priv->dev,PRUSS_CAN_TX_PRU_1 ); +return pru_tx_can_intr (irq, dev_id);
why are you returning here? is is possible the you have a can frame to receivce?
drivers/net/can/pruss_can.c line 732
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ } +/* Disable RX mailbox interrupts and let NAPI reenable them */ +if (intc_status &2 ) { +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +napi_schedule (&priv->napi); + } + +return IRQ_HANDLED ;
^^^^^^^^^^^^
that might not be true....
drivers/net/can/pruss_can.c line 742
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+} + +static int pru_can_open (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +s32 err; + +/* register interrupt handler */ + err =request_irq (priv->trx_irq, &pru_rx_can_intr,IRQF_SHARED , +"pru_can_irq" , ndev);
use dev->name for interrupt name
Quoted 61 lines
drivers/net/can/pruss_can.c new file +61
-0+if (err) { +dev_err (priv->dev,"error requesting rx interrupt\n" ); + goto exit_trx_irq; + } + err =open_candev (ndev); +if (err) { +dev_err (priv->dev,"open_candev() failed %d\n" , err); + goto exit_open; + } + +pru_can_emu_init (priv->dev, priv->can.clock.freq); + priv->tx_tail =PRUSS_CAN_MB_MIN ; + priv->tx_head =PRUSS_CAN_MB_MAX ; +pru_can_start (ndev); +napi_enable (&priv->napi); +netif_start_queue (ndev); +return 0 ; + +exit_open: +free_irq (priv->trx_irq, ndev); +exit_trx_irq: +return err; +} + +static int pru_can_close (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); + +netif_stop_queue (ndev); +napi_disable (&priv->napi); +close_candev (ndev); +free_irq (priv->trx_irq, ndev); +return 0 ; +} + +static const struct net_device_ops pru_can_netdev_ops = { + .ndo_open = pru_can_open, + .ndo_stop = pru_can_close, + .ndo_start_xmit = pru_can_start_xmit, +}; + +/* Shows all the mailbox IDs */ +static ssize_t pru_sysfs_mbx_id_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct can_emu_priv *priv =netdev_priv (to_net_dev (dev)); + +return snprintf (buf,PAGE_SIZE ,"<mbx_no:mbx_id>\n" +"0:0x%X 1:0x%X 2:0x%X 3:0x%X " +"4:0x%X 5:0x%X 6:0x%X 7:0x%X\n" , + priv->mbx_id[0 ], priv->mbx_id[1 ], + priv->mbx_id[2 ], priv->mbx_id[3 ], + priv->mbx_id[4 ], priv->mbx_id[5 ], + priv->mbx_id[6 ], priv->mbx_id[7 ]); +} + +/* +* Sets Mailbox IDs +* This should be programmed as mbx_num:mbx_id (in hex) +* eg: $ echo 0:0x123 > /sys/class/net/can0/mbx_id +*/
drivers/net/can/pruss_can.c line 813
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static ssize_t pru_sysfs_mbx_id_set (struct device *dev, +struct device_attribute *attr,const char *buf,size_t count) +{ +struct net_device *ndev =to_net_dev (dev); +struct can_emu_priv *priv =netdev_priv (ndev); +unsigned long can_id; +unsigned long mbx_num; +char mbx[2 ] = {*buf,'\0' };/* mbx num */ +ssize_t ret = count; +s32 err;
I think you have to lock here:rtnl_lock();
Quoted 49 lines
drivers/net/can/pruss_can.c new file +49
-0+ +if (ndev->flags &IFF_UP ) { + ret = -EBUSY ; + goto out; + } + +if (*(buf +1 ) !=':' ) { + ret = -EINVAL ; + goto out; + } + + err =strict_strtoul (mbx,0 , &mbx_num); +if (err) { + ret = err; + goto out; + } + +if (mbx_num >7 ) { + ret = -EINVAL ; + goto out; + } + + err =strict_strtoul ((buf +2 ),0 , &can_id); +if (err) { + ret = err; + goto out; + } + + priv->mbx_id[mbx_num] = can_id; +pru_can_rx_id_set (priv->dev, priv->mbx_id[mbx_num], mbx_num); + +return ret; +out: +dev_err (priv->dev,"invalid buffer format\n" ); +return ret; +} + +static DEVICE_ATTR(mbx_id, S_IWUSR | S_IRUGO, + pru_sysfs_mbx_id_show, pru_sysfs_mbx_id_set); + +static struct attribute *pru_sysfs_attrs[] = { + &dev_attr_mbx_id.attr, +NULL , +}; + +static struct attribute_group pru_sysfs_attr_group = { + .attrs = pru_sysfs_attrs, +}; +
drivers/net/can/pruss_can.c line 872
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static int __devinit pru_can_probe (struct platform_device *pdev) +{ +struct net_device *ndev =NULL ; +const struct da850_evm_pruss_can_data *pdata; +struct can_emu_priv *priv =NULL ; +struct device *dev = &pdev->dev; +struct clk *clk_pruss; +const struct firmware *fw_rx; +const struct firmware *fw_tx; +u32 err;
use int
drivers/net/can/pruss_can.c line 879
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ + pdata = dev->platform_data; +if (!pdata) { +dev_err (&pdev->dev,"platform data not found\n" ); +return -EINVAL ; + } + (pdata->setup)();
no need fot the ( )
Quoted 22 lines
drivers/net/can/pruss_can.c new file +22
-0+ + ndev =alloc_candev (sizeof (struct can_emu_priv ),PRUSS_CAN_MB_MAX +1 ); +if (!ndev) { +dev_err (&pdev->dev,"alloc_candev failed\n" ); + err = -ENOMEM ; + goto probe_exit; + } + + ndev->sysfs_groups[0 ] = &pru_sysfs_attr_group; + + priv =netdev_priv (ndev); + + priv->trx_irq =platform_get_irq (to_platform_device (dev->parent),0 ); +if (!priv->trx_irq) { +dev_err (&pdev->dev,"unable to get pru " +"interrupt resources!\n" ); + err = -ENODEV ; + goto probe_exit; + } + + priv->ndev = ndev; + priv->dev = dev;
drivers/net/can/pruss_can.c line 911
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ + priv->can.bittiming_const = &pru_can_bittiming_const; + priv->can.do_set_bittiming = pru_can_set_bittiming; + priv->can.do_set_mode = pru_can_set_mode; + priv->can.do_get_state = pru_can_get_state; + priv->can_tx_cntx.pruno =PRUSS_CAN_TX_PRU_1 ; + priv->can_rx_cntx.pruno =PRUSS_CAN_RX_PRU_0 ; + +/* we support local echo, no arp */ + ndev->flags |= (IFF_ECHO |IFF_NOARP );
no need to se NOARP
Quoted 14 lines
drivers/net/can/pruss_can.c new file +14
-0+ +/* pdev->dev->device_private->driver_data = ndev */ +platform_set_drvdata (pdev, ndev); +SET_NETDEV_DEV (ndev, &pdev->dev); + ndev->netdev_ops = &pru_can_netdev_ops; + + priv->clk_timer =clk_get (&pdev->dev,"pll1_sysclk2" ); +if (IS_ERR (priv->clk_timer)) { +dev_err (&pdev->dev,"no timer clock available\n" ); + err =PTR_ERR (priv->clk_timer); + priv->clk_timer =NULL ; + goto probe_exit_candev; + } +
drivers/net/can/pruss_can.c line 935
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ priv->can.clock.freq =clk_get_rate (priv->clk_timer); + + clk_pruss =clk_get (NULL ,"pruss" ); +if (IS_ERR (clk_pruss)) { +dev_err (&pdev->dev,"no clock available: pruss\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + priv->clk_freq_pru =clk_get_rate (clk_pruss); +clk_put (clk_pruss);
why do you put the clock here?
Quoted 23 lines
drivers/net/can/pruss_can.c new file +23
-0+ + err =register_candev (ndev); +if (err) { +dev_err (&pdev->dev,"register_candev() failed\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + + err =request_firmware (&fw_tx,"PRU_CAN_Emulation_Tx.bin" , + &pdev->dev); +if (err) { +dev_err (&pdev->dev,"can't load firmware\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + +dev_info (&pdev->dev,"fw_tx size %d. downloading...\n" , fw_tx->size); + + err =request_firmware (&fw_rx,"PRU_CAN_Emulation_Rx.bin" , + &pdev->dev); +if (err) { +dev_err (&pdev->dev,"can't load firmware\n" ); + err = -ENODEV ;
drivers/net/can/pruss_can.c line 968
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ goto probe_release_fw; + } +dev_info (&pdev->dev,"fw_rx size %d. downloading...\n" , fw_rx->size); + +/* init the pru */ +pru_can_emu_init (priv->dev, priv->can.clock.freq); +udelay (200 ); + +netif_napi_add (ndev, &priv->napi, pru_can_rx_poll, +PRUSS_DEF_NAPI_WEIGHT );
personally I'd wait to add the interface to napi until the firmware is loaded.
Quoted 96 lines
drivers/net/can/pruss_can.c new file +96
-0+ +pruss_enable (priv->dev,PRUSS_CAN_RX_PRU_0 ); +pruss_enable (priv->dev,PRUSS_CAN_TX_PRU_1 ); + +/* download firmware into pru */ + err =pruss_load (priv->dev,PRUSS_CAN_RX_PRU_0 , + (u32 *)fw_rx->data, (fw_rx->size /4 )); +if (err) { +dev_err (&pdev->dev,"firmware download error\n" ); + err = -ENODEV ; + goto probe_release_fw_1; + } +release_firmware (fw_rx); + err =pruss_load (priv->dev,PRUSS_CAN_TX_PRU_1 , + (u32 *)fw_tx->data, (fw_tx->size /4 )); +if (err) { +dev_err (&pdev->dev,"firmware download error\n" ); + err = -ENODEV ; + goto probe_release_fw_1; + } +release_firmware (fw_tx); + +pruss_run (priv->dev,PRUSS_CAN_RX_PRU_0 ); +pruss_run (priv->dev,PRUSS_CAN_TX_PRU_1 ); + +dev_info (&pdev->dev, +"%s device registered (trx_irq = %d, clk = %d)\n" , +PRUSS_CAN_DRV_NAME , priv->trx_irq, priv->can.clock.freq); + +return 0 ; + +probe_release_fw_1: +release_firmware (fw_rx); +probe_release_fw: +release_firmware (fw_tx); +probe_exit_clk: +clk_put (priv->clk_timer); +probe_exit_candev: +if (NULL != ndev) +free_candev (ndev); +probe_exit: +return err; +} + +static int __devexit pru_can_remove(struct platform_device *pdev) +{ +struct net_device *ndev =platform_get_drvdata (pdev); +struct can_emu_priv *priv =netdev_priv (ndev); + +pru_can_stop (ndev); +pru_can_emu_exit (priv->dev); +clk_put (priv->clk_timer); +unregister_candev (ndev); +free_candev (ndev); +platform_set_drvdata (pdev,NULL ); +return 0 ; +} + +#ifdef CONFIG_PM +static int pru_can_suspend (struct platform_device *pdev, +pm_message_t mesg) +{ +dev_info (&pdev->dev,"%s not yet implemented\n" , __func__); +return 0 ; +} + +static int pru_can_resume (struct platform_device *pdev) +{ +dev_info (&pdev->dev,"%s not yet implemented\n" , __func__); +return 0 ; +} +#else +#define pru_can_suspend NULL +#define pru_can_resume NULL +#endif + +static struct platform_driver omapl_pru_can_driver = { + .probe = pru_can_probe, + .remove =__devexit_p (pru_can_remove), + .suspend = pru_can_suspend, + .resume = pru_can_resume, + .driver = { + .name =PRUSS_CAN_DRV_NAME , + .owner =THIS_MODULE , + }, +}; + +static int __init pru_can_init (void ) +{ + pr_debug(KERN_INFO PRUSS_CAN_DRV_DESC "\n"); + return platform_driver_register(&omapl_pru_can_driver); +} + +module_init (pru_can_init); + +static void __exit pru_can_exit(void)
drivers/net/can/pruss_can.c line 1074
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+{ + pr_debug(KERN_INFO PRUSS_CAN_DRV_DESC " unloaded\n"); +platform_driver_unregister (&omapl_pru_can_driver); +} + +module_exit (pru_can_exit); + +MODULE_AUTHOR ("Subhasish Ghosh <[email protected]>" ); +MODULE_LICENSE ("GPL" ); +MODULE_DESCRIPTION ("omapl pru CAN netdevice driver" );
regards, Marc
Signature · 12 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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110422/bd2fdced/attachment-0001.sig>
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Hi,
Quoted 16 lines · Marc Kleine-Budde
Quoted 1 line · Subhasish Ghosh
This patch adds support for the CAN device emulated on PRUSS.
After commenting the code inline, some remarks:
- Your tx path looks broken, see commits inline
- Please setup a proper struct to describe your register layout, make use of arrays for rx and tx
- don't use u32, s32 for not hardware related variables like return codes and loop counter.
- the routines that load and save the can data bytes from/into your mailbox look way to complicated. Please write down the layout so that we can think of a elegant way to do it
- a lot of functions unconditionally return 0, make them void if no error can happen
- think about using managed devices, that would simplify the probe and release function
I agree with Marc's comments and would like to add:
- Use just *one* value per sysfs file
A few more comments inline...
Quoted 46 lines
Quoted 13 lines
Signed-off-by Subhasish Ghosh <[email protected]>--- drivers/net/can/Kconfig | 7 + drivers/net/can/Makefile | 1 + drivers/net/can/pruss_can.c | 1074 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1082 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pruss_can.cdrivers/net/can/Kconfig +0
-0drivers/net/can/Kconfig config PCH_CAN · line 119
Quoted 9 lines
drivers/net/can/Kconfig +6
-0@@ -111,6 +111,13 @@ config PCH_CAN embedded processor. This driver can access CAN bus. +config CAN_TI_DA8XX_PRU + depends on CAN_DEV && ARCH_DAVINCI && ARCH_DAVINCI_DA850 + tristate "PRU based CAN emulation for DA8XX" + ---help--- + Enable this to emulate a CAN controller on the PRU of DA8XX. + If not sure, mark NPlease indent the help text with 1 tab and 2 spaces
Quoted 21 lines
drivers/net/can/Kconfig +1
-0+ source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"drivers/net/can/Makefile +1
-0@@ -15,6 +15,7 @@ obj-$(CONFIG_CAN_SJA1000) += sja1000/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_AT91) += at91_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o +obj-$(CONFIG_CAN_TI_DA8XX_PRU) += pruss_can.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o obj-$(CONFIG_CAN_BFIN) += bfin_can.o obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.odrivers/net/can/pruss_can.c new file +0
-0
...
Quoted 15 lines
is this array const?
Quoted 14 lines
drivers/net/can/pruss_can.c new file +14
-0+static u32 pruss_intc_init[19 ][3 ] = { + {PRUSS_INTC_POLARITY0 ,PRU_INTC_REGMAP_MASK ,0xFFFFFFFF }, + {PRUSS_INTC_POLARITY1 ,PRU_INTC_REGMAP_MASK ,0xFFFFFFFF }, + {PRUSS_INTC_TYPE0 ,PRU_INTC_REGMAP_MASK ,0x1C000000 }, + {PRUSS_INTC_TYPE1 ,PRU_INTC_REGMAP_MASK ,0 }, + {PRUSS_INTC_GLBLEN ,0 ,1 }, + {PRUSS_INTC_HOSTMAP0 ,PRU_INTC_REGMAP_MASK ,0x03020100 }, + {PRUSS_INTC_HOSTMAP1 ,PRU_INTC_REGMAP_MASK ,0x07060504 }, + {PRUSS_INTC_HOSTMAP2 ,PRU_INTC_REGMAP_MASK ,0x0000908 }, + {PRUSS_INTC_CHANMAP0 ,PRU_INTC_REGMAP_MASK ,0 }, + {PRUSS_INTC_CHANMAP8 ,PRU_INTC_REGMAP_MASK ,0x00020200 }, + {PRUSS_INTC_STATIDXCLR ,0 ,32 }, + {PRUSS_INTC_STATIDXCLR ,0 ,19 }, + {PRUSS_INTC_ENIDXSET ,0 ,19 },
Quoted 8 lines
drivers/net/can/pruss_can.c line 252
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ {PRUSS_INTC_STATIDXCLR, 0, 18}, + {PRUSS_INTC_ENIDXSET, 0, 18}, + {PRUSS_INTC_STATIDXCLR, 0, 34}, + {PRUSS_INTC_ENIDXSET, 0, 34}, + {PRUSS_INTC_ENIDXSET, 0, 32}, + {PRUSS_INTC_HOSTINTEN, 0, 5}please add ","
Also a struct to describe each entry would improve readability. Then you could also use ARRAY_SIZE.
Quoted 1 line
Quoted 1 line
+};
...
Quoted 11 lines
Quoted 11 lines
drivers/net/can/pruss_can.c new file +11
-0+static int pru_can_set_bittiming (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct can_bittiming *bt = &priv->can.bittiming; +u32 value; + + value = priv->can.clock.freq / bt->bitrate; +pruss_writel (priv->dev,PRUSS_CAN_TIMING_VAL_TX , value); +pruss_writel (priv->dev,PRUSS_CAN_BIT_TIMING_VAL_RX , value); + + value = (bt->phase_seg2 + bt->phase_seg1 +
Quoted 8 lines
drivers/net/can/pruss_can.c line 386
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ bt->prop_seg + 1) * bt->brp; + value = (value >>1 ) -PRUSS_CAN_TIMER_SETUP_DELAY ; + value = (value <<16 ) | value; +pruss_writel (priv->dev,PRUSS_CAN_TIMING_VAL_RX , value); + + value = (PRUSS_CAN_GPIO_SETUP_DELAY * + (priv->clk_freq_pru /1000000 ) /1000 ) / +PRUSS_CAN_DELAY_LOOP_LENGTH ;
This calculation looks delicate. 64-bit math would be safer.
Quoted 4 lines
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+ +pruss_writel (priv->dev,PRUSS_CAN_TIMING_SETUP , value); +return 0 ; +}
...
Quoted 39 lines
Quoted 39 lines
drivers/net/can/pruss_can.c new file +39
-0+static int pru_can_err (struct net_device *ndev,int int_status, +int err_status) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; +u32 tx_err_cnt, rx_err_cnt; + + skb =alloc_can_err_skb (ndev, &cf); +if (!skb) { +if (printk_ratelimit ()) +dev_err (priv->dev, +"alloc_can_err_skb() failed\n" ); +return -ENOMEM ; + } + +if (err_status &PRUSS_CAN_GSR_BIT_EPM ) {/* error passive int */ + priv->can.state =CAN_STATE_ERROR_PASSIVE ; + ++priv->can.can_stats.error_passive; + cf->can_id |=CAN_ERR_CRTL ; + tx_err_cnt =pru_can_get_error_cnt (priv->dev, +PRUSS_CAN_TX_PRU_1 ); + rx_err_cnt =pru_can_get_error_cnt (priv->dev, +PRUSS_CAN_RX_PRU_0 ); +if (tx_err_cnt >PRUSS_CAN_ERROR_ACTIVE -1 ) + cf->data[1 ] |=CAN_ERR_CRTL_TX_PASSIVE ; +if (rx_err_cnt >PRUSS_CAN_ERROR_ACTIVE -1 ) + cf->data[1 ] |=CAN_ERR_CRTL_RX_PASSIVE ; + +dev_dbg (priv->ndev->dev.parent,"Error passive interrupt\n" ); + } + +if (err_status &PRUSS_CAN_GSR_BIT_BFM ) { + priv->can.state =CAN_STATE_BUS_OFF ; + cf->can_id |=CAN_ERR_BUSOFF ; +/* +* Disable all interrupts in bus-off to avoid int hog +* this should be handled by the pru
Quoted 8 lines
drivers/net/can/pruss_can.c line 616
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+*/ +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , false); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +can_bus_off (ndev); +dev_dbg (priv->ndev->dev.parent,"Bus off mode\n" ); + } + +netif_rx (skb);
You should use netif_receive_skb(skb) here as well.
Quoted 25 lines
Quoted 25 lines
drivers/net/can/pruss_can.c new file +25
-0+ stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +return 0 ; +} + +static int pru_can_rx_poll (struct napi_struct *napi,int quota) +{ +struct net_device *ndev = napi->dev; +struct can_emu_priv *priv =netdev_priv (ndev); +u32 bit_set, mbxno =0 ; +u32 num_pkts =0 ; + +if (!netif_running (ndev)) +return 0 ; + + do { + /* rx int sys_evt -> 33 */ + pru_can_clr_intc_status(priv->dev, PRUSS_CAN_RX_PRU_0); + if (pru_can_intr_stat_get(priv->dev, &priv->can_rx_cntx)) + return num_pkts; + + if (PRUSS_CAN_ISR_BIT_RRI & priv->can_rx_cntx.intr_stat) { + mbxno = PRUSS_CAN_RTR_BUFF_NUM; + pru_can_rx(ndev, mbxno); + num_pkts++;
Quoted 8 lines
drivers/net/can/pruss_can.c line 649
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ }else { +/* Extract the mboxno from the status */ + bit_set =fls (priv->can_rx_cntx.intr_stat &0xFF ); +if (bit_set) { + num_pkts++; + mbxno = bit_set -1 ; +if (PRUSS_CAN_ISR_BIT_ESI & priv->can_rx_cntx. + intr_stat) {
if (PRUSS_CAN_ISR_BIT_ESI &
priv->can_rx_cntx.intr_stat) {Is more readable.
Quoted 5 lines
drivers/net/can/pruss_can.c line 654
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+pru_can_gbl_stat_get (priv->dev, + &priv->can_rx_cntx); +pru_can_err (ndev, + priv->can_rx_cntx.intr_stat, + priv->can_rx_cntx.gbl_stat);
Please fix bogous indention.
Quoted 16 lines
Quoted 16 lines
drivers/net/can/pruss_can.c new file +16
-0+ }else +pru_can_rx (ndev, mbxno); + }else +break ; + } + }while (((PRUSS_CAN_TX_INT_STAT &pru_can_get_intc_status (priv->dev)) + && (num_pkts < quota))); + +/* Enable packet interrupt if all pkts are handled */ +if (!(PRUSS_CAN_TX_INT_STAT &pru_can_get_intc_status (priv->dev))) { +napi_complete (napi); +/* Re-enable RX mailbox interrupts */ +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , true); + } +return num_pkts; +}
...
Quoted 6 lines
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+/* Shows all the mailbox IDs */ +static ssize_t pru_sysfs_mbx_id_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct can_emu_priv *priv =netdev_priv (to_net_dev (dev)); +
Quoted 8 lines
drivers/net/can/pruss_can.c line 797
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+return snprintf (buf,PAGE_SIZE ,"<mbx_no:mbx_id>\n" +"0:0x%X 1:0x%X 2:0x%X 3:0x%X " +"4:0x%X 5:0x%X 6:0x%X 7:0x%X\n" , + priv->mbx_id[0 ], priv->mbx_id[1 ], + priv->mbx_id[2 ], priv->mbx_id[3 ], + priv->mbx_id[4 ], priv->mbx_id[5 ], + priv->mbx_id[6 ], priv->mbx_id[7 ]); +}
As mentioned above, just one value per sysfs file, please...
Quoted 5 lines
Quoted 5 lines
+/*
+ * Sets Mailbox IDs
+ * This should be programmed as mbx_num:mbx_id (in hex)
+ * eg: $ echo 0:0x123 >/sys/class/net/can0/mbx_id
+ */
... which would also avoid string parsing.
Quoted 40 lines
drivers/net/can/pruss_can.c line 872
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static int __devinit pru_can_probe (struct platform_device *pdev) +{ +struct net_device *ndev =NULL ; +const struct da850_evm_pruss_can_data *pdata; +struct can_emu_priv *priv =NULL ; +struct device *dev = &pdev->dev; +struct clk *clk_pruss; +const struct firmware *fw_rx; +const struct firmware *fw_tx; +u32 err;use int
drivers/net/can/pruss_can.c line 879
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ + pdata = dev->platform_data; +if (!pdata) { +dev_err (&pdev->dev,"platform data not found\n" ); +return -EINVAL ; + } + (pdata->setup)();no need fot the ( )
Quoted 19 lines
drivers/net/can/pruss_can.c new file +19
-0+ + ndev =alloc_candev (sizeof (struct can_emu_priv ),PRUSS_CAN_MB_MAX +1 ); +if (!ndev) { +dev_err (&pdev->dev,"alloc_candev failed\n" ); + err = -ENOMEM ; + goto probe_exit; + } + + ndev->sysfs_groups[0 ] = &pru_sysfs_attr_group; + + priv =netdev_priv (ndev); + + priv->trx_irq =platform_get_irq (to_platform_device (dev->parent),0 ); +if (!priv->trx_irq) { +dev_err (&pdev->dev,"unable to get pru " +"interrupt resources!\n" ); + err = -ENODEV ; + goto probe_exit; + }
Quoted 8 lines
drivers/net/can/pruss_can.c line 906
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ + priv->ndev = ndev; + priv->dev = dev; + + priv->can.bittiming_const = &pru_can_bittiming_const; + priv->can.do_set_bittiming = pru_can_set_bittiming; + priv->can.do_set_mode = pru_can_set_mode; + priv->can.do_get_state = pru_can_get_state;
Please remove that callback. It's not needed as state changes are handled properly.
Quoted 106 lines
drivers/net/can/pruss_can.c line 911
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ priv->can_tx_cntx.pruno =PRUSS_CAN_TX_PRU_1 ; + priv->can_rx_cntx.pruno =PRUSS_CAN_RX_PRU_0 ; + +/* we support local echo, no arp */ + ndev->flags |= (IFF_ECHO |IFF_NOARP );no need to se NOARP
Quoted 14 lines
drivers/net/can/pruss_can.c new file +14
-0+ +/* pdev->dev->device_private->driver_data = ndev */ +platform_set_drvdata (pdev, ndev); +SET_NETDEV_DEV (ndev, &pdev->dev); + ndev->netdev_ops = &pru_can_netdev_ops; + + priv->clk_timer =clk_get (&pdev->dev,"pll1_sysclk2" ); +if (IS_ERR (priv->clk_timer)) { +dev_err (&pdev->dev,"no timer clock available\n" ); + err =PTR_ERR (priv->clk_timer); + priv->clk_timer =NULL ; + goto probe_exit_candev; + } +drivers/net/can/pruss_can.c line 935
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ priv->can.clock.freq =clk_get_rate (priv->clk_timer); + + clk_pruss =clk_get (NULL ,"pruss" ); +if (IS_ERR (clk_pruss)) { +dev_err (&pdev->dev,"no clock available: pruss\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + priv->clk_freq_pru =clk_get_rate (clk_pruss); +clk_put (clk_pruss);why do you put the clock here?
Quoted 23 lines
drivers/net/can/pruss_can.c new file +23
-0+ + err =register_candev (ndev); +if (err) { +dev_err (&pdev->dev,"register_candev() failed\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + + err =request_firmware (&fw_tx,"PRU_CAN_Emulation_Tx.bin" , + &pdev->dev); +if (err) { +dev_err (&pdev->dev,"can't load firmware\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + +dev_info (&pdev->dev,"fw_tx size %d. downloading...\n" , fw_tx->size); + + err =request_firmware (&fw_rx,"PRU_CAN_Emulation_Rx.bin" , + &pdev->dev); +if (err) { +dev_err (&pdev->dev,"can't load firmware\n" ); + err = -ENODEV ;drivers/net/can/pruss_can.c line 968
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ goto probe_release_fw; + } +dev_info (&pdev->dev,"fw_rx size %d. downloading...\n" , fw_rx->size); + +/* init the pru */ +pru_can_emu_init (priv->dev, priv->can.clock.freq); +udelay (200 ); + +netif_napi_add (ndev, &priv->napi, pru_can_rx_poll, +PRUSS_DEF_NAPI_WEIGHT );personally I'd wait to add the interface to napi until the firmware is loaded.
Quoted 35 lines
drivers/net/can/pruss_can.c new file +35
-0+ +pruss_enable (priv->dev,PRUSS_CAN_RX_PRU_0 ); +pruss_enable (priv->dev,PRUSS_CAN_TX_PRU_1 ); + +/* download firmware into pru */ + err =pruss_load (priv->dev,PRUSS_CAN_RX_PRU_0 , + (u32 *)fw_rx->data, (fw_rx->size /4 )); +if (err) { +dev_err (&pdev->dev,"firmware download error\n" ); + err = -ENODEV ; + goto probe_release_fw_1; + } +release_firmware (fw_rx); + err =pruss_load (priv->dev,PRUSS_CAN_TX_PRU_1 , + (u32 *)fw_tx->data, (fw_tx->size /4 )); +if (err) { +dev_err (&pdev->dev,"firmware download error\n" ); + err = -ENODEV ; + goto probe_release_fw_1; + } +release_firmware (fw_tx); + +pruss_run (priv->dev,PRUSS_CAN_RX_PRU_0 ); +pruss_run (priv->dev,PRUSS_CAN_TX_PRU_1 ); + +dev_info (&pdev->dev, +"%s device registered (trx_irq = %d, clk = %d)\n" , +PRUSS_CAN_DRV_NAME , priv->trx_irq, priv->can.clock.freq); + +return 0 ; + +probe_release_fw_1: +release_firmware (fw_rx); +probe_release_fw: +release_firmware (fw_tx);
Quoted 8 lines
drivers/net/can/pruss_can.c line 1011
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+probe_exit_clk: +clk_put (priv->clk_timer); +probe_exit_candev: +if (NULL != ndev) +free_candev (ndev); +probe_exit: +return err; +}
Thanks,
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 2 lines
- Use just *one* value per sysfs file
SG - I felt adding entry for each mbx_id will clutter the sysfs.
Is it ok to do that.
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
+static u32 pruss_intc_init[19][3] = {
+ {PRUSS_INTC_POLARITY0,PRU_INTC_REGMAP_MASK,0xFFFFFFFF},
+ {PRUSS_INTC_POLARITY1,PRU_INTC_REGMAP_MASK,0xFFFFFFFF},
+ {PRUSS_INTC_TYPE0,PRU_INTC_REGMAP_MASK,0x1C000000},
+ {PRUSS_INTC_TYPE1,PRU_INTC_REGMAP_MASK, 0},
+ {PRUSS_INTC_GLBLEN, 0, 1},
+ {PRUSS_INTC_HOSTMAP0,PRU_INTC_REGMAP_MASK,0x03020100},
+ {PRUSS_INTC_HOSTMAP1,PRU_INTC_REGMAP_MASK,0x07060504},
+ {PRUSS_INTC_HOSTMAP2,PRU_INTC_REGMAP_MASK,0x0000908},
+ {PRUSS_INTC_CHANMAP0,PRU_INTC_REGMAP_MASK, 0},
+ {PRUSS_INTC_CHANMAP8,PRU_INTC_REGMAP_MASK,0x00020200},
+ {PRUSS_INTC_STATIDXCLR, 0, 32},
+ {PRUSS_INTC_STATIDXCLR, 0, 19},
+ {PRUSS_INTC_ENIDXSET, 0, 19},
+ {PRUSS_INTC_STATIDXCLR, 0, 18},
+ {PRUSS_INTC_ENIDXSET, 0, 18},
+ {PRUSS_INTC_STATIDXCLR, 0, 34},
+ {PRUSS_INTC_ENIDXSET, 0, 34},
+ {PRUSS_INTC_ENIDXSET, 0, 32},
+ {PRUSS_INTC_HOSTINTEN, 0, 5}
Quoted 4 lines
Quoted 1 line
please add ","
Also a struct to describe each entry would improve readability. Then you could also use
ARRAY_SIZE.
SG _ I could not follow this, are you recommending that I create a structure with three variables and then create
an array for it.
something like:
const static struct [] = {
{
unsigned int reg_base;
unsigned int reg_mask;
unsigned int reg_val;
},
...
};
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
+ value = (
PRUSS_CAN_GPIO_SETUP_DELAY*
+ (priv->clk_freq_pru/ 1000000) / 1000) /
+PRUSS_CAN_DELAY_LOOP_LENGTH;This calculation looks delicate. 64-bit math would be safer.
SG - This one works fine. I am dividing it twice to avoid the problem.
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
+
pru_can_mask_ints(priv->dev, PRUSS_CAN_TX_PRU_1, false);
+pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, false);
+can_bus_off(ndev);
+dev_dbg(priv->ndev->dev.parent, "Bus off mode\n");
+ }
+
+netif_rx(skb);You should use
netif_receive_skb(skb)here as well.
SG - Ok, Will do.
Quoted 5 lines
if (
PRUSS_CAN_ISR_BIT_ESI&priv->can_rx_cntx.intr_stat) {Is more readable.
SG - Ok, Will do.
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
+ pru_can_gbl_stat_get(priv->dev,
+&priv->can_rx_cntx);
+ pru_can_err(ndev,
+priv->can_rx_cntx.intr_stat,
+priv->can_rx_cntx.gbl_stat);Please fix bogous indention.
SG - Ok, Will do.
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
+
+ pdata =dev->platform_data;
+ if (!pdata) {
+dev_err(&pdev->dev, "platform data not found\n");
+ return-EINVAL;
+ }
+ (pdata->setup)();no need fot the ( )
SG - Ok, Will do.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
+ }
+
+priv->ndev= ndev;
+priv->dev= dev;
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
+
+priv->can.bittiming_const=&pru_can_bittiming_const;
+priv->can.do_set_bittiming=pru_can_set_bittiming;
+priv->can.do_set_mode=pru_can_set_mode;
+priv->can.do_get_state=pru_can_get_state;Please remove that callback. It's not needed as state changes are handled properly.
SG -- Ok, Will do
Marc Kleine-Budde
maintainerRe: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 31 lines · Subhasish Ghosh
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
+static u32 pruss_intc_init[19][3] = {
+ {PRUSS_INTC_POLARITY0,PRU_INTC_REGMAP_MASK,0xFFFFFFFF},
+ {PRUSS_INTC_POLARITY1,PRU_INTC_REGMAP_MASK,0xFFFFFFFF},
+ {PRUSS_INTC_TYPE0,PRU_INTC_REGMAP_MASK,0x1C000000},
+ {PRUSS_INTC_TYPE1,PRU_INTC_REGMAP_MASK, 0},
+ {PRUSS_INTC_GLBLEN, 0, 1},
+ {PRUSS_INTC_HOSTMAP0,PRU_INTC_REGMAP_MASK,0x03020100},
+ {PRUSS_INTC_HOSTMAP1,PRU_INTC_REGMAP_MASK,0x07060504},
+ {PRUSS_INTC_HOSTMAP2,PRU_INTC_REGMAP_MASK,0x0000908},
+ {PRUSS_INTC_CHANMAP0,PRU_INTC_REGMAP_MASK, 0},
+ {PRUSS_INTC_CHANMAP8,PRU_INTC_REGMAP_MASK,0x00020200},
+ {PRUSS_INTC_STATIDXCLR, 0, 32},
+ {PRUSS_INTC_STATIDXCLR, 0, 19},
+ {PRUSS_INTC_ENIDXSET, 0, 19},
+ {PRUSS_INTC_STATIDXCLR, 0, 18},
+ {PRUSS_INTC_ENIDXSET, 0, 18},
+ {PRUSS_INTC_STATIDXCLR, 0, 34},
+ {PRUSS_INTC_ENIDXSET, 0, 34},
+ {PRUSS_INTC_ENIDXSET, 0, 32},
+ {PRUSS_INTC_HOSTINTEN, 0, 5}Quoted 4 lines
Quoted 1 line
please add ","
Also a struct to describe each entry would improve readability. Then you could also use
ARRAY_SIZE.SG _ I could not follow this, are you recommending that I create a structure with three variables and then create
an array for it.
something like:
Quoted 8 lines
const static struct [] = { { unsigned int reg_base; unsigned int reg_mask; unsigned int reg_val; },...
};
I think this isn't valid C. It should look like this:
struct pruss_intc_init {
unsigned long reg_base;
u32 reg_mask;
u32 reg+val;
};static const struct pruss_intc_init pruss_initc_init[] = {
{ .reg_base = 0xdeadbeef, .reg_mask = 0xaa, .reg_val = 0x55 },
...
};I'm not sure about the datatype of reg_base. I haven't looked at the code that uses this array.
cheers, Marc
Signature · 12 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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110427/899d2e60/attachment.sig>
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Subhasish GhoshQuoted 5 lines · Subhasish Ghosh
Quoted 2 lines
- Use just one value per sysfs file
SG - I felt adding entry for each
mbx_idwill clutter the sysfs.
Is it ok to do that.
That is probably not much better either.
Note also that every sysfs file needs to come with associated documentation in Documentation/ABI/*/ to make sure that users will know exactly how the file is meant to work.
Why do you need to export these values in the first place? Is it just for debugging or do you expect all CAN user space to look at this?
If it's for debugging, please don't export the files through sysfs. Depending on how useful the data is to regular users, you can still export it through a debugfs file in that case, which has much less strict rules.
If the file is instead meant as part of the regular operation of the device, it should not be in debugfs but probably be integrated into the CAN socket interface, so that users don't need to work with two different ways of getting to the device (socket and sysfs).
Arnd
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 22 lines · Subhasish Ghosh
Quoted 5 lines
Quoted 2 lines
- Use just one value per sysfs file
SG - I felt adding entry for each
mbx_idwill clutter the sysfs.
Is it ok to do that.That is probably not much better either.
Note also that every sysfs file needs to come with associated documentation in
Documentation/ABI/*/to make sure that users will know exactly how the file is meant to work.Why do you need to export these values in the first place? Is it just for debugging or do you expect all CAN user space to look at this?
If it's for debugging, please don't export the files through sysfs. Depending on how useful the data is to regular users, you can still export it through a debugfs file in that case, which has much less strict rules.
Quoted 5 lines
If the file is instead meant as part of the regular operation of the device, it should not be in debugfs but probably be integrated into the CAN socket interface, so that users don't need to work with two different ways of getting to the device (socket and sysfs).
CAN requires mail box IDs to be programmed in. But, the socket CAN subsystem supports only software filtering of the mail box IDs.
So, the mail box IDs programmed into socket CAN during initialization does not propagate into the hardware. This is planned to be a future implementation in Socket CAN.
In our case, we support hardware filtering, to work around with this, Wolfgang (Socket CAN owner) suggested that we implement this using sysfs.
These setting are not for debugging, but to program the mail box IDs into the hardware.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 7 lines · Subhasish Ghosh
CAN requires mail box IDs to be programmed in. But, the socket CAN subsystem supports only software filtering of the mail box IDs.
So, the mail box IDs programmed into socket CAN during initialization does not propagate into the hardware. This is planned to be a future implementation in Socket CAN.
Quoted 6 lines
In our case, we support hardware filtering, to work around with this, Wolfgang (Socket CAN owner) suggested that we implement this using sysfs.
These setting are not for debugging, but to program the mail box IDs into the hardware.
Ok, I see. Can you point me to that discussion?
Wolfgang, I'm a bit worried by the API being split between sockets and sysfs. The problem is that once the sysfs API is established, users will start relying on it, and you can no longer migrate away from it, even when a later version of the Socket CAN also supports setting through a different interface. What is the current interface to set mail box IDs in software? How hard would it be to implement that feature in Socket CAN?
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Arnd
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Hi Arnd,
Quoted 15 lines · Arnd Bergmann
Quoted 7 lines · Subhasish Ghosh
CAN requires mail box IDs to be programmed in. But, the socket CAN subsystem supports only software filtering of the mail box IDs.
So, the mail box IDs programmed into socket CAN during initialization does not propagate into the hardware. This is planned to be a future implementation in Socket CAN.
Quoted 6 lines
In our case, we support hardware filtering, to work around with this, Wolfgang (Socket CAN owner) suggested that we implement this using sysfs.
These setting are not for debugging, but to program the mail box IDs into the hardware.
Quoted 7 lines
Ok, I see. Can you point me to that discussion?
Wolfgang, I'm a bit worried by the API being split between sockets and sysfs. The problem is that once the sysfs API is established, users will start relying on it, and you can no longer migrate away from it, even when a later version of the Socket CAN also supports setting through a different interface. What is the current interface to set mail box IDs in software?
Note that this CAN controller is *very* special. It cannot handle all CAN id's due to a lack or resources. The PRUSS firmware is able to manage just up to 8 different CAN identifiers out of the usual 4096 (12-bit) or even more for the extended CAN ids using 29 bits. There is no other CAN controller with such rather serious limitations and therefore there exists also no appropriate interface. I think using sysfs is OK for such device-specific parameters, at least for the time being.
Quoted 1 line
How hard would it be to implement that feature in Socket CAN?
CAN controllers usually provide some kind of hardware CAN id filtering, but in a very hardware dependent way. A generic interface may be able to handle the PRUSS restrictions as well. CAN devices are usually configured through the netlink interface. e.g.
$ ip link set can0 up type can bitrate 125000
and such a common interface would be netlink based as well.
Quoted 2 lines
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Any ideas on how to handle hardware filtering in a generic way are welcome. I will try to come up with a proposal sooner than later.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 8 lines · Wolfgang Grandegger
Quoted 5 lines · Arnd Bergmann
Wolfgang, I'm a bit worried by the API being split between sockets and sysfs. The problem is that once the sysfs API is established, users will start relying on it, and you can no longer migrate away from it, even when a later version of the Socket CAN also supports setting through a different interface. What is the current interface to set mail box IDs in software?
Quoted 4 lines
Note that this CAN controller is *very* special. It cannot handle all CAN id's due to a lack or resources. The PRUSS firmware is able to manage just up to 8 different CAN identifiers out of the usual 4096 (12-bit) or even more for the extended CAN ids using 29 bits.
So for other controllers, they can simply access every ID within the range (12 or 29 bits), but there is no need to configure?
What are these IDs for? Do they identify a local port, a remote address, a connection, or something else?
Quoted 5 lines
There is
no other CAN controller with such rather serious limitations and therefore there exists also no appropriate interface. I think using sysfs is OK for such device-specific parameters, at least for the time being.
It sounds like it's not very scalable, especially since the implementation is done completely in firmware. Imagine a new firmware version suddenly supporting 256 ids instead of 8 -- you'd then have to create 256 sysfs files to be compatible if I understand you correctly.
Quoted 10 lines
Quoted 1 line
How hard would it be to implement that feature in Socket CAN?
CAN controllers usually provide some kind of hardware CAN id filtering, but in a very hardware dependent way. A generic interface may be able to handle the PRUSS restrictions as well. CAN devices are usually configured through the netlink interface. e.g.
$ ip link set can0 up type can bitrate 125000and such a common interface would be netlink based as well.
Agreed.
Quoted 5 lines
Quoted 2 lines
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Any ideas on how to handle hardware filtering in a generic way are welcome. I will try to come up with a proposal sooner than later.
It sounds to me like the best solution would be change the firmware to lift that restriction and simply allow all IDs, in case it's not actually a hardware limitation (which sounds unlikely).
If that's not possible, maybe it's possible to define a generic filtering interface using netlink, and then either do it completely in the kernel, or using the hardware support.
Arnd
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 9 lines · Arnd Bergmann
Quoted 8 lines · Wolfgang Grandegger
Quoted 5 lines · Arnd Bergmann
Wolfgang, I'm a bit worried by the API being split between sockets and sysfs. The problem is that once the sysfs API is established, users will start relying on it, and you can no longer migrate away from it, even when a later version of the Socket CAN also supports setting through a different interface. What is the current interface to set mail box IDs in software?
Quoted 7 lines
Quoted 4 lines
Note that this CAN controller is *very* special. It cannot handle all CAN id's due to a lack or resources. The PRUSS firmware is able to manage just up to 8 different CAN identifiers out of the usual 4096 (12-bit) or even more for the extended CAN ids using 29 bits.
So for other controllers, they can simply access every ID within the range (12 or 29 bits), but there is no need to configure?
Yes, 11 or 29 bits, to be correct.
Quoted 2 lines
What are these IDs for? Do they identify a local port, a remote address, a connection, or something else?
It's a message identifier, which is used for bus arbitration and which other CAN nodes can listen to. See also:
http://lxr.linux.no/#linux+v2.6.38/Documentation/networking/can.txt#L146
http://en.wikipedia.org/wiki/Controller_area_network
Quoted 10 lines
Quoted 5 lines
There is
no other CAN controller with such rather serious limitations and therefore there exists also no appropriate interface. I think using sysfs is OK for such device-specific parameters, at least for the time being.It sounds like it's not very scalable, especially since the implementation is done completely in firmware. Imagine a new firmware version suddenly supporting 256 ids instead of 8 -- you'd then have to create 256 sysfs files to be compatible if I understand you correctly.
Well, than an array of CAN identifiers per file would indeed be more appropriate.
Quoted 16 lines
Quoted 10 lines
Quoted 1 line
How hard would it be to implement that feature in Socket CAN?
CAN controllers usually provide some kind of hardware CAN id filtering, but in a very hardware dependent way. A generic interface may be able to handle the PRUSS restrictions as well. CAN devices are usually configured through the netlink interface. e.g.
$ ip link set can0 up type can bitrate 125000and such a common interface would be netlink based as well.
Agreed.
Quoted 3 lines
Quoted 2 lines
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Quoted 6 lines
Quoted 2 lines
Any ideas on how to handle hardware filtering in a generic way are welcome. I will try to come up with a proposal sooner than later.
It sounds to me like the best solution would be change the firmware to lift that restriction and simply allow all IDs, in case it's not actually a hardware limitation (which sounds unlikely).
Yes, that would be best but they told us, that it's not possible with the available hardware resources. Subhasish?
Quoted 3 lines
If that's not possible, maybe it's possible to define a generic filtering interface using netlink, and then either do it completely in the kernel, or using the hardware support.
Well, I hesitate to implement an interface especially for such an exotic device. Fine if it could be handled by a generic CAN hardware filter interface, which is especially useful for normal CAN controllers.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 7 lines
Quoted 4 lines
It sounds to me like the best solution would be change the firmware to lift that restriction and simply allow all IDs, in case it's not actually a hardware limitation (which sounds unlikely).
Yes, that would be best but they told us, that it's not possible with the available hardware resources. Subhasish?
Yes, In case if we allow the ALL implementation, it hogs the CPU. In that case we do not need the PRU. The whole purpose of the PRU is to offload the processor for any such implementations.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 11 lines · On Tue, 10 May 2011 15:41:49 +0530 "Subhasish Ghosh"…
Quoted 7 lines
Quoted 4 lines
It sounds to me like the best solution would be change the firmware to lift that restriction and simply allow all IDs, in case it's not actually a hardware limitation (which sounds unlikely).
Yes, that would be best but they told us, that it's not possible with the available hardware resources. Subhasish?
Yes, In case if we allow the ALL implementation, it hogs the CPU. In that case we do not need the PRU. The whole purpose of the PRU is to offload the processor for any such implementations.
So the kernel presumably needs to switch between using the PRU and native according to the number of ids being requested at the time ?
That would be roughly what we do with other things (eg IP multicast) so that apps don't need to know all the innards
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 6 lines
Quoted 3 lines
Yes, In case if we allow the ALL implementation, it hogs the CPU. In that case we do not need the PRU. The whole purpose of the PRU is to offload the processor for any such implementations.
So the kernel presumably needs to switch between using the PRU and native according to the number of ids being requested at the time ?
All the IDs are programmed into the PRU data RAM.
The Kernel receives interrupts based upon these IDs.
I could not clearly follow "PRU and native", could you please elaborate.
Quoted 2 lines
That would be roughly what we do with other things (eg IP multicast) so that apps don't need to know all the innards
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 11 lines · Subhasish Ghosh
Quoted 7 lines
Quoted 4 lines
Yes, In case if we allow the ALL implementation, it hogs the CPU. In that case we do not need the PRU. The whole purpose of the PRU is to offload the processor for any such implementations.
So the kernel presumably needs to switch between using the PRU and native according to the number of ids being requested at the time ?
All the IDs are programmed into the PRU data RAM.
The Kernel receives interrupts based upon these IDs.
I could not clearly follow "PRU and native", could you please elaborate.
We would really like all CAN drivers to behave the same way. All other drivers are able to work without filters, so pruss_can should allow that too, even if it becomes a CPU hog at that time.
It seems to me that the pruss can implementation has one thing backwards:
it assumes a specific usage model for CAN that it is trying to do offload for. However, that usage model is currently not even supported by Socket CAN. If I understand Wolfgang correctly, it is in fact considered an unwanted limitation of the pruss can driver, instead of a useful feature.
If that interpretation is right, I would seriously recommend rethinking the design of the CAN firmware for pruss, so you can start doing something useful with the offload engine that fits into the Socket CAN API, or that would be a useful extension to Socket CAN that is also implementable in the kernel for all other drivers in a meaningful way.
Arnd
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 5 lines · Arnd Bergmann
If that interpretation is right, I would seriously recommend rethinking the design of the CAN firmware for pruss, so you can start doing something useful with the offload engine that fits into the Socket CAN API, or that would be a useful extension to Socket CAN that is also implementable in the kernel for all other drivers in a meaningful way.
I've looked some more into the CAN socket implementation, and I suppose that the idea of the pruss driver was really to help do the work from the can_rcv_filter function in hardware.
Doing this right would really mean supporting both a mode where any new filter that gets added to socket can ends up being added to the hardware as long as it fits, similar to how we can add additional unicast mac addresses to an ethernet NIC. However, when the filters from all user sockets combined can not be represented in the hardware driver, the hardware needs to be put into a less efficient mode where all packets are returned to the kernel and processed in software.
Arnd
Marc Kleine-Budde
maintainerRe: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Arnd Bergmann
If that interpretation is right, I would seriously recommend rethinking the design of the CAN firmware for pruss, so you can start doing something useful with the offload engine that fits into the Socket CAN API, or that would be a useful extension to Socket CAN that is also implementable in the kernel for all other drivers in a meaningful way.
I've looked some more into the CAN socket implementation, and I suppose that the idea of the pruss driver was really to help do the work from the
can_rcv_filterfunction in hardware.
Quoted 7 lines
Doing this right would really mean supporting both a mode where any new filter that gets added to socket can ends up being added to the hardware as long as it fits, similar to how we can add additional unicast mac addresses to an ethernet NIC. However, when the filters from all user sockets combined can not be represented in the hardware driver, the hardware needs to be put into a less efficient mode where all packets are returned to the kernel and processed in software.
I'm not sure if reprogramming hardware filters on the fly works on evey can core. The more conservative solution would be to configure the filter list globally (+when the interface is down) via netlink.
regards, Marc
Signature · 12 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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110512/d1257aeb/attachment.sig>
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 3 lines
I'm not sure if reprogramming hardware filters on the fly works on evey can core. The more conservative solution would be to configure the filter list globally (+when the interface is down) via netlink.
For anything that isn't so braindead it ought to be done on the fly and behind the users back to avoid having to make app code specially aware. If the lists are fixed either in firmware or in software the stack needs to error attempts to use anything else - and as you say you need some kind of way to configure those global lists, preferably portably.
Alan
can: hardware vs. software filter
Quoted 9 lines · Alan Cox
Quoted 3 lines
I'm not sure if reprogramming hardware filters on the fly works on evey can core. The more conservative solution would be to configure the filter list globally (+when the interface is down) via netlink.
For anything that isn't so braindead it ought to be done on the fly and behind the users back to avoid having to make app code specially aware.
If the lists are fixed either in firmware or in software the stack needs to error attempts to use anything else
That is the best guarantee to let users never use this. A tool like 'candump' should not know about any pre-configured filter that is in place. It's the responsibility of the root user to put proper preconfigured filters.
A lot of code would be spent to allow an application to request software filters that fit in the preconfigured ones.
IMHO its the root users job to judge about a proper preconfigured filter that suits the applications that run. The applications should then be able to request any software filter they like, the root user decided on limiting traffic.
This poses a problem in that an application may depend on these filters for its proper operation. That exactly is the responsibility of the root user who restricts the preconfigured filter.
Kurt
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Arnd BergmannQuoted 10 lines · Arnd Bergmann
Quoted 5 lines · Arnd Bergmann
If that interpretation is right, I would seriously recommend rethinking the design of the CAN firmware for pruss, so you can start doing something useful with the offload engine that fits into the Socket CAN API, or that would be a useful extension to Socket CAN that is also implementable in the kernel for all other drivers in a meaningful way.
I've looked some more into the CAN socket implementation, and I suppose that the idea of the pruss driver was really to help do the work from the
can_rcv_filterfunction in hardware.
That software filter is per socket while the hardware filter will be per device.
Quoted 7 lines
Doing this right would really mean supporting both a mode where any new filter that gets added to socket can ends up being added to the hardware as long as it fits, similar to how we can add additional unicast mac addresses to an ethernet NIC. However, when the filters from all user sockets combined can not be represented in the hardware driver, the hardware needs to be put into a less efficient mode where all packets are returned to the kernel and processed in software.
Well, that seems sophisticated resulting in a complex implementation (may code line) also because hardware filters are very hardware dependent. Usually just one global filter can be defined. I think that's overkill. A simple interface using:
ip link set can0 type can filter <id>:<mask> [<id>:<mask> ...]
would just be fine.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 5 lines · Wolfgang Grandegger
A simple interface using:
ip link set can0 type can filter <id>:<mask> [<id>:<mask> ...]
would just be fine.
Yep.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Wolfgang GrandeggerQuoted 8 lines · Wolfgang Grandegger
Well, that seems sophisticated resulting in a complex implementation (may code line) also because hardware filters are very hardware dependent. Usually just one global filter can be defined. I think that's overkill. A simple interface using:
ip link set can0 type can filter <id>:<mask> [<id>:<mask> ...]
would just be fine.
Ok, fair enough. Still I would suggest you first come up with a reasonable user interface (the one you posted may be just right, I don't know), and then let someone do the implementation in the pruss firmware that is the best match for the user interface, rather than the other way around.
Arnd
Marc Kleine-Budde
maintainerRe: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Wolfgang Grandegger
Well, that seems sophisticated resulting in a complex implementation (may code line) also because hardware filters are very hardware dependent. Usually just one global filter can be defined. I think that's overkill. A simple interface using:
ip link set can0 type can filter <id>:<mask> [<id>:<mask> ...]
Quoted 7 lines
Quoted 1 line
would just be fine.
Ok, fair enough. Still I would suggest you first come up with a reasonable user interface (the one you posted may be just right, I don't know), and then let someone do the implementation in the pruss firmware that is the best match for the user interface, rather than the other way around.
I suggested some time ago to implement the more-or-less standard <id>:<mask> filter instead of just a single <id> (per filter entry). The cost is probably quite low, it's just a single AND operation per filter entry more expensive.
regards, Marc
Signature · 12 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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110512/be9eb351/attachment.sig>
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Wolfgang GrandeggerQuoted 8 lines · Wolfgang Grandegger
Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · Arnd Bergmann
If that interpretation is right, I would seriously recommend rethinking the design of the CAN firmware for pruss, so you can start doing something useful with the offload engine that fits into the Socket CAN API, or that would be a useful extension to Socket CAN that is also implementable in the kernel for all other drivers in a meaningful way.
Quoted 6 lines
Quoted 3 lines
I've looked some more into the CAN socket implementation, and I suppose that the idea of the pruss driver was really to help do the work from the
can_rcv_filterfunction in hardware.That software filter is per socket while the hardware filter will be per device.
Hi all,
i took some while to get behind all the arguments for me :-)
Wolfgangs suggestion
Quoted 4 lines
A simple interface using:
ip link set can0 type can filter <id>:<mask> [<id>:<mask> ...]
indeed would just be fine - but IMHO it doesn't help for the pruss CAN driver.
The problem is, that you plan to filter on a CAN-identifier base. This is not only very application dependent (as Kurt already pointed out) - it also does not bring any safety that your system does not explode on heavy CAN load.
E.g. assume you need the CAN-IDs 0x100, 0x200 and 0x300 in your application and for that reason you configure these IDs in the pruss CAN driver.
What if someone generates a 100% CAN busload exactly on CAN-ID 0x100 then?
Worst case (1MBit/s, DLC=0) you would need to handle about 21.000 irqs/s for the correctly received CAN frames with the filtered CAN-ID 0x100 ...
At the beginnig of the SocketCAN development, we had a PowerPC (E603) @ 133MHz that was able to handle 4 (dumb) SJA1000 CAN controllers without problems.
Maybe i should tell a bit more about what's happening at CAN frame receive time:
1. IRQ happens
2. Read CAN frame from CAN controller registers
3. Allocate a socketbuffer (skb) and queue it into the netdevice rx queue
4. Softirq handles the new skb
5. Check the (specialized/optimized) CAN-ID filters for this CAN device
6. Enqueue the data to the sockets recv buffer(s) and/or drop the skb.
This all depends heavily on Linux networking (skb handling, caching, etc) and is pretty fast and optimized!! That was also the reason why it ran on the old PowerPC that smoothly. The mostly seen effect if anything drops is when the application (holding the socket) was not fast enough to handle the incoming data. NB: For that reason we implemented a CAN content filter (CAN_BCM) that is able to do content filtering and timeout monitoring in Kernelspace - all performed in the SoftIRQ.
Having 'Mailboxes' bound to CAN-IDs is something that's useful for 8/16 bit CPUs where an application is tightly bound to the embedded ECUs functionality.
IMO you should try to implement an 'open' pruss CAN driver without filtering as it doesn't really help (see above). The networking stack can cope with the load. In all cases (with/without filters) the reduction the irqload could be a way to investigate (e.g. think about dropping CAN frames based on the irqload).
But you should try the 'open' way first. And i think with your current driver you could just filter for one CAN-ID (e.g. 0x100) and then produce the heavy CAN busload with this CAN-ID 0x100 and a second CAN node, right?
Regards,
Oliver
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 7 lines · On Thursday 12 May 2011 16:41:58 Oliver Hartkopp wrote:
E.g. assume you need the CAN-IDs
0x100,0x200and0x300in your application and for that reason you configure these IDs in the pruss CAN driver.What if someone generates a 100% CAN busload exactly on CAN-ID
0x100then?Worst case (1MBit/s, DLC=0) you would need to handle about 21.000 irqs/s for the correctly received CAN frames with the filtered CAN-ID
0x100...
Then I guess the main thing that a "smart" CAN implementation like pruss should do is interrupt mitigation. When you have a constant flow of packets coming in, the hardware should be able to DMA a lot of them into kernel memory before the driver is required to pick them up, and only get into interrupt driven mode when the kernel has managed to process all outstanding packets.
Quoted 7 lines
This all depends heavily on Linux networking (skb handling, caching, etc) and is pretty fast and optimized!! That was also the reason why it ran on the old PowerPC that smoothly. The mostly seen effect if anything drops is when the application (holding the socket) was not fast enough to handle the incoming data. NB: For that reason we implemented a CAN content filter (
CAN_BCM) that is able to do content filtering and timeout monitoring in Kernelspace - all performed in the SoftIRQ.
Right, dropping packets that no process is waiting for should be done as early as possible. In pruss-can, the idea was to do it in hardware, which doesn't really work all that well for the reasons discussed before. Dropping the frames in the NAPI poll function (softirq time) seems like a logical choice.
Quoted 2 lines
Having 'Mailboxes' bound to CAN-IDs is something that's useful for 8/16 bit CPUs where an application is tightly bound to the embedded ECUs functionality.
Makes sense.
Arnd
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 24 lines · Arnd Bergmann
Quoted 7 lines · On Thursday 12 May 2011 16:41:58 Oliver Hartkopp wrote:
E.g. assume you need the CAN-IDs
0x100,0x200and0x300in your application and for that reason you configure these IDs in the pruss CAN driver.What if someone generates a 100% CAN busload exactly on CAN-ID
0x100then?Worst case (1MBit/s, DLC=0) you would need to handle about 21.000 irqs/s for the correctly received CAN frames with the filtered CAN-ID
0x100...Then I guess the main thing that a "smart" CAN implementation like pruss should do is interrupt mitigation. When you have a constant flow of packets coming in, the hardware should be able to DMA a lot of them into kernel memory before the driver is required to pick them up, and only get into interrupt driven mode when the kernel has managed to process all outstanding packets.
Quoted 7 lines
This all depends heavily on Linux networking (skb handling, caching, etc) and is pretty fast and optimized!! That was also the reason why it ran on the old PowerPC that smoothly. The mostly seen effect if anything drops is when the application (holding the socket) was not fast enough to handle the incoming data. NB: For that reason we implemented a CAN content filter (
CAN_BCM) that is able to do content filtering and timeout monitoring in Kernelspace - all performed in the SoftIRQ.
Quoted 5 lines
Right, dropping packets that no process is waiting for should be done as early as possible. In pruss-can, the idea was to do it in hardware, which doesn't really work all that well for the reasons discussed before. Dropping the frames in the NAPI poll function (softirq time) seems like a logical choice.
In 'real world' CAN setups you'll never see 21.000 CAN frames per second (and therefore 21.000 irqs/s) - you are usually designing CAN network traffic with less than 60% busload. So interrupt rates somewhere below 1000 irqs/s can be assumed.
Quoted 1 line
From [email protected] Fri May 20 08:44:36 2011
From: [email protected] ()
Date: Fri, 20 May 2011 12:44:36 -0000
Subject: No subject
Message-ID: <[email protected]>
@Marc/Wolfgang: Would this be also your recommendation for a CAN controller design that supports SocketCAN in the best way?
As the Linux network stack supports hardware timestamps too, this could be an additional (optional!) feature.
Regards,
Oliver
Quoted 6 lines
Quoted 2 lines
Having 'Mailboxes' bound to CAN-IDs is something that's useful for 8/16 bit CPUs where an application is tightly bound to the embedded ECUs functionality.
Makes sense.
Arnd
Marc Kleine-Budde
maintainerRe: [PATCH v4 1/1] can: add pruss CAN driver.
On 05/23/2011 08:21 AM, Oliver Hartkopp wrote:
[...]
Quoted 9 lines
In 'real world' CAN setups you'll never see 21.000 CAN frames per second (and therefore 21.000 irqs/s) - you are usually designing CAN network traffic with less than 60% busload. So interrupt rates somewhere below 1000 irqs/s can be assumed.
From what i've seen so far a 3-4 messages rx FIFO and NAPI support just make it.
@Marc/Wolfgang: Would this be also your recommendation for a CAN controller design that supports SocketCAN in the best way?
If you have a rx FIFO NAPI is the way to go. For a single mailbox it adds overhead, if you can read the CAN frame in the interrupt handler. The error messages should probably generated from NAPI, too. Especially the I'm-the-only-CAN-node-on-the-net-and-get-no-ACK error message.
However IIRC David said that every new driver should implement NAPI.
Quoted 2 lines
As the Linux network stack supports hardware timestamps too, this could be an additional (optional!) feature.
regards, Marc
Signature · 12 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 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110523/24c9c00f/attachment.sig>
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Oliver HartkoppHi Oliver,
sorry for the late answer.
Quoted 31 lines · Oliver Hartkopp
Quoted 24 lines · Arnd Bergmann
Quoted 7 lines · On Thursday 12 May 2011 16:41:58 Oliver Hartkopp wrote:
E.g. assume you need the CAN-IDs
0x100,0x200and0x300in your application and for that reason you configure these IDs in the pruss CAN driver.What if someone generates a 100% CAN busload exactly on CAN-ID
0x100then?Worst case (1MBit/s, DLC=0) you would need to handle about 21.000 irqs/s for the correctly received CAN frames with the filtered CAN-ID
0x100...Then I guess the main thing that a "smart" CAN implementation like pruss should do is interrupt mitigation. When you have a constant flow of packets coming in, the hardware should be able to DMA a lot of them into kernel memory before the driver is required to pick them up, and only get into interrupt driven mode when the kernel has managed to process all outstanding packets.
Quoted 7 lines
This all depends heavily on Linux networking (skb handling, caching, etc) and is pretty fast and optimized!! That was also the reason why it ran on the old PowerPC that smoothly. The mostly seen effect if anything drops is when the application (holding the socket) was not fast enough to handle the incoming data. NB: For that reason we implemented a CAN content filter (
CAN_BCM) that is able to do content filtering and timeout monitoring in Kernelspace - all performed in the SoftIRQ.Quoted 5 lines
Right, dropping packets that no process is waiting for should be done as early as possible. In pruss-can, the idea was to do it in hardware, which doesn't really work all that well for the reasons discussed before. Dropping the frames in the NAPI poll function (softirq time) seems like a logical choice.
Quoted 6 lines
In 'real world' CAN setups you'll never see 21.000 CAN frames per second (and therefore 21.000 irqs/s) - you are usually designing CAN network traffic with less than 60% busload. So interrupt rates somewhere below 1000 irqs/s can be assumed.
Quoted 1 line
From what i've seen so far a 3-4 messages rx FIFO and NAPI support just make it.
I think you speak about the SJA100 which is able to buffer 64 byte corresponding to up to 4 messages. There are CAN controllers able to queue more or just one message and then NAPI adds overhead.
Quoted 2 lines
@Marc/Wolfgang: Would this be also your recommendation for a CAN controller design that supports SocketCAN in the best way?
Anyway, NAPI *always* useful as it helps with the infamous interrupt flooding.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Arnd BergmannQuoted 17 lines · Arnd Bergmann
Quoted 11 lines · Subhasish Ghosh
Quoted 7 lines
Quoted 4 lines
Yes, In case if we allow the ALL implementation, it hogs the CPU. In that case we do not need the PRU. The whole purpose of the PRU is to offload the processor for any such implementations.
So the kernel presumably needs to switch between using the PRU and native according to the number of ids being requested at the time ?
All the IDs are programmed into the PRU data RAM.
The Kernel receives interrupts based upon these IDs.
I could not clearly follow "PRU and native", could you please elaborate.We would really like all CAN drivers to behave the same way. All other drivers are able to work without filters, so
pruss_canshould allow that too, even if it becomes a CPU hog at that time.
Quoted 5 lines
It seems to me that the pruss can implementation has one thing backwards:
it assumes a specific usage model for CAN that it is trying to do offload for. However, that usage model is currently not even supported by Socket CAN. If I understand Wolfgang correctly, it is in fact considered an unwanted limitation of the pruss can driver, instead of a useful feature.
"Unwanted" is not the right word. I see it as a piece of CAN hardware with some serious limitations and I doubt that it will make real CAN users happy. But well, I might be wrong.
Quoted 5 lines
If that interpretation is right, I would seriously recommend rethinking the design of the CAN firmware for pruss, so you can start doing something useful with the offload engine that fits into the Socket CAN API, or that would be a useful extension to Socket CAN that is also implementable in the kernel for all other drivers in a meaningful way.
It would be really nice if they could provide a better firmware. Anyway, the generic CAN hardware filter interface we spoke about in a previous mail would fit for the PRUSS CAN hardware as well. It just needs to be implemented.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Wolfgang GrandeggerQuoted 11 lines
Quoted 2 lines
How hard would it be to implement that feature in Socket CAN?
CAN controllers usually provide some kind of hardware CAN id filtering, but in a very hardware dependent way. A generic interface may be able to handle the PRUSS restrictions as well. CAN devices are usually configured through the netlink interface. e.g.
$ ip link set can0 up type can bitrate 125000and such a common interface would be netlink based as well.
ack.
Quoted 6 lines
Quoted 3 lines
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Any ideas on how to handle hardware filtering in a generic way are welcome. I will try to come up with a proposal sooner than later.
When doing so, I'd vote for an unlimited(by software) list of hardware filters (id/mask). The hardware must abort when no more filters are available.
I think that when using hardware filters, knowing the actual device
with it's amount of hardware filters is the least of your problems. Userspace applications that suddenly stop working properly due to
hw filters (i.e. some traffic not coming in anymore) will be a major
source of bugreports.
Kurt
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Hi Kurt,
Quoted 13 lines · Kurt Van Dijck
Quoted 11 lines
Quoted 2 lines
How hard would it be to implement that feature in Socket CAN?
CAN controllers usually provide some kind of hardware CAN id filtering, but in a very hardware dependent way. A generic interface may be able to handle the PRUSS restrictions as well. CAN devices are usually configured through the netlink interface. e.g.
$ ip link set can0 up type can bitrate 125000and such a common interface would be netlink based as well.
ack.
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Any ideas on how to handle hardware filtering in a generic way are welcome. I will try to come up with a proposal sooner than later.
When doing so, I'd vote for an unlimited(by software) list of hardware filters (id/mask). The hardware must abort when no more filters are available.
Sounds good and not even to complicated. For the SJA1000 we would just allow to set the global mask.
Quoted 5 lines
I think that when using hardware filters, knowing the actual device with it's amount of hardware filters is the least of your problems. Userspace applications that suddenly stop working properly due to hw filters (i.e. some traffic not coming in anymore) will be a major source of bugreports.
Well, hardware filtering will be off by default and must explicitly be set by the user, like for the bitrate setting.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 7 lines · Wolfgang Grandegger
Quoted 2 lines · Kurt Van Dijck
When doing so, I'd vote for an unlimited(by software) list of hardware filters (id/mask). The hardware must abort when no more filters are available.
Sounds good and not even to complicated. For the SJA1000 we would just allow to set the global mask.
Yes. "unlimited(by software)" was a bit misleading at first reading, as we should not filter IDs by software in the irq handler. But to create a API that supports as much HW filters as the hardware provides is a good idea.
Quoted 8 lines
Quoted 5 lines
I think that when using hardware filters, knowing the actual device with it's amount of hardware filters is the least of your problems. Userspace applications that suddenly stop working properly due to hw filters (i.e. some traffic not coming in anymore) will be a major source of bugreports.
Well, hardware filtering will be off by default and must explicitly be set by the user, like for the bitrate setting.
To be correct: By the admin.
The setting of CAN HW filters has a system-wide effect to all users on the local host. The same effect as we have for the setting of the bitrate. This is the major difference to the user-configurable per-socket CAN-ID filters that are provided e.g. by the CAN_RAW socket.
As the current netlink configuration interface for CAN interfaces is not accessible for standard users also this would be the right place to extend the netlink interface.
Regards,
Oliver
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Kurt Van DijckHi Kurt,
Quoted 13 lines · Kurt Van Dijck
Quoted 11 lines
Quoted 2 lines
How hard would it be to implement that feature in Socket CAN?
CAN controllers usually provide some kind of hardware CAN id filtering, but in a very hardware dependent way. A generic interface may be able to handle the PRUSS restrictions as well. CAN devices are usually configured through the netlink interface. e.g.
$ ip link set can0 up type can bitrate 125000and such a common interface would be netlink based as well.
ack.
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Is that something that Subhasish or someone else could to as a prerequisite to merging the driver?
Any ideas on how to handle hardware filtering in a generic way are welcome. I will try to come up with a proposal sooner than later.
When doing so, I'd vote for an unlimited(by software) list of hardware filters (id/mask). The hardware must abort when no more filters are available.
Sounds good and not even to complicated. For the SJA1000 we would just allow to set the global mask.
Quoted 5 lines
I think that when using hardware filters, knowing the actual device with it's amount of hardware filters is the least of your problems. Userspace applications that suddenly stop working properly due to hw filters (i.e. some traffic not coming in anymore) will be a major source of bugreports.
Well, hardware filtering will be off by default and must explicitly be set by the user, like for the bitrate setting.
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Subhasish GhoshQuoted 5 lines · Subhasish Ghosh
Quoted 2 lines
- Use just *one* value per sysfs file
SG - I felt adding entry for each
mbx_idwill clutter the sysfs.
Is it ok to do that.
No, see:
http://lxr.linux.no/#linux+v2.6.38/Documentation/filesystems/sysfs.txt#L56
Quoted 31 lines
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
+static u32 pruss_intc_init[19][3] = {
+ {PRUSS_INTC_POLARITY0,PRU_INTC_REGMAP_MASK,0xFFFFFFFF},
+ {PRUSS_INTC_POLARITY1,PRU_INTC_REGMAP_MASK,0xFFFFFFFF},
+ {PRUSS_INTC_TYPE0,PRU_INTC_REGMAP_MASK,0x1C000000},
+ {PRUSS_INTC_TYPE1,PRU_INTC_REGMAP_MASK, 0},
+ {PRUSS_INTC_GLBLEN, 0, 1},
+ {PRUSS_INTC_HOSTMAP0,PRU_INTC_REGMAP_MASK,0x03020100},
+ {PRUSS_INTC_HOSTMAP1,PRU_INTC_REGMAP_MASK,0x07060504},
+ {PRUSS_INTC_HOSTMAP2,PRU_INTC_REGMAP_MASK,0x0000908},
+ {PRUSS_INTC_CHANMAP0,PRU_INTC_REGMAP_MASK, 0},
+ {PRUSS_INTC_CHANMAP8,PRU_INTC_REGMAP_MASK,0x00020200},
+ {PRUSS_INTC_STATIDXCLR, 0, 32},
+ {PRUSS_INTC_STATIDXCLR, 0, 19},
+ {PRUSS_INTC_ENIDXSET, 0, 19},
+ {PRUSS_INTC_STATIDXCLR, 0, 18},
+ {PRUSS_INTC_ENIDXSET, 0, 18},
+ {PRUSS_INTC_STATIDXCLR, 0, 34},
+ {PRUSS_INTC_ENIDXSET, 0, 34},
+ {PRUSS_INTC_ENIDXSET, 0, 32},
+ {PRUSS_INTC_HOSTINTEN, 0, 5}Quoted 4 lines
Quoted 1 line
please add ","
Also a struct to describe each entry would improve readability. Then you could also use
ARRAY_SIZE.SG _ I could not follow this, are you recommending that I create a structure with three variables and then create
an array for it.
something like:
Quoted 8 lines
const static struct [] = { { unsigned int reg_base; unsigned int reg_mask; unsigned int reg_val; },...
};
Yes:
struct s_name {
unsigned int base;
unsigned int mask;
unsigned int val;};
const static struct s_name array[] = {
...};
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
+ value = (
PRUSS_CAN_GPIO_SETUP_DELAY*
+ (priv->clk_freq_pru/ 1000000) / 1000) /
+PRUSS_CAN_DELAY_LOOP_LENGTH;This calculation looks delicate. 64-bit math would be safer.
SG - This one works fine. I am dividing it twice to avoid the problem.
Yes, but what if the frequency increases with the next generation of the hardware?
Wolfgang.
Re: [PATCH v4 1/1] can: add pruss CAN driver.
Quoted 10 lines · Wolfgang Grandegger
Quoted 5 lines · Subhasish Ghosh
Quoted 2 lines
- Use just *one* value per sysfs file
SG - I felt adding entry for each
mbx_idwill clutter the sysfs.
Is it ok to do that.No, see:
http://lxr.linux.no/#linux+v2.6.38/Documentation/filesystems/sysfs.txt#L56
s/No/Yes/
Sorry for the confusion.
Wolfgang.
Marc Kleine-Budde
maintainerRe: [PATCH v4 1/1] can: add pruss CAN driver.
In reply to Subhasish GhoshI'm not sure whether this message made it to the netdev mailinglist. This is a resend.
Quoted 1 line · Subhasish Ghosh
This patch adds support for the CAN device emulated on PRUSS.
After commenting the code inline, some remarks:
- Your tx path looks broken, see commits inline
- Please setup a proper struct to describe your register layout, make use of arrays for rx and tx
- don't use u32, s32 for not hardware related variables like return codes and loop counter.
- the routines that load and save the can data bytes from/into your mailbox look way to complicated. Please write down the layout so that we can think of a elegant way to do it
- a lot of functions unconditionally return 0, make them void if no error can happen
- think about using managed devices, that would simplify the probe and release function
Quoted 14 lines
Signed-off-by Subhasish Ghosh <[email protected]>--- drivers/net/can/Kconfig | 7 + drivers/net/can/Makefile | 1 + drivers/net/can/pruss_can.c | 1074 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1082 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/pruss_can.cdrivers/net/can/Kconfig +0
-0
drivers/net/can/Kconfig config PCH_CAN · line 119
Quoted 9 lines
drivers/net/can/Kconfig +6
-0@@ -111,6 +111,13 @@ config PCH_CAN embedded processor. This driver can access CAN bus. +config CAN_TI_DA8XX_PRU + depends on CAN_DEV && ARCH_DAVINCI && ARCH_DAVINCI_DA850 + tristate "PRU based CAN emulation for DA8XX" + ---help--- + Enable this to emulate a CAN controller on the PRU of DA8XX. + If not sure, mark N
Please indent the help text with 1 tab and 2 spaces
Quoted 62 lines
drivers/net/can/Kconfig +1
-0+ source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig"drivers/net/can/Makefile +1
-0@@ -15,6 +15,7 @@ obj-$(CONFIG_CAN_SJA1000) += sja1000/ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_AT91) += at91_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o +obj-$(CONFIG_CAN_TI_DA8XX_PRU) += pruss_can.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o obj-$(CONFIG_CAN_BFIN) += bfin_can.o obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.odrivers/net/can/pruss_can.c new file +40
-0@@ -0,0 +1,1074 @@ +/* +* TI DA8XX PRU CAN Emulation device driver +* Author: [email protected] +* +* This driver supports TI's PRU CAN Emulation and the +* specs for the same is available at <http://www.ti.com> +* +* Copyright (C) 2010, 2011 Texas Instruments Incorporated +* +* 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. +* +* This program is distributed as is WITHOUT ANY WARRANTY of any +* kind, whether express or implied; without even the implied warranty +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/bitops.h> +#include <linux/interrupt.h> +#include <linux/errno.h> +#include <linux/netdevice.h> +#include <linux/skbuff.h> +#include <linux/platform_device.h> +#include <linux/firmware.h> +#include <linux/clk.h> +#include <linux/types.h> +#include <linux/sysfs.h> +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> +#include <linux/mfd/pruss.h> + +#define PRUSS_CAN_RX_PRU_0 PRUSS_NUM0 +#define PRUSS_CAN_TX_PRU_1 PRUSS_NUM1 +#define PRUSS_CAN_TX_SYS_EVT 34
drivers/net/can/pruss_can.c line 50
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+#define PRUSS_CAN_RX_SYS_EVT 33 +#define PRUSS_CAN_ARM2DSP_SYS_EVT 32 +#define PRUSS_CAN_DELAY_LOOP_LENGTH 2 +#define PRUSS_CAN_TIMER_SETUP_DELAY 14 +#define PRUSS_CAN_GPIO_SETUP_DELAY 150 +#define PRUSS_CAN_TX_GBL_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x04) +#define PRUSS_CAN_TX_INTR_MASK_REG (PRUSS_PRU1_BASE_ADDRESS + 0x08) +#define PRUSS_CAN_TX_INTR_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x0C) +#define PRUSS_CAN_TX_MBOX0_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x10) +#define PRUSS_CAN_TX_ERR_CNTR_REG (PRUSS_PRU1_BASE_ADDRESS + 0x30)
I think Wolfgang and me have asked you to describe the register layout with a struct.
Quoted 32 lines
drivers/net/can/pruss_can.c new file +32
-0+#define PRUSS_CAN_TX_INT_STAT 0x2 +#define PRUSS_CAN_MBOX_OFFSET 0x40 +#define PRUSS_CAN_MBOX_SIZE 0x10 +#define PRUSS_CAN_MBOX_STAT_OFFSET 0x10 +#define PRUSS_CAN_MBOX_STAT_SIZE 0x04 +#define PRUSS_CAN_GBL_STAT_REG_VAL 0x00000040 +#define PRUSS_CAN_INTR_MASK_REG_VAL 0x00004000 +#define PRUSS_CAN_TIMING_VAL_TX (PRUSS_PRU1_BASE_ADDRESS + 0xC0) +#define PRUSS_CAN_TIMING_SETUP (PRUSS_PRU1_BASE_ADDRESS + 0xC4) +#define PRUSS_CAN_RX_GBL_STAT_REG (PRUSS_PRU0_BASE_ADDRESS + 0x04) +#define PRUSS_CAN_RX_INTR_MASK_REG (PRUSS_PRU0_BASE_ADDRESS + 0x08) +#define PRUSS_CAN_RX_INTR_STAT_REG (PRUSS_PRU0_BASE_ADDRESS + 0x0C) +#define PRUSS_CAN_RX_ERR_CNTR_REG (PRUSS_PRU0_BASE_ADDRESS + 0x34) +#define PRUSS_CAN_TIMING_VAL_RX (PRUSS_PRU0_BASE_ADDRESS + 0xD0) +#define PRUSS_CAN_BIT_TIMING_VAL_RX (PRUSS_PRU0_BASE_ADDRESS + 0xD4) +#define PRUSS_CAN_ID_MAP (PRUSS_PRU0_BASE_ADDRESS + 0xF0) +#define PRUSS_CAN_ERROR_ACTIVE 128 +#define PRUSS_CAN_GBL_STAT_REG_MASK 0x1F +#define PRUSS_CAN_GBL_STAT_REG_SET_TX 0x80 +#define PRUSS_CAN_GBL_STAT_REG_SET_RX 0x40 +#define PRUSS_CAN_GBL_STAT_REG_STOP_TX 0x7F +#define PRUSS_CAN_GBL_STAT_REG_STOP_RX 0xBF +#define PRUSS_CAN_SET_TX_REQ 0x00000080 +#define PRUSS_CAN_STD_FRAME_MASK 18 +#define PRUSS_CAN_START 1 +#define PRUSS_CAN_MB_MIN 0 +#define PRUSS_CAN_MB_MAX 7 +#define PRUSS_CAN_MID_IDE BIT(29) +#define PRUSS_CAN_ISR_BIT_CCI BIT(15) +#define PRUSS_CAN_ISR_BIT_ESI BIT(14) +#define PRUSS_CAN_ISR_BIT_SRDI BIT(13) +#define PRUSS_CAN_ISR_BIT_RRI BIT(8)
drivers/net/can/pruss_can.c line 92
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+#define PRUSS_CAN_GSR_BIT_EPM BIT(4) +#define PRUSS_CAN_GSR_BIT_BFM BIT(3) +#define PRUSS_CAN_RTR_BUFF_NUM 8 +#define PRUSS_DEF_NAPI_WEIGHT 8 +#define PRUSS_CAN_DRV_NAME "da8xx_pruss_can" +#define PRUSS_CAN_DRV_DESC "TI PRU CAN Controller Driver v1.0" + +enum can_tx_dir { +TRANSMIT =1 , +RECEIVE
please add a "," after RECEIVE
drivers/net/can/pruss_can.c line 95
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+}; + +struct can_mbox {
I suggest to add a namespace to these structs, e.g. pru_can_mbox. Same comment applies to the other structs.
drivers/net/can/pruss_can.c line 96
Quoted 1 line
drivers/net/can/pruss_can.c new file +1
-0+canid_t can_id;
You write this struct into the hardware, don't you? So you should not use kernel internal types to describe your hardware layout. Think about declaring this struct __packed__
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+u8 data[8 ]; +u16 datalength; +u16 crc; +}; + +struct can_emu_cntx { +enum can_tx_dir txdir; +struct can_mbox mbox; +u32 mboxno; +u8 pruno; +u32 gbl_stat; +u32 intr_stat; +u32 mbox_stat;
drivers/net/can/pruss_can.c line 119
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+}; + +struct can_emu_priv { +struct can_priv can; +struct napi_struct napi; +struct net_device *ndev; +struct device *dev; +struct clk *clk_timer; +struct can_emu_cntx can_tx_cntx; +struct can_emu_cntx can_rx_cntx;
I have not looked at the rest of the code, but it smells that you should make this an array of two cntx.
Quoted 14 lines
drivers/net/can/pruss_can.c new file +14
-0+unsigned int clk_freq_pru; +unsigned int trx_irq; +unsigned int tx_head; +unsigned int tx_tail; +unsigned int tx_next; +unsigned int mbx_id[8 ]; +}; + +static struct can_bittiming_const pru_can_bittiming_const = { + .name =PRUSS_CAN_DRV_NAME , + .tseg1_min =1 , + .tseg1_max =16 , + .tseg2_min =1 , + .tseg2_max =8 ,
drivers/net/can/pruss_can.c line 143
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ .sjw_max = 4, + .brp_min = 1, + .brp_max = 256, + .brp_inc = 1, +}; + +static int pru_can_mbox_write (struct device *dev, +struct can_emu_cntx *emu_cntx) +{ +u32 offset =0 ;
^^^^^
not needed
drivers/net/can/pruss_can.c line 156
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+ + offset =PRUSS_PRU1_BASE_ADDRESS +PRUSS_CAN_MBOX_OFFSET + + (PRUSS_CAN_MBOX_SIZE * emu_cntx->mboxno); + +pruss_writel_multi (dev, offset, (u32 *) &(emu_cntx->mbox),4 ); + +return 0 ; +} + +static int pru_can_mbox_read (struct device *dev, +struct can_emu_cntx *emu_cntx) +{ +u32 offset =0 ;
dito
drivers/net/can/pruss_can.c line 161
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ + offset =PRUSS_PRU0_BASE_ADDRESS +PRUSS_CAN_MBOX_OFFSET + + (PRUSS_CAN_MBOX_SIZE * emu_cntx->mboxno); + +pruss_readl_multi (dev, offset, (u32 *) &emu_cntx->mbox,4 );
where does this "4" come from? consider using sizeof()
drivers/net/can/pruss_can.c line 163
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ +return 0 ;
why do you return 0 here? pruss_readl_multi is not void, although it always returns 0, too. consider make all void.
drivers/net/can/pruss_can.c line 170
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+} + +static int pru_can_rx_id_set (struct device *dev,u32 nodeid,u32 mboxno) +{ +pruss_writel (dev, (PRUSS_CAN_ID_MAP + (mboxno *4 )), nodeid); + +return 0 ;
consider making this a void function.
drivers/net/can/pruss_can.c line 181
Quoted 11 lines
drivers/net/can/pruss_can.c new file +11
-0+} + +static int pru_can_intr_stat_get (struct device *dev, +struct can_emu_cntx *emu_cntx) +{ +if (emu_cntx->pruno ==PRUCORE_1 ) +pruss_readl (dev,PRUSS_CAN_TX_INTR_STAT_REG , + &emu_cntx->intr_stat); +else if (emu_cntx->pruno ==PRUCORE_0 ) +pruss_readl (dev,PRUSS_CAN_RX_INTR_STAT_REG , + &emu_cntx->intr_stat);
If you describe the register layout with a struct with an array containing with rx and tx registers you can get rid of the if..else.. use emu_cntx->pruno as index to the array.
drivers/net/can/pruss_can.c line 183
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+else +return -EINVAL ;
It's an internally used function, if emu_cntx->pruno is bogous here you've got really big problems. I think it's save to remove this. Then this function would become a void function.
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ +return 0 ; +} + +static int pru_can_gbl_stat_get (struct device *dev, +struct can_emu_cntx *emu_cntx)
drivers/net/can/pruss_can.c line 199
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+{ +if (emu_cntx->pruno ==PRUCORE_1 ) +pruss_readl (dev,PRUSS_CAN_TX_GBL_STAT_REG , + &emu_cntx->gbl_stat); +else if (emu_cntx->pruno ==PRUCORE_0 ) +pruss_readl (dev,PRUSS_CAN_RX_GBL_STAT_REG , + &emu_cntx->gbl_stat); +else +return -EINVAL ; +return 0 ;
Same comments apply here, too.
Quoted 18 lines
drivers/net/can/pruss_can.c new file +18
-0+} + +static int pru_can_tx_mode_set (struct device *dev,bool transfer_flag, +enum can_tx_dir ecan_trx) +{ +u32 value; + +if (ecan_trx ==TRANSMIT ) { +pruss_readl (dev,PRUSS_CAN_RX_GBL_STAT_REG , &value); +if (transfer_flag) { + value &=PRUSS_CAN_GBL_STAT_REG_MASK ; + value |=PRUSS_CAN_GBL_STAT_REG_SET_TX ; + }else + value &=PRUSS_CAN_GBL_STAT_REG_STOP_TX ; + +pruss_writel (dev,PRUSS_CAN_RX_GBL_STAT_REG , value); +pruss_writel (dev,PRUSS_CAN_TX_GBL_STAT_REG , value); + }else if (ecan_trx ==RECEIVE ) {
drivers/net/can/pruss_can.c line 227
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+pruss_readl (dev,PRUSS_CAN_RX_GBL_STAT_REG , &value); +if (transfer_flag) { + value &=PRUSS_CAN_GBL_STAT_REG_MASK ; + value |=PRUSS_CAN_GBL_STAT_REG_SET_RX ; + }else + value &=PRUSS_CAN_GBL_STAT_REG_STOP_RX ; + +pruss_writel (dev,PRUSS_CAN_RX_GBL_STAT_REG , value); +pruss_writel (dev,PRUSS_CAN_TX_GBL_STAT_REG , value); + }else
Same comments apply here, too.
drivers/net/can/pruss_can.c line 187
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+return -EINVAL ; + +return 0 ; +} +
is this array const?
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static u32 pruss_intc_init[19 ][3 ] = { + {PRUSS_INTC_POLARITY0 ,PRU_INTC_REGMAP_MASK ,0xFFFFFFFF }, + {PRUSS_INTC_POLARITY1 ,PRU_INTC_REGMAP_MASK ,0xFFFFFFFF }, + {PRUSS_INTC_TYPE0 ,PRU_INTC_REGMAP_MASK ,0x1C000000 }, + {PRUSS_INTC_TYPE1 ,PRU_INTC_REGMAP_MASK ,0 }, + {PRUSS_INTC_GLBLEN ,0 ,1 }, + {PRUSS_INTC_HOSTMAP0 ,PRU_INTC_REGMAP_MASK ,0x03020100 }, + {PRUSS_INTC_HOSTMAP1 ,PRU_INTC_REGMAP_MASK ,0x07060504 }, + {PRUSS_INTC_HOSTMAP2 ,PRU_INTC_REGMAP_MASK ,0x0000908 }, + {PRUSS_INTC_CHANMAP0 ,PRU_INTC_REGMAP_MASK ,0 },
drivers/net/can/pruss_can.c line 252
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ {PRUSS_INTC_CHANMAP8, PRU_INTC_REGMAP_MASK, 0x00020200}, + {PRUSS_INTC_STATIDXCLR, 0, 32}, + {PRUSS_INTC_STATIDXCLR, 0, 19}, + {PRUSS_INTC_ENIDXSET, 0, 19}, + {PRUSS_INTC_STATIDXCLR, 0, 18}, + {PRUSS_INTC_ENIDXSET, 0, 18}, + {PRUSS_INTC_STATIDXCLR, 0, 34}, + {PRUSS_INTC_ENIDXSET, 0, 34}, + {PRUSS_INTC_ENIDXSET, 0, 32}, + {PRUSS_INTC_HOSTINTEN, 0, 5}
please add ","
drivers/net/can/pruss_can.c line 258
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+}; + +static int pru_can_emu_init (struct device *dev,u32 pruclock) +{ +u32 value[8 ] = {[0 ...7 ] =1 }; +u32 i;
we usually use plain ints as a for-loop variable.
drivers/net/can/pruss_can.c line 261
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+ +/* PRU Internal Registers Initializations */ +pruss_writel_multi (dev,PRUSS_CAN_TX_MBOX0_STAT_REG , value,8 );
use sizeof(), or ARRAY_SIZE
drivers/net/can/pruss_can.c line 265
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+ + *value =PRUSS_CAN_GBL_STAT_REG_VAL ; +pruss_writel (dev,PRUSS_CAN_TX_GBL_STAT_REG , value[0 ]); +pruss_writel (dev,PRUSS_CAN_RX_GBL_STAT_REG , value[0 ]);
why not:pruss_writel(dev, PRUSS_CAN_TX_GBL_STAT_REG, PRUSS_CAN_GBL_STAT_REG_VAL);
drivers/net/can/pruss_can.c line 271
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ + *value =PRUSS_CAN_INTR_MASK_REG_VAL ; +pruss_writel (dev,PRUSS_CAN_TX_INTR_MASK_REG , value[0 ]); +pruss_writel (dev,PRUSS_CAN_RX_INTR_MASK_REG , value[0 ]); + +for (i =0 ; i <19 ; i++)
ARRAY_SIZE
drivers/net/can/pruss_can.c line 276
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ (i <12 ) ?pruss_rmwl (dev, pruss_intc_init[i][0 ], + pruss_intc_init[i][1 ], + pruss_intc_init[i][2 ]) : +pruss_idx_writel (dev, pruss_intc_init[i][0 ], + pruss_intc_init[i][2 ]);
if..else here, please
or put the stuff into two arrays
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+return 0 ; +} + +static void pru_can_emu_exit (struct device *dev)
drivers/net/can/pruss_can.c line 290
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+{ +pruss_disable (dev,PRUSS_CAN_RX_PRU_0 ); +pruss_disable (dev,PRUSS_CAN_TX_PRU_1 ); +} + +static int pru_can_tx (struct device *dev,u8 mboxnumber,u8 pruno) +{ +u32 value =0 ; + +if (PRUCORE_1 == pruno) {
we usually write it the other way round...:
if (pruno == PRUCORE_1)
drivers/net/can/pruss_can.c line 295
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ value =PRUSS_CAN_SET_TX_REQ ; +pruss_writel (dev, ((PRUSS_PRU1_BASE_ADDRESS + + (PRUSS_CAN_MBOX_STAT_OFFSET + + (PRUSS_CAN_MBOX_STAT_SIZE * mboxnumber))) + &0xFFFF ), value);
don't use value, use PRUSS_CAN_SET_TX_REQ directly
drivers/net/can/pruss_can.c line 304
Quoted 9 lines
drivers/net/can/pruss_can.c new file +9
-0+ }else if (PRUCORE_0 == pruno) { +pruss_readl (dev,PRUSS_CAN_RX_INTR_STAT_REG , &value); + value = value & ~(1 << mboxnumber); +pruss_writel (dev,PRUSS_CAN_RX_INTR_STAT_REG , value); + value =0 ; +pruss_writel (dev, ((PRUSS_PRU0_BASE_ADDRESS + + (PRUSS_CAN_MBOX_STAT_OFFSET + + (PRUSS_CAN_MBOX_STAT_SIZE * mboxnumber))) + &0xFFFF ), value);
same here
drivers/net/can/pruss_can.c line 306
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ }else +return -EINVAL ;
trust your own code, get rid of the -EINVAL, make this a void function.
drivers/net/can/pruss_can.c line 315
Quoted 9 lines
drivers/net/can/pruss_can.c new file +9
-0+return 0 ; +} + +static int pru_can_reset_tx (struct device *dev) +{ +pruss_idx_writel (dev,PRUSS_INTC_STATIDXCLR ,PRUSS_CAN_ARM2DSP_SYS_EVT ); +pruss_idx_writel (dev,PRUSS_INTC_ENIDXSET ,PRUSS_CAN_ARM2DSP_SYS_EVT ); +pruss_idx_writel (dev,PRUSS_INTC_STATIDXSET ,PRUSS_CAN_ARM2DSP_SYS_EVT ); +return 0 ;
void function?
drivers/net/can/pruss_can.c line 320
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+} + +static void pru_can_mask_ints (struct device *dev,u32 trx,bool enable) +{ +u32 value =0 ;
not needed
drivers/net/can/pruss_can.c line 323
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+ + value = (trx ==PRUSS_CAN_TX_PRU_1 ) ? +PRUSS_CAN_TX_SYS_EVT :PRUSS_CAN_RX_SYS_EVT ;
use a struct with arrays for the register description
drivers/net/can/pruss_can.c line 325
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ enable ?pruss_idx_writel (dev,PRUSS_INTC_ENIDXSET , value) : +pruss_idx_writel (dev,PRUSS_INTC_ENIDXCLR , value);
if..else
drivers/net/can/pruss_can.c line 330
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+} + +static unsigned int pru_can_get_error_cnt (struct device *dev,u8 pruno) +{ +u32 value =0 ;
not needed
drivers/net/can/pruss_can.c line 337
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ +if (pruno ==PRUSS_CAN_RX_PRU_0 ) +pruss_readl (dev,PRUSS_CAN_RX_ERR_CNTR_REG , &value); +else if (pruno ==PRUSS_CAN_TX_PRU_1 ) +pruss_readl (dev,PRUSS_CAN_TX_ERR_CNTR_REG , &value); +else +return -EINVAL ;
remove the -EINVAL
drivers/net/can/pruss_can.c line 344
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ +return value; +} + +static unsigned int pru_can_get_intc_status (struct device *dev) +{ +u32 value =0 ;
not needed
drivers/net/can/pruss_can.c line 352
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ +pruss_readl (dev,PRUSS_INTC_STATCLRINT1 , &value); +return value; +} + +static void pru_can_clr_intc_status (struct device *dev,u32 trx) +{ +u32 value =0 ;
dito
drivers/net/can/pruss_can.c line 323
Quoted 3 lines
drivers/net/can/pruss_can.c new file +3
-0+ + value = (trx ==PRUSS_CAN_TX_PRU_1 ) ? +PRUSS_CAN_TX_SYS_EVT :PRUSS_CAN_RX_SYS_EVT ;
use a struct + array for the resiter desc
drivers/net/can/pruss_can.c line 363
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+pruss_idx_writel (dev,PRUSS_INTC_STATIDXCLR , value); +} + +static int pru_can_get_state (const struct net_device *ndev, +enum can_state *state) +{ +struct can_emu_priv *priv =netdev_priv (ndev); + *state = priv->can.state;
we don't implemnt this function anymore..
Quoted 31 lines
drivers/net/can/pruss_can.c new file +31
-0+ +return 0 ; +} + +static int pru_can_set_bittiming (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct can_bittiming *bt = &priv->can.bittiming; +u32 value; + + value = priv->can.clock.freq / bt->bitrate; +pruss_writel (priv->dev,PRUSS_CAN_TIMING_VAL_TX , value); +pruss_writel (priv->dev,PRUSS_CAN_BIT_TIMING_VAL_RX , value); + + value = (bt->phase_seg2 + bt->phase_seg1 + + bt->prop_seg +1 ) * bt->brp; + value = (value >>1 ) -PRUSS_CAN_TIMER_SETUP_DELAY ; + value = (value <<16 ) | value; +pruss_writel (priv->dev,PRUSS_CAN_TIMING_VAL_RX , value); + + value = (PRUSS_CAN_GPIO_SETUP_DELAY * + (priv->clk_freq_pru /1000000 ) /1000 ) / +PRUSS_CAN_DELAY_LOOP_LENGTH ; + +pruss_writel (priv->dev,PRUSS_CAN_TIMING_SETUP , value); +return 0 ; +} + +static void pru_can_stop (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev);
drivers/net/can/pruss_can.c line 404
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , false); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +pru_can_reset_tx (priv->dev); + priv->can.state =CAN_STATE_STOPPED ; +} + +/* +* This is to just set the can state to ERROR_ACTIVE +* ip link set canX up type can bitrate 125000
fix the comment
Quoted 42 lines
drivers/net/can/pruss_can.c new file +42
-0+*/ +static void pru_can_start (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); + +if (priv->can.state !=CAN_STATE_STOPPED ) +pru_can_stop (ndev); + +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , true); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , true); + +pru_can_gbl_stat_get (priv->dev, &priv->can_tx_cntx); +pru_can_gbl_stat_get (priv->dev, &priv->can_rx_cntx); + +if (PRUSS_CAN_GSR_BIT_EPM & priv->can_tx_cntx.gbl_stat) + priv->can.state =CAN_STATE_ERROR_PASSIVE ; +else if (PRUSS_CAN_GSR_BIT_BFM & priv->can_tx_cntx.gbl_stat) + priv->can.state =CAN_STATE_BUS_OFF ; +else + priv->can.state =CAN_STATE_ERROR_ACTIVE ; +} + +static int pru_can_set_mode (struct net_device *ndev,enum can_mode mode) +{ +int ret =0 ; + +switch (mode) { +case CAN_MODE_START : +pru_can_start (ndev); +netif_wake_queue (ndev); +break ; +default : + ret = -EOPNOTSUPP ; +break ; + } +return ret; +} + +static netdev_tx_t pru_can_start_xmit (struct sk_buff *skb, +struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev);
drivers/net/can/pruss_can.c line 456
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+struct can_frame *cf = (struct can_frame *)skb->data; +int count; +u8 *data = cf->data; +u8 dlc = cf->can_dlc; +u8 *pdata =NULL ; + +if (can_dropped_invalid_skb (ndev, skb)) +return NETDEV_TX_OK ; + +netif_stop_queue (ndev);
why do you stop the queue unconditionally here?
drivers/net/can/pruss_can.c line 469
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+if (cf->can_id &CAN_EFF_FLAG )/* Extended frame format */ + priv->can_tx_cntx.mbox.can_id = + (cf->can_id &CAN_EFF_MASK ) |PRUSS_CAN_MID_IDE ; +else /* Standard frame format */ + priv->can_tx_cntx.mbox.can_id = + (cf->can_id &CAN_SFF_MASK ) <<PRUSS_CAN_STD_FRAME_MASK ; + +if (cf->can_id &CAN_RTR_FLAG )/* Remote transmission request */ + priv->can_tx_cntx.mbox.can_id |=CAN_RTR_FLAG ; + + pdata = &priv->can_tx_cntx.mbox.data[0 ] + (dlc -1 ); +for (count =0 ; count < (u8 ) dlc; count++) + *pdata-- = *data++;
What does this loop do? endianess conversion? Please don't open code this.
drivers/net/can/pruss_can.c line 471
Quoted 2 lines
drivers/net/can/pruss_can.c new file +2
-0+ + priv->can_tx_cntx.mbox.datalength = (u16 ) dlc;
no need to cast
drivers/net/can/pruss_can.c line 480
Quoted 9 lines
drivers/net/can/pruss_can.c new file +9
-0+ priv->can_tx_cntx.mbox.crc =0 ; +/* +* search for the next available mbx +* if the next mbx is busy, then try the next + 1 +* do this until the head is reached. +* if still unable to tx, stop accepting any packets +* if able to tx and the head is reached, then reset next to tail, i.e mbx0 +* if head is not reached, then just point to the next mbx +*/
indention, please
Your tx algorithm looks fishy. You always use can_get_echo_skb(ndev, 0). This means you can have only 1 can frame on the fly. But you say you look for a free mailbox. You have to tx mailboxes in a defined order, otherwise your hardware/firmware is broken. If your hardware transmits frames in order, you always know which one will be the next free mailbox. You have a power of 2 number of mailboxes, you can simply apply a mask to get to the real mailbox number. No need for manual wrap around. Have a look at the at91_can tx sheme.
Activate the tx_interrupt, putting a can frame into a mailbox, stop the tx_queue if there are no free mailboxes, or in case of a wrap around. Reenable the tx_queue in the tx_complete interrupt handler.
drivers/net/can/pruss_can.c line 484
Quoted 4 lines
drivers/net/can/pruss_can.c new file +4
-0+for (; priv->tx_next <= priv->tx_head; priv->tx_next++) { + priv->can_tx_cntx.mboxno = priv->tx_next; +if (-1 ==pru_can_mbox_write (priv->dev, + &priv->can_tx_cntx)) {
this function will always return 0.
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+if (priv->tx_next == priv->tx_head) { + priv->tx_next = priv->tx_tail; +netif_stop_queue (ndev);/* IF stalled */ +dev_err (priv->dev, +"%s: no tx mbx available" , __func__); +return NETDEV_TX_BUSY ; + }else
drivers/net/can/pruss_can.c line 501
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+continue ; + }else { +/* set transmit request */ +pru_can_tx (priv->dev, priv->tx_next, +PRUSS_CAN_TX_PRU_1 ); +pru_can_tx_mode_set (priv->dev, false,RECEIVE ); +pru_can_tx_mode_set (priv->dev, true,TRANSMIT ); +pru_can_reset_tx (priv->dev); + priv->tx_next++; +can_put_echo_skb (skb, ndev,0 );
^^^
see comment above
Quoted 20 lines
drivers/net/can/pruss_can.c new file +20
-0+break ; + } + } +if (priv->tx_next > priv->tx_head) + priv->tx_next = priv->tx_tail; + +return NETDEV_TX_OK ; +} + +static int pru_can_rx (struct net_device *ndev,u32 mbxno) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; +u8 *data =NULL ; +u8 *pdata =NULL ; +int count =0 ; + + skb =alloc_can_skb (ndev, &cf);
drivers/net/can/pruss_can.c line 531
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+if (!skb) { +if (printk_ratelimit ()) +dev_err (priv->dev, +"alloc_can_skb() failed\n" ); +return -ENOMEM ; + } + data = cf->data; +/* get payload */ + priv->can_rx_cntx.mboxno = mbxno; +if (pru_can_mbox_read (priv->dev, &priv->can_rx_cntx)) {
function always returns 0!
drivers/net/can/pruss_can.c line 539
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+dev_err (priv->dev,"failed to get data from mailbox\n" ); +return -EAGAIN ; + } +/* give ownweship to pru */ +pru_can_tx (priv->dev, mbxno,PRUSS_CAN_RX_PRU_0 ); + +/* get data length code */ + cf->can_dlc =get_can_dlc (priv->can_rx_cntx.mbox.datalength &0xF );
This looks to complicated. Please state how the individual can bytes are placed in the mailbox, so that we can think of a simpler way to do this.
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+if (cf->can_dlc <=4 ) { + pdata = &priv->can_rx_cntx.mbox.data[4 ] + (4 - cf->can_dlc); +for (count =0 ; count < cf->can_dlc; count++) + *data++ = *pdata++; + }else { + pdata = &priv->can_rx_cntx.mbox.data[4 ]; +for (count =0 ; count <4 ; count++)
drivers/net/can/pruss_can.c line 556
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ *data++ = *pdata++; + pdata = &priv->can_rx_cntx.mbox.data[3 ] - (cf->can_dlc -5 ); +for (count =0 ; count < cf->can_dlc -4 ; count++) + *data++ = *pdata++; + } + +/* get id extended or std */ +if (priv->can_rx_cntx.mbox.can_id &PRUSS_CAN_MID_IDE ) + cf->can_id = (priv->can_rx_cntx.mbox.can_id &CAN_EFF_MASK ) + |CAN_EFF_FLAG ;
the usual way is to write the "|" at the end of the line.
drivers/net/can/pruss_can.c line 562
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+else + cf->can_id = (priv->can_rx_cntx.mbox.can_id >>18 ) + &CAN_SFF_MASK ; + +if (priv->can_rx_cntx.mbox.can_id &CAN_RTR_FLAG ) + cf->can_id |=CAN_RTR_FLAG ;
please don't copy any data to the can frame in case if an RTR message.
Quoted 112 lines
drivers/net/can/pruss_can.c new file +112
-0+ + stats->rx_bytes += cf->can_dlc; +netif_receive_skb (skb); + stats->rx_packets++; +return 0 ; +} + +static int pru_can_err (struct net_device *ndev,int int_status, +int err_status) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +struct can_frame *cf; +struct sk_buff *skb; +u32 tx_err_cnt, rx_err_cnt; + + skb =alloc_can_err_skb (ndev, &cf); +if (!skb) { +if (printk_ratelimit ()) +dev_err (priv->dev, +"alloc_can_err_skb() failed\n" ); +return -ENOMEM ; + } + +if (err_status &PRUSS_CAN_GSR_BIT_EPM ) {/* error passive int */ + priv->can.state =CAN_STATE_ERROR_PASSIVE ; + ++priv->can.can_stats.error_passive; + cf->can_id |=CAN_ERR_CRTL ; + tx_err_cnt =pru_can_get_error_cnt (priv->dev, +PRUSS_CAN_TX_PRU_1 ); + rx_err_cnt =pru_can_get_error_cnt (priv->dev, +PRUSS_CAN_RX_PRU_0 ); +if (tx_err_cnt >PRUSS_CAN_ERROR_ACTIVE -1 ) + cf->data[1 ] |=CAN_ERR_CRTL_TX_PASSIVE ; +if (rx_err_cnt >PRUSS_CAN_ERROR_ACTIVE -1 ) + cf->data[1 ] |=CAN_ERR_CRTL_RX_PASSIVE ; + +dev_dbg (priv->ndev->dev.parent,"Error passive interrupt\n" ); + } + +if (err_status &PRUSS_CAN_GSR_BIT_BFM ) { + priv->can.state =CAN_STATE_BUS_OFF ; + cf->can_id |=CAN_ERR_BUSOFF ; +/* +* Disable all interrupts in bus-off to avoid int hog +* this should be handled by the pru +*/ +pru_can_mask_ints (priv->dev,PRUSS_CAN_TX_PRU_1 , false); +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +can_bus_off (ndev); +dev_dbg (priv->ndev->dev.parent,"Bus off mode\n" ); + } + +netif_rx (skb); + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +return 0 ; +} + +static int pru_can_rx_poll (struct napi_struct *napi,int quota) +{ +struct net_device *ndev = napi->dev; +struct can_emu_priv *priv =netdev_priv (ndev); +u32 bit_set, mbxno =0 ; +u32 num_pkts =0 ; + +if (!netif_running (ndev)) +return 0 ; + +do { +/* rx int sys_evt -> 33 */ +pru_can_clr_intc_status (priv->dev,PRUSS_CAN_RX_PRU_0 ); +if (pru_can_intr_stat_get (priv->dev, &priv->can_rx_cntx)) +return num_pkts; + +if (PRUSS_CAN_ISR_BIT_RRI & priv->can_rx_cntx.intr_stat) { + mbxno =PRUSS_CAN_RTR_BUFF_NUM ; +pru_can_rx (ndev, mbxno); + num_pkts++; + }else { +/* Extract the mboxno from the status */ + bit_set =fls (priv->can_rx_cntx.intr_stat &0xFF ); +if (bit_set) { + num_pkts++; + mbxno = bit_set -1 ; +if (PRUSS_CAN_ISR_BIT_ESI & priv->can_rx_cntx. + intr_stat) { +pru_can_gbl_stat_get (priv->dev, + &priv->can_rx_cntx); +pru_can_err (ndev, + priv->can_rx_cntx.intr_stat, + priv->can_rx_cntx.gbl_stat); + }else +pru_can_rx (ndev, mbxno); + }else +break ; + } + }while (((PRUSS_CAN_TX_INT_STAT &pru_can_get_intc_status (priv->dev)) + && (num_pkts < quota))); + +/* Enable packet interrupt if all pkts are handled */ +if (!(PRUSS_CAN_TX_INT_STAT &pru_can_get_intc_status (priv->dev))) { +napi_complete (napi); +/* Re-enable RX mailbox interrupts */ +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , true); + } +return num_pkts; +} + +static irqreturn_t pru_tx_can_intr (int irq,void *dev_id) +{ +struct net_device *ndev = dev_id;
drivers/net/can/pruss_can.c line 684
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+struct can_emu_priv *priv =netdev_priv (ndev); +struct net_device_stats *stats = &ndev->stats; +u32 bit_set, mbxno; + +pru_can_intr_stat_get (priv->dev, &priv->can_tx_cntx); +if ((PRUSS_CAN_ISR_BIT_CCI & priv->can_tx_cntx.intr_stat) + || (PRUSS_CAN_ISR_BIT_SRDI & priv->can_tx_cntx.intr_stat)) { +dev_dbg (priv->ndev->dev.parent,"tx_int_status = 0x%X\n" , + priv->can_tx_cntx.intr_stat); +can_free_echo_skb (ndev,0 );
^^^
make no sense if using multiple tx mailboxes
drivers/net/can/pruss_can.c line 690
Quoted 6 lines
drivers/net/can/pruss_can.c new file +6
-0+ }else { + bit_set =fls (priv->can_tx_cntx.intr_stat &0xFF ); +if (!bit_set) { +dev_err (priv->dev,"%s: invalid mailbox number\n" , + __func__); +can_free_echo_skb (ndev,0 );
^^^^
drivers/net/can/pruss_can.c line 703
Quoted 13 lines
drivers/net/can/pruss_can.c new file +13
-0+ }else { + mbxno = bit_set -1 ; +if (PRUSS_CAN_ISR_BIT_ESI & priv->can_tx_cntx. + intr_stat) { +/* read gsr and ack pru */ +pru_can_gbl_stat_get (priv->dev, + &priv->can_tx_cntx); +pru_can_err (ndev, priv->can_tx_cntx.intr_stat, + priv->can_tx_cntx.gbl_stat); + }else { + stats->tx_packets++; +/* stats->tx_bytes += dlc; */ +/*can_get_echo_skb(ndev, 0);*/
??
drivers/net/can/pruss_can.c line 708
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+ } + } + } +netif_wake_queue (ndev); +can_get_echo_skb (ndev,0 );
again?
drivers/net/can/pruss_can.c line 713
Quoted 5 lines
drivers/net/can/pruss_can.c new file +5
-0+pru_can_tx_mode_set (priv->dev, true,RECEIVE ); +return IRQ_HANDLED ; +} + +static irqreturn_t pru_rx_can_intr (int irq,void *dev_id)
why is this function calles rx_can_intr it's a generic interrupt function..
drivers/net/can/pruss_can.c line 724
Quoted 11 lines
drivers/net/can/pruss_can.c new file +11
-0+{ +struct net_device *ndev = dev_id; +struct can_emu_priv *priv =netdev_priv (ndev); +u32 intc_status =0 ; + + intc_status =pru_can_get_intc_status (priv->dev); + +/* tx int sys_evt -> 34 */ +if (intc_status &4 ) { +pru_can_clr_intc_status (priv->dev,PRUSS_CAN_TX_PRU_1 ); +return pru_tx_can_intr (irq, dev_id);
why are you returning here? is is possible the you have a can frame to receivce?
drivers/net/can/pruss_can.c line 732
Quoted 8 lines
drivers/net/can/pruss_can.c new file +8
-0+ } +/* Disable RX mailbox interrupts and let NAPI reenable them */ +if (intc_status &2 ) { +pru_can_mask_ints (priv->dev,PRUSS_CAN_RX_PRU_0 , false); +napi_schedule (&priv->napi); + } + +return IRQ_HANDLED ;
^^^^^^^^^^^^
that might not be true....
drivers/net/can/pruss_can.c line 742
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+} + +static int pru_can_open (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); +s32 err; + +/* register interrupt handler */ + err =request_irq (priv->trx_irq, &pru_rx_can_intr,IRQF_SHARED , +"pru_can_irq" , ndev);
use dev->name for interrupt name
Quoted 61 lines
drivers/net/can/pruss_can.c new file +61
-0+if (err) { +dev_err (priv->dev,"error requesting rx interrupt\n" ); + goto exit_trx_irq; + } + err =open_candev (ndev); +if (err) { +dev_err (priv->dev,"open_candev() failed %d\n" , err); + goto exit_open; + } + +pru_can_emu_init (priv->dev, priv->can.clock.freq); + priv->tx_tail =PRUSS_CAN_MB_MIN ; + priv->tx_head =PRUSS_CAN_MB_MAX ; +pru_can_start (ndev); +napi_enable (&priv->napi); +netif_start_queue (ndev); +return 0 ; + +exit_open: +free_irq (priv->trx_irq, ndev); +exit_trx_irq: +return err; +} + +static int pru_can_close (struct net_device *ndev) +{ +struct can_emu_priv *priv =netdev_priv (ndev); + +netif_stop_queue (ndev); +napi_disable (&priv->napi); +close_candev (ndev); +free_irq (priv->trx_irq, ndev); +return 0 ; +} + +static const struct net_device_ops pru_can_netdev_ops = { + .ndo_open = pru_can_open, + .ndo_stop = pru_can_close, + .ndo_start_xmit = pru_can_start_xmit, +}; + +/* Shows all the mailbox IDs */ +static ssize_t pru_sysfs_mbx_id_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct can_emu_priv *priv =netdev_priv (to_net_dev (dev)); + +return snprintf (buf,PAGE_SIZE ,"<mbx_no:mbx_id>\n" +"0:0x%X 1:0x%X 2:0x%X 3:0x%X " +"4:0x%X 5:0x%X 6:0x%X 7:0x%X\n" , + priv->mbx_id[0 ], priv->mbx_id[1 ], + priv->mbx_id[2 ], priv->mbx_id[3 ], + priv->mbx_id[4 ], priv->mbx_id[5 ], + priv->mbx_id[6 ], priv->mbx_id[7 ]); +} + +/* +* Sets Mailbox IDs +* This should be programmed as mbx_num:mbx_id (in hex) +* eg: $ echo 0:0x123 > /sys/class/net/can0/mbx_id +*/
drivers/net/can/pruss_can.c line 813
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static ssize_t pru_sysfs_mbx_id_set (struct device *dev, +struct device_attribute *attr,const char *buf,size_t count) +{ +struct net_device *ndev =to_net_dev (dev); +struct can_emu_priv *priv =netdev_priv (ndev); +unsigned long can_id; +unsigned long mbx_num; +char mbx[2 ] = {*buf,'\0' };/* mbx num */ +ssize_t ret = count; +s32 err;
I think you have to lock here:rtnl_lock();
Quoted 49 lines
drivers/net/can/pruss_can.c new file +49
-0+ +if (ndev->flags &IFF_UP ) { + ret = -EBUSY ; + goto out; + } + +if (*(buf +1 ) !=':' ) { + ret = -EINVAL ; + goto out; + } + + err =strict_strtoul (mbx,0 , &mbx_num); +if (err) { + ret = err; + goto out; + } + +if (mbx_num >7 ) { + ret = -EINVAL ; + goto out; + } + + err =strict_strtoul ((buf +2 ),0 , &can_id); +if (err) { + ret = err; + goto out; + } + + priv->mbx_id[mbx_num] = can_id; +pru_can_rx_id_set (priv->dev, priv->mbx_id[mbx_num], mbx_num); + +return ret; +out: +dev_err (priv->dev,"invalid buffer format\n" ); +return ret; +} + +static DEVICE_ATTR(mbx_id, S_IWUSR | S_IRUGO, + pru_sysfs_mbx_id_show, pru_sysfs_mbx_id_set); + +static struct attribute *pru_sysfs_attrs[] = { + &dev_attr_mbx_id.attr, +NULL , +}; + +static struct attribute_group pru_sysfs_attr_group = { + .attrs = pru_sysfs_attrs, +}; +
drivers/net/can/pruss_can.c line 872
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+static int __devinit pru_can_probe (struct platform_device *pdev) +{ +struct net_device *ndev =NULL ; +const struct da850_evm_pruss_can_data *pdata; +struct can_emu_priv *priv =NULL ; +struct device *dev = &pdev->dev; +struct clk *clk_pruss; +const struct firmware *fw_rx; +const struct firmware *fw_tx; +u32 err;
use int
drivers/net/can/pruss_can.c line 879
Quoted 7 lines
drivers/net/can/pruss_can.c new file +7
-0+ + pdata = dev->platform_data; +if (!pdata) { +dev_err (&pdev->dev,"platform data not found\n" ); +return -EINVAL ; + } + (pdata->setup)();
no need fot the ( )
Quoted 22 lines
drivers/net/can/pruss_can.c new file +22
-0+ + ndev =alloc_candev (sizeof (struct can_emu_priv ),PRUSS_CAN_MB_MAX +1 ); +if (!ndev) { +dev_err (&pdev->dev,"alloc_candev failed\n" ); + err = -ENOMEM ; + goto probe_exit; + } + + ndev->sysfs_groups[0 ] = &pru_sysfs_attr_group; + + priv =netdev_priv (ndev); + + priv->trx_irq =platform_get_irq (to_platform_device (dev->parent),0 ); +if (!priv->trx_irq) { +dev_err (&pdev->dev,"unable to get pru " +"interrupt resources!\n" ); + err = -ENODEV ; + goto probe_exit; + } + + priv->ndev = ndev; + priv->dev = dev;
drivers/net/can/pruss_can.c line 911
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ + priv->can.bittiming_const = &pru_can_bittiming_const; + priv->can.do_set_bittiming = pru_can_set_bittiming; + priv->can.do_set_mode = pru_can_set_mode; + priv->can.do_get_state = pru_can_get_state; + priv->can_tx_cntx.pruno =PRUSS_CAN_TX_PRU_1 ; + priv->can_rx_cntx.pruno =PRUSS_CAN_RX_PRU_0 ; + +/* we support local echo, no arp */ + ndev->flags |= (IFF_ECHO |IFF_NOARP );
no need to se NOARP
Quoted 14 lines
drivers/net/can/pruss_can.c new file +14
-0+ +/* pdev->dev->device_private->driver_data = ndev */ +platform_set_drvdata (pdev, ndev); +SET_NETDEV_DEV (ndev, &pdev->dev); + ndev->netdev_ops = &pru_can_netdev_ops; + + priv->clk_timer =clk_get (&pdev->dev,"pll1_sysclk2" ); +if (IS_ERR (priv->clk_timer)) { +dev_err (&pdev->dev,"no timer clock available\n" ); + err =PTR_ERR (priv->clk_timer); + priv->clk_timer =NULL ; + goto probe_exit_candev; + } +
drivers/net/can/pruss_can.c line 935
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ priv->can.clock.freq =clk_get_rate (priv->clk_timer); + + clk_pruss =clk_get (NULL ,"pruss" ); +if (IS_ERR (clk_pruss)) { +dev_err (&pdev->dev,"no clock available: pruss\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + priv->clk_freq_pru =clk_get_rate (clk_pruss); +clk_put (clk_pruss);
why do you put the clock here?
Quoted 23 lines
drivers/net/can/pruss_can.c new file +23
-0+ + err =register_candev (ndev); +if (err) { +dev_err (&pdev->dev,"register_candev() failed\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + + err =request_firmware (&fw_tx,"PRU_CAN_Emulation_Tx.bin" , + &pdev->dev); +if (err) { +dev_err (&pdev->dev,"can't load firmware\n" ); + err = -ENODEV ; + goto probe_exit_clk; + } + +dev_info (&pdev->dev,"fw_tx size %d. downloading...\n" , fw_tx->size); + + err =request_firmware (&fw_rx,"PRU_CAN_Emulation_Rx.bin" , + &pdev->dev); +if (err) { +dev_err (&pdev->dev,"can't load firmware\n" ); + err = -ENODEV ;
drivers/net/can/pruss_can.c line 968
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+ goto probe_release_fw; + } +dev_info (&pdev->dev,"fw_rx size %d. downloading...\n" , fw_rx->size); + +/* init the pru */ +pru_can_emu_init (priv->dev, priv->can.clock.freq); +udelay (200 ); + +netif_napi_add (ndev, &priv->napi, pru_can_rx_poll, +PRUSS_DEF_NAPI_WEIGHT );
personally I'd wait to add the interface to napi until the firmware is loaded.
Quoted 96 lines
drivers/net/can/pruss_can.c new file +96
-0+ +pruss_enable (priv->dev,PRUSS_CAN_RX_PRU_0 ); +pruss_enable (priv->dev,PRUSS_CAN_TX_PRU_1 ); + +/* download firmware into pru */ + err =pruss_load (priv->dev,PRUSS_CAN_RX_PRU_0 , + (u32 *)fw_rx->data, (fw_rx->size /4 )); +if (err) { +dev_err (&pdev->dev,"firmware download error\n" ); + err = -ENODEV ; + goto probe_release_fw_1; + } +release_firmware (fw_rx); + err =pruss_load (priv->dev,PRUSS_CAN_TX_PRU_1 , + (u32 *)fw_tx->data, (fw_tx->size /4 )); +if (err) { +dev_err (&pdev->dev,"firmware download error\n" ); + err = -ENODEV ; + goto probe_release_fw_1; + } +release_firmware (fw_tx); + +pruss_run (priv->dev,PRUSS_CAN_RX_PRU_0 ); +pruss_run (priv->dev,PRUSS_CAN_TX_PRU_1 ); + +dev_info (&pdev->dev, +"%s device registered (trx_irq = %d, clk = %d)\n" , +PRUSS_CAN_DRV_NAME , priv->trx_irq, priv->can.clock.freq); + +return 0 ; + +probe_release_fw_1: +release_firmware (fw_rx); +probe_release_fw: +release_firmware (fw_tx); +probe_exit_clk: +clk_put (priv->clk_timer); +probe_exit_candev: +if (NULL != ndev) +free_candev (ndev); +probe_exit: +return err; +} + +static int __devexit pru_can_remove(struct platform_device *pdev) +{ +struct net_device *ndev =platform_get_drvdata (pdev); +struct can_emu_priv *priv =netdev_priv (ndev); + +pru_can_stop (ndev); +pru_can_emu_exit (priv->dev); +clk_put (priv->clk_timer); +unregister_candev (ndev); +free_candev (ndev); +platform_set_drvdata (pdev,NULL ); +return 0 ; +} + +#ifdef CONFIG_PM +static int pru_can_suspend (struct platform_device *pdev, +pm_message_t mesg) +{ +dev_info (&pdev->dev,"%s not yet implemented\n" , __func__); +return 0 ; +} + +static int pru_can_resume (struct platform_device *pdev) +{ +dev_info (&pdev->dev,"%s not yet implemented\n" , __func__); +return 0 ; +} +#else +#define pru_can_suspend NULL +#define pru_can_resume NULL +#endif + +static struct platform_driver omapl_pru_can_driver = { + .probe = pru_can_probe, + .remove =__devexit_p (pru_can_remove), + .suspend = pru_can_suspend, + .resume = pru_can_resume, + .driver = { + .name =PRUSS_CAN_DRV_NAME , + .owner =THIS_MODULE , + }, +}; + +static int __init pru_can_init (void ) +{ + pr_debug(KERN_INFO PRUSS_CAN_DRV_DESC "\n"); + return platform_driver_register(&omapl_pru_can_driver); +} + +module_init (pru_can_init); + +static void __exit pru_can_exit(void)
drivers/net/can/pruss_can.c line 1074
Quoted 10 lines
drivers/net/can/pruss_can.c new file +10
-0+{ + pr_debug(KERN_INFO PRUSS_CAN_DRV_DESC " unloaded\n"); +platform_driver_unregister (&omapl_pru_can_driver); +} + +module_exit (pru_can_exit); + +MODULE_AUTHOR ("Subhasish Ghosh <[email protected]>" ); +MODULE_LICENSE ("GPL" ); +MODULE_DESCRIPTION ("omapl pru CAN netdevice driver" );
regards, 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 |