Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383887 > unrolled thread

RE: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-21 08:40 +0200
    Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-25 16:10 +0200
      RE: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-26 02:10 +0200
        Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:30 +0200
          Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-28 12:00 +0200

#1383887 — RE: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core

Hi,

...
> 
>  /**
> + * usb_gadget_start - start the usb gadget controller and connect to
> +bus
> + * @gadget: the gadget device to start
> + *
> + * This is external API for use by OTG core.
> + *
> + * Start the usb device controller and connect to bus (enable pull).
> + */
> +static int usb_gadget_start(struct usb_gadget *gadget) {
> +	int ret;
> +	struct usb_udc *udc = NULL;
> +
> +	dev_dbg(&gadget->dev, "%s\n", __func__);
> +	mutex_lock(&udc_lock);
> +	list_for_each_entry(udc, &udc_list, list)
> +		if (udc->gadget == gadget)
> +			goto found;
> +
> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> +		__func__);
> +	mutex_unlock(&udc_lock);
> +	return -EINVAL;
> +
> +found:
> +	ret = usb_gadget_udc_start(udc);
> +	if (ret)
> +		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
> +			ret);
> +	else
> +		usb_udc_connect_control(udc);

For drd, it's fine, but for real otg, gadget connect should be done
by loc_conn() instead of gadget start.

> +
> +	mutex_unlock(&udc_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * usb_gadget_stop - disconnect from bus and stop the usb gadget
> + * @gadget: The gadget device we want to stop
> + *
> + * This is external API for use by OTG core.
> + *
> + * Disconnect from the bus (disable pull) and stop the
> + * gadget controller.
> + */
> +static int usb_gadget_stop(struct usb_gadget *gadget) {
> +	struct usb_udc *udc = NULL;
> +
> +	dev_dbg(&gadget->dev, "%s\n", __func__);
> +	mutex_lock(&udc_lock);
> +	list_for_each_entry(udc, &udc_list, list)
> +		if (udc->gadget == gadget)
> +			goto found;
> +
> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> +		__func__);
> +	mutex_unlock(&udc_lock);
> +	return -EINVAL;
> +
> +found:
> +	usb_gadget_disconnect(udc->gadget);

Likewise, gadget disconnect also should be done by loc_conn()
instead of gadget stop.

> +	udc->driver->disconnect(udc->gadget);
> +	usb_gadget_udc_stop(udc);
> +	mutex_unlock(&udc_lock);
> +
> +	return 0;
> +}
> +

Li Jun

[toc] | [next] | [standalone]

#1386489

Hi,

On 21/04/16 09:38, Jun Li wrote:
> Hi,
> 
> ...
>>
>>  /**
>> + * usb_gadget_start - start the usb gadget controller and connect to
>> +bus
>> + * @gadget: the gadget device to start
>> + *
>> + * This is external API for use by OTG core.
>> + *
>> + * Start the usb device controller and connect to bus (enable pull).
>> + */
>> +static int usb_gadget_start(struct usb_gadget *gadget) {
>> +	int ret;
>> +	struct usb_udc *udc = NULL;
>> +
>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>> +	mutex_lock(&udc_lock);
>> +	list_for_each_entry(udc, &udc_list, list)
>> +		if (udc->gadget == gadget)
>> +			goto found;
>> +
>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +		__func__);
>> +	mutex_unlock(&udc_lock);
>> +	return -EINVAL;
>> +
>> +found:
>> +	ret = usb_gadget_udc_start(udc);
>> +	if (ret)
>> +		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
>> +			ret);
>> +	else
>> +		usb_udc_connect_control(udc);
> 
> For drd, it's fine, but for real otg, gadget connect should be done
> by loc_conn() instead of gadget start.

It is upto the OTG state machine to call gadget_start() when it needs to
connect to the bus (i.e. loc_conn()). I see no point in calling
gadget start before.

Do you see any issue in doing so?

cheers,
-roger

> 
>> +
>> +	mutex_unlock(&udc_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * usb_gadget_stop - disconnect from bus and stop the usb gadget
>> + * @gadget: The gadget device we want to stop
>> + *
>> + * This is external API for use by OTG core.
>> + *
>> + * Disconnect from the bus (disable pull) and stop the
>> + * gadget controller.
>> + */
>> +static int usb_gadget_stop(struct usb_gadget *gadget) {
>> +	struct usb_udc *udc = NULL;
>> +
>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>> +	mutex_lock(&udc_lock);
>> +	list_for_each_entry(udc, &udc_list, list)
>> +		if (udc->gadget == gadget)
>> +			goto found;
>> +
>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>> +		__func__);
>> +	mutex_unlock(&udc_lock);
>> +	return -EINVAL;
>> +
>> +found:
>> +	usb_gadget_disconnect(udc->gadget);
> 
> Likewise, gadget disconnect also should be done by loc_conn()
> instead of gadget stop.
> 
>> +	udc->driver->disconnect(udc->gadget);
>> +	usb_gadget_udc_stop(udc);
>> +	mutex_unlock(&udc_lock);
>> +
>> +	return 0;
>> +}
>> +
> 
> Li Jun
> 

