[RFC 0/4] [flexcan/powerpc] Add support for powerpc flexcan (freescale p1010) -V7

[RFC 0/4] [flexcan/powerpc] Add support for powerpc flexcan (freescale p1010) -V7

[Robin Holt]

Marc, Wolfgang or U Bhaskar,

This patch set should have all your comments included.  It is based on
the David S. Miller net-next-2.6 tree commit 19fd617.

I have compiled each patch in the series individually for both arm and
powerpc (cheated on ppc and reordered them with the last patch first so
I could select CAN_FLEXCAN.

With all the patches applied, my p1010rdb works for communicating between
its two can ports and also can communicate with an external PSOC.  I have
done no testing beyond compile testing on an arm system as I have no
access to an arm based system.

For the first three patches in the series, I believe they are all ready
for submission to David S. Miller for the netdev tree.  This is the
first submission of patch 4 which, I believe, will end up going through
the linuxppc-dev list and can go at any time after the flexcan patches
are submitted.  As long the first and last patches are both applied,
the tree will compile for either ppc or arm, but will not work without
all patches.  If the ppc patch (4/4) gets applied before 1/4, we could
have a random config tester detect a compile failure on ppc if they
select NET, CAN, and CAN_FLEXCAN.

Thanks,
Robin Holt

[RFC 1/4] [flexcan] Remove #include <mach/clock.h>

[Robin Holt]

powerpc does not have a mach-####/clock.h.  When testing, I found neither
arm nor powerpc needed the mach/clock.h at all so I removed it.

Signed-off-by: Robin Holt <holt@sgi.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
To: Wolfgang Grandegger <wg@grandegger.com>
To: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de
Cc: netdev@vger.kernel.org
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
---
 drivers/net/can/flexcan.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 1767811..586b2cd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -35,8 +35,6 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-#include <mach/clock.h>
-
 #define DRV_NAME			"flexcan"
 
 /* 8 for RX fifo and 2 error handling */
-- 
1.7.2.1

[RFC 2/4] [flexcan] Abstract off read/write for big/little endian.

[Robin Holt]

Make flexcan driver handle register reads in the appropriate endianess.
This was a basic search and replace and then define some inlines.

Signed-off-by: Robin Holt <holt@sgi.com>
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
To: Wolfgang Grandegger <wg@grandegger.com>
To: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de
Cc: netdev@vger.kernel.org
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
---
 drivers/net/can/flexcan.c |  140 ++++++++++++++++++++++++++------------------
 1 files changed, 83 insertions(+), 57 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 586b2cd..68cbe52 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -190,6 +190,31 @@ static struct can_bittiming_const flexcan_bittiming_const = {
 };
 
 /*
+ * Abstract off the read/write for arm versus ppc.
+ */
+#if defined(__BIG_ENDIAN)
+static inline u32 flexcan_read(void __iomem *addr)
+{
+	return in_be32(addr);
+}
+
+static inline void flexcan_write(u32 val, void __iomem *addr)
+{
+	out_be32(addr, val);
+}
+#else
+static inline u32 flexcan_read(void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static inline void flexcan_write(u32 val, void __iomem *addr)
+{
+	writel(val, addr);
+}
+#endif
+
+/*
  * Swtich transceiver on or off
  */
 static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on)
@@ -210,9 +235,9 @@ static inline void flexcan_chip_enable(struct flexcan_priv *priv)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg;
 
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg &= ~FLEXCAN_MCR_MDIS;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 
 	udelay(10);
 }
