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

[Robin Holt <holt@sgi.com>]

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@grandegger.com]
> >>> Sent: Tuesday, August 09, 2011 4:19 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.
> >>>
> >>> 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=b
> >>>>>> lo
> >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72
> >>>>>> 9f
> >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972
> >>>>>> 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.

Will do.

Robin