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

[Wolfgang Grandegger <wg@grandegger.com>]

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.