-----Original Message-----
From: Shawn Guo [mailto:shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
Sent: Tuesday, March 14, 2017 9:08 AM
On Wed, Mar 01, 2017 at 05:35:04PM +0200, Madalin Bucur wrote:

quoted

Signed-off-by: Madalin Bucur <redacted>

Empty commit log is generally not welcome, especially when the commit
adds so many files.

Hi Shawn, thank you for your time. I'll split this into several separate
patches, with better commit logs.

quoted

---
 arch/arm64/boot/dts/freescale/fsl-ls1043-post.dtsi | 41 +++++++++++
 arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts  |  2 +
 arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts  | 75

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

quoted

 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi     | 73

+++++++++++++++++++-

quoted

 .../boot/dts/freescale/qoriq-bman1-portals.dtsi    | 67

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

quoted

 .../boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi    | 42 ++++++++++++
 .../boot/dts/freescale/qoriq-fman3-0-1g-0.dtsi     | 41 +++++++++++
 .../boot/dts/freescale/qoriq-fman3-0-1g-1.dtsi     | 41 +++++++++++
 .../boot/dts/freescale/qoriq-fman3-0-1g-2.dtsi     | 41 +++++++++++
 .../boot/dts/freescale/qoriq-fman3-0-1g-3.dtsi     | 41 +++++++++++
 .../boot/dts/freescale/qoriq-fman3-0-1g-4.dtsi     | 41 +++++++++++
 .../boot/dts/freescale/qoriq-fman3-0-1g-5.dtsi     | 41 +++++++++++
 arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi   | 80

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

quoted

 .../boot/dts/freescale/qoriq-qman1-portals.dtsi    | 76

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

quoted

 14 files changed, 701 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1043-post.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-bman1-

portals.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-

0.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-

0.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-

1.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-

2.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-

3.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-

4.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-

5.dtsi

quoted

 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/qoriq-qman1-

portals.dtsi

I'm not comfortable with so many and complex include level.  Can you
please elaborate it a bit and help us understand the necessity of doing
that?

The DPAA (Data Path Acceleration Architecture) 1.x was first introduced
on PowerPC QorIQ devices, where it was included in more than ten SoCs,
each with its own particular implementation of it. There are SoCs that
implement one FMan, others with two, some have none, some have one or
two 10G ports. In order to allow reuse and reduce redundancy, the DPAA
PPC component nodes were split into separate files a long time ago.
I'm just carrying over this layout to the DPAA 1.x ARM platforms.

quoted

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043-post.dtsi

b/arch/arm64/boot/dts/freescale/fsl-ls1043-post.dtsi

quoted

new file mode 100644
index 0000000..bf443d2

--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043-post.dtsi

@@ -0,0 +1,41 @@
+/*
+ * QorIQ FMan v3 device tree nodes for ls1043
+ *
+ * Copyright 2015-2016 Freescale Semiconductor Inc.
+ *
+ * SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+ */

It's still controversial whether we should use SPDX tag.

https://lkml.org/lkml/2017/2/28/750

We're already making use of SPDX tags for u-boot updates and
internally this was accepted already. The remaining question is
if the SPDX tag use is or is not welcome in the kernel tree.

quoted

+
+&soc {
+
+/include/ "qoriq-fman3-0.dtsi"
+/include/ "qoriq-fman3-0-1g-0.dtsi"
+/include/ "qoriq-fman3-0-1g-1.dtsi"
+/include/ "qoriq-fman3-0-1g-2.dtsi"
+/include/ "qoriq-fman3-0-1g-3.dtsi"
+/include/ "qoriq-fman3-0-1g-4.dtsi"
+/include/ "qoriq-fman3-0-1g-5.dtsi"
+/include/ "qoriq-fman3-0-10g-0.dtsi"

We usually put the includes at the beginning of the file, and #include
is more recommended than /include/.

I'm not making use of the header file inclusion feature #include provides
(nor plan to) in these files thus I've selected /include/ here.

quoted

+	fman@1a00000 {
+		enet0: ethernet@e0000 {
+		};
+
+		enet1: ethernet@e2000 {
+		};
+
+		enet2: ethernet@e4000 {
+		};
+
+		enet3: ethernet@e6000 {
+		};
+
+		enet4: ethernet@e8000 {
+		};
+
+		enet5: ethernet@ea000 {
+		};
+
+		enet6: ethernet@f0000 {
+		};
+	};

I do not quite understand why these nodes are empty.

These nodes provide the aliases (and custom SoC mapping) for the
FMan ports that are used on this particular SoC. The particular
node details are found in the port dtsi file thus no information
is required here. Given the fact that the numbering and actual
ports that are in use can vary between SoCs, the aliases cannot
be included in the port dtsi nor in the FMan dtsi.

quoted

+};

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts

b/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts

quoted

index 0989d63..ee66bb2 100644

--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts

@@ -181,3 +181,5 @@
 		reg = <0>;
 	};
 };
+
+/include/ "fsl-ls1043-post.dtsi"

Move it to header of the file.

This is to be included at the end, to make sure the references are
met and to allow overrides if needed.

quoted

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts

b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts

quoted

index c37110b..d94f003 100644

--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts

@@ -139,3 +139,78 @@
 &duart1 {
 	status = "okay";
 };
+
+/include/ "fsl-ls1043-post.dtsi"
+

Ditto

quoted

