RE: [PATCH] Device Tree Bindings for Freescale TDM controller

Thanks Scott for the review.

Will send an updated revision with the comments taken care.

Regards
Poonam

-----Original Message-----
From: Wood Scott-B07421
Sent: Saturday, March 17, 2012 12:00 AM
To: Aggrwal Poonam-B10812
Cc: devicetree-discuss@lists.ozlabs.org; linuxppc-dev@lists.ozlabs.org;
Singh Sandeep-B37400
Subject: Re: [PATCH] Device Tree Bindings for Freescale TDM controller
=20
On 03/15/2012 08:30 PM, Poonam Aggrwal wrote:

quoted

From: Poonam Aggrwal <redacted>

This TDM controller is available in various Freescale SOCs like
MPC8315, P1020, P1022, P1010.

Signed-off-by: Sandeep Singh <redacted>
Signed-off-by: Poonam Aggrwal <redacted>
---
 Documentation/devicetree/bindings/tdm/fsl-tdm.txt |   71

+++++++++++++++++++++

quoted

 1 files changed, 71 insertions(+), 0 deletions(-)  create mode 100644
Documentation/devicetree/bindings/tdm/fsl-tdm.txt

diff --git a/Documentation/devicetree/bindings/tdm/fsl-tdm.txt

b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt
new file mode 100644
index 0000000..61431e3

--- /dev/null
+++ b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt

@@ -0,0 +1,71 @@
+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

quoted

+TDM Device Tree Binding
+Copyright (C) 2012 Freescale Semiconductor Inc.
+
+NOTE: The bindings described in this document are preliminary and
+subject to change.
+
+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

quoted

+TDM (Time Division Multiplexing)
+
+DESCRIPTION
+
+The TDM is full duplex serial port designed to allow various devices
+including digital signal processors (DSPs) to communicate with a
+variety of serial devices including industry standard framers, codecs,

other DSPs and microprocessors.

quoted

+
+The below properties describe the device tree bindings for Freescale
+TDM controller.
+This TDM controller is available on various Freescale Processors like
+MPC8313, P1020, P1022 and P1010.
+
+PROPERTIES
+
+  - compatible
+      Usage: required
+      Value type: <string>
+      Definition: Should contain "fsl,mpc8315-tdm".
+	  So mpc8313 will have compatible =3D "fsl,mpc8315-tdm";
+	  p1010 will have compatible "fsl,p1010-tdm", "fsl,mpc8315-tdm";

=20
Shouldn't mpc8313 have:
compatible =3D "fsl,mpc8313-tdm", "fsl,mpc8315-tdm"?
=20
I thought we were going to use 8313 as the canonical implementation, not
8315.

MPC8315 was the first FSL platform to have this controller.
MPC8313 does not have TDM.

=20

quoted

+  - reg
+      Usage: required
+      Value type: <tdm-reg-offset tdm-reg-size dmac-reg-offset dmac-

reg-size>

quoted

+      Definition: A standard property. Specifies the physical address
+	  offset and length of the TDM registers and TDM DMAC registers for
+	  the device.

=20
Just say there's two reg resources, and that the first is the TDM
registers and the second is the TDM DMAC registers.
=20
It's typically not going to be the actual physical address, but rather an
offset that gets translated through a parent node's ranges.

Okay, I think we missed this comment, you already gave this earlier.
Sorry for that.

=20
Remove "value type"; it's standard.
=20

Okay. So just definition must suffice, right?

quoted

+  - clock-frequency
+      Usage: optional
+      Value type: <u32>
+      Definition: The frequency at which the TDM block is operating.

=20
Will this frequency ever need to be > 4GHz?

Don't think so, at max this will be CCB, not sure if CCB on our platforms m=
ay get bigger than 4G ever.

=20
Might want to specify as u32 or u64, as ePAPR suggests.

Means Value type: <u32 or u64>?
In this case the driver must always use 64bit data structure to read this. =
Is this correct?

=20

quoted

+  - interrupts
+      Usage: required
+      Value type: <tdm-err-intr tdm-err-intr-type dmac-intr dmac-intr-

type>

quoted

+      Definition: This field defines two interrupt specifiers namely

interrupt

quoted

+	  number and interrupt type for TDM error and TDM DMAC.

=20
What is "tdm-err-intr-type"?  The interrupt specifier encoding is defined
by the interrupt controller.  There might be one cell, two cells, four
cells, etc.  Remove "value type", it's standard.
=20

okay

quoted

+  - phy-handle
+      Usage: optional
+      Value type: <phandle>
+      Definition: Phandle of the line controller node or framer node

eg. SLIC,

quoted

+	  E1\T1 etc.

=20
Use a forward slash -- this isn't a Windows filesystem path. :-)
=20

Okay, agreed.

quoted

+  - fsl-max-time-slots
+      Usage: required
+      Value type: <u32>
+      Definition: Maximum number of 8-bit time slots in one TDM frame.
+	  This is the maximum number which TDM hardware supports.

=20
fsl,tdm-max-time-slots

Sure. This again got missed.

=20

quoted

+
+EXAMPLE
+
+	tdm@16000 {
+		device_type =3D "tdm";

=20
No device_type

Okay.

=20

quoted

+		compatible =3D "fsl,p1010-tdm", "fsl,mpc8315-tdm";
+		reg =3D <0x16000 0x200 0x2c000 0x2000>;
+		clock-frequency =3D <0>;

=20
Show a real clock-frequency, perhaps with a comment saying it's typically
filled in by boot software.

Okay.

=20

quoted

+		interrupts =3D <16 8 62 8>;
+		phy-handle =3D <zarlink1>

=20
That phy-handle is invalid syntax, perhaps you meant:
=20
	phy-handle =3D <&zarlink1>;

Yes. Will correct it.

=20

quoted

+		fsl-max-time-slots =3D <128>

=20
Missing semicolons on the last two properties.
=20

Ok.

-Scott