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

[Konrad Dybcio <konrad.dybcio@somainline.org>]

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