| Message ID | 1435374156-19214-10-git-send-email-lina.iyer@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote: >Hi Lina, Kevin, > >On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote: >> Platform drivers may have additional setup inorder before the domain can >> be powered off. Allow, platform drivers to register power on/off > >Bogus comma. > Will fix. >> callbacks against a domain provider. >> >> While registering the callback ensure that the domain is neither in >> power on/off state. The domain should be active. To ensure that the >> platform callback registration doesntrace with genpd power on/off, > >doesn't race > Argh, thanks. >> execute the registration from a CPU on that domain. >> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > >> --- /dev/null >> +++ b/arch/arm/include/asm/pm_domain.h >> @@ -0,0 +1,27 @@ >> +/* >> + * arch/arm/include/asm/pm_domain.h >> + * >> + * Copyright (C) 2015 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#ifndef __ASM_ARM_PM_DOMAIN_H >> +#define __ASM_ARM_PM_DOMAIN_H >> + >> +#include <linux/pm_domain.h> >> + >> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS) >> +extern int register_platform_domain_handlers(struct of_phandle_args *args, >> + int (*pd_down)(struct generic_pm_domain *), >> + int (*pd_up)(struct generic_pm_domain *)); > >This looks a bit convoluted to me... > >> --- a/arch/arm/kernel/domains.c >> +++ b/arch/arm/kernel/domains.c >> @@ -9,10 +9,19 @@ >> #include <linux/module.h> >> #include <linux/platform_device.h> >> >> +#include <asm/pm_domain.h> >> + >> #define NAME_MAX 16 >> >> +struct platform_cb { >> + int (*power_off)(struct generic_pm_domain *); >> + int (*power_on)(struct generic_pm_domain *); >> +}; >> + >> struct arm_pm_domain { >> struct generic_pm_domain genpd; >> + struct platform_cb plat_handler; >> + struct spinlock_t lock; >> }; >> >> static inline >> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d) >> >> static int arm_pd_power_down(struct generic_pm_domain *genpd) >> { >> + struct arm_pm_domain *arm_pd = to_arm_pd(genpd); >> + >> + if (arm_pd->plat_handler.power_off) >> + return arm_pd->plat_handler.power_off(genpd); >> + >> /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */ >> return 0; >> } >> >> static int arm_pd_power_up(struct generic_pm_domain *genpd) >> { >> + struct arm_pm_domain *arm_pd = to_arm_pd(genpd); >> + >> + if (arm_pd->plat_handler.power_on) >> + return arm_pd->plat_handler.power_on(genpd); >> + >> /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */ >> return 0; >> } > >> @@ -152,6 +230,7 @@ static int arm_domain_init(void) >> pd->genpd.power_off = arm_pd_power_down; >> pd->genpd.power_on = arm_pd_power_up; > >Shouldn't these .power_off() and .power_on() be set up from platform code >instead, in the platform-specific code that creates the PM domain? > >The PM Domain containing the CPU may contain other devices, in which >case it's already set up from platform-specific code, which would conflict >with arm_domain_init()? > In my first RFC, the platform code was creating a domain. In this RFC, we are flaunting an idea that a generic code could setup the domains without any intervention from platform code. It will do everything common on most ARM platforms. Architectures that do not have anything specific in their domain, would benefit from such an initialization. If we were to export this genpd and then platform code could attach more devices to the genpd, or make it a sub of another genpd, would that work? >Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains >(both for devices and CPUs) on R-Mobile, and >arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi. > >R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters, >with separate PM Domains for L2/SCU and sub-domains for the CPUs. If you could get the CPU genpd from the ARM common code, you could embed that in the L2 domain. On QCOM's big.LITTLE SoC, there would a PM domain for each of the CPUs and then there would be a coherency PM domain that would contain the big and LITTLE cluster domains. In that case, the platform code would create and initialize the coherency domain and make the CPU domains a sub of the coherency domain. Would something like that work? >Unfortunately we don't have SMP support for it, so currently dtsi describes >the first cpu core only. The full structure should look like this > > cpus { > cpu0: cpu@0 { > compatible = "arm,cortex-a15"; > power-domains = <&pd_a2sl>; > next-level-cache = <&L2_CA15>; > }; > > cpu1: cpu@1 { > compatible = "arm,cortex-a15"; > power-domains = <&pd_a2sl>; > next-level-cache = <&L2_CA15>; > }; > > cpu2: cpu@2 { > compatible = "arm,cortex-a15"; > power-domains = <&pd_a2sl>; > next-level-cache = <&L2_CA15>; > }; > > cpu3: cpu@3 { > compatible = "arm,cortex-a15"; > power-domains = <&pd_a2sl>; > next-level-cache = <&L2_CA15>; > }; > > cpu4: cpu@4 { > compatible = "arm,cortex-a7"; > power-domains = <&pd_a2kl>; > next-level-cache = <&L2_CA7>; > }; > > cpu5: cpu@5 { > compatible = "arm,cortex-a7"; > power-domains = <&pd_a2kl>; > next-level-cache = <&L2_CA7>; > }; > > cpu6: cpu@6 { > compatible = "arm,cortex-a7"; > power-domains = <&pd_a2kl>; > next-level-cache = <&L2_CA7>; > }; > > cpu7: cpu@7 { > compatible = "arm,cortex-a7"; > power-domains = <&pd_a2kl>; > next-level-cache = <&L2_CA7>; > }; > }; > > L2_CA15: cache-controller@0 { > compatible = "cache"; > power-domains = <&pd_a3sm>; > }; > > L2_CA7: cache-controller@1 { > compatible = "cache"; > power-domains = <&pd_a3km>; > }; > >And the PM Domain part (which is complete in upstream): > > pd_c4: c4@0 { > #power-domain-cells = <0>; > > pd_a3sm: a3sm@20 { > reg = <20>; > #power-domain-cells = <0>; > > pd_a2sl: a2sl@21 { > reg = <21>; > #power-domain-cells = <0>; > }; > }; > > pd_a3km: a3km@22 { > reg = <22>; > #size-cells = <0>; > #power-domain-cells = <0>; > > pd_a2kl: a2kl@23 { > reg = <23>; > #power-domain-cells = <0>; > }; > }; > }; > Thanks for the example. Would it work, if the platform code initalizes the pd_a3sm, pd_a3km and pd_c4 and set up the hierarchy and to add the CPU domains, you could query the ARM and then add the domains to the pd_a3sm and pd_a3km? Thanks for looking through the patch. -- Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote: >Hi Lina, > >On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote: >Perhaps the generic one can be optional, and provide helpers for common >CPU operations? Then the platform-specific driver can handle all PM domains, >and delegate to the generic CPU helpers where appropriate. > >Does that make sense? > Well it does. Thanks Geert. My RFC v1 [1] did exacly that. But, it didnt fit into the big picture of things well. Here is where we wanted to head towards, in the long run. Today, we have CPU_PM for CPU runtime and we have runtime PM for others, we want to unify and move to a generic runtime PM for the CPUs as well. To that effort, we want to bring in generic code all into the fold of CPU runtime PM and CPU domain runtime PM. A generic CPU PM domain with its own genpd callback for ->power_on() and ->power_off() would help handle the common power on/off stuff there and possibly call into GIC and others that currently use CPU_PM from there. So the common node needs to be the handle of power on/off callbacks from the genpd, when all the CPUs are entering idle or resuming. With what you have suggested, the platform driver creates the genpd and would pass the CPU genpd to the common code for common operations. (This was what was done in [1]). The platform driver would set the power_on() and power_off() callbacks and that would have to be overriden in order handle common CPU domain suspend/resume activities. Overwriting members of an object allocated by the platform driver, is something we should avoid. Or instead of allocating the memory in your platform driver for the CPU genpd, you could query and get the genpd and then add your additions on top. You could add other flags like GENPD_FLAG_PM_CLK, but *not* overwrite the power on/off callbacks in the genpd. They still have to be registered separately like in this patch. Again, not every elegant, IMO. Another option, that might be cleaner, is that we could have a PM domain for CPUs that would set up the compatibility flag to "arm,pd" and you could nest that domain inside pd_a2sl and pd_a2kl. pd_c4: c4@0 { [...] pd_a3sm: a3sm@20 { [...] pd_a2sl: a2sl@21 { reg = <21>; #power-domain-cells = <0>; pd_cpu_sl: pd1 { <-- Virtual PM domain #power-domain-cells = <0>; }; }; }; }; cpus { cpu0: cpu@0 { compatible = "arm,cortex-a15"; power-domains = <&pd_cpu_sl>; <-- here we refer to the virtual PM domain next-level-cache = <&L2_CA15>; }; [...] }; This the common code would get its own callbacks and when that genpd powers off, the platform genpd would power down. But no new code is needed in your platform driver. The benefit is that platform code and generic CPU domain code may exist and act in parallel and may only be related if specified in the DT and the problem with that approach is that this virtual PM domain is not a h/w domain, hence specifying in DT is questionable. What do you think? Thanks, Lina [1]. http://www.spinics.net/lists/arm-kernel/msg423430.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote: >Hi Lina, > >On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >> On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote: >>> On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >>>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote: [...] >> With what you have suggested, the platform driver creates the genpd and >> would pass the CPU genpd to the common code for common operations. (This >> was what was done in [1]). The platform driver would set the power_on() >> and power_off() callbacks and that would have to be overriden in order >> handle common CPU domain suspend/resume activities. Overwriting members >> of an object allocated by the platform driver, is something we should >> avoid. > >Instead of letting the generic code override the .power_{on,off}() callbacks, >the platform code could call the generic CPU-related methods from its own >.power_{on,off}() callbacks? > We dont want to impose on platform drivers to call back into the CPU domain core code to do common domain power down activities. On a generic ARM CPU domain, we may not even need a platform specific domain callbacks. IMHO, this is not desirable. >struct rmobile_pm_domain already has .suspend() and .resume() methods. >The former is used to e.g. prevent the PM domains containing CPUs to be >powered down (in the absence of cpuidle integration). That requires scanning >the DT for CPUs, and it would indeed be good to have that scanning support >in generic code. >The latter became unused after the removal of sh7372 support, which did have >some cpuidle integration. > The CPUs may still be attached to domain, even if they are not powered off by CPUIdle. The code increases the reference count for every online CPU and any CPUs that may come online. The reference count, only goes down when the CPU powers down through CPUIdle or hotplug. So, if a CPU is not integrated with CPUIdle, it would remain in use (unless powered off by hotplug) and the domain would not be powered down. >> Or instead of allocating the memory in your platform driver for the CPU >> genpd, you could query and get the genpd and then add your additions on >> top. You could add other flags like GENPD_FLAG_PM_CLK, but *not* >> overwrite the power on/off callbacks in the genpd. They still have to be >> registered separately like in this patch. Again, not every elegant, IMO. > >Just wondering, can I set up the .attach_dev() and .detach_dev()? > I am presuming, you are concerned with the fore mentioned case of CPUs not participating in CPUIdle. With pm_runtime_put_sync() not happening for CPUs that do not power down, you should not have to worry about the domain being powered down. Please correct me if I misunderstood your point. >Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK >or .attach_dev() and .detach_dev() for CPU PM domains, as they are only >needed for devices with MSTP clocks. CPU and L2 cache don't have these, >and there are no other devices in e.g. a3sm and a2sl. > >The GIC has an MSTP clock, but it's part of a different power domain. > Okay. [...] >> This the common code would get its own callbacks and when that genpd >> powers off, the platform genpd would power down. But no new code is >> needed in your platform driver. The benefit is that platform code and >> generic CPU domain code may exist and act in parallel and may only be >> related if specified in the DT and the problem with that approach is >> that this virtual PM domain is not a h/w domain, hence specifying in DT >> is questionable. > >Indeed, I don't like this option, as the DT would no longer describe HW, >but the Linux implementation. > >So let's continue with your approach, and see how it turns out. We can >always change and improvide code, while changing DT is more complicated. > Agreed. Thanks for your time, Geert. -- Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 06 2015 at 09:18 -0600, Lina Iyer wrote: >On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote: >>Hi Lina, >> >>On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >>>On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote: >>>>On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >>>>>On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote: > >[...] > >>>With what you have suggested, the platform driver creates the genpd and >>>would pass the CPU genpd to the common code for common operations. (This >>>was what was done in [1]). The platform driver would set the power_on() >>>and power_off() callbacks and that would have to be overriden in order >>>handle common CPU domain suspend/resume activities. Overwriting members >>>of an object allocated by the platform driver, is something we should >>>avoid. >> >>Instead of letting the generic code override the .power_{on,off}() callbacks, >>the platform code could call the generic CPU-related methods from its own >>.power_{on,off}() callbacks? >> >We dont want to impose on platform drivers to call back into the CPU >domain core code to do common domain power down activities. On a generic >ARM CPU domain, we may not even need a platform specific domain >callbacks. IMHO, this is not desirable. > Any thoughts on this? >>struct rmobile_pm_domain already has .suspend() and .resume() methods. >>The former is used to e.g. prevent the PM domains containing CPUs to be >>powered down (in the absence of cpuidle integration). That requires scanning >>the DT for CPUs, and it would indeed be good to have that scanning support >>in generic code. >>The latter became unused after the removal of sh7372 support, which did have >>some cpuidle integration. >> >The CPUs may still be attached to domain, even if they are not powered >off by CPUIdle. The code increases the reference count for every online >CPU and any CPUs that may come online. The reference count, only goes >down when the CPU powers down through CPUIdle or hotplug. So, if a CPU >is not integrated with CPUIdle, it would remain in use (unless powered >off by hotplug) and the domain would not be powered down. > >>>Or instead of allocating the memory in your platform driver for the CPU >>>genpd, you could query and get the genpd and then add your additions on >>>top. You could add other flags like GENPD_FLAG_PM_CLK, but *not* >>>overwrite the power on/off callbacks in the genpd. They still have to be >>>registered separately like in this patch. Again, not every elegant, IMO. >> >>Just wondering, can I set up the .attach_dev() and .detach_dev()? >> >I am presuming, you are concerned with the fore mentioned case of CPUs >not participating in CPUIdle. With pm_runtime_put_sync() not happening >for CPUs that do not power down, you should not have to worry about the >domain being powered down. > >Please correct me if I misunderstood your point. > >>Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK >>or .attach_dev() and .detach_dev() for CPU PM domains, as they are only >>needed for devices with MSTP clocks. CPU and L2 cache don't have these, >>and there are no other devices in e.g. a3sm and a2sl. >> >>The GIC has an MSTP clock, but it's part of a different power domain. >> >Okay. > >[...] > >>>This the common code would get its own callbacks and when that genpd >>>powers off, the platform genpd would power down. But no new code is >>>needed in your platform driver. The benefit is that platform code and >>>generic CPU domain code may exist and act in parallel and may only be >>>related if specified in the DT and the problem with that approach is >>>that this virtual PM domain is not a h/w domain, hence specifying in DT >>>is questionable. >> >>Indeed, I don't like this option, as the DT would no longer describe HW, >>but the Linux implementation. >> >>So let's continue with your approach, and see how it turns out. We can >>always change and improvide code, while changing DT is more complicated. >> >Agreed. > >Thanks for your time, Geert. > >-- Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/include/asm/cpu.h b/arch/arm/include/asm/cpu.h index 2744f06..4a7e346 100644 --- a/arch/arm/include/asm/cpu.h +++ b/arch/arm/include/asm/cpu.h @@ -22,5 +22,4 @@ struct cpuinfo_arm { }; DECLARE_PER_CPU(struct cpuinfo_arm, cpu_data); - #endif diff --git a/arch/arm/include/asm/pm_domain.h b/arch/arm/include/asm/pm_domain.h new file mode 100644 index 0000000..d13c291 --- /dev/null +++ b/arch/arm/include/asm/pm_domain.h @@ -0,0 +1,27 @@ +/* + * arch/arm/include/asm/pm_domain.h + * + * Copyright (C) 2015 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#ifndef __ASM_ARM_PM_DOMAIN_H +#define __ASM_ARM_PM_DOMAIN_H + +#include <linux/pm_domain.h> + +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS) +extern int register_platform_domain_handlers(struct of_phandle_args *args, + int (*pd_down)(struct generic_pm_domain *), + int (*pd_up)(struct generic_pm_domain *)); +#else +static inline +int register_platform_domain_handlers(struct of_phandle_args *args, + int (*pd_down)(struct generic_pm_domain *), + int (*pd_up)(struct generic_pm_domain *)) +{ return -ENODEV; } +#endif + +#endif diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c index 680c3fb..2f5ba3f 100644 --- a/arch/arm/kernel/domains.c +++ b/arch/arm/kernel/domains.c @@ -9,10 +9,19 @@ #include <linux/module.h> #include <linux/platform_device.h> +#include <asm/pm_domain.h> + #define NAME_MAX 16 +struct platform_cb { + int (*power_off)(struct generic_pm_domain *); + int (*power_on)(struct generic_pm_domain *); +}; + struct arm_pm_domain { struct generic_pm_domain genpd; + struct platform_cb plat_handler; + struct spinlock_t lock; }; static inline @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d) static int arm_pd_power_down(struct generic_pm_domain *genpd) { + struct arm_pm_domain *arm_pd = to_arm_pd(genpd); + + if (arm_pd->plat_handler.power_off) + return arm_pd->plat_handler.power_off(genpd); + /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */ return 0; } static int arm_pd_power_up(struct generic_pm_domain *genpd) { + struct arm_pm_domain *arm_pd = to_arm_pd(genpd); + + if (arm_pd->plat_handler.power_on) + return arm_pd->plat_handler.power_on(genpd); + /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */ return 0; } +static void __register_platform_domain_handlers(void *data) +{ + struct device *cpu_dev = get_cpu_device(smp_processor_id()); + struct generic_pm_domain *genpd; + struct arm_pm_domain *arm_pd; + unsigned long flags; + struct platform_cb *handler = data; + + genpd = pd_to_genpd(cpu_dev->pm_domain); + BUG_ON(IS_ERR_OR_NULL(genpd)); + arm_pd = to_arm_pd(genpd); + + /* + * Lock to avoid race with other CPUs in the same domain + * trying to set the function pointers. + */ + spin_lock_irqsave(&arm_pd->lock, flags); + arm_pd->plat_handler.power_on = handler->power_on; + arm_pd->plat_handler.power_off = handler->power_off; + spin_unlock_irqrestore(&arm_pd->lock, flags); +} + +int register_platform_domain_handlers(struct of_phandle_args *args, + int (*power_on)(struct generic_pm_domain *), + int (*power_off)(struct generic_pm_domain *)) +{ + struct platform_cb handler; + struct generic_pm_domain *genpd, *p; + struct device *cpu_dev; + int cpu; + + handler.power_on = power_on; + handler.power_off = power_off; + + genpd = of_genpd_get_from_provider(args); + get_online_cpus(); + for_each_online_cpu(cpu) { + cpu_dev = get_cpu_device(cpu); + p = pm_genpd_lookup_dev(cpu_dev); + /* + * Execute the registration on the requested 'cpu', so we dont + * race with the domain->power_off or ->power_on calls. + * By executing on that 'cpu', we are assured that the 'cpu' is + * awake, therefore the domain. This will avoid a spin lock + * between power on/off calls and this function. + */ + if (p == genpd) { + smp_call_function_single(cpu, + __register_platform_domain_handlers, + &handler, true); + break; + } + } + put_online_cpus(); + + return 0; +} +EXPORT_SYMBOL(register_platform_domain_handlers); + static void run_cpu(void *unused) { struct device *cpu_dev = get_cpu_device(smp_processor_id()); @@ -91,7 +169,7 @@ static int arm_domain_cpu_init(void) /* FIXME: this is open-coding of_cpu_device_node_get(), but I want handle to cpu_dev */ cpu_dev = get_cpu_device(cpuid); if (!cpu_dev) { - pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid); + pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid); return -ENODEV; } @@ -152,6 +230,7 @@ static int arm_domain_init(void) pd->genpd.power_off = arm_pd_power_down; pd->genpd.power_on = arm_pd_power_up; pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE; + spin_lock_init(&pd->lock); platform_set_drvdata(pdev, pd); dev_dbg(dev, "adding as generic power domain.\n");
Platform drivers may have additional setup inorder before the domain can be powered off. Allow, platform drivers to register power on/off callbacks against a domain provider. While registering the callback ensure that the domain is neither in power on/off state. The domain should be active. To ensure that the platform callback registration doesntrace with genpd power on/off, execute the registration from a CPU on that domain. Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- arch/arm/include/asm/cpu.h | 1 - arch/arm/include/asm/pm_domain.h | 27 ++++++++++++++ arch/arm/kernel/domains.c | 81 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 arch/arm/include/asm/pm_domain.h