Message-ID: <5767C1B9.2060805@ti.com>
Date:	Mon, 20 Jun 2016 13:13:13 +0300
From:	Roger Quadros <rogerq@...com>
To:	Felipe Balbi <balbi@...nel.org>, <peter.chen@...escale.com>,
	<yoshihiro.shimoda.uh@...esas.com>
CC:	<tony@...mide.com>, <gregkh@...uxfoundation.org>,
	<dan.j.williams@...el.com>, <mathias.nyman@...ux.intel.com>,
	<Joao.Pinto@...opsys.com>, <sergei.shtylyov@...entembedded.com>,
	<jun.li@...escale.com>, <grygorii.strashko@...com>,
	<robh@...nel.org>, <nsekhar@...com>, <b-liu@...com>,
	<joe@...ches.com>, <linux-usb@...r.kernel.org>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<devicetree@...r.kernel.org>
Subject: Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

Hi,

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

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

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

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

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

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

right, it should be static.

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

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

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

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

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

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

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

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

I'll fix up the message to

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

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

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

> below. BTW, that work_struct looks unnecessary.

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

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

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

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

Maybe Peter can chime in.

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

I agree.

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

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

GPL v2.

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

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

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

Yoshihiro,

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

cheers,
-roger



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists