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: <YK+/RJX2DXvyHh0J@builder.lan>
Date:   Thu, 27 May 2021 10:48:20 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...ainline.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)

On Wed 26 May 04:21 CDT 2021, Konrad Dybcio wrote:

> Hi,
> 
> 
> > BSD license in all the files please.
> 
> msm8996.dtsi is gpl2-only, so I wasn't sure I can include it..
> 

Very good question, I think it's fine, would prefer it that way and will
be merge it, but ianal.

> 
> >> +};
> >> \ 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)..
> 

Right, we have some lingering patches on our side for this as well,
unfortunately I have one db82c0 that gets unstable when we bring the
last 2 CPUs up to speed.

I started looking at this and found that some care needs to be taken
when switching between the lower half and higher half of the frequencies
(or perhaps it was voltages?) Unfortunately I didn't conclude anything
in this area, but I would be happy to see this resolved.

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

I'm in agreement with you that we should vote for these somewhere, so
I'm fine with us agreeing that they are voted for here for now.

> 
> 
> > 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..
> 

But if this happens you need to override the giant pinctrl-0 in such
device dts and hope that you don't miss any of the entries anyways.

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

Right, 100 is the new "limit", but 80 is still a good target. What I'm
opposing is that you wrap at 44.

Thanks,
Bjorn