[6/7] arm64: dts: Add ipq9574 SoC and AL02 board support

  From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>

Add initial device tree support for Qualcomm IPQ9574 SoC
and AL02 board

Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
Co-developed-by: devi priya <quic_devipriy@quicinc.com>
Signed-off-by: devi priya <quic_devipriy@quicinc.com>
Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
---
 arch/arm64/boot/dts/qcom/Makefile            |   1 +
 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
 create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
  

On 10.01.2023 13:13, devi priya wrote:
> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> 
> Add initial device tree support for Qualcomm IPQ9574 SoC
> and AL02 board
> 
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>  arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>  3 files changed, 388 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 3e79496292e7..872c62028a0b 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c2.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq9574-al02-c7.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-alcatel-idol347.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-asus-z00l.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-huawei-g7.dtb
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> new file mode 100644
> index 000000000000..ae3c32f3e16a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-only
BSD3?

> +/*
> + * IPQ9574 AL02-C7 board device tree source
> + *
> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
Happy new year!

> + */
> +
> +/dts-v1/;
> +
> +#include "ipq9574.dtsi"
> +
> +/ {
> +	model = "Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7";
> +	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
> +	interrupt-parent = <&intc>;
> +
> +	aliases {
> +		serial0 = &blsp1_uart2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +};
> +
> +&blsp1_uart2 {
> +	pinctrl-0 = <&uart2_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&sdhc_1 {
> +	pinctrl-0 = <&emmc_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&tlmm {
> +	emmc_pins: emmc-state {
sdc_default? I suppose you'll introduce a corresponding sleep state
later on, so that'll be easier to distinguish.

> +		emmc-clk-pins {
> +			pins = "gpio5";
> +			function = "sdc_clk";
> +			drive-strength = <8>;
> +			bias-disable;
> +		};
Please add a newline between subsequent nodes.
> +		emmc-cmd-pins {
> +			pins = "gpio4";
> +			function = "sdc_cmd";
> +			drive-strength = <8>;
> +			bias-pull-up;
> +		};
> +		emmc-data-pins {
> +			pins = "gpio0", "gpio1", "gpio2",
> +			     "gpio3", "gpio6", "gpio7",
> +			     "gpio8", "gpio9";
The indentation here is wrong.

> +			function = "sdc_data";
> +			drive-strength = <8>;
> +			bias-pull-up;
> +		};
> +		emmc-rclk-pins {
> +			pins = "gpio10";
> +			function = "sdc_rclk";
> +			drive-strength = <8>;
> +			bias-pull-down;
> +		};
> +	};
> +
> +};
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> new file mode 100644
> index 000000000000..188d18688a77
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * IPQ9574 SoC device tree source
> + *
> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,gcc-ipq9574.h>
> +#include <dt-bindings/reset/qcom,gcc-ipq9574.h>
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	clocks {
> +		bias_pll_ubi_nc_clk: bias_pll_ubi_nc_clk {
Could you tell us something about the purpose of this clock? First
time seeing it, your gcc driver reveals it's connected to at least
PCIe.

> +			compatible = "fixed-clock";
> +			clock-frequency = <353000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy2_pipe_clk: pcie30_phy2_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy3_pipe_clk: pcie30_phy3_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		usb3phy_0_cc_pipe_clk: usb3phy_0_cc_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <125000000>;
> +			#clock-cells = <0>;
> +		};
Do not define these pipe clocks. You can leave the GCC entries as <0>
until you introduce the QMPPHY support, which then you can feed as it
provides these clocks.

> +
> +		sleep_clk: sleep-clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <32000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		xo_board_clk: xo-board-clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <24000000>;
The clock frequency should be moved to the device DT, because the
clock is on the board and not on the SoC.

> +			#clock-cells = <0>;
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x1>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU2: cpu@2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x2>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU3: cpu@3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x3>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		L2_0: l2-cache {
> +			compatible = "cache";
> +			cache-level = <2>;
> +		};
> +	};
> +
> +	memory@40000000 {
> +		device_type = "memory";
> +		/* We expect the bootloader to fill in the size */
> +		reg = <0x0 0x40000000 0x0 0x0>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a73-pmu";
> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		tz_region: memory@4a600000 {
> +			reg = <0x0 0x4a600000 0x0 0x400000>;
> +			no-map;
> +		};
That's.. surprisingly little reserved memory.. No hyp? No PIL regions
that make the board explode when something touches them?

> +	};
> +
> +	soc: soc@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0 0xffffffff>;
Is 32 bits enough for this SoC's bus? Newer ones use 36 or more..

> +		compatible = "simple-bus";
> +
> +		tlmm: pinctrl@1000000 {
> +			compatible = "qcom,ipq9574-tlmm";
> +			reg = <0x01000000 0x300000>;
> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&tlmm 0 0 65>;
> +			gpio-reserved-ranges = <59 1>;
I see it's assigned to [rx0, pwm23, qdss_tracedata_a].. Is this
board-specific or is this pin supposed to be forbidden on all IPQ9574
boards?

> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +
> +			uart2_pins: uart2-state {
> +				pins = "gpio34", "gpio35";
> +				function = "blsp2_uart";
> +				drive-strength = <8>;
> +				bias-disable;
> +			};
> +		};
> +
> +		gcc: clock-controller@1800000 {
> +			compatible = "qcom,gcc-ipq9574";
> +			reg = <0x1800000 0x80000>;
> +			clocks = <&xo_board_clk>,
> +				<&sleep_clk>,
> +				<&bias_pll_ubi_nc_clk>,
> +				<&pcie30_phy0_pipe_clk>,
> +				<&pcie30_phy1_pipe_clk>,
> +				<&pcie30_phy2_pipe_clk>,
> +				<&pcie30_phy3_pipe_clk>,
> +				<&usb3phy_0_cc_pipe_clk>;
> +			clock-names = "xo",
> +				"sleep_clk",
> +				"bias_pll_ubi_nc_clk",
> +				"pcie30_phy0_pipe_clk",
> +				"pcie30_phy1_pipe_clk",
> +				"pcie30_phy2_pipe_clk",
> +				"pcie30_phy3_pipe_clk",
> +				"usb3phy_0_cc_pipe_clk";
Please touch up the indentation.

> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		sdhc_1: sdhci@7804000 {
> +			compatible = "qcom,sdhci-msm-v5";
> +			reg = <0x7804000 0x1000>, <0x7805000 0x1000>;
> +			reg-names = "hc_mem", "cmdq_mem";
> +
> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
> +				   <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
Please touch up the indentation.

> +			interrupt-names = "hc_irq", "pwr_irq";
> +
> +			clocks = <&xo_board_clk>,
> +			       <&gcc GCC_SDCC1_AHB_CLK>,
> +			       <&gcc GCC_SDCC1_APPS_CLK>;
Please touch up the indentation.

> +			clock-names = "xo", "iface", "core";
The order should be "iface", "core", "xo" as per Documentation/devicetree/bindings/mmc/sdhci-msm.yaml

> +			mmc-ddr-1_8v;
> +			mmc-hs200-1_8v;
> +			mmc-hs400-1_8v;
> +			mmc-hs400-enhanced-strobe;
Are these the limitations of the controller? Otherwise they should
probably be moved to the device-specific DT.

> +			max-frequency = <384000000>;
> +			bus-width = <8>;

> +			non-removable;
And this property too.
> +			status = "disabled";
> +		};
> +
> +		blsp1_uart2: serial@78b1000 {
> +			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
> +			reg = <0x078b1000 0x200>;
> +			interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_BLSP1_UART3_APPS_CLK>,
> +				 <&gcc GCC_BLSP1_AHB_CLK>;
> +			clock-names = "core", "iface";
> +			status = "disabled";
> +		};
> +
> +		intc: interrupt-controller@b000000 {
> +			compatible = "qcom,msm-qgic2";
> +			reg = <0x0b000000 0x1000>,  /* GICD */
> +			      <0x0b002000 0x1000>,  /* GICC */
> +			      <0x0b001000 0x1000>,  /* GICH */
> +			      <0x0b004000 0x1000>;  /* GICV */
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +			ranges = <0 0x0b00c000 0x3000>;
> +
> +			v2m0: v2m@0 {
> +				compatible = "arm,gic-v2m-frame";
> +				reg = <0x0 0xffd>;
> +				msi-controller;
> +			};
> +
> +			v2m1: v2m@1 {
> +				compatible = "arm,gic-v2m-frame";
> +				reg = <0x1000 0xffd>;
> +				msi-controller;
> +			};
> +
> +			v2m2: v2m@2 {
> +				compatible = "arm,gic-v2m-frame";
> +				reg = <0x2000 0xffd>;
> +				msi-controller;
> +			};
> +		};
> +
> +		timer@b120000 {
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0xb120000 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			clock-frequency = <24000000>;
Drop, something earlier in the boot chain already writes to CNTFRQ_ELn.

> +
> +			frame@b120000 {
> +				reg = <0xb121000 0x1000>,
> +				      <0xb122000 0x1000>;
> +				frame-number = <0>;
> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +					     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +
> +			frame@b123000 {
> +				reg = <0xb123000 0x1000>;
> +				frame-number = <1>;
> +				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b124000 {
> +				reg = <0xb124000 0x1000>;
> +				frame-number = <2>;
> +				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b125000 {
> +				reg = <0xb125000 0x1000>;
> +				frame-number = <3>;
> +				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b126000 {
> +				reg = <0xb126000 0x1000>;
> +				frame-number = <4>;
> +				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b127000 {
> +				reg = <0xb127000 0x1000>;
> +				frame-number = <5>;
> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b128000 {
> +				reg = <0xb128000 0x1000>;
> +				frame-number = <6>;
> +				interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +		clock-frequency = <24000000>;
Drop, something earlier in the boot chain already writes to CNTFRQ_ELn.

Konrad
> +	};
> +};
  

On 10/01/2023 13:13, devi priya wrote:
> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> 
> Add initial device tree support for Qualcomm IPQ9574 SoC
> and AL02 board
> 
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>  arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>  3 files changed, 388 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 3e79496292e7..872c62028a0b 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c2.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq9574-al02-c7.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-alcatel-idol347.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-asus-z00l.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-huawei-g7.dtb
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> new file mode 100644
> index 000000000000..ae3c32f3e16a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Not dual licensed? Not BSD?

> +/*
> + * IPQ9574 AL02-C7 board device tree source
> + *
> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include "ipq9574.dtsi"
> +
> +/ {
> +	model = "Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7";
> +	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
> +	interrupt-parent = <&intc>;
> +
> +	aliases {
> +		serial0 = &blsp1_uart2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +};
> +
> +&blsp1_uart2 {
> +	pinctrl-0 = <&uart2_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&sdhc_1 {
> +	pinctrl-0 = <&emmc_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&tlmm {
> +	emmc_pins: emmc-state {
> +		emmc-clk-pins {
> +			pins = "gpio5";
> +			function = "sdc_clk";
> +			drive-strength = <8>;
> +			bias-disable;
> +		};
> +		emmc-cmd-pins {
> +			pins = "gpio4";
> +			function = "sdc_cmd";
> +			drive-strength = <8>;
> +			bias-pull-up;
> +		};
> +		emmc-data-pins {
> +			pins = "gpio0", "gpio1", "gpio2",
> +			     "gpio3", "gpio6", "gpio7",
> +			     "gpio8", "gpio9";
> +			function = "sdc_data";
> +			drive-strength = <8>;
> +			bias-pull-up;
> +		};
> +		emmc-rclk-pins {
> +			pins = "gpio10";
> +			function = "sdc_rclk";
> +			drive-strength = <8>;
> +			bias-pull-down;
> +		};
> +	};
> +
> +};
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> new file mode 100644
> index 000000000000..188d18688a77
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * IPQ9574 SoC device tree source
> + *
> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,gcc-ipq9574.h>
> +#include <dt-bindings/reset/qcom,gcc-ipq9574.h>
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	clocks {
> +		bias_pll_ubi_nc_clk: bias_pll_ubi_nc_clk {

No undercores in node names.

> +			compatible = "fixed-clock";
> +			clock-frequency = <353000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy2_pipe_clk: pcie30_phy2_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pcie30_phy3_pipe_clk: pcie30_phy3_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <250000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		usb3phy_0_cc_pipe_clk: usb3phy_0_cc_pipe_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <125000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		sleep_clk: sleep-clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <32000>;

That's not a property of the SoC, but board. Either entire clock or at
least frequency to indicate that the board is providing the clock.

> +			#clock-cells = <0>;> +		};
> +
> +		xo_board_clk: xo-board-clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <24000000>;

Ditto.

> +			#clock-cells = <0>;
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x1>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU2: cpu@2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x2>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU3: cpu@3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a73";
> +			reg = <0x3>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		L2_0: l2-cache {
> +			compatible = "cache";
> +			cache-level = <2>;
> +		};
> +	};
> +
> +	memory@40000000 {
> +		device_type = "memory";
> +		/* We expect the bootloader to fill in the size */
> +		reg = <0x0 0x40000000 0x0 0x0>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a73-pmu";
> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		tz_region: memory@4a600000 {
> +			reg = <0x0 0x4a600000 0x0 0x400000>;
> +			no-map;
> +		};
> +	};
> +
> +	soc: soc@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0 0xffffffff>;
> +		compatible = "simple-bus";
> +
> +		tlmm: pinctrl@1000000 {
> +			compatible = "qcom,ipq9574-tlmm";
> +			reg = <0x01000000 0x300000>;
> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&tlmm 0 0 65>;
> +			gpio-reserved-ranges = <59 1>;

Hm, why reserved ranges are in SoC?

> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +
> +			uart2_pins: uart2-state {
> +				pins = "gpio34", "gpio35";
> +				function = "blsp2_uart";
> +				drive-strength = <8>;
> +				bias-disable;
> +			};
> +		};
> +
> +		gcc: clock-controller@1800000 {
> +			compatible = "qcom,gcc-ipq9574";
> +			reg = <0x1800000 0x80000>;
> +			clocks = <&xo_board_clk>,
> +				<&sleep_clk>,
> +				<&bias_pll_ubi_nc_clk>,
> +				<&pcie30_phy0_pipe_clk>,
> +				<&pcie30_phy1_pipe_clk>,
> +				<&pcie30_phy2_pipe_clk>,
> +				<&pcie30_phy3_pipe_clk>,
> +				<&usb3phy_0_cc_pipe_clk>;
> +			clock-names = "xo",
> +				"sleep_clk",

Misaligned. Multiple other places probably as well.

> +				"bias_pll_ubi_nc_clk",
> +				"pcie30_phy0_pipe_clk",
> +				"pcie30_phy1_pipe_clk",
> +				"pcie30_phy2_pipe_clk",
> +				"pcie30_phy3_pipe_clk",
> +				"usb3phy_0_cc_pipe_clk";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		sdhc_1: sdhci@7804000 {
> +			compatible = "qcom,sdhci-msm-v5";
> +			reg = <0x7804000 0x1000>, <0x7805000 0x1000>;
> +			reg-names = "hc_mem", "cmdq_mem";
> +
> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
> +				   <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;

Like here.

> +			interrupt-names = "hc_irq", "pwr_irq";
> +
> +			clocks = <&xo_board_clk>,
> +			       <&gcc GCC_SDCC1_AHB_CLK>,
> +			       <&gcc GCC_SDCC1_APPS_CLK>;

And here

> +			clock-names = "xo", "iface", "core";

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +			mmc-ddr-1_8v;
> +			mmc-hs200-1_8v;
> +			mmc-hs400-1_8v;
> +			mmc-hs400-enhanced-strobe;
> +			max-frequency = <384000000>;
> +			bus-width = <8>;
> +			non-removable;
> +			status = "disabled";
> +		};
> +
> +		blsp1_uart2: serial@78b1000 {
> +			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
> +			reg = <0x078b1000 0x200>;
> +			interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_BLSP1_UART3_APPS_CLK>,
> +				 <&gcc GCC_BLSP1_AHB_CLK>;
> +			clock-names = "core", "iface";
> +			status = "disabled";
> +		};
> +
> +		intc: interrupt-controller@b000000 {
> +			compatible = "qcom,msm-qgic2";
> +			reg = <0x0b000000 0x1000>,  /* GICD */
> +			      <0x0b002000 0x1000>,  /* GICC */
> +			      <0x0b001000 0x1000>,  /* GICH */
> +			      <0x0b004000 0x1000>;  /* GICV */
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +			ranges = <0 0x0b00c000 0x3000>;
> +
> +			v2m0: v2m@0 {
> +				compatible = "arm,gic-v2m-frame";
> +				reg = <0x0 0xffd>;
> +				msi-controller;
> +			};
> +
> +			v2m1: v2m@1 {
> +				compatible = "arm,gic-v2m-frame";
> +				reg = <0x1000 0xffd>;
> +				msi-controller;
> +			};
> +
> +			v2m2: v2m@2 {
> +				compatible = "arm,gic-v2m-frame";
> +				reg = <0x2000 0xffd>;
> +				msi-controller;
> +			};
> +		};
> +
> +		timer@b120000 {
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0xb120000 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			clock-frequency = <24000000>;
> +
> +			frame@b120000 {
> +				reg = <0xb121000 0x1000>,
> +				      <0xb122000 0x1000>;
> +				frame-number = <0>;
> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +					     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +
> +			frame@b123000 {
> +				reg = <0xb123000 0x1000>;
> +				frame-number = <1>;
> +				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b124000 {
> +				reg = <0xb124000 0x1000>;
> +				frame-number = <2>;
> +				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b125000 {
> +				reg = <0xb125000 0x1000>;
> +				frame-number = <3>;
> +				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b126000 {
> +				reg = <0xb126000 0x1000>;
> +				frame-number = <4>;
> +				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b127000 {
> +				reg = <0xb127000 0x1000>;
> +				frame-number = <5>;
> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			frame@b128000 {
> +				reg = <0xb128000 0x1000>;
> +				frame-number = <6>;
> +				interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +		clock-frequency = <24000000>;

Is this allowed in recent designs?

> +	};
> +};

Best regards,
Krzysztof
  

On 10/01/2023 13:13, devi priya wrote:
> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> 
> Add initial device tree support for Qualcomm IPQ9574 SoC
> and AL02 board
> 
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>

Don't use CAPITAL CASE FOR NAMES BUT Camel Case. Although different
languages might have different rules, but most cases when transliterated
to English (or Latin alphabet), the name Start with capital letter. The
same in your case.

> ---
>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>  arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++

One more:

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Best regards,
Krzysztof
  

On 11/01/2023 10:44, Krzysztof Kozlowski wrote:
> And here
> 
>> +			clock-names = "xo", "iface", "core";
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

Apologies, wrong template. Correct comment:

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

Best regards,
Krzysztof
  

On 1/11/2023 3:14 PM, Krzysztof Kozlowski wrote:
> On 10/01/2023 13:13, devi priya wrote:
>> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>
>> Add initial device tree support for Qualcomm IPQ9574 SoC
>> and AL02 board
>>
>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/Makefile            |   1 +
>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>>   3 files changed, 388 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index 3e79496292e7..872c62028a0b 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c1.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c2.dtb
>> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq9574-al02-c7.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-alcatel-idol347.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-asus-z00l.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-huawei-g7.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> new file mode 100644
>> index 000000000000..ae3c32f3e16a
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
> Not dual licensed? Not BSD?
>
>> +/*
>> + * IPQ9574 AL02-C7 board device tree source
>> + *
>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "ipq9574.dtsi"
>> +
>> +/ {
>> +	model = "Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7";
>> +	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>> +	interrupt-parent = <&intc>;
Can this be droppeed? since it is already part of DTSI.
>> +
>> +	aliases {
>> +		serial0 = &blsp1_uart2;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +};
>> +
>> +&blsp1_uart2 {
>> +	pinctrl-0 = <&uart2_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&sdhc_1 {
>> +	pinctrl-0 = <&emmc_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&tlmm {
>> +	emmc_pins: emmc-state {
>> +		emmc-clk-pins {
>> +			pins = "gpio5";
>> +			function = "sdc_clk";
>> +			drive-strength = <8>;
>> +			bias-disable;
>> +		};
>> +		emmc-cmd-pins {
>> +			pins = "gpio4";
>> +			function = "sdc_cmd";
>> +			drive-strength = <8>;
>> +			bias-pull-up;
>> +		};
>> +		emmc-data-pins {
>> +			pins = "gpio0", "gpio1", "gpio2",
>> +			     "gpio3", "gpio6", "gpio7",
>> +			     "gpio8", "gpio9";
>> +			function = "sdc_data";
>> +			drive-strength = <8>;
>> +			bias-pull-up;
>> +		};
>> +		emmc-rclk-pins {
>> +			pins = "gpio10";
>> +			function = "sdc_rclk";
>> +			drive-strength = <8>;
>> +			bias-pull-down;
>> +		};
>> +	};
>> +
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> new file mode 100644
>> index 000000000000..188d18688a77
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * IPQ9574 SoC device tree source
>> + *
>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>> +#include <dt-bindings/reset/qcom,gcc-ipq9574.h>
>> +
>> +/ {
>> +	interrupt-parent = <&intc>;
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	clocks {
>> +		bias_pll_ubi_nc_clk: bias_pll_ubi_nc_clk {
> No undercores in node names.
>
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <353000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy2_pipe_clk: pcie30_phy2_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy3_pipe_clk: pcie30_phy3_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		usb3phy_0_cc_pipe_clk: usb3phy_0_cc_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <125000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		sleep_clk: sleep-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <32000>;
> That's not a property of the SoC, but board. Either entire clock or at
> least frequency to indicate that the board is providing the clock.
>
>> +			#clock-cells = <0>;> +		};
>> +
>> +		xo_board_clk: xo-board-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <24000000>;
> Ditto.
>
>> +			#clock-cells = <0>;
>> +		};
>> +	};
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		CPU0: cpu@0 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x0>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU1: cpu@1 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x1>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU2: cpu@2 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x2>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU3: cpu@3 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x3>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		L2_0: l2-cache {
>> +			compatible = "cache";
>> +			cache-level = <2>;
>> +		};
>> +	};
>> +
>> +	memory@40000000 {
>> +		device_type = "memory";
>> +		/* We expect the bootloader to fill in the size */
>> +		reg = <0x0 0x40000000 0x0 0x0>;
>> +	};
>> +
>> +	pmu {
>> +		compatible = "arm,cortex-a73-pmu";
>> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>> +	};
>> +
>> +	psci {
>> +		compatible = "arm,psci-1.0";
>> +		method = "smc";
>> +	};
>> +
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		tz_region: memory@4a600000 {
>> +			reg = <0x0 0x4a600000 0x0 0x400000>;
>> +			no-map;
>> +		};
>> +	};
>> +
>> +	soc: soc@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0 0 0 0xffffffff>;
>> +		compatible = "simple-bus";
>> +
>> +		tlmm: pinctrl@1000000 {
>> +			compatible = "qcom,ipq9574-tlmm";
>> +			reg = <0x01000000 0x300000>;
>> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			gpio-ranges = <&tlmm 0 0 65>;
>> +			gpio-reserved-ranges = <59 1>;
> Hm, why reserved ranges are in SoC?
>
>> +			interrupt-controller;
>> +			#interrupt-cells = <2>;
>> +
>> +			uart2_pins: uart2-state {
>> +				pins = "gpio34", "gpio35";
>> +				function = "blsp2_uart";
>> +				drive-strength = <8>;
>> +				bias-disable;
>> +			};
>> +		};
>> +
>> +		gcc: clock-controller@1800000 {
>> +			compatible = "qcom,gcc-ipq9574";
>> +			reg = <0x1800000 0x80000>;
Address can be padded to 8-hex-digits. Please take care of this in all 
nodes.
>> +			clocks = <&xo_board_clk>,
>> +				<&sleep_clk>,
>> +				<&bias_pll_ubi_nc_clk>,
>> +				<&pcie30_phy0_pipe_clk>,
>> +				<&pcie30_phy1_pipe_clk>,
>> +				<&pcie30_phy2_pipe_clk>,
>> +				<&pcie30_phy3_pipe_clk>,
>> +				<&usb3phy_0_cc_pipe_clk>;
>> +			clock-names = "xo",
>> +				"sleep_clk",
> Misaligned. Multiple other places probably as well.
>
>> +				"bias_pll_ubi_nc_clk",
>> +				"pcie30_phy0_pipe_clk",
>> +				"pcie30_phy1_pipe_clk",
>> +				"pcie30_phy2_pipe_clk",
>> +				"pcie30_phy3_pipe_clk",
>> +				"usb3phy_0_cc_pipe_clk";
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +		};
>> +
>> +		sdhc_1: sdhci@7804000 {
>> +			compatible = "qcom,sdhci-msm-v5";
>> +			reg = <0x7804000 0x1000>, <0x7805000 0x1000>;
>> +			reg-names = "hc_mem", "cmdq_mem";
>> +
>> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
>> +				   <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
> Like here.
>
>> +			interrupt-names = "hc_irq", "pwr_irq";
>> +
>> +			clocks = <&xo_board_clk>,
>> +			       <&gcc GCC_SDCC1_AHB_CLK>,
>> +			       <&gcc GCC_SDCC1_APPS_CLK>;
> And here
>
>> +			clock-names = "xo", "iface", "core";
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>
>> +			mmc-ddr-1_8v;
>> +			mmc-hs200-1_8v;
>> +			mmc-hs400-1_8v;
>> +			mmc-hs400-enhanced-strobe;
>> +			max-frequency = <384000000>;
>> +			bus-width = <8>;
>> +			non-removable;
>> +			status = "disabled";
>> +		};
>> +
>> +		blsp1_uart2: serial@78b1000 {
>> +			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>> +			reg = <0x078b1000 0x200>;
>> +			interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_BLSP1_UART3_APPS_CLK>,
>> +				 <&gcc GCC_BLSP1_AHB_CLK>;
>> +			clock-names = "core", "iface";
>> +			status = "disabled";
>> +		};
>> +
>> +		intc: interrupt-controller@b000000 {
>> +			compatible = "qcom,msm-qgic2";
>> +			reg = <0x0b000000 0x1000>,  /* GICD */
>> +			      <0x0b002000 0x1000>,  /* GICC */
>> +			      <0x0b001000 0x1000>,  /* GICH */
>> +			      <0x0b004000 0x1000>;  /* GICV */
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <3>;
>> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +			ranges = <0 0x0b00c000 0x3000>;
>> +
>> +			v2m0: v2m@0 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x0 0xffd>;
>> +				msi-controller;
>> +			};
>> +
>> +			v2m1: v2m@1 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x1000 0xffd>;
>> +				msi-controller;
>> +			};
>> +
>> +			v2m2: v2m@2 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x2000 0xffd>;
>> +				msi-controller;
>> +			};
>> +		};
>> +
>> +		timer@b120000 {
>> +			compatible = "arm,armv7-timer-mem";
>> +			reg = <0xb120000 0x1000>;
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges;
>> +			clock-frequency = <24000000>;
>> +
>> +			frame@b120000 {
>> +				reg = <0xb121000 0x1000>,
>> +				      <0xb122000 0x1000>;
>> +				frame-number = <0>;
>> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
>> +					     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>> +			};
>> +
>> +			frame@b123000 {
>> +				reg = <0xb123000 0x1000>;
>> +				frame-number = <1>;
>> +				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b124000 {
>> +				reg = <0xb124000 0x1000>;
>> +				frame-number = <2>;
>> +				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b125000 {
>> +				reg = <0xb125000 0x1000>;
>> +				frame-number = <3>;
>> +				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b126000 {
>> +				reg = <0xb126000 0x1000>;
>> +				frame-number = <4>;
>> +				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b127000 {
>> +				reg = <0xb127000 0x1000>;
>> +				frame-number = <5>;
>> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b128000 {
>> +				reg = <0xb128000 0x1000>;
>> +				frame-number = <6>;
>> +				interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +		};
>> +	};
>> +
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> +		clock-frequency = <24000000>;
> Is this allowed in recent designs?
>
>> +	};
>> +};
> Best regards,
> Krzysztof
>
  

On 1/11/2023 3:14 PM, Krzysztof Kozlowski wrote:
> On 10/01/2023 13:13, devi priya wrote:
>> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>
>> Add initial device tree support for Qualcomm IPQ9574 SoC
>> and AL02 board
>>
>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/Makefile            |   1 +
>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>>   3 files changed, 388 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index 3e79496292e7..872c62028a0b 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c1.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c2.dtb
>> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq9574-al02-c7.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-alcatel-idol347.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-asus-z00l.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-huawei-g7.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> new file mode 100644
>> index 000000000000..ae3c32f3e16a
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
> 
> Not dual licensed? Not BSD?
Okay, will add dual license
> 
>> +/*
>> + * IPQ9574 AL02-C7 board device tree source
>> + *
>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "ipq9574.dtsi"
>> +
>> +/ {
>> +	model = "Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7";
>> +	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>> +	interrupt-parent = <&intc>;
>> +
>> +	aliases {
>> +		serial0 = &blsp1_uart2;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +};
>> +
>> +&blsp1_uart2 {
>> +	pinctrl-0 = <&uart2_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&sdhc_1 {
>> +	pinctrl-0 = <&emmc_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&tlmm {
>> +	emmc_pins: emmc-state {
>> +		emmc-clk-pins {
>> +			pins = "gpio5";
>> +			function = "sdc_clk";
>> +			drive-strength = <8>;
>> +			bias-disable;
>> +		};
>> +		emmc-cmd-pins {
>> +			pins = "gpio4";
>> +			function = "sdc_cmd";
>> +			drive-strength = <8>;
>> +			bias-pull-up;
>> +		};
>> +		emmc-data-pins {
>> +			pins = "gpio0", "gpio1", "gpio2",
>> +			     "gpio3", "gpio6", "gpio7",
>> +			     "gpio8", "gpio9";
>> +			function = "sdc_data";
>> +			drive-strength = <8>;
>> +			bias-pull-up;
>> +		};
>> +		emmc-rclk-pins {
>> +			pins = "gpio10";
>> +			function = "sdc_rclk";
>> +			drive-strength = <8>;
>> +			bias-pull-down;
>> +		};
>> +	};
>> +
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> new file mode 100644
>> index 000000000000..188d18688a77
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * IPQ9574 SoC device tree source
>> + *
>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>> +#include <dt-bindings/reset/qcom,gcc-ipq9574.h>
>> +
>> +/ {
>> +	interrupt-parent = <&intc>;
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	clocks {
>> +		bias_pll_ubi_nc_clk: bias_pll_ubi_nc_clk {
> 
> No undercores in node names.
Okay
> 
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <353000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy2_pipe_clk: pcie30_phy2_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy3_pipe_clk: pcie30_phy3_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		usb3phy_0_cc_pipe_clk: usb3phy_0_cc_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <125000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		sleep_clk: sleep-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <32000>;
> 
> That's not a property of the SoC, but board. Either entire clock or at
> least frequency to indicate that the board is providing the clock.
Sure, will move it to board dts
> 
>> +			#clock-cells = <0>;> +		};
>> +
>> +		xo_board_clk: xo-board-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <24000000>;
> 
> Ditto.
Okay
> 
>> +			#clock-cells = <0>;
>> +		};
>> +	};
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		CPU0: cpu@0 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x0>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU1: cpu@1 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x1>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU2: cpu@2 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x2>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU3: cpu@3 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x3>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		L2_0: l2-cache {
>> +			compatible = "cache";
>> +			cache-level = <2>;
>> +		};
>> +	};
>> +
>> +	memory@40000000 {
>> +		device_type = "memory";
>> +		/* We expect the bootloader to fill in the size */
>> +		reg = <0x0 0x40000000 0x0 0x0>;
>> +	};
>> +
>> +	pmu {
>> +		compatible = "arm,cortex-a73-pmu";
>> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>> +	};
>> +
>> +	psci {
>> +		compatible = "arm,psci-1.0";
>> +		method = "smc";
>> +	};
>> +
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		tz_region: memory@4a600000 {
>> +			reg = <0x0 0x4a600000 0x0 0x400000>;
>> +			no-map;
>> +		};
>> +	};
>> +
>> +	soc: soc@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0 0 0 0xffffffff>;
>> +		compatible = "simple-bus";
>> +
>> +		tlmm: pinctrl@1000000 {
>> +			compatible = "qcom,ipq9574-tlmm";
>> +			reg = <0x01000000 0x300000>;
>> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			gpio-ranges = <&tlmm 0 0 65>;
>> +			gpio-reserved-ranges = <59 1>;
> 
> Hm, why reserved ranges are in SoC?
As the gpio is forbidden on all ipq9574 boards, we have added it in SoC
> 
>> +			interrupt-controller;
>> +			#interrupt-cells = <2>;
>> +
>> +			uart2_pins: uart2-state {
>> +				pins = "gpio34", "gpio35";
>> +				function = "blsp2_uart";
>> +				drive-strength = <8>;
>> +				bias-disable;
>> +			};
>> +		};
>> +
>> +		gcc: clock-controller@1800000 {
>> +			compatible = "qcom,gcc-ipq9574";
>> +			reg = <0x1800000 0x80000>;
>> +			clocks = <&xo_board_clk>,
>> +				<&sleep_clk>,
>> +				<&bias_pll_ubi_nc_clk>,
>> +				<&pcie30_phy0_pipe_clk>,
>> +				<&pcie30_phy1_pipe_clk>,
>> +				<&pcie30_phy2_pipe_clk>,
>> +				<&pcie30_phy3_pipe_clk>,
>> +				<&usb3phy_0_cc_pipe_clk>;
>> +			clock-names = "xo",
>> +				"sleep_clk",
> 
> Misaligned. Multiple other places probably as well.
Sure, will do
> 
>> +				"bias_pll_ubi_nc_clk",
>> +				"pcie30_phy0_pipe_clk",
>> +				"pcie30_phy1_pipe_clk",
>> +				"pcie30_phy2_pipe_clk",
>> +				"pcie30_phy3_pipe_clk",
>> +				"usb3phy_0_cc_pipe_clk";
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +		};
>> +
>> +		sdhc_1: sdhci@7804000 {
>> +			compatible = "qcom,sdhci-msm-v5";
>> +			reg = <0x7804000 0x1000>, <0x7805000 0x1000>;
>> +			reg-names = "hc_mem", "cmdq_mem";
>> +
>> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
>> +				   <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
> 
> Like here.
Okay
> 
>> +			interrupt-names = "hc_irq", "pwr_irq";
>> +
>> +			clocks = <&xo_board_clk>,
>> +			       <&gcc GCC_SDCC1_AHB_CLK>,
>> +			       <&gcc GCC_SDCC1_APPS_CLK>;
> 
> And here
Okay
> 
>> +			clock-names = "xo", "iface", "core";
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
Sure, will do
> 
>> +			mmc-ddr-1_8v;
>> +			mmc-hs200-1_8v;
>> +			mmc-hs400-1_8v;
>> +			mmc-hs400-enhanced-strobe;
>> +			max-frequency = <384000000>;
>> +			bus-width = <8>;
>> +			non-removable;
>> +			status = "disabled";
>> +		};
>> +
>> +		blsp1_uart2: serial@78b1000 {
>> +			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>> +			reg = <0x078b1000 0x200>;
>> +			interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_BLSP1_UART3_APPS_CLK>,
>> +				 <&gcc GCC_BLSP1_AHB_CLK>;
>> +			clock-names = "core", "iface";
>> +			status = "disabled";
>> +		};
>> +
>> +		intc: interrupt-controller@b000000 {
>> +			compatible = "qcom,msm-qgic2";
>> +			reg = <0x0b000000 0x1000>,  /* GICD */
>> +			      <0x0b002000 0x1000>,  /* GICC */
>> +			      <0x0b001000 0x1000>,  /* GICH */
>> +			      <0x0b004000 0x1000>;  /* GICV */
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <3>;
>> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +			ranges = <0 0x0b00c000 0x3000>;
>> +
>> +			v2m0: v2m@0 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x0 0xffd>;
>> +				msi-controller;
>> +			};
>> +
>> +			v2m1: v2m@1 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x1000 0xffd>;
>> +				msi-controller;
>> +			};
>> +
>> +			v2m2: v2m@2 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x2000 0xffd>;
>> +				msi-controller;
>> +			};
>> +		};
>> +
>> +		timer@b120000 {
>> +			compatible = "arm,armv7-timer-mem";
>> +			reg = <0xb120000 0x1000>;
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges;
>> +			clock-frequency = <24000000>;
>> +
>> +			frame@b120000 {
>> +				reg = <0xb121000 0x1000>,
>> +				      <0xb122000 0x1000>;
>> +				frame-number = <0>;
>> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
>> +					     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>> +			};
>> +
>> +			frame@b123000 {
>> +				reg = <0xb123000 0x1000>;
>> +				frame-number = <1>;
>> +				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b124000 {
>> +				reg = <0xb124000 0x1000>;
>> +				frame-number = <2>;
>> +				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b125000 {
>> +				reg = <0xb125000 0x1000>;
>> +				frame-number = <3>;
>> +				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b126000 {
>> +				reg = <0xb126000 0x1000>;
>> +				frame-number = <4>;
>> +				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b127000 {
>> +				reg = <0xb127000 0x1000>;
>> +				frame-number = <5>;
>> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b128000 {
>> +				reg = <0xb128000 0x1000>;
>> +				frame-number = <6>;
>> +				interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +		};
>> +	};
>> +
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> +		clock-frequency = <24000000>;
> 
> Is this allowed in recent designs?
Okay, will drop
> 
>> +	};
>> +};
> 
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya
  

On 1/11/2023 3:18 PM, Krzysztof Kozlowski wrote:
> On 11/01/2023 10:44, Krzysztof Kozlowski wrote:
>> And here
>>
>>> +			clock-names = "xo", "iface", "core";
>>
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> Apologies, wrong template. Correct comment:
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
> 
Sure, will check
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya
  

On 1/11/2023 8:22 PM, Kathiravan Thirumoorthy wrote:
> 
> On 1/11/2023 3:14 PM, Krzysztof Kozlowski wrote:
>> On 10/01/2023 13:13, devi priya wrote:
>>> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>>
>>> Add initial device tree support for Qualcomm IPQ9574 SoC
>>> and AL02 board
>>>
>>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>>> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
>>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>>> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/Makefile            |   1 +
>>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>>>   3 files changed, 388 insertions(+)
>>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/Makefile 
>>> b/arch/arm64/boot/dts/qcom/Makefile
>>> index 3e79496292e7..872c62028a0b 100644
>>> --- a/arch/arm64/boot/dts/qcom/Makefile
>>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>>> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)    += ipq6018-cp01-c1.dtb
>>>   dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk01.dtb
>>>   dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk10-c1.dtb
>>>   dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk10-c2.dtb
>>> +dtb-$(CONFIG_ARCH_QCOM)    += ipq9574-al02-c7.dtb
>>>   dtb-$(CONFIG_ARCH_QCOM)    += msm8916-alcatel-idol347.dtb
>>>   dtb-$(CONFIG_ARCH_QCOM)    += msm8916-asus-z00l.dtb
>>>   dtb-$(CONFIG_ARCH_QCOM)    += msm8916-huawei-g7.dtb
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts 
>>> b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>> new file mode 100644
>>> index 000000000000..ae3c32f3e16a
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>> @@ -0,0 +1,69 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>> Not dual licensed? Not BSD?
>>
>>> +/*
>>> + * IPQ9574 AL02-C7 board device tree source
>>> + *
>>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>>> reserved.
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "ipq9574.dtsi"
>>> +
>>> +/ {
>>> +    model = "Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7";
>>> +    compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>>> +    interrupt-parent = <&intc>;
> Can this be droppeed? since it is already part of DTSI.
Sure okay
>>> +
>>> +    aliases {
>>> +        serial0 = &blsp1_uart2;
>>> +    };
>>> +
>>> +    chosen {
>>> +        stdout-path = "serial0:115200n8";
>>> +    };
>>> +};
>>> +
>>> +&blsp1_uart2 {
>>> +    pinctrl-0 = <&uart2_pins>;
>>> +    pinctrl-names = "default";
>>> +    status = "okay";
>>> +};
>>> +
>>> +&sdhc_1 {
>>> +    pinctrl-0 = <&emmc_pins>;
>>> +    pinctrl-names = "default";
>>> +    status = "okay";
>>> +};
>>> +
>>> +&tlmm {
>>> +    emmc_pins: emmc-state {
>>> +        emmc-clk-pins {
>>> +            pins = "gpio5";
>>> +            function = "sdc_clk";
>>> +            drive-strength = <8>;
>>> +            bias-disable;
>>> +        };
>>> +        emmc-cmd-pins {
>>> +            pins = "gpio4";
>>> +            function = "sdc_cmd";
>>> +            drive-strength = <8>;
>>> +            bias-pull-up;
>>> +        };
>>> +        emmc-data-pins {
>>> +            pins = "gpio0", "gpio1", "gpio2",
>>> +                 "gpio3", "gpio6", "gpio7",
>>> +                 "gpio8", "gpio9";
>>> +            function = "sdc_data";
>>> +            drive-strength = <8>;
>>> +            bias-pull-up;
>>> +        };
>>> +        emmc-rclk-pins {
>>> +            pins = "gpio10";
>>> +            function = "sdc_rclk";
>>> +            drive-strength = <8>;
>>> +            bias-pull-down;
>>> +        };
>>> +    };
>>> +
>>> +};
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi 
>>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> new file mode 100644
>>> index 000000000000..188d18688a77
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> @@ -0,0 +1,318 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * IPQ9574 SoC device tree source
>>> + *
>>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights 
>>> reserved.
>>> + */
>>> +
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>>> +#include <dt-bindings/reset/qcom,gcc-ipq9574.h>
>>> +
>>> +/ {
>>> +    interrupt-parent = <&intc>;
>>> +    #address-cells = <2>;
>>> +    #size-cells = <2>;
>>> +
>>> +    clocks {
>>> +        bias_pll_ubi_nc_clk: bias_pll_ubi_nc_clk {
>> No undercores in node names.
>>
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <353000000>;
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>> +        pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <250000000>;
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>> +        pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <250000000>;
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>> +        pcie30_phy2_pipe_clk: pcie30_phy2_pipe_clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <250000000>;
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>> +        pcie30_phy3_pipe_clk: pcie30_phy3_pipe_clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <250000000>;
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>> +        usb3phy_0_cc_pipe_clk: usb3phy_0_cc_pipe_clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <125000000>;
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>> +        sleep_clk: sleep-clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <32000>;
>> That's not a property of the SoC, but board. Either entire clock or at
>> least frequency to indicate that the board is providing the clock.
>>
>>> +            #clock-cells = <0>;> +        };
>>> +
>>> +        xo_board_clk: xo-board-clk {
>>> +            compatible = "fixed-clock";
>>> +            clock-frequency = <24000000>;
>> Ditto.
>>
>>> +            #clock-cells = <0>;
>>> +        };
>>> +    };
>>> +
>>> +    cpus {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        CPU0: cpu@0 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a73";
>>> +            reg = <0x0>;
>>> +            enable-method = "psci";
>>> +            next-level-cache = <&L2_0>;
>>> +        };
>>> +
>>> +        CPU1: cpu@1 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a73";
>>> +            reg = <0x1>;
>>> +            enable-method = "psci";
>>> +            next-level-cache = <&L2_0>;
>>> +        };
>>> +
>>> +        CPU2: cpu@2 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a73";
>>> +            reg = <0x2>;
>>> +            enable-method = "psci";
>>> +            next-level-cache = <&L2_0>;
>>> +        };
>>> +
>>> +        CPU3: cpu@3 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a73";
>>> +            reg = <0x3>;
>>> +            enable-method = "psci";
>>> +            next-level-cache = <&L2_0>;
>>> +        };
>>> +
>>> +        L2_0: l2-cache {
>>> +            compatible = "cache";
>>> +            cache-level = <2>;
>>> +        };
>>> +    };
>>> +
>>> +    memory@40000000 {
>>> +        device_type = "memory";
>>> +        /* We expect the bootloader to fill in the size */
>>> +        reg = <0x0 0x40000000 0x0 0x0>;
>>> +    };
>>> +
>>> +    pmu {
>>> +        compatible = "arm,cortex-a73-pmu";
>>> +        interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | 
>>> IRQ_TYPE_LEVEL_HIGH)>;
>>> +    };
>>> +
>>> +    psci {
>>> +        compatible = "arm,psci-1.0";
>>> +        method = "smc";
>>> +    };
>>> +
>>> +    reserved-memory {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +        ranges;
>>> +
>>> +        tz_region: memory@4a600000 {
>>> +            reg = <0x0 0x4a600000 0x0 0x400000>;
>>> +            no-map;
>>> +        };
>>> +    };
>>> +
>>> +    soc: soc@0 {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges = <0 0 0 0xffffffff>;
>>> +        compatible = "simple-bus";
>>> +
>>> +        tlmm: pinctrl@1000000 {
>>> +            compatible = "qcom,ipq9574-tlmm";
>>> +            reg = <0x01000000 0x300000>;
>>> +            interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>>> +            gpio-controller;
>>> +            #gpio-cells = <2>;
>>> +            gpio-ranges = <&tlmm 0 0 65>;
>>> +            gpio-reserved-ranges = <59 1>;
>> Hm, why reserved ranges are in SoC?
>>
>>> +            interrupt-controller;
>>> +            #interrupt-cells = <2>;
>>> +
>>> +            uart2_pins: uart2-state {
>>> +                pins = "gpio34", "gpio35";
>>> +                function = "blsp2_uart";
>>> +                drive-strength = <8>;
>>> +                bias-disable;
>>> +            };
>>> +        };
>>> +
>>> +        gcc: clock-controller@1800000 {
>>> +            compatible = "qcom,gcc-ipq9574";
>>> +            reg = <0x1800000 0x80000>;
> Address can be padded to 8-hex-digits. Please take care of this in all 
> nodes.
Sure, will do
>>> +            clocks = <&xo_board_clk>,
>>> +                <&sleep_clk>,
>>> +                <&bias_pll_ubi_nc_clk>,
>>> +                <&pcie30_phy0_pipe_clk>,
>>> +                <&pcie30_phy1_pipe_clk>,
>>> +                <&pcie30_phy2_pipe_clk>,
>>> +                <&pcie30_phy3_pipe_clk>,
>>> +                <&usb3phy_0_cc_pipe_clk>;
>>> +            clock-names = "xo",
>>> +                "sleep_clk",
>> Misaligned. Multiple other places probably as well.
>>
>>> +                "bias_pll_ubi_nc_clk",
>>> +                "pcie30_phy0_pipe_clk",
>>> +                "pcie30_phy1_pipe_clk",
>>> +                "pcie30_phy2_pipe_clk",
>>> +                "pcie30_phy3_pipe_clk",
>>> +                "usb3phy_0_cc_pipe_clk";
>>> +            #clock-cells = <1>;
>>> +            #reset-cells = <1>;
>>> +        };
>>> +
>>> +        sdhc_1: sdhci@7804000 {
>>> +            compatible = "qcom,sdhci-msm-v5";
>>> +            reg = <0x7804000 0x1000>, <0x7805000 0x1000>;
>>> +            reg-names = "hc_mem", "cmdq_mem";
>>> +
>>> +            interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
>>> +                   <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
>> Like here.
>>
>>> +            interrupt-names = "hc_irq", "pwr_irq";
>>> +
>>> +            clocks = <&xo_board_clk>,
>>> +                   <&gcc GCC_SDCC1_AHB_CLK>,
>>> +                   <&gcc GCC_SDCC1_APPS_CLK>;
>> And here
>>
>>> +            clock-names = "xo", "iface", "core";
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>
>>> +            mmc-ddr-1_8v;
>>> +            mmc-hs200-1_8v;
>>> +            mmc-hs400-1_8v;
>>> +            mmc-hs400-enhanced-strobe;
>>> +            max-frequency = <384000000>;
>>> +            bus-width = <8>;
>>> +            non-removable;
>>> +            status = "disabled";
>>> +        };
>>> +
>>> +        blsp1_uart2: serial@78b1000 {
>>> +            compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>>> +            reg = <0x078b1000 0x200>;
>>> +            interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
>>> +            clocks = <&gcc GCC_BLSP1_UART3_APPS_CLK>,
>>> +                 <&gcc GCC_BLSP1_AHB_CLK>;
>>> +            clock-names = "core", "iface";
>>> +            status = "disabled";
>>> +        };
>>> +
>>> +        intc: interrupt-controller@b000000 {
>>> +            compatible = "qcom,msm-qgic2";
>>> +            reg = <0x0b000000 0x1000>,  /* GICD */
>>> +                  <0x0b002000 0x1000>,  /* GICC */
>>> +                  <0x0b001000 0x1000>,  /* GICH */
>>> +                  <0x0b004000 0x1000>;  /* GICV */
>>> +            #address-cells = <1>;
>>> +            #size-cells = <1>;
>>> +            interrupt-controller;
>>> +            #interrupt-cells = <3>;
>>> +            interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>>> +            ranges = <0 0x0b00c000 0x3000>;
>>> +
>>> +            v2m0: v2m@0 {
>>> +                compatible = "arm,gic-v2m-frame";
>>> +                reg = <0x0 0xffd>;
>>> +                msi-controller;
>>> +            };
>>> +
>>> +            v2m1: v2m@1 {
>>> +                compatible = "arm,gic-v2m-frame";
>>> +                reg = <0x1000 0xffd>;
>>> +                msi-controller;
>>> +            };
>>> +
>>> +            v2m2: v2m@2 {
>>> +                compatible = "arm,gic-v2m-frame";
>>> +                reg = <0x2000 0xffd>;
>>> +                msi-controller;
>>> +            };
>>> +        };
>>> +
>>> +        timer@b120000 {
>>> +            compatible = "arm,armv7-timer-mem";
>>> +            reg = <0xb120000 0x1000>;
>>> +            #address-cells = <1>;
>>> +            #size-cells = <1>;
>>> +            ranges;
>>> +            clock-frequency = <24000000>;
>>> +
>>> +            frame@b120000 {
>>> +                reg = <0xb121000 0x1000>,
>>> +                      <0xb122000 0x1000>;
>>> +                frame-number = <0>;
>>> +                interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>>> +            };
>>> +
>>> +            frame@b123000 {
>>> +                reg = <0xb123000 0x1000>;
>>> +                frame-number = <1>;
>>> +                interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>>> +                status = "disabled";
>>> +            };
>>> +
>>> +            frame@b124000 {
>>> +                reg = <0xb124000 0x1000>;
>>> +                frame-number = <2>;
>>> +                interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>>> +                status = "disabled";
>>> +            };
>>> +
>>> +            frame@b125000 {
>>> +                reg = <0xb125000 0x1000>;
>>> +                frame-number = <3>;
>>> +                interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>>> +                status = "disabled";
>>> +            };
>>> +
>>> +            frame@b126000 {
>>> +                reg = <0xb126000 0x1000>;
>>> +                frame-number = <4>;
>>> +                interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>>> +                status = "disabled";
>>> +            };
>>> +
>>> +            frame@b127000 {
>>> +                reg = <0xb127000 0x1000>;
>>> +                frame-number = <5>;
>>> +                interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
>>> +                status = "disabled";
>>> +            };
>>> +
>>> +            frame@b128000 {
>>> +                reg = <0xb128000 0x1000>;
>>> +                frame-number = <6>;
>>> +                interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>>> +                status = "disabled";
>>> +            };
>>> +        };
>>> +    };
>>> +
>>> +    timer {
>>> +        compatible = "arm,armv8-timer";
>>> +        interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | 
>>> IRQ_TYPE_LEVEL_LOW)>,
>>> +                 <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | 
>>> IRQ_TYPE_LEVEL_LOW)>,
>>> +                 <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | 
>>> IRQ_TYPE_LEVEL_LOW)>,
>>> +                 <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | 
>>> IRQ_TYPE_LEVEL_LOW)>;
>>> +        clock-frequency = <24000000>;
>> Is this allowed in recent designs?
>>
>>> +    };
>>> +};
>> Best regards,
>> Krzysztof
>>
Best Regards,
Devi Priya
  

On 1/11/2023 3:15 PM, Krzysztof Kozlowski wrote:
> On 10/01/2023 13:13, devi priya wrote:
>> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>
>> Add initial device tree support for Qualcomm IPQ9574 SoC
>> and AL02 board
>>
>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> 
> Don't use CAPITAL CASE FOR NAMES BUT Camel Case. Although different
> languages might have different rules, but most cases when transliterated
> to English (or Latin alphabet), the name Start with capital letter. The
> same in your case.
Sure, okay
> 
>> ---
>>   arch/arm64/boot/dts/qcom/Makefile            |   1 +
>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
> 
> One more:
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
> 
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya
  

On 2023-01-10 12:13, devi priya wrote:
> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> 
> Add initial device tree support for Qualcomm IPQ9574 SoC
> and AL02 board
> 
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>  arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>  3 files changed, 388 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile
> b/arch/arm64/boot/dts/qcom/Makefile
> index 3e79496292e7..872c62028a0b 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c2.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq9574-al02-c7.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-alcatel-idol347.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-asus-z00l.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-huawei-g7.dtb
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> new file mode 100644
> index 000000000000..ae3c32f3e16a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts

[...]

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

Interesting choice for the PPIs...

> +		clock-frequency = <24000000>;

Please drop this and fix the firmware. No system built within
past 10 years should need it.

         M.
  

On 1/13/2023 7:19 PM, Marc Zyngier wrote:
> On 2023-01-10 12:13, devi priya wrote:
>> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>
>> Add initial device tree support for Qualcomm IPQ9574 SoC
>> and AL02 board
>>
>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>>  arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>>  arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>>  3 files changed, 388 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>  create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile
>> b/arch/arm64/boot/dts/qcom/Makefile
>> index 3e79496292e7..872c62028a0b 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)    += ipq6018-cp01-c1.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk01.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk10-c1.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk10-c2.dtb
>> +dtb-$(CONFIG_ARCH_QCOM)    += ipq9574-al02-c7.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)    += msm8916-alcatel-idol347.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)    += msm8916-asus-z00l.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)    += msm8916-huawei-g7.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> new file mode 100644
>> index 000000000000..ae3c32f3e16a
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
> 
> [...]
> 
>> +    timer {
>> +        compatible = "arm,armv8-timer";
>> +        interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>,
>> +                 <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>,
>> +                 <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>,
>> +                 <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>;
> 
> Interesting choice for the PPIs...
> 
>> +        clock-frequency = <24000000>;
> 
> Please drop this and fix the firmware. No system built within
> past 10 years should need it.
Sure, will drop!
> 
>          M.
Best Regards,
Devi Priya
  

On 13/01/2023 14:29, Devi Priya wrote:
>>> +
>>> +	soc: soc@0 {
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		ranges = <0 0 0 0xffffffff>;
>>> +		compatible = "simple-bus";
>>> +
>>> +		tlmm: pinctrl@1000000 {
>>> +			compatible = "qcom,ipq9574-tlmm";
>>> +			reg = <0x01000000 0x300000>;
>>> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>>> +			gpio-controller;
>>> +			#gpio-cells = <2>;
>>> +			gpio-ranges = <&tlmm 0 0 65>;
>>> +			gpio-reserved-ranges = <59 1>;
>>
>> Hm, why reserved ranges are in SoC?
> As the gpio is forbidden on all ipq9574 boards, we have added it in SoC

Why it is forbidden on all boards? I guess it depends on the firmware
and this can differ, can't it?

Best regards,
Krzysztof
  

On 1/10/2023 6:20 PM, Konrad Dybcio wrote:
> 
> 
> On 10.01.2023 13:13, devi priya wrote:
>> From: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>>
>> Add initial device tree support for Qualcomm IPQ9574 SoC
>> and AL02 board
>>
>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>> Co-developed-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> Signed-off-by: POOVENDHAN SELVARAJ <quic_poovendh@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/Makefile            |   1 +
>>   arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts |  69 ++++
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi        | 318 +++++++++++++++++++
>>   3 files changed, 388 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>>   create mode 100644 arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index 3e79496292e7..872c62028a0b 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c1.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk10-c2.dtb
>> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq9574-al02-c7.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-alcatel-idol347.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-asus-z00l.dtb
>>   dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-huawei-g7.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> new file mode 100644
>> index 000000000000..ae3c32f3e16a
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dts
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
> BSD3?
> 
Sure, will add dual license
>> +/*
>> + * IPQ9574 AL02-C7 board device tree source
>> + *
>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> Happy new year!
> 
Sure, will update!
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "ipq9574.dtsi"
>> +
>> +/ {
>> +	model = "Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7";
>> +	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>> +	interrupt-parent = <&intc>;
>> +
>> +	aliases {
>> +		serial0 = &blsp1_uart2;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +};
>> +
>> +&blsp1_uart2 {
>> +	pinctrl-0 = <&uart2_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&sdhc_1 {
>> +	pinctrl-0 = <&emmc_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&tlmm {
>> +	emmc_pins: emmc-state {
> sdc_default? I suppose you'll introduce a corresponding sleep state
> later on, so that'll be easier to distinguish.
Sure, will update the node name. But, we don't have power management 
states. It would always be in the running state.
> 
>> +		emmc-clk-pins {
>> +			pins = "gpio5";
>> +			function = "sdc_clk";
>> +			drive-strength = <8>;
>> +			bias-disable;
>> +		};
> Please add a newline between subsequent nodes.
Sure, okay
>> +		emmc-cmd-pins {
>> +			pins = "gpio4";
>> +			function = "sdc_cmd";
>> +			drive-strength = <8>;
>> +			bias-pull-up;
>> +		};
>> +		emmc-data-pins {
>> +			pins = "gpio0", "gpio1", "gpio2",
>> +			     "gpio3", "gpio6", "gpio7",
>> +			     "gpio8", "gpio9";
> The indentation here is wrong.
Sure, will update
> 
>> +			function = "sdc_data";
>> +			drive-strength = <8>;
>> +			bias-pull-up;
>> +		};
>> +		emmc-rclk-pins {
>> +			pins = "gpio10";
>> +			function = "sdc_rclk";
>> +			drive-strength = <8>;
>> +			bias-pull-down;
>> +		};
>> +	};
>> +
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> new file mode 100644
>> index 000000000000..188d18688a77
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * IPQ9574 SoC device tree source
>> + *
>> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>> +#include <dt-bindings/reset/qcom,gcc-ipq9574.h>
>> +
>> +/ {
>> +	interrupt-parent = <&intc>;
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	clocks {
>> +		bias_pll_ubi_nc_clk: bias_pll_ubi_nc_clk {
> Could you tell us something about the purpose of this clock? First
> time seeing it, your gcc driver reveals it's connected to at least
> PCIe.
Bias_pll_ubi_nc_clk (353MHz) is a backup source for 
Q6_AXIM2_CLK/PCIE2_AXIM_CLK/PCIE3_AXIM_CLK/SNOC-CLK.
> 
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <353000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy0_pipe_clk: pcie30_phy0_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy1_pipe_clk: pcie30_phy1_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy2_pipe_clk: pcie30_phy2_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		pcie30_phy3_pipe_clk: pcie30_phy3_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <250000000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		usb3phy_0_cc_pipe_clk: usb3phy_0_cc_pipe_clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <125000000>;
>> +			#clock-cells = <0>;
>> +		};
> Do not define these pipe clocks. You can leave the GCC entries as <0>
> until you introduce the QMPPHY support, which then you can feed as it
> provides these clocks.
Sure, okay
> 
>> +
>> +		sleep_clk: sleep-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <32000>;
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		xo_board_clk: xo-board-clk {
>> +			compatible = "fixed-clock";
>> +			clock-frequency = <24000000>;
> The clock frequency should be moved to the device DT, because the
> clock is on the board and not on the SoC.
Okay, will move to device DT
> 
>> +			#clock-cells = <0>;
>> +		};
>> +	};
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		CPU0: cpu@0 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x0>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU1: cpu@1 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x1>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU2: cpu@2 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x2>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		CPU3: cpu@3 {
>> +			device_type = "cpu";
>> +			compatible = "arm,cortex-a73";
>> +			reg = <0x3>;
>> +			enable-method = "psci";
>> +			next-level-cache = <&L2_0>;
>> +		};
>> +
>> +		L2_0: l2-cache {
>> +			compatible = "cache";
>> +			cache-level = <2>;
>> +		};
>> +	};
>> +
>> +	memory@40000000 {
>> +		device_type = "memory";
>> +		/* We expect the bootloader to fill in the size */
>> +		reg = <0x0 0x40000000 0x0 0x0>;
>> +	};
>> +
>> +	pmu {
>> +		compatible = "arm,cortex-a73-pmu";
>> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>> +	};
>> +
>> +	psci {
>> +		compatible = "arm,psci-1.0";
>> +		method = "smc";
>> +	};
>> +
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		tz_region: memory@4a600000 {
>> +			reg = <0x0 0x4a600000 0x0 0x400000>;
>> +			no-map;
>> +		};
> That's.. surprisingly little reserved memory.. No hyp? No PIL regions
> that make the board explode when something touches them?
> 
The reserved memory for TZ region is 4Mb. yes,we have hypervisor and PIL 
regions.
The memory for PIL region will be separately reserved by the remoteproc
>> +	};
>> +
>> +	soc: soc@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0 0 0 0xffffffff>;
> Is 32 bits enough for this SoC's bus? Newer ones use 36 or more..
Yes, as the maximum range supported by ipq9574 is 4GB, 32 bits would be 
sufficient
> 
>> +		compatible = "simple-bus";
>> +
>> +		tlmm: pinctrl@1000000 {
>> +			compatible = "qcom,ipq9574-tlmm";
>> +			reg = <0x01000000 0x300000>;
>> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			gpio-ranges = <&tlmm 0 0 65>;
>> +			gpio-reserved-ranges = <59 1>;
> I see it's assigned to [rx0, pwm23, qdss_tracedata_a].. Is this
> board-specific or is this pin supposed to be forbidden on all IPQ9574
> boards?
This pin is reserved by TZ and is forbidden on all IPQ9574 boards.
> 
>> +			interrupt-controller;
>> +			#interrupt-cells = <2>;
>> +
>> +			uart2_pins: uart2-state {
>> +				pins = "gpio34", "gpio35";
>> +				function = "blsp2_uart";
>> +				drive-strength = <8>;
>> +				bias-disable;
>> +			};
>> +		};
>> +
>> +		gcc: clock-controller@1800000 {
>> +			compatible = "qcom,gcc-ipq9574";
>> +			reg = <0x1800000 0x80000>;
>> +			clocks = <&xo_board_clk>,
>> +				<&sleep_clk>,
>> +				<&bias_pll_ubi_nc_clk>,
>> +				<&pcie30_phy0_pipe_clk>,
>> +				<&pcie30_phy1_pipe_clk>,
>> +				<&pcie30_phy2_pipe_clk>,
>> +				<&pcie30_phy3_pipe_clk>,
>> +				<&usb3phy_0_cc_pipe_clk>;
>> +			clock-names = "xo",
>> +				"sleep_clk",
>> +				"bias_pll_ubi_nc_clk",
>> +				"pcie30_phy0_pipe_clk",
>> +				"pcie30_phy1_pipe_clk",
>> +				"pcie30_phy2_pipe_clk",
>> +				"pcie30_phy3_pipe_clk",
>> +				"usb3phy_0_cc_pipe_clk";
> Please touch up the indentation.
> 
Sure okay
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +		};
>> +
>> +		sdhc_1: sdhci@7804000 {
>> +			compatible = "qcom,sdhci-msm-v5";
>> +			reg = <0x7804000 0x1000>, <0x7805000 0x1000>;
>> +			reg-names = "hc_mem", "cmdq_mem";
>> +
>> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
>> +				   <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
> Please touch up the indentation.
Okay
> 
>> +			interrupt-names = "hc_irq", "pwr_irq";
>> +
>> +			clocks = <&xo_board_clk>,
>> +			       <&gcc GCC_SDCC1_AHB_CLK>,
>> +			       <&gcc GCC_SDCC1_APPS_CLK>;
> Please touch up the indentation.
> 
Sure, will do
>> +			clock-names = "xo", "iface", "core";
> The order should be "iface", "core", "xo" as per Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
Got it, will update
> 
>> +			mmc-ddr-1_8v;
>> +			mmc-hs200-1_8v;
>> +			mmc-hs400-1_8v;
>> +			mmc-hs400-enhanced-strobe;
> Are these the limitations of the controller? Otherwise they should
> probably be moved to the device-specific DT.
Yes, these are specific to the controller, hence added them in SoC
> 
>> +			max-frequency = <384000000>;
>> +			bus-width = <8>;
> 
>> +			non-removable;
> And this property too.
Yes, it is specific to the controller
>> +			status = "disabled";
>> +		};
>> +
>> +		blsp1_uart2: serial@78b1000 {
>> +			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>> +			reg = <0x078b1000 0x200>;
>> +			interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_BLSP1_UART3_APPS_CLK>,
>> +				 <&gcc GCC_BLSP1_AHB_CLK>;
>> +			clock-names = "core", "iface";
>> +			status = "disabled";
>> +		};
>> +
>> +		intc: interrupt-controller@b000000 {
>> +			compatible = "qcom,msm-qgic2";
>> +			reg = <0x0b000000 0x1000>,  /* GICD */
>> +			      <0x0b002000 0x1000>,  /* GICC */
>> +			      <0x0b001000 0x1000>,  /* GICH */
>> +			      <0x0b004000 0x1000>;  /* GICV */
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <3>;
>> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +			ranges = <0 0x0b00c000 0x3000>;
>> +
>> +			v2m0: v2m@0 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x0 0xffd>;
>> +				msi-controller;
>> +			};
>> +
>> +			v2m1: v2m@1 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x1000 0xffd>;
>> +				msi-controller;
>> +			};
>> +
>> +			v2m2: v2m@2 {
>> +				compatible = "arm,gic-v2m-frame";
>> +				reg = <0x2000 0xffd>;
>> +				msi-controller;
>> +			};
>> +		};
>> +
>> +		timer@b120000 {
>> +			compatible = "arm,armv7-timer-mem";
>> +			reg = <0xb120000 0x1000>;
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges;
>> +			clock-frequency = <24000000>;
> Drop, something earlier in the boot chain already writes to CNTFRQ_ELn.
> 
Sure, okay
>> +
>> +			frame@b120000 {
>> +				reg = <0xb121000 0x1000>,
>> +				      <0xb122000 0x1000>;
>> +				frame-number = <0>;
>> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
>> +					     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>> +			};
>> +
>> +			frame@b123000 {
>> +				reg = <0xb123000 0x1000>;
>> +				frame-number = <1>;
>> +				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b124000 {
>> +				reg = <0xb124000 0x1000>;
>> +				frame-number = <2>;
>> +				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b125000 {
>> +				reg = <0xb125000 0x1000>;
>> +				frame-number = <3>;
>> +				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b126000 {
>> +				reg = <0xb126000 0x1000>;
>> +				frame-number = <4>;
>> +				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b127000 {
>> +				reg = <0xb127000 0x1000>;
>> +				frame-number = <5>;
>> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +
>> +			frame@b128000 {
>> +				reg = <0xb128000 0x1000>;
>> +				frame-number = <6>;
>> +				interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +		};
>> +	};
>> +
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> +		clock-frequency = <24000000>;
> Drop, something earlier in the boot chain already writes to CNTFRQ_ELn.
> 
Okay
> Konrad
>> +	};
>> +};
Best Regards,
Devi Priya
  

On 1/13/2023 7:50 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 14:29, Devi Priya wrote:
>>>> +
>>>> +	soc: soc@0 {
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <1>;
>>>> +		ranges = <0 0 0 0xffffffff>;
>>>> +		compatible = "simple-bus";
>>>> +
>>>> +		tlmm: pinctrl@1000000 {
>>>> +			compatible = "qcom,ipq9574-tlmm";
>>>> +			reg = <0x01000000 0x300000>;
>>>> +			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>>>> +			gpio-controller;
>>>> +			#gpio-cells = <2>;
>>>> +			gpio-ranges = <&tlmm 0 0 65>;
>>>> +			gpio-reserved-ranges = <59 1>;
>>>
>>> Hm, why reserved ranges are in SoC?
>> As the gpio is forbidden on all ipq9574 boards, we have added it in SoC
> 
> Why it is forbidden on all boards? I guess it depends on the firmware
> and this can differ, can't it?
> 
This GPIO is protected and used by the TZ firmware and is forbidden on 
all the boards & firmware
> Best regards,
> Krzysztof
> 
Regards,
Devi Priya
  

On 24/01/2023 08:19, Devi Priya wrote:
>>>>> +			gpio-ranges = <&tlmm 0 0 65>;
>>>>> +			gpio-reserved-ranges = <59 1>;
>>>>
>>>> Hm, why reserved ranges are in SoC?
>>> As the gpio is forbidden on all ipq9574 boards, we have added it in SoC
>>
>> Why it is forbidden on all boards? I guess it depends on the firmware
>> and this can differ, can't it?
>>
> This GPIO is protected and used by the TZ firmware and is forbidden on 
> all the boards & firmware

OK

Best regards,
Krzysztof
  

On 24.01.2023 08:19, Devi Priya wrote:
> 
> 
> On 1/13/2023 7:50 PM, Krzysztof Kozlowski wrote:
>> On 13/01/2023 14:29, Devi Priya wrote:
>>>>> +
>>>>> +    soc: soc@0 {
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <1>;
>>>>> +        ranges = <0 0 0 0xffffffff>;
>>>>> +        compatible = "simple-bus";
>>>>> +
>>>>> +        tlmm: pinctrl@1000000 {
>>>>> +            compatible = "qcom,ipq9574-tlmm";
>>>>> +            reg = <0x01000000 0x300000>;
>>>>> +            interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +            gpio-controller;
>>>>> +            #gpio-cells = <2>;
>>>>> +            gpio-ranges = <&tlmm 0 0 65>;
>>>>> +            gpio-reserved-ranges = <59 1>;
>>>>
>>>> Hm, why reserved ranges are in SoC?
>>> As the gpio is forbidden on all ipq9574 boards, we have added it in SoC
>>
>> Why it is forbidden on all boards? I guess it depends on the firmware
>> and this can differ, can't it?
>>
> This GPIO is protected and used by the TZ firmware and is forbidden on all the boards & firmware
If it's protected on *all* boards and *all* firmwares (for any
good reason that you probably have internally), perhaps it
would be better to describe it in the .c driver.. wdyt?

Konrad
>> Best regards,
>> Krzysztof
>>
> Regards,
> Devi Priya
  

On 1/24/2023 3:16 PM, Konrad Dybcio wrote:
> 
> 
> On 24.01.2023 08:19, Devi Priya wrote:
>>
>>
>> On 1/13/2023 7:50 PM, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 14:29, Devi Priya wrote:
>>>>>> +
>>>>>> +    soc: soc@0 {
>>>>>> +        #address-cells = <1>;
>>>>>> +        #size-cells = <1>;
>>>>>> +        ranges = <0 0 0 0xffffffff>;
>>>>>> +        compatible = "simple-bus";
>>>>>> +
>>>>>> +        tlmm: pinctrl@1000000 {
>>>>>> +            compatible = "qcom,ipq9574-tlmm";
>>>>>> +            reg = <0x01000000 0x300000>;
>>>>>> +            interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>>>>>> +            gpio-controller;
>>>>>> +            #gpio-cells = <2>;
>>>>>> +            gpio-ranges = <&tlmm 0 0 65>;
>>>>>> +            gpio-reserved-ranges = <59 1>;
>>>>>
>>>>> Hm, why reserved ranges are in SoC?
>>>> As the gpio is forbidden on all ipq9574 boards, we have added it in SoC
>>>
>>> Why it is forbidden on all boards? I guess it depends on the firmware
>>> and this can differ, can't it?
>>>
>> This GPIO is protected and used by the TZ firmware and is forbidden on all the boards & firmware
> If it's protected on *all* boards and *all* firmwares (for any
> good reason that you probably have internally), perhaps it
> would be better to describe it in the .c driver.. wdyt?
> 
Sure, okay. Will add the gpio to reserved_gpios entry in the pinctrl driver
> Konrad
>>> Best regards,
>>> Krzysztof
>>>
>> Regards,
>> Devi Priya