RE: [PATCH v8 08/14] usb: otg: add OTG/dual-role core

[Jun Li]

> -----Original Message-----
> From: Peter Chen [mailto:hzpeterchen@xxxxxxxxx]
> Sent: Wednesday, May 25, 2016 10:44 AM
> To: Roger Quadros <rogerq@xxxxxx>
> Cc: peter.chen@xxxxxxxxxxxxx; balbi@xxxxxxxxxx; tony@xxxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; dan.j.williams@xxxxxxxxx;
> mathias.nyman@xxxxxxxxxxxxxxx; Joao.Pinto@xxxxxxxxxxxx;
> sergei.shtylyov@xxxxxxxxxxxxxxxxxx; jun.li@xxxxxxxxxxxxx;
> grygorii.strashko@xxxxxx; yoshihiro.shimoda.uh@xxxxxxxxxxx;
> robh@xxxxxxxxxx; nsekhar@xxxxxx; b-liu@xxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> linux-omap@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8 08/14] usb: otg: add OTG/dual-role core
> 
> On Tue, May 24, 2016 at 12:45:46PM +0300, Roger Quadros wrote:
> > Hi Peter,
> >
> > I have one question here. Please see below.
> >
> > On 13/05/16 13:03, Roger Quadros wrote:
> > > It provides APIs for the following tasks
> > >
> > > - Registering an OTG/dual-role capable controller
> > > - Registering Host and Gadget controllers to OTG core
> > > - Providing inputs to and kicking the OTG state machine
> > >
> > > Provide a dual-role device (DRD) state machine.
> > > DRD mode is a reduced functionality OTG mode. In this mode we don't
> > > support SRP, HNP and dynamic role-swap.
> > >
> > > In DRD operation, the controller mode (Host or Peripheral) is
> > > decided based on the ID pin status. Once a cable plug (Type-A or
> > > Type-B) is attached the controller selects the state and doesn't
> > > change till the cable in unplugged and a different cable type is
> > > inserted.
> > >
> > > As we don't need most of the complex OTG states and OTG timers we
> > > implement a lean DRD state machine in usb-otg.c.
> > > The DRD state machine is only interested in 2 hardware inputs 'id'
> > > and 'b_sess_vld'.
> > >
> > > Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> > > ---
> > >  drivers/usb/common/Makefile  |    2 +-
> > >  drivers/usb/common/usb-otg.c | 1042
> ++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/usb/core/Kconfig     |    4 +-
> > >  include/linux/usb/gadget.h   |    2 +
> > >  include/linux/usb/hcd.h      |    1 +
> > >  include/linux/usb/otg-fsm.h  |    7 +
> > >  include/linux/usb/otg.h      |  154 ++++++-
> > >  7 files changed, 1206 insertions(+), 6 deletions(-)  create mode
> > > 100644 drivers/usb/common/usb-otg.c
> > >
> >
> > <snip>
> >
> > > +
> > > +/**
> > > + * usb_otg_register() - Register the OTG/dual-role device to OTG
> > > +core
> > > + * @dev: OTG/dual-role controller device.
> > > + * @config: OTG configuration.
> > > + *
> > > + * Registers the OTG/dual-role controller device with the USB OTG
> core.
> > > + *
> > > + * Return: struct usb_otg * if success, ERR_PTR() if error.
> > > + */
> > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > +				 struct usb_otg_config *config) {
> > > +	struct usb_otg *otg;
> > > +	struct otg_wait_data *wait;
> > > +	int ret = 0;
> > > +
> > > +	if (!dev || !config || !config->fsm_ops)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	/* already in list? */
> > > +	mutex_lock(&otg_list_mutex);
> > > +	if (usb_otg_get_data(dev)) {
> > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > +			__func__);
> > > +		ret = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	/* allocate and add to list */
> > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > +	if (!otg) {
> > > +		ret = -ENOMEM;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	otg->dev = dev;
> > > +	otg->caps = config->otg_caps;
> >
> > Here, we should be checking if user needs to disable any OTG features.
> > So,
> >
> > 	if (dev->of_node)
> > 		of_usb_update_otg_caps(dev->of_node, &otg->caps);
> >
> > Do you agree?
> > This means we need to change otg->caps from 'struct usb_otg_caps *caps;'
> > to 'struct usb_otg_caps caps;' so that we can modify the local copy
> > instead of the one passed by the OTG controller.
> 
> Why can't modify the one from OTG controller directly?

Yes, if user wants to disable any OTG features, this should have been
done in 'config->otg_caps', if not, 'config->otg_caps' from controller
driver is invalid and making no sense.

> 
> >
> > We can also move of_usb_update_otg_caps() to otg.h.
> >
> > We will also need to modify the udc-core code so that it sets
> > gadget->otg_caps to the modified otg_caps from the OTG core. This will
> > ensure that the right OTG descriptors are sent.
> >
> > So we will have to introduce an API.
> >
> > struct usb_otg_caps *usb_otg_get_otg_caps(struct device *otg_dev)
> >
> > And in udc-core.c,
> >
> > static int udc_bind_to_driver(struct usb_udc *udc, struct
> > usb_gadget_driver *driver) { ..
> >         ret = driver->bind(udc->gadget, driver);
> >         if (ret)
> >                 goto err1;
> >
> >         /* If OTG, the otg core starts the UDC when needed */
> >         if (udc->gadget->otg_dev) {
> > +		udc->gadget->is_otg = true;
> 
> gadget->is_otg is only set to true if fully OTG is supported and it
> needs to send OTG descriptors at this case. DRD devices should not send
> OTG descriptors.
> 
> > +		udc->gadget->otg_caps = usb_otg_get_otg_caps(udc->gadget-
> >otg_dev);
> 
> Getting otg capabilities should be prior to driver->bind since
> usb_otg_descriptor_init is called at that. Besides, Gadget driver may be
> probed before otg driver is registered
> 
> I am wonder if we can implement defer probe for gadget/udc/host driver if
> otg driver is not probed, in that case, some designs can be simpler like
> wait list in otg driver.

I even don't see much benefit of this kind of random order of OTG/HCD/GADGET
registration, anyway OTG register can be done firstly.
HCD and GADGET are both separated drivers, but OTG is newly added
and what we need is just a registration in controller driver.
  
> 
> >                 mutex_unlock(&udc_lock);
> >                 usb_otg_register_gadget(udc->gadget, &otg_gadget_intf);
> >                 mutex_lock(&udc_lock);
> >         } else {
> >                 ret = usb_gadget_udc_start(udc);
> >                 if (ret) {
> >                         driver->unbind(udc->gadget);
> >                         goto err1;
> >                 }
> >                 usb_udc_connect_control(udc);
> >         }
> > ..
> > }
> >
> > What do you say?
> >
> > regards,
> > -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
> 
> --
> 
> Best Regards,
> Peter Chen