Re: [PATCH v4 1/1] can: add pruss CAN driver.

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 40604 bytes --]

I'm not sure whether this message made it to the netdev mailinglist.
This is a resend.

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 <subhasish@mistralsolutions.com>
> ---
>  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: subhasish@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
> +#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 <subhasish@mistralsolutions.com>");
> +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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]