Hi Ido, > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Ido Shayevitz > Sent: Monday, July 30, 2012 7:53 PM > To: Anton Tikhomirov > Cc: 'Ido Shayevitz'; 'Felipe Balbi'; linux-usb@xxxxxxxxxxxxxxx > 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:idos@xxxxxxxxxxxxxx] > >> 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 majordomo@xxxxxxxxxxxxxxx > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
