Re: Re: [PATCH v4 1/5] PM: domains: Allow devices attached to genpd to be managed by HW

[Bjorn Andersson]

On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> > > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > >
> > > Some power-domains may be capable of relying on the HW to control the power
> > > for a device that's hooked up to it. Typically, for these kinds of
> > > configurations the consumer driver should be able to change the behavior of
> > > power domain at runtime, control the power domain in SW mode for certain
> > > configurations and handover the control to HW mode for other usecases.
> > >
> > > To allow a consumer driver to change the behaviour of the PM domain for its
> > > device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover,
> > > let's add a corresponding optional genpd callback, ->set_hwmode_dev(),
> > > which the genpd provider should implement if it can support switching
> > > between HW controlled mode and SW controlled mode. Similarly, add the
> > > dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and
> > > its corresponding optional genpd callback, ->get_hwmode_dev(), which the
> > > genpd provider can also implement for reading back the mode from the
> > > hardware.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > >  drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pm_domain.h | 17 ++++++++++++
> > >  2 files changed, 86 insertions(+)
> > >
> > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > index a1f6cba3ae6c..41b6411d0ef5 100644
> > > --- a/drivers/pmdomain/core.c
> > > +++ b/drivers/pmdomain/core.c
> > > @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
> > >
> > > +/**
> > > + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain.
> >
> > This isn't proper kernel-doc
> 
> Sorry, I didn't quite get that. What is wrong?
> 

https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
says that there should be () after the function name, and below there
should be a Return:

> >
> > > + *
> > > + * @dev: Device for which the HW-mode should be changed.
> > > + * @enable: Value to set or unset the HW-mode.
> > > + *
> > > + * Some PM domains can rely on HW signals to control the power for a device. To
> > > + * allow a consumer driver to switch the behaviour for its device in runtime,
> > > + * which may be beneficial from a latency or energy point of view, this function
> > > + * may be called.
> > > + *
> > > + * It is assumed that the users guarantee that the genpd wouldn't be detached
> > > + * while this routine is getting called.
> > > + *
> > > + * Returns 0 on success and negative error values on failures.
> > > + */
> > > +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable)
> > > +{
> > > +     struct generic_pm_domain *genpd;
> > > +     int ret = 0;
> > > +
> > > +     genpd = dev_to_genpd_safe(dev);
> > > +     if (!genpd)
> > > +             return -ENODEV;
> > > +
> > > +     if (!genpd->set_hwmode_dev)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     genpd_lock(genpd);
> > > +
> > > +     if (dev_gpd_data(dev)->hw_mode == enable)
> >
> > Between this and the gdsc patch, the hw_mode state might not match the
> > hardware state at boot.
> >
> > With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
> > false) will not bring control to SW - which might be fatal.
> 
> Right, good point.
> 
> I think we have two ways to deal with this:
> 1) If the provider is supporting ->get_hwmode_dev(), we can let
> genpd_add_device() invoke it to synchronize the state.

I'd suggest that we skip the optimization for now and just let the
update hit the driver on each call.

> 2) If the provider doesn't support ->get_hwmode_dev() we need to call
> ->set_hwmode_dev() to allow an initial state to be set.
> 
> The question is then, if we need to allow ->get_hwmode_dev() to be
> optional, if the ->set_hwmode_dev() is supported - or if we can
> require it. What's your thoughts around this?
> 

Iiuc this resource can be shared between multiple clients, and we're
in either case returning the shared state. That would mean a client
acting upon the returned value, is subject to races.

I'm therefore inclined to say that we shouldn't have a getter, other
than for debugging purposes, in which case reading the HW-state or
failing would be reasonable outcomes.

Regards,
Bjorn