Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1615161 > unrolled thread

[PATCH v3 0/3] usb: dwc3: dual-role support

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] usb: dwc3: dual-role support Roger Quadros <rogerq@ti.com> - 2017-04-03 14:30 +0200
    [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros <rogerq@ti.com> - 2017-04-03 14:30 +0200
      Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support kbuild test robot <lkp@intel.com> - 2017-04-03 21:30 +0200
        Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support Roger Quadros <rogerq@ti.com> - 2017-04-04 09:50 +0200
      [PATCH v4 3/3] usb: dwc3: Add dual-role support Roger Quadros <rogerq@ti.com> - 2017-04-04 09:50 +0200
    [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Roger Quadros <rogerq@ti.com> - 2017-04-03 14:30 +0200
      Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Alan Stern <stern@rowland.harvard.edu> - 2017-04-03 16:20 +0200
        Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Felipe Balbi <balbi@kernel.org> - 2017-04-04 09:50 +0200
          Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Alan Stern <stern@rowland.harvard.edu> - 2017-04-04 16:20 +0200
            Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Felipe Balbi <balbi@kernel.org> - 2017-04-05 10:40 +0200
              Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same  gadget device Alan Stern <stern@rowland.harvard.edu> - 2017-04-05 16:20 +0200
                Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device Felipe Balbi <balbi@kernel.org> - 2017-04-10 12:10 +0200
    [PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode Roger Quadros <rogerq@ti.com> - 2017-04-03 14:30 +0200

#1615161 — [PATCH v3 0/3] usb: dwc3: dual-role support

Hi,

Taking a simplistic approach this time. We don't use the OTG controller
block at all. Instead we just rely on ID events via extcon framework.

I tried to get rid of workqueue but unfortunately it causes too much pain
when the UDC is unregistered.

https://hastebin.com/upugaqogol.xml
https://hastebin.com/yayaduqodo.xml

So I've still kept the workqueue around.

We also have debugfs role switching, but I don't yet see how we
can use this for real testing as I still need to manually plug/unplug
the USB cables (host vs device) to test switching :).

v3:
- restructure and simplify. Remove OTG controller code, only rely on extcon.

cheers,
-roger

Roger Quadros (3):
  usb: udc: allow adding and removing the same gadget device
  usb: dwc3: make role-switching work with debugfs/mode
  usb: dwc3: Add dual-role support

 drivers/usb/dwc3/Makefile     |   4 +
 drivers/usb/dwc3/core.c       |  18 ++---
 drivers/usb/dwc3/core.h       |  22 ++++++
 drivers/usb/dwc3/debugfs.c    |  49 +++++++++++--
 drivers/usb/dwc3/drd.c        | 167 ++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c |   1 +
 6 files changed, 242 insertions(+), 19 deletions(-)
 create mode 100644 drivers/usb/dwc3/drd.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1615162 — [PATCH v3 3/3] usb: dwc3: Add dual-role support

If dr_mode is "otg" then support dual role mode of operation.
Currently this mode is only supported when an extcon handle is
present in the dwc3 device tree node. This is needed to
get the ID status events of the port.

We're using a workqueue to manage the dual-role state transitions
as the extcon notifier (dwc3_drd_notifier) is called in an atomic
context by extcon_sync() and this doesn't go well with
usb_del_gadget_udc() causing a lockdep and softirq warning.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/Makefile |   4 ++
 drivers/usb/dwc3/core.c   |  15 +----
 drivers/usb/dwc3/core.h   |  19 ++++++
 drivers/usb/dwc3/drd.c    | 167 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+), 12 deletions(-)
 create mode 100644 drivers/usb/dwc3/drd.c

diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index ffca340..f15fabb 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -17,6 +17,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
 	dwc3-y				+= gadget.o ep0.o
 endif
 
+ifneq ($(CONFIG_USB_DWC3_DUAL_ROLE),)
+	dwc3-y				+= drd.o
+endif
+
 ifneq ($(CONFIG_USB_DWC3_ULPI),)
 	dwc3-y				+= ulpi.o
 endif
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index bf917c2..15c05a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -864,12 +864,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		/* start in peripheral role by default */
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-		ret = dwc3_gadget_init(dwc);
+		ret = dwc3_drd_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize gadget\n");
+				dev_err(dev, "failed to initialize dual-role\n");
 			return ret;
 		}
 		break;
