Re: [PATCH 1/1] arm: dts: rockchip: Add initial support for LinkStar H68K-1432v1 Board with RK3568 SoC

[Heiko Stuebner <heiko@sntech.de>]

Hi Erik,

Am Freitag, 18. Juli 2025, 13:50:58 Mitteleuropäische Sommerzeit schrieb Erik Beck:
> Hello,
> 
> Below please find a patch to three files to provide initial support for
> the LinkStar H68K-1432v1 board with Rockchip rk3568 SOC. 
> 
> This has been tested on the hardware and works well in support of
> router features. Namely, it supports:
> 
> * Two 1 Gbs Ethernet ports
> * Two 2.5 Gbs Ethernet ports
> * Two USB 3.0 Type A sockets
> * One USB 3.0 Type C socket
> * One USB 2.0 Type A socket
> * WiFi
> * LED
> * Debug console
> 
> I look forward to receiving and responding to questions, comments,
> concerns, and critiques.
> 
> Thank you for your reviews.
> 
> Regards,
> 
> Erik

The above test reads more like a cover-letter instead of a commit
message. Please take look at how other board additions are worded.

> Signed-off-by: Erik Beck <xunil@tahomasoft.com>
> Tested-by: Erik Beck <xunil@tahomasoft.com>
> 
>  .../devicetree/bindings/arm/rockchip.yaml          |   5 +
>  arch/arm64/boot/dts/rockchip/Makefile              |   1 +
>  .../dts/rockchip/rk3568-linkstar-h68k-1432v1.dts   | 777

please split this into two commits, one adding the yaml binding and
one adding the board devicetree.

Also please check scripts/get_maintainer.pl to see who to Cc explicitly
(like the devicetree maintainers)


>  +++++++++++++++++++++ 3 files changed, 783 insertions(+)
> 
> 
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml
> b/Documentation/devicetree/bindings/arm/rockchip.yaml index
> 28db6bd6aa5b..e48b168cfffe 100644 ---

please try to use "git send-email" to send patches, because it looks
like your mail program mangled the patch by adding line breaks
(more of them below). So the patch most likely won't apply.


> diff --git
> a/arch/arm64/boot/dts/rockchip/rk3568-linkstar-h68k-1432v1.dts
> b/arch/arm64/boot/dts/rockchip/rk3568-linkstar-h68k-1432v1.dts new file
> mode 100644 index 000000000000..f8f80c7616cd --- /dev/null +++
> b/arch/arm64/boot/dts/rockchip/rk3568-linkstar-h68k-1432v1.dts @@ -0,0
> +1,777 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +// Copyright
> (c) 2022 AmadeusGhost <amadeus@jmu.edu.cn> +//
> +// Copyright (c) 2025 TahomaSoft xunil@tahomasoft.com
> +//
> +// Support for Seeed LinkStar H68K-1432v1
> +// Also likely supports LinkStar H68K-1432v2
> +// NB: Hinlink H68K is same or very similar under different trade name

please use preferred comment style - see other kernel devicetrees.

> +
> +/dts-v1/;
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +#include "rk3568.dtsi"
> +
> +
> +/ {
> +        model = "Seeed LinkStar H68K-1432V1 (RK3568) DDR4 Board";
> +        compatible = "seeed,rk3568-linkstar-h68k-1432v1",
> "rockchip,rk3568"; +
> +        aliases {
> +                ethernet0 = &gmac0;
> +                ethernet1 = &gmac1;
> +
> +                /* fixed eMMC */
> +                mmc0 = &sdhci;  /* sdhci:= @fe310000 */ /* mmcblk0...
> */

no need for the comments before and after the alias, same below

> +
> +                /* removable uSD/TF Card */
> +                mmc1 = &sdmmc0; /* sdmmc0:= @fe2b0000 */ /* mmcblk1...
> */ +
> +                rtc0 = &rk809;
> +
> +               led-boot = &led_one; /* status LED, green */
> +               led-failsafe = &led_three; /* heartbeat LED */
> +               led-running = &led_one; /* status LED, green */
> +               led-upgrade = &led_two; /* function LED, amber */

I don't think those are specified? What is supposed to use those?

> +

unneeded empty line

> +        };
> +

[...]

> +        gpio-keys {
> +                compatible = "gpio-keys";
> +                pinctrl-0 = <&reset_button_pin>;
> +                pinctrl-names = "default";
> +
> +        /* Middle inset/recessed button,
> +                  marked by clockwise arrow/circle */
> +
> +                button-reset { /* gpiochip=0, line=0 */

again unneeded comment ... that content is contained in the gpios
property already

> +                        label = "button:system:reset";
> +                        gpios = <&gpio0 RK_PA0 GPIO_ACTIVE_LOW>;
> +                        linux,code = <KEY_RESTART>;
> +                        debounce-interval = <50>;
> +                };
> +
> +        };
> +
> + /* gpio chip 0, line 24 responds to console key press */
> +
> +        gpio-leds {
> +                compatible = "gpio-leds";
> +                pinctrl-names = "default";
> +                pinctrl-0 = <&led_white_pin>, <&led_green_pin>,
> +                         <&led_amber_pin>, <&led_blue_pin>;
> +
> +                /* White LED inset in power button */

that commment is helpful, so could stay

> +
> +                led_zero: led_white   { /* gpiochip=0, line=14 */

preferred naming would probably be
  led_white: led-0 {}

similar for the other ones

> +                        color = <LED_COLOR_ID_WHITE>;
> +                        default-state = "on";
> +                        function = LED_FUNCTION_POWER;
> +                        gpios = <&gpio0 RK_PB6 GPIO_ACTIVE_HIGH>;
> +                        label = "power_button_led:white_led";
> +                        linux,default-trigger = "default-on";
> +
> +                };
> +
> +        vcc12v_dcin: vcc12v-dcin {

vcc12v_dcin: regulator-vcc12v-dcin {}

same for the others

> +                compatible = "regulator-fixed";
> +                regulator-always-on;
> +                regulator-boot-on;
> +                regulator-min-microvolt = <12000000>;
> +                regulator-max-microvolt = <12000000>;
> +                regulator-name = "vcc12v_dcin";
> +        };
> +

> +&gmac0 {
> +        assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
> +        assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>;
> +        assigned-clock-rates = <0>, <125000000>;
> +        clock_in_out = "output";
> +        phy-mode = "rgmii-id";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&gmac0_miim
> +                     &gmac0_tx_bus2
> +                     &gmac0_rx_bus2
> +                     &gmac0_rgmii_clk
> +                     &gmac0_rgmii_bus>;
> +        snps,reset-gpio = <&gpio2 RK_PD3 GPIO_ACTIVE_LOW>;
> +        snps,reset-active-low;
> +        snps,reset-delays-us = <0 20000 100000>;
> +        tx_delay = <0x3c>;
> +        rx_delay = <0x2f>;

please see other recent mails about those
The rgmii-id above should not need those delays?

> +        phy-handle = <&rgmii_phy0>;
> +        status = "okay";
> +};
> +
> + /* combphy0/multi-phy0 supports one of: usb3.0 otg, sata0 */

drop the comments

> +&combphy0 {
> +        status = "okay";
> +
> +};
> +
> + /* combphy1/multi-phy1 supports one of:
> +                         - usb3.0 host
> +                         - sata1
> +                         - qsgmii/sgmii
> + */
> +
> +&combphy1 {
> +        status = "okay";
> +};
> +
> + /* combphy2/multi-phy2 supports one of:
> +                         - pcie2.1
> +                         - sata2
> +                         - qsgmii/sgmii
> + */
> +
> +&combphy2 {
> +        status = "okay";
> +};
> +

I did stop here for now.

Heiko