Re: [PATCH] USB device driver of Topcliff PCH

[Maurus Cuelenaere]

2010/9/7 Masayuki Ohtake <masa-korg@xxxxxxxxxxxxxxx>
>
> This patch adds the USB device driver of Topcliff PCH.
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus.
> Topcliff PCH has USB device I/F. Using this I/F, it is able to access system
> devices connected to USB device.
>
> Signed-off-by: Masayuki Ohtake <masa-korg@xxxxxxxxxxxxxxx>

No need to mail all these people, just the maintainers and appropriate mailing
lists will do.

> ---
> Âdrivers/usb/gadget/Kconfig    Â|  24 +
> Âdrivers/usb/gadget/Makefile    |  Â2 +-
> Âdrivers/usb/gadget/gadget_chips.h | Â Â7 +
> Âdrivers/usb/gadget/pch_udc.c   Â| 3153 +++++++++++++++++++++++++++++++++++++
> Âdrivers/usb/gadget/pch_udc.h   Â| Â495 ++++++
> Â5 files changed, 3680 insertions(+), 1 deletions(-)
> Âcreate mode 100755 drivers/usb/gadget/pch_udc.c
> Âcreate mode 100755 drivers/usb/gadget/pch_udc.h
>
[snip]
> diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
> new file mode 100755
> index 0000000..2065c11
> --- /dev/null
> +++ b/drivers/usb/gadget/pch_udc.c
[snip]
> +
> +const char   ep0_string[] = "ep0in";
> +static DEFINE_SPINLOCK(udc_stall_spinlock); Â Â/* stall spin lock */
> +static u32 pch_udc_base;
> +static union pch_udc_setup_data setup_data; Â Â/* received setup data */
> +static unsigned long ep0out_buf[64];
> +static dma_addr_t dma_addr;
> +struct pch_udc_dev *pch_udc; Â Â Â Â Â /* pointer to device object */
> +int speed_fs;

I'd put all these in struct phc_udc_dev or similar, so you could have multiple
controllers.

> +
> +module_param_named(speed_fs, speed_fs, bool, S_IRUGO);
> +MODULE_PARM_DESC(speed_fs, "true for Full speed operation");
> +MODULE_DESCRIPTION("OKISEMI PCH UDC - USB Device Controller");
> +MODULE_LICENSE("GPL");
> +
> +/**
> + * pch_udc_write_csr - Write the command and status registers.
> + * @val: Â Â Â value to be written to CSR register
> + * @addr: Â Â Âaddress of CSR register
> + */
> +inline void pch_udc_write_csr(unsigned long val, unsigned long addr)

Make all these functions static.

> +{
> + Â Â Â int count = MAX_LOOP;
> +
> + Â Â Â /* Wait till idle */
> + Â Â Â while ((count > 0) &&\
> + Â Â Â Â Â Â Â (ioread32((u32 *)(PCH_UDC_CSR_BUSY_ADDR + pch_udc_base)) &
> + Â Â Â Â Â Â Â PCH_UDC_CSR_BUSY))

Why not define PCH_UDC_* as (void __iomem*) so no casting is necessary.

> + Â Â Â Â Â Â Â count--;
> +
> + Â Â Â if (count < 0)
> + Â Â Â Â Â Â Â pr_debug("%s: wait error; count = %x", __func__, count);
> +
> + Â Â Â iowrite32(val, (u32 *)addr);
> + Â Â Â /* Wait till idle */
> + Â Â Â count = MAX_LOOP;
> + Â Â Â while ((count > 0) &&
> + Â Â Â Â Â Â Â (ioread32((u32 *)(PCH_UDC_CSR_BUSY_ADDR + pch_udc_base)) &
> + Â Â Â Â Â Â Â PCH_UDC_CSR_BUSY))
> + Â Â Â Â Â Â Â count--;
> +
> + Â Â Â if (count < 0)
> + Â Â Â Â Â Â Â pr_debug("%s: wait error; count = %x", __func__, count);
> +
> +}
> +
> +/**
> + * pch_udc_read_csr - Read the command and status registers.
> + * @addr: Â Â Âaddress of CSR register
> + * Returns
> + * Â Â content of CSR register
> + */
> +inline u32 pch_udc_read_csr(unsigned long addr)

void __iomem *addr

> +{
> + Â Â Â int count = MAX_LOOP;
> +
> + Â Â Â /* Wait till idle */
> + Â Â Â while ((count > 0) &&
> + Â Â Â Â Â Â Â (ioread32((u32 *)(PCH_UDC_CSR_BUSY_ADDR + pch_udc_base)) &
> + Â Â Â Â Â Â Â PCH_UDC_CSR_BUSY))
> + Â Â Â Â Â Â Â count--;
> +
> + Â Â Â if (count < 0)
> + Â Â Â Â Â Â Â pr_debug("%s: wait error; count = %x", __func__, count);
> + Â Â Â /* Dummy read */
> + Â Â Â ioread32((u32 *)addr);
> + Â Â Â count = MAX_LOOP;
> + Â Â Â /* Wait till idle */
> + Â Â Â while ((count > 0) &&
> + Â Â Â Â Â Â Â (ioread32((u32 *)(PCH_UDC_CSR_BUSY_ADDR + pch_udc_base)) &
> + Â Â Â Â Â Â Â PCH_UDC_CSR_BUSY))
> + Â Â Â Â Â Â Â count--;
> + Â Â Â /* actual read */
> + Â Â Â if (count < 0)
> + Â Â Â Â Â Â Â pr_debug("%s: wait error; count = %x", __func__, count);
> +
> + Â Â Â return ioread32((u32 *)addr);
> +}
> +
> +/**
> + * pch_udc_rmt_wakeup - Initiate for remote wakeup
> + * @dev: Â Â Â Reference to pch_udc_regs structure
> + */
> +inline void pch_udc_rmt_wakeup(struct pch_udc_regs *dev)
> +{
> + Â Â Â PCH_UDC_BIT_SET(&dev->devctl, 1 << UDC_DEVCTL_RES);
> + Â Â Â mdelay(1);
> + Â Â Â PCH_UDC_BIT_CLR(&dev->devctl, 1 << UDC_DEVCTL_RES);
> +}
> +
> +/**
> + * pch_udc_get_frame - Get the current frame from device status register
> + * @dev: Â Â Â Reference to pch_udc_regs structure
> + * Retern   Âcurrent frame
> + */
> +inline int pch_udc_get_frame(struct pch_udc_regs *dev)
> +{
> + Â Â Â u32 frame;
> +
> + Â Â Â frame = ioread32(&dev->devsts);

Why not just get rid of struct pch_udc_regs and do something like:

static inline u32 pch_readl(struct pch_udc_dev *dev, unsigned long reg)
{
	return ioread32(dev->base_addr + reg);
}

(and similar for pch_writel)

> + Â Â Â return (frame & UDC_DEVSTS_TS_MASK) >> UDC_DEVSTS_TS_OFS;
> +}
[snip]
> +static struct usb_request *pch_udc_alloc_request(struct usb_ep *usbep,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âgfp_t gfp)
> +{
> + Â Â Â struct pch_udc_request Â*req;
> +    struct pch_udc_ep        *ep;
> +
> + Â Â Â pr_debug("%s: enter", __func__);
> + Â Â Â if (usbep == NULL)
> + Â Â Â Â Â Â Â return NULL;
> +
> + Â Â Â ep = container_of(usbep, struct pch_udc_ep, ep);
> + Â Â Â pr_debug("%s: ep %s", __func__, usbep->name);
> + Â Â Â req = kzalloc(sizeof(struct pch_udc_request), gfp);
> + Â Â Â if (req == NULL) {
> + Â Â Â Â Â Â Â pr_debug("%s: no memory for request", __func__);
> + Â Â Â Â Â Â Â return NULL;
> + Â Â Â }
> + Â Â Â memset(req, 0, sizeof(struct pch_udc_request));

kzalloc does this..

> + Â Â Â req->req.dma = DMA_ADDR_INVALID;
> + Â Â Â INIT_LIST_HEAD(&req->queue);
> +
> + Â Â Â if (ep->dma != NULL) {
> +        struct pch_udc_data_dma_desc  Â*dma_desc;
> +
> + Â Â Â Â Â Â Â /* ep0 in requests are allocated from data pool here */
> + Â Â Â Â Â Â Â dma_desc = pci_pool_alloc(ep->dev->data_requests, gfp,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&req->td_data_phys);
> + Â Â Â Â Â Â Â if (NULL == dma_desc) {
> + Â Â Â Â Â Â Â Â Â Â Â kfree(req);
> + Â Â Â Â Â Â Â Â Â Â Â return NULL;
> + Â Â Â Â Â Â Â }
> +
> + Â Â Â Â Â Â Â pr_debug("%s: req = 0x%p dma_desc = 0x%p, "
> + Â Â Â Â Â Â Â Â Â Â Â "td_phys = 0x%08lx", __func__,
> + Â Â Â Â Â Â Â Â Â Â Â req, dma_desc, (unsigned long)req->td_data_phys);
> +
> + Â Â Â Â Â Â Â /* prevent from using desc. - set HOST BUSY */
> + Â Â Â Â Â Â Â dma_desc->status |= PCH_UDC_BS_HST_BSY;
> + Â Â Â Â Â Â Â dma_desc->dataptr = __constant_cpu_to_le32(DMA_ADDR_INVALID);
> + Â Â Â Â Â Â Â req->td_data = dma_desc;
> + Â Â Â Â Â Â Â req->td_data_last = dma_desc;
> + Â Â Â Â Â Â Â req->chain_len = 1;
> + Â Â Â }
> + Â Â Â return &req->req;
> +}
[snip]
> +
> +/**
> + * pch_udc_start_next_txrequest - This function starts
> + * Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â the next transmission requirement
> + * @ep: Â Â Â ÂReference to the endpoint structure
> + */
> +static void pch_udc_start_next_txrequest(struct pch_udc_ep *ep)
> +{
> + Â Â Â struct pch_udc_request *req;
> +
> + Â Â Â pr_debug("%s: enter", __func__);
> + Â Â Â if (pch_udc_read_ep_control(ep->regs) & (1 << UDC_EPCTL_P))
> + Â Â Â Â Â Â Â return;
> +
> + Â Â Â if (!list_empty(&ep->queue)) {

Convert this into:
if (list_empty(&ep->queue))
	return;

That way you get rid of the unnecessary spacing below

> + Â Â Â Â Â Â Â /* next request */
> + Â Â Â Â Â Â Â req = list_entry(ep->queue.next, struct pch_udc_request, queue);
> + Â Â Â Â Â Â Â if (req && !req->dma_going) {
> + Â Â Â Â Â Â Â Â Â Â Â pr_debug("%s: Set request: req=%p req->td_data=%p",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __func__, req, req->td_data);
> + Â Â Â Â Â Â Â Â Â Â Â if (req->td_data) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct pch_udc_data_dma_desc *td_data;
> +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â while (pch_udc_read_ep_control(ep->regs) &
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(1 << UDC_EPCTL_S))
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â udelay(100);
> +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â req->dma_going = 1;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Clear the descriptor pointer */
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_ep_set_ddptr(ep->regs, 0);
> +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â td_data = req->td_data;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â while (1) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â td_data->status = (td_data->status &
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â~PCH_UDC_BUFF_STS) |
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_BS_HST_RDY;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if ((td_data->status &
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_DMA_LAST) ==
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_DMA_LAST)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â td_data =
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (struct pch_udc_data_dma_desc *)\
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â phys_to_virt(td_data->next);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Write the descriptor pointer */
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_ep_set_ddptr(ep->regs,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âreq->td_data_phys);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_set_dma(ep->dev->regs, DMA_DIR_TX);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Set the poll demand bit */
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_ep_set_pd(ep->regs);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_enable_ep_interrupts(ep->dev->regs,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 1 << (ep->in ? ep->num :\
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âep->num + UDC_EPINT_OUT_EP0));
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_ep_clear_nak(ep->regs);
> + Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â }
> + Â Â Â }
> +}
> +
> +/**
> + * pch_udc_complete_transfer - This function completes a transfer
> + * @ep: Â Â Â Â Â Â Â ÂReference to the endpoint structure
> + */
> +static void pch_udc_complete_transfer(struct pch_udc_ep    Â*ep)
> +{
> + Â Â Â struct pch_udc_request *req;
> +
> + Â Â Â pr_debug("%s: enter", __func__);
> + Â Â Â if (!list_empty(&ep->queue)) {

Same here

> + Â Â Â Â Â Â Â pr_debug("%s: list_entry", __func__);
> + Â Â Â Â Â Â Â req = list_entry(ep->queue.next, struct pch_udc_request, queue);
> + Â Â Â Â Â Â Â if (req && ((req->td_data_last->status & PCH_UDC_BUFF_STS) ==
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_BS_DMA_DONE)) {
> +#ifdef DMA_PPB_WITH_DESC_UPDATE
> + Â Â Â Â Â Â Â Â Â Â Â struct pch_udc_data_dma_desc *td_data = req->td_data;
> + Â Â Â Â Â Â Â Â Â Â Â for (i = 0; i < req->chain_len; i++) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if ((td_data->status & PCH_UDC_RXTX_STS) !=
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_RTS_SUCC) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pr_err("Invalid RXTX status (0x%08x)"
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â" epstatus=0x%08x\n",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(td_data->status &
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â PCH_UDC_RXTX_STS),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(int)(ep->epsts));
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â td_data = (struct pch_udc_data_dma_desc *)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âphys_to_virt(td_data->next);
> + Â Â Â Â Â Â Â Â Â Â Â }
> +#else
> + Â Â Â Â Â Â Â Â Â Â Â if ((req->td_data_last->status & PCH_UDC_RXTX_STS) !=
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_RTS_SUCC) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pr_err("Invalid RXTX status (0x%08x)"
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â" epstatus=0x%08x\n",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(req->td_data_last->status &
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â PCH_UDC_RXTX_STS),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(int)(ep->epsts));
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return;
> + Â Â Â Â Â Â Â Â Â Â Â }
> +#endif
> + Â Â Â Â Â Â Â Â Â Â Â req->req.actual = req->req.length;
> + Â Â Â Â Â Â Â Â Â Â Â req->td_data_last->status = PCH_UDC_BS_HST_BSY |
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_DMA_LAST;
> + Â Â Â Â Â Â Â Â Â Â Â req->td_data->status = PCH_UDC_BS_HST_BSY |
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPCH_UDC_DMA_LAST;
> + Â Â Â Â Â Â Â Â Â Â Â /* complete req */
> + Â Â Â Â Â Â Â Â Â Â Â complete_req(ep, req, 0);
> + Â Â Â Â Â Â Â Â Â Â Â req->dma_going = 0;
> + Â Â Â Â Â Â Â Â Â Â Â if (!list_empty(&ep->queue)) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â while (pch_udc_read_ep_control(ep->regs) &
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(1 << UDC_EPCTL_S))
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â udelay(100);
> +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_ep_clear_nak(ep->regs);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_enable_ep_interrupts(ep->dev->regs,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 1 << (ep->in ? ep->num : ep->num +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂUDC_EPINT_OUT_EP0));
> + Â Â Â Â Â Â Â Â Â Â Â } else {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pch_udc_disable_ep_interrupts(ep->dev->regs,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 1 << (ep->in ? ep->num : ep->num +
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂUDC_EPINT_OUT_EP0));
> + Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â }
> + Â Â Â }
> +}
[snip]
> +
> +/**
> + * pch_udc_svc_enum_interrupt - This function handles a USB speed enumeration
> + * Â Â Â Â Â Â Â Â Â Â Â Â Â Â done interrupt
> + * @dev: Â Â Â Reference to driver structure
> + */
> +void
> +pch_udc_svc_enum_interrupt(struct pch_udc_dev *dev)
> +{
> + Â Â Â u32 dev_stat, dev_speed;
> + Â Â Â u32 speed = USB_SPEED_FULL;
> +
> + Â Â Â pr_debug("%s: enter", __func__);
> + Â Â Â dev_stat = pch_udc_read_device_status(dev->regs);
> + Â Â Â dev_speed = (dev_stat & UDC_DEVSTS_ENUM_SPEED_MASK) >>
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂUDC_DEVSTS_ENUM_SPEED_OFS;
> +
> + Â Â Â pr_debug("%s: dev_speed = 0x%08x", __func__, dev_speed);
> +
> + Â Â Â if (dev_speed == UDC_DEVSTS_ENUM_SPEED_HIGH) {
> + Â Â Â Â Â Â Â pr_debug("HighSpeed");
> + Â Â Â Â Â Â Â speed = USB_SPEED_HIGH;
> + Â Â Â } else if (dev_speed == UDC_DEVSTS_ENUM_SPEED_FULL) {
> + Â Â Â Â Â Â Â pr_debug("FullSpeed");
> + Â Â Â Â Â Â Â speed = USB_SPEED_FULL;
> + Â Â Â } else if (dev_speed == UDC_DEVSTS_ENUM_SPEED_LOW) {
> + Â Â Â Â Â Â Â pr_debug("LowSpeed?");
> + Â Â Â Â Â Â Â speed = USB_SPEED_LOW;
> + Â Â Â } else {
> + Â Â Â Â Â Â Â pr_debug("FullSpeed?");

BUG() perhaps? Also, change this into a switch statement

> + Â Â Â }
> + Â Â Â dev->gadget.speed = speed;
> +
> + Â Â Â pch_udc_activate_control_ep(dev);
> +
> + Â Â Â /* enable ep0 interrupts */
> + Â Â Â pch_udc_enable_ep_interrupts(dev->regs, 1 << UDC_EPINT_IN_EP0 |
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 1 << UDC_EPINT_OUT_EP0);
> + Â Â Â /* enable DMA */
> + Â Â Â pch_udc_set_dma(dev->regs, DMA_DIR_TX);
> + Â Â Â pch_udc_set_dma(dev->regs, DMA_DIR_RX);
> + Â Â Â pch_udc_ep_set_rrdy(dev->ep[UDC_EP0OUT_IDX].regs);
> +
> +
> + Â Â Â pr_debug("%s: EP mask set to %x", __func__,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âioread32(&dev->regs->epirqmsk));
> +}
[snip]
> +
> +int pch_udc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +    unsigned long      resource;
> +    unsigned long      len;
> +    int                   retval = 0;
> +    struct pch_udc_dev   Â*dev;
> +
> + Â Â Â dev_dbg(&pdev->dev, "%s: enter", __func__);
> + Â Â Â /* one udc only */
> + Â Â Â if (pch_udc != NULL) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: already probed", __func__);
> + Â Â Â Â Â Â Â return -EBUSY;
> + Â Â Â }
> +
> + Â Â Â /* init */
> + Â Â Â dev = kzalloc(sizeof(struct pch_udc_dev), GFP_KERNEL);
> + Â Â Â if (dev == NULL) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: no memory for device structure",
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __func__);
> + Â Â Â Â Â Â Â return -ENOMEM;
> + Â Â Â }
> + Â Â Â memset(dev, 0, sizeof(struct pch_udc_dev));

