Date:	Wed, 9 Sep 2015 13:01:14 +0300
From:	Roger Quadros <rogerq@...com>
To:	Li Jun <b47624@...escale.com>
CC:	<stern@...land.harvard.edu>, <balbi@...com>,
	<gregkh@...uxfoundation.org>, <peter.chen@...escale.com>,
	<dan.j.williams@...el.com>, <jun.li@...escale.com>,
	<mathias.nyman@...ux.intel.com>, <tony@...mide.com>,
	<Joao.Pinto@...opsys.com>, <abrestic@...omium.org>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-omap@...r.kernel.org>
Subject: Re: [PATCH v4 07/13] usb: otg: add OTG core

On 09/09/15 09:20, Li Jun wrote:
> On Mon, Sep 07, 2015 at 01:53:19PM +0300, Roger Quadros wrote:
>> On 07/09/15 10:40, Li Jun wrote:
>>> On Mon, Aug 24, 2015 at 04:21:18PM +0300, 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
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@...com>
>>>> ---
>>>>  MAINTAINERS                  |    4 +-
>>>>  drivers/usb/Kconfig          |    2 +-
>>>>  drivers/usb/Makefile         |    1 +
>>>>  drivers/usb/common/Makefile  |    3 +-
>>>>  drivers/usb/common/usb-otg.c | 1061 ++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/usb/common/usb-otg.h |   71 +++
>>>>  drivers/usb/core/Kconfig     |   11 +-
>>>>  include/linux/usb/otg.h      |  189 +++++++-
>>>>  8 files changed, 1321 insertions(+), 21 deletions(-)
>>>>  create mode 100644 drivers/usb/common/usb-otg.c
>>>>  create mode 100644 drivers/usb/common/usb-otg.h
>>>>
>>>
>>> ... ...
>>>
>>>> +
>>>> +/**
>>>> + * Get OTG device from host or gadget device.
>>>> + *
>>>> + * For non device tree boot, the OTG controller is assumed to be
>>>> + * the parent of the host/gadget device.
>>>
>>> This assumption/restriction maybe a problem, as I pointed in your previous
>>> version, usb_create_hcd() use the passed dev as its dev, but,
>>> usb_add_gadget_udc() use the passed dev as its parent dev, so often the
>>> host and gadget don't share the same parent device, at least it doesn't
>>> apply to chipidea case.
>>
>> Let's provide a way for OTG driver to provide the OTG core exactly which is
>> the related host/gadget device.
>>
>>>
>>>> + * For device tree boot, the OTG controller is derived from the
>>>> + * "otg-controller" property.
>>>> + */
>>>> +static struct device *usb_otg_get_device(struct device *hcd_gcd_dev)
>>>> +{
>>>> +	struct device *otg_dev;
>>>> +
>>>> +	if (!hcd_gcd_dev)
>>>> +		return NULL;
>>>> +
>>>> +	if (hcd_gcd_dev->of_node) {
>>>> +		struct device_node *np;
>>>> +		struct platform_device *pdev;
>>>> +
>>>> +		np = of_parse_phandle(hcd_gcd_dev->of_node, "otg-controller",
>>>> +				      0);
>>>> +		if (!np)
>>>> +			goto legacy;	/* continue legacy way */
>>>> +
>>>> +		pdev = of_find_device_by_node(np);
>>>> +		of_node_put(np);
>>>> +		if (!pdev) {
>>>> +			dev_err(&pdev->dev, "couldn't get otg-controller device\n");
>>>> +			return NULL;
>>>> +		}
>>>> +
>>>> +		otg_dev = &pdev->dev;
>>>> +		return otg_dev;
>>>> +	}
>>>> +
>>>> +legacy:
>>>> +	/* otg device is parent and must be registered */
>>>> +	otg_dev = hcd_gcd_dev->parent;
>>>> +	if (!usb_otg_get_data(otg_dev))
>>>> +		return NULL;
>>>> +
>>>> +	return otg_dev;
>>>> +}
>>>> +
>>>
>>> ... ...
>>>
>>>> +static void usb_otg_init_timers(struct usb_otg *otgd, unsigned *timeouts)
>>>> +{
>>>> +	struct otg_fsm *fsm = &otgd->fsm;
>>>> +	unsigned long tmouts[NUM_OTG_FSM_TIMERS];
>>>> +	int i;
>>>> +
>>>> +	/* set default timeouts */
>>>> +	tmouts[A_WAIT_VRISE] = TA_WAIT_VRISE;
>>>> +	tmouts[A_WAIT_VFALL] = TA_WAIT_VFALL;
>>>> +	tmouts[A_WAIT_BCON] = TA_WAIT_BCON;
>>>> +	tmouts[A_AIDL_BDIS] = TA_AIDL_BDIS;
>>>> +	tmouts[A_BIDL_ADIS] = TA_BIDL_ADIS;
>>>> +	tmouts[B_ASE0_BRST] = TB_ASE0_BRST;
>>>> +	tmouts[B_SE0_SRP] = TB_SE0_SRP;
>>>> +	tmouts[B_SRP_FAIL] = TB_SRP_FAIL;
>>>> +
>>>> +	/* set controller provided timeouts */
>>>> +	if (timeouts) {
>>>> +		for (i = 0; i < NUM_OTG_FSM_TIMERS; i++) {
>>>> +			if (timeouts[i])
>>>> +				tmouts[i] = timeouts[i];
>>>> +		}
>>>> +	}
>>>> +
>>>> +	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);
>>>> +
>>>> +	/* FIXME: what about A_WAIT_ENUM? */
>>>
>>> Either you init it as other timers, or you remove all of it, otherwise
>>> there will be NULL pointer crash.
>>
>> I want to initialize it but was not sure about the timeout value.
>> What timeout value I must use?
>>
> 
> It's not defined in OTG spec, I don't know either.
> or you filter it out when add/del timer in below 2 functions.
> 	if (id == A_WAIT_ENUM)
> 		return;
> 

