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

[Kumar Gala]

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 at sgi.com>
>>>>> Acked-by: Marc Kleine-Budde <mkl at pengutronix.de>,
>>>>> Acked-by: Wolfgang Grandegger <wg at grandegger.com>,
>>>>> Cc: U Bhaskar-B22300 <B22300 at freescale.com>
>>>>> Cc: socketcan-core at lists.berlios.de,
>>>>> Cc: netdev at vger.kernel.org,
>>>>> Cc: PPC list <linuxppc-dev at lists.ozlabs.org>
>>>>> Cc: Kumar Gala <galak at 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