[PATCH v2 0/3] Add DTS for SDM845 SoC and MTP

[PATCH v2 0/3] Add DTS for SDM845 SoC and MTP

[Rajendra Nayak]

Now that we have serial[1], pinctrl[2] and clock[3]
drivers for SDM845 SoC out on the lists, heres the
basic dts files needed to boot a SDM845 based MTP
board to a ramfs based serial console shell.

[1] https://patchwork.ozlabs.org/cover/860251/
[2] https://patchwork.kernel.org/patch/10157143/
[3] https://lkml.org/lkml/2018/1/22/78

changes in v2:
* dropped cpu-map
* dropped GIC_CPU_MASK_SIMPLE()
* Added new cpu compatible for kryo385
* added ITS node, marked as disabled
* other cosmetic fixes 

Rajendra Nayak (3):
  Documentation: dt-bindings: arm: Document kryo385 cpu
  arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP
  arm64: dts: sdm845: Add serial console support

 Documentation/devicetree/bindings/arm/cpus.txt |   1 +
 arch/arm64/boot/dts/qcom/Makefile              |   1 +
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts        |  55 +++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi           | 298 +++++++++++++++++++++++++
 4 files changed, 355 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
 create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 1/3] Documentation: dt-bindings: arm: Document kryo385 cpu

[Rajendra Nayak]

Document the compatible string for the Kryo385 cpus found in qualcomm
SoCs.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index a0009b72e9be..19611ccb61d9 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -184,6 +184,7 @@ described below.
 			    "nvidia,tegra186-denver"
 			    "qcom,krait"
 			    "qcom,kryo"
+			    "qcom,kryo385"
 			    "qcom,scorpion"
 	- enable-method
 		Value type: <stringlist>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/3] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

[Rajendra Nayak]

Add a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/Makefile       |   1 +
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts |  13 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi    | 277 ++++++++++++++++++++++++++++++++
 3 files changed, 291 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
 create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi

diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 55ec5ee7f7e8..9319e74b8906 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -6,3 +6,4 @@ dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-mtp.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= msm8992-bullhead-rev-101.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-angler-rev-101.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-mtp.dtb
+dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-mtp.dtb
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
new file mode 100644
index 000000000000..617c7bb25fb1
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+/dts-v1/;
+
+#include "sdm845.dtsi"
+
+/ {
+	model = "Qualcomm Technologies, Inc. SDM845 MTP";
+	compatible = "qcom,sdm845-mtp";
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
new file mode 100644
index 000000000000..02520f19e4ca
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -0,0 +1,277 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	model = "Qualcomm Technologies, Inc. SDM845";
+
+	interrupt-parent = <&intc>;
+
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	chosen { };
+
+	memory {
+		device_type = "memory";
+		/* We expect the bootloader to fill in the reg */
+		reg = <0 0 0 0>;
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		CPU0: cpu at 0 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+			next-level-cache = <&L2_0>;
+			L2_0: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+				L3_0: l3-cache {
+				      compatible = "cache";
+				};
+			};
+		};
+
+		CPU1: cpu at 100 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+			next-level-cache = <&L2_100>;
+			L2_100: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU2: cpu at 200 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x200>;
+			enable-method = "psci";
+			next-level-cache = <&L2_200>;
+			L2_200: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU3: cpu at 300 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x300>;
+			enable-method = "psci";
+			next-level-cache = <&L2_300>;
+			L2_300: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU4: cpu at 400 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x400>;
+			enable-method = "psci";
+			next-level-cache = <&L2_400>;
+			L2_400: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU5: cpu at 500 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x500>;
+			enable-method = "psci";
+			next-level-cache = <&L2_500>;
+			L2_500: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU6: cpu at 600 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x600>;
+			enable-method = "psci";
+			next-level-cache = <&L2_600>;
+			L2_600: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU7: cpu at 700 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x700>;
+			enable-method = "psci";
+			next-level-cache = <&L2_700>;
+			L2_700: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 1 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_PPI 2 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_PPI 3 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_PPI 0 IRQ_TYPE_LEVEL_LOW>;
+	};
+
+	clocks {
+		xo_board: xo_board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <19200000>;
+		};
+
+		sleep_clk: sleep_clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32764>;
+		};
+	};
+
+	psci {
+		compatible = "arm,psci-1.0";
+		method = "smc";
+	};
+
+	soc {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0 0 0xffffffff>;
+		compatible = "simple-bus";
+
+		intc: interrupt-controller at 17a00000 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "arm,gic-v3";
+			#interrupt-cells = <3>;
+			interrupt-controller;
+			#redistributor-regions = <1>;
+			redistributor-stride = <0x0 0x20000>;
+			reg = <0x17a00000 0x10000>,     /* GICD */
+			      <0x17a60000 0x100000>;    /* GICR * 8 */
+			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+
+			gic-its at 17a40000 {
+				compatible = "arm,gic-v3-its";
+				msi-controller;
+				#msi-cells = <1>;
+				reg = <0x17a40000 0x20000>;
+				status = "disabled";
+			};
+		};
+
+		gcc: clock-controller at 100000 {
+			compatible = "qcom,gcc-sdm845";
+			reg = <0x100000 0x1f0000>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		tlmm: pinctrl at 3400000 {
+			compatible = "qcom,sdm845-pinctrl";
+			reg = <0x03400000 0xc00000>;
+			interrupts = <GIC_SPI 208 IRQ_TYPE_NONE>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		timer at 17c90000 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			compatible = "arm,armv7-timer-mem";
+			reg = <0x17c90000 0x1000>;
+
+			frame at 17ca0000 {
+				frame-number = <0>;
+				interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17ca0000 0x1000>,
+				      <0x17cb0000 0x1000>;
+			};
+
+			frame at 17cc0000 {
+				frame-number = <1>;
+				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17cc0000 0x1000>;
+				status = "disabled";
+			};
+
+			frame at 17cd0000 {
+				frame-number = <2>;
+				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17cd0000 0x1000>;
+				status = "disabled";
+			};
+
+			frame at 17ce0000 {
+				frame-number = <3>;
+				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17ce0000 0x1000>;
+				status = "disabled";
+			};
+
+			frame at 17cf0000 {
+				frame-number = <4>;
+				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17cf0000 0x1000>;
+				status = "disabled";
+			};
+
+			frame at 17d00000 {
+				frame-number = <5>;
+				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17d00000 0x1000>;
+				status = "disabled";
+			};
+
+			frame at 17d10000 {
+				frame-number = <6>;
+				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
+				reg = <0x17d10000 0x1000>;
+				status = "disabled";
+			};
+		};
+
+		spmi_bus: qcom,spmi at c440000 {
+			compatible = "qcom,spmi-pmic-arb";
+			reg = <0xc440000 0x1100>,
+			      <0xc600000 0x2000000>,
+			      <0xe600000 0x100000>,
+			      <0xe700000 0xa0000>,
+			      <0xc40a000 0x26000>;
+			reg-names = "core", "chnls", "obsrvr", "intr", "cnfg";
+			interrupt-names = "periph_irq";
+			interrupts = <GIC_SPI 481 IRQ_TYPE_NONE>;
+			qcom,ee = <0>;
+			qcom,channel = <0>;
+			#address-cells = <2>;
+			#size-cells = <0>;
+			interrupt-controller;
+			#interrupt-cells = <4>;
+			cell-index = <0>;
+		};
+
+	};
+};
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 3/3] arm64: dts: sdm845: Add serial console support

[Rajendra Nayak]

