[RFC PATCH v1 0/2] phy: rockchip: inno-usb2: fix USB gadget hot-plug on peripheral-only OTG ports

[RFC PATCH v1 0/2] phy: rockchip: inno-usb2: fix USB gadget hot-plug on peripheral-only OTG ports

[Andreas Zdziarstek]

Hi All,

I had an interesting time trying out the USB OTG micro-USB port on the
Odroid-M1S (Rockchip RK3566) on the mainline Kernel. My intention was to
set up a cdc_ncm+cdc_acm peripheral gadget as a hot-pluggable debug
port.

For the most part that went swimmingly. Just not quite with the
hot-plugging. The apparent first problem was that peripheral mode only
works if the host is connected at boot time of the RK3566 Kernel,
regardless if the dr_mode settings is "otg" or "peripheral". When no
connection is present at boot, gadget setup fails with a
"failed to enable ep0out" error and the port seems to end up in an
unrecoverable state afterwards.

I investigated a little as to why. With my very limited understanding
of the subsystems involved, I think I have identified some problems.
All seem largely related to each other.

* The M1S references its 5V OTG VBUS switch as phy-supply of the OTG
  port in its DT. The phy core unconditionally enables that on phy
  power_on. From then on it powers its own VBUSDET input, making further
  cable VBUS detection impossible. Also, apparently, after reading the
  schematic, that means even with a cable connected at boot (and
  peripheral mode therefore working), the board will try to back-power
  the host. Seems my host port is pretty robust, luckily.
* The inno-usb2 driver seems to generally lack DRP power supply VBUS
  switching support? At least PHY and VBUS supply are semantically
  treated the same.
* The inno-usb2 driver's state machine powers off the Phy in several
  places: on an initial no-VBUS sample, on DCP detection and on
  disconnect. That seems to happen unrelated to controller-initiated
  phy_power_on/off calls and looks like the main cause the gadget setup
  fails and then borks the controller state.

My proposed fixes are in two places:

* set the micro-USB M1S port to "peripheral" in DTS and remove the
  supply node. The board has two USB-A host ports, so device mode is the
  most likely usage scenario for the microUSB and real compliant DRP
  does not seem to be in the cards in the current state. Also, this
  fixes the potentially harmful back-powering issue. (2nd patch)
* keep the inno-usb2 phy driver from autonomously powering off when in
  peripheral mode. This makes hot-plugging and gadget-setup without a
  connected cable work. (1st patch)

This "works great for me". No issues whatsoever with the M1S in device
mode. I can plug/unplug/replug at will at any time, rock solid.

However, I am also doubting myself if I have completely misunderstood
the whole Dual-Role status quo and should have just done something
differently.

Also even if I am right about the problems, I would agree with anyone
saying that this isn't the "proper" fix for the whole situation. It
seems the rockchip vendor kernel is doing DRP related stuff differently,
e.g. an additional vbus-supply setting in DT with apparent support for
VBUS role-switching. Fully automatic OTG with gadget support *should*
be possible to do. Possibly a lot of work, though.

Still, I would say having a solid peripheral-only option including
hot-plugging is an improvement to the status-quo.

Host-only configs in other DTs should be unaffected as the inno-usb2
changes won't matter there. The autonomous power-off will still happen,
for better or worse.

Would love to get some insights on this from the experts.

Cheers,
Andy

Andreas Zdziarstek (2):
  phy: rockchip: inno-usb2: keep peripheral-only OTG port active
  arm64: dts: rockchip: fix gadget hot-plug on ODROID-M1S micro-USB

 .../boot/dts/rockchip/rk3566-odroid-m1s.dts   |  2 +-
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 27 +++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.53.0

[RFC PATCH v1 1/2] phy: rockchip: inno-usb2: keep peripheral-only OTG port active

[Andreas Zdziarstek]

The OTG state machine autonomously suspends the OTG port whenever it
decides no usable VBUS session is present. On the initial sample after
init, when a dedicated charger is detected, on peripheral disconnect
and on host-session end. It calls rockchip_usb2phy_power_off()
directly, underneath the USB controller.

For a dr_mode="peripheral" port this kills the USB gadget on any boot
without an attached cable. The state machine suspends the port shortly
after init, resulting in "failed to enable ep0out", ep0 start transfer
returns -EINVAL). The gadget remains unrecoverable in user space
afterwards.

Skip the state machine's autonomous power-offs when the port is
peripheral-only. VBUS detection, extcon signalling and charger
detection are unaffected, and controller-initiated power management
via phy_power_on()/phy_power_off() keeps working.

Signed-off-by: Andreas Zdziarstek <andreas.zdziarstek@gmail.com>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 27 +++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 7d8a533f24ae..9b138b7aaeb8 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -670,6 +670,23 @@ static const struct phy_ops rockchip_usb2phy_ops = {
 	.owner		= THIS_MODULE,
 };
 
+/*
+ * Autonomous power-off from the OTG state machine or charger detection.
+ *
+ * A peripheral-only port shouldn't be power-gated behind the controller's
+ * back. If the gadget (re)binds while the PHY is suspended, the
+ * controller's ep0 setup fails and the gadget stays dead even when a VBUS
+ * session appears later. Keep the phy active on peripheral ports for
+ * correct connection detection.
+ */
+static void rockchip_usb2phy_sm_power_off(struct rockchip_usb2phy_port *rport)
+{
+	if (rport->mode == USB_DR_MODE_PERIPHERAL)
+		return;
+
+	rockchip_usb2phy_power_off(rport->phy);
+}
+
 static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
 {
 	struct rockchip_usb2phy_port *rport =
@@ -693,7 +710,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
 	case OTG_STATE_UNDEFINED:
 		rport->state = OTG_STATE_B_IDLE;
 		if (!vbus_attach)
-			rockchip_usb2phy_power_off(rport->phy);
+			rockchip_usb2phy_sm_power_off(rport);
 		fallthrough;
 	case OTG_STATE_B_IDLE:
 		if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) > 0) {
@@ -719,7 +736,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
 					break;
 				case POWER_SUPPLY_TYPE_USB_DCP:
 					dev_dbg(&rport->phy->dev, "dcp cable is connected\n");
-					rockchip_usb2phy_power_off(rport->phy);
+					rockchip_usb2phy_sm_power_off(rport);
 					notify_charger = true;
 					sch_work = true;
 					cable = EXTCON_CHG_USB_DCP;
@@ -765,7 +782,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
 			rphy->chg_type = POWER_SUPPLY_TYPE_UNKNOWN;
 			rport->state = OTG_STATE_B_IDLE;
 			delay = 0;
-			rockchip_usb2phy_power_off(rport->phy);
+			rockchip_usb2phy_sm_power_off(rport);
 		}
 		sch_work = true;
 		break;
@@ -773,7 +790,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
 		if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) == 0) {
 			dev_dbg(&rport->phy->dev, "usb otg host disconnect\n");
 			rport->state = OTG_STATE_B_IDLE;
-			rockchip_usb2phy_power_off(rport->phy);
+			rockchip_usb2phy_sm_power_off(rport);
 		}
 		break;
 	default:
@@ -838,7 +855,7 @@ static void rockchip_chg_detect_work(struct work_struct *work)
 	switch (rphy->chg_state) {
 	case USB_CHG_STATE_UNDEFINED:
 		if (!rport->suspended && !vbus_attach)
-			rockchip_usb2phy_power_off(rport->phy);
+			rockchip_usb2phy_sm_power_off(rport);
 		/* put the controller in non-driving mode */
 		if (!vbus_attach)
 			property_enable(rphy->grf, &rphy->phy_cfg->chg_det.opmode, false);
-- 
2.53.0

[RFC PATCH v1 2/2] arm64: dts: rockchip: fix gadget hot-plug on ODROID-M1S micro-USB

[Andreas Zdziarstek]

Referencing the vcc5v0_usb2_otg switch as phy-supply of the OTG port
makes the phy core enable it as soon as the USB controller powers the
PHY, so the board permanently drives its own 5 V onto the micro-USB
connector's VBUS pin. That blinds the PHY's session detection: VBUSDET
(fed from connector VBUS through the on-board 10k/15k divider) is
always high, no bvalid edge can ever occur on plug or unplug, and the
OTG state machine never wakes from its initial state. A USB gadget
only enumerates if the cable is already attached at power-on. It also
back-drives 5 V against the host port.

The inno-usb2 driver has no role-managed VBUS supply support, and the
practical use of this port is as the board's USB device/download port.
Separate Type-A ports are available for host functions.

Drop the phy-supply so the port never sources VBUS and pin the
controller to dr_mode="peripheral" to match that.

Fixes: 10dc64fe0f98 ("arm64: dts: rockchip: Add Hardkernel ODROID-M1S")
Signed-off-by: Andreas Zdziarstek <andreas.zdziarstek@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3566-odroid-m1s.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-odroid-m1s.dts b/arch/arm64/boot/dts/rockchip/rk3566-odroid-m1s.dts
index 33bc5249d729..d9d4a31373a0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-odroid-m1s.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-odroid-m1s.dts
@@ -602,6 +602,7 @@ &usb_host0_ohci {
 };
 
 &usb_host0_xhci {
+	dr_mode = "peripheral";
 	status = "okay";
 };
 
@@ -627,7 +628,6 @@ &usb2phy0_host {
 };
 
 &usb2phy0_otg {
-	phy-supply = <&vcc5v0_usb2_otg>;
 	status = "okay";
 };
 
-- 
2.53.0

Re: [RFC PATCH v1 1/2] phy: rockchip: inno-usb2: keep peripheral-only OTG port active

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Medium] The patch introduces a permanent power state leak for peripheral-only ports by skipping autonomous PHY power-offs but failing to skip autonomous power-ons.

Pre-existing issues:
- [Critical] The charger detection logic accesses uninitialized `chg_det` registers on SoCs like RK3036, inadvertently corrupting GRF register offset 0x0.
- [High] Delayed works (`otg_sm_work`, `chg_work`, `sm_work`) are not properly canceled on driver unbind, and mutual re-arming prevents clean teardown, leading to use-after-free.
- [High] The `cable` variable in `rockchip_usb2phy_otg_sm_work` is declared as `static`, which causes state corruption if multiple OTG ports exist.
- [High] Unprotected concurrent modifications to OTG and charger state variables (`rport->state`, `rphy->chg_state`, `rphy->chg_type`).
- [Medium] Concurrent lockless access to `rport->suspended` and hardware clock/register state between workqueues and the PHY core leads to a data race and clock leaks.
--

--- Patch [1]: [RFC PATCH v1 1/2] phy: rockchip: inno-usb2: keep peripheral-only OTG port active ---
commit dc6ab852562662efe7db14ed699e4f8c82b9f053
Author: Andreas Zdziarstek <andreas.zdziarstek@gmail.com>

phy: rockchip: inno-usb2: keep peripheral-only OTG port active

This commit updates the OTG state machine to skip autonomous power-offs
when the port is peripheral-only. This prevents the state machine from
suspending the port and killing the USB gadget when booting without an
attached cable.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 7d8a533f24aea..9b138b7aaeb89 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -670,6 +670,23 @@ static const struct phy_ops rockchip_usb2phy_ops = {
[ ... ]
> +static void rockchip_usb2phy_sm_power_off(struct rockchip_usb2phy_port *rport)
> +{
> +	if (rport->mode == USB_DR_MODE_PERIPHERAL)
> +		return;
> +
> +	rockchip_usb2phy_power_off(rport->phy);
> +}

[Severity: Medium]
The patch updates this function to skip powering off the PHY when the mode is
USB_DR_MODE_PERIPHERAL during a cable detach. However, further down in
rockchip_usb2phy_otg_sm_work(), the state machine still autonomously powers
ON the PHY when a cable is attached.

Does this asymmetrical behavior leave the PHY powered on indefinitely after
the first cable insertion, preventing system suspend?

> +
>  static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
>  {
>  	struct rockchip_usb2phy_port *rport =
> @@ -693,7 +710,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
>  	case OTG_STATE_UNDEFINED:
>  		rport->state = OTG_STATE_B_IDLE;

[Severity: High]
This is a pre-existing issue, but is it safe to locklessly modify shared state
variables like rport->state here?

This state machine can run in the delayed workqueue, and is concurrently
invoked synchronously by the threaded IRQ handler
rockchip_usb2phy_bvalid_irq() as well as the rockchip_chg_detect_work()
workqueue.

Could this lack of mutual exclusion allow interleaved state transitions on
multiple CPUs and corrupt the state machine?

>  		if (!vbus_attach)
> -			rockchip_usb2phy_power_off(rport->phy);
> +			rockchip_usb2phy_sm_power_off(rport);
>  		fallthrough;
>  	case OTG_STATE_B_IDLE:
>  		if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) > 0) {
> @@ -719,7 +736,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
>  					break;
>  				case POWER_SUPPLY_TYPE_USB_DCP:
>  					dev_dbg(&rport->phy->dev, "dcp cable is connected\n");
> -					rockchip_usb2phy_power_off(rport->phy);
> +					rockchip_usb2phy_sm_power_off(rport);
>  					notify_charger = true;
>  					sch_work = true;
>  					cable = EXTCON_CHG_USB_DCP;

[Severity: High]
This is a pre-existing issue, but does the static declaration of this cable
variable cause state corruption if multiple OTG ports exist?

SoCs like RK3399 define multiple USB2 PHY instances with OTG ports. If VBUS
events overlap or alternate between ports, could the shared cable variable be
overwritten by the latest event, causing disconnected ports to report the
charger type of a different port to extcon?

> @@ -765,7 +782,7 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
[ ... ]
>  		if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) == 0) {
>  			dev_dbg(&rport->phy->dev, "usb otg host disconnect\n");
>  			rport->state = OTG_STATE_B_IDLE;
> -			rockchip_usb2phy_power_off(rport->phy);
> +			rockchip_usb2phy_sm_power_off(rport);
>  		}
>  		break;
>  	default:

