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
