Re: [PATCH 3/3] arm64: dts: cix: add Sky1 USB/DP combo PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: cix: enabling usbc_phy0..3 asserts PHY resets at boot
- [High] phy: cix: broken mode transitions, clock leak, and phy_ops in cix_udphy
- [Medium] arm64: dts: cix: missing OF graph ports for usbc_phy Type-C switches
--

commit d16228024f6482f816de21fc474d6f5027baff41
Author: Gary Yang <gary.yang@cixtech.com>

arm64: dts: cix: add Sky1 USB/DP combo PHY

This commit adds the four USB/DP combo PHY device tree nodes to the CIX Sky1
SoC dtsi and enables them with Type-C orientation-switch and mode-switch
properties on the Radxa Orion O6 board.

> diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> index e39c87774c128..76f75185b6a8e 100644
> --- a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> +++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> @@ -117,3 +117,31 @@ &s5_gpio2 {
>  &uart2 {
>  	status = "okay";
>  };
> +
> +&usbc_phy0 {
> +	orientation-switch;
> +	mode-switch;

[Severity: Medium]
How are Type-C connector nodes expected to link to the orientation-switch
and mode-switch registered by usbc_phy0 through usbc_phy3?

In drivers/phy/cix/phy-cix-usbdp.c, cix_udphy_probe() checks for these
boolean properties and registers a Type-C switch and mux on the PHY device
fwnode via udphy_setup_orien_switch() and udphy_setup_typec_mux():

drivers/phy/cix/phy-cix-usbdp.c:cix_udphy_probe() {
	...
	if (device_property_present(dev, "orientation-switch")) {
		ret = udphy_setup_orien_switch(udphy);
		...
	}

	if (device_property_present(dev, "mode-switch")) {
		ret = udphy_setup_typec_mux(udphy);
		...
	}
}

When a Type-C connector driver looks up its switch or mux through
fwnode_typec_switch_get() or fwnode_typec_mux_get(),
fwnode_connection_find_matches() -> fwnode_graph_devcon_matches() resolves
boolean orientation-switch and mode-switch providers by walking OF graph
endpoints (port or ports with remote-endpoint) from the connector node to
the provider node.

However, neither sky1.dtsi nor sky1-orion-o6.dts defines any port or ports
nodes on usbc_phy0..3, and
Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml sets
additionalProperties: false while only allowing usb-port@[0-9a-f]+ and
dp-port@[0-9a-f]+ child nodes:

  patternProperties:
    "^usb-port@[0-9a-f]+$":
      ...
    "^dp-port@[0-9a-f]+$":
      ...
  additionalProperties: false

Without OF graph port definitions in the binding and device tree nodes,
won't any usb-c-connector node be unable to bind to these switches or fail
DT schema validation?

> +	cix,default-conf = /bits/ 8 <0x03>;
> +	status = "okay";
> +};

[Severity: High]
Does enabling usbc_phy0 through usbc_phy3 here before any USB or DisplayPort
consumer nodes are present in the device tree break bootloader-initialized
USB or DisplayPort connections at boot?

Neither arch/arm64/boot/dts/cix/sky1.dtsi nor
arch/arm64/boot/dts/cix/sky1-orion-o6.dts currently has any consumer nodes
referencing usb3_phy0..3 or usbc0..3_dp_phy. When cix_udphy_probe() runs for
each enabled PHY node, it unconditionally asserts both the core and APB
resets and sets cur_mode to UDPHY_MODE_NONE:

drivers/phy/cix/phy-cix-usbdp.c:cix_udphy_probe() {
	...
	reset_control_assert(udphy->reset);
	reset_control_assert(udphy->preset);
	...
	udphy_set_default_mode(udphy);
}

drivers/phy/cix/phy-cix-usbdp.c:udphy_set_default_mode() {
	udphy->cur_mode = UDPHY_MODE_NONE;
}

Although commit ba3650ed160e ("phy: cix: add USB/DP combo PHY driver for
Sky1") states that the bootloader may hand over the port already initialized,
asserting reset and preset in cix_udphy_probe() while no consumers exist to
power the PHYs back on leaves all four PHYs permanently held in reset after
probe. Wouldn't this tear down any active bootloader-initialized DisplayPort
(such as an EFI framebuffer output) or USB link on Radxa Orion O6?

[ ... ]

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index 0a820f45feb96..672aade42d6f8 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -606,6 +606,102 @@ mbox_ap2sfh: mailbox@80a0000 {
>  			cix,mbox-dir = "tx";
>  		};
>  
> +		usbc_phy0: usb-phy@9030000 {
> +			compatible = "cix,sky1-usbdp-phy";
> +			reg = <0x00 0x9030000 0x00 0x40000>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			clocks = <&scmi_clk CLK_TREE_USB3C_DRD_PHY3_GATE>;
> +			resets = <&s5_syscon SKY1_USB_DP_PHY0_RST_N>,
> +				 <&s5_syscon SKY1_USB_DP_PHY0_PRST_N>;
> +			reset-names = "core", "apb";
> +			cix,usbphy-syscon = <&s5_syscon 0>;
> +
> +			status = "disabled";
> +
> +			usb3_phy0: usb-port@0 {
> +				reg = <0x0>;
> +				#phy-cells = <0>;
> +			};
> +
> +			usbc0_dp_phy: dp-port@1 {
> +				reg = <0x1>;
> +				#phy-cells = <0>;
> +			};
> +		};

[Severity: High]
When these PHY nodes are enabled on Radxa Orion O6, can the cix,sky1-usbdp-phy
driver handle USB3 PHY initialization and Type-C mode transitions without
skipping lane reconfiguration or leaking udphy->apb_clk enable references?

First, in drivers/phy/cix/phy-cix-usbdp.c, cix_usbdp_phy_power_on() checks
udphy->cur_mode against udphy->next_mode:

drivers/phy/cix/phy-cix-usbdp.c:cix_usbdp_phy_power_on() {
	if (udphy->cur_mode == udphy->next_mode)
		return 0;

	if ((udphy->cur_mode == UDPHY_MODE_USB || udphy->cur_mode == UDPHY_MODE_DP) &&
	    udphy->next_mode == UDPHY_MODE_DP_USB)
		return 0;

	if (phy_cfgs->udphy_init) {
		ret = phy_cfgs->udphy_init(udphy);
		...
	}
}

If the PHY is brought up in 4-lane DP-only mode (udphy->cur_mode ==
UDPHY_MODE_DP, which programs sky1_udphy_dp_conf in sky1_udphy_init()) and
usbdp_typec_mux_set() later switches udphy->next_mode to UDPHY_MODE_DP_USB, a
subsequent call to cix_u3phy_init() -> cix_usbdp_phy_power_on() hits the early
return above and skips sky1_udphy_init(). Doesn't that leave the hardware in
4-lane DP-only configuration with USB3 non-functional?

Second, if USB is active (udphy->cur_mode == UDPHY_MODE_USB, with
udphy->apb_clk already enabled by sky1_udphy_init()) and udphy->next_mode
changes to UDPHY_MODE_DP, udphy_dp_phy_power_on() calls
cix_usbdp_phy_power_on() -> sky1_udphy_init(), which calls
sky1_udphy_clk_enable() -> clk_prepare_enable(udphy->apb_clk) a second time
without a preceding disable, and then overwrites udphy->cur_mode:

drivers/phy/cix/phy-cix-usbdp.c:udphy_dp_phy_power_on() {
	...
	ret = cix_usbdp_phy_power_on(udphy);
	if (ret)
		goto unlock;

	phy_set_bus_width(phy, udphy_dplane_get(udphy));
	if (udphy->next_mode == UDPHY_MODE_DP)
		udphy->cur_mode = UDPHY_MODE_DP;
	else
		udphy->cur_mode |= UDPHY_MODE_DP;
}

Because overwriting udphy->cur_mode clears the UDPHY_MODE_USB bit,
cix_u3phy_exit() becomes a no-op and cix_usbdp_phy_power_off() ->
sky1_udphy_exit() only calls clk_disable_unprepare(udphy->apb_clk) once. Does
this leak a prepare/enable reference on udphy->apb_clk?

Third, cix_u3phy_ops only wires up .set_mode and .exit, omitting .init and
.power_on:

drivers/phy/cix/phy-cix-usbdp.c:cix_u3phy_ops {
	.set_mode       = cix_u3phy_set_mode,
	.exit		= cix_u3phy_exit,
	.owner		= THIS_MODULE,
};

Won't standard USB controller drivers that call phy_init() and phy_power_on()
before accessing controller registers or before calling phy_set_mode()
operate while usb3_phy0..3 are still held in reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102081532.2305142-1-gary.yang@cixtech.com?part=3