| Message ID | 20190125103925.15753-5-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A3700 USB S2RAM support | expand |
On Fri, 25 Jan 2019, Miquel Raynal wrote: > Add suspend/resume callbacks to reset the host controller properly > during S2RAM operation. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/usb/host/ehci-orion.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > index 3109f082949e..0dab099b5d15 100644 > --- a/drivers/usb/host/ehci-orion.c > +++ b/drivers/usb/host/ehci-orion.c > @@ -182,6 +182,23 @@ static int ehci_orion_drv_reset(struct usb_hcd *hcd) > return ret; > } > > +static int __maybe_unused ehci_orion_drv_suspend(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + > + return ehci_suspend(hcd, device_may_wakeup(dev)); Okay, good. > +} > + > +static int __maybe_unused ehci_orion_drv_resume(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + > + return ehci_resume(hcd, device_may_wakeup(dev)); Not so good. The second argument here is force_reset; presumably you want it always to be false. (Note that the v3 version of this patch did not call device_may_wakeup() in ehci_orion_drv_resume.) Yes, the API is not symmetrical. So sue me... Alan Stern
Hi Alan, Alan Stern <stern@rowland.harvard.edu> wrote on Fri, 25 Jan 2019 21:56:27 -0500 (EST): > On Fri, 25 Jan 2019, Miquel Raynal wrote: > > > Add suspend/resume callbacks to reset the host controller properly > > during S2RAM operation. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/usb/host/ehci-orion.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > > index 3109f082949e..0dab099b5d15 100644 > > --- a/drivers/usb/host/ehci-orion.c > > +++ b/drivers/usb/host/ehci-orion.c > > @@ -182,6 +182,23 @@ static int ehci_orion_drv_reset(struct usb_hcd *hcd) > > return ret; > > } > > > > +static int __maybe_unused ehci_orion_drv_suspend(struct device *dev) > > +{ > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > + > > + return ehci_suspend(hcd, device_may_wakeup(dev)); > > Okay, good. > > > +} > > + > > +static int __maybe_unused ehci_orion_drv_resume(struct device *dev) > > +{ > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > + > > + return ehci_resume(hcd, device_may_wakeup(dev)); > > Not so good. The second argument here is force_reset; presumably you > want it always to be false. (Note that the v3 version of this patch > did not call device_may_wakeup() in ehci_orion_drv_resume.) > > Yes, the API is not symmetrical. So sue me... Oh right, I completely overlooked that one. Indeed the second parameter should be "false", as in the v3. Do you mind if I send a v5 only for this patch? If the rest looks good to you of course. Thanks, Miquèl
On Mon, 28 Jan 2019, Miquel Raynal wrote: > Hi Alan, > > Alan Stern <stern@rowland.harvard.edu> wrote on Fri, 25 Jan 2019 > 21:56:27 -0500 (EST): > > > On Fri, 25 Jan 2019, Miquel Raynal wrote: > > > > > Add suspend/resume callbacks to reset the host controller properly > > > during S2RAM operation. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/usb/host/ehci-orion.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > > > index 3109f082949e..0dab099b5d15 100644 > > > --- a/drivers/usb/host/ehci-orion.c > > > +++ b/drivers/usb/host/ehci-orion.c > > > @@ -182,6 +182,23 @@ static int ehci_orion_drv_reset(struct usb_hcd *hcd) > > > return ret; > > > } > > > > > > +static int __maybe_unused ehci_orion_drv_suspend(struct device *dev) > > > +{ > > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > > + > > > + return ehci_suspend(hcd, device_may_wakeup(dev)); > > > > Okay, good. > > > > > +} > > > + > > > +static int __maybe_unused ehci_orion_drv_resume(struct device *dev) > > > +{ > > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > > + > > > + return ehci_resume(hcd, device_may_wakeup(dev)); > > > > Not so good. The second argument here is force_reset; presumably you > > want it always to be false. (Note that the v3 version of this patch > > did not call device_may_wakeup() in ehci_orion_drv_resume.) > > > > Yes, the API is not symmetrical. So sue me... > > Oh right, I completely overlooked that one. Indeed the second parameter > should be "false", as in the v3. Do you mind if I send a v5 only for > this patch? If the rest looks good to you of course. To avoid confusing Greg, it would be best to send v5 of all the patches that he should apply. The rest of the EHCI changes look okay, and when you fix this you can add: Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern
Hi Alan, Alan Stern <stern@rowland.harvard.edu> wrote on Mon, 28 Jan 2019 10:27:53 -0500 (EST): > On Mon, 28 Jan 2019, Miquel Raynal wrote: > > > Hi Alan, > > > > Alan Stern <stern@rowland.harvard.edu> wrote on Fri, 25 Jan 2019 > > 21:56:27 -0500 (EST): > > > > > On Fri, 25 Jan 2019, Miquel Raynal wrote: > > > > > > > Add suspend/resume callbacks to reset the host controller properly > > > > during S2RAM operation. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/usb/host/ehci-orion.c | 18 ++++++++++++++++++ > > > > 1 file changed, 18 insertions(+) > > > > > > > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c > > > > index 3109f082949e..0dab099b5d15 100644 > > > > --- a/drivers/usb/host/ehci-orion.c > > > > +++ b/drivers/usb/host/ehci-orion.c > > > > @@ -182,6 +182,23 @@ static int ehci_orion_drv_reset(struct usb_hcd *hcd) > > > > return ret; > > > > } > > > > > > > > +static int __maybe_unused ehci_orion_drv_suspend(struct device *dev) > > > > +{ > > > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > > > + > > > > + return ehci_suspend(hcd, device_may_wakeup(dev)); > > > > > > Okay, good. > > > > > > > +} > > > > + > > > > +static int __maybe_unused ehci_orion_drv_resume(struct device *dev) > > > > +{ > > > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > > > + > > > > + return ehci_resume(hcd, device_may_wakeup(dev)); > > > > > > Not so good. The second argument here is force_reset; presumably you > > > want it always to be false. (Note that the v3 version of this patch > > > did not call device_may_wakeup() in ehci_orion_drv_resume.) > > > > > > Yes, the API is not symmetrical. So sue me... > > > > Oh right, I completely overlooked that one. Indeed the second parameter > > should be "false", as in the v3. Do you mind if I send a v5 only for > > this patch? If the rest looks good to you of course. > > To avoid confusing Greg, it would be best to send v5 of all the patches > that he should apply. The rest of the EHCI changes look okay, and when > you fix this you can add: > > Acked-by: Alan Stern <stern@rowland.harvard.edu> Sure! Thanks, Miquèl
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c index 3109f082949e..0dab099b5d15 100644 --- a/drivers/usb/host/ehci-orion.c +++ b/drivers/usb/host/ehci-orion.c @@ -182,6 +182,23 @@ static int ehci_orion_drv_reset(struct usb_hcd *hcd) return ret; } +static int __maybe_unused ehci_orion_drv_suspend(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + + return ehci_suspend(hcd, device_may_wakeup(dev)); +} + +static int __maybe_unused ehci_orion_drv_resume(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + + return ehci_resume(hcd, device_may_wakeup(dev)); +} + +static SIMPLE_DEV_PM_OPS(ehci_orion_pm_ops, ehci_orion_drv_suspend, + ehci_orion_drv_resume); + static const struct ehci_driver_overrides orion_overrides __initconst = { .extra_priv_size = sizeof(struct orion_ehci_hcd), .reset = ehci_orion_drv_reset, @@ -334,6 +351,7 @@ static struct platform_driver ehci_orion_driver = { .driver = { .name = "orion-ehci", .of_match_table = ehci_orion_dt_ids, + .pm = &ehci_orion_pm_ops, }, };
Add suspend/resume callbacks to reset the host controller properly during S2RAM operation. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/usb/host/ehci-orion.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)