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

-----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;
netdev@vger.kernel.org; Kumar Gala; socketcan-core@lists.berlios.de; PPC
list
Subject: Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree
binding.

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

quoted

Hi Robin,

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

Your first sentence could be misleading. Please just describe what the
patch does and why, something like:

"This patch cleans up the documentation of the device-tree binding for
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.

I borrowed heavily from your message. ;)

quoted

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.

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

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

be used for the controller.

quoted

quoted

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

to

quoted

quoted

-	the CAN Protocol Interface(CPI) to be either the peripheral clock
-	(driven by the PLL) or the crystal oscillator clock. The selected

clock

quoted

quoted

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

(Sclock).

quoted

quoted

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

that

quoted

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

selects

quoted

quoted

-			     the peripheral clock. PLL clock is fed to the
-			     prescaler to generate the Serial Clock (Sclock).
-			     Valid values are "oscillator" and "platform"
-			     "oscillator": CAN engine clock source is

oscillator clock.

quoted

quoted

-			     "platform" The CAN engine clock source is the bus

clock

quoted

quoted

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

additional

quoted

quoted

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

FlexCAN.

quoted

quoted

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

SOC.

quoted

quoted

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

Do we really want to drop the documentation for that binding. I think
something like the following text would be still useful:

------------------------
Flexcan CAN contoller on Freescale's ARM and PowerPC processors

Required properties:

- 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

Example:

  can@1c000 {
          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
          reg = <0x1c000 0x1000>;
          interrupts = <48 0x2>;
          interrupt-parent = <&mpic>;
  };
-------------------------

Done, except the

quoted

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

line is
	compatible = "fsl,flexcan", "fsl,flexcan-p1010";

quoted

What do you think?

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

-		can0@1c000 {
-			fsl,flexcan-clock-source = "platform";
-		};
-
-		can1@1d000 {
-			fsl,flexcan-clock-source = "platform";
-		};
-
 		usb@22000 {
 			phy_type = "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 @@
 		};

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

Does fit on one line.

quoted

 			reg = <0x1c000 0x1000>;
 			interrupts = <48 0x2>;
 			interrupt-parent = <&mpic>;
-			fsl,flexcan-clock-divider = <2>;
 		};

 		can1@1d000 {
-			compatible = "fsl,flexcan-v1.0";
+			compatible = "fsl,p1010-flexcan",
+					"fsl,flexcan";

Ditto

quoted

 			reg = <0x1d000 0x1000>;
 			interrupts = <61 0x2>;
 			interrupt-parent = <&mpic>;
-			fsl,flexcan-clock-divider = <2>;
 		};

 		L2: l2-cache-controller@20000 {

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

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

[Bhaskar] As there are two CAN controllers on P1010,So won't it be better
	    to distinguish it by can0 and can1 instead by simple "can" ?

Robin