@@ -222,9 +247,9 @@ static inline void flexcan_chip_disable(struct flexcan_priv *priv)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg;
 
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg |= FLEXCAN_MCR_MDIS;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 }
 
 static int flexcan_get_berr_counter(const struct net_device *dev,
@@ -232,7 +257,7 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
 {
 	const struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg = readl(&regs->ecr);
+	u32 reg = flexcan_read(&regs->ecr);
 
 	bec->txerr = (reg >> 0) & 0xff;
 	bec->rxerr = (reg >> 8) & 0xff;
@@ -266,15 +291,15 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (cf->can_dlc > 0) {
 		u32 data = be32_to_cpup((__be32 *)&cf->data[0]);
-		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
+		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
 	}
 	if (cf->can_dlc > 3) {
 		u32 data = be32_to_cpup((__be32 *)&cf->data[4]);
-		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
+		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
 	}
 
-	writel(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
-	writel(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
+	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
 
 	kfree_skb(skb);
 
@@ -462,8 +487,8 @@ static void flexcan_read_fifo(const struct net_device *dev,
 	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
 	u32 reg_ctrl, reg_id;
 
-	reg_ctrl = readl(&mb->can_ctrl);
-	reg_id = readl(&mb->can_id);
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	reg_id = flexcan_read(&mb->can_id);
 	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
 		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
@@ -473,12 +498,12 @@ static void flexcan_read_fifo(const struct net_device *dev,
 		cf->can_id |= CAN_RTR_FLAG;
 	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
 
-	*(__be32 *)(cf->data + 0) = cpu_to_be32(readl(&mb->data[0]));
-	*(__be32 *)(cf->data + 4) = cpu_to_be32(readl(&mb->data[1]));
+	*(__be32 *)(cf->data + 0) = cpu_to_be32(flexcan_read(&mb->data[0]));
+	*(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
 
 	/* mark as read */
-	writel(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
-	readl(&regs->timer);
+	flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
+	flexcan_read(&regs->timer);
 }
 
 static int flexcan_read_frame(struct net_device *dev)
@@ -514,17 +539,17 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	 * The error bits are cleared on read,
 	 * use saved value from irq handler.
 	 */
-	reg_esr = readl(&regs->esr) | priv->reg_esr;
+	reg_esr = flexcan_read(&regs->esr) | priv->reg_esr;
 
 	/* handle state changes */
 	work_done += flexcan_poll_state(dev, reg_esr);
 
 	/* handle RX-FIFO */
-	reg_iflag1 = readl(&regs->iflag1);
+	reg_iflag1 = flexcan_read(&regs->iflag1);
 	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
 	       work_done < quota) {
 		work_done += flexcan_read_frame(dev);
-		reg_iflag1 = readl(&regs->iflag1);
+		reg_iflag1 = flexcan_read(&regs->iflag1);
 	}
 
 	/* report bus errors */
@@ -534,8 +559,8 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	if (work_done < quota) {
 		napi_complete(napi);
 		/* enable IRQs */
-		writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
-		writel(priv->reg_ctrl_default, &regs->ctrl);
+		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
 	}
 
 	return work_done;
@@ -549,9 +574,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg_iflag1, reg_esr;
 
-	reg_iflag1 = readl(&regs->iflag1);
-	reg_esr = readl(&regs->esr);
-	writel(FLEXCAN_ESR_ERR_INT, &regs->esr);	/* ACK err IRQ */
+	reg_iflag1 = flexcan_read(&regs->iflag1);
+	reg_esr = flexcan_read(&regs->esr);
+	flexcan_write(FLEXCAN_ESR_ERR_INT, &regs->esr);	/* ACK err IRQ */
 
 	/*
 	 * schedule NAPI in case of:
@@ -567,16 +592,16 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		 * save them for later use.
 		 */
 		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
-		writel(FLEXCAN_IFLAG_DEFAULT & ~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE,
-		       &regs->imask1);
-		writel(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
+		flexcan_write(FLEXCAN_IFLAG_DEFAULT &
+			~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->imask1);
+		flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
 		       &regs->ctrl);
 		napi_schedule(&priv->napi);
 	}
 
 	/* FIFO overflow */
 	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
-		writel(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
+		flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
 		dev->stats.rx_over_errors++;
 		dev->stats.rx_errors++;
 	}
@@ -585,7 +610,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
 		/* tx_bytes is incremented in flexcan_start_xmit */
 		stats->tx_packets++;
-		writel((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
+		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
 		netif_wake_queue(dev);
 	}
 
@@ -599,7 +624,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg;
 
-	reg = readl(&regs->ctrl);
+	reg = flexcan_read(&regs->ctrl);
 	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
 		 FLEXCAN_CTRL_RJW(0x3) |
 		 FLEXCAN_CTRL_PSEG1(0x7) |
@@ -623,11 +648,11 @@ static void flexcan_set_bittiming(struct net_device *dev)
 		reg |= FLEXCAN_CTRL_SMP;
 
 	dev_info(dev->dev.parent, "writing ctrl=0x%08x\n", reg);
-	writel(reg, &regs->ctrl);
+	flexcan_write(reg, &regs->ctrl);
 
 	/* print chip status */
 	dev_dbg(dev->dev.parent, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
-		readl(&regs->mcr), readl(&regs->ctrl));
+		flexcan_read(&regs->mcr), flexcan_read(&regs->ctrl));
 }
 
 /*
@@ -648,10 +673,10 @@ static int flexcan_chip_start(struct net_device *dev)
 	flexcan_chip_enable(priv);
 
 	/* soft reset */
-	writel(FLEXCAN_MCR_SOFTRST, &regs->mcr);
+	flexcan_write(FLEXCAN_MCR_SOFTRST, &regs->mcr);
 	udelay(10);
 
-	reg_mcr = readl(&regs->mcr);
+	reg_mcr = flexcan_read(&regs->mcr);
 	if (reg_mcr & FLEXCAN_MCR_SOFTRST) {
 		dev_err(dev->dev.parent,
 			"Failed to softreset can module (mcr=0x%08x)\n",
@@ -673,12 +698,12 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * choose format C
 	 *
 	 */
-	reg_mcr = readl(&regs->mcr);
+	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
 		FLEXCAN_MCR_IDAM_C;
 	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
-	writel(reg_mcr, &regs->mcr);
+	flexcan_write(reg_mcr, &regs->mcr);
 
 	/*
 	 * CTRL
@@ -696,7 +721,7 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * (FLEXCAN_CTRL_ERR_MSK), too. Otherwise we don't get any
 	 * warning or bus passive interrupts.
 	 */
-	reg_ctrl = readl(&regs->ctrl);
+	reg_ctrl = flexcan_read(&regs->ctrl);
 	reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
 	reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
 		FLEXCAN_CTRL_ERR_STATE | FLEXCAN_CTRL_ERR_MSK;
@@ -704,38 +729,39 @@ static int flexcan_chip_start(struct net_device *dev)
 	/* save for later use */
 	priv->reg_ctrl_default = reg_ctrl;
 	dev_dbg(dev->dev.parent, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
-	writel(reg_ctrl, &regs->ctrl);
+	flexcan_write(reg_ctrl, &regs->ctrl);
 
 	for (i = 0; i < ARRAY_SIZE(regs->cantxfg); i++) {
-		writel(0, &regs->cantxfg[i].can_ctrl);
-		writel(0, &regs->cantxfg[i].can_id);
-		writel(0, &regs->cantxfg[i].data[0]);
-		writel(0, &regs->cantxfg[i].data[1]);
+		flexcan_write(0, &regs->cantxfg[i].can_ctrl);
+		flexcan_write(0, &regs->cantxfg[i].can_id);
+		flexcan_write(0, &regs->cantxfg[i].data[0]);
+		flexcan_write(0, &regs->cantxfg[i].data[1]);
 
 		/* put MB into rx queue */
-		writel(FLEXCAN_MB_CNT_CODE(0x4), &regs->cantxfg[i].can_ctrl);
+		flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
+			&regs->cantxfg[i].can_ctrl);
 	}
 
 	/* acceptance mask/acceptance code (accept everything) */
-	writel(0x0, &regs->rxgmask);
-	writel(0x0, &regs->rx14mask);
-	writel(0x0, &regs->rx15mask);
+	flexcan_write(0x0, &regs->rxgmask);
+	flexcan_write(0x0, &regs->rx14mask);
+	flexcan_write(0x0, &regs->rx15mask);
 
 	flexcan_transceiver_switch(priv, 1);
 
 	/* synchronize with the can bus */
-	reg_mcr = readl(&regs->mcr);
+	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_HALT;
-	writel(reg_mcr, &regs->mcr);
+	flexcan_write(reg_mcr, &regs->mcr);
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	/* enable FIFO interrupts */
-	writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+	flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
 
 	/* print chip status */
 	dev_dbg(dev->dev.parent, "%s: reading mcr=0x%08x ctrl=0x%08x\n",
-		__func__, readl(&regs->mcr), readl(&regs->ctrl));
+		__func__, flexcan_read(&regs->mcr), flexcan_read(&regs->ctrl));
 
 	return 0;
 
@@ -757,12 +783,12 @@ static void flexcan_chip_stop(struct net_device *dev)
 	u32 reg;
 
 	/* Disable all interrupts */
-	writel(0, &regs->imask1);
+	flexcan_write(0, &regs->imask1);
 
 	/* Disable + halt module */
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 
 	flexcan_transceiver_switch(priv, 0);
 	priv->can.state = CAN_STATE_STOPPED;
@@ -854,24 +880,24 @@ static int __devinit register_flexcandev(struct net_device *dev)
 
 	/* select "bus clock", chip must be disabled */
 	flexcan_chip_disable(priv);
-	reg = readl(&regs->ctrl);
+	reg = flexcan_read(&regs->ctrl);
 	reg |= FLEXCAN_CTRL_CLK_SRC;
-	writel(reg, &regs->ctrl);
+	flexcan_write(reg, &regs->ctrl);
 
 	flexcan_chip_enable(priv);
 
 	/* set freeze, halt and activate FIFO, restrict register access */
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 
 	/*
 	 * Currently we only support newer versions of this core
 	 * featuring a RX FIFO. Older cores found on some Coldfire
 	 * derivates are not yet supported.
 	 */
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	if (!(reg & FLEXCAN_MCR_FEN)) {
 		dev_err(dev->dev.parent,
 			"Could not enable RX FIFO, unsupported core\n");
-- 
1.7.2.1

[RFC 3/4] [flexcan] Add of_match to platform_device definition.

[Robin Holt]

On powerpc, the OpenFirmware devices are not matched without specifying
an of_match array.  Introduce that array as that is used for matching
on the Freescale P1010 processor.

Signed-off-by: Robin Holt <holt@sgi.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
To: Wolfgang Grandegger <wg@grandegger.com>
To: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de
Cc: netdev@vger.kernel.org
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
---
 drivers/net/can/flexcan.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 68cbe52..662f832 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1027,8 +1027,19 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id flexcan_of_match[] = {
+	{
+		.compatible = "fsl,flexcan",
+	},
+	{},
+};
+
 static struct platform_driver flexcan_driver = {
-	.driver.name = DRV_NAME,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = flexcan_of_match,
+	},
 	.probe = flexcan_probe,
 	.remove = __devexit_p(flexcan_remove),
 };
-- 
1.7.2.1

[RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Robin Holt]

I added a clock source for the p1010rdb so the flexcan driver
could find its clock frequency.

Signed-off-by: Robin Holt <holt@sgi.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>,
To: Wolfgang Grandegger <wg@grandegger.com>,
To: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de,
Cc: netdev@vger.kernel.org,
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>

---

Could I also get a ruling on the Kconfig language.  I could do it either
with the 85xx_HAVE_CAN_FLEXCAN in or out of the Kconfig file.  It felt
like the right thing to do was without, but the arm Kconfig files do
it this way and a patch from freescale had something similar so I went
this route.

 arch/powerpc/platforms/85xx/Kconfig    |    6 +++
 arch/powerpc/platforms/85xx/Makefile   |    1 +
 arch/powerpc/platforms/85xx/clock.c    |   59 ++++++++++++++++++++++++++++++++
 arch/powerpc/platforms/85xx/p1010rdb.c |    8 ++++
 4 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/platforms/85xx/clock.c

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 498534c..ed4cf92 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -26,6 +26,10 @@ config MPC8560_ADS
 	help
 	  This option enables support for the MPC 8560 ADS board
 
+config 85xx_HAVE_CAN_FLEXCAN
+	bool
+	select HAVE_CAN_FLEXCAN if NET && CAN
+
 config MPC85xx_CDS
 	bool "Freescale MPC85xx CDS"
 	select DEFAULT_UIMAGE
@@ -70,6 +74,8 @@ config MPC85xx_RDB
 config P1010_RDB
 	bool "Freescale P1010RDB"
 	select DEFAULT_UIMAGE
+	select 85xx_HAVE_CAN_FLEXCAN
+	select PPC_CLOCK if CAN_FLEXCAN
 	help
 	  This option enables support for the MPC85xx RDB (P1010 RDB) board
 
diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index a971b32..64ad7a4 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS)  += mpc85xx_ds.o
 obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
 obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
 obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
+obj-$(CONFIG_PPC_CLOCK)   += clock.o
 obj-$(CONFIG_P1022_DS)    += p1022_ds.o
 obj-$(CONFIG_P1023_RDS)   += p1023_rds.o
 obj-$(CONFIG_P2040_RDB)   += p2040_rdb.o corenet_ds.o
diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
new file mode 100644
index 0000000..a25cbf3
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/clock.c
@@ -0,0 +1,59 @@
+
+#include <linux/device.h>
+#include <linux/err.h>
+
+#include <asm/clk_interface.h>
+
+#include <sysdev/fsl_soc.h>
+
+/*
+ * p1010rdb needs to provide a clock source for the flexcan driver.
+ */
+struct clk {
+	unsigned long rate;
+} p1010_rdb_system_clock;
+
+static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
+{
+	const char *dev_init_name;
+
+	if (!dev)
+		return ERR_PTR(-ENOENT);
+
+	/*
+	 * The can devices are named ffe1c000.can0 and ffe1d000.can1 on
+	 * the p1010rdb.  Check for the "can" portion of that name before
+	 * returning a clock source.
+	 */
+	dev_init_name = dev_name(dev);
+	if (strlen(dev_init_name) != 13)
+		return ERR_PTR(-ENOENT);
+	dev_init_name += 9;
+	if (strncmp(dev_init_name, "can", 3))
+		return ERR_PTR(-ENOENT);
+
+	return &p1010_rdb_system_clock;
+}
+
+static void p1010_rdb_clk_put(struct clk *clk)
+{
+	return;
+}
+
+static unsigned long p1010_rdb_clk_get_rate(struct clk *clk)
+{
+	return clk->rate;
+}
+
+static struct clk_interface p1010_rdb_clk_functions = {
+	.clk_get		= p1010_rdb_clk_get,
+	.clk_get_rate		= p1010_rdb_clk_get_rate,
+	.clk_put		= p1010_rdb_clk_put,
+};
+
+void __init p1010_rdb_clk_init(void)
+{
+	p1010_rdb_system_clock.rate = fsl_get_sys_freq();
+	clk_functions = p1010_rdb_clk_functions;
+}
+
diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
index d7387fa..29e04d6 100644
--- a/arch/powerpc/platforms/85xx/p1010rdb.c
+++ b/arch/powerpc/platforms/85xx/p1010rdb.c
@@ -81,6 +81,13 @@ static void __init p1010_rdb_setup_arch(void)
 	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
 }
 
+static void __init p1010_rdb_init(void)
+{
+#ifdef PPC_CLOCK
+	p1010_rdb_clk_init();
+#endif
+}
+
 static struct of_device_id __initdata p1010rdb_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
@@ -111,6 +118,7 @@ define_machine(p1010_rdb) {
 	.name			= "P1010 RDB",
 	.probe			= p1010_rdb_probe,
 	.setup_arch		= p1010_rdb_setup_arch,
+	.init			= p1010_rdb_init,
 	.init_IRQ		= p1010_rdb_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
-- 
1.7.2.1

Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Robin Holt]

Argh.  I sent an earlier (non-working) version of this patch.  Here is
the correct one.

I added a clock source for the p1010rdb so the flexcan driver
could find its clock frequency.

Signed-off-by: Robin Holt <holt@sgi.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>,
To: Wolfgang Grandegger <wg@grandegger.com>,
To: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de,
Cc: netdev@vger.kernel.org,
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
---
 arch/powerpc/platforms/85xx/Kconfig    |    6 +++
 arch/powerpc/platforms/85xx/Makefile   |    1 +
 arch/powerpc/platforms/85xx/clock.c    |   59 ++++++++++++++++++++++++++++++++
 arch/powerpc/platforms/85xx/p1010rdb.c |   10 +++++
 4 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/platforms/85xx/clock.c

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 498534c..ed4cf92 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -26,6 +26,10 @@ config MPC8560_ADS
 	help
 	  This option enables support for the MPC 8560 ADS board
 
+config 85xx_HAVE_CAN_FLEXCAN
+	bool
+	select HAVE_CAN_FLEXCAN if NET && CAN
+
 config MPC85xx_CDS
 	bool "Freescale MPC85xx CDS"
 	select DEFAULT_UIMAGE
@@ -70,6 +74,8 @@ config MPC85xx_RDB
 config P1010_RDB
 	bool "Freescale P1010RDB"
 	select DEFAULT_UIMAGE
+	select 85xx_HAVE_CAN_FLEXCAN
+	select PPC_CLOCK if CAN_FLEXCAN
 	help
 	  This option enables support for the MPC85xx RDB (P1010 RDB) board
 
diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index a971b32..64ad7a4 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS)  += mpc85xx_ds.o
 obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
 obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
 obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
+obj-$(CONFIG_PPC_CLOCK)   += clock.o
 obj-$(CONFIG_P1022_DS)    += p1022_ds.o
 obj-$(CONFIG_P1023_RDS)   += p1023_rds.o
 obj-$(CONFIG_P2040_RDB)   += p2040_rdb.o corenet_ds.o
diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
new file mode 100644
index 0000000..a25cbf3
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/clock.c
@@ -0,0 +1,59 @@
+
+#include <linux/device.h>
+#include <linux/err.h>
+
+#include <asm/clk_interface.h>
+
+#include <sysdev/fsl_soc.h>
+
+/*
+ * p1010rdb needs to provide a clock source for the flexcan driver.
+ */
+struct clk {
+	unsigned long rate;
+} p1010_rdb_system_clock;
+
+static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
+{
+	const char *dev_init_name;
+
+	if (!dev)
+		return ERR_PTR(-ENOENT);
+
+	/*
+	 * The can devices are named ffe1c000.can0 and ffe1d000.can1 on
+	 * the p1010rdb.  Check for the "can" portion of that name before
+	 * returning a clock source.
+	 */
+	dev_init_name = dev_name(dev);
+	if (strlen(dev_init_name) != 13)
+		return ERR_PTR(-ENOENT);
+	dev_init_name += 9;
+	if (strncmp(dev_init_name, "can", 3))
+		return ERR_PTR(-ENOENT);
+
+	return &p1010_rdb_system_clock;
+}
+
+static void p1010_rdb_clk_put(struct clk *clk)
+{
+	return;
+}
+
+static unsigned long p1010_rdb_clk_get_rate(struct clk *clk)
+{
+	return clk->rate;
+}
+
+static struct clk_interface p1010_rdb_clk_functions = {
+	.clk_get		= p1010_rdb_clk_get,
+	.clk_get_rate		= p1010_rdb_clk_get_rate,
+	.clk_put		= p1010_rdb_clk_put,
+};
+
+void __init p1010_rdb_clk_init(void)
+{
+	p1010_rdb_system_clock.rate = fsl_get_sys_freq();
+	clk_functions = p1010_rdb_clk_functions;
+}
+
diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
index d7387fa..d0afbf9 100644
--- a/arch/powerpc/platforms/85xx/p1010rdb.c
+++ b/arch/powerpc/platforms/85xx/p1010rdb.c
@@ -81,6 +81,15 @@ static void __init p1010_rdb_setup_arch(void)
 	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
 }
 
+extern void p1010_rdb_clk_init(void);
+
+static void __init p1010_rdb_init(void)
+{
+#ifdef CONFIG_PPC_CLOCK
+	p1010_rdb_clk_init();
+#endif
+}
+
 static struct of_device_id __initdata p1010rdb_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
@@ -111,6 +120,7 @@ define_machine(p1010_rdb) {
 	.name			= "P1010 RDB",
 	.probe			= p1010_rdb_probe,
 	.setup_arch		= p1010_rdb_setup_arch,
+	.init			= p1010_rdb_init,
 	.init_IRQ		= p1010_rdb_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
-- 
1.7.2.1

Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Wolfgang Grandegger]

On 08/09/2011 08:33 AM, Robin Holt wrote:
> Argh.  I sent an earlier (non-working) version of this patch.  Here is
> the correct one.

Please always resend the complete series of patches with an incremented
version number. Furthermore, this is not an RFC any more. A prefix
similar to [PATCH nfsl_get_sys_freq() et-next-2.6 v2] would be perfect.

> I added a clock source for the p1010rdb so the flexcan driver
> could find its clock frequency.
> 
> Signed-off-by: Robin Holt <holt@sgi.com>
> To: Marc Kleine-Budde <mkl@pengutronix.de>,
> To: Wolfgang Grandegger <wg@grandegger.com>,
> To: U Bhaskar-B22300 <B22300@freescale.com>
> Cc: socketcan-core@lists.berlios.de,
> Cc: netdev@vger.kernel.org,
> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
> ---
>  arch/powerpc/platforms/85xx/Kconfig    |    6 +++
>  arch/powerpc/platforms/85xx/Makefile   |    1 +
>  arch/powerpc/platforms/85xx/clock.c    |   59 ++++++++++++++++++++++++++++++++
>  arch/powerpc/platforms/85xx/p1010rdb.c |   10 +++++
>  4 files changed, 76 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/platforms/85xx/clock.c
> 
> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> index 498534c..ed4cf92 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -26,6 +26,10 @@ config MPC8560_ADS
>  	help
>  	  This option enables support for the MPC 8560 ADS board
>  
> +config 85xx_HAVE_CAN_FLEXCAN
> +	bool
> +	select HAVE_CAN_FLEXCAN if NET && CAN
> +

Why do you need that? More below...

>  config MPC85xx_CDS
>  	bool "Freescale MPC85xx CDS"
>  	select DEFAULT_UIMAGE
> @@ -70,6 +74,8 @@ config MPC85xx_RDB
>  config P1010_RDB
>  	bool "Freescale P1010RDB"
>  	select DEFAULT_UIMAGE
> +	select 85xx_HAVE_CAN_FLEXCAN
> +	select PPC_CLOCK if CAN_FLEXCAN

	select HAVE_CAN_FLEXCAN
	select PPC_CLOCK

Should just be fine, or have I missed something.

>  	help
>  	  This option enables support for the MPC85xx RDB (P1010 RDB) board
>  
> diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
> index a971b32..64ad7a4 100644
> --- a/arch/powerpc/platforms/85xx/Makefile
> +++ b/arch/powerpc/platforms/85xx/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS)  += mpc85xx_ds.o
>  obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
>  obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
>  obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
> +obj-$(CONFIG_PPC_CLOCK)   += clock.o

I would put that to the beginning or before the board settings.

>  obj-$(CONFIG_P1022_DS)    += p1022_ds.o
>  obj-$(CONFIG_P1023_RDS)   += p1023_rds.o
>  obj-$(CONFIG_P2040_RDB)   += p2040_rdb.o corenet_ds.o
> diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
> new file mode 100644
> index 0000000..a25cbf3
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/clock.c
> @@ -0,0 +1,59 @@
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +
> +#include <asm/clk_interface.h>
> +
> +#include <sysdev/fsl_soc.h>
> +
> +/*
> + * p1010rdb needs to provide a clock source for the flexcan driver.
> + */
> +struct clk {
> +	unsigned long rate;
> +} p1010_rdb_system_clock;
> +
> +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
> +{
> +	const char *dev_init_name;
> +
> +	if (!dev)
> +		return ERR_PTR(-ENOENT);
> +
> +	/*
> +	 * The can devices are named ffe1c000.can0 and ffe1d000.can1 on
> +	 * the p1010rdb.  Check for the "can" portion of that name before
> +	 * returning a clock source.
> +	 */
> +	dev_init_name = dev_name(dev);
> +	if (strlen(dev_init_name) != 13)
> +		return ERR_PTR(-ENOENT);
> +	dev_init_name += 9;
> +	if (strncmp(dev_init_name, "can", 3))
> +		return ERR_PTR(-ENOENT);

What's that good for? Also it's wrong to rely on the special name of the
node. I think it can be removed.

> +	return &p1010_rdb_system_clock;

Just returning fsl_get_sys_freq() here would already be fine. I'm also
missing the factor of two here:

        return fsl_get_sys_freq() / 2; ????

> +}
> +
> +static void p1010_rdb_clk_put(struct clk *clk)
> +{
> +	return;
> +}
> +
> +static unsigned long p1010_rdb_clk_get_rate(struct clk *clk)
> +{
> +	return clk->rate;
> +}
> +
> +static struct clk_interface p1010_rdb_clk_functions = {
> +	.clk_get		= p1010_rdb_clk_get,
> +	.clk_get_rate		= p1010_rdb_clk_get_rate,
> +	.clk_put		= p1010_rdb_clk_put,
> +};
> +
> +void __init p1010_rdb_clk_init(void)
> +{
> +	p1010_rdb_system_clock.rate = fsl_get_sys_freq();

> +	clk_functions = p1010_rdb_clk_functions;
> +}

The name is too specific. The idea is that the interface could be used
for other 85xx processors as well, not only the p1010. The prefix
"mpc85xx_" instead of "p1010_rdb" seems more appropriate to me.

> diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
> index d7387fa..d0afbf9 100644
> --- a/arch/powerpc/platforms/85xx/p1010rdb.c
> +++ b/arch/powerpc/platforms/85xx/p1010rdb.c
> @@ -81,6 +81,15 @@ static void __init p1010_rdb_setup_arch(void)
>  	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
>  }
>  
> +extern void p1010_rdb_clk_init(void);
> +
> +static void __init p1010_rdb_init(void)
> +{
> +#ifdef CONFIG_PPC_CLOCK
> +	p1010_rdb_clk_init();
> +#endif
> +}

The #ifdef's are not needed if CONFIG_PPC_CLOCK is selected in the Kconfig.

>  static struct of_device_id __initdata p1010rdb_ids[] = {
>  	{ .type = "soc", },
>  	{ .compatible = "soc", },
> @@ -111,6 +120,7 @@ define_machine(p1010_rdb) {
>  	.name			= "P1010 RDB",
>  	.probe			= p1010_rdb_probe,
>  	.setup_arch		= p1010_rdb_setup_arch,
> +	.init			= p1010_rdb_init,
>  	.init_IRQ		= p1010_rdb_pic_init,
>  #ifdef CONFIG_PCI
>  	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,

We also need cleanup patches for the Freescale stuff.

Thanks,

Wolfgang.

Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Robin Holt]

On Tue, Aug 09, 2011 at 09:11:33AM +0200, Wolfgang Grandegger wrote:
> > +	return &p1010_rdb_system_clock;
> 
> Just returning fsl_get_sys_freq() here would already be fine. I'm also
> missing the factor of two here:
> 
>         return fsl_get_sys_freq() / 2; ????

I am working on the other comments right now as well, but this one
brought up a good question.  The old algorithm in the original freescale
patches I started with actually did, essentially:

	...clock.freq = DIV_ROUND_CLOSEST(fsl_get_sys_freq() / 2, 1000) * 1000

The end result was before:
	...clock.freq=0x0bebc200
After:
	...clock.freq=0x0bebc1fe

Is that rounding relavent?

Thanks,
Robin

Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Marc Kleine-Budde]

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

