[RFC] 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 <kamlesh@ti.com>
---
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(+)


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

Best regards,

On Feb 21, 2025 at 19:18:10 +0530, Kamlesh Gurudasani wrote:
> Add support for writeable power domain states from debugfs.

ACK.

+CC'ed a few more folks who maybe interested...
 
> Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state
> node in debugfs.
> 
> Signed-off-by: Kamlesh Gurudasani <kamlesh@ti.com>
> ---
> 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.

> 
> 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(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 9b2f28b34bb5..6aba0c672da0 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -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);

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

> +	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

Where's this used?

> +
> +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,

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

> +			    &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

On Fri, 21 Feb 2025 at 14:48, Kamlesh Gurudasani <kamlesh@ti.com> wrote:
>
> 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 <kamlesh@ti.com>
> ---
> 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(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 9b2f28b34bb5..6aba0c672da0 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -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;

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?

> +}
> +
> +#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,
> --
> Kamlesh Gurudasani <kamlesh@ti.com>
>

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