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

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 8293 bytes --]

Hi,

On Mon, Apr 07, 2014 at 02:36:13PM +0530, sundeep subbaraya wrote:
> >> +/**
> >> + * xudc_wrstatus - Sets up the usb device status stages.
> >> + * @udc: pointer to the usb device controller structure.
> >> + */
> >> +static void xudc_wrstatus(struct xusb_udc *udc)
> >> +{
> >> +     u32 epcfgreg;
> >> +
> >> +     epcfgreg = udc->read_fn(udc->base_address +
> >> +                             udc->ep[XUSB_EP_NUMBER_ZERO].offset)|
> >> +                             XUSB_EP_CFG_DATA_TOGGLE_MASK;
> >
> > are you really trying to mask here ? If you're trying to mask you should
> > be using a bitwise and.
> 
> This is used for making DATA1 packet for status stage during control transfers.
> IP has internally a weak check for alternating between DATA0 and DATA1
> packets using
> this bit. Firmware can set this bit to send a DATA1 packet. As we
> always need to send DATA1
> for status stage, we force IP to send DATA1 packet whatever maybe in this
> bit because of alternating behavior. Is this is confusing for the name
> *_TOGGLE_MASK ?

yeah, I guess it was the suffix _MASK, nevermind then ;-)

> >> +static int xudc_get_frame(struct usb_gadget *gadget)
> >> +{
> >> +
> >> +     struct xusb_udc *udc = to_udc(gadget);
> >> +     unsigned long flags;
> >> +     int retval;
> >> +
> >> +     if (!gadget)
> >> +             return -ENODEV;
> >
> > oh boy... so you first deref gadget, then you check for it ?
> 
> Yes i too had this doubt after looking at some of the functions (like
> ep_queue) of other controller drivers.

if there are other gadget drivers doing this, they're wrong and need to
be fixed.

> I tested sending a NULL to container_of macro I see no NULL exception.

yeah, container_of() will *always* return a valid pointer, even if it's
argument is NULL.

> Hence using container_of
> macro first and then checking for NULL input is fine. If you insist

no, it's not. You'd waste cpu cycles with pointer arithmetic for no
reason.

> this i need to change code at other
> places too.

yes.

> >> +static int xudc_wakeup(struct usb_gadget *gadget)
> >> +{
> >> +     struct xusb_udc *udc = to_udc(gadget);
> >> +     u32             crtlreg;
> >> +     int             status = -EINVAL;
> >> +     unsigned long   flags;
> >> +
> >> +     spin_lock_irqsave(&udc->lock, flags);
> >> +
> >> +     /* Remote wake up not enabled by host */
> >> +     if (!udc->remote_wkp)
> >> +             goto done;
> >> +
> >> +     crtlreg = udc->read_fn(udc->base_address + XUSB_CONTROL_OFFSET);
> >> +     /* set remote wake up bit */
> >> +     udc->write_fn(udc->base_address, XUSB_CONTROL_OFFSET, crtlreg |
> >> +                     XUSB_CONTROL_USB_RMTWAKE_MASK);
> >> +     /* wait for a while and reset remote wake up bit */
> >> +     mdelay(2);
> >
> > why 2 ms ? why not 5 ? why not 1 ? shouldn't you be polling a bit in a
> > register or something ?
> 
> IP datasheet says writing Remote wake bit to this register will send
> remote wake up
> signalling to host immediately and this bit will not be cleared by
> hardware, firmware has
> to clear it. There is no bit for polling.

then you need a better comment stating this detail.

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

> >> +     }
> >> +     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);
}


> >> +     udc->write_fn(udc->base_address, XUSB_TESTMODE_OFFSET, 0);
> >> +
> >> +     xudc_reinit(udc);
> >> +
> >> +     /* Set device address to 0.*/
> >> +     udc->write_fn(udc->base_address, XUSB_ADDRESS_OFFSET, 0);
> >> +
> >> +     ret = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
> >> +     if (ret)
> >> +             goto fail;
> >> +
> >> +     /* Enable the interrupts.*/
> >> +     udc->write_fn(udc->base_address, XUSB_IER_OFFSET,
> >> +                     XUSB_STATUS_GLOBAL_INTR_MASK | XUSB_STATUS_RESET_MASK |
> >> +                     XUSB_STATUS_SUSPEND_MASK |
> >> +                     XUSB_STATUS_RESUME_MASK |
> >
> > you're enabling resume IRQ but never handling it.
> 
> it is handled in interrupt handler. Please take a look at xudc_irq.

it's mishandled.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]