add imx usb driver for mx28/6x

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

Signed-off-by Richard Zhao <[email protected]>

---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 };
 
 

Quoted 26 lines · Richard Zhao

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

Signed-off-by Richard Zhao <[email protected]>

---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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)
 }
 

include/linux/usb/otg.h usb_phy_set_suspend(struct usb_phy *x, int suspend) · line 261

Quoted 4 lines

include/linux/usb/otg.h +3 -0

 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.

Signature · 2 lines

-- 
balbi

Quoted 27 lines · Felipe Balbi

Quoted 26 lines · Richard Zhao

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

Signed-off-by Richard Zhao <[email protected]>

---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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)
 }
 

Quoted 7 lines

include/linux/usb/otg.h usb_phy_set_suspend(struct usb_phy *x, int suspend) · line 261

Quoted 4 lines

include/linux/usb/otg.h +3 -0

 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.

Quoted 2 lines

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.

Quoted 35 lines · Richard Zhao

Quoted 27 lines · Felipe Balbi

Quoted 26 lines · Richard Zhao

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

Signed-off-by Richard Zhao <[email protected]>

---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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)
 }
 

Quoted 7 lines

include/linux/usb/otg.h usb_phy_set_suspend(struct usb_phy *x, int suspend) · line 261

Quoted 4 lines

include/linux/usb/otg.h +3 -0

 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.

Quoted 2 lines

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:

Quoted 5 lines

Quoted 3 lines

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 ?

Signature · 1 line

balbi

Quoted 35 lines · Richard Zhao

Quoted 27 lines · Felipe Balbi

Quoted 26 lines · Richard Zhao

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

Signed-off-by Richard Zhao <[email protected]>

---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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)
 }
 

Quoted 7 lines

include/linux/usb/otg.h usb_phy_set_suspend(struct usb_phy *x, int suspend) · line 261

Quoted 4 lines

include/linux/usb/otg.h +3 -0

 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.

Quoted 2 lines

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:

(quoted diff) +1 -1

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

Quoted 4 lines

Quoted 2 lines

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 ?

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

Signed-off-by Richard Zhao <[email protected]>

---
 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +0 -0

index 38ab3f4..385641d 100644

include/linux/usb/otg.h +9 -0

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

Signature · 7 lines

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

Quoted 24 lines

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

Signed-off-by Richard Zhao <[email protected]>

---

 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 

Quoted 1 line

};

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

Quoted 16 lines

include/linux/usb/otg.h +8 -0

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

Quoted 4 lines

otg_start_srp(struct usb_otg *otg)
{

if (otg && otg->start_srp)

Dear Greg KH,

Quoted 35 lines · Richard Zhao

Quoted 24 lines

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

Signed-off-by Richard Zhao <[email protected]>

---

 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 

Quoted 1 line

};

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

Quoted 3 lines

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 ?

Quoted 46 lines

Dear Greg KH,

Quoted 43 lines · Richard Zhao

Quoted 24 lines

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

Signed-off-by Richard Zhao <[email protected]>

---

 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 

Quoted 1 line

};

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

Quoted 1 line

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

Dear Greg KH,

Quoted 47 lines · Marek Vasut

Quoted 46 lines

Dear Greg KH,

Quoted 43 lines · Richard Zhao

Quoted 24 lines

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

Signed-off-by Richard Zhao <[email protected]>

---

 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 

Quoted 1 line

};

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

Quoted 5 lines

Quoted 1 line

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.

Quoted 52 lines

Dear Greg KH,

Quoted 50 lines · Marek Vasut

Quoted 48 lines

Dear Greg KH,

Quoted 41 lines · Richard Zhao

Quoted 24 lines

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

Signed-off-by Richard Zhao <[email protected]>

---

 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 

Quoted 1 line

};

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

Quoted 3 lines

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 ?

Quoted 5 lines

Quoted 3 lines

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

Dear Greg KH,

Quoted 59 lines · Marek Vasut

Quoted 57 lines

Dear Greg KH,

Quoted 48 lines · Marek Vasut

Quoted 47 lines

Dear Greg KH,

Quoted 44 lines · Richard Zhao

Quoted 24 lines

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

Signed-off-by Richard Zhao <[email protected]>

---

 include/linux/usb/otg.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/usb/otg.h +4 -0

index 38ab3f4..385641d 100644
@@ -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);
 

