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

[Roger Quadros <rogerq@ti.com>]

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.

> 
> If you directly call to usb_udc_vbus_handler() in drd state machine
> 
> Then:
> 
> Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of each other)

It's not so easy. We are again creating a circular dependency and all
build configurations don't work like I pointed above.

The only optimization I could do is make CONFIG_OTG tristate and
allow it to be built as 'm' if both hcd and gadget are 'm'.
i.e. case (2).

cheers,
-roger

>>
>> Do you have any ideas to solve that case?
>>
>> cheers,
>> -roger
>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>> @@ -660,9 +830,15 @@ static ssize_t
>>>>>>>>>>>> usb_udc_softconn_store(struct
>>>>>>>> device *dev,
>>>>>>>>>>>>  		return -EOPNOTSUPP;
>>>>>>>>>>>>  	}
>>>>>>>>>>>>
>>>>>>>>>>>> +	/* In OTG mode we don't support softconnect, but
>> b_bus_req */
>>>>>>>>>>>> +	if (udc->gadget->otg_dev) {
>>>>>>>>>>>> +		dev_err(dev, "soft-connect not supported in OTG
>> mode\n");
>>>>>>>>>>>> +		return -EOPNOTSUPP;
>>>>>>>>>>>> +	}
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> The soft-connect can be supported at dual-role mode currently,
>>>>>>>>>>> we can use b_bus_req entry once it is implemented later.
>>>>>>>>>>
>>>>>>>>>> Soft-connect should be done via sysfs handling within the OTG
>> core.
>>>>>>>>>> This can be added later. I don't want anything outside the OTG
>>>>>>>>>> core to handle soft-connect behaviour as it will be hard to
>>>>>>>>>> keep things in sync.
>>>>>>>>>>
>>>>>>>>>> I can update the comment to something like this.
>>>>>>>>>>
>>>>>>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG
>>>>>>>>>> core */
>>>>>>>>>
>>>>>>>>> Ok, let's Felipe decide it.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>  	if (sysfs_streq(buf, "connect")) {
>>>>>>>>>>>>  		usb_gadget_udc_start(udc);
>>>>>>>>>>>> -		usb_gadget_connect(udc->gadget);
>>>>>>>>>>>> +		usb_udc_connect_control(udc);
>>>>>>>>>>>
>>>>>>>>>>> This line seems to be not related with this patch.
>>>>>>>>>>>
>>>>>>>>>> Right. I'll remove it.
>>>>>>>>>>
>>>>>>>>>> cheers,
>>>>>>>>>> -roger
>>>>>>>>>