Patchwork [v11,4/5] powerpc: Add flexcan device support for p1010rdb.

login
register
mail settings
Submitter holt@sgi.com
Date Aug. 10, 2011, 4:27 p.m.
Message ID <1312993670-23999-5-git-send-email-holt@sgi.com>
Download mbox | patch
Permalink /patch/109402/
State Not Applicable
Headers show

Comments

holt@sgi.com - Aug. 10, 2011, 4:27 p.m.
I added a simple clock source for the p1010rdb so the flexcan driver
could determine a clock frequency.  The p1010 flexcan device only has
an oscillator of system bus frequency divided by 2.

Signed-off-by: Robin Holt <holt@sgi.com>
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
Cc: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de,
Cc: netdev@vger.kernel.org,
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
 arch/powerpc/platforms/85xx/Kconfig    |    2 +
 arch/powerpc/platforms/85xx/Makefile   |    2 +
 arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
 arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
 4 files changed, 64 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/platforms/85xx/clock.c
Kumar Gala - Aug. 10, 2011, 5:01 p.m.
On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:

> I added a simple clock source for the p1010rdb so the flexcan driver
> could determine a clock frequency.  The p1010 flexcan device only has
> an oscillator of system bus frequency divided by 2.
> 
> Signed-off-by: Robin Holt <holt@sgi.com>
> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
> Cc: U Bhaskar-B22300 <B22300@freescale.com>
> Cc: socketcan-core@lists.berlios.de,
> Cc: netdev@vger.kernel.org,
> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> ---
> arch/powerpc/platforms/85xx/Kconfig    |    2 +
> arch/powerpc/platforms/85xx/Makefile   |    2 +
> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
> 4 files changed, 64 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/clock.c

I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.

This either seems to global or I'm missing something.

I still think the clk / freq info should be in the device tree and handled in the driver and NOT arch/powerpc platform code.

- k
Wolfgang Grandegger - Aug. 10, 2011, 6:16 p.m.
On 08/10/2011 07:01 PM, Kumar Gala wrote:
> 
> On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
> 
>> I added a simple clock source for the p1010rdb so the flexcan driver
>> could determine a clock frequency.  The p1010 flexcan device only has
>> an oscillator of system bus frequency divided by 2.
>>
>> Signed-off-by: Robin Holt <holt@sgi.com>
>> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
>> Cc: U Bhaskar-B22300 <B22300@freescale.com>
>> Cc: socketcan-core@lists.berlios.de,
>> Cc: netdev@vger.kernel.org,
>> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
>> Cc: Kumar Gala <galak@kernel.crashing.org>
>> ---
>> arch/powerpc/platforms/85xx/Kconfig    |    2 +
>> arch/powerpc/platforms/85xx/Makefile   |    2 +
>> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
>> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
>> 4 files changed, 64 insertions(+), 0 deletions(-)
>> create mode 100644 arch/powerpc/platforms/85xx/clock.c
> 
> I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.

The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" for
Flexcan devices.

> This either seems to global or I'm missing something.

This patch extends the existing Flexcan platform driver for ARM for the
PowerPC using the device tree. Due to the nice integration of the device
tree (of-platform) into the platform driver and devices, the difference
are quite small (see patches 1..3). Apart from the endianess issue, only
the clock needs to be handled in a common way. As ARM already uses the
clk interface, we found it straight-forward to implement it for the
P1010, or more general for the 85xx, as well, instead of using an
additional helper function.

> I still think the clk / freq info should be in the device tree and handled in the driver and NOT arch/powerpc platform code.

If I understand you correctly, you want the boot-loader to provide the
relevant information by fixing up the device tree, which then can be
handled arch-independently by the driver, right?

Wolfgang.
Kumar Gala - Aug. 11, 2011, 4:46 a.m.
On Aug 10, 2011, at 1:16 PM, Wolfgang Grandegger wrote:

> On 08/10/2011 07:01 PM, Kumar Gala wrote:
>> 
>> On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
>> 
>>> I added a simple clock source for the p1010rdb so the flexcan driver
>>> could determine a clock frequency.  The p1010 flexcan device only has
>>> an oscillator of system bus frequency divided by 2.
>>> 
>>> Signed-off-by: Robin Holt <holt@sgi.com>
>>> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
>>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
>>> Cc: U Bhaskar-B22300 <B22300@freescale.com>
>>> Cc: socketcan-core@lists.berlios.de,
>>> Cc: netdev@vger.kernel.org,
>>> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
>>> Cc: Kumar Gala <galak@kernel.crashing.org>
>>> ---
>>> arch/powerpc/platforms/85xx/Kconfig    |    2 +
>>> arch/powerpc/platforms/85xx/Makefile   |    2 +
>>> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
>>> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
>>> 4 files changed, 64 insertions(+), 0 deletions(-)
>>> create mode 100644 arch/powerpc/platforms/85xx/clock.c
>> 
>> I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.
> 
> The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" for
> Flexcan devices.
> 
>> This either seems to global or I'm missing something.
> 
> This patch extends the existing Flexcan platform driver for ARM for the
> PowerPC using the device tree. Due to the nice integration of the device
> tree (of-platform) into the platform driver and devices, the difference
> are quite small (see patches 1..3). Apart from the endianess issue, only
> the clock needs to be handled in a common way. As ARM already uses the
> clk interface, we found it straight-forward to implement it for the
> P1010, or more general for the 85xx, as well, instead of using an
> additional helper function.

I see, that.  What concerns me is there are numerous clocks / frequencies that exist inside a MPC85xx/P1010 SOC.  The code I'm seeing does NOT seem to do anything to relate this clock JUST to the flexcan.

>> I still think the clk / freq info should be in the device tree and handled in the driver and NOT arch/powerpc platform code.
> 
> If I understand you correctly, you want the boot-loader to provide the
> relevant information by fixing up the device tree, which then can be
> handled arch-independently by the driver, right?

Yes, that is part of what I want.

- k
Wolfgang Grandegger - Aug. 11, 2011, 7:26 a.m.
On 08/11/2011 06:46 AM, Kumar Gala wrote:
> 
> On Aug 10, 2011, at 1:16 PM, Wolfgang Grandegger wrote:
> 
>> On 08/10/2011 07:01 PM, Kumar Gala wrote:
>>>
>>> On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
>>>
>>>> I added a simple clock source for the p1010rdb so the flexcan driver
>>>> could determine a clock frequency.  The p1010 flexcan device only has
>>>> an oscillator of system bus frequency divided by 2.
>>>>
>>>> Signed-off-by: Robin Holt <holt@sgi.com>
>>>> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
>>>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
>>>> Cc: U Bhaskar-B22300 <B22300@freescale.com>
>>>> Cc: socketcan-core@lists.berlios.de,
>>>> Cc: netdev@vger.kernel.org,
>>>> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
>>>> Cc: Kumar Gala <galak@kernel.crashing.org>
>>>> ---
>>>> arch/powerpc/platforms/85xx/Kconfig    |    2 +
>>>> arch/powerpc/platforms/85xx/Makefile   |    2 +
>>>> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
>>>> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
>>>> 4 files changed, 64 insertions(+), 0 deletions(-)
>>>> create mode 100644 arch/powerpc/platforms/85xx/clock.c
>>>
>>> I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.
>>
>> The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" for
>> Flexcan devices.
>>
>>> This either seems to global or I'm missing something.
>>
>> This patch extends the existing Flexcan platform driver for ARM for the
>> PowerPC using the device tree. Due to the nice integration of the device
>> tree (of-platform) into the platform driver and devices, the difference
>> are quite small (see patches 1..3). Apart from the endianess issue, only
>> the clock needs to be handled in a common way. As ARM already uses the
>> clk interface, we found it straight-forward to implement it for the
>> P1010, or more general for the 85xx, as well, instead of using an
>> additional helper function.
> 
> I see, that.  What concerns me is there are numerous clocks / frequencies that exist inside a MPC85xx/P1010 SOC.  The code I'm seeing does NOT seem to do anything to relate this clock JUST to the flexcan.

The clk interface is not commonly used on PowerPC, I know. It's just to
provide compatibility with ARM. An alternative would be to use some
helper function.

>>> I still think the clk / freq info should be in the device tree and handled in the driver and NOT arch/powerpc platform code.
>>
>> If I understand you correctly, you want the boot-loader to provide the
>> relevant information by fixing up the device tree, which then can be
>> handled arch-independently by the driver, right?
> 
> Yes, that is part of what I want.

This works fine if we just have *one* fixed clock source and frequency.
When there are choices (source and divider) it does make sense to allow
the user to select the frequency via DTS file entries for Linux. Then we
need arch-specific code anyway (to set the relevant registers).
Furthermore we rely on the boot-loader (the usual argument) which is not
a problem for new boards (with new boot-loader), of course.

Wolfgang.
holt@sgi.com - Aug. 11, 2011, 10:42 a.m.
On Wed, Aug 10, 2011 at 11:46:27PM -0500, Kumar Gala wrote:
> 
> On Aug 10, 2011, at 1:16 PM, Wolfgang Grandegger wrote:
> 
> > On 08/10/2011 07:01 PM, Kumar Gala wrote:
> >> 
> >> On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
> >> 
> >>> I added a simple clock source for the p1010rdb so the flexcan driver
> >>> could determine a clock frequency.  The p1010 flexcan device only has
> >>> an oscillator of system bus frequency divided by 2.
> >>> 
> >>> Signed-off-by: Robin Holt <holt@sgi.com>
> >>> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
> >>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
> >>> Cc: U Bhaskar-B22300 <B22300@freescale.com>
> >>> Cc: socketcan-core@lists.berlios.de,
> >>> Cc: netdev@vger.kernel.org,
> >>> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
> >>> Cc: Kumar Gala <galak@kernel.crashing.org>
> >>> ---
> >>> arch/powerpc/platforms/85xx/Kconfig    |    2 +
> >>> arch/powerpc/platforms/85xx/Makefile   |    2 +
> >>> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
> >>> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
> >>> 4 files changed, 64 insertions(+), 0 deletions(-)
> >>> create mode 100644 arch/powerpc/platforms/85xx/clock.c
> >> 
> >> I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.
> > 
> > The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" for
> > Flexcan devices.
> > 
> >> This either seems to global or I'm missing something.
> > 
> > This patch extends the existing Flexcan platform driver for ARM for the
> > PowerPC using the device tree. Due to the nice integration of the device
> > tree (of-platform) into the platform driver and devices, the difference
> > are quite small (see patches 1..3). Apart from the endianess issue, only
> > the clock needs to be handled in a common way. As ARM already uses the
> > clk interface, we found it straight-forward to implement it for the
> > P1010, or more general for the 85xx, as well, instead of using an
> > additional helper function.
> 
> I see, that.  What concerns me is there are numerous clocks /
> frequencies that exist inside a MPC85xx/P1010 SOC.  The code I'm seeing
> does NOT seem to do anything to relate this clock JUST to the flexcan.

        if (!dev->of_node ||
            !of_device_is_compatible(dev->of_node, "fsl,flexcan"))
                return ERR_PTR(-ENOENT);

That should relate it just to flexcan, right?  Plus it has the added
benefit of being a baby-step in the direction of implementing a clkdev
type thing for powerpc which did look fairly slick to me, but I may
be confused.

It sounds like Wolfgang is defering to you.  Give it an honest evaluation
and tell me which direction you would like me to go.  I don't have a
strong preference either way.  The alternative I gave to Wolfgang of
using a flexcan property to avoid needing any clk_get_rate seems fairly
hackish at this point, but I have had more time to get used to the
'hack in a 85xx clock' method.

Thanks,
Robin
Kumar Gala - Aug. 11, 2011, 2:17 p.m.
On Aug 11, 2011, at 5:42 AM, Robin Holt wrote:

