Messages in this thread | | | Subject | Re: [RFC v2 3/6] PM / Domains: introduce power-states consistent with c-states. | From | Marc Titinger <> | Date | Fri, 9 Oct 2015 12:04:26 +0200 |
| |
On 08/10/2015 18:27, Lina Iyer wrote: > On Tue, Oct 06 2015 at 08:27 -0600, Marc Titinger wrote: >> This patch allows cluster-level C-states to being soaked in as generic >> domain power states, in order for the domain governor to chose the most >> efficient power state compatible with the device constraints. Similarly, >> devices can register power-states into the cluster domain, in a manner >> consistent with c-states. >> > A domain power state as depicted in the DT is no different than the CPU > idle state. I think you can achieve this without adding another > compatible - arm,power-state. > yes, thats'a bit hacky, I was facing the genpd_lock'ing issue I believe you mentioned recently and lead to your simplification work in the case of CPUs. the power-state discrimination was to allow cpu-idle to call runtime_put/get only for those states where genpd is to call the platform code and select the optimal c-state. Eventually this discrimination would be useless. Alternatively I've been wondering if we could have one domain (and one lock) per CPU, and have a parent domain for the cluster.
> I think I am still at loss trying to understand why a device is > populating the domain's power states. > >> With Juno, in this example the c-state 'cluster-sleep-0 ' is known from >> each cluster generic domain, as the deepest sate. >> >> cat /sys/kernel/debug/pm_genpd/* >> >> Domain State name Enter (ns) / Exit (ns) >> ------------------------------------------------------------- >> a53_pd cluster-sleep-0 1500000 / 800000 >> a57_pd cluster-sleep-0 1500000 / 800000 >> >> domain status pstate slaves >> /device runtime status >> ----------------------------------------------------------------------- >> a53_pd on >> /devices/system/cpu/cpu0 active >> /devices/system/cpu/cpu3 suspended >> /devices/system/cpu/cpu4 suspended >> /devices/system/cpu/cpu5 suspended >> /devices/platform/D1 suspended >> a57_pd cluster-sleep-0 >> /devices/system/cpu/cpu1 suspended >> /devices/system/cpu/cpu2 suspended >> >> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> >> --- >> .../devicetree/bindings/arm/idle-states.txt | 21 ++++- >> .../devicetree/bindings/power/power_domain.txt | 29 ++++++ >> arch/arm64/boot/dts/arm/juno.dts | 10 ++- >> drivers/base/power/cpu-pd.c | 5 ++ >> drivers/base/power/domain.c | 100 >> +++++++++++++++++++++ >> include/linux/pm_domain.h | 3 + >> 6 files changed, 163 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt >> b/Documentation/devicetree/bindings/arm/idle-states.txt >> index a8274ea..18fdeaf 100644 >> --- a/Documentation/devicetree/bindings/arm/idle-states.txt >> +++ b/Documentation/devicetree/bindings/arm/idle-states.txt >> @@ -270,7 +270,8 @@ follows: >> - compatible >> Usage: Required >> Value type: <stringlist> >> - Definition: Must be "arm,idle-state". >> + Definition: Must be "arm,idle-state", >> + or "arm,power-state" (see section 5) >> >> - local-timer-stop >> Usage: See definition >> @@ -680,7 +681,23 @@ cpus { >> }; >> >> =========================================== >> -5 - References >> +5 - power state >> +=========================================== >> + >> +Device in a generic power domain may expose an intermediate retention >> +state that can be opted to by the domain governor when the last-man >> +CPU is powered off. Those power-states will not be entered by the >> +cpuidle.ops based on a state index, but instead can be elected by the >> +domain governor and entered to by the generic domain. >> + > Agreed. > >> + - compatible >> + Usage: Required >> + Value type: <stringlist> >> + Definition: Must be "arm,power-state". >> + >> + >> +=========================================== >> +6 - References >> =========================================== >> >> [1] ARM Linux Kernel documentation - CPUs bindings >> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt >> b/Documentation/devicetree/bindings/power/power_domain.txt >> index 0f8ed37..d437385 100644 >> --- a/Documentation/devicetree/bindings/power/power_domain.txt >> +++ b/Documentation/devicetree/bindings/power/power_domain.txt >> @@ -29,6 +29,16 @@ Optional properties: >> specified by this binding. More details about power domain >> specifier are >> available in the next section. >> >> + - power-states : a phandle of an idle-state that shall be soaked into a >> + generic domain power state. >> + CPU domains: Deep c-states that match a cluster power-off can be >> delegated to the >> + generic power domain. Device other than CPUs may have register >> intermediate >> + power states in the same domain. The domain governor can do a good >> job in >> + electing a power state when the last cpu is powered off as devices >> in the >> + same genpd may register intermediate states. >> > Devices may enable a certain domain state, but should not be defining > the domain state. The domain its state and the domain governor may > choose to enter that state on a vote from its devices. > >> + Devices : a device may register an intermediate c-state matching a >> memory >> + retention feature for instance. >> > This point onwards is where I need clarity. > > Thanks, > Lina > >> + >> Example: >> >> power: power-controller@12340000 { >> @@ -55,6 +65,25 @@ Example 2: >> #power-domain-cells = <1>; >> }; >> >> +Example 3: >> + >> + pm-domains { >> + a57_pd: a57_pd@ { >> + /* will have a57 platform >> ARM_PD_METHOD_OF_DECLARE*/ >> + compatible = "arm,pd","arm,cortex-a57"; >> + #power-domain-cells = <0>; >> + power-states = <&CLUSTER_SLEEP_0>; >> + }; >> + >> + a53_pd: a53_pd@ { >> + /* will have a a53 platform >> ARM_PD_METHOD_OF_DECLARE*/ >> + compatible = "arm,pd","arm,cortex-a53"; >> + #power-domain-cells = <0>; >> + power-states = <&CLUSTER_SLEEP_0>; >> + }; >> + }; >> + >> + >> The nodes above define two power controllers: 'parent' and 'child'. >> Domains created by the 'child' power controller are subdomains of '0' >> power >> domain provided by the 'parent' power controller. >> diff --git a/arch/arm64/boot/dts/arm/juno.dts >> b/arch/arm64/boot/dts/arm/juno.dts >> index 499f035..cadc5de 100644 >> --- a/arch/arm64/boot/dts/arm/juno.dts >> +++ b/arch/arm64/boot/dts/arm/juno.dts >> @@ -47,7 +47,7 @@ >> }; >> >> CLUSTER_SLEEP_0: cluster-sleep-0 { >> - compatible = "arm,idle-state"; >> + compatible = "arm,idle-state","arm,power-state"; >> arm,psci-suspend-param = <0x1010000>; >> local-timer-stop; >> entry-latency-us = <800>; >> @@ -128,13 +128,17 @@ >> pm-domains { >> >> a57_pd: a57_pd@ { >> - compatible = "arm,pd"; >> + /* will have the a53 platform ARM_PD_METHOD_OF_DECLARE*/ >> + compatible = "arm,pd","arm,cortex-a57"; >> #power-domain-cells = <0>; >> + power-states = <&CLUSTER_SLEEP_0>; >> }; >> >> a53_pd: a53_pd@ { >> - compatible = "arm,pd"; >> + /* will have the a53 platform ARM_PD_METHOD_OF_DECLARE*/ >> + compatible = "arm,pd","arm,cortex-a57"; >> #power-domain-cells = <0>; >> + power-states = <&CLUSTER_SLEEP_0>; >> }; >> }; >> >> diff --git a/drivers/base/power/cpu-pd.c b/drivers/base/power/cpu-pd.c >> index 5f30025..3c4cb12 100644 >> --- a/drivers/base/power/cpu-pd.c >> +++ b/drivers/base/power/cpu-pd.c >> @@ -163,6 +163,11 @@ int of_register_cpu_pm_domain(struct device_node >> *dn, >> pm_genpd_init(pd->genpd, &simple_qos_governor, false); >> of_genpd_add_provider_simple(dn, pd->genpd); >> >> + /* if a cpuidle-state is declared in this domain node, it will >> + * be the domain's job to enter/exit this state, if the device >> + * subdomain constraints are compatible */ >> + of_genpd_device_parse_states(dn, &pd->genpd); >> + >> /* Attach the CPUs to the CPU PM domain */ >> return of_pm_domain_attach_cpus(); >> } >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index e5f4c00b..1ca28a2 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -2156,6 +2156,104 @@ static void genpd_dev_pm_sync(struct device *dev) >> genpd_queue_power_off_work(pd); >> } >> >> +static int dt_cpuidle_to_genpd_power_state(struct genpd_power_state >> + *genpd_state, >> + const struct of_device_id *matches, >> + struct device_node *state_node) >> +{ >> + const struct of_device_id *match_id; >> + int err = 0; >> + u32 latency; >> + >> + match_id = of_match_node(matches, state_node); >> + if (!match_id) >> + return -ENODEV; >> + >> + err = of_property_read_u32(state_node, "wakeup-latency-us", >> &latency); >> + if (err) { >> + u32 entry_latency, exit_latency; >> + >> + err = of_property_read_u32(state_node, "entry-latency-us", >> + &entry_latency); >> + if (err) { >> + pr_debug(" * %s missing entry-latency-us property\n", >> + state_node->full_name); >> + return -EINVAL; >> + } >> + >> + err = of_property_read_u32(state_node, "exit-latency-us", >> + &exit_latency); >> + if (err) { >> + pr_debug(" * %s missing exit-latency-us property\n", >> + state_node->full_name); >> + return -EINVAL; >> + } >> + /* >> + * If wakeup-latency-us is missing, default to entry+exit >> + * latencies as defined in idle states bindings >> + */ >> + latency = entry_latency + exit_latency; >> + } >> + >> + genpd_state->power_on_latency_ns = 1000 * latency; >> + >> + err = of_property_read_u32(state_node, "entry-latency-us", >> &latency); >> + if (err) { >> + pr_debug(" * %s missing min-residency-us property\n", >> + state_node->full_name); >> + return -EINVAL; >> + } >> + >> + genpd_state->power_off_latency_ns = 1000 * latency; >> + >> + return 0; >> +} >> + >> +static const struct of_device_id power_state_match[] = { >> + {.compatible = "arm,power-state", >> + }, >> +}; >> + >> +int of_genpd_device_parse_states(struct device_node *np, >> + struct generic_pm_domain *genpd) >> +{ >> + struct device_node *state_node; >> + int i, err = 0; >> + >> + for (i = 0;; i++) { >> + struct genpd_power_state genpd_state; >> + >> + state_node = of_parse_phandle(np, "power-states", i); >> + if (!state_node) >> + break; >> + >> + err = dt_cpuidle_to_genpd_power_state(&genpd_state, >> + power_state_match, >> + state_node); >> + if (err) { >> + pr_err >> + ("Parsing idle state node %s failed with err %d\n", >> + state_node->full_name, err); >> + err = -EINVAL; >> + break; >> + } >> +#ifdef CONFIG_PM_ADVANCED_DEBUG >> + genpd_state.name = kstrndup(state_node->name, >> + GENPD_MAX_NAME_SIZE, GFP_KERNEL); >> + if (!genpd_state.name) >> + err = -ENOMEM; >> +#endif >> + of_node_put(state_node); >> + err = pm_genpd_insert_state(genpd, &genpd_state); >> + if (err) >> + break; >> +#ifdef CONFIG_PM_ADVANCED_DEBUG >> + kfree(genpd_state.name); >> +#endif >> + } >> + return err; >> +} >> + >> /** >> * genpd_dev_pm_attach - Attach a device to its PM domain using DT. >> * @dev: Device to attach. >> @@ -2224,6 +2322,8 @@ int genpd_dev_pm_attach(struct device *dev) >> return ret; >> } >> >> + of_genpd_device_parse_states(dev->of_node, pd); >> + >> dev->pm_domain->detach = genpd_dev_pm_detach; >> dev->pm_domain->sync = genpd_dev_pm_sync; >> pm_genpd_poweron(pd); >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index 8a4eab0..791a8ac 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -302,6 +302,9 @@ struct generic_pm_domain *__of_genpd_xlate_onecell( >> struct of_phandle_args *genpdspec, >> void *data); >> >> +int of_genpd_device_parse_states(struct device_node *np, >> + struct generic_pm_domain *genpd); >> + >> int genpd_dev_pm_attach(struct device *dev); >> #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ >> static inline int __of_genpd_add_provider(struct device_node *np, >> -- >> 1.9.1 >>
| |