On 04/22/2011 02:11 PM, Subhasish Ghosh wrote:
> 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

> 
> 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.c
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 5dec456..4682a4f 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -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

> +
>  source "drivers/net/can/mscan/Kconfig"
>  
>  source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 53c82a7..d0b7cbd 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -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
> diff --git a/drivers/net/can/pruss_can.c b/drivers/net/can/pruss_can.c
> new file mode 100644
> index 0000000..7702509
> --- /dev/null
> +++ b/drivers/net/can/pruss_can.c
> @@ -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
> +#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.

> +#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)
> +#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

> +};
> +
> +struct can_mbox {

I suggest to add a namespace to these structs, e.g. pru_can_mbox. Same
comment applies to the other structs.

> +     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__

> +     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;
> +};
> +
> +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.

> +     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,
> +     .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

> +
> +     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

> +
> +     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()

> +
> +     return 0;

why do you return 0 here? pruss_readl_multi is not void, although it
always returns 0, too. consider make all void.
> +}
> +
> +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.
> +}
> +
> +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.

> +     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.

> +
> +     return 0;
> +}
> +
> +static int pru_can_gbl_stat_get(struct device *dev,
> +             struct can_emu_cntx *emu_cntx)
> +{
> +     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.

> +}
> +
> +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) {
> +             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.

> +             return -EINVAL;
> +
> +     return 0;
> +}
> +

is this array const?
> +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}

please add ","

> +};
> +
> +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.
> +
> +     /* PRU Internal Registers Initializations */
> +     pruss_writel_multi(dev, PRUSS_CAN_TX_MBOX0_STAT_REG, value, 8);

use sizeof(), or ARRAY_SIZE

> +
> +     *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);

> +
> +     *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
> +             (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

> +     return 0;
> +}
> +
> +static void pru_can_emu_exit(struct device *dev)
> +{
> +     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)
> +             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

> +     } 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

> +     } else
> +             return -EINVAL;

trust your own code, get rid of the -EINVAL, make this a void function.

> +     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?
> +}
> +
> +static void pru_can_mask_ints(struct device *dev, u32 trx, bool enable)
> +{
> +     u32 value = 0;

not needed
> +
> +     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

> +     enable ? pruss_idx_writel(dev, PRUSS_INTC_ENIDXSET, value) :
> +             pruss_idx_writel(dev, PRUSS_INTC_ENIDXCLR, value);

if..else
> +}
> +
> +static unsigned int pru_can_get_error_cnt(struct device *dev, u8 pruno)
> +{
> +     u32 value = 0;
not needed
> +
> +     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

> +
> +     return value;
> +}
> +
> +static unsigned int pru_can_get_intc_status(struct device *dev)
> +{
> +     u32 value = 0;
not needed
> +
> +     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
> +
> +     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

> +     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..
> +
> +     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);
> +
> +     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

> + */
> +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);
> +     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?


> +     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.

> +
> +     priv->can_tx_cntx.mbox.datalength = (u16) dlc;
no need to cast

> +     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.

> +     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.

> +                     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
> +                             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

> +                     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);
> +     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!

> +             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.

> +     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++)
> +                     *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.

> +     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.

> +
> +     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;
> +     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

> +     } 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);
                                               ^^^^
> +             } 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);*/

??

> +                     }
> +             }
> +     }
> +     netif_wake_queue(ndev);
> +     can_get_echo_skb(ndev, 0);
again?
> +     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..

> +{
> +     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?
> +     }
> +     /* 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....

> +}
> +
> +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

> +     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
> + */
> +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();
> +
> +     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,
> +};
> +
> +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
> +
> +     pdata = dev->platform_data;
> +     if (!pdata) {
> +             dev_err(&pdev->dev, "platform data not found\n");
> +             return -EINVAL;
> +     }
> +     (pdata->setup)();

no need fot the ( )

> +
> +     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;
> +
> +     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

> +
> +     /* 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;
> +     }
> +
> +     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?
> +
> +     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;
> +             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.

> +
> +     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)
> +{
> +     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

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core

Reply via email to