Hi Anton, On Thu, August 2, 2012 1:09 am, Anton Tikhomirov wrote: > > >> -----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. I meant micro-B type and I assume that there is an ID transition. On driver initilization (dwc3_otg_reset) I read the OSTS with the reset value of the ID pin. If the reset value of the ID pin is 1, I guess that we will see the problem you describe. Therefore we can activate the state machine from the set_peripheral function in case that reset value of the ID pin is 1, eventhough the host was not set. Similar logic can be done in set_host. This way we will cover these corner cases and avoid uneeded activation of the work function. Thanks ! >> 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 > 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
