On 20/06/16 15:03, Felipe Balbi wrote:

Hi,

Roger Quadros [off-list ref] writes:

quoted

quoted

Roger Quadros [off-list ref] writes:

quoted

It provides APIs for the following tasks

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

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

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

quoted

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

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

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

IDLE is pretty much given, though ;-)

quoted

quoted

quoted

DRD mode is a reduced functionality OTG mode. In this mode
we don't support SRP, HNP and dynamic role-swap.

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

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

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

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

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8689dcb..ed596ec 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig

@@ -32,6 +32,23 @@ if USB_SUPPORT
 config USB_COMMON
 	tristate
 
+config USB_OTG_CORE
+	tristate

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

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

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

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

Not always. See how USB_COMMON works.

quoted

quoted

quoted

diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
new file mode 100644
index 0000000..a23ab1e
--- /dev/null
+++ b/drivers/usb/common/usb-otg.c

@@ -0,0 +1,877 @@
+/**
+ * drivers/usb/common/usb-otg.c - USB OTG core
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/usb/of.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/gadget.h>
+#include <linux/workqueue.h>
+
+/* OTG device list */
+LIST_HEAD(otg_list);

not static? Who needs to touch this private list?

right, it should be static.

quoted

quoted

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

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

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

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

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

quoted

quoted

quoted

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

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

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

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

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

So you suggest we allow a NULL pointer dereference here?

quoted

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

you don't neet that.

quoted

quoted

quoted

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

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

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

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

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

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

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

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

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

quoted

quoted

quoted

+EXPORT_SYMBOL_GPL(drd_statemachine);

why is this exported at all? It only runs from the work_struct

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

quoted

below. BTW, that work_struct looks unnecessary.

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

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

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

quoted

quoted

quoted

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

why do you need to cope with custom work handlers?

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

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

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

quoted

quoted

quoted

+static void usb_otg_start_fsm(struct usb_otg *otg)
+{
+	struct otg_fsm *fsm = &otg->fsm;
+
+	if (fsm->running)
+		goto kick_fsm;
+
+	if (!otg->host) {
+		dev_info(otg->dev, "otg: can't start till host registers\n");
+		return;
+	}
+
+	if (!otg->gadget) {
+		dev_info(otg->dev,
+			 "otg: can't start till gadget UDC registers\n");
+		return;
+	}

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

That message in the test was misleading. It could also be a
used as a warning if users did something wrong.

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

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

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

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

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

quoted

quoted

quoted

+MODULE_LICENSE("GPL");

GPL or GPL 2-only?

GPL v2.

quoted

quoted

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index f4fc0aa..1d74fb8 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h

@@ -328,6 +328,7 @@ struct usb_gadget_ops {
  * @in_epnum: last used in ep number
  * @mA: last set mA value
  * @otg_caps: OTG capabilities of this gadget.
+ * @otg_dev: OTG controller device, if needs to be used with OTG core.

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

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

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

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

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

Good to know.

cheers,
-roger