Re: [PATCH v11 4/5] powerpc: Add flexcan device support for p1010rdb.

On Aug 11, 2011, at 5:42 AM, Robin Holt wrote:

On Wed, Aug 10, 2011 at 11:46:27PM -0500, Kumar Gala wrote:

quoted

=20
On Aug 10, 2011, at 1:16 PM, Wolfgang Grandegger wrote:
=20

quoted

On 08/10/2011 07:01 PM, Kumar Gala wrote:

quoted

=20
On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
=20

quoted

I added a simple clock source for the p1010rdb so the flexcan =

driver

quoted

quoted

quoted

quoted

could determine a clock frequency.  The p1010 flexcan device only =

has

quoted

quoted

quoted

quoted

an oscillator of system bus frequency divided by 2.
=20
Signed-off-by: Robin Holt <redacted>
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>,
Acked-by: Wolfgang Grandegger <redacted>,
Cc: U Bhaskar-B22300 <redacted>
Cc: socketcan-core@lists.berlios.de,
Cc: netdev@vger.kernel.org,
Cc: PPC list <redacted>
Cc: Kumar Gala <redacted>
---
arch/powerpc/platforms/85xx/Kconfig    |    2 +
arch/powerpc/platforms/85xx/Makefile   |    2 +
arch/powerpc/platforms/85xx/clock.c    |   52 =

++++++++++++++++++++++++++++++++

quoted

quoted

quoted

quoted

arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
4 files changed, 64 insertions(+), 0 deletions(-)
create mode 100644 arch/powerpc/platforms/85xx/clock.c

=20
I dont understand how mpc85xx_clk_functions() ends up being =

associated with the frequency the flexcan is running at.

quoted

quoted

=20
The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" =

for

quoted

quoted

Flexcan devices.
=20

quoted

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

=20
This patch extends the existing Flexcan platform driver for ARM for =

the

quoted

quoted

PowerPC using the device tree. Due to the nice integration of the =

device

quoted

quoted

tree (of-platform) into the platform driver and devices, the =

difference

quoted

quoted

are quite small (see patches 1..3). Apart from the endianess issue, =

only

quoted

quoted

the clock needs to be handled in a common way. As ARM already uses =

the

quoted

quoted

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.

=20
I see, that.  What concerns me is there are numerous clocks /
frequencies that exist inside a MPC85xx/P1010 SOC.  The code I'm =

seeing

quoted

does NOT seem to do anything to relate this clock JUST to the =

flexcan.

=20
       if (!dev->of_node ||
           !of_device_is_compatible(dev->of_node, "fsl,flexcan"))
               return ERR_PTR(-ENOENT);
=20
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.
=20
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=