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

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

[Robin Holt]

Marc, Wolfgang or U Bhaskar,

This patch set should have all your comments included.

I did implement a very simple clock source in the p1010rdb.c file, which,
unfortunately, your tree will not have so please do not apply the last
patch in the series.  That will need to go to the powerpc folks and
follow the p1010rdb patch from freescale.

Could you please apply the first three patches to a test branch, compile
and test them on an arm based system?  I would like to at least feel
comfortable that I have not broken anything there.

I have tested the full set on a p1010rdb with an external PSOC based
can communicator.  That PSOC code has a bunch of erroneous can comms it
can generate, but I do not know how the developer of that code injects
those errors.  As a result, no error handling from the can input has been
tested.  I have tested both flexcan interfaces on the board and both work
with these patches in addition to the other p1010rdb patches not included.

This series has one additional patch which replaces the mach/clock.h
with the linux/clkdev.h include.

Thanks,
Robin Holt

[RFC 1/5] [flexcan] Replace mach/clock.h with linux/clkdev.h

[Robin Holt]

powerpc does not have a mach-####/clock.h.  It does implement
a generic clkdev.h which is what appears to be needed to
compile flexcan.c for either arm or powerpc.

Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/net/can/flexcan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 1767811..ea4e24a 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -24,6 +24,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/platform/flexcan.h>
+#include <linux/clkdev.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/if_arp.h>
@@ -35,8 +36,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/5] [flexcan] Abstract off read/write for big/little endian.

[Robin Holt]

First step in converting the flexcan driver from supporting just arm to
supporting both arm and powerpc architectures.

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
---
 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 ea4e24a..ecdd4e6 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -191,6 +191,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)
@@ -211,9 +236,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);
 }
@@ -223,9 +248,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,
@@ -233,7 +258,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;
@@ -267,15 +292,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);
 
@@ -463,8 +488,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
@@ -474,12 +499,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)
@@ -515,17 +540,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 */
@@ -535,8 +560,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;
@@ -550,9 +575,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:
@@ -568,16 +593,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++;
 	}
@@ -586,7 +611,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);
 	}
 
@@ -600,7 +625,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) |
@@ -624,11 +649,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));
 }
 
 /*
@@ -649,10 +674,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",
@@ -674,12 +699,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
@@ -697,7 +722,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;
@@ -705,38 +730,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;
 
@@ -758,12 +784,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;
@@ -855,24 +881,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/5] [flexcan] Add of_match to platform_device definition.

[Robin Holt]

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

---
I kept the of_match for "fsl,flexcan-v1.0" for the time being.  I will
happily drop it for final submission once I have a boot loader worked
up that matches on either string.
---
 drivers/net/can/flexcan.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index ecdd4e6..d4ac81b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1028,8 +1028,22 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id flexcan_of_match[] = {
+	{
+		.compatible = "fsl,flexcan-v1.0",
+	},
+	{
+		.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/5] [flexcan] Add support for FLEXCAN_DEBUG

[Robin Holt]

Add a wrapper function for a register dump when a developer defines
FLEXCAN_DEBUG.

Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/net/can/flexcan.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index d4ac81b..c2fb829 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -215,6 +215,35 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
 }
 #endif
 
+#if defined(FLEXCAN_DEBUG)
+void _flexcan_reg_dump(struct net_device *dev, const char *file, int line,
+		       const char *func)
+{
+	const struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_regs __iomem *regs = priv->base;
+
+	netdev_info("flexcan_reg_dump:%s:%d:%s()\n", file, line, func);
+	netdev_info("\t  mcr 0x%08x  ctrl 0x%08x timer 0x%08x   rxg 0x%08x",
+		flexcan_read(&regs->mcr),
+		flexcan_read(&regs->ctrl),
+		flexcan_read(&regs->timer),
+		flexcan_read(&regs->rxgmask));
+	netdev_info("\t rx14 0x%08x  rx15 0x%08x   ecr 0x%08x   esr 0x%08x",
+		flexcan_read(&regs->rx14mask),
+		flexcan_read(&regs->rx15mask),
+		flexcan_read(&regs->ecr),
+		flexcan_read(&regs->esr));
+	netdev_info("\timsk2 0x%08x imsk1 0x%08x iflg2 0x%08x iflg1 0x%08x",
+		flexcan_read(&regs->imask2),
+		flexcan_read(&regs->imask1),
+		flexcan_read(&regs->iflag2),
+		flexcan_read(&regs->iflag1));
+}
+#define flexcan_reg_dump(_d) _flexcan_reg_dump(_d, __FILE__, __LINE__, __func__)
+#else
+#define flexcan_reg_dump(_d)
+#endif
+
 /*
  * Swtich transceiver on or off
  */
@@ -275,6 +304,8 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 can_id;
 	u32 ctrl = FLEXCAN_MB_CNT_CODE(0xc) | (cf->can_dlc << 16);
 
+	flexcan_reg_dump(dev);
+
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
 
@@ -307,6 +338,8 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* tx_packets is incremented in flexcan_irq */
 	stats->tx_bytes += cf->can_dlc;
 
+	flexcan_reg_dump(dev);
+
 	return NETDEV_TX_OK;
 }
 
@@ -575,6 +608,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg_iflag1, reg_esr;
 
+	flexcan_reg_dump(dev);
+
 	reg_iflag1 = flexcan_read(&regs->iflag1);
 	reg_esr = flexcan_read(&regs->esr);
 	flexcan_write(FLEXCAN_ESR_ERR_INT, &regs->esr);	/* ACK err IRQ */
@@ -615,6 +650,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		netif_wake_queue(dev);
 	}
 
+	flexcan_reg_dump(dev);
+
 	return IRQ_HANDLED;
 }
 
-- 
1.7.2.1

[RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

flexcan driver needs the clk_get, clk_get_rate, etc functions
to work.  This patch provides the minimum functionality.

Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 arch/powerpc/platforms/85xx/p1010rdb.c |   78 ++++++++++++++++++++++++++++++++
 1 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
index 3540a88..8f78ddd 100644
--- a/arch/powerpc/platforms/85xx/p1010rdb.c
+++ b/arch/powerpc/platforms/85xx/p1010rdb.c
@@ -28,6 +28,7 @@
 #include <asm/udbg.h>
 #include <asm/mpic.h>
 #include <asm/swiotlb.h>
+#include <asm/clk_interface.h>
 
 #include <sysdev/fsl_soc.h>
 #include <sysdev/fsl_pci.h>
@@ -164,6 +165,82 @@ static void __init p1010_rdb_setup_arch(void)
 	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
 }
 
+/*
+ * p1010rdb needs to provide a clock source for the flexcan driver.
+ */
+struct clk {
+	unsigned long rate;
+} p1010rdb_system_clk;
+
+static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
+{
+	struct clk *clk;
+	u32 *of_property;
+	unsigned long clock_freq, clock_divider;
+	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);
+
+	of_property = (u32 *)of_get_property(dev->of_node, "clock_freq", NULL);
+	if (!of_property)
+		return ERR_PTR(-ENOENT);
+	clock_freq = *of_property;
+
+	of_property = (u32 *)of_get_property(dev->of_node,
+					     "fsl,flexcan-clock-divider", NULL);
+	if (!of_property)
+		return ERR_PTR(-ENOENT);
+	clock_divider = *of_property;
+
+	clk = kmalloc(sizeof(struct clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	clk->rate = DIV_ROUND_CLOSEST(clock_freq / clock_divider, 1000);
+	clk->rate *= 1000;
+
+	return clk;
+}
+
+static void p1010_rdb_clk_put(struct clk *clk)
+{
+	kfree(clk);
+}
+
+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,
+};
+
+static void __init p1010_rdb_clk_init(void)
+{
+	clk_functions = p1010_rdb_clk_functions;
+}
+
+static void __init p1010_rdb_init(void)
+{
+	p1010_rdb_clk_init();
+}
+
 static struct of_device_id __initdata p1010rdb_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
@@ -195,6 +272,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 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/06/2011 04:34 PM, Robin Holt wrote:
> flexcan driver needs the clk_get, clk_get_rate, etc functions
> to work.  This patch provides the minimum functionality.

This needs some more general thoughts... apart from the question where
the code should go.

Like for the MSCAN on the MPC5200, the user should be *able* to select
an appropriate clock source and divider via DTS node properties.
Currently it seems, that the DTS properties must match some
pre-configured values, most likely set by the boot loader. Please
correct me if I'm wrong. For me this is generic and should go into the
Flexcan driver. From there, a platform specific function, e.g.
flexcan_set_clock() might be called.


> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
> To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/powerpc/platforms/85xx/p1010rdb.c |   78 ++++++++++++++++++++++++++++++++
>  1 files changed, 78 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
> index 3540a88..8f78ddd 100644
> --- a/arch/powerpc/platforms/85xx/p1010rdb.c
> +++ b/arch/powerpc/platforms/85xx/p1010rdb.c
> @@ -28,6 +28,7 @@
>  #include <asm/udbg.h>
>  #include <asm/mpic.h>
>  #include <asm/swiotlb.h>
> +#include <asm/clk_interface.h>
>  
>  #include <sysdev/fsl_soc.h>
>  #include <sysdev/fsl_pci.h>
> @@ -164,6 +165,82 @@ static void __init p1010_rdb_setup_arch(void)
>  	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
>  }
>  
> +/*
> + * p1010rdb needs to provide a clock source for the flexcan driver.
> + */
> +struct clk {
> +	unsigned long rate;
> +} p1010rdb_system_clk;
> +
> +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
> +{

I persoanlly don't like the

> +	struct clk *clk;
> +	u32 *of_property;
> +	unsigned long clock_freq, clock_divider;
> +	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);

There are dedicated functions to find the of node. But with my above
proposal we do not need to provide p1010_rdb_clk_get().

More comments on the other patches soon...

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
> On 08/06/2011 04:34 PM, Robin Holt wrote:
> > flexcan driver needs the clk_get, clk_get_rate, etc functions
> > to work.  This patch provides the minimum functionality.
> 
> This needs some more general thoughts... apart from the question where
> the code should go.
> 
> Like for the MSCAN on the MPC5200, the user should be *able* to select
> an appropriate clock source and divider via DTS node properties.
> Currently it seems, that the DTS properties must match some
> pre-configured values, most likely set by the boot loader. Please
> correct me if I'm wrong. For me this is generic and should go into the
> Flexcan driver. From there, a platform specific function, e.g.
> flexcan_set_clock() might be called.

The P1010 really only supports the system bus clock for a source.  I was
wrong last week when I said it supported either.  That was a confusion
I have because of a task I was assigned a couple months ago.

It can select different divider values.

Robin

> 
> 
> > Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
> > To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> > To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> > Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > ---
> >  arch/powerpc/platforms/85xx/p1010rdb.c |   78 ++++++++++++++++++++++++++++++++
> >  1 files changed, 78 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
> > index 3540a88..8f78ddd 100644
> > --- a/arch/powerpc/platforms/85xx/p1010rdb.c
> > +++ b/arch/powerpc/platforms/85xx/p1010rdb.c
> > @@ -28,6 +28,7 @@
> >  #include <asm/udbg.h>
> >  #include <asm/mpic.h>
> >  #include <asm/swiotlb.h>
> > +#include <asm/clk_interface.h>
> >  
> >  #include <sysdev/fsl_soc.h>
> >  #include <sysdev/fsl_pci.h>
> > @@ -164,6 +165,82 @@ static void __init p1010_rdb_setup_arch(void)
> >  	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
> >  }
> >  
> > +/*
> > + * p1010rdb needs to provide a clock source for the flexcan driver.
> > + */
> > +struct clk {
> > +	unsigned long rate;
> > +} p1010rdb_system_clk;
> > +
> > +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
> > +{
> 
> I persoanlly don't like the
> 
> > +	struct clk *clk;
> > +	u32 *of_property;
> > +	unsigned long clock_freq, clock_divider;
> > +	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);
> 
> There are dedicated functions to find the of node. But with my above
> proposal we do not need to provide p1010_rdb_clk_get().
> 
> More comments on the other patches soon...
> 
> Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
> On 08/06/2011 04:34 PM, Robin Holt wrote:
> > flexcan driver needs the clk_get, clk_get_rate, etc functions
> > to work.  This patch provides the minimum functionality.
> 
> This needs some more general thoughts... apart from the question where
> the code should go.
> 
> Like for the MSCAN on the MPC5200, the user should be *able* to select
> an appropriate clock source and divider via DTS node properties.
> Currently it seems, that the DTS properties must match some
> pre-configured values, most likely set by the boot loader. Please
> correct me if I'm wrong. For me this is generic and should go into the
> Flexcan driver. From there, a platform specific function, e.g.
> flexcan_set_clock() might be called.

OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
periphereal clock frequency which is system bus frequency divided
by 2.  The clock source can not be changed, but the clock divider can
by freezing the interface and setting the CTRL register.  This appears
to only be done by the boot loader.  I do not see why we can not leave
that functionality in the boot loader and then go back to a variation
on my earlier flexcan_clk_* patch.  Is that close to the direction you
think we should go or have I completely misunderstood your wishes?

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1483 bytes --]

On 08/08/2011 01:31 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
>> On 08/06/2011 04:34 PM, Robin Holt wrote:
>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
>>> to work.  This patch provides the minimum functionality.
>>
>> This needs some more general thoughts... apart from the question where
>> the code should go.
>>
>> Like for the MSCAN on the MPC5200, the user should be *able* to select
>> an appropriate clock source and divider via DTS node properties.
>> Currently it seems, that the DTS properties must match some
>> pre-configured values, most likely set by the boot loader. Please
>> correct me if I'm wrong. For me this is generic and should go into the
>> Flexcan driver. From there, a platform specific function, e.g.
>> flexcan_set_clock() might be called.
> 
> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
> periphereal clock frequency which is system bus frequency divided
> by 2.  The clock source can not be changed, but the clock divider can
> by freezing the interface and setting the CTRL register.  This appears

Which bit(s) in the CTRL register is/are this?

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 02:07:32PM +0200, Marc Kleine-Budde wrote:
> On 08/08/2011 01:31 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
> >> On 08/06/2011 04:34 PM, Robin Holt wrote:
> >>> flexcan driver needs the clk_get, clk_get_rate, etc functions
> >>> to work.  This patch provides the minimum functionality.
> >>
> >> This needs some more general thoughts... apart from the question where
> >> the code should go.
> >>
> >> Like for the MSCAN on the MPC5200, the user should be *able* to select
> >> an appropriate clock source and divider via DTS node properties.
> >> Currently it seems, that the DTS properties must match some
> >> pre-configured values, most likely set by the boot loader. Please
> >> correct me if I'm wrong. For me this is generic and should go into the
> >> Flexcan driver. From there, a platform specific function, e.g.
> >> flexcan_set_clock() might be called.
> > 
> > OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
> > periphereal clock frequency which is system bus frequency divided
> > by 2.  The clock source can not be changed, but the clock divider can
> > by freezing the interface and setting the CTRL register.  This appears
> 
> Which bit(s) in the CTRL register is/are this?

PRESDIV bits 24-31.  Documented on the P1010 reference manual section 21.3.3.2.

Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

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

On 08/08/2011 02:48 PM, Robin Holt wrote:
>>> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
>>> periphereal clock frequency which is system bus frequency divided
>>> by 2.  The clock source can not be changed, but the clock divider can
>>> by freezing the interface and setting the CTRL register.  This appears

>> Which bit(s) in the CTRL register is/are this?

> PRESDIV bits 24-31.  Documented on the P1010 reference manual section 21.3.3.2.

We have and use these bits on arm, too. These bits are calculated and
set by the driver[1]. These bits are touched in freescale's 2.6.35
version of the flexcan driver, too.

From my point of view it makes no sense to specify this divider in the
OF-tree. We just need the frequency of the clock entering the flexcan
core. Which is, as you say, the system bus freq/2.

[1]
http://lxr.linux.no/linux+v3.0.1/drivers/net/can/flexcan.c#L597
-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

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

On 08/08/2011 01:31 PM, Robin Holt wrote:
> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
> periphereal clock frequency which is system bus frequency divided
> by 2.  The clock source can not be changed, but the clock divider can

On arm the clock source is selected by bit 13 of CTRL:

> This bit selects the clock source to the CAN protocol interface (CPI)
> to be either the peripheral clock (driven by the PLL) or the crystal
> oscillator clock. The selected clock is the one fed to the prescaler
> to generate the SCLK (SCLK). In order to guarantee reliable
> operation, this bit must only be changed while the module is in
> disable mode. See Section 24.4.8.4, “Protocol Timing,” for more
> information.

> 0 The CAN engine clock source is the oscillator clock (24.576 MHz)
> 1 The CAN engine clock source is the bus clock (66.5 MHz)

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 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 01:31 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
>> On 08/06/2011 04:34 PM, Robin Holt wrote:
>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
>>> to work.  This patch provides the minimum functionality.
>>
>> This needs some more general thoughts... apart from the question where
>> the code should go.
>>
>> Like for the MSCAN on the MPC5200, the user should be *able* to select
>> an appropriate clock source and divider via DTS node properties.
>> Currently it seems, that the DTS properties must match some
>> pre-configured values, most likely set by the boot loader. Please
>> correct me if I'm wrong. For me this is generic and should go into the
>> Flexcan driver. From there, a platform specific function, e.g.
>> flexcan_set_clock() might be called.
> 
> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
> periphereal clock frequency which is system bus frequency divided
> by 2.  The clock source can not be changed, but the clock divider can
> by freezing the interface and setting the CTRL register.  This appears
> to only be done by the boot loader.  I do not see why we can not leave

And likely Freescale's bootloader does also fixup the DTS Flexcan node.
Ah, oh, there's already someting in the mainline U-BOOT:

commit 65bb8b060a873fa4f5188f2951081f6011259614
Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Date:   Fri Mar 4 20:27:58 2011 +0530

    powerpc/85xx: Fix up clock_freq property in CAN node of dts

    Fix up the device tree property associated with the Flexcan clock
    frequency. This property is used to calculate the bit timing parameters
    for Flexcan.

    Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
    Signed-off-by: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>


> that functionality in the boot loader and then go back to a variation
> on my earlier flexcan_clk_* patch.  Is that close to the direction you
> think we should go or have I completely misunderstood your wishes?

