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. If host is not set, we will start the peripheral on cable connect, as you 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
