Re: Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support

[Felipe Balbi]

Hi,

On Tue, Apr 08, 2014 at 09:31:29PM +0530, sundeep subbaraya wrote:
> >> >> +static const struct usb_gadget_ops xusb_udc_ops = {
> >> >> +     .get_frame = xudc_get_frame,
> >> >> +     .wakeup = xudc_wakeup,
> >> >> +     .udc_start = xudc_start,
> >> >> +     .udc_stop  = xudc_stop,
> >> >
> >> > no pullup ??? What gives ? This HW doesn't support it ? really ?
> >>
> >> Yes, there is no pull up. writing to control register to start udc is
> >> sufficient for this IP.
> >
> > right, but is there a bit inside control register which actually starts
> > the IP ? You might want to move that to ->pullup(), see how we did on
> > drivers/usb/dwc3/gadget.c::dwc3_gadget_pullup(); we're basically using
> > that to control RUN/STOP bit in DCTL register. That bit has two
> > functions: a) actually enable the ip; b) connect data pullups.
> >
> > You can actually see with a scope that the line goes high and low when
> > you mess with that bit.
> >
> > The reason I suggest this is because you don't want to let your host see
> > a connection until your gadget driver is registered and ready to go and
> > that's what the pullup method would guarantee.
> 
> No.There are only two bits in Control register, one for Ready bit, another for
> sending remote wakeup and remaining are reserved as zeroes. Until ready is
> set host do not see the gadget.

so this READY bit is what you want to toggle on pullup().

> >> >> +     }
> >> >> +     if (intrstatus & XUSB_STATUS_SUSPEND_MASK) {
> >> >> +
> >> >> +             DBG("Suspend\n");
> >> >> +
> >> >> +             /* Enable the reset and resume */
> >> >> +             intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> >> >> +             intrreg |= XUSB_STATUS_RESET_MASK | XUSB_STATUS_RESUME_MASK;
> >> >> +             udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
> >> >> +             udc->usb_state = USB_STATE_SUSPENDED;
> >> >> +
> >> >> +             if (udc->driver->suspend)
> >> >> +                     udc->driver->suspend(&udc->gadget);
> >> >> +     }
> >> >
> >> > when are you going to call driver->resume() ??
> >>
> >> When an interrupt occurs we first check if udc->usb_state =
> >> USB_STATE_SUSPENDED,
> >> if yes driver->resume is called. Also if Resume bit is set then it is
> >> cleared too. Resume status bit is set
> >> when device is resumed by host after device sends Remote wakeup
> >> signalling to host.
> >
> > <snip>
> >
> >> >> +static irqreturn_t xudc_irq(int irq, void *_udc)
> >> >> +{
> >> >> +     struct xusb_udc *udc = _udc;
> >> >> +     u32 intrstatus;
> >> >> +     u32 intrreg;
> >> >> +     u8 index;
> >> >> +     u32 bufintr;
> >> >> +
> >> >> +     spin_lock(&(udc->lock));
> >> >> +
> >> >> +     intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> >> >> +     intrreg &= ~XUSB_STATUS_INTR_EVENT_MASK;
> >> >> +     udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
> >> >> +
> >> >> +     /* Read the Interrupt Status Register.*/
> >> >> +     intrstatus = udc->read_fn(udc->base_address + XUSB_STATUS_OFFSET);
> >> >> +
> >> >> +     if (udc->usb_state == USB_STATE_SUSPENDED) {
> >> >> +
> >> >> +             DBG("Resume\n");
> >> >> +
> >> >> +             if (intrstatus & XUSB_STATUS_RESUME_MASK) {
> >> >> +                     /* Enable the reset and suspend */
> >> >> +                     intrreg = udc->read_fn(udc->base_address +
> >> >> +                                             XUSB_IER_OFFSET);
> >> >> +                     intrreg |= XUSB_STATUS_RESET_MASK |
> >> >> +                                     XUSB_STATUS_SUSPEND_MASK;
> >> >> +                     udc->write_fn(udc->base_address, XUSB_IER_OFFSET,
> >> >> +                                     intrreg);
> >> >> +             }
> >> >> +             udc->usb_state = 0;
> >> >> +
> >> >> +             if (udc->driver->resume)
> >> >> +                     udc->driver->resume(&udc->gadget);
> >
> > this is wrong, note that you would call ->resume() *every time*
> > usb_state == SUSPENDED and there's an interrupt. This means that if
> > gadget is suspended and you remove the cable, then you first call
> > ->resume() and then ->disconnect().
> >
> >> Here. calling driver->resume.
> >
> > Here's what I would do:
> >
> > if (intrstatus & XUSB_STATUS_RESUME_MASK) {
> >         bool condition = udc->usb_state != USB_STATE_SUSPENDED;
> >         dev_WARN_ONCE(dev, condition, "Resume IRQ while not suspended\n");
> >
> >         /* Enable the reset and suspend */
> >         intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> >         intrreg |= XUSB_STATUS_RESET_MASK | XUSB_STATUS_SUSPEND_MASK;
> >         udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
> >
> >         if (udc->driver->resume)
> >                 udc->driver_resume(&udc->gadget);
> > }
> 
> Resume Interrupt bit is set only when Resume happens by device sending
> Remote wakeup.

so if the host drives resume signalling there will be no interrupt ?
Well, this is wrong :-) The gadget driver needs to know about that too.
The host side can decide to wake you up.

> I am assuming we need to call driver->resume for every
> driver->suspend. Hence I implemented

no you don't, you need to let host wake you up or you drive remote
wakeup on the bus. The thing is: if the bus is suspended, there will be
*no* activity on the bus prior to host driving Resume signal or device
driving Remote Wakeup signal.

Enable RESUME IRQ on probe and never disable it, it'll be a lot easier
to implement it.

-- 
balbi

Attachment: signature.asc
Description: Digital signature