diff mbox series

[v2,3/3] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro

Message ID 20220805234411.303055-4-tom@tom-fitzhenry.me.uk
State Superseded
Headers show
Series Add support for the Pine64 PinePhone Pro phone | expand

Commit Message

Tom Fitzhenry Aug. 5, 2022, 11:44 p.m. UTC 
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.

This is derived from a combination of https://gitlab.com/pine64-org/linux
and https://megous.com/git/linux.

https://wiki.pine64.org/wiki/PinePhone_Pro

Co-developed-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Martijn Braam <martijn@brixit.nl>
Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk>
---
 arch/arm64/boot/dts/rockchip/Makefile         |   1 +
 .../dts/rockchip/rk3399-pinephone-pro.dts     | 385 ++++++++++++++++++
 2 files changed, 386 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts

Comments

Tom Fitzhenry Aug. 6, 2022, 2:37 a.m. UTC | #1 
On 6/8/22 12:10, Caleb Connolly wrote:
> I was surprised to see this series, and this patch especially.
> An almost ready to submit version of this patch with considerably more 
> functionality has been sat around for a while but unfortunately never 
> sent [1].

Firstly, thank you for your review!

I'm not sure why that other patch series has never been submitted. It 
was prepared 3 months ago (unbeknownst to me, at the time of v1), but 
since then has not been submitted.

I would feel uncomfortable submitting that patch series, since I am not 
familiar with parts of the full DT. In time I intend to be, but for now 
I think we'd benefit from having a base DT mainlined, on top of which we 
can iterate and parallelise.

> According to the link below (and my own knowledge of PPP development) 
> Kamil is the original author of this patch, both Kamil and Martijn 
> created the initial version of the devicetree. Given that you're using 
> their work as a base, Kamil's authorship should be respected in the 
> patch you submit.

I agree authorship is important, and thus Kamil, Martijn and Megi are 
listed as Co-developed-by in this patch.

> Their original patch [2] contained SoBs from them and Martijn, those are 
> both missing below. Both of their signed-off-by tags should be added 
> before this patch hits the mailing list, and the same for Ondrej. The 
> order also seems wrong (Ondrej should be last before you).

Yes, this patch's acceptance is blocked until all Co-developed-by 
authors (Kamil, Martjin, Megi) provide their Signed-off-by to this patch.

> Support for the volume keys, accelerometer, magnetometer, GPIO LEDs, 
> touchscreen, modem codec and audio support are all missing here, but 
> they're included in the patches you referenced. In their current state 
> (see Martijn's commit [1] or my 5.19 rebase [3]) the DT for these 
> components has been worked on by several people, tested by the larger 
> user community, and are already supported in mainline. It seems strange 
> not to include them and just leads to a bunch of extra busywork to add 
> them back later. It's easy enough to drop any of these nodes during 
> review if they become an issue.

To keep this patch series light and easy-to-review, I'd be keen to keep 
it as small as possible for now. This DT is >18 months old out-of-tree 
(across multiple repos), so I think this minimal DT being mainlined is 
an improvement over the status quo.

The existing work that the community has done will still be useful, 
albeit in future patch series. This DT just allows that future work to 
be done iteratively, and in parallel.

> With that being said, I've left some feedback below, with it addressed 
> and the authorship/SoB situation sorted out:
> 
> Reviewed-by: Caleb Connolly <kc@postmarketos.org>

Thank you for your comments, I appreciate your review! I will ensure v3 
addresses those.
Caleb Connolly Aug. 6, 2022, 2:58 p.m. UTC | #2 
On 06/08/2022 03:37, Tom Fitzhenry wrote:
> On 6/8/22 12:10, Caleb Connolly wrote:
>> I was surprised to see this series, and this patch especially.
>> An almost ready to submit version of this patch with considerably more 
>> functionality has been sat around for a while but unfortunately never sent [1].
> 
> Firstly, thank you for your review!
> 
> I'm not sure why that other patch series has never been submitted. It was 
> prepared 3 months ago (unbeknownst to me, at the time of v1), but since then has 
> not been submitted.
> 
> I would feel uncomfortable submitting that patch series, since I am not familiar 
> with parts of the full DT. In time I intend to be, but for now I think we'd 
> benefit from having a base DT mainlined, on top of which we can iterate and 
> parallelise.
> 
>> According to the link below (and my own knowledge of PPP development) Kamil is 
>> the original author of this patch, both Kamil and Martijn created the initial 
>> version of the devicetree. Given that you're using their work as a base, 
>> Kamil's authorship should be respected in the patch you submit.
> 
> I agree authorship is important, and thus Kamil, Martijn and Megi are listed as 
> Co-developed-by in this patch.
To be clear, by authorship I mean literally the author of the patch, the 
previous patch in this series was created by Samuel and you left his authorship 
intact - it has "From: Samuel Dionne-Riel <samuel@dionne-riel.com>" at the top, 
so when a maintainer picks it up and it ends up in Linux, anyone looking at it 
will see that he was the author of the patch.

This patch is from you, there is no From: tag overriding it, and the diffstat in 
your cover letter shows you as the author. Whilst you have obviously made some 
heavy changes to this patch, it isn't your original work as you state yourself 
in the commit message. Authorship should be attributed to Kamil as they are the 
author of the earliest version of this patch we have.
> 
>> Their original patch [2] contained SoBs from them and Martijn, those are both 
>> missing below. Both of their signed-off-by tags should be added before this 
>> patch hits the mailing list, and the same for Ondrej. The order also seems 
>> wrong (Ondrej should be last before you).
> 
> Yes, this patch's acceptance is blocked until all Co-developed-by authors 
> (Kamil, Martjin, Megi) provide their Signed-off-by to this patch.
You should obtain SoBs from Co-developers /before/ sending this patch upstream, 
this indicates that everyone is on board and that the patch has some sensible 
history. The mailing list isn't the place to ask your co-developers to sign off 
a patch after you've made extensive changes to it.

I missed the following points in my original email:

Please read the documentation on modifying patches [1] as it applies here, and 
add a comment before your SoB explaining your changes, something like

[tom: strip down to minimal booting DT for initial support]

This way the history of the patch is preserved properly for anyone looking back 
at it in the future. In a similar vein, replace the external git links with 
links to exact commits so they don't degrade over time.
> 
>> Support for the volume keys, accelerometer, magnetometer, GPIO LEDs, 
>> touchscreen, modem codec and audio support are all missing here, but they're 
>> included in the patches you referenced. In their current state (see Martijn's 
>> commit [1] or my 5.19 rebase [3]) the DT for these components has been worked 
>> on by several people, tested by the larger user community, and are already 
>> supported in mainline. It seems strange not to include them and just leads to 
>> a bunch of extra busywork to add them back later. It's easy enough to drop any 
>> of these nodes during review if they become an issue.
> 
> To keep this patch series light and easy-to-review, I'd be keen to keep it as
fwiw, a few extra nodes now is a lot easier to review than a bunch of individual 
small patches later. But yes, better to get this done in some form than not at all.
> small as possible for now. This DT is >18 months old out-of-tree (across 
> multiple repos), so I think this minimal DT being mainlined is an improvement 
> over the status quo.
definitely, it'll be nice to start to see some upstream story for the famed 
"linux phone" ;)

[1]: https://www.kernel.org/doc/html/latest/maintainer/modifying-patches.html
> 
> The existing work that the community has done will still be useful, albeit in 
> future patch series. This DT just allows that future work to be done 
> iteratively, and in parallel.
> 
>> With that being said, I've left some feedback below, with it addressed and the 
>> authorship/SoB situation sorted out:
>>
>> Reviewed-by: Caleb Connolly <kc@postmarketos.org>
> 
> Thank you for your comments, I appreciate your review! I will ensure v3 
> addresses those.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index ef79a672804a1..cb42e0a15808e 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -42,6 +42,7 @@  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-neo4.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopi-r4s.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-orangepi.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinebook-pro.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-pinephone-pro.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-puma-haikou.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-roc-pc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-roc-pc-mezzanine.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
new file mode 100644
index 0000000000000..f5608487ad58f
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -0,0 +1,385 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020 Martijn Braam <martijn@brixit.nl>
+ * Copyright (c) 2021 Kamil Trzciński <ayufan@ayufan.eu>
+ */
+
+// PinePhone Pro datasheet: https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
+
+/dts-v1/;
+#include <dt-bindings/input/gpio-keys.h>
+#include <dt-bindings/input/linux-event-codes.h>
+#include "rk3399.dtsi"
+#include "rk3399-t-opp.dtsi"
+
+/ {
+	model = "Pine64 PinePhonePro";
+	compatible = "pine64,pinephone-pro", "rockchip,rk3399";
+	chassis-type = "handset";
+
+	aliases {
+                mmc0 = &sdio0;
+                mmc1 = &sdmmc;
+                mmc2 = &sdhci;
+        };
+
+	chosen {
+		stdout-path = "serial2:115200n8";
+	};
+
+	gpio-key-power {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwrbtn_pin>;
+
+		power {
+			debounce-interval = <20>;
+			// Per "PMU Controler", page 4.
+			gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>;
+			label = "Power";
+			linux,code = <KEY_POWER>;
+			wakeup-source;
+		};
+	};
+
+	/* Power tree */
+	/* Root power source */
+	vcc_sysin: vcc-sysin {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_sysin";
+		regulator-always-on;
+		regulator-boot-on;
+	};
+
+	/* Main 3.3v supply */
+	vcc3v3_sys: wifi_bat: vcc3v3-sys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&vcc_sysin>;
+	};
+
+	vcca1v8_s3: vcc1v8-s3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcca1v8_s3";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc3v3_sys>;
+		regulator-always-on;
+		regulator-boot-on;
+	};
+};
+
+&cpu_l0 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l1 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l2 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l3 {
+	cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_b0 {
+	cpu-supply = <&vdd_cpu_b>;
+};
+
+&cpu_b1 {
+	cpu-supply = <&vdd_cpu_b>;
+};
+
+&emmc_phy {
+	status = "okay";
+};
+
+&i2c0 {
+	// Per "SCL clock frequency", page 30, RK818 datasheet.
+	clock-frequency = <400000>;
+	i2c-scl-rising-time-ns = <168>;
+	i2c-scl-falling-time-ns = <4>;
+	status = "okay";
+
+	// Per "PMIC RK818-3", page 13.
+	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";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_l>;
+		rockchip,system-power-controller;
+		wakeup-source;
+
+		vcc1-supply = <&vcc_sysin>;
+		vcc2-supply = <&vcc_sysin>;
+		vcc3-supply = <&vcc_sysin>;
+		vcc4-supply = <&vcc_sysin>;
+		vcc6-supply = <&vcc_sysin>;
+		vcc7-supply = <&vcc3v3_sys>;
+		vcc8-supply = <&vcc_sysin>;
+		vcc9-supply = <&vcc3v3_sys>;
+
+		regulators {
+			vdd_cpu_l: DCDC_REG1 {
+				regulator-name = "vdd_cpu_l";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <750000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <6001>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vdd_center: DCDC_REG2 {
+				regulator-name = "vdd_center";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <6001>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcc_ddr: DCDC_REG3 {
+				regulator-name = "vcc_ddr";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_1v8: vcc_wl: DCDC_REG4 {
+				regulator-name = "vcc_1v8";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			// Audio codec.
+			vcca3v0_codec: LDO_REG1 {
+				regulator-name = "vcca3v0_codec";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			// Touch screen.
+			vcc3v0_touch: LDO_REG2 {
+				regulator-name = "vcc3v0_touch";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vcca1v8_codec: LDO_REG3 {
+				regulator-name = "vcca1v8_codec";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_power_on: LDO_REG4 {
+				regulator-name = "vcc_power_on";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_3v0: LDO_REG5 {
+				regulator-name = "vcc_3v0";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_1v5: LDO_REG6 {
+				regulator-name = "vcc_1v5";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1500000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc1v8_dvp: LDO_REG7 {
+				regulator-name = "vcc1v8_dvp";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+			};
+
+			vcc3v3_s3: LDO_REG8 {
+				regulator-name = "vcc3v3_s3";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			vccio_sd: LDO_REG9 {
+				regulator-name = "vccio_sd";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			vcc3v3_s0: SWITCH_REG {
+				regulator-name = "vcc3v3_s0";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-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;
+		};
+	};
+
+	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>;
+		regulator-ramp-delay = <1000>;
+		regulator-always-on;
+		regulator-boot-on;
+
+		regulator-state-mem {
+			regulator-off-in-suspend;
+		};
+	};
+};
+
+&io_domains {
+        status = "okay";
+
+        bt656-supply = <&vcc1v8_dvp>;
+        audio-supply = <&vcca1v8_codec>;
+        sdmmc-supply = <&vccio_sd>;
+        gpio1830-supply = <&vcc_3v0>;
+};
+
+&pmu_io_domains {
+	pmu1830-supply = <&vcc_1v8>;
+	status = "okay";
+};
+
+&pinctrl {
+	buttons {
+		pwrbtn_pin: pwrbtn-pin {
+			rockchip,pins = <0 RK_PA5 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
+	pmic {
+		pmic_int_l: pmic-int-l {
+			rockchip,pins = <1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
+		vsel1_pin: vsel1-pin {
+			rockchip,pins = <1 RK_PC1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		vsel2_pin: vsel2-pin {
+			rockchip,pins = <1 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+};
+
+// Per "SDMMC Controler", page 6.
+&sdmmc {
+	bus-width = <4>;
+	cap-sd-highspeed;
+	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	max-frequency = <150000000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
+	vmmc-supply = <&vcc3v3_sys>;
+	vqmmc-supply = <&vccio_sd>;
+	status = "okay";
+};
+
+&sdhci {
+	bus-width = <8>;
+	mmc-hs200-1_8v;
+	non-removable;
+	status = "okay";
+};
+
+// Enable thermal sensors.
+&tsadc {
+	/* tshut mode 0:CRU 1:GPIO */
+	rockchip,hw-tshut-mode = <1>;
+	/* tshut polarity 0:LOW 1:HIGH */
+	rockchip,hw-tshut-polarity = <1>;
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};