The devfreq passive governor scales the frequency of a "child" device based
on the current frequency of a "parent" device (not parent/child in the
sense of device hierarchy). As of today, the passive governor requires one
of the following to work correctly:
1. The parent and child device have the same number of frequencies
2. The child device driver passes a mapping function to translate from
parent frequency to child frequency.
When (1) is not true, (2) is the only option right now. But often times,
all that is required is a simple mapping from parent's frequency to child's
frequency.
Since OPPs already support pointing to other "required-opps", add support
for using that to map from parent device frequency to child device
frequency. That way, every child device driver doesn't have to implement a
separate mapping function anytime (1) isn't true.
Some common (but not comprehensive) reason for needing a devfreq passive
governor to adjust the frequency of one device based on another are:
1. These were the combination of frequencies that were validated/screened
during the manufacturing process.
2. These are the sensible performance combinations between two devices
interacting with each other. So that when one runs fast the other
doesn't become the bottleneck.
3. Hardware bugs requiring some kind of frequency ratio between devices.
For example, the following mapping can't be captured in DT as it stands
today because the parent and child device have different number of OPPs.
But with this patch series, this mapping can be captured cleanly.
In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something
like this with the following changes:
bus_g2d_400: bus0 {
compatible = "samsung,exynos-bus";
clocks = <&cmu_top CLK_ACLK_G2D_400>;
clock-names = "bus";
operating-points-v2 = <&bus_g2d_400_opp_table>;
status = "disabled";
};
bus_noc2: bus9 {
compatible = "samsung,exynos-bus";
clocks = <&cmu_mif CLK_ACLK_BUS2_400>;
clock-names = "bus";
operating-points-v2 = <&bus_noc2_opp_table>;
status = "disabled";
};
bus_g2d_400_opp_table: opp_table2 {
compatible = "operating-points-v2";
opp-shared;
opp-400000000 {
opp-hz = /bits/ 64 <400000000>;
opp-microvolt = <1075000>;
required-opps = <&noc2_400>;
};
opp-267000000 {
opp-hz = /bits/ 64 <267000000>;
opp-microvolt = <1000000>;
required-opps = <&noc2_200>;
};
opp-200000000 {
opp-hz = /bits/ 64 <200000000>;
opp-microvolt = <975000>;
required-opps = <&noc2_200>;
};
opp-160000000 {
opp-hz = /bits/ 64 <160000000>;
opp-microvolt = <962500>;
required-opps = <&noc2_134>;
};
opp-134000000 {
opp-hz = /bits/ 64 <134000000>;
opp-microvolt = <950000>;
required-opps = <&noc2_134>;
};
opp-100000000 {
opp-hz = /bits/ 64 <100000000>;
opp-microvolt = <937500>;
required-opps = <&noc2_100>;
};
};
bus_noc2_opp_table: opp_table6 {
compatible = "operating-points-v2";
noc2_400: opp-400000000 {
opp-hz = /bits/ 64 <400000000>;
};
noc2_200: opp-200000000 {
opp-hz = /bits/ 64 <200000000>;
};
noc2_134: opp-134000000 {
opp-hz = /bits/ 64 <134000000>;
};
noc2_100: opp-100000000 {
opp-hz = /bits/ 64 <100000000>;
};
};
-Saravana
v1 -> v2:
- Cached OPP table reference in devfreq to avoid looking up every time.
- Renamed variable in passive governor to be more intuitive.
- Updated cover letter with examples.
Saravana Kannan (4):
OPP: Allow required-opps even if the device doesn't have power-domains
OPP: Add function to look up required OPP's for a given OPP
PM / devfreq: Cache OPP table reference in devfreq
PM / devfreq: Add required OPPs support to passive governor
drivers/devfreq/devfreq.c | 6 ++++
drivers/devfreq/governor_passive.c | 20 ++++++++---
drivers/opp/core.c | 56 +++++++++++++++++++++++++++++-
drivers/opp/of.c | 14 --------
include/linux/devfreq.h | 1 +
include/linux/pm_opp.h | 11 ++++++
6 files changed, 88 insertions(+), 20 deletions(-)
--
2.22.0.410.gd8fdbe21b5-goog
A Device-A can have a (minimum) performance requirement on another
Device-B to be able to function correctly. This performance requirement
on Device-B can also change based on the current performance level of
Device-A.
The existing required-opps feature fits well to describe this need. So,
instead of limiting required-opps to point to only PM-domain devices,
allow it to point to any device.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/opp/core.c | 2 +-
drivers/opp/of.c | 14 --------------
2 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..74c7bdc6f463 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
return 0;
/* Single genpd case */
- if (!genpd_virt_devs) {
+ if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
pstate = opp->required_opps[0]->pstate;
ret = dev_pm_genpd_set_performance_state(dev, pstate);
if (ret) {
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index c10c782d15aa..7c8336e94aff 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
*/
count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells");
- if (!count_pd)
- goto put_np;
-
if (count_pd > 1) {
genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
GFP_KERNEL);
@@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
if (IS_ERR(required_opp_tables[i]))
goto free_required_tables;
-
- /*
- * We only support genpd's OPPs in the "required-opps" for now,
- * as we don't know how much about other cases. Error out if the
- * required OPP doesn't belong to a genpd.
- */
- if (!required_opp_tables[i]->is_genpd) {
- dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
- required_np);
- goto free_required_tables;
- }
}
goto put_np;
--
2.22.0.410.gd8fdbe21b5-goog
Add a function that allows looking up required OPPs given a source OPP
table, destination OPP table and the source OPP.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/opp/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_opp.h | 11 +++++++++
2 files changed, 65 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 74c7bdc6f463..4f7870bffbf8 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1830,6 +1830,60 @@ void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
dev_err(virt_dev, "Failed to find required device entry\n");
}
+/**
+ * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP.
+ * @src_table: OPP table which has dst_table as one of its required OPP table.
+ * @dst_table: Required OPP table of the src_table.
+ * @pstate: OPP of the src_table.
+ *
+ * This function returns the OPP (present in @dst_table) pointed out by the
+ * "required-opps" property of the OPP (present in @src_table).
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ *
+ * Return: destination table OPP on success, otherwise NULL on errors.
+ */
+struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
+ struct opp_table *dst_table,
+ struct dev_pm_opp *src_opp)
+{
+ struct dev_pm_opp *opp, *dest_opp = NULL;
+ int i;
+
+ if (!src_table || !dst_table || !src_opp)
+ return NULL;
+
+ for (i = 0; i < src_table->required_opp_count; i++) {
+ if (src_table->required_opp_tables[i]->np == dst_table->np)
+ break;
+ }
+
+ if (unlikely(i == src_table->required_opp_count)) {
+ pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
+ __func__, src_table, dst_table);
+ return NULL;
+ }
+
+ mutex_lock(&src_table->lock);
+
+ list_for_each_entry(opp, &src_table->opp_list, node) {
+ if (opp == src_opp) {
+ dest_opp = opp->required_opps[i];
+ dev_pm_opp_get(dest_opp);
+ goto unlock;
+ }
+ }
+
+ pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
+ dst_table);
+
+unlock:
+ mutex_unlock(&src_table->lock);
+
+ return dest_opp;
+}
+
/**
* dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
* @src_table: OPP table which has dst_table as one of its required OPP table.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index b150fe97ce5a..bc5c68bdfc8d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -134,6 +134,9 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_set_genpd_virt_dev(struct device *dev, struct device *virt_dev, int index);
void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table, struct device *virt_dev);
int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
+struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
+ struct opp_table *dst_table,
+ struct dev_pm_opp *src_opp);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -307,6 +310,14 @@ static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table
return -ENOTSUPP;
}
+static inline struct dev_pm_opp *dev_pm_opp_xlate_opp(
+ struct opp_table *src_table,
+ struct opp_table *dst_table,
+ struct dev_pm_opp *src_opp)
+{
+ return NULL;
+}
+
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
return -ENOTSUPP;
--
2.22.0.410.gd8fdbe21b5-goog
The OPP table can be used often in devfreq. Trying to get it each time can
be expensive, so cache it in the devfreq struct.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/devfreq/devfreq.c | 6 ++++++
include/linux/devfreq.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 6b6991f0e873..ac62b78dc035 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
if (devfreq->profile->exit)
devfreq->profile->exit(devfreq->dev.parent);
+ if (devfreq->opp_table)
+ dev_pm_opp_put_opp_table(devfreq->opp_table);
mutex_destroy(&devfreq->lock);
kfree(devfreq);
}
@@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->max_freq = devfreq->scaling_max_freq;
devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
+ devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
+ if (IS_ERR(devfreq->opp_table))
+ devfreq->opp_table = NULL;
+
atomic_set(&devfreq->suspend_count, 0);
dev_set_name(&devfreq->dev, "devfreq%d",
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index fbffa74bfc1b..0d877c9513d7 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -156,6 +156,7 @@ struct devfreq {
struct devfreq_dev_profile *profile;
const struct devfreq_governor *governor;
char governor_name[DEVFREQ_NAME_LEN];
+ struct opp_table *opp_table;
struct notifier_block nb;
struct delayed_work work;
--
2.22.0.410.gd8fdbe21b5-goog
Look at the required OPPs of the "parent" device to determine the OPP that
is required from the slave device managed by the passive governor. This
allows having mappings between a parent device and a slave device even when
they don't have the same number of OPPs.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 3bc29acbd54e..f6de03de7a64 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -22,7 +22,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
= (struct devfreq_passive_data *)devfreq->data;
struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
unsigned long child_freq = ULONG_MAX;
- struct dev_pm_opp *opp;
+ struct dev_pm_opp *opp = NULL, *p_opp = NULL;
int i, count, ret = 0;
/*
@@ -59,13 +59,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
* list of parent device. Because in this case, *freq is temporary
* value which is decided by ondemand governor.
*/
- opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
- if (IS_ERR(opp)) {
- ret = PTR_ERR(opp);
+ p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
+ if (IS_ERR(p_opp)) {
+ ret = PTR_ERR(p_opp);
goto out;
}
- dev_pm_opp_put(opp);
+ if (devfreq->opp_table && parent_devfreq->opp_table)
+ opp = dev_pm_opp_xlate_opp(parent_devfreq->opp_table,
+ devfreq->opp_table, p_opp);
+ if (opp) {
+ *freq = dev_pm_opp_get_freq(opp);
+ dev_pm_opp_put(opp);
+ goto out;
+ }
/*
* Get the OPP table's index of decided freqeuncy by governor
@@ -92,6 +99,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
*freq = child_freq;
out:
+ if (!IS_ERR_OR_NULL(opp))
+ dev_pm_opp_put(p_opp);
+
return ret;
}
--
2.22.0.410.gd8fdbe21b5-goog
>The OPP table can be used often in devfreq. Trying to get it each time can
>be expensive, so cache it in the devfreq struct.
>
>Signed-off-by: Saravana Kannan <[email protected]>
>---
> drivers/devfreq/devfreq.c | 6 ++++++
> include/linux/devfreq.h | 1 +
> 2 files changed, 7 insertions(+)
Acked-by: MyungJoo Ham <[email protected]>
Cheers,
MyungJoo
>Look at the required OPPs of the "parent" device to determine the OPP that
>is required from the slave device managed by the passive governor. This
>allows having mappings between a parent device and a slave device even when
>they don't have the same number of OPPs.
>
>Signed-off-by: Saravana Kannan <[email protected]>
>---
> drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
Acked-by: MyungJoo Ham <[email protected]>
Cheers,
MyungJoo
Hi Saravana,
On 19. 6. 26. 오전 6:33, Saravana Kannan wrote:
> The OPP table can be used often in devfreq. Trying to get it each time can
> be expensive, so cache it in the devfreq struct.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> drivers/devfreq/devfreq.c | 6 ++++++
> include/linux/devfreq.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..ac62b78dc035 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
> if (devfreq->profile->exit)
> devfreq->profile->exit(devfreq->dev.parent);
>
> + if (devfreq->opp_table)
> + dev_pm_opp_put_opp_table(devfreq->opp_table);
> mutex_destroy(&devfreq->lock);
> kfree(devfreq);
> }
> @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->max_freq = devfreq->scaling_max_freq;
>
> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> + devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> + if (IS_ERR(devfreq->opp_table))
> + devfreq->opp_table = NULL;
> +
> atomic_set(&devfreq->suspend_count, 0);
>
> dev_set_name(&devfreq->dev, "devfreq%d",
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fbffa74bfc1b..0d877c9513d7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -156,6 +156,7 @@ struct devfreq {
> struct devfreq_dev_profile *profile;
> const struct devfreq_governor *governor;
> char governor_name[DEVFREQ_NAME_LEN];
> + struct opp_table *opp_table;
> struct notifier_block nb;
> struct delayed_work work;
>
>
Looks good to me.
Reviewed-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Saravana,
On 19. 6. 26. 오전 6:33, Saravana Kannan wrote:
> Look at the required OPPs of the "parent" device to determine the OPP that
> is required from the slave device managed by the passive governor. This
> allows having mappings between a parent device and a slave device even when
> they don't have the same number of OPPs.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> drivers/devfreq/governor_passive.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 3bc29acbd54e..f6de03de7a64 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -22,7 +22,7 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> = (struct devfreq_passive_data *)devfreq->data;
> struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
> unsigned long child_freq = ULONG_MAX;
> - struct dev_pm_opp *opp;
> + struct dev_pm_opp *opp = NULL, *p_opp = NULL;
> int i, count, ret = 0;
>
> /*
> @@ -59,13 +59,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> * list of parent device. Because in this case, *freq is temporary
> * value which is decided by ondemand governor.
> */
> - opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> - if (IS_ERR(opp)) {
> - ret = PTR_ERR(opp);
> + p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> + if (IS_ERR(p_opp)) {
> + ret = PTR_ERR(p_opp);
> goto out;
> }
>
> - dev_pm_opp_put(opp);
> + if (devfreq->opp_table && parent_devfreq->opp_table)
> + opp = dev_pm_opp_xlate_opp(parent_devfreq->opp_table,
> + devfreq->opp_table, p_opp);
> + if (opp) {
> + *freq = dev_pm_opp_get_freq(opp);
> + dev_pm_opp_put(opp);
> + goto out;
> + }
>
> /*
> * Get the OPP table's index of decided freqeuncy by governor
> @@ -92,6 +99,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> *freq = child_freq;
>
> out:
> + if (!IS_ERR_OR_NULL(opp))
> + dev_pm_opp_put(p_opp);
> +
> return ret;
> }
>
>
I agree this approach. It is necessary.
Acked-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hey Saravana,
Thanks for taking time to post out this series.
On 6/26/19 3:03 AM, Saravana Kannan wrote:
> A Device-A can have a (minimum) performance requirement on another
> Device-B to be able to function correctly. This performance requirement
> on Device-B can also change based on the current performance level of
> Device-A.
>
> The existing required-opps feature fits well to describe this need. So,
> instead of limiting required-opps to point to only PM-domain devices,
> allow it to point to any device.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> drivers/opp/core.c | 2 +-
> drivers/opp/of.c | 14 --------------
> 2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..74c7bdc6f463 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
> return 0;
>
> /* Single genpd case */
> - if (!genpd_virt_devs) {
> + if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
https://patchwork.kernel.org/patch/10940671/
This was already removed as a part of ^^ and is in linux-next.
> pstate = opp->required_opps[0]->pstate;
> ret = dev_pm_genpd_set_performance_state(dev, pstate);
> if (ret) {
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c10c782d15aa..7c8336e94aff 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> */
> count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells");
> - if (!count_pd)
> - goto put_np;
> -
> if (count_pd > 1) {
> genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> GFP_KERNEL);
> @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>
> if (IS_ERR(required_opp_tables[i]))
> goto free_required_tables;
> -
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know how much about other cases. Error out if the
> - * required OPP doesn't belong to a genpd.
> - */
> - if (!required_opp_tables[i]->is_genpd) {
> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> - required_np);
> - goto free_required_tables;
> - }
I expect the series to not work as is in its current state since I
see a circular dependency here. The required-opp tables of the parent
devfreq won't be populated until we add the opp-table of the child
devfreq node while the child devfreq using passive governor would
return -EPROBE_DEFER until the parent devfreq probes.
The same applies to this patch -> https://patchwork.kernel.org/patch
/11046147/ I posted out based on your series. So we would probably have
to address the dependency here.
> }
>
> goto put_np;
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hey Saravana,
Thanks for taking time to post out this series.
On 6/26/19 3:03 AM, Saravana Kannan wrote:
> A Device-A can have a (minimum) performance requirement on another
> Device-B to be able to function correctly. This performance requirement
> on Device-B can also change based on the current performance level of
> Device-A.
>
> The existing required-opps feature fits well to describe this need. So,
> instead of limiting required-opps to point to only PM-domain devices,
> allow it to point to any device.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> drivers/opp/core.c | 2 +-
> drivers/opp/of.c | 14 --------------
> 2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..74c7bdc6f463 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
> return 0;
>
> /* Single genpd case */
> - if (!genpd_virt_devs) {
> + if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
https://patchwork.kernel.org/patch/10940671/
This was already removed as a part of ^^ and is in linux-next.
> pstate = opp->required_opps[0]->pstate;
> ret = dev_pm_genpd_set_performance_state(dev, pstate);
> if (ret) {
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c10c782d15aa..7c8336e94aff 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> */
> count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells");
> - if (!count_pd)
> - goto put_np;
> -
> if (count_pd > 1) {
> genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> GFP_KERNEL);
> @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>
> if (IS_ERR(required_opp_tables[i]))
> goto free_required_tables;
> -
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know how much about other cases. Error out if the
> - * required OPP doesn't belong to a genpd.
> - */
> - if (!required_opp_tables[i]->is_genpd) {
> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> - required_np);
> - goto free_required_tables;
> - }
I expect the series to not work as is in its current state since I
see a circular dependency here. The required-opp tables of the parent
devfreq won't be populated until we add the opp-table of the child
devfreq node while the child devfreq using passive governor would
return -EPROBE_DEFER until the parent devfreq probes.
The same applies to this patch -> https://patchwork.kernel.org/patch
/11046147/ I posted out based on your series. So we would probably have
to address the dependency here.
> }
>
> goto put_np;
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hey Saravana,
On 6/26/19 3:03 AM, Saravana Kannan wrote:
> The OPP table can be used often in devfreq. Trying to get it each time can
> be expensive, so cache it in the devfreq struct.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> drivers/devfreq/devfreq.c | 6 ++++++
> include/linux/devfreq.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..ac62b78dc035 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
> if (devfreq->profile->exit)
> devfreq->profile->exit(devfreq->dev.parent);
>
> + if (devfreq->opp_table)
> + dev_pm_opp_put_opp_table(devfreq->opp_table);
> mutex_destroy(&devfreq->lock);
> kfree(devfreq);
> }
> @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->max_freq = devfreq->scaling_max_freq;
>
> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> + devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> + if (IS_ERR(devfreq->opp_table))
> + devfreq->opp_table = NULL;
> +
> atomic_set(&devfreq->suspend_count, 0);
>
> dev_set_name(&devfreq->dev, "devfreq%d",
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fbffa74bfc1b..0d877c9513d7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -156,6 +156,7 @@ struct devfreq {
> struct devfreq_dev_profile *profile;
> const struct devfreq_governor *governor;
> char governor_name[DEVFREQ_NAME_LEN];
> + struct opp_table *opp_table;
please add it to the function docs as well
> struct notifier_block nb;
> struct delayed_work work;
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Tue, Jul 16, 2019 at 10:18 AM Sibi Sankar <[email protected]> wrote:
>
> Hey Saravana,
> Thanks for taking time to post out this series.
>
> On 6/26/19 3:03 AM, Saravana Kannan wrote:
> > A Device-A can have a (minimum) performance requirement on another
> > Device-B to be able to function correctly. This performance requirement
> > on Device-B can also change based on the current performance level of
> > Device-A.
> >
> > The existing required-opps feature fits well to describe this need. So,
> > instead of limiting required-opps to point to only PM-domain devices,
> > allow it to point to any device.
> >
> > Signed-off-by: Saravana Kannan <[email protected]>
> > ---
> > drivers/opp/core.c | 2 +-
> > drivers/opp/of.c | 14 --------------
> > 2 files changed, 1 insertion(+), 15 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 0e7703fe733f..74c7bdc6f463 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
> > return 0;
> >
> > /* Single genpd case */
> > - if (!genpd_virt_devs) {
> > + if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
> https://patchwork.kernel.org/patch/10940671/
> This was already removed as a part of ^^ and is in linux-next.
Thanks for the heads up.
>
> > pstate = opp->required_opps[0]->pstate;
> > ret = dev_pm_genpd_set_performance_state(dev, pstate);
> > if (ret) {
> > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > index c10c782d15aa..7c8336e94aff 100644
> > --- a/drivers/opp/of.c
> > +++ b/drivers/opp/of.c
> > @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> > */
> > count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> > "#power-domain-cells");
> > - if (!count_pd)
> > - goto put_np;
> > -
> > if (count_pd > 1) {
> > genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> > GFP_KERNEL);
> > @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> >
> > if (IS_ERR(required_opp_tables[i]))
> > goto free_required_tables;
> > -
> > - /*
> > - * We only support genpd's OPPs in the "required-opps" for now,
> > - * as we don't know how much about other cases. Error out if the
> > - * required OPP doesn't belong to a genpd.
> > - */
> > - if (!required_opp_tables[i]->is_genpd) {
> > - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> > - required_np);
> > - goto free_required_tables;
> > - }
>
> I expect the series to not work as is in its current state since I
> see a circular dependency here. The required-opp tables of the parent
> devfreq won't be populated until we add the opp-table of the child
> devfreq node while the child devfreq using passive governor would
> return -EPROBE_DEFER until the parent devfreq probes.
I looked into this when I wrote the patches. I thought I handled it
correctly. Let me take another look.
-Saravana
> The same applies to this patch -> https://patchwork.kernel.org/patch
> /11046147/ I posted out based on your series. So we would probably have
> to address the dependency here.
>
> > }
> >
> > goto put_np;
> >
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
On Tue, Jul 16, 2019 at 10:36 AM Sibi Sankar <[email protected]> wrote:
>
> Hey Saravana,
>
> On 6/26/19 3:03 AM, Saravana Kannan wrote:
> > The OPP table can be used often in devfreq. Trying to get it each time can
> > be expensive, so cache it in the devfreq struct.
> >
> > Signed-off-by: Saravana Kannan <[email protected]>
> > ---
> > drivers/devfreq/devfreq.c | 6 ++++++
> > include/linux/devfreq.h | 1 +
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 6b6991f0e873..ac62b78dc035 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
> > if (devfreq->profile->exit)
> > devfreq->profile->exit(devfreq->dev.parent);
> >
> > + if (devfreq->opp_table)
> > + dev_pm_opp_put_opp_table(devfreq->opp_table);
> > mutex_destroy(&devfreq->lock);
> > kfree(devfreq);
> > }
> > @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> > devfreq->max_freq = devfreq->scaling_max_freq;
> >
> > devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> > + devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> > + if (IS_ERR(devfreq->opp_table))
> > + devfreq->opp_table = NULL;
> > +
> > atomic_set(&devfreq->suspend_count, 0);
> >
> > dev_set_name(&devfreq->dev, "devfreq%d",
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index fbffa74bfc1b..0d877c9513d7 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -156,6 +156,7 @@ struct devfreq {
> > struct devfreq_dev_profile *profile;
> > const struct devfreq_governor *governor;
> > char governor_name[DEVFREQ_NAME_LEN];
> > + struct opp_table *opp_table;
>
> please add it to the function docs as well
Will do.
-Saravana