2018-01-25 16:34:29

by Rajendra Nayak

[permalink] [raw]
Subject: [PATCH 0/2] Add DTS for SDM845 SoC and MTP

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

Rajendra Nayak (2):
arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP
arm64: dts: sdm845: Add serial console support

arch/arm64/boot/dts/qcom/Makefile | 1 +
arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++
arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 14 ++
arch/arm64/boot/dts/qcom/sdm845-pins.dtsi | 32 +++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 330 ++++++++++++++++++++++++++++++
5 files changed, 390 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
create mode 100644 arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
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



2018-01-25 16:33:27

by Rajendra Nayak

[permalink] [raw]
Subject: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

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

Signed-off-by: Rajendra Nayak <[email protected]>
---
arch/arm64/boot/dts/qcom/Makefile | 1 +
arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++
arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 11 ++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 308 +++++++++++++++++++++++++++++++
4 files changed, 333 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
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..c57701bed084 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..95e41e42bee1
--- /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-mtp.dtsi"
+
+/ {
+ model = "Qualcomm Technologies, Inc. SDM845 MTP";
+ compatible = "qcom,sdm845-mtp";
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
new file mode 100644
index 000000000000..5b1022c20bad
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include "sdm845.dtsi"
+
+/ {
+ soc {
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
new file mode 100644
index 000000000000..a21f4912b3e2
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -0,0 +1,308 @@
+// 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@0 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ 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@100 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x100>;
+ enable-method = "psci";
+ next-level-cache = <&L2_100>;
+ L2_100: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ CPU2: cpu@200 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x200>;
+ enable-method = "psci";
+ next-level-cache = <&L2_200>;
+ L2_200: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ CPU3: cpu@300 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x300>;
+ enable-method = "psci";
+ next-level-cache = <&L2_300>;
+ L2_300: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ CPU4: cpu@400 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x400>;
+ enable-method = "psci";
+ next-level-cache = <&L2_400>;
+ L2_400: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ CPU5: cpu@500 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x500>;
+ enable-method = "psci";
+ next-level-cache = <&L2_500>;
+ L2_500: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ CPU6: cpu@600 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x600>;
+ enable-method = "psci";
+ next-level-cache = <&L2_600>;
+ L2_600: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ CPU7: cpu@700 {
+ device_type = "cpu";
+ compatible = "qcom,kryo";
+ reg = <0x0 0x700>;
+ enable-method = "psci";
+ next-level-cache = <&L2_700>;
+ L2_700: l2-cache {
+ compatible = "cache";
+ next-level-cache = <&L3_0>;
+ };
+ };
+
+ cpu-map {
+ cluster0 {
+ core0 {
+ cpu = <&CPU0>;
+ };
+
+ core1 {
+ cpu = <&CPU1>;
+ };
+
+ core2 {
+ cpu = <&CPU2>;
+ };
+
+ core3 {
+ cpu = <&CPU3>;
+ };
+ };
+
+ cluster1 {
+ core0 {
+ cpu = <&CPU4>;
+ };
+
+ core1 {
+ cpu = <&CPU5>;
+ };
+
+ core2 {
+ cpu = <&CPU6>;
+ };
+
+ core3 {
+ cpu = <&CPU7>;
+ };
+ };
+ };
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 0 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
+ };
+
+ clocks {
+ xo_board: xo_board {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <19200000>;
+ clock-output-names = "xo_board";
+ };
+
+ sleep_clk: sleep_clk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32764>;
+ clock-output-names = "sleep_clk";
+ };
+ };
+
+ psci {
+ compatible = "arm,psci-1.0";
+ method = "smc";
+ };
+
+ soc: soc {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0 0 0xffffffff>;
+ compatible = "simple-bus";
+
+ intc: interrupt-controller@17a00000 {
+ 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>;
+ };
+
+ gcc: clock-controller@100000 {
+ compatible = "qcom,gcc-sdm845";
+ reg = <0x100000 0x1f0000>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ };
+
+ tlmm: pinctrl@03400000 {
+ 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@17C90000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ compatible = "arm,armv7-timer-mem";
+ reg = <0x17C90000 0x1000>;
+ clock-frequency = <19200000>;
+
+ frame@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@17cc0000 {
+ frame-number = <1>;
+ interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x17cc0000 0x1000>;
+ status = "disabled";
+ };
+
+ frame@17cd0000 {
+ frame-number = <2>;
+ interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x17cd0000 0x1000>;
+ status = "disabled";
+ };
+
+ frame@17ce0000 {
+ frame-number = <3>;
+ interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x17ce0000 0x1000>;
+ status = "disabled";
+ };
+
+ frame@17cf0000 {
+ frame-number = <4>;
+ interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x17cf0000 0x1000>;
+ status = "disabled";
+ };
+
+ frame@17d00000 {
+ frame-number = <5>;
+ interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x17d00000 0x1000>;
+ status = "disabled";
+ };
+
+ frame@17d10000 {
+ frame-number = <6>;
+ interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x17d10000 0x1000>;
+ status = "disabled";
+ };
+ };
+
+ spmi_bus: qcom,spmi@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


2018-01-25 16:35:04

by Rajendra Nayak

[permalink] [raw]
Subject: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

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

Signed-off-by: Rajendra Nayak <[email protected]>
---
This patch is based on the current proposed DT bindings for
the geni based serial driver [1] and also depends on the GCC
driver [2] which adds dt-bindings/clock/qcom,gcc-sdm845.h header.
This can only be merged once the dependent patches do.
[1] https://patchwork.ozlabs.org/cover/860251/
[2] https://lkml.org/lkml/2018/1/22/78

arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 3 +++
arch/arm64/boot/dts/qcom/sdm845-pins.dtsi | 32 +++++++++++++++++++++++++++++++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 22 +++++++++++++++++++++
3 files changed, 57 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/sdm845-pins.dtsi

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
index 5b1022c20bad..640a48cd628b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
@@ -7,5 +7,8 @@

/ {
soc {
+ serial@a84000 {
+ status = "okay";
+ };
};
};
diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
new file mode 100644
index 000000000000..b97f99e6f4b4
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+&tlmm {
+ 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 a21f4912b3e2..529f4ba3a1db 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";
@@ -304,5 +305,26 @@
cell-index = <0>;
};

+ qup_1: qcom,geni_se@ac0000 {
+ compatible = "qcom,geni-se-qup";
+ reg = <0xac0000 0x6000>;
+ };
+
+ qup_uart2: serial@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";
+ };
};
};
+#include "sdm845-pins.dtsi"
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


