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

From: Wolfgang Grandegger <hidden>
Date: 2011-08-09 14:14:55
Also in: netdev

On 08/09/2011 04:09 PM, Robin Holt wrote:

On Tue, Aug 09, 2011 at 04:03:38PM +0200, Wolfgang Grandegger wrote:

quoted

On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote:

quoted

-----Original Message-----
From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
Sent: Tuesday, August 09, 2011 7:06 PM
To: Wolfgang Grandegger
Cc: Robin Holt; U Bhaskar-B22300; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org;
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.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:

quoted

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

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

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

quoted

Hi Bhaskar,

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

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.

quoted

On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote:

quoted

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

mess.

quoted

If the powerpc guys are happy with a clock interfaces based
approach somewhere in arch/ppc, I'm more than happy to

remove:

quoted

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

quoted

[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

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
- clock-frequency           /   a single clock-frequency

attribute

quoted

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

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

"platform";

quoted

  p1010si.dtsi:			compatible = "fsl,flexcan-

v1.0";

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
  p1010si.dtsi:			fsl,flexcan-clock-divider =

<2>;

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
  p1010si.dtsi:			compatible = "fsl,flexcan-

v1.0";

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
  p1010si.dtsi:			fsl,flexcan-clock-divider =

<2>;

quoted

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.

quoted

[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

quoted

clock is supported

quoted

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

we

quoted

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

quoted

	clock-frequency. clock-frequency is not filled by somebody in the

dts file. It will be done by u-boot.

quoted

	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

quoted

	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?

Yep, you are right. Keep it simple...

Wolfgang,

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help