* Neil Zhang | 2011-09-19 16:07:25 [+0800]:
>diff --git a/arch/arm/plat-pxa/include/plat/usb.h b/arch/arm/plat-pxa/include/plat/usb.h
>new file mode 100644
>index 0000000..a0fac14
>--- /dev/null
>+++ b/arch/arm/plat-pxa/include/plat/usb.h
>@@ -0,0 +1,53 @@
>+/*
>+ * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
>+ *
>+ * 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.
>+ */
>+
>+#ifndef __MV_PLATFORM_USB_H
>+#define __MV_PLATFORM_USB_H
>+
>+#include <linux/usb/mv_usb.h>
>+
>+enum pxa_ehci_type {
>+ EHCI_UNDEFINED = 0,
>+ PXA_U2OEHCI, /* pxa 168, 9xx */
>+ PXA_SPH, /* pxa 168, 9xx SPH */
>+ MMP3_HSIC, /* mmp3 hsic */
>+ MMP3_FSIC, /* mmp3 fsic */
>+};
>+
>+enum {
>+ MV_USB_MODE_OTG,
>+ MV_USB_MODE_HOST,
>+};
>+
>+enum {
>+ VBUS_LOW = 0,
>+ VBUS_HIGH = 1 << 0,
>+};
Those two do not look that arm specifc, do they? Since you keep the
mv_udc.h file mind moving those there?
>+struct mv_usb_addon_irq {
>+ unsigned int irq;
>+ int (*poll)(void);
>+};
>+
>+struct mv_usb_platform_data {
>+ unsigned int clknum;
>+ char **clkname;
>+ struct mv_usb_addon_irq *id; /* Only valid for OTG. ID pin change*/
>+ struct mv_usb_addon_irq *vbus; /* valid for OTG/UDC. VBUS change*/
>+ /* only valid for HCD. OTG or Host only*/
>+ unsigned int mode;
>+
>+ int (*phy_init)(unsigned int regbase);
>+ void (*phy_deinit)(unsigned int regbase);
>+ int (*set_vbus)(unsigned int vbus);
>+ int (*private_init)(struct mv_op_regs *opregs,
>+ unsigned int phyregs);
>+};
>+
>+#endif
>diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>index 9ba725a..c36da63 100644
>--- a/drivers/usb/gadget/Makefile
>+++ b/drivers/usb/gadget/Makefile
>@@ -28,7 +28,7 @@ obj-$(CONFIG_USB_S3C_HSUDC) += s3c-hsudc.o
> obj-$(CONFIG_USB_LANGWELL) += langwell_udc.o
> obj-$(CONFIG_USB_EG20T) += pch_udc.o
> obj-$(CONFIG_USB_PXA_U2O) += mv_udc.o
>-mv_udc-y := mv_udc_core.o mv_udc_phy.o
>+mv_udc-y := mv_udc_core.o
> obj-$(CONFIG_USB_CI13XXX_MSM) += ci13xxx_msm.o
> obj-$(CONFIG_USB_FUSB300) += fusb300_udc.o
>
>diff --git a/drivers/usb/gadget/mv_udc.h b/drivers/usb/gadget/mv_udc.h
>index 65f1f7c..ef777d9 100644
>--- a/drivers/usb/gadget/mv_udc.h
>+++ b/drivers/usb/gadget/mv_udc.h
>@@ -1,9 +1,15 @@
>+/*
>+ * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
>+ *
>+ * 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.
>+ */
>
> #ifndef __MV_UDC_H
> #define __MV_UDC_H
>
>-#define VUSBHS_MAX_PORTS 8
>-
> #define DQH_ALIGNMENT 2048
> #define DTD_ALIGNMENT 64
> #define DMA_BOUNDARY 4096
>@@ -21,152 +27,11 @@
> #define WAIT_FOR_OUT_STATUS 3
> #define DATA_STATE_RECV 4
>
>-#define CAPLENGTH_MASK (0xff)
>-#define DCCPARAMS_DEN_MASK (0x1f)
>-
>-#define HCSPARAMS_PPC (0x10)
>-
>-/* Frame Index Register Bit Masks */
>-#define USB_FRINDEX_MASKS 0x3fff
>-
>-/* Command Register Bit Masks */
>-#define USBCMD_RUN_STOP (0x00000001)
>-#define USBCMD_CTRL_RESET (0x00000002)
>-#define USBCMD_SETUP_TRIPWIRE_SET (0x00002000)
>-#define USBCMD_SETUP_TRIPWIRE_CLEAR (~USBCMD_SETUP_TRIPWIRE_SET)
>-
>-#define USBCMD_ATDTW_TRIPWIRE_SET (0x00004000)
>-#define USBCMD_ATDTW_TRIPWIRE_CLEAR (~USBCMD_ATDTW_TRIPWIRE_SET)
>-
>-/* bit 15,3,2 are for frame list size */
>-#define USBCMD_FRAME_SIZE_1024 (0x00000000) /* 000 */
>-#define USBCMD_FRAME_SIZE_512 (0x00000004) /* 001 */
>-#define USBCMD_FRAME_SIZE_256 (0x00000008) /* 010 */
>-#define USBCMD_FRAME_SIZE_128 (0x0000000C) /* 011 */
>-#define USBCMD_FRAME_SIZE_64 (0x00008000) /* 100 */
>-#define USBCMD_FRAME_SIZE_32 (0x00008004) /* 101 */
>-#define USBCMD_FRAME_SIZE_16 (0x00008008) /* 110 */
>-#define USBCMD_FRAME_SIZE_8 (0x0000800C) /* 111 */
>-
>-#define EPCTRL_TX_ALL_MASK (0xFFFF0000)
>-#define EPCTRL_RX_ALL_MASK (0x0000FFFF)
>-
>-#define EPCTRL_TX_DATA_TOGGLE_RST (0x00400000)
>-#define EPCTRL_TX_EP_STALL (0x00010000)
>-#define EPCTRL_RX_EP_STALL (0x00000001)
>-#define EPCTRL_RX_DATA_TOGGLE_RST (0x00000040)
>-#define EPCTRL_RX_ENABLE (0x00000080)
>-#define EPCTRL_TX_ENABLE (0x00800000)
>-#define EPCTRL_CONTROL (0x00000000)
>-#define EPCTRL_ISOCHRONOUS (0x00040000)
>-#define EPCTRL_BULK (0x00080000)
>-#define EPCTRL_INT (0x000C0000)
>-#define EPCTRL_TX_TYPE (0x000C0000)
>-#define EPCTRL_RX_TYPE (0x0000000C)
>-#define EPCTRL_DATA_TOGGLE_INHIBIT (0x00000020)
>-#define EPCTRL_TX_EP_TYPE_SHIFT (18)
>-#define EPCTRL_RX_EP_TYPE_SHIFT (2)
>-
>-#define EPCOMPLETE_MAX_ENDPOINTS (16)
>-
>-/* endpoint list address bit masks */
>-#define USB_EP_LIST_ADDRESS_MASK 0xfffff800
>-
>-#define PORTSCX_W1C_BITS 0x2a
>-#define PORTSCX_PORT_RESET 0x00000100
>-#define PORTSCX_PORT_POWER 0x00001000
>-#define PORTSCX_FORCE_FULL_SPEED_CONNECT 0x01000000
>-#define PORTSCX_PAR_XCVR_SELECT 0xC0000000
>-#define PORTSCX_PORT_FORCE_RESUME 0x00000040
>-#define PORTSCX_PORT_SUSPEND 0x00000080
>-#define PORTSCX_PORT_SPEED_FULL 0x00000000
>-#define PORTSCX_PORT_SPEED_LOW 0x04000000
>-#define PORTSCX_PORT_SPEED_HIGH 0x08000000
>-#define PORTSCX_PORT_SPEED_MASK 0x0C000000
>-
>-/* USB MODE Register Bit Masks */
>-#define USBMODE_CTRL_MODE_IDLE 0x00000000
>-#define USBMODE_CTRL_MODE_DEVICE 0x00000002
>-#define USBMODE_CTRL_MODE_HOST 0x00000003
>-#define USBMODE_CTRL_MODE_RSV 0x00000001
>-#define USBMODE_SETUP_LOCK_OFF 0x00000008
>-#define USBMODE_STREAM_DISABLE 0x00000010
>-
>-/* USB STS Register Bit Masks */
>-#define USBSTS_INT 0x00000001
>-#define USBSTS_ERR 0x00000002
>-#define USBSTS_PORT_CHANGE 0x00000004
>-#define USBSTS_FRM_LST_ROLL 0x00000008
>-#define USBSTS_SYS_ERR 0x00000010
>-#define USBSTS_IAA 0x00000020
>-#define USBSTS_RESET 0x00000040
>-#define USBSTS_SOF 0x00000080
>-#define USBSTS_SUSPEND 0x00000100
>-#define USBSTS_HC_HALTED 0x00001000
>-#define USBSTS_RCL 0x00002000
>-#define USBSTS_PERIODIC_SCHEDULE 0x00004000
>-#define USBSTS_ASYNC_SCHEDULE 0x00008000
>-
>-
>-/* Interrupt Enable Register Bit Masks */
>-#define USBINTR_INT_EN (0x00000001)
>-#define USBINTR_ERR_INT_EN (0x00000002)
>-#define USBINTR_PORT_CHANGE_DETECT_EN (0x00000004)
>-
>-#define USBINTR_ASYNC_ADV_AAE (0x00000020)
>-#define USBINTR_ASYNC_ADV_AAE_ENABLE (0x00000020)
>-#define USBINTR_ASYNC_ADV_AAE_DISABLE (0xFFFFFFDF)
>-
>-#define USBINTR_RESET_EN (0x00000040)
>-#define USBINTR_SOF_UFRAME_EN (0x00000080)
>-#define USBINTR_DEVICE_SUSPEND (0x00000100)
>-
>-#define USB_DEVICE_ADDRESS_MASK (0xfe000000)
>-#define USB_DEVICE_ADDRESS_BIT_SHIFT (25)
>-
>-struct mv_cap_regs {
>- u32 caplength_hciversion;
>- u32 hcsparams; /* HC structural parameters */
>- u32 hccparams; /* HC Capability Parameters*/
>- u32 reserved[5];
>- u32 dciversion; /* DC version number and reserved 16 bits */
>- u32 dccparams; /* DC Capability Parameters */
>-};
>-
>-struct mv_op_regs {
>- u32 usbcmd; /* Command register */
>- u32 usbsts; /* Status register */
>- u32 usbintr; /* Interrupt enable */
>- u32 frindex; /* Frame index */
>- u32 reserved1[1];
>- u32 deviceaddr; /* Device Address */
>- u32 eplistaddr; /* Endpoint List Address */
>- u32 ttctrl; /* HOST TT status and control */
>- u32 burstsize; /* Programmable Burst Size */
>- u32 txfilltuning; /* Host Transmit Pre-Buffer Packet Tuning */
>- u32 reserved[4];
>- u32 epnak; /* Endpoint NAK */
>- u32 epnaken; /* Endpoint NAK Enable */
>- u32 configflag; /* Configured Flag register */
>- u32 portsc[VUSBHS_MAX_PORTS]; /* Port Status/Control x, x = 1..8 */
>- u32 otgsc;
>- u32 usbmode; /* USB Host/Device mode */
>- u32 epsetupstat; /* Endpoint Setup Status */
>- u32 epprime; /* Endpoint Initialize */
>- u32 epflush; /* Endpoint De-initialize */
>- u32 epstatus; /* Endpoint Status */
>- u32 epcomplete; /* Endpoint Interrupt On Complete */
>- u32 epctrlx[16]; /* Endpoint Control, where x = 0.. 15 */
>- u32 mcr; /* Mux Control */
>- u32 isr; /* Interrupt Status */
>- u32 ier; /* Interrupt Enable */
>-};
>-
> struct mv_udc {
> struct usb_gadget gadget;
> struct usb_gadget_driver *driver;
> spinlock_t lock;
>- struct completion *done;
>+ struct completion *done; /* make sure release is done */
> struct platform_device *dev;
> int irq;
>
>@@ -176,24 +41,25 @@ struct mv_udc {
> unsigned int max_eps;
> struct mv_dqh *ep_dqh;
> size_t ep_dqh_size;
>- dma_addr_t ep_dqh_dma;
>+ dma_addr_t ep_dqh_dma; /* dma address of QH */
>
>- struct dma_pool *dtd_pool;
>+ struct dma_pool *dtd_pool; /* dma pool for DTD */
I'm more a fan of having the comment above the variable instead of
behind it. But come on. That one is pretty obvious, isn't it? So if you
could please remove some of those obvious.
>diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
>index ce1ac2b..dcd9352 100644
>--- a/drivers/usb/gadget/mv_udc_core.c
>+++ b/drivers/usb/gadget/mv_udc_core.c
>@@ -1,3 +1,14 @@
>+/*
>+ * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
>+ * Author: Chao Xie <chao.xie@xxxxxxxxxxx>
>+ * Neil Zhang <zhangwm@xxxxxxxxxxx>
>+ *
>+ * 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/module.h>
> #include <linux/pci.h>
> #include <linux/dma-mapping.h>
>@@ -22,9 +33,12 @@
> #include <linux/irq.h>
> #include <linux/platform_device.h>
> #include <linux/clk.h>
>+#include <linux/usb/mv_usb.h>
> #include <asm/system.h>
> #include <asm/unaligned.h>
>
>+#include <plat/usb.h>
>+
> #include "mv_udc.h"
>
> #define DRIVER_DESC "Marvell PXA USB Device Controller driver"
>@@ -987,6 +1001,22 @@ static struct usb_ep_ops mv_ep_ops = {
> .fifo_flush = mv_ep_fifo_flush, /* flush fifo */
> };
>
>+static void udc_clock_enable(struct mv_udc *udc)
>+{
>+ unsigned int i;
>+
>+ for (i = 0; i < udc->clknum; i++)
>+ clk_enable(udc->clk[i]);
>+}
>+
>+static void udc_clock_disable(struct mv_udc *udc)
>+{
>+ unsigned int i;
>+
>+ for (i = 0; i < udc->clknum; i++)
>+ clk_disable(udc->clk[i]);
>+}
>+
> static void udc_stop(struct mv_udc *udc)
> {
> u32 tmp;
>@@ -1880,9 +1910,11 @@ static void gadget_release(struct device *_dev)
> kfree(udc);
> }
>
>-static int mv_udc_remove(struct platform_device *dev)
>+static __devexit int mv_udc_remove(struct platform_device *dev)
> {
> struct mv_udc *udc = the_controller;
>+ int clk_i;
>+
> DECLARE_COMPLETION(done);
>
> usb_del_gadget_udc(&udc->gadget);
>@@ -1915,6 +1947,9 @@ static int mv_udc_remove(struct platform_device *dev)
> kfree(udc->status_req);
> }
>
>+ for (clk_i = 0; clk_i <= udc->clknum; clk_i++)
>+ clk_put(udc->clk[clk_i]);
>+
> device_unregister(&udc->gadget.dev);
>
> /* free dev, wait for the release() finished */
>@@ -1925,35 +1960,50 @@ static int mv_udc_remove(struct platform_device *dev)
> return 0;
> }
>
>-int mv_udc_probe(struct platform_device *dev)
>+static int __devinit mv_udc_probe(struct platform_device *dev)
> {
>- struct mv_udc *udc;
>+ struct mv_usb_platform_data *pdata = dev->dev.platform_data;
>+ struct mv_udc *udc = NULL;
> int retval = 0;
>+ int clk_i = 0;
> struct resource *r;
> size_t size;
>
>- udc = kzalloc(sizeof *udc, GFP_KERNEL);
>+ if (pdata == NULL) {
>+ dev_err(&dev->dev, "missing platform_data\n");
>+ retval = -ENODEV;
>+ goto err_pdata;
>+ }
>+
>+ size = sizeof(*udc) + sizeof(struct clk *) * pdata->clknum;
>+ udc = kzalloc(size, GFP_KERNEL);
> if (udc == NULL) {
> dev_err(&dev->dev, "failed to allocate memory for udc\n");
> retval = -ENOMEM;
>- goto error;
>+ goto err_alloc_private;
> }
>
>+ the_controller = udc;
If you move to the new way of start/stop you can get rid of this global
the_controller thingy. What do you think?
>+
> spin_lock_init(&udc->lock);
>
> udc->dev = dev;
>-
>- udc->clk = clk_get(&dev->dev, "U2OCLK");
>- if (IS_ERR(udc->clk)) {
>- retval = PTR_ERR(udc->clk);
>- goto error;
>+ udc->pdata = dev->dev.platform_data;
>+
>+ udc->clknum = pdata->clknum;
>+ for (clk_i = 0; clk_i < udc->clknum; clk_i++) {
>+ udc->clk[clk_i] = clk_get(&dev->dev, pdata->clkname[clk_i]);
>+ if (IS_ERR(udc->clk[clk_i])) {
>+ retval = PTR_ERR(udc->clk[clk_i]);
>+ goto err_get_clk;
>+ }
> }
>
>- r = platform_get_resource_byname(udc->dev, IORESOURCE_MEM, "u2o");
>+ r = platform_get_resource_byname(udc->dev, IORESOURCE_MEM, "capregs");
> if (r == NULL) {
> dev_err(&dev->dev, "no I/O memory resource defined\n");
> retval = -ENODEV;
>- goto error;
>+ goto err_get_cap_regs;
> }
>
> udc->cap_regs = (struct mv_cap_regs __iomem *)
>@@ -1961,29 +2011,31 @@ int mv_udc_probe(struct platform_device *dev)
> if (udc->cap_regs == NULL) {
> dev_err(&dev->dev, "failed to map I/O memory\n");
> retval = -EBUSY;
>- goto error;
>+ goto err_map_cap_regs;
> }
>
>- r = platform_get_resource_byname(udc->dev, IORESOURCE_MEM, "u2ophy");
>+ r = platform_get_resource_byname(udc->dev, IORESOURCE_MEM, "phyregs");
> if (r == NULL) {
> dev_err(&dev->dev, "no phy I/O memory resource defined\n");
> retval = -ENODEV;
>- goto error;
>+ goto err_get_phy_regs;
> }
>
> udc->phy_regs = (unsigned int)ioremap(r->start, resource_size(r));
No, ioremal returns __iomem and a pointer. Casting to int is wrong. Keep
the pointer please.
> if (udc->phy_regs == 0) {
> dev_err(&dev->dev, "failed to map phy I/O memory\n");
> retval = -EBUSY;
>- goto error;
>+ goto err_map_phy_regs;
> }
>
> /* we will acces controller register, so enable the clk */
>- clk_enable(udc->clk);
>- retval = mv_udc_phy_init(udc->phy_regs);
>- if (retval) {
>- dev_err(&dev->dev, "phy initialization error %d\n", retval);
>- goto error;
>+ udc_clock_enable(udc);
>+ if (pdata->phy_init) {
>+ retval = pdata->phy_init(udc->phy_regs);
>+ if (retval) {
>+ dev_err(&dev->dev, "phy init error %d\n", retval);
>+ goto err_phy_init;
>+ }
> }
>
> udc->op_regs = (struct mv_op_regs __iomem *)((u32)udc->cap_regs
>@@ -1991,7 +2043,7 @@ int mv_udc_probe(struct platform_device *dev)
> & CAPLENGTH_MASK));
> udc->max_eps = readl(&udc->cap_regs->dccparams) & DCCPARAMS_DEN_MASK;
>
>- size = udc->max_eps * sizeof(struct mv_dqh) *2;
>+ size = udc->max_eps * sizeof(struct mv_dqh) * 2;
> size = (size + DQH_ALIGNMENT - 1) & ~(DQH_ALIGNMENT - 1);
> udc->ep_dqh = dma_alloc_coherent(&dev->dev, size,
> &udc->ep_dqh_dma, GFP_KERNEL);
>@@ -1999,7 +2051,7 @@ int mv_udc_probe(struct platform_device *dev)
> if (udc->ep_dqh == NULL) {
> dev_err(&dev->dev, "allocate dQH memory failed\n");
> retval = -ENOMEM;
>- goto error;
>+ goto err_alloc_dqh;
> }
> udc->ep_dqh_size = size;
>
>@@ -2012,15 +2064,15 @@ int mv_udc_probe(struct platform_device *dev)
>
> if (!udc->dtd_pool) {
> retval = -ENOMEM;
>- goto error;
>+ goto err_alloc_dtd_pool;
> }
>
>- size = udc->max_eps * sizeof(struct mv_ep) *2;
>+ size = udc->max_eps * sizeof(struct mv_ep) * 2;
> udc->eps = kzalloc(size, GFP_KERNEL);
> if (udc->eps == NULL) {
> dev_err(&dev->dev, "allocate ep memory failed\n");
> retval = -ENOMEM;
>- goto error;
>+ goto err_alloc_eps;
> }
>
> /* initialize ep0 status request structure */
>@@ -2028,7 +2080,7 @@ int mv_udc_probe(struct platform_device *dev)
> if (!udc->status_req) {
> dev_err(&dev->dev, "allocate status_req memory failed\n");
> retval = -ENOMEM;
>- goto error;
>+ goto err_alloc_status_req;
> }
> INIT_LIST_HEAD(&udc->status_req->queue);
>
>@@ -2045,15 +2097,16 @@ int mv_udc_probe(struct platform_device *dev)
> if (r == NULL) {
> dev_err(&dev->dev, "no IRQ resource defined\n");
> retval = -ENODEV;
>- goto error;
>+ goto err_get_irq;
> }
> udc->irq = r->start;
> if (request_irq(udc->irq, mv_udc_irq,
> IRQF_DISABLED | IRQF_SHARED, driver_name, udc)) {
> dev_err(&dev->dev, "Request irq %d for UDC failed\n",
> udc->irq);
>+ udc->irq = 0;
this is not required, is it?
> retval = -ENODEV;
>- goto error;
>+ goto err_request_irq;
> }
>
> /* initialize gadget structure */
>diff --git a/drivers/usb/gadget/mv_udc_phy.c b/drivers/usb/gadget/mv_udc_phy.c
>deleted file mode 100644
>index d4dea97..0000000
>--- a/drivers/usb/gadget/mv_udc_phy.c
>+++ /dev/null
Not sure it is a good idea to hide the phys in arch/arm instead of here.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
