[PATCH v4 0/4] RK3576 USB Enablement

[PATCH v4 0/4] RK3576 USB Enablement

[Nicolas Frattaroli]

This series is the result of what I thought would be a quick 10 minute
job, but turned out to be more like 3 days of pain, suffering, and
confusion. This should be expected with USB Type C though.

The first patch in the series extends the inno usb2 PHY driver to fiddle
with some GRF flags in that driver when the PHY is connected to a USB
Type C port. Without this change, devices on USB-C simply don't
enumerate at all, as the state machine gets stuck waiting for vbus to go
low or something along those lines.

An alternate way to implement this would've been a vendor property in
the PHY binding which is then checked for in the driver and needs to be
present in all rockchip inno u2phy instances that happen to be connected
to a USB Type C connector. This is what downstream does, for example.

Patch 2 and 3 allow Super Speed in reverse orientation on USB Type-C
connectors to work, but I am not entirely confident in the solution I
arrived at.

Patch 4 adds the USB related nodes, including associated regulators and
Type C controllers, to the Sige5 tree.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v4:
- Rebase onto v6.16-rc1, no other changes
- Link to v3: https://lore.kernel.org/r/20250507-rk3576-sige5-usb-v3-0-89bf5a614ccf@collabora.com

Changes in v3:
- Drop the utmi clock patch. This was always a speculative fix for a
  problem I could no longer reproduce, and it doesn't conform to the
  binding, as Robo-rob correctly caught.
- Link to v2: https://lore.kernel.org/r/20250505-rk3576-sige5-usb-v2-0-d5ba4305f3be@collabora.com

Changes in v2:
- Rebased onto next-20250505
- Drop the u2susphy quirk, as I can no longer reproduce the original
  problem with various amounts of ripping up the DT and changing the
  config. Yeah I'm not super hyped about this now being a heisenbug
  either.
- Drop the bindings patch, as Rob showed me there's a way to do this
  without extending the bindings
- Rewrite the usb 2 phy driver patch to no longer walk an OF graph from
  PHY to connector, but instead first find the USB controller that uses
  this PHY, and then use the USB controller's existing graph connection
  to the usb connector.
- Adjust the Sige5 DTS patch to now have two port connections from the
  USB connector to the drd0 USB controller, one for high-speed aka
  USB2, one for super-speed aka USB3, ordered as per its binding.
- Add a patch for rk3576.dtsi to reference u2phy1 as a clock in the drd1
  controller.
- Add two patches to fix USB Type-C super speed in reverse orientation.
- Link to v1: https://lore.kernel.org/r/20250407-rk3576-sige5-usb-v1-0-67eec166f82f@collabora.com

---
Nicolas Frattaroli (4):
      phy: rockchip: inno-usb2: add soft vbusvalid control
      phy: rockchip: usbdp: move orientation handling further down
      phy: rockchip: usbdp: reset USB3 and reinit on orientation switch
      arm64: dts: rockchip: enable USB on Sige5

 .../boot/dts/rockchip/rk3576-armsom-sige5.dts      | 160 +++++++++++++++++++++
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c      | 113 ++++++++++++++-
 drivers/phy/rockchip/phy-rockchip-usbdp.c          | 154 +++++++++++++-------
 3 files changed, 373 insertions(+), 54 deletions(-)
---
base-commit: d9946fe286439c2aeaa7953b8c316efe5b83d515
change-id: 20250328-rk3576-sige5-usb-230102aeeaca

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v4 1/4] phy: rockchip: inno-usb2: add soft vbusvalid control

[Nicolas Frattaroli]

With USB type C connectors, the vbus detect pin of the OTG controller
attached to it is pulled high by a USB Type C controller chip such as
the fusb302. This means USB enumeration on Type-C ports never works, as
the vbus is always seen as high.

Rockchip added some GRF register flags to deal with this situation. The
RK3576 TRM calls these "soft_vbusvalid_bvalid" (con0 bit index 15) and
"soft_vbusvalid_bvalid_sel" (con0 bit index 14).

Downstream introduces a new vendor property which tells the USB 2 PHY
that it's connected to a type C port, but we can do better. Since in
such an arrangement, we'll have an OF graph connection from the USB
controller to the USB connector anyway, we can walk said OF graph and
check the connector's compatible to determine this without adding any
further vendor properties.

Do keep in mind that the usbdp PHY driver seemingly fiddles with these
register fields as well, but what it does doesn't appear to be enough
for us to get working USB enumeration, presumably because the whole
vbus_attach logic needs to be adjusted as well either way.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 113 +++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index b0f23690ec3002202c0f33a6988f5509622fa10e..4f89bd6568cd3a7a1d2c10e9cddda9f3bd997ed0 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/of_irq.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -114,6 +115,8 @@ struct rockchip_chg_det_reg {
 /**
  * struct rockchip_usb2phy_port_cfg - usb-phy port configuration.
  * @phy_sus: phy suspend register.
+ * @svbus_en: soft vbus bvalid enable register.
+ * @svbus_sel: soft vbus bvalid selection register.
  * @bvalid_det_en: vbus valid rise detection enable register.
  * @bvalid_det_st: vbus valid rise detection status register.
  * @bvalid_det_clr: vbus valid rise detection clear register.
@@ -140,6 +143,8 @@ struct rockchip_chg_det_reg {
  */
 struct rockchip_usb2phy_port_cfg {
 	struct usb2phy_reg	phy_sus;
+	struct usb2phy_reg	svbus_en;
+	struct usb2phy_reg	svbus_sel;
 	struct usb2phy_reg	bvalid_det_en;
 	struct usb2phy_reg	bvalid_det_st;
 	struct usb2phy_reg	bvalid_det_clr;
@@ -203,6 +208,7 @@ struct rockchip_usb2phy_cfg {
  * @event_nb: hold event notification callback.
  * @state: define OTG enumeration states before device reset.
  * @mode: the dr_mode of the controller.
+ * @typec_vbus_det: whether to apply Type C logic to OTG vbus detection.
  */
 struct rockchip_usb2phy_port {
 	struct phy	*phy;
@@ -222,6 +228,7 @@ struct rockchip_usb2phy_port {
 	struct notifier_block	event_nb;
 	enum usb_otg_state	state;
 	enum usb_dr_mode	mode;
+	bool typec_vbus_det;
 };
 
 /**
@@ -495,6 +502,13 @@ static int rockchip_usb2phy_init(struct phy *phy)
 	mutex_lock(&rport->mutex);
 
 	if (rport->port_id == USB2PHY_PORT_OTG) {
+		if (rport->typec_vbus_det) {
+			if (rport->port_cfg->svbus_en.enable &&
+					rport->port_cfg->svbus_sel.enable) {
+				property_enable(rphy->grf, &rport->port_cfg->svbus_en, true);
+				property_enable(rphy->grf, &rport->port_cfg->svbus_sel, true);
+			}
+		}
 		if (rport->mode != USB_DR_MODE_HOST &&
 		    rport->mode != USB_DR_MODE_UNKNOWN) {
 			/* clear bvalid status and enable bvalid detect irq */
@@ -535,8 +549,7 @@ static int rockchip_usb2phy_init(struct phy *phy)
 			if (ret)
 				goto out;
 
-			schedule_delayed_work(&rport->otg_sm_work,
-					      OTG_SCHEDULE_DELAY * 3);
+			schedule_delayed_work(&rport->otg_sm_work, 0);
 		} else {
 			/* If OTG works in host only mode, do nothing. */
 			dev_dbg(&rport->phy->dev, "mode %d\n", rport->mode);
@@ -666,8 +679,17 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
 	unsigned long delay;
 	bool vbus_attach, sch_work, notify_charger;
 
-	vbus_attach = property_enabled(rphy->grf,
-				       &rport->port_cfg->utmi_bvalid);
+	if (rport->port_cfg->svbus_en.enable && rport->typec_vbus_det) {
+		if (property_enabled(rphy->grf, &rport->port_cfg->svbus_en) &&
+		    property_enabled(rphy->grf, &rport->port_cfg->svbus_sel)) {
+			vbus_attach = true;
+		} else {
+			vbus_attach = false;
+		}
+	} else {
+		vbus_attach = property_enabled(rphy->grf,
+					       &rport->port_cfg->utmi_bvalid);
+	}
 
 	sch_work = false;
 	notify_charger = false;
@@ -1276,6 +1298,83 @@ static int rockchip_otg_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static const char *const rockchip_usb2phy_typec_cons[] = {
+	"usb-c-connector",
+	NULL,
+};
+
+static struct device_node *rockchip_usb2phy_to_controller(struct rockchip_usb2phy *rphy)
+{
+	struct device_node *np;
+	struct device_node *parent;
+
+	for_each_node_with_property(np, "phys") {
+		struct of_phandle_iterator it;
+		int ret;
+
+		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
+			parent = of_get_parent(it.node);
+			if (it.node != rphy->dev->of_node && rphy->dev->of_node != parent) {
+				if (parent)
+					of_node_put(parent);
+				continue;
+			}
+
+			/*
+			 * Either the PHY phandle we're iterating or its parent
+			 * matched, we don't care about which out of the two in
+			 * particular as we just need to know it's the right
+			 * USB controller for this PHY.
+			 */
+			of_node_put(it.node);
+			of_node_put(parent);
+			return np;
+		}
+	}
+
+	return NULL;
+}
+
+static bool rockchip_usb2phy_otg_is_type_c(struct rockchip_usb2phy *rphy)
+{
+	struct device_node *controller = rockchip_usb2phy_to_controller(rphy);
+	struct device_node *ports;
+	struct device_node *ep = NULL;
+	struct device_node *parent;
+
+	if (!controller)
+		return false;
+
+	ports = of_get_child_by_name(controller, "ports");
+	if (ports) {
+		of_node_put(controller);
+		controller = ports;
+	}
+
+	for_each_of_graph_port(controller, port) {
+		ep = of_get_child_by_name(port, "endpoint");
+		if (!ep)
+			continue;
+
+		parent = of_graph_get_remote_port_parent(ep);
+		of_node_put(ep);
+		if (!parent)
+			continue;
+
+		if (of_device_compatible_match(parent, rockchip_usb2phy_typec_cons)) {
+			of_node_put(parent);
+			of_node_put(controller);
+			return true;
+		}
+
+		of_node_put(parent);
+	}
+
+	of_node_put(controller);
+
+	return false;
+}
+
 static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
 					  struct rockchip_usb2phy_port *rport,
 					  struct device_node *child_np)
@@ -1297,6 +1396,8 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
 
 	mutex_init(&rport->mutex);
 
+	rport->typec_vbus_det = rockchip_usb2phy_otg_is_type_c(rphy);
+
 	rport->mode = of_usb_get_dr_mode_by_phy(child_np, -1);
 	if (rport->mode == USB_DR_MODE_HOST ||
 	    rport->mode == USB_DR_MODE_UNKNOWN) {
@@ -2050,6 +2151,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
 		.port_cfgs	= {
 			[USB2PHY_PORT_OTG] = {
 				.phy_sus	= { 0x0000, 8, 0, 0, 0x1d1 },
+				.svbus_en	= { 0x0000, 15, 15, 0, 1 },
+				.svbus_sel	= { 0x0000, 14, 14, 0, 1 },
 				.bvalid_det_en	= { 0x00c0, 1, 1, 0, 1 },
 				.bvalid_det_st	= { 0x00c4, 1, 1, 0, 1 },
 				.bvalid_det_clr = { 0x00c8, 1, 1, 0, 1 },
@@ -2087,6 +2190,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
 		.port_cfgs	= {
 			[USB2PHY_PORT_OTG] = {
 				.phy_sus	= { 0x2000, 8, 0, 0, 0x1d1 },
+				.svbus_en	= { 0x2000, 15, 15, 0, 1 },
+				.svbus_sel	= { 0x2000, 14, 14, 0, 1 },
 				.bvalid_det_en	= { 0x20c0, 1, 1, 0, 1 },
 				.bvalid_det_st	= { 0x20c4, 1, 1, 0, 1 },
 				.bvalid_det_clr = { 0x20c8, 1, 1, 0, 1 },

-- 
2.49.0

Re: [PATCH v4 1/4] phy: rockchip: inno-usb2: add soft vbusvalid control

[neil.armstrong]

On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> With USB type C connectors, the vbus detect pin of the OTG controller
> attached to it is pulled high by a USB Type C controller chip such as
> the fusb302. This means USB enumeration on Type-C ports never works, as
> the vbus is always seen as high.
> 
> Rockchip added some GRF register flags to deal with this situation. The
> RK3576 TRM calls these "soft_vbusvalid_bvalid" (con0 bit index 15) and
> "soft_vbusvalid_bvalid_sel" (con0 bit index 14).
> 
> Downstream introduces a new vendor property which tells the USB 2 PHY
> that it's connected to a type C port, but we can do better. Since in
> such an arrangement, we'll have an OF graph connection from the USB
> controller to the USB connector anyway, we can walk said OF graph and
> check the connector's compatible to determine this without adding any
> further vendor properties.
> 
> Do keep in mind that the usbdp PHY driver seemingly fiddles with these
> register fields as well, but what it does doesn't appear to be enough
> for us to get working USB enumeration, presumably because the whole
> vbus_attach logic needs to be adjusted as well either way.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 113 +++++++++++++++++++++++++-
>   1 file changed, 109 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index b0f23690ec3002202c0f33a6988f5509622fa10e..4f89bd6568cd3a7a1d2c10e9cddda9f3bd997ed0 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -17,6 +17,7 @@
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/of.h>
> +#include <linux/of_graph.h>
>   #include <linux/of_irq.h>
>   #include <linux/phy/phy.h>
>   #include <linux/platform_device.h>
> @@ -114,6 +115,8 @@ struct rockchip_chg_det_reg {
>   /**
>    * struct rockchip_usb2phy_port_cfg - usb-phy port configuration.
>    * @phy_sus: phy suspend register.
> + * @svbus_en: soft vbus bvalid enable register.
> + * @svbus_sel: soft vbus bvalid selection register.
>    * @bvalid_det_en: vbus valid rise detection enable register.
>    * @bvalid_det_st: vbus valid rise detection status register.
>    * @bvalid_det_clr: vbus valid rise detection clear register.
> @@ -140,6 +143,8 @@ struct rockchip_chg_det_reg {
>    */
>   struct rockchip_usb2phy_port_cfg {
>   	struct usb2phy_reg	phy_sus;
> +	struct usb2phy_reg	svbus_en;
> +	struct usb2phy_reg	svbus_sel;
>   	struct usb2phy_reg	bvalid_det_en;
>   	struct usb2phy_reg	bvalid_det_st;
>   	struct usb2phy_reg	bvalid_det_clr;
> @@ -203,6 +208,7 @@ struct rockchip_usb2phy_cfg {
>    * @event_nb: hold event notification callback.
>    * @state: define OTG enumeration states before device reset.
>    * @mode: the dr_mode of the controller.
> + * @typec_vbus_det: whether to apply Type C logic to OTG vbus detection.
>    */
>   struct rockchip_usb2phy_port {
>   	struct phy	*phy;
> @@ -222,6 +228,7 @@ struct rockchip_usb2phy_port {
>   	struct notifier_block	event_nb;
>   	enum usb_otg_state	state;
>   	enum usb_dr_mode	mode;
> +	bool typec_vbus_det;
>   };
>   
>   /**
> @@ -495,6 +502,13 @@ static int rockchip_usb2phy_init(struct phy *phy)
>   	mutex_lock(&rport->mutex);
>   
>   	if (rport->port_id == USB2PHY_PORT_OTG) {
> +		if (rport->typec_vbus_det) {
> +			if (rport->port_cfg->svbus_en.enable &&
> +					rport->port_cfg->svbus_sel.enable) {
> +				property_enable(rphy->grf, &rport->port_cfg->svbus_en, true);
> +				property_enable(rphy->grf, &rport->port_cfg->svbus_sel, true);
> +			}
> +		}
>   		if (rport->mode != USB_DR_MODE_HOST &&
>   		    rport->mode != USB_DR_MODE_UNKNOWN) {
>   			/* clear bvalid status and enable bvalid detect irq */
> @@ -535,8 +549,7 @@ static int rockchip_usb2phy_init(struct phy *phy)
>   			if (ret)
>   				goto out;
>   
> -			schedule_delayed_work(&rport->otg_sm_work,
> -					      OTG_SCHEDULE_DELAY * 3);
> +			schedule_delayed_work(&rport->otg_sm_work, 0);
>   		} else {
>   			/* If OTG works in host only mode, do nothing. */
>   			dev_dbg(&rport->phy->dev, "mode %d\n", rport->mode);
> @@ -666,8 +679,17 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
>   	unsigned long delay;
>   	bool vbus_attach, sch_work, notify_charger;
>   
> -	vbus_attach = property_enabled(rphy->grf,
> -				       &rport->port_cfg->utmi_bvalid);
> +	if (rport->port_cfg->svbus_en.enable && rport->typec_vbus_det) {
> +		if (property_enabled(rphy->grf, &rport->port_cfg->svbus_en) &&
> +		    property_enabled(rphy->grf, &rport->port_cfg->svbus_sel)) {

Why do you check the registers since you always enable those bits on those conditions:
	rport->port_id == USB2PHY_PORT_OTG
	rport->typec_vbus_det
	rport->port_cfg->svbus_en.enable
	rport->typec_vbus_det
Can't you us them instead ?

Neil

> +			vbus_attach = true;
> +		} else {
> +			vbus_attach = false;
> +		}
> +	} else {
> +		vbus_attach = property_enabled(rphy->grf,
> +					       &rport->port_cfg->utmi_bvalid);
> +	}
>   
>   	sch_work = false;
>   	notify_charger = false;
> @@ -1276,6 +1298,83 @@ static int rockchip_otg_event(struct notifier_block *nb,
>   	return NOTIFY_DONE;
>   }
>   
> +static const char *const rockchip_usb2phy_typec_cons[] = {
> +	"usb-c-connector",
> +	NULL,
> +};
> +
> +static struct device_node *rockchip_usb2phy_to_controller(struct rockchip_usb2phy *rphy)
> +{
> +	struct device_node *np;
> +	struct device_node *parent;
> +
> +	for_each_node_with_property(np, "phys") {
> +		struct of_phandle_iterator it;
> +		int ret;
> +
> +		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
> +			parent = of_get_parent(it.node);
> +			if (it.node != rphy->dev->of_node && rphy->dev->of_node != parent) {
> +				if (parent)
> +					of_node_put(parent);
> +				continue;
> +			}
> +
> +			/*
> +			 * Either the PHY phandle we're iterating or its parent
> +			 * matched, we don't care about which out of the two in
> +			 * particular as we just need to know it's the right
> +			 * USB controller for this PHY.
> +			 */
> +			of_node_put(it.node);
> +			of_node_put(parent);
> +			return np;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool rockchip_usb2phy_otg_is_type_c(struct rockchip_usb2phy *rphy)
> +{
> +	struct device_node *controller = rockchip_usb2phy_to_controller(rphy);
> +	struct device_node *ports;
> +	struct device_node *ep = NULL;
> +	struct device_node *parent;
> +
> +	if (!controller)
> +		return false;
> +
> +	ports = of_get_child_by_name(controller, "ports");
> +	if (ports) {
> +		of_node_put(controller);
> +		controller = ports;
> +	}
> +
> +	for_each_of_graph_port(controller, port) {
> +		ep = of_get_child_by_name(port, "endpoint");
> +		if (!ep)
> +			continue;
> +
> +		parent = of_graph_get_remote_port_parent(ep);
> +		of_node_put(ep);
> +		if (!parent)
> +			continue;
> +
> +		if (of_device_compatible_match(parent, rockchip_usb2phy_typec_cons)) {
> +			of_node_put(parent);
> +			of_node_put(controller);
> +			return true;
> +		}
> +
> +		of_node_put(parent);
> +	}
> +
> +	of_node_put(controller);
> +
> +	return false;
> +}

This DT parsing is quite complex, but seems it's the only way to detect such features
without bindings change, not sure it's really beneficial.

> +
>   static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
>   					  struct rockchip_usb2phy_port *rport,
>   					  struct device_node *child_np)
> @@ -1297,6 +1396,8 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
>   
>   	mutex_init(&rport->mutex);
>   
> +	rport->typec_vbus_det = rockchip_usb2phy_otg_is_type_c(rphy);
> +
>   	rport->mode = of_usb_get_dr_mode_by_phy(child_np, -1);
>   	if (rport->mode == USB_DR_MODE_HOST ||
>   	    rport->mode == USB_DR_MODE_UNKNOWN) {
> @@ -2050,6 +2151,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
>   		.port_cfgs	= {
>   			[USB2PHY_PORT_OTG] = {
>   				.phy_sus	= { 0x0000, 8, 0, 0, 0x1d1 },
> +				.svbus_en	= { 0x0000, 15, 15, 0, 1 },
> +				.svbus_sel	= { 0x0000, 14, 14, 0, 1 },
>   				.bvalid_det_en	= { 0x00c0, 1, 1, 0, 1 },
>   				.bvalid_det_st	= { 0x00c4, 1, 1, 0, 1 },
>   				.bvalid_det_clr = { 0x00c8, 1, 1, 0, 1 },
> @@ -2087,6 +2190,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
>   		.port_cfgs	= {
>   			[USB2PHY_PORT_OTG] = {
>   				.phy_sus	= { 0x2000, 8, 0, 0, 0x1d1 },
> +				.svbus_en	= { 0x2000, 15, 15, 0, 1 },
> +				.svbus_sel	= { 0x2000, 14, 14, 0, 1 },
>   				.bvalid_det_en	= { 0x20c0, 1, 1, 0, 1 },
>   				.bvalid_det_st	= { 0x20c4, 1, 1, 0, 1 },
>   				.bvalid_det_clr = { 0x20c8, 1, 1, 0, 1 },
> 

Thanks,
Neil

Re: [PATCH v4 1/4] phy: rockchip: inno-usb2: add soft vbusvalid control

[Nicolas Frattaroli]

Hi Neil,

thank you for the review! Responses are inline.

On Friday, 13 June 2025 11:02:40 Central European Summer Time neil.armstrong@linaro.org wrote:
> On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> > With USB type C connectors, the vbus detect pin of the OTG controller
> > attached to it is pulled high by a USB Type C controller chip such as
> > the fusb302. This means USB enumeration on Type-C ports never works, as
> > the vbus is always seen as high.
> > 
> > Rockchip added some GRF register flags to deal with this situation. The
> > RK3576 TRM calls these "soft_vbusvalid_bvalid" (con0 bit index 15) and
> > "soft_vbusvalid_bvalid_sel" (con0 bit index 14).
> > 
> > Downstream introduces a new vendor property which tells the USB 2 PHY
> > that it's connected to a type C port, but we can do better. Since in
> > such an arrangement, we'll have an OF graph connection from the USB
> > controller to the USB connector anyway, we can walk said OF graph and
> > check the connector's compatible to determine this without adding any
> > further vendor properties.
> > 
> > Do keep in mind that the usbdp PHY driver seemingly fiddles with these
> > register fields as well, but what it does doesn't appear to be enough
> > for us to get working USB enumeration, presumably because the whole
> > vbus_attach logic needs to be adjusted as well either way.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 113 +++++++++++++++++++++++++-
> >   1 file changed, 109 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index b0f23690ec3002202c0f33a6988f5509622fa10e..4f89bd6568cd3a7a1d2c10e9cddda9f3bd997ed0 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -17,6 +17,7 @@
> >   #include <linux/module.h>
> >   #include <linux/mutex.h>
> >   #include <linux/of.h>
> > +#include <linux/of_graph.h>
> >   #include <linux/of_irq.h>
> >   #include <linux/phy/phy.h>
> >   #include <linux/platform_device.h>
> > @@ -114,6 +115,8 @@ struct rockchip_chg_det_reg {
> >   /**
> >    * struct rockchip_usb2phy_port_cfg - usb-phy port configuration.
> >    * @phy_sus: phy suspend register.
> > + * @svbus_en: soft vbus bvalid enable register.
> > + * @svbus_sel: soft vbus bvalid selection register.
> >    * @bvalid_det_en: vbus valid rise detection enable register.
> >    * @bvalid_det_st: vbus valid rise detection status register.
> >    * @bvalid_det_clr: vbus valid rise detection clear register.
> > @@ -140,6 +143,8 @@ struct rockchip_chg_det_reg {
> >    */
> >   struct rockchip_usb2phy_port_cfg {
> >   	struct usb2phy_reg	phy_sus;
> > +	struct usb2phy_reg	svbus_en;
> > +	struct usb2phy_reg	svbus_sel;
> >   	struct usb2phy_reg	bvalid_det_en;
> >   	struct usb2phy_reg	bvalid_det_st;
> >   	struct usb2phy_reg	bvalid_det_clr;
> > @@ -203,6 +208,7 @@ struct rockchip_usb2phy_cfg {
> >    * @event_nb: hold event notification callback.
> >    * @state: define OTG enumeration states before device reset.
> >    * @mode: the dr_mode of the controller.
> > + * @typec_vbus_det: whether to apply Type C logic to OTG vbus detection.
> >    */
> >   struct rockchip_usb2phy_port {
> >   	struct phy	*phy;
> > @@ -222,6 +228,7 @@ struct rockchip_usb2phy_port {
> >   	struct notifier_block	event_nb;
> >   	enum usb_otg_state	state;
> >   	enum usb_dr_mode	mode;
> > +	bool typec_vbus_det;
> >   };
> >   
> >   /**
> > @@ -495,6 +502,13 @@ static int rockchip_usb2phy_init(struct phy *phy)
> >   	mutex_lock(&rport->mutex);
> >   
> >   	if (rport->port_id == USB2PHY_PORT_OTG) {
> > +		if (rport->typec_vbus_det) {
> > +			if (rport->port_cfg->svbus_en.enable &&
> > +					rport->port_cfg->svbus_sel.enable) {
> > +				property_enable(rphy->grf, &rport->port_cfg->svbus_en, true);
> > +				property_enable(rphy->grf, &rport->port_cfg->svbus_sel, true);
> > +			}
> > +		}
> >   		if (rport->mode != USB_DR_MODE_HOST &&
> >   		    rport->mode != USB_DR_MODE_UNKNOWN) {
> >   			/* clear bvalid status and enable bvalid detect irq */
> > @@ -535,8 +549,7 @@ static int rockchip_usb2phy_init(struct phy *phy)
> >   			if (ret)
> >   				goto out;
> >   
> > -			schedule_delayed_work(&rport->otg_sm_work,
> > -					      OTG_SCHEDULE_DELAY * 3);
> > +			schedule_delayed_work(&rport->otg_sm_work, 0);
> >   		} else {
> >   			/* If OTG works in host only mode, do nothing. */
> >   			dev_dbg(&rport->phy->dev, "mode %d\n", rport->mode);
> > @@ -666,8 +679,17 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> >   	unsigned long delay;
> >   	bool vbus_attach, sch_work, notify_charger;
> >   
> > -	vbus_attach = property_enabled(rphy->grf,
> > -				       &rport->port_cfg->utmi_bvalid);
> > +	if (rport->port_cfg->svbus_en.enable && rport->typec_vbus_det) {
> > +		if (property_enabled(rphy->grf, &rport->port_cfg->svbus_en) &&
> > +		    property_enabled(rphy->grf, &rport->port_cfg->svbus_sel)) {
> 
> Why do you check the registers since you always enable those bits on those conditions:
> 	rport->port_id == USB2PHY_PORT_OTG
> 	rport->typec_vbus_det
> 	rport->port_cfg->svbus_en.enable
> 	rport->typec_vbus_det
> Can't you us them instead ?

That is an excellent question. I think I was imitating downstream code
here without thinking too much. This whole construct seems a little fishy,
as these GRF registers are then never cleared again if it's not in OTG
mode anymore. I'll look into it further, including cross-checking what
those bits actually do based on the TRMs of various SoCs that use this
hardware.

> 
> Neil
> 
> > +			vbus_attach = true;
> > +		} else {
> > +			vbus_attach = false;
> > +		}
> > +	} else {
> > +		vbus_attach = property_enabled(rphy->grf,
> > +					       &rport->port_cfg->utmi_bvalid);
> > +	}
> >   
> >   	sch_work = false;
> >   	notify_charger = false;
> > @@ -1276,6 +1298,83 @@ static int rockchip_otg_event(struct notifier_block *nb,
> >   	return NOTIFY_DONE;
> >   }
> >   
> > +static const char *const rockchip_usb2phy_typec_cons[] = {
> > +	"usb-c-connector",
> > +	NULL,
> > +};
> > +
> > +static struct device_node *rockchip_usb2phy_to_controller(struct rockchip_usb2phy *rphy)
> > +{
> > +	struct device_node *np;
> > +	struct device_node *parent;
> > +
> > +	for_each_node_with_property(np, "phys") {
> > +		struct of_phandle_iterator it;
> > +		int ret;
> > +
> > +		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
> > +			parent = of_get_parent(it.node);
> > +			if (it.node != rphy->dev->of_node && rphy->dev->of_node != parent) {
> > +				if (parent)
> > +					of_node_put(parent);
> > +				continue;
> > +			}
> > +
> > +			/*
> > +			 * Either the PHY phandle we're iterating or its parent
> > +			 * matched, we don't care about which out of the two in
> > +			 * particular as we just need to know it's the right
> > +			 * USB controller for this PHY.
> > +			 */
> > +			of_node_put(it.node);
> > +			of_node_put(parent);
> > +			return np;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool rockchip_usb2phy_otg_is_type_c(struct rockchip_usb2phy *rphy)
> > +{
> > +	struct device_node *controller = rockchip_usb2phy_to_controller(rphy);
> > +	struct device_node *ports;
> > +	struct device_node *ep = NULL;
> > +	struct device_node *parent;
> > +
> > +	if (!controller)
> > +		return false;
> > +
> > +	ports = of_get_child_by_name(controller, "ports");
> > +	if (ports) {
> > +		of_node_put(controller);
> > +		controller = ports;
> > +	}
> > +
> > +	for_each_of_graph_port(controller, port) {
> > +		ep = of_get_child_by_name(port, "endpoint");
> > +		if (!ep)
> > +			continue;
> > +
> > +		parent = of_graph_get_remote_port_parent(ep);
> > +		of_node_put(ep);
> > +		if (!parent)
> > +			continue;
> > +
> > +		if (of_device_compatible_match(parent, rockchip_usb2phy_typec_cons)) {
> > +			of_node_put(parent);
> > +			of_node_put(controller);
> > +			return true;
> > +		}
> > +
> > +		of_node_put(parent);
> > +	}
> > +
> > +	of_node_put(controller);
> > +
> > +	return false;
> > +}
> 
> This DT parsing is quite complex, but seems it's the only way to detect such features
> without bindings change, not sure it's really beneficial.

Yeah, v1 abused the existing connector binding by having the HS graph
port point to the PHY instead, and Rob wasn't happy with that. He
suggested this can actually be done just by walking the graph.

FWIW, even if the PHY knew its controller, that would still only get rid
of rockchip_usb2phy_to_controller, and leave the graph walking to get to
the connector in place.

So a bindings change would only be of measurable benefit if it added a
phandle property in USB PHYs to the connector or something, and I'm not
sure if that's the right solution either. It'd once again shift the
burden of remembering to let PHYs know about their connectors onto board
DTs, which forget such things all the time.

The benefit of this solution is that board DTs don't need to think about
this at all, and the search is unambiguous, as we really don't lack any
information, we just lack a direct view of that information in the node
our driver receives.

> 
> > +
> >   static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> >   					  struct rockchip_usb2phy_port *rport,
> >   					  struct device_node *child_np)
> > @@ -1297,6 +1396,8 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> >   
> >   	mutex_init(&rport->mutex);
> >   
> > +	rport->typec_vbus_det = rockchip_usb2phy_otg_is_type_c(rphy);
> > +
> >   	rport->mode = of_usb_get_dr_mode_by_phy(child_np, -1);
> >   	if (rport->mode == USB_DR_MODE_HOST ||
> >   	    rport->mode == USB_DR_MODE_UNKNOWN) {
> > @@ -2050,6 +2151,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
> >   		.port_cfgs	= {
> >   			[USB2PHY_PORT_OTG] = {
> >   				.phy_sus	= { 0x0000, 8, 0, 0, 0x1d1 },
> > +				.svbus_en	= { 0x0000, 15, 15, 0, 1 },
> > +				.svbus_sel	= { 0x0000, 14, 14, 0, 1 },
> >   				.bvalid_det_en	= { 0x00c0, 1, 1, 0, 1 },
> >   				.bvalid_det_st	= { 0x00c4, 1, 1, 0, 1 },
> >   				.bvalid_det_clr = { 0x00c8, 1, 1, 0, 1 },
> > @@ -2087,6 +2190,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
> >   		.port_cfgs	= {
> >   			[USB2PHY_PORT_OTG] = {
> >   				.phy_sus	= { 0x2000, 8, 0, 0, 0x1d1 },
> > +				.svbus_en	= { 0x2000, 15, 15, 0, 1 },
> > +				.svbus_sel	= { 0x2000, 14, 14, 0, 1 },
> >   				.bvalid_det_en	= { 0x20c0, 1, 1, 0, 1 },
> >   				.bvalid_det_st	= { 0x20c4, 1, 1, 0, 1 },
> >   				.bvalid_det_clr = { 0x20c8, 1, 1, 0, 1 },
> > 
> 
> Thanks,
> Neil
> 

Kind regards,
Nicolas Frattaroli

Re: [PATCH v4 1/4] phy: rockchip: inno-usb2: add soft vbusvalid control

[Nicolas Frattaroli]

On Friday, 13 June 2025 11:02:40 Central European Summer Time neil.armstrong@linaro.org wrote:
> On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> > With USB type C connectors, the vbus detect pin of the OTG controller
> > attached to it is pulled high by a USB Type C controller chip such as
> > the fusb302. This means USB enumeration on Type-C ports never works, as
> > the vbus is always seen as high.
> > 
> > Rockchip added some GRF register flags to deal with this situation. The
> > RK3576 TRM calls these "soft_vbusvalid_bvalid" (con0 bit index 15) and
> > "soft_vbusvalid_bvalid_sel" (con0 bit index 14).
> > 
> > Downstream introduces a new vendor property which tells the USB 2 PHY
> > that it's connected to a type C port, but we can do better. Since in
> > such an arrangement, we'll have an OF graph connection from the USB
> > controller to the USB connector anyway, we can walk said OF graph and
> > check the connector's compatible to determine this without adding any
> > further vendor properties.
> > 
> > Do keep in mind that the usbdp PHY driver seemingly fiddles with these
> > register fields as well, but what it does doesn't appear to be enough
> > for us to get working USB enumeration, presumably because the whole
> > vbus_attach logic needs to be adjusted as well either way.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 113 +++++++++++++++++++++++++-
> >   1 file changed, 109 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index b0f23690ec3002202c0f33a6988f5509622fa10e..4f89bd6568cd3a7a1d2c10e9cddda9f3bd997ed0 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > [...]
> > @@ -666,8 +679,17 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> >   	unsigned long delay;
> >   	bool vbus_attach, sch_work, notify_charger;
> >   
> > -	vbus_attach = property_enabled(rphy->grf,
> > -				       &rport->port_cfg->utmi_bvalid);
> > +	if (rport->port_cfg->svbus_en.enable && rport->typec_vbus_det) {
> > +		if (property_enabled(rphy->grf, &rport->port_cfg->svbus_en) &&
> > +		    property_enabled(rphy->grf, &rport->port_cfg->svbus_sel)) {
> 
> Why do you check the registers since you always enable those bits on those conditions:
> 	rport->port_id == USB2PHY_PORT_OTG
> 	rport->typec_vbus_det
> 	rport->port_cfg->svbus_en.enable
> 	rport->typec_vbus_det
> Can't you us them instead ?

I did some more looking into this, and agree that I can drop the
property_enabled lines here. The other concern I had immediately (that
the bits never get turned off, which seemed fishy) isn't a concern after
all, because after sleeping on it some more, I realised that it's probably
not very likely that a USB-C connector is ever going to morph into anything
else spontaneously.

Thank you for spotting this!

> 
> Neil
> 
> [...]

Kind regards,
Nicolas Frattaroli

[PATCH v4 2/4] phy: rockchip: usbdp: move orientation handling further down

[Nicolas Frattaroli]

The orientation handling code isn't referenced until very far down the
driver code.

Move it down some ways so it can later reference other driver functions
without needing forward declarations.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-usbdp.c | 100 +++++++++++++++---------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index c066cc0a7b4f10fc3cd8779323c369360893520d..fff54900feea601c8fe6bf4c7123dfebc5661a15 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -651,56 +651,6 @@ static void rk_udphy_set_typec_default_mapping(struct rk_udphy *udphy)
 	udphy->mode = UDPHY_MODE_DP_USB;
 }
 
-static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
-				 enum typec_orientation orien)
-{
-	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
-
-	mutex_lock(&udphy->mutex);
-
-	if (orien == TYPEC_ORIENTATION_NONE) {
-		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
-		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
-		/* unattached */
-		rk_udphy_usb_bvalid_enable(udphy, false);
-		goto unlock_ret;
-	}
-
-	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
-	rk_udphy_set_typec_default_mapping(udphy);
-	rk_udphy_usb_bvalid_enable(udphy, true);
-
-unlock_ret:
-	mutex_unlock(&udphy->mutex);
-	return 0;
-}
-
-static void rk_udphy_orien_switch_unregister(void *data)
-{
-	struct rk_udphy *udphy = data;
-
-	typec_switch_unregister(udphy->sw);
-}
-
-static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
-{
-	struct typec_switch_desc sw_desc = { };
-
-	sw_desc.drvdata = udphy;
-	sw_desc.fwnode = dev_fwnode(udphy->dev);
-	sw_desc.set = rk_udphy_orien_sw_set;
-
-	udphy->sw = typec_switch_register(udphy->dev, &sw_desc);
-	if (IS_ERR(udphy->sw)) {
-		dev_err(udphy->dev, "Error register typec orientation switch: %ld\n",
-			PTR_ERR(udphy->sw));
-		return PTR_ERR(udphy->sw);
-	}
-
-	return devm_add_action_or_reset(udphy->dev,
-					rk_udphy_orien_switch_unregister, udphy);
-}
-
 static int rk_udphy_refclk_set(struct rk_udphy *udphy)
 {
 	unsigned long rate;
@@ -1451,6 +1401,56 @@ static struct phy *rk_udphy_phy_xlate(struct device *dev, const struct of_phandl
 	return ERR_PTR(-EINVAL);
 }
 
+static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
+				 enum typec_orientation orien)
+{
+	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
+
+	mutex_lock(&udphy->mutex);
+
+	if (orien == TYPEC_ORIENTATION_NONE) {
+		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
+		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
+		/* unattached */
+		rk_udphy_usb_bvalid_enable(udphy, false);
+		goto unlock_ret;
+	}
+
+	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
+	rk_udphy_set_typec_default_mapping(udphy);
+	rk_udphy_usb_bvalid_enable(udphy, true);
+
+unlock_ret:
+	mutex_unlock(&udphy->mutex);
+	return 0;
+}
+
+static void rk_udphy_orien_switch_unregister(void *data)
+{
+	struct rk_udphy *udphy = data;
+
+	typec_switch_unregister(udphy->sw);
+}
+
+static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
+{
+	struct typec_switch_desc sw_desc = { };
+
+	sw_desc.drvdata = udphy;
+	sw_desc.fwnode = dev_fwnode(udphy->dev);
+	sw_desc.set = rk_udphy_orien_sw_set;
+
+	udphy->sw = typec_switch_register(udphy->dev, &sw_desc);
+	if (IS_ERR(udphy->sw)) {
+		dev_err(udphy->dev, "Error register typec orientation switch: %ld\n",
+			PTR_ERR(udphy->sw));
+		return PTR_ERR(udphy->sw);
+	}
+
+	return devm_add_action_or_reset(udphy->dev,
+					rk_udphy_orien_switch_unregister, udphy);
+}
+
 static int rk_udphy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

-- 
2.49.0

Re: [PATCH v4 2/4] phy: rockchip: usbdp: move orientation handling further down

[neil.armstrong]

On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> The orientation handling code isn't referenced until very far down the
> driver code.
> 
> Move it down some ways so it can later reference other driver functions
> without needing forward declarations.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   drivers/phy/rockchip/phy-rockchip-usbdp.c | 100 +++++++++++++++---------------
>   1 file changed, 50 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index c066cc0a7b4f10fc3cd8779323c369360893520d..fff54900feea601c8fe6bf4c7123dfebc5661a15 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -651,56 +651,6 @@ static void rk_udphy_set_typec_default_mapping(struct rk_udphy *udphy)
>   	udphy->mode = UDPHY_MODE_DP_USB;
>   }
>   
> -static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
> -				 enum typec_orientation orien)
> -{
> -	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> -
> -	mutex_lock(&udphy->mutex);
> -
> -	if (orien == TYPEC_ORIENTATION_NONE) {
> -		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
> -		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
> -		/* unattached */
> -		rk_udphy_usb_bvalid_enable(udphy, false);
> -		goto unlock_ret;
> -	}
> -
> -	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
> -	rk_udphy_set_typec_default_mapping(udphy);
> -	rk_udphy_usb_bvalid_enable(udphy, true);
> -
> -unlock_ret:
> -	mutex_unlock(&udphy->mutex);
> -	return 0;
> -}
> -
> -static void rk_udphy_orien_switch_unregister(void *data)
> -{
> -	struct rk_udphy *udphy = data;
> -
> -	typec_switch_unregister(udphy->sw);
> -}
> -
> -static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
> -{
> -	struct typec_switch_desc sw_desc = { };
> -
> -	sw_desc.drvdata = udphy;
> -	sw_desc.fwnode = dev_fwnode(udphy->dev);
> -	sw_desc.set = rk_udphy_orien_sw_set;
> -
> -	udphy->sw = typec_switch_register(udphy->dev, &sw_desc);
> -	if (IS_ERR(udphy->sw)) {
> -		dev_err(udphy->dev, "Error register typec orientation switch: %ld\n",
> -			PTR_ERR(udphy->sw));
> -		return PTR_ERR(udphy->sw);
> -	}
> -
> -	return devm_add_action_or_reset(udphy->dev,
> -					rk_udphy_orien_switch_unregister, udphy);
> -}
> -
>   static int rk_udphy_refclk_set(struct rk_udphy *udphy)
>   {
>   	unsigned long rate;
> @@ -1451,6 +1401,56 @@ static struct phy *rk_udphy_phy_xlate(struct device *dev, const struct of_phandl
>   	return ERR_PTR(-EINVAL);
>   }
>   
> +static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
> +				 enum typec_orientation orien)
> +{
> +	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> +
> +	mutex_lock(&udphy->mutex);
> +
> +	if (orien == TYPEC_ORIENTATION_NONE) {
> +		gpiod_set_value_cansleep(udphy->sbu1_dc_gpio, 0);
> +		gpiod_set_value_cansleep(udphy->sbu2_dc_gpio, 0);
> +		/* unattached */
> +		rk_udphy_usb_bvalid_enable(udphy, false);
> +		goto unlock_ret;
> +	}
> +
> +	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
> +	rk_udphy_set_typec_default_mapping(udphy);
> +	rk_udphy_usb_bvalid_enable(udphy, true);
> +
> +unlock_ret:
> +	mutex_unlock(&udphy->mutex);
> +	return 0;
> +}
> +
> +static void rk_udphy_orien_switch_unregister(void *data)
> +{
> +	struct rk_udphy *udphy = data;
> +
> +	typec_switch_unregister(udphy->sw);
> +}
> +
> +static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
> +{
> +	struct typec_switch_desc sw_desc = { };
> +
> +	sw_desc.drvdata = udphy;
> +	sw_desc.fwnode = dev_fwnode(udphy->dev);
> +	sw_desc.set = rk_udphy_orien_sw_set;
> +
> +	udphy->sw = typec_switch_register(udphy->dev, &sw_desc);
> +	if (IS_ERR(udphy->sw)) {
> +		dev_err(udphy->dev, "Error register typec orientation switch: %ld\n",
> +			PTR_ERR(udphy->sw));
> +		return PTR_ERR(udphy->sw);
> +	}
> +
> +	return devm_add_action_or_reset(udphy->dev,
> +					rk_udphy_orien_switch_unregister, udphy);
> +}
> +
>   static int rk_udphy_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH v4 3/4] phy: rockchip: usbdp: reset USB3 and reinit on orientation switch

[Nicolas Frattaroli]

Until now, super speed on Type-C only worked in one orientation. This is
because on an orientation switch, the UDPHY was never reinitialised.

Heiko presented a patch to do this[1], but there were concerns over the
correctness of it[2]. Experimentally using Heiko's patch on RK3576 did
make me run into issues, though they seemed to be related to the
orientation switch actually happening while a clock driving a GRF
register was disabled.

The key issue is that the hardware wants the USB 3 controller to be held
in reset while the PHY is being reconfigured, otherwise we may run into
hard-to-catch race conditions.

Either way, this patch implements the required ordering in a somewhat
unpleasant way: we get the USB 3 controller from the DT, and use runtime
power management to forcibly suspend it while the UDPHY is being
reconfigured, and then forcibly resume it later. As an added pain in the
rear, the suspend/resume of the USB 3 controller also tries fiddling
with the USB 3 PHY part of the UDPHY, which means we introduce an atomic
flag to skip suspending/resuming the UDPHY if we're resetting the USB 3
controller. We may just need to skip trying to acquire the mutex again,
but both ways work for me in practice.

This solution may in fact be complete rubbish, but it works to get USB 3
Super Speed working in both cable orientations on my board.

Link: https://lore.kernel.org/all/20250226103810.3746018-3-heiko@sntech.de/ [1]
Link: https://lore.kernel.org/linux-rockchip/h57ok2hw6os7bcafqkrqknfvm7hnu25m2oe54qmrsuzdwqlos3@m4och2fcdm7s/ [2]
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/phy/rockchip/phy-rockchip-usbdp.c | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index fff54900feea601c8fe6bf4c7123dfebc5661a15..5cd6bbc367f69bca15c2a94a07e72f850b381ae3 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -200,6 +200,10 @@ struct rk_udphy {
 	/* PHY devices */
 	struct phy *phy_dp;
 	struct phy *phy_u3;
+
+	/* USB 3 controller device */
+	struct device *ctrl_u3;
+	atomic_t ctrl_resetting;
 };
 
 static const struct rk_udphy_dp_tx_drv_ctrl rk3588_dp_tx_drv_ctrl_rbr_hbr[4][4] = {
@@ -1255,6 +1259,9 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
 	struct rk_udphy *udphy = phy_get_drvdata(phy);
 	int ret = 0;
 
+	if (atomic_read(&udphy->ctrl_resetting))
+		return 0;
+
 	mutex_lock(&udphy->mutex);
 	/* DP only or high-speed, disable U3 port */
 	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
@@ -1273,6 +1280,9 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
 {
 	struct rk_udphy *udphy = phy_get_drvdata(phy);
 
+	if (atomic_read(&udphy->ctrl_resetting))
+		return 0;
+
 	mutex_lock(&udphy->mutex);
 	/* DP only or high-speed */
 	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
@@ -1401,10 +1411,31 @@ static struct phy *rk_udphy_phy_xlate(struct device *dev, const struct of_phandl
 	return ERR_PTR(-EINVAL);
 }
 
+static struct device_node *rk_udphy_to_controller(struct rk_udphy *udphy)
+{
+	struct device_node *np;
+
+	for_each_node_with_property(np, "phys") {
+		struct of_phandle_iterator it;
+		int ret;
+
+		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
+			if (it.node != udphy->dev->of_node)
+				continue;
+
+			of_node_put(it.node);
+			return np;
+		}
+	}
+
+	return NULL;
+}
+
 static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
 				 enum typec_orientation orien)
 {
 	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
+	int ret;
 
 	mutex_lock(&udphy->mutex);
 
@@ -1420,6 +1451,18 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
 	rk_udphy_set_typec_default_mapping(udphy);
 	rk_udphy_usb_bvalid_enable(udphy, true);
 
+	if (udphy->status != UDPHY_MODE_NONE && udphy->ctrl_u3) {
+		atomic_set(&udphy->ctrl_resetting, 1);
+		pm_runtime_force_suspend(udphy->ctrl_u3);
+
+		ret = rk_udphy_setup(udphy);
+		if (!ret)
+			clk_bulk_disable_unprepare(udphy->num_clks, udphy->clks);
+
+		pm_runtime_force_resume(udphy->ctrl_u3);
+		atomic_set(&udphy->ctrl_resetting, 0);
+	}
+
 unlock_ret:
 	mutex_unlock(&udphy->mutex);
 	return 0;
@@ -1430,12 +1473,22 @@ static void rk_udphy_orien_switch_unregister(void *data)
 	struct rk_udphy *udphy = data;
 
 	typec_switch_unregister(udphy->sw);
+	put_device(udphy->ctrl_u3);
 }
 
 static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
 {
+	struct device_node *ctrl = rk_udphy_to_controller(udphy);
 	struct typec_switch_desc sw_desc = { };
 
+	if (ctrl) {
+		udphy->ctrl_u3 = bus_find_device_by_of_node(udphy->dev->bus, ctrl);
+		of_node_put(ctrl);
+	}
+
+	if (!udphy->ctrl_u3)
+		dev_info(udphy->dev, "couldn't find this PHY's USB3 controller\n");
+
 	sw_desc.drvdata = udphy;
 	sw_desc.fwnode = dev_fwnode(udphy->dev);
 	sw_desc.set = rk_udphy_orien_sw_set;
@@ -1499,6 +1552,7 @@ static int rk_udphy_probe(struct platform_device *pdev)
 		return ret;
 
 	mutex_init(&udphy->mutex);
+	atomic_set(&udphy->ctrl_resetting, 0);
 	platform_set_drvdata(pdev, udphy);
 
 	if (device_property_present(dev, "orientation-switch")) {

-- 
2.49.0

Re: [PATCH v4 3/4] phy: rockchip: usbdp: reset USB3 and reinit on orientation switch

[neil.armstrong]

Hi,

On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> Until now, super speed on Type-C only worked in one orientation. This is
> because on an orientation switch, the UDPHY was never reinitialised.
> 
> Heiko presented a patch to do this[1], but there were concerns over the
> correctness of it[2]. Experimentally using Heiko's patch on RK3576 did
> make me run into issues, though they seemed to be related to the
> orientation switch actually happening while a clock driving a GRF
> register was disabled.
> 
> The key issue is that the hardware wants the USB 3 controller to be held
> in reset while the PHY is being reconfigured, otherwise we may run into
> hard-to-catch race conditions.
> 
> Either way, this patch implements the required ordering in a somewhat
> unpleasant way: we get the USB 3 controller from the DT, and use runtime
> power management to forcibly suspend it while the UDPHY is being
> reconfigured, and then forcibly resume it later. As an added pain in the
> rear, the suspend/resume of the USB 3 controller also tries fiddling
> with the USB 3 PHY part of the UDPHY, which means we introduce an atomic
> flag to skip suspending/resuming the UDPHY if we're resetting the USB 3
> controller. We may just need to skip trying to acquire the mutex again,
> but both ways work for me in practice.
> 
> This solution may in fact be complete rubbish, but it works to get USB 3
> Super Speed working in both cable orientations on my board.

Yeah this is kind of a hack, and we have a similar situation on Qualcomm platforms
when dealing with USB2-only and DP-only altmodes, where we need the DWC3 to
disable the usb3 path. Hopefully on Qcom combo PHYs we can switch lanes without
disable the USB3 PHY.

So the proper solution would have the dwc3 driver to also react to mux and
orientation events and disable the usb3 path to allow the phy to reconfigure
itself.

We don't have any concrete proposal yet, but we will get something soonish.

Neil

> 
> Link: https://lore.kernel.org/all/20250226103810.3746018-3-heiko@sntech.de/ [1]
> Link: https://lore.kernel.org/linux-rockchip/h57ok2hw6os7bcafqkrqknfvm7hnu25m2oe54qmrsuzdwqlos3@m4och2fcdm7s/ [2]
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   drivers/phy/rockchip/phy-rockchip-usbdp.c | 54 +++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index fff54900feea601c8fe6bf4c7123dfebc5661a15..5cd6bbc367f69bca15c2a94a07e72f850b381ae3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -200,6 +200,10 @@ struct rk_udphy {
>   	/* PHY devices */
>   	struct phy *phy_dp;
>   	struct phy *phy_u3;
> +
> +	/* USB 3 controller device */
> +	struct device *ctrl_u3;
> +	atomic_t ctrl_resetting;
>   };
>   
>   static const struct rk_udphy_dp_tx_drv_ctrl rk3588_dp_tx_drv_ctrl_rbr_hbr[4][4] = {
> @@ -1255,6 +1259,9 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
>   	struct rk_udphy *udphy = phy_get_drvdata(phy);
>   	int ret = 0;
>   
> +	if (atomic_read(&udphy->ctrl_resetting))
> +		return 0;
> +
>   	mutex_lock(&udphy->mutex);
>   	/* DP only or high-speed, disable U3 port */
>   	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
> @@ -1273,6 +1280,9 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>   {
>   	struct rk_udphy *udphy = phy_get_drvdata(phy);
>   
> +	if (atomic_read(&udphy->ctrl_resetting))
> +		return 0;
> +
>   	mutex_lock(&udphy->mutex);
>   	/* DP only or high-speed */
>   	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
> @@ -1401,10 +1411,31 @@ static struct phy *rk_udphy_phy_xlate(struct device *dev, const struct of_phandl
>   	return ERR_PTR(-EINVAL);
>   }
>   
> +static struct device_node *rk_udphy_to_controller(struct rk_udphy *udphy)
> +{
> +	struct device_node *np;
> +
> +	for_each_node_with_property(np, "phys") {
> +		struct of_phandle_iterator it;
> +		int ret;
> +
> +		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
> +			if (it.node != udphy->dev->of_node)
> +				continue;
> +
> +			of_node_put(it.node);
> +			return np;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>   static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>   				 enum typec_orientation orien)
>   {
>   	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> +	int ret;
>   
>   	mutex_lock(&udphy->mutex);
>   
> @@ -1420,6 +1451,18 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>   	rk_udphy_set_typec_default_mapping(udphy);
>   	rk_udphy_usb_bvalid_enable(udphy, true);
>   
> +	if (udphy->status != UDPHY_MODE_NONE && udphy->ctrl_u3) {
> +		atomic_set(&udphy->ctrl_resetting, 1);
> +		pm_runtime_force_suspend(udphy->ctrl_u3);
> +
> +		ret = rk_udphy_setup(udphy);
> +		if (!ret)
> +			clk_bulk_disable_unprepare(udphy->num_clks, udphy->clks);
> +
> +		pm_runtime_force_resume(udphy->ctrl_u3);
> +		atomic_set(&udphy->ctrl_resetting, 0);
> +	}
> +
>   unlock_ret:
>   	mutex_unlock(&udphy->mutex);
>   	return 0;
> @@ -1430,12 +1473,22 @@ static void rk_udphy_orien_switch_unregister(void *data)
>   	struct rk_udphy *udphy = data;
>   
>   	typec_switch_unregister(udphy->sw);
> +	put_device(udphy->ctrl_u3);
>   }
>   
>   static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
>   {
> +	struct device_node *ctrl = rk_udphy_to_controller(udphy);
>   	struct typec_switch_desc sw_desc = { };
>   
> +	if (ctrl) {
> +		udphy->ctrl_u3 = bus_find_device_by_of_node(udphy->dev->bus, ctrl);
> +		of_node_put(ctrl);
> +	}
> +
> +	if (!udphy->ctrl_u3)
> +		dev_info(udphy->dev, "couldn't find this PHY's USB3 controller\n");
> +
>   	sw_desc.drvdata = udphy;
>   	sw_desc.fwnode = dev_fwnode(udphy->dev);
>   	sw_desc.set = rk_udphy_orien_sw_set;
> @@ -1499,6 +1552,7 @@ static int rk_udphy_probe(struct platform_device *pdev)
>   		return ret;
>   
>   	mutex_init(&udphy->mutex);
> +	atomic_set(&udphy->ctrl_resetting, 0);
>   	platform_set_drvdata(pdev, udphy);
>   
>   	if (device_property_present(dev, "orientation-switch")) {
> 

Re: [PATCH v4 3/4] phy: rockchip: usbdp: reset USB3 and reinit on orientation switch

[Nicolas Frattaroli]

On Friday, 13 June 2025 10:55:43 Central European Summer Time neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> > Until now, super speed on Type-C only worked in one orientation. This is
> > because on an orientation switch, the UDPHY was never reinitialised.
> > 
> > Heiko presented a patch to do this[1], but there were concerns over the
> > correctness of it[2]. Experimentally using Heiko's patch on RK3576 did
> > make me run into issues, though they seemed to be related to the
> > orientation switch actually happening while a clock driving a GRF
> > register was disabled.
> > 
> > The key issue is that the hardware wants the USB 3 controller to be held
> > in reset while the PHY is being reconfigured, otherwise we may run into
> > hard-to-catch race conditions.
> > 
> > Either way, this patch implements the required ordering in a somewhat
> > unpleasant way: we get the USB 3 controller from the DT, and use runtime
> > power management to forcibly suspend it while the UDPHY is being
> > reconfigured, and then forcibly resume it later. As an added pain in the
> > rear, the suspend/resume of the USB 3 controller also tries fiddling
> > with the USB 3 PHY part of the UDPHY, which means we introduce an atomic
> > flag to skip suspending/resuming the UDPHY if we're resetting the USB 3
> > controller. We may just need to skip trying to acquire the mutex again,
> > but both ways work for me in practice.
> > 
> > This solution may in fact be complete rubbish, but it works to get USB 3
> > Super Speed working in both cable orientations on my board.
> 
> Yeah this is kind of a hack, and we have a similar situation on Qualcomm platforms
> when dealing with USB2-only and DP-only altmodes, where we need the DWC3 to
> disable the usb3 path. Hopefully on Qcom combo PHYs we can switch lanes without
> disable the USB3 PHY.
> 
> So the proper solution would have the dwc3 driver to also react to mux and
> orientation events and disable the usb3 path to allow the phy to reconfigure
> itself.
> 
> We don't have any concrete proposal yet, but we will get something soonish.

Thanks you for the additional insight, I do agree that having the
controller be in charge of this is the better solution. That way, the PHY
doesn't need to know its controller, and the controller driver is in charge
of ordering the resets and reconfigure right, which means the ugly reset
loop breaking in this patch isn't needed.

If you do end up sending a patch series with a proposal, please +Cc me,
so that I can see if it also works for Rockchip.

If the hack isn't acceptable to be merged until then, then I'll drop
this patch from future revisions of the series to get the other parts
in.

> 
> Neil

Best regards,
Nicolas Frattaroli

> 
> > 
> > Link: https://lore.kernel.org/all/20250226103810.3746018-3-heiko@sntech.de/ [1]
> > Link: https://lore.kernel.org/linux-rockchip/h57ok2hw6os7bcafqkrqknfvm7hnu25m2oe54qmrsuzdwqlos3@m4och2fcdm7s/ [2]
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   drivers/phy/rockchip/phy-rockchip-usbdp.c | 54 +++++++++++++++++++++++++++++++
> >   1 file changed, 54 insertions(+)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> > index fff54900feea601c8fe6bf4c7123dfebc5661a15..5cd6bbc367f69bca15c2a94a07e72f850b381ae3 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> > @@ -200,6 +200,10 @@ struct rk_udphy {
> >   	/* PHY devices */
> >   	struct phy *phy_dp;
> >   	struct phy *phy_u3;
> > +
> > +	/* USB 3 controller device */
> > +	struct device *ctrl_u3;
> > +	atomic_t ctrl_resetting;
> >   };
> >   
> >   static const struct rk_udphy_dp_tx_drv_ctrl rk3588_dp_tx_drv_ctrl_rbr_hbr[4][4] = {
> > @@ -1255,6 +1259,9 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
> >   	struct rk_udphy *udphy = phy_get_drvdata(phy);
> >   	int ret = 0;
> >   
> > +	if (atomic_read(&udphy->ctrl_resetting))
> > +		return 0;
> > +
> >   	mutex_lock(&udphy->mutex);
> >   	/* DP only or high-speed, disable U3 port */
> >   	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
> > @@ -1273,6 +1280,9 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
> >   {
> >   	struct rk_udphy *udphy = phy_get_drvdata(phy);
> >   
> > +	if (atomic_read(&udphy->ctrl_resetting))
> > +		return 0;
> > +
> >   	mutex_lock(&udphy->mutex);
> >   	/* DP only or high-speed */
> >   	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
> > @@ -1401,10 +1411,31 @@ static struct phy *rk_udphy_phy_xlate(struct device *dev, const struct of_phandl
> >   	return ERR_PTR(-EINVAL);
> >   }
> >   
> > +static struct device_node *rk_udphy_to_controller(struct rk_udphy *udphy)
> > +{
> > +	struct device_node *np;
> > +
> > +	for_each_node_with_property(np, "phys") {
> > +		struct of_phandle_iterator it;
> > +		int ret;
> > +
> > +		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
> > +			if (it.node != udphy->dev->of_node)
> > +				continue;
> > +
> > +			of_node_put(it.node);
> > +			return np;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >   static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
> >   				 enum typec_orientation orien)
> >   {
> >   	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> > +	int ret;
> >   
> >   	mutex_lock(&udphy->mutex);
> >   
> > @@ -1420,6 +1451,18 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
> >   	rk_udphy_set_typec_default_mapping(udphy);
> >   	rk_udphy_usb_bvalid_enable(udphy, true);
> >   
> > +	if (udphy->status != UDPHY_MODE_NONE && udphy->ctrl_u3) {
> > +		atomic_set(&udphy->ctrl_resetting, 1);
> > +		pm_runtime_force_suspend(udphy->ctrl_u3);
> > +
> > +		ret = rk_udphy_setup(udphy);
> > +		if (!ret)
> > +			clk_bulk_disable_unprepare(udphy->num_clks, udphy->clks);
> > +
> > +		pm_runtime_force_resume(udphy->ctrl_u3);
> > +		atomic_set(&udphy->ctrl_resetting, 0);
> > +	}
> > +
> >   unlock_ret:
> >   	mutex_unlock(&udphy->mutex);
> >   	return 0;
> > @@ -1430,12 +1473,22 @@ static void rk_udphy_orien_switch_unregister(void *data)
> >   	struct rk_udphy *udphy = data;
> >   
> >   	typec_switch_unregister(udphy->sw);
> > +	put_device(udphy->ctrl_u3);
> >   }
> >   
> >   static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
> >   {
> > +	struct device_node *ctrl = rk_udphy_to_controller(udphy);
> >   	struct typec_switch_desc sw_desc = { };
> >   
> > +	if (ctrl) {
> > +		udphy->ctrl_u3 = bus_find_device_by_of_node(udphy->dev->bus, ctrl);
> > +		of_node_put(ctrl);
> > +	}
> > +
> > +	if (!udphy->ctrl_u3)
> > +		dev_info(udphy->dev, "couldn't find this PHY's USB3 controller\n");
> > +
> >   	sw_desc.drvdata = udphy;
> >   	sw_desc.fwnode = dev_fwnode(udphy->dev);
> >   	sw_desc.set = rk_udphy_orien_sw_set;
> > @@ -1499,6 +1552,7 @@ static int rk_udphy_probe(struct platform_device *pdev)
> >   		return ret;
> >   
> >   	mutex_init(&udphy->mutex);
> > +	atomic_set(&udphy->ctrl_resetting, 0);
> >   	platform_set_drvdata(pdev, udphy);
> >   
> >   	if (device_property_present(dev, "orientation-switch")) {
> > 
> 
> 

Re: [PATCH v4 3/4] phy: rockchip: usbdp: reset USB3 and reinit on orientation switch

[Neil Armstrong]

On 13/06/2025 15:21, Nicolas Frattaroli wrote:
> On Friday, 13 June 2025 10:55:43 Central European Summer Time neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 10/06/2025 16:07, Nicolas Frattaroli wrote:
>>> Until now, super speed on Type-C only worked in one orientation. This is
>>> because on an orientation switch, the UDPHY was never reinitialised.
>>>
>>> Heiko presented a patch to do this[1], but there were concerns over the
>>> correctness of it[2]. Experimentally using Heiko's patch on RK3576 did
>>> make me run into issues, though they seemed to be related to the
>>> orientation switch actually happening while a clock driving a GRF
>>> register was disabled.
>>>
>>> The key issue is that the hardware wants the USB 3 controller to be held
>>> in reset while the PHY is being reconfigured, otherwise we may run into
>>> hard-to-catch race conditions.
>>>
>>> Either way, this patch implements the required ordering in a somewhat
>>> unpleasant way: we get the USB 3 controller from the DT, and use runtime
>>> power management to forcibly suspend it while the UDPHY is being
>>> reconfigured, and then forcibly resume it later. As an added pain in the
>>> rear, the suspend/resume of the USB 3 controller also tries fiddling
>>> with the USB 3 PHY part of the UDPHY, which means we introduce an atomic
>>> flag to skip suspending/resuming the UDPHY if we're resetting the USB 3
>>> controller. We may just need to skip trying to acquire the mutex again,
>>> but both ways work for me in practice.
>>>
>>> This solution may in fact be complete rubbish, but it works to get USB 3
>>> Super Speed working in both cable orientations on my board.
>>
>> Yeah this is kind of a hack, and we have a similar situation on Qualcomm platforms
>> when dealing with USB2-only and DP-only altmodes, where we need the DWC3 to
>> disable the usb3 path. Hopefully on Qcom combo PHYs we can switch lanes without
>> disable the USB3 PHY.
>>
>> So the proper solution would have the dwc3 driver to also react to mux and
>> orientation events and disable the usb3 path to allow the phy to reconfigure
>> itself.
>>
>> We don't have any concrete proposal yet, but we will get something soonish.
> 
> Thanks you for the additional insight, I do agree that having the
> controller be in charge of this is the better solution. That way, the PHY
> doesn't need to know its controller, and the controller driver is in charge
> of ordering the resets and reconfigure right, which means the ugly reset
> loop breaking in this patch isn't needed.
> 
> If you do end up sending a patch series with a proposal, please +Cc me,
> so that I can see if it also works for Rockchip.

Of course I'll add you in CC if this happens
Neil

> 
> If the hack isn't acceptable to be merged until then, then I'll drop
> this patch from future revisions of the series to get the other parts
> in.
> 
>>
>> Neil
> 
> Best regards,
> Nicolas Frattaroli
> 
>>
>>>
>>> Link: https://lore.kernel.org/all/20250226103810.3746018-3-heiko@sntech.de/ [1]
>>> Link: https://lore.kernel.org/linux-rockchip/h57ok2hw6os7bcafqkrqknfvm7hnu25m2oe54qmrsuzdwqlos3@m4och2fcdm7s/ [2]
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>> ---
>>>    drivers/phy/rockchip/phy-rockchip-usbdp.c | 54 +++++++++++++++++++++++++++++++
>>>    1 file changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>>> index fff54900feea601c8fe6bf4c7123dfebc5661a15..5cd6bbc367f69bca15c2a94a07e72f850b381ae3 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>>> @@ -200,6 +200,10 @@ struct rk_udphy {
>>>    	/* PHY devices */
>>>    	struct phy *phy_dp;
>>>    	struct phy *phy_u3;
>>> +
>>> +	/* USB 3 controller device */
>>> +	struct device *ctrl_u3;
>>> +	atomic_t ctrl_resetting;
>>>    };
>>>    
>>>    static const struct rk_udphy_dp_tx_drv_ctrl rk3588_dp_tx_drv_ctrl_rbr_hbr[4][4] = {
>>> @@ -1255,6 +1259,9 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
>>>    	struct rk_udphy *udphy = phy_get_drvdata(phy);
>>>    	int ret = 0;
>>>    
>>> +	if (atomic_read(&udphy->ctrl_resetting))
>>> +		return 0;
>>> +
>>>    	mutex_lock(&udphy->mutex);
>>>    	/* DP only or high-speed, disable U3 port */
>>>    	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
>>> @@ -1273,6 +1280,9 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>>>    {
>>>    	struct rk_udphy *udphy = phy_get_drvdata(phy);
>>>    
>>> +	if (atomic_read(&udphy->ctrl_resetting))
>>> +		return 0;
>>> +
>>>    	mutex_lock(&udphy->mutex);
>>>    	/* DP only or high-speed */
>>>    	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
>>> @@ -1401,10 +1411,31 @@ static struct phy *rk_udphy_phy_xlate(struct device *dev, const struct of_phandl
>>>    	return ERR_PTR(-EINVAL);
>>>    }
>>>    
>>> +static struct device_node *rk_udphy_to_controller(struct rk_udphy *udphy)
>>> +{
>>> +	struct device_node *np;
>>> +
>>> +	for_each_node_with_property(np, "phys") {
>>> +		struct of_phandle_iterator it;
>>> +		int ret;
>>> +
>>> +		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
>>> +			if (it.node != udphy->dev->of_node)
>>> +				continue;
>>> +
>>> +			of_node_put(it.node);
>>> +			return np;
>>> +		}
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>>    static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>>>    				 enum typec_orientation orien)
>>>    {
>>>    	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
>>> +	int ret;
>>>    
>>>    	mutex_lock(&udphy->mutex);
>>>    
>>> @@ -1420,6 +1451,18 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>>>    	rk_udphy_set_typec_default_mapping(udphy);
>>>    	rk_udphy_usb_bvalid_enable(udphy, true);
>>>    
>>> +	if (udphy->status != UDPHY_MODE_NONE && udphy->ctrl_u3) {
>>> +		atomic_set(&udphy->ctrl_resetting, 1);
>>> +		pm_runtime_force_suspend(udphy->ctrl_u3);
>>> +
>>> +		ret = rk_udphy_setup(udphy);
>>> +		if (!ret)
>>> +			clk_bulk_disable_unprepare(udphy->num_clks, udphy->clks);
>>> +
>>> +		pm_runtime_force_resume(udphy->ctrl_u3);
>>> +		atomic_set(&udphy->ctrl_resetting, 0);
>>> +	}
>>> +
>>>    unlock_ret:
>>>    	mutex_unlock(&udphy->mutex);
>>>    	return 0;
>>> @@ -1430,12 +1473,22 @@ static void rk_udphy_orien_switch_unregister(void *data)
>>>    	struct rk_udphy *udphy = data;
>>>    
>>>    	typec_switch_unregister(udphy->sw);
>>> +	put_device(udphy->ctrl_u3);
>>>    }
>>>    
>>>    static int rk_udphy_setup_orien_switch(struct rk_udphy *udphy)
>>>    {
>>> +	struct device_node *ctrl = rk_udphy_to_controller(udphy);
>>>    	struct typec_switch_desc sw_desc = { };
>>>    
>>> +	if (ctrl) {
>>> +		udphy->ctrl_u3 = bus_find_device_by_of_node(udphy->dev->bus, ctrl);
>>> +		of_node_put(ctrl);
>>> +	}
>>> +
>>> +	if (!udphy->ctrl_u3)
>>> +		dev_info(udphy->dev, "couldn't find this PHY's USB3 controller\n");
>>> +
>>>    	sw_desc.drvdata = udphy;
>>>    	sw_desc.fwnode = dev_fwnode(udphy->dev);
>>>    	sw_desc.set = rk_udphy_orien_sw_set;
>>> @@ -1499,6 +1552,7 @@ static int rk_udphy_probe(struct platform_device *pdev)
>>>    		return ret;
>>>    
>>>    	mutex_init(&udphy->mutex);
>>> +	atomic_set(&udphy->ctrl_resetting, 0);
>>>    	platform_set_drvdata(pdev, udphy);
>>>    
>>>    	if (device_property_present(dev, "orientation-switch")) {
>>>
>>
>>
> 
> 
> 
> 
> 

[PATCH v4 4/4] arm64: dts: rockchip: enable USB on Sige5

[Nicolas Frattaroli]

The ArmSoM Sige5 has several USB ports: a Type-A USB 3 port (USB2 lines
going through a hub), a Type-A USB 2.0 port (also going through a hub),
a Type-C DC input port that has absolutely no USB data connection and a
Type-C port with USB3.2 Gen1x1 that's also the maskrom programming port.

Enable these ports, and set the device role to be host for the host
ports.

The data capable Type-C USB port uses a fusb302 for data role switching.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../boot/dts/rockchip/rk3576-armsom-sige5.dts      | 160 +++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts b/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts
index b09e789c75c47fec7cf7e9810ab0dcca32d9404a..a2b6cdf0e0b949a9df946606efedfbc747f99975 100644
--- a/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts
@@ -205,6 +205,33 @@ vcc_3v3_ufs_s0: regulator-vcc-ufs-s0 {
 		regulator-max-microvolt = <3300000>;
 		vin-supply = <&vcc_5v0_sys>;
 	};
+
+	vcc_5v0_typec0: regulator-vcc-5v0-typec0 {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpios = <&gpio4 RK_PA6 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usb_otg0_pwren>;
+		regulator-name = "vcc_5v0_typec0";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&vcc_5v0_device>;
+	};
+	vcc_5v0_usbhost: regulator-vcc-5v0-usbhost {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpios = <&gpio4 RK_PA4 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usb_host_pwren>;
+		regulator-name = "vcc_5v0_usbhost";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&vcc_5v0_device>;
+	};
+};
+
+&combphy1_psu {
+	status = "okay";
 };
 
 &combphy0_ps {
@@ -643,6 +670,58 @@ regulator-state-mem {
 &i2c2 {
 	status = "okay";
 
+	usbc0: typec-portc@22 {
+		compatible = "fcs,fusb302";
+		reg = <0x22>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <RK_PA5 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usbc0_interrupt>;
+		vbus-supply = <&vcc_5v0_typec0>;
+
+		connector {
+			compatible = "usb-c-connector";
+			label = "USB-C";
+			data-role = "dual";
+			/* fusb302 supports PD Rev 2.0 Ver 1.2 */
+			pd-revision = /bits/ 8 <0x2 0x0 0x1 0x2>;
+			power-role = "source";
+			source-pdos = <PDO_FIXED(5000, 2000,
+						 PDO_FIXED_USB_COMM | PDO_FIXED_DATA_SWAP)>;
+
+			altmodes {
+				displayport {
+					svid = /bits/ 16 <0xff01>;
+					vdo = <0xffffffff>;
+				};
+			};
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					usbc0_hs_ep: endpoint {
+						remote-endpoint = <&usb_drd0_hs_ep>;
+					};
+				};
+				port@1 {
+					reg = <1>;
+					usbc0_ss_ep: endpoint {
+						remote-endpoint = <&usb_drd0_ss_ep>;
+					};
+				};
+				port@2 {
+					reg = <2>;
+					usbc0_dp_ep: endpoint {
+						remote-endpoint = <&usbdp_phy_ep>;
+					};
+				};
+			};
+		};
+	};
+
 	hym8563: rtc@51 {
 		compatible = "haoyu,hym8563";
 		reg = <0x51>;
@@ -729,6 +808,24 @@ pcie_reset: pcie-reset {
 			rockchip,pins = <2 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
+
+	usb {
+		usb_host_pwren: usb-host-pwren {
+			rockchip,pins = <4 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+		usb_otg0_pwren: usb-otg0-pwren {
+			rockchip,pins = <4 RK_PA6 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+		usbc0_interrupt: usbc0-interrupt {
+			rockchip,pins = <0 RK_PA5 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+		usbc0_sbu1: usbc0-sbu1 {
+			rockchip,pins = <2 RK_PA6 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+		usbc0_sbu2: usbc0-sbu2 {
+			rockchip,pins = <2 RK_PA7 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
 };
 
 &sai1 {
@@ -770,11 +867,74 @@ &sdmmc {
 	status = "okay";
 };
 
+&u2phy0 {
+	status = "okay";
+};
+
+&u2phy0_otg {
+	status = "okay";
+};
+
+&u2phy1 {
+	status = "okay";
+};
+
+&u2phy1_otg {
+	phy-supply = <&vcc_5v0_usbhost>;
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-0 = <&uart0m0_xfer>;
 	status = "okay";
 };
 
+&usb_drd0_dwc3 {
+	usb-role-switch;
+	dr_mode = "otg";
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			usb_drd0_hs_ep: endpoint {
+				remote-endpoint = <&usbc0_hs_ep>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			usb_drd0_ss_ep: endpoint {
+				remote-endpoint = <&usbc0_ss_ep>;
+			};
+		};
+	};
+};
+
+&usb_drd1_dwc3 {
+	dr_mode = "host";
+	status = "okay";
+};
+
+&usbdp_phy {
+	mode-switch;
+	orientation-switch;
+	pinctrl-names = "default";
+	pinctrl-0 = <&usbc0_sbu1 &usbc0_sbu2>;
+	sbu1-dc-gpios = <&gpio2 RK_PA6 GPIO_ACTIVE_HIGH>;
+	sbu2-dc-gpios = <&gpio2 RK_PA7 GPIO_ACTIVE_HIGH>;
+	status = "okay";
+
+	port {
+		usbdp_phy_ep: endpoint {
+			remote-endpoint = <&usbc0_dp_ep>;
+		};
+	};
+};
+
 &vop {
 	status = "okay";
 };

-- 
2.49.0