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

From: Felipe Balbi
Date: Mon Apr 07 2014 - 12:38:00 EST

Next message: Rob Herring: "Re: [PATCHv2 1/5] Input: add common DT binding for touchscreens"
Previous message: Catalin Marinas: "Re: [PATCH] arm64: Add missing Kconfig for CONFIG_STRICT_DEVMEM"
In reply to: sundeep subbaraya: "Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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: signature.asc
Description: Digital signature

Next message: Rob Herring: "Re: [PATCHv2 1/5] Input: add common DT binding for touchscreens"
Previous message: Catalin Marinas: "Re: [PATCH] arm64: Add missing Kconfig for CONFIG_STRICT_DEVMEM"
In reply to: sundeep subbaraya: "Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]