Looks like it is used for some workaround. See phy-fsl-usb.c line 269

/*
 * Workaround for a_host suspending too fast.  When a_bus_req=0,
 * a_host will start by SRP.  It needs to set b_hnp_enable before
 * actually suspending to start HNP
 */
void a_wait_enum(unsigned long foo)
{
        VDBG("a_wait_enum timeout\n");
        if (!fsl_otg_dev->phy.otg->host->b_hnp_enable)
                fsl_otg_add_timer(&fsl_otg_dev->fsm, a_wait_enum_tmr);
        else
                otg_statemachine(&fsl_otg_dev->fsm);
}

I'll filter it out for now as per your suggestion. We can look back
into it if we face issues as mentioned in the workaround.

>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * OTG FSM ops function to add timer
>>>> + */
>>>> +static void usb_otg_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
>>>> +{
>>>> +	struct usb_otg *otgd = container_of(fsm, struct usb_otg, fsm);
>>>> +	struct otg_timer *otgtimer = &otgd->timers[id];
>>>> +	struct hrtimer *timer = &otgtimer->timer;
>>>> +
>>>> +	if (!otgd->fsm_running)
>>>> +		return;
>>>> +
>>>> +	/* if timer is already active, exit */
>>>> +	if (hrtimer_active(timer)) {
>>>> +		dev_err(otgd->dev, "otg: timer %d is already running\n", id);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	hrtimer_start(timer, otgtimer->timeout, HRTIMER_MODE_REL);
>>>> +}
>>>> +
>>>> +/**
>>>> + * OTG FSM ops function to delete timer
>>>> + */
>>>> +static void usb_otg_del_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
>>>> +{
>>>> +	struct usb_otg *otgd = container_of(fsm, struct usb_otg, fsm);
>>>> +	struct hrtimer *timer = &otgd->timers[id].timer;
>>>> +
>>>> +	hrtimer_cancel(timer);
>>>> +}
>>>> +
> 
> ... ...
> 
>>>> +	}
>>>> +
>>>> +	otgd->dev = dev;
>>>> +	otgd->caps = &config->otg_caps;
>>>
>>> How about define otgd->caps as a pointer, then don't need copy it.
>>
>> otgd->caps is a pointer.
>>
> 
> okay, you are right.
> 
>>>
>>>> +	INIT_WORK(&otgd->work, usb_otg_work);
>>>> +	otgd->wq = create_singlethread_workqueue("usb_otg");
>>>> +	if (!otgd->wq) {
>>>> +		dev_err(dev, "otg: %s: can't create workqueue\n",
>>>> +			__func__);
>>>> +		ret = -ENOMEM;
>>>> +		goto err_wq;
>>>> +	}
>>>> +
>>>> +	usb_otg_init_timers(otgd, config->otg_timeouts);
>>>> +
>>>> +	/* create copy of original ops */
>>>> +	otgd->fsm_ops = *config->fsm_ops;
>>>
>>> The same, use a pointer is enough?
>>
>> We are creating a copy because we are overriding timer ops.
>>
> 
> okay.
> 
>>>
>>>> +	/* 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;
>>>> + *
> 
> ... ...
> 
>>>> + * 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.
>>>
>>> I am confused on how to use this function.
>>> - This function should be called before start fsm per usb_otg_start_fsm().
>>
>> yes.
>>
>>> - Called by usb_add_hcd(), so we need call usb_add_hcd() before start fsm.
>>
>> yes.
>>
>>> - If I want to add hcd when switch to host role, and remove hcd when switch
>>>   to peripheral, with this design, I cannot use this function?
>>
>> You add hcd only once during the life of the OTG device. If it is linked to the
>> OTG controller the OTG fsm manages the start/stop of hcd using the otg_hcd_ops.
>>
>> "usb/core/hcd.c"
>> static struct otg_hcd_ops otg_hcd_intf = {
>>         .add = usb_otg_add_hcd,
>>         .remove = usb_otg_remove_hcd,
>> };
>>
>> Your otg driver must use teh usb_otg_add/remove_hcd to start/stop the controller.
>> Using usb_remove_hcd() means the hcd resource is no longer available and the
>> otg fsm will be stopped.
>>
>>> - How about split it out of usb_add_hcd()?
>>
>> Adding the HCD and starting/stopping the hcd is split into
>> usb_add/remove_hcd() and usb_otg_add/remove_hcd() for OTG case.
>>
> 
> Catch your point now.
> Do usb_add_hcd to register hcd before start fsm, and use usb_otg_start_host()
> to start/stop host role for otg fsm.
> 
correct :).

>>>
>>>> + *
>>>> + * Returns: 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 *otgd;
>>>> +	struct device *hcd_dev = hcd->self.controller;
>>>> +	struct device *otg_dev = usb_otg_get_device(hcd_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);
>>>> +	otgd = usb_otg_get_data(otg_dev);
>>>> +	mutex_unlock(&otg_list_mutex);
>>>> +	if (!otgd) {
>>>> +		dev_dbg(hcd_dev,
>>>> +			"otg: controller not yet registered. waiting..\n");
>>>> +		/*
>>>> +		 * otg controller might register later. Put the hcd in
>>>> +		 * wait list and call us back when ready
>>>> +		 */
>>>> +		if (usb_otg_hcd_wait_add(otg_dev, hcd, irqnum, irqflags, ops)) {
>>>> +			dev_dbg(hcd_dev, "otg: failed to add to wait list\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	/* HCD will be started by OTG fsm when needed */
>>>> +	mutex_lock(&otgd->fsm.lock);
>>>
>>> If call usb_add_hcd() when start host role, deadlock.
>>
>> No. You must call usb_otg_add_hcd() to start host role.
>>
>>>
>>>> +	if (otgd->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 == otgd->primary_hcd.hcd) {
>>>> +			if (otgd->shared_hcd.hcd) {
>>>> +				dev_err(otg_dev, "otg: shared host already registered\n");
>>>> +				goto err;
>>>> +			}
>>>> +
>>>> +			otgd->shared_hcd.hcd = hcd;
>>>> +			otgd->shared_hcd.irqnum = irqnum;
>>>> +			otgd->shared_hcd.irqflags = irqflags;
>>>> +			otgd->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;
>>>> +		}
>>>> +
>>>> +		otgd->primary_hcd.hcd = hcd;
>>>> +		otgd->primary_hcd.irqnum = irqnum;
>>>> +		otgd->primary_hcd.irqflags = irqflags;
>>>> +		otgd->primary_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 (otgd->shared_hcd.hcd || !otgd->primary_hcd.hcd->shared_hcd) {
>>>> +		otgd->fsm.otg->host = hcd_to_bus(hcd);
>>>
>>> otgd->host = hcd_to_bus(hcd);
>>
>> ok. So we set host at both places. struct usb_otg in struct otg_fsm starts to
>> feel redundant now. I think we should get rid of it and get the usb_otg struct
>> using container_of() instead.
>>
> 
> Then you may come to force existing otg fsm code to use your OTG core.

Let's leave it around then.

> 
>>>
>>>> +		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
>>>> +
>>>> +		/* start FSM */
>>>> +		usb_otg_start_fsm(&otgd->fsm);
>>>> +	} else {
>>>> +		dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
>>>> +	}
>>>> +
>>>> +	mutex_unlock(&otgd->fsm.lock);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err:
>>>> +	mutex_unlock(&otgd->fsm.lock);
>>>> +	return -EINVAL;
>>>
>>> Return non-zero, then if err, do we need call usb_otg_add_hcd() after
>>> usb_otg_register_hcd() fails?
>>
>> You should not call usb_otg_register_hcd() but usb_add_hcd().
>> If that fails then you fail as ususal.
> 
> My point is if we use usb_add_hcd(), but failed in usb_otg_register_hcd(),
> then usb_otg_add_hcd() will be called in *all* error case, is this your
> expectation?
> 	if (usb_otg_register_hcd(hcd, irqnum, irqflags, &otg_hcd_intf))
> 		return usb_otg_add_hcd(hcd, irqnum, irqflags);
> 

Yes, my intention was that if otg fails then it is a non otg HCD so register normally.
Let me correct my previous statement. If you are absolutely sure
that the HCD is for otg/dual-role usage then you should call usb_otg_register_hcd().

>>>> + * @fsm_running: state machine running/stopped indicator
>>>> + */
>>>>  struct usb_otg {
>>>>  	u8			default_a;
>>>>  
>>>>  	struct phy		*phy;
>>>>  	/* old usb_phy interface */
>>>>  	struct usb_phy		*usb_phy;
>>>> +
>>>
>>> add a blank line?
>>>
> 
> You missed this.

Sorry. Did you suggest to remove that blank line
or add a new one before usb_phy?

> 
>>>>  	struct usb_bus		*host;
>>>>  	struct usb_gadget	*gadget;
>>>>  
>>>>  	enum usb_otg_state	state;
>>>>  
>>>> +	struct device *dev;
>>>> +	struct usb_otg_caps *caps;
>>>> +	struct otg_fsm fsm;
>>>> +	struct otg_fsm_ops fsm_ops;
>>>> +
>>>> +	/* internal use only */
>>>> +	struct otg_hcd primary_hcd;
>>>> +	struct otg_hcd shared_hcd;
>>>> +	struct otg_gadget_ops *gadget_ops;
>>>> +	struct otg_timer timers[NUM_OTG_FSM_TIMERS];
>>>> +	struct list_head list;
>>>> +	struct work_struct work;
>>>> -- 
>>>> 2.1.4
> 

--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists