On 2025/1/12 16:20, Chukun Pan wrote:

Hi,

quoted

+	/* pi6c pcie clock generator */
+	vcc3v3_pi6c: regulator-vcc3v3-pi6c {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpios = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>;

Although both gpio and gpios can be used here, I tend 
to use gpio to keep it consistent with other parts.
(vcc3v3_sd, vcc3v4_rf, vcc5v0_usb30_otg0 ...)

Yes, I overlooked it, I will change it to this:
gpio = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>;

quoted

+&mdio1 {
+	rgmii_phy1: ethernet-phy@0 {

Only one rgmii phy I think '1' is not needed?
Or `phy@address: ethernet-phy@address {`

I used sgmii_phy0 rgmii_phy1 to name them, they are
indeed redundant, and sgmii is not in mainline yet,
I will change rgmii_phy1 to rgmii_phy.

quoted

+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <0x0>;

The phy address should be 3. Can you try it?

It seems to work fine when using address 0x3, I will modify it.

quoted

+		pinctrl-names = "default";
+		pinctrl-0 = <&eth1_phy_rst>;

Is pinctrl necessary here?

I will remove it.

The node now looks like this:
&mdio1 {
	rgmii_phy: ethernet-phy@3 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x3>;
		reset-assert-us = <20000>;
		reset-deassert-us = <100000>;
		reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
		rx-internal-delay-ps = <1500>;
		tx-internal-delay-ps = <1500>;
	};
};

quoted

+&sdmmc1 {
+   ...
+	wifi: wifi@1 {
+   ...
+&uart1 {
+   ...
+	bluetooth: bluetooth {

No aliases needed.

I will remove it.

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao