lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 7 Apr 2014 14:36:13 +0530
From:	sundeep subbaraya <sundeep.lkml@...il.com>
To:	balbi@...com
Cc:	Subbaraya Sundeep Bhatta <subbaraya.sundeep.bhatta@...inx.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Michal Simek <michals@...inx.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Subbaraya Sundeep Bhatta <sbhatta@...inx.com>
Subject: Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support

Hi Felipe,

On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi <balbi@...com> wrote:
> On Thu, Apr 03, 2014 at 01:05:19PM +0530, Subbaraya Sundeep Bhatta wrote:
>> This patch adds xilinx axi usb2 device driver support
>>
>> Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@...inx.com>
>> ---
>> Changes for v2:
>>       - Added Resume
>>       - Added Remote wakeup
>>       - Fixed v1 comments
>>
>>  drivers/usb/gadget/Kconfig      |   14 +
>>  drivers/usb/gadget/Makefile     |    1 +
>>  drivers/usb/gadget/udc-xilinx.c | 2057 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 2072 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/usb/gadget/udc-xilinx.c
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 8154165..db43a79 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -466,6 +466,20 @@ config USB_EG20T
>>         ML7213/ML7831 is companion chip for Intel Atom E6xx series.
>>         ML7213/ML7831 is completely compatible for Intel EG20T PCH.
>>
>> +config USB_GADGET_XILINX
>> +     tristate "Xilinx USB Driver"
>> +     depends on COMPILE_TEST
>> +     help
>> +       USB peripheral controller driver for Xilinx AXI USB2 device.
>> +       Xilinx AXI USB2 device is a soft IP which supports both full
>> +       and high speed USB 2.0 data transfers. It has seven configurable
>> +       endpoints(bulk or interrupt or isochronous), as well as
>> +       endpoint zero(for control transfers).
>> +
>> +       Say "y" to link the driver statically, or "m" to build a
>> +       dynamically linked module called "xilinx_udc" and force all
>> +       gadget drivers to also be dynamically linked.
>> +
>>  #
>>  # LAST -- dummy/emulated controller
>>  #
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 5f150bc..8a3fc0b 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -36,6 +36,7 @@ obj-$(CONFIG_USB_FUSB300)   += fusb300_udc.o
>>  obj-$(CONFIG_USB_FOTG210_UDC)        += fotg210-udc.o
>>  obj-$(CONFIG_USB_MV_U3D)     += mv_u3d_core.o
>>  obj-$(CONFIG_USB_GR_UDC)     += gr_udc.o
>> +obj-$(CONFIG_USB_GADGET_XILINX)      += udc-xilinx.o
>>
>>  # USB Functions
>>  usb_f_acm-y                  := f_acm.o
>> diff --git a/drivers/usb/gadget/udc-xilinx.c b/drivers/usb/gadget/udc-xilinx.c
>> new file mode 100644
>> index 0000000..5709aeb
>> --- /dev/null
>> +++ b/drivers/usb/gadget/udc-xilinx.c
>> @@ -0,0 +1,2057 @@
>> +/*
>> + * Xilinx USB peripheral controller driver
>> + *
>> + * Copyright (C) 2004 by Thomas Rathbone
>> + * Copyright (C) 2005 by HP Labs
>> + * Copyright (C) 2005 by David Brownell
>> + * Copyright (C) 2010 - 2014 Xilinx, Inc.
>> + *
>> + * Some parts of this driver code is based on the driver for at91-series
>> + * USB peripheral controller (at91_udc.c).
>> + *
>> + * This program is free software; you can redistribute it
>> + * and/or modify it under the terms of the GNU General Public
>> + * License as published by the Free Software Foundation;
>> + * either version 2 of the License, or (at your option) any
>> + * later version.
>> + */
>> +#include <linux/interrupt.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/prefetch.h>
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/io.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/delay.h>
>> +#include "gadget_chips.h"
>> +
>> +/* Register offsets for the USB device.*/
>> +#define XUSB_EP0_CONFIG_OFFSET               0x0000  /* EP0 Config Reg Offset */
>> +#define XUSB_SETUP_PKT_ADDR_OFFSET   0x0080  /* Setup Packet Address */
>> +#define XUSB_ADDRESS_OFFSET          0x0100  /* Address Register */
>> +#define XUSB_CONTROL_OFFSET          0x0104  /* Control Register */
>> +#define XUSB_STATUS_OFFSET           0x0108  /* Status Register */
>> +#define XUSB_FRAMENUM_OFFSET         0x010C  /* Frame Number Register */
>> +#define XUSB_IER_OFFSET                      0x0110  /* Interrupt Enable Register */
>> +#define XUSB_BUFFREADY_OFFSET                0x0114  /* Buffer Ready Register */
>> +#define XUSB_TESTMODE_OFFSET         0x0118  /* Test Mode Register */
>> +#define XUSB_DMA_RESET_OFFSET                0x0200  /* DMA Soft Reset Register */
>> +#define XUSB_DMA_CONTROL_OFFSET              0x0204  /* DMA Control Register */
>> +#define XUSB_DMA_DSAR_ADDR_OFFSET    0x0208  /* DMA source Address Reg */
>> +#define XUSB_DMA_DDAR_ADDR_OFFSET    0x020C  /* DMA destination Addr Reg */
>> +#define XUSB_DMA_LENGTH_OFFSET               0x0210  /* DMA Length Register */
>> +#define XUSB_DMA_STATUS_OFFSET               0x0214  /* DMA Status Register */
>> +
>> +/* Endpoint Configuration Space offsets */
>> +#define XUSB_EP_CFGSTATUS_OFFSET     0x00    /* Endpoint Config Status  */
>> +#define XUSB_EP_BUF0COUNT_OFFSET     0x08    /* Buffer 0 Count */
>> +#define XUSB_EP_BUF1COUNT_OFFSET     0x0C    /* Buffer 1 Count */
>> +
>> +#define XUSB_CONTROL_USB_READY_MASK  0x80000000 /* USB ready Mask */
>> +#define XUSB_CONTROL_USB_RMTWAKE_MASK        0x40000000 /* Remote wake up mask */
>> +
>> +/* Interrupt register related masks.*/
>> +#define XUSB_STATUS_GLOBAL_INTR_MASK 0x80000000 /* Global Intr Enable */
>> +#define XUSB_STATUS_RESUME_MASK              0x01000000 /* USB Resume Mask */
>> +#define XUSB_STATUS_RESET_MASK               0x00800000 /* USB Reset Mask */
>> +#define XUSB_STATUS_SUSPEND_MASK     0x00400000 /* USB Suspend Mask */
>> +#define XUSB_STATUS_FIFO_BUFF_RDY_MASK       0x00100000 /* FIFO Buff Ready Mask */
>> +#define XUSB_STATUS_FIFO_BUFF_FREE_MASK      0x00080000 /* FIFO Buff Free Mask */
>> +#define XUSB_STATUS_SETUP_PACKET_MASK        0x00040000 /* Setup packet received */
>> +#define XUSB_STATUS_EP1_BUFF2_COMP_MASK      0x00000200 /* EP 1 Buff 2 Processed */
>> +#define XUSB_STATUS_EP1_BUFF1_COMP_MASK      0x00000002 /* EP 1 Buff 1 Processed */
>> +#define XUSB_STATUS_EP0_BUFF2_COMP_MASK      0x00000100 /* EP 0 Buff 2 Processed */
>> +#define XUSB_STATUS_EP0_BUFF1_COMP_MASK      0x00000001 /* EP 0 Buff 1 Processed */
>> +#define XUSB_STATUS_HIGH_SPEED_MASK  0x00010000 /* USB Speed Mask */
>> +/* Suspend,Reset and Disconnect Mask */
>> +#define XUSB_STATUS_INTR_EVENT_MASK  0x00C00000
>> +/* Buffers  completion Mask */
>> +#define XUSB_STATUS_INTR_BUFF_COMP_ALL_MASK  0x0000FEFF
>> +/* Mask for buffer 0 and buffer 1 completion for all Endpoints */
>> +#define XUSB_STATUS_INTR_BUFF_COMP_SHIFT_MASK        0x00000101
>> +#define XUSB_STATUS_EP_BUFF2_SHIFT   8          /* EP buffer offset */
>> +
>> +/* Endpoint Configuration Status Register */
>> +#define XUSB_EP_CFG_VALID_MASK               0x80000000 /* Endpoint Valid bit */
>> +#define XUSB_EP_CFG_STALL_MASK               0x40000000 /* Endpoint Stall bit */
>> +#define XUSB_EP_CFG_DATA_TOGGLE_MASK 0x08000000 /* Endpoint Data toggle */
>> +
>> +/* USB device specific global configuration constants.*/
>> +#define XUSB_MAX_ENDPOINTS           8       /* Maximum End Points */
>> +#define XUSB_EP_NUMBER_ZERO          0       /* End point Zero */
>> +/* DPRAM is the source address for DMA transfer */
>> +#define XUSB_DMA_READ_FROM_DPRAM     0x80000000
>> +#define XUSB_DMA_DMASR_BUSY          0x80000000 /* DMA busy */
>> +#define XUSB_DMA_DMASR_ERROR         0x40000000 /* DMA Error */
>> +/*
>> + * When this bit is set, the DMA buffer ready bit is set by hardware upon
>> + * DMA transfer completion.
>> + */
>> +#define XUSB_DMA_BRR_CTRL            0x40000000 /* DMA bufready ctrl bit */
>> +/* Phase States */
>> +#define SETUP_PHASE                  0x0000  /* Setup Phase */
>> +#define DATA_PHASE                   0x0001  /* Data Phase */
>> +#define STATUS_PHASE                 0x0002  /* Status Phase */
>> +
>> +#define EP0_MAX_PACKET               64 /* Endpoint 0 maximum packet length */
>> +
>> +/* container_of helper macros */
>> +#define to_udc(g)     container_of((g), struct xusb_udc, gadget)
>> +#define to_xusb_ep(ep)        container_of((ep), struct xusb_ep, ep_usb)
>> +#define to_xusb_req(req) container_of((req), struct xusb_req, usb_req)
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +#ifdef DEBUG
>> +#define DBG(fmt, args...)    pr_debug("[%s]  " fmt "\n", \
>> +                             __func__, ## args)
>> +#else
>> +#define DBG(fmt, args...)    do {} while (0)
>> +#endif
>> +
>> +#ifdef VERBOSE
>> +#define VDBG         DBG
>> +#else
>> +#define VDBG(stuff...)       do {} while (0)
>> +#endif
>> +
>> +#define ERR(stuff...)                pr_err("udc: " stuff)
>> +#define WARNING(stuff...)            pr_warn("udc: " stuff)
>> +#define INFO(stuff...)               pr_info("udc: " stuff)
>
> NACK, this is always to cleanup later. Please stick to
> dev_{info,err,warn,dbg,vdbg}()

Sure. i will revert earlier code for debug messages using dev_

>> +struct xusb_udc {
>> +     struct usb_gadget gadget;
>> +     struct xusb_ep ep[8];
>> +     struct usb_gadget_driver *driver;
>> +     struct cmdbuf ch9cmd;
>> +     u32 usb_state;
>> +     u32 remote_wkp;
>> +     unsigned int (*read_fn)(void __iomem *);
>> +     void (*write_fn)(void __iomem *, u32, u32);
>
> why do you need these to be function pointers ? Because of endianness ?
> generic readl()/writel() already take care of that.
>
>> +     void __iomem *base_address;
>> +     spinlock_t lock;
>> +     bool dma_enabled;
>> +};
>> +
>> +/* Endpoint buffer start addresses in the core */
>> +static u32 rambase[8] = { 0x22, 0x1000, 0x1100, 0x1200, 0x1300, 0x1400, 0x1500,
>> +                     0x1600 };
>> +
>> +static const char driver_name[] = "xilinx-udc";
>> +static const char ep0name[] = "ep0";
>> +
>> +/* Control endpoint configuration.*/
>> +static struct usb_endpoint_descriptor config_bulk_out_desc = {
>
> should be const, you never modify this.

Ok.

>
>> +     .bLength = USB_DT_ENDPOINT_SIZE,
>> +     .bDescriptorType = USB_DT_ENDPOINT,
>> +     .bEndpointAddress = USB_DIR_OUT,
>> +     .bmAttributes = USB_ENDPOINT_XFER_BULK,
>> +     .wMaxPacketSize = __constant_cpu_to_le16(0x40),
>
> let's use the decimal here just for clarity.

Ok

>
>> +/**
>> + * 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 ?

>
>> +     udc->write_fn(udc->base_address, udc->ep[XUSB_EP_NUMBER_ZERO].offset,
>> +                     epcfgreg);
>> +     udc->write_fn(udc->base_address, udc->ep[XUSB_EP_NUMBER_ZERO].offset +
>> +                       XUSB_EP_BUF0COUNT_OFFSET, 0);
>> +     udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET, 1);
>
> you can improve redability on this by defining some local variables:

Ok .

>
> struct xusb_udc_ep *ep0 = &udc->ep[XUSB_EP_NUMBER_ZERO];
> u32 reg;
>
> reg = xudc_readl(udc->base_address, ep0->offset);
> reg |= XUSB_EP_CFG_DATA_TOGGLE_MASK;
> xudc_writel(udc->base_address, ep0->offset + XUSB_EP_BUF0COUNT_OFFSET, 0);
> xudc_writel(udc->base_address, XUSB_BUFFREADY_OFFSET, 1);
>
> and so on, likewise for all other functions.
>
>> +static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
>
> please prepend this with xudc_, it makes tracing a lot easier.

Ok

>
>> +{
>> +     struct xusb_udc *udc;
>> +     int rc = 0;
>> +     unsigned long timeout;
>> +
>> +     udc = ep->udc;
>> +     /*
>> +      * Set the addresses in the DMA source and
>> +      * destination registers and then set the length
>> +      * into the DMA length register.
>> +      */
>> +     udc->write_fn(udc->base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
>> +     udc->write_fn(udc->base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
>> +     udc->write_fn(udc->base_address, XUSB_DMA_LENGTH_OFFSET, length);
>> +
>> +     /*
>> +      * Wait till DMA transaction is complete and
>> +      * check whether the DMA transaction was
>> +      * successful.
>> +     */
>> +     while ((udc->read_fn(ep->udc->base_address + XUSB_DMA_STATUS_OFFSET) &
>> +                     XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
>> +             timeout = jiffies + 10000;
>> +
>> +             if (time_after(jiffies, timeout)) {
>> +                     rc = -ETIMEDOUT;
>> +                     goto clean;
>> +             }
>> +     }
>
> don't you get an IRQ for DMA completion ? If you do, you could be using
> wait_for_completion()

ok will use DMA irqs.

>
>> +     if ((udc->read_fn(udc->base_address + XUSB_DMA_STATUS_OFFSET) &
>> +                             XUSB_DMA_DMASR_ERROR) == XUSB_DMA_DMASR_ERROR){
>> +             DBG("DMA Error\n");
>> +             rc = -EINVAL;
>> +     }
>> +clean:
>> +     if (ep->is_in) {
>> +             dma_unmap_single(udc->gadget.dev.parent, src,
>> +                                     length, DMA_TO_DEVICE);
>> +     } else {
>> +             dma_unmap_single(udc->gadget.dev.parent, dst,
>> +                                     length, DMA_FROM_DEVICE);
>
> NACK, use generic usb_gadget_map_reqeust() and
> usb_gadget_unmap_request().

Ok

>
>> +static int dma_send(struct xusb_ep *ep, u8 *buffer, u32 length)
>
> prepend with xudc_

Ok.

>
>> +{
>> +     u32 *eprambase;
>> +     dma_addr_t src;
>> +     u32 dst;
>> +     int ret;
>> +     struct xusb_udc *udc;
>> +
>> +     udc = ep->udc;
>> +     src = dma_map_single(udc->gadget.dev.parent, buffer, length,
>> +                             DMA_TO_DEVICE);
>> +     if (dma_mapping_error(udc->gadget.dev.parent, src)) {
>> +             DBG("failed to map DMA\n");
>> +             return -EFAULT;
>> +     }
>
> use generic mapping functions.

Ok.

>
>> +static int dma_receive(struct xusb_ep *ep, u8 *buffer, u32 length)
>
> prepend with xudc_
>
>> +{
>> +     u32 *eprambase;
>> +     u32 src;
>> +     dma_addr_t dst;
>> +     int ret;
>> +     struct xusb_udc *udc;
>> +
>> +     udc = ep->udc;
>> +     dst = dma_map_single(udc->gadget.dev.parent, buffer, length,
>> +                             DMA_FROM_DEVICE);
>> +     if (dma_mapping_error(udc->gadget.dev.parent, dst)) {
>> +             DBG("failed to map DMA\n");
>> +             return -EFAULT;
>> +     }
>
> use generic mapping functions

Ok

>
>> +
>> +     if (!ep->curbufnum && !ep->buffer0ready) {
>> +             /* Get the Buffer address and copy the transmit data */
>> +             eprambase = (u32 __force *)(ep->udc->base_address +
>> +                             ep->rambase);
>> +             src = virt_to_phys(eprambase);
>> +             udc->write_fn(udc->base_address, XUSB_DMA_CONTROL_OFFSET,
>> +                             XUSB_DMA_BRR_CTRL | XUSB_DMA_READ_FROM_DPRAM |
>> +                             (1 << ep->epnumber));
>> +             ep->buffer0ready = 1;
>> +             ep->curbufnum = 1;
>> +     } else if (ep->curbufnum && !ep->buffer1ready) {
>> +             /* Get the Buffer address and copy the transmit data */
>> +             eprambase = (u32 __force *)(ep->udc->base_address +
>> +                             ep->rambase + ep->ep_usb.maxpacket);
>> +             src = virt_to_phys(eprambase);
>> +             udc->write_fn(udc->base_address, XUSB_DMA_CONTROL_OFFSET,
>> +                             XUSB_DMA_BRR_CTRL | XUSB_DMA_READ_FROM_DPRAM |
>> +                             (1 << (ep->epnumber +
>> +                             XUSB_STATUS_EP_BUFF2_SHIFT)));
>> +             ep->buffer1ready = 1;
>> +             ep->curbufnum = 0;
>> +     } else {
>> +             /* None of the ping-pong buffers are ready currently */
>> +             return 1;
>
> you *must* return a proper error code here. -EAGAIN sounds like the one
> you want.

Ok.

>
>> +static int xudc_eptxrx(struct xusb_ep *ep, u8 *bufferptr, u32 bufferlen)
>> +{
>> +     u32 *eprambase;
>> +     u32 bytestosend;
>> +     u8 *temprambase;
>> +     int rc = 0;
>> +     struct xusb_udc *udc = ep->udc;
>> +
>> +     bytestosend = bufferlen;
>> +     if (udc->dma_enabled) {
>> +             if (ep->is_in)
>> +                     rc = dma_send(ep, bufferptr, bufferlen);
>> +             else
>> +                     rc = dma_receive(ep, bufferptr, bufferlen);
>> +             return rc;
>> +     }
>> +     /* Put the transmit buffer into the correct ping-pong buffer.*/
>> +     if (!ep->curbufnum && !ep->buffer0ready) {
>> +             /* Get the Buffer address and copy the transmit data.*/
>> +             eprambase = (u32 __force *)(udc->base_address + ep->rambase);
>> +             while (bytestosend > 3) {
>> +                     if (ep->is_in)
>> +                             *eprambase++ = *(u32 *)bufferptr;
>> +                     else
>> +                             *(u32 *)bufferptr = *eprambase++;
>> +                     bufferptr += 4;
>> +                     bytestosend -= 4;
>> +             }
>> +             temprambase = (u8 *)eprambase;
>> +             while (bytestosend--) {
>> +                     if (ep->is_in)
>> +                             *temprambase++ = *bufferptr++;
>> +                     else
>> +                             *bufferptr++ = *temprambase++;
>> +             }
>> +             /*
>> +              * Set the Buffer count register with transmit length
>> +              * and enable the buffer for transmission.
>> +              */
>> +             if (ep->is_in)
>> +                     udc->write_fn(udc->base_address, ep->offset +
>> +                                     XUSB_EP_BUF0COUNT_OFFSET, bufferlen);
>> +             udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET,
>> +                             1 << ep->epnumber);
>> +             ep->buffer0ready = 1;
>> +             ep->curbufnum = 1;
>> +     } else if ((ep->curbufnum == 1) && (!ep->buffer1ready)) {
>> +             /* Get the Buffer address and copy the transmit data.*/
>> +             eprambase = (u32 __force *)(udc->base_address + ep->rambase +
>> +                             ep->ep_usb.maxpacket);
>> +             while (bytestosend > 3) {
>> +                     if (ep->is_in)
>> +                             *eprambase++ = *(u32 *)bufferptr;
>> +                     else
>> +                             *(u32 *)bufferptr = *eprambase++;
>> +                     bufferptr += 4;
>> +                     bytestosend -= 4;
>> +             }
>> +             temprambase = (u8 *)eprambase;
>> +             while (bytestosend--) {
>> +                     if (ep->is_in)
>> +                             *temprambase++ = *bufferptr++;
>> +                     else
>> +                             *bufferptr++ = *temprambase++;
>> +             }
>> +             /*
>> +              * Set the Buffer count register with transmit
>> +              * length and enable the buffer for
>> +              * transmission.
>> +              */
>> +             if (ep->is_in)
>> +                     udc->write_fn(udc->base_address, ep->offset +
>> +                                     XUSB_EP_BUF1COUNT_OFFSET, bufferlen);
>> +             udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET,
>> +                             1 << (ep->epnumber +
>> +                             XUSB_STATUS_EP_BUFF2_SHIFT));
>> +             ep->buffer1ready = 1;
>> +             ep->curbufnum = 0;
>> +     } else {
>> +             /* None of the ping-pong buffers are ready currently */
>> +             return 1;
>> +     }
>> +     return rc;
>> +}
>> +
>> +/**
>> + * xudc_done - Exeutes the endpoint data transfer completion tasks.
>> + * @ep: pointer to the usb device endpoint structure.
>> + * @req: pointer to the usb request structure.
>> + * @status: Status of the data transfer.
>> + *
>> + * Deletes the message from the queue and updates data transfer completion
>> + * status.
>> + */
>> +static void xudc_done(struct xusb_ep *ep, struct xusb_req *req, int status)
>> +{
>> +     u8 stopped = ep->stopped;
>> +
>> +     list_del_init(&req->queue);
>> +
>> +     if (req->usb_req.status == -EINPROGRESS)
>> +             req->usb_req.status = status;
>> +     else
>> +             status = req->usb_req.status;
>> +
>> +     if (status && status != -ESHUTDOWN)
>> +             DBG("%s done %p, status %d\n", ep->ep_usb.name, req, status);
>
> dev_dbg()
>
>> +     ep->stopped = 1;
>> +
>> +     spin_unlock(&ep->udc->lock);
>> +     if (req->usb_req.complete)
>> +             req->usb_req.complete(&ep->ep_usb, &req->usb_req);
>> +     spin_lock(&ep->udc->lock);
>> +
>> +     ep->stopped = stopped;
>> +}
>> +
>> +/**
>> + * xudc_read_fifo - Reads the data from the given endpoint buffer.
>> + * @ep: pointer to the usb device endpoint structure.
>> + * @req: pointer to the usb request structure.
>> + *
>> + * Return: 1 if request completed/dequeued or 0 if req is not
>> + *           completed
>
> should return 0 on success and negative errno in case it doesn't
> complete. You must review your error handling!!

Ok

>
>> +static int xudc_read_fifo(struct xusb_ep *ep, struct xusb_req *req)
>> +{
>> +     u8 *buf;
>> +     u32 is_short, count, bufferspace;
>
> avoid multiple declarations in one line.

Ok.

>
>> +     u8 bufoffset;
>> +     u8 two_pkts = 0;
>> +     struct xusb_udc *udc = ep->udc;
>> +
>> +     if ((ep->buffer0ready == 1) && (ep->buffer1ready == 1)) {
>> +             DBG("Packet NOT ready!\n");
>> +             return 0;
>> +     }
>> +top:
>> +     if (ep->curbufnum)
>> +             bufoffset = XUSB_EP_BUF1COUNT_OFFSET;
>> +     else
>> +             bufoffset = XUSB_EP_BUF0COUNT_OFFSET;
>> +     count = udc->read_fn(ep->udc->base_address + ep->offset + bufoffset);
>> +     if (!ep->buffer0ready && !ep->buffer1ready)
>> +             two_pkts = 1;
>> +
>> +     DBG("curbufnum is %d  and buf0rdy is %d, buf1rdy is %d\n",
>> +             ep->curbufnum, ep->buffer0ready, ep->buffer1ready);
>> +
>> +     buf = req->usb_req.buf + req->usb_req.actual;
>> +     prefetchw(buf);
>> +     bufferspace = req->usb_req.length - req->usb_req.actual;
>> +     req->usb_req.actual += min(count, bufferspace);
>> +     is_short = count < ep->ep_usb.maxpacket;
>> +
>> +     if (unlikely(!bufferspace)) {
>> +             /*
>> +              * This happens when the driver's buffer
>> +              * is smaller than what the host sent.
>> +              * discard the extra data.
>> +              */
>> +             if (req->usb_req.status != -EOVERFLOW)
>> +                     DBG("%s overflow %d\n", ep->ep_usb.name, count);
>> +                     req->usb_req.status = -EOVERFLOW;
>> +     } else {
>> +             switch (xudc_eptxrx(ep, buf, count)) {
>> +             case 0:
>> +                     VDBG("read %s, %d bytes%s req %p %d/%d\n",
>> +                             ep->ep_usb.name, count, is_short ? "/S" : "",
>> +                             req, req->usb_req.actual, req->usb_req.length);
>> +                     bufferspace -= count;
>> +                     /* Completion */
>> +                     if ((req->usb_req.actual ==
>> +                             req->usb_req.length) || is_short) {
>> +                             xudc_done(ep, req, 0);
>> +                             return 1;
>> +                     }
>> +                     if (two_pkts) {
>> +                             two_pkts = 0;
>> +                             goto top;
>> +                     }
>> +                     break;
>> +             case 1:
>> +                     VDBG("Rx buffers busy\n");
>> +                     req->usb_req.actual -= min(count, bufferspace);
>> +                     break;
>> +             case -EINVAL:
>> +             case -EFAULT:
>> +             case -ETIMEDOUT:
>> +                     /* DMA error, dequeue the request */
>> +                     xudc_done(ep, req, -ECONNRESET);
>> +                     return 1;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_write_fifo - Writes data into the given endpoint buffer.
>> + * @ep: pointer to the usb device endpoint structure.
>> + * @req: pointer to the usb request structure.
>> + *
>> + * Return: 1 if request completed/dequeued or 0 if req is not
>> + *           completed
>> + *
>> + * Loads endpoint buffer for an IN packet.
>> + */
>> +static int xudc_write_fifo(struct xusb_ep *ep, struct xusb_req *req)
>> +{
>> +     u8 *buf;
>> +     u32 max;
>> +     u32 length;
>> +     int is_last, is_short = 0;
>> +
>> +     max = le16_to_cpu(ep->desc->wMaxPacketSize);
>> +     buf = req->usb_req.buf + req->usb_req.actual;
>> +     prefetch(buf);
>> +     length = req->usb_req.length - req->usb_req.actual;
>> +     length = min(length, max);
>> +
>> +     switch (xudc_eptxrx(ep, buf, length)) {
>> +     case 0:
>> +             req->usb_req.actual += length;
>> +             if (unlikely(length != max)) {
>> +                     is_last = is_short = 1;
>> +             } else {
>> +                     if (likely(req->usb_req.length !=
>> +                                req->usb_req.actual) || req->usb_req.zero)
>> +                             is_last = 0;
>> +                     else
>> +                             is_last = 1;
>> +             }
>> +             VDBG("wrote %s %d bytes%s%s %d left %p\n", ep->ep_usb.name,
>> +                     length, is_last ? "/L" : "", is_short ? "/S" : "",
>> +                     req->usb_req.length - req->usb_req.actual, req);
>> +             if (is_last) {
>> +                     xudc_done(ep, req, 0);
>> +                     return 1;
>> +             }
>> +             break;
>> +     case 1:
>> +             VDBG("Tx buffers busy\n");
>> +             break;
>> +     case -EINVAL:
>> +     case -EFAULT:
>> +     case -ETIMEDOUT:
>> +             /* DMA error, dequeue the request */
>> +             xudc_done(ep, req, -ECONNRESET);
>> +             return 1;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_nuke - Cleans up the data transfer message list.
>> + * @ep: pointer to the usb device endpoint structure.
>> + * @status: Status of the data transfer.
>> + */
>> +static void xudc_nuke(struct xusb_ep *ep, int status)
>> +{
>> +     struct xusb_req *req;
>> +
>> +     while (!list_empty(&ep->queue)) {
>> +             req = list_entry(ep->queue.next, struct xusb_req, queue);
>> +             xudc_done(ep, req, status);
>> +     }
>> +}
>> +
>> +/***************************** Endpoint related functions*********************/
>> +/**
>> + * xudc_ep_set_halt - Stalls/unstalls the given endpoint.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + * @value: value to indicate stall/unstall.
>> + *
>> + * Return: 0 for success and error value on failure
>> + */
>> +static int xudc_ep_set_halt(struct usb_ep *_ep, int value)
>> +{
>> +     struct xusb_ep *ep = to_xusb_ep(_ep);
>> +     unsigned long flags;
>> +     u32 epcfgreg;
>> +     struct xusb_udc *udc = ep->udc;
>> +
>> +     if (!_ep || (!ep->desc && ep->epnumber))
>> +             return -EINVAL;
>> +
>> +     spin_lock_irqsave(&udc->lock, flags);
>> +
>> +     if (ep->is_in && (!list_empty(&ep->queue)) && value) {
>> +             spin_unlock_irqrestore(&udc->lock, flags);
>> +             return -EAGAIN;
>> +     }
>> +     if ((ep->buffer0ready == 1) || (ep->buffer1ready == 1)) {
>> +             spin_unlock_irqrestore(&udc->lock, flags);
>> +             return -EAGAIN;
>> +     }
>> +     if (value) {
>> +             /* Stall the device.*/
>> +             epcfgreg = udc->read_fn(udc->base_address +
>> +                                ep->offset);
>> +             epcfgreg |= XUSB_EP_CFG_STALL_MASK;
>> +
>> +             udc->write_fn(udc->base_address, ep->offset, epcfgreg);
>> +             ep->stopped = 1;
>> +     } else {
>> +             ep->stopped = 0;
>> +             /* Unstall the device.*/
>> +             epcfgreg = udc->read_fn(udc->base_address +
>> +                                         ep->offset);
>> +             epcfgreg &= ~XUSB_EP_CFG_STALL_MASK;
>> +             udc->write_fn(udc->base_address, ep->offset, epcfgreg);
>> +             if (ep->epnumber) {
>> +                     /* Reset the toggle bit.*/
>> +                     epcfgreg = udc->read_fn(ep->udc->base_address +
>> +                                                 ep->offset);
>> +                     epcfgreg &= ~XUSB_EP_CFG_DATA_TOGGLE_MASK;
>> +                     udc->write_fn(udc->base_address, ep->offset, epcfgreg);
>> +             }
>> +     }
>> +     spin_unlock_irqrestore(&udc->lock, flags);
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_ep_enable - Enables the given endpoint.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + * @desc: pointer to usb endpoint descriptor.
>> + *
>> + * Return: 0 for success and error value on failure
>> + */
>> +static int xudc_ep_enable(struct usb_ep *_ep,
>> +                       const struct usb_endpoint_descriptor *desc)
>> +{
>> +     struct xusb_ep *ep = to_xusb_ep(_ep);
>> +     u32 tmp;
>> +     u8 eptype = 0;
>> +     unsigned long flags;
>> +     u32 epcfg;
>> +     struct xusb_udc *udc = ep->udc;
>> +
>> +     /*
>> +      * The check for _ep->name == ep0name is not done as this enable is used
>> +      * for enabling ep0 also. In other gadget drivers, this ep name is not
>> +      * used.
>> +      */
>> +     if (!_ep || !desc || ep->desc ||
>> +                     desc->bDescriptorType != USB_DT_ENDPOINT) {
>> +             DBG("bad ep or descriptor\n");
>> +             return -EINVAL;
>> +     }
>> +     if (!udc->driver || udc->gadget.speed == USB_SPEED_UNKNOWN) {
>> +             DBG("bogus device state\n");
>> +             return -ESHUTDOWN;
>> +     }
>> +
>> +     ep->is_in = ((desc->bEndpointAddress & USB_DIR_IN) != 0);
>> +     /* Bit 3...0:endpoint number */
>> +     ep->epnumber = (desc->bEndpointAddress & 0x0f);
>> +     ep->stopped = 0;
>> +     ep->desc = desc;
>> +     ep->ep_usb.desc = desc;
>> +     tmp = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
>> +
>> +     spin_lock_irqsave(&udc->lock, flags);
>> +
>> +     ep->ep_usb.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
>> +     switch (tmp) {
>> +     case USB_ENDPOINT_XFER_CONTROL:
>> +             DBG("only one control endpoint\n");
>> +             /* NON- ISO */
>> +             eptype = 0;
>> +             spin_unlock_irqrestore(&ep->udc->lock, flags);
>> +             return -EINVAL;
>> +     case USB_ENDPOINT_XFER_INT:
>> +             /* NON- ISO */
>> +             eptype = 0;
>> +             if (ep->ep_usb.maxpacket > 64)
>> +                     goto bogus_max;
>> +             break;
>> +     case USB_ENDPOINT_XFER_BULK:
>> +             /* NON- ISO */
>> +             eptype = 0;
>> +             switch (ep->ep_usb.maxpacket) {
>> +             case 8:
>> +             case 16:
>> +             case 32:
>> +             case 64:
>> +             case 512:
>> +                     goto ok;
>> +             }
>> +bogus_max:
>> +             DBG("bogus maxpacket %d\n", ep->ep_usb.maxpacket);
>> +             spin_unlock_irqrestore(&ep->udc->lock, flags);
>> +             return -EINVAL;
>> +     case USB_ENDPOINT_XFER_ISOC:
>> +             /* ISO */
>> +             eptype = 1;
>> +             ep->is_iso = 1;
>> +             break;
>> +     }
>> +ok:
>> +     ep->eptype = eptype;
>> +     ep->buffer0ready = 0;
>> +     ep->buffer1ready = 0;
>> +     ep->curbufnum = 0;
>> +     ep->rambase = rambase[ep->epnumber];
>> +     xudc_epconfig(ep, udc);
>> +
>> +     DBG("Enable Endpoint %d max pkt is %d\n",
>> +                     ep->epnumber, ep->ep_usb.maxpacket);
>> +
>> +     /* Enable the End point.*/
>> +     epcfg = udc->read_fn(udc->base_address + ep->offset);
>> +     epcfg |= XUSB_EP_CFG_VALID_MASK;
>> +     udc->write_fn(udc->base_address, ep->offset, epcfg);
>> +     if (ep->epnumber)
>> +             ep->rambase <<= 2;
>> +
>> +     if (ep->epnumber)
>> +             udc->write_fn(udc->base_address, XUSB_IER_OFFSET,
>> +                             (udc->read_fn(ep->udc->base_address +
>> +                             XUSB_IER_OFFSET) |
>> +                             (XUSB_STATUS_INTR_BUFF_COMP_SHIFT_MASK <<
>> +                             ep->epnumber)));
>> +     if (ep->epnumber && !ep->is_in) {
>> +             /* Set the buffer ready bits.*/
>> +             udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET,
>> +                             1 << ep->epnumber);
>> +             ep->buffer0ready = 1;
>> +             udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET,
>> +                             (1 << (ep->epnumber +
>> +                             XUSB_STATUS_EP_BUFF2_SHIFT)));
>> +             ep->buffer1ready = 1;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&udc->lock, flags);
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_ep_disable - Disables the given endpoint.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + *
>> + * Return: 0 for success and error value on failure
>> + */
>> +static int xudc_ep_disable(struct usb_ep *_ep)
>> +{
>> +     struct xusb_ep *ep = to_xusb_ep(_ep);
>> +     unsigned long flags;
>> +     u32 epcfg;
>> +     struct xusb_udc *udc = ep->udc;
>> +
>> +     udc = ep->udc;
>> +     if (ep == &udc->ep[XUSB_EP_NUMBER_ZERO]) {
>> +             DBG("Ep0 disable called\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     spin_lock_irqsave(&udc->lock, flags);
>> +     xudc_nuke(ep, -ESHUTDOWN);
>> +
>> +     /* Restore the endpoint's pristine config */
>> +     ep->desc = NULL;
>> +     ep->ep_usb.desc = NULL;
>> +     ep->stopped = 1;
>> +
>> +     DBG("USB Ep %d disable\n ", ep->epnumber);
>> +     /* Disable the endpoint.*/
>> +     epcfg = udc->read_fn(udc->base_address + ep->offset);
>> +     epcfg &= ~XUSB_EP_CFG_VALID_MASK;
>> +     udc->write_fn(udc->base_address, ep->offset, epcfg);
>> +
>> +     spin_unlock_irqrestore(&udc->lock, flags);
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_ep_alloc_request - Initializes the request queue.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + * @gfp_flags: Flags related to the request call.
>> + *
>> + * Return: pointer to request structure on success and a NULL on failure.
>> + */
>> +static struct usb_request *xudc_ep_alloc_request(struct usb_ep *_ep,
>> +                                              gfp_t gfp_flags)
>> +{
>> +     struct xusb_req *req;
>> +
>> +     req = kzalloc(sizeof(*req), gfp_flags);
>> +     if (!req)
>> +             return NULL;
>> +     req->ep = to_xusb_ep(_ep);
>> +     INIT_LIST_HEAD(&req->queue);
>> +     return &req->usb_req;
>> +}
>> +
>> +/**
>> + * xudc_free_request - Releases the request from queue.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + * @_req: pointer to the usb request structure.
>> + */
>> +static void xudc_free_request(struct usb_ep *_ep, struct usb_request *_req)
>> +{
>> +     struct xusb_req *req = to_xusb_req(_req);
>> +
>> +     kfree(req);
>> +}
>> +
>> +/**
>> + * xudc_ep_queue - Adds the request to the queue.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + * @_req: pointer to the usb request structure.
>> + * @gfp_flags: Flags related to the request call.
>> + *
>> + * Return: 0 for success and error value on failure
>> + */
>> +static int xudc_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
>> +                      gfp_t gfp_flags)
>> +{
>> +     struct xusb_req *req;
>> +     struct xusb_ep *ep;
>> +     unsigned long flags;
>> +     u32 length, count;
>> +     u8 *corebuf;
>> +     struct xusb_udc *udc;
>> +
>> +     req = to_xusb_req(_req);
>> +     ep = to_xusb_ep(_ep);
>> +
>> +     if (!_req || !_req->complete || !_req->buf
>> +                     || !list_empty(&req->queue)) {
>> +             DBG("invalid request\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!_ep || (!ep->desc && ep->ep_usb.name != ep0name)) {
>> +             DBG("invalid ep\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     udc = ep->udc;
>> +     if (!udc || !udc->driver || udc->gadget.speed == USB_SPEED_UNKNOWN) {
>> +             DBG("bogus device state\n");
>> +             return -EINVAL;
>> +     }
>> +     spin_lock_irqsave(&udc->lock, flags);
>> +
>> +     _req->status = -EINPROGRESS;
>> +     _req->actual = 0;
>> +     /* Try to kickstart any empty and idle queue */
>> +     if (list_empty(&ep->queue)) {
>> +             if (!ep->epnumber) {
>> +                     ep->data = req;
>> +                     if (udc->ch9cmd.setup.bRequestType & USB_DIR_IN) {
>> +                             udc->ch9cmd.wrtptr = req->usb_req.buf +
>> +                                                     req->usb_req.actual;
>> +                             prefetch(udc->ch9cmd.wrtptr);
>> +                             length = req->usb_req.length -
>> +                                             req->usb_req.actual;
>> +                             corebuf = (void __force *) ((ep->rambase << 2) +
>> +                                         ep->udc->base_address);
>> +                             udc->ch9cmd.writecount = length;
>> +                             length = count = min_t(u32, length,
>> +                                                     EP0_MAX_PACKET);
>> +                             while (length--)
>> +                                     *corebuf++ = *udc->ch9cmd.wrtptr++;
>> +                             udc->write_fn(udc->base_address,
>> +                                             XUSB_EP_BUF0COUNT_OFFSET,
>> +                                             count);
>> +                             udc->write_fn(udc->base_address,
>> +                                             XUSB_BUFFREADY_OFFSET, 1);
>> +                             udc->ch9cmd.writecount -= count;
>> +                     } else {
>> +                             if (udc->ch9cmd.setup.wLength) {
>> +                                     udc->ch9cmd.readptr =
>> +                                             req->usb_req.buf +
>> +                                                     req->usb_req.actual;
>> +                                     udc->write_fn(udc->base_address,
>> +                                               XUSB_EP_BUF0COUNT_OFFSET,
>> +                                               req->usb_req.length);
>> +                                     udc->write_fn(udc->base_address,
>> +                                               XUSB_BUFFREADY_OFFSET, 1);
>> +                             } else {
>> +                                     xudc_wrstatus(udc);
>> +                                     req = NULL;
>> +                             }
>> +                     }
>> +             } else {
>> +                     if (ep->is_in) {
>> +                             VDBG("xudc_write_fifo called from queue\n");
>> +                             if (xudc_write_fifo(ep, req) == 1)
>> +                                     req = NULL;
>> +                     } else {
>> +                             VDBG("xudc_read_fifo called from queue\n");
>> +                             if (xudc_read_fifo(ep, req) == 1)
>> +                                     req = NULL;
>> +                     }
>> +             }
>> +     }
>
> looks like you need some refactoring here to avoid deep indentations.

Ok

>
>> +     if (req != NULL)
>> +             list_add_tail(&req->queue, &ep->queue);
>> +
>> +     spin_unlock_irqrestore(&udc->lock, flags);
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_ep_dequeue - Removes the request from the queue.
>> + * @_ep: pointer to the usb device endpoint structure.
>> + * @_req: pointer to the usb request structure.
>> + *
>> + * Return: 0 for success and error value on failure
>> + */
>> +static int xudc_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> +{
>> +     struct xusb_ep *ep;
>> +     struct xusb_req *req;
>> +     unsigned long flags;
>> +
>> +     ep = to_xusb_ep(_ep);
>> +
>> +     if (!_ep || ep->ep_usb.name == ep0name)
>> +             return -EINVAL;
>> +
>> +     spin_lock_irqsave(&ep->udc->lock, flags);
>> +     /* Make sure it's actually queued on this endpoint */
>> +     list_for_each_entry(req, &ep->queue, queue) {
>> +             if (&req->usb_req == _req)
>> +                     break;
>> +     }
>> +     if (&req->usb_req != _req) {
>> +             spin_unlock_irqrestore(&ep->udc->lock, flags);
>> +             return -EINVAL;
>> +     }
>> +
>> +     xudc_done(ep, req, -ECONNRESET);
>> +     spin_unlock_irqrestore(&ep->udc->lock, flags);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct usb_ep_ops xusb_ep_ops = {
>> +     .enable = xudc_ep_enable,
>> +     .disable = xudc_ep_disable,
>> +
>> +     .alloc_request = xudc_ep_alloc_request,
>> +     .free_request = xudc_free_request,
>> +
>> +     .queue = xudc_ep_queue,
>> +     .dequeue = xudc_ep_dequeue,
>> +     .set_halt = xudc_ep_set_halt,
>> +};
>> +
>> +/**
>> + * xudc_get_frame - Reads the current usb frame number.
>> + * @gadget: pointer to the usb gadget structure.
>> + *
>> + * Return: current frame number for success and error value on failure.
>> + */
>> +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.
I tested sending a NULL to container_of macro I see no NULL exception.
Hence using container_of
macro first and then checking for NULL input is fine. If you insist
this i need to change code at other
places too.

>
>> +     local_irq_save(flags);
>> +     retval = udc->read_fn(udc->base_address + XUSB_FRAMENUM_OFFSET);
>> +     local_irq_restore(flags);
>
> yeah I'm not a big fan of anybody messing with local_irq_*.

Ok

>
>> +     return retval;
>> +}
>> +
>> +/**
>> + * xudc_wakeup - Send remote wakeup signal to host
>> + * @gadget: pointer to the usb gadget structure.
>> + *
>> + * Return: 0 on success and error on failure
>> + */
>> +
>> +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.

>
>> +     udc->write_fn(udc->base_address, XUSB_CONTROL_OFFSET, crtlreg &
>> +                     ~XUSB_CONTROL_USB_RMTWAKE_MASK);
>> +     status = 0;
>> +done:
>> +     spin_unlock_irqrestore(&udc->lock, flags);
>> +     return status;
>> +}
>> +
>> +/**
>> + * xudc_reinit - Restores inital software state.
>> + * @udc: pointer to the usb device controller structure.
>> + */
>> +static void xudc_reinit(struct xusb_udc *udc)
>> +{
>> +     u32 ep_number;
>> +     char name[4];
>> +
>> +     INIT_LIST_HEAD(&udc->gadget.ep_list);
>> +
>> +     for (ep_number = 0; ep_number < XUSB_MAX_ENDPOINTS; ep_number++) {
>> +             struct xusb_ep *ep = &udc->ep[ep_number];
>> +
>> +             if (ep_number) {
>> +                     list_add_tail(&ep->ep_usb.ep_list,
>> +                                     &udc->gadget.ep_list);
>> +                     ep->ep_usb.maxpacket = (unsigned short)~0;
>> +                     sprintf(name, "ep%d", ep_number);
>> +                     strcpy(ep->name, name);
>> +                     ep->ep_usb.name = ep->name;
>> +             } else {
>> +                     ep->ep_usb.name = ep0name;
>> +                     ep->ep_usb.maxpacket = 0x40;
>> +             }
>> +
>> +             ep->ep_usb.ops = &xusb_ep_ops;
>> +             ep->udc = udc;
>> +             ep->epnumber = ep_number;
>> +             ep->desc = NULL;
>> +             ep->stopped = 0;
>> +             /*
>> +              * The configuration register address offset between
>> +              * each endpoint is 0x10.
>> +              */
>> +             ep->offset = XUSB_EP0_CONFIG_OFFSET +
>> +                                     (ep_number * 0x10);
>> +             ep->is_in = 0;
>> +             ep->is_iso = 0;
>> +             ep->maxpacket = 0;
>> +             xudc_epconfig(ep, udc);
>> +
>> +             /* Initialize one queue per endpoint */
>> +             INIT_LIST_HEAD(&ep->queue);
>> +     }
>> +}
>> +
>> +/**
>> + * xudc_stop_activity - Stops any further activity on the device.
>> + * @udc: pointer to the usb device controller structure.
>> + */
>> +static void xudc_stop_activity(struct xusb_udc *udc)
>> +{
>> +     int i;
>> +     struct xusb_ep *ep;
>> +
>> +     for (i = 0; i < XUSB_MAX_ENDPOINTS; i++) {
>> +             ep = &udc->ep[i];
>> +             xudc_nuke(ep, -ESHUTDOWN);
>> +     }
>> +}
>> +
>> +/**
>> + * xudc_start - Starts the device.
>> + * @gadget: pointer to the usb gadget structure
>> + * @driver: pointer to gadget driver structure
>> + *
>> + * Return: zero always
>> + */
>> +static int xudc_start(struct usb_gadget *gadget,
>> +                     struct usb_gadget_driver *driver)
>> +{
>> +     struct xusb_udc *udc = to_udc(gadget);
>> +     const struct usb_endpoint_descriptor *d = &config_bulk_out_desc;
>> +     u32 crtlreg;
>> +
>> +     driver->driver.bus = NULL;
>> +     /* hook up the driver */
>> +     udc->driver = driver;
>> +     udc->gadget.dev.driver = &driver->driver;
>> +     udc->gadget.speed = driver->max_speed;
>> +
>> +     /* Enable the control endpoint. */
>> +     xudc_ep_enable(&udc->ep[XUSB_EP_NUMBER_ZERO].ep_usb, d);
>> +     /* Set Address to zero */
>> +     udc->write_fn(udc->base_address, XUSB_ADDRESS_OFFSET, 0);
>> +     /* Start device */
>> +     crtlreg = udc->read_fn(udc->base_address + XUSB_CONTROL_OFFSET);
>> +     crtlreg |= XUSB_CONTROL_USB_READY_MASK;
>> +     udc->write_fn(udc->base_address, XUSB_CONTROL_OFFSET, crtlreg);
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * xudc_stop - stops the device.
>> + * @gadget: pointer to the usb gadget structure
>> + * @driver: pointer to usb gadget driver structure
>> + *
>> + * Return: zero always
>> + */
>> +static int xudc_stop(struct usb_gadget *gadget,
>> +             struct usb_gadget_driver *driver)
>> +{
>> +     struct xusb_udc *udc = to_udc(gadget);
>> +     unsigned long flags;
>> +     u32 crtlreg;
>> +
>> +     /* Disable USB device.*/
>> +     crtlreg = udc->read_fn(udc->base_address + XUSB_CONTROL_OFFSET);
>> +     crtlreg &= ~XUSB_CONTROL_USB_READY_MASK;
>> +     udc->write_fn(udc->base_address, XUSB_CONTROL_OFFSET, crtlreg);
>> +     spin_lock_irqsave(&udc->lock, flags);
>> +     udc->gadget.speed = USB_SPEED_UNKNOWN;
>> +     xudc_stop_activity(udc);
>> +     spin_unlock_irqrestore(&udc->lock, flags);
>> +
>> +     udc->gadget.dev.driver = NULL;
>> +     udc->driver = NULL;
>> +
>> +     return 0;
>> +}
>> +
>> +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.

>
>> +static void xudc_startup_handler(struct xusb_udc *udc, u32 intrstatus)
>> +{
>> +     u32 intrreg;
>> +
>> +     if (intrstatus & XUSB_STATUS_RESET_MASK) {
>> +
>> +             DBG("Reset\n");
>> +
>> +             if (intrstatus & XUSB_STATUS_HIGH_SPEED_MASK)
>> +                     udc->gadget.speed = USB_SPEED_HIGH;
>> +             else
>> +                     udc->gadget.speed = USB_SPEED_FULL;
>> +
>> +             /* Set device address and remote wakeup to 0 */
>> +             udc->write_fn(udc->base_address, XUSB_ADDRESS_OFFSET, 0);
>> +             udc->remote_wkp = 0;
>> +
>> +             /* Enable the suspend and resume */
>> +             intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
>> +             intrreg |= XUSB_STATUS_SUSPEND_MASK | XUSB_STATUS_RESUME_MASK;
>> +             udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
>> +
>> +             xudc_stop_activity(udc);
>
> you need to conditionally call gadget_driver->disconnect(), you also
> need to make sure test modes are cleared, clear all stalls, etc.

Ok . before udc_stop is called gadget layer calls disconnect. I will
add driver->disconnect
when device is disconnected from host.

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

>
>> +static void xudc_getstatus(struct xusb_udc *udc)
>> +{
>> +     u16 status = 0;
>> +     u32 epcfgreg;
>> +     struct xusb_ep *ep0;
>> +     int epnum;
>> +     struct xusb_ep *ep;
>> +     u32 halt;
>> +     u32 *corebuf;
>> +
>> +     ep0 = &udc->ep[XUSB_EP_NUMBER_ZERO];
>> +     switch (udc->ch9cmd.setup.bRequestType & USB_RECIP_MASK) {
>> +     case USB_RECIP_DEVICE:
>> +             /* Get device status */
>> +             status = 1 << USB_DEVICE_SELF_POWERED;
>> +             if (udc->remote_wkp)
>> +                     status |= (1 << USB_DEVICE_REMOTE_WAKEUP);
>> +             break;
>> +     case USB_RECIP_INTERFACE:
>> +             break;
>> +     case USB_RECIP_ENDPOINT:
>> +             epnum = udc->ch9cmd.setup.wIndex & USB_ENDPOINT_NUMBER_MASK;
>> +             ep = &udc->ep[epnum];
>> +             epcfgreg = udc->read_fn(udc->base_address +
>> +                             udc->ep[epnum].offset);
>> +             halt = epcfgreg & XUSB_EP_CFG_STALL_MASK;
>> +             if (udc->ch9cmd.setup.wIndex & USB_DIR_IN) {
>> +                     if (!ep->is_in)
>> +                             goto stall;
>> +             } else {
>> +                     if (ep->is_in)
>> +                             goto stall;
>> +             }
>> +             if (halt)
>> +                     status = 1 << USB_ENDPOINT_HALT;
>> +             break;
>> +     default:
>> +             goto stall;
>> +     }
>> +
>> +     udc->ch9cmd.writecount = 0;
>> +     corebuf = (void __force *) ((ep0->rambase << 2) + udc->base_address);
>> +     status = cpu_to_le16(status);
>> +     memcpy((void *)corebuf, (void *)&status, 2);
>> +     udc->write_fn(udc->base_address, XUSB_EP_BUF0COUNT_OFFSET, 2);
>> +     udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET, 1);
>> +     return;
>> +stall:
>> +     dev_err(&udc->gadget.dev, "Can't respond to getstatus request\n");
>> +     xudc_ep0_stall(udc);
>> +     return;
>> +}
>> +
>> +/**
>> + * xudc_set_clear_feature - Executes the set feature and clear feature commands.
>> + * @udc: pointer to the usb device controller structure.
>> + *
>> + * Processes the SET_FEATURE and CLEAR_FEATURE commands.
>> + */
>> +static void xudc_set_clear_feature(struct xusb_udc *udc)
>> +{
>> +     u8 endpoint;
>> +     u8 outinbit;
>> +     u32 epcfgreg;
>> +     int flag = (udc->ch9cmd.setup.bRequest == USB_REQ_SET_FEATURE ? 1 : 0);
>> +     switch (udc->ch9cmd.setup.bRequestType) {
>> +     case USB_RECIP_DEVICE:
>> +             switch (udc->ch9cmd.setup.wValue) {
>> +             case USB_DEVICE_TEST_MODE:
>> +                     /*
>> +                      * The Test Mode will be executed
>> +                      * after the status phase.
>> +                      */
>> +                     break;
>> +             case USB_DEVICE_REMOTE_WAKEUP:
>> +                     if (flag)
>> +                             udc->remote_wkp = 1;
>> +                     else
>> +                             udc->remote_wkp = 0;
>> +                     break;
>> +             default:
>> +                     xudc_ep0_stall(udc);
>> +                     break;
>> +             }
>> +             break;
>> +     case USB_RECIP_ENDPOINT:
>> +             if (!udc->ch9cmd.setup.wValue) {
>> +                     endpoint = udc->ch9cmd.setup.wIndex &
>> +                                     USB_ENDPOINT_NUMBER_MASK;
>> +                     outinbit = udc->ch9cmd.setup.wIndex &
>> +                                     USB_ENDPOINT_DIR_MASK;
>> +                     outinbit = outinbit >> 7;
>> +
>> +                     /* Make sure direction matches.*/
>> +                     if (outinbit != udc->ep[endpoint].is_in) {
>> +                             xudc_ep0_stall(udc);
>> +                             return;
>> +                     }
>> +                     epcfgreg = udc->read_fn(udc->base_address +
>> +                                             udc->ep[endpoint].
>> +                                             offset);
>> +                     if (!endpoint) {
>> +                             /* Clear the stall.*/
>> +                             epcfgreg &= ~XUSB_EP_CFG_STALL_MASK;
>> +                             udc->write_fn(udc->base_address,
>> +                                             udc->ep[endpoint].offset,
>> +                                             epcfgreg);
>> +                     } else {
>> +                             if (flag) {
>> +                                     epcfgreg |= XUSB_EP_CFG_STALL_MASK;
>> +                                     udc->write_fn(udc->base_address,
>> +                                                     udc->ep[endpoint].
>> +                                                     offset, epcfgreg);
>> +                             } else {
>> +                                     /* Unstall the endpoint.*/
>> +                                     epcfgreg &= ~(XUSB_EP_CFG_STALL_MASK |
>> +                                               XUSB_EP_CFG_DATA_TOGGLE_MASK);
>> +                                     udc->write_fn(udc->base_address,
>> +                                                     udc->ep[endpoint].
>> +                                                     offset, epcfgreg);
>> +                             }
>> +                     }
>> +             }
>> +             break;
>> +     default:
>> +             xudc_ep0_stall(udc);
>> +             return;
>> +     }
>> +     /* Cause and valid status phase to be issued.*/
>> +     xudc_wrstatus(udc);
>> +     return;
>> +}
>> +
>> +/**
>> + * xudc_reset_ep0 - reset ep0 request
>> + * @ep0: pointer to the endpoint structure.
>> + *
>> + * Resets endpoint 0 request.
>> + */
>> +static void xudc_reset_ep0(struct xusb_ep *ep0)
>> +{
>> +     ep0->no_queue = 0;
>> +     xudc_nuke(ep0, -ECONNRESET);
>> +}
>> +
>> +/**
>> + * xudc_handle_setup - Processes the setup packet.
>> + * @udc: pointer to the usb device controller structure.
>> + * @setup: pointer to the usb control request structure.
>> + *
>> + * Process setup packet and delegate to gadget layer.
>> + */
>> +static void xudc_handle_setup(struct xusb_udc *udc,
>> +                             struct usb_ctrlrequest *setup)
>> +{
>> +     u32 *ep0rambase;
>> +     struct xusb_ep *ep0 = &udc->ep[XUSB_EP_NUMBER_ZERO];
>> +
>> +     /* Load up the chapter 9 command buffer.*/
>> +     ep0rambase = (u32 __force *) (udc->base_address +
>> +                               XUSB_SETUP_PKT_ADDR_OFFSET);
>> +     memcpy((void *)&udc->ch9cmd.setup, (void *)ep0rambase, 8);
>> +
>> +     setup->bRequestType = udc->ch9cmd.setup.bRequestType;
>> +     setup->bRequest     = udc->ch9cmd.setup.bRequest;
>> +     setup->wValue       = udc->ch9cmd.setup.wValue;
>> +     setup->wIndex       = udc->ch9cmd.setup.wIndex;
>> +     setup->wLength      = udc->ch9cmd.setup.wLength;
>> +
>> +     udc->ch9cmd.setup.wValue = cpu_to_le16(udc->ch9cmd.setup.wValue);
>> +     udc->ch9cmd.setup.wIndex = cpu_to_le16(udc->ch9cmd.setup.wIndex);
>> +     udc->ch9cmd.setup.wLength = cpu_to_le16(udc->ch9cmd.setup.wLength);
>> +
>> +     /* Restore ReadPtr to data buffer.*/
>> +     udc->ch9cmd.readptr = &udc->ch9cmd.readbuff[0];
>> +     udc->ch9cmd.readcount = 0;
>> +
>> +     if (udc->ch9cmd.setup.bRequestType & USB_DIR_IN) {
>> +             /* Execute the get command.*/
>> +             udc->ch9cmd.setupseqrx = STATUS_PHASE;
>> +             udc->ch9cmd.setupseqtx = DATA_PHASE;
>> +     } else {
>> +             /* Execute the put command.*/
>> +             udc->ch9cmd.setupseqrx = DATA_PHASE;
>> +             udc->ch9cmd.setupseqtx = STATUS_PHASE;
>> +     }
>> +
>> +     xudc_reset_ep0(ep0);
>> +
>> +     switch (udc->ch9cmd.setup.bRequest) {
>> +     case USB_REQ_GET_STATUS:
>> +             /* Data+Status phase form udc */
>> +             if ((udc->ch9cmd.setup.bRequestType &
>> +                             (USB_DIR_IN | USB_TYPE_MASK)) !=
>> +                             (USB_DIR_IN | USB_TYPE_STANDARD))
>> +                     break;
>> +             ep0->no_queue = 1;
>> +             xudc_getstatus(udc);
>> +             return;
>> +     case USB_REQ_SET_ADDRESS:
>> +             /* Status phase from udc */
>> +             if (udc->ch9cmd.setup.bRequestType != (USB_DIR_OUT |
>> +                             USB_TYPE_STANDARD | USB_RECIP_DEVICE))
>> +                     break;
>> +             ep0->no_queue = 1;
>> +             xudc_wrstatus(udc);
>> +             return;
>> +     case USB_REQ_CLEAR_FEATURE:
>> +     case USB_REQ_SET_FEATURE:
>> +             /* Requests with no data phase, status phase from udc */
>> +             if ((udc->ch9cmd.setup.bRequestType & USB_TYPE_MASK)
>> +                             != USB_TYPE_STANDARD)
>> +                     break;
>> +             ep0->no_queue = 1;
>> +             xudc_set_clear_feature(udc);
>> +             return;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     spin_unlock(&udc->lock);
>> +     if (udc->driver->setup(&udc->gadget, setup) < 0)
>> +             xudc_ep0_stall(udc);
>> +     spin_lock(&udc->lock);
>> +}
>> +
>> +/**
>> + * xudc_ep0_out - Processes the endpoint 0 OUT token.
>> + * @udc: pointer to the usb device controller structure.
>> + */
>> +static void xudc_ep0_out(struct xusb_udc *udc)
>> +{
>> +     struct xusb_ep *ep;
>> +     u8 count;
>> +     u8 *ep0rambase;
>> +     u16 index;
>> +
>> +     ep = &udc->ep[0];
>> +     switch (udc->ch9cmd.setupseqrx) {
>> +     case STATUS_PHASE:
>> +             /*
>> +              * This resets both state machines for the next
>> +              * Setup packet.
>> +              */
>> +             udc->ch9cmd.setupseqrx = SETUP_PHASE;
>> +             udc->ch9cmd.setupseqtx = SETUP_PHASE;
>> +             if (!ep->no_queue) {
>> +                     ep->data->usb_req.actual = ep->data->usb_req.length;
>> +                     xudc_done(ep, ep->data, 0);
>> +             }
>> +             ep->no_queue = 0;
>> +             break;
>> +     case DATA_PHASE:
>> +             count = udc->read_fn(udc->base_address +
>> +                             XUSB_EP_BUF0COUNT_OFFSET);
>> +             /* Copy the data to be received from the DPRAM. */
>> +             ep0rambase =
>> +                     (u8 __force *) (udc->base_address +
>> +                             (udc->ep[XUSB_EP_NUMBER_ZERO].rambase << 2));
>> +
>> +             for (index = 0; index < count; index++)
>> +                     *udc->ch9cmd.readptr++ = *ep0rambase++;
>> +
>> +             udc->ch9cmd.readcount += count;
>> +             if (udc->ch9cmd.setup.wLength == udc->ch9cmd.readcount) {
>> +                     xudc_wrstatus(udc);
>> +             } else {
>> +                     /* Set the Tx packet size and the Tx enable bit.*/
>> +                     udc->write_fn(udc->base_address,
>> +                                     XUSB_EP_BUF0COUNT_OFFSET, 0);
>> +                     udc->write_fn(udc->base_address,
>> +                                     XUSB_BUFFREADY_OFFSET, 1);
>> +             }
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +}
>> +
>> +/**
>> + * xudc_ep0_in - Processes the endpoint 0 IN token.
>> + * @udc: pointer to the usb device controller structure.
>> + */
>> +static void xudc_ep0_in(struct xusb_udc *udc)
>> +{
>> +     struct xusb_ep *ep;
>> +     u32 epcfgreg;
>> +     u16 count;
>> +     u16 length;
>> +     u8 *ep0rambase;
>> +
>> +     ep = &udc->ep[0];
>> +     switch (udc->ch9cmd.setupseqtx) {
>> +     case STATUS_PHASE:
>> +             switch (udc->ch9cmd.setup.bRequest) {
>> +             case USB_REQ_SET_ADDRESS:
>> +                     /* Set the address of the device.*/
>> +                     udc->write_fn(udc->base_address,
>> +                                     XUSB_ADDRESS_OFFSET,
>> +                                     udc->ch9cmd.setup.wValue);
>> +                     break;
>> +             case USB_REQ_SET_FEATURE:
>> +                     if (udc->ch9cmd.setup.bRequestType ==
>> +                                     USB_RECIP_DEVICE) {
>> +                             if (udc->ch9cmd.setup.wValue ==
>> +                                             USB_DEVICE_TEST_MODE)
>> +                                     udc->write_fn(udc->base_address,
>> +                                                     XUSB_TESTMODE_OFFSET,
>> +                                                     TEST_J);
>> +                     }
>> +                     break;
>> +             }
>> +             if (!ep->no_queue) {
>> +                     ep->data->usb_req.actual = udc->ch9cmd.setup.wLength;
>> +                     xudc_done(ep, ep->data, 0);
>> +             }
>> +             ep->no_queue = 0;
>> +             break;
>> +     case DATA_PHASE:
>> +             if (!udc->ch9cmd.writecount) {
>> +                     /*
>> +                      * We're done with data transfer, next
>> +                      * will be zero length OUT with data toggle of
>> +                      * 1. Setup data_toggle.
>> +                      */
>> +                     epcfgreg = udc->read_fn(udc->base_address +
>> +                             udc->ep[XUSB_EP_NUMBER_ZERO].offset);
>> +                     epcfgreg |= XUSB_EP_CFG_DATA_TOGGLE_MASK;
>> +                     udc->write_fn(udc->base_address,
>> +                             udc->ep[XUSB_EP_NUMBER_ZERO].offset, epcfgreg);
>> +                     count = 0;
>> +                     udc->ch9cmd.setupseqtx = STATUS_PHASE;
>> +             } else {
>> +                     length = count = min_t(u32, udc->ch9cmd.writecount,
>> +                                             EP0_MAX_PACKET);
>> +                     /* Copy the data to be transmitted into the DPRAM. */
>> +                     ep0rambase = (u8 __force *) (udc->base_address +
>> +                             (udc->ep[XUSB_EP_NUMBER_ZERO].rambase << 2));
>> +                     while (length--)
>> +                             *ep0rambase++ = *udc->ch9cmd.wrtptr++;
>> +
>> +                     udc->ch9cmd.writecount -= count;
>> +             }
>> +             udc->write_fn(udc->base_address, XUSB_EP_BUF0COUNT_OFFSET,
>> +                             count);
>> +             udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET, 1);
>> +             break;
>> +     default:
>> +             break;
>> +     }
>> +}
>> +
>> +/**
>> + * xudc_ctrl_ep_handler - Endpoint 0 interrupt handler.
>> + * @udc: pointer to the udc structure.
>> + * @intrstatus:      It's the mask value for the interrupt sources on endpoint 0.
>> + *
>> + * Processes the commands received during enumeration phase.
>> + */
>> +static void xudc_ctrl_ep_handler(struct xusb_udc *udc, u32 intrstatus)
>> +{
>> +     struct usb_ctrlrequest ctrl;
>> +
>> +     if (intrstatus & XUSB_STATUS_SETUP_PACKET_MASK) {
>> +             xudc_handle_setup(udc, &ctrl);
>> +     } else {
>> +             if (intrstatus & XUSB_STATUS_FIFO_BUFF_RDY_MASK)
>> +                     xudc_ep0_out(udc);
>> +             else if (intrstatus & XUSB_STATUS_FIFO_BUFF_FREE_MASK)
>> +                     xudc_ep0_in(udc);
>> +     }
>> +}
>> +
>> +/**
>> + * xudc_nonctrl_ep_handler - Non control endpoint interrupt handler.
>> + * @udc: pointer to the udc structure.
>> + * @epnum: End point number for which the interrupt is to be processed
>> + * @intrstatus:      mask value for interrupt sources of endpoints other
>> + *           than endpoint 0.
>> + *
>> + * Processes the buffer completion interrupts.
>> + */
>> +static void xudc_nonctrl_ep_handler(struct xusb_udc *udc, u8 epnum,
>> +                                     u32 intrstatus)
>> +{
>> +
>> +     struct xusb_req *req;
>> +     struct xusb_ep *ep;
>> +
>> +     ep = &udc->ep[epnum];
>> +     /* Process the End point interrupts.*/
>> +     if (intrstatus & (XUSB_STATUS_EP0_BUFF1_COMP_MASK << epnum))
>> +             ep->buffer0ready = 0;
>> +     if (intrstatus & (XUSB_STATUS_EP0_BUFF2_COMP_MASK << epnum))
>> +             ep->buffer1ready = 0;
>> +
>> +     if (list_empty(&ep->queue))
>> +             req = NULL;
>> +     else
>> +             req = list_entry(ep->queue.next, struct xusb_req, queue);
>> +     if (!req)
>> +             return;
>> +
>> +     if (ep->is_in)
>> +             xudc_write_fifo(ep, req);
>> +     else
>> +             xudc_read_fifo(ep, req);
>> +}
>> +
>> +/**
>> + * xudc_irq - The main interrupt handler.
>> + * @irq: The interrupt number.
>> + * @_udc: pointer to the usb device controller structure.
>> + *
>> + * Return: IRQ_HANDLED after the interrupt is handled.
>> + */
>> +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);

Here. calling driver->resume.

>> +     }
>> +
>> +     /* Call the handler for the event interrupt.*/
>> +     if (intrstatus & XUSB_STATUS_INTR_EVENT_MASK) {
>> +             /*
>> +              * Check if there is any action to be done for :
>> +              * - USB Reset received {XUSB_STATUS_RESET_MASK}
>> +              * - USB Suspend received {XUSB_STATUS_SUSPEND_MASK}
>> +              */
>> +             xudc_startup_handler(udc, intrstatus);
>> +     }
>> +
>> +     /* Check the buffer completion interrupts */
>> +     if (intrstatus & XUSB_STATUS_INTR_BUFF_COMP_ALL_MASK) {
>> +             /* Enable Reset and Suspend */
>> +             intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
>> +             intrreg |= XUSB_STATUS_SUSPEND_MASK | XUSB_STATUS_RESET_MASK;
>> +             udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
>> +
>> +             if (intrstatus & XUSB_STATUS_EP0_BUFF1_COMP_MASK)
>> +                     xudc_ctrl_ep_handler(udc, intrstatus);
>> +
>> +             for (index = 1; index < 8; index++) {
>> +                     bufintr = ((intrstatus &
>> +                                     (XUSB_STATUS_EP1_BUFF1_COMP_MASK <<
>> +                                                     (index - 1))) ||
>> +                                (intrstatus &
>> +                                     (XUSB_STATUS_EP1_BUFF2_COMP_MASK <<
>> +                                                     (index - 1))));
>> +                     if (bufintr)
>> +                             xudc_nonctrl_ep_handler(udc, index,
>> +                                             intrstatus);
>> +             }
>> +     }
>> +     spin_unlock(&(udc->lock));
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * xudc_probe - The device probe function for driver initialization.
>> + * @pdev: pointer to the platform device structure.
>> + *
>> + * Return: 0 for success and error value on failure
>> + */
>> +static int xudc_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct resource *res;
>> +     struct xusb_udc *udc;
>> +     int irq;
>> +     int ret;
>> +
>> +     udc = devm_kzalloc(&pdev->dev, sizeof(*udc), GFP_KERNEL);
>> +     if (!udc)
>> +             return -ENOMEM;
>> +
>> +     /* Map the registers */
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     udc->base_address = devm_ioremap_resource(&pdev->dev, res);
>> +     if (!udc->base_address)
>> +             return -ENOMEM;
>> +
>> +     irq = platform_get_irq(pdev, 0);
>> +     if (irq < 0) {
>> +             dev_err(&pdev->dev, "unable to get irq\n");
>> +             return irq;
>> +     }
>> +     ret = devm_request_irq(&pdev->dev, irq, xudc_irq, 0,
>> +                             dev_name(&pdev->dev), udc);
>> +     if (ret < 0) {
>> +             dev_dbg(&pdev->dev, "unable to request irq %d", irq);
>> +             goto fail;
>> +     }
>> +
>> +     udc->dma_enabled = of_property_read_bool(np, "xlnx,has-builtin-dma");
>> +
>> +     /* Setup gadget structure */
>> +     udc->gadget.ops = &xusb_udc_ops;
>> +     udc->gadget.max_speed = USB_SPEED_HIGH;
>> +     udc->gadget.speed = USB_SPEED_UNKNOWN;
>> +     udc->gadget.ep0 = &udc->ep[XUSB_EP_NUMBER_ZERO].ep_usb;
>> +     udc->gadget.name = driver_name;
>> +
>> +     dev_set_name(&udc->gadget.dev, "xilinx_udc");
>
> this line isn't necessary.

Ok

>
>> +     spin_lock_init(&udc->lock);
>> +
>> +     /* Check for IP endianness */
>> +     udc->write_fn = xudc_write32_be;
>> +     udc->read_fn = xudc_read32_be;
>> +     udc->write_fn(udc->base_address, XUSB_TESTMODE_OFFSET, TEST_J);
>> +     if ((udc->read_fn(udc->base_address + XUSB_TESTMODE_OFFSET))
>> +                     != TEST_J) {
>> +             udc->write_fn = xudc_write32;
>> +             udc->read_fn = xudc_read32;
>> +     }
>
> hmm... isn't there a configuration register to check this out ?

No. There is no config register for checking endainess.

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

>
>> +                     XUSB_STATUS_FIFO_BUFF_RDY_MASK |
>> +                     XUSB_STATUS_FIFO_BUFF_FREE_MASK |
>> +                     XUSB_STATUS_EP0_BUFF1_COMP_MASK);
>> +
>> +     platform_set_drvdata(pdev, udc);
>> +
>> +     VDBG("%s #%d at 0x%08X mapped to 0x%08X\n", driver_name, 0,
>> +             (u32)res->start, (u32 __force)udc->base_address);
>> +
>> +     return 0;
>> +
>> +fail:
>> +     dev_err(&pdev->dev, "probe failed, %d\n", ret);
>> +     return ret;
>> +}
>> +
>> +/**
>> + * xudc_remove - Releases the resources allocated during the initialization.
>> + * @pdev: pointer to the platform device structure.
>> + *
>> + * Return: 0 always
>> + */
>> +static int xudc_remove(struct platform_device *pdev)
>> +{
>> +     struct xusb_udc *udc = platform_get_drvdata(pdev);
>> +
>> +     dev_dbg(&pdev->dev, "remove\n");
>
> this is *really* unnecessary.

Ok. will remove.

Thanks,
Sundeep.B.S.

>
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