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

Tue Aug 9 22:49:19 EST 2011

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.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.

Thanks,
Robin