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

[Ulf Hansson]

On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:
>
>
>
> On 2/15/2024 9:57 PM, Ulf Hansson wrote:
> > On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> >>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> >>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> 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:
> >>>>>
> >>>>> Thanks for the pointers!
> >>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>>> + *
> >>>>>>>>> + * @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.
> >>>>>
> >>>>> Okay.
> >>>>>
> >>>>>>
> >>>>>>> 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.
> >>>>>
> >>>>> Not sure I understand this, but I also don't have in-depth knowledge
> >>>>> of how the HW works.
> >>>>>
> >>>>> Isn't the HW mode set on a per device basis?
> >>>>>
> >>>>>>
> >>>>>> 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.
> >>>>>
> >>>>> If you only want this for debug purposes, it seems better to keep it
> >>>>> closer to the rpmh code, rather than adding generic callbacks to the
> >>>>> genpd interface.
> >>>>>
> >>>>> So to conclude, you think having a ->set_hwmode_dev() callback should
> >>>>> be sufficient and no caching of the current state?
> >>>>>
> >>>>> Abel, what's your thoughts around this?
> >>>>>
> >>>>
> >>>> We believe it is good to have get_hwmode_dev() callback supported from
> >>>> GenPD, since if multiple devices share a GenPD, and if one device moves
> >>>> the GenPD to HW mode, the other device won't be aware of it and second
> >>>> device's dev_gpd_data(dev)->hw_mode will still be false.
> >>>>
> >>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign
> >>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev()
> >>>> callback, consumer drivers can use this API to sync the actual HW mode
> >>>> of the GenPD.
> >>>
> >>> Hmm, I thought the HW mode was being set on a per device basis, via
> >>> its PM domain. Did I get that wrong?
> >>>
> >>> Are you saying there could be multiple devices sharing the same PM
> >>> domain and thus also sharing the same HW mode? In that case, it sure
> >>> sounds like we have synchronization issues to deal with too.
> >>>
> >>
> >> Sorry my bad, currently we don't have usecase where multiple devices
> >> sharing the same PM domain that have HW control support, so there is no
> >> synchronization issue.
> >
> > Okay, good!
> >
> >>
> >> But it would be good to have .get_hwmode_dev() callback for consumer
> >> drivers to query the actual GenPD mode from HW, whenever they require it.
> >
> > Okay, no objection from my side.
> >
> > Then the final question is if we need a variable to keep a cache of
> > the current HW mode for each device. Perhaps we should start simple
> > and just always invoke the callbacks from genpd, what do you think?
> >
>
> Yes, agree, we can remove the variable and just always invoke the
> callbacks from genpd. But we may need the variable to reflect GenPD
> mode in debugfs genpd_summary, or need to invoke get callback there as
> well to get the current mode.

Hmm, after some more thinking I believe it may be best to keep the
variable after all. For reasons you point out above.

However, we need a way to synchronize the initial HW mode state for a
device. Therefore I suggest we invoke the ->get_hwmode_dev() callback
from genpd_add_device() and store its return value in the variable.
Later the variable can be used for debugfs and returned from
dev_pm_genpd_get_hwmode() too.

That should work, right?

Kind regards
Uffe