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

Commit Message

Abel Vesa Jan. 22, 2024, 8:47 a.m. UTC

From: Ulf Hansson <[e-mail]>

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 <[e-mail]>
Signed-off-by: Abel Vesa <[e-mail]>
Reviewed-by: Dmitry Baryshkov <[e-mail]>
---
 drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h | 17 ++++++++++++
 2 files changed, 86 insertions(+)

Comments

Ulf Hansson Jan. 23, 2024, 12:53 p.m. UTC | #1

[...]

> +
> +/**
> + * dev_pm_genpd_get_hwmode - Get the HW mode setting for the device.
> + *
> + * @dev: Device for which the current HW-mode setting should be fetched.
> + *
> + * This helper function allows consumer drivers to fetch the current HW mode
> + * setting of its the device.
> + *
> + * It is assumed that the users guarantee that the genpd wouldn't be detached
> + * while this routine is getting called.
> + */
> +bool dev_pm_genpd_get_hwmode(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       genpd = dev_to_genpd_safe(dev);
> +       if (!genpd)
> +               return false;
> +
> +       if (genpd->get_hwmode_dev)
> +               return genpd->get_hwmode_dev(genpd, dev);

Not sure why I haven't spotted this before - but we should probably
assign dev_gpd_data(dev)->hw_mode here, rather than returning the
result from the callback directly.

> +
> +       return dev_gpd_data(dev)->hw_mode;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);

[...]

Kind regards
Uffe

Bjorn Andersson Jan. 31, 2024, 1:09 a.m. UTC | #2

On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> From: Ulf Hansson <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Abel Vesa <[e-mail]>
> Reviewed-by: Dmitry Baryshkov <[e-mail]>
> ---
>  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

> + *
> + * @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.

Regards,
Bjorn

Ulf Hansson Jan. 31, 2024, 12:12 p.m. UTC | #3

On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
>
> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> > From: Ulf Hansson <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Abel Vesa <[e-mail]>
> > Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > ---
> >  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

Bjorn Andersson Feb. 1, 2024, 11:51 p.m. UTC | #4

On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
> >
> > On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> > > From: Ulf Hansson <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Abel Vesa <[e-mail]>
> > > Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > ---
> > >  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:

> >
> > > + *
> > > + * @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.

> 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.

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.

Regards,
Bjorn

Ulf Hansson Feb. 2, 2024, 12:29 p.m. UTC | #5

On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
>
> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> > On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
> > >
> > > On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> > > > From: Ulf Hansson <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Abel Vesa <[e-mail]>
> > > > Reviewed-by: Dmitry Baryshkov <[e-mail]>
> > > > ---
> > > >  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?

Kind regards
Uffe

Jagadeesh Kona Feb. 13, 2024, 1:10 p.m. UTC | #6

On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
>>
>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
>>>>
>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
>>>>> From: Ulf Hansson <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Abel Vesa <[e-mail]>
>>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
>>>>> ---
>>>>>   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

Ulf Hansson Feb. 13, 2024, 1:51 p.m. UTC | #7

On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <[e-mail]> wrote:
>
>
>
> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> > On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
> >>
> >> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> >>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
> >>>>
> >>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> >>>>> From: Ulf Hansson <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>> Signed-off-by: Abel Vesa <[e-mail]>
> >>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >>>>> ---
> >>>>>   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.

Kind regards
Uffe

Jagadeesh Kona Feb. 14, 2024, 4:29 a.m. UTC | #8

On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <[e-mail]> wrote:
>>
>>
>>
>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
>>>>
>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
>>>>>>
>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
>>>>>>> From: Ulf Hansson <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>> Signed-off-by: Abel Vesa <[e-mail]>
>>>>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
>>>>>>> ---
>>>>>>>    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.

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.

Thanks,
Jagadeesh

Ulf Hansson Feb. 15, 2024, 4:27 p.m. UTC | #9

On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <[e-mail]> wrote:
>
>
>
> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> > On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> >>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
> >>>>
> >>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> >>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> >>>>>>> From: Ulf Hansson <[e-mail]>
> >>>>>>>
> >>>>>>> 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 <[e-mail]>
> >>>>>>> Signed-off-by: Abel Vesa <[e-mail]>
> >>>>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >>>>>>> ---
> >>>>>>>    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?

Kind regards
Uffe

Jagadeesh Kona Feb. 16, 2024, 8 a.m. UTC | #10

On 2/15/2024 9:57 PM, Ulf Hansson wrote:
> On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <[e-mail]> wrote:
>>
>>
>>
>> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
>>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
>>>>>>
>>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
>>>>>>>>> From: Ulf Hansson <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>> Signed-off-by: Abel Vesa <[e-mail]>
>>>>>>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>     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.

Thanks,
Jagadeesh

> Kind regards
> Uffe

Ulf Hansson Feb. 28, 2024, 2:53 p.m. UTC | #11

On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <[e-mail]> wrote:
>
>
>
> On 2/15/2024 9:57 PM, Ulf Hansson wrote:
> > On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
> >>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <[e-mail]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
> >>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
> >>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> >>>>>>>>> From: Ulf Hansson <[e-mail]>
> >>>>>>>>>
> >>>>>>>>> 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 <[e-mail]>
> >>>>>>>>> Signed-off-by: Abel Vesa <[e-mail]>
> >>>>>>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>>     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

Jagadeesh Kona March 1, 2024, 11:24 a.m. UTC | #12

On 2/28/2024 8:23 PM, Ulf Hansson wrote:
> On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <[e-mail]> wrote:
>>
>>
>>
>> On 2/15/2024 9:57 PM, Ulf Hansson wrote:
>>> On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 2/13/2024 7:21 PM, Ulf Hansson wrote:
>>>>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <[e-mail]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote:
>>>>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote:
>>>>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
>>>>>>>>>>> From: Ulf Hansson <[e-mail]>
>>>>>>>>>>>
>>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>>> Signed-off-by: Abel Vesa <[e-mail]>
>>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>      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

Patch

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.
+ *
+ * @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)
+		goto out;
+
+	ret = genpd->set_hwmode_dev(genpd, dev, enable);
+	if (!ret)
+		dev_gpd_data(dev)->hw_mode = enable;
+
+out:
+	genpd_unlock(genpd);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_set_hwmode);
+
+/**
+ * dev_pm_genpd_get_hwmode - Get the HW mode setting for the device.
+ *
+ * @dev: Device for which the current HW-mode setting should be fetched.
+ *
+ * This helper function allows consumer drivers to fetch the current HW mode
+ * setting of its the device.
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ */
+bool dev_pm_genpd_get_hwmode(struct device *dev)
+{
+	struct generic_pm_domain *genpd;
+
+	genpd = dev_to_genpd_safe(dev);
+	if (!genpd)
+		return false;
+
+	if (genpd->get_hwmode_dev)
+		return genpd->get_hwmode_dev(genpd, dev);
+
+	return dev_gpd_data(dev)->hw_mode;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);
+
 static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
 	unsigned int state_idx = genpd->state_idx;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b97c5e9820f9..5a26423a7ee1 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -148,6 +148,10 @@  struct generic_pm_domain {
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
 	struct gpd_dev_ops dev_ops;
+	int (*set_hwmode_dev)(struct generic_pm_domain *domain,
+			      struct device *dev, bool enable);
+	bool (*get_hwmode_dev)(struct generic_pm_domain *domain,
+			      struct device *dev);
 	int (*attach_dev)(struct generic_pm_domain *domain,
 			  struct device *dev);
 	void (*detach_dev)(struct generic_pm_domain *domain,
@@ -210,6 +214,7 @@  struct generic_pm_domain_data {
 	unsigned int performance_state;
 	unsigned int default_pstate;
 	unsigned int rpm_pstate;
+	bool hw_mode;
 	void *data;
 };
 
@@ -239,6 +244,8 @@  int dev_pm_genpd_remove_notifier(struct device *dev);
 void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
 ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
 void dev_pm_genpd_synced_poweroff(struct device *dev);
+int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
+bool dev_pm_genpd_get_hwmode(struct device *dev);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -307,6 +314,16 @@  static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
 static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
 { }
 
+static inline int dev_pm_genpd_set_hwmode(struct device *dev, bool enable)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline bool dev_pm_genpd_get_hwmode(struct device *dev)
+{
+	return false;
+}
+
 #define simple_qos_governor		(*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
 #endif