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? > > > + * > > + * @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. 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? Kind regards Uffe