[PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

[PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

[Federico Vaga]

Signed-off-by: Federico Vaga <federico.vaga@gmail.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Alan Cox <alan@linux.intel.com>
---
 drivers/net/can/Kconfig       |   11 +
 drivers/net/can/Makefile      |    1 +
 drivers/net/can/sta2x11_can.c | 1085 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1097 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/sta2x11_can.c

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index bb709fd..5b1baef 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -122,6 +122,17 @@ source "drivers/net/can/usb/Kconfig"
 
 source "drivers/net/can/softing/Kconfig"
 
+config CAN_STA2X11
+	depends on CAN_DEV && HAS_IOMEM && MFD_STA2X11
+	tristate "CAN STA2X11"
+	---help---
+	  Driver for the STA2x11 CAN controller
+	  Supports CAN protocol version 2.0 part A and B
+	  Bit rates up to 1 MBit/s
+	  32 Message Objects
+	  Programmable loop-back mode for self-test operation
+	  8-bit non-multiplex Motorola HC08 compatible module interface
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 938be37..00474b6 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -22,5 +22,6 @@ obj-$(CONFIG_CAN_BFIN)		+= bfin_can.o
 obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
 obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
 obj-$(CONFIG_PCH_CAN)		+= pch_can.o
+obj-$(CONFIG_CAN_STA2X11)	+= sta2x11_can.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/sta2x11_can.c b/drivers/net/can/sta2x11_can.c
new file mode 100644
index 0000000..9194b02
--- /dev/null
+++ b/drivers/net/can/sta2x11_can.c
@@ -0,0 +1,1085 @@
+/*
+ * Copyright (c) 2010-2011 Wind River Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include <linux/interrupt.h>
+#include <linux/debugfs.h>
+#include <linux/mfd/sta2x11-mfd.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+
+#define PCI_DEVICE_ID_STMICRO_CAN	0xCC11
+
+#define CAN_CR			0x0	/* Control Register */
+#define CAN_CR_INI		0x01	/* Initialization */
+#define CAN_CR_IE		0x02	/* Interrupt Enable */
+#define CAN_CR_SIE		0x04	/* Status Interrupt Enable */
+#define CAN_CR_EIE		0x08	/* Error Interrupt Enable */
+#define CAN_CR_DAR		0x20	/* Disable Automatic Re-transmission */
+#define CAN_CR_CCE		0x40	/* Change Configuration Enable */
+#define CAN_CR_TME		0x80	/* Test Mode Enable */
+
+#define CAN_SR			0x04	/* Status Register */
+#define CAN_SR_LEC		0x07	/* Last Error Code */
+#define CAN_SR_LEC_STUFF	0x01	/* Stuff error */
+#define CAN_SR_LEC_FORM		0x02	/* Form error */
+#define CAN_SR_LEC_ACK		0x03	/* Acknowledgement error */
+#define CAN_SR_LEC_BIT1		0x04	/* Bit1 error */
+#define CAN_SR_LEC_BIT0		0x05	/* Bit0 error */
+#define CAN_SR_LEC_CRC		0x06	/* CRC error */
+#define CAN_SR_TXOK		0x08	/* Transmit Message Successfully */
+#define CAN_SR_RXOK		0x10	/* Receive Message Successfully */
+#define CAN_SR_EPAS		0x20	/* Error Passive */
+#define CAN_SR_WARN		0x40	/* Warning Status */
+#define CAN_SR_BOFF		0x80	/* Bus Off Status */
+
+#define CAN_ERR			0x08	/* Error Counter Register */
+#define CAN_ERR_TEC		0xFF	/* Transmit Error Counter */
+#define CAN_ERR_REC		0x7F00	/* Receive Error Counter */
+#define CAN_ERR_RP		0x8000	/* Receive Error Passive */
+
+#define CAN_BTR			0x0C	/* Bit Timing Register */
+
+#define CAN_BRPR		0x18	/* BRP Extension Register */
+
+#define CAN_IDR			0x10	/* Interrupt Identifier Register */
+#define CAN_IDR_STATUS		0x8000	/* Status Interrupt Identifier */
+
+#define CAN_TXR1R		0x100	/* Transmission Request Register */
+#define CAN_TXR2R		0x104	/* Transmission Request Register */
+
+#define CAN_ND1R		0x120	/* New Data Register */
+#define CAN_ND2R		0x124	/* New Data Register */
+
+#define CAN_IP1R		0x140	/* Interrupt Pending Register */
+#define CAN_IP2R		0x144	/* Interrupt Pending Register */
+
+#define CAN_MV1R		0x160	/* Message Valid Register */
+#define CAN_MV2R		0x164	/* Message Valid Register */
+
+#define CAN_IF1_CRR		0x20	/* Command Request Register */
+#define CAN_IF2_CRR		0x80	/* Command Request Register */
+#define CAN_IF_CRR_BUSY		0x8000  /* Busy Flag */
+#define CAN_IF_CRR_MSG		0x3F	/* Message Number */
+
+#define CAN_IF1_CMR		0x24	/* Command Mask Register */
+#define CAN_IF2_CMR		0x84	/* Command Mask Register */
+#define CAN_IF_CMR_WR		0x80	/* Write/Read to/from Message Object */
+#define CAN_IF_CMR_MSK		0x40	/* Transfer Mask Bits */
+#define CAN_IF_CMR_AR		0x20	/* Transfer Arbitration Bits */
+#define CAN_IF_CMR_CTL		0x10	/* Transfer Control Bits */
+#define CAN_IF_CMR_CPI		0x08	/* Clear Interrupt Pending Bit */
+#define CAN_IF_CMR_TXR		0x04	/* Clear TxRqst/NewDat Bit */
+#define CAN_IF_CMR_CND		0x04	/* Clear TxRqst/NewDat Bit */
+#define CAN_IF_CMR_D30		0x02	/* Transfer Data Bytes 3:0 */
+#define CAN_IF_CMR_C74		0x01	/* Transfer Data Bytes 7:4 */
+
+#define CAN_IF1_M1R		0x28	/* Mask Register */
+#define CAN_IF2_M1R		0x88	/* Mask Register */
+
+#define CAN_IF1_M2R		0x2C	/* Mask Register */
+#define CAN_IF2_M2R		0x8C	/* Mask Register */
+#define CAN_IF_M2R_MXTD		0x8000
+#define CAN_IF_M2R_MDIR		0x4000
+
+#define CAN_IF1_A1R		0x30	/* Message Arbitration Register */
+#define CAN_IF2_A1R		0x90	/* Message Arbitration Register */
+
+#define CAN_IF1_A2R		0x34	/* Message Arbitration Register */
+#define CAN_IF2_A2R		0x94	/* Message Arbitration Register */
+#define CAN_IF_A2R_MSGVAL	0x8000
+#define CAN_IF_A2R_XTD		0x4000
+#define CAN_IF_A2R_DIR		0x2000
+
+#define CAN_IF1_MCR		0x38	/* Message Control Register */
+#define CAN_IF2_MCR		0x98	/* Message Control Register */
+#define CAN_IF_MCR_NEWD		0x8000
+#define CAN_IF_MCR_MSGL		0x4000
+#define CAN_IF_MCR_INTP		0x2000
+#define CAN_IF_MCR_UMSK		0x1000
+#define CAN_IF_MCR_TXIE		0x800
+#define CAN_IF_MCR_RXIE		0x400
+#define CAN_IF_MCR_RMT		0x200
+#define CAN_IF_MCR_TXR		0x100
+#define CAN_IF_MCR_EOB		0x80
+
+#define CAN_IF1_DATA1		0x3C	/* Buffer Register */
+#define CAN_IF1_DATA2		0x40	/* Buffer Register */
+#define CAN_IF1_DATB1		0x44	/* Buffer Register */
+#define CAN_IF1_DATB2		0x48	/* Buffer Register */
+#define CAN_IF1_DATAV {CAN_IF1_DATA1, CAN_IF1_DATA2, \
+			CAN_IF1_DATB1, CAN_IF1_DATB2}
+
+#define CAN_IF2_DATA1		0x9C	/* Buffer Register */
+#define CAN_IF2_DATA2		0xA0	/* Buffer Register */
+#define CAN_IF2_DATB1		0xA4	/* Buffer Register */
+#define CAN_IF2_DATB2		0xA8	/* Buffer Register */
+#define CAN_IF2_DATAV {CAN_IF2_DATA1, CAN_IF2_DATA2, \
+			CAN_IF2_DATB1, CAN_IF2_DATB2}
+
+#define STA2X11_ECHO_SKB_MAX	1
+
+#define MSGOBJ_FIRST		0x01
+#define MSGOBJ_LAST		0x20
+
+/* max. number of interrupts handled in ISR */
+#define STA2X11_MAX_IRQ		20
+
+/*
+ * STA2X11 private data structure
+ */
+struct sta2x11_priv {
+	struct can_priv can;	/* must be the first member */
+	int open_time;
+	struct net_device *dev;
+	void __iomem *reg_base;	/* ioremap'ed address to registers */
+	struct dentry *dentry;
+	struct timer_list txtimer;
+};
+
+#define STA2X11_APB_FREQ 104000000
+
+/*
+ * 32 messages are available, but only 2 messages are used.
+ * TX and RX message objects
+ */
+#define STA2X11_OBJ_TX 1
+#define STA2X11_OBJ_RX 2
+
+static struct can_bittiming_const sta2x11_can_bittiming_const = {
+	.name = KBUILD_MODNAME,
+	.tseg1_min = 2,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 1024,
+	.brp_inc = 1,
+};
+
+static void sta2x11_can_write_reg(struct sta2x11_priv *priv, uint32_t val, int reg)
+{
+	writel(val, priv->reg_base + reg);
+}
+
+static uint32_t sta2x11_can_read_reg(struct sta2x11_priv *priv, int reg)
+{
+	return readl(priv->reg_base + reg);
+}
+
+static void sta2x11_can_clear_interrupts(struct sta2x11_priv *priv)
+{
+	uint32_t mo;
+
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_CPI, CAN_IF1_CMR);
+	for (mo = MSGOBJ_FIRST; mo <= MSGOBJ_LAST; mo++)
+		sta2x11_can_write_reg(priv, mo, CAN_IF1_CRR);
+}
+
+static void sta2x11_can_enable_objs(const struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+	/* RX message object */
+	/* command mask */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR |
+				    CAN_IF_CMR_MSK | CAN_IF_CMR_CTL,
+				    CAN_IF1_CMR);
+	/* mask */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_M1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_M2R);
+	/* arb */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A1R);
+	sta2x11_can_write_reg(priv, CAN_IF_A2R_MSGVAL, CAN_IF1_A2R);
+	/* control */
+	sta2x11_can_write_reg(priv, CAN_IF_MCR_RXIE | CAN_IF_MCR_UMSK |
+				    CAN_IF_MCR_EOB, CAN_IF1_MCR);
+
+	sta2x11_can_write_reg(priv, STA2X11_OBJ_RX, CAN_IF1_CRR);
+
+	/* TX message object */
+	/* command mask */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR |
+				    CAN_IF_CMR_CTL, CAN_IF1_CMR);
+	/* arb */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A2R);
+	/* control */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_MCR);
+	/* Write to RAM */
+	sta2x11_can_write_reg(priv, STA2X11_OBJ_TX, CAN_IF1_CRR);
+}
+
+static void sta2x11_can_disable_objs(struct sta2x11_priv *priv)
+{
+	/* RX message object */
+	/* command mask */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR |
+				    CAN_IF_CMR_CTL, CAN_IF1_CMR);
+	/* arb */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A2R);
+	/* control */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_MCR);
+	/* command */
+	sta2x11_can_write_reg(priv, STA2X11_OBJ_RX, CAN_IF1_CRR);
+
+	/* TX message object */
+	/* command mask */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR |
+				    CAN_IF_CMR_CTL, CAN_IF1_CMR);
+	/* arb */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A2R);
+	/* control */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_MCR);
+	/* command */
+	sta2x11_can_write_reg(priv, STA2X11_OBJ_TX, CAN_IF1_CRR);
+}
+
+static void sta2x11_can_reset_mode(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) {
+		/* cancel timer handling tx request poll */
+		del_timer_sync(&priv->txtimer);
+	}
+
+	/* enable configuration and puts chip in bus-off, disable interrupts */
+	sta2x11_can_write_reg(priv, CAN_CR_CCE | CAN_CR_INI, CAN_CR);
+
+	priv->can.state = CAN_STATE_STOPPED;
+
+	sta2x11_can_clear_interrupts(priv);
+
+	/* clear status interrupt */
+	sta2x11_can_read_reg(priv, CAN_SR);
+	/* clear status register */
+	sta2x11_can_write_reg(priv, 0x0, CAN_SR);
+
+	/* disable all used message objects */
+	sta2x11_can_disable_objs(priv);
+}
+
+static void sta2x11_can_normal_mode(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	uint32_t ctrl;
+
+	sta2x11_can_clear_interrupts(priv);
+
+	/* clear status interrupt */
+	sta2x11_can_read_reg(priv, CAN_SR);
+	/* clear status register */
+	sta2x11_can_write_reg(priv, CAN_SR_LEC, CAN_SR);
+
+	/* enable all used message objects */
+	sta2x11_can_enable_objs(dev);
+
+	/* clear bus-off */
+	ctrl = CAN_CR_IE | CAN_CR_EIE;
+	if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+		ctrl |= CAN_CR_SIE;
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		ctrl |= CAN_CR_DAR;
+
+	sta2x11_can_write_reg(priv, ctrl, CAN_CR);
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+}
+
+static void sta2x11_can_chipset_init(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct pci_dev *pdev = to_pci_dev(dev->dev.parent);
+	int data_reg[] = CAN_IF1_DATAV;
+	uint32_t mo;
+	unsigned int i;
+
+
+	/* config clock and release device from reset */
+	sta2x11_apbreg_mask(pdev, APBREG_PCG, APBREG_CAN, 0);
+	sta2x11_apbreg_mask(pdev, APBREG_PUR, APBREG_CAN, 0);
+	msleep_interruptible(100);
+	sta2x11_apbreg_mask(pdev, APBREG_PCG, APBREG_CAN, APBREG_CAN);
+	sta2x11_apbreg_mask(pdev, APBREG_PUR, APBREG_CAN, APBREG_CAN);
+	msleep_interruptible(100);
+
+	/* enable configuration and put chip in bus-off, disable interrupts */
+	sta2x11_can_write_reg(priv, CAN_CR_CCE | CAN_CR_INI, CAN_CR);
+
+	/* clear status interrupt */
+	sta2x11_can_read_reg(priv, CAN_SR);
+	/* clear status register */
+	sta2x11_can_write_reg(priv, CAN_SR_LEC, CAN_SR);
+
+	sta2x11_can_clear_interrupts(priv);
+
+	/* Invalidate message objects */
+	/* command mask */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_MSK |
+				    CAN_IF_CMR_AR | CAN_IF_CMR_CTL |
+				    CAN_IF_CMR_D30 | CAN_IF_CMR_C74,
+				    CAN_IF1_CMR);
+	/* mask */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_M1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_M2R);
+	/* arb */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A2R);
+	/* control */
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_MCR);
+	/* data */
+	for (i = 0; i < 4; i++)
+		sta2x11_can_write_reg(priv, 0x0, data_reg[i]);
+
+	/* send command to all 32 messages */
+	for (mo = MSGOBJ_FIRST; mo <= MSGOBJ_LAST; mo++)
+		sta2x11_can_write_reg(priv, mo, CAN_IF1_CRR);
+}
+
+static void sta2x11_can_start(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+	if (priv->can.state != CAN_STATE_STOPPED)
+		sta2x11_can_reset_mode(dev);
+
+	sta2x11_can_normal_mode(dev);
+}
+
+static void sta2x11_can_write_data(struct sta2x11_priv *priv,
+			       struct can_frame *cf, u8 dlc)
+{
+	int data_reg[] = CAN_IF1_DATAV;
+	uint32_t val = 0;
+	int i;
+
+	for (i = 0; i < dlc; i++) {
+		if (i & 0x1) {
+			val |= cf->data[i] << 8;
+			sta2x11_can_write_reg(priv, val, data_reg[i / 2]);
+		} else {
+			val = cf->data[i];
+		}
+	}
+	/* if dlc is an even number the last byte must be write */
+	if (i & 0x1)
+		sta2x11_can_write_reg(priv, val, data_reg[i / 2]);
+
+}
+
+static void sta2x11_can_ar_config(struct sta2x11_priv *priv, uint32_t id,
+				  uint32_t dir)
+{
+	/* Arbitration configuration */
+	if (id & CAN_EFF_FLAG) { /* extended identifier */
+		id &= CAN_EFF_MASK;
+		sta2x11_can_write_reg(priv, id & 0xFFFF, CAN_IF1_A1R);
+		sta2x11_can_write_reg(priv, CAN_IF_A2R_MSGVAL | CAN_IF_A2R_XTD |
+					    dir | (id >> 16), CAN_IF1_A2R);
+	} else { /* standard identifier */
+		id &= CAN_SFF_MASK;
+		sta2x11_can_write_reg(priv, 0X0, CAN_IF1_A1R);
+		sta2x11_can_write_reg(priv, CAN_IF_A2R_MSGVAL | dir | (id << 2),
+					    CAN_IF1_A2R);
+	}
+}
+
+static int sta2x11_can_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	uint32_t dlc, id, dir = 0, cmr, ctrl;
+
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
+
+	if ((sta2x11_can_read_reg(priv, CAN_TXR1R) & STA2X11_OBJ_TX)) {
+		dev_err(dev->dev.parent, "TX register is still occupied!\n");
+		return NETDEV_TX_BUSY;
+	}
+
+	/* It doesn't accept new message during transmission */
+	netif_stop_queue(dev);
+
+	dlc = cf->can_dlc & 0x0f;
+	id = cf->can_id;
+
+	/* Message Control Register configuration */
+	cmr = CAN_IF_CMR_WR | CAN_IF_CMR_AR | CAN_IF_CMR_CTL;
+	if (!(id & CAN_RTR_FLAG)) {
+		/* transmission */
+		dir = CAN_IF_A2R_DIR;
+		cmr |= CAN_IF_CMR_D30 | CAN_IF_CMR_C74;
+	}
+	sta2x11_can_write_reg(priv, cmr, CAN_IF1_CMR);
+
+	sta2x11_can_ar_config(priv, id, dir);
+
+	/* control */
+	ctrl = CAN_IF_MCR_TXR | CAN_IF_MCR_EOB;
+
+	if (dir) {
+		/* control */
+		ctrl |= dlc;
+		/* Write data to IF1 data registers */
+		sta2x11_can_write_data(priv, cf, dlc);
+	}
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		/* use polling in one-shot mode */
+		ctrl |= CAN_IF_MCR_NEWD;
+	else
+		ctrl |= CAN_IF_MCR_TXIE;
+
+	sta2x11_can_write_reg(priv, ctrl, CAN_IF1_MCR);
+
+	/* start data transfer to RAM by writing on CRR the destination */
+	sta2x11_can_write_reg(priv, STA2X11_OBJ_TX, CAN_IF1_CRR);
+
+	stats->tx_bytes += dlc;
+	dev->trans_start = jiffies;
+
+	can_put_echo_skb(skb, dev, 0);
+
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) {
+		/*
+		 * when automatic re-transmission mode is disabled the txRqst
+		 * bit of the respective message buffer is not set,
+		 * we don't know if the transmission started or not ...
+		 */
+		mod_timer(&priv->txtimer, jiffies + HZ / 100);
+	}
+	return NETDEV_TX_OK;
+}
+
+static int sta2x11_can_err(struct net_device *dev, uint32_t status)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *pcf;
+	struct sk_buff *skb;
+	uint32_t err, rec, tec, lec;
+	uint32_t can_data[4] = {0};
+	canid_t can_id = 0;
+	int i;
+
+	dev_dbg(dev->dev.parent, "status interrupt (0x%04x)\n", status);
+
+	if (status & CAN_SR_BOFF) {
+		if (priv->can.state != CAN_STATE_BUS_OFF) {
+			dev_dbg(dev->dev.parent, "entering busoff state\n");
+			/* disable interrupts */
+			sta2x11_can_write_reg(priv, CAN_CR_INI, CAN_CR);
+			can_id |= CAN_ERR_BUSOFF;
+			priv->can.state = CAN_STATE_BUS_OFF;
+			can_bus_off(dev);
+		}
+	} else if (status & CAN_SR_EPAS) {
+		if (priv->can.state != CAN_STATE_ERROR_PASSIVE) {
+			dev_dbg(dev->dev.parent,
+				"entering error passive state\n");
+			can_id |= CAN_ERR_CRTL;
+
+			err = sta2x11_can_read_reg(priv, CAN_ERR);
+			tec = (err & CAN_ERR_TEC);
+			rec = (err & CAN_ERR_REC) >> 8;
+
+			if (tec > rec)
+				can_data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
+			else
+				can_data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
+
+			priv->can.state = CAN_STATE_ERROR_PASSIVE;
+			priv->can.can_stats.error_passive++;
+		}
+	} else if (status & CAN_SR_WARN) {
+		if (priv->can.state != CAN_STATE_ERROR_WARNING) {
+			dev_dbg(dev->dev.parent,
+				"entering error warning state\n");
+			can_id |= CAN_ERR_CRTL;
+
+			err = sta2x11_can_read_reg(priv, CAN_ERR);
+			tec = (err & CAN_ERR_TEC);
+			rec = (err & CAN_ERR_REC) >> 8;
+
+			if (tec > rec)
+				can_data[1] |= CAN_ERR_CRTL_TX_WARNING;
+			else
+				can_data[1] |= CAN_ERR_CRTL_RX_WARNING;
+
+			priv->can.state = CAN_STATE_ERROR_WARNING;
+			priv->can.can_stats.error_warning++;
+		}
+	} else if (priv->can.state != CAN_STATE_ERROR_ACTIVE) {
+		dev_dbg(dev->dev.parent, "entering error active state\n");
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	}
+
+	lec = status & CAN_SR_LEC;
+
+	if (lec && (lec != CAN_SR_LEC)) {
+		if (lec == CAN_SR_LEC_ACK) {
+			dev_dbg(dev->dev.parent, "ack error\n");
+			can_id |= CAN_ERR_ACK;
+			stats->tx_errors++;
+		} else {
+			priv->can.can_stats.bus_error++;
+			stats->rx_errors++;
+
+			can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+			switch (lec) {
+			case CAN_SR_LEC_STUFF:
+				dev_dbg(dev->dev.parent, "stuff error\n");
+				can_data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			case CAN_SR_LEC_FORM:
+				dev_dbg(dev->dev.parent, "form error\n");
+				can_data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case CAN_SR_LEC_BIT1:
+				dev_dbg(dev->dev.parent, "bit1 error\n");
+				can_data[2] |= CAN_ERR_PROT_BIT1;
+				break;
+			case CAN_SR_LEC_BIT0:
+				dev_dbg(dev->dev.parent, "bit0 error\n");
+				can_data[2] |= CAN_ERR_PROT_BIT0;
+				break;
+			case CAN_SR_LEC_CRC:
+				dev_dbg(dev->dev.parent, "crc error\n");
+				can_data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
+				break;
+			}
+		}
+	}
+
+	if (can_id) {
+		skb = alloc_can_err_skb(dev, &pcf);
+		if (unlikely(!skb))
+			return -ENOMEM;
+		pcf->can_id |= can_id;
+		for (i = 0; i < 4; i++)
+			pcf->data[i] = can_data[i];
+		netif_rx(skb);
+
+		stats->rx_packets++;
+		stats->rx_bytes += pcf->can_dlc;
+	}
+	return 0;
+}
+
+static int sta2x11_can_status_interrupt(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	uint32_t status;
+
+	/* get status */
+	status = sta2x11_can_read_reg(priv, CAN_SR);
+	/* reset the status register including RXOK and TXOK */
+	sta2x11_can_write_reg(priv, CAN_SR_LEC, CAN_SR);
+
+	return sta2x11_can_err(dev, status);
+}
+
+/*
+ * Reading data from the Interface Register 2
+ */
+static void sta2x11_can_read_data(struct sta2x11_priv *priv,
+			      struct can_frame *cf, u8 dlc)
+{
+	int data_reg[] = CAN_IF2_DATAV;
+	uint32_t val = 0;
+	int i;
+
+	for (i = 0; i < dlc; i++) {
+		if (i & 0x1) {
+			cf->data[i] = val >> 8;
+		} else {
+			val = sta2x11_can_read_reg(priv, data_reg[i / 2]);
+			cf->data[i] = val & 0xFF;
+		}
+	}
+}
+
+static void sta2x11_can_rx(struct net_device *dev, unsigned int mo, uint32_t ctrl)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	uint32_t arb1, arb2;
+
+	skb = alloc_can_skb(dev, &cf);
+	if (skb == NULL)
+		return;
+
+	/* Read Arbitration 2 Register */
+	arb2 = sta2x11_can_read_reg(priv, CAN_IF2_A2R);
+	if (arb2 & CAN_IF_A2R_XTD) {
+		/* Massage has an extended identifier */
+		arb1 = sta2x11_can_read_reg(priv, CAN_IF2_A1R);
+		cf->can_id = (((arb2 & 0x1FFF) << 16) | arb1 | CAN_EFF_FLAG);
+	} else {
+		/* Massage hasn't an extended identifier */
+		cf->can_id = ((arb2 & 0x1FFF) >> 2);
+	}
+
+	if (arb2 & CAN_IF_A2R_DIR) {
+		cf->can_id |= CAN_RTR_FLAG;
+		cf->can_dlc = 0;
+	} else {
+		cf->can_dlc = get_can_dlc(ctrl & 0xF);
+		sta2x11_can_read_data(priv, cf, cf->can_dlc);
+	}
+
+	netif_rx(skb);
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+}
+
+static void sta2x11_can_rx_interrupt(struct net_device *dev, unsigned int mo)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *pcf;
+	struct sk_buff *skb;
+	uint32_t ctrl;
+
+	/* clear interrupt, read control, data, arbitration */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_CPI | CAN_IF_CMR_CND |
+				    CAN_IF_CMR_AR | CAN_IF_CMR_CTL |
+				    CAN_IF_CMR_D30 | CAN_IF_CMR_C74,
+				    CAN_IF2_CMR);
+	sta2x11_can_write_reg(priv, mo, CAN_IF2_CRR);
+
+	ctrl = sta2x11_can_read_reg(priv, CAN_IF2_MCR);
+
+	if (ctrl & CAN_IF_MCR_MSGL) {
+		dev_dbg(dev->dev.parent, "rx overrun error\n");
+		stats->rx_over_errors++;
+		stats->rx_errors++;
+		skb = alloc_can_err_skb(dev, &pcf);
+		if (likely(skb)) {
+			pcf->can_id |= CAN_ERR_CRTL;
+			pcf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+			netif_rx(skb);
+
+			stats->rx_packets++;
+			stats->rx_bytes += pcf->can_dlc;
+		}
+	}
+
+	sta2x11_can_rx(dev, mo, ctrl);
+
+	/* reset message object */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR |
+				    CAN_IF_CMR_CTL, CAN_IF2_CMR);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF2_M1R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF2_M2R);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF2_A1R);
+	sta2x11_can_write_reg(priv, CAN_IF_A2R_MSGVAL, CAN_IF2_A2R);
+	sta2x11_can_write_reg(priv, CAN_IF_MCR_RXIE | CAN_IF_MCR_UMSK |
+				    CAN_IF_MCR_EOB, CAN_IF2_MCR);
+	sta2x11_can_write_reg(priv, mo, CAN_IF2_CRR);
+}
+
+static void sta2x11_can_tx_interrupt(struct net_device *dev, unsigned int mo)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+
+	/* clear interrupt */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_CPI | CAN_IF_CMR_CTL,
+				    CAN_IF2_CMR);
+	sta2x11_can_write_reg(priv, mo, CAN_IF2_CRR);
+
+	/* invalidate */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR,
+				    CAN_IF2_CMR);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF2_A2R);
+	sta2x11_can_write_reg(priv, mo, CAN_IF2_CRR);
+
+	stats->tx_packets++;
+	can_get_echo_skb(dev, 0);
+	netif_wake_queue(dev);
+}
+
+static irqreturn_t sta2x11_can_interrupt(int irq, void *dev_id)
+{
+	struct net_device *dev = (struct net_device *)dev_id;
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	uint32_t intid;
+	int n = 0;
+
+	/* shared interrupts and IRQ off? */
+	if (priv->can.state == CAN_STATE_STOPPED)
+		return IRQ_NONE;
+
+	while (n < STA2X11_MAX_IRQ) {
+
+		/* read the highest pending interrupt request */
+		intid = sta2x11_can_read_reg(priv, CAN_IDR);
+		if (!intid)
+			break;
+
+		switch (intid) {
+		case CAN_IDR_STATUS:
+			sta2x11_can_status_interrupt(dev);
+			break;
+		case STA2X11_OBJ_RX:
+			sta2x11_can_rx_interrupt(dev, intid);
+			break;
+		case STA2X11_OBJ_TX:
+			sta2x11_can_tx_interrupt(dev, intid);
+			break;
+		default:
+			dev_err(dev->dev.parent, "Unexpected interrupt %i",
+				intid);
+			sta2x11_can_clear_interrupts(priv);
+			break;
+		}
+
+		n++;
+	}
+
+	return n ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int sta2x11_can_open(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	int err;
+
+	sta2x11_can_reset_mode(dev);
+
+	err = open_candev(dev);
+	if (err)
+		return err;
+
+	err = request_irq(dev->irq, &sta2x11_can_interrupt, 0 /* FIXME */,
+			  dev->name, (void *)dev);
+	if (err) {
+		close_candev(dev);
+		return -EAGAIN;
+	}
+
+	sta2x11_can_start(dev);
+	priv->open_time = jiffies;
+	netif_start_queue(dev);
+
+	return 0;
+}
+
+static int sta2x11_can_close(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+	netif_stop_queue(dev);
+	sta2x11_can_reset_mode(dev);
+
+	free_irq(dev->irq, (void *)dev);
+	close_candev(dev);
+
+	priv->open_time = 0;
+
+	return 0;
+}
+
+static int sta2x11_can_set_bittiming(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	uint32_t reg;
+
+	reg = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
+	    (((bt->phase_seg2 - 1) & 0x7) << 4);
+	reg <<= 8;
+	reg |= ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
+	sta2x11_can_write_reg(priv, reg, CAN_BTR);
+
+	reg = ((bt->brp - 1) >> 6) & 0xf;
+	sta2x11_can_write_reg(priv, reg, CAN_BRPR);
+
+	return 0;
+}
+
+static int sta2x11_can_set_mode(struct net_device *dev, enum can_mode mode)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+	if (!priv->open_time)
+		return -EINVAL;
+
+	switch (mode) {
+	case CAN_MODE_START:
+		sta2x11_can_start(dev);
+		if (netif_queue_stopped(dev))
+			netif_wake_queue(dev);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static struct net_device *sta2x11_can_alloc(void)
+{
+	struct net_device *dev;
+	struct sta2x11_priv *priv;
+
+	dev = alloc_candev(sizeof(struct sta2x11_priv), STA2X11_ECHO_SKB_MAX);
+	if (!dev)
+		return NULL;
+
+	priv = netdev_priv(dev);
+
+	priv->dev = dev;
+
+	/* Configuring CAN */
+	priv->can.bittiming_const = &sta2x11_can_bittiming_const;
+	priv->can.do_set_bittiming = sta2x11_can_set_bittiming;
+	priv->can.do_set_mode = sta2x11_can_set_mode;
+	priv->can.clock.freq = STA2X11_APB_FREQ / 2;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_ONE_SHOT |
+				       CAN_CTRLMODE_BERR_REPORTING;
+
+	return dev;
+}
+
+static void sta2x11_can_free(struct net_device *dev)
+{
+	free_candev(dev);
+}
+
+static const struct net_device_ops sta2x11_can_netdev_ops = {
+	.ndo_open = sta2x11_can_open,
+	.ndo_stop = sta2x11_can_close,
+	.ndo_start_xmit = sta2x11_can_start_xmit,
+};
+
+static void sta2x11_can_tx_poll(unsigned long xdev)
+{
+	struct net_device *dev = (struct net_device *)xdev;
+	struct sta2x11_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+
+	if (sta2x11_can_read_reg(priv, CAN_ND1R) & STA2X11_OBJ_TX) {
+		dev_dbg(dev->dev.parent, "one-shot tx failed\n");
+		stats->tx_errors++;
+		stats->tx_dropped++;
+		can_free_echo_skb(dev, 0);
+	} else {
+		stats->tx_packets++;
+		can_get_echo_skb(dev, 0);
+	}
+
+	/* invalidate */
+	sta2x11_can_write_reg(priv, CAN_IF_CMR_WR | CAN_IF_CMR_AR,
+				    CAN_IF1_CMR);
+	sta2x11_can_write_reg(priv, 0x0, CAN_IF1_A2R);
+	sta2x11_can_write_reg(priv, STA2X11_OBJ_TX, CAN_IF1_CRR);
+
+	netif_wake_queue(dev);
+}
+
+static int sta2x11_can_register(struct net_device *dev)
+{
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+	/* local echo */
+	dev->flags |= IFF_ECHO;
+	dev->netdev_ops = &sta2x11_can_netdev_ops;
+
+	/* init timer handling tx request poll for one-shot mode */
+	init_timer(&priv->txtimer);
+	priv->txtimer.data = (unsigned long)dev;
+	priv->txtimer.function = sta2x11_can_tx_poll;
+
+	sta2x11_can_chipset_init(dev);
+	sta2x11_can_reset_mode(dev);
+
+	return register_candev(dev);
+}
+
+static void sta2x11_can_unregister(struct net_device *dev)
+{
+	sta2x11_can_reset_mode(dev);
+	unregister_candev(dev);
+}
+
+DEFINE_PCI_DEVICE_TABLE(sta2x11_can_pci_tbl) = {
+	{
+		PCI_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN),
+		.driver_data = 0,
+	},
+	{},
+};
+
+/*
+ * Static definition of debugfs 32bit registers, on sta2x11 there is only
+ * one CAN bus
+ */
+#define REG(regname) {.name = #regname, .offset = regname}
+static struct debugfs_reg32 sta2x11_can_regs[] = {
+	REG(CAN_CR), REG(CAN_SR), REG(CAN_ERR), REG(CAN_BTR),
+	REG(CAN_BRPR), REG(CAN_IDR), REG(CAN_TXR1R), REG(CAN_TXR2R),
+	REG(CAN_ND1R), REG(CAN_ND2R), REG(CAN_IP1R), REG(CAN_IP2R),
+	REG(CAN_MV1R), REG(CAN_MV2R),
+};
+#undef REG
+static struct debugfs_regset32 sta2x11_can_regset = {
+	.regs = sta2x11_can_regs,
+	.nregs = ARRAY_SIZE(sta2x11_can_regs),
+};
+
+static int __devinit
+sta2x11_can_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+	struct net_device *dev;
+	struct sta2x11_priv *priv;
+	int rc = 0;
+
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "pci_enable_device FAILED\n");
+		goto out;
+	}
+
+	rc = pci_request_regions(pdev, KBUILD_MODNAME);
+	if (rc) {
+		dev_err(&pdev->dev, "pci_request_regions FAILED\n");
+		goto out_disable_device;
+	}
+
+	pci_set_master(pdev);
+	pci_enable_msi(pdev);
+
+	dev = sta2x11_can_alloc();
+	if (!dev) {
+		rc = -ENOMEM;
+		goto out_release_regions;
+	}
+
+	dev->irq = pdev->irq;
+	priv = netdev_priv(dev);
+
+	priv->reg_base = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+
+	if (!priv->reg_base) {
+		dev_err(&pdev->dev,
+			"device has no PCI memory resources, "
+			"failing adapter\n");
+		rc = -ENOMEM;
+		goto out_kfree_sta2x11;
+	}
+
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	rc = sta2x11_can_register(dev);
+	if (rc) {
+		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
+			KBUILD_MODNAME, rc);
+		goto out_iounmap;
+	}
+
+	pci_set_drvdata(pdev, dev);
+
+	/* Configure debugfs */
+	sta2x11_can_regset.base = priv->reg_base;
+	priv->dentry = debugfs_create_regset32("sta2x11_can", S_IFREG | S_IRUGO,
+			NULL, &sta2x11_can_regset);
+
+	return 0;
+
+out_iounmap:
+	pci_iounmap(pdev, priv->reg_base);
+out_kfree_sta2x11:
+	sta2x11_can_free(dev);
+out_release_regions:
+	pci_disable_msi(pdev);
+	pci_release_regions(pdev);
+out_disable_device:
+	/*
+	 * do not call pci_disable_device on sta2x11 because it
+	 * break all other Bus masters on this EP
+	 */
+out:
+	return rc;
+}
+
+static void __devexit sta2x11_can_pci_remove(struct pci_dev *pdev)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	struct sta2x11_priv *priv = netdev_priv(dev);
+
+
+	if (priv->dentry)
+		debugfs_remove(priv->dentry);
+
+	pci_set_drvdata(pdev, NULL);
+
+	sta2x11_can_unregister(dev);
+	pci_iounmap(pdev, priv->reg_base);
+	sta2x11_can_free(dev);
+
+	pci_disable_msi(pdev);
+	pci_release_regions(pdev);
+	/*
+	 * do not call pci_disable_device on sta2x11 because it
+	 * break all other Bus masters on this EP
+	 */
+}
+
+static struct pci_driver sta2x11_pci_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = sta2x11_can_pci_tbl,
+	.probe = sta2x11_can_pci_probe,
+	.remove = __devexit_p(sta2x11_can_pci_remove),
+};
+
+static __init int sta2x11_can_init(void)
+{
+	return pci_register_driver(&sta2x11_pci_driver);
+}
+
+/* needs to be started after the sta2x11_apbreg driver */
+late_initcall(sta2x11_can_init);
+
+static __exit void sta2x11_can_exit(void)
+{
+	pci_unregister_driver(&sta2x11_pci_driver);
+}
+
+module_exit(sta2x11_can_exit);
+
+MODULE_AUTHOR("Wind River");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION(KBUILD_MODNAME "CAN netdevice driver");
+MODULE_DEVICE_TABLE(pci, sta2x11_pci_tbl);
-- 
1.7.7.6