[toc] | [prev] | [next] | [standalone]

#1387022

Hi

> -----Original Message-----
> From: Roger Quadros [mailto:rogerq@ti.com]
> Sent: Monday, April 25, 2016 10:04 PM
> To: Jun Li <jun.li@nxp.com>; stern@rowland.harvard.edu; balbi@kernel.org;
> gregkh@linuxfoundation.org; peter.chen@freescale.com
> Cc: dan.j.williams@intel.com; jun.li@freescale.com;
> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core
> 
> Hi,
> 
> On 21/04/16 09:38, Jun Li wrote:
> > Hi,
> >
> > ...
> >>
> >>  /**
> >> + * usb_gadget_start - start the usb gadget controller and connect to
> >> +bus
> >> + * @gadget: the gadget device to start
> >> + *
> >> + * This is external API for use by OTG core.
> >> + *
> >> + * Start the usb device controller and connect to bus (enable pull).
> >> + */
> >> +static int usb_gadget_start(struct usb_gadget *gadget) {
> >> +	int ret;
> >> +	struct usb_udc *udc = NULL;
> >> +
> >> +	dev_dbg(&gadget->dev, "%s\n", __func__);
> >> +	mutex_lock(&udc_lock);
> >> +	list_for_each_entry(udc, &udc_list, list)
> >> +		if (udc->gadget == gadget)
> >> +			goto found;
> >> +
> >> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >> +		__func__);
> >> +	mutex_unlock(&udc_lock);
> >> +	return -EINVAL;
> >> +
> >> +found:
> >> +	ret = usb_gadget_udc_start(udc);
> >> +	if (ret)
> >> +		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
> >> +			ret);
> >> +	else
> >> +		usb_udc_connect_control(udc);
> >
> > For drd, it's fine, but for real otg, gadget connect should be done by
> > loc_conn() instead of gadget start.
> 
> It is upto the OTG state machine to call gadget_start() when it needs to
> connect to the bus (i.e. loc_conn()). I see no point in calling gadget
> start before.
> 
> Do you see any issue in doing so?

This is what OTG state machine does:
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;

You intend to abstract something common in this api when start gadget,
which should be called by otg_set_protocol(fsm, PROTO_GADGET); and
drd_set_protocol(fsm, PROTO_GADGET); right?

So you may move usb_udc_connect_control(IMO usb_gadget_connect()
is better)out of usb_gadget_start(), then for drd:

case OTG_STATE_B_PERIPHERAL:
     drd_set_protocol(fsm, PROTO_GADGET);
     otg_drv_vbus(otg, 0);
     usb_gadget_connect();

Li Jun

> 
> cheers,
> -roger
> 
> >
> >> +
> >> +	mutex_unlock(&udc_lock);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +/**
> >> + * usb_gadget_stop - disconnect from bus and stop the usb gadget
> >> + * @gadget: The gadget device we want to stop
> >> + *
> >> + * This is external API for use by OTG core.
> >> + *
> >> + * Disconnect from the bus (disable pull) and stop the
> >> + * gadget controller.
> >> + */
> >> +static int usb_gadget_stop(struct usb_gadget *gadget) {
> >> +	struct usb_udc *udc = NULL;
> >> +
> >> +	dev_dbg(&gadget->dev, "%s\n", __func__);
> >> +	mutex_lock(&udc_lock);
> >> +	list_for_each_entry(udc, &udc_list, list)
> >> +		if (udc->gadget == gadget)
> >> +			goto found;
> >> +
> >> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
> >> +		__func__);
> >> +	mutex_unlock(&udc_lock);
> >> +	return -EINVAL;
> >> +
> >> +found:
> >> +	usb_gadget_disconnect(udc->gadget);
> >
> > Likewise, gadget disconnect also should be done by loc_conn() instead
> > of gadget stop.
> >
> >> +	udc->driver->disconnect(udc->gadget);
> >> +	usb_gadget_udc_stop(udc);
> >> +	mutex_unlock(&udc_lock);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >
> > Li Jun
> >

[toc] | [prev] | [next] | [standalone]

#1388918

On 26/04/16 03:07, Jun Li wrote:
> Hi
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:rogerq@ti.com]
>> Sent: Monday, April 25, 2016 10:04 PM
>> To: Jun Li <jun.li@nxp.com>; stern@rowland.harvard.edu; balbi@kernel.org;
>> gregkh@linuxfoundation.org; peter.chen@freescale.com
>> Cc: dan.j.williams@intel.com; jun.li@freescale.com;
>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core
>>
>> Hi,
>>
>> On 21/04/16 09:38, Jun Li wrote:
>>> Hi,
>>>
>>> ...
>>>>
>>>>  /**
>>>> + * usb_gadget_start - start the usb gadget controller and connect to
>>>> +bus
>>>> + * @gadget: the gadget device to start
>>>> + *
>>>> + * This is external API for use by OTG core.
>>>> + *
>>>> + * Start the usb device controller and connect to bus (enable pull).
>>>> + */
>>>> +static int usb_gadget_start(struct usb_gadget *gadget) {
>>>> +	int ret;
>>>> +	struct usb_udc *udc = NULL;
>>>> +
>>>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>>>> +	mutex_lock(&udc_lock);
>>>> +	list_for_each_entry(udc, &udc_list, list)
>>>> +		if (udc->gadget == gadget)
>>>> +			goto found;
>>>> +
>>>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>> +		__func__);
>>>> +	mutex_unlock(&udc_lock);
>>>> +	return -EINVAL;
>>>> +
>>>> +found:
>>>> +	ret = usb_gadget_udc_start(udc);
>>>> +	if (ret)
>>>> +		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
>>>> +			ret);
>>>> +	else
>>>> +		usb_udc_connect_control(udc);
>>>
>>> For drd, it's fine, but for real otg, gadget connect should be done by
>>> loc_conn() instead of gadget start.
>>
>> It is upto the OTG state machine to call gadget_start() when it needs to
>> connect to the bus (i.e. loc_conn()). I see no point in calling gadget
>> start before.
>>
>> Do you see any issue in doing so?
> 
> This is what OTG state machine does:
> 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;
> 
> You intend to abstract something common in this api when start gadget,
> which should be called by otg_set_protocol(fsm, PROTO_GADGET); and
> drd_set_protocol(fsm, PROTO_GADGET); right?
> 
> So you may move usb_udc_connect_control(IMO usb_gadget_connect()
> is better)out of usb_gadget_start(), then for drd:
> 
> case OTG_STATE_B_PERIPHERAL:
>      drd_set_protocol(fsm, PROTO_GADGET);
>      otg_drv_vbus(otg, 0);
>      usb_gadget_connect();

OK. I understand now. I'll implement your suggestion. Thanks.

cheers,
-roger

>>>
>>>> +
>>>> +	mutex_unlock(&udc_lock);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * usb_gadget_stop - disconnect from bus and stop the usb gadget
>>>> + * @gadget: The gadget device we want to stop
>>>> + *
>>>> + * This is external API for use by OTG core.
>>>> + *
>>>> + * Disconnect from the bus (disable pull) and stop the
>>>> + * gadget controller.
>>>> + */
>>>> +static int usb_gadget_stop(struct usb_gadget *gadget) {
>>>> +	struct usb_udc *udc = NULL;
>>>> +
>>>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>>>> +	mutex_lock(&udc_lock);
>>>> +	list_for_each_entry(udc, &udc_list, list)
>>>> +		if (udc->gadget == gadget)
>>>> +			goto found;
>>>> +
>>>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>> +		__func__);
>>>> +	mutex_unlock(&udc_lock);
>>>> +	return -EINVAL;
>>>> +
>>>> +found:
>>>> +	usb_gadget_disconnect(udc->gadget);
>>>
>>> Likewise, gadget disconnect also should be done by loc_conn() instead
>>> of gadget stop.
>>>
>>>> +	udc->driver->disconnect(udc->gadget);
>>>> +	usb_gadget_udc_stop(udc);
>>>> +	mutex_unlock(&udc_lock);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>
>>> Li Jun
>>>

[toc] | [prev] | [next] | [standalone]

#1389949

Hi,

