linux-kernel - Re: [PATCH v4 1/1] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro

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] [day] [month] [year] [list]

Message-Id: <7782b803-b331-4b19-84f5-86d1f06b0259@www.fastmail.com>
Date:   Mon, 29 Aug 2022 04:28:03 +0000
From:   "Tom Fitzhenry" <tom@...-fitzhenry.me.uk>
To:     Ondřej Jirman <megi@....cz>
Cc:     "Rob Herring" <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        "Heiko Stuebner" <heiko@...ech.de>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        phone-devel@...r.kernel.org, "Martijn Braam" <martijn@...xit.nl>,
        Kamil Trzciński <ayufan@...fan.eu>,
        "Caleb Connolly" <kc@...tmarketos.org>,
        Nícolas F . R . A . Prado <n@...aprado.net>
Subject: Re: [PATCH v4 1/1] arm64: dts: rockchip: Add initial support for Pine64
 PinePhone Pro

Thanks for the review Megi.

On Mon, 22 Aug 2022, at 8:33 AM, Ondřej Jirman wrote:
>> +			vdd_center: DCDC_REG2 {
>> +				regulator-name = "vdd_center";
>> +				regulator-always-on;
>> +				regulator-boot-on;
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <1350000>;
>
> Looks like a wrong top voltage. https://megous.com/dl/tmp/ad3dcc62bd00f41f.png

I will fix this in v5.

>> +			vcca3v0_codec: LDO_REG1 {
>> +				regulator-name = "vcca3v0_codec";
>> +				regulator-always-on;
>> +				regulator-boot-on;
>
> This should not be always on, but only enabled by the codec when needed.
> You don't have codec described in this DT.

I will fix this in v5.

>> +			vcc3v0_touch: LDO_REG2 {
>> +				regulator-name = "vcc3v0_touch";
>> +				regulator-always-on;
>> +				regulator-boot-on;
>
> This should not be always on. It should be enabled by touch controller,
> when needed. You don't have touch controller described in this DT.

I will fix this in v5.

>> +			vcca1v8_codec: LDO_REG3 {
>> +				regulator-name = "vcca1v8_codec";
>> +				regulator-always-on;
>> +				regulator-boot-on;
>
> This should not be always on, but only enabled by the codec when needed,
> I suppose. Also modem codec is supplied by vcc1v8_codec which may need
> a gpio configured as pull-down or drive low to be properly disabled,
> and it is not defined in this DT. Please make sure that regulator's input
> doesn't float and is disabled by default.

I will fix this in v5, including adding a regulator for vcc1v8_codec.

>> +&gpu_opp_table {
>> +	opp00 {
>> +		opp-hz = /bits/ 64 <200000000>;
>> +		opp-microvolt = <825000 825000 975000>;
>> +	};
>> +	opp01 {
>> +		opp-hz = /bits/ 64 <297000000>;
>> +		opp-microvolt = <825000 825000 975000>;
>> +	};
>> +	opp02 {
>> +		opp-hz = /bits/ 64 <400000000>;
>> +		opp-microvolt = <825000 825000 975000>;
>> +	};
>> +	opp03 {
>> +		opp-hz = /bits/ 64 <500000000>;
>> +		opp-microvolt = <875000 875000 975000>;
>> +	};
>> +	opp04 {
>> +		opp-hz = /bits/ 64 <600000000>;
>> +		opp-microvolt = <925000 925000 975000>;
>> +	};
>
> ^^^ Why replicate all these OPPs, when they have identical preferred voltage
> in rk3399-opp.dtsi? Also GPU is not being enabled in the DT.
>
> You don't need display output support to enable the gpu right away.

My bad, I had forgotten that I'd originally decided to leave this out of the first patch series. I will address this in the patch series when I enable the GPU.

>> +	opp05 {
>> +		status = "disabled";
>> +	};
>> +};
>> +
>> +
>
> ^ extra space

I will fix this in v5.