Quoted 1 line

};

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

Quoted 5 lines

Quoted 1 line

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.

Quoted 2 lines

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?

Quoted 1 line

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

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

Quoted 2 lines

greg k-h

Best regards,
Marek Vasut

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 <[email protected]>

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

drivers/usb/chipidea/ci.h +2 -0

index 50911f8..71d7080 100644
@@ -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;
 };

drivers/usb/chipidea/core.c +4 -0

index 15e03b3..0492786 100644
@@ -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);

drivers/usb/chipidea/core.c +0 -0

 	if (ret < 0) {

drivers/usb/chipidea/host.c +1 -0

index 9eacd21..7bc11a5 100644
@@ -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)

drivers/usb/chipidea/udc.c +7 -4

index 51f9694..22139fae 100644
@@ -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);

include/linux/usb/chipidea.h +2 -0

index edb90d6..c46c9e9 100644
@@ -5,12 +5,15 @@
 #ifndef __LINUX_USB_CHIPIDEA_H
 #define __LINUX_USB_CHIPIDEA_H
 
+#include <linux/usb/otg.h>
+

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.

Richard Zhao <[email protected]> writes:

Hi,

Quoted 3 lines

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]

drivers/usb/chipidea/core.c static int __devinit ci_hdrc_probe(struct platform_device *pdev) · line 371

Quoted 7 lines

drivers/usb/chipidea/core.c +4 -0

 	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]

Quoted 11 lines

include/linux/usb/chipidea.h +2 -0

+#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;

include/linux/usb/chipidea.h line 16

Quoted 1 line

include/linux/usb/chipidea.h +1 -0

+	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

Dear Alexander Shishkin,

Quoted 21 lines

Richard Zhao <[email protected]> writes:

Hi,

Quoted 3 lines

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]

drivers/usb/chipidea/core.c static int __devinit ci_hdrc_probe(struct platform_device *pdev) · line 371

Quoted 7 lines

drivers/usb/chipidea/core.c +4 -0

 	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?

Quoted 14 lines

[snip]

Quoted 11 lines

include/linux/usb/chipidea.h +2 -0

+#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;

Quoted 6 lines

include/linux/usb/chipidea.h line 16

Quoted 1 line

include/linux/usb/chipidea.h +1 -0

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

Quoted 5 lines

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

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

Quoted 12 lines

Quoted 5 lines

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

Dear Alexander Shishkin,

Quoted 35 lines

Richard Zhao <[email protected]> writes:

Hi,

Quoted 3 lines

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]

drivers/usb/chipidea/core.c static int __devinit ci_hdrc_probe(struct platform_device *pdev) · line 371

Quoted 7 lines

drivers/usb/chipidea/core.c +4 -0

 	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]

Quoted 11 lines

include/linux/usb/chipidea.h +2 -0

+#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;

Quoted 6 lines

include/linux/usb/chipidea.h line 16

Quoted 1 line

include/linux/usb/chipidea.h +1 -0

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

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 <[email protected]>

---
 drivers/usb/host/ehci-hcd.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

drivers/usb/host/ehci-hcd.c +2 -0

index 5597e60..389252e 100644
@@ -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

Dear Richard Zhao,

Quoted 4 lines

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?

Just add host support.

Signed-off-by Richard Zhao <[email protected]>

Signed-off-by Marek Vasut <[email protected]>

Cc Peter Chen <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

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

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode 100644
index 0000000..beb75d6

Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file +20 -0

@@ -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>;
+};

drivers/usb/chipidea/Makefile +0 -0

index cc34937..ffa2f63 100644
@@ -12,3 +12,11 @@ endif
 ifneq ($(CONFIG_ARCH_MSM),)

(quoted diff) +0 -0

b/drivers/usb/chipidea/ci13xxx_imx.c new file mode 100644 index 0000000..9f64b3c

drivers/usb/chipidea/ci13xxx_imx.c new file +57 -0

@@ -0,0 +1,191 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012 Marek Vasut <[email protected]>
+ * 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;

drivers/usb/chipidea/ci13xxx_imx.c new file +11 -0

+		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) {

drivers/usb/chipidea/ci13xxx_imx.c new file +57 -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);

drivers/usb/chipidea/ci13xxx_imx.c new file +5 -0

