From: Peter Chen <hzpeterchen@gmail.com> To: Roger Quadros <rogerq@ti.com> Cc: Felipe Balbi <balbi@kernel.org>, stern@rowland.harvard.edu, 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, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org Subject: Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Date: Fri, 8 Apr 2016 15:45:40 +0800 [thread overview] Message-ID: <20160408074540.GB9070@shlinux2.ap.freescale.net> (raw) In-Reply-To: <57075ACE.1010702@ti.com> On Fri, Apr 08, 2016 at 10:16:30AM +0300, Roger Quadros wrote: > On 08/04/16 04:01, Peter Chen wrote: > > On Thu, Apr 07, 2016 at 01:40:21PM +0300, Roger Quadros wrote: > >> On 07/04/16 12:42, Peter Chen wrote: > >>> On Wed, Apr 06, 2016 at 09:32:22AM +0300, Roger Quadros wrote: > >>>> On 06/04/16 09:09, Felipe Balbi wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> Roger Quadros <rogerq@ti.com> writes: > >>>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > >>>>>> index 2ca2cef..6b1930d 100644 > >>>>>> --- a/drivers/usb/core/hcd.c > >>>>>> +++ b/drivers/usb/core/hcd.c > >>>>>> @@ -2706,6 +2706,7 @@ int usb_add_hcd(struct usb_hcd *hcd, > >>>>>> int retval; > >>>>>> struct usb_device *rhdev; > >>>>>> > >>>>>> + hcd->flags = 0; > >>>>> > >>> > >>> I am not sure if this usb_add(remove)_hcd pair is safe and clean enough > >>> for start/stop host role. From my point, we may need to do like > >>> .probe/.remove host platform driver interface. In that case, we can make > >> > >> probe and remove are meant to be called from bus layer. > >> I do not see a way how OTG framework can call probe/remove of HCD driver. > >> Some HCDs may be platform devices, some PCI, so different entities are calling > >> the HCD .probe hook. > >> > >>> sure the clocks and regulators are off, and hcd will be zero-initialized > >> > >> why can't we make that sure that is taken care of within the hcd_ops? > >> Why should some driver keep its regulators and clocks enabled when hcd is stopped? > >> It doesn't need to. If it is doing so now, it needs to be fixed. > >> > > > > Well, you may misunderstand me. I mean your hcd_ops->start or ->stop > > is hard to be a general one which only calls usb_hcd_add or > > usb_hcd_remove. It needs to implement like .probe or .remove at platform > > driver, some example code like host_start and host_stop at > > drivers/usb/chipidea/host.c. > > > The only extra thing the host_start/stop() of that driver is doing is > enabling/disabling the VBUS regulator. > In the OTG/dual role scope VBUS regulator handling has to be done via the > OTG driver using the otg ops otg_drv_vbus() and not at HCD level. Do you agree? > I agree with you that vbus handling should be handled by otg_drv_vbus for OTG case. > I don't want to complicate the OTG to HCD interface by adding new hooks there. > If HCD driver wants to do something special for OTG case it can always do that > within the struct hc_driver interface. But ideally OTG specific handling must > be done in the OTG driver. All that the HCD driver should care about is making sure > all used resources are disabled once usb_remove_hcd() is called. > In this patch, we set hcd->flag as 0 to fix one problem, but if any problems are still existed due to non-zero hcd structure. We have tested a lot for insmod/rmmod procedure for host driver, it doesn't show many problems. In a word, once the host role is stopped do we need to remove all things belong to host, and re-initialized all things belong to host when the host role is re-started? From my point is 'yes'. host_stop() { usb_remove_hcd(hcd); usb_put_hcd(hcd); platform_deinit(); /* clock, gpio, etc */ } host_start() { platform_init(); /* clock, gpio, etc */ usb_create_hcd(); ... /* initialize hcd structure from platform information */ usb_add_hcd(hcd); } Maybe you have already done something, but there is no user now. Things will be more clear after your post your dwc3 changes :) -- Best Regards, Peter Chen
WARNING: multiple messages have this Message-ID (diff)
From: Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> Cc: Felipe Balbi <balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org, peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org, dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org, mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org, Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org, abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Date: Fri, 8 Apr 2016 15:45:40 +0800 [thread overview] Message-ID: <20160408074540.GB9070@shlinux2.ap.freescale.net> (raw) In-Reply-To: <57075ACE.1010702-l0cyMroinI0@public.gmane.org> On Fri, Apr 08, 2016 at 10:16:30AM +0300, Roger Quadros wrote: > On 08/04/16 04:01, Peter Chen wrote: > > On Thu, Apr 07, 2016 at 01:40:21PM +0300, Roger Quadros wrote: > >> On 07/04/16 12:42, Peter Chen wrote: > >>> On Wed, Apr 06, 2016 at 09:32:22AM +0300, Roger Quadros wrote: > >>>> On 06/04/16 09:09, Felipe Balbi wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes: > >>>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > >>>>>> index 2ca2cef..6b1930d 100644 > >>>>>> --- a/drivers/usb/core/hcd.c > >>>>>> +++ b/drivers/usb/core/hcd.c > >>>>>> @@ -2706,6 +2706,7 @@ int usb_add_hcd(struct usb_hcd *hcd, > >>>>>> int retval; > >>>>>> struct usb_device *rhdev; > >>>>>> > >>>>>> + hcd->flags = 0; > >>>>> > >>> > >>> I am not sure if this usb_add(remove)_hcd pair is safe and clean enough > >>> for start/stop host role. From my point, we may need to do like > >>> .probe/.remove host platform driver interface. In that case, we can make > >> > >> probe and remove are meant to be called from bus layer. > >> I do not see a way how OTG framework can call probe/remove of HCD driver. > >> Some HCDs may be platform devices, some PCI, so different entities are calling > >> the HCD .probe hook. > >> > >>> sure the clocks and regulators are off, and hcd will be zero-initialized > >> > >> why can't we make that sure that is taken care of within the hcd_ops? > >> Why should some driver keep its regulators and clocks enabled when hcd is stopped? > >> It doesn't need to. If it is doing so now, it needs to be fixed. > >> > > > > Well, you may misunderstand me. I mean your hcd_ops->start or ->stop > > is hard to be a general one which only calls usb_hcd_add or > > usb_hcd_remove. It needs to implement like .probe or .remove at platform > > driver, some example code like host_start and host_stop at > > drivers/usb/chipidea/host.c. > > > The only extra thing the host_start/stop() of that driver is doing is > enabling/disabling the VBUS regulator. > In the OTG/dual role scope VBUS regulator handling has to be done via the > OTG driver using the otg ops otg_drv_vbus() and not at HCD level. Do you agree? > I agree with you that vbus handling should be handled by otg_drv_vbus for OTG case. > I don't want to complicate the OTG to HCD interface by adding new hooks there. > If HCD driver wants to do something special for OTG case it can always do that > within the struct hc_driver interface. But ideally OTG specific handling must > be done in the OTG driver. All that the HCD driver should care about is making sure > all used resources are disabled once usb_remove_hcd() is called. > In this patch, we set hcd->flag as 0 to fix one problem, but if any problems are still existed due to non-zero hcd structure. We have tested a lot for insmod/rmmod procedure for host driver, it doesn't show many problems. In a word, once the host role is stopped do we need to remove all things belong to host, and re-initialized all things belong to host when the host role is re-started? From my point is 'yes'. host_stop() { usb_remove_hcd(hcd); usb_put_hcd(hcd); platform_deinit(); /* clock, gpio, etc */ } host_start() { platform_init(); /* clock, gpio, etc */ usb_create_hcd(); ... /* initialize hcd structure from platform information */ usb_add_hcd(hcd); } Maybe you have already done something, but there is no user now. Things will be more clear after your post your dwc3 changes :) -- 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
next prev parent reply other threads:[~2016-04-08 7:52 UTC|newest] Thread overview: 155+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-04-05 14:05 [PATCH v6 00/12] USB OTG/dual-role framework Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-05 14:05 ` [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-06 6:09 ` Felipe Balbi 2016-04-06 6:09 ` Felipe Balbi 2016-04-06 6:32 ` Roger Quadros 2016-04-06 6:32 ` Roger Quadros 2016-04-07 9:42 ` Peter Chen 2016-04-07 10:40 ` Roger Quadros 2016-04-07 10:40 ` Roger Quadros 2016-04-08 1:01 ` Peter Chen 2016-04-08 1:01 ` Peter Chen 2016-04-08 7:16 ` Roger Quadros 2016-04-08 7:16 ` Roger Quadros 2016-04-08 7:45 ` Peter Chen [this message] 2016-04-08 7:45 ` Peter Chen 2016-04-18 2:29 ` Peter Chen 2016-04-18 14:11 ` Alan Stern 2016-04-18 14:11 ` Alan Stern 2016-04-19 1:56 ` Peter Chen 2016-04-19 1:56 ` Peter Chen 2016-04-20 8:15 ` Roger Quadros 2016-04-20 8:15 ` Roger Quadros 2016-04-20 9:40 ` Peter Chen 2016-04-05 14:05 ` [PATCH v6 02/12] usb: hcd.h: Add OTG to HCD interface Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 7:41 ` Peter Chen 2016-04-05 14:05 ` [PATCH v6 03/12] usb: otg-fsm: use usb_otg wherever possible Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 7:42 ` Peter Chen 2016-04-05 14:05 ` [PATCH v6 04/12] usb: otg-fsm: move host controller operations into usb_otg->hcd_ops Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 8:00 ` Peter Chen 2016-04-18 8:00 ` Peter Chen 2016-04-05 14:05 ` [PATCH v6 05/12] usb: gadget.h: Add OTG to gadget interface Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-05 14:05 ` [PATCH v6 06/12] usb: otg: get rid of CONFIG_USB_OTG_FSM in favour of CONFIG_USB_OTG Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 8:05 ` Peter Chen 2016-04-20 8:12 ` Roger Quadros 2016-04-20 8:12 ` Roger Quadros 2016-04-05 14:05 ` [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-07 8:52 ` Yoshihiro Shimoda 2016-04-07 11:45 ` Roger Quadros 2016-04-08 11:22 ` Yoshihiro Shimoda 2016-04-11 10:54 ` Roger Quadros 2016-04-14 8:36 ` Yoshihiro Shimoda 2016-04-14 10:59 ` Roger Quadros 2016-04-14 11:15 ` Yoshihiro Shimoda 2016-04-14 11:15 ` Yoshihiro Shimoda 2016-04-14 11:32 ` Roger Quadros 2016-04-14 11:32 ` Roger Quadros 2016-04-15 9:59 ` Yoshihiro Shimoda 2016-04-15 9:59 ` Yoshihiro Shimoda 2016-04-15 10:57 ` Roger Quadros 2016-04-15 10:57 ` Roger Quadros 2016-04-15 10:03 ` Yoshihiro Shimoda 2016-04-19 9:18 ` Peter Chen 2016-04-20 5:08 ` Yoshihiro Shimoda 2016-04-20 7:03 ` Roger Quadros 2016-04-22 6:05 ` Peter Chen 2016-04-22 6:05 ` Peter Chen 2016-04-22 1:26 ` Peter Chen 2016-04-22 3:34 ` Peter Chen 2016-04-22 3:34 ` Peter Chen 2016-04-22 5:57 ` Yoshihiro Shimoda 2016-04-22 5:57 ` Yoshihiro Shimoda 2016-04-15 9:25 ` Peter Chen 2016-04-15 9:25 ` Peter Chen 2016-04-15 11:00 ` Roger Quadros 2016-04-15 11:00 ` Roger Quadros 2016-04-18 2:09 ` Peter Chen 2016-04-20 6:54 ` Roger Quadros 2016-04-20 6:54 ` Roger Quadros 2016-04-20 9:26 ` Peter Chen 2016-04-19 8:06 ` Peter Chen 2016-04-20 7:02 ` Roger Quadros 2016-04-20 7:02 ` Roger Quadros 2016-04-20 9:39 ` Peter Chen 2016-04-21 6:52 ` Peter Chen 2016-04-21 6:52 ` Peter Chen 2016-04-25 14:05 ` Roger Quadros 2016-04-25 14:05 ` Roger Quadros 2016-04-26 2:07 ` Jun Li 2016-04-26 3:47 ` Peter Chen 2016-04-26 3:47 ` Peter Chen 2016-04-26 5:11 ` Jun Li 2016-04-26 5:11 ` Jun Li 2016-04-26 6:28 ` Peter Chen 2016-04-26 6:28 ` Peter Chen 2016-04-26 7:00 ` Jun Li 2016-04-26 7:00 ` Jun Li 2016-04-26 8:21 ` Peter Chen 2016-04-27 3:15 ` Peter Chen 2016-04-27 10:59 ` Roger Quadros 2016-04-27 10:59 ` Roger Quadros 2016-04-28 1:54 ` Peter Chen 2016-04-28 1:54 ` Peter Chen 2016-04-28 8:01 ` Roger Quadros 2016-04-28 8:01 ` Roger Quadros 2016-04-27 11:15 ` Roger Quadros 2016-04-05 14:05 ` [PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 6:29 ` Peter Chen 2016-04-19 8:14 ` Peter Chen 2016-04-20 6:47 ` Roger Quadros 2016-04-20 6:47 ` Roger Quadros 2016-04-20 6:46 ` Roger Quadros 2016-04-20 6:46 ` Roger Quadros 2016-04-27 10:16 ` Jun Li 2016-04-27 11:00 ` Roger Quadros 2016-04-27 11:11 ` Roger Quadros 2016-04-27 12:49 ` Jun Li 2016-04-27 13:18 ` Jun Li 2016-04-05 14:05 ` [PATCH v6 09/12] usb: gadget: udc: adapt " Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 6:59 ` Peter Chen 2016-04-18 6:59 ` Peter Chen 2016-04-20 6:51 ` Roger Quadros 2016-04-20 6:51 ` Roger Quadros 2016-04-21 6:38 ` Jun Li 2016-04-25 14:04 ` Roger Quadros 2016-04-26 0:07 ` Jun Li 2016-04-27 11:22 ` Roger Quadros 2016-04-27 11:22 ` Roger Quadros 2016-04-28 9:54 ` Roger Quadros 2016-04-28 9:54 ` Roger Quadros 2016-04-28 10:23 ` Jun Li 2016-04-28 12:22 ` Roger Quadros 2016-05-03 7:06 ` Jun Li 2016-05-03 15:44 ` Roger Quadros 2016-05-04 1:47 ` Peter Chen 2016-05-04 1:47 ` Peter Chen 2016-05-04 3:35 ` Peter Chen 2016-05-04 6:37 ` Roger Quadros 2016-05-04 7:53 ` Peter Chen 2016-05-04 8:03 ` Jun Li 2016-05-04 8:40 ` Roger Quadros 2016-05-04 8:40 ` Roger Quadros 2016-05-04 8:39 ` Peter Chen 2016-04-05 14:05 ` [PATCH v6 10/12] usb: doc: dt-binding: Add otg-controller property Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-05 14:05 ` [PATCH v6 11/12] usb: core: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-18 7:08 ` Peter Chen 2016-04-27 14:35 ` Roger Quadros 2016-04-27 14:35 ` Roger Quadros 2016-04-05 14:05 ` [PATCH v6 12/12] usb: host: xhci-plat: Add otg device to platform data Roger Quadros 2016-04-05 14:05 ` Roger Quadros 2016-04-06 3:23 ` Yoshihiro Shimoda 2016-04-06 3:23 ` Yoshihiro Shimoda 2016-04-06 6:30 ` Roger Quadros 2016-04-06 6:30 ` Roger Quadros
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160408074540.GB9070@shlinux2.ap.freescale.net \ --to=hzpeterchen@gmail.com \ --cc=Joao.Pinto@synopsys.com \ --cc=abrestic@chromium.org \ --cc=balbi@kernel.org \ --cc=dan.j.williams@intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=jun.li@freescale.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=mathias.nyman@linux.intel.com \ --cc=peter.chen@freescale.com \ --cc=rogerq@ti.com \ --cc=stern@rowland.harvard.edu \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.