[PATCH 0/3] Add USB role switch support to DWC2

[PATCH 0/3] Add USB role switch support to DWC2

[Amelie Delaunay]

When using usb-c connector (but it can also be the case with a micro-b
connector), iddig, avalid, bvalid, vbusvalid input signals may not be
connected to the DWC2 OTG controller.
DWC2 OTG controller features an overriding control of the PHY voltage valid
and ID input signals.
So, missing signals can be forced using usb role from usb role switch and
this override feature.

This series adds support for usb role switch to dwc2, by using overriding
control of the PHY voltage valid and ID input signals.

It has been tested on stm32mp157c-dk2 [1], which has a Type-C connector
managed by a Type-C port controller, and connected to USB OTG controller.

[1] https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html

Amelie Delaunay (3):
  usb: dwc2: override PHY input signals with usb role switch support
  usb: dwc2: don't use ID/Vbus detection if usb-role-switch on STM32MP15
    SoCs
  ARM: dts: stm32: enable usb-role-switch on USB OTG on stm32mp15xx-dkx

 arch/arm/boot/dts/stm32mp15xx-dkx.dtsi |   2 +-
 drivers/usb/dwc2/Kconfig               |   1 +
 drivers/usb/dwc2/Makefile              |   2 +-
 drivers/usb/dwc2/core.h                |   8 ++
 drivers/usb/dwc2/drd.c                 | 190 +++++++++++++++++++++++++
 drivers/usb/dwc2/gadget.c              |   2 +-
 drivers/usb/dwc2/params.c              |   4 +-
 drivers/usb/dwc2/platform.c            |  13 ++
 8 files changed, 218 insertions(+), 4 deletions(-)
 create mode 100644 drivers/usb/dwc2/drd.c

-- 
2.17.1

[PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Amelie Delaunay]

This patch adds support for usb role switch to dwc2, by using overriding
control of the PHY voltage valid and ID input signals.

iddig signal (ID) can be overridden:
- when setting GUSBCFG_FORCEHOSTMODE, iddig input pin is overridden with 1;
- when setting GUSBCFG_FORCEDEVMODE, iddig input pin is overridden with 0.

avalid/bvalid/vbusvalid signals can be overridden respectively with:
- GOTGCTL_AVALOEN + GOTGCTL_AVALOVAL
- GOTGCTL_BVALOEN + GOTGCTL_BVALOVAL
- GOTGCTL_VBVALEN + GOTGCTL_VBVALOVAL

It is possible to determine valid sessions thanks to usb role switch:
- if USB_ROLE_NONE then !avalid && !bvalid && !vbusvalid
- if USB_ROLE_DEVICE then !avalid && bvalid && vbusvalid
- if USB_ROLE_HOST then avalid && !bvalid && vbusvalid

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 drivers/usb/dwc2/Kconfig    |   1 +
 drivers/usb/dwc2/Makefile   |   2 +-
 drivers/usb/dwc2/core.h     |   8 ++
 drivers/usb/dwc2/drd.c      | 190 ++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/gadget.c   |   2 +-
 drivers/usb/dwc2/platform.c |  13 +++
 6 files changed, 214 insertions(+), 2 deletions(-)
 create mode 100644 drivers/usb/dwc2/drd.c

diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
index 16e1aa304edc..dceb8f32414e 100644
--- a/drivers/usb/dwc2/Kconfig
+++ b/drivers/usb/dwc2/Kconfig
@@ -47,6 +47,7 @@ config USB_DWC2_PERIPHERAL
 config USB_DWC2_DUAL_ROLE
 	bool "Dual Role mode"
 	depends on (USB=y && USB_GADGET=y) || (USB_DWC2=m && USB && USB_GADGET)
+	select USB_ROLE_SWITCH
 	help
 	  Select this option if you want the driver to work in a dual-role
 	  mode. In this mode both host and gadget features are enabled, and
diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile
index 440320cc20a4..2bcd6945df46 100644
--- a/drivers/usb/dwc2/Makefile
+++ b/drivers/usb/dwc2/Makefile
@@ -3,7 +3,7 @@ ccflags-$(CONFIG_USB_DWC2_DEBUG)	+= -DDEBUG
 ccflags-$(CONFIG_USB_DWC2_VERBOSE)	+= -DVERBOSE_DEBUG
 
 obj-$(CONFIG_USB_DWC2)			+= dwc2.o
-dwc2-y					:= core.o core_intr.o platform.o
+dwc2-y					:= core.o core_intr.o platform.o drd.o
 dwc2-y					+= params.o
 
 ifneq ($(filter y,$(CONFIG_USB_DWC2_HOST) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 132d687f1590..9f529e27eaaa 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -860,6 +860,7 @@ struct dwc2_hregs_backup {
  *                      - USB_DR_MODE_PERIPHERAL
  *                      - USB_DR_MODE_HOST
  *                      - USB_DR_MODE_OTG
+ * @role_sw:		usb_role_switch handle
  * @hcd_enabled:	Host mode sub-driver initialization indicator.
  * @gadget_enabled:	Peripheral mode sub-driver initialization indicator.
  * @ll_hw_enabled:	Status of low-level hardware resources.
@@ -1054,6 +1055,7 @@ struct dwc2_hsotg {
 	struct dwc2_core_params params;
 	enum usb_otg_state op_state;
 	enum usb_dr_mode dr_mode;
+	struct usb_role_switch *role_sw;
 	unsigned int hcd_enabled:1;
 	unsigned int gadget_enabled:1;
 	unsigned int ll_hw_enabled:1;
@@ -1376,6 +1378,11 @@ static inline int dwc2_is_device_mode(struct dwc2_hsotg *hsotg)
 	return (dwc2_readl(hsotg, GINTSTS) & GINTSTS_CURMODE_HOST) == 0;
 }
 
+int dwc2_drd_init(struct dwc2_hsotg *hsotg);
+void dwc2_drd_suspend(struct dwc2_hsotg *hsotg);
+void dwc2_drd_resume(struct dwc2_hsotg *hsotg);
+void dwc2_drd_exit(struct dwc2_hsotg *hsotg);
+
 /*
  * Dump core registers and SPRAM
  */
@@ -1392,6 +1399,7 @@ int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2);
 int dwc2_gadget_init(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *dwc2,
 				       bool reset);
+void dwc2_hsotg_core_disconnect(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_disconnect(struct dwc2_hsotg *dwc2);
 int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, int testmode);
diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
new file mode 100644
index 000000000000..032efffa37ab
--- /dev/null
+++ b/drivers/usb/dwc2/drd.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * drd.c - DesignWare USB2 DRD Controller Dual-role support
+ *
+ * Copyright (C) 2020 STMicroelectronics
+ *
+ * Author(s): Amelie Delaunay <amelie.delaunay@st.com>
+ */
+
+#include <linux/iopoll.h>
+#include <linux/platform_device.h>
+#include <linux/usb/role.h>
+#include "core.h"
+
+static void dwc2_ovr_init(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 gotgctl;
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+
+	gotgctl = dwc2_readl(hsotg, GOTGCTL);
+	gotgctl |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN | GOTGCTL_VBVALOEN;
+	gotgctl |= GOTGCTL_DBNCE_FLTR_BYPASS;
+	gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+
+	dwc2_force_mode(hsotg, false);
+
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+}
+
+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)) ||
+	    (!valid && !(gotgctl & GOTGCTL_ASESVLD)))
+		return -EALREADY;
+
+	if (valid)
+		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
+	else
+		gotgctl &= ~(GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+
+	return 0;
+}
+
+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)) ||
+	    (!valid && !(gotgctl & GOTGCTL_BSESVLD)))
+		return -EALREADY;
+
+	if (valid)
+		gotgctl |= GOTGCTL_BVALOVAL | GOTGCTL_VBVALOVAL;
+	else
+		gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_VBVALOVAL);
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+
+	return 0;
+}
+
+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;
+
+	/* Skip session not in line with dr_mode */
+	if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
+	    (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
+		return -EINVAL;
+
+	/* Skip session if core is in test mode */
+	if (role == USB_ROLE_NONE && hsotg->test_mode) {
+		dev_dbg(hsotg->dev, "Core is in test mode\n");
+		return -EBUSY;
+	}
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+
+	if (role == USB_ROLE_HOST) {
+		if (dwc2_ovr_avalid(hsotg, true))
+			goto unlock;
+
+		if (hsotg->dr_mode == USB_DR_MODE_OTG)
+			/*
+			 * This will raise a Connector ID Status Change
+			 * Interrupt - connID A
+			 */
+			dwc2_force_mode(hsotg, true);
+	} else if (role == USB_ROLE_DEVICE) {
+		if (dwc2_ovr_bvalid(hsotg, true))
+			goto unlock;
+
+		if (hsotg->dr_mode == USB_DR_MODE_OTG)
+			/*
+			 * This will raise a Connector ID Status Change
+			 * Interrupt - connID B
+			 */
+			dwc2_force_mode(hsotg, false);
+
+		/* This clear DCTL.SFTDISCON bit */
+		dwc2_hsotg_core_connect(hsotg);
+	} else {
+		if (dwc2_is_device_mode(hsotg)) {
+			if (!dwc2_ovr_bvalid(hsotg, false))
+				/* This set DCTL.SFTDISCON bit */
+				dwc2_hsotg_core_disconnect(hsotg);
+		} else {
+			dwc2_ovr_avalid(hsotg, false);
+		}
+	}
+
+unlock:
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
+	dev_dbg(hsotg->dev, "%s-session valid\n",
+		role == USB_ROLE_NONE ? "No" :
+		role == USB_ROLE_HOST ? "A" : "B");
+
+	return 0;
+}
+
+int dwc2_drd_init(struct dwc2_hsotg *hsotg)
+{
+	struct usb_role_switch_desc role_sw_desc = {0};
+	struct usb_role_switch *role_sw;
+	int ret;
+
+	if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
+		return 0;
+
+	role_sw_desc.driver_data = hsotg;
+	role_sw_desc.fwnode = dev_fwnode(hsotg->dev);
+	role_sw_desc.set = dwc2_drd_role_sw_set;
+	role_sw_desc.allow_userspace_control = true;
+
+	role_sw = usb_role_switch_register(hsotg->dev, &role_sw_desc);
+	if (IS_ERR(role_sw)) {
+		ret = PTR_ERR(role_sw);
+		dev_err(hsotg->dev,
+			"failed to register role switch: %d\n", ret);
+		return ret;
+	}
+
+	hsotg->role_sw = role_sw;
+
+	/* Enable override and initialize values */
+	dwc2_ovr_init(hsotg);
+
+	return 0;
+}
+
+void dwc2_drd_suspend(struct dwc2_hsotg *hsotg)
+{
+	u32 gintsts, gintmsk;
+
+	if (hsotg->role_sw && !hsotg->params.external_id_pin_ctl) {
+		gintmsk = dwc2_readl(hsotg, GINTMSK);
+		gintmsk &= ~GINTSTS_CONIDSTSCHNG;
+		dwc2_writel(hsotg, gintmsk, GINTMSK);
+		gintsts = dwc2_readl(hsotg, GINTSTS);
+		dwc2_writel(hsotg, gintsts | GINTSTS_CONIDSTSCHNG, GINTSTS);
+	}
+}
+
+void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
+{
+	u32 gintsts, gintmsk;
+
+	if (hsotg->role_sw && !hsotg->params.external_id_pin_ctl) {
+		gintsts = dwc2_readl(hsotg, GINTSTS);
+		dwc2_writel(hsotg, gintsts | GINTSTS_CONIDSTSCHNG, GINTSTS);
+		gintmsk = dwc2_readl(hsotg, GINTMSK);
+		gintmsk |= GINTSTS_CONIDSTSCHNG;
+		dwc2_writel(hsotg, gintmsk, GINTMSK);
+	}
+}
+
+void dwc2_drd_exit(struct dwc2_hsotg *hsotg)
+{
+	if (hsotg->role_sw)
+		usb_role_switch_unregister(hsotg->role_sw);
+}
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 12b98b466287..e7702252d48c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3532,7 +3532,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
 		dwc2_readl(hsotg, DOEPCTL0));
 }
 
-static void dwc2_hsotg_core_disconnect(struct dwc2_hsotg *hsotg)
+void dwc2_hsotg_core_disconnect(struct dwc2_hsotg *hsotg)
 {
 	/* set the soft-disconnect bit */
 	dwc2_set_bit(hsotg, DCTL, DCTL_SFTDISCON);
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index e571c8ae65ec..629a119f04b7 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -317,6 +317,8 @@ static int dwc2_driver_remove(struct platform_device *dev)
 	if (hsotg->params.activate_stm_id_vb_detection)
 		regulator_disable(hsotg->usb33d);
 
+	dwc2_drd_exit(hsotg);
+
 	if (hsotg->ll_hw_enabled)
 		dwc2_lowlevel_hw_disable(hsotg);
 
@@ -532,6 +534,13 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		dwc2_writel(hsotg, ggpio, GGPIO);
 	}
 
+	retval = dwc2_drd_init(hsotg);
+	if (retval) {
+		if (retval != -EPROBE_DEFER)
+			dev_err(hsotg->dev, "failed to initialize dual-role\n");
+		goto error_init;
+	}
+
 	if (hsotg->dr_mode != USB_DR_MODE_HOST) {
 		retval = dwc2_gadget_init(hsotg);
 		if (retval)
@@ -594,6 +603,8 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 	if (is_device_mode)
 		dwc2_hsotg_suspend(dwc2);
 
+	dwc2_drd_suspend(dwc2);
+
 	if (dwc2->params.activate_stm_id_vb_detection) {
 		unsigned long flags;
 		u32 ggpio, gotgctl;
@@ -674,6 +685,8 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	/* Need to restore FORCEDEVMODE/FORCEHOSTMODE */
 	dwc2_force_dr_mode(dwc2);
 
+	dwc2_drd_resume(dwc2);
+
 	if (dwc2_is_device_mode(dwc2))
 		ret = dwc2_hsotg_resume(dwc2);
 
-- 
2.17.1

[PATCH 2/3] usb: dwc2: don't use ID/Vbus detection if usb-role-switch on STM32MP15 SoCs

[Amelie Delaunay]

If usb-role-switch is present in the device tree, it means that ID and Vbus
signals are not connected to the OTG controller but to an external
component (GPIOs, Type-C controller). In this configuration, usb role
switch is used to force valid sessions on STM32MP15 SoCs.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 drivers/usb/dwc2/params.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index ce736d67c7c3..d6690ee7af01 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -183,9 +183,11 @@ static void dwc2_set_stm32mp15_fsotg_params(struct dwc2_hsotg *hsotg)
 static void dwc2_set_stm32mp15_hsotg_params(struct dwc2_hsotg *hsotg)
 {
 	struct dwc2_core_params *p = &hsotg->params;
+	struct device_node *np = hsotg->dev->of_node;
 
 	p->otg_cap = DWC2_CAP_PARAM_NO_HNP_SRP_CAPABLE;
-	p->activate_stm_id_vb_detection = true;
+	p->activate_stm_id_vb_detection =
+		!of_property_read_bool(np, "usb-role-switch");
 	p->host_rx_fifo_size = 440;
 	p->host_nperio_tx_fifo_size = 256;
 	p->host_perio_tx_fifo_size = 256;
-- 
2.17.1

[PATCH 3/3] ARM: dts: stm32: enable usb-role-switch on USB OTG on stm32mp15xx-dkx

[Amelie Delaunay]

Now that USB OTG driver supports usb role switch by overriding PHY input
signals (A-Valid, B-Valid and Vbus-Valid), enable it on stm32mp15xx-dkx.
dr_mode needn't to be forced to Peripheral anymore, it is set to OTG in
SoC device tree.
USB role (USB_ROLE_NONE, USB_ROLE_DEVICE, USB_ROLE_HOST) will be provided
by STUSB1600 Type-C controller driver.

This patch depends on "Add STUSB160x Type-C port controller support"
series, which is under review.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 arch/arm/boot/dts/stm32mp15xx-dkx.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi
index d946e0a02f5c..ab31db801eec 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi
@@ -595,9 +595,9 @@
 };
 
 &usbotg_hs {
-	dr_mode = "peripheral";
 	phys = <&usbphyc_port1 0>;
 	phy-names = "usb2-phy";
+	usb-role-switch;
 	status = "okay";
 };
 
-- 
2.17.1

Re: [PATCH 0/3] Add USB role switch support to DWC2

[Minas Harutyunyan]

Hi,

On 6/16/2020 6:07 PM, Amelie Delaunay wrote:
> When using usb-c connector (but it can also be the case with a micro-b
> connector), iddig, avalid, bvalid, vbusvalid input signals may not be
> connected to the DWC2 OTG controller.
> DWC2 OTG controller features an overriding control of the PHY voltage valid
> and ID input signals.
> So, missing signals can be forced using usb role from usb role switch and
> this override feature.
> 
> This series adds support for usb role switch to dwc2, by using overriding
> control of the PHY voltage valid and ID input signals.
> 
> It has been tested on stm32mp157c-dk2 [1], which has a Type-C connector
> managed by a Type-C port controller, and connected to USB OTG controller.
> 
> [1] https://urldefense.com/v3/__https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html__;!!A4F2R9G_pg!KPU6pLcAMaVrFxzSp3peLkjZzWJzjsc__Kl878UYSLY2Cnv5NMmwACsY4kTCow0$
> 
> Amelie Delaunay (3):
>    usb: dwc2: override PHY input signals with usb role switch support
>    usb: dwc2: don't use ID/Vbus detection if usb-role-switch on STM32MP15
>      SoCs
>    ARM: dts: stm32: enable usb-role-switch on USB OTG on stm32mp15xx-dkx
> 
>   arch/arm/boot/dts/stm32mp15xx-dkx.dtsi |   2 +-
>   drivers/usb/dwc2/Kconfig               |   1 +
>   drivers/usb/dwc2/Makefile              |   2 +-
>   drivers/usb/dwc2/core.h                |   8 ++
>   drivers/usb/dwc2/drd.c                 | 190 +++++++++++++++++++++++++
>   drivers/usb/dwc2/gadget.c              |   2 +-
>   drivers/usb/dwc2/params.c              |   4 +-
>   drivers/usb/dwc2/platform.c            |  13 ++
>   8 files changed, 218 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/usb/dwc2/drd.c
> 
For dwc2:

Acked-by Minas Harutyunyan <hminas@synopsys.com>

Thanks,
Minas

RE: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Martin Blumenstingl]

Hello Amelie,

thank you for this patch - I am hoping that it will help us on Amlogic
Meson8, Meson8b, Meson8m2 and GXBB SoCs as well.
On these SoCs the ID detection is performed by the PHY IP and needs to
be polled.
I think usb_role_switch is the perfect framework for this on dwc2 side.
For the PHY driver I'm going to implement the cable state using the
extcon framework and then having a new usb-conn-extcon driver. This is
just to give you an overview why I'm interested in this.

[...]
> +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;
> +
> +	/* Skip session not in line with dr_mode */
> +	if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
> +	    (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
> +		return -EINVAL;
> +
> +	/* Skip session if core is in test mode */
> +	if (role == USB_ROLE_NONE && hsotg->test_mode) {
> +		dev_dbg(hsotg->dev, "Core is in test mode\n");
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
due to this spin_lock_irqsave() ...

> +	if (role == USB_ROLE_HOST) {
> +		if (dwc2_ovr_avalid(hsotg, true))
> +			goto unlock;
> +
> +		if (hsotg->dr_mode == USB_DR_MODE_OTG)
> +			/*
> +			 * This will raise a Connector ID Status Change
> +			 * Interrupt - connID A
> +			 */
> +			dwc2_force_mode(hsotg, true);
... we cannot sleep in here. the call flow is:
dwc2_drd_role_sw_set
  spin_lock_irqsave
  dwc2_force_mode
    dwc2_wait_for_mode
      usleep_range

> +	} else if (role == USB_ROLE_DEVICE) {
> +		if (dwc2_ovr_bvalid(hsotg, true))
> +			goto unlock;
> +
> +		if (hsotg->dr_mode == USB_DR_MODE_OTG)
> +			/*
> +			 * This will raise a Connector ID Status Change
> +			 * Interrupt - connID B
> +			 */
> +			dwc2_force_mode(hsotg, false);
(same sleeping issue here)

[...]
+int dwc2_drd_init(struct dwc2_hsotg *hsotg)
+{
+	struct usb_role_switch_desc role_sw_desc = {0};
+	struct usb_role_switch *role_sw;
+	int ret;
+
+	if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
+		return 0;
should we also return early here if dr_mode != "otg"?

[...]
@@ -532,6 +534,13 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		dwc2_writel(hsotg, ggpio, GGPIO);
 	}
 
+	retval = dwc2_drd_init(hsotg);
+	if (retval) {
+		if (retval != -EPROBE_DEFER)
+			dev_err(hsotg->dev, "failed to initialize dual-role\n");
+		goto error_init;
+	}
+
 	if (hsotg->dr_mode != USB_DR_MODE_HOST) {
 		retval = dwc2_gadget_init(hsotg);
 		if (retval)
I think dwc2_driver_probe() needs a new label (for example named
error_drd) which then calls dwc2_drd_exit. See [0] which I have
submitted as a patch for Linux 5.8, so it's not in usb-next yet.

Also in general I think you need to submit a dt-bindings patch that
documents the usb-role-switch property. Personally I would use
Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml as
reference for that.

Can you please keep me Cc'ed on a v2 because I'm not subscribed to the
linux-usb mailing list?
I am going to test this on Amlogic SoCs - once I made "everything else"
work I can give my Tested-by as well.


Thank you!
Martin


[0] https://patchwork.kernel.org/patch/11642957/

Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Amelie DELAUNAY]

Hi Martin,

On 7/4/20 7:42 PM, Martin Blumenstingl wrote:
> Hello Amelie,
> 
> thank you for this patch - I am hoping that it will help us on Amlogic
> Meson8, Meson8b, Meson8m2 and GXBB SoCs as well.
> On these SoCs the ID detection is performed by the PHY IP and needs to
> be polled.
> I think usb_role_switch is the perfect framework for this on dwc2 side.
> For the PHY driver I'm going to implement the cable state using the
> extcon framework and then having a new usb-conn-extcon driver. This is
> just to give you an overview why I'm interested in this.
> 

I'm wondering, why use extcon framework and not the usb role switch API 
? This patch on dwc2 is tested on STM32MP157C-DK2 board with STUSB160x 
Type-C controller driver recently pushed with usb role switch. You can 
have a look here https://lore.kernel.org/patchwork/patch/1256238/.

> [...]
>> +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;
>> +
>> +     /* Skip session not in line with dr_mode */
>> +     if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
>> +         (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
>> +             return -EINVAL;
>> +
>> +     /* Skip session if core is in test mode */
>> +     if (role == USB_ROLE_NONE && hsotg->test_mode) {
>> +             dev_dbg(hsotg->dev, "Core is in test mode\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     spin_lock_irqsave(&hsotg->lock, flags);
> due to this spin_lock_irqsave() ...
> 
>> +     if (role == USB_ROLE_HOST) {
>> +             if (dwc2_ovr_avalid(hsotg, true))
>> +                     goto unlock;
>> +
>> +             if (hsotg->dr_mode == USB_DR_MODE_OTG)
>> +                     /*
>> +                      * This will raise a Connector ID Status Change
>> +                      * Interrupt - connID A
>> +                      */
>> +                     dwc2_force_mode(hsotg, true);
> ... we cannot sleep in here. the call flow is:
> dwc2_drd_role_sw_set
>    spin_lock_irqsave
>    dwc2_force_mode
>      dwc2_wait_for_mode
>        usleep_range
> 

In fact, with the avalid or bvalid overriding + the debounce filter 
bypass, GINTSTS_CURMOD is already in the expected mode, so that we exit 
the loop directly, without running into usleep_range.

>> +     } else if (role == USB_ROLE_DEVICE) {
>> +             if (dwc2_ovr_bvalid(hsotg, true))
>> +                     goto unlock;
>> +
>> +             if (hsotg->dr_mode == USB_DR_MODE_OTG)
>> +                     /*
>> +                      * This will raise a Connector ID Status Change
>> +                      * Interrupt - connID B
>> +                      */
>> +                     dwc2_force_mode(hsotg, false);
> (same sleeping issue here)
> 
> [...]
> +int dwc2_drd_init(struct dwc2_hsotg *hsotg)
> +{
> +       struct usb_role_switch_desc role_sw_desc = {0};
> +       struct usb_role_switch *role_sw;
> +       int ret;
> +
> +       if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
> +               return 0;
> should we also return early here if dr_mode != "otg"?
> 

No, because when VBUS is not connected to the controller, you also need 
to get the usb_role_none from the usb-role-switch to catch the 
unattached state (also in Peripheral or Host only mode).

> [...]
> @@ -532,6 +534,13 @@ static int dwc2_driver_probe(struct platform_device 
> *dev)
>                   dwc2_writel(hsotg, ggpio, GGPIO);
>           }
> 
> +       retval = dwc2_drd_init(hsotg);
> +       if (retval) {
> +               if (retval != -EPROBE_DEFER)
> +                       dev_err(hsotg->dev, "failed to initialize 
> dual-role\n");
> +               goto error_init;
> +       }
> +
>           if (hsotg->dr_mode != USB_DR_MODE_HOST) {
>                   retval = dwc2_gadget_init(hsotg);
>                   if (retval)
> I think dwc2_driver_probe() needs a new label (for example named
> error_drd) which then calls dwc2_drd_exit. See [0] which I have
> submitted as a patch for Linux 5.8, so it's not in usb-next yet.
> 

I agree.

> Also in general I think you need to submit a dt-bindings patch that
> documents the usb-role-switch property. Personally I would use
> Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml as
> reference for that.
> 

Sure. Will be done in V2 with new label for drd_exit in probe failure 
path. I'll rebase my patch on yours to avoid conflicts.

> Can you please keep me Cc'ed on a v2 because I'm not subscribed to the
> linux-usb mailing list?

OK. Thanks for your review and future tests!
Regards,
Amelie

> I am going to test this on Amlogic SoCs - once I made "everything else"
> work I can give my Tested-by as well.
> 
> 
> Thank you!
> Martin
> 
> 
> [0] https://patchwork.kernel.org/patch/11642957/

Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Martin Blumenstingl]

Hi Amelie,

On Tue, Jul 7, 2020 at 6:13 PM Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>
> Hi Martin,
>
> On 7/4/20 7:42 PM, Martin Blumenstingl wrote:
> > Hello Amelie,
> >
> > thank you for this patch - I am hoping that it will help us on Amlogic
> > Meson8, Meson8b, Meson8m2 and GXBB SoCs as well.
> > On these SoCs the ID detection is performed by the PHY IP and needs to
> > be polled.
> > I think usb_role_switch is the perfect framework for this on dwc2 side.
> > For the PHY driver I'm going to implement the cable state using the
> > extcon framework and then having a new usb-conn-extcon driver. This is
> > just to give you an overview why I'm interested in this.
> >
>
> I'm wondering, why use extcon framework and not the usb role switch API
> ? This patch on dwc2 is tested on STM32MP157C-DK2 board with STUSB160x
> Type-C controller driver recently pushed with usb role switch. You can
> have a look here https://lore.kernel.org/patchwork/patch/1256238/.
one of the boards that I'm working on is for example the Odroid-C1. It
has a Micro-USB port and there's no Type-C controller present.

in the next few days I'll try to send my idea as RFC, but this is the
.dts I've come up with so far:
&usb0 {
    dr_mode = "otg";
    usb-role-switch;

    connector {
        compatible = "extcon-usb-b-connector", "usb-b-connector";
        type = "micro";
        extcon = <&usb0_phy>;
        vbus-supply = <&usb_vbus>;
    };
};

I did this for two reasons:
1. I think the PHY is not a connector and thus it's driver shouldn't
implement any connector specific logic (managing VBUS)
2. without the connector there would be a circular dependency: the USB
controller needs the PHY to initialize but the PHY would need the USB
controller so it can manage the role switch

(or in other words: the connector replaces the Type-C controller in this case)

> > [...]
> >> +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;
> >> +
> >> +     /* Skip session not in line with dr_mode */
> >> +     if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
> >> +         (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
> >> +             return -EINVAL;
> >> +
> >> +     /* Skip session if core is in test mode */
> >> +     if (role == USB_ROLE_NONE && hsotg->test_mode) {
> >> +             dev_dbg(hsotg->dev, "Core is in test mode\n");
> >> +             return -EBUSY;
> >> +     }
> >> +
> >> +     spin_lock_irqsave(&hsotg->lock, flags);
> > due to this spin_lock_irqsave() ...
> >
> >> +     if (role == USB_ROLE_HOST) {
> >> +             if (dwc2_ovr_avalid(hsotg, true))
> >> +                     goto unlock;
> >> +
> >> +             if (hsotg->dr_mode == USB_DR_MODE_OTG)
> >> +                     /*
> >> +                      * This will raise a Connector ID Status Change
> >> +                      * Interrupt - connID A
> >> +                      */
> >> +                     dwc2_force_mode(hsotg, true);
> > ... we cannot sleep in here. the call flow is:
> > dwc2_drd_role_sw_set
> >    spin_lock_irqsave
> >    dwc2_force_mode
> >      dwc2_wait_for_mode
> >        usleep_range
> >
>
> In fact, with the avalid or bvalid overriding + the debounce filter
> bypass, GINTSTS_CURMOD is already in the expected mode, so that we exit
> the loop directly, without running into usleep_range.
on my Amlogic SoC this is not the case:
The kernel complains because of that usleep_range from within the
spinlock context

Please let me know if/how I can help debug this.

[...]
> > +int dwc2_drd_init(struct dwc2_hsotg *hsotg)
> > +{
> > +       struct usb_role_switch_desc role_sw_desc = {0};
> > +       struct usb_role_switch *role_sw;
> > +       int ret;
> > +
> > +       if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
> > +               return 0;
> > should we also return early here if dr_mode != "otg"?
> >
>
> No, because when VBUS is not connected to the controller, you also need
> to get the usb_role_none from the usb-role-switch to catch the
> unattached state (also in Peripheral or Host only mode).
I see - thank you for the explanation!


Best regards,
Martin

Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Amelie DELAUNAY]

Hi Martin,

On 7/7/20 8:55 PM, Martin Blumenstingl wrote:
> Hi Amelie,
> 
> On Tue, Jul 7, 2020 at 6:13 PM Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>>
>> Hi Martin,
>>
>> On 7/4/20 7:42 PM, Martin Blumenstingl wrote:
>>> Hello Amelie,
>>>
>>> thank you for this patch - I am hoping that it will help us on Amlogic
>>> Meson8, Meson8b, Meson8m2 and GXBB SoCs as well.
>>> On these SoCs the ID detection is performed by the PHY IP and needs to
>>> be polled.
>>> I think usb_role_switch is the perfect framework for this on dwc2 side.
>>> For the PHY driver I'm going to implement the cable state using the
>>> extcon framework and then having a new usb-conn-extcon driver. This is
>>> just to give you an overview why I'm interested in this.
>>>
>>
>> I'm wondering, why use extcon framework and not the usb role switch API
>> ? This patch on dwc2 is tested on STM32MP157C-DK2 board with STUSB160x
>> Type-C controller driver recently pushed with usb role switch. You can
>> have a look here https://lore.kernel.org/patchwork/patch/1256238/.
> one of the boards that I'm working on is for example the Odroid-C1. It
> has a Micro-USB port and there's no Type-C controller present.
> 
> in the next few days I'll try to send my idea as RFC, but this is the
> .dts I've come up with so far:
> &usb0 {
>      dr_mode = "otg";
>      usb-role-switch;
> 
>      connector {
>          compatible = "extcon-usb-b-connector", "usb-b-connector";
>          type = "micro";
>          extcon = <&usb0_phy>;
>          vbus-supply = <&usb_vbus>;
>      };
> };
> 
> I did this for two reasons:
> 1. I think the PHY is not a connector and thus it's driver shouldn't
> implement any connector specific logic (managing VBUS)
> 2. without the connector there would be a circular dependency: the USB
> controller needs the PHY to initialize but the PHY would need the USB
> controller so it can manage the role switch
> 
> (or in other words: the connector replaces the Type-C controller in this case)
> 
>>> [...]
>>>> +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;
>>>> +
>>>> +     /* Skip session not in line with dr_mode */
>>>> +     if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
>>>> +         (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
>>>> +             return -EINVAL;
>>>> +
>>>> +     /* Skip session if core is in test mode */
>>>> +     if (role == USB_ROLE_NONE && hsotg->test_mode) {
>>>> +             dev_dbg(hsotg->dev, "Core is in test mode\n");
>>>> +             return -EBUSY;
>>>> +     }
>>>> +
>>>> +     spin_lock_irqsave(&hsotg->lock, flags);
>>> due to this spin_lock_irqsave() ...
>>>
>>>> +     if (role == USB_ROLE_HOST) {
>>>> +             if (dwc2_ovr_avalid(hsotg, true))
>>>> +                     goto unlock;
>>>> +
>>>> +             if (hsotg->dr_mode == USB_DR_MODE_OTG)
>>>> +                     /*
>>>> +                      * This will raise a Connector ID Status Change
>>>> +                      * Interrupt - connID A
>>>> +                      */
>>>> +                     dwc2_force_mode(hsotg, true);
>>> ... we cannot sleep in here. the call flow is:
>>> dwc2_drd_role_sw_set
>>>     spin_lock_irqsave
>>>     dwc2_force_mode
>>>       dwc2_wait_for_mode
>>>         usleep_range
>>>
>>
>> In fact, with the avalid or bvalid overriding + the debounce filter
>> bypass, GINTSTS_CURMOD is already in the expected mode, so that we exit
>> the loop directly, without running into usleep_range.
> on my Amlogic SoC this is not the case:
> The kernel complains because of that usleep_range from within the
> spinlock context
> 
> Please let me know if/how I can help debug this.
> 

Could you please test with:

static int dwc2_drd_role_sw_set(struct device *dev, enum usb_role role)
{
	struct dwc2_hsotg *hsotg = dev_get_drvdata(dev);
	unsigned long flags;
	int already = 0;

	/* Skip session not in line with dr_mode */
	if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
	    (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
		return -EINVAL;

	/* Skip session if core is in test mode */
	if (role == USB_ROLE_NONE && hsotg->test_mode) {
		dev_dbg(hsotg->dev, "Core is in test mode\n");
		return -EBUSY;
	}

	spin_lock_irqsave(&hsotg->lock, flags);

	if (role == USB_ROLE_HOST) {
		already = dwc2_ovr_avalid(hsotg, true);
	} else if (role == USB_ROLE_DEVICE) {
		already = dwc2_ovr_bvalid(hsotg, true);
		/* This clear DCTL.SFTDISCON bit */
		dwc2_hsotg_core_connect(hsotg);
	} else {
		if (dwc2_is_device_mode(hsotg)) {
		    if (!dwc2_ovr_bvalid(hsotg, false))
			/* This set DCTL.SFTDISCON bit */
			dwc2_hsotg_core_disconnect(hsotg);
		} else {
			dwc2_ovr_avalid(hsotg, false);
		}
	}

	spin_unlock_irqrestore(&hsotg->lock, flags);

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

	dev_dbg(hsotg->dev, "%s-session valid\n",
		role == USB_ROLE_NONE ? "No" :
		role == USB_ROLE_HOST ? "A" : "B");

	return 0;
}


dwc2_force_mode is called outside the spin_lock_irqsave so the kernel 
should not complain. I've tested on my setup and the behavior seems the 
same.

Regards,
Amelie

Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Martin Blumenstingl]

Hello Amelie,

sorry for the late reply

On Wed, Jul 8, 2020 at 6:00 PM Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
[...]
> Could you please test with:
>
> static int dwc2_drd_role_sw_set(struct device *dev, enum usb_role role)
> {
>         struct dwc2_hsotg *hsotg = dev_get_drvdata(dev);
>         unsigned long flags;
>         int already = 0;
>
>         /* Skip session not in line with dr_mode */
>         if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
>             (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
>                 return -EINVAL;
>
>         /* Skip session if core is in test mode */
>         if (role == USB_ROLE_NONE && hsotg->test_mode) {
>                 dev_dbg(hsotg->dev, "Core is in test mode\n");
>                 return -EBUSY;
>         }
>
>         spin_lock_irqsave(&hsotg->lock, flags);
>
>         if (role == USB_ROLE_HOST) {
>                 already = dwc2_ovr_avalid(hsotg, true);
>         } else if (role == USB_ROLE_DEVICE) {
>                 already = dwc2_ovr_bvalid(hsotg, true);
>                 /* This clear DCTL.SFTDISCON bit */
>                 dwc2_hsotg_core_connect(hsotg);
>         } else {
>                 if (dwc2_is_device_mode(hsotg)) {
>                     if (!dwc2_ovr_bvalid(hsotg, false))
>                         /* This set DCTL.SFTDISCON bit */
>                         dwc2_hsotg_core_disconnect(hsotg);
>                 } else {
>                         dwc2_ovr_avalid(hsotg, false);
>                 }
>         }
>
>         spin_unlock_irqrestore(&hsotg->lock, flags);
>
>         if (!already &&
>             role != USB_ROLE_NONE && hsotg->dr_mode == USB_DR_MODE_OTG)
>                 /* This will raise a Connector ID Status Change Interrupt */
>                 dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
>
>         dev_dbg(hsotg->dev, "%s-session valid\n",
>                 role == USB_ROLE_NONE ? "No" :
>                 role == USB_ROLE_HOST ? "A" : "B");
>
>         return 0;
> }
>
>
> dwc2_force_mode is called outside the spin_lock_irqsave so the kernel
> should not complain. I've tested on my setup and the behavior seems the
> same.
this one is looking good - the previous kernel warnings are now gone!
thank you very much


Best regards,
Martin

Re: [PATCH 0/3] Add USB role switch support to DWC2

[Alexandre Torgue]

Hi Amélie

On 6/16/20 4:07 PM, Amelie Delaunay wrote:
> When using usb-c connector (but it can also be the case with a micro-b
> connector), iddig, avalid, bvalid, vbusvalid input signals may not be
> connected to the DWC2 OTG controller.
> DWC2 OTG controller features an overriding control of the PHY voltage valid
> and ID input signals.
> So, missing signals can be forced using usb role from usb role switch and
> this override feature.
> 
> This series adds support for usb role switch to dwc2, by using overriding
> control of the PHY voltage valid and ID input signals.
> 
> It has been tested on stm32mp157c-dk2 [1], which has a Type-C connector
> managed by a Type-C port controller, and connected to USB OTG controller.
> 
> [1] https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html
> 
> Amelie Delaunay (3):
>    usb: dwc2: override PHY input signals with usb role switch support
>    usb: dwc2: don't use ID/Vbus detection if usb-role-switch on STM32MP15
>      SoCs
>    ARM: dts: stm32: enable usb-role-switch on USB OTG on stm32mp15xx-dkx
> 
>   arch/arm/boot/dts/stm32mp15xx-dkx.dtsi |   2 +-
>   drivers/usb/dwc2/Kconfig               |   1 +
>   drivers/usb/dwc2/Makefile              |   2 +-
>   drivers/usb/dwc2/core.h                |   8 ++
>   drivers/usb/dwc2/drd.c                 | 190 +++++++++++++++++++++++++
>   drivers/usb/dwc2/gadget.c              |   2 +-
>   drivers/usb/dwc2/params.c              |   4 +-
>   drivers/usb/dwc2/platform.c            |  13 ++
>   8 files changed, 218 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/usb/dwc2/drd.c
> 

DT patch applied on stm32-next.

Thanks
Alex

Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Amelie DELAUNAY]

Hi Felipe,
Hi Martin,

I saw the issue reported ([balbi-usb:testing/next 2/17] 
drivers/usb/dwc2/drd.c:80:38: error: no member named 'test_mode' in 
'struct dwc2_hsotg'). I prepare a V2 fixing it + adressing Martin's 
comments.

Regards,
Amelie

On 7/19/20 9:56 PM, Martin Blumenstingl wrote:
> Hello Amelie,
> 
> sorry for the late reply
> 
> On Wed, Jul 8, 2020 at 6:00 PM Amelie DELAUNAY <amelie.delaunay@st.com> 
> wrote:
> [...]
>> Could you please test with:
>>
>> static int dwc2_drd_role_sw_set(struct device *dev, enum usb_role role)
>> {
>>         struct dwc2_hsotg *hsotg = dev_get_drvdata(dev);
>>         unsigned long flags;
>>         int already = 0;
>>
>>         /* Skip session not in line with dr_mode */
>>         if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
>>             (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
>>                 return -EINVAL;
>>
>>         /* Skip session if core is in test mode */
>>         if (role == USB_ROLE_NONE && hsotg->test_mode) {
>>                 dev_dbg(hsotg->dev, "Core is in test mode\n");
>>                 return -EBUSY;
>>         }
>>
>>         spin_lock_irqsave(&hsotg->lock, flags);
>>
>>         if (role == USB_ROLE_HOST) {
>>                 already = dwc2_ovr_avalid(hsotg, true);
>>         } else if (role == USB_ROLE_DEVICE) {
>>                 already = dwc2_ovr_bvalid(hsotg, true);
>>                 /* This clear DCTL.SFTDISCON bit */
>>                 dwc2_hsotg_core_connect(hsotg);
>>         } else {
>>                 if (dwc2_is_device_mode(hsotg)) {
>>                     if (!dwc2_ovr_bvalid(hsotg, false))
>>                         /* This set DCTL.SFTDISCON bit */
>>                         dwc2_hsotg_core_disconnect(hsotg);
>>                 } else {
>>                         dwc2_ovr_avalid(hsotg, false);
>>                 }
>>         }
>>
>>         spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>>         if (!already &&
>>             role != USB_ROLE_NONE && hsotg->dr_mode == USB_DR_MODE_OTG)
>>                 /* This will raise a Connector ID Status Change Interrupt */
>>                 dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
>>
>>         dev_dbg(hsotg->dev, "%s-session valid\n",
>>                 role == USB_ROLE_NONE ? "No" :
>>                 role == USB_ROLE_HOST ? "A" : "B");
>>
>>         return 0;
>> }
>>
>>
>> dwc2_force_mode is called outside the spin_lock_irqsave so the kernel
>> should not complain. I've tested on my setup and the behavior seems the
>> same.
> this one is looking good - the previous kernel warnings are now gone!
> thank you very much
> 
> 
> Best regards,
> Martin

Re: [PATCH 0/3] Add USB role switch support to DWC2

[Amelie DELAUNAY]

Hi Felipe,

I saw that you took DT patch (ARM: dts: stm32: enable usb-role-switch on 
USB OTG on stm32mp15xx-dkx) in your next branch. As it was already in
Alex' stm32-next branch, a potential merge conflict could occurred.

Regards,
Amelie

On 7/21/20 10:54 AM, Alexandre Torgue wrote:
> Hi Amélie
> 
> On 6/16/20 4:07 PM, Amelie Delaunay wrote:
>> When using usb-c connector (but it can also be the case with a micro-b
>> connector), iddig, avalid, bvalid, vbusvalid input signals may not be
>> connected to the DWC2 OTG controller.
>> DWC2 OTG controller features an overriding control of the PHY voltage 
>> valid
>> and ID input signals.
>> So, missing signals can be forced using usb role from usb role switch and
>> this override feature.
>>
>> This series adds support for usb role switch to dwc2, by using overriding
>> control of the PHY voltage valid and ID input signals.
>>
>> It has been tested on stm32mp157c-dk2 [1], which has a Type-C connector
>> managed by a Type-C port controller, and connected to USB OTG controller.
>>
>> [1] https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html
>>
>> Amelie Delaunay (3):
>>    usb: dwc2: override PHY input signals with usb role switch support
>>    usb: dwc2: don't use ID/Vbus detection if usb-role-switch on STM32MP15
>>      SoCs
>>    ARM: dts: stm32: enable usb-role-switch on USB OTG on stm32mp15xx-dkx
>>
>>   arch/arm/boot/dts/stm32mp15xx-dkx.dtsi |   2 +-
>>   drivers/usb/dwc2/Kconfig               |   1 +
>>   drivers/usb/dwc2/Makefile              |   2 +-
>>   drivers/usb/dwc2/core.h                |   8 ++
>>   drivers/usb/dwc2/drd.c                 | 190 +++++++++++++++++++++++++
>>   drivers/usb/dwc2/gadget.c              |   2 +-
>>   drivers/usb/dwc2/params.c              |   4 +-
>>   drivers/usb/dwc2/platform.c            |  13 ++
>>   8 files changed, 218 insertions(+), 4 deletions(-)
>>   create mode 100644 drivers/usb/dwc2/drd.c
>>
> 
> DT patch applied on stm32-next.
> 
> Thanks
> Alex

Re: [PATCH 0/3] Add USB role switch support to DWC2

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]


(please, no top-posting ;-)

Hi,

Amelie DELAUNAY <amelie.delaunay@st.com> writes:

> Hi Felipe,
>
> I saw that you took DT patch (ARM: dts: stm32: enable usb-role-switch on 
> USB OTG on stm32mp15xx-dkx) in your next branch. As it was already in
> Alex' stm32-next branch, a potential merge conflict could occurred.

Thanks for letting me know, I have dropped it.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]