Re: [RFC PATCH 2/3] pmdomain: core: Add a genpd config to support unknown initial status

On 26-08-20 11:56:40, Ulf Hansson wrote:
> On Wed, Aug 19, 2026 at 7:40 PM Abel Vesa <[email protected]> wrote:
> >
> > On 26-08-19 17:14:52, Ulf Hansson wrote:
> > > From: Ulf Hansson <[email protected]>
> > >
> > > It's not always possible for a genpd provider to know the initial status
> > > for its corresponding PM domain(s). To register the PM domain in a safe
> > > state, the genpd provider driver may therefore have to initialize the
> > > genpd's status to be powered off, as a way to prevent the PM domain from
> > > being used when it actually could be powered off.
> > >
> > > In these cases we may end up to power on a PM domain through the genpd
> > > subsystem, while from the HW point of view it may already be powered on.
> > > Under these circumstances, it may also be required to keep the PM domain
> > > powered on until all the consumer devices have been probed. In other words,
> > > until the ->sync_state() callback for the genpd provider in question have
> > > been called.
> > >
> > > To support this behaviour for a genpd provider, let's introduce a new genpd
> > > configuration, GENPD_FLAG_OFF_STAY_ON.
> > >
> > > Reported-by: Maulik Shah <[email protected]>
> > > Link: https://lore.kernel.org/all/[email protected]/
> > > Signed-off-by: Ulf Hansson <[email protected]>
> > > ---
> > >  drivers/pmdomain/core.c   | 6 +++++-
> > >  include/linux/pm_domain.h | 9 +++++++++
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > index 7345c06f1d55..be4fff2f4664 100644
> > > --- a/drivers/pmdomain/core.c
> > > +++ b/drivers/pmdomain/core.c
> > > @@ -189,6 +189,7 @@ static const struct genpd_lock_ops genpd_raw_spin_ops = {
> > >  #define genpd_is_dev_name_fw(genpd)  (genpd->flags & GENPD_FLAG_DEV_NAME_FW)
> > >  #define genpd_is_no_sync_state(genpd)        (genpd->flags & GENPD_FLAG_NO_SYNC_STATE)
> > >  #define genpd_is_no_stay_on(genpd)   (genpd->flags & GENPD_FLAG_NO_STAY_ON)
> > > +#define genpd_is_off_stay_on(genpd)  (genpd->flags & GENPD_FLAG_OFF_STAY_ON)
> >
> > Maybe genpd_is_state_unknown ?
> >
> > >
> > >  static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
> > >               const struct generic_pm_domain *genpd)
> > > @@ -2445,7 +2446,10 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
> > >  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> > >  static void genpd_set_stay_on(struct generic_pm_domain *genpd, bool is_off)
> > >  {
> > > -     genpd->stay_on = !genpd_is_no_stay_on(genpd) && !is_off;
> > > +     if (genpd_is_off_stay_on(genpd))
> > > +             genpd->stay_on = is_off;
> > > +     else
> > > +             genpd->stay_on = !genpd_is_no_stay_on(genpd) && !is_off;
> > >  }
> > >  #else
> > >  static void genpd_set_stay_on(struct generic_pm_domain *genpd, bool is_off)
> > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > > index f925614aebdb..9929347e1d7f 100644
> > > --- a/include/linux/pm_domain.h
> > > +++ b/include/linux/pm_domain.h
> > > @@ -121,6 +121,14 @@ struct dev_pm_domain_list {
> > >   *                           powered-off until the ->sync_state() callback is
> > >   *                           invoked. This flag informs genpd to allow a
> > >   *                           power-off without waiting for ->sync_state().
> > > + *
> > > + * GENPD_FLAG_OFF_STAY_ON:   Use this flag to inform genpd that its initial
> > > + *                           status for the PM domain is set to powered off,
> > > + *                           which may not correctly reflect the state of the
> > > + *                           HW, as it's unknown. If the PM domain becomes
> > > + *                           powered on during boot, genpd will prevent it
> > > + *                           from being powered off until the ->sync_state
> > > + *                           callback is invoked for it.
> >
> > Maybe GENPD_FLAG_STATE_UNKNOWN ?
> 
> Hmm, I understand your idea to clarify this and it sounds reasonable to me.
> 
> However, "STATE" sounds a bit vague, what do you think of
> "GENPD_FLAG_POWER_UNKNOWN"?

I guess that could work too. I was suggesting "STATE" because the
gpd_status holds STATE_ON and STATE_OFF. So it would be the flag that
says the state is unknown.

> 
> >
> > Or probably it would be better to just add a new state to the gpd_status
> > enum, like GENPD_STATE_UNKNOWN.
> 
> Well, the problem with that is how it should be treated by genpd
> internals, when there are child/parent domains. Having an in-between
> status sounds unnecessarily complicated to me, at least for the
> purpose of $subject patch.

As for it being unnecesarily complicated, it would actually be the
true description of such PD. Otherwise we might end up with cases
where it is inconsistent with the actual hardware state.

But I agree, it isn't something that should be part of this patch.

Either way, I think the patch looks good:

Reviewed-by: Abel Vesa <[email protected]>