[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Richard Zhao]

The work is based on usb-next which includes ci13xxx rework by Alexander Shishkin.

Status:
  - this version only tested host
  - usbotg controler works at host role

Todo:
  - usbh1 has strange system error when enumerate devices for on-board hub.
  - high speed usb connecton detect failed for the second time. It needs hub code hack.

Thanks Peter Chen for greate help.

Richard Zhao (7):
  usb: chipidea: permit user select USB_EHCI_ROOT_HUB_TT
  usb: chipidea: remove zero check of hw_ep_max
  usb: chipidea: add imx on-soc utmi phy driver
  usb: chipidea: add imx driver binding
  ARM: imx6q: correct device name of usbphy and usboh3 clock export
  ARM: imx6q: add anatop initialization for usb controllers
  ARM: dts: imx6q-sabrelite: add usb devices

 arch/arm/boot/dts/imx6q-sabrelite.dts |   19 ++++-
 arch/arm/boot/dts/imx6q.dtsi          |   44 ++++++++-
 arch/arm/mach-imx/clk-imx6q.c         |    7 +-
 arch/arm/mach-imx/mach-imx6q.c        |   60 +++++++++++
 drivers/usb/chipidea/Makefile         |    4 +
 drivers/usb/chipidea/ci13xxx_imx.c    |  177 +++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c           |    2 +-
 drivers/usb/chipidea/phy-imx-utmi.c   |  140 ++++++++++++++++++++++++++
 drivers/usb/host/Kconfig              |    2 +-
 9 files changed, 449 insertions(+), 6 deletions(-)
 create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
 create mode 100644 drivers/usb/chipidea/phy-imx-utmi.c

-- 
1.7.5.4

[PATCH v1 1/7] usb: chipidea: permit user select USB_EHCI_ROOT_HUB_TT

[Richard Zhao]

The ci13xxx hcd require USB_EHCI_ROOT_HUB_TT.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/usb/host/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 684a7bb..89d0371 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -65,7 +65,7 @@ config USB_EHCI_HCD
 
 config USB_EHCI_ROOT_HUB_TT
 	bool "Root Hub Transaction Translators"
-	depends on USB_EHCI_HCD
+	depends on USB_EHCI_HCD || USB_CHIPIDEA
 	---help---
 	  Some EHCI chips have vendor-specific extensions to integrate
 	  transaction translators, so that no OHCI or UHCI companion
-- 
1.7.5.4

[PATCH v1 1/7] usb: chipidea: permit user select USB_EHCI_ROOT_HUB_TT

[Marek Vasut]

Dear Richard Zhao,

> The ci13xxx hcd require USB_EHCI_ROOT_HUB_TT.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>

I already sent similar patch a few hours ago.

> ---
>  drivers/usb/host/Kconfig |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 684a7bb..89d0371 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -65,7 +65,7 @@ config USB_EHCI_HCD
> 
>  config USB_EHCI_ROOT_HUB_TT
>  	bool "Root Hub Transaction Translators"
> -	depends on USB_EHCI_HCD
> +	depends on USB_EHCI_HCD || USB_CHIPIDEA
>  	---help---
>  	  Some EHCI chips have vendor-specific extensions to integrate
>  	  transaction translators, so that no OHCI or UHCI companion

Best regards,
Marek Vasut

[PATCH v1 1/7] usb: chipidea: permit user select USB_EHCI_ROOT_HUB_TT

[Greg KH]

On Tue, May 15, 2012 at 04:27:15PM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> > The ci13xxx hcd require USB_EHCI_ROOT_HUB_TT.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> 
> I already sent similar patch a few hours ago.

But yours was more "complete" :)

I'll queue up yours in a bit, thanks.

greg k-h

[PATCH v1 1/7] usb: chipidea: permit user select USB_EHCI_ROOT_HUB_TT

[Marek Vasut]

Dear Richard Zhao,

> The ci13xxx hcd require USB_EHCI_ROOT_HUB_TT.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>

Ok, this has been sorted out, that makes it 6 patches to go ;-)

> ---
>  drivers/usb/host/Kconfig |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 684a7bb..89d0371 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -65,7 +65,7 @@ config USB_EHCI_HCD
> 
>  config USB_EHCI_ROOT_HUB_TT
>  	bool "Root Hub Transaction Translators"
> -	depends on USB_EHCI_HCD
> +	depends on USB_EHCI_HCD || USB_CHIPIDEA
>  	---help---
>  	  Some EHCI chips have vendor-specific extensions to integrate
>  	  transaction translators, so that no OHCI or UHCI companion

Best regards,
Marek Vasut

[PATCH v1 2/7] usb: chipidea: remove zero check of hw_ep_max

[Richard Zhao]

It's 0 for host only device.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/usb/chipidea/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index f568b8e..15e03b3 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -195,7 +195,7 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
 		ffs_nr(DCCPARAMS_DEN);
 	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
 
-	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
+	if (ci->hw_ep_max > ENDPT_MAX)
 		return -ENODEV;
 
 	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
-- 
1.7.5.4

[PATCH v1 2/7] usb: chipidea: remove zero check of hw_ep_max

[Marek Vasut]

Dear Richard Zhao,

> It's 0 for host only device.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/usb/chipidea/core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index f568b8e..15e03b3 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -195,7 +195,7 @@ static int hw_device_init(struct ci13xxx *ci, void
> __iomem *base) ffs_nr(DCCPARAMS_DEN);
>  	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
> 
> -	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
> +	if (ci->hw_ep_max > ENDPT_MAX)

Well, maybe this check should be extended a bit? Let me send a subsequent patch 
in reply to this, see what you think about it?

>  		return -ENODEV;
> 
>  	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",

Best regards,
Marek Vasut

[PATCH] usb: chipidea: improve the validation of endpoint count

[Marek Vasut]

The endpoint count is zero for host-only controller. On the other hand,
the endpoint count is non-zero for OTG capable controller.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Richard Zhao <richard.zhao@freescale.com>
Cc: alexander.shishkin at linux.intel.com
Cc: B20596 at freescale.com
Cc: B29397 at freescale.com
Cc: dong.aisheng at linaro.org
Cc: fabio.estevam at freescale.com
Cc: gregkh at linuxfoundation.org
Cc: kernel at pengutronix.de
Cc: linux-arm-kernel at lists.infradead.org
Cc: marex at denx.de
Cc: shawn.guo at linaro.org
To: linux-usb at vger.kernel.org
---
 drivers/usb/chipidea/core.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index f568b8e..b29a204 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -174,6 +174,7 @@ u8 hw_port_test_get(struct ci13xxx *ci)
 static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
 {
 	u32 reg;
+	int dc;
 
 	/* bank is a module variable */
 	ci->hw_bank.abs = base;
@@ -195,7 +196,12 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
 		ffs_nr(DCCPARAMS_DEN);
 	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
 
-	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
+	dc = hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC);
+
+	if (dc && (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX))
+		return -ENODEV;
+
+	if (!dc && (ci->hw_ep_max != 0))
 		return -ENODEV;
 
 	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
-- 
1.7.10

[PATCH] usb: chipidea: improve the validation of endpoint count

[Alexander Shishkin]

Marek Vasut <marex@denx.de> writes:

> The endpoint count is zero for host-only controller. On the other hand,
> the endpoint count is non-zero for OTG capable controller.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Richard Zhao <richard.zhao@freescale.com>
> Cc: alexander.shishkin at linux.intel.com
> Cc: B20596 at freescale.com
> Cc: B29397 at freescale.com
> Cc: dong.aisheng at linaro.org
> Cc: fabio.estevam at freescale.com
> Cc: gregkh at linuxfoundation.org
> Cc: kernel at pengutronix.de
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: marex at denx.de
> Cc: shawn.guo at linaro.org
> To: linux-usb at vger.kernel.org
> ---
>  drivers/usb/chipidea/core.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index f568b8e..b29a204 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -174,6 +174,7 @@ u8 hw_port_test_get(struct ci13xxx *ci)
>  static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
>  {
>  	u32 reg;
> +	int dc;
>  
>  	/* bank is a module variable */
>  	ci->hw_bank.abs = base;
> @@ -195,7 +196,12 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
>  		ffs_nr(DCCPARAMS_DEN);
>  	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
>  
> -	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
> +	dc = hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC);
> +
> +	if (dc && (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX))
> +		return -ENODEV;
> +
> +	if (!dc && (ci->hw_ep_max != 0))
>  		return -ENODEV;

According to the spec, this doesn't happen. And taking into account the
size of the bitfield encoding the number of endpoints, the (hw_ep_max >
ENDPT_MAX) check doesn't make much sense either. Again, according to the
spec, this field really encodes the number of hardware endpoints
(meaning both OUT and IN is one hw endpoint) and the valid values for
this field are 0-16, so even comparing it against ENDPT_MAX makes little
sense.

Having said that, there is already a check in udc.c for the DC bit and
it should be enough. If any particular controller reports wrong number
of endpoints, it needs to be considered separately.

Regards,
--
Alex

[PATCH v1 2/7] usb: chipidea: remove zero check of hw_ep_max

[Peter Chen]

On Tue, May 15, 2012 at 09:58:18PM +0800, Richard Zhao wrote:
> It's 0 for host only device.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/usb/chipidea/core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index f568b8e..15e03b3 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -195,7 +195,7 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
>  		ffs_nr(DCCPARAMS_DEN);
>  	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
>  
> -	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
> +	if (ci->hw_ep_max > ENDPT_MAX)
>  		return -ENODEV;
Can you move this get hw_ep_max code to udc.c? As hw_ep_max is only used
for device driver. What do you think, Alex?
>  
>  	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
> -- 
> 1.7.5.4
> 

-- 

Best Regards,
Peter Chen

[PATCH v1 2/7] usb: chipidea: remove zero check of hw_ep_max

[Richard Zhao]

On Wed, May 16, 2012 at 01:32:50PM +0800, Peter Chen wrote:
> On Tue, May 15, 2012 at 09:58:18PM +0800, Richard Zhao wrote:
> > It's 0 for host only device.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  drivers/usb/chipidea/core.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index f568b8e..15e03b3 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -195,7 +195,7 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
> >  		ffs_nr(DCCPARAMS_DEN);
> >  	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
> >  
> > -	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
> > +	if (ci->hw_ep_max > ENDPT_MAX)
> >  		return -ENODEV;
> Can you move this get hw_ep_max code to udc.c? As hw_ep_max is only used
> for device driver. What do you think, Alex?
Yes, But since Greg has applied the patch, one may need to rebase or let
it be as it is.

Thanks
Richard
> >  
> >  	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
> > -- 
> > 1.7.5.4
> > 
> 
> -- 
> 
> Best Regards,
> Peter Chen

[PATCH v1 2/7] usb: chipidea: remove zero check of hw_ep_max

[Alexander Shishkin]

Peter Chen <peter.chen@freescale.com> writes:

> On Tue, May 15, 2012 at 09:58:18PM +0800, Richard Zhao wrote:
>> It's 0 for host only device.
>> 
>> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
>> ---
>>  drivers/usb/chipidea/core.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index f568b8e..15e03b3 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -195,7 +195,7 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
>>  		ffs_nr(DCCPARAMS_DEN);
>>  	ci->hw_ep_max = reg * 2;   /* cache hw ENDPT_MAX */
>>  
>> -	if (ci->hw_ep_max == 0 || ci->hw_ep_max > ENDPT_MAX)
>> +	if (ci->hw_ep_max > ENDPT_MAX)
>>  		return -ENODEV;
> Can you move this get hw_ep_max code to udc.c? As hw_ep_max is only used
> for device driver. What do you think, Alex?

Yes, I think this needs to go to udc.c in the future.

Regards,
--
Alex

[PATCH v1 3/7] usb: chipidea: add imx on-soc utmi phy driver

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/usb/chipidea/Makefile       |    4 +
 drivers/usb/chipidea/phy-imx-utmi.c |  140 +++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/chipidea/phy-imx-utmi.c

diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index cc34937..0f34c0c 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -12,3 +12,7 @@ endif
 ifneq ($(CONFIG_ARCH_MSM),)
 	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
 endif
+
+ifneq ($(CONFIG_ARCH_MXC),)
+	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o
+endif
diff --git a/drivers/usb/chipidea/phy-imx-utmi.c b/drivers/usb/chipidea/phy-imx-utmi.c
new file mode 100644
index 0000000..f7a0fe7
--- /dev/null
+++ b/drivers/usb/chipidea/phy-imx-utmi.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/usb/otg.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+
+#define DRIVER_NAME "imx_utmi"
+
+#define HW_USBPHY_PWD	(0x00000000)
+#define HW_USBPHY_CTRL	0x00000030
+
+#define BM_USBPHY_CTRL_SFTRST 0x80000000
+#define BM_USBPHY_CTRL_CLKGATE 0x40000000
+#define BM_USBPHY_CTRL_ENUTMILEVEL3 0x00008000
+#define BM_USBPHY_CTRL_ENUTMILEVEL2 0x00004000
+
+struct imx_utmi_phy {
+	struct usb_phy phy;
+	struct clk *clk;
+};
+
+#define to_imx_phy(p) container_of((p), struct imx_utmi_phy, phy)
+
+static int imx_utmi_phy_init(struct usb_phy *phy)
+{
+	struct imx_utmi_phy *imx_phy;
+	u32 reg;
+
+	imx_phy = to_imx_phy(phy);
+
+	clk_prepare_enable(imx_phy->clk);
+
+	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
+	reg &= ~BM_USBPHY_CTRL_CLKGATE;
+	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
+
+	/* Reset USBPHY module */
+	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
+	reg |= BM_USBPHY_CTRL_SFTRST;
+	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
+	udelay(10);
+
+	/* Remove CLKGATE and SFTRST */
+	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
+	reg &= ~(BM_USBPHY_CTRL_CLKGATE | BM_USBPHY_CTRL_SFTRST);
+	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
+	udelay(10);
+
+	/* Power up the PHY */
+	writel_relaxed(0, phy->io_priv + HW_USBPHY_PWD);
+	/* enable FS/LS device */
+	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
+	reg |= BM_USBPHY_CTRL_ENUTMILEVEL2 | BM_USBPHY_CTRL_ENUTMILEVEL3;
+	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
+
+	return 0;
+}
+
+static void imx_utmi_phy_shutdown(struct usb_phy *phy)
+{
+	struct imx_utmi_phy *imx_phy;
+
+	imx_phy = to_imx_phy(phy);
+	clk_disable_unprepare(imx_phy->clk);
+}
+
+static int imx_utmi_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *base;
+	struct clk *clk;
+	struct imx_utmi_phy *imx_phy;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "can't get device resources\n");
+		return -ENOENT;
+	}
+
+	base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!base)
+		return -EBUSY;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	imx_phy = devm_kzalloc(&pdev->dev, sizeof(*imx_phy), GFP_KERNEL);
+	if (!imx_phy)
+		return -ENOMEM;
+
+	imx_phy->phy.io_priv = base;
+	imx_phy->phy.dev = &pdev->dev;
+	imx_phy->phy.label = DRIVER_NAME;
+	imx_phy->phy.init = imx_utmi_phy_init;
+	imx_phy->phy.shutdown = imx_utmi_phy_shutdown;
+	imx_phy->clk = clk;
+	platform_set_drvdata(pdev, &imx_phy->phy);
+
+	return 0;
+}
+
+static const struct of_device_id imx_utmi_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-usbphy", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver imx_utmi_driver = {
+	.probe = imx_utmi_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = imx_utmi_dt_ids,
+	 },
+};
+
+static int __init imx_utmi_init(void)
+{
+	return platform_driver_register(&imx_utmi_driver);
+}
+postcore_initcall(imx_utmi_init);
+
+static void __exit imx_utmi_exit(void)
+{
+	platform_driver_unregister(&imx_utmi_driver);
+}
+module_exit(imx_utmi_exit);
-- 
1.7.5.4

[PATCH v1 3/7] usb: chipidea: add imx on-soc utmi phy driver

[Marek Vasut]

Dear Richard Zhao,

> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/usb/chipidea/Makefile       |    4 +
>  drivers/usb/chipidea/phy-imx-utmi.c |  140
> +++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 0
> deletions(-)
>  create mode 100644 drivers/usb/chipidea/phy-imx-utmi.c
> 
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index cc34937..0f34c0c 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -12,3 +12,7 @@ endif
>  ifneq ($(CONFIG_ARCH_MSM),)
>  	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
>  endif
> +
> +ifneq ($(CONFIG_ARCH_MXC),)
> +	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o
> +endif
> diff --git a/drivers/usb/chipidea/phy-imx-utmi.c
> b/drivers/usb/chipidea/phy-imx-utmi.c new file mode 100644
> index 0000000..f7a0fe7
> --- /dev/null
> +++ b/drivers/usb/chipidea/phy-imx-utmi.c
> @@ -0,0 +1,140 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/usb/otg.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +
> +#define DRIVER_NAME "imx_utmi"
> +
> +#define HW_USBPHY_PWD	(0x00000000)

Drop the braces here

