Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core

From: Roger Quadros
Date: Wed Jun 08 2016 - 03:34:09 EST

Next message: H. Peter Anvin: "Re: [RFC PATCH] sys_read: add a compat_sys_read for 64bit system"
Previous message: Tomeu Vizoso: "[PATCH] spi: rockchip: Signal unfinished DMA transfers"
Next in thread: Peter Chen: "Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 24/05/16 05:53, Peter Chen wrote:
> On Mon, May 23, 2016 at 01:36:51PM +0300, Roger Quadros wrote:
>> On 23/05/16 13:34, Jun Li wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Roger Quadros [mailto:rogerq@xxxxxx]
>>>> Sent: Monday, May 23, 2016 6:12 PM
>>>> To: Peter Chen <hzpeterchen@xxxxxxxxx>
>>>> Cc: Jun Li <jun.li@xxxxxxx>; peter.chen@xxxxxxxxxxxxx; balbi@xxxxxxxxxx;
>>>> tony@xxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; dan.j.williams@xxxxxxxxx;
>>>> mathias.nyman@xxxxxxxxxxxxxxx; Joao.Pinto@xxxxxxxxxxxx;
>>>> sergei.shtylyov@xxxxxxxxxxxxxxxxxx; jun.li@xxxxxxxxxxxxx;
>>>> grygorii.strashko@xxxxxx; yoshihiro.shimoda.uh@xxxxxxxxxxx;
>>>> robh@xxxxxxxxxx; nsekhar@xxxxxx; b-liu@xxxxxx; linux-usb@xxxxxxxxxxxxxxx;
>>>> linux-omap@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>>> devicetree@xxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> On 23/05/16 06:21, Peter Chen wrote:
>>>>> On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote:
>>>>>> On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote:
>>>>>>> On 18/05/16 17:46, Jun Li wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>> '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?
>>>>>>
>>>>>>> 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.
>>>
>>> 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

Next message: H. Peter Anvin: "Re: [RFC PATCH] sys_read: add a compat_sys_read for 64bit system"
Previous message: Tomeu Vizoso: "[PATCH] spi: rockchip: Signal unfinished DMA transfers"
Next in thread: Peter Chen: "Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]