@@ -891,14 +889,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		/* role might have changed since start */
-		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
-			dwc3_host_exit(dwc);
-			/* Add back UDC to match dwc3_gadget_exit() */
-			usb_add_gadget_udc(dwc->dev, &dwc->gadget);
-		}
-
-		dwc3_gadget_exit(dwc);
+		dwc3_drd_exit(dwc);
 		break;
 	default:
 		/* do nothing */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index adb04af..8402d8f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -786,6 +786,10 @@ struct dwc3_scratchpad_array {
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
  * @current_dr_role: current role of operation when in dual-role mode
+ * @edev: extcon handle
+ * @edev_nb: extcon notifier
+ * @drd_work: dual-role work
+ * @drd_prevent_change: flag to prevent dual-role state change
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -903,6 +907,11 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 	u32			current_dr_role;
+	struct extcon_dev	*edev;
+	struct notifier_block	edev_nb;
+	bool			drd_prevent_change;
+	struct work_struct	drd_work;
+
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
@@ -1225,6 +1234,16 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
 { return 0; }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
+int dwc3_drd_init(struct dwc3 *dwc);
+void dwc3_drd_exit(struct dwc3 *dwc);
+#else
+static inline int dwc3_drd_init(struct dwc3 *dwc)
+{ return 0; }
+static void dwc3_drd_exit(struct dwc3 *dwc);
+{ }
+#endif
+
 /* power management interface */
 #if !IS_ENABLED(CONFIG_USB_DWC3_HOST)
 int dwc3_gadget_suspend(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
new file mode 100644
index 0000000..c9b02a3
--- /dev/null
+++ b/drivers/usb/dwc3/drd.c
@@ -0,0 +1,167 @@
+/**
+ * drd.c - DesignWare USB3 DRD Controller Dual-role support
+ *
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: Roger Quadros <rogerq@ti.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/extcon.h>
+
+#include "debug.h"
+#include "core.h"
+#include "gadget.h"
+
+static void dwc3_drd_update(struct dwc3 *dwc)
+{
+	int id;
+	int new_role;
+	unsigned long flags;
+
+	if (dwc->drd_prevent_change)
+		return;
+
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	/* Host means ID is 0 */
+	id = !id;
+
+	if (!id)
+		new_role = DWC3_GCTL_PRTCAP_HOST;
+	else
+		new_role = DWC3_GCTL_PRTCAP_DEVICE;
+
+	if (dwc->current_dr_role == new_role)
+		return;
+
+	/* stop old role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		usb_del_gadget_udc(&dwc->gadget);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, new_role);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_event_buffers_setup(dwc);
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		break;
+	default:
+		break;
+	}
+}
+
+static void dwc3_drd_work(struct work_struct *work)
+{
+	struct dwc3 *dwc = container_of(work, struct dwc3,
+					drd_work);
+	dwc3_drd_update(dwc);
+}
+
+static int dwc3_drd_notifier(struct notifier_block *nb,
+			     unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb);
+
+	queue_work(system_power_efficient_wq, &dwc->drd_work);
+
+	return NOTIFY_DONE;
+}
+
+int dwc3_drd_init(struct dwc3 *dwc)
+{
+	int ret;
+	int id;
+	struct device *dev = dwc->dev;
+
+	INIT_WORK(&dwc->drd_work, dwc3_drd_work);
+
+	if (dev->of_node) {
+		if (of_property_read_bool(dev->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+
+		if (IS_ERR(dwc->edev))
+			return PTR_ERR(dwc->edev);
+	} else {
+		return -ENODEV;
+	}
+
+	dwc->edev_nb.notifier_call = dwc3_drd_notifier;
+	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
+				       &dwc->edev_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
+		return -ENODEV;
+	}
+
+	/* sanity check id & vbus states */
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	if (id < 0) {
+		dev_err(dwc->dev, "Invalid USB cable state. ID %d", id);
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* start in peripheral role by default */
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	ret = dwc3_gadget_init(dwc);
+	if (ret)
+		goto fail;
+
+	/* check & update drd state */
+	dwc3_drd_update(dwc);
+
+	return 0;
+
+fail:
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	return ret;
+}
+
+void dwc3_drd_exit(struct dwc3 *dwc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->drd_prevent_change = true;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	/* role might have changed since start, stop active controller */
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
+		dwc3_host_exit(dwc);
+		/* Add back UDC to match dwc3_gadget_exit() */
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+	}
+
+	dwc3_gadget_exit(dwc);
+}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1615511 — Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support

[Multipart message — attachments visible in raw view] — view raw

Hi Roger,

[auto build test ERROR on balbi-usb/next]
[also build test ERROR on v4.11-rc5 next-20170403]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Roger-Quadros/usb-dwc3-dual-role-support/20170404-022401
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: i386-randconfig-x010-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/usb/dwc3/core.c:44:0:
>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
    { }
    ^
>> drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' used but never defined
    static void dwc3_drd_exit(struct dwc3 *dwc);
                ^~~~~~~~~~~~~
--
   In file included from drivers/usb/dwc3/trace.h:27:0,
                    from drivers/usb/dwc3/trace.c:19:
>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
    { }
    ^
   In file included from drivers/usb/dwc3/trace.h:27:0,
                    from drivers/usb/dwc3/trace.c:19:
   drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' declared 'static' but never defined [-Wunused-function]
    static void dwc3_drd_exit(struct dwc3 *dwc);
                ^~~~~~~~~~~~~

vim +1243 drivers/usb/dwc3/core.h

  1236	#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
  1237	int dwc3_drd_init(struct dwc3 *dwc);
  1238	void dwc3_drd_exit(struct dwc3 *dwc);
  1239	#else
  1240	static inline int dwc3_drd_init(struct dwc3 *dwc)
  1241	{ return 0; }
> 1242	static void dwc3_drd_exit(struct dwc3 *dwc);
> 1243	{ }
  1244	#endif
  1245	
  1246	/* power management interface */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1615760 — Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support

Felipe,

I'll send a revised patch to fix this.

cheers,
-roger

On 03/04/17 22:21, kbuild test robot wrote:
> Hi Roger,
> 
> [auto build test ERROR on balbi-usb/next]
> [also build test ERROR on v4.11-rc5 next-20170403]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Roger-Quadros/usb-dwc3-dual-role-support/20170404-022401
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
> config: i386-randconfig-x010-201714 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    In file included from drivers/usb/dwc3/core.c:44:0:
>>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
>     { }
>     ^
>>> drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' used but never defined
>     static void dwc3_drd_exit(struct dwc3 *dwc);
>                 ^~~~~~~~~~~~~
> --
>    In file included from drivers/usb/dwc3/trace.h:27:0,
>                     from drivers/usb/dwc3/trace.c:19:
>>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
>     { }
>     ^
>    In file included from drivers/usb/dwc3/trace.h:27:0,
>                     from drivers/usb/dwc3/trace.c:19:
>    drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' declared 'static' but never defined [-Wunused-function]
>     static void dwc3_drd_exit(struct dwc3 *dwc);
>                 ^~~~~~~~~~~~~
> 
> vim +1243 drivers/usb/dwc3/core.h
> 
>   1236	#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>   1237	int dwc3_drd_init(struct dwc3 *dwc);
>   1238	void dwc3_drd_exit(struct dwc3 *dwc);
>   1239	#else
>   1240	static inline int dwc3_drd_init(struct dwc3 *dwc)
>   1241	{ return 0; }
>> 1242	static void dwc3_drd_exit(struct dwc3 *dwc);
>> 1243	{ }
>   1244	#endif
>   1245	
>   1246	/* power management interface */
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

[toc] | [prev] | [next] | [standalone]

#1615757 — [PATCH v4 3/3] usb: dwc3: Add dual-role support

If dr_mode is "otg" then support dual role mode of operation.
Currently this mode is only supported when an extcon handle is
present in the dwc3 device tree node. This is needed to
get the ID status events of the port.

We're using a workqueue to manage the dual-role state transitions
as the extcon notifier (dwc3_drd_notifier) is called in an atomic
context by extcon_sync() and this doesn't go well with
usb_del_gadget_udc() causing a lockdep and softirq warning.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v4:
-fix build error if CONFIG_USB_DWC3_DUAL_ROLE is not set

 drivers/usb/dwc3/Makefile |   4 ++
 drivers/usb/dwc3/core.c   |  15 +----
 drivers/usb/dwc3/core.h   |  19 ++++++
 drivers/usb/dwc3/drd.c    | 167 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+), 12 deletions(-)
 create mode 100644 drivers/usb/dwc3/drd.c

diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index ffca340..f15fabb 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -17,6 +17,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
 	dwc3-y				+= gadget.o ep0.o
 endif
 
+ifneq ($(CONFIG_USB_DWC3_DUAL_ROLE),)
+	dwc3-y				+= drd.o
+endif
+
 ifneq ($(CONFIG_USB_DWC3_ULPI),)
 	dwc3-y				+= ulpi.o
 endif
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index bf917c2..15c05a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -864,12 +864,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		/* start in peripheral role by default */
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-		ret = dwc3_gadget_init(dwc);
+		ret = dwc3_drd_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize gadget\n");
+				dev_err(dev, "failed to initialize dual-role\n");
 			return ret;
 		}
 		break;
@@ -891,14 +889,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		/* role might have changed since start */
-		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
-			dwc3_host_exit(dwc);
-			/* Add back UDC to match dwc3_gadget_exit() */
-			usb_add_gadget_udc(dwc->dev, &dwc->gadget);
-		}
-
-		dwc3_gadget_exit(dwc);
+		dwc3_drd_exit(dwc);
 		break;
 	default:
 		/* do nothing */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index adb04af..b1be4a3 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -786,6 +786,10 @@ struct dwc3_scratchpad_array {
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
  * @current_dr_role: current role of operation when in dual-role mode
+ * @edev: extcon handle
+ * @edev_nb: extcon notifier
+ * @drd_work: dual-role work
+ * @drd_prevent_change: flag to prevent dual-role state change
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -903,6 +907,11 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 	u32			current_dr_role;
+	struct extcon_dev	*edev;
+	struct notifier_block	edev_nb;
+	bool			drd_prevent_change;
+	struct work_struct	drd_work;
+
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
@@ -1225,6 +1234,16 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
 { return 0; }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
+int dwc3_drd_init(struct dwc3 *dwc);
+void dwc3_drd_exit(struct dwc3 *dwc);
+#else
+static inline int dwc3_drd_init(struct dwc3 *dwc)
+{ return 0; }
+static inline void dwc3_drd_exit(struct dwc3 *dwc)
+{ }
+#endif
+
 /* power management interface */
 #if !IS_ENABLED(CONFIG_USB_DWC3_HOST)
 int dwc3_gadget_suspend(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
new file mode 100644
index 0000000..c9b02a3
--- /dev/null
+++ b/drivers/usb/dwc3/drd.c
@@ -0,0 +1,167 @@
+/**
+ * drd.c - DesignWare USB3 DRD Controller Dual-role support
+ *
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: Roger Quadros <rogerq@ti.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/extcon.h>
+
+#include "debug.h"
+#include "core.h"
+#include "gadget.h"
+
+static void dwc3_drd_update(struct dwc3 *dwc)
+{
+	int id;
+	int new_role;
+	unsigned long flags;
+
+	if (dwc->drd_prevent_change)
+		return;
+
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	/* Host means ID is 0 */
+	id = !id;
+
+	if (!id)
+		new_role = DWC3_GCTL_PRTCAP_HOST;
+	else
+		new_role = DWC3_GCTL_PRTCAP_DEVICE;
+
+	if (dwc->current_dr_role == new_role)
+		return;
+
+	/* stop old role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		usb_del_gadget_udc(&dwc->gadget);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, new_role);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_event_buffers_setup(dwc);
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		break;
+	default:
+		break;
+	}
+}
+
+static void dwc3_drd_work(struct work_struct *work)
+{
+	struct dwc3 *dwc = container_of(work, struct dwc3,
+					drd_work);
+	dwc3_drd_update(dwc);
+}
+
+static int dwc3_drd_notifier(struct notifier_block *nb,
+			     unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb);
+
+	queue_work(system_power_efficient_wq, &dwc->drd_work);
+
+	return NOTIFY_DONE;
+}
+
+int dwc3_drd_init(struct dwc3 *dwc)
+{
+	int ret;
+	int id;
+	struct device *dev = dwc->dev;
+
+	INIT_WORK(&dwc->drd_work, dwc3_drd_work);
+
+	if (dev->of_node) {
+		if (of_property_read_bool(dev->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+
+		if (IS_ERR(dwc->edev))
+			return PTR_ERR(dwc->edev);
+	} else {
+		return -ENODEV;
+	}
+
+	dwc->edev_nb.notifier_call = dwc3_drd_notifier;
+	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
+				       &dwc->edev_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
+		return -ENODEV;
+	}
+
+	/* sanity check id & vbus states */
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	if (id < 0) {
+		dev_err(dwc->dev, "Invalid USB cable state. ID %d", id);
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* start in peripheral role by default */
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	ret = dwc3_gadget_init(dwc);
+	if (ret)
+		goto fail;
+
+	/* check & update drd state */
+	dwc3_drd_update(dwc);
+
+	return 0;
+
+fail:
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	return ret;
+}
+
+void dwc3_drd_exit(struct dwc3 *dwc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->drd_prevent_change = true;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	/* role might have changed since start, stop active controller */
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
+		dwc3_host_exit(dwc);
+		/* Add back UDC to match dwc3_gadget_exit() */
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+	}
+
+	dwc3_gadget_exit(dwc);
+}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1615163 — [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
repeatedly on the same gadget->dev structure.

We need to clear the gadget->dev structure so that kobject_init()
doesn't complain about already initialized object.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/gadget/udc/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d685d82..efce68e 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
 	device_unregister(&gadget->dev);
+	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1615229 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

On Mon, 3 Apr 2017, Roger Quadros wrote:

> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> repeatedly on the same gadget->dev structure.
> 
> We need to clear the gadget->dev structure so that kobject_init()
> doesn't complain about already initialized object.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/gadget/udc/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index d685d82..efce68e 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>  	flush_work(&gadget->work);
>  	device_unregister(&udc->dev);
>  	device_unregister(&gadget->dev);
> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>  }
>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);

