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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
