[RFC 5/5] [powerpc] Implement a p1010rdb clock source.
Wolfgang Grandegger
wg at grandegger.com
Wed Aug 10 00:03:38 EST 2011
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.
Wolfgang.
More information about the devicetree-discuss
mailing list