Re: [PATCH 00/16] Support for Eberspächer Flexcard DCAN function

Re: [PATCH 00/16] Support for Eberspächer Flexcard DCAN function

[Marc Kleine-Budde]

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

Hey Bene,

On 09/09/2013 09:24 AM, Benedikt Spranger wrote:
> The Eberspächer Flexcard is a multifunctional PCI Mezzanine Card. This
> patch-series adds support for the DCAN function. The Flexcard supports
> up too 8 CAN devices and has some CAN related special features:
> - own FIFO per device
> - DMA for receiving CAN frames
> - Firmware tracks CAN echo packets

Please include linux-can@vger.kernel.org when posting CAN related patches.

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: 259 bytes --]

Re: [PATCH 01/16] c_can_platform: add FlexCard D-CAN support

[Marc Kleine-Budde]

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

On 09/09/2013 09:24 AM, Benedikt Spranger wrote:
> FlexCard supports up to 8 D-CAN devices with a shared DMA-capable receive
> function. Add support for FlexCard D-CAN.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>

What are the prerequisites for this series? This doesn't compile against
current net-next/master (e7d33bb lockref: add ability to mark lockrefs
"dead").

More Comments inline.
Marc

> ---
>  drivers/net/can/c_can/c_can.h          |   1 +
>  drivers/net/can/c_can/c_can_platform.c | 149 ++++++++++++++++++++++++++++++++-
>  2 files changed, 148 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index d2e1c21..d2e2d20 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -148,6 +148,7 @@ enum c_can_dev_id {
>  	BOSCH_C_CAN_PLATFORM,
>  	BOSCH_C_CAN,
>  	BOSCH_D_CAN,
> +	BOSCH_D_CAN_FLEXCARD,
>  };
>  
>  /* c_can private data structure */
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index c6f838d..43a3e3f 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -32,6 +32,7 @@
>  #include <linux/clk.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/flexcard.h>
>  
>  #include <linux/can/dev.h>
>  
> @@ -81,6 +82,112 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
>  	writel(val, priv->raminit_ctrlreg);
>  }
>  
> +static int c_can_rx_pkt(void *p, void *data, size_t len)
> +{

Why are the first two arguments a void *, why do we have len, that's
never used? Has the length already been checked, is it guaranteed >=
sizeof(struct fc_packet_buf)?

> +	struct net_device *dev = p;
> +	struct net_device_stats *stats = &dev->stats;
> +	struct fc_packet_buf *pb = data;
> +	union fc_packet_types *pt = &pb->packet;
> +	struct can_frame *frame;
> +	struct sk_buff *skb;
> +	u32 flags, id, state, type;
> +
> +	switch (le32_to_cpu(pb->header.type)) {
> +	case fc_packet_type_can:
> +		skb = alloc_can_skb(dev, &frame);
> +		if (!skb) {
> +			stats->rx_dropped++;
> +			return -ENOMEM;
> +		}
> +
> +		id = le32_to_cpu(pt->can_packet.id);
> +		flags = le32_to_cpu(pt->can_packet.flags);
> +
> +		if (id & BIT(29))
> +			frame->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +		else
> +			frame->can_id = id & CAN_SFF_MASK;
> +
> +		if (flags & BIT(13))
> +			frame->can_id |= CAN_RTR_FLAG;
> +		frame->can_dlc = (flags >> 8) & 0xf;

please use get_can_dlc((flags >> 8) & 0xf)

> +		memcpy(frame->data, pt->can_packet.data, frame->can_dlc);

Please don't copy data if you receive an RTR frame. What about the
endianess of the data?

> +		netif_receive_skb(skb);
> +
> +		stats->rx_packets++;
> +		stats->rx_bytes += frame->can_dlc;
> +		break;
> +
> +	case fc_packet_type_can_error:
> +		stats->rx_errors++;
> +
> +		skb = alloc_can_err_skb(dev, &frame);
> +		if (!skb)
> +			return -ENOMEM;
> +
> +		type = le32_to_cpu(pt->can_error_packet.type);
> +		state = le32_to_cpu(pt->can_error_packet.state);
> +
> +		switch (state) {
> +		case fc_can_state_warning:
> +			frame->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> +			frame->data[1] |= CAN_ERR_CRTL_TX_WARNING;
> +			frame->can_id |= CAN_ERR_CRTL;
> +			break;
> +		case fc_can_state_error_passive:
> +			frame->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> +			frame->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> +			frame->can_id |= CAN_ERR_CRTL;
> +			break;
> +		case fc_can_state_bus_off:
> +			frame->can_id |= CAN_ERR_BUSOFF;
> +			break;
> +		}
> +
> +		switch (type) {
> +		case fc_can_error_stuff:
> +			frame->data[2] |= CAN_ERR_PROT_STUFF;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_form:
> +			frame->data[2] |= CAN_ERR_PROT_FORM;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_acknowledge:
> +			frame->can_id |= CAN_ERR_ACK;
> +			break;
> +		case fc_can_error_bit1:
> +			frame->data[2] |= CAN_ERR_PROT_BIT1;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_bit0:
> +			frame->data[2] |= CAN_ERR_PROT_BIT0;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_crc:
> +			frame->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_parity:
> +			frame->data[1] |= CAN_ERR_PROT_UNSPEC;
> +			frame->can_id |= CAN_ERR_CRTL;
> +			break;
> +		}
> +		frame->data[5] = pt->can_error_packet.rx_error_counter;
> +		frame->data[6] = pt->can_error_packet.tx_error_counter;
> +
> +		stats->rx_bytes += frame->can_dlc;
> +		stats->rx_packets++;
> +
> +		netif_receive_skb(skb);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct platform_device_id c_can_id_table[] = {
>  	[BOSCH_C_CAN_PLATFORM] = {
>  		.name = KBUILD_MODNAME,
> @@ -93,6 +200,10 @@ static struct platform_device_id c_can_id_table[] = {
>  	[BOSCH_D_CAN] = {
>  		.name = "d_can",
>  		.driver_data = BOSCH_D_CAN,
> +	},
> +	[BOSCH_D_CAN_FLEXCARD] = {
> +		.name = "flexcard-d_can",
> +		.driver_data = BOSCH_D_CAN_FLEXCARD,
>  	}, {
>  	}
>  };
> @@ -108,7 +219,7 @@ MODULE_DEVICE_TABLE(of, c_can_of_table);
>  static int c_can_plat_probe(struct platform_device *pdev)
>  {
>  	int ret;
> -	void __iomem *addr;
> +	void __iomem *addr, *reset_reg;
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
>  	const struct of_device_id *match;
> @@ -167,6 +278,8 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	}
>  
>  	priv = netdev_priv(dev);
> +	priv->can.clock.freq = clk_get_rate(clk);
> +
>  	switch (id->driver_data) {
>  	case BOSCH_C_CAN:
>  		priv->regs = reg_map_c_can;
> @@ -199,7 +312,26 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  			dev_info(&pdev->dev, "control memory is not used for raminit\n");
>  		else
>  			priv->raminit = c_can_hw_raminit;
> +

Please remove

>  		break;
> +	case BOSCH_D_CAN_FLEXCARD:
> +		priv->regs = reg_map_d_can;
> +		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> +		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
> +		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
> +		priv->instance = pdev->id;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		reset_reg = ioremap(res->start, resource_size(res));
> +		if (!reset_reg || priv->instance < 0) {

priv->instance in unsinged

> +			dev_info(&pdev->dev, "Can't reset device.\n");
> +		} else {
> +			writel(1 << (4 + priv->instance), addr);
> +			udelay(100);
> +		}
> +		iounmap(reset_reg);
> +		priv->can.clock.freq = 16000000;
> +
>  	default:
>  		ret = -EINVAL;
>  		goto exit_free_device;
> @@ -208,7 +340,6 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	dev->irq = irq;
>  	priv->base = addr;
>  	priv->device = &pdev->dev;
> -	priv->can.clock.freq = clk_get_rate(clk);
>  	priv->priv = clk;
>  	priv->type = id->driver_data;
>  
> @@ -222,10 +353,22 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  		goto exit_free_device;
>  	}
>  
> +	if (id->driver_data == BOSCH_D_CAN_FLEXCARD) {
> +		ret = fc_register_rx_pkt(FC_CANIF_OFFSET + pdev->id,
> +				dev, c_can_rx_pkt);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +					"registering RX-CB failed %d\n", ret);
> +			goto exit_unregister_device;
> +		}
> +	}
> +
>  	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
>  		 KBUILD_MODNAME, priv->base, dev->irq);
>  	return 0;
>  
> +exit_unregister_device:
> +	unregister_c_can_dev(dev);
>  exit_free_device:
>  	free_c_can_dev(dev);
>  exit_iounmap:
> @@ -246,6 +389,8 @@ static int c_can_plat_remove(struct platform_device *pdev)
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct resource *mem;
>  
> +	if (priv->type == BOSCH_D_CAN_FLEXCARD)
> +		fc_unregister_rx_pkt(FC_CANIF_OFFSET + priv->instance);
>  	unregister_c_can_dev(dev);
>  
>  	free_c_can_dev(dev);
> 

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: 259 bytes --]

