Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

[Peter Chen]

On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
> Hi,
> 
> On 27/04/16 06:15, Peter Chen wrote:
> > On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
> >> On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: Peter Chen [mailto:hzpeterchen@xxxxxxxxx]
> >>>> Sent: Tuesday, April 26, 2016 2:28 PM
> >>>> To: Jun Li <jun.li@xxxxxxx>
> >>>> Cc: Roger Quadros <rogerq@xxxxxx>; 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 07/12] usb: otg: add OTG/dual-role core
> >>>>
> >>>> On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> >>>>> Hi
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Peter Chen [mailto:hzpeterchen@xxxxxxxxx]
> >>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
> >>>>>> To: Jun Li <jun.li@xxxxxxx>
> >>>>>> Cc: Roger Quadros <rogerq@xxxxxx>; 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 07/12] usb: otg: add OTG/dual-role core
> >>>>>>
> >>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> >>>>>>>> +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;
> >>>>>>>> +
> >>>>>>>> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> >>>>>>>> +	     otg->caps->adp_support) && !config->otg_work)
> >>>>>>>> +		dev_info(dev, "otg: limiting to dual-role\n");
> >>>>>>>
> >>>>>>> dev_err, this should be an error.
> >>>>>>
> >>>>>> The condition may be wrong, but it is an information to show that
> >>>>>> current OTG is dual-role.
> >>>>>
> >>>>> This should not happen in any correct design, I even doubt if we
> >>>>> should try to continue by "downgrade" it to be duel role, currently
> >>>>> the only example user is dual role, so doing like this can't be tested
> >>>>> by real case, this downgrade is not so easy like we image, at least
> >>>>> for chipidea otg driver, simply replace a queue worker may not work,
> >>>>> as we have much more difference between the 2 configs.
> >>>>>
> >>>>
> >>>> Would you show more why chipidea can't work just replace the work item,
> >>>> and see if anything we still can improve for this framework?
> >>>
> >>> In real OTG, we need enable AVV irq,
> >>
> >> Enable and Handling AVV is platform stuff. In this framework, we are
> >> focus on how otg device manages host and gadget together, and the state
> >> machine when the related otg event occurs.
> >>
> >>> but for duel role, nobody care/handle,
> >>> there are much more resource required for OTG: timers, hnp polling,
> >>> otg test device handling... 
> >>
> >> They are common things for fully OTG fsm, you can move them
> >> to common code (In fact, hnp polling handling is already common code).
> >>
> >>>
> >>> with current design, chipidea driver can support real OTG with its own
> >>> queue worker, or DRD with Roger's drd work item if config is correct.
> >>>
> >>> But improve something to work on a *wrong* config will make it complicated
> >>> and does not make much sense IMO.
> >>>
> >>
> >> What does above "config" you mean?
> >>
> >> If the configure is fully OTG, you can choose different state machine,
> >> eg otg_statemachine, if you find it is hard for chipidea to use this
> >> framework, just list the reason, and see if we can improve.
> >>
> > 
> > Roger, after discussing with Jun off line, we think usb_otg_register
> > should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
> > ADP), since this patch set does not cover fully otg features, the users
> 
> But this series isn't preventing full otg implementation. You can
> still do that via config->otg_work.
> 
> I can modify the following condition to return -ENOTSUPP instead of
> defaulting to dual-role
> 
> struct usb_otg *usb_otg_register(...)
> {
> ...
>         if ((otg->caps->hnp_support || otg->caps->srp_support ||
>              otg->caps->adp_support) && !config->otg_work) {
> 		dev_err(dev, "otg: otg_work must be provided for OTG support\n");
> 		return -ENOTSUPP;
> 	}
> ...
> }
> 

According to Jun, this framework still needs to improve (maybe small) even
using user work item for fully OTG, so the first step is let the drd
work well:)

-- 

Best Regards,
Peter Chen