RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

From: Jun Li
Date: Mon Apr 25 2016 - 22:08:08 EST


Hi Roger

> -----Original Message-----
> From: Roger Quadros [mailto:rogerq@xxxxxx]
> Sent: Tuesday, April 05, 2016 10:05 PM
> To: stern@xxxxxxxxxxxxxxxxxxx; balbi@xxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; peter.chen@xxxxxxxxxxxxx
> Cc: dan.j.williams@xxxxxxxxx; jun.li@xxxxxxxxxxxxx;
> mathias.nyman@xxxxxxxxxxxxxxx; tony@xxxxxxxxxxx; Joao.Pinto@xxxxxxxxxxxx;
> abrestic@xxxxxxxxxxxx; r.baldyga@xxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx; Roger Quadros
> <rogerq@xxxxxx>
> Subject: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>
> It provides APIs for the following tasks
>
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
>
> Provide a dual-role device (DRD) state machine.
> DRD mode is a reduced functionality OTG mode. In this mode we don't
> support SRP, HNP and dynamic role-swap.
>
> In DRD operation, the controller mode (Host or Peripheral) is decided
> based on the ID pin status. Once a cable plug (Type-A or Type-B) is
> attached the controller selects the state and doesn't change till the
> cable in unplugged and a different cable type is inserted.
>
> As we don't need most of the complex OTG states and OTG timers we
> implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs 'id' and
> 'b_sess_vld'.
>
> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> ---

...

> +/**
> + * Register pending host/gadget and remove entry from wait list */
> +static void usb_otg_flush_wait(struct device *otg_dev) {
> + struct otg_wait_data *wait;
> + struct otg_hcd *host;
> + struct otg_gcd *gadget;
> +
> + mutex_lock(&wait_list_mutex);
> +
> + wait = usb_otg_get_wait(otg_dev);
> + if (!wait)
> + goto done;
> +
> + dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
> + gadget = &wait->gcd;
> + if (gadget)

If (gadget->gadget)

> + usb_otg_register_gadget(gadget->gadget, gadget->ops);
> +
> + host = &wait->primary_hcd;
> + if (host->hcd)
> + usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
> + host->ops);
> +
> + host = &wait->shared_hcd;
> + if (host->hcd)
> + usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
> + host->ops);
> +
> + list_del(&wait->list);
> + kfree(wait);
> +
> +done:
> + mutex_unlock(&wait_list_mutex);
> +}
> +
> +/**
> + * Check if the OTG device is in our OTG list and return
> + * usb_otg data, else NULL.
> + *
> + * otg_list_mutex must be held.
> + */
> +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;
> +}

Could you export it to be a public API, we may need access usb_otg
in common host driver for handling of enumeration of otg test device.

...

> +/**
> + * Called when entering a DRD state.
> + * 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;
> + case OTG_STATE_UNDEFINED:
> + case OTG_STATE_B_SRP_INIT:
> + case OTG_STATE_B_WAIT_ACON:
> + case OTG_STATE_B_HOST:
> + case OTG_STATE_A_IDLE:
> + case OTG_STATE_A_WAIT_VRISE:
> + case OTG_STATE_A_WAIT_BCON:
> + case OTG_STATE_A_SUSPEND:
> + case OTG_STATE_A_PERIPHERAL:
> + case OTG_STATE_A_WAIT_VFALL:
> + case OTG_STATE_A_VBUS_ERR:

Remove above unused states.

> + default:
> + dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
> + __func__, usb_otg_state_string(new_state));
> + break;
> + }
> +
> + otg->state = new_state;
> +}
> +
> +/**
> + * DRD state change judgement
> + *
> + * 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
> + */
> +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;
> +
> + /* invalid states for DRD */
> + case OTG_STATE_B_SRP_INIT:
> + case OTG_STATE_B_WAIT_ACON:
> + case OTG_STATE_B_HOST:
> + case OTG_STATE_A_IDLE:
> + case OTG_STATE_A_WAIT_VRISE:
> + case OTG_STATE_A_WAIT_BCON:
> + case OTG_STATE_A_SUSPEND:
> + case OTG_STATE_A_PERIPHERAL:
> + case OTG_STATE_A_WAIT_VFALL:
> + case OTG_STATE_A_VBUS_ERR:

Remove above unused states and add a default:

> + dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
> + __func__, usb_otg_state_string(state));
> + drd_set_state(fsm, OTG_STATE_UNDEFINED);
> + 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);
> +
> +/**
> + * OTG FSM/DRD work function

DRD work function

> + */
> +static void usb_otg_work(struct work_struct *work) {

usb_drd_work() name is better as it's only for drd.

> + struct usb_otg *otg = container_of(work, struct usb_otg, work);
> +
> + pm_runtime_get_sync(otg->dev);
> + drd_statemachine(otg);
> + pm_runtime_put_sync(otg->dev);
> +}
> +
> +/**
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> + struct usb_otg_config *config)
> +{
> + struct usb_otg *otg;
> + struct otg_wait_data *wait;
> + 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 = config->otg_caps;
> +
> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> + otg->caps->adp_support) && !config->otg_work)
> + dev_info(dev, "otg: limiting to dual-role\n");

dev_err, this should be an error.

> +
> + if (config->otg_work) /* custom otg_work ? */
> + INIT_WORK(&otg->work, config->otg_work);
> + else
> + INIT_WORK(&otg->work, usb_otg_work);
> +
> + otg->wq = create_singlethread_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);
> +
> + /* were we in wait list? */
> + mutex_lock(&wait_list_mutex);
> + wait = usb_otg_get_wait(dev);
> + mutex_unlock(&wait_list_mutex);
> + if (wait) {
> + /* register pending host/gadget and flush from list */
> + usb_otg_flush_wait(dev);
> + }
> +
> + return otg;
> +
> +err_wq:
> + kfree(otg);
> +unlock:
> + mutex_unlock(&otg_list_mutex);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register);
> +