Isn't this dangerous?  It's quite possible that the device_unregister() 
call on the previous line invokes the gadget->dev.release callback, 
which might deallocate gadget.  If that happens, your new memset will 
oops.

In general, if an object relies on reference counting for its lifetime, 
you cannot register and unregister it more than once.  A typical issue 
is that some code retains a reference to the old instance and tries to 
use it after the new instance has been registered, thereby messing up 
the new instance.  I don't know if that is possible in this case, but 
it is something to watch out for.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1615754 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 3 Apr 2017, Roger Quadros wrote:
>
>> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> repeatedly on the same gadget->dev structure.
>> 
>> We need to clear the gadget->dev structure so that kobject_init()
>> doesn't complain about already initialized object.
>> 
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/gadget/udc/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index d685d82..efce68e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>>  	flush_work(&gadget->work);
>>  	device_unregister(&udc->dev);
>>  	device_unregister(&gadget->dev);
>> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>>  }
>>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>
> Isn't this dangerous?  It's quite possible that the device_unregister() 

not on the gadget API, no.

> call on the previous line invokes the gadget->dev.release callback, 
> which might deallocate gadget.  If that happens, your new memset will 
> oops.

that won't happen. struct usb_gadget is a member of the UDC's private
structure, like this:

struct dwc3 {
	[...]
	struct usb_gadget	gadget;
	struct usb_gadget_driver *gadget_driver;
	[...]
};

