[PATCH v2 00/12] add imx usb driver for mx28/6x

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Richard Zhao]

The work is based on  ci13xxx rework done by Alexander Shishkin.

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

Changes since V1:
 - Merge the work of Marek
 - re-implement connect change notify
 - imx6q: move part of code to clk
 - imx6q: add config-on-boot gpios

Richard Zhao (12):
  usb: otg: add notify_connect_change callback
  usb: chipidea: permit driver bindings pass phy pointer
  usb: ehci-hcd: notify phy when connect change
  usb: otg: add basic mxs phy driver support
  usb: chipidea: add imx driver binding
  ARM: imx6q: correct device name of usbphy and usb controller clock
    export
  ARM: imx6q: add config-on-boot gpios
  ARM: imx6q: add usbphy clocks
  ARM: imx6q: disable usb charger detector
  ARM: dts: imx6q-sabrelite: add usb devices
  ARM: mxs: clk_register_clkdev mx28 usb clocks
  ARM: dts: imx28-evk: add usb devices

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |   20 ++
 Documentation/devicetree/bindings/usb/mxs-phy.txt  |   13 ++
 arch/arm/boot/dts/imx28-evk.dts                    |   18 ++
 arch/arm/boot/dts/imx28.dtsi                       |   12 +-
 arch/arm/boot/dts/imx6q-sabrelite.dts              |   25 +++-
 arch/arm/boot/dts/imx6q.dtsi                       |   44 ++++-
 arch/arm/mach-imx/clk-imx6q.c                      |   12 +-
 arch/arm/mach-imx/mach-imx6q.c                     |   60 ++++++
 drivers/clk/mxs/clk-imx28.c                        |    4 +
 drivers/usb/chipidea/Makefile                      |    8 +
 drivers/usb/chipidea/ci.h                          |    2 +
 drivers/usb/chipidea/ci13xxx_imx.c                 |  191 +++++++++++++++++++
 drivers/usb/chipidea/core.c                        |    4 +
 drivers/usb/chipidea/host.c                        |    1 +
 drivers/usb/chipidea/udc.c                         |   11 +-
 drivers/usb/host/ehci-hcd.c                        |    9 +
 drivers/usb/otg/Kconfig                            |    8 +
 drivers/usb/otg/Makefile                           |    1 +
 drivers/usb/otg/mxs-phy.c                          |  196 ++++++++++++++++++++
 include/linux/usb/chipidea.h                       |    3 +
 include/linux/usb/otg.h                            |   13 ++
 21 files changed, 644 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
 create mode 100644 Documentation/devicetree/bindings/usb/mxs-phy.txt
 create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
 create mode 100644 drivers/usb/otg/mxs-phy.c

-- 
1.7.5.4

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Richard Zhao]

This let usb phy driver has a chance to change hw settings when connect
status change.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index 38ab3f4..385641d 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -117,6 +117,10 @@ struct usb_phy {
 	int	(*set_suspend)(struct usb_phy *x,
 				int suspend);
 
+	/* notify phy connect status change */
+	int	(*notify_connect_change)(struct usb_phy *x,
+					 int port,
+					 int connected);
 };
 
 
@@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
 }
 
 static inline int
+usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
+{
+	if (x->notify_connect_change != NULL)
+		return x->notify_connect_change(x, port, connected);
+	else
+		return 0;
+}
+
+static inline int
 otg_start_srp(struct usb_otg *otg)
 {
 	if (otg && otg->start_srp)
-- 
1.7.5.4

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Felipe Balbi]

On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> This let usb phy driver has a chance to change hw settings when connect
> status change.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  include/linux/usb/otg.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> index 38ab3f4..385641d 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -117,6 +117,10 @@ struct usb_phy {
>  	int	(*set_suspend)(struct usb_phy *x,
>  				int suspend);
>  
> +	/* notify phy connect status change */
> +	int	(*notify_connect_change)(struct usb_phy *x,
> +					 int port,
> +					 int connected);
>  };
>  
>  
> @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
>  }
>  
>  static inline int
> +usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
> +{
> +	if (x->notify_connect_change != NULL)

you don't need this != NULL. See that no other wrapper has it. Keep
consistency.

Also, this is the kind of stuff that I have been queueing for Greg so I
should be in Cc.

-- 
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/20120521/8b2dba91/attachment-0001.sig>

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Richard Zhao]

On Mon, May 21, 2012 at 12:26:36PM +0300, Felipe Balbi wrote:
> On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > This let usb phy driver has a chance to change hw settings when connect
> > status change.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  include/linux/usb/otg.h |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > index 38ab3f4..385641d 100644
> > --- a/include/linux/usb/otg.h
> > +++ b/include/linux/usb/otg.h
> > @@ -117,6 +117,10 @@ struct usb_phy {
> >  	int	(*set_suspend)(struct usb_phy *x,
> >  				int suspend);
> >  
> > +	/* notify phy connect status change */
> > +	int	(*notify_connect_change)(struct usb_phy *x,
> > +					 int port,
> > +					 int connected);
> >  };
> >  
> >  
> > @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
> >  }
> >  
> >  static inline int
> > +usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
> > +{
> > +	if (x->notify_connect_change != NULL)
> 
> you don't need this != NULL. See that no other wrapper has it. Keep
> consistency.
notify_connect_change callback is not a must. I just let the phy driver
have a chance to do whatever it needs to do.
> 
> Also, this is the kind of stuff that I have been queueing for Greg so I
> should be in Cc.
Ah, I'm really sorry. I just find out I used the result of get_maintainer
for v1 patchset.

Thanks
Richard
> 
> -- 
> balbi

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Felipe Balbi]

On Mon, May 21, 2012 at 05:36:16PM +0800, Richard Zhao wrote:
> On Mon, May 21, 2012 at 12:26:36PM +0300, Felipe Balbi wrote:
> > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > This let usb phy driver has a chance to change hw settings when connect
> > > status change.
> > > 
> > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > ---
> > >  include/linux/usb/otg.h |   13 +++++++++++++
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > index 38ab3f4..385641d 100644
> > > --- a/include/linux/usb/otg.h
> > > +++ b/include/linux/usb/otg.h
> > > @@ -117,6 +117,10 @@ struct usb_phy {
> > >  	int	(*set_suspend)(struct usb_phy *x,
> > >  				int suspend);
> > >  
> > > +	/* notify phy connect status change */
> > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > +					 int port,
> > > +					 int connected);
> > >  };
> > >  
> > >  
> > > @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
> > >  }
> > >  
> > >  static inline int
> > > +usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
> > > +{
> > > +	if (x->notify_connect_change != NULL)
> > 
> > you don't need this != NULL. See that no other wrapper has it. Keep
> > consistency.
> notify_connect_change callback is not a must. I just let the phy driver
> have a chance to do whatever it needs to do.

what I meant was:

-	if (x->notify_connect_change != NULL)
+	if (x->notify_connect_change)

> > Also, this is the kind of stuff that I have been queueing for Greg so I
> > should be in Cc.
> Ah, I'm really sorry. I just find out I used the result of get_maintainer
> for v1 patchset.

hmm... ok, looks like I should patch MAINTAINERS file for that :-p

Greg, is it ok ?

-- 
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/20120521/52db93ea/attachment.sig>

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Richard Zhao]

On Mon, May 21, 2012 at 12:38:41PM +0300, Felipe Balbi wrote:
> On Mon, May 21, 2012 at 05:36:16PM +0800, Richard Zhao wrote:
> > On Mon, May 21, 2012 at 12:26:36PM +0300, Felipe Balbi wrote:
> > > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > > This let usb phy driver has a chance to change hw settings when connect
> > > > status change.
> > > > 
> > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > > ---
> > > >  include/linux/usb/otg.h |   13 +++++++++++++
> > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > index 38ab3f4..385641d 100644
> > > > --- a/include/linux/usb/otg.h
> > > > +++ b/include/linux/usb/otg.h
> > > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > >  				int suspend);
> > > >  
> > > > +	/* notify phy connect status change */
> > > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > > +					 int port,
> > > > +					 int connected);
> > > >  };
> > > >  
> > > >  
> > > > @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
> > > >  }
> > > >  
> > > >  static inline int
> > > > +usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
> > > > +{
> > > > +	if (x->notify_connect_change != NULL)
> > > 
> > > you don't need this != NULL. See that no other wrapper has it. Keep
> > > consistency.
> > notify_connect_change callback is not a must. I just let the phy driver
> > have a chance to do whatever it needs to do.
> 
> what I meant was:
> 
> -	if (x->notify_connect_change != NULL)
> +	if (x->notify_connect_change)
Ah, Sure.

Thanks
Richard
> 
> > > Also, this is the kind of stuff that I have been queueing for Greg so I
> > > should be in Cc.
> > Ah, I'm really sorry. I just find out I used the result of get_maintainer
> > for v1 patchset.
> 
> hmm... ok, looks like I should patch MAINTAINERS file for that :-p
> 
> Greg, is it ok ?
> 
> -- 
> balbi

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Greg KH]

On Mon, May 21, 2012 at 12:38:41PM +0300, Felipe Balbi wrote:
> On Mon, May 21, 2012 at 05:36:16PM +0800, Richard Zhao wrote:
> > On Mon, May 21, 2012 at 12:26:36PM +0300, Felipe Balbi wrote:
> > > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > > This let usb phy driver has a chance to change hw settings when connect
> > > > status change.
> > > > 
> > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > > ---
> > > >  include/linux/usb/otg.h |   13 +++++++++++++
> > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > index 38ab3f4..385641d 100644
> > > > --- a/include/linux/usb/otg.h
> > > > +++ b/include/linux/usb/otg.h
> > > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > >  				int suspend);
> > > >  
> > > > +	/* notify phy connect status change */
> > > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > > +					 int port,
> > > > +					 int connected);
> > > >  };
> > > >  
> > > >  
> > > > @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
> > > >  }
> > > >  
> > > >  static inline int
> > > > +usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
> > > > +{
> > > > +	if (x->notify_connect_change != NULL)
> > > 
> > > you don't need this != NULL. See that no other wrapper has it. Keep
> > > consistency.
> > notify_connect_change callback is not a must. I just let the phy driver
> > have a chance to do whatever it needs to do.
> 
> what I meant was:
> 
> -	if (x->notify_connect_change != NULL)
> +	if (x->notify_connect_change)
> 
> > > Also, this is the kind of stuff that I have been queueing for Greg so I
> > > should be in Cc.
> > Ah, I'm really sorry. I just find out I used the result of get_maintainer
> > for v1 patchset.
> 
> hmm... ok, looks like I should patch MAINTAINERS file for that :-p
> 
> Greg, is it ok ?

Yes, please send me a patch for MAINTAINERS for this.

greg k-h

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Greg KH]

On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> This let usb phy driver has a chance to change hw settings when connect
> status change.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  include/linux/usb/otg.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> index 38ab3f4..385641d 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -117,6 +117,10 @@ struct usb_phy {
>  	int	(*set_suspend)(struct usb_phy *x,
>  				int suspend);
>  
> +	/* notify phy connect status change */
> +	int	(*notify_connect_change)(struct usb_phy *x,
> +					 int port,
> +					 int connected);
>  };

No, please make this two different callbacks.  As you see in your code
when you implemented this, you really have:
	if (connected) {
		do this...
	} else {
		do that...
	}

So you there is no consolidation in the driver, so just make it 2
callbacks, especially as someone wanted to make connected -1 just for a
tristate, which would be impossible to document properly...

greg k-h


>  
>  
> @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
>  }
>  
>  static inline int
> +usb_phy_notify_connect_change(struct usb_phy *x, int port, int connected)
> +{
> +	if (x->notify_connect_change != NULL)
> +		return x->notify_connect_change(x, port, connected);
> +	else
> +		return 0;
> +}
> +
> +static inline int
>  otg_start_srp(struct usb_otg *otg)
>  {
>  	if (otg && otg->start_srp)
> -- 
> 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 v2 01/12] usb: otg: add notify_connect_change callback

[Marek Vasut]

Dear Greg KH,

> On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > This let usb phy driver has a chance to change hw settings when connect
> > status change.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> > 
> >  include/linux/usb/otg.h |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > index 38ab3f4..385641d 100644
> > --- a/include/linux/usb/otg.h
> > +++ b/include/linux/usb/otg.h
> > @@ -117,6 +117,10 @@ struct usb_phy {
> > 
> >  	int	(*set_suspend)(struct usb_phy *x,
> >  	
> >  				int suspend);
> > 
> > +	/* notify phy connect status change */
> > +	int	(*notify_connect_change)(struct usb_phy *x,
> > +					 int port,
> > +					 int connected);
> > 
> >  };
> 
> No, please make this two different callbacks.  As you see in your code
> when you implemented this, you really have:
> 	if (connected) {
> 		do this...
> 	} else {
> 		do that...
> 	}
> 
> So you there is no consolidation in the driver, so just make it 2
> callbacks, especially as someone wanted to make connected -1 just for a
> tristate, which would be impossible to document properly...

Or he could make connected an enum maybe ?

> 
> greg k-h
> 
> > @@ -252,6 +256,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
> > 
> >  }
> >  
> >  static inline int
> > 
> > +usb_phy_notify_connect_change(struct usb_phy *x, int port, int
> > connected) +{
> > +	if (x->notify_connect_change != NULL)
> > +		return x->notify_connect_change(x, port, connected);
> > +	else
> > +		return 0;
> > +}
> > +
> > +static inline int
> > 
> >  otg_start_srp(struct usb_otg *otg)
> >  {
> >  
> >  	if (otg && otg->start_srp)

Best regards,
Marek Vasut

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Greg KH]

On Mon, May 21, 2012 at 07:36:40PM +0200, Marek Vasut wrote:
> Dear Greg KH,
> 
> > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > This let usb phy driver has a chance to change hw settings when connect
> > > status change.
> > > 
> > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > ---
> > > 
> > >  include/linux/usb/otg.h |   13 +++++++++++++
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > index 38ab3f4..385641d 100644
> > > --- a/include/linux/usb/otg.h
> > > +++ b/include/linux/usb/otg.h
> > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > 
> > >  	int	(*set_suspend)(struct usb_phy *x,
> > >  	
> > >  				int suspend);
> > > 
> > > +	/* notify phy connect status change */
> > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > +					 int port,
> > > +					 int connected);
> > > 
> > >  };
> > 
> > No, please make this two different callbacks.  As you see in your code
> > when you implemented this, you really have:
> > 	if (connected) {
> > 		do this...
> > 	} else {
> > 		do that...
> > 	}
> > 
> > So you there is no consolidation in the driver, so just make it 2
> > callbacks, especially as someone wanted to make connected -1 just for a
> > tristate, which would be impossible to document properly...
> 
> Or he could make connected an enum maybe ?

No, again, that will make nothing easier on the driver end at all.

What's the problem with different functions that people keep resisting?

greg k-h

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Marek Vasut]

Dear Greg KH,

> On Mon, May 21, 2012 at 07:36:40PM +0200, Marek Vasut wrote:
> > Dear Greg KH,
> > 
> > > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > > This let usb phy driver has a chance to change hw settings when
> > > > connect status change.
> > > > 
> > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > > ---
> > > > 
> > > >  include/linux/usb/otg.h |   13 +++++++++++++
> > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > index 38ab3f4..385641d 100644
> > > > --- a/include/linux/usb/otg.h
> > > > +++ b/include/linux/usb/otg.h
> > > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > > 
> > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > >  	
> > > >  				int suspend);
> > > > 
> > > > +	/* notify phy connect status change */
> > > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > > +					 int port,
> > > > +					 int connected);
> > > > 
> > > >  };
> > > 
> > > No, please make this two different callbacks.  As you see in your code
> > > 
> > > when you implemented this, you really have:
> > > 	if (connected) {
> > > 	
> > > 		do this...
> > > 	
> > > 	} else {
> > > 	
> > > 		do that...
> > > 	
> > > 	}
> > > 
> > > So you there is no consolidation in the driver, so just make it 2
> > > callbacks, especially as someone wanted to make connected -1 just for a
> > > tristate, which would be impossible to document properly...
> > 
> > Or he could make connected an enum maybe ?
> 
> No, again, that will make nothing easier on the driver end at all.
> 
> What's the problem with different functions that people keep resisting?

The structure grows a few bytes ... so the kernel eats a bit more ram.

> 
> greg k-h

Best regards,
Marek Vasut

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Greg KH]

On Mon, May 21, 2012 at 08:32:21PM +0200, Marek Vasut wrote:
> Dear Greg KH,
> 
> > On Mon, May 21, 2012 at 07:36:40PM +0200, Marek Vasut wrote:
> > > Dear Greg KH,
> > > 
> > > > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > > > This let usb phy driver has a chance to change hw settings when
> > > > > connect status change.
> > > > > 
> > > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > > > ---
> > > > > 
> > > > >  include/linux/usb/otg.h |   13 +++++++++++++
> > > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > > index 38ab3f4..385641d 100644
> > > > > --- a/include/linux/usb/otg.h
> > > > > +++ b/include/linux/usb/otg.h
> > > > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > > > 
> > > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > > >  	
> > > > >  				int suspend);
> > > > > 
> > > > > +	/* notify phy connect status change */
> > > > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > > > +					 int port,
> > > > > +					 int connected);
> > > > > 
> > > > >  };
> > > > 
> > > > No, please make this two different callbacks.  As you see in your code
> > > > 
> > > > when you implemented this, you really have:
> > > > 	if (connected) {
> > > > 	
> > > > 		do this...
> > > > 	
> > > > 	} else {
> > > > 	
> > > > 		do that...
> > > > 	
> > > > 	}
> > > > 
> > > > So you there is no consolidation in the driver, so just make it 2
> > > > callbacks, especially as someone wanted to make connected -1 just for a
> > > > tristate, which would be impossible to document properly...
> > > 
> > > Or he could make connected an enum maybe ?
> > 
> > No, again, that will make nothing easier on the driver end at all.
> > 
> > What's the problem with different functions that people keep resisting?
> 
> The structure grows a few bytes ... so the kernel eats a bit more ram.

The time it took you to write this email just made up for that extra
ram, sorry :)

It doesn't matter, have people leaned nothing from our past mistakes?

greg k-h

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Marek Vasut]

Dear Greg KH,

> On Mon, May 21, 2012 at 08:32:21PM +0200, Marek Vasut wrote:
> > Dear Greg KH,
> > 
> > > On Mon, May 21, 2012 at 07:36:40PM +0200, Marek Vasut wrote:
> > > > Dear Greg KH,
> > > > 
> > > > > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > > > > This let usb phy driver has a chance to change hw settings when
> > > > > > connect status change.
> > > > > > 
> > > > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > > > > ---
> > > > > > 
> > > > > >  include/linux/usb/otg.h |   13 +++++++++++++
> > > > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > > > index 38ab3f4..385641d 100644
> > > > > > --- a/include/linux/usb/otg.h
> > > > > > +++ b/include/linux/usb/otg.h
> > > > > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > > > > 
> > > > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > > > >  	
> > > > > >  				int suspend);
> > > > > > 
> > > > > > +	/* notify phy connect status change */
> > > > > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > > > > +					 int port,
> > > > > > +					 int connected);
> > > > > > 
> > > > > >  };
> > > > > 
> > > > > No, please make this two different callbacks.  As you see in your
> > > > > code
> > > > > 
> > > > > when you implemented this, you really have:
> > > > > 	if (connected) {
> > > > > 	
> > > > > 		do this...
> > > > > 	
> > > > > 	} else {
> > > > > 	
> > > > > 		do that...
> > > > > 	
> > > > > 	}
> > > > > 
> > > > > So you there is no consolidation in the driver, so just make it 2
> > > > > callbacks, especially as someone wanted to make connected -1 just
> > > > > for a tristate, which would be impossible to document properly...
> > > > 
> > > > Or he could make connected an enum maybe ?
> > > 
> > > No, again, that will make nothing easier on the driver end at all.
> > > 
> > > What's the problem with different functions that people keep resisting?
> > 
> > The structure grows a few bytes ... so the kernel eats a bit more ram.
> 
> The time it took you to write this email just made up for that extra
> ram, sorry :)

Heh :-) But is this email constantly allocated in your kernel space?

> It doesn't matter, have people leaned nothing from our past mistakes?

All right, I won't argue with you. Richard, let's obey.

> greg k-h

Best regards,
Marek Vasut

[PATCH v2 01/12] usb: otg: add notify_connect_change callback

[Richard Zhao]

On Mon, May 21, 2012 at 09:27:50PM +0200, Marek Vasut wrote:
> Dear Greg KH,
> 
> > On Mon, May 21, 2012 at 08:32:21PM +0200, Marek Vasut wrote:
> > > Dear Greg KH,
> > > 
> > > > On Mon, May 21, 2012 at 07:36:40PM +0200, Marek Vasut wrote:
> > > > > Dear Greg KH,
> > > > > 
> > > > > > On Mon, May 21, 2012 at 05:23:46PM +0800, Richard Zhao wrote:
> > > > > > > This let usb phy driver has a chance to change hw settings when
> > > > > > > connect status change.
> > > > > > > 
> > > > > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  include/linux/usb/otg.h |   13 +++++++++++++
> > > > > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > > > > index 38ab3f4..385641d 100644
> > > > > > > --- a/include/linux/usb/otg.h
> > > > > > > +++ b/include/linux/usb/otg.h
> > > > > > > @@ -117,6 +117,10 @@ struct usb_phy {
> > > > > > > 
> > > > > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > > > > >  	
> > > > > > >  				int suspend);
> > > > > > > 
> > > > > > > +	/* notify phy connect status change */
> > > > > > > +	int	(*notify_connect_change)(struct usb_phy *x,
> > > > > > > +					 int port,
> > > > > > > +					 int connected);
> > > > > > > 
> > > > > > >  };
> > > > > > 
> > > > > > No, please make this two different callbacks.  As you see in your
> > > > > > code
> > > > > > 
> > > > > > when you implemented this, you really have:
> > > > > > 	if (connected) {
> > > > > > 	
> > > > > > 		do this...
> > > > > > 	
> > > > > > 	} else {
> > > > > > 	
> > > > > > 		do that...
> > > > > > 	
> > > > > > 	}
> > > > > > 
> > > > > > So you there is no consolidation in the driver, so just make it 2
> > > > > > callbacks, especially as someone wanted to make connected -1 just
> > > > > > for a tristate, which would be impossible to document properly...
> > > > > 
> > > > > Or he could make connected an enum maybe ?
> > > > 
> > > > No, again, that will make nothing easier on the driver end at all.
> > > > 
> > > > What's the problem with different functions that people keep resisting?
> > > 
> > > The structure grows a few bytes ... so the kernel eats a bit more ram.
> > 
> > The time it took you to write this email just made up for that extra
> > ram, sorry :)
> 
> Heh :-) But is this email constantly allocated in your kernel space?
> 
> > It doesn't matter, have people leaned nothing from our past mistakes?
> 
> All right, I won't argue with you. Richard, let's obey.
Sure. I'll change it. I think Greq's point here is maintenance. And
4-byte ram consume don't cost to be noticed.

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

[PATCH v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Richard Zhao]

Sometimes, the driver bindings may know what phy they use.
For example, when using device tree, the usb controller may have a
phandler pointing to usb phy.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/usb/chipidea/ci.h    |    2 ++
 drivers/usb/chipidea/core.c  |    4 ++++
 drivers/usb/chipidea/host.c  |    1 +
 drivers/usb/chipidea/udc.c   |   11 +++++++----
 include/linux/usb/chipidea.h |    3 +++
 5 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 50911f8..71d7080 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -160,6 +160,8 @@ struct ci13xxx {
 
 	struct ci13xxx_udc_driver	*udc_driver;
 	int				vbus_active;
+	/* FIXME: some day, we'll not use global phy */
+	bool				global_phy;
 	struct usb_phy			*transceiver;
 	struct usb_hcd			*hcd;
 };
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 15e03b3..0492786 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -365,6 +365,10 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 
 	ci->dev = dev;
 	ci->udc_driver = dev->platform_data;
+	if (ci->udc_driver->phy)
+		ci->transceiver = ci->udc_driver->phy;
+	else
+		ci->global_phy = 1;
 
 	ret = hw_device_init(ci, base);
 	if (ret < 0) {
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 9eacd21..7bc11a5 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -121,6 +121,7 @@ static int host_start(struct ci13xxx *ci)
 	ehci = hcd_to_ehci(hcd);
 	ehci->caps = ci->hw_bank.cap;
 	ehci->has_hostpc = ci->hw_bank.lpm;
+	ehci->transceiver = ci->transceiver;
 
 	ret = usb_add_hcd(hcd, 0, 0);
 	if (ret)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 51f9694..22139fae 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1687,7 +1687,8 @@ static int udc_start(struct ci13xxx *udc)
 
 	udc->gadget.ep0 = &udc->ep0in->ep;
 
-	udc->transceiver = usb_get_transceiver();
+	if (udc->global_phy) {
+		udc->transceiver = usb_get_transceiver();
 
 	if (udc->udc_driver->flags & CI13XXX_REQUIRE_TRANSCEIVER) {
 		if (udc->transceiver == NULL) {
@@ -1731,7 +1732,8 @@ static int udc_start(struct ci13xxx *udc)
 remove_trans:
 	if (udc->transceiver) {
 		otg_set_peripheral(udc->transceiver->otg, &udc->gadget);
-		usb_put_transceiver(udc->transceiver);
+		if (udc->global_phy)
+			usb_put_transceiver(udc->transceiver);
 	}
 
 	dev_err(dev, "error = %i\n", retval);
@@ -1740,7 +1742,7 @@ remove_dbg:
 unreg_device:
 	device_unregister(&udc->gadget.dev);
 put_transceiver:
-	if (udc->transceiver)
+	if (udc->transceiver && udc->global_phy)
 		usb_put_transceiver(udc->transceiver);
 free_pools:
 	dma_pool_destroy(udc->td_pool);
@@ -1774,7 +1776,8 @@ static void udc_stop(struct ci13xxx *udc)
 
 	if (udc->transceiver) {
 		otg_set_peripheral(udc->transceiver->otg, NULL);
-		usb_put_transceiver(udc->transceiver);
+		if (udc->global_phy)
+			usb_put_transceiver(udc->transceiver);
 	}
 	dbg_remove_files(&udc->gadget.dev);
 	device_unregister(&udc->gadget.dev);
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index edb90d6..c46c9e9 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -5,12 +5,15 @@
 #ifndef __LINUX_USB_CHIPIDEA_H
 #define __LINUX_USB_CHIPIDEA_H
 
+#include <linux/usb/otg.h>
+
 struct ci13xxx;
 struct ci13xxx_udc_driver {
 	const char	*name;
 	/* offset of the capability registers */
 	uintptr_t	 capoffset;
 	unsigned	 power_budget;
+	struct usb_phy	*phy;
 	unsigned long	 flags;
 #define CI13XXX_REGS_SHARED		BIT(0)
 #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)
-- 
1.7.5.4

[PATCH v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Marek Vasut]

Dear Richard Zhao,

> Sometimes, the driver bindings may know what phy they use.
> For example, when using device tree, the usb controller may have a
> phandler pointing to usb phy.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>

Reviewed-by: Marek Vasut <marex@denx.de>

> ---
>  drivers/usb/chipidea/ci.h    |    2 ++
>  drivers/usb/chipidea/core.c  |    4 ++++
>  drivers/usb/chipidea/host.c  |    1 +
>  drivers/usb/chipidea/udc.c   |   11 +++++++----
>  include/linux/usb/chipidea.h |    3 +++
>  5 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 50911f8..71d7080 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -160,6 +160,8 @@ struct ci13xxx {
> 
>  	struct ci13xxx_udc_driver	*udc_driver;
>  	int				vbus_active;
> +	/* FIXME: some day, we'll not use global phy */
> +	bool				global_phy;
>  	struct usb_phy			*transceiver;
>  	struct usb_hcd			*hcd;
>  };
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 15e03b3..0492786 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -365,6 +365,10 @@ static int __devinit ci_hdrc_probe(struct
> platform_device *pdev)
> 
>  	ci->dev = dev;
>  	ci->udc_driver = dev->platform_data;
> +	if (ci->udc_driver->phy)
> +		ci->transceiver = ci->udc_driver->phy;
> +	else
> +		ci->global_phy = 1;
> 
>  	ret = hw_device_init(ci, base);
>  	if (ret < 0) {
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 9eacd21..7bc11a5 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -121,6 +121,7 @@ static int host_start(struct ci13xxx *ci)
>  	ehci = hcd_to_ehci(hcd);
>  	ehci->caps = ci->hw_bank.cap;
>  	ehci->has_hostpc = ci->hw_bank.lpm;
> +	ehci->transceiver = ci->transceiver;
> 
>  	ret = usb_add_hcd(hcd, 0, 0);
>  	if (ret)
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 51f9694..22139fae 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1687,7 +1687,8 @@ static int udc_start(struct ci13xxx *udc)
> 
>  	udc->gadget.ep0 = &udc->ep0in->ep;
> 
> -	udc->transceiver = usb_get_transceiver();
> +	if (udc->global_phy) {
> +		udc->transceiver = usb_get_transceiver();
> 
>  	if (udc->udc_driver->flags & CI13XXX_REQUIRE_TRANSCEIVER) {
>  		if (udc->transceiver == NULL) {
> @@ -1731,7 +1732,8 @@ static int udc_start(struct ci13xxx *udc)
>  remove_trans:
>  	if (udc->transceiver) {
>  		otg_set_peripheral(udc->transceiver->otg, &udc->gadget);
> -		usb_put_transceiver(udc->transceiver);
> +		if (udc->global_phy)
> +			usb_put_transceiver(udc->transceiver);
>  	}
> 
>  	dev_err(dev, "error = %i\n", retval);
> @@ -1740,7 +1742,7 @@ remove_dbg:
>  unreg_device:
>  	device_unregister(&udc->gadget.dev);
>  put_transceiver:
> -	if (udc->transceiver)
> +	if (udc->transceiver && udc->global_phy)
>  		usb_put_transceiver(udc->transceiver);
>  free_pools:
>  	dma_pool_destroy(udc->td_pool);
> @@ -1774,7 +1776,8 @@ static void udc_stop(struct ci13xxx *udc)
> 
>  	if (udc->transceiver) {
>  		otg_set_peripheral(udc->transceiver->otg, NULL);
> -		usb_put_transceiver(udc->transceiver);
> +		if (udc->global_phy)
> +			usb_put_transceiver(udc->transceiver);
>  	}
>  	dbg_remove_files(&udc->gadget.dev);
>  	device_unregister(&udc->gadget.dev);
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index edb90d6..c46c9e9 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -5,12 +5,15 @@
>  #ifndef __LINUX_USB_CHIPIDEA_H
>  #define __LINUX_USB_CHIPIDEA_H
> 
> +#include <linux/usb/otg.h>
> +
>  struct ci13xxx;
>  struct ci13xxx_udc_driver {
>  	const char	*name;
>  	/* offset of the capability registers */
>  	uintptr_t	 capoffset;
>  	unsigned	 power_budget;
> +	struct usb_phy	*phy;
>  	unsigned long	 flags;
>  #define CI13XXX_REGS_SHARED		BIT(0)
>  #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)

Best regards,
Marek Vasut

[PATCH v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Alexander Shishkin]

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

Hi,

> Sometimes, the driver bindings may know what phy they use.
> For example, when using device tree, the usb controller may have a
> phandler pointing to usb phy.

Some comments below:

[snip]
>  	ci->dev = dev;
>  	ci->udc_driver = dev->platform_data;
> +	if (ci->udc_driver->phy)
> +		ci->transceiver = ci->udc_driver->phy;
> +	else
> +		ci->global_phy = 1;

Nitpick: since it's a bool, don't you want to use "true" instead?

[snip]
> +#include <linux/usb/otg.h>
> +
>  struct ci13xxx;
>  struct ci13xxx_udc_driver {
>  	const char	*name;
>  	/* offset of the capability registers */
>  	uintptr_t	 capoffset;
>  	unsigned	 power_budget;
> +	struct usb_phy	*phy;

This structure is really platform information, not per-device
instance. Consider, for example, situation when you have 2 controllers
in one system, they'll be using the same platform structure, but they'll
have different phys. Right?

Regards,
--
Alex

[PATCH v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Marek Vasut]

Dear Alexander Shishkin,

> Richard Zhao <richard.zhao@freescale.com> writes:
> 
> Hi,
> 
> > Sometimes, the driver bindings may know what phy they use.
> > For example, when using device tree, the usb controller may have a
> > phandler pointing to usb phy.
> 
> Some comments below:
> 
> [snip]
> 
> >  	ci->dev = dev;
> >  	ci->udc_driver = dev->platform_data;
> > 
> > +	if (ci->udc_driver->phy)
> > +		ci->transceiver = ci->udc_driver->phy;
> > +	else
> > +		ci->global_phy = 1;
> 
> Nitpick: since it's a bool, don't you want to use "true" instead?
> 
> [snip]
> 
> > +#include <linux/usb/otg.h>
> > +
> > 
> >  struct ci13xxx;
> >  struct ci13xxx_udc_driver {
> >  
> >  	const char	*name;
> >  	/* offset of the capability registers */
> >  	uintptr_t	 capoffset;
> >  	unsigned	 power_budget;
> > 
> > +	struct usb_phy	*phy;
> 
> This structure is really platform information, not per-device
> instance. Consider, for example, situation when you have 2 controllers
> in one system, they'll be using the same platform structure, but they'll
> have different phys. Right?

Not really, won't you allocate separate structure for them?

> 
> Regards,
> --
> Alex

Best regards,
Marek Vasut

[PATCH v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Richard Zhao]

On Tue, May 22, 2012 at 04:07:31PM +0200, Marek Vasut wrote:
> Dear Alexander Shishkin,
> 
> > Richard Zhao <richard.zhao@freescale.com> writes:
> > 
> > Hi,
> > 
> > > Sometimes, the driver bindings may know what phy they use.
> > > For example, when using device tree, the usb controller may have a
> > > phandler pointing to usb phy.
> > 
> > Some comments below:
> > 
> > [snip]
> > 
> > >  	ci->dev = dev;
> > >  	ci->udc_driver = dev->platform_data;
> > > 
> > > +	if (ci->udc_driver->phy)
> > > +		ci->transceiver = ci->udc_driver->phy;
> > > +	else
> > > +		ci->global_phy = 1;
> > 
> > Nitpick: since it's a bool, don't you want to use "true" instead?
Yes. Thanks.
> > 
> > [snip]
> > 
> > > +#include <linux/usb/otg.h>
> > > +
> > > 
> > >  struct ci13xxx;
> > >  struct ci13xxx_udc_driver {
> > >  
> > >  	const char	*name;
> > >  	/* offset of the capability registers */
> > >  	uintptr_t	 capoffset;
> > >  	unsigned	 power_budget;
> > > 
> > > +	struct usb_phy	*phy;
> > 
> > This structure is really platform information, not per-device
> > instance. Consider, for example, situation when you have 2 controllers
> > in one system, they'll be using the same platform structure, but they'll
> > have different phys. Right?
> 
> Not really, won't you allocate separate structure for them?
Finally, we'll get phy instance from phy lib. But I don't know when it
is ready. So I add this code for handy use before phy lib.

For now, it's ok. When I register platform device, it dup the plat data
memory.

Thanks
Richard
> 
> > 
> > Regards,
> > --
> > Alex
> 
> Best regards,
> Marek Vasut
> --
> 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 v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Chen Peter-B29397]

 
> Finally, we'll get phy instance from phy lib. But I don't know when it
> is ready. So I add this code for handy use before phy lib.
> 
not phy lib, but phy driver. lib is different with driver.

> For now, it's ok. When I register platform device, it dup the plat data
> memory.
> 
> Thanks
> Richard
> >
> > >
> > > Regards,
> > > --
> > > Alex
> >
> > Best regards,
> > Marek Vasut
> > --
> > 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 v2 02/12] usb: chipidea: permit driver bindings pass phy pointer

[Alexander Shishkin]

Marek Vasut <marex@denx.de> writes:

> Dear Alexander Shishkin,
>
>> Richard Zhao <richard.zhao@freescale.com> writes:
>> 
>> Hi,
>> 
>> > Sometimes, the driver bindings may know what phy they use.
>> > For example, when using device tree, the usb controller may have a
>> > phandler pointing to usb phy.
>> 
>> Some comments below:
>> 
>> [snip]
>> 
>> >  	ci->dev = dev;
>> >  	ci->udc_driver = dev->platform_data;
>> > 
>> > +	if (ci->udc_driver->phy)
>> > +		ci->transceiver = ci->udc_driver->phy;
>> > +	else
>> > +		ci->global_phy = 1;
>> 
>> Nitpick: since it's a bool, don't you want to use "true" instead?
>> 
>> [snip]
>> 
>> > +#include <linux/usb/otg.h>
>> > +
>> > 
>> >  struct ci13xxx;
>> >  struct ci13xxx_udc_driver {
>> >  
>> >  	const char	*name;
>> >  	/* offset of the capability registers */
>> >  	uintptr_t	 capoffset;
>> >  	unsigned	 power_budget;
>> > 
>> > +	struct usb_phy	*phy;
>> 
>> This structure is really platform information, not per-device
>> instance. Consider, for example, situation when you have 2 controllers
>> in one system, they'll be using the same platform structure, but they'll
>> have different phys. Right?
>
> Not really, won't you allocate separate structure for them?

Currently, it's a static structure, see 05/12 and my comment there.

Regards,
--
Alex

[PATCH v2 03/12] usb: ehci-hcd: notify phy when connect change

[Richard Zhao]

It still has below limitations:
- it does not work for multi-phy ehci
- the best place is after debounce, but I can't get
  phy there

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

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 5597e60..389252e 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -928,6 +928,15 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
 			pstatus = ehci_readl(ehci,
 					 &ehci->regs->port_status[i]);
 
+			/* FIXME:
+			 * - it does not work for multi-phy ehci
+			 * - the best place is after debounce, but I can't get
+			 *   phy there
+			 */
+			if ((pstatus & PORT_CSC) && ehci->transceiver)
+				usb_phy_notify_connect_change(ehci->transceiver,
+						i, pstatus & PORT_CONNECT);
+
 			if (pstatus & PORT_OWNER)
 				continue;
 			if (!(test_bit(i, &ehci->suspended_ports) &&
-- 
1.7.5.4

[PATCH v2 03/12] usb: ehci-hcd: notify phy when connect change

[Marek Vasut]

Dear Richard Zhao,

> It still has below limitations:
> - it does not work for multi-phy ehci
> - the best place is after debounce, but I can't get
>   phy there

Now, you'll laugh as this would be an obviously stupid question -- but where 
exactly does the debounce happen in the code?

> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/usb/host/ehci-hcd.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 5597e60..389252e 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -928,6 +928,15 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
>  			pstatus = ehci_readl(ehci,
>  					 &ehci->regs->port_status[i]);
> 
> +			/* FIXME:
> +			 * - it does not work for multi-phy ehci
> +			 * - the best place is after debounce, but I can't get
> +			 *   phy there
> +			 */
> +			if ((pstatus & PORT_CSC) && ehci->transceiver)
> +				usb_phy_notify_connect_change(ehci->transceiver,
> +						i, pstatus & PORT_CONNECT);
> +
>  			if (pstatus & PORT_OWNER)
>  				continue;
>  			if (!(test_bit(i, &ehci->suspended_ports) &&

Best regards,
Marek Vasut

[PATCH v2 03/12] usb: ehci-hcd: notify phy when connect change

[Richard Zhao]

Hi Marek,

On Tue, May 22, 2012 at 06:23:49AM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> > It still has below limitations:
> > - it does not work for multi-phy ehci
> > - the best place is after debounce, but I can't get
> >   phy there
> 
> Now, you'll laugh as this would be an obviously stupid question -- but where 
> exactly does the debounce happen in the code?
I meant hub_port_debounce function at drivers/usb/core/hub.c.

Thanks
Richard

[PATCH v2 04/12] usb: otg: add basic mxs phy driver support

[Richard Zhao]

mxs phy is used in Freescale i.MX SoCs, for example
imx23, imx28, imx6Q. This patch adds the basic host
support.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Peter Chen <peter.chen@freescale.com>
---
 Documentation/devicetree/bindings/usb/mxs-phy.txt |   13 ++
 drivers/usb/otg/Kconfig                           |    8 +
 drivers/usb/otg/Makefile                          |    1 +
 drivers/usb/otg/mxs-phy.c                         |  196 +++++++++++++++++++++
 4 files changed, 218 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/mxs-phy.txt
 create mode 100644 drivers/usb/otg/mxs-phy.c

diff --git a/Documentation/devicetree/bindings/usb/mxs-phy.txt b/Documentation/devicetree/bindings/usb/mxs-phy.txt
new file mode 100644
index 0000000..5835b27
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/mxs-phy.txt
@@ -0,0 +1,13 @@
+* Freescale MXS USB Phy Device
+
+Required properties:
+- compatible: Should be "fsl,imx23-usbphy"
+- reg: Should contain registers location and length
+- interrupts: Should contain phy interrupt
+
+Example:
+usbphy1: usbphy at 020c9000 {
+	compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
+	reg = <0x020c9000 0x1000>;
+	interrupts = <0 44 0x04>;
+};
diff --git a/drivers/usb/otg/Kconfig b/drivers/usb/otg/Kconfig
index 5c87db0..3a9bc44 100644
--- a/drivers/usb/otg/Kconfig
+++ b/drivers/usb/otg/Kconfig
@@ -116,6 +116,14 @@ config FSL_USB2_OTG
 	help
 	  Enable this to support Freescale USB OTG transceiver.
 
+config USB_MXS_PHY
+	tristate "Freescale MXS USB PHY support"
+	select USB_OTG_UTILS
+	help
+	  Enable this to support the Freescale MXS USB PHY.
+
+	  MXS Phy is used by some of the i.MX SoCs, for example imx23/28/6x.
+
 config USB_MV_OTG
 	tristate "Marvell USB OTG support"
 	depends on USB_EHCI_MV && USB_MV_UDC && USB_SUSPEND
diff --git a/drivers/usb/otg/Makefile b/drivers/usb/otg/Makefile
index 41aa509..a844b8d 100644
--- a/drivers/usb/otg/Makefile
+++ b/drivers/usb/otg/Makefile
@@ -20,4 +20,5 @@ obj-$(CONFIG_USB_MSM_OTG)	+= msm_otg.o
 obj-$(CONFIG_AB8500_USB)	+= ab8500-usb.o
 fsl_usb2_otg-objs		:= fsl_otg.o otg_fsm.o
 obj-$(CONFIG_FSL_USB2_OTG)	+= fsl_usb2_otg.o
+obj-$(CONFIG_USB_MXS_PHY)	+= mxs-phy.o
 obj-$(CONFIG_USB_MV_OTG)	+= mv_otg.o
diff --git a/drivers/usb/otg/mxs-phy.c b/drivers/usb/otg/mxs-phy.c
new file mode 100644
index 0000000..cc232ee
--- /dev/null
+++ b/drivers/usb/otg/mxs-phy.c
@@ -0,0 +1,196 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012 Marek Vasut <marex@denx.de>
+ * on behalf of DENX Software Engineering GmbH
+ *
+ * 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 "mxs_phy"
+
+#define HW_USBPHY_PWD				0x00
+#define HW_USBPHY_CTRL				0x30
+#define HW_USBPHY_CTRL_SET			0x34
+#define HW_USBPHY_CTRL_CLR			0x38
+
+#define BM_USBPHY_CTRL_SFTRST			BIT(31)
+#define BM_USBPHY_CTRL_CLKGATE			BIT(30)
+#define BM_USBPHY_CTRL_ENUTMILEVEL3		BIT(15)
+#define BM_USBPHY_CTRL_ENUTMILEVEL2		BIT(14)
+#define BM_USBPHY_CTRL_ENHOSTDISCONDETECT	BIT(1)
+
+struct mxs_phy {
+	struct usb_phy phy;
+	struct clk *clk;
+	int connected:1;
+};
+
+#define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
+
+static void mxs_phy_hw_init(struct mxs_phy *mxs_phy)
+{
+	void __iomem *base = mxs_phy->phy.io_priv;
+
+	writel_relaxed(BM_USBPHY_CTRL_CLKGATE, base + HW_USBPHY_CTRL_CLR);
+
+	/* Reset USBPHY module */
+	writel_relaxed(BM_USBPHY_CTRL_SFTRST, base + HW_USBPHY_CTRL_SET);
+	udelay(10);
+
+	/* Remove CLKGATE and SFTRST */
+	writel_relaxed(BM_USBPHY_CTRL_CLKGATE | BM_USBPHY_CTRL_SFTRST,
+			base + HW_USBPHY_CTRL_CLR);
+	udelay(10);
+
+	/* Power up the PHY */
+	writel_relaxed(0, base + HW_USBPHY_PWD);
+
+	/* enable FS/LS device */
+	writel_relaxed(BM_USBPHY_CTRL_ENUTMILEVEL2 |
+			BM_USBPHY_CTRL_ENUTMILEVEL3,
+			base + HW_USBPHY_CTRL_SET);
+
+}
+
+static int mxs_phy_init(struct usb_phy *phy)
+{
+	struct mxs_phy *mxs_phy = to_mxs_phy(phy);
+
+	clk_prepare_enable(mxs_phy->clk);
+	mxs_phy_hw_init(mxs_phy);
+
+	return 0;
+}
+
+static void mxs_phy_shutdown(struct usb_phy *phy)
+{
+	struct mxs_phy *mxs_phy = to_mxs_phy(phy);
+
+	writel_relaxed(BM_USBPHY_CTRL_CLKGATE,
+			phy->io_priv + HW_USBPHY_CTRL_SET);
+
+	clk_disable_unprepare(mxs_phy->clk);
+}
+
+static int mxs_phy_connect_change(struct usb_phy *phy, int port, int connected)
+{
+	struct mxs_phy *mxs_phy = to_mxs_phy(phy);
+
+	dev_dbg(phy->dev, "connect change: port %d, connected %d\n", port, connected);
+
+	if (connected) {
+		if (mxs_phy->connected)
+			return 0;
+		mxs_phy_hw_init(to_mxs_phy(phy));
+		writel_relaxed(BM_USBPHY_CTRL_ENHOSTDISCONDETECT,
+				phy->io_priv + HW_USBPHY_CTRL_SET);
+		mxs_phy->connected = 1;
+	} else {
+		if (!mxs_phy->connected)
+			return 0;
+		writel_relaxed(BM_USBPHY_CTRL_ENHOSTDISCONDETECT,
+				phy->io_priv + HW_USBPHY_CTRL_CLR);
+		mxs_phy->connected = 0;
+	}
+
+	return 0;
+}
+
+static int mxs_phy_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *base;
+	struct clk *clk;
+	struct mxs_phy *mxs_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)) {
+		dev_err(&pdev->dev, "can't get the clock!");
+		return PTR_ERR(clk);
+	}
+
+	mxs_phy = devm_kzalloc(&pdev->dev, sizeof(*mxs_phy), GFP_KERNEL);
+	if (!mxs_phy) {
+		dev_err(&pdev->dev, "Failed to allocate USB PHY structure!\n");
+		return -ENOMEM;
+	}
+
+	mxs_phy->phy.io_priv			= base;
+	mxs_phy->phy.dev			= &pdev->dev;
+	mxs_phy->phy.label			= DRIVER_NAME;
+	mxs_phy->phy.init			= mxs_phy_init;
+	mxs_phy->phy.shutdown			= mxs_phy_shutdown;
+	mxs_phy->phy.notify_connect_change	= mxs_phy_connect_change;
+
+	ATOMIC_INIT_NOTIFIER_HEAD(&mxs_phy->phy.notifier);
+
+	mxs_phy->clk = clk;
+
+	platform_set_drvdata(pdev, &mxs_phy->phy);
+
+	return 0;
+}
+
+static int __devexit mxs_phy_remove(struct platform_device *pdev)
+{
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id mxs_phy_dt_ids[] = {
+	{ .compatible = "fsl,imx23-usbphy", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver mxs_phy_driver = {
+	.probe = mxs_phy_probe,
+	.remove = __devexit_p(mxs_phy_remove),
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = mxs_phy_dt_ids,
+	 },
+};
+
+static int __init mxs_phy_module_init(void)
+{
+	return platform_driver_register(&mxs_phy_driver);
+}
+postcore_initcall(mxs_phy_module_init);
+
+static void __exit mxs_phy_module_exit(void)
+{
+	platform_driver_unregister(&mxs_phy_driver);
+}
+module_exit(mxs_phy_module_exit);
+
+MODULE_ALIAS("platform:mxs-usb-phy");
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
+MODULE_DESCRIPTION("Freescale MXS USB PHY driver");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao]

Just add host support.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Peter Chen <peter.chen@freescale.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt        |   20 ++
 drivers/usb/chipidea/Makefile                      |    8 +
 drivers/usb/chipidea/ci13xxx_imx.c                 |  191 ++++++++++++++++++++
 3 files changed, 219 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
 create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
new file mode 100644
index 0000000..beb75d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -0,0 +1,20 @@
+* Freescale i.MX ci13xxx usb controllers
+
+Required properties:
+- compatible: Should be "fsl,imx31-usb"
+- reg: Should contain registers location and length
+- interrupts: Should contain controller interrupt
+
+Optional properties:
+- fsl,usbphy: phandler of usb phy that connects to the only one port
+- fsl,hub-reset-gpios: gpio used to reset on-board usb hub
+- fsl,vbus-power-gpios: gpio used to set vbus power of the only one port
+
+Examples:
+usb at 02184000 { /* USB OTG */
+	compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
+	reg = <0x02184000 0x200>;
+	interrupts = <0 43 0x04>;
+	fsl,usbphy = <&usbphy1>;
+	fsl,vbus-power-gpios = <&gpio3 22 0>;
+};
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index cc34937..ffa2f63 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -12,3 +12,11 @@ endif
 ifneq ($(CONFIG_ARCH_MSM),)
 	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
 endif
+
+ifneq ($(CONFIG_ARCH_MXC),)
+	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
+else
+  ifneq ($(CONFIG_ARCH_MXS),)
+	  obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
+  endif
+endif
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
new file mode 100644
index 0000000..9f64b3c
--- /dev/null
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012 Marek Vasut <marex@denx.de>
+ * on behalf of DENX Software Engineering GmbH
+ *
+ * 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/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/usb/chipidea.h>
+#include <linux/usb/ehci_def.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/chipidea.h>
+#include <linux/usb/otg.h>
+#include <linux/dma-mapping.h>
+#include <linux/clk.h>
+#include <linux/string.h>
+
+#include "ci.h"
+
+#define PORT0_STATUS	0x184
+
+#define pdev_to_phy(pdev) \
+	((struct usb_phy *)platform_get_drvdata(pdev))
+
+struct ci13xxx_imx_data {
+	struct device_node *phy_np;
+	struct usb_phy *phy;
+	struct platform_device *ci_pdev;
+	struct clk *clk;
+	int gpio_hub_rst, gpio_vbus_pwr;
+};
+
+static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver __devinitdata  = {
+	.name			= "ci13xxx_imx",
+	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
+				  CI13XXX_PULLUP_ON_VBUS |
+				  CI13XXX_DISABLE_STREAMING,
+	.capoffset		= DEF_CAPOFFSET,
+};
+
+static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
+{
+	struct ci13xxx_imx_data *data;
+	struct platform_device *plat_ci, *phy_pdev;
+	struct platform_device_info ci_pdevinfo;
+	struct device_node *phy_np;
+	struct resource *res;
+	int vbus_pwr;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
+		return -ENOMEM;
+	}
+	data->gpio_hub_rst = -1;
+	data->gpio_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;
+	}
+
+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk)) {
+		dev_err(&pdev->dev, "Failed to get clock!\n");
+		return PTR_ERR(data->clk);
+	}
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to prepare or enable clock!\n");
+		return ret;
+	}
+
+	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
+	if (phy_np) {
+		data->phy_np = 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 &&
+			    try_module_get(phy_pdev->dev.driver->owner)) {
+				usb_phy_init(phy);
+				data->phy = phy;
+			}
+		}
+	}
+
+	/* we only support host now, so enable vbus here */
+	vbus_pwr = of_get_named_gpio(pdev->dev.of_node,
+					"fsl,vbus-power-gpios", 0);
+	if (gpio_is_valid(vbus_pwr) &&
+	    !devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
+		gpio_direction_output(vbus_pwr, 1);
+		data->gpio_hub_rst = vbus_pwr;
+	}
+
+	ci13xxx_imx_udc_driver.phy = data->phy;
+	memset(&ci_pdevinfo, 0, sizeof(ci_pdevinfo));
+	ci_pdevinfo.parent = &pdev->dev;
+	ci_pdevinfo.name = "ci_hdrc";
+	ci_pdevinfo.id = (int)res->start;
+	ci_pdevinfo.res = pdev->resource;
+	ci_pdevinfo.num_res = pdev->num_resources;
+	ci_pdevinfo.data = &ci13xxx_imx_udc_driver;
+	ci_pdevinfo.size_data = sizeof(ci13xxx_imx_udc_driver);
+	ci_pdevinfo.dma_mask = DMA_BIT_MASK(32);
+
+	plat_ci = platform_device_register_full(&ci_pdevinfo);
+	if (IS_ERR(plat_ci)) {
+		dev_err(&pdev->dev, "can't register ci_hdrc platform device\n");
+		ret = PTR_ERR(plat_ci);
+		goto put_np;
+	}
+
+	data->ci_pdev = plat_ci;
+	platform_set_drvdata(pdev, data);
+
+	pm_runtime_no_callbacks(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	return 0;
+
+put_np:
+	if (phy_np)
+		of_node_put(phy_np);
+	return ret;
+}
+
+static int __devexit ci13xxx_imx_remove(struct platform_device *pdev)
+{
+	struct ci13xxx_imx_data *data = platform_get_drvdata(pdev);
+
+	kfree(data->ci_pdev->dev.dma_mask);
+	data->ci_pdev->dev.dma_mask = NULL;
+	platform_device_unregister(data->ci_pdev);
+
+	if (data->gpio_vbus_pwr != -1)
+		gpio_direction_output(data->gpio_vbus_pwr, 0);
+	if (data->gpio_hub_rst != -1)
+		gpio_direction_output(data->gpio_hub_rst, 0);
+
+	if (data->phy) {
+		usb_phy_shutdown(data->phy);
+		module_put(data->phy->dev->driver->owner);
+	}
+
+	of_node_put(data->phy_np);
+
+	clk_disable_unprepare(data->clk);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id ci13xxx_imx_dt_ids[] = {
+	{ .compatible = "fsl,imx31-usb", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver ci13xxx_imx_driver = {
+	.probe = ci13xxx_imx_probe,
+	.remove = __devexit_p(ci13xxx_imx_remove),
+	.driver = {
+		.name = "imx_usb",
+		.owner = THIS_MODULE,
+		.of_match_table = ci13xxx_imx_dt_ids,
+	 },
+};
+
+module_platform_driver(ci13xxx_imx_driver);
+
+MODULE_ALIAS("platform:imx-usb");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CI13xxx i.MX USB binding");
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_AUTHOR("Richard Zhao <richard.zhao@freescale.com>");
-- 
1.7.5.4

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Marek Vasut]

Dear Richard Zhao,

> Just add host support.

Maybe rephrase it to something like "This patch supports only the host-mode 
functionality so far."

> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Peter Chen <peter.chen@freescale.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |   20 ++
>  drivers/usb/chipidea/Makefile                      |    8 +
>  drivers/usb/chipidea/ci13xxx_imx.c                 |  191
> ++++++++++++++++++++ 3 files changed, 219 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode
> 100644
> index 0000000..beb75d6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -0,0 +1,20 @@
> +* Freescale i.MX ci13xxx usb controllers
> +
> +Required properties:
> +- compatible: Should be "fsl,imx31-usb"
> +- reg: Should contain registers location and length
> +- interrupts: Should contain controller interrupt
> +
> +Optional properties:
> +- fsl,usbphy: phandler of usb phy that connects to the only one port
> +- fsl,hub-reset-gpios: gpio used to reset on-board usb hub
> +- fsl,vbus-power-gpios: gpio used to set vbus power of the only one port
> +
> +Examples:
> +usb at 02184000 { /* USB OTG */
> +	compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
> +	reg = <0x02184000 0x200>;
> +	interrupts = <0 43 0x04>;
> +	fsl,usbphy = <&usbphy1>;
> +	fsl,vbus-power-gpios = <&gpio3 22 0>;
> +};
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index cc34937..ffa2f63 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -12,3 +12,11 @@ endif
>  ifneq ($(CONFIG_ARCH_MSM),)
>  	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
>  endif
> +
> +ifneq ($(CONFIG_ARCH_MXC),)
> +	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> +else
> +  ifneq ($(CONFIG_ARCH_MXS),)
> +	  obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> +  endif
> +endif

I think someone commented on these patches (my previous set of patches 
actually), that this should be fixed in a way that you can actually select which 
bindings you want to compile in (and not limit it via Makefile like it's now).

> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> b/drivers/usb/chipidea/ci13xxx_imx.c new file mode 100644
> index 0000000..9f64b3c
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -0,0 +1,191 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright (C) 2012 Marek Vasut <marex@denx.de>
> + * on behalf of DENX Software Engineering GmbH
> + *
> + * 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/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/usb/chipidea.h>
> +#include <linux/usb/ehci_def.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/chipidea.h>
> +#include <linux/usb/otg.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/clk.h>
> +#include <linux/string.h>
> +
> +#include "ci.h"
> +
> +#define PORT0_STATUS	0x184
> +
> +#define pdev_to_phy(pdev) \
> +	((struct usb_phy *)platform_get_drvdata(pdev))
> +
> +struct ci13xxx_imx_data {
> +	struct device_node *phy_np;
> +	struct usb_phy *phy;
> +	struct platform_device *ci_pdev;
> +	struct clk *clk;
> +	int gpio_hub_rst, gpio_vbus_pwr;
> +};
> +
> +static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver __devinitdata  = {
> +	.name			= "ci13xxx_imx",
> +	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> +				  CI13XXX_PULLUP_ON_VBUS |
> +				  CI13XXX_DISABLE_STREAMING,
> +	.capoffset		= DEF_CAPOFFSET,
> +};
> +
> +static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
> +{
> +	struct ci13xxx_imx_data *data;
> +	struct platform_device *plat_ci, *phy_pdev;
> +	struct platform_device_info ci_pdevinfo;
> +	struct device_node *phy_np;
> +	struct resource *res;
> +	int vbus_pwr;
> +	int ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> +		return -ENOMEM;
> +	}
> +	data->gpio_hub_rst = -1;
> +	data->gpio_vbus_pwr = -1;

This really freaks me out. Maybe we should have some kind of a regulator that 
manages the VBUS and let this driver toggle the regulator when fitting? That'd 
solve the problem to some extent, as other people would be able to supply NOP 
regulator.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "can't get device resources!\n");

Can't ... capital "C" ;-)

> +		return -ENOENT;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev, "Failed to get clock!\n");
> +		return PTR_ERR(data->clk);
> +	}
> +
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to prepare or enable clock!\n");
> +		return ret;
> +	}
> +
> +	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> +	if (phy_np) {
> +		data->phy_np = phy_np;
> +		phy_pdev = of_find_device_by_node(phy_np);
> +		if (phy_pdev) {
> +			struct usb_phy *phy;

Do we really want to define new variables in the middle of a code?

> +			phy = pdev_to_phy(phy_pdev);
> +			if (phy &&
> +			    try_module_get(phy_pdev->dev.driver->owner)) {
> +				usb_phy_init(phy);
> +				data->phy = phy;
> +			}
> +		}
> +	}
> +
> +	/* we only support host now, so enable vbus here */
> +	vbus_pwr = of_get_named_gpio(pdev->dev.of_node,
> +					"fsl,vbus-power-gpios", 0);

(This one is for my personal enlightment) Why is the property called "gpios" 
(plural) if it's only one GPIO?

> +	if (gpio_is_valid(vbus_pwr) &&
> +	    !devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
> +		gpio_direction_output(vbus_pwr, 1);
> +		data->gpio_hub_rst = vbus_pwr;
> +	}
> +
> +	ci13xxx_imx_udc_driver.phy = data->phy;
> +	memset(&ci_pdevinfo, 0, sizeof(ci_pdevinfo));
> +	ci_pdevinfo.parent = &pdev->dev;
> +	ci_pdevinfo.name = "ci_hdrc";
> +	ci_pdevinfo.id = (int)res->start;
> +	ci_pdevinfo.res = pdev->resource;
> +	ci_pdevinfo.num_res = pdev->num_resources;
> +	ci_pdevinfo.data = &ci13xxx_imx_udc_driver;
> +	ci_pdevinfo.size_data = sizeof(ci13xxx_imx_udc_driver);
> +	ci_pdevinfo.dma_mask = DMA_BIT_MASK(32);
> +
> +	plat_ci = platform_device_register_full(&ci_pdevinfo);
> +	if (IS_ERR(plat_ci)) {
> +		dev_err(&pdev->dev, "can't register ci_hdrc platform device\n");
> +		ret = PTR_ERR(plat_ci);
> +		goto put_np;
> +	}
> +
> +	data->ci_pdev = plat_ci;
> +	platform_set_drvdata(pdev, data);
> +
> +	pm_runtime_no_callbacks(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +
> +put_np:
> +	if (phy_np)
> +		of_node_put(phy_np);
> +	return ret;
> +}
> +
> +static int __devexit ci13xxx_imx_remove(struct platform_device *pdev)
> +{
> +	struct ci13xxx_imx_data *data = platform_get_drvdata(pdev);
> +
> +	kfree(data->ci_pdev->dev.dma_mask);
> +	data->ci_pdev->dev.dma_mask = NULL;
> +	platform_device_unregister(data->ci_pdev);
> +
> +	if (data->gpio_vbus_pwr != -1)
> +		gpio_direction_output(data->gpio_vbus_pwr, 0);
> +	if (data->gpio_hub_rst != -1)
> +		gpio_direction_output(data->gpio_hub_rst, 0);
> +
> +	if (data->phy) {
> +		usb_phy_shutdown(data->phy);
> +		module_put(data->phy->dev->driver->owner);
> +	}
> +
> +	of_node_put(data->phy_np);
> +
> +	clk_disable_unprepare(data->clk);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ci13xxx_imx_dt_ids[] = {
> +	{ .compatible = "fsl,imx31-usb", },

Wow, now it's imx31-usb ?

> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver ci13xxx_imx_driver = {
> +	.probe = ci13xxx_imx_probe,
> +	.remove = __devexit_p(ci13xxx_imx_remove),
> +	.driver = {
> +		.name = "imx_usb",
> +		.owner = THIS_MODULE,
> +		.of_match_table = ci13xxx_imx_dt_ids,
> +	 },
> +};
> +
> +module_platform_driver(ci13xxx_imx_driver);
> +

Richard, thanks for your work on these patches so far. Sorry for taking so long 
with reviewing these.

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 22, 2012 at 06:30:56AM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> > Just add host support.
> 
> Maybe rephrase it to something like "This patch supports only the host-mode 
> functionality so far."
Thanks.
> 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Peter Chen <peter.chen@freescale.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |   20 ++
> >  drivers/usb/chipidea/Makefile                      |    8 +
> >  drivers/usb/chipidea/ci13xxx_imx.c                 |  191
> > ++++++++++++++++++++ 3 files changed, 219 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode
> > 100644
> > index 0000000..beb75d6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -0,0 +1,20 @@
> > +* Freescale i.MX ci13xxx usb controllers
> > +
> > +Required properties:
> > +- compatible: Should be "fsl,imx31-usb"
> > +- reg: Should contain registers location and length
> > +- interrupts: Should contain controller interrupt
> > +
> > +Optional properties:
> > +- fsl,usbphy: phandler of usb phy that connects to the only one port
> > +- fsl,hub-reset-gpios: gpio used to reset on-board usb hub
> > +- fsl,vbus-power-gpios: gpio used to set vbus power of the only one port
> > +
> > +Examples:
> > +usb at 02184000 { /* USB OTG */
> > +	compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
> > +	reg = <0x02184000 0x200>;
> > +	interrupts = <0 43 0x04>;
> > +	fsl,usbphy = <&usbphy1>;
> > +	fsl,vbus-power-gpios = <&gpio3 22 0>;
> > +};
> > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> > index cc34937..ffa2f63 100644
> > --- a/drivers/usb/chipidea/Makefile
> > +++ b/drivers/usb/chipidea/Makefile
> > @@ -12,3 +12,11 @@ endif
> >  ifneq ($(CONFIG_ARCH_MSM),)
> >  	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
> >  endif
> > +
> > +ifneq ($(CONFIG_ARCH_MXC),)
> > +	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> > +else
> > +  ifneq ($(CONFIG_ARCH_MXS),)
> > +	  obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> > +  endif
> > +endif
> 
> I think someone commented on these patches (my previous set of patches 
> actually), that this should be fixed in a way that you can actually select which 
> bindings you want to compile in (and not limit it via Makefile like it's now).
Yes. I think you're right.

Alexander,

Do you think it's a good idea to let user select binding driver directly
and the binding driver config depends on chipidea config?
> 
> > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> > b/drivers/usb/chipidea/ci13xxx_imx.c new file mode 100644
> > index 0000000..9f64b3c
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > @@ -0,0 +1,191 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + * Copyright (C) 2012 Marek Vasut <marex@denx.de>
> > + * on behalf of DENX Software Engineering GmbH
> > + *
> > + * 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/of_platform.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/usb/chipidea.h>
> > +#include <linux/usb/ehci_def.h>
> > +#include <linux/usb/gadget.h>
> > +#include <linux/usb/chipidea.h>
> > +#include <linux/usb/otg.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/clk.h>
> > +#include <linux/string.h>
> > +
> > +#include "ci.h"
> > +
> > +#define PORT0_STATUS	0x184
> > +
> > +#define pdev_to_phy(pdev) \
> > +	((struct usb_phy *)platform_get_drvdata(pdev))
> > +
> > +struct ci13xxx_imx_data {
> > +	struct device_node *phy_np;
> > +	struct usb_phy *phy;
> > +	struct platform_device *ci_pdev;
> > +	struct clk *clk;
> > +	int gpio_hub_rst, gpio_vbus_pwr;
> > +};
> > +
> > +static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver __devinitdata  = {
> > +	.name			= "ci13xxx_imx",
> > +	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> > +				  CI13XXX_PULLUP_ON_VBUS |
> > +				  CI13XXX_DISABLE_STREAMING,
> > +	.capoffset		= DEF_CAPOFFSET,
> > +};
> > +
> > +static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
> > +{
> > +	struct ci13xxx_imx_data *data;
> > +	struct platform_device *plat_ci, *phy_pdev;
> > +	struct platform_device_info ci_pdevinfo;
> > +	struct device_node *phy_np;
> > +	struct resource *res;
> > +	int vbus_pwr;
> > +	int ret;
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data) {
> > +		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> > +		return -ENOMEM;
> > +	}
> > +	data->gpio_hub_rst = -1;
ah, I should remove hub reset here.
> > +	data->gpio_vbus_pwr = -1;
> 
> This really freaks me out. Maybe we should have some kind of a regulator that 
> manages the VBUS and let this driver toggle the regulator when fitting? That'd 
> solve the problem to some extent, as other people would be able to supply NOP 
> regulator.
Good point. It looks like we still lack gpio controlled fixed regulator
driver.
> 
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "can't get device resources!\n");
> 
> Can't ... capital "C" ;-)
thanks.
> 
> > +		return -ENOENT;
> > +	}
> > +
> > +	data->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(data->clk)) {
> > +		dev_err(&pdev->dev, "Failed to get clock!\n");
> > +		return PTR_ERR(data->clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(data->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to prepare or enable clock!\n");
> > +		return ret;
> > +	}
> > +
> > +	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> > +	if (phy_np) {
> > +		data->phy_np = phy_np;
> > +		phy_pdev = of_find_device_by_node(phy_np);
> > +		if (phy_pdev) {
> > +			struct usb_phy *phy;
> 
> Do we really want to define new variables in the middle of a code?
It's not a problem in kernel using gcc. You can find the style though
the kernel.
> 
> > +			phy = pdev_to_phy(phy_pdev);
> > +			if (phy &&
> > +			    try_module_get(phy_pdev->dev.driver->owner)) {
> > +				usb_phy_init(phy);
> > +				data->phy = phy;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* we only support host now, so enable vbus here */
> > +	vbus_pwr = of_get_named_gpio(pdev->dev.of_node,
> > +					"fsl,vbus-power-gpios", 0);
> 
> (This one is for my personal enlightment) Why is the property called "gpios" 
> (plural) if it's only one GPIO?
It's just DT naming method. It can follow a gpio array.
> 
> > +	if (gpio_is_valid(vbus_pwr) &&
> > +	    !devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
> > +		gpio_direction_output(vbus_pwr, 1);
> > +		data->gpio_hub_rst = vbus_pwr;
> > +	}
> > +
> > +	ci13xxx_imx_udc_driver.phy = data->phy;
> > +	memset(&ci_pdevinfo, 0, sizeof(ci_pdevinfo));
> > +	ci_pdevinfo.parent = &pdev->dev;
> > +	ci_pdevinfo.name = "ci_hdrc";
> > +	ci_pdevinfo.id = (int)res->start;
> > +	ci_pdevinfo.res = pdev->resource;
> > +	ci_pdevinfo.num_res = pdev->num_resources;
> > +	ci_pdevinfo.data = &ci13xxx_imx_udc_driver;
> > +	ci_pdevinfo.size_data = sizeof(ci13xxx_imx_udc_driver);
> > +	ci_pdevinfo.dma_mask = DMA_BIT_MASK(32);
> > +
> > +	plat_ci = platform_device_register_full(&ci_pdevinfo);
> > +	if (IS_ERR(plat_ci)) {
> > +		dev_err(&pdev->dev, "can't register ci_hdrc platform device\n");
> > +		ret = PTR_ERR(plat_ci);
> > +		goto put_np;
> > +	}
> > +
> > +	data->ci_pdev = plat_ci;
> > +	platform_set_drvdata(pdev, data);
> > +
> > +	pm_runtime_no_callbacks(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	return 0;
> > +
> > +put_np:
> > +	if (phy_np)
> > +		of_node_put(phy_np);
> > +	return ret;
> > +}
> > +
> > +static int __devexit ci13xxx_imx_remove(struct platform_device *pdev)
> > +{
> > +	struct ci13xxx_imx_data *data = platform_get_drvdata(pdev);
> > +
> > +	kfree(data->ci_pdev->dev.dma_mask);
> > +	data->ci_pdev->dev.dma_mask = NULL;
> > +	platform_device_unregister(data->ci_pdev);
> > +
> > +	if (data->gpio_vbus_pwr != -1)
> > +		gpio_direction_output(data->gpio_vbus_pwr, 0);
> > +	if (data->gpio_hub_rst != -1)
> > +		gpio_direction_output(data->gpio_hub_rst, 0);
> > +
> > +	if (data->phy) {
> > +		usb_phy_shutdown(data->phy);
> > +		module_put(data->phy->dev->driver->owner);
> > +	}
> > +
> > +	of_node_put(data->phy_np);
> > +
> > +	clk_disable_unprepare(data->clk);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ci13xxx_imx_dt_ids[] = {
> > +	{ .compatible = "fsl,imx31-usb", },
> 
> Wow, now it's imx31-usb ?
Yes. we discussed that imx31 is the first SoC that use this IP.
> 
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver ci13xxx_imx_driver = {
> > +	.probe = ci13xxx_imx_probe,
> > +	.remove = __devexit_p(ci13xxx_imx_remove),
> > +	.driver = {
> > +		.name = "imx_usb",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = ci13xxx_imx_dt_ids,
> > +	 },
> > +};
> > +
> > +module_platform_driver(ci13xxx_imx_driver);
> > +
> 
> Richard, thanks for your work on these patches so far. Sorry for taking so long 
> with reviewing these.
My pleasure! Thanks for your review too!

Richard
> 

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Alexander Shishkin]

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

> On Tue, May 22, 2012 at 06:30:56AM +0200, Marek Vasut wrote:
>> Dear Richard Zhao,
>> 
>> > Just add host support.
>> 
>> Maybe rephrase it to something like "This patch supports only the host-mode 
>> functionality so far."
> Thanks.
>> 
>> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
>> > Signed-off-by: Marek Vasut <marex@denx.de>
>> > Cc: Peter Chen <peter.chen@freescale.com>
>> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > ---
>> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |   20 ++
>> >  drivers/usb/chipidea/Makefile                      |    8 +
>> >  drivers/usb/chipidea/ci13xxx_imx.c                 |  191
>> > ++++++++++++++++++++ 3 files changed, 219 insertions(+), 0 deletions(-)
>> >  create mode 100644 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> >  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
>> > 
>> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode
>> > 100644
>> > index 0000000..beb75d6
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> > @@ -0,0 +1,20 @@
>> > +* Freescale i.MX ci13xxx usb controllers
>> > +
>> > +Required properties:
>> > +- compatible: Should be "fsl,imx31-usb"
>> > +- reg: Should contain registers location and length
>> > +- interrupts: Should contain controller interrupt
>> > +
>> > +Optional properties:
>> > +- fsl,usbphy: phandler of usb phy that connects to the only one port
>> > +- fsl,hub-reset-gpios: gpio used to reset on-board usb hub
>> > +- fsl,vbus-power-gpios: gpio used to set vbus power of the only one port
>> > +
>> > +Examples:
>> > +usb at 02184000 { /* USB OTG */
>> > +	compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
>> > +	reg = <0x02184000 0x200>;
>> > +	interrupts = <0 43 0x04>;
>> > +	fsl,usbphy = <&usbphy1>;
>> > +	fsl,vbus-power-gpios = <&gpio3 22 0>;
>> > +};
>> > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
>> > index cc34937..ffa2f63 100644
>> > --- a/drivers/usb/chipidea/Makefile
>> > +++ b/drivers/usb/chipidea/Makefile
>> > @@ -12,3 +12,11 @@ endif
>> >  ifneq ($(CONFIG_ARCH_MSM),)
>> >  	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
>> >  endif
>> > +
>> > +ifneq ($(CONFIG_ARCH_MXC),)
>> > +	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
>> > +else
>> > +  ifneq ($(CONFIG_ARCH_MXS),)
>> > +	  obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
>> > +  endif
>> > +endif
>> 
>> I think someone commented on these patches (my previous set of patches 
>> actually), that this should be fixed in a way that you can actually select which 
>> bindings you want to compile in (and not limit it via Makefile like it's now).
> Yes. I think you're right.
>
> Alexander,
>
> Do you think it's a good idea to let user select binding driver directly
> and the binding driver config depends on chipidea config?

I don't have a strong opinion on this, although I prefer it the way it
is now, because, imo:

  * in case of =m (and that's the only sane way of compiling it anyway),
    these all are compiled as modules, which you simply don't install if
    you don't want them;
  * all of them get compile-tested every time you change something in
    the driver, which is a good thing;
  * the fewer kconfig options the better.

Regards,
--
Alex

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Felipe Balbi]

Hi,

On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
> > Do you think it's a good idea to let user select binding driver directly
> > and the binding driver config depends on chipidea config?
> 
> I don't have a strong opinion on this, although I prefer it the way it
> is now, because, imo:
> 
>   * in case of =m (and that's the only sane way of compiling it anyway),
>     these all are compiled as modules, which you simply don't install if
>     you don't want them;
>   * all of them get compile-tested every time you change something in
>     the driver, which is a good thing;

only true for $(ARCH) builds. I would like to see these drivers being
compile tested on linux-next on all arches. Thus the patches I just
sent.

-- 
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/20120522/d1046287/attachment.sig>

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
> > > Do you think it's a good idea to let user select binding driver directly
> > > and the binding driver config depends on chipidea config?
> > 
> > I don't have a strong opinion on this, although I prefer it the way it
> > is now, because, imo:
> > 
> >   * in case of =m (and that's the only sane way of compiling it anyway),
> >     these all are compiled as modules, which you simply don't install if
> >     you don't want them;
> >   * all of them get compile-tested every time you change something in
> >     the driver, which is a good thing;
> 
> only true for $(ARCH) builds. I would like to see these drivers being
> compile tested on linux-next on all arches. Thus the patches I just
> sent.
The idea is great. But
- how can I make sure it pass for all arch? There' 27 folder in arch/.
- it's hard to predict one driver depends on what.
- for embedded kernel, people like built-in drivers, and people will
  have things they don't need at all.

Thanks
Richard
> 
> -- 
> balbi

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Felipe Balbi]

On Tue, May 22, 2012 at 06:31:40PM +0800, Richard Zhao wrote:
> On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
> > > > Do you think it's a good idea to let user select binding driver directly
> > > > and the binding driver config depends on chipidea config?
> > > 
> > > I don't have a strong opinion on this, although I prefer it the way it
> > > is now, because, imo:
> > > 
> > >   * in case of =m (and that's the only sane way of compiling it anyway),
> > >     these all are compiled as modules, which you simply don't install if
> > >     you don't want them;
> > >   * all of them get compile-tested every time you change something in
> > >     the driver, which is a good thing;
> > 
> > only true for $(ARCH) builds. I would like to see these drivers being
> > compile tested on linux-next on all arches. Thus the patches I just
> > sent.
> The idea is great. But
> - how can I make sure it pass for all arch? There' 27 folder in arch/.
> - it's hard to predict one driver depends on what.
> - for embedded kernel, people like built-in drivers, and people will
>   have things they don't need at all.

that's true to some extent, but until we know for sure that all of that
is compiling fine and all dependencies are properly handled, I wouldn't
like to see Kconfig or Makefile being abused. That has happened before
and will happen again if we allow it.

My suggestion to Alex is to remove all dependencies for at least a
couple of merge windows and only add dependencies for stuff which
actually matters; like only building the PCI glue layer when CONFIG_PCI
is defined instead of when ARCH_X86 is defined and so on.

When it gets to a product, that can be easily optimized and when we have
decided what's the best way to place the choices, we will do so. Until
then, we like to use linux-next for compile testing everything.

-- 
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/20120522/433f4274/attachment-0001.sig>

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Alexander Shishkin]

Felipe Balbi <balbi@ti.com> writes:

> On Tue, May 22, 2012 at 06:31:40PM +0800, Richard Zhao wrote:
>> On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
>> > Hi,
>> > 
>> > On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
>> > > > Do you think it's a good idea to let user select binding driver directly
>> > > > and the binding driver config depends on chipidea config?
>> > > 
>> > > I don't have a strong opinion on this, although I prefer it the way it
>> > > is now, because, imo:
>> > > 
>> > >   * in case of =m (and that's the only sane way of compiling it anyway),
>> > >     these all are compiled as modules, which you simply don't install if
>> > >     you don't want them;
>> > >   * all of them get compile-tested every time you change something in
>> > >     the driver, which is a good thing;
>> > 
>> > only true for $(ARCH) builds. I would like to see these drivers being
>> > compile tested on linux-next on all arches. Thus the patches I just
>> > sent.
>> The idea is great. But
>> - how can I make sure it pass for all arch? There' 27 folder in arch/.
>> - it's hard to predict one driver depends on what.
>> - for embedded kernel, people like built-in drivers, and people will
>>   have things they don't need at all.
>
> that's true to some extent, but until we know for sure that all of that
> is compiling fine and all dependencies are properly handled, I wouldn't
> like to see Kconfig or Makefile being abused. That has happened before
> and will happen again if we allow it.
>
> My suggestion to Alex is to remove all dependencies for at least a
> couple of merge windows and only add dependencies for stuff which
> actually matters; like only building the PCI glue layer when CONFIG_PCI
> is defined instead of when ARCH_X86 is defined and so on.

That's what I mean to do as well. I wouldn't dream of making something
like this x86 specific. :)

> When it gets to a product, that can be easily optimized and when we have
> decided what's the best way to place the choices, we will do so. Until
> then, we like to use linux-next for compile testing everything.

Seconded.

Regards,
--
Alex

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 22, 2012 at 01:41:30PM +0300, Alexander Shishkin wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > On Tue, May 22, 2012 at 06:31:40PM +0800, Richard Zhao wrote:
> >> On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
> >> > Hi,
> >> > 
> >> > On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
> >> > > > Do you think it's a good idea to let user select binding driver directly
> >> > > > and the binding driver config depends on chipidea config?
> >> > > 
> >> > > I don't have a strong opinion on this, although I prefer it the way it
> >> > > is now, because, imo:
> >> > > 
> >> > >   * in case of =m (and that's the only sane way of compiling it anyway),
> >> > >     these all are compiled as modules, which you simply don't install if
> >> > >     you don't want them;
> >> > >   * all of them get compile-tested every time you change something in
> >> > >     the driver, which is a good thing;
> >> > 
> >> > only true for $(ARCH) builds. I would like to see these drivers being
> >> > compile tested on linux-next on all arches. Thus the patches I just
> >> > sent.
> >> The idea is great. But
> >> - how can I make sure it pass for all arch? There' 27 folder in arch/.
> >> - it's hard to predict one driver depends on what.
> >> - for embedded kernel, people like built-in drivers, and people will
> >>   have things they don't need at all.
> >
> > that's true to some extent, but until we know for sure that all of that
> > is compiling fine and all dependencies are properly handled, I wouldn't
> > like to see Kconfig or Makefile being abused. That has happened before
> > and will happen again if we allow it.
> >
> > My suggestion to Alex is to remove all dependencies for at least a
> > couple of merge windows and only add dependencies for stuff which
> > actually matters; like only building the PCI glue layer when CONFIG_PCI
> > is defined instead of when ARCH_X86 is defined and so on.
> 
> That's what I mean to do as well. I wouldn't dream of making something
> like this x86 specific. :)
Alex, Have you made the decision that remove all dependencies and leave
only ones that has to be there? If yes, I'll try the way, though I don't
feel good about that.

Thanks
Richard
> 
> > When it gets to a product, that can be easily optimized and when we have
> > decided what's the best way to place the choices, we will do so. Until
> > then, we like to use linux-next for compile testing everything.
> 
> Seconded.
> 
> Regards,
> --
> Alex
> --
> 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 v2 05/12] usb: chipidea: add imx driver binding

[Alexander Shishkin]

Richard Zhao <linuxzsc@gmail.com> writes:

> On Tue, May 22, 2012 at 01:41:30PM +0300, Alexander Shishkin wrote:
>> Felipe Balbi <balbi@ti.com> writes:
>> 
>> > On Tue, May 22, 2012 at 06:31:40PM +0800, Richard Zhao wrote:
>> >> On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
>> >> > Hi,
>> >> > 
>> >> > On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
>> >> > > > Do you think it's a good idea to let user select binding driver directly
>> >> > > > and the binding driver config depends on chipidea config?
>> >> > > 
>> >> > > I don't have a strong opinion on this, although I prefer it the way it
>> >> > > is now, because, imo:
>> >> > > 
>> >> > >   * in case of =m (and that's the only sane way of compiling it anyway),
>> >> > >     these all are compiled as modules, which you simply don't install if
>> >> > >     you don't want them;
>> >> > >   * all of them get compile-tested every time you change something in
>> >> > >     the driver, which is a good thing;
>> >> > 
>> >> > only true for $(ARCH) builds. I would like to see these drivers being
>> >> > compile tested on linux-next on all arches. Thus the patches I just
>> >> > sent.
>> >> The idea is great. But
>> >> - how can I make sure it pass for all arch? There' 27 folder in arch/.
>> >> - it's hard to predict one driver depends on what.
>> >> - for embedded kernel, people like built-in drivers, and people will
>> >>   have things they don't need at all.
>> >
>> > that's true to some extent, but until we know for sure that all of that
>> > is compiling fine and all dependencies are properly handled, I wouldn't
>> > like to see Kconfig or Makefile being abused. That has happened before
>> > and will happen again if we allow it.
>> >
>> > My suggestion to Alex is to remove all dependencies for at least a
>> > couple of merge windows and only add dependencies for stuff which
>> > actually matters; like only building the PCI glue layer when CONFIG_PCI
>> > is defined instead of when ARCH_X86 is defined and so on.
>> 
>> That's what I mean to do as well. I wouldn't dream of making something
>> like this x86 specific. :)
> Alex, Have you made the decision that remove all dependencies and leave
> only ones that has to be there? If yes, I'll try the way, though I don't
> feel good about that.

Yes, I like Felipe's suggestion. Currently (with Felipe's patches from
yesterday), there is only one dependency (PCI for ci13xxx_pci, which is
ok). So let's try to keep it that way.

I'll push the patches to my tree on github [1] soon, which I propose to
use for all chipidea-related patches.

[1] git://github.com/virtuoso/linux-ci.git

Regards,
--
Alex

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Felipe Balbi]

On Wed, May 23, 2012 at 04:02:21PM +0300, Alexander Shishkin wrote:
> Richard Zhao <linuxzsc@gmail.com> writes:
> 
> > On Tue, May 22, 2012 at 01:41:30PM +0300, Alexander Shishkin wrote:
> >> Felipe Balbi <balbi@ti.com> writes:
> >> 
> >> > On Tue, May 22, 2012 at 06:31:40PM +0800, Richard Zhao wrote:
> >> >> On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
> >> >> > Hi,
> >> >> > 
> >> >> > On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
> >> >> > > > Do you think it's a good idea to let user select binding driver directly
> >> >> > > > and the binding driver config depends on chipidea config?
> >> >> > > 
> >> >> > > I don't have a strong opinion on this, although I prefer it the way it
> >> >> > > is now, because, imo:
> >> >> > > 
> >> >> > >   * in case of =m (and that's the only sane way of compiling it anyway),
> >> >> > >     these all are compiled as modules, which you simply don't install if
> >> >> > >     you don't want them;
> >> >> > >   * all of them get compile-tested every time you change something in
> >> >> > >     the driver, which is a good thing;
> >> >> > 
> >> >> > only true for $(ARCH) builds. I would like to see these drivers being
> >> >> > compile tested on linux-next on all arches. Thus the patches I just
> >> >> > sent.
> >> >> The idea is great. But
> >> >> - how can I make sure it pass for all arch? There' 27 folder in arch/.
> >> >> - it's hard to predict one driver depends on what.
> >> >> - for embedded kernel, people like built-in drivers, and people will
> >> >>   have things they don't need at all.
> >> >
> >> > that's true to some extent, but until we know for sure that all of that
> >> > is compiling fine and all dependencies are properly handled, I wouldn't
> >> > like to see Kconfig or Makefile being abused. That has happened before
> >> > and will happen again if we allow it.
> >> >
> >> > My suggestion to Alex is to remove all dependencies for at least a
> >> > couple of merge windows and only add dependencies for stuff which
> >> > actually matters; like only building the PCI glue layer when CONFIG_PCI
> >> > is defined instead of when ARCH_X86 is defined and so on.
> >> 
> >> That's what I mean to do as well. I wouldn't dream of making something
> >> like this x86 specific. :)
> > Alex, Have you made the decision that remove all dependencies and leave
> > only ones that has to be there? If yes, I'll try the way, though I don't
> > feel good about that.
> 
> Yes, I like Felipe's suggestion. Currently (with Felipe's patches from
> yesterday), there is only one dependency (PCI for ci13xxx_pci, which is
> ok). So let's try to keep it that way.
> 
> I'll push the patches to my tree on github [1] soon, which I propose to
> use for all chipidea-related patches.
> 
> [1] git://github.com/virtuoso/linux-ci.git

you should add it to MAINTAINERS file.

T: git git://github.com/virtuoso/linux-ci.git

-- 
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/20120523/12f81d7e/attachment.sig>

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Alexander Shishkin]

Felipe Balbi <balbi@ti.com> writes:

> On Wed, May 23, 2012 at 04:02:21PM +0300, Alexander Shishkin wrote:
>> Richard Zhao <linuxzsc@gmail.com> writes:
>> 
>> > On Tue, May 22, 2012 at 01:41:30PM +0300, Alexander Shishkin wrote:
>> >> Felipe Balbi <balbi@ti.com> writes:
>> >> 
>> >> > On Tue, May 22, 2012 at 06:31:40PM +0800, Richard Zhao wrote:
>> >> >> On Tue, May 22, 2012 at 01:06:26PM +0300, Felipe Balbi wrote:
>> >> >> > Hi,
>> >> >> > 
>> >> >> > On Tue, May 22, 2012 at 12:56:52PM +0300, Alexander Shishkin wrote:
>> >> >> > > > Do you think it's a good idea to let user select binding driver directly
>> >> >> > > > and the binding driver config depends on chipidea config?
>> >> >> > > 
>> >> >> > > I don't have a strong opinion on this, although I prefer it the way it
>> >> >> > > is now, because, imo:
>> >> >> > > 
>> >> >> > >   * in case of =m (and that's the only sane way of compiling it anyway),
>> >> >> > >     these all are compiled as modules, which you simply don't install if
>> >> >> > >     you don't want them;
>> >> >> > >   * all of them get compile-tested every time you change something in
>> >> >> > >     the driver, which is a good thing;
>> >> >> > 
>> >> >> > only true for $(ARCH) builds. I would like to see these drivers being
>> >> >> > compile tested on linux-next on all arches. Thus the patches I just
>> >> >> > sent.
>> >> >> The idea is great. But
>> >> >> - how can I make sure it pass for all arch? There' 27 folder in arch/.
>> >> >> - it's hard to predict one driver depends on what.
>> >> >> - for embedded kernel, people like built-in drivers, and people will
>> >> >>   have things they don't need at all.
>> >> >
>> >> > that's true to some extent, but until we know for sure that all of that
>> >> > is compiling fine and all dependencies are properly handled, I wouldn't
>> >> > like to see Kconfig or Makefile being abused. That has happened before
>> >> > and will happen again if we allow it.
>> >> >
>> >> > My suggestion to Alex is to remove all dependencies for at least a
>> >> > couple of merge windows and only add dependencies for stuff which
>> >> > actually matters; like only building the PCI glue layer when CONFIG_PCI
>> >> > is defined instead of when ARCH_X86 is defined and so on.
>> >> 
>> >> That's what I mean to do as well. I wouldn't dream of making something
>> >> like this x86 specific. :)
>> > Alex, Have you made the decision that remove all dependencies and leave
>> > only ones that has to be there? If yes, I'll try the way, though I don't
>> > feel good about that.
>> 
>> Yes, I like Felipe's suggestion. Currently (with Felipe's patches from
>> yesterday), there is only one dependency (PCI for ci13xxx_pci, which is
>> ok). So let's try to keep it that way.
>> 
>> I'll push the patches to my tree on github [1] soon, which I propose to
>> use for all chipidea-related patches.
>> 
>> [1] git://github.com/virtuoso/linux-ci.git
>
> you should add it to MAINTAINERS file.
>
> T: git git://github.com/virtuoso/linux-ci.git

Sure.

Regrards,
--
Alex

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Shawn Guo]

On Tue, May 22, 2012 at 01:35:17PM +0300, Felipe Balbi wrote:
> My suggestion to Alex is to remove all dependencies for at least a
> couple of merge windows and only add dependencies for stuff which
> actually matters; like only building the PCI glue layer when CONFIG_PCI
> is defined instead of when ARCH_X86 is defined and so on.
> 
As ci13xxx_imx is a DT only driver, it needs to depend on CONFIG_OF.

-- 
Regards,
Shawn

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Felipe Balbi]

On Tue, May 22, 2012 at 09:52:59PM +0800, Shawn Guo wrote:
> On Tue, May 22, 2012 at 01:35:17PM +0300, Felipe Balbi wrote:
> > My suggestion to Alex is to remove all dependencies for at least a
> > couple of merge windows and only add dependencies for stuff which
> > actually matters; like only building the PCI glue layer when CONFIG_PCI
> > is defined instead of when ARCH_X86 is defined and so on.
> > 
> As ci13xxx_imx is a DT only driver, it needs to depend on CONFIG_OF.

yes, and I have said that it should depend on CONFIG_OF, rather then
ARCH_IMX... that's fine.

-- 
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/20120522/9be189fa/attachment-0001.sig>

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Alexander Shishkin]

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

> +#include "ci.h"
> +
> +#define PORT0_STATUS	0x184

This doesn't seem to be used anywhere.

[snip]
> +	ci13xxx_imx_udc_driver.phy = data->phy;

See my comment to the other patch about phy pointer being a per-device
thing rather then per-platform driver. Now, (at the risk of being
flamed) I think it could work if you allocated the
ci13xxx_imx_udc_driver dynamically in every probe, but that's not what
you want to do for the actual platform data. So, I suggest we come up
with a better way of passing phy to the driver. I also think that it's
really about time we renamed "struct ci13xxx_udc_driver" to something
that emphasizes the "platform" in it. What do you think?

Regards,
--
Alex

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Alexander Shishkin]

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> Richard Zhao <richard.zhao@freescale.com> writes:
>
>> +#include "ci.h"
>> +
>> +#define PORT0_STATUS	0x184
>
> This doesn't seem to be used anywhere.

Please disregard this one.

Regards,
--
Alex

[PATCH v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 22, 2012 at 01:24:57PM +0300, Alexander Shishkin wrote:
> Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
> 
> > Richard Zhao <richard.zhao@freescale.com> writes:
> >
> >> +#include "ci.h"
> >> +
> >> +#define PORT0_STATUS	0x184
> >
> > This doesn't seem to be used anywhere.
> 
> Please disregard this one.
Thanks.

Richard
> 
> Regards,
> --
> Alex
> --
> 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 v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao]

On Tue, May 22, 2012 at 01:21:44PM +0300, Alexander Shishkin wrote:
> Richard Zhao <richard.zhao@freescale.com> writes:
> 
> > +#include "ci.h"
> > +
> > +#define PORT0_STATUS	0x184
> 
> This doesn't seem to be used anywhere.
> 
> [snip]
> > +	ci13xxx_imx_udc_driver.phy = data->phy;
> 
> See my comment to the other patch about phy pointer being a per-device
> thing rather then per-platform driver. Now, (at the risk of being
> flamed) I think it could work if you allocated the
> ci13xxx_imx_udc_driver dynamically in every probe, but that's not what
> you want to do for the actual platform data. 
At least for imx, most platfrom data memory is duplicated when add
device. arch/arm/plat-mxc/devices/
> So, I suggest we come up
> with a better way of passing phy to the driver. I also think that it's
> really about time we renamed "struct ci13xxx_udc_driver" to something
> that emphasizes the "platform" in it. What do you think?
struct ci13xxx_plat_data ?

Thanks
Richard
> 
> Regards,
> --
> Alex
> --
> 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 v2 06/12] ARM: imx6q: correct device name of usbphy and usb controller clock export

[Richard Zhao]

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

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 96e9edd..f99509a 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -391,7 +391,10 @@ 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.usb");
+	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
+	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
+	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
 	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 v2 06/12] ARM: imx6q: correct device name of usbphy and usb controller clock export

[Marek Vasut]

Dear Richard Zhao,

We will need similar patch for mx28 I think?

Otherwise
Reviewed-by: Marek Vasut <marex@denx.de>

> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx6q.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 96e9edd..f99509a 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -391,7 +391,10 @@ 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.usb");
> +	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
> +	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
> +	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
>  	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");

Best regards,
Marek Vasut

[PATCH v2 06/12] ARM: imx6q: correct device name of usbphy and usb controller clock export

[Richard Zhao]

On Tue, May 22, 2012 at 06:31:28AM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> We will need similar patch for mx28 I think?
It has mx28 clk patch. hmm..., something is wrong. I'll fix the name.

Thanks
Richard
> 
> Otherwise
> Reviewed-by: Marek Vasut <marex@denx.de>
> 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  arch/arm/mach-imx/clk-imx6q.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> > index 96e9edd..f99509a 100644
> > --- a/arch/arm/mach-imx/clk-imx6q.c
> > +++ b/arch/arm/mach-imx/clk-imx6q.c
> > @@ -391,7 +391,10 @@ 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.usb");
> > +	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
> > +	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
> > +	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
> >  	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");
> 
> Best regards,
> Marek Vasut
> 

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Richard Zhao]

Sometimes, boards have gpios that don't own by any driver or owner
by a generic driver that don't like hacks. Such gpios is normally
output and need setup once on boot. So I introduce the config-on-boot
gpios.

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
 arch/arm/mach-imx/mach-imx6q.c        |   35 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index e0ec929..1dd2261 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -17,6 +17,13 @@
 	model = "Freescale i.MX6 Quad SABRE Lite Board";
 	compatible = "fsl,imx6q-sabrelite", "fsl,imx6q";
 
+	config-on-boot {
+		output-gpios = <
+				&gpio3 22 0>;	/* vbus reset */
+		output-gpio-values = <
+				1>;		/* vbus reset */
+	};
+
 	memory {
 		reg = <0x10000000 0x40000000>;
 	};
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index b47e98b..577cf19 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -19,6 +19,7 @@
 #include <linux/irqdomain.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/pinctrl/machine.h>
@@ -113,6 +114,38 @@ static void __init imx6q_sabrelite_init(void)
 	imx6q_sabrelite_cko1_setup();
 }
 
+static void __init imx6q_config_on_boot(void)
+{
+	struct device_node *np;
+	struct property *pp;
+	int cnt, len, i;
+	int gpio;
+
+	np = of_find_node_by_path("/config-on-boot");
+	if (!np)
+		return;
+	cnt = of_gpio_named_count(np, "output-gpios");
+	pp = of_find_property(np, "output-gpio-values", &len);
+	if (!pp || cnt != len / sizeof(u32)) {
+		pr_err("Invalid config-on-boot gpios!\n");
+		of_node_put(np);
+		return;
+	}
+	for (i = 0; i < cnt; i++) {
+		gpio = of_get_named_gpio(np, "output-gpios", i);
+		if (gpio_is_valid(gpio))
+			gpio_request_one(gpio, GPIOF_OUT_INIT_HIGH,
+					"config-on-boot");
+	}
+
+	of_node_put(np);
+}
+
+static void __init imx6q_post_populate(void)
+{
+	imx6q_config_on_boot();
+}
+
 static void __init imx6q_init_machine(void)
 {
 	/*
@@ -126,6 +159,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 v2 07/12] ARM: imx6q: add config-on-boot gpios

[Shawn Guo]

Apart from my comments below, we really need to know Rob and Grant's
opinion on this.

On Mon, May 21, 2012 at 05:23:52PM +0800, Richard Zhao wrote:
> Sometimes, boards have gpios that don't own by any driver or owner
> by a generic driver that don't like hacks. Such gpios is normally
> output and need setup once on boot. So I introduce the config-on-boot
> gpios.
> 
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
>  arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
>  arch/arm/mach-imx/mach-imx6q.c        |   35 +++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index e0ec929..1dd2261 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -17,6 +17,13 @@
>  	model = "Freescale i.MX6 Quad SABRE Lite Board";
>  	compatible = "fsl,imx6q-sabrelite", "fsl,imx6q";
>  
> +	config-on-boot {
> +		output-gpios = <
> +				&gpio3 22 0>;	/* vbus reset */
> +		output-gpio-values = <
> +				1>;		/* vbus reset */
> +	};
> +

So it looks like something not specific to imx board but generic to
other boards.  If so, we may need to come up with a generic binding
document for this.

Is this only used to configure output pin?  Will there be any pin that
needs to be configured as input but not owned by any driver?  Hopefully
not.

>  	memory {
>  		reg = <0x10000000 0x40000000>;
>  	};
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index b47e98b..577cf19 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -19,6 +19,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/pinctrl/machine.h>
> @@ -113,6 +114,38 @@ static void __init imx6q_sabrelite_init(void)
>  	imx6q_sabrelite_cko1_setup();
>  }
>  
> +static void __init imx6q_config_on_boot(void)