Re: [PATCH 02/16] c_can: add generic D-CAN RAM initialization support

[Marc Kleine-Budde]

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

On 09/09/2013 09:24 AM, Benedikt Spranger wrote:
> D-CAN provides an automatic RAM initialization. The initialization can be
> executed while D-CAN is in the init state.
> 
> It should be checked if this is compatible with TI's d_can implementation
> which does not have or does not use this bit which is here according to the
> d_can specification.

Added AnilKumar Ch to the loop, who contributed the d_can support for
TI. Bene please collect a Tested-by for this patch.

Marc

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 38 ++++++++++++++++++++++++++++++++++++--
>  drivers/net/can/c_can/c_can.h |  2 ++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index a668cd4..081620b 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -68,6 +68,8 @@
>  #define TEST_SILENT		BIT(3)
>  #define TEST_BASIC		BIT(2)
>  
> +#define FUNC_RAMINIT		BIT(3)
> +
>  /* status register */
>  #define STATUS_PDA		BIT(10)
>  #define STATUS_BOFF		BIT(7)
> @@ -573,6 +575,7 @@ static int c_can_set_bittiming(struct net_device *dev)
>  	u32 ten_bit_brp;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	const struct can_bittiming *bt = &priv->can.bittiming;
> +	int retry;
>  
>  	/* c_can provides a 6-bit brp and 4-bit brpe fields */
>  	ten_bit_brp = bt->brp - 1;
> @@ -590,11 +593,42 @@ static int c_can_set_bittiming(struct net_device *dev)
>  		"setting BTR=%04x BRPE=%04x\n", reg_btr, reg_brpe);
>  
>  	ctrl_save = priv->read_reg(priv, C_CAN_CTRL_REG);
> -	priv->write_reg(priv, C_CAN_CTRL_REG,
> -			ctrl_save | CONTROL_CCE | CONTROL_INIT);
> +	ctrl_save &= ~CONTROL_INIT;
> +
> +	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_CCE | CONTROL_INIT);
> +	retry = 1000;
> +	while (!(priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_INIT)) {
> +		if (!retry) {
> +			netdev_info(dev, "CCTRL: set CONTROL_INIT failed\n");
> +			return -EIO;
> +		}
> +		udelay(10);
> +		retry--;
> +	}
>  	priv->write_reg(priv, C_CAN_BTR_REG, reg_btr);
>  	priv->write_reg(priv, C_CAN_BRPEXT_REG, reg_brpe);
> +
> +	priv->write_reg(priv, C_CAN_FUNC_REG, FUNC_RAMINIT);
> +	retry = 1000;
> +	while (priv->read_reg(priv, C_CAN_FUNC_REG) & FUNC_RAMINIT) {
> +		if (!retry) {
> +			netdev_info(dev, "CFR: FUNC_RAMINIT failed\n");
> +			return -EIO;
> +		}
> +		udelay(10);
> +		retry--;
> +	}
> +
>  	priv->write_reg(priv, C_CAN_CTRL_REG, ctrl_save);
> +	retry = 1000;
> +	while (priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_INIT) {
> +		if (!retry) {
> +			netdev_info(dev, "CCTRL: clear CONTROL_INIT failed\n");
> +			return -EIO;
> +		}
> +		udelay(10);
> +		retry--;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index d2e2d20..9f0eda8 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -61,6 +61,7 @@ enum reg {
>  	C_CAN_INTPND2_REG,
>  	C_CAN_MSGVAL1_REG,
>  	C_CAN_MSGVAL2_REG,
> +	C_CAN_FUNC_REG,
>  };
>  
>  static const u16 reg_map_c_can[] = {
> @@ -112,6 +113,7 @@ static const u16 reg_map_d_can[] = {
>  	[C_CAN_BRPEXT_REG]	= 0x0E,
>  	[C_CAN_INT_REG]		= 0x10,
>  	[C_CAN_TEST_REG]	= 0x14,
> +	[C_CAN_FUNC_REG]	= 0x18,
>  	[C_CAN_TXRQST1_REG]	= 0x88,
>  	[C_CAN_TXRQST2_REG]	= 0x8A,
>  	[C_CAN_NEWDAT1_REG]	= 0x9C,
> 


-- 
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: 259 bytes --]

Re: [PATCH 03/16] c_can: simplify arbitration register handling

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The IF1/2 Arbitration register of the C_CAN/D_CAN ip core is internaly
> a 32bit register. Do not use two different 16bit variables to handle
> the IF1/2 Arbitration register.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>

Looks good, nitpit inline.

Marc
> ---
>  drivers/net/can/c_can/c_can.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 081620b..bdb7bcd 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -117,9 +117,9 @@
>  				IF_COMM_DATAA | IF_COMM_DATAB)
>  
>  /* IFx arbitration */
> -#define IF_ARB_MSGVAL		BIT(15)
> -#define IF_ARB_MSGXTD		BIT(14)
> -#define IF_ARB_TRANSMIT		BIT(13)
> +#define IF_ARB_MSGVAL		BIT(31)
> +#define IF_ARB_MSGXTD		BIT(30)
> +#define IF_ARB_TRANSMIT		BIT(29)
>  
>  /* IFx message control */
>  #define IF_MCONT_NEWDAT		BIT(15)
> @@ -133,6 +133,7 @@
>  #define IF_MCONT_TXRQST		BIT(8)
>  #define IF_MCONT_EOB		BIT(7)
>  #define IF_MCONT_DLC_MASK	0xf
> +#define IF_MCONT_DLC_MAX	8

It's not used, is it?

>  
>  /*
>   * IFx register masks:
> @@ -336,7 +337,7 @@ static void c_can_write_msg_object(struct net_device *dev,
>  			int iface, struct can_frame *frame, int objno)
>  {
>  	int i;
> -	u16 flags = 0;
> +	u32 flags = IF_ARB_MSGVAL;
>  	unsigned int id;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> @@ -349,11 +350,11 @@ static void c_can_write_msg_object(struct net_device *dev,
>  	} else
>  		id = ((frame->can_id & CAN_SFF_MASK) << 18);
>  
> -	flags |= IF_ARB_MSGVAL;
> +	id |= flags;
>  
>  	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
>  				IFX_WRITE_LOW_16BIT(id));
> -	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), flags |
> +	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
>  				IFX_WRITE_HIGH_16BIT(id));
>  
>  	for (i = 0; i < frame->can_dlc; i += 2) {
> @@ -439,7 +440,7 @@ static void c_can_handle_lost_msg_obj(struct net_device *dev,
>  
>  static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
>  {
> -	u16 flags, data;
> +	u16 data;
>  	int i;
>  	unsigned int val;
>  	struct c_can_priv *priv = netdev_priv(dev);
> @@ -455,16 +456,15 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
>  
>  	frame->can_dlc = get_can_dlc(ctrl & 0x0F);
>  
> -	flags =	priv->read_reg(priv, C_CAN_IFACE(ARB2_REG, iface));
> -	val = priv->read_reg(priv, C_CAN_IFACE(ARB1_REG, iface)) |
> -		(flags << 16);
> +	val = (priv->read_reg(priv, C_CAN_IFACE(ARB2_REG, iface)) << 16);
> +	val |= priv->read_reg(priv, C_CAN_IFACE(ARB1_REG, iface));
>  
> -	if (flags & IF_ARB_MSGXTD)
> +	if (val & IF_ARB_MSGXTD)
>  		frame->can_id = (val & CAN_EFF_MASK) | CAN_EFF_FLAG;
>  	else
>  		frame->can_id = (val >> 18) & CAN_SFF_MASK;
>  
> -	if (flags & IF_ARB_TRANSMIT)
> +	if (val & IF_ARB_TRANSMIT)
>  		frame->can_id |= CAN_RTR_FLAG;
>  	else {
>  		for (i = 0; i < frame->can_dlc; i += 2) {
> @@ -500,10 +500,11 @@ static void c_can_setup_receive_object(struct net_device *dev, int iface,
>  	priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface),
>  			IFX_WRITE_HIGH_16BIT(mask) | BIT(13));
>  
> +	id |= IF_ARB_MSGVAL;
>  	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
>  			IFX_WRITE_LOW_16BIT(id));
>  	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
> -			(IF_ARB_MSGVAL | IFX_WRITE_HIGH_16BIT(id)));
> +			(IFX_WRITE_HIGH_16BIT(id)));
>  
>  	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), mcont);
>  	c_can_object_put(dev, iface, objno, IF_COMM_ALL & ~IF_COMM_TXRQST);
> 


-- 
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: 259 bytes --]

Re: [PATCH 05/16] c_can: use 32 bit access for D_CAN

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Other than the C_CAN 16 bit memory interface the D_CAN uses a 32bit memory
> interface. This causes some trouble while accessing the IFxCMR register in
> two 16bit chunks. Use 32bit access on D_CAN.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 56 +++++++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 886163f..b3cfb85 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -262,11 +262,32 @@ static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv)
>  
>  static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index)

What about introducing priv->{read,write}_reg32 function pointers?

>  {
> -	u32 val = priv->read_reg(priv, index);
> -	val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
> +	u32 val;
> +
> +	if (priv->type == BOSCH_D_CAN || priv->type == BOSCH_D_CAN_FLEXCARD) {
> +		val = readl(priv->base + priv->regs[index]);
> +	} else {
> +		val = priv->read_reg(priv, index);
> +		val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
> +	}
>  	return val;
>  }
>  
> +static void c_can_writereg32(struct c_can_priv *priv, enum reg index,
> +		u16 high, u16 low)
> +{

If we decide not to introduce read/write 32 bit function pointers,
please rename your function to c_can_write_reg32() to match the pattern
of the read_reg32() function.

As you have converted some of the potential users of write_reg32() to
work with a single 32 bit value instead of two 16 bits, I think it's
better to call this function with a single 32 bite value.

> +	u32 val;
> +
> +	if (priv->type == BOSCH_D_CAN || priv->type == BOSCH_D_CAN_FLEXCARD) {
> +		val = high << 16;
> +		val |= low;
> +		writel(val, priv->base + priv->regs[index]);
> +	} else {
> +		priv->write_reg(priv, index + 1, high);
> +		priv->write_reg(priv, index, low);
> +	}
> +}
> +
>  static void c_can_enable_all_interrupts(struct c_can_priv *priv,
>  						int enable)
>  {
> @@ -309,10 +330,8 @@ static inline void c_can_object_get(struct net_device *dev,
>  	 * register and message RAM must be complete in 6 CAN-CLK
>  	 * period.
>  	 */
> -	priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
> -			IFX_WRITE_LOW_16BIT(mask));
> -	priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
> -			IFX_WRITE_LOW_16BIT(objno));
> +	c_can_writereg32(priv, C_CAN_IFACE(COMREQ_REG, iface),
> +			IFX_WRITE_LOW_16BIT(mask), IFX_WRITE_LOW_16BIT(objno));
>  
>  	if (c_can_msg_obj_is_busy(priv, iface))
>  		netdev_err(dev, "timed out in object get\n");
> @@ -329,9 +348,8 @@ static inline void c_can_object_put(struct net_device *dev,
>  	 * register and message RAM must be complete in 6 CAN-CLK
>  	 * period.
>  	 */
> -	priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
> -			(IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask)));
> -	priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
> +	c_can_writereg32(priv, C_CAN_IFACE(COMREQ_REG, iface),
> +			IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask),
>  			IFX_WRITE_LOW_16BIT(objno));
>  
>  	if (c_can_msg_obj_is_busy(priv, iface))
> @@ -496,23 +514,19 @@ static void c_can_setup_receive_object(struct net_device *dev, int iface,
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> -	priv->write_reg(priv, C_CAN_IFACE(MASK1_REG, iface),
> -			IFX_WRITE_LOW_16BIT(mask));
> -
>  	/* According to C_CAN documentation, the reserved bit
>  	 * in IFx_MASK2 register is fixed 1
>  	 */
> -	priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface),
> -			IFX_WRITE_HIGH_16BIT(mask) | BIT(13));
> -
> +	c_can_writereg32(priv, C_CAN_IFACE(MASK1_REG, iface),
> +			IFX_WRITE_HIGH_16BIT(mask) | BIT(13),
> +			IFX_WRITE_LOW_16BIT(mask));
>  	id |= IF_ARB_MSGVAL;
> -	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
> -			IFX_WRITE_LOW_16BIT(id));
> -	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
> -			(IFX_WRITE_HIGH_16BIT(id)));
> +	c_can_writereg32(priv, C_CAN_IFACE(ARB1_REG, iface),
> +			IFX_WRITE_HIGH_16BIT(id), IFX_WRITE_LOW_16BIT(id));
> +	c_can_writereg32(priv, C_CAN_IFACE(MSGCTRL_REG, iface), 0, mcont);
>  
> -	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), mcont);
> -	c_can_object_put(dev, iface, objno, IF_COMM_ALL & ~IF_COMM_TXRQST);
> +	c_can_object_put(dev, iface, objno, IF_COMM_WR | IF_COMM_MASK |
> +			 IF_COMM_ARB | IF_COMM_CONTROL | IF_COMM_CLR_INT_PND);

