public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH v5 0/2] RK3576 USB Enablement
@ 2025-06-19 18:36 Nicolas Frattaroli
  2025-06-19 18:36 ` [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control Nicolas Frattaroli
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-06-19 18:36 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Kever Yang,
	Frank Wang, Neil Armstrong
  Cc: Alexey Charkov, Sebastian Reichel, kernel, linux-phy, devicetree,
	linux-arm-kernel, linux-rockchip, linux-kernel,
	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 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 v5:
- drop orientation handling patches, as this needs a proper solution in
  dwc3
- remove redundant property_enabled checks on svbus flags in otg_sm_work
- Link to v4: https://lore.kernel.org/r/20250610-rk3576-sige5-usb-v4-0-7e7f779619c1@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 (2):
      phy: rockchip: inno-usb2: add soft vbusvalid control
      arm64: dts: rockchip: enable USB on Sige5

 .../boot/dts/rockchip/rk3576-armsom-sige5.dts      | 160 +++++++++++++++++++++
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c      | 108 +++++++++++++-
 2 files changed, 264 insertions(+), 4 deletions(-)
---
base-commit: 1d07605c859ee3f483f07acd461452d9e505c58c
change-id: 20250328-rk3576-sige5-usb-230102aeeaca

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control
  2025-06-19 18:36 [PATCH v5 0/2] RK3576 USB Enablement Nicolas Frattaroli
@ 2025-06-19 18:36 ` Nicolas Frattaroli
  2025-06-19 19:42   ` Heiko Stuebner
  2025-06-19 18:36 ` [PATCH v5 2/2] arm64: dts: rockchip: enable USB on Sige5 Nicolas Frattaroli
  2025-06-19 21:50 ` (subset) [PATCH v5 0/2] RK3576 USB Enablement Heiko Stuebner
  2 siblings, 1 reply; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-06-19 18:36 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Kever Yang,
	Frank Wang, Neil Armstrong
  Cc: Alexey Charkov, Sebastian Reichel, kernel, linux-phy, devicetree,
	linux-arm-kernel, linux-rockchip, linux-kernel,
	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 | 108 +++++++++++++++++++++++++-
 1 file changed, 104 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..71810c07e4150ea81f65a8a932541b144e95a137 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,12 @@ 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) {
+		vbus_attach = true;
+	} else {
+		vbus_attach = property_enabled(rphy->grf,
+					       &rport->port_cfg->utmi_bvalid);
+	}
 
 	sch_work = false;
 	notify_charger = false;
@@ -1276,6 +1293,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 +1391,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 +2146,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 +2185,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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/2] arm64: dts: rockchip: enable USB on Sige5
  2025-06-19 18:36 [PATCH v5 0/2] RK3576 USB Enablement Nicolas Frattaroli
  2025-06-19 18:36 ` [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control Nicolas Frattaroli
@ 2025-06-19 18:36 ` Nicolas Frattaroli
  2025-06-19 21:50 ` (subset) [PATCH v5 0/2] RK3576 USB Enablement Heiko Stuebner
  2 siblings, 0 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-06-19 18:36 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Kever Yang,
	Frank Wang, Neil Armstrong
  Cc: Alexey Charkov, Sebastian Reichel, kernel, linux-phy, devicetree,
	linux-arm-kernel, linux-rockchip, linux-kernel,
	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 34e51cd71eac0395c7f36c892fc0711f6c324aea..b52646d76454671d83e7d684f67c2186f8b8c3d7 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 {
@@ -631,6 +658,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>;
@@ -736,6 +815,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 {
@@ -777,11 +874,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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control
  2025-06-19 18:36 ` [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control Nicolas Frattaroli
@ 2025-06-19 19:42   ` Heiko Stuebner
  2025-06-19 20:23     ` Nicolas Frattaroli
  0 siblings, 1 reply; 7+ messages in thread
From: Heiko Stuebner @ 2025-06-19 19:42 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Kever Yang, Frank Wang,
	Neil Armstrong, Nicolas Frattaroli, Sebastian Reichel
  Cc: Alexey Charkov, kernel, linux-phy, devicetree, linux-arm-kernel,
	linux-rockchip, linux-kernel, Nicolas Frattaroli

Hi Nicolas,

Am Donnerstag, 19. Juni 2025, 20:36:36 Mitteleuropäische Sommerzeit schrieb 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).

I would expect a paragraph more about what those bits (and their
functionality) actually do here :-) 


> 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.


In the rk3588 TRM I find USB2PHY_GRF_CON4
bit3 - sft_vbus_sel (VBUS software control select)
bit2 - sft_vbus (When sft_vbus_sel is 1'b1, vbusvalid and bvalid is
                 controlled by sft_vbus)

Is that the same stuff as you're adding for rk3576 ?

My last dance with rk3588-type-c is already some months back, but I do
remember running into "some" issue - but don't remember which one ;-)


Heiko

> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 108 +++++++++++++++++++++++++-
>  1 file changed, 104 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..71810c07e4150ea81f65a8a932541b144e95a137 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,12 @@ 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) {
> +		vbus_attach = true;
> +	} else {
> +		vbus_attach = property_enabled(rphy->grf,
> +					       &rport->port_cfg->utmi_bvalid);
> +	}
>  
>  	sch_work = false;
>  	notify_charger = false;
> @@ -1276,6 +1293,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 +1391,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 +2146,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 +2185,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 },
> 
> 






^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control
  2025-06-19 19:42   ` Heiko Stuebner
@ 2025-06-19 20:23     ` Nicolas Frattaroli
  2025-06-20 14:35       ` Nicolas Frattaroli
  0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-06-19 20:23 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Kever Yang, Frank Wang,
	Neil Armstrong, Sebastian Reichel, Heiko Stuebner
  Cc: Alexey Charkov, kernel, linux-phy, devicetree, linux-arm-kernel,
	linux-rockchip, linux-kernel

Hi Heiko,

On Thursday, 19 June 2025 21:42:00 Central European Summer Time Heiko Stuebner wrote:
> Hi Nicolas,
> 
> Am Donnerstag, 19. Juni 2025, 20:36:36 Mitteleuropäische Sommerzeit schrieb 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).
> 
> I would expect a paragraph more about what those bits (and their
> functionality) actually do here :-) 

:( fiiine

Quick non-patch explainer though, in case it helps you spot a problem in
the code: looks like svbus_sel to 1 tells the SoC that the OTG
controller's vbus valid and bvalid signal is controlled by the svbus_en
GRF flag instead of whatever the controller does based on what it sees
on the voltage lines.

> 
> 
> > 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.
> 
> 
> In the rk3588 TRM I find USB2PHY_GRF_CON4
> bit3 - sft_vbus_sel (VBUS software control select)
> bit2 - sft_vbus (When sft_vbus_sel is 1'b1, vbusvalid and bvalid is
>                  controlled by sft_vbus)
> 
> Is that the same stuff as you're adding for rk3576 ?

Yes, these appear to be the same ones. I'd need to check whether Type-C
USB devices enumerate on RK3588 first to see if we have the same problem
there though (it would be weird if it weren't a problem there).

If you pick the DT patch, I can send a new version of the soft vbusvalid
control patch with RK3588 addressed as well, if I manage to confirm it's
the same thing there.

> 
> My last dance with rk3588-type-c is already some months back, but I do
> remember running into "some" issue - but don't remember which one ;-)
> 
> 
> Heiko
> 

Kind regards,
Nicolas Frattaroli

> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 108 +++++++++++++++++++++++++-
> >  1 file changed, 104 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..71810c07e4150ea81f65a8a932541b144e95a137 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,12 @@ 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) {
> > +		vbus_attach = true;
> > +	} else {
> > +		vbus_attach = property_enabled(rphy->grf,
> > +					       &rport->port_cfg->utmi_bvalid);
> > +	}
> >  
> >  	sch_work = false;
> >  	notify_charger = false;
> > @@ -1276,6 +1293,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 +1391,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 +2146,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 +2185,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 },
> > 
> > 
> 
> 
> 
> 
> 






^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: (subset) [PATCH v5 0/2] RK3576 USB Enablement
  2025-06-19 18:36 [PATCH v5 0/2] RK3576 USB Enablement Nicolas Frattaroli
  2025-06-19 18:36 ` [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control Nicolas Frattaroli
  2025-06-19 18:36 ` [PATCH v5 2/2] arm64: dts: rockchip: enable USB on Sige5 Nicolas Frattaroli
@ 2025-06-19 21:50 ` Heiko Stuebner
  2 siblings, 0 replies; 7+ messages in thread
From: Heiko Stuebner @ 2025-06-19 21:50 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Kever Yang, Frank Wang,
	Neil Armstrong, Nicolas Frattaroli
  Cc: Heiko Stuebner, Alexey Charkov, Sebastian Reichel, kernel,
	linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
	linux-kernel


On Thu, 19 Jun 2025 20:36:35 +0200, Nicolas Frattaroli wrote:
> 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.
> 
> [...]

Applied, thanks!

[2/2] arm64: dts: rockchip: enable USB on Sige5
      commit: 64df8e2e207a2152201ef3515baacd8816c13282

Adapted to the other Sige5 series I just applied.
Please double-check.


Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control
  2025-06-19 20:23     ` Nicolas Frattaroli
@ 2025-06-20 14:35       ` Nicolas Frattaroli
  0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-06-20 14:35 UTC (permalink / raw)
  To: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Kever Yang, Frank Wang,
	Neil Armstrong, Sebastian Reichel, Heiko Stuebner
  Cc: Alexey Charkov, kernel, linux-phy, devicetree, linux-arm-kernel,
	linux-rockchip, linux-kernel

Hi Heiko, a quick follow-up,

On Thursday, 19 June 2025 22:23:35 Central European Summer Time Nicolas Frattaroli wrote:
> Hi Heiko,
> 
> On Thursday, 19 June 2025 21:42:00 Central European Summer Time Heiko Stuebner wrote:
> > Hi Nicolas,
> > 
> > Am Donnerstag, 19. Juni 2025, 20:36:36 Mitteleuropäische Sommerzeit schrieb 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).
> > 
> > I would expect a paragraph more about what those bits (and their
> > functionality) actually do here :-) 
> 
> :( fiiine
> 
> Quick non-patch explainer though, in case it helps you spot a problem in
> the code: looks like svbus_sel to 1 tells the SoC that the OTG
> controller's vbus valid and bvalid signal is controlled by the svbus_en
> GRF flag instead of whatever the controller does based on what it sees
> on the voltage lines.
> 
> > 
> > 
> > > 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.
> > 
> > 
> > In the rk3588 TRM I find USB2PHY_GRF_CON4
> > bit3 - sft_vbus_sel (VBUS software control select)
> > bit2 - sft_vbus (When sft_vbus_sel is 1'b1, vbusvalid and bvalid is
> >                  controlled by sft_vbus)
> > 
> > Is that the same stuff as you're adding for rk3576 ?
> 
> Yes, these appear to be the same ones. I'd need to check whether Type-C
> USB devices enumerate on RK3588 first to see if we have the same problem
> there though (it would be weird if it weren't a problem there).
> 
> If you pick the DT patch, I can send a new version of the soft vbusvalid
> control patch with RK3588 addressed as well, if I manage to confirm it's
> the same thing there.

So I tested this on RK3588, and could not reproduce the issue. Then, out of
curiosity, I reverted the patch and tested on the Sige5 again. I could also
not reproduce the issue anymore (?!). Even with the udphy line commented out
that sets those GRF regs as well, I can't get it to have issues enumerating
things on Type-C anymore.

The downstream commit this was based on is here:

https://github.com/rockchip-linux/kernel/commit/7d2237b0adc2e0a0105d63b645528993b44c8c36

So for now, this patch can be considered "abandoned, maybe unnecessary"
until the problem rears its head again for someone. I really don't get
why it works now :(

> 
> > 
> > My last dance with rk3588-type-c is already some months back, but I do
> > remember running into "some" issue - but don't remember which one ;-)
> > 
> > 
> > Heiko
> > 
> 
> Kind regards,
> Nicolas Frattaroli
> 
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 108 +++++++++++++++++++++++++-
> > >  1 file changed, 104 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..71810c07e4150ea81f65a8a932541b144e95a137 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,12 @@ 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) {
> > > +		vbus_attach = true;
> > > +	} else {
> > > +		vbus_attach = property_enabled(rphy->grf,
> > > +					       &rport->port_cfg->utmi_bvalid);
> > > +	}
> > >  
> > >  	sch_work = false;
> > >  	notify_charger = false;
> > > @@ -1276,6 +1293,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 +1391,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 +2146,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 +2185,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 },
> > > 
> > > 
> > 
> > 
> > 
> > 
> > 
> 
> 






^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-06-20 14:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-19 18:36 [PATCH v5 0/2] RK3576 USB Enablement Nicolas Frattaroli
2025-06-19 18:36 ` [PATCH v5 1/2] phy: rockchip: inno-usb2: add soft vbusvalid control Nicolas Frattaroli
2025-06-19 19:42   ` Heiko Stuebner
2025-06-19 20:23     ` Nicolas Frattaroli
2025-06-20 14:35       ` Nicolas Frattaroli
2025-06-19 18:36 ` [PATCH v5 2/2] arm64: dts: rockchip: enable USB on Sige5 Nicolas Frattaroli
2025-06-19 21:50 ` (subset) [PATCH v5 0/2] RK3576 USB Enablement Heiko Stuebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox