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

On Tue, Aug 09, 2011 at 09:11:33AM +0200, Wolfgang Grandegger wrote:

On 08/09/2011 08:33 AM, Robin Holt wrote:

quoted

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

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

quoted

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

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

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 498534c..ed4cf92 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig

@@ -26,6 +26,10 @@ config MPC8560_ADS
 	help
 	  This option enables support for the MPC 8560 ADS board
 
+config 85xx_HAVE_CAN_FLEXCAN
+	bool
+	select HAVE_CAN_FLEXCAN if NET && CAN
+

Why do you need that? More below...

Happily removed.

quoted

 config MPC85xx_CDS
 	bool "Freescale MPC85xx CDS"
 	select DEFAULT_UIMAGE

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

	select HAVE_CAN_FLEXCAN
	select PPC_CLOCK

Should just be fine, or have I missed something.

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

quoted

 	help
 	  This option enables support for the MPC85xx RDB (P1010 RDB) board
 

diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index a971b32..64ad7a4 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile

@@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS)  += mpc85xx_ds.o
 obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
 obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
 obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
+obj-$(CONFIG_PPC_CLOCK)   += clock.o

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

Done.

quoted

 obj-$(CONFIG_P1022_DS)    += p1022_ds.o
 obj-$(CONFIG_P1023_RDS)   += p1023_rds.o
 obj-$(CONFIG_P2040_RDB)   += p2040_rdb.o corenet_ds.o

diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
new file mode 100644
index 0000000..a25cbf3
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/clock.c

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

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

Removed.

quoted

+	return &p1010_rdb_system_clock;

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

        return fsl_get_sys_freq() / 2; ????

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

quoted

+}
+
+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();

quoted

+	clk_functions = p1010_rdb_clk_functions;
+}

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

Done.

quoted

diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
index d7387fa..d0afbf9 100644
--- a/arch/powerpc/platforms/85xx/p1010rdb.c
+++ b/arch/powerpc/platforms/85xx/p1010rdb.c

@@ -81,6 +81,15 @@ static void __init p1010_rdb_setup_arch(void)
 	printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
 }
 
+extern void p1010_rdb_clk_init(void);
+
+static void __init p1010_rdb_init(void)
+{
+#ifdef CONFIG_PPC_CLOCK
+	p1010_rdb_clk_init();
+#endif
+}

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

Done.

quoted

 static struct of_device_id __initdata p1010rdb_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },

@@ -111,6 +120,7 @@ define_machine(p1010_rdb) {
 	.name			= "P1010 RDB",
 	.probe			= p1010_rdb_probe,
 	.setup_arch		= p1010_rdb_setup_arch,
+	.init			= p1010_rdb_init,
 	.init_IRQ		= p1010_rdb_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,

Thank you very much for your patience with me,
Robin