Add the qup uart node and geni se instance needed to
support the serial console on the MTP.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 42 +++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi    | 21 +++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index 617c7bb25fb1..42fbf2ab9a2d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -10,4 +10,46 @@
 / {
 	model = "Qualcomm Technologies, Inc. SDM845 MTP";
 	compatible = "qcom,sdm845-mtp";
+
+	aliases {
+		serial0 = &qup_uart2;
+	};
+
+	chosen {
+		stdout-path = "serial0";
+	};
+
+	soc {
+		serial at a84000 {
+			status = "okay";
+		};
+
+		pinctrl at 3400000 {
+			qup_uart2_default: qup_uart2_default {
+				pinmux {
+					function = "qup9";
+					pins = "gpio4", "gpio5";
+				};
+
+				pinconf {
+					pins = "gpio4", "gpio5";
+					drive-strength = <2>;
+					bias-disable;
+				};
+			};
+
+			qup_uart2_sleep: qup_uart2_sleep {
+				pinmux {
+					function = "gpio";
+					pins = "gpio4", "gpio5";
+				};
+
+				pinconf {
+					pins = "gpio4", "gpio5";
+					drive-strength = <2>;
+					bias-disable;
+				};
+			};
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 02520f19e4ca..c4ce70840acf 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4,6 +4,7 @@
  */
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/clock/qcom,gcc-sdm845.h>
 
 / {
 	model = "Qualcomm Technologies, Inc. SDM845";
@@ -273,5 +274,25 @@
 			cell-index = <0>;
 		};
 
+		qup_1: qcom,geni_se at ac0000 {
+			compatible = "qcom,geni-se-qup";
+			reg = <0xac0000 0x6000>;
+		};
+
+		qup_uart2: serial at a84000 {
+			compatible = "qcom,geni-console", "qcom,geni-uart";
+			reg = <0xa84000 0x4000>;
+			reg-names = "se_phys";
+			clock-names = "se-clk", "m-ahb", "s-ahb";
+			clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>,
+				 <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>,
+				 <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>;
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&qup_uart2_default>;
+			pinctrl-1 = <&qup_uart2_sleep>;
+			interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>;
+			qcom,wrapper-core = <&qup_1>;
+			status = "disabled";
+		};
 	};
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 1/3] Documentation: dt-bindings: arm: Document kryo385 cpu

[Rob Herring]

On Wed, Jan 31, 2018 at 09:49:39PM +0530, Rajendra Nayak wrote:
> Document the compatible string for the Kryo385 cpus found in qualcomm
> SoCs.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>  1 file changed, 1 insertion(+)

"dt-bindings: arm: " for the subject prefix. Otherwise,

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 3/3] arm64: dts: sdm845: Add serial console support

[Doug Anderson]

Hi,

