* [PATCH v5 1/3] OPP: Add function to look up required OPP's for a given OPP
2021-02-03 9:23 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Hsin-Yi Wang
@ 2021-02-03 9:23 ` Hsin-Yi Wang
2021-02-04 5:46 ` Viresh Kumar
2021-02-03 9:23 ` [PATCH v5 2/3] PM / devfreq: Cache OPP table reference in devfreq Hsin-Yi Wang
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Hsin-Yi Wang @ 2021-02-03 9:23 UTC (permalink / raw)
To: Viresh Kumar, linux-pm
Cc: Nishanth Menon, Stephen Boyd, Rafael J . Wysocki, linux-kernel,
MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
From: Saravana Kannan <saravanak@google.com>
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 <saravanak@google.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
drivers/opp/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_opp.h | 11 ++++++++
2 files changed, 69 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index dc95d29e94c1b..878f066b972cc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2398,6 +2398,64 @@ devm_pm_opp_attach_genpd(struct device *dev, const char **names,
}
EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
+/**
+ * dev_pm_opp_xlate_required_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.
+ *
+ * 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_required_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 ||
+ !src_table->required_opp_tables)
+ return NULL;
+
+ /* required-opps not fully initialized yet */
+ if (lazy_linking_pending(src_table))
+ return NULL;
+
+ for (i = 0; i < src_table->required_opp_count; i++) {
+ if (src_table->required_opp_tables[i] == dst_table)
+ 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 ab1d15ce559db..6f5f72a7f601c 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -156,6 +156,9 @@ struct opp_table *devm_pm_opp_register_set_opp_helper(struct device *dev, int (*
struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
+struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
+ struct opp_table *dst_table,
+ struct dev_pm_opp *src_opp);
int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
@@ -367,6 +370,14 @@ static inline struct opp_table *devm_pm_opp_attach_genpd(struct device *dev,
return ERR_PTR(-EOPNOTSUPP);
}
+static inline struct dev_pm_opp *dev_pm_opp_xlate_required_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_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate)
{
return -EOPNOTSUPP;
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5 1/3] OPP: Add function to look up required OPP's for a given OPP
2021-02-03 9:23 ` [PATCH v5 1/3] OPP: Add function to look up required OPP's for a given OPP Hsin-Yi Wang
@ 2021-02-04 5:46 ` Viresh Kumar
0 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2021-02-04 5:46 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Viresh Kumar, linux-pm, Nishanth Menon, Stephen Boyd,
Rafael J . Wysocki, linux-kernel, MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
On 03-02-21, 17:23, Hsin-Yi Wang wrote:
> From: Saravana Kannan <saravanak@google.com>
>
> 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 <saravanak@google.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> drivers/opp/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_opp.h | 11 ++++++++
> 2 files changed, 69 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index dc95d29e94c1b..878f066b972cc 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2398,6 +2398,64 @@ devm_pm_opp_attach_genpd(struct device *dev, const char **names,
> }
> EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
>
> +/**
> + * dev_pm_opp_xlate_required_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.
> + *
> + * 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_required_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 ||
> + !src_table->required_opp_tables)
> + return NULL;
> +
> + /* required-opps not fully initialized yet */
> + if (lazy_linking_pending(src_table))
> + return NULL;
> +
> + for (i = 0; i < src_table->required_opp_count; i++) {
> + if (src_table->required_opp_tables[i] == dst_table)
> + 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 ab1d15ce559db..6f5f72a7f601c 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -156,6 +156,9 @@ struct opp_table *devm_pm_opp_register_set_opp_helper(struct device *dev, int (*
> struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
> void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> struct opp_table *devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct device ***virt_devs);
> +struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table,
> + struct opp_table *dst_table,
> + struct dev_pm_opp *src_opp);
> int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
> int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
> int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
> @@ -367,6 +370,14 @@ static inline struct opp_table *devm_pm_opp_attach_genpd(struct device *dev,
> return ERR_PTR(-EOPNOTSUPP);
> }
>
> +static inline struct dev_pm_opp *dev_pm_opp_xlate_required_opp(
> + struct opp_table *src_table,
> + struct opp_table *dst_table,
> + struct dev_pm_opp *src_opp)
> +{
> + return NULL;
> +}
Like other routines that return opp *, don't return NULL on errors but
a valid error number instead. And follow the declaration format of
other routines from this file, don't break lines etc..
> +
> static inline int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate)
> {
> return -EOPNOTSUPP;
> --
> 2.30.0.365.g02bc693789-goog
--
viresh
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 2/3] PM / devfreq: Cache OPP table reference in devfreq
2021-02-03 9:23 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Hsin-Yi Wang
2021-02-03 9:23 ` [PATCH v5 1/3] OPP: Add function to look up required OPP's for a given OPP Hsin-Yi Wang
@ 2021-02-03 9:23 ` Hsin-Yi Wang
2021-02-03 9:24 ` [PATCH v5 3/3] PM / devfreq: Add required OPPs support to passive governor Hsin-Yi Wang
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Hsin-Yi Wang @ 2021-02-03 9:23 UTC (permalink / raw)
To: Viresh Kumar, linux-pm
Cc: Nishanth Menon, Stephen Boyd, Rafael J . Wysocki, linux-kernel,
MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
From: Saravana Kannan <saravanak@google.com>
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 <saravanak@google.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
drivers/devfreq/devfreq.c | 6 ++++++
include/linux/devfreq.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 6aa10de792b33..a5899c9ae16fc 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -757,6 +757,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);
}
@@ -844,6 +846,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
}
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, "%s", dev_name(dev));
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index b6d3bae1c74d8..26ea0850be9bb 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -137,6 +137,7 @@ struct devfreq_stats {
* using devfreq.
* @profile: device-specific devfreq profile
* @governor: method how to choose frequency based on the usage.
+ * @opp_table: Reference to OPP table of dev.parent, if one exists.
* @nb: notifier block used to notify devfreq object that it should
* reevaluate operable frequencies. Devfreq users may use
* devfreq.nb to the corresponding register notifier call chain.
@@ -173,6 +174,7 @@ struct devfreq {
struct device dev;
struct devfreq_dev_profile *profile;
const struct devfreq_governor *governor;
+ struct opp_table *opp_table;
struct notifier_block nb;
struct delayed_work work;
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v5 3/3] PM / devfreq: Add required OPPs support to passive governor
2021-02-03 9:23 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Hsin-Yi Wang
2021-02-03 9:23 ` [PATCH v5 1/3] OPP: Add function to look up required OPP's for a given OPP Hsin-Yi Wang
2021-02-03 9:23 ` [PATCH v5 2/3] PM / devfreq: Cache OPP table reference in devfreq Hsin-Yi Wang
@ 2021-02-03 9:24 ` Hsin-Yi Wang
2021-02-04 2:49 ` Viresh Kumar
2021-02-03 10:12 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Chanwoo Choi
2021-02-04 5:41 ` Viresh Kumar
4 siblings, 1 reply; 11+ messages in thread
From: Hsin-Yi Wang @ 2021-02-03 9:24 UTC (permalink / raw)
To: Viresh Kumar, linux-pm
Cc: Nishanth Menon, Stephen Boyd, Rafael J . Wysocki, linux-kernel,
MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
From: Saravana Kannan <saravanak@google.com>
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 <saravanak@google.com>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
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 63332e4a65ae8..8d92b1964f9c3 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -19,7 +19,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;
/*
@@ -56,13 +56,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_required_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
@@ -89,6 +96,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.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] PM / devfreq: Add required OPPs support to passive governor
2021-02-03 9:24 ` [PATCH v5 3/3] PM / devfreq: Add required OPPs support to passive governor Hsin-Yi Wang
@ 2021-02-04 2:49 ` Viresh Kumar
2021-02-04 7:07 ` Hsin-Yi Wang
0 siblings, 1 reply; 11+ messages in thread
From: Viresh Kumar @ 2021-02-04 2:49 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Viresh Kumar, linux-pm, Nishanth Menon, Stephen Boyd,
Rafael J . Wysocki, linux-kernel, MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
On 03-02-21, 17:24, Hsin-Yi Wang wrote:
> From: Saravana Kannan <saravanak@google.com>
>
> 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 <saravanak@google.com>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> 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 63332e4a65ae8..8d92b1964f9c3 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -19,7 +19,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;
I would initialize p_opp to ERR_PTR(-ENODEV) to avoid using
IS_ERR_OR_NULL. There is no need to initialize opp as well.
> int i, count, ret = 0;
>
> /*
> @@ -56,13 +56,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;
Perhaps just return from here, the goto is useless here.
> }
>
> - dev_pm_opp_put(opp);
> + if (devfreq->opp_table && parent_devfreq->opp_table)
> + opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
> + devfreq->opp_table, p_opp);
> + if (opp) {
This needs to be part of the above if block itself, else the opp will
always be NULL, isn't it ?
> + *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
> @@ -89,6 +96,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> *freq = child_freq;
>
> out:
> + if (!IS_ERR_OR_NULL(opp))
you should be checking for p_opp here, isn't it ? And perhaps we don't
need this check as well as p_opp can't be invalid here.
> + dev_pm_opp_put(p_opp);
> +
> return ret;
> }
>
> --
> 2.30.0.365.g02bc693789-goog
--
viresh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] PM / devfreq: Add required OPPs support to passive governor
2021-02-04 2:49 ` Viresh Kumar
@ 2021-02-04 7:07 ` Hsin-Yi Wang
0 siblings, 0 replies; 11+ messages in thread
From: Hsin-Yi Wang @ 2021-02-04 7:07 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Linux PM, Nishanth Menon, Stephen Boyd,
Rafael J . Wysocki, lkml, MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
On Thu, Feb 4, 2021 at 10:49 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-02-21, 17:24, Hsin-Yi Wang wrote:
> > From: Saravana Kannan <saravanak@google.com>
> >
> > 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 <saravanak@google.com>
> > Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > 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 63332e4a65ae8..8d92b1964f9c3 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -19,7 +19,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;
>
> I would initialize p_opp to ERR_PTR(-ENODEV) to avoid using
> IS_ERR_OR_NULL. There is no need to initialize opp as well.
>
> > int i, count, ret = 0;
> >
> > /*
> > @@ -56,13 +56,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;
>
> Perhaps just return from here, the goto is useless here.
>
> > }
> >
> > - dev_pm_opp_put(opp);
> > + if (devfreq->opp_table && parent_devfreq->opp_table)
> > + opp = dev_pm_opp_xlate_required_opp(parent_devfreq->opp_table,
> > + devfreq->opp_table, p_opp);
> > + if (opp) {
>
> This needs to be part of the above if block itself, else the opp will
> always be NULL, isn't it ?
>
> > + *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
> > @@ -89,6 +96,9 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> > *freq = child_freq;
> >
> > out:
> > + if (!IS_ERR_OR_NULL(opp))
>
> you should be checking for p_opp here, isn't it ? And perhaps we don't
> need this check as well as p_opp can't be invalid here.
>
> > + dev_pm_opp_put(p_opp);
> > +
> > return ret;
> > }
> >
> > --
> > 2.30.0.365.g02bc693789-goog
>
Thanks for the review. I'll fix them and send next version
> --
> viresh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 0/3] Add required-opps support to devfreq passive gov
2021-02-03 9:23 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Hsin-Yi Wang
` (2 preceding siblings ...)
2021-02-03 9:24 ` [PATCH v5 3/3] PM / devfreq: Add required OPPs support to passive governor Hsin-Yi Wang
@ 2021-02-03 10:12 ` Chanwoo Choi
2021-02-04 5:41 ` Viresh Kumar
4 siblings, 0 replies; 11+ messages in thread
From: Chanwoo Choi @ 2021-02-03 10:12 UTC (permalink / raw)
To: Hsin-Yi Wang, Viresh Kumar, linux-pm
Cc: Nishanth Menon, Stephen Boyd, Rafael J . Wysocki, linux-kernel,
MyungJoo Ham ),
Kyungmin Park, Saravana Kannan
Hi Hsin-Yi,
Thanks for the patch. I already reviewed this patch.
But, I'll check these again and test it.
On 2/3/21 6:23 PM, Hsin-Yi Wang wrote:
> 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
>
> v4 -> v5:
> - drop patch "OPP: Improve required-opps linking" and rebase to
> https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=opp/linux-next
> - Compare pointers in dev_pm_opp_xlate_required_opp().
>
> v3 -> v4:
> - Fixed documentation comments
> - Fixed order of functions in .h file
> - Renamed the new xlate API
> - Caused _set_required_opps() to fail if all required opps tables aren't
> linked.
> v2 -> v3:
> - Rebased onto linux-next.
> - Added documentation comment for new fields.
> - Added support for lazy required-opps linking.
> - Updated Ack/Reviewed-bys.
> 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 (3):
> 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 | 58 ++++++++++++++++++++++++++++++
> include/linux/devfreq.h | 2 ++
> include/linux/pm_opp.h | 11 ++++++
> 5 files changed, 92 insertions(+), 5 deletions(-)
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 0/3] Add required-opps support to devfreq passive gov
2021-02-03 9:23 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Hsin-Yi Wang
` (3 preceding siblings ...)
2021-02-03 10:12 ` [PATCH v5 0/3] Add required-opps support to devfreq passive gov Chanwoo Choi
@ 2021-02-04 5:41 ` Viresh Kumar
2021-02-04 8:12 ` Hsin-Yi Wang
2021-02-04 8:53 ` Chanwoo Choi
4 siblings, 2 replies; 11+ messages in thread
From: Viresh Kumar @ 2021-02-04 5:41 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Viresh Kumar, linux-pm, Nishanth Menon, Stephen Boyd,
Rafael J . Wysocki, linux-kernel, MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
On 03-02-21, 17:23, Hsin-Yi Wang wrote:
> 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.
So you guys aren't interested in dev_pm_opp_set_opp() but just the
translation of the required-OPPs ?
I am fine with most of the stuff and I would like to take it via OPP
tree, hope that would be fine ?
--
viresh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 0/3] Add required-opps support to devfreq passive gov
2021-02-04 5:41 ` Viresh Kumar
@ 2021-02-04 8:12 ` Hsin-Yi Wang
2021-02-04 8:53 ` Chanwoo Choi
1 sibling, 0 replies; 11+ messages in thread
From: Hsin-Yi Wang @ 2021-02-04 8:12 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Linux PM, Nishanth Menon, Stephen Boyd,
Rafael J . Wysocki, lkml, MyungJoo Ham ),
Kyungmin Park, Chanwoo Choi, Saravana Kannan
On Thu, Feb 4, 2021 at 1:41 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-02-21, 17:23, Hsin-Yi Wang wrote:
> > 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.
>
> So you guys aren't interested in dev_pm_opp_set_opp() but just the
> translation of the required-OPPs ?
>
I think this series focuses on required-opps.
> I am fine with most of the stuff and I would like to take it via OPP
> tree, hope that would be fine ?
>
Sounds good to me, thanks.
> --
> viresh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 0/3] Add required-opps support to devfreq passive gov
2021-02-04 5:41 ` Viresh Kumar
2021-02-04 8:12 ` Hsin-Yi Wang
@ 2021-02-04 8:53 ` Chanwoo Choi
1 sibling, 0 replies; 11+ messages in thread
From: Chanwoo Choi @ 2021-02-04 8:53 UTC (permalink / raw)
To: Viresh Kumar, Hsin-Yi Wang
Cc: Viresh Kumar, linux-pm, Nishanth Menon, Stephen Boyd,
Rafael J . Wysocki, linux-kernel, MyungJoo Ham ),
Kyungmin Park, Saravana Kannan
Hi Viresh,
On 2/4/21 2:41 PM, Viresh Kumar wrote:
> On 03-02-21, 17:23, Hsin-Yi Wang wrote:
>> 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.
>
> So you guys aren't interested in dev_pm_opp_set_opp() but just the
> translation of the required-OPPs ?
>
> I am fine with most of the stuff and I would like to take it via OPP
> tree, hope that would be fine ?
I agree. Take these patches to OPP tree.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 11+ messages in thread