Skip to content

arm64: dts: rockchip: flipper-one: wire Type-C up port VBUS to the connector #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
munzzyy wants to merge 1 commit into
base: flipper-devel
Choose a base branch
from
Open

arm64: dts: rockchip: flipper-one: wire Type-C up port VBUS to the connector #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions arch/arm64/boot/dts/rockchip/rk3576-flipper-one-rev-f0b1c2.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
compatible = "regulator-fixed";
enable-active-high;
regulator-name = "vusb_typec_up";
regulator-always-on; /* Remove once VBUS is wired up in the DT binding and driver */
regulator-min-microvolt = <5200000>;
regulator-max-microvolt = <5200000>;
gpios = <&gpio_expander 0x7 GPIO_ACTIVE_HIGH>;
Expand Down Expand Up @@ -69,6 +68,10 @@
};
};

&typec_up_con {
vbus-supply = <&vusb_typec_up>;
};
Comment on lines +72 to +73

@alchark alchark Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work, because a generic Type-C connector which is not managed by an actual device (such as the mux here) has no idea how or when to switch the VBUS on or off. It's just the dumb receptacle on the board.

VBUS management is a feature of the mux controller, so the property must belong there (either directly or in a subnode)

@munzzyy munzzyy Jul 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the driver already does exactly that, and there's a mainline board relying on it.

hd3ss3220 is the thing that requests VBUS, and it doesn't look on its own node. With no connector child it walks the graph to the connector and looks there:

ep = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220->dev), NULL);
connector = fwnode_graph_get_remote_port_parent(ep);
...
vbus = devm_of_regulator_get_optional(hd3ss3220->dev, to_of_node(connector), "vbus");

arch/arm64/boot/dts/qcom/lemans-evk.dts has the same shape this patch produces: a standalone connector-0 with compatible = "usb-c-connector" carrying vbus-supply = <&usb0_vbus>, whose port@1 endpoint links to the hd3ss3220's port@0. That board depends on the lookup succeeding, since id-gpios is only read when hd3ss3220->vbus came back non-NULL, and its whole VBUS-follows-ID behaviour hangs off that.

It's also where the feature came from. f8d2bf7c0c5d ("usb: typec: hd3ss3220: Enable VBUS based on ID pin state") is Krishna Kurapati's, reviewed by Heikki Krogerus, and lemans-evk is the Qualcomm board it was written against.

On the modelling point, I don't think we disagree. VBUS management stays with the mux, and the mux driver is still what enables and disables the regulator. vbus-supply on the connector isn't claiming the receptacle manages anything, it describes which regulator feeds that receptacle's VBUS pin, which is why usb-connector.yaml is where the property is defined rather than in each controller's binding.

If you'd still rather it sat on &usbmux, that's possible but it needs two upstream changes first: a binding change, because ti,hd3ss3220.yaml is additionalProperties: false and describes neither vbus-supply nor a connector subnode, and a driver change, because nothing looks for the regulator on the mux node today. I'm happy to write both if that's the shape you want. The connector placement is just the one that works with no upstream changes at all.

@munzzyy munzzyy Jul 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing on the subnode option, since that's the other shape you floated and I don't think it does what we'd want here.

Adding a connector child to &usbmux changes which branch probe takes:

connector = device_get_named_child_node(hd3ss3220->dev, "connector");
if (connector) {
	hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
} else {
	...
	hd3ss3220->role_sw = usb_role_switch_get(hd3ss3220->dev);
}

A bare connector { vbus-supply = <&vusb_typec_up>; } has no usb-role-switch reference, and its parent is the mux rather than a role switch, so fwnode_usb_role_switch_get() comes back NULL. NULL isn't an error, so probe carries on happily, and then usb_role_switch_set_role() opens with if (IS_ERR_OR_NULL(sw)) return 0;. Role switching would go quietly dead: no probe failure, nothing in dmesg. Same shape of bug as the VBUS one.

To use the subnode we'd have to re-describe the role-switch link inside it too, on top of a binding patch to allow connector at all.

So the three shapes, as I see them:

  • connector node (this patch): works on current mainline, no kernel changes needed, precedent in qcom/lemans-evk.dts
  • connector subnode of the mux: binding patch, plus re-wiring the role switch into the subnode or role switching silently stops
  • directly on &usbmux: binding patch plus a driver patch, since nothing reads the regulator from the mux node today

Happy to write the patches for either of the bottom two if that's the shape you want. I just wanted the tradeoff visible before we pick one.


&uart1 {
pinctrl-names = "default";
pinctrl-0 = <&uart1m1_xfer>;
Expand All @@ -85,7 +88,6 @@
&usbmux {
interrupt-parent = <&gpio_expander>;
interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
vbus-supply = <&vusb_typec_up>; /* Wire it up in the DT binding and driver */
};

&vcc5v0_device_s0 {
Expand Down