linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
Cc: Jun Li <jun.li-3arQi8VN3Tc@public.gmane.org>,
	"stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org"
	<stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>,
	"balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org"
	<balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	"gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org"
	<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>,
	"peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org"
	<peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>,
	"dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org"
	<dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	"jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org"
	<jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org>,
	"mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org"
	<mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>,
	"tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org"
	<tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>,
	"Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org"
	<Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>,
	"abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org"
	<abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
	"r.baldyga-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org"
	<r.baldyga-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>,
	"linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core
Date: Wed, 4 May 2016 09:47:05 +0800	[thread overview]
Message-ID: <20160504014705.GC31013@shlinux2.ap.freescale.net> (raw)
In-Reply-To: <5728C76E.9010405-l0cyMroinI0@public.gmane.org>

On Tue, May 03, 2016 at 06:44:46PM +0300, Roger Quadros wrote:
> Hi,
> 
> On 03/05/16 10:06, 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?
> >>>
> >>> "loc_conn" is a standard output parameter in OTG spec, it deserves a
> >>> separate api, yes, current implementation of OTG state machine code
> >>> seems allow you to combine the 2 things into one, but don't do that,
> >>> because they do not always happen together, e.g. for peripheral only B
> >>> device (also a part OTG spec: section 7.3), will be fixed in gadget
> >>> mode, but it will do gadget connect and disconnect in its diff states,
> >>> so, to make the framework common, let's keep them separated.
> >>
> >> I'm sorry but I didn't understand your comment about "it will do gadget
> >> connect and disconnect in its diff states"
> > 
> > Gadget connect means loc_conn(1).
> > 
> >>
> >> I am reading the OTG v2.0 specification and loc_conn is always true when
> >> b_peripheral or a_peripheral is true and false otherwise.
> > 
> > If you only talk about these 2 states, yes, loc_conn is ture.
> > 
> >>
> >> loc_conn is just an internal state variable and it corresponds to our
> >> gadget_start/stop() state.
> > 
> > It's not an internal variable, there are OTG state machine
> > parameters tables(table 7-x) in OTG spec which have clear lists
> > which are "internal variable", which are "input", which are "output"...
> > 
> > Those APIs are driven directly from OTG spec, easily understood so
> > code reader can know what's those APIs for. For real OTG, I don't
> > see the benefit if get rid of it.
> 
> OK, no issues if we don't get rid of it. But I am still in favor of
> doing a connect in usb_gadget_start(), because
> 
> 1) If we split connect/disconnect() and usb_gadget_start/stop() then there is
> additional overhead of keeping track whether connect was called or not during
> usb_gadget_stop(). Plus we need to take care that users don't call connect/disconnect
> outside of start/stop. It is just complicating things.
> 
> 2) for many controllers there is no difference between run/stop and
> connect/disconnect. i.e. a single register bit controls both.
> 
> 3) it fits well with the OTG specification. OTG specification says
> that loc_conn *variable* must be true *after* the device has signalled a connect.
> So OTG state machine can safely set loc_conn variable to true after doing
> otg_set_protocol(fsm, PROTO_GADGET); and set it to false otherwise.
> 
> Note, OTG specification does not say to take any action based on loc_conn.
> It is just a connect indicator variable. So we might have to fix this in the
> OTG state machine.
> 
> My suggestion is to keep it simple for now. Try the OTG implementation,
> and later if we find issues then extend it as required.
> 

I agree with you, roger.

loc_conn action is only needed for peripheral, we can't do real !loc_conn
at host mode. Besides, loc_conn is the output, we need to set/clear it after
state has changed, and current gadget framework already takes well for
connection and disconnection state, so it is better to delete loc_conn
action at otg_fsm->ops, and only keeps it as indicator for OTG FSM
reference.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  parent reply	other threads:[~2016-05-04  1:47 UTC|newest]

Thread overview: 100+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-05 14:05 [PATCH v6 00/12] USB OTG/dual-role framework Roger Quadros
2016-04-05 14:05 ` [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros
2016-04-06  6:09   ` Felipe Balbi
     [not found]     ` <87zit72rqz.fsf-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2016-04-06  6:32       ` Roger Quadros
2016-04-07  9:42         ` Peter Chen
2016-04-07 10:40           ` Roger Quadros
     [not found]             ` <57063915.7000700-l0cyMroinI0@public.gmane.org>
2016-04-08  1:01               ` Peter Chen
2016-04-08  7:16                 ` Roger Quadros
     [not found]                   ` <57075ACE.1010702-l0cyMroinI0@public.gmane.org>