2018-01-26 20:33:28

by Evan Green

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

Hi Rajendra,

On Thu, Jan 25, 2018 at 8:32 AM, Rajendra Nayak <[email protected]> wrote:
> Add a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files
>
> Signed-off-by: Rajendra Nayak <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++
> arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 11 ++
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 308 +++++++++++++++++++++++++++++++
> 4 files changed, 333 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
> 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..c57701bed084 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

Minor nit: This should be a tab before the += to be consistent.

(Unfortunately I don't have the expertise quite yet to comment on the
rest of this).

-Evan

2018-01-26 22:15:46

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

On 01/25, Rajendra Nayak wrote:
> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi

Do we really need two files? Maybe collapse the two?

> create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> new file mode 100644
> index 000000000000..a21f4912b3e2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -0,0 +1,308 @@
> +// 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@0 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + 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@100 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x100>;
> + enable-method = "psci";
> + next-level-cache = <&L2_100>;
> + L2_100: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU2: cpu@200 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x200>;
> + enable-method = "psci";
> + next-level-cache = <&L2_200>;
> + L2_200: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU3: cpu@300 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x300>;
> + enable-method = "psci";
> + next-level-cache = <&L2_300>;
> + L2_300: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU4: cpu@400 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x400>;
> + enable-method = "psci";
> + next-level-cache = <&L2_400>;
> + L2_400: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU5: cpu@500 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x500>;
> + enable-method = "psci";
> + next-level-cache = <&L2_500>;
> + L2_500: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU6: cpu@600 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x600>;
> + enable-method = "psci";
> + next-level-cache = <&L2_600>;
> + L2_600: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU7: cpu@700 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x700>;
> + enable-method = "psci";
> + next-level-cache = <&L2_700>;
> + L2_700: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&CPU0>;
> + };
> +
> + core1 {
> + cpu = <&CPU1>;
> + };
> +
> + core2 {
> + cpu = <&CPU2>;
> + };
> +
> + core3 {
> + cpu = <&CPU3>;
> + };
> + };
> +
> + cluster1 {
> + core0 {
> + cpu = <&CPU4>;
> + };
> +
> + core1 {
> + cpu = <&CPU5>;
> + };
> +
> + core2 {
> + cpu = <&CPU6>;
> + };
> +
> + core3 {
> + cpu = <&CPU7>;
> + };
> + };
> + };

From what I recall, this layout causes the kernel to spew
warnings? I mean to say this is the power/performance view, but
not the architectural view.

> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,

Are we supposed to use the GIC_CPU_MASK_SIMPLE macros still?

> + <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 0 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
> + };
> +
> + clocks {
> + xo_board: xo_board {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <19200000>;
> + clock-output-names = "xo_board";

We can drop clock-output-names on these.

> + };
> +
> + sleep_clk: sleep_clk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32764>;
> + clock-output-names = "sleep_clk";
> + };
> + };
> +
> + psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> +
> + soc: soc {

Will anyone use this phandle?

> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0 0 0xffffffff>;
> + compatible = "simple-bus";
> +
> + intc: interrupt-controller@17a00000 {
> + 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>;

Can you also add the ITS node please and mark it as disabled?
I'll send a patch to the list to skip status = "disabled" ones.
We may want to support ITS on these SoCs if the firmware is
different.

> + };
> +
> + gcc: clock-controller@100000 {
> + compatible = "qcom,gcc-sdm845";
> + reg = <0x100000 0x1f0000>;
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> + };
> +
> + tlmm: pinctrl@03400000 {

Drop leading zeroes please.

> + 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@17C90000 {

Lowercase hex please.

> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + compatible = "arm,armv7-timer-mem";
> + reg = <0x17C90000 0x1000>;

Lowercase hex please.

> + clock-frequency = <19200000>;

Drop this? Or we can't read it from the hardware so we have to
hardcode it?

> +
> + frame@17CA0000 {

Lowecase again.

> + frame-number = <0>;
> + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17CA0000 0x1000>,
> + <0x17CB0000 0x1000>;
> + };
> +

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-01-26 22:18:49

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

On 01/25, Rajendra Nayak wrote:
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> new file mode 100644
> index 000000000000..b97f99e6f4b4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +&tlmm {

I'm not the maintainer, but I find this approach to the pins
really annoying. I have to flip to another file to figure out how
a board has configured the pins. And we may bring in a bunch of
settings that we don't ever use on some board too. Why can't we
put the settings in the board file directly?

> + 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 a21f4912b3e2..529f4ba3a1db 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";

I'd expect some sort of serial alias node stuff here too.

> @@ -304,5 +305,26 @@
> cell-index = <0>;
> };
>
> + qup_1: qcom,geni_se@ac0000 {
> + compatible = "qcom,geni-se-qup";
> + reg = <0xac0000 0x6000>;
> + };
> +
> + qup_uart2: serial@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>;

Is this binding still being reviewed? Ugly.

> + status = "disabled";
> + };
> };
> };
> +#include "sdm845-pins.dtsi"

Why at the bottom?

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-01-29 08:14:46

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP



On 01/27/2018 03:45 AM, Stephen Boyd wrote:
> On 01/25, Rajendra Nayak wrote:
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
>
> Do we really need two files? Maybe collapse the two?

will do. Not sure why, but this is how all other qualcomm
boards are structured with an almost empty .dts file.

