Re: [PATCH 2/2] ARM: dts: qcom: Add SDX55 Modem and MTP board support

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 24 Nov 08:00 CST 2020, Manivannan Sadhasivam wrote:

> Add basic devicetree support for SDX55 Modem and MTP board from

Again, let's call is "SDX55 platform".

> Qualcomm. The SDX55 modem features an ARM Cortex A7 CPU which forms the
> Application Processor Sub System (APSS) along with standard Qualcomm
> peripherals like GCC, TLMM, BLSP, QPIC, BAM etc... Along with these,
> there exists the networking parts such as IPA, MHI, PCIE-EP, EMAC
> etc..

And here you can mention that there's a modem.

> 
> Currently, this basic devicetree support includes GCC, RPMh clock, INTC
> and Debug UART.
> 
> Co-developed-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm/boot/dts/Makefile           |   3 +-
>  arch/arm/boot/dts/qcom-sdx55-mtp.dts |  27 ++++
>  arch/arm/boot/dts/qcom-sdx55.dtsi    | 205 +++++++++++++++++++++++++++
>  3 files changed, 234 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/qcom-sdx55-mtp.dts
>  create mode 100644 arch/arm/boot/dts/qcom-sdx55.dtsi
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index ce66ffd5a1bb..1505c6cdc5ca 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -917,7 +917,8 @@ dtb-$(CONFIG_ARCH_QCOM) += \
>  	qcom-msm8974-sony-xperia-amami.dtb \
>  	qcom-msm8974-sony-xperia-castor.dtb \
>  	qcom-msm8974-sony-xperia-honami.dtb \
> -	qcom-mdm9615-wp8548-mangoh-green.dtb
> +	qcom-mdm9615-wp8548-mangoh-green.dtb \
> +	qcom-sdx55-mtp.dtb
>  dtb-$(CONFIG_ARCH_RDA) += \
>  	rda8810pl-orangepi-2g-iot.dtb \
>  	rda8810pl-orangepi-i96.dtb
> diff --git a/arch/arm/boot/dts/qcom-sdx55-mtp.dts b/arch/arm/boot/dts/qcom-sdx55-mtp.dts
> new file mode 100644
> index 000000000000..8c39db4ae792
> --- /dev/null
> +++ b/arch/arm/boot/dts/qcom-sdx55-mtp.dts
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2020, Linaro Ltd.
> + */
> +
> +/dts-v1/;
> +
> +#include "qcom-sdx55.dtsi"
> +
> +/ {
> +	model = "Qualcomm Technologies, Inc. SDX55 MTP";
> +	compatible = "qcom,sdx55-mtp", "qcom,sdx55", "qcom,mtp";

Do we need "qcom,mtp"?

> +	qcom,board-id = <0x5010008 0x0>;
> +
> +	aliases {
> +		serial0 = &blsp1_uart3;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +};
> +
> +&blsp1_uart3 {
> +	status = "ok";
> +};
> diff --git a/arch/arm/boot/dts/qcom-sdx55.dtsi b/arch/arm/boot/dts/qcom-sdx55.dtsi
> new file mode 100644
> index 000000000000..9e0b964e4c57
> --- /dev/null
> +++ b/arch/arm/boot/dts/qcom-sdx55.dtsi
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * SDX55 SoC device tree source
> + *
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2020, Linaro Ltd.
> + */
> +
> +/dts-v1/;

We seem to mix a little bit in the arm/qcom dts files, but the general
pattern is to put this only in the dts files.

> +
> +#include <dt-bindings/clock/qcom,gcc-sdx55.h>
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	model = "Qualcomm Technologies, Inc. SDX55";
> +	compatible = "qcom,sdx55";

model and compatible should always be specified in the including .dts
(as you do), so I would prefer if you omit these.

> +	qcom,msm-id = <357 0x10000>, <368 0x10000>, <418 0x10000>;
> +	interrupt-parent = <&intc>;
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0 0>;
> +	};
> +
> +	clocks {
> +		xo_board: xo-board {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <38400000>;
> +			clock-output-names = "xo_board";
> +		};
> +
> +		sleep_clk: sleep-clk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <32000>;
> +		};
> +
> +		pll_test_clk: pll-test-clk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <400000000>;
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +		};
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	soc: soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +		compatible = "simple-bus";
> +
> +		timer {

If timer doesn't have a reg, it should go outside soc {}

Regards,
Bjorn