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

Groups > linux.kernel > #1420564 > unrolled thread

[PATCH v11 08/14] usb: otg: add OTG/dual-role core

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v11 08/14] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-06-13 10:00 +0200
    Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-20 09:50 +0200
      Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-06-20 12:20 +0200
        Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-06-20 14:30 +0200
          Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-20 14:50 +0200
        Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-20 14:30 +0200
          Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 08:50 +0200
            Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 09:30 +0200
              Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 10:20 +0200
                Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 10:30 +0200
                  Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 14:00 +0200
                    Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 14:40 +0200
                      Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 15:30 +0200
                        Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 16:50 +0200
                          Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-22 05:50 +0200
                            Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-22 09:00 +0200
                              Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-22 09:40 +0200
                                Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-22 10:10 +0200
                            RE: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-06-23 09:50 +0200
        RE: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-06-21 04:40 +0200
          RE: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 09:30 +0200
      Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-20 14:00 +0200
        Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-20 14:20 +0200
          Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 08:40 +0200
            Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 09:30 +0200
              Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 11:20 +0200
                Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 12:10 +0200
                  Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Tony Lindgren <tony@atomide.com> - 2016-06-21 13:00 +0200
                    Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-21 13:00 +0200
                  Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-21 18:50 +0200
                    Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-22 09:00 +0200
                      Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-06-22 10:00 +0200
                        Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-22 10:20 +0200
                      Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-06-22 10:00 +0200
                        Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Felipe Balbi <balbi@kernel.org> - 2016-06-22 10:20 +0200
                          Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-06-22 10:40 +0200
                        RE: [PATCH v11 08/14] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-06-23 09:50 +0200

Page 1 of 2  [1] 2  Next page →

#1420564 — [PATCH v11 08/14] usb: otg: add OTG/dual-role core

It provides APIs for the following tasks

- Registering an OTG/dual-role capable controller
- Registering Host and Gadget controllers to OTG core
- Providing inputs to and kicking the OTG state machine

Provide a dual-role device (DRD) state machine.
DRD mode is a reduced functionality OTG mode. In this mode
we don't support SRP, HNP and dynamic role-swap.

In DRD operation, the controller mode (Host or Peripheral)
is decided based on the ID pin status. Once a cable plug (Type-A
or Type-B) is attached the controller selects the state
and doesn't change till the cable in unplugged and a different
cable type is inserted.

As we don't need most of the complex OTG states and OTG timers
we implement a lean DRD state machine in usb-otg.c.
The DRD state machine is only interested in 2 hardware inputs
'id' and 'b_sess_vld'.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v11:
- remove usb_otg_kick_fsm().
- typo fixes: structa/structure, upto/up to.
- remove "obj-$(CONFIG_USB_OTG_CORE)     += common/" from Makefile.

 drivers/usb/Kconfig          |  18 +
 drivers/usb/common/Makefile  |   6 +-
 drivers/usb/common/usb-otg.c | 877 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/Kconfig     |  14 -
 drivers/usb/gadget/Kconfig   |   1 +
 include/linux/usb/gadget.h   |   2 +
 include/linux/usb/hcd.h      |   1 +
 include/linux/usb/otg-fsm.h  |   7 +
 include/linux/usb/otg.h      | 174 ++++++++-
 9 files changed, 1070 insertions(+), 30 deletions(-)
 create mode 100644 drivers/usb/common/usb-otg.c

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8689dcb..ed596ec 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -32,6 +32,23 @@ if USB_SUPPORT
 config USB_COMMON
 	tristate
 
+config USB_OTG_CORE
+	tristate
+
+config USB_OTG
+	bool "OTG/Dual-role support"
+	depends on PM && USB && USB_GADGET
+	default n
+	---help---
+	  The most notable feature of USB OTG is support for a
+	  "Dual-Role" device, which can act as either a device
+	  or a host. The initial role is decided by the type of
+	  plug inserted and can be changed later when two dual
+	  role devices talk to each other.
+
+	  Select this only if your board has Mini-AB/Micro-AB
+	  connector.
+
 config USB_ARCH_HAS_HCD
 	def_bool y
 
@@ -40,6 +57,7 @@ config USB
 	tristate "Support for Host-side USB"
 	depends on USB_ARCH_HAS_HCD
 	select USB_COMMON
+	select USB_OTG_CORE
 	select NLS  # for UTF-8 strings
 	---help---
 	  Universal Serial Bus (USB) is a specification for a serial bus
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index f8f2c88..5122b3f 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -7,5 +7,7 @@ usb-common-y			  += common.o
 usb-common-$(CONFIG_USB_LED_TRIG) += led.o
 
 obj-$(CONFIG_USB_ULPI_BUS)	+= ulpi.o
-usbotg-y		:= usb-otg-fsm.o
-obj-$(CONFIG_USB_OTG)	+= usbotg.o
+ifeq ($(CONFIG_USB_OTG),y)
+usbotg-y		:= usb-otg.o usb-otg-fsm.o
+obj-$(CONFIG_USB_OTG_CORE)	+= usbotg.o
+endif
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
new file mode 100644
index 0000000..a23ab1e
--- /dev/null
+++ b/drivers/usb/common/usb-otg.c
@@ -0,0 +1,877 @@
+/**
+ * drivers/usb/common/usb-otg.c - USB OTG core
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
+ * Author: 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 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/usb/of.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/gadget.h>
+#include <linux/workqueue.h>
+
+/* OTG device list */
+LIST_HEAD(otg_list);
+static DEFINE_MUTEX(otg_list_mutex);
+
+static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
+{
+	if (!hcd->primary_hcd)
+		return 1;
+	return hcd == hcd->primary_hcd;
+}
+
+/**
+ * usb_otg_get_data() - get usb_otg data structure
+ * @otg_dev:	OTG controller device
+ *
+ * Check if the OTG device is in our OTG list and return
+ * usb_otg data, else NULL.
+ *
+ * otg_list_mutex must be held.
+ *
+ * Return: usb_otg data on success, NULL otherwise.
+ */
+static struct usb_otg *usb_otg_get_data(struct device *otg_dev)
+{
+	struct usb_otg *otg;
+
+	if (!otg_dev)
+		return NULL;
+
+	list_for_each_entry(otg, &otg_list, list) {
+		if (otg->dev == otg_dev)
+			return otg;
+	}
+
+	return NULL;
+}
+
+/**
+ * usb_otg_start_host() - start/stop the host controller
+ * @otg:	usb_otg instance
+ * @on:		true to start, false to stop
+ *
+ * Start/stop the USB host controller. This function is meant
+ * for use by the OTG controller driver.
+ *
+ * Return: 0 on success, error value otherwise.
+ */
+int usb_otg_start_host(struct usb_otg *otg, int on)
+{
+	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
+	int ret;
+
+	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
+	if (!otg->host) {
+		WARN_ONCE(1, "otg: fsm running without host\n");
+		return 0;
+	}
+
+	if (on) {
+		if (otg->flags & OTG_FLAG_HOST_RUNNING)
+			return 0;
+
+		/* start host */
+		ret = hcd_ops->add(otg->primary_hcd.hcd,
+				   otg->primary_hcd.irqnum,
+				   otg->primary_hcd.irqflags);
+		if (ret) {
+			dev_err(otg->dev, "otg: host add failed %d\n", ret);
+			return ret;
+		}
+
+		if (otg->shared_hcd.hcd) {
+			ret = hcd_ops->add(otg->shared_hcd.hcd,
+					   otg->shared_hcd.irqnum,
+					   otg->shared_hcd.irqflags);
+			if (ret) {
+				dev_err(otg->dev, "otg: shared host add failed %d\n",
+					ret);
+				hcd_ops->remove(otg->primary_hcd.hcd);
+				return ret;
+			}
+		}
+		otg->flags |= OTG_FLAG_HOST_RUNNING;
+	} else {
+		if (!(otg->flags & OTG_FLAG_HOST_RUNNING))
+			return 0;
+
+		otg->flags &= ~OTG_FLAG_HOST_RUNNING;
+
+		/* stop host */
+		if (otg->shared_hcd.hcd)
+			hcd_ops->remove(otg->shared_hcd.hcd);
+
+		hcd_ops->remove(otg->primary_hcd.hcd);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_start_host);
+
+/**
+ * usb_otg_start_gadget() - start/stop the gadget controller
+ * @otg:	usb_otg instance
+ * @on:		true to start, false to stop
+ *
+ * Start/stop the USB gadget controller. This function is meant
+ * for use by the OTG controller driver.
+ *
+ * Return: 0 on success, error value otherwise.
+ */
+int usb_otg_start_gadget(struct usb_otg *otg, int on)
+{
+	struct usb_gadget *gadget = otg->gadget;
+	int ret;
+
+	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
+	if (!gadget) {
+		WARN_ONCE(1, "otg: fsm running without gadget\n");
+		return 0;
+	}
+
+	if (on) {
+		if (otg->flags & OTG_FLAG_GADGET_RUNNING)
+			return 0;
+
+		ret = otg->gadget_ops->start(otg->gadget);
+		if (ret) {
+			dev_err(otg->dev, "otg: gadget start failed: %d\n",
+				ret);
+			return ret;
+		}
+
+		otg->flags |= OTG_FLAG_GADGET_RUNNING;
+	} else {
+		if (!(otg->flags & OTG_FLAG_GADGET_RUNNING))
+			return 0;
+
+		ret = otg->gadget_ops->stop(otg->gadget);
+		if (ret) {
+			dev_err(otg->dev, "otg: gadget stop failed: %d\n",
+				ret);
+			return ret;
+		}
+		otg->flags &= ~OTG_FLAG_GADGET_RUNNING;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_start_gadget);
+
+/**
+ * drd_set_protocol() -  Set USB protocol if possible
+ * @fsm:	DRD FSM instance
+ * @protocol:	USB protocol to set the state machine to
+ *
+ * Sets the OTG FSM protocol to @protocol if it changed.
+ * fsm->lock must be held.
+ *
+ * Return: 0 on success, error value otherwise.
+ */
+static int drd_set_protocol(struct otg_fsm *fsm, int protocol)
+{
+	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
+	int ret = 0;
+
+	if (fsm->protocol != protocol) {
+		dev_dbg(otg->dev, "otg: changing role fsm->protocol= %d; new protocol= %d\n",
+			fsm->protocol, protocol);
+		/* stop old protocol */
+		if (fsm->protocol == PROTO_HOST) {
+			ret = otg_start_host(otg, 0);
+		} else if (fsm->protocol == PROTO_GADGET) {
+			otg->gadget_ops->connect_control(otg->gadget, false);
+			ret = otg_start_gadget(otg, 0);
+		}
+
+		if (ret)
+			return ret;
+
+		/* start new protocol */
+		if (protocol == PROTO_HOST) {
+			ret = otg_start_host(otg, 1);
+		} else if (protocol == PROTO_GADGET) {
+			ret = otg_start_gadget(otg, 1);
+			otg->gadget_ops->connect_control(otg->gadget, true);
+		}
+
+		if (ret)
+			return ret;
+
+		fsm->protocol = protocol;
+		return 0;
+	}
+
+	return 0;
+}
+
+/**
+ * drd_set_state() - Set the DRD state machine state.
+ * @fsm:	DRD FSM instance
+ * @new_state:	the new state the DRD FSM must be set to
+ *
+ * Sets the state of the DRD state machine.
+ * fsm->lock must be held.
+ */
+static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
+{
+	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
+
+	if (otg->state == new_state)
+		return;
+
+	fsm->state_changed = 1;
+	dev_dbg(otg->dev, "otg: set state: %s\n",
+		usb_otg_state_string(new_state));
+	switch (new_state) {
+	case OTG_STATE_B_IDLE:
+		drd_set_protocol(fsm, PROTO_UNDEF);
+		otg_drv_vbus(otg, 0);
+		break;
+	case OTG_STATE_B_PERIPHERAL:
+		drd_set_protocol(fsm, PROTO_GADGET);
+		otg_drv_vbus(otg, 0);
+		break;
+	case OTG_STATE_A_HOST:
+		drd_set_protocol(fsm, PROTO_HOST);
+		otg_drv_vbus(otg, 1);
+		break;
+	default:
+		dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
+			 __func__, usb_otg_state_string(new_state));
+		break;
+	}
+
+	otg->state = new_state;
+}
+
+/**
+ * drd_statemachine() - DRD state change judgement
+ * @otg:	usb_otg instance
+ *
+ * Checks the state machine inputs and state and makes a state change
+ * if required.
+ *
+ * For DRD we're only interested in some of the OTG states
+ * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
+ *	OTG_STATE_B_PERIPHERAL: peripheral active
+ *	OTG_STATE_A_HOST: host active
+ * we're only interested in the following inputs
+ *	fsm->id, fsm->b_sess_vld
+ *
+ * Return: 0 if state wasn't changed, 1 if state changed.
+ */
+int drd_statemachine(struct usb_otg *otg)
+{
+	struct otg_fsm *fsm = &otg->fsm;
+	enum usb_otg_state state;
+	int ret;
+
+	mutex_lock(&fsm->lock);
+
+	fsm->state_changed = 0;
+	state = otg->state;
+
+	switch (state) {
+	case OTG_STATE_UNDEFINED:
+		if (!fsm->id)
+			drd_set_state(fsm, OTG_STATE_A_HOST);
+		else if (fsm->id && fsm->b_sess_vld)
+			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
+		else
+			drd_set_state(fsm, OTG_STATE_B_IDLE);
+		break;
+	case OTG_STATE_B_IDLE:
+		if (!fsm->id)
+			drd_set_state(fsm, OTG_STATE_A_HOST);
+		else if (fsm->b_sess_vld)
+			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
+		break;
+	case OTG_STATE_B_PERIPHERAL:
+		if (!fsm->id)
+			drd_set_state(fsm, OTG_STATE_A_HOST);
+		else if (!fsm->b_sess_vld)
+			drd_set_state(fsm, OTG_STATE_B_IDLE);
+		break;
+	case OTG_STATE_A_HOST:
+		if (fsm->id && fsm->b_sess_vld)
+			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
+		else if (fsm->id && !fsm->b_sess_vld)
+			drd_set_state(fsm, OTG_STATE_B_IDLE);
+		break;
+
+	default:
+		dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
+			__func__, usb_otg_state_string(state));
+		break;
+	}
+
+	ret = fsm->state_changed;
+	mutex_unlock(&fsm->lock);
+	dev_dbg(otg->dev, "otg: quit statemachine, changed %d\n",
+		fsm->state_changed);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drd_statemachine);
+
+/**
+ * usb_drd_work() - Dual-role state machine work function
+ * @work: work_struct context
+ *
+ * Runs the DRD state machine. Scheduled whenever there is a change
+ * in FSM inputs.
+ */
+static void usb_drd_work(struct work_struct *work)
+{
+	struct usb_otg *otg = container_of(work, struct usb_otg, work);
+
+	pm_runtime_get_sync(otg->dev);
+	while (drd_statemachine(otg))
+		;
+	pm_runtime_put_sync(otg->dev);
+}
+
+/**
+ * usb_otg_register() - Register the OTG/dual-role device to OTG core
+ * @dev: OTG/dual-role controller device.
+ * @config: OTG configuration.
+ *
+ * Registers the OTG/dual-role controller device with the USB OTG core.
+ *
+ * Return: struct usb_otg * if success, ERR_PTR() otherwise.
+ */
+struct usb_otg *usb_otg_register(struct device *dev,
+				 struct usb_otg_config *config)
+{
+	struct usb_otg *otg;
+	int ret = 0;
+
+	if (!dev || !config || !config->fsm_ops)
+		return ERR_PTR(-EINVAL);
+
+	/* already in list? */
+	mutex_lock(&otg_list_mutex);
+	if (usb_otg_get_data(dev)) {
+		dev_err(dev, "otg: %s: device already in otg list\n",
+			__func__);
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	/* allocate and add to list */
+	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
+	if (!otg) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	otg->dev = dev;
+	/* otg->caps is controller caps + DT overrides */
+	otg->caps = *config->otg_caps;
+	ret = of_usb_update_otg_caps(dev->of_node, &otg->caps);
+	if (ret)
+		goto err_wq;
+
+	if ((otg->caps.hnp_support || otg->caps.srp_support ||
+	     otg->caps.adp_support) && !config->otg_work) {
+		dev_err(dev,
+			"otg: otg_work must be provided for OTG support\n");
+		ret = -EINVAL;
+		goto err_wq;
+	}
+
+	if (config->otg_work)	/* custom otg_work ? */
+		INIT_WORK(&otg->work, config->otg_work);
+	else
+		INIT_WORK(&otg->work, usb_drd_work);
+
+	otg->wq = create_freezable_workqueue("usb_otg");
+	if (!otg->wq) {
+		dev_err(dev, "otg: %s: can't create workqueue\n",
+			__func__);
+		ret = -ENOMEM;
+		goto err_wq;
+	}
+
+	/* set otg ops */
+	otg->fsm.ops = config->fsm_ops;
+
+	mutex_init(&otg->fsm.lock);
+
+	list_add_tail(&otg->list, &otg_list);
+	mutex_unlock(&otg_list_mutex);
+
+	return otg;
+
+err_wq:
+	kfree(otg);
+unlock:
+	mutex_unlock(&otg_list_mutex);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(usb_otg_register);
+
+/**
+ * usb_otg_unregister() - Unregister the OTG/dual-role device from USB OTG core
+ * @dev: OTG controller device.
+ *
+ * Unregisters the OTG/dual-role controller device from USB OTG core.
+ * Prevents unregistering till both the associated Host and Gadget controllers
+ * have unregistered from the OTG core.
+ *
+ * Return: 0 on success, error value otherwise.
+ */
+int usb_otg_unregister(struct device *dev)
+{
+	struct usb_otg *otg;
+
+	mutex_lock(&otg_list_mutex);
+	otg = usb_otg_get_data(dev);
+	if (!otg) {
+		dev_err(dev, "otg: %s: device not in otg list\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	/* prevent unregister till both host & gadget have unregistered */
+	if (otg->host || otg->gadget) {
+		dev_err(dev, "otg: %s: host/gadget still registered\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EBUSY;
+	}
+
+	/* OTG FSM is halted when host/gadget unregistered */
+	destroy_workqueue(otg->wq);
+
+	/* remove from otg list */
+	list_del(&otg->list);
+	kfree(otg);
+	mutex_unlock(&otg_list_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_unregister);
+
+/**
+ * usb_otg_start_fsm() - Start the OTG FSM
+ * @otg:	usb_otg instance
+ *
+ * Start the OTG FSM if we can. The HCD, UDC and gadget function driver
+ * must be ready for the OTG FSM to start.
+ *
+ * fsm->lock must be held.
+ */
+static void usb_otg_start_fsm(struct usb_otg *otg)
+{
+	struct otg_fsm *fsm = &otg->fsm;
+
+	if (fsm->running)
+		goto kick_fsm;
+
+	if (!otg->host) {
+		dev_info(otg->dev, "otg: can't start till host registers\n");
+		return;
+	}
+
+	if (!otg->gadget) {
+		dev_info(otg->dev,
+			 "otg: can't start till gadget UDC registers\n");
+		return;
+	}
+
+	if (!otg->gadget_ready) {
+		dev_info(otg->dev,
+			 "otg: can't start till gadget function registers\n");
+		return;
+	}
+
+	fsm->running = true;
+kick_fsm:
+	queue_work(otg->wq, &otg->work);
+}
+
+/**
+ * usb_otg_stop_fsm() - Stop the OTG FSM
+ * @otg:	usb_otg instance
+ *
+ * Stops the HCD, UDC and the OTG FSM.
+ *
+ * fsm->lock must be held.
+ */
+static void usb_otg_stop_fsm(struct usb_otg *otg)
+{
+	struct otg_fsm *fsm = &otg->fsm;
+
+	if (!fsm->running)
+		return;
+
+	/* no more new events queued */
+	fsm->running = false;
+
+	flush_workqueue(otg->wq);
+	otg->state = OTG_STATE_UNDEFINED;
+
+	/* stop host/gadget immediately */
+	if (fsm->protocol == PROTO_HOST) {
+		otg_start_host(otg, 0);
+	} else if (fsm->protocol == PROTO_GADGET) {
+		otg->gadget_ops->connect_control(otg->gadget, false);
+		otg_start_gadget(otg, 0);
+	}
+	fsm->protocol = PROTO_UNDEF;
+}
+
+/**
+ * usb_otg_sync_inputs() - Sync OTG inputs with the OTG state machine
+ * @otg:	usb_otg instance
+ *
+ * Used by the OTG driver to update the inputs to the OTG
+ * state machine.
+ *
+ * Can be called in IRQ context.
+ */
+void usb_otg_sync_inputs(struct usb_otg *otg)
+{
+	/* Don't kick FSM till it has started */
+	if (!otg->fsm.running)
+		return;
+
+	/* Kick FSM */
+	queue_work(otg->wq, &otg->work);
+}
+EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
+
+/**
+ * usb_otg_register_hcd() - Register the host controller to OTG core
+ * @hcd:	host controller
+ * @irqnum:	interrupt number
+ * @irqflags:	interrupt flags
+ * @ops:	HCD ops to interface with the HCD
+ *
+ * This is used by the USB Host stack to register the host controller
+ * to the OTG core. Host controller must not be started by the
+ * caller as it is left up to the OTG state machine to do so.
+ * hcd->otg_dev must contain the related otg controller device.
+ *
+ * Return: 0 on success, error value otherwise.
+ */
+int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
+			 unsigned long irqflags, struct otg_hcd_ops *ops)
+{
+	struct usb_otg *otg;
+	struct device *hcd_dev = hcd->self.controller;
+	struct device *otg_dev = hcd->otg_dev;
+
+	if (!otg_dev)
+		return -EINVAL;
+
+	/* we're otg but otg controller might not yet be registered */
+	mutex_lock(&otg_list_mutex);
+	otg = usb_otg_get_data(otg_dev);
+	mutex_unlock(&otg_list_mutex);
+	if (!otg) {
+		dev_dbg(hcd_dev,
+			"otg: controller not yet registered. deferring.\n");
+		return -EPROBE_DEFER;
+	}
+
+	/* HCD will be started by OTG fsm when needed */
+	mutex_lock(&otg->fsm.lock);
+	if (otg->primary_hcd.hcd) {
+		/* probably a shared HCD ? */
+		if (usb_otg_hcd_is_primary_hcd(hcd)) {
+			dev_err(otg_dev, "otg: primary host already registered\n");
+			goto err;
+		}
+
+		if (hcd->shared_hcd == otg->primary_hcd.hcd) {
+			if (otg->shared_hcd.hcd) {
+				dev_err(otg_dev, "otg: shared host already registered\n");
+				goto err;
+			}
+
+			otg->shared_hcd.hcd = hcd;
+			otg->shared_hcd.irqnum = irqnum;
+			otg->shared_hcd.irqflags = irqflags;
+			otg->shared_hcd.ops = ops;
+			dev_info(otg_dev, "otg: shared host %s registered\n",
+				 dev_name(hcd->self.controller));
+		} else {
+			dev_err(otg_dev, "otg: invalid shared host %s\n",
+				dev_name(hcd->self.controller));
+			goto err;
+		}
+	} else {
+		if (!usb_otg_hcd_is_primary_hcd(hcd)) {
+			dev_err(otg_dev, "otg: primary host must be registered first\n");
+			goto err;
+		}
+
+		otg->primary_hcd.hcd = hcd;
+		otg->primary_hcd.irqnum = irqnum;
+		otg->primary_hcd.irqflags = irqflags;
+		otg->primary_hcd.ops = ops;
+		otg->hcd_ops = ops;
+		dev_info(otg_dev, "otg: primary host %s registered\n",
+			 dev_name(hcd->self.controller));
+	}
+
+	/*
+	 * we're ready only if we have shared HCD
+	 * or we don't need shared HCD.
+	 */
+	if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
+		otg->host = hcd_to_bus(hcd);
+		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
+
+		/* start FSM */
+		usb_otg_start_fsm(otg);
+	} else {
+		dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
+	}
+
+	mutex_unlock(&otg->fsm.lock);
+
+	return 0;
+
+err:
+	mutex_unlock(&otg->fsm.lock);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
+
+/**
+ * usb_otg_unregister_hcd() - Unregister the host controller from OTG core
+ * @hcd:	host controller device
+ *
+ * This is used by the USB Host stack to unregister the host controller
+ * from the OTG core. Ensures that host controller is not running
+ * on successful return.
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+int usb_otg_unregister_hcd(struct usb_hcd *hcd)
+{
+	struct usb_otg *otg;
+	struct device *hcd_dev = hcd_to_bus(hcd)->controller;
+	struct device *otg_dev = hcd->otg_dev;
+
+	if (!otg_dev)
+		return -EINVAL;	/* we're definitely not OTG */
+
+	mutex_lock(&otg_list_mutex);
+	otg = usb_otg_get_data(otg_dev);
+	mutex_unlock(&otg_list_mutex);
+	if (!otg) {
+		dev_err(hcd_dev, "otg: host %s wasn't registered with otg\n",
+			dev_name(hcd_dev));
+		return -EINVAL;
+	}
+
+	mutex_lock(&otg->fsm.lock);
+	if (hcd == otg->primary_hcd.hcd) {
+		otg->primary_hcd.hcd = NULL;
+		dev_info(otg_dev, "otg: primary host %s unregistered\n",
+			 dev_name(hcd_dev));
+	} else if (hcd == otg->shared_hcd.hcd) {
+		otg->shared_hcd.hcd = NULL;
+		dev_info(otg_dev, "otg: shared host %s unregistered\n",
+			 dev_name(hcd_dev));
+	} else {
+		mutex_unlock(&otg->fsm.lock);
+		dev_err(otg_dev, "otg: host %s wasn't registered with otg\n",
+			dev_name(hcd_dev));
+		return -EINVAL;
+	}
+
+	/* stop FSM & Host */
+	usb_otg_stop_fsm(otg);
+	otg->host = NULL;
+
+	mutex_unlock(&otg->fsm.lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_unregister_hcd);
+
+/**
+ * usb_otg_register_gadget() - Register the gadget controller to OTG core
+ * @gadget:	gadget controller instance
+ * @ops:	gadget interface ops
+ *
+ * This is used by the USB gadget stack to register the gadget controller
+ * to the OTG core. Gadget controller must not be started by the
+ * caller as it is left up to the OTG state machine to do so.
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+int usb_otg_register_gadget(struct usb_gadget *gadget,
+			    struct otg_gadget_ops *ops)
+{
+	struct usb_otg *otg;
+	struct device *gadget_dev = &gadget->dev;
+	struct device *otg_dev = gadget->otg_dev;
+
+	if (!otg_dev)
+		return -EINVAL;	/* we're definitely not OTG */
+
+	/* we're otg but otg controller might not yet be registered */
+	mutex_lock(&otg_list_mutex);
+	otg = usb_otg_get_data(otg_dev);
+	mutex_unlock(&otg_list_mutex);
+	if (!otg) {
+		dev_dbg(gadget_dev,
+			"otg: controller not yet registered, deferring.\n");
+		return -EPROBE_DEFER;
+	}
+
+	mutex_lock(&otg->fsm.lock);
+	if (otg->gadget) {
+		dev_err(otg_dev, "otg: gadget already registered with otg\n");
+		mutex_unlock(&otg->fsm.lock);
+		return -EINVAL;
+	}
+
+	otg->gadget = gadget;
+	otg->gadget_ops = ops;
+	dev_info(otg_dev, "otg: gadget %s registered\n",
+		 dev_name(&gadget->dev));
+
+	/* FSM will be started in usb_otg_gadget_ready() */
+	mutex_unlock(&otg->fsm.lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_register_gadget);
+
+/**
+ * usb_otg_unregister_gadget() - Unregister the gadget controller from OTG core
+ * @gadget:	gadget controller
+ *
+ * This is used by the USB gadget stack to unregister the gadget controller
+ * from the OTG core. Ensures that gadget controller is not running
+ * on successful return.
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+int usb_otg_unregister_gadget(struct usb_gadget *gadget)
+{
+	struct usb_otg *otg;
+	struct device *gadget_dev = &gadget->dev;
+	struct device *otg_dev = gadget->otg_dev;
+
+	if (!otg_dev)
+		return -EINVAL;
+
+	mutex_lock(&otg_list_mutex);
+	otg = usb_otg_get_data(otg_dev);
+	mutex_unlock(&otg_list_mutex);
+	if (!otg) {
+		dev_err(gadget_dev,
+			"otg: gadget %s wasn't registered with otg\n",
+			dev_name(&gadget->dev));
+		return -EINVAL;
+	}
+
+	mutex_lock(&otg->fsm.lock);
+	if (otg->gadget != gadget) {
+		mutex_unlock(&otg->fsm.lock);
+		dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
+			dev_name(&gadget->dev));
+		return -EINVAL;
+	}
+
+	/* FSM must be stopped in usb_otg_gadget_ready() */
+	if (otg->gadget_ready) {
+		dev_err(otg_dev,
+			"otg: gadget %s unregistered before being unready, forcing stop\n",
+			dev_name(&gadget->dev));
+		usb_otg_stop_fsm(otg);
+	}
+
+	otg->gadget = NULL;
+	mutex_unlock(&otg->fsm.lock);
+
+	dev_info(otg_dev, "otg: gadget %s unregistered\n",
+		 dev_name(&gadget->dev));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_unregister_gadget);
+
+/**
+ * usb_otg_gadget_ready() - Notify gadget function driver ready status
+ * @gadget:	gadget controller
+ * @ready:	0: function driver ready, 1: function driver not ready
+ *
+ * Notify the OTG core about status of the gadget function driver.
+ * As OTG core is responsible to start/stop the gadget controller, it
+ * must be aware when the gadget function driver is available or not.
+ * This function is used by the Gadget core to inform the OTG core
+ * about the gadget function driver readyness.
+ *
+ * Return: 0 on sucess, error value otherwise.
+ */
+int usb_otg_gadget_ready(struct usb_gadget *gadget, bool ready)
+{
+	struct usb_otg *otg;
+	struct device *gadget_dev = &gadget->dev;
+	struct device *otg_dev = gadget->otg_dev;
+
+	if (!otg_dev)
+		return -EINVAL;
+
+	mutex_lock(&otg_list_mutex);
+	otg = usb_otg_get_data(otg_dev);
+	mutex_unlock(&otg_list_mutex);
+	if (!otg) {
+		dev_err(gadget_dev,
+			"otg: gadget %s wasn't registered with otg\n",
+			dev_name(&gadget->dev));
+		return -EINVAL;
+	}
+
+	mutex_lock(&otg->fsm.lock);
+	if (otg->gadget != gadget) {
+		mutex_unlock(&otg->fsm.lock);
+		dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
+			dev_name(&gadget->dev));
+		return -EINVAL;
+	}
+
+	/* Start/stop FSM & gadget */
+	otg->gadget_ready = ready;
+	if (ready)
+		usb_otg_start_fsm(otg);
+	else
+		usb_otg_stop_fsm(otg);
+
+	dev_dbg(otg_dev, "otg: gadget %s %sready\n", dev_name(&gadget->dev),
+		ready ? "" : "not ");
+
+	mutex_unlock(&otg->fsm.lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_gadget_ready);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index ae228d0..37f8c54 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -41,20 +41,6 @@ config USB_DYNAMIC_MINORS
 
 	  If you are unsure about this, say N here.
 
-config USB_OTG
-	bool "OTG support"
-	depends on PM
-	default n
-	help
-	  The most notable feature of USB OTG is support for a
-	  "Dual-Role" device, which can act as either a device
-	  or a host. The initial role is decided by the type of
-	  plug inserted and can be changed later when two dual
-	  role devices talk to each other.
-
-	  Select this only if your board has Mini-AB/Micro-AB
-	  connector.
-
 config USB_OTG_WHITELIST
 	bool "Rely on OTG and EH Targeted Peripherals List"
 	depends on USB
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 3c3f31c..5fc9095 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -16,6 +16,7 @@
 menuconfig USB_GADGET
 	tristate "USB Gadget Support"
 	select USB_COMMON
+	select USB_OTG_CORE
 	select NLS
 	help
 	   USB is a master/slave protocol, organized with one master
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index f4fc0aa..1d74fb8 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -328,6 +328,7 @@ struct usb_gadget_ops {
  * @in_epnum: last used in ep number
  * @mA: last set mA value
  * @otg_caps: OTG capabilities of this gadget.
+ * @otg_dev: OTG controller device, if needs to be used with OTG core.
  * @sg_supported: true if we can handle scatter-gather
  * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
  *	gadget driver must provide a USB OTG descriptor.
@@ -385,6 +386,7 @@ struct usb_gadget {
 	unsigned			in_epnum;
 	unsigned			mA;
 	struct usb_otg_caps		*otg_caps;
+	struct device			*otg_dev;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 7729c1f..36bd54f 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -185,6 +185,7 @@ struct usb_hcd {
 	struct mutex		*bandwidth_mutex;
 	struct usb_hcd		*shared_hcd;
 	struct usb_hcd		*primary_hcd;
+	struct device		*otg_dev;	/* OTG controller device */
 
 
 #define HCD_BUFFER_POOLS	4
diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
index 26e6531..943714a 100644
--- a/include/linux/usb/otg-fsm.h
+++ b/include/linux/usb/otg-fsm.h
@@ -60,6 +60,11 @@ enum otg_fsm_timer {
 /**
  * struct otg_fsm - OTG state machine according to the OTG spec
  *
+ * DRD mode hardware Inputs
+ *
+ * @id:		TRUE for B-device, FALSE for A-device.
+ * @b_sess_vld:	VBUS voltage in regulation.
+ *
  * OTG hardware Inputs
  *
  *	Common inputs for A and B device
@@ -132,6 +137,7 @@ enum otg_fsm_timer {
  * a_clr_err:	Asserted (by application ?) to clear a_vbus_err due to an
  *		overcurrent condition and causes the A-device to transition
  *		to a_wait_vfall
+ * running:	state machine running/stopped indicator
  */
 struct otg_fsm {
 	/* Input */
@@ -187,6 +193,7 @@ struct otg_fsm {
 	int b_ase0_brst_tmout;
 	int a_bidl_adis_tmout;
 
+	bool running;
 	struct otg_fsm_ops *ops;
 
 	/* Current usb protocol used: 0:undefine; 1:host; 2:client */
diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index 85b8fb5..9d72951 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -10,10 +10,67 @@
 #define __LINUX_USB_OTG_H
 
 #include <linux/phy/phy.h>
-#include <linux/usb/phy.h>
-#include <linux/usb/otg-fsm.h>
+#include <linux/device.h>
+#include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/otg-fsm.h>
+#include <linux/usb/phy.h>
+
+/**
+ * struct otg_hcd - host controller state and interface
+ *
+ * @hcd: host controller
+ * @irqnum: IRQ number
+ * @irqflags: IRQ flags
+ * @ops: OTG to host controller interface
+ * @otg_dev: OTG controller device
+ */
+struct otg_hcd {
+	struct usb_hcd *hcd;
+	unsigned int irqnum;
+	unsigned long irqflags;
+	struct otg_hcd_ops *ops;
+	struct device *otg_dev;
+};
+
+/**
+ * struct usb_otg_caps - describes the otg capabilities of the device
+ * @otg_rev: The OTG revision number the device is compliant with, it's
+ *		in binary-coded decimal (i.e. 2.0 is 0200H).
+ * @hnp_support: Indicates if the device supports HNP.
+ * @srp_support: Indicates if the device supports SRP.
+ * @adp_support: Indicates if the device supports ADP.
+ */
+struct usb_otg_caps {
+	u16 otg_rev;
+	bool hnp_support;
+	bool srp_support;
+	bool adp_support;
+};
 
+/**
+ * struct usb_otg - usb otg controller state
+ *
+ * @default_a: Indicates we are an A device. i.e. Host.
+ * @phy: USB PHY interface
+ * @usb_phy: old usb_phy interface
+ * @host: host controller bus
+ * @gadget: gadget device
+ * @state: current OTG state
+ * @dev: OTG controller device
+ * @caps: OTG capabilities revision, hnp, srp, etc
+ * @fsm: OTG finite state machine
+ * @hcd_ops: host controller interface
+ * ------- internal use only -------
+ * @primary_hcd: primary host state and interface
+ * @shared_hcd: shared host state and interface
+ * @gadget_ops: gadget controller interface
+ * @list: list of OTG controllers
+ * @work: OTG state machine work
+ * @wq: OTG state machine work queue
+ * @flags: to track if host/gadget is running
+ */
 struct usb_otg {
 	u8			default_a;
 
@@ -24,9 +81,25 @@ struct usb_otg {
 	struct usb_gadget	*gadget;
 
 	enum usb_otg_state	state;
+	struct device *dev;
+	struct usb_otg_caps	caps;
 	struct otg_fsm fsm;
 	struct otg_hcd_ops	*hcd_ops;
 
+	/* internal use only */
+	struct otg_hcd primary_hcd;
+	struct otg_hcd shared_hcd;
+	struct otg_gadget_ops *gadget_ops;
+	bool gadget_ready;
+	struct list_head list;
+	struct work_struct work;
+	struct workqueue_struct *wq;
+	u32 flags;
+#define OTG_FLAG_GADGET_RUNNING (1 << 0)
+#define OTG_FLAG_HOST_RUNNING (1 << 1)
+	/* use otg->fsm.lock for serializing access */
+
+/*------------- deprecated interface -----------------------------*/
 	/* bind/unbind the host controller */
 	int	(*set_host)(struct usb_otg *otg, struct usb_bus *host);
 
@@ -42,26 +115,95 @@ struct usb_otg {
 
 	/* start or continue HNP role switch */
 	int	(*start_hnp)(struct usb_otg *otg);
-
+/*---------------------------------------------------------------*/
 };
 
 /**
- * struct usb_otg_caps - describes the otg capabilities of the device
- * @otg_rev: The OTG revision number the device is compliant with, it's
- *		in binary-coded decimal (i.e. 2.0 is 0200H).
- * @hnp_support: Indicates if the device supports HNP.
- * @srp_support: Indicates if the device supports SRP.
- * @adp_support: Indicates if the device supports ADP.
+ * struct usb_otg_config - OTG controller configuration
+ * @caps: OTG capabilities of the controller
+ * @ops: OTG FSM operations
+ * @otg_work: optional custom OTG state machine work function
  */
-struct usb_otg_caps {
-	u16 otg_rev;
-	bool hnp_support;
-	bool srp_support;
-	bool adp_support;
+struct usb_otg_config {
+	struct usb_otg_caps *otg_caps;
+	struct otg_fsm_ops *fsm_ops;
+	void (*otg_work)(struct work_struct *work);
 };
 
 extern const char *usb_otg_state_string(enum usb_otg_state state);
 
+#if IS_ENABLED(CONFIG_USB_OTG)
+struct usb_otg *usb_otg_register(struct device *dev,
+				 struct usb_otg_config *config);
+int usb_otg_unregister(struct device *dev);
+int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
+			 unsigned long irqflags, struct otg_hcd_ops *ops);
+int usb_otg_unregister_hcd(struct usb_hcd *hcd);
+int usb_otg_register_gadget(struct usb_gadget *gadget,
+			    struct otg_gadget_ops *ops);
+int usb_otg_unregister_gadget(struct usb_gadget *gadget);
+void usb_otg_sync_inputs(struct usb_otg *otg);
+int usb_otg_start_host(struct usb_otg *otg, int on);
+int usb_otg_start_gadget(struct usb_otg *otg, int on);
+int usb_otg_gadget_ready(struct usb_gadget *gadget, bool ready);
+
+#else /* CONFIG_USB_OTG */
+
+static inline struct usb_otg *usb_otg_register(struct device *dev,
+					       struct usb_otg_config *config)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline int usb_otg_unregister(struct device *dev)
+{
+	return -ENOTSUPP;
+}
+
+static inline int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
+				       unsigned long irqflags,
+				       struct otg_hcd_ops *ops)
+{
+	return -ENOTSUPP;
+}
+
+static inline int usb_otg_unregister_hcd(struct usb_hcd *hcd)
+{
+	return -ENOTSUPP;
+}
+
+static inline int usb_otg_register_gadget(struct usb_gadget *gadget,
+					  struct otg_gadget_ops *ops)
+{
+	return -ENOTSUPP;
+}
+
+static inline int usb_otg_unregister_gadget(struct usb_gadget *gadget)
+{
+	return -ENOTSUPP;
+}
+
+static inline void usb_otg_sync_inputs(struct usb_otg *otg)
+{
+}
+
+static inline int usb_otg_start_host(struct usb_otg *otg, int on)
+{
+	return -ENOTSUPP;
+}
+
+static inline int usb_otg_start_gadget(struct usb_otg *otg, int on)
+{
+	return -ENOTSUPP;
+}
+
+static inline int usb_otg_gadget_ready(struct usb_gadget *gadget, bool ready)
+{
+	return -ENOTSUPP;
+}
+#endif /* CONFIG_USB_OTG */
+
+/*------------- deprecated interface -----------------------------*/
 /* Context: can sleep */
 static inline int
 otg_start_hnp(struct usb_otg *otg)
@@ -113,6 +255,8 @@ otg_start_srp(struct usb_otg *otg)
 	return -ENOTSUPP;
 }
 
+/*---------------------------------------------------------------*/
+
 /* for OTG controller drivers (and maybe other stuff) */
 extern int usb_bus_start_enum(struct usb_bus *bus, unsigned port_num);
 
@@ -237,4 +381,6 @@ static inline int otg_start_gadget(struct usb_otg *otg, int on)
 	return otg->fsm.ops->start_gadget(otg, on);
 }
 
+int drd_statemachine(struct usb_otg *otg);
+
 #endif /* __LINUX_USB_OTG_H */
-- 
2.7.4

[toc] | [next] | [standalone]

#1426291

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

Hi,

Roger Quadros <rogerq@ti.com> writes:
> It provides APIs for the following tasks
>
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine

I think I have already mentioned this, but after over 10 years of OTG,
nobody seems to care about it, why are we still touching at all I don't
know. For common non-OTG role-swapping we really don't need any of this
and, quite frankly, I fail to see enough users for this.

Apparently there's only chipidea which, AFAICT, already had working
dual-role before this OTG State Machine was added to the kernel.

> Provide a dual-role device (DRD) state machine.

there's not such thing as DRD state machine. You don't need to go
through all these states, actually.

> DRD mode is a reduced functionality OTG mode. In this mode
> we don't support SRP, HNP and dynamic role-swap.
>
> In DRD operation, the controller mode (Host or Peripheral)
> is decided based on the ID pin status. Once a cable plug (Type-A
> or Type-B) is attached the controller selects the state
> and doesn't change till the cable in unplugged and a different
> cable type is inserted.
>
> As we don't need most of the complex OTG states and OTG timers
> we implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs
> 'id' and 'b_sess_vld'.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> v11:
> - remove usb_otg_kick_fsm().
> - typo fixes: structa/structure, upto/up to.
> - remove "obj-$(CONFIG_USB_OTG_CORE)     += common/" from Makefile.
>
>  drivers/usb/Kconfig          |  18 +
>  drivers/usb/common/Makefile  |   6 +-
>  drivers/usb/common/usb-otg.c | 877 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/core/Kconfig     |  14 -
>  drivers/usb/gadget/Kconfig   |   1 +
>  include/linux/usb/gadget.h   |   2 +
>  include/linux/usb/hcd.h      |   1 +
>  include/linux/usb/otg-fsm.h  |   7 +
>  include/linux/usb/otg.h      | 174 ++++++++-
>  9 files changed, 1070 insertions(+), 30 deletions(-)
>  create mode 100644 drivers/usb/common/usb-otg.c
>
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8689dcb..ed596ec 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -32,6 +32,23 @@ if USB_SUPPORT
>  config USB_COMMON
>  	tristate
>  
> +config USB_OTG_CORE
> +	tristate

why tristate if you can never set it to 'M'?

> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> new file mode 100644
> index 0000000..a23ab1e
> --- /dev/null
> +++ b/drivers/usb/common/usb-otg.c
> @@ -0,0 +1,877 @@
> +/**
> + * drivers/usb/common/usb-otg.c - USB OTG core
> + *
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
> + * Author: 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 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/usb/of.h>
> +#include <linux/usb/otg.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/workqueue.h>
> +
> +/* OTG device list */
> +LIST_HEAD(otg_list);

not static? Who needs to touch this private list?

> +static DEFINE_MUTEX(otg_list_mutex);
> +
> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
> +{
> +	if (!hcd->primary_hcd)
> +		return 1;

these seems inverted. If hcd->primary is NULL (meaning, there's no
->primary_hcd), then we tell caller that this _is_ a primary hcd? Care
to explain?

> +	return hcd == hcd->primary_hcd;
> +}
> +
> +/**
> + * usb_otg_get_data() - get usb_otg data structure
> + * @otg_dev:	OTG controller device
> + *
> + * Check if the OTG device is in our OTG list and return
> + * usb_otg data, else NULL.
> + *
> + * otg_list_mutex must be held.
> + *
> + * Return: usb_otg data on success, NULL otherwise.
> + */
> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev)
> +{
> +	struct usb_otg *otg;
> +
> +	if (!otg_dev)
> +		return NULL;
> +
> +	list_for_each_entry(otg, &otg_list, list) {
> +		if (otg->dev == otg_dev)
> +			return otg;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * usb_otg_start_host() - start/stop the host controller
> + * @otg:	usb_otg instance
> + * @on:		true to start, false to stop
> + *
> + * Start/stop the USB host controller. This function is meant
> + * for use by the OTG controller driver.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_start_host(struct usb_otg *otg, int on)
> +{
> +	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
> +	int ret;
> +
> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
> +	if (!otg->host) {
> +		WARN_ONCE(1, "otg: fsm running without host\n");

if (WARN_ONCE(!otg->host, "otg: fsm running without host\n"))
	return 0;

but, frankly, if you require a 'host' and a 'gadget' don't start this
layer until you have both.

> +		return 0;
> +	}
> +
> +	if (on) {
> +		if (otg->flags & OTG_FLAG_HOST_RUNNING)
> +			return 0;
> +
> +		/* start host */
> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
> +				   otg->primary_hcd.irqnum,
> +				   otg->primary_hcd.irqflags);

this is usb_add_hcd(), is it not? Why add an indirection?

> +		if (ret) {
> +			dev_err(otg->dev, "otg: host add failed %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (otg->shared_hcd.hcd) {
> +			ret = hcd_ops->add(otg->shared_hcd.hcd,
> +					   otg->shared_hcd.irqnum,
> +					   otg->shared_hcd.irqflags);
> +			if (ret) {
> +				dev_err(otg->dev, "otg: shared host add failed %d\n",
> +					ret);
> +				hcd_ops->remove(otg->primary_hcd.hcd);
> +				return ret;
> +			}
> +		}
> +		otg->flags |= OTG_FLAG_HOST_RUNNING;
> +	} else {
> +		if (!(otg->flags & OTG_FLAG_HOST_RUNNING))
> +			return 0;
> +
> +		otg->flags &= ~OTG_FLAG_HOST_RUNNING;
> +
> +		/* stop host */
> +		if (otg->shared_hcd.hcd)
> +			hcd_ops->remove(otg->shared_hcd.hcd);

usb_del_hcd()?

> +		hcd_ops->remove(otg->primary_hcd.hcd);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_start_host);
> +
> +/**
> + * usb_otg_start_gadget() - start/stop the gadget controller
> + * @otg:	usb_otg instance
> + * @on:		true to start, false to stop
> + *
> + * Start/stop the USB gadget controller. This function is meant
> + * for use by the OTG controller driver.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_start_gadget(struct usb_otg *otg, int on)
> +{
> +	struct usb_gadget *gadget = otg->gadget;
> +	int ret;
> +
> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
> +	if (!gadget) {
> +		WARN_ONCE(1, "otg: fsm running without gadget\n");
> +		return 0;
> +	}

ditto

> +	if (on) {
> +		if (otg->flags & OTG_FLAG_GADGET_RUNNING)
> +			return 0;
> +
> +		ret = otg->gadget_ops->start(otg->gadget);
> +		if (ret) {
> +			dev_err(otg->dev, "otg: gadget start failed: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		otg->flags |= OTG_FLAG_GADGET_RUNNING;
> +	} else {
> +		if (!(otg->flags & OTG_FLAG_GADGET_RUNNING))
> +			return 0;
> +
> +		ret = otg->gadget_ops->stop(otg->gadget);
> +		if (ret) {
> +			dev_err(otg->dev, "otg: gadget stop failed: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		otg->flags &= ~OTG_FLAG_GADGET_RUNNING;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_start_gadget);
> +
> +/**
> + * drd_set_protocol() -  Set USB protocol if possible
> + * @fsm:	DRD FSM instance
> + * @protocol:	USB protocol to set the state machine to
> + *
> + * Sets the OTG FSM protocol to @protocol if it changed.
> + * fsm->lock must be held.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +static int drd_set_protocol(struct otg_fsm *fsm, int protocol)
> +{
> +	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
> +	int ret = 0;
> +
> +	if (fsm->protocol != protocol) {
> +		dev_dbg(otg->dev, "otg: changing role fsm->protocol= %d; new protocol= %d\n",
> +			fsm->protocol, protocol);
> +		/* stop old protocol */
> +		if (fsm->protocol == PROTO_HOST) {
> +			ret = otg_start_host(otg, 0);
> +		} else if (fsm->protocol == PROTO_GADGET) {
> +			otg->gadget_ops->connect_control(otg->gadget, false);
> +			ret = otg_start_gadget(otg, 0);
> +		}
> +
> +		if (ret)
> +			return ret;
> +
> +		/* start new protocol */
> +		if (protocol == PROTO_HOST) {
> +			ret = otg_start_host(otg, 1);
> +		} else if (protocol == PROTO_GADGET) {
> +			ret = otg_start_gadget(otg, 1);
> +			otg->gadget_ops->connect_control(otg->gadget, true);
> +		}
> +
> +		if (ret)
> +			return ret;
> +
> +		fsm->protocol = protocol;
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * drd_set_state() - Set the DRD state machine state.
> + * @fsm:	DRD FSM instance
> + * @new_state:	the new state the DRD FSM must be set to
> + *
> + * Sets the state of the DRD state machine.
> + * fsm->lock must be held.
> + */
> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
> +{
> +	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
> +
> +	if (otg->state == new_state)
> +		return;
> +
> +	fsm->state_changed = 1;
> +	dev_dbg(otg->dev, "otg: set state: %s\n",
> +		usb_otg_state_string(new_state));
> +	switch (new_state) {
> +	case OTG_STATE_B_IDLE:
> +		drd_set_protocol(fsm, PROTO_UNDEF);
> +		otg_drv_vbus(otg, 0);
> +		break;
> +	case OTG_STATE_B_PERIPHERAL:
> +		drd_set_protocol(fsm, PROTO_GADGET);
> +		otg_drv_vbus(otg, 0);
> +		break;
> +	case OTG_STATE_A_HOST:
> +		drd_set_protocol(fsm, PROTO_HOST);
> +		otg_drv_vbus(otg, 1);
> +		break;
> +	default:
> +		dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
> +			 __func__, usb_otg_state_string(new_state));
> +		break;
> +	}
> +
> +	otg->state = new_state;
> +}
> +
> +/**
> + * drd_statemachine() - DRD state change judgement
> + * @otg:	usb_otg instance
> + *
> + * Checks the state machine inputs and state and makes a state change
> + * if required.
> + *
> + * For DRD we're only interested in some of the OTG states
> + * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
> + *	OTG_STATE_B_PERIPHERAL: peripheral active
> + *	OTG_STATE_A_HOST: host active
> + * we're only interested in the following inputs
> + *	fsm->id, fsm->b_sess_vld
> + *
> + * Return: 0 if state wasn't changed, 1 if state changed.
> + */
> +int drd_statemachine(struct usb_otg *otg)
> +{
> +	struct otg_fsm *fsm = &otg->fsm;
> +	enum usb_otg_state state;
> +	int ret;
> +
> +	mutex_lock(&fsm->lock);
> +
> +	fsm->state_changed = 0;
> +	state = otg->state;
> +
> +	switch (state) {
> +	case OTG_STATE_UNDEFINED:
> +		if (!fsm->id)
> +			drd_set_state(fsm, OTG_STATE_A_HOST);
> +		else if (fsm->id && fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
> +		else
> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
> +		break;
> +	case OTG_STATE_B_IDLE:
> +		if (!fsm->id)
> +			drd_set_state(fsm, OTG_STATE_A_HOST);
> +		else if (fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
> +		break;
> +	case OTG_STATE_B_PERIPHERAL:
> +		if (!fsm->id)
> +			drd_set_state(fsm, OTG_STATE_A_HOST);
> +		else if (!fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
> +		break;
> +	case OTG_STATE_A_HOST:
> +		if (fsm->id && fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
> +		else if (fsm->id && !fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
> +		break;
> +
> +	default:
> +		dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
> +			__func__, usb_otg_state_string(state));
> +		break;
> +	}
> +
> +	ret = fsm->state_changed;
> +	mutex_unlock(&fsm->lock);
> +	dev_dbg(otg->dev, "otg: quit statemachine, changed %d\n",
> +		fsm->state_changed);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drd_statemachine);

why is this exported at all? It only runs from the work_struct
below. BTW, that work_struct looks unnecessary.

> +
> +/**
> + * usb_drd_work() - Dual-role state machine work function
> + * @work: work_struct context
> + *
> + * Runs the DRD state machine. Scheduled whenever there is a change
> + * in FSM inputs.
> + */
> +static void usb_drd_work(struct work_struct *work)
> +{
> +	struct usb_otg *otg = container_of(work, struct usb_otg, work);
> +
> +	pm_runtime_get_sync(otg->dev);
> +	while (drd_statemachine(otg))
> +		;
> +	pm_runtime_put_sync(otg->dev);

so, once this gets kicked it'll keep on running until a state doesn't
change.

> +}
> +
> +/**
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() otherwise.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> +				 struct usb_otg_config *config)
> +{
> +	struct usb_otg *otg;
> +	int ret = 0;
> +
> +	if (!dev || !config || !config->fsm_ops)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* already in list? */
> +	mutex_lock(&otg_list_mutex);
> +	if (usb_otg_get_data(dev)) {
> +		dev_err(dev, "otg: %s: device already in otg list\n",
> +			__func__);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	/* allocate and add to list */
> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> +	if (!otg) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	otg->dev = dev;
> +	/* otg->caps is controller caps + DT overrides */
> +	otg->caps = *config->otg_caps;
> +	ret = of_usb_update_otg_caps(dev->of_node, &otg->caps);
> +	if (ret)
> +		goto err_wq;
> +
> +	if ((otg->caps.hnp_support || otg->caps.srp_support ||
> +	     otg->caps.adp_support) && !config->otg_work) {
> +		dev_err(dev,
> +			"otg: otg_work must be provided for OTG support\n");
> +		ret = -EINVAL;
> +		goto err_wq;
> +	}
> +
> +	if (config->otg_work)	/* custom otg_work ? */
> +		INIT_WORK(&otg->work, config->otg_work);
> +	else
> +		INIT_WORK(&otg->work, usb_drd_work);

why do you need to cope with custom work handlers?

> +	otg->wq = create_freezable_workqueue("usb_otg");
> +	if (!otg->wq) {
> +		dev_err(dev, "otg: %s: can't create workqueue\n",
> +			__func__);
> +		ret = -ENOMEM;
> +		goto err_wq;
> +	}
> +
> +	/* set otg ops */
> +	otg->fsm.ops = config->fsm_ops;
> +
> +	mutex_init(&otg->fsm.lock);
> +
> +	list_add_tail(&otg->list, &otg_list);
> +	mutex_unlock(&otg_list_mutex);
> +
> +	return otg;
> +
> +err_wq:
> +	kfree(otg);
> +unlock:
> +	mutex_unlock(&otg_list_mutex);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register);
> +
> +/**
> + * usb_otg_unregister() - Unregister the OTG/dual-role device from USB OTG core
> + * @dev: OTG controller device.
> + *
> + * Unregisters the OTG/dual-role controller device from USB OTG core.
> + * Prevents unregistering till both the associated Host and Gadget controllers
> + * have unregistered from the OTG core.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister(struct device *dev)
> +{
> +	struct usb_otg *otg;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otg = usb_otg_get_data(dev);
> +	if (!otg) {
> +		dev_err(dev, "otg: %s: device not in otg list\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* prevent unregister till both host & gadget have unregistered */
> +	if (otg->host || otg->gadget) {
> +		dev_err(dev, "otg: %s: host/gadget still registered\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EBUSY;
> +	}

I think a better choice would've been to unregister host and gadget in
case they haven't been unregistered yet. That's a common
expectation. Specially since driver core does the same thing when you
unregister a parent device.

> +	/* OTG FSM is halted when host/gadget unregistered */
> +	destroy_workqueue(otg->wq);
> +
> +	/* remove from otg list */
> +	list_del(&otg->list);
> +	kfree(otg);
> +	mutex_unlock(&otg_list_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister);
> +
> +/**
> + * usb_otg_start_fsm() - Start the OTG FSM
> + * @otg:	usb_otg instance
> + *
> + * Start the OTG FSM if we can. The HCD, UDC and gadget function driver
> + * must be ready for the OTG FSM to start.
> + *
> + * fsm->lock must be held.
> + */
> +static void usb_otg_start_fsm(struct usb_otg *otg)
> +{
> +	struct otg_fsm *fsm = &otg->fsm;
> +
> +	if (fsm->running)
> +		goto kick_fsm;
> +
> +	if (!otg->host) {
> +		dev_info(otg->dev, "otg: can't start till host registers\n");
> +		return;
> +	}
> +
> +	if (!otg->gadget) {
> +		dev_info(otg->dev,
> +			 "otg: can't start till gadget UDC registers\n");
> +		return;
> +	}

okay, so you never kick the FSM until host and gadget are
registered. Why do you need to test for the case where the FSM is
running without host/gadget?

You seem to guarantee that will never be the case.

> +	if (!otg->gadget_ready) {
> +		dev_info(otg->dev,
> +			 "otg: can't start till gadget function registers\n");
> +		return;
> +	}
> +
> +	fsm->running = true;
> +kick_fsm:
> +	queue_work(otg->wq, &otg->work);
> +}
> +
> +/**
> + * usb_otg_stop_fsm() - Stop the OTG FSM
> + * @otg:	usb_otg instance
> + *
> + * Stops the HCD, UDC and the OTG FSM.
> + *
> + * fsm->lock must be held.
> + */
> +static void usb_otg_stop_fsm(struct usb_otg *otg)
> +{
> +	struct otg_fsm *fsm = &otg->fsm;
> +
> +	if (!fsm->running)
> +		return;
> +
> +	/* no more new events queued */
> +	fsm->running = false;
> +
> +	flush_workqueue(otg->wq);
> +	otg->state = OTG_STATE_UNDEFINED;
> +
> +	/* stop host/gadget immediately */
> +	if (fsm->protocol == PROTO_HOST) {
> +		otg_start_host(otg, 0);
> +	} else if (fsm->protocol == PROTO_GADGET) {
> +		otg->gadget_ops->connect_control(otg->gadget, false);
> +		otg_start_gadget(otg, 0);
> +	}
> +	fsm->protocol = PROTO_UNDEF;
> +}
> +
> +/**
> + * usb_otg_sync_inputs() - Sync OTG inputs with the OTG state machine
> + * @otg:	usb_otg instance
> + *
> + * Used by the OTG driver to update the inputs to the OTG
> + * state machine.
> + *
> + * Can be called in IRQ context.
> + */
> +void usb_otg_sync_inputs(struct usb_otg *otg)
> +{
> +	/* Don't kick FSM till it has started */
> +	if (!otg->fsm.running)
> +		return;
> +
> +	/* Kick FSM */
> +	queue_work(otg->wq, &otg->work);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
> +
> +/**
> + * usb_otg_register_hcd() - Register the host controller to OTG core
> + * @hcd:	host controller
> + * @irqnum:	interrupt number
> + * @irqflags:	interrupt flags
> + * @ops:	HCD ops to interface with the HCD
> + *
> + * This is used by the USB Host stack to register the host controller
> + * to the OTG core. Host controller must not be started by the
> + * caller as it is left up to the OTG state machine to do so.
> + * hcd->otg_dev must contain the related otg controller device.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> +			 unsigned long irqflags, struct otg_hcd_ops *ops)
> +{
> +	struct usb_otg *otg;
> +	struct device *hcd_dev = hcd->self.controller;
> +	struct device *otg_dev = hcd->otg_dev;
> +
> +	if (!otg_dev)
> +		return -EINVAL;
> +
> +	/* we're otg but otg controller might not yet be registered */
> +	mutex_lock(&otg_list_mutex);
> +	otg = usb_otg_get_data(otg_dev);
> +	mutex_unlock(&otg_list_mutex);
> +	if (!otg) {
> +		dev_dbg(hcd_dev,
> +			"otg: controller not yet registered. deferring.\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	/* HCD will be started by OTG fsm when needed */
> +	mutex_lock(&otg->fsm.lock);
> +	if (otg->primary_hcd.hcd) {
> +		/* probably a shared HCD ? */
> +		if (usb_otg_hcd_is_primary_hcd(hcd)) {
> +			dev_err(otg_dev, "otg: primary host already registered\n");
> +			goto err;
> +		}
> +
> +		if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> +			if (otg->shared_hcd.hcd) {
> +				dev_err(otg_dev, "otg: shared host already registered\n");
> +				goto err;
> +			}
> +
> +			otg->shared_hcd.hcd = hcd;
> +			otg->shared_hcd.irqnum = irqnum;
> +			otg->shared_hcd.irqflags = irqflags;
> +			otg->shared_hcd.ops = ops;
> +			dev_info(otg_dev, "otg: shared host %s registered\n",
> +				 dev_name(hcd->self.controller));
> +		} else {
> +			dev_err(otg_dev, "otg: invalid shared host %s\n",
> +				dev_name(hcd->self.controller));
> +			goto err;
> +		}
> +	} else {
> +		if (!usb_otg_hcd_is_primary_hcd(hcd)) {
> +			dev_err(otg_dev, "otg: primary host must be registered first\n");
> +			goto err;
> +		}
> +
> +		otg->primary_hcd.hcd = hcd;
> +		otg->primary_hcd.irqnum = irqnum;
> +		otg->primary_hcd.irqflags = irqflags;
> +		otg->primary_hcd.ops = ops;
> +		otg->hcd_ops = ops;
> +		dev_info(otg_dev, "otg: primary host %s registered\n",
> +			 dev_name(hcd->self.controller));
> +	}
> +
> +	/*
> +	 * we're ready only if we have shared HCD
> +	 * or we don't need shared HCD.
> +	 */
> +	if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> +		otg->host = hcd_to_bus(hcd);
> +		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
> +
> +		/* start FSM */
> +		usb_otg_start_fsm(otg);
> +	} else {
> +		dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
> +	}
> +
> +	mutex_unlock(&otg->fsm.lock);
> +
> +	return 0;
> +
> +err:
> +	mutex_unlock(&otg->fsm.lock);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
> +
> +/**
> + * usb_otg_unregister_hcd() - Unregister the host controller from OTG core
> + * @hcd:	host controller device
> + *
> + * This is used by the USB Host stack to unregister the host controller
> + * from the OTG core. Ensures that host controller is not running
> + * on successful return.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister_hcd(struct usb_hcd *hcd)
> +{
> +	struct usb_otg *otg;
> +	struct device *hcd_dev = hcd_to_bus(hcd)->controller;
> +	struct device *otg_dev = hcd->otg_dev;
> +
> +	if (!otg_dev)
> +		return -EINVAL;	/* we're definitely not OTG */
> +
> +	mutex_lock(&otg_list_mutex);
> +	otg = usb_otg_get_data(otg_dev);
> +	mutex_unlock(&otg_list_mutex);
> +	if (!otg) {
> +		dev_err(hcd_dev, "otg: host %s wasn't registered with otg\n",
> +			dev_name(hcd_dev));
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&otg->fsm.lock);
> +	if (hcd == otg->primary_hcd.hcd) {
> +		otg->primary_hcd.hcd = NULL;
> +		dev_info(otg_dev, "otg: primary host %s unregistered\n",
> +			 dev_name(hcd_dev));
> +	} else if (hcd == otg->shared_hcd.hcd) {
> +		otg->shared_hcd.hcd = NULL;
> +		dev_info(otg_dev, "otg: shared host %s unregistered\n",
> +			 dev_name(hcd_dev));
> +	} else {
> +		mutex_unlock(&otg->fsm.lock);
> +		dev_err(otg_dev, "otg: host %s wasn't registered with otg\n",
> +			dev_name(hcd_dev));
> +		return -EINVAL;
> +	}
> +
> +	/* stop FSM & Host */
> +	usb_otg_stop_fsm(otg);
> +	otg->host = NULL;
> +
> +	mutex_unlock(&otg->fsm.lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister_hcd);
> +
> +/**
> + * usb_otg_register_gadget() - Register the gadget controller to OTG core
> + * @gadget:	gadget controller instance
> + * @ops:	gadget interface ops
> + *
> + * This is used by the USB gadget stack to register the gadget controller
> + * to the OTG core. Gadget controller must not be started by the
> + * caller as it is left up to the OTG state machine to do so.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_register_gadget(struct usb_gadget *gadget,
> +			    struct otg_gadget_ops *ops)
> +{
> +	struct usb_otg *otg;
> +	struct device *gadget_dev = &gadget->dev;
> +	struct device *otg_dev = gadget->otg_dev;
> +
> +	if (!otg_dev)
> +		return -EINVAL;	/* we're definitely not OTG */
> +
> +	/* we're otg but otg controller might not yet be registered */
> +	mutex_lock(&otg_list_mutex);
> +	otg = usb_otg_get_data(otg_dev);
> +	mutex_unlock(&otg_list_mutex);
> +	if (!otg) {
> +		dev_dbg(gadget_dev,
> +			"otg: controller not yet registered, deferring.\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	mutex_lock(&otg->fsm.lock);
> +	if (otg->gadget) {
> +		dev_err(otg_dev, "otg: gadget already registered with otg\n");
> +		mutex_unlock(&otg->fsm.lock);
> +		return -EINVAL;
> +	}
> +
> +	otg->gadget = gadget;
> +	otg->gadget_ops = ops;
> +	dev_info(otg_dev, "otg: gadget %s registered\n",
> +		 dev_name(&gadget->dev));
> +
> +	/* FSM will be started in usb_otg_gadget_ready() */
> +	mutex_unlock(&otg->fsm.lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register_gadget);
> +
> +/**
> + * usb_otg_unregister_gadget() - Unregister the gadget controller from OTG core
> + * @gadget:	gadget controller
> + *
> + * This is used by the USB gadget stack to unregister the gadget controller
> + * from the OTG core. Ensures that gadget controller is not running
> + * on successful return.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister_gadget(struct usb_gadget *gadget)
> +{
> +	struct usb_otg *otg;
> +	struct device *gadget_dev = &gadget->dev;
> +	struct device *otg_dev = gadget->otg_dev;
> +
> +	if (!otg_dev)
> +		return -EINVAL;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otg = usb_otg_get_data(otg_dev);
> +	mutex_unlock(&otg_list_mutex);
> +	if (!otg) {
> +		dev_err(gadget_dev,
> +			"otg: gadget %s wasn't registered with otg\n",
> +			dev_name(&gadget->dev));
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&otg->fsm.lock);
> +	if (otg->gadget != gadget) {
> +		mutex_unlock(&otg->fsm.lock);
> +		dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
> +			dev_name(&gadget->dev));
> +		return -EINVAL;
> +	}
> +
> +	/* FSM must be stopped in usb_otg_gadget_ready() */
> +	if (otg->gadget_ready) {
> +		dev_err(otg_dev,
> +			"otg: gadget %s unregistered before being unready, forcing stop\n",
> +			dev_name(&gadget->dev));
> +		usb_otg_stop_fsm(otg);
> +	}
> +
> +	otg->gadget = NULL;
> +	mutex_unlock(&otg->fsm.lock);
> +
> +	dev_info(otg_dev, "otg: gadget %s unregistered\n",
> +		 dev_name(&gadget->dev));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister_gadget);
> +
> +/**
> + * usb_otg_gadget_ready() - Notify gadget function driver ready status
> + * @gadget:	gadget controller
> + * @ready:	0: function driver ready, 1: function driver not ready
> + *
> + * Notify the OTG core about status of the gadget function driver.
> + * As OTG core is responsible to start/stop the gadget controller, it
> + * must be aware when the gadget function driver is available or not.
> + * This function is used by the Gadget core to inform the OTG core
> + * about the gadget function driver readyness.
> + *
> + * Return: 0 on sucess, error value otherwise.
> + */
> +int usb_otg_gadget_ready(struct usb_gadget *gadget, bool ready)
> +{
> +	struct usb_otg *otg;
> +	struct device *gadget_dev = &gadget->dev;
> +	struct device *otg_dev = gadget->otg_dev;
> +
> +	if (!otg_dev)
> +		return -EINVAL;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otg = usb_otg_get_data(otg_dev);
> +	mutex_unlock(&otg_list_mutex);
> +	if (!otg) {
> +		dev_err(gadget_dev,
> +			"otg: gadget %s wasn't registered with otg\n",
> +			dev_name(&gadget->dev));
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&otg->fsm.lock);
> +	if (otg->gadget != gadget) {
> +		mutex_unlock(&otg->fsm.lock);
> +		dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
> +			dev_name(&gadget->dev));
> +		return -EINVAL;
> +	}
> +
> +	/* Start/stop FSM & gadget */
> +	otg->gadget_ready = ready;
> +	if (ready)
> +		usb_otg_start_fsm(otg);
> +	else
> +		usb_otg_stop_fsm(otg);
> +
> +	dev_dbg(otg_dev, "otg: gadget %s %sready\n", dev_name(&gadget->dev),
> +		ready ? "" : "not ");
> +
> +	mutex_unlock(&otg->fsm.lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_gadget_ready);
> +
> +MODULE_LICENSE("GPL");

GPL or GPL 2-only?

> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index f4fc0aa..1d74fb8 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>   * @in_epnum: last used in ep number
>   * @mA: last set mA value
>   * @otg_caps: OTG capabilities of this gadget.
> + * @otg_dev: OTG controller device, if needs to be used with OTG core.

do you really know of any platform which has a separate OTG controller?

-- 
balbi

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

#1426466

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

Hi,

On 20/06/16 10:45, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
> 
> I think I have already mentioned this, but after over 10 years of OTG,
> nobody seems to care about it, why are we still touching at all I don't
> know. For common non-OTG role-swapping we really don't need any of this
> and, quite frankly, I fail to see enough users for this.
> 
> Apparently there's only chipidea which, AFAICT, already had working
> dual-role before this OTG State Machine was added to the kernel.
> 
>> Provide a dual-role device (DRD) state machine.
> 
> there's not such thing as DRD state machine. You don't need to go
> through all these states, actually.

There are 3 states though.
HOST (id = 0)
PERIPHERAL (id = 1, vbus = 1)
IDLE (id = 1, vbus = 0).

> 
>> DRD mode is a reduced functionality OTG mode. In this mode
>> we don't support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral)
>> is decided based on the ID pin status. Once a cable plug (Type-A
>> or Type-B) is attached the controller selects the state
>> and doesn't change till the cable in unplugged and a different
>> cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers
>> we implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs
>> 'id' and 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> v11:
>> - remove usb_otg_kick_fsm().
>> - typo fixes: structa/structure, upto/up to.
>> - remove "obj-$(CONFIG_USB_OTG_CORE)     += common/" from Makefile.
>>
>>  drivers/usb/Kconfig          |  18 +
>>  drivers/usb/common/Makefile  |   6 +-
>>  drivers/usb/common/usb-otg.c | 877 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/core/Kconfig     |  14 -
>>  drivers/usb/gadget/Kconfig   |   1 +
>>  include/linux/usb/gadget.h   |   2 +
>>  include/linux/usb/hcd.h      |   1 +
>>  include/linux/usb/otg-fsm.h  |   7 +
>>  include/linux/usb/otg.h      | 174 ++++++++-
>>  9 files changed, 1070 insertions(+), 30 deletions(-)
>>  create mode 100644 drivers/usb/common/usb-otg.c
>>
>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>> index 8689dcb..ed596ec 100644
>> --- a/drivers/usb/Kconfig
>> +++ b/drivers/usb/Kconfig
>> @@ -32,6 +32,23 @@ if USB_SUPPORT
>>  config USB_COMMON
>>  	tristate
>>  
>> +config USB_OTG_CORE
>> +	tristate
> 
> why tristate if you can never set it to 'M'?

This gets internally set to M if either USB or GADGET is M.
We select it in USB and GADGET.
This was the only way I could get usb-otg.c to build as

m if USB OR GADGET is m
built-in if USB and GADGET are built in.

> 
>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>> new file mode 100644
>> index 0000000..a23ab1e
>> --- /dev/null
>> +++ b/drivers/usb/common/usb-otg.c
>> @@ -0,0 +1,877 @@
>> +/**
>> + * drivers/usb/common/usb-otg.c - USB OTG core
>> + *
>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
>> + * Author: 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 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.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/usb/of.h>
>> +#include <linux/usb/otg.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/workqueue.h>
>> +
>> +/* OTG device list */
>> +LIST_HEAD(otg_list);
> 
> not static? Who needs to touch this private list?

right, it should be static.

> 
>> +static DEFINE_MUTEX(otg_list_mutex);
>> +
>> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
>> +{
>> +	if (!hcd->primary_hcd)
>> +		return 1;
> 
> these seems inverted. If hcd->primary is NULL (meaning, there's no
> ->primary_hcd), then we tell caller that this _is_ a primary hcd? Care
> to explain?

hcd->primary_hcd is a link used by the shared hcd to point to the primary_hcd.
primary_hcd's have this link as NULL.

> 
>> +	return hcd == hcd->primary_hcd;
>> +}
>> +
>> +/**
>> + * usb_otg_get_data() - get usb_otg data structure
>> + * @otg_dev:	OTG controller device
>> + *
>> + * Check if the OTG device is in our OTG list and return
>> + * usb_otg data, else NULL.
>> + *
>> + * otg_list_mutex must be held.
>> + *
>> + * Return: usb_otg data on success, NULL otherwise.
>> + */
>> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev)
>> +{
>> +	struct usb_otg *otg;
>> +
>> +	if (!otg_dev)
>> +		return NULL;
>> +
>> +	list_for_each_entry(otg, &otg_list, list) {
>> +		if (otg->dev == otg_dev)
>> +			return otg;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * usb_otg_start_host() - start/stop the host controller
>> + * @otg:	usb_otg instance
>> + * @on:		true to start, false to stop
>> + *
>> + * Start/stop the USB host controller. This function is meant
>> + * for use by the OTG controller driver.
>> + *
>> + * Return: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_start_host(struct usb_otg *otg, int on)
>> +{
>> +	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
>> +	int ret;
>> +
>> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>> +	if (!otg->host) {
>> +		WARN_ONCE(1, "otg: fsm running without host\n");
> 
> if (WARN_ONCE(!otg->host, "otg: fsm running without host\n"))
> 	return 0;
> 
> but, frankly, if you require a 'host' and a 'gadget' don't start this
> layer until you have both.

We don't start the layer till we have both host and gadget. But
this API is for external use and might be called at any time.

I could change the warning message to
 
if (WARN_ONCE(!otg->host, "otg: %s called in invalid context\n", __func__))
	return 0;

> 
>> +		return 0;
>> +	}
>> +
>> +	if (on) {
>> +		if (otg->flags & OTG_FLAG_HOST_RUNNING)
>> +			return 0;
>> +
>> +		/* start host */
>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
>> +				   otg->primary_hcd.irqnum,
>> +				   otg->primary_hcd.irqflags);
> 
> this is usb_add_hcd(), is it not? Why add an indirection?

I've introduced the host and gadget ops interface to get around the
circular dependency issue we can't avoid.
otg needs to call host/gadget functions and host/gadget also needs to
call otg functions.

> 
>> +		if (ret) {
>> +			dev_err(otg->dev, "otg: host add failed %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		if (otg->shared_hcd.hcd) {
>> +			ret = hcd_ops->add(otg->shared_hcd.hcd,
>> +					   otg->shared_hcd.irqnum,
>> +					   otg->shared_hcd.irqflags);
>> +			if (ret) {
>> +				dev_err(otg->dev, "otg: shared host add failed %d\n",
>> +					ret);
>> +				hcd_ops->remove(otg->primary_hcd.hcd);
>> +				return ret;
>> +			}
>> +		}
>> +		otg->flags |= OTG_FLAG_HOST_RUNNING;
>> +	} else {
>> +		if (!(otg->flags & OTG_FLAG_HOST_RUNNING))
>> +			return 0;
>> +
>> +		otg->flags &= ~OTG_FLAG_HOST_RUNNING;
>> +
>> +		/* stop host */
>> +		if (otg->shared_hcd.hcd)
>> +			hcd_ops->remove(otg->shared_hcd.hcd);
> 
> usb_del_hcd()?
> 
>> +		hcd_ops->remove(otg->primary_hcd.hcd);
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_start_host);
>> +
>> +/**
>> + * usb_otg_start_gadget() - start/stop the gadget controller
>> + * @otg:	usb_otg instance
>> + * @on:		true to start, false to stop
>> + *
>> + * Start/stop the USB gadget controller. This function is meant
>> + * for use by the OTG controller driver.
>> + *
>> + * Return: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_start_gadget(struct usb_otg *otg, int on)
>> +{
>> +	struct usb_gadget *gadget = otg->gadget;
>> +	int ret;
>> +
>> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>> +	if (!gadget) {
>> +		WARN_ONCE(1, "otg: fsm running without gadget\n");
>> +		return 0;
>> +	}
> 
> ditto

I'll fix up the message to

("otg: %s called in invalid context\n", __func__)

> 
>> +	if (on) {
>> +		if (otg->flags & OTG_FLAG_GADGET_RUNNING)
>> +			return 0;
>> +
>> +		ret = otg->gadget_ops->start(otg->gadget);
>> +		if (ret) {
>> +			dev_err(otg->dev, "otg: gadget start failed: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +
>> +		otg->flags |= OTG_FLAG_GADGET_RUNNING;
>> +	} else {
>> +		if (!(otg->flags & OTG_FLAG_GADGET_RUNNING))
>> +			return 0;
>> +
>> +		ret = otg->gadget_ops->stop(otg->gadget);
>> +		if (ret) {
>> +			dev_err(otg->dev, "otg: gadget stop failed: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +		otg->flags &= ~OTG_FLAG_GADGET_RUNNING;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_start_gadget);
>> +
>> +/**
>> + * drd_set_protocol() -  Set USB protocol if possible
>> + * @fsm:	DRD FSM instance
>> + * @protocol:	USB protocol to set the state machine to
>> + *
>> + * Sets the OTG FSM protocol to @protocol if it changed.
>> + * fsm->lock must be held.
>> + *
>> + * Return: 0 on success, error value otherwise.
>> + */
>> +static int drd_set_protocol(struct otg_fsm *fsm, int protocol)
>> +{
>> +	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +	int ret = 0;
>> +
>> +	if (fsm->protocol != protocol) {
>> +		dev_dbg(otg->dev, "otg: changing role fsm->protocol= %d; new protocol= %d\n",
>> +			fsm->protocol, protocol);
>> +		/* stop old protocol */
>> +		if (fsm->protocol == PROTO_HOST) {
>> +			ret = otg_start_host(otg, 0);
>> +		} else if (fsm->protocol == PROTO_GADGET) {
>> +			otg->gadget_ops->connect_control(otg->gadget, false);
>> +			ret = otg_start_gadget(otg, 0);
>> +		}
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* start new protocol */
>> +		if (protocol == PROTO_HOST) {
>> +			ret = otg_start_host(otg, 1);
>> +		} else if (protocol == PROTO_GADGET) {
>> +			ret = otg_start_gadget(otg, 1);
>> +			otg->gadget_ops->connect_control(otg->gadget, true);
>> +		}
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		fsm->protocol = protocol;
>> +		return 0;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * drd_set_state() - Set the DRD state machine state.
>> + * @fsm:	DRD FSM instance
>> + * @new_state:	the new state the DRD FSM must be set to
>> + *
>> + * Sets the state of the DRD state machine.
>> + * fsm->lock must be held.
>> + */
>> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
>> +{
>> +	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +
>> +	if (otg->state == new_state)
>> +		return;
>> +
>> +	fsm->state_changed = 1;
>> +	dev_dbg(otg->dev, "otg: set state: %s\n",
>> +		usb_otg_state_string(new_state));
>> +	switch (new_state) {
>> +	case OTG_STATE_B_IDLE:
>> +		drd_set_protocol(fsm, PROTO_UNDEF);
>> +		otg_drv_vbus(otg, 0);
>> +		break;
>> +	case OTG_STATE_B_PERIPHERAL:
>> +		drd_set_protocol(fsm, PROTO_GADGET);
>> +		otg_drv_vbus(otg, 0);
>> +		break;
>> +	case OTG_STATE_A_HOST:
>> +		drd_set_protocol(fsm, PROTO_HOST);
>> +		otg_drv_vbus(otg, 1);
>> +		break;
>> +	default:
>> +		dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
>> +			 __func__, usb_otg_state_string(new_state));
>> +		break;
>> +	}
>> +
>> +	otg->state = new_state;
>> +}
>> +
>> +/**
>> + * drd_statemachine() - DRD state change judgement
>> + * @otg:	usb_otg instance
>> + *
>> + * Checks the state machine inputs and state and makes a state change
>> + * if required.
>> + *
>> + * For DRD we're only interested in some of the OTG states
>> + * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
>> + *	OTG_STATE_B_PERIPHERAL: peripheral active
>> + *	OTG_STATE_A_HOST: host active
>> + * we're only interested in the following inputs
>> + *	fsm->id, fsm->b_sess_vld
>> + *
>> + * Return: 0 if state wasn't changed, 1 if state changed.
>> + */
>> +int drd_statemachine(struct usb_otg *otg)
>> +{
>> +	struct otg_fsm *fsm = &otg->fsm;
>> +	enum usb_otg_state state;
>> +	int ret;
>> +
>> +	mutex_lock(&fsm->lock);
>> +
>> +	fsm->state_changed = 0;
>> +	state = otg->state;
>> +
>> +	switch (state) {
>> +	case OTG_STATE_UNDEFINED:
>> +		if (!fsm->id)
>> +			drd_set_state(fsm, OTG_STATE_A_HOST);
>> +		else if (fsm->id && fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
>> +		else
>> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
>> +		break;
>> +	case OTG_STATE_B_IDLE:
>> +		if (!fsm->id)
>> +			drd_set_state(fsm, OTG_STATE_A_HOST);
>> +		else if (fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
>> +		break;
>> +	case OTG_STATE_B_PERIPHERAL:
>> +		if (!fsm->id)
>> +			drd_set_state(fsm, OTG_STATE_A_HOST);
>> +		else if (!fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
>> +		break;
>> +	case OTG_STATE_A_HOST:
>> +		if (fsm->id && fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
>> +		else if (fsm->id && !fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
>> +		break;
>> +
>> +	default:
>> +		dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
>> +			__func__, usb_otg_state_string(state));
>> +		break;
>> +	}
>> +
>> +	ret = fsm->state_changed;
>> +	mutex_unlock(&fsm->lock);
>> +	dev_dbg(otg->dev, "otg: quit statemachine, changed %d\n",
>> +		fsm->state_changed);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(drd_statemachine);
> 
> why is this exported at all? It only runs from the work_struct

You're right. It shouldn't be exported.

> below. BTW, that work_struct looks unnecessary.

why? The kick could be triggered from an interrupt context. e.g. otg_irq.

> 
>> +
>> +/**
>> + * usb_drd_work() - Dual-role state machine work function
>> + * @work: work_struct context
>> + *
>> + * Runs the DRD state machine. Scheduled whenever there is a change
>> + * in FSM inputs.
>> + */
>> +static void usb_drd_work(struct work_struct *work)
>> +{
>> +	struct usb_otg *otg = container_of(work, struct usb_otg, work);
>> +
>> +	pm_runtime_get_sync(otg->dev);
>> +	while (drd_statemachine(otg))
>> +		;
>> +	pm_runtime_put_sync(otg->dev);
> 
> so, once this gets kicked it'll keep on running until a state doesn't
> change.

yes.
> 
>> +}
>> +
>> +/**
>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>> + * @dev: OTG/dual-role controller device.
>> + * @config: OTG configuration.
>> + *
>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>> + *
>> + * Return: struct usb_otg * if success, ERR_PTR() otherwise.
>> + */
>> +struct usb_otg *usb_otg_register(struct device *dev,
>> +				 struct usb_otg_config *config)
>> +{
>> +	struct usb_otg *otg;
>> +	int ret = 0;
>> +
>> +	if (!dev || !config || !config->fsm_ops)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	/* already in list? */
>> +	mutex_lock(&otg_list_mutex);
>> +	if (usb_otg_get_data(dev)) {
>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>> +			__func__);
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	/* allocate and add to list */
>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>> +	if (!otg) {
>> +		ret = -ENOMEM;
>> +		goto unlock;
>> +	}
>> +
>> +	otg->dev = dev;
>> +	/* otg->caps is controller caps + DT overrides */
>> +	otg->caps = *config->otg_caps;
>> +	ret = of_usb_update_otg_caps(dev->of_node, &otg->caps);
>> +	if (ret)
>> +		goto err_wq;
>> +
>> +	if ((otg->caps.hnp_support || otg->caps.srp_support ||
>> +	     otg->caps.adp_support) && !config->otg_work) {
>> +		dev_err(dev,
>> +			"otg: otg_work must be provided for OTG support\n");
>> +		ret = -EINVAL;
>> +		goto err_wq;
>> +	}
>> +
>> +	if (config->otg_work)	/* custom otg_work ? */
>> +		INIT_WORK(&otg->work, config->otg_work);
>> +	else
>> +		INIT_WORK(&otg->work, usb_drd_work);
> 
> why do you need to cope with custom work handlers?

It was just a provision to provide your own state machine if the generic
one does not meet your needs. But i'm OK to get rid of it as well.

Maybe Peter can chime in.

> 
>> +	otg->wq = create_freezable_workqueue("usb_otg");
>> +	if (!otg->wq) {
>> +		dev_err(dev, "otg: %s: can't create workqueue\n",
>> +			__func__);
>> +		ret = -ENOMEM;
>> +		goto err_wq;
>> +	}
>> +
>> +	/* set otg ops */
>> +	otg->fsm.ops = config->fsm_ops;
>> +
>> +	mutex_init(&otg->fsm.lock);
>> +
>> +	list_add_tail(&otg->list, &otg_list);
>> +	mutex_unlock(&otg_list_mutex);
>> +
>> +	return otg;
>> +
>> +err_wq:
>> +	kfree(otg);
>> +unlock:
>> +	mutex_unlock(&otg_list_mutex);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_register);
>> +
>> +/**
>> + * usb_otg_unregister() - Unregister the OTG/dual-role device from USB OTG core
>> + * @dev: OTG controller device.
>> + *
>> + * Unregisters the OTG/dual-role controller device from USB OTG core.
>> + * Prevents unregistering till both the associated Host and Gadget controllers
>> + * have unregistered from the OTG core.
>> + *
>> + * Return: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_unregister(struct device *dev)
>> +{
>> +	struct usb_otg *otg;
>> +
>> +	mutex_lock(&otg_list_mutex);
>> +	otg = usb_otg_get_data(dev);
>> +	if (!otg) {
>> +		dev_err(dev, "otg: %s: device not in otg list\n",
>> +			__func__);
>> +		mutex_unlock(&otg_list_mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* prevent unregister till both host & gadget have unregistered */
>> +	if (otg->host || otg->gadget) {
>> +		dev_err(dev, "otg: %s: host/gadget still registered\n",
>> +			__func__);
>> +		mutex_unlock(&otg_list_mutex);
>> +		return -EBUSY;
>> +	}
> 
> I think a better choice would've been to unregister host and gadget in
> case they haven't been unregistered yet. That's a common
> expectation. Specially since driver core does the same thing when you
> unregister a parent device.

I agree.

> 
>> +	/* OTG FSM is halted when host/gadget unregistered */
>> +	destroy_workqueue(otg->wq);
>> +
>> +	/* remove from otg list */
>> +	list_del(&otg->list);
>> +	kfree(otg);
>> +	mutex_unlock(&otg_list_mutex);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_unregister);
>> +
>> +/**
>> + * usb_otg_start_fsm() - Start the OTG FSM
>> + * @otg:	usb_otg instance
>> + *
>> + * Start the OTG FSM if we can. The HCD, UDC and gadget function driver
>> + * must be ready for the OTG FSM to start.
>> + *
>> + * fsm->lock must be held.
>> + */
>> +static void usb_otg_start_fsm(struct usb_otg *otg)
>> +{
>> +	struct otg_fsm *fsm = &otg->fsm;
>> +
>> +	if (fsm->running)
>> +		goto kick_fsm;
>> +
>> +	if (!otg->host) {
>> +		dev_info(otg->dev, "otg: can't start till host registers\n");
>> +		return;
>> +	}
>> +
>> +	if (!otg->gadget) {
>> +		dev_info(otg->dev,
>> +			 "otg: can't start till gadget UDC registers\n");
>> +		return;
>> +	}
> 
> okay, so you never kick the FSM until host and gadget are
> registered. Why do you need to test for the case where the FSM is
> running without host/gadget?

That message in the test was misleading. It could also be a
used as a warning if users did something wrong.
> 
> You seem to guarantee that will never be the case.
> 
>> +	if (!otg->gadget_ready) {
>> +		dev_info(otg->dev,
>> +			 "otg: can't start till gadget function registers\n");
>> +		return;
>> +	}
>> +
>> +	fsm->running = true;
>> +kick_fsm:
>> +	queue_work(otg->wq, &otg->work);
>> +}
>> +
>> +/**
>> + * usb_otg_stop_fsm() - Stop the OTG FSM
>> + * @otg:	usb_otg instance
>> + *
>> + * Stops the HCD, UDC and the OTG FSM.
>> + *
>> + * fsm->lock must be held.
>> + */
>> +static void usb_otg_stop_fsm(struct usb_otg *otg)
>> +{
>> +	struct otg_fsm *fsm = &otg->fsm;
>> +
>> +	if (!fsm->running)
>> +		return;
>> +
>> +	/* no more new events queued */
>> +	fsm->running = false;
>> +
>> +	flush_workqueue(otg->wq);
>> +	otg->state = OTG_STATE_UNDEFINED;
>> +
>> +	/* stop host/gadget immediately */
>> +	if (fsm->protocol == PROTO_HOST) {
>> +		otg_start_host(otg, 0);
>> +	} else if (fsm->protocol == PROTO_GADGET) {
>> +		otg->gadget_ops->connect_control(otg->gadget, false);
>> +		otg_start_gadget(otg, 0);
>> +	}
>> +	fsm->protocol = PROTO_UNDEF;
>> +}
>> +
>> +/**
>> + * usb_otg_sync_inputs() - Sync OTG inputs with the OTG state machine
>> + * @otg:	usb_otg instance
>> + *
>> + * Used by the OTG driver to update the inputs to the OTG
>> + * state machine.
>> + *
>> + * Can be called in IRQ context.
>> + */
>> +void usb_otg_sync_inputs(struct usb_otg *otg)
>> +{
>> +	/* Don't kick FSM till it has started */
>> +	if (!otg->fsm.running)
>> +		return;
>> +
>> +	/* Kick FSM */
>> +	queue_work(otg->wq, &otg->work);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
>> +
>> +/**
>> + * usb_otg_register_hcd() - Register the host controller to OTG core
>> + * @hcd:	host controller
>> + * @irqnum:	interrupt number
>> + * @irqflags:	interrupt flags
>> + * @ops:	HCD ops to interface with the HCD
>> + *
>> + * This is used by the USB Host stack to register the host controller
>> + * to the OTG core. Host controller must not be started by the
>> + * caller as it is left up to the OTG state machine to do so.
>> + * hcd->otg_dev must contain the related otg controller device.
>> + *
>> + * Return: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
>> +			 unsigned long irqflags, struct otg_hcd_ops *ops)
>> +{
>> +	struct usb_otg *otg;
>> +	struct device *hcd_dev = hcd->self.controller;
>> +	struct device *otg_dev = hcd->otg_dev;
>> +
>> +	if (!otg_dev)
>> +		return -EINVAL;
>> +
>> +	/* we're otg but otg controller might not yet be registered */
>> +	mutex_lock(&otg_list_mutex);
>> +	otg = usb_otg_get_data(otg_dev);
>> +	mutex_unlock(&otg_list_mutex);
>> +	if (!otg) {
>> +		dev_dbg(hcd_dev,
>> +			"otg: controller not yet registered. deferring.\n");
>> +		return -EPROBE_DEFER;
>> +	}
>> +
>> +	/* HCD will be started by OTG fsm when needed */
>> +	mutex_lock(&otg->fsm.lock);
>> +	if (otg->primary_hcd.hcd) {
>> +		/* probably a shared HCD ? */
>> +		if (usb_otg_hcd_is_primary_hcd(hcd)) {
>> +			dev_err(otg_dev, "otg: primary host already registered\n");
>> +			goto err;
>> +		}
>> +
>> +		if (hcd->shared_hcd == otg->primary_hcd.hcd) {
>> +			if (otg->shared_hcd.hcd) {
>> +				dev_err(otg_dev, "otg: shared host already registered\n");
>> +				goto err;
>> +			}
>> +
>> +			otg->shared_hcd.hcd = hcd;
>> +			otg->shared_hcd.irqnum = irqnum;
>> +			otg->shared_hcd.irqflags = irqflags;
>> +			otg->shared_hcd.ops = ops;
>> +			dev_info(otg_dev, "otg: shared host %s registered\n",
>> +				 dev_name(hcd->self.controller));
>> +		} else {
>> +			dev_err(otg_dev, "otg: invalid shared host %s\n",
>> +				dev_name(hcd->self.controller));
>> +			goto err;
>> +		}
>> +	} else {
>> +		if (!usb_otg_hcd_is_primary_hcd(hcd)) {
>> +			dev_err(otg_dev, "otg: primary host must be registered first\n");
>> +			goto err;
>> +		}
>> +
>> +		otg->primary_hcd.hcd = hcd;
>> +		otg->primary_hcd.irqnum = irqnum;
>> +		otg->primary_hcd.irqflags = irqflags;
>> +		otg->primary_hcd.ops = ops;
>> +		otg->hcd_ops = ops;
>> +		dev_info(otg_dev, "otg: primary host %s registered\n",
>> +			 dev_name(hcd->self.controller));
>> +	}
>> +
>> +	/*
>> +	 * we're ready only if we have shared HCD
>> +	 * or we don't need shared HCD.
>> +	 */
>> +	if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
>> +		otg->host = hcd_to_bus(hcd);
>> +		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
>> +
>> +		/* start FSM */
>> +		usb_otg_start_fsm(otg);
>> +	} else {
>> +		dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
>> +	}
>> +
>> +	mutex_unlock(&otg->fsm.lock);
>> +
>> +	return 0;
>> +
>> +err:
>> +	mutex_unlock(&otg->fsm.lock);
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
>> +
>> +/**
>> + * usb_otg_unregister_hcd() - Unregister the host controller from OTG core
>> + * @hcd:	host controller device
>> + *
>> + * This is used by the USB Host stack to unregister the host controller
>> + * from the OTG core. Ensures that host controller is not running
>> + * on successful return.
>> + *
>> + * Returns: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_unregister_hcd(struct usb_hcd *hcd)
>> +{
>> +	struct usb_otg *otg;
>> +	struct device *hcd_dev = hcd_to_bus(hcd)->controller;
>> +	struct device *otg_dev = hcd->otg_dev;
>> +
>> +	if (!otg_dev)
>> +		return -EINVAL;	/* we're definitely not OTG */
>> +
>> +	mutex_lock(&otg_list_mutex);
>> +	otg = usb_otg_get_data(otg_dev);
>> +	mutex_unlock(&otg_list_mutex);
>> +	if (!otg) {
>> +		dev_err(hcd_dev, "otg: host %s wasn't registered with otg\n",
>> +			dev_name(hcd_dev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&otg->fsm.lock);
>> +	if (hcd == otg->primary_hcd.hcd) {
>> +		otg->primary_hcd.hcd = NULL;
>> +		dev_info(otg_dev, "otg: primary host %s unregistered\n",
>> +			 dev_name(hcd_dev));
>> +	} else if (hcd == otg->shared_hcd.hcd) {
>> +		otg->shared_hcd.hcd = NULL;
>> +		dev_info(otg_dev, "otg: shared host %s unregistered\n",
>> +			 dev_name(hcd_dev));
>> +	} else {
>> +		mutex_unlock(&otg->fsm.lock);
>> +		dev_err(otg_dev, "otg: host %s wasn't registered with otg\n",
>> +			dev_name(hcd_dev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* stop FSM & Host */
>> +	usb_otg_stop_fsm(otg);
>> +	otg->host = NULL;
>> +
>> +	mutex_unlock(&otg->fsm.lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_unregister_hcd);
>> +
>> +/**
>> + * usb_otg_register_gadget() - Register the gadget controller to OTG core
>> + * @gadget:	gadget controller instance
>> + * @ops:	gadget interface ops
>> + *
>> + * This is used by the USB gadget stack to register the gadget controller
>> + * to the OTG core. Gadget controller must not be started by the
>> + * caller as it is left up to the OTG state machine to do so.
>> + *
>> + * Returns: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_register_gadget(struct usb_gadget *gadget,
>> +			    struct otg_gadget_ops *ops)
>> +{
>> +	struct usb_otg *otg;
>> +	struct device *gadget_dev = &gadget->dev;
>> +	struct device *otg_dev = gadget->otg_dev;
>> +
>> +	if (!otg_dev)
>> +		return -EINVAL;	/* we're definitely not OTG */
>> +
>> +	/* we're otg but otg controller might not yet be registered */
>> +	mutex_lock(&otg_list_mutex);
>> +	otg = usb_otg_get_data(otg_dev);
>> +	mutex_unlock(&otg_list_mutex);
>> +	if (!otg) {
>> +		dev_dbg(gadget_dev,
>> +			"otg: controller not yet registered, deferring.\n");
>> +		return -EPROBE_DEFER;
>> +	}
>> +
>> +	mutex_lock(&otg->fsm.lock);
>> +	if (otg->gadget) {
>> +		dev_err(otg_dev, "otg: gadget already registered with otg\n");
>> +		mutex_unlock(&otg->fsm.lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	otg->gadget = gadget;
>> +	otg->gadget_ops = ops;
>> +	dev_info(otg_dev, "otg: gadget %s registered\n",
>> +		 dev_name(&gadget->dev));
>> +
>> +	/* FSM will be started in usb_otg_gadget_ready() */
>> +	mutex_unlock(&otg->fsm.lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_register_gadget);
>> +
>> +/**
>> + * usb_otg_unregister_gadget() - Unregister the gadget controller from OTG core
>> + * @gadget:	gadget controller
>> + *
>> + * This is used by the USB gadget stack to unregister the gadget controller
>> + * from the OTG core. Ensures that gadget controller is not running
>> + * on successful return.
>> + *
>> + * Returns: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_unregister_gadget(struct usb_gadget *gadget)
>> +{
>> +	struct usb_otg *otg;
>> +	struct device *gadget_dev = &gadget->dev;
>> +	struct device *otg_dev = gadget->otg_dev;
>> +
>> +	if (!otg_dev)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&otg_list_mutex);
>> +	otg = usb_otg_get_data(otg_dev);
>> +	mutex_unlock(&otg_list_mutex);
>> +	if (!otg) {
>> +		dev_err(gadget_dev,
>> +			"otg: gadget %s wasn't registered with otg\n",
>> +			dev_name(&gadget->dev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&otg->fsm.lock);
>> +	if (otg->gadget != gadget) {
>> +		mutex_unlock(&otg->fsm.lock);
>> +		dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
>> +			dev_name(&gadget->dev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* FSM must be stopped in usb_otg_gadget_ready() */
>> +	if (otg->gadget_ready) {
>> +		dev_err(otg_dev,
>> +			"otg: gadget %s unregistered before being unready, forcing stop\n",
>> +			dev_name(&gadget->dev));
>> +		usb_otg_stop_fsm(otg);
>> +	}
>> +
>> +	otg->gadget = NULL;
>> +	mutex_unlock(&otg->fsm.lock);
>> +
>> +	dev_info(otg_dev, "otg: gadget %s unregistered\n",
>> +		 dev_name(&gadget->dev));
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_unregister_gadget);
>> +
>> +/**
>> + * usb_otg_gadget_ready() - Notify gadget function driver ready status
>> + * @gadget:	gadget controller
>> + * @ready:	0: function driver ready, 1: function driver not ready
>> + *
>> + * Notify the OTG core about status of the gadget function driver.
>> + * As OTG core is responsible to start/stop the gadget controller, it
>> + * must be aware when the gadget function driver is available or not.
>> + * This function is used by the Gadget core to inform the OTG core
>> + * about the gadget function driver readyness.
>> + *
>> + * Return: 0 on sucess, error value otherwise.
>> + */
>> +int usb_otg_gadget_ready(struct usb_gadget *gadget, bool ready)
>> +{
>> +	struct usb_otg *otg;
>> +	struct device *gadget_dev = &gadget->dev;
>> +	struct device *otg_dev = gadget->otg_dev;
>> +
>> +	if (!otg_dev)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&otg_list_mutex);
>> +	otg = usb_otg_get_data(otg_dev);
>> +	mutex_unlock(&otg_list_mutex);
>> +	if (!otg) {
>> +		dev_err(gadget_dev,
>> +			"otg: gadget %s wasn't registered with otg\n",
>> +			dev_name(&gadget->dev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&otg->fsm.lock);
>> +	if (otg->gadget != gadget) {
>> +		mutex_unlock(&otg->fsm.lock);
>> +		dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
>> +			dev_name(&gadget->dev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Start/stop FSM & gadget */
>> +	otg->gadget_ready = ready;
>> +	if (ready)
>> +		usb_otg_start_fsm(otg);
>> +	else
>> +		usb_otg_stop_fsm(otg);
>> +
>> +	dev_dbg(otg_dev, "otg: gadget %s %sready\n", dev_name(&gadget->dev),
>> +		ready ? "" : "not ");
>> +
>> +	mutex_unlock(&otg->fsm.lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_gadget_ready);
>> +
>> +MODULE_LICENSE("GPL");
> 
> GPL or GPL 2-only?

GPL v2.

> 
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index f4fc0aa..1d74fb8 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>>   * @in_epnum: last used in ep number
>>   * @mA: last set mA value
>>   * @otg_caps: OTG capabilities of this gadget.
>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> 
> do you really know of any platform which has a separate OTG controller?
> 

Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
and gadget.

[1] http://article.gmane.org/gmane.linux.ports.tegra/22969

Yoshihiro,

How is the dual-role architecture on your Renesas platform?

cheers,
-roger

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

#1426549

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

On 20/06/16 15:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> It provides APIs for the following tasks
>>>>
>>>> - Registering an OTG/dual-role capable controller
>>>> - Registering Host and Gadget controllers to OTG core
>>>> - Providing inputs to and kicking the OTG state machine
>>>
>>> I think I have already mentioned this, but after over 10 years of OTG,
>>> nobody seems to care about it, why are we still touching at all I don't
>>> know. For common non-OTG role-swapping we really don't need any of this
>>> and, quite frankly, I fail to see enough users for this.
>>>
>>> Apparently there's only chipidea which, AFAICT, already had working
>>> dual-role before this OTG State Machine was added to the kernel.
>>>
>>>> Provide a dual-role device (DRD) state machine.
>>>
>>> there's not such thing as DRD state machine. You don't need to go
>>> through all these states, actually.
>>
>> There are 3 states though.
>> HOST (id = 0)
>> PERIPHERAL (id = 1, vbus = 1)
>> IDLE (id = 1, vbus = 0).
> 
> IDLE is pretty much given, though ;-)
> 
>>>> DRD mode is a reduced functionality OTG mode. In this mode
>>>> we don't support SRP, HNP and dynamic role-swap.
>>>>
>>>> In DRD operation, the controller mode (Host or Peripheral)
>>>> is decided based on the ID pin status. Once a cable plug (Type-A
>>>> or Type-B) is attached the controller selects the state
>>>> and doesn't change till the cable in unplugged and a different
>>>> cable type is inserted.
>>>>
>>>> As we don't need most of the complex OTG states and OTG timers
>>>> we implement a lean DRD state machine in usb-otg.c.
>>>> The DRD state machine is only interested in 2 hardware inputs
>>>> 'id' and 'b_sess_vld'.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>> v11:
>>>> - remove usb_otg_kick_fsm().
>>>> - typo fixes: structa/structure, upto/up to.
>>>> - remove "obj-$(CONFIG_USB_OTG_CORE)     += common/" from Makefile.
>>>>
>>>>  drivers/usb/Kconfig          |  18 +
>>>>  drivers/usb/common/Makefile  |   6 +-
>>>>  drivers/usb/common/usb-otg.c | 877 +++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/usb/core/Kconfig     |  14 -
>>>>  drivers/usb/gadget/Kconfig   |   1 +
>>>>  include/linux/usb/gadget.h   |   2 +
>>>>  include/linux/usb/hcd.h      |   1 +
>>>>  include/linux/usb/otg-fsm.h  |   7 +
>>>>  include/linux/usb/otg.h      | 174 ++++++++-
>>>>  9 files changed, 1070 insertions(+), 30 deletions(-)
>>>>  create mode 100644 drivers/usb/common/usb-otg.c
>>>>
>>>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>>>> index 8689dcb..ed596ec 100644
>>>> --- a/drivers/usb/Kconfig
>>>> +++ b/drivers/usb/Kconfig
>>>> @@ -32,6 +32,23 @@ if USB_SUPPORT
>>>>  config USB_COMMON
>>>>  	tristate
>>>>  
>>>> +config USB_OTG_CORE
>>>> +	tristate
>>>
>>> why tristate if you can never set it to 'M'?
>>
>> This gets internally set to M if either USB or GADGET is M.
>> We select it in USB and GADGET.
>> This was the only way I could get usb-otg.c to build as
>>
>> m if USB OR GADGET is m
>> built-in if USB and GADGET are built in.
> 
> I could only see a "select USB_OTG_CORE", select will always set it 'y'
> and disregard dependencies. Maybe I missed something else.

Not always. See how USB_COMMON works.
> 
>>>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>>>> new file mode 100644
>>>> index 0000000..a23ab1e
>>>> --- /dev/null
>>>> +++ b/drivers/usb/common/usb-otg.c
>>>> @@ -0,0 +1,877 @@
>>>> +/**
>>>> + * drivers/usb/common/usb-otg.c - USB OTG core
>>>> + *
>>>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
>>>> + * Author: 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 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.
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_platform.h>
>>>> +#include <linux/usb/of.h>
>>>> +#include <linux/usb/otg.h>
>>>> +#include <linux/usb/gadget.h>
>>>> +#include <linux/workqueue.h>
>>>> +
>>>> +/* OTG device list */
>>>> +LIST_HEAD(otg_list);
>>>
>>> not static? Who needs to touch this private list?
>>
>> right, it should be static.
>>
>>>
>>>> +static DEFINE_MUTEX(otg_list_mutex);
>>>> +
>>>> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
>>>> +{
>>>> +	if (!hcd->primary_hcd)
>>>> +		return 1;
>>>
>>> these seems inverted. If hcd->primary is NULL (meaning, there's no
>>> ->primary_hcd), then we tell caller that this _is_ a primary hcd? Care
>>> to explain?
>>
>> hcd->primary_hcd is a link used by the shared hcd to point to the
>> primary_hcd.  primary_hcd's have this link as NULL.
> 
> So the following check is unnecessary and should always evaluate to
> false, right ?

Actually primary_hcd's not having a shared HCD have hcd->primary_hcd as NULL
and those having a shared HCD do have it pointing to the primary hcd.

> 
>>>> +	return hcd == hcd->primary_hcd;
>>>> +}
>>>> +
>>>> +/**
>>>> + * usb_otg_get_data() - get usb_otg data structure
>>>> + * @otg_dev:	OTG controller device
>>>> + *
>>>> + * Check if the OTG device is in our OTG list and return
>>>> + * usb_otg data, else NULL.
>>>> + *
>>>> + * otg_list_mutex must be held.
>>>> + *
>>>> + * Return: usb_otg data on success, NULL otherwise.
>>>> + */
>>>> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev)
>>>> +{
>>>> +	struct usb_otg *otg;
>>>> +
>>>> +	if (!otg_dev)
>>>> +		return NULL;
>>>> +
>>>> +	list_for_each_entry(otg, &otg_list, list) {
>>>> +		if (otg->dev == otg_dev)
>>>> +			return otg;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * usb_otg_start_host() - start/stop the host controller
>>>> + * @otg:	usb_otg instance
>>>> + * @on:		true to start, false to stop
>>>> + *
>>>> + * Start/stop the USB host controller. This function is meant
>>>> + * for use by the OTG controller driver.
>>>> + *
>>>> + * Return: 0 on success, error value otherwise.
>>>> + */
>>>> +int usb_otg_start_host(struct usb_otg *otg, int on)
>>>> +{
>>>> +	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
>>>> +	int ret;
>>>> +
>>>> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>>>> +	if (!otg->host) {
>>>> +		WARN_ONCE(1, "otg: fsm running without host\n");
>>>
>>> if (WARN_ONCE(!otg->host, "otg: fsm running without host\n"))
>>> 	return 0;
>>>
>>> but, frankly, if you require a 'host' and a 'gadget' don't start this
>>> layer until you have both.
>>
>> We don't start the layer till we have both host and gadget. But
>> this API is for external use and might be called at any time.
> 
> well, if callers call this at the wrong time, it's callers' fault. Let
> them oops so we catch the error.

So you suggest we allow a NULL pointer dereference here?

> 
>> I could change the warning message to
>>  
>> if (WARN_ONCE(!otg->host, "otg: %s called in invalid context\n", __func__))
>> 	return 0;
> 
> you don't neet that.
> 
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	if (on) {
>>>> +		if (otg->flags & OTG_FLAG_HOST_RUNNING)
>>>> +			return 0;
>>>> +
>>>> +		/* start host */
>>>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
>>>> +				   otg->primary_hcd.irqnum,
>>>> +				   otg->primary_hcd.irqflags);
>>>
>>> this is usb_add_hcd(), is it not? Why add an indirection?
>>
>> I've introduced the host and gadget ops interface to get around the
>> circular dependency issue we can't avoid.
>> otg needs to call host/gadget functions and host/gadget also needs to
>> call otg functions.
> 
> IMO, this shows a fragility of your design. You're, now, lying to
> usb_hcd and usb_udc and making them register into a virtual layer that
> doesn't exist. And that layer will end up calling the real registration
> function when some magic event happens.
> 
> This is only really needed for quirky devices like dwc3 (but see more on
> dwc3 below) where host and peripheral registers shadow each
> other. Otherwise we would be able to always keep hcd and udc always
> registered. They would get different interrupt statuses anyway and
> nothing would ever break.

Well I only had the opportunity to work with dwc3 so I had to ensure
the design worked with it.

> 
> However, when it comes to dwc3, we already have all the code necessary
> to workaround this issue by destroying the XHCI pdev when OTG interrupt
> says we should be peripheral (and vice-versa). DWC3 also keeps track of
> the OTG states for those folks who really care about OTG (Hint: nobody
> has cared for the past 10 years, why would they do so now?) and we don't
> need a SW state machine when the HW handles that for us, right?

Where is the code? I'd like to test dual-role on TI platforms.

> 
> As for chipidea, IIRC, that doesn't need a SW state machine either, but
> I know very little about that IP and don't even have documentation on
> it. My understanding, however, is that chipidea behaves kinda like MUSB,
> which changes roles automatically in HW based on ID pin state.
> 
>>>> +EXPORT_SYMBOL_GPL(drd_statemachine);
>>>
>>> why is this exported at all? It only runs from the work_struct
>>
>> You're right. It shouldn't be exported.
>>
>>> below. BTW, that work_struct looks unnecessary.
>>
>> why? The kick could be triggered from an interrupt
>> context. e.g. otg_irq.
> 
> We have threaded IRQ handlers in the kernel, right? Make use of that
> and, with a little smart locking and IRQ masking, you can run the OTG
> IRQ thread almost completely lockless ;-)

Not a problem if we have the constraint that usb_otg_sync_inputs()
needs to be called in thread context only.

> 
>>>> +/**
>>>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>>>> + * @dev: OTG/dual-role controller device.
>>>> + * @config: OTG configuration.
>>>> + *
>>>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>>>> + *
>>>> + * Return: struct usb_otg * if success, ERR_PTR() otherwise.
>>>> + */
>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>> +				 struct usb_otg_config *config)
>>>> +{
>>>> +	struct usb_otg *otg;
>>>> +	int ret = 0;
>>>> +
>>>> +	if (!dev || !config || !config->fsm_ops)
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	/* already in list? */
>>>> +	mutex_lock(&otg_list_mutex);
>>>> +	if (usb_otg_get_data(dev)) {
>>>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>>>> +			__func__);
>>>> +		ret = -EINVAL;
>>>> +		goto unlock;
>>>> +	}
>>>> +
>>>> +	/* allocate and add to list */
>>>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>> +	if (!otg) {
>>>> +		ret = -ENOMEM;
>>>> +		goto unlock;
>>>> +	}
>>>> +
>>>> +	otg->dev = dev;
>>>> +	/* otg->caps is controller caps + DT overrides */
>>>> +	otg->caps = *config->otg_caps;
>>>> +	ret = of_usb_update_otg_caps(dev->of_node, &otg->caps);
>>>> +	if (ret)
>>>> +		goto err_wq;
>>>> +
>>>> +	if ((otg->caps.hnp_support || otg->caps.srp_support ||
>>>> +	     otg->caps.adp_support) && !config->otg_work) {
>>>> +		dev_err(dev,
>>>> +			"otg: otg_work must be provided for OTG support\n");
>>>> +		ret = -EINVAL;
>>>> +		goto err_wq;
>>>> +	}
>>>> +
>>>> +	if (config->otg_work)	/* custom otg_work ? */
>>>> +		INIT_WORK(&otg->work, config->otg_work);
>>>> +	else
>>>> +		INIT_WORK(&otg->work, usb_drd_work);
>>>
>>> why do you need to cope with custom work handlers?
>>
>> It was just a provision to provide your own state machine if the generic
>> one does not meet your needs. But i'm OK to get rid of it as well.
> 
> If you allow for this, every time there is a limitation, people will
> just provide a copy of the state machine with a small change here and
> there instead of fixing the real issue.

I agree with you here. I'll get rid of the custom_otg_work.

> 
>>>> +static void usb_otg_start_fsm(struct usb_otg *otg)
>>>> +{
>>>> +	struct otg_fsm *fsm = &otg->fsm;
>>>> +
>>>> +	if (fsm->running)
>>>> +		goto kick_fsm;
>>>> +
>>>> +	if (!otg->host) {
>>>> +		dev_info(otg->dev, "otg: can't start till host registers\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (!otg->gadget) {
>>>> +		dev_info(otg->dev,
>>>> +			 "otg: can't start till gadget UDC registers\n");
>>>> +		return;
>>>> +	}
>>>
>>> okay, so you never kick the FSM until host and gadget are
>>> registered. Why do you need to test for the case where the FSM is
>>> running without host/gadget?
>>
>> That message in the test was misleading. It could also be a
>> used as a warning if users did something wrong.
> 
> this usb_otg_start_fsm() establishes a contract. That contract says that
> the USB OTG FSM won't start until host and gadget are running and
> registered, yada yada yada. Drivers trying to kicking the FSM without
> calling usb_otg_start_fsm() first deserve to oops.

I'm considering the worst case where OTG controller, host controller and gadget controller
are 3 independent entities which can get probed in any order.

OTG controller driver doesn't really know when host and gadget register.
All it cares about is getting the hardware events and kicking the OTG machine.

(NOTE: when I say OTG controller it might as well be just the dual-role bits
that handle the ID and VBUS interrupts).

usb_otg_start_fsm() is not public.
usb_otg_sync_inputs() is the public function that the OTG driver will use.

> 
>>>> +MODULE_LICENSE("GPL");
>>>
>>> GPL or GPL 2-only?
>>
>> GPL v2.
>>
>>>
>>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>>> index f4fc0aa..1d74fb8 100644
>>>> --- a/include/linux/usb/gadget.h
>>>> +++ b/include/linux/usb/gadget.h
>>>> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>>>>   * @in_epnum: last used in ep number
>>>>   * @mA: last set mA value
>>>>   * @otg_caps: OTG capabilities of this gadget.
>>>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>>>
>>> do you really know of any platform which has a separate OTG controller?
>>>
>>
>> Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
>> and gadget.
>>
>> [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> 
> that's not an OTG controller, it's just a mux. No different than Intel's
> mux for swapping between XHCI and peripheral-only DWC3.
> 
> frankly, I would NEVER talk about OTG when type-C comes into play. They
> are two competing standards and, apparently, type-C is winning when it
> comes to role-swapping.
> 

Good to know.

cheers,
-roger

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

#1426561

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

Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>>>>> index 8689dcb..ed596ec 100644
>>>>> --- a/drivers/usb/Kconfig
>>>>> +++ b/drivers/usb/Kconfig
>>>>> @@ -32,6 +32,23 @@ if USB_SUPPORT
>>>>>  config USB_COMMON
>>>>>  	tristate
>>>>>  
>>>>> +config USB_OTG_CORE
>>>>> +	tristate
>>>>
>>>> why tristate if you can never set it to 'M'?
>>>
>>> This gets internally set to M if either USB or GADGET is M.
>>> We select it in USB and GADGET.
>>> This was the only way I could get usb-otg.c to build as
>>>
>>> m if USB OR GADGET is m
>>> built-in if USB and GADGET are built in.
>> 
>> I could only see a "select USB_OTG_CORE", select will always set it 'y'
>> and disregard dependencies. Maybe I missed something else.
>
> Not always. See how USB_COMMON works.

USB_COMMON is always 'y'. That could be changes a bool as well.

Do you have any defconfig where USB_COMMON or USB_OTG_CORE gets set to
'm'?

>>>>> +static DEFINE_MUTEX(otg_list_mutex);
>>>>> +
>>>>> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
>>>>> +{
>>>>> +	if (!hcd->primary_hcd)
>>>>> +		return 1;
>>>>
>>>> these seems inverted. If hcd->primary is NULL (meaning, there's no
>>>> ->primary_hcd), then we tell caller that this _is_ a primary hcd? Care
>>>> to explain?
>>>
>>> hcd->primary_hcd is a link used by the shared hcd to point to the
>>> primary_hcd.  primary_hcd's have this link as NULL.
>> 
>> So the following check is unnecessary and should always evaluate to
>> false, right ?
>
> Actually primary_hcd's not having a shared HCD have hcd->primary_hcd as NULL
> and those having a shared HCD do have it pointing to the primary hcd.

But look at your check:

is_primary(struct usb_hcd *hcd)
{
	if (!hcd->primary_hcd)
        	return true;

	return hcd == hcd->primary_hcd;
}

if you're passing a primary hcd, you're gonna return on the first
branch. If you're passing a secondary hcd, then your equality will
always be false. 

IOW, this can be reduced to:

is_primary(struct usb_hcd *hcd)
{
	return !hcd->primary_hcd;
}

right?

>>>>> +int usb_otg_start_host(struct usb_otg *otg, int on)
>>>>> +{
>>>>> +	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
>>>>> +	int ret;
>>>>> +
>>>>> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>>>>> +	if (!otg->host) {
>>>>> +		WARN_ONCE(1, "otg: fsm running without host\n");
>>>>
>>>> if (WARN_ONCE(!otg->host, "otg: fsm running without host\n"))
>>>> 	return 0;
>>>>
>>>> but, frankly, if you require a 'host' and a 'gadget' don't start this
>>>> layer until you have both.
>>>
>>> We don't start the layer till we have both host and gadget. But
>>> this API is for external use and might be called at any time.
>> 
>> well, if callers call this at the wrong time, it's callers' fault. Let
>> them oops so we catch the error.
>
> So you suggest we allow a NULL pointer dereference here?

yes, it's a clear violation of the API contract. The only situation
where this would ever trigger, is if somebody is calling
usb_otg_start_host() without calling start_fsm() first. That shouldn't
be valid.

>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>> +	if (on) {
>>>>> +		if (otg->flags & OTG_FLAG_HOST_RUNNING)
>>>>> +			return 0;
>>>>> +
>>>>> +		/* start host */
>>>>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
>>>>> +				   otg->primary_hcd.irqnum,
>>>>> +				   otg->primary_hcd.irqflags);
>>>>
>>>> this is usb_add_hcd(), is it not? Why add an indirection?
>>>
>>> I've introduced the host and gadget ops interface to get around the
>>> circular dependency issue we can't avoid.
>>> otg needs to call host/gadget functions and host/gadget also needs to
>>> call otg functions.
>> 
>> IMO, this shows a fragility of your design. You're, now, lying to
>> usb_hcd and usb_udc and making them register into a virtual layer that
>> doesn't exist. And that layer will end up calling the real registration
>> function when some magic event happens.
>> 
>> This is only really needed for quirky devices like dwc3 (but see more on
>> dwc3 below) where host and peripheral registers shadow each
>> other. Otherwise we would be able to always keep hcd and udc always
>> registered. They would get different interrupt statuses anyway and
>> nothing would ever break.
>
> Well I only had the opportunity to work with dwc3 so I had to ensure
> the design worked with it.

but this is exactly what I'm pointing you to. DWC3 does not need to go
through this because the HW maintains state machine for you.

>> However, when it comes to dwc3, we already have all the code necessary
>> to workaround this issue by destroying the XHCI pdev when OTG interrupt
>> says we should be peripheral (and vice-versa). DWC3 also keeps track of
>> the OTG states for those folks who really care about OTG (Hint: nobody
>> has cared for the past 10 years, why would they do so now?) and we don't
>> need a SW state machine when the HW handles that for us, right?
>
> Where is the code? I'd like to test dual-role on TI platforms.

Well, we just need an OTG IRQ handler to call dwc3_gadget_suspend() (or
that function renamed to match the usage) and something similar for the
host side.

It's all doable in a day or two.

>>> why? The kick could be triggered from an interrupt
>>> context. e.g. otg_irq.
>> 
>> We have threaded IRQ handlers in the kernel, right? Make use of that
>> and, with a little smart locking and IRQ masking, you can run the OTG
>> IRQ thread almost completely lockless ;-)
>
> Not a problem if we have the constraint that usb_otg_sync_inputs()
> needs to be called in thread context only.

that should be the case, right? If you're registering/unregistering
devices, you can't possibly call this from hardirq context.

>>>>> +	if (config->otg_work)	/* custom otg_work ? */
>>>>> +		INIT_WORK(&otg->work, config->otg_work);
>>>>> +	else
>>>>> +		INIT_WORK(&otg->work, usb_drd_work);
>>>>
>>>> why do you need to cope with custom work handlers?
>>>
>>> It was just a provision to provide your own state machine if the generic
>>> one does not meet your needs. But i'm OK to get rid of it as well.
>> 
>> If you allow for this, every time there is a limitation, people will
>> just provide a copy of the state machine with a small change here and
>> there instead of fixing the real issue.
>
> I agree with you here. I'll get rid of the custom_otg_work.

thanks

>>>>> +static void usb_otg_start_fsm(struct usb_otg *otg)
>>>>> +{
>>>>> +	struct otg_fsm *fsm = &otg->fsm;
>>>>> +
>>>>> +	if (fsm->running)
>>>>> +		goto kick_fsm;
>>>>> +
>>>>> +	if (!otg->host) {
>>>>> +		dev_info(otg->dev, "otg: can't start till host registers\n");
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	if (!otg->gadget) {
>>>>> +		dev_info(otg->dev,
>>>>> +			 "otg: can't start till gadget UDC registers\n");
>>>>> +		return;
>>>>> +	}
>>>>
>>>> okay, so you never kick the FSM until host and gadget are
>>>> registered. Why do you need to test for the case where the FSM is
>>>> running without host/gadget?
>>>
>>> That message in the test was misleading. It could also be a
>>> used as a warning if users did something wrong.
>> 
>> this usb_otg_start_fsm() establishes a contract. That contract says that
>> the USB OTG FSM won't start until host and gadget are running and
>> registered, yada yada yada. Drivers trying to kicking the FSM without
>> calling usb_otg_start_fsm() first deserve to oops.
>
> I'm considering the worst case where OTG controller, host controller
> and gadget controller are 3 independent entities which can get probed
> in any order.

there is no such thing as OTG controller :-) Even in our wildest dreams,
the most we get is a multiplexer inside the SoC to mux signals to HCD or
UDC. DWC3, when configured as a dual-role-capable IP, has its own OTG
block. But that's all self-contained inside DWC3 itself :-)

> OTG controller driver doesn't really know when host and gadget
> register.  All it cares about is getting the hardware events and
> kicking the OTG machine.

Nothing should be kicking the OTG state machine anyways, until all parts
are ready, registered, running, etc.

> (NOTE: when I say OTG controller it might as well be just the
> dual-role bits that handle the ID and VBUS interrupts).

right

> usb_otg_start_fsm() is not public.
> usb_otg_sync_inputs() is the public function that the OTG driver will use.

the outcome is the same, right?

-- 
balbi

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

#1426550

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

Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:
>>> It provides APIs for the following tasks
>>>
>>> - Registering an OTG/dual-role capable controller
>>> - Registering Host and Gadget controllers to OTG core
>>> - Providing inputs to and kicking the OTG state machine
>> 
>> I think I have already mentioned this, but after over 10 years of OTG,
>> nobody seems to care about it, why are we still touching at all I don't
>> know. For common non-OTG role-swapping we really don't need any of this
>> and, quite frankly, I fail to see enough users for this.
>> 
>> Apparently there's only chipidea which, AFAICT, already had working
>> dual-role before this OTG State Machine was added to the kernel.
>> 
>>> Provide a dual-role device (DRD) state machine.
>> 
>> there's not such thing as DRD state machine. You don't need to go
>> through all these states, actually.
>
> There are 3 states though.
> HOST (id = 0)
> PERIPHERAL (id = 1, vbus = 1)
> IDLE (id = 1, vbus = 0).

IDLE is pretty much given, though ;-)

>>> DRD mode is a reduced functionality OTG mode. In this mode
>>> we don't support SRP, HNP and dynamic role-swap.
>>>
>>> In DRD operation, the controller mode (Host or Peripheral)
>>> is decided based on the ID pin status. Once a cable plug (Type-A
>>> or Type-B) is attached the controller selects the state
>>> and doesn't change till the cable in unplugged and a different
>>> cable type is inserted.
>>>
>>> As we don't need most of the complex OTG states and OTG timers
>>> we implement a lean DRD state machine in usb-otg.c.
>>> The DRD state machine is only interested in 2 hardware inputs
>>> 'id' and 'b_sess_vld'.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>> v11:
>>> - remove usb_otg_kick_fsm().
>>> - typo fixes: structa/structure, upto/up to.
>>> - remove "obj-$(CONFIG_USB_OTG_CORE)     += common/" from Makefile.
>>>
>>>  drivers/usb/Kconfig          |  18 +
>>>  drivers/usb/common/Makefile  |   6 +-
>>>  drivers/usb/common/usb-otg.c | 877 +++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/usb/core/Kconfig     |  14 -
>>>  drivers/usb/gadget/Kconfig   |   1 +
>>>  include/linux/usb/gadget.h   |   2 +
>>>  include/linux/usb/hcd.h      |   1 +
>>>  include/linux/usb/otg-fsm.h  |   7 +
>>>  include/linux/usb/otg.h      | 174 ++++++++-
>>>  9 files changed, 1070 insertions(+), 30 deletions(-)
>>>  create mode 100644 drivers/usb/common/usb-otg.c
>>>
>>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>>> index 8689dcb..ed596ec 100644
>>> --- a/drivers/usb/Kconfig
>>> +++ b/drivers/usb/Kconfig
>>> @@ -32,6 +32,23 @@ if USB_SUPPORT
>>>  config USB_COMMON
>>>  	tristate
>>>  
>>> +config USB_OTG_CORE
>>> +	tristate
>> 
>> why tristate if you can never set it to 'M'?
>
> This gets internally set to M if either USB or GADGET is M.
> We select it in USB and GADGET.
> This was the only way I could get usb-otg.c to build as
>
> m if USB OR GADGET is m
> built-in if USB and GADGET are built in.

I could only see a "select USB_OTG_CORE", select will always set it 'y'
and disregard dependencies. Maybe I missed something else.

>>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>>> new file mode 100644
>>> index 0000000..a23ab1e
>>> --- /dev/null
>>> +++ b/drivers/usb/common/usb-otg.c
>>> @@ -0,0 +1,877 @@
>>> +/**
>>> + * drivers/usb/common/usb-otg.c - USB OTG core
>>> + *
>>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
>>> + * Author: 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 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.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/list.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/usb/of.h>
>>> +#include <linux/usb/otg.h>
>>> +#include <linux/usb/gadget.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +/* OTG device list */
>>> +LIST_HEAD(otg_list);
>> 
>> not static? Who needs to touch this private list?
>
> right, it should be static.
>
>> 
>>> +static DEFINE_MUTEX(otg_list_mutex);
>>> +
>>> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
>>> +{
>>> +	if (!hcd->primary_hcd)
>>> +		return 1;
>> 
>> these seems inverted. If hcd->primary is NULL (meaning, there's no
>> ->primary_hcd), then we tell caller that this _is_ a primary hcd? Care
>> to explain?
>
> hcd->primary_hcd is a link used by the shared hcd to point to the
> primary_hcd.  primary_hcd's have this link as NULL.

So the following check is unnecessary and should always evaluate to
false, right ?

>>> +	return hcd == hcd->primary_hcd;
>>> +}
>>> +
>>> +/**
>>> + * usb_otg_get_data() - get usb_otg data structure
>>> + * @otg_dev:	OTG controller device
>>> + *
>>> + * Check if the OTG device is in our OTG list and return
>>> + * usb_otg data, else NULL.
>>> + *
>>> + * otg_list_mutex must be held.
>>> + *
>>> + * Return: usb_otg data on success, NULL otherwise.
>>> + */
>>> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev)
>>> +{
>>> +	struct usb_otg *otg;
>>> +
>>> +	if (!otg_dev)
>>> +		return NULL;
>>> +
>>> +	list_for_each_entry(otg, &otg_list, list) {
>>> +		if (otg->dev == otg_dev)
>>> +			return otg;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +/**
>>> + * usb_otg_start_host() - start/stop the host controller
>>> + * @otg:	usb_otg instance
>>> + * @on:		true to start, false to stop
>>> + *
>>> + * Start/stop the USB host controller. This function is meant
>>> + * for use by the OTG controller driver.
>>> + *
>>> + * Return: 0 on success, error value otherwise.
>>> + */
>>> +int usb_otg_start_host(struct usb_otg *otg, int on)
>>> +{
>>> +	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
>>> +	int ret;
>>> +
>>> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>>> +	if (!otg->host) {
>>> +		WARN_ONCE(1, "otg: fsm running without host\n");
>> 
>> if (WARN_ONCE(!otg->host, "otg: fsm running without host\n"))
>> 	return 0;
>> 
>> but, frankly, if you require a 'host' and a 'gadget' don't start this
>> layer until you have both.
>
> We don't start the layer till we have both host and gadget. But
> this API is for external use and might be called at any time.

well, if callers call this at the wrong time, it's callers' fault. Let
them oops so we catch the error.

> I could change the warning message to
>  
> if (WARN_ONCE(!otg->host, "otg: %s called in invalid context\n", __func__))
> 	return 0;

you don't neet that.

>>> +		return 0;
>>> +	}
>>> +
>>> +	if (on) {
>>> +		if (otg->flags & OTG_FLAG_HOST_RUNNING)
>>> +			return 0;
>>> +
>>> +		/* start host */
>>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
>>> +				   otg->primary_hcd.irqnum,
>>> +				   otg->primary_hcd.irqflags);
>> 
>> this is usb_add_hcd(), is it not? Why add an indirection?
>
> I've introduced the host and gadget ops interface to get around the
> circular dependency issue we can't avoid.
> otg needs to call host/gadget functions and host/gadget also needs to
> call otg functions.

IMO, this shows a fragility of your design. You're, now, lying to
usb_hcd and usb_udc and making them register into a virtual layer that
doesn't exist. And that layer will end up calling the real registration
function when some magic event happens.

This is only really needed for quirky devices like dwc3 (but see more on
dwc3 below) where host and peripheral registers shadow each
other. Otherwise we would be able to always keep hcd and udc always
registered. They would get different interrupt statuses anyway and
nothing would ever break.

However, when it comes to dwc3, we already have all the code necessary
to workaround this issue by destroying the XHCI pdev when OTG interrupt
says we should be peripheral (and vice-versa). DWC3 also keeps track of
the OTG states for those folks who really care about OTG (Hint: nobody
has cared for the past 10 years, why would they do so now?) and we don't
need a SW state machine when the HW handles that for us, right?

As for chipidea, IIRC, that doesn't need a SW state machine either, but
I know very little about that IP and don't even have documentation on
it. My understanding, however, is that chipidea behaves kinda like MUSB,
which changes roles automatically in HW based on ID pin state.

>>> +EXPORT_SYMBOL_GPL(drd_statemachine);
>> 
>> why is this exported at all? It only runs from the work_struct
>
> You're right. It shouldn't be exported.
>
>> below. BTW, that work_struct looks unnecessary.
>
> why? The kick could be triggered from an interrupt
> context. e.g. otg_irq.

We have threaded IRQ handlers in the kernel, right? Make use of that
and, with a little smart locking and IRQ masking, you can run the OTG
IRQ thread almost completely lockless ;-)

>>> +/**
>>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>>> + * @dev: OTG/dual-role controller device.
>>> + * @config: OTG configuration.
>>> + *
>>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>>> + *
>>> + * Return: struct usb_otg * if success, ERR_PTR() otherwise.
>>> + */
>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>> +				 struct usb_otg_config *config)
>>> +{
>>> +	struct usb_otg *otg;
>>> +	int ret = 0;
>>> +
>>> +	if (!dev || !config || !config->fsm_ops)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	/* already in list? */
>>> +	mutex_lock(&otg_list_mutex);
>>> +	if (usb_otg_get_data(dev)) {
>>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>>> +			__func__);
>>> +		ret = -EINVAL;
>>> +		goto unlock;
>>> +	}
>>> +
>>> +	/* allocate and add to list */
>>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>> +	if (!otg) {
>>> +		ret = -ENOMEM;
>>> +		goto unlock;
>>> +	}
>>> +
>>> +	otg->dev = dev;
>>> +	/* otg->caps is controller caps + DT overrides */
>>> +	otg->caps = *config->otg_caps;
>>> +	ret = of_usb_update_otg_caps(dev->of_node, &otg->caps);
>>> +	if (ret)
>>> +		goto err_wq;
>>> +
>>> +	if ((otg->caps.hnp_support || otg->caps.srp_support ||
>>> +	     otg->caps.adp_support) && !config->otg_work) {
>>> +		dev_err(dev,
>>> +			"otg: otg_work must be provided for OTG support\n");
>>> +		ret = -EINVAL;
>>> +		goto err_wq;
>>> +	}
>>> +
>>> +	if (config->otg_work)	/* custom otg_work ? */
>>> +		INIT_WORK(&otg->work, config->otg_work);
>>> +	else
>>> +		INIT_WORK(&otg->work, usb_drd_work);
>> 
>> why do you need to cope with custom work handlers?
>
> It was just a provision to provide your own state machine if the generic
> one does not meet your needs. But i'm OK to get rid of it as well.

If you allow for this, every time there is a limitation, people will
just provide a copy of the state machine with a small change here and
there instead of fixing the real issue.

>>> +static void usb_otg_start_fsm(struct usb_otg *otg)
>>> +{
>>> +	struct otg_fsm *fsm = &otg->fsm;
>>> +
>>> +	if (fsm->running)
>>> +		goto kick_fsm;
>>> +
>>> +	if (!otg->host) {
>>> +		dev_info(otg->dev, "otg: can't start till host registers\n");
>>> +		return;
>>> +	}
>>> +
>>> +	if (!otg->gadget) {
>>> +		dev_info(otg->dev,
>>> +			 "otg: can't start till gadget UDC registers\n");
>>> +		return;
>>> +	}
>> 
>> okay, so you never kick the FSM until host and gadget are
>> registered. Why do you need to test for the case where the FSM is
>> running without host/gadget?
>
> That message in the test was misleading. It could also be a
> used as a warning if users did something wrong.

this usb_otg_start_fsm() establishes a contract. That contract says that
the USB OTG FSM won't start until host and gadget are running and
registered, yada yada yada. Drivers trying to kicking the FSM without
calling usb_otg_start_fsm() first deserve to oops.

>>> +MODULE_LICENSE("GPL");
>> 
>> GPL or GPL 2-only?
>
> GPL v2.
>
>> 
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index f4fc0aa..1d74fb8 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>>>   * @in_epnum: last used in ep number
>>>   * @mA: last set mA value
>>>   * @otg_caps: OTG capabilities of this gadget.
>>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> 
>> do you really know of any platform which has a separate OTG controller?
>> 
>
> Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> and gadget.
>
> [1] http://article.gmane.org/gmane.linux.ports.tegra/22969

that's not an OTG controller, it's just a mux. No different than Intel's
mux for swapping between XHCI and peripheral-only DWC3.

frankly, I would NEVER talk about OTG when type-C comes into play. They
are two competing standards and, apparently, type-C is winning when it
comes to role-swapping.

-- 
balbi

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

#1427355

On Mon, Jun 20, 2016 at 03:03:37PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> >>> +
> >>> +		/* start host */
> >>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
> >>> +				   otg->primary_hcd.irqnum,
> >>> +				   otg->primary_hcd.irqflags);
> >> 
> >> this is usb_add_hcd(), is it not? Why add an indirection?
> >
> > I've introduced the host and gadget ops interface to get around the
> > circular dependency issue we can't avoid.
> > otg needs to call host/gadget functions and host/gadget also needs to
> > call otg functions.
> 
> IMO, this shows a fragility of your design. You're, now, lying to
> usb_hcd and usb_udc and making them register into a virtual layer that
> doesn't exist. And that layer will end up calling the real registration
> function when some magic event happens.
> 
> This is only really needed for quirky devices like dwc3 (but see more on
> dwc3 below) where host and peripheral registers shadow each
> other. Otherwise we would be able to always keep hcd and udc always
> registered. They would get different interrupt statuses anyway and
> nothing would ever break.
> 
> However, when it comes to dwc3, we already have all the code necessary
> to workaround this issue by destroying the XHCI pdev when OTG interrupt
> says we should be peripheral (and vice-versa). DWC3 also keeps track of
> the OTG states for those folks who really care about OTG (Hint: nobody
> has cared for the past 10 years, why would they do so now?) and we don't
> need a SW state machine when the HW handles that for us, right?
> 
> As for chipidea, IIRC, that doesn't need a SW state machine either, but
> I know very little about that IP and don't even have documentation on
> it. My understanding, however, is that chipidea behaves kinda like MUSB,
> which changes roles automatically in HW based on ID pin state.

Chipidea needs to set register for USB role manually.

> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >> 
> >> do you really know of any platform which has a separate OTG controller?
> >> 
> >
> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> > and gadget.
> >
> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> 
> that's not an OTG controller, it's just a mux. No different than Intel's
> mux for swapping between XHCI and peripheral-only DWC3.
> 
> frankly, I would NEVER talk about OTG when type-C comes into play. They
> are two competing standards and, apparently, type-C is winning when it
> comes to role-swapping.
> 

In fact, OTG is mis-used by people. Currently, if the port is dual-role,
It will be considered as an OTG port.

You are right, if the connector is type-c, it will be called as "type-c
port" by people :)

-- 

Best Regards,
Peter Chen

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

#1427394

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> >>> +
>> >>> +		/* start host */
>> >>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
>> >>> +				   otg->primary_hcd.irqnum,
>> >>> +				   otg->primary_hcd.irqflags);
>> >> 
>> >> this is usb_add_hcd(), is it not? Why add an indirection?
>> >
>> > I've introduced the host and gadget ops interface to get around the
>> > circular dependency issue we can't avoid.
>> > otg needs to call host/gadget functions and host/gadget also needs to
>> > call otg functions.
>> 
>> IMO, this shows a fragility of your design. You're, now, lying to
>> usb_hcd and usb_udc and making them register into a virtual layer that
>> doesn't exist. And that layer will end up calling the real registration
>> function when some magic event happens.
>> 
>> This is only really needed for quirky devices like dwc3 (but see more on
>> dwc3 below) where host and peripheral registers shadow each
>> other. Otherwise we would be able to always keep hcd and udc always
>> registered. They would get different interrupt statuses anyway and
>> nothing would ever break.
>> 
>> However, when it comes to dwc3, we already have all the code necessary
>> to workaround this issue by destroying the XHCI pdev when OTG interrupt
>> says we should be peripheral (and vice-versa). DWC3 also keeps track of
>> the OTG states for those folks who really care about OTG (Hint: nobody
>> has cared for the past 10 years, why would they do so now?) and we don't
>> need a SW state machine when the HW handles that for us, right?
>> 
>> As for chipidea, IIRC, that doesn't need a SW state machine either, but
>> I know very little about that IP and don't even have documentation on
>> it. My understanding, however, is that chipidea behaves kinda like MUSB,
>> which changes roles automatically in HW based on ID pin state.
>
> Chipidea needs to set register for USB role manually.

okay, so chipidea has private control of role. Much like dwc3. That's good.

>> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >> 
>> >> do you really know of any platform which has a separate OTG controller?
>> >> 
>> >
>> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
>> > and gadget.
>> >
>> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> 
>> that's not an OTG controller, it's just a mux. No different than Intel's
>> mux for swapping between XHCI and peripheral-only DWC3.
>> 
>> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> are two competing standards and, apparently, type-C is winning when it
>> comes to role-swapping.
>> 
>
> In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> It will be considered as an OTG port.

That's because "dual-role" is a non-standard OTG. Seen as people really
didn't care about OTG, we (linux-usb folks) ended up naturally referring
to "non-standard OTG" as "dual-role". Just to avoid confusion.

> You are right, if the connector is type-c, it will be called as "type-c
> port" by people :)

oh no, that's not what I'm talking about. If you read Type-C and PD
specs, they define their own method for data role swapping. USB OTG
doesn't fit on top of a Type-C environment. It's not about what people
will call it, it's really that OTG can't work on top of type-c. For
starters, there's no ID pin ;-)

-- 
balbi

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

#1427441

On Tue, Jun 21, 2016 at 10:19:32AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> >>> +
> >> >>> +		/* start host */
> >> >>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
> >> >>> +				   otg->primary_hcd.irqnum,
> >> >>> +				   otg->primary_hcd.irqflags);
> >> >> 
> >> >> this is usb_add_hcd(), is it not? Why add an indirection?
> >> >
> >> > I've introduced the host and gadget ops interface to get around the
> >> > circular dependency issue we can't avoid.
> >> > otg needs to call host/gadget functions and host/gadget also needs to
> >> > call otg functions.
> >> 
> >> IMO, this shows a fragility of your design. You're, now, lying to
> >> usb_hcd and usb_udc and making them register into a virtual layer that
> >> doesn't exist. And that layer will end up calling the real registration
> >> function when some magic event happens.
> >> 
> >> This is only really needed for quirky devices like dwc3 (but see more on
> >> dwc3 below) where host and peripheral registers shadow each
> >> other. Otherwise we would be able to always keep hcd and udc always
> >> registered. They would get different interrupt statuses anyway and
> >> nothing would ever break.
> >> 
> >> However, when it comes to dwc3, we already have all the code necessary
> >> to workaround this issue by destroying the XHCI pdev when OTG interrupt
> >> says we should be peripheral (and vice-versa). DWC3 also keeps track of
> >> the OTG states for those folks who really care about OTG (Hint: nobody
> >> has cared for the past 10 years, why would they do so now?) and we don't
> >> need a SW state machine when the HW handles that for us, right?
> >> 
> >> As for chipidea, IIRC, that doesn't need a SW state machine either, but
> >> I know very little about that IP and don't even have documentation on
> >> it. My understanding, however, is that chipidea behaves kinda like MUSB,
> >> which changes roles automatically in HW based on ID pin state.
> >
> > Chipidea needs to set register for USB role manually.
> 
> okay, so chipidea has private control of role. Much like dwc3. That's good.
> 
> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >> >> 
> >> >> do you really know of any platform which has a separate OTG controller?
> >> >> 
> >> >
> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> >> > and gadget.
> >> >
> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> 
> >> that's not an OTG controller, it's just a mux. No different than Intel's
> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> 
> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> >> are two competing standards and, apparently, type-C is winning when it
> >> comes to role-swapping.
> >> 
> >
> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> > It will be considered as an OTG port.
> 
> That's because "dual-role" is a non-standard OTG. Seen as people really
> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> to "non-standard OTG" as "dual-role". Just to avoid confusion.

So, unless we use OTG FSM defined in OTG spec, we should not mention
"OTG" in Linux, right?
 
> 
> > You are right, if the connector is type-c, it will be called as "type-c
> > port" by people :)
> 
> oh no, that's not what I'm talking about. If you read Type-C and PD
> specs, they define their own method for data role swapping. USB OTG
> doesn't fit on top of a Type-C environment. It's not about what people
> will call it, it's really that OTG can't work on top of type-c. For
> starters, there's no ID pin ;-)

I know type-c, yes, there is no relationship between OTG and type-c.

-- 

Best Regards,
Peter Chen

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

#1427446

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> Peter Chen <hzpeterchen@gmail.com> writes:
>> >> >>> +
>> >> >>> +		/* start host */
>> >> >>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
>> >> >>> +				   otg->primary_hcd.irqnum,
>> >> >>> +				   otg->primary_hcd.irqflags);
>> >> >> 
>> >> >> this is usb_add_hcd(), is it not? Why add an indirection?
>> >> >
>> >> > I've introduced the host and gadget ops interface to get around the
>> >> > circular dependency issue we can't avoid.
>> >> > otg needs to call host/gadget functions and host/gadget also needs to
>> >> > call otg functions.
>> >> 
>> >> IMO, this shows a fragility of your design. You're, now, lying to
>> >> usb_hcd and usb_udc and making them register into a virtual layer that
>> >> doesn't exist. And that layer will end up calling the real registration
>> >> function when some magic event happens.
>> >> 
>> >> This is only really needed for quirky devices like dwc3 (but see more on
>> >> dwc3 below) where host and peripheral registers shadow each
>> >> other. Otherwise we would be able to always keep hcd and udc always
>> >> registered. They would get different interrupt statuses anyway and
>> >> nothing would ever break.
>> >> 
>> >> However, when it comes to dwc3, we already have all the code necessary
>> >> to workaround this issue by destroying the XHCI pdev when OTG interrupt
>> >> says we should be peripheral (and vice-versa). DWC3 also keeps track of
>> >> the OTG states for those folks who really care about OTG (Hint: nobody
>> >> has cared for the past 10 years, why would they do so now?) and we don't
>> >> need a SW state machine when the HW handles that for us, right?
>> >> 
>> >> As for chipidea, IIRC, that doesn't need a SW state machine either, but
>> >> I know very little about that IP and don't even have documentation on
>> >> it. My understanding, however, is that chipidea behaves kinda like MUSB,
>> >> which changes roles automatically in HW based on ID pin state.
>> >
>> > Chipidea needs to set register for USB role manually.
>> 
>> okay, so chipidea has private control of role. Much like dwc3. That's good.
>> 
>> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >> >> 
>> >> >> do you really know of any platform which has a separate OTG controller?
>> >> >> 
>> >> >
>> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
>> >> > and gadget.
>> >> >
>> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> >> 
>> >> that's not an OTG controller, it's just a mux. No different than Intel's
>> >> mux for swapping between XHCI and peripheral-only DWC3.
>> >> 
>> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> >> are two competing standards and, apparently, type-C is winning when it
>> >> comes to role-swapping.
>> >> 
>> >
>> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
>> > It will be considered as an OTG port.
>> 
>> That's because "dual-role" is a non-standard OTG. Seen as people really
>> didn't care about OTG, we (linux-usb folks) ended up naturally referring
>> to "non-standard OTG" as "dual-role". Just to avoid confusion.
>
> So, unless we use OTG FSM defined in OTG spec, we should not mention
> "OTG" in Linux, right?

to avoid confusion with the terminology, yes. With that settled, let's
figure out how you can deliver what your marketting guys are asking of
you.

>> > You are right, if the connector is type-c, it will be called as "type-c
>> > port" by people :)
>> 
>> oh no, that's not what I'm talking about. If you read Type-C and PD
>> specs, they define their own method for data role swapping. USB OTG
>> doesn't fit on top of a Type-C environment. It's not about what people
>> will call it, it's really that OTG can't work on top of type-c. For
>> starters, there's no ID pin ;-)
>
> I know type-c, yes, there is no relationship between OTG and type-c.

okay, thanks

-- 
balbi

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

#1427673

On Tue, Jun 21, 2016 at 11:18:21AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> Peter Chen <hzpeterchen@gmail.com> writes:
> >> >> >>> +
> >> >> >>> +		/* start host */
> >> >> >>> +		ret = hcd_ops->add(otg->primary_hcd.hcd,
> >> >> >>> +				   otg->primary_hcd.irqnum,
> >> >> >>> +				   otg->primary_hcd.irqflags);
> >> >> >> 
> >> >> >> this is usb_add_hcd(), is it not? Why add an indirection?
> >> >> >
> >> >> > I've introduced the host and gadget ops interface to get around the
> >> >> > circular dependency issue we can't avoid.
> >> >> > otg needs to call host/gadget functions and host/gadget also needs to
> >> >> > call otg functions.
> >> >> 
> >> >> IMO, this shows a fragility of your design. You're, now, lying to
> >> >> usb_hcd and usb_udc and making them register into a virtual layer that
> >> >> doesn't exist. And that layer will end up calling the real registration
> >> >> function when some magic event happens.
> >> >> 
> >> >> This is only really needed for quirky devices like dwc3 (but see more on
> >> >> dwc3 below) where host and peripheral registers shadow each
> >> >> other. Otherwise we would be able to always keep hcd and udc always
> >> >> registered. They would get different interrupt statuses anyway and
> >> >> nothing would ever break.
> >> >> 
> >> >> However, when it comes to dwc3, we already have all the code necessary
> >> >> to workaround this issue by destroying the XHCI pdev when OTG interrupt
> >> >> says we should be peripheral (and vice-versa). DWC3 also keeps track of
> >> >> the OTG states for those folks who really care about OTG (Hint: nobody
> >> >> has cared for the past 10 years, why would they do so now?) and we don't
> >> >> need a SW state machine when the HW handles that for us, right?
> >> >> 
> >> >> As for chipidea, IIRC, that doesn't need a SW state machine either, but
> >> >> I know very little about that IP and don't even have documentation on
> >> >> it. My understanding, however, is that chipidea behaves kinda like MUSB,
> >> >> which changes roles automatically in HW based on ID pin state.
> >> >
> >> > Chipidea needs to set register for USB role manually.
> >> 
> >> okay, so chipidea has private control of role. Much like dwc3. That's good.
> >> 
> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >> >> >> 
> >> >> >> do you really know of any platform which has a separate OTG controller?
> >> >> >> 
> >> >> >
> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> >> >> > and gadget.
> >> >> >
> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> >> 
> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> >> 
> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> >> >> are two competing standards and, apparently, type-C is winning when it
> >> >> comes to role-swapping.
> >> >> 
> >> >
> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> >> > It will be considered as an OTG port.
> >> 
> >> That's because "dual-role" is a non-standard OTG. Seen as people really
> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> >
> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> > "OTG" in Linux, right?
> 
> to avoid confusion with the terminology, yes. With that settled, let's
> figure out how you can deliver what your marketting guys are asking of
> you.
> 

Since nxp SoC claims they are OTG compliance, we need to pass usb.org
test. The internal bsp has passed PET test, and formal compliance test
is on the way (should pass too). 

The dual-role and OTG compliance use the same zImage, but different
dtb.

-- 

Best Regards,
Peter Chen

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

#1427704

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >> >> >> 
>> >> >> >> do you really know of any platform which has a separate OTG controller?
>> >> >> >> 
>> >> >> >
>> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
>> >> >> > and gadget.
>> >> >> >
>> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> >> >> 
>> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
>> >> >> mux for swapping between XHCI and peripheral-only DWC3.
>> >> >> 
>> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> >> >> are two competing standards and, apparently, type-C is winning when it
>> >> >> comes to role-swapping.
>> >> >> 
>> >> >
>> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
>> >> > It will be considered as an OTG port.
>> >> 
>> >> That's because "dual-role" is a non-standard OTG. Seen as people really
>> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
>> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
>> >
>> > So, unless we use OTG FSM defined in OTG spec, we should not mention
>> > "OTG" in Linux, right?
>> 
>> to avoid confusion with the terminology, yes. With that settled, let's
>> figure out how you can deliver what your marketting guys are asking of
>> you.
>> 
>
> Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> test. The internal bsp has passed PET test, and formal compliance test
> is on the way (should pass too). 
>
> The dual-role and OTG compliance use the same zImage, but different
> dtb.

okay, that's good to know. Now, the question really is: considering we
only have one user for this generic OTG FSM layer, do we really need to
make it generic at all? I mean, just look at how invasive a change that
is.

My fear is that, as stated before, we don't have enough variance to be
able to design something that many could use. On top of that, most folks
are moving to type-c connector which, in reality, can't really implement
OTG.

Considering that Apple/Intel have already announced [1] that they will
use type-c connector, it's not too farfetched to speculate that CarPlay
will, eventually, rely on Power Delivery for role swapping. IOW, OTG has
its days counted. In 2 years' time, the market will have moved on to
Type-C and the generic OTG layer will be left to bit rot as time goes
by.

This is why I think that these changes should be local to chipidea,
considering chipidea is the only user for them. As for dwc3, we can get
something much simpler since, at least so far, there's no full OTG
requirement from anywhere I know and, even if OTG becomes a requirement
for any of dwc3 users, the HW handles the state machine for us.

What do you think?

[1] https://thunderbolttechnology.net/blog/thunderbolt-3-usb-c-does-it-all

-- 
balbi

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

#1427744

On Tue, Jun 21, 2016 at 03:35:00PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >> >> >> >> 
> >> >> >> >> do you really know of any platform which has a separate OTG controller?
> >> >> >> >> 
> >> >> >> >
> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> >> >> >> > and gadget.
> >> >> >> >
> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> >> >> 
> >> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> >> >> 
> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> >> >> >> are two competing standards and, apparently, type-C is winning when it
> >> >> >> comes to role-swapping.
> >> >> >> 
> >> >> >
> >> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> >> >> > It will be considered as an OTG port.
> >> >> 
> >> >> That's because "dual-role" is a non-standard OTG. Seen as people really
> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> >> >
> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> >> > "OTG" in Linux, right?
> >> 
> >> to avoid confusion with the terminology, yes. With that settled, let's
> >> figure out how you can deliver what your marketting guys are asking of
> >> you.
> >> 
> >
> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> > test. The internal bsp has passed PET test, and formal compliance test
> > is on the way (should pass too). 
> >
> > The dual-role and OTG compliance use the same zImage, but different
> > dtb.
> 
> okay, that's good to know. Now, the question really is: considering we
> only have one user for this generic OTG FSM layer, do we really need to
> make it generic at all? I mean, just look at how invasive a change that
> is.
> 

If the chipidea is the only user for this roger's framework, I don't
think it is necessary. In fact, Roger introduces this framework, and
the first user is dwc3, we think it can be used for others. Let's
just discuss if it is necessary for dual-role switch.

-- 

Best Regards,
Peter Chen

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

#1427839

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
>> >> >> >> >> 
>> >> >> >> >> do you really know of any platform which has a separate OTG controller?
>> >> >> >> >> 
>> >> >> >> >
>> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
>> >> >> >> > and gadget.
>> >> >> >> >
>> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
>> >> >> >> 
>> >> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
>> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
>> >> >> >> 
>> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
>> >> >> >> are two competing standards and, apparently, type-C is winning when it
>> >> >> >> comes to role-swapping.
>> >> >> >> 
>> >> >> >
>> >> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
>> >> >> > It will be considered as an OTG port.
>> >> >> 
>> >> >> That's because "dual-role" is a non-standard OTG. Seen as people really
>> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
>> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
>> >> >
>> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
>> >> > "OTG" in Linux, right?
>> >> 
>> >> to avoid confusion with the terminology, yes. With that settled, let's
>> >> figure out how you can deliver what your marketting guys are asking of
>> >> you.
>> >> 
>> >
>> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
>> > test. The internal bsp has passed PET test, and formal compliance test
>> > is on the way (should pass too). 
>> >
>> > The dual-role and OTG compliance use the same zImage, but different
>> > dtb.
>> 
>> okay, that's good to know. Now, the question really is: considering we
>> only have one user for this generic OTG FSM layer, do we really need to
>> make it generic at all? I mean, just look at how invasive a change that
>> is.
>
> If the chipidea is the only user for this roger's framework, I don't
> think it is necessary. In fact, Roger introduces this framework, and
> the first user is dwc3, we think it can be used for others. Let's

Right, we need to look at the history of dwc3 to figure out why the
conclusion that dwc3 needs this was made.

Roger started working on this framework when Power on Reset section of
databook had some details which weren't always clear and, for safety, we
always had reset asserted for a really long time. It was so long (about
400 ms) that resetting dwc3 for each role swap was just too much.

Coupled with that, the OTG chapter wasn't very clear either on
expections from Host and Peripheral side initialization in OTG/DRD
systems.

More recent version of dwc3 databook have a much better description of
how and which reset bits _must_ be asserted and which shouldn't be
touched unless it's for debugging purposes. When I implemented that, our
->probe() went from 400ms down to about 50us.

Coupled with that, the OTG chapter also became a lot clearer to the
point that it states you just don't initialize anything other than the
OTG block, and just wait for OTG interrupt to do whatever it is you need
to do.

This meant that we could actually afford to do full reinitialization of
dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
roles properly.

(The reason for needing soft-reset during role swap is kinda long. But
in summary dwc3 shadows register writes to both host and peripheral
sides)

> just discuss if it is necessary for dual-role switch.

fair. However, if we have a single user we don't have a Generic
layer. There's not enough variance to come up with truly generic
architecture for this.

-- 
balbi

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

#1428393

On Tue, Jun 21, 2016 at 05:47:47PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >> >> >> >> >> 
> >> >> >> >> >> do you really know of any platform which has a separate OTG controller?
> >> >> >> >> >> 
> >> >> >> >> >
> >> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> >> >> >> >> > and gadget.
> >> >> >> >> >
> >> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> >> >> >> >> 
> >> >> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
> >> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> >> >> >> >> 
> >> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> >> >> >> >> are two competing standards and, apparently, type-C is winning when it
> >> >> >> >> comes to role-swapping.
> >> >> >> >> 
> >> >> >> >
> >> >> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> >> >> >> > It will be considered as an OTG port.
> >> >> >> 
> >> >> >> That's because "dual-role" is a non-standard OTG. Seen as people really
> >> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> >> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> >> >> >
> >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> >> >> > "OTG" in Linux, right?
> >> >> 
> >> >> to avoid confusion with the terminology, yes. With that settled, let's
> >> >> figure out how you can deliver what your marketting guys are asking of
> >> >> you.
> >> >> 
> >> >
> >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> >> > test. The internal bsp has passed PET test, and formal compliance test
> >> > is on the way (should pass too). 
> >> >
> >> > The dual-role and OTG compliance use the same zImage, but different
> >> > dtb.
> >> 
> >> okay, that's good to know. Now, the question really is: considering we
> >> only have one user for this generic OTG FSM layer, do we really need to
> >> make it generic at all? I mean, just look at how invasive a change that
> >> is.
> >
> > If the chipidea is the only user for this roger's framework, I don't
> > think it is necessary. In fact, Roger introduces this framework, and
> > the first user is dwc3, we think it can be used for others. Let's
> 
> Right, we need to look at the history of dwc3 to figure out why the
> conclusion that dwc3 needs this was made.
> 
> Roger started working on this framework when Power on Reset section of
> databook had some details which weren't always clear and, for safety, we
> always had reset asserted for a really long time. It was so long (about
> 400 ms) that resetting dwc3 for each role swap was just too much.
> 
> Coupled with that, the OTG chapter wasn't very clear either on
> expections from Host and Peripheral side initialization in OTG/DRD
> systems.
> 
> More recent version of dwc3 databook have a much better description of
> how and which reset bits _must_ be asserted and which shouldn't be
> touched unless it's for debugging purposes. When I implemented that, our
> ->probe() went from 400ms down to about 50us.
> 
> Coupled with that, the OTG chapter also became a lot clearer to the
> point that it states you just don't initialize anything other than the
> OTG block, and just wait for OTG interrupt to do whatever it is you need
> to do.
> 
> This meant that we could actually afford to do full reinitialization of
> dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
> roles properly.
> 
> (The reason for needing soft-reset during role swap is kinda long. But
> in summary dwc3 shadows register writes to both host and peripheral
> sides)
> 
> > just discuss if it is necessary for dual-role switch.
> 
> fair. However, if we have a single user we don't have a Generic
> layer. There's not enough variance to come up with truly generic
> architecture for this.
> 
> -- 

I have put some points in my last reply [1], I summery it here to
see if a generic framework is deserved or not?

1. If there are some parts we can use during the role switch
- The common start/stop host and peripheral operation
eg, when switch from host to peripheral, all drivers can use
usb_remove_hcd to finish it.
- A common workqueue to handle vbus and id event
- sysfs for role switch

2. Does a mux driver can do it well? Yoshihiro, here we need your
point. The main point is if we need to call USB API to change
roles (eg, usb_remove_hcd) during the role switch, thanks.


[1] http://www.spinics.net/lists/linux-usb/msg142974.html
-- 

Best Regards,
Peter Chen

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

#1428478

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
>> >> >> > "OTG" in Linux, right?
>> >> >> 
>> >> >> to avoid confusion with the terminology, yes. With that settled, let's
>> >> >> figure out how you can deliver what your marketting guys are asking of
>> >> >> you.
>> >> >> 
>> >> >
>> >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
>> >> > test. The internal bsp has passed PET test, and formal compliance test
>> >> > is on the way (should pass too). 
>> >> >
>> >> > The dual-role and OTG compliance use the same zImage, but different
>> >> > dtb.
>> >> 
>> >> okay, that's good to know. Now, the question really is: considering we
>> >> only have one user for this generic OTG FSM layer, do we really need to
>> >> make it generic at all? I mean, just look at how invasive a change that
>> >> is.
>> >
>> > If the chipidea is the only user for this roger's framework, I don't
>> > think it is necessary. In fact, Roger introduces this framework, and
>> > the first user is dwc3, we think it can be used for others. Let's
>> 
>> Right, we need to look at the history of dwc3 to figure out why the
>> conclusion that dwc3 needs this was made.
>> 
>> Roger started working on this framework when Power on Reset section of
>> databook had some details which weren't always clear and, for safety, we
>> always had reset asserted for a really long time. It was so long (about
>> 400 ms) that resetting dwc3 for each role swap was just too much.
>> 
>> Coupled with that, the OTG chapter wasn't very clear either on
>> expections from Host and Peripheral side initialization in OTG/DRD
>> systems.
>> 
>> More recent version of dwc3 databook have a much better description of
>> how and which reset bits _must_ be asserted and which shouldn't be
>> touched unless it's for debugging purposes. When I implemented that, our
>> ->probe() went from 400ms down to about 50us.
>> 
>> Coupled with that, the OTG chapter also became a lot clearer to the
>> point that it states you just don't initialize anything other than the
>> OTG block, and just wait for OTG interrupt to do whatever it is you need
>> to do.
>> 
>> This meant that we could actually afford to do full reinitialization of
>> dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
>> roles properly.
>> 
>> (The reason for needing soft-reset during role swap is kinda long. But
>> in summary dwc3 shadows register writes to both host and peripheral
>> sides)
>> 
>> > just discuss if it is necessary for dual-role switch.
>> 
>> fair. However, if we have a single user we don't have a Generic
>> layer. There's not enough variance to come up with truly generic
>> architecture for this.
>> 
>> -- 
>
> I have put some points in my last reply [1], I summery it here to
> see if a generic framework is deserved or not?
>
> 1. If there are some parts we can use during the role switch
> - The common start/stop host and peripheral operation
> eg, when switch from host to peripheral, all drivers can use
> usb_remove_hcd to finish it.

a UDC such as dwc3 already implements start/stop for peripheral and
host. Why would go through and indirection layer that just comes back to
us? (well, dwc3's host side, start/stop translates to adding/removing
xhci-plat's device)

> - A common workqueue to handle vbus and id event

I already have a threaded IRQ handler. Why do I need a workqueue?

> - sysfs for role switch

A generic sysfs is desirable, but I really don't know where to put it.
Maybe it's enough to go down the hwmon route and just have an agreement
of filename and contents to be written to.

-- 
balbi

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

#1428501

On Wed, Jun 22, 2016 at 09:51:19AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> >> >> >> > "OTG" in Linux, right?
> >> >> >> 
> >> >> >> to avoid confusion with the terminology, yes. With that settled, let's
> >> >> >> figure out how you can deliver what your marketting guys are asking of
> >> >> >> you.
> >> >> >> 
> >> >> >
> >> >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> >> >> > test. The internal bsp has passed PET test, and formal compliance test
> >> >> > is on the way (should pass too). 
> >> >> >
> >> >> > The dual-role and OTG compliance use the same zImage, but different
> >> >> > dtb.
> >> >> 
> >> >> okay, that's good to know. Now, the question really is: considering we
> >> >> only have one user for this generic OTG FSM layer, do we really need to
> >> >> make it generic at all? I mean, just look at how invasive a change that
> >> >> is.
> >> >
> >> > If the chipidea is the only user for this roger's framework, I don't
> >> > think it is necessary. In fact, Roger introduces this framework, and
> >> > the first user is dwc3, we think it can be used for others. Let's
> >> 
> >> Right, we need to look at the history of dwc3 to figure out why the
> >> conclusion that dwc3 needs this was made.
> >> 
> >> Roger started working on this framework when Power on Reset section of
> >> databook had some details which weren't always clear and, for safety, we
> >> always had reset asserted for a really long time. It was so long (about
> >> 400 ms) that resetting dwc3 for each role swap was just too much.
> >> 
> >> Coupled with that, the OTG chapter wasn't very clear either on
> >> expections from Host and Peripheral side initialization in OTG/DRD
> >> systems.
> >> 
> >> More recent version of dwc3 databook have a much better description of
> >> how and which reset bits _must_ be asserted and which shouldn't be
> >> touched unless it's for debugging purposes. When I implemented that, our
> >> ->probe() went from 400ms down to about 50us.
> >> 
> >> Coupled with that, the OTG chapter also became a lot clearer to the
> >> point that it states you just don't initialize anything other than the
> >> OTG block, and just wait for OTG interrupt to do whatever it is you need
> >> to do.
> >> 
> >> This meant that we could actually afford to do full reinitialization of
> >> dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
> >> roles properly.
> >> 
> >> (The reason for needing soft-reset during role swap is kinda long. But
> >> in summary dwc3 shadows register writes to both host and peripheral
> >> sides)
> >> 
> >> > just discuss if it is necessary for dual-role switch.
> >> 
> >> fair. However, if we have a single user we don't have a Generic
> >> layer. There's not enough variance to come up with truly generic
> >> architecture for this.
> >> 
> >> -- 
> >
> > I have put some points in my last reply [1], I summery it here to
> > see if a generic framework is deserved or not?
> >
> > 1. If there are some parts we can use during the role switch
> > - The common start/stop host and peripheral operation
> > eg, when switch from host to peripheral, all drivers can use
> > usb_remove_hcd to finish it.
> 
> a UDC such as dwc3 already implements start/stop for peripheral and
> host. Why would go through and indirection layer that just comes back to
> us? (well, dwc3's host side, start/stop translates to adding/removing
> xhci-plat's device)
> 
> > - A common workqueue to handle vbus and id event
> 
> I already have a threaded IRQ handler. Why do I need a workqueue?
> 

I know it can be done in individual driver, don't you think
we need a common part to manage the dual-role switch process,
since dual-role switch is used more and more common, and
there are so many switch methods:

- ID pin
- sysfs
- type-c
- OTG FSM
- Registers

Maybe Roger's framework is a little complicated, but if it is the
correct direction, we can improve it.
  
> > - sysfs for role switch
> 
> A generic sysfs is desirable, but I really don't know where to put it.
> Maybe it's enough to go down the hwmon route and just have an agreement
> of filename and contents to be written to.
> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

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

#1428532

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> >> >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
>> >> >> >> > "OTG" in Linux, right?
>> >> >> >> 
>> >> >> >> to avoid confusion with the terminology, yes. With that settled, let's
>> >> >> >> figure out how you can deliver what your marketting guys are asking of
>> >> >> >> you.
>> >> >> >> 
>> >> >> >
>> >> >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
>> >> >> > test. The internal bsp has passed PET test, and formal compliance test
>> >> >> > is on the way (should pass too). 
>> >> >> >
>> >> >> > The dual-role and OTG compliance use the same zImage, but different
>> >> >> > dtb.
>> >> >> 
>> >> >> okay, that's good to know. Now, the question really is: considering we
>> >> >> only have one user for this generic OTG FSM layer, do we really need to
>> >> >> make it generic at all? I mean, just look at how invasive a change that
>> >> >> is.
>> >> >
>> >> > If the chipidea is the only user for this roger's framework, I don't
>> >> > think it is necessary. In fact, Roger introduces this framework, and
>> >> > the first user is dwc3, we think it can be used for others. Let's
>> >> 
>> >> Right, we need to look at the history of dwc3 to figure out why the
>> >> conclusion that dwc3 needs this was made.
>> >> 
>> >> Roger started working on this framework when Power on Reset section of
>> >> databook had some details which weren't always clear and, for safety, we
>> >> always had reset asserted for a really long time. It was so long (about
>> >> 400 ms) that resetting dwc3 for each role swap was just too much.
>> >> 
>> >> Coupled with that, the OTG chapter wasn't very clear either on
>> >> expections from Host and Peripheral side initialization in OTG/DRD
>> >> systems.
>> >> 
>> >> More recent version of dwc3 databook have a much better description of
>> >> how and which reset bits _must_ be asserted and which shouldn't be
>> >> touched unless it's for debugging purposes. When I implemented that, our
>> >> ->probe() went from 400ms down to about 50us.
>> >> 
>> >> Coupled with that, the OTG chapter also became a lot clearer to the
>> >> point that it states you just don't initialize anything other than the
>> >> OTG block, and just wait for OTG interrupt to do whatever it is you need
>> >> to do.
>> >> 
>> >> This meant that we could actually afford to do full reinitialization of
>> >> dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
>> >> roles properly.
>> >> 
>> >> (The reason for needing soft-reset during role swap is kinda long. But
>> >> in summary dwc3 shadows register writes to both host and peripheral
>> >> sides)
>> >> 
>> >> > just discuss if it is necessary for dual-role switch.
>> >> 
>> >> fair. However, if we have a single user we don't have a Generic
>> >> layer. There's not enough variance to come up with truly generic
>> >> architecture for this.
>> >> 
>> >> -- 
>> >
>> > I have put some points in my last reply [1], I summery it here to
>> > see if a generic framework is deserved or not?
>> >
>> > 1. If there are some parts we can use during the role switch
>> > - The common start/stop host and peripheral operation
>> > eg, when switch from host to peripheral, all drivers can use
>> > usb_remove_hcd to finish it.
>> 
>> a UDC such as dwc3 already implements start/stop for peripheral and
>> host. Why would go through and indirection layer that just comes back to
>> us? (well, dwc3's host side, start/stop translates to adding/removing
>> xhci-plat's device)
>> 
>> > - A common workqueue to handle vbus and id event
>> 
>> I already have a threaded IRQ handler. Why do I need a workqueue?
>> 
>
> I know it can be done in individual driver, don't you think
> we need a common part to manage the dual-role switch process,

A common part will be a requirement when we have at least 3 users for
it. Right now there's only one. So how can this be common at all?

> since dual-role switch is used more and more common, and
> there are so many switch methods:
>
> - ID pin
> - sysfs
> - type-c
> - OTG FSM
> - Registers
>
> Maybe Roger's framework is a little complicated, but if it is the
> correct direction, we can improve it.

IMO, we don't have enough users

-- 
balbi

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

#1429541

Hi,

> From: Peter Chen
> Sent: Wednesday, June 22, 2016 12:34 PM
> 
> On Tue, Jun 21, 2016 at 05:47:47PM +0300, Felipe Balbi wrote:
> >
> > Hi,
> >
> > Peter Chen <hzpeterchen@gmail.com> writes:
> > >> >> >> >> >>> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> > >> >> >> >> >>
> > >> >> >> >> >> do you really know of any platform which has a separate OTG controller?
> > >> >> >> >> >>
> > >> >> >> >> >
> > >> >> >> >> > Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> > >> >> >> >> > and gadget.
> > >> >> >> >> >
> > >> >> >> >> > [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> > >> >> >> >>
> > >> >> >> >> that's not an OTG controller, it's just a mux. No different than Intel's
> > >> >> >> >> mux for swapping between XHCI and peripheral-only DWC3.
> > >> >> >> >>
> > >> >> >> >> frankly, I would NEVER talk about OTG when type-C comes into play. They
> > >> >> >> >> are two competing standards and, apparently, type-C is winning when it
> > >> >> >> >> comes to role-swapping.
> > >> >> >> >>
> > >> >> >> >
> > >> >> >> > In fact, OTG is mis-used by people. Currently, if the port is dual-role,
> > >> >> >> > It will be considered as an OTG port.
> > >> >> >>
> > >> >> >> That's because "dual-role" is a non-standard OTG. Seen as people really
> > >> >> >> didn't care about OTG, we (linux-usb folks) ended up naturally referring
> > >> >> >> to "non-standard OTG" as "dual-role". Just to avoid confusion.
> > >> >> >
> > >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> > >> >> > "OTG" in Linux, right?
> > >> >>
> > >> >> to avoid confusion with the terminology, yes. With that settled, let's
> > >> >> figure out how you can deliver what your marketting guys are asking of
> > >> >> you.
> > >> >>
> > >> >
> > >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> > >> > test. The internal bsp has passed PET test, and formal compliance test
> > >> > is on the way (should pass too).
> > >> >
> > >> > The dual-role and OTG compliance use the same zImage, but different
> > >> > dtb.
> > >>
> > >> okay, that's good to know. Now, the question really is: considering we
> > >> only have one user for this generic OTG FSM layer, do we really need to
> > >> make it generic at all? I mean, just look at how invasive a change that
> > >> is.
> > >
> > > If the chipidea is the only user for this roger's framework, I don't
> > > think it is necessary. In fact, Roger introduces this framework, and
> > > the first user is dwc3, we think it can be used for others. Let's
> >
> > Right, we need to look at the history of dwc3 to figure out why the
> > conclusion that dwc3 needs this was made.
> >
> > Roger started working on this framework when Power on Reset section of
> > databook had some details which weren't always clear and, for safety, we
> > always had reset asserted for a really long time. It was so long (about
> > 400 ms) that resetting dwc3 for each role swap was just too much.
> >
> > Coupled with that, the OTG chapter wasn't very clear either on
> > expections from Host and Peripheral side initialization in OTG/DRD
> > systems.
> >
> > More recent version of dwc3 databook have a much better description of
> > how and which reset bits _must_ be asserted and which shouldn't be
> > touched unless it's for debugging purposes. When I implemented that, our
> > ->probe() went from 400ms down to about 50us.
> >
> > Coupled with that, the OTG chapter also became a lot clearer to the
> > point that it states you just don't initialize anything other than the
> > OTG block, and just wait for OTG interrupt to do whatever it is you need
> > to do.
> >
> > This meant that we could actually afford to do full reinitialization of
> > dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
> > roles properly.
> >
> > (The reason for needing soft-reset during role swap is kinda long. But
> > in summary dwc3 shadows register writes to both host and peripheral
> > sides)
> >
> > > just discuss if it is necessary for dual-role switch.
> >
> > fair. However, if we have a single user we don't have a Generic
> > layer. There's not enough variance to come up with truly generic
> > architecture for this.
> >
> > --
> 
> I have put some points in my last reply [1], I summery it here to
> see if a generic framework is deserved or not?
> 
> 1. If there are some parts we can use during the role switch
> - The common start/stop host and peripheral operation
> eg, when switch from host to peripheral, all drivers can use
> usb_remove_hcd to finish it.
> - A common workqueue to handle vbus and id event
> - sysfs for role switch
> 
> 2. Does a mux driver can do it well? Yoshihiro, here we need your
> point. The main point is if we need to call USB API to change
> roles (eg, usb_remove_hcd) during the role switch, thanks.

In my platform, it doesn't need to call USB API (usb_remove_hcd) when
"A-host" is changed to "A-peripheral".
(Since this is a prototype local code, the code is not upstreaming yet though.)

Best regards,
Yoshihiro Shimoda

> 
> [1] http://www.spinics.net/lists/linux-usb/msg142974.html
> --
> 
> Best Regards,
> Peter Chen

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

#1427219

Hi Roger,

> From: Roger Quadros
> Sent: Monday, June 20, 2016 7:13 PM
> 
> Hi,
> 
> On 20/06/16 10:45, Felipe Balbi wrote:
< snip >
> >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >> index f4fc0aa..1d74fb8 100644
> >> --- a/include/linux/usb/gadget.h
> >> +++ b/include/linux/usb/gadget.h
> >> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
> >>   * @in_epnum: last used in ep number
> >>   * @mA: last set mA value
> >>   * @otg_caps: OTG capabilities of this gadget.
> >> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> >
> > do you really know of any platform which has a separate OTG controller?
> >
> 
> Andrew had pointed out in [1] that Tegra210 has separate blocks for OTG, host
> and gadget.
> 
> [1] http://article.gmane.org/gmane.linux.ports.tegra/22969
> 
> Yoshihiro,
> 
> How is the dual-role architecture on your Renesas platform?

About the dual-role architecture, Renesas platform (R-Car H3) has a USB 2.0 host controller (EHCI/OHCI)
with OTG function and a separate USB 2.0 peripheral controller (HS-USB).
The OTG function is related to some PHY control registers, so I intend to add the OTG/Dual-role core
support into the phy driver (drivers/phy/phy-rcar-gen3-usb2.c).

Best regards,
Yoshihiro Shimoda

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web