| Message ID | 20160626072838.28082-13-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jun 26, 2016 at 12:28:29AM -0700, Stephen Boyd wrote: > Sometimes the usb wrapper device is part of a power domain that > needs to stay on as long as the device is active. Let's get and > put the device in driver probe/remove so that we keep the power > domain powered as long as the device is attached. We can fine > tune this later to handle wakeup interrupts, etc. for finer grain > power management later, but this is necessary to make sure we can > keep accessing the device right now. Since some of the controllers work abnormal if we enables runtime pm unconditionally, so I use one system flag CI_HDRC_SUPPORTS_RUNTIME_PM for it. I can't understand why you can't access device without enable parent's runtime pm, the controller will not enter runtime suspend without that flag. Peter > > Cc: Peter Chen <peter.chen@nxp.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> > --- > drivers/usb/chipidea/ci_hdrc_msm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c > index 520c85e701ef..430856ef1be3 100644 > --- a/drivers/usb/chipidea/ci_hdrc_msm.c > +++ b/drivers/usb/chipidea/ci_hdrc_msm.c > @@ -80,6 +80,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) > > pm_runtime_no_callbacks(&pdev->dev); > pm_runtime_enable(&pdev->dev); > + pm_runtime_get(&pdev->dev); > > return 0; > } > @@ -88,6 +89,7 @@ static int ci_hdrc_msm_remove(struct platform_device *pdev) > { > struct platform_device *plat_ci = platform_get_drvdata(pdev); > > + pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > ci_hdrc_remove_device(plat_ci); > > -- > 2.9.0.rc2.8.ga28705d > > -- > 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
Quoting Peter Chen (2016-06-28 23:46:00) > On Sun, Jun 26, 2016 at 12:28:29AM -0700, Stephen Boyd wrote: > > Sometimes the usb wrapper device is part of a power domain that > > needs to stay on as long as the device is active. Let's get and > > put the device in driver probe/remove so that we keep the power > > domain powered as long as the device is attached. We can fine > > tune this later to handle wakeup interrupts, etc. for finer grain > > power management later, but this is necessary to make sure we can > > keep accessing the device right now. > > Since some of the controllers work abnormal if we enables runtime > pm unconditionally, so I use one system flag CI_HDRC_SUPPORTS_RUNTIME_PM > for it. I can't understand why you can't access device without enable > parent's runtime pm, the controller will not enter runtime suspend > without that flag. Correct, the child device of ci_hdrc_msm will be able to do runtime PM and keep the parent enabled if the CI_HDRC_SUPPORTS_RUNTIME_PM flag is set. But even if that flag isn't set, the ci_hdrc_msm driver is calling pm_runtime_enable() on the same device that it would be called on if the CI_HDRC_SUPPORTS_RUNTIME_PM flag was set. That allows runtime PM transition of child devices such as the usb ports (usb1-port1 for example) to propagate up all the way to the ci_hdrc_msm device and disable any attached power domains. Why don't we call runtime PM functions on the ci->dev for all cases of ci->supports_runtime_pm? It seems like the glue drivers should be managing their own device power states and the ci->dev should be managed by core.c code. Another solution would be to remove the call to pm_runtime_enable() from ci_hdrc_msm. That would make sure we don't call the power domain code when the device changes runtime PM states and rely on the fact that power domains are turned on during device driver probe.
On Wed, Jun 29, 2016 at 05:43:30PM -0700, Stephen Boyd wrote: > Quoting Peter Chen (2016-06-28 23:46:00) > > On Sun, Jun 26, 2016 at 12:28:29AM -0700, Stephen Boyd wrote: > > > Sometimes the usb wrapper device is part of a power domain that > > > needs to stay on as long as the device is active. Let's get and > > > put the device in driver probe/remove so that we keep the power > > > domain powered as long as the device is attached. We can fine > > > tune this later to handle wakeup interrupts, etc. for finer grain > > > power management later, but this is necessary to make sure we can > > > keep accessing the device right now. > > > > Since some of the controllers work abnormal if we enables runtime > > pm unconditionally, so I use one system flag CI_HDRC_SUPPORTS_RUNTIME_PM > > for it. I can't understand why you can't access device without enable > > parent's runtime pm, the controller will not enter runtime suspend > > without that flag. > > Correct, the child device of ci_hdrc_msm will be able to do runtime PM > and keep the parent enabled if the CI_HDRC_SUPPORTS_RUNTIME_PM flag is > set. But even if that flag isn't set, the ci_hdrc_msm driver is calling > pm_runtime_enable() on the same device that it would be called on if the > CI_HDRC_SUPPORTS_RUNTIME_PM flag was set. That allows runtime PM > transition of child devices such as the usb ports (usb1-port1 for > example) to propagate up all the way to the ci_hdrc_msm device and > disable any attached power domains. Sorry, I can't get you. If the chipidea core's runtime is disabled, the port under the controller will not be in runtime suspended, only the bus will be in suspended due to USB core enables runtime PM by default. > > Why don't we call runtime PM functions on the ci->dev for all cases of > ci->supports_runtime_pm? It seems like the glue drivers should be > managing their own device power states and the ci->dev should be managed > by core.c code. > This is current design. Chipidea core manages portsc.phcd and PHY's PM (through PHY's API), and glue layer manages its own clocks on the system bus for register visit (and data transfer if necessary).
Quoting Peter Chen (2016-06-29 18:39:01) > On Wed, Jun 29, 2016 at 05:43:30PM -0700, Stephen Boyd wrote: > > Quoting Peter Chen (2016-06-28 23:46:00) > > > On Sun, Jun 26, 2016 at 12:28:29AM -0700, Stephen Boyd wrote: > > > > Sometimes the usb wrapper device is part of a power domain that > > > > needs to stay on as long as the device is active. Let's get and > > > > put the device in driver probe/remove so that we keep the power > > > > domain powered as long as the device is attached. We can fine > > > > tune this later to handle wakeup interrupts, etc. for finer grain > > > > power management later, but this is necessary to make sure we can > > > > keep accessing the device right now. > > > > > > Since some of the controllers work abnormal if we enables runtime > > > pm unconditionally, so I use one system flag CI_HDRC_SUPPORTS_RUNTIME_PM > > > for it. I can't understand why you can't access device without enable > > > parent's runtime pm, the controller will not enter runtime suspend > > > without that flag. > > > > Correct, the child device of ci_hdrc_msm will be able to do runtime PM > > and keep the parent enabled if the CI_HDRC_SUPPORTS_RUNTIME_PM flag is > > set. But even if that flag isn't set, the ci_hdrc_msm driver is calling > > pm_runtime_enable() on the same device that it would be called on if the > > CI_HDRC_SUPPORTS_RUNTIME_PM flag was set. That allows runtime PM > > transition of child devices such as the usb ports (usb1-port1 for > > example) to propagate up all the way to the ci_hdrc_msm device and > > disable any attached power domains. > > Sorry, I can't get you. > > If the chipidea core's runtime is disabled, the port under the > controller will not be in runtime suspended, only the bus will > be in suspended due to USB core enables runtime PM by default. Hmm sorry, I was confused too. From what I can tell, if I don't call pm_runtime_set_active() on the glue device, it will runtime suspend once I call pm_runtime_enable() on it (which we do in ci_hdrc_mms_probe()). When we runtime suspend the glue device, we turn off the power domain associated with it too. The runtime pm enabled state of the core device doesn't seem to matter either way here. So perhaps I should be calling pm_runtime_set_active() before pm_runtime_enable() there instead of doing the get/put? It isn't clear to me when we should be calling pm_runtime_get() vs. pm_runtime_set_active() though. > > > > > Why don't we call runtime PM functions on the ci->dev for all cases of > > ci->supports_runtime_pm? It seems like the glue drivers should be > > managing their own device power states and the ci->dev should be managed > > by core.c code. > > > > This is current design. Chipidea core manages portsc.phcd and PHY's PM > (through PHY's API), and glue layer manages its own clocks on the system > bus for register visit (and data transfer if necessary). > Sorry, I mean this code in core.c pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); pm_runtime_set_autosuspend_delay(&pdev->dev, 2000); pm_runtime_mark_last_busy(ci->dev); pm_runtime_use_autosuspend(&pdev->dev); which confused me. I thought pdev->dev was the glue device, but it's the same as ci->dev, the core device. I get it now, but I'd like to change all the calls there to use ci->dev to be clearer.
On Thu, Jun 30, 2016 at 01:30:54PM -0700, Stephen Boyd wrote: > Quoting Peter Chen (2016-06-29 18:39:01) > > On Wed, Jun 29, 2016 at 05:43:30PM -0700, Stephen Boyd wrote: > > > Quoting Peter Chen (2016-06-28 23:46:00) > > > > On Sun, Jun 26, 2016 at 12:28:29AM -0700, Stephen Boyd wrote: > > > > > Sometimes the usb wrapper device is part of a power domain that > > > > > needs to stay on as long as the device is active. Let's get and > > > > > put the device in driver probe/remove so that we keep the power > > > > > domain powered as long as the device is attached. We can fine > > > > > tune this later to handle wakeup interrupts, etc. for finer grain > > > > > power management later, but this is necessary to make sure we can > > > > > keep accessing the device right now. > > > > > > > > Since some of the controllers work abnormal if we enables runtime > > > > pm unconditionally, so I use one system flag CI_HDRC_SUPPORTS_RUNTIME_PM > > > > for it. I can't understand why you can't access device without enable > > > > parent's runtime pm, the controller will not enter runtime suspend > > > > without that flag. > > > > > > Correct, the child device of ci_hdrc_msm will be able to do runtime PM > > > and keep the parent enabled if the CI_HDRC_SUPPORTS_RUNTIME_PM flag is > > > set. But even if that flag isn't set, the ci_hdrc_msm driver is calling > > > pm_runtime_enable() on the same device that it would be called on if the > > > CI_HDRC_SUPPORTS_RUNTIME_PM flag was set. That allows runtime PM > > > transition of child devices such as the usb ports (usb1-port1 for > > > example) to propagate up all the way to the ci_hdrc_msm device and > > > disable any attached power domains. > > > > Sorry, I can't get you. > > > > If the chipidea core's runtime is disabled, the port under the > > controller will not be in runtime suspended, only the bus will > > be in suspended due to USB core enables runtime PM by default. > > Hmm sorry, I was confused too. > > From what I can tell, if I don't call pm_runtime_set_active() on the > glue device, it will runtime suspend once I call pm_runtime_enable() on > it (which we do in ci_hdrc_mms_probe()). When we runtime suspend the > glue device, we turn off the power domain associated with it too. The > runtime pm enabled state of the core device doesn't seem to matter > either way here. So perhaps I should be calling pm_runtime_set_active() > before pm_runtime_enable() there instead of doing the get/put? It isn't > clear to me when we should be calling pm_runtime_get() vs. > pm_runtime_set_active() though. > > > > > > > > > Why don't we call runtime PM functions on the ci->dev for all cases of > > > ci->supports_runtime_pm? It seems like the glue drivers should be > > > managing their own device power states and the ci->dev should be managed > > > by core.c code. > > > > > > > This is current design. Chipidea core manages portsc.phcd and PHY's PM > > (through PHY's API), and glue layer manages its own clocks on the system > > bus for register visit (and data transfer if necessary). > > > > Sorry, I mean this code in core.c > > pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > pm_runtime_set_autosuspend_delay(&pdev->dev, 2000); > pm_runtime_mark_last_busy(ci->dev); > pm_runtime_use_autosuspend(&pdev->dev); > > which confused me. I thought pdev->dev was the glue device, but it's the > same as ci->dev, the core device. I get it now, but I'd like to change > all the calls there to use ci->dev to be clearer. Yes, please do it. Glue device is the parent for core device, and at core.c, only ci_hdrc_add_device and ci_get_platdata will touch glue device.
diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c index 520c85e701ef..430856ef1be3 100644 --- a/drivers/usb/chipidea/ci_hdrc_msm.c +++ b/drivers/usb/chipidea/ci_hdrc_msm.c @@ -80,6 +80,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) pm_runtime_no_callbacks(&pdev->dev); pm_runtime_enable(&pdev->dev); + pm_runtime_get(&pdev->dev); return 0; } @@ -88,6 +89,7 @@ static int ci_hdrc_msm_remove(struct platform_device *pdev) { struct platform_device *plat_ci = platform_get_drvdata(pdev); + pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); ci_hdrc_remove_device(plat_ci);
Sometimes the usb wrapper device is part of a power domain that needs to stay on as long as the device is active. Let's get and put the device in driver probe/remove so that we keep the power domain powered as long as the device is attached. We can fine tune this later to handle wakeup interrupts, etc. for finer grain power management later, but this is necessary to make sure we can keep accessing the device right now. Cc: Peter Chen <peter.chen@nxp.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- drivers/usb/chipidea/ci_hdrc_msm.c | 2 ++ 1 file changed, 2 insertions(+)