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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
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/
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/
Hi Amelie,
On Tue, Jul 7, 2020 at 6:13 PM Amelie DELAUNAY <[email protected]> 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
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 <[email protected]> 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
Hello Amelie,
sorry for the late reply
On Wed, Jul 8, 2020 at 6:00 PM Amelie DELAUNAY <[email protected]> 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
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 <[email protected]>
> 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