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-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 7 Sep 2010 14:53:05 +0200
From:	Maurus Cuelenaere <mcuelenaere@...il.com>
To:	Masayuki Ohtake <masa-korg@....okisemi.com>
Cc:	Randy Dunlap <randy.dunlap@...cle.com>,
	Peter Korsgaard <peter.korsgaard@...co.com>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Michal Nazarewicz <m.nazarewicz@...sung.com>,
	linux-usb <linux-usb@...r.kernel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Fabien Chouteau <fabien.chouteau@...co.com>,
	david Brownell <dbrownell@...rs.sourceforge.net>,
	Christoph Egger <siccegge@...d.informatik.uni-erlangen.de>,
	LKML <linux-kernel@...r.kernel.org>, MeeGo <meego-dev@...go.com>,
	"Wang, Qi" <qi.wang@...el.com>,
	"Wang, Yong Y" <yong.y.wang@...el.com>,
	Andrew <andrew.chih.howe.khor@...el.com>,
	Intel OTC <joel.clark@...el.com>,
	"Foster, Margie" <margie.foster@...el.com>,
	Arjan <arjan@...ux.intel.com>,
	Toshiharu Okada <okada533@....okisemi.com>,
	Takahiro Shimizu <shimizu394@....okisemi.com>,
	Tomoya Morinaga <morinaga526@....okisemi.com>
Subject: Re: [PATCH] USB device driver of Topcliff PCH

2010/9/7 Masayuki Ohtake <masa-korg@....okisemi.com>
>
> 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@....okisemi.com>

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