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

[Peter Chen]

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



-- 

Best Regards,
Peter Chen