| 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: <afe968a0-f14b-423d-81a7-c1046d2b32bb@cherry.de>
Date: Wed, 7 Jan 2026 16:46:22 +0100
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Chaoyi Chen <kernel@...kyi.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
Chaoyi Chen <chaoyi.chen@...k-chips.com>,
Kever Yang <kever.yang@...k-chips.com>, Jonas Karlman <jonas@...boo.se>,
John Clark <inindev@...il.com>, FUKAUMI Naoki <naoki@...xa.com>,
Jimmy Hon <honyuenkwun@...il.com>, Dragan Simic <dsimic@...jaro.org>,
Michael Riesch <michael.riesch@...labora.com>,
Peter Robinson <pbrobinson@...il.com>, Alexey Charkov <alchark@...il.com>,
Shawn Lin <shawn.lin@...k-chips.com>,
Sebastian Reichel <sebastian.reichel@...labora.com>,
Andy Yan <andy.yan@...k-chips.com>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: dts: rockchip: Add rk3576 evb2 board
Hi Chaoyi,
On 1/7/26 8:03 AM, Chaoyi Chen wrote:
[...]
> diff --git a/arch/arm64/boot/dts/rockchip/rk3576-evb2-v10.dts b/arch/arm64/boot/dts/rockchip/rk3576-evb2-v10.dts
> new file mode 100644
> index 000000000000..52788c514ec0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-evb2-v10.dts
> @@ -0,0 +1,997 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2025 Rockchip Electronics Co., Ltd.
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +#include "rk3576.dtsi"
> +
> +/ {
> + model = "Rockchip RK3576 EVB2 V10 Board";
> + compatible = "rockchip,rk3576-evb2-v10", "rockchip,rk3576";
> +
> + aliases {
> + ethernet0 = &gmac0;
> + ethernet1 = &gmac1;
> + };
> +
> + chosen: chosen {
Why a label here?
There are also many other instances of nodes being labelled but whose
label is never used. I would understand for some if you want to have
DTSOs working with this DTB, but here chosen really doesn't make much
sense to me?
> + stdout-path = "serial0:1500000n8";
> + };
> +
> + adc_keys: adc-keys {
Are we expecting to extend this node from another DT? Why the label?
Won't comment on all other labeled-but-no-phandle-use instances, please
check.
> + vcc3v3_rtc_s5: regulator-vcc3v3-rtc-s5 {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc3v3_rtc_s5";
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + vin-supply = <&vcc_sys>;
If this is for the rtc, shouldn't we declare this dependency in the RTC
device node and not have it always-on?
> + };
> +
> + vcc3v3_sata_pwren: vcc3v3-sata-pwren {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc3v3_sata_pwren";
> + enable-active-high;
> + regulator-boot-on;
> + regulator-always-on;
Why do we have this always-on? Seems like we're missing a dependency on
this regulator in the SATA controller?
> + gpio = <&gpio4 RK_PC7 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&satapm_pwren>;
> + };
> +
> + vcc5v0_device: regulator-vcc5v0-device {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc5v0_device";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + vin-supply = <&vcc12v_dcin>;
> + };
> +
> + vcc5v0_host: regulator-vcc5v0-host {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc5v0_host";
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + enable-active-high;
> + gpio = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;
> + vin-supply = <&vcc5v0_device>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&usb_host_pwren>;
> + };
> +
I assume both of the above are related to USB operating in host or
device mode? Maybe there's a way to have something more useful to the
user in regulator-name (and possibly the regulator node name) so that
they have an idea what this pertains to?
Additionally, why is this always-on? I would assume the USB controller
is capable of controlling its regulator(s)?
[...]
> + vcc_ufs_s0: regulator-vcc-ufs-s0 {
We also have another regulator for UFS that is mentioned in the UFS
controller node but not this one, why?
> + compatible = "regulator-fixed";
> + regulator-name = "vcc_ufs_s0";
> + regulator-boot-on;
> + regulator-always-on;
Why always on?
[...]
> +&mdio0 {
> + rgmii_phy0: ethernet-phy@1 {
> + compatible = "ethernet-phy-id4f51.e91b";
Is MDIO auto-detection broken such that you need to specify the PHY
vendor and product id? Which PHY is that? Why can't you use c22 or c45
compatible? A comment would be nice.
> + reg = <0x1>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&rgmii_phy0_rst>;
> + reset-assert-us = <20000>;
> + reset-deassert-us = <100000>;
> + reset-gpios = <&gpio3 RK_PD3 GPIO_ACTIVE_LOW>;
> + };
> +};
> +
> +&mdio1 {
> + rgmii_phy1: ethernet-phy@1 {
> + compatible = "ethernet-phy-id4f51.e91b";
Ditto.
[...]
> +&sdhci {
> + bus-width = <8>;
> + full-pwr-cycle-in-suspend;
> + max-frequency = <200000000>;
Already that value in rk3576.dtsi.
> + mmc-hs400-1_8v;
> + mmc-hs400-enhanced-strobe;
> + no-sdio;
> + no-sd;
> + non-removable;
> + status = "okay";
> +};
> +
> +&sdmmc {
> + bus-width = <4>;
> + cap-sd-highspeed;
> + disable-wp;
> + max-frequency = <200000000>;
Already that value in rk3576.dtsi.
> + no-sdio;
> + no-mmc;
> + sd-uhs-sdr104;
> + vqmmc-supply = <&vccio_sd_s0>;
> + status = "okay";
> +};
> +
> +&saradc {
This is not alphabetically sorted, it should be before &sata0.
[...]
> + bluetooth {
> + compatible = "brcm,bcm43438-bt";
> + clocks = <&hym8563>;
> + clock-names = "lpo";
> + device-wakeup-gpios = <&gpio1 RK_PD4 GPIO_ACTIVE_HIGH>;
> + interrupt-parent = <&gpio0>;
> + interrupts = <RK_PB1 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-0 = <&bt_reg_on &bt_wake_host &host_wake_bt>;
> + pinctrl-names = "default";
> + shutdown-gpios = <&gpio1 RK_PC7 GPIO_ACTIVE_HIGH>;
Is this GPIO only controlling Bluetooth or also WiFi? I've seen a few
combo chips where there's a common GPIO that controls both WiFi and
Bluetooth. Making this bluetooth-specific means we need Bluetooth on for
WiFi to work, a bit unexpected and should probably be modeled another way.
Cheers,
Quentin
Powered by blists - more mailing lists