> +#define HW_USBPHY_CTRL	0x00000030
> +
> +#define BM_USBPHY_CTRL_SFTRST 0x80000000
> +#define BM_USBPHY_CTRL_CLKGATE 0x40000000
> +#define BM_USBPHY_CTRL_ENUTMILEVEL3 0x00008000
> +#define BM_USBPHY_CTRL_ENUTMILEVEL2 0x00004000
> +
> +struct imx_utmi_phy {
> +	struct usb_phy phy;
> +	struct clk *clk;
> +};
> +
> +#define to_imx_phy(p) container_of((p), struct imx_utmi_phy, phy)
> +
> +static int imx_utmi_phy_init(struct usb_phy *phy)
> +{
> +	struct imx_utmi_phy *imx_phy;
> +	u32 reg;
> +
> +	imx_phy = to_imx_phy(phy);
> +
> +	clk_prepare_enable(imx_phy->clk);
> +
> +	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
> +	reg &= ~BM_USBPHY_CTRL_CLKGATE;
> +	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
> +
> +	/* Reset USBPHY module */
> +	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
> +	reg |= BM_USBPHY_CTRL_SFTRST;
> +	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
> +	udelay(10);
> +
> +	/* Remove CLKGATE and SFTRST */
> +	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
> +	reg &= ~(BM_USBPHY_CTRL_CLKGATE | BM_USBPHY_CTRL_SFTRST);
> +	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);
> +	udelay(10);

You might want to check my PHY driver for the proper init sequence ... it uses 
mxs_reset_block() (or maybe there's already stmp_reset_block() ? )

> +
> +	/* Power up the PHY */
> +	writel_relaxed(0, phy->io_priv + HW_USBPHY_PWD);
> +	/* enable FS/LS device */
> +	reg = readl_relaxed(phy->io_priv + HW_USBPHY_CTRL);
> +	reg |= BM_USBPHY_CTRL_ENUTMILEVEL2 | BM_USBPHY_CTRL_ENUTMILEVEL3;
> +	writel_relaxed(reg, phy->io_priv + HW_USBPHY_CTRL);

Well, you can retrieve this from my PHY driver ;-)

> +
> +	return 0;
> +}
> +
> +static void imx_utmi_phy_shutdown(struct usb_phy *phy)
> +{
> +	struct imx_utmi_phy *imx_phy;
> +
> +	imx_phy = to_imx_phy(phy);
> +	clk_disable_unprepare(imx_phy->clk);

You forgot to disable the PHY here ... pull it out of my patchset and be done 
with it ;-)

> +}
> +
> +static int imx_utmi_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	void __iomem *base;
> +	struct clk *clk;
> +	struct imx_utmi_phy *imx_phy;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "can't get device resources\n");
> +		return -ENOENT;
> +	}
> +
> +	base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!base)
> +		return -EBUSY;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	imx_phy = devm_kzalloc(&pdev->dev, sizeof(*imx_phy), GFP_KERNEL);
> +	if (!imx_phy)
> +		return -ENOMEM;
> +
> +	imx_phy->phy.io_priv = base;
> +	imx_phy->phy.dev = &pdev->dev;
> +	imx_phy->phy.label = DRIVER_NAME;
> +	imx_phy->phy.init = imx_utmi_phy_init;
> +	imx_phy->phy.shutdown = imx_utmi_phy_shutdown;
> +	imx_phy->clk = clk;
> +	platform_set_drvdata(pdev, &imx_phy->phy);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx_utmi_dt_ids[] = {
> +	{ .compatible = "fsl,imx6q-usbphy", },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver imx_utmi_driver = {
> +	.probe = imx_utmi_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = imx_utmi_dt_ids,
> +	 },
> +};
> +
> +static int __init imx_utmi_init(void)
> +{
> +	return platform_driver_register(&imx_utmi_driver);
> +}
> +postcore_initcall(imx_utmi_init);
> +
> +static void __exit imx_utmi_exit(void)
> +{
> +	platform_driver_unregister(&imx_utmi_driver);
> +}

module_platform_driver() will make this clearer ... check my ci13xxx_mxs.c 
binding glue for usage.

> +module_exit(imx_utmi_exit);

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/usb/chipidea/Makefile      |    2 +-
 drivers/usb/chipidea/ci13xxx_imx.c |  177 ++++++++++++++++++++++++++++++++++++
 2 files changed, 178 insertions(+), 1 deletions(-)
 create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c

diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 0f34c0c..6821385 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -14,5 +14,5 @@ ifneq ($(CONFIG_ARCH_MSM),)
 endif
 
 ifneq ($(CONFIG_ARCH_MXC),)
-	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o
+	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o ci13xxx_imx.o
 endif
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
new file mode 100644
index 0000000..5f36f07
--- /dev/null
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -0,0 +1,177 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/pm_runtime.h>
+#include <linux/usb/ulpi.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/chipidea.h>
+#include <linux/usb/otg.h>
+#include <linux/dma-mapping.h>
+
+#include "ci.h"
+
+#define pdev_to_phy(pdev) \
+	((struct usb_phy *)platform_get_drvdata(pdev))
+
+static void ci13xxx_imx_notify_event(struct ci13xxx *udc, unsigned event)
+{
+#if 0
+	struct device *dev = udc->gadget.dev.parent;
+	int val;
+
+	switch (event) {
+	case CI13XXX_CONTROLLER_RESET_EVENT:
+		dev_dbg(dev, "CI13XXX_CONTROLLER_RESET_EVENT received\n");
+		writel(0, USB_AHBBURST);
+		writel(0, USB_AHBMODE);
+		break;
+	case CI13XXX_CONTROLLER_STOPPED_EVENT:
+		dev_dbg(dev, "CI13XXX_CONTROLLER_STOPPED_EVENT received\n");
+		/*
+		 * Put the transceiver in non-driving mode. Otherwise host
+		 * may not detect soft-disconnection.
+		 */
+		val = usb_phy_io_read(udc->transceiver, ULPI_FUNC_CTRL);
+		val &= ~ULPI_FUNC_CTRL_OPMODE_MASK;
+		val |= ULPI_FUNC_CTRL_OPMODE_NONDRIVING;
+		usb_phy_io_write(udc->transceiver, val, ULPI_FUNC_CTRL);
+		break;
+	default:
+		dev_dbg(dev, "unknown ci13xxx_udc event\n");
+		break;
+	}
+#endif
+}
+
+static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver = {
+	.name			= "ci13xxx_imx",
+	.flags			= CI13XXX_REGS_SHARED |
+				  CI13XXX_PULLUP_ON_VBUS |
+				  CI13XXX_DISABLE_STREAMING,
+	.capoffset		= 0x100,
+
+	.notify_event		= ci13xxx_imx_notify_event,
+};
+
+static int ci13xxx_imx_probe(struct platform_device *pdev)
+{
+	struct platform_device *plat_ci, *phy_pdev;
+	struct device_node *phy_np;
+	struct resource *res;
+	int hub_reset, vbus_pwr;
+	int ret;
+
+	dev_dbg(&pdev->dev, "ci13xxx_imx_probe\n");
+
+	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
+	if (phy_np) {
+		phy_pdev = of_find_device_by_node(phy_np);
+		if (phy_pdev) {
+			struct usb_phy *phy;
+			phy = pdev_to_phy(phy_pdev);
+			if (phy)
+				usb_phy_init(phy);
+		}
+		of_node_put(phy_np);
+	}
+
+	hub_reset = of_get_named_gpio(pdev->dev.of_node, "fsl,hub-reset", 0);
+	if (hub_reset > 0 &&
+	    devm_gpio_request(&pdev->dev, hub_reset, "hub-reset")) {
+		gpio_direction_output(hub_reset, 0);
+		udelay(10);
+		gpio_direction_output(hub_reset, 1);
+	}
+
+	/* we only support host now, so enable vbus here */
+	vbus_pwr = of_get_named_gpio(pdev->dev.of_node, "fsl,vbus-power", 0);
+	if (vbus_pwr > 0 &&
+	    devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
+		gpio_direction_output(vbus_pwr, 1);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "can't get device resources\n");
+		return -ENOENT;
+	}
+
+	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
+	if (!plat_ci) {
+		dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
+		return -ENOMEM;
+	}
+
+	ret = platform_device_add_resources(plat_ci, pdev->resource,
+					    pdev->num_resources);
+	if (ret) {
+		dev_err(&pdev->dev, "can't add resources to platform device\n");
+		goto put_platform;
+	}
+
+	ret = platform_device_add_data(plat_ci, &ci13xxx_imx_udc_driver,
+				       sizeof(ci13xxx_imx_udc_driver));
+	if (ret)
+		goto put_platform;
+
+	plat_ci->dev.dma_mask =
+		kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
+	if (!plat_ci->dev.dma_mask) {
+		ret = -ENOMEM;
+		goto put_platform;
+	}
+	*plat_ci->dev.dma_mask = DMA_BIT_MASK(32);
+	plat_ci->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+
+	ret = platform_device_add(plat_ci);
+	if (ret)
+		goto free_dma_mask;
+
+	pm_runtime_no_callbacks(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	return 0;
+
+free_dma_mask:
+	kfree(plat_ci->dev.dma_mask);
+	plat_ci->dev.dma_mask = NULL;
+put_platform:
+	platform_device_put(plat_ci);
+
+	return ret;
+}
+
+static const struct of_device_id ci13xxx_imx_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-usboh3", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver ci13xxx_imx_driver = {
+	.probe = ci13xxx_imx_probe,
+	.driver = {
+		.name = "imx_usboh3",
+		.of_match_table = ci13xxx_imx_dt_ids,
+	 },
+};
+
+MODULE_ALIAS("platform:imx_usboh3");
+
+static int __init ci13xxx_imx_init(void)
+{
+	return platform_driver_register(&ci13xxx_imx_driver);
+}
+module_init(ci13xxx_imx_init);
+
+MODULE_LICENSE("GPL v2");
-- 
1.7.5.4

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Russell King - ARM Linux]

On Tue, May 15, 2012 at 09:58:20PM +0800, Richard Zhao wrote:
> +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> +	if (!plat_ci) {
> +		dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = platform_device_add_resources(plat_ci, pdev->resource,
> +					    pdev->num_resources);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't add resources to platform device\n");
> +		goto put_platform;
> +	}
> +
> +	ret = platform_device_add_data(plat_ci, &ci13xxx_imx_udc_driver,
> +				       sizeof(ci13xxx_imx_udc_driver));
> +	if (ret)
> +		goto put_platform;
> +
> +	plat_ci->dev.dma_mask =
> +		kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> +	if (!plat_ci->dev.dma_mask) {
> +		ret = -ENOMEM;
> +		goto put_platform;
> +	}
> +	*plat_ci->dev.dma_mask = DMA_BIT_MASK(32);
> +	plat_ci->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +
> +	ret = platform_device_add(plat_ci);

Is there a reason not to use platform_device_register_full() ?

Also, you don't set the parent device - is there a dependency between
'pdev' and this new platform device you're creating?  If so, it should
be a child of 'pdev'.

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 15, 2012 at 03:03:55PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 15, 2012 at 09:58:20PM +0800, Richard Zhao wrote:
> > +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> > +	if (!plat_ci) {
> > +		dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = platform_device_add_resources(plat_ci, pdev->resource,
> > +					    pdev->num_resources);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "can't add resources to platform device\n");
> > +		goto put_platform;
> > +	}
> > +
> > +	ret = platform_device_add_data(plat_ci, &ci13xxx_imx_udc_driver,
> > +				       sizeof(ci13xxx_imx_udc_driver));
> > +	if (ret)
> > +		goto put_platform;
> > +
> > +	plat_ci->dev.dma_mask =
> > +		kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> > +	if (!plat_ci->dev.dma_mask) {
> > +		ret = -ENOMEM;
> > +		goto put_platform;
> > +	}
> > +	*plat_ci->dev.dma_mask = DMA_BIT_MASK(32);
> > +	plat_ci->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> > +
> > +	ret = platform_device_add(plat_ci);
> 
> Is there a reason not to use platform_device_register_full() ?
> 
> Also, you don't set the parent device - is there a dependency between
> 'pdev' and this new platform device you're creating?  If so, it should
> be a child of 'pdev'.
Thanks for your comment. All taken.

Richard

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 15, 2012 at 03:03:55PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 15, 2012 at 09:58:20PM +0800, Richard Zhao wrote:
> > +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> > +	if (!plat_ci) {
> > +		dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = platform_device_add_resources(plat_ci, pdev->resource,
> > +					    pdev->num_resources);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "can't add resources to platform device\n");
> > +		goto put_platform;
> > +	}
> > +
> > +	ret = platform_device_add_data(plat_ci, &ci13xxx_imx_udc_driver,
> > +				       sizeof(ci13xxx_imx_udc_driver));
> > +	if (ret)
> > +		goto put_platform;
> > +
> > +	plat_ci->dev.dma_mask =
> > +		kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> > +	if (!plat_ci->dev.dma_mask) {
> > +		ret = -ENOMEM;
> > +		goto put_platform;
> > +	}
> > +	*plat_ci->dev.dma_mask = DMA_BIT_MASK(32);
> > +	plat_ci->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> > +
> > +	ret = platform_device_add(plat_ci);
> 
> Is there a reason not to use platform_device_register_full() ?
platform_device_register_full() allocate the dma_mask but
platform_device_unregister() didn't free it. I have to free it
manually? Sounds strange.

Thanks
Richard
> 
> Also, you don't set the parent device - is there a dependency between
> 'pdev' and this new platform device you're creating?  If so, it should
> be a child of 'pdev'.
> 

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Marek Vasut]

Dear Richard Zhao,

> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/usb/chipidea/Makefile      |    2 +-
>  drivers/usb/chipidea/ci13xxx_imx.c |  177
> ++++++++++++++++++++++++++++++++++++ 2 files changed, 178 insertions(+), 1
> deletions(-)
>  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
> 
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 0f34c0c..6821385 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -14,5 +14,5 @@ ifneq ($(CONFIG_ARCH_MSM),)
>  endif
> 
>  ifneq ($(CONFIG_ARCH_MXC),)
> -	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o
> +	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o ci13xxx_imx.o

You're not actually supposed to use the UTMI PHY always. The PHY is separate 
from the USB stuff, so maybe put it into drivers/usb/otg/ like I had my driver 
placed?

>  endif
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> b/drivers/usb/chipidea/ci13xxx_imx.c new file mode 100644
> index 0000000..5f36f07
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -0,0 +1,177 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/usb/ulpi.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/chipidea.h>
> +#include <linux/usb/otg.h>
> +#include <linux/dma-mapping.h>
> +
> +#include "ci.h"
> +
> +#define pdev_to_phy(pdev) \
> +	((struct usb_phy *)platform_get_drvdata(pdev))
> +
> +static void ci13xxx_imx_notify_event(struct ci13xxx *udc, unsigned event)
> +{
> +#if 0

Woooo, what a big slab of dead code :-)

> +	struct device *dev = udc->gadget.dev.parent;
> +	int val;
> +
> +	switch (event) {
> +	case CI13XXX_CONTROLLER_RESET_EVENT:
> +		dev_dbg(dev, "CI13XXX_CONTROLLER_RESET_EVENT received\n");
> +		writel(0, USB_AHBBURST);
> +		writel(0, USB_AHBMODE);
> +		break;
> +	case CI13XXX_CONTROLLER_STOPPED_EVENT:
> +		dev_dbg(dev, "CI13XXX_CONTROLLER_STOPPED_EVENT received\n");
> +		/*
> +		 * Put the transceiver in non-driving mode. Otherwise host
> +		 * may not detect soft-disconnection.
> +		 */
> +		val = usb_phy_io_read(udc->transceiver, ULPI_FUNC_CTRL);
> +		val &= ~ULPI_FUNC_CTRL_OPMODE_MASK;
> +		val |= ULPI_FUNC_CTRL_OPMODE_NONDRIVING;
> +		usb_phy_io_write(udc->transceiver, val, ULPI_FUNC_CTRL);
> +		break;
> +	default:
> +		dev_dbg(dev, "unknown ci13xxx_udc event\n");
> +		break;
> +	}
> +#endif
> +}
> +
> +static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver = {
> +	.name			= "ci13xxx_imx",
> +	.flags			= CI13XXX_REGS_SHARED |
> +				  CI13XXX_PULLUP_ON_VBUS |
> +				  CI13XXX_DISABLE_STREAMING,
> +	.capoffset		= 0x100,
> +
> +	.notify_event		= ci13xxx_imx_notify_event,
> +};
> +
> +static int ci13xxx_imx_probe(struct platform_device *pdev)
> +{
> +	struct platform_device *plat_ci, *phy_pdev;
> +	struct device_node *phy_np;
> +	struct resource *res;
> +	int hub_reset, vbus_pwr;
> +	int ret;
> +
> +	dev_dbg(&pdev->dev, "ci13xxx_imx_probe\n");
> +
> +	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> +	if (phy_np) {
> +		phy_pdev = of_find_device_by_node(phy_np);
> +		if (phy_pdev) {
> +			struct usb_phy *phy;
> +			phy = pdev_to_phy(phy_pdev);
> +			if (phy)
> +				usb_phy_init(phy);
> +		}
> +		of_node_put(phy_np);
> +	}
> +
> +	hub_reset = of_get_named_gpio(pdev->dev.of_node, "fsl,hub-reset", 0);
> +	if (hub_reset > 0 &&
> +	    devm_gpio_request(&pdev->dev, hub_reset, "hub-reset")) {
> +		gpio_direction_output(hub_reset, 0);
> +		udelay(10);
> +		gpio_direction_output(hub_reset, 1);
> +	}
> +
> +	/* we only support host now, so enable vbus here */
> +	vbus_pwr = of_get_named_gpio(pdev->dev.of_node, "fsl,vbus-power", 0);
> +	if (vbus_pwr > 0 &&
> +	    devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
> +		gpio_direction_output(vbus_pwr, 1);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "can't get device resources\n");
> +		return -ENOENT;
> +	}
> +
> +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> +	if (!plat_ci) {
> +		dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = platform_device_add_resources(plat_ci, pdev->resource,
> +					    pdev->num_resources);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't add resources to platform device\n");
> +		goto put_platform;
> +	}
> +
> +	ret = platform_device_add_data(plat_ci, &ci13xxx_imx_udc_driver,
> +				       sizeof(ci13xxx_imx_udc_driver));
> +	if (ret)
> +		goto put_platform;
> +
> +	plat_ci->dev.dma_mask =
> +		kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);

Can this not be devm_kzalloc()ated?

> +	if (!plat_ci->dev.dma_mask) {
> +		ret = -ENOMEM;
> +		goto put_platform;
> +	}
> +	*plat_ci->dev.dma_mask = DMA_BIT_MASK(32);
> +	plat_ci->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +
> +	ret = platform_device_add(plat_ci);
> +	if (ret)
> +		goto free_dma_mask;
> +
> +	pm_runtime_no_callbacks(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +
> +free_dma_mask:
> +	kfree(plat_ci->dev.dma_mask);
> +	plat_ci->dev.dma_mask = NULL;
> +put_platform:
> +	platform_device_put(plat_ci);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id ci13xxx_imx_dt_ids[] = {
> +	{ .compatible = "fsl,imx6q-usboh3", },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver ci13xxx_imx_driver = {
> +	.probe = ci13xxx_imx_probe,
> +	.driver = {
> +		.name = "imx_usboh3",
> +		.of_match_table = ci13xxx_imx_dt_ids,

you're missing .remove() call here, check my implementation ;-)

> +	 },
> +};
> +
> +MODULE_ALIAS("platform:imx_usboh3");
> +
> +static int __init ci13xxx_imx_init(void)
> +{
> +	return platform_driver_register(&ci13xxx_imx_driver);
> +}
> +module_init(ci13xxx_imx_init);

module_platform_driver().

> +
> +MODULE_LICENSE("GPL v2");

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Alexander Shishkin]

Richard Zhao <richard.zhao@freescale.com> writes:

> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>

Besides things that were already pointed out, some minor comments below:

> ---
>  drivers/usb/chipidea/Makefile      |    2 +-
>  drivers/usb/chipidea/ci13xxx_imx.c |  177 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 178 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
>
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 0f34c0c..6821385 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -14,5 +14,5 @@ ifneq ($(CONFIG_ARCH_MSM),)
>  endif
>  
>  ifneq ($(CONFIG_ARCH_MXC),)
> -	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o
> +	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o ci13xxx_imx.o
>  endif
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> new file mode 100644
> index 0000000..5f36f07
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -0,0 +1,177 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/usb/ulpi.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/chipidea.h>
> +#include <linux/usb/otg.h>
> +#include <linux/dma-mapping.h>
> +
> +#include "ci.h"
> +
> +#define pdev_to_phy(pdev) \
> +	((struct usb_phy *)platform_get_drvdata(pdev))
> +
> +static void ci13xxx_imx_notify_event(struct ci13xxx *udc, unsigned event)
> +{
> +#if 0
> +	struct device *dev = udc->gadget.dev.parent;
> +	int val;
> +
> +	switch (event) {
> +	case CI13XXX_CONTROLLER_RESET_EVENT:
> +		dev_dbg(dev, "CI13XXX_CONTROLLER_RESET_EVENT received\n");
> +		writel(0, USB_AHBBURST);
> +		writel(0, USB_AHBMODE);
> +		break;
> +	case CI13XXX_CONTROLLER_STOPPED_EVENT:
> +		dev_dbg(dev, "CI13XXX_CONTROLLER_STOPPED_EVENT received\n");
> +		/*
> +		 * Put the transceiver in non-driving mode. Otherwise host
> +		 * may not detect soft-disconnection.
> +		 */
> +		val = usb_phy_io_read(udc->transceiver, ULPI_FUNC_CTRL);
> +		val &= ~ULPI_FUNC_CTRL_OPMODE_MASK;
> +		val |= ULPI_FUNC_CTRL_OPMODE_NONDRIVING;
> +		usb_phy_io_write(udc->transceiver, val, ULPI_FUNC_CTRL);
> +		break;
> +	default:
> +		dev_dbg(dev, "unknown ci13xxx_udc event\n");
> +		break;
> +	}
> +#endif
> +}
> +
> +static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver = {
> +	.name			= "ci13xxx_imx",
> +	.flags			= CI13XXX_REGS_SHARED |
> +				  CI13XXX_PULLUP_ON_VBUS |
> +				  CI13XXX_DISABLE_STREAMING,
> +	.capoffset		= 0x100,

There's a DEF_CAPOFFSET for this in <linux/usb/chipidea.h>, because this
is really a single most popular offset of the capability registers in
chipidea controllers.

> +
> +	.notify_event		= ci13xxx_imx_notify_event,
> +};
> +
> +static int ci13xxx_imx_probe(struct platform_device *pdev)
> +{
> +	struct platform_device *plat_ci, *phy_pdev;
> +	struct device_node *phy_np;
> +	struct resource *res;
> +	int hub_reset, vbus_pwr;
> +	int ret;
> +
> +	dev_dbg(&pdev->dev, "ci13xxx_imx_probe\n");

Certain people don't take kindly to adding debug prints like this one. :)

> +
> +	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> +	if (phy_np) {
> +		phy_pdev = of_find_device_by_node(phy_np);
> +		if (phy_pdev) {
> +			struct usb_phy *phy;
> +			phy = pdev_to_phy(phy_pdev);
> +			if (phy)
> +				usb_phy_init(phy);
> +		}
> +		of_node_put(phy_np);
> +	}
> +
> +	hub_reset = of_get_named_gpio(pdev->dev.of_node, "fsl,hub-reset", 0);
> +	if (hub_reset > 0 &&
> +	    devm_gpio_request(&pdev->dev, hub_reset, "hub-reset")) {
> +		gpio_direction_output(hub_reset, 0);
> +		udelay(10);
> +		gpio_direction_output(hub_reset, 1);
> +	}
> +
> +	/* we only support host now, so enable vbus here */
> +	vbus_pwr = of_get_named_gpio(pdev->dev.of_node, "fsl,vbus-power", 0);
> +	if (vbus_pwr > 0 &&
> +	    devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
> +		gpio_direction_output(vbus_pwr, 1);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "can't get device resources\n");
> +		return -ENOENT;
> +	}
> +
> +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);

Regarding the instance id here: you probably want to use something more
meaningful than the starting address. I was thinking about stealing id
allocator from Felipe's dwc3 (dwc3_get_device_id()), but decided to
first deal with more important things.

I was considering using idr for this, but it kind of feels like an
overkill for this purpose. So I'm wondering if it makes sense to
generalize dwc3_get_device_id() so that other drivers can make use of
it?

> +	if (!plat_ci) {
> +		dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = platform_device_add_resources(plat_ci, pdev->resource,
> +					    pdev->num_resources);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't add resources to platform device\n");
> +		goto put_platform;
> +	}
> +
> +	ret = platform_device_add_data(plat_ci, &ci13xxx_imx_udc_driver,
> +				       sizeof(ci13xxx_imx_udc_driver));
> +	if (ret)
> +		goto put_platform;
> +
> +	plat_ci->dev.dma_mask =
> +		kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> +	if (!plat_ci->dev.dma_mask) {
> +		ret = -ENOMEM;
> +		goto put_platform;
> +	}
> +	*plat_ci->dev.dma_mask = DMA_BIT_MASK(32);
> +	plat_ci->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +
> +	ret = platform_device_add(plat_ci);
> +	if (ret)
> +		goto free_dma_mask;
> +
> +	pm_runtime_no_callbacks(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +
> +free_dma_mask:
> +	kfree(plat_ci->dev.dma_mask);
> +	plat_ci->dev.dma_mask = NULL;
> +put_platform:
> +	platform_device_put(plat_ci);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id ci13xxx_imx_dt_ids[] = {
> +	{ .compatible = "fsl,imx6q-usboh3", },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver ci13xxx_imx_driver = {
> +	.probe = ci13xxx_imx_probe,
> +	.driver = {
> +		.name = "imx_usboh3",
> +		.of_match_table = ci13xxx_imx_dt_ids,
> +	 },
> +};
> +
> +MODULE_ALIAS("platform:imx_usboh3");
> +
> +static int __init ci13xxx_imx_init(void)
> +{
> +	return platform_driver_register(&ci13xxx_imx_driver);
> +}
> +module_init(ci13xxx_imx_init);
> +
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.5.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Felipe Balbi]

Hi,

On Wed, May 16, 2012 at 02:38:44PM +0300, Alexander Shishkin wrote:
> > +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> 
> Regarding the instance id here: you probably want to use something more
> meaningful than the starting address. I was thinking about stealing id
> allocator from Felipe's dwc3 (dwc3_get_device_id()), but decided to
> first deal with more important things.
> 
> I was considering using idr for this, but it kind of feels like an
> overkill for this purpose. So I'm wondering if it makes sense to
> generalize dwc3_get_device_id() so that other drivers can make use of
> it?

Yeah, I once considered a situation were the platform_bus implementation
would handle that for us. never got to implement it though.

I would be cool to see though. Not sure how Greg would feel about it.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120516/227f265c/attachment.sig>

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Richard Zhao]

On Wed, May 16, 2012 at 02:38:44PM +0300, Alexander Shishkin wrote:
> Richard Zhao <richard.zhao@freescale.com> writes:
> 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> 
> Besides things that were already pointed out, some minor comments below:
Thanks for your review.

> 
> > ---
> >  drivers/usb/chipidea/Makefile      |    2 +-
> >  drivers/usb/chipidea/ci13xxx_imx.c |  177 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 178 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
> >
> > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> > index 0f34c0c..6821385 100644
> > --- a/drivers/usb/chipidea/Makefile
> > +++ b/drivers/usb/chipidea/Makefile
> > @@ -14,5 +14,5 @@ ifneq ($(CONFIG_ARCH_MSM),)
> >  endif
> >  
> >  ifneq ($(CONFIG_ARCH_MXC),)
> > -	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o
> > +	obj-$(CONFIG_USB_CHIPIDEA)	+= phy-imx-utmi.o ci13xxx_imx.o
> >  endif
> > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> > new file mode 100644
> > index 0000000..5f36f07
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > @@ -0,0 +1,177 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/usb/ulpi.h>
> > +#include <linux/usb/gadget.h>
> > +#include <linux/usb/chipidea.h>
> > +#include <linux/usb/otg.h>
> > +#include <linux/dma-mapping.h>
> > +
> > +#include "ci.h"
> > +
> > +#define pdev_to_phy(pdev) \
> > +	((struct usb_phy *)platform_get_drvdata(pdev))
> > +
> > +static void ci13xxx_imx_notify_event(struct ci13xxx *udc, unsigned event)
> > +{
> > +#if 0
> > +	struct device *dev = udc->gadget.dev.parent;
> > +	int val;
> > +
> > +	switch (event) {
> > +	case CI13XXX_CONTROLLER_RESET_EVENT:
> > +		dev_dbg(dev, "CI13XXX_CONTROLLER_RESET_EVENT received\n");
> > +		writel(0, USB_AHBBURST);
> > +		writel(0, USB_AHBMODE);
> > +		break;
> > +	case CI13XXX_CONTROLLER_STOPPED_EVENT:
> > +		dev_dbg(dev, "CI13XXX_CONTROLLER_STOPPED_EVENT received\n");
> > +		/*
> > +		 * Put the transceiver in non-driving mode. Otherwise host
> > +		 * may not detect soft-disconnection.
> > +		 */
> > +		val = usb_phy_io_read(udc->transceiver, ULPI_FUNC_CTRL);
> > +		val &= ~ULPI_FUNC_CTRL_OPMODE_MASK;
> > +		val |= ULPI_FUNC_CTRL_OPMODE_NONDRIVING;
> > +		usb_phy_io_write(udc->transceiver, val, ULPI_FUNC_CTRL);
> > +		break;
> > +	default:
> > +		dev_dbg(dev, "unknown ci13xxx_udc event\n");
> > +		break;
> > +	}
> > +#endif
> > +}
> > +
> > +static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver = {
> > +	.name			= "ci13xxx_imx",
> > +	.flags			= CI13XXX_REGS_SHARED |
> > +				  CI13XXX_PULLUP_ON_VBUS |
> > +				  CI13XXX_DISABLE_STREAMING,
> > +	.capoffset		= 0x100,
> 
> There's a DEF_CAPOFFSET for this in <linux/usb/chipidea.h>, because this
> is really a single most popular offset of the capability registers in
> chipidea controllers.
Thanks.
> 
> > +
> > +	.notify_event		= ci13xxx_imx_notify_event,
> > +};
> > +
> > +static int ci13xxx_imx_probe(struct platform_device *pdev)
> > +{
> > +	struct platform_device *plat_ci, *phy_pdev;
> > +	struct device_node *phy_np;
> > +	struct resource *res;
> > +	int hub_reset, vbus_pwr;
> > +	int ret;
> > +
> > +	dev_dbg(&pdev->dev, "ci13xxx_imx_probe\n");
> 
> Certain people don't take kindly to adding debug prints like this one. :)
Removed. Thanks.
> 
> > +
> > +	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> > +	if (phy_np) {
> > +		phy_pdev = of_find_device_by_node(phy_np);
> > +		if (phy_pdev) {
> > +			struct usb_phy *phy;
> > +			phy = pdev_to_phy(phy_pdev);
> > +			if (phy)
> > +				usb_phy_init(phy);
> > +		}
> > +		of_node_put(phy_np);
> > +	}
> > +
> > +	hub_reset = of_get_named_gpio(pdev->dev.of_node, "fsl,hub-reset", 0);
> > +	if (hub_reset > 0 &&
> > +	    devm_gpio_request(&pdev->dev, hub_reset, "hub-reset")) {
> > +		gpio_direction_output(hub_reset, 0);
> > +		udelay(10);
> > +		gpio_direction_output(hub_reset, 1);
> > +	}
> > +
> > +	/* we only support host now, so enable vbus here */
> > +	vbus_pwr = of_get_named_gpio(pdev->dev.of_node, "fsl,vbus-power", 0);
> > +	if (vbus_pwr > 0 &&
> > +	    devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
> > +		gpio_direction_output(vbus_pwr, 1);
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "can't get device resources\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> 
> Regarding the instance id here: you probably want to use something more
> meaningful than the starting address. I was thinking about stealing id
> allocator from Felipe's dwc3 (dwc3_get_device_id()), but decided to
> first deal with more important things.
hmm... Is dwc3_get_device_id() really meaningful than base address?
It just record the sequence number.

Thanks
Richard
> 
> I was considering using idr for this, but it kind of feels like an
> overkill for this purpose. So I'm wondering if it makes sense to
> generalize dwc3_get_device_id() so that other drivers can make use of
> it?
> 
 

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Alexander Shishkin]

Richard Zhao <richard.zhao@freescale.com> writes:

> On Wed, May 16, 2012 at 02:38:44PM +0300, Alexander Shishkin wrote:
>> > +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
>> 
>> Regarding the instance id here: you probably want to use something more
>> meaningful than the starting address. I was thinking about stealing id
>> allocator from Felipe's dwc3 (dwc3_get_device_id()), but decided to
>> first deal with more important things.
> hmm... Is dwc3_get_device_id() really meaningful than base address?
> It just record the sequence number.

Well, if you indeed have more than one controller, you might want to
know, for example, which one registered first. Or, one might simply not
like the look of "/sys/bus/platform/devices/ci_hdrc.-5898240".

Regards,
--
Alex

[PATCH v1 4/7] usb: chipidea: add imx driver binding

[Richard Zhao]

On Wed, May 16, 2012 at 05:18:47PM +0300, Alexander Shishkin wrote:
> Richard Zhao <richard.zhao@freescale.com> writes:
> 
> > On Wed, May 16, 2012 at 02:38:44PM +0300, Alexander Shishkin wrote:
> >> > +	plat_ci = platform_device_alloc("ci_hdrc", (int)res->start);
> >> 
> >> Regarding the instance id here: you probably want to use something more
> >> meaningful than the starting address. I was thinking about stealing id
> >> allocator from Felipe's dwc3 (dwc3_get_device_id()), but decided to
> >> first deal with more important things.
> > hmm... Is dwc3_get_device_id() really meaningful than base address?
> > It just record the sequence number.
> 
> Well, if you indeed have more than one controller, you might want to
> know, for example, which one registered first. Or, one might simply not
> like the look of "/sys/bus/platform/devices/ci_hdrc.-5898240".
ah, it's singed integer. Then I agree dwc3_get_device_id() is a better
and simple way, and we can find the physical device by looking at
ci_hdrc devicie's parent.

Thanks
Richard
> 
> Regards,
> --
> Alex
> 

[PATCH v1 5/7] ARM: imx6q: correct device name of usbphy and usboh3 clock export

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 arch/arm/mach-imx/clk-imx6q.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 1618989..f7eb965 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -390,7 +390,12 @@ int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");
 	clk_register_clkdev(clk[gpt_ipg_per], "per", "imx-gpt.0");
 	clk_register_clkdev(clk[twd], NULL, "smp_twd");
-	clk_register_clkdev(clk[usboh3], NULL, "usboh3");
+	clk_register_clkdev(clk[usboh3], NULL, "2184000.usboh3");
+	clk_register_clkdev(clk[usboh3], NULL, "2184200.usboh3");
+	clk_register_clkdev(clk[usboh3], NULL, "2184400.usboh3");
+	clk_register_clkdev(clk[usboh3], NULL, "2184600.usboh3");
+	clk_register_clkdev(clk[pll3_usb_otg], NULL, "20c9000.usbphy");
+	clk_register_clkdev(clk[pll3_usb_otg], NULL, "20ca000.usbphy");
 	clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
 	clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
 	clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
-- 
1.7.5.4

[PATCH v1 6/7] ARM: imx6q: add anatop initialization for usb controllers

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 arch/arm/mach-imx/mach-imx6q.c |   60 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 74e44d3..4530c45 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -22,6 +22,7 @@
 #include <linux/pinctrl/machine.h>
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
+#include <linux/mfd/anatop.h>
 #include <asm/smp_twd.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/hardware/gic.h>
@@ -82,6 +83,63 @@ static void __init imx6q_sabrelite_init(void)
 				ksz9021rn_phy_fixup);
 }
 