On Wed, Jan 31, 2018 at 8:19 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> Add the qup uart node and geni se instance needed to
> support the serial console on the MTP.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 42 +++++++++++++++++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sdm845.dtsi    | 21 +++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 617c7bb25fb1..42fbf2ab9a2d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -10,4 +10,46 @@
>  / {
>         model = "Qualcomm Technologies, Inc. SDM845 MTP";
>         compatible = "qcom,sdm845-mtp";
> +
> +       aliases {
> +               serial0 = &qup_uart2;
> +       };
> +
> +       chosen {
> +               stdout-path = "serial0";
> +       };
> +
> +       soc {

I don't know if there's an official position, but in general I'm seen
people use the actual "soc" alias here.  AKA at the top level of this
dts, just do:

&soc {
  ...
};

Normally doing stuff like that is useful so you don't need to
replicate the whole hierarchy.  In this case that's not a huge
savings, but it can be nice to be consistent.  In the very least it
saves you a level of indentation.


> +               serial at a84000 {
> +                       status = "okay";
> +               };

Similarly here you can use the alias from the sdm845.dtsi file to
avoid replicating the hierarchy.  AKA at the top level do:

&qup_uart2 {
  status = "okay";
};

In this case it saves you 2 levels of indentation.

> +
> +               pinctrl at 3400000 {
> +                       qup_uart2_default: qup_uart2_default {

I'm not sure how persnickety I should be, but according to
<https://elinux.org/Device_Tree_Linux>:

  node names use dash "-" instead of underscore "_"

...but, of course, labels can't use dashes (and the same page says to
use underscore for labels).  This is why, in rk3288 for instance, you
see:

i2c2_xfer: i2c2-xfer {
  rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
  <6 10 RK_FUNC_1 &pcfg_pull_none>;
};

AKA the label and the node name are the same but the label uses "_"
and the node names use "-".


> +                               pinmux {
> +                                       function = "qup9";
> +                                       pins = "gpio4", "gpio5";
> +                               };
> +
> +                               pinconf {
> +                                       pins = "gpio4", "gpio5";
> +                                       drive-strength = <2>;
> +                                       bias-disable;
> +                               };
> +                       };
> +
> +                       qup_uart2_sleep: qup_uart2_sleep {
> +                               pinmux {
> +                                       function = "gpio";
> +                                       pins = "gpio4", "gpio5";
> +                               };
> +
> +                               pinconf {
> +                                       pins = "gpio4", "gpio5";
> +                                       drive-strength = <2>;
> +                                       bias-disable;
> +                               };
> +                       };
> +               };

As per my (admittedly tardy) response to v1, these shouldn't be in the
board file because every board that happens to need uart2 will need to
duplicate these definitions.


> +       };
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 02520f19e4ca..c4ce70840acf 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -4,6 +4,7 @@
>   */
>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,gcc-sdm845.h>
>
>  / {
>         model = "Qualcomm Technologies, Inc. SDM845";
> @@ -273,5 +274,25 @@
>                         cell-index = <0>;
>                 };
>
> +               qup_1: qcom,geni_se at ac0000 {
> +                       compatible = "qcom,geni-se-qup";
> +                       reg = <0xac0000 0x6000>;

I think you may have mentioned this in another context, but this
doesn't match the current bindings.  Some clocks should be here.
...and it looks like the uart should be a subnode.


> +               };
> +
> +               qup_uart2: serial at a84000 {
> +                       compatible = "qcom,geni-console", "qcom,geni-uart";
> +                       reg = <0xa84000 0x4000>;
> +                       reg-names = "se_phys";
> +                       clock-names = "se-clk", "m-ahb", "s-ahb";
> +                       clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>,
> +                                <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>,
> +                                <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>;
> +                       pinctrl-names = "default", "sleep";
> +                       pinctrl-0 = <&qup_uart2_default>;
> +                       pinctrl-1 = <&qup_uart2_sleep>;

In general the "SoC" dtsi file shouldn't be relying on definitions
that are made in the board "dts" file.

...if everyone disagrees with me and we really decide to duplicate all
the pinctrl in all board files, we'll need to move the
"pinctrl-names", "pinctrl-0", and "pinctrl-1" to the board files too.
;)


-Doug

[PATCH v2 2/3] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

[Doug Anderson]

Hi,

On Wed, Jan 31, 2018 at 8:19 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> new file mode 100644
> index 000000000000..02520f19e4ca
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +       model = "Qualcomm Technologies, Inc. SDM845";

I'm fairly certain that "model" doesn't belong in the SoC .dtsi file.
Only in the board .dts file.


