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