| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5617912A.6080600@baylibre.com>
Date: Fri, 9 Oct 2015 12:04:26 +0200
From: Marc Titinger <mtitinger@...libre.com>
To: Lina Iyer <lina.iyer@...aro.org>
Cc: khilman@...nel.org, rjw@...ysocki.net, ahaslam@...libre.com,
bcousson@...libre.com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org,
Marc Titinger <mtitinger+renesas@...libre.com>
Subject: Re: [RFC v2 3/6] PM / Domains: introduce power-states consistent with
c-states.
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@...libre.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@...40000 {
>> @@ -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
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists