Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

[Brian Norris <briannorris@chromium.org>]

Hi,

On Wed, Aug 24, 2016 at 04:48:01PM -0400, Robert Foss wrote:
> On 2016-08-22 11:23 PM, Brian Norris wrote:
> >+ others
> >
> >Hi Robert and Felipe,
> >
> >I have a few questions for one or both of you. I'm not really an expert
> >on runtime PM, so please take my questions with a grain of salt.
> >
> >On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote:
> >>From: Robert Foss <robert.foss@collabora.com>
> >>
> >>Enable runtime PM for the xhci-plat device so that the parent device
> >>may implement runtime PM.
> >>
> >>Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >>
> >>Tested-by: Robert Foss <robert.foss@collabora.com>
> >>---
> >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
> >> 1 file changed, 27 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >>index ed56bf9..ba4efe7 100644
> >>--- a/drivers/usb/host/xhci-plat.c
> >>+++ b/drivers/usb/host/xhci-plat.c
> >>@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >> 	if (ret)
> >> 		goto dealloc_usb2_hcd;
> >>
> >>+	pm_runtime_set_active(&pdev->dev);
> >>+	pm_runtime_enable(&pdev->dev);
> >>+
> >
> >How does it help to enable PM runtime like this, if you don't have any
> >kind of runtime_{suspend,resume}() callbacks?
> 
> Andrew, I think you understand the inner workings of this code
> better than me, maybe you could give a short summary?

I believe Andrew is fairly busy, and I already talked with him a bit
about this. This is needed (as per your (or his?) commit message) only
if you have a parent device that wants to implement runtime PM. Now
depending on what you want to do with "runtime PM", this might mean
the parent device has to suspend xhci_{suspend,resume}() on behalf of
xhci-plat.c. Not very nice layering IMO, but it has been done before...

So I guess this comes down to "what does a parent device/driver want to
do"? If that's (e.g.) just putting some PHY into a slightly lower power
mode, then maybe it's fine for xhci-plat not to do anything else. But if
you actually want to completely power down the parent bus, reset an
accompanying dual-role/OTG controller, etc., then this really isn't
sufficient, AFAICT. But maybe that's just an indictment of the poor
structure of dwc3's host-mode support, more than it is an indictment of
this patch.

Brian