The boot loader might not chose the optimum clock source and frequency,
which might even be application dependent. Therefore it would be nice to
allow the user to change it if necessary. Some CAN interfaces do even
allow to use an external clock source. The main question is where we add
that functionality. As more as I think of it, the clock interface would
not be that bad, especially if it's available.

Furthermore, if the bootloader sets the clock source and divider, we do
not need device tree properties for it. A simply register lookup would
reveal what values are used. We may just need the input clock source.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 03:00 PM, Marc Kleine-Budde wrote:
> On 08/08/2011 02:48 PM, Robin Holt wrote:
>>>> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
>>>> periphereal clock frequency which is system bus frequency divided
>>>> by 2.  The clock source can not be changed, but the clock divider can
>>>> by freezing the interface and setting the CTRL register.  This appears
> 
>>> Which bit(s) in the CTRL register is/are this?
> 
>> PRESDIV bits 24-31.  Documented on the P1010 reference manual section 21.3.3.2.
> 
> We have and use these bits on arm, too. These bits are calculated and
> set by the driver[1]. These bits are touched in freescale's 2.6.35
> version of the flexcan driver, too.

OK, now it starts making sense. And these bits are set according to
relevant DTS properties, I assume. And the input frequency is derived
from the "clock_freq" property entered by the U-Boot bootloader.

> From my point of view it makes no sense to specify this divider in the
> OF-tree. We just need the frequency of the clock entering the flexcan
> core. Which is, as you say, the system bus freq/2.
> 
> [1]
> http://lxr.linux.no/linux+v3.0.1/drivers/net/can/flexcan.c#L597

Yep, I just wrote a similar comment in my previous mail.

So, of we want to allow the user to define the clock source and divider,
we should provide an interface for both, ARM and PPC.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3890 bytes --]

On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote:
> On 08/08/2011 01:31 PM, Robin Holt wrote:
>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
>>> On 08/06/2011 04:34 PM, Robin Holt wrote:
>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
>>>> to work.  This patch provides the minimum functionality.
>>>
>>> This needs some more general thoughts... apart from the question where
>>> the code should go.
>>>
>>> Like for the MSCAN on the MPC5200, the user should be *able* to select
>>> an appropriate clock source and divider via DTS node properties.
>>> Currently it seems, that the DTS properties must match some
>>> pre-configured values, most likely set by the boot loader. Please
>>> correct me if I'm wrong. For me this is generic and should go into the
>>> Flexcan driver. From there, a platform specific function, e.g.
>>> flexcan_set_clock() might be called.
>>
>> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
>> periphereal clock frequency which is system bus frequency divided
>> by 2.  The clock source can not be changed, but the clock divider can
>> by freezing the interface and setting the CTRL register.  This appears
>> to only be done by the boot loader.  I do not see why we can not leave
> 
> And likely Freescale's bootloader does also fixup the DTS Flexcan node.
> Ah, oh, there's already someting in the mainline U-BOOT:
> 
> commit 65bb8b060a873fa4f5188f2951081f6011259614
> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Date:   Fri Mar 4 20:27:58 2011 +0530
> 
>     powerpc/85xx: Fix up clock_freq property in CAN node of dts
> 
>     Fix up the device tree property associated with the Flexcan clock
>     frequency. This property is used to calculate the bit timing parameters
>     for Flexcan.
> 
>     Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>     Signed-off-by: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> 
> 
>> that functionality in the boot loader and then go back to a variation
>> on my earlier flexcan_clk_* patch.  Is that close to the direction you
>> think we should go or have I completely misunderstood your wishes?
> 
> The boot loader might not chose the optimum clock source and frequency,
> which might even be application dependent. Therefore it would be nice to
> allow the user to change it if necessary. Some CAN interfaces do even
> allow to use an external clock source. The main question is where we add
> that functionality. As more as I think of it, the clock interface would
> not be that bad, especially if it's available.
> 
> Furthermore, if the bootloader sets the clock source and divider, we do
> not need device tree properties for it. A simply register lookup would
> reveal what values are used. We may just need the input clock source.

If the bootloader touches the divider _in_ the flexcan core, that would
make absolutely no sense. The clock divider in the flexcan core (in the
CTRL register) is the bitrate pre-scaler calculated by the bit-timing
algorithm.

What we need in the device tree is, from my point of view.
a) the used clock source (bus clock or xtal clock)
b) the frequency of that clock

These problems are solved on arm via:
a) bus clock is hard coded [1]
b) get that clock frequency via clk_get_rate().

Marc

[1] I just talked to Sascha (the i.mx maintainer), there's no support
for the xtal clock, which is the OSC_AUDIO on mx35, in the i.mx clock
framework so far.

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

> commit 65bb8b060a873fa4f5188f2951081f6011259614
> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Date:   Fri Mar 4 20:27:58 2011 +0530

On a side note, that commit fixes up "fsl,flexcan-v1.0"
...
+       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
+                       "clock_freq", gd->bus_clk, 1);

Should I go back to flexcan-v1.0 in my patches?

Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 03:44:36PM +0200, Marc Kleine-Budde wrote:
> On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote:
> > On 08/08/2011 01:31 PM, Robin Holt wrote:
> >> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
> >>> On 08/06/2011 04:34 PM, Robin Holt wrote:
> >>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
> >>>> to work.  This patch provides the minimum functionality.
> >>>
> >>> This needs some more general thoughts... apart from the question where
> >>> the code should go.
> >>>
> >>> Like for the MSCAN on the MPC5200, the user should be *able* to select
> >>> an appropriate clock source and divider via DTS node properties.
> >>> Currently it seems, that the DTS properties must match some
> >>> pre-configured values, most likely set by the boot loader. Please
> >>> correct me if I'm wrong. For me this is generic and should go into the
> >>> Flexcan driver. From there, a platform specific function, e.g.
> >>> flexcan_set_clock() might be called.
> >>
> >> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
> >> periphereal clock frequency which is system bus frequency divided
> >> by 2.  The clock source can not be changed, but the clock divider can
> >> by freezing the interface and setting the CTRL register.  This appears
> >> to only be done by the boot loader.  I do not see why we can not leave
> > 
> > And likely Freescale's bootloader does also fixup the DTS Flexcan node.
> > Ah, oh, there's already someting in the mainline U-BOOT:
> > 
> > commit 65bb8b060a873fa4f5188f2951081f6011259614
> > Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Date:   Fri Mar 4 20:27:58 2011 +0530
> > 
> >     powerpc/85xx: Fix up clock_freq property in CAN node of dts
> > 
> >     Fix up the device tree property associated with the Flexcan clock
> >     frequency. This property is used to calculate the bit timing parameters
> >     for Flexcan.
> > 
> >     Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >     Signed-off-by: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> > 
> > 
> >> that functionality in the boot loader and then go back to a variation
> >> on my earlier flexcan_clk_* patch.  Is that close to the direction you
> >> think we should go or have I completely misunderstood your wishes?
> > 
> > The boot loader might not chose the optimum clock source and frequency,
> > which might even be application dependent. Therefore it would be nice to
> > allow the user to change it if necessary. Some CAN interfaces do even
> > allow to use an external clock source. The main question is where we add
> > that functionality. As more as I think of it, the clock interface would
> > not be that bad, especially if it's available.
> > 
> > Furthermore, if the bootloader sets the clock source and divider, we do
> > not need device tree properties for it. A simply register lookup would
> > reveal what values are used. We may just need the input clock source.
> 
> If the bootloader touches the divider _in_ the flexcan core, that would
> make absolutely no sense. The clock divider in the flexcan core (in the
> CTRL register) is the bitrate pre-scaler calculated by the bit-timing
> algorithm.
> 
> What we need in the device tree is, from my point of view.
> a) the used clock source (bus clock or xtal clock)
> b) the frequency of that clock
> 
> These problems are solved on arm via:
> a) bus clock is hard coded [1]
> b) get that clock frequency via clk_get_rate().

Just to make sure I understand correctly, the clk_get_rate() return
value comes from the device tree and a mach specific handler, right?
And 'mach-specific' really means what, a processor family?

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 03:56 PM, Robin Holt wrote:
>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Date:   Fri Mar 4 20:27:58 2011 +0530
> 
> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> ...
> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> +                       "clock_freq", gd->bus_clk, 1);
> 
> Should I go back to flexcan-v1.0 in my patches?

Well, no. Let's wait. I don't think we need it. Also, it sets
"clock_freq" while

 http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt

documents "clock-frequencies"... :-(.

Wolfgang,

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 03:44 PM, Marc Kleine-Budde wrote:
> On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote:
>> On 08/08/2011 01:31 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
>>>> On 08/06/2011 04:34 PM, Robin Holt wrote:
>>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
>>>>> to work.  This patch provides the minimum functionality.
>>>>
>>>> This needs some more general thoughts... apart from the question where
>>>> the code should go.
>>>>
>>>> Like for the MSCAN on the MPC5200, the user should be *able* to select
>>>> an appropriate clock source and divider via DTS node properties.
>>>> Currently it seems, that the DTS properties must match some
>>>> pre-configured values, most likely set by the boot loader. Please
>>>> correct me if I'm wrong. For me this is generic and should go into the
>>>> Flexcan driver. From there, a platform specific function, e.g.
>>>> flexcan_set_clock() might be called.
>>>
>>> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
>>> periphereal clock frequency which is system bus frequency divided
>>> by 2.  The clock source can not be changed, but the clock divider can
>>> by freezing the interface and setting the CTRL register.  This appears
>>> to only be done by the boot loader.  I do not see why we can not leave
>>
>> And likely Freescale's bootloader does also fixup the DTS Flexcan node.
>> Ah, oh, there's already someting in the mainline U-BOOT:
>>
>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>
>>     powerpc/85xx: Fix up clock_freq property in CAN node of dts
>>
>>     Fix up the device tree property associated with the Flexcan clock
>>     frequency. This property is used to calculate the bit timing parameters
>>     for Flexcan.
>>
>>     Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>     Signed-off-by: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>>
>>
>>> that functionality in the boot loader and then go back to a variation
>>> on my earlier flexcan_clk_* patch.  Is that close to the direction you
>>> think we should go or have I completely misunderstood your wishes?
>>
>> The boot loader might not chose the optimum clock source and frequency,
>> which might even be application dependent. Therefore it would be nice to
>> allow the user to change it if necessary. Some CAN interfaces do even
>> allow to use an external clock source. The main question is where we add
>> that functionality. As more as I think of it, the clock interface would
>> not be that bad, especially if it's available.
>>
>> Furthermore, if the bootloader sets the clock source and divider, we do
>> not need device tree properties for it. A simply register lookup would
>> reveal what values are used. We may just need the input clock source.
> 
> If the bootloader touches the divider _in_ the flexcan core, that would
> make absolutely no sense. The clock divider in the flexcan core (in the
> CTRL register) is the bitrate pre-scaler calculated by the bit-timing
> algorithm.

Right, as I realized in the meantime. I'm still looking for some special
p1010 registers for the divider. Unfortunately, the manual is only
available under NDA :-(.

> What we need in the device tree is, from my point of view.
> a) the used clock source (bus clock or xtal clock)
> b) the frequency of that clock

Yes, and maybe an additional divider, like available for the MPC512x:

http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/mpc5xxx-mscan.txt
http://lxr.linux.no/#linux+v3.0.1/drivers/net/can/mscan/mpc5xxx_can.c#L132

Here is documented what you can expect from the PowerPC SOCs:

http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt

And there they also speak an *additional" clock divider. Maybe that
forseen for the next generations. U Bhaska, could you clarify that? Thanks?

> These problems are solved on arm via:
> a) bus clock is hard coded [1]
> b) get that clock frequency via clk_get_rate().

OK. The clk interface is fine and it should derive the frequency from
the relevant register settings and the bus clock frequency.

> Marc
> 
> [1] I just talked to Sascha (the i.mx maintainer), there's no support
> for the xtal clock, which is the OSC_AUDIO on mx35, in the i.mx clock
> framework so far.

OK. We may want to provide an interface to select taht sometimes later,
also because the P1010 does only support *one* clock source.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4582 bytes --]

On 08/08/2011 04:03 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 03:44:36PM +0200, Marc Kleine-Budde wrote:
>> On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote:
>>> On 08/08/2011 01:31 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/06/2011 04:34 PM, Robin Holt wrote:
>>>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
>>>>>> to work.  This patch provides the minimum functionality.
>>>>>
>>>>> This needs some more general thoughts... apart from the question where
>>>>> the code should go.
>>>>>
>>>>> Like for the MSCAN on the MPC5200, the user should be *able* to select
>>>>> an appropriate clock source and divider via DTS node properties.
>>>>> Currently it seems, that the DTS properties must match some
>>>>> pre-configured values, most likely set by the boot loader. Please
>>>>> correct me if I'm wrong. For me this is generic and should go into the
>>>>> Flexcan driver. From there, a platform specific function, e.g.
>>>>> flexcan_set_clock() might be called.
>>>>
>>>> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
>>>> periphereal clock frequency which is system bus frequency divided
>>>> by 2.  The clock source can not be changed, but the clock divider can
>>>> by freezing the interface and setting the CTRL register.  This appears
>>>> to only be done by the boot loader.  I do not see why we can not leave
>>>
>>> And likely Freescale's bootloader does also fixup the DTS Flexcan node.
>>> Ah, oh, there's already someting in the mainline U-BOOT:
>>>
>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>
>>>     powerpc/85xx: Fix up clock_freq property in CAN node of dts
>>>
>>>     Fix up the device tree property associated with the Flexcan clock
>>>     frequency. This property is used to calculate the bit timing parameters
>>>     for Flexcan.
>>>
>>>     Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>     Signed-off-by: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>>>
>>>
>>>> that functionality in the boot loader and then go back to a variation
>>>> on my earlier flexcan_clk_* patch.  Is that close to the direction you
>>>> think we should go or have I completely misunderstood your wishes?
>>>
>>> The boot loader might not chose the optimum clock source and frequency,
>>> which might even be application dependent. Therefore it would be nice to
>>> allow the user to change it if necessary. Some CAN interfaces do even
>>> allow to use an external clock source. The main question is where we add
>>> that functionality. As more as I think of it, the clock interface would
>>> not be that bad, especially if it's available.
>>>
>>> Furthermore, if the bootloader sets the clock source and divider, we do
>>> not need device tree properties for it. A simply register lookup would
>>> reveal what values are used. We may just need the input clock source.
>>
>> If the bootloader touches the divider _in_ the flexcan core, that would
>> make absolutely no sense. The clock divider in the flexcan core (in the
>> CTRL register) is the bitrate pre-scaler calculated by the bit-timing
>> algorithm.
>>
>> What we need in the device tree is, from my point of view.
>> a) the used clock source (bus clock or xtal clock)
>> b) the frequency of that clock
>>
>> These problems are solved on arm via:
>> a) bus clock is hard coded [1]
>> b) get that clock frequency via clk_get_rate().
> 
> Just to make sure I understand correctly, the clk_get_rate() return
> value comes from the device tree and a mach specific handler, right?
> And 'mach-specific' really means what, a processor family?

I'm talking about the mainline driver, that has no device tree support.
The clock stuff on arm currently goes like this:

The driver asks for the clock related to the device. The architecture
code has previously connected the flexcan device to an arch specific
(i.mx25, i.mx35) clock. That clock is returned. Enable/disable/get_rate
are working on that specific clock.

hope that helps, 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> Date:   Fri Mar 4 20:27:58 2011 +0530
> > 
> > On a side note, that commit fixes up "fsl,flexcan-v1.0"
> > ...
> > +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> > +                       "clock_freq", gd->bus_clk, 1);
> > 
> > Should I go back to flexcan-v1.0 in my patches?
> 
> Well, no. Let's wait. I don't think we need it. Also, it sets
> "clock_freq" while
> 
>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> 
> documents "clock-frequencies"... :-(.

You answered a different question that I was asking.  I was asking if
I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
line 5.  The clock_freq looks like a uboot change will need to be made
as well.

Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 04:19:20PM +0200, Marc Kleine-Budde wrote:
> On 08/08/2011 04:03 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 03:44:36PM +0200, Marc Kleine-Budde wrote:
> >> On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote:
> >>> On 08/08/2011 01:31 PM, Robin Holt wrote:
> >>>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote:
> >>>>> On 08/06/2011 04:34 PM, Robin Holt wrote:
> >>>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions
> >>>>>> to work.  This patch provides the minimum functionality.
> >>>>>
> >>>>> This needs some more general thoughts... apart from the question where
> >>>>> the code should go.
> >>>>>
> >>>>> Like for the MSCAN on the MPC5200, the user should be *able* to select
> >>>>> an appropriate clock source and divider via DTS node properties.
> >>>>> Currently it seems, that the DTS properties must match some
> >>>>> pre-configured values, most likely set by the boot loader. Please
> >>>>> correct me if I'm wrong. For me this is generic and should go into the
> >>>>> Flexcan driver. From there, a platform specific function, e.g.
> >>>>> flexcan_set_clock() might be called.
> >>>>
> >>>> OK.  Dug a bit more.  The p1010 built-in clocksource seems to be the
> >>>> periphereal clock frequency which is system bus frequency divided
> >>>> by 2.  The clock source can not be changed, but the clock divider can
> >>>> by freezing the interface and setting the CTRL register.  This appears
> >>>> to only be done by the boot loader.  I do not see why we can not leave
> >>>
> >>> And likely Freescale's bootloader does also fixup the DTS Flexcan node.
> >>> Ah, oh, there's already someting in the mainline U-BOOT:
> >>>
> >>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>
> >>>     powerpc/85xx: Fix up clock_freq property in CAN node of dts
> >>>
> >>>     Fix up the device tree property associated with the Flexcan clock
> >>>     frequency. This property is used to calculate the bit timing parameters
> >>>     for Flexcan.
> >>>
> >>>     Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>     Signed-off-by: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> >>>
> >>>
> >>>> that functionality in the boot loader and then go back to a variation
> >>>> on my earlier flexcan_clk_* patch.  Is that close to the direction you
> >>>> think we should go or have I completely misunderstood your wishes?
> >>>
> >>> The boot loader might not chose the optimum clock source and frequency,
> >>> which might even be application dependent. Therefore it would be nice to
> >>> allow the user to change it if necessary. Some CAN interfaces do even
> >>> allow to use an external clock source. The main question is where we add
> >>> that functionality. As more as I think of it, the clock interface would
> >>> not be that bad, especially if it's available.
> >>>
> >>> Furthermore, if the bootloader sets the clock source and divider, we do
> >>> not need device tree properties for it. A simply register lookup would
> >>> reveal what values are used. We may just need the input clock source.
> >>
> >> If the bootloader touches the divider _in_ the flexcan core, that would
> >> make absolutely no sense. The clock divider in the flexcan core (in the
> >> CTRL register) is the bitrate pre-scaler calculated by the bit-timing
> >> algorithm.
> >>
> >> What we need in the device tree is, from my point of view.
> >> a) the used clock source (bus clock or xtal clock)
> >> b) the frequency of that clock
> >>
> >> These problems are solved on arm via:
> >> a) bus clock is hard coded [1]
> >> b) get that clock frequency via clk_get_rate().
> > 
> > Just to make sure I understand correctly, the clk_get_rate() return
> > value comes from the device tree and a mach specific handler, right?
> > And 'mach-specific' really means what, a processor family?
> 
> I'm talking about the mainline driver, that has no device tree support.
> The clock stuff on arm currently goes like this:

