diff mbox

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

Message ID 20180125163216.29018-2-rnayak@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Rajendra Nayak Jan. 25, 2018, 4:32 p.m. UTC
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-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

Comments

Evan Green Jan. 26, 2018, 8:31 p.m. UTC | #1
Hi Rajendra,

On Thu, Jan 25, 2018 at 8:32 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> 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-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
Stephen Boyd Jan. 26, 2018, 10:15 p.m. UTC | #2
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>;
> +			};
> +
Rajendra Nayak Jan. 29, 2018, 8:13 a.m. UTC | #3
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>;
>> +			};
>> +
>
Rajendra Nayak Jan. 30, 2018, 8:48 a.m. UTC | #4
On 01/27/2018 02:01 AM, Evan Green wrote:
> Hi Rajendra,
> 
> On Thu, Jan 25, 2018 at 8:32 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> 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-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
>
Stephen Boyd Jan. 30, 2018, 9:48 a.m. UTC | #5
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
Rajendra Nayak Jan. 30, 2018, 10:25 a.m. UTC | #6
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.
Bjorn Andersson Feb. 6, 2018, 6:54 p.m. UTC | #7
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
Rob Herring (Arm) Feb. 6, 2018, 8:26 p.m. UTC | #8
On Fri, Jan 26, 2018 at 4:15 PM, Stephen Boyd <sboyd@codeaurora.org> 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
Rob Herring (Arm) Feb. 6, 2018, 8:31 p.m. UTC | #9
On Thu, Jan 25, 2018 at 10:32 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> 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-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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajendra Nayak Feb. 7, 2018, 4:14 a.m. UTC | #10
[]..

>>> +             };
>>> +
>>> +             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.
Rajendra Nayak Feb. 7, 2018, 4:15 a.m. UTC | #11
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.
Rajendra Nayak Feb. 7, 2018, 4:47 a.m. UTC | #12
[]..

>> +
>> +#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/
Rob Herring (Arm) Feb. 7, 2018, 5:37 p.m. UTC | #13
On Tue, Feb 6, 2018 at 10:47 PM, Rajendra Nayak <rnayak@codeaurora.org> 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
diff mbox

Patch

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>;
+		};
+
+	};
+};