[PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

dev_pm_opp_attach_genpd() allows attaching an arbitrary number of
power domains to an OPP table. In that case, the genpd core will
create a virtual device for each of the power domains.

At the moment, the OPP core only calls
dev_pm_genpd_set_performance_state() on these virtual devices.
It does not attempt to power on the power domains. Therefore
the required power domain might never get turned on.

So far, dev_pm_opp_attach_genpd() is only used in qcom-cpufreq-nvmem.c
to attach the CPR power domain to the CPU OPP table. The CPR driver
does not check if it was actually powered on so this did not cause
any problems. However, other drivers (e.g. rpmpd) might ignore the
performance state until the power domain is actually powered on.

Since these virtual devices are managed exclusively by the OPP core,
I would say that it should also be responsible to ensure they are
enabled.

This commit implements this similar to the non-virtual power domains;
we create device links for each of attached power domains so that they
are turned on whenever the consumer device is active.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Related discussion: https://lore.kernel.org/linux-arm-msm/20200426123140.GA190483@gerhold.net/

v1: https://lore.kernel.org/linux-pm/20200730080146.25185-4-stephan@gerhold.net/
Changes in v2:
  - Use device links instead of enabling the power domains in
    dev_pm_opp_set_rate()
---
 drivers/opp/core.c | 34 +++++++++++++++++++++++++++++++++-
 drivers/opp/opp.h  |  1 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 8b3c3986f589..7e53a7b94c59 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include "opp.h"
@@ -1964,10 +1965,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 		if (!opp_table->genpd_virt_devs[index])
 			continue;
 
+		if (opp_table->genpd_virt_links && opp_table->genpd_virt_links[index])
+			device_link_del(opp_table->genpd_virt_links[index]);
 		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
 		opp_table->genpd_virt_devs[index] = NULL;
 	}
 
+	kfree(opp_table->genpd_virt_links);
 	kfree(opp_table->genpd_virt_devs);
 	opp_table->genpd_virt_devs = NULL;
 }
@@ -1999,8 +2003,10 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 {
 	struct opp_table *opp_table;
 	struct device *virt_dev;
-	int index = 0, ret = -EINVAL;
+	struct device_link *dev_link;
+	int index = 0, ret = -EINVAL, num_devs;
 	const char **name = names;
+	u32 flags;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
 	if (IS_ERR(opp_table))
@@ -2049,6 +2055,32 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 		name++;
 	}
 
