Patchwork [1/1] usb: fsl-mxc-udc: fix build error due to mach/hardware.h

login
register
mail settings
Submitter Felipe Balbi
Date Jan. 11, 2013, 12:50 p.m.
Message ID <20130111125059.GA12715@arwen.pp.htv.fi>
Download mbox | patch
Permalink /patch/211326/
State Superseded
Headers show

Comments

Felipe Balbi - Jan. 11, 2013, 12:50 p.m.
Hi,

On Fri, Jan 11, 2013 at 05:56:28PM +0800, Peter Chen wrote:
> It changes the driver to use platform_device_id rather than cpu_is_xxx
> to determine the SoC type, and updates the platform code accordingly.
> 
> Compile ok at imx_v6_v7_defconfig with CONFIG_USB_FSL_USB2 enable.
> Tested at mx51 bbg board, it works ok after enable phy clock
> (Need another patch to fix this problem)
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx25.c                     |    6 +-
>  arch/arm/mach-imx/clk-imx27.c                     |    6 +-
>  arch/arm/mach-imx/clk-imx31.c                     |    6 +-
>  arch/arm/mach-imx/clk-imx35.c                     |    6 +-
>  arch/arm/mach-imx/clk-imx51-imx53.c               |    6 +-
>  arch/arm/mach-imx/devices/devices-common.h        |    1 +
>  arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c |   15 +++---
>  drivers/usb/gadget/fsl_mxc_udc.c                  |   17 +++----
>  drivers/usb/gadget/fsl_udc_core.c                 |   52 +++++++++++++-------
>  drivers/usb/gadget/fsl_usb2_udc.h                 |   13 ++++--
>  include/linux/fsl_devices.h                       |    8 +++
>  11 files changed, 82 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-imx25.c b/arch/arm/mach-imx/clk-imx25.c
> index b197aa7..67e353d 100644
> --- a/arch/arm/mach-imx/clk-imx25.c
> +++ b/arch/arm/mach-imx/clk-imx25.c
> @@ -254,9 +254,9 @@ int __init mx25_clocks_init(void)
>  	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
>  	clk_register_clkdev(clk[usbotg_ahb], "ahb", "mxc-ehci.2");
>  	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2");
> -	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usbotg_ahb], "ahb", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
> +	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx25");
> +	clk_register_clkdev(clk[usbotg_ahb], "ahb", "imx-udc-mx25");
> +	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx25");
>  	clk_register_clkdev(clk[nfc_ipg_per], NULL, "imx25-nand.0");
>  	/* i.mx25 has the i.mx35 type cspi */
>  	clk_register_clkdev(clk[cspi1_ipg], NULL, "imx35-cspi.0");
> diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
> index 4c1d1e4..1ffe3b5 100644
> --- a/arch/arm/mach-imx/clk-imx27.c
> +++ b/arch/arm/mach-imx/clk-imx27.c
> @@ -236,9 +236,9 @@ int __init mx27_clocks_init(unsigned long fref)
>  	clk_register_clkdev(clk[lcdc_ahb_gate], "ahb", "imx21-fb.0");
>  	clk_register_clkdev(clk[csi_ahb_gate], "ahb", "imx27-camera.0");
>  	clk_register_clkdev(clk[per4_gate], "per", "imx27-camera.0");
> -	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "fsl-usb2-udc");
> +	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx27");
> +	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "imx-udc-mx27");
> +	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "imx-udc-mx27");
>  	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.0");
>  	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "mxc-ehci.0");
>  	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "mxc-ehci.0");
> diff --git a/arch/arm/mach-imx/clk-imx31.c b/arch/arm/mach-imx/clk-imx31.c
> index 8be64e0..ef66eaf 100644
> --- a/arch/arm/mach-imx/clk-imx31.c
> +++ b/arch/arm/mach-imx/clk-imx31.c
> @@ -139,9 +139,9 @@ int __init mx31_clocks_init(unsigned long fref)
>  	clk_register_clkdev(clk[usb_div_post], "per", "mxc-ehci.2");
>  	clk_register_clkdev(clk[usb_gate], "ahb", "mxc-ehci.2");
>  	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
> -	clk_register_clkdev(clk[usb_div_post], "per", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usb_gate], "ahb", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
> +	clk_register_clkdev(clk[usb_div_post], "per", "imx-udc-mx31");
> +	clk_register_clkdev(clk[usb_gate], "ahb", "imx-udc-mx31");
> +	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx31");
>  	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
>  	/* i.mx31 has the i.mx21 type uart */
>  	clk_register_clkdev(clk[uart1_gate], "per", "imx21-uart.0");
> diff --git a/arch/arm/mach-imx/clk-imx35.c b/arch/arm/mach-imx/clk-imx35.c
> index 66f3d65..69fe9c8 100644
> --- a/arch/arm/mach-imx/clk-imx35.c
> +++ b/arch/arm/mach-imx/clk-imx35.c
> @@ -251,9 +251,9 @@ int __init mx35_clocks_init()
>  	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2");
>  	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
>  	clk_register_clkdev(clk[usbotg_gate], "ahb", "mxc-ehci.2");
> -	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usbotg_gate], "ahb", "fsl-usb2-udc");
> +	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx35");
> +	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx35");
> +	clk_register_clkdev(clk[usbotg_gate], "ahb", "imx-udc-mx35");
>  	clk_register_clkdev(clk[wdog_gate], NULL, "imx2-wdt.0");
>  	clk_register_clkdev(clk[nfc_div], NULL, "imx25-nand.0");
>  	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
> diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
> index 579023f..fb7cb84 100644
> --- a/arch/arm/mach-imx/clk-imx51-imx53.c
> +++ b/arch/arm/mach-imx/clk-imx51-imx53.c
> @@ -269,9 +269,9 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
>  	clk_register_clkdev(clk[usboh3_per_gate], "per", "mxc-ehci.2");
>  	clk_register_clkdev(clk[usboh3_gate], "ipg", "mxc-ehci.2");
>  	clk_register_clkdev(clk[usboh3_gate], "ahb", "mxc-ehci.2");
> -	clk_register_clkdev(clk[usboh3_per_gate], "per", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usboh3_gate], "ipg", "fsl-usb2-udc");
> -	clk_register_clkdev(clk[usboh3_gate], "ahb", "fsl-usb2-udc");
> +	clk_register_clkdev(clk[usboh3_per_gate], "per", "imx-udc-mx51");
> +	clk_register_clkdev(clk[usboh3_gate], "ipg", "imx-udc-mx51");
> +	clk_register_clkdev(clk[usboh3_gate], "ahb", "imx-udc-mx51");
>  	clk_register_clkdev(clk[nfc_gate], NULL, "imx51-nand");
>  	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "imx-ssi.0");
>  	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1");
> diff --git a/arch/arm/mach-imx/devices/devices-common.h b/arch/arm/mach-imx/devices/devices-common.h
> index 6277baf..9bd5777 100644
> --- a/arch/arm/mach-imx/devices/devices-common.h
> +++ b/arch/arm/mach-imx/devices/devices-common.h
> @@ -63,6 +63,7 @@ struct platform_device *__init imx_add_flexcan(
>  
>  #include <linux/fsl_devices.h>
>  struct imx_fsl_usb2_udc_data {
> +	const char *devid;
>  	resource_size_t iobase;
>  	resource_size_t irq;
>  };
> diff --git a/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c b/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
> index 37e4439..fb527c7 100644
> --- a/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
> +++ b/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
> @@ -11,35 +11,36 @@
>  #include "../hardware.h"
>  #include "devices-common.h"
>  
> -#define imx_fsl_usb2_udc_data_entry_single(soc)				\
> +#define imx_fsl_usb2_udc_data_entry_single(soc, _devid)			\
>  	{								\
> +		.devid = _devid,					\
>  		.iobase = soc ## _USB_OTG_BASE_ADDR,			\
>  		.irq = soc ## _INT_USB_OTG,				\
>  	}
>  
>  #ifdef CONFIG_SOC_IMX25
>  const struct imx_fsl_usb2_udc_data imx25_fsl_usb2_udc_data __initconst =
> -	imx_fsl_usb2_udc_data_entry_single(MX25);
> +	imx_fsl_usb2_udc_data_entry_single(MX25, "imx-udc-mx25");
>  #endif /* ifdef CONFIG_SOC_IMX25 */
>  
>  #ifdef CONFIG_SOC_IMX27
>  const struct imx_fsl_usb2_udc_data imx27_fsl_usb2_udc_data __initconst =
> -	imx_fsl_usb2_udc_data_entry_single(MX27);
> +	imx_fsl_usb2_udc_data_entry_single(MX27, "imx-udc-mx27");
>  #endif /* ifdef CONFIG_SOC_IMX27 */
>  
>  #ifdef CONFIG_SOC_IMX31
>  const struct imx_fsl_usb2_udc_data imx31_fsl_usb2_udc_data __initconst =
> -	imx_fsl_usb2_udc_data_entry_single(MX31);
> +	imx_fsl_usb2_udc_data_entry_single(MX31, "imx-udc-mx31");
>  #endif /* ifdef CONFIG_SOC_IMX31 */
>  
>  #ifdef CONFIG_SOC_IMX35
>  const struct imx_fsl_usb2_udc_data imx35_fsl_usb2_udc_data __initconst =
> -	imx_fsl_usb2_udc_data_entry_single(MX35);
> +	imx_fsl_usb2_udc_data_entry_single(MX35, "imx-udc-mx35");
>  #endif /* ifdef CONFIG_SOC_IMX35 */
>  
>  #ifdef CONFIG_SOC_IMX51
>  const struct imx_fsl_usb2_udc_data imx51_fsl_usb2_udc_data __initconst =
> -	imx_fsl_usb2_udc_data_entry_single(MX51);
> +	imx_fsl_usb2_udc_data_entry_single(MX51, "imx-udc-mx51");
>  #endif
>  
>  struct platform_device *__init imx_add_fsl_usb2_udc(
> @@ -57,7 +58,7 @@ struct platform_device *__init imx_add_fsl_usb2_udc(
>  			.flags = IORESOURCE_IRQ,
>  		},
>  	};
> -	return imx_add_platform_device_dmamask("fsl-usb2-udc", -1,
> +	return imx_add_platform_device_dmamask(data->devid, -1,
>  			res, ARRAY_SIZE(res),
>  			pdata, sizeof(*pdata), DMA_BIT_MASK(32));
>  }
> diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
> index 1b0f086..f313085 100644
> --- a/drivers/usb/gadget/fsl_mxc_udc.c
> +++ b/drivers/usb/gadget/fsl_mxc_udc.c
> @@ -18,8 +18,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  
> -#include <mach/hardware.h>
> -
>  static struct clk *mxc_ahb_clk;
>  static struct clk *mxc_per_clk;
>  static struct clk *mxc_ipg_clk;
> @@ -28,7 +26,7 @@ static struct clk *mxc_ipg_clk;
>  #define USBPHYCTRL_OTGBASE_OFFSET	0x608
>  #define USBPHYCTRL_EVDO			(1 << 23)
>  
> -int fsl_udc_clk_init(struct platform_device *pdev)
> +int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
>  {
>  	struct fsl_usb2_platform_data *pdata;
>  	unsigned long freq;
> @@ -59,7 +57,7 @@ int fsl_udc_clk_init(struct platform_device *pdev)
>  	clk_prepare_enable(mxc_per_clk);
>  
>  	/* make sure USB_CLK is running at 60 MHz +/- 1000 Hz */
> -	if (!cpu_is_mx51()) {
> +	if (!(devtype == IMX51_UDC)) {
>  		freq = clk_get_rate(mxc_per_clk);
>  		if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
>  		    (freq < 59999000 || freq > 60001000)) {
> @@ -79,19 +77,18 @@ eclkrate:
>  	return ret;
>  }
>  
> -void fsl_udc_clk_finalize(struct platform_device *pdev)
> +void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
> +	struct platform_device *pdev)
>  {
>  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
> -	if (cpu_is_mx35()) {
> +	if (devtype == IMX35_UDC) {
>  		unsigned int v;
>  
>  		/* workaround ENGcm09152 for i.MX35 */
>  		if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
> -			v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
> -					USBPHYCTRL_OTGBASE_OFFSET));
> +			v = readl(pdata->regs + USBPHYCTRL_OTGBASE_OFFSET);
>  			writel(v | USBPHYCTRL_EVDO,
> -				MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
> -					USBPHYCTRL_OTGBASE_OFFSET));
> +				pdata->regs + USBPHYCTRL_OTGBASE_OFFSET);
>  		}
>  	}
>  
> diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> index c19f7f1..9f9005b 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -41,6 +41,7 @@
>  #include <linux/fsl_devices.h>
>  #include <linux/dmapool.h>
>  #include <linux/delay.h>
> +#include <linux/of_device.h>
>  
>  #include <asm/byteorder.h>
>  #include <asm/io.h>
> @@ -2438,17 +2439,13 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  	unsigned int i;
>  	u32 dccparams;
>  
> -	if (strcmp(pdev->name, driver_name)) {
> -		VDBG("Wrong device");
> -		return -ENODEV;
> -	}
> -
>  	udc_controller = kzalloc(sizeof(struct fsl_udc), GFP_KERNEL);
>  	if (udc_controller == NULL) {
>  		ERR("malloc udc failed\n");
>  		return -ENOMEM;
>  	}
>  
> +	udc_controller->devtype = pdev->id_entry->driver_data;
>  	pdata = pdev->dev.platform_data;
>  	udc_controller->pdata = pdata;
>  	spin_lock_init(&udc_controller->lock);
> @@ -2505,7 +2502,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  #endif
>  
>  	/* Initialize USB clocks */
> -	ret = fsl_udc_clk_init(pdev);
> +	ret = fsl_udc_clk_init(udc_controller->devtype, pdev);
>  	if (ret < 0)
>  		goto err_iounmap_noclk;
>  
> @@ -2547,7 +2544,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  		dr_controller_setup(udc_controller);
>  	}
>  
> -	fsl_udc_clk_finalize(pdev);
> +	fsl_udc_clk_finalize(udc_controller->devtype, pdev);
>  
>  	/* Setup gadget structure */
>  	udc_controller->gadget.ops = &fsl_gadget_ops;
> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>  
>  	return fsl_udc_resume(NULL);
>  }
> -
>  /*-------------------------------------------------------------------------
>  	Register entry point for the peripheral controller driver
>  --------------------------------------------------------------------------*/
> -
> +static struct platform_device_id fsl_udc_devtype[] = {

should be const

> +	{
> +		.name = "imx-udc-mx25",
> +		.driver_data = IMX25_UDC,
> +	}, {
> +		.name = "imx-udc-mx27",
> +		.driver_data = IMX27_UDC,
> +	}, {
> +		.name = "imx-udc-mx31",
> +		.driver_data = IMX31_UDC,
> +	}, {
> +		.name = "imx-udc-mx35",
> +		.driver_data = IMX35_UDC,
> +	}, {
> +		.name = "imx-udc-mx51",
> +		.driver_data = IMX51_UDC,
> +	}
> +};
> +MODULE_DEVICE_TABLE(platform, fsl_udc_devtype);
>  static struct platform_driver udc_driver = {
> -	.remove  = __exit_p(fsl_udc_remove),
> +	.remove		= __exit_p(fsl_udc_remove),
> +	/* Just for FSL i.mx SoC currently */
> +	.id_table	= fsl_udc_devtype,
>  	/* these suspend and resume are not usb suspend and resume */
> -	.suspend = fsl_udc_suspend,
> -	.resume  = fsl_udc_resume,
> -	.driver  = {
> -		.name = (char *)driver_name,
> -		.owner = THIS_MODULE,
> -		/* udc suspend/resume called from OTG driver */
> -		.suspend = fsl_udc_otg_suspend,
> -		.resume  = fsl_udc_otg_resume,
> +	.suspend	= fsl_udc_suspend,
> +	.resume		= fsl_udc_resume,
> +	.driver		= {
> +			.name = (char *)driver_name,
> +			.owner = THIS_MODULE,
> +			/* udc suspend/resume called from OTG driver */
> +			.suspend = fsl_udc_otg_suspend,
> +			.resume  = fsl_udc_otg_resume,
>  	},
>  };
>  
> diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
> index f61a967..bc1f6d0 100644
> --- a/drivers/usb/gadget/fsl_usb2_udc.h
> +++ b/drivers/usb/gadget/fsl_usb2_udc.h
> @@ -505,6 +505,8 @@ struct fsl_udc {
>  	u32 ep0_dir;		/* Endpoint zero direction: can be
>  				   USB_DIR_IN or USB_DIR_OUT */
>  	u8 device_address;	/* Device USB address */
> +	/* devtype for kinds of SoC, only i.mx uses it now */
> +	enum fsl_udc_type devtype;

to me this looks wrong as it will grow forever. Are you sure you don't
have a way to detect the revision in runtime ?

BTW, it looks to me that, in order to remove cpu_is_*() from that
driver, all you have to do is:



The only problem is that you're accessing PHY address space directly
without even ioremap() it first, not to mention that PHY address space
should only be accessed by a PHY (drivers/usb/phy) driver.

All of these details are all fixed in chipidea. More and more I consider
just deleting this driver and forcing you guys to use chipidea.

That whole MX35_IO_ADDRESS() is really wrong. It shouldn't be used
outside of arch/mach-imx/, that's why it sits in a mach/ header.

As I said before, this patch is too big for -rc and is unnecessary
considering patch I wrote above. Note that there is no problems in
checking if ULPI PHY clk is 60MHz on all arches and, for the workaround,
you already have a runtime check.

Shawn, it can be broken down into smaller pieces because you can *FIX
THE COMPILE BREAKAGE* with a very small patch as above (only issue now
is usage of MX32_IO_ADDRESS()).
Shawn Guo - Jan. 14, 2013, 1:12 a.m.
Balbi,

On Fri, Jan 11, 2013 at 02:50:59PM +0200, Felipe Balbi wrote:
> As I said before, this patch is too big for -rc and is unnecessary
> considering patch I wrote above. Note that there is no problems in
> checking if ULPI PHY clk is 60MHz on all arches and, for the workaround,
> you already have a runtime check.
> 
Ok, I did not have these facts on my mind.  If these are true, the
cpu_is_xxx() shouldn't be necessary there from the beginning, and we
can simply remove them then.

> Shawn, it can be broken down into smaller pieces because you can *FIX
> THE COMPILE BREAKAGE* with a very small patch as above (only issue now
> is usage of MX32_IO_ADDRESS()).
> 
The MX35_IO_ADDRESS() also seems unnecessary, since as Peter's patch
suggested that pdata->regs can be used instead.

Shawn
Peter Chen - Jan. 14, 2013, 4:39 a.m.
On Fri, Jan 11, 2013 at 02:50:59PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Jan 11, 2013 at 05:56:28PM +0800, Peter Chen wrote:
> > It changes the driver to use platform_device_id rather than cpu_is_xxx
> > to determine the SoC type, and updates the platform code accordingly.
> > 
> > Compile ok at imx_v6_v7_defconfig with CONFIG_USB_FSL_USB2 enable.
> > Tested at mx51 bbg board, it works ok after enable phy clock
> > (Need another patch to fix this problem)
> > 
> > -
> > +static struct platform_device_id fsl_udc_devtype[] = {
> 
> should be const
OK.
> 
> > +	{
> > +		.name = "imx-udc-mx25",
> > +		.driver_data = IMX25_UDC,
> > +	}, {
> > +		.name = "imx-udc-mx27",
> > +		.driver_data = IMX27_UDC,
> > +	}, {
> > +		.name = "imx-udc-mx31",
> > +		.driver_data = IMX31_UDC,
> > +	}, {
> > +		.name = "imx-udc-mx35",
> > +		.driver_data = IMX35_UDC,
> > +	}, {
> > +		.name = "imx-udc-mx51",
> > +		.driver_data = IMX51_UDC,
> > +	}
> > +};
> > +MODULE_DEVICE_TABLE(platform, fsl_udc_devtype);
> >  static struct platform_driver udc_driver = {
> > -	.remove  = __exit_p(fsl_udc_remove),
> > +	.remove		= __exit_p(fsl_udc_remove),
> > +	/* Just for FSL i.mx SoC currently */
> > +	.id_table	= fsl_udc_devtype,
> >  	/* these suspend and resume are not usb suspend and resume */
> > -	.suspend = fsl_udc_suspend,
> > -	.resume  = fsl_udc_resume,
> > -	.driver  = {
> > -		.name = (char *)driver_name,
> > -		.owner = THIS_MODULE,
> > -		/* udc suspend/resume called from OTG driver */
> > -		.suspend = fsl_udc_otg_suspend,
> > -		.resume  = fsl_udc_otg_resume,
> > +	.suspend	= fsl_udc_suspend,
> > +	.resume		= fsl_udc_resume,
> > +	.driver		= {
> > +			.name = (char *)driver_name,
> > +			.owner = THIS_MODULE,
> > +			/* udc suspend/resume called from OTG driver */
> > +			.suspend = fsl_udc_otg_suspend,
> > +			.resume  = fsl_udc_otg_resume,
> >  	},
> >  };
> >  
> > diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
> > index f61a967..bc1f6d0 100644
> > --- a/drivers/usb/gadget/fsl_usb2_udc.h
> > +++ b/drivers/usb/gadget/fsl_usb2_udc.h
> > @@ -505,6 +505,8 @@ struct fsl_udc {
> >  	u32 ep0_dir;		/* Endpoint zero direction: can be
> >  				   USB_DIR_IN or USB_DIR_OUT */
> >  	u8 device_address;	/* Device USB address */
> > +	/* devtype for kinds of SoC, only i.mx uses it now */
> > +	enum fsl_udc_type devtype;
> 
> to me this looks wrong as it will grow forever. Are you sure you don't
> have a way to detect the revision in runtime ?
> 
> BTW, it looks to me that, in order to remove cpu_is_*() from that
> driver, all you have to do is:
> 
> diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
> index 1b0f086..f06102d 100644
> --- a/drivers/usb/gadget/fsl_mxc_udc.c
> +++ b/drivers/usb/gadget/fsl_mxc_udc.c
> @@ -18,8 +18,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  
> -#include <mach/hardware.h>
> -
>  static struct clk *mxc_ahb_clk;
>  static struct clk *mxc_per_clk;
>  static struct clk *mxc_ipg_clk;
> @@ -59,14 +57,12 @@ int fsl_udc_clk_init(struct platform_device *pdev)
>  	clk_prepare_enable(mxc_per_clk);
>  
>  	/* make sure USB_CLK is running at 60 MHz +/- 1000 Hz */
> -	if (!cpu_is_mx51()) {
> -		freq = clk_get_rate(mxc_per_clk);
> -		if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
> -		    (freq < 59999000 || freq > 60001000)) {
> -			dev_err(&pdev->dev, "USB_CLK=%lu, should be 60MHz\n", freq);
> -			ret = -EINVAL;
> -			goto eclkrate;
> -		}
> +	freq = clk_get_rate(mxc_per_clk);
> +	if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
> +			(freq < 59999000 || freq > 60001000)) {
> +		dev_err(&pdev->dev, "USB_CLK=%lu, should be 60MHz\n", freq);
> +		ret = -EINVAL;
> +		goto eclkrate;
>  	}
For mx51, the otg port, the pdata->phy_mode is FSL_USB2_PHY_UTMI_WIDE,
the freq of "per_clk" is 166250000. So, your patch does not work.
>  
>  	return 0;
> @@ -82,17 +78,15 @@ eclkrate:
>  void fsl_udc_clk_finalize(struct platform_device *pdev)
>  {
>  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
> -	if (cpu_is_mx35()) {
> -		unsigned int v;
> +	unsigned int v;
>  
> -		/* workaround ENGcm09152 for i.MX35 */
> -		if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
> -			v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
> +	/* workaround ENGcm09152 for i.MX35 */
> +	if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
> +		v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
>  					USBPHYCTRL_OTGBASE_OFFSET));
> -			writel(v | USBPHYCTRL_EVDO,
> +		writel(v | USBPHYCTRL_EVDO,
>  				MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
>  					USBPHYCTRL_OTGBASE_OFFSET));
> -		}
>  	}
>  
>  	/* ULPI transceivers don't need usbpll */
chipidea's phy interface uses the same register mapping with controller's.
eg, controller register is $BASE, the PHY interface is $BASE + 0x600 or
$BASE + 0x800. So, as a workaround, we can ioremap phy interface 
at fsl_mxc_udc.c, and iounmap after finishing using.

The problem at my code is I have not remapped it before using.
> 
> 
> The only problem is that you're accessing PHY address space directly
> without even ioremap() it first, not to mention that PHY address space
> should only be accessed by a PHY (drivers/usb/phy) driver.
> 
> All of these details are all fixed in chipidea. More and more I consider
> just deleting this driver and forcing you guys to use chipidea.
> 
> That whole MX35_IO_ADDRESS() is really wrong. It shouldn't be used
> outside of arch/mach-imx/, that's why it sits in a mach/ header.
> 
> As I said before, this patch is too big for -rc and is unnecessary
> considering patch I wrote above. Note that there is no problems in
> checking if ULPI PHY clk is 60MHz on all arches and, for the workaround,
> you already have a runtime check.
> 
> Shawn, it can be broken down into smaller pieces because you can *FIX
> THE COMPILE BREAKAGE* with a very small patch as above (only issue now
> is usage of MX32_IO_ADDRESS()).
> 
> -- 
> balbi
Felipe Balbi - Jan. 14, 2013, 7:50 a.m.
On Mon, Jan 14, 2013 at 09:12:43AM +0800, Shawn Guo wrote:
> Balbi,
> 
> On Fri, Jan 11, 2013 at 02:50:59PM +0200, Felipe Balbi wrote:
> > As I said before, this patch is too big for -rc and is unnecessary
> > considering patch I wrote above. Note that there is no problems in
> > checking if ULPI PHY clk is 60MHz on all arches and, for the workaround,
> > you already have a runtime check.
> > 
> Ok, I did not have these facts on my mind.  If these are true, the
> cpu_is_xxx() shouldn't be necessary there from the beginning, and we
> can simply remove them then.
> 
> > Shawn, it can be broken down into smaller pieces because you can *FIX
> > THE COMPILE BREAKAGE* with a very small patch as above (only issue now
> > is usage of MX32_IO_ADDRESS()).
> > 
> The MX35_IO_ADDRESS() also seems unnecessary, since as Peter's patch
> suggested that pdata->regs can be used instead.

pdata->regs is a hack. The 'canonical' way to pass addresses to drivers
is via struct resource.

Patch

diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
index 1b0f086..f06102d 100644
--- a/drivers/usb/gadget/fsl_mxc_udc.c
+++ b/drivers/usb/gadget/fsl_mxc_udc.c
@@ -18,8 +18,6 @@ 
 #include <linux/platform_device.h>
 #include <linux/io.h>
 
-#include <mach/hardware.h>
-
 static struct clk *mxc_ahb_clk;
 static struct clk *mxc_per_clk;
 static struct clk *mxc_ipg_clk;
@@ -59,14 +57,12 @@  int fsl_udc_clk_init(struct platform_device *pdev)
 	clk_prepare_enable(mxc_per_clk);
 
 	/* make sure USB_CLK is running at 60 MHz +/- 1000 Hz */
-	if (!cpu_is_mx51()) {
-		freq = clk_get_rate(mxc_per_clk);
-		if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
-		    (freq < 59999000 || freq > 60001000)) {
-			dev_err(&pdev->dev, "USB_CLK=%lu, should be 60MHz\n", freq);
-			ret = -EINVAL;
-			goto eclkrate;
-		}
+	freq = clk_get_rate(mxc_per_clk);
+	if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
+			(freq < 59999000 || freq > 60001000)) {
+		dev_err(&pdev->dev, "USB_CLK=%lu, should be 60MHz\n", freq);
+		ret = -EINVAL;
+		goto eclkrate;
 	}
 
 	return 0;
@@ -82,17 +78,15 @@  eclkrate:
 void fsl_udc_clk_finalize(struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
-	if (cpu_is_mx35()) {
-		unsigned int v;
+	unsigned int v;
 
-		/* workaround ENGcm09152 for i.MX35 */
-		if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
-			v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
+	/* workaround ENGcm09152 for i.MX35 */
+	if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
+		v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
 					USBPHYCTRL_OTGBASE_OFFSET));
-			writel(v | USBPHYCTRL_EVDO,
+		writel(v | USBPHYCTRL_EVDO,
 				MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
 					USBPHYCTRL_OTGBASE_OFFSET));
-		}
 	}
 
 	/* ULPI transceivers don't need usbpll */