| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <16a1c897-cefc-6c4b-3031-d7fc025513f1@connolly.tech>
Date: Sat, 06 Mar 2021 18:31:11 +0000
From: Caleb Connolly <caleb@...nolly.tech>
To: Konrad Dybcio <konrad.dybcio@...ainline.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Kees Cook <keescook@...omium.org>,
Anton Vorontsov <anton@...msg.org>,
Colin Cross <ccross@...roid.com>,
Tony Luck <tony.luck@...el.com>
Cc: ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: add support for the Pixel 2 XL
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@...nolly.tech>
>> ---
>> 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@...nolly.tech>
>> + */
>> +
>> +/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@...nolly.tech> */
>> +
>> +#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
>
Powered by blists - more mailing lists