Do you intend to make this function non-gpio-config only.  Otherwise,
we may need to have "gpio" in the function name, as well as the
"config-on-boot" node name.

Also, if we define the binding as generic one, we may need to find a
common place to implement the function.

> +{
> +	struct device_node *np;
> +	struct property *pp;
> +	int cnt, len, i;
> +	int gpio;
> +
> +	np = of_find_node_by_path("/config-on-boot");
> +	if (!np)
> +		return;
> +	cnt = of_gpio_named_count(np, "output-gpios");
> +	pp = of_find_property(np, "output-gpio-values", &len);
> +	if (!pp || cnt != len / sizeof(u32)) {
> +		pr_err("Invalid config-on-boot gpios!\n");
> +		of_node_put(np);
> +		return;
> +	}
> +	for (i = 0; i < cnt; i++) {
> +		gpio = of_get_named_gpio(np, "output-gpios", i);
> +		if (gpio_is_valid(gpio))
> +			gpio_request_one(gpio, GPIOF_OUT_INIT_HIGH,
> +					"config-on-boot");

So all the gpios will use the same name.

And doesn't the reset generally need a pulse or edge signal, e.g. pull
down for a msec and pull up?

Regards,
Shawn

> +	}
> +
> +	of_node_put(np);
> +}
> +
> +static void __init imx6q_post_populate(void)
> +{
> +	imx6q_config_on_boot();
> +}
> +
>  static void __init imx6q_init_machine(void)
>  {
>  	/*
> @@ -126,6 +159,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 v2 07/12] ARM: imx6q: add config-on-boot gpios

[Marek Vasut]

Dear Shawn Guo,

> Apart from my comments below, we really need to know Rob and Grant's
> opinion on this.

Hm ... so why don't you actually write per-board driver ? :-) That'd handle 
every single board very well, you'd configure the GPIOs there, every single 
additional necessary thing that has to be configured would sink there too etc.

> On Mon, May 21, 2012 at 05:23:52PM +0800, Richard Zhao wrote:
> > Sometimes, boards have gpios that don't own by any driver or owner
> > by a generic driver that don't like hacks. Such gpios is normally
> > output and need setup once on boot. So I introduce the config-on-boot
> > gpios.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > ---
> > 
> >  arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
> >  arch/arm/mach-imx/mach-imx6q.c        |   35
> >  +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0
> >  deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > b/arch/arm/boot/dts/imx6q-sabrelite.dts index e0ec929..1dd2261 100644
> > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > @@ -17,6 +17,13 @@
> > 
> >  	model = "Freescale i.MX6 Quad SABRE Lite Board";
> >  	compatible = "fsl,imx6q-sabrelite", "fsl,imx6q";
> > 
> > +	config-on-boot {
> > +		output-gpios = <
> > +				&gpio3 22 0>;	/* vbus reset */
> > +		output-gpio-values = <
> > +				1>;		/* vbus reset */
> > +	};
> > +
> 
> So it looks like something not specific to imx board but generic to
> other boards.  If so, we may need to come up with a generic binding
> document for this.
> 
> Is this only used to configure output pin?  Will there be any pin that
> needs to be configured as input but not owned by any driver?  Hopefully
> not.
> 
> >  	memory {
> >  	
> >  		reg = <0x10000000 0x40000000>;
> >  	
> >  	};
> > 
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c
> > b/arch/arm/mach-imx/mach-imx6q.c index b47e98b..577cf19 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -19,6 +19,7 @@
> > 
> >  #include <linux/irqdomain.h>
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> > 
> > +#include <linux/of_gpio.h>
> > 
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/pinctrl/machine.h>
> > 
> > @@ -113,6 +114,38 @@ static void __init imx6q_sabrelite_init(void)
> > 
> >  	imx6q_sabrelite_cko1_setup();
> >  
> >  }
> > 
> > +static void __init imx6q_config_on_boot(void)
> 
> Do you intend to make this function non-gpio-config only.  Otherwise,
> we may need to have "gpio" in the function name, as well as the
> "config-on-boot" node name.
> 
> Also, if we define the binding as generic one, we may need to find a
> common place to implement the function.
> 
> > +{
> > +	struct device_node *np;
> > +	struct property *pp;
> > +	int cnt, len, i;
> > +	int gpio;
> > +
> > +	np = of_find_node_by_path("/config-on-boot");
> > +	if (!np)
> > +		return;
> > +	cnt = of_gpio_named_count(np, "output-gpios");
> > +	pp = of_find_property(np, "output-gpio-values", &len);
> > +	if (!pp || cnt != len / sizeof(u32)) {
> > +		pr_err("Invalid config-on-boot gpios!\n");
> > +		of_node_put(np);
> > +		return;
> > +	}
> > +	for (i = 0; i < cnt; i++) {
> > +		gpio = of_get_named_gpio(np, "output-gpios", i);
> > +		if (gpio_is_valid(gpio))
> > +			gpio_request_one(gpio, GPIOF_OUT_INIT_HIGH,
> > +					"config-on-boot");
> 
> So all the gpios will use the same name.
> 
> And doesn't the reset generally need a pulse or edge signal, e.g. pull
> down for a msec and pull up?
> 
> Regards,
> Shawn
> 
> > +	}
> > +
> > +	of_node_put(np);
> > +}
> > +
> > +static void __init imx6q_post_populate(void)
> > +{
> > +	imx6q_config_on_boot();
> > +}
> > +
> > 
> >  static void __init imx6q_init_machine(void)
> >  {
> >  
> >  	/*
> > 
> > @@ -126,6 +159,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();
> >  
> >  }

Best regards,
Marek Vasut

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Shawn Guo]

On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote:
> Dear Shawn Guo,
>
>> Apart from my comments below, we really need to know Rob and Grant's
>> opinion on this.
>
> Hm ... so why don't you actually write per-board driver ? :-) That'd handle
> every single board very well, you'd configure the GPIOs there, every single
> additional necessary thing that has to be configured would sink there too etc.
>
I'm not completely sure about how the per-board driver looks like
exactly.  Can you have some code demonstrating it?

Regards,
Shawn

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Marek Vasut]

Dear Shawn Guo,

> On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote:
> > Dear Shawn Guo,
> > 
> >> Apart from my comments below, we really need to know Rob and Grant's
> >> opinion on this.
> > 
> > Hm ... so why don't you actually write per-board driver ? :-) That'd
> > handle every single board very well, you'd configure the GPIOs there,
> > every single additional necessary thing that has to be configured would
> > sink there too etc.
> 
> I'm not completely sure about how the per-board driver looks like
> exactly.  Can you have some code demonstrating it?

Well such per-board driver would handle the board-specific init, like 
configuring the hub GPIO (as Richard has to do so on mx6q) or flick some other 
GPIOs, configure registers etc.

You can not depend on a bootloader to do such init for you, even though the 
bootloader should do it and should do it right.

> 
> Regards,
> Shawn

Best regards,
Marek Vasut

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Shawn Guo]

On Tue, May 22, 2012 at 06:20:16AM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote:
> > > Dear Shawn Guo,
> > > 
> > >> Apart from my comments below, we really need to know Rob and Grant's
> > >> opinion on this.
> > > 
> > > Hm ... so why don't you actually write per-board driver ? :-) That'd
> > > handle every single board very well, you'd configure the GPIOs there,
> > > every single additional necessary thing that has to be configured would
> > > sink there too etc.
> > 
> > I'm not completely sure about how the per-board driver looks like
> > exactly.  Can you have some code demonstrating it?
> 
> Well such per-board driver would handle the board-specific init, like 
> configuring the hub GPIO (as Richard has to do so on mx6q) or flick some other 
> GPIOs, configure registers etc.
> 
So when you say driver, you just meant a C file like imx6q-sabrelite.c
containing a number of init functions to be called by mach-imx6q.c?

-- 
Regards,
Shawn

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Marek Vasut]

Dear Shawn Guo,

> On Tue, May 22, 2012 at 06:20:16AM +0200, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote:
> > > > Dear Shawn Guo,
> > > > 
> > > >> Apart from my comments below, we really need to know Rob and Grant's
> > > >> opinion on this.
> > > > 
> > > > Hm ... so why don't you actually write per-board driver ? :-) That'd
> > > > handle every single board very well, you'd configure the GPIOs there,
> > > > every single additional necessary thing that has to be configured
> > > > would sink there too etc.
> > > 
> > > I'm not completely sure about how the per-board driver looks like
> > > exactly.  Can you have some code demonstrating it?
> > 
> > Well such per-board driver would handle the board-specific init, like
> > configuring the hub GPIO (as Richard has to do so on mx6q) or flick some
> > other GPIOs, configure registers etc.
> 
> So when you say driver, you just meant a C file like imx6q-sabrelite.c
> containing a number of init functions to be called by mach-imx6q.c?

Something like that ... but didn't Linus complain that we have too many files in 
arch/arm/ ?

Best regards,
Marek Vasut

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Shawn Guo]

On 22 May 2012 13:22, Marek Vasut <marex@denx.de> wrote:
> Something like that ... but didn't Linus complain that we have too many files in
> arch/arm/ ?

So what do you exactly mean by per-board driver?

Regards,
Shawn

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Marek Vasut]

Dear Shawn Guo,

> On 22 May 2012 13:22, Marek Vasut <marex@denx.de> wrote:
> > Something like that ... but didn't Linus complain that we have too many
> > files in arch/arm/ ?
> 
> So what do you exactly mean by per-board driver?

Exactly such fixup as there's for mx6q-sabrelite. But then, if you have too 
broken bootloader, you'll need such a fixup for every such board. Or do you want 
to let mach-imx6q.c grow in size with each broken board?

> Regards,
> Shawn

Best regards,
Marek Vasut

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Richard Zhao]

On Tue, May 22, 2012 at 07:27:30AM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On 22 May 2012 13:22, Marek Vasut <marex@denx.de> wrote:
> > > Something like that ... but didn't Linus complain that we have too many
> > > files in arch/arm/ ?
> > 
> > So what do you exactly mean by per-board driver?
> 
> Exactly such fixup as there's for mx6q-sabrelite. But then, if you have too 
> broken bootloader, you'll need such a fixup for every such board. Or do you want 
> to let mach-imx6q.c grow in size with each broken board?
As Shawn said, this patch is trying to abstract common things.
We don't need to repeat the code that interpret output-gpios.

Generally discuss often draw no conclusion. So let's talk about
code.

Thanks
Richard
> 
> > Regards,
> > Shawn
> 
> Best regards,
> Marek Vasut
> 

[PATCH v2 07/12] ARM: imx6q: add config-on-boot gpios

[Richard Zhao]

On Tue, May 22, 2012 at 11:17:12AM +0800, Shawn Guo wrote:
> Apart from my comments below, we really need to know Rob and Grant's
> opinion on this.
Exactly. I cced them to draw their attention.
> 
> On Mon, May 21, 2012 at 05:23:52PM +0800, Richard Zhao wrote:
> > Sometimes, boards have gpios that don't own by any driver or owner
> > by a generic driver that don't like hacks. Such gpios is normally
> > output and need setup once on boot. So I introduce the config-on-boot
> > gpios.
> > 
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >  arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
> >  arch/arm/mach-imx/mach-imx6q.c        |   35 +++++++++++++++++++++++++++++++++
> >  2 files changed, 42 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > index e0ec929..1dd2261 100644
> > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > @@ -17,6 +17,13 @@
> >  	model = "Freescale i.MX6 Quad SABRE Lite Board";
> >  	compatible = "fsl,imx6q-sabrelite", "fsl,imx6q";
> >  
> > +	config-on-boot {
> > +		output-gpios = <
> > +				&gpio3 22 0>;	/* vbus reset */
> > +		output-gpio-values = <
> > +				1>;		/* vbus reset */
> > +	};
> > +
> 
> So it looks like something not specific to imx board but generic to
> other boards.  If so, we may need to come up with a generic binding
> document for this.
Yes.
> 
> Is this only used to configure output pin?  Will there be any pin that
> needs to be configured as input but not owned by any driver?  Hopefully
> not.
I didn't see the case yet. Input pin normally has a corresponding driver. 
> 
> >  	memory {
> >  		reg = <0x10000000 0x40000000>;
> >  	};
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> > index b47e98b..577cf19 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/irqdomain.h>
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/pinctrl/machine.h>
> > @@ -113,6 +114,38 @@ static void __init imx6q_sabrelite_init(void)
> >  	imx6q_sabrelite_cko1_setup();
> >  }
> >  
> > +static void __init imx6q_config_on_boot(void)
> 
> Do you intend to make this function non-gpio-config only.  Otherwise,
> we may need to have "gpio" in the function name, as well as the
> "config-on-boot" node name.
config-on-boot is not specific for gpio, but may be for others. For
example, regulators. For now, it only has output gpios.
> 
> Also, if we define the binding as generic one, we may need to find a
> common place to implement the function.
Yes. But it's better be called at machine init after populate devices.
> 
> > +{
> > +	struct device_node *np;
> > +	struct property *pp;
> > +	int cnt, len, i;
> > +	int gpio;
> > +
> > +	np = of_find_node_by_path("/config-on-boot");
> > +	if (!np)
> > +		return;
> > +	cnt = of_gpio_named_count(np, "output-gpios");
> > +	pp = of_find_property(np, "output-gpio-values", &len);
> > +	if (!pp || cnt != len / sizeof(u32)) {
> > +		pr_err("Invalid config-on-boot gpios!\n");
> > +		of_node_put(np);
> > +		return;
> > +	}
> > +	for (i = 0; i < cnt; i++) {
> > +		gpio = of_get_named_gpio(np, "output-gpios", i);
> > +		if (gpio_is_valid(gpio))
> > +			gpio_request_one(gpio, GPIOF_OUT_INIT_HIGH,
> > +					"config-on-boot");
> 
> So all the gpios will use the same name.
Yes.
> 
> And doesn't the reset generally need a pulse or edge signal, e.g. pull
> down for a msec and pull up?
strict reset is too complicated for config-on-boot. It may need delay
some time case by case. I don't have the case yet. But someone may
add it if it really needed, probably with addon properties.

Thanks
Richard
> 
> Regards,
> Shawn
> 
> > +	}
> > +
> > +	of_node_put(np);
> > +}
> > +
> > +static void __init imx6q_post_populate(void)
> > +{
> > +	imx6q_config_on_boot();
> > +}
> > +
> >  static void __init imx6q_init_machine(void)
> >  {
> >  	/*
> > @@ -126,6 +159,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 v2 08/12] ARM: imx6q: add usbphy clocks

[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 f99509a..0d003aa 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -152,7 +152,7 @@ enum mx6q_clks {
 	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
 	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
 	pll4_audio, pll5_video, pll6_mlb, pll7_usb_host, pll8_enet, ssi1_ipg,
-	ssi2_ipg, ssi3_ipg, clk_max
+	ssi2_ipg, ssi3_ipg, usbphy1_gate, usbphy2_gate, clk_max
 };
 
 static struct clk *clk[clk_max];
@@ -197,6 +197,9 @@ int __init mx6q_clocks_init(void)
 	clk[pll7_usb_host] = imx_clk_pllv3(IMX_PLLV3_USB,	"pll7_usb_host","osc", base + 0x20, 0x2000,   0x3);
 	clk[pll8_enet]     = imx_clk_pllv3(IMX_PLLV3_ENET,	"pll8_enet",	"osc", base + 0xe0, 0x182000, 0x3);
 
+	clk[usbphy1_gate] = imx_clk_gate("usbphy1_gate", "pll3_usb_otg", base + 0x10, 6);
+	clk[usbphy2_gate] = imx_clk_gate("usbphy2_gate", "pll7_usb_host", base + 0x20, 6);
+
 	/*                                name              parent_name        reg       idx */
 	clk[pll2_pfd0_352m] = imx_clk_pfd("pll2_pfd0_352m", "pll2_bus",     base + 0x100, 0);
 	clk[pll2_pfd1_594m] = imx_clk_pfd("pll2_pfd1_594m", "pll2_bus",     base + 0x100, 1);
@@ -395,6 +398,8 @@ int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
 	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
 	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
+	clk_register_clkdev(clk[usbphy1_gate], NULL, "20c9000.usbphy");
+	clk_register_clkdev(clk[usbphy2_gate], 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 v2 08/12] ARM: imx6q: add usbphy clocks

[Marek Vasut]

Dear Richard Zhao,

Maybe reorder this so it's in before the PHY driver?

Reviewed-by: Marek Vasut <marex@denx.de>

> 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 f99509a..0d003aa 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -152,7 +152,7 @@ enum mx6q_clks {
>  	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
>  	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
>  	pll4_audio, pll5_video, pll6_mlb, pll7_usb_host, pll8_enet, ssi1_ipg,
> -	ssi2_ipg, ssi3_ipg, clk_max
> +	ssi2_ipg, ssi3_ipg, usbphy1_gate, usbphy2_gate, clk_max
>  };
> 
>  static struct clk *clk[clk_max];
> @@ -197,6 +197,9 @@ int __init mx6q_clocks_init(void)
>  	clk[pll7_usb_host] = imx_clk_pllv3(IMX_PLLV3_USB,	
"pll7_usb_host","osc",
> base + 0x20, 0x2000,   0x3); clk[pll8_enet]     =
> imx_clk_pllv3(IMX_PLLV3_ENET,	"pll8_enet",	"osc", base + 0xe0, 0x182000,
> 0x3);
> 
> +	clk[usbphy1_gate] = imx_clk_gate("usbphy1_gate", "pll3_usb_otg", base +
> 0x10, 6); +	clk[usbphy2_gate] = imx_clk_gate("usbphy2_gate",
> "pll7_usb_host", base + 0x20, 6); +
>  	/*                                name              parent_name       
> reg       idx */ clk[pll2_pfd0_352m] = imx_clk_pfd("pll2_pfd0_352m",
> "pll2_bus",     base + 0x100, 0); clk[pll2_pfd1_594m] =
> imx_clk_pfd("pll2_pfd1_594m", "pll2_bus",     base + 0x100, 1); @@ -395,6
> +398,8 @@ int __init mx6q_clocks_init(void)
>  	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
>  	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
>  	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
> +	clk_register_clkdev(clk[usbphy1_gate], NULL, "20c9000.usbphy");
> +	clk_register_clkdev(clk[usbphy2_gate], 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");

Best regards,
Marek Vasut

[PATCH v2 08/12] ARM: imx6q: add usbphy clocks

[Shawn Guo]

On Tue, May 22, 2012 at 06:32:13AM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> Maybe reorder this so it's in before the PHY driver?
> 
It should not matter, as I suppose all those "usb: ..." patches will
go via usb tree, while all those "ARM: ..." will go via arm-soc tree.

-- 
Regards,
Shawn

[PATCH v2 08/12] ARM: imx6q: add usbphy clocks

[Marek Vasut]

Dear Shawn Guo,

> On Tue, May 22, 2012 at 06:32:13AM +0200, Marek Vasut wrote:
> > Dear Richard Zhao,
> > 
> > Maybe reorder this so it's in before the PHY driver?
> 
> It should not matter, as I suppose all those "usb: ..." patches will
> go via usb tree, while all those "ARM: ..." will go via arm-soc tree.

It's easier to bisect, but ok.

Best regards,
Marek Vasut

[PATCH v2 08/12] ARM: imx6q: add usbphy clocks

[Richard Zhao]

On Tue, May 22, 2012 at 06:32:13AM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> Maybe reorder this so it's in before the PHY driver?
Patch series always driver come first.
> 
> Reviewed-by: Marek Vasut <marex@denx.de>
> 
> > 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 f99509a..0d003aa 100644
> > --- a/arch/arm/mach-imx/clk-imx6q.c
> > +++ b/arch/arm/mach-imx/clk-imx6q.c
> > @@ -152,7 +152,7 @@ enum mx6q_clks {
> >  	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
> >  	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
> >  	pll4_audio, pll5_video, pll6_mlb, pll7_usb_host, pll8_enet, ssi1_ipg,
> > -	ssi2_ipg, ssi3_ipg, clk_max
> > +	ssi2_ipg, ssi3_ipg, usbphy1_gate, usbphy2_gate, clk_max
> >  };
> > 
> >  static struct clk *clk[clk_max];
> > @@ -197,6 +197,9 @@ int __init mx6q_clocks_init(void)
> >  	clk[pll7_usb_host] = imx_clk_pllv3(IMX_PLLV3_USB,	
> "pll7_usb_host","osc",
could you disable auto-wrap?

Thanks
Richard
> > base + 0x20, 0x2000,   0x3); clk[pll8_enet]     =
> > imx_clk_pllv3(IMX_PLLV3_ENET,	"pll8_enet",	"osc", base + 0xe0, 0x182000,
> > 0x3);
> > 
> > +	clk[usbphy1_gate] = imx_clk_gate("usbphy1_gate", "pll3_usb_otg", base +
> > 0x10, 6); +	clk[usbphy2_gate] = imx_clk_gate("usbphy2_gate",
> > "pll7_usb_host", base + 0x20, 6); +
> >  	/*                                name              parent_name       
> > reg       idx */ clk[pll2_pfd0_352m] = imx_clk_pfd("pll2_pfd0_352m",
> > "pll2_bus",     base + 0x100, 0); clk[pll2_pfd1_594m] =
> > imx_clk_pfd("pll2_pfd1_594m", "pll2_bus",     base + 0x100, 1); @@ -395,6
> > +398,8 @@ int __init mx6q_clocks_init(void)
> >  	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
> >  	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
> >  	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
> > +	clk_register_clkdev(clk[usbphy1_gate], NULL, "20c9000.usbphy");
> > +	clk_register_clkdev(clk[usbphy2_gate], 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");
> 
> Best regards,
> Marek Vasut
> 

[PATCH v2 08/12] ARM: imx6q: add usbphy clocks

[Shawn Guo]

On Mon, May 21, 2012 at 05:23:53PM +0800, Richard Zhao wrote:
> 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 f99509a..0d003aa 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -152,7 +152,7 @@ enum mx6q_clks {
>  	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
>  	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
>  	pll4_audio, pll5_video, pll6_mlb, pll7_usb_host, pll8_enet, ssi1_ipg,
> -	ssi2_ipg, ssi3_ipg, clk_max
> +	ssi2_ipg, ssi3_ipg, usbphy1_gate, usbphy2_gate, clk_max

Can you drop the "_gate" suffix?  We do not have such suffix for leaf
clock in imx6q clock file.

Regards,
Shawn

>  };
>  
>  static struct clk *clk[clk_max];
> @@ -197,6 +197,9 @@ int __init mx6q_clocks_init(void)
>  	clk[pll7_usb_host] = imx_clk_pllv3(IMX_PLLV3_USB,	"pll7_usb_host","osc", base + 0x20, 0x2000,   0x3);
>  	clk[pll8_enet]     = imx_clk_pllv3(IMX_PLLV3_ENET,	"pll8_enet",	"osc", base + 0xe0, 0x182000, 0x3);
>  
> +	clk[usbphy1_gate] = imx_clk_gate("usbphy1_gate", "pll3_usb_otg", base + 0x10, 6);
> +	clk[usbphy2_gate] = imx_clk_gate("usbphy2_gate", "pll7_usb_host", base + 0x20, 6);
> +
>  	/*                                name              parent_name        reg       idx */
>  	clk[pll2_pfd0_352m] = imx_clk_pfd("pll2_pfd0_352m", "pll2_bus",     base + 0x100, 0);
>  	clk[pll2_pfd1_594m] = imx_clk_pfd("pll2_pfd1_594m", "pll2_bus",     base + 0x100, 1);
> @@ -395,6 +398,8 @@ int __init mx6q_clocks_init(void)
>  	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
>  	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
>  	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
> +	clk_register_clkdev(clk[usbphy1_gate], NULL, "20c9000.usbphy");
> +	clk_register_clkdev(clk[usbphy2_gate], 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 v2 08/12] ARM: imx6q: add usbphy clocks

[Richard Zhao]

On Tue, May 22, 2012 at 01:37:08PM +0800, Shawn Guo wrote:
> On Mon, May 21, 2012 at 05:23:53PM +0800, Richard Zhao wrote:
> > 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 f99509a..0d003aa 100644
> > --- a/arch/arm/mach-imx/clk-imx6q.c
> > +++ b/arch/arm/mach-imx/clk-imx6q.c
> > @@ -152,7 +152,7 @@ enum mx6q_clks {
> >  	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
> >  	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
> >  	pll4_audio, pll5_video, pll6_mlb, pll7_usb_host, pll8_enet, ssi1_ipg,
> > -	ssi2_ipg, ssi3_ipg, clk_max
> > +	ssi2_ipg, ssi3_ipg, usbphy1_gate, usbphy2_gate, clk_max
> 
> Can you drop the "_gate" suffix?  We do not have such suffix for leaf
> clock in imx6q clock file.
ok

Thanks
Richard
 

[PATCH v2 09/12] ARM: imx6q: disable usb charger detector

[Richard Zhao]

The external charger detector needs to be disabled,
or the signal at DP will be poor

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

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 577cf19..0bfd8c7 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -25,6 +25,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>
@@ -141,9 +142,33 @@ static void __init imx6q_config_on_boot(void)
 	of_node_put(np);
 }
 
+static void __init imx6q_usb_init(void)
+{
+#define HW_ANADIG_USB1_CHRG_DETECT		0x000001b0
+#define HW_ANADIG_USB2_CHRG_DETECT		0x00000210
+
+#define BM_ANADIG_USB_CHRG_DETECT_EN_B		0x00100000
+#define BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B	0x00080000
+
+	/*
+	 * The external charger detector needs to be disabled,
+	 * or the signal at DP will be poor
+	 */
+
+	anatop_write_reg(NULL, HW_ANADIG_USB1_CHRG_DETECT,
+			BM_ANADIG_USB_CHRG_DETECT_EN_B
+			| BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B,
+			~0);
+	anatop_write_reg(NULL, HW_ANADIG_USB2_CHRG_DETECT,
+			BM_ANADIG_USB_CHRG_DETECT_EN_B |
+			BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B,
+			~0);
+}
+
 static void __init imx6q_post_populate(void)
 {
 	imx6q_config_on_boot();
+	imx6q_usb_init();
 }
 
 static void __init imx6q_init_machine(void)
-- 
1.7.5.4

[PATCH v2 09/12] ARM: imx6q: disable usb charger detector

[Shawn Guo]

On Mon, May 21, 2012 at 05:23:54PM +0800, Richard Zhao wrote:
> +static void __init imx6q_usb_init(void)
> +{
> +#define HW_ANADIG_USB1_CHRG_DETECT		0x000001b0
> +#define HW_ANADIG_USB2_CHRG_DETECT		0x00000210
> +
> +#define BM_ANADIG_USB_CHRG_DETECT_EN_B		0x00100000
> +#define BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B	0x00080000
> +
> +	/*
> +	 * The external charger detector needs to be disabled,
> +	 * or the signal at DP will be poor
> +	 */
> +
Nit: drop this unnecessary new line.

Regards,
Shawn

> +	anatop_write_reg(NULL, HW_ANADIG_USB1_CHRG_DETECT,
> +			BM_ANADIG_USB_CHRG_DETECT_EN_B
> +			| BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B,
> +			~0);
> +	anatop_write_reg(NULL, HW_ANADIG_USB2_CHRG_DETECT,
> +			BM_ANADIG_USB_CHRG_DETECT_EN_B |
> +			BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B,
> +			~0);
> +}
> +

[PATCH v2 09/12] ARM: imx6q: disable usb charger detector

[Richard Zhao]

On Tue, May 22, 2012 at 11:48:11AM +0800, Shawn Guo wrote:
> On Mon, May 21, 2012 at 05:23:54PM +0800, Richard Zhao wrote:
> > +static void __init imx6q_usb_init(void)
> > +{
> > +#define HW_ANADIG_USB1_CHRG_DETECT		0x000001b0
> > +#define HW_ANADIG_USB2_CHRG_DETECT		0x00000210
> > +
> > +#define BM_ANADIG_USB_CHRG_DETECT_EN_B		0x00100000
> > +#define BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B	0x00080000
> > +
> > +	/*
> > +	 * The external charger detector needs to be disabled,
> > +	 * or the signal at DP will be poor
> > +	 */
> > +
> Nit: drop this unnecessary new line.
OK. 

Thanks
Richard
> 
> Regards,
> Shawn
> 
> > +	anatop_write_reg(NULL, HW_ANADIG_USB1_CHRG_DETECT,
> > +			BM_ANADIG_USB_CHRG_DETECT_EN_B
> > +			| BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B,
> > +			~0);
> > +	anatop_write_reg(NULL, HW_ANADIG_USB2_CHRG_DETECT,
> > +			BM_ANADIG_USB_CHRG_DETECT_EN_B |
> > +			BM_ANADIG_USB_CHRG_DETECT_CHK_CHRG_B,
> > +			~0);
> > +}
> > +
> 

[PATCH v2 10/12] 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 |   18 ++++++++++++-
 arch/arm/boot/dts/imx6q.dtsi          |   44 +++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 1dd2261..1d28957 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -48,10 +48,26 @@
 					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 */
+			usb at 02184000 { /* USB OTG */
+				fsl,vbus-power-gpios = <&gpio3 22 0>;
+				status = "okay";
+			};
+
+			usb at 02184200 { /* USB1 */
+				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 8c90cba..f67048e 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", "fsl,imx23-usbphy";
 				reg = <0x020c9000 0x1000>;
 				interrupts = <0 44 0x04>;
 			};
 
-			usbphy at 020ca000 { /* USBPHY2 */
+			usbphy2: usbphy at 020ca000 {
+				compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
 				reg = <0x020ca000 0x1000>;
 				interrupts = <0 45 0x04>;
 			};
@@ -485,6 +487,9 @@
 				compatible = "fsl,imx6q-iomuxc";
 				reg = <0x020e0000 0x4000>;
 
+				pinctrl-names = "default";
+				pinctrl-0 = <&pinctrl_gpio_hog>;
+
 				/* shared pinctrl settings */
 				audmux {
 					pinctrl_audmux_1: audmux-1 {
@@ -495,6 +500,11 @@
 					};
 				};
 
+				gpios {
+					pinctrl_gpio_hog: gpiohog {
+					};
+				};
+
 				i2c1 {
 					pinctrl_i2c1_1: i2c1grp-1 {
 						fsl,pins = <137 0x4001b8b1	/* MX6Q_PAD_EIM_D21__I2C1_SCL */
@@ -573,6 +583,36 @@
 				reg = <0x0217c000 0x4000>;
 			};
 
+			usb at 02184000 { /* USB OTG */
+				compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
+				reg = <0x02184000 0x200>;
+				interrupts = <0 43 0x04>;
+				fsl,usbphy = <&usbphy1>;
+				status = "disabled";
+			};
+
+			usb at 02184200 { /* USB1 */
+				compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
+				reg = <0x02184200 0x200>;
+				interrupts = <0 40 0x04>;
+				fsl,usbphy = <&usbphy2>;
+				status = "disabled";
+			};
+
+			usb at 02184400 { /* USB2 */
+				compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
+				reg = <0x02184400 0x200>;
+				interrupts = <0 41 0x04>;
+				status = "disabled";
+			};
+
+			usb at 02184600 { /* USB3 */
+				compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
+				reg = <0x02184600 0x200>;
+				interrupts = <0 42 0x04>;
+				status = "disabled";
+			};
+
 			ethernet at 02188000 {
 				compatible = "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
-- 
1.7.5.4

[PATCH v2 10/12] ARM: dts: imx6q-sabrelite: add usb devices

[Shawn Guo]

On Mon, May 21, 2012 at 05:23:55PM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-sabrelite.dts |   18 ++++++++++++-
>  arch/arm/boot/dts/imx6q.dtsi          |   44 +++++++++++++++++++++++++++++++-
>  2 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 1dd2261..1d28957 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -48,10 +48,26 @@
>  					status = "okay";
>  				};
>  			};
> -
> +			iomuxc at 020e0000 {
> +				gpios {
> +					pinctrl_gpio_hog: gpiohog {

This isn't really welcomed, and should really be removed after we have
gpio_request() be able to call pinctrl to set the mux.

> +						fsl,pins = <1044 0x80000000
> +							   144  0x80000000>;	/* MX6Q_PAD_GPIO_17__GPIO_7_12 */

Here are what I get from fsl,imx6q-pinctrl.txt.  The comment was put on
the wrong line?

MX6Q_PAD_EIM_D22__GPIO_3_22                     144
MX6Q_PAD_GPIO_17__GPIO_7_12                     1044 

> +					};
> +				};
> +			};
>  		};
>  
...

> @@ -495,6 +500,11 @@
>  					};
>  				};
>  
> +				gpios {
> +					pinctrl_gpio_hog: gpiohog {
> +					};
> +				};
> +
Why the empty nodes?

-- 
Regards,
Shawn

[PATCH v2 10/12] ARM: dts: imx6q-sabrelite: add usb devices

[Richard Zhao]

On Tue, May 22, 2012 at 01:10:08PM +0800, Shawn Guo wrote:
> On Mon, May 21, 2012 at 05:23:55PM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx6q-sabrelite.dts |   18 ++++++++++++-
> >  arch/arm/boot/dts/imx6q.dtsi          |   44 +++++++++++++++++++++++++++++++-
> >  2 files changed, 59 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > index 1dd2261..1d28957 100644
> > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> > @@ -48,10 +48,26 @@
> >  					status = "okay";
> >  				};
> >  			};
> > -
> > +			iomuxc at 020e0000 {
> > +				gpios {
> > +					pinctrl_gpio_hog: gpiohog {
> 
> This isn't really welcomed, and should really be removed after we have
> gpio_request() be able to call pinctrl to set the mux.
Yes, but we don't have now. If I add dependency which don't even hit
next tree, I experienced people refuse pick my patch.
Of course, If you're ok to pick other patches, I can leave the code
locally for test.
> 
> > +						fsl,pins = <1044 0x80000000
> > +							   144  0x80000000>;	/* MX6Q_PAD_GPIO_17__GPIO_7_12 */
> 
> Here are what I get from fsl,imx6q-pinctrl.txt.  The comment was put on
> the wrong line?
> 
> MX6Q_PAD_EIM_D22__GPIO_3_22                     144
> MX6Q_PAD_GPIO_17__GPIO_7_12                     1044 
Good catch.
> 
> > +					};
> > +				};
> > +			};
> >  		};
> >  
> ...
> 
> > @@ -495,6 +500,11 @@
> >  					};
> >  				};
> >  
> > +				gpios {
> > +					pinctrl_gpio_hog: gpiohog {
> > +					};
> > +				};
> > +
> Why the empty nodes?
It's place holder that let use know to add gpio hog-on-boot in board dts.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn
> 

[PATCH v2 10/12] ARM: dts: imx6q-sabrelite: add usb devices

[Shawn Guo]

On Tue, May 22, 2012 at 01:59:02PM +0800, Richard Zhao wrote:
> Yes, but we don't have now. If I add dependency which don't even hit
> next tree, I experienced people refuse pick my patch.
> Of course, If you're ok to pick other patches, I can leave the code
> locally for test.

I'm fine for now.

> > > +				gpios {
> > > +					pinctrl_gpio_hog: gpiohog {
> > > +					};
> > > +				};
> > > +
> > Why the empty nodes?
> It's place holder that let use know to add gpio hog-on-boot in board dts.
> 
It's meaningless.  Please remove it.

-- 
Regards,
Shawn

[PATCH v2 10/12] ARM: dts: imx6q-sabrelite: add usb devices

[Richard Zhao]

On Tue, May 22, 2012 at 02:08:31PM +0800, Shawn Guo wrote:
> On Tue, May 22, 2012 at 01:59:02PM +0800, Richard Zhao wrote:
> > Yes, but we don't have now. If I add dependency which don't even hit
> > next tree, I experienced people refuse pick my patch.
> > Of course, If you're ok to pick other patches, I can leave the code
> > locally for test.
> 
> I'm fine for now.
> 
> > > > +				gpios {
> > > > +					pinctrl_gpio_hog: gpiohog {
> > > > +					};
> > > > +				};
> > > > +
> > > Why the empty nodes?
> > It's place holder that let use know to add gpio hog-on-boot in board dts.
> > 
> It's meaningless.  Please remove it.
Ok.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn
> 

[PATCH v2 11/12] ARM: mxs: clk_register_clkdev mx28 usb clocks

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/clk/mxs/clk-imx28.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
index 2826a26..5330b27 100644
--- a/drivers/clk/mxs/clk-imx28.c
+++ b/drivers/clk/mxs/clk-imx28.c
@@ -328,6 +328,10 @@ int __init mx28_clocks_init(void)
 	clk_register_clkdevs(clks[fec], fec_lookups, ARRAY_SIZE(fec_lookups));
 	clk_register_clkdevs(clks[can0], can0_lookups, ARRAY_SIZE(can0_lookups));
 	clk_register_clkdevs(clks[can1], can1_lookups, ARRAY_SIZE(can1_lookups));
+	clk_register_clkdev(clks[usb0_pwr], NULL, "8007c000.usbphy");
+	clk_register_clkdev(clks[usb1_pwr], NULL, "8007e000.usbphy");
+	clk_register_clkdev(clks[usb0], NULL, "80080000.usbctrl");
+	clk_register_clkdev(clks[usb1], NULL, "80090000.usbctrl");
 
 	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
 		clk_prepare_enable(clks[clks_init_on[i]]);
-- 
1.7.5.4

[PATCH v2 11/12] ARM: mxs: clk_register_clkdev mx28 usb clocks