+static void __init imx6q_post_populate(void)
+{
+	u32 val;
+
+#define HW_ANADIG_USB2_CHRG_DETECT	(0x00000210)
+#define BM_ANADIG_USB2_CHRG_DETECT_EN_B 0x00100000
+#define BM_ANADIG_USB2_CHRG_DETECT_CHK_CHRG_B 0x00080000
+
+#define HW_ANADIG_USB2_PLL_480_CTRL	(0x00000020)
+#define BM_ANADIG_USB2_PLL_480_CTRL_BYPASS 0x00010000
+#define BM_ANADIG_USB2_PLL_480_CTRL_ENABLE 0x00002000
+#define BM_ANADIG_USB2_PLL_480_CTRL_POWER 0x00001000
+#define BM_ANADIG_USB2_PLL_480_CTRL_EN_USB_CLKS 0x00000040
+
+	/* Some phy and power's special controls for host1
+	 * 1. The external charger detector needs to be disabled
+	 * or the signal at DP will be poor
+	 * 2. The PLL's power and output to usb for host 1
+	 * is totally controlled by IC, so the Software only needs
+	 * to enable them at initializtion.
+	 */
+
+	anatop_write_reg(NULL, HW_ANADIG_USB2_CHRG_DETECT,
+			BM_ANADIG_USB2_CHRG_DETECT_EN_B |
+			BM_ANADIG_USB2_CHRG_DETECT_CHK_CHRG_B,
+			~0);
+	anatop_write_reg(NULL, HW_ANADIG_USB2_PLL_480_CTRL, 0,
+			BM_ANADIG_USB2_PLL_480_CTRL_BYPASS);
+
+	val = BM_ANADIG_USB2_PLL_480_CTRL_ENABLE |
+	      BM_ANADIG_USB2_PLL_480_CTRL_POWER |
+	      BM_ANADIG_USB2_PLL_480_CTRL_EN_USB_CLKS;
+	anatop_write_reg(NULL, HW_ANADIG_USB2_PLL_480_CTRL, val, val);
+
+	/* Some phy and power's special controls for otg
+	 * 1. The external charger detector needs to be disabled
+	 * or the signal at DP will be poor
+	 * 2. The EN_USB_CLKS is always enabled.
+	 * The PLL's power is controlled by usb and others who
+	 * use pll3 too.
+	 */
+#define HW_ANADIG_USB1_CHRG_DETECT      (0x000001b0)
+#define BM_ANADIG_USB1_CHRG_DETECT_EN_B 0x00100000
+#define BM_ANADIG_USB1_CHRG_DETECT_CHK_CHRG_B 0x00080000
+
+#define HW_ANADIG_USB1_PLL_480_CTRL     (0x00000010)
+#define BM_ANADIG_USB1_PLL_480_CTRL_EN_USB_CLKS 0x00000040
+
+	anatop_write_reg(NULL, HW_ANADIG_USB1_CHRG_DETECT,
+			BM_ANADIG_USB1_CHRG_DETECT_EN_B
+			| BM_ANADIG_USB1_CHRG_DETECT_CHK_CHRG_B,
+			~0);
+	anatop_write_reg(NULL, HW_ANADIG_USB1_PLL_480_CTRL,
+			BM_ANADIG_USB1_PLL_480_CTRL_EN_USB_CLKS,
+			BM_ANADIG_USB1_PLL_480_CTRL_EN_USB_CLKS);
+}
+
 static void __init imx6q_init_machine(void)
 {
 	/*
@@ -95,6 +153,8 @@ static void __init imx6q_init_machine(void)
 
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 
+	imx6q_post_populate();
+
 	imx6q_pm_init();
 }
 
-- 
1.7.5.4

[PATCH v1 7/7] ARM: dts: imx6q-sabrelite: add usb devices

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |   19 +++++++++++++-
 arch/arm/boot/dts/imx6q.dtsi          |   44 +++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 2f631f2..ccc794e 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -41,10 +41,27 @@
 					status = "okay";
 				};
 			};
-
+			iomuxc at 020e0000 {
+				gpios {
+					pinctrl_gpio_hog: gpiohog {
+						fsl,pins = <1044 0x80000000
+							   144  0x80000000>;	/* MX6Q_PAD_GPIO_17__GPIO_7_12 */
+					};
+				};
+			};
 		};
 
 		aips-bus at 02100000 { /* AIPS2 */
+			usboh3 at 02184000 { /* USB OTG */
+				fsl,vbus-power = <&gpio3 22 0>;
+				status = "okay";
+			};
+
+			usboh3 at 02184200 { /* USB1 */
+				fsl,hub-reset = <&gpio7 12 0>;
+				status = "okay";
+			};
+
 			ethernet at 02188000 {
 				phy-mode = "rgmii";
 				phy-reset-gpios = <&gpio3 23 0>;
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index b5a15c4..863cb74 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -444,12 +444,14 @@
 				};
 			};
 
