[PATCH v2 0/3] usb: dwc2: Add USB DRD support on CV1800 Series SoC

[PATCH v2 0/3] usb: dwc2: Add USB DRD support on CV1800 Series SoC

[Inochi Amaoto]

The current DRD implementation of DRD will cause a interrupt flow on
CV1800 Series SoC. It has two reasons:
1. The DRD does not sync the phy mode when change the controller mode,
   it is a violation of the DRD as it does not rely on a OTG phy.
2. The CV1800 is entered with a Session End Detected interrupt, but the
   DRD code has no change on it, so the controller will enter the
   interrupt storm.

This patch add fixes for these two problem, and I have tested this on
a Huashan PI board (it has a CV1812H SoC).

Note: This patches set does not resolve all the problems, as
      usb_add_hcd() will override the role set by dwc2, but I am not
      sure whether it should change skip_phy_initialization of hcd.
      So I kept it as it is and users still needs to set the role
      in the userspace as "role-switch-default-mode" in devicetree
      takes no effect.

Changed from v1:
- https://lore.kernel.org/linux-usb/20261110070627.217946-1-inochiama@gmail.com
1. the DTS patch is dropped and will be sent separately.
patch 2:
1. fix wrong is_host check in dwc2_drd_init()
2. remove unnecessary role check before setting phy mode in
   dwc2_drd_role_sw_set()


Inochi Amaoto (3):
  usb: dwc2: gadget: honor pullup requests in core_connect
  usb: dwc2: drd: Sync the generic PHY mode with role requests
  usb: dwc2: Add USB DRD support on CV1800 series SoC

 drivers/usb/dwc2/core.h     |  5 ++-
 drivers/usb/dwc2/drd.c      | 75 ++++++++++++++++++++++++++++---------
 drivers/usb/dwc2/gadget.c   |  3 +-
 drivers/usb/dwc2/params.c   |  1 +
 drivers/usb/dwc2/platform.c |  4 +-
 5 files changed, 68 insertions(+), 20 deletions(-)

--
2.56.0

[PATCH v2 1/3] usb: dwc2: gadget: honor pullup requests in core_connect

[Inochi Amaoto]

The connector ID work initializes the peripheral controller and calls
dwc2_hsotg_core_connect() even when no gadget driver has requested a
connection. After a host-to-device role change, this clears SFTDISCON
and lets the host enumerate an unconfigured gadget.

Check enabled in the connection helper. So the device will keep
disconnect until the controller is ready

Fixes: 510ffaa48eac ("usb: dwc2: Initialize the USB core for peripheral mode")
Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/usb/dwc2/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6c8a9f05fff0..5c8e85493289 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3664,7 +3664,8 @@ void dwc2_hsotg_core_disconnect(struct dwc2_hsotg *hsotg)
 void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg)
 {
 	/* remove the soft-disconnect and let's go */
-	if (!hsotg->role_sw || (dwc2_readl(hsotg, GOTGCTL) & GOTGCTL_BSESVLD))
+	if (hsotg->enabled &&
+	    (!hsotg->role_sw || (dwc2_readl(hsotg, GOTGCTL) & GOTGCTL_BSESVLD)))
 		dwc2_clear_bit(hsotg, DCTL, DCTL_SFTDISCON);
 }
 
-- 
2.56.0

[PATCH v2 2/3] usb: dwc2: drd: Sync the generic PHY mode with role requests

[Inochi Amaoto]

The USB DRD does not reply on a OTG phy, which means the controller
should not reply on the switch behavior from PHY. Instead, it should
sync the phy state to avoid a brolen role.

Set the generic PHY mode after resolving the default role and before
updating the controller. And disable the controller clock if there
is a failure and it is disabled.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/usb/dwc2/core.h     |  2 +-
 drivers/usb/dwc2/drd.c      | 46 +++++++++++++++++++++++++++++--------
 drivers/usb/dwc2/platform.c |  4 +++-
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 015a5fb7842d..298b418e6859 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1400,7 +1400,7 @@ static inline int dwc2_is_device_mode(struct dwc2_hsotg *hsotg)
 
 int dwc2_drd_init(struct dwc2_hsotg *hsotg);
 void dwc2_drd_suspend(struct dwc2_hsotg *hsotg);
-void dwc2_drd_resume(struct dwc2_hsotg *hsotg);
+int dwc2_drd_resume(struct dwc2_hsotg *hsotg);
 void dwc2_drd_exit(struct dwc2_hsotg *hsotg);
 
 /*
diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
index 1ad8fa3f862a..3dc194657e82 100644
--- a/drivers/usb/dwc2/drd.c
+++ b/drivers/usb/dwc2/drd.c
@@ -9,6 +9,7 @@
 
 #include <linux/clk.h>
 #include <linux/iopoll.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/usb/role.h>
 #include "core.h"
@@ -88,6 +89,7 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
 	struct dwc2_hsotg *hsotg = usb_role_switch_get_drvdata(sw);
 	unsigned long flags;
 	int already = 0;
+	int ret = 0;
 
 	/* Skip session not in line with dr_mode */
 	if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
@@ -111,14 +113,11 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
 	 * mode and sessions. It is the case if cable is plugged at boot.
 	 */
 	if (!hsotg->ll_hw_enabled && hsotg->clk) {
-		int ret = clk_prepare_enable(hsotg->clk);
-
+		ret = clk_prepare_enable(hsotg->clk);
 		if (ret)
 			return ret;
 	}
 
-	spin_lock_irqsave(&hsotg->lock, flags);
-
 	if (role == USB_ROLE_NONE) {
 		/* default operation mode when usb role is USB_ROLE_NONE */
 		if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
@@ -127,6 +126,13 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
 			role = USB_ROLE_DEVICE;
 	}
 
+	ret = phy_set_mode(hsotg->phy, role == USB_ROLE_HOST ?
+				PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
+	if (ret)
+		goto out_clk;
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+
 	if ((IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) ||
 	     IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)) &&
 	     dwc2_is_device_mode(hsotg) &&
@@ -160,26 +166,40 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
 		/* This will raise a Connector ID Status Change Interrupt */
 		dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
 
-	if (!hsotg->ll_hw_enabled && hsotg->clk)
-		clk_disable_unprepare(hsotg->clk);
-
 	dev_dbg(hsotg->dev, "%s-session valid\n",
 		role == USB_ROLE_NONE ? "No" :
 		role == USB_ROLE_HOST ? "A" : "B");
 
-	return 0;
+out_clk:
+	if (!hsotg->ll_hw_enabled && hsotg->clk)
+		clk_disable_unprepare(hsotg->clk);
+
+	return ret;
 }
 
 int dwc2_drd_init(struct dwc2_hsotg *hsotg)
 {
 	struct usb_role_switch_desc role_sw_desc = {0};
 	struct usb_role_switch *role_sw;
+	bool is_host;
 	int ret;
 
 	if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
 		return 0;
 
 	hsotg->role_sw_default_mode = usb_get_role_switch_default_mode(hsotg->dev);
+
+	if (hsotg->role_sw_default_mode != USB_DR_MODE_UNKNOWN)
+		is_host = hsotg->role_sw_default_mode == USB_DR_MODE_HOST;
+	else
+		is_host = hsotg->dr_mode == USB_DR_MODE_HOST;
+
+	ret = phy_set_mode(hsotg->phy,
+			   is_host ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
+	if (ret)
+		return dev_err_probe(hsotg->dev, ret,
+				     "failed to set default PHY mode\n");
+
 	role_sw_desc.driver_data = hsotg;
 	role_sw_desc.fwnode = dev_fwnode(hsotg->dev);
 	role_sw_desc.set = dwc2_drd_role_sw_set;
@@ -214,10 +234,11 @@ void dwc2_drd_suspend(struct dwc2_hsotg *hsotg)
 	}
 }
 
-void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
+int dwc2_drd_resume(struct dwc2_hsotg *hsotg)
 {
 	u32 gintsts, gintmsk;
 	enum usb_role role;
+	int ret;
 
 	if (hsotg->role_sw) {
 		/* get last known role (as the get ops isn't implemented by this driver) */
@@ -230,6 +251,11 @@ void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
 				role = USB_ROLE_DEVICE;
 		}
 
+		ret = phy_set_mode(hsotg->phy, role == USB_ROLE_HOST ?
+				   PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
+		if (ret)
+			return ret;
+
 		/* restore last role that may have been lost */
 		if (role == USB_ROLE_HOST)
 			dwc2_ovr_avalid(hsotg, true);
@@ -250,6 +276,8 @@ void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
 		gintmsk |= GINTSTS_CONIDSTSCHNG;
 		dwc2_writel(hsotg, gintmsk, GINTMSK);
 	}
+
+	return 0;
 }
 
 void dwc2_drd_exit(struct dwc2_hsotg *hsotg)
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ef0d73077034..ab39d6b36b19 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -783,7 +783,9 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 		/* Need to restore FORCEDEVMODE/FORCEHOSTMODE */
 		dwc2_force_dr_mode(dwc2);
 	} else {
-		dwc2_drd_resume(dwc2);
+		ret = dwc2_drd_resume(dwc2);
+		if (ret)
+			return ret;
 	}
 
 	if (dwc2_is_device_mode(dwc2))
-- 
2.56.0

[PATCH v2 3/3] usb: dwc2: Add USB DRD support on CV1800 series SoC

[Inochi Amaoto]

On CV1800 series SoC, it has a problem that it enters the role switch
with a persistent Session End Detected interrupt. As a result, GOTGINT
bit 2 remains set after write-one-to-clear accesses and prevents a
normal boot.

Keeping the BVALID override asserted in host mode avoids the observed
interrupt storm while allowing host enumeration and device operation.
As the BVALOVAL field is device only in the datasheet, this change is
added as a quirk.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/usb/dwc2/core.h   |  3 +++
 drivers/usb/dwc2/drd.c    | 29 +++++++++++++++++++++--------
 drivers/usb/dwc2/params.c |  1 +
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 298b418e6859..52ac64f67f41 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -357,6 +357,8 @@ enum dwc2_ep0_state {
  *                      case.
  *                      0 - No (default)
  *                      1 - Yes
+ * @force_bvalid_in_host: Set the BVALID override value to 1 in host mode to
+ *                       avoid persistent Session End Detected interrupts.
  * @power_down:         Specifies whether the controller support power_down.
  *			If power_down is enabled, the controller will enter
  *			power_down in both peripheral and host mode when
@@ -460,6 +462,7 @@ struct dwc2_core_params {
 	bool reload_ctl;
 	bool uframe_sched;
 	bool external_id_pin_ctl;
+	bool force_bvalid_in_host;
 
 	int power_down;
 #define DWC2_POWER_DOWN_PARAM_NONE		0
diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
index 3dc194657e82..3df63c0b88d8 100644
--- a/drivers/usb/dwc2/drd.c
+++ b/drivers/usb/dwc2/drd.c
@@ -28,10 +28,13 @@ static void dwc2_ovr_init(struct dwc2_hsotg *hsotg)
 	gotgctl = dwc2_readl(hsotg, GOTGCTL);
 	dwc2_ovr_gotgctl(gotgctl);
 	gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
-	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
+	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST) {
 		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
-	else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL)
+		if (hsotg->params.force_bvalid_in_host)
+			gotgctl |= GOTGCTL_BVALOVAL;
+	} else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL) {
 		gotgctl |= GOTGCTL_BVALOVAL | GOTGCTL_VBVALOVAL;
+	}
 	dwc2_writel(hsotg, gotgctl, GOTGCTL);
 
 	spin_unlock_irqrestore(&hsotg->lock, flags);
@@ -45,18 +48,26 @@ static int dwc2_ovr_avalid(struct dwc2_hsotg *hsotg, bool valid)
 	u32 gotgctl = dwc2_readl(hsotg, GOTGCTL);
 
 	/* Check if A-Session is already in the right state */
-	if ((valid && (gotgctl & GOTGCTL_ASESVLD)) ||
+	if ((valid && (gotgctl & GOTGCTL_ASESVLD) &&
+	     (!hsotg->params.force_bvalid_in_host ||
+	      (dwc2_is_host_mode(hsotg) && (gotgctl & GOTGCTL_BVALOVAL) &&
+	       (gotgctl & GOTGCTL_AVALOVAL)))) ||
 	    (!valid && !(gotgctl & GOTGCTL_ASESVLD)))
 		return -EALREADY;
 
 	/* Always enable overrides to handle the resume case */
 	dwc2_ovr_gotgctl(gotgctl);
 
-	gotgctl &= ~GOTGCTL_BVALOVAL;
-	if (valid)
+	if (valid) {
 		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
-	else
-		gotgctl &= ~(GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
+		if (hsotg->params.force_bvalid_in_host)
+			gotgctl |= GOTGCTL_BVALOVAL;
+		else
+			gotgctl &= ~GOTGCTL_BVALOVAL;
+	} else {
+		gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL |
+			     GOTGCTL_VBVALOVAL);
+	}
 	dwc2_writel(hsotg, gotgctl, GOTGCTL);
 
 	return 0;
@@ -67,7 +78,9 @@ static int dwc2_ovr_bvalid(struct dwc2_hsotg *hsotg, bool valid)
 	u32 gotgctl = dwc2_readl(hsotg, GOTGCTL);
 
 	/* Check if B-Session is already in the right state */
-	if ((valid && (gotgctl & GOTGCTL_BSESVLD)) ||
+	if ((valid && (gotgctl & GOTGCTL_BSESVLD) &&
+	     (!hsotg->params.force_bvalid_in_host ||
+	      (dwc2_is_device_mode(hsotg) && !(gotgctl & GOTGCTL_AVALOVAL)))) ||
 	    (!valid && !(gotgctl & GOTGCTL_BSESVLD)))
 		return -EALREADY;
 
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 091bfcfef753..16db9c3c1991 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -222,6 +222,7 @@ static void dwc2_set_cv1800_params(struct dwc2_hsotg *hsotg)
 {
 	struct dwc2_core_params *p = &hsotg->params;
 
+	p->force_bvalid_in_host = true;
 	p->otg_caps.hnp_support = false;
 	p->otg_caps.srp_support = false;
 	p->host_dma = false;
-- 
2.56.0

Re: [PATCH v2 1/3] usb: dwc2: gadget: honor pullup requests in core_connect

[sashiko-bot]

> The connector ID work initializes the peripheral controller and calls
> dwc2_hsotg_core_connect() even when no gadget driver has requested a
> connection. After a host-to-device role change, this clears SFTDISCON
> and lets the host enumerate an unconfigured gadget.
> 
> Check enabled in the connection helper. So the device will keep
> disconnect until the controller is ready
> 
> Fixes: 510ffaa48eac ("usb: dwc2: Initialize the USB core for peripheral mode")
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v2 3/3] usb: dwc2: Add USB DRD support on CV1800 series SoC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] usb: dwc2: BVALOVAL cleared in host mode in
  dwc2_ovr_init()/dwc2_ovr_avalid()
--

commit 5b3a06c8f6671fc6eef64472919667bd9e08400a
Author: Inochi Amaoto <inochiama@gmail.com>

usb: dwc2: Add USB DRD support on CV1800 series SoC

This commit adds a force_bvalid_in_host quirk parameter and enables it on
CV1800 series SoCs so that GOTGCTL_BVALOVAL remains set in host mode to
avoid a persistent Session End Detected interrupt storm.

> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 3dc194657e821..3df63c0b88d80 100644
> --- a/drivers/usb/dwc2/drd.c
> +++ b/drivers/usb/dwc2/drd.c
> @@ -28,10 +28,13 @@ static void dwc2_ovr_init(struct dwc2_hsotg *hsotg)
>  	gotgctl = dwc2_readl(hsotg, GOTGCTL);
>  	dwc2_ovr_gotgctl(gotgctl);
>  	gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
> -	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
> +	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST) {
>  		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
> -	else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL)
> +		if (hsotg->params.force_bvalid_in_host)
> +			gotgctl |= GOTGCTL_BVALOVAL;
> +	} else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL) {
>  		gotgctl |= GOTGCTL_BVALOVAL | GOTGCTL_VBVALOVAL;
> +	}
>  	dwc2_writel(hsotg, gotgctl, GOTGCTL);
>  
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> @@ -45,18 +48,26 @@ static int dwc2_ovr_avalid(struct dwc2_hsotg *hsotg, bool valid)
>  	u32 gotgctl = dwc2_readl(hsotg, GOTGCTL);
>  
>  	/* Check if A-Session is already in the right state */
> -	if ((valid && (gotgctl & GOTGCTL_ASESVLD)) ||
> +	if ((valid && (gotgctl & GOTGCTL_ASESVLD) &&
> +	     (!hsotg->params.force_bvalid_in_host ||
> +	      (dwc2_is_host_mode(hsotg) && (gotgctl & GOTGCTL_BVALOVAL) &&
> +	       (gotgctl & GOTGCTL_AVALOVAL)))) ||
>  	    (!valid && !(gotgctl & GOTGCTL_ASESVLD)))
>  		return -EALREADY;
>  
>  	/* Always enable overrides to handle the resume case */
>  	dwc2_ovr_gotgctl(gotgctl);
>  
> -	gotgctl &= ~GOTGCTL_BVALOVAL;
> -	if (valid)
> +	if (valid) {
>  		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
> -	else
> -		gotgctl &= ~(GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
> +		if (hsotg->params.force_bvalid_in_host)
> +			gotgctl |= GOTGCTL_BVALOVAL;
> +		else
> +			gotgctl &= ~GOTGCTL_BVALOVAL;
> +	} else {
> +		gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL |
> +			     GOTGCTL_VBVALOVAL);
> +	}
>  	dwc2_writel(hsotg, gotgctl, GOTGCTL);
>  
>  	return 0;

[Severity: High]
Can dwc2_ovr_avalid(hsotg, false) and dwc2_ovr_init() still clear
GOTGCTL_BVALOVAL while the controller is in host mode, triggering the
GOTGINT_SES_END_DET interrupt storm this patch aims to prevent?

When switching from USB_ROLE_HOST to USB_ROLE_NONE, dwc2_drd_role_sw_set()
calls dwc2_ovr_avalid(hsotg, false) and drops hsotg->lock before
dwc2_force_mode() switches the controller out of host mode:

drivers/usb/dwc2/drd.c:dwc2_drd_role_sw_set() {
	...
		} else {
			dwc2_ovr_avalid(hsotg, false);
		}
	}

	spin_unlock_irqrestore(&hsotg->lock, flags);

	if (!already && hsotg->dr_mode == USB_DR_MODE_OTG)
		/* This will raise a Connector ID Status Change Interrupt */
		dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
}

Because dwc2_ovr_avalid(hsotg, false) clears GOTGCTL_BVALOVAL while the
controller is still in host mode, interrupts are re-enabled at
spin_unlock_irqrestore() before dwc2_force_mode() switches to device mode
(and if dr_mode is USB_DR_MODE_HOST, dwc2_force_mode() is not called at
all).

Similarly, dwc2_ovr_init() forces host mode at the end of the function
whenever hsotg->dr_mode == USB_DR_MODE_HOST or hsotg->role_sw_default_mode
== USB_DR_MODE_HOST, but only sets GOTGCTL_BVALOVAL when
role_sw_default_mode == USB_DR_MODE_HOST. If dr_mode is USB_DR_MODE_HOST
and role_sw_default_mode is not, dwc2_ovr_init() enters host mode with
GOTGCTL_BVALOVAL cleared.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111055628.183891-1-inochiama@gmail.com?part=3

Re: [PATCH v2 2/3] usb: dwc2: drd: Sync the generic PHY mode with role requests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] usb: dwc2: drd: wrong PHY mode for USB_DR_MODE_HOST on
  USB_ROLE_NONE
- [Medium] usb: dwc2: drd: phy_set_mode() called while PHY is exited in
  dwc2_drd_role_sw_set()
--

commit 2436b9203ac01c46908592e593c8d76d66df4918
Author: Inochi Amaoto <inochiama@gmail.com>

usb: dwc2: drd: Sync the generic PHY mode with role requests

This commit updates dwc2_drd_init(), dwc2_drd_role_sw_set(), and
dwc2_drd_resume() to synchronize the generic PHY mode with the requested
or default USB role.

> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 1ad8fa3f862a1..3dc194657e821 100644
> --- a/drivers/usb/dwc2/drd.c
> +++ b/drivers/usb/dwc2/drd.c
[ ... ]
> @@ -111,14 +113,11 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>  	 * mode and sessions. It is the case if cable is plugged at boot.
>  	 */
>  	if (!hsotg->ll_hw_enabled && hsotg->clk) {
> -		int ret = clk_prepare_enable(hsotg->clk);
> -
> +		ret = clk_prepare_enable(hsotg->clk);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	spin_lock_irqsave(&hsotg->lock, flags);
> -
>  	if (role == USB_ROLE_NONE) {
>  		/* default operation mode when usb role is USB_ROLE_NONE */
>  		if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
> @@ -127,6 +126,13 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>  			role = USB_ROLE_DEVICE;
>  	}
>  
> +	ret = phy_set_mode(hsotg->phy, role == USB_ROLE_HOST ?
> +				PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
> +	if (ret)
> +		goto out_clk;

[Severity: Medium]
Can this call phy_set_mode() while hsotg->phy is powered off and exited?

In USB_DR_MODE_PERIPHERAL, dwc2_driver_probe() and dwc2_hsotg_udc_stop()
call dwc2_lowlevel_hw_disable(), which runs phy_power_off() and phy_exit()
and clears hsotg->ll_hw_enabled.  When dwc2_drd_role_sw_set() runs before
udc_start (for example when a cable is plugged in at boot), only
hsotg->clk is enabled while hsotg->phy remains uninitialized.

phy_set_mode_ext() invokes phy->ops->set_mode() without checking
phy->init_count, so PHY drivers that gate clocks or assert resets in
phy_exit() (such as phy-meson-gxl-usb2 or phy-ingenic-usb) will perform
unclocked register accesses.  When dwc2_hsotg_udc_start() later calls
dwc2_lowlevel_hw_enable(), phy_init() also resets the PHY without
re-applying the mode.

[ ... ]

> @@ -160,26 +166,40 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
[ ... ]
>  int dwc2_drd_init(struct dwc2_hsotg *hsotg)
>  {
>  	struct usb_role_switch_desc role_sw_desc = {0};
>  	struct usb_role_switch *role_sw;
> +	bool is_host;
>  	int ret;
>  
>  	if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
>  		return 0;
>  
>  	hsotg->role_sw_default_mode = usb_get_role_switch_default_mode(hsotg->dev);
> +
> +	if (hsotg->role_sw_default_mode != USB_DR_MODE_UNKNOWN)
> +		is_host = hsotg->role_sw_default_mode == USB_DR_MODE_HOST;
> +	else
> +		is_host = hsotg->dr_mode == USB_DR_MODE_HOST;
> +
> +	ret = phy_set_mode(hsotg->phy,
> +			   is_host ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
> +	if (ret)
> +		return dev_err_probe(hsotg->dev, ret,
> +				     "failed to set default PHY mode\n");
[ ... ]
> @@ -230,6 +251,11 @@ void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
>  				role = USB_ROLE_DEVICE;
>  		}
>  
> +		ret = phy_set_mode(hsotg->phy, role == USB_ROLE_HOST ?
> +				   PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Does this switch hsotg->phy to PHY_MODE_USB_DEVICE on USB_ROLE_NONE when
hsotg->dr_mode is USB_DR_MODE_HOST and role_sw_default_mode is
USB_DR_MODE_UNKNOWN?

When role is USB_ROLE_NONE and role-switch-default-mode is not set, role
stays USB_ROLE_NONE so that A-session valid overrides are not asserted.
Both dwc2_drd_role_sw_set() and dwc2_drd_resume() then pass
PHY_MODE_USB_DEVICE to phy_set_mode() even when hsotg->dr_mode is
USB_DR_MODE_HOST, leaving the PHY in device mode while the controller stays
in host mode.

In addition, if hsotg->dr_mode is USB_DR_MODE_HOST while
role_sw_default_mode is USB_DR_MODE_PERIPHERAL, dwc2_drd_init() sets
is_host to false and configures PHY_MODE_USB_DEVICE, whereas
dwc2_ovr_init() forces the controller into host mode:

drivers/usb/dwc2/drd.c:dwc2_ovr_init() {
	...
	dwc2_force_mode(hsotg, (hsotg->dr_mode == USB_DR_MODE_HOST) ||
				(hsotg->role_sw_default_mode == USB_DR_MODE_HOST));
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111055628.183891-1-inochiama@gmail.com?part=2