kzalloc already does this for you

> + Â Â Â /* pci setup */
> + Â Â Â if (pci_enable_device(pdev) < 0) {
> + Â Â Â Â Â Â Â kfree(dev);
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: pci_enable_device failed", __func__);
> + Â Â Â Â Â Â Â return -ENODEV;
> + Â Â Â }
> + Â Â Â dev->active = 1;
> + Â Â Â pci_set_drvdata(pdev, dev);
> +
> + Â Â Â /* PCI resource allocation */
> + Â Â Â resource = pci_resource_start(pdev, 1);
> + Â Â Â len = pci_resource_len(pdev, 1);
> + Â Â Â dev_dbg(&pdev->dev, "%s: resource %lx, len %ld",
> + Â Â Â Â Â Â Â Â Â Â Â __func__, resource, len);
> +
> + Â Â Â if (request_mem_region(resource, len, KBUILD_MODNAME) == NULL) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: pci device used already", __func__);
> + Â Â Â Â Â Â Â retval = -EBUSY;
> + Â Â Â Â Â Â Â goto finished;
> + Â Â Â }
> + Â Â Â dev->phys_addr = resource;
> + Â Â Â dev->mem_region = 1;
> +
> + Â Â Â dev->virt_addr = ioremap_nocache(resource, len);
> + Â Â Â if (dev->virt_addr == NULL) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: device memory cannot be mapped",
> + Â Â Â Â Â Â Â Â Â Â Â __func__);
> + Â Â Â Â Â Â Â retval = -ENOMEM;
> + Â Â Â Â Â Â Â goto finished;
> + Â Â Â }
> + Â Â Â dev_dbg(&pdev->dev, "%s: device memory mapped at %x", __func__,
> + Â Â Â Â Â Â Â (int)dev->virt_addr);
> +
> + Â Â Â if (pdev->irq == 0) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: irq not set", __func__);
> + Â Â Â Â Â Â Â retval = -ENODEV;
> + Â Â Â Â Â Â Â goto finished;
> + Â Â Â }
> +
> + Â Â Â pch_udc = dev;
> +
> + Â Â Â /* initialize the hardware */
> + Â Â Â if (pch_udc_pcd_init(dev) != 0)
> + Â Â Â Â Â Â Â goto finished;
> +
> + Â Â Â if (request_irq(pdev->irq, pch_udc_isr, IRQF_SHARED,
> + Â Â Â Â Â Â Â Â Â Â Â KBUILD_MODNAME, dev) != 0) {
> + Â Â Â Â Â Â Â dev_err(&pdev->dev, "%s: request_irq(%d) fail", __func__,
> + Â Â Â Â Â Â Â Â Â Â Â pdev->irq);
> + Â Â Â Â Â Â Â retval = -ENODEV;
> + Â Â Â Â Â Â Â goto finished;
> + Â Â Â }
> + Â Â Â dev->irq = pdev->irq;
> + Â Â Â dev->irq_registered = 1;
> +
> + Â Â Â pci_set_master(pdev);
> + Â Â Â pci_try_set_mwi(pdev);
> +
> + Â Â Â /* device struct setup */
> + Â Â Â spin_lock_init(&dev->lock);
> + Â Â Â dev->pdev = pdev;
> + Â Â Â dev->gadget.ops = &pch_udc_ops;
> +
> + Â Â Â retval = init_dma_pools(dev);
> + Â Â Â if (retval != 0)
> + Â Â Â Â Â Â Â goto finished;
> +
> + Â Â Â dev_set_name(&dev->gadget.dev, "gadget");
> + Â Â Â dev->gadget.dev.parent = &pdev->dev;
> + Â Â Â dev->gadget.dev.dma_mask = pdev->dev.dma_mask;
> + Â Â Â dev->gadget.dev.release = gadget_release;
> + Â Â Â dev->gadget.name = KBUILD_MODNAME;
> + Â Â Â dev->gadget.is_dualspeed = 1;
> +
> + Â Â Â retval = device_register(&dev->gadget.dev);
> + Â Â Â if (retval != 0)
> + Â Â Â Â Â Â Â goto finished;
> + Â Â Â dev->registered = 1;
> +
> + Â Â Â /* Put the device in disconnected state till a driver is bound */
> + Â Â Â pch_udc_set_disconnect(dev->regs);
> + Â Â Â return 0;
> +
> +finished:
> + Â Â Â pch_udc_remove(pdev);
> + Â Â Â return retval;
> +}
> +
> +static const struct pci_device_id pch_udc_pcidev_id[] = {
> + Â Â Â {
> + Â Â Â Â Â Â Â PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PCH1_UDC),
> + Â Â Â Â Â Â Â .class = (PCI_CLASS_SERIAL_USB << 8) | 0xfe,
> + Â Â Â Â Â Â Â .class_mask = 0xffffffff,
> + Â Â Â },
> + Â Â Â { 0 },
> +};
> +
> +MODULE_DEVICE_TABLE(pci, pch_udc_pcidev_id);
> +
> +
> +static struct pci_driver pch_udc_driver = {
> + Â Â Â .name = KBUILD_MODNAME,
> + Â Â Â .id_table = Â Â pch_udc_pcidev_id,
> + Â Â Â .probe = Â Â Â Âpch_udc_probe,
> + Â Â Â .remove = Â Â Â pch_udc_remove,
> + Â Â Â .suspend = Â Â Âpch_udc_suspend,
> + Â Â Â .resume = Â Â Â pch_udc_resume,
> + Â Â Â .shutdown = Â Â pch_udc_shutdown,

Make all these functions static

> +};
> +
> +static int __init pch_udc_pci_init(void)
> +{
> + Â Â Â return pci_register_driver(&pch_udc_driver);
> +}
> +module_init(pch_udc_pci_init);
> +
> +static void __exit pch_udc_pci_exit(void)
> +{
> + Â Â Â pci_unregister_driver(&pch_udc_driver);
> +}
> +module_exit(pch_udc_pci_exit);
> diff --git a/drivers/usb/gadget/pch_udc.h b/drivers/usb/gadget/pch_udc.h
> new file mode 100755
> index 0000000..55c22ef
> --- /dev/null
> +++ b/drivers/usb/gadget/pch_udc.h
> @@ -0,0 +1,495 @@
> +/*
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *
> + * 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; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ÂSee the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA Â02111-1307, USA.
> + */
> +
> +#ifndef _PCH_UDC_H_
> +#define _PCH_UDC_H_
> +
> +/* Address offset of Registers */
> +#define UDC_EPIN_REGS_ADDR Â Â Â Â Â Â 0x000
> +#define UDC_EPOUT_REGS_ADDR Â Â Â Â Â Â0x200
> +#define UDC_EP_REG_OFS Â Â Â Â Â Â Â Â 0x20 Â Â/* Offset to next EP */
> +#define UDC_DEVCFG_ADDR Â Â Â Â Â Â Â Â Â Â Â Â0x400
> +#define PCH_UDC_CSR_BUSY_ADDR Â Â Â Â Â0x4f0
> +#define PCH_UDC_SRST_ADDR Â Â Â Â Â Â Â0x4fc
> +#define UDC_CSR_ADDR Â Â Â Â Â Â Â Â Â 0x500
> +
> +/* Bit position in UDC CSR Busy status Register */
> +#define PCH_UDC_CSR_BUSY Â Â Â Â Â Â Â 1
> +/* Bit position in UDC Soft reset Register */
> +#define PCH_UDC_PSRST Â Â Â Â Â Â Â Â Â1
> +#define PCH_UDC_SRST Â Â Â Â Â Â Â Â Â 0
> +
[snip]
> +
> +/**
> + * pch_udc_csrs - Structure to Endpoint configuration registers
> + */
> +struct pch_udc_csrs {
> + Â Â Â u32 ne[PCH_UDC_USED_EP_NUM * 2];

Why not do away with the structs and do something like:

#define PCH_UDC_CSR(ep)				(UDC_CSR_ADDR + ep*4)

(and similar for the others)

> +/* Starting bit position */
> +#define UDC_CSR_NE_NUM_OFS Â Â Â Â Â Â Â Â Â Â 0
> +#define UDC_CSR_NE_DIR_OFS Â Â Â Â Â Â Â Â Â Â 4
> +#define UDC_CSR_NE_TYPE_OFS Â Â Â Â Â Â Â Â Â Â5
> +#define UDC_CSR_NE_CFG_OFS Â Â Â Â Â Â Â Â Â Â 7
> +#define UDC_CSR_NE_INTF_OFS Â Â Â Â Â Â Â Â Â Â11
> +#define UDC_CSR_NE_ALT_OFS Â Â Â Â Â Â Â Â Â Â 15
> +#define UDC_CSR_NE_MAX_PKT_OFS Â Â Â Â Â Â Â Â 19
> +/* Mask patern */
> +#define UDC_CSR_NE_NUM_MASK Â Â Â Â Â Â Â Â Â Â0x0000000f
> +#define UDC_CSR_NE_DIR_MASK Â Â Â Â Â Â Â Â Â Â0x00000010
> +#define UDC_CSR_NE_TYPE_MASK Â Â Â Â Â Â Â Â Â 0x00000060
> +#define UDC_CSR_NE_CFG_MASK Â Â Â Â Â Â Â Â Â Â0x00000780
> +#define UDC_CSR_NE_INTF_MASK Â Â Â Â Â Â Â Â Â 0x00007800
> +#define UDC_CSR_NE_ALT_MASK Â Â Â Â Â Â Â Â Â Â0x00078000
> +#define UDC_CSR_NE_MAX_PKT_MASK Â Â Â Â Â Â Â Â Â Â Â Â0x3ff80000
> +};
> +
[snip]
> +
> +/**
> + * pch_udc_request - Structure holding a PCH USB device request
> + * @req            Âembedded ep request
> + * @td_data_phys    phys. address
> + * @td_data      Âfirst dma desc. of chain
> + * @td_data_last    last dma desc. of chain
> + * @queue       Âassociated queue
> + * @dma_going     ÂDMA in progress for request
> + * @dma_mapped     DMA memory mapped for request
> + * @dma_done      DMA completed for request
> + * @chain_len     Âchain length
> + */
> +struct pch_udc_request /* request packet */
> +{

I don't see any reason to not put this in the main .c file?
(same for struct pch_udc_{ep,request})

> +    struct usb_request       Âreq;
> +    dma_addr_t           Âtd_data_phys;
> +    struct pch_udc_data_dma_desc  Â*td_data;
> +    struct pch_udc_data_dma_desc  Â*td_data_last;
> +    struct list_head        Âqueue;
> +    unsigned            Âdma_going:1,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_mapped:1,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dma_done:1;
> +    unsigned            Âchain_len;
> +};
> +
[snip]
> +
> +struct pch_udc_dev {
> +    struct usb_gadget        gadget;
> +    struct usb_gadget_driver    Â*driver;
> +    struct pci_dev         Â*pdev;
> + Â Â Â /* all endpoints */
> +    struct pch_udc_ep        ep[PCH_UDC_EP_NUM];
> +    spinlock_t           Âlock;
> +    unsigned    Âactive:1,
> + Â Â Â Â Â Â Â Â Â Â Â stall:1,
> + Â Â Â Â Â Â Â Â Â Â Â prot_stall:1,
> + Â Â Â Â Â Â Â Â Â Â Â irq_registered:1,
> + Â Â Â Â Â Â Â Â Â Â Â mem_region:1,
> + Â Â Â Â Â Â Â Â Â Â Â registered:1,
> + Â Â Â Â Â Â Â Â Â Â Â suspended:1,
> + Â Â Â Â Â Â Â Â Â Â Â connected:1,
> + Â Â Â Â Â Â Â Â Â Â Â set_cfg_not_acked:1,
> + Â Â Â Â Â Â Â Â Â Â Â waiting_zlp_ack:1;
> +    struct pch_udc_csrs __iomem   *csr;
> +    struct pch_udc_regs __iomem   *regs;
> + Â Â Â struct pch_udc_ep_regs __iomem Â*ep_regs;

These pointers just seem unnecessary, especially as you could easily construct
them in-place by adding the appropriate offset to your base address..

> +    struct pci_pool         *data_requests;
> +    struct pci_pool         *stp_requests;
> +    unsigned long          phys_addr;
> +    void __iomem          Â*virt_addr;
> +    unsigned            Âirq;
> +    struct pch_udc_cfg_data     cfg_data;
> +};
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—