From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756425AbcEYDwd (ORCPT ); Tue, 24 May 2016 23:52:33 -0400 Received: from mail-db3on0095.outbound.protection.outlook.com ([157.55.234.95]:27064 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756398AbcEYDw2 convert rfc822-to-8bit (ORCPT ); Tue, 24 May 2016 23:52:28 -0400 From: Jun Li To: Peter Chen , Roger Quadros CC: "peter.chen@freescale.com" , "balbi@kernel.org" , "tony@atomide.com" , "gregkh@linuxfoundation.org" , "dan.j.williams@intel.com" , "mathias.nyman@linux.intel.com" , "Joao.Pinto@synopsys.com" , "sergei.shtylyov@cogentembedded.com" , "jun.li@freescale.com" , "grygorii.strashko@ti.com" , "yoshihiro.shimoda.uh@renesas.com" , "robh@kernel.org" , "nsekhar@ti.com" , "b-liu@ti.com" , "linux-usb@vger.kernel.org" , "linux-omap@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: RE: [PATCH v8 08/14] usb: otg: add OTG/dual-role core Thread-Topic: [PATCH v8 08/14] usb: otg: add OTG/dual-role core Thread-Index: AQHRrP9sqfg6l/ahB0Gor+Wq8+BtSZ/H6BgAgAEcnoCAAAZAkA== Date: Wed, 25 May 2016 03:19:30 +0000 Message-ID: References: <1463133808-10630-1-git-send-email-rogerq@ti.com> <1463133808-10630-9-git-send-email-rogerq@ti.com> <574422CA.8080002@ti.com> <20160525024427.GA31142@shlinux2> In-Reply-To: <20160525024427.GA31142@shlinux2> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=nxp.com; x-originating-ip: [192.158.241.86] x-ms-office365-filtering-correlation-id: f81562a7-5988-47e2-925c-08d3844b62d6 x-microsoft-exchange-diagnostics: 1;AM4PR04MB2131;5:1IZGA+P2L8muT7TD3pc7ueCm9ULfmJCj5zDl1/ByAz9g0xrrCmY5VZlZzvKUDh6PvFJXCzVguN8fAfZPb5Cueiw+dUs2ptgLyBJwndnCrNY/mMxE3BRo0X8fRGvWVeW9PFeDyFpNCrFbX7240XJfyg==;24:mBC7jw/A6YuzNDZWJbj6HLCTj0yMqK614kd8vNr7OqrmAl3MeYkOVZbByjBOB3P3nB4glfFdaip2AZMHNS9C8BpK45LS3TajKVyJt6Ad3xY=;7:RIomvNRteDjCPTq6B2P0Rj1Q/4Fy1kOvOW4oue2U8oan7nw1gWVKDsa8THz1x+h7ai+S+14mhEDsteLSMWfiXS/ijskQ5IWhiN4slvWZK2KVzottgfxwJjqa6vZ9rfMpREQjffUoj8iY0GUmu8j4vuOKrD8LOuX+S++Nlq55ifche3ZDaAE+eDbZvePuFqGT x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM4PR04MB2131; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6055026);SRVR:AM4PR04MB2131;BCL:0;PCL:0;RULEID:;SRVR:AM4PR04MB2131; x-forefront-prvs: 09538D3531 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(377454003)(13464003)(24454002)(5001770100001)(2950100001)(2900100001)(106116001)(81166006)(5002640100001)(50986999)(87936001)(77096005)(54356999)(122556002)(5008740100001)(92566002)(5004730100002)(76176999)(15975445007)(33656002)(8936002)(93886004)(76576001)(19580395003)(5003600100002)(19580405001)(10400500002)(9686002)(11100500001)(8666003)(74316001)(8676002)(5890100001)(102836003)(86362001)(6116002)(3846002)(3280700002)(1220700001)(3660700001)(2906002)(4326007)(586003)(189998001)(66066001)(7059030);DIR:OUT;SFP:1101;SCL:1;SRVR:AM4PR04MB2131;H:AM4PR04MB2130.eurprd04.prod.outlook.com;FPR:;SPF:None;MLV:sfv;LANG:en; spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 25 May 2016 03:19:30.4233 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR04MB2131 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Peter Chen [mailto:hzpeterchen@gmail.com] > Sent: Wednesday, May 25, 2016 10:44 AM > To: Roger Quadros > Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > gregkh@linuxfoundation.org; dan.j.williams@intel.com; > mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; > linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > 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 > > > --- > > > 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 > > > > > > > > > > > > + > > > +/** > > > + * 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@vger.kernel.org More majordomo > > info at http://vger.kernel.org/majordomo-info.html > > -- > > Best Regards, > Peter Chen From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jun Li Subject: RE: [PATCH v8 08/14] usb: otg: add OTG/dual-role core Date: Wed, 25 May 2016 03:19:30 +0000 Message-ID: References: <1463133808-10630-1-git-send-email-rogerq@ti.com> <1463133808-10630-9-git-send-email-rogerq@ti.com> <574422CA.8080002@ti.com> <20160525024427.GA31142@shlinux2> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <20160525024427.GA31142@shlinux2> Content-Language: en-US Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Peter Chen , Roger Quadros Cc: "peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org" , "balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org" , "tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org" , "gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org" , "dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" , "mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org" , "Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org" , "sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org" , "jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org" , "grygorii.strashko-l0cyMroinI0@public.gmane.org" , "yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org" , "robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org" , "nsekhar-l0cyMroinI0@public.gmane.org" , "b-liu-l0cyMroinI0@public.gmane.org" , "linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: devicetree@vger.kernel.org > -----Original Message----- > From: Peter Chen [mailto:hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] > Sent: Wednesday, May 25, 2016 10:44 AM > To: Roger Quadros > Cc: peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org; balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org; > gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org; dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org; > mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org; Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org; > sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org; jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org; > grygorii.strashko-l0cyMroinI0@public.gmane.org; yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org; > robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; nsekhar-l0cyMroinI0@public.gmane.org; b-liu-l0cyMroinI0@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; > linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; > devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > 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 > > > --- > > > 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 > > > > > > > > > > > > + > > > +/** > > > + * 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo > > info at http://vger.kernel.org/majordomo-info.html > > -- > > Best Regards, > Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html