Re: [PATCH v3 07/11] usb: otg: add OTG core

[Li Jun <b47624@freescale.com>]

Hi,

[...]

> >> +	otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
> > 
> > 2 timers are missing: B_DATA_PLS, B_SSEND_SRP.
> 
> Those 2 are not used by usb-otg-fsm.c. We can add it when usb-otg-fsm.c is updated.
> 

ok.

> > 
> >> +}

[...]

> >> +
> >> +/**
> >> + * 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);
> >> +	struct otg_hcd_ops *hcd_ops;
> >> +
> >> +	dev_dbg(otgd->dev, "otg: %s %d\n", __func__, on);
> >> +	if (!fsm->otg->host) {
> >> +		WARN_ONCE(1, "otg: fsm running without host\n");
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (on) {
> >> +		/* OTG device operations */
> >> +		if (otgd->start_host)
> >> +			otgd->start_host(fsm, on);
> >> +
> >> +		/* start host */
> >> +		hcd_ops = otgd->primary_hcd.ops;
> >> +		hcd_ops->add(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum,
> >> +			     otgd->primary_hcd.irqflags);
> >> +		if (otgd->shared_hcd.hcd) {
> >> +			hcd_ops = otgd->shared_hcd.ops;
> >> +			hcd_ops->add(otgd->shared_hcd.hcd,
> >> +				     otgd->shared_hcd.irqnum,
> >> +				     otgd->shared_hcd.irqflags);
> >> +		}
> >> +	} else {
> >> +		/* stop host */
> >> +		if (otgd->shared_hcd.hcd) {
> >> +			hcd_ops = otgd->shared_hcd.ops;
> >> +			hcd_ops->remove(otgd->shared_hcd.hcd);
> >> +		}
> >> +		hcd_ops = otgd->primary_hcd.ops;
> >> +		hcd_ops->remove(otgd->primary_hcd.hcd);
> >> +
> >> +		/* OTG device operations */
> >> +		if (otgd->start_host)
> >> +			otgd->start_host(fsm, on);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> > I do not see much benefit by this override function, usb_add/remove_hcd
> > can be simply included by controller's start_host function, also there
> > maybe some additional operations after usb_add_hcd, but this override
> > function limit usb_add_hcd() is the last step.
> 
> I had tried host start/stop way before but Alan's suggestion was to use
> bind/unbind the host controller completely as that is much simpler
> 
> [1] http://article.gmane.org/gmane.linux.usb.general/123842
> 

I did not mean host start/stop in your first version, I agree using
usb_add/remove_hcd() for simple.

> > 
> > Maybe your intention is to make usb_add_hcd is the only operation required
> > to start host, so ideally controller driver need not define its start_host
> > routine for this otg ops, I am not sure if this can work for different otg
> 
> Yes that was the intention.
> 
> > platforms. If the shared code is only usb_add/remove_hcd(), maybe leave this
> > ops defined by controller driver can make core code simple and give flexibility
> > to controller drivers.
> 
> We don't completely override start/stop_host(). The flexibility is still there.
> We call controllers start_host(1) before starting the controller and controllers
> start_host(0) after stopping the controller.
> So the the controller can still do what they want in otg_fsm_ops.start_host/gadget().
> 

But if controller driver wants to do something after usb_otg_add_hcd(),
it's impossible with your current usb_otg_start_host().

> The OTG core only takes care of actually starting/stopping the host controller.
> 
> If we don't do that then the code in usb_otg_start_host() has to be pasted
> in every OTG controller driver. This is code duplication.
> 

Actually the only duplication code may be a function call to original
usb_add/remove_hcd().

> > 
> >> +
> >> +/**
> >> + * 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);
> >> +	struct usb_gadget *gadget = fsm->otg->gadget;
> >> +
> >> +	dev_dbg(otgd->dev, "otg: %s %d\n", __func__, on);
> >> +	if (!gadget) {
> >> +		WARN_ONCE(1, "otg: fsm running without gadget\n");
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (on) {
> >> +		/* OTG device operations */
> >> +		if (otgd->start_gadget)
> >> +			otgd->start_gadget(fsm, on);
> >> +
> >> +		otgd->gadget_ops->start(fsm->otg->gadget);
> >> +	} else {
> >> +		otgd->gadget_ops->stop(fsm->otg->gadget);
> >> +
> >> +		/* OTG device operations */
> >> +		if (otgd->start_gadget)
> >> +			otgd->start_gadget(fsm, on);
> >> +	}
> >> +
> >> +	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);
> >> +
> >> +	otg_statemachine(&otgd->fsm);
> > 
> > Need consider runtime pm, or you want to rely on controller driver take
> > care of it?
> 
> For simplicity let's say that controller driver takes care of it.
> 

Then controller driver need add runtime pm for every otg fsm ops.

Code like below can make it simple:
runtime_pm_get_sync(otgd->dev);
otg_statemachine(&otgd->fsm);
runtime_pm_get_put(otgd->dev);

There is another problem, otg work will only do one state transition, but
in some cases we may need successive state transitions.  

> > 
> >> +}
> >> +
> >> +/**
> >> + * 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.
> >> + *

[...]

> >> +/**
> >> + * 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, "otg: can't start till host registers\n");
> >> +		return;
> >> +	}
> >> +
> > 
> > This cannot work, fsm->otg->host is set in usb_otg_register_hcd(), which is
> > called by usb_add_hcd(), but usb_add_hcd() should be called only if otg fsm
> > already started to some A-device state, deadlock.
> 
> I've changed usb_add_hcd() behaviour. Now usb_otg_add_hcd() does the real work of adding
> the hcd. usb_add_hcd() prevents the add if it is an otg hcd and just registers
> with OTG core.
> 

So you expect the controller driver still call usb_add_hcd() before otg fsm
start, in which it only registers the created hcd with OTG core.

> > 
> >> +	if (!fsm->otg->gadget) {
> >> +		dev_info(otgd->dev, "otg: can't start till gadget registers\n");
> >> +		return;
> >> +	}
> >> +
> >> +	otgd->fsm_running = true;
> >> +kick_fsm:
> >> +	queue_work(otgd->wq, &otgd->work);
> >> +}
> >> +

[...]

> >> +
> >> +/**
> >> + * usb_otg_register_hcd - Register Host controller to OTG core
> >> + * @hcd:	Host controller device
> >> + * @irqnum:	interrupt number
> >> + * @irqflags:	interrupt flags
> >> + * @ops:	HCD ops to add/remove 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 upto the OTG state machine to do so.
> >> + *
> >> + * 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 otg_data *otgd;
> >> +	struct device *otg_dev = hcd->self.controller->parent;
> >> +

I see normally we directly use controller dev for hcd->self.controller,
usb_create_hcd(... struct device *dev, ...)
{
	... ...
	hcd->self.controller = dev;
	... ...
}

For register gadget, it's okay since:
int usb_add_gadget_udc_release(struct device *parent, ...)
{
	... ...
	gadget->dev.parent = parent;
	... ...
}

So we need parent dev for usb_otg_register(struct device *dev,...), and child dev
for usb_create_hcd(struct device *dev,...)? dwc3 is designed like this?

> >> +	mutex_lock(&otg_list_mutex);
> >> +	otgd = usb_otg_device_get_otgd(otg_dev);
> >> +	if (!otgd) {
> >> +		dev_dbg(otg_dev, "otg: %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->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);
> >> +		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
> >> +
> >> +		/* start FSM */
> >> +		usb_otg_start_fsm(&otgd->fsm);
> > 
> > usb_otg_register_hcd() is called before usb_otg_add_hcd(), start fsm on
> > this point can make sense since hcd has not been added?
> 
> for OTG/DRD HCD case:
> - usb_add_hcd() does not really ADD (or START) the HCD. It just registers with OTG core.
> - FSM takes care of ADDing (or STARTing) the HCD when it wants using the
> usb_otg_add_hcd() call.

Understood.

> - FSM does not need HCD to be already added. It just needs it to be registered.

My point is only registering hcd to OTG core cannot be a valid *input* to make
otg fsm state can be changed, so it's making no sense to call usb_otg_start_fsm(),
but it's no harm.

> It takes care of strting it when it wants to.
> 

Any otg fsm state change(or start it to make its state change) need some otg fsm
input or variables change happen.

> > 
> >> +	} 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;
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_otg_register_hcd);

[...]

> >> +#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)
> >> +
> > 
> > All otg timer timeout value should be in some *range* defined by otg spec,
> > not some specific value, I don't think one specific value can meet all otg
> > platforms, so we need find a way to make those value can be configured by
> > controller drivers.
> 
> OK. How about introducing 'struct usb_otg_config' which must be passed
> to usb_otg_register().
>

I think it's okay.
 
> /* otg controller configuration */
> struct usb_otg_config {
> 	/* OTG caps */
> 	struct usb_otg_caps otg_caps;

You can use a pointer to avoid data copy.

> 
> 	/* OTG Timer timeouts in ms. If 0, sane default will be used */
> 	int ta_wait_vrise;
> 	...
> };
> 

> > 
> >> 2.1.4
> >>
> > 
> > I assume I will have more comments after try more.
> 
> Sure, no worries :)
> 
> cheers,
> -roger