2022-06-10 08:58:45

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 0/8] OPP: Add support for multiple clocks

Hello,

This patchset adds support for device with multiple clocks. None of the clocks
is considered primary in this case and all are handled equally.

This is rebased over a lot of other OPP changes and is pushed here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/clk

Krzysztof, can you please test this for your use case. I wasn't able to test he
multiple clock support.

--
Viresh

Viresh Kumar (8):
OPP: Use consistent names for OPP table instances
OPP: Remove rate_not_available parameter to _opp_add()
OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
OPP: Make dev_pm_opp_set_opp() independent of frequency
OPP: Allow multiple clocks for a device
OPP: Add key specific assert() method to key finding helpers
OPP: Assert clk_count == 1 for single clk helpers
OPP: Provide a simple implementation to configure multiple clocks

drivers/opp/core.c | 337 +++++++++++++++++++++++++++++++----------
drivers/opp/cpu.c | 12 +-
drivers/opp/debugfs.c | 27 +++-
drivers/opp/of.c | 91 +++++++----
drivers/opp/opp.h | 22 +--
include/linux/pm_opp.h | 17 ++-
6 files changed, 378 insertions(+), 128 deletions(-)

--
2.31.1.272.g89b43f80a514


2022-06-10 09:08:07

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 6/8] OPP: Add key specific assert() method to key finding helpers

The helpers for the clock key, at least, would need to assert that the
helpers are called only for single clock case. Prepare for that by
adding an argument to the key finding helpers.

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 56 +++++++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 1e143bd8e589..b8e6dc0a9b36 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -477,10 +477,15 @@ static struct dev_pm_opp *_opp_table_find_key(struct opp_table *opp_table,
unsigned long *key, int index, bool available,
unsigned long (*read)(struct dev_pm_opp *opp, int index),
bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
- unsigned long opp_key, unsigned long key))
+ unsigned long opp_key, unsigned long key),
+ bool (*assert)(struct opp_table *opp_table))
{
struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);

+ /* Assert that the requirement is met */
+ if (assert && !assert(opp_table))
+ return ERR_PTR(-EINVAL);
+
mutex_lock(&opp_table->lock);

list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
@@ -505,7 +510,8 @@ static struct dev_pm_opp *
_find_key(struct device *dev, unsigned long *key, int index, bool available,
unsigned long (*read)(struct dev_pm_opp *opp, int index),
bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
- unsigned long opp_key, unsigned long key))
+ unsigned long opp_key, unsigned long key),
+ bool (*assert)(struct opp_table *opp_table))
{
struct opp_table *opp_table;
struct dev_pm_opp *opp;
@@ -518,7 +524,7 @@ _find_key(struct device *dev, unsigned long *key, int index, bool available,
}

opp = _opp_table_find_key(opp_table, key, index, available, read,
- compare);
+ compare, assert);

dev_pm_opp_put_opp_table(opp_table);

@@ -527,35 +533,42 @@ _find_key(struct device *dev, unsigned long *key, int index, bool available,

static struct dev_pm_opp *_find_key_exact(struct device *dev,
unsigned long key, int index, bool available,
- unsigned long (*read)(struct dev_pm_opp *opp, int index))
+ unsigned long (*read)(struct dev_pm_opp *opp, int index),
+ bool (*assert)(struct opp_table *opp_table))
{
/*
* The value of key will be updated here, but will be ignored as the
* caller doesn't need it.
*/
- return _find_key(dev, &key, index, available, read, _compare_exact);
+ return _find_key(dev, &key, index, available, read, _compare_exact,
+ assert);
}

static struct dev_pm_opp *_opp_table_find_key_ceil(struct opp_table *opp_table,
unsigned long *key, int index, bool available,
- unsigned long (*read)(struct dev_pm_opp *opp, int index))
+ unsigned long (*read)(struct dev_pm_opp *opp, int index),
+ bool (*assert)(struct opp_table *opp_table))
{
return _opp_table_find_key(opp_table, key, index, available, read,
- _compare_ceil);
+ _compare_ceil, assert);
}

static struct dev_pm_opp *_find_key_ceil(struct device *dev, unsigned long *key,
int index, bool available,
- unsigned long (*read)(struct dev_pm_opp *opp, int index))
+ unsigned long (*read)(struct dev_pm_opp *opp, int index),
+ bool (*assert)(struct opp_table *opp_table))
{
- return _find_key(dev, key, index, available, read, _compare_ceil);
+ return _find_key(dev, key, index, available, read, _compare_ceil,
+ assert);
}

static struct dev_pm_opp *_find_key_floor(struct device *dev,
unsigned long *key, int index, bool available,
- unsigned long (*read)(struct dev_pm_opp *opp, int index))
+ unsigned long (*read)(struct dev_pm_opp *opp, int index),
+ bool (*assert)(struct opp_table *opp_table))
{
- return _find_key(dev, key, index, available, read, _compare_floor);
+ return _find_key(dev, key, index, available, read, _compare_floor,
+ assert);
}

/**
@@ -584,14 +597,15 @@ static struct dev_pm_opp *_find_key_floor(struct device *dev,
struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
unsigned long freq, bool available)
{
- return _find_key_exact(dev, freq, 0, available, _read_freq);
+ return _find_key_exact(dev, freq, 0, available, _read_freq, NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);

static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
unsigned long *freq)
{
- return _opp_table_find_key_ceil(opp_table, freq, 0, true, _read_freq);
+ return _opp_table_find_key_ceil(opp_table, freq, 0, true, _read_freq,
+ NULL);
}

/**
@@ -615,7 +629,7 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
unsigned long *freq)
{
- return _find_key_ceil(dev, freq, 0, true, _read_freq);
+ return _find_key_ceil(dev, freq, 0, true, _read_freq, NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);

@@ -640,7 +654,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
unsigned long *freq)
{
- return _find_key_floor(dev, freq, 0, true, _read_freq);
+ return _find_key_floor(dev, freq, 0, true, _read_freq, NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);

@@ -662,7 +676,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,
unsigned int level)
{
- return _find_key_exact(dev, level, 0, true, _read_level);
+ return _find_key_exact(dev, level, 0, true, _read_level, NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_level_exact);

@@ -684,8 +698,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_level_exact);
struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,
unsigned int *level)
{
- return _find_key_ceil(dev, (unsigned long *)level, 0, true,
- _read_level);
+ return _find_key_ceil(dev, (unsigned long *)level, 0, true, _read_level,
+ NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_level_ceil);

@@ -711,7 +725,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_level_ceil);
struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev, unsigned int *bw,
int index)
{
- return _find_key_ceil(dev, (unsigned long *)bw, index, true, _read_bw);
+ return _find_key_ceil(dev, (unsigned long *)bw, index, true, _read_bw,
+ NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_bw_ceil);

@@ -737,7 +752,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_bw_ceil);
struct dev_pm_opp *dev_pm_opp_find_bw_floor(struct device *dev,
unsigned int *bw, int index)
{
- return _find_key_floor(dev, (unsigned long *)bw, index, true, _read_bw);
+ return _find_key_floor(dev, (unsigned long *)bw, index, true, _read_bw,
+ NULL);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_find_bw_floor);

--
2.31.1.272.g89b43f80a514

2022-06-10 09:27:37

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 2/8] OPP: Remove rate_not_available parameter to _opp_add()

commit 32715be4fe95 ("opp: Fix adding OPP entries in a wrong order if
rate is unavailable") removed the only user of this field, get rid of
rest of it now.

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 4 ++--
drivers/opp/of.c | 10 ++++------
drivers/opp/opp.h | 2 +-
3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 404f43759066..fe447f41c99e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1695,7 +1695,7 @@ void _required_opps_available(struct dev_pm_opp *opp, int count)
* should be considered an error by the callers of _opp_add().
*/
int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
- struct opp_table *opp_table, bool rate_not_available)
+ struct opp_table *opp_table)
{
struct list_head *head;
int ret;
@@ -1774,7 +1774,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
new_opp->available = true;
new_opp->dynamic = dynamic;

- ret = _opp_add(dev, new_opp, opp_table, false);
+ ret = _opp_add(dev, new_opp, opp_table);
if (ret) {
/* Don't return error for duplicate OPPs */
if (ret == -EBUSY)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e07fc31de416..bec9644a7260 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -808,8 +808,8 @@ static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
return ret;
}

-static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *opp_table,
- struct device_node *np, bool *rate_not_available)
+static int _read_opp_key(struct dev_pm_opp *new_opp,
+ struct opp_table *opp_table, struct device_node *np)
{
bool found = false;
u64 rate;
@@ -825,7 +825,6 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *opp_table
new_opp->rate = (unsigned long)rate;
found = true;
}
- *rate_not_available = !!ret;

/*
* Bandwidth consists of peak and average (optional) values:
@@ -881,13 +880,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
struct dev_pm_opp *new_opp;
u32 val;
int ret;
- bool rate_not_available = false;

new_opp = _opp_allocate(opp_table);
if (!new_opp)
return ERR_PTR(-ENOMEM);

- ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
+ ret = _read_opp_key(new_opp, opp_table, np);
if (ret < 0) {
dev_err(dev, "%s: opp key field not found\n", __func__);
goto free_opp;
@@ -920,7 +918,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
if (opp_table->is_genpd)
new_opp->pstate = pm_genpd_opp_to_performance_state(dev, new_opp);

- ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
+ ret = _opp_add(dev, new_opp, opp_table);
if (ret) {
/* Don't return error for duplicate OPPs */
if (ret == -EBUSY)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 407eee9f10ab..4d8894ef2975 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -226,7 +226,7 @@ struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_
struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
void _opp_free(struct dev_pm_opp *opp);
int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
-int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
+int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk);
--
2.31.1.272.g89b43f80a514

2022-06-10 09:37:51

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 3/8] OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

Reuse _opp_compare_key() in _opp_add_static_v2() instead of just
comparing frequency while finding suspend frequency. Also add a comment
over _opp_compare_key() explaining its return values.

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 6 ++++++
drivers/opp/of.c | 4 ++--
2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index fe447f41c99e..9f284dc0d9d7 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1618,6 +1618,12 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
return true;
}

+/*
+ * Returns
+ * 0: opp1 == opp2
+ * 1: opp1 > opp2
+ * -1: opp1 < opp2
+ */
int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
{
if (opp1->rate != opp2->rate)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index bec9644a7260..843923ab9d66 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -929,8 +929,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
/* OPP to select on device suspend */
if (of_property_read_bool(np, "opp-suspend")) {
if (opp_table->suspend_opp) {
- /* Pick the OPP with higher rate as suspend OPP */
- if (new_opp->rate > opp_table->suspend_opp->rate) {
+ /* Pick the OPP with higher rate/bw/level as suspend OPP */
+ if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
opp_table->suspend_opp->suspend = false;
new_opp->suspend = true;
opp_table->suspend_opp = new_opp;
--
2.31.1.272.g89b43f80a514

2022-06-10 10:07:00

by Viresh Kumar

[permalink] [raw]
Subject: [PATCH 4/8] OPP: Make dev_pm_opp_set_opp() independent of frequency

dev_pm_opp_set_opp() can be called for any device, it may or may not
have a frequency value associated with it.

If a frequency value isn't available, we pass 0 to _set_opp(). Make it
optional instead by making _set_opp() accept a pointer instead, as the
frequency value is anyway available in the OPP. This makes
dev_pm_opp_set_opp() and _set_opp() completely independent of any
special key value.

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 52 +++++++++++++++++++++++++++++++++-------------
drivers/opp/opp.h | 4 ++--
2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9f284dc0d9d7..6368ae2d7360 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -766,19 +766,33 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
return ret;
}

-static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
- unsigned long freq)
+static inline int _generic_set_opp_clk_only(struct device *dev,
+ struct opp_table *opp_table, struct dev_pm_opp *opp, void *data)
{
+ unsigned long *target = data;
+ unsigned long freq;
int ret;

/* We may reach here for devices which don't change frequency */
- if (IS_ERR(clk))
+ if (IS_ERR(opp_table->clk))
return 0;

- ret = clk_set_rate(clk, freq);
+ /* One of target and opp must be available */
+ if (target) {
+ freq = *target;
+ } else if (opp) {
+ freq = opp->rate;
+ } else {
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
+ ret = clk_set_rate(opp_table->clk, freq);
if (ret) {
dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
ret);
+ } else {
+ opp_table->rate_clk_single = freq;
}

return ret;
@@ -972,7 +986,7 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
}

static int _set_opp(struct device *dev, struct opp_table *opp_table,
- struct dev_pm_opp *opp, unsigned long freq)
+ struct dev_pm_opp *opp, void *clk_data, bool forced)
{
struct dev_pm_opp *old_opp;
int scaling_down, ret;
@@ -987,15 +1001,14 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
old_opp = opp_table->current_opp;

/* Return early if nothing to do */
- if (old_opp == opp && opp_table->current_rate == freq &&
- opp_table->enabled) {
+ if (!forced && old_opp == opp && opp_table->enabled) {
dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
return 0;
}

dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
- __func__, opp_table->current_rate, freq, old_opp->level,
- opp->level, old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
+ __func__, old_opp->rate, opp->rate, old_opp->level, opp->level,
+ old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
opp->bandwidth ? opp->bandwidth[0].peak : 0);

scaling_down = _opp_compare_key(old_opp, opp);
@@ -1028,7 +1041,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
}
}

- ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
+ ret = _generic_set_opp_clk_only(dev, opp_table, opp, clk_data);
if (ret)
return ret;

@@ -1064,7 +1077,6 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
/* Make sure current_opp doesn't get freed */
dev_pm_opp_get(opp);
opp_table->current_opp = opp;
- opp_table->current_rate = freq;

return ret;
}
@@ -1085,6 +1097,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
struct opp_table *opp_table;
unsigned long freq = 0, temp_freq;
struct dev_pm_opp *opp = NULL;
+ bool forced = false;
int ret;

opp_table = _find_opp_table(dev);
@@ -1102,7 +1115,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
* equivalent to a clk_set_rate()
*/
if (!_get_opp_count(opp_table)) {
- ret = _generic_set_opp_clk_only(dev, opp_table->clk, target_freq);
+ ret = _generic_set_opp_clk_only(dev, opp_table, NULL,
+ &target_freq);
goto put_opp_table;
}

@@ -1123,12 +1137,22 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
__func__, freq, ret);
goto put_opp_table;
}
+
+ /*
+ * An OPP entry specifies the highest frequency at which other
+ * properties of the OPP entry apply. Even if the new OPP is
+ * same as the old one, we may still reach here for a different
+ * value of the frequency. In such a case, do not abort but
+ * configure the hardware to the desired frequency forcefully.
+ */
+ forced = opp_table->rate_clk_single != target_freq;
}

- ret = _set_opp(dev, opp_table, opp, freq);
+ ret = _set_opp(dev, opp_table, opp, &target_freq, forced);

if (target_freq)
dev_pm_opp_put(opp);
+
put_opp_table:
dev_pm_opp_put_opp_table(opp_table);
return ret;
@@ -1156,7 +1180,7 @@ int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
return PTR_ERR(opp_table);
}

- ret = _set_opp(dev, opp_table, opp, opp ? opp->rate : 0);
+ ret = _set_opp(dev, opp_table, opp, NULL, false);
dev_pm_opp_put_opp_table(opp_table);

return ret;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4d8894ef2975..131fc7c05db8 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -138,7 +138,7 @@ enum opp_table_access {
* @clock_latency_ns_max: Max clock latency in nanoseconds.
* @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
* @shared_opp: OPP is shared between multiple devices.
- * @current_rate: Currently configured frequency.
+ * @rate_clk_single: Currently configured frequency for single clk.
* @current_opp: Currently configured OPP for the table.
* @suspend_opp: Pointer to OPP to be used during device suspend.
* @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
@@ -187,7 +187,7 @@ struct opp_table {

unsigned int parsed_static_opps;
enum opp_table_access shared_opp;
- unsigned long current_rate;
+ unsigned long rate_clk_single;
struct dev_pm_opp *current_opp;
struct dev_pm_opp *suspend_opp;

--
2.31.1.272.g89b43f80a514

2022-06-22 14:02:44

by Jon Hunter

[permalink] [raw]
Subject: Re: [PATCH 3/8] OPP: Reuse _opp_compare_key() in _opp_add_static_v2()


On 10/06/2022 09:20, Viresh Kumar wrote:
> Reuse _opp_compare_key() in _opp_add_static_v2() instead of just
> comparing frequency while finding suspend frequency. Also add a comment
> over _opp_compare_key() explaining its return values.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/opp/core.c | 6 ++++++
> drivers/opp/of.c | 4 ++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index fe447f41c99e..9f284dc0d9d7 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1618,6 +1618,12 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
> return true;
> }
>
> +/*
> + * Returns
> + * 0: opp1 == opp2
> + * 1: opp1 > opp2
> + * -1: opp1 < opp2
> + */
> int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> {
> if (opp1->rate != opp2->rate)
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index bec9644a7260..843923ab9d66 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -929,8 +929,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> /* OPP to select on device suspend */
> if (of_property_read_bool(np, "opp-suspend")) {
> if (opp_table->suspend_opp) {
> - /* Pick the OPP with higher rate as suspend OPP */
> - if (new_opp->rate > opp_table->suspend_opp->rate) {
> + /* Pick the OPP with higher rate/bw/level as suspend OPP */
> + if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
> opp_table->suspend_opp->suspend = false;
> new_opp->suspend = true;
> opp_table->suspend_opp = new_opp;


FYI ... if I checkout commit 00d776d33da9 ("OPP: Reuse
_opp_compare_key() in _opp_add_static_v2()") from next-20220622
it does not compile ...

drivers/opp/of.c: In function ‘_opp_add_static_v2’:
drivers/opp/of.c:933:25: error: passing argument 1 of ‘_opp_compare_key’ from incompatible pointer type [-Werror=incompatible-pointer-types]
if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
^~~~~~~~~
In file included from drivers/opp/of.c:22:0:
drivers/opp/opp.h:228:5: note: expected ‘struct dev_pm_opp *’ but argument is of type ‘struct opp_table *’
int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
^~~~~~~~~~~~~~~~
drivers/opp/of.c:933:8: error: too many arguments to function ‘_opp_compare_key’
if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
^~~~~~~~~~~~~~~~
In file included from drivers/opp/of.c:22:0:
drivers/opp/opp.h:228:5: note: declared here
int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
^~~~~~~~~~~~~~~~

This breaks bisecting -next and so would be good to fix this.

Cheers
Jon

--
nvpublic

2022-06-22 14:11:43

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH 3/8] OPP: Reuse _opp_compare_key() in _opp_add_static_v2()

On 22-06-22, 14:58, Jon Hunter wrote:
> FYI ... if I checkout commit 00d776d33da9 ("OPP: Reuse
> _opp_compare_key() in _opp_add_static_v2()") from next-20220622
> it does not compile ...
>
> drivers/opp/of.c: In function ‘_opp_add_static_v2’:
> drivers/opp/of.c:933:25: error: passing argument 1 of ‘_opp_compare_key’ from incompatible pointer type [-Werror=incompatible-pointer-types]
> if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
> ^~~~~~~~~
> In file included from drivers/opp/of.c:22:0:
> drivers/opp/opp.h:228:5: note: expected ‘struct dev_pm_opp *’ but argument is of type ‘struct opp_table *’
> int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
> ^~~~~~~~~~~~~~~~
> drivers/opp/of.c:933:8: error: too many arguments to function ‘_opp_compare_key’
> if (_opp_compare_key(opp_table, new_opp, opp_table->suspend_opp) == 1) {
> ^~~~~~~~~~~~~~~~
> In file included from drivers/opp/of.c:22:0:
> drivers/opp/opp.h:228:5: note: declared here
> int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
> ^~~~~~~~~~~~~~~~
>
> This breaks bisecting -next and so would be good to fix this.

Yeah, this was reported yesterday and is already fixed in my branch, along with
few more fixes.

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

It hasn't landed into linux-next/master yet though.

--
viresh