| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250718092155.5c756e3f.xunil@tahomasoft.com>
Date: Fri, 18 Jul 2025 09:21:55 -0400
From: Erik Beck <xunil@...omasoft.com>
To: Heiko Stuebner <heiko@...ech.de>
Cc: linux-rockchip@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, soc@...ts.linux.dev
Subject: Re: [PATCH 1/1] arm: dts: rockchip: Add initial support for
LinkStar H68K-1432v1 Board with RK3568 SoC
Thanks for the prompt feedback Heiko!
I'll get to work on responding to your comments, which are quite
helpful.
Regards,
Erik
On Fri, 18 Jul 2025 15:13:21 +0200
Heiko Stuebner <heiko@...ech.de> wrote:
> 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