* [FOR DISCUSSION 0/8] Dove PMU support @ 2015-02-14 15:26 ` Russell King - ARM Linux 0 siblings, 0 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-14 15:26 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: devicetree, Greg Kroah-Hartman, Ian Campbell, Kumar Gala, Len Brown, linux-arm-kernel, linux-pm, Mark Rutland, Pawel Moll, Rob Herring This is a re-posting of the patch set which I posted almost 10 months ago to support the Dove PMU, with a few additional changes. This set is based upon 3.19. In this set are: * two patches which Rafael originally acked, but there was indecision last time around how to handle them due to potential conflicts with work that Ulf was doing. These patches have been updated to apply cleanly to 3.19. I don't know if people want to take these as fixes to the PM domain code or not (hence why I'm posting this series during the merge window - if it weren't for this, I'd hold it off.) * what I regard as a fix to the PM domain code; as a result of a previous commit, the PM domain code mismatches the runtime PM state, which leads to the PM domain being unexpectedly left on. This patch works around that. (It's been sent recently as well but in an old thread.) * the addition of the core Dove PMU driver, which consists of a reset, IRQ controller, and power domains. The reset and power domain code has to be closely related due to the power up/down requirements of the GPU/VPU subsystems needing to be performed atomically. (This requirement prevents it using the MFD infrastructure, because we would need to hold spinlocks while calling several different sub-drivers.) * addition of the RTC interrupt, so we can now receive and act on alarms generated by the Dove RTC. * addition of the DT descriptions for the GPU and VPU power domains. These patches do not themselves add the DT descriptions for these units, so these patches serve as illustrations how these should be described. There are a few things which are missing from this driver, such sa the DT binding documentation. This will follow once people are happy with the first four patches, in particular where to locate the PMU driver in the kernel tree. Currently, I have it in arch/arm/mach-dove, which is where the legacy Dove code lives, so it's not ideal. There are some gotchas with it - PM domains need to be created prior to any device which uses them being probed, so it is best if the driver is initialised really early in the kernel boot. We may be able to get away with a core_initcall() or a postcore_initcall(). I'd ideally like to get these queued for merging in the _next_ merge window if at all possible, if only to reduce the number of patches I've been carrying for the last few years. arch/arm/Kconfig | 1 + arch/arm/boot/dts/dove.dtsi | 25 +++ arch/arm/mach-dove/Makefile | 1 + arch/arm/mach-dove/common.c | 2 + arch/arm/mach-dove/common.h | 1 + arch/arm/mach-dove/include/mach/pm.h | 17 -- arch/arm/mach-dove/irq.c | 87 -------- arch/arm/mach-dove/pmu.c | 410 +++++++++++++++++++++++++++++++++++ drivers/base/power/domain.c | 26 ++- drivers/base/power/runtime.c | 5 + include/linux/pm.h | 1 + 11 files changed, 466 insertions(+), 110 deletions(-) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [FOR DISCUSSION 0/8] Dove PMU support @ 2015-02-14 15:26 ` Russell King - ARM Linux 0 siblings, 0 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-14 15:26 UTC (permalink / raw) To: linux-arm-kernel This is a re-posting of the patch set which I posted almost 10 months ago to support the Dove PMU, with a few additional changes. This set is based upon 3.19. In this set are: * two patches which Rafael originally acked, but there was indecision last time around how to handle them due to potential conflicts with work that Ulf was doing. These patches have been updated to apply cleanly to 3.19. I don't know if people want to take these as fixes to the PM domain code or not (hence why I'm posting this series during the merge window - if it weren't for this, I'd hold it off.) * what I regard as a fix to the PM domain code; as a result of a previous commit, the PM domain code mismatches the runtime PM state, which leads to the PM domain being unexpectedly left on. This patch works around that. (It's been sent recently as well but in an old thread.) * the addition of the core Dove PMU driver, which consists of a reset, IRQ controller, and power domains. The reset and power domain code has to be closely related due to the power up/down requirements of the GPU/VPU subsystems needing to be performed atomically. (This requirement prevents it using the MFD infrastructure, because we would need to hold spinlocks while calling several different sub-drivers.) * addition of the RTC interrupt, so we can now receive and act on alarms generated by the Dove RTC. * addition of the DT descriptions for the GPU and VPU power domains. These patches do not themselves add the DT descriptions for these units, so these patches serve as illustrations how these should be described. There are a few things which are missing from this driver, such sa the DT binding documentation. This will follow once people are happy with the first four patches, in particular where to locate the PMU driver in the kernel tree. Currently, I have it in arch/arm/mach-dove, which is where the legacy Dove code lives, so it's not ideal. There are some gotchas with it - PM domains need to be created prior to any device which uses them being probed, so it is best if the driver is initialised really early in the kernel boot. We may be able to get away with a core_initcall() or a postcore_initcall(). I'd ideally like to get these queued for merging in the _next_ merge window if at all possible, if only to reduce the number of patches I've been carrying for the last few years. arch/arm/Kconfig | 1 + arch/arm/boot/dts/dove.dtsi | 25 +++ arch/arm/mach-dove/Makefile | 1 + arch/arm/mach-dove/common.c | 2 + arch/arm/mach-dove/common.h | 1 + arch/arm/mach-dove/include/mach/pm.h | 17 -- arch/arm/mach-dove/irq.c | 87 -------- arch/arm/mach-dove/pmu.c | 410 +++++++++++++++++++++++++++++++++++ drivers/base/power/domain.c | 26 ++- drivers/base/power/runtime.c | 5 + include/linux/pm.h | 1 + 11 files changed, 466 insertions(+), 110 deletions(-) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/8] pm: domains: quieten down generic pm domains 2015-02-14 15:26 ` Russell King - ARM Linux (?) @ 2015-02-14 15:27 ` Russell King -1 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Len Brown, Greg Kroah-Hartman, linux-pm PM domains are rather noisy; scheduling behaviour can cause callbacks to take longer, which causes them to spit out a warning-level message each time a callback takes a little longer than the previous time. There really isn't a need for this, except when debugging. Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/base/power/domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 0d8780c04a5e..b3fbc21da2dc 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -173,8 +173,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd) genpd->power_on_latency_ns = elapsed_ns; genpd->max_off_time_changed = true; genpd_recalc_cpu_exit_latency(genpd); - pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n", - genpd->name, "on", elapsed_ns); + pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n", + genpd->name, "on", elapsed_ns); return ret; } @@ -199,8 +199,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd) genpd->power_off_latency_ns = elapsed_ns; genpd->max_off_time_changed = true; - pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n", - genpd->name, "off", elapsed_ns); + pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n", + genpd->name, "off", elapsed_ns); return ret; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/8] pm: domains: avoid potential oops in pm_genpd_remove_device() 2015-02-14 15:26 ` Russell King - ARM Linux (?) (?) @ 2015-02-14 15:27 ` Russell King -1 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Len Brown, Greg Kroah-Hartman, linux-pm pm_genpd_remove_device() should only be called with valid and present pm domain. There are circumstances where we may end up with something that isn't a generic PM domain in dev->pm_domain (eg, vga_switcheroo stuff.) Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/base/power/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index b3fbc21da2dc..11a1023fa64a 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd, dev_dbg(dev, "%s()\n", __func__); - if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev) + if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev) || IS_ERR_OR_NULL(dev->pm_domain) || pd_to_genpd(dev->pm_domain) != genpd) return -EINVAL; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-14 15:26 ` Russell King - ARM Linux ` (2 preceding siblings ...) (?) @ 2015-02-14 15:27 ` Russell King 2015-02-15 4:24 ` Ulf Hansson 2015-02-17 18:53 ` Rafael J. Wysocki -1 siblings, 2 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Len Brown, Greg Kroah-Hartman, linux-pm Synchronise the PM domain status with runtime PM's status. This provides a better solution to the problem than commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes"). The above commit added a call to power up the PM domain when a device attaches to the domain. The assumption is that the device driver will cause a runtime PM transition, which will synchronise the PM domain state with the runtime PM state. However, runtime PM will, by default, assume that the device is initially suspended. So we have two subsystems which have differing initial state expectations. This is silly. Runtime PM requires that pm_runtime_set_active() is called before pm_runtime_enable() if the device is already powered up. However, PM domains are not informed of this, and this is where the problem really lies. We need to keep PM domains properly and fully informed of their associated device status. We fix this by adding a new callback - runtime_set_status() which is triggered whenever a successful call to __pm_runtime_set_status(). PM domain code hooks into this, and updates the PM domain appropriately. This means a driver with the following sequence: pm_runtime_set_active(dev); pm_runtime_enable(dev); will trigger the PM domain to be powered up at this point, which keeps runtime PM and PM domains properly in sync with each other. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/base/power/domain.c | 16 +++++++++++++++- drivers/base/power/runtime.c | 5 +++++ include/linux/pm.h | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 11a1023fa64a..2a700cea41fc 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev) return 0; } +static int pm_genpd_runtime_set_status(struct device *dev) +{ + int ret; + + dev_dbg(dev, "%s()\n", __func__); + + if (pm_runtime_suspended(dev)) + ret = pm_genpd_runtime_suspend(dev); + else + ret = pm_genpd_runtime_resume(dev); + + return ret; +} + static bool pd_ignore_unused; static int __init pd_ignore_unused_setup(char *__unused) { @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd, genpd->max_off_time_changed = true; genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; + genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status; genpd->domain.ops.prepare = pm_genpd_prepare; genpd->domain.ops.suspend = pm_genpd_suspend; genpd->domain.ops.suspend_late = pm_genpd_suspend_late; @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; - pm_genpd_poweron(pd); return 0; } diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 5070c4fe8542..a958cae67a37 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) struct device *parent = dev->parent; unsigned long flags; bool notify_parent = false; + pm_callback_t callback; int error = 0; if (status != RPM_ACTIVE && status != RPM_SUSPENDED) @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) out_set: __update_runtime_status(dev, status); dev->power.runtime_error = 0; + if (dev->power.no_callbacks) + goto out; + callback = RPM_GET_CALLBACK(dev, runtime_set_status); + rpm_callback(callback, dev); out: spin_unlock_irqrestore(&dev->power.lock, flags); diff --git a/include/linux/pm.h b/include/linux/pm.h index 8b5976364619..ee098585d577 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -316,6 +316,7 @@ struct dev_pm_ops { int (*runtime_suspend)(struct device *dev); int (*runtime_resume)(struct device *dev); int (*runtime_idle)(struct device *dev); + int (*runtime_set_status)(struct device *dev); }; #ifdef CONFIG_PM_SLEEP -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-14 15:27 ` [PATCH 3/8] pm: domains: sync runtime PM status with PM domains Russell King @ 2015-02-15 4:24 ` Ulf Hansson 2015-02-17 18:53 ` Rafael J. Wysocki 1 sibling, 0 replies; 39+ messages in thread From: Ulf Hansson @ 2015-02-15 4:24 UTC (permalink / raw) To: Russell King Cc: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm Hi Russell, On 14 February 2015 at 16:27, Russell King <rmk+kernel@arm.linux.org.uk> wrote: > Synchronise the PM domain status with runtime PM's status. This > provides a better solution to the problem than commit 2ed127697eb1 > ("PM / Domains: Power on the PM domain right after attach completes"). > > The above commit added a call to power up the PM domain when a device > attaches to the domain. The assumption is that the device driver > will cause a runtime PM transition, which will synchronise the PM > domain state with the runtime PM state. > > However, runtime PM will, by default, assume that the device is > initially suspended. So we have two subsystems which have differing > initial state expectations. This is silly. > > Runtime PM requires that pm_runtime_set_active() is called before > pm_runtime_enable() if the device is already powered up. However, PM > domains are not informed of this, and this is where the problem really > lies. We need to keep PM domains properly and fully informed of their > associated device status. > > We fix this by adding a new callback - runtime_set_status() which is > triggered whenever a successful call to __pm_runtime_set_status(). > PM domain code hooks into this, and updates the PM domain appropriately. > > This means a driver with the following sequence: > > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > will trigger the PM domain to be powered up at this point, which keeps > runtime PM and PM domains properly in sync with each other. The issue you are trying to solve here was raised by me during the discussion around the below merged commit. "PM / Domains: Power on the PM domain right after attach completes" You may find the complete thread here: http://marc.info/?l=linux-pm&m=141623756506128&w=2 Especially I mention a "scenario 5", which is the issue you observed and what we decided to keep as a limitation. Sorry about that! Moreover, the below commit is also related to the similar issues and this fixed a regression. 67732cd34382 ( PM / Domains: Fix initial default state of the need_restore flag). I just wanted to make sure you have all the background to why we ended up with current solution... Regarding $subject patch, I think it's an interesting approach but I need some more time to think about it. Unfortunate I am OOF the next two weeks so will have to promise you to review this as soon as I get back. BTW, I would also appreciate if you could keep me on cc for any further changes related to genpd. Kind regards Uffe > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/base/power/domain.c | 16 +++++++++++++++- > drivers/base/power/runtime.c | 5 +++++ > include/linux/pm.h | 1 + > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 11a1023fa64a..2a700cea41fc 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev) > return 0; > } > > +static int pm_genpd_runtime_set_status(struct device *dev) > +{ > + int ret; > + > + dev_dbg(dev, "%s()\n", __func__); > + > + if (pm_runtime_suspended(dev)) > + ret = pm_genpd_runtime_suspend(dev); > + else > + ret = pm_genpd_runtime_resume(dev); > + > + return ret; > +} > + > static bool pd_ignore_unused; > static int __init pd_ignore_unused_setup(char *__unused) > { > @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd, > genpd->max_off_time_changed = true; > genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; > genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; > + genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status; > genpd->domain.ops.prepare = pm_genpd_prepare; > genpd->domain.ops.suspend = pm_genpd_suspend; > genpd->domain.ops.suspend_late = pm_genpd_suspend_late; > @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev) > } > > dev->pm_domain->detach = genpd_dev_pm_detach; > - pm_genpd_poweron(pd); > > return 0; > } > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 5070c4fe8542..a958cae67a37 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > struct device *parent = dev->parent; > unsigned long flags; > bool notify_parent = false; > + pm_callback_t callback; > int error = 0; > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > out_set: > __update_runtime_status(dev, status); > dev->power.runtime_error = 0; > + if (dev->power.no_callbacks) > + goto out; > + callback = RPM_GET_CALLBACK(dev, runtime_set_status); > + rpm_callback(callback, dev); > out: > spin_unlock_irqrestore(&dev->power.lock, flags); > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 8b5976364619..ee098585d577 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -316,6 +316,7 @@ struct dev_pm_ops { > int (*runtime_suspend)(struct device *dev); > int (*runtime_resume)(struct device *dev); > int (*runtime_idle)(struct device *dev); > + int (*runtime_set_status)(struct device *dev); > }; > > #ifdef CONFIG_PM_SLEEP > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-14 15:27 ` [PATCH 3/8] pm: domains: sync runtime PM status with PM domains Russell King 2015-02-15 4:24 ` Ulf Hansson @ 2015-02-17 18:53 ` Rafael J. Wysocki 2015-02-17 19:42 ` Alan Stern 2015-02-17 19:42 ` Russell King - ARM Linux 1 sibling, 2 replies; 39+ messages in thread From: Rafael J. Wysocki @ 2015-02-17 18:53 UTC (permalink / raw) To: Russell King Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Alan Stern First off, sorry for being slow to respond lately, I'm traveling now. Also adding Alan Stern to the CC (please always do that for discussions regarding the runtime PM core). On Saturday, February 14, 2015 03:27:40 PM Russell King wrote: > Synchronise the PM domain status with runtime PM's status. This > provides a better solution to the problem than commit 2ed127697eb1 > ("PM / Domains: Power on the PM domain right after attach completes"). > > The above commit added a call to power up the PM domain when a device > attaches to the domain. The assumption is that the device driver > will cause a runtime PM transition, which will synchronise the PM > domain state with the runtime PM state. > > However, runtime PM will, by default, assume that the device is > initially suspended. So we have two subsystems which have differing > initial state expectations. This is silly. > > Runtime PM requires that pm_runtime_set_active() is called before > pm_runtime_enable() if the device is already powered up. Well, this is an oversimplification of sorts. What really is expected is that the RPM status of the device will agree with its actual state at the moment when pm_runtime_enable() is called. If the actual state of the device is "not powered", then it is invalid to call pm_runtime_set_active() before pm_runtime_enable() even. > However, PM domains are not informed of this, and this is where the problem > really lies. We need to keep PM domains properly and fully informed of their > associated device status. This goes backwards. Rather, PM domains should tell everyone about what they have done to the device IMO. That is, since PM domains power up the device, it would be logical to change its RPM status at that point to me. That may lead to one subtle problem, though. Suppose that, in addition to either having or not having a power domain under it, the device has a clock that is managed directly by the driver. The driver may then want to do the clock management in its runtime PM callbacks. However, if genpd changes the device's RPM status to "active", the runtime PM framework will not execute the resume callback for the device, so things will break if the clock is not actually enabled to start with. IOW, we need a way for the driver and genpd to agree on what the RPM status of the device should be set to before pm_runtime_enable() is executed. > We fix this by adding a new callback - runtime_set_status() which is I'm not sure if that's the way to address that, though. Our intention for the pm_runtime_set_active() etc. calls was that they didn't trigger any callbacks, since were are supposed to be executed with runtime PM disabled. Moreover -> > triggered whenever a successful call to __pm_runtime_set_status(). > PM domain code hooks into this, and updates the PM domain appropriately. > > This means a driver with the following sequence: > > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > will trigger the PM domain to be powered up at this point, which keeps > runtime PM and PM domains properly in sync with each other. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/base/power/domain.c | 16 +++++++++++++++- > drivers/base/power/runtime.c | 5 +++++ > include/linux/pm.h | 1 + > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 11a1023fa64a..2a700cea41fc 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev) > return 0; > } > > +static int pm_genpd_runtime_set_status(struct device *dev) > +{ > + int ret; > + > + dev_dbg(dev, "%s()\n", __func__); > + > + if (pm_runtime_suspended(dev)) > + ret = pm_genpd_runtime_suspend(dev); > + else > + ret = pm_genpd_runtime_resume(dev); > + > + return ret; > +} > + > static bool pd_ignore_unused; > static int __init pd_ignore_unused_setup(char *__unused) > { > @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd, > genpd->max_off_time_changed = true; > genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; > genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; > + genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status; > genpd->domain.ops.prepare = pm_genpd_prepare; > genpd->domain.ops.suspend = pm_genpd_suspend; > genpd->domain.ops.suspend_late = pm_genpd_suspend_late; > @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev) > } > > dev->pm_domain->detach = genpd_dev_pm_detach; > - pm_genpd_poweron(pd); > > return 0; > } > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 5070c4fe8542..a958cae67a37 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > struct device *parent = dev->parent; > unsigned long flags; > bool notify_parent = false; > + pm_callback_t callback; > int error = 0; > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > out_set: > __update_runtime_status(dev, status); > dev->power.runtime_error = 0; > + if (dev->power.no_callbacks) > + goto out; > + callback = RPM_GET_CALLBACK(dev, runtime_set_status); > + rpm_callback(callback, dev); -> That is specific to PM domains and arguably not needed otherwise, so I would define it in struct dev_pm_domain outside of dev_pm_ops. > out: > spin_unlock_irqrestore(&dev->power.lock, flags); > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 8b5976364619..ee098585d577 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -316,6 +316,7 @@ struct dev_pm_ops { > int (*runtime_suspend)(struct device *dev); > int (*runtime_resume)(struct device *dev); > int (*runtime_idle)(struct device *dev); > + int (*runtime_set_status)(struct device *dev); > }; > > #ifdef CONFIG_PM_SLEEP > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-17 18:53 ` Rafael J. Wysocki @ 2015-02-17 19:42 ` Alan Stern 2015-02-17 19:42 ` Russell King - ARM Linux 1 sibling, 0 replies; 39+ messages in thread From: Alan Stern @ 2015-02-17 19:42 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Russell King, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Tue, 17 Feb 2015, Rafael J. Wysocki wrote: > On Saturday, February 14, 2015 03:27:40 PM Russell King wrote: > > Synchronise the PM domain status with runtime PM's status. This > > provides a better solution to the problem than commit 2ed127697eb1 > > ("PM / Domains: Power on the PM domain right after attach completes"). > > > > The above commit added a call to power up the PM domain when a device > > attaches to the domain. The assumption is that the device driver > > will cause a runtime PM transition, which will synchronise the PM > > domain state with the runtime PM state. > > > > However, runtime PM will, by default, assume that the device is > > initially suspended. So we have two subsystems which have differing > > initial state expectations. This is silly. As Rafael says, that's putting it a little strongly. The runtime PM status is initialized to RPM_SUSPENDED, simply because it has to be initialized to _something_. But the core doesn't expect that value to be meaningful initially. > > Runtime PM requires that pm_runtime_set_active() is called before > > pm_runtime_enable() if the device is already powered up. > > Well, this is an oversimplification of sorts. > > What really is expected is that the RPM status of the device will agree > with its actual state at the moment when pm_runtime_enable() is called. > > If the actual state of the device is "not powered", then it is invalid to > call pm_runtime_set_active() before pm_runtime_enable() even. In practice it wouldn't make any difference, so long as pm_runtime_set_suspended() is called later on but before pm_runtime_enable(). > > However, PM domains are not informed of this, and this is where the problem > > really lies. We need to keep PM domains properly and fully informed of their > > associated device status. > > This goes backwards. Rather, PM domains should tell everyone about what they > have done to the device IMO. That is, since PM domains power up the device, > it would be logical to change its RPM status at that point to me. > > That may lead to one subtle problem, though. > > Suppose that, in addition to either having or not having a power domain under > it, the device has a clock that is managed directly by the driver. The driver > may then want to do the clock management in its runtime PM callbacks. > However, if genpd changes the device's RPM status to "active", the runtime > PM framework will not execute the resume callback for the device, so things > will break if the clock is not actually enabled to start with. > > IOW, we need a way for the driver and genpd to agree on what the RPM status > of the device should be set to before pm_runtime_enable() is executed. Agreed. For example, the PM domain may simply have a policy that all devices must start out in the active state, and require member drivers to enforce that policy. Or the PM domain might directly invoke a driver's runtime-PM callback routine if the device appears to be in the "wrong" state initially. > > We fix this by adding a new callback - runtime_set_status() which is > > I'm not sure if that's the way to address that, though. > > Our intention for the pm_runtime_set_active() etc. calls was that they didn't > trigger any callbacks, since were are supposed to be executed with runtime > PM disabled. > > Moreover -> ... > -> That is specific to PM domains and arguably not needed otherwise, so I would > define it in struct dev_pm_domain outside of dev_pm_ops. My thought exactly. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-17 18:53 ` Rafael J. Wysocki 2015-02-17 19:42 ` Alan Stern @ 2015-02-17 19:42 ` Russell King - ARM Linux 2015-02-18 7:02 ` Rafael J. Wysocki 1 sibling, 1 reply; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-17 19:42 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Alan Stern On Tue, Feb 17, 2015 at 07:53:47PM +0100, Rafael J. Wysocki wrote: > First off, sorry for being slow to respond lately, I'm traveling now. > > Also adding Alan Stern to the CC (please always do that for discussions > regarding the runtime PM core). If that is the case, how about adding them into MAINTAINERS? Without this information in there, Alan will probably be forgotten for future patches. > On Saturday, February 14, 2015 03:27:40 PM Russell King wrote: > > Runtime PM requires that pm_runtime_set_active() is called before > > pm_runtime_enable() if the device is already powered up. > > Well, this is an oversimplification of sorts. > > What really is expected is that the RPM status of the device will agree > with its actual state at the moment when pm_runtime_enable() is called. > > If the actual state of the device is "not powered", then it is invalid to > call pm_runtime_set_active() before pm_runtime_enable() even. We're both saying exactly the same thing, so we're in full agreement, which is good. :) > > However, PM domains are not informed of this, and this is where the problem > > really lies. We need to keep PM domains properly and fully informed of their > > associated device status. > > This goes backwards. Rather, PM domains should tell everyone about > what they have done to the device IMO. That is, since PM domains > power up the device, it would be logical to change its RPM status at > that point to me. Right, so it may make sense for runtime PM to query the PM domain state, and synchronise itself with that. > That may lead to one subtle problem, though. > > Suppose that, in addition to either having or not having a power domain > under it, the device has a clock that is managed directly by the driver. > The driver may then want to do the clock management in its runtime PM > callbacks. However, if genpd changes the device's RPM status to "active", > the runtime PM framework will not execute the resume callback for the > device, so things will break if the clock is not actually enabled to > start with. > > IOW, we need a way for the driver and genpd to agree on what the RPM status > of the device should be set to before pm_runtime_enable() is executed. It's worse than that. If, in the probe, we decide at a point to query the PM domain status, and transfer it into the RPM status, how does the driver know whether it needs to do a "put" to cause a transition from active to suspended? In the case where the PM domain was suspended, the RPM status would also be suspended at that point, and it requires a RPM get to resume it. If the PM domain was active, then it would require a pm_runtime_put() operation to allow it to suspend. This is why I decided that the methodology in the runtime PM code should apply to PM domains: runtime PM requires the actual state to match the runtime PM state prior to calling pm_runtime_enable(). We should also require that the PM domain state must also match the runtime PM state prior to runtime PM being enabled too. > > We fix this by adding a new callback - runtime_set_status() which is > > I'm not sure if that's the way to address that, though. I've come to the conclusion that this isn't a good way to handle it, because those drivers which may make use of PM domains without using runtime PM will be probed with the PM domain powered down. I think we've got a rather sticky problem here, and my proposed solution will cause problems in that scenario. Another possibility may be to trigger PM domains to try to power down the domain when it sees the driver call pm_runtime_enable(). If the driver never calls pm_runtime_enable(), the PM domain will be left active. If it does call this function, the effect of this will be to synchronise the PM domain with the runtime PM state. > Our intention for the pm_runtime_set_active() etc. calls was that they didn't > trigger any callbacks, since were are supposed to be executed with runtime > PM disabled. > > Moreover -> > > > triggered whenever a successful call to __pm_runtime_set_status(). > > PM domain code hooks into this, and updates the PM domain appropriately. > > > > This means a driver with the following sequence: > > > > pm_runtime_set_active(dev); > > pm_runtime_enable(dev); > > > > will trigger the PM domain to be powered up at this point, which keeps > > runtime PM and PM domains properly in sync with each other. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > drivers/base/power/domain.c | 16 +++++++++++++++- > > drivers/base/power/runtime.c | 5 +++++ > > include/linux/pm.h | 1 + > > 3 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 11a1023fa64a..2a700cea41fc 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -741,6 +741,20 @@ static int pm_genpd_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int pm_genpd_runtime_set_status(struct device *dev) > > +{ > > + int ret; > > + > > + dev_dbg(dev, "%s()\n", __func__); > > + > > + if (pm_runtime_suspended(dev)) > > + ret = pm_genpd_runtime_suspend(dev); > > + else > > + ret = pm_genpd_runtime_resume(dev); > > + > > + return ret; > > +} > > + > > static bool pd_ignore_unused; > > static int __init pd_ignore_unused_setup(char *__unused) > > { > > @@ -1907,6 +1921,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd, > > genpd->max_off_time_changed = true; > > genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend; > > genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume; > > + genpd->domain.ops.runtime_set_status = pm_genpd_runtime_set_status; > > genpd->domain.ops.prepare = pm_genpd_prepare; > > genpd->domain.ops.suspend = pm_genpd_suspend; > > genpd->domain.ops.suspend_late = pm_genpd_suspend_late; > > @@ -2223,7 +2238,6 @@ int genpd_dev_pm_attach(struct device *dev) > > } > > > > dev->pm_domain->detach = genpd_dev_pm_detach; > > - pm_genpd_poweron(pd); > > > > return 0; > > } > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index 5070c4fe8542..a958cae67a37 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -981,6 +981,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > > struct device *parent = dev->parent; > > unsigned long flags; > > bool notify_parent = false; > > + pm_callback_t callback; > > int error = 0; > > > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > > @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > > out_set: > > __update_runtime_status(dev, status); > > dev->power.runtime_error = 0; > > + if (dev->power.no_callbacks) > > + goto out; > > + callback = RPM_GET_CALLBACK(dev, runtime_set_status); > > + rpm_callback(callback, dev); > > -> That is specific to PM domains and arguably not needed otherwise, > so I would define it in struct dev_pm_domain outside of dev_pm_ops. How does runtime PM know that we're using PM domains though? How does runtime PM know that it can cast the dev_pm_ops pointer safely? -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-17 19:42 ` Russell King - ARM Linux @ 2015-02-18 7:02 ` Rafael J. Wysocki 2015-02-18 10:03 ` Russell King - ARM Linux 0 siblings, 1 reply; 39+ messages in thread From: Rafael J. Wysocki @ 2015-02-18 7:02 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Alan Stern On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote: > On Tue, Feb 17, 2015 at 07:53:47PM +0100, Rafael J. Wysocki wrote: [cut] > > > > What really is expected is that the RPM status of the device will agree > > with its actual state at the moment when pm_runtime_enable() is called. > > > > If the actual state of the device is "not powered", then it is invalid to > > call pm_runtime_set_active() before pm_runtime_enable() even. > > We're both saying exactly the same thing, so we're in full agreement, > which is good. :) > > > > However, PM domains are not informed of this, and this is where the problem > > > really lies. We need to keep PM domains properly and fully informed of their > > > associated device status. > > > > This goes backwards. Rather, PM domains should tell everyone about > > what they have done to the device IMO. That is, since PM domains > > power up the device, it would be logical to change its RPM status at > > that point to me. > > Right, so it may make sense for runtime PM to query the PM domain state, > and synchronise itself with that. > > > That may lead to one subtle problem, though. > > > > Suppose that, in addition to either having or not having a power domain > > under it, the device has a clock that is managed directly by the driver. > > The driver may then want to do the clock management in its runtime PM > > callbacks. However, if genpd changes the device's RPM status to "active", > > the runtime PM framework will not execute the resume callback for the > > device, so things will break if the clock is not actually enabled to > > start with. > > > > IOW, we need a way for the driver and genpd to agree on what the RPM status > > of the device should be set to before pm_runtime_enable() is executed. > > It's worse than that. If, in the probe, we decide at a point to query > the PM domain status, and transfer it into the RPM status, how does the > driver know whether it needs to do a "put" to cause a transition from > active to suspended? When ->probe() runs, the cases are: (1) There are no power domains, so the device is "active" when the clock is enabled and "suspended" when it is disabled. (2) There is a power domain which is initially off. The RPM status of the device has to be "suspended" or the domain needs to be powered up. (3) There is a power domain which is initially on. The RPM status of the device depends on the clock state like in (1). Also it may not be possible to power down the domain. Essentially, (1) and (3) are equivalent from the ->probe() perspective. Moreover, if the driver does not intend to enable the clock, the device is "suspended" regardless of the domain state. This means that the only really interesting case is (2) and only when ->probe() will attempt to enable the clock. In that case it needs to do something like (a) Bump up the device's runtime PM usage counter. (b) Execute "power up the device for me" (missing). (c) Enable the clock, do whatever it wants to the device, set the RPM status to "active" and call pm_runtime_enable(). (d) Drop down the device's runtime PM usage counter (the core will trigger suspend). > In the case where the PM domain was suspended, the RPM status would also > be suspended at that point, and it requires a RPM get to resume it. Yes, it does, if the driver wants to access the device. > If the PM domain was active, then it would require a pm_runtime_put() > operation to allow it to suspend. > > This is why I decided that the methodology in the runtime PM code should > apply to PM domains: runtime PM requires the actual state to match the > runtime PM state prior to calling pm_runtime_enable(). We should also > require that the PM domain state must also match the runtime PM state > prior to runtime PM being enabled too. Agreed. > > > We fix this by adding a new callback - runtime_set_status() which is > > > > I'm not sure if that's the way to address that, though. > > I've come to the conclusion that this isn't a good way to handle it, > because those drivers which may make use of PM domains without using > runtime PM will be probed with the PM domain powered down. What would they use PM domains for, then? PM_RUNTIME is gone now and PM is implied by PM_SLEEP, so you can't have system suspend support without runtime PM (which presumably might be the case in question). > I think we've got a rather sticky problem here, and my proposed > solution will cause problems in that scenario. > > Another possibility may be to trigger PM domains to try to power down > the domain when it sees the driver call pm_runtime_enable(). If the > driver never calls pm_runtime_enable(), the PM domain will be left > active. If it does call this function, the effect of this will be to > synchronise the PM domain with the runtime PM state. That's more along the lines of what I'm thinking about. I don't have a clean idea how to implement that, though. > > Our intention for the pm_runtime_set_active() etc. calls was that they didn't > > trigger any callbacks, since were are supposed to be executed with runtime > > PM disabled. > > > > Moreover -> [cut] > > > @@ -1029,6 +1030,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > > > out_set: > > > __update_runtime_status(dev, status); > > > dev->power.runtime_error = 0; > > > + if (dev->power.no_callbacks) > > > + goto out; > > > + callback = RPM_GET_CALLBACK(dev, runtime_set_status); > > > + rpm_callback(callback, dev); > > > > -> That is specific to PM domains and arguably not needed otherwise, > > so I would define it in struct dev_pm_domain outside of dev_pm_ops. > > How does runtime PM know that we're using PM domains though? How > does runtime PM know that it can cast the dev_pm_ops pointer safely? dev->pm_domain is set then, so you basically do if (dev->pm_domain && dev->pm_domain->runtime_set_status) dev->pm_domain->runtime_set_status(dev); (or whatever the new callback may be). And if power domains are in use, dev->pm_domain has to be set before doing anything with runtime PM to the device or things may break in more interesting ways. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 7:02 ` Rafael J. Wysocki @ 2015-02-18 10:03 ` Russell King - ARM Linux 2015-02-18 15:12 ` Alan Stern 2015-02-18 16:46 ` Rafael J. Wysocki 0 siblings, 2 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-18 10:03 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Alan Stern On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote: > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote: > > It's worse than that. If, in the probe, we decide at a point to query > > the PM domain status, and transfer it into the RPM status, how does the > > driver know whether it needs to do a "put" to cause a transition from > > active to suspended? > > When ->probe() runs, the cases are: > (1) There are no power domains, so the device is "active" when the clock is > enabled and "suspended" when it is disabled. > (2) There is a power domain which is initially off. The RPM status of the > device has to be "suspended" or the domain needs to be powered up. (quick reply) That's not entirely correct. As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right after attach completes) the power domain will _always_ be powered on prior to ->probe() being called, if the device was attached to the PM domain just before ->probe() is called, inspite of the domain being powered off before the device was attached to the domain. If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is called (before they're attached, but in the kernel boot sequence) the work for powering down the PM domains will be queued until a point where it can be scheduled - which will be after devices are attached. That can allow the PM domain(s) to be powered down (as no driver is attached) which then results in the driver's ->probe function being called with the PM domain OFF. So, right now, there's no way for a driver to know with certainty whether the device it is about to probe is powered up or powered down. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 10:03 ` Russell King - ARM Linux @ 2015-02-18 15:12 ` Alan Stern 2015-02-18 15:42 ` Russell King - ARM Linux 2015-02-18 16:52 ` Rafael J. Wysocki 2015-02-18 16:46 ` Rafael J. Wysocki 1 sibling, 2 replies; 39+ messages in thread From: Alan Stern @ 2015-02-18 15:12 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Rafael J. Wysocki, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Wed, 18 Feb 2015, Russell King - ARM Linux wrote: > On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote: > > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote: > > > It's worse than that. If, in the probe, we decide at a point to query > > > the PM domain status, and transfer it into the RPM status, how does the > > > driver know whether it needs to do a "put" to cause a transition from > > > active to suspended? > > > > When ->probe() runs, the cases are: > > (1) There are no power domains, so the device is "active" when the clock is > > enabled and "suspended" when it is disabled. > > (2) There is a power domain which is initially off. The RPM status of the > > device has to be "suspended" or the domain needs to be powered up. > > (quick reply) > > That's not entirely correct. > > As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right > after attach completes) the power domain will _always_ be powered on prior > to ->probe() being called, if the device was attached to the PM domain > just before ->probe() is called, inspite of the domain being powered off > before the device was attached to the domain. > > If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is > called (before they're attached, but in the kernel boot sequence) the > work for powering down the PM domains will be queued until a point where > it can be scheduled - which will be after devices are attached. That can > allow the PM domain(s) to be powered down (as no driver is attached) which > then results in the driver's ->probe function being called with the PM > domain OFF. > > So, right now, there's no way for a driver to know with certainty whether > the device it is about to probe is powered up or powered down. Russell: I'm not totally familiar with how PM domains are intended to work. The impression I get from looking through the code is that they are highly over-engineered -- but that could just be a result of my ignorance. At any rate, I don't have a clear picture of the problem you are trying to solve. What's the general outline? Are you talking about a device that _belongs_ to a PM domain or one that _depends_ on a PM domain? If the device belongs to the domain, how does it get added (i.e., by its driver during probe or by some other mechanism)? Are you asking about how the driver can tell what the PM domain's runtime status is? Or would you like to add a way for the driver to set the PM domain's runtime status? What else am I missing? Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 15:12 ` Alan Stern @ 2015-02-18 15:42 ` Russell King - ARM Linux 2015-02-18 16:52 ` Rafael J. Wysocki 1 sibling, 0 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-18 15:42 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Wed, Feb 18, 2015 at 10:12:17AM -0500, Alan Stern wrote: > Russell: > > I'm not totally familiar with how PM domains are intended to work. The > impression I get from looking through the code is that they are highly > over-engineered -- but that could just be a result of my ignorance. > > At any rate, I don't have a clear picture of the problem you are trying > to solve. What's the general outline? Are you talking about a device > that _belongs_ to a PM domain or one that _depends_ on a PM domain? > If the device belongs to the domain, how does it get added (i.e., by > its driver during probe or by some other mechanism)? > > Are you asking about how the driver can tell what the PM domain's > runtime status is? Or would you like to add a way for the driver to > set the PM domain's runtime status? > > What else am I missing? The problem is that the PM domain stuff doesn't work very well, and I believe it's problems are just being hacked around rather than being viewed from a higher level and fixed properly. :) The issue that I'm seeing with 3.19 is that I have a device which does not need to be powered up to probe. It's default state when the kernel initially starts to boot is that the power domain associated with it is powered down. Hence, the driver is written such that its probe function does this: pm_runtime_use_autosuspend(vi->dev); pm_runtime_set_autosuspend_delay(vi->dev, 100); pm_runtime_enable(vi->dev); The driver lives as a module on the filesystem, and so it's loaded after the kernel has mounted its rootfs and udev has started. Now, I boot this platform in two ways: I have a DT mode (which I have found is not that stable - in that it causes problems with HDMI), and a legacy mode. When I boot in legacy mode, the PM domain is created early in the kernel boot (at arch_initcall time). As part of the PM domain initialisation, I read the current state of the hardware and tell PM domains whether it was enabled or not. In this case, it starts off disabled. Right after they're created, I call pm_genpd_poweroff_unused() and then register a notifier for platform devices. The call to pm_genpd_poweroff_unused() causes a work to be queued, which will only get executed when we reach a scheduling or preemption point. The platform devices then get created, and the notifier attaches the appropriate PM domains to the devices. In 3.18 and previous kernels, this resulted in the PM domains being left in their initial state. This means that the presumption by the driver that the device is suspended is correct and valid. In 3.19, this causes the PM domain to be powered up immediately. However, because we get to a preemption or scheduling point before the driver module is inserted, this allows the PM domain to power back down. The module is then loaded, and everything works correctly. The PM domain debugfs file indicates that the PM domain is off. Now, if I boot in DT mode, the game changes. In this case, the PM domains are created as above, but without the platform device notifier - we rely on the generic code to attach the PM domain to the device a driver probe time. What this means is that when the pm_genpd_poweroff_unused() work executes, it finds that the domain is already powered down. The module is then loaded as in the legacy case, and at this point, the PM domain is attached to the device just before the driver is probed. This causes the device to be powered on at this point. However, the runtime PM state remains as its initialisation default - suspended. Time passes, and the PM domain debug file continues to indicate that the runtime PM state of the device is suspended, but the PM domain state is "on". So, if I boot in legacy mode, things work fine, but only because of the fluke that the pm_genpd_poweroff_unused() happens to be executed before the device is probed, undoing the powering up of the domain at attachment time. If I boot in DT mode, the order changes, and I'm left with the PM domain and RPM in an inconsistent state. Right now, there is _no_ way for a device driver to know whether the PM domain attached to its device is powered up or not. As runtime PM needs to know (from the device driver) the initial state of the device, the device driver can not determine whether it is powered up or not. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 15:12 ` Alan Stern 2015-02-18 15:42 ` Russell King - ARM Linux @ 2015-02-18 16:52 ` Rafael J. Wysocki 2015-02-18 17:26 ` Russell King - ARM Linux 1 sibling, 1 reply; 39+ messages in thread From: Rafael J. Wysocki @ 2015-02-18 16:52 UTC (permalink / raw) To: Alan Stern Cc: Russell King - ARM Linux, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Wednesday, February 18, 2015 10:12:17 AM Alan Stern wrote: > On Wed, 18 Feb 2015, Russell King - ARM Linux wrote: > > > On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote: > > > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote: > > > > It's worse than that. If, in the probe, we decide at a point to query > > > > the PM domain status, and transfer it into the RPM status, how does the > > > > driver know whether it needs to do a "put" to cause a transition from > > > > active to suspended? > > > > > > When ->probe() runs, the cases are: > > > (1) There are no power domains, so the device is "active" when the clock is > > > enabled and "suspended" when it is disabled. > > > (2) There is a power domain which is initially off. The RPM status of the > > > device has to be "suspended" or the domain needs to be powered up. > > > > (quick reply) > > > > That's not entirely correct. > > > > As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right > > after attach completes) the power domain will _always_ be powered on prior > > to ->probe() being called, if the device was attached to the PM domain > > just before ->probe() is called, inspite of the domain being powered off > > before the device was attached to the domain. > > > > If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is > > called (before they're attached, but in the kernel boot sequence) the > > work for powering down the PM domains will be queued until a point where > > it can be scheduled - which will be after devices are attached. That can > > allow the PM domain(s) to be powered down (as no driver is attached) which > > then results in the driver's ->probe function being called with the PM > > domain OFF. > > > > So, right now, there's no way for a driver to know with certainty whether > > the device it is about to probe is powered up or powered down. > > Russell: > > I'm not totally familiar with how PM domains are intended to work. The > impression I get from looking through the code is that they are highly > over-engineered Well, that very likely is my fault. :-) > -- but that could just be a result of my ignorance. You may be right anyway, though ... > At any rate, I don't have a clear picture of the problem you are trying > to solve. What's the general outline? Are you talking about a device > that _belongs_ to a PM domain or one that _depends_ on a PM domain? > If the device belongs to the domain, how does it get added (i.e., by > its driver during probe or by some other mechanism)? > > Are you asking about how the driver can tell what the PM domain's > runtime status is? Or would you like to add a way for the driver to > set the PM domain's runtime status? The problem, as I see it, is that drivers have certain expectations about the initial power states of devices that may or may not be met if power domains are in use. Those drivers might have been developed on systems without direct control of power domains where the conditions for the driver at the ->probe() time were always the same. With power domains, though, they may change depending on what's going on with the other devices in the domain, for example. So the question is how we can address that in the cleanest way possible. Rafael ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 16:52 ` Rafael J. Wysocki @ 2015-02-18 17:26 ` Russell King - ARM Linux 2015-02-18 18:05 ` Rafael J. Wysocki 2015-02-18 18:42 ` Alan Stern 0 siblings, 2 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-18 17:26 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Alan Stern, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Wed, Feb 18, 2015 at 05:52:25PM +0100, Rafael J. Wysocki wrote: > Those drivers might have been developed on systems without direct control of > power domains where the conditions for the driver at the ->probe() time were > always the same. With power domains, though, they may change depending on > what's going on with the other devices in the domain, for example. > > So the question is how we can address that in the cleanest way possible. Okay, going back to what was said earlier, we have three possibilities for drivers: 1) A driver which makes no use of runtime PM and no use of PM domains. 2) A driver which makes no use of runtime PM but may have a PM domain attached. 3) A driver which uses runtime PM, and assumes that the device was initially suspended. It calls pm_enable_device() optionally with a preceding call to pm_runtime_set_suspended(). 4) A driver which uses runtime PM, and calls pm_runtime_set_active() followed by pm_enable_device(). What we want to end up with in the ideal situation is that drivers which fall into class: 1 - may see their devices in any state; it is up to them to deal with whatever state the device is initially in. 2 - should see their devices in a powered up state as they have no way to inform that the device is active. 3 - should see their devices in a suspended state. 4 - should see their devices in a powered up state. The problem here is that we have no way to know this prior to the drivers probe function being called. Whatever we do at this point, it is not going to satisfy the requirements of everyone. So, let's take what we're currently doing on DT, and make it the same across the board. In platform_drv_probe(), let's do this: /* attach the domain */ ret = dev_pm_domain_attach(_dev, true); if (ret == -EPROBE_DEFER) goto defer; /* power up the domain - and hold it powered up */ ret = dev_pm_domain_pre_probe(_dev); if (ret != -ENOSYS) goto error; ret = drv->probe(dev); /* * remove the "hold" on the domain by this device, and start * tracking its runtime PM status. */ dev_pm_domain_post_probe(_dev); if (ret) dev_pm_domain_detach(_dev, true); What this means is that while the probe function is running, we guarantee that the PM domain will always be powered up. We also guarantee that after the probe function has returned, we will then start tracking the runtime PM state, and if the device driver leaves runtime PM in the "suspended" state, PM domains will get to hear about it at that point, and can transition the PM domain back to "off" mode. Both these transitions only cause the PM domain to be affected; no runtime PM callbacks are issued for either of these two changes. (We already have that for the initial state right now where attaching a device to the PM domain causes the PM domain to be powered up.) This is merely an extension of the current scheme. Think of dev_pm_domain_post_probe() as a method of synchronising the state of PM domains with the state of runtime PM across all attached devices. Aside: I need to do some further checks; while considering this, I think I've come across a worrying problem with "PM / Domains: Power on the PM domain right after attach completes". I think this will result in the driver's runtime_resume callback being invoked _before_ the probe function. I can't test this right now as I'm in the middle of upgrading my dev box and it doesn't have a functional install for building ARM kernels yet. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 17:26 ` Russell King - ARM Linux @ 2015-02-18 18:05 ` Rafael J. Wysocki 2015-03-04 19:57 ` Ulf Hansson 2015-02-18 18:42 ` Alan Stern 1 sibling, 1 reply; 39+ messages in thread From: Rafael J. Wysocki @ 2015-02-18 18:05 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Alan Stern, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Wednesday, February 18, 2015 05:26:51 PM Russell King - ARM Linux wrote: > On Wed, Feb 18, 2015 at 05:52:25PM +0100, Rafael J. Wysocki wrote: > > Those drivers might have been developed on systems without direct control of > > power domains where the conditions for the driver at the ->probe() time were > > always the same. With power domains, though, they may change depending on > > what's going on with the other devices in the domain, for example. > > > > So the question is how we can address that in the cleanest way possible. > > Okay, going back to what was said earlier, we have three possibilities > for drivers: > > 1) A driver which makes no use of runtime PM and no use of PM domains. > 2) A driver which makes no use of runtime PM but may have a PM domain > attached. > 3) A driver which uses runtime PM, and assumes that the device was > initially suspended. It calls pm_enable_device() optionally with > a preceding call to pm_runtime_set_suspended(). > 4) A driver which uses runtime PM, and calls pm_runtime_set_active() > followed by pm_enable_device(). > > What we want to end up with in the ideal situation is that drivers which > fall into class: > > 1 - may see their devices in any state; it is up to them to deal with > whatever state the device is initially in. > 2 - should see their devices in a powered up state as they have no way > to inform that the device is active. > 3 - should see their devices in a suspended state. > 4 - should see their devices in a powered up state. > > The problem here is that we have no way to know this prior to the drivers > probe function being called. Whatever we do at this point, it is not > going to satisfy the requirements of everyone. > > So, let's take what we're currently doing on DT, and make it the same > across the board. In platform_drv_probe(), let's do this: > > /* attach the domain */ > ret = dev_pm_domain_attach(_dev, true); > if (ret == -EPROBE_DEFER) > goto defer; > > /* power up the domain - and hold it powered up */ > ret = dev_pm_domain_pre_probe(_dev); > if (ret != -ENOSYS) > goto error; > > ret = drv->probe(dev); > > /* > * remove the "hold" on the domain by this device, and start > * tracking its runtime PM status. > */ > dev_pm_domain_post_probe(_dev); > > if (ret) > dev_pm_domain_detach(_dev, true); > > What this means is that while the probe function is running, we guarantee > that the PM domain will always be powered up. We also guarantee that > after the probe function has returned, we will then start tracking the > runtime PM state, and if the device driver leaves runtime PM in the > "suspended" state, PM domains will get to hear about it at that point, > and can transition the PM domain back to "off" mode. > > Both these transitions only cause the PM domain to be affected; no > runtime PM callbacks are issued for either of these two changes. (We > already have that for the initial state right now where attaching a > device to the PM domain causes the PM domain to be powered up.) > > This is merely an extension of the current scheme. Think of > dev_pm_domain_post_probe() as a method of synchronising the state of > PM domains with the state of runtime PM across all attached devices. I like this. And I would add two new "pre_probe" and "post_probe" (what about "init" and "remove"?) to struct dev_pm_domain for that. The "pre_probe" ("init") thing would be useful for solving one more problem related to PM domains I have elsewhere. In that case I need to defer probing one of the device in the domain until one of the other devices is registered. Rafael ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 18:05 ` Rafael J. Wysocki @ 2015-03-04 19:57 ` Ulf Hansson 2015-03-04 20:11 ` Russell King - ARM Linux 0 siblings, 1 reply; 39+ messages in thread From: Ulf Hansson @ 2015-03-04 19:57 UTC (permalink / raw) To: Rafael J. Wysocki, Russell King - ARM Linux, Alan Stern Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Kevin Hilman On 18 February 2015 at 19:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, February 18, 2015 05:26:51 PM Russell King - ARM Linux wrote: >> On Wed, Feb 18, 2015 at 05:52:25PM +0100, Rafael J. Wysocki wrote: >> > Those drivers might have been developed on systems without direct control of >> > power domains where the conditions for the driver at the ->probe() time were >> > always the same. With power domains, though, they may change depending on >> > what's going on with the other devices in the domain, for example. >> > >> > So the question is how we can address that in the cleanest way possible. >> >> Okay, going back to what was said earlier, we have three possibilities >> for drivers: >> >> 1) A driver which makes no use of runtime PM and no use of PM domains. >> 2) A driver which makes no use of runtime PM but may have a PM domain >> attached. >> 3) A driver which uses runtime PM, and assumes that the device was >> initially suspended. It calls pm_enable_device() optionally with >> a preceding call to pm_runtime_set_suspended(). >> 4) A driver which uses runtime PM, and calls pm_runtime_set_active() >> followed by pm_enable_device(). >> >> What we want to end up with in the ideal situation is that drivers which >> fall into class: >> >> 1 - may see their devices in any state; it is up to them to deal with >> whatever state the device is initially in. >> 2 - should see their devices in a powered up state as they have no way >> to inform that the device is active. >> 3 - should see their devices in a suspended state. >> 4 - should see their devices in a powered up state. >> >> The problem here is that we have no way to know this prior to the drivers >> probe function being called. Whatever we do at this point, it is not >> going to satisfy the requirements of everyone. >> >> So, let's take what we're currently doing on DT, and make it the same >> across the board. In platform_drv_probe(), let's do this: >> >> /* attach the domain */ >> ret = dev_pm_domain_attach(_dev, true); >> if (ret == -EPROBE_DEFER) >> goto defer; >> >> /* power up the domain - and hold it powered up */ >> ret = dev_pm_domain_pre_probe(_dev); >> if (ret != -ENOSYS) >> goto error; >> >> ret = drv->probe(dev); >> >> /* >> * remove the "hold" on the domain by this device, and start >> * tracking its runtime PM status. >> */ >> dev_pm_domain_post_probe(_dev); >> >> if (ret) >> dev_pm_domain_detach(_dev, true); >> >> What this means is that while the probe function is running, we guarantee >> that the PM domain will always be powered up. We also guarantee that >> after the probe function has returned, we will then start tracking the >> runtime PM state, and if the device driver leaves runtime PM in the >> "suspended" state, PM domains will get to hear about it at that point, >> and can transition the PM domain back to "off" mode. >> >> Both these transitions only cause the PM domain to be affected; no >> runtime PM callbacks are issued for either of these two changes. (We >> already have that for the initial state right now where attaching a >> device to the PM domain causes the PM domain to be powered up.) >> >> This is merely an extension of the current scheme. Think of >> dev_pm_domain_post_probe() as a method of synchronising the state of >> PM domains with the state of runtime PM across all attached devices. > > I like this. Me too! > > And I would add two new "pre_probe" and "post_probe" (what about "init" > and "remove"?) to struct dev_pm_domain for that. Actually what Russell propose, is almost exactly what I proposed in the following below patchset way back. At that point we decided to reject that approach. The only difference is the naming of dev_pm_domain_pre|post_probe(). I decided to name them dev_pm_domain_get|put(). :-) http://marc.info/?l=linux-pm&m=141320895122707&w=2 > > The "pre_probe" ("init") thing would be useful for solving one more problem > related to PM domains I have elsewhere. In that case I need to defer probing > one of the device in the domain until one of the other devices is registered. > > Rafael > So I wonder if I should maybe refresh that patchset? Or maybe Russell would like to pick them up? No matter what, I happy to help! Kind regards Uffe ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-03-04 19:57 ` Ulf Hansson @ 2015-03-04 20:11 ` Russell King - ARM Linux 0 siblings, 0 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-03-04 20:11 UTC (permalink / raw) To: Ulf Hansson Cc: Rafael J. Wysocki, Alan Stern, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Kevin Hilman On Wed, Mar 04, 2015 at 08:57:40PM +0100, Ulf Hansson wrote: > So I wonder if I should maybe refresh that patchset? Or maybe Russell > would like to pick them up? I'm in no state to do anything at the moment - see my mail last night "rmk's taking it easy for a while" (which everyone seems to have ignored). -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 17:26 ` Russell King - ARM Linux 2015-02-18 18:05 ` Rafael J. Wysocki @ 2015-02-18 18:42 ` Alan Stern 1 sibling, 0 replies; 39+ messages in thread From: Alan Stern @ 2015-02-18 18:42 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Rafael J. Wysocki, Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm On Wed, 18 Feb 2015, Russell King - ARM Linux wrote: > So, let's take what we're currently doing on DT, and make it the same > across the board. In platform_drv_probe(), let's do this: > > /* attach the domain */ > ret = dev_pm_domain_attach(_dev, true); > if (ret == -EPROBE_DEFER) > goto defer; > > /* power up the domain - and hold it powered up */ > ret = dev_pm_domain_pre_probe(_dev); > if (ret != -ENOSYS) > goto error; > > ret = drv->probe(dev); > > /* > * remove the "hold" on the domain by this device, and start > * tracking its runtime PM status. > */ > dev_pm_domain_post_probe(_dev); > > if (ret) > dev_pm_domain_detach(_dev, true); > > What this means is that while the probe function is running, we guarantee > that the PM domain will always be powered up. We also guarantee that > after the probe function has returned, we will then start tracking the > runtime PM state, and if the device driver leaves runtime PM in the > "suspended" state, PM domains will get to hear about it at that point, > and can transition the PM domain back to "off" mode. > > Both these transitions only cause the PM domain to be affected; no > runtime PM callbacks are issued for either of these two changes. (We > already have that for the initial state right now where attaching a > device to the PM domain causes the PM domain to be powered up.) > > This is merely an extension of the current scheme. Think of > dev_pm_domain_post_probe() as a method of synchronising the state of > PM domains with the state of runtime PM across all attached devices. This seems like the right sort of approach. The Runtime PM core has a general philosophy that it can never force anything to be in the suspended state. Therefore if you need something to be in a well-defined power state for any length of time (or if you need to know the power state), you have to force it into the active state. > Aside: > I need to do some further checks; while considering this, I think I've > come across a worrying problem with "PM / Domains: Power on the PM domain > right after attach completes". I think this will result in the driver's > runtime_resume callback being invoked _before_ the probe function. I > can't test this right now as I'm in the middle of upgrading my dev box > and it doesn't have a functional install for building ARM kernels yet. Subsystems always face this problem, because the device core sets dev->driver before calling the probe routine. (There's a similar problem on the other side, where the device core doesn't clear dev->driver until after the remove routine has returned.) To cope with this, the subsystem-level runtime-PM callbacks have to be careful not to invoke the driver-level callbacks before probing is finished or after removal has started. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/8] pm: domains: sync runtime PM status with PM domains 2015-02-18 10:03 ` Russell King - ARM Linux 2015-02-18 15:12 ` Alan Stern @ 2015-02-18 16:46 ` Rafael J. Wysocki 1 sibling, 0 replies; 39+ messages in thread From: Rafael J. Wysocki @ 2015-02-18 16:46 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Len Brown, Greg Kroah-Hartman, linux-pm, Alan Stern On Wednesday, February 18, 2015 10:03:22 AM Russell King - ARM Linux wrote: > On Wed, Feb 18, 2015 at 08:02:30AM +0100, Rafael J. Wysocki wrote: > > On Tuesday, February 17, 2015 07:42:43 PM Russell King - ARM Linux wrote: > > > It's worse than that. If, in the probe, we decide at a point to query > > > the PM domain status, and transfer it into the RPM status, how does the > > > driver know whether it needs to do a "put" to cause a transition from > > > active to suspended? > > > > When ->probe() runs, the cases are: > > (1) There are no power domains, so the device is "active" when the clock is > > enabled and "suspended" when it is disabled. > > (2) There is a power domain which is initially off. The RPM status of the > > device has to be "suspended" or the domain needs to be powered up. > > (quick reply) > > That's not entirely correct. > > As a result of 2ed127697eb13 (PM / Domains: Power on the PM domain right > after attach completes) the power domain will _always_ be powered on prior > to ->probe() being called, if the device was attached to the PM domain > just before ->probe() is called, inspite of the domain being powered off > before the device was attached to the domain. > > If the PM domain is attached earlier, and pm_genpd_poweroff_unused() is > called (before they're attached, but in the kernel boot sequence) the > work for powering down the PM domains will be queued until a point where > it can be scheduled - which will be after devices are attached. That can > allow the PM domain(s) to be powered down (as no driver is attached) which > then results in the driver's ->probe function being called with the PM > domain OFF. > > So, right now, there's no way for a driver to know with certainty whether > the device it is about to probe is powered up or powered down. And it looks that the driver is in a module which is only loaded after we've called the pm_genpd_poweroff_unused(). So clearly commit 2ed127697eb13 doesn't work for that driver. We might just revert that commit, but that wouldn't make the problem go away. Namely, the domain may be powered up or down by *another* driver whose device belongs to it in parallel with your ->probe() anyway. Rafael ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-14 15:26 ` Russell King - ARM Linux ` (3 preceding siblings ...) (?) @ 2015-02-14 15:27 ` Russell King 2015-02-14 17:02 ` Sebastian Hesselbarth 2015-02-14 17:09 ` Andrew Lunn -1 siblings, 2 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: linux-arm-kernel The PMU device contains an interrupt controller, power control and resets. The interrupt controller is a little sub-standard in that there is no race free way to clear down pending interrupts, so we try to avoid problems by reducing the window as much as possible, and clearing as infrequently as possible. The interrupt support is implemented using an IRQ domain, and the parent interrupt referenced in the standard DT way. The power domains and reset support is closely related - there is a defined sequence for powering down a domain which is tightly coupled with asserting the reset. Hence, it makes sense to group these two together. This patch adds the core PMU driver: power domains must be defined in the DT file in order to make use of them. The reset controller can be referenced in the standard way for reset controllers. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/Kconfig | 1 + arch/arm/mach-dove/Makefile | 1 + arch/arm/mach-dove/common.c | 2 + arch/arm/mach-dove/common.h | 1 + arch/arm/mach-dove/include/mach/pm.h | 17 -- arch/arm/mach-dove/irq.c | 87 -------- arch/arm/mach-dove/pmu.c | 410 +++++++++++++++++++++++++++++++++++ 7 files changed, 415 insertions(+), 104 deletions(-) create mode 100644 arch/arm/mach-dove/pmu.c diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 97d07ed60a0b..08e7608d1c52 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -519,6 +519,7 @@ config ARCH_DOVE select PINCTRL select PINCTRL_DOVE select PLAT_ORION_LEGACY + select PM_GENERIC_DOMAINS if PM help Support for the Marvell Dove SoC 88AP510 diff --git a/arch/arm/mach-dove/Makefile b/arch/arm/mach-dove/Makefile index b608a21919fb..b039b91c97b9 100644 --- a/arch/arm/mach-dove/Makefile +++ b/arch/arm/mach-dove/Makefile @@ -1,5 +1,6 @@ obj-y += common.o obj-$(CONFIG_DOVE_LEGACY) += irq.o mpp.o obj-$(CONFIG_PCI) += pcie.o +obj-$(CONFIG_PM_GENERIC_DOMAINS)+= pmu.o obj-$(CONFIG_MACH_DOVE_DB) += dove-db-setup.o obj-$(CONFIG_MACH_CM_A510) += cm-a510.o diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index 0d1a89298ece..195871c87819 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -377,6 +377,8 @@ void __init dove_setup_cpu_wins(void) void __init dove_init(void) { + dove_init_pmu(); + pr_info("Dove 88AP510 SoC, TCLK = %d MHz.\n", (dove_tclk + 499999) / 1000000); diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h index 1d725224d146..3e4a4178efa6 100644 --- a/arch/arm/mach-dove/common.h +++ b/arch/arm/mach-dove/common.h @@ -25,6 +25,7 @@ void dove_map_io(void); void dove_init(void); void dove_init_early(void); void dove_init_irq(void); +int dove_init_pmu(void); void dove_setup_cpu_wins(void); void dove_ge00_init(struct mv643xx_eth_platform_data *eth_data); void dove_sata_init(struct mv_sata_platform_data *sata_data); diff --git a/arch/arm/mach-dove/include/mach/pm.h b/arch/arm/mach-dove/include/mach/pm.h index b47f75038686..625a89c15c1f 100644 --- a/arch/arm/mach-dove/include/mach/pm.h +++ b/arch/arm/mach-dove/include/mach/pm.h @@ -51,22 +51,5 @@ #define CLOCK_GATING_GIGA_PHY_MASK (1 << CLOCK_GATING_BIT_GIGA_PHY) #define PMU_INTERRUPT_CAUSE (DOVE_PMU_VIRT_BASE + 0x50) -#define PMU_INTERRUPT_MASK (DOVE_PMU_VIRT_BASE + 0x54) - -static inline int pmu_to_irq(int pin) -{ - if (pin < NR_PMU_IRQS) - return pin + IRQ_DOVE_PMU_START; - - return -EINVAL; -} - -static inline int irq_to_pmu(int irq) -{ - if (IRQ_DOVE_PMU_START <= irq && irq < NR_IRQS) - return irq - IRQ_DOVE_PMU_START; - - return -EINVAL; -} #endif diff --git a/arch/arm/mach-dove/irq.c b/arch/arm/mach-dove/irq.c index 4a5a7aedcb76..924d8afe4597 100644 --- a/arch/arm/mach-dove/irq.c +++ b/arch/arm/mach-dove/irq.c @@ -7,86 +7,14 @@ * License version 2. This program is licensed "as is" without any * warranty of any kind, whether express or implied. */ - -#include <linux/kernel.h> #include <linux/init.h> #include <linux/irq.h> -#include <linux/gpio.h> #include <linux/io.h> -#include <asm/mach/arch.h> #include <plat/irq.h> -#include <asm/mach/irq.h> -#include <mach/pm.h> #include <mach/bridge-regs.h> #include <plat/orion-gpio.h> #include "common.h" -static void pmu_irq_mask(struct irq_data *d) -{ - int pin = irq_to_pmu(d->irq); - u32 u; - - u = readl(PMU_INTERRUPT_MASK); - u &= ~(1 << (pin & 31)); - writel(u, PMU_INTERRUPT_MASK); -} - -static void pmu_irq_unmask(struct irq_data *d) -{ - int pin = irq_to_pmu(d->irq); - u32 u; - - u = readl(PMU_INTERRUPT_MASK); - u |= 1 << (pin & 31); - writel(u, PMU_INTERRUPT_MASK); -} - -static void pmu_irq_ack(struct irq_data *d) -{ - int pin = irq_to_pmu(d->irq); - u32 u; - - /* - * The PMU mask register is not RW0C: it is RW. This means that - * the bits take whatever value is written to them; if you write - * a '1', you will set the interrupt. - * - * Unfortunately this means there is NO race free way to clear - * these interrupts. - * - * So, let's structure the code so that the window is as small as - * possible. - */ - u = ~(1 << (pin & 31)); - u &= readl_relaxed(PMU_INTERRUPT_CAUSE); - writel_relaxed(u, PMU_INTERRUPT_CAUSE); -} - -static struct irq_chip pmu_irq_chip = { - .name = "pmu_irq", - .irq_mask = pmu_irq_mask, - .irq_unmask = pmu_irq_unmask, - .irq_ack = pmu_irq_ack, -}; - -static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc) -{ - unsigned long cause = readl(PMU_INTERRUPT_CAUSE); - - cause &= readl(PMU_INTERRUPT_MASK); - if (cause == 0) { - do_bad_IRQ(irq, desc); - return; - } - - for (irq = 0; irq < NR_PMU_IRQS; irq++) { - if (!(cause & (1 << irq))) - continue; - irq = pmu_to_irq(irq); - generic_handle_irq(irq); - } -} - static int __initdata gpio0_irqs[4] = { IRQ_DOVE_GPIO_0_7, IRQ_DOVE_GPIO_8_15, @@ -142,8 +70,6 @@ __exception_irq_entry dove_legacy_handle_irq(struct pt_regs *regs) void __init dove_init_irq(void) { - int i; - orion_irq_init(0, IRQ_VIRT_BASE + IRQ_MASK_LOW_OFF); orion_irq_init(32, IRQ_VIRT_BASE + IRQ_MASK_HIGH_OFF); @@ -162,17 +88,4 @@ void __init dove_init_irq(void) orion_gpio_init(NULL, 64, 8, DOVE_GPIO2_VIRT_BASE, 0, IRQ_DOVE_GPIO_START + 64, gpio2_irqs); - - /* - * Mask and clear PMU interrupts - */ - writel(0, PMU_INTERRUPT_MASK); - writel(0, PMU_INTERRUPT_CAUSE); - - for (i = IRQ_DOVE_PMU_START; i < NR_IRQS; i++) { - irq_set_chip_and_handler(i, &pmu_irq_chip, handle_level_irq); - irq_set_status_flags(i, IRQ_LEVEL); - set_irq_flags(i, IRQF_VALID); - } - irq_set_chained_handler(IRQ_DOVE_PMU, pmu_irq_handler); } diff --git a/arch/arm/mach-dove/pmu.c b/arch/arm/mach-dove/pmu.c new file mode 100644 index 000000000000..2d1325995a68 --- /dev/null +++ b/arch/arm/mach-dove/pmu.c @@ -0,0 +1,410 @@ +/* + * Marvell Dove PMU support + */ +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/pm_domain.h> +#include <linux/reset.h> +#include <linux/reset-controller.h> +#include <linux/sched.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +#include <asm/mach/irq.h> + +#include <mach/hardware.h> +#include <mach/pm.h> + +#define PMC_SW_RST 0x30 +#define PMC_IRQ_CAUSE 0x50 +#define PMC_IRQ_MASK 0x54 + +#define PMU_PWR 0x10 +#define PMU_PWR_DOWN_GPU BIT(2) +#define PMU_PWR_DOWN_VPU BIT(3) +#define PMU_ISO 0x58 +#define PMU_ISO_VPU BIT(0) +#define PMU_ISO_GPU BIT(1) +#define PMU_ISO_CPU BIT(2) +#define PMU_ISO_CORE BIT(3) + +struct pmu_data { + spinlock_t lock; + struct device_node *of_node; + void __iomem *pmc_base; + void __iomem *pmu_base; + struct irq_chip_generic *irq_gc; + struct irq_domain *irq_domain; +#ifdef CONFIG_RESET_CONTROLLER + struct reset_controller_dev reset; +#endif +}; + +/* + * The PMU contains a register to reset various subsystems within the + * SoC. Export this as a reset controller. + */ +#ifdef CONFIG_RESET_CONTROLLER +#define rcdev_to_pmu(rcdev) container_of(rcdev, struct pmu_data, reset) + +static int pmu_reset_reset(struct reset_controller_dev *rc, unsigned long id) +{ + struct pmu_data *pmu = rcdev_to_pmu(rc); + unsigned long flags; + u32 val; + + spin_lock_irqsave(&pmu->lock, flags); + val = readl_relaxed(pmu->pmc_base + PMC_SW_RST); + writel_relaxed(val & ~BIT(id), pmu->pmc_base + PMC_SW_RST); + writel_relaxed(val | BIT(id), pmu->pmc_base + PMC_SW_RST); + spin_unlock_irqrestore(&pmu->lock, flags); + + return 0; +} + +static int pmu_reset_assert(struct reset_controller_dev *rc, unsigned long id) +{ + struct pmu_data *pmu = rcdev_to_pmu(rc); + unsigned long flags; + u32 val = ~BIT(id); + + spin_lock_irqsave(&pmu->lock, flags); + val &= readl_relaxed(pmu->pmc_base + PMC_SW_RST); + writel_relaxed(val, pmu->pmc_base + PMC_SW_RST); + spin_unlock_irqrestore(&pmu->lock, flags); + + return 0; +} + +static int pmu_reset_deassert(struct reset_controller_dev *rc, unsigned long id) +{ + struct pmu_data *pmu = rcdev_to_pmu(rc); + unsigned long flags; + u32 val = BIT(id); + + spin_lock_irqsave(&pmu->lock, flags); + val |= readl_relaxed(pmu->pmc_base + PMC_SW_RST); + writel_relaxed(val, pmu->pmc_base + PMC_SW_RST); + spin_unlock_irqrestore(&pmu->lock, flags); + + return 0; +} + +static struct reset_control_ops pmu_reset_ops = { + .reset = pmu_reset_reset, + .assert = pmu_reset_assert, + .deassert = pmu_reset_deassert, +}; + +static struct reset_controller_dev pmu_reset __initdata = { + .ops = &pmu_reset_ops, + .owner = THIS_MODULE, + .nr_resets = 32, +}; + +static void __init pmu_reset_init(struct pmu_data *pmu) +{ + int ret; + + pmu->reset = pmu_reset; + pmu->reset.of_node = pmu->of_node; + + ret = reset_controller_register(&pmu->reset); + if (ret) + pr_err("pmu: %s failed: %d\n", "reset_controller_register", ret); +} +#else +static void __init pmu_reset_init(struct pmu_data *pmu) +{ +} +#endif + +struct pmu_domain { + struct pmu_data *pmu; + u32 pwr_mask; + u32 rst_mask; + u32 iso_mask; + struct generic_pm_domain base; +}; + +#define to_pmu_domain(dom) container_of(dom, struct pmu_domain, base) + +/* + * This deals with the "old" Marvell sequence of bringing a power domain + * down/up, which is: apply power, release reset, disable isolators. + * + * Later devices apparantly use a different sequence: power up, disable + * isolators, assert repair signal, enable SRMA clock, enable AXI clock, + * enable module clock, deassert reset. + * + * Note: reading the assembly, it seems that the IO accessors have an + * unfortunate side-effect - they cause memory already read into registers + * for the if () to be re-read for the bit-set or bit-clear operation. + * The code is written to avoid this. + */ +static int pmu_domain_power_off(struct generic_pm_domain *domain) +{ + struct pmu_domain *pmu_dom = to_pmu_domain(domain); + struct pmu_data *pmu = pmu_dom->pmu; + unsigned long flags; + unsigned int val; + void __iomem *pmu_base = pmu->pmu_base; + void __iomem *pmc_base = pmu->pmc_base; + + spin_lock_irqsave(&pmu->lock, flags); + + /* Enable isolators */ + if (pmu_dom->iso_mask) { + val = ~pmu_dom->iso_mask; + val &= readl_relaxed(pmu_base + PMU_ISO); + writel_relaxed(val, pmu_base + PMU_ISO); + } + + /* Reset unit */ + if (pmu_dom->rst_mask) { + val = ~pmu_dom->rst_mask; + val &= readl_relaxed(pmc_base + PMC_SW_RST); + writel_relaxed(val, pmc_base + PMC_SW_RST); + } + + /* Power down */ + val = readl_relaxed(pmu_base + PMU_PWR) | pmu_dom->pwr_mask; + writel_relaxed(val, pmu_base + PMU_PWR); + + spin_unlock_irqrestore(&pmu->lock, flags); + + return 0; +} + +static int pmu_domain_power_on(struct generic_pm_domain *domain) +{ + struct pmu_domain *pmu_dom = to_pmu_domain(domain); + struct pmu_data *pmu = pmu_dom->pmu; + unsigned long flags; + unsigned int val; + void __iomem *pmu_base = pmu->pmu_base; + void __iomem *pmc_base = pmu->pmc_base; + + spin_lock_irqsave(&pmu->lock, flags); + + /* Power on */ + val = ~pmu_dom->pwr_mask & readl_relaxed(pmu_base + PMU_PWR); + writel_relaxed(val, pmu_base + PMU_PWR); + + /* Release reset */ + if (pmu_dom->rst_mask) { + val = pmu_dom->rst_mask; + val |= readl_relaxed(pmc_base + PMC_SW_RST); + writel_relaxed(val, pmc_base + PMC_SW_RST); + } + + /* Disable isolators */ + if (pmu_dom->iso_mask) { + val = pmu_dom->iso_mask; + val |= readl_relaxed(pmu_base + PMU_ISO); + writel_relaxed(val, pmu_base + PMU_ISO); + } + + spin_unlock_irqrestore(&pmu->lock, flags); + + return 0; +} + +static void __pmu_domain_register(struct pmu_domain *domain, + struct device_node *np) +{ + unsigned int val = readl_relaxed(domain->pmu->pmu_base + PMU_PWR); + + domain->base.power_off = pmu_domain_power_off; + domain->base.power_on = pmu_domain_power_on; + + pm_genpd_init(&domain->base, NULL, !(val & domain->pwr_mask)); + + if (np) + of_genpd_add_provider_simple(np, &domain->base); +} + +/* PMU IRQ controller */ +static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc) +{ + struct pmu_data *pmu = irq_get_handler_data(irq); + struct irq_chip_generic *gc = pmu->irq_gc; + struct irq_domain *domain = pmu->irq_domain; + void __iomem *base = gc->reg_base; + u32 stat = readl_relaxed(base + PMC_IRQ_CAUSE) & gc->mask_cache; + u32 done = ~0; + + if (stat == 0) { + do_bad_IRQ(irq, desc); + return; + } + + while (stat) { + u32 hwirq = fls(stat) - 1; + + stat &= ~(1 << hwirq); + done &= ~(1 << hwirq); + + generic_handle_irq(irq_find_mapping(domain, hwirq)); + } + + /* + * The PMU mask register is not RW0C: it is RW. This means that + * the bits take whatever value is written to them; if you write + * a '1', you will set the interrupt. + * + * Unfortunately this means there is NO race free way to clear + * these interrupts. + * + * So, let's structure the code so that the window is as small as + * possible. + */ + irq_gc_lock(gc); + done &= readl_relaxed(base + PMC_IRQ_CAUSE); + writel_relaxed(done, base + PMC_IRQ_CAUSE); + irq_gc_unlock(gc); +} + +static int __init dove_init_pmu_irq(struct pmu_data *pmu, int irq) +{ + const char *name = "pmu_irq"; + struct irq_chip_generic *gc; + struct irq_domain *domain; + int ret; + + /* mask and clear all interrupts */ + writel(0, pmu->pmc_base + PMC_IRQ_MASK); + writel(0, pmu->pmc_base + PMC_IRQ_CAUSE); + + domain = irq_domain_add_linear(pmu->of_node, NR_PMU_IRQS, + &irq_generic_chip_ops, NULL); + if (!domain) { + pr_err("%s: unable to add irq domain\n", name); + return -ENOMEM; + } + + ret = irq_alloc_domain_generic_chips(domain, NR_PMU_IRQS, 1, name, + handle_level_irq, + IRQ_NOREQUEST | IRQ_NOPROBE, 0, + IRQ_GC_INIT_MASK_CACHE); + if (ret) { + pr_err("%s: unable to alloc irq domain gc: %d\n", name, ret); + irq_domain_remove(domain); + return ret; + } + + gc = irq_get_domain_generic_chip(domain, 0); + gc->reg_base = pmu->pmc_base; + gc->chip_types[0].regs.mask = PMC_IRQ_MASK; + gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; + gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit; + + pmu->irq_domain = domain; + pmu->irq_gc = gc; + + /* If no of_node, populate the domain */ + if (!pmu->of_node) + irq_domain_associate_many(pmu->irq_domain, IRQ_DOVE_PMU_START, + 0, NR_PMU_IRQS); + + irq_set_handler_data(irq, pmu); + irq_set_chained_handler(irq, pmu_irq_handler); + + return 0; +} + +/* + * pmu { + * compatible = "marvell,pmu"; + * reg = <0xd0000 0x8000> <0xd8000 0x8000>; + * interrupts = <33>; + * #reset-cells = 1; + * #power-domain-cells = <0>; + * vpu_domain: vpu-domain { + * marvell,pmu_pwr_mask = <0x00000008>; + * marvell,pmu_iso_mask = <0x00000001>; + * resets = <&pmu 16>; + * }; + * gpu_domain: gpu-domain { + * marvell,pmu_pwr_mask = <0x00000004>; + * marvell,pmu_iso_mask = <0x00000002>; + * resets = <&pmu 18>; + * }; + * }; + */ +int __init dove_init_pmu(void) +{ + struct device_node *np_pmu, *np; + struct pmu_data *pmu; + int ret, parent_irq; + + /* Lookup the PMU node */ + np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu"); + if (!np_pmu) + return 0; + + pmu = kzalloc(sizeof(*pmu), GFP_KERNEL); + if (!pmu) + return -ENOMEM; + + spin_lock_init(&pmu->lock); + pmu->of_node = np_pmu; + pmu->pmc_base = of_iomap(pmu->of_node, 0); + pmu->pmu_base = of_iomap(pmu->of_node, 1); + if (!pmu->pmc_base || !pmu->pmu_base) { + pr_err("%s: failed to map PMU\n", np_pmu->name); + iounmap(pmu->pmu_base); + iounmap(pmu->pmc_base); + kfree(pmu); + return -ENOMEM; + } + + parent_irq = irq_of_parse_and_map(pmu->of_node, 0); + if (!parent_irq) + pr_err("%s: no interrupt specified\n", np_pmu->name); + + pmu_reset_init(pmu); + + for_each_available_child_of_node(pmu->of_node, np) { + struct of_phandle_args args; + struct pmu_domain *domain; + + domain = kzalloc(sizeof(*domain), GFP_KERNEL); + if (!domain) + break; + + domain->pmu = pmu; + domain->base.name = kstrdup(np->name, GFP_KERNEL); + if (!domain->base.name) { + kfree(domain); + break; + } + + of_property_read_u32(np, "marvell,pmu_pwr_mask", + &domain->pwr_mask); + of_property_read_u32(np, "marvell,pmu_iso_mask", + &domain->iso_mask); + + ret = of_parse_phandle_with_args(np, "resets", "#reset-cells", + 0, &args); + if (ret == 0) { + if (args.np == pmu->of_node) + domain->rst_mask = BIT(args.args[0]); + of_node_put(args.np); + } + + __pmu_domain_register(domain, np); + } + pm_genpd_poweroff_unused(); + + ret = dove_init_pmu_irq(pmu, parent_irq); + if (ret) + pr_err("dove_init_pmu_irq() failed: %d\n", ret); + + return 0; +} -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-14 15:27 ` [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets Russell King @ 2015-02-14 17:02 ` Sebastian Hesselbarth 2015-02-15 16:36 ` Russell King - ARM Linux 2015-02-16 15:58 ` Russell King - ARM Linux 2015-02-14 17:09 ` Andrew Lunn 1 sibling, 2 replies; 39+ messages in thread From: Sebastian Hesselbarth @ 2015-02-14 17:02 UTC (permalink / raw) To: linux-arm-kernel On 14.02.2015 16:27, Russell King wrote: > The PMU device contains an interrupt controller, power control and > resets. The interrupt controller is a little sub-standard in that > there is no race free way to clear down pending interrupts, so we try > to avoid problems by reducing the window as much as possible, and > clearing as infrequently as possible. > > The interrupt support is implemented using an IRQ domain, and the > parent interrupt referenced in the standard DT way. > > The power domains and reset support is closely related - there is a > defined sequence for powering down a domain which is tightly coupled > with asserting the reset. Hence, it makes sense to group these two > together. > > This patch adds the core PMU driver: power domains must be defined in > the DT file in order to make use of them. The reset controller can > be referenced in the standard way for reset controllers. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- [...] > diff --git a/arch/arm/mach-dove/pmu.c b/arch/arm/mach-dove/pmu.c > new file mode 100644 > index 000000000000..2d1325995a68 > --- /dev/null > +++ b/arch/arm/mach-dove/pmu.c > @@ -0,0 +1,410 @@ > +/* > + * Marvell Dove PMU support > + */ [...] > +/* > + * pmu { > + * compatible = "marvell,pmu"; Russell, Should we be more precise and call it "marvell,dove-pmu" ? Also, can we have an additional "syscon" compatible for that node? That will allow us to get rid of the messy iomem stuff current pinctrl-dove is doing to get access to the pmu pinctrl registers. > + * reg = <0xd0000 0x8000> <0xd8000 0x8000>; > + * interrupts = <33>; > + * #reset-cells = 1; > + * #power-domain-cells = <0>; > + * vpu_domain: vpu-domain { > + * marvell,pmu_pwr_mask = <0x00000008>; > + * marvell,pmu_iso_mask = <0x00000001>; > + * resets = <&pmu 16>; > + * }; > + * gpu_domain: gpu-domain { > + * marvell,pmu_pwr_mask = <0x00000004>; > + * marvell,pmu_iso_mask = <0x00000002>; > + * resets = <&pmu 18>; > + * }; > + * }; > + */ > +int __init dove_init_pmu(void) > +{ How about we copy the clk subsystem way of installing early probed pm for DT here? For example: #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn) and static int __init dove_pmu_init(struct device_node *np) { ... } PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu); Sebastian > + struct device_node *np_pmu, *np; > + struct pmu_data *pmu; > + int ret, parent_irq; > + > + /* Lookup the PMU node */ > + np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu"); > + if (!np_pmu) > + return 0; > + > + pmu = kzalloc(sizeof(*pmu), GFP_KERNEL); > + if (!pmu) > + return -ENOMEM; > + > + spin_lock_init(&pmu->lock); > + pmu->of_node = np_pmu; > + pmu->pmc_base = of_iomap(pmu->of_node, 0); > + pmu->pmu_base = of_iomap(pmu->of_node, 1); > + if (!pmu->pmc_base || !pmu->pmu_base) { > + pr_err("%s: failed to map PMU\n", np_pmu->name); > + iounmap(pmu->pmu_base); > + iounmap(pmu->pmc_base); > + kfree(pmu); > + return -ENOMEM; > + } > + > + parent_irq = irq_of_parse_and_map(pmu->of_node, 0); > + if (!parent_irq) > + pr_err("%s: no interrupt specified\n", np_pmu->name); > + > + pmu_reset_init(pmu); > + > + for_each_available_child_of_node(pmu->of_node, np) { > + struct of_phandle_args args; > + struct pmu_domain *domain; > + > + domain = kzalloc(sizeof(*domain), GFP_KERNEL); > + if (!domain) > + break; > + > + domain->pmu = pmu; > + domain->base.name = kstrdup(np->name, GFP_KERNEL); > + if (!domain->base.name) { > + kfree(domain); > + break; > + } > + > + of_property_read_u32(np, "marvell,pmu_pwr_mask", > + &domain->pwr_mask); > + of_property_read_u32(np, "marvell,pmu_iso_mask", > + &domain->iso_mask); > + > + ret = of_parse_phandle_with_args(np, "resets", "#reset-cells", > + 0, &args); > + if (ret == 0) { > + if (args.np == pmu->of_node) > + domain->rst_mask = BIT(args.args[0]); > + of_node_put(args.np); > + } > + > + __pmu_domain_register(domain, np); > + } > + pm_genpd_poweroff_unused(); > + > + ret = dove_init_pmu_irq(pmu, parent_irq); > + if (ret) > + pr_err("dove_init_pmu_irq() failed: %d\n", ret); > + > + return 0; > +} > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-14 17:02 ` Sebastian Hesselbarth @ 2015-02-15 16:36 ` Russell King - ARM Linux 2015-02-16 15:58 ` Russell King - ARM Linux 1 sibling, 0 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-15 16:36 UTC (permalink / raw) To: linux-arm-kernel On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote: > Should we be more precise and call it "marvell,dove-pmu" ? Andrew made the same comment, consider it done. > Also, can we have an additional "syscon" compatible for that node? I'll look into it, thanks. > How about we copy the clk subsystem way of installing early probed > pm for DT here? > > For example: > > #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn) > > and > > static int __init dove_pmu_init(struct device_node *np) { ... } > PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu); Not sure it's worth the overhead. Firstly, sticking the PMU into the clock OF tables doesn't seem like the right thing to do. Secondly, it means modifying the asm-generic vmlinux.lds.S, adding another config symbol for PMUs to enable that table, adding a sentinel entry, and finally adding some code somewhere to scan this new table which right now is only going to have at most one entry. That seems like an awful lot of overhead. It would have been easier if there was a generic mechanism, like the initcall mechanism, where it's possible to add these kinds of compatible matches automatically without having to mess around with so many different files. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-14 17:02 ` Sebastian Hesselbarth 2015-02-15 16:36 ` Russell King - ARM Linux @ 2015-02-16 15:58 ` Russell King - ARM Linux 2015-02-16 16:30 ` Sebastian Hesselbarth 1 sibling, 1 reply; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-16 15:58 UTC (permalink / raw) To: linux-arm-kernel On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote: > How about we copy the clk subsystem way of installing early probed > pm for DT here? > > For example: > > #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn) > > and > > static int __init dove_pmu_init(struct device_node *np) { ... } > PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu); Well, Rob's response was basically "use the machine descriptor" so I guess it needs to be explicitly called from arch/arm/mach-mvebu/dove.c:dove_init(). If you disagree, please discuss with Rob (on To:) and try and find a way forward and let me know how to deal with this. Thanks. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-16 15:58 ` Russell King - ARM Linux @ 2015-02-16 16:30 ` Sebastian Hesselbarth 2015-02-16 16:52 ` Russell King - ARM Linux 0 siblings, 1 reply; 39+ messages in thread From: Sebastian Hesselbarth @ 2015-02-16 16:30 UTC (permalink / raw) To: linux-arm-kernel On 16.02.2015 16:58, Russell King - ARM Linux wrote: > On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote: >> How about we copy the clk subsystem way of installing early probed >> pm for DT here? >> >> For example: >> >> #define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn) >> >> and >> >> static int __init dove_pmu_init(struct device_node *np) { ... } >> PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu); > > Well, Rob's response was basically "use the machine descriptor" so I > guess it needs to be explicitly called from > arch/arm/mach-mvebu/dove.c:dove_init(). Ok, I am very fine with that, too. Still, we'd have to find a proper place for the driver, don't we? Sebastian ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-16 16:30 ` Sebastian Hesselbarth @ 2015-02-16 16:52 ` Russell King - ARM Linux 2015-02-16 17:54 ` Andrew Lunn 0 siblings, 1 reply; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-16 16:52 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 16, 2015 at 05:30:24PM +0100, Sebastian Hesselbarth wrote: > On 16.02.2015 16:58, Russell King - ARM Linux wrote: > >On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote: > >>How about we copy the clk subsystem way of installing early probed > >>pm for DT here? > >> > >>For example: > >> > >>#define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn) > >> > >>and > >> > >>static int __init dove_pmu_init(struct device_node *np) { ... } > >>PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu); > > > >Well, Rob's response was basically "use the machine descriptor" so I > >guess it needs to be explicitly called from > >arch/arm/mach-mvebu/dove.c:dove_init(). > > Ok, I am very fine with that, too. > > Still, we'd have to find a proper place for the driver, don't we? Yep - I'm not sure creating drivers/pmu for (at the moment) one driver is a particularly good idea. Maybe something in drivers/soc/ ? -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-16 16:52 ` Russell King - ARM Linux @ 2015-02-16 17:54 ` Andrew Lunn 0 siblings, 0 replies; 39+ messages in thread From: Andrew Lunn @ 2015-02-16 17:54 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 16, 2015 at 04:52:54PM +0000, Russell King - ARM Linux wrote: > On Mon, Feb 16, 2015 at 05:30:24PM +0100, Sebastian Hesselbarth wrote: > > On 16.02.2015 16:58, Russell King - ARM Linux wrote: > > >On Sat, Feb 14, 2015 at 06:02:11PM +0100, Sebastian Hesselbarth wrote: > > >>How about we copy the clk subsystem way of installing early probed > > >>pm for DT here? > > >> > > >>For example: > > >> > > >>#define PM_OF_DECLARE(name, compat, fn) OF_DECLARE_1(clk, name, compat, fn) > > >> > > >>and > > >> > > >>static int __init dove_pmu_init(struct device_node *np) { ... } > > >>PM_OF_DECLARE(dove_pmu, "marvell,dove-pmu", dove_init_pmu); > > > > > >Well, Rob's response was basically "use the machine descriptor" so I > > >guess it needs to be explicitly called from > > >arch/arm/mach-mvebu/dove.c:dove_init(). > > > > Ok, I am very fine with that, too. > > > > Still, we'd have to find a proper place for the driver, don't we? > > Yep - I'm not sure creating drivers/pmu for (at the moment) one driver > is a particularly good idea. Maybe something in drivers/soc/ ? If you did create drivers/pmu, you could move drives/soc/pmc.c into it. But drivers/soc also seems like a good place for the dove PMU code. Andrew ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-14 15:27 ` [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets Russell King 2015-02-14 17:02 ` Sebastian Hesselbarth @ 2015-02-14 17:09 ` Andrew Lunn 2015-02-15 16:26 ` Russell King - ARM Linux 1 sibling, 1 reply; 39+ messages in thread From: Andrew Lunn @ 2015-02-14 17:09 UTC (permalink / raw) To: linux-arm-kernel Hi Russell > +static struct reset_control_ops pmu_reset_ops = { > + .reset = pmu_reset_reset, > + .assert = pmu_reset_assert, > + .deassert = pmu_reset_deassert, > +}; > + > +static struct reset_controller_dev pmu_reset __initdata = { > + .ops = &pmu_reset_ops, > + .owner = THIS_MODULE, Dumb question: Is this still needed? There have been a lot of patches from Wolfram Sang removing similar THIS_MODULE statements. > + * pmu { > + * compatible = "marvell,pmu"; Is this maybe too generic? I'm sure Marvell has more than one pmu. Maybe "marvell,dove-pmu" > +int __init dove_init_pmu(void) > +{ > + struct device_node *np_pmu, *np; > + struct pmu_data *pmu; > + int ret, parent_irq; > + > + /* Lookup the PMU node */ > + np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu"); > + if (!np_pmu) > + return 0; > + > + pmu = kzalloc(sizeof(*pmu), GFP_KERNEL); > + if (!pmu) > + return -ENOMEM; > + > + spin_lock_init(&pmu->lock); > + pmu->of_node = np_pmu; > + pmu->pmc_base = of_iomap(pmu->of_node, 0); > + pmu->pmu_base = of_iomap(pmu->of_node, 1); > + if (!pmu->pmc_base || !pmu->pmu_base) { > + pr_err("%s: failed to map PMU\n", np_pmu->name); > + iounmap(pmu->pmu_base); > + iounmap(pmu->pmc_base); > + kfree(pmu); > + return -ENOMEM; > + } > + > + parent_irq = irq_of_parse_and_map(pmu->of_node, 0); > + if (!parent_irq) > + pr_err("%s: no interrupt specified\n", np_pmu->name); > + Is it not fatal to be missing the interrupt? Seems like return -EINVAL would be a good idea? Andrew ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets 2015-02-14 17:09 ` Andrew Lunn @ 2015-02-15 16:26 ` Russell King - ARM Linux 0 siblings, 0 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2015-02-15 16:26 UTC (permalink / raw) To: linux-arm-kernel On Sat, Feb 14, 2015 at 06:09:39PM +0100, Andrew Lunn wrote: > Hi Russell > > > +static struct reset_control_ops pmu_reset_ops = { > > + .reset = pmu_reset_reset, > > + .assert = pmu_reset_assert, > > + .deassert = pmu_reset_deassert, > > +}; > > + > > +static struct reset_controller_dev pmu_reset __initdata = { > > + .ops = &pmu_reset_ops, > > + .owner = THIS_MODULE, > > Dumb question: Is this still needed? There have been a lot of patches > from Wolfram Sang removing similar THIS_MODULE statements. If this is not set, then .owner remains NULL; reset_controller_register() is not wrapped to hide the initialisation of this member. (IMHO, that's exactly why hiding it is bad - it has created inconsistencies because some APIs need an explicit initialisation, others don't.) > > + * pmu { > > + * compatible = "marvell,pmu"; > > Is this maybe too generic? I'm sure Marvell has more than one pmu. > Maybe "marvell,dove-pmu" Changed. > > +int __init dove_init_pmu(void) > > +{ > > + struct device_node *np_pmu, *np; > > + struct pmu_data *pmu; > > + int ret, parent_irq; > > + > > + /* Lookup the PMU node */ > > + np_pmu = of_find_compatible_node(NULL, NULL, "marvell,pmu"); > > + if (!np_pmu) > > + return 0; > > + > > + pmu = kzalloc(sizeof(*pmu), GFP_KERNEL); > > + if (!pmu) > > + return -ENOMEM; > > + > > + spin_lock_init(&pmu->lock); > > + pmu->of_node = np_pmu; > > + pmu->pmc_base = of_iomap(pmu->of_node, 0); > > + pmu->pmu_base = of_iomap(pmu->of_node, 1); > > + if (!pmu->pmc_base || !pmu->pmu_base) { > > + pr_err("%s: failed to map PMU\n", np_pmu->name); > > + iounmap(pmu->pmu_base); > > + iounmap(pmu->pmc_base); > > + kfree(pmu); > > + return -ENOMEM; > > + } > > + > > + parent_irq = irq_of_parse_and_map(pmu->of_node, 0); > > + if (!parent_irq) > > + pr_err("%s: no interrupt specified\n", np_pmu->name); > > + > > Is it not fatal to be missing the interrupt? Seems like return -EINVAL > would be a good idea? I don't think so. The lack of parent interrupt shouldn't stop the rest of the PMU code initialising - least of all only because the only user of this right now is the RTC. It may make sense to avoid initialising the PMU's IRQ support in that case though. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20150214152659.GI8656-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* [PATCH 5/8] ARM: dt: dove: add Dove PMU DT entry to dove.dtsi 2015-02-14 15:26 ` Russell King - ARM Linux @ 2015-02-14 15:27 ` Russell King -1 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Add the PMU description to the Dove DT file. The PMU provides multiple features, including an interrupt, reset, power and isolation controller. Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> --- arch/arm/boot/dts/dove.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index a5441d5482a6..1e664eb8bdcb 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -380,6 +380,15 @@ status = "disabled"; }; + pmu: power-management@d0000 { + compatible = "marvell,pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + interrupt-controller; + #interrupt-cells = <1>; + #reset-cells = <1>; + }; + thermal: thermal-diode@d001c { compatible = "marvell,dove-thermal"; reg = <0xd001c 0x0c>, <0xd005c 0x08>; -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 5/8] ARM: dt: dove: add Dove PMU DT entry to dove.dtsi @ 2015-02-14 15:27 ` Russell King 0 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: linux-arm-kernel Add the PMU description to the Dove DT file. The PMU provides multiple features, including an interrupt, reset, power and isolation controller. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/boot/dts/dove.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index a5441d5482a6..1e664eb8bdcb 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -380,6 +380,15 @@ status = "disabled"; }; + pmu: power-management at d0000 { + compatible = "marvell,pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + interrupt-controller; + #interrupt-cells = <1>; + #reset-cells = <1>; + }; + thermal: thermal-diode at d001c { compatible = "marvell,dove-thermal"; reg = <0xd001c 0x0c>, <0xd005c 0x08>; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 6/8] ARM: dt: dove: wire up RTC interrupt 2015-02-14 15:26 ` Russell King - ARM Linux @ 2015-02-14 15:27 ` Russell King -1 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Now that we have a PMU driver, we can wire up the RTC interrupt in the DT description for Dove. Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> --- arch/arm/boot/dts/dove.dtsi | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index 1e664eb8bdcb..00b61d2be66b 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -624,6 +624,8 @@ rtc: real-time-clock@d8500 { compatible = "marvell,orion-rtc"; reg = <0xd8500 0x20>; + interrupt-parent = <&pmu>; + interrupts = <5>; }; gconf: global-config@e802c { -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 6/8] ARM: dt: dove: wire up RTC interrupt @ 2015-02-14 15:27 ` Russell King 0 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:27 UTC (permalink / raw) To: linux-arm-kernel Now that we have a PMU driver, we can wire up the RTC interrupt in the DT description for Dove. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/boot/dts/dove.dtsi | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index 1e664eb8bdcb..00b61d2be66b 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -624,6 +624,8 @@ rtc: real-time-clock at d8500 { compatible = "marvell,orion-rtc"; reg = <0xd8500 0x20>; + interrupt-parent = <&pmu>; + interrupts = <5>; }; gconf: global-config at e802c { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 7/8] ARM: dt: dove: add video decoder power domain description 2015-02-14 15:26 ` Russell King - ARM Linux @ 2015-02-14 15:28 ` Russell King -1 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:28 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Add the description of the video decoder power domain to the PMU DT entry. Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> --- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index 00b61d2be66b..a47c7cef380f 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -387,6 +387,13 @@ interrupt-controller; #interrupt-cells = <1>; #reset-cells = <1>; + + vpu_domain: vpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000008>; + marvell,pmu_iso_mask = <0x00000001>; + resets = <&pmu 16>; + }; }; thermal: thermal-diode@d001c { -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 7/8] ARM: dt: dove: add video decoder power domain description @ 2015-02-14 15:28 ` Russell King 0 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:28 UTC (permalink / raw) To: linux-arm-kernel Add the description of the video decoder power domain to the PMU DT entry. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index 00b61d2be66b..a47c7cef380f 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -387,6 +387,13 @@ interrupt-controller; #interrupt-cells = <1>; #reset-cells = <1>; + + vpu_domain: vpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000008>; + marvell,pmu_iso_mask = <0x00000001>; + resets = <&pmu 16>; + }; }; thermal: thermal-diode at d001c { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 8/8] ARM: dt: dove: add GPU power domain description 2015-02-14 15:26 ` Russell King - ARM Linux @ 2015-02-14 15:28 ` Russell King -1 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:28 UTC (permalink / raw) To: Andrew Lunn, Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Add the description of the GPU power domain to the PMU DT entry. Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> --- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index a47c7cef380f..081f968c521a 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -394,6 +394,13 @@ marvell,pmu_iso_mask = <0x00000001>; resets = <&pmu 16>; }; + + gpu_domain: gpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000004>; + marvell,pmu_iso_mask = <0x00000002>; + resets = <&pmu 18>; + }; }; thermal: thermal-diode@d001c { -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 8/8] ARM: dt: dove: add GPU power domain description @ 2015-02-14 15:28 ` Russell King 0 siblings, 0 replies; 39+ messages in thread From: Russell King @ 2015-02-14 15:28 UTC (permalink / raw) To: linux-arm-kernel Add the description of the GPU power domain to the PMU DT entry. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index a47c7cef380f..081f968c521a 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -394,6 +394,13 @@ marvell,pmu_iso_mask = <0x00000001>; resets = <&pmu 16>; }; + + gpu_domain: gpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000004>; + marvell,pmu_iso_mask = <0x00000002>; + resets = <&pmu 18>; + }; }; thermal: thermal-diode at d001c { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [FOR DISCUSSION 0/8] Dove PMU support 2015-02-14 15:26 ` Russell King - ARM Linux @ 2015-02-14 16:49 ` Andrew Lunn -1 siblings, 0 replies; 39+ messages in thread From: Andrew Lunn @ 2015-02-14 16:49 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Jason Cooper, Rafael J. Wysocki, Sebastian Hesselbarth, devicetree, Greg Kroah-Hartman, Ian Campbell, Kumar Gala, Len Brown, linux-arm-kernel, linux-pm, Mark Rutland, Pawel Moll, Rob Herring Hi Russell Thanks for posting these. Lets try to get them merged. > There are a few things which are missing from this driver, such sa the > DT binding documentation. This will follow once people are happy with > the first four patches, in particular where to locate the PMU driver > in the kernel tree. Currently, I have it in arch/arm/mach-dove, which > is where the legacy Dove code lives, so it's not ideal. Maybe time for a drivers/pmu? arch/arm/mach-dove is not good, we want to be able to use this code from arch/arm/mach-mvebu. Andrew ^ permalink raw reply [flat|nested] 39+ messages in thread
* [FOR DISCUSSION 0/8] Dove PMU support @ 2015-02-14 16:49 ` Andrew Lunn 0 siblings, 0 replies; 39+ messages in thread From: Andrew Lunn @ 2015-02-14 16:49 UTC (permalink / raw) To: linux-arm-kernel Hi Russell Thanks for posting these. Lets try to get them merged. > There are a few things which are missing from this driver, such sa the > DT binding documentation. This will follow once people are happy with > the first four patches, in particular where to locate the PMU driver > in the kernel tree. Currently, I have it in arch/arm/mach-dove, which > is where the legacy Dove code lives, so it's not ideal. Maybe time for a drivers/pmu? arch/arm/mach-dove is not good, we want to be able to use this code from arch/arm/mach-mvebu. Andrew ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2015-03-04 20:11 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-14 15:26 [FOR DISCUSSION 0/8] Dove PMU support Russell King - ARM Linux 2015-02-14 15:26 ` Russell King - ARM Linux 2015-02-14 15:27 ` [PATCH 1/8] pm: domains: quieten down generic pm domains Russell King 2015-02-14 15:27 ` [PATCH 2/8] pm: domains: avoid potential oops in pm_genpd_remove_device() Russell King 2015-02-14 15:27 ` [PATCH 3/8] pm: domains: sync runtime PM status with PM domains Russell King 2015-02-15 4:24 ` Ulf Hansson 2015-02-17 18:53 ` Rafael J. Wysocki 2015-02-17 19:42 ` Alan Stern 2015-02-17 19:42 ` Russell King - ARM Linux 2015-02-18 7:02 ` Rafael J. Wysocki 2015-02-18 10:03 ` Russell King - ARM Linux 2015-02-18 15:12 ` Alan Stern 2015-02-18 15:42 ` Russell King - ARM Linux 2015-02-18 16:52 ` Rafael J. Wysocki 2015-02-18 17:26 ` Russell King - ARM Linux 2015-02-18 18:05 ` Rafael J. Wysocki 2015-03-04 19:57 ` Ulf Hansson 2015-03-04 20:11 ` Russell King - ARM Linux 2015-02-18 18:42 ` Alan Stern 2015-02-18 16:46 ` Rafael J. Wysocki 2015-02-14 15:27 ` [PATCH 4/8] ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets Russell King 2015-02-14 17:02 ` Sebastian Hesselbarth 2015-02-15 16:36 ` Russell King - ARM Linux 2015-02-16 15:58 ` Russell King - ARM Linux 2015-02-16 16:30 ` Sebastian Hesselbarth 2015-02-16 16:52 ` Russell King - ARM Linux 2015-02-16 17:54 ` Andrew Lunn 2015-02-14 17:09 ` Andrew Lunn 2015-02-15 16:26 ` Russell King - ARM Linux [not found] ` <20150214152659.GI8656-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2015-02-14 15:27 ` [PATCH 5/8] ARM: dt: dove: add Dove PMU DT entry to dove.dtsi Russell King 2015-02-14 15:27 ` Russell King 2015-02-14 15:27 ` [PATCH 6/8] ARM: dt: dove: wire up RTC interrupt Russell King 2015-02-14 15:27 ` Russell King 2015-02-14 15:28 ` [PATCH 7/8] ARM: dt: dove: add video decoder power domain description Russell King 2015-02-14 15:28 ` Russell King 2015-02-14 15:28 ` [PATCH 8/8] ARM: dt: dove: add GPU " Russell King 2015-02-14 15:28 ` Russell King 2015-02-14 16:49 ` [FOR DISCUSSION 0/8] Dove PMU support Andrew Lunn 2015-02-14 16:49 ` Andrew Lunn
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.