-			usbphy at 020c9000 { /* USBPHY1 */
+			usbphy1: usbphy at 020c9000 {
+				compatible = "fsl,imx6q-usbphy";
 				reg = <0x020c9000 0x1000>;
 				interrupts = <0 44 0x04>;
 			};
 
-			usbphy at 020ca000 { /* USBPHY2 */
+			usbphy2: usbphy at 020ca000 {
+				compatible = "fsl,imx6q-usbphy";
 				reg = <0x020ca000 0x1000>;
 				interrupts = <0 45 0x04>;
 			};
@@ -485,7 +487,15 @@
 				compatible = "fsl,imx6q-iomuxc";
 				reg = <0x020e0000 0x4000>;
 
+				pinctrl-names = "default";
+				pinctrl-0 = <&pinctrl_gpio_hog>;
+
 				/* shared pinctrl settings */
+				gpios {
+					pinctrl_gpio_hog: gpiohog {
+					};
+				};
+
 				usdhc3 {
 					pinctrl_usdhc3_1: usdhc3grp-1 {
 						fsl,pins = <1273 0x17059	/* MX6Q_PAD_SD3_CMD__USDHC3_CMD */
@@ -550,6 +560,36 @@
 				reg = <0x0217c000 0x4000>;
 			};
 
+			usboh3 at 02184000 { /* USB OTG */
+				compatible = "fsl,imx6q-usboh3";
+				reg = <0x02184000 0x200>;
+				interrupts = <0 43 0x04>;
+				fsl,usbphy = <&usbphy1>;
+				status = "disabled";
+			};
+
+			usboh3 at 02184200 { /* USB1 */
+				compatible = "fsl,imx6q-usboh3";
+				reg = <0x02184200 0x200>;
+				interrupts = <0 40 0x04>;
+				fsl,usbphy = <&usbphy2>;
+				status = "disabled";
+			};
+
+			usboh3 at 02184400 { /* USB2 */
+				compatible = "fsl,imx6q-usboh3";
+				reg = <0x02184400 0x200>;
+				interrupts = <0 41 0x04>;
+				status = "disabled";
+			};
+
+			usboh3 at 02184600 { /* USB3 */
+				compatible = "fsl,imx6q-usboh3";
+				reg = <0x02184600 0x200>;
+				interrupts = <0 42 0x04>;
+				status = "disabled";
+			};
+
 			ethernet at 02188000 {
 				compatible = "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
-- 
1.7.5.4

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Marek Vasut]

Dear Richard Zhao,

> The work is based on usb-next which includes ci13xxx rework by Alexander
> Shishkin.
> 
> Status:
>   - this version only tested host
>   - usbotg controler works at host role
> 
> Todo:
>   - usbh1 has strange system error when enumerate devices for on-board hub.
>   - high speed usb connecton detect failed for the second time. It needs
> hub code hack.
> 
> Thanks Peter Chen for greate help.

Hm, I think we should somehow coordinate, because I think we're working on the 
same thing and we're duplicating work.

> 
> Richard Zhao (7):
>   usb: chipidea: permit user select USB_EHCI_ROOT_HUB_TT
>   usb: chipidea: remove zero check of hw_ep_max
>   usb: chipidea: add imx on-soc utmi phy driver
>   usb: chipidea: add imx driver binding
>   ARM: imx6q: correct device name of usbphy and usboh3 clock export
>   ARM: imx6q: add anatop initialization for usb controllers
>   ARM: dts: imx6q-sabrelite: add usb devices
> 
>  arch/arm/boot/dts/imx6q-sabrelite.dts |   19 ++++-
>  arch/arm/boot/dts/imx6q.dtsi          |   44 ++++++++-
>  arch/arm/mach-imx/clk-imx6q.c         |    7 +-
>  arch/arm/mach-imx/mach-imx6q.c        |   60 +++++++++++
>  drivers/usb/chipidea/Makefile         |    4 +
>  drivers/usb/chipidea/ci13xxx_imx.c    |  177
> +++++++++++++++++++++++++++++++++ drivers/usb/chipidea/core.c           | 
>   2 +-
>  drivers/usb/chipidea/phy-imx-utmi.c   |  140 ++++++++++++++++++++++++++
>  drivers/usb/host/Kconfig              |    2 +-
>  9 files changed, 449 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
>  create mode 100644 drivers/usb/chipidea/phy-imx-utmi.c

Best regards,
Marek Vasut

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Greg KH]

On Tue, May 15, 2012 at 04:28:18PM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> > The work is based on usb-next which includes ci13xxx rework by Alexander
> > Shishkin.
> > 
> > Status:
> >   - this version only tested host
> >   - usbotg controler works at host role
> > 
> > Todo:
> >   - usbh1 has strange system error when enumerate devices for on-board hub.
> >   - high speed usb connecton detect failed for the second time. It needs
> > hub code hack.
> > 
> > Thanks Peter Chen for greate help.
> 
> Hm, I think we should somehow coordinate, because I think we're working on the 
> same thing and we're duplicating work.

Ok, can the two of you work together on this so I don't get conflicting
patches, and so you both can verify it works properly for your systems?

thanks,

greg k-h

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Marek Vasut]

Dear Greg KH,

> On Tue, May 15, 2012 at 04:28:18PM +0200, Marek Vasut wrote:
> > Dear Richard Zhao,
> > 
> > > The work is based on usb-next which includes ci13xxx rework by
> > > Alexander Shishkin.
> > > 
> > > Status:
> > >   - this version only tested host
> > >   - usbotg controler works at host role
> > > 
> > > Todo:
> > >   - usbh1 has strange system error when enumerate devices for on-board
> > >   hub. - high speed usb connecton detect failed for the second time.
> > >   It needs
> > > 
> > > hub code hack.
> > > 
> > > Thanks Peter Chen for greate help.
> > 
> > Hm, I think we should somehow coordinate, because I think we're working
> > on the same thing and we're duplicating work.
> 
> Ok, can the two of you work together on this so I don't get conflicting
> patches, and so you both can verify it works properly for your systems?

Yes, I'd prefer to.

Richard, are these 7 patches the only stuff that I have to apply on top of 
linux-usb/usb-next to get this working?

> thanks,
> 
> greg k-h

Best regards,
Marek Vasut

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Richard Zhao]

On Tue, May 15, 2012 at 06:30:14PM +0200, Marek Vasut wrote:
> Dear Greg KH,
> 
> > On Tue, May 15, 2012 at 04:28:18PM +0200, Marek Vasut wrote:
> > > Dear Richard Zhao,
> > > 
> > > > The work is based on usb-next which includes ci13xxx rework by
> > > > Alexander Shishkin.
> > > > 
> > > > Status:
> > > >   - this version only tested host
> > > >   - usbotg controler works at host role
> > > > 
> > > > Todo:
> > > >   - usbh1 has strange system error when enumerate devices for on-board
> > > >   hub. - high speed usb connecton detect failed for the second time.
> > > >   It needs
> > > > 
> > > > hub code hack.
> > > > 
> > > > Thanks Peter Chen for greate help.
> > > 
> > > Hm, I think we should somehow coordinate, because I think we're working
> > > on the same thing and we're duplicating work.
> > 
> > Ok, can the two of you work together on this so I don't get conflicting
> > patches, and so you both can verify it works properly for your systems?
> 
> Yes, I'd prefer to.
Me too. I think we can start with review each other's patch.
> 
> Richard, are these 7 patches the only stuff that I have to apply on top of 
> linux-usb/usb-next to get this working?
Yes.

Thanks
Richard
> 
> > thanks,
> > 
> > greg k-h
> 
> Best regards,
> Marek Vasut
> 

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Peter Chen]

> > Yes, I'd prefer to.
> Me too. I think we can start with review each other's patch.
> > 
> > Richard, are these 7 patches the only stuff that I have to apply on top of 
> > linux-usb/usb-next to get this working?
> Yes.

Hi Marek and Richard,

The i.mx28 and i.mx6q use the same USB controller and USB PHY,
Your patchset can be totally the same, only different is platform
part. Below are my commnets for your patchset:
- Use drivers/usb/otg/mxs-phy.c as PHY's driver.
- Use drivers/usb/chipidea/ci13xxx_imx.c as controller's driver,
all freescale usb uses the same controller.
- Use DT for platform support, phandle can work around current
multi-phy support problem. 

My proposal how you work together (already discussed with richard)
- Richard picks marek's controller's patch, and add DT support for
i.mx28, send marek, me, shawn privately.
- Marek tunes this DT patched patchset at i.mx28 platform,
and add Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
for patchset, and send to mail list for review
- Richard tests it and give some comments if there is any problems
at i.mx6q.
- Marek re-sends this patchset to mail list for review

Marek, if you agree with it, please take the responsibilities of the
owner with this patchset, and be active during this patchset review
procedure.

-- 

Best Regards,
Peter Chen

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Marek Vasut]

Dear Peter Chen,

> > > Yes, I'd prefer to.
> > 
> > Me too. I think we can start with review each other's patch.
> > 
> > > Richard, are these 7 patches the only stuff that I have to apply on top
> > > of linux-usb/usb-next to get this working?
> > 
> > Yes.
> 
> Hi Marek and Richard,
> 
> The i.mx28 and i.mx6q use the same USB controller and USB PHY,
> Your patchset can be totally the same, only different is platform
> part. Below are my commnets for your patchset:
> - Use drivers/usb/otg/mxs-phy.c as PHY's driver.
> - Use drivers/usb/chipidea/ci13xxx_imx.c as controller's driver,
> all freescale usb uses the same controller.
> - Use DT for platform support, phandle can work around current
> multi-phy support problem.
> 
> My proposal how you work together (already discussed with richard)
> - Richard picks marek's controller's patch, and add DT support for
> i.mx28, send marek, me, shawn privately.
> - Marek tunes this DT patched patchset at i.mx28 platform,
> and add Signed-off-by: Richard Zhao <richard.zhao@freescale.com>

You mean SoB by me, dont you ? ;-)

> for patchset, and send to mail list for review
> - Richard tests it and give some comments if there is any problems
> at i.mx6q.
> - Marek re-sends this patchset to mail list for review
> 
> Marek, if you agree with it, please take the responsibilities of the
> owner with this patchset, and be active during this patchset review
> procedure.

Fine with me, where can I get DT-enabled MXS kernel tree please?

Best regards,
Marek Vasut

[PATCH v1 0/7] preliminary work for adding imx6q_sabrelite usb support

[Shawn Guo]

On Wed, May 16, 2012 at 03:22:48PM +0200, Marek Vasut wrote:
> Fine with me, where can I get DT-enabled MXS kernel tree please?
> 
linux-next

-- 
Regards,
Shawn