On 08/09/2011 01:40 PM, Robin Holt wrote:
> On Tue, Aug 09, 2011 at 09:11:33AM +0200, Wolfgang Grandegger wrote:
>>> +	return &p1010_rdb_system_clock;
>>
>> Just returning fsl_get_sys_freq() here would already be fine. I'm also
>> missing the factor of two here:
>>
>>         return fsl_get_sys_freq() / 2; ????
> 
> I am working on the other comments right now as well, but this one
> brought up a good question.  The old algorithm in the original freescale
> patches I started with actually did, essentially:
> 
> 	...clock.freq = DIV_ROUND_CLOSEST(fsl_get_sys_freq() / 2, 1000) * 1000
> 
> The end result was before:
> 	...clock.freq=0x0bebc200

That's exactly 200 MHz

> After:
> 	...clock.freq=0x0bebc1fe

this is 199 999 998 Hz

> Is that rounding relavent?

_If_ 200 MHz is correct, this would be an error of 0.000001%. But the
fsl code rounds to closest KHz. Rounding introduces errors in most of
the cases. IMHO it's better not to round here.

For example the usual at91can is clocked with 99.532800 MHz, this is a
least the value the arm clock framework reports.

Cheers, 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: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Robin Holt]

On Tue, Aug 09, 2011 at 09:11:33AM +0200, Wolfgang Grandegger wrote:
> On 08/09/2011 08:33 AM, Robin Holt wrote:
> > Argh.  I sent an earlier (non-working) version of this patch.  Here is
> > the correct one.
> 
> Please always resend the complete series of patches with an incremented
> version number. Furthermore, this is not an RFC any more. A prefix
> similar to [PATCH nfsl_get_sys_freq() et-next-2.6 v2] would be perfect.
> 
> > I added a clock source for the p1010rdb so the flexcan driver
> > could find its clock frequency.
> > 
> > Signed-off-by: Robin Holt <holt@sgi.com>
> > To: Marc Kleine-Budde <mkl@pengutronix.de>,
> > To: Wolfgang Grandegger <wg@grandegger.com>,
> > To: U Bhaskar-B22300 <B22300@freescale.com>
> > Cc: socketcan-core@lists.berlios.de,
> > Cc: netdev@vger.kernel.org,
> > Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
> > ---
> >  arch/powerpc/platforms/85xx/Kconfig    |    6 +++
> >  arch/powerpc/platforms/85xx/Makefile   |    1 +
> >  arch/powerpc/platforms/85xx/clock.c    |   59 ++++++++++++++++++++++++++++++++
> >  arch/powerpc/platforms/85xx/p1010rdb.c |   10 +++++
> >  4 files changed, 76 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/powerpc/platforms/85xx/clock.c
> > 
> > diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> > index 498534c..ed4cf92 100644
> > --- a/arch/powerpc/platforms/85xx/Kconfig
> > +++ b/arch/powerpc/platforms/85xx/Kconfig
> > @@ -26,6 +26,10 @@ config MPC8560_ADS
> >  	help
> >  	  This option enables support for the MPC 8560 ADS board
> >  
> > +config 85xx_HAVE_CAN_FLEXCAN
> > +	bool
> > +	select HAVE_CAN_FLEXCAN if NET && CAN
> > +
> 
> Why do you need that? More below...

Happily removed.

> >  config MPC85xx_CDS
> >  	bool "Freescale MPC85xx CDS"
> >  	select DEFAULT_UIMAGE
> > @@ -70,6 +74,8 @@ config MPC85xx_RDB
> >  config P1010_RDB
> >  	bool "Freescale P1010RDB"
> >  	select DEFAULT_UIMAGE
> > +	select 85xx_HAVE_CAN_FLEXCAN
> > +	select PPC_CLOCK if CAN_FLEXCAN
> 
> 	select HAVE_CAN_FLEXCAN
> 	select PPC_CLOCK
> 
> Should just be fine, or have I missed something.

If I don't have the if NET && CAN, then 'make savedefconfig' will
complain.  Otherwise, removed 85xx_HAVE_CAN_FLEXCAN.

> >  	help
> >  	  This option enables support for the MPC85xx RDB (P1010 RDB) board
> >  
> > diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
> > index a971b32..64ad7a4 100644
> > --- a/arch/powerpc/platforms/85xx/Makefile
> > +++ b/arch/powerpc/platforms/85xx/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS)  += mpc85xx_ds.o
> >  obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
> >  obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
> >  obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
> > +obj-$(CONFIG_PPC_CLOCK)   += clock.o
> 
> I would put that to the beginning or before the board settings.

Done.

