linux-kernel - Re: [PATCH v3 2/2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <460cd42b-4192-6761-7313-268a684e1e28@tom-fitzhenry.me.uk>
Date:   Mon, 22 Aug 2022 10:19:00 +1000
From:   Tom Fitzhenry <tom@...-fitzhenry.me.uk>
To:     Nícolas F. R. A. Prado <n@...aprado.net>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        heiko@...ech.de, martijn@...xit.nl, ayufan@...fan.eu, megi@....cz,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, phone-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] arm64: dts: rockchip: Add initial support for
 Pine64 PinePhone Pro

On 18/8/22 13:05, Nícolas F. R. A. Prado wrote:

> thanks for getting the upstreaming of this DT going. Some comments below.

No worries, thank you for your review!

> You're also adding the SD controller here. Does it work as is? If so add it to
> the commit description as well.
I will note this in v4.
>> +/* PinePhone Pro datasheet:
> First comment line should be empty following the coding style [1]. Like you did
> for the copyrights above.
>
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

I will do this in v4.

> This signal is called vcc_sys in the datasheet, so I suggest we keep that name
> here. It's not everyday that we get a device with a publicly available datasheet
> :^).

Indeed! :) I will do this in v4.

> + rk818: pmic@1c {
>> +		compatible = "rockchip,rk818";
>> +		reg = <0x1c>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <RK_PC5 IRQ_TYPE_LEVEL_LOW>;
>> +		#clock-cells = <1>;
>> +		clock-output-names = "xin32k", "rk808-clkout2";
> What about keeping the datasheet names here too? clk32kout1, clk32kout2
Per Megi's response, I'll stick with the current names.
>> +			vcc_1v8: vcc_wl: DCDC_REG4 {
>  From the datasheet, vcc_wl is actually wired to vcc3v3_sys. But looks like
> vcc_wl is only used for bluetooth and you're not enabling it yet anyway, so just
> drop this extra label, and it can be added when bluetooth is added (or not, and
> then the bluetooth supply just points directly to vcc3v3_sys).
Good catch, I will remove the vcc_wl label.
>> +			vcc_power_on: LDO_REG4 {
>> +				regulator-name = "vcc_power_on";
> The name on the datasheet for this one is rk818_pwr_on.
I will use the name rk818_pwr_on in v4.
>> +
>> +&cluster1_opp {
>> +	opp06 {
>> +		status = "disabled";
>> +	};
> There's actually an opp06 node in the OPP for RK3399-T, only that the frequency
> is slightly lower. Maybe you could keep it enabled but override the frequency?
>
> Or given the above point about the max voltages, maybe it would be best to have
> a separate OPP table after all?
Per Megi's response/rationale, I'll keep the existing table, but 
re-introduce cluster1_opp/opp06 with updated frequency/voltage, aligned 
with the RK3399-T datasheet.
>> +
>> +	opp07 {
>> +		status = "disabled";
>> +	};
>> +};
>> +
>> +&io_domains {
>> +	status = "okay";
> Let's keep the status at the end of the node for consistency with the rest.
I will do this in v4.