2016-04-08  7:45                     ` Peter Chen
2016-04-18  2:29         ` Peter Chen
2016-04-18 14:11           ` Alan Stern
     [not found]             ` <Pine.LNX.4.44L0.1604181007240.1775-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2016-04-19  1:56               ` Peter Chen
2016-04-20  8:15                 ` Roger Quadros
2016-04-20  9:40                   ` Peter Chen
2016-04-05 14:05 ` [PATCH v6 02/12] usb: hcd.h: Add OTG to HCD interface Roger Quadros
2016-04-18  7:41   ` Peter Chen
2016-04-05 14:05 ` [PATCH v6 03/12] usb: otg-fsm: use usb_otg wherever possible Roger Quadros
2016-04-18  7:42   ` Peter Chen
2016-04-05 14:05 ` [PATCH v6 04/12] usb: otg-fsm: move host controller operations into usb_otg->hcd_ops Roger Quadros
     [not found]   ` <1459865117-7032-5-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
2016-04-18  8:00     ` Peter Chen
2016-04-05 14:05 ` [PATCH v6 05/12] usb: gadget.h: Add OTG to gadget interface Roger Quadros
2016-04-05 14:05 ` [PATCH v6 06/12] usb: otg: get rid of CONFIG_USB_OTG_FSM in favour of CONFIG_USB_OTG Roger Quadros
2016-04-18  8:05   ` Peter Chen
     [not found]     ` <20160418080514.GG4477-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-20  8:12       ` Roger Quadros
2016-04-05 14:05 ` [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros
2016-04-07  8:52   ` Yoshihiro Shimoda
2016-04-07 11:45     ` Roger Quadros
2016-04-08 11:22       ` Yoshihiro Shimoda
2016-04-11 10:54         ` Roger Quadros
2016-04-14  8:36           ` Yoshihiro Shimoda
2016-04-14 10:59             ` Roger Quadros
     [not found]               ` <570F7827.8050707-l0cyMroinI0@public.gmane.org>
2016-04-14 11:15                 ` Yoshihiro Shimoda
     [not found]                   ` <SG2PR06MB09195754753D7FDDAB05D3FDD8970-ESzmfEwOt/zNQ8RBPPB5A20DtJ1/0DrXvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-04-14 11:32                     ` Roger Quadros
     [not found]                       ` <570F7FB3.2040807-l0cyMroinI0@public.gmane.org>
2016-04-15  9:59                         ` Yoshihiro Shimoda
     [not found]                           ` <SG2PR06MB09195F411F6BBE840F7A22D2D8680-ESzmfEwOt/zNQ8RBPPB5A20DtJ1/0DrXvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-04-15 10:57                             ` Roger Quadros
2016-04-15 10:03                       ` Yoshihiro Shimoda
2016-04-19  9:18                         ` Peter Chen
2016-04-20  5:08                           ` Yoshihiro Shimoda
2016-04-20  7:03                             ` Roger Quadros
     [not found]                               ` <571729C6.5000200-l0cyMroinI0@public.gmane.org>
2016-04-22  6:05                                 ` Peter Chen
2016-04-22  1:26                         ` Peter Chen
     [not found]                           ` <20160422012646.GA29299-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-22  3:34                             ` Peter Chen
     [not found]                               ` <20160422033414.GB29299-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-22  5:57                                 ` Yoshihiro Shimoda
     [not found]   ` <1459865117-7032-8-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
2016-04-15  9:25     ` Peter Chen
2016-04-15 11:00       ` Roger Quadros
2016-04-18  2:09         ` Peter Chen
2016-04-20  6:54           ` Roger Quadros
2016-04-20  9:26             ` Peter Chen
2016-04-21  6:52     ` Peter Chen
2016-04-25 14:05       ` Roger Quadros
2016-04-19  8:06   ` Peter Chen
     [not found]     ` <20160419080649.GJ4477-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-20  7:02       ` Roger Quadros
2016-04-20  9:39         ` Peter Chen
2016-04-26  2:07   ` Jun Li
     [not found]     ` <AM4PR04MB213045DC3D1C1D67A7D9AF0689630-WOempg8NbQQzjTQnahXoOs9NdZoXdze2vxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-04-26  3:47       ` Peter Chen
2016-04-26  5:11         ` Jun Li
     [not found]           ` <AM4PR04MB2130124412E09799A2CB01BF89630-WOempg8NbQQzjTQnahXoOs9NdZoXdze2vxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-04-26  6:28             ` Peter Chen
2016-04-26  7:00               ` Jun Li
2016-04-26  8:21                 ` Peter Chen
2016-04-27  3:15                   ` Peter Chen
     [not found]                     ` <20160427031509.GA22637-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-27 10:59                       ` Roger Quadros
     [not found]                         ` <57209BA0.6040508-l0cyMroinI0@public.gmane.org>
2016-04-28  1:54                           ` Peter Chen
     [not found]                             ` <20160428015409.GA12199-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-28  8:01                               ` Roger Quadros
2016-04-27 11:15     ` Roger Quadros
2016-04-05 14:05 ` [PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros
2016-04-18  6:29   ` Peter Chen
2016-04-19  8:14     ` Peter Chen
     [not found]       ` <20160419081446.GA15789-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-20  6:47         ` Roger Quadros
     [not found]     ` <20160418062937.GA4477-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-20  6:46       ` Roger Quadros
2016-04-27 10:16   ` Jun Li
2016-04-27 11:00     ` Roger Quadros
2016-04-27 11:11       ` Roger Quadros
2016-04-27 12:49         ` Jun Li
2016-04-27 13:18           ` Jun Li
2016-04-05 14:05 ` [PATCH v6 09/12] usb: gadget: udc: adapt " Roger Quadros
     [not found]   ` <1459865117-7032-10-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
2016-04-18  6:59     ` Peter Chen
     [not found]       ` <20160418065954.GB4477-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2016-04-20  6:51         ` Roger Quadros
2016-04-21  6:38   ` Jun Li
2016-04-25 14:04     ` Roger Quadros
2016-04-26  0:07       ` Jun Li
     [not found]         ` <AM4PR04MB2130B0F024C0B275514020DE89630-WOempg8NbQQzjTQnahXoOs9NdZoXdze2vxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-04-27 11:22           ` Roger Quadros
     [not found]             ` <5720A106.1030702-l0cyMroinI0@public.gmane.org>
2016-04-28  9:54               ` Roger Quadros
2016-04-28 10:23                 ` Jun Li
2016-04-28 12:22                   ` Roger Quadros
2016-05-03  7:06                     ` Jun Li
2016-05-03 15:44                       ` Roger Quadros
     [not found]                         ` <5728C76E.9010405-l0cyMroinI0@public.gmane.org>
2016-05-04  1:47                           ` Peter Chen [this message]
2016-05-04  3:35                         ` Peter Chen
2016-05-04  6:37                           ` Roger Quadros
2016-05-04  7:53                             ` Peter Chen
2016-05-04  8:03                             ` Jun Li
     [not found]                               ` <AM4PR04MB2130076A1E77301D143C9CAE897B0-WOempg8NbQQzjTQnahXoOs9NdZoXdze2vxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-05-04  8:40                                 ` Roger Quadros
2016-05-04  8:39                                   ` Peter Chen
2016-04-05 14:05 ` [PATCH v6 10/12] usb: doc: dt-binding: Add otg-controller property Roger Quadros
2016-04-05 14:05 ` [PATCH v6 11/12] usb: core: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros
2016-04-18  7:08   ` Peter Chen
2016-04-27 14:35     ` Roger Quadros
     [not found] ` <1459865117-7032-1-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
2016-04-05 14:05   ` [PATCH v6 12/12] usb: host: xhci-plat: Add otg device to platform data Roger Quadros
     [not found]     ` <1459865117-7032-13-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
2016-04-06  3:23       ` Yoshihiro Shimoda
     [not found]         ` <SG2PR06MB09197D03470D6E6083270343D89F0-ESzmfEwOt/zNQ8RBPPB5A20DtJ1/0DrXvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-04-06  6:30           ` Roger Quadros

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160504014705.GC31013@shlinux2.ap.freescale.net \
    --to=hzpeterchen-re5jqeeqqe8avxtiumwx3w@public.gmane.org \
    --cc=Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org \
    --cc=abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org \
    --cc=jun.li-3arQi8VN3Tc@public.gmane.org \
    --cc=jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org \
    --cc=peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org \
    --cc=r.baldyga-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=rogerq-l0cyMroinI0@public.gmane.org \
    --cc=stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org \
    --cc=tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).