> -----Original Message-----
> From: [email protected] [mailto:linux-usb-
> [email protected]] On Behalf Of Ido Shayevitz
> Sent: Wednesday, August 01, 2012 11:44 PM
> To: Anton Tikhomirov
> Cc: 'Ido Shayevitz'; 'Felipe Balbi'; [email protected]; linux-
> [email protected]
> Subject: RE: [RFC/PATCH v3] usb: dwc3: Introduce OTG driver for dwc3
>
>
> Hi Anton,
>
> On Tue, July 31, 2012 11:27 pm, Anton Tikhomirov wrote:
> > Hi Ido,
> >
> >> -----Original Message-----
> >> From: [email protected] [mailto:linux-usb-
> >> [email protected]] On Behalf Of Ido Shayevitz
> >> Sent: Monday, July 30, 2012 10:16 PM
> >> To: Anton Tikhomirov
> >> Cc: 'Ido Shayevitz'; 'Felipe Balbi'; [email protected]
> >> Subject: RE: [RFC/PATCH v3] usb: dwc3: Introduce OTG driver for dwc3
> >>
> >> Hi Anton,
> >>
> >> On Mon, July 30, 2012 5:25 am, Anton Tikhomirov wrote:
> >> > Hi Ido,
> >> >
> >> >> >
> >> >> > You activate sm only if gadget and host are ready. At the same
> >> time,
> >> >> > in dwc3_otg_interrupt() you schedule work if interrupt happens. In
> >> >> > situation
> >> >> > when host is not set yet, but cable is plugged, we will have
> >> unwanted
> >> >> sm
> >> >> > activation
> >> >> > (in interrupt handler) and, as a consequence, repeating error
> >> "unable
> >> >> to
> >> >> > start A-device\n"
> >> >> > in dwc3_otg_sm_work().
> >> >>
> >> >> Host and gadget should set themselves to the otg on drivers probe,
> in
> >> >> boot
> >> >> time. So cable connect happens later.
> >> >> If the scenario you describe does happen it means that the xHCI
> >> driver
> >> >> was
> >> >> not loaded into the kernel, but cable with micro-A was plugged into
> >> your
> >> >> device, so need add host support to the menuconfig.
> >> >> It should be enough to select in the menuconfig CONFIG_USB and
> >> >> CONFIG_USB_XHCI_HCD.
> >> >
> >> > Agree. But what if my controller supports DRD mode, but I _want_ to
> >> keep
> >> > this
> >> > option (XHCI support) off, for any reason. By the way, it seems we
> >> will
> >> > have
> >> > similar effect when micro-A cable is plugged after
> > otg_set_host(phy->otg,
> >> > NULL)
> >> > call. Of course such situations are rare.
> >> >
> >>
> >> OK, so I will avoid the re-schedule of the sm_work after this error
> will
> >> be printed (so will be printed once...)
> >> Thanks...
> >
> > One more thing. Currently the work is first time scheduled in two cases:
> > when
> > interrupt happens or both, peripheral and host, were set. If host is not
> > set
> > (and probably won't be) the work will _not_ be scheduled (and peripheral
> > will
> > not start) till we connect and then disconnect micro-A cable. In other
> > words:
> > we should be able to use peripheral even if host is not supported (and
> > vice
> > versa?). What's your opinion?
>
> The idea behind schedule the work after both are set, is that we don't
> want a case that we scheduled the work after only the gadget was set but
> the ID pin is zero. In this case we will get wasted run of work state
> machine.
How about to stop the state machine after the first error "unable to start
A-device" (in OTG_STATE_A_IDLE). It will be restarted (in irq handler) when
user unplugs the micro-A cable.
> If host is not set, we will start the peripheral on cable connect, as you
What cable type do you mean?
If type is micro-A, then we will have otg event (id 1->0) and we can start
peripheral as soon as user unplugs the cable (id 0->1). In this case, it's
a little bit strange, that user needs to plug micro-A cable to start
peripheral (read machine).
If type is micro-B, then how can we catch this event? (Do you remember:
host is not set, preventing us to start machine, and no interrupt since
there
is no id transition). Now dwc3_gadget_run_stop() is called from
dwc3_gadget_pullup() only if dwc->vbus_active is 1, and from
dwc3_gadget_vbus_session() which won't be called (and consequently
vbus_active
won't be set) since machine is not running.
> said, and if ID pin is 1. When we start the peripheral we actually just
> turn on the run stop bit, while the dwc3 gadget driver was already up and
> allocated its needed memory, so there shouldn't be problem to wait until
> cable connect...
>
> >>
> >> >> But if your controller DWC_MODE (see core.c) does not support DRD,
> or
> >> >> the
> >> >> cable plugged in is micro-B then this error should not be printed
> >> >> eventhough the host was not set.
> >> >>
> >> >> >> >> + } else {
> >> >> >> >> + if (otg->phy->state == OTG_STATE_A_HOST) {
> >> >> >> >> + dwc3_otg_start_host(otg, 0);
> >> >> >> >> + otg->host = NULL;
> >> >> >> >> + otg->phy->state = OTG_STATE_UNDEFINED;
> >> >> >> >> + schedule_work(&dotg->sm_work);
> >> >> >> >> + } else {
> >> >> >> >> + otg->host = NULL;
> >> >> >> >> + }
> >> >> >> >> + }
> >> >> >> >> +
> >> >> >> >> + return 0;
> >> >> >> >> +}
> >> >> >> >> +
> >> >> >> >> +/**
> >> >> >> >> + * dwc3_otg_start_peripheral - bind/unbind the peripheral
> >> >> >> controller.
> >> >> >> >> + *
> >> >> >> >> + * @otg: Pointer to the otg_transceiver structure.
> >> >> >> >> + * @gadget: pointer to the usb_gadget structure.
> >> >> >> >
> >> >> >> > This comment doesn't match the function definition (@on, not
> >> > @gadget).
> >> >> >> >
> >> >> >> >> + *
> >> >> >> >> + * Returns 0 on success otherwise negative errno.
> >> >> >> >> + */
> >> >> >> >> +static int dwc3_otg_start_peripheral(struct usb_otg *otg, int
> > on)
> >> >> >> >> +{
> >> >> >> >> + struct dwc3_otg *dotg = container_of(otg, struct dwc3_otg,
> >> >> otg);
> >> >> >> >> +
> >> >> >> >> + if (!otg->gadget)
> >> >> >> >> + return -EINVAL;
> >> >> >> >> +
> >> >> >> >> + if (on) {
> >> >> >> >> + dev_dbg(otg->phy->dev, "%s: turn on gadget %s\n",
> >> >> >> >> + __func__,
> >> > otg->gadget->name);
> >> >> >> >> + dwc3_otg_set_peripheral_regs(dotg);
> >> >> >> >> + usb_gadget_vbus_connect(otg->gadget);
> >> >> >> >> + } else {
> >> >> >> >> + dev_dbg(otg->phy->dev, "%s: turn off gadget %s\n",
> >> >> >> >> + __func__,
> >> > otg->gadget->name);
> >> >> >> >> + usb_gadget_vbus_disconnect(otg->gadget);
> >> >> >> >> + }
> >> >> >> >> +
> >> >> >> >> + return 0;
> >> >> >> >> +}
> >> >> >> >> +
> >> >> >> >> +/**
> >> >> >> >> + * dwc3_otg_set_peripheral - bind/unbind the peripheral
> >> >> controller
> >> >> >> >> driver.
> >> >> >> >> + *
> >> >> >> >> + * @otg: Pointer to the otg_transceiver structure.
> >> >> >> >> + * @gadget: pointer to the usb_gadget structure.
> >> >> >> >> + *
> >> >> >> >> + * Returns 0 on success otherwise negative errno.
> >> >> >> >> + */
> >> >> >> >> +static int dwc3_otg_set_peripheral(struct usb_otg *otg,
> >> >> >> >> + struct usb_gadget *gadget)
> >> >> >> >> +{
> >> >> >> >> + struct dwc3_otg *dotg = container_of(otg, struct dwc3_otg,
> >> >> otg);
> >> >> >> >> +
> >> >> >> >> + if (gadget) {
> >> >> >> >> + dev_dbg(otg->phy->dev, "%s: set gadget %s\n",
> >> >> >> >> + __func__, gadget->name);
> >> >> >> >> + otg->gadget = gadget;
> >> >> >> >> +
> >> >> >> >> + /*
> >> >> >> >> + * Only after both peripheral and host are set then
> >> >> check
> >> >> >> >> + * OTG sm. This prevents unnecessary activation of
> >> > the
> >> >> sm
> >> >> >> >> + * in case the ID is grounded.
> >> >> >> >> + */
> >> >> >> >> + if (otg->host)
> >> >> >> >> + schedule_work(&dotg->sm_work);
> >> >> >> >> + } else {
> >> >> >> >> + if (otg->phy->state == OTG_STATE_B_PERIPHERAL) {
> >> >> >> >> + dwc3_otg_start_peripheral(otg, 0);
> >> >> >> >> + otg->gadget = NULL;
> >> >> >> >> + otg->phy->state = OTG_STATE_UNDEFINED;
> >> >> >> >> + schedule_work(&dotg->sm_work);
> >> >> >> >> + } else {
> >> >> >> >> + otg->gadget = NULL;
> >> >> >> >> + }
> >> >> >> >> + }
> >> >> >> >> +
> >> >> >> >> + return 0;
> >> >> >> >> +}
> >> >> >> >> +
> >> >> >> >> +/**
> >> >> >> >> + * dwc3_otg_interrupt - interrupt handler for dwc3 otg
events.
> >> >> >> >
> >> >> >> > You forgot about @irq here.
> >> >> >> >
> >> >> >> >> + * @_dotg: Pointer to out controller context structure
> >> >> >> >> + *
> >> >> >> >> + * Returns IRQ_HANDLED on success otherwise IRQ_NONE.
> >> >> >> >> + */
> >> >> >> >> +static irqreturn_t dwc3_otg_interrupt(int irq, void *_dotg)
> >> >> >> >> +{
> >> >> >> >> + struct dwc3_otg *dotg = (struct dwc3_otg *)_dotg;
> >> >> >> >> + u32 oevt_reg;
> >> >> >> >> + int ret = IRQ_NONE;
> >> >> >> >> + u32 handled_irqs = 0;
> >> >> >> >> +
> >> >> >> >> + oevt_reg = dwc3_readl(dotg->regs, DWC3_OEVT);
> >> >> >> >> +
> >> >> >> >> + if (oevt_reg & DWC3_OEVTEN_OTGCONIDSTSCHNGEVNT) {
> >> >> >> >> + /*
> >> >> >> >> + * ID sts has changed, read it and later, in the
> >> >> workqueue
> >> >> >> >> + * function, switch from A to B or from B to A.
> >> >> >> >> + */
> >> >> >> >> + dotg->osts = dwc3_readl(dotg->regs, DWC3_OSTS);
> >> >> >> >> + if ((dotg->otg.phy->state == OTG_STATE_B_IDLE) ||
> >> >> >> >> + (dotg->otg.phy->state == OTG_STATE_A_IDLE)) {
> >> >> >> >> +
> >> >> >> >> + /*
> >> >> >> >> + * OTG state is ABOUT to change to A or B
> >> >> device,
> >> >> >> > but
> >> >> >> >> + * since ID sts was changed, then we return
> >> > the
> >> >> >> > state
> >> >> >> >> + * machine to the start point.
> >> >> >> >> + */
> >> >> >> >> + dotg->otg.phy->state = OTG_STATE_UNDEFINED;
> >> >> >> >> + }
> >> >> >> >> + schedule_work(&dotg->sm_work);
> >> >> >> >> +
> >> >> >> >> + handled_irqs |= DWC3_OEVTEN_OTGCONIDSTSCHNGEVNT;
> >> >> >> >> + ret = IRQ_HANDLED;
> >> >> >> >> + }
> >> >> >> >> +
> >> >> >> >> + /*
> >> >> >> >> + * Clear the interrupts we handled.
> >> >> >> >> + */
> >> >> >> >> + if (ret == IRQ_HANDLED)
> >> >> >> >> + dwc3_writel(dotg->regs, DWC3_OEVT, handled_irqs);
> >> >> >> >> +
> >> >> >> >> + return ret;
> >> >> >> >> +}
> >> >> >> >> +
> >> >> >> >> +/**
> >> >> >> >> + * dwc3_otg_sm_work - workqueue function.
> >> >> >> >> + *
> >> >> >> >> + * @w: Pointer to the dwc3 otg workqueue
> >> >> >> >
> >> >> >> > 'workqueue' term is confusing here. You use _default_ workqueue
> >> >> >> > (has type 'struct workqueue_struct') and add work (pointed by
> >> @w)
> >> >> to
> >> >> >> this
> >> >> >> > workqueue.
> >> >> >> >
> >> >> >> >> + *
> >> >> >> >> + * NOTE: After any change in phy->state,
> >> >> >> >> + * we must reschdule the state machine.
> >> >> >> >> + */
> >> >> >> >> +static void dwc3_otg_sm_work(struct work_struct *w)
> >> >> >> >> +{
> >> >> >> >> + struct dwc3_otg *dotg = container_of(w, struct dwc3_otg,
> >> >> sm_work);
> >> >> >> >> + struct usb_phy *phy = dotg->otg.phy;
> >> >> >> >> +
> >> >> >> >> + dev_dbg(phy->dev, "%s state\n", otg_state_string(phy-
> >> >> >state));
> >> >> >> >> +
> >> >> >> >> + /* Check OTG state */
> >> >> >> >> + switch (phy->state) {
> >> >> >> >> + case OTG_STATE_UNDEFINED:
> >> >> >> >> + /* Switch to A or B-Device according to IDSTS */
> >> >> >> >> + if (dotg->osts & DWC3_OTG_OSTS_CONIDSTS)
> >> >> >> >> + phy->state = OTG_STATE_B_IDLE;
> >> >> >> >> + else
> >> >> >> >> + phy->state = OTG_STATE_A_IDLE;
> >> >> >> >> +
> >> >> >> >> + schedule_work(&dotg->sm_work);
> >> >> >> >> + break;
> >> >> >> >> + case OTG_STATE_B_IDLE:
> >> >> >> >> + if (dwc3_otg_start_peripheral(&dotg->otg, 1)) {
> >> >> >> >> + /*
> >> >> >> >> + * Probably set_peripheral was not called
> >> > yet.
> >> >> >> >> + * We will re-try as soon as it will be
> >> > called
> >> >> >> >> + */
> >> >> >> >> + dev_err(phy->dev,
> >> >> >> >> + "unable to start B-device\n");
> >> >> >> >> + phy->state = OTG_STATE_UNDEFINED;
> >> >> >> >> + } else
> >> >> >> >> + phy->state = OTG_STATE_B_PERIPHERAL;
> >> >> >
> >> >> > By Linux coding style you should use braces here since another
> >> branch
> >> >> of
> >> >> > the conditional statement has them. Please check the code for
this.
> >> >> >
> >> >>
> >> >> Thanks, I will add this.
> >> >>
> >> >> Thanks for the review,
> >> >> Ido
> >> >> --
> >> >> Consultant for Qualcomm Innovation Center, Inc.
> >> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> >> >>
> >> >> --
> >> >> To unsubscribe from this list: send the line "unsubscribe linux-usb"
> >> in
> >> >> the body of a message to [email protected]
> >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> >> in
> >> > the body of a message to [email protected]
> >> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> >
> >>
> >> Ido
> >> --
> >> Consultant for Qualcomm Innovation Center, Inc.
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> >> the body of a message to [email protected]
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
> Ido
> --
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