> >  obj-$(CONFIG_P1022_DS)    += p1022_ds.o
> >  obj-$(CONFIG_P1023_RDS)   += p1023_rds.o
> >  obj-$(CONFIG_P2040_RDB)   += p2040_rdb.o corenet_ds.o
> > diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
> > new file mode 100644
> > index 0000000..a25cbf3
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/85xx/clock.c
> > @@ -0,0 +1,59 @@
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +
> > +#include <asm/clk_interface.h>
> > +
> > +#include <sysdev/fsl_soc.h>
> > +
> > +/*
> > + * p1010rdb needs to provide a clock source for the flexcan driver.
> > + */
> > +struct clk {
> > +	unsigned long rate;
> > +} p1010_rdb_system_clock;
> > +
> > +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
> > +{
> > +	const char *dev_init_name;
> > +
> > +	if (!dev)
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	/*
> > +	 * The can devices are named ffe1c000.can0 and ffe1d000.can1 on
> > +	 * the p1010rdb.  Check for the "can" portion of that name before
> > +	 * returning a clock source.
> > +	 */
> > +	dev_init_name = dev_name(dev);
> > +	if (strlen(dev_init_name) != 13)
> > +		return ERR_PTR(-ENOENT);
> > +	dev_init_name += 9;
> > +	if (strncmp(dev_init_name, "can", 3))
> > +		return ERR_PTR(-ENOENT);
> 
> What's that good for? Also it's wrong to rely on the special name of the
> node. I think it can be removed.

Removed.

> 
> > +	return &p1010_rdb_system_clock;
> 
> Just returning fsl_get_sys_freq() here would already be fine. I'm also
> missing the factor of two here:
> 
>         return fsl_get_sys_freq() / 2; ????

If we were going that route, I would return NULL here and do that
return on the clk_get_rate() call, correct?  I just tested with that
and it worked.

> > +}
> > +
> > +static void p1010_rdb_clk_put(struct clk *clk)
> > +{
> > +	return;
> > +}
> > +
> > +static unsigned long p1010_rdb_clk_get_rate(struct clk *clk)
> > +{
> > +	return clk->rate;
> > +}
> > +
> > +static struct clk_interface p1010_rdb_clk_functions = {
> > +	.clk_get		= p1010_rdb_clk_get,
> > +	.clk_get_rate		= p1010_rdb_clk_get_rate,
> > +	.clk_put		= p1010_rdb_clk_put,
> > +};
> > +
> > +void __init p1010_rdb_clk_init(void)
> > +{
> > +	p1010_rdb_system_clock.rate = fsl_get_sys_freq();
> 
> > +	clk_functions = p1010_rdb_clk_functions;
> > +}
> 
> The name is too specific. The idea is that the interface could be used
> for other 85xx processors as well, not only the p1010. The prefix
> "mpc85xx_" instead of "p1010_rdb" seems more appropriate to me.

Done.

> 
> > diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
> > index d7387fa..d0afbf9 100644
> > --- a/arch/powerpc/platforms/85xx/p1010rdb.c
> > +++ b/arch/powerpc/platforms/85xx/p1010rdb.c
> > @@ -81,6 +81,15 @@ static void __init p1010_rdb_setup_arch(void)
> >  	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
> >  }
> >  
> > +extern void p1010_rdb_clk_init(void);
> > +
> > +static void __init p1010_rdb_init(void)
> > +{
> > +#ifdef CONFIG_PPC_CLOCK
> > +	p1010_rdb_clk_init();
> > +#endif
> > +}
> 
> The #ifdef's are not needed if CONFIG_PPC_CLOCK is selected in the Kconfig.

Done.

> 
> >  static struct of_device_id __initdata p1010rdb_ids[] = {
> >  	{ .type = "soc", },
> >  	{ .compatible = "soc", },
> > @@ -111,6 +120,7 @@ define_machine(p1010_rdb) {
> >  	.name			= "P1010 RDB",
> >  	.probe			= p1010_rdb_probe,
> >  	.setup_arch		= p1010_rdb_setup_arch,
> > +	.init			= p1010_rdb_init,
> >  	.init_IRQ		= p1010_rdb_pic_init,
> >  #ifdef CONFIG_PCI
> >  	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,

Thank you very much for your patience with me,
Robin

Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Marc Kleine-Budde]

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

On 08/09/2011 07:55 AM, Robin Holt wrote:
> I added a clock source for the p1010rdb so the flexcan driver
> could find its clock frequency.
> 
> Signed-off-by: Robin Holt <holt@sgi.com>
> To: Marc Kleine-Budde <mkl@pengutronix.de>,
> To: Wolfgang Grandegger <wg@grandegger.com>,
> To: U Bhaskar-B22300 <B22300@freescale.com>
> Cc: socketcan-core@lists.berlios.de,
> Cc: netdev@vger.kernel.org,
> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>

After fixing Wolfgangs remarks add by Acked-by, for what it's worth in
the ppc-world :)

Some nitpicking from me :) - Darn you send another patch. This comments
still apply.

> 
> ---
> 
> Could I also get a ruling on the Kconfig language.  I could do it either
> with the 85xx_HAVE_CAN_FLEXCAN in or out of the Kconfig file.  It felt
> like the right thing to do was without, but the arm Kconfig files do
> it this way and a patch from freescale had something similar so I went
> this route.
> 
>  arch/powerpc/platforms/85xx/Kconfig    |    6 +++
>  arch/powerpc/platforms/85xx/Makefile   |    1 +
>  arch/powerpc/platforms/85xx/clock.c    |   59 ++++++++++++++++++++++++++++++++
>  arch/powerpc/platforms/85xx/p1010rdb.c |    8 ++++
>  4 files changed, 74 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/platforms/85xx/clock.c
> 
> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> index 498534c..ed4cf92 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -26,6 +26,10 @@ config MPC8560_ADS
>  	help
>  	  This option enables support for the MPC 8560 ADS board
>  
> +config 85xx_HAVE_CAN_FLEXCAN
> +	bool
> +	select HAVE_CAN_FLEXCAN if NET && CAN
> +
>  config MPC85xx_CDS
>  	bool "Freescale MPC85xx CDS"
>  	select DEFAULT_UIMAGE
> @@ -70,6 +74,8 @@ config MPC85xx_RDB
>  config P1010_RDB
>  	bool "Freescale P1010RDB"
>  	select DEFAULT_UIMAGE
> +	select 85xx_HAVE_CAN_FLEXCAN
> +	select PPC_CLOCK if CAN_FLEXCAN
>  	help
>  	  This option enables support for the MPC85xx RDB (P1010 RDB) board
>  
> diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
> index a971b32..64ad7a4 100644
> --- a/arch/powerpc/platforms/85xx/Makefile
> +++ b/arch/powerpc/platforms/85xx/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS)  += mpc85xx_ds.o
>  obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
>  obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
>  obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
> +obj-$(CONFIG_PPC_CLOCK)   += clock.o
>  obj-$(CONFIG_P1022_DS)    += p1022_ds.o
>  obj-$(CONFIG_P1023_RDS)   += p1023_rds.o
>  obj-$(CONFIG_P2040_RDB)   += p2040_rdb.o corenet_ds.o
> diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
> new file mode 100644
> index 0000000..a25cbf3
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/clock.c
> @@ -0,0 +1,59 @@

