Dove PMU support
Subsystems (14)
cover Dove PMU support
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 has been re-worked to try an alternative approach, syncing the PM domain state with the runtime PM state after the probe has completed.
* 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.
I've also included the DT binding documentation as a separate patch this time around, and moved the PMU driver to drivers/soc/dove.
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 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. Failing that, if at least the patches which Rafael has already acked could be applied, that would at least get /some/ form of forward progress.
Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 +++ arch/arm/boot/dts/dove.dtsi | 25 ++ arch/arm/mach-mvebu/Kconfig | 1 + drivers/base/platform.c | 2 + drivers/base/power/common.c | 15 + drivers/base/power/domain.c | 33 +- drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 399 +++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 + include/linux/soc/dove/pmu.h | 6 + 12 files changed, 532 insertions(+), 5 deletions(-)
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 1/9 pm: domains: quieten down generic pm domains
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.
--- drivers/base/power/domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/base/power/domain.c +4 -4
@@ -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; }
Signature · 1 line
1.8.3.1
Re: [PATCH 1/9] pm: domains: quieten down generic pm domains
Quoted 7 lines · Russell King
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.
Signed-off-by Russell King <[email protected]>
Quoted 37 lines
--- drivers/base/power/domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/base/power/domain.c +4
-5@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] pm: domains: quieten down generic pm domains
In reply to Russell KingQuoted 7 lines · Russell King
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.
Signed-off-by Russell King <[email protected]>
We also take several timings even for callbacks that don't exist, and I've got a forthcoming patch to clean that up as well.
Kevin
A message that is not in the archive (sent off-list or not yet received): 20150312183020.GU8656-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org
PATCH 4/9 dt-bindings: add Marvell PMU documentation
Add the required DT binding documentation for the Marvell PMU driver.
--- Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/dove/pmu.txt
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +49 -0
@@ -0,0 +1,49 @@ +Device Tree bindings for Marvell PMU + +Required properties: + - compatible: value should be "marvell,dove-pmu". + - reg: two base addresses and sizes of the PM controller and PMU. + - interrupts: single interrupt number for the PMU interrupt + - interrupt-controller: must be specified as the PMU itself is an + interrupt controller. + - #interrupt-cells: must be 1. + - #reset-cells: must be 1. + +Optional properties: + - None + +Domain descriptions are listed as child nodes of the power management +node. Each domain has the following properties: + +Required properties: + - #power-domain-cells: must be 0. + +Optional properties: + - marvell,pmu_pwr_mask: specifies the mask value for PMU power register + - marvell,pmu_iso_mask: specifies the mask value for PMU isolation register + - resets: points to the reset manager (PMU node) and reset index. + +Example: + + pmu: power-management@d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + 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>; + }; + + gpu_domain: gpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000004>; + marvell,pmu_iso_mask = <0x00000002>; + resets = <&pmu 18>; + }; + };
Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): E1YW7t0-0003mE-My-eh5Bv4kxaXIANfyc6IWni62ZND6+EDdj@public.gmane.org
Re: [PATCH 4/9] dt-bindings: add Marvell PMU documentation
Quoted 14 lines · Russell King
Add the required DT binding documentation for the Marvell PMU driver.
Signed-off-by Russell King <rmk+kernel-lFZ/[email protected]>--- Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/dove/pmu.txtDocumentation/devicetree/bindings/soc/dove/pmu.txt new file +0
-0
Documentation/devicetree/bindings/soc/dove/pmu.txt line 8
Quoted 8 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +8
-0@@ -0,0 +1,49 @@ +Device Tree bindings for Marvell PMU + +Required properties: + - compatible: value should be "marvell,dove-pmu". + - reg: two base addresses and sizes of the PM controller and PMU. + - interrupts: single interrupt number for the PMU interrupt + - interrupt-controller: must be specified as the PMU itself is an + interrupt controller.
Is it always an interrupt controller or just in low power modes like ones typically tacked on to GICs?
Documentation/devicetree/bindings/soc/dove/pmu.txt line 15
Quoted 7 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +7
-0+ - #interrupt-cells: must be 1. + - #reset-cells: must be 1. + +Optional properties: + - None + +Domain descriptions are listed as child nodes of the power management
"Power domain descriptions..." just to be clear.
Quoted 35 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +34
-1+node. Each domain has the following properties: + +Required properties: + - #power-domain-cells: must be 0. + +Optional properties: + - marvell,pmu_pwr_mask: specifies the mask value for PMU power register + - marvell,pmu_iso_mask: specifies the mask value for PMU isolation register + - resets: points to the reset manager (PMU node) and reset index. + +Example: + + pmu: power-management@d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + 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>; + }; + + gpu_domain: gpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000004>; + marvell,pmu_iso_mask = <0x00000002>; + resets = <&pmu 18>; + }; + }; --
Quoted 2 lines
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): CAL_Jsq+R70GVsuig7Ebo49_MeneD_RZ=E0haTVsb_xFhTR7-pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Re: [PATCH 4/9] dt-bindings: add Marvell PMU documentation
Quoted 19 lines · Rob Herring
Quoted 17 lines · Russell King
Add the required DT binding documentation for the Marvell PMU driver.
Signed-off-by Russell King <rmk+kernel-lFZ/[email protected]>--- Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/dove/pmu.txtDocumentation/devicetree/bindings/soc/dove/pmu.txt new file +3
-0@@ -0,0 +1,49 @@ +Device Tree bindings for Marvell PMU + +Required properties:
Quoted 8 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt line 8
Quoted 5 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +5
-0+ - compatible: value should be "marvell,dove-pmu". + - reg: two base addresses and sizes of the PM controller and PMU. + - interrupts: single interrupt number for the PMU interrupt + - interrupt-controller: must be specified as the PMU itself is an + interrupt controller.Is it always an interrupt controller or just in low power modes like ones typically tacked on to GICs?
Always. It's an interrupt controller: it takes interrupts from other parts of the system with individual status and masks, and forwards it on as a single interrupt input to the upstream interrupt controller.
Quoted 9 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt line 15
Quoted 7 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +7
-0+ - #interrupt-cells: must be 1. + - #reset-cells: must be 1. + +Optional properties: + - None + +Domain descriptions are listed as child nodes of the power management"Power domain descriptions..." just to be clear.
Ok.
Signature · 6 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] dt-bindings: add Marvell PMU documentation
In reply to a message not in the archiveQuoted 4 lines · On Thursday 12 March 2015 18:31:10 Russell King wrote:
Add the required DT binding documentation for the Marvell PMU driver.
Signed-off-by Russell King <rmk+kernel-lFZ/[email protected]>
The binding and DT nodes look ok to me, I'm definitely fine with taking those through the arm-soc tree, regardless of how we merge the other changes. The actual driver should probably go the same way, unless you want to put them in a branch with the drivers/base changes and have them merged through Rafael's tree.
Who is picking up patches for mach-dove at the moment? I know that Gregory Clement has taken over the mvebu git tree from Jason for the time being, but he's not listed as a maintainer for dove/mv78xx0/orion and wasn't on Cc to these mails as a consequence (I've added him for the discussion here).
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/9 pm: domains: avoid potential oops in pm_genpd_remove_device()
In reply to Russell King - ARM Linuxpm_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.)
--- drivers/base/power/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/base/power/domain.c +1 -1
@@ -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 ;
Signature · 1 line
1.8.3.1
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 4 lines · Russell King
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 indev->pm_domain(eg,vga_switcheroostuff.)
Could the "vga_switcheroo" code deal with this instead?
Quoted 12 lines
Signed-off-by Russell King <[email protected]>--- drivers/base/power/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/base/power/domain.c +0
-0
drivers/base/power/domain.c int pm_genpd_remove_device(struct generic_pm_domain *genpd, · line 1512
Quoted 5 lines
drivers/base/power/domain.c +1
-1@@ -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)
The are two issues with this approach.
1. pm_genpd_present() is build only for CONFIG_PM_SLEEP set.
2. pm_genpd_present() totally ignores holding the mutex which protects the list of GPDs.
Quoted 10 lines
drivers/base/power/domain.c +0
-1|| IS_ERR_OR_NULL(dev->pm_domain) || pd_to_genpd(dev->pm_domain) != genpd)return -EINVAL ; --1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kind regards
Uffe
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 7 lines · Ulf Hansson
Quoted 4 lines · Russell King
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 indev->pm_domain(eg,vga_switcheroostuff.)Could the "
vga_switcheroo" code deal with this instead?
How is there any possibility what so ever that vga_switcherroo could deal with this?
The problem is if something which isn't a generic PM domain is registered in dev->pm_domain, pm_genpd_remove_device() will treat it as a generic PM domain, and try and de-reference it as if it were a generic PM domain.
The problem is the generic PM domain code _assuming_ that whatever it finds in dev->pm_domain is always a generic PM domain. That is a broken assumption.
Quoted 16 lines
Quoted 16 lines
Signed-off-by Russell King <[email protected]>--- drivers/base/power/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/base/power/domain.c +0
-0@@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,dev_dbg (dev,"%s()\n" , __func__);
Quoted 8 lines
drivers/base/power/domain.c int pm_genpd_remove_device(struct generic_pm_domain *genpd, · line 1512
Quoted 2 lines
drivers/base/power/domain.c +1
-1- if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev) + if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)The are two issues with this approach.
1.
pm_genpd_present()is build only forCONFIG_PM_SLEEPset.
2.pm_genpd_present()totally ignores holding the mutex which protects the list of GPDs.
Okay, I'll fix both of those by making it always available and by taking the appropriate lock, and removing the duplication in genpd_dev_pm_detach().
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 12 lines · Russell King - ARM Linux
Quoted 7 lines · Ulf Hansson
Quoted 4 lines · Russell King
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 indev->pm_domain(eg,vga_switcheroostuff.)Could the "
vga_switcheroo" code deal with this instead?How is there any possibility what so ever that
vga_switcherroocould deal with this?
Quoted 7 lines
The problem is if something which isn't a generic PM domain is registered in
dev->pm_domain,pm_genpd_remove_device()will treat it as a generic PM domain, and try and de-reference it as if it were a generic PM domain.The problem is the generic PM domain code _assuming_ that whatever it finds in
dev->pm_domainis always a generic PM domain. That is a broken assumption.
Indeed, currently it's not possible to know which derivative of dev_pm_domain is used. For genpd, you have to look it up in the genpd list.
This also complicates adding debug code.
In addition, it can be statically or dynamically allocated. Hence sometimes %ps can tell you the type, sometimes it can't.
Should we add a type field to struct dev_pm_domain?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 22 lines · Geert Uytterhoeven
Quoted 12 lines · Russell King - ARM Linux
Quoted 7 lines · Ulf Hansson
Quoted 4 lines · Russell King
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 indev->pm_domain(eg,vga_switcheroostuff.)Could the "
vga_switcheroo" code deal with this instead?How is there any possibility what so ever that
vga_switcherroocould deal with this?Quoted 7 lines
The problem is if something which isn't a generic PM domain is registered in
dev->pm_domain,pm_genpd_remove_device()will treat it as a generic PM domain, and try and de-reference it as if it were a generic PM domain.The problem is the generic PM domain code _assuming_ that whatever it finds in
dev->pm_domainis always a generic PM domain. That is a broken assumption.
Quoted 7 lines
Indeed, currently it's not possible to know which derivative of
dev_pm_domainis used. For genpd, you have to look it up in the genpd list.
This also complicates adding debug code.
In addition, it can be statically or dynamically allocated. Hence sometimes %ps can tell you the type, sometimes it can't.Should we add a type field to struct
dev_pm_domain?
Then we'd have to keep track of all dev_pm_domain derivatives somehow and I'd prefer to avoid that if possible.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
In reply to Ulf HanssonQuoted 9 lines · Ulf Hansson
Quoted 8 lines · Russell King
drivers/base/power/domain.c +0
-0@@ -1509,7 +1509,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,dev_dbg (dev,"%s()\n" , __func__);
Quoted 8 lines
drivers/base/power/domain.c int pm_genpd_remove_device(struct generic_pm_domain *genpd, · line 1512
Quoted 2 lines
drivers/base/power/domain.c +1
-1- if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev) + if (!pm_genpd_present(genpd) || IS_ERR_OR_NULL(dev)The are two issues with this approach.
1.
pm_genpd_present()is build only forCONFIG_PM_SLEEPset.
2.pm_genpd_present()totally ignores holding the mutex which protects the list of GPDs.
Well, having looked more deeply at this, the PM domain code has more problems.
void pm_genpd_syscore_poweroff(struct device *dev)
{
genpd_syscore_switch(dev, true);
}EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweroff);
void pm_genpd_syscore_poweron(struct device *dev)
{
genpd_syscore_switch(dev, false);
}EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
both of these functions are externally callable without holding the gpd_list_lock mutex, which results in:
static void genpd_syscore_switch(struct device *dev, bool suspend)
{
struct generic_pm_domain *genpd;
genpd = dev_to_genpd(dev);
if (!pm_genpd_present(genpd))
return;calling this function without holding the gpd_list_lock either.
It also seems that there's a variety of ways which the generic PM domain code gets from a struct device to a generic_pm_domain - one is the above. Another is having this open coded:
mutex_lock(&gpd_list_lock);
list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
if (&gpd->domain == dev->pm_domain) {
pd = gpd;
break;
}
}
mutex_unlock(&gpd_list_lock);This is only _not_ buggy because we only ever add to the list, never remove from it, so we can be sure that a successfully obtained "pd" will remain valid after the lock is dropped.
Another is:
genpd = dev_to_genpd(dev);
if (IS_ERR(genpd))
...when we're sure that dev->pm_domain points at a generic PM domain (in the various runtime PM functions.) It should be also noted that dev_to_genpd() is exported to the whole kernel - is that safe?
In any case, I think dev_to_genpd() at least needs some commentry before it saying that it is only for use when we know for certain that the device has a generic_pm_domain attached, or the PM domain power is NULL or an error pointer.
This is turning out to be a right can of worms...
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
In reply to Russell KingQuoted 2 lines · Russell King
pm_genpd_remove_device()should only be called with valid and present pm domain.
Terminology nit: the patch adds a check for a valid an present genpd, not pm domain (which I think of as dev->pm_domain.)
Quoted 3 lines
There are circumstances where we may end up with something that isn't a generic PM domain in
dev->pm_domain(eg,vga_switcheroostuff.)
Wouldn't that particular case be caught by this check in pm_genpd_remove_device():
|| pd_to_genpd(dev->pm_domain) != genpd
Maybe I'm not quite following your description, but it looks like someone is trying to call pm_genpd_remove_device() on something that was not added with pm_genpd_add_device(). Where/how is that happening?
Kevin
Re: [PATCH 2/9] pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 7 lines · Kevin Hilman
Quoted 2 lines · Russell King
pm_genpd_remove_device()should only be called with valid and present pm domain.Terminology nit: the patch adds a check for a valid an present genpd, not pm domain (which I think of as
dev->pm_domain.)
Right, people are sloppy with terminology, it's my turn to be equally sloppy :) I'll fix it eventually (but I've just sent the next round of patches.) Can you re-comment on the new set so it doesn't get lost please? I'm not about to immediately go through another update round for such a trivial change (which alone takes about 5 minutes due to the number of branches and patches on top of this stuff).
Quoted 7 lines
Quoted 3 lines
There are circumstances where we may end up with something that isn't a generic PM domain in
dev->pm_domain(eg,vga_switcheroostuff.)Wouldn't that particular case be caught by this check in
pm_genpd_remove_device():|| pd_to_genpd(dev->pm_domain) != genpd
Well, let's look at what pd_to_genpd() does:
static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
{
return container_of(pd, struct generic_pm_domain, domain);
}Now, both pd_to_genpd() and dev_to_genpd() are available for use outside of the genpd code. What that means is that other code _can_ decide to do this (and indeed I do have code which does this):
struct generic_pm_domain *genpd = dev_to_genpd(dev);
while (1) {
if (pm_genpd_remove_device(genpd, dev) != -EAGAIN)
break;
cond_resched();
}Now, as there is no way for anything outside of genpd to know whether the return value from this is a real genpd structure or not - and pm_genpd_remove_device() tries hard to validate the genpd passed in - but not sufficiently to catch bad cases where dev->pm_domain is not attached to a genpd.
What I'm doing is ensuring that the weak-but-looking-like-belt-and-braces test in pm_genpd_remove_device() is actually sufficiently strong to catch bad uses which may otherwise result in subtle out-of-bounds corruption or list corruption.
Now, the alternative solution would be to have pm_genpd_lookup_name() exported from the genpd code, so I can actually look up the genpd which is _supposed_ to be registered against the device and use that instead. However, that's potentially dangerous because there is no refcounting against genpds obtained in this way.
So I decided better validation was the lesser of the evils.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 3/9 pm: domains: sync runtime PM status with PM domains after probe
In reply to Russell King - ARM LinuxSynchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
--- drivers/base/platform.c | 2 ++ drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 23 +++++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 5 files changed, 45 insertions(+)
drivers/base/platform.c +2 -0
@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe (dev);if (ret)dev_pm_domain_detach (_dev, true); +else +dev_pm_domain_sync (_dev); }if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER ) {
drivers/base/power/common.c +15 -0
@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach (dev, power_off); }EXPORT_SYMBOL_GPL (dev_pm_domain_detach); + +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +* +* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/ +void dev_pm_domain_sync (struct device *dev) +{ +if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync (dev); +}
drivers/base/power/domain.c +23 -0
@@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)genpd_queue_power_off_work (pd); } +static void genpd_dev_pm_sync (struct device *dev) +{ +struct generic_pm_domain *pd =NULL , *gpd; + +if (!dev->pm_domain) +return ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) { + pd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock); + +if (!pd) +return ; + +genpd_queue_power_off_work (pd); +} +/** * genpd_dev_pm_attach - Attach a device to its PM domain using DT. * @dev: Device to attach.
@@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; + dev->pm_domain->sync = genpd_dev_pm_sync;pm_genpd_poweron (pd);return 0 ;
include/linux/pm.h +1 -0
@@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);struct dev_pm_domain {struct dev_pm_ops ops;void (*detach)(struct device *dev,bool power_off); +void (*sync)(struct device *dev); };/*
include/linux/pm_domain.h +4 -0
@@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,#ifdef CONFIG_PM extern int dev_pm_domain_attach (struct device *dev,bool power_on);extern void dev_pm_domain_detach (struct device *dev,bool power_off); +void dev_pm_domain_sync (struct device *dev);#else static inline int dev_pm_domain_attach (struct device *dev,bool power_on) {return -ENODEV ; }static inline void dev_pm_domain_detach (struct device *dev,bool power_off) {} +static inline void dev_pm_domain_sync (struct device *dev) +{ +}#endif #endif /* _LINUX_PM_DOMAIN_H */
Signature · 1 line
1.8.3.1
Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe
Quoted 15 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
Quoted 5 lines
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Signed-off-by Russell King <[email protected]>
That works for me.
Kevin, Ulf?
Quoted 119 lines
--- drivers/base/platform.c | 2 ++ drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 23 +++++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 5 files changed, 45 insertions(+)drivers/base/platform.c +2
-0@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe (dev);if (ret)dev_pm_domain_detach (_dev, true); +else +dev_pm_domain_sync (_dev); }if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER ) {drivers/base/power/common.c +15
-0@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach (dev, power_off); }EXPORT_SYMBOL_GPL (dev_pm_domain_detach); + +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +* +* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/ +void dev_pm_domain_sync (struct device *dev) +{ +if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync (dev); +}drivers/base/power/domain.c +23
-0@@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)genpd_queue_power_off_work (pd); } +static void genpd_dev_pm_sync (struct device *dev) +{ +struct generic_pm_domain *pd =NULL , *gpd; + +if (!dev->pm_domain) +return ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) { + pd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock); + +if (!pd) +return ; + +genpd_queue_power_off_work (pd); +} +/** * genpd_dev_pm_attach - Attach a device to its PM domain using DT. * @dev: Device to attach. @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; + dev->pm_domain->sync = genpd_dev_pm_sync;pm_genpd_poweron (pd);return 0 ;include/linux/pm.h +1
-0@@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);struct dev_pm_domain {struct dev_pm_ops ops;void (*detach)(struct device *dev,bool power_off); +void (*sync)(struct device *dev); };/* include/linux/pm_domain.h +4
-0@@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,#ifdef CONFIG_PM extern int dev_pm_domain_attach (struct device *dev,bool power_on);extern void dev_pm_domain_detach (struct device *dev,bool power_off); +void dev_pm_domain_sync (struct device *dev);#else static inline int dev_pm_domain_attach (struct device *dev,bool power_on) {return -ENODEV ; }static inline void dev_pm_domain_detach (struct device *dev,bool power_off) {} +static inline void dev_pm_domain_sync (struct device *dev) +{ +}#endif #endif /* _LINUX_PM_DOMAIN_H */
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe
In reply to Russell KingQuoted 15 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
Quoted 7 lines
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Signed-off-by Russell King <[email protected]>--- drivers/base/platform.c | 2 ++
Don't we need this for more buses than the platform bus?
Quoted 32 lines
drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 23 +++++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 5 files changed, 45 insertions(+)drivers/base/platform.c +2
-0@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe (dev);if (ret)dev_pm_domain_detach (_dev, true); +else +dev_pm_domain_sync (_dev); }if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER ) {drivers/base/power/common.c +5
-0@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach (dev, power_off); }EXPORT_SYMBOL_GPL (dev_pm_domain_detach); + +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +*
drivers/base/power/common.c void dev_pm_domain_detach(struct device *dev, bool power_off) · line 151
Quoted 10 lines
drivers/base/power/common.c +10
-0+* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/ +void dev_pm_domain_sync (struct device *dev) +{ +if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync (dev); +}
This is more of a taste and flavour comment, regarding the design approach.
To address the issue which @subject patch does, and together with the original problem, which was about making sure a PM domain stays powered during probe, that to me seems like a perfect match for a get/put API.
The current solution from commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes"), is to me just a hacky workaround (which obviously wasn't the proper solution) . $subject patch follows that approach.
What do you think of using a get/put approach instead, that would also give us nice symmetry of the API. As you know I have patches available for this, I am happy to post them if needed.
Quoted 13 lines
drivers/base/power/domain.c +5
-0@@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)genpd_queue_power_off_work (pd); } +static void genpd_dev_pm_sync (struct device *dev) +{ +struct generic_pm_domain *pd =NULL , *gpd; + +if (!dev->pm_domain)
drivers/base/power/domain.c static void genpd_dev_pm_detach(struct device *dev, bool power_off) · line 2174
Quoted 10 lines
drivers/base/power/domain.c +10
-0+return ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) { + pd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock);
Walking through the gpd list seems a bit "heavy". Can't we just expect the caller to have a valid generic_pm_domain pointer for its device?
Quoted 54 lines
drivers/base/power/domain.c +8
-0+ +if (!pd) +return ; + +genpd_queue_power_off_work (pd); +} +/** * genpd_dev_pm_attach - Attach a device to its PM domain using DT. * @dev: Device to attach. @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; + dev->pm_domain->sync = genpd_dev_pm_sync;pm_genpd_poweron (pd);return 0 ;include/linux/pm.h +1
-0@@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);struct dev_pm_domain {struct dev_pm_ops ops;void (*detach)(struct device *dev,bool power_off); +void (*sync)(struct device *dev); };/* include/linux/pm_domain.h +4
-1@@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,#ifdef CONFIG_PM extern int dev_pm_domain_attach (struct device *dev,bool power_on);extern void dev_pm_domain_detach (struct device *dev,bool power_off); +void dev_pm_domain_sync (struct device *dev);#else static inline int dev_pm_domain_attach (struct device *dev,bool power_on) {return -ENODEV ; }static inline void dev_pm_domain_detach (struct device *dev,bool power_off) {} +static inline void dev_pm_domain_sync (struct device *dev) +{ +}#endif #endif /* _LINUX_PM_DOMAIN_H */ --1.8.3.1
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kind regards
Uffe
Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe
Quoted 20 lines · Ulf Hansson
Quoted 19 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Quoted 5 lines
Quoted 3 lines
Signed-off-by Russell King <[email protected]>--- drivers/base/platform.c | 2 ++Don't we need this for more buses than the platform bus?
As you very well know, only the platform bus does this automatic binding of a PM domain based on OF - something that you yourself were involved in adding (your sign-off is on the patch adding it, so I assume that you reviewed that patch as thoroughly as you seem to be reviewing mine) which is the cause of my problems.
Quoted 50 lines
Quoted 32 lines
drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 23 +++++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 5 files changed, 45 insertions(+)drivers/base/platform.c +2
-0@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe (dev);if (ret)dev_pm_domain_detach (_dev, true); +else +dev_pm_domain_sync (_dev); }if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER ) {drivers/base/power/common.c +5
-0@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach (dev, power_off); }EXPORT_SYMBOL_GPL (dev_pm_domain_detach); + +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +* drivers/base/power/common.c void dev_pm_domain_detach(struct device *dev, bool power_off) · line 151
Quoted 10 lines
drivers/base/power/common.c +10
-0+* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/ +void dev_pm_domain_sync (struct device *dev) +{ +if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync (dev); +}This is more of a taste and flavour comment, regarding the design approach.
To address the issue which @subject patch does, and together with the original problem, which was about making sure a PM domain stays powered during probe, that to me seems like a perfect match for a get/put API.
Quoted 8 lines
The current solution from commit
2ed127697eb1("PM / Domains: Power on the PM domain right after attach completes"), is to me just a hacky workaround (which obviously wasn't the proper solution) . $subject patch follows that approach.What do you think of using a get/put approach instead, that would also give us nice symmetry of the API. As you know I have patches available for this, I am happy to post them if needed.
What I think you're proposing is nothing less than a total rewrite of the PM domain code.
If that's what it's going to take to get this stuff in, then I'm just not interested in persuing this anymore, sorry. I don't have the time and effort for that - something that people well know when they send me emails that go unanswered...
I'm just trying to fix the problem which you created - and this is the way which was discussed and settled upon. If you _now_ want a different approach, that's up to _you_ to implement. Stop wasting my time.
Quoted 18 lines
Quoted 18 lines
drivers/base/power/domain.c +10
-0@@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)genpd_queue_power_off_work (pd); } +static void genpd_dev_pm_sync (struct device *dev) +{ +struct generic_pm_domain *pd =NULL , *gpd; + +if (!dev->pm_domain) +return ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) {
Quoted 8 lines
drivers/base/power/domain.c static void genpd_dev_pm_detach(struct device *dev, bool power_off) · line 2174
Quoted 5 lines
drivers/base/power/domain.c +5
-0+ pd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock);Walking through the gpd list seems a bit "heavy". Can't we just expect the caller to have a valid
generic_pm_domainpointer for its device?
No you can't. See the second patch in this series. dev->pm_domain can contain other stuff which isn't a generic_pm_domain. generic_pm_domain is just one instance of a pm_domain implementation. Others exist.
I would have assumed you would know these details as you have decided to co-maintain this code, or if not, you'd be prepared to audit the kernel to find out what might be in dev->pm_domain so that you have due diligence before commenting on something you clearly know nothing about...
What I find even _more_ unacceptable is that you are question this, but you didn't question the exact same code which was part of Tomasz Figa's patch to add the OF-based PM domain code. On the face of it, this strikes of double standards - somehow I have to justify my code more than other people do.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe
Quoted 25 lines · Russell King - ARM Linux
Quoted 24 lines · Ulf Hansson
Quoted 22 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Signed-off-by Russell King <[email protected]>--- drivers/base/platform.c | 2 ++
Quoted 7 lines
Quoted 1 line
Don't we need this for more buses than the platform bus?
As you very well know, only the platform bus does this automatic binding of a PM domain based on OF - something that you yourself were involved in adding (your sign-off is on the patch adding it, so I assume that you reviewed that patch as thoroughly as you seem to be reviewing mine) which is the cause of my problems.
Besides the platform bus, we also have spi, sdio, i2c, amba. I was thinking that those should suffer some that same issue you are trying to fix, right?
Quoted 56 lines
Quoted 56 lines
Quoted 33 lines
drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 23 +++++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 5 files changed, 45 insertions(+)drivers/base/platform.c +2
-0@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe (dev);if (ret)dev_pm_domain_detach (_dev, true); +else +dev_pm_domain_sync (_dev); }if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER ) {drivers/base/power/common.c +5
-0@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach (dev, power_off); }EXPORT_SYMBOL_GPL (dev_pm_domain_detach); + +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +* drivers/base/power/common.c void dev_pm_domain_detach(struct device *dev, bool power_off) · line 151
Quoted 10 lines
drivers/base/power/common.c +10
-0+* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/ +void dev_pm_domain_sync (struct device *dev) +{ +if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync (dev); +}This is more of a taste and flavour comment, regarding the design approach.
To address the issue which @subject patch does, and together with the original problem, which was about making sure a PM domain stays powered during probe, that to me seems like a perfect match for a get/put API.
The current solution from commit
2ed127697eb1("PM / Domains: Power on the PM domain right after attach completes"), is to me just a hacky workaround (which obviously wasn't the proper solution) . $subject patch follows that approach.
Quoted 6 lines
Quoted 3 lines
What do you think of using a get/put approach instead, that would also give us nice symmetry of the API. As you know I have patches available for this, I am happy to post them if needed.
What I think you're proposing is nothing less than a total rewrite of the PM domain code.
No, that's not what I proposed. Sorry if I was unclear.
Instead of the >sync() API+callback, my proposual was to add a ->get() and a ->put() API+callback. The APIs will have to be called from the buses probe functions, to make sure the PM domain stays powered during probe and gets gated if possible when leaving probe.
That will then make it possible to revert 2ed127697eb1 ("PM / Domains:
Power on the PM domain right after attach completes")
I will (re)post these patches, since it's easier to discuss/look at code directly.
Quoted 46 lines
If that's what it's going to take to get this stuff in, then I'm just not interested in persuing this anymore, sorry. I don't have the time and effort for that - something that people well know when they send me emails that go unanswered...
I'm just trying to fix the problem which you created - and this is the way which was discussed and settled upon. If you _now_ want a different approach, that's up to _you_ to implement. Stop wasting my time.
Quoted 18 lines
Quoted 18 lines
drivers/base/power/domain.c +10
-0@@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)genpd_queue_power_off_work (pd); } +static void genpd_dev_pm_sync (struct device *dev) +{ +struct generic_pm_domain *pd =NULL , *gpd; + +if (!dev->pm_domain) +return ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) {Quoted 8 lines
drivers/base/power/domain.c static void genpd_dev_pm_detach(struct device *dev, bool power_off) · line 2174
Quoted 5 lines
drivers/base/power/domain.c +5
-0+ pd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock);Walking through the gpd list seems a bit "heavy". Can't we just expect the caller to have a valid
generic_pm_domainpointer for its device?No you can't. See the second patch in this series.
dev->pm_domaincan contain other stuff which isn't ageneric_pm_domain.generic_pm_domainis just one instance of apm_domainimplementation. Others exist.I would have assumed you would know these details as you have decided to co-maintain this code, or if not, you'd be prepared to audit the kernel to find out what might be in
dev->pm_domainso that you have due diligence before commenting on something you clearly know nothing about...
Quoted 5 lines
What I find even _more_ unacceptable is that you are question this, but you didn't question the exact same code which was part of Tomasz Figa's patch to add the OF-based PM domain code. On the face of it, this strikes of double standards - somehow I have to justify my code more than other people do.
I am really sorry if I made you upset Russell, I was trying to ask for you most valuable feedback, but apparently I failed miserably.
Kind regards
Uffe
Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe
In reply to Ulf HanssonQuoted 20 lines · Ulf Hansson
Quoted 19 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Quoted 5 lines
Quoted 3 lines
Signed-off-by Russell King <[email protected]>--- drivers/base/platform.c | 2 ++Don't we need this for more buses than the platform bus?
It's worth noting that I won't be fixing SDIO:
sdio_acpi_set_handle(func);
ret = device_add(&func->dev);
if (ret == 0) {
sdio_func_set_present(func);
dev_pm_domain_attach(&func->dev, false);
}which is inherently racy - the driver can be probed in device_add(). What happened to "setup stuff, then publish"...
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Re: [PATCH 3/9] pm: domains: sync runtime PM status with PM domains after probe
In reply to Russell KingQuoted 15 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
Quoted 5 lines
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Signed-off-by Russell King <[email protected]>
I think this is a good fix to the existing problem. One minor nit on a comment below, otherwise
[...]
drivers/base/power/common.c void dev_pm_domain_detach(struct device *dev, bool power_off) · line 146
Quoted 10 lines
drivers/base/power/common.c +10
-0+ +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +* +* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/
Tt's not the *device* tha needs to be properly suspended after the probe (since it's already/still runtime suspended), but the pm_domain that would be potentially powered down. Hence, I'd reword the last sentence slightly:
This ensures that a device which enables runtime PM in suspended state, and never transitions to active in its probe handler gives an opportunity for the PM domain to be powered down after the probe.
Kevin
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
In reply to Russell King - ARM LinuxThe 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.
--- arch/arm/mach-mvebu/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 399 +++++++++++++++++++++++++++++++++++++++++++ include/linux/soc/dove/pmu.h | 6 + 5 files changed, 408 insertions(+) create mode 100644 drivers/soc/dove/Makefile create mode 100644 drivers/soc/dove/pmu.c create mode 100644 include/linux/soc/dove/pmu.h
arch/arm/mach-mvebu/Kconfig +1 -0
@@ -82,6 +82,7 @@ config MACH_DOVE select MACH_MVEBU_ANY select ORION_IRQCHIP select ORION_TIMER + select PM_GENERIC_DOMAINS if PM select PINCTRL_DOVE help Say 'Y' here if you want your kernel to support the
drivers/soc/Makefile +1 -0
@@ -2,6 +2,7 @@ # Makefile for the Linux Kernel SOC specific device drivers. # +obj-$(CONFIG_MACH_DOVE) += dove/ obj-$(CONFIG_ARCH_QCOM) += qcom/ obj-$(CONFIG_ARCH_TEGRA) += tegra/ obj-$(CONFIG_SOC_TI) += ti/
drivers/soc/dove/Makefile new file +1 -0
@@ -0,0 +1 @@ +obj-y += pmu.o
drivers/soc/dove/pmu.c new file +399 -0
399 lines not on this page · raw message
include/linux/soc/dove/pmu.h new file +6 -0
@@ -0,0 +1,6 @@ +#ifndef LINUX_SOC_DOVE_PMU_H +#define LINUX_SOC_DOVE_PMU_H + +int dove_init_pmu (void ); + +#endif
Signature · 1 line
1.8.3.1
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
drivers/soc/dove/pmu.c line 338
Quoted 10 lines · On Thursday 12 March 2015 18:31:15 Russell King wrote:
drivers/soc/dove/pmu.c new file +10
-0+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,dove-pmu" ); +if (!np_pmu) +return 0 ;
What is the reason that this is not a platform_driver? I think you should try to make it one, or explain in the changelog the reason for not making it one. This obviously ties in with the question I asked about who calls dove_init_pmu().
drivers/soc/dove/pmu.c line 388
Quoted 7 lines
drivers/soc/dove/pmu.c new file +7
-0+ 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); + }
In particular, manually parsing the "resets" property is something we should try to avoid. With a platform driver, this could become devm_reset_control_get(), otherwise I think of_reset_control_get() would also do the right thing here.
Arnd
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
Quoted 16 lines · Arnd Bergmann
drivers/soc/dove/pmu.c line 338
Quoted 10 lines · On Thursday 12 March 2015 18:31:15 Russell King wrote:
drivers/soc/dove/pmu.c new file +10
-0+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,dove-pmu" ); +if (!np_pmu) +return 0 ;What is the reason that this is not a
platform_driver? I think you should try to make it one, or explain in the changelog the reason for not making it one. This obviously ties in with the question I asked about who callsdove_init_pmu().
It's because I need for the PM domain support to be available early right now, and much of this code pre-dates the integration of OF with PM domains. Even booting with DT, I still need some platform devices manually declared and bound to these PM domains in order to (a) properly test this code, (b) test other parts of the system effectively, and
(c) have a working system.
However, I do agree that in the longer term this should probably be converted to a platform device driver, assuming that everything that makes use of it copes with it.
However, one thing that is a concern with that is the PM domain code does not yet support the removal of PM domains. I guess we should ensure that PM domain providers set driver.suppress_bind_attrs so that at least userspace can't unbind these drivers.
Quoted 8 lines
drivers/soc/dove/pmu.c line 388
Quoted 7 lines
drivers/soc/dove/pmu.c new file +7
-0+ 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); + }
Quoted 4 lines
In particular, manually parsing the "resets" property is something we should try to avoid. With a platform driver, this could become
devm_reset_control_get(), otherwise I thinkof_reset_control_get()would also do the right thing here.
The hardware requires a specific sequence of register writes for the PM domain code, which includes the reset register.
The problem is that if we were to use the reset API directly from the PM domain code, we would have to separate the locks for the reset code from the PM domain code. That then leads to there being a race between the reset code potentially being able to write to the reset register in the middle of a PM domain sequence.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
Quoted 25 lines · On Friday 13 March 2015 12:29:44 Russell King - ARM Linux wrote:
Quoted 16 lines · Arnd Bergmann
drivers/soc/dove/pmu.c line 338
Quoted 10 lines · On Thursday 12 March 2015 18:31:15 Russell King wrote:
drivers/soc/dove/pmu.c new file +10
-0+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,dove-pmu" ); +if (!np_pmu) +return 0 ;What is the reason that this is not a
platform_driver? I think you should try to make it one, or explain in the changelog the reason for not making it one. This obviously ties in with the question I asked about who callsdove_init_pmu().It's because I need for the PM domain support to be available early right now, and much of this code pre-dates the integration of OF with PM domains. Even booting with DT, I still need some platform devices manually declared and bound to these PM domains in order to (a) properly test this code, (b) test other parts of the system effectively, and
(c) have a working system.
Quoted 3 lines
However, I do agree that in the longer term this should probably be converted to a platform device driver, assuming that everything that makes use of it copes with it.
Ok. Given that the device is (also) an irqchip, would it work to use IRQCHIP_DECLARE() to have the driver initialized at init_irq() time as an alternative, or is that too early for some of the things this driver does?
Quoted 8 lines
The hardware requires a specific sequence of register writes for the PM domain code, which includes the reset register.
The problem is that if we were to use the reset API directly from the PM domain code, we would have to separate the locks for the reset code from the PM domain code. That then leads to there being a race between the reset code potentially being able to write to the reset register in the middle of a PM domain sequence.
I see. I may be missing something here, but I think you could still use of_reset_controller_get() once the reset controller is enabled and then manually access the register from the PM domain code while holding the pmu lock. You wouldn't be able to use device_reset() or similar as you explain, but you could avoid parsing the DT manually.
Arnd
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
Quoted 10 lines · Arnd Bergmann
Quoted 8 lines · On Friday 13 March 2015 12:29:44 Russell King - ARM Linux wrote:
The hardware requires a specific sequence of register writes for the PM domain code, which includes the reset register.
The problem is that if we were to use the reset API directly from the PM domain code, we would have to separate the locks for the reset code from the PM domain code. That then leads to there being a race between the reset code potentially being able to write to the reset register in the middle of a PM domain sequence.
Quoted 5 lines
I see. I may be missing something here, but I think you could still use
of_reset_controller_get()once the reset controller is enabled and then manually access the register from the PM domain code while holding the pmu lock. You wouldn't be able to usedevice_reset()or similar as you explain, but you could avoid parsing the DT manually.
What about the case when CONFIG_RESET_CONTROLLER is disabled? In that case, I would then need to manually parse this anyway.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
Quoted 11 lines · On Friday 13 March 2015 12:47:48 Russell King - ARM Linux wrote:
Quoted 10 lines · Arnd Bergmann
Quoted 8 lines · On Friday 13 March 2015 12:29:44 Russell King - ARM Linux wrote:
The hardware requires a specific sequence of register writes for the PM domain code, which includes the reset register.
The problem is that if we were to use the reset API directly from the PM domain code, we would have to separate the locks for the reset code from the PM domain code. That then leads to there being a race between the reset code potentially being able to write to the reset register in the middle of a PM domain sequence.
Quoted 8 lines
Quoted 5 lines
I see. I may be missing something here, but I think you could still use
of_reset_controller_get()once the reset controller is enabled and then manually access the register from the PM domain code while holding the pmu lock. You wouldn't be able to usedevice_reset()or similar as you explain, but you could avoid parsing the DT manually.What about the case when
CONFIG_RESET_CONTROLLERis disabled? In that case, I would then need to manually parse this anyway.
I think the easiest and cleanest way would be to let that case not happen. Adding the reset controller core makes the kernel about 2kb larger, but in most multiplatform configurations, you get it already.
Arnd
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
Quoted 3 lines · Arnd Bergmann
I think the easiest and cleanest way would be to let that case not happen. Adding the reset controller core makes the kernel about 2kb larger, but in most multiplatform configurations, you get it already.
Well, I personally hate selecting visible Kconfig symbols. It's against my religion.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
Quoted 8 lines · On Friday 13 March 2015 15:28:43 Russell King - ARM Linux wrote:
Quoted 3 lines · Arnd Bergmann
I think the easiest and cleanest way would be to let that case not happen. Adding the reset controller core makes the kernel about 2kb larger, but in most multiplatform configurations, you get it already.
Well, I personally hate selecting visible
Kconfigsymbols. It's against my religion.
I know what you mean. I already created a patch for this:
https://lkml.org/lkml/2015/2/24/317
Need to dig that out again and address the comments I got.
Arnd
PATCH 5/9 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
In reply to Arnd BergmannQuoted 4 lines · Arnd Bergmann
Ok. Given that the device is (also) an irqchip, would it work to use
IRQCHIP_DECLARE()to have the driver initialized atinit_irq()time as an alternative, or is that too early for some of the things this driver does?
I've just looked at that - it's not as easy as that, the macro is defined in drivers/irqchip/irqchip.h which would need a relative path to pick up - I'm not a fan of relative include paths.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
In reply to Russell King - ARM LinuxAdd the PMU description to the Dove DT file. The PMU provides multiple features, including an interrupt, reset, power and isolation controller.
--- arch/arm/boot/dts/dove.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/arm/boot/dts/dove.dtsi +9 -0
@@ -380,6 +380,15 @@ status = "disabled"; }; + pmu: power-management at d0000 { + compatible = "marvell,dove-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>;
Signature · 1 line
1.8.3.1
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
Dear Russell King,
arch/arm/boot/dts/dove.dtsi line 385
Quoted 3 lines · Russell King
arch/arm/boot/dts/dove.dtsi +3
-0+ pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>;
If I'm not wrong, the registers of the RTC are at 0xd8500, which is also covered by this new pmu node. It does work because you're using of_iomap(), but it isn't really ideal.
Maybe some sort of syscon/regmap thing should be used?
Best regards,
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
Quoted 12 lines · Thomas Petazzoni
Dear Russell King,
arch/arm/boot/dts/dove.dtsi line 385
Quoted 3 lines · Russell King
arch/arm/boot/dts/dove.dtsi +3
-0+ pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>;If I'm not wrong, the registers of the RTC are at
0xd8500, which is also covered by this new pmu node. It does work because you're usingof_iomap(), but it isn't really ideal.
Quoted 1 line
Maybe some sort of syscon/regmap thing should be used?
Please look at the locking that would require and the locking required for PM domains (and as I've previously explained, apparently the hardware requires a specific sequence of writes.)
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
In reply to Russell KingHi Russell,
Quoted 13 lines · Russell King
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 <[email protected]>--- arch/arm/boot/dts/dove.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)arch/arm/boot/dts/dove.dtsi +0
-0
arch/arm/boot/dts/dove.dtsi line 385
Quoted 6 lines
arch/arm/boot/dts/dove.dtsi +3
-0@@ -380,6 +380,15 @@ status = "disabled"; }; + pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>;
Here you overlap some other nodes such as the thermal one (from 0xd001c to 0xd0028), the clock gate one (from 0xd0038 to 0xd003c), the gpio one, the pinctrl one ...
Could you be more specific?
Thanks,
Gregory
Quoted 10 lines
arch/arm/boot/dts/dove.dtsi +6
-0+ interrupts = <33>; + interrupt-controller; + #interrupt-cells = <1>; + #reset-cells = <1>; + }; + thermal: thermal-diode at d001c { compatible = "marvell,dove-thermal"; reg = <0xd001c 0x0c>, <0xd005c 0x08>;
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
Quoted 7 lines · Gregory CLEMENT
arch/arm/boot/dts/dove.dtsi line 385
Quoted 3 lines
arch/arm/boot/dts/dove.dtsi +3
-0+ pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>;Here you overlap some other nodes such as the thermal one (from
0xd001cto0xd0028), the clock gate one (from0xd0038to0xd003c), the gpio one, the pinctrl one ...
Yes, I'm well aware of that, but here's the thing...
I'm describing the hardware here.
The real problem is that Dove (etc) went down the path of breaking up the PMU device into multiple smaller devices each specifying a sub-set of the register range. By doing that, Dove moved away from describing the hardware - instead, we've described the Linux _implementation_ with its separate (sub-)devices - its the implementation's choice that we'd break up the PMU into these separate devices, almost to the point of specifying each individual register.
What could be done to work around this oversight is to mvoe these devices beneath the PMU node, which IMHO makes complete sense as these are sub-devices of the PMU/PMC rather than separate devices.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Sebastian Hesselbarth
maintainerPATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Gregory CLEMENT
arch/arm/boot/dts/dove.dtsi line 385
Quoted 3 lines
arch/arm/boot/dts/dove.dtsi +3
-0+ pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>;Here you overlap some other nodes such as the thermal one (from
0xd001cto0xd0028), the clock gate one (from0xd0038to0xd003c), the gpio one, the pinctrl one ...
Quoted 7 lines
Yes, I'm well aware of that, but here's the thing...
I'm describing the hardware here.
The real problem is that Dove (etc) went down the path of breaking up the PMU device into multiple smaller devices each specifying a sub-set of the register range. By doing that, Dove moved away from describing
Russell,
I totally agree that from today's point-of-view having a single pmu node with sub-nodes would have been the better approach.
Quoted 8 lines
the hardware - instead, we've described the Linux _implementation_ with its separate (sub-)devices - its the implementation's choice that we'd break up the PMU into these separate devices, almost to the point of specifying each individual register.
What could be done to work around this oversight is to mvoe these devices beneath the PMU node, which IMHO makes complete sense as these are sub-devices of the PMU/PMC rather than separate devices.
If we do this, we should have a look at syscon and potentially also simple-mfd which will allow us to share the registers and register platform_devices for the sub-nodes.
The current pmu power/reset/irq can still use the extra locking and directly access MMIO registers - but for the others we should really look into using regmaps.
The only thing here is that power domain descriptions are also encoded as direct sub-nodes of the pmu node, right?
Sebastian
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
Quoted 10 lines · Sebastian Hesselbarth
If we do this, we should have a look at syscon and potentially also simple-mfd which will allow us to share the registers and register
platform_devicesfor the sub-nodes.The current pmu power/reset/irq can still use the extra locking and directly access MMIO registers - but for the others we should really look into using regmaps.
The only thing here is that power domain descriptions are also encoded as direct sub-nodes of the pmu node, right?
Yes, I've been debating about putting those under a domains { } sub-node, like we do with the ports { } sub-node for of-graph stuff.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 6/9 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
In reply to Sebastian HesselbarthQuoted 3 lines · Sebastian Hesselbarth
If we do this, we should have a look at syscon and potentially also simple-mfd which will allow us to share the registers and register
platform_devicesfor the sub-nodes.
I've just sent a pull request for simple-mfd to the ARM SoC maintainers. I see that you Free Electron guys make good use of it.
Yours,
Linus Walleij
PATCH 7/9 ARM: dt: dove: wire up RTC interrupt
In reply to Russell King - ARM LinuxNow that we have a PMU driver, we can wire up the RTC interrupt in the DT description for Dove.
--- arch/arm/boot/dts/dove.dtsi | 2 ++ 1 file changed, 2 insertions(+)
arch/arm/boot/dts/dove.dtsi +2 -0
@@ -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 {
Signature · 1 line
1.8.3.1
PATCH 8/9 ARM: dt: dove: add video decoder power domain description
In reply to Russell King - ARM LinuxAdd the description of the video decoder power domain to the PMU DT entry.
--- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/dove.dtsi +7 -0
@@ -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 {
Signature · 1 line
1.8.3.1
PATCH 9/9 ARM: dt: dove: add GPU power domain description
In reply to Russell King - ARM LinuxAdd the description of the GPU power domain to the PMU DT entry.
--- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/dove.dtsi +7 -0
@@ -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 {
Signature · 1 line
1.8.3.1
cover Dove PMU support
In reply to Russell King - ARM LinuxQuoted 6 lines · On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
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:
Quoted 7 lines
* 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.)
I don't seem to have received the first three patches for some reason. Can you check if you got them back from the mailing list?
Quoted 14 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 +++ arch/arm/boot/dts/dove.dtsi | 25 ++ arch/arm/mach-mvebu/Kconfig | 1 + drivers/base/platform.c | 2 + drivers/base/power/common.c | 15 + drivers/base/power/domain.c | 33 +- drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 399 +++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 + include/linux/soc/dove/pmu.h | 6 + 12 files changed, 532 insertions(+), 5 deletions(-)
I see add the header file and the global dove_init_pmu() function, but I don't see a caller of that function. Is that intentional, or did you accidentally leave out another patch that you meant to include?
Arnd
cover Dove PMU support
Quoted 15 lines · Arnd Bergmann
Quoted 6 lines · On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
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:
Quoted 7 lines
* 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.)
Quoted 2 lines
I don't seem to have received the first three patches for some reason. Can you check if you got them back from the mailing list?
That's because you're not listed as the maintainer for PM domain stuff, and I forgot to ensure lakml was copied on all patches.
Quoted 14 lines
Quoted 5 lines
Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 +++ arch/arm/boot/dts/dove.dtsi | 25 ++ arch/arm/mach-mvebu/Kconfig | 1 + drivers/base/platform.c | 2 + drivers/base/power/common.c | 15 +Quoted 8 lines
drivers/base/power/domain.c | 33 +- drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 399 +++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 + include/linux/soc/dove/pmu.h | 6 + 12 files changed, 532 insertions(+), 5 deletions(-)
Quoted 3 lines
I see add the header file and the global
dove_init_pmu()function, but I don't see a caller of that function. Is that intentional, or did you accidentally leave out another patch that you meant to include?
I kind'a did - it needs an explicit call from arch/arm/mach-mvebu/dove.c which I haven't added even in my tree (because I don't use that path, even when I test DT booting - I still use most of the arch/arm/mach-dove code when DT booting.) I'll add that now.
Of course, I also have a patch which adds legacy support to arch/arm/mach-dove, but I've assumed you're not interested in that...
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
cover Dove PMU support
Quoted 20 lines · On Friday 13 March 2015 12:11:27 Russell King - ARM Linux wrote:
Quoted 15 lines · Arnd Bergmann
Quoted 5 lines · On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 +++ arch/arm/boot/dts/dove.dtsi | 25 ++ arch/arm/mach-mvebu/Kconfig | 1 + drivers/base/platform.c | 2 + drivers/base/power/common.c | 15 +Quoted 8 lines
drivers/base/power/domain.c | 33 +- drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 399 +++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 + include/linux/soc/dove/pmu.h | 6 + 12 files changed, 532 insertions(+), 5 deletions(-)Quoted 3 lines
I see add the header file and the global
dove_init_pmu()function, but I don't see a caller of that function. Is that intentional, or did you accidentally leave out another patch that you meant to include?
Quoted 7 lines
I kind'a did - it needs an explicit call from
arch/arm/mach-mvebu/dove.cwhich I haven't added even in my tree (because I don't use that path, even when I test DT booting - I still use most of thearch/arm/mach-dovecode when DT booting.) I'll add that now.Of course, I also have a patch which adds legacy support to
arch/arm/mach-dove, but I've assumed you're not interested in that...
You mean legacy support in mach-mvebu?
I don't mind that at all, it was the mvebu maintainers that decided it would be best to combine the multiplatform and DT work in order to simplify both. I've also sent a patch set that moves mach-dove into multiplatform, which is a different way of doing the same thing.
Arnd
cover Dove PMU support
Quoted 26 lines · Arnd Bergmann
Quoted 25 lines · On Friday 13 March 2015 12:11:27 Russell King - ARM Linux wrote:
Quoted 15 lines · Arnd Bergmann
Quoted 5 lines · On Thursday 12 March 2015 18:30:21 Russell King - ARM Linux wrote:
Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 +++ arch/arm/boot/dts/dove.dtsi | 25 ++ arch/arm/mach-mvebu/Kconfig | 1 + drivers/base/platform.c | 2 + drivers/base/power/common.c | 15 +Quoted 8 lines
drivers/base/power/domain.c | 33 +- drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 399 +++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 + include/linux/soc/dove/pmu.h | 6 + 12 files changed, 532 insertions(+), 5 deletions(-)Quoted 3 lines
I see add the header file and the global
dove_init_pmu()function, but I don't see a caller of that function. Is that intentional, or did you accidentally leave out another patch that you meant to include?I kind'a did - it needs an explicit call from
arch/arm/mach-mvebu/dove.cwhich I haven't added even in my tree (because I don't use that path, even when I test DT booting - I still use most of thearch/arm/mach-dovecode when DT booting.) I'll add that now.
Quoted 4 lines
Quoted 2 lines
Of course, I also have a patch which adds legacy support to
arch/arm/mach-dove, but I've assumed you're not interested in that...You mean legacy support in mach-mvebu?
No.
As you'll see, this uses a platform device notifier to hook the devices onto the appropriate PM domain, which is why the driver needs to be registered early. This also gets used in DT mode with one legacy platform device which is dynamically registered (something which can't happen with DT.)
arch/arm/Kconfig | 1 + arch/arm/mach-dove/common.c | 25 ++++++++++ arch/arm/mach-dove/include/mach/pm.h | 17 ------- arch/arm/mach-dove/irq.c | 87 -------------------------------- drivers/soc/Makefile | 1 + drivers/soc/dove/pmu.c | 97 ++++++++++++++++++++++++++++++++++++ include/linux/soc/dove/pmu.h | 18 +++++++ 7 files changed, 142 insertions(+), 104 deletions(-)
arch/arm/Kconfig +1 -0
@@ -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
arch/arm/mach-dove/common.c +25 -0
@@ -16,6 +16,7 @@#include <linux/platform_data/dma-mv_xor.h> #include <linux/platform_data/usb-ehci-orion.h> #include <linux/platform_device.h> +#include <linux/soc/dove/pmu.h> #include <asm/hardware/cache-tauros2.h> #include <asm/mach/arch.h> #include <asm/mach/map.h>
@@ -375,6 +376,29 @@ void __init dove_setup_cpu_wins(void) DOVE_SCRATCHPAD_SIZE); } +static const struct dove_pmu_domain_initdata pmu_domains[]__initconst = { + { + .pwr_mask =PMU_PWR_VPU_PWR_DWN_MASK , + .rst_mask =PMU_SW_RST_VIDEO_MASK , + .iso_mask =PMU_ISO_VIDEO_MASK , + .name ="vpu-domain" , + }, { + .pwr_mask =PMU_PWR_GPU_PWR_DWN_MASK , + .rst_mask =PMU_SW_RST_GPU_MASK , + .iso_mask =PMU_ISO_GPU_MASK , + .name ="gpu-domain" , + }, { +/* sentinel */ + }, +}; + +static const struct dove_pmu_initdata pmu_data__initconst = { + .pmc_base =DOVE_PMU_VIRT_BASE , + .pmu_base =DOVE_PMU_VIRT_BASE +0x8000 , + .irq =IRQ_DOVE_PMU , + .domains = pmu_domains, +}; +void __init dove_init (void ) { pr_info("Dove 88AP510 SoC, TCLK = %d MHz.\n",
@@ -389,6 +413,7 @@ void __init dove_init(void)dove_clk_init ();/* internal devices that every board has */ +dove_init_pmu_legacy (&pmu_data);dove_rtc_init ();dove_xor0_init ();dove_xor1_init ();
arch/arm/mach-dove/include/mach/pm.h +0 -17
@@ -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
arch/arm/mach-dove/irq.c +0 -87
111 lines not on this page · raw message
drivers/soc/Makefile +1 -0
@@ -2,6 +2,7 @@ # Makefile for the Linux Kernel SOC specific device drivers. # +obj-$(CONFIG_ARCH_DOVE) += dove/ obj-$(CONFIG_MACH_DOVE) += dove/ obj-$(CONFIG_ARCH_QCOM) += qcom/ obj-$(CONFIG_ARCH_TEGRA) += tegra/
drivers/soc/dove/pmu.c +97 -0
103 lines not on this page · raw message
include/linux/soc/dove/pmu.h +18 -0
@@ -1,6 +1,24 @@#ifndef LINUX_SOC_DOVE_PMU_H #define LINUX_SOC_DOVE_PMU_H +#include <linux/types.h> + +struct dove_pmu_domain_initdata { +u32 pwr_mask; +u32 rst_mask; +u32 iso_mask; +const char *name; +}; + +struct dove_pmu_initdata { +void __iomem *pmc_base; +void __iomem *pmu_base; +int irq; +const struct dove_pmu_domain_initdata *domains; +}; + +int dove_init_pmu_legacy (const struct dove_pmu_initdata *); +int dove_init_pmu (void );#endif
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
cover Dove PMU support
Quoted 7 lines · On Friday 13 March 2015 12:32:11 Russell King - ARM Linux wrote:
Quoted 5 lines · Arnd Bergmann
Quoted 2 lines · On Friday 13 March 2015 12:11:27 Russell King - ARM Linux wrote:
Of course, I also have a patch which adds legacy support to
arch/arm/mach-dove, but I've assumed you're not interested in that...You mean legacy support in mach-mvebu?
Quoted 7 lines
No.
As you'll see, this uses a platform device notifier to hook the devices onto the appropriate PM domain, which is why the driver needs to be registered early. This also gets used in DT mode with one legacy platform device which is dynamically registered (something which can't happen with DT.)
Ok, got it. I think that's fine too. If this helps you spend less time on forward-porting your patches and in turn completing the full dove support with DT faster, I'm all for merging this one too.
Arnd
cover Dove PMU support
In reply to Russell King - ARM LinuxThis is the third re-posting of the patch set which I posted almost 11 months ago to support the Dove PMU, with a few additional changes. This set is based upon 3.19.
In this set are:
* one patch 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. This patches have been updated to apply cleanly to 3.19. This patch should be applied anyway.
* factor out code which gets a validated generic PM domain, which we will make use of in later patches. (new)
* improve the validation of the generic PM domain pointer passed into pm_genpd_remove_device(). (updated)
* synchronise the state of the generic PM domain after a device is probed. Other solutions may be possible, but require a larger patch series to resolve. (updated for patch 2)
* DT binding documentation for the Dove PMU driver, updated with comments from Rob.
* 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/syscon infrastructure, because we would need to hold spinlocks while calling several different sub-drivers.) This currently needs to be available early on in the init sequence, so an explicit initialisation call is added to mach-mvebu to achieve this. (updated)
* 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.
Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 +++ arch/arm/boot/dts/dove.dtsi | 25 ++ arch/arm/mach-mvebu/Kconfig | 1 + arch/arm/mach-mvebu/dove.c | 2 + drivers/amba/bus.c | 4 +- drivers/base/platform.c | 2 + drivers/base/power/common.c | 15 + drivers/base/power/domain.c | 64 +++- drivers/i2c/i2c-core.c | 2 + drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 406 +++++++++++++++++++++ drivers/spi/spi.c | 2 + include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 + include/linux/soc/dove/pmu.h | 6 + 16 files changed, 564 insertions(+), 21 deletions(-)
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 1/10 pm: domains: quieten down generic pm domains
In reply to Russell King - ARM LinuxPM 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.
--- drivers/base/power/domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/base/power/domain.c +4 -4
@@ -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; }
Signature · 1 line
1.8.3.1
PATCH 1/10 pm: domains: quieten down generic pm domains
Quoted 8 lines · Russell King
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 Ulf Hansson <[email protected]>Signed-off-by Russell King <[email protected]>
PATCH 2/10 pm: domains: factor out code to get the generic PM domain from a struct device
In reply to Russell King - ARM LinuxThe PM domain code contains two methods to get the generic PM domain for a struct device. One is dev_to_genpd() which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded into genpd_dev_pm_detach() which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).
This commit factors out the safer version and documents it.
--- drivers/base/power/domain.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)
drivers/base/power/domain.c +27 -13
@@ -68,6 +68,31 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)return genpd; } +/* +* Get the generic PM domain for a particular struct device. +* This validates the struct device pointer, the PM domain pointer, +* and checks that the PM domain pointer is a real generic PM domain. +* Any failure results in NULL being returned. +*/ +static struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev) +{ +struct generic_pm_domain *genpd =NULL , *gpd; + +if (IS_ERR_OR_NULL (dev) ||IS_ERR_OR_NULL (dev->pm_domain)) +return NULL ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) { + genpd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock); + +return genpd; +} +struct generic_pm_domain *dev_to_genpd (struct device *dev) {if (IS_ERR_OR_NULL (dev->pm_domain))
@@ -2120,21 +2145,10 @@ EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);*/ static void genpd_dev_pm_detach (struct device *dev,bool power_off) { -struct generic_pm_domain *pd =NULL , *gpd; +struct generic_pm_domain *pd;int ret =0 ; -if (!dev->pm_domain) -return ; - -mutex_lock (&gpd_list_lock); -list_for_each_entry (gpd, &gpd_list, gpd_list_node) { -if (&gpd->domain == dev->pm_domain) { - pd = gpd; -break ; - } - } -mutex_unlock (&gpd_list_lock); - + pd =pm_genpd_lookup_dev (dev);if (!pd)return ;
Signature · 1 line
1.8.3.1
PATCH 2/10 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 9 lines · Russell King
The PM domain code contains two methods to get the generic PM domain for a struct device. One is
dev_to_genpd()which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded intogenpd_dev_pm_detach()which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).This commit factors out the safer version and documents it.
Signed-off-by Russell King <[email protected]>
On top of this, I assume you're thinking that the externally available dev_to_genpd() should probably be replaced by pm_genpd_lookup_dev(), and dev_to_genpd() should only be used internally to genpd?
Kevin
PATCH 2/10 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 10 lines · Kevin Hilman
Quoted 8 lines · Russell King
The PM domain code contains two methods to get the generic PM domain for a struct device. One is
dev_to_genpd()which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded intogenpd_dev_pm_detach()which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).This commit factors out the safer version and documents it.
Quoted 7 lines
Quoted 1 line
Signed-off-by Russell King <[email protected]>Acked-by Kevin Hilman <[email protected]>On top of this, I assume you're thinking that the externally available
dev_to_genpd()should probably be replaced bypm_genpd_lookup_dev(), anddev_to_genpd()should only be used internally to genpd?
I think that would be a sensible improvement over leaving the unsafe dev_to_genpd() exposed.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 3/10 pm: domains: avoid potential oops in pm_genpd_remove_device()
In reply to Russell King - ARM Linuxpm_genpd_remove_device() tries hard to validate the generic PM domain passed to it, but the validation is not complete.
dev->pm_domain contains a struct dev_pm_domain, which is the "base class" of generic PM domains. Other users of dev_pm_domains include stuff like vga_switheroo. Hence, a device could have a generic PM domain or a vga_switcheroo PM domain in dev->pm_domain.
We need ot be certain that the PM domain is actually valid before we try to remove it. We can do this easily as we have a way to get the current validated generic PM domain for a struct device. This must match the generic PM domain being requested for removal.
Convert the code to use this alternative validation method instead.
--- drivers/base/power/domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/base/power/domain.c +1 -3
@@ -1534,9 +1534,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) - ||IS_ERR_OR_NULL (dev->pm_domain) - ||pd_to_genpd (dev->pm_domain) != genpd) +if (!genpd || genpd !=pm_genpd_lookup_dev (dev))return -EINVAL ;genpd_acquire_lock (genpd);
Signature · 1 line
1.8.3.1
PATCH 3/10 pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 10 lines · Russell King
pm_genpd_remove_device()tries hard to validate the generic PM domain passed to it, but the validation is not complete.
dev->pm_domaincontains a structdev_pm_domain, which is the "base class" of generic PM domains. Other users ofdev_pm_domainsinclude stuff likevga_switheroo. Hence, a device could have a generic PM domain or avga_switcherooPM domain indev->pm_domain.We need ot be certain that the PM domain is actually valid before we try to remove it.
nit: We need to be certain that the PM domain is a valid genpd before we try to remove the device.
Quoted 7 lines
We can do this easily as we have a way to get the current validated generic PM domain for a struct device. This must match the generic PM domain being requested for removal.
Convert the code to use this alternative validation method instead.
Signed-off-by Russell King <[email protected]>
Otherwise,
PATCH 4/10 pm: domains: sync runtime PM status with PM domains after probe
In reply to Russell King - ARM LinuxSynchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
--- drivers/amba/bus.c | 4 +++- drivers/base/platform.c | 2 ++ drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 12 ++++++++++++ drivers/i2c/i2c-core.c | 2 ++ drivers/spi/spi.c | 2 ++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 8 files changed, 41 insertions(+), 1 deletion(-)
drivers/amba/bus.c +3 -1
@@ -205,8 +205,10 @@ static int amba_probe(struct device *dev)pm_runtime_enable (dev); ret = pcdrv->probe (pcdev, id); -if (ret ==0 ) +if (ret ==0 ) { +dev_pm_domain_sync (dev);break ; + }pm_runtime_disable (dev);pm_runtime_set_suspended (dev);
drivers/base/platform.c +2 -0
@@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe (dev);if (ret)dev_pm_domain_detach (_dev, true); +else +dev_pm_domain_sync (_dev); }if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER ) {
drivers/base/power/common.c +15 -0
@@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach (dev, power_off); }EXPORT_SYMBOL_GPL (dev_pm_domain_detach); + +/** +* dev_pm_domain_sync - synchronise the PM domain state with its devices +* @dev: device corresponding with domain +* +* Synchronise the PM domain state with the recently probed device, which +* may be in a variety of PM states. This ensures that a device which +* enables runtime PM in suspended state, and never transitions to active +* in its probe handler is properly suspended after the probe. +*/ +void dev_pm_domain_sync (struct device *dev) +{ +if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync (dev); +}
drivers/base/power/domain.c +12 -0
@@ -2169,6 +2169,17 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)genpd_queue_power_off_work (pd); } +static void genpd_dev_pm_sync (struct device *dev) +{ +struct generic_pm_domain *pd; + + pd =pm_genpd_lookup_dev (dev); +if (!pd) +return ; + +genpd_queue_power_off_work (pd); +} +/** * genpd_dev_pm_attach - Attach a device to its PM domain using DT. * @dev: Device to attach.
@@ -2235,6 +2246,7 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; + dev->pm_domain->sync = genpd_dev_pm_sync;pm_genpd_poweron (pd);return 0 ;
drivers/i2c/i2c-core.c +2 -0
@@ -659,6 +659,8 @@ static int i2c_device_probe(struct device *dev) client));if (status)dev_pm_domain_detach (&client->dev, true); +else +dev_pm_domain_sync (&client->dev); }return status;
drivers/spi/spi.c +2 -0
@@ -270,6 +270,8 @@ static int spi_drv_probe(struct device *dev) ret = sdrv->probe (to_spi_device (dev));if (ret)dev_pm_domain_detach (dev, true); +else +dev_pm_domain_sync (dev); }return ret;
include/linux/pm.h +1 -0
@@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev);struct dev_pm_domain {struct dev_pm_ops ops;void (*detach)(struct device *dev,bool power_off); +void (*sync)(struct device *dev); };/*
include/linux/pm_domain.h +4 -0
@@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,#ifdef CONFIG_PM extern int dev_pm_domain_attach (struct device *dev,bool power_on);extern void dev_pm_domain_detach (struct device *dev,bool power_off); +void dev_pm_domain_sync (struct device *dev);#else static inline int dev_pm_domain_attach (struct device *dev,bool power_on) {return -ENODEV ; }static inline void dev_pm_domain_detach (struct device *dev,bool power_off) {} +static inline void dev_pm_domain_sync (struct device *dev) +{ +}#endif #endif /* _LINUX_PM_DOMAIN_H */
Signature · 1 line
1.8.3.1
PATCH 4/10 pm: domains: sync runtime PM status with PM domains after probe
Quoted 15 lines · Russell King
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in 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 in order to match the behaviour required by drivers that make no use of runtime PM. 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, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe.
Quoted 5 lines
In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe.
Signed-off-by Russell King <[email protected]>
[ Since I hadn't seen this version yet, repeating comment from the
previous version so it doesn't get lost. ]
I think this is a good fix to the existing problem. One minor nit on a comment below, otherwise:
Quoted 9 lines
+/**
+ *dev_pm_domain_sync- synchronise the PM domain state with its devices
+ * @dev: device corresponding with domain
+ *
+ * Synchronise the PM domain state with the recently probed device, which
+ * may be in a variety of PM states. This ensures that a device which
+ * enables runtime PM in suspended state, and never transitions to active
+ * in its probe handler is properly suspended after the probe.
+ */
It's not the *device* tha needs to be properly suspended after the probe (since it's already/still runtime suspended), but the pm_domain that would be potentially powered down. Hence, I'd reword the last sentence slightly:
This ensures that a device which enables runtime PM in suspended state, and never transitions to active in its probe handler gives an opportunity for the PM domain to be powered down after the probe.
Kevin
PATCH 5/10 dt-bindings: add Marvell PMU documentation
In reply to Russell King - ARM LinuxAdd the required DT binding documentation for the Marvell PMU driver.
--- Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/dove/pmu.txt
Documentation/devicetree/bindings/soc/dove/pmu.txt new file +49 -0
@@ -0,0 +1,49 @@ +Device Tree bindings for Marvell PMU + +Required properties: + - compatible: value should be "marvell,dove-pmu". + - reg: two base addresses and sizes of the PM controller and PMU. + - interrupts: single interrupt number for the PMU interrupt + - interrupt-controller: must be specified as the PMU itself is an + interrupt controller. + - #interrupt-cells: must be 1. + - #reset-cells: must be 1. + +Optional properties: + - None + +Power domain descriptions are listed as child nodes of the power management +node. Each domain has the following properties: + +Required properties: + - #power-domain-cells: must be 0. + +Optional properties: + - marvell,pmu_pwr_mask: specifies the mask value for PMU power register + - marvell,pmu_iso_mask: specifies the mask value for PMU isolation register + - resets: points to the reset manager (PMU node) and reset index. + +Example: + + pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + 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>; + }; + + gpu_domain: gpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000004>; + marvell,pmu_iso_mask = <0x00000002>; + resets = <&pmu 18>; + }; + };
Signature · 1 line
1.8.3.1
PATCH 5/10 dt-bindings: add Marvell PMU documentation
Quoted 3 lines · Russell King
Add the required DT binding documentation for the Marvell PMU driver.
Signed-off-by Russell King <[email protected]>
Quoted 63 lines
--- Documentation/devicetree/bindings/soc/dove/pmu.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/dove/pmu.txtDocumentation/devicetree/bindings/soc/dove/pmu.txt new file +49
-1@@ -0,0 +1,49 @@ +Device Tree bindings for Marvell PMU + +Required properties: + - compatible: value should be "marvell,dove-pmu". + - reg: two base addresses and sizes of the PM controller and PMU. + - interrupts: single interrupt number for the PMU interrupt + - interrupt-controller: must be specified as the PMU itself is an + interrupt controller. + - #interrupt-cells: must be 1. + - #reset-cells: must be 1. + +Optional properties: + - None + +Power domain descriptions are listed as child nodes of the power management +node. Each domain has the following properties: + +Required properties: + - #power-domain-cells: must be 0. + +Optional properties: + - marvell,pmu_pwr_mask: specifies the mask value for PMU power register + - marvell,pmu_iso_mask: specifies the mask value for PMU isolation register + - resets: points to the reset manager (PMU node) and reset index. + +Example: + + pmu: power-management at d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + 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>; + }; + + gpu_domain: gpu-domain { + #power-domain-cells = <0>; + marvell,pmu_pwr_mask = <0x00000004>; + marvell,pmu_iso_mask = <0x00000002>; + resets = <&pmu 18>; + }; + }; --1.8.3.1
PATCH 6/10 ARM: dove: create a proper PMU driver for power domains, PMU IRQs and resets
In reply to Russell King - ARM LinuxThe 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.
--- arch/arm/mach-mvebu/Kconfig | 1 + arch/arm/mach-mvebu/dove.c | 2 + drivers/soc/Makefile | 1 + drivers/soc/dove/Makefile | 1 + drivers/soc/dove/pmu.c | 406 +++++++++++++++++++++++++++++++++++++++++++ include/linux/soc/dove/pmu.h | 6 + 6 files changed, 417 insertions(+) create mode 100644 drivers/soc/dove/Makefile create mode 100644 drivers/soc/dove/pmu.c create mode 100644 include/linux/soc/dove/pmu.h
arch/arm/mach-mvebu/Kconfig +1 -0
@@ -82,6 +82,7 @@ config MACH_DOVE select MACH_MVEBU_ANY select ORION_IRQCHIP select ORION_TIMER + select PM_GENERIC_DOMAINS if PM select PINCTRL_DOVE help Say 'Y' here if you want your kernel to support the
arch/arm/mach-mvebu/dove.c +2 -0
@@ -12,6 +12,7 @@#include <linux/mbus.h> #include <linux/of.h> #include <linux/of_platform.h> +#include <linux/soc/dove/pmu.h> #include <asm/hardware/cache-tauros2.h> #include <asm/mach/arch.h> #include "common.h"
@@ -24,6 +25,7 @@ static void __init dove_init(void)tauros2_init (0 );#endif BUG_ON (mvebu_mbus_dt_init (false)); +dove_init_pmu ();of_platform_populate (NULL , of_default_bus_match_table,NULL ,NULL ); }
drivers/soc/Makefile +1 -0
@@ -2,6 +2,7 @@ # Makefile for the Linux Kernel SOC specific device drivers. # +obj-$(CONFIG_MACH_DOVE) += dove/ obj-$(CONFIG_ARCH_QCOM) += qcom/ obj-$(CONFIG_ARCH_TEGRA) += tegra/ obj-$(CONFIG_SOC_TI) += ti/
drivers/soc/dove/Makefile new file +1 -0
@@ -0,0 +1 @@ +obj-y += pmu.o
drivers/soc/dove/pmu.c new file +406 -0
406 lines not on this page · raw message
include/linux/soc/dove/pmu.h new file +6 -0
@@ -0,0 +1,6 @@ +#ifndef LINUX_SOC_DOVE_PMU_H +#define LINUX_SOC_DOVE_PMU_H + +int dove_init_pmu (void ); + +#endif
Signature · 1 line
1.8.3.1
PATCH 7/10 ARM: dt: dove: add Dove PMU DT entry to dove.dtsi
In reply to Russell King - ARM LinuxAdd the PMU description to the Dove DT file. The PMU provides multiple features, including an interrupt, reset, power and isolation controller.
--- arch/arm/boot/dts/dove.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/arm/boot/dts/dove.dtsi +9 -0
@@ -380,6 +380,15 @@ status = "disabled"; }; + pmu: power-management at d0000 { + compatible = "marvell,dove-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>;
Signature · 1 line
1.8.3.1
PATCH 8/10 ARM: dt: dove: wire up RTC interrupt
In reply to Russell King - ARM LinuxNow that we have a PMU driver, we can wire up the RTC interrupt in the DT description for Dove.
--- arch/arm/boot/dts/dove.dtsi | 2 ++ 1 file changed, 2 insertions(+)
arch/arm/boot/dts/dove.dtsi +2 -0
@@ -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 {
Signature · 1 line
1.8.3.1
PATCH 9/10 ARM: dt: dove: add video decoder power domain description
In reply to Russell King - ARM LinuxAdd the description of the video decoder power domain to the PMU DT entry.
--- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/dove.dtsi +7 -0
@@ -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 {
Signature · 1 line
1.8.3.1
PATCH 10/10 ARM: dt: dove: add GPU power domain description
In reply to Russell King - ARM LinuxAdd the description of the GPU power domain to the PMU DT entry.
--- arch/arm/boot/dts/dove.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/dove.dtsi +7 -0
@@ -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 {
Signature · 1 line
1.8.3.1
cover Dove PMU support
In reply to Russell King - ARM LinuxQuoted 28 lines · Russell King - ARM Linux
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 has been re-worked to try an alternative approach, syncing the PM domain state with the runtime PM state after the probe has completed.
* 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.)
Quoted 8 lines
* 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.
I can apply patches [1-3/9] from this series if that helps.
Rafael
cover Dove PMU support
Quoted 32 lines · Rafael J. Wysocki
Quoted 31 lines · Russell King - ARM Linux
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 has been re-worked to try an alternative approach, syncing the PM domain state with the runtime PM state after the probe has completed.
* 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.
Quoted 7 lines
Quoted 5 lines
* 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.
I can apply patches [1-3/9] from this series if that helps.
I mean from the next version of it ([FOR DISCUSSION 0/10] Dove PMU support).
These patches to be precise:
https://patchwork.kernel.org/patch/6006991/
https://patchwork.kernel.org/patch/6007001/
https://patchwork.kernel.org/patch/6007021/
unless there are objectsions (which I haven't seen).
Rafael
cover Dove PMU support
Quoted 39 lines · Rafael J. Wysocki
Quoted 38 lines · Rafael J. Wysocki
Quoted 36 lines · Russell King - ARM Linux
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 has been re-worked to try an alternative approach, syncing the PM domain state with the runtime PM state after the probe has completed.
* 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.
Quoted 3 lines
Quoted 1 line
I can apply patches [1-3/9] from this series if that helps.
I mean from the next version of it ([FOR DISCUSSION 0/10] Dove PMU support).
Kevin had some comments on patch 2 which I think ought to be addressed. He's quite right that dev_to_genpd() should be hidden from view and replaced with the safer pm_genpd_lookup_dev() version - even though pm_genpd_lookup_dev() itself is not fully safe against the domain on a device changing beneath it.
If you want to hold on, I'll respin with his comments there addressed, or if you prefer to merge them as-is and fix Kevin's comments afterwards, that's also fine by me.
I'd just be happy to see some progress on this series. :)
Thanks.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
cover Dove PMU support
Quoted 50 lines · Russell King - ARM Linux
Quoted 39 lines · Rafael J. Wysocki
Quoted 38 lines · Rafael J. Wysocki
Quoted 36 lines · Russell King - ARM Linux
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 has been re-worked to try an alternative approach, syncing the PM domain state with the runtime PM state after the probe has completed.
* 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.
Quoted 3 lines
Quoted 1 line
I can apply patches [1-3/9] from this series if that helps.
I mean from the next version of it ([FOR DISCUSSION 0/10] Dove PMU support).
Kevin had some comments on patch 2 which I think ought to be addressed. He's quite right that
dev_to_genpd()should be hidden from view and replaced with the saferpm_genpd_lookup_dev()version - even thoughpm_genpd_lookup_dev()itself is not fully safe against the domain on a device changing beneath it.
Quoted 3 lines
If you want to hold on, I'll respin with his comments there addressed, or if you prefer to merge them as-is and fix Kevin's comments afterwards, that's also fine by me.
Both work for me, actually. I think that Kevin has ACKed the patch despite the comments, so it would be fine to apply it I guess.
Quoted 1 line
I'd just be happy to see some progress on this series. :)
Sure.
cover Dove PMU support
Quoted 2 lines · Rafael J. Wysocki
Both work for me, actually. I think that Kevin has ACKed the patch despite the comments, so it would be fine to apply it I guess.
Here's the set of three - with the second one reworked.
drivers/base/power/domain.c | 58 +++++++++++++++++++++++++++++---------------- include/linux/pm_domain.h | 6 ++--- 2 files changed, 40 insertions(+), 24 deletions(-)
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 1/3 pm: domains: quieten down generic pm domains
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.
--- drivers/base/power/domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/base/power/domain.c +4 -4
@@ -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; }
Signature · 1 line
1.8.3.1
PATCH 2/3 pm: domains: factor out code to get the generic PM domain from a struct device
In reply to Russell King - ARM LinuxThe PM domain code contains two methods to get the generic PM domain for a struct device. One is dev_to_genpd() which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded into genpd_dev_pm_detach() which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).
This commit factors out the safer version, documents it, and hides the unsafe dev_to_genpd().
--- drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++-------------- include/linux/pm_domain.h | 6 +++--- 2 files changed, 35 insertions(+), 17 deletions(-)
drivers/base/power/domain.c +32 -14
@@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)return genpd; } -struct generic_pm_domain *dev_to_genpd (struct device *dev) +/* +* Get the generic PM domain for a particular struct device. +* This validates the struct device pointer, the PM domain pointer, +* and checks that the PM domain pointer is a real generic PM domain. +* Any failure results in NULL being returned. +*/ +struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev) +{ +struct generic_pm_domain *genpd =NULL , *gpd; + +if (IS_ERR_OR_NULL (dev) ||IS_ERR_OR_NULL (dev->pm_domain)) +return NULL ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) { + genpd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock); + +return genpd; +} + +/* +* This should only be used where we are certain that the pm_domain +* attached to the device is a genpd domain. +*/ +static struct generic_pm_domain *dev_to_genpd (struct device *dev) {if (IS_ERR_OR_NULL (dev->pm_domain))return ERR_PTR (-EINVAL );
@@ -2120,21 +2149,10 @@ EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);*/ static void genpd_dev_pm_detach (struct device *dev,bool power_off) { -struct generic_pm_domain *pd =NULL , *gpd; +struct generic_pm_domain *pd;int ret =0 ; -if (!dev->pm_domain) -return ; - -mutex_lock (&gpd_list_lock); -list_for_each_entry (gpd, &gpd_list, gpd_list_node) { -if (&gpd->domain == dev->pm_domain) { - pd = gpd; -break ; - } - } -mutex_unlock (&gpd_list_lock); - + pd =pm_genpd_lookup_dev (dev);if (!pd)return ;
include/linux/pm_domain.h +3 -3
@@ -129,7 +129,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)return to_gpd_data (dev->power.subsys_data->domain_data); } -extern struct generic_pm_domain *dev_to_genpd (struct device *dev); +extern struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev);extern int __pm_genpd_add_device (struct generic_pm_domain *genpd,struct device *dev,struct gpd_timing_data *td);
@@ -166,9 +166,9 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev) {return ERR_PTR (-ENOSYS ); } -static inline struct generic_pm_domain *dev_to_genpd (struct device *dev) +static inline struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev) { -return ERR_PTR (-ENOSYS ); +return NULL ; } static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
Signature · 1 line
1.8.3.1
PATCH 2/3 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 21 lines · Russell King
The PM domain code contains two methods to get the generic PM domain for a struct device. One is
dev_to_genpd()which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded intogenpd_dev_pm_detach()which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).This commit factors out the safer version, documents it, and hides the unsafe
dev_to_genpd().Signed-off-by Russell King <[email protected]>--- drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++-------------- include/linux/pm_domain.h | 6 +++--- 2 files changed, 35 insertions(+), 17 deletions(-)drivers/base/power/domain.c +0
-0@@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)return genpd;
drivers/base/power/domain.c static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name) · line 77
Quoted 10 lines
drivers/base/power/domain.c +7
-1} -struct generic_pm_domain *dev_to_genpd (struct device *dev) +/* +* Get the generic PM domain for a particular struct device. +* This validates the struct device pointer, the PM domain pointer, +* and checks that the PM domain pointer is a real generic PM domain. +* Any failure results in NULL being returned. +*/ +struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev)
I wouldn't mind keeping also this function static, unless you foresee users outside genpd?
If converting to static please rename it to genpd_lookup_dev().
Quoted 76 lines
drivers/base/power/domain.c +25
-13+{ +struct generic_pm_domain *genpd =NULL , *gpd; + +if (IS_ERR_OR_NULL (dev) ||IS_ERR_OR_NULL (dev->pm_domain)) +return NULL ; + +mutex_lock (&gpd_list_lock); +list_for_each_entry (gpd, &gpd_list, gpd_list_node) { +if (&gpd->domain == dev->pm_domain) { + genpd = gpd; +break ; + } + } +mutex_unlock (&gpd_list_lock); + +return genpd; +} + +/* +* This should only be used where we are certain that the pm_domain +* attached to the device is a genpd domain. +*/ +static struct generic_pm_domain *dev_to_genpd (struct device *dev) {if (IS_ERR_OR_NULL (dev->pm_domain))return ERR_PTR (-EINVAL );@@ -2120,21 +2149,10 @@ EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);*/ static void genpd_dev_pm_detach (struct device *dev,bool power_off) { -struct generic_pm_domain *pd =NULL , *gpd; +struct generic_pm_domain *pd;int ret =0 ; -if (!dev->pm_domain) -return ; - -mutex_lock (&gpd_list_lock); -list_for_each_entry (gpd, &gpd_list, gpd_list_node) { -if (&gpd->domain == dev->pm_domain) { - pd = gpd; -break ; - } - } -mutex_unlock (&gpd_list_lock); - + pd =pm_genpd_lookup_dev (dev);if (!pd)return ;include/linux/pm_domain.h +3
-4@@ -129,7 +129,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)return to_gpd_data (dev->power.subsys_data->domain_data); } -extern struct generic_pm_domain *dev_to_genpd (struct device *dev); +extern struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev);extern int __pm_genpd_add_device (struct generic_pm_domain *genpd,struct device *dev,struct gpd_timing_data *td);@@ -166,9 +166,9 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev) {return ERR_PTR (-ENOSYS ); } -static inline struct generic_pm_domain *dev_to_genpd (struct device *dev) +static inline struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev) { -return ERR_PTR (-ENOSYS ); +return NULL ; } static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, --
Quoted 2 lines
1.8.3.1
Kind regards
Uffe
PATCH 2/3 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 24 lines · Ulf Hansson
Quoted 23 lines · Russell King
The PM domain code contains two methods to get the generic PM domain for a struct device. One is
dev_to_genpd()which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded intogenpd_dev_pm_detach()which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).This commit factors out the safer version, documents it, and hides the unsafe
dev_to_genpd().Signed-off-by Russell King <[email protected]>--- drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++-------------- include/linux/pm_domain.h | 6 +++--- 2 files changed, 35 insertions(+), 17 deletions(-)drivers/base/power/domain.c +0
-0@@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)return genpd; }
Quoted 11 lines
drivers/base/power/domain.c static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name) · line 77
Quoted 8 lines
drivers/base/power/domain.c +7
-1-struct generic_pm_domain *dev_to_genpd (struct device *dev) +/* +* Get the generic PM domain for a particular struct device. +* This validates the struct device pointer, the PM domain pointer, +* and checks that the PM domain pointer is a real generic PM domain. +* Any failure results in NULL being returned. +*/ +struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev)I wouldn't mind keeping also this function static, unless you foresee users outside genpd?
_I_ have users of it outside at the moment.
Once the major churn in PM domains is over and I have no use for this externally, I'll consider making it static in my patch set - but all the time that doing so results in breakage for me...
Plus, you're asking me to do yet another 20+ minute re-spin of this patch set. I'm going to refuse; I'm almost at the point of just not giving a damn on this stuff, it's wasting too much of my time. Especially as, yet again, you should have replied to the emails in the previous round suggesting this change.
Rafael, please merge these three as we previously agreed. Thanks.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 2/3 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 43 lines · Russell King - ARM Linux
Quoted 24 lines · Ulf Hansson
Quoted 23 lines · Russell King
The PM domain code contains two methods to get the generic PM domain for a struct device. One is
dev_to_genpd()which is only safe when we know for certain that the device has a generic PM domain attached. The other is coded intogenpd_dev_pm_detach()which ensures that the PM domain in the struct device is a generic PM domain (and so is safer).This commit factors out the safer version, documents it, and hides the unsafe
dev_to_genpd().Signed-off-by Russell King <[email protected]>--- drivers/base/power/domain.c | 46 +++++++++++++++++++++++++++++++-------------- include/linux/pm_domain.h | 6 +++--- 2 files changed, 35 insertions(+), 17 deletions(-)drivers/base/power/domain.c +0
-0@@ -68,7 +68,36 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)return genpd; }Quoted 11 lines
drivers/base/power/domain.c static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name) · line 77
Quoted 8 lines
drivers/base/power/domain.c +7
-1-struct generic_pm_domain *dev_to_genpd (struct device *dev) +/* +* Get the generic PM domain for a particular struct device. +* This validates the struct device pointer, the PM domain pointer, +* and checks that the PM domain pointer is a real generic PM domain. +* Any failure results in NULL being returned. +*/ +struct generic_pm_domain *pm_genpd_lookup_dev (struct device *dev)I wouldn't mind keeping also this function static, unless you foresee users outside genpd?
_I_ have users of it outside at the moment.
Once the major churn in PM domains is over and I have no use for this externally, I'll consider making it static in my patch set - but all the time that doing so results in breakage for me...
Quoted 7 lines
Plus, you're asking me to do yet another 20+ minute re-spin of this patch set. I'm going to refuse; I'm almost at the point of just not giving a damn on this stuff, it's wasting too much of my time. Especially as, yet again, you should have replied to the emails in the previous round suggesting this change.
Rafael, please merge these three as we previously agreed. Thanks.
I said I would do that, didn't I?
I've applied this series plus https://patchwork.kernel.org/patch/6056201/
and https://patchwork.kernel.org/patch/6057441/ on top of that.
Please let me know if you need anything more from the core.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
PATCH 2/3 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 4 lines · Rafael J. Wysocki
Quoted 1 line · Russell King - ARM Linux
Rafael, please merge these three as we previously agreed. Thanks.
I said I would do that, didn't I?
I wasn't sure if you were going to hold back on it as a result of Uwe's comments.
Quoted 3 lines
I've applied this series plus https://patchwork.kernel.org/patch/6056201/
and https://patchwork.kernel.org/patch/6057441/ on top of that. Please let me know if you need anything more from the core.
I'll replicate what you've applied and test it here (build in progress...)
Thanks!
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 2/3 pm: domains: factor out code to get the generic PM domain from a struct device
Quoted 1 line · Russell King - ARM Linux
I'll replicate what you've applied and test it here (build in progress...)
It looks like it works as desired.
Thanks.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 3/3 pm: domains: avoid potential oops in pm_genpd_remove_device()
In reply to Russell King - ARM Linuxpm_genpd_remove_device() tries hard to validate the generic PM domain passed to it, but the validation is not complete.
dev->pm_domain contains a struct dev_pm_domain, which is the "base class" of generic PM domains. Other users of dev_pm_domains include stuff like vga_switheroo. Hence, a device could have a generic PM domain or a vga_switcheroo PM domain in dev->pm_domain.
We need ot be certain that the PM domain is actually valid before we try to remove it. We can do this easily as we have a way to get the current validated generic PM domain for a struct device. This must match the generic PM domain being requested for removal.
Convert the code to use this alternative validation method instead.
--- drivers/base/power/domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/base/power/domain.c +1 -3
@@ -1538,9 +1538,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) - ||IS_ERR_OR_NULL (dev->pm_domain) - ||pd_to_genpd (dev->pm_domain) != genpd) +if (!genpd || genpd !=pm_genpd_lookup_dev (dev))return -EINVAL ;genpd_acquire_lock (genpd);
Signature · 1 line
1.8.3.1
PATCH 3/3 pm: domains: avoid potential oops in pm_genpd_remove_device()
Quoted 13 lines · Russell King
pm_genpd_remove_device()tries hard to validate the generic PM domain passed to it, but the validation is not complete.
dev->pm_domaincontains a structdev_pm_domain, which is the "base class" of generic PM domains. Other users ofdev_pm_domainsinclude stuff likevga_switheroo. Hence, a device could have a generic PM domain or avga_switcherooPM domain indev->pm_domain.We need ot be certain that the PM domain is actually valid before we try to remove it. We can do this easily as we have a way to get the current validated generic PM domain for a struct device. This must match the generic PM domain being requested for removal.
Quoted 4 lines
Convert the code to use this alternative validation method instead.
Acked-by Kevin Hilman <[email protected]>Signed-off-by Russell King <[email protected]>
Quoted 22 lines
--- drivers/base/power/domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/base/power/domain.c +1
-4@@ -1538,9 +1538,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) - ||IS_ERR_OR_NULL (dev->pm_domain) - ||pd_to_genpd (dev->pm_domain) != genpd) +if (!genpd || genpd !=pm_genpd_lookup_dev (dev))return -EINVAL ;genpd_acquire_lock (genpd); --1.8.3.1