+	/* Create device links to enable the power domains when necessary */
+	opp_table->genpd_virt_links = kcalloc(opp_table->required_opp_count,
+					      sizeof(*opp_table->genpd_virt_links),
+					      GFP_KERNEL);
+	if (!opp_table->genpd_virt_links)
+		goto err;
+
+	/* Turn on power domain initially if consumer is active */
+	pm_runtime_get_noresume(dev);
+	flags = DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS;
+	if (pm_runtime_active(dev))
+		flags |= DL_FLAG_RPM_ACTIVE;
+
+	num_devs = index;
+	for (index = 0; index < num_devs; index++) {
+		dev_link = device_link_add(dev, opp_table->genpd_virt_devs[index],
+					   flags);
+		if (!dev_link) {
+			dev_err(dev, "Failed to create device link\n");
+			pm_runtime_put(dev);
+			goto err;
+		}
+		opp_table->genpd_virt_links[index] = dev_link;
+	}
+	pm_runtime_put(dev);
+
 	if (virt_devs)
 		*virt_devs = opp_table->genpd_virt_devs;
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 78e876ec803e..be5526cdbdba 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -186,6 +186,7 @@ struct opp_table {
 
 	struct mutex genpd_virt_dev_lock;
 	struct device **genpd_virt_devs;
+	struct device_link **genpd_virt_links;
 	struct opp_table **required_opp_tables;
 	unsigned int required_opp_count;
 
-- 
2.28.0

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 26-08-20, 11:33, Stephan Gerhold wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 8b3c3986f589..7e53a7b94c59 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -17,6 +17,7 @@
>  #include <linux/device.h>
>  #include <linux/export.h>
>  #include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include "opp.h"
> @@ -1964,10 +1965,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
>  		if (!opp_table->genpd_virt_devs[index])
>  			continue;
>  
> +		if (opp_table->genpd_virt_links && opp_table->genpd_virt_links[index])
> +			device_link_del(opp_table->genpd_virt_links[index]);
>  		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
>  		opp_table->genpd_virt_devs[index] = NULL;
>  	}
>  
> +	kfree(opp_table->genpd_virt_links);
>  	kfree(opp_table->genpd_virt_devs);
>  	opp_table->genpd_virt_devs = NULL;
>  }
> @@ -1999,8 +2003,10 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>  {
>  	struct opp_table *opp_table;
>  	struct device *virt_dev;
> -	int index = 0, ret = -EINVAL;
> +	struct device_link *dev_link;
> +	int index = 0, ret = -EINVAL, num_devs;
>  	const char **name = names;
> +	u32 flags;
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
>  	if (IS_ERR(opp_table))
> @@ -2049,6 +2055,32 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,

I was about to apply this patch when I noticed that this routine does
return the array of virtual devices back to the caller, like the qcom
cpufreq driver in this case. IIRC we did it this way as a generic
solution for this in the OPP core wasn't preferable.

And so I think again if this patch should be picked instead of letting
the platform handle this ?

-- 
viresh

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Thu, Aug 27, 2020 at 03:31:04PM +0530, Viresh Kumar wrote:
> On 26-08-20, 11:33, Stephan Gerhold wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 8b3c3986f589..7e53a7b94c59 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> >  #include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >  
> >  #include "opp.h"
> > @@ -1964,10 +1965,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
> >  		if (!opp_table->genpd_virt_devs[index])
> >  			continue;
> >  
> > +		if (opp_table->genpd_virt_links && opp_table->genpd_virt_links[index])
> > +			device_link_del(opp_table->genpd_virt_links[index]);
> >  		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
> >  		opp_table->genpd_virt_devs[index] = NULL;
> >  	}
> >  
> > +	kfree(opp_table->genpd_virt_links);
> >  	kfree(opp_table->genpd_virt_devs);
> >  	opp_table->genpd_virt_devs = NULL;
> >  }
> > @@ -1999,8 +2003,10 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> >  {
> >  	struct opp_table *opp_table;
> >  	struct device *virt_dev;
> > -	int index = 0, ret = -EINVAL;
> > +	struct device_link *dev_link;
> > +	int index = 0, ret = -EINVAL, num_devs;
> >  	const char **name = names;
> > +	u32 flags;
> >  
> >  	opp_table = dev_pm_opp_get_opp_table(dev);
> >  	if (IS_ERR(opp_table))
> > @@ -2049,6 +2055,32 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> 
> I was about to apply this patch when I noticed that this routine does
> return the array of virtual devices back to the caller, like the qcom
> cpufreq driver in this case. IIRC we did it this way as a generic
> solution for this in the OPP core wasn't preferable.
> 

Hmm. Actually I was using this parameter for initial testing, and forced
on the power domains from the qcom-cpufreq-nvmem driver. For my v1 patch
I wanted to enable the power domains in dev_pm_opp_set_rate(), so there
using the virt_devs parameter was not possible.

On the other hand, creating the device links would be possible from the
platform driver by using the parameter.

> And so I think again if this patch should be picked instead of letting
> the platform handle this ?
> 

It seems like originally the motivation for the parameter was that
cpufreq consumers do *not* need to power on the power domains:

Commit 17a8f868ae3e ("opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()"):
 "The cpufreq drivers don't need to do runtime PM operations on
  the virtual devices returned by dev_pm_domain_attach_by_name() and so
  the virtual devices weren't shared with the callers of
  dev_pm_opp_attach_genpd() earlier.

  But the IO device drivers would want to do that. This patch updates
  the prototype of dev_pm_opp_attach_genpd() to accept another argument
  to return the pointer to the array of genpd virtual devices."

But the reason why I made this patch is that actually something *should*
enable the power domains even for the cpufreq case.
If every user of dev_pm_opp_attach_genpd() ends up creating these device
links we might as well manage those directly from the OPP core.

I cannot think of any use case where you would not want to manage those
power domains using device links. And if there is such a use case,
chances are good that this use case is so special that using the OPP API
to set the performance states would not work either. In either case,
this seems like something that should be discussed once there is such a
use case.

At the moment, there are only two users of dev_pm_opp_attach_genpd():

  - cpufreq (qcom-cpufreq-nvmem)
  - I/O (venus, recently added in linux-next [1])

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=9a538b83612c8b5848bf840c2ddcd86dda1c8c76

In fact, venus adds the device link exactly the same way as in my patch.
So we could move that to the OPP core, simplify the venus code and
remove the virt_devs parameter. That would be my suggestion.

I can submit a v3 with that if you agree (or we take this patch as-is
and remove the parameter separately - I just checked and creating a
device link twice does not seem to cause any problems...)

Thanks!
Stephan

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 27-08-20, 13:44, Stephan Gerhold wrote:
> Hmm. Actually I was using this parameter for initial testing, and forced
> on the power domains from the qcom-cpufreq-nvmem driver. For my v1 patch
> I wanted to enable the power domains in dev_pm_opp_set_rate(), so there
> using the virt_devs parameter was not possible.

Right, as we really do not want to enable it there and leave it for
the real consumers to handle.

> On the other hand, creating the device links would be possible from the
> platform driver by using the parameter.

Right.

> > And so I think again if this patch should be picked instead of letting
> > the platform handle this ?
> 
> It seems like originally the motivation for the parameter was that
> cpufreq consumers do *not* need to power on the power domains:
> 
> Commit 17a8f868ae3e ("opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()"):
>  "The cpufreq drivers don't need to do runtime PM operations on
>   the virtual devices returned by dev_pm_domain_attach_by_name() and so
>   the virtual devices weren't shared with the callers of
>   dev_pm_opp_attach_genpd() earlier.
> 
>   But the IO device drivers would want to do that. This patch updates
>   the prototype of dev_pm_opp_attach_genpd() to accept another argument
>   to return the pointer to the array of genpd virtual devices."

Not just that I believe. There were also arguments that only the real
consumer knows how to handle multiple power domains. For example for a
USB or Camera module which can work in multiple modes, we may want to
enable only one power domain in say slow mode and another power domain
in fast mode. And so these kind of complex behavior/choices better be
left for the end consumer and not try to handle this generically in
the OPP core.

> But the reason why I made this patch is that actually something *should*
> enable the power domains even for the cpufreq case.

Ulf, what do you think about this ? IIRC from our previous discussions
someone asked me not do so.

> If every user of dev_pm_opp_attach_genpd() ends up creating these device
> links we might as well manage those directly from the OPP core.

Sure, I am all in for reducing code duplication, but ...

> I cannot think of any use case where you would not want to manage those
> power domains using device links. And if there is such a use case,
> chances are good that this use case is so special that using the OPP API
> to set the performance states would not work either. In either case,
> this seems like something that should be discussed once there is such a
> use case.

The example I gave earlier shows a common case where we need to handle
this at the end users which still want to use the OPP API.

> At the moment, there are only two users of dev_pm_opp_attach_genpd():
> 
>   - cpufreq (qcom-cpufreq-nvmem)
>   - I/O (venus, recently added in linux-next [1])
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=9a538b83612c8b5848bf840c2ddcd86dda1c8c76
> 
> In fact, venus adds the device link exactly the same way as in my patch.
> So we could move that to the OPP core, simplify the venus code and
> remove the virt_devs parameter. That would be my suggestion.
> 
> I can submit a v3 with that if you agree (or we take this patch as-is
> and remove the parameter separately - I just checked and creating a
> device link twice does not seem to cause any problems...)

I normally tend to agree with the logic that lets only focus on what's
upstream and not think of virtual cases which may never happen. But I
was told that this is too common of a scenario and so it made sense to
do it this way.

Maybe Ulf can again throw some light here :)

-- 
viresh

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Ulf Hansson]

+ Daniel

> > Commit 17a8f868ae3e ("opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()"):
> >  "The cpufreq drivers don't need to do runtime PM operations on
> >   the virtual devices returned by dev_pm_domain_attach_by_name() and so
> >   the virtual devices weren't shared with the callers of
> >   dev_pm_opp_attach_genpd() earlier.
> >
> >   But the IO device drivers would want to do that. This patch updates
> >   the prototype of dev_pm_opp_attach_genpd() to accept another argument
> >   to return the pointer to the array of genpd virtual devices."
>
> Not just that I believe. There were also arguments that only the real
> consumer knows how to handle multiple power domains. For example for a
> USB or Camera module which can work in multiple modes, we may want to
> enable only one power domain in say slow mode and another power domain
> in fast mode. And so these kind of complex behavior/choices better be
> left for the end consumer and not try to handle this generically in
> the OPP core.
>
> > But the reason why I made this patch is that actually something *should*
> > enable the power domains even for the cpufreq case.
>
> Ulf, what do you think about this ? IIRC from our previous discussions
> someone asked me not do so.

Yes, that's correct, I recall that now as well.

In some cases I have been told that, depending on the running use
case, one of the PM domains could stay off while the other needed to
be on. I was trying to find some thread in the archive, but I failed.
Sorry.

>
> > If every user of dev_pm_opp_attach_genpd() ends up creating these device
> > links we might as well manage those directly from the OPP core.
>
> Sure, I am all in for reducing code duplication, but ...
>
> > I cannot think of any use case where you would not want to manage those
> > power domains using device links. And if there is such a use case,
> > chances are good that this use case is so special that using the OPP API
> > to set the performance states would not work either. In either case,
> > this seems like something that should be discussed once there is such a
> > use case.
>
> The example I gave earlier shows a common case where we need to handle
> this at the end users which still want to use the OPP API.
>
> > At the moment, there are only two users of dev_pm_opp_attach_genpd():
> >
> >   - cpufreq (qcom-cpufreq-nvmem)
> >   - I/O (venus, recently added in linux-next [1])
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=9a538b83612c8b5848bf840c2ddcd86dda1c8c76
> >
> > In fact, venus adds the device link exactly the same way as in my patch.
> > So we could move that to the OPP core, simplify the venus code and
> > remove the virt_devs parameter. That would be my suggestion.
> >
> > I can submit a v3 with that if you agree (or we take this patch as-is
> > and remove the parameter separately - I just checked and creating a
> > device link twice does not seem to cause any problems...)
>
> I normally tend to agree with the logic that lets only focus on what's
> upstream and not think of virtual cases which may never happen. But I
> was told that this is too common of a scenario and so it made sense to
> do it this way.
>
> Maybe Ulf can again throw some light here :)

There is another series that is being discussed [1], which could be
used by the consumer driver to help manage the device links. Maybe
that is the way we should go, to leave room for flexibility.

[1]
[PATCH v3 0/2] Introduce multi PM domains helpers
https://www.spinics.net/lists/kernel/msg3565672.html

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Fri, Aug 28, 2020 at 12:05:11PM +0530, Viresh Kumar wrote:
> On 27-08-20, 13:44, Stephan Gerhold wrote:
> > Hmm. Actually I was using this parameter for initial testing, and forced
> > on the power domains from the qcom-cpufreq-nvmem driver. For my v1 patch
> > I wanted to enable the power domains in dev_pm_opp_set_rate(), so there
> > using the virt_devs parameter was not possible.
> 
> Right, as we really do not want to enable it there and leave it for
> the real consumers to handle.
> 
> > On the other hand, creating the device links would be possible from the
> > platform driver by using the parameter.
> 
> Right.
> 
> > > And so I think again if this patch should be picked instead of letting
> > > the platform handle this ?
> > 
> > It seems like originally the motivation for the parameter was that
> > cpufreq consumers do *not* need to power on the power domains:
> > 
> > Commit 17a8f868ae3e ("opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()"):
> >  "The cpufreq drivers don't need to do runtime PM operations on
> >   the virtual devices returned by dev_pm_domain_attach_by_name() and so
> >   the virtual devices weren't shared with the callers of
> >   dev_pm_opp_attach_genpd() earlier.
> > 
> >   But the IO device drivers would want to do that. This patch updates
> >   the prototype of dev_pm_opp_attach_genpd() to accept another argument
> >   to return the pointer to the array of genpd virtual devices."
> 
> Not just that I believe. There were also arguments that only the real
> consumer knows how to handle multiple power domains. For example for a
> USB or Camera module which can work in multiple modes, we may want to
> enable only one power domain in say slow mode and another power domain
> in fast mode. And so these kind of complex behavior/choices better be
> left for the end consumer and not try to handle this generically in
> the OPP core.
> 

I was thinking about something like that, but can you actually drop
your performance state vote for one of the power domains using
"required-opps"?

At the moment it does not seem possible. I tried adding a special OPP
using opp-level = <0> to reference that from required-opps, but the OPP
core does not allow this:

	vddcx: Not all nodes have performance state set (7: 6)
	vddcx: Failed to add OPP table for index 0: -2

So the "virt_devs" parameter would allow you to disable the power
domain, but not to drop your performance state vote (you could only vote
for the lowest OPP, not 0...)

This is why I said: "And if there is such a use case, chances are good
that this use case is so special that using the OPP API to set the
performance states would not work either."

It seems to me that there is more work needed to make such a use case
really work, but it's hard to speculate without a real example.

> > But the reason why I made this patch is that actually something *should*
> > enable the power domains even for the cpufreq case.
> 
> Ulf, what do you think about this ? IIRC from our previous discussions
> someone asked me not do so.
> 
> > If every user of dev_pm_opp_attach_genpd() ends up creating these device
> > links we might as well manage those directly from the OPP core.
> 
> Sure, I am all in for reducing code duplication, but ...
> 
> > I cannot think of any use case where you would not want to manage those
> > power domains using device links. And if there is such a use case,
> > chances are good that this use case is so special that using the OPP API
> > to set the performance states would not work either. In either case,
> > this seems like something that should be discussed once there is such a
> > use case.
> 
> The example I gave earlier shows a common case where we need to handle
> this at the end users which still want to use the OPP API.
> 
> > At the moment, there are only two users of dev_pm_opp_attach_genpd():
> > 
> >   - cpufreq (qcom-cpufreq-nvmem)
> >   - I/O (venus, recently added in linux-next [1])
> > 
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=9a538b83612c8b5848bf840c2ddcd86dda1c8c76
> > 
> > In fact, venus adds the device link exactly the same way as in my patch.
> > So we could move that to the OPP core, simplify the venus code and
> > remove the virt_devs parameter. That would be my suggestion.
> > 
> > I can submit a v3 with that if you agree (or we take this patch as-is
> > and remove the parameter separately - I just checked and creating a
> > device link twice does not seem to cause any problems...)
> 
> I normally tend to agree with the logic that lets only focus on what's
> upstream and not think of virtual cases which may never happen. But I
> was told that this is too common of a scenario and so it made sense to
> do it this way.
> 

Well, if we're talking about theoretical use cases it's even hard to
make assumptions for the CPUFreq use case.

Theoretically speaking you would assume that you can disable or at least
reduce the performance votes for a power domain used by a CPU if it is
turned off (hotplug / idle). In practice this is probably hard because
the CPU will need these resources until it's actually off, so if the CPU
enters the idle state itself we don't really have a chance to do that
from Linux.

On qcom this is handled by voting for resources controlled by a remote
processor (which knows when we go to idle), so having the power domains
always on is just what I need.
But who knows what you would need on other platforms...

Honestly, I don't know what would be best in this case. I suppose if in
doubt we should rather duplicate some code for now and de-duplicate later
when we (might) know more about other use cases.

FWIW, I think my original motivation to enable the power domains from
the OPP core was that I wanted to suggest making it possible for the OPP
core to manage power domains without involving the consumer driver.

For example, a device tree property like "required-power-domains":

	cpu-opp-table {
		compatible = "operating-points-v2";
		required-power-domains = "mx";

		opp-998400000 {
			opp-hz = /bits/ 64 <998400000>;
			required-opps = <&rpmpd_opp_super_turbo>;
		};
	};

Main motivation for this change is something I mentioned a while ago [1]:
Since the order and names of the power domains is currently hardcoded
in the consumer driver, the device tree does not give any hint
which power domain we are actually controlling with the "required-opps".

Plus, with the current approach it would be quite hard to add new power
domains to an OPP table, or even to change their order. Not sure if this
would really happen that often, though.

But with all the discussions about whether it's a good idea to let the
OPP core enable the power domains or not, I'm not sure if this is really
something that would work properly...

Well, I wanted to mention it :)

Thanks!
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20200526205403.GA7256@gerhold.net/

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

Hi Viresh,

On Fri, Aug 28, 2020 at 11:57:28AM +0200, Stephan Gerhold wrote:
> On Fri, Aug 28, 2020 at 12:05:11PM +0530, Viresh Kumar wrote:
> > On 27-08-20, 13:44, Stephan Gerhold wrote:
> > > Hmm. Actually I was using this parameter for initial testing, and forced
> > > on the power domains from the qcom-cpufreq-nvmem driver. For my v1 patch
> > > I wanted to enable the power domains in dev_pm_opp_set_rate(), so there
> > > using the virt_devs parameter was not possible.
> > 
> > Right, as we really do not want to enable it there and leave it for
> > the real consumers to handle.
> > 
> > > On the other hand, creating the device links would be possible from the
> > > platform driver by using the parameter.
> > 
> > Right.
> > 
> > > > And so I think again if this patch should be picked instead of letting
> > > > the platform handle this ?
> > > 
> > > It seems like originally the motivation for the parameter was that
> > > cpufreq consumers do *not* need to power on the power domains:
> > > 
> > > Commit 17a8f868ae3e ("opp: Return genpd virtual devices from dev_pm_opp_attach_genpd()"):
> > >  "The cpufreq drivers don't need to do runtime PM operations on
> > >   the virtual devices returned by dev_pm_domain_attach_by_name() and so
> > >   the virtual devices weren't shared with the callers of
> > >   dev_pm_opp_attach_genpd() earlier.
> > > 
> > >   But the IO device drivers would want to do that. This patch updates
> > >   the prototype of dev_pm_opp_attach_genpd() to accept another argument
> > >   to return the pointer to the array of genpd virtual devices."
> > 
> > Not just that I believe. There were also arguments that only the real
> > consumer knows how to handle multiple power domains. For example for a
> > USB or Camera module which can work in multiple modes, we may want to
> > enable only one power domain in say slow mode and another power domain
> > in fast mode. And so these kind of complex behavior/choices better be
> > left for the end consumer and not try to handle this generically in
> > the OPP core.
> > 
> [...]
> 
> It seems to me that there is more work needed to make such a use case
> really work, but it's hard to speculate without a real example.
> 

So it seems like we have a real example now. :)

As mentioned in my other mail [1] it turns out I actually have such a
use case. I briefly explained it in [2], basically the clock that
provides higher CPU frequencies has some voltage requirements that
should be voted for using a power domain.

The clock that provides the lower CPU frequencies has no such
requirement, so I need to scale (and power on) a power domain only for
some of the OPPs.

[1]: https://lore.kernel.org/linux-pm/20200831154938.GA33622@gerhold.net/
[2]: https://lore.kernel.org/linux-arm-msm/20200910162610.GA7008@gerhold.net/

So I think it would be good to discuss this use case first before we
decide on this patch (how to enable power domains managed by the OPP
core). I think there are two problems that need to be solved:

1. How can we drop our performance state votes for some of the OPPs?
   I explained that problem earlier already:

> 
> I was thinking about something like that, but can you actually drop
> your performance state vote for one of the power domains using
> "required-opps"?
> 
> At the moment it does not seem possible. I tried adding a special OPP
> using opp-level = <0> to reference that from required-opps, but the OPP
> core does not allow this:
> 
> 	vddcx: Not all nodes have performance state set (7: 6)
> 	vddcx: Failed to add OPP table for index 0: -2
> 
> So the "virt_devs" parameter would allow you to disable the power
> domain, but not to drop your performance state vote (you could only vote
> for the lowest OPP, not 0...)

Not sure if it makes sense but I think somehow allowing the additional
opp-level = <0> would be a simple solution. For example:

	rpmpd: power-controller {
		rpmpd_opp_table: opp-table {
			compatible = "operating-points-v2";

            /*
             * This one can be referenced to drop the performance state
             * vote within required-opps.
             */
            rpmpd_opp_none: opp0 {
                opp-level = <0>;
            };

			rpmpd_opp_retention: opp1 {
				opp-level = <1>;
			};

            /* ... */
        };
    };                

	cpu_opp_table: cpu-opp-table {
		compatible = "operating-points-v2";
		opp-shared;

        /* Power domain is only needed for frequencies >= 998 MHz */
		opp-200000000 {
			opp-hz = /bits/ 64 <200000000>;
			required-opps = <&rpmpd_opp_none>; /* = drop perf state */
		};
		opp-998400000 {
			opp-hz = /bits/ 64 <998400000>;
			required-opps = <&rpmpd_opp_svs_soc>;
		};
		opp-1209600000 {
			opp-hz = /bits/ 64 <1209600000>;
			required-opps = <&rpmpd_opp_nominal>;
		};
	};	

2. Where/when to enable the power domains: I need to enable the power
   domain whenever I have a vote for a performance state. In the example
   above the power domain should get enabled for >= 998 MHz and disabled
   otherwise.

   At least for the CPUFreq case the "virt_devs" parameter does not
   really help in this case...
   dev_pm_opp_set_rate() is called within cpufreq-dt which is supposed
   to be generic. So I can't enable the power domains myself from there.
   Even if I made a custom cpufreq driver that has control over the
   dev_pm_opp_set_rate() call - I don't really know exactly in the
   driver for which frequencies a power domain is needed.

   On the other hand, the OPP core does have that information.
   This brings me back to my PATCH v1 (where I used runtime PM functions
   instead of device links). If I modify it to enable the power domain
   whenever we have a performance state vote > 0 when setting an OPP,
   it would do exactly what I need...

   I don't think it makes sense to do performance state votes without
   enabling a power domain, so this approach sounds good to me...

What do you think?

Thanks!
Stephan

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 11-09-20, 10:34, Stephan Gerhold wrote:
> On Fri, Aug 28, 2020 at 11:57:28AM +0200, Stephan Gerhold wrote:
> > It seems to me that there is more work needed to make such a use case
> > really work, but it's hard to speculate without a real example.
> > 
> 
> So it seems like we have a real example now. :)

We told you :)

> As mentioned in my other mail [1] it turns out I actually have such a
> use case. I briefly explained it in [2], basically the clock that
> provides higher CPU frequencies has some voltage requirements that
> should be voted for using a power domain.
> 
> The clock that provides the lower CPU frequencies has no such
> requirement, so I need to scale (and power on) a power domain only for
> some of the OPPs.
> 
> [1]: https://lore.kernel.org/linux-pm/20200831154938.GA33622@gerhold.net/
> [2]: https://lore.kernel.org/linux-arm-msm/20200910162610.GA7008@gerhold.net/
> 
> So I think it would be good to discuss this use case first before we
> decide on this patch (how to enable power domains managed by the OPP
> core). I think there are two problems that need to be solved:
> 
> 1. How can we drop our performance state votes for some of the OPPs?
>    I explained that problem earlier already:
> 
> > 
> > I was thinking about something like that, but can you actually drop
> > your performance state vote for one of the power domains using
> > "required-opps"?
> > 
> > At the moment it does not seem possible. I tried adding a special OPP
> > using opp-level = <0> to reference that from required-opps, but the OPP
> > core does not allow this:
> > 
> > 	vddcx: Not all nodes have performance state set (7: 6)
> > 	vddcx: Failed to add OPP table for index 0: -2

This should fix it.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 214c1619b445..2483e765318a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2117,9 +2117,6 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
        int dest_pstate = -EINVAL;
        int i;
 
-       if (!pstate)
-               return 0;
-
        /*
         * Normally the src_table will have the "required_opps" property set to
         * point to one of the OPPs in the dst_table, but in some cases the
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e72753be7dc7..1a9cb96484bb 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -842,7 +842,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 {
        struct device_node *np;
-       int ret, count = 0, pstate_count = 0;
+       int ret, count = 0;
        struct dev_pm_opp *opp;
 
        /* OPP table is already initialized for the device */
@@ -876,20 +876,13 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
                goto remove_static_opp;
        }
 
-       list_for_each_entry(opp, &opp_table->opp_list, node)
-               pstate_count += !!opp->pstate;
-
-       /* Either all or none of the nodes shall have performance state set */
-       if (pstate_count && pstate_count != count) {
-               dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
-                       count, pstate_count);
-               ret = -ENOENT;
-               goto remove_static_opp;
+       list_for_each_entry(opp, &opp_table->opp_list, node) {
+               if (opp->pstate) {
+                       opp_table->genpd_performance_state = true;
+                       break;
+               }
        }
 
-       if (pstate_count)
-               opp_table->genpd_performance_state = true;
-
        return 0;
 
 remove_static_opp:


> Not sure if it makes sense but I think somehow allowing the additional
> opp-level = <0> would be a simple solution. For example:
> 
> 	rpmpd: power-controller {
> 		rpmpd_opp_table: opp-table {
> 			compatible = "operating-points-v2";
> 
>             /*
>              * This one can be referenced to drop the performance state
>              * vote within required-opps.
>              */
>             rpmpd_opp_none: opp0 {
>                 opp-level = <0>;
>             };
> 
> 			rpmpd_opp_retention: opp1 {
> 				opp-level = <1>;
> 			};
> 
>             /* ... */
>         };
>     };                
> 
> 	cpu_opp_table: cpu-opp-table {
> 		compatible = "operating-points-v2";
> 		opp-shared;
> 
>         /* Power domain is only needed for frequencies >= 998 MHz */
> 		opp-200000000 {
> 			opp-hz = /bits/ 64 <200000000>;
> 			required-opps = <&rpmpd_opp_none>; /* = drop perf state */
> 		};
> 		opp-998400000 {
> 			opp-hz = /bits/ 64 <998400000>;
> 			required-opps = <&rpmpd_opp_svs_soc>;
> 		};
> 		opp-1209600000 {
> 			opp-hz = /bits/ 64 <1209600000>;
> 			required-opps = <&rpmpd_opp_nominal>;
> 		};
> 	};	

Yes, makes sense.

> 2. Where/when to enable the power domains: I need to enable the power
>    domain whenever I have a vote for a performance state. In the example
>    above the power domain should get enabled for >= 998 MHz and disabled
>    otherwise.

- Why do you need to enable these power domains like this ?
- What will happen if you don't enable them at all ?
- What will happen if you enable them for ever ?

AFAIU, these are kind of virtual domains which are there just to vote in behalf
of the OS. Only if the accumulated vote is greater than zero, the actual power
domain will start consuming power. Otherwise it should be disabled.

Or is that wrong ?

>    At least for the CPUFreq case the "virt_devs" parameter does not
>    really help in this case...
>    dev_pm_opp_set_rate() is called within cpufreq-dt which is supposed
>    to be generic. So I can't enable the power domains myself from there.
>    Even if I made a custom cpufreq driver that has control over the
>    dev_pm_opp_set_rate() call - I don't really know exactly in the
>    driver for which frequencies a power domain is needed.
> 
>    On the other hand, the OPP core does have that information.
>    This brings me back to my PATCH v1 (where I used runtime PM functions
>    instead of device links). If I modify it to enable the power domain
>    whenever we have a performance state vote > 0 when setting an OPP,
>    it would do exactly what I need...
> 
>    I don't think it makes sense to do performance state votes without
>    enabling a power domain, so this approach sounds good to me...

-- 
viresh

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

On Fri, Sep 11, 2020 at 02:55:38PM +0530, Viresh Kumar wrote:
> > As mentioned in my other mail [1] it turns out I actually have such a
> > use case. I briefly explained it in [2], basically the clock that
> > provides higher CPU frequencies has some voltage requirements that
> > should be voted for using a power domain.
> > 
> > The clock that provides the lower CPU frequencies has no such
> > requirement, so I need to scale (and power on) a power domain only for
> > some of the OPPs.
> > 
> > [1]: https://lore.kernel.org/linux-pm/20200831154938.GA33622@gerhold.net/
> > [2]: https://lore.kernel.org/linux-arm-msm/20200910162610.GA7008@gerhold.net/
> > 
> > So I think it would be good to discuss this use case first before we
> > decide on this patch (how to enable power domains managed by the OPP
> > core). I think there are two problems that need to be solved:
> > 
> > 1. How can we drop our performance state votes for some of the OPPs?
> >    I explained that problem earlier already:
> > 
> > > 
> > > I was thinking about something like that, but can you actually drop
> > > your performance state vote for one of the power domains using
> > > "required-opps"?
> > > 
> > > At the moment it does not seem possible. I tried adding a special OPP
> > > using opp-level = <0> to reference that from required-opps, but the OPP
> > > core does not allow this:
> > > 
> > > 	vddcx: Not all nodes have performance state set (7: 6)
> > > 	vddcx: Failed to add OPP table for index 0: -2
> 
> This should fix it.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 214c1619b445..2483e765318a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2117,9 +2117,6 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
>         int dest_pstate = -EINVAL;
>         int i;
>  
> -       if (!pstate)
> -               return 0;
> -
>         /*
>          * Normally the src_table will have the "required_opps" property set to
>          * point to one of the OPPs in the dst_table, but in some cases the
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index e72753be7dc7..1a9cb96484bb 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -842,7 +842,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>  static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
>  {
>         struct device_node *np;
> -       int ret, count = 0, pstate_count = 0;
> +       int ret, count = 0;
>         struct dev_pm_opp *opp;
>  
>         /* OPP table is already initialized for the device */
> @@ -876,20 +876,13 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
>                 goto remove_static_opp;
>         }
>  
> -       list_for_each_entry(opp, &opp_table->opp_list, node)
> -               pstate_count += !!opp->pstate;
> -
> -       /* Either all or none of the nodes shall have performance state set */
> -       if (pstate_count && pstate_count != count) {
> -               dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
> -                       count, pstate_count);
> -               ret = -ENOENT;
> -               goto remove_static_opp;
> +       list_for_each_entry(opp, &opp_table->opp_list, node) {
> +               if (opp->pstate) {
> +                       opp_table->genpd_performance_state = true;
> +                       break;
> +               }
>         }
>  
> -       if (pstate_count)
> -               opp_table->genpd_performance_state = true;
> -
>         return 0;
>  
>  remove_static_opp:
> 

Thanks, I will test this early next week!

> > 2. Where/when to enable the power domains: I need to enable the power
> >    domain whenever I have a vote for a performance state. In the example
> >    above the power domain should get enabled for >= 998 MHz and disabled
> >    otherwise.
> 

I will answer your questions for my use case, maybe Ulf or someone else
who knows more about power domains can chime in and say more about the
general case:

> - Why do you need to enable these power domains like this ?

At the moment power domains seem to have two independent states:
they can be powered on/off and they can have a performance state.
Both seem to be managed independently, e.g. a power domain can have a
non-zero performance state set but at the same time powered off.

The OPP core only calls dev_pm_genpd_set_performance_state(). No matter
if the power domain is powered on or not this will end up in the
set_performance_state() callback of the power domain driver.

And this is where behavior varies depending on the power domain driver.
Some drivers (e.g. qcom-cpr) will set the performance state even if
powered off, others (in my case: qcom rpmpd) will ignore the performance
state until their power_on() callback is called.

In general, I would expect that a power domain should be powered on
whenever there is at least one performance state vote > 0.
This does not seem to be the case at the moment, though.

> - What will happen if you don't enable them at all ?

If I add the power domains needed for CPUFreq to the CPU OPP table,
without further changes, nothing will power on the power domains.
There is no difference for qcom-cpr, but at least rpmpd will ignore all
the performance state votes.

> - What will happen if you enable them for ever ?
> 
> AFAIU, these are kind of virtual domains which are there just to vote in behalf
> of the OS. Only if the accumulated vote is greater than zero, the actual power
> domain will start consuming power. Otherwise it should be disabled.
> 
> Or is that wrong ?
> 

Right, at least in case of rpmpd the actual power domain is managed by
some "RPM" remote processor which accumulates votes from all the CPUs
running in the SoC.

I believe the power domains in my case are even always-on in the RPM,
but still the RPM interface seems to provide a way to vote for
enabling/disabling the power domains. Sadly it is barely documented,
so I'm not sure what happens if we keep the power domains permanently
powered on. Maybe it will block certain low power modes?

The current implementation in drivers/soc/qcom/rpmpd.c does suggest that
we should vote for "power off" when the power domains are no longer
needed by Linux.

(+CC Bjorn, Andy and linux-arm-msm, maybe they know more...)

Thanks!
Stephan

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 26-08-20, 11:33, Stephan Gerhold wrote:
> dev_pm_opp_attach_genpd() allows attaching an arbitrary number of
> power domains to an OPP table. In that case, the genpd core will
> create a virtual device for each of the power domains.
> 
> At the moment, the OPP core only calls
> dev_pm_genpd_set_performance_state() on these virtual devices.
> It does not attempt to power on the power domains. Therefore
> the required power domain might never get turned on.
> 
> So far, dev_pm_opp_attach_genpd() is only used in qcom-cpufreq-nvmem.c
> to attach the CPR power domain to the CPU OPP table. The CPR driver
> does not check if it was actually powered on so this did not cause
> any problems. However, other drivers (e.g. rpmpd) might ignore the
> performance state until the power domain is actually powered on.
> 
> Since these virtual devices are managed exclusively by the OPP core,
> I would say that it should also be responsible to ensure they are
> enabled.
> 
> This commit implements this similar to the non-virtual power domains;
> we create device links for each of attached power domains so that they
> are turned on whenever the consumer device is active.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---

Applied with some changes, hope that is fine:

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
[ Viresh: Rearranged the code to remove extra loop and minor cleanup ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 37 ++++++++++++++++++++++++++++++++++++-
 drivers/opp/opp.h  |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e65174725a4d..b608b0253079 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include "opp.h"
@@ -1967,10 +1968,15 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 		if (!opp_table->genpd_virt_devs[index])
 			continue;
 
+		if (opp_table->genpd_virt_links[index])
+			device_link_del(opp_table->genpd_virt_links[index]);
+
 		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
 		opp_table->genpd_virt_devs[index] = NULL;
 	}
 
+	kfree(opp_table->genpd_virt_links);
+	opp_table->genpd_virt_links = NULL;
 	kfree(opp_table->genpd_virt_devs);
 	opp_table->genpd_virt_devs = NULL;
 }
@@ -2002,8 +2008,10 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 {
 	struct opp_table *opp_table;
 	struct device *virt_dev;
+	struct device_link *dev_link;
 	int index = 0, ret = -EINVAL;
 	const char **name = names;
+	u32 flags;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
 	if (IS_ERR(opp_table))
@@ -2030,6 +2038,21 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	if (!opp_table->genpd_virt_devs)
 		goto unlock;
 
+	opp_table->genpd_virt_links = kcalloc(opp_table->required_opp_count,
+					      sizeof(*opp_table->genpd_virt_links),
+					      GFP_KERNEL);
+	if (!opp_table->genpd_virt_links) {
+		kfree(opp_table->genpd_virt_devs);
+		opp_table->genpd_virt_devs = NULL;
+		goto unlock;
+	}
+
+	/* Turn on power domain initially if consumer is active */
+	pm_runtime_get_noresume(dev);
+	flags = DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS;
+	if (pm_runtime_active(dev))
+		flags |= DL_FLAG_RPM_ACTIVE;
+
 	while (*name) {
 		if (index >= opp_table->required_opp_count) {
 			dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
@@ -2043,12 +2066,23 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 			dev_err(dev, "Couldn't attach to pm_domain: %d\n", ret);
 			goto err;
 		}
-
 		opp_table->genpd_virt_devs[index] = virt_dev;
+
+		/* Create device links to manage runtime PM */
+		dev_link = device_link_add(dev, opp_table->genpd_virt_devs[index],
+					   flags);
+		if (!dev_link) {
+			dev_err(dev, "Failed to create device link\n");
+			goto err;
+		}
+		opp_table->genpd_virt_links[index] = dev_link;
+
 		index++;
 		name++;
 	}
 
+	pm_runtime_put(dev);
+
 	if (virt_devs)
 		*virt_devs = opp_table->genpd_virt_devs;
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
@@ -2056,6 +2090,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	return opp_table;
 
 err:
+	pm_runtime_put(dev);
 	_opp_detach_genpd(opp_table);
 unlock:
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 78e876ec803e..be5526cdbdba 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -186,6 +186,7 @@ struct opp_table {
 
 	struct mutex genpd_virt_dev_lock;
 	struct device **genpd_virt_devs;
+	struct device_link **genpd_virt_links;
 	struct opp_table **required_opp_tables;
 	unsigned int required_opp_count;

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Stephan Gerhold]

Hi Viresh,

On Mon, Aug 31, 2020 at 05:44:57PM +0530, Viresh Kumar wrote:
> On 26-08-20, 11:33, Stephan Gerhold wrote:
> > dev_pm_opp_attach_genpd() allows attaching an arbitrary number of
> > power domains to an OPP table. In that case, the genpd core will
> > create a virtual device for each of the power domains.
> > 
> > At the moment, the OPP core only calls
> > dev_pm_genpd_set_performance_state() on these virtual devices.
> > It does not attempt to power on the power domains. Therefore
> > the required power domain might never get turned on.
> > 
> > So far, dev_pm_opp_attach_genpd() is only used in qcom-cpufreq-nvmem.c
> > to attach the CPR power domain to the CPU OPP table. The CPR driver
> > does not check if it was actually powered on so this did not cause
> > any problems. However, other drivers (e.g. rpmpd) might ignore the
> > performance state until the power domain is actually powered on.
> > 
> > Since these virtual devices are managed exclusively by the OPP core,
> > I would say that it should also be responsible to ensure they are
> > enabled.
> > 
> > This commit implements this similar to the non-virtual power domains;
> > we create device links for each of attached power domains so that they
> > are turned on whenever the consumer device is active.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> 
> Applied with some changes, hope that is fine:
> 

I appreciate it, thank you! But actually after our discussion regarding
the "manage multiple power domains which might not always need to be on"
use case I would like to explore that a bit further before we decide on
a final solution that complicates changes later.

The reason I mention this is that after our discussion I have been
(again) staring at the vendor implementation of CPU frequency scaling of
the platform I'm working on (qcom msm8916). Actually there seems to be yet
another power domain that is scaled only for some CPU frequencies within
the clock driver. (The vendor driver implies this is a requirement of
the PLL clock that is used for higher CPU frequencies, but not sure...)

I don't fully understand how to implement this in mainline yet. I have
started some research on these "voltage requirements" for clocks, and
based on previous discussions [1] and patches [2] it seems like I'm
*not* supposed to add this to the clock driver, but rather as another
required-opp to the CPU OPP table.

If that is the case, we would pretty much have a situation like you
described, a power domain that should only be scaled for some of the
OPPs (the higher CPU frequencies).

But first I need to do some more research, and probably discuss how to
handle that power domain separately first. I think it would be easier if
we postpone this patch till then. I don't think anyone else needs this
patch at the moment.

[1]: https://lore.kernel.org/linux-clk/9439bd29e3ccd5424a8e9b464c8c7bd9@codeaurora.org/
[2]: https://lore.kernel.org/linux-pm/20190320094918.20234-1-rnayak@codeaurora.org/

> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> [ Viresh: Rearranged the code to remove extra loop and minor cleanup ]

FWIW, the reason I put this into an extra loop is that the
dev_pm_domain_attach_by_name() might return -EPROBE_DEFER (or some other
error) for some of the power domains. If you create the device links
before attaching all domains you might unnecessarily turn on+off some of
them.

Having it in a separate loop avoids that, because we only start powering
on the power domains when we know that all the power domains are
available.

Thanks!
Stephan

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c | 37 ++++++++++++++++++++++++++++++++++++-
>  drivers/opp/opp.h  |  1 +
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e65174725a4d..b608b0253079 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -17,6 +17,7 @@
>  #include <linux/device.h>
>  #include <linux/export.h>
>  #include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include "opp.h"
> @@ -1967,10 +1968,15 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
>  		if (!opp_table->genpd_virt_devs[index])
>  			continue;
>  
> +		if (opp_table->genpd_virt_links[index])
> +			device_link_del(opp_table->genpd_virt_links[index]);
> +
>  		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
>  		opp_table->genpd_virt_devs[index] = NULL;
>  	}
>  
> +	kfree(opp_table->genpd_virt_links);
> +	opp_table->genpd_virt_links = NULL;
>  	kfree(opp_table->genpd_virt_devs);
>  	opp_table->genpd_virt_devs = NULL;
>  }
> @@ -2002,8 +2008,10 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>  {
>  	struct opp_table *opp_table;
>  	struct device *virt_dev;
> +	struct device_link *dev_link;
>  	int index = 0, ret = -EINVAL;
>  	const char **name = names;
> +	u32 flags;
>  
>  	opp_table = dev_pm_opp_get_opp_table(dev);
>  	if (IS_ERR(opp_table))
> @@ -2030,6 +2038,21 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>  	if (!opp_table->genpd_virt_devs)
>  		goto unlock;
>  
> +	opp_table->genpd_virt_links = kcalloc(opp_table->required_opp_count,
> +					      sizeof(*opp_table->genpd_virt_links),
> +					      GFP_KERNEL);
> +	if (!opp_table->genpd_virt_links) {
> +		kfree(opp_table->genpd_virt_devs);
> +		opp_table->genpd_virt_devs = NULL;
> +		goto unlock;
> +	}
> +
> +	/* Turn on power domain initially if consumer is active */
> +	pm_runtime_get_noresume(dev);
> +	flags = DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS;
> +	if (pm_runtime_active(dev))
> +		flags |= DL_FLAG_RPM_ACTIVE;
> +
>  	while (*name) {
>  		if (index >= opp_table->required_opp_count) {
>  			dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
> @@ -2043,12 +2066,23 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>  			dev_err(dev, "Couldn't attach to pm_domain: %d\n", ret);
>  			goto err;
>  		}
> -
>  		opp_table->genpd_virt_devs[index] = virt_dev;
> +
> +		/* Create device links to manage runtime PM */
> +		dev_link = device_link_add(dev, opp_table->genpd_virt_devs[index],
> +					   flags);
> +		if (!dev_link) {
> +			dev_err(dev, "Failed to create device link\n");
> +			goto err;
> +		}
> +		opp_table->genpd_virt_links[index] = dev_link;
> +
>  		index++;
>  		name++;
>  	}
>  
> +	pm_runtime_put(dev);
> +
>  	if (virt_devs)
>  		*virt_devs = opp_table->genpd_virt_devs;
>  	mutex_unlock(&opp_table->genpd_virt_dev_lock);
> @@ -2056,6 +2090,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>  	return opp_table;
>  
>  err:
> +	pm_runtime_put(dev);
>  	_opp_detach_genpd(opp_table);
>  unlock:
>  	mutex_unlock(&opp_table->genpd_virt_dev_lock);
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 78e876ec803e..be5526cdbdba 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -186,6 +186,7 @@ struct opp_table {
>  
>  	struct mutex genpd_virt_dev_lock;
>  	struct device **genpd_virt_devs;
> +	struct device_link **genpd_virt_links;
>  	struct opp_table **required_opp_tables;
>  	unsigned int required_opp_count;

Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

[Viresh Kumar]

On 31-08-20, 17:49, Stephan Gerhold wrote:
> I appreciate it, thank you! But actually after our discussion regarding
> the "manage multiple power domains which might not always need to be on"
> use case I would like to explore that a bit further before we decide on
> a final solution that complicates changes later.
> 
> The reason I mention this is that after our discussion I have been
> (again) staring at the vendor implementation of CPU frequency scaling of
> the platform I'm working on (qcom msm8916). Actually there seems to be yet
> another power domain that is scaled only for some CPU frequencies within
> the clock driver. (The vendor driver implies this is a requirement of
> the PLL clock that is used for higher CPU frequencies, but not sure...)
> 
> I don't fully understand how to implement this in mainline yet. I have
> started some research on these "voltage requirements" for clocks, and
> based on previous discussions [1] and patches [2] it seems like I'm
> *not* supposed to add this to the clock driver, but rather as another
> required-opp to the CPU OPP table.
> 
> If that is the case, we would pretty much have a situation like you
> described, a power domain that should only be scaled for some of the
> OPPs (the higher CPU frequencies).
> 
> But first I need to do some more research, and probably discuss how to
> handle that power domain separately first. I think it would be easier if
> we postpone this patch till then. I don't think anyone else needs this
> patch at the moment.

Heh, okay, I can keep it out of my tree then :)

> [1]: https://lore.kernel.org/linux-clk/9439bd29e3ccd5424a8e9b464c8c7bd9@codeaurora.org/
> [2]: https://lore.kernel.org/linux-pm/20190320094918.20234-1-rnayak@codeaurora.org/
> 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > [ Viresh: Rearranged the code to remove extra loop and minor cleanup ]
> 
> FWIW, the reason I put this into an extra loop is that the
> dev_pm_domain_attach_by_name() might return -EPROBE_DEFER (or some other
> error) for some of the power domains. If you create the device links
> before attaching all domains you might unnecessarily turn on+off some of
> them.

Hmm, but that shouldn't have any significant side affects, right ? And
shouldn't result in some other sort of error. It makes the code look
more sensible/readable and so I would prefer to keep a single loop if
it doesn't make something not-work.

> Having it in a separate loop avoids that, because we only start powering
> on the power domains when we know that all the power domains are
> available.

-- 
viresh