> On Wed, Aug 10, 2011 at 11:46:27PM -0500, Kumar Gala wrote:
>> 
>> On Aug 10, 2011, at 1:16 PM, Wolfgang Grandegger wrote:
>> 
>>> On 08/10/2011 07:01 PM, Kumar Gala wrote:
>>>> 
>>>> On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
>>>> 
>>>>> I added a simple clock source for the p1010rdb so the flexcan driver
>>>>> could determine a clock frequency.  The p1010 flexcan device only has
>>>>> an oscillator of system bus frequency divided by 2.
>>>>> 
>>>>> Signed-off-by: Robin Holt <holt@sgi.com>
>>>>> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
>>>>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>,
>>>>> Cc: U Bhaskar-B22300 <B22300@freescale.com>
>>>>> Cc: socketcan-core@lists.berlios.de,
>>>>> Cc: netdev@vger.kernel.org,
>>>>> Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
>>>>> Cc: Kumar Gala <galak@kernel.crashing.org>
>>>>> ---
>>>>> arch/powerpc/platforms/85xx/Kconfig    |    2 +
>>>>> arch/powerpc/platforms/85xx/Makefile   |    2 +
>>>>> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
>>>>> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
>>>>> 4 files changed, 64 insertions(+), 0 deletions(-)
>>>>> create mode 100644 arch/powerpc/platforms/85xx/clock.c
>>>> 
>>>> I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.
>>> 
>>> The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" for
>>> Flexcan devices.
>>> 
>>>> This either seems to global or I'm missing something.
>>> 
>>> This patch extends the existing Flexcan platform driver for ARM for the
>>> PowerPC using the device tree. Due to the nice integration of the device
>>> tree (of-platform) into the platform driver and devices, the difference
>>> are quite small (see patches 1..3). Apart from the endianess issue, only
>>> the clock needs to be handled in a common way. As ARM already uses the
>>> clk interface, we found it straight-forward to implement it for the
>>> P1010, or more general for the 85xx, as well, instead of using an
>>> additional helper function.
>> 
>> I see, that.  What concerns me is there are numerous clocks /
>> frequencies that exist inside a MPC85xx/P1010 SOC.  The code I'm seeing
>> does NOT seem to do anything to relate this clock JUST to the flexcan.
> 
>        if (!dev->of_node ||
>            !of_device_is_compatible(dev->of_node, "fsl,flexcan"))
>                return ERR_PTR(-ENOENT);
> 
> That should relate it just to flexcan, right?  Plus it has the added
> benefit of being a baby-step in the direction of implementing a clkdev
> type thing for powerpc which did look fairly slick to me, but I may
> be confused.
> 
> It sounds like Wolfgang is defering to you.  Give it an honest evaluation
> and tell me which direction you would like me to go.  I don't have a
> strong preference either way.  The alternative I gave to Wolfgang of
> using a flexcan property to avoid needing any clk_get_rate seems fairly
> hackish at this point, but I have had more time to get used to the
> 'hack in a 85xx clock' method.

For some time we've been adding 'clock-frequency' nodes in the device tree to abstract having to know this headache in the kernel and adding a bunch of SoC specific code all the time.  So pushing this to the firmware is exactly where we want it for FSL PPC SoCs.

We need to make sure the device tree binding has details on a 'clock-frequency' property.

- k

Patch

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 498534c..c4304ae 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -70,6 +70,8 @@  config MPC85xx_RDB
 config P1010_RDB
 	bool "Freescale P1010RDB"
 	select DEFAULT_UIMAGE
+	select HAVE_CAN_FLEXCAN if NET && CAN
+	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..cc7f381 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -3,6 +3,8 @@ 
 #
 obj-$(CONFIG_SMP) += smp.o
 
+obj-$(CONFIG_PPC_CLOCK)   += clock.o
+
 obj-$(CONFIG_MPC8540_ADS) += mpc85xx_ads.o
 obj-$(CONFIG_MPC8560_ADS) += mpc85xx_ads.o
 obj-$(CONFIG_MPC85xx_CDS) += mpc85xx_cds.o
diff --git a/arch/powerpc/platforms/85xx/clock.c b/arch/powerpc/platforms/85xx/clock.c
new file mode 100644
index 0000000..0b6db1d
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/clock.c
@@ -0,0 +1,52 @@ 
+/*
+ * Copyright 2011 SGI, inc.
+ *
+ * This code is licensed for use under the GPL V2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+
+#include <asm/clk_interface.h>
+
+#include <sysdev/fsl_soc.h>
+
+/*
+ * p1010 needs to provide a clock source for the flexcan driver. The
+ * oscillator for the p1010 processor is only ever the system clock / 2.
+ */
+
+static struct clk *mpc85xx_clk_get(struct device *dev, const char *id)
+{
+	if (!dev)
+		return ERR_PTR(-ENOENT);
+
+	if (!dev->of_node ||
+	    !of_device_is_compatible(dev->of_node, "fsl,flexcan"))
+		return ERR_PTR(-ENOENT);
+
+	return NULL;
+}
+
+static void mpc85xx_clk_put(struct clk *clk)
+{
+	return;
+}
+
+static unsigned long mpc85xx_clk_get_rate(struct clk *clk)
+{
+	return fsl_get_sys_freq() / 2;
+}
+
+static struct clk_interface mpc85xx_clk_functions = {
+	.clk_get = mpc85xx_clk_get,
+	.clk_get_rate = mpc85xx_clk_get_rate,
+	.clk_put = mpc85xx_clk_put,
+};
+
+void __init mpc85xx_clk_init(void)
+{
+	clk_functions = mpc85xx_clk_functions;
+}
diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c
index d7387fa..5e52122 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");
 }
 
+extern void mpc85xx_clk_init(void);
+
+static void __init p1010_rdb_init(void)
+{
+	mpc85xx_clk_init();
+}
+
 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,