Re: [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

[Wolfgang Grandegger]

On 05/17/2012 10:59 PM, Federico Vaga wrote:
> Signed-off-by: Federico Vaga <federico.vaga@gmail.com>
> Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
> Cc: Alan Cox <alan@linux.intel.com>
> ---
>  drivers/net/can/Kconfig       |   11 +
>  drivers/net/can/Makefile      |    1 +
>  drivers/net/can/sta2x11_can.c | 1085 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1097 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/sta2x11_can.c

Thanks for your contribution. At a first glance, this driver looks
similar to the pch_can and the c_can driver. It seems that a C_CAN based
controller is used on that board as well. If that's true, it should be
handled by the C_CAN driver. To get ride of the obsolete pch_can driver,
I sent some time ago the patch "[RFC/PATCH] c_can: add driver for the
PCH CAN controller":

  http://marc.info/?t=132991563600003&r=1&w=4

I could serve as base of a generic c_can_pci driver.

Wolfgang.

Re: [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

[Federico Vaga]

> Thanks for your contribution. At a first glance, this driver looks
> similar to the pch_can and the c_can driver. It seems that a C_CAN based
> controller is used on that board as well. If that's true, it should be
> handled by the C_CAN driver. To get ride of the obsolete pch_can driver,
> I sent some time ago the patch "[RFC/PATCH] c_can: add driver for the
> PCH CAN controller":
>
>  http://marc.info/?t=132991563600003&r=1&w=4
>
> I could serve as base of a generic c_can_pci driver.

You are right, this is a C-CAN but it isn't specified on the board manual.
I compared the board manual with the Bosch C-CAN manual, and it is the same.
I'm working on a PCI module for this board, when it is ready we can think for
a generic c_can_pci

Thank you

-- 
Federico Vaga

Re: [PATCH] STA2X11 CAN: CAN driver for the STA2X11 board

[Wolfgang Grandegger]

On 05/26/2012 10:36 AM, Federico Vaga wrote:
>> Thanks for your contribution. At a first glance, this driver looks
>> similar to the pch_can and the c_can driver. It seems that a C_CAN based
>> controller is used on that board as well. If that's true, it should be
>> handled by the C_CAN driver. To get ride of the obsolete pch_can driver,
>> I sent some time ago the patch "[RFC/PATCH] c_can: add driver for the
>> PCH CAN controller":
>>
>>  http://marc.info/?t=132991563600003&r=1&w=4
>>
>> I could serve as base of a generic c_can_pci driver.
> 
> You are right, this is a C-CAN but it isn't specified on the board manual.
> I compared the board manual with the Bosch C-CAN manual, and it is the same.

Like with the Intel EG20T PCH CAN controller.

> I'm working on a PCI module for this board, when it is ready we can think for
> a generic c_can_pci

That would be nice, thanks. You might have realized that AnilKumar has
provided support for the D_CAN controllers to the C_CAN driver which
will show up mainline soon.

Wolfgang.

generic module for c-can on pci

[Federico Vaga]

As suggested I developed a generic module for C-CAN
on PCI. Probably I will do some changes about our
specific board, but I think that the module is generic
enough.

[PATCH RFC] c_can_pci: generic module for c_can on PCI

[Federico Vaga]

Signed-off-by: Federico Vaga <federico.vaga@gmail.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Alan Cox <alan@linux.intel.com>
---
 drivers/net/can/c_can/Kconfig     |   11 +-
 drivers/net/can/c_can/Makefile    |    1 +
 drivers/net/can/c_can/c_can_pci.c |  221 +++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/can/c_can/c_can_pci.c

diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
index ffb9773..74ef97d 100644
--- a/drivers/net/can/c_can/Kconfig
+++ b/drivers/net/can/c_can/Kconfig
@@ -2,14 +2,19 @@ menuconfig CAN_C_CAN
 	tristate "Bosch C_CAN devices"
 	depends on CAN_DEV && HAS_IOMEM
 
-if CAN_C_CAN
-
 config CAN_C_CAN_PLATFORM
 	tristate "Generic Platform Bus based C_CAN driver"
+	depends on CAN_C_CAN
 	---help---
 	  This driver adds support for the C_CAN chips connected to
 	  the "platform bus" (Linux abstraction for directly to the
 	  processor attached devices) which can be found on various
 	  boards from ST Microelectronics (http://www.st.com)
 	  like the SPEAr1310 and SPEAr320 evaluation boards.
-endif
+
+config CAN_C_CAN_PCI
+	tristate "Generic PCI Bus based C_CAN driver"
+	depends on CAN_C_CAN
+	---help---
+	  This driver adds support for the C_CAN chips connected to
+	  the PCI bus.
diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile
index 9273f6d..ad1cc84 100644
--- a/drivers/net/can/c_can/Makefile
+++ b/drivers/net/can/c_can/Makefile
@@ -4,5 +4,6 @@
 
 obj-$(CONFIG_CAN_C_CAN) += c_can.o
 obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
+obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
new file mode 100644
index 0000000..b635375
--- /dev/null
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -0,0 +1,221 @@
+/*
+ * Platform CAN bus driver for Bosch C_CAN controller
+ *
+ * Copyright (C) 2012 Federico Vaga <federico.vaga@gmail.com>
+  *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/clk.h>
+#include <linux/pci.h>
+#include <linux/can/dev.h>
+
+#include "c_can.h"
+
+enum c_can_pci_reg_align {
+	C_CAN_REG_ALIGN_16,
+	C_CAN_REG_ALIGN_32,
+};
+
+struct c_can_pci_data {
+	unsigned int reg_align;	/* Set the register alignment in the memory */
+	unsigned int freq;	/* Set the frequency if clk is not usable */
+};
+
+/*
+ * 16-bit c_can registers can be arranged differently in the memory
+ * architecture of different implementations. For example: 16-bit
+ * registers can be aligned to a 16-bit boundary or 32-bit boundary etc.
+ * Handle the same by providing a common read/write interface.
+ */
+static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
+						void *reg)
+{
+	return readw(reg);
+}
+
+static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv,
+						void *reg, u16 val)
+{
+	writew(val, reg);
+}
+
+static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv,
+						void *reg)
+{
+	return readw(reg + (long)reg - (long)priv->regs);
+}
+
+static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
+						void *reg, u16 val)
+{
+	writew(val, reg + (long)reg - (long)priv->regs);
+}
+
+static int __devinit c_can_pci_probe(struct pci_dev *pdev,
+				     const struct pci_device_id *ent)
+{
+	struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data;
+	struct c_can_priv *priv;
+	struct net_device *dev;
+	void __iomem *addr;
+	struct clk *clk;
+	int ret;
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "pci_enable_device FAILED\n");
+		goto out;
+	}
+
+	ret = pci_request_regions(pdev, KBUILD_MODNAME);
+	if (ret) {
+		dev_err(&pdev->dev, "pci_request_regions FAILED\n");
+		goto out_disable_device;
+	}
+
+	pci_set_master(pdev);
+	pci_enable_msi(pdev);
+
+	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+	if (!addr) {
+		dev_err(&pdev->dev,
+			"device has no PCI memory resources, "
+			"failing adapter\n");
+		ret = -ENOMEM;
+		goto out_release_regions;
+	}
+
+	/* allocate the c_can device */
+	dev = alloc_c_can_dev();
+	if (!dev) {
+		ret = -ENOMEM;
+		goto out_iounmap;
+	}
+
+	priv = netdev_priv(dev);
+	pci_set_drvdata(pdev, dev);
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	dev->irq = pdev->irq;
+	priv->regs = addr;
+
+	if (!c_can_pci_data->freq) {
+		/* get the appropriate clk */
+		clk = clk_get(&pdev->dev, NULL);
+		if (IS_ERR(clk)) {
+			dev_err(&pdev->dev, "no clock defined\n");
+			ret = -ENODEV;
+			goto out_free_c_can;
+		}
+		priv->can.clock.freq = clk_get_rate(clk);
+		priv->priv = clk;
+	} else {
+		priv->can.clock.freq = c_can_pci_data->freq;
+		priv->priv = NULL;
+	}
+
+	switch (c_can_pci_data->reg_align) {
+	case C_CAN_REG_ALIGN_32:
+		priv->read_reg = c_can_pci_read_reg_aligned_to_32bit;
+		priv->write_reg = c_can_pci_write_reg_aligned_to_32bit;
+		break;
+	case C_CAN_REG_ALIGN_16:
+	default:
+		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
+		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
+		break;
+	}
+
+	ret = register_c_can_dev(dev);
+	if (ret) {
+		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
+			KBUILD_MODNAME, ret);
+		goto out_free_clock;
+	}
+
+	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
+		 KBUILD_MODNAME, priv->regs, dev->irq);
+
+	return 0;
+
+out_free_clock:
+	if (!priv->priv)
+		clk_put(priv->priv);
+out_free_c_can:
+	pci_set_drvdata(pdev, NULL);
+	free_c_can_dev(dev);
+out_iounmap:
+	pci_iounmap(pdev, addr);
+out_release_regions:
+	pci_disable_msi(pdev);
+	pci_clear_master(pdev);
+	pci_release_regions(pdev);
+out_disable_device:
+	/*
+	 * do not call pci_disable_device on sta2x11 because it
+	 * break all other Bus masters on this EP
+	 */
+	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
+	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
+		goto out;
+	pci_disable_device(pdev);
+out:
+	return ret;
+}
+
+static void __devexit c_can_pci_remove(struct pci_dev *pdev)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	struct c_can_priv *priv = netdev_priv(dev);
+
+	pci_set_drvdata(pdev, NULL);
+	free_c_can_dev(dev);
+	if (!priv->priv)
+		clk_put(priv->priv);
+	pci_iounmap(pdev, priv->regs);
+	pci_disable_msi(pdev);
+	pci_clear_master(pdev);
+	pci_release_regions(pdev);
+	/*
+	 * do not call pci_disable_device on sta2x11 because it
+	 * break all other Bus masters on this EP
+	 */
+	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
+	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
+		return;
+	pci_disable_device(pdev);
+}
+
+static struct c_can_pci_data c_can_sta2x11= {
+	.reg_align = C_CAN_REG_ALIGN_32,
+	.freq = 52000000, /* 52 Mhz */
+};
+
+#define C_CAN_ID(_vend, _dev, _driverdata) {		\
+	PCI_DEVICE(_vend, _dev),			\
+	.driver_data = (unsigned long)&_driverdata,	\
+}
+DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
+	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
+		 c_can_sta2x11),
+	{},
+};
+static struct pci_driver sta2x11_pci_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = c_can_pci_tbl,
+	.probe = c_can_pci_probe,
+	.remove = __devexit_p(c_can_pci_remove),
+};
+
+module_pci_driver(sta2x11_pci_driver);
+
+MODULE_AUTHOR("Federico Vaga <federico.vaga@gmail.com>");
+MODULE_LICENSE("GPL V2");
+MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller");
+MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);
-- 
1.7.10.2

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Marc Kleine-Budde]

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

On 06/04/2012 03:32 PM, Federico Vaga wrote:
> Signed-off-by: Federico Vaga <federico.vaga@gmail.com>
> Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
> Cc: Alan Cox <alan@linux.intel.com>

Please port you driver to the recent c_can changes. Use the c_can branch
of the linux-can-next repo[1] as base for your work. You have to rework
the register access function. Please have a look if there are devm_
variants for the registration/mapping of the pci and clock.

[1] https://gitorious.org/linux-can/linux-can-next

More comments inline. Marc

> ---
>  drivers/net/can/c_can/Kconfig     |   11 +-
>  drivers/net/can/c_can/Makefile    |    1 +
>  drivers/net/can/c_can/c_can_pci.c |  221 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/can/c_can/c_can_pci.c
> 
> diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
> index ffb9773..74ef97d 100644
> --- a/drivers/net/can/c_can/Kconfig
> +++ b/drivers/net/can/c_can/Kconfig
> @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN
>  	tristate "Bosch C_CAN devices"
>  	depends on CAN_DEV && HAS_IOMEM
>  
> -if CAN_C_CAN

please keep the if CAN_C_CAN...

> -
>  config CAN_C_CAN_PLATFORM
>  	tristate "Generic Platform Bus based C_CAN driver"
> +	depends on CAN_C_CAN

...then you don't have to add the depends on here.

>  	---help---
>  	  This driver adds support for the C_CAN chips connected to
>  	  the "platform bus" (Linux abstraction for directly to the
>  	  processor attached devices) which can be found on various
>  	  boards from ST Microelectronics (http://www.st.com)
>  	  like the SPEAr1310 and SPEAr320 evaluation boards.
> -endif

... Just move you pci driver inside the if...endif block...
> +
> +config CAN_C_CAN_PCI
> +	tristate "Generic PCI Bus based C_CAN driver"
> +	depends on CAN_C_CAN

...and remove the depends on CAN_C_CAN. You probably have to add a
depends on PCI.

> +	---help---
> +	  This driver adds support for the C_CAN chips connected to
> +	  the PCI bus.
> diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile
> index 9273f6d..ad1cc84 100644
> --- a/drivers/net/can/c_can/Makefile
> +++ b/drivers/net/can/c_can/Makefile
> @@ -4,5 +4,6 @@
>  
>  obj-$(CONFIG_CAN_C_CAN) += c_can.o
>  obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
> +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> new file mode 100644
> index 0000000..b635375
> --- /dev/null
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -0,0 +1,221 @@
> +/*
> + * Platform CAN bus driver for Bosch C_CAN controller
> + *
> + * Copyright (C) 2012 Federico Vaga <federico.vaga@gmail.com>
> +  *
   ^^^ double space :)

> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/clk.h>
> +#include <linux/pci.h>
> +#include <linux/can/dev.h>
> +
> +#include "c_can.h"
> +
> +enum c_can_pci_reg_align {
> +	C_CAN_REG_ALIGN_16,
> +	C_CAN_REG_ALIGN_32,
> +};
> +
> +struct c_can_pci_data {
> +	unsigned int reg_align;	/* Set the register alignment in the memory */
        ^^^^^^^^^^^^
use the enum you defined above.

> +	unsigned int freq;	/* Set the frequency if clk is not usable */
> +};
> +
> +/*
> + * 16-bit c_can registers can be arranged differently in the memory
> + * architecture of different implementations. For example: 16-bit
> + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc.
> + * Handle the same by providing a common read/write interface.
> + */
> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
> +						void *reg)
> +{
> +	return readw(reg);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv,
> +						void *reg, u16 val)
> +{
> +	writew(val, reg);
> +}
> +
> +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv,
> +						void *reg)
> +{
> +	return readw(reg + (long)reg - (long)priv->regs);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
> +						void *reg, u16 val)
> +{
> +	writew(val, reg + (long)reg - (long)priv->regs);
> +}
> +
> +static int __devinit c_can_pci_probe(struct pci_dev *pdev,
> +				     const struct pci_device_id *ent)
> +{
> +	struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data;
> +	struct c_can_priv *priv;
> +	struct net_device *dev;
> +	void __iomem *addr;
> +	struct clk *clk;
> +	int ret;
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "pci_enable_device FAILED\n");
> +		goto out;
> +	}
> +
> +	ret = pci_request_regions(pdev, KBUILD_MODNAME);
> +	if (ret) {
> +		dev_err(&pdev->dev, "pci_request_regions FAILED\n");
> +		goto out_disable_device;
> +	}
> +
> +	pci_set_master(pdev);
> +	pci_enable_msi(pdev);
> +
> +	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> +	if (!addr) {
> +		dev_err(&pdev->dev,
> +			"device has no PCI memory resources, "
> +			"failing adapter\n");
> +		ret = -ENOMEM;
> +		goto out_release_regions;
> +	}
> +
> +	/* allocate the c_can device */
> +	dev = alloc_c_can_dev();
> +	if (!dev) {
> +		ret = -ENOMEM;
> +		goto out_iounmap;
> +	}
> +
> +	priv = netdev_priv(dev);
> +	pci_set_drvdata(pdev, dev);
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	dev->irq = pdev->irq;
> +	priv->regs = addr;
> +
> +	if (!c_can_pci_data->freq) {
> +		/* get the appropriate clk */
> +		clk = clk_get(&pdev->dev, NULL);
> +		if (IS_ERR(clk)) {
> +			dev_err(&pdev->dev, "no clock defined\n");
> +			ret = -ENODEV;
> +			goto out_free_c_can;
> +		}
> +		priv->can.clock.freq = clk_get_rate(clk);
> +		priv->priv = clk;
> +	} else {
> +		priv->can.clock.freq = c_can_pci_data->freq;
> +		priv->priv = NULL;
> +	}
> +
> +	switch (c_can_pci_data->reg_align) {
> +	case C_CAN_REG_ALIGN_32:
> +		priv->read_reg = c_can_pci_read_reg_aligned_to_32bit;
> +		priv->write_reg = c_can_pci_write_reg_aligned_to_32bit;
> +		break;
> +	case C_CAN_REG_ALIGN_16:
> +	default:
> +		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
> +		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
> +		break;
> +	}
> +
> +	ret = register_c_can_dev(dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> +			KBUILD_MODNAME, ret);
> +		goto out_free_clock;
> +	}
> +
> +	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> +		 KBUILD_MODNAME, priv->regs, dev->irq);
> +
> +	return 0;
> +
> +out_free_clock:
> +	if (!priv->priv)
           ^^^

looks fishy

> +		clk_put(priv->priv);
> +out_free_c_can:
> +	pci_set_drvdata(pdev, NULL);
> +	free_c_can_dev(dev);
> +out_iounmap:
> +	pci_iounmap(pdev, addr);
> +out_release_regions:
> +	pci_disable_msi(pdev);
> +	pci_clear_master(pdev);
> +	pci_release_regions(pdev);
> +out_disable_device:
> +	/*
> +	 * do not call pci_disable_device on sta2x11 because it
> +	 * break all other Bus masters on this EP
> +	 */
> +	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> +	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> +		goto out;
> +	pci_disable_device(pdev);
> +out:
> +	return ret;
> +}
> +
> +static void __devexit c_can_pci_remove(struct pci_dev *pdev)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +	struct c_can_priv *priv = netdev_priv(dev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +	free_c_can_dev(dev);
> +	if (!priv->priv)
dito
> +		clk_put(priv->priv);
> +	pci_iounmap(pdev, priv->regs);
> +	pci_disable_msi(pdev);
> +	pci_clear_master(pdev);
> +	pci_release_regions(pdev);
> +	/*
> +	 * do not call pci_disable_device on sta2x11 because it
> +	 * break all other Bus masters on this EP
> +	 */
> +	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> +	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> +		return;
> +	pci_disable_device(pdev);
> +}
> +
> +static struct c_can_pci_data c_can_sta2x11= {
> +	.reg_align = C_CAN_REG_ALIGN_32,
> +	.freq = 52000000, /* 52 Mhz */
> +};
> +
> +#define C_CAN_ID(_vend, _dev, _driverdata) {		\
> +	PCI_DEVICE(_vend, _dev),			\
> +	.driver_data = (unsigned long)&_driverdata,	\
> +}
> +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
^^^^