> +       clocks {
> +               xo_board: xo_board {

Just to make it explicit: see my comments in patch 3/3 in this series
about using "_" in node names.  I believe this should be:

  xo_board: xo-board {


> +               spmi_bus: qcom,spmi at c440000 {

Drop the qcom in the node name.  AKA, I believe this should be:

spmi_bus: spmi at c440000 {

Specifically the node name is supposed to be a generic component name
then with an address.  I see that Rob Herring said the same thing when
he reviewed v1 of this patch just now (it seems like people are still
commenting there, so make sure you collect the latest feedback from
there when re-spinning).


-Doug

[PATCH v2 3/3] arm64: dts: sdm845: Add serial console support

[Rajendra Nayak]

[]..

>> @@ -10,4 +10,46 @@
>>  / {
>>         model = "Qualcomm Technologies, Inc. SDM845 MTP";
>>         compatible = "qcom,sdm845-mtp";
>> +
>> +       aliases {
>> +               serial0 = &qup_uart2;
>> +       };
>> +
>> +       chosen {
>> +               stdout-path = "serial0";
>> +       };
>> +
>> +       soc {
> 
> I don't know if there's an official position, but in general I'm seen
> people use the actual "soc" alias here.  AKA at the top level of this
> dts, just do:
> 
> &soc {
>   ...
> };
> 
> Normally doing stuff like that is useful so you don't need to
> replicate the whole hierarchy.  In this case that's not a huge
> savings, but it can be nice to be consistent.  In the very least it
> saves you a level of indentation.
> 
> 
>> +               serial at a84000 {
>> +                       status = "okay";
>> +               };
> 
> Similarly here you can use the alias from the sdm845.dtsi file to
> avoid replicating the hierarchy.  AKA at the top level do:
> 
> &qup_uart2 {
>   status = "okay";
> };
> 
> In this case it saves you 2 levels of indentation.

Right. Andy/Bjorn, are there any preferences here?
I see we don't do this for the other board files, and I not sure
theres a specific reasoning for how its currently done and if we
need to stick to it.

> 
>> +
>> +               pinctrl at 3400000 {
>> +                       qup_uart2_default: qup_uart2_default {
> 
> I'm not sure how persnickety I should be, but according to
> <https://elinux.org/Device_Tree_Linux>:
> 
>   node names use dash "-" instead of underscore "_"
> 
> ...but, of course, labels can't use dashes (and the same page says to
> use underscore for labels).  This is why, in rk3288 for instance, you
> see:
> 
> i2c2_xfer: i2c2-xfer {
>   rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
>   <6 10 RK_FUNC_1 &pcfg_pull_none>;
> };
> 
> AKA the label and the node name are the same but the label uses "_"
> and the node names use "-".

Sure, I'll fix these up.

[]
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 02520f19e4ca..c4ce70840acf 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -4,6 +4,7 @@
>>   */
>>
>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-sdm845.h>
>>
>>  / {
>>         model = "Qualcomm Technologies, Inc. SDM845";
>> @@ -273,5 +274,25 @@
>>                         cell-index = <0>;
>>                 };
>>
>> +               qup_1: qcom,geni_se at ac0000 {
>> +                       compatible = "qcom,geni-se-qup";
>> +                       reg = <0xac0000 0x6000>;
> 
> I think you may have mentioned this in another context, but this
> doesn't match the current bindings.  Some clocks should be here.
> ...and it looks like the uart should be a subnode.

right, these were tested with the v1 set for serial. I will update them.

regards
Rajendra

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 2/3] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

[Rajendra Nayak]

On 02/07/2018 03:25 AM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jan 31, 2018 at 8:19 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> new file mode 100644
>> index 000000000000..02520f19e4ca
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -0,0 +1,277 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> +       model = "Qualcomm Technologies, Inc. SDM845";
> 
> I'm fairly certain that "model" doesn't belong in the SoC .dtsi file.
> Only in the board .dts file.
> 
> 
>> +       clocks {
>> +               xo_board: xo_board {
> 
> Just to make it explicit: see my comments in patch 3/3 in this series
> about using "_" in node names.  I believe this should be:
> 
>   xo_board: xo-board {
> 
> 
>> +               spmi_bus: qcom,spmi at c440000 {
> 
> Drop the qcom in the node name.  AKA, I believe this should be:
> 
> spmi_bus: spmi at c440000 {
> 
> Specifically the node name is supposed to be a generic component name
> then with an address.  I see that Rob Herring said the same thing when
> he reviewed v1 of this patch just now (it seems like people are still
> commenting there, so make sure you collect the latest feedback from
> there when re-spinning).

yes, I'll make sure I fix up based on Robs' review of the v1.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation