Re: [RFC][PATCH v2 00/13] USB: OTG/DRD Core functionality

[Roger Quadros <rogerq@ti.com>]

On 22/04/15 05:17, Peter Chen wrote:
> On Tue, Apr 21, 2015 at 10:34:01AM +0300, Roger Quadros wrote:
>> On 21/04/15 09:04, Peter Chen wrote:
>>>  
>>>>
>>>> On 20/04/15 06:05, Peter Chen wrote:
>>>>> On Tue, Apr 14, 2015 at 01:41:47PM +0300, Roger Quadros wrote:
>>>>>> This is an attempt to centralize OTG/Dual-role functionality in the kernel.
>>>>>> As of now I've got Dual-role functionality working pretty reliably on
>>>>>> dra7-evm. xhci side of things for OTG/DRD use are fixed in
>>>>>> http://thread.gmane.org/gmane.linux.kernel/1923161
>>>>>
>>>>> Hi Roger,
>>>>>
>>>>> Currently, there are two main problems for DRD/OTG framework.
>>>>>
>>>>> - For multi-platform supports, we may define CONFIG_USB_OTG, but the
>>>>> gadget should not add its otg descriptor to its configuration
>>>>> descriptors if it does not support one of otg features (SRP/HNP/ADP).
>>>>> Macpaul Lin's patch set [1] is the right way to do it.
>>>>
>>>> Agreed. That check (whether OTG descriptor can be added and which version
>>>> of it) has to be done at runtime and it must be added only if hardware supports
>>>> OTG _and_ kernel OTG support is enabled.
>>>>
>>>
>>> Ok, let's put this patch set in mainline first, since your patch set may need some
>>> information from it.
>>>  
>>>>> - We are lack of framework to handle OTG (DRD) switch, it is great you
>>>>> are designing it. The main problem for this framework is how to handle
>>>>> DRD/OTG FSM unify. My thought is we add two paths for them separate.
>>>>> For easy, I suggest if the platform supports one of otg features, then
>>>>> it goes to fully otg fsm, else it goes to simply otg fsm (like your
>>>>> drd fsm). If you agree with it too, you may not need to add another
>>>> "dr_mode"
>>>>> value.
>>>>
>>>> It would be nice that way but unfortunately it does't work in all cases.
>>>> e.g. What if the SoC itself supports all OTG features but the board is not
>>>> designed for OTG. Or the product designer simply is not interested in full OTG
>>>> support but just dual-role. So we need some flexibility for the device
>>>> tree/platform-data to specify that. This is where a new "dr_mode" == "dual-
>>>> role" is needed.
>>>>
>>>
>>> Since "dr_mode" has been widely used now, if we add a new property for it,  we
>>> need to change all drivers.
>>>
>>> Your OTG/DRD framework needs to (partial) use otg fsm, and we will not teach old
>>> driver to use it since there are some driver related stuffs.
>>
>> fair enough. Let's not change dr_mode then and decide based on other parameters.
>>
>>>
>>> SRP/HNP/ADP support can be board level capabilities, and we may consider the
>>> otg device which does not support otg fsm (hardware finishes fsm). So I suggest
>>> we have below properties at dts:
>>>
>>> - otg-support /* fully otg support */
>>> - otg-fsm-support /* fully otg fsm support */
>>
>> what is the difference between otg-support and otg-fsm-support?
> 
> Like I mentioned at above, the hardware finishes HNP/SRP which does not
> use otg fsm code (usb-otg-fsm.c), most of legacy otg platforms (musb?)
> use this way, for these platforms, only need to set otg-support = 1

So dr_mode = "otg" _and_ otg-support = 1?

Again wouldn't this involve changes to dts for musb like platforms
supporting full OTG?

Instead we could add a new field saying otg-fsm-type
"otg-hw", "otg-sw", "drd-sw"

If the field is absent it defaults to "otg-hw".
This also means we don't need otg-fsm-support flag.

Now the pseudo-code to decide fsm is

if (dr_mode == "otg" && CONFIG_USB_OTG)
	if (otg-fsm-type == "otg-sw") {
		if (CONFIG_USB_OTG_FSM)
			full otg fsm support via sw
		else
			error "CONFIG_USB_OTG_FSM" not set
	} else if (otg-fsm-type == "drd-sw") {
		dual role fsm support
	} else {
		full otg support via hw
	}

	if (otg-fsm-type == "otg
else
	error "CONFIG_USB_OTG" not set

> 
> For platforms which software finishes HNP/SRP using otg fsm code, need
> to set both flags.
> 
> For platforms which only do role switch through id pin, do not need to
> set both.
> 

OK. I get it now.

>>
>>> - otg-ver /* eh & otg supplement version */
>>
>> we can get otg version from the OTG controller. What exactly is the
>> otg-ver in dts for?
> 
> Since most of otg stuffs are software's, eg, for otg descriptor, we will
> only use otg 2.0 descriptor when both CONFIG_USB_OTG20 and otg-ver = 20
> are set.

CONFIG_USB_OTG20 is redundant now. Plus I mentioned in the respective thread
that it is not suitable for booting single image on different platforms.

As of now I can see 2 inputs regarding otg-ver
- One comes from otg-ver DT property or platform data.
- Second that may come from OTG controller registers. e.g. It might support
OTG v3.0 but system designer wants to limit to OTG v2.0 via otg-ver.

Controller driver can decide among the 2 and set the appropriate otg version
in the data structure.

> 
>>
>>> - adp-support /* board adp support */ 
>>> - srp-support /* board srp support */
>>> - hnp-support /* board hnp support */
>>
>> So if these options are not provided in DTS but the OTG core supports them then
>> we keep the respective feature disabled?
> 
> Yes.
> 
>> Won't this need dts change for existing boards?
> 
> Does you know any dts based platform supports hnp/srp?

I'm the wrong person to ask this. Maybe Felipe/Tony can comment.
Irrespective of whether any dts platforms supports hsn/srp or not
we must assume that till date dr_mode = otg implies full otg support
and we cannot change its meaning.

We can add new fields to indicate dual-role mode which is a new feature.

> For chipidea platform, currently, we depends on kernel
> configurations (CONFIG_USB_OTG_FSM), but it is incorrect way.
> 
>>
>> Instead how about having disable flags instead.
>>  - adp-disable	/* board doesn't support adp */
>>  - srp-disable	/* board doesn't support srp */
>>  - hnp-disable	/* board doesn't support hnp */
>>
>> Now, if the flags are not provided in dts we use the OTG core's flags.
>>
> 
> How the OTG core's know if it supports these?

By OTG core, I meant OTG controller core. At least the dwc3 OTG controller has
register bits to identify if it supports adp/srp/hnp.

But we also need to keep in mind that adp feature can be provided separately even
if the OTG controller core doesn't support it.

cheers,
-roger

> 
>>>
>>> Currently, if CONFIG_USB_OTG and CONFIG_USB_OTG_FSM are enabled, we will
>>> have otg fsm code (usb-otg-fsm.c).
>>>
>>> if (otg-support & otg-fsm-support)
>>> 	this device has fully otg support, and will follow full otg fsm transitions. 
>>> else
>>> 	this device is drd, and will follow simple otg fsm transtions. 
>>>
>>
>> cheers,
>> -roger
>>
>