Re: [PATCH v2] rockchip: rk3588s: Add support for ROC-PC-RK3588S

[Quentin Schulz via U-Boot <u-boot@lists.u-boot-project.org>]

Hi Fabio,

On 7/22/26 3:27 PM, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@nabladev.com>
> 
> The ROC-PC-RK3588S board is an SBC made by Firefly, based on the RK3588S
> SoC.
> 
> Add support for it.
> 
> Signed-off-by: Fabio Estevam <festevam@nabladev.com>
> ---
> Changes since v1:
> - Use the traditional environment offsets of 0x3f8000 and 0x400000.
> - Keep clock-names in the SPL device tree.
> - Drop CONFIG_RTL8169 as the onboard RTL8211F uses CONFIG_PHY_REALTEK.
> - Add the accepted upstream USB device-mode changes to
>    rk3588s-roc-pc-u-boot.dtsi pending the next devicetree-rebasing sync:
>    https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=9d1e32bfdda1aef3291e47c2949b3b9978162a5c
> 
>   arch/arm/dts/rk3588s-roc-pc-u-boot.dtsi  | 13 ++++
>   arch/arm/mach-rockchip/rk3588/Kconfig    |  7 ++
>   board/firefly/roc-pc-rk3588s/Kconfig     | 12 ++++
>   board/firefly/roc-pc-rk3588s/MAINTAINERS |  7 ++
>   configs/roc-pc-rk3588s_defconfig         | 90 ++++++++++++++++++++++++
>   doc/board/rockchip/rockchip.rst          |  1 +
>   include/configs/roc-pc-rk3588s.h         |  8 +++
>   7 files changed, 138 insertions(+)
>   create mode 100644 arch/arm/dts/rk3588s-roc-pc-u-boot.dtsi
>   create mode 100644 board/firefly/roc-pc-rk3588s/Kconfig
>   create mode 100644 board/firefly/roc-pc-rk3588s/MAINTAINERS
>   create mode 100644 configs/roc-pc-rk3588s_defconfig
>   create mode 100644 include/configs/roc-pc-rk3588s.h
> 
> diff --git a/arch/arm/dts/rk3588s-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3588s-roc-pc-u-boot.dtsi
> new file mode 100644
> index 000000000000..7d97f64ec7a9
> --- /dev/null
> +++ b/arch/arm/dts/rk3588s-roc-pc-u-boot.dtsi
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +#include "rk3588s-u-boot.dtsi"
> +
> +&usb_host0_xhci {
> +	role-switch-default-mode = "peripheral";
> +	usb-role-switch;
> +	status = "okay";
> +};
> +
> +&usbdp_phy0 {
> +	status = "okay";
> +};

This is not fixing an important bug, so I would rather we wait for it to 
appear in devicetree-rebasing and then cherry-pick it (or wait for it to 
eventually make it to U-Boot during one of the dts/upstream sync in a 
few months).

Ideally, we shouldn't also enable drivers in the defconfig for things 
that aren't available (yet) in the Device Tree as to not mislead the 
user as to what's currently supported by U-Boot for this device. 
Dropping the above Deivce Tree changes would mean USBDP and OTG-related 
symbols would better be removed from the defconfig as well.

> diff --git a/arch/arm/mach-rockchip/rk3588/Kconfig b/arch/arm/mach-rockchip/rk3588/Kconfig
> index 675f13ab66d7..841ae490e85a 100644
> --- a/arch/arm/mach-rockchip/rk3588/Kconfig
> +++ b/arch/arm/mach-rockchip/rk3588/Kconfig
> @@ -208,6 +208,12 @@ config TARGET_RK3588_NEU6
>   	  IO board and Neu6a needs to mount on top of this IO board in order to
>   	  create complete Edgeble Neural Compute Module 6B(Neu6B) IO platform.
>   
> +config TARGET_ROC_PC_RK3588S
> +	bool "Firefly ROC PC RK3588S"
> +	help
> +	  ROC-PC-RK3588S is a single board computer from Firefly
> +	  using the Rockchip RK3588S.
> +
>   config TARGET_ROCK5A_RK3588
>   	bool "Radxa ROCK5A RK3588 board"
>   	help
> @@ -421,6 +427,7 @@ config SYS_SOC
>   source "board/armsom/sige7-rk3588/Kconfig"
>   source "board/coolpi/genbook_cm5_rk3588/Kconfig"
>   source "board/edgeble/neural-compute-module-6/Kconfig"
> +source "board/firefly/roc-pc-rk3588s/Kconfig"
>   source "board/friendlyelec/cm3588-nas-rk3588/Kconfig"
>   source "board/friendlyelec/nanopc-t6-rk3588/Kconfig"
>   source "board/friendlyelec/nanopi-r6c-rk3588s/Kconfig"
> diff --git a/board/firefly/roc-pc-rk3588s/Kconfig b/board/firefly/roc-pc-rk3588s/Kconfig
> new file mode 100644
> index 000000000000..bded7a2fcba1
> --- /dev/null
> +++ b/board/firefly/roc-pc-rk3588s/Kconfig
> @@ -0,0 +1,12 @@
> +if TARGET_ROC_PC_RK3588S
> +
> +config SYS_BOARD
> +	default "roc-pc-rk3588s"
> +
> +config SYS_VENDOR
> +	default "firefly"
> +
> +config SYS_CONFIG_NAME
> +	default "roc-pc-rk3588s"
> +
> +endif
> diff --git a/board/firefly/roc-pc-rk3588s/MAINTAINERS b/board/firefly/roc-pc-rk3588s/MAINTAINERS
> new file mode 100644
> index 000000000000..9a2bb4e69e89
> --- /dev/null
> +++ b/board/firefly/roc-pc-rk3588s/MAINTAINERS
> @@ -0,0 +1,7 @@
> +ROC-PC-RK3588S
> +M:	Fabio Estevam <festevam@nabladev.com>
> +S:	Maintained
> +F:	arch/arm/dts/rk3588s-roc-pc-u-boot.dtsi
> +F:	board/firefly/roc-pc-rk3588s/
> +F:	configs/roc-pc-rk3588s_defconfig
> +F:	include/configs/roc-pc-rk3588s.h
> diff --git a/configs/roc-pc-rk3588s_defconfig b/configs/roc-pc-rk3588s_defconfig
> new file mode 100644
> index 000000000000..0941c3c08236
> --- /dev/null
> +++ b/configs/roc-pc-rk3588s_defconfig
> @@ -0,0 +1,90 @@
> +CONFIG_ARM=y
> +CONFIG_SKIP_LOWLEVEL_INIT=y
> +CONFIG_SYS_HAS_NONCACHED_MEMORY=y
> +CONFIG_COUNTER_FREQUENCY=24000000
> +CONFIG_ARCH_ROCKCHIP=y
> +CONFIG_ENV_OFFSET=0x3F8000

Can you please use 0x3f8000 here (that is, lowercase f). It'll then 
match the default value we specify for Rockchip and not appear in the 
defconfig.

Please also use savedefconfig before sending upstream, 
CONFIG_SYS_HAS_NONCACHED_MEMORY cannot be y for example, as it depends 
on CONFIG_RTL8169=y, which we don't have anymore since v2.

The rest looks good to me, thanks!
Quentin