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

[Jun Li]

Hi

> -----Original Message-----
> From: Roger Quadros [mailto:rogerq@xxxxxx]
> Sent: Thursday, April 28, 2016 5:55 PM
> To: Jun Li <jun.li@xxxxxxx>; stern@xxxxxxxxxxxxxxxxxxx; balbi@xxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; peter.chen@xxxxxxxxxxxxx
> Cc: 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
> 
> 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@xxxxxx]
> >>> Sent: Monday, April 25, 2016 10:04 PM
> >>> To: Jun Li <jun.li@xxxxxxx>; stern@xxxxxxxxxxxxxxxxxxx;
> >>> balbi@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> >>> peter.chen@xxxxxxxxxxxxx
> >>> Cc: 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
> >>>
> >>> 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?

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

Li Jun 
    
> 
> 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@xxxxxxxxxxxxxxx More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> >