Please add your copyright notice and add a license, e.g. "This file is
released under GPLv2"

> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +
> +#include <asm/clk_interface.h>
> +
> +#include <sysdev/fsl_soc.h>
> +
> +/*
> + * p1010rdb needs to provide a clock source for the flexcan driver.
> + */
> +struct clk {
> +	unsigned long rate;
> +} p1010_rdb_system_clock;
> +
> +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
> +{
> +	const char *dev_init_name;
> +
> +	if (!dev)
> +		return ERR_PTR(-ENOENT);
> +
> +	/*
> +	 * The can devices are named ffe1c000.can0 and ffe1d000.can1 on
> +	 * the p1010rdb.  Check for the "can" portion of that name before
> +	 * returning a clock source.
> +	 */
> +	dev_init_name = dev_name(dev);
> +	if (strlen(dev_init_name) != 13)
> +		return ERR_PTR(-ENOENT);
> +	dev_init_name += 9;
> +	if (strncmp(dev_init_name, "can", 3))
> +		return ERR_PTR(-ENOENT);
> +
> +	return &p1010_rdb_system_clock;
> +}
> +
> +static void p1010_rdb_clk_put(struct clk *clk)
> +{
> +	return;
> +}
> +
> +static unsigned long p1010_rdb_clk_get_rate(struct clk *clk)
> +{
> +	return clk->rate;
> +}
> +
> +static struct clk_interface p1010_rdb_clk_functions = {
> +	.clk_get		= p1010_rdb_clk_get,
> +	.clk_get_rate		= p1010_rdb_clk_get_rate,
> +	.clk_put		= p1010_rdb_clk_put,

no indention here please, just one space before the "=". YMMV

> +};
> +
> +void __init p1010_rdb_clk_init(void)
> +{
> +	p1010_rdb_system_clock.rate = fsl_get_sys_freq();
> +	clk_functions = p1010_rdb_clk_functions;
> +}
> +
> diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
> index d7387fa..29e04d6 100644
> --- a/arch/powerpc/platforms/85xx/p1010rdb.c
> +++ b/arch/powerpc/platforms/85xx/p1010rdb.c
> @@ -81,6 +81,13 @@ static void __init p1010_rdb_setup_arch(void)
>  	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
>  }
>  
> +static void __init p1010_rdb_init(void)
> +{
> +#ifdef PPC_CLOCK
> +	p1010_rdb_clk_init();
> +#endif
> +}
> +
>  static struct of_device_id __initdata p1010rdb_ids[] = {
>  	{ .type = "soc", },
>  	{ .compatible = "soc", },
> @@ -111,6 +118,7 @@ define_machine(p1010_rdb) {
>  	.name			= "P1010 RDB",
>  	.probe			= p1010_rdb_probe,
>  	.setup_arch		= p1010_rdb_setup_arch,
> +	.init			= p1010_rdb_init,
>  	.init_IRQ		= p1010_rdb_pic_init,
>  #ifdef CONFIG_PCI
>  	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,

cheers, 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: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Robin Holt]

On Tue, Aug 09, 2011 at 02:33:31PM +0200, Marc Kleine-Budde wrote:
> On 08/09/2011 07:55 AM, Robin Holt wrote:
> > I added a clock source for the p1010rdb so the flexcan driver
> > could find its clock frequency.
> > 
> > Signed-off-by: Robin Holt <holt@sgi.com>
> > To: Marc Kleine-Budde <mkl@pengutronix.de>,
> > To: Wolfgang Grandegger <wg@grandegger.com>,
> > To: U Bhaskar-B22300 <B22300@freescale.com>
> > Cc: socketcan-core@lists.berlios.de,
> > Cc: netdev@vger.kernel.org,
> > Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
> 
> After fixing Wolfgangs remarks add by Acked-by, for what it's worth in
> the ppc-world :)

Does that go for the other two patches in the series you have not
commented on in many revs as well?

Robin

Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.

[Marc Kleine-Budde]

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

On 08/09/2011 02:40 PM, Robin Holt wrote:
> On Tue, Aug 09, 2011 at 02:33:31PM +0200, Marc Kleine-Budde wrote:
>> On 08/09/2011 07:55 AM, Robin Holt wrote:
>>> I added a clock source for the p1010rdb so the flexcan driver
>>> could find its clock frequency.
>>>
>>> Signed-off-by: Robin Holt <holt@sgi.com>
>>> To: Marc Kleine-Budde <mkl@pengutronix.de>,
>>> To: Wolfgang Grandegger <wg@grandegger.com>,
>>> To: U Bhaskar-B22300 <B22300@freescale.com>
>>> Cc: socketcan-core@lists.berlios.de,
>>> Cc: netdev@vger.kernel.org,
>>> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
>>
>> After fixing Wolfgangs remarks add by Acked-by, for what it's worth in
>> the ppc-world :)
> 
> Does that go for the other two patches in the series you have not
> commented on in many revs as well?

They look pretty good so far! I'll compile the next round on arm and
hopefully find time to test on HW.

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