>
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> new file mode 100644
>> index 000000000000..a21f4912b3e2
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -0,0 +1,308 @@
>> +// 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@0 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + 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@100 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x100>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_100>;
>> + L2_100: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU2: cpu@200 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x200>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_200>;
>> + L2_200: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU3: cpu@300 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x300>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_300>;
>> + L2_300: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU4: cpu@400 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x400>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_400>;
>> + L2_400: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU5: cpu@500 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x500>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_500>;
>> + L2_500: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU6: cpu@600 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x600>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_600>;
>> + L2_600: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU7: cpu@700 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x700>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_700>;
>> + L2_700: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + cpu-map {
>> + cluster0 {
>> + core0 {
>> + cpu = <&CPU0>;
>> + };
>> +
>> + core1 {
>> + cpu = <&CPU1>;
>> + };
>> +
>> + core2 {
>> + cpu = <&CPU2>;
>> + };
>> +
>> + core3 {
>> + cpu = <&CPU3>;
>> + };
>> + };
>> +
>> + cluster1 {
>> + core0 {
>> + cpu = <&CPU4>;
>> + };
>> +
>> + core1 {
>> + cpu = <&CPU5>;
>> + };
>> +
>> + core2 {
>> + cpu = <&CPU6>;
>> + };
>> +
>> + core3 {
>> + cpu = <&CPU7>;
>> + };
>> + };
>> + };
>
> From what I recall, this layout causes the kernel to spew
> warnings? I mean to say this is the power/performance view, but
> not the architectural view.

hmm, I haven't seen any warnings with this when I boot up on my sdm845
MTP. Will recheck.

>
>> + };
>> +
>> + timer {
>> + compatible = "arm,armv8-timer";
>> + interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>
> Are we supposed to use the GIC_CPU_MASK_SIMPLE macros still?

Not sure, is there another way?

>
>> + <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 0 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
>> + };
>> +
>> + clocks {
>> + xo_board: xo_board {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <19200000>;
>> + clock-output-names = "xo_board";
>
> We can drop clock-output-names on these.

will do.

>
>> + };
>> +
>> + sleep_clk: sleep_clk {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <32764>;
>> + clock-output-names = "sleep_clk";
>> + };
>> + };
>> +
>> + psci {
>> + compatible = "arm,psci-1.0";
>> + method = "smc";
>> + };
>> +
>> + soc: soc {
>
> Will anyone use this phandle?

maybe not, will drop.

>
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0 0 0 0xffffffff>;
>> + compatible = "simple-bus";
>> +
>> + intc: interrupt-controller@17a00000 {
>> + 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>;
>
> Can you also add the ITS node please and mark it as disabled?
> I'll send a patch to the list to skip status = "disabled" ones.
> We may want to support ITS on these SoCs if the firmware is
> different.

will add.

>
>> + };
>> +
>> + gcc: clock-controller@100000 {
>> + compatible = "qcom,gcc-sdm845";
>> + reg = <0x100000 0x1f0000>;
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + };
>> +
>> + tlmm: pinctrl@03400000 {
>
> Drop leading zeroes please.
>
>> + 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@17C90000 {
>
> Lowercase hex please.
>
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> + compatible = "arm,armv7-timer-mem";
>> + reg = <0x17C90000 0x1000>;
>
> Lowercase hex please.
>
>> + clock-frequency = <19200000>;
>
> Drop this? Or we can't read it from the hardware so we have to
> hardcode it?

will drop, shouldn't be needed.

>
>> +
>> + frame@17CA0000 {
>
> Lowecase again.
>
>> + frame-number = <0>;
>> + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
>> + reg = <0x17CA0000 0x1000>,
>> + <0x17CB0000 0x1000>;
>> + };
>> +
>

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

2018-01-29 08:19:30

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support



On 01/27/2018 03:48 AM, Stephen Boyd wrote:
> On 01/25, Rajendra Nayak wrote:
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> new file mode 100644
>> index 000000000000..b97f99e6f4b4
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +&tlmm {
>
> I'm not the maintainer, but I find this approach to the pins
> really annoying. I have to flip to another file to figure out how
> a board has configured the pins. And we may bring in a bunch of
> settings that we don't ever use on some board too. Why can't we
> put the settings in the board file directly?

I was just keeping it consistent with how things are for other
qualcomm platforms. I can move this to the board dts if no one else
sees any issues.

>
>> + 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 a21f4912b3e2..529f4ba3a1db 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";
>
> I'd expect some sort of serial alias node stuff here too.

yes, will add.

>
>> @@ -304,5 +305,26 @@
>> cell-index = <0>;
>> };
>>
>> + qup_1: qcom,geni_se@ac0000 {
>> + compatible = "qcom,geni-se-qup";
>> + reg = <0xac0000 0x6000>;
>> + };
>> +
>> + qup_uart2: serial@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>;
>
> Is this binding still being reviewed? Ugly.

yes, its still being reviewed

>
>> + status = "disabled";
>> + };
>> };
>> };
>> +#include "sdm845-pins.dtsi"
>
> Why at the bottom?

Again keeping it consistent with how things are in msm8916/msm8992/msm8996 dtsi files.

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

2018-01-30 08:50:24

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP


On 01/27/2018 02:01 AM, Evan Green wrote:
> Hi Rajendra,
>
> On Thu, Jan 25, 2018 at 8:32 AM, Rajendra Nayak <[email protected]> wrote:
>> Add a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files
>>
>> Signed-off-by: Rajendra Nayak <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/Makefile | 1 +
>> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++
>> arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 11 ++
>> arch/arm64/boot/dts/qcom/sdm845.dtsi | 308 +++++++++++++++++++++++++++++++
>> 4 files changed, 333 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
>> 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..c57701bed084 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
>
> Minor nit: This should be a tab before the += to be consistent.

thanks, will fix when I respin.

>
> (Unfortunately I don't have the expertise quite yet to comment on the
> rest of this).
>
> -Evan
>

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

2018-01-30 09:49:06

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

