Re: [PATCH RFC] pmdomain: core: add support for writeble power domain state

[Ulf Hansson]

+ Sudeep, Cristian

On Mon, 3 Mar 2025 at 10:58, Dhruva Gole <d-gole@xxxxxx> wrote:
>
> Ulf,
>
> On Feb 28, 2025 at 13:39:44 +0100, Ulf Hansson wrote:
> > On Fri, 21 Feb 2025 at 14:48, Kamlesh Gurudasani <kamlesh@xxxxxx> 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@xxxxxx>
> > > ---
> > > 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?
>
> 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...

Sure!

[...]

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

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

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.

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

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