I'm actually thinking that struct usb_gadget shouldn't have a struct
device at all. Just a pointer to a device, that would solve all these
issues.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1616061 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

On Tue, 4 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Mon, 3 Apr 2017, Roger Quadros wrote:
> >
> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> >> repeatedly on the same gadget->dev structure.
> >> 
> >> We need to clear the gadget->dev structure so that kobject_init()
> >> doesn't complain about already initialized object.
> >> 
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >>  drivers/usb/gadget/udc/core.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> >> index d685d82..efce68e 100644
> >> --- a/drivers/usb/gadget/udc/core.c
> >> +++ b/drivers/usb/gadget/udc/core.c
> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >>  	flush_work(&gadget->work);
> >>  	device_unregister(&udc->dev);
> >>  	device_unregister(&gadget->dev);
> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >>  }
> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >
> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> 
> not on the gadget API, no.
> 
> > call on the previous line invokes the gadget->dev.release callback, 
> > which might deallocate gadget.  If that happens, your new memset will 
> > oops.
> 
> that won't happen. struct usb_gadget is a member of the UDC's private
> structure, like this:
> 
> struct dwc3 {
> 	[...]
> 	struct usb_gadget	gadget;
> 	struct usb_gadget_driver *gadget_driver;
> 	[...]
> };