+	{ /* sentinel */ }
+};
+
+static struct platform_driver ci13xxx_imx_driver = {
+	.probe = ci13xxx_imx_probe,

Dear Richard Zhao,

Quoted 1 line

Just add host support.

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

Quoted 53 lines

Signed-off-by Richard Zhao <[email protected]>

Signed-off-by Marek Vasut <[email protected]>

Cc Peter Chen <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

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

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode 100644
index 0000000..beb75d6

Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file +20 -0

@@ -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>;
+};

drivers/usb/chipidea/Makefile +4 -0

index cc34937..ffa2f63 100644
@@ -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

drivers/usb/chipidea/Makefile endif · line 22

Quoted 4 lines

drivers/usb/chipidea/Makefile +4 -0

+  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).

Quoted 65 lines

(quoted diff) +0 -0

b/drivers/usb/chipidea/ci13xxx_imx.c new file mode 100644 index 0000000..9f64b3c

drivers/usb/chipidea/ci13xxx_imx.c new file +58 -0

@@ -0,0 +1,191 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012 Marek Vasut <[email protected]>
+ * 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;

drivers/usb/chipidea/ci13xxx_imx.c line 66

Quoted 8 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +8 -0

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

drivers/usb/chipidea/ci13xxx_imx.c line 67

Quoted 1 line

drivers/usb/chipidea/ci13xxx_imx.c new file +1 -0

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

drivers/usb/chipidea/ci13xxx_imx.c line 71

Quoted 4 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +3 -0

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

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

Quoted 16 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +15 -0

+		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;
+	}
+

drivers/usb/chipidea/ci13xxx_imx.c line 92

Quoted 6 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +6 -0

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

drivers/usb/chipidea/ci13xxx_imx.c line 104

Quoted 13 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +12 -0

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

Quoted 62 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +61 -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);

drivers/usb/chipidea/ci13xxx_imx.c line 171

Quoted 6 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +6 -0

