Message-ID: <19bcfe92-8c5a-4736-828a-6680ad81463c@quicinc.com>
Date: Fri, 1 Mar 2024 16:54:06 +0530
From: Jagadeesh Kona <quic_jkona@...cinc.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Bjorn Andersson <andersson@...nel.org>, Abel Vesa <abel.vesa@...aro.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Michael Turquette
	<mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Stanimir Varbanov
	<stanimir.k.varbanov@...il.com>,
        Vikash Garodia <quic_vgarodia@...cinc.com>,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Mauro Carvalho Chehab
	<mchehab@...nel.org>,
        Taniya Das <quic_tdas@...cinc.com>,
        Dmitry Baryshkov
	<dmitry.baryshkov@...aro.org>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-media@...r.kernel.org>
Subject: Re: [PATCH v4 1/5] PM: domains: Allow devices attached to genpd to be
 managed by HW



On 2/28/2024 8:23 PM, Ulf Hansson wrote:
> On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <quic_jkona@...cinc.com> wrote:
>>
>>
>>
>> On 2/15/2024 9:57 PM, Ulf Hansson wrote:
>>> On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@...cinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
>>>>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@...cinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>>>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@...nel.org> 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@...nel.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
>>>>>>>>>>> From: Ulf Hansson <ulf.hansson@...aro.org>
>>>>>>>>>>>
>>>>>>>>>>> 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@...aro.org>
>>>>>>>>>>> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
>>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>      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?
> 

Yes, it should work.

Thanks,
Jagadeesh

> Kind regards
> Uffe

Powered by blists - more mailing lists