Re: [PATCH v14 2/2] arm64: dts: rockchip: Add ALIENTEK QuarkPi-CA2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] arm64: dts: rockchip: broken USB-C and USBDP PHY graph in quarkpi-ca2
- [Medium] arm64: dts: rockchip: gmac1 snps,reset-gpio wipes YT8531 clock on resume
- [Low] arm64: dts: rockchip: duplicate headphone DAPM routes in quarkpi-ca2
- [Low] arm64: dts: rockchip: unused regulators and redundant DT properties
--

commit df968b833dae8e9d1c2068c44391cf022074881d
Author: Bob Hui <bg9oxa@163.com>

arm64: dts: rockchip: Add ALIENTEK QuarkPi-CA2

This commit adds the device tree for the ALIENTEK QuarkPi-CA2, an RK3588S
based card computer.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-quarkpi-ca2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-quarkpi-ca2.dts
> new file mode 100644
> index 0000000000000..d11b14f23675c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-quarkpi-ca2.dts
> @@ -0,0 +1,1210 @@

[ ... ]

> +	analog_sound: analog-sound {
> +		compatible = "simple-audio-card";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&hp_det>;
> +		simple-audio-card,name = "QuarkPi-CA2 ES8388";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,bitclock-master = <&sound_cpu>;
> +		simple-audio-card,frame-master = <&sound_cpu>;
> +		simple-audio-card,mclk-fs = <256>;
> +		simple-audio-card,aux-devs = <&hp_amp>;
> +		simple-audio-card,hp-det-gpios = <&gpio1 RK_PC4 GPIO_ACTIVE_HIGH>;
> +		simple-audio-card,pin-switches = "Headphones", "Headset Mic";
> +		simple-audio-card,widgets =
> +			"Headphone", "Headphones",
> +			"Microphone", "Headset Mic";
> +		simple-audio-card,routing =
> +			"Headphones", "LOUT1",
> +			"Headphones", "ROUT1",
> +			"Headphones", "Headphone Power OUTL",
> +			"Headphones", "Headphone Power OUTR",
> +			"Headphone Power INL", "LOUT1",
> +			"Headphone Power INR", "ROUT1",
> +			"LINPUT1", "Headset Mic",
> +			"RINPUT1", "Headset Mic";

[Severity: Low]
In analog_sound, LOUT1 and ROUT1 are routed into the hp_amp simple-audio-amplifier
via Headphone Power INL and Headphone Power INR, with Headphone Power OUTL and
Headphone Power OUTR feeding Headphones.

Should the direct "Headphones", "LOUT1" and "Headphones", "ROUT1" entries be
dropped so that the DAPM graph does not also define an unamplified bypass path
around the headphone amplifier?

[ ... ]

> +	combophy_avdd0v85: regulator-combophy-avdd0v85 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "combophy_avdd0v85";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <850000>;
> +		regulator-max-microvolt = <850000>;
> +		vin-supply = <&vdd_0v85_s0>;
> +	};
> +
> +	combophy_avdd1v8: regulator-combophy-avdd1v8 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "combophy_avdd1v8";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&avcc_1v8_s0>;
> +	};

[ ... ]

> +&gmac1 {
> +	phy-mode = "rgmii-id";
> +	clock_in_out = "input";
> +
> +	snps,reset-gpio = <&gpio0 RK_PD5 GPIO_ACTIVE_LOW>;
> +	snps,reset-active-low;
> +	snps,reset-delays-us = <0 20000 100000>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac1_miim
> +		     &gmac1_tx_bus2
> +		     &gmac1_rx_bus2
> +		     &gmac1_rgmii_clk
> +		     &gmac1_rgmii_bus
> +		     &gmac1_clkinout>;
> +
> +	phy-handle = <&rgmii_phy1>;
> +	status = "okay";
> +};

[ ... ]

