Hi > -----Original Message----- > From: Roger Quadros [mailto:rogerq@xxxxxx] > Sent: Wednesday, May 04, 2016 2:37 PM > To: Peter Chen <hzpeterchen@xxxxxxxxx> > Cc: Jun Li <jun.li@xxxxxxx>; stern@xxxxxxxxxxxxxxxxxxx; balbi@xxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; peter.chen@xxxxxxxxxxxxx; > dan.j.williams@xxxxxxxxx; jun.li@xxxxxxxxxxxxx; > mathias.nyman@xxxxxxxxxxxxxxx; tony@xxxxxxxxxxx; Joao.Pinto@xxxxxxxxxxxx; > abrestic@xxxxxxxxxxxx; r.baldyga@xxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core > > Peter, > > On 04/05/16 06:35, Peter Chen wrote: > > 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. > >> > > > > Just talked with Jun, he is worried if loc_conn != pullup_dp at some > > situations. So, how about only calling start gadget at > > usb_start_gadget, > > Which situations? When to pull-up DP is decided by application (while vbus is on), not only by driver state machine. > > > and pullup_dp at drd_set_state (see below). > > > > > > static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state > > new_state) { > > struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm); > > > > if (otg->state == new_state) > > return; > > > > fsm->state_changed = 1; > > dev_dbg(otg->dev, "otg: set state: %s\n", > > usb_otg_state_string(new_state)); > > switch (new_state) { > > case OTG_STATE_B_IDLE: > > + usb_udc_vbus_handler(gadget, false); > > This is redundant, When we switch from PROTO_GADGET to PROTO_UNDEF we do a > usb_gadget_stop(), and a usb_gadget_disconnect() is done there. > > > drd_set_protocol(fsm, PROTO_UNDEF); > > otg_drv_vbus(otg, 0); > > break; > > case OTG_STATE_B_PERIPHERAL: > > drd_set_protocol(fsm, PROTO_GADGET); > > + usb_udc_vbus_handler(gadget, true); > > This is redundant as well since usb_gadget_start() is doing a > usb_gadget_connect(). > > > otg_drv_vbus(otg, 0); > > break; > > ...... > > }; > > > > } > > > > When the OTG FSM is added to this framework, it can keep > > usb_fsm->ops->loc_conn, and using the current FSM. > > > > I have no strong opinions for or against usb_fsm->ops->loc_conn. > Although strictly speaking, we shouldn't take any action based on loc_conn. Strictly speaking(OTG spec), all you does is for loc_conn, but you think it's start_gadget. > It is just a state variable indicator. Nobody check this "indicator". Of cos, this is not a big deal, you can define the new API as is, do udc_start() + gadget_connect() in one shot, it's up to user to decide if use your usb_otg_start_gadget(), in case of udc_start() followed by gadget_connect() is not wanted, user can/need do udc_start() and something else before do gadget_connect. > > cheers, > -roger -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