+&soc {
+	fman@1a00000 {
+		ethernet@e0000 {

You defined enet0 label.  Why don't you use it?

The enet0 label is used by u-boot for fix-ups, providing the
actual offset here makes it easier to follow.

quoted

+			phy-handle = <&qsgmii_phy1>;
+			phy-connection-type = "qsgmii";
+		};
+
+		ethernet@e2000 {
+			phy-handle = <&qsgmii_phy2>;
+			phy-connection-type = "qsgmii";
+		};
+
+		ethernet@e4000 {
+			phy-handle = <&rgmii_phy1>;
+			phy-connection-type = "rgmii";
+		};
+
+		ethernet@e6000 {
+			phy-handle = <&rgmii_phy2>;
+			phy-connection-type = "rgmii";
+		};
+
+		ethernet@e8000 {
+			phy-handle = <&qsgmii_phy3>;
+			phy-connection-type = "qsgmii";
+		};
+
+		ethernet@ea000 {
+			phy-handle = <&qsgmii_phy4>;
+			phy-connection-type = "qsgmii";
+		};
+
+		ethernet@f0000 { /* 10GEC1 */
+			phy-handle = <&aqr105_phy>;
+			phy-connection-type = "xgmii";
+		};
+
+		mdio@fc000 {

Use label reference the node you already define, so that you do not have
to maintain the device tree hierarchy for referencing the node.

quoted

+			rgmii_phy1: ethernet-phy@1 {
+				reg = <0x1>;
+			};
+
+			rgmii_phy2: ethernet-phy@2 {
+				reg = <0x2>;
+			};
+
+			qsgmii_phy1: ethernet-phy@3 {
+				reg = <0x4>;

The unit address in the node name should match 'reg' value.

This needs to be addressed.

quoted

+			};
+
+			qsgmii_phy2: ethernet-phy@4 {
+				reg = <0x5>;
+			};
+
+			qsgmii_phy3: ethernet-phy@5 {
+				reg = <0x6>;
+			};
+
+			qsgmii_phy4: ethernet-phy@6 {
+				reg = <0x7>;
+			};
+		};
+
+		mdio@fd000 {
+			aqr105_phy: ethernet-phy@c {
+				compatible = "ethernet-phy-ieee802.3-c45";
+				interrupts = <0 132 4>;
+				reg = <0x1>;
+			};
+		};
+	};
+};

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi

b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi

quoted

index ec13a6e..ac1e0af 100644

--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi

@@ -52,6 +52,17 @@
 	#address-cells = <2>;
 	#size-cells = <2>;

+	aliases {
+		fman0 = &fman0;
+		ethernet0 = &enet0;
+		ethernet1 = &enet1;
+		ethernet2 = &enet2;
+		ethernet3 = &enet3;
+		ethernet4 = &enet4;
+		ethernet5 = &enet5;
+		ethernet6 = &enet6;
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;

@@ -106,6 +117,36 @@
 		      /* DRAM space 1, size: 2GiB DRAM */
 	};

+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		bman_fbpr: bman-fbpr {
+			size = <0 0x1000000>;
+			alignment = <0 0x1000000>;
+		};
+
+		qman_fqd: qman-fqd {
+			size = <0 0x400000>;
+			alignment = <0 0x400000>;
+		};
+
+		qman_pfdr: qman-pfdr {
+			size = <0 0x2000000>;
+			alignment = <0 0x2000000>;
+		};
+	};
+
+	bportals: bman-portals@508000000 {
+		ranges = <0x0 0x5 0x08000000 0x8000000>;
+	};
+
+	qportals: qman-portals@500000000 {
+		ranges = <0x0 0x5 0x00000000 0x8000000>;
+	};

Why are these two devices directly under root, not soc node?

We'll look into this during the patch split.

quoted

+
+
 	sysclk: sysclk {
 		compatible = "fixed-clock";
 		#clock-cells = <0>;

@@ -152,7 +193,7 @@
 		interrupts = <1 9 0xf08>;
 	};

-	soc {
+	soc: soc {
 		compatible = "simple-bus";
 		#address-cells = <2>;
 		#size-cells = <2>;

@@ -333,6 +374,18 @@
 			};
 		};

+		qman: qman@1880000 {
+			compatible = "fsl,qman";
+			reg = <0x00 0x1880000 0x0 0x10000>;
+			interrupts = <0 45 0x4>;
+		};
+
+		bman: bman@1890000 {
+			compatible = "fsl,bman";
+			reg = <0x00 0x1890000 0x0 0x10000>;
+			interrupts = <0 45 0x4>;
+		};
+
 		dspi0: dspi@2100000 {
 			compatible = "fsl,ls1043a-dspi", "fsl,ls1021a-v1.0-

dspi";

quoted

 			#address-cells = <1>;

@@ -686,3 +739,21 @@
 	};

 };
+
+&bman_fbpr {
+	compatible = "fsl,bman-fbpr";
+	alloc-ranges = <0 0 0x10000 0>;
+};
+
+&qman_fqd {
+	compatible = "fsl,qman-fqd";
+	alloc-ranges = <0 0 0x10000 0>;
+};
+
+&qman_pfdr {
+	compatible = "fsl,qman-pfdr";
+	alloc-ranges = <0 0 0x10000 0>;
+};

Why do you need to separate these from the nodes in reserved-memory?

I stop right here.  Honestly, I do not like the patch as its current
form.  I guess you should do something to make the reviewing of the
changes a bit easier.

Shawn

These will be moved there. The new patch set will allow easier reviewing
by splitting the changes into multiple patches.

Madalin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html