> +		usb_con: connector {
> +			compatible = "usb-c-connector";
> +			label = "USB-C";
> +			power-role = "dual";
> +			data-role = "dual";
> +			try-power-role = "sink";
> +			source-pdos = <PDO_FIXED(5000, 3000, PDO_FIXED_USB_COMM)>;
> +			sink-pdos = <PDO_FIXED(5000, 1000, PDO_FIXED_USB_COMM)>;
> +			op-sink-microwatt = <1000000>;
> +
> +			altmodes {
> +				displayport {
> +					svid = /bits/ 16 <0xff01>;
> +					vdo = <0xffffffff>;
> +				};
> +			};
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					usbc0_orien_sw: endpoint {
> +						remote-endpoint = <&usbdp_phy0_orientation_switch>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					usbc0_role_sw: endpoint {
> +						remote-endpoint = <&dwc3_0_role_switch>;
> +					};
> +				};
> +
> +				port@2 {
> +					reg = <2>;
> +
> +					dp_altmode_mux: endpoint {
> +						remote-endpoint = <&usbdp_phy0_dp_altmode_mux>;
> +					};
> +				};
> +			};
> +		};

[ ... ]

> +&mdio1 {
> +	rgmii_phy1: ethernet-phy@1 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0x1>;
> +
> +		motorcomm,clk-out-frequency-hz = <125000000>;
> +		motorcomm,keep-pll-enabled;
> +	};
> +};

[Severity: Medium]
Does placing snps,reset-gpio on gmac1 break Ethernet across system suspend and
resume?

Here gmac1 sets clock_in_out = "input", so the MAC relies on the 125 MHz
reference clock output from the YT8531 PHY configured via
motorcomm,clk-out-frequency-hz = <125000000> on rgmii_phy1.

In drivers/net/phy/motorcomm.c, yt8531_probe() programs
motorcomm,clk-out-frequency-hz into YTPHY_SYNCE_CFG_REG only once when the PHY
is registered:

drivers/net/phy/motorcomm.c:yt8531_probe() {
	...
	if (device_property_read_u32(dev, "motorcomm,clk-out-frequency-hz", &freq))
		freq = YTPHY_DTS_OUTPUT_CLK_DIS;
	...
	return ytphy_modify_ext_with_lock(phydev, YTPHY_SYNCE_CFG_REG, mask,
					  val);
}

When snps,reset-gpio is placed on the gmac1 node (which is also deprecated in
Documentation/devicetree/bindings/net/snps,dwmac.yaml and missing a pinctrl
entry for gpio0 RK_PD5), stmmac_resume() calls stmmac_mdio_reset() when
resuming without Wake-on-LAN:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_resume() {
	...
	if (!device_may_wakeup(priv->device) || !priv->wolopts) {
		...
		/* reset the phy so that it's ready */
		if (priv->mii)
			stmmac_mdio_reset(priv->mii);
	}
	...
}

stmmac_mdio_reset() pulses the snps,reset-gpio line and hardware-resets the
YT8531 PHY. Because stmmac sets mac_managed_pm = true, mdio_bus_phy_resume()
skips phy_init_hw(), and neither yt8531_probe() nor yt8531_config_init() runs
after that reset. Doesn't that revert YTPHY_SYNCE_CFG_REG to its 25 MHz reset
default (stopping the 125 MHz reference clock needed by stmmac_hw_setup()) and
lose the RGMII delay configuration?

Note that moving reset-gpios into rgmii_phy1 while keeping
compatible = "ethernet-phy-ieee802.3-c22" would also cause issues because
fwnode_mdiobus_register_phy() calls get_phy_device() before per-PHY
reset-gpios is deasserted, and phy_detach_internal() asserts per-PHY
reset-gpios on interface down. Would describing the reset on the mdio1 bus
node (reset-gpios, reset-delay-us, reset-post-delay-us) avoid resetting the
PHY behind the PHY driver's back?

[ ... ]

> +&sata0 {
> +	/* shares combphy0_ps with pcie2x1l2 */
> +	status = "disabled";
> +};

