Re: [PATCH 7/7] arm64: dts: qcom: Add support for SONY Xperia X Performance / XZ / XZs (msm8996, Tone platform)

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

On Tue 25 May 15:02 CDT 2021, Konrad Dybcio wrote:

> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> Add support for following boards:
> 
> - Xperia X Performance (dora)
> - Xperia XZ (kagura)
> - Xperia XZs (keyaki)
> 
> They are all based on the SONY Tone platform and feature largely similar hardware
> with the most obvious differences being lack of USB-C and ToF sensor on Dora and
> different camera sensor on Keyaki.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> ---
>  arch/arm64/boot/dts/qcom/Makefile             |   6 +
>  .../msm8996-pmi8996-sony-xperia-tone-dora.dts |  11 +
>  ...sm8996-pmi8996-sony-xperia-tone-kagura.dts |  11 +
>  ...sm8996-pmi8996-sony-xperia-tone-keyaki.dts |  11 +
>  .../qcom/msm8996-sony-xperia-tone-dora.dts    |  27 +
>  .../qcom/msm8996-sony-xperia-tone-kagura.dts  |  15 +
>  .../qcom/msm8996-sony-xperia-tone-keyaki.dts  |  26 +
>  .../dts/qcom/msm8996-sony-xperia-tone.dtsi    | 980 ++++++++++++++++++
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |  12 +-
>  9 files changed, 1093 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-pmi8996-sony-xperia-tone-dora.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-pmi8996-sony-xperia-tone-kagura.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-pmi8996-sony-xperia-tone-keyaki.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone-dora.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone-kagura.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone-keyaki.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index ca4a7819d2c4..d079dc33d833 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -25,6 +25,12 @@ dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-sony-xperia-kitakami-satsuki.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-sony-xperia-kitakami-sumire.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-sony-xperia-kitakami-suzuran.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-mtp.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-sony-xperia-tone-dora.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-sony-xperia-tone-kagura.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-sony-xperia-tone-keyaki.dtb

's' > 'p', please keep them sorted alphabetically.

That said, perhaps it would look better to move the "pmi8996" part later
in the name?

> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-pmi8996-sony-xperia-tone-dora.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-pmi8996-sony-xperia-tone-kagura.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-pmi8996-sony-xperia-tone-keyaki.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-asus-novago-tp370ql.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-hp-envy-x2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-lenovo-miix-630.dtb
> diff --git a/arch/arm64/boot/dts/qcom/msm8996-pmi8996-sony-xperia-tone-dora.dts b/arch/arm64/boot/dts/qcom/msm8996-pmi8996-sony-xperia-tone-dora.dts
> new file mode 100644
> index 000000000000..b57ea0824ea5
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8996-pmi8996-sony-xperia-tone-dora.dts
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0

BSD license in all the files please.

> +/*
> + * Copyright (c) 2021, Konrad Dybcio <konrad.dybcio@somainline.org>
> + */
> +
> +#include "msm8996-sony-xperia-tone-dora.dts"
> +#include "pmi8996.dtsi"
> +
> +/ {
> +	model = "Sony Xperia X Performance (PMI8996)";
> +};
> \ No newline at end of file
[..]
> diff --git a/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi b/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
> new file mode 100644
> index 000000000000..4644d5f9d1a6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
> @@ -0,0 +1,980 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021, AngeloGioacchino Del Regno
> + *                     <angelogioacchino.delregno@somainline.org>
> + * Copyright (c) 2021, Konrad Dybcio <konrad.dybcio@somainline.org>
> + */
> +
> +#include "msm8996.dtsi"
> +#include "pm8994.dtsi"
> +#include "pmi8994.dtsi"
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/input/gpio-keys.h>

This seems to be unused for now.

> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-mpp.h>
> +
> +/delete-node/ &hdmi;
> +/delete-node/ &hdmi_phy;
> +/delete-node/ &mdp5_intf3_out;
> +/delete-node/ &slpi_region;
> +/delete-node/ &venus_region;
> +/delete-node/ &zap_shader_region;
> +
> +/ {
> +	qcom,msm-id = <246 0x30001>; /* MSM8996 V3.1 (Final) */
> +	qcom,pmic-id = <0x20009 0x2000a 0 0>; /* PM8994 + PMI8994 */
> +	qcom,board-id = <8 0>;
> +
> +	chosen {
> +		/*
> +		 * Due to an unknown-for-a-few-years regression,
> +		 * SDHCI only works on MSM8996 in PIO (lame) mode.
> +		 */
> +		bootargs = "sdhci.debug_quirks=0x40 sdhci.debug_quirks2=0x4 maxcpus=2";

What's up with maxcpus=2? Is this simply because the last 2 are really
really slow?

> +	};
> +
> +	reserved-memory {
> +		ramoops@a7f00000 {
> +			compatible = "ramoops";
> +			reg = <0 0xa7f00000 0 0x100000>;
> +			record-size = <0x20000>;
> +			console-size = <0x40000>;
> +			ftrace-size = <0x20000>;
> +			pmsg-size = <0x20000>;
> +			ecc-size = <16>;
> +		};
> +
> +		cont_splash_mem: memory@83401000 {
> +			reg = <0 0x83401000 0 0x23ff000>;
> +			no-map;
> +		};
> +
> +		zap_shader_region: gpu@90400000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0x0 0x90400000 0x0 0x2000>;
> +			no-map;
> +		};
> +
> +		slpi_region: memory@90500000 {
> +			reg = <0 0x90500000 0 0xa00000>;
> +			no-map;
> +		};
> +
> +		venus_region: memory@90f00000 {
> +			reg = <0 0x90f00000 0 0x500000>;
> +			no-map;
> +		};
> +	};
> +
> +	panel_tvdd: tvdd-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "panel_tvdd";
> +		gpio = <&tlmm 50 GPIO_ACTIVE_HIGH>;
> +		pinctrl-0 = <&tp_vddio_en>;
> +		pinctrl-names = "default";
> +	};
> +
> +	usb3_id: usb3-id {
> +		compatible = "linux,extcon-usb-gpio";
> +		id-gpio = <&tlmm 25 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_detect>;
> +	};
> +
> +	vph_pwr: vph-pwr-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <3700000>;
> +		regulator-max-microvolt = <3700000>;
> +		regulator-name = "vph_pwr";
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};
> +
> +	wlan_en: wlan-en-1-8v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "wlan-en-regulator";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wl_reg_on>;
> +
> +		/* WLAN card specific delay */
> +		startup-delay-us = <70000>;
> +		enable-active-high;
> +	};
> +};
> +
> +&blsp1_i2c3 {
> +	status = "okay";
> +	clock-frequency = <355000>;
> +
> +	tof_sensor: vl53l0x@29 {
> +		compatible = "st,vl53l0x";
> +		reg = <0x29>;
> +	};
> +};
> +
> +&blsp1_uart2 {
> +	status = "okay";
> +};
> +
> +&blsp2_i2c5 {
> +	status = "okay";
> +	clock-frequency = <355000>;
> +
> +	/* FUSB301 USB-C controller */
> +};
> +
> +&blsp2_i2c6 {
> +	status = "okay";
> +	clock-frequency = <355000>;
> +
> +	synaptics@2c {
> +		compatible = "syna,rmi4-i2c";
> +		reg = <0x2c>;
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <125 IRQ_TYPE_EDGE_FALLING>;
> +		vdd-supply = <&panel_tvdd>;
> +
> +		syna,reset-delay-ms = <220>;
> +		syna,startup-delay-ms = <220>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		rmi4-f01@1 {
> +			reg = <0x1>;
> +			syna,nosleep-mode = <1>;
> +		};
> +
> +		rmi4-f11@11 {
> +			reg = <0x11>;
> +			syna,sensor-type = <1>;
> +		};
> +	};
> +};
> +
> +&blsp2_uart2 {
> +	status = "okay";
> +};
> +
> +&camera0_mclk {
> +	drive-strength = <2>;
> +	output-low;
> +};
> +
> +&camera0_pwdn {
> +	drive-strength = <2>;
> +	output-low;
> +};
> +
> +&camera0_rst {
> +	pins = "gpio30";
> +	drive-strength = <2>;
> +	output-low;
> +};
> +
> +&camera2_mclk {
> +	drive-strength = <2>;
> +	output-low;
> +};
> +
> +&camera2_rst {
> +	drive-strength = <2>;
> +	output-low;
> +};
> +
> +&CPU0 {
> +	cpu-supply = <&pmi8994_s11>;

Isn't this the supply to the CPU-subsystem-internal LDO that actually
feeds the CPU? Is there a benefit to describing this here?

> +};
> +
> +&CPU1 {
> +	cpu-supply = <&pmi8994_s11>;
> +};
> +
> +&CPU2 {
> +	cpu-supply = <&pmi8994_s11>;
> +};
> +
> +&CPU3 {
> +	cpu-supply = <&pmi8994_s11>;
> +};
> +
> +&hsusb_phy1 {
> +	status = "okay";
> +
> +	vdda-pll-supply = <&pm8994_l12>;
> +	vdda-phy-dpdm-supply = <&pm8994_l24>;
> +};
> +
> +&mmcc {
> +	vdd-gfx-supply = <&vdd_gfx>;
> +};
> +
> +&pcie0 {
> +	status = "okay";
> +	perst-gpio = <&tlmm 35 GPIO_ACTIVE_LOW>;
> +	wake-gpio = <&tlmm 37 GPIO_ACTIVE_HIGH>;
> +	vddpe-3v3-supply = <&wlan_en>;
> +	vdda-supply = <&pm8994_l28>;
> +};
> +
> +&pcie_phy {
> +	status = "okay";
> +
> +	vdda-phy-supply = <&pm8994_l28>;
> +	vdda-pll-supply = <&pm8994_l12>;
> +};
> +
> +&pm8994_gpios {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pm8994_gpio_1 &pm8994_vol_down_n &pm8994_vol_up_n
> +		     &pm8994_cam_snap_n &pm8994_cam_focus_n &pm8994_gpio_6
> +		     &pm8994_nfc_dload &pm8994_gpio_8 &pm8994_gpio_9
> +		     &pm8994_gpio_nfc_clk &pm8994_gpio_11 &pm8994_gpio_12
> +		     &pm8994_ear_en &pm8994_gpio_14 &pm8994_pm_divclk1
> +		     &pm8994_pmi_clk &pm8994_gpio_17 &pm8994_rome_sleep
> +		     &pm8994_gpio_19 &pm8994_gpio_22>;

Shouldn't several of these reference be done from the relevant nodes?

For the ones that isn't, and that you're not going to ever change I
think it would look better to have a single:

pm8994_gpios_defaults: default-state {
	nc {
		nc pins...
	};

	vol-up {
		...
	};

	...
};

> +
> +	gpio-line-names =
> +		"NC",
> +		"VOL_DOWN_N",
> +		"VOL_UP_N",
> +		"SNAPSHOT_N",
> +		"FOCUS_N",
> +		"NC",
> +		"NFC_VEN",
> +		"NC",
> +		"NC",
> +		"NC",
> +		"NC",
> +		"NC",
> +		"EAR_EN",
> +		"NC",
> +		"PM_DIVCLK1",
> +		"PMI_CLK",
> +		"NC",
> +		"WL_SLEEP_CLK",
> +		"NC",
> +		"PMIC_SPON",
> +		"UIM_BATT_ALARM",
> +		"PMK_SLEEP_CLK";
> +
> +	pm8994_gpio_1: pm-gpio1-nc {
> +		pins = "gpio1";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +		drive-push-pull;
> +		bias-high-impedance;
> +	};
> +
> +	pm8994_vol_down_n: vol-down-n {
> +		pins = "gpio2";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +		drive-push-pull;
> +		input-enable;
> +		bias-pull-up;
> +		qcom,drive-strength = <PMIC_GPIO_STRENGTH_NO>;
> +		power-source = <PM8994_GPIO_S4>;
> +	};
> +
[..]
> +/*
> + * For reasons that are currently unknown
> + * (but probably related to fusb301), USB
> + * takes about 6 minutes to wake up (nothing
> + * interesting in kernel logs), but then it
> + * works as it should.

This is funny (but please make it ~80 chars wide).

Regards,
Bjorn

> + */
> +&usb3 {
> +	status = "okay";
> +	qcom,select-utmi-as-pipe-clk;
> +};
> +
> +&usb3_dwc3 {
> +	extcon = <&usb3_id>;
> +	dr_mode = "peripheral";
> +	phys = <&hsusb_phy1>;
> +	phy-names = "usb2-phy";
> +	snps,hird-threshold = /bits/ 8 <0>;
> +};