On Thu, 24 Oct 2019 at 18:31, Sudeep Holla [off-list ref] wrote:

On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:

quoted

Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
CPU number as an in-parameter and tries to attach the CPU's struct device
to its corresponding PM domain.

Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
specify "psci" as the "name" of the PM domain to attach to. Additionally,
let's also prepare the attached device to be power managed via runtime PM.

Signed-off-by: Ulf Hansson <redacted>
---
 drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
 drivers/cpuidle/cpuidle-psci.h        |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 3f5143ccc3e0..7429fd7626a1 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c

@@ -9,9 +9,11 @@

 #define pr_fmt(fmt) "CPUidle PSCI: " fmt

+#include <linux/cpu.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/psci.h>
 #include <linux/slab.h>
 #include <linux/string.h>

@@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
      return ret;
 }
 subsys_initcall(psci_idle_init_domains);
+
+struct device *psci_dt_attach_cpu(int cpu)
+{
+     struct device *dev;
+
+     /* Currently limit the hierarchical topology to be used in OSI mode. */
+     if (!psci_has_osi_support())
+             return NULL;
+
+     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");

This clarifies the need for the fixed name. But why not just go by index 0
as the consumer of these psci power-domains will have only one power domain
entry. Why do we need this name compulsory ?

The idea is to be future proof. If I recall correctly, the CPU node on
some QCOM SoCs may also have "CPR" PM domain specified, thus
"multiple" power-domains could be specified.

In any case, using "psci" doesn't really hurt, right?

Further, it's specified as
optional in the generic binding, do we make it "required" for this psci
idle states binding anywhere that I missed ?

Good point! Unless you tell me differently, I will update the DT doc
to clarify this is "required".

Kind regards
Uffe