Yes.  So what?  Can't the UDC driver use the refcount inside struct 
usb_gadget to control the lifetime of its private structure?

(By the way, can you tell what's going on in net2280.c?  I must be
missing something; it looks like gadget_release() would quickly run
into problems because it calls dev_get_drvdata() for &gadget->dev, but
net2280_probe() never calls dev_set_drvdata() for that device.  
Furthermore, net2280_remove() continues to reference the net2280 struct
after calling usb_del_gadget_udc(), and it never does seem to do a
final put.)

> I'm actually thinking that struct usb_gadget shouldn't have a struct
> device at all. Just a pointer to a device, that would solve all these
> issues.

A pointer to which device?  The UDC?  That would change the directory 
layout in sysfs.

Or a pointer to a separate dynamically allocated device (the way struct 
usb_hcd contains a pointer to the root hub device)?  That would work.  
If the UDC driver wanted to re-register the gadget, it would have to 
allocate a new device.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1616688 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> >> repeatedly on the same gadget->dev structure.
>> >> 
>> >> We need to clear the gadget->dev structure so that kobject_init()
>> >> doesn't complain about already initialized object.
>> >> 
>> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> >> ---
>> >>  drivers/usb/gadget/udc/core.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >> 
>> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> >> index d685d82..efce68e 100644
>> >> --- a/drivers/usb/gadget/udc/core.c
>> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >>  	flush_work(&gadget->work);
>> >>  	device_unregister(&udc->dev);
>> >>  	device_unregister(&gadget->dev);
>> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >
>> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> 
>> not on the gadget API, no.
>> 
>> > call on the previous line invokes the gadget->dev.release callback, 
>> > which might deallocate gadget.  If that happens, your new memset will 
>> > oops.
>> 
>> that won't happen. struct usb_gadget is a member of the UDC's private
>> structure, like this:
>> 
>> struct dwc3 {
>> 	[...]
>> 	struct usb_gadget	gadget;
>> 	struct usb_gadget_driver *gadget_driver;
>> 	[...]
>> };
>
> Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> usb_gadget to control the lifetime of its private structure?

nope, not being used. At least not yet.

> (By the way, can you tell what's going on in net2280.c?  I must be
> missing something; it looks like gadget_release() would quickly run
> into problems because it calls dev_get_drvdata() for &gadget->dev, but
> net2280_probe() never calls dev_set_drvdata() for that device.  
> Furthermore, net2280_remove() continues to reference the net2280 struct
> after calling usb_del_gadget_udc(), and it never does seem to do a
> final put.)

static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
	struct net2280		*dev;
	unsigned long		resource, len;
	void			__iomem *base = NULL;
	int			retval, i;

	/* alloc, and start init */
	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
	if (dev == NULL) {
		retval = -ENOMEM;
		goto done;
	}

	pci_set_drvdata(pdev, dev);
	^^^^^^^^^^^^^^^^^^^^^^^^^^^

>> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> device at all. Just a pointer to a device, that would solve all these
>> issues.
>
> A pointer to which device?  The UDC?  That would change the directory 
> layout in sysfs.

indeed. Would that be a problem?

> Or a pointer to a separate dynamically allocated device (the way struct 
> usb_hcd contains a pointer to the root hub device)?  That would work.  
> If the UDC driver wanted to re-register the gadget, it would have to 
> allocate a new device.

