Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751910AbcD1IBu (ORCPT ); Thu, 28 Apr 2016 04:01:50 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:55195 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751112AbcD1IBs (ORCPT ); Thu, 28 Apr 2016 04:01:48 -0400 Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core To: Peter Chen References: <1459865117-7032-1-git-send-email-rogerq@ti.com> <1459865117-7032-8-git-send-email-rogerq@ti.com> <20160426034711.GC2978@peterchendt> <20160426062809.GD2978@peterchendt> <20160426082106.GE2978@peterchendt> <20160427031509.GA22637@shlinux2.ap.freescale.net> <57209BA0.6040508@ti.com> <20160428015409.GA12199@shlinux2.ap.freescale.net> CC: Jun Li , "stern@rowland.harvard.edu" , "balbi@kernel.org" , "gregkh@linuxfoundation.org" , "peter.chen@freescale.com" , "dan.j.williams@intel.com" , "jun.li@freescale.com" , "mathias.nyman@linux.intel.com" , "tony@atomide.com" , "Joao.Pinto@synopsys.com" , "abrestic@chromium.org" , "r.baldyga@samsung.com" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-omap@vger.kernel.org" From: Roger Quadros Message-ID: <5721C34C.8000504@ti.com> Date: Thu, 28 Apr 2016 11:01:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160428015409.GA12199@shlinux2.ap.freescale.net> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5860 Lines: 144 On 28/04/16 04:54, Peter Chen wrote: > 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@gmail.com] >>>>>> Sent: Tuesday, April 26, 2016 2:28 PM >>>>>> To: Jun Li >>>>>> Cc: Roger Quadros ; stern@rowland.harvard.edu; >>>>>> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com; >>>>>> dan.j.williams@intel.com; jun.li@freescale.com; >>>>>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com; >>>>>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org; >>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org >>>>>> 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@gmail.com] >>>>>>>> Sent: Tuesday, April 26, 2016 11:47 AM >>>>>>>> To: Jun Li >>>>>>>> Cc: Roger Quadros ; stern@rowland.harvard.edu; >>>>>>>> balbi@kernel.org; gregkh@linuxfoundation.org; >>>>>>>> peter.chen@freescale.com; dan.j.williams@intel.com; >>>>>>>> jun.li@freescale.com; mathias.nyman@linux.intel.com; >>>>>>>> tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org; >>>>>>>> r.baldyga@samsung.com; linux-usb@vger.kernel.org; >>>>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org >>>>>>>> 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:) > Agreed. Until one OTG implementation is tested we can't really say it works :). cheers, -roger