Hi,

On 24/05/16 05:53, Peter Chen wrote:

On Mon, May 23, 2016 at 01:36:51PM +0300, Roger Quadros wrote:

quoted

On 23/05/16 13:34, Jun Li wrote:

quoted

Hi

quoted

-----Original Message-----
From: Roger Quadros [mailto:rogerq@ti.com]
Sent: Monday, May 23, 2016 6:12 PM
To: Peter Chen <redacted>
Cc: Jun Li <redacted>; peter.chen@freescale.com; balbi@kernel.org;
tony@atomide.com; gregkh@linuxfoundation.org; dan.j.williams@intel.com;
mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com;
sergei.shtylyov@cogentembedded.com; jun.li@freescale.com;
grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com;
robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org;
linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org
Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core

On 23/05/16 06:21, Peter Chen wrote:

quoted

On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote:

quoted

On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote:

quoted

On 18/05/16 17:46, Jun Li wrote:

quoted

quoted

quoted

quoted

I didn't want to have complex Kconfig so decided to have otg as
built-in only.
What do you want me to change in existing code? and why?

Remove those stuff which only for pass diff driver config Like
every controller driver need a duplicated

static struct otg_hcd_ops ci_hcd_ops = {
    ...
}

This is an exception only. Every controller driver doesn't need to
implement hcd_ops. It is implemented in the hcd core.

quoted

And here is another example, for gadget connect, otg driver can
directly call to usb_udc_vbus_handler() in drd state machine, but
you create another interface:

.connect_control = usb_gadget_connect_control,

If the symbol is defined in one driver which is 'm', another
driver reference it should be 'm' as well, then there is no this
kind of problem as my understanding.

That is fine as long as all are 'm'. but how do you solve the case
when Gadget is built in and host is 'm'? OTG has to be built-in
and you will need an hcd to gadget interface.

Hcd to gadget interface? Or you want to say otg to host interface?

Sorry, I meant to say host to otg interface.

quoted

I think hcd and gadget are independent each other, now

Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol)

It is actually a circular dependency for both.
 hcd <--> otg and gadget <--> otg

hcd -> otg for usb_otg_register/unregister_hcd otg -> hcd for
usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg,
usb_hub_find_child

gadget -> otg for usb_otg_register/unregister_gadget
otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler

Now consider what will happen if I get rid of the otg_hcd and

otg_gadget interfaces.

quoted

quoted

quoted

'y' means built-in, 'm' means module.

1) hcd 'y', gadget 'y'
otg has to be 'y' for proper build.

2) hcd 'm', gadget 'm'
otg has to be 'm' for proper build.

3) hcd 'y', gadget 'm'
Build will fail always.
If otg is 'y', otg build will fail due to dependency on gadget.
If otg is 'm', hcd build will fail due to dependency on otg.

4) hcd 'm', gadget 'y'
Build will fail always.
If otg is 'y', otg build will fail due to dependency on hcd.
If otg is 'm', gadget build will fails due to dependency on otg.

So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops
to remove otg->hcd and otg->gadget dependency.

Now we can address 3) and 4) like so

3) hcd 'y', gadget 'm'
otg has to be 'y' for proper build.

4) hcd 'm', gadget 'y'
otg has to be 'y' for proper build.

How about this:
Moving usb_otg_register/unregister_hcd to host driver to remove
dependency hcd->otg. And moving usb_otg_get_data to common.c.

Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data
returns NULL, the host/device driver's probe return -EPROBE_DEFER.
When the otg driver is probed successfully, the host/device will be
re-probed again, and usb_otg_register_hcd will be called again.

And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and
otg_gadget_ops. Below build dependency issues can be fixed.
What do you think?

quoted

1) hcd 'y', gadget 'y'
otg has to be 'y' for proper build.

2) hcd 'm', gadget 'm'
otg has to be 'm' for proper build.

3) hcd 'y', gadget 'm'
Build will fail always.
If otg is 'y', otg build will fail due to dependency on gadget.
If otg is 'm', hcd build will fail due to dependency on otg.

4) hcd 'm', gadget 'y'
Build will fail always.
If otg is 'y', otg build will fail due to dependency on hcd.
If otg is 'm', gadget build will fails due to dependency on otg.

--

After thinking more, my suggestion can't work. How about moving
CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG can
only be built in.

I tried this but it still doesn't resolve 3 and 4. I.e.
it can't be automatically set to 'y' when either of hcd/gadget is 'y'
and the other is 'm'.

USB_OTG only can be selected by *user*, can't be automatically set to
'y' like you said, no matter what's hcd/gadget config, USB_OTG can be
disabled in any cases.

Yes. I am on the same page with that.

quoted

I think Peter's intention is to make USB_OTG to be 'bool', either disabled;
or built-in(if enabled), something like your original idea?

I'm fine with my original idea if you guys can confirm :).

Please do it.

OK. I managed to fix this and get usbotg to be as module when both USB and GADGET
are modules with the following patch.

What we do is use USB_OTG_CORE as a hidden tristate symbol that is used to build usbotg.o
only if USB_OTG is enabled.

USB_OTG is used to enable/disable OTG and it has to be bool since it is used at mutiple
places in the code.

cheers,
-roger

---

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8689dcb..ed596ec 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig

@@ -32,6 +32,23 @@ if USB_SUPPORT
 config USB_COMMON
 	tristate
 
+config USB_OTG_CORE
+	tristate
+
+config USB_OTG
+	bool "OTG/Dual-role support"
+	depends on PM && USB && USB_GADGET
+	default n
+	---help---
+	  The most notable feature of USB OTG is support for a
+	  "Dual-Role" device, which can act as either a device
+	  or a host. The initial role is decided by the type of
+	  plug inserted and can be changed later when two dual
+	  role devices talk to each other.
+
+	  Select this only if your board has Mini-AB/Micro-AB
+	  connector.
+
 config USB_ARCH_HAS_HCD
 	def_bool y
 

@@ -40,6 +57,7 @@ config USB
 	tristate "Support for Host-side USB"
 	depends on USB_ARCH_HAS_HCD
 	select USB_COMMON
+	select USB_OTG_CORE
 	select NLS  # for UTF-8 strings
 	---help---
 	  Universal Serial Bus (USB) is a specification for a serial bus

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 16a5b55..03f7204 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile

@@ -59,6 +59,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs/
 obj-$(CONFIG_USB_GADGET)	+= gadget/
 
 obj-$(CONFIG_USB_COMMON)	+= common/
-obj-$(CONFIG_USB_OTG)		+= common/
+obj-$(CONFIG_USB_OTG_CORE)	+= common/
 
 obj-$(CONFIG_USBIP_CORE)	+= usbip/

diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index 730d928..5122b3f 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile

@@ -7,5 +7,7 @@ usb-common-y			  += common.o
 usb-common-$(CONFIG_USB_LED_TRIG) += led.o
 
 obj-$(CONFIG_USB_ULPI_BUS)	+= ulpi.o
+ifeq ($(CONFIG_USB_OTG),y)
 usbotg-y		:= usb-otg.o usb-otg-fsm.o
-obj-$(CONFIG_USB_OTG)	+= usbotg.o
+obj-$(CONFIG_USB_OTG_CORE)	+= usbotg.o
+endif

diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index f5eb7f5..98fcfa2 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c

@@ -16,6 +16,7 @@
 
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/usb/of.h>

@@ -841,3 +842,5 @@ int usb_otg_gadget_ready(struct usb_gadget *gadget, bool ready)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_otg_gadget_ready);
+
+MODULE_LICENSE("GPL");

diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index de371e6..37f8c54 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig

@@ -41,20 +41,6 @@ config USB_DYNAMIC_MINORS
 
 	  If you are unsure about this, say N here.
 
-config USB_OTG
-	bool "OTG/Dual-role support"
-	depends on PM && USB_GADGET
-	default n
-	help
-	  The most notable feature of USB OTG is support for a
-	  "Dual-Role" device, which can act as either a device
-	  or a host. The initial role is decided by the type of
-	  plug inserted and can be changed later when two dual
-	  role devices talk to each other.
-
-	  Select this only if your board has Mini-AB/Micro-AB
-	  connector.
-
 config USB_OTG_WHITELIST
 	bool "Rely on OTG and EH Targeted Peripherals List"
 	depends on USB

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 2057add..9d55384 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig

@@ -16,6 +16,7 @@
 menuconfig USB_GADGET
 	tristate "USB Gadget Support"
 	select USB_COMMON
+	select USB_OTG_CORE
 	select NLS
 	help
 	   USB is a master/slave protocol, organized with one master

-- 
2.7.4