On 04/05/16 11:03, Jun Li wrote: > 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. So when OTG state is B_PERIPHERAL or A_PERIPHERAL we still want DP pull-up to be disabled? > >> >>> 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. Please don't get me wrong. I want the full OTG state machine to be able to use usb_otg_start/stop_gadget(). Just trying to understand the real problem. 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
