Re: [PATCH] arm64: dts: sdm630 SoC and Sony Pioneer (Xperia XA2) support

[Craig]

On 24 September 2018 20:19:29 BST, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>On Sat 11 Aug 09:25 PDT 2018, Craig Tatlor wrote:
>
>> Initial device tree support for Qualcomm SDM630 SoC and
>> Sony Pioneer (Xperia XA2).
>> 
>> SDM630 is based off of the SDM660 soc and all SDM660 specific drivers
>are
>> compatible with it. SDM660 is also based off of MSM8998 so it uses
>some
>> of its drivers aswell.
>
>Consider adding both sdm630 and sdm660 compatibles to the bindings and
>drivers and use the right one in the dts, in case we find details that
>differs in the future.

This includes pinctrl and GCC?
>
>> 
>> The device tree is based on the CAF 4.4 kernel tree.
>> 
>> The device can be booted into the initrd with a shell over UART.
>> 
>> Signed-off-by: Craig Tatlor <ctatlor97@xxxxxxxxx>
>[..]
>> diff --git a/arch/arm64/boot/dts/qcom/sdm630-pins.dtsi
>b/arch/arm64/boot/dts/qcom/sdm630-pins.dtsi
>> new file mode 100644
>> index 000000000000..78b79c1076f1
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm630-pins.dtsi
>> @@ -0,0 +1,17 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, Craig Tatlor. */
>> +
>> +&tlmm {
>> +	blsp1_uart1_default: blsp1_uart1_default {
>> +		pinmux {
>> +			pins = "gpio0", "gpio1", "gpio2", "gpio3";
>> +			function = "gpio";
>
>Please put these in the sdm630.dtsi directly, rather than spreading the
>pins out in a separate file.
>
Okay, just followed what 8996 did
>> +		};
>> +
>> +		pinconf {
>> +			pins = "gpio0", "gpio1", "gpio2", "gpio3";
>> +			drive-strength = <2>;
>> +			bias-disable;
>
>Please extend &blsp1_uart1_default in the pioneer dtsi with these
>"electrical properties".
Are you meaning to put this in the pioneer DTS or just drive strength and bias?
>
>> +		};
>> +	};
>> +};
>[..]
>> diff --git a/arch/arm64/boot/dts/qcom/sdm630-pioneer.dtsi
>b/arch/arm64/boot/dts/qcom/sdm630-pioneer.dtsi
>> new file mode 100644
>> index 000000000000..512792c23369
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm630-pioneer.dtsi
>> @@ -0,0 +1,22 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, Craig Tatlor. */
>> +
>> +#include "sdm630.dtsi"
>> +
>> +/ {
>> +	aliases {
>> +		serial0 = &blsp1_uart1;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +};
>> +
>> +&soc {
>> +	serial@c170000 {
>
>Please reference this by &blsp1_uart1, rather than duplicating the
>hierarchy.
Okay, should the same still apply once more stuff is in soc? Or should I use references for them?
>
>> +		status = "okay";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&blsp1_uart1_default>;
>> +	};
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi
>b/arch/arm64/boot/dts/qcom/sdm630.dtsi
>> new file mode 100644
>> index 000000000000..8a544979b7c0
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
>> @@ -0,0 +1,383 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, Craig Tatlor. */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-sdm660.h>
>> +
>> +/ {
>> +	model = "Qualcomm Technologies, Inc. SDM630";
>
>We expect the board to always override this, so no need to specify it
>here.
Yup
>> +
>> +	interrupt-parent = <&intc>;
>> +
>> +	qcom,msm-id = <318 0x0>;
>> +
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	chosen { };
>> +
>> +	memory {
>> +		device_type = "memory";
>> +		/* We expect the bootloader to fill in the reg */
>> +		reg = <0 0 0 0>;
>> +	};
>> +
>> +
>
>Extra empty line.
Okay
>> +	cpus {
>[..]
>> +
>> +	timer {
>
>Please sort these nodes by name, except for "soc" which is convenient
>to have last.
Okay
>
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 1 IRQ_TYPE_LEVEL_LOW>,
>> +			     <GIC_PPI 2 IRQ_TYPE_LEVEL_LOW>,
>> +			     <GIC_PPI 3 IRQ_TYPE_LEVEL_LOW>,
>> +			     <GIC_PPI 0 IRQ_TYPE_LEVEL_LOW>;
>> +	};
>[..]
>> +	firmware {
>> +		scm {
>> +			compatible = "qcom,scm-sdm660";
>> +		};
>> +	};
>> +
>> +
>
>Extra empty line.
Okay
>
>> +	rpm-glink {
>> +		compatible = "qcom,glink-rpm";
>> +
>> +		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
>> +
>> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
>> +
>> +		mboxes = <&apcs_glb 0>;
>
>Remove a few of these extra empty lines and add the rpm_requests
>channel
>here while you're at it:
>
Sure
>		rpm_requests: glink-channel {
>                        compatible = "qcom,rpm-sdm660";
>                        qcom,glink-channels = "rpm_requests";
>                };
>
>> +	};
>> +
>> +	soc: soc {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0 0 0 0xffffffff>;
>> +		compatible = "simple-bus";
>> +
>> +		intc: interrupt-controller@17a00000 {
>
>Please sort these nodes by base address.
>
Will do
>> +			compatible = "arm,gic-v3";
>> +			reg = <0x17a00000 0x10000>,
>> +			      <0x17b00000 0x100000>;
>> +			#interrupt-cells = <3>;
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges;
>> +			interrupt-controller;
>> +			#redistributor-regions = <1>;
>> +			redistributor-stride = <0x0 0x20000>;
>> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		gcc: clock-controller@100000 {
>> +			compatible = "qcom,gcc-sdm660";
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +			#power-domain-cells = <1>;
>> +			reg = <0x100000 0x94000>;
>
>Please 0-pad addresses in "reg", makes it easier to sort them as well
>(but keep the @address after the node name unpadded)
Sure, how much should I pad up to?
>
>> +		};
>> +
>[..]
>> +		rpm_msg_ram: memory@778000 {
>> +			compatible = "qcom,rpm-msg-ram";
>> +			reg = <0x778000 0x7000>;
>> +		};
>> +
>> +		apcs_glb: mailbox@17911000 {
>> +			compatible = "qcom,msm8998-apcs-hmss-global";
>
>Please update the apcs ipc driver with a sdm660 compatible and update
>this.
Right
>
>> +			reg = <0x17911000 0x1000>;
>> +
>> +			#mbox-cells = <1>;
>> +		};
>
>Regards,
>Bjorn