Hi, (add -M -C flags to git format-patch) On Thu, Sep 22, 2011 at 03:03:48PM +0800, Neil Zhang wrote: > This patch do the following things: > > 1. Adjust the head files, move the common MACRO and type definition to > include/linux/usb/mv_usb.h. > 2. Add mv_usb_platform_data structure, make the driver fits all the > marvell platform using the same ChipIdea usb ip. > 3. Some SOC may has mutiple clock sources, so let me define it in > mv_usb_platform_data and give two helper function to deal with the > clock named udc_clock_enable/udc_clock_disable. ideally, drivers would not need to care about clocks. Don't you have pm_runtime support hiding clock operations from drivers ?? > 4. Different SOC will have some difference in PHY initialization, > so we will remove file mv_udc_phy.c and add two funtions in > mv_usb_platform_data, let the platform relative driver to realize it. Ideally, you would provide your transceiver driver, rather than hacking it around. btw, this is too big for my taste. Care to split it further ? > Signed-off-by: Neil Zhang <zhangwm@xxxxxxxxxxx> > --- > arch/arm/plat-pxa/include/plat/usb.h | 44 +++++++ > drivers/usb/gadget/Makefile | 2 +- > drivers/usb/gadget/mv_udc.h | 201 ++++++-------------------------- > drivers/usb/gadget/mv_udc_core.c | 184 ++++++++++++++++++++--------- > drivers/usb/gadget/mv_udc_phy.c | 214 ---------------------------------- > include/linux/usb/mv_usb.h | 193 ++++++++++++++++++++++++++++++ > 6 files changed, 406 insertions(+), 432 deletions(-) > create mode 100644 arch/arm/plat-pxa/include/plat/usb.h > delete mode 100644 drivers/usb/gadget/mv_udc_phy.c > create mode 100644 include/linux/usb/mv_usb.h > > 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..2fa82a5 > --- /dev/null > +++ b/arch/arm/plat-pxa/include/plat/usb.h > @@ -0,0 +1,44 @@ > +/* > + * 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 */ > +}; > + > +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)(void __iomem *regbase); > + void (*phy_deinit)(void __iomem *regbase); > + int (*set_vbus)(unsigned int vbus); > + int (*private_init)(struct mv_op_regs *opregs, > + unsigned int phyregs); > +}; > + > +#endif I would lilke it better if you move this header to include/linux/platform_data. We want to be able to build drivers on all architectures even if they will never be supported there. That's to catch compile errors easily via linux-next. > 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..7ea85f2 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,158 +27,17 @@ > #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 */ > -}; this doesn't seem to be part of this patch. Why do you need to move this code ? > 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 */ adding this comment isn't part of this patch. > struct platform_device *dev; > int irq; > > struct mv_cap_regs __iomem *cap_regs; > struct mv_op_regs __iomem *op_regs; > - unsigned int phy_regs; > + void __iomem *phy_regs; not part of this patch. > unsigned int max_eps; > struct mv_dqh *ep_dqh; > size_t ep_dqh_size; > @@ -201,7 +66,12 @@ struct mv_udc { > remote_wakeup:1, > softconnected:1, > force_fs:1; > - struct clk *clk; > + > + struct mv_usb_platform_data *pdata; > + > + /* some SOC has mutiple clock sources for USB*/ > + unsigned int clknum; > + struct clk *clk[0]; > }; > > /* endpoint data structure */ > @@ -245,17 +115,18 @@ struct mv_req { > #define EP_MAX_LENGTH_TRANSFER 0x4000 > > struct mv_dqh { > - /* Bits 16..26 Bit 15 is Interrupt On Setup */ > + /* Mult(31-30) , Zlt(29) , Max Pkt len(26-16) and IOS(15) */ > u32 max_packet_length; > u32 curr_dtd_ptr; /* Current dTD Pointer */ > u32 next_dtd_ptr; /* Next dTD Pointer */ > - /* Total bytes (16..30), IOC (15), INT (8), STS (0-7) */ > + > + /* Total bytes (30-16), IOC (15), MultO(11-10), STS (7-0) */ > u32 size_ioc_int_sts; > - u32 buff_ptr0; /* Buffer pointer Page 0 (12-31) */ > - u32 buff_ptr1; /* Buffer pointer Page 1 (12-31) */ > - u32 buff_ptr2; /* Buffer pointer Page 2 (12-31) */ > - u32 buff_ptr3; /* Buffer pointer Page 3 (12-31) */ > - u32 buff_ptr4; /* Buffer pointer Page 4 (12-31) */ > + u32 buff_ptr0; /* Buffer pointer Page 0 (31-12) */ > + u32 buff_ptr1; /* Buffer pointer Page 1 (31-12) */ > + u32 buff_ptr2; /* Buffer pointer Page 2 (31-12) */ > + u32 buff_ptr3; /* Buffer pointer Page 3 (31-12) */ > + u32 buff_ptr4; /* Buffer pointer Page 4 (31-12) */ updating those comments isn't part of this patch. > u32 reserved1; > /* 8 bytes of setup data that follows the Setup PID */ > u8 setup_buffer[8]; > @@ -264,19 +135,25 @@ struct mv_dqh { > > > #define DTD_NEXT_TERMINATE (0x00000001) > -#define DTD_IOC (0x00008000) > -#define DTD_STATUS_ACTIVE (0x00000080) > -#define DTD_STATUS_HALTED (0x00000040) > -#define DTD_STATUS_DATA_BUFF_ERR (0x00000020) > +#define DTD_STATUS_MASK (0X000000FF) > #define DTD_STATUS_TRANSACTION_ERR (0x00000008) > -#define DTD_RESERVED_FIELDS (0x00007F00) > -#define DTD_ERROR_MASK (0x68) > -#define DTD_ADDR_MASK (0xFFFFFFE0) > -#define DTD_PACKET_SIZE 0x7FFF0000 > +#define DTD_STATUS_DATA_BUFF_ERR (0x00000020) > +#define DTD_STATUS_HALTED (0x00000040) > +#define DTD_STATUS_ACTIVE (0x00000080) > +#define DTD_ERROR_MASK (DTD_STATUS_HALTED \ > + | DTD_STATUS_DATA_BUFF_ERR \ > + | DTD_STATUS_TRANSACTION_ERR) > +#define DTD_RESERVED_FIELDS (0x80007F00) > +#define DTD_IOC (0x00008000) > +#define DTD_PACKET_SIZE (0x7FFF0000) > #define DTD_LENGTH_BIT_POS (16) > > +#define REQ_UNCOMPLETE (1) > + > struct mv_dtd { > + /* Next TD pointer(31-5), T(0) set indicate invalid */ > u32 dtd_next; > + /* Total bytes (30-16), IOC (15), MultO(11-10), STS (7-0) */ > u32 size_ioc_sts; > u32 buff_ptr0; /* Buffer pointer Page 0 */ > u32 buff_ptr1; /* Buffer pointer Page 1 */ > @@ -289,6 +166,4 @@ struct mv_dtd { > struct mv_dtd *next_dtd_virt; > }; > > -extern int mv_udc_phy_init(unsigned int base); > - > #endif > diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c > index ce1ac2b..2153da1 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. > + */ adding this header isn't part of this patch. > #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> drivers shouldn't use <arch>, <asm> or <plat> headers. Or at least, should avoid as much as possible. > + > #include "mv_udc.h" > > #define DRIVER_DESC "Marvell PXA USB Device Controller driver" > @@ -48,6 +62,8 @@ > static const char driver_name[] = "mv_udc"; > static const char driver_desc[] = DRIVER_DESC; > > +static DECLARE_COMPLETION(release_done); > + > /* controller device global variable */ > static struct mv_udc *the_controller; > int mv_usb_otgsc; > @@ -987,6 +1003,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; > @@ -1877,18 +1909,15 @@ static void gadget_release(struct device *_dev) > struct mv_udc *udc = the_controller; > > complete(udc->done); > - kfree(udc); > } > > -static int mv_udc_remove(struct platform_device *dev) > +static __devexit int mv_udc_remove(struct platform_device *dev) not part of this patch. > { > struct mv_udc *udc = the_controller; > - DECLARE_COMPLETION(done); > + int clk_i; > > usb_del_gadget_udc(&udc->gadget); > > - udc->done = &done; > - > /* free memory allocated in probe */ > if (udc->dtd_pool) > dma_pool_destroy(udc->dtd_pool); > @@ -1907,83 +1936,101 @@ static int mv_udc_remove(struct platform_device *dev) > udc->cap_regs = NULL; > > if (udc->phy_regs) > - iounmap((void *)udc->phy_regs); > - udc->phy_regs = 0; > + iounmap(udc->phy_regs); > + udc->phy_regs = NULL; > > if (udc->status_req) { > kfree(udc->status_req->req.buf); > 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 */ > - wait_for_completion(&done); > - > + wait_for_completion(udc->done); should you have a timeout here ? wait_for_completion_timeout() ?? > the_controller = NULL; > + kfree(udc); > > 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"); > + return -ENODEV; > + } > + > + 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; > + return -ENOMEM; > } > > + the_controller = udc; > + udc->done = &release_done; > 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_put_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_put_clk; > } > > udc->cap_regs = (struct mv_cap_regs __iomem *) > - ioremap(r->start, resource_size(r)); > + ioremap(r->start, resource_size(r)); what's this change ?? Seems like you're just making the indentation even uglier. (please break this patch) -- balbi
Attachment:
signature.asc
Description: Digital signature