On 01/29, Rajendra Nayak wrote:
>
>
> On 01/27/2018 03:45 AM, Stephen Boyd wrote:
> > On 01/25, Rajendra Nayak wrote:
> >> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> >> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
> >
> > Do we really need two files? Maybe collapse the two?
>
> will do. Not sure why, but this is how all other qualcomm
> boards are structured with an almost empty .dts file.

I think it's because we have v1, v2, v3, etc. when we're sorting
out the versions of silicon, but then those are all dropped when
everything is done. Upstream we probably never care.

> >> +
> >> + core2 {
> >> + cpu = <&CPU6>;
> >> + };
> >> +
> >> + core3 {
> >> + cpu = <&CPU7>;
> >> + };
> >> + };
> >> + };
> >
> > From what I recall, this layout causes the kernel to spew
> > warnings? I mean to say this is the power/performance view, but
> > not the architectural view.
>
> hmm, I haven't seen any warnings with this when I boot up on my sdm845
> MTP. Will recheck.

Ok! I will recheck as well.

>
> >
> >> + };
> >> +
> >> + timer {
> >> + compatible = "arm,armv8-timer";
> >> + interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> >
> > Are we supposed to use the GIC_CPU_MASK_SIMPLE macros still?
>
> Not sure, is there another way?

Me either. See this thread from Marc[1]. I guess just drop them?

[1] http://lkml.iu.edu/hypermail/linux/kernel/1702.0/03522.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-01-30 10:25:56

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP



On 01/30/2018 03:18 PM, Stephen Boyd wrote:
>>>> + };
>>>> +
>>>> + timer {
>>>> + compatible = "arm,armv8-timer";
>>>> + interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>>> Are we supposed to use the GIC_CPU_MASK_SIMPLE macros still?
>> Not sure, is there another way?
> Me either. See this thread from Marc[1]. I guess just drop them?
>
> [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.0/03522.html

thanks, Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt does seem to explain
how to specify PPI CPU affinity for GICv3 using a 4th cell if needed which I hadn't read :/

I'll send in a patch to get rid of them on 8996 as well.

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

2018-02-06 18:39:53

by Douglas Anderson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

Hi,

On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <[email protected]> wrote:
> On 01/25, Rajendra Nayak wrote:
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> new file mode 100644
>> index 000000000000..b97f99e6f4b4
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +&tlmm {
>
> I'm not the maintainer, but I find this approach to the pins
> really annoying. I have to flip to another file to figure out how
> a board has configured the pins. And we may bring in a bunch of
> settings that we don't ever use on some board too. Why can't we
> put the settings in the board file directly?

I'm not so familiar with how things work with Qualcomm, but in general
I think putting this in the "board" file is a bad idea. I'd be OK
with putting this directly in the SoC file (though it might get
unwieldy?), but not moving things to the board file as was done with
v2 of this patch.

Said another way: nearly board that uses SDM845 that uses UART2 will
have the same definitions for these pins so we shouldn't be
duplicating it across every board, right?

I'll also respond to the v2 patch so it's obvious there is feedback there...

-Doug

2018-02-06 18:57:00

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

On Thu 25 Jan 08:32 PST 2018, Rajendra Nayak wrote:
> + spmi_bus: qcom,spmi@c440000 {
[..]
> + };
> +

While we have the chance, please remove this empty line.

> + };
> +};

Regards,
Bjorn

2018-02-06 19:01:43

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

On Mon 29 Jan 00:18 PST 2018, Rajendra Nayak wrote:
> On 01/27/2018 03:48 AM, Stephen Boyd wrote:
> > On 01/25, Rajendra Nayak wrote:
> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> >> new file mode 100644
> >> index 000000000000..b97f99e6f4b4
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> >> @@ -0,0 +1,32 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +&tlmm {
> >
> > I'm not the maintainer, but I find this approach to the pins
> > really annoying. I have to flip to another file to figure out how
> > a board has configured the pins. And we may bring in a bunch of
> > settings that we don't ever use on some board too. Why can't we
> > put the settings in the board file directly?
>
> I was just keeping it consistent with how things are for other
> qualcomm platforms. I can move this to the board dts if no one else
> sees any issues.
>

What we decided recently for 8916 (at least) was that we define the
functional properties in the soc dtsi and add the electrical properties
in the board dts(i).

I fully agree with Stephen on this one, but as you say we're keeping the
pinconf in separate files for the other platforms/boards.


I'll prepare and bring some new guidelines to our Thursday meeting and
we can decide what to do based on that.

Regards,
Bjorn

2018-02-06 19:07:22

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote:

> Hi,
>
> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <[email protected]> wrote:
> > On 01/25, Rajendra Nayak wrote:
> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> >> new file mode 100644
> >> index 000000000000..b97f99e6f4b4
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> >> @@ -0,0 +1,32 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +&tlmm {
> >
> > I'm not the maintainer, but I find this approach to the pins
> > really annoying. I have to flip to another file to figure out how
> > a board has configured the pins. And we may bring in a bunch of
> > settings that we don't ever use on some board too. Why can't we
> > put the settings in the board file directly?
>
> I'm not so familiar with how things work with Qualcomm, but in general
> I think putting this in the "board" file is a bad idea. I'd be OK
> with putting this directly in the SoC file (though it might get
> unwieldy?), but not moving things to the board file as was done with
> v2 of this patch.
>
> Said another way: nearly board that uses SDM845 that uses UART2 will
> have the same definitions for these pins so we shouldn't be
> duplicating it across every board, right?
>

We've run into several cases where different boards uses the same
function but requires board specific electrical configuration.

So what we decided was to keep the pinmux in the soc-file (where e.g.
the uart definition is) and then extend it with the board specific
electrical properties (the pinconf), in the board files.

This does come with the complexity of having the pinctrl nodes split in
two places, but the responsibilities of the two parts is clear and we
remove the need for all board files to ensure the appropriate pinmux is
in place.


NB. We did discuss adding "sane defaults" for the pinconf in the soc
dtsi, but we end up spending considerable time debugging issues stemming
from not having the right pinconf; so better make this explicit and say
that the board has to specify it's config.

Regards,
Bjorn

2018-02-06 19:50:46

by Douglas Anderson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

Hi,

On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson
<[email protected]> wrote:
> On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote:
>
>> Hi,
>>
>> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <[email protected]> wrote:
>> > On 01/25, Rajendra Nayak wrote:
>> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> >> new file mode 100644
>> >> index 000000000000..b97f99e6f4b4
>> >> --- /dev/null
>> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> >> @@ -0,0 +1,32 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/*
>> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> >> + */
>> >> +
>> >> +&tlmm {
>> >
>> > I'm not the maintainer, but I find this approach to the pins
>> > really annoying. I have to flip to another file to figure out how
>> > a board has configured the pins. And we may bring in a bunch of
>> > settings that we don't ever use on some board too. Why can't we
>> > put the settings in the board file directly?
>>
>> I'm not so familiar with how things work with Qualcomm, but in general
>> I think putting this in the "board" file is a bad idea. I'd be OK
>> with putting this directly in the SoC file (though it might get
>> unwieldy?), but not moving things to the board file as was done with
>> v2 of this patch.
>>
>> Said another way: nearly board that uses SDM845 that uses UART2 will
>> have the same definitions for these pins so we shouldn't be
>> duplicating it across every board, right?
>>
>
> We've run into several cases where different boards uses the same
> function but requires board specific electrical configuration.
>
> So what we decided was to keep the pinmux in the soc-file (where e.g.
> the uart definition is) and then extend it with the board specific
> electrical properties (the pinconf), in the board files.
>
> This does come with the complexity of having the pinctrl nodes split in
> two places, but the responsibilities of the two parts is clear and we
> remove the need for all board files to ensure the appropriate pinmux is
> in place.
>
>
> NB. We did discuss adding "sane defaults" for the pinconf in the soc
> dtsi, but we end up spending considerable time debugging issues stemming
> from not having the right pinconf; so better make this explicit and say
> that the board has to specify it's config.

Whoops, saw your responses _after_ I sent my response to v2. In any
case this makes sense to me then! On Rockchip boards I've been
involved in we often added "sane defaults", but I can see how that
could be confusing in different ways. I'm happy with your choice and
it seems like a happy medium. The sdm845.dtsi file can have the main
definition of the nodes and can thus refer to the nodes. Then you
just add the extra bit in the board file.

What you propose is not what happened in v2 of the series
<https://patchwork.kernel.org/patch/10194201/> though. In v2 _both_
the pinconf and the pinmux moved to the board file. That's wrong.


To make it concrete, you'd have something like this (this has the
wrong bindings from the UART, but folks get the picture hopefully):


In sdm845.dtsi:

qup_uart2: serial@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";
};

tlmm: pinctrl@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>;

qup_uart2_default: qup_uart2_default {
pinmux {
function = "qup9";
pins = "gpio4", "gpio5";
};
};

qup_uart2_sleep: qup_uart2_sleep {
pinmux {
function = "gpio";
pins = "gpio4", "gpio5";
};
};
};

In sdm845-mtp.dts:

&qup_uart2_default {
pinconf {
pins = "gpio4", "gpio5";
drive-strength = <2>;
bias-disable;
};
};

&qup_uart2_sleep {
pinconf {
pins = "gpio4", "gpio5";
drive-strength = <2>;
bias-disable;
};
};

2018-02-06 20:07:23

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

On Tue 06 Feb 11:49 PST 2018, Doug Anderson wrote:

> Hi,
>
> On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson
> <[email protected]> wrote:
> > On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote:
> >
> >> Hi,
> >>
> >> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <[email protected]> wrote:
> >> > On 01/25, Rajendra Nayak wrote:
> >> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> >> >> new file mode 100644
> >> >> index 000000000000..b97f99e6f4b4
> >> >> --- /dev/null
> >> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
> >> >> @@ -0,0 +1,32 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +/*
> >> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> >> >> + */
> >> >> +
> >> >> +&tlmm {
> >> >
> >> > I'm not the maintainer, but I find this approach to the pins
> >> > really annoying. I have to flip to another file to figure out how
> >> > a board has configured the pins. And we may bring in a bunch of
> >> > settings that we don't ever use on some board too. Why can't we
> >> > put the settings in the board file directly?
> >>
> >> I'm not so familiar with how things work with Qualcomm, but in general
> >> I think putting this in the "board" file is a bad idea. I'd be OK
> >> with putting this directly in the SoC file (though it might get
> >> unwieldy?), but not moving things to the board file as was done with
> >> v2 of this patch.
> >>
> >> Said another way: nearly board that uses SDM845 that uses UART2 will
> >> have the same definitions for these pins so we shouldn't be
> >> duplicating it across every board, right?
> >>
> >
> > We've run into several cases where different boards uses the same
> > function but requires board specific electrical configuration.
> >
> > So what we decided was to keep the pinmux in the soc-file (where e.g.
> > the uart definition is) and then extend it with the board specific
> > electrical properties (the pinconf), in the board files.
> >
> > This does come with the complexity of having the pinctrl nodes split in
> > two places, but the responsibilities of the two parts is clear and we
> > remove the need for all board files to ensure the appropriate pinmux is
> > in place.
> >
> >
> > NB. We did discuss adding "sane defaults" for the pinconf in the soc
> > dtsi, but we end up spending considerable time debugging issues stemming
> > from not having the right pinconf; so better make this explicit and say
> > that the board has to specify it's config.
>
> Whoops, saw your responses _after_ I sent my response to v2. In any
> case this makes sense to me then! On Rockchip boards I've been
> involved in we often added "sane defaults", but I can see how that
> could be confusing in different ways. I'm happy with your choice and
> it seems like a happy medium. The sdm845.dtsi file can have the main
> definition of the nodes and can thus refer to the nodes. Then you
> just add the extra bit in the board file.
>
> What you propose is not what happened in v2 of the series
> <https://patchwork.kernel.org/patch/10194201/> though. In v2 _both_
> the pinconf and the pinmux moved to the board file. That's wrong.
>
>
> To make it concrete, you'd have something like this (this has the
> wrong bindings from the UART, but folks get the picture hopefully):
>
>
> In sdm845.dtsi:
>
> qup_uart2: serial@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";
> };
>
> tlmm: pinctrl@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>;
>
> qup_uart2_default: qup_uart2_default {
> pinmux {
> function = "qup9";
> pins = "gpio4", "gpio5";
> };
> };
>
> qup_uart2_sleep: qup_uart2_sleep {
> pinmux {
> function = "gpio";
> pins = "gpio4", "gpio5";
> };
> };
> };
>
> In sdm845-mtp.dts:
>
> &qup_uart2_default {
> pinconf {
> pins = "gpio4", "gpio5";
> drive-strength = <2>;
> bias-disable;
> };
> };
>
> &qup_uart2_sleep {
> pinconf {
> pins = "gpio4", "gpio5";
> drive-strength = <2>;
> bias-disable;
> };
> };

