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

[Caleb Connolly]

Hi Konrad,

On 06/03/2021 10:49 am, Konrad Dybcio wrote:
>
> 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.
>
Yeah, I think it's useful to have a shared compatible for similar 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!
Ah, I haven't touched this device in a while, seems like we don't need 
this anymore on 5.11
>
> 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?
This is based on the MTP board, which does not specify a voltage. Do you 
know what the correct voltage is here? I can find references to 
3300000-3500000 uv but I don't know if that's correct. It seems to be 
derived directly from the battery / power voltage, so this seems about 
right.
>
>> +
>> +&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.

Dropped.

Thanks for the review.

Regards,

Caleb

>
> Konrad
>