Hi Ido,
> -----Original Message-----
> From: [email protected] [mailto:linux-usb-
> [email protected]] On Behalf Of Ido Shayevitz
> Sent: Monday, July 30, 2012 7:53 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 Sun, July 29, 2012 7:10 pm, Anton Tikhomirov wrote:
> > Hi Ido,
> >
> > Some more comments.
> >
> >> -----Original Message-----
> >> From: Ido Shayevitz [mailto:[email protected]]
> >> Sent: Sunday, July 29, 2012 9:55 PM
> >> To: Anton Tikhomirov
> >> Subject: RE: [RFC/PATCH v3] usb: dwc3: Introduce OTG driver for dwc3
> >> >> +
> >> >> +/**
> >> >> + * dwc3_otg_set_host - bind/unbind the host controller driver.
> >> >> + *
> >> >> + * @otg: Pointer to the otg_transceiver structure.
> >> >> + * @host: Pointer to the usb_bus structure.
> >> >> + *
> >> >> + * Returns 0 on success otherwise negative errno.
> >> >> + */
> >> >> +static int dwc3_otg_set_host(struct usb_otg *otg, struct usb_bus
> > *host)
> >> >> +{
> >> >> + struct dwc3_otg *dotg = container_of(otg, struct dwc3_otg,
> otg);
> >> >> +
> >> >> + if (host) {
> >> >> + dev_dbg(otg->phy->dev, "%s: set host %s\n",
> >> >> + __func__, host->bus_name);
> >> >> + otg->host = host;
> >> >> +
> >> >> + /*
> >> >> + * Only after both peripheral and host are set then
> check
> >> >> + * OTG sm. This prevents unnecessary activation of
the
> sm
> >> >> + * in case the ID is high.
> >> >> + */
> >> >> + if (otg->gadget)
> >> >> + schedule_work(&dotg->sm_work);
> >
> > 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.
> 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
