Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.

From: Kumar Gala <hidden>
Date: 2011-08-10 16:53:27
Also in: netdev

On Aug 10, 2011, at 11:00 AM, Robin Holt wrote:

On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:

quoted

=20
=20

quoted

-----Original Message-----
From: Robin Holt [mailto:holt@sgi.com]
Sent: Wednesday, August 10, 2011 7:46 PM
To: Wolfgang Grandegger
Cc: Robin Holt; Marc Kleine-Budde; U Bhaskar-B22300; Wood =

Scott-B07421;

quoted

netdev@vger.kernel.org; Kumar Gala; socketcan-core@lists.berlios.de; =

PPC

quoted

list
Subject: Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device =

tree

quoted

binding.
=20
On Wed, Aug 10, 2011 at 03:47:43PM +0200, Wolfgang Grandegger wrote:

quoted

Hi Robin,
=20
On 08/10/2011 05:06 AM, Robin Holt wrote:

quoted

In working with the socketcan developers, we have come to the
conclusion the Documentation...fsl-flexcan.txt device tree
documentation needs to be cleaned up.  The driver does not depend
upon any properties other

=20
Your first sentence could be misleading. Please just describe what =

the

quoted

patch does and why, something like:
=20
"This patch cleans up the documentation of the device-tree binding =

for

quoted

the Flexcan devices on Freescale's PowerPC and ARM cores. Extra
properties are not needed as the frequency of the source clock is
fixed..." and so on.

=20
I borrowed heavily from your message. ;)
=20

quoted

than the required properties so we are removing the file.
Additionally, the p1010*dts* files are not following the standard
for node naming in that they have a trailing -v1.0.

=20

quoted

Signed-off-by: Robin Holt <redacted>
To: Marc Kleine-Budde <mkl@pengutronix.de>,
To: Wolfgang Grandegger <redacted>,
To: U Bhaskar-B22300 <redacted>
To: Scott Wood <redacted>
Cc: socketcan-core@lists.berlios.de,
Cc: netdev@vger.kernel.org,
Cc: PPC list <redacted>
Cc: Kumar Gala <redacted>
---
.../devicetree/bindings/net/can/fsl-flexcan.txt    |   61 =

----------

quoted

----------

quoted

arch/powerpc/boot/dts/p1010rdb.dts                 |    8 ---
arch/powerpc/boot/dts/p1010si.dtsi                 |    8 +-
3 files changed, 4 insertions(+), 73 deletions(-)  delete mode
100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
=20
diff --git
a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
deleted file mode 100644
index 1a729f0..0000000

--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ /dev/null

@@ -1,61 +0,0 @@
-CAN Device Tree Bindings
-------------------------
-2011 Freescale Semiconductor, Inc.
-
-fsl,flexcan-v1.0 nodes
------------------------
-In addition to the required compatible-, reg- and

interrupt-properties, you can -also specify which clock source =

shall

quoted

be used for the controller.

quoted

-
-CPI Clock- Can Protocol Interface Clock
-	This CLK_SRC bit of CTRL(control register) selects the clock =

source

quoted

to

quoted

-	the CAN Protocol Interface(CPI) to be either the peripheral =

clock

quoted

-	(driven by the PLL) or the crystal oscillator clock. The =

selected

quoted

clock

quoted

-	is the one fed to the prescaler to generate the Serial Clock

(Sclock).

quoted

-	The PRESDIV field of CTRL(control register) controls a prescaler

that

quoted

-	generates the Serial Clock (Sclock), whose period defines the
-	time quantum used to compose the CAN waveform.
-
-Can Engine Clock Source
-	There are two sources for CAN clock
-	- Platform Clock  It represents the bus clock
-	- Oscillator Clock
-
-	Peripheral Clock (PLL)
-	--------------
-		     |
-		    ---------		      -------------
-		    |       |CPI Clock	      | Prescaler |       Sclock
-		    |       |---------------->| (1.. 256) |------------>
-		    ---------		      -------------
-                     |  |
-	--------------  ---------------------CLK_SRC
-	Oscillator Clock
-
-- fsl,flexcan-clock-source : CAN Engine Clock Source.This =

property

quoted

selects

quoted

-			     the peripheral clock. PLL clock is fed to =

the

quoted
quoted
quoted
quoted
-			     prescaler to generate the Serial Clock =

(Sclock).

quoted
quoted
quoted
quoted
-			     Valid values are "oscillator" and =

"platform"

quoted

-			     "oscillator": CAN engine clock source is

oscillator clock.

quoted

-			     "platform" The CAN engine clock source is =

the bus

quoted

clock

quoted

