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
--
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