static?

> +	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
> +		 c_can_sta2x11),
> +	{},
> +};
> +static struct pci_driver sta2x11_pci_driver = {
> +	.name = KBUILD_MODNAME,
> +	.id_table = c_can_pci_tbl,
> +	.probe = c_can_pci_probe,
> +	.remove = __devexit_p(c_can_pci_remove),
> +};
> +
> +module_pci_driver(sta2x11_pci_driver);
> +
> +MODULE_AUTHOR("Federico Vaga <federico.vaga@gmail.com>");
> +MODULE_LICENSE("GPL V2");

IIRC, the correct case is "GPL v2"

> +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller");
> +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);


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

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Alan Cox]

> +enum c_can_pci_reg_align {
> +	C_CAN_REG_ALIGN_16,
> +	C_CAN_REG_ALIGN_32,
> +};

Anythign wrong with 

bool aligned32;

> +
> +struct c_can_pci_data {
> +	unsigned int reg_align;	/* Set the register alignment in the memory */

Not the enum .. indeed

> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
> +						void *reg)

I'm a bit worried this function name might be too short ;)


> +	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> +		 KBUILD_MODNAME, priv->regs, dev->irq);

dev_dbg

> +	 * do not call pci_disable_device on sta2x11 because it
> +	 * break all other Bus masters on this EP
> +	 */
> +	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> +	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> +		goto out;

Is that the disabling or the dropping it into D3. We have a PCI quirk
flag for the latter. See "quirk_no_ata_d3". That will also avoid any
accidents elsewhere. Right now the quirk has "ata" in the name but the
ata is just historically because we had to quirk various disk controllers.

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Alessandro Rubini]

> Anythign wrong with 
> 
> bool aligned32;

I personally think booleans are evil.  But both this and the other
thing:

>> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
>> +						void *reg)
> 
> I'm a bit worried this function name might be too short ;)

come from the platform driver this is based on (I already blamed
federico offlist for not preserving authorship of the original file).

So, this file is mostly copied from the platform driver, which is a
duplication of code.  A mandated duplication, given how the thing
is currently laid out: the c_can core driver exports functions that
the other two files are using (the platform and the new pci driver).

In my opinion, it would be much better to have one less layer and no
exports at all. The core driver should be a platform driver, and the
pci driver would just build platform data and register the platform
device.

Sure this isn't up to federico, who has the pci device but cannot
access any boards where the previous driver is used.  What do the
maintainers think? I (or federico :) may propose a reshaping, if
the idea makes sense.

/alessandro, another user of the sta2x11 where c_can_pci lives

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Federico Vaga]

> > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv
> > *priv, +						void *reg)
> 
> I'm a bit worried this function name might be too short ;)

I know :) I was inspired by the same function in c_can_platform.c


About these function I suggest to move them into c_can.c because they 
are the same for c_can_platform.c and c_can_pci.c Then add a new field 
c_can_priv->offset which can be used to shift the register offset 
coherently with the memory alignment. Finally, remove c_can_priv-
>read_reg and c_can_priv->write_reg and use internal c_can.c function to 
read and write registers.

static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
{
	return readw(priv->base + (priv->regs[index] << priv->offset));
}
static void c_can_write_reg(struct c_can_priv *priv, enum reg index,
						u16 val)
{
	writew(val, priv->base + (priv->regs[index] << priv->offset));
}


If it's ok, I can made a patch for this in the next days.

> > +	 * do not call pci_disable_device on sta2x11 because it
> > +	 * break all other Bus masters on this EP
> > +	 */
> > +	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> > +	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> > +		goto out;
> 
> Is that the disabling or the dropping it into D3. We have a PCI quirk
> flag for the latter. See "quirk_no_ata_d3". That will also avoid any
> accidents elsewhere. Right now the quirk has "ata" in the name but the
> ata is just historically because we had to quirk various disk
> controllers.

We are investigating if this is still necessary on the current version 
of the board.

-- 
Federico Vaga

RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Bhupesh SHARMA]

> -----Original Message-----
> From: linux-can-owner@vger.kernel.org [mailto:linux-can-
> owner@vger.kernel.org] On Behalf Of Federico Vaga
> Sent: Monday, June 04, 2012 10:16 PM
> To: Alan Cox
> Cc: Wolfgang Grandegger; Marc Kleine-Budde; Giancarlo ASNAGHI; Alan
> Cox; Alessandro Rubini; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
> 
> > > +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv
> > > *priv, +						void *reg)
> >
> > I'm a bit worried this function name might be too short ;)
> 
> I know :) I was inspired by the same function in c_can_platform.c

There was a purpose to keeping these long function names when I wrote
the c_can_platform driver initially. These were kept to support the 
SoCs (even the flaky ones) which I could trace at that time and used C_CAN controllers
(e.g. Hynix, ST's SPEAr eMPUs, etc..) and had different register bank layouts.

In some of these SoC's the C_CAN registers which are essentially 16-bit or 32-bit
registers are aligned always to a 32-bit boundary (i.e. even a 16-bit register
is aligned to 32-bit boundary).

So, I had to implement two variants of the read/write reg routines. I am not sure your SoC implementation needs them.
If it does, I will categorize it as flaky as well :)
 
> About these function I suggest to move them into c_can.c because they
> are the same for c_can_platform.c and c_can_pci.c Then add a new field
> c_can_priv->offset which can be used to shift the register offset
> coherently with the memory alignment. Finally, remove c_can_priv-
> >read_reg and c_can_priv->write_reg and use internal c_can.c function
> to
> read and write registers.

See above. There was a reason for keeping these routines in c_can_platform.c
Simply put, every platform having a Bosch C_CAN module can have it's own implementation
of the bus (for example you use PCI) and register bank layout (16-bit or 32-bit aligned).

I would suggest to keep the same arrangement.

> static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
> {
> 	return readw(priv->base + (priv->regs[index] << priv->offset));
> }
> static void c_can_write_reg(struct c_can_priv *priv, enum reg index,
> 						u16 val)
> {
> 	writew(val, priv->base + (priv->regs[index] << priv->offset));
> }
> 
> 
> If it's ok, I can made a patch for this in the next days.
> 

[snip..]


Regards,
Bhupesh

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Federico Vaga]

> In some of these SoC's the C_CAN registers which are essentially
> 16-bit or 32-bit registers are aligned always to a 32-bit boundary
> (i.e. even a 16-bit register is aligned to 32-bit boundary).
> 
> So, I had to implement two variants of the read/write reg routines. I
> am not sure your SoC implementation needs them. If it does, I will
> categorize it as flaky as well :)

My implementation is align to 32, but I'm trying to make a generic PCI 
wrapper (some other could be aligned to 16)
 
> See above. There was a reason for keeping these routines in
> c_can_platform.c Simply put, every platform having a Bosch C_CAN
> module can have it's own implementation of the bus (for example you
> use PCI) and register bank layout (16-bit or 32-bit aligned).

I don't understand the reason to keep these functions in 
c_can_platform.c . Two generic read/write functions could be written 
into c_can.c by using a shift value (0 if aligned to 16, 1 if aligned to 
32) as I showed in the previous mail:

> > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
> > {
> > 
> > 	return readw(priv->base + (priv->regs[index] << priv->offset));
> > 
> > }
> > static void c_can_write_reg(struct c_can_priv *priv, enum reg index,
> > 
> > 						u16 val)
> > 
> > {
> > 
> > 	writew(val, priv->base + (priv->regs[index] << priv->offset));
> > 
> > }

Every platform having a Bosch C_CAN/D_CAN can specify its shift value (0 
or 1) and it's done.

-- 
Federico Vaga

RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Bhupesh SHARMA]

> -----Original Message-----
> From: Federico Vaga [mailto:federico.vaga@gmail.com]
> Sent: Tuesday, June 05, 2012 4:49 PM
> To: Bhupesh SHARMA
> Cc: Alan Cox; Wolfgang Grandegger; Marc Kleine-Budde; Giancarlo
> ASNAGHI; Alan Cox; Alessandro Rubini; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
> 
> > In some of these SoC's the C_CAN registers which are essentially
> > 16-bit or 32-bit registers are aligned always to a 32-bit boundary
> > (i.e. even a 16-bit register is aligned to 32-bit boundary).
> >
> > So, I had to implement two variants of the read/write reg routines. I
> > am not sure your SoC implementation needs them. If it does, I will
> > categorize it as flaky as well :)
> 
> My implementation is align to 32, but I'm trying to make a generic PCI
> wrapper (some other could be aligned to 16)

So it means your implementation is also flaky and you are probably wasting HW memory
space while integrating the Bosch C_CAN module in your SoC :)

> > See above. There was a reason for keeping these routines in
> > c_can_platform.c Simply put, every platform having a Bosch C_CAN
> > module can have it's own implementation of the bus (for example you
> > use PCI) and register bank layout (16-bit or 32-bit aligned).
> 
> I don't understand the reason to keep these functions in
> c_can_platform.c . Two generic read/write functions could be written
> into c_can.c by using a shift value (0 if aligned to 16, 1 if aligned
> to
> 32) as I showed in the previous mail:
> 
> > > static u16 c_can_read_reg(struct c_can_priv *priv, enum reg index)
> > > {
> > >
> > > 	return readw(priv->base + (priv->regs[index] << priv->offset));
> > >
> > > }
> > > static void c_can_write_reg(struct c_can_priv *priv, enum reg
> index,
> > >
> > > 						u16 val)
> > >
> > > {
> > >
> > > 	writew(val, priv->base + (priv->regs[index] << priv->offset));
> > >
> > > }
> 
> Every platform having a Bosch C_CAN/D_CAN can specify its shift value
> (0
> or 1) and it's done.
> 

I am not a big fan of adding platform specific flakes in any core file, that why we keep the platform
file separate from the core ones. But I will left Marc and Wolfgang to further comment on the same.

Regards,
Bhupesh

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Alessandro Rubini]

>> My implementation is align to 32, but I'm trying to make a generic PCI
>> wrapper (some other could be aligned to 16)
 
> So it means your implementation is also flaky and you are probably
> wasting HW memory space while integrating the Bosch C_CAN module in
> your SoC :)

Then I may say _your_ implementation is flaky because it wastes one
bit in the address decoder and a lot of logic gates in the data
bus. It's normal to align registers at 32 bits, as it's simpler and
faster.  Most SoCs have only 32-bit aligned registers, for a reason.

> I am not a big fan of adding platform specific flakes in any core
> file, that why we keep the platform file separate from the core
> ones.

A number of other drivers have a shift parameter, because it's very
common for the hardware integrator to feel free to choose the easiest
wiring for the device.  The choice to keep the platform driver
separate from the core driver only adds complication in my opinion:
you need to export 4 symbols and yhen every user must duplicate code
(like federico is replicating theplatform driver in the pci driver).

I'd really prefer to have the core driver be a platform driver, and
the others just add platform data to describe how it is wired. That's
actually the reason why the platform bus exists.

> But I will left Marc and Wolfgang to further comment on the same.

I agree: let them decide.

/alessandro

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Marc Kleine-Budde]

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

On 06/05/2012 03:13 PM, Alessandro Rubini wrote:
>>> My implementation is align to 32, but I'm trying to make a generic PCI
>>> wrapper (some other could be aligned to 16)
>  
>> So it means your implementation is also flaky and you are probably
>> wasting HW memory space while integrating the Bosch C_CAN module in
>> your SoC :)
> 
> Then I may say _your_ implementation is flaky because it wastes one
> bit in the address decoder and a lot of logic gates in the data
> bus. It's normal to align registers at 32 bits, as it's simpler and
> faster.  Most SoCs have only 32-bit aligned registers, for a reason.
> 
>> I am not a big fan of adding platform specific flakes in any core
>> file, that why we keep the platform file separate from the core
>> ones.
> 
> A number of other drivers have a shift parameter, because it's very
> common for the hardware integrator to feel free to choose the easiest
> wiring for the device.  The choice to keep the platform driver
> separate from the core driver only adds complication in my opinion:
> you need to export 4 symbols and yhen every user must duplicate code
> (like federico is replicating theplatform driver in the pci driver).
> 
> I'd really prefer to have the core driver be a platform driver, and
> the others just add platform data to describe how it is wired. That's
> actually the reason why the platform bus exists.
> 
>> But I will left Marc and Wolfgang to further comment on the same.
> 
> I agree: let them decide.

I personally like the "pci device sets up a platform device" idea.

My question is, is this considered being a good practise?

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

RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Bhupesh SHARMA]

> -----Original Message-----
> From: rubini@gnudd.com [mailto:rubini@gnudd.com]
> Sent: Tuesday, June 05, 2012 6:44 PM
> To: Bhupesh SHARMA
> Cc: federico.vaga@gmail.com; alan@lxorguk.ukuu.org.uk;
> wg@grandegger.com; mkl@pengutronix.de; Giancarlo ASNAGHI;
> alan@linux.intel.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
> 
> >> My implementation is align to 32, but I'm trying to make a generic
> PCI
> >> wrapper (some other could be aligned to 16)
> 
> > So it means your implementation is also flaky and you are probably
> > wasting HW memory space while integrating the Bosch C_CAN module in
> > your SoC :)
> 
> Then I may say _your_ implementation is flaky because it wastes one
> bit in the address decoder and a lot of logic gates in the data
> bus. It's normal to align registers at 32 bits, as it's simpler and
> faster.  Most SoCs have only 32-bit aligned registers, for a reason.

You missed my original point. I mentioned in my first mail itself, that I studied a
few SoCs integrating the C_CAN module from Bosch before writing the driver.
Not all have aligned their register space to a 32-bit boundary. 
My platform driver still supports them. This _does_ not imply that our
SoC has/may have the same problem :)

Each SoC designer can have his/her own different view on this sort of implementation.
The platform driver was written to support both the implementations (SW is supposed
to support all sort of HW design constraints :) ).

> > I am not a big fan of adding platform specific flakes in any core
> > file, that why we keep the platform file separate from the core
> > ones.
> 
> A number of other drivers have a shift parameter, because it's very
> common for the hardware integrator to feel free to choose the easiest
> wiring for the device.  The choice to keep the platform driver
> separate from the core driver only adds complication in my opinion:
> you need to export 4 symbols and yhen every user must duplicate code
> (like federico is replicating theplatform driver in the pci driver).
> 
> I'd really prefer to have the core driver be a platform driver, and
> the others just add platform data to describe how it is wired. That's
> actually the reason why the platform bus exists.
> 
> > But I will left Marc and Wolfgang to further comment on the same.
> 
> I agree: let them decide.

Sure..

Regards,
Bhupesh

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Alessandro Rubini]

> I personally like the "pci device sets up a platform device" idea.

Good. Than me or federico will submit a proposal. 
 
> My question is, is this considered being a good practise?

I don't think there are many pci bridges around, but platform drivers
exists just for that reason: to be instantiated when you know how the
wiring ("platform") details.  I.e., somebody registers the platform
device associated to the driver.

Sometimes the platform device is compiled in, sometimes it comes from
the device tree. I think it can come from PCI as well.

thanks
/alessandro, apologizing with Bhupesh Sharma for his tone in the previous mail

RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[AnilKumar, Chimata]

Hi Alessandro/Federico,

On Tue, Jun 05, 2012 at 19:00:13, Alessandro Rubini wrote:
> > I personally like the "pci device sets up a platform device" idea.
> 
> Good. Than me or federico will submit a proposal. 
>  

I am late to the discussion, is there any specific reason to maintain a
separate platform file (c_can_pci.c). I think 90% of the code is copied
from c_can_paltform.c, code changes will be less if you merge to existing
c_can platform driver. You can look at D_CAN integration to C_CAN driver.

[1] https://gitorious.org/linux-can/linux-can-next

Regards
AnilKumar

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Alessandro Rubini]

> I am late to the discussion, is there any specific reason to maintain a
> separate platform file (c_can_pci.c).

Because it depends on pci and ifdef is bad.

> I think 90% of the code is copied from c_can_paltform.c, code
> changes will be less if you merge to existing c_can platform driver.

Yes, but then we need to ifdef around, which merges two bad files
into a single but worse file.

But since the only current user of c_can is the platform device, why
not merging the platform with the core and having pci just register a
platform device?  The only problem I see is that we need cooperation,
because neither me nor federico have a c_can equipped board besides
the pci one.

thanks
/alessandro

RE: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Bhupesh SHARMA]

Hi,

> -----Original Message-----
> From: rubini@gnudd.com [mailto:rubini@gnudd.com]
> Sent: Tuesday, June 05, 2012 10:20 PM
> To: anilkumar@ti.com
> Cc: mkl@pengutronix.de; Bhupesh SHARMA; federico.vaga@gmail.com;
> alan@lxorguk.ukuu.org.uk; wg@grandegger.com; Giancarlo ASNAGHI;
> alan@linux.intel.com; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI
> 
> > I am late to the discussion, is there any specific reason to maintain
> a
> > separate platform file (c_can_pci.c).
> 
> Because it depends on pci and ifdef is bad.
> 
> > I think 90% of the code is copied from c_can_paltform.c, code
> > changes will be less if you merge to existing c_can platform driver.
> 
> Yes, but then we need to ifdef around, which merges two bad files
> into a single but worse file.
> 
> But since the only current user of c_can is the platform device, why
> not merging the platform with the core and having pci just register a
> platform device?  The only problem I see is that we need cooperation,
> because neither me nor federico have a c_can equipped board besides
> the pci one.
> 

I can see examples of where different platform files are present for SJA CAN controller
as well depending on the underlying bus being used: OpenFirmware, ISA, PCI, etc..,
whilst there is a single core file there as well 'sja1000.c'

[1] Kvaser PCI platform driver, using services exposed by sja1000 core:
	http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/kvaser_pci.c

[2] EMS PCI platform driver, using services exposed by sja1000 core:
	http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/ems_pci.c

[3] SJA1000 core:
	http://lxr.linux.no/linux+v3.4.1/drivers/net/can/sja1000/sja1000.c

Here each platform driver has its own version of register read/write routine implementation.
The C_CAN approach is similar to that used by SJA1000. Instead of merging the "platform with the core",
I would instead suggest to have two separate platform drivers (for each bus type) and invoke common
routines kept in say another file 'c_can_platform_common.c', thus insuring that there is no code
duplicity and we have a clean hierarchical structure as well. So we can have:
	- Core file, c_can.c
	- Common platform file, c_can_platform_common.c
	- Platform file, c_can_platform.c, c_can_pci.c, etc..

This ensures that nothing breaks at the end of the existing C_CAN users and we have a clean
file structure as well.

Ofcourse, Wolfgang has a better idea of this structure, as he defined the same for SJA1000 and I 
consulted with him on this, while he was reviewing my initial C_CAN patch set. I will let him and Marc
comment further on my proposal. Your comments are also most welcome :)

Regards,
Bhupesh

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Federico Vaga]

How we proceed?
I submit my c_can_pci.c as a separated module, we create a
c_can_platform_common.c,
or we are thinking about a generic c_can.c as plaftorm driver?

-- 
Federico Vaga

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Wolfgang Grandegger]

On 06/04/2012 06:45 PM, Alessandro Rubini wrote:
>> Anythign wrong with 
>>
>> bool aligned32;
> 
> I personally think booleans are evil.  But both this and the other
> thing:
> 
>>> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
>>> +						void *reg)
>>
>> I'm a bit worried this function name might be too short ;)
> 
> come from the platform driver this is based on (I already blamed
> federico offlist for not preserving authorship of the original file).
> 
> So, this file is mostly copied from the platform driver, which is a
> duplication of code.  A mandated duplication, given how the thing
> is currently laid out: the c_can core driver exports functions that
> the other two files are using (the platform and the new pci driver).
> 
> In my opinion, it would be much better to have one less layer and no
> exports at all. The core driver should be a platform driver, and the
> pci driver would just build platform data and register the platform
> device.

Do you have examples for that approach? Not sure yet if it really saves
code and makes it more readable.

> Sure this isn't up to federico, who has the pci device but cannot
> access any boards where the previous driver is used.  What do the
> maintainers think? I (or federico :) may propose a reshaping, if
> the idea makes sense.

I would suggest to provide the c_can_pci driver using the *current* API,
even if it's not optimal. Federicos patch then already looks quite good.
It should use the new register access methods introduced by the D_CAN
support patch, though.

Any further improvements to the device abstraction and a more consistent
handling of the platform data are welcome.

Wolfgang.

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Wolfgang Grandegger]

Hi Federico,

here comes my late review. Mark and others have already commented and I
will focus on further improvements...

On 06/04/2012 03:32 PM, Federico Vaga wrote:

A few more words would be nice here.

> Signed-off-by: Federico Vaga <federico.vaga@gmail.com>
> Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
> Cc: Alan Cox <alan@linux.intel.com>
> ---
>  drivers/net/can/c_can/Kconfig     |   11 +-
>  drivers/net/can/c_can/Makefile    |    1 +
>  drivers/net/can/c_can/c_can_pci.c |  221 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/can/c_can/c_can_pci.c
> 
> diff --git a/drivers/net/can/c_can/Kconfig b/drivers/net/can/c_can/Kconfig
> index ffb9773..74ef97d 100644
> --- a/drivers/net/can/c_can/Kconfig
> +++ b/drivers/net/can/c_can/Kconfig
> @@ -2,14 +2,19 @@ menuconfig CAN_C_CAN
>  	tristate "Bosch C_CAN devices"
>  	depends on CAN_DEV && HAS_IOMEM
>  
> -if CAN_C_CAN
> -

