Hi Sebastian, > -----Original Message----- > From: Sebastian Andrzej Siewior [mailto:bigeasy@xxxxxxxxxxxxx] > Sent: Tuesday, September 20, 2011 6:47 PM > To: Neil Zhang > Cc: linux-usb@xxxxxxxxxxxxxxx; Chao Xie; Haojian Zhuang > Subject: Re: [PATCH 01/15] USB: mv_udc: refine the driver structure. > > * 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? It seems not arm specific, but it is used in platform related driver too, so I'll move it to include/linux/usb/mv_usb.h. > >+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. Thanks for find out this issue, I will cast it to unsigned long here, and platform related driver need to cast back to void __iomem * before readl/writel. > > > 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. We have some different SOC use this USB IP and the PHY setting are different. So it maybe a reasonable choice to realize it in platform relative driver. > > Sebastian Best Regards, Neil Zhang -- 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