-		             (platform clock).
-
-- fsl,flexcan-clock-divider : for the reference and system clock, =

an

quoted

additional

quoted

-			      clock divider can be specified.
-- clock-frequency: frequency required to calculate the bitrate =

for

quoted

FlexCAN.

quoted

-
-Note:
-	- v1.0 of flexcan-v1.0 represent the IP block version for P1010

SOC.

quoted

-	- P1010 does not have oscillator as the Clock Source.So the =

default

quoted

-	  Clock Source is platform clock.
-Examples:
-
-	can0@1c000 {
-		compatible =3D "fsl,flexcan-v1.0";
-		reg =3D <0x1c000 0x1000>;
-		interrupts =3D <48 0x2>;
-		interrupt-parent =3D <&mpic>;
-		fsl,flexcan-clock-source =3D "platform";
-		fsl,flexcan-clock-divqider =3D <2>;
-		clock-frequency =3D <fixed by u-boot>;
-	};

=20
Do we really want to drop the documentation for that binding. I =

think

quoted

something like the following text would be still useful:
=20
------------------------
Flexcan CAN contoller on Freescale's ARM and PowerPC processors
=20
Required properties:
=20
- compatible : Should be "fsl,flexcan" and optionally
              "fsl,flexcan-<processor>"
- reg : Offset and length of the register set for this device
- interrupts : Interrupt tuple for this device
=20
Example:
=20
 can@1c000 {
         compatible =3D "fsl,p1010-flexcan", "fsl,flexcan";
         reg =3D <0x1c000 0x1000>;
         interrupts =3D <48 0x2>;
         interrupt-parent =3D <&mpic>;
 };
-------------------------

=20
Done, except the

quoted

         compatible =3D "fsl,p1010-flexcan", "fsl,flexcan";

=20
line is
	compatible =3D "fsl,flexcan", "fsl,flexcan-p1010";
=20

quoted

=20
What do you think?
=20

quoted

diff --git a/arch/powerpc/boot/dts/p1010rdb.dts

b/arch/powerpc/boot/dts/p1010rdb.dts
index 6b33b73..d6a0bb2 100644

--- a/arch/powerpc/boot/dts/p1010rdb.dts
+++ b/arch/powerpc/boot/dts/p1010rdb.dts

@@ -169,14 +169,6 @@

			};
		};
=20
-		can0@1c000 {
-			fsl,flexcan-clock-source =3D "platform";
-		};
-
-		can1@1d000 {
-			fsl,flexcan-clock-source =3D "platform";
-		};
-
		usb@22000 {
			phy_type =3D "utmi";
		};

diff --git a/arch/powerpc/boot/dts/p1010si.dtsi

b/arch/powerpc/boot/dts/p1010si.dtsi
index 7f51104..20c396d 100644

--- a/arch/powerpc/boot/dts/p1010si.dtsi
+++ b/arch/powerpc/boot/dts/p1010si.dtsi

@@ -141,19 +141,19 @@

		};
=20
		can0@1c000 {
-			compatible =3D "fsl,flexcan-v1.0";
+			compatible =3D "fsl,p1010-flexcan",
+					"fsl,flexcan";

=20
Does fit on one line.
=20

quoted

			reg =3D <0x1c000 0x1000>;
			interrupts =3D <48 0x2>;
			interrupt-parent =3D <&mpic>;
-			fsl,flexcan-clock-divider =3D <2>;
		};
=20
		can1@1d000 {
-			compatible =3D "fsl,flexcan-v1.0";
+			compatible =3D "fsl,p1010-flexcan",
+					"fsl,flexcan";

=20
Ditto
=20

quoted

			reg =3D <0x1d000 0x1000>;
			interrupts =3D <61 0x2>;
			interrupt-parent =3D <&mpic>;
-			fsl,flexcan-clock-divider =3D <2>;
		};
=20
		L2: l2-cache-controller@20000 {

=20
Please also correct the node names (not using the number suffix).

=20
So the node names should be
		can@1c000 {
		can@1d000 {
correct?
=20

[Bhaskar] As there are two CAN controllers on P1010,So won't it be =

better

quoted

	    to distinguish it by can0 and can1 instead by simple "can" ?

=20
It looks like the way to do that is to assign a label to those devices
and then associate the label with an alias.  I have no idea how that
works under the hood, but it is the way other files are set up.  Take =

look at arch/powerpc/boot/dts/bamboo.dts for how they define the =

serial

interfaces.
=20
Grant or Wolfgang, is that the right way to handle the concern about
names or does it have no practical effect with the Linux kernel?

It has not effect.  The label is just if you need to reference it via =
some other means.

- k=

`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