[RFC 5/5] [powerpc] Implement a p1010rdb clock source.
Wolfgang Grandegger
wg at grandegger.com
Wed Aug 10 00:14:55 EST 2011
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 at sgi.com]
>>>> Sent: Tuesday, August 09, 2011 7:06 PM
>>>> To: Wolfgang Grandegger
>>>> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core at lists.berlios.de;
>>>> netdev at vger.kernel.org; Devicetree-discuss at lists.ozlabs.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 at grandegger.com]
>>>>>>>> Sent: Tuesday, August 09, 2011 4:19 PM
>>>>>>>> To: U Bhaskar-B22300
>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core at lists.berlios.de;
>>>>>>>> netdev at vger.kernel.org; Devicetree-discuss at lists.ozlabs.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 at grandegger.com]
>>>>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM
>>>>>>>>>> To: U Bhaskar-B22300
>>>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core at lists.berlios.de;
>>>>>>>>>> netdev at vger.kernel.org; Devicetree-discuss at 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 at pengutronix.de]
>>>>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM
>>>>>>>>>>>> To: Wolfgang Grandegger
>>>>>>>>>>>> Cc: socketcan-core at lists.berlios.de; netdev at 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.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 at 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,
More information about the devicetree-discuss
mailing list