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:h...@sgi.com] > >> Sent: Tuesday, August 09, 2011 7:06 PM > >> To: Wolfgang Grandegger > >> Cc: Robin Holt; U Bhaskar-B22300; socketcan-c...@lists.berlios.de; > >> net...@vger.kernel.org; Devicetree-discuss@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:w...@grandegger.com] > >>>>>> Sent: Tuesday, August 09, 2011 4:19 PM > >>>>>> To: U Bhaskar-B22300 > >>>>>> Cc: Marc Kleine-Budde; socketcan-c...@lists.berlios.de; > >>>>>> net...@vger.kernel.org; Devicetree-discuss@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:w...@grandegger.com] > >>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM > >>>>>>>> To: U Bhaskar-B22300 > >>>>>>>> Cc: Marc Kleine-Budde; socketcan-c...@lists.berlios.de; > >>>>>>>> net...@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:m...@pengutronix.de] > >>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM > >>>>>>>>>> To: Wolfgang Grandegger > >>>>>>>>>> Cc: socketcan-c...@lists.berlios.de; net...@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@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? Thanks, Robin _______________________________________________ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
