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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3fbf451e-6bab-d72a-1d6b-851ece99c95b@somainline.org>
Date:   Wed, 26 May 2021 11:21:34 +0200
From:   Konrad Dybcio <konrad.dybcio@...ainline.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     ~postmarketos/upstreaming@...ts.sr.ht, martin.botka@...ainline.org,
        angelogioacchino.delregno@...ainline.org,
        marijn.suijten@...ainline.org, jamipkettunen@...ainline.org,
        Andy Gross <agross@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH 7/7] arm64: dts: qcom: Add support for SONY Xperia X
 Performance / XZ / XZs (msm8996, Tone platform)

Hi,


> BSD license in all the files please.

msm8996.dtsi is gpl2-only, so I wasn't sure I can include it..


>> +};
>> \ No newline at end of file
> [..]

Yikes... my mistake


>> +#include "pmi8994.dtsi"
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/input/gpio-keys.h>
> This seems to be unused for now.

Right. I will add the keys in a separate patch, this one is plenty big already.


>> +		/*
>> +		 * 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?

Yeah, I think the L2 and cci being stuck at bl clocks are to blame again.. there was a lot of msm8996 cpufreq work but I am not sure if anybody got it to *actually* work in the end, I'll try to look into this soon(tm)..


>> +};
>> +
>> +&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?

I believe it was related to the cpufreq-dt madness, I will remove this.


>> +
>> +&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?

Certainly, but not all peripherals have drivers upstream, and other ones

are hardcoded to their state downstream, and knowing what shady stuff

some vendors do, I'm reluctant to trust the bootloader-default state with

my expensive toys..



> 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 {
> 		...
> 	};
>
> 	...
> };

That does look like a good idea, but I also think it would become a big

mess if any of these pins turned out required for some obscure peripheral,

and then I'd have to dig it out of there, re-create the pin definition outside

and I think you know where this is going..


> +/*
> + * 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).

Eh, Torvalds said 100 is fine :P


Konrad