[Shawn Guo]

On Mon, May 21, 2012 at 05:23:56PM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/clk/mxs/clk-imx28.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
> index 2826a26..5330b27 100644
> --- a/drivers/clk/mxs/clk-imx28.c
> +++ b/drivers/clk/mxs/clk-imx28.c
> @@ -328,6 +328,10 @@ int __init mx28_clocks_init(void)
>  	clk_register_clkdevs(clks[fec], fec_lookups, ARRAY_SIZE(fec_lookups));
>  	clk_register_clkdevs(clks[can0], can0_lookups, ARRAY_SIZE(can0_lookups));
>  	clk_register_clkdevs(clks[can1], can1_lookups, ARRAY_SIZE(can1_lookups));
> +	clk_register_clkdev(clks[usb0_pwr], NULL, "8007c000.usbphy");
> +	clk_register_clkdev(clks[usb1_pwr], NULL, "8007e000.usbphy");
> +	clk_register_clkdev(clks[usb0], NULL, "80080000.usbctrl");
> +	clk_register_clkdev(clks[usb1], NULL, "80090000.usbctrl");
>  
You changed usb node name from "usbctrl" to "usb".  The clk lookup
needs to be updated correspondingly.

-- 
Regards,
Shawn

[PATCH v2 11/12] ARM: mxs: clk_register_clkdev mx28 usb clocks

[Richard Zhao]