[Severity: Medium]
This isn't a bug introduced by this patch, but does invoking the internal
power management functions directly from the workqueue bypass the generic PHY
framework mutex?

The generic PHY subsystem calls phy_power_on() (protected by phy->mutex),
which invokes rockchip_usb2phy_power_on(). Concurrently, the driver's
workqueues invoke rockchip_usb2phy_power_on() directly without acquiring
any locks.

Since rockchip_usb2phy_power_on() performs a check-then-act on
rport->suspended:

rockchip_usb2phy_power_on() {
    if (!rport->suspended)
        return 0;
    ret = clk_prepare_enable(rphy->clk480m);
    ...
}

Could both threads evaluate rport->suspended as true simultaneously, causing
double clock enables but only a single disable later?

[Severity: High]
This is a pre-existing issue, but how are these delayed works safely torn
down?

The works otg_sm_work and chg_work re-arm each other. In
rockchip_usb2phy_exit(), they are cancelled sequentially:

rockchip_usb2phy_exit() {
    cancel_delayed_work_sync(&rport->otg_sm_work);
    cancel_delayed_work_sync(&rport->chg_work);
    ...
}

If chg_work executes while otg_sm_work is being cancelled, can it re-arm
otg_sm_work after the cancellation finishes?

Additionally, since these works are initialized on devm-managed memory but
there is no driver .remove callback to cancel them, could pending works
execute on devm-freed memory upon driver unbind?

> @@ -838,7 +855,7 @@ static void rockchip_chg_detect_work(struct work_struct *work)
>  	switch (rphy->chg_state) {
>  	case USB_CHG_STATE_UNDEFINED:
>  		if (!rport->suspended && !vbus_attach)
> -			rockchip_usb2phy_power_off(rport->phy);
> +			rockchip_usb2phy_sm_power_off(rport);
>  		/* put the controller in non-driving mode */
>  		if (!vbus_attach)
>  			property_enable(rphy->grf, &rphy->phy_cfg->chg_det.opmode, false);

[Severity: Critical]
This is a pre-existing issue, but does this logic safely handle SoCs where
chg_det is not defined?

For SoCs like RK3036, the rk3036_phy_cfgs does not define chg_det. However,
rockchip_usb2phy_otg_port_init() unconditionally initializes charger detection.
When a cable is attached, rockchip_chg_detect_work() invokes
property_enable() on the zeroed chg_det fields:

property_enable() {
    ...
    return regmap_write(base, reg->offset, val);
}

Because it lacks validation, would this silently write to reg->offset 0x0
with a write mask, corrupting the General Register Files (GRF) at offset 0x0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706223627.113814-1-andreas.zdziarstek@gmail.com?part=1