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

[Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>]

On 08/09/2011 04:09 PM, Robin Holt wrote:
> On Tue, Aug 09, 2011 at 04:03:38PM +0200, Wolfgang Grandegger wrote:
>> On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
>>>> Sent: Tuesday, August 09, 2011 7:06 PM
>>>> To: Wolfgang Grandegger
>>>> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; Marc Kleine-
>>>> Budde
>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>
>>>> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote:
>>>>> On 08/09/2011 02:49 PM, Robin Holt wrote:
>>>>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>>>>>>>> Sent: Tuesday, August 09, 2011 4:19 PM
>>>>>>>> To: U Bhaskar-B22300
>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>>>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.
>>>>>>>>
>>>>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
>>>>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM
>>>>>>>>>> To: U Bhaskar-B22300
>>>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
>>>>>>>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
>>>>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
>>>>>>>>>>>> To: Wolfgang Grandegger
>>>>>>>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.gi
>>>>>>>>>>> t;a=b
>>>>>>>>>>> lo
>>>>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h
>>>>>>>>>>> =1a72
>>>>>>>>>>> 9f
>>>>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85
>>>>>>>>>>> 16972
>>>>>>>>>>> cb
>>>>>>>>>>> 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?
>>>>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if
>>>> the CCB gets changed that will be taken care by the u-boot fixup code for
>>>>>>> 	clock-frequency. clock-frequency is not filled by somebody in the
>>>> dts file. It will be done by u-boot.
>>>>>>> 	For clock source,I can't say right now, that's why I have kept a
>>>> property for this in the can node. So that in future, we need to fill it
>>>>>>> 	appropriately
>>>>>>
>>>>>> Speaking of the dts file, I have left the p1010si.dtsi file with the
>>>>>> fsl,flexcan-v1.0 .compatible definition.  The flexcan folks (IIRC
>>>>>> Wolfgang) objected to that as it does not follow the standard which
>>>>>> should be just fsl,flexcan.
>>>>>>
>>>>>> How would you like to change that?  Should I add it as part of this
>>>>>> patch, add another patch to the series, or let you take care of it?
>>>>>>
>>>>>> Also, I assume the uboot project will need to be changed as well to
>>>>>> reflect the corrected name.
>>>>>
>>>>> I think you should provide patches within this series to cleanup the
>>>>> obsolete stuff, dts and binding doc.
>>>>
>>>> It reads to me that the binding doc now reduces just the required
>>>> properties.  Should I remove the file entirely?
>>> [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity
>>>  	    about the CAN functionality
>>> 		can0@1c000 {
>>>                  compatible = "fsl,flexcan";
>>>                  reg = <0x1c000 0x1000>;
>>>                  interrupts = <48 0x2>;
>>>                  interrupt-parent = <&mpic>;
>>>                  clock-frequency = <fixed by u-boot>;
>>>          	}; 
>>
>> Yes, I also find the introduction is quite useful, with some related
>> correction.
> 
> I am not sure what is useful.  The clock source bits are all wrong.
> When that is removed, you end up with a discussion about the prescaler
> which is actually related to the flexcan.c file and has nothing
> to do with the device node.  Maybe I am just going back into my
> not-communicating-well mode.  Could you follow up with what you think
> belongs in the introduction of the binding file?

Yep, you are right. Keep it simple...

Wolfgang,