Re: [PATCH 5/5] arm64: dts: qcom: sm8350-lemonade(p): new devices

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 10/16/23 14:47, Nia Espera wrote:
> Device tree files for OnePlus 9 and 9 Pro. Details of supported features
> mentioned in the cover letter for this patch series, but for
> accessibility also repeated here:
> 
> - USB OTG
> - UFS
> - Framebuffer display
> - Touchscreen (for lemonade)
> - Power & volume down keys
> - Battery reading
> - Modem, IPA, and remoteproc bringup
> 
> Steps to get booting:
> 
> - Wipe dtbo partition
> - Flash vbmeta with disabled verity bit
> - Flash kernel and initfs to boot partition with CLI args pd_ignore_unused
> and clk_ignore_unused
> - Flash rootfs to some other partition (probably super or userdata)
> 
> Signed-off-by: Nia Espera <nespera@igalia.com>
> ---
>   arch/arm64/boot/dts/qcom/Makefile                  |    2 +
>   .../arm64/boot/dts/qcom/sm8350-oneplus-common.dtsi | 1247 ++++++++++++++++++++
>   .../boot/dts/qcom/sm8350-oneplus-lemonade.dts      |   82 ++
>   .../boot/dts/qcom/sm8350-oneplus-lemonadep.dts     |   37 +
>   4 files changed, 1368 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 2cca20563a1d..369ad4721b29 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -211,6 +211,8 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sm8250-xiaomi-elish-csot.dtb
>   dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-hdk.dtb
>   dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-microsoft-surface-duo2.dtb
>   dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-mtp.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-oneplus-lemonade.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-oneplus-lemonadep.dtb
>   dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-sony-xperia-sagami-pdx214.dtb
>   dtb-$(CONFIG_ARCH_QCOM)	+= sm8350-sony-xperia-sagami-pdx215.dtb
>   dtb-$(CONFIG_ARCH_QCOM)	+= sm8450-hdk.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sm8350-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sm8350-oneplus-common.dtsi
> new file mode 100644
> index 000000000000..2f6768f35259
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm8350-oneplus-common.dtsi
> @@ -0,0 +1,1247 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2023 Caleb Connolly <caleb.connolly@linaro.org>
> + *
> + * Copyright (c) 2023 Igalia S.L.
> + * Authors:
> + *	Nia Espera <nespera@igalia.com>
> + */
> +
> +#include <dt-bindings/iio/qcom,spmi-adc7-pm8350.h>
> +#include <dt-bindings/iio/qcom,spmi-adc7-pm8350b.h>
> +#define SMB139x_1_SID 0x0b
> +#define SMB139x_2_SID 0x0c
> +#include <dt-bindings/iio/qcom,spmi-adc7-smb139x.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +#include "sm8350.dtsi"
> +#include "pm8350.dtsi"
> +#include "pm8350b.dtsi"
> +#include "pm8350c.dtsi"
> +#include "pmk8350.dtsi"
> +#include "pmr735a.dtsi"
> +#include "pmr735b.dtsi"
> +
> +/ {
> +	/* As with the Sony devices, msm-id and board-id aren't needed here */
This became "common knowledge" since then, we can omit the comment now.

> +	chassis-type = "handset";
> +	interrupt-parent = <&intc>;
> +
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		framebuffer: framebuffer@e4d00000 {
> +			compatible = "simple-framebuffer";
> +			reg = <0 0xe4d00000 0 0x2400000>;
> +			width = <1080>;
> +			height = <2412>;
> +			stride = <(1080 * 4)>;
> +			format = "a8r8g8b8";
> +			/*
> +			 * That's (going to be) a lot of clocks, but it's
> +			 * necessary due to unused clk cleanup & no panel
> +			 * driver yet.
> +			 */
> +			clocks = <&gcc GCC_DISP_HF_AXI_CLK>,
> +				 <&gcc GCC_DISP_SF_AXI_CLK>;
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vol_down_n>;
> +
> +		key-vol-up {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			gpios = <&pmk8350_gpios 6 GPIO_ACTIVE_LOW>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};
> +	};
> +
> +	bat: battery {
> +		compatible = "simple-battery";
> +		device-chemistry = "lithium-ion";
> +		voltage-min-design-microvolt = <3200000>;
> +		energy-full-design-microwatt-hours = <15840000>;
> +		charge-full-design-microamp-hours = <2225000>;
> +	};
> +
> +	vph_pwr: vph-pwr-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vph_pwr";
> +		regulator-min-microvolt = <3700000>;
> +		regulator-max-microvolt = <3700000>;
> +
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};
> +
> +	display_panel_avdd: display_regulator@1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "display_panel_avdd";
> +		regulator-min-microvolt = <5500000>;
> +		regulator-max-microvolt = <5500000>;
> +		regulator-enable-ramp-delay = <233>;
> +
> +		enable-active-high;
> +		regulator-boot-on;
> +	};
> +
> +	/*
> +	 * Hack; OP9 bootloader specifically checks that the timer node has
> +	 * this label.
> +	 */
> +	arch_timer: timer {};
> +};
> +
> +&reserved_memory {
> +	/* EFI splash screen */
> +	memory@e1000000 {
framebuffer@e10...

> +		reg = <0 0xe4d00000 0 0x02400000>;
> +		no-map;
> +		label = "cont_splash_region";
This label is unnecessary (perhaps even unused?)

> +	};
> +
> +	ramoops: ramoops@E9700000 {
> +		compatible = "ramoops";
> +		reg = <0 0xe9700000 0 0x05b8000>;
> +		record-size =	<0x40000>;
> +		console-size =	<0x40000>;
> +		ftrace-size =	<0x200000>;
> +		pmsg-size =	<0x200000>;
> +		devinfo-size =	<0x08000>;
> +		dumpinfo-size =	<0x08000>;
> +		rsv01info-size=	<0x08000>;
> +		rsv02info-size=	<0x08000>;
> +		rsv03info-size=	<0x08000>;
> +		rsv04info-size=	<0x08000>;
> +		rsv05info-size=	<0x08000>;
> +		ecc-size=	<0x0>;
Please use a single space before and after the '=' sign.
Please drop the unused-and-undocumented properties (make CHECK_DTBS=1 
qcom/sm8350-oneplus-lemonade.dtb)

> +	};
> +
> +	/* bootloader log buffer */
> +	memory@9fff7000 {
bootloader-log@, drop comment

> +		reg = <0x00 0x9fff7000 0x00 0x8000>;
please be consistent with the usage of "different zeroes"

> +	};
> +
> +	/* unknown; "param_mem" downstream */
> +	memory@ea700000 {
reserved@, drop comment, probably it's for the kernel command line 
parameters in some hacky setup, but if so, it would be freed the moment
Linux is jumped to.

[...]

> +
> +	/* regulators-2 unused for now */
Any good reason?

> +
> +	regulators-3 {
> +		compatible = "qcom,pmr735a-rpmh-regulators";
> +		qcom,pmic-id = "e";
> +
> +		vdd-s1-supply = <&vph_pwr>;
> +		vdd-s2-supply = <&vph_pwr>;
> +		vdd-s3-supply = <&vph_pwr>;
> +
> +		vdd-l1-l2-supply = <&pmr735a_s2>;
> +		vdd-l3-supply = <&pmr735a_s1>;
> +		vdd-l4-supply = <&pm8350c_s1>;
> +		vdd-l5-l6-supply = <&pm8350c_s1>;
> +		vdd-l7-bob-supply = <&vreg_bob>;
> +
> +		pmr735a_s1: smps1 {
> +			regulator-name = "pmr735a_s1";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1280000>;
> +		};
> +
> +		pmr735a_s2: smps2 {
> +			regulator-name = "pmr735a_s2";
> +			regulator-min-microvolt = <500000>;
> +			regulator-max-microvolt = <976000>;
> +		};
> +
> +		pmr735a_s3: smps3 {
> +			regulator-name = "pmr735a_s3";
> +			regulator-min-microvolt = <2208000>;
> +			regulator-max-microvolt = <2352000>;
> +		};
> +
> +		pmr735a_l1: ldo1 {
> +			regulator-name = "pmr735a_l1";
> +			regulator-min-microvolt = <912000>;
> +			regulator-max-microvolt = <912000>;
> +		};
> +
> +		pmr735a_l2: ldo2 {
> +			regulator-name = "pmr735a_l2";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +		};
> +
> +		pmr735a_l3: ldo3 {
> +			regulator-name = "pmr735a_l3";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +		};
> +
> +		pmr735a_l4: ldo4 {
> +			regulator-name = "pmr735a_l4";
> +			regulator-min-microvolt = <1776000>;
> +			regulator-max-microvolt = <1872000>;
> +		};
> +
> +		pmr735a_l5: ldo5 {
> +			regulator-name = "pmr735a_l5";
> +			regulator-min-microvolt = <800000>;
> +			regulator-max-microvolt = <800000>;
> +		};
> +
> +		pmr735a_l6: ldo6 {
> +			regulator-name = "pmr735a_l6";
> +			regulator-min-microvolt = <480000>;
> +			regulator-max-microvolt = <904000>;
> +		};
> +
> +		pmr735a_l7: ldo7 {
> +			regulator-name = "pmr735a_l7";
> +			regulator-min-microvolt = <2800000>;
> +			regulator-max-microvolt = <2800000>;
> +		};
> +	};
> +};
> +
> +&adsp {
> +	firmware-name = "qcom/OnePlus/lemonade/adsp.mbn";
> +	status = "okay";
> +};
> +
> +&cdsp {
> +	firmware-name = "qcom/OnePlus/lemonade/cdsp.mbn";
> +	status = "okay";
> +};
> +
> +&slpi {
> +	firmware-name = "qcom/OnePlus/lemonade/slpi.mbn";
> +	status = "okay";
> +};
> +
> +&ipa {
> +	qcom,gsi-loader = "self";
> +	memory-region = <&pil_ipa_fw_mem>;
> +	firmware-name = "qcom/OnePlus/lemonade/ipa_fws.mbn";
> +	status = "okay";
> +};
> +
> +&mpss {
> +	firmware-name = "qcom/OnePlus/lemonade/modem.mbn",
> +			"qcom/OnePlus/lemonade/mcfg_hw.mbn";
> +
> +	status = "okay";
> +};
> +
> +&i2c4 {
> +	clock-frequency = <400000>;
> +	status = "okay";
> +
> +	/* Touchscreens: Syna TCM oncell or Samsung s6sy761 */
Synaptics with a samsung panel? Are you sure it's not a reference device 
dt leftover?

[...]

> +&pm8350_gpios {
> +	usb2_vbus_boost_default: usb2_vbus_boost_default {
No underscores in node names, use '-', all throughout the file.


> +		pins = "gpio8";
> +		function = "normal";
> +		output-low;
> +		power-source = <0x00>; > +		phandle = <0x5e1>;
Please drop the decompiler-generated phandle= properties

[...]

> +	sde_dsi_active: sde-dsi-active {
> +		pins = "gpio24";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		bias-disable = <0>;
This is a boolean property, should be "bias-disable;"

[...]

> +&usb_1 {
> +	/* Bug in interconnect driver breaks USB */
> +	/delete-property/ interconnects;
> +	/delete-property/ interconnect-names;
Can you elaborate?

> +
> +	/*
> +	 * USB3 is not tested (though it is enabled downstream) so limit to
> +	 * high-speed for now.
> +	 */
> +	qcom,select-utmi-as-pipe-clk;
> +
> +	status = "okay";
> +};
> +
> +&usb_1_dwc3 {
> +	/* Mode switching is untested */
> +	dr_mode = "peripheral";
> +	maximum-speed = "high-speed";
> +	phys = <&usb_1_hsphy>;
> +	phy-names = "usb2-phy";
> +};
> +
> +&usb_1_hsphy {
> +	vdda-pll-supply = <&pm8350_l5>;
> +	vdda18-supply = <&pm8350c_l1>;
> +	vdda33-supply = <&pm8350_l2>;
> +
> +	status = "okay";
> +};
> +
> +/* Enabling this is necessary only for displayport */
If DP is wired up, I would strongly guess that USB3 is too.

> +&usb_1_qmpphy {
> +	vdda-phy-supply = <&pm8350_l6>;
> +	vdda-pll-supply = <&pm8350_l1>;
> +
> +	status = "okay";
> +};
> +
> +&i2c2 {
Please sort the node references alphabetically

> +	clock-frequency = <100000>;
> +	status = "okay";
> +
> +	bq27541: fuel-gauge@55 {
> +		compatible = "ti,bq27541";
> +		reg = <0x55>;
> +		monitored-battery = <&bat>;
> +	};
> +};
> +/* Crypto drivers currently fail & cause an XPU violation */
No need, see commit 4d29db2043610dd70be00a61f26fd64256a2a6c5
[...]

> +	/* Modem antenna pins exclusive to lemonade */
> +	rf_cable_ant1_active: rf_cable_ant1_active {
> +		pins = "gpio27";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +	rf_cable_ant2_active: rf_cable_ant2_active {
Please add a newline between subnodes

Konrad