Re: [PATCH] arm64: dts: add support for the Pixel 2 XL

[Konrad Dybcio]

On 05.03.2021 22:35, Caleb Connolly wrote:
> Add a minimal devicetree capable of booting on the Pixel 2 XL MSM8998
> device.
>
> It's currently possible to boot the device into postmarketOS with USB
> networking, however the display panel depends on Display Stream
> Compression which is not yet supported in the kernel.
>
> The bootloader also requires that the dtbo partition contains a device
> tree overlay with a particular id which has to be overlayed onto the
> existing dtb. It's possible to use a specially crafted dtbo partition to
> workaround this, more information is available here:
>
>     https://gitlab.com/calebccff/dtbo-google-wahoo-mainline
>
> Signed-off-by: Caleb Connolly <caleb@xxxxxxxxxxxxx>
> ---
> It's possible to get wifi working by running Bjorns diag-router in the
> background, without this the wifi firmware crashes every 10 seconds or
> so. This is the same issue encountered on the OnePlus 5.
>
>  arch/arm64/boot/dts/qcom/Makefile             |   1 +
>  .../boot/dts/qcom/msm8998-google-taimen.dts   |  14 +
>  .../boot/dts/qcom/msm8998-google-wahoo.dtsi   | 391 ++++++++++++++++++
>  3 files changed, 406 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8998-google-taimen.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8998-google-wahoo.dtsi
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 5113fac80b7a..d942d3ec3928 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -16,6 +16,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-msft-lumia-cityman.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8994-sony-xperia-kitakami-sumire.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8996-mtp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-asus-novago-tp370ql.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-google-taimen.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-hp-envy-x2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-lenovo-miix-630.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-mtp.dtb
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-google-taimen.dts b/arch/arm64/boot/dts/qcom/msm8998-google-taimen.dts
> new file mode 100644
> index 000000000000..ffaaafe14037
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8998-google-taimen.dts
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, Caleb Connolly <caleb@xxxxxxxxxxxxx>
> + */
> +
> +/dts-v1/;
> +
> +#include "msm8998-google-wahoo.dtsi"
> +
> +/ {
> +	model = "Google Pixel 2 XL";
> +	compatible = "google,taimen", "google,wahoo", "qcom,msm8998", "qcom,msm8998-mtp";

Drop the mtp compatible. Also, afaict wahoo is a shared platform name for P2/2XL, so perhaps using the same naming scheme we used for Xperias/Lumias (soc-vendor-platform-board) would clear up some confusion. In this case, I'm not sure about the wahoo compatible, but I reckon it's fine for it to stay so that it's easier to introduce potential quirks that concern both devices.


> +	qcom,msm-id = <0x124 0x20001>;

Move it to the common dtsi, unless the other Pixel ships with a different SoC revision.


> +};
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-google-wahoo.dtsi b/arch/arm64/boot/dts/qcom/msm8998-google-wahoo.dtsi
> new file mode 100644
> index 000000000000..0c221ead2df7
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8998-google-wahoo.dtsi
> @@ -0,0 +1,391 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2020 Caleb Connolly <caleb@xxxxxxxxxxxxx> */
> +
> +#include "msm8998.dtsi"
> +#include "pm8998.dtsi"
> +#include "pmi8998.dtsi"
> +#include "pm8005.dtsi"
> +
> +/delete-node/ &mpss_mem;
> +/delete-node/ &venus_mem;
> +/delete-node/ &mba_mem;
> +/delete-node/ &slpi_mem;
> +
> +/ {
> +	aliases {
> +	};
> +
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		/* Add "earlycon" intended to be used in combination with UART serial console */
> +		bootargs = "clk_ignore_unused earlycon console=ttyGS0,115200";// loglevel=10 drm.debug=15 debug";

clk_ignore_unused is a BIG hack!

You should trace which clocks are important for it to stay alive and fix it on the driver side.

What breaks if it's not there? Does it still happen with Angelo's clk patches that got in for the 5.12

window?

Aside from that, //loglevel... should also go.


> +
> +	vph_pwr: vph-pwr-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vph_pwr";
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};
> +};

Don't you need to specify voltage here?


> +
> +&blsp1_uart3 {
> +	status = "disabled";
> +
> +	bluetooth {
> +		compatible = "qcom,wcn3990-bt";
> +
> +		vddio-supply = <&vreg_s4a_1p8>;
> +		vddxo-supply = <&vreg_l7a_1p8>;
> +		vddrf-supply = <&vreg_l17a_1p3>;
> +		vddch0-supply = <&vreg_l25a_3p3>;
> +		max-speed = <3200000>;
> +	};
> +};

Either enable the UART or rid the bluetooth for now.


> +
> +&pm8005_lsid1 {
> +	pm8005-regulators {
> +		compatible = "qcom,pm8005-regulators";
> +
> +		vdd_s1-supply = <&vph_pwr>;
> +
> +		pm8005_s1: s1 { /* VDD_GFX supply */

regulator-name = "vdd_gfx";


> +
> +&spmi_bus {
> +	pmic@0 {
> +		compatible = "qcom,pm8994", "qcom,spmi-pmic";
> +		reg = <0x0 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pon@800 {
> +			compatible = "qcom,pm8916-pon";
> +
> +			reg = <0x800>;
> +			mode-bootloader = <0x2>;
> +			mode-recovery = <0x1>;
> +
> +			pwrkey {
> +				compatible = "qcom,pm8941-pwrkey";
> +				interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
> +				debounce = <15625>;
> +				bias-pull-up;
> +				linux,code = <KEY_POWER>;
> +			};
> +		};
> +	};
> +};

That's a lot of indentation, it would be better to add a label: somewhere instead.. Moreover, the exact same pwrkey node is already present in pm8998.dtsi, so you should just drop this.


Konrad