[v2] arm64: dts: rockchip: add ROCK 5B+ support

[PATCH v2 2/5] arm64: dts: rockchip: move rock 5b to include file

Posted by Sebastian Reichel 7 months, 1 week ago

Radxa released some more boards, which are based on the original
Rock 5B. Move its board description into an include file to avoid
unnecessary duplication.

NOTE: this should be merged with the previous commit to ensure
bisectability. The rename happens in a separete commit during
development because git does not properly detect the rename when
the original filename is reused in the same commit. This means

1. it's a lot harder to review the changes
2. it's a lot harder to rebase the patch series

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts  | 52 ++++++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi | 40 ------------------
 2 files changed, 52 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
new file mode 100644
index 0000000000000000000000000000000000000000..9407a7c9910ada1f6c803d2e15785a9cbd9bd655
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+/dts-v1/;
+
+#include "rk3588-rock-5b.dtsi"
+
+/ {
+	model = "Radxa ROCK 5B";
+	compatible = "radxa,rock-5b", "rockchip,rk3588";
+};
+
+&sdio {
+	max-frequency = <200000000>;
+	no-sd;
+	no-mmc;
+	non-removable;
+	bus-width = <4>;
+	cap-sdio-irq;
+	disable-wp;
+	keep-power-in-suspend;
+	wakeup-source;
+	sd-uhs-sdr12;
+	sd-uhs-sdr25;
+	sd-uhs-sdr50;
+	sd-uhs-sdr104;
+	vmmc-supply = <&vcc3v3_pcie2x1l0>;
+	vqmmc-supply = <&vcc_1v8_s3>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&sdiom0_pins>;
+	status = "okay";
+};
+
+&uart6 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
+	status = "okay";
+};
+
+&pinctrl {
+	usb {
+		vcc5v0_host_en: vcc5v0-host-en {
+			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+};
+
+&vcc5v0_host {
+	enable-active-high;
+	gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&vcc5v0_host_en>;
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
index 17f4fd054cd3d1c4e23ccfe014a9c4b9d7ad1a06..6052787d2560978d2bae6cfbeea5fc1d419d583a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
@@ -8,9 +8,6 @@
 #include "rk3588.dtsi"
 
 / {
-	model = "Radxa ROCK 5B";
-	compatible = "radxa,rock-5b", "rockchip,rk3588";
-
 	aliases {
 		mmc0 = &sdhci;
 		mmc1 = &sdmmc;
@@ -139,10 +136,6 @@ vcc5v0_host: regulator-vcc5v0-host {
 		regulator-always-on;
 		regulator-min-microvolt = <5000000>;
 		regulator-max-microvolt = <5000000>;
-		enable-active-high;
-		gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&vcc5v0_host_en>;
 		vin-supply = <&vcc5v0_sys>;
 	};
 
@@ -488,12 +481,6 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
 			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
 	};
-
-	usb {
-		vcc5v0_host_en: vcc5v0-host-en {
-			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
 };
 
 &pwm1 {
@@ -530,27 +517,6 @@ &sdmmc {
 	status = "okay";
 };
 
-&sdio {
-	max-frequency = <200000000>;
-	no-sd;
-	no-mmc;
-	non-removable;
-	bus-width = <4>;
-	cap-sdio-irq;
-	disable-wp;
-	keep-power-in-suspend;
-	wakeup-source;
-	sd-uhs-sdr12;
-	sd-uhs-sdr25;
-	sd-uhs-sdr50;
-	sd-uhs-sdr104;
-	vmmc-supply = <&vcc3v3_pcie2x1l0>;
-	vqmmc-supply = <&vcc_1v8_s3>;
-	pinctrl-names = "default";
-	pinctrl-0 = <&sdiom0_pins>;
-	status = "okay";
-};
-
 &sfc {
 	pinctrl-names = "default";
 	pinctrl-0 = <&fspim2_pins>;
@@ -566,12 +532,6 @@ flash@0 {
 	};
 };
 
-&uart6 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
-	status = "okay";
-};
-
 &spi2 {
 	status = "okay";
 	assigned-clocks = <&cru CLK_SPI2>;

-- 
2.47.2

Re: [PATCH v2 2/5] arm64: dts: rockchip: move rock 5b to include file

Posted by Diederik de Haas 7 months, 1 week ago

Hi,

On Thu May 8, 2025 at 7:48 PM CEST, Sebastian Reichel wrote:
> Radxa released some more boards, which are based on the original
> Rock 5B. Move its board description into an include file to avoid
> unnecessary duplication.

Aren't you moving it *out of* an/the include file?
If so, the patch Subject and the above line should be updated so that
they correctly reflect what is changed in this patch.

The above text is correct (and the same ...) as patch 1, but in this
patch you move things out of the dtsi which are unique per board.

> NOTE: this should be merged with the previous commit to ensure
> bisectability. The rename happens in a separete commit during
> development because git does not properly detect the rename when
> the original filename is reused in the same commit. This means
>
> 1. it's a lot harder to review the changes
> 2. it's a lot harder to rebase the patch series

Or did I fall prey to the exact thing you described here?

Cheers,
  Diederik

> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts  | 52 ++++++++++++++++++++++++
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi | 40 ------------------
>  2 files changed, 52 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> new file mode 100644
> index 0000000000000000000000000000000000000000..9407a7c9910ada1f6c803d2e15785a9cbd9bd655
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +/dts-v1/;
> +
> +#include "rk3588-rock-5b.dtsi"
> +
> +/ {
> +	model = "Radxa ROCK 5B";
> +	compatible = "radxa,rock-5b", "rockchip,rk3588";
> +};
> +
> +&sdio {
> +	max-frequency = <200000000>;
> +	no-sd;
> +	no-mmc;
> +	non-removable;
> +	bus-width = <4>;
> +	cap-sdio-irq;
> +	disable-wp;
> +	keep-power-in-suspend;
> +	wakeup-source;
> +	sd-uhs-sdr12;
> +	sd-uhs-sdr25;
> +	sd-uhs-sdr50;
> +	sd-uhs-sdr104;
> +	vmmc-supply = <&vcc3v3_pcie2x1l0>;
> +	vqmmc-supply = <&vcc_1v8_s3>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdiom0_pins>;
> +	status = "okay";
> +};
> +
> +&uart6 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
> +	status = "okay";
> +};
> +
> +&pinctrl {
> +	usb {
> +		vcc5v0_host_en: vcc5v0-host-en {
> +			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +};
> +
> +&vcc5v0_host {
> +	enable-active-high;
> +	gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&vcc5v0_host_en>;
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> index 17f4fd054cd3d1c4e23ccfe014a9c4b9d7ad1a06..6052787d2560978d2bae6cfbeea5fc1d419d583a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> @@ -8,9 +8,6 @@
>  #include "rk3588.dtsi"
>  
>  / {
> -	model = "Radxa ROCK 5B";
> -	compatible = "radxa,rock-5b", "rockchip,rk3588";
> -
>  	aliases {
>  		mmc0 = &sdhci;
>  		mmc1 = &sdmmc;
> @@ -139,10 +136,6 @@ vcc5v0_host: regulator-vcc5v0-host {
>  		regulator-always-on;
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
> -		enable-active-high;
> -		gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&vcc5v0_host_en>;
>  		vin-supply = <&vcc5v0_sys>;
>  	};
>  
> @@ -488,12 +481,6 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>  			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>  		};
>  	};
> -
> -	usb {
> -		vcc5v0_host_en: vcc5v0-host-en {
> -			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> -		};
> -	};
>  };
>  
>  &pwm1 {
> @@ -530,27 +517,6 @@ &sdmmc {
>  	status = "okay";
>  };
>  
> -&sdio {
> -	max-frequency = <200000000>;
> -	no-sd;
> -	no-mmc;
> -	non-removable;
> -	bus-width = <4>;
> -	cap-sdio-irq;
> -	disable-wp;
> -	keep-power-in-suspend;
> -	wakeup-source;
> -	sd-uhs-sdr12;
> -	sd-uhs-sdr25;
> -	sd-uhs-sdr50;
> -	sd-uhs-sdr104;
> -	vmmc-supply = <&vcc3v3_pcie2x1l0>;
> -	vqmmc-supply = <&vcc_1v8_s3>;
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&sdiom0_pins>;
> -	status = "okay";
> -};
> -
>  &sfc {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&fspim2_pins>;
> @@ -566,12 +532,6 @@ flash@0 {
>  	};
>  };
>  
> -&uart6 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
> -	status = "okay";
> -};
> -
>  &spi2 {
>  	status = "okay";
>  	assigned-clocks = <&cru CLK_SPI2>;

Re: [PATCH v2 2/5] arm64: dts: rockchip: move rock 5b to include file

Posted by Heiko Stübner 7 months, 1 week ago

Am Freitag, 9. Mai 2025, 14:44:57 Mitteleuropäische Sommerzeit schrieb Diederik de Haas:
> Hi,
> 
> On Thu May 8, 2025 at 7:48 PM CEST, Sebastian Reichel wrote:
> > Radxa released some more boards, which are based on the original
> > Rock 5B. Move its board description into an include file to avoid
> > unnecessary duplication.
> 
> Aren't you moving it *out of* an/the include file?
> If so, the patch Subject and the above line should be updated so that
> they correctly reflect what is changed in this patch.
> 
> The above text is correct (and the same ...) as patch 1, but in this
> patch you move things out of the dtsi which are unique per board.
> 
> > NOTE: this should be merged with the previous commit to ensure
> > bisectability. The rename happens in a separete commit during
> > development because git does not properly detect the rename when
> > the original filename is reused in the same commit. This means
> >
> > 1. it's a lot harder to review the changes
> > 2. it's a lot harder to rebase the patch series
> 
> Or did I fall prey to the exact thing you described here?

I think Sebastian's idea  is, that I squash both patches when applying.
This split makes it easy(er) to review because patch1 is just a rename.

And merging them when applying then makes it again not break bisectability.


Heiko


> Cheers,
>   Diederik
> 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts  | 52 ++++++++++++++++++++++++
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi | 40 ------------------
> >  2 files changed, 52 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..9407a7c9910ada1f6c803d2e15785a9cbd9bd655
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -0,0 +1,52 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +
> > +/dts-v1/;
> > +
> > +#include "rk3588-rock-5b.dtsi"
> > +
> > +/ {
> > +	model = "Radxa ROCK 5B";
> > +	compatible = "radxa,rock-5b", "rockchip,rk3588";
> > +};
> > +
> > +&sdio {
> > +	max-frequency = <200000000>;
> > +	no-sd;
> > +	no-mmc;
> > +	non-removable;
> > +	bus-width = <4>;
> > +	cap-sdio-irq;
> > +	disable-wp;
> > +	keep-power-in-suspend;
> > +	wakeup-source;
> > +	sd-uhs-sdr12;
> > +	sd-uhs-sdr25;
> > +	sd-uhs-sdr50;
> > +	sd-uhs-sdr104;
> > +	vmmc-supply = <&vcc3v3_pcie2x1l0>;
> > +	vqmmc-supply = <&vcc_1v8_s3>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sdiom0_pins>;
> > +	status = "okay";
> > +};
> > +
> > +&uart6 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
> > +	status = "okay";
> > +};
> > +
> > +&pinctrl {
> > +	usb {
> > +		vcc5v0_host_en: vcc5v0-host-en {
> > +			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +	};
> > +};
> > +
> > +&vcc5v0_host {
> > +	enable-active-high;
> > +	gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&vcc5v0_host_en>;
> > +};
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> > index 17f4fd054cd3d1c4e23ccfe014a9c4b9d7ad1a06..6052787d2560978d2bae6cfbeea5fc1d419d583a 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> > @@ -8,9 +8,6 @@
> >  #include "rk3588.dtsi"
> >  
> >  / {
> > -	model = "Radxa ROCK 5B";
> > -	compatible = "radxa,rock-5b", "rockchip,rk3588";
> > -
> >  	aliases {
> >  		mmc0 = &sdhci;
> >  		mmc1 = &sdmmc;
> > @@ -139,10 +136,6 @@ vcc5v0_host: regulator-vcc5v0-host {
> >  		regulator-always-on;
> >  		regulator-min-microvolt = <5000000>;
> >  		regulator-max-microvolt = <5000000>;
> > -		enable-active-high;
> > -		gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> > -		pinctrl-names = "default";
> > -		pinctrl-0 = <&vcc5v0_host_en>;
> >  		vin-supply = <&vcc5v0_sys>;
> >  	};
> >  
> > @@ -488,12 +481,6 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >  			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> >  		};
> >  	};
> > -
> > -	usb {
> > -		vcc5v0_host_en: vcc5v0-host-en {
> > -			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > -		};
> > -	};
> >  };
> >  
> >  &pwm1 {
> > @@ -530,27 +517,6 @@ &sdmmc {
> >  	status = "okay";
> >  };
> >  
> > -&sdio {
> > -	max-frequency = <200000000>;
> > -	no-sd;
> > -	no-mmc;
> > -	non-removable;
> > -	bus-width = <4>;
> > -	cap-sdio-irq;
> > -	disable-wp;
> > -	keep-power-in-suspend;
> > -	wakeup-source;
> > -	sd-uhs-sdr12;
> > -	sd-uhs-sdr25;
> > -	sd-uhs-sdr50;
> > -	sd-uhs-sdr104;
> > -	vmmc-supply = <&vcc3v3_pcie2x1l0>;
> > -	vqmmc-supply = <&vcc_1v8_s3>;
> > -	pinctrl-names = "default";
> > -	pinctrl-0 = <&sdiom0_pins>;
> > -	status = "okay";
> > -};
> > -
> >  &sfc {
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&fspim2_pins>;
> > @@ -566,12 +532,6 @@ flash@0 {
> >  	};
> >  };
> >  
> > -&uart6 {
> > -	pinctrl-names = "default";
> > -	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
> > -	status = "okay";
> > -};
> > -
> >  &spi2 {
> >  	status = "okay";
> >  	assigned-clocks = <&cru CLK_SPI2>;
> 
>

Re: [PATCH v2 2/5] arm64: dts: rockchip: move rock 5b to include file

Posted by Sebastian Reichel 7 months, 1 week ago

Hi,

On Fri, May 09, 2025 at 02:54:00PM +0200, Heiko Stübner wrote:
> Am Freitag, 9. Mai 2025, 14:44:57 Mitteleuropäische Sommerzeit schrieb Diederik de Haas:
> > Hi,
> > 
> > On Thu May 8, 2025 at 7:48 PM CEST, Sebastian Reichel wrote:
> > > Radxa released some more boards, which are based on the original
> > > Rock 5B. Move its board description into an include file to avoid
> > > unnecessary duplication.
> > 
> > Aren't you moving it *out of* an/the include file?
> > If so, the patch Subject and the above line should be updated so that
> > they correctly reflect what is changed in this patch.
> > 
> > The above text is correct (and the same ...) as patch 1, but in this
> > patch you move things out of the dtsi which are unique per board.
> > 
> > > NOTE: this should be merged with the previous commit to ensure
> > > bisectability. The rename happens in a separete commit during
> > > development because git does not properly detect the rename when
> > > the original filename is reused in the same commit. This means
> > >
> > > 1. it's a lot harder to review the changes
> > > 2. it's a lot harder to rebase the patch series
> > 
> > Or did I fall prey to the exact thing you described here?
> 
> I think Sebastian's idea  is, that I squash both patches when applying.
> This split makes it easy(er) to review because patch1 is just a rename.
> 
> And merging them when applying then makes it again not break bisectability.

Correct. This is a lot easier to review than what git generates when
having these two patches squashed together, which is a huge diff of
all 1000+ lines in the file (I tried really hard to convince it that
this is mostly a rename with --find-renames and --find-copies). You
can see this kind of mess in patch 2 of the ROCK 5T series that
Nicolas just send (I will comment on that and suggest to do the same
thing I did to ease review. In his case it should even be possible
to do it in a bisectable way without needing a squash :)).

Greetings,

-- Sebastian

> 
> 
> Heiko
> 
> 
> > Cheers,
> >   Diederik
> > 
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts  | 52 ++++++++++++++++++++++++
> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi | 40 ------------------
> > >  2 files changed, 52 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..9407a7c9910ada1f6c803d2e15785a9cbd9bd655
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > @@ -0,0 +1,52 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "rk3588-rock-5b.dtsi"
> > > +
> > > +/ {
> > > +	model = "Radxa ROCK 5B";
> > > +	compatible = "radxa,rock-5b", "rockchip,rk3588";
> > > +};
> > > +
> > > +&sdio {
> > > +	max-frequency = <200000000>;
> > > +	no-sd;
> > > +	no-mmc;
> > > +	non-removable;
> > > +	bus-width = <4>;
> > > +	cap-sdio-irq;
> > > +	disable-wp;
> > > +	keep-power-in-suspend;
> > > +	wakeup-source;
> > > +	sd-uhs-sdr12;
> > > +	sd-uhs-sdr25;
> > > +	sd-uhs-sdr50;
> > > +	sd-uhs-sdr104;
> > > +	vmmc-supply = <&vcc3v3_pcie2x1l0>;
> > > +	vqmmc-supply = <&vcc_1v8_s3>;
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&sdiom0_pins>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&uart6 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&pinctrl {
> > > +	usb {
> > > +		vcc5v0_host_en: vcc5v0-host-en {
> > > +			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > > +		};
> > > +	};
> > > +};
> > > +
> > > +&vcc5v0_host {
> > > +	enable-active-high;
> > > +	gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&vcc5v0_host_en>;
> > > +};
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> > > index 17f4fd054cd3d1c4e23ccfe014a9c4b9d7ad1a06..6052787d2560978d2bae6cfbeea5fc1d419d583a 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
> > > @@ -8,9 +8,6 @@
> > >  #include "rk3588.dtsi"
> > >  
> > >  / {
> > > -	model = "Radxa ROCK 5B";
> > > -	compatible = "radxa,rock-5b", "rockchip,rk3588";
> > > -
> > >  	aliases {
> > >  		mmc0 = &sdhci;
> > >  		mmc1 = &sdmmc;
> > > @@ -139,10 +136,6 @@ vcc5v0_host: regulator-vcc5v0-host {
> > >  		regulator-always-on;
> > >  		regulator-min-microvolt = <5000000>;
> > >  		regulator-max-microvolt = <5000000>;
> > > -		enable-active-high;
> > > -		gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> > > -		pinctrl-names = "default";
> > > -		pinctrl-0 = <&vcc5v0_host_en>;
> > >  		vin-supply = <&vcc5v0_sys>;
> > >  	};
> > >  
> > > @@ -488,12 +481,6 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> > >  			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> > >  		};
> > >  	};
> > > -
> > > -	usb {
> > > -		vcc5v0_host_en: vcc5v0-host-en {
> > > -			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > > -		};
> > > -	};
> > >  };
> > >  
> > >  &pwm1 {
> > > @@ -530,27 +517,6 @@ &sdmmc {
> > >  	status = "okay";
> > >  };
> > >  
> > > -&sdio {
> > > -	max-frequency = <200000000>;
> > > -	no-sd;
> > > -	no-mmc;
> > > -	non-removable;
> > > -	bus-width = <4>;
> > > -	cap-sdio-irq;
> > > -	disable-wp;
> > > -	keep-power-in-suspend;
> > > -	wakeup-source;
> > > -	sd-uhs-sdr12;
> > > -	sd-uhs-sdr25;
> > > -	sd-uhs-sdr50;
> > > -	sd-uhs-sdr104;
> > > -	vmmc-supply = <&vcc3v3_pcie2x1l0>;
> > > -	vqmmc-supply = <&vcc_1v8_s3>;
> > > -	pinctrl-names = "default";
> > > -	pinctrl-0 = <&sdiom0_pins>;
> > > -	status = "okay";
> > > -};
> > > -
> > >  &sfc {
> > >  	pinctrl-names = "default";
> > >  	pinctrl-0 = <&fspim2_pins>;
> > > @@ -566,12 +532,6 @@ flash@0 {
> > >  	};
> > >  };
> > >  
> > > -&uart6 {
> > > -	pinctrl-names = "default";
> > > -	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
> > > -	status = "okay";
> > > -};
> > > -
> > >  &spi2 {
> > >  	status = "okay";
> > >  	assigned-clocks = <&cru CLK_SPI2>;
> > 
> > 
> 
> 
> 
>

Re: [PATCH v2 2/5] arm64: dts: rockchip: move rock 5b to include file

Posted by Diederik de Haas 7 months, 1 week ago

Hi,

On Fri May 9, 2025 at 3:08 PM CEST, Sebastian Reichel wrote:
> On Fri, May 09, 2025 at 02:54:00PM +0200, Heiko Stübner wrote:
>> Am Freitag, 9. Mai 2025, 14:44:57 Mitteleuropäische Sommerzeit schrieb Diederik de Haas:
>> > On Thu May 8, 2025 at 7:48 PM CEST, Sebastian Reichel wrote:
>> > > Radxa released some more boards, which are based on the original
>> > > Rock 5B. Move its board description into an include file to avoid
>> > > unnecessary duplication.
>> > 
>> > Aren't you moving it *out of* an/the include file?
>> > If so, the patch Subject and the above line should be updated so that
>> > they correctly reflect what is changed in this patch.
>> > 
>> > The above text is correct (and the same ...) as patch 1, but in this
>> > patch you move things out of the dtsi which are unique per board.
>> > 
>> > > NOTE: this should be merged with the previous commit to ensure
>> > > bisectability. The rename happens in a separete commit during
>> > > development because git does not properly detect the rename when
>> > > the original filename is reused in the same commit. This means
>> > >
>> > > 1. it's a lot harder to review the changes
>> > > 2. it's a lot harder to rebase the patch series
>> > 
>> > Or did I fall prey to the exact thing you described here?
>> 
>> I think Sebastian's idea  is, that I squash both patches when applying.
>> This split makes it easy(er) to review because patch1 is just a rename.
>> 
>> And merging them when applying then makes it again not break bisectability.
>
> Correct. This is a lot easier to review than what git generates when
> having these two patches squashed together, which is a huge diff of
> all 1000+ lines in the file (I tried really hard to convince it that
> this is mostly a rename with --find-renames and --find-copies). You
> can see this kind of mess in patch 2 of the ROCK 5T series that
> Nicolas just send (I will comment on that and suggest to do the same
> thing I did to ease review. In his case it should even be possible
> to do it in a bisectable way without needing a squash :)).

Thank you both for the clarification :-)

Cheers,
  Diederik

>> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>> > > ---
>> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts  | 52 ++++++++++++++++++++++++
>> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi | 40 ------------------
>> > >  2 files changed, 52 insertions(+), 40 deletions(-)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> > > new file mode 100644
>> > > index 0000000000000000000000000000000000000000..9407a7c9910ada1f6c803d2e15785a9cbd9bd655
>> > > --- /dev/null
>> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> > > @@ -0,0 +1,52 @@
>> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> > > +
>> > > +/dts-v1/;
>> > > +
>> > > +#include "rk3588-rock-5b.dtsi"
>> > > +
>> > > +/ {
>> > > +	model = "Radxa ROCK 5B";
>> > > +	compatible = "radxa,rock-5b", "rockchip,rk3588";
>> > > +};
>> > > +
>> > > +&sdio {
>> > > +	max-frequency = <200000000>;
>> > > +	no-sd;
>> > > +	no-mmc;
>> > > +	non-removable;
>> > > +	bus-width = <4>;
>> > > +	cap-sdio-irq;
>> > > +	disable-wp;
>> > > +	keep-power-in-suspend;
>> > > +	wakeup-source;
>> > > +	sd-uhs-sdr12;
>> > > +	sd-uhs-sdr25;
>> > > +	sd-uhs-sdr50;
>> > > +	sd-uhs-sdr104;
>> > > +	vmmc-supply = <&vcc3v3_pcie2x1l0>;
>> > > +	vqmmc-supply = <&vcc_1v8_s3>;
>> > > +	pinctrl-names = "default";
>> > > +	pinctrl-0 = <&sdiom0_pins>;
>> > > +	status = "okay";
>> > > +};
>> > > +
>> > > +&uart6 {
>> > > +	pinctrl-names = "default";
>> > > +	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
>> > > +	status = "okay";
>> > > +};
>> > > +
>> > > +&pinctrl {
>> > > +	usb {
>> > > +		vcc5v0_host_en: vcc5v0-host-en {
>> > > +			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
>> > > +		};
>> > > +	};
>> > > +};
>> > > +
>> > > +&vcc5v0_host {
>> > > +	enable-active-high;
>> > > +	gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
>> > > +	pinctrl-names = "default";
>> > > +	pinctrl-0 = <&vcc5v0_host_en>;
>> > > +};
>> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
>> > > index 17f4fd054cd3d1c4e23ccfe014a9c4b9d7ad1a06..6052787d2560978d2bae6cfbeea5fc1d419d583a 100644
>> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
>> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtsi
>> > > @@ -8,9 +8,6 @@
>> > >  #include "rk3588.dtsi"
>> > >  
>> > >  / {
>> > > -	model = "Radxa ROCK 5B";
>> > > -	compatible = "radxa,rock-5b", "rockchip,rk3588";
>> > > -
>> > >  	aliases {
>> > >  		mmc0 = &sdhci;
>> > >  		mmc1 = &sdmmc;
>> > > @@ -139,10 +136,6 @@ vcc5v0_host: regulator-vcc5v0-host {
>> > >  		regulator-always-on;
>> > >  		regulator-min-microvolt = <5000000>;
>> > >  		regulator-max-microvolt = <5000000>;
>> > > -		enable-active-high;
>> > > -		gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
>> > > -		pinctrl-names = "default";
>> > > -		pinctrl-0 = <&vcc5v0_host_en>;
>> > >  		vin-supply = <&vcc5v0_sys>;
>> > >  	};
>> > >  
>> > > @@ -488,12 +481,6 @@ pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>> > >  			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>> > >  		};
>> > >  	};
>> > > -
>> > > -	usb {
>> > > -		vcc5v0_host_en: vcc5v0-host-en {
>> > > -			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
>> > > -		};
>> > > -	};
>> > >  };
>> > >  
>> > >  &pwm1 {
>> > > @@ -530,27 +517,6 @@ &sdmmc {
>> > >  	status = "okay";
>> > >  };
>> > >  
>> > > -&sdio {
>> > > -	max-frequency = <200000000>;
>> > > -	no-sd;
>> > > -	no-mmc;
>> > > -	non-removable;
>> > > -	bus-width = <4>;
>> > > -	cap-sdio-irq;
>> > > -	disable-wp;
>> > > -	keep-power-in-suspend;
>> > > -	wakeup-source;
>> > > -	sd-uhs-sdr12;
>> > > -	sd-uhs-sdr25;
>> > > -	sd-uhs-sdr50;
>> > > -	sd-uhs-sdr104;
>> > > -	vmmc-supply = <&vcc3v3_pcie2x1l0>;
>> > > -	vqmmc-supply = <&vcc_1v8_s3>;
>> > > -	pinctrl-names = "default";
>> > > -	pinctrl-0 = <&sdiom0_pins>;
>> > > -	status = "okay";
>> > > -};
>> > > -
>> > >  &sfc {
>> > >  	pinctrl-names = "default";
>> > >  	pinctrl-0 = <&fspim2_pins>;
>> > > @@ -566,12 +532,6 @@ flash@0 {
>> > >  	};
>> > >  };
>> > >  
>> > > -&uart6 {
>> > > -	pinctrl-names = "default";
>> > > -	pinctrl-0 = <&uart6m1_xfer &uart6m1_ctsn &uart6m1_rtsn>;
>> > > -	status = "okay";
>> > > -};
>> > > -
>> > >  &spi2 {
>> > >  	status = "okay";
>> > >  	assigned-clocks = <&cru CLK_SPI2>;
>> > 
>> > 
>> 
>> 
>> 
>>

[PATCH v2 1/5] arm64: dts: rockchip: move rock 5b to include file
[PATCH v2 2/5] arm64: dts: rockchip: move rock 5b to include file
[PATCH v2 3/5] dt-bindings: arm: rockchip: Add Radxa ROCK 5B+
[PATCH v2 4/5] arm64: dts: rockchip: add Rock 5B+
[PATCH v2 5/5] arm64: dts: rockchip: add USB-C support for ROCK 5B+