PinePhone Pro is a RK3399 based phone produced by Pine64.
Add a basic DTS for it. This is a working base that will allow myself and
others to add more nodes.
Changes since v1:
* Simplified the DT to a minimal base.
* Introduced the RK3399-T OPPs.
Samuel Dionne-Riel (1):
arm64: dts: rockchip: Add RK3399-T opp
Tom Fitzhenry (2):
dt-bindings: arm: rockchip: Add PinePhone Pro bindings
arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro
.../devicetree/bindings/arm/rockchip.yaml | 5 +
arch/arm64/boot/dts/rockchip/Makefile | 1 +
.../dts/rockchip/rk3399-pinephone-pro.dts | 385 ++++++++++++++++++
.../arm64/boot/dts/rockchip/rk3399-t-opp.dtsi | 118 ++++++
4 files changed, 509 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
--
2.37.1
From: Samuel Dionne-Riel <[email protected]>
These tables were derived from the regular RK3399 table, by dropping
entries exceeding recommended operating conditions from the datasheet,
and clamping the last exceeding value where it made sense.
Signed-off-by: Samuel Dionne-Riel <[email protected]>
---
.../arm64/boot/dts/rockchip/rk3399-t-opp.dtsi | 118 ++++++++++++++++++
1 file changed, 118 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
new file mode 100644
index 0000000000000..ec153015d9d13
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
+ * Copyright (c) 2022 Samuel Dionne-Riel <[email protected]>
+ */
+
+/ {
+ cluster0_opp: opp-table-0 {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp00 {
+ opp-hz = /bits/ 64 <408000000>;
+ opp-microvolt = <825000 825000 925000>;
+ clock-latency-ns = <40000>;
+ };
+ opp01 {
+ opp-hz = /bits/ 64 <600000000>;
+ opp-microvolt = <825000 825000 925000>;
+ };
+ opp02 {
+ opp-hz = /bits/ 64 <816000000>;
+ opp-microvolt = <850000 850000 925000>;
+ };
+ opp03 {
+ opp-hz = /bits/ 64 <1008000000>;
+ opp-microvolt = <925000 925000 925000>;
+ };
+ };
+
+ cluster1_opp: opp-table-1 {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp00 {
+ opp-hz = /bits/ 64 <408000000>;
+ opp-microvolt = <825000 825000 1150000>;
+ clock-latency-ns = <40000>;
+ };
+ opp01 {
+ opp-hz = /bits/ 64 <600000000>;
+ opp-microvolt = <825000 825000 1150000>;
+ };
+ opp02 {
+ opp-hz = /bits/ 64 <816000000>;
+ opp-microvolt = <825000 825000 1150000>;
+ };
+ opp03 {
+ opp-hz = /bits/ 64 <1008000000>;
+ opp-microvolt = <875000 875000 1150000>;
+ };
+ opp04 {
+ opp-hz = /bits/ 64 <1200000000>;
+ opp-microvolt = <950000 950000 1150000>;
+ };
+ opp05 {
+ opp-hz = /bits/ 64 <1416000000>;
+ opp-microvolt = <1025000 1025000 1150000>;
+ };
+ opp06 {
+ opp-hz = /bits/ 64 <1500000000>;
+ opp-microvolt = <1100000 1100000 1150000>;
+ };
+ };
+
+ gpu_opp_table: opp-table-2 {
+ compatible = "operating-points-v2";
+
+ 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>;
+ };
+ };
+};
+
+&cpu_l0 {
+ operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_l1 {
+ operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_l2 {
+ operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_l3 {
+ operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_b0 {
+ operating-points-v2 = <&cluster1_opp>;
+};
+
+&cpu_b1 {
+ operating-points-v2 = <&cluster1_opp>;
+};
+
+&gpu {
+ operating-points-v2 = <&gpu_opp_table>;
+};
--
2.37.1
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 <[email protected]>
Co-developed-by: Martijn Braam <[email protected]>
Co-developed-by: Kamil Trzciński <[email protected]>
Signed-off-by: Tom Fitzhenry <[email protected]>
---
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
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 <[email protected]>
+ * Copyright (c) 2021 Kamil Trzciński <[email protected]>
+ */
+
+// 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";
+};
--
2.37.1
Document board compatible names for Pine64 PinePhonePro.
https://wiki.pine64.org/wiki/PinePhone_Pro
Signed-off-by: Tom Fitzhenry <[email protected]>
---
Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
index 7811ba64149cb..8ddedbd1ce317 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.yaml
+++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
@@ -494,6 +494,11 @@ properties:
- const: pine64,pinenote
- const: rockchip,rk3566
+ - description: Pine64 PinePhonePro
+ items:
+ - const: pine64,pinephone-pro
+ - const: rockchip,rk3399
+
- description: Pine64 Rock64
items:
- const: pine64,rock64
--
2.37.1
On 06/08/2022 00:44, Tom Fitzhenry wrote:
> 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.
Hi,
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].
Creating a minimal patch to just finally get the PinePhone pro into mainline is
not a bad idea, but as it stands I'm not fully on board with this patch.
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.
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).
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.
With that being said, I've left some feedback below, with it addressed and the
authorship/SoB situation sorted out:
Reviewed-by: Caleb Connolly <[email protected]>
Alternatively, I'd be more than happy to review a new revision with the
aforementioned nodes re-included.
Kind Regards,
Caleb
[1]:
https://git.sr.ht/~martijnbraam/linux-unlisted/commit/01f0c90f6c4c1649f17a0ced72fc32a26d509e06
[2]:
https://gitlab.com/pine64-org/linux/-/commit/261d3b5f8ac503f97da810986d1d6422430c8531
[3]:
https://gitlab.com/pine64-org/linux/-/blob/caleb/ppp-5.19-rc5/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>
> 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 <[email protected]>
> Co-developed-by: Martijn Braam <[email protected]>
> Co-developed-by: Kamil Trzciński <[email protected]>
> Signed-off-by: Tom Fitzhenry <[email protected]>
> ---
> 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
>
> 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 <[email protected]>
> + * Copyright (c) 2021 Kamil Trzciński <[email protected]>
> + */
> +
> +// PinePhone Pro datasheet: https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
Use C-style comments
> +
> +/dts-v1/;
> +#include <dt-bindings/input/gpio-keys.h>
This header is unused and can be dropped.
> +#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.
Comments like these aren't useful and should be dropped
> + gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>;
> + label = "Power";
> + linux,code = <KEY_POWER>;
> + wakeup-source;
> + };
> + };
> +
> + /* Power tree */
> + /* Root power source */
These too
> + 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.
Drop
> + clock-frequency = <400000>;
> + i2c-scl-rising-time-ns = <168>;
> + i2c-scl-falling-time-ns = <4>;
> + status = "okay";
> +
> + // Per "PMIC RK818-3", page 13.
Drop
> + 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.
Drop
> + 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.
Drop
> + 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.
Drop
> +&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.
Drop
> +&tsadc {
> + /* tshut mode 0:CRU 1:GPIO */
Drop
> + rockchip,hw-tshut-mode = <1>;
> + /* tshut polarity 0:LOW 1:HIGH */
Drop
> + rockchip,hw-tshut-polarity = <1>;
> + status = "okay";
> +};
> +
> +&uart2 {
> + status = "okay";
> +};
Hi,
On 06/08/2022 00:44, Tom Fitzhenry wrote:
> PinePhone Pro is a RK3399 based phone produced by Pine64.
Please CC [email protected] for phone related patches
>
> Add a basic DTS for it. This is a working base that will allow myself and
> others to add more nodes.
>
> Changes since v1:
> * Simplified the DT to a minimal base.
> * Introduced the RK3399-T OPPs.
>
> Samuel Dionne-Riel (1):
> arm64: dts: rockchip: Add RK3399-T opp
>
> Tom Fitzhenry (2):
> dt-bindings: arm: rockchip: Add PinePhone Pro bindings
> arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro
>
> .../devicetree/bindings/arm/rockchip.yaml | 5 +
> arch/arm64/boot/dts/rockchip/Makefile | 1 +
> .../dts/rockchip/rk3399-pinephone-pro.dts | 385 ++++++++++++++++++
> .../arm64/boot/dts/rockchip/rk3399-t-opp.dtsi | 118 ++++++
> 4 files changed, 509 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
>
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 <[email protected]>
Thank you for your comments, I appreciate your review! I will ensure v3
addresses those.
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 <[email protected]>" 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 <[email protected]>
>
> Thank you for your comments, I appreciate your review! I will ensure v3
> addresses those.
Hi Caleb and Tom,
more below.
On Sat, Aug 06, 2022 at 03:58:17PM +0100, Caleb Connolly wrote:
> On 06/08/2022 03:37, Tom Fitzhenry wrote:
> > On 6/8/22 12:10, Caleb Connolly wrote:
> > > 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
> <[email protected]>" 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.
Looks like there's some confusion here, that I may have added to at some
point by a bit of patch squashing, and some SoB tag creativity. ;)
Let me try to clear it up a bit, since it's my squashed patch this discussion
and this submission is based around. Patch [2] you're linking to is not an
original patch by Martijn or Kamil.
It's just a squashed bunch of patches that I took from a secret repo that Kamil
shared with me once upon a time, so that I can help with kernel development and
camera support for Pinephone Pro about a year ago, when it was still a secret
thing.
*AFAIK*, original patches in the secret repo were never published by authors,
but only through my squashed patch from my kernel tree. All the prior work seems
to be either in unlisted git repos, or in login gated git repos.
So hereby I give my Signed-off-by: Ondrej Jriman <[email protected]> for the patch
[2] this submission by Tom was developed from, stating that to the best of my
knowledge:
1) Original author is Martijn Braam and "Fuzhou Rockchip Electronics Co., Ltd".
Kamil Trzciński extended the Martijn's original DT with some things that are
mostly stripped off in this submission, other than a few oneliners and the
gpio power key node. I also have maybe one or two lines in this submission.
2) Original work was published under MIT or GPL2.0+
3) Original work is missing copyright header from Rockchip and any SoB on the
patches. I have added the SoB for Martijn/Kamil myself without asking them.
Sorry for that. And I guess this is the source of some confusion here, too.
That should cover the requirement b) of "Developer’s Certificate of Origin 1.1"
which should be all that's necessary.
So now that we have all the needed SoB for the base patch, and Tom already has
SoB for his modifications, all the Co-developed-by are in place, and copyright
notices from original authors are untouched in the code, except for Rockchip,
the way forward should be to re-add missing:
Copyright (c) 2021 Fuzhou Rockchip Electronics Co., Ltd
to the code and add my SoB to the commit message. That should make the
provenance of this code clear and properly accounted for.
SoB from Martijn would be nice to have, just to reassure about the Rockchip
copyright, which I assume authored the Android factory kernel code/DT. But I'm
presonally quite sure this code is MIT/GPL2.0+, regardless.
------
If you want more details, then here is the original patch series from the secret
repo: https://megous.com/git/linux/log/?h=ayufan-secret/pinephone-pro
Martijn's original patch as taken over by Kamil is
https://megous.com/git/linux/commit/?h=ayufan-secret/pinephone-pro&id=9f1a4867c21a9fa88177bd0903bdc1d82e213310
It has Martijn's copyright and is published under GPL2.0+ OR MIT. This is the
earliest available code this all is based on.
It will sure have some history, too, given that almost all of rk818 node content
matches the decompiled Android factory image's device tree to the
T and that's like 50% of the code in this submission. :) That's where my guess
that the original patch is missing Rockchip's copyright is comming from.
Android DT was never published in code form for all I know. I guess that's what
we get for requiring DT be licensed under MIT license. Some Rockchip engineer
likely wrote half of this submission. Rockchip copyright is thus probably
missing from the patch. Decompiled DT here: https://megous.com/dl/tmp/01_dtbdump_rockchip,android.dts
Kamil seems to have these changes in this submission:
https://megous.com/git/linux/commit/?h=ayufan-secret/pinephone-pro&id=30976b1d8b7c7958474bd54fc86db79ba11d7a17
https://megous.com/git/linux/commit/?h=ayufan-secret/pinephone-pro&id=28b33d187f6b8dd672115e69a19d8d9a6725c834
https://megous.com/git/linux/commit/?h=ayufan-secret/pinephone-pro&id=083d7b514a7ac0e97a18bf4012259f4d9fa37733
https://megous.com/git/linux/commit/?h=ayufan-secret/pinephone-pro&id=bbc9ca6051500baf28296908d92286ff1c4087e0
(just the power key part)
kind regards,
Ondrej
> 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]
These rules are for subsystem maintainers. At least it says so on top.
> 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.
On 06/08/2022 01:44, Tom Fitzhenry wrote:
Thank you for your patch. There is something to discuss/improve.
> 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 <[email protected]>
> + * Copyright (c) 2021 Kamil Trzciński <[email protected]>
> + */
> +
> +// 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 {
Just gpio-keys like all sources are doing.
> + compatible = "gpio-keys";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pwrbtn_pin>;
> +
> + power {
Schema requires generic naming. Either "key" or "key-0" or "key-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 {
regulator-vcc-sysin
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "regulator-fixed";
> + regulator-name = "vcc_sysin";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + /* Main 3.3v supply */
> + vcc3v3_sys: wifi_bat: vcc3v3-sys {
regulator-.....
> + 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 {
regulator-.....
> + 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;
> + };
> +};
> +
Best regards,
Krzysztof
On 06/08/2022 04: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.
But you miss their SoB... Without them you should not send it. It does
not pass checkpatch, does it?
>
>> 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 add SoB based on original work. When you send a patch, it is
expected to be ready (so having correct DCO chain), not incomplete from
our process point of view.
Best regards,
Krzysztof
On 8/8/22 16:38, Krzysztof Kozlowski wrote:
>> I agree authorship is important, and thus Kamil, Martijn and Megi are
>> listed as Co-developed-by in this patch.
>
> But you miss their SoB... Without them you should not send it. It does
> not pass checkpatch, does it?
Sorry, my bad. I see
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
lists to use checkpatch. I had read this but had since forgotten. I will
ensure future patches pass checkpatch.
On 08/08/2022 14:12, Ondřej Jirman wrote:
> Hello,
>
> On Mon, Aug 08, 2022 at 09:35:55AM +0300, Krzysztof Kozlowski wrote:
>> On 06/08/2022 01:44, Tom Fitzhenry wrote:
>>
>> [...]
>>
>>> +
>>> + /* Power tree */
>>> + /* Root power source */
>>> + vcc_sysin: vcc-sysin {
>>
>> regulator-vcc-sysin
>
> Interestingly, most DTS files in rockchip/ use a -regulator
> suffix and none use regulator- prefix. And this is inconsistent
> across the larger DTS tree, because outside of rockchip/ most
> DTS use a prefix.
>
> Checked by grep -R 'regulator.*{' | grep -v state-me
Can be a suffix. Just pick one pattern.
Best regards,
Krzysztof
Hello,
On Mon, Aug 08, 2022 at 09:35:55AM +0300, Krzysztof Kozlowski wrote:
> On 06/08/2022 01:44, Tom Fitzhenry wrote:
>
> [...]
>
> > +
> > + /* Power tree */
> > + /* Root power source */
> > + vcc_sysin: vcc-sysin {
>
> regulator-vcc-sysin
Interestingly, most DTS files in rockchip/ use a -regulator
suffix and none use regulator- prefix. And this is inconsistent
across the larger DTS tree, because outside of rockchip/ most
DTS use a prefix.
Checked by grep -R 'regulator.*{' | grep -v state-me
regards,
o.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc_sysin";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > +
> > + /* Main 3.3v supply */
> > + vcc3v3_sys: wifi_bat: vcc3v3-sys {
>
> regulator-.....
>
> > + 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 {
>
> regulator-.....
>
> > + 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;
> > + };
> > +};
> > +
> Best regards,
> Krzysztof
Hi,
On Sat, Aug 06, 2022 at 09:44:09AM +1000, Tom Fitzhenry wrote:
> From: Samuel Dionne-Riel <[email protected]>
>
> These tables were derived from the regular RK3399 table, by dropping
> entries exceeding recommended operating conditions from the datasheet,
> and clamping the last exceeding value where it made sense.
Do we really need to duplicate the whole OPP table of rk3399-opp.dtsi
just to disable a few top opp## entries?
This will make it more annoying to add PVTM/eFuse leakage based voltage
selection support later on, because then there would have to be identical
multi-level operating points across multiple files. And that sounds like
a LOT of dupplication for little benefit.
Also Pinephone Pro has RK3399S not -T. RK3399 seems to be RK3399 selected for
low leakage (values I've seen from eFuses indicate the leakage is half that of
RK3399 available in Pinebook Pro)
I'd suggest just adding references to select operating point nodes that
are "too much" and disabling them with status = "disabled". This
can be done from the pinephone device tree file directly.
Otherwise we'll eventually end up with several files containing
something like this [1] and only differing in absence of some opp## nodes
and not their actual useful content.
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk3399-opp.dtsi#L6
kind regards,
o.
> Signed-off-by: Samuel Dionne-Riel <[email protected]>
> ---
> .../arm64/boot/dts/rockchip/rk3399-t-opp.dtsi | 118 ++++++++++++++++++
> 1 file changed, 118 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
> new file mode 100644
> index 0000000000000..ec153015d9d13
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
> + * Copyright (c) 2022 Samuel Dionne-Riel <[email protected]>
> + */
> +
> +/ {
> + cluster0_opp: opp-table-0 {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + opp00 {
> + opp-hz = /bits/ 64 <408000000>;
> + opp-microvolt = <825000 825000 925000>;
> + clock-latency-ns = <40000>;
> + };
> + opp01 {
> + opp-hz = /bits/ 64 <600000000>;
> + opp-microvolt = <825000 825000 925000>;
> + };
> + opp02 {
> + opp-hz = /bits/ 64 <816000000>;
> + opp-microvolt = <850000 850000 925000>;
> + };
> + opp03 {
> + opp-hz = /bits/ 64 <1008000000>;
> + opp-microvolt = <925000 925000 925000>;
> + };
> + };
> +
> + cluster1_opp: opp-table-1 {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + opp00 {
> + opp-hz = /bits/ 64 <408000000>;
> + opp-microvolt = <825000 825000 1150000>;
> + clock-latency-ns = <40000>;
> + };
> + opp01 {
> + opp-hz = /bits/ 64 <600000000>;
> + opp-microvolt = <825000 825000 1150000>;
> + };
> + opp02 {
> + opp-hz = /bits/ 64 <816000000>;
> + opp-microvolt = <825000 825000 1150000>;
> + };
> + opp03 {
> + opp-hz = /bits/ 64 <1008000000>;
> + opp-microvolt = <875000 875000 1150000>;
> + };
> + opp04 {
> + opp-hz = /bits/ 64 <1200000000>;
> + opp-microvolt = <950000 950000 1150000>;
> + };
> + opp05 {
> + opp-hz = /bits/ 64 <1416000000>;
> + opp-microvolt = <1025000 1025000 1150000>;
> + };
> + opp06 {
> + opp-hz = /bits/ 64 <1500000000>;
> + opp-microvolt = <1100000 1100000 1150000>;
> + };
> + };
> +
> + gpu_opp_table: opp-table-2 {
> + compatible = "operating-points-v2";
> +
> + 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>;
> + };
> + };
> +};
> +
> +&cpu_l0 {
> + operating-points-v2 = <&cluster0_opp>;
> +};
> +
> +&cpu_l1 {
> + operating-points-v2 = <&cluster0_opp>;
> +};
> +
> +&cpu_l2 {
> + operating-points-v2 = <&cluster0_opp>;
> +};
> +
> +&cpu_l3 {
> + operating-points-v2 = <&cluster0_opp>;
> +};
> +
> +&cpu_b0 {
> + operating-points-v2 = <&cluster1_opp>;
> +};
> +
> +&cpu_b1 {
> + operating-points-v2 = <&cluster1_opp>;
> +};
> +
> +&gpu {
> + operating-points-v2 = <&gpu_opp_table>;
> +};
> --
> 2.37.1
>
Hi,
On 8/8/22, Ondřej Jirman <[email protected]> wrote:
> Hi,
>
> On Sat, Aug 06, 2022 at 09:44:09AM +1000, Tom Fitzhenry wrote:
>> From: Samuel Dionne-Riel <[email protected]>
>>
>> These tables were derived from the regular RK3399 table, by dropping
>> entries exceeding recommended operating conditions from the datasheet,
>> and clamping the last exceeding value where it made sense.
>
> Do we really need to duplicate the whole OPP table of rk3399-opp.dtsi
> just to disable a few top opp## entries?
>
> This will make it more annoying to add PVTM/eFuse leakage based voltage
> selection support later on, because then there would have to be identical
> multi-level operating points across multiple files. And that sounds like
> a LOT of dupplication for little benefit.
>
> Also Pinephone Pro has RK3399S not -T. RK3399 seems to be RK3399 selected
> for
> low leakage (values I've seen from eFuses indicate the leakage is half that
> of
> RK3399 available in Pinebook Pro)
The vendor (PINE64) asked me to make these changes while stating specifically
that the Pinephone Pro uses the RK3399-T. Though earlier units and current
batches use the RK3399[s], the design was reportedly made with the RK3399-T
in mind.
The device was also designed to use the OPP from the RK3399-T on RK3399[s]
for the designed thermal operation of the device, reportedly.
> I'd suggest just adding references to select operating point nodes that
> are "too much" and disabling them with status = "disabled". This
> can be done from the pinephone device tree file directly.
>
> Otherwise we'll eventually end up with several files containing
> something like this [1] and only differing in absence of some opp## nodes
> and not their actual useful content.
As to why this is a new file? I have assumed it would be preferable since
this is how it was done for the "OP1" variant of the RK3399. I will defer to
the Rockchip and ARM maintainers to determine which way is better.
I will note that in practice I agree here.
Cheers,
> [1]
> https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk3399-opp.dtsi#L6
>
> kind regards,
> o.
>
>> Signed-off-by: Samuel Dionne-Riel <[email protected]>
>> ---
>> .../arm64/boot/dts/rockchip/rk3399-t-opp.dtsi | 118 ++++++++++++++++++
>> 1 file changed, 118 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
>> b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
>> new file mode 100644
>> index 0000000000000..ec153015d9d13
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-t-opp.dtsi
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
>> + * Copyright (c) 2022 Samuel Dionne-Riel <[email protected]>
>> + */
>> +
>> +/ {
>> + cluster0_opp: opp-table-0 {
>> + compatible = "operating-points-v2";
>> + opp-shared;
>> +
>> + opp00 {
>> + opp-hz = /bits/ 64 <408000000>;
>> + opp-microvolt = <825000 825000 925000>;
>> + clock-latency-ns = <40000>;
>> + };
>> + opp01 {
>> + opp-hz = /bits/ 64 <600000000>;
>> + opp-microvolt = <825000 825000 925000>;
>> + };
>> + opp02 {
>> + opp-hz = /bits/ 64 <816000000>;
>> + opp-microvolt = <850000 850000 925000>;
>> + };
>> + opp03 {
>> + opp-hz = /bits/ 64 <1008000000>;
>> + opp-microvolt = <925000 925000 925000>;
>> + };
>> + };
>> +
>> + cluster1_opp: opp-table-1 {
>> + compatible = "operating-points-v2";
>> + opp-shared;
>> +
>> + opp00 {
>> + opp-hz = /bits/ 64 <408000000>;
>> + opp-microvolt = <825000 825000 1150000>;
>> + clock-latency-ns = <40000>;
>> + };
>> + opp01 {
>> + opp-hz = /bits/ 64 <600000000>;
>> + opp-microvolt = <825000 825000 1150000>;
>> + };
>> + opp02 {
>> + opp-hz = /bits/ 64 <816000000>;
>> + opp-microvolt = <825000 825000 1150000>;
>> + };
>> + opp03 {
>> + opp-hz = /bits/ 64 <1008000000>;
>> + opp-microvolt = <875000 875000 1150000>;
>> + };
>> + opp04 {
>> + opp-hz = /bits/ 64 <1200000000>;
>> + opp-microvolt = <950000 950000 1150000>;
>> + };
>> + opp05 {
>> + opp-hz = /bits/ 64 <1416000000>;
>> + opp-microvolt = <1025000 1025000 1150000>;
>> + };
>> + opp06 {
>> + opp-hz = /bits/ 64 <1500000000>;
>> + opp-microvolt = <1100000 1100000 1150000>;
>> + };
>> + };
>> +
>> + gpu_opp_table: opp-table-2 {
>> + compatible = "operating-points-v2";
>> +
>> + 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>;
>> + };
>> + };
>> +};
>> +
>> +&cpu_l0 {
>> + operating-points-v2 = <&cluster0_opp>;
>> +};
>> +
>> +&cpu_l1 {
>> + operating-points-v2 = <&cluster0_opp>;
>> +};
>> +
>> +&cpu_l2 {
>> + operating-points-v2 = <&cluster0_opp>;
>> +};
>> +
>> +&cpu_l3 {
>> + operating-points-v2 = <&cluster0_opp>;
>> +};
>> +
>> +&cpu_b0 {
>> + operating-points-v2 = <&cluster1_opp>;
>> +};
>> +
>> +&cpu_b1 {
>> + operating-points-v2 = <&cluster1_opp>;
>> +};
>> +
>> +&gpu {
>> + operating-points-v2 = <&gpu_opp_table>;
>> +};
>> --
>> 2.37.1
>>
>
--
— Samuel Dionne-Riel
On Sat, 06 Aug 2022 09:44:10 +1000, Tom Fitzhenry wrote:
> Document board compatible names for Pine64 PinePhonePro.
>
> https://wiki.pine64.org/wiki/PinePhone_Pro
>
> Signed-off-by: Tom Fitzhenry <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
Acked-by: Rob Herring <[email protected]>