| Message ID | 20220815123004.252014-3-tom@tom-fitzhenry.me.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the Pine64 PinePhone Pro phone | expand |
On 15/08/2022 13:30, Tom Fitzhenry wrote: > From: Martijn Braam <martijn@brixit.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. > > https://wiki.pine64.org/wiki/PinePhone_Pro > > This is derived from > https://gitlab.com/pine64-org/linux/-/commit/261d3b5f8ac503f97da810986d1d6422430c8531 > with fixes from https://megous.com/git/linux. > > Signed-off-by: Martijn Braam <martijn@brixit.nl> > Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu> > [no SoB, but Kamil is happy for this patch to be submitted] > Co-developed-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Co-developed-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk> > Signed-off-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk> Reviewed-by: Caleb Connolly <kc@postmarketos.org> > --- > arch/arm64/boot/dts/rockchip/Makefile | 1 + > .../dts/rockchip/rk3399-pinephone-pro.dts | 394 ++++++++++++++++++ > 2 files changed, 395 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..f9f1dcd4f6494 > --- /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@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/linux-event-codes.h> > +#include "rk3399.dtsi" > +#include "rk3399-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-keys { > + compatible = "gpio-keys"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwrbtn_pin>; > + > + power { > + debounce-interval = <20>; > + gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>; > + label = "Power"; > + linux,code = <KEY_POWER>; > + wakeup-source; > + }; > + }; > + > + vcc_sysin: vcc-sysin-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "vcc_sysin"; > + regulator-always-on; > + regulator-boot-on; > + }; > + > + vcc3v3_sys: 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; > + }; > +}; > + > +&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 { > + clock-frequency = <400000>; > + i2c-scl-rising-time-ns = <168>; > + i2c-scl-falling-time-ns = <4>; > + status = "okay"; > + > + 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; > + }; > + }; > + > + 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; > + }; > + }; > + > + 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; > + }; > + }; > +}; > + > +&cluster0_opp { > + opp04 { > + status = "disabled"; > + }; > + > + opp05 { > + status = "disabled"; > + }; > +}; > + > +&cluster1_opp { > + opp06 { > + status = "disabled"; > + }; > + > + opp07 { > + status = "disabled"; > + }; > +}; > + > +&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>; > + }; > + }; > +}; > + > +&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"; > +}; > + > +&tsadc { > + rockchip,hw-tshut-mode = <1>; > + rockchip,hw-tshut-polarity = <1>; > + status = "okay"; > +}; > + > +&uart2 { > + status = "okay"; > +}; > -- > 2.37.1 >
On 15/08/2022 15:30, Tom Fitzhenry wrote: > + > +/dts-v1/; > +#include <dt-bindings/input/linux-event-codes.h> > +#include "rk3399.dtsi" > +#include "rk3399-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-keys { > + compatible = "gpio-keys"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwrbtn_pin>; > + > + power { Still wrong. Do not ignore comments and test your DTS with dtbs_check. Best regards, Krzysztof
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@brixit.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@brixit.nl> > + * Copyright (c) 2021 Kamil Trzciński <ayufan@ayufan.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 > + 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)... 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? > + > + 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@nfraprado.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>; > +}; [..]
On Mon, Aug 15, 2022 at 10:30:04PM +1000, Tom Fitzhenry wrote: > From: Martijn Braam <martijn@brixit.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. > > https://wiki.pine64.org/wiki/PinePhone_Pro > > This is derived from > https://gitlab.com/pine64-org/linux/-/commit/261d3b5f8ac503f97da810986d1d6422430c8531 > with fixes from https://megous.com/git/linux. > > Signed-off-by: Martijn Braam <martijn@brixit.nl> > Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu> > [no SoB, but Kamil is happy for this patch to be submitted] > Co-developed-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Co-developed-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk> > Signed-off-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk> Tested-by: Nícolas F. R. A. Prado <n@nfraprado.net> Indeed booting from the SD card also works with this minimal DT. Also confirmed that eMMC, thermal sensors, power button, regulators and UART are all working. Thanks, Nícolas
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@brixit.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@brixit.nl> > > + * Copyright (c) 2021 Kamil Trzciński <ayufan@ayufan.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@nfraprado.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>; > > +}; > [..]
On Sun, Aug 21, 2022 at 11:00:05AM +0200, megi xff wrote: [...] > 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 The minimums on the regulators also seem out of range in the datasheet, for that matter. regards, o.
On 16/8/22 17:52, Krzysztof Kozlowski wrote: > Still wrong. My bad, I will fix this in v4. > test your DTS with dtbs_check. Will do.
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.
On 21/8/22 19:00, Ondřej Jirman wrote: > 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 Good spot, I will fix these in v4.
On Sun, 21 Aug 2022, at 12:11 AM, Nícolas F. R. A. Prado wrote: > Indeed booting from the SD card also works with this minimal DT. Also confirmed > that eMMC, thermal sensors, power button, regulators and UART are all working. Thanks for the testing! I will include your Tested-by in v4.
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..f9f1dcd4f6494 --- /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@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/linux-event-codes.h> +#include "rk3399.dtsi" +#include "rk3399-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-keys { + compatible = "gpio-keys"; + pinctrl-names = "default"; + pinctrl-0 = <&pwrbtn_pin>; + + power { + debounce-interval = <20>; + gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>; + label = "Power"; + linux,code = <KEY_POWER>; + wakeup-source; + }; + }; + + vcc_sysin: vcc-sysin-regulator { + compatible = "regulator-fixed"; + regulator-name = "vcc_sysin"; + regulator-always-on; + regulator-boot-on; + }; + + vcc3v3_sys: 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; + }; +}; + +&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 { + clock-frequency = <400000>; + i2c-scl-rising-time-ns = <168>; + i2c-scl-falling-time-ns = <4>; + status = "okay"; + + 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; + }; + }; + + 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; + }; + }; + + 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; + }; + }; +}; + +&cluster0_opp { + opp04 { + status = "disabled"; + }; + + opp05 { + status = "disabled"; + }; +}; + +&cluster1_opp { + opp06 { + status = "disabled"; + }; + + opp07 { + status = "disabled"; + }; +}; + +&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>; + }; + }; +}; + +&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"; +}; + +&tsadc { + rockchip,hw-tshut-mode = <1>; + rockchip,hw-tshut-polarity = <1>; + status = "okay"; +}; + +&uart2 { + status = "okay"; +};