Message-ID: <20220821090005.tshf7z2kklaet7ll@core>
Date:   Sun, 21 Aug 2022 11:00:05 +0200
From:   Ondřej Jirman <megi@....cz>
To:     Nícolas F. R. A. Prado <n@...aprado.net>
Cc:     Tom Fitzhenry <tom@...-fitzhenry.me.uk>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, heiko@...ech.de,
        martijn@...xit.nl, ayufan@...fan.eu, 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

Hi Nícolas,

On Wed, Aug 17, 2022 at 11:05:47PM -0400, Nícolas F. R. A. Prado wrote:
> Hi Tom,
> 
> thanks for getting the upstreaming of this DT going. Some comments below.
> 
> On Mon, Aug 15, 2022 at 10:30:04PM +1000, Tom Fitzhenry wrote:
> > From: Martijn Braam <martijn@...xit.nl>
> > 
> > This is a basic DT containing regulators and UART, intended to be a
> > base that myself and others can add additional nodes in future patches.
> > 
> > Tested to work: booting from eMMC, output over UART.
> 
> You're also adding the SD controller here. Does it work as is? If so add it to
> the commit description as well.
> 
> > 
> [..]
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > @@ -0,0 +1,394 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2020 Martijn Braam <martijn@...xit.nl>
> > + * Copyright (c) 2021 Kamil Trzciński <ayufan@...fan.eu>
> > + */
> > +
> > +/* 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
> 
> > + * https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
> > + */
> [..]
> > +	vcc_sysin: vcc-sysin-regulator {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_sysin";
> 
> 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
> :^).
> 
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +	};
> [..]
> > +	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

Rather not. My guess is that trying to change the names will break the clock
tree, because xin32k is hardcoded in the rk3399 clk driver as parent for
mux_pll_p.

https://elixir.bootlin.com/linux/latest/source/drivers/clk/rockchip/clk-rk3399.c#L109

These names are not just a cosmetic thing.

> 
> > +		pinctrl-names = "default";
> [..]
> > +			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).
> 
> > +				regulator-name = "vcc_1v8";
> [..]
> > +			vcc_power_on: LDO_REG4 {
> > +				regulator-name = "vcc_power_on";
> 
> The name on the datasheet for this one is rk818_pwr_on.
> 
> > +				regulator-always-on;
> [..]
> > +&cluster0_opp {
> > +	opp04 {
> > +		status = "disabled";
> > +	};
> > +
> > +	opp05 {
> > +		status = "disabled";
> > +	};
> > +};
> 
> I saw the discussion on the previous version about using the rk3399-opp.dtsi
> here, but the thing is, this OPP has greater values for the max voltage than the
> maximum allowed on the OPP table you posted previously (for RK3399-T)...

Max voltages in OPP table are irreleavant for Pinephone Pro DT, because the CPU
regulators in the PMIC are capable of setting the preferred voltage or very
close to it.

The actual min/max limits that are enforced by the kernel for the board are set
on the CPU regulator, which are actually wrongly set in this DT. See:

https://dl.radxa.com/rockpi4/docs/hw/datasheets/Rockchip%20RK3399-T%20Datasheet%20V1.0-20210818.pdf

+			vdd_cpu_l: DCDC_REG1 {
+				regulator-name = "vdd_cpu_l";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <750000>;
+				regulator-max-microvolt = <1350000>;

Should be 975000 (or at most 1300000 to fit within absmax)

+				regulator-ramp-delay = <6001>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};


+	vdd_cpu_b: regulator@40 {
+		compatible = "silergy,syr827";
+		reg = <0x40>;
+		fcs,suspend-voltage-selector = <1>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&vsel1_pin>;
+		regulator-name = "vdd_cpu_b";
+		regulator-min-microvolt = <712500>;
+		regulator-max-microvolt = <1500000>;
+		regulator-ramp-delay = <1000>;
+		regulator-always-on;
+		regulator-boot-on;
+
+		regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};

Should be 1150000 (or at most 1300000 to fit within absmax)

+	vdd_gpu: regulator@41 {
+		compatible = "silergy,syr828";
+		reg = <0x41>;
+		fcs,suspend-voltage-selector = <1>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&vsel2_pin>;
+		regulator-name = "vdd_gpu";
+		regulator-min-microvolt = <712500>;
+		regulator-max-microvolt = <1500000>;

Should be 975000 (or at most 1300000 to fit within absmax)

+		regulator-ramp-delay = <1000>;
+		regulator-always-on;
+		regulator-boot-on;
+
+		regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};

The max values in OPP table also still fit within abs. max values for RK3399-T,
so they are safe in any case.

> Same thing for the GPU OPP.
> 
> > +
> > +&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?

No separate OPP table, please, unless absolutely necessary.

kind regards,
	Ondrej

> > +
> > +	opp07 {
> > +		status = "disabled";
> > +	};
> > +};
> > +
> > +&io_domains {
> > +	status = "okay";
> 
> Let's keep the status at the end of the node for consistency with the rest.
> 
> With these points addressed:
> 
> Reviewed-by: Nícolas F. R. A. Prado <n@...aprado.net>
> 
> I'll also try to give this a test shortly.
> 
> Thanks,
> Nícolas
> 
> > +
> > +	bt656-supply = <&vcc1v8_dvp>;
> > +	audio-supply = <&vcca1v8_codec>;
> > +	sdmmc-supply = <&vccio_sd>;
> > +	gpio1830-supply = <&vcc_3v0>;
> > +};
> [..]

Powered by blists - more mailing lists