That could be done as well, if maintaining the directory structure is a
must.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1616984 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

On Wed, 5 Apr 2017, Felipe Balbi wrote:

> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >> >>  	flush_work(&gadget->work);
> >> >>  	device_unregister(&udc->dev);
> >> >>  	device_unregister(&gadget->dev);
> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >
> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> >> 
> >> not on the gadget API, no.
> >> 
> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> > oops.
> >> 
> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> structure, like this:
> >> 
> >> struct dwc3 {
> >> 	[...]
> >> 	struct usb_gadget	gadget;
> >> 	struct usb_gadget_driver *gadget_driver;
> >> 	[...]
> >> };
> >
> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> > usb_gadget to control the lifetime of its private structure?
> 
> nope, not being used. At least not yet.

I'm not convinced (yet)...

> > (By the way, can you tell what's going on in net2280.c?  I must be
> > missing something; it looks like gadget_release() would quickly run
> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
> > net2280_probe() never calls dev_set_drvdata() for that device.  
> > Furthermore, net2280_remove() continues to reference the net2280 struct
> > after calling usb_del_gadget_udc(), and it never does seem to do a
> > final put.)
> 
> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> 	struct net2280		*dev;
> 	unsigned long		resource, len;
> 	void			__iomem *base = NULL;
> 	int			retval, i;
> 
> 	/* alloc, and start init */
> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> 	if (dev == NULL) {
> 		retval = -ENOMEM;
> 		goto done;
> 	}
> 
> 	pci_set_drvdata(pdev, dev);
> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^

That sets the driver data in the struct pci_dev, not in
dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
driver data in dev->gadget.dev.

(Even after all these years, I still get bothered by the way Dave 
Brownell used to call everything "dev"...  IIRC, at one time he had a 
line of code that went something like:  dev->dev.dev = &pdev->dev !)

> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
> >> device at all. Just a pointer to a device, that would solve all these
> >> issues.
> >
> > A pointer to which device?  The UDC?  That would change the directory 
> > layout in sysfs.
> 
> indeed. Would that be a problem?

Possibly for some userspace tool.

> > Or a pointer to a separate dynamically allocated device (the way struct 
> > usb_hcd contains a pointer to the root hub device)?  That would work.  
> > If the UDC driver wanted to re-register the gadget, it would have to 
> > allocate a new device.
> 
> That could be done as well, if maintaining the directory structure is a
> must.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1619775 — Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Wed, 5 Apr 2017, Felipe Balbi wrote:
>
>> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >> >>  	flush_work(&gadget->work);
>> >> >>  	device_unregister(&udc->dev);
>> >> >>  	device_unregister(&gadget->dev);
>> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >> >>  }
>> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >
>> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> >> 
>> >> not on the gadget API, no.
>> >> 
>> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> > which might deallocate gadget.  If that happens, your new memset will 
>> >> > oops.
>> >> 
>> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> structure, like this:
>> >> 
>> >> struct dwc3 {
>> >> 	[...]
>> >> 	struct usb_gadget	gadget;
>> >> 	struct usb_gadget_driver *gadget_driver;
>> >> 	[...]
>> >> };
>> >
>> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> > usb_gadget to control the lifetime of its private structure?
>> 
>> nope, not being used. At least not yet.
>
> I'm not convinced (yet)...
>
>> > (By the way, can you tell what's going on in net2280.c?  I must be
>> > missing something; it looks like gadget_release() would quickly run
>> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
>> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> > final put.)
>> 
>> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> 	struct net2280		*dev;
>> 	unsigned long		resource, len;
>> 	void			__iomem *base = NULL;
>> 	int			retval, i;
>> 
>> 	/* alloc, and start init */
>> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> 	if (dev == NULL) {
>> 		retval = -ENOMEM;
>> 		goto done;
>> 	}
>> 
>> 	pci_set_drvdata(pdev, dev);
>> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> That sets the driver data in the struct pci_dev, not in
> dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> driver data in dev->gadget.dev.

hmmm, indeed. The same is happening with other callers of
usb_add_gadget_udc_release().

I guess this should be enough?

