Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Rahul, Pankaj, Arun,

[adding linux-arm-kernel, devicetree MLs and DT people on Cc]

I think it's good time to stop accepting DTS files like this and force new ones to use the proper structure with soc node, labels for every node and node references.

In case of previous Exynos 5 SoCs I hadn't complained, because they shared a lot of data with already existing exynos5.dtsi, but since Exynos5260 is completely different, I'd say it should be converted to the new layout.

As an example you can look at arch/arm/boot/dts/s3c64xx.dtsi and files that include it or, for more complete structures, DTS of other platforms, such as imx6*.

Btw. Please remember that linux-samsung-soc mailing list is just a convenient utility for reviewers of Samsung-specific patches to have all of them in one place. Sending patches to it alone is not enough - a general kernel ML list needs to be CCed as well, in this case linux-arm-kernel.

Also, please see my comments inline, for review comments unrelated to the issue described above.

On 05.02.2014 06:16, Rahul Sharma wrote:
The patch adds the dts files for exynos5260.

Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
Signed-off-by: Rahul Sharma <rahul.sharma@xxxxxxxxxxx>
Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
---
  arch/arm/boot/dts/exynos5260-pinctrl.dtsi |  572 +++++++++++++++++++++++++++++
  arch/arm/boot/dts/exynos5260.dtsi         |  317 ++++++++++++++++
  2 files changed, 889 insertions(+)
  create mode 100644 arch/arm/boot/dts/exynos5260-pinctrl.dtsi
  create mode 100644 arch/arm/boot/dts/exynos5260.dtsi

diff --git a/arch/arm/boot/dts/exynos5260-pinctrl.dtsi b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
new file mode 100644
index 0000000..3f2c5c4
--- /dev/null
+++ b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
@@ -0,0 +1,572 @@
+/*
+ * Samsung's Exynos5260 SoC pin-mux and pin-config device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Samsung's Exynos5260 SoC pin-mux and pin-config options are listed as device
+ * tree nodes are listed in this file.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+/ {
+	pinctrl@11600000 {

[snip]

+		spi0_bus: spi0-bus {
+			samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2", "gpa2-3";

What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?

+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <0>;
+		};

[snip]

diff --git a/arch/arm/boot/dts/exynos5260.dtsi b/arch/arm/boot/dts/exynos5260.dtsi
new file mode 100644
index 0000000..32a95c7
--- /dev/null
+++ b/arch/arm/boot/dts/exynos5260.dtsi
@@ -0,0 +1,317 @@
+/*
+ * SAMSUNG EXYNOS5260 SoC device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include "skeleton.dtsi"
+#include "exynos5260-pinctrl.dtsi"
+
+#include <dt-bindings/clk/exynos5260-clk.h>
+
+/ {
+	compatible = "samsung,exynos5260";
+	interrupt-parent = <&gic>;
+
+	aliases {
+		pinctrl0 = &pinctrl_0;
+		pinctrl1 = &pinctrl_1;
+		pinctrl2 = &pinctrl_2;
+	};
+
+	chipid@10000000 {
+		compatible = "samsung,exynos4210-chipid";
+		reg = <0x10000000 0x100>;
+	};
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0>;

nit: Please make this consistent with CPUs 10x below, by using hex here as well.

+			cci-control-port = <&cci_control1>;
+		};

nit: Please keep 1 blank line of spacing between nodes.

+		cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <1>;
+			cci-control-port = <&cci_control1>;
+		};
+		cpu@100 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x100>;
+			cci-control-port = <&cci_control0>;
+		};
+		cpu@101 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x101>;
+			cci-control-port = <&cci_control0>;
+		};
+		cpu@102 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x102>;
+			cci-control-port = <&cci_control0>;
+		};
+		cpu@103 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a7";
+			reg = <0x103>;
+			cci-control-port = <&cci_control0>;
+		};
+	};
+
+	cmus {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+

I don't think there is a need to group these nodes under a parent node that doesn't give any additional information, especially when the CMUs are scattered trough the whole address space, while we'd like to keep the nodes ordered by their addresses, as most platforms do.

+		cmu_top: clock-controller@10010000 {
+			compatible = "exynos5260-cmu-top", "samsung,exynos5260-clock";

I don't think that having the "samsung,exynos5260-clock" compatible string for every CMU is appropriate here, because there is no way to automatically recognize which CMU it is. Since every CMU instance is different, they need to have different compatible strings.

+			reg = <0x10010000 0x10000>;
+			#clock-cells = <1>;
+		};

[snip]

+	mct@100B0000 {
+		compatible = "samsung,exynos4210-mct";
+		reg = <0x100B0000 0x1000>;
+		interrupt-controller;
+		#interrups-cells = <1>;

MCT is not an interrupt controller, so the 2 properties above are incorrect.

+		interrupt-parent = <&mct_map>;
+		interrupts = <0>, <1>, <2>, <3>,
+				<4>, <5>, <6>, <7>,
+				<8>, <9>, <10>, <11>;
+		clocks = <&cmu_top FIN_PLL>, <&cmu_peri PERI_CLK_MCT>;
+		clock-names = "fin_pll", "mct";
+
+		mct_map: mct-map {
+			#interrupt-cells = <1>;
+			#address-cells = <0>;
+			#size-cells = <0>;
+			interrupt-map = <0 &gic 0 104 0>,
+					<1 &gic 0 105 0>,
+					<2 &gic 0 106 0>,
+					<3 &gic 0 107 0>,
+					<4 &gic 0 122 0>,
+					<5 &gic 0 123 0>,
+					<6 &gic 0 124 0>,
+					<7 &gic 0 125 0>,
+					<8 &gic 0 126 0>,
+					<9 &gic 0 127 0>,
+					<10 &gic 0 128 0>,
+					<11 &gic 0 129 0>;
+		};

There is no need for interrupt-map here, because all the interrupts are from GIC.

+	};

[snip]

+	mmc_0: mmc0@12140000 {
+		compatible = "samsung,exynos5250-dw-mshc";
+		reg = <0x12140000 0x2000>;
+		interrupts = <0 156 0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&cmu_fsys FSYS_CLK_MMC0>, <&cmu_top TOP_SCLK_MMC0>;
+		clock-names = "biu", "ciu";
+		fifo-depth = <0x40>;

nit: It might be more readable to use decimal 64 here.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux