Skip to contentSkip to conversation

pmdomain: core: add support for writeble power domain state

RFC PATCH In review Kamlesh Gurudasani 3 participants 5 messages Feb 21, 2025 – Mar 5, 2025 Generic PM domains

lore
Share & tools
lore

Kamlesh Gurudasani

author
#0 lore raw

RFC PATCH 1/1 pmdomain: core: add support for writeble power domain state

Add support for writeable power domain states from debugfs.

Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state node in debugfs.

Signed-off-by Kamlesh Gurudasani <[email protected]>

---
This has turn out to be really helpful when debugging SCMI protocol for power domain management.

Reference has been taken from clock framework which provides similar CLOCK_ALLOW_WRITE_DEBUGFS, which helps to test clocks from debugfs.

---
 drivers/pmdomain/core.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/pmdomain/core.c +77 -0
index 9b2f28b34bb5..6aba0c672da0 100644
@@ -1298,6 +1298,60 @@ late_initcall_sync(genpd_power_off_unused);
 
 #ifdef CONFIG_PM_SLEEP
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+/*
+ * This can be dangerous, therefore don't provide any real compile time
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+static int genpd_state_set(void *data, u64 val)
+{
+
+	struct generic_pm_domain *genpd = data;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (val == 1) {
+		genpd->power_on(genpd);
+		genpd->status = GENPD_STATE_ON;
+	} else if (val == 0) {
+		genpd->power_off(genpd);
+		genpd->status = GENPD_STATE_OFF;
+	}
+
+	genpd_unlock(genpd);
+	return 0;
+}
+
+#define pd_state_mode	0644
+
+static int genpd_state_get(void *data, u64 *val)
+{
+
+	struct generic_pm_domain *genpd = data;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (genpd->status == GENPD_STATE_OFF)
+		*val = 0;
+	else
+		*val = 1;
+
+	genpd_unlock(genpd);
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(pd_state_fops, genpd_state_get,
+			 genpd_state_set, "%llu\n");
+
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 /**
  * genpd_sync_power_off - Synchronously power off a PM domain and its parents.
  * @genpd: PM domain to power off, if possible.

@@ -3639,6 +3693,11 @@ static void genpd_debug_add(struct generic_pm_domain *genpd)
 	if (genpd->set_performance_state)
 		debugfs_create_file("perf_state", 0444,
 				    d, genpd, &perf_state_fops);
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+	debugfs_create_file("pd_state", 0644, d, genpd,
+			    &pd_state_fops);
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 }
 
 static int __init genpd_debug_init(void)

@@ -3653,6 +3712,24 @@ static int __init genpd_debug_init(void)
 	list_for_each_entry(genpd, &gpd_list, gpd_list_node)
 		genpd_debug_add(genpd);
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+	pr_warn("\n");
+	pr_warn("********************************************************************\n");
+	pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE           **\n");
+	pr_warn("**                                                                **\n");
+	pr_warn("**  WRITEABLE POWER DOMAIN STATE DEBUGFS SUPPORT HAS BEEN ENABLED **\n");
+	pr_warn("**  IN THIS KERNEL                                                **\n");
+	pr_warn("** This means that this kernel is built to expose pd operations   **\n");
+	pr_warn("** such as enabling, disabling, etc.                              **\n");
+	pr_warn("** to userspace, which may compromise security on your system.    **\n");
+	pr_warn("**                                                                **\n");
+	pr_warn("** If you see this message and you are not debugging the          **\n");
+	pr_warn("** kernel, report this immediately to your vendor!                **\n");
+	pr_warn("**                                                                **\n");
+	pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE           **\n");
+	pr_warn("********************************************************************\n");
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 	return 0;
 }
 late_initcall(genpd_debug_init);

---
base-commit: d4b0fd87ff0d4338b259dc79b2b3c6f7e70e8afa
change-id: 20250221-pm-debug-0824da30890f

Best regards,

Signature · 1 line
Kamlesh Gurudasani <[email protected]>
#1 lore raw
Quoted 1 line · Kamlesh Gurudasani

Add support for writeable power domain states from debugfs.

ACK.

+CC'ed a few more folks who maybe interested...

Quoted 7 lines

Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state node in debugfs.

Signed-off-by Kamlesh Gurudasani <[email protected]>

---
This has turn out to be really helpful when debugging SCMI protocol for power domain management.

Agreed, I have found this patch very good for turning off/on power domains in a "raw" form if you will.
We have also been using something called k3conf on TI K3
class of devices like AM62x family where we basically use a user space tool to send power on/off/ device status without it going via the kernel genPD. This approach that Kamlesh is suggesting helps us leverage the kernel stacks (like SCMI) but while also going via the pmdomain core driver. This patch helps abstract underneath protocol layers used for power domain operations in SOCs...

Here's how it looks for anyone wondering: [1]
We also talked about this at last year's Embedded Open Source Summit, Seattle [2] where we mentioned we'd be upstreaming this patch and the audience seemed interested to use it.

Quoted 18 lines

Reference has been taken from clock framework which provides similar CLOCK_ALLOW_WRITE_DEBUGFS, which helps to test clocks from debugfs.

---
 drivers/pmdomain/core.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/pmdomain/core.c +3 -0
index 9b2f28b34bb5..6aba0c672da0 100644
@@ -1298,6 +1298,60 @@ late_initcall_sync(genpd_power_off_unused);
 
 #ifdef CONFIG_PM_SLEEP
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+/*
+ * This can be dangerous, therefore don't provide any real compile time

drivers/pmdomain/core.c late_initcall_sync(genpd_power_off_unused); · line 1313

Quoted 10 lines
drivers/pmdomain/core.c +10 -0
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+static int genpd_state_set(void *data, u64 val)
+{
+
+	struct generic_pm_domain *genpd = data;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);

Why has it been kept interruptible? Other places I see are just using genpd_lock(pd);

Quoted 6 lines
drivers/pmdomain/core.c +6 -0
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (val == 1) {
+		genpd->power_on(genpd);
+		genpd->status = GENPD_STATE_ON;

drivers/pmdomain/core.c late_initcall_sync(genpd_power_off_unused); · line 1329

Quoted 10 lines
drivers/pmdomain/core.c +10 -0
+	} else if (val == 0) {
+		genpd->power_off(genpd);
+		genpd->status = GENPD_STATE_OFF;
+	}
+
+	genpd_unlock(genpd);
+	return 0;
+}
+
+#define pd_state_mode	0644

Where's this used?

Quoted 29 lines
drivers/pmdomain/core.c +25 -0
+
+static int genpd_state_get(void *data, u64 *val)
+{
+
+	struct generic_pm_domain *genpd = data;
+	int ret = 0;
+
+	ret = genpd_lock_interruptible(genpd);
+	if (ret)
+		return -ERESTARTSYS;
+
+	if (genpd->status == GENPD_STATE_OFF)
+		*val = 0;
+	else
+		*val = 1;
+
+	genpd_unlock(genpd);
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(pd_state_fops, genpd_state_get,
+			 genpd_state_set, "%llu\n");
+
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 /**
  * genpd_sync_power_off - Synchronously power off a PM domain and its parents.
  * @genpd: PM domain to power off, if possible.

drivers/pmdomain/core.c static void genpd_debug_add(struct generic_pm_domain *genpd) · line 3697

Quoted 5 lines
drivers/pmdomain/core.c +2 -0
@@ -3639,6 +3693,11 @@ static void genpd_debug_add(struct generic_pm_domain *genpd)
 	if (genpd->set_performance_state)
 		debugfs_create_file("perf_state", 0444,
 				    d, genpd, &perf_state_fops);
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+	debugfs_create_file("pd_state", 0644, d, genpd,

Ah this is probably where you wanted to use the #define'd pd_state_mode I guess....

drivers/pmdomain/core.c static int __init genpd_debug_init(void) · line 3714

Quoted 10 lines
drivers/pmdomain/core.c +3 -0
+			    &pd_state_fops);
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 }
 
 static int __init genpd_debug_init(void)

@@ -3653,6 +3712,24 @@ static int __init genpd_debug_init(void)
 	list_for_each_entry(genpd, &gpd_list, gpd_list_node)
 		genpd_debug_add(genpd);

Only minor cleanups pending, otherwise patch looks good to me. Hoping more people can also try it out and jump on this thread with their thoughts....

Refs:
[1] https://gist.github.com/DhruvaG2000/f3ec24252d4cf2800c97c2e336d0b5db
[2]
https://eoss24.sched.com/event/1aBEs/a-primer-to-clocks-and-power-management-through-arm-scmi-dhruva-gole-kamlesh-gurudasani-texas-instruments

Signature · 3 lines
Best regards,
Dhruva Gole
Texas Instruments Incorporated
#2 lore raw
In reply to Kamlesh Gurudasani
Quoted 41 lines · Kamlesh Gurudasani

Add support for writeable power domain states from debugfs.

Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state node in debugfs.

Signed-off-by Kamlesh Gurudasani <[email protected]>

---
This has turn out to be really helpful when debugging SCMI protocol for power domain management.

Reference has been taken from clock framework which provides similar CLOCK_ALLOW_WRITE_DEBUGFS, which helps to test clocks from debugfs.

---
 drivers/pmdomain/core.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/pmdomain/core.c +16 -0
index 9b2f28b34bb5..6aba0c672da0 100644
@@ -1298,6 +1298,60 @@ late_initcall_sync(genpd_power_off_unused);
 
 #ifdef CONFIG_PM_SLEEP
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+/*
+ * This can be dangerous, therefore don't provide any real compile time
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+static int genpd_state_set(void *data, u64 val)
+{
+
+       struct generic_pm_domain *genpd = data;
+       int ret = 0;
+
+       ret = genpd_lock_interruptible(genpd);
+       if (ret)
+               return -ERESTARTSYS;
+

drivers/pmdomain/core.c late_initcall_sync(genpd_power_off_unused); · line 1326

Quoted 10 lines
drivers/pmdomain/core.c +10 -0
+       if (val == 1) {
+               genpd->power_on(genpd);
+               genpd->status = GENPD_STATE_ON;
+       } else if (val == 0) {
+               genpd->power_off(genpd);
+               genpd->status = GENPD_STATE_OFF;
+       }
+
+       genpd_unlock(genpd);
+       return 0;

This makes the behaviour in genpd inconsistent and I am worried about that, even if it's for debugging/development.

For example, what if there is a device hooked up to the genpd which requires its PM domain to stay on? And what about child domains?

Quoted 73 lines
drivers/pmdomain/core.c +51 -1
+}
+
+#define pd_state_mode  0644
+
+static int genpd_state_get(void *data, u64 *val)
+{
+
+       struct generic_pm_domain *genpd = data;
+       int ret = 0;
+
+       ret = genpd_lock_interruptible(genpd);
+       if (ret)
+               return -ERESTARTSYS;
+
+       if (genpd->status == GENPD_STATE_OFF)
+               *val = 0;
+       else
+               *val = 1;
+
+       genpd_unlock(genpd);
+       return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(pd_state_fops, genpd_state_get,
+                        genpd_state_set, "%llu\n");
+
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 /**
  * genpd_sync_power_off - Synchronously power off a PM domain and its parents.
  * @genpd: PM domain to power off, if possible.

@@ -3639,6 +3693,11 @@ static void genpd_debug_add(struct generic_pm_domain *genpd)
        if (genpd->set_performance_state)
                debugfs_create_file("perf_state", 0444,
                                    d, genpd, &perf_state_fops);
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+       debugfs_create_file("pd_state", 0644, d, genpd,
+                           &pd_state_fops);
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 }
 
 static int __init genpd_debug_init(void)

@@ -3653,6 +3712,24 @@ static int __init genpd_debug_init(void)
        list_for_each_entry(genpd, &gpd_list, gpd_list_node)
                genpd_debug_add(genpd);
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+       pr_warn("\n");
+       pr_warn("********************************************************************\n");
+       pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE           **\n");
+       pr_warn("**                                                                **\n");
+       pr_warn("**  WRITEABLE POWER DOMAIN STATE DEBUGFS SUPPORT HAS BEEN ENABLED **\n");
+       pr_warn("**  IN THIS KERNEL                                                **\n");
+       pr_warn("** This means that this kernel is built to expose pd operations   **\n");
+       pr_warn("** such as enabling, disabling, etc.                              **\n");
+       pr_warn("** to userspace, which may compromise security on your system.    **\n");
+       pr_warn("**                                                                **\n");
+       pr_warn("** If you see this message and you are not debugging the          **\n");
+       pr_warn("** kernel, report this immediately to your vendor!                **\n");
+       pr_warn("**                                                                **\n");
+       pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE           **\n");
+       pr_warn("********************************************************************\n");
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
        return 0;
 }
 late_initcall(genpd_debug_init);
 
---

base-commit: d4b0fd87ff0d4338b259dc79b2b3c6f7e70e8afa
change-id: 20250221-pm-debug-0824da30890f

Quoted 4 lines

Best regards,
--
Kamlesh Gurudasani <[email protected]>

When working with genpd and SCMI PM domains, a more robust way to debug things would be to implement a slim skeleton driver for a consumer device. In principle it just needs some basic runtime PM support and the corresponding device hooked up to the SCMI PM domain in DT. In this way, we can use the existing sysfs interface for runtime PM, to control and debug the behaviour of the genpd/SCMI PM domain.

I have a bunch of local code/drivers for this already. Even for testing the SCMI perf domain with OPPs. I can share them with you, if you are interested?

Kind regards
Uffe

#3 lore raw

Ulf,

Quoted 56 lines · Ulf Hansson
Quoted 41 lines · Kamlesh Gurudasani

Add support for writeable power domain states from debugfs.

Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state node in debugfs.

Signed-off-by Kamlesh Gurudasani <[email protected]>

---
This has turn out to be really helpful when debugging SCMI protocol for power domain management.

Reference has been taken from clock framework which provides similar CLOCK_ALLOW_WRITE_DEBUGFS, which helps to test clocks from debugfs.

---
 drivers/pmdomain/core.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/pmdomain/core.c +16 -0
index 9b2f28b34bb5..6aba0c672da0 100644
@@ -1298,6 +1298,60 @@ late_initcall_sync(genpd_power_off_unused);
 
 #ifdef CONFIG_PM_SLEEP
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+/*
+ * This can be dangerous, therefore don't provide any real compile time
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+static int genpd_state_set(void *data, u64 val)
+{
+
+       struct generic_pm_domain *genpd = data;
+       int ret = 0;
+
+       ret = genpd_lock_interruptible(genpd);
+       if (ret)
+               return -ERESTARTSYS;
+

drivers/pmdomain/core.c late_initcall_sync(genpd_power_off_unused); · line 1326

Quoted 10 lines
drivers/pmdomain/core.c +10 -0
+       if (val == 1) {
+               genpd->power_on(genpd);
+               genpd->status = GENPD_STATE_ON;
+       } else if (val == 0) {
+               genpd->power_off(genpd);
+               genpd->status = GENPD_STATE_OFF;
+       }
+
+       genpd_unlock(genpd);
+       return 0;

This makes the behaviour in genpd inconsistent and I am worried about that, even if it's for debugging/development.

Quoted 2 lines

For example, what if there is a device hooked up to the genpd which requires its PM domain to stay on? And what about child domains?

Thanks for taking a look.

Agreed that there maybe some paths in genpd that this patch maybe ignoring and thus could break something fundamental while debugging.

Perhaps we can split this patch up and remove the state_set part till we figure out the right way of doing it without breaking genPD

BUT, as I said in my previous response I feel that if one is enabling DEBUG config then anyway they are literally aware of the risk that they are taking, by exposing raw PD on/off operations from user space. Perhaps we can continue our debate on the reply I gave earlier on this thread...

Quoted 74 lines
Quoted 74 lines
drivers/pmdomain/core.c +51 -1
+}
+
+#define pd_state_mode  0644
+
+static int genpd_state_get(void *data, u64 *val)
+{
+
+       struct generic_pm_domain *genpd = data;
+       int ret = 0;
+
+       ret = genpd_lock_interruptible(genpd);
+       if (ret)
+               return -ERESTARTSYS;
+
+       if (genpd->status == GENPD_STATE_OFF)
+               *val = 0;
+       else
+               *val = 1;
+
+       genpd_unlock(genpd);
+       return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(pd_state_fops, genpd_state_get,
+                        genpd_state_set, "%llu\n");
+
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 /**
  * genpd_sync_power_off - Synchronously power off a PM domain and its parents.
  * @genpd: PM domain to power off, if possible.

@@ -3639,6 +3693,11 @@ static void genpd_debug_add(struct generic_pm_domain *genpd)
        if (genpd->set_performance_state)
                debugfs_create_file("perf_state", 0444,
                                    d, genpd, &perf_state_fops);
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+       debugfs_create_file("pd_state", 0644, d, genpd,
+                           &pd_state_fops);
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
 }
 
 static int __init genpd_debug_init(void)

@@ -3653,6 +3712,24 @@ static int __init genpd_debug_init(void)
        list_for_each_entry(genpd, &gpd_list, gpd_list_node)
                genpd_debug_add(genpd);
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+       pr_warn("\n");
+       pr_warn("********************************************************************\n");
+       pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE           **\n");
+       pr_warn("**                                                                **\n");
+       pr_warn("**  WRITEABLE POWER DOMAIN STATE DEBUGFS SUPPORT HAS BEEN ENABLED **\n");
+       pr_warn("**  IN THIS KERNEL                                                **\n");
+       pr_warn("** This means that this kernel is built to expose pd operations   **\n");
+       pr_warn("** such as enabling, disabling, etc.                              **\n");
+       pr_warn("** to userspace, which may compromise security on your system.    **\n");
+       pr_warn("**                                                                **\n");
+       pr_warn("** If you see this message and you are not debugging the          **\n");
+       pr_warn("** kernel, report this immediately to your vendor!                **\n");
+       pr_warn("**                                                                **\n");
+       pr_warn("**     NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE           **\n");
+       pr_warn("********************************************************************\n");
+#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
+
        return 0;
 }
 late_initcall(genpd_debug_init);
 
---

base-commit: d4b0fd87ff0d4338b259dc79b2b3c6f7e70e8afa
change-id: 20250221-pm-debug-0824da30890f

Quoted 10 lines
Quoted 4 lines

Best regards,
--
Kamlesh Gurudasani <[email protected]>

When working with genpd and SCMI PM domains, a more robust way to debug things would be to implement a slim skeleton driver for a consumer device. In principle it just needs some basic runtime PM support and the corresponding device hooked up to the SCMI PM domain in DT. In this way, we can use the existing sysfs interface for

But this will just be a per-device limited solution right? It still won't allow us a generic way of dealing with all possible scmi IDs without having to rebuild the system... Or maybe I am misunderstanding/ missing something...

Quoted 2 lines

runtime PM, to control and debug the behaviour of the genpd/SCMI PM domain.

If I were to come from a user's perspective, then they will want to be able to get a full view of the system's power status at a glance. Today, I am not aware of a way how we can do that from genpd. This makes debugging what is _actually_ ON/OFF in the system a bit tricky..

Users may not even care much for example about the kernel drivers for certain controllers but still want to ensure that those controller's registers are accessible to them to use via something like devmem2.
Another application for the get_status part of this patch is for studying the overall power draw of the system by judging which all IDs are on/off at a glance.

Hence, if you feel that for now the state_get part of this patch makes sense it will still help users query the status of all the pd id's in the system.

Thinking of it... What if we modify the existing status_show() itself with another column that shows current status similar to runtime status? status_show today only does a print based off of genpd->status ... and never really goes and queries the firmware again if by any chance some other event or activity in the system may have turned ON the device.

For eg. if we have another remote processor request a resource
but genPD was unaware of this request so it just assumes that resource is still off-0... That's just wrong IMO.

What I am proposing is can we have a get_state callback in genPD which would be = something like power_ops->state_get in scmi pmdomains today.
This will help the core pmdomains get an up-to-date view of the system in the debugFS. Whether genPD decides to update it's internal genpd->status based on the query is another issue.
But from what it looks like, we definitely have a requirement here to make sure pm_genpd_summary shows a true-to life status of the power domains. Not just rely on linux runtime pm or assume that linux is the only real software who knows and does it all.

Quoted 4 lines

I have a bunch of local code/drivers for this already. Even for testing the SCMI perf domain with OPPs. I can share them with you, if you are interested?

Yes, please do share. We would love to see your ideas on this so we can come up with a better solutions together.

Signature · 3 lines
Best regards,
Dhruva Gole
Texas Instruments Incorporated
#4 lore raw

+ Sudeep, Cristian

Quoted 68 lines · Dhruva Gole

Ulf,

Quoted 56 lines · Ulf Hansson
Quoted 41 lines · Kamlesh Gurudasani

Add support for writeable power domain states from debugfs.

Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state node in debugfs.

Signed-off-by Kamlesh Gurudasani <[email protected]>

---
This has turn out to be really helpful when debugging SCMI protocol for power domain management.

Reference has been taken from clock framework which provides similar CLOCK_ALLOW_WRITE_DEBUGFS, which helps to test clocks from debugfs.

---
 drivers/pmdomain/core.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/pmdomain/core.c +16 -0
index 9b2f28b34bb5..6aba0c672da0 100644
@@ -1298,6 +1298,60 @@ late_initcall_sync(genpd_power_off_unused);
 
 #ifdef CONFIG_PM_SLEEP
 
+#ifdef GENPD_ALLOW_WRITE_DEBUGFS
+/*
+ * This can be dangerous, therefore don't provide any real compile time
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+static int genpd_state_set(void *data, u64 val)
+{
+
+       struct generic_pm_domain *genpd = data;
+       int ret = 0;
+
+       ret = genpd_lock_interruptible(genpd);
+       if (ret)
+               return -ERESTARTSYS;
+

drivers/pmdomain/core.c late_initcall_sync(genpd_power_off_unused); · line 1326

Quoted 10 lines
drivers/pmdomain/core.c +10 -0
+       if (val == 1) {
+               genpd->power_on(genpd);
+               genpd->status = GENPD_STATE_ON;
+       } else if (val == 0) {
+               genpd->power_off(genpd);
+               genpd->status = GENPD_STATE_OFF;
+       }
+
+       genpd_unlock(genpd);
+       return 0;

This makes the behaviour in genpd inconsistent and I am worried about that, even if it's for debugging/development.

Quoted 2 lines

For example, what if there is a device hooked up to the genpd which requires its PM domain to stay on? And what about child domains?

Thanks for taking a look.

Agreed that there maybe some paths in genpd that this patch maybe ignoring and thus could break something fundamental while debugging.

Quoted 8 lines

Perhaps we can split this patch up and remove the state_set part till we figure out the right way of doing it without breaking genPD

BUT, as I said in my previous response I feel that if one is enabling DEBUG config then anyway they are literally aware of the risk that they are taking, by exposing raw PD on/off operations from user space. Perhaps we can continue our debate on the reply I gave earlier on this thread...

Sure!

[...]

Quoted 12 lines
Quoted 6 lines

When working with genpd and SCMI PM domains, a more robust way to debug things would be to implement a slim skeleton driver for a consumer device. In principle it just needs some basic runtime PM support and the corresponding device hooked up to the SCMI PM domain in DT. In this way, we can use the existing sysfs interface for

But this will just be a per-device limited solution right? It still won't allow us a generic way of dealing with all possible scmi IDs without having to rebuild the system... Or maybe I am misunderstanding/ missing something...

Quoted 7 lines
Quoted 2 lines

runtime PM, to control and debug the behaviour of the genpd/SCMI PM domain.

If I were to come from a user's perspective, then they will want to be able to get a full view of the system's power status at a glance. Today, I am not aware of a way how we can do that from genpd. This makes debugging what is _actually_ ON/OFF in the system a bit tricky..

There are debugfs support for genpd. It should give you just this.

Quoted 18 lines

Users may not even care much for example about the kernel drivers for certain controllers but still want to ensure that those controller's registers are accessible to them to use via something like devmem2.
Another application for the get_status part of this patch is for studying the overall power draw of the system by judging which all IDs are on/off at a glance.

Hence, if you feel that for now the state_get part of this patch makes sense it will still help users query the status of all the pd id's in the system.

Thinking of it... What if we modify the existing status_show() itself with another column that shows current status similar to runtime status? status_show today only does a print based off of genpd->status ... and never really goes and queries the firmware again if by any chance some other event or activity in the system may have turned ON the device.

Quoted 3 lines

For eg. if we have another remote processor request a resource
but genPD was unaware of this request so it just assumes that resource is still off-0... That's just wrong IMO.

I see. So in principle you want a way to ask the SCMI FW about it's current state - without involving the view Linux/genpd has.

Quoted 11 lines

What I am proposing is can we have a get_state callback in genPD which would be = something like power_ops->state_get in scmi pmdomains today.
This will help the core pmdomains get an up-to-date view of the system in the debugFS. Whether genPD decides to update it's internal genpd->status based on the query is another issue.
But from what it looks like, we definitely have a requirement here to make sure pm_genpd_summary shows a true-to life status of the power domains. Not just rely on linux runtime pm or assume that linux is the only real software who knows and does it all.

Well, I am not sure I agree, but maybe I don't have the complete picture.

To me, this sounds like an SCMI specific debug thing. Don't we have something like that already?

I have looped in Sudeep and Cristian to comment too.

Quoted 9 lines
Quoted 5 lines

I have a bunch of local code/drivers for this already. Even for testing the SCMI perf domain with OPPs. I can share them with you, if you are interested?

Yes, please do share. We would love to see your ideas on this so we can come up with a better solutions together.

The below is from an old optee tree and its build git. There is a DTS file added for QEMU in one commit and another commit that adds SCMI power/perf domains, along with other added test devices for a PM domain driver and a PM test driver:
git.linaro.org/people/ulf.hansson/optee_build.git qemu_v8_scmi

I have also published my PM test drivers at the git below. One platform driver adds/manages PM domains. Another one, is a simple platform driver, that has PM and OPP support along with some debugfs things that has been useful for me. git.kernel.org/pub/scm/linux/kernel/git/ulfh/linux-pm.git debug_pm

Kind regards
Uffe