diff mbox series

[3/8] usb: chipidea: host: add suspend/resume support for host controller

Message ID 20221009160813.776829-4-xu.yang_2@nxp.com
State Superseded
Headers show
Series [1/8] usb: chipidea: core: add controller resume support when controller is powered off | expand

Commit Message

Xu Yang Oct. 9, 2022, 4:08 p.m. UTC 
The controller's power may be powered off during system suspend. This
will add suspend/resume support when the controller suffers power lost.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/chipidea/host.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

kernel test robot Oct. 9, 2022, 12:22 p.m. UTC | #1 
Hi Xu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on usb/usb-testing linus/master v6.0 next-20221007]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xu-Yang/add-power-lost-support-during-system-suspend-resume/20221009-160800
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 7cd04013fbf3e6dcb67ca6b59aa813269a2ad9ce
config: riscv-randconfig-r042-20221009
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/92b60869daf11540ef49a3df8f3442c12c783ce4
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Xu-Yang/add-power-lost-support-during-system-suspend-resume/20221009-160800
        git checkout 92b60869daf11540ef49a3df8f3442c12c783ce4
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "ehci_suspend" [drivers/usb/chipidea/ci_hdrc.ko] undefined!
>> ERROR: modpost: "ehci_resume" [drivers/usb/chipidea/ci_hdrc.ko] undefined!
Xu Yang Oct. 10, 2022, 2:54 a.m. UTC | #2 
Hi Greg,

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Monday, October 10, 2022 2:36 AM
> To: Xu Yang <xu.yang_2@nxp.com>
> Cc: peter.chen@kernel.org; Jun Li <jun.li@nxp.com>; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 3/8] usb: chipidea: host: add suspend/resume support for host controller
> 
> Caution: EXT Email
> 
> On Mon, Oct 10, 2022 at 12:08:08AM +0800, Xu Yang wrote:
> > The controller's power may be powered off during system suspend. This
> > will add suspend/resume support when the controller suffers power lost.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/chipidea/host.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index bc3634a54c6b..dc033272c31e 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -459,6 +459,16 @@ static void ci_hdrc_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> >       ci_hdrc_free_dma_aligned_buffer(urb);
> >  }
> >
> > +static void ci_hdrc_host_suspend(struct ci_hdrc *ci)
> > +{
> > +     ehci_suspend(ci->hcd, device_may_wakeup(ci->dev));
> 
> Why is the chipidea driver somehow now tied to the ehci driver?

The chipidea host controller driver has been tied to ehci driver from the 
beginning. Here, this patch will enhance the host controller by using ehci
suspend/resume functions.

> 
> Are you sure you want this?  Why?  Is hcd always a ehci device?

As the host controller may suffer power lost after system suspend, there is
a need to restore it correctlly after system resume. ehci_suspend/resume()
will handle the generic parts of controller suspend/resume, at the same time,
ehci_resume() could handle power lost case by reporting this event to root
hub and reset host controller in a normal way. So it's exactly a good choice
to use them. 

As per chipidea RM: The USB 2.0 Controller is an instantiation of an 
EHCI-compatible core which supports high-, full-, and low-speed operation.
So this hcd will be a ehci device all the time.

Thanks,
Xu Yang

> 
> thanks,
> 
> greg k-h
Greg KH Oct. 10, 2022, 6:31 a.m. UTC | #3 
On Mon, Oct 10, 2022 at 02:54:02AM +0000, Xu Yang wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Monday, October 10, 2022 2:36 AM
> > To: Xu Yang <xu.yang_2@nxp.com>
> > Cc: peter.chen@kernel.org; Jun Li <jun.li@nxp.com>; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: [EXT] Re: [PATCH 3/8] usb: chipidea: host: add suspend/resume support for host controller
> > 
> > Caution: EXT Email
> > 
> > On Mon, Oct 10, 2022 at 12:08:08AM +0800, Xu Yang wrote:
> > > The controller's power may be powered off during system suspend. This
> > > will add suspend/resume support when the controller suffers power lost.
> > >
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > ---
> > >  drivers/usb/chipidea/host.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > > index bc3634a54c6b..dc033272c31e 100644
> > > --- a/drivers/usb/chipidea/host.c
> > > +++ b/drivers/usb/chipidea/host.c
> > > @@ -459,6 +459,16 @@ static void ci_hdrc_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> > >       ci_hdrc_free_dma_aligned_buffer(urb);
> > >  }
> > >
> > > +static void ci_hdrc_host_suspend(struct ci_hdrc *ci)
> > > +{
> > > +     ehci_suspend(ci->hcd, device_may_wakeup(ci->dev));
> > 
> > Why is the chipidea driver somehow now tied to the ehci driver?
> 
> The chipidea host controller driver has been tied to ehci driver from the 
> beginning. Here, this patch will enhance the host controller by using ehci
> suspend/resume functions.
> 
> > 
> > Are you sure you want this?  Why?  Is hcd always a ehci device?
> 
> As the host controller may suffer power lost after system suspend, there is
> a need to restore it correctlly after system resume. ehci_suspend/resume()
> will handle the generic parts of controller suspend/resume, at the same time,
> ehci_resume() could handle power lost case by reporting this event to root
> hub and reset host controller in a normal way. So it's exactly a good choice
> to use them. 
> 
> As per chipidea RM: The USB 2.0 Controller is an instantiation of an 
> EHCI-compatible core which supports high-, full-, and low-speed operation.
> So this hcd will be a ehci device all the time.

Ah, sorry, you are right.  But the build breaks so you need to fix
something up to so that this dependancy really is there.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index bc3634a54c6b..dc033272c31e 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -459,6 +459,16 @@  static void ci_hdrc_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
 	ci_hdrc_free_dma_aligned_buffer(urb);
 }
 
+static void ci_hdrc_host_suspend(struct ci_hdrc *ci)
+{
+	ehci_suspend(ci->hcd, device_may_wakeup(ci->dev));
+}
+
+static void ci_hdrc_host_resume(struct ci_hdrc *ci, bool power_lost)
+{
+	ehci_resume(ci->hcd, power_lost);
+}
+
 int ci_hdrc_host_init(struct ci_hdrc *ci)
 {
 	struct ci_role_driver *rdrv;
@@ -472,6 +482,8 @@  int ci_hdrc_host_init(struct ci_hdrc *ci)
 
 	rdrv->start	= host_start;
 	rdrv->stop	= host_stop;
+	rdrv->suspend	= ci_hdrc_host_suspend;
+	rdrv->resume	= ci_hdrc_host_resume;
 	rdrv->irq	= host_irq;
 	rdrv->name	= "host";
 	ci->roles[CI_ROLE_HOST] = rdrv;