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

Commit Message

Roger Quadros May 13, 2016, 10:03 a.m. UTC

The OTG state machine needs a mechanism to start and
stop the gadget controller as well as connect/disconnect
from the bus. Add usb_gadget_start(), usb_gadget_stop()
and usb_gadget_connect_control().

Introduce usb_otg_add_gadget_udc() to allow controller drivers
to register a gadget controller that is part of an OTG instance.

Register with OTG core when gadget function driver
is available and unregister when function driver is unbound.

We need to unlock the usb_lock mutex before calling
usb_otg_register_gadget() in udc_bind_to_driver() and
usb_gadget_remove_driver() else it will cause a circular
locking dependency.

Ignore softconnect sysfs control when we're in OTG
mode as OTG FSM takes care of gadget softconnect using
the b_bus_req mechanism.

Signed-off-by: Roger Quadros <[e-mail]>
---
 drivers/usb/gadget/udc/udc-core.c | 194 ++++++++++++++++++++++++++++++++++++--
 include/linux/usb/gadget.h        |   4 +
 2 files changed, 189 insertions(+), 9 deletions(-)

Comments

Peter Chen May 16, 2016, 7:02 a.m. UTC | #1

On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> +
> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
> +{
> +	struct usb_udc *udc;
> +
> +	mutex_lock(&udc_lock);
> +	udc = usb_gadget_to_udc(gadget);
> +	if (!udc) {
> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> +			__func__);
> +		mutex_unlock(&udc_lock);
> +		return -EINVAL;
> +	}
> +
> +	if (connect) {
> +		if (!gadget->connected)
> +			usb_gadget_connect(udc->gadget);
> +	} else {
> +		if (gadget->connected) {
> +			usb_gadget_disconnect(udc->gadget);
> +			udc->driver->disconnect(udc->gadget);
> +		}
> +	}
> +
> +	mutex_unlock(&udc_lock);
> +
> +	return 0;
> +}
> +

Since this is called for vbus interrupt, why not using
usb_udc_vbus_handler directly, and call udc->driver->disconnect
at usb_gadget_stop.

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

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

Roger Quadros May 16, 2016, 8:26 a.m. UTC | #2

Hi,

On 16/05/16 10:02, Peter Chen wrote:
> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>> +
>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
>> +{
>> +	struct usb_udc *udc;
>> +
>> +	mutex_lock(&udc_lock);
>> +	udc = usb_gadget_to_udc(gadget);
>> +	if (!udc) {
>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +			__func__);
>> +		mutex_unlock(&udc_lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (connect) {
>> +		if (!gadget->connected)
>> +			usb_gadget_connect(udc->gadget);
>> +	} else {
>> +		if (gadget->connected) {
>> +			usb_gadget_disconnect(udc->gadget);
>> +			udc->driver->disconnect(udc->gadget);
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&udc_lock);
>> +
>> +	return 0;
>> +}
>> +
> 
> Since this is called for vbus interrupt, why not using
> usb_udc_vbus_handler directly, and call udc->driver->disconnect
> at usb_gadget_stop.

We can't assume that this is always called for vbus interrupt so
I decided not to call usb_udc_vbus_handler.

udc->vbus is really pointless for us. We keep vbus states in our
state machine and leave udc->vbus as ture always.

Why do you want to move udc->driver->disconnect() to stop?
If USB controller disconnected from bus then the gadget driver
must be notified about the disconnect immediately. The controller
may or may not be stopped by the core.

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

> 
>>  	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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen May 16, 2016, 9:23 a.m. UTC | #3

On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> Hi,
> 
> On 16/05/16 10:02, Peter Chen wrote:
> > On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >> +
> >> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
> >> +{
> >> +	struct usb_udc *udc;
> >> +
> >> +	mutex_lock(&udc_lock);
> >> +	udc = usb_gadget_to_udc(gadget);
> >> +	if (!udc) {
> >> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >> +			__func__);
> >> +		mutex_unlock(&udc_lock);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (connect) {
> >> +		if (!gadget->connected)
> >> +			usb_gadget_connect(udc->gadget);
> >> +	} else {
> >> +		if (gadget->connected) {
> >> +			usb_gadget_disconnect(udc->gadget);
> >> +			udc->driver->disconnect(udc->gadget);
> >> +		}
> >> +	}
> >> +
> >> +	mutex_unlock(&udc_lock);
> >> +
> >> +	return 0;
> >> +}
> >> +
> > 
> > Since this is called for vbus interrupt, why not using
> > usb_udc_vbus_handler directly, and call udc->driver->disconnect
> > at usb_gadget_stop.
> 
> We can't assume that this is always called for vbus interrupt so
> I decided not to call usb_udc_vbus_handler.
> 
> udc->vbus is really pointless for us. We keep vbus states in our
> state machine and leave udc->vbus as ture always.
> 
> Why do you want to move udc->driver->disconnect() to stop?
> If USB controller disconnected from bus then the gadget driver
> must be notified about the disconnect immediately. The controller
> may or may not be stopped by the core.
> 

Then, would you give some comments when this API will be used?
I was assumed it is only used for drd state machine.

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

Roger Quadros May 16, 2016, 9:51 a.m. UTC | #4

On 16/05/16 12:23, Peter Chen wrote:
> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>> Hi,
>>
>> On 16/05/16 10:02, Peter Chen wrote:
>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>> +
>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
>>>> +{
>>>> +	struct usb_udc *udc;
>>>> +
>>>> +	mutex_lock(&udc_lock);
>>>> +	udc = usb_gadget_to_udc(gadget);
>>>> +	if (!udc) {
>>>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>> +			__func__);
>>>> +		mutex_unlock(&udc_lock);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (connect) {
>>>> +		if (!gadget->connected)
>>>> +			usb_gadget_connect(udc->gadget);
>>>> +	} else {
>>>> +		if (gadget->connected) {
>>>> +			usb_gadget_disconnect(udc->gadget);
>>>> +			udc->driver->disconnect(udc->gadget);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	mutex_unlock(&udc_lock);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>
>>> Since this is called for vbus interrupt, why not using
>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
>>> at usb_gadget_stop.
>>
>> We can't assume that this is always called for vbus interrupt so
>> I decided not to call usb_udc_vbus_handler.
>>
>> udc->vbus is really pointless for us. We keep vbus states in our
>> state machine and leave udc->vbus as ture always.
>>
>> Why do you want to move udc->driver->disconnect() to stop?
>> If USB controller disconnected from bus then the gadget driver
>> must be notified about the disconnect immediately. The controller
>> may or may not be stopped by the core.
>>
> 
> Then, would you give some comments when this API will be used?
> I was assumed it is only used for drd state machine.

drd_state machine didn't even need this API in the first place :).
You guys wanted me to separate out start/stop and connect/disconnect for full OTG case.
Won't full OTG state machine want to use this API? If not what would it use?

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jun Li May 17, 2016, 7:38 a.m. UTC | #5

Hi

> -----Original Message-----
> From: Roger Quadros [mailto:[e-mail]]
> Sent: Monday, May 16, 2016 5:52 PM
> To: Peter Chen <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> 
> On 16/05/16 12:23, Peter Chen wrote:
> > On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> >> Hi,
> >>
> >> On 16/05/16 10:02, Peter Chen wrote:
> >>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >>>> +
> >>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget,
> >>>> +bool connect) {
> >>>> +	struct usb_udc *udc;
> >>>> +
> >>>> +	mutex_lock(&udc_lock);
> >>>> +	udc = usb_gadget_to_udc(gadget);
> >>>> +	if (!udc) {
> >>>> +		dev_err(gadget->dev.parent, "%s: gadget not
> registered.\n",
> >>>> +			__func__);
> >>>> +		mutex_unlock(&udc_lock);
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	if (connect) {
> >>>> +		if (!gadget->connected)
> >>>> +			usb_gadget_connect(udc->gadget);
> >>>> +	} else {
> >>>> +		if (gadget->connected) {
> >>>> +			usb_gadget_disconnect(udc->gadget);
> >>>> +			udc->driver->disconnect(udc->gadget);
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	mutex_unlock(&udc_lock);
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>
> >>> Since this is called for vbus interrupt, why not using
> >>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at
> >>> usb_gadget_stop.
> >>
> >> We can't assume that this is always called for vbus interrupt so I
> >> decided not to call usb_udc_vbus_handler.
> >>
> >> udc->vbus is really pointless for us. We keep vbus states in our
> >> state machine and leave udc->vbus as ture always.
> >>
> >> Why do you want to move udc->driver->disconnect() to stop?
> >> If USB controller disconnected from bus then the gadget driver must
> >> be notified about the disconnect immediately. The controller may or
> >> may not be stopped by the core.
> >>
> >
> > Then, would you give some comments when this API will be used?
> > I was assumed it is only used for drd state machine.
> 
> drd_state machine didn't even need this API in the first place :).
> You guys wanted me to separate out start/stop and connect/disconnect for
> full OTG case.
> Won't full OTG state machine want to use this API? If not what would it
> use?

Instead create those new interfaces/symbol here and there just aim to
address build problems in diff configures, Could we only allow meaningful
combination of those 3 drivers configures?

Hcd=y, gadget=y, otg=y or
Hcd=m, gadget=m, otg=m

Li Jun

> 
> 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
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roger Quadros May 17, 2016, 8:08 a.m. UTC | #6

On 17/05/16 10:38, Jun Li wrote:
> Hi
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:[e-mail]]
>> Sent: Monday, May 16, 2016 5:52 PM
>> To: Peter Chen <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 16/05/16 12:23, Peter Chen wrote:
>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 16/05/16 10:02, Peter Chen wrote:
>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>>>> +
>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget,
>>>>>> +bool connect) {
>>>>>> +	struct usb_udc *udc;
>>>>>> +
>>>>>> +	mutex_lock(&udc_lock);
>>>>>> +	udc = usb_gadget_to_udc(gadget);
>>>>>> +	if (!udc) {
>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not
>> registered.\n",
>>>>>> +			__func__);
>>>>>> +		mutex_unlock(&udc_lock);
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (connect) {
>>>>>> +		if (!gadget->connected)
>>>>>> +			usb_gadget_connect(udc->gadget);
>>>>>> +	} else {
>>>>>> +		if (gadget->connected) {
>>>>>> +			usb_gadget_disconnect(udc->gadget);
>>>>>> +			udc->driver->disconnect(udc->gadget);
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	mutex_unlock(&udc_lock);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Since this is called for vbus interrupt, why not using
>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at
>>>>> usb_gadget_stop.
>>>>
>>>> We can't assume that this is always called for vbus interrupt so I
>>>> decided not to call usb_udc_vbus_handler.
>>>>
>>>> udc->vbus is really pointless for us. We keep vbus states in our
>>>> state machine and leave udc->vbus as ture always.
>>>>
>>>> Why do you want to move udc->driver->disconnect() to stop?
>>>> If USB controller disconnected from bus then the gadget driver must
>>>> be notified about the disconnect immediately. The controller may or
>>>> may not be stopped by the core.
>>>>
>>>
>>> Then, would you give some comments when this API will be used?
>>> I was assumed it is only used for drd state machine.
>>
>> drd_state machine didn't even need this API in the first place :).
>> You guys wanted me to separate out start/stop and connect/disconnect for
>> full OTG case.
>> Won't full OTG state machine want to use this API? If not what would it
>> use?
> 
> Instead create those new interfaces/symbol here and there just aim to
> address build problems in diff configures, Could we only allow meaningful
> combination of those 3 drivers configures?
> 
> Hcd=y, gadget=y, otg=y or
> Hcd=m, gadget=m, otg=m

This is still a limitation.

It is perfectly fine to have
hcd=m, gadget=y
or
hcd=y, gadget=m

cheers,
-roger

> 
> Li Jun
> 
>>
>> 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
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jun Li May 17, 2016, 8:28 a.m. UTC | #7

Hi Roger,

> -----Original Message-----
> From: Roger Quadros [mailto:[e-mail]]
> Sent: Tuesday, May 17, 2016 4:09 PM
> To: Jun Li <[e-mail]>; Peter Chen <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> 
> On 17/05/16 10:38, Jun Li wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Roger Quadros [mailto:[e-mail]]
> >> Sent: Monday, May 16, 2016 5:52 PM
> >> To: Peter Chen <[e-mail]>
> >> Cc: [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> On 16/05/16 12:23, Peter Chen wrote:
> >>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> >>>> Hi,
> >>>>
> >>>> On 16/05/16 10:02, Peter Chen wrote:
> >>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >>>>>> +
> >>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget,
> >>>>>> +bool connect) {
> >>>>>> +	struct usb_udc *udc;
> >>>>>> +
> >>>>>> +	mutex_lock(&udc_lock);
> >>>>>> +	udc = usb_gadget_to_udc(gadget);
> >>>>>> +	if (!udc) {
> >>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not
> >> registered.\n",
> >>>>>> +			__func__);
> >>>>>> +		mutex_unlock(&udc_lock);
> >>>>>> +		return -EINVAL;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	if (connect) {
> >>>>>> +		if (!gadget->connected)
> >>>>>> +			usb_gadget_connect(udc->gadget);
> >>>>>> +	} else {
> >>>>>> +		if (gadget->connected) {
> >>>>>> +			usb_gadget_disconnect(udc->gadget);
> >>>>>> +			udc->driver->disconnect(udc->gadget);
> >>>>>> +		}
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	mutex_unlock(&udc_lock);
> >>>>>> +
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>
> >>>>> Since this is called for vbus interrupt, why not using
> >>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at
> >>>>> usb_gadget_stop.
> >>>>
> >>>> We can't assume that this is always called for vbus interrupt so I
> >>>> decided not to call usb_udc_vbus_handler.
> >>>>
> >>>> udc->vbus is really pointless for us. We keep vbus states in our
> >>>> state machine and leave udc->vbus as ture always.
> >>>>
> >>>> Why do you want to move udc->driver->disconnect() to stop?
> >>>> If USB controller disconnected from bus then the gadget driver must
> >>>> be notified about the disconnect immediately. The controller may or
> >>>> may not be stopped by the core.
> >>>>
> >>>
> >>> Then, would you give some comments when this API will be used?
> >>> I was assumed it is only used for drd state machine.
> >>
> >> drd_state machine didn't even need this API in the first place :).
> >> You guys wanted me to separate out start/stop and connect/disconnect
> >> for full OTG case.
> >> Won't full OTG state machine want to use this API? If not what would
> >> it use?
> >
> > Instead create those new interfaces/symbol here and there just aim to
> > address build problems in diff configures, Could we only allow
> > meaningful combination of those 3 drivers configures?
> >
> > Hcd=y, gadget=y, otg=y or
> > Hcd=m, gadget=m, otg=m
> 
> This is still a limitation.
> 
> It is perfectly fine to have
> hcd=m, gadget=y
> or
> hcd=y, gadget=m

I agree it makes sense to have above configs in non-otg case, that is,
the 'y' driver can work without 'm' driver loaded.

But,
in otg enabled(y/m) case, the otherwise config of my list can't make
any sense from my point view. That is: some driver is built-in, but
it can't work at all if another 'm' driver is not loaded,

in another words, the otg driver has to be 'm' if its dependent driver
is 'm', correct?    

Li Jun

> 
> cheers,
> -roger
> 
> >
> > Li Jun
> >
> >>
> >> 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
> >>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen May 18, 2016, 3:18 a.m. UTC | #8

On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote:
> On 16/05/16 12:23, Peter Chen wrote:
> > On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> >> Hi,
> >>
> >> On 16/05/16 10:02, Peter Chen wrote:
> >>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >>>> +
> >>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
> >>>> +{
> >>>> +	struct usb_udc *udc;
> >>>> +
> >>>> +	mutex_lock(&udc_lock);
> >>>> +	udc = usb_gadget_to_udc(gadget);
> >>>> +	if (!udc) {
> >>>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >>>> +			__func__);
> >>>> +		mutex_unlock(&udc_lock);
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	if (connect) {
> >>>> +		if (!gadget->connected)
> >>>> +			usb_gadget_connect(udc->gadget);
> >>>> +	} else {
> >>>> +		if (gadget->connected) {
> >>>> +			usb_gadget_disconnect(udc->gadget);
> >>>> +			udc->driver->disconnect(udc->gadget);
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	mutex_unlock(&udc_lock);
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>
> >>> Since this is called for vbus interrupt, why not using
> >>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
> >>> at usb_gadget_stop.
> >>
> >> We can't assume that this is always called for vbus interrupt so
> >> I decided not to call usb_udc_vbus_handler.
> >>
> >> udc->vbus is really pointless for us. We keep vbus states in our
> >> state machine and leave udc->vbus as ture always.
> >>
> >> Why do you want to move udc->driver->disconnect() to stop?
> >> If USB controller disconnected from bus then the gadget driver
> >> must be notified about the disconnect immediately. The controller
> >> may or may not be stopped by the core.
> >>
> > 
> > Then, would you give some comments when this API will be used?
> > I was assumed it is only used for drd state machine.
> 
> drd_state machine didn't even need this API in the first place :).
> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case.
> Won't full OTG state machine want to use this API? If not what would it use?
> 

Oh, I meant only drd and fully otg state machine needs it. I am
wondering if we need have a new API to do it. Two questions:

- Except for vbus interrupt, any chances this API will be used at
current logic?
- When this API is called but without a coming gadget->stop?

Roger Quadros May 18, 2016, 12:42 p.m. UTC | #9

On 17/05/16 11:28, Jun Li wrote:
> Hi Roger,
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:[e-mail]]
>> Sent: Tuesday, May 17, 2016 4:09 PM
>> To: Jun Li <[e-mail]>; Peter Chen <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 10:38, Jun Li wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Roger Quadros [mailto:[e-mail]]
>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>> To: Peter Chen <[e-mail]>
>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> On 16/05/16 12:23, Peter Chen wrote:
>>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 16/05/16 10:02, Peter Chen wrote:
>>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>>>>>> +
>>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget,
>>>>>>>> +bool connect) {
>>>>>>>> +	struct usb_udc *udc;
>>>>>>>> +
>>>>>>>> +	mutex_lock(&udc_lock);
>>>>>>>> +	udc = usb_gadget_to_udc(gadget);
>>>>>>>> +	if (!udc) {
>>>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not
>>>> registered.\n",
>>>>>>>> +			__func__);
>>>>>>>> +		mutex_unlock(&udc_lock);
>>>>>>>> +		return -EINVAL;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	if (connect) {
>>>>>>>> +		if (!gadget->connected)
>>>>>>>> +			usb_gadget_connect(udc->gadget);
>>>>>>>> +	} else {
>>>>>>>> +		if (gadget->connected) {
>>>>>>>> +			usb_gadget_disconnect(udc->gadget);
>>>>>>>> +			udc->driver->disconnect(udc->gadget);
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	mutex_unlock(&udc_lock);
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>
>>>>>>> Since this is called for vbus interrupt, why not using
>>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at
>>>>>>> usb_gadget_stop.
>>>>>>
>>>>>> We can't assume that this is always called for vbus interrupt so I
>>>>>> decided not to call usb_udc_vbus_handler.
>>>>>>
>>>>>> udc->vbus is really pointless for us. We keep vbus states in our
>>>>>> state machine and leave udc->vbus as ture always.
>>>>>>
>>>>>> Why do you want to move udc->driver->disconnect() to stop?
>>>>>> If USB controller disconnected from bus then the gadget driver must
>>>>>> be notified about the disconnect immediately. The controller may or
>>>>>> may not be stopped by the core.
>>>>>>
>>>>>
>>>>> Then, would you give some comments when this API will be used?
>>>>> I was assumed it is only used for drd state machine.
>>>>
>>>> drd_state machine didn't even need this API in the first place :).
>>>> You guys wanted me to separate out start/stop and connect/disconnect
>>>> for full OTG case.
>>>> Won't full OTG state machine want to use this API? If not what would
>>>> it use?
>>>
>>> Instead create those new interfaces/symbol here and there just aim to
>>> address build problems in diff configures, Could we only allow
>>> meaningful combination of those 3 drivers configures?
>>>
>>> Hcd=y, gadget=y, otg=y or
>>> Hcd=m, gadget=m, otg=m
>>
>> This is still a limitation.
>>
>> It is perfectly fine to have
>> hcd=m, gadget=y
>> or
>> hcd=y, gadget=m
> 
> I agree it makes sense to have above configs in non-otg case, that is,
> the 'y' driver can work without 'm' driver loaded.
> 
> But,
> in otg enabled(y/m) case, the otherwise config of my list can't make
> any sense from my point view. That is: some driver is built-in, but
> it can't work at all if another 'm' driver is not loaded,
> 
> in another words, the otg driver has to be 'm' if its dependent driver
> is 'm', correct?

If both host and gadget are 'm' then otg can be 'm', but if either host or
gadget is built in then we have no choice but to make otg as built-in.

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?

cheers,
-roger
    
> 
> Li Jun
> 
>>
>> cheers,
>> -roger
>>
>>>
>>> Li Jun
>>>
>>>>
>>>> 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
>>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roger Quadros May 18, 2016, 12:45 p.m. UTC | #10

On 18/05/16 06:18, Peter Chen wrote:
> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote:
>> On 16/05/16 12:23, Peter Chen wrote:
>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 16/05/16 10:02, Peter Chen wrote:
>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>>>> +
>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
>>>>>> +{
>>>>>> +	struct usb_udc *udc;
>>>>>> +
>>>>>> +	mutex_lock(&udc_lock);
>>>>>> +	udc = usb_gadget_to_udc(gadget);
>>>>>> +	if (!udc) {
>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>>>> +			__func__);
>>>>>> +		mutex_unlock(&udc_lock);
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (connect) {
>>>>>> +		if (!gadget->connected)
>>>>>> +			usb_gadget_connect(udc->gadget);
>>>>>> +	} else {
>>>>>> +		if (gadget->connected) {
>>>>>> +			usb_gadget_disconnect(udc->gadget);
>>>>>> +			udc->driver->disconnect(udc->gadget);
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	mutex_unlock(&udc_lock);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Since this is called for vbus interrupt, why not using
>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
>>>>> at usb_gadget_stop.
>>>>
>>>> We can't assume that this is always called for vbus interrupt so
>>>> I decided not to call usb_udc_vbus_handler.
>>>>
>>>> udc->vbus is really pointless for us. We keep vbus states in our
>>>> state machine and leave udc->vbus as ture always.
>>>>
>>>> Why do you want to move udc->driver->disconnect() to stop?
>>>> If USB controller disconnected from bus then the gadget driver
>>>> must be notified about the disconnect immediately. The controller
>>>> may or may not be stopped by the core.
>>>>
>>>
>>> Then, would you give some comments when this API will be used?
>>> I was assumed it is only used for drd state machine.
>>
>> drd_state machine didn't even need this API in the first place :).
>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case.
>> Won't full OTG state machine want to use this API? If not what would it use?
>>
> 
> Oh, I meant only drd and fully otg state machine needs it. I am
> wondering if we need have a new API to do it. Two questions:

OK.
> 
> - Except for vbus interrupt, any chances this API will be used at
> current logic?

I don't think so. But we can't assume caller behaviour for any API.

> - When this API is called but without a coming gadget->stop?
> 
Never for DRD case. But we want to catch wrong users.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jun Li May 18, 2016, 1:12 p.m. UTC | #11

Hi

> -----Original Message-----
> From: Roger Quadros [mailto:[e-mail]]
> Sent: Wednesday, May 18, 2016 8:43 PM
> To: Jun Li <[e-mail]>; Peter Chen <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> 
> On 17/05/16 11:28, Jun Li wrote:
> > Hi Roger,
> >
> >> -----Original Message-----
> >> From: Roger Quadros [mailto:[e-mail]]
> >> Sent: Tuesday, May 17, 2016 4:09 PM
> >> To: Jun Li <[e-mail]>; Peter Chen <[e-mail]>
> >> Cc: [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>
> >> On 17/05/16 10:38, Jun Li wrote:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: Roger Quadros [mailto:[e-mail]]
> >>>> Sent: Monday, May 16, 2016 5:52 PM
> >>>> To: Peter Chen <[e-mail]>
> >>>> Cc: [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]
> >>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
> >>>>
> >>>> On 16/05/16 12:23, Peter Chen wrote:
> >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 16/05/16 10:02, Peter Chen wrote:
> >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >>>>>>>> +
> >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget
> >>>>>>>> +*gadget, bool connect) {
> >>>>>>>> +	struct usb_udc *udc;
> >>>>>>>> +
> >>>>>>>> +	mutex_lock(&udc_lock);
> >>>>>>>> +	udc = usb_gadget_to_udc(gadget);
> >>>>>>>> +	if (!udc) {
> >>>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not
> >>>> registered.\n",
> >>>>>>>> +			__func__);
> >>>>>>>> +		mutex_unlock(&udc_lock);
> >>>>>>>> +		return -EINVAL;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	if (connect) {
> >>>>>>>> +		if (!gadget->connected)
> >>>>>>>> +			usb_gadget_connect(udc->gadget);
> >>>>>>>> +	} else {
> >>>>>>>> +		if (gadget->connected) {
> >>>>>>>> +			usb_gadget_disconnect(udc->gadget);
> >>>>>>>> +			udc->driver->disconnect(udc->gadget);
> >>>>>>>> +		}
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	mutex_unlock(&udc_lock);
> >>>>>>>> +
> >>>>>>>> +	return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Since this is called for vbus interrupt, why not using
> >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
> >>>>>>> at usb_gadget_stop.
> >>>>>>
> >>>>>> We can't assume that this is always called for vbus interrupt so
> >>>>>> I decided not to call usb_udc_vbus_handler.
> >>>>>>
> >>>>>> udc->vbus is really pointless for us. We keep vbus states in our
> >>>>>> state machine and leave udc->vbus as ture always.
> >>>>>>
> >>>>>> Why do you want to move udc->driver->disconnect() to stop?
> >>>>>> If USB controller disconnected from bus then the gadget driver
> >>>>>> must be notified about the disconnect immediately. The controller
> >>>>>> may or may not be stopped by the core.
> >>>>>>
> >>>>>
> >>>>> Then, would you give some comments when this API will be used?
> >>>>> I was assumed it is only used for drd state machine.
> >>>>
> >>>> drd_state machine didn't even need this API in the first place :).
> >>>> You guys wanted me to separate out start/stop and
> >>>> connect/disconnect for full OTG case.
> >>>> Won't full OTG state machine want to use this API? If not what
> >>>> would it use?
> >>>
> >>> Instead create those new interfaces/symbol here and there just aim
> >>> to address build problems in diff configures, Could we only allow
> >>> meaningful combination of those 3 drivers configures?
> >>>
> >>> Hcd=y, gadget=y, otg=y or
> >>> Hcd=m, gadget=m, otg=m
> >>
> >> This is still a limitation.
> >>
> >> It is perfectly fine to have
> >> hcd=m, gadget=y
> >> or
> >> hcd=y, gadget=m
> >
> > I agree it makes sense to have above configs in non-otg case, that is,
> > the 'y' driver can work without 'm' driver loaded.
> >
> > But,
> > in otg enabled(y/m) case, the otherwise config of my list can't make
> > any sense from my point view. That is: some driver is built-in, but it
> > can't work at all if another 'm' driver is not loaded,
> >
> > in another words, the otg driver has to be 'm' if its dependent driver
> > is 'm', correct?
> 
> If both host and gadget are 'm' then otg can be 'm', but if either host or
> gadget is built in then we have no choice but to make otg as built-in.
> 
> 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 = {
    ...
}

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.

Li Jun 
   
> 
> cheers,
> -roger
> 
> >
> > Li Jun
> >
> >>
> >> cheers,
> >> -roger
> >>
> >>>
> >>> Li Jun
> >>>
> >>>>
> >>>> 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
> >>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roger Quadros May 18, 2016, 1:43 p.m. UTC | #12

On 18/05/16 16:12, Jun Li wrote:
> Hi
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:[e-mail]]
>> Sent: Wednesday, May 18, 2016 8:43 PM
>> To: Jun Li <[e-mail]>; Peter Chen <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>
>> On 17/05/16 11:28, Jun Li wrote:
>>> Hi Roger,
>>>
>>>> -----Original Message-----
>>>> From: Roger Quadros [mailto:[e-mail]]
>>>> Sent: Tuesday, May 17, 2016 4:09 PM
>>>> To: Jun Li <[e-mail]>; Peter Chen <[e-mail]>
>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>
>>>> On 17/05/16 10:38, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Roger Quadros [mailto:[e-mail]]
>>>>>> Sent: Monday, May 16, 2016 5:52 PM
>>>>>> To: Peter Chen <[e-mail]>
>>>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail]
>>>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core
>>>>>>
>>>>>> On 16/05/16 12:23, Peter Chen wrote:
>>>>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 16/05/16 10:02, Peter Chen wrote:
>>>>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>>>>>>>> +
>>>>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget
>>>>>>>>>> +*gadget, bool connect) {
>>>>>>>>>> +	struct usb_udc *udc;
>>>>>>>>>> +
>>>>>>>>>> +	mutex_lock(&udc_lock);
>>>>>>>>>> +	udc = usb_gadget_to_udc(gadget);
>>>>>>>>>> +	if (!udc) {
>>>>>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not
>>>>>> registered.\n",
>>>>>>>>>> +			__func__);
>>>>>>>>>> +		mutex_unlock(&udc_lock);
>>>>>>>>>> +		return -EINVAL;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	if (connect) {
>>>>>>>>>> +		if (!gadget->connected)
>>>>>>>>>> +			usb_gadget_connect(udc->gadget);
>>>>>>>>>> +	} else {
>>>>>>>>>> +		if (gadget->connected) {
>>>>>>>>>> +			usb_gadget_disconnect(udc->gadget);
>>>>>>>>>> +			udc->driver->disconnect(udc->gadget);
>>>>>>>>>> +		}
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	mutex_unlock(&udc_lock);
>>>>>>>>>> +
>>>>>>>>>> +	return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Since this is called for vbus interrupt, why not using
>>>>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
>>>>>>>>> at usb_gadget_stop.
>>>>>>>>
>>>>>>>> We can't assume that this is always called for vbus interrupt so
>>>>>>>> I decided not to call usb_udc_vbus_handler.
>>>>>>>>
>>>>>>>> udc->vbus is really pointless for us. We keep vbus states in our
>>>>>>>> state machine and leave udc->vbus as ture always.
>>>>>>>>
>>>>>>>> Why do you want to move udc->driver->disconnect() to stop?
>>>>>>>> If USB controller disconnected from bus then the gadget driver
>>>>>>>> must be notified about the disconnect immediately. The controller
>>>>>>>> may or may not be stopped by the core.
>>>>>>>>
>>>>>>>
>>>>>>> Then, would you give some comments when this API will be used?
>>>>>>> I was assumed it is only used for drd state machine.
>>>>>>
>>>>>> drd_state machine didn't even need this API in the first place :).
>>>>>> You guys wanted me to separate out start/stop and
>>>>>> connect/disconnect for full OTG case.
>>>>>> Won't full OTG state machine want to use this API? If not what
>>>>>> would it use?
>>>>>
>>>>> Instead create those new interfaces/symbol here and there just aim
>>>>> to address build problems in diff configures, Could we only allow
>>>>> meaningful combination of those 3 drivers configures?
>>>>>
>>>>> Hcd=y, gadget=y, otg=y or
>>>>> Hcd=m, gadget=m, otg=m
>>>>
>>>> This is still a limitation.
>>>>
>>>> It is perfectly fine to have
>>>> hcd=m, gadget=y
>>>> or
>>>> hcd=y, gadget=m
>>>
>>> I agree it makes sense to have above configs in non-otg case, that is,
>>> the 'y' driver can work without 'm' driver loaded.
>>>
>>> But,
>>> in otg enabled(y/m) case, the otherwise config of my list can't make
>>> any sense from my point view. That is: some driver is built-in, but it
>>> can't work at all if another 'm' driver is not loaded,
>>>
>>> in another words, the otg driver has to be 'm' if its dependent driver
>>> is 'm', correct?
>>
>> If both host and gadget are 'm' then otg can be 'm', but if either host or
>> gadget is built in then we have no choice but to make otg as built-in.
>>
>> 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.

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
>>>>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jun Li May 18, 2016, 2:46 p.m. UTC | #13

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

I think hcd and gadget are independent each other, now

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

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)

Li Jun

> 
> 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
> >>>>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roger Quadros May 19, 2016, 7:32 a.m. UTC | #14

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
>>>>>>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen May 20, 2016, 1:39 a.m. UTC | #15

On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote:
> On 18/05/16 06:18, Peter Chen wrote:
> > On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote:
> >> On 16/05/16 12:23, Peter Chen wrote:
> >>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> >>>> Hi,
> >>>>
> >>>> On 16/05/16 10:02, Peter Chen wrote:
> >>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >>>>>> +
> >>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
> >>>>>> +{
> >>>>>> +	struct usb_udc *udc;
> >>>>>> +
> >>>>>> +	mutex_lock(&udc_lock);
> >>>>>> +	udc = usb_gadget_to_udc(gadget);
> >>>>>> +	if (!udc) {
> >>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >>>>>> +			__func__);
> >>>>>> +		mutex_unlock(&udc_lock);
> >>>>>> +		return -EINVAL;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	if (connect) {
> >>>>>> +		if (!gadget->connected)
> >>>>>> +			usb_gadget_connect(udc->gadget);
> >>>>>> +	} else {
> >>>>>> +		if (gadget->connected) {
> >>>>>> +			usb_gadget_disconnect(udc->gadget);
> >>>>>> +			udc->driver->disconnect(udc->gadget);
> >>>>>> +		}
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	mutex_unlock(&udc_lock);
> >>>>>> +
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>
> >>>>> Since this is called for vbus interrupt, why not using
> >>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
> >>>>> at usb_gadget_stop.
> >>>>
> >>>> We can't assume that this is always called for vbus interrupt so
> >>>> I decided not to call usb_udc_vbus_handler.
> >>>>
> >>>> udc->vbus is really pointless for us. We keep vbus states in our
> >>>> state machine and leave udc->vbus as ture always.
> >>>>
> >>>> Why do you want to move udc->driver->disconnect() to stop?
> >>>> If USB controller disconnected from bus then the gadget driver
> >>>> must be notified about the disconnect immediately. The controller
> >>>> may or may not be stopped by the core.
> >>>>
> >>>
> >>> Then, would you give some comments when this API will be used?
> >>> I was assumed it is only used for drd state machine.
> >>
> >> drd_state machine didn't even need this API in the first place :).
> >> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case.
> >> Won't full OTG state machine want to use this API? If not what would it use?
> >>
> > 
> > Oh, I meant only drd and fully otg state machine needs it. I am
> > wondering if we need have a new API to do it. Two questions:
> 
> OK.
> > 
> > - Except for vbus interrupt, any chances this API will be used at
> > current logic?
> 
> I don't think so. But we can't assume caller behaviour for any API.
> 
> > - When this API is called but without a coming gadget->stop?
> > 
> Never for DRD case. But we want to catch wrong users.
> 

In future, otg_start_gadget will be used for both DRD and fully OTG FSM.
There is no otg_loc_conn at current DRD FSM, but there is
otg_loc_conn at current OTG FSM, see below.

DRD FSM:
	case OTG_STATE_B_IDLE:
		drd_set_protocol(fsm, PROTO_UNDEF);
		otg_drv_vbus(otg, 0);
		break;
	case OTG_STATE_B_PERIPHERAL:
		drd_set_protocol(fsm, PROTO_GADGET);
		otg_drv_vbus(otg, 0);
		break;

OTG FSM:
	case OTG_STATE_B_IDLE:
		otg_drv_vbus(otg, 0);
		otg_chrg_vbus(otg, 0);
		otg_loc_conn(otg, 0);
		otg_loc_sof(otg, 0);
		/*
		 * Driver is responsible for starting ADP probing
		 * if ADP sensing times out.
		 */
		otg_start_adp_sns(otg);
		otg_set_protocol(fsm, PROTO_UNDEF);
		otg_add_timer(otg, B_SE0_SRP);
		break;
	case OTG_STATE_B_PERIPHERAL:
		otg_chrg_vbus(otg, 0);
		otg_loc_sof(otg, 0);
		otg_set_protocol(fsm, PROTO_GADGET);
		otg_loc_conn(otg, 1);
		break;

My original suggestion is to have an API to do pull dp and this API
will be used at both DRD and OTG FSM, and called at otg_loc_conn.
The (de)initialize is the same for both two FSMs, it both includes
init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET)
otg_loc_conn(otg, 1);

What do you think?

Roger Quadros May 20, 2016, 7:26 a.m. UTC | #16

Peter,

On 20/05/16 04:39, Peter Chen wrote:
> On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote:
>> On 18/05/16 06:18, Peter Chen wrote:
>>> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote:
>>>> On 16/05/16 12:23, Peter Chen wrote:
>>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 16/05/16 10:02, Peter Chen wrote:
>>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>>>>>>>> +
>>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
>>>>>>>> +{
>>>>>>>> +	struct usb_udc *udc;
>>>>>>>> +
>>>>>>>> +	mutex_lock(&udc_lock);
>>>>>>>> +	udc = usb_gadget_to_udc(gadget);
>>>>>>>> +	if (!udc) {
>>>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>>>>>> +			__func__);
>>>>>>>> +		mutex_unlock(&udc_lock);
>>>>>>>> +		return -EINVAL;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	if (connect) {
>>>>>>>> +		if (!gadget->connected)
>>>>>>>> +			usb_gadget_connect(udc->gadget);
>>>>>>>> +	} else {
>>>>>>>> +		if (gadget->connected) {
>>>>>>>> +			usb_gadget_disconnect(udc->gadget);
>>>>>>>> +			udc->driver->disconnect(udc->gadget);
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	mutex_unlock(&udc_lock);
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>
>>>>>>> Since this is called for vbus interrupt, why not using
>>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
>>>>>>> at usb_gadget_stop.
>>>>>>
>>>>>> We can't assume that this is always called for vbus interrupt so
>>>>>> I decided not to call usb_udc_vbus_handler.
>>>>>>
>>>>>> udc->vbus is really pointless for us. We keep vbus states in our
>>>>>> state machine and leave udc->vbus as ture always.
>>>>>>
>>>>>> Why do you want to move udc->driver->disconnect() to stop?
>>>>>> If USB controller disconnected from bus then the gadget driver
>>>>>> must be notified about the disconnect immediately. The controller
>>>>>> may or may not be stopped by the core.
>>>>>>
>>>>>
>>>>> Then, would you give some comments when this API will be used?
>>>>> I was assumed it is only used for drd state machine.
>>>>
>>>> drd_state machine didn't even need this API in the first place :).
>>>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case.
>>>> Won't full OTG state machine want to use this API? If not what would it use?
>>>>
>>>
>>> Oh, I meant only drd and fully otg state machine needs it. I am
>>> wondering if we need have a new API to do it. Two questions:
>>
>> OK.
>>>
>>> - Except for vbus interrupt, any chances this API will be used at
>>> current logic?
>>
>> I don't think so. But we can't assume caller behaviour for any API.
>>
>>> - When this API is called but without a coming gadget->stop?
>>>
>> Never for DRD case. But we want to catch wrong users.
>>
> 
> In future, otg_start_gadget will be used for both DRD and fully OTG FSM.
> There is no otg_loc_conn at current DRD FSM, but there is
> otg_loc_conn at current OTG FSM, see below.
> 
> DRD FSM:
> 	case OTG_STATE_B_IDLE:
> 		drd_set_protocol(fsm, PROTO_UNDEF);
> 		otg_drv_vbus(otg, 0);
> 		break;
> 	case OTG_STATE_B_PERIPHERAL:
> 		drd_set_protocol(fsm, PROTO_GADGET);
> 		otg_drv_vbus(otg, 0);
> 		break;
> 
> OTG FSM:
> 	case OTG_STATE_B_IDLE:
> 		otg_drv_vbus(otg, 0);
> 		otg_chrg_vbus(otg, 0);
> 		otg_loc_conn(otg, 0);
> 		otg_loc_sof(otg, 0);
> 		/*
> 		 * Driver is responsible for starting ADP probing
> 		 * if ADP sensing times out.
> 		 */
> 		otg_start_adp_sns(otg);
> 		otg_set_protocol(fsm, PROTO_UNDEF);
> 		otg_add_timer(otg, B_SE0_SRP);
> 		break;
> 	case OTG_STATE_B_PERIPHERAL:
> 		otg_chrg_vbus(otg, 0);
> 		otg_loc_sof(otg, 0);
> 		otg_set_protocol(fsm, PROTO_GADGET);
> 		otg_loc_conn(otg, 1);
> 		break;
> 
> My original suggestion is to have an API to do pull dp and this API
> will be used at both DRD and OTG FSM, and called at otg_loc_conn.

The API is usb_gadget_connect_control();

> The (de)initialize is the same for both two FSMs, it both includes
> init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET)
> otg_loc_conn(otg, 1);
> 
> What do you think?
> 

I think loc_conn is a bit confusing to drd users. Another issue I see is that 
DRD controller drivers will need to explicitly pass .loc_conn ops via the otg_fsm_ops.
This is an additional step and totally unnecessary as it can be automatically done
via direct DRD -> UDC-core call.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen May 21, 2016, 2:29 a.m. UTC | #17

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.

Peter Chen May 21, 2016, 2:44 a.m. UTC | #18

On Fri, May 20, 2016 at 10:26:03AM +0300, Roger Quadros wrote:
> Peter,
> 
> On 20/05/16 04:39, Peter Chen wrote:
> > On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote:
> >> On 18/05/16 06:18, Peter Chen wrote:
> >>> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote:
> >>>> On 16/05/16 12:23, Peter Chen wrote:
> >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 16/05/16 10:02, Peter Chen wrote:
> >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> >>>>>>>> +
> >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
> >>>>>>>> +{
> >>>>>>>> +	struct usb_udc *udc;
> >>>>>>>> +
> >>>>>>>> +	mutex_lock(&udc_lock);
> >>>>>>>> +	udc = usb_gadget_to_udc(gadget);
> >>>>>>>> +	if (!udc) {
> >>>>>>>> +		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >>>>>>>> +			__func__);
> >>>>>>>> +		mutex_unlock(&udc_lock);
> >>>>>>>> +		return -EINVAL;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	if (connect) {
> >>>>>>>> +		if (!gadget->connected)
> >>>>>>>> +			usb_gadget_connect(udc->gadget);
> >>>>>>>> +	} else {
> >>>>>>>> +		if (gadget->connected) {
> >>>>>>>> +			usb_gadget_disconnect(udc->gadget);
> >>>>>>>> +			udc->driver->disconnect(udc->gadget);
> >>>>>>>> +		}
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	mutex_unlock(&udc_lock);
> >>>>>>>> +
> >>>>>>>> +	return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Since this is called for vbus interrupt, why not using
> >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect
> >>>>>>> at usb_gadget_stop.
> >>>>>>
> >>>>>> We can't assume that this is always called for vbus interrupt so
> >>>>>> I decided not to call usb_udc_vbus_handler.
> >>>>>>
> >>>>>> udc->vbus is really pointless for us. We keep vbus states in our
> >>>>>> state machine and leave udc->vbus as ture always.
> >>>>>>
> >>>>>> Why do you want to move udc->driver->disconnect() to stop?
> >>>>>> If USB controller disconnected from bus then the gadget driver
> >>>>>> must be notified about the disconnect immediately. The controller
> >>>>>> may or may not be stopped by the core.
> >>>>>>
> >>>>>
> >>>>> Then, would you give some comments when this API will be used?
> >>>>> I was assumed it is only used for drd state machine.
> >>>>
> >>>> drd_state machine didn't even need this API in the first place :).
> >>>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case.
> >>>> Won't full OTG state machine want to use this API? If not what would it use?
> >>>>
> >>>
> >>> Oh, I meant only drd and fully otg state machine needs it. I am
> >>> wondering if we need have a new API to do it. Two questions:
> >>
> >> OK.
> >>>
> >>> - Except for vbus interrupt, any chances this API will be used at
> >>> current logic?
> >>
> >> I don't think so. But we can't assume caller behaviour for any API.
> >>
> >>> - When this API is called but without a coming gadget->stop?
> >>>
> >> Never for DRD case. But we want to catch wrong users.
> >>
> > 
> > In future, otg_start_gadget will be used for both DRD and fully OTG FSM.
> > There is no otg_loc_conn at current DRD FSM, but there is
> > otg_loc_conn at current OTG FSM, see below.
> > 
> > DRD FSM:
> > 	case OTG_STATE_B_IDLE:
> > 		drd_set_protocol(fsm, PROTO_UNDEF);
> > 		otg_drv_vbus(otg, 0);
> > 		break;
> > 	case OTG_STATE_B_PERIPHERAL:
> > 		drd_set_protocol(fsm, PROTO_GADGET);
> > 		otg_drv_vbus(otg, 0);
> > 		break;
> > 
> > OTG FSM:
> > 	case OTG_STATE_B_IDLE:
> > 		otg_drv_vbus(otg, 0);
> > 		otg_chrg_vbus(otg, 0);
> > 		otg_loc_conn(otg, 0);
> > 		otg_loc_sof(otg, 0);
> > 		/*
> > 		 * Driver is responsible for starting ADP probing
> > 		 * if ADP sensing times out.
> > 		 */
> > 		otg_start_adp_sns(otg);
> > 		otg_set_protocol(fsm, PROTO_UNDEF);
> > 		otg_add_timer(otg, B_SE0_SRP);
> > 		break;
> > 	case OTG_STATE_B_PERIPHERAL:
> > 		otg_chrg_vbus(otg, 0);
> > 		otg_loc_sof(otg, 0);
> > 		otg_set_protocol(fsm, PROTO_GADGET);
> > 		otg_loc_conn(otg, 1);
> > 		break;
> > 
> > My original suggestion is to have an API to do pull dp and this API
> > will be used at both DRD and OTG FSM, and called at otg_loc_conn.
> 
> The API is usb_gadget_connect_control();
> 
> > The (de)initialize is the same for both two FSMs, it both includes
> > init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET)
> > otg_loc_conn(otg, 1);
> > 
> > What do you think?
> > 
> 
> I think loc_conn is a bit confusing to drd users. Another issue I see is that 
> DRD controller drivers will need to explicitly pass .loc_conn ops via the otg_fsm_ops.
> This is an additional step and totally unnecessary as it can be automatically done
> via direct DRD -> UDC-core call.
> 

If you are stick to that, let's follow your way if Felipe agree with it
too, although it lets the DRD state machine look different with OTG's.

Peter Chen May 23, 2016, 3:21 a.m. UTC | #19

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.

Roger Quadros May 23, 2016, 10:11 a.m. UTC | #20

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

I think some kconfig trickery can be done to get what we want.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jun Li May 23, 2016, 10:34 a.m. UTC | #21

Hi

> -----Original Message-----
> From: Roger Quadros [mailto:[e-mail]]
> Sent: Monday, May 23, 2016 6:12 PM
> To: Peter Chen <[e-mail]>
> Cc: Jun Li <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> 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.

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?

Li Jun

> 
> I think some kconfig trickery can be done to get what we want.
> 
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roger Quadros May 23, 2016, 10:36 a.m. UTC | #22

On 23/05/16 13:34, Jun Li wrote:
> Hi
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:[e-mail]]
>> Sent: Monday, May 23, 2016 6:12 PM
>> To: Peter Chen <[e-mail]>
>> Cc: Jun Li <[e-mail]>; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> 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 :).

cheers,
-roger
> 
> Li Jun
> 
>>
>> I think some kconfig trickery can be done to get what we want.
>>
>> cheers,
>> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen May 24, 2016, 2:53 a.m. UTC | #23

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:[e-mail]]
> >> Sent: Monday, May 23, 2016 6:12 PM
> >> To: Peter Chen <[e-mail]>
> >> Cc: Jun Li <[e-mail]>; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> 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.

Peter Chen June 1, 2016, 7:38 a.m. UTC | #24

On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>  	}
>  	mutex_unlock(&udc_lock);
>  
> +	mutex_unlock(&udc_lock);
> +

Here, you have one more mutex_unlock.

Peter
>  	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
>  	flush_work(&gadget->work);
>  	device_unregister(&udc->dev);
> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>  
>  /* ------------------------------------------------------------------------- */
>  
> +struct otg_gadget_ops otg_gadget_intf = {
> +	.start = usb_gadget_start,
> +	.stop = usb_gadget_stop,
> +	.connect_control = usb_gadget_connect_control,
> +};
> +
> +/* udc_lock must be held */
>  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
>  {
>  	int ret;
> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>  	ret = driver->bind(udc->gadget, driver);
>  	if (ret)
>  		goto err1;
> -	ret = usb_gadget_udc_start(udc);
> -	if (ret) {
> -		driver->unbind(udc->gadget);
> -		goto err1;
> +
> +	/* If OTG, the otg core starts the UDC when needed */
> +	if (udc->gadget->otg_dev) {
> +		mutex_unlock(&udc_lock);
> +		usb_otg_register_gadget(udc->gadget, &otg_gadget_intf);
> +		mutex_lock(&udc_lock);
> +	} else {
> +		ret = usb_gadget_udc_start(udc);
> +		if (ret) {
> +			driver->unbind(udc->gadget);
> +			goto err1;
> +		}
> +		usb_udc_connect_control(udc);
>  	}
> -	usb_udc_connect_control(udc);
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  	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;
> +	}
> +
>  	if (sysfs_streq(buf, "connect")) {
>  		usb_gadget_udc_start(udc);
> -		usb_gadget_connect(udc->gadget);
> +		usb_udc_connect_control(udc);
>  	} else if (sysfs_streq(buf, "disconnect")) {
>  		usb_gadget_disconnect(udc->gadget);
>  		udc->driver->disconnect(udc->gadget);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3ecfddd..79d654f 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
>  extern void usb_del_gadget_udc(struct usb_gadget *gadget);
>  extern char *usb_get_gadget_udc_name(void);
>  
> +extern int usb_otg_add_gadget_udc(struct device *parent,
> +				  struct usb_gadget *gadget,
> +				  struct device *otg_dev);
> +
>  /*-------------------------------------------------------------------------*/
>  
>  /* utility to simplify dealing with string descriptors */
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roger Quadros June 2, 2016, 11:07 a.m. UTC | #25

On 01/06/16 10:38, Peter Chen wrote:
> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote:
>> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>>  	}
>>  	mutex_unlock(&udc_lock);
>>  
>> +	mutex_unlock(&udc_lock);
>> +
> 
> Here, you have one more mutex_unlock.

Will fix it. Thanks.

>>  	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
>>  	flush_work(&gadget->work);
>>  	device_unregister(&udc->dev);
>> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>>  
>>  /* ------------------------------------------------------------------------- */
>>  
>> +struct otg_gadget_ops otg_gadget_intf = {
>> +	.start = usb_gadget_start,
>> +	.stop = usb_gadget_stop,
>> +	.connect_control = usb_gadget_connect_control,
>> +};
>> +
>> +/* udc_lock must be held */
>>  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
>>  {
>>  	int ret;
>> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>>  	ret = driver->bind(udc->gadget, driver);
>>  	if (ret)
>>  		goto err1;
>> -	ret = usb_gadget_udc_start(udc);
>> -	if (ret) {
>> -		driver->unbind(udc->gadget);
>> -		goto err1;
>> +
>> +	/* If OTG, the otg core starts the UDC when needed */
>> +	if (udc->gadget->otg_dev) {
>> +		mutex_unlock(&udc_lock);
>> +		usb_otg_register_gadget(udc->gadget, &otg_gadget_intf);
>> +		mutex_lock(&udc_lock);
>> +	} else {
>> +		ret = usb_gadget_udc_start(udc);
>> +		if (ret) {
>> +			driver->unbind(udc->gadget);
>> +			goto err1;
>> +		}
>> +		usb_udc_connect_control(udc);
>>  	}
>> -	usb_udc_connect_control(udc);
>>  
>>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>  	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;
>> +	}
>> +
>>  	if (sysfs_streq(buf, "connect")) {
>>  		usb_gadget_udc_start(udc);
>> -		usb_gadget_connect(udc->gadget);
>> +		usb_udc_connect_control(udc);
>>  	} else if (sysfs_streq(buf, "disconnect")) {
>>  		usb_gadget_disconnect(udc->gadget);
>>  		udc->driver->disconnect(udc->gadget);
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 3ecfddd..79d654f 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
>>  extern void usb_del_gadget_udc(struct usb_gadget *gadget);
>>  extern char *usb_get_gadget_udc_name(void);
>>  
>> +extern int usb_otg_add_gadget_udc(struct device *parent,
>> +				  struct usb_gadget *gadget,
>> +				  struct device *otg_dev);
>> +
>>  /*-------------------------------------------------------------------------*/
>>  
>>  /* utility to simplify dealing with string descriptors */
>> -- 
>> 2.7.4
>>

--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 4151597..21c85ef 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,11 @@ 
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/of.h>
+
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -325,6 +330,119 @@  static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 }
 
 /**
+ * usb_gadget_to_udc - get the UDC owning the gadget
+ *
+ * udc_lock must be held.
+ * Returs NULL if UDC is not found.
+ */
+static struct usb_udc *usb_gadget_to_udc(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc;
+
+	list_for_each_entry(udc, &udc_list, list)
+		if (udc->gadget == gadget)
+			return udc;
+
+	return NULL;
+}
+
+/**
+ * usb_gadget_start - start the usb gadget controller
+ * @gadget: the gadget device to start
+ *
+ * This is external API for use by OTG core.
+ *
+ * Start the usb device controller. Does not connect to the bus.
+ */
+static int usb_gadget_start(struct usb_gadget *gadget)
+{
+	int ret;
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	ret = usb_gadget_udc_start(udc);
+	if (ret)
+		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
+			ret);
+
+	mutex_unlock(&udc_lock);
+
+	return ret;
+}
+
+/**
+ * usb_gadget_stop - stop the usb gadget controller
+ * @gadget: the gadget device we want to stop
+ *
+ * This is external API for use by OTG core.
+ *
+ * Stop the gadget controller. Does not disconnect from the bus.
+ * Caller must ensure that gadget has disconnected from the bus
+ * before calling usb_gadget_stop().
+ */
+static int usb_gadget_stop(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	if (gadget->connected) {
+		dev_err(gadget->dev.parent,
+			"%s: called while still connected\n", __func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	usb_gadget_udc_stop(udc);
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+
+static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
+{
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	if (connect) {
+		if (!gadget->connected)
+			usb_gadget_connect(udc->gadget);
+	} else {
+		if (gadget->connected) {
+			usb_gadget_disconnect(udc->gadget);
+			udc->driver->disconnect(udc->gadget);
+		}
+	}
+
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+
+/**
  * usb_udc_release - release the usb_udc struct
  * @dev: the dev member within usb_udc
  *
@@ -486,6 +604,33 @@  int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc);
 
+/**
+ * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
+ * @parent: the parent device to this udc. Usually the controller
+ * driver's device.
+ * @gadget: the gadget to be added to the list
+ * @otg_dev: the OTG controller device
+ *
+ * If otg_dev is NULL then device tree node is checked
+ * for OTG controller via the otg-controller property.
+ * Returns zero on success, negative errno otherwise.
+ */
+int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget,
+			   struct device *otg_dev)
+{
+	if (!otg_dev) {
+		gadget->otg_dev = of_usb_get_otg(parent->of_node);
+		if (!gadget->otg_dev)
+			return -ENODEV;
+	} else {
+		gadget->otg_dev = otg_dev;
+	}
+
+	return usb_add_gadget_udc_release(parent, gadget, NULL);
+}
+EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc);
+
+/* udc_lock must be held */
 static void usb_gadget_remove_driver(struct usb_udc *udc)
 {
 	dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n",
@@ -493,10 +638,18 @@  static void usb_gadget_remove_driver(struct usb_udc *udc)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	usb_gadget_disconnect(udc->gadget);
-	udc->driver->disconnect(udc->gadget);
+	/* If OTG, the otg core ensures UDC is stopped on unregister */
+	if (udc->gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		usb_otg_unregister_gadget(udc->gadget);
+		mutex_lock(&udc_lock);
+	} else {
+		usb_gadget_disconnect(udc->gadget);
+		udc->driver->disconnect(udc->gadget);
+		usb_gadget_udc_stop(udc);
+	}
+
 	udc->driver->unbind(udc->gadget);
-	usb_gadget_udc_stop(udc);
 
 	udc->driver = NULL;
 	udc->dev.driver = NULL;
@@ -530,6 +683,8 @@  void usb_del_gadget_udc(struct usb_gadget *gadget)
 	}
 	mutex_unlock(&udc_lock);
 
+	mutex_unlock(&udc_lock);
+
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
@@ -539,6 +694,13 @@  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 /* ------------------------------------------------------------------------- */
 
+struct otg_gadget_ops otg_gadget_intf = {
+	.start = usb_gadget_start,
+	.stop = usb_gadget_stop,
+	.connect_control = usb_gadget_connect_control,
+};
+
+/* udc_lock must be held */
 static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
 {
 	int ret;
@@ -553,12 +715,20 @@  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
 		goto err1;
-	ret = usb_gadget_udc_start(udc);
-	if (ret) {
-		driver->unbind(udc->gadget);
-		goto err1;
+
+	/* If OTG, the otg core starts the UDC when needed */
+	if (udc->gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		usb_otg_register_gadget(udc->gadget, &otg_gadget_intf);
+		mutex_lock(&udc_lock);
+	} else {
+		ret = usb_gadget_udc_start(udc);
+		if (ret) {
+			driver->unbind(udc->gadget);
+			goto err1;
+		}
+		usb_udc_connect_control(udc);
 	}
-	usb_udc_connect_control(udc);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	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;
+	}
+
 	if (sysfs_streq(buf, "connect")) {
 		usb_gadget_udc_start(udc);
-		usb_gadget_connect(udc->gadget);
+		usb_udc_connect_control(udc);
 	} else if (sysfs_streq(buf, "disconnect")) {
 		usb_gadget_disconnect(udc->gadget);
 		udc->driver->disconnect(udc->gadget);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3ecfddd..79d654f 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1162,6 +1162,10 @@  extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
 extern char *usb_get_gadget_udc_name(void);
 
+extern int usb_otg_add_gadget_udc(struct device *parent,
+				  struct usb_gadget *gadget,
+				  struct device *otg_dev);
+
 /*-------------------------------------------------------------------------*/
 
 /* utility to simplify dealing with string descriptors */