What is the difference between device tree support and the clkdev based
clock sources using of_match to find a clock source for a particular
device.  It looks to me like those are filled in based upon device tree
information, but I _TRULY_ do not know what I am talking about.

> The driver asks for the clock related to the device. The architecture
> code has previously connected the flexcan device to an arch specific
> (i.mx25, i.mx35) clock. That clock is returned. Enable/disable/get_rate
> are working on that specific clock.

I will go and study that some more.  I did my cross-compile using
mxs_defconfig.  Is there a better config I should be using?  I typically
compile a kernel with the drivers I desire and then build my cscope
database using the files used in that build.

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 04:21 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>
>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>> ...
>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>> +                       "clock_freq", gd->bus_clk, 1);
>>>
>>> Should I go back to flexcan-v1.0 in my patches?
>>
>> Well, no. Let's wait. I don't think we need it. Also, it sets
>> "clock_freq" while
>>
>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>
>> documents "clock-frequencies"... :-(.
> 
> You answered a different question that I was asking.  I was asking if
> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> line 5.  The clock_freq looks like a uboot change will need to be made
> as well.

Well, I wrote above: "Well, no. Let's wait. I don't think we need it."

For the P1010 we can sinmply derive the clock frequency from
"fsl_get_sys_freq()", which is fine for the time being. No extra
properties, etc. The clk implemetation might go into

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c

or

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c

And may depend on HAVE_CAN_FLEXCAN

BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
you using?

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 04:21 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>
> >>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>> ...
> >>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>> +                       "clock_freq", gd->bus_clk, 1);
> >>>
> >>> Should I go back to flexcan-v1.0 in my patches?
> >>
> >> Well, no. Let's wait. I don't think we need it. Also, it sets
> >> "clock_freq" while
> >>
> >>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>
> >> documents "clock-frequencies"... :-(.
> > 
> > You answered a different question that I was asking.  I was asking if
> > I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> > line 5.  The clock_freq looks like a uboot change will need to be made
> > as well.
> 
> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> 
> For the P1010 we can sinmply derive the clock frequency from
> "fsl_get_sys_freq()", which is fine for the time being. No extra
> properties, etc. The clk implemetation might go into
> 
>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> 
> or
> 
>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> 
> And may depend on HAVE_CAN_FLEXCAN
> 
> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> you using?

I am starting with the v3.0 kernel, apply one patch from the freescale BSP
we receive under NDA which introduces the P1010RDB board into the QorIQ
platform, and then work from there for the flexcan stuff.  That patch
introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
that Kconfig bit, so I have tweaked it to be selected automatically
when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
selection to determine is we are going to build the flexcan.c file.

Our contact with Freescale would prefer that I not post that patch until
we get the OK from freescale to do so since we received it under NDA.

Robin
> 
> Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 04:21 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>
> >>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>> ...
> >>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>> +                       "clock_freq", gd->bus_clk, 1);
> >>>
> >>> Should I go back to flexcan-v1.0 in my patches?
> >>
> >> Well, no. Let's wait. I don't think we need it. Also, it sets
> >> "clock_freq" while
> >>
> >>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>
> >> documents "clock-frequencies"... :-(.
> > 
> > You answered a different question that I was asking.  I was asking if
> > I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> > line 5.  The clock_freq looks like a uboot change will need to be made
> > as well.
> 
> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."

My question remains "What should we be naming the device tree node in
general.  Line 5 of the fsl-flexcan.txt file specifically calls the node
"fsl,flexcan-v1.0"  In the .dts file the freescale patches introduces into
the arch/powerpc portion of the kernel, they call it that same thing.
Likewise, in the code already checked into uboot it is the same name.
Whether it is needed or not for the clock frequency, it does need to
be consistent between the .dts file and the driver for device discovery
to work.

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 04:44 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>
>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>> ...
>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>
>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>
>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>> "clock_freq" while
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>
>>>> documents "clock-frequencies"... :-(.
>>>
>>> You answered a different question that I was asking.  I was asking if
>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>> as well.
>>
>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>
>> For the P1010 we can sinmply derive the clock frequency from
>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>> properties, etc. The clk implemetation might go into
>>
>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>
>> or
>>
>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>
>> And may depend on HAVE_CAN_FLEXCAN
>>
>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>> you using?
> 
> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> we receive under NDA which introduces the P1010RDB board into the QorIQ
> platform, and then work from there for the flexcan stuff.  That patch
> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> that Kconfig bit, so I have tweaked it to be selected automatically
> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> selection to determine is we are going to build the flexcan.c file.

ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
we should do it differently for PowerPC. 

For mainline inclusion, you should provide your patches against the
David Millers "net-next-2.6" tree, which already seems to have support
for the P1010RDB:

  config P1010_RDB
        bool "Freescale P1010RDB"
        select DEFAULT_UIMAGE
        help
          This option enables support for the MPC85xx RDB (P1010 RDB) board

          P1010RDB contains P1010Si, which provides CPU performance up to 800
          MHz and 1600 DMIPS, additional functionality and faster interfaces
          (DDR3/3L, SATA II, and PCI  Express).


> Our contact with Freescale would prefer that I not post that patch until
> we get the OK from freescale to do so since we received it under NDA.

I don't think we currently need it. I prefer dropping and cleaning up
the device tree stuff as it is not needed for the P1010 anyway. If a
new processor shows up with enhanced capabilities requiring
configuration via device tree, we or somebody else can provide a patch.
Marc, what do you think?

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 04:44 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>
> >>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>> ...
> >>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>
> >>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>
> >>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>> "clock_freq" while
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>
> >>>> documents "clock-frequencies"... :-(.
> >>>
> >>> You answered a different question that I was asking.  I was asking if
> >>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>> as well.
> >>
> >> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>
> >> For the P1010 we can sinmply derive the clock frequency from
> >> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >> properties, etc. The clk implemetation might go into
> >>
> >>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>
> >> or
> >>
> >>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>
> >> And may depend on HAVE_CAN_FLEXCAN
> >>
> >> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >> you using?
> > 
> > I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> > we receive under NDA which introduces the P1010RDB board into the QorIQ
> > platform, and then work from there for the flexcan stuff.  That patch
> > introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> > that Kconfig bit, so I have tweaked it to be selected automatically
> > when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> > selection to determine is we are going to build the flexcan.c file.
> 
> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> we should do it differently for PowerPC. 
> 
> For mainline inclusion, you should provide your patches against the
> David Millers "net-next-2.6" tree, which already seems to have support
> for the P1010RDB:
> 
>   config P1010_RDB
>         bool "Freescale P1010RDB"
>         select DEFAULT_UIMAGE
>         help
>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> 
>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>           (DDR3/3L, SATA II, and PCI  Express).
> 
> 
> > Our contact with Freescale would prefer that I not post that patch until
> > we get the OK from freescale to do so since we received it under NDA.
> 
> I don't think we currently need it. I prefer dropping and cleaning up
> the device tree stuff as it is not needed for the P1010 anyway. If a
> new processor shows up with enhanced capabilities requiring
> configuration via device tree, we or somebody else can provide a patch.
> Marc, what do you think?

I will rebase shortly and provide a newer set of patches.

I do think powerpc does need the device tree support.  That is how the flexcan_probe
is getting called.  How would you suggest I do it otherwise?

Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4310 bytes --]

On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> On 08/08/2011 04:44 PM, Robin Holt wrote:
>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>
>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>> ...
>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>
>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>
>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>> "clock_freq" while
>>>>>
>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>
>>>>> documents "clock-frequencies"... :-(.
>>>>
>>>> You answered a different question that I was asking.  I was asking if
>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>> as well.
>>>
>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>
>>> For the P1010 we can sinmply derive the clock frequency from
>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>> properties, etc. The clk implemetation might go into
>>>
>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>
>>> or
>>>
>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>
>>> And may depend on HAVE_CAN_FLEXCAN
>>>
>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>> you using?
>>
>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>> platform, and then work from there for the flexcan stuff.  That patch
>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>> that Kconfig bit, so I have tweaked it to be selected automatically
>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>> selection to determine is we are going to build the flexcan.c file.
> 
> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> we should do it differently for PowerPC. 
> 
> For mainline inclusion, you should provide your patches against the
> David Millers "net-next-2.6" tree, which already seems to have support
> for the P1010RDB:
> 
>   config P1010_RDB
>         bool "Freescale P1010RDB"
>         select DEFAULT_UIMAGE
>         help
>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> 
>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>           (DDR3/3L, SATA II, and PCI  Express).
> 
> 
>> Our contact with Freescale would prefer that I not post that patch until
>> we get the OK from freescale to do so since we received it under NDA.
> 
> I don't think we currently need it. I prefer dropping and cleaning up
> the device tree stuff as it is not needed for the P1010 anyway. If a
> new processor shows up with enhanced capabilities requiring
> configuration via device tree, we or somebody else can provide a patch.
> Marc, what do you think?

ACK - The device tree bindings as in mainline's Documentation is a mess.
If the powerpc guys are happy with a clock interfaces based approach
somewhere in arch/ppc, I'm more than happy to remove:
- fsl,flexcan-clock-source (not implemented, even in the fsl driver)

- fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
- clock-frequency           /   a single clock-frequency attribute

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 04:48 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:21 PM, Robin Holt wrote:
...
>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> 
> My question remains "What should we be naming the device tree node in
> general.  Line 5 of the fsl-flexcan.txt file specifically calls the node
> "fsl,flexcan-v1.0"  In the .dts file the freescale patches introduces into
> the arch/powerpc portion of the kernel, they call it that same thing.

We should provide a patch removing that doc. The version suffix does not
follow the device tree convention. A proper compatibility string would be:

  "fsl,p1010-flexcan", "fsl,flexcan"

But as the Flexcan on the P1010 is not treated differently,
"fsl,flexcan" is just fine. Also, the v1.0 is only for the PowerPC SOCs
(ignoring ARM).

> Likewise, in the code already checked into uboot it is the same name.
> Whether it is needed or not for the clock frequency, it does need to
> be consistent between the .dts file and the driver for device discovery
> to work.

Yes, depending on what we decide we need to clean that up as well.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 05:09 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>
>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>> ...
>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>
>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>
>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>> "clock_freq" while
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>
>>>>>> documents "clock-frequencies"... :-(.
>>>>>
>>>>> You answered a different question that I was asking.  I was asking if
>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>> as well.
>>>>
>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>
>>>> For the P1010 we can sinmply derive the clock frequency from
>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>> properties, etc. The clk implemetation might go into
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>
>>>> or
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>
>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>
>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>> you using?
>>>
>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>> platform, and then work from there for the flexcan stuff.  That patch
>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>> selection to determine is we are going to build the flexcan.c file.
>>
>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>> we should do it differently for PowerPC. 
>>
>> For mainline inclusion, you should provide your patches against the
>> David Millers "net-next-2.6" tree, which already seems to have support
>> for the P1010RDB:
>>
>>   config P1010_RDB
>>         bool "Freescale P1010RDB"
>>         select DEFAULT_UIMAGE
>>         help
>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>
>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>           (DDR3/3L, SATA II, and PCI  Express).
>>
>>
>>> Our contact with Freescale would prefer that I not post that patch until
>>> we get the OK from freescale to do so since we received it under NDA.
>>
>> I don't think we currently need it. I prefer dropping and cleaning up
>> the device tree stuff as it is not needed for the P1010 anyway. If a
>> new processor shows up with enhanced capabilities requiring
>> configuration via device tree, we or somebody else can provide a patch.
>> Marc, what do you think?
> 
> I will rebase shortly and provide a newer set of patches.
> 
> I do think powerpc does need the device tree support.  That is how the flexcan_probe
> is getting called.  How would you suggest I do it otherwise?

Why do you think that?

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
> On 08/08/2011 05:09 PM, Robin Holt wrote:
>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>
>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>> ...
>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>
>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>
>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>> "clock_freq" while
>>>>>>>
>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>
>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>
>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>> as well.
>>>>>
>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>
>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>> properties, etc. The clk implemetation might go into
>>>>>
>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>
>>>>> or
>>>>>
>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>
>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>
>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>> you using?
>>>>
>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>> selection to determine is we are going to build the flexcan.c file.
>>>
>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>> we should do it differently for PowerPC. 
>>>
>>> For mainline inclusion, you should provide your patches against the
>>> David Millers "net-next-2.6" tree, which already seems to have support
>>> for the P1010RDB:
>>>
>>>   config P1010_RDB
>>>         bool "Freescale P1010RDB"
>>>         select DEFAULT_UIMAGE
>>>         help
>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>
>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>
>>>
>>>> Our contact with Freescale would prefer that I not post that patch until
>>>> we get the OK from freescale to do so since we received it under NDA.
>>>
>>> I don't think we currently need it. I prefer dropping and cleaning up
>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>> new processor shows up with enhanced capabilities requiring
>>> configuration via device tree, we or somebody else can provide a patch.
>>> Marc, what do you think?
>>
>> I will rebase shortly and provide a newer set of patches.
>>
>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>> is getting called.  How would you suggest I do it otherwise?
> 
> Why do you think that?

To be clear. I mean we do not need the extra "fsl," properties for the
clock source and divider and frequency.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1169 bytes --]

On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
>>>> Our contact with Freescale would prefer that I not post that patch until
>>>> we get the OK from freescale to do so since we received it under NDA.
>>>
>>> I don't think we currently need it. I prefer dropping and cleaning up
>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>> new processor shows up with enhanced capabilities requiring
>>> configuration via device tree, we or somebody else can provide a patch.
>>> Marc, what do you think?
>>
>> I will rebase shortly and provide a newer set of patches.
>>
>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>> is getting called.  How would you suggest I do it otherwise?

I think Wolfgang was talking about removing the clock* attributes from
the device tree, not the device tree bindings at all.

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 05:18:57PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:09 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>
> >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>> ...
> >>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>
> >>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>
> >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>> "clock_freq" while
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>
> >>>>>> documents "clock-frequencies"... :-(.
> >>>>>
> >>>>> You answered a different question that I was asking.  I was asking if
> >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>> as well.
> >>>>
> >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>
> >>>> For the P1010 we can sinmply derive the clock frequency from
> >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>> properties, etc. The clk implemetation might go into
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>
> >>>> or
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>
> >>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>
> >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>> you using?
> >>>
> >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>> platform, and then work from there for the flexcan stuff.  That patch
> >>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>> selection to determine is we are going to build the flexcan.c file.
> >>
> >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >> we should do it differently for PowerPC. 
> >>
> >> For mainline inclusion, you should provide your patches against the
> >> David Millers "net-next-2.6" tree, which already seems to have support
> >> for the P1010RDB:
> >>
> >>   config P1010_RDB
> >>         bool "Freescale P1010RDB"
> >>         select DEFAULT_UIMAGE
> >>         help
> >>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>
> >>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>           (DDR3/3L, SATA II, and PCI  Express).
> >>
> >>
> >>> Our contact with Freescale would prefer that I not post that patch until
> >>> we get the OK from freescale to do so since we received it under NDA.
> >>
> >> I don't think we currently need it. I prefer dropping and cleaning up
> >> the device tree stuff as it is not needed for the P1010 anyway. If a
> >> new processor shows up with enhanced capabilities requiring
> >> configuration via device tree, we or somebody else can provide a patch.
> >> Marc, what do you think?
> > 
> > I will rebase shortly and provide a newer set of patches.
> > 
> > I do think powerpc does need the device tree support.  That is how the flexcan_probe
> > is getting called.  How would you suggest I do it otherwise?
> 
> Why do you think that?

In patch 3/5 in this series (attached below), I made a change in how
device discovery works.  Without that of_match stuff, the flexcan
driver was never getting its flexcan_probe function called.  As soon
as I added that, it worked.  Looking at the driver_register path, this
appeared to be the "correct" way to implement the device discovery.
Did I miss something?

Thanks,
Robin


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-sJ/iWh9BUns@public.gmane.org>
To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

---
I kept the of_match for "fsl,flexcan-v1.0" for the time being.  I will
happily drop it for final submission once I have a boot loader worked
up that matches on either string.
---
 drivers/net/can/flexcan.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index ecdd4e6..d4ac81b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1028,8 +1028,22 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id flexcan_of_match[] = {
+	{
+		.compatible = "fsl,flexcan-v1.0",
+	},
+	{
+		.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

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 05:25 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:18:57PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 05:09 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>>
>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>>> ...
>>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>>
>>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>>
>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>>> "clock_freq" while
>>>>>>>>
>>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>>
>>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>>
>>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>>> as well.
>>>>>>
>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>>
>>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>>> properties, etc. The clk implemetation might go into
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>>
>>>>>> or
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>>
>>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>>
>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>>> you using?
>>>>>
>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>>> selection to determine is we are going to build the flexcan.c file.
>>>>
>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>>> we should do it differently for PowerPC. 
>>>>
>>>> For mainline inclusion, you should provide your patches against the
>>>> David Millers "net-next-2.6" tree, which already seems to have support
>>>> for the P1010RDB:
>>>>
>>>>   config P1010_RDB
>>>>         bool "Freescale P1010RDB"
>>>>         select DEFAULT_UIMAGE
>>>>         help
>>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>>
>>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>>
>>>>
>>>>> Our contact with Freescale would prefer that I not post that patch until
>>>>> we get the OK from freescale to do so since we received it under NDA.
>>>>
>>>> I don't think we currently need it. I prefer dropping and cleaning up
>>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>>> new processor shows up with enhanced capabilities requiring
>>>> configuration via device tree, we or somebody else can provide a patch.
>>>> Marc, what do you think?
>>>
>>> I will rebase shortly and provide a newer set of patches.
>>>
>>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>>> is getting called.  How would you suggest I do it otherwise?
>>
>> Why do you think that?
> 
> In patch 3/5 in this series (attached below), I made a change in how
> device discovery works.  Without that of_match stuff, the flexcan
> driver was never getting its flexcan_probe function called.  As soon
> as I added that, it worked.  Looking at the driver_register path, this
> appeared to be the "correct" way to implement the device discovery.
> Did I miss something?

I already clarified my statement. Hope you agree now.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>
>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>> ...
>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>
>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>
>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>> "clock_freq" while
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>
>>>>>> documents "clock-frequencies"... :-(.
>>>>>
>>>>> You answered a different question that I was asking.  I was asking if
>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>> as well.
>>>>
>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>
>>>> For the P1010 we can sinmply derive the clock frequency from
>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>> properties, etc. The clk implemetation might go into
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>
>>>> or
>>>>
>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>
>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>
>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>> you using?
>>>
>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>> platform, and then work from there for the flexcan stuff.  That patch
>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>> selection to determine is we are going to build the flexcan.c file.
>>
>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>> we should do it differently for PowerPC. 
>>
>> For mainline inclusion, you should provide your patches against the
>> David Millers "net-next-2.6" tree, which already seems to have support
>> for the P1010RDB:
>>
>>   config P1010_RDB
>>         bool "Freescale P1010RDB"
>>         select DEFAULT_UIMAGE
>>         help
>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>
>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>           (DDR3/3L, SATA II, and PCI  Express).
>>
>>
>>> Our contact with Freescale would prefer that I not post that patch until
>>> we get the OK from freescale to do so since we received it under NDA.
>>
>> I don't think we currently need it. I prefer dropping and cleaning up
>> the device tree stuff as it is not needed for the P1010 anyway. If a
>> new processor shows up with enhanced capabilities requiring
>> configuration via device tree, we or somebody else can provide a patch.
>> Marc, what do you think?
> 
> ACK - The device tree bindings as in mainline's Documentation is a mess.
> If the powerpc guys are happy with a clock interfaces based approach
> somewhere in arch/ppc, I'm more than happy to remove:
> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> 
> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> - clock-frequency           /   a single clock-frequency attribute

In the "net-next-2.6" tree there is also:

 $ grep flexcan arch/powerpc/boots/dts/*.dts
  p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
  p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
  p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
  p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
  p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
  p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;

Especially the fsl,flexcan-clock-divider = <2>; might make people think,
that they could set something else.

Wolfgang.



> Marc
> 
> 
> 
> 
> _______________________________________________
> Socketcan-core mailing list
> Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 05:22:41PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
> > On 08/08/2011 05:09 PM, Robin Holt wrote:
> >> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
> >>> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>>
> >>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>>> ...
> >>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>>
> >>>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>>
> >>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>>> "clock_freq" while
> >>>>>>>
> >>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>>
> >>>>>>> documents "clock-frequencies"... :-(.
> >>>>>>
> >>>>>> You answered a different question that I was asking.  I was asking if
> >>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>>> as well.
> >>>>>
> >>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>>
> >>>>> For the P1010 we can sinmply derive the clock frequency from
> >>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>>> properties, etc. The clk implemetation might go into
> >>>>>
> >>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>>
> >>>>> or
> >>>>>
> >>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>>
> >>>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>>
> >>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>>> you using?
> >>>>
> >>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>>> platform, and then work from there for the flexcan stuff.  That patch
> >>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>>> selection to determine is we are going to build the flexcan.c file.
> >>>
> >>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >>> we should do it differently for PowerPC. 
> >>>
> >>> For mainline inclusion, you should provide your patches against the
> >>> David Millers "net-next-2.6" tree, which already seems to have support
> >>> for the P1010RDB:
> >>>
> >>>   config P1010_RDB
> >>>         bool "Freescale P1010RDB"
> >>>         select DEFAULT_UIMAGE
> >>>         help
> >>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>>
> >>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>>           (DDR3/3L, SATA II, and PCI  Express).
> >>>
> >>>
> >>>> Our contact with Freescale would prefer that I not post that patch until
> >>>> we get the OK from freescale to do so since we received it under NDA.
> >>>
> >>> I don't think we currently need it. I prefer dropping and cleaning up
> >>> the device tree stuff as it is not needed for the P1010 anyway. If a
> >>> new processor shows up with enhanced capabilities requiring
> >>> configuration via device tree, we or somebody else can provide a patch.
> >>> Marc, what do you think?
> >>
> >> I will rebase shortly and provide a newer set of patches.
> >>
> >> I do think powerpc does need the device tree support.  That is how the flexcan_probe
> >> is getting called.  How would you suggest I do it otherwise?
> > 
> > Why do you think that?
> 
> To be clear. I mean we do not need the extra "fsl," properties for the
> clock source and divider and frequency.

I agree with that.  The can definition in the .dts file, however,
should be can0@... "fsl,flexcan" in an ideal world, correct?  If that
is correct, then I will make the of_match string match fsl,flexcan and
update the .dts file accordingly.

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 05:38 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:22:41PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote:
>>> On 08/08/2011 05:09 PM, Robin Holt wrote:
>>>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>>>
>>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>>>> ...
>>>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>>>
>>>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>>>
>>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>>>> "clock_freq" while
>>>>>>>>>
>>>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>>>
>>>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>>>
>>>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>>>> as well.
>>>>>>>
>>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>>>
>>>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>>>> properties, etc. The clk implemetation might go into
>>>>>>>
>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>>>
>>>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>>>
>>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>>>> you using?
>>>>>>
>>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>>>> selection to determine is we are going to build the flexcan.c file.
>>>>>
>>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>>>> we should do it differently for PowerPC. 
>>>>>
>>>>> For mainline inclusion, you should provide your patches against the
>>>>> David Millers "net-next-2.6" tree, which already seems to have support
>>>>> for the P1010RDB:
>>>>>
>>>>>   config P1010_RDB
>>>>>         bool "Freescale P1010RDB"
>>>>>         select DEFAULT_UIMAGE
>>>>>         help
>>>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>>>
>>>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>>>
>>>>>
>>>>>> Our contact with Freescale would prefer that I not post that patch until
>>>>>> we get the OK from freescale to do so since we received it under NDA.
>>>>>
>>>>> I don't think we currently need it. I prefer dropping and cleaning up
>>>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>>>> new processor shows up with enhanced capabilities requiring
>>>>> configuration via device tree, we or somebody else can provide a patch.
>>>>> Marc, what do you think?
>>>>
>>>> I will rebase shortly and provide a newer set of patches.
>>>>
>>>> I do think powerpc does need the device tree support.  That is how the flexcan_probe
>>>> is getting called.  How would you suggest I do it otherwise?
>>>
>>> Why do you think that?
>>
>> To be clear. I mean we do not need the extra "fsl," properties for the
>> clock source and divider and frequency.
> 
> I agree with that.  The can definition in the .dts file, however,
> should be can0@... "fsl,flexcan" in an ideal world, correct?  If that

No, it's normally <device-type>@<address>.

> is correct, then I will make the of_match string match fsl,flexcan and
> update the .dts file accordingly.

As I said. For the P1010 the clock get function just needs to return
"fsl_get_sys_freq()". No need to inspect the device tree. And I would
provide the clk implementation in

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c

or even:

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
...

> > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> >> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>
> >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>> ...
> >>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>
> >>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>
> >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>> "clock_freq" while
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>
> >>>>>> documents "clock-frequencies"... :-(.
> >>>>>
> >>>>> You answered a different question that I was asking.  I was asking if
> >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>> as well.
> >>>>
> >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>
> >>>> For the P1010 we can sinmply derive the clock frequency from
> >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>> properties, etc. The clk implemetation might go into
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>
> >>>> or
> >>>>
> >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>
> >>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>
> >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>> you using?
> >>>
> >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>> platform, and then work from there for the flexcan stuff.  That patch
> >>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>> selection to determine is we are going to build the flexcan.c file.
> >>
> >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >> we should do it differently for PowerPC. 
> >>
> >> For mainline inclusion, you should provide your patches against the
> >> David Millers "net-next-2.6" tree, which already seems to have support
> >> for the P1010RDB:
> >>
> >>   config P1010_RDB
> >>         bool "Freescale P1010RDB"
> >>         select DEFAULT_UIMAGE
> >>         help
> >>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>
> >>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>           (DDR3/3L, SATA II, and PCI  Express).
> >>
> >>
> >>> Our contact with Freescale would prefer that I not post that patch until
> >>> we get the OK from freescale to do so since we received it under NDA.
> >>
> >> I don't think we currently need it. I prefer dropping and cleaning up
> >> the device tree stuff as it is not needed for the P1010 anyway. If a
> >> new processor shows up with enhanced capabilities requiring
> >> configuration via device tree, we or somebody else can provide a patch.
> >> Marc, what do you think?
> > 
> > ACK - The device tree bindings as in mainline's Documentation is a mess.
> > If the powerpc guys are happy with a clock interfaces based approach
> > somewhere in arch/ppc, I'm more than happy to remove:
> > - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> > 
> > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> > - clock-frequency           /   a single clock-frequency attribute
> 
> In the "net-next-2.6" tree there is also:
> 
>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> 
> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> that they could set something else.

I am currently lost on the direction.  I think I need something like:

1) Patch 1/5 removing the "#include <mach/clock.h>" stays.
2) Patch 2/5 abstracting readl/writel stays.
3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.
4) Patch 4/5 I have not been given clear direction to not do it but have
   not gotten a favorable response.
5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c
   patch which determines the clock source not using the device tree
   information, but rather from some system register.  I need more detail
   on how this would work for both arm and powerpc.  How would I absract
   that or am I providing a flexcan_clk_* set of functions like I have
   in earlier generations of the patch set?

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

Ignore this.  We cross in the mail.  I will go back to your other thread.

Robin

On Mon, Aug 08, 2011 at 10:55:40AM -0500, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
> > On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> ...
> 
> > > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> > >> On 08/08/2011 04:44 PM, Robin Holt wrote:
> > >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> > >>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> > >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> > >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> > >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> > >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > >>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> > >>>>>>>
> > >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> > >>>>>>> ...
> > >>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> > >>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> > >>>>>>>
> > >>>>>>> Should I go back to flexcan-v1.0 in my patches?
> > >>>>>>
> > >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> > >>>>>> "clock_freq" while
> > >>>>>>
> > >>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> > >>>>>>
> > >>>>>> documents "clock-frequencies"... :-(.
> > >>>>>
> > >>>>> You answered a different question that I was asking.  I was asking if
> > >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> > >>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> > >>>>> as well.
> > >>>>
> > >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> > >>>>
> > >>>> For the P1010 we can sinmply derive the clock frequency from
> > >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> > >>>> properties, etc. The clk implemetation might go into
> > >>>>
> > >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> > >>>>
> > >>>> or
> > >>>>
> > >>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> > >>>>
> > >>>> And may depend on HAVE_CAN_FLEXCAN
> > >>>>
> > >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> > >>>> you using?
> > >>>
> > >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> > >>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> > >>> platform, and then work from there for the flexcan stuff.  That patch
> > >>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> > >>> that Kconfig bit, so I have tweaked it to be selected automatically
> > >>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> > >>> selection to determine is we are going to build the flexcan.c file.
> > >>
> > >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> > >> we should do it differently for PowerPC. 
> > >>
> > >> For mainline inclusion, you should provide your patches against the
> > >> David Millers "net-next-2.6" tree, which already seems to have support
> > >> for the P1010RDB:
> > >>
> > >>   config P1010_RDB
> > >>         bool "Freescale P1010RDB"
> > >>         select DEFAULT_UIMAGE
> > >>         help
> > >>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> > >>
> > >>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> > >>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> > >>           (DDR3/3L, SATA II, and PCI  Express).
> > >>
> > >>
> > >>> Our contact with Freescale would prefer that I not post that patch until
> > >>> we get the OK from freescale to do so since we received it under NDA.
> > >>
> > >> I don't think we currently need it. I prefer dropping and cleaning up
> > >> the device tree stuff as it is not needed for the P1010 anyway. If a
> > >> new processor shows up with enhanced capabilities requiring
> > >> configuration via device tree, we or somebody else can provide a patch.
> > >> Marc, what do you think?
> > > 
> > > ACK - The device tree bindings as in mainline's Documentation is a mess.
> > > If the powerpc guys are happy with a clock interfaces based approach
> > > somewhere in arch/ppc, I'm more than happy to remove:
> > > - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> > > 
> > > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> > > - clock-frequency           /   a single clock-frequency attribute
> > 
> > In the "net-next-2.6" tree there is also:
> > 
> >  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> > 
> > Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> > that they could set something else.
> 
> I am currently lost on the direction.  I think I need something like:
> 
> 1) Patch 1/5 removing the "#include <mach/clock.h>" stays.
> 2) Patch 2/5 abstracting readl/writel stays.
> 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.
> 4) Patch 4/5 I have not been given clear direction to not do it but have
>    not gotten a favorable response.
> 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c
>    patch which determines the clock source not using the device tree
>    information, but rather from some system register.  I need more detail
>    on how this would work for both arm and powerpc.  How would I absract
>    that or am I providing a flexcan_clk_* set of functions like I have
>    in earlier generations of the patch set?
> 
> Thanks,
> Robin
> _______________________________________________
> Socketcan-core mailing list
> Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 05:55 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
>> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> ...
> 
>>> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
>>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
>>>>>>>>>
>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
>>>>>>>>> ...
>>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
>>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
>>>>>>>>>
>>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
>>>>>>>>
>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
>>>>>>>> "clock_freq" while
>>>>>>>>
>>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>>>
>>>>>>>> documents "clock-frequencies"... :-(.
>>>>>>>
>>>>>>> You answered a different question that I was asking.  I was asking if
>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
>>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
>>>>>>> as well.
>>>>>>
>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
>>>>>>
>>>>>> For the P1010 we can sinmply derive the clock frequency from
>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
>>>>>> properties, etc. The clk implemetation might go into
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
>>>>>>
>>>>>> or
>>>>>>
>>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
>>>>>>
>>>>>> And may depend on HAVE_CAN_FLEXCAN
>>>>>>
>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
>>>>>> you using?
>>>>>
>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
>>>>> platform, and then work from there for the flexcan stuff.  That patch
>>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
>>>>> that Kconfig bit, so I have tweaked it to be selected automatically
>>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
>>>>> selection to determine is we are going to build the flexcan.c file.
>>>>
>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
>>>> we should do it differently for PowerPC. 
>>>>
>>>> For mainline inclusion, you should provide your patches against the
>>>> David Millers "net-next-2.6" tree, which already seems to have support
>>>> for the P1010RDB:
>>>>
>>>>   config P1010_RDB
>>>>         bool "Freescale P1010RDB"
>>>>         select DEFAULT_UIMAGE
>>>>         help
>>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
>>>>
>>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
>>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
>>>>           (DDR3/3L, SATA II, and PCI  Express).
>>>>
>>>>
>>>>> Our contact with Freescale would prefer that I not post that patch until
>>>>> we get the OK from freescale to do so since we received it under NDA.
>>>>
>>>> I don't think we currently need it. I prefer dropping and cleaning up
>>>> the device tree stuff as it is not needed for the P1010 anyway. If a
>>>> new processor shows up with enhanced capabilities requiring
>>>> configuration via device tree, we or somebody else can provide a patch.
>>>> Marc, what do you think?
>>>
>>> ACK - The device tree bindings as in mainline's Documentation is a mess.
>>> If the powerpc guys are happy with a clock interfaces based approach
>>> somewhere in arch/ppc, I'm more than happy to remove:
>>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
>>>
>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>> - clock-frequency           /   a single clock-frequency attribute
>>
>> In the "net-next-2.6" tree there is also:
>>
>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>
>> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
>> that they could set something else.
> 
> I am currently lost on the direction.  I think I need something like:
> 
> 1) Patch 1/5 removing the "#include <mach/clock.h>" stays.

OK.

> 2) Patch 2/5 abstracting readl/writel stays.

OK.

> 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.

Yep.

> 4) Patch 4/5 I have not been given clear direction to not do it but have
>    not gotten a favorable response.

Please drop this one for mainline.

> 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c

No, I just would prefer a more general place, e.g.:

 http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c

Furthermore you need patches to cleanup some DTS and platform files and
the Documentation.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 05:55 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote:
> >> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote:
> > ...
> > 
> >>> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote:
> >>>> On 08/08/2011 04:44 PM, Robin Holt wrote:
> >>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote:
> >>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote:
> >>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote:
> >>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote:
> >>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614
> >>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>>>>> Date:   Fri Mar 4 20:27:58 2011 +0530
> >>>>>>>>>
> >>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0"
> >>>>>>>>> ...
> >>>>>>>>> +       do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0",
> >>>>>>>>> +                       "clock_freq", gd->bus_clk, 1);
> >>>>>>>>>
> >>>>>>>>> Should I go back to flexcan-v1.0 in my patches?
> >>>>>>>>
> >>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets
> >>>>>>>> "clock_freq" while
> >>>>>>>>
> >>>>>>>>  http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>>>
> >>>>>>>> documents "clock-frequencies"... :-(.
> >>>>>>>
> >>>>>>> You answered a different question that I was asking.  I was asking if
> >>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on
> >>>>>>> line 5.  The clock_freq looks like a uboot change will need to be made
> >>>>>>> as well.
> >>>>>>
> >>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it."
> >>>>>>
> >>>>>> For the P1010 we can sinmply derive the clock frequency from
> >>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra
> >>>>>> properties, etc. The clk implemetation might go into
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> >>>>>>
> >>>>>> or
> >>>>>>
> >>>>>>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c
> >>>>>>
> >>>>>> And may depend on HAVE_CAN_FLEXCAN
> >>>>>>
> >>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are
> >>>>>> you using?
> >>>>>
> >>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP
> >>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ
> >>>>> platform, and then work from there for the flexcan stuff.  That patch
> >>>>> introduces the HAVE_CAN_FLEXCAN.  I do not like how freescale structured
> >>>>> that Kconfig bit, so I have tweaked it to be selected automatically
> >>>>> when P1010RDB, NET, and CAN are selected.  That allows the CAN_FLEXCAN
> >>>>> selection to determine is we are going to build the flexcan.c file.
> >>>>
> >>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why
> >>>> we should do it differently for PowerPC. 
> >>>>
> >>>> For mainline inclusion, you should provide your patches against the
> >>>> David Millers "net-next-2.6" tree, which already seems to have support
> >>>> for the P1010RDB:
> >>>>
> >>>>   config P1010_RDB
> >>>>         bool "Freescale P1010RDB"
> >>>>         select DEFAULT_UIMAGE
> >>>>         help
> >>>>           This option enables support for the MPC85xx RDB (P1010 RDB) board
> >>>>
> >>>>           P1010RDB contains P1010Si, which provides CPU performance up to 800
> >>>>           MHz and 1600 DMIPS, additional functionality and faster interfaces
> >>>>           (DDR3/3L, SATA II, and PCI  Express).
> >>>>
> >>>>
> >>>>> Our contact with Freescale would prefer that I not post that patch until
> >>>>> we get the OK from freescale to do so since we received it under NDA.
> >>>>
> >>>> I don't think we currently need it. I prefer dropping and cleaning up
> >>>> the device tree stuff as it is not needed for the P1010 anyway. If a
> >>>> new processor shows up with enhanced capabilities requiring
> >>>> configuration via device tree, we or somebody else can provide a patch.
> >>>> Marc, what do you think?
> >>>
> >>> ACK - The device tree bindings as in mainline's Documentation is a mess.
> >>> If the powerpc guys are happy with a clock interfaces based approach
> >>> somewhere in arch/ppc, I'm more than happy to remove:
> >>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> >>>
> >>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>> - clock-frequency           /   a single clock-frequency attribute
> >>
> >> In the "net-next-2.6" tree there is also:
> >>
> >>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>
> >> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> >> that they could set something else.
> > 
> > I am currently lost on the direction.  I think I need something like:
> > 
> > 1) Patch 1/5 removing the "#include <mach/clock.h>" stays.
> 
> OK.

Is that an Acked-by: or not?

> 
> > 2) Patch 2/5 abstracting readl/writel stays.
> 
> OK.

Is that an Acked-by: or not?

> 
> > 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays.
> 
> Yep.

Done.

> 
> > 4) Patch 4/5 I have not been given clear direction to not do it but have
> >    not gotten a favorable response.
> 
> Please drop this one for mainline.

Done.

> 
> > 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c
> 
> No, I just would prefer a more general place, e.g.:
> 
>  http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c
> 
> Furthermore you need patches to cleanup some DTS and platform files and
> the Documentation.

So we would stay with the clk_* functions.  I assume clk_get() would
return NULL, clk_get_rate() would just return fsl_get_sys_freq() and
the other functions would do nothing.  Doesn't this really polute what
clk_* functions are supposed to do?  Aren't we making flexcan dictate
a different behavior for powerpc than for the arm (and possibly other)
architectures?

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/08/2011 06:08 PM, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote:
...
> So we would stay with the clk_* functions.  I assume clk_get() would
> return NULL, clk_get_rate() would just return fsl_get_sys_freq() and
> the other functions would do nothing.  Doesn't this really polute what
> clk_* functions are supposed to do?  Aren't we making flexcan dictate
> a different behavior for powerpc than for the arm (and possibly other)
> architectures?

Well, I see it as one way to provide compatibility with the ARM port. If
the PowerPC people don't like it, we can switch to something else,
whatever they suggest.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1333 bytes --]

On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>> ACK - The device tree bindings as in mainline's Documentation is a mess.
>> If the powerpc guys are happy with a clock interfaces based approach
>> somewhere in arch/ppc, I'm more than happy to remove:
>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
>>
>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>> - clock-frequency           /   a single clock-frequency attribute
> 
> In the "net-next-2.6" tree there is also:
> 
>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> 
> Especially the fsl,flexcan-clock-divider = <2>; might make people think,
> that they could set something else.

ARGH... :D

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 08:37:37PM +0200, Wolfgang Grandegger wrote:
> On 08/08/2011 06:08 PM, Robin Holt wrote:
> > On Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote:
> ...
> > So we would stay with the clk_* functions.  I assume clk_get() would
> > return NULL, clk_get_rate() would just return fsl_get_sys_freq() and
> > the other functions would do nothing.  Doesn't this really polute what
> > clk_* functions are supposed to do?  Aren't we making flexcan dictate
> > a different behavior for powerpc than for the arm (and possibly other)
> > architectures?
> 
> Well, I see it as one way to provide compatibility with the ARM port. If
> the PowerPC people don't like it, we can switch to something else,
> whatever they suggest.

I have spent the last few hours and I think I found the communication
problem and I think it is me.

I assumed long ago we would be better off implementing a Kconfig language
which does "select PPC_CLOCK".  This assumption was in part because I
did not understand what I was doing when I started this (still don't
honestly), but I did know the freescale patch series did a select
PPC_CLOCK.

Here is my patch for introducing the p1010 clock source.  Am I finally
starting to understand your guidance?

Thanks,
Robin

------------------------------------------------------------------------

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/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 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Mon, Aug 08, 2011 at 02:14:03PM -0500, Robin Holt wrote:
> On Mon, Aug 08, 2011 at 08:37:37PM +0200, Wolfgang Grandegger wrote:
> > On 08/08/2011 06:08 PM, Robin Holt wrote:
> > > On Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote:
> > ...
> > > So we would stay with the clk_* functions.  I assume clk_get() would
> > > return NULL, clk_get_rate() would just return fsl_get_sys_freq() and
> > > the other functions would do nothing.  Doesn't this really polute what
> > > clk_* functions are supposed to do?  Aren't we making flexcan dictate
> > > a different behavior for powerpc than for the arm (and possibly other)
> > > architectures?
> > 
> > Well, I see it as one way to provide compatibility with the ARM port. If
> > the PowerPC people don't like it, we can switch to something else,
> > whatever they suggest.
> 
> I have spent the last few hours and I think I found the communication
> problem and I think it is me.
> 
> I assumed long ago we would be better off implementing a Kconfig language
> which does "select PPC_CLOCK".  This assumption was in part because I
> did not understand what I was doing when I started this (still don't
> honestly), but I did know the freescale patch series did a select
> PPC_CLOCK.
> 
> Here is my patch for introducing the p1010 clock source.  Am I finally
> starting to understand your guidance?
> 
> Thanks,
> Robin
> 

Here is _ALL_ of the patch.  Sorry about the earlier noise.

Robin

------------------------------------------------------------------------


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 5/5] [powerpc] Implement a p1010rdb clock source.

[U Bhaskar-B22300]

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Tuesday, August 09, 2011 12:23 AM
> To: Wolfgang Grandegger
> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U Bhaskar-
> B22300
> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> 
> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >> ACK - The device tree bindings as in mainline's Documentation is a
> mess.
> >> If the powerpc guys are happy with a clock interfaces based approach
> >> somewhere in arch/ppc, I'm more than happy to remove:
> >> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
[Bhaskar]I have pushed the FlexCAN series of patches, It contains the usage of all the fields posted in the FlexCAN bindings at
http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blob;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cbebdc8274  
> >>
> >> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >> - clock-frequency           /   a single clock-frequency attribute
> >
> > In the "net-next-2.6" tree there is also:
> >
> >  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
> >   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >
> > Especially the fsl,flexcan-clock-divider = <2>; might make people
> > think, that they could set something else.
>
[Bhaskar] As it is mentioned in the Flexcan bindings, the need of fsl,flexcan-clock-divider = <2>;
	    But I kept it as "2" because FlexCan clock source is the platform clock and it is CCB/2
	    If the "2" is misleading, the bindings can be changed or some text can be written to make the meaning of "2"
          Understandable , Please suggest ..  
 

 
> ARGH... :D
> 
> 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   |

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2316 bytes --]

On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>> ACK - The device tree bindings as in mainline's Documentation is a
>> mess.
>>>> If the powerpc guys are happy with a clock interfaces based approach
>>>> somewhere in arch/ppc, I'm more than happy to remove:
>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)

> [Bhaskar]I have pushed the FlexCAN series of patches, It contains the
> usage of all the fields posted in the FlexCAN bindings at 
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blob;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cbebdc8274

I've commented the patches. They are in a very bad shape. Please test
Robin's patches.

>>>>
>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>>> - clock-frequency           /   a single clock-frequency attribute
>>>
>>> In the "net-next-2.6" tree there is also:
>>>
>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>
>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
>>> think, that they could set something else.
>>
> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan
> clock source is the platform clock and it is CCB/2 If the "2" is
> misleading, the bindings can be changed or some text can be written
> to make the meaning of "2" Understandable , Please suggest ..

The clock devider is crap. Why not specify the clockrate that goes into
the flexcan core?

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

Hi Bhaskar,

On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> 
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
>> Sent: Tuesday, August 09, 2011 12:23 AM
>> To: Wolfgang Grandegger
>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U Bhaskar-
>> B22300
>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>
>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>> ACK - The device tree bindings as in mainline's Documentation is a
>> mess.
>>>> If the powerpc guys are happy with a clock interfaces based approach
>>>> somewhere in arch/ppc, I'm more than happy to remove:
>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver)
> [Bhaskar]I have pushed the FlexCAN series of patches, It contains the usage of all the fields posted in the FlexCAN bindings at
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blob;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cbebdc8274  

As Marc already pointed out, Robin already has a much more advanced
patch stack in preparation. Especially your patches do not care about
the already existing Flexcan core on the Freescale's ARM socks.

>>>>
>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>>> - clock-frequency           /   a single clock-frequency attribute
>>>
>>> In the "net-next-2.6" tree there is also:
>>>
>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>>   p1010rdb.dts:			fsl,flexcan-clock-source = "platform";
>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>
>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
>>> think, that they could set something else.
>>
> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of fsl,flexcan-clock-divider = <2>;
> 	    But I kept it as "2" because FlexCan clock source is the platform clock and it is CCB/2
> 	    If the "2" is misleading, the bindings can be changed or some text can be written to make the meaning of "2"
>           Understandable , Please suggest ..  

The clock source and frequency is fixed. Why do we need an extra
properties for that. We have panned to remove these bogus bindings from
the Linux kernel, which sneaked in *without* any review on the relevant
mailing lists (at least I have not realized any posting). We do not
think they are really needed. They just confuse the user. We also prefer
to use the compatibility string "fsl,flexcan" instead
"fsl,flexcan-v1.0". It's unusual to add a version number, which is  for
the Flexcan on the PowerPC cores only, I assume, but there will be
device tree for ARM soon. A proper compatibility string would be
"fsl,p1010-flexcan" if we really need to distinguish.

Please join the discussion on the mailing list helping to get this
driver mainline.

Thanks,

Wolfgang.

RE: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[U Bhaskar-B22300]

> -----Original Message-----
> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> Sent: Tuesday, August 09, 2011 2:03 PM
> To: U Bhaskar-B22300
> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> 
> Hi Bhaskar,
> 
> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Tuesday, August 09, 2011 12:23 AM
> >> To: Wolfgang Grandegger
> >> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U
> >> Bhaskar- B22300
> >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>
> >> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >>>> ACK - The device tree bindings as in mainline's Documentation is a
> >> mess.
> >>>> If the powerpc guys are happy with a clock interfaces based
> >>>> approach somewhere in arch/ppc, I'm more than happy to remove:
> >>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> >>>> driver)
> > [Bhaskar]I have pushed the FlexCAN series of patches, It contains the
> > usage of all the fields posted in the FlexCAN bindings at
> > http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo
> > b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f
> > 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb
> > ebdc8274
> 
> As Marc already pointed out, Robin already has a much more advanced patch
> stack in preparation. Especially your patches do not care about the
> already existing Flexcan core on the Freescale's ARM socks.
[Bhaskar] No, the patches are taking care of the existing ARM functionality.
	I have not tested on the ARM based board, but the patches are made in a 
      Manner that it should not break the ARM based functionality.
> 
> >>>>
> >>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>>> - clock-frequency           /   a single clock-frequency attribute
> >>>
> >>> In the "net-next-2.6" tree there is also:
> >>>
> >>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> "platform";
> >>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> "platform";
> >>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>
> >>> Especially the fsl,flexcan-clock-divider = <2>; might make people
> >>> think, that they could set something else.
> >>
> > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> fsl,flexcan-clock-divider = <2>;
> > 	    But I kept it as "2" because FlexCan clock source is the
> platform clock and it is CCB/2
> > 	    If the "2" is misleading, the bindings can be changed or some
> text can be written to make the meaning of "2"
> >           Understandable , Please suggest ..
> 
> The clock source and frequency is fixed. Why do we need an extra
> properties for that. We have panned to remove these bogus bindings from
> the Linux kernel, which sneaked in *without* any review on the relevant
> mailing lists (at least I have not realized any posting). We do not think
> they are really needed. They just confuse the user. We also prefer to use
> the compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". It's
> unusual to add a version number, which is  for the Flexcan on the PowerPC
> cores only, I assume, but there will be device tree for ARM soon. A
> proper compatibility string would be "fsl,p1010-flexcan" if we really
> need to distinguish.
> 
[Bhaskar] About clock source.. There can be two sources of clock for the CAN.
	Oscillator or the platform clock, but at present only platform clock is supported
	in P1010.If we remove the fsl,flexcan-clock-source property, we will lost the flexibility
	of changing the clock source ..
	   
          About clock-frequency... it is also not fixed. It depends on the platform clock which in turns
          Depends on the CCB clock. So it will be better to keep clock-frequency property which is getting fixed via u-boot.   	

	    Agreed on the discussion of changing "fsl,flexcan-v1.0" to "fsl,flexcan"
	
> Please join the discussion on the mailing list helping to get this driver
> mainline.
> 
> Thanks,
> 
> Wolfgang.

RE: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[U Bhaskar-B22300]

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Tuesday, August 09, 2011 1:43 PM
> To: U Bhaskar-B22300
> Cc: Wolfgang Grandegger; socketcan-core@lists.berlios.de;
> netdev@vger.kernel.org
> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> 
> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> >> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >>>> ACK - The device tree bindings as in mainline's Documentation is a
> >> mess.
> >>>> If the powerpc guys are happy with a clock interfaces based
> >>>> approach somewhere in arch/ppc, I'm more than happy to remove:
> >>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> >>>> driver)
> 
> > [Bhaskar]I have pushed the FlexCAN series of patches, It contains the
> > usage of all the fields posted in the FlexCAN bindings at
> > http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo
> > b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f
> > 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb
> > ebdc8274
> 
> I've commented the patches. They are in a very bad shape. Please test
> Robin's patches.
> 
> >>>>
> >>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>>> - clock-frequency           /   a single clock-frequency attribute
> >>>
> >>> In the "net-next-2.6" tree there is also:
> >>>
> >>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> "platform";
> >>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> "platform";
> >>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>
> >>> Especially the fsl,flexcan-clock-divider = <2>; might make people
> >>> think, that they could set something else.
> >>
> > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> > fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan
> > clock source is the platform clock and it is CCB/2 If the "2" is
> > misleading, the bindings can be changed or some text can be written to
> > make the meaning of "2" Understandable , Please suggest ..
> 
> The clock devider is crap. Why not specify the clockrate that goes into
> the flexcan core?
[Bhaskar] The reason why I placed the "fsl,flexcan-clock-divider" property is just because the earlier implementations
		 Of CAN also follows the same approach. Please see below the approach of mscan.
                compatible = "fsl,mpc5121-mscan";
                interrupts = <13 0x8>;
                interrupt-parent = <&ipic>;
                reg = <0x1380 0x80>;
                fsl,mscan-clock-source = "ref";
                fsl,mscan-clock-divider = <3>;
        };
	If you want we can remove the fsl,flexcan-clock-divider property. Please comment ..
	
> 
> 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   |

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/09/2011 11:34 AM, U Bhaskar-B22300 wrote:
> 
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
>> Sent: Tuesday, August 09, 2011 1:43 PM
>> To: U Bhaskar-B22300
>> Cc: Wolfgang Grandegger; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>
>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>>>> ACK - The device tree bindings as in mainline's Documentation is a
>>>> mess.
>>>>>> If the powerpc guys are happy with a clock interfaces based
>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
>>>>>> driver)
>>
>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains the
>>> usage of all the fields posted in the FlexCAN bindings at
>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo
>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f
>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb
>>> ebdc8274
>>
>> I've commented the patches. They are in a very bad shape. Please test
>> Robin's patches.
>>
>>>>>>
>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>>>>> - clock-frequency           /   a single clock-frequency attribute
>>>>>
>>>>> In the "net-next-2.6" tree there is also:
>>>>>
>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>> "platform";
>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>> "platform";
>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>>>
>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
>>>>> think, that they could set something else.
>>>>
>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
>>> fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan
>>> clock source is the platform clock and it is CCB/2 If the "2" is
>>> misleading, the bindings can be changed or some text can be written to
>>> make the meaning of "2" Understandable , Please suggest ..
>>
>> The clock devider is crap. Why not specify the clockrate that goes into
>> the flexcan core?
> [Bhaskar] The reason why I placed the "fsl,flexcan-clock-divider" property is just because the earlier implementations
> 		 Of CAN also follows the same approach. Please see below the approach of mscan.
>                 compatible = "fsl,mpc5121-mscan";
>                 interrupts = <13 0x8>;
>                 interrupt-parent = <&ipic>;
>                 reg = <0x1380 0x80>;
>                 fsl,mscan-clock-source = "ref";
>                 fsl,mscan-clock-divider = <3>;
>         };
> 	If you want we can remove the fsl,flexcan-clock-divider property. Please comment ..

For that platform the user can *change* these properties so select
another clock-source or clock-divider. This is not the case for the
P1010. Therefore these properties are not needed.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> 
> 
>> -----Original Message-----
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Tuesday, August 09, 2011 2:03 PM
>> To: U Bhaskar-B22300
>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>
>> Hi Bhaskar,
>>
>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>>>> Sent: Tuesday, August 09, 2011 12:23 AM
>>>> To: Wolfgang Grandegger
>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U
>>>> Bhaskar- B22300
>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>
>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>>>> ACK - The device tree bindings as in mainline's Documentation is a
>>>> mess.
>>>>>> If the powerpc guys are happy with a clock interfaces based
>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
>>>>>> driver)
>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains the
>>> usage of all the fields posted in the FlexCAN bindings at
>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo
>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f
>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb
>>> ebdc8274
>>
>> As Marc already pointed out, Robin already has a much more advanced patch
>> stack in preparation. Especially your patches do not care about the
>> already existing Flexcan core on the Freescale's ARM socks.
> [Bhaskar] No, the patches are taking care of the existing ARM functionality.
> 	I have not tested on the ARM based board, but the patches are made in a 
>       Manner that it should not break the ARM based functionality.
>>
>>>>>>
>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>>>>> - clock-frequency           /   a single clock-frequency attribute
>>>>>
>>>>> In the "net-next-2.6" tree there is also:
>>>>>
>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>> "platform";
>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>> "platform";
>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>>>
>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
>>>>> think, that they could set something else.
>>>>
>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
>> fsl,flexcan-clock-divider = <2>;
>>> 	    But I kept it as "2" because FlexCan clock source is the
>> platform clock and it is CCB/2
>>> 	    If the "2" is misleading, the bindings can be changed or some
>> text can be written to make the meaning of "2"
>>>           Understandable , Please suggest ..
>>
>> The clock source and frequency is fixed. Why do we need an extra
>> properties for that. We have panned to remove these bogus bindings from
>> the Linux kernel, which sneaked in *without* any review on the relevant
>> mailing lists (at least I have not realized any posting). We do not think
>> they are really needed. They just confuse the user. We also prefer to use
>> the compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". It's
>> unusual to add a version number, which is  for the Flexcan on the PowerPC
>> cores only, I assume, but there will be device tree for ARM soon. A
>> proper compatibility string would be "fsl,p1010-flexcan" if we really
>> need to distinguish.
>>
> [Bhaskar] About clock source.. There can be two sources of clock for the CAN.
> 	Oscillator or the platform clock, but at present only platform clock is supported
> 	in P1010.If we remove the fsl,flexcan-clock-source property, we will lost the flexibility
> 	of changing the clock source ..
> 	   
>           About clock-frequency... it is also not fixed. It depends on the platform clock which in turns
>           Depends on the CCB clock. So it will be better to keep clock-frequency property which is getting fixed via u-boot.   	

The frequency is fixed to CCB-frequency / 2. Will that ever change? What
can we expect from future Flexcan hardware? Will it support further
clock sources?

Wolfgang.

RE: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[U Bhaskar-B22300]

> -----Original Message-----
> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> Sent: Tuesday, August 09, 2011 4:19 PM
> To: U Bhaskar-B22300
> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> 
> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> >> Sent: Tuesday, August 09, 2011 2:03 PM
> >> To: U Bhaskar-B22300
> >> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> >> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>
> >> Hi Bhaskar,
> >>
> >> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> >>>> Sent: Tuesday, August 09, 2011 12:23 AM
> >>>> To: Wolfgang Grandegger
> >>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U
> >>>> Bhaskar- B22300
> >>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>>
> >>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >>>>>> ACK - The device tree bindings as in mainline's Documentation is
> >>>>>> a
> >>>> mess.
> >>>>>> If the powerpc guys are happy with a clock interfaces based
> >>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
> >>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> >>>>>> driver)
> >>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains
> >>> the usage of all the fields posted in the FlexCAN bindings at
> >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b
> >>> lo
> >>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72
> >>> 9f
> >>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972
> >>> cb
> >>> ebdc8274
> >>
> >> As Marc already pointed out, Robin already has a much more advanced
> >> patch stack in preparation. Especially your patches do not care about
> >> the already existing Flexcan core on the Freescale's ARM socks.
> > [Bhaskar] No, the patches are taking care of the existing ARM
> functionality.
> > 	I have not tested on the ARM based board, but the patches are made
> in a
> >       Manner that it should not break the ARM based functionality.
> >>
> >>>>>>
> >>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>>>>> - clock-frequency           /   a single clock-frequency attribute
> >>>>>
> >>>>> In the "net-next-2.6" tree there is also:
> >>>>>
> >>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >> "platform";
> >>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >> "platform";
> >>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>>>
> >>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
> >>>>> think, that they could set something else.
> >>>>
> >>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> >> fsl,flexcan-clock-divider = <2>;
> >>> 	    But I kept it as "2" because FlexCan clock source is the
> >> platform clock and it is CCB/2
> >>> 	    If the "2" is misleading, the bindings can be changed or some
> >> text can be written to make the meaning of "2"
> >>>           Understandable , Please suggest ..
> >>
> >> The clock source and frequency is fixed. Why do we need an extra
> >> properties for that. We have panned to remove these bogus bindings
> >> from the Linux kernel, which sneaked in *without* any review on the
> >> relevant mailing lists (at least I have not realized any posting). We
> >> do not think they are really needed. They just confuse the user. We
> >> also prefer to use the compatibility string "fsl,flexcan" instead
> >> "fsl,flexcan-v1.0". It's unusual to add a version number, which is
> >> for the Flexcan on the PowerPC cores only, I assume, but there will
> >> be device tree for ARM soon. A proper compatibility string would be
> >> "fsl,p1010-flexcan" if we really need to distinguish.
> >>
> > [Bhaskar] About clock source.. There can be two sources of clock for
> the CAN.
> > 	Oscillator or the platform clock, but at present only platform
> clock is supported
> > 	in P1010.If we remove the fsl,flexcan-clock-source property, we
> will lost the flexibility
> > 	of changing the clock source ..
> >
> >           About clock-frequency... it is also not fixed. It depends on
> the platform clock which in turns
> >           Depends on the CCB clock. So it will be better to keep clock-
> frequency property which is getting fixed via u-boot.
> 
> The frequency is fixed to CCB-frequency / 2. Will that ever change? What
> can we expect from future Flexcan hardware? Will it support further clock
> sources?
[Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.
	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
	appropriately
> 
> Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
> 
> 
> > -----Original Message-----
> > From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> > Sent: Tuesday, August 09, 2011 4:19 PM
> > To: U Bhaskar-B22300
> > Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> > netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> > 
> > On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> > >> Sent: Tuesday, August 09, 2011 2:03 PM
> > >> To: U Bhaskar-B22300
> > >> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> > >> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> > >>
> > >> Hi Bhaskar,
> > >>
> > >> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> > >>>> Sent: Tuesday, August 09, 2011 12:23 AM
> > >>>> To: Wolfgang Grandegger
> > >>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U
> > >>>> Bhaskar- B22300
> > >>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> > >>>>
> > >>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> > >>>>>> ACK - The device tree bindings as in mainline's Documentation is
> > >>>>>> a
> > >>>> mess.
> > >>>>>> If the powerpc guys are happy with a clock interfaces based
> > >>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
> > >>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> > >>>>>> driver)
> > >>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains
> > >>> the usage of all the fields posted in the FlexCAN bindings at
> > >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b
> > >>> lo
> > >>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72
> > >>> 9f
> > >>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972
> > >>> cb
> > >>> ebdc8274
> > >>
> > >> As Marc already pointed out, Robin already has a much more advanced
> > >> patch stack in preparation. Especially your patches do not care about
> > >> the already existing Flexcan core on the Freescale's ARM socks.
> > > [Bhaskar] No, the patches are taking care of the existing ARM
> > functionality.
> > > 	I have not tested on the ARM based board, but the patches are made
> > in a
> > >       Manner that it should not break the ARM based functionality.
> > >>
> > >>>>>>
> > >>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> > >>>>>> - clock-frequency           /   a single clock-frequency attribute
> > >>>>>
> > >>>>> In the "net-next-2.6" tree there is also:
> > >>>>>
> > >>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> > >>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> > >> "platform";
> > >>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> > >> "platform";
> > >>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> > >>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> > >>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> > >>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> > >>>>>
> > >>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
> > >>>>> think, that they could set something else.
> > >>>>
> > >>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> > >> fsl,flexcan-clock-divider = <2>;
> > >>> 	    But I kept it as "2" because FlexCan clock source is the
> > >> platform clock and it is CCB/2
> > >>> 	    If the "2" is misleading, the bindings can be changed or some
> > >> text can be written to make the meaning of "2"
> > >>>           Understandable , Please suggest ..
> > >>
> > >> The clock source and frequency is fixed. Why do we need an extra
> > >> properties for that. We have panned to remove these bogus bindings
> > >> from the Linux kernel, which sneaked in *without* any review on the
> > >> relevant mailing lists (at least I have not realized any posting). We
> > >> do not think they are really needed. They just confuse the user. We
> > >> also prefer to use the compatibility string "fsl,flexcan" instead
> > >> "fsl,flexcan-v1.0". It's unusual to add a version number, which is
> > >> for the Flexcan on the PowerPC cores only, I assume, but there will
> > >> be device tree for ARM soon. A proper compatibility string would be
> > >> "fsl,p1010-flexcan" if we really need to distinguish.
> > >>
> > > [Bhaskar] About clock source.. There can be two sources of clock for
> > the CAN.
> > > 	Oscillator or the platform clock, but at present only platform
> > clock is supported
> > > 	in P1010.If we remove the fsl,flexcan-clock-source property, we
> > will lost the flexibility
> > > 	of changing the clock source ..
> > >
> > >           About clock-frequency... it is also not fixed. It depends on
> > the platform clock which in turns
> > >           Depends on the CCB clock. So it will be better to keep clock-
> > frequency property which is getting fixed via u-boot.
> > 
> > The frequency is fixed to CCB-frequency / 2. Will that ever change? What
> > can we expect from future Flexcan hardware? Will it support further clock
> > sources?
> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
> 	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.
> 	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
> 	appropriately

Speaking of the dts file, I have left the p1010si.dtsi file with
the fsl,flexcan-v1.0 .compatible definition.  The flexcan folks
(IIRC Wolfgang) objected to that as it does not follow the standard
which should be just fsl,flexcan.

How would you like to change that?  Should I add it as part of this patch,
add another patch to the series, or let you take care of it?

Also, I assume the uboot project will need to be changed as well to
reflect the corrected name.

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1077 bytes --]

Bhaskar,

On 08/09/2011 02:41 PM, U Bhaskar-B22300 wrote:
>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What
>> can we expect from future Flexcan hardware? Will it support further clock
>> sources?
> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
> 	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.
> 	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
> 	appropriately

are you actually following the discussion about Robin's patches? Robin
has provided patch that work without the clock frequency in the DT at all..

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

RE: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[U Bhaskar-B22300]

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: Tuesday, August 09, 2011 6:21 PM
> To: U Bhaskar-B22300
> Cc: Wolfgang Grandegger; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> 
> Bhaskar,
> 
> On 08/09/2011 02:41 PM, U Bhaskar-B22300 wrote:
> >> The frequency is fixed to CCB-frequency / 2. Will that ever change?
> >> What can we expect from future Flexcan hardware? Will it support
> >> further clock sources?
> > [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the
> CCB gets changed that will be taken care by the u-boot fixup code for
> > 	clock-frequency. clock-frequency is not filled by somebody in the
> dts file. It will be done by u-boot.
> > 	For clock source,I can't say right now, that's why I have kept a
> property for this in the can node. So that in future, we need to fill it
> > 	appropriately
> 
> are you actually following the discussion about Robin's patches? Robin
> has provided patch that work without the clock frequency in the DT at
> all..
[Bhaskar] Hi Marc,
	Yes I am following the discussion on Robin's patch. Just going tru the patches. I will respond
> 
> 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   |

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/09/2011 02:49 PM, Robin Holt wrote:
> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
>>
>>
>>> -----Original Message-----
>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>>> Sent: Tuesday, August 09, 2011 4:19 PM
>>> To: U Bhaskar-B22300
>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>
>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>>>>> Sent: Tuesday, August 09, 2011 2:03 PM
>>>>> To: U Bhaskar-B22300
>>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>>
>>>>> Hi Bhaskar,
>>>>>
>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
>>>>>>> To: Wolfgang Grandegger
>>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U
>>>>>>> Bhaskar- B22300
>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>>>>
>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>>>>>>> ACK - The device tree bindings as in mainline's Documentation is
>>>>>>>>> a
>>>>>>> mess.
>>>>>>>>> If the powerpc guys are happy with a clock interfaces based
>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
>>>>>>>>> driver)
>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains
>>>>>> the usage of all the fields posted in the FlexCAN bindings at
>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b
>>>>>> lo
>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72
>>>>>> 9f
>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972
>>>>>> cb
>>>>>> ebdc8274
>>>>>
>>>>> As Marc already pointed out, Robin already has a much more advanced
>>>>> patch stack in preparation. Especially your patches do not care about
>>>>> the already existing Flexcan core on the Freescale's ARM socks.
>>>> [Bhaskar] No, the patches are taking care of the existing ARM
>>> functionality.
>>>> 	I have not tested on the ARM based board, but the patches are made
>>> in a
>>>>       Manner that it should not break the ARM based functionality.
>>>>>
>>>>>>>>>
>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
>>>>>>>>> - clock-frequency           /   a single clock-frequency attribute
>>>>>>>>
>>>>>>>> In the "net-next-2.6" tree there is also:
>>>>>>>>
>>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>>>>> "platform";
>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>>>>> "platform";
>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
>>>>>>>>
>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
>>>>>>>> think, that they could set something else.
>>>>>>>
>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
>>>>> fsl,flexcan-clock-divider = <2>;
>>>>>> 	    But I kept it as "2" because FlexCan clock source is the
>>>>> platform clock and it is CCB/2
>>>>>> 	    If the "2" is misleading, the bindings can be changed or some
>>>>> text can be written to make the meaning of "2"
>>>>>>           Understandable , Please suggest ..
>>>>>
>>>>> The clock source and frequency is fixed. Why do we need an extra
>>>>> properties for that. We have panned to remove these bogus bindings
>>>>> from the Linux kernel, which sneaked in *without* any review on the
>>>>> relevant mailing lists (at least I have not realized any posting). We
>>>>> do not think they are really needed. They just confuse the user. We
>>>>> also prefer to use the compatibility string "fsl,flexcan" instead
>>>>> "fsl,flexcan-v1.0". It's unusual to add a version number, which is
>>>>> for the Flexcan on the PowerPC cores only, I assume, but there will
>>>>> be device tree for ARM soon. A proper compatibility string would be
>>>>> "fsl,p1010-flexcan" if we really need to distinguish.
>>>>>
>>>> [Bhaskar] About clock source.. There can be two sources of clock for
>>> the CAN.
>>>> 	Oscillator or the platform clock, but at present only platform
>>> clock is supported
>>>> 	in P1010.If we remove the fsl,flexcan-clock-source property, we
>>> will lost the flexibility
>>>> 	of changing the clock source ..
>>>>
>>>>           About clock-frequency... it is also not fixed. It depends on
>>> the platform clock which in turns
>>>>           Depends on the CCB clock. So it will be better to keep clock-
>>> frequency property which is getting fixed via u-boot.
>>>
>>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What
>>> can we expect from future Flexcan hardware? Will it support further clock
>>> sources?
>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
>> 	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.
>> 	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
>> 	appropriately
> 
> Speaking of the dts file, I have left the p1010si.dtsi file with
> the fsl,flexcan-v1.0 .compatible definition.  The flexcan folks
> (IIRC Wolfgang) objected to that as it does not follow the standard
> which should be just fsl,flexcan.
> 
> How would you like to change that?  Should I add it as part of this patch,
> add another patch to the series, or let you take care of it?
> 
> Also, I assume the uboot project will need to be changed as well to
> reflect the corrected name.

I think you should provide patches within this series to cleanup the
obsolete stuff, dts and binding doc.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
> On 08/09/2011 02:49 PM, Robin Holt wrote:
> > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> >>> Sent: Tuesday, August 09, 2011 4:19 PM
> >>> To: U Bhaskar-B22300
> >>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
> >>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
> >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>
> >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> >>>>> Sent: Tuesday, August 09, 2011 2:03 PM
> >>>>> To: U Bhaskar-B22300
> >>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
> >>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
> >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>>>
> >>>>> Hi Bhaskar,
> >>>>>
> >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
> >>>>>>> To: Wolfgang Grandegger
> >>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U
> >>>>>>> Bhaskar- B22300
> >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>>>>>
> >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >>>>>>>>> ACK - The device tree bindings as in mainline's Documentation is
> >>>>>>>>> a
> >>>>>>> mess.
> >>>>>>>>> If the powerpc guys are happy with a clock interfaces based
> >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
> >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> >>>>>>>>> driver)
> >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains
> >>>>>> the usage of all the fields posted in the FlexCAN bindings at
> >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b
> >>>>>> lo
> >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72
> >>>>>> 9f
> >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972
> >>>>>> cb
> >>>>>> ebdc8274
> >>>>>
> >>>>> As Marc already pointed out, Robin already has a much more advanced
> >>>>> patch stack in preparation. Especially your patches do not care about
> >>>>> the already existing Flexcan core on the Freescale's ARM socks.
> >>>> [Bhaskar] No, the patches are taking care of the existing ARM
> >>> functionality.
> >>>> 	I have not tested on the ARM based board, but the patches are made
> >>> in a
> >>>>       Manner that it should not break the ARM based functionality.
> >>>>>
> >>>>>>>>>
> >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>>>>>>>> - clock-frequency           /   a single clock-frequency attribute
> >>>>>>>>
> >>>>>>>> In the "net-next-2.6" tree there is also:
> >>>>>>>>
> >>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >>>>> "platform";
> >>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >>>>> "platform";
> >>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>>>>>>
> >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
> >>>>>>>> think, that they could set something else.
> >>>>>>>
> >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> >>>>> fsl,flexcan-clock-divider = <2>;
> >>>>>> 	    But I kept it as "2" because FlexCan clock source is the
> >>>>> platform clock and it is CCB/2
> >>>>>> 	    If the "2" is misleading, the bindings can be changed or some
> >>>>> text can be written to make the meaning of "2"
> >>>>>>           Understandable , Please suggest ..
> >>>>>
> >>>>> The clock source and frequency is fixed. Why do we need an extra
> >>>>> properties for that. We have panned to remove these bogus bindings
> >>>>> from the Linux kernel, which sneaked in *without* any review on the
> >>>>> relevant mailing lists (at least I have not realized any posting). We
> >>>>> do not think they are really needed. They just confuse the user. We
> >>>>> also prefer to use the compatibility string "fsl,flexcan" instead
> >>>>> "fsl,flexcan-v1.0". It's unusual to add a version number, which is
> >>>>> for the Flexcan on the PowerPC cores only, I assume, but there will
> >>>>> be device tree for ARM soon. A proper compatibility string would be
> >>>>> "fsl,p1010-flexcan" if we really need to distinguish.
> >>>>>
> >>>> [Bhaskar] About clock source.. There can be two sources of clock for
> >>> the CAN.
> >>>> 	Oscillator or the platform clock, but at present only platform
> >>> clock is supported
> >>>> 	in P1010.If we remove the fsl,flexcan-clock-source property, we
> >>> will lost the flexibility
> >>>> 	of changing the clock source ..
> >>>>
> >>>>           About clock-frequency... it is also not fixed. It depends on
> >>> the platform clock which in turns
> >>>>           Depends on the CCB clock. So it will be better to keep clock-
> >>> frequency property which is getting fixed via u-boot.
> >>>
> >>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What
> >>> can we expect from future Flexcan hardware? Will it support further clock
> >>> sources?
> >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
> >> 	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.
> >> 	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
> >> 	appropriately
> > 
> > Speaking of the dts file, I have left the p1010si.dtsi file with
> > the fsl,flexcan-v1.0 .compatible definition.  The flexcan folks
> > (IIRC Wolfgang) objected to that as it does not follow the standard
> > which should be just fsl,flexcan.
> > 
> > How would you like to change that?  Should I add it as part of this patch,
> > add another patch to the series, or let you take care of it?
> > 
> > Also, I assume the uboot project will need to be changed as well to
> > reflect the corrected name.
> 
> I think you should provide patches within this series to cleanup the
> obsolete stuff, dts and binding doc.

Will do.

Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/09/2011 02:41 PM, U Bhaskar-B22300 wrote:
> 
> 
>> -----Original Message-----
>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
...
>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What
>> can we expect from future Flexcan hardware? Will it support further clock
>> sources?
> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
> 	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.

U-Boot fills in "clock_freq", anyway...

> 	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
> 	appropriately

This just confirms that we currently can live without clock-frequency,
clock-source and clock-divider properties. If it's really required
sometime in the future, we can add it, I think.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
> On 08/09/2011 02:49 PM, Robin Holt wrote:
> > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> >>> Sent: Tuesday, August 09, 2011 4:19 PM
> >>> To: U Bhaskar-B22300
> >>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> >>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>
> >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> >>>>> Sent: Tuesday, August 09, 2011 2:03 PM
> >>>>> To: U Bhaskar-B22300
> >>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> >>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>>>
> >>>>> Hi Bhaskar,
> >>>>>
> >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
> >>>>>>> To: Wolfgang Grandegger
> >>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U
> >>>>>>> Bhaskar- B22300
> >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>>>>>
> >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >>>>>>>>> ACK - The device tree bindings as in mainline's Documentation is
> >>>>>>>>> a
> >>>>>>> mess.
> >>>>>>>>> If the powerpc guys are happy with a clock interfaces based
> >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove:
> >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> >>>>>>>>> driver)
> >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains
> >>>>>> the usage of all the fields posted in the FlexCAN bindings at
> >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b
> >>>>>> lo
> >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72
> >>>>>> 9f
> >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972
> >>>>>> cb
> >>>>>> ebdc8274
> >>>>>
> >>>>> As Marc already pointed out, Robin already has a much more advanced
> >>>>> patch stack in preparation. Especially your patches do not care about
> >>>>> the already existing Flexcan core on the Freescale's ARM socks.
> >>>> [Bhaskar] No, the patches are taking care of the existing ARM
> >>> functionality.
> >>>> 	I have not tested on the ARM based board, but the patches are made
> >>> in a
> >>>>       Manner that it should not break the ARM based functionality.
> >>>>>
> >>>>>>>>>
> >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or
> >>>>>>>>> - clock-frequency           /   a single clock-frequency attribute
> >>>>>>>>
> >>>>>>>> In the "net-next-2.6" tree there is also:
> >>>>>>>>
> >>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >>>>> "platform";
> >>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >>>>> "platform";
> >>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-v1.0";
> >>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider = <2>;
> >>>>>>>>
> >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people
> >>>>>>>> think, that they could set something else.
> >>>>>>>
> >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of
> >>>>> fsl,flexcan-clock-divider = <2>;
> >>>>>> 	    But I kept it as "2" because FlexCan clock source is the
> >>>>> platform clock and it is CCB/2
> >>>>>> 	    If the "2" is misleading, the bindings can be changed or some
> >>>>> text can be written to make the meaning of "2"
> >>>>>>           Understandable , Please suggest ..
> >>>>>
> >>>>> The clock source and frequency is fixed. Why do we need an extra
> >>>>> properties for that. We have panned to remove these bogus bindings
> >>>>> from the Linux kernel, which sneaked in *without* any review on the
> >>>>> relevant mailing lists (at least I have not realized any posting). We
> >>>>> do not think they are really needed. They just confuse the user. We
> >>>>> also prefer to use the compatibility string "fsl,flexcan" instead
> >>>>> "fsl,flexcan-v1.0". It's unusual to add a version number, which is
> >>>>> for the Flexcan on the PowerPC cores only, I assume, but there will
> >>>>> be device tree for ARM soon. A proper compatibility string would be
> >>>>> "fsl,p1010-flexcan" if we really need to distinguish.
> >>>>>
> >>>> [Bhaskar] About clock source.. There can be two sources of clock for
> >>> the CAN.
> >>>> 	Oscillator or the platform clock, but at present only platform
> >>> clock is supported
> >>>> 	in P1010.If we remove the fsl,flexcan-clock-source property, we
> >>> will lost the flexibility
> >>>> 	of changing the clock source ..
> >>>>
> >>>>           About clock-frequency... it is also not fixed. It depends on
> >>> the platform clock which in turns
> >>>>           Depends on the CCB clock. So it will be better to keep clock-
> >>> frequency property which is getting fixed via u-boot.
> >>>
> >>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What
> >>> can we expect from future Flexcan hardware? Will it support further clock
> >>> sources?
> >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for
> >> 	clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot.
> >> 	For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it 
> >> 	appropriately
> > 
> > Speaking of the dts file, I have left the p1010si.dtsi file with
> > the fsl,flexcan-v1.0 .compatible definition.  The flexcan folks
> > (IIRC Wolfgang) objected to that as it does not follow the standard
> > which should be just fsl,flexcan.
> > 
> > How would you like to change that?  Should I add it as part of this patch,
> > add another patch to the series, or let you take care of it?
> > 
> > Also, I assume the uboot project will need to be changed as well to
> > reflect the corrected name.
> 
> I think you should provide patches within this series to cleanup the
> obsolete stuff, dts and binding doc.

It reads to me that the binding doc now reduces just the required
properties.  Should I remove the file entirely?

Robin

RE: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[U Bhaskar-B22300]

> -----Original Message-----
> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
> Sent: Tuesday, August 09, 2011 7:06 PM
> To: Wolfgang Grandegger
> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; Marc Kleine-
> Budde
> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> 
> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
> > On 08/09/2011 02:49 PM, Robin Holt wrote:
> > > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
> > >>
> > >>
> > >>> -----Original Message-----
> > >>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> > >>> Sent: Tuesday, August 09, 2011 4:19 PM
> > >>> To: U Bhaskar-B22300
> > >>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> > >>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> > >>>
> > >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> > >>>>
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
> > >>>>> Sent: Tuesday, August 09, 2011 2:03 PM
> > >>>>> To: U Bhaskar-B22300
> > >>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> > >>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
> source.
> > >>>>>
> > >>>>> Hi Bhaskar,
> > >>>>>
> > >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>> -----Original Message-----
> > >>>>>>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> > >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
> > >>>>>>> To: Wolfgang Grandegger
> > >>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U
> > >>>>>>> Bhaskar- B22300
> > >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
> source.
> > >>>>>>>
> > >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> > >>>>>>>>> ACK - The device tree bindings as in mainline's
> > >>>>>>>>> Documentation is a
> > >>>>>>> mess.
> > >>>>>>>>> If the powerpc guys are happy with a clock interfaces based
> > >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to
> remove:
> > >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> > >>>>>>>>> driver)
> > >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It
> > >>>>>> contains the usage of all the fields posted in the FlexCAN
> > >>>>>> bindings at
> > >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi
> > >>>>>> t;a=b
> > >>>>>> lo
> > >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h
> > >>>>>> =1a72
> > >>>>>> 9f
> > >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85
> > >>>>>> 16972
> > >>>>>> cb
> > >>>>>> ebdc8274
> > >>>>>
> > >>>>> As Marc already pointed out, Robin already has a much more
> > >>>>> advanced patch stack in preparation. Especially your patches do
> > >>>>> not care about the already existing Flexcan core on the
> Freescale's ARM socks.
> > >>>> [Bhaskar] No, the patches are taking care of the existing ARM
> > >>> functionality.
> > >>>> 	I have not tested on the ARM based board, but the patches are
> > >>>> made
> > >>> in a
> > >>>>       Manner that it should not break the ARM based functionality.
> > >>>>>
> > >>>>>>>>>
> > >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in
> arch/ppc, or
> > >>>>>>>>> - clock-frequency           /   a single clock-frequency
> attribute
> > >>>>>>>>
> > >>>>>>>> In the "net-next-2.6" tree there is also:
> > >>>>>>>>
> > >>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> > >>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> > >>>>> "platform";
> > >>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> > >>>>> "platform";
> > >>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
> v1.0";
> > >>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
> <2>;
> > >>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
> v1.0";
> > >>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
> <2>;
> > >>>>>>>>
> > >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make
> > >>>>>>>> people think, that they could set something else.
> > >>>>>>>
> > >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need
> > >>>>>> of
> > >>>>> fsl,flexcan-clock-divider = <2>;
> > >>>>>> 	    But I kept it as "2" because FlexCan clock source is the
> > >>>>> platform clock and it is CCB/2
> > >>>>>> 	    If the "2" is misleading, the bindings can be changed or
> > >>>>>> some
> > >>>>> text can be written to make the meaning of "2"
> > >>>>>>           Understandable , Please suggest ..
> > >>>>>
> > >>>>> The clock source and frequency is fixed. Why do we need an extra
> > >>>>> properties for that. We have panned to remove these bogus
> > >>>>> bindings from the Linux kernel, which sneaked in *without* any
> > >>>>> review on the relevant mailing lists (at least I have not
> > >>>>> realized any posting). We do not think they are really needed.
> > >>>>> They just confuse the user. We also prefer to use the
> > >>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0".
> > >>>>> It's unusual to add a version number, which is for the Flexcan
> > >>>>> on the PowerPC cores only, I assume, but there will be device
> > >>>>> tree for ARM soon. A proper compatibility string would be
> "fsl,p1010-flexcan" if we really need to distinguish.
> > >>>>>
> > >>>> [Bhaskar] About clock source.. There can be two sources of clock
> > >>>> for
> > >>> the CAN.
> > >>>> 	Oscillator or the platform clock, but at present only
> platform
> > >>> clock is supported
> > >>>> 	in P1010.If we remove the fsl,flexcan-clock-source property,
> we
> > >>> will lost the flexibility
> > >>>> 	of changing the clock source ..
> > >>>>
> > >>>>           About clock-frequency... it is also not fixed. It
> > >>>> depends on
> > >>> the platform clock which in turns
> > >>>>           Depends on the CCB clock. So it will be better to keep
> > >>>> clock-
> > >>> frequency property which is getting fixed via u-boot.
> > >>>
> > >>> The frequency is fixed to CCB-frequency / 2. Will that ever
> > >>> change? What can we expect from future Flexcan hardware? Will it
> > >>> support further clock sources?
> > >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if
> the CCB gets changed that will be taken care by the u-boot fixup code for
> > >> 	clock-frequency. clock-frequency is not filled by somebody in the
> dts file. It will be done by u-boot.
> > >> 	For clock source,I can't say right now, that's why I have kept a
> property for this in the can node. So that in future, we need to fill it
> > >> 	appropriately
> > >
> > > Speaking of the dts file, I have left the p1010si.dtsi file with the
> > > fsl,flexcan-v1.0 .compatible definition.  The flexcan folks (IIRC
> > > Wolfgang) objected to that as it does not follow the standard which
> > > should be just fsl,flexcan.
> > >
> > > How would you like to change that?  Should I add it as part of this
> > > patch, add another patch to the series, or let you take care of it?
> > >
> > > Also, I assume the uboot project will need to be changed as well to
> > > reflect the corrected name.
> >
> > I think you should provide patches within this series to cleanup the
> > obsolete stuff, dts and binding doc.
> 
> It reads to me that the binding doc now reduces just the required
> properties.  Should I remove the file entirely?
[Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity
 	    about the CAN functionality
		can0@1c000 {
                 compatible = "fsl,flexcan";
                 reg = <0x1c000 0x1000>;
                 interrupts = <48 0x2>;
                 interrupt-parent = <&mpic>;
                 clock-frequency = <fixed by u-boot>;
         	}; 
> 
> Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Tue, Aug 09, 2011 at 01:44:16PM +0000, U Bhaskar-B22300 wrote:
> 
> 
> > -----Original Message-----
> > From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
> > Sent: Tuesday, August 09, 2011 7:06 PM
> > To: Wolfgang Grandegger
> > Cc: Robin Holt; U Bhaskar-B22300; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
> > netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; Marc Kleine-
> > Budde
> > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> > 
> > On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
> > > On 08/09/2011 02:49 PM, Robin Holt wrote:
...
> > > > How would you like to change that?  Should I add it as part of this
> > > > patch, add another patch to the series, or let you take care of it?
> > > >
> > > > Also, I assume the uboot project will need to be changed as well to
> > > > reflect the corrected name.
> > >
> > > I think you should provide patches within this series to cleanup the
> > > obsolete stuff, dts and binding doc.
> > 
> > It reads to me that the binding doc now reduces just the required
> > properties.  Should I remove the file entirely?
> [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity
>  	    about the CAN functionality
> 		can0@1c000 {
>                  compatible = "fsl,flexcan";
>                  reg = <0x1c000 0x1000>;
>                  interrupts = <48 0x2>;
>                  interrupt-parent = <&mpic>;
>                  clock-frequency = <fixed by u-boot>;
>          	}; 

I am not sure what clarity we get for it.  Here it is as a work in progress:


CAN Device Tree Bindings
------------------------
2011 Freescale Semiconductor, Inc.

fsl,flexcan nodes
-----------------------
Only the required compatible-, reg- and interrupt-properties are supported.

Examples:
	can0@1c000 {
		compatible = "fsl,flexcan";
		reg = <0x1c000 0x1000>;
		interrupts = <48 0x2>;
		interrupt-parent = <&mpic>;
	};

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote:
> 
> 
>> -----Original Message-----
>> From: Robin Holt [mailto:holt@sgi.com]
>> Sent: Tuesday, August 09, 2011 7:06 PM
>> To: Wolfgang Grandegger
>> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de;
>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine-
>> Budde
>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>
>> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
>>> On 08/09/2011 02:49 PM, Robin Holt wrote:
>>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>>>>>> Sent: Tuesday, August 09, 2011 4:19 PM
>>>>>> To: U Bhaskar-B22300
>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>>>
>>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM
>>>>>>>> To: U Bhaskar-B22300
>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
>>>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
>> source.
>>>>>>>>
>>>>>>>> Hi Bhaskar,
>>>>>>>>
>>>>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
>>>>>>>>>> To: Wolfgang Grandegger
>>>>>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U
>>>>>>>>>> Bhaskar- B22300
>>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
>> source.
>>>>>>>>>>
>>>>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>>>>>>>>>> ACK - The device tree bindings as in mainline's
>>>>>>>>>>>> Documentation is a
>>>>>>>>>> mess.
>>>>>>>>>>>> If the powerpc guys are happy with a clock interfaces based
>>>>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to
>> remove:
>>>>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
>>>>>>>>>>>> driver)
>>>>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It
>>>>>>>>> contains the usage of all the fields posted in the FlexCAN
>>>>>>>>> bindings at
>>>>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi
>>>>>>>>> t;a=b
>>>>>>>>> lo
>>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h
>>>>>>>>> =1a72
>>>>>>>>> 9f
>>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85
>>>>>>>>> 16972
>>>>>>>>> cb
>>>>>>>>> ebdc8274
>>>>>>>>
>>>>>>>> As Marc already pointed out, Robin already has a much more
>>>>>>>> advanced patch stack in preparation. Especially your patches do
>>>>>>>> not care about the already existing Flexcan core on the
>> Freescale's ARM socks.
>>>>>>> [Bhaskar] No, the patches are taking care of the existing ARM
>>>>>> functionality.
>>>>>>> 	I have not tested on the ARM based board, but the patches are
>>>>>>> made
>>>>>> in a
>>>>>>>       Manner that it should not break the ARM based functionality.
>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in
>> arch/ppc, or
>>>>>>>>>>>> - clock-frequency           /   a single clock-frequency
>> attribute
>>>>>>>>>>>
>>>>>>>>>>> In the "net-next-2.6" tree there is also:
>>>>>>>>>>>
>>>>>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>>>>>>>> "platform";
>>>>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>>>>>>>> "platform";
>>>>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
>> v1.0";
>>>>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
>> <2>;
>>>>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
>> v1.0";
>>>>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
>> <2>;
>>>>>>>>>>>
>>>>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make
>>>>>>>>>>> people think, that they could set something else.
>>>>>>>>>>
>>>>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need
>>>>>>>>> of
>>>>>>>> fsl,flexcan-clock-divider = <2>;
>>>>>>>>> 	    But I kept it as "2" because FlexCan clock source is the
>>>>>>>> platform clock and it is CCB/2
>>>>>>>>> 	    If the "2" is misleading, the bindings can be changed or
>>>>>>>>> some
>>>>>>>> text can be written to make the meaning of "2"
>>>>>>>>>           Understandable , Please suggest ..
>>>>>>>>
>>>>>>>> The clock source and frequency is fixed. Why do we need an extra
>>>>>>>> properties for that. We have panned to remove these bogus
>>>>>>>> bindings from the Linux kernel, which sneaked in *without* any
>>>>>>>> review on the relevant mailing lists (at least I have not
>>>>>>>> realized any posting). We do not think they are really needed.
>>>>>>>> They just confuse the user. We also prefer to use the
>>>>>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0".
>>>>>>>> It's unusual to add a version number, which is for the Flexcan
>>>>>>>> on the PowerPC cores only, I assume, but there will be device
>>>>>>>> tree for ARM soon. A proper compatibility string would be
>> "fsl,p1010-flexcan" if we really need to distinguish.
>>>>>>>>
>>>>>>> [Bhaskar] About clock source.. There can be two sources of clock
>>>>>>> for
>>>>>> the CAN.
>>>>>>> 	Oscillator or the platform clock, but at present only
>> platform
>>>>>> clock is supported
>>>>>>> 	in P1010.If we remove the fsl,flexcan-clock-source property,
>> we
>>>>>> will lost the flexibility
>>>>>>> 	of changing the clock source ..
>>>>>>>
>>>>>>>           About clock-frequency... it is also not fixed. It
>>>>>>> depends on
>>>>>> the platform clock which in turns
>>>>>>>           Depends on the CCB clock. So it will be better to keep
>>>>>>> clock-
>>>>>> frequency property which is getting fixed via u-boot.
>>>>>>
>>>>>> The frequency is fixed to CCB-frequency / 2. Will that ever
>>>>>> change? What can we expect from future Flexcan hardware? Will it
>>>>>> support further clock sources?
>>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if
>> the CCB gets changed that will be taken care by the u-boot fixup code for
>>>>> 	clock-frequency. clock-frequency is not filled by somebody in the
>> dts file. It will be done by u-boot.
>>>>> 	For clock source,I can't say right now, that's why I have kept a
>> property for this in the can node. So that in future, we need to fill it
>>>>> 	appropriately
>>>>
>>>> Speaking of the dts file, I have left the p1010si.dtsi file with the
>>>> fsl,flexcan-v1.0 .compatible definition.  The flexcan folks (IIRC
>>>> Wolfgang) objected to that as it does not follow the standard which
>>>> should be just fsl,flexcan.
>>>>
>>>> How would you like to change that?  Should I add it as part of this
>>>> patch, add another patch to the series, or let you take care of it?
>>>>
>>>> Also, I assume the uboot project will need to be changed as well to
>>>> reflect the corrected name.
>>>
>>> I think you should provide patches within this series to cleanup the
>>> obsolete stuff, dts and binding doc.
>>
>> It reads to me that the binding doc now reduces just the required
>> properties.  Should I remove the file entirely?
> [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity
>  	    about the CAN functionality
> 		can0@1c000 {
>                  compatible = "fsl,flexcan";
>                  reg = <0x1c000 0x1000>;
>                  interrupts = <48 0x2>;
>                  interrupt-parent = <&mpic>;
>                  clock-frequency = <fixed by u-boot>;
>          	}; 

Yes, I also find the introduction is quite useful, with some related
correction.

Wolfgang.

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Robin Holt]

On Tue, Aug 09, 2011 at 04:03:38PM +0200, Wolfgang Grandegger wrote:
> On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Robin Holt [mailto:holt@sgi.com]
> >> Sent: Tuesday, August 09, 2011 7:06 PM
> >> To: Wolfgang Grandegger
> >> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de;
> >> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine-
> >> Budde
> >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>
> >> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
> >>> On 08/09/2011 02:49 PM, Robin Holt wrote:
> >>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> >>>>>> Sent: Tuesday, August 09, 2011 4:19 PM
> >>>>>> To: U Bhaskar-B22300
> >>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
> >>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
> >>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
> >>>>>>
> >>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> >>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM
> >>>>>>>> To: U Bhaskar-B22300
> >>>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de;
> >>>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org
> >>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
> >> source.
> >>>>>>>>
> >>>>>>>> Hi Bhaskar,
> >>>>>>>>
> >>>>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
> >>>>>>>>>> To: Wolfgang Grandegger
> >>>>>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U
> >>>>>>>>>> Bhaskar- B22300
> >>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
> >> source.
> >>>>>>>>>>
> >>>>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
> >>>>>>>>>>>> ACK - The device tree bindings as in mainline's
> >>>>>>>>>>>> Documentation is a
> >>>>>>>>>> mess.
> >>>>>>>>>>>> If the powerpc guys are happy with a clock interfaces based
> >>>>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to
> >> remove:
> >>>>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
> >>>>>>>>>>>> driver)
> >>>>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It
> >>>>>>>>> contains the usage of all the fields posted in the FlexCAN
> >>>>>>>>> bindings at
> >>>>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi
> >>>>>>>>> t;a=b
> >>>>>>>>> lo
> >>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h
> >>>>>>>>> =1a72
> >>>>>>>>> 9f
> >>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85
> >>>>>>>>> 16972
> >>>>>>>>> cb
> >>>>>>>>> ebdc8274
> >>>>>>>>
> >>>>>>>> As Marc already pointed out, Robin already has a much more
> >>>>>>>> advanced patch stack in preparation. Especially your patches do
> >>>>>>>> not care about the already existing Flexcan core on the
> >> Freescale's ARM socks.
> >>>>>>> [Bhaskar] No, the patches are taking care of the existing ARM
> >>>>>> functionality.
> >>>>>>> 	I have not tested on the ARM based board, but the patches are
> >>>>>>> made
> >>>>>> in a
> >>>>>>>       Manner that it should not break the ARM based functionality.
> >>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in
> >> arch/ppc, or
> >>>>>>>>>>>> - clock-frequency           /   a single clock-frequency
> >> attribute
> >>>>>>>>>>>
> >>>>>>>>>>> In the "net-next-2.6" tree there is also:
> >>>>>>>>>>>
> >>>>>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
> >>>>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >>>>>>>> "platform";
> >>>>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
> >>>>>>>> "platform";
> >>>>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
> >> v1.0";
> >>>>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
> >> <2>;
> >>>>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
> >> v1.0";
> >>>>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
> >> <2>;
> >>>>>>>>>>>
> >>>>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make
> >>>>>>>>>>> people think, that they could set something else.
> >>>>>>>>>>
> >>>>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need
> >>>>>>>>> of
> >>>>>>>> fsl,flexcan-clock-divider = <2>;
> >>>>>>>>> 	    But I kept it as "2" because FlexCan clock source is the
> >>>>>>>> platform clock and it is CCB/2
> >>>>>>>>> 	    If the "2" is misleading, the bindings can be changed or
> >>>>>>>>> some
> >>>>>>>> text can be written to make the meaning of "2"
> >>>>>>>>>           Understandable , Please suggest ..
> >>>>>>>>
> >>>>>>>> The clock source and frequency is fixed. Why do we need an extra
> >>>>>>>> properties for that. We have panned to remove these bogus
> >>>>>>>> bindings from the Linux kernel, which sneaked in *without* any
> >>>>>>>> review on the relevant mailing lists (at least I have not
> >>>>>>>> realized any posting). We do not think they are really needed.
> >>>>>>>> They just confuse the user. We also prefer to use the
> >>>>>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0".
> >>>>>>>> It's unusual to add a version number, which is for the Flexcan
> >>>>>>>> on the PowerPC cores only, I assume, but there will be device
> >>>>>>>> tree for ARM soon. A proper compatibility string would be
> >> "fsl,p1010-flexcan" if we really need to distinguish.
> >>>>>>>>
> >>>>>>> [Bhaskar] About clock source.. There can be two sources of clock
> >>>>>>> for
> >>>>>> the CAN.
> >>>>>>> 	Oscillator or the platform clock, but at present only
> >> platform
> >>>>>> clock is supported
> >>>>>>> 	in P1010.If we remove the fsl,flexcan-clock-source property,
> >> we
> >>>>>> will lost the flexibility
> >>>>>>> 	of changing the clock source ..
> >>>>>>>
> >>>>>>>           About clock-frequency... it is also not fixed. It
> >>>>>>> depends on
> >>>>>> the platform clock which in turns
> >>>>>>>           Depends on the CCB clock. So it will be better to keep
> >>>>>>> clock-
> >>>>>> frequency property which is getting fixed via u-boot.
> >>>>>>
> >>>>>> The frequency is fixed to CCB-frequency / 2. Will that ever
> >>>>>> change? What can we expect from future Flexcan hardware? Will it
> >>>>>> support further clock sources?
> >>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if
> >> the CCB gets changed that will be taken care by the u-boot fixup code for
> >>>>> 	clock-frequency. clock-frequency is not filled by somebody in the
> >> dts file. It will be done by u-boot.
> >>>>> 	For clock source,I can't say right now, that's why I have kept a
> >> property for this in the can node. So that in future, we need to fill it
> >>>>> 	appropriately
> >>>>
> >>>> Speaking of the dts file, I have left the p1010si.dtsi file with the
> >>>> fsl,flexcan-v1.0 .compatible definition.  The flexcan folks (IIRC
> >>>> Wolfgang) objected to that as it does not follow the standard which
> >>>> should be just fsl,flexcan.
> >>>>
> >>>> How would you like to change that?  Should I add it as part of this
> >>>> patch, add another patch to the series, or let you take care of it?
> >>>>
> >>>> Also, I assume the uboot project will need to be changed as well to
> >>>> reflect the corrected name.
> >>>
> >>> I think you should provide patches within this series to cleanup the
> >>> obsolete stuff, dts and binding doc.
> >>
> >> It reads to me that the binding doc now reduces just the required
> >> properties.  Should I remove the file entirely?
> > [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity
> >  	    about the CAN functionality
> > 		can0@1c000 {
> >                  compatible = "fsl,flexcan";
> >                  reg = <0x1c000 0x1000>;
> >                  interrupts = <48 0x2>;
> >                  interrupt-parent = <&mpic>;
> >                  clock-frequency = <fixed by u-boot>;
> >          	}; 
> 
> Yes, I also find the introduction is quite useful, with some related
> correction.

I am not sure what is useful.  The clock source bits are all wrong.
When that is removed, you end up with a discussion about the prescaler
which is actually related to the flexcan.c file and has nothing
to do with the device node.  Maybe I am just going back into my
not-communicating-well mode.  Could you follow up with what you think
belongs in the introduction of the binding file?

Thanks,
Robin

Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

[Wolfgang Grandegger]

On 08/09/2011 04:09 PM, Robin Holt wrote:
> On Tue, Aug 09, 2011 at 04:03:38PM +0200, Wolfgang Grandegger wrote:
>> On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
>>>> Sent: Tuesday, August 09, 2011 7:06 PM
>>>> To: Wolfgang Grandegger
>>>> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; Marc Kleine-
>>>> Budde
>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>
>>>> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/09/2011 02:49 PM, Robin Holt wrote:
>>>>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>>>>>>>> Sent: Tuesday, August 09, 2011 4:19 PM
>>>>>>>> To: U Bhaskar-B22300
>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>>>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>>>>>
>>>>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>>>>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM
>>>>>>>>>> To: U Bhaskar-B22300
>>>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>>>>>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
>>>> source.
>>>>>>>>>>
>>>>>>>>>> Hi Bhaskar,
>>>>>>>>>>
>>>>>>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>> From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
>>>>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
>>>>>>>>>>>> To: Wolfgang Grandegger
>>>>>>>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; U
>>>>>>>>>>>> Bhaskar- B22300
>>>>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock
>>>> source.
>>>>>>>>>>>>
>>>>>>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:
>>>>>>>>>>>>>> ACK - The device tree bindings as in mainline's
>>>>>>>>>>>>>> Documentation is a
>>>>>>>>>>>> mess.
>>>>>>>>>>>>>> If the powerpc guys are happy with a clock interfaces based
>>>>>>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to
>>>> remove:
>>>>>>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl
>>>>>>>>>>>>>> driver)
>>>>>>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It
>>>>>>>>>>> contains the usage of all the fields posted in the FlexCAN
>>>>>>>>>>> bindings at
>>>>>>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi
>>>>>>>>>>> t;a=b
>>>>>>>>>>> lo
>>>>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h
>>>>>>>>>>> =1a72
>>>>>>>>>>> 9f
>>>>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85
>>>>>>>>>>> 16972
>>>>>>>>>>> cb
>>>>>>>>>>> ebdc8274
>>>>>>>>>>
>>>>>>>>>> As Marc already pointed out, Robin already has a much more
>>>>>>>>>> advanced patch stack in preparation. Especially your patches do
>>>>>>>>>> not care about the already existing Flexcan core on the
>>>> Freescale's ARM socks.
>>>>>>>>> [Bhaskar] No, the patches are taking care of the existing ARM
>>>>>>>> functionality.
>>>>>>>>> 	I have not tested on the ARM based board, but the patches are
>>>>>>>>> made
>>>>>>>> in a
>>>>>>>>>       Manner that it should not break the ARM based functionality.
>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in
>>>> arch/ppc, or
>>>>>>>>>>>>>> - clock-frequency           /   a single clock-frequency
>>>> attribute
>>>>>>>>>>>>>
>>>>>>>>>>>>> In the "net-next-2.6" tree there is also:
>>>>>>>>>>>>>
>>>>>>>>>>>>>  $ grep flexcan arch/powerpc/boots/dts/*.dts
>>>>>>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>>>>>>>>>> "platform";
>>>>>>>>>>>>>   p1010rdb.dts:			fsl,flexcan-clock-source =
>>>>>>>>>> "platform";
>>>>>>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
>>>> v1.0";
>>>>>>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
>>>> <2>;
>>>>>>>>>>>>>   p1010si.dtsi:			compatible = "fsl,flexcan-
>>>> v1.0";
>>>>>>>>>>>>>   p1010si.dtsi:			fsl,flexcan-clock-divider =
>>>> <2>;
>>>>>>>>>>>>>
>>>>>>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make
>>>>>>>>>>>>> people think, that they could set something else.
>>>>>>>>>>>>
>>>>>>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need
>>>>>>>>>>> of
>>>>>>>>>> fsl,flexcan-clock-divider = <2>;
>>>>>>>>>>> 	    But I kept it as "2" because FlexCan clock source is the
>>>>>>>>>> platform clock and it is CCB/2
>>>>>>>>>>> 	    If the "2" is misleading, the bindings can be changed or
>>>>>>>>>>> some
>>>>>>>>>> text can be written to make the meaning of "2"
>>>>>>>>>>>           Understandable , Please suggest ..
>>>>>>>>>>
>>>>>>>>>> The clock source and frequency is fixed. Why do we need an extra
>>>>>>>>>> properties for that. We have panned to remove these bogus
>>>>>>>>>> bindings from the Linux kernel, which sneaked in *without* any
>>>>>>>>>> review on the relevant mailing lists (at least I have not
>>>>>>>>>> realized any posting). We do not think they are really needed.
>>>>>>>>>> They just confuse the user. We also prefer to use the
>>>>>>>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0".
>>>>>>>>>> It's unusual to add a version number, which is for the Flexcan
>>>>>>>>>> on the PowerPC cores only, I assume, but there will be device
>>>>>>>>>> tree for ARM soon. A proper compatibility string would be
>>>> "fsl,p1010-flexcan" if we really need to distinguish.
>>>>>>>>>>
>>>>>>>>> [Bhaskar] About clock source.. There can be two sources of clock
>>>>>>>>> for
>>>>>>>> the CAN.
>>>>>>>>> 	Oscillator or the platform clock, but at present only
>>>> platform
>>>>>>>> clock is supported
>>>>>>>>> 	in P1010.If we remove the fsl,flexcan-clock-source property,
>>>> we
>>>>>>>> will lost the flexibility
>>>>>>>>> 	of changing the clock source ..
>>>>>>>>>
>>>>>>>>>           About clock-frequency... it is also not fixed. It
>>>>>>>>> depends on
>>>>>>>> the platform clock which in turns
>>>>>>>>>           Depends on the CCB clock. So it will be better to keep
>>>>>>>>> clock-
>>>>>>>> frequency property which is getting fixed via u-boot.
>>>>>>>>
>>>>>>>> The frequency is fixed to CCB-frequency / 2. Will that ever
>>>>>>>> change? What can we expect from future Flexcan hardware? Will it
>>>>>>>> support further clock sources?
>>>>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if
>>>> the CCB gets changed that will be taken care by the u-boot fixup code for
>>>>>>> 	clock-frequency. clock-frequency is not filled by somebody in the
>>>> dts file. It will be done by u-boot.
>>>>>>> 	For clock source,I can't say right now, that's why I have kept a
>>>> property for this in the can node. So that in future, we need to fill it
>>>>>>> 	appropriately
>>>>>>
>>>>>> Speaking of the dts file, I have left the p1010si.dtsi file with the
>>>>>> fsl,flexcan-v1.0 .compatible definition.  The flexcan folks (IIRC
>>>>>> Wolfgang) objected to that as it does not follow the standard which
>>>>>> should be just fsl,flexcan.
>>>>>>
>>>>>> How would you like to change that?  Should I add it as part of this
>>>>>> patch, add another patch to the series, or let you take care of it?
>>>>>>
>>>>>> Also, I assume the uboot project will need to be changed as well to
>>>>>> reflect the corrected name.
>>>>>
>>>>> I think you should provide patches within this series to cleanup the
>>>>> obsolete stuff, dts and binding doc.
>>>>
>>>> It reads to me that the binding doc now reduces just the required
>>>> properties.  Should I remove the file entirely?
>>> [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity
>>>  	    about the CAN functionality
>>> 		can0@1c000 {
>>>                  compatible = "fsl,flexcan";
>>>                  reg = <0x1c000 0x1000>;
>>>                  interrupts = <48 0x2>;
>>>                  interrupt-parent = <&mpic>;
>>>                  clock-frequency = <fixed by u-boot>;
>>>          	}; 
>>
>> Yes, I also find the introduction is quite useful, with some related
>> correction.
> 
> I am not sure what is useful.  The clock source bits are all wrong.
> When that is removed, you end up with a discussion about the prescaler
> which is actually related to the flexcan.c file and has nothing
> to do with the device node.  Maybe I am just going back into my
> not-communicating-well mode.  Could you follow up with what you think
> belongs in the introduction of the binding file?

Yep, you are right. Keep it simple...

Wolfgang,