Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Sricharan R <sricharan@codeaurora.org>
Cc: robh+dt@kernel.org, sboyd@codeaurora.org,
	linus.walleij@linaro.org, agross@kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/6] arm64: dts: Add ipq6018 SoC and CP01 board support
Date: Fri, 7 Jun 2019 20:48:35 -0700
Message-ID: <20190608034835.GH24059@builder> (raw)
In-Reply-To: <1559754961-26783-6-git-send-email-sricharan@codeaurora.org>

On Wed 05 Jun 10:16 PDT 2019, Sricharan R wrote:

> Add initial device tree support for the Qualcomm IPQ6018 SoC and
> CP01 evaluation board.
> 
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>

Please fix the order of these (or add a Co-developed-by).

> ---
>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>  arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dts |  35 ++++
>  arch/arm64/boot/dts/qcom/ipq6018.dtsi        | 231 +++++++++++++++++++++++++++
>  3 files changed, 267 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/ipq6018.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 21d548f..ac22dbb 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -2,6 +2,7 @@
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8096-db820c.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= ipq8074-hk01.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= ipq6018-cp01-c1.dtb

Sort order.

>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-mtp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8992-bullhead-rev-101.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-angler-rev-101.dtb
> diff --git a/arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dts b/arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dts
> new file mode 100644
> index 0000000..ac7cb22
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq6018-cp01-c1.dts
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IPQ6018 CP01 board device tree source
> + *
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include "ipq6018.dtsi"
> +
> +/ {
> +	#address-cells = <0x2>;
> +	#size-cells = <0x2>;

This is a count, write it in base 10..

> +	model = "Qualcomm Technologies, Inc. IPQ6018/AP-CP01-C1";
> +	compatible = "qcom,ipq6018-cp01", "qcom,ipq6018";
> +	interrupt-parent = <&intc>;

Changing #address-cells, #size-cells and interrupt-parent will break the
dtsi, so I think you should specify them there.

> +};
> +
> +&tlmm {

Please sort your nodes based on address, then node name, then label.

> +	uart_pins: uart_pins {
> +		mux {
> +			pins = "gpio44", "gpio45";
> +			function = "blsp2_uart";
> +			drive-strength = <8>;
> +			bias-pull-down;
> +		};
> +	};
> +};
> +
> +&blsp1_uart3 {
> +	pinctrl-0 = <&uart_pins>;
> +	pinctrl-names = "default";
> +	status = "ok";
> +};
> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> new file mode 100644
> index 0000000..79cccdd
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IPQ6018 SoC device tree source
> + *
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,gcc-ipq6018.h>
> +
> +/ {
> +	model = "Qualcomm Technologies, Inc. IPQ6018";
> +	compatible = "qcom,ipq6018";

No need for model and compatible in the dtsi, these should always be
specified by the including file.

> +
> +	chosen {
> +		bootargs = "console=ttyMSM0,115200,n8 rw init=/init";

Do you really need console? Can't you use stdout-path?

And there's no need to specify init=/init.

> +		bootargs-append = " swiotlb=1 clk_ignore_unused";

I'm hoping that you will work on removing the need for
clk_ignore_unused.

> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		tz:tz@48500000 {

Space after :

> +			no-map;
> +			reg = <0x0 0x48500000 0x0 0x00200000>;

I would prefer to have the reg first in these nodes, then the region's
properties.

> +		};
> +	};
> +
> +	soc: soc {
> +		#address-cells = <0x1>;
> +		#size-cells = <0x1>;
> +		ranges = <0 0 0 0xffffffff>;
> +		dma-ranges;
> +		compatible = "simple-bus";
> +
> +		intc: interrupt-controller@b000000 {

As described above, please sort your nodes based on address, node name
and lastly label name.

> +			compatible = "qcom,msm-qgic2";
> +			interrupt-controller;
> +			#interrupt-cells = <0x3>;
> +			reg = <0xb000000 0x1000>, <0xb002000 0x1000>;
> +		};
> +
> +		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)>;
> +		};
> +
> +		timer@b120000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0xb120000 0x1000>;

Please pad addresses in reg to 8 digits, to make them faster to compare.

> +			clock-frequency = <19200000>;
> +
> +			frame@b120000 {
> +				frame-number = <0>;
> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +					     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb121000 0x1000>,
> +				      <0xb122000 0x1000>;
> +			};
> +
> +			frame@b123000 {
> +				frame-number = <1>;
> +				interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb123000 0x1000>;
> +				status = "disabled";
> +			};
> +
> +			frame@b124000 {
> +				frame-number = <2>;
> +				interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb124000 0x1000>;
> +				status = "disabled";
> +			};
> +
> +			frame@b125000 {
> +				frame-number = <3>;
> +				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb125000 0x1000>;
> +				status = "disabled";
> +			};
> +
> +			frame@b126000 {
> +				frame-number = <4>;
> +				interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb126000 0x1000>;
> +				status = "disabled";
> +			};
> +
> +			frame@b127000 {
> +				frame-number = <5>;
> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb127000 0x1000>;
> +				status = "disabled";
> +			};
> +
> +			frame@b128000 {
> +				frame-number = <6>;
> +				interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0xb128000 0x1000>;
> +				status = "disabled";
> +			};
> +		};
> +
> +		gcc: gcc@1800000 {
> +			compatible = "qcom,gcc-ipq6018";
> +			reg = <0x1800000 0x80000>;
> +			#clock-cells = <0x1>;

This is a count, use base 10.

> +			#reset-cells = <0x1>;
> +		};
> +
> +		blsp1_uart3: serial@78b1000 {
> +			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
> +			reg = <0x78b1000 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";
> +		};
> +
> +		tlmm: pinctrl@1000000 {
> +			compatible = "qcom,ipq6018-pinctrl";
> +			reg = <0x1000000 0x300000>;
> +			interrupts = <GIC_SPI 0xd0 IRQ_TYPE_NONE>;
> +			gpio-controller;
> +			#gpio-cells = <0x2>;

gpio-ranges = <&tlmm 0 80>;

> +			interrupt-controller;
> +			#interrupt-cells = <0x2>;
> +
> +			uart_pins: uart_pins {
> +				pins = "gpio44", "gpio45";
> +				function = "blsp2_uart";
> +				drive-strength = <8>;
> +				bias-pull-down;
> +			};
> +		};
> +	};
> +
> +	psci: psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	cpus: cpus {
> +		#address-cells = <0x1>;
> +		#size-cells = <0x0>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			enable-method = "psci";
> +			reg = <0x1>;
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU2: cpu@2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			enable-method = "psci";
> +			reg = <0x2>;
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		CPU3: cpu@3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53";
> +			enable-method = "psci";
> +			reg = <0x3>;
> +			next-level-cache = <&L2_0>;
> +		};
> +
> +		L2_0: l2-cache {
> +			compatible = "cache";
> +			cache-level = <0x2>;
> +		};
> +	};
> +
> +	pmuv8: pmu {
> +		compatible = "arm,armv8-pmuv3";
> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) |
> +					 IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	clocks {
> +		sleep_clk: sleep_clk {

Don't use _ in the node names.

> +			compatible = "fixed-clock";
> +			clock-frequency = <32000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		xo: xo {
> +			compatible = "fixed-clock";
> +			clock-frequency = <24000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		bias_pll_cc_clk {

Please give this a label and reference it from the node that uses it
(regardless of the implementation matching by clock name).

> +			compatible = "fixed-clock";
> +			clock-frequency = <300000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		bias_pll_nss_noc_clk {
> +			compatible = "fixed-clock";
> +			clock-frequency = <416500000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		usb3phy_0_cc_pipe_clk {

This should come from the PHY.

> +			compatible = "fixed-clock";
> +			clock-frequency = <125000000>;
> +			#clock-cells = <0>;
> +		};

Regards,
Bjorn

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-05 17:15 [PATCH 0/6] Add minimal boot support for IPQ6018 Sricharan R
2019-06-05 17:15 ` [PATCH 1/6] pinctrl: qcom: Add ipq6018 pinctrl driver Sricharan R
2019-06-08  3:26   ` Bjorn Andersson
2019-06-10 11:00     ` Sricharan R
2019-06-05 17:15 ` [PATCH 2/6] dt-bindings: qcom: Add ipq6018 bindings Sricharan R
2019-06-08  3:27   ` Bjorn Andersson
2019-06-10 11:01     ` Sricharan R
2019-06-19 14:54   ` Rob Herring
2019-06-05 17:15 ` [PATCH 3/6] clk: qcom: Add DT bindings for ipq6018 gcc clock controller Sricharan R
2019-06-08  3:33   ` Bjorn Andersson
2019-06-05 17:16 ` [PATCH 5/6] arm64: dts: Add ipq6018 SoC and CP01 board support Sricharan R
2019-06-05 17:26   ` Marc Zyngier
2019-06-08  2:44     ` Sricharan R
2019-06-05 20:41   ` Christian Lamparter
2019-06-10 10:09     ` Sricharan R
2019-06-10 12:15       ` Christian Lamparter
2019-06-12  9:48         ` Sricharan R
2019-06-14 20:41           ` Christian Lamparter
2019-06-19 14:42             ` Sricharan R
2019-06-20 15:32               ` Christian Lamparter
2019-06-24 12:08                 ` Sricharan R
2019-06-08  3:48   ` Bjorn Andersson [this message]
2019-06-10 15:45     ` Sricharan R
2019-06-10 16:48       ` Stephen Boyd
2019-06-05 17:16 ` [PATCH 6/6] arm64: defconfig: Enable qcom ipq6018 clock and pinctrl Sricharan R
2019-06-08  3:32   ` Bjorn Andersson
2019-06-05 17:26 ` [PATCH 0/6] Add minimal boot support for IPQ6018 Sricharan R
2019-06-07 23:08 ` Linus Walleij
     [not found] ` <1559754961-26783-5-git-send-email-sricharan@codeaurora.org>
2019-06-08  3:32   ` [PATCH 4/6] clk: qcom: Add ipq6018 Global Clock Controller support Bjorn Andersson
2019-06-10 11:47     ` Sricharan R
2019-06-10 16:58       ` Bjorn Andersson
2019-06-05 17:28 [PATCH 0/6] Add minimal boot support for IPQ6018 Sricharan R
2019-06-05 17:28 ` [PATCH 5/6] arm64: dts: Add ipq6018 SoC and CP01 board support Sricharan R
2019-06-05 17:34   ` Sudeep Holla
2019-06-08  2:44     ` Sricharan R

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190608034835.GH24059@builder \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@codeaurora.org \
    --cc=sricharan@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox