LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC][PATCH 0/9] USB: OTG Core functionality
@ 2015-03-18 13:55 Roger Quadros
  2015-03-18 13:55 ` [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd() Roger Quadros
                   ` (9 more replies)
  0 siblings, 10 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:55 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

Hi,

[NOTE: RFC only. Not for merge yet.]

This is an attempt to centralize OTG functionality in the kernel.
Still work in progress but I wanted to get an early feedback
to avoid major rework. :) 

Why?:
----

Most of the OTG drivers have been dealing with the OTG state machine
themselves and there is a scope for code re-use. This has been
partly addressed by the usb/common/usb-otg-fsm.c but it still
leaves the instantiation of the state machine and OTG timers
to the controller drivers. We re-use usb-otg-fsm.c but
go one step further by instantiating the state machine and timers
thus making it easier for drivers to implement OTG functionality.

Newer OTG cores support standard host interface (e.g. xHCI?) so
host and gadget functionality are no longer closely knit like older
cores. There needs to be a way to co-ordinate the operation of the
host and gadget in OTG mode. i.e. to stop and start them from a
central location. This central location should be the USB OTG core.

Host and gadget controllers might be sharing resources and can't
be always running. One has to be stopped for the other to run.
This can't be done as of now and can be done from the OTG core.

What?:
-----

The OTG core instantiates the OTG Finite State Machine
per OTG controller and manages starting/stopping the
host and gadget controllers based on the bus state.
    
It provides APIs for the following
    
- Registering an OTG capable controller
struct otg_fsm *usb_otg_register(struct device *parent_dev,
                                 struct otg_fsm_ops *fsm_ops);
int usb_otg_unregister(struct device *parent_dev);

- Registering Host controllers to OTG core (used by hcd-core)
int usb_otg_register_hcd(struct usb_hcd *hcd);
int usb_otg_unregister_hcd(struct usb_hcd *hcd);

- Registering Gadget controllers to OTG core (used by udc-core)
int usb_otg_register_gadget(struct usb_gadget *gadget);
int usb_otg_unregister_gadget(struct usb_gadget *gadget);

- Providing inputs to and kicking the OTG state machine
void usb_otg_sync_inputs(struct otg_fsm *fsm);
int usb_otg_kick_fsm(struct device *hcd_gcd_device);

'struct otg_fsm' is the interface to the OTG state machine.
It contains inputs to the fsm, status of the fsm and operations
for the OTG controller driver.

Usage model:
-----------

- The OTG controller device is assumed to be the parent of
the host and gadget controller. It must call usb_otg_register()
before populating the host and gadget devices so that the OTG
core is aware that it is an OTG device before the host & gadget
register. The OTG controller must provide struct otg_fsm_ops *
which will be called by the OTG core depending on OTG bus state.

- The host/gadget core stacks are modified to inform the OTG core
whenever a new host/gadget device is added. The OTG core then
checks if the host/gadget is part of the OTG controller and if yes
then prevents the host/gadget from starting till both host and
gadget are registered, OTG state machine is running and the
USB bus state is appropriate to start host/gadget.
 For this APIs have been added to host/gadget stacks to start/stop
the controllers from the OTG core.

- No modification is needed for the host/gadget controller drivers.
They must ensure that their start/stop methods can be called repeatedly
and any shared resources between host & gadget are properly managed.
The OTG core ensures that both are not started simultaneously.

- The OTG core instantiates one OTG state machine per OTG
controller and the necessary OTG timers to manage OTG state timeouts.
The state machine is started when both host & gadget register and
stopped when either of them unregisters. The controllers are started
and stopped depending on bus state.

- During the lifetime of the OTG state machine, inputs can be
provided to it by modifying the appropriate members of 'struct otg_fsm'
and calling usb_otg_sync_inputs(). This is typically done by the
OTG controller driver that called usb_otg_register() since it is
the only external component that has the 'struct otg_fsm' handle.

Pending items:
- We probably need a otg class.
- sysfs interface for application OTG inputs and OTG status information
- resolve symbol dependency for module use.
- otg driver for dwc3 core to get dual-role working on dra7-evm.

cheers,
-roger

Roger Quadros (9):
  usb: hcd: Introduce usb_start/stop_hcd()
  usb: gadget: add usb_gadget_start/stop()
  usb: otg: add OTG core
  usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable
  usb: hcd: adapt to OTG
  usb: gadget: udc: adapt to OTG
  usb: dwc3: adapt to OTG core
  usb: otg-fsm: Remove unused members in struct otg_fsm
  usb: otg-fsm: Add documentation for struct otg_fsm

 drivers/usb/Makefile              |   1 +
 drivers/usb/common/Makefile       |   1 +
 drivers/usb/common/usb-otg.c      | 732 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/common/usb-otg.h      |  71 ++++
 drivers/usb/core/Kconfig          |   8 +
 drivers/usb/core/hcd.c            | 153 +++++---
 drivers/usb/core/hub.c            |  11 +-
 drivers/usb/dwc3/core.c           | 101 ++++++
 drivers/usb/dwc3/core.h           |   6 +
 drivers/usb/dwc3/platform_data.h  |   1 +
 drivers/usb/gadget/udc/udc-core.c | 172 ++++++++-
 include/linux/usb/gadget.h        |   3 +
 include/linux/usb/hcd.h           |   2 +
 include/linux/usb/otg-fsm.h       |  81 ++++-
 include/linux/usb/usb-otg.h       |  86 +++++
 15 files changed, 1357 insertions(+), 72 deletions(-)
 create mode 100644 drivers/usb/common/usb-otg.c
 create mode 100644 drivers/usb/common/usb-otg.h
 create mode 100644 include/linux/usb/usb-otg.h

-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
@ 2015-03-18 13:55 ` Roger Quadros
  2015-03-18 19:49   ` Alan Stern
  2015-03-19  1:46   ` Peter Chen
  2015-03-18 13:55 ` [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop() Roger Quadros
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:55 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

To support OTG we want a mechanism to start and stop
the HCD from the OTG state machine. Add usb_start_hcd()
and usb_stop_hcd().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/core/hcd.c  | 147 ++++++++++++++++++++++++++++++++----------------
 include/linux/usb/hcd.h |   2 +
 2 files changed, 100 insertions(+), 49 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..e28bd9d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2613,7 +2613,76 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
 }
 
 /**
- * usb_add_hcd - finish generic HCD structure initialization and register
+ * usb_start_hcd - start the HCD
+ * @hcd: the usb_hcd structure to initialize
+ *
+ * calls the drivers start() routine, registers the root hub
+ * and creates the USB sysfs attributes.
+ */
+int usb_start_hcd(struct usb_hcd *hcd)
+{
+	int retval;
+	struct usb_device *rhdev = hcd->self.root_hub;
+
+	if (hcd->state != HC_STATE_HALT) {
+		dev_err(hcd->self.controller, "not starting a running HCD\n");
+		return -EINVAL;
+	}
+
+	hcd->state = HC_STATE_RUNNING;
+	retval = hcd->driver->start(hcd);
+	if (retval < 0) {
+		dev_err(hcd->self.controller, "startup error %d\n", retval);
+		hcd->state = HC_STATE_HALT;
+		return retval;
+	}
+
+	/* starting here, usbcore will pay attention to this root hub */
+	if ((retval = register_root_hub(hcd)) != 0)
+		goto err_register_root_hub;
+
+	retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
+	if (retval < 0) {
+		pr_err("Cannot register USB bus sysfs attributes: %d\n",
+		       retval);
+		goto error_create_attr_group;
+	}
+	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
+		usb_hcd_poll_rh_status(hcd);
+
+	return retval;
+
+error_create_attr_group:
+	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
+	if (HC_IS_RUNNING(hcd->state))
+		hcd->state = HC_STATE_QUIESCING;
+	spin_lock_irq(&hcd_root_hub_lock);
+	hcd->rh_registered = 0;
+	spin_unlock_irq(&hcd_root_hub_lock);
+
+#ifdef CONFIG_PM
+	cancel_work_sync(&hcd->wakeup_work);
+#endif
+	mutex_lock(&usb_bus_list_lock);
+	usb_disconnect(&rhdev);		/* Sets rhdev to NULL */
+	mutex_unlock(&usb_bus_list_lock);
+err_register_root_hub:
+	hcd->rh_pollable = 0;
+	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
+	del_timer_sync(&hcd->rh_timer);
+	hcd->driver->stop(hcd);
+	hcd->state = HC_STATE_HALT;
+
+	/* In case the HCD restarted the timer, stop it again. */
+	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
+	del_timer_sync(&hcd->rh_timer);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(usb_start_hcd);
+
+/**
+ * usb_add_hcd - finish generic HCD structure initialization and register.
  * @hcd: the usb_hcd structure to initialize
  * @irqnum: Interrupt line to allocate
  * @irqflags: Interrupt type flags
@@ -2757,50 +2826,12 @@ int usb_add_hcd(struct usb_hcd *hcd,
 			goto err_request_irq;
 	}
 
-	hcd->state = HC_STATE_RUNNING;
-	retval = hcd->driver->start(hcd);
-	if (retval < 0) {
-		dev_err(hcd->self.controller, "startup error %d\n", retval);
+	retval = usb_start_hcd(hcd);
+	if (retval)
 		goto err_hcd_driver_start;
-	}
-
-	/* starting here, usbcore will pay attention to this root hub */
-	if ((retval = register_root_hub(hcd)) != 0)
-		goto err_register_root_hub;
-
-	retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
-	if (retval < 0) {
-		printk(KERN_ERR "Cannot register USB bus sysfs attributes: %d\n",
-		       retval);
-		goto error_create_attr_group;
-	}
-	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
-		usb_hcd_poll_rh_status(hcd);
-
-	return retval;
 
-error_create_attr_group:
-	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
-	if (HC_IS_RUNNING(hcd->state))
-		hcd->state = HC_STATE_QUIESCING;
-	spin_lock_irq(&hcd_root_hub_lock);
-	hcd->rh_registered = 0;
-	spin_unlock_irq(&hcd_root_hub_lock);
+	return 0;
 
-#ifdef CONFIG_PM
-	cancel_work_sync(&hcd->wakeup_work);
-#endif
-	mutex_lock(&usb_bus_list_lock);
-	usb_disconnect(&rhdev);		/* Sets rhdev to NULL */
-	mutex_unlock(&usb_bus_list_lock);
-err_register_root_hub:
-	hcd->rh_pollable = 0;
-	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	del_timer_sync(&hcd->rh_timer);
-	hcd->driver->stop(hcd);
-	hcd->state = HC_STATE_HALT;
-	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	del_timer_sync(&hcd->rh_timer);
 err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
@@ -2830,18 +2861,20 @@ err_phy:
 EXPORT_SYMBOL_GPL(usb_add_hcd);
 
 /**
- * usb_remove_hcd - shutdown processing for generic HCDs
- * @hcd: the usb_hcd structure to remove
- * Context: !in_interrupt()
+ * usb_stop_hcd - stop the HCD
+ * @hcd: the usb_hcd structure to initialize
  *
- * Disconnects the root hub, then reverses the effects of usb_add_hcd(),
- * invoking the HCD's stop() method.
+ * removes the USB sysfs attributes, disconnects the root hub
+ * and calls the driver's stop() routine.
  */
-void usb_remove_hcd(struct usb_hcd *hcd)
+void usb_stop_hcd(struct usb_hcd *hcd)
 {
 	struct usb_device *rhdev = hcd->self.root_hub;
 
-	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
+	if (hcd->state == HC_STATE_HALT) {
+		dev_err(hcd->self.controller, "not stopping a halted HCD\n");
+		return;
+	}
 
 	usb_get_dev(rhdev);
 	sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
@@ -2888,6 +2921,22 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	/* In case the HCD restarted the timer, stop it again. */
 	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
 	del_timer_sync(&hcd->rh_timer);
+}
+EXPORT_SYMBOL_GPL(usb_stop_hcd);
+
+/**
+ * usb_remove_hcd - shutdown processing for generic HCDs
+ * @hcd: the usb_hcd structure to remove
+ * Context: !in_interrupt()
+ *
+ * Disconnects the root hub, then reverses the effects of usb_add_hcd(),
+ * invoking the HCD's stop() method.
+ */
+void usb_remove_hcd(struct usb_hcd *hcd)
+{
+	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
+
+	usb_stop_hcd(hcd);
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		if (hcd->irq > 0)
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 68b1e83..12aaf4c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -433,6 +433,8 @@ extern void usb_put_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd);
 extern int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags);
+extern int usb_start_hcd(struct usb_hcd *hcd);
+extern void usb_stop_hcd(struct usb_hcd *hcd);
 extern void usb_remove_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
 
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
  2015-03-18 13:55 ` [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd() Roger Quadros
@ 2015-03-18 13:55 ` Roger Quadros
  2015-03-19  3:30   ` Peter Chen
  2015-03-18 13:55 ` [RFC][PATCH 3/9] usb: otg: add OTG core Roger Quadros
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:55 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

The OTG state machine needs a mechanism to start and
stop the gadget controller. Add usb_gadget_start()
and usb_gadget_stop().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
 include/linux/usb/gadget.h        |   3 +
 2 files changed, 158 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 5a81cb0..69b4123 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -35,6 +35,8 @@
  * @dev - the child device to the actual controller
  * @gadget - the gadget. For use by the class code
  * @list - for use by the udc class driver
+ * @running - udc is running
+ * @softconnect - sysfs softconnect says OK to connect
  *
  * This represents the internal data structure which is used by the UDC-class
  * to hold information about udc driver and gadget together.
@@ -44,6 +46,8 @@ struct usb_udc {
 	struct usb_gadget		*gadget;
 	struct device			dev;
 	struct list_head		list;
+	bool				running;
+	bool				softconnect;
 };
 
 static struct class *udc_class;
@@ -187,7 +191,14 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
  */
 static inline int usb_gadget_udc_start(struct usb_udc *udc)
 {
-	return udc->gadget->ops->udc_start(udc->gadget, udc->driver);
+	int ret;
+
+	dev_dbg(&udc->dev, "%s\n", __func__);
+	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
+	if (!ret)
+		udc->running = 1;
+
+	return ret;
 }
 
 /**
@@ -204,10 +215,140 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
  */
 static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 {
+	dev_dbg(&udc->dev, "%s\n", __func__);
 	udc->gadget->ops->udc_stop(udc->gadget);
+	udc->running = 0;
 }
 
 /**
+ * usb_udc_start - Start the usb device controller only if conditions met
+ * @udc: The UDC to be started
+ *
+ * Start the UDC and connect to bus (enable pull) only if the
+ * following conditions are met
+ * - UDC is not already running
+ * - gadget function driver is loaded
+ * - userspace softconnect says OK to connect
+ *
+ * NOTE: udc_lock must be held by caller.
+ *
+ * Returs 0 on success or not ready to start, else negative errno.
+ */
+static int usb_udc_start(struct usb_udc *udc)
+{
+	int ret;
+
+	if (udc->running) {
+		dev_err(&udc->dev, "%s: not starting as already running\n",
+			__func__);
+		return -EBUSY;
+	}
+
+	if (udc->driver && udc->softconnect) {
+		ret = usb_gadget_udc_start(udc);
+		if (ret)
+			return ret;
+
+		usb_gadget_connect(udc->gadget);
+	}
+
+	return 0;
+}
+
+/**
+ * usb_udc_stop - Stop the usb device controller
+ * @udc: The UDC to be stopped
+ *
+ * Disconnect from bus (disable pull) and stop the UDC.
+ *
+ * NOTE: udc_lock must be held by caller.
+ *
+ * Returs 0 on success, else negative errno.
+ */
+static int usb_udc_stop(struct usb_udc *udc)
+{
+	if (!udc->running) {
+		dev_err(&udc->dev, "%s: not stopping as already halted\n",
+			__func__);
+		return -EBUSY;
+	}
+
+	usb_gadget_disconnect(udc->gadget);
+	udc->driver->disconnect(udc->gadget);
+	usb_gadget_udc_stop(udc);
+
+	return 0;
+}
+
+/**
+ * usb_gadget_start - start the usb gadget controller
+ * @gadget: the gadget device to start
+ *
+ * This is external API for use by OTG core.
+ *
+ * Start the usb device controller and connect to bus (enable pull).
+ * There is no guarantee that the controller is started
+ * as we might be missing some dependencies
+ * i.e. gadget function driver not loaded or softconnect disabled.
+ */
+int usb_gadget_start(struct usb_gadget *gadget)
+{
+	int ret;
+	struct usb_udc *udc = NULL;
+
+	dev_dbg(&gadget->dev, "%s\n", __func__);
+	mutex_lock(&udc_lock);
+	list_for_each_entry(udc, &udc_list, list)
+		if (udc->gadget == gadget)
+			goto found;
+
+	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+		__func__);
+	mutex_unlock(&udc_lock);
+	return -EINVAL;
+
+found:
+	ret = usb_udc_start(udc);
+	mutex_unlock(&udc_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_start);
+
+/**
+ * usb_gadget_stop - stop the usb gadget
+ * @gadget: The gadget device we want to stop
+ *
+ * This is external API for use by OTG core.
+ *
+ * Disconnect from the bus (disable pull) and stop the
+ * gadget controller.
+ */
+int usb_gadget_stop(struct usb_gadget *gadget)
+{
+	int ret;
+	struct usb_udc *udc = NULL;
+
+	dev_dbg(&gadget->dev, "%s\n", __func__);
+	mutex_lock(&udc_lock);
+	list_for_each_entry(udc, &udc_list, list)
+		if (udc->gadget == gadget)
+			goto found;
+
+	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+		__func__);
+	mutex_unlock(&udc_lock);
+	return -EINVAL;
+
+found:
+	ret = usb_udc_stop(udc);
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_stop);
+
+/**
  * usb_udc_release - release the usb_udc struct
  * @dev: the dev member within usb_udc
  *
@@ -278,6 +419,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 		goto err3;
 
 	udc->gadget = gadget;
+	udc->softconnect = 1;	/* connect by default */
 
 	mutex_lock(&udc_lock);
 	list_add_tail(&udc->list, &udc_list);
@@ -329,10 +471,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	usb_gadget_disconnect(udc->gadget);
-	udc->driver->disconnect(udc->gadget);
+	usb_udc_stop(udc);
 	udc->driver->unbind(udc->gadget);
-	usb_gadget_udc_stop(udc);
 
 	udc->driver = NULL;
 	udc->dev.driver = NULL;
@@ -378,6 +518,7 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 /* ------------------------------------------------------------------------- */
 
+/* udc_lock must be held */
 static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
 {
 	int ret;
@@ -392,12 +533,12 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
 		goto err1;
-	ret = usb_gadget_udc_start(udc);
+
+	ret = usb_udc_start(udc);
 	if (ret) {
 		driver->unbind(udc->gadget);
 		goto err1;
 	}
-	usb_gadget_connect(udc->gadget);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
@@ -510,12 +651,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
 	}
 
 	if (sysfs_streq(buf, "connect")) {
-		usb_gadget_udc_start(udc);
-		usb_gadget_connect(udc->gadget);
+		mutex_lock(&udc_lock);
+		udc->softconnect = 1;
+		usb_udc_start(udc);
+		mutex_unlock(&udc_lock);
 	} else if (sysfs_streq(buf, "disconnect")) {
-		usb_gadget_disconnect(udc->gadget);
-		udc->driver->disconnect(udc->gadget);
-		usb_gadget_udc_stop(udc);
+		mutex_lock(&udc_lock);
+		udc->softconnect = 0;
+		usb_udc_stop(udc);
+		mutex_unlock(&udc_lock);
 	} else {
 		dev_err(dev, "unsupported command '%s'\n", buf);
 		return -EINVAL;
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e2f00fd..7ada7e6 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -922,6 +922,9 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver);
  */
 int usb_gadget_unregister_driver(struct usb_gadget_driver *driver);
 
+int usb_gadget_start(struct usb_gadget *gadget);
+int usb_gadget_stop(struct usb_gadget *gadget);
+
 extern int usb_add_gadget_udc_release(struct device *parent,
 		struct usb_gadget *gadget, void (*release)(struct device *dev));
 extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
  2015-03-18 13:55 ` [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd() Roger Quadros
  2015-03-18 13:55 ` [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop() Roger Quadros
@ 2015-03-18 13:55 ` Roger Quadros
  2015-03-19  3:40   ` Peter Chen
  2015-03-19  8:26   ` Li Jun
  2015-03-18 13:55 ` [RFC][PATCH 4/9] usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros
                   ` (6 subsequent siblings)
  9 siblings, 2 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:55 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

The OTG core instantiates the OTG Finite State Machine
per OTG controller and manages starting/stopping the
host and gadget controllers based on the bus state.

It provides APIs for the following tasks

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

TODO:
- sysfs interface to allow application inputs to OTG state machine
- otg class?

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/Makefile         |   1 +
 drivers/usb/common/Makefile  |   1 +
 drivers/usb/common/usb-otg.c | 732 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/common/usb-otg.h |  71 +++++
 drivers/usb/core/Kconfig     |   8 +
 include/linux/usb/usb-otg.h  |  86 +++++
 6 files changed, 899 insertions(+)
 create mode 100644 drivers/usb/common/usb-otg.c
 create mode 100644 drivers/usb/common/usb-otg.h
 create mode 100644 include/linux/usb/usb-otg.h

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 2f1e2aa..07f59a5 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -60,5 +60,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
 obj-$(CONFIG_USB_GADGET)	+= gadget/
 
 obj-$(CONFIG_USB_COMMON)	+= common/
+obj-$(CONFIG_USB_OTG_CORE)	+= common/
 
 obj-$(CONFIG_USBIP_CORE)	+= usbip/
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index ca2f8bd..573fc75 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -7,3 +7,4 @@ usb-common-y			  += common.o
 usb-common-$(CONFIG_USB_LED_TRIG) += led.o
 
 obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
+obj-$(CONFIG_USB_OTG_CORE) += usb-otg.o
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
new file mode 100644
index 0000000..1433fc9
--- /dev/null
+++ b/drivers/usb/common/usb-otg.c
@@ -0,0 +1,732 @@
+/**
+ * drivers/usb/common/usb-otg.c - USB OTG core
+ *
+ * Copyright (C) 2015 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/timer.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/phy.h>	/* enum usb_otg_state */
+#include <linux/usb/gadget.h>
+#include <linux/usb/usb-otg.h>
+#include <linux/workqueue.h>
+
+#include "usb-otg.h"
+
+/* to link timer with callback data */
+struct otg_timer {
+	struct timer_list timer;
+	/* callback data */
+	int *timeout_bit;
+	struct otg_data *otgd;
+};
+
+struct otg_data {
+	struct device *dev;	/* HCD & GCD's parent device */
+
+	struct otg_fsm fsm;
+	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
+	 * HCD is bus_to_hcd(fsm->otg->host)
+	 * GCD is fsm->otg->gadget
+	 */
+	struct otg_fsm_ops fsm_ops;	/* private copy for override */
+	struct usb_otg otg;
+	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
+
+	/* saved hooks to OTG device */
+	int (*start_host)(struct otg_fsm *fsm, int on);
+	int (*start_gadget)(struct otg_fsm *fsm, int on);
+
+	struct list_head list;
+
+	struct work_struct work;	/* OTG FSM work */
+	struct workqueue_struct *wq;
+
+	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
+
+	bool fsm_running;
+	bool gadget_can_start;		/* OTG FSM says gadget can start */
+	bool host_can_start;		/* OTG FSM says host can start */
+
+	/* use otg->fsm.lock for serializing access */
+};
+
+/* OTG device list */
+LIST_HEAD(otg_list);
+static DEFINE_MUTEX(otg_list_mutex);
+
+/**
+ * check if device is in our OTG list and return
+ * otg_data, else NULL.
+ *
+ * otg_list_mutex must be held.
+ */
+static struct otg_data *usb_otg_device_get_otgd(struct device *parent_dev)
+{
+	struct otg_data *otgd;
+
+	list_for_each_entry(otgd, &otg_list, list) {
+		if (otgd->dev == parent_dev)
+			return otgd;
+	}
+
+	return NULL;
+}
+
+/**
+ * timer callback to set timeout bit and kick FSM
+ */
+static void set_tmout(unsigned long data)
+{
+	struct otg_timer *tmr_data;
+
+	tmr_data = (struct otg_timer *)data;
+
+	if (tmr_data->timeout_bit)
+		*tmr_data->timeout_bit = 1;
+
+	usb_otg_sync_inputs(&tmr_data->otgd->fsm);
+}
+
+/**
+ * Initialize one OTG timer with callback, timeout and timeout bit
+ */
+static void otg_timer_init(enum otg_fsm_timer id, struct otg_data *otgd,
+			   void (*callback)(unsigned long),
+			   unsigned long expires_ms,
+			   int *timeout_bit)
+{
+	struct otg_timer *otgtimer = &otgd->timers[id];
+	struct timer_list *timer = &otgtimer->timer;
+
+	init_timer(timer);
+	timer->function = callback;
+	timer->expires = jiffies + msecs_to_jiffies(expires_ms);
+	timer->data = (unsigned long)otgtimer;
+
+	otgtimer->timeout_bit = timeout_bit;
+	otgtimer->otgd = otgd;
+}
+
+/**
+ * Initialize standard OTG timers
+ */
+static void usb_otg_init_timers(struct otg_data *otgd)
+{
+	struct otg_fsm *fsm = &otgd->fsm;
+
+	otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE, &fsm->a_wait_vrise_tmout);
+	otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL, &fsm->a_wait_vfall_tmout);
+	otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON, &fsm->a_wait_bcon_tmout);
+	otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS, &fsm->a_aidl_bdis_tmout);
+	otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS, &fsm->a_bidl_adis_tmout);
+	otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST, &fsm->b_ase0_brst_tmout);
+
+	otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP, &fsm->b_se0_srp);
+	otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL, &fsm->b_srp_done);
+
+	otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
+}
+
+/**
+ * OTG FSM ops function to add timer
+ */
+static void usb_otg_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+	struct timer_list *timer = &otgd->timers[id].timer;
+
+	if (!otgd->fsm_running)
+		return;
+
+	/* if timer is already active, exit */
+	if (timer_pending(timer)) {
+		dev_err(otgd->dev, "USB-OTG: Timer %d is already running\n",
+			id);
+	}
+
+	add_timer(timer);
+}
+
+/**
+ * OTG FSM ops function to delete timer
+ */
+static void usb_otg_del_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+	struct timer_list *timer = &otgd->timers[id].timer;
+
+	del_timer(timer);
+}
+
+/**
+ * OTG FSM ops function to start/stop host
+ */
+static int usb_otg_start_host(struct otg_fsm *fsm, int on)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+	int ret = 0;
+	struct usb_hcd *hcd;
+
+	dev_info(otgd->dev, "%s %d\n", __func__, on);
+	if (!fsm->otg->host) {
+		WARN_ONCE(1, "OTG FSM running without Host\n");
+		return 0;
+	}
+
+	hcd = bus_to_hcd(fsm->otg->host);
+	otgd->host_can_start = on;
+	if (hcd->shared_hcd && !otgd->shared_hcd) {
+		dev_info(otgd->dev, "%s: waiting for Shared HCD to register\n",
+			 __func__);
+		return 0;
+	}
+
+	/* call OTG device operations */
+	if (otgd->start_host) {
+		ret = otgd->start_host(fsm, on);
+		if (ret)
+			return ret;
+	}
+
+	if (on) {
+		usb_start_hcd(hcd);
+		if (hcd->shared_hcd)
+			usb_start_hcd(hcd->shared_hcd);
+	} else {
+		if (hcd->shared_hcd)
+			usb_stop_hcd(hcd->shared_hcd);
+		usb_stop_hcd(hcd);
+	}
+
+	return 0;
+}
+
+/**
+ * OTG FSM ops function to start/stop gadget
+ */
+static int usb_otg_start_gadget(struct otg_fsm *fsm, int on)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+	int ret = 0;
+	struct usb_gadget *gadget = fsm->otg->gadget;
+
+	dev_info(otgd->dev, "%s %d\n", __func__, on);
+	if (!gadget) {
+		WARN_ONCE(1, "OTG FSM running without Gadget\n");
+		return 0;
+	}
+
+	/* call OTG device operations */
+	if (otgd->start_gadget) {
+		ret = otgd->start_gadget(fsm, on);
+		if (ret)
+			return ret;
+	}
+
+	otgd->gadget_can_start = on;
+	if (on)
+		usb_gadget_start(fsm->otg->gadget);
+	else
+		usb_gadget_stop(fsm->otg->gadget);
+
+	return 0;
+}
+
+/**
+ * OTG FSM work function
+ */
+static void usb_otg_work(struct work_struct *work)
+{
+	struct otg_data *otgd = container_of(work, struct otg_data, work);
+
+	if (otg_statemachine(&otgd->fsm)) {
+		/* state changed. any action ? */
+	}
+}
+
+/**
+ * usb_otg_register() - Register the OTG device to OTG core
+ * @parent_device:	parent device of Host & Gadget controllers.
+ * @otg_fsm_ops:	otg state machine ops.
+ *
+ * Register parent device that contains both HCD and GCD into
+ * the USB OTG core. HCD and GCD will be prevented from starting
+ * till both are available for use.
+ *
+ * Return: struct otg_fsm * if success, NULL if error.
+ */
+struct otg_fsm *usb_otg_register(struct device *parent_dev,
+				 struct otg_fsm_ops *fsm_ops)
+{
+	struct otg_data *otgd;
+	int ret = 0;
+
+	if (!parent_dev || !fsm_ops)
+		return ERR_PTR(-EINVAL);
+
+	/* already in list? */
+	mutex_lock(&otg_list_mutex);
+	if (usb_otg_device_get_otgd(parent_dev)) {
+		dev_err(parent_dev, "%s: device already in OTG list\n",
+			__func__);
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	/* allocate and add to list */
+	otgd = kzalloc(sizeof(*otgd), GFP_KERNEL);
+	if (!otgd) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	otgd->dev = parent_dev;
+	INIT_WORK(&otgd->work, usb_otg_work);
+	otgd->wq = create_singlethread_workqueue("usb_otg");
+	if (!otgd->wq) {
+		dev_err(parent_dev, "%s: can't create workqueue\n", __func__);
+		ret = -ENODEV;
+		goto err_wq;
+	}
+
+	usb_otg_init_timers(otgd);
+
+	/* save original start host/gadget ops */
+	otgd->start_host = fsm_ops->start_host;
+	otgd->start_gadget = fsm_ops->start_gadget;
+	/* create copy of original ops */
+	otgd->fsm_ops = *fsm_ops;
+	/* override ops */
+	otgd->fsm_ops.start_host = usb_otg_start_host;
+	otgd->fsm_ops.start_gadget = usb_otg_start_gadget;
+	/* FIXME: we ignore caller's timer ops */
+	otgd->fsm_ops.add_timer = usb_otg_add_timer;
+	otgd->fsm_ops.del_timer = usb_otg_del_timer;
+	/* set otg ops */
+	otgd->fsm.ops = &otgd->fsm_ops;
+	otgd->fsm.otg = &otgd->otg;
+
+	mutex_init(&otgd->fsm.lock);
+
+	list_add_tail(&otgd->list, &otg_list);
+	mutex_unlock(&otg_list_mutex);
+	return &otgd->fsm;
+
+err_wq:
+	kfree(otgd);
+unlock:
+	mutex_unlock(&otg_list_mutex);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(usb_otg_register);
+
+/**
+ * usb_otg_unregister() - Unregister the OTG device from USB OTG core
+ * @parent_device:	parent device of Host & Gadget controllers.
+ *
+ * Unregister parent OTG deviced from USB OTG core.
+ * Prevents unregistering till both Host and Gadget controllers
+ * have unregistered from the OTG core.
+ *
+ * Return: 0 on success, error value otherwise.
+ */
+int usb_otg_unregister(struct device *parent_dev)
+{
+	struct otg_data *otgd;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(parent_dev);
+	if (!otgd) {
+		dev_err(parent_dev, "%s: device not in OTG list\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	/* prevent unregister till both host & gadget have unregistered */
+	if (otgd->fsm.otg->host || otgd->fsm.otg->gadget) {
+		dev_err(parent_dev, "%s: host/gadget still registered\n",
+			__func__);
+		return -EBUSY;
+	}
+
+	/* OTG FSM is halted when host/gadget unregistered */
+	destroy_workqueue(otgd->wq);
+
+	/* remove from otg list */
+	list_del(&otgd->list);
+	kfree(otgd);
+	mutex_unlock(&otg_list_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_unregister);
+
+/**
+ * start/kick the OTG FSM if we can
+ * fsm->lock must be held
+ */
+static void usb_otg_start_fsm(struct otg_fsm *fsm)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+
+	if (otgd->fsm_running)
+		goto kick_fsm;
+
+	if (!fsm->otg->host) {
+		dev_info(otgd->dev, "Not starting OTG till Host registers\n");
+		return;
+	}
+
+	if (!fsm->otg->gadget) {
+		dev_info(otgd->dev, "Not starting OTG till Gadget registers\n");
+		return;
+	}
+
+	otgd->fsm_running = true;
+kick_fsm:
+	queue_work(otgd->wq, &otgd->work);
+}
+
+/**
+ * stop the OTG FSM. Stops Host & Gadget controllers as well.
+ * fsm->lock must be held
+ */
+static void usb_otg_stop_fsm(struct otg_fsm *fsm)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+	int i;
+
+	if (!otgd->fsm_running)
+		return;
+
+	/* no more new events queued */
+	otgd->fsm_running = false;
+
+	/* Stop state machine / timers */
+	for (i = 0; i < ARRAY_SIZE(otgd->timers); i++)
+		del_timer_sync(&otgd->timers[i].timer);
+
+	flush_workqueue(otgd->wq);
+
+	/* stop host/gadget immediately */
+	if (fsm->protocol == PROTO_HOST)
+		otg_start_host(fsm, 0);
+	else if (fsm->protocol == PROTO_GADGET)
+		otg_start_gadget(fsm, 0);
+}
+
+/**
+ * usb_otg_sync_inputs - Sync OTG inputs with the OTG state machine
+ * @fsm:	OTG FSM 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 otg_fsm *fsm)
+{
+	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
+
+	/* Don't kick FSM till it has started */
+	if (!otgd->fsm_running)
+		return;
+
+	/* Kick FSM */
+	queue_work(otgd->wq, &otgd->work);
+}
+EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
+
+/**
+ * usb_otg_kick_fsm - Kick the OTG state machine
+ * @hcd_gcd_device:	Host/Gadget controller device
+ *
+ * Used by USB host/device stack to sync OTG related
+ * events to the OTG state machine.
+ * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+int usb_otg_kick_fsm(struct device *hcd_gcd_device)
+{
+	struct otg_data *otgd;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(hcd_gcd_device->parent);
+	if (!otgd) {
+		dev_err(hcd_gcd_device, "%s: invalid host/gadget device\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -ENODEV;
+	}
+
+	mutex_unlock(&otg_list_mutex);
+	usb_otg_sync_inputs(&otgd->fsm);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
+
+/**
+ * usb_otg_register_hcd - Register Host controller to OTG core
+ * @hcd:	Host controller device
+ *
+ * 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 upto the OTG state machine to do so.
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+int usb_otg_register_hcd(struct usb_hcd *hcd)
+{
+	struct otg_data *otgd;
+	struct usb_bus *bus = hcd_to_bus(hcd);
+	struct device *otg_dev = bus->controller->parent;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(otg_dev);
+	if (!otgd) {
+		dev_err(otg_dev, "%s: device not registered to OTG core\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	mutex_unlock(&otg_list_mutex);
+	/* HCD will be started by OTG fsm when needed */
+	mutex_lock(&otgd->fsm.lock);
+	if (otgd->fsm.otg->host) {
+		/* probably a shared hcd ? */
+		if (usb_hcd_is_primary_hcd(hcd)) {
+			dev_err(otg_dev, "%s: hcd already registered\n",
+				__func__);
+			goto err;
+		}
+
+		if (hcd->shared_hcd == bus_to_hcd(otgd->fsm.otg->host)) {
+			if (otgd->shared_hcd) {
+				dev_err(otg_dev, "%s: shared HCD already registered\n",
+					__func__);
+				goto err;
+			}
+
+			otgd->shared_hcd = hcd;
+			dev_info(otg_dev, "USB-OTG: Shared Host %s registered\n",
+				 dev_name(bus->controller));
+			/* we might be pending a start_host */
+			if (otgd->host_can_start)
+				usb_otg_start_host(&otgd->fsm, 1);
+			goto unlock;
+		} else {
+			dev_err(otg_dev, "%s: invalid shared hcd\n",
+					__func__);
+			goto err;
+		}
+	}
+
+	otgd->fsm.otg->host = bus;
+
+	/* FIXME: set bus->otg_port if this is true OTG port with HNP */
+	dev_info(otg_dev, "USB-OTG: Primary Host %s registered\n",
+		 dev_name(bus->controller));
+
+unlock:
+	/* Kick FSM */
+	usb_otg_start_fsm(&otgd->fsm);
+	mutex_unlock(&otgd->fsm.lock);
+
+	return 0;
+
+err:
+	mutex_unlock(&otgd->fsm.lock);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
+
+/**
+ * usb_otg_unregister_hcd - Unregister 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 otg_data *otgd;
+	struct usb_bus *bus = hcd_to_bus(hcd);
+	struct device *otg_dev = bus->controller->parent;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(otg_dev);
+	if (!otgd) {
+		dev_err(otg_dev, "%s: device not registered to OTG core\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	mutex_unlock(&otg_list_mutex);
+
+	mutex_lock(&otgd->fsm.lock);
+	if (otgd->fsm.otg->host != bus) {
+		dev_err(otg_dev, "%s: host wansn't registered with OTG\n",
+			__func__);
+		mutex_unlock(&otgd->fsm.lock);
+		return -EINVAL;
+	}
+
+	/* stop FSM & Host */
+	usb_otg_stop_fsm(&otgd->fsm);
+	otgd->fsm.otg->host = NULL;
+
+	mutex_unlock(&otgd->fsm.lock);
+	dev_info(otg_dev, "USB-OTG: Host %s unregistered\n",
+		 dev_name(bus->controller));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_unregister_hcd);
+
+/**
+ * usb_otg_register_gadget - Register Gadget controller to OTG core
+ * @gadget:	Gadget controller
+ *
+ * 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 upto the OTG state machine to do so.
+ *
+ * As gadget controller needs to be prevented from starting till other
+ * conditions are met (i.e. driver loaded, softconnet enabled),
+ * the gadget stack can use the usb_otg_gadget_can_start() function
+ * to check if OTG core has given the permission to start the gadget.
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+int usb_otg_register_gadget(struct usb_gadget *gadget)
+{
+	struct otg_data *otgd;
+	struct device *otg_dev = gadget->dev.parent;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(otg_dev);
+	if (!otgd) {
+		dev_err(otg_dev, "%s: device not registered to OTG core\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	mutex_unlock(&otg_list_mutex);
+
+	mutex_lock(&otgd->fsm.lock);
+	if (otgd->fsm.otg->gadget) {
+		dev_err(otg_dev, "%s: gadget already registered with OTG\n",
+			__func__);
+		mutex_unlock(&otgd->fsm.lock);
+		return -EINVAL;
+	}
+
+	otgd->fsm.otg->gadget = gadget;
+	dev_info(otg_dev, "USB-OTG: Gadget %s registered\n",
+		 dev_name(&gadget->dev));
+
+	/* Kick FSM */
+	usb_otg_start_fsm(&otgd->fsm);
+	mutex_unlock(&otgd->fsm.lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_register_gadget);
+
+/**
+ * usb_otg_unregister_gadget - Unregister 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 otg_data *otgd;
+	struct device *otg_dev = gadget->dev.parent;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(otg_dev);
+	if (!otgd) {
+		dev_err(otg_dev, "%s: device not registered to OTG core\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	mutex_unlock(&otg_list_mutex);
+
+	mutex_lock(&otgd->fsm.lock);
+	if (otgd->fsm.otg->gadget != gadget) {
+		dev_err(otg_dev, "%s: gadget wasn't registered with OTG\n",
+			__func__);
+		mutex_unlock(&otgd->fsm.lock);
+		return -EINVAL;
+	}
+
+	/* Stop FSM & gadget */
+	usb_otg_stop_fsm(&otgd->fsm);
+	otgd->fsm.otg->gadget = NULL;
+	mutex_unlock(&otgd->fsm.lock);
+
+	dev_info(otg_dev, "USB-OTG: Gadget %s unregistered\n",
+		 dev_name(&gadget->dev));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_otg_unregister_gadget);
+
+/**
+ * usb_otg_gadget_can_start - check if Gadget can be started from OTG perspective
+ * @gadget:	Gadget controller
+ *
+ * This is used by the USB Gadget stack to check if Gadget controller
+ * can be started from the OTG state machines perspective.
+ * i.e. we're in 'b_peripheral' state.
+ *
+ * Returns: 0 on success, error value otherwise.
+ */
+bool usb_otg_gadget_can_start(struct usb_gadget *gadget)
+{
+	struct otg_data *otgd;
+	struct device *otg_dev = gadget->dev.parent;
+
+	mutex_lock(&otg_list_mutex);
+	otgd = usb_otg_device_get_otgd(otg_dev);
+	if (!otgd) {
+		dev_err(otg_dev, "%s: device not registered to OTG core\n",
+			__func__);
+		mutex_unlock(&otg_list_mutex);
+		return -EINVAL;
+	}
+
+	mutex_unlock(&otg_list_mutex);
+
+	return otgd->gadget_can_start;
+}
+EXPORT_SYMBOL_GPL(usb_otg_gadget_can_start);
diff --git a/drivers/usb/common/usb-otg.h b/drivers/usb/common/usb-otg.h
new file mode 100644
index 0000000..05331f0
--- /dev/null
+++ b/drivers/usb/common/usb-otg.h
@@ -0,0 +1,71 @@
+/**
+ * drivers/usb/common/usb-otg.h - USB OTG core local header
+ *
+ * Copyright (C) 2015 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.
+ */
+
+#ifndef __DRIVERS_USB_COMMON_USB_OTG_H
+#define __DRIVERS_USB_COMMON_USB_OTG_H
+
+/*
+ *  A-DEVICE timing constants
+ */
+
+/* Wait for VBUS Rise  */
+#define TA_WAIT_VRISE        (100)	/* a_wait_vrise: section 7.1.2
+					 * a_wait_vrise_tmr: section 7.4.5.1
+					 * TA_VBUS_RISE <= 100ms, section 4.4
+					 * Table 4-1: Electrical Characteristics
+					 * ->DC Electrical Timing
+					 */
+/* Wait for VBUS Fall  */
+#define TA_WAIT_VFALL        (1000)	/* a_wait_vfall: section 7.1.7
+					 * a_wait_vfall_tmr: section: 7.4.5.2
+					 */
+/* Wait for B-Connect */
+#define TA_WAIT_BCON         (10000)	/* a_wait_bcon: section 7.1.3
+					 * TA_WAIT_BCON: should be between 1100
+					 * and 30000 ms, section 5.5, Table 5-1
+					 */
+/* A-Idle to B-Disconnect */
+#define TA_AIDL_BDIS         (5000)	/* a_suspend min 200 ms, section 5.2.1
+					 * TA_AIDL_BDIS: section 5.5, Table 5-1
+					 */
+/* B-Idle to A-Disconnect */
+#define TA_BIDL_ADIS         (500)	/* TA_BIDL_ADIS: section 5.2.1
+					 * 500ms is used for B switch to host
+					 * for safe
+					 */
+
+/*
+ * B-device timing constants
+ */
+
+/* Data-Line Pulse Time*/
+#define TB_DATA_PLS          (10)	/* b_srp_init,continue 5~10ms
+					 * section:5.1.3
+					 */
+/* SRP Fail Time  */
+#define TB_SRP_FAIL          (6000)	/* b_srp_init,fail time 5~6s
+					 * section:5.1.6
+					 */
+/* A-SE0 to B-Reset  */
+#define TB_ASE0_BRST         (155)	/* minimum 155 ms, section:5.3.1 */
+/* SE0 Time Before SRP */
+#define TB_SE0_SRP           (1000)	/* b_idle,minimum 1s, section:5.1.2 */
+/* SSEND time before SRP */
+#define TB_SSEND_SRP         (1500)	/* minimum 1.5 sec, section:5.1.2 */
+
+#define TB_SESS_VLD          (1000)
+
+#endif /* __DRIVERS_USB_COMMON_USB_OTG_H */
diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index cc0ced0..43a0d2d 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -84,3 +84,11 @@ config USB_OTG_FSM
 	  Implements OTG Finite State Machine as specified in On-The-Go
 	  and Embedded Host Supplement to the USB Revision 2.0 Specification.
 
+config USB_OTG_CORE
+	tristate "USB OTG core"
+	depends on USB
+	select USB_OTG_FSM
+	help
+	  Standardize the way OTG is implemented on Linux. The OTG state
+	  machine is instantiated here instead of being done in each controller
+	  driver.
diff --git a/include/linux/usb/usb-otg.h b/include/linux/usb/usb-otg.h
new file mode 100644
index 0000000..52ab7b1
--- /dev/null
+++ b/include/linux/usb/usb-otg.h
@@ -0,0 +1,86 @@
+/**
+ * include/linux/usb/usb-otg.h - USB OTG core
+ *
+ * Copyright (C) 2015 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.
+ */
+
+#ifndef __LINUX_USB_OTG_CORE_H
+#define __LINUX_USB_OTG_CORE_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>
+
+#if IS_ENABLED(CONFIG_USB_OTG_CORE)
+struct otg_fsm *usb_otg_register(struct device *parent_dev,
+				 struct otg_fsm_ops *fsm_ops);
+int usb_otg_unregister(struct device *parent_dev);
+int usb_otg_register_hcd(struct usb_hcd *hcd);
+int usb_otg_unregister_hcd(struct usb_hcd *hcd);
+int usb_otg_register_gadget(struct usb_gadget *gadget);
+int usb_otg_unregister_gadget(struct usb_gadget *gadget);
+void usb_otg_sync_inputs(struct otg_fsm *fsm);
+int usb_otg_kick_fsm(struct device *hcd_gcd_device);
+bool usb_otg_gadget_can_start(struct usb_gadget *gadget);
+
+#else /* CONFIG_USB_OTG_CORE */
+
+static inline struct otg_fsm *usb_otg_register(struct device *parent_dev,
+					       struct otg_fsm_ops *fsm_ops)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline int usb_otg_unregister(struct device *parent_dev)
+{
+	return -ENOSYS;
+}
+
+static inline int usb_otg_register_hcd(struct usb_hcd *hcd)
+{
+	return -ENOSYS;
+}
+
+int usb_otg_unregister_hcd(struct usb_hcd *hcd)
+{
+	return -ENOSYS;
+}
+
+static inline int usb_otg_register_gadget(struct usb_gadget *gadget)
+{
+	return -ENOSYS;
+}
+
+static inline int usb_otg_unregister_gadget(struct usb_gadget *gadget)
+{
+	return -ENOSYS;
+}
+
+static inline void usb_otg_sync_inputs(struct otg_fsm *fsm)
+{
+}
+
+int usb_otg_kick_fsm(struct device *hcd_gcd_device)
+{
+	return -ENOSYS;
+}
+
+bool usb_otg_gadget_can_start(struct usb_gadget *gadget)
+{
+	return true;
+}
+#endif /* CONFIG_USB_OTG_CORE */
+
+#endif /* __LINUX_USB_OTG_CORE */
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 4/9] usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (2 preceding siblings ...)
  2015-03-18 13:55 ` [RFC][PATCH 3/9] usb: otg: add OTG core Roger Quadros
@ 2015-03-18 13:55 ` Roger Quadros
  2015-03-18 13:55 ` [RFC][PATCH 5/9] usb: hcd: adapt to OTG Roger Quadros
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:55 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

This is the a_set_b_hnp_enable flag in the OTG state machine
diagram and must be set when the A-Host has successfully set
the b_hnp_enable feature of the OTG-B-Peripheral attached to it.

When this bit changes we kick our OTG FSM to make note of the
change and act accordingly.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/core/hub.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d7c3d5a..ab0d498 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -22,6 +22,7 @@
 #include <linux/usb/hcd.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/quirks.h>
+#include <linux/usb/usb-otg.h>
 #include <linux/workqueue.h>
 #include <linux/mutex.h>
 #include <linux/random.h>
@@ -2270,6 +2271,9 @@ static int usb_enumerate_device_otg(struct usb_device *udev)
 						"can't set HNP mode: %d\n",
 						err);
 					bus->b_hnp_enable = 0;
+				} else {
+					/* notify OTG fsm about a_set_b_hnp_enable */
+					usb_otg_kick_fsm(udev->bus->controller);
 				}
 			}
 		}
@@ -4244,8 +4248,13 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
 	 */
 	if (!hdev->parent) {
 		delay = HUB_ROOT_RESET_TIME;
-		if (port1 == hdev->bus->otg_port)
+		if (port1 == hdev->bus->otg_port) {
 			hdev->bus->b_hnp_enable = 0;
+#ifdef CONFIG_USB_OTG
+			/* notify OTG fsm about a_set_b_hnp_enable change */
+			usb_otg_kick_fsm(hdev->bus->controller);
+#endif
+		}
 	}
 
 	/* Some low speed devices have problems with the quick delay, so */
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 5/9] usb: hcd: adapt to OTG
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (3 preceding siblings ...)
  2015-03-18 13:55 ` [RFC][PATCH 4/9] usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros
@ 2015-03-18 13:55 ` Roger Quadros
  2015-03-18 13:56 ` [RFC][PATCH 6/9] usb: gadget: udc: " Roger Quadros
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:55 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

Register with OTG core as part of usb_add_hcd() and
unregister from it in usb_remove_hcd().

For OTG device the HCD is actually started or stopped
from the OTG FSM.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/core/hcd.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index e28bd9d..9e8ecc9 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -46,6 +46,7 @@
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb/phy.h>
+#include <linux/usb/usb-otg.h>
 
 #include "usb.h"
 
@@ -2826,9 +2827,12 @@ int usb_add_hcd(struct usb_hcd *hcd,
 			goto err_request_irq;
 	}
 
-	retval = usb_start_hcd(hcd);
-	if (retval)
-		goto err_hcd_driver_start;
+	/* If OTG device, OTG core takes care of starting HCD */
+	if (usb_otg_register_hcd(hcd)) {
+		retval = usb_start_hcd(hcd);
+		if (retval)
+			goto err_hcd_driver_start;
+	}
 
 	return 0;
 
@@ -2936,7 +2940,9 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 {
 	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
 
-	usb_stop_hcd(hcd);
+	/* If OTG device, OTG core takes care of stopping HCD */
+	if (usb_otg_unregister_hcd(hcd))
+		usb_stop_hcd(hcd);
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		if (hcd->irq > 0)
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 6/9] usb: gadget: udc: adapt to OTG
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (4 preceding siblings ...)
  2015-03-18 13:55 ` [RFC][PATCH 5/9] usb: hcd: adapt to OTG Roger Quadros
@ 2015-03-18 13:56 ` Roger Quadros
  2015-03-18 13:56 ` [RFC][PATCH 7/9] usb: dwc3: adapt to OTG core Roger Quadros
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:56 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

Register with OTG core as part of usb_add_gadget_udc_release()
and unregister from it in usb_del_gadget_udc().

In the OTG use case we may not yet be in "b_peripheral"
state so we shouldn't allow starting the UDC till
OTG core says so.

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

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 69b4123..aad5173 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/usb-otg.h>
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -237,6 +238,7 @@ static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 static int usb_udc_start(struct usb_udc *udc)
 {
 	int ret;
+	bool otg_can_start;
 
 	if (udc->running) {
 		dev_err(&udc->dev, "%s: not starting as already running\n",
@@ -244,7 +246,8 @@ static int usb_udc_start(struct usb_udc *udc)
 		return -EBUSY;
 	}
 
-	if (udc->driver && udc->softconnect) {
+	otg_can_start = usb_otg_gadget_can_start(udc->gadget);
+	if (otg_can_start && udc->driver && udc->softconnect) {
 		ret = usb_gadget_udc_start(udc);
 		if (ret)
 			return ret;
@@ -432,6 +435,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	mutex_unlock(&udc_lock);
 
+	usb_otg_register_gadget(gadget);
+
 	return 0;
 
 err4:
@@ -509,6 +514,7 @@ found:
 	if (udc->driver)
 		usb_gadget_remove_driver(udc);
 
+	usb_otg_unregister_gadget(gadget);
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 7/9] usb: dwc3: adapt to OTG core
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (5 preceding siblings ...)
  2015-03-18 13:56 ` [RFC][PATCH 6/9] usb: gadget: udc: " Roger Quadros
@ 2015-03-18 13:56 ` Roger Quadros
  2015-03-18 13:56 ` [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm Roger Quadros
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:56 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

Register with the USB OTG core.

Use extcon framework to get VBUS/ID cable events and
kick the OTG state machine.

TODO:
- OTG driver ops to configure dwc3 otg core during role switches.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c          | 101 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc3/core.h          |   6 +++
 drivers/usb/dwc3/platform_data.h |   1 +
 3 files changed, 108 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9f0e209..9d8c306 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -39,6 +39,7 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/of.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/usb-otg.h>
 
 #include "platform_data.h"
 #include "core.h"
@@ -656,6 +657,82 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
 	return 0;
 }
 
+static void dwc3_sync_cable_state_to_otg(struct dwc3 *dwc)
+{
+	int id, vbus;
+
+	/* get ID */
+	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
+	/* Host means ID == 0 */
+	id = !id;
+
+	/* get VBUS */
+	vbus = extcon_get_cable_state(dwc->edev, "USB");
+	dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
+
+	dwc->fsm->id = id;
+	dwc->fsm->a_vbus_vld = vbus;
+	/* simulate b_sess_vld */
+	dwc->fsm->b_sess_vld = id && vbus;
+	usb_otg_sync_inputs(dwc->fsm);
+}
+
+static int dwc3_otg_notifier(struct notifier_block *nb,
+		unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, otg_nb);
+
+	dwc3_sync_cable_state_to_otg(dwc);
+
+	return NOTIFY_DONE;
+}
+
+static struct otg_fsm_ops dwc3_otg_ops; /* FIXME. need real stuff */
+
+static int dwc3_otg_init(struct dwc3 *dwc)
+{
+	int ret, id, vbus;
+
+	if (!dwc->edev) {
+		dev_err(dwc->dev, "No extcon device found for OTG mode\n");
+		return -ENODEV;
+	}
+
+	dwc->otg_nb.notifier_call = dwc3_otg_notifier;
+	ret = extcon_register_notifier(dwc->edev, &dwc->otg_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
+		return -ENODEV;
+	}
+
+	/* sanity check id & vbus states */
+	id = extcon_get_cable_state(dwc->edev, "USB-HOST");
+	vbus = extcon_get_cable_state(dwc->edev, "USB");
+	if (id < 0 || vbus < 0) {
+		dev_err(dwc->dev, "Invalid USB cable state. id %d, vbus %d\n",
+			id, vbus);
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* register parent as OTG device with USB core */
+	dwc->fsm = usb_otg_register(dwc->dev, &dwc3_otg_ops);
+	if (IS_ERR(dwc->fsm)) {
+		dev_err(dwc->dev, "Failed to register with OTG core\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	dwc3_sync_cable_state_to_otg(dwc);
+
+	return 0;
+
+fail:
+	extcon_unregister_notifier(dwc->edev, &dwc->otg_nb);
+
+	return ret;
+}
+
 static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
@@ -680,6 +757,12 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		break;
 	case USB_DR_MODE_OTG:
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
+		ret = dwc3_otg_init(dwc);
+		if (ret) {
+			dev_err(dev, "failed to initialize OTG\n");
+			return ret;
+		}
+
 		ret = dwc3_host_init(dwc);
 		if (ret) {
 			dev_err(dev, "failed to initialize host\n");
@@ -712,6 +795,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 	case USB_DR_MODE_OTG:
 		dwc3_host_exit(dwc);
 		dwc3_gadget_exit(dwc);
+		usb_otg_unregister(dwc->dev);
 		break;
 	default:
 		/* do nothing */
@@ -799,6 +883,16 @@ static int dwc3_probe(struct platform_device *pdev)
 	hird_threshold = 12;
 
 	if (node) {
+		if (of_property_read_bool(node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
+
+		if (IS_ERR(dwc->edev)) {
+			dev_vdbg(dev, "couldn't get extcon device\n");
+			return -EPROBE_DEFER;
+		}
+
 		dwc->maximum_speed = of_usb_get_maximum_speed(node);
 		dwc->has_lpm_erratum = of_property_read_bool(node,
 				"snps,has-lpm-erratum");
@@ -839,6 +933,13 @@ static int dwc3_probe(struct platform_device *pdev)
 		of_property_read_u8(node, "snps,tx_de_emphasis",
 				&tx_de_emphasis);
 	} else if (pdata) {
+		if (pdata->extcon) {
+			dwc->edev = extcon_get_extcon_dev(pdata->extcon);
+			if (!dwc->edev) {
+				dev_vdbg(dev, "couldn't get extcon device\n");
+				return -EPROBE_DEFER;
+			}
+		}
 		dwc->maximum_speed = pdata->maximum_speed;
 		dwc->has_lpm_erratum = pdata->has_lpm_erratum;
 		if (pdata->lpm_nyet_threshold)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index d201910..2ece013 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -30,8 +30,10 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/otg-fsm.h>
 
 #include <linux/phy/phy.h>
+#include <linux/extcon.h>
 
 #define DWC3_MSG_MAX	500
 
@@ -738,6 +740,10 @@ struct dwc3 {
 	struct phy		*usb2_generic_phy;
 	struct phy		*usb3_generic_phy;
 
+	struct extcon_dev	*edev;	/* USB cable events ID & VBUS */
+	struct notifier_block	otg_nb;	/* notifier for USB cable events */
+	struct otg_fsm		*fsm;
+
 	void __iomem		*regs;
 	size_t			regs_size;
 
diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
index a3a3b6d5..9552601 100644
--- a/drivers/usb/dwc3/platform_data.h
+++ b/drivers/usb/dwc3/platform_data.h
@@ -24,6 +24,7 @@ struct dwc3_platform_data {
 	enum usb_device_speed maximum_speed;
 	enum usb_dr_mode dr_mode;
 	bool tx_fifo_resize;
+	const char *extcon;	/* extcon name for USB cable events ID/VBUS */
 
 	unsigned is_utmi_l1_suspend:1;
 	u8 hird_threshold;
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (6 preceding siblings ...)
  2015-03-18 13:56 ` [RFC][PATCH 7/9] usb: dwc3: adapt to OTG core Roger Quadros
@ 2015-03-18 13:56 ` Roger Quadros
  2015-03-19  3:46   ` Peter Chen
  2015-03-18 13:56 ` [RFC][PATCH 9/9] usb: otg-fsm: Add documentation for " Roger Quadros
  2015-03-18 17:37 ` [RFC][PATCH 0/9] USB: OTG Core functionality Tony Lindgren
  9 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:56 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

These members are not used anywhere so remove them.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 include/linux/usb/otg-fsm.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
index b6ba1bf..176c4fc 100644
--- a/include/linux/usb/otg-fsm.h
+++ b/include/linux/usb/otg-fsm.h
@@ -95,11 +95,6 @@ struct otg_fsm {
 	int b_hnp_enable;
 	int a_clr_err;
 
-	/* Informative variables */
-	int a_bus_drop_inf;
-	int a_bus_req_inf;
-	int a_clr_err_inf;
-	int b_bus_req_inf;
 	/* Auxilary informative variables */
 	int a_suspend_req_inf;
 
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC][PATCH 9/9] usb: otg-fsm: Add documentation for struct otg_fsm
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (7 preceding siblings ...)
  2015-03-18 13:56 ` [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm Roger Quadros
@ 2015-03-18 13:56 ` Roger Quadros
  2015-03-18 17:37 ` [RFC][PATCH 0/9] USB: OTG Core functionality Tony Lindgren
  9 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-18 13:56 UTC (permalink / raw)
  To: gregkh, balbi, stern
  Cc: dan.j.williams, peter.chen, jun.li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap, Roger Quadros

struct otg_fsm is the interface to the OTG state machine.

Document the input, output and internal state variables.
Definations are taken from Table 7-2 and Table 7-4 of
the USB OTG & EH Specification Rev.2.0

Re-arrange some of the members as per use case for more
clarity.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 include/linux/usb/otg-fsm.h | 76 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
index 176c4fc..10f7a1d 100644
--- a/include/linux/usb/otg-fsm.h
+++ b/include/linux/usb/otg-fsm.h
@@ -57,37 +57,101 @@ enum otg_fsm_timer {
 	NUM_OTG_FSM_TIMERS,
 };
 
-/* OTG state machine according to the OTG spec */
+/**
+ * struct otg_fsm - OTG state machine according to the OTG spec
+ *
+ * OTG hardware Inputs
+ *
+ *	Common inputs for A and B device
+ * @id:		TRUE for B-device, FALSE for A-device.
+ * @adp_change: TRUE when current ADP measurement (n) value, compared to the
+ *		ADP measurement taken at n-2, differs by more than CADP_THR
+ * @power_up:	TRUE when the OTG device first powers up its USB system and
+ *		ADP measurement taken if ADP capable
+ *
+ *	A-Device state inputs
+ * @a_srp_det:	TRUE if the A-device detects SRP
+ * @a_vbus_vld:	TRUE when VBUS voltage is in regulation
+ * @b_conn:	TRUE if the A-device detects connection from the B-device
+ * @a_bus_resume: TRUE when the B-device detects that the A-device is signaling
+ *		  a resume (K state)
+ * @a_conn:	TRUE if the B-device detects a connection from the A-device
+ * @b_se0_srp:	TRUE when the line has been at SE0 for more than the minimum
+ *		time before generating SRP
+ * @b_ssend_srp: TRUE when the VBUS has been below VOTG_SESS_VLD for more than
+ *		 the minimum time before generating SRP
+ * @b_sess_vld:	TRUE when the B-device detects that the voltage on VBUS is
+ *		above VOTG_SESS_VLD
+ * @test_device: TRUE when the B-device switches to B-Host and detects an OTG test device
+ *		 FIXME: must be set by host/hub driver
+ *
+ *	Application inputs (A-Device)
+ * @a_bus_drop:	TRUE when A-device application needs to power down the bus
+ * @a_bus_req:	TRUE when A-device application wants to use the bus.
+ *		FALSE to suspend the bus
+ *
+ *	Application inputs (B-Device)
+ * @b_bus_req:	TRUE during the time that the Application running on the
+ *		B-device wants to use the bus
+ *
+ *	Auxilary inputs
+ * @a_sess_vld:		??
+ * @b_bus_suspend:	??
+ * @b_bus_resume:	??
+ *
+ * OTG Output status. Read only for users. updated by otg_ops() helpers
+ *
+ *	Outputs for Both A and B device
+ * @drv_vbus:	TRUE when A-device is driving VBUS
+ * @loc_conn:	TRUE when the local device has signaled that it is connected to the bus
+ * @loc_sof:	TRUE when the local device is generating activity on the bus
+ * @adp_prb:	TRUE when the local device is in the process of doing ADP probing
+ *
+ *	Outputs for B-device state
+ * @adp_sns:	TRUE when the B-device is in the process of carrying out ADP sensing
+ * @data_pulse: TRUE when the B-device is performing data line pulsing
+ *
+ * Internal Variables
+ *
+ * a_set_b_hnp_en: TRUE when the A-device has successfully set the b_hnp_enable bit in the B-device.
+ *		   FIXME: OTG fsm uses otg->host->b_hnp_enable instead
+ * b_srp_done:	TRUE when the B-device has completed initiating SRP
+ * b_hnp_enable: TRUE when the B-device has accepted the SetFeature(b_hnp_enable) B-device
+ *		 FIXME: OTG fsm uses otg->gadget->b_hnp_enable instead
+ * 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
+ */
 struct otg_fsm {
 	/* Input */
 	int id;
 	int adp_change;
 	int power_up;
-	int test_device;
-	int a_bus_drop;
-	int a_bus_req;
 	int a_srp_det;
 	int a_vbus_vld;
 	int b_conn;
 	int a_bus_resume;
 	int a_bus_suspend;
 	int a_conn;
-	int b_bus_req;
 	int b_se0_srp;
 	int b_ssend_srp;
 	int b_sess_vld;
+	int test_device;
+	int a_bus_drop;
+	int a_bus_req;
+	int b_bus_req;
+
 	/* Auxilary inputs */
 	int a_sess_vld;
 	int b_bus_resume;
 	int b_bus_suspend;
 
 	/* Output */
-	int data_pulse;
 	int drv_vbus;
 	int loc_conn;
 	int loc_sof;
 	int adp_prb;
 	int adp_sns;
+	int data_pulse;
 
 	/* Internal variables */
 	int a_set_b_hnp_en;
-- 
2.1.0


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 0/9] USB: OTG Core functionality
  2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
                   ` (8 preceding siblings ...)
  2015-03-18 13:56 ` [RFC][PATCH 9/9] usb: otg-fsm: Add documentation for " Roger Quadros
@ 2015-03-18 17:37 ` Tony Lindgren
  2015-03-19 10:31   ` Roger Quadros
  9 siblings, 1 reply; 40+ messages in thread
From: Tony Lindgren @ 2015-03-18 17:37 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

* Roger Quadros <rogerq@ti.com> [150318 07:00]:
> Hi,
> 
> [NOTE: RFC only. Not for merge yet.]
> 
> This is an attempt to centralize OTG functionality in the kernel.
> Still work in progress but I wanted to get an early feedback
> to avoid major rework. :) 
> 
> Why?:
> ----
> 
> Most of the OTG drivers have been dealing with the OTG state machine
> themselves and there is a scope for code re-use. This has been
> partly addressed by the usb/common/usb-otg-fsm.c but it still
> leaves the instantiation of the state machine and OTG timers
> to the controller drivers. We re-use usb-otg-fsm.c but
> go one step further by instantiating the state machine and timers
> thus making it easier for drivers to implement OTG functionality.
> 
> Newer OTG cores support standard host interface (e.g. xHCI?) so
> host and gadget functionality are no longer closely knit like older
> cores. There needs to be a way to co-ordinate the operation of the
> host and gadget in OTG mode. i.e. to stop and start them from a
> central location. This central location should be the USB OTG core.
> 
> Host and gadget controllers might be sharing resources and can't
> be always running. One has to be stopped for the other to run.
> This can't be done as of now and can be done from the OTG core.
> 
> What?:
> -----
> 
> The OTG core instantiates the OTG Finite State Machine
> per OTG controller and manages starting/stopping the
> host and gadget controllers based on the bus state.
>     
> It provides APIs for the following
>     
> - Registering an OTG capable controller
> struct otg_fsm *usb_otg_register(struct device *parent_dev,
>                                  struct otg_fsm_ops *fsm_ops);
> int usb_otg_unregister(struct device *parent_dev);
> 
> - Registering Host controllers to OTG core (used by hcd-core)
> int usb_otg_register_hcd(struct usb_hcd *hcd);
> int usb_otg_unregister_hcd(struct usb_hcd *hcd);
> 
> - Registering Gadget controllers to OTG core (used by udc-core)
> int usb_otg_register_gadget(struct usb_gadget *gadget);
> int usb_otg_unregister_gadget(struct usb_gadget *gadget);
> 
> - Providing inputs to and kicking the OTG state machine
> void usb_otg_sync_inputs(struct otg_fsm *fsm);
> int usb_otg_kick_fsm(struct device *hcd_gcd_device);
> 
> 'struct otg_fsm' is the interface to the OTG state machine.
> It contains inputs to the fsm, status of the fsm and operations
> for the OTG controller driver.

Sounds good to me. I take you're also planning to provide some
common /sys/kernel/debug/otg type interfaces for OTG validation
tests? For things like SRP etc.

Regards,

Tony
 
> Usage model:
> -----------
> 
> - The OTG controller device is assumed to be the parent of
> the host and gadget controller. It must call usb_otg_register()
> before populating the host and gadget devices so that the OTG
> core is aware that it is an OTG device before the host & gadget
> register. The OTG controller must provide struct otg_fsm_ops *
> which will be called by the OTG core depending on OTG bus state.
> 
> - The host/gadget core stacks are modified to inform the OTG core
> whenever a new host/gadget device is added. The OTG core then
> checks if the host/gadget is part of the OTG controller and if yes
> then prevents the host/gadget from starting till both host and
> gadget are registered, OTG state machine is running and the
> USB bus state is appropriate to start host/gadget.
>  For this APIs have been added to host/gadget stacks to start/stop
> the controllers from the OTG core.
> 
> - No modification is needed for the host/gadget controller drivers.
> They must ensure that their start/stop methods can be called repeatedly
> and any shared resources between host & gadget are properly managed.
> The OTG core ensures that both are not started simultaneously.
> 
> - The OTG core instantiates one OTG state machine per OTG
> controller and the necessary OTG timers to manage OTG state timeouts.
> The state machine is started when both host & gadget register and
> stopped when either of them unregisters. The controllers are started
> and stopped depending on bus state.
> 
> - During the lifetime of the OTG state machine, inputs can be
> provided to it by modifying the appropriate members of 'struct otg_fsm'
> and calling usb_otg_sync_inputs(). This is typically done by the
> OTG controller driver that called usb_otg_register() since it is
> the only external component that has the 'struct otg_fsm' handle.
> 
> Pending items:
> - We probably need a otg class.
> - sysfs interface for application OTG inputs and OTG status information
> - resolve symbol dependency for module use.
> - otg driver for dwc3 core to get dual-role working on dra7-evm.
> 
> cheers,
> -roger
> 
> Roger Quadros (9):
>   usb: hcd: Introduce usb_start/stop_hcd()
>   usb: gadget: add usb_gadget_start/stop()
>   usb: otg: add OTG core
>   usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable
>   usb: hcd: adapt to OTG
>   usb: gadget: udc: adapt to OTG
>   usb: dwc3: adapt to OTG core
>   usb: otg-fsm: Remove unused members in struct otg_fsm
>   usb: otg-fsm: Add documentation for struct otg_fsm
> 
>  drivers/usb/Makefile              |   1 +
>  drivers/usb/common/Makefile       |   1 +
>  drivers/usb/common/usb-otg.c      | 732 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/common/usb-otg.h      |  71 ++++
>  drivers/usb/core/Kconfig          |   8 +
>  drivers/usb/core/hcd.c            | 153 +++++---
>  drivers/usb/core/hub.c            |  11 +-
>  drivers/usb/dwc3/core.c           | 101 ++++++
>  drivers/usb/dwc3/core.h           |   6 +
>  drivers/usb/dwc3/platform_data.h  |   1 +
>  drivers/usb/gadget/udc/udc-core.c | 172 ++++++++-
>  include/linux/usb/gadget.h        |   3 +
>  include/linux/usb/hcd.h           |   2 +
>  include/linux/usb/otg-fsm.h       |  81 ++++-
>  include/linux/usb/usb-otg.h       |  86 +++++
>  15 files changed, 1357 insertions(+), 72 deletions(-)
>  create mode 100644 drivers/usb/common/usb-otg.c
>  create mode 100644 drivers/usb/common/usb-otg.h
>  create mode 100644 include/linux/usb/usb-otg.h
> 
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-18 13:55 ` [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd() Roger Quadros
@ 2015-03-18 19:49   ` Alan Stern
  2015-03-18 21:41     ` Tony Lindgren
  2015-03-19 11:38     ` Roger Quadros
  2015-03-19  1:46   ` Peter Chen
  1 sibling, 2 replies; 40+ messages in thread
From: Alan Stern @ 2015-03-18 19:49 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, dan.j.williams, peter.chen, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Wed, 18 Mar 2015, Roger Quadros wrote:

> To support OTG we want a mechanism to start and stop
> the HCD from the OTG state machine. Add usb_start_hcd()
> and usb_stop_hcd().
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

There are a few problems in this proposed patch.

> +int usb_start_hcd(struct usb_hcd *hcd)
> +{
> +	int retval;
> +	struct usb_device *rhdev = hcd->self.root_hub;
> +
> +	if (hcd->state != HC_STATE_HALT) {
> +		dev_err(hcd->self.controller, "not starting a running HCD\n");
> +		return -EINVAL;
> +	}
> +
> +	hcd->state = HC_STATE_RUNNING;
> +	retval = hcd->driver->start(hcd);
> +	if (retval < 0) {
> +		dev_err(hcd->self.controller, "startup error %d\n", retval);
> +		hcd->state = HC_STATE_HALT;
> +		return retval;
> +	}
> +
> +	/* starting here, usbcore will pay attention to this root hub */
> +	if ((retval = register_root_hub(hcd)) != 0)
> +		goto err_register_root_hub;

If the host controller is started more than once, you will end up
unregistering and re-registering the root hub.  The device core does
not allow this.  Once a device has been unregistered, you must not try
to register it again -- you have to allocate a new device and register
it instead.

Also, although you call the driver's ->start method multiple times, the
->reset method is called only once, when the controller is first 
probed.  It's not clear that this will work in a situation where the HC 
and the UDC share hardware state; after the UDC is stopped it may be 
necessary to reset the HC before it can run again.

It might be possible to make this work, but I suspect quite a few 
drivers would need rewriting first.  As another example of the problems 
you face, consider how stopping a host controller will interact with 
the driver's PM support (both system suspend and runtime suspend).

It would be a lot simpler to unbind the host controller driver
completely when switching to device mode and rebind it when switching
back.  I guess that is the sort of heavy-duty approach you want to
avoid, but it may be the only practical way forward.

Alan Stern


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-18 19:49   ` Alan Stern
@ 2015-03-18 21:41     ` Tony Lindgren
  2015-03-19  1:51       ` Alan Stern
  2015-03-19 11:38     ` Roger Quadros
  1 sibling, 1 reply; 40+ messages in thread
From: Tony Lindgren @ 2015-03-18 21:41 UTC (permalink / raw)
  To: Alan Stern
  Cc: Roger Quadros, gregkh, balbi, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

* Alan Stern <stern@rowland.harvard.edu> [150318 12:50]:
> On Wed, 18 Mar 2015, Roger Quadros wrote:
> 
> > To support OTG we want a mechanism to start and stop
> > the HCD from the OTG state machine. Add usb_start_hcd()
> > and usb_stop_hcd().
> > 
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> There are a few problems in this proposed patch.
> 
> > +int usb_start_hcd(struct usb_hcd *hcd)
> > +{
> > +	int retval;
> > +	struct usb_device *rhdev = hcd->self.root_hub;
> > +
> > +	if (hcd->state != HC_STATE_HALT) {
> > +		dev_err(hcd->self.controller, "not starting a running HCD\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	hcd->state = HC_STATE_RUNNING;
> > +	retval = hcd->driver->start(hcd);
> > +	if (retval < 0) {
> > +		dev_err(hcd->self.controller, "startup error %d\n", retval);
> > +		hcd->state = HC_STATE_HALT;
> > +		return retval;
> > +	}
> > +
> > +	/* starting here, usbcore will pay attention to this root hub */
> > +	if ((retval = register_root_hub(hcd)) != 0)
> > +		goto err_register_root_hub;
> 
> If the host controller is started more than once, you will end up
> unregistering and re-registering the root hub.  The device core does
> not allow this.  Once a device has been unregistered, you must not try
> to register it again -- you have to allocate a new device and register
> it instead.
> 
> Also, although you call the driver's ->start method multiple times, the
> ->reset method is called only once, when the controller is first 
> probed.  It's not clear that this will work in a situation where the HC 
> and the UDC share hardware state; after the UDC is stopped it may be 
> necessary to reset the HC before it can run again.
> 
> It might be possible to make this work, but I suspect quite a few 
> drivers would need rewriting first.  As another example of the problems 
> you face, consider how stopping a host controller will interact with 
> the driver's PM support (both system suspend and runtime suspend).
> 
> It would be a lot simpler to unbind the host controller driver
> completely when switching to device mode and rebind it when switching
> back.  I guess that is the sort of heavy-duty approach you want to
> avoid, but it may be the only practical way forward.

Hmm from memory I think the OTG spec assumes the USB devices are
suspended when attempting the role change? I could be totally wrong,
it's been a really long time since I've looked at the OTG spec, but
maybe that would make it easier to deal with thing?

Regards,

Tony

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-18 13:55 ` [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd() Roger Quadros
  2015-03-18 19:49   ` Alan Stern
@ 2015-03-19  1:46   ` Peter Chen
  1 sibling, 0 replies; 40+ messages in thread
From: Peter Chen @ 2015-03-19  1:46 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Wed, Mar 18, 2015 at 03:55:55PM +0200, Roger Quadros wrote:
> To support OTG we want a mechanism to start and stop
> the HCD from the OTG state machine. Add usb_start_hcd()
> and usb_stop_hcd().

Hi Roger,

You may not need to create another pair of hcd APIs for doing
it, you can use usb_add_hcd/usb_remove_hcd directly, it is safer
and cleaner. The chipidea uses it for both ID role switch use case
and OTG FSM use case.

Peter
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/core/hcd.c  | 147 ++++++++++++++++++++++++++++++++----------------
>  include/linux/usb/hcd.h |   2 +
>  2 files changed, 100 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 45a915c..e28bd9d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2613,7 +2613,76 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
>  }
>  
>  /**
> - * usb_add_hcd - finish generic HCD structure initialization and register
> + * usb_start_hcd - start the HCD
> + * @hcd: the usb_hcd structure to initialize
> + *
> + * calls the drivers start() routine, registers the root hub
> + * and creates the USB sysfs attributes.
> + */
> +int usb_start_hcd(struct usb_hcd *hcd)
> +{
> +	int retval;
> +	struct usb_device *rhdev = hcd->self.root_hub;
> +
> +	if (hcd->state != HC_STATE_HALT) {
> +		dev_err(hcd->self.controller, "not starting a running HCD\n");
> +		return -EINVAL;
> +	}
> +
> +	hcd->state = HC_STATE_RUNNING;
> +	retval = hcd->driver->start(hcd);
> +	if (retval < 0) {
> +		dev_err(hcd->self.controller, "startup error %d\n", retval);
> +		hcd->state = HC_STATE_HALT;
> +		return retval;
> +	}
> +
> +	/* starting here, usbcore will pay attention to this root hub */
> +	if ((retval = register_root_hub(hcd)) != 0)
> +		goto err_register_root_hub;
> +
> +	retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
> +	if (retval < 0) {
> +		pr_err("Cannot register USB bus sysfs attributes: %d\n",
> +		       retval);
> +		goto error_create_attr_group;
> +	}
> +	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
> +		usb_hcd_poll_rh_status(hcd);
> +
> +	return retval;
> +
> +error_create_attr_group:
> +	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> +	if (HC_IS_RUNNING(hcd->state))
> +		hcd->state = HC_STATE_QUIESCING;
> +	spin_lock_irq(&hcd_root_hub_lock);
> +	hcd->rh_registered = 0;
> +	spin_unlock_irq(&hcd_root_hub_lock);
> +
> +#ifdef CONFIG_PM
> +	cancel_work_sync(&hcd->wakeup_work);
> +#endif
> +	mutex_lock(&usb_bus_list_lock);
> +	usb_disconnect(&rhdev);		/* Sets rhdev to NULL */
> +	mutex_unlock(&usb_bus_list_lock);
> +err_register_root_hub:
> +	hcd->rh_pollable = 0;
> +	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> +	del_timer_sync(&hcd->rh_timer);
> +	hcd->driver->stop(hcd);
> +	hcd->state = HC_STATE_HALT;
> +
> +	/* In case the HCD restarted the timer, stop it again. */
> +	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> +	del_timer_sync(&hcd->rh_timer);
> +
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(usb_start_hcd);
> +
> +/**
> + * usb_add_hcd - finish generic HCD structure initialization and register.
>   * @hcd: the usb_hcd structure to initialize
>   * @irqnum: Interrupt line to allocate
>   * @irqflags: Interrupt type flags
> @@ -2757,50 +2826,12 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  			goto err_request_irq;
>  	}
>  
> -	hcd->state = HC_STATE_RUNNING;
> -	retval = hcd->driver->start(hcd);
> -	if (retval < 0) {
> -		dev_err(hcd->self.controller, "startup error %d\n", retval);
> +	retval = usb_start_hcd(hcd);
> +	if (retval)
>  		goto err_hcd_driver_start;
> -	}
> -
> -	/* starting here, usbcore will pay attention to this root hub */
> -	if ((retval = register_root_hub(hcd)) != 0)
> -		goto err_register_root_hub;
> -
> -	retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
> -	if (retval < 0) {
> -		printk(KERN_ERR "Cannot register USB bus sysfs attributes: %d\n",
> -		       retval);
> -		goto error_create_attr_group;
> -	}
> -	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
> -		usb_hcd_poll_rh_status(hcd);
> -
> -	return retval;
>  
> -error_create_attr_group:
> -	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> -	if (HC_IS_RUNNING(hcd->state))
> -		hcd->state = HC_STATE_QUIESCING;
> -	spin_lock_irq(&hcd_root_hub_lock);
> -	hcd->rh_registered = 0;
> -	spin_unlock_irq(&hcd_root_hub_lock);
> +	return 0;
>  
> -#ifdef CONFIG_PM
> -	cancel_work_sync(&hcd->wakeup_work);
> -#endif
> -	mutex_lock(&usb_bus_list_lock);
> -	usb_disconnect(&rhdev);		/* Sets rhdev to NULL */
> -	mutex_unlock(&usb_bus_list_lock);
> -err_register_root_hub:
> -	hcd->rh_pollable = 0;
> -	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> -	del_timer_sync(&hcd->rh_timer);
> -	hcd->driver->stop(hcd);
> -	hcd->state = HC_STATE_HALT;
> -	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> -	del_timer_sync(&hcd->rh_timer);
>  err_hcd_driver_start:
>  	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
>  		free_irq(irqnum, hcd);
> @@ -2830,18 +2861,20 @@ err_phy:
>  EXPORT_SYMBOL_GPL(usb_add_hcd);
>  
>  /**
> - * usb_remove_hcd - shutdown processing for generic HCDs
> - * @hcd: the usb_hcd structure to remove
> - * Context: !in_interrupt()
> + * usb_stop_hcd - stop the HCD
> + * @hcd: the usb_hcd structure to initialize
>   *
> - * Disconnects the root hub, then reverses the effects of usb_add_hcd(),
> - * invoking the HCD's stop() method.
> + * removes the USB sysfs attributes, disconnects the root hub
> + * and calls the driver's stop() routine.
>   */
> -void usb_remove_hcd(struct usb_hcd *hcd)
> +void usb_stop_hcd(struct usb_hcd *hcd)
>  {
>  	struct usb_device *rhdev = hcd->self.root_hub;
>  
> -	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
> +	if (hcd->state == HC_STATE_HALT) {
> +		dev_err(hcd->self.controller, "not stopping a halted HCD\n");
> +		return;
> +	}
>  
>  	usb_get_dev(rhdev);
>  	sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
> @@ -2888,6 +2921,22 @@ void usb_remove_hcd(struct usb_hcd *hcd)
>  	/* In case the HCD restarted the timer, stop it again. */
>  	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
>  	del_timer_sync(&hcd->rh_timer);
> +}
> +EXPORT_SYMBOL_GPL(usb_stop_hcd);
> +
> +/**
> + * usb_remove_hcd - shutdown processing for generic HCDs
> + * @hcd: the usb_hcd structure to remove
> + * Context: !in_interrupt()
> + *
> + * Disconnects the root hub, then reverses the effects of usb_add_hcd(),
> + * invoking the HCD's stop() method.
> + */
> +void usb_remove_hcd(struct usb_hcd *hcd)
> +{
> +	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
> +
> +	usb_stop_hcd(hcd);
>  
>  	if (usb_hcd_is_primary_hcd(hcd)) {
>  		if (hcd->irq > 0)
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 68b1e83..12aaf4c 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -433,6 +433,8 @@ extern void usb_put_hcd(struct usb_hcd *hcd);
>  extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd);
>  extern int usb_add_hcd(struct usb_hcd *hcd,
>  		unsigned int irqnum, unsigned long irqflags);
> +extern int usb_start_hcd(struct usb_hcd *hcd);
> +extern void usb_stop_hcd(struct usb_hcd *hcd);
>  extern void usb_remove_hcd(struct usb_hcd *hcd);
>  extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
>  
> -- 
> 2.1.0
> 

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-18 21:41     ` Tony Lindgren
@ 2015-03-19  1:51       ` Alan Stern
  2015-03-19  2:38         ` Tony Lindgren
  0 siblings, 1 reply; 40+ messages in thread
From: Alan Stern @ 2015-03-19  1:51 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Roger Quadros, gregkh, balbi, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On Wed, 18 Mar 2015, Tony Lindgren wrote:

> > If the host controller is started more than once, you will end up
> > unregistering and re-registering the root hub.  The device core does
> > not allow this.  Once a device has been unregistered, you must not try
> > to register it again -- you have to allocate a new device and register
> > it instead.
> > 
> > Also, although you call the driver's ->start method multiple times, the
> > ->reset method is called only once, when the controller is first 
> > probed.  It's not clear that this will work in a situation where the HC 
> > and the UDC share hardware state; after the UDC is stopped it may be 
> > necessary to reset the HC before it can run again.
> > 
> > It might be possible to make this work, but I suspect quite a few 
> > drivers would need rewriting first.  As another example of the problems 
> > you face, consider how stopping a host controller will interact with 
> > the driver's PM support (both system suspend and runtime suspend).
> > 
> > It would be a lot simpler to unbind the host controller driver
> > completely when switching to device mode and rebind it when switching
> > back.  I guess that is the sort of heavy-duty approach you want to
> > avoid, but it may be the only practical way forward.
> 
> Hmm from memory I think the OTG spec assumes the USB devices are
> suspended when attempting the role change? I could be totally wrong,
> it's been a really long time since I've looked at the OTG spec, but
> maybe that would make it easier to deal with thing?

This patch deals with the host side, not the device side.  The fact
that the device is suspended is not relevant to the issues above.

Besides, the problems I outlined are more connected with the way 
Linux's host-side USB stack is organized, and not so much with the 
details of the OTG spec.

Alan Stern


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-19  1:51       ` Alan Stern
@ 2015-03-19  2:38         ` Tony Lindgren
  0 siblings, 0 replies; 40+ messages in thread
From: Tony Lindgren @ 2015-03-19  2:38 UTC (permalink / raw)
  To: Alan Stern
  Cc: Roger Quadros, gregkh, balbi, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

* Alan Stern <stern@rowland.harvard.edu> [150318 18:51]:
> On Wed, 18 Mar 2015, Tony Lindgren wrote:
> 
> > > If the host controller is started more than once, you will end up
> > > unregistering and re-registering the root hub.  The device core does
> > > not allow this.  Once a device has been unregistered, you must not try
> > > to register it again -- you have to allocate a new device and register
> > > it instead.
> > > 
> > > Also, although you call the driver's ->start method multiple times, the
> > > ->reset method is called only once, when the controller is first 
> > > probed.  It's not clear that this will work in a situation where the HC 
> > > and the UDC share hardware state; after the UDC is stopped it may be 
> > > necessary to reset the HC before it can run again.
> > > 
> > > It might be possible to make this work, but I suspect quite a few 
> > > drivers would need rewriting first.  As another example of the problems 
> > > you face, consider how stopping a host controller will interact with 
> > > the driver's PM support (both system suspend and runtime suspend).
> > > 
> > > It would be a lot simpler to unbind the host controller driver
> > > completely when switching to device mode and rebind it when switching
> > > back.  I guess that is the sort of heavy-duty approach you want to
> > > avoid, but it may be the only practical way forward.
> > 
> > Hmm from memory I think the OTG spec assumes the USB devices are
> > suspended when attempting the role change? I could be totally wrong,
> > it's been a really long time since I've looked at the OTG spec, but
> > maybe that would make it easier to deal with thing?
> 
> This patch deals with the host side, not the device side.  The fact
> that the device is suspended is not relevant to the issues above.

OK, got it.
 
> Besides, the problems I outlined are more connected with the way 
> Linux's host-side USB stack is organized, and not so much with the 
> details of the OTG spec.

Yes thanks for the explanation.

Regards,

Tony

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-18 13:55 ` [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop() Roger Quadros
@ 2015-03-19  3:30   ` Peter Chen
  2015-03-19 10:14     ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Peter Chen @ 2015-03-19  3:30 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
> The OTG state machine needs a mechanism to start and
> stop the gadget controller. Add usb_gadget_start()
> and usb_gadget_stop().
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
>  include/linux/usb/gadget.h        |   3 +
>  2 files changed, 158 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index 5a81cb0..69b4123 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -35,6 +35,8 @@
>   * @dev - the child device to the actual controller
>   * @gadget - the gadget. For use by the class code
>   * @list - for use by the udc class driver
> + * @running - udc is running

Doesn't OTG FSM should know it?

Peter
> + * @softconnect - sysfs softconnect says OK to connect
>   *
>   * This represents the internal data structure which is used by the UDC-class
>   * to hold information about udc driver and gadget together.
> @@ -44,6 +46,8 @@ struct usb_udc {
>  	struct usb_gadget		*gadget;
>  	struct device			dev;
>  	struct list_head		list;
> +	bool				running;
> +	bool				softconnect;
>  };
>  
>  static struct class *udc_class;
> @@ -187,7 +191,14 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>   */
>  static inline int usb_gadget_udc_start(struct usb_udc *udc)
>  {
> -	return udc->gadget->ops->udc_start(udc->gadget, udc->driver);
> +	int ret;
> +
> +	dev_dbg(&udc->dev, "%s\n", __func__);
> +	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
> +	if (!ret)
> +		udc->running = 1;
> +
> +	return ret;
>  }
>  
>  /**
> @@ -204,10 +215,140 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
>   */
>  static inline void usb_gadget_udc_stop(struct usb_udc *udc)
>  {
> +	dev_dbg(&udc->dev, "%s\n", __func__);
>  	udc->gadget->ops->udc_stop(udc->gadget);
> +	udc->running = 0;
>  }
>  
>  /**
> + * usb_udc_start - Start the usb device controller only if conditions met
> + * @udc: The UDC to be started
> + *
> + * Start the UDC and connect to bus (enable pull) only if the
> + * following conditions are met
> + * - UDC is not already running
> + * - gadget function driver is loaded
> + * - userspace softconnect says OK to connect
> + *
> + * NOTE: udc_lock must be held by caller.
> + *
> + * Returs 0 on success or not ready to start, else negative errno.
> + */
> +static int usb_udc_start(struct usb_udc *udc)
> +{
> +	int ret;
> +
> +	if (udc->running) {
> +		dev_err(&udc->dev, "%s: not starting as already running\n",
> +			__func__);
> +		return -EBUSY;
> +	}
> +
> +	if (udc->driver && udc->softconnect) {
> +		ret = usb_gadget_udc_start(udc);
> +		if (ret)
> +			return ret;
> +
> +		usb_gadget_connect(udc->gadget);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * usb_udc_stop - Stop the usb device controller
> + * @udc: The UDC to be stopped
> + *
> + * Disconnect from bus (disable pull) and stop the UDC.
> + *
> + * NOTE: udc_lock must be held by caller.
> + *
> + * Returs 0 on success, else negative errno.
> + */
> +static int usb_udc_stop(struct usb_udc *udc)
> +{
> +	if (!udc->running) {
> +		dev_err(&udc->dev, "%s: not stopping as already halted\n",
> +			__func__);
> +		return -EBUSY;
> +	}
> +
> +	usb_gadget_disconnect(udc->gadget);
> +	udc->driver->disconnect(udc->gadget);
> +	usb_gadget_udc_stop(udc);
> +
> +	return 0;
> +}
> +
> +/**
> + * usb_gadget_start - start the usb gadget controller
> + * @gadget: the gadget device to start
> + *
> + * This is external API for use by OTG core.
> + *
> + * Start the usb device controller and connect to bus (enable pull).
> + * There is no guarantee that the controller is started
> + * as we might be missing some dependencies
> + * i.e. gadget function driver not loaded or softconnect disabled.
> + */
> +int usb_gadget_start(struct usb_gadget *gadget)
> +{
> +	int ret;
> +	struct usb_udc *udc = NULL;
> +
> +	dev_dbg(&gadget->dev, "%s\n", __func__);
> +	mutex_lock(&udc_lock);
> +	list_for_each_entry(udc, &udc_list, list)
> +		if (udc->gadget == gadget)
> +			goto found;
> +
> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> +		__func__);
> +	mutex_unlock(&udc_lock);
> +	return -EINVAL;
> +
> +found:
> +	ret = usb_udc_start(udc);
> +	mutex_unlock(&udc_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_start);
> +
> +/**
> + * usb_gadget_stop - stop the usb gadget
> + * @gadget: The gadget device we want to stop
> + *
> + * This is external API for use by OTG core.
> + *
> + * Disconnect from the bus (disable pull) and stop the
> + * gadget controller.
> + */
> +int usb_gadget_stop(struct usb_gadget *gadget)
> +{
> +	int ret;
> +	struct usb_udc *udc = NULL;
> +
> +	dev_dbg(&gadget->dev, "%s\n", __func__);
> +	mutex_lock(&udc_lock);
> +	list_for_each_entry(udc, &udc_list, list)
> +		if (udc->gadget == gadget)
> +			goto found;
> +
> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> +		__func__);
> +	mutex_unlock(&udc_lock);
> +	return -EINVAL;
> +
> +found:
> +	ret = usb_udc_stop(udc);
> +	mutex_unlock(&udc_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_stop);
> +
> +/**
>   * usb_udc_release - release the usb_udc struct
>   * @dev: the dev member within usb_udc
>   *
> @@ -278,6 +419,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>  		goto err3;
>  
>  	udc->gadget = gadget;
> +	udc->softconnect = 1;	/* connect by default */
>  
>  	mutex_lock(&udc_lock);
>  	list_add_tail(&udc->list, &udc_list);
> @@ -329,10 +471,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  
> -	usb_gadget_disconnect(udc->gadget);
> -	udc->driver->disconnect(udc->gadget);
> +	usb_udc_stop(udc);
>  	udc->driver->unbind(udc->gadget);
> -	usb_gadget_udc_stop(udc);
>  
>  	udc->driver = NULL;
>  	udc->dev.driver = NULL;
> @@ -378,6 +518,7 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>  
>  /* ------------------------------------------------------------------------- */
>  
> +/* udc_lock must be held */
>  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
>  {
>  	int ret;
> @@ -392,12 +533,12 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>  	ret = driver->bind(udc->gadget, driver);
>  	if (ret)
>  		goto err1;
> -	ret = usb_gadget_udc_start(udc);
> +
> +	ret = usb_udc_start(udc);
>  	if (ret) {
>  		driver->unbind(udc->gadget);
>  		goto err1;
>  	}
> -	usb_gadget_connect(udc->gadget);
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  	return 0;
> @@ -510,12 +651,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
>  	}
>  
>  	if (sysfs_streq(buf, "connect")) {
> -		usb_gadget_udc_start(udc);
> -		usb_gadget_connect(udc->gadget);
> +		mutex_lock(&udc_lock);
> +		udc->softconnect = 1;
> +		usb_udc_start(udc);
> +		mutex_unlock(&udc_lock);
>  	} else if (sysfs_streq(buf, "disconnect")) {
> -		usb_gadget_disconnect(udc->gadget);
> -		udc->driver->disconnect(udc->gadget);
> -		usb_gadget_udc_stop(udc);
> +		mutex_lock(&udc_lock);
> +		udc->softconnect = 0;
> +		usb_udc_stop(udc);
> +		mutex_unlock(&udc_lock);
>  	} else {
>  		dev_err(dev, "unsupported command '%s'\n", buf);
>  		return -EINVAL;
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e2f00fd..7ada7e6 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -922,6 +922,9 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver);
>   */
>  int usb_gadget_unregister_driver(struct usb_gadget_driver *driver);
>  
> +int usb_gadget_start(struct usb_gadget *gadget);
> +int usb_gadget_stop(struct usb_gadget *gadget);
> +
>  extern int usb_add_gadget_udc_release(struct device *parent,
>  		struct usb_gadget *gadget, void (*release)(struct device *dev));
>  extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
> -- 
> 2.1.0
> 

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-18 13:55 ` [RFC][PATCH 3/9] usb: otg: add OTG core Roger Quadros
@ 2015-03-19  3:40   ` Peter Chen
  2015-03-19 10:18     ` Roger Quadros
  2015-03-19  8:26   ` Li Jun
  1 sibling, 1 reply; 40+ messages in thread
From: Peter Chen @ 2015-03-19  3:40 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
> The OTG core instantiates the OTG Finite State Machine
> per OTG controller and manages starting/stopping the
> host and gadget controllers based on the bus state.
> 
> It provides APIs for the following tasks
> 
> - Registering an OTG capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
> 
> TODO:
> - sysfs interface to allow application inputs to OTG state machine
> - otg class?
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/Makefile         |   1 +
>  drivers/usb/common/Makefile  |   1 +
>  drivers/usb/common/usb-otg.c | 732 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/common/usb-otg.h |  71 +++++
>  drivers/usb/core/Kconfig     |   8 +
>  include/linux/usb/usb-otg.h  |  86 +++++
>  6 files changed, 899 insertions(+)
>  create mode 100644 drivers/usb/common/usb-otg.c
>  create mode 100644 drivers/usb/common/usb-otg.h
>  create mode 100644 include/linux/usb/usb-otg.h
> 
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 2f1e2aa..07f59a5 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -60,5 +60,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
>  obj-$(CONFIG_USB_GADGET)	+= gadget/
>  
>  obj-$(CONFIG_USB_COMMON)	+= common/
> +obj-$(CONFIG_USB_OTG_CORE)	+= common/
>  
>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
> index ca2f8bd..573fc75 100644
> --- a/drivers/usb/common/Makefile
> +++ b/drivers/usb/common/Makefile
> @@ -7,3 +7,4 @@ usb-common-y			  += common.o
>  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>  
>  obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
> +obj-$(CONFIG_USB_OTG_CORE) += usb-otg.o
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> new file mode 100644
> index 0000000..1433fc9
> --- /dev/null
> +++ b/drivers/usb/common/usb-otg.c
> @@ -0,0 +1,732 @@
> +/**
> + * drivers/usb/common/usb-otg.c - USB OTG core
> + *
> + * Copyright (C) 2015 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/timer.h>
> +#include <linux/usb/otg.h>
> +#include <linux/usb/phy.h>	/* enum usb_otg_state */
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/usb-otg.h>
> +#include <linux/workqueue.h>
> +
> +#include "usb-otg.h"
> +
> +/* to link timer with callback data */
> +struct otg_timer {
> +	struct timer_list timer;
> +	/* callback data */
> +	int *timeout_bit;
> +	struct otg_data *otgd;
> +};
> +
> +struct otg_data {
> +	struct device *dev;	/* HCD & GCD's parent device */
> +
> +	struct otg_fsm fsm;
> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
> +	 * HCD is bus_to_hcd(fsm->otg->host)
> +	 * GCD is fsm->otg->gadget
> +	 */
> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
> +	struct usb_otg otg;
> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
> +
> +	/* saved hooks to OTG device */
> +	int (*start_host)(struct otg_fsm *fsm, int on);
> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
> +
> +	struct list_head list;
> +
> +	struct work_struct work;	/* OTG FSM work */
> +	struct workqueue_struct *wq;
> +
> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
> +
> +	bool fsm_running;
> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
> +	bool host_can_start;		/* OTG FSM says host can start */
> +
> +	/* use otg->fsm.lock for serializing access */
> +};

What's the relationship between struct usb_otg otg and this one?

> +
> +/* OTG device list */
> +LIST_HEAD(otg_list);
> +static DEFINE_MUTEX(otg_list_mutex);
> +
> +/**
> + * check if device is in our OTG list and return
> + * otg_data, else NULL.
> + *
> + * otg_list_mutex must be held.
> + */
> +static struct otg_data *usb_otg_device_get_otgd(struct device *parent_dev)
> +{
> +	struct otg_data *otgd;
> +
> +	list_for_each_entry(otgd, &otg_list, list) {
> +		if (otgd->dev == parent_dev)
> +			return otgd;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * timer callback to set timeout bit and kick FSM
> + */
> +static void set_tmout(unsigned long data)
> +{
> +	struct otg_timer *tmr_data;
> +
> +	tmr_data = (struct otg_timer *)data;
> +
> +	if (tmr_data->timeout_bit)
> +		*tmr_data->timeout_bit = 1;
> +
> +	usb_otg_sync_inputs(&tmr_data->otgd->fsm);
> +}
> +
> +/**
> + * Initialize one OTG timer with callback, timeout and timeout bit
> + */
> +static void otg_timer_init(enum otg_fsm_timer id, struct otg_data *otgd,
> +			   void (*callback)(unsigned long),
> +			   unsigned long expires_ms,
> +			   int *timeout_bit)
> +{
> +	struct otg_timer *otgtimer = &otgd->timers[id];
> +	struct timer_list *timer = &otgtimer->timer;
> +
> +	init_timer(timer);
> +	timer->function = callback;
> +	timer->expires = jiffies + msecs_to_jiffies(expires_ms);
> +	timer->data = (unsigned long)otgtimer;
> +

The timer for TB_DATA_PLS is about 10ms or less, it is not suitable
for using kernel timer, hrtimer is suitable choice.

> +	otgtimer->timeout_bit = timeout_bit;
> +	otgtimer->otgd = otgd;
> +}
> +
> +/**
> + * Initialize standard OTG timers
> + */
> +static void usb_otg_init_timers(struct otg_data *otgd)
> +{
> +	struct otg_fsm *fsm = &otgd->fsm;
> +
> +	otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE, &fsm->a_wait_vrise_tmout);
> +	otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL, &fsm->a_wait_vfall_tmout);
> +	otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON, &fsm->a_wait_bcon_tmout);
> +	otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS, &fsm->a_aidl_bdis_tmout);
> +	otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS, &fsm->a_bidl_adis_tmout);
> +	otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST, &fsm->b_ase0_brst_tmout);
> +
> +	otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP, &fsm->b_se0_srp);
> +	otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL, &fsm->b_srp_done);
> +
> +	otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
> +}
> +
> +/**
> + * OTG FSM ops function to add timer
> + */
> +static void usb_otg_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +	struct timer_list *timer = &otgd->timers[id].timer;
> +
> +	if (!otgd->fsm_running)
> +		return;
> +
> +	/* if timer is already active, exit */
> +	if (timer_pending(timer)) {
> +		dev_err(otgd->dev, "USB-OTG: Timer %d is already running\n",
> +			id);
> +	}
> +
> +	add_timer(timer);
> +}
> +
> +/**
> + * OTG FSM ops function to delete timer
> + */
> +static void usb_otg_del_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +	struct timer_list *timer = &otgd->timers[id].timer;
> +
> +	del_timer(timer);
> +}
> +
> +/**
> + * OTG FSM ops function to start/stop host
> + */
> +static int usb_otg_start_host(struct otg_fsm *fsm, int on)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +	int ret = 0;
> +	struct usb_hcd *hcd;
> +
> +	dev_info(otgd->dev, "%s %d\n", __func__, on);
> +	if (!fsm->otg->host) {
> +		WARN_ONCE(1, "OTG FSM running without Host\n");
> +		return 0;
> +	}
> +
> +	hcd = bus_to_hcd(fsm->otg->host);
> +	otgd->host_can_start = on;
> +	if (hcd->shared_hcd && !otgd->shared_hcd) {
> +		dev_info(otgd->dev, "%s: waiting for Shared HCD to register\n",
> +			 __func__);
> +		return 0;
> +	}
> +
> +	/* call OTG device operations */
> +	if (otgd->start_host) {
> +		ret = otgd->start_host(fsm, on);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (on) {
> +		usb_start_hcd(hcd);
> +		if (hcd->shared_hcd)
> +			usb_start_hcd(hcd->shared_hcd);
> +	} else {
> +		if (hcd->shared_hcd)
> +			usb_stop_hcd(hcd->shared_hcd);
> +		usb_stop_hcd(hcd);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * OTG FSM ops function to start/stop gadget
> + */
> +static int usb_otg_start_gadget(struct otg_fsm *fsm, int on)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +	int ret = 0;
> +	struct usb_gadget *gadget = fsm->otg->gadget;
> +
> +	dev_info(otgd->dev, "%s %d\n", __func__, on);
> +	if (!gadget) {
> +		WARN_ONCE(1, "OTG FSM running without Gadget\n");
> +		return 0;
> +	}
> +
> +	/* call OTG device operations */
> +	if (otgd->start_gadget) {
> +		ret = otgd->start_gadget(fsm, on);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	otgd->gadget_can_start = on;
> +	if (on)
> +		usb_gadget_start(fsm->otg->gadget);
> +	else
> +		usb_gadget_stop(fsm->otg->gadget);
> +
> +	return 0;
> +}
> +
> +/**
> + * OTG FSM work function
> + */
> +static void usb_otg_work(struct work_struct *work)
> +{
> +	struct otg_data *otgd = container_of(work, struct otg_data, work);
> +
> +	if (otg_statemachine(&otgd->fsm)) {
> +		/* state changed. any action ? */
> +	}
> +}
> +
> +/**
> + * usb_otg_register() - Register the OTG device to OTG core
> + * @parent_device:	parent device of Host & Gadget controllers.
> + * @otg_fsm_ops:	otg state machine ops.
> + *
> + * Register parent device that contains both HCD and GCD into
> + * the USB OTG core. HCD and GCD will be prevented from starting
> + * till both are available for use.
> + *
> + * Return: struct otg_fsm * if success, NULL if error.
> + */
> +struct otg_fsm *usb_otg_register(struct device *parent_dev,
> +				 struct otg_fsm_ops *fsm_ops)
> +{
> +	struct otg_data *otgd;
> +	int ret = 0;
> +
> +	if (!parent_dev || !fsm_ops)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* already in list? */
> +	mutex_lock(&otg_list_mutex);
> +	if (usb_otg_device_get_otgd(parent_dev)) {
> +		dev_err(parent_dev, "%s: device already in OTG list\n",
> +			__func__);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	/* allocate and add to list */
> +	otgd = kzalloc(sizeof(*otgd), GFP_KERNEL);
> +	if (!otgd) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	otgd->dev = parent_dev;
> +	INIT_WORK(&otgd->work, usb_otg_work);
> +	otgd->wq = create_singlethread_workqueue("usb_otg");
> +	if (!otgd->wq) {
> +		dev_err(parent_dev, "%s: can't create workqueue\n", __func__);
> +		ret = -ENODEV;
> +		goto err_wq;
> +	}
> +
> +	usb_otg_init_timers(otgd);
> +
> +	/* save original start host/gadget ops */
> +	otgd->start_host = fsm_ops->start_host;
> +	otgd->start_gadget = fsm_ops->start_gadget;
> +	/* create copy of original ops */
> +	otgd->fsm_ops = *fsm_ops;
> +	/* override ops */
> +	otgd->fsm_ops.start_host = usb_otg_start_host;
> +	otgd->fsm_ops.start_gadget = usb_otg_start_gadget;
> +	/* FIXME: we ignore caller's timer ops */
> +	otgd->fsm_ops.add_timer = usb_otg_add_timer;
> +	otgd->fsm_ops.del_timer = usb_otg_del_timer;
> +	/* set otg ops */
> +	otgd->fsm.ops = &otgd->fsm_ops;
> +	otgd->fsm.otg = &otgd->otg;
> +
> +	mutex_init(&otgd->fsm.lock);
> +
> +	list_add_tail(&otgd->list, &otg_list);
> +	mutex_unlock(&otg_list_mutex);
> +	return &otgd->fsm;
> +
> +err_wq:
> +	kfree(otgd);
> +unlock:
> +	mutex_unlock(&otg_list_mutex);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register);
> +
> +/**
> + * usb_otg_unregister() - Unregister the OTG device from USB OTG core
> + * @parent_device:	parent device of Host & Gadget controllers.
> + *
> + * Unregister parent OTG deviced from USB OTG core.
> + * Prevents unregistering till both Host and Gadget controllers
> + * have unregistered from the OTG core.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister(struct device *parent_dev)
> +{
> +	struct otg_data *otgd;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(parent_dev);
> +	if (!otgd) {
> +		dev_err(parent_dev, "%s: device not in OTG list\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* prevent unregister till both host & gadget have unregistered */
> +	if (otgd->fsm.otg->host || otgd->fsm.otg->gadget) {
> +		dev_err(parent_dev, "%s: host/gadget still registered\n",
> +			__func__);
> +		return -EBUSY;
> +	}
> +
> +	/* OTG FSM is halted when host/gadget unregistered */
> +	destroy_workqueue(otgd->wq);
> +
> +	/* remove from otg list */
> +	list_del(&otgd->list);
> +	kfree(otgd);
> +	mutex_unlock(&otg_list_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister);
> +
> +/**
> + * start/kick the OTG FSM if we can
> + * fsm->lock must be held
> + */
> +static void usb_otg_start_fsm(struct otg_fsm *fsm)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +
> +	if (otgd->fsm_running)
> +		goto kick_fsm;
> +
> +	if (!fsm->otg->host) {
> +		dev_info(otgd->dev, "Not starting OTG till Host registers\n");
> +		return;
> +	}
> +
> +	if (!fsm->otg->gadget) {
> +		dev_info(otgd->dev, "Not starting OTG till Gadget registers\n");
> +		return;
> +	}
> +
> +	otgd->fsm_running = true;
> +kick_fsm:
> +	queue_work(otgd->wq, &otgd->work);
> +}
> +
> +/**
> + * stop the OTG FSM. Stops Host & Gadget controllers as well.
> + * fsm->lock must be held
> + */
> +static void usb_otg_stop_fsm(struct otg_fsm *fsm)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +	int i;
> +
> +	if (!otgd->fsm_running)
> +		return;
> +
> +	/* no more new events queued */
> +	otgd->fsm_running = false;
> +
> +	/* Stop state machine / timers */
> +	for (i = 0; i < ARRAY_SIZE(otgd->timers); i++)
> +		del_timer_sync(&otgd->timers[i].timer);
> +
> +	flush_workqueue(otgd->wq);
> +
> +	/* stop host/gadget immediately */
> +	if (fsm->protocol == PROTO_HOST)
> +		otg_start_host(fsm, 0);
> +	else if (fsm->protocol == PROTO_GADGET)
> +		otg_start_gadget(fsm, 0);
> +}
> +
> +/**
> + * usb_otg_sync_inputs - Sync OTG inputs with the OTG state machine
> + * @fsm:	OTG FSM 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 otg_fsm *fsm)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +
> +	/* Don't kick FSM till it has started */
> +	if (!otgd->fsm_running)
> +		return;
> +
> +	/* Kick FSM */
> +	queue_work(otgd->wq, &otgd->work);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
> +
> +/**
> + * usb_otg_kick_fsm - Kick the OTG state machine
> + * @hcd_gcd_device:	Host/Gadget controller device
> + *
> + * Used by USB host/device stack to sync OTG related
> + * events to the OTG state machine.
> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_kick_fsm(struct device *hcd_gcd_device)
> +{
> +	struct otg_data *otgd;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(hcd_gcd_device->parent);
> +	if (!otgd) {
> +		dev_err(hcd_gcd_device, "%s: invalid host/gadget device\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -ENODEV;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +	usb_otg_sync_inputs(&otgd->fsm);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
> +
> +/**
> + * usb_otg_register_hcd - Register Host controller to OTG core
> + * @hcd:	Host controller device
> + *
> + * 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 upto the OTG state machine to do so.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_register_hcd(struct usb_hcd *hcd)
> +{
> +	struct otg_data *otgd;
> +	struct usb_bus *bus = hcd_to_bus(hcd);
> +	struct device *otg_dev = bus->controller->parent;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(otg_dev);
> +	if (!otgd) {
> +		dev_err(otg_dev, "%s: device not registered to OTG core\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +	/* HCD will be started by OTG fsm when needed */
> +	mutex_lock(&otgd->fsm.lock);
> +	if (otgd->fsm.otg->host) {
> +		/* probably a shared hcd ? */
> +		if (usb_hcd_is_primary_hcd(hcd)) {
> +			dev_err(otg_dev, "%s: hcd already registered\n",
> +				__func__);
> +			goto err;
> +		}
> +
> +		if (hcd->shared_hcd == bus_to_hcd(otgd->fsm.otg->host)) {
> +			if (otgd->shared_hcd) {
> +				dev_err(otg_dev, "%s: shared HCD already registered\n",
> +					__func__);
> +				goto err;
> +			}
> +
> +			otgd->shared_hcd = hcd;
> +			dev_info(otg_dev, "USB-OTG: Shared Host %s registered\n",
> +				 dev_name(bus->controller));
> +			/* we might be pending a start_host */
> +			if (otgd->host_can_start)
> +				usb_otg_start_host(&otgd->fsm, 1);
> +			goto unlock;
> +		} else {
> +			dev_err(otg_dev, "%s: invalid shared hcd\n",
> +					__func__);
> +			goto err;
> +		}
> +	}
> +
> +	otgd->fsm.otg->host = bus;
> +
> +	/* FIXME: set bus->otg_port if this is true OTG port with HNP */
> +	dev_info(otg_dev, "USB-OTG: Primary Host %s registered\n",
> +		 dev_name(bus->controller));
> +
> +unlock:
> +	/* Kick FSM */
> +	usb_otg_start_fsm(&otgd->fsm);
> +	mutex_unlock(&otgd->fsm.lock);
> +
> +	return 0;
> +
> +err:
> +	mutex_unlock(&otgd->fsm.lock);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
> +
> +/**
> + * usb_otg_unregister_hcd - Unregister 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 otg_data *otgd;
> +	struct usb_bus *bus = hcd_to_bus(hcd);
> +	struct device *otg_dev = bus->controller->parent;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(otg_dev);
> +	if (!otgd) {
> +		dev_err(otg_dev, "%s: device not registered to OTG core\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +
> +	mutex_lock(&otgd->fsm.lock);
> +	if (otgd->fsm.otg->host != bus) {
> +		dev_err(otg_dev, "%s: host wansn't registered with OTG\n",
> +			__func__);
> +		mutex_unlock(&otgd->fsm.lock);
> +		return -EINVAL;
> +	}
> +
> +	/* stop FSM & Host */
> +	usb_otg_stop_fsm(&otgd->fsm);
> +	otgd->fsm.otg->host = NULL;
> +
> +	mutex_unlock(&otgd->fsm.lock);
> +	dev_info(otg_dev, "USB-OTG: Host %s unregistered\n",
> +		 dev_name(bus->controller));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister_hcd);
> +
> +/**
> + * usb_otg_register_gadget - Register Gadget controller to OTG core
> + * @gadget:	Gadget controller
> + *
> + * 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 upto the OTG state machine to do so.
> + *
> + * As gadget controller needs to be prevented from starting till other
> + * conditions are met (i.e. driver loaded, softconnet enabled),
> + * the gadget stack can use the usb_otg_gadget_can_start() function
> + * to check if OTG core has given the permission to start the gadget.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_register_gadget(struct usb_gadget *gadget)
> +{
> +	struct otg_data *otgd;
> +	struct device *otg_dev = gadget->dev.parent;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(otg_dev);
> +	if (!otgd) {
> +		dev_err(otg_dev, "%s: device not registered to OTG core\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +
> +	mutex_lock(&otgd->fsm.lock);
> +	if (otgd->fsm.otg->gadget) {
> +		dev_err(otg_dev, "%s: gadget already registered with OTG\n",
> +			__func__);
> +		mutex_unlock(&otgd->fsm.lock);
> +		return -EINVAL;
> +	}
> +
> +	otgd->fsm.otg->gadget = gadget;
> +	dev_info(otg_dev, "USB-OTG: Gadget %s registered\n",
> +		 dev_name(&gadget->dev));
> +
> +	/* Kick FSM */
> +	usb_otg_start_fsm(&otgd->fsm);
> +	mutex_unlock(&otgd->fsm.lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register_gadget);
> +
> +/**
> + * usb_otg_unregister_gadget - Unregister 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 otg_data *otgd;
> +	struct device *otg_dev = gadget->dev.parent;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(otg_dev);
> +	if (!otgd) {
> +		dev_err(otg_dev, "%s: device not registered to OTG core\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +
> +	mutex_lock(&otgd->fsm.lock);
> +	if (otgd->fsm.otg->gadget != gadget) {
> +		dev_err(otg_dev, "%s: gadget wasn't registered with OTG\n",
> +			__func__);
> +		mutex_unlock(&otgd->fsm.lock);
> +		return -EINVAL;
> +	}
> +
> +	/* Stop FSM & gadget */
> +	usb_otg_stop_fsm(&otgd->fsm);
> +	otgd->fsm.otg->gadget = NULL;
> +	mutex_unlock(&otgd->fsm.lock);
> +
> +	dev_info(otg_dev, "USB-OTG: Gadget %s unregistered\n",
> +		 dev_name(&gadget->dev));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister_gadget);
> +
> +/**
> + * usb_otg_gadget_can_start - check if Gadget can be started from OTG perspective
> + * @gadget:	Gadget controller
> + *
> + * This is used by the USB Gadget stack to check if Gadget controller
> + * can be started from the OTG state machines perspective.
> + * i.e. we're in 'b_peripheral' state.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +bool usb_otg_gadget_can_start(struct usb_gadget *gadget)
> +{
> +	struct otg_data *otgd;
> +	struct device *otg_dev = gadget->dev.parent;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(otg_dev);
> +	if (!otgd) {
> +		dev_err(otg_dev, "%s: device not registered to OTG core\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -EINVAL;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +
> +	return otgd->gadget_can_start;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_gadget_can_start);
> diff --git a/drivers/usb/common/usb-otg.h b/drivers/usb/common/usb-otg.h
> new file mode 100644
> index 0000000..05331f0
> --- /dev/null
> +++ b/drivers/usb/common/usb-otg.h
> @@ -0,0 +1,71 @@
> +/**
> + * drivers/usb/common/usb-otg.h - USB OTG core local header
> + *
> + * Copyright (C) 2015 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.
> + */
> +
> +#ifndef __DRIVERS_USB_COMMON_USB_OTG_H
> +#define __DRIVERS_USB_COMMON_USB_OTG_H
> +
> +/*
> + *  A-DEVICE timing constants
> + */
> +
> +/* Wait for VBUS Rise  */
> +#define TA_WAIT_VRISE        (100)	/* a_wait_vrise: section 7.1.2
> +					 * a_wait_vrise_tmr: section 7.4.5.1
> +					 * TA_VBUS_RISE <= 100ms, section 4.4
> +					 * Table 4-1: Electrical Characteristics
> +					 * ->DC Electrical Timing
> +					 */
> +/* Wait for VBUS Fall  */
> +#define TA_WAIT_VFALL        (1000)	/* a_wait_vfall: section 7.1.7
> +					 * a_wait_vfall_tmr: section: 7.4.5.2
> +					 */
> +/* Wait for B-Connect */
> +#define TA_WAIT_BCON         (10000)	/* a_wait_bcon: section 7.1.3
> +					 * TA_WAIT_BCON: should be between 1100
> +					 * and 30000 ms, section 5.5, Table 5-1
> +					 */
> +/* A-Idle to B-Disconnect */
> +#define TA_AIDL_BDIS         (5000)	/* a_suspend min 200 ms, section 5.2.1
> +					 * TA_AIDL_BDIS: section 5.5, Table 5-1
> +					 */
> +/* B-Idle to A-Disconnect */
> +#define TA_BIDL_ADIS         (500)	/* TA_BIDL_ADIS: section 5.2.1
> +					 * 500ms is used for B switch to host
> +					 * for safe
> +					 */
> +
> +/*
> + * B-device timing constants
> + */
> +
> +/* Data-Line Pulse Time*/
> +#define TB_DATA_PLS          (10)	/* b_srp_init,continue 5~10ms
> +					 * section:5.1.3
> +					 */
> +/* SRP Fail Time  */
> +#define TB_SRP_FAIL          (6000)	/* b_srp_init,fail time 5~6s
> +					 * section:5.1.6
> +					 */
> +/* A-SE0 to B-Reset  */
> +#define TB_ASE0_BRST         (155)	/* minimum 155 ms, section:5.3.1 */
> +/* SE0 Time Before SRP */
> +#define TB_SE0_SRP           (1000)	/* b_idle,minimum 1s, section:5.1.2 */
> +/* SSEND time before SRP */
> +#define TB_SSEND_SRP         (1500)	/* minimum 1.5 sec, section:5.1.2 */
> +
> +#define TB_SESS_VLD          (1000)
> +
> +#endif /* __DRIVERS_USB_COMMON_USB_OTG_H */
> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> index cc0ced0..43a0d2d 100644
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -84,3 +84,11 @@ config USB_OTG_FSM
>  	  Implements OTG Finite State Machine as specified in On-The-Go
>  	  and Embedded Host Supplement to the USB Revision 2.0 Specification.
>  
> +config USB_OTG_CORE
> +	tristate "USB OTG core"
> +	depends on USB
> +	select USB_OTG_FSM
> +	help
> +	  Standardize the way OTG is implemented on Linux. The OTG state
> +	  machine is instantiated here instead of being done in each controller
> +	  driver.
> diff --git a/include/linux/usb/usb-otg.h b/include/linux/usb/usb-otg.h
> new file mode 100644
> index 0000000..52ab7b1
> --- /dev/null
> +++ b/include/linux/usb/usb-otg.h
> @@ -0,0 +1,86 @@
> +/**
> + * include/linux/usb/usb-otg.h - USB OTG core
> + *
> + * Copyright (C) 2015 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.
> + */
> +
> +#ifndef __LINUX_USB_OTG_CORE_H
> +#define __LINUX_USB_OTG_CORE_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>
> +
> +#if IS_ENABLED(CONFIG_USB_OTG_CORE)
> +struct otg_fsm *usb_otg_register(struct device *parent_dev,
> +				 struct otg_fsm_ops *fsm_ops);
> +int usb_otg_unregister(struct device *parent_dev);
> +int usb_otg_register_hcd(struct usb_hcd *hcd);
> +int usb_otg_unregister_hcd(struct usb_hcd *hcd);
> +int usb_otg_register_gadget(struct usb_gadget *gadget);
> +int usb_otg_unregister_gadget(struct usb_gadget *gadget);
> +void usb_otg_sync_inputs(struct otg_fsm *fsm);
> +int usb_otg_kick_fsm(struct device *hcd_gcd_device);
> +bool usb_otg_gadget_can_start(struct usb_gadget *gadget);
> +
> +#else /* CONFIG_USB_OTG_CORE */
> +
> +static inline struct otg_fsm *usb_otg_register(struct device *parent_dev,
> +					       struct otg_fsm_ops *fsm_ops)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
> +static inline int usb_otg_unregister(struct device *parent_dev)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int usb_otg_register_hcd(struct usb_hcd *hcd)
> +{
> +	return -ENOSYS;
> +}
> +
> +int usb_otg_unregister_hcd(struct usb_hcd *hcd)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int usb_otg_register_gadget(struct usb_gadget *gadget)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int usb_otg_unregister_gadget(struct usb_gadget *gadget)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void usb_otg_sync_inputs(struct otg_fsm *fsm)
> +{
> +}
> +
> +int usb_otg_kick_fsm(struct device *hcd_gcd_device)
> +{
> +	return -ENOSYS;
> +}
> +
> +bool usb_otg_gadget_can_start(struct usb_gadget *gadget)
> +{
> +	return true;
> +}
> +#endif /* CONFIG_USB_OTG_CORE */
> +
> +#endif /* __LINUX_USB_OTG_CORE */
> -- 
> 2.1.0
> 

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm
  2015-03-18 13:56 ` [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm Roger Quadros
@ 2015-03-19  3:46   ` Peter Chen
  2015-03-19 10:20     ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Peter Chen @ 2015-03-19  3:46 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Wed, Mar 18, 2015 at 03:56:02PM +0200, Roger Quadros wrote:
> These members are not used anywhere so remove them.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  include/linux/usb/otg-fsm.h | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
> index b6ba1bf..176c4fc 100644
> --- a/include/linux/usb/otg-fsm.h
> +++ b/include/linux/usb/otg-fsm.h
> @@ -95,11 +95,6 @@ struct otg_fsm {
>  	int b_hnp_enable;
>  	int a_clr_err;
>  
> -	/* Informative variables */
> -	int a_bus_drop_inf;
> -	int a_bus_req_inf;
> -	int a_clr_err_inf;
> -	int b_bus_req_inf;
>  	/* Auxilary informative variables */
>  	int a_suspend_req_inf;
>  

But the above are defined at: ch 7.4.4, On-The-Go and Embedded Host
Supplement to the USB Revision 2.0 Specification

> -- 
> 2.1.0
> 

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-18 13:55 ` [RFC][PATCH 3/9] usb: otg: add OTG core Roger Quadros
  2015-03-19  3:40   ` Peter Chen
@ 2015-03-19  8:26   ` Li Jun
  2015-03-19 10:30     ` Roger Quadros
  1 sibling, 1 reply; 40+ messages in thread
From: Li Jun @ 2015-03-19  8:26 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
> The OTG core instantiates the OTG Finite State Machine
> per OTG controller and manages starting/stopping the
> host and gadget controllers based on the bus state.
> 
> It provides APIs for the following tasks
> 
> - Registering an OTG capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
> 
> TODO:
> - sysfs interface to allow application inputs to OTG state machine
> - otg class?
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> +struct otg_data {
> +	struct device *dev;	/* HCD & GCD's parent device */
> +
> +	struct otg_fsm fsm;
> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
> +	 * HCD is bus_to_hcd(fsm->otg->host)
> +	 * GCD is fsm->otg->gadget
> +	 */
> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
> +	struct usb_otg otg;
> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
> +
> +	/* saved hooks to OTG device */
> +	int (*start_host)(struct otg_fsm *fsm, int on);
> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
> +
> +	struct list_head list;
> +
> +	struct work_struct work;	/* OTG FSM work */
> +	struct workqueue_struct *wq;
> +
> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
> +
> +	bool fsm_running;
> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
> +	bool host_can_start;		/* OTG FSM says host can start */

Do not understand above 2 *_can_start flags from the patch, which are set
only when you really start host/gadget, to prevent host/gadget driver to
start it out of otg fsm control?

Li Jun
> +
> +	/* use otg->fsm.lock for serializing access */
> +};
> +
> + * Can be called in IRQ context.
> + */
> +void usb_otg_sync_inputs(struct otg_fsm *fsm)
> +{
> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> +
> +	/* Don't kick FSM till it has started */
> +	if (!otgd->fsm_running)
> +		return;
> +
> +	/* Kick FSM */
> +	queue_work(otgd->wq, &otgd->work);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
> +
> +/**
> + * usb_otg_kick_fsm - Kick the OTG state machine
> + * @hcd_gcd_device:	Host/Gadget controller device
> + *
> + * Used by USB host/device stack to sync OTG related
> + * events to the OTG state machine.
> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
> + *
There are quite a few otg fsm variables which should be updated when 
events/interrupts(b_conn, a_srp_det, ...) occur, how is your plan to
update them? Still rely on specific controller driver irq handler to
capture all those events and update them?

Li Jun
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_kick_fsm(struct device *hcd_gcd_device)
> +{
> +	struct otg_data *otgd;
> +
> +	mutex_lock(&otg_list_mutex);
> +	otgd = usb_otg_device_get_otgd(hcd_gcd_device->parent);
> +	if (!otgd) {
> +		dev_err(hcd_gcd_device, "%s: invalid host/gadget device\n",
> +			__func__);
> +		mutex_unlock(&otg_list_mutex);
> +		return -ENODEV;
> +	}
> +
> +	mutex_unlock(&otg_list_mutex);
> +	usb_otg_sync_inputs(&otgd->fsm);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
> -- 
> 2.1.0
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-19  3:30   ` Peter Chen
@ 2015-03-19 10:14     ` Roger Quadros
  2015-03-19 14:09       ` Li Jun
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 10:14 UTC (permalink / raw)
  To: Peter Chen
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On 19/03/15 05:30, Peter Chen wrote:
> On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
>> The OTG state machine needs a mechanism to start and
>> stop the gadget controller. Add usb_gadget_start()
>> and usb_gadget_stop().
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
>>  include/linux/usb/gadget.h        |   3 +
>>  2 files changed, 158 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>> index 5a81cb0..69b4123 100644
>> --- a/drivers/usb/gadget/udc/udc-core.c
>> +++ b/drivers/usb/gadget/udc/udc-core.c
>> @@ -35,6 +35,8 @@
>>   * @dev - the child device to the actual controller
>>   * @gadget - the gadget. For use by the class code
>>   * @list - for use by the udc class driver
>> + * @running - udc is running
> 
> Doesn't OTG FSM should know it?

Not really, as the gadget driver might not have been loaded yet or userspace might
have disabled softconnect when the OTG FSM wants UDC to start.

So only UDC knows if it has really started or not based on this flag.

cheers,
-roger

> 
> Peter
>> + * @softconnect - sysfs softconnect says OK to connect
>>   *
>>   * This represents the internal data structure which is used by the UDC-class
>>   * to hold information about udc driver and gadget together.
>> @@ -44,6 +46,8 @@ struct usb_udc {
>>  	struct usb_gadget		*gadget;
>>  	struct device			dev;
>>  	struct list_head		list;
>> +	bool				running;
>> +	bool				softconnect;
>>  };
>>  
>>  static struct class *udc_class;
>> @@ -187,7 +191,14 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>>   */
>>  static inline int usb_gadget_udc_start(struct usb_udc *udc)
>>  {
>> -	return udc->gadget->ops->udc_start(udc->gadget, udc->driver);
>> +	int ret;
>> +
>> +	dev_dbg(&udc->dev, "%s\n", __func__);
>> +	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
>> +	if (!ret)
>> +		udc->running = 1;
>> +
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -204,10 +215,140 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
>>   */
>>  static inline void usb_gadget_udc_stop(struct usb_udc *udc)
>>  {
>> +	dev_dbg(&udc->dev, "%s\n", __func__);
>>  	udc->gadget->ops->udc_stop(udc->gadget);
>> +	udc->running = 0;
>>  }
>>  
>>  /**
>> + * usb_udc_start - Start the usb device controller only if conditions met
>> + * @udc: The UDC to be started
>> + *
>> + * Start the UDC and connect to bus (enable pull) only if the
>> + * following conditions are met
>> + * - UDC is not already running
>> + * - gadget function driver is loaded
>> + * - userspace softconnect says OK to connect
>> + *
>> + * NOTE: udc_lock must be held by caller.
>> + *
>> + * Returs 0 on success or not ready to start, else negative errno.
>> + */
>> +static int usb_udc_start(struct usb_udc *udc)
>> +{
>> +	int ret;
>> +
>> +	if (udc->running) {
>> +		dev_err(&udc->dev, "%s: not starting as already running\n",
>> +			__func__);
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (udc->driver && udc->softconnect) {
>> +		ret = usb_gadget_udc_start(udc);
>> +		if (ret)
>> +			return ret;
>> +
>> +		usb_gadget_connect(udc->gadget);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * usb_udc_stop - Stop the usb device controller
>> + * @udc: The UDC to be stopped
>> + *
>> + * Disconnect from bus (disable pull) and stop the UDC.
>> + *
>> + * NOTE: udc_lock must be held by caller.
>> + *
>> + * Returs 0 on success, else negative errno.
>> + */
>> +static int usb_udc_stop(struct usb_udc *udc)
>> +{
>> +	if (!udc->running) {
>> +		dev_err(&udc->dev, "%s: not stopping as already halted\n",
>> +			__func__);
>> +		return -EBUSY;
>> +	}
>> +
>> +	usb_gadget_disconnect(udc->gadget);
>> +	udc->driver->disconnect(udc->gadget);
>> +	usb_gadget_udc_stop(udc);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * usb_gadget_start - start the usb gadget controller
>> + * @gadget: the gadget device to start
>> + *
>> + * This is external API for use by OTG core.
>> + *
>> + * Start the usb device controller and connect to bus (enable pull).
>> + * There is no guarantee that the controller is started
>> + * as we might be missing some dependencies
>> + * i.e. gadget function driver not loaded or softconnect disabled.
>> + */
>> +int usb_gadget_start(struct usb_gadget *gadget)
>> +{
>> +	int ret;
>> +	struct usb_udc *udc = NULL;
>> +
>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>> +	mutex_lock(&udc_lock);
>> +	list_for_each_entry(udc, &udc_list, list)
>> +		if (udc->gadget == gadget)
>> +			goto found;
>> +
>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +		__func__);
>> +	mutex_unlock(&udc_lock);
>> +	return -EINVAL;
>> +
>> +found:
>> +	ret = usb_udc_start(udc);
>> +	mutex_unlock(&udc_lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_gadget_start);
>> +
>> +/**
>> + * usb_gadget_stop - stop the usb gadget
>> + * @gadget: The gadget device we want to stop
>> + *
>> + * This is external API for use by OTG core.
>> + *
>> + * Disconnect from the bus (disable pull) and stop the
>> + * gadget controller.
>> + */
>> +int usb_gadget_stop(struct usb_gadget *gadget)
>> +{
>> +	int ret;
>> +	struct usb_udc *udc = NULL;
>> +
>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>> +	mutex_lock(&udc_lock);
>> +	list_for_each_entry(udc, &udc_list, list)
>> +		if (udc->gadget == gadget)
>> +			goto found;
>> +
>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +		__func__);
>> +	mutex_unlock(&udc_lock);
>> +	return -EINVAL;
>> +
>> +found:
>> +	ret = usb_udc_stop(udc);
>> +	mutex_unlock(&udc_lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_gadget_stop);
>> +
>> +/**
>>   * usb_udc_release - release the usb_udc struct
>>   * @dev: the dev member within usb_udc
>>   *
>> @@ -278,6 +419,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
>>  		goto err3;
>>  
>>  	udc->gadget = gadget;
>> +	udc->softconnect = 1;	/* connect by default */
>>  
>>  	mutex_lock(&udc_lock);
>>  	list_add_tail(&udc->list, &udc_list);
>> @@ -329,10 +471,8 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>>  
>>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>  
>> -	usb_gadget_disconnect(udc->gadget);
>> -	udc->driver->disconnect(udc->gadget);
>> +	usb_udc_stop(udc);
>>  	udc->driver->unbind(udc->gadget);
>> -	usb_gadget_udc_stop(udc);
>>  
>>  	udc->driver = NULL;
>>  	udc->dev.driver = NULL;
>> @@ -378,6 +518,7 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>>  
>>  /* ------------------------------------------------------------------------- */
>>  
>> +/* udc_lock must be held */
>>  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
>>  {
>>  	int ret;
>> @@ -392,12 +533,12 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>>  	ret = driver->bind(udc->gadget, driver);
>>  	if (ret)
>>  		goto err1;
>> -	ret = usb_gadget_udc_start(udc);
>> +
>> +	ret = usb_udc_start(udc);
>>  	if (ret) {
>>  		driver->unbind(udc->gadget);
>>  		goto err1;
>>  	}
>> -	usb_gadget_connect(udc->gadget);
>>  
>>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>  	return 0;
>> @@ -510,12 +651,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
>>  	}
>>  
>>  	if (sysfs_streq(buf, "connect")) {
>> -		usb_gadget_udc_start(udc);
>> -		usb_gadget_connect(udc->gadget);
>> +		mutex_lock(&udc_lock);
>> +		udc->softconnect = 1;
>> +		usb_udc_start(udc);
>> +		mutex_unlock(&udc_lock);
>>  	} else if (sysfs_streq(buf, "disconnect")) {
>> -		usb_gadget_disconnect(udc->gadget);
>> -		udc->driver->disconnect(udc->gadget);
>> -		usb_gadget_udc_stop(udc);
>> +		mutex_lock(&udc_lock);
>> +		udc->softconnect = 0;
>> +		usb_udc_stop(udc);
>> +		mutex_unlock(&udc_lock);
>>  	} else {
>>  		dev_err(dev, "unsupported command '%s'\n", buf);
>>  		return -EINVAL;
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e2f00fd..7ada7e6 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -922,6 +922,9 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver);
>>   */
>>  int usb_gadget_unregister_driver(struct usb_gadget_driver *driver);
>>  
>> +int usb_gadget_start(struct usb_gadget *gadget);
>> +int usb_gadget_stop(struct usb_gadget *gadget);
>> +
>>  extern int usb_add_gadget_udc_release(struct device *parent,
>>  		struct usb_gadget *gadget, void (*release)(struct device *dev));
>>  extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
>> -- 
>> 2.1.0
>>
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-19  3:40   ` Peter Chen
@ 2015-03-19 10:18     ` Roger Quadros
  2015-03-20  7:45       ` Peter Chen
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 10:18 UTC (permalink / raw)
  To: Peter Chen
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On 19/03/15 05:40, Peter Chen wrote:
> On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
>> The OTG core instantiates the OTG Finite State Machine
>> per OTG controller and manages starting/stopping the
>> host and gadget controllers based on the bus state.
>>
>> It provides APIs for the following tasks
>>
>> - Registering an OTG capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> TODO:
>> - sysfs interface to allow application inputs to OTG state machine
>> - otg class?
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/Makefile         |   1 +
>>  drivers/usb/common/Makefile  |   1 +
>>  drivers/usb/common/usb-otg.c | 732 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/common/usb-otg.h |  71 +++++
>>  drivers/usb/core/Kconfig     |   8 +
>>  include/linux/usb/usb-otg.h  |  86 +++++
>>  6 files changed, 899 insertions(+)
>>  create mode 100644 drivers/usb/common/usb-otg.c
>>  create mode 100644 drivers/usb/common/usb-otg.h
>>  create mode 100644 include/linux/usb/usb-otg.h
>>
>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
>> index 2f1e2aa..07f59a5 100644
>> --- a/drivers/usb/Makefile
>> +++ b/drivers/usb/Makefile
>> @@ -60,5 +60,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
>>  obj-$(CONFIG_USB_GADGET)	+= gadget/
>>  
>>  obj-$(CONFIG_USB_COMMON)	+= common/
>> +obj-$(CONFIG_USB_OTG_CORE)	+= common/
>>  
>>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
>> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
>> index ca2f8bd..573fc75 100644
>> --- a/drivers/usb/common/Makefile
>> +++ b/drivers/usb/common/Makefile
>> @@ -7,3 +7,4 @@ usb-common-y			  += common.o
>>  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>>  
>>  obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
>> +obj-$(CONFIG_USB_OTG_CORE) += usb-otg.o
>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>> new file mode 100644
>> index 0000000..1433fc9
>> --- /dev/null
>> +++ b/drivers/usb/common/usb-otg.c
>> @@ -0,0 +1,732 @@
>> +/**
>> + * drivers/usb/common/usb-otg.c - USB OTG core
>> + *
>> + * Copyright (C) 2015 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/timer.h>
>> +#include <linux/usb/otg.h>
>> +#include <linux/usb/phy.h>	/* enum usb_otg_state */
>> +#include <linux/usb/gadget.h>
>> +#include <linux/usb/usb-otg.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "usb-otg.h"
>> +
>> +/* to link timer with callback data */
>> +struct otg_timer {
>> +	struct timer_list timer;
>> +	/* callback data */
>> +	int *timeout_bit;
>> +	struct otg_data *otgd;
>> +};
>> +
>> +struct otg_data {
>> +	struct device *dev;	/* HCD & GCD's parent device */
>> +
>> +	struct otg_fsm fsm;
>> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
>> +	 * HCD is bus_to_hcd(fsm->otg->host)
>> +	 * GCD is fsm->otg->gadget
>> +	 */
>> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
>> +	struct usb_otg otg;
>> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
>> +
>> +	/* saved hooks to OTG device */
>> +	int (*start_host)(struct otg_fsm *fsm, int on);
>> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
>> +
>> +	struct list_head list;
>> +
>> +	struct work_struct work;	/* OTG FSM work */
>> +	struct workqueue_struct *wq;
>> +
>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
>> +
>> +	bool fsm_running;
>> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
>> +	bool host_can_start;		/* OTG FSM says host can start */
>> +
>> +	/* use otg->fsm.lock for serializing access */
>> +};
> 
> What's the relationship between struct usb_otg otg and this one?

Did you mean why struct usb_otg otg is there in struct otg_data?
Just for easy allocation. fsm_ops only contains the pointer to struct usb_otg.

> 
>> +
>> +/* OTG device list */
>> +LIST_HEAD(otg_list);
>> +static DEFINE_MUTEX(otg_list_mutex);
>> +
>> +/**
>> + * check if device is in our OTG list and return
>> + * otg_data, else NULL.
>> + *
>> + * otg_list_mutex must be held.
>> + */
>> +static struct otg_data *usb_otg_device_get_otgd(struct device *parent_dev)
>> +{
>> +	struct otg_data *otgd;
>> +
>> +	list_for_each_entry(otgd, &otg_list, list) {
>> +		if (otgd->dev == parent_dev)
>> +			return otgd;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * timer callback to set timeout bit and kick FSM
>> + */
>> +static void set_tmout(unsigned long data)
>> +{
>> +	struct otg_timer *tmr_data;
>> +
>> +	tmr_data = (struct otg_timer *)data;
>> +
>> +	if (tmr_data->timeout_bit)
>> +		*tmr_data->timeout_bit = 1;
>> +
>> +	usb_otg_sync_inputs(&tmr_data->otgd->fsm);
>> +}
>> +
>> +/**
>> + * Initialize one OTG timer with callback, timeout and timeout bit
>> + */
>> +static void otg_timer_init(enum otg_fsm_timer id, struct otg_data *otgd,
>> +			   void (*callback)(unsigned long),
>> +			   unsigned long expires_ms,
>> +			   int *timeout_bit)
>> +{
>> +	struct otg_timer *otgtimer = &otgd->timers[id];
>> +	struct timer_list *timer = &otgtimer->timer;
>> +
>> +	init_timer(timer);
>> +	timer->function = callback;
>> +	timer->expires = jiffies + msecs_to_jiffies(expires_ms);
>> +	timer->data = (unsigned long)otgtimer;
>> +
> 
> The timer for TB_DATA_PLS is about 10ms or less, it is not suitable
> for using kernel timer, hrtimer is suitable choice.

good catch. I will switch to hrtimer then.

> 
>> +	otgtimer->timeout_bit = timeout_bit;
>> +	otgtimer->otgd = otgd;
>> +}
>> +
>> +/**
>> + * Initialize standard OTG timers
>> + */
>> +static void usb_otg_init_timers(struct otg_data *otgd)
>> +{
>> +	struct otg_fsm *fsm = &otgd->fsm;
>> +
>> +	otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE, &fsm->a_wait_vrise_tmout);
>> +	otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL, &fsm->a_wait_vfall_tmout);
>> +	otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON, &fsm->a_wait_bcon_tmout);
>> +	otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS, &fsm->a_aidl_bdis_tmout);
>> +	otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS, &fsm->a_bidl_adis_tmout);
>> +	otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST, &fsm->b_ase0_brst_tmout);
>> +
>> +	otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP, &fsm->b_se0_srp);
>> +	otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL, &fsm->b_srp_done);
>> +
>> +	otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
>> +}

<snip>

cheers,
-roger


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm
  2015-03-19  3:46   ` Peter Chen
@ 2015-03-19 10:20     ` Roger Quadros
  0 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 10:20 UTC (permalink / raw)
  To: Peter Chen
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On 19/03/15 05:46, Peter Chen wrote:
> On Wed, Mar 18, 2015 at 03:56:02PM +0200, Roger Quadros wrote:
>> These members are not used anywhere so remove them.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  include/linux/usb/otg-fsm.h | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
>> index b6ba1bf..176c4fc 100644
>> --- a/include/linux/usb/otg-fsm.h
>> +++ b/include/linux/usb/otg-fsm.h
>> @@ -95,11 +95,6 @@ struct otg_fsm {
>>  	int b_hnp_enable;
>>  	int a_clr_err;
>>  
>> -	/* Informative variables */
>> -	int a_bus_drop_inf;
>> -	int a_bus_req_inf;
>> -	int a_clr_err_inf;
>> -	int b_bus_req_inf;
>>  	/* Auxilary informative variables */
>>  	int a_suspend_req_inf;
>>  
> 
> But the above are defined at: ch 7.4.4, On-The-Go and Embedded Host
> Supplement to the USB Revision 2.0 Specification

I can leave them there then but just add a note saying that they are
not yet used by the OTG FSM.

cheers,
-roger

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-19  8:26   ` Li Jun
@ 2015-03-19 10:30     ` Roger Quadros
  2015-03-19 14:41       ` Li Jun
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 10:30 UTC (permalink / raw)
  To: Li Jun
  Cc: gregkh, balbi, stern, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On 19/03/15 10:26, Li Jun wrote:
> On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
>> The OTG core instantiates the OTG Finite State Machine
>> per OTG controller and manages starting/stopping the
>> host and gadget controllers based on the bus state.
>>
>> It provides APIs for the following tasks
>>
>> - Registering an OTG capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> TODO:
>> - sysfs interface to allow application inputs to OTG state machine
>> - otg class?
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> +struct otg_data {
>> +	struct device *dev;	/* HCD & GCD's parent device */
>> +
>> +	struct otg_fsm fsm;
>> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
>> +	 * HCD is bus_to_hcd(fsm->otg->host)
>> +	 * GCD is fsm->otg->gadget
>> +	 */
>> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
>> +	struct usb_otg otg;
>> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
>> +
>> +	/* saved hooks to OTG device */
>> +	int (*start_host)(struct otg_fsm *fsm, int on);
>> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
>> +
>> +	struct list_head list;
>> +
>> +	struct work_struct work;	/* OTG FSM work */
>> +	struct workqueue_struct *wq;
>> +
>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
>> +
>> +	bool fsm_running;
>> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
>> +	bool host_can_start;		/* OTG FSM says host can start */
> 
> Do not understand above 2 *_can_start flags from the patch, which are set
> only when you really start host/gadget, to prevent host/gadget driver to
> start it out of otg fsm control?

host_can_start is used only by this driver in the following _unlikely_ condition
and could probably be got rid of.
- Primary HCD and gadget registers
- OTG FSM signals host to start but it can't start as shared HCD isn't yet registered.
so we set this flag.
- when shared HCD registers, we check the flag and explicitly start both the host controllers.

gadget_can_start is used by the gadget driver through the usb_otg_gadget_can_start() API.
This is needed because gadget might start at a later time depending on either
- gadget function driver loads
- userspace enables softconnect.

> 
> Li Jun
>> +
>> +	/* use otg->fsm.lock for serializing access */
>> +};
>> +
>> + * Can be called in IRQ context.
>> + */
>> +void usb_otg_sync_inputs(struct otg_fsm *fsm)
>> +{
>> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
>> +
>> +	/* Don't kick FSM till it has started */
>> +	if (!otgd->fsm_running)
>> +		return;
>> +
>> +	/* Kick FSM */
>> +	queue_work(otgd->wq, &otgd->work);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
>> +
>> +/**
>> + * usb_otg_kick_fsm - Kick the OTG state machine
>> + * @hcd_gcd_device:	Host/Gadget controller device
>> + *
>> + * Used by USB host/device stack to sync OTG related
>> + * events to the OTG state machine.
>> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
>> + *
> There are quite a few otg fsm variables which should be updated when 
> events/interrupts(b_conn, a_srp_det, ...) occur, how is your plan to
> update them? Still rely on specific controller driver irq handler to
> capture all those events and update them?

Yes, my plan was that the OTG controller driver will handle those
interrupts, update otg_fsm members and call usb_otg_sync_inputs() to
notify the OTG FSM.

cheers,
-roger

> 
> Li Jun
>> + * Returns: 0 on success, error value otherwise.
>> + */
>> +int usb_otg_kick_fsm(struct device *hcd_gcd_device)
>> +{
>> +	struct otg_data *otgd;
>> +
>> +	mutex_lock(&otg_list_mutex);
>> +	otgd = usb_otg_device_get_otgd(hcd_gcd_device->parent);
>> +	if (!otgd) {
>> +		dev_err(hcd_gcd_device, "%s: invalid host/gadget device\n",
>> +			__func__);
>> +		mutex_unlock(&otg_list_mutex);
>> +		return -ENODEV;
>> +	}
>> +
>> +	mutex_unlock(&otg_list_mutex);
>> +	usb_otg_sync_inputs(&otgd->fsm);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
>> -- 
>> 2.1.0
>>


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 0/9] USB: OTG Core functionality
  2015-03-18 17:37 ` [RFC][PATCH 0/9] USB: OTG Core functionality Tony Lindgren
@ 2015-03-19 10:31   ` Roger Quadros
  0 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 10:31 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: gregkh, balbi, stern, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On 18/03/15 19:37, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [150318 07:00]:
>> Hi,
>>
>> [NOTE: RFC only. Not for merge yet.]
>>
>> This is an attempt to centralize OTG functionality in the kernel.
>> Still work in progress but I wanted to get an early feedback
>> to avoid major rework. :) 
>>
>> Why?:
>> ----
>>
>> Most of the OTG drivers have been dealing with the OTG state machine
>> themselves and there is a scope for code re-use. This has been
>> partly addressed by the usb/common/usb-otg-fsm.c but it still
>> leaves the instantiation of the state machine and OTG timers
>> to the controller drivers. We re-use usb-otg-fsm.c but
>> go one step further by instantiating the state machine and timers
>> thus making it easier for drivers to implement OTG functionality.
>>
>> Newer OTG cores support standard host interface (e.g. xHCI?) so
>> host and gadget functionality are no longer closely knit like older
>> cores. There needs to be a way to co-ordinate the operation of the
>> host and gadget in OTG mode. i.e. to stop and start them from a
>> central location. This central location should be the USB OTG core.
>>
>> Host and gadget controllers might be sharing resources and can't
>> be always running. One has to be stopped for the other to run.
>> This can't be done as of now and can be done from the OTG core.
>>
>> What?:
>> -----
>>
>> The OTG core instantiates the OTG Finite State Machine
>> per OTG controller and manages starting/stopping the
>> host and gadget controllers based on the bus state.
>>     
>> It provides APIs for the following
>>     
>> - Registering an OTG capable controller
>> struct otg_fsm *usb_otg_register(struct device *parent_dev,
>>                                  struct otg_fsm_ops *fsm_ops);
>> int usb_otg_unregister(struct device *parent_dev);
>>
>> - Registering Host controllers to OTG core (used by hcd-core)
>> int usb_otg_register_hcd(struct usb_hcd *hcd);
>> int usb_otg_unregister_hcd(struct usb_hcd *hcd);
>>
>> - Registering Gadget controllers to OTG core (used by udc-core)
>> int usb_otg_register_gadget(struct usb_gadget *gadget);
>> int usb_otg_unregister_gadget(struct usb_gadget *gadget);
>>
>> - Providing inputs to and kicking the OTG state machine
>> void usb_otg_sync_inputs(struct otg_fsm *fsm);
>> int usb_otg_kick_fsm(struct device *hcd_gcd_device);
>>
>> 'struct otg_fsm' is the interface to the OTG state machine.
>> It contains inputs to the fsm, status of the fsm and operations
>> for the OTG controller driver.
> 
> Sounds good to me. I take you're also planning to provide some
> common /sys/kernel/debug/otg type interfaces for OTG validation
> tests? For things like SRP etc.

Yes that's the plan.

cheers,
-roger

> 
> Regards,
> 
> Tony
>  
>> Usage model:
>> -----------
>>
>> - The OTG controller device is assumed to be the parent of
>> the host and gadget controller. It must call usb_otg_register()
>> before populating the host and gadget devices so that the OTG
>> core is aware that it is an OTG device before the host & gadget
>> register. The OTG controller must provide struct otg_fsm_ops *
>> which will be called by the OTG core depending on OTG bus state.
>>
>> - The host/gadget core stacks are modified to inform the OTG core
>> whenever a new host/gadget device is added. The OTG core then
>> checks if the host/gadget is part of the OTG controller and if yes
>> then prevents the host/gadget from starting till both host and
>> gadget are registered, OTG state machine is running and the
>> USB bus state is appropriate to start host/gadget.
>>  For this APIs have been added to host/gadget stacks to start/stop
>> the controllers from the OTG core.
>>
>> - No modification is needed for the host/gadget controller drivers.
>> They must ensure that their start/stop methods can be called repeatedly
>> and any shared resources between host & gadget are properly managed.
>> The OTG core ensures that both are not started simultaneously.
>>
>> - The OTG core instantiates one OTG state machine per OTG
>> controller and the necessary OTG timers to manage OTG state timeouts.
>> The state machine is started when both host & gadget register and
>> stopped when either of them unregisters. The controllers are started
>> and stopped depending on bus state.
>>
>> - During the lifetime of the OTG state machine, inputs can be
>> provided to it by modifying the appropriate members of 'struct otg_fsm'
>> and calling usb_otg_sync_inputs(). This is typically done by the
>> OTG controller driver that called usb_otg_register() since it is
>> the only external component that has the 'struct otg_fsm' handle.
>>
>> Pending items:
>> - We probably need a otg class.
>> - sysfs interface for application OTG inputs and OTG status information
>> - resolve symbol dependency for module use.
>> - otg driver for dwc3 core to get dual-role working on dra7-evm.
>>
>> cheers,
>> -roger
>>
>> Roger Quadros (9):
>>   usb: hcd: Introduce usb_start/stop_hcd()
>>   usb: gadget: add usb_gadget_start/stop()
>>   usb: otg: add OTG core
>>   usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable
>>   usb: hcd: adapt to OTG
>>   usb: gadget: udc: adapt to OTG
>>   usb: dwc3: adapt to OTG core
>>   usb: otg-fsm: Remove unused members in struct otg_fsm
>>   usb: otg-fsm: Add documentation for struct otg_fsm
>>
>>  drivers/usb/Makefile              |   1 +
>>  drivers/usb/common/Makefile       |   1 +
>>  drivers/usb/common/usb-otg.c      | 732 ++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/common/usb-otg.h      |  71 ++++
>>  drivers/usb/core/Kconfig          |   8 +
>>  drivers/usb/core/hcd.c            | 153 +++++---
>>  drivers/usb/core/hub.c            |  11 +-
>>  drivers/usb/dwc3/core.c           | 101 ++++++
>>  drivers/usb/dwc3/core.h           |   6 +
>>  drivers/usb/dwc3/platform_data.h  |   1 +
>>  drivers/usb/gadget/udc/udc-core.c | 172 ++++++++-
>>  include/linux/usb/gadget.h        |   3 +
>>  include/linux/usb/hcd.h           |   2 +
>>  include/linux/usb/otg-fsm.h       |  81 ++++-
>>  include/linux/usb/usb-otg.h       |  86 +++++
>>  15 files changed, 1357 insertions(+), 72 deletions(-)
>>  create mode 100644 drivers/usb/common/usb-otg.c
>>  create mode 100644 drivers/usb/common/usb-otg.h
>>  create mode 100644 include/linux/usb/usb-otg.h
>>
>> -- 
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-18 19:49   ` Alan Stern
  2015-03-18 21:41     ` Tony Lindgren
@ 2015-03-19 11:38     ` Roger Quadros
  2015-03-19 14:17       ` Alan Stern
  2015-03-20  6:32       ` Peter Chen
  1 sibling, 2 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 11:38 UTC (permalink / raw)
  To: Alan Stern
  Cc: gregkh, balbi, dan.j.williams, peter.chen, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On 18/03/15 21:49, Alan Stern wrote:
> On Wed, 18 Mar 2015, Roger Quadros wrote:
> 
>> To support OTG we want a mechanism to start and stop
>> the HCD from the OTG state machine. Add usb_start_hcd()
>> and usb_stop_hcd().
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> There are a few problems in this proposed patch.
> 
>> +int usb_start_hcd(struct usb_hcd *hcd)
>> +{
>> +	int retval;
>> +	struct usb_device *rhdev = hcd->self.root_hub;
>> +
>> +	if (hcd->state != HC_STATE_HALT) {
>> +		dev_err(hcd->self.controller, "not starting a running HCD\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	hcd->state = HC_STATE_RUNNING;
>> +	retval = hcd->driver->start(hcd);
>> +	if (retval < 0) {
>> +		dev_err(hcd->self.controller, "startup error %d\n", retval);
>> +		hcd->state = HC_STATE_HALT;
>> +		return retval;
>> +	}
>> +
>> +	/* starting here, usbcore will pay attention to this root hub */
>> +	if ((retval = register_root_hub(hcd)) != 0)
>> +		goto err_register_root_hub;
> 
> If the host controller is started more than once, you will end up
> unregistering and re-registering the root hub.  The device core does
> not allow this.  Once a device has been unregistered, you must not try
> to register it again -- you have to allocate a new device and register
> it instead.

Understood.

> 
> Also, although you call the driver's ->start method multiple times, the
> ->reset method is called only once, when the controller is first 
> probed.  It's not clear that this will work in a situation where the HC 
> and the UDC share hardware state; after the UDC is stopped it may be 
> necessary to reset the HC before it can run again.

Yes, good point.
> 
> It might be possible to make this work, but I suspect quite a few 
> drivers would need rewriting first.  As another example of the problems 
> you face, consider how stopping a host controller will interact with 
> the driver's PM support (both system suspend and runtime suspend).

Right. This needs more work than I thought.
> 
> It would be a lot simpler to unbind the host controller driver
> completely when switching to device mode and rebind it when switching
> back.  I guess that is the sort of heavy-duty approach you want to
> avoid, but it may be the only practical way forward.

So you mean directly calling usb_add/remove_hcd() from the OTG core?
I don't see any issues with that other than it being a heavy-duty operation
like you said and hope that it doesn't violate the OTG spec timing.

Looking at Figure 5-3: "HNP Sequence of Events (FS)" of the OTG 2.0 spec
we have about 150ms (X10) to switch from B-Device detected A connect (b_wait_acon state)
to driving bus reset (b_host state). I don't think this should be an issue in modern SoCs
but I'm not very sure.

In any case I can migrate to the add/remove hcd approach to simplify things.

cheers,
-roger

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-19 10:14     ` Roger Quadros
@ 2015-03-19 14:09       ` Li Jun
  2015-03-19 14:50         ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Li Jun @ 2015-03-19 14:09 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Peter Chen, gregkh, balbi, stern, dan.j.williams, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On Thu, Mar 19, 2015 at 12:14:39PM +0200, Roger Quadros wrote:
> On 19/03/15 05:30, Peter Chen wrote:
> > On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
> >> The OTG state machine needs a mechanism to start and
> >> stop the gadget controller. Add usb_gadget_start()
> >> and usb_gadget_stop().
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
> >>  include/linux/usb/gadget.h        |   3 +
> >>  2 files changed, 158 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> >> index 5a81cb0..69b4123 100644
> >> --- a/drivers/usb/gadget/udc/udc-core.c
> >> +++ b/drivers/usb/gadget/udc/udc-core.c
> >> @@ -35,6 +35,8 @@
> >>   * @dev - the child device to the actual controller
> >>   * @gadget - the gadget. For use by the class code
> >>   * @list - for use by the udc class driver
> >> + * @running - udc is running
> > 
> > Doesn't OTG FSM should know it?
> 
> Not really, as the gadget driver might not have been loaded yet or userspace might
> have disabled softconnect when the OTG FSM wants UDC to start.
> 
> So only UDC knows if it has really started or not based on this flag.
> 

why this can not be known by check the otg fsm state? i.e. if the device in
b_peripheral or a_peripheral state, udc should had started, isn't it?

Li Jun
> cheers,
> -roger
> 
> > 
> > Peter
> >> + * @softconnect - sysfs softconnect says OK to connect
> >>   *
> > 
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-19 11:38     ` Roger Quadros
@ 2015-03-19 14:17       ` Alan Stern
  2015-03-20  6:32       ` Peter Chen
  1 sibling, 0 replies; 40+ messages in thread
From: Alan Stern @ 2015-03-19 14:17 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, dan.j.williams, peter.chen, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Thu, 19 Mar 2015, Roger Quadros wrote:

> > It would be a lot simpler to unbind the host controller driver
> > completely when switching to device mode and rebind it when switching
> > back.  I guess that is the sort of heavy-duty approach you want to
> > avoid, but it may be the only practical way forward.
> 
> So you mean directly calling usb_add/remove_hcd() from the OTG core?

Well, actually I meant calling device_release_driver() and
driver_probe_device().  But usb_add/remove_hcd() might work just about
as well, although I think some of the HCDs may still require a little
rewriting.  Peter Chen says it works for Chipidea.

> I don't see any issues with that other than it being a heavy-duty operation
> like you said and hope that it doesn't violate the OTG spec timing.

The timing is a tough issue.  I'm not sure what we can do about it, 
though.

> Looking at Figure 5-3: "HNP Sequence of Events (FS)" of the OTG 2.0 spec
> we have about 150ms (X10) to switch from B-Device detected A connect (b_wait_acon state)
> to driving bus reset (b_host state). I don't think this should be an issue in modern SoCs
> but I'm not very sure.
> 
> In any case I can migrate to the add/remove hcd approach to simplify things.

Good luck.

Alan Stern


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-19 10:30     ` Roger Quadros
@ 2015-03-19 14:41       ` Li Jun
  2015-03-19 14:54         ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Li Jun @ 2015-03-19 14:41 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On Thu, Mar 19, 2015 at 12:30:46PM +0200, Roger Quadros wrote:
> On 19/03/15 10:26, Li Jun wrote:
> > On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
> >> The OTG core instantiates the OTG Finite State Machine
> >> per OTG controller and manages starting/stopping the
> >> host and gadget controllers based on the bus state.
> >>
> >> It provides APIs for the following tasks
> >>
> >> - Registering an OTG capable controller
> >> - Registering Host and Gadget controllers to OTG core
> >> - Providing inputs to and kicking the OTG state machine
> >>
> >> TODO:
> >> - sysfs interface to allow application inputs to OTG state machine
> >> - otg class?
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >> +struct otg_data {
> >> +	struct device *dev;	/* HCD & GCD's parent device */
> >> +
> >> +	struct otg_fsm fsm;
> >> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
> >> +	 * HCD is bus_to_hcd(fsm->otg->host)
> >> +	 * GCD is fsm->otg->gadget
> >> +	 */
> >> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
> >> +	struct usb_otg otg;
> >> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
> >> +
> >> +	/* saved hooks to OTG device */
> >> +	int (*start_host)(struct otg_fsm *fsm, int on);
> >> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
> >> +
> >> +	struct list_head list;
> >> +
> >> +	struct work_struct work;	/* OTG FSM work */
> >> +	struct workqueue_struct *wq;
> >> +
> >> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
> >> +
> >> +	bool fsm_running;
> >> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
> >> +	bool host_can_start;		/* OTG FSM says host can start */
> > 
> > Do not understand above 2 *_can_start flags from the patch, which are set
> > only when you really start host/gadget, to prevent host/gadget driver to
> > start it out of otg fsm control?
> 
> host_can_start is used only by this driver in the following _unlikely_ condition
> and could probably be got rid of.
> - Primary HCD and gadget registers
> - OTG FSM signals host to start but it can't start as shared HCD isn't yet registered.
> so we set this flag.
> - when shared HCD registers, we check the flag and explicitly start both the host controllers.
> 
> gadget_can_start is used by the gadget driver through the usb_otg_gadget_can_start() API.
> This is needed because gadget might start at a later time depending on either
> - gadget function driver loads

I think it should be loaded before kick off OTG fsm.

> - userspace enables softconnect.

Why use the input of softconnect in userspace? I suppose all inputs should be
in scope/defined by OTG spec(a_bus_req, b_bus_req...).
So it's possible the gadget has not been started(pull up DP) while the OTG fsm
is in b_peripheral state? If yes, it's breaking OTG spec 7.2.3:
... 
When a high-speed capable B-device enters this state it shall enable its pull-up
on D+. After the B-device enables its pull-up, it shall monitor the state of the
bus to determine if a bus reset is being signaled by the A-device.
...

So for B-device, it should start gadget(pull up DP) if detects valid BSV vbus,
that's the only condition.

Li Jun
> 
> > 
> > Li Jun
> >> +
> >> +	/* use otg->fsm.lock for serializing access */
> >> +};
> >> +
> >> + * Can be called in IRQ context.
> >> + */
> >> +void usb_otg_sync_inputs(struct otg_fsm *fsm)
> >> +{
> >> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> >> +
> >> +	/* Don't kick FSM till it has started */
> >> +	if (!otgd->fsm_running)
> >> +		return;
> >> +
> >> +	/* Kick FSM */
> >> +	queue_work(otgd->wq, &otgd->work);
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
> >> +
> >> +/**
> >> + * usb_otg_kick_fsm - Kick the OTG state machine
> >> + * @hcd_gcd_device:	Host/Gadget controller device
> >> + *
> >> + * Used by USB host/device stack to sync OTG related
> >> + * events to the OTG state machine.
> >> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
> >> + *
> > There are quite a few otg fsm variables which should be updated when 
> > events/interrupts(b_conn, a_srp_det, ...) occur, how is your plan to
> > update them? Still rely on specific controller driver irq handler to
> > capture all those events and update them?
> 
> Yes, my plan was that the OTG controller driver will handle those
> interrupts, update otg_fsm members and call usb_otg_sync_inputs() to
> notify the OTG FSM.
> 
> cheers,
> -roger
> 
> > 
> > Li Jun
> >> + * Returns: 0 on success, error value otherwise.
> >> + */
> >> +int usb_otg_kick_fsm(struct device *hcd_gcd_device)
> >> +{
> >> +	struct otg_data *otgd;
> >> +
> >> +	mutex_lock(&otg_list_mutex);
> >> +	otgd = usb_otg_device_get_otgd(hcd_gcd_device->parent);
> >> +	if (!otgd) {
> >> +		dev_err(hcd_gcd_device, "%s: invalid host/gadget device\n",
> >> +			__func__);
> >> +		mutex_unlock(&otg_list_mutex);
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	mutex_unlock(&otg_list_mutex);
> >> +	usb_otg_sync_inputs(&otgd->fsm);
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
> >> -- 
> >> 2.1.0
> >>
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-19 14:09       ` Li Jun
@ 2015-03-19 14:50         ` Roger Quadros
  2015-03-20  7:18           ` Peter Chen
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 14:50 UTC (permalink / raw)
  To: Li Jun
  Cc: Peter Chen, gregkh, balbi, stern, dan.j.williams, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On 19/03/15 16:09, Li Jun wrote:
> On Thu, Mar 19, 2015 at 12:14:39PM +0200, Roger Quadros wrote:
>> On 19/03/15 05:30, Peter Chen wrote:
>>> On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
>>>> The OTG state machine needs a mechanism to start and
>>>> stop the gadget controller. Add usb_gadget_start()
>>>> and usb_gadget_stop().
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
>>>>  include/linux/usb/gadget.h        |   3 +
>>>>  2 files changed, 158 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>>>> index 5a81cb0..69b4123 100644
>>>> --- a/drivers/usb/gadget/udc/udc-core.c
>>>> +++ b/drivers/usb/gadget/udc/udc-core.c
>>>> @@ -35,6 +35,8 @@
>>>>   * @dev - the child device to the actual controller
>>>>   * @gadget - the gadget. For use by the class code
>>>>   * @list - for use by the udc class driver
>>>> + * @running - udc is running
>>>
>>> Doesn't OTG FSM should know it?
>>
>> Not really, as the gadget driver might not have been loaded yet or userspace might
>> have disabled softconnect when the OTG FSM wants UDC to start.
>>
>> So only UDC knows if it has really started or not based on this flag.
>>
> 
> why this can not be known by check the otg fsm state? i.e. if the device in
> b_peripheral or a_peripheral state, udc should had started, isn't it?

If gadget function driver (which is different from UDC driver) is not yet loaded
then we can't start UDC even if otg fsm is in b_peripheral.
Also, if userspace has disabled softconnect we can't start UDC.

So, b_peripheral != UDC_started.

I've tried to address this issue by adding the checks in usb_gadget_start().

cheers,
-roger

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-19 14:41       ` Li Jun
@ 2015-03-19 14:54         ` Roger Quadros
  0 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-19 14:54 UTC (permalink / raw)
  To: Li Jun
  Cc: gregkh, balbi, stern, dan.j.williams, peter.chen, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On 19/03/15 16:41, Li Jun wrote:
> On Thu, Mar 19, 2015 at 12:30:46PM +0200, Roger Quadros wrote:
>> On 19/03/15 10:26, Li Jun wrote:
>>> On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
>>>> The OTG core instantiates the OTG Finite State Machine
>>>> per OTG controller and manages starting/stopping the
>>>> host and gadget controllers based on the bus state.
>>>>
>>>> It provides APIs for the following tasks
>>>>
>>>> - Registering an OTG capable controller
>>>> - Registering Host and Gadget controllers to OTG core
>>>> - Providing inputs to and kicking the OTG state machine
>>>>
>>>> TODO:
>>>> - sysfs interface to allow application inputs to OTG state machine
>>>> - otg class?
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>> +struct otg_data {
>>>> +	struct device *dev;	/* HCD & GCD's parent device */
>>>> +
>>>> +	struct otg_fsm fsm;
>>>> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
>>>> +	 * HCD is bus_to_hcd(fsm->otg->host)
>>>> +	 * GCD is fsm->otg->gadget
>>>> +	 */
>>>> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
>>>> +	struct usb_otg otg;
>>>> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
>>>> +
>>>> +	/* saved hooks to OTG device */
>>>> +	int (*start_host)(struct otg_fsm *fsm, int on);
>>>> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
>>>> +
>>>> +	struct list_head list;
>>>> +
>>>> +	struct work_struct work;	/* OTG FSM work */
>>>> +	struct workqueue_struct *wq;
>>>> +
>>>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
>>>> +
>>>> +	bool fsm_running;
>>>> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
>>>> +	bool host_can_start;		/* OTG FSM says host can start */
>>>
>>> Do not understand above 2 *_can_start flags from the patch, which are set
>>> only when you really start host/gadget, to prevent host/gadget driver to
>>> start it out of otg fsm control?
>>
>> host_can_start is used only by this driver in the following _unlikely_ condition
>> and could probably be got rid of.
>> - Primary HCD and gadget registers
>> - OTG FSM signals host to start but it can't start as shared HCD isn't yet registered.
>> so we set this flag.
>> - when shared HCD registers, we check the flag and explicitly start both the host controllers.
>>
>> gadget_can_start is used by the gadget driver through the usb_otg_gadget_can_start() API.
>> This is needed because gadget might start at a later time depending on either
>> - gadget function driver loads
> 
> I think it should be loaded before kick off OTG fsm.

Agree. This could be done.

> 
>> - userspace enables softconnect.
> 
> Why use the input of softconnect in userspace? I suppose all inputs should be
> in scope/defined by OTG spec(a_bus_req, b_bus_req...).
> So it's possible the gadget has not been started(pull up DP) while the OTG fsm
> is in b_peripheral state? If yes, it's breaking OTG spec 7.2.3:

Good point.

> ... 
> When a high-speed capable B-device enters this state it shall enable its pull-up
> on D+. After the B-device enables its pull-up, it shall monitor the state of the
> bus to determine if a bus reset is being signaled by the A-device.
> ...
> 
> So for B-device, it should start gadget(pull up DP) if detects valid BSV vbus,
> that's the only condition.

OK. so we can ignore the softconnect in the OTG case and I can get rid of the
udc/gadget_running flags.

so now b_peripheral means UDC started.

cheers,
-roger

> 
> Li Jun
>>
>>>
>>> Li Jun
>>>> +
>>>> +	/* use otg->fsm.lock for serializing access */
>>>> +};
>>>> +
>>>> + * Can be called in IRQ context.
>>>> + */
>>>> +void usb_otg_sync_inputs(struct otg_fsm *fsm)
>>>> +{
>>>> +	struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
>>>> +
>>>> +	/* Don't kick FSM till it has started */
>>>> +	if (!otgd->fsm_running)
>>>> +		return;
>>>> +
>>>> +	/* Kick FSM */
>>>> +	queue_work(otgd->wq, &otgd->work);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
>>>> +
>>>> +/**
>>>> + * usb_otg_kick_fsm - Kick the OTG state machine
>>>> + * @hcd_gcd_device:	Host/Gadget controller device
>>>> + *
>>>> + * Used by USB host/device stack to sync OTG related
>>>> + * events to the OTG state machine.
>>>> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
>>>> + *
>>> There are quite a few otg fsm variables which should be updated when 
>>> events/interrupts(b_conn, a_srp_det, ...) occur, how is your plan to
>>> update them? Still rely on specific controller driver irq handler to
>>> capture all those events and update them?
>>
>> Yes, my plan was that the OTG controller driver will handle those
>> interrupts, update otg_fsm members and call usb_otg_sync_inputs() to
>> notify the OTG FSM.
>>
>> cheers,
>> -roger
>>
>>>
>>> Li Jun
>>>> + * Returns: 0 on success, error value otherwise.
>>>> + */
>>>> +int usb_otg_kick_fsm(struct device *hcd_gcd_device)
>>>> +{
>>>> +	struct otg_data *otgd;
>>>> +
>>>> +	mutex_lock(&otg_list_mutex);
>>>> +	otgd = usb_otg_device_get_otgd(hcd_gcd_device->parent);
>>>> +	if (!otgd) {
>>>> +		dev_err(hcd_gcd_device, "%s: invalid host/gadget device\n",
>>>> +			__func__);
>>>> +		mutex_unlock(&otg_list_mutex);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	mutex_unlock(&otg_list_mutex);
>>>> +	usb_otg_sync_inputs(&otgd->fsm);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
>>>> -- 
>>>> 2.1.0
>>>>
>>


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-19 11:38     ` Roger Quadros
  2015-03-19 14:17       ` Alan Stern
@ 2015-03-20  6:32       ` Peter Chen
  2015-03-20  9:49         ` Roger Quadros
  1 sibling, 1 reply; 40+ messages in thread
From: Peter Chen @ 2015-03-20  6:32 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Alan Stern, gregkh, balbi, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Thu, Mar 19, 2015 at 01:38:32PM +0200, Roger Quadros wrote:
> On 18/03/15 21:49, Alan Stern wrote:
> > On Wed, 18 Mar 2015, Roger Quadros wrote:
> > 
> >> To support OTG we want a mechanism to start and stop
> >> the HCD from the OTG state machine. Add usb_start_hcd()
> >> and usb_stop_hcd().
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> > 
> > There are a few problems in this proposed patch.
> > 
> >> +int usb_start_hcd(struct usb_hcd *hcd)
> >> +{
> >> +	int retval;
> >> +	struct usb_device *rhdev = hcd->self.root_hub;
> >> +
> >> +	if (hcd->state != HC_STATE_HALT) {
> >> +		dev_err(hcd->self.controller, "not starting a running HCD\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	hcd->state = HC_STATE_RUNNING;
> >> +	retval = hcd->driver->start(hcd);
> >> +	if (retval < 0) {
> >> +		dev_err(hcd->self.controller, "startup error %d\n", retval);
> >> +		hcd->state = HC_STATE_HALT;
> >> +		return retval;
> >> +	}
> >> +
> >> +	/* starting here, usbcore will pay attention to this root hub */
> >> +	if ((retval = register_root_hub(hcd)) != 0)
> >> +		goto err_register_root_hub;
> > 
> > If the host controller is started more than once, you will end up
> > unregistering and re-registering the root hub.  The device core does
> > not allow this.  Once a device has been unregistered, you must not try
> > to register it again -- you have to allocate a new device and register
> > it instead.
> 
> Understood.
> 
> > 
> > Also, although you call the driver's ->start method multiple times, the
> > ->reset method is called only once, when the controller is first 
> > probed.  It's not clear that this will work in a situation where the HC 
> > and the UDC share hardware state; after the UDC is stopped it may be 
> > necessary to reset the HC before it can run again.
> 
> Yes, good point.
> > 
> > It might be possible to make this work, but I suspect quite a few 
> > drivers would need rewriting first.  As another example of the problems 
> > you face, consider how stopping a host controller will interact with 
> > the driver's PM support (both system suspend and runtime suspend).
> 
> Right. This needs more work than I thought.
> > 
> > It would be a lot simpler to unbind the host controller driver
> > completely when switching to device mode and rebind it when switching
> > back.  I guess that is the sort of heavy-duty approach you want to
> > avoid, but it may be the only practical way forward.
> 
> So you mean directly calling usb_add/remove_hcd() from the OTG core?
> I don't see any issues with that other than it being a heavy-duty operation
> like you said and hope that it doesn't violate the OTG spec timing.
> 
> Looking at Figure 5-3: "HNP Sequence of Events (FS)" of the OTG 2.0 spec
> we have about 150ms (X10) to switch from B-Device detected A connect (b_wait_acon state)
> to driving bus reset (b_host state). I don't think this should be an issue in modern SoCs
> but I'm not very sure.
> 

It is not related toadd/remove hcd, it is the time from receiving PCD
to issue BUS_RESET, the Linux stack can't satisfy OTG spec (150ms) due
to there are some de-bounce waitings.

> In any case I can migrate to the add/remove hcd approach to simplify things.
> 

It should be no problem, we use it more than 1 years.

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-19 14:50         ` Roger Quadros
@ 2015-03-20  7:18           ` Peter Chen
  2015-03-20  9:46             ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Peter Chen @ 2015-03-20  7:18 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Li Jun, gregkh, balbi, stern, dan.j.williams, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On Thu, Mar 19, 2015 at 04:50:31PM +0200, Roger Quadros wrote:
> On 19/03/15 16:09, Li Jun wrote:
> > On Thu, Mar 19, 2015 at 12:14:39PM +0200, Roger Quadros wrote:
> >> On 19/03/15 05:30, Peter Chen wrote:
> >>> On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
> >>>> The OTG state machine needs a mechanism to start and
> >>>> stop the gadget controller. Add usb_gadget_start()
> >>>> and usb_gadget_stop().
> >>>>
> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>>> ---
> >>>>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
> >>>>  include/linux/usb/gadget.h        |   3 +
> >>>>  2 files changed, 158 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> >>>> index 5a81cb0..69b4123 100644
> >>>> --- a/drivers/usb/gadget/udc/udc-core.c
> >>>> +++ b/drivers/usb/gadget/udc/udc-core.c
> >>>> @@ -35,6 +35,8 @@
> >>>>   * @dev - the child device to the actual controller
> >>>>   * @gadget - the gadget. For use by the class code
> >>>>   * @list - for use by the udc class driver
> >>>> + * @running - udc is running
> >>>
> >>> Doesn't OTG FSM should know it?
> >>
> >> Not really, as the gadget driver might not have been loaded yet or userspace might
> >> have disabled softconnect when the OTG FSM wants UDC to start.
> >>
> >> So only UDC knows if it has really started or not based on this flag.
> >>
> > 
> > why this can not be known by check the otg fsm state? i.e. if the device in
> > b_peripheral or a_peripheral state, udc should had started, isn't it?
> 
> If gadget function driver (which is different from UDC driver) is not yet loaded
> then we can't start UDC even if otg fsm is in b_peripheral.
> Also, if userspace has disabled softconnect we can't start UDC.
> 
> So, b_peripheral != UDC_started.
> 
> I've tried to address this issue by adding the checks in usb_gadget_start().
> 

Ok, maybe we have different understanding for 'B-Device' at software,
In spec, it says the Micro-AB receptacle with nothing connected can be
'B-Device', but in fact, we may not enable device mode before loading
gadget driver, in chipidea fsm design, if the gadget driver is not
loaded, the FSM will not start, and it is at OTG_STATE_UNDEFINED.

One more thing is we may need to find a place to issue SRP when we
load gadget driver, since we may at b_idle  at that time due to host
closes the vbus (timeout for a_wait_bcon).

What is the "softconnect" used for? In otg fsm, we use b_bus_req for FSM.

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-19 10:18     ` Roger Quadros
@ 2015-03-20  7:45       ` Peter Chen
  2015-03-20  9:18         ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Peter Chen @ 2015-03-20  7:45 UTC (permalink / raw)
  To: Roger Quadros
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On Thu, Mar 19, 2015 at 12:18:55PM +0200, Roger Quadros wrote:
> On 19/03/15 05:40, Peter Chen wrote:
> > On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
> >> The OTG core instantiates the OTG Finite State Machine
> >> per OTG controller and manages starting/stopping the
> >> host and gadget controllers based on the bus state.
> >>
> >> It provides APIs for the following tasks
> >>
> >> - Registering an OTG capable controller
> >> - Registering Host and Gadget controllers to OTG core
> >> - Providing inputs to and kicking the OTG state machine
> >>
> >> TODO:
> >> - sysfs interface to allow application inputs to OTG state machine
> >> - otg class?
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >>  drivers/usb/Makefile         |   1 +
> >>  drivers/usb/common/Makefile  |   1 +
> >>  drivers/usb/common/usb-otg.c | 732 +++++++++++++++++++++++++++++++++++++++++++
> >>  drivers/usb/common/usb-otg.h |  71 +++++
> >>  drivers/usb/core/Kconfig     |   8 +
> >>  include/linux/usb/usb-otg.h  |  86 +++++
> >>  6 files changed, 899 insertions(+)
> >>  create mode 100644 drivers/usb/common/usb-otg.c
> >>  create mode 100644 drivers/usb/common/usb-otg.h
> >>  create mode 100644 include/linux/usb/usb-otg.h
> >>
> >> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> >> index 2f1e2aa..07f59a5 100644
> >> --- a/drivers/usb/Makefile
> >> +++ b/drivers/usb/Makefile
> >> @@ -60,5 +60,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
> >>  obj-$(CONFIG_USB_GADGET)	+= gadget/
> >>  
> >>  obj-$(CONFIG_USB_COMMON)	+= common/
> >> +obj-$(CONFIG_USB_OTG_CORE)	+= common/
> >>  
> >>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> >> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
> >> index ca2f8bd..573fc75 100644
> >> --- a/drivers/usb/common/Makefile
> >> +++ b/drivers/usb/common/Makefile
> >> @@ -7,3 +7,4 @@ usb-common-y			  += common.o
> >>  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
> >>  
> >>  obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
> >> +obj-$(CONFIG_USB_OTG_CORE) += usb-otg.o
> >> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> >> new file mode 100644
> >> index 0000000..1433fc9
> >> --- /dev/null
> >> +++ b/drivers/usb/common/usb-otg.c
> >> @@ -0,0 +1,732 @@
> >> +/**
> >> + * drivers/usb/common/usb-otg.c - USB OTG core
> >> + *
> >> + * Copyright (C) 2015 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/timer.h>
> >> +#include <linux/usb/otg.h>
> >> +#include <linux/usb/phy.h>	/* enum usb_otg_state */
> >> +#include <linux/usb/gadget.h>
> >> +#include <linux/usb/usb-otg.h>
> >> +#include <linux/workqueue.h>
> >> +
> >> +#include "usb-otg.h"
> >> +
> >> +/* to link timer with callback data */
> >> +struct otg_timer {
> >> +	struct timer_list timer;
> >> +	/* callback data */
> >> +	int *timeout_bit;
> >> +	struct otg_data *otgd;
> >> +};
> >> +
> >> +struct otg_data {
> >> +	struct device *dev;	/* HCD & GCD's parent device */
> >> +
> >> +	struct otg_fsm fsm;
> >> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
> >> +	 * HCD is bus_to_hcd(fsm->otg->host)
> >> +	 * GCD is fsm->otg->gadget
> >> +	 */
> >> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
> >> +	struct usb_otg otg;
> >> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
> >> +
> >> +	/* saved hooks to OTG device */
> >> +	int (*start_host)(struct otg_fsm *fsm, int on);
> >> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
> >> +
> >> +	struct list_head list;
> >> +
> >> +	struct work_struct work;	/* OTG FSM work */
> >> +	struct workqueue_struct *wq;
> >> +
> >> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
> >> +
> >> +	bool fsm_running;
> >> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
> >> +	bool host_can_start;		/* OTG FSM says host can start */
> >> +
> >> +	/* use otg->fsm.lock for serializing access */
> >> +};
> > 
> > What's the relationship between struct usb_otg otg and this one?
> 
> Did you mean why struct usb_otg otg is there in struct otg_data?
> Just for easy allocation. fsm_ops only contains the pointer to struct usb_otg.
> 

The reason why I ask this question is the most structures at usb_otg
(only enum usb_otg_state)are useless for otg_fsm, this structure may
only for hardware otg fsm driver, so your OTG framework should only
for software FSM drivers, right?

Peter

> > 
> >> +
> >> +/* OTG device list */
> >> +LIST_HEAD(otg_list);
> >> +static DEFINE_MUTEX(otg_list_mutex);
> >> +
> >> +/**
> >> + * check if device is in our OTG list and return
> >> + * otg_data, else NULL.
> >> + *
> >> + * otg_list_mutex must be held.
> >> + */
> >> +static struct otg_data *usb_otg_device_get_otgd(struct device *parent_dev)
> >> +{
> >> +	struct otg_data *otgd;
> >> +
> >> +	list_for_each_entry(otgd, &otg_list, list) {
> >> +		if (otgd->dev == parent_dev)
> >> +			return otgd;
> >> +	}
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >> +/**
> >> + * timer callback to set timeout bit and kick FSM
> >> + */
> >> +static void set_tmout(unsigned long data)
> >> +{
> >> +	struct otg_timer *tmr_data;
> >> +
> >> +	tmr_data = (struct otg_timer *)data;
> >> +
> >> +	if (tmr_data->timeout_bit)
> >> +		*tmr_data->timeout_bit = 1;
> >> +
> >> +	usb_otg_sync_inputs(&tmr_data->otgd->fsm);
> >> +}
> >> +
> >> +/**
> >> + * Initialize one OTG timer with callback, timeout and timeout bit
> >> + */
> >> +static void otg_timer_init(enum otg_fsm_timer id, struct otg_data *otgd,
> >> +			   void (*callback)(unsigned long),
> >> +			   unsigned long expires_ms,
> >> +			   int *timeout_bit)
> >> +{
> >> +	struct otg_timer *otgtimer = &otgd->timers[id];
> >> +	struct timer_list *timer = &otgtimer->timer;
> >> +
> >> +	init_timer(timer);
> >> +	timer->function = callback;
> >> +	timer->expires = jiffies + msecs_to_jiffies(expires_ms);
> >> +	timer->data = (unsigned long)otgtimer;
> >> +
> > 
> > The timer for TB_DATA_PLS is about 10ms or less, it is not suitable
> > for using kernel timer, hrtimer is suitable choice.
> 
> good catch. I will switch to hrtimer then.
> 
> > 
> >> +	otgtimer->timeout_bit = timeout_bit;
> >> +	otgtimer->otgd = otgd;
> >> +}
> >> +
> >> +/**
> >> + * Initialize standard OTG timers
> >> + */
> >> +static void usb_otg_init_timers(struct otg_data *otgd)
> >> +{
> >> +	struct otg_fsm *fsm = &otgd->fsm;
> >> +
> >> +	otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE, &fsm->a_wait_vrise_tmout);
> >> +	otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL, &fsm->a_wait_vfall_tmout);
> >> +	otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON, &fsm->a_wait_bcon_tmout);
> >> +	otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS, &fsm->a_aidl_bdis_tmout);
> >> +	otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS, &fsm->a_bidl_adis_tmout);
> >> +	otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST, &fsm->b_ase0_brst_tmout);
> >> +
> >> +	otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP, &fsm->b_se0_srp);
> >> +	otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL, &fsm->b_srp_done);
> >> +
> >> +	otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
> >> +}
> 
> <snip>
> 
> cheers,
> -roger
> 

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-20  7:45       ` Peter Chen
@ 2015-03-20  9:18         ` Roger Quadros
  2015-03-20  9:32           ` Peter Chen
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-20  9:18 UTC (permalink / raw)
  To: Peter Chen
  Cc: gregkh, balbi, stern, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On 20/03/15 09:45, Peter Chen wrote:
> On Thu, Mar 19, 2015 at 12:18:55PM +0200, Roger Quadros wrote:
>> On 19/03/15 05:40, Peter Chen wrote:
>>> On Wed, Mar 18, 2015 at 03:55:57PM +0200, Roger Quadros wrote:
>>>> The OTG core instantiates the OTG Finite State Machine
>>>> per OTG controller and manages starting/stopping the
>>>> host and gadget controllers based on the bus state.
>>>>
>>>> It provides APIs for the following tasks
>>>>
>>>> - Registering an OTG capable controller
>>>> - Registering Host and Gadget controllers to OTG core
>>>> - Providing inputs to and kicking the OTG state machine
>>>>
>>>> TODO:
>>>> - sysfs interface to allow application inputs to OTG state machine
>>>> - otg class?
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>  drivers/usb/Makefile         |   1 +
>>>>  drivers/usb/common/Makefile  |   1 +
>>>>  drivers/usb/common/usb-otg.c | 732 +++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/usb/common/usb-otg.h |  71 +++++
>>>>  drivers/usb/core/Kconfig     |   8 +
>>>>  include/linux/usb/usb-otg.h  |  86 +++++
>>>>  6 files changed, 899 insertions(+)
>>>>  create mode 100644 drivers/usb/common/usb-otg.c
>>>>  create mode 100644 drivers/usb/common/usb-otg.h
>>>>  create mode 100644 include/linux/usb/usb-otg.h
>>>>
>>>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
>>>> index 2f1e2aa..07f59a5 100644
>>>> --- a/drivers/usb/Makefile
>>>> +++ b/drivers/usb/Makefile
>>>> @@ -60,5 +60,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
>>>>  obj-$(CONFIG_USB_GADGET)	+= gadget/
>>>>  
>>>>  obj-$(CONFIG_USB_COMMON)	+= common/
>>>> +obj-$(CONFIG_USB_OTG_CORE)	+= common/
>>>>  
>>>>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
>>>> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
>>>> index ca2f8bd..573fc75 100644
>>>> --- a/drivers/usb/common/Makefile
>>>> +++ b/drivers/usb/common/Makefile
>>>> @@ -7,3 +7,4 @@ usb-common-y			  += common.o
>>>>  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>>>>  
>>>>  obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
>>>> +obj-$(CONFIG_USB_OTG_CORE) += usb-otg.o
>>>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>>>> new file mode 100644
>>>> index 0000000..1433fc9
>>>> --- /dev/null
>>>> +++ b/drivers/usb/common/usb-otg.c
>>>> @@ -0,0 +1,732 @@
>>>> +/**
>>>> + * drivers/usb/common/usb-otg.c - USB OTG core
>>>> + *
>>>> + * Copyright (C) 2015 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/timer.h>
>>>> +#include <linux/usb/otg.h>
>>>> +#include <linux/usb/phy.h>	/* enum usb_otg_state */
>>>> +#include <linux/usb/gadget.h>
>>>> +#include <linux/usb/usb-otg.h>
>>>> +#include <linux/workqueue.h>
>>>> +
>>>> +#include "usb-otg.h"
>>>> +
>>>> +/* to link timer with callback data */
>>>> +struct otg_timer {
>>>> +	struct timer_list timer;
>>>> +	/* callback data */
>>>> +	int *timeout_bit;
>>>> +	struct otg_data *otgd;
>>>> +};
>>>> +
>>>> +struct otg_data {
>>>> +	struct device *dev;	/* HCD & GCD's parent device */
>>>> +
>>>> +	struct otg_fsm fsm;
>>>> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
>>>> +	 * HCD is bus_to_hcd(fsm->otg->host)
>>>> +	 * GCD is fsm->otg->gadget
>>>> +	 */
>>>> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
>>>> +	struct usb_otg otg;
>>>> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
>>>> +
>>>> +	/* saved hooks to OTG device */
>>>> +	int (*start_host)(struct otg_fsm *fsm, int on);
>>>> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
>>>> +
>>>> +	struct list_head list;
>>>> +
>>>> +	struct work_struct work;	/* OTG FSM work */
>>>> +	struct workqueue_struct *wq;
>>>> +
>>>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
>>>> +
>>>> +	bool fsm_running;
>>>> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
>>>> +	bool host_can_start;		/* OTG FSM says host can start */
>>>> +
>>>> +	/* use otg->fsm.lock for serializing access */
>>>> +};
>>>
>>> What's the relationship between struct usb_otg otg and this one?
>>
>> Did you mean why struct usb_otg otg is there in struct otg_data?
>> Just for easy allocation. fsm_ops only contains the pointer to struct usb_otg.
>>
> 
> The reason why I ask this question is the most structures at usb_otg
> (only enum usb_otg_state)are useless for otg_fsm, this structure may
> only for hardware otg fsm driver, so your OTG framework should only
> for software FSM drivers, right?

right. we only need it for enum usb_otg_state.
Do you see how we can improve it?

cheers,
-roger

> 
> Peter
> 
>>>
>>>> +
>>>> +/* OTG device list */
>>>> +LIST_HEAD(otg_list);
>>>> +static DEFINE_MUTEX(otg_list_mutex);
>>>> +
>>>> +/**
>>>> + * check if device is in our OTG list and return
>>>> + * otg_data, else NULL.
>>>> + *
>>>> + * otg_list_mutex must be held.
>>>> + */
>>>> +static struct otg_data *usb_otg_device_get_otgd(struct device *parent_dev)
>>>> +{
>>>> +	struct otg_data *otgd;
>>>> +
>>>> +	list_for_each_entry(otgd, &otg_list, list) {
>>>> +		if (otgd->dev == parent_dev)
>>>> +			return otgd;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * timer callback to set timeout bit and kick FSM
>>>> + */
>>>> +static void set_tmout(unsigned long data)
>>>> +{
>>>> +	struct otg_timer *tmr_data;
>>>> +
>>>> +	tmr_data = (struct otg_timer *)data;
>>>> +
>>>> +	if (tmr_data->timeout_bit)
>>>> +		*tmr_data->timeout_bit = 1;
>>>> +
>>>> +	usb_otg_sync_inputs(&tmr_data->otgd->fsm);
>>>> +}
>>>> +
>>>> +/**
>>>> + * Initialize one OTG timer with callback, timeout and timeout bit
>>>> + */
>>>> +static void otg_timer_init(enum otg_fsm_timer id, struct otg_data *otgd,
>>>> +			   void (*callback)(unsigned long),
>>>> +			   unsigned long expires_ms,
>>>> +			   int *timeout_bit)
>>>> +{
>>>> +	struct otg_timer *otgtimer = &otgd->timers[id];
>>>> +	struct timer_list *timer = &otgtimer->timer;
>>>> +
>>>> +	init_timer(timer);
>>>> +	timer->function = callback;
>>>> +	timer->expires = jiffies + msecs_to_jiffies(expires_ms);
>>>> +	timer->data = (unsigned long)otgtimer;
>>>> +
>>>
>>> The timer for TB_DATA_PLS is about 10ms or less, it is not suitable
>>> for using kernel timer, hrtimer is suitable choice.
>>
>> good catch. I will switch to hrtimer then.
>>
>>>
>>>> +	otgtimer->timeout_bit = timeout_bit;
>>>> +	otgtimer->otgd = otgd;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Initialize standard OTG timers
>>>> + */
>>>> +static void usb_otg_init_timers(struct otg_data *otgd)
>>>> +{
>>>> +	struct otg_fsm *fsm = &otgd->fsm;
>>>> +
>>>> +	otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE, &fsm->a_wait_vrise_tmout);
>>>> +	otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL, &fsm->a_wait_vfall_tmout);
>>>> +	otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON, &fsm->a_wait_bcon_tmout);
>>>> +	otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS, &fsm->a_aidl_bdis_tmout);
>>>> +	otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS, &fsm->a_bidl_adis_tmout);
>>>> +	otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST, &fsm->b_ase0_brst_tmout);
>>>> +
>>>> +	otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP, &fsm->b_se0_srp);
>>>> +	otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL, &fsm->b_srp_done);
>>>> +
>>>> +	otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
>>>> +}
>>
>> <snip>
>>
>> cheers,
>> -roger
>>
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [RFC][PATCH 3/9] usb: otg: add OTG core
  2015-03-20  9:18         ` Roger Quadros
@ 2015-03-20  9:32           ` Peter Chen
  0 siblings, 0 replies; 40+ messages in thread
From: Peter Chen @ 2015-03-20  9:32 UTC (permalink / raw)
  To: Roger Quadros, balbi
  Cc: gregkh, stern, dan.j.williams, Jun.Li, mathias.nyman, linux-usb,
	linux-kernel, linux-omap

 
> >>>>
> >>>> - Registering an OTG capable controller
> >>>> - Registering Host and Gadget controllers to OTG core
> >>>> - Providing inputs to and kicking the OTG state machine
> >>>>
> >>>> TODO:
> >>>> - sysfs interface to allow application inputs to OTG state machine
> >>>> - otg class?
> >>>>
> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>>> ---
> >>>>  drivers/usb/Makefile         |   1 +
> >>>>  drivers/usb/common/Makefile  |   1 +
> >>>>  drivers/usb/common/usb-otg.c | 732
> >>>> +++++++++++++++++++++++++++++++++++++++++++
> >>>>  drivers/usb/common/usb-otg.h |  71 +++++
> >>>>  drivers/usb/core/Kconfig     |   8 +
> >>>>  include/linux/usb/usb-otg.h  |  86 +++++
> >>>>  6 files changed, 899 insertions(+)  create mode 100644
> >>>> drivers/usb/common/usb-otg.c  create mode 100644
> >>>> drivers/usb/common/usb-otg.h  create mode 100644
> >>>> include/linux/usb/usb-otg.h
> >>>>
> >>>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile index
> >>>> 2f1e2aa..07f59a5 100644
> >>>> --- a/drivers/usb/Makefile
> >>>> +++ b/drivers/usb/Makefile
> >>>> @@ -60,5 +60,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+=
> renesas_usbhs/
> >>>>  obj-$(CONFIG_USB_GADGET)	+= gadget/
> >>>>
> >>>>  obj-$(CONFIG_USB_COMMON)	+= common/
> >>>> +obj-$(CONFIG_USB_OTG_CORE)	+= common/
> >>>>
> >>>>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> >>>> diff --git a/drivers/usb/common/Makefile
> >>>> b/drivers/usb/common/Makefile index ca2f8bd..573fc75 100644
> >>>> --- a/drivers/usb/common/Makefile
> >>>> +++ b/drivers/usb/common/Makefile
> >>>> @@ -7,3 +7,4 @@ usb-common-y			  += common.o
> >>>>  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
> >>>>
> >>>>  obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
> >>>> +obj-$(CONFIG_USB_OTG_CORE) += usb-otg.o
> >>>> diff --git a/drivers/usb/common/usb-otg.c
> >>>> b/drivers/usb/common/usb-otg.c new file mode 100644 index
> >>>> 0000000..1433fc9
> >>>> --- /dev/null
> >>>> +++ b/drivers/usb/common/usb-otg.c
> >>>> @@ -0,0 +1,732 @@
> >>>> +/**
> >>>> + * drivers/usb/common/usb-otg.c - USB OTG core
> >>>> + *
> >>>> + * Copyright (C) 2015 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/timer.h>
> >>>> +#include <linux/usb/otg.h>
> >>>> +#include <linux/usb/phy.h>	/* enum usb_otg_state */
> >>>> +#include <linux/usb/gadget.h>
> >>>> +#include <linux/usb/usb-otg.h>
> >>>> +#include <linux/workqueue.h>
> >>>> +
> >>>> +#include "usb-otg.h"
> >>>> +
> >>>> +/* to link timer with callback data */ struct otg_timer {
> >>>> +	struct timer_list timer;
> >>>> +	/* callback data */
> >>>> +	int *timeout_bit;
> >>>> +	struct otg_data *otgd;
> >>>> +};
> >>>> +
> >>>> +struct otg_data {
> >>>> +	struct device *dev;	/* HCD & GCD's parent device */
> >>>> +
> >>>> +	struct otg_fsm fsm;
> >>>> +	/* HCD, GCD and usb_otg_state are present in otg_fsm->otg
> >>>> +	 * HCD is bus_to_hcd(fsm->otg->host)
> >>>> +	 * GCD is fsm->otg->gadget
> >>>> +	 */
> >>>> +	struct otg_fsm_ops fsm_ops;	/* private copy for override */
> >>>> +	struct usb_otg otg;
> >>>> +	struct usb_hcd *shared_hcd;	/* if shared HCD registered */
> >>>> +
> >>>> +	/* saved hooks to OTG device */
> >>>> +	int (*start_host)(struct otg_fsm *fsm, int on);
> >>>> +	int (*start_gadget)(struct otg_fsm *fsm, int on);
> >>>> +
> >>>> +	struct list_head list;
> >>>> +
> >>>> +	struct work_struct work;	/* OTG FSM work */
> >>>> +	struct workqueue_struct *wq;
> >>>> +
> >>>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
> >>>> +
> >>>> +	bool fsm_running;
> >>>> +	bool gadget_can_start;		/* OTG FSM says gadget can start */
> >>>> +	bool host_can_start;		/* OTG FSM says host can start */
> >>>> +
> >>>> +	/* use otg->fsm.lock for serializing access */ };
> >>>
> >>> What's the relationship between struct usb_otg otg and this one?
> >>
> >> Did you mean why struct usb_otg otg is there in struct otg_data?
> >> Just for easy allocation. fsm_ops only contains the pointer to struct usb_otg.
> >>
> >
> > The reason why I ask this question is the most structures at usb_otg
> > (only enum usb_otg_state)are useless for otg_fsm, this structure may
> > only for hardware otg fsm driver, so your OTG framework should only
> > for software FSM drivers, right?
> 
> right. we only need it for enum usb_otg_state.
> Do you see how we can improve it?
> 

Felipe & Roger, if we need to go on supporting legacies otg driver, we need to
keep struct usb_otg unchanged, and teach new otg driver using Roger's framework.

Peter


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-20  7:18           ` Peter Chen
@ 2015-03-20  9:46             ` Roger Quadros
  2015-03-20 11:08               ` Roger Quadros
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-20  9:46 UTC (permalink / raw)
  To: Peter Chen
  Cc: Li Jun, gregkh, balbi, stern, dan.j.williams, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On 20/03/15 09:18, Peter Chen wrote:
> On Thu, Mar 19, 2015 at 04:50:31PM +0200, Roger Quadros wrote:
>> On 19/03/15 16:09, Li Jun wrote:
>>> On Thu, Mar 19, 2015 at 12:14:39PM +0200, Roger Quadros wrote:
>>>> On 19/03/15 05:30, Peter Chen wrote:
>>>>> On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
>>>>>> The OTG state machine needs a mechanism to start and
>>>>>> stop the gadget controller. Add usb_gadget_start()
>>>>>> and usb_gadget_stop().
>>>>>>
>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>> ---
>>>>>>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
>>>>>>  include/linux/usb/gadget.h        |   3 +
>>>>>>  2 files changed, 158 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>>>>>> index 5a81cb0..69b4123 100644
>>>>>> --- a/drivers/usb/gadget/udc/udc-core.c
>>>>>> +++ b/drivers/usb/gadget/udc/udc-core.c
>>>>>> @@ -35,6 +35,8 @@
>>>>>>   * @dev - the child device to the actual controller
>>>>>>   * @gadget - the gadget. For use by the class code
>>>>>>   * @list - for use by the udc class driver
>>>>>> + * @running - udc is running
>>>>>
>>>>> Doesn't OTG FSM should know it?
>>>>
>>>> Not really, as the gadget driver might not have been loaded yet or userspace might
>>>> have disabled softconnect when the OTG FSM wants UDC to start.
>>>>
>>>> So only UDC knows if it has really started or not based on this flag.
>>>>
>>>
>>> why this can not be known by check the otg fsm state? i.e. if the device in
>>> b_peripheral or a_peripheral state, udc should had started, isn't it?
>>
>> If gadget function driver (which is different from UDC driver) is not yet loaded
>> then we can't start UDC even if otg fsm is in b_peripheral.
>> Also, if userspace has disabled softconnect we can't start UDC.
>>
>> So, b_peripheral != UDC_started.
>>
>> I've tried to address this issue by adding the checks in usb_gadget_start().
>>
> 
> Ok, maybe we have different understanding for 'B-Device' at software,
> In spec, it says the Micro-AB receptacle with nothing connected can be
> 'B-Device', but in fact, we may not enable device mode before loading
> gadget driver, in chipidea fsm design, if the gadget driver is not
> loaded, the FSM will not start, and it is at OTG_STATE_UNDEFINED.

Right. I mixed up into thinking that we should respect the softconnect
while in OTG mode. It seems that we should ignore it.

> 
> One more thing is we may need to find a place to issue SRP when we
> load gadget driver, since we may at b_idle  at that time due to host
> closes the vbus (timeout for a_wait_bcon).

Issuing SRP should be done by the otg-fsm and not udc-core.
The udc-core can at the least call usb_otg_kick_fsm() after setting the gadget
driver so that otg-fsm knows that we now have a valid gadget and can take
necessary action. i.e. change from b_idle to b_srp_init and then to b_peripheral.

> 
> What is the "softconnect" used for? In otg fsm, we use b_bus_req for FSM.
> 
I understand now that we shouldn't bother with softconnect when we are in OTG fsm mode.
That solves our problem with the running flags.

So now, b_peripheral == UDC_started.

I will address this in v2.

cheers,
-roger

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
  2015-03-20  6:32       ` Peter Chen
@ 2015-03-20  9:49         ` Roger Quadros
  0 siblings, 0 replies; 40+ messages in thread
From: Roger Quadros @ 2015-03-20  9:49 UTC (permalink / raw)
  To: Peter Chen
  Cc: Alan Stern, gregkh, balbi, dan.j.williams, jun.li, mathias.nyman,
	linux-usb, linux-kernel, linux-omap

On 20/03/15 08:32, Peter Chen wrote:
> On Thu, Mar 19, 2015 at 01:38:32PM +0200, Roger Quadros wrote:
>> On 18/03/15 21:49, Alan Stern wrote:
>>> On Wed, 18 Mar 2015, Roger Quadros wrote:
>>>
>>>> To support OTG we want a mechanism to start and stop
>>>> the HCD from the OTG state machine. Add usb_start_hcd()
>>>> and usb_stop_hcd().
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> There are a few problems in this proposed patch.
>>>
>>>> +int usb_start_hcd(struct usb_hcd *hcd)
>>>> +{
>>>> +	int retval;
>>>> +	struct usb_device *rhdev = hcd->self.root_hub;
>>>> +
>>>> +	if (hcd->state != HC_STATE_HALT) {
>>>> +		dev_err(hcd->self.controller, "not starting a running HCD\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	hcd->state = HC_STATE_RUNNING;
>>>> +	retval = hcd->driver->start(hcd);
>>>> +	if (retval < 0) {
>>>> +		dev_err(hcd->self.controller, "startup error %d\n", retval);
>>>> +		hcd->state = HC_STATE_HALT;
>>>> +		return retval;
>>>> +	}
>>>> +
>>>> +	/* starting here, usbcore will pay attention to this root hub */
>>>> +	if ((retval = register_root_hub(hcd)) != 0)
>>>> +		goto err_register_root_hub;
>>>
>>> If the host controller is started more than once, you will end up
>>> unregistering and re-registering the root hub.  The device core does
>>> not allow this.  Once a device has been unregistered, you must not try
>>> to register it again -- you have to allocate a new device and register
>>> it instead.
>>
>> Understood.
>>
>>>
>>> Also, although you call the driver's ->start method multiple times, the
>>> ->reset method is called only once, when the controller is first 
>>> probed.  It's not clear that this will work in a situation where the HC 
>>> and the UDC share hardware state; after the UDC is stopped it may be 
>>> necessary to reset the HC before it can run again.
>>
>> Yes, good point.
>>>
>>> It might be possible to make this work, but I suspect quite a few 
>>> drivers would need rewriting first.  As another example of the problems 
>>> you face, consider how stopping a host controller will interact with 
>>> the driver's PM support (both system suspend and runtime suspend).
>>
>> Right. This needs more work than I thought.
>>>
>>> It would be a lot simpler to unbind the host controller driver
>>> completely when switching to device mode and rebind it when switching
>>> back.  I guess that is the sort of heavy-duty approach you want to
>>> avoid, but it may be the only practical way forward.
>>
>> So you mean directly calling usb_add/remove_hcd() from the OTG core?
>> I don't see any issues with that other than it being a heavy-duty operation
>> like you said and hope that it doesn't violate the OTG spec timing.
>>
>> Looking at Figure 5-3: "HNP Sequence of Events (FS)" of the OTG 2.0 spec
>> we have about 150ms (X10) to switch from B-Device detected A connect (b_wait_acon state)
>> to driving bus reset (b_host state). I don't think this should be an issue in modern SoCs
>> but I'm not very sure.
>>
> 
> It is not related toadd/remove hcd, it is the time from receiving PCD
> to issue BUS_RESET, the Linux stack can't satisfy OTG spec (150ms) due
> to there are some de-bounce waitings.

OK.

> 
>> In any case I can migrate to the add/remove hcd approach to simplify things.
>>
> 
> It should be no problem, we use it more than 1 years.
> 

Good to know this.

cheers,
-roger

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-20  9:46             ` Roger Quadros
@ 2015-03-20 11:08               ` Roger Quadros
  2015-03-21  1:30                 ` Peter Chen
  0 siblings, 1 reply; 40+ messages in thread
From: Roger Quadros @ 2015-03-20 11:08 UTC (permalink / raw)
  To: Peter Chen
  Cc: Li Jun, gregkh, balbi, stern, dan.j.williams, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On 20/03/15 11:46, Roger Quadros wrote:
> On 20/03/15 09:18, Peter Chen wrote:
>> On Thu, Mar 19, 2015 at 04:50:31PM +0200, Roger Quadros wrote:
>>> On 19/03/15 16:09, Li Jun wrote:
>>>> On Thu, Mar 19, 2015 at 12:14:39PM +0200, Roger Quadros wrote:
>>>>> On 19/03/15 05:30, Peter Chen wrote:
>>>>>> On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
>>>>>>> The OTG state machine needs a mechanism to start and
>>>>>>> stop the gadget controller. Add usb_gadget_start()
>>>>>>> and usb_gadget_stop().
>>>>>>>
>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>> ---
>>>>>>>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
>>>>>>>  include/linux/usb/gadget.h        |   3 +
>>>>>>>  2 files changed, 158 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
>>>>>>> index 5a81cb0..69b4123 100644
>>>>>>> --- a/drivers/usb/gadget/udc/udc-core.c
>>>>>>> +++ b/drivers/usb/gadget/udc/udc-core.c
>>>>>>> @@ -35,6 +35,8 @@
>>>>>>>   * @dev - the child device to the actual controller
>>>>>>>   * @gadget - the gadget. For use by the class code
>>>>>>>   * @list - for use by the udc class driver
>>>>>>> + * @running - udc is running
>>>>>>
>>>>>> Doesn't OTG FSM should know it?
>>>>>
>>>>> Not really, as the gadget driver might not have been loaded yet or userspace might
>>>>> have disabled softconnect when the OTG FSM wants UDC to start.
>>>>>
>>>>> So only UDC knows if it has really started or not based on this flag.
>>>>>
>>>>
>>>> why this can not be known by check the otg fsm state? i.e. if the device in
>>>> b_peripheral or a_peripheral state, udc should had started, isn't it?
>>>
>>> If gadget function driver (which is different from UDC driver) is not yet loaded
>>> then we can't start UDC even if otg fsm is in b_peripheral.
>>> Also, if userspace has disabled softconnect we can't start UDC.
>>>
>>> So, b_peripheral != UDC_started.
>>>
>>> I've tried to address this issue by adding the checks in usb_gadget_start().
>>>
>>
>> Ok, maybe we have different understanding for 'B-Device' at software,
>> In spec, it says the Micro-AB receptacle with nothing connected can be
>> 'B-Device', but in fact, we may not enable device mode before loading
>> gadget driver, in chipidea fsm design, if the gadget driver is not
>> loaded, the FSM will not start, and it is at OTG_STATE_UNDEFINED.
> 
> Right. I mixed up into thinking that we should respect the softconnect
> while in OTG mode. It seems that we should ignore it.
> 
>>
>> One more thing is we may need to find a place to issue SRP when we
>> load gadget driver, since we may at b_idle  at that time due to host
>> closes the vbus (timeout for a_wait_bcon).
> 
> Issuing SRP should be done by the otg-fsm and not udc-core.
> The udc-core can at the least call usb_otg_kick_fsm() after setting the gadget
> driver so that otg-fsm knows that we now have a valid gadget and can take
> necessary action. i.e. change from b_idle to b_srp_init and then to b_peripheral.

To clarify further. Is it right to assume that OTG FSM will not be started till
both gadget UDC driver _AND_ gadget function driver are loaded?

And it will be stopped when either of them unloads.

This simplifies things a lot.

cheers,
-roger

> 
>>
>> What is the "softconnect" used for? In otg fsm, we use b_bus_req for FSM.
>>
> I understand now that we shouldn't bother with softconnect when we are in OTG fsm mode.
> That solves our problem with the running flags.
> 
> So now, b_peripheral == UDC_started.
> 
> I will address this in v2.
> 
> cheers,
> -roger
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop()
  2015-03-20 11:08               ` Roger Quadros
@ 2015-03-21  1:30                 ` Peter Chen
  0 siblings, 0 replies; 40+ messages in thread
From: Peter Chen @ 2015-03-21  1:30 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Li Jun, gregkh, balbi, stern, dan.j.williams, jun.li,
	mathias.nyman, linux-usb, linux-kernel, linux-omap

On Fri, Mar 20, 2015 at 01:08:25PM +0200, Roger Quadros wrote:
> On 20/03/15 11:46, Roger Quadros wrote:
> > On 20/03/15 09:18, Peter Chen wrote:
> >> On Thu, Mar 19, 2015 at 04:50:31PM +0200, Roger Quadros wrote:
> >>> On 19/03/15 16:09, Li Jun wrote:
> >>>> On Thu, Mar 19, 2015 at 12:14:39PM +0200, Roger Quadros wrote:
> >>>>> On 19/03/15 05:30, Peter Chen wrote:
> >>>>>> On Wed, Mar 18, 2015 at 03:55:56PM +0200, Roger Quadros wrote:
> >>>>>>> The OTG state machine needs a mechanism to start and
> >>>>>>> stop the gadget controller. Add usb_gadget_start()
> >>>>>>> and usb_gadget_stop().
> >>>>>>>
> >>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>>>>>> ---
> >>>>>>>  drivers/usb/gadget/udc/udc-core.c | 166 +++++++++++++++++++++++++++++++++++---
> >>>>>>>  include/linux/usb/gadget.h        |   3 +
> >>>>>>>  2 files changed, 158 insertions(+), 11 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> >>>>>>> index 5a81cb0..69b4123 100644
> >>>>>>> --- a/drivers/usb/gadget/udc/udc-core.c
> >>>>>>> +++ b/drivers/usb/gadget/udc/udc-core.c
> >>>>>>> @@ -35,6 +35,8 @@
> >>>>>>>   * @dev - the child device to the actual controller
> >>>>>>>   * @gadget - the gadget. For use by the class code
> >>>>>>>   * @list - for use by the udc class driver
> >>>>>>> + * @running - udc is running
> >>>>>>
> >>>>>> Doesn't OTG FSM should know it?
> >>>>>
> >>>>> Not really, as the gadget driver might not have been loaded yet or userspace might
> >>>>> have disabled softconnect when the OTG FSM wants UDC to start.
> >>>>>
> >>>>> So only UDC knows if it has really started or not based on this flag.
> >>>>>
> >>>>
> >>>> why this can not be known by check the otg fsm state? i.e. if the device in
> >>>> b_peripheral or a_peripheral state, udc should had started, isn't it?
> >>>
> >>> If gadget function driver (which is different from UDC driver) is not yet loaded
> >>> then we can't start UDC even if otg fsm is in b_peripheral.
> >>> Also, if userspace has disabled softconnect we can't start UDC.
> >>>
> >>> So, b_peripheral != UDC_started.
> >>>
> >>> I've tried to address this issue by adding the checks in usb_gadget_start().
> >>>
> >>
> >> Ok, maybe we have different understanding for 'B-Device' at software,
> >> In spec, it says the Micro-AB receptacle with nothing connected can be
> >> 'B-Device', but in fact, we may not enable device mode before loading
> >> gadget driver, in chipidea fsm design, if the gadget driver is not
> >> loaded, the FSM will not start, and it is at OTG_STATE_UNDEFINED.
> > 
> > Right. I mixed up into thinking that we should respect the softconnect
> > while in OTG mode. It seems that we should ignore it.
> > 
> >>
> >> One more thing is we may need to find a place to issue SRP when we
> >> load gadget driver, since we may at b_idle  at that time due to host
> >> closes the vbus (timeout for a_wait_bcon).
> > 
> > Issuing SRP should be done by the otg-fsm and not udc-core.
> > The udc-core can at the least call usb_otg_kick_fsm() after setting the gadget
> > driver so that otg-fsm knows that we now have a valid gadget and can take
> > necessary action. i.e. change from b_idle to b_srp_init and then to b_peripheral.
> 
> To clarify further. Is it right to assume that OTG FSM will not be started till
> both gadget UDC driver _AND_ gadget function driver are loaded?
> 
> And it will be stopped when either of them unloads.
> 
> This simplifies things a lot.

Yes, you are right.

> 
> cheers,
> -roger
> 
> > 
> >>
> >> What is the "softconnect" used for? In otg fsm, we use b_bus_req for FSM.
> >>
> > I understand now that we shouldn't bother with softconnect when we are in OTG fsm mode.
> > That solves our problem with the running flags.
> > 
> > So now, b_peripheral == UDC_started.
> > 
> > I will address this in v2.
> > 
> > cheers,
> > -roger
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

-- 

Best Regards,
Peter Chen

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2015-03-21  1:31 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-18 13:55 [RFC][PATCH 0/9] USB: OTG Core functionality Roger Quadros
2015-03-18 13:55 ` [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd() Roger Quadros
2015-03-18 19:49   ` Alan Stern
2015-03-18 21:41     ` Tony Lindgren
2015-03-19  1:51       ` Alan Stern
2015-03-19  2:38         ` Tony Lindgren
2015-03-19 11:38     ` Roger Quadros
2015-03-19 14:17       ` Alan Stern
2015-03-20  6:32       ` Peter Chen
2015-03-20  9:49         ` Roger Quadros
2015-03-19  1:46   ` Peter Chen
2015-03-18 13:55 ` [RFC][PATCH 2/9] usb: gadget: add usb_gadget_start/stop() Roger Quadros
2015-03-19  3:30   ` Peter Chen
2015-03-19 10:14     ` Roger Quadros
2015-03-19 14:09       ` Li Jun
2015-03-19 14:50         ` Roger Quadros
2015-03-20  7:18           ` Peter Chen
2015-03-20  9:46             ` Roger Quadros
2015-03-20 11:08               ` Roger Quadros
2015-03-21  1:30                 ` Peter Chen
2015-03-18 13:55 ` [RFC][PATCH 3/9] usb: otg: add OTG core Roger Quadros
2015-03-19  3:40   ` Peter Chen
2015-03-19 10:18     ` Roger Quadros
2015-03-20  7:45       ` Peter Chen
2015-03-20  9:18         ` Roger Quadros
2015-03-20  9:32           ` Peter Chen
2015-03-19  8:26   ` Li Jun
2015-03-19 10:30     ` Roger Quadros
2015-03-19 14:41       ` Li Jun
2015-03-19 14:54         ` Roger Quadros
2015-03-18 13:55 ` [RFC][PATCH 4/9] usb: otg: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros
2015-03-18 13:55 ` [RFC][PATCH 5/9] usb: hcd: adapt to OTG Roger Quadros
2015-03-18 13:56 ` [RFC][PATCH 6/9] usb: gadget: udc: " Roger Quadros
2015-03-18 13:56 ` [RFC][PATCH 7/9] usb: dwc3: adapt to OTG core Roger Quadros
2015-03-18 13:56 ` [RFC][PATCH 8/9] usb: otg-fsm: Remove unused members in struct otg_fsm Roger Quadros
2015-03-19  3:46   ` Peter Chen
2015-03-19 10:20     ` Roger Quadros
2015-03-18 13:56 ` [RFC][PATCH 9/9] usb: otg-fsm: Add documentation for " Roger Quadros
2015-03-18 17:37 ` [RFC][PATCH 0/9] USB: OTG Core functionality Tony Lindgren
2015-03-19 10:31   ` Roger Quadros

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).