[ ... ]

> +&sdmmc {
> +	max-frequency = <150000000>;
> +	no-sdio;
> +	no-mmc;
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;
> +	disable-wp;
> +	sd-uhs-sdr104;
> +	vmmc-supply = <&vcc_3v3_sd_s0>;
> +	vqmmc-supply = <&vccio_sd_s0>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdmmc_bus4 &sdmmc_clk &sdmmc_cmd &sdmmc_det>;
> +	status = "okay";
> +};

[Severity: Low]
This isn't a bug, but there are a few unused or redundant definitions in the
device tree:

1. combophy_avdd0v85 and combophy_avdd1v8 are defined as always-on fixed
   regulators without GPIO control and have no consumer references anywhere in
   the DT.
2. sata0 is already status = "disabled" in rk3588-base.dtsi, so the override
   here is redundant.
3. sdmmc sets both no-mmc and cap-mmc-highspeed; since no-mmc disables MMC
   card initialization in mmc_of_parse(), cap-mmc-highspeed has no effect.

Could these be cleaned up?

[ ... ]

> +&usbdp_phy0 {
> +	mode-switch;
> +	orientation-switch;
> +	sbu1-dc-gpios = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
> +	sbu2-dc-gpios = <&gpio1 RK_PA7 GPIO_ACTIVE_HIGH>;
> +	rockchip,dp-lane-mux = <2 3>;
> +	status = "okay";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		usbdp_phy0_orientation_switch: endpoint@0 {
> +			reg = <0>;
> +			remote-endpoint = <&usbc0_orien_sw>;
> +		};\n+
> +		usbdp_phy0_dp_altmode_mux: endpoint@1 {
> +			reg = <1>;
> +			remote-endpoint = <&dp_altmode_mux>;
> +		};
> +
> +		usbdp_phy0_dp_in: endpoint@2 {
> +			reg = <2>;
> +			remote-endpoint = <&dp0_out_con>;
> +		};
> +	};
> +};

[Severity: Medium]
Can DisplayPort AltMode and the USB-C graph work as intended with this
usbdp_phy0 and usb_con layout?

1. In drivers/phy/rockchip/phy-rockchip-usbdp.c, rk_udphy_probe() checks for
   port 3, endpoint 0 before registering the DRM AUX bridge:

   drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_probe() {
       ...
       dp_aux_ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 3, 0, 0);
       if (dp_aux_ep) {
           ret = drm_aux_bridge_register(dev);
           fwnode_handle_put(dp_aux_ep);
           if (ret)
               return ret;
       }
       ...
   }

   Here usbdp_phy0 uses the deprecated single port container with endpoint@0,
   endpoint@1, and endpoint@2 instead of a ports node with port@0 through
   port@3 as described in
   Documentation/devicetree/bindings/phy/phy-rockchip-usbdp.yaml. Because
   port 3 is absent, fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 3, 0, 0)
   returns NULL and drm_aux_bridge_register() is never called.

2. In usb_con (compatible = "usb-c-connector"), port@0 is wired to
   usbdp_phy0_orientation_switch and port@1 is wired to dwc3_0_role_switch.
   Per Documentation/devicetree/bindings/connector/usb-connector.yaml, port@0
   is for High Speed (the USB controller role switch) and port@1 is for Super
   Speed (the USBDP PHY), so port@0 and port@1 appear to be swapped.

3. usbdp_phy0 sets rockchip,dp-lane-mux = <2 3> alongside mode-switch and
   orientation-switch. In rk_udphy_parse_lane_mux_data(), specifying
   rockchip,dp-lane-mux forces static UDPHY_MODE_DP_USB and 2-lane DP at probe
   time, whereas phy-rockchip-usbdp.yaml states that rockchip,dp-lane-mux is
   not needed when DP lanes are mapped dynamically by DisplayPort AltMode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-quarkpi-ca2-v14-1-290990440b67@163.com?part=2