@@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 static void gadget_release(struct device *_dev)
 {
-	struct net2280	*dev = dev_get_drvdata(_dev);
+	struct net2280	*dev = dev_get_drvdata(_dev->parent);
 
 	kfree(dev);
 }


> (Even after all these years, I still get bothered by the way Dave 
> Brownell used to call everything "dev"...  IIRC, at one time he had a 
> line of code that went something like:  dev->dev.dev = &pdev->dev !)

:-)

>> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> >> device at all. Just a pointer to a device, that would solve all these
>> >> issues.
>> >
>> > A pointer to which device?  The UDC?  That would change the directory 
>> > layout in sysfs.
>> 
>> indeed. Would that be a problem?
>
> Possibly for some userspace tool.

yeah, we can do dynamic allocation of the device pointer, no issue.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1615165 — [PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode

If dr_mode == "otg", we start by default in PERIPHERAL mode.
Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
is called.

When debugfs/mode is changed AND we're in dual-role mode,
handle the switch by stopping and starting the respective
host/gadget controllers.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c    | 21 +++++++++++---------
 drivers/usb/dwc3/core.h    |  3 +++
 drivers/usb/dwc3/debugfs.c | 49 +++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..bf917c2 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
 	reg |= DWC3_GCTL_PRTCAPDIR(mode);
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	dwc->current_dr_role = mode;
 }
 
 u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
@@ -277,7 +279,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length)
  *
  * Returns 0 on success otherwise negative errno.
  */
-static int dwc3_event_buffers_setup(struct dwc3 *dwc)
+int dwc3_event_buffers_setup(struct dwc3 *dwc)
 {
 	struct dwc3_event_buffer	*evt;
 
@@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		ret = dwc3_host_init(dwc);
-		if (ret) {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize host\n");
-			return ret;
-		}
-
+		/* start in peripheral role by default */
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
@@ -894,7 +891,13 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_host_exit(dwc);
+		/* role might have changed since start */
+		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
+			dwc3_host_exit(dwc);
+			/* Add back UDC to match dwc3_gadget_exit() */
+			usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		}
+
 		dwc3_gadget_exit(dwc);
 		break;
 	default:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7ffdee5..adb04af 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -785,6 +785,7 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @current_dr_role: current role of operation when in dual-role mode
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -901,6 +902,7 @@ struct dwc3 {
 	size_t			regs_size;
 
 	enum usb_dr_mode	dr_mode;
+	u32			current_dr_role;
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
@@ -1167,6 +1169,7 @@ struct dwc3_gadget_ep_cmd_params {
 /* prototypes */
 void dwc3_set_mode(struct dwc3 *dwc, u32 mode);
 u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type);
+int dwc3_event_buffers_setup(struct dwc3 *dwc);
 
 /* check whether we are on the DWC_usb3 core */
 static inline bool dwc3_is_usb3(struct dwc3 *dwc)
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 31926dd..a74a83b 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
 		return -EFAULT;
 
 	if (!strncmp(buf, "host", 4))
-		mode |= DWC3_GCTL_PRTCAP_HOST;
+		mode = DWC3_GCTL_PRTCAP_HOST;
 
 	if (!strncmp(buf, "device", 6))
-		mode |= DWC3_GCTL_PRTCAP_DEVICE;
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
 
 	if (!strncmp(buf, "otg", 3))
-		mode |= DWC3_GCTL_PRTCAP_OTG;
+		mode = DWC3_GCTL_PRTCAP_OTG;
 
-	if (mode) {
-		spin_lock_irqsave(&dwc->lock, flags);
-		dwc3_set_mode(dwc, mode);
-		spin_unlock_irqrestore(&dwc->lock, flags);
+	if (!mode)
+		return -EINVAL;
+
+	if (mode == dwc->current_dr_role)
+		goto exit;
+
+	/* prevent role switching if we're not dual-role */
+	if (dwc->dr_mode != USB_DR_MODE_OTG)
+		return -EINVAL;
+
+	/* stop old role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		usb_del_gadget_udc(&dwc->gadget);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, mode);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_event_buffers_setup(dwc);
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		break;
+	default:
+		break;
 	}
+exit:
 	return count;
 }
 
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web