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. 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