Please don't change unrelated things. It's done that way also in other
CAN subdirectories.

>  config CAN_C_CAN_PLATFORM
>  	tristate "Generic Platform Bus based C_CAN driver"
> +	depends on CAN_C_CAN
>  	---help---
>  	  This driver adds support for the C_CAN chips connected to
>  	  the "platform bus" (Linux abstraction for directly to the
>  	  processor attached devices) which can be found on various
>  	  boards from ST Microelectronics (http://www.st.com)
>  	  like the SPEAr1310 and SPEAr320 evaluation boards.
> -endif
> +
> +config CAN_C_CAN_PCI
> +	tristate "Generic PCI Bus based C_CAN driver"
> +	depends on CAN_C_CAN
> +	---help---
> +	  This driver adds support for the C_CAN chips connected to
> +	  the PCI bus.
> diff --git a/drivers/net/can/c_can/Makefile b/drivers/net/can/c_can/Makefile
> index 9273f6d..ad1cc84 100644
> --- a/drivers/net/can/c_can/Makefile
> +++ b/drivers/net/can/c_can/Makefile
> @@ -4,5 +4,6 @@
>  
>  obj-$(CONFIG_CAN_C_CAN) += c_can.o
>  obj-$(CONFIG_CAN_C_CAN_PLATFORM) += c_can_platform.o
> +obj-$(CONFIG_CAN_C_CAN_PCI) += c_can_pci.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> new file mode 100644
> index 0000000..b635375
> --- /dev/null
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -0,0 +1,221 @@
> +/*
> + * Platform CAN bus driver for Bosch C_CAN controller

s /Platform/PCI/ ?

> + *
> + * Copyright (C) 2012 Federico Vaga <federico.vaga@gmail.com>
> +  *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/clk.h>
> +#include <linux/pci.h>
> +#include <linux/can/dev.h>
> +
> +#include "c_can.h"
> +
> +enum c_can_pci_reg_align {
> +	C_CAN_REG_ALIGN_16,
> +	C_CAN_REG_ALIGN_32,
> +};
> +
> +struct c_can_pci_data {
> +	unsigned int reg_align;	/* Set the register alignment in the memory */

Should be "enum c_can_pci_reg_align" here.

> +	unsigned int freq;	/* Set the frequency if clk is not usable */
> +};
> +
> +/*
> + * 16-bit c_can registers can be arranged differently in the memory
> + * architecture of different implementations. For example: 16-bit
> + * registers can be aligned to a 16-bit boundary or 32-bit boundary etc.
> + * Handle the same by providing a common read/write interface.
> + */
> +static u16 c_can_pci_read_reg_aligned_to_16bit(struct c_can_priv *priv,
> +						void *reg)
> +{
> +	return readw(reg);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_16bit(struct c_can_priv *priv,
> +						void *reg, u16 val)
> +{
> +	writew(val, reg);
> +}
> +
> +static u16 c_can_pci_read_reg_aligned_to_32bit(struct c_can_priv *priv,
> +						void *reg)
> +{
> +	return readw(reg + (long)reg - (long)priv->regs);
> +}
> +
> +static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
> +						void *reg, u16 val)
> +{
> +	writew(val, reg + (long)reg - (long)priv->regs);
> +}

This will look better with the new register access methods.

> +static int __devinit c_can_pci_probe(struct pci_dev *pdev,
> +				     const struct pci_device_id *ent)
> +{
> +	struct c_can_pci_data *c_can_pci_data = (void *)ent->driver_data;
> +	struct c_can_priv *priv;
> +	struct net_device *dev;
> +	void __iomem *addr;
> +	struct clk *clk;
> +	int ret;
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "pci_enable_device FAILED\n");
> +		goto out;
> +	}
> +
> +	ret = pci_request_regions(pdev, KBUILD_MODNAME);
> +	if (ret) {
> +		dev_err(&pdev->dev, "pci_request_regions FAILED\n");
> +		goto out_disable_device;
> +	}
> +
> +	pci_set_master(pdev);
> +	pci_enable_msi(pdev);
> +
> +	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> +	if (!addr) {
> +		dev_err(&pdev->dev,
> +			"device has no PCI memory resources, "
> +			"failing adapter\n");
> +		ret = -ENOMEM;
> +		goto out_release_regions;
> +	}
> +
> +	/* allocate the c_can device */
> +	dev = alloc_c_can_dev();
> +	if (!dev) {
> +		ret = -ENOMEM;
> +		goto out_iounmap;
> +	}
> +
> +	priv = netdev_priv(dev);
> +	pci_set_drvdata(pdev, dev);
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	dev->irq = pdev->irq;
> +	priv->regs = addr;
> +
> +	if (!c_can_pci_data->freq) {
> +		/* get the appropriate clk */
> +		clk = clk_get(&pdev->dev, NULL);
> +		if (IS_ERR(clk)) {
> +			dev_err(&pdev->dev, "no clock defined\n");
> +			ret = -ENODEV;
> +			goto out_free_c_can;
> +		}
> +		priv->can.clock.freq = clk_get_rate(clk);
> +		priv->priv = clk;
> +	} else {
> +		priv->can.clock.freq = c_can_pci_data->freq;
> +		priv->priv = NULL;
> +	}
> +
> +	switch (c_can_pci_data->reg_align) {
> +	case C_CAN_REG_ALIGN_32:
> +		priv->read_reg = c_can_pci_read_reg_aligned_to_32bit;
> +		priv->write_reg = c_can_pci_write_reg_aligned_to_32bit;
> +		break;
> +	case C_CAN_REG_ALIGN_16:
> +	default:
> +		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
> +		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
> +		break;
> +	}
> +
> +	ret = register_c_can_dev(dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> +			KBUILD_MODNAME, ret);
> +		goto out_free_clock;
> +	}
> +
> +	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> +		 KBUILD_MODNAME, priv->regs, dev->irq);
> +
> +	return 0;
> +
> +out_free_clock:
> +	if (!priv->priv)
> +		clk_put(priv->priv);
> +out_free_c_can:
> +	pci_set_drvdata(pdev, NULL);
> +	free_c_can_dev(dev);
> +out_iounmap:
> +	pci_iounmap(pdev, addr);
> +out_release_regions:
> +	pci_disable_msi(pdev);
> +	pci_clear_master(pdev);
> +	pci_release_regions(pdev);
> +out_disable_device:
> +	/*
> +	 * do not call pci_disable_device on sta2x11 because it
> +	 * break all other Bus masters on this EP
> +	 */

Puh!

> +	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> +	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> +		goto out;
> +	pci_disable_device(pdev);
> +out:
> +	return ret;
> +}
> +
> +static void __devexit c_can_pci_remove(struct pci_dev *pdev)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +	struct c_can_priv *priv = netdev_priv(dev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +	free_c_can_dev(dev);

Should be moved a few line down.

> +	if (!priv->priv)
> +		clk_put(priv->priv);
> +	pci_iounmap(pdev, priv->regs);
> +	pci_disable_msi(pdev);
> +	pci_clear_master(pdev);
> +	pci_release_regions(pdev);
> +	/*
> +	 * do not call pci_disable_device on sta2x11 because it
> +	 * break all other Bus masters on this EP
> +	 */
> +	if(pdev->vendor == PCI_VENDOR_ID_STMICRO &&
> +	   pdev->device == PCI_DEVICE_ID_STMICRO_CAN)
> +		return;
> +	pci_disable_device(pdev);
> +}
> +
> +static struct c_can_pci_data c_can_sta2x11= {
> +	.reg_align = C_CAN_REG_ALIGN_32,
> +	.freq = 52000000, /* 52 Mhz */
> +};
> +
> +#define C_CAN_ID(_vend, _dev, _driverdata) {		\
> +	PCI_DEVICE(_vend, _dev),			\
> +	.driver_data = (unsigned long)&_driverdata,	\
> +}
> +DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
> +	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
> +		 c_can_sta2x11),
> +	{},
> +};
> +static struct pci_driver sta2x11_pci_driver = {

Why do you not use a generic name here?

> +	.name = KBUILD_MODNAME,
> +	.id_table = c_can_pci_tbl,
> +	.probe = c_can_pci_probe,
> +	.remove = __devexit_p(c_can_pci_remove),
> +};
> +
> +module_pci_driver(sta2x11_pci_driver);
> +
> +MODULE_AUTHOR("Federico Vaga <federico.vaga@gmail.com>");
> +MODULE_LICENSE("GPL V2");
> +MODULE_DESCRIPTION("PCI CAN bus driver for Bosch C_CAN controller");
> +MODULE_DEVICE_TABLE(pci, c_can_pci_tbl);

Thanks for your contribution.

Wolfgang.

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Wolfgang Grandegger]

On 06/11/2012 03:18 PM, Federico Vaga wrote:
> How we proceed?
> I submit my c_can_pci.c as a separated module, we create a
> c_can_platform_common.c,
> or we are thinking about a generic c_can.c as plaftorm driver?

I would accept your patch with the remaining fixes especially the new
register access methods introduced by the D_CAN support patch recently.

Any further improvements to the device abstraction and a more consistent
handling of the platform data or register access should be addressed by
sub-sequent patches.

Wolfgang.

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Alessandro Rubini]

>> In my opinion, it would be much better to have one less layer and no
>> exports at all. The core driver should be a platform driver, and the
>> pci driver would just build platform data and register the platform
>> device.
> 
> Do you have examples for that approach? Not sure yet if it really saves
> code and makes it more readable.

Maybe the physmap mtd driver is a good example. Everybody's using it
(but not from PCI). I found drivers/pcmcia/bcm63xx_pcmcia.c that
registers a platform driver from a pci probe function, but I'm sure
there are other ones.

OTOH, I have another example of how not to do stuff, but I won't point
fingers now (it's not a CAN thing).

I just think the platform bus is there just for this reason: to provide
data to a generic driver, without module dependencies and such stuff. 

> I would suggest to provide the c_can_pci driver using the *current* API,
> even if it's not optimal. Federicos patch then already looks quite good.
> It should use the new register access methods introduced by the D_CAN
> support patch, though.

Great. When it's in I'll show my proposal as an RFC patch, as time permits,
so we'll see if it's better or not.

> Any further improvements to the device abstraction and a more consistent
> handling of the platform data are welcome.

Good to know, thanks a lot

/alessandro

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Federico Vaga]

> > +out_free_clock:
> > +	if (!priv->priv)
> 
>            ^^^
> 
> looks fishy

Also c_can_platform.c use priv->priv when it needs to get clk. I can add 
a comment to specify what the statement do.

-- 
Federico Vaga

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Marc Kleine-Budde]

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

On 06/12/2012 04:25 PM, Federico Vaga wrote:
>>> +out_free_clock:
>>> +	if (!priv->priv)
>>
>>            ^^^
>>
>> looks fishy
> 
> Also c_can_platform.c use priv->priv when it needs to get clk. I can add 
> a comment to specify what the statement do.

> +out_free_clock:
> +	if (!priv->priv)
> +		clk_put(priv->priv);

Why do you call clk_put on priv->priv, if priv->priv is NULL?

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

Re: [PATCH RFC] c_can_pci: generic module for c_can on PCI

[Federico Vaga]

> > +out_free_clock:
> > +	if (!priv->priv)
> > +		clk_put(priv->priv);
> 
> Why do you call clk_put on priv->priv, if priv->priv is NULL?

Right!

if(priv->priv)
    clk_put(priv->priv);

-- 
Federico Vaga