Hi Ido, > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Ido Shayevitz > Sent: Monday, July 30, 2012 10:16 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 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? > > >> 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 > > > > 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