On Tue, May 22, 2012 at 12:48:48PM +0800, Shawn Guo wrote:
> On Mon, May 21, 2012 at 05:23:56PM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  drivers/clk/mxs/clk-imx28.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
> > index 2826a26..5330b27 100644
> > --- a/drivers/clk/mxs/clk-imx28.c
> > +++ b/drivers/clk/mxs/clk-imx28.c
> > @@ -328,6 +328,10 @@ int __init mx28_clocks_init(void)
> >  	clk_register_clkdevs(clks[fec], fec_lookups, ARRAY_SIZE(fec_lookups));
> >  	clk_register_clkdevs(clks[can0], can0_lookups, ARRAY_SIZE(can0_lookups));
> >  	clk_register_clkdevs(clks[can1], can1_lookups, ARRAY_SIZE(can1_lookups));
> > +	clk_register_clkdev(clks[usb0_pwr], NULL, "8007c000.usbphy");
> > +	clk_register_clkdev(clks[usb1_pwr], NULL, "8007e000.usbphy");
> > +	clk_register_clkdev(clks[usb0], NULL, "80080000.usbctrl");
> > +	clk_register_clkdev(clks[usb1], NULL, "80090000.usbctrl");
> >  
> You changed usb node name from "usbctrl" to "usb".  The clk lookup
> needs to be updated correspondingly.
Yes, I just noticed it too.

Thanks
Richard
> 
> -- 
> Regards,
> Shawn
> 

[PATCH v2 12/12] ARM: dts: imx28-evk: add usb devices

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 arch/arm/boot/dts/imx28-evk.dts |   18 ++++++++++++++++++
 arch/arm/boot/dts/imx28.dtsi    |   12 ++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
index ee520a5..ca6fe30 100644
--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -73,10 +73,28 @@
 				pinctrl-0 = <&duart_pins_a>;
 				status = "okay";
 			};
+
+			usbphy0: usbphy at 8007c000 {
+				status = "okay";
+			};
+
+			usbphy1: usbphy at 8007e000 {
+				status = "okay";
+			};
 		};
 	};
 
 	ahb at 80080000 {
+		usb0: usb at 80080000 {
+			fsl,vbus-power = <&gpio3 9 0>;
+			status = "okay";
+		};
+
+		usb1: usb at 80090000 {
+			fsl,vbus-power = <&gpio3 8 0>;
+			status = "okay";
+		};
+
 		mac0: ethernet at 800f0000 {
 			phy-mode = "rmii";
 			pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 4634cb8..502ad05 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -441,11 +441,13 @@
 			};
 
 			usbphy0: usbphy at 8007c000 {
+				compatible = "fsl,imx28-usbphy", "fsl,imx23-usbphy";
 				reg = <0x8007c000 0x2000>;
 				status = "disabled";
 			};
 
 			usbphy1: usbphy at 8007e000 {
+				compatible = "fsl,imx28-usbphy", "fsl,imx23-usbphy";
 				reg = <0x8007e000 0x2000>;
 				status = "disabled";
 			};
@@ -459,13 +461,19 @@
 		reg = <0x80080000 0x80000>;
 		ranges;
 
-		usbctrl0: usbctrl at 80080000 {
+		usb0: usb at 80080000 {
+			compatible = "fsl,imx28-usb", "fsl,imx31-usb";
 			reg = <0x80080000 0x10000>;
+			interrupts = <93>;
+			fsl,usbphy = <&usbphy0>;
 			status = "disabled";
 		};
 
-		usbctrl1: usbctrl at 80090000 {
+		usb1: usb at 80090000 {
+			compatible = "fsl,imx28-usb", "fsl,imx31-usb";
 			reg = <0x80090000 0x10000>;
+			interrupts = <92>;
+			fsl,usbphy = <&usbphy1>;
 			status = "disabled";
 		};
 
-- 
1.7.5.4

[PATCH v2 12/12] ARM: dts: imx28-evk: add usb devices

[Shawn Guo]

On Mon, May 21, 2012 at 05:23:57PM +0800, Richard Zhao wrote:
>  	ahb at 80080000 {
> +		usb0: usb at 80080000 {
> +			fsl,vbus-power = <&gpio3 9 0>;

fsl,vbus-power-gpios

> +			status = "okay";
> +		};
> +
> +		usb1: usb at 80090000 {
> +			fsl,vbus-power = <&gpio3 8 0>;

Ditto

> +			status = "okay";
> +		};
> +

-- 
Regards,
Shawn

[PATCH v2 12/12] ARM: dts: imx28-evk: add usb devices

[Richard Zhao]

On Tue, May 22, 2012 at 12:52:43PM +0800, Shawn Guo wrote:
> On Mon, May 21, 2012 at 05:23:57PM +0800, Richard Zhao wrote:
> >  	ahb at 80080000 {
> > +		usb0: usb at 80080000 {
> > +			fsl,vbus-power = <&gpio3 9 0>;
> 
> fsl,vbus-power-gpios
Yes.
> 
> > +			status = "okay";
> > +		};
> > +
> > +		usb1: usb at 80090000 {
> > +			fsl,vbus-power = <&gpio3 8 0>;
> 
> Ditto
Yes.

I'll test mx28 too next time before send out patch.
Maybe need your help :)

Thanks
Richard
> 
> > +			status = "okay";
> > +		};
> > +
> 
> -- 
> Regards,
> Shawn
> 

[PATCH v2 12/12] ARM: dts: imx28-evk: add usb devices

[Marek Vasut]

Dear Richard Zhao,

> On Tue, May 22, 2012 at 12:52:43PM +0800, Shawn Guo wrote:
> > On Mon, May 21, 2012 at 05:23:57PM +0800, Richard Zhao wrote:
> > >  	ahb at 80080000 {
> > > 
> > > +		usb0: usb at 80080000 {
> > > +			fsl,vbus-power = <&gpio3 9 0>;
> > 
> > fsl,vbus-power-gpios
> 
> Yes.
> 
> > > +			status = "okay";
> > > +		};
> > > +
> > > +		usb1: usb at 80090000 {
> > > +			fsl,vbus-power = <&gpio3 8 0>;
> > 
> > Ditto
> 
> Yes.
> 
> I'll test mx28 too next time before send out patch.
> Maybe need your help :)

I'm still stuck a bit with the DT conversion :-( I'll start testing ASAP.

> Thanks
> Richard
> 
> > > +			status = "okay";
> > > +		};
> > > +

Best regards,
Marek Vasut

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Fabio Estevam]

Hi Richard,

On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
<richard.zhao@freescale.com> wrote:
> The work is based on ?ci13xxx rework done by Alexander Shishkin.
>
> Status:
> ?- this version only support host
> ?- usbotg controler works at host role
>
> Changes since V1:
> ?- Merge the work of Marek
> ?- re-implement connect change notify
> ?- imx6q: move part of code to clk
> ?- imx6q: add config-on-boot gpios

Can you please also add the driver into the defconfig files?

What about the existing USB host drivers for mx27/mx31/mx35/mx25/mx35?

Is the idea to switch to drivers/usb/chipidea/host.c too?

Regards,

Fabio Estevam

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Marek Vasut]

Dear Fabio Estevam,

> Hi Richard,
> 
> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
> 
> <richard.zhao@freescale.com> wrote:
> > The work is based on  ci13xxx rework done by Alexander Shishkin.
> > 
> > Status:
> >  - this version only support host
> >  - usbotg controler works at host role
> > 
> > Changes since V1:
> >  - Merge the work of Marek
> >  - re-implement connect change notify
> >  - imx6q: move part of code to clk
> >  - imx6q: add config-on-boot gpios
> 
> Can you please also add the driver into the defconfig files?
> 
> What about the existing USB host drivers for mx27/mx31/mx35/mx25/mx35?
> 
> Is the idea to switch to drivers/usb/chipidea/host.c too?

I think the core is the same, it's just the ci13xxx-imx driver that'd need to be 
written differently for these

> 
> Regards,
> 
> Fabio Estevam

Best regards,
Marek Vasut

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Subodh Nijsure]

On 05/21/2012 12:41 PM, Marek Vasut wrote:
> Dear Fabio Estevam,
>
>> Hi Richard,
>>
>> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
>>
>> <richard.zhao@freescale.com>  wrote:
>>> The work is based on  ci13xxx rework done by Alexander Shishkin.
>>>
>>> Status:
>>>   - this version only support host
>>>   - usbotg controler works at host role
>>>
>>> Changes since V1:
>>>   - Merge the work of Marek
>>>   - re-implement connect change notify
>>>   - imx6q: move part of code to clk
>>>   - imx6q: add config-on-boot gpios
>> Can you please also add the driver into the defconfig files?
>>
>> What about the existing USB host drivers for mx27/mx31/mx35/mx25/mx35?
>>
>> Is the idea to switch to drivers/usb/chipidea/host.c too?
> I think the core is the same, it's just the ci13xxx-imx driver that'd need to be
> written differently for these

So that means driver that you did are not going to be up streamed for 
mx28/mxs? And those interested should be tracking this patch set?

-Subodh

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Marek Vasut]

Dear Subodh Nijsure,

> On 05/21/2012 12:41 PM, Marek Vasut wrote:
> > Dear Fabio Estevam,
> > 
> >> Hi Richard,
> >> 
> >> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
> >> 
> >> <richard.zhao@freescale.com>  wrote:
> >>> The work is based on  ci13xxx rework done by Alexander Shishkin.
> >>> 
> >>> Status:
> >>>   - this version only support host
> >>>   - usbotg controler works at host role
> >>> 
> >>> Changes since V1:
> >>>   - Merge the work of Marek
> >>>   - re-implement connect change notify
> >>>   - imx6q: move part of code to clk
> >>>   - imx6q: add config-on-boot gpios
> >> 
> >> Can you please also add the driver into the defconfig files?
> >> 
> >> What about the existing USB host drivers for mx27/mx31/mx35/mx25/mx35?
> >> 
> >> Is the idea to switch to drivers/usb/chipidea/host.c too?
> > 
> > I think the core is the same, it's just the ci13xxx-imx driver that'd
> > need to be written differently for these
> 
> So that means driver that you did are not going to be up streamed for
> mx28/mxs? And those interested should be tracking this patch set?

This here is definitelly going to support mx28 and I'm slowly working through 
the DT stuff to test this, all right?

We were talking about imx.{xz|x,y is [0-9a-z]} \ imx.{23,28,6q}

> 
> -Subodh

Best regards,
Marek Vasut

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Chen Peter-B29397]

 
> > >
> > >> Hi Richard,
> > >>
> > >> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
> > >>
> > >> <richard.zhao@freescale.com>  wrote:
> > >>> The work is based on  ci13xxx rework done by Alexander Shishkin.
> > >>>
> > >>> Status:
> > >>>   - this version only support host
> > >>>   - usbotg controler works at host role
> > >>>
> > >>> Changes since V1:
> > >>>   - Merge the work of Marek
> > >>>   - re-implement connect change notify
> > >>>   - imx6q: move part of code to clk
> > >>>   - imx6q: add config-on-boot gpios
> > >>
> > >> Can you please also add the driver into the defconfig files?
> > >>
> > >> What about the existing USB host drivers for
> mx27/mx31/mx35/mx25/mx35?
> > >>
> > >> Is the idea to switch to drivers/usb/chipidea/host.c too?
> > >
> > > I think the core is the same, it's just the ci13xxx-imx driver that'd
> > > need to be written differently for these
> >
> > So that means driver that you did are not going to be up streamed for
> > mx28/mxs? And those interested should be tracking this patch set?
> 
> This here is definitelly going to support mx28 and I'm slowly working
> through
> the DT stuff to test this, all right?
> 
> We were talking about imx.{xz|x,y is [0-9a-z]} \ imx.{23,28,6q}
>
This driver needs to work on both DT and non-DT support.
Let's do it after next version when we need to add it for old mxc platform.

 
> >
> > -Subodh
> 
> Best regards,
> Marek Vasut

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Richard Zhao]

On Tue, May 22, 2012 at 09:38:33AM +0800, Chen Peter-B29397 wrote:
>  
> > > >
> > > >> Hi Richard,
> > > >>
> > > >> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
> > > >>
> > > >> <richard.zhao@freescale.com>  wrote:
> > > >>> The work is based on  ci13xxx rework done by Alexander Shishkin.
> > > >>>
> > > >>> Status:
> > > >>>   - this version only support host
> > > >>>   - usbotg controler works at host role
> > > >>>
> > > >>> Changes since V1:
> > > >>>   - Merge the work of Marek
> > > >>>   - re-implement connect change notify
> > > >>>   - imx6q: move part of code to clk
> > > >>>   - imx6q: add config-on-boot gpios
> > > >>
> > > >> Can you please also add the driver into the defconfig files?
> > > >>
> > > >> What about the existing USB host drivers for
> > mx27/mx31/mx35/mx25/mx35?
> > > >>
> > > >> Is the idea to switch to drivers/usb/chipidea/host.c too?
> > > >
> > > > I think the core is the same, it's just the ci13xxx-imx driver that'd
> > > > need to be written differently for these
> > >
> > > So that means driver that you did are not going to be up streamed for
> > > mx28/mxs? And those interested should be tracking this patch set?
> > 
> > This here is definitelly going to support mx28 and I'm slowly working
> > through
> > the DT stuff to test this, all right?
> > 
> > We were talking about imx.{xz|x,y is [0-9a-z]} \ imx.{23,28,6q}
> >
> This driver needs to work on both DT and non-DT support.
> Let's do it after next version when we need to add it for old mxc platform.
I don't think so. We only need to go right direction and don't do
extra work for what is going to be abandoned.

Thanks
Richard
> 
>  
> > >
> > > -Subodh
> > 
> > Best regards,
> > Marek Vasut
> 

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Chen Peter-B29397]

 
> > >
> > > This here is definitelly going to support mx28 and I'm slowly working
> > > through
> > > the DT stuff to test this, all right?
> > >
> > > We were talking about imx.{xz|x,y is [0-9a-z]} \ imx.{23,28,6q}
> > >
> > This driver needs to work on both DT and non-DT support.
> > Let's do it after next version when we need to add it for old mxc
> platform.
> I don't think so. We only need to go right direction and don't do
> extra work for what is going to be abandoned.
Are you sure all i.mx platform will support DT in future?

> 
> Thanks
> Richard
> >
> >
> > > >
> > > > -Subodh
> > >
> > > Best regards,
> > > Marek Vasut
> >

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Richard Zhao]

On Tue, May 22, 2012 at 09:44:14AM +0800, Chen Peter-B29397 wrote:
>  
> > > >
> > > > This here is definitelly going to support mx28 and I'm slowly working
> > > > through
> > > > the DT stuff to test this, all right?
> > > >
> > > > We were talking about imx.{xz|x,y is [0-9a-z]} \ imx.{23,28,6q}
> > > >
> > > This driver needs to work on both DT and non-DT support.
> > > Let's do it after next version when we need to add it for old mxc
> > platform.
> > I don't think so. We only need to go right direction and don't do
> > extra work for what is going to be abandoned.
> Are you sure all i.mx platform will support DT in future?
Finally, yes.
> 
> > 
> > Thanks
> > Richard
> > >
> > >
> > > > >
> > > > > -Subodh
> > > >
> > > > Best regards,
> > > > Marek Vasut
> > >

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Shawn Guo]

On 22 May 2012 09:44, Chen Peter-B29397 <B29397@freescale.com> wrote:
>> > This driver needs to work on both DT and non-DT support.
>> > Let's do it after next version when we need to add it for old mxc
>> platform.
>> I don't think so. We only need to go right direction and don't do
>> extra work for what is going to be abandoned.
> Are you sure all i.mx platform will support DT in future?
>
I'm with Richard on this.  With all new drivers support DT only, we
can attract people to convert their platform to DT, if they want to
use the new driver.  Otherwise, people lose motivation to move to DT.

Regards,
Shawn

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Richard Zhao]

On Mon, May 21, 2012 at 04:34:15PM -0300, Fabio Estevam wrote:
> Hi Richard,
> 
> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
> <richard.zhao@freescale.com> wrote:
> > The work is based on ?ci13xxx rework done by Alexander Shishkin.
> >
> > Status:
> > ?- this version only support host
> > ?- usbotg controler works at host role
> >
> > Changes since V1:
> > ?- Merge the work of Marek
> > ?- re-implement connect change notify
> > ?- imx6q: move part of code to clk
> > ?- imx6q: add config-on-boot gpios
> 
> Can you please also add the driver into the defconfig files?
Thanks for you reminder.
> 
> What about the existing USB host drivers for mx27/mx31/mx35/mx25/mx35?
> 
> Is the idea to switch to drivers/usb/chipidea/host.c too?
Yes. but I'm now just focusing on imx6q and imx28.

Thanks
Richard
> 
> Regards,
> 
> Fabio Estevam
> 

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Dirk Behme]

On 22.05.2012 07:08, Richard Zhao wrote:
> On Mon, May 21, 2012 at 04:34:15PM -0300, Fabio Estevam wrote:
>> Hi Richard,
>>
>> On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
>> <richard.zhao@freescale.com> wrote:
>>> The work is based on  ci13xxx rework done by Alexander Shishkin.
>>>
>>> Status:
>>>  - this version only support host
>>>  - usbotg controler works at host role
>>>
>>> Changes since V1:
>>>  - Merge the work of Marek
>>>  - re-implement connect change notify
>>>  - imx6q: move part of code to clk
>>>  - imx6q: add config-on-boot gpios
>> Can you please also add the driver into the defconfig files?
> Thanks for you reminder.

For i.MX6 I use something like [1].

Best regards

Dirk

[1]

From: Dirk Behme <dirk.behme@de.bosch.com>
Subject: [PATCH] ARM: imx_v6_v7_defconfig: Enable chipidea USB host driver

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
  arch/arm/configs/imx_v6_v7_defconfig |    8 ++++++--
  1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig 
b/arch/arm/configs/imx_v6_v7_defconfig
index c3a828e..45001ef 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -130,6 +130,7 @@ CONFIG_GPIO_SYSFS=y
  CONFIG_WATCHDOG=y
  CONFIG_IMX2_WDT=y
  CONFIG_MFD_MC13XXX=y
+CONFIG_MFD_ANATOP=y
  CONFIG_REGULATOR=y
  CONFIG_REGULATOR_FIXED_VOLTAGE=y
  CONFIG_REGULATOR_MC13783=y
@@ -166,9 +167,12 @@ CONFIG_LOGO_LINUX_MONO=y
  CONFIG_LOGO_LINUX_VGA16=y
  CONFIG_LOGO_LINUX_CLUT224=y
  CONFIG_USB=y
-CONFIG_USB_EHCI_HCD=y
-CONFIG_USB_EHCI_MXC=y
+CONFIG_USB_EHCI_ROOT_HUB_TT=y
+CONFIG_USB_CHIPIDEA=y
+CONFIG_USB_CHIPIDEA_HOST=y
+CONFIG_USB_CHIPIDEA_DEBUG=y
  CONFIG_USB_STORAGE=y
+CONFIG_USB_MXS_PHY=y
  CONFIG_MMC=y
  CONFIG_MMC_SDHCI=y
  CONFIG_MMC_SDHCI_PLTFM=y

[PATCH v2 00/12] add imx usb driver for mx28/6x

[Richard Zhao]

On Tue, May 22, 2012 at 01:08:31PM +0800, Richard Zhao wrote:
> On Mon, May 21, 2012 at 04:34:15PM -0300, Fabio Estevam wrote:
> > Hi Richard,
> > 
> > On Mon, May 21, 2012 at 6:23 AM, Richard Zhao
> > <richard.zhao@freescale.com> wrote:
> > > The work is based on ?ci13xxx rework done by Alexander Shishkin.
> > >
> > > Status:
> > > ?- this version only support host
> > > ?- usbotg controler works at host role
> > >
> > > Changes since V1:
> > > ?- Merge the work of Marek
> > > ?- re-implement connect change notify
> > > ?- imx6q: move part of code to clk
> > > ?- imx6q: add config-on-boot gpios
> > 
> > Can you please also add the driver into the defconfig files?
> Thanks for you reminder.
I'll not put it in defconfig until we can get ride of mxc usb driver.
It cannot be in one image.

Thanks
Richard
> > 
> > What about the existing USB host drivers for mx27/mx31/mx35/mx25/mx35?
> > 
> > Is the idea to switch to drivers/usb/chipidea/host.c too?
> Yes. but I'm now just focusing on imx6q and imx28.
> 
> Thanks
> Richard
> > 
> > Regards,
> > 
> > Fabio Estevam
> > 
> 
> --
> 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
>