+
+	return 0;
+}
+
+static const struct of_device_id ci13xxx_imx_dt_ids[] = {
+	{ .compatible = "fsl,imx31-usb", },

Wow, now it's imx31-usb ?

Quoted 11 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +10 -0

+	{ /* 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,

drivers/usb/chipidea/ci13xxx_imx.c line 186

Quoted 5 lines

drivers/usb/chipidea/ci13xxx_imx.c new file +5 -0

+	 },
+};
+
+module_platform_driver(ci13xxx_imx_driver);
+

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

Quoted 6 lines · Marek Vasut

Dear Richard Zhao,

Quoted 1 line

Just add host support.

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

Thanks.

Quoted 53 lines

Quoted 53 lines

Signed-off-by Richard Zhao <[email protected]>

Signed-off-by Marek Vasut <[email protected]>

Cc Peter Chen <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

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

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode 100644
index 0000000..beb75d6

Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file +20 -0

@@ -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>;
+};

drivers/usb/chipidea/Makefile +4 -0

index cc34937..ffa2f63 100644
@@ -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

Quoted 8 lines

drivers/usb/chipidea/Makefile endif · line 22

Quoted 4 lines

drivers/usb/chipidea/Makefile +4 -0

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

Quoted 2 lines

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;

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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.

Signature · 2 lines

-- 
balbi

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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;

Quoted 3 lines

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.

Quoted 16 lines · Richard Zhao

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 31 lines · Felipe Balbi

Quoted 16 lines · Richard Zhao

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

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. :)

Quoted 4 lines

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

Quoted 38 lines · Alexander Shishkin

Quoted 35 lines · Felipe Balbi

Quoted 16 lines · Richard Zhao

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 39 lines · Richard Zhao

Quoted 38 lines · Alexander Shishkin

Quoted 35 lines · Felipe Balbi

Quoted 16 lines · Richard Zhao

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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

Quoted 51 lines · Alexander Shishkin

Quoted 39 lines · Richard Zhao

Quoted 38 lines · Alexander Shishkin

Quoted 35 lines · Felipe Balbi

Quoted 16 lines · Richard Zhao

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alexander Shishkin

Quoted 2 lines

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;

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

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

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.

Quoted 5 lines · Felipe Balbi

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.

drivers/usb/chipidea/ci13xxx_imx.c line 30

Quoted 3 lines · Richard Zhao

drivers/usb/chipidea/ci13xxx_imx.c new file +3 -0

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

This doesn't seem to be used anywhere.

Quoted 7 lines · Alexander Shishkin

drivers/usb/chipidea/ci13xxx_imx.c line 30

Quoted 3 lines · Richard Zhao

drivers/usb/chipidea/ci13xxx_imx.c new file +3 -0

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

This doesn't seem to be used anywhere.

Please disregard this one.

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

drivers/usb/chipidea/ci13xxx_imx.c line 30

Quoted 3 lines · Richard Zhao

drivers/usb/chipidea/ci13xxx_imx.c new file +3 -0

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

This doesn't seem to be used anywhere.

[snip]

drivers/usb/chipidea/ci13xxx_imx.c line 111

Quoted 1 line

drivers/usb/chipidea/ci13xxx_imx.c new file +1 -0

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

Signed-off-by Richard Zhao <[email protected]>

---
 arch/arm/mach-imx/clk-imx6q.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

arch/arm/mach-imx/clk-imx6q.c +0 -0

index 96e9edd..f99509a 100644
@@ -391,7 +391,10 @@ int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");

Dear Richard Zhao,

We will need similar patch for mx28 I think?

Otherwise

Reviewed-by Marek Vasut <[email protected]>

Quoted 11 lines

Signed-off-by Richard Zhao <[email protected]>

---
 arch/arm/mach-imx/clk-imx6q.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

arch/arm/mach-imx/clk-imx6q.c +0 -0

index 96e9edd..f99509a 100644
@@ -391,7 +391,10 @@ int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");

arch/arm/mach-imx/clk-imx6q.c int __init mx6q_clocks_init(void) · line 400

Quoted 10 lines

arch/arm/mach-imx/clk-imx6q.c +4 -1

 	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

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 <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

---
 arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
 arch/arm/mach-imx/mach-imx6q.c        |   35 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

index e0ec929..1dd2261 100644

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

 	memory {
 		reg = <0x10000000 0x40000000>;
 	};

arch/arm/mach-imx/mach-imx6q.c +1 -0

index b47e98b..577cf19 100644
@@ -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>

arch/arm/mach-imx/mach-imx6q.c +11 -0

+{
+	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);

arch/arm/mach-imx/mach-imx6q.c +12 -0

+	}
+
+	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();
 }
 

Signature · 1 line

1.7.5.4

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

Quoted 22 lines

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 <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

---

 arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
 arch/arm/mach-imx/mach-imx6q.c        |   35

+++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/arch/arm/boot/dts/imx6q-sabrelite.dts index e0ec929..1dd2261 100644

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

@@ -17,6 +17,13 @@
 

arch/arm/boot/dts/imx6q-sabrelite.dts line 27

Quoted 10 lines

arch/arm/boot/dts/imx6q-sabrelite.dts +7 -0

 	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.

Quoted 23 lines

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

 	memory {
 

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

 		reg = <0x10000000 0x40000000>;
 
 	};

(quoted diff) +0 -0

b/arch/arm/mach-imx/mach-imx6q.c index b47e98b..577cf19 100644

arch/arm/mach-imx/mach-imx6q.c +1 -0

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

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_sabrelite_init(void) · line 119

Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c +1 -0

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

Quoted 11 lines

arch/arm/mach-imx/mach-imx6q.c +11 -0

+{
+	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);

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_sabrelite_init(void) · line 138

Quoted 10 lines

arch/arm/mach-imx/mach-imx6q.c +10 -0

+	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

Quoted 18 lines

arch/arm/mach-imx/mach-imx6q.c +10 -0

+	}
+
+	of_node_put(np);
+}
+
+static void __init imx6q_post_populate(void)
+{
+	imx6q_config_on_boot();
+}
+
 

static void __init imx6q_init_machine(void)
{

arch/arm/mach-imx/mach-imx6q.c +0 -0

 	/*

@@ -126,6 +159,8 @@ static void __init imx6q_init_machine(void)
 

Quoted 8 lines

arch/arm/mach-imx/mach-imx6q.c +2 -0

 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 
+	imx6q_post_populate();
+
 
 	imx6q_pm_init();
 

}

Dear Shawn Guo,

Quoted 2 lines

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.

Quoted 5 lines

Dear Shawn Guo,

Quoted 2 lines

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

Quoted 4 lines

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

Dear Shawn Guo,

Quoted 11 lines · Marek Vasut

Quoted 9 lines

Dear Shawn Guo,

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 16 lines

Dear Shawn Guo,

Quoted 13 lines · Marek Vasut

Quoted 9 lines

Dear Shawn Guo,

Quoted 2 lines

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?

Quoted 3 lines

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

Quoted 2 lines

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

Dear Shawn Guo,

Quoted 5 lines · Marek Vasut

Quoted 2 lines

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?

Quoted 3 lines

Regards,
Shawn

Best regards,
Marek Vasut

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

Quoted 22 lines

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 <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

---
 arch/arm/boot/dts/imx6q-sabrelite.dts |    7 ++++++
 arch/arm/mach-imx/mach-imx6q.c        |   35 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

index e0ec929..1dd2261 100644
@@ -17,6 +17,13 @@
 	model = "Freescale i.MX6 Quad SABRE Lite Board";
 	compatible = "fsl,imx6q-sabrelite", "fsl,imx6q";
 

arch/arm/boot/dts/imx6q-sabrelite.dts line 26

Quoted 7 lines

arch/arm/boot/dts/imx6q-sabrelite.dts +7 -0

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

Quoted 20 lines

arch/arm/boot/dts/imx6q-sabrelite.dts +0 -0

 	memory {
 		reg = <0x10000000 0x40000000>;
 	};

arch/arm/mach-imx/mach-imx6q.c +1 -0

index b47e98b..577cf19 100644
@@ -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();
 }
 

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_sabrelite_init(void) · line 117

Quoted 1 line

arch/arm/mach-imx/mach-imx6q.c +1 -0

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

Quoted 16 lines

arch/arm/mach-imx/mach-imx6q.c +15 -0

+{
+	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;

arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_sabrelite_init(void) · line 138

Quoted 6 lines

arch/arm/mach-imx/mach-imx6q.c +6 -0

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

Quoted 26 lines

arch/arm/mach-imx/mach-imx6q.c +12 -0

+	}
+
+	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();
 }
 

Signature · 1 line

1.7.5.4

Signed-off-by Richard Zhao <[email protected]>

---
 arch/arm/mach-imx/clk-imx6q.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-imx/clk-imx6q.c +1 -1

index f99509a..0d003aa 100644
@@ -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];

base + 0x20, 0x2000, 0x3); clk[pll8_enet] =
imx_clk_pllv3(IMX_PLLV3_ENET, "pll8_enet", "osc", base + 0xe0, 0x182000,
0x3);

arch/arm/mach-imx/clk-imx6q.c +1 -0

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

Dear Richard Zhao,

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

Quoted 3 lines

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.

Dear Richard Zhao,

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

Reviewed-by Marek Vasut <[email protected]>

Quoted 17 lines

Signed-off-by Richard Zhao <[email protected]>

---
 arch/arm/mach-imx/clk-imx6q.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-imx/clk-imx6q.c +1 -1

index f99509a..0d003aa 100644
@@ -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
 };
 

arch/arm/mach-imx/clk-imx6q.c int __init mx6q_clocks_init(void) · line 197

Quoted 3 lines

arch/arm/mach-imx/clk-imx6q.c +0 -0

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

Quoted 12 lines

base + 0x20, 0x2000, 0x3); clk[pll8_enet] =
imx_clk_pllv3(IMX_PLLV3_ENET, "pll8_enet", "osc", base + 0xe0, 0x182000,
0x3);

arch/arm/mach-imx/clk-imx6q.c +1 -0

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

arch/arm/mach-imx/clk-imx6q.c int __init mx6q_clocks_init(void) · line 405

Quoted 8 lines

arch/arm/mach-imx/clk-imx6q.c +2 -0

 	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

Signed-off-by Richard Zhao <[email protected]>

---
 arch/arm/mach-imx/clk-imx6q.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-imx/clk-imx6q.c +0 -0

index f99509a..0d003aa 100644

arch/arm/mach-imx/clk-imx6q.c +5 -0

 };
 
 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");

Signature · 1 line

1.7.5.4

Quoted 5 lines · Richard Zhao

Signed-off-by Richard Zhao <[email protected]>

---
 arch/arm/mach-imx/clk-imx6q.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-imx/clk-imx6q.c enum mx6q_clks { · line 155

Quoted 10 lines

arch/arm/mach-imx/clk-imx6q.c +1 -1

index f99509a..0d003aa 100644
@@ -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.