I think the last change is unrelated.

>  
>  	netdev_dbg(dev, "obj no:%d, msgval:0x%08x\n", objno,
>  			c_can_read_reg32(priv, C_CAN_MSGVAL1_REG));
> 

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: 259 bytes --]

Re: [PATCH 06/16] c_can: consider set bittiming may fail

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> While setting the bittiming the C_CAN/D_CAN may fail. Do not bring up the
> CAN interface in this case.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index b3cfb85..3fa5347 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -689,9 +689,10 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>   * - set operating mode
>   * - configure message objects
>   */
> -static void c_can_chip_config(struct net_device *dev)
> +static int c_can_chip_config(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	int ret;
>  
>  	/* enable automatic retransmission */
>  	priv->write_reg(priv, C_CAN_CTRL_REG,
> @@ -726,15 +727,20 @@ static void c_can_chip_config(struct net_device *dev)
>  	priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
>  
>  	/* set bittiming params */
> -	c_can_set_bittiming(dev);
> +	ret = c_can_set_bittiming(dev);
> +
> +	return ret;

Maybe just:
	return c_can_set_bittiming(dev);
>  }
>  
> -static void c_can_start(struct net_device *dev)
> +static int c_can_start(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	int ret;
>  
>  	/* basic c_can configuration */
> -	c_can_chip_config(dev);
> +	ret = c_can_chip_config(dev);
> +	if (ret)
> +		goto out;

No need for a goto here, IMHO.

>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
> @@ -743,6 +749,9 @@ static void c_can_start(struct net_device *dev)
>  
>  	/* enable status change, error and module interrupts */
>  	c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
> +
> +out:
> +	return ret;
>  }
>  
>  static void c_can_stop(struct net_device *dev)
> @@ -758,9 +767,15 @@ static void c_can_stop(struct net_device *dev)
>  
>  static int c_can_set_mode(struct net_device *dev, enum can_mode mode)
>  {
> +	int ret;
> +
>  	switch (mode) {
>  	case CAN_MODE_START:
> -		c_can_start(dev);
> +		ret = c_can_start(dev);
> +		if (ret) {
> +			c_can_stop(dev);
> +			return ret;
> +		}
>  		netif_wake_queue(dev);
>  		break;
>  	default:
> 

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: 259 bytes --]

Re: [PATCH 08/16] c_can: Add FlexCard CAN TX fifo support

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The FlexCard DCAN implementation contains a specialized TX fifo function.
> Add the TX support for this function.

This patch looks a bit fishy. Is this compatible with existing
c_can/d_can hardware. It looks like you "use" frame->dlc to transport
some information.

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 77 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 65 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 39e2bb0..4b94f2d 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -41,6 +41,7 @@
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
>  
> +#include <linux/flexcard.h>
>  #include "c_can.h"
>  
>  /* Number of interface registers */
> @@ -566,24 +567,60 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	u32 msg_obj_no;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct can_frame *frame = (struct can_frame *)skb->data;
> +	int tx_fifo;
>  
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
>  
> -	msg_obj_no = get_tx_next_msg_obj(priv);
> +	tx_fifo = frame->can_dlc & FC_TXFIFO_FLAG;

Can you describe what you have encoded into to frame->can_dlc?

> +	frame->can_dlc &= FC_TXFIFO_DLC_MASK;
>  
> -	/* prepare message object for transmission */
> -	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> -	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> +	if (tx_fifo) {
> +		u32 id, *data, ctrl;
>  
> -	/*
> -	 * we have to stop the queue in case of a wrap around or
> -	 * if the next TX message object is still in use
> -	 */
> -	priv->tx_next++;
> -	if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv)) ||
> -			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
> -		netif_stop_queue(dev);
> +		if (readl(priv->base + FC_TXFIFO_STAT) &
> +				FC_TXFIFO_STAT_FULL) {
> +			netif_stop_queue(dev);
> +			return NETDEV_TX_BUSY;
> +		}
> +
> +		if (frame->can_id & CAN_EFF_FLAG) {
> +			id = frame->can_id & CAN_EFF_MASK;
> +			id |= FC_TXFIFO_MSGID_EXT;
> +		} else {
> +			id = frame->can_id & CAN_SFF_MASK;
> +			/* StdID is left alligned */
> +			id <<= FC_TXFIFO_MSGID_STDID_SHIFT;
> +		}
> +
> +		writel(id, priv->base + FC_TXFIFO_MSGID);
> +		writel(frame->can_dlc, priv->base + FC_TXFIFO_MSGCTRL);
> +
> +		if (frame->can_dlc) {
> +			data = (u32 *) frame->data;
> +			writel(data[0], priv->base + FC_TXFIFO_MSGDA);
> +			writel(data[1], priv->base + FC_TXFIFO_MSGDB);
> +		}
> +
> +		ctrl = readl(priv->base + FC_TXFIFO_CTRL);
> +		ctrl |= FC_TXFIFO_CTRL_REQ;
> +		writel(ctrl, priv->base + FC_TXFIFO_CTRL);
> +		kfree_skb(skb);
> +	} else {
> +		msg_obj_no = get_tx_next_msg_obj(priv);
> +
> +		/* prepare message object for transmission */
> +		c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> +		priv->tx_next++;
> +
> +		can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> +		/* we have to stop the queue in case of a wrap around or
> +		 * if the next TX message object is still in use
> +		 */
> +		if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv))
> +			|| ((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
> +			netif_stop_queue(dev);
> +	}
>  
>  	return NETDEV_TX_OK;
>  }
> @@ -683,6 +720,8 @@ static void c_can_configure_msg_objects(struct net_device *dev, int invalidate)
>  				   IF_MASK_MDIR | IF_MASK_RES, 0,
>  				   IF_MCONT_UMASK | IF_MCONT_EOB |
>  				   IF_MCONT_RXIE | IF_MCONT_DLC_MAX);
> +
> +	c_can_inval_msg_object(dev, 0, FC_TXFIFO_MO);
>  }
>  
>  /*
> @@ -740,8 +779,13 @@ static int c_can_chip_config(struct net_device *dev)
>  static int c_can_start(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	u32 conf;
>  	int ret;
>  
> +	conf = readl(priv->base + FC_TXFIFO_CONF);
> +	conf |= FC_TXFIFO_CONF_EN;
> +	writel(conf, priv->base + FC_TXFIFO_CONF);
> +
>  	/* basic c_can configuration */
>  	ret = c_can_chip_config(dev);
>  	if (ret)
> @@ -762,6 +806,11 @@ out:
>  static void c_can_stop(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	u32 conf;
> +
> +	conf = readl(priv->base + FC_TXFIFO_CONF);
> +	conf &= ~FC_TXFIFO_CONF_EN;
> +	writel(conf, priv->base + FC_TXFIFO_CONF);
>  
>  	/* disable all interrupts */
>  	c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> @@ -1350,6 +1399,8 @@ int register_c_can_dev(struct net_device *dev)
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	int err;
>  
> +	writel(0, priv->base + FC_TXFIFO_CONF);
> +
>  	c_can_pm_runtime_enable(priv);
>  
>  	dev->flags |= IFF_ECHO;	/* we support local echo */
> @@ -1369,6 +1420,8 @@ void unregister_c_can_dev(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> +	writel(0, priv->base + FC_TXFIFO_CONF);
> +
>  	unregister_candev(dev);
>  
>  	c_can_pm_runtime_disable(priv);
> 

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: 259 bytes --]

Re: [PATCH 10/16] c_can: add 16bit align 32bit access functions

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The FlexCard only allows 32bit access to DCAN registers, otherwise the
> register value can be wraped up. Add a new helper function to access
> registers 16bit aligned but 32bit access.

You are modifying code you have previously added. You should shuffle
your patches that this is not necessary.

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c          |  1 +
>  drivers/net/can/c_can/c_can.h          |  1 +
>  drivers/net/can/c_can/c_can_platform.c | 38 ++++++++++++++++++++++++++++++++--
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index c573399..46d741d 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1305,6 +1305,7 @@ struct net_device *alloc_c_can_dev(void)
>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>  					CAN_CTRLMODE_LISTENONLY |
>  					CAN_CTRLMODE_BERR_REPORTING;
> +	spin_lock_init(&priv->lock);
>  
>  	return dev;
>  }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 9f0eda8..beea437 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -175,6 +175,7 @@ struct c_can_priv {
>  	u32 __iomem *raminit_ctrlreg;
>  	unsigned int instance;
>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
> +	spinlock_t lock;
>  };
>  
>  struct net_device *alloc_c_can_dev(void);
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 43a3e3f..c6c1eb4 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -58,6 +58,40 @@ static void c_can_plat_write_reg_aligned_to_16bit(struct c_can_priv *priv,
>  	writew(val, priv->base + priv->regs[index]);
>  }
>  
> +#define ALIGN_DOWN(p, a)	((typeof(p))round_down((unsigned long)(p), (a)))
> +
> +static u16 c_can_plat_read_16bit_align_32bit_access(struct c_can_priv *priv,
> +						    enum reg index)
> +{
> +	void *addr = priv->base + priv->regs[index];

Please compile with C=2. Should be void __iomem *.

> +	u32 reg;
> +	reg = readl(ALIGN_DOWN(addr, 4));
> +
> +	return IS_ALIGNED((unsigned long)addr, 4) ?
> +			reg & 0xffff :
> +			reg >> 16;
> +}
> +
> +static void c_can_plat_write_16bit_align_32bit_access(struct c_can_priv *priv,
> +						      enum reg index, u16 val)
> +{
> +	unsigned long flags;
> +	void *addr = priv->base + priv->regs[index];

dito

> +	u32 reg;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	reg = readl(ALIGN_DOWN(addr, 4));
> +	if (IS_ALIGNED((unsigned long)addr, 4)) {
> +		reg &= 0xffff0000;
> +		reg |= val;
> +	} else {
> +		reg &= 0xffff;
> +		reg |= val << 16;
> +	}
> +	writel(reg, ALIGN_DOWN(addr, 4));
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
>  static u16 c_can_plat_read_reg_aligned_to_32bit(struct c_can_priv *priv,
>  						enum reg index)
>  {
> @@ -317,8 +351,8 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	case BOSCH_D_CAN_FLEXCARD:
>  		priv->regs = reg_map_d_can;
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> -		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
> -		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
> +		priv->read_reg = c_can_plat_read_16bit_align_32bit_access;
> +		priv->write_reg = c_can_plat_write_16bit_align_32bit_access;
>  		priv->instance = pdev->id;
>  
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> 

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: 259 bytes --]

Re: [PATCH 11/16] c_can: stop netqueue if hardware is busy

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Unlike other network devices the FlexCard do not support interrupts on TX.
> Emulate the TX interrupt by polling the MOTRX registers and restart the
> netqueue if one or more message object is available for transtition of CAN
> frames.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 61 ++++++++++++++++++++++++++++++++++++++++---
>  drivers/net/can/c_can/c_can.h |  4 +++
>  2 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 46d741d..02f7c89 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -550,18 +550,66 @@ static void c_can_inval_msg_object(struct net_device *dev, int iface, int objno)
>  
>  static inline int c_can_is_next_tx_obj_busy(struct c_can_priv *priv, int objno)
>  {
> -	int val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
> +	int val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG + (objno / 32) * 4);
>  
>  	/*
>  	 * as transmission request register's bit n-1 corresponds to
>  	 * message object n, we need to handle the same properly.
>  	 */
> -	if (val & (1 << (objno - 1)))
> +	if (val & (1 << ((objno % 32) - 1)))
>  		return 1;
>  
>  	return 0;
>  }
>  
> +static void c_can_motrx_add_timer(struct net_device *dev)
> +{
> +	struct c_can_priv *priv = netdev_priv(dev);
> +
> +	priv->timer.expires = round_jiffies_relative(1);
> +	add_timer(&priv->timer);
> +}
> +
> +static void c_can_motrx_poll(unsigned long data)
> +{
> +	struct net_device *dev = (struct net_device *)data;
> +	struct c_can_priv *priv = netdev_priv(dev);
> +	u32 val, empty = 0;
> +	int i;
> +
> +	for (i = 0; i < C_CAN_MOTRX_NR; i++) {
> +		val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG + i * 4);
> +
> +		if (val < priv->motrx[i]) {
> +			netif_wake_queue(dev);
> +			return;
> +		}
> +
> +		empty |= val;
> +	}
> +
> +	if (!empty) {
> +		netif_wake_queue(dev);
> +		return;
> +	}
> +
> +	c_can_motrx_add_timer(dev);
> +}
> +
> +static void c_can_motrx_monitor(struct net_device *dev)
> +{
> +	struct c_can_priv *priv = netdev_priv(dev);
> +	int i;
> +
> +	if (priv->type != BOSCH_D_CAN_FLEXCARD)
> +		return;
> +
> +	for (i = 0; i < C_CAN_MOTRX_NR; i++)
> +		priv->motrx[i] = c_can_read_reg32(priv,
> +				C_CAN_TXRQST1_REG + i * 4);
> +	c_can_motrx_add_timer(dev);
> +}
> +
>  static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  					struct net_device *dev)
>  {
> @@ -582,6 +630,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  		if (readl(priv->base + FC_TXFIFO_STAT) &
>  				FC_TXFIFO_STAT_FULL) {
>  			netif_stop_queue(dev);
> +			c_can_motrx_monitor(dev);
>  			return NETDEV_TX_BUSY;
>  		}
>  
> @@ -619,8 +668,10 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  		 * if the next TX message object is still in use
>  		 */
>  		if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv))
> -			|| ((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
> +			|| ((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)) {
>  			netif_stop_queue(dev);
> +			c_can_motrx_monitor(dev);
> +		}
>  	}
>  
>  	return NETDEV_TX_OK;
> @@ -1272,6 +1323,7 @@ static int c_can_close(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> +	del_timer_sync(&priv->timer);
>  	netif_stop_queue(dev);
>  	napi_disable(&priv->napi);
>  	c_can_stop(dev);
> @@ -1306,6 +1358,9 @@ struct net_device *alloc_c_can_dev(void)
>  					CAN_CTRLMODE_LISTENONLY |
>  					CAN_CTRLMODE_BERR_REPORTING;
>  	spin_lock_init(&priv->lock);
> +	priv->timer.function = c_can_motrx_poll;
> +	priv->timer.data = (unsigned long) dev;
> +	init_timer_deferrable(&priv->timer);
>  
>  	return dev;
>  }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index beea437..6d0f805 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -153,6 +153,8 @@ enum c_can_dev_id {
>  	BOSCH_D_CAN_FLEXCARD,
>  };
>  
> +#define C_CAN_MOTRX_NR	1

Are there reasons to have C_CAN_MOTRX_NR != 1? If not, please remove the
array and make code more clean.

>  /* c_can private data structure */
>  struct c_can_priv {
>  	struct can_priv can;	/* must be the first member */
> @@ -176,6 +178,8 @@ struct c_can_priv {
>  	unsigned int instance;
>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
>  	spinlock_t lock;
> +	struct timer_list timer;
> +	u32 motrx[C_CAN_MOTRX_NR];
>  };
>  
>  struct net_device *alloc_c_can_dev(void);
> 

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: 259 bytes --]

Re: [PATCH 11/16] c_can: stop netqueue if hardware is busy

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Unlike other network devices the FlexCard do not support interrupts on TX.
> Emulate the TX interrupt by polling the MOTRX registers and restart the
> netqueue if one or more message object is available for transtition of CAN
> frames.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>

It's not onlynetif_wake_queue() that you have to call on TX-complete.
Have a look at c_can_do_tx(), better call it, or trigger napi from your
timer.

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: 259 bytes --]

Re: [PATCH 12/16] c_can: Add flag to disable automatic retransmission of CAN frames

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The C_CAN/D_CAN controler can automatic retransmit CAN after arbitration loss.
> Add a flag to CAN ctrlmode set the appropiate behaivior of this feature.

We already have this flag, it's called CAN_CTRLMODE_ONE_SHOT.

Marc

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c    | 16 +++++++++-------
>  include/uapi/linux/can/netlink.h |  2 ++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 02f7c89..51ca2a6 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -785,33 +785,35 @@ static void c_can_configure_msg_objects(struct net_device *dev, int invalidate)
>  static int c_can_chip_config(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	u16 reg;
>  	int ret;
>  
> -	/* enable automatic retransmission */
> -	priv->write_reg(priv, C_CAN_CTRL_REG,
> -			CONTROL_ENABLE_AR);
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_DAR)
> +		reg = CONTROL_DISABLE_AR;
> +	else
> +		reg = CONTROL_ENABLE_AR;
>  
>  	if ((priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) &&
>  	    (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)) {
>  		/* loopback + silent mode : useful for hot self-test */
>  		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
> -				CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> +				reg | CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
>  		priv->write_reg(priv, C_CAN_TEST_REG,
>  				TEST_LBACK | TEST_SILENT);
>  	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>  		/* loopback mode : useful for self-test function */
>  		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
> -				CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> +				reg | CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
>  		priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK);
>  	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
>  		/* silent mode : bus-monitoring mode */
>  		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
> -				CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> +				reg | CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
>  		priv->write_reg(priv, C_CAN_TEST_REG, TEST_SILENT);
>  	} else
>  		/* normal mode*/
>  		priv->write_reg(priv, C_CAN_CTRL_REG,
> -				CONTROL_EIE | CONTROL_SIE | CONTROL_IE);
> +				reg | CONTROL_EIE | CONTROL_SIE | CONTROL_IE);
>  
>  	/* configure message objects */
>  	c_can_configure_msg_objects(dev, 1);
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index 14966dd..fdce5af 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -88,6 +88,8 @@ struct can_ctrlmode {
>  #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
>  #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
>  #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
> +#define CAN_CTRLMODE_DAR		0x20	/* Disable Automatic
> +						 * Retransmission */
>  
>  /*
>   * CAN device statistics
> 


-- 
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: 259 bytes --]

Re: [PATCH 12/16] c_can: Add flag to disable automatic retransmission of CAN frames

[Marc Kleine-Budde]

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

On 09/09/2013 12:21 PM, Marc Kleine-Budde wrote:
> On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
>> The C_CAN/D_CAN controler can automatic retransmit CAN after arbitration loss.
>> Add a flag to CAN ctrlmode set the appropiate behaivior of this feature.
> 
> We already have this flag, it's called CAN_CTRLMODE_ONE_SHOT.

BTW: Have a look at commit:

    ee6f098 can: c_can: disable one shot mode until driver is fixed

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: 259 bytes --]

Re: [PATCH 13/16] c_can: flexcard: add ioctl to reset FIFO message object

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The FlexCard FIFO reset implementation did not reset the FIFO message object.
> Since the FIFO configuration is done from userspace a function to reset the
> FIFO message object is needed. Add an ioctl to reset the FIFO message object
> from userspace.

Can you elaborate this a bit.

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
>
> ---
>  drivers/net/can/c_can/c_can.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 51ca2a6..670a6b1 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1447,7 +1447,21 @@ void free_c_can_dev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(free_c_can_dev);
>  
> +static int c_can_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	switch (cmd) {
> +	case SIOCDEVPRIVATE:
> +		c_can_inval_msg_object(dev, 0, FC_TXFIFO_MO);
> +		break;

What happens if you call this ioctl on non flexcard HW?

> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops c_can_netdev_ops = {
> +	.ndo_do_ioctl = c_can_ioctl,
>  	.ndo_open = c_can_open,
>  	.ndo_stop = c_can_close,
>  	.ndo_start_xmit = c_can_start_xmit,
> 

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: 259 bytes --]

Re: [PATCH 15/16] flexcard: can: Configure CAN loopback packages (TXACK)

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Signed-off-by: Holger Dengler <dengler@linutronix.de>

I'm missing your S-o-b and a patch description. I'm finding some
uncommon use of frame->can_dlc.

Marc

> ---
>  drivers/net/can/c_can/c_can.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 670a6b1..92c1444 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -358,11 +358,13 @@ static inline void c_can_object_put(struct net_device *dev,
>  }
>  
>  static void c_can_write_msg_object(struct net_device *dev,
> -			int iface, struct can_frame *frame, int objno)
> +			int iface, struct can_frame *frame, int objno,
> +			int txie_off)
>  {
>  	u32 flags = IF_ARB_MSGVAL;
>  	unsigned int id;
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	u32 int_en;
>  
>  	if (!(frame->can_id & CAN_RTR_FLAG))
>  		flags |= IF_ARB_TRANSMIT;
> @@ -388,9 +390,10 @@ static void c_can_write_msg_object(struct net_device *dev,
>  			 frame->data[4] | frame->data[5] << 8);
>  
>  	/* enable interrupt for this message object */
> -	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
> -			IF_MCONT_TXIE | IF_MCONT_TXRQST | IF_MCONT_EOB |
> -			frame->can_dlc);
> +	int_en = frame->can_dlc;
> +	if (!txie_off)
> +		int_en |= IF_MCONT_TXIE | IF_MCONT_TXRQST | IF_MCONT_EOB;
> +	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), int_en);
>  	c_can_object_put(dev, iface, objno, IF_COMM_ALL);
>  }
>  
> @@ -616,16 +619,17 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	u32 msg_obj_no;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct can_frame *frame = (struct can_frame *)skb->data;
> -	int tx_fifo;
> +	int tx_fifo, txie_off;
>  
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
>  
>  	tx_fifo = frame->can_dlc & FC_TXFIFO_FLAG;
> +	txie_off = frame->can_dlc & FC_TXACKOFF_FLAG;
>  	frame->can_dlc &= FC_TXFIFO_DLC_MASK;
>  
>  	if (tx_fifo) {
> -		u32 id, *data, ctrl;
> +		u32 id, *data, ctrl, conf;
>  
>  		if (readl(priv->base + FC_TXFIFO_STAT) &
>  				FC_TXFIFO_STAT_FULL) {
> @@ -652,6 +656,11 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  			writel(data[1], priv->base + FC_TXFIFO_MSGDB);
>  		}
>  
> +		conf = readl(priv->base + FC_TXFIFO_CONF);
> +		conf &= ~FC_TXFIFO_CONF_TXACK;
> +		conf |= (txie_off ? 0 : FC_TXFIFO_CONF_TXACK);
> +		writel(conf, priv->base + FC_TXFIFO_CONF);
> +
>  		ctrl = readl(priv->base + FC_TXFIFO_CTRL);
>  		ctrl |= FC_TXFIFO_CTRL_REQ;
>  		writel(ctrl, priv->base + FC_TXFIFO_CTRL);
> @@ -660,7 +669,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  		msg_obj_no = get_tx_next_msg_obj(priv);
>  
>  		/* prepare message object for transmission */
> -		c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> +		c_can_write_msg_object(dev, 0, frame, msg_obj_no, txie_off);
>  		priv->tx_next++;
>  
>  		can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> 


-- 
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: 259 bytes --]

Re: [PATCH 16/16] c_can: fix TX packet accounting

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Due to the integration of the FlexCard support the TX stats accounting got
> lost. Readd the accounting.

Please squash into the appropriate patch.

Marc

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 92c1444..b409cf2 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -619,6 +619,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	u32 msg_obj_no;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct can_frame *frame = (struct can_frame *)skb->data;
> +	struct net_device_stats *stats = &dev->stats;
>  	int tx_fifo, txie_off;
>  
>  	if (can_dropped_invalid_skb(dev, skb))
> @@ -664,6 +665,9 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  		ctrl = readl(priv->base + FC_TXFIFO_CTRL);
>  		ctrl |= FC_TXFIFO_CTRL_REQ;
>  		writel(ctrl, priv->base + FC_TXFIFO_CTRL);
> +
> +		stats->tx_bytes += frame->can_dlc;
> +		stats->tx_packets++;
>  		kfree_skb(skb);
>  	} else {
>  		msg_obj_no = get_tx_next_msg_obj(priv);
> @@ -672,6 +676,8 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  		c_can_write_msg_object(dev, 0, frame, msg_obj_no, txie_off);
>  		priv->tx_next++;
>  
> +		stats->tx_bytes += frame->can_dlc;
> +		stats->tx_packets++;
>  		can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>  		/* we have to stop the queue in case of a wrap around or
>  		 * if the next TX message object is still in use
> 


-- 
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: 259 bytes --]

Re: [PATCH 04/16] c_can: fix receive buffer configuration

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The former implementation lacks some initialization of receive buffer.
> First the maximal buffer length was 0. The Buffer direction was not set
> and the mask register a reserverd flag was cleared.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index bdb7bcd..886163f 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -116,6 +116,11 @@
>  				IF_COMM_CONTROL | IF_COMM_TXRQST | \
>  				IF_COMM_DATAA | IF_COMM_DATAB)
>  
> +/* IFx mask */
> +#define IF_MASK_MXTD		BIT(31)
> +#define IF_MASK_MDIR		BIT(30)
> +#define IF_MASK_RES		BIT(29)
> +
>  /* IFx arbitration */
>  #define IF_ARB_MSGVAL		BIT(31)
>  #define IF_ARB_MSGXTD		BIT(30)
> @@ -653,11 +658,15 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>  
>  	/* setup receive message objects */
>  	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> -		c_can_setup_receive_object(dev, 0, i, 0, 0,
> -			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
> -
> -	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> -			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
> +		c_can_setup_receive_object(dev, 0, i,
> +					   IF_MASK_MDIR | IF_MASK_RES, 0,
> +					   IF_MCONT_UMASK | IF_MCONT_RXIE |
> +					   IF_MCONT_DLC_MAX);

DLC_MAX should be added in this patch. not in 3.

> +
> +	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST,
> +				   IF_MASK_MDIR | IF_MASK_RES, 0,
> +				   IF_MCONT_UMASK | IF_MCONT_EOB |
> +				   IF_MCONT_RXIE | IF_MCONT_DLC_MAX);
>  }
>  
>  /*
> 

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: 259 bytes --]

Re: [PATCH 09/16] c_can: expicit 32bit access on D_CAN to message buffer data register

[Marc Kleine-Budde]

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

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> change the 16bit access of ARB1_REG and DATA1/2_REG to a 32bit access.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 4b94f2d..c573399 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -360,7 +360,6 @@ static inline void c_can_object_put(struct net_device *dev,
>  static void c_can_write_msg_object(struct net_device *dev,
>  			int iface, struct can_frame *frame, int objno)
>  {
> -	int i;
>  	u32 flags = IF_ARB_MSGVAL;
>  	unsigned int id;
>  	struct c_can_priv *priv = netdev_priv(dev);
> @@ -376,15 +375,17 @@ static void c_can_write_msg_object(struct net_device *dev,
>  
>  	id |= flags;
>  
> -	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
> -				IFX_WRITE_LOW_16BIT(id));
> -	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
> -				IFX_WRITE_HIGH_16BIT(id));
> +	c_can_writereg32(priv, C_CAN_IFACE(ARB1_REG, iface),
> +			IFX_WRITE_HIGH_16BIT(id),
> +			IFX_WRITE_LOW_16BIT(id));
> +
> +	c_can_writereg32(priv, C_CAN_IFACE(DATA1_REG, iface),
> +			 frame->data[2] | frame->data[3] << 8,
> +			 frame->data[0] | frame->data[1] << 8);

You can use something like beXX_to_cpup((__be32 *)&cf->data[0]) here.

>  
> -	for (i = 0; i < frame->can_dlc; i += 2) {
> -		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> -				frame->data[i] | (frame->data[i + 1] << 8));
> -	}
> +	c_can_writereg32(priv, C_CAN_IFACE(DATA3_REG, iface),
> +			 frame->data[6] | frame->data[7] << 8,
> +			 frame->data[4] | frame->data[5] << 8);

You write here the upper 32 bit unconditionally, the original code doesn't.

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: 259 bytes --]

Re: [PATCH 05/16] c_can: use 32 bit access for D_CAN

[T Thayer]

Hi,

What is the status of this patch?

This fixes the problem I was seeing for the D_CAN portion of the
driver which was
1. Transmitting 2 copies (probably the result of an AutoRetry)
2. Triggering a RX IRQ on a transmit because the Message Object # was
assigned to RX.
3. CAN statistics were not incrementing properly.

I just joined this mailing list and I don't have this email in my
history so I'm copying it below.

Thanks,

TT

--------------- snipped -----------------

On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Other than the C_CAN 16 bit memory interface the D_CAN uses a 32bit memory
> interface. This causes some trouble while accessing the IFxCMR register in
> two 16bit chunks. Use 32bit access on D_CAN.
>
> Signed-off-by: Benedikt Spranger <b.spranger <at> linutronix.de>
> ---
>  drivers/net/can/c_can/c_can.c | 56 +++++++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 886163f..b3cfb85 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
>  <at>  <at>  -262,11 +262,32  <at>  <at>  static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv)
>
>  static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index)

What about introducing priv->{read,write}_reg32 function pointers?

>  {
> - u32 val = priv->read_reg(priv, index);
> - val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
> + u32 val;
> +
> + if (priv->type == BOSCH_D_CAN || priv->type == BOSCH_D_CAN_FLEXCARD) {
> + val = readl(priv->base + priv->regs[index]);
> + } else {
> + val = priv->read_reg(priv, index);
> + val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
> + }
>   return val;
>  }
>
> +static void c_can_writereg32(struct c_can_priv *priv, enum reg index,
> + u16 high, u16 low)
> +{

If we decide not to introduce read/write 32 bit function pointers,
please rename your function to c_can_write_reg32() to match the pattern
of the read_reg32() function.

As you have converted some of the potential users of write_reg32() to
work with a single 32 bit value instead of two 16 bits, I think it's
better to call this function with a single 32 bite value.

> + u32 val;
> +
> + if (priv->type == BOSCH_D_CAN || priv->type == BOSCH_D_CAN_FLEXCARD) {
> + val = high << 16;
> + val |= low;
> + writel(val, priv->base + priv->regs[index]);
> + } else {
> + priv->write_reg(priv, index + 1, high);
> + priv->write_reg(priv, index, low);
> + }
> +}
> +
>  static void c_can_enable_all_interrupts(struct c_can_priv *priv,
>   int enable)
>  {
>  <at>  <at>  -309,10 +330,8  <at>  <at>  static inline void c_can_object_get(struct net_device *dev,
>   * register and message RAM must be complete in 6 CAN-CLK
>   * period.
>   */
> - priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
> - IFX_WRITE_LOW_16BIT(mask));
> - priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
> - IFX_WRITE_LOW_16BIT(objno));
> + c_can_writereg32(priv, C_CAN_IFACE(COMREQ_REG, iface),
> + IFX_WRITE_LOW_16BIT(mask), IFX_WRITE_LOW_16BIT(objno));
>
>   if (c_can_msg_obj_is_busy(priv, iface))
>   netdev_err(dev, "timed out in object get\n");
>  <at>  <at>  -329,9 +348,8  <at>  <at>  static inline void c_can_object_put(struct net_device *dev,
>   * register and message RAM must be complete in 6 CAN-CLK
>   * period.
>   */
> - priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
> - (IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask)));
> - priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
> + c_can_writereg32(priv, C_CAN_IFACE(COMREQ_REG, iface),
> + IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask),
>   IFX_WRITE_LOW_16BIT(objno));
>
>   if (c_can_msg_obj_is_busy(priv, iface))
>  <at>  <at>  -496,23 +514,19  <at>  <at>  static void c_can_setup_receive_object(struct net_device *dev, int iface,
>  {
>   struct c_can_priv *priv = netdev_priv(dev);
>
> - priv->write_reg(priv, C_CAN_IFACE(MASK1_REG, iface),
> - IFX_WRITE_LOW_16BIT(mask));
> -
>   /* According to C_CAN documentation, the reserved bit
>   * in IFx_MASK2 register is fixed 1
>   */
> - priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface),
> - IFX_WRITE_HIGH_16BIT(mask) | BIT(13));
> -
> + c_can_writereg32(priv, C_CAN_IFACE(MASK1_REG, iface),
> + IFX_WRITE_HIGH_16BIT(mask) | BIT(13),
> + IFX_WRITE_LOW_16BIT(mask));
>   id |= IF_ARB_MSGVAL;
> - priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
> - IFX_WRITE_LOW_16BIT(id));
> - priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
> - (IFX_WRITE_HIGH_16BIT(id)));
> + c_can_writereg32(priv, C_CAN_IFACE(ARB1_REG, iface),
> + IFX_WRITE_HIGH_16BIT(id), IFX_WRITE_LOW_16BIT(id));
> + c_can_writereg32(priv, C_CAN_IFACE(MSGCTRL_REG, iface), 0, mcont);
>
> - priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), mcont);
> - c_can_object_put(dev, iface, objno, IF_COMM_ALL & ~IF_COMM_TXRQST);
> + c_can_object_put(dev, iface, objno, IF_COMM_WR | IF_COMM_MASK |
> + IF_COMM_ARB | IF_COMM_CONTROL | IF_COMM_CLR_INT_PND);

I think the last change is unrelated.

>
>   netdev_dbg(dev, "obj no:%d, msgval:0x%08x\n", objno,
>   c_can_read_reg32(priv, C_CAN_MSGVAL1_REG));
>

Marc

Re: [PATCH 05/16] c_can: use 32 bit access for D_CAN

[Marc Kleine-Budde]

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

On 11/16/2013 05:37 PM, T Thayer wrote:
> What is the status of this patch?

The mail you copied reflects the current status, nothing happened since
then. Feel free to adopt this patch and address my comments.

> This fixes the problem I was seeing for the D_CAN portion of the
> driver which was
> 1. Transmitting 2 copies (probably the result of an AutoRetry)
> 2. Triggering a RX IRQ on a transmit because the Message Object # was
> assigned to RX.
> 3. CAN statistics were not incrementing properly.
> 
> I just joined this mailing list and I don't have this email in my
> history so I'm copying it below.

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: 259 bytes --]