On 27/04/16 14:22, Roger Quadros wrote:
> On 26/04/16 03:07, Jun Li wrote:
>> Hi
>>
>>> -----Original Message-----
>>> From: Roger Quadros [mailto:rogerq@ti.com]
>>> Sent: Monday, April 25, 2016 10:04 PM
>>> To: Jun Li <jun.li@nxp.com>; stern@rowland.harvard.edu; balbi@kernel.org;
>>> gregkh@linuxfoundation.org; peter.chen@freescale.com
>>> Cc: dan.j.williams@intel.com; jun.li@freescale.com;
>>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
>>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core
>>>
>>> Hi,
>>>
>>> On 21/04/16 09:38, Jun Li wrote:
>>>> Hi,
>>>>
>>>> ...
>>>>>
>>>>>  /**
>>>>> + * usb_gadget_start - start the usb gadget controller and connect to
>>>>> +bus
>>>>> + * @gadget: the gadget device to start
>>>>> + *
>>>>> + * This is external API for use by OTG core.
>>>>> + *
>>>>> + * Start the usb device controller and connect to bus (enable pull).
>>>>> + */
>>>>> +static int usb_gadget_start(struct usb_gadget *gadget) {
>>>>> +	int ret;
>>>>> +	struct usb_udc *udc = NULL;
>>>>> +
>>>>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>>>>> +	mutex_lock(&udc_lock);
>>>>> +	list_for_each_entry(udc, &udc_list, list)
>>>>> +		if (udc->gadget == gadget)
>>>>> +			goto found;
>>>>> +
>>>>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>>> +		__func__);
>>>>> +	mutex_unlock(&udc_lock);
>>>>> +	return -EINVAL;
>>>>> +
>>>>> +found:
>>>>> +	ret = usb_gadget_udc_start(udc);
>>>>> +	if (ret)
>>>>> +		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
>>>>> +			ret);
>>>>> +	else
>>>>> +		usb_udc_connect_control(udc);
>>>>
>>>> For drd, it's fine, but for real otg, gadget connect should be done by
>>>> loc_conn() instead of gadget start.
>>>
>>> It is upto the OTG state machine to call gadget_start() when it needs to
>>> connect to the bus (i.e. loc_conn()). I see no point in calling gadget
>>> start before.
>>>
>>> Do you see any issue in doing so?
>>
>> This is what OTG state machine does:
>> 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;

On second thoughts, after seen the OTG state machine.
otg_set_protocol(fsm, PROTO_GADGET); is always followed by otg_loc_conn(otg, 1);
And whenever protocol changes to anything other the PROTO_GADGET, we use
otg_loc_conn(otg, 0);

So otg_loc_conn seems redundant. Can we just get rid of it?

usb_gadget_start() implies that gadget controller starts up and enables pull.
usb_gadget_stop() implies that gadget controller disables pull and stops.


Can you please explain why just these 2 APIs are not sufficient for full OTG?

Do we want anything to happen between gadget controller start/stop and
pull on/off?

cheers,
-roger

>>
>> You intend to abstract something common in this api when start gadget,
>> which should be called by otg_set_protocol(fsm, PROTO_GADGET); and
>> drd_set_protocol(fsm, PROTO_GADGET); right?
>>
>> So you may move usb_udc_connect_control(IMO usb_gadget_connect()
>> is better)out of usb_gadget_start(), then for drd:
>>
>> case OTG_STATE_B_PERIPHERAL:
>>      drd_set_protocol(fsm, PROTO_GADGET);
>>      otg_drv_vbus(otg, 0);
>>      usb_gadget_connect();
> 
> OK. I understand now. I'll implement your suggestion. Thanks.
> 
> cheers,
> -roger
> 
>>>>
>>>>> +
>>>>> +	mutex_unlock(&udc_lock);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * usb_gadget_stop - disconnect from bus and stop the usb gadget
>>>>> + * @gadget: The gadget device we want to stop
>>>>> + *
>>>>> + * This is external API for use by OTG core.
>>>>> + *
>>>>> + * Disconnect from the bus (disable pull) and stop the
>>>>> + * gadget controller.
>>>>> + */
>>>>> +static int usb_gadget_stop(struct usb_gadget *gadget) {
>>>>> +	struct usb_udc *udc = NULL;
>>>>> +
>>>>> +	dev_dbg(&gadget->dev, "%s\n", __func__);
>>>>> +	mutex_lock(&udc_lock);
>>>>> +	list_for_each_entry(udc, &udc_list, list)
>>>>> +		if (udc->gadget == gadget)
>>>>> +			goto found;
>>>>> +
>>>>> +	dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
>>>>> +		__func__);
>>>>> +	mutex_unlock(&udc_lock);
>>>>> +	return -EINVAL;
>>>>> +
>>>>> +found:
>>>>> +	usb_gadget_disconnect(udc->gadget);
>>>>
>>>> Likewise, gadget disconnect also should be done by loc_conn() instead
>>>> of gadget stop.
>>>>
>>>>> +	udc->driver->disconnect(udc->gadget);
>>>>> +	usb_gadget_udc_stop(udc);
>>>>> +	mutex_unlock(&udc_lock);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>
>>>> Li Jun
>>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web