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:

quoted

On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote:

quoted

quoted

-----Original Message-----
From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
Sent: Tuesday, August 09, 2011 4:19 PM
To: U Bhaskar-B22300
Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source.

On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote:

quoted

quoted

-----Original Message-----
From: Wolfgang Grandegger [mailto:wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org]
Sent: Tuesday, August 09, 2011 2:03 PM
To: U Bhaskar-B22300
Cc: Marc Kleine-Budde; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.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:

quoted

quoted

-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
Sent: Tuesday, August 09, 2011 12:23 AM
To: Wolfgang Grandegger
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.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:

quoted

quoted

ACK - The device tree bindings as in mainline's Documentation is
a

mess.

quoted

quoted

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.

quoted

	I have not tested on the ARM based board, but the patches are made

in a

quoted

      Manner that it should not break the ARM based functionality.

quoted

quoted

quoted

quoted

quoted

- 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";

quoted

quoted

quoted

  p1010rdb.dts:			fsl,flexcan-clock-source =

"platform";

quoted

quoted

quoted

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

quoted

	    But I kept it as "2" because FlexCan clock source is the

platform clock and it is CCB/2

quoted

	    If the "2" is misleading, the bindings can be changed or some

text can be written to make the meaning of "2"

quoted

          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.

quoted

	Oscillator or the platform clock, but at present only platform

clock is supported

quoted

	in P1010.If we remove the fsl,flexcan-clock-source property, we

will lost the flexibility

quoted

	of changing the clock source ..

          About clock-frequency... it is also not fixed. It depends on

the platform clock which in turns

quoted

          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?

Robin