> -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Ido Shayevitz > Sent: Wednesday, August 01, 2012 11:44 PM > To: Anton Tikhomirov > Cc: 'Ido Shayevitz'; 'Felipe Balbi'; Paul.Zimmerman@xxxxxxxxxxxx; linux- > usb@xxxxxxxxxxxxxxx > 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: 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? > > 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 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 > > > > 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
