[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <10784977.EvYhyI6sBW@phil>
Date: Fri, 18 Jul 2025 15:13:21 +0200
From: Heiko Stuebner <heiko@...ech.de>
To: linux-rockchip@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
soc@...ts.linux.dev, Erik Beck <xunil@...omasoft.com>
Subject:
Re: [PATCH 1/1] arm: dts: rockchip: Add initial support for LinkStar
H68K-1432v1 Board with RK3568 SoC
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@...omasoft.com>
> Tested-by: Erik Beck <xunil@...omasoft.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@....edu.cn> +//
> +// Copyright (c) 2025 TahomaSoft xunil@...omasoft.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
Powered by blists - more mailing lists