Received-SPF: pass (google.com: domain of linux-kernel+bounces-63564-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <87b7967f-d8c4-426e-92ed-5a418c702481@quicinc.com>
Date: Tue, 13 Feb 2024 18:40:05 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/5] PM: domains: Allow devices attached to genpd to be
 managed by HW
Content-Language: en-US
To: Ulf Hansson <ulf.hansson@linaro.org>,
        Bjorn Andersson
	<andersson@kernel.org>,
        Abel Vesa <abel.vesa@linaro.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>,
        Kevin Hilman
	<khilman@kernel.org>, Pavel Machek <pavel@ucw.cz>,
        Len Brown
	<len.brown@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Andy
 Gross" <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        "Michael Turquette" <mturquette@baylibre.com>,
        Stephen Boyd
	<sboyd@kernel.org>,
        Stanimir Varbanov <stanimir.k.varbanov@gmail.com>,
        Vikash
 Garodia <quic_vgarodia@quicinc.com>,
        Bryan O'Donoghue
	<bryan.odonoghue@linaro.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Taniya Das <quic_tdas@quicinc.com>,
        Dmitry Baryshkov
	<dmitry.baryshkov@linaro.org>,
        <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <linux-media@vger.kernel.org>
References: <20240122-gdsc-hwctrl-v4-0-9061e8a7aa07@linaro.org>
 <20240122-gdsc-hwctrl-v4-1-9061e8a7aa07@linaro.org>
 <tax3c6o5qjegy6tv3zbgrd5rencfvypr3zg7twxfrmdngscp74@n44ei3q63g64>
 <CAPDyKFpdtrWbzNksLoY++aOY7Ltyt1HhtLZo8bj8sQ05-4Sq0g@mail.gmail.com>
 <l7icfezpajren25545n4cjtqehhividt5b2dxnxgetdsshc3k3@tdws423qdblk>
 <CAPDyKFp1vg2+-pHJ_idkdhb_zZUMpq7W17DnCCGj0eTwd4jFbQ@mail.gmail.com>
From: Jagadeesh Kona <quic_jkona@quicinc.com>
In-Reply-To: <CAPDyKFp1vg2+-pHJ_idkdhb_zZUMpq7W17DnCCGj0eTwd4jFbQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.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@kernel.org> wrote:
>>>>
>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
>>>>> From: Ulf Hansson <ulf.hansson@linaro.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@linaro.org>
>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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.

Thanks,
Jagadeesh

> Kind regards
> Uffe