Correct.


This example does however show another thing that I really do not like;
When you have a lot of nodes I find it very useful to maintain some sort
of grouping, to know that I can find a node describing properties
related to some block close to related blocks - e.g. nodes describing
a pmic block is close to other nodes for that pmic.

Today we seem to have a mixture of bus-based grouping, arbitrary
grouping and no grouping at all in our upstream dtsi files, so I think
we should set some guidelines here as well.

Regards,
Bjorn

2018-02-06 20:28:05

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

On Fri, Jan 26, 2018 at 4:15 PM, Stephen Boyd <[email protected]> wrote:
> On 01/25, Rajendra Nayak wrote:
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
>
> Do we really need two files? Maybe collapse the two?
>
>> create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> new file mode 100644
>> index 000000000000..a21f4912b3e2
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -0,0 +1,308 @@
>> +// 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@0 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + 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@100 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x100>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_100>;
>> + L2_100: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU2: cpu@200 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x200>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_200>;
>> + L2_200: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU3: cpu@300 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x300>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_300>;
>> + L2_300: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU4: cpu@400 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x400>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_400>;
>> + L2_400: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU5: cpu@500 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x500>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_500>;
>> + L2_500: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU6: cpu@600 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x600>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_600>;
>> + L2_600: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + CPU7: cpu@700 {
>> + device_type = "cpu";
>> + compatible = "qcom,kryo";
>> + reg = <0x0 0x700>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_700>;
>> + L2_700: l2-cache {
>> + compatible = "cache";
>> + next-level-cache = <&L3_0>;
>> + };
>> + };
>> +
>> + cpu-map {
>> + cluster0 {
>> + core0 {
>> + cpu = <&CPU0>;
>> + };
>> +
>> + core1 {
>> + cpu = <&CPU1>;
>> + };
>> +
>> + core2 {
>> + cpu = <&CPU2>;
>> + };
>> +
>> + core3 {
>> + cpu = <&CPU3>;
>> + };
>> + };
>> +
>> + cluster1 {
>> + core0 {
>> + cpu = <&CPU4>;
>> + };
>> +
>> + core1 {
>> + cpu = <&CPU5>;
>> + };
>> +
>> + core2 {
>> + cpu = <&CPU6>;
>> + };
>> +
>> + core3 {
>> + cpu = <&CPU7>;
>> + };
>> + };
>> + };
>
> From what I recall, this layout causes the kernel to spew
> warnings? I mean to say this is the power/performance view, but
> not the architectural view.
>
>> + };
>> +
>> + timer {
>> + compatible = "arm,armv8-timer";
>> + interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>
> Are we supposed to use the GIC_CPU_MASK_SIMPLE macros still?
>
>> + <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 0 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
>> + };
>> +
>> + clocks {
>> + xo_board: xo_board {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <19200000>;
>> + clock-output-names = "xo_board";
>
> We can drop clock-output-names on these.
>
>> + };
>> +
>> + sleep_clk: sleep_clk {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <32764>;
>> + clock-output-names = "sleep_clk";
>> + };
>> + };
>> +
>> + psci {
>> + compatible = "arm,psci-1.0";
>> + method = "smc";
>> + };
>> +
>> + soc: soc {
>
> Will anyone use this phandle?
>
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0 0 0 0xffffffff>;
>> + compatible = "simple-bus";
>> +
>> + intc: interrupt-controller@17a00000 {
>> + 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>;
>
> Can you also add the ITS node please and mark it as disabled?
> I'll send a patch to the list to skip status = "disabled" ones.
> We may want to support ITS on these SoCs if the firmware is
> different.
>
>> + };
>> +
>> + gcc: clock-controller@100000 {
>> + compatible = "qcom,gcc-sdm845";
>> + reg = <0x100000 0x1f0000>;
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + };
>> +
>> + tlmm: pinctrl@03400000 {
>
> Drop leading zeroes please.

Build dtbs with W=2 and fix the warnings so reviewers don't have to
waste their time on these issues.

Rob

2018-02-06 20:32:49

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

On Thu, Jan 25, 2018 at 10:32 AM, Rajendra Nayak <[email protected]> wrote:
> Add a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files
>
> Signed-off-by: Rajendra Nayak <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++
> arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 11 ++
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 308 +++++++++++++++++++++++++++++++
> 4 files changed, 333 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
> 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..c57701bed084 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..95e41e42bee1
> --- /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-mtp.dtsi"
> +
> +/ {
> + model = "Qualcomm Technologies, Inc. SDM845 MTP";
> + compatible = "qcom,sdm845-mtp";
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
> new file mode 100644
> index 000000000000..5b1022c20bad
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include "sdm845.dtsi"
> +
> +/ {
> + soc {
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> new file mode 100644
> index 000000000000..a21f4912b3e2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -0,0 +1,308 @@
> +// 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";

This should only be in the board level file.

> +
> + interrupt-parent = <&intc>;
> +
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + chosen { };
> +
> + memory {
> + device_type = "memory";
> + /* We expect the bootloader to fill in the reg */

The start address is variable? If not you should populate the base and
have a unit-address.

> + reg = <0 0 0 0>;
> + };
> +
> + cpus {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + CPU0: cpu@0 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + 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@100 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x100>;
> + enable-method = "psci";
> + next-level-cache = <&L2_100>;
> + L2_100: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU2: cpu@200 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x200>;
> + enable-method = "psci";
> + next-level-cache = <&L2_200>;
> + L2_200: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU3: cpu@300 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x300>;
> + enable-method = "psci";
> + next-level-cache = <&L2_300>;
> + L2_300: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU4: cpu@400 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x400>;
> + enable-method = "psci";
> + next-level-cache = <&L2_400>;
> + L2_400: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU5: cpu@500 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x500>;
> + enable-method = "psci";
> + next-level-cache = <&L2_500>;
> + L2_500: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU6: cpu@600 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x600>;
> + enable-method = "psci";
> + next-level-cache = <&L2_600>;
> + L2_600: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + CPU7: cpu@700 {
> + device_type = "cpu";
> + compatible = "qcom,kryo";
> + reg = <0x0 0x700>;
> + enable-method = "psci";
> + next-level-cache = <&L2_700>;
> + L2_700: l2-cache {
> + compatible = "cache";
> + next-level-cache = <&L3_0>;
> + };
> + };
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&CPU0>;
> + };
> +
> + core1 {
> + cpu = <&CPU1>;
> + };
> +
> + core2 {
> + cpu = <&CPU2>;
> + };
> +
> + core3 {
> + cpu = <&CPU3>;
> + };
> + };
> +
> + cluster1 {
> + core0 {
> + cpu = <&CPU4>;
> + };
> +
> + core1 {
> + cpu = <&CPU5>;
> + };
> +
> + core2 {
> + cpu = <&CPU6>;
> + };
> +
> + core3 {
> + cpu = <&CPU7>;
> + };
> + };
> + };
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> + <GIC_PPI 0 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
> + };
> +
> + clocks {
> + xo_board: xo_board {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <19200000>;
> + clock-output-names = "xo_board";
> + };
> +
> + sleep_clk: sleep_clk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32764>;
> + clock-output-names = "sleep_clk";
> + };
> + };
> +
> + psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> +
> + soc: soc {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0 0 0xffffffff>;
> + compatible = "simple-bus";
> +
> + intc: interrupt-controller@17a00000 {
> + 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>;
> + };
> +
> + gcc: clock-controller@100000 {
> + compatible = "qcom,gcc-sdm845";

sdm845-gcc is the preferred order.

> + reg = <0x100000 0x1f0000>;
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> + };
> +
> + tlmm: pinctrl@03400000 {
> + 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@17C90000 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + compatible = "arm,armv7-timer-mem";
> + reg = <0x17C90000 0x1000>;
> + clock-frequency = <19200000>;
> +
> + frame@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@17cc0000 {
> + frame-number = <1>;
> + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17cc0000 0x1000>;
> + status = "disabled";
> + };
> +
> + frame@17cd0000 {
> + frame-number = <2>;
> + interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17cd0000 0x1000>;
> + status = "disabled";
> + };
> +
> + frame@17ce0000 {
> + frame-number = <3>;
> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17ce0000 0x1000>;
> + status = "disabled";
> + };
> +
> + frame@17cf0000 {
> + frame-number = <4>;
> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17cf0000 0x1000>;
> + status = "disabled";
> + };
> +
> + frame@17d00000 {
> + frame-number = <5>;
> + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17d00000 0x1000>;
> + status = "disabled";
> + };
> +
> + frame@17d10000 {
> + frame-number = <6>;
> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x17d10000 0x1000>;
> + status = "disabled";
> + };
> + };
> +
> + spmi_bus: qcom,spmi@c440000 {

spmi@...

> + 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2018-02-06 20:38:02

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support

On Fri, Jan 26, 2018 at 4:18 PM, Stephen Boyd <[email protected]> wrote:
> On 01/25, Rajendra Nayak wrote:
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> new file mode 100644
>> index 000000000000..b97f99e6f4b4
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +&tlmm {
>
> I'm not the maintainer, but I find this approach to the pins
> really annoying. I have to flip to another file to figure out how
> a board has configured the pins. And we may bring in a bunch of
> settings that we don't ever use on some board too. Why can't we
> put the settings in the board file directly?

FYI, there's a pending dtc change to allow deleting unreferenced nodes
which can solve the bloat problem.

Rob

2018-02-07 04:15:18

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support



On 02/07/2018 01:19 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson
> <[email protected]> wrote:
>> On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote:
>>
>>> Hi,
>>>
>>> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <[email protected]> wrote:
>>>> On 01/25, Rajendra Nayak wrote:
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>>>>> new file mode 100644
>>>>> index 000000000000..b97f99e6f4b4
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>>>>> @@ -0,0 +1,32 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>>> + */
>>>>> +
>>>>> +&tlmm {
>>>>
>>>> I'm not the maintainer, but I find this approach to the pins
>>>> really annoying. I have to flip to another file to figure out how
>>>> a board has configured the pins. And we may bring in a bunch of
>>>> settings that we don't ever use on some board too. Why can't we
>>>> put the settings in the board file directly?
>>>
>>> I'm not so familiar with how things work with Qualcomm, but in general
>>> I think putting this in the "board" file is a bad idea. I'd be OK
>>> with putting this directly in the SoC file (though it might get
>>> unwieldy?), but not moving things to the board file as was done with
>>> v2 of this patch.
>>>
>>> Said another way: nearly board that uses SDM845 that uses UART2 will
>>> have the same definitions for these pins so we shouldn't be
>>> duplicating it across every board, right?
>>>
>>
>> We've run into several cases where different boards uses the same
>> function but requires board specific electrical configuration.
>>
>> So what we decided was to keep the pinmux in the soc-file (where e.g.
>> the uart definition is) and then extend it with the board specific
>> electrical properties (the pinconf), in the board files.
>>
>> This does come with the complexity of having the pinctrl nodes split in
>> two places, but the responsibilities of the two parts is clear and we
>> remove the need for all board files to ensure the appropriate pinmux is
>> in place.
>>
>>
>> NB. We did discuss adding "sane defaults" for the pinconf in the soc
>> dtsi, but we end up spending considerable time debugging issues stemming
>> from not having the right pinconf; so better make this explicit and say
>> that the board has to specify it's config.
>
> Whoops, saw your responses _after_ I sent my response to v2. In any
> case this makes sense to me then! On Rockchip boards I've been
> involved in we often added "sane defaults", but I can see how that
> could be confusing in different ways. I'm happy with your choice and
> it seems like a happy medium. The sdm845.dtsi file can have the main
> definition of the nodes and can thus refer to the nodes. Then you
> just add the extra bit in the board file.
>
> What you propose is not what happened in v2 of the series
> <https://patchwork.kernel.org/patch/10194201/> though. In v2 _both_
> the pinconf and the pinmux moved to the board file. That's wrong.

got it. I'll fix this up in my v3. Thanks for the review.

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

2018-02-07 04:15:33

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

[]..

>>> + };
>>> +
>>> + gcc: clock-controller@100000 {
>>> + compatible = "qcom,gcc-sdm845";
>>> + reg = <0x100000 0x1f0000>;
>>> + #clock-cells = <1>;
>>> + #reset-cells = <1>;
>>> + };
>>> +
>>> + tlmm: pinctrl@03400000 {
>>
>> Drop leading zeroes please.
>
> Build dtbs with W=2 and fix the warnings so reviewers don't have to
> waste their time on these issues.

Noted. Thanks Rob.

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

2018-02-07 04:17:06

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP



On 02/07/2018 12:24 AM, Bjorn Andersson wrote:
> On Thu 25 Jan 08:32 PST 2018, Rajendra Nayak wrote:
>> + spmi_bus: qcom,spmi@c440000 {
> [..]
>> + };
>> +
>
> While we have the chance, please remove this empty line.

Will do. Thanks for the review.

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

2018-02-07 04:48:30

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

[]..

>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> + model = "Qualcomm Technologies, Inc. SDM845";
>
> This should only be in the board level file.

thanks, will fix.

>
>> +
>> + interrupt-parent = <&intc>;
>> +
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> +
>> + chosen { };
>> +
>> + memory {
>> + device_type = "memory";
>> + /* We expect the bootloader to fill in the reg */
>
> The start address is variable? If not you should populate the base and
> have a unit-address.

sure, I'll check and update.

>
>> + reg = <0 0 0 0>;
>> + };
>> +

[]..
>> +
>> + soc: soc {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0 0 0 0xffffffff>;
>> + compatible = "simple-bus";
>> +
>> + intc: interrupt-controller@17a00000 {
>> + 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>;
>> + };
>> +
>> + gcc: clock-controller@100000 {
>> + compatible = "qcom,gcc-sdm845";
>
> sdm845-gcc is the preferred order.

This is still proposed as part of the GCC patch for sdm845 [1]
(which looks like has neither you nor the DT list copied :/ )
Also looking at Documentation/devicetree/bindings/clock/qcom,gcc.txt,
I see we seem to follow the gcc-<soc> convention for compatible all along :(

"qcom,gcc-apq8064"
"qcom,gcc-apq8084"
"qcom,gcc-ipq8064"
"qcom,gcc-ipq4019"
"qcom,gcc-ipq8074"
"qcom,gcc-msm8660"
"qcom,gcc-msm8916"
"qcom,gcc-msm8960"
"qcom,gcc-msm8974"
"qcom,gcc-msm8974pro"
"qcom,gcc-msm8974pro-ac"
"qcom,gcc-msm8994"
"qcom,gcc-msm8996"
"qcom,gcc-mdm9615"

[1] https://patchwork.kernel.org/patch/10193895/

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

2018-02-07 17:38:56

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/2] arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP

On Tue, Feb 6, 2018 at 10:47 PM, Rajendra Nayak <[email protected]> wrote:
> []..
>
>>> +
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +/ {
>>> + model = "Qualcomm Technologies, Inc. SDM845";
>>
>> This should only be in the board level file.
>
> thanks, will fix.
>
>>
>>> +
>>> + interrupt-parent = <&intc>;
>>> +
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>> +
>>> + chosen { };
>>> +
>>> + memory {
>>> + device_type = "memory";
>>> + /* We expect the bootloader to fill in the reg */
>>
>> The start address is variable? If not you should populate the base and
>> have a unit-address.
>
> sure, I'll check and update.
>
>>
>>> + reg = <0 0 0 0>;
>>> + };
>>> +
>
> []..
>>> +
>>> + soc: soc {
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + ranges = <0 0 0 0xffffffff>;
>>> + compatible = "simple-bus";
>>> +
>>> + intc: interrupt-controller@17a00000 {
>>> + 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>;
>>> + };
>>> +
>>> + gcc: clock-controller@100000 {
>>> + compatible = "qcom,gcc-sdm845";
>>
>> sdm845-gcc is the preferred order.
>
> This is still proposed as part of the GCC patch for sdm845 [1]
> (which looks like has neither you nor the DT list copied :/ )
> Also looking at Documentation/devicetree/bindings/clock/qcom,gcc.txt,
> I see we seem to follow the gcc-<soc> convention for compatible all along :(
>
> "qcom,gcc-apq8064"
> "qcom,gcc-apq8084"
> "qcom,gcc-ipq8064"
> "qcom,gcc-ipq4019"
> "qcom,gcc-ipq8074"
> "qcom,gcc-msm8660"
> "qcom,gcc-msm8916"
> "qcom,gcc-msm8960"
> "qcom,gcc-msm8974"
> "qcom,gcc-msm8974pro"
> "qcom,gcc-msm8974pro-ac"
> "qcom,gcc-msm8994"
> "qcom,gcc-msm8996"
> "qcom,gcc-mdm9615"

Okay, I guess the pattern for this is pretty much established.

Rob