OPP: Add support for multiple clocks*
Subsystems (6)
v2 cover OPP: Add support for multiple clocks*
Hello,
This patchset adds support for devices with multiple clocks. None of the clocks is considered primary in this case and all are handled equally.
The drivers, for multiple clock case, are expected to call dev_pm_opp_set_opp() to set the specific OPP. Though how they find the target OPP is left for the users to handle. For some, we may have another unique OPP property, like level, which can be used to find the OPP. While in case of others, we may want to implement freq-based OPP finder APIs for multiple clock rates. I have decided not to implement them in advance, and add them only someone wants to use them.
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/linux-next
V1->V2:
- Fix broken git bisect for:
OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
- Include binding changes written by Krzysztof earlier.
- Check config_clks before calling it, it isn't always set.
- Add config_clks for Tegra30's devfreq to handle its corner case.
- _opp_compare_key() supports multi-clk case now, earlier it skipped freq comparison for such a case.
- New patch to compare all bandwidth values as well in _opp_compare_key().
- New patch to remove *_noclk() interface.
- Various other minor fixes.
--
Viresh
Krzysztof Kozlowski (1):
dt-bindings: opp: accept array of frequencies
Viresh Kumar (12):
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: Compare bandwidths for all paths in _opp_compare_key()
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
OPP: Allow config_clks helper for single clk case
PM / devfreq: tegra30: Register config_clks helper
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
.../devicetree/bindings/opp/opp-v2-base.yaml | 10 + drivers/devfreq/tegra30-devfreq.c | 22 +- drivers/opp/core.c | 404 +++++++++++++----- drivers/opp/cpu.c | 12 +- drivers/opp/debugfs.c | 27 +- drivers/opp/of.c | 139 +++--- drivers/opp/opp.h | 24 +- include/linux/pm_opp.h | 29 +- 8 files changed, 466 insertions(+), 201 deletions(-)
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 1/13 OPP: Use consistent names for OPP table instances
The OPP table is called "opp_table" at most of the places and "table" at few. Make all of them follow the same naming convention, "opp_table".
--- drivers/opp/core.c | 7 ++++--- drivers/opp/cpu.c | 12 ++++++------ drivers/opp/of.c | 12 ++++++------ 3 files changed, 16 insertions(+), 15 deletions(-)
drivers/opp/core.c +4 -3
@@ -1585,15 +1585,16 @@ void dev_pm_opp_remove_all_dynamic(struct device *dev) }EXPORT_SYMBOL_GPL (dev_pm_opp_remove_all_dynamic); -struct dev_pm_opp *_opp_allocate (struct opp_table *table) +struct dev_pm_opp *_opp_allocate (struct opp_table *opp_table) {struct dev_pm_opp *opp;int supply_count, supply_size, icc_size;/* Allocate space for at least one supply */ - supply_count = table->regulator_count >0 ? table->regulator_count :1 ; + supply_count = opp_table->regulator_count >0 ? + opp_table->regulator_count :1 ; supply_size =sizeof (*opp->supplies) * supply_count; - icc_size =sizeof (*opp->bandwidth) * table->path_count; + icc_size =sizeof (*opp->bandwidth) * opp_table->path_count;/* allocate new OPP node and supplies structures */ opp =kzalloc (sizeof (*opp) + supply_size + icc_size,GFP_KERNEL );
drivers/opp/cpu.c +6 -6
@@ -41,7 +41,7 @@* the table if any of the mentioned functions have been invoked in the interim. */ int dev_pm_opp_init_cpufreq_table (struct device *dev, -struct cpufreq_frequency_table **table) +struct cpufreq_frequency_table **opp_table) {struct dev_pm_opp *opp;struct cpufreq_frequency_table *freq_table =NULL ;
@@ -76,7 +76,7 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, freq_table[i].driver_data = i; freq_table[i].frequency =CPUFREQ_TABLE_END ; - *table = &freq_table[0 ]; + *opp_table = &freq_table[0 ]; out:if (ret)
@@ -94,13 +94,13 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_init_cpufreq_table);* Free up the table allocated by dev_pm_opp_init_cpufreq_table */ void dev_pm_opp_free_cpufreq_table (struct device *dev, -struct cpufreq_frequency_table **table) +struct cpufreq_frequency_table **opp_table) { -if (!table) +if (!opp_table)return ; -kfree (*table); - *table =NULL ; +kfree (*opp_table); + *opp_table =NULL ; }EXPORT_SYMBOL_GPL (dev_pm_opp_free_cpufreq_table);#endif /* CONFIG_CPU_FREQ */
drivers/opp/of.c +6 -6
@@ -767,7 +767,7 @@ void dev_pm_opp_of_remove_table(struct device *dev) }EXPORT_SYMBOL_GPL (dev_pm_opp_of_remove_table); -static int _read_bw (struct dev_pm_opp *new_opp,struct opp_table *table, +static int _read_bw (struct dev_pm_opp *new_opp,struct opp_table *opp_table,struct device_node *np,bool peak) {const char *name = peak ?"opp-peak-kBps" :"opp-avg-kBps" ;
@@ -780,9 +780,9 @@ static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *table,return -ENODEV ; count = prop->length /sizeof (u32); -if (table->path_count != count) { +if (opp_table->path_count != count) {pr_err ("%s: Mismatch between %s and paths (%d %d)\n" , - __func__, name, count, table->path_count); + __func__, name, count, opp_table->path_count);return -EINVAL ; }
@@ -808,7 +808,7 @@ static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *table,return ret; } -static int _read_opp_key (struct dev_pm_opp *new_opp,struct opp_table *table, +static int _read_opp_key (struct dev_pm_opp *new_opp,struct opp_table *opp_table,struct device_node *np,bool *rate_not_available) {bool found = false;
@@ -832,10 +832,10 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *table,* opp-peak-kBps = <path1_value path2_value>; * opp-avg-kBps = <path1_value path2_value>; */ - ret =_read_bw (new_opp, table, np, true); + ret =_read_bw (new_opp, opp_table, np, true);if (!ret) { found = true; - ret =_read_bw (new_opp, table, np, false); + ret =_read_bw (new_opp, opp_table, np, false); }/* The properties were found but we failed to parse them */
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 2/13 OPP: Remove rate_not_available parameter to _opp_add()
In reply to Viresh Kumarcommit 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.
--- drivers/opp/core.c | 4 ++-- drivers/opp/of.c | 10 ++++------ drivers/opp/opp.h | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-)
drivers/opp/core.c +2 -2
@@ -1713,7 +1713,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;
@@ -1792,7 +1792,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 )
drivers/opp/of.c +4 -6
@@ -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 )
drivers/opp/opp.h +1 -1
@@ -247,7 +247,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);
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 3/13 OPP: Reuse _opp_compare_key() in _opp_add_static_v2()
In reply to Viresh KumarReuse _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.
--- drivers/opp/core.c | 6 ++++++ drivers/opp/of.c | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-)
drivers/opp/core.c +6 -0
@@ -1636,6 +1636,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)
drivers/opp/of.c +2 -2
@@ -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 (new_opp, opp_table->suspend_opp) ==1 ) { opp_table->suspend_opp->suspend = false; new_opp->suspend = true; opp_table->suspend_opp = new_opp;
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 4/13 OPP: Make dev_pm_opp_set_opp() independent of frequency
In reply to Viresh Kumardev_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.
--- drivers/opp/core.c | 52 +++++++++++++++++++++++++++++++++------------- drivers/opp/opp.h | 4 ++-- 2 files changed, 40 insertions(+), 16 deletions(-)
drivers/opp/core.c +38 -14
@@ -784,19 +784,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;
@@ -990,7 +1004,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;
@@ -1005,15 +1019,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);
@@ -1046,7 +1059,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;
@@ -1082,7 +1095,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; }
@@ -1103,6 +1115,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);
@@ -1120,7 +1133,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; }
@@ -1141,12 +1155,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;
@@ -1174,7 +1198,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;
drivers/opp/opp.h +2 -2
@@ -159,7 +159,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.
@@ -208,7 +208,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;
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 5/13 dt-bindings: opp: accept array of frequencies
In reply to Viresh KumarFrom: Krzysztof Kozlowski <[email protected]>
Devices might need to control several clocks when scaling the frequency and voltage. Allow passing array of clock frequencies, similarly to the voltages.
--- Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)
Documentation/devicetree/bindings/opp/opp-v2-base.yaml +10 -0
@@ -50,6 +50,16 @@ select: false property to uniquely identify the OPP nodes exists. Devices like power domains must have another (implementation dependent) property. + Entries for multiple clocks shall be provided in the same field, as + array of frequencies. The OPP binding doesn't provide any provisions + to relate the values to their clocks or the order in which the clocks + need to be configured and that is left for the implementation + specific binding. + minItems: 1 + maxItems: 16 + items: + maxItems: 1 + opp-microvolt: description: | Voltage for the OPP
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 6/13 OPP: Allow multiple clocks for a device
In reply to Viresh KumarThis patch adds support to allow multiple clocks for a device.
The design is pretty much similar to how this is done for regulators, and platforms can supply their own version of the config_clks() callback if they have multiple clocks for their device. The core manages the calls via opp_table->config_clks() eventually.
We have kept both "clk" and "clks" fields in the OPP table structure and the reason is provided as a comment in _opp_set_clknames(). The same isn't done for "rates" though and we use rates[0] at most of the places now.
--- drivers/opp/core.c | 199 +++++++++++++++++++++++++++++------------ drivers/opp/debugfs.c | 27 +++++- drivers/opp/of.c | 69 +++++++++++--- drivers/opp/opp.h | 16 ++-- include/linux/pm_opp.h | 7 +- 5 files changed, 235 insertions(+), 83 deletions(-)
drivers/opp/core.c +141 -58
@@ -181,7 +181,7 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)return 0 ; } -return opp->rate; +return opp->rates[0 ]; }EXPORT_SYMBOL_GPL (dev_pm_opp_get_freq);
@@ -430,7 +430,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_count);/* Helpers to read keys */ static unsigned long _read_freq (struct dev_pm_opp *opp,int index) { -return opp->rate; +return opp->rates[0 ]; }static unsigned long _read_level (struct dev_pm_opp *opp,int index)
@@ -784,22 +784,19 @@ 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 opp_table *opp_table,struct dev_pm_opp *opp,void *data) +static int +_opp_config_clk_single (struct device *dev,struct opp_table *opp_table, +struct dev_pm_opp *opp,void *data,bool scaling_down) {unsigned long *target = data;unsigned long freq;int ret; -/* We may reach here for devices which don't change frequency */ -if (IS_ERR (opp_table->clk)) -return 0 ; -/* One of target and opp must be available */ if (target) { freq = *target; }else if (opp) { - freq = opp->rate; + freq = opp->rates[0 ]; }else {WARN_ON (1 );return -EINVAL ;
@@ -1025,11 +1022,11 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, }dev_dbg (dev,"%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n" , - __func__, old_opp->rate, opp->rate, old_opp->level, opp->level, - old_opp->bandwidth ? old_opp->bandwidth[0 ].peak :0 , + __func__, old_opp->rates[0 ], opp->rates[0 ], 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); + scaling_down =_opp_compare_key (opp_table, old_opp, opp);if (scaling_down ==-1 ) scaling_down =0 ;
@@ -1059,9 +1056,11 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, } } - ret =_generic_set_opp_clk_only (dev, opp_table, opp, clk_data); -if (ret) -return ret; +if (opp_table->config_clks) { + ret = opp_table->config_clks (dev, opp_table, opp, clk_data, scaling_down); +if (ret) +return ret; + }/* Scaling down? Configure required OPPs after frequency */ if (scaling_down) {
@@ -1133,8 +1132,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,NULL , - &target_freq); + ret = opp_table->config_clks (dev, opp_table,NULL , + &target_freq, false); goto put_opp_table; }
@@ -1255,6 +1254,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)INIT_LIST_HEAD (&opp_table->dev_list);INIT_LIST_HEAD (&opp_table->lazy); + opp_table->clk =ERR_PTR (-ENODEV ); +/* Mark regulator count uninitialized */ opp_table->regulator_count =-1 ;
@@ -1301,18 +1302,22 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,int ret;/* -* Return early if we don't need to get clk or we have already tried it +* Return early if we don't need to get clk or we have already done it * earlier. */ -if (!getclk ||IS_ERR (opp_table) || opp_table->clk) +if (!getclk ||IS_ERR (opp_table) || !IS_ERR (opp_table->clk) || + opp_table->clks)return opp_table;/* Find clk for the device */ opp_table->clk =clk_get (dev,NULL ); ret =PTR_ERR_OR_ZERO (opp_table->clk); -if (!ret) +if (!ret) { + opp_table->config_clks = _opp_config_clk_single; + opp_table->clk_count =1 ;return opp_table; + }if (ret == -ENOENT ) {dev_dbg (dev,"%s: Couldn't find clock: %d\n" , __func__, ret);
@@ -1417,7 +1422,7 @@ static void _opp_table_kref_release(struct kref *kref)_of_clear_opp_table (opp_table); -/* Release clk */ +/* Release automatically acquired single clk */ if (!IS_ERR (opp_table->clk))clk_put (opp_table->clk);
@@ -1505,7 +1510,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)mutex_lock (&opp_table->lock);list_for_each_entry (iter, &opp_table->opp_list, node) { -if (iter->rate == freq) { +if (iter->rates[0 ] == freq) { opp = iter;break ; }
@@ -1612,24 +1617,28 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic);struct dev_pm_opp *_opp_allocate (struct opp_table *opp_table) {struct dev_pm_opp *opp; -int supply_count, supply_size, icc_size; +int supply_count, supply_size, icc_size, clk_size;/* Allocate space for at least one supply */ supply_count = opp_table->regulator_count >0 ? opp_table->regulator_count :1 ; supply_size =sizeof (*opp->supplies) * supply_count; + clk_size =sizeof (*opp->rates) * opp_table->clk_count; icc_size =sizeof (*opp->bandwidth) * opp_table->path_count;/* allocate new OPP node and supplies structures */ - opp =kzalloc (sizeof (*opp) + supply_size + icc_size,GFP_KERNEL ); - + opp =kzalloc (sizeof (*opp) + supply_size + clk_size + icc_size,GFP_KERNEL );if (!opp)return NULL ; -/* Put the supplies at the end of the OPP structure as an empty array */ +/* Put the supplies, bw and clock at the end of the OPP structure */ opp->supplies = (struct dev_pm_opp_supply *)(opp +1 ); + + opp->rates = (unsigned long *)(opp->supplies + supply_count); +if (icc_size) - opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies + supply_count); + opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->rates + opp_table->clk_count); +INIT_LIST_HEAD (&opp->node);return opp;
@@ -1660,21 +1669,43 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,return true; } +static int _opp_compare_rate (struct opp_table *opp_table, +struct dev_pm_opp *opp1,struct dev_pm_opp *opp2) +{ +int i; + +for (i =0 ; i < opp_table->clk_count; i++) { +if (opp1->rates[i] != opp2->rates[i]) +return opp1->rates[i] < opp2->rates[i] ?-1 :1 ; + } + +/* Same rates for both OPPs */ +return 0 ; +} +/* * Returns * 0: opp1 == opp2 * 1: opp1 > opp2 * -1: opp1 < opp2 */ -int _opp_compare_key (struct dev_pm_opp *opp1,struct dev_pm_opp *opp2) +int _opp_compare_key (struct opp_table *opp_table,struct dev_pm_opp *opp1, +struct dev_pm_opp *opp2) { -if (opp1->rate != opp2->rate) -return opp1->rate < opp2->rate ?-1 :1 ; +int ret; + + ret =_opp_compare_rate (opp_table, opp1, opp2); +if (ret) +return ret; +if (opp1->bandwidth && opp2->bandwidth && opp1->bandwidth[0 ].peak != opp2->bandwidth[0 ].peak)return opp1->bandwidth[0 ].peak < opp2->bandwidth[0 ].peak ?-1 :1 ; +if (opp1->level != opp2->level)return opp1->level < opp2->level ?-1 :1 ; + +/* Duplicate OPPs */ return 0 ; }
@@ -1694,7 +1725,7 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,* loop. */ list_for_each_entry (opp, &opp_table->opp_list, node) { - opp_cmp =_opp_compare_key (new_opp, opp); + opp_cmp =_opp_compare_key (opp_table, new_opp, opp);if (opp_cmp >0 ) { *head = &opp->node;continue ;
@@ -1705,8 +1736,8 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,/* Duplicate OPPs */ dev_warn (dev,"%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n" , - __func__, opp->rate, opp->supplies[0 ].u_volt, - opp->available, new_opp->rate, + __func__, opp->rates[0 ], opp->supplies[0 ].u_volt, + opp->available, new_opp->rates[0 ], new_opp->supplies[0 ].u_volt, new_opp->available);/* Should we compare voltages for all regulators here ? */
@@ -1727,7 +1758,7 @@ void _required_opps_available(struct dev_pm_opp *opp, int count) opp->available = false;pr_warn ("%s: OPP not supported by required OPP %pOF (%lu)\n" , - __func__, opp->required_opps[i]->np, opp->rate); + __func__, opp->required_opps[i]->np, opp->rates[0 ]);return ; } }
@@ -1768,7 +1799,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,if (!_opp_supported_by_regulators (new_opp, opp_table)) { new_opp->available = false;dev_warn (dev,"%s: OPP not supported by regulators (%lu)\n" , - __func__, new_opp->rate); + __func__, new_opp->rates[0 ]); }/* required-opps not fully initialized yet */
@@ -1814,7 +1845,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,return -ENOMEM ;/* populate the opp table */ - new_opp->rate = freq; + new_opp->rates[0 ] = freq; tol = u_volt * opp_table->voltage_tolerance_v1 /100 ; new_opp->supplies[0 ].u_volt = u_volt; new_opp->supplies[0 ].u_volt_min = u_volt - tol;
@@ -2017,6 +2048,17 @@ static void _opp_put_regulators(struct opp_table *opp_table) opp_table->regulator_count =-1 ; } +static void _put_clks (struct opp_table *opp_table,int count) +{ +int i; + +for (i = count -1 ; i >=0 ; i--) +clk_put (opp_table->clks[i]); + +kfree (opp_table->clks); + opp_table->clks =NULL ; +} +/** * _opp_set_clknames() - Set clk names for the device * @dev: Device for which clk names is being set.
@@ -2031,10 +2073,12 @@ static void _opp_put_regulators(struct opp_table *opp_table)* This must be called before any OPPs are initialized for the device. */ static int _opp_set_clknames (struct opp_table *opp_table,struct device *dev, -const char *const names[]) +const char *const names[], +config_clks_t config_clks) {const char *const *temp = names; -int count =0 ; +int count =0 , ret, i; +struct clk *clk;/* Count number of clks */ while (*temp++)
@@ -2047,28 +2091,60 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,if (!count && !names[1 ]) count =1 ; -/* We support only one clock name for now */ -if (count !=1 ) +/* Fail early for invalid configurations */ +if (!count || (config_clks && count ==1 ) || (!config_clks && count >1 ))return -EINVAL ;/* Another CPU that shares the OPP table has set the clkname ? */ -if (opp_table->clk_configured) +if (opp_table->clks)return 0 ; -/* clk shouldn't be initialized at this point */ -if (WARN_ON (opp_table->clk)) -return -EBUSY ; + opp_table->clks =kmalloc_array (count,sizeof (*opp_table->clks), +GFP_KERNEL ); +if (!opp_table->clks) +return -ENOMEM ; -/* Find clk for the device */ - opp_table->clk =clk_get (dev, names[0 ]); -if (IS_ERR (opp_table->clk)) { -return dev_err_probe (dev,PTR_ERR (opp_table->clk), -"%s: Couldn't find clock\n" , __func__); +/* Find clks for the device */ +for (i =0 ; i < count; i++) { + clk =clk_get (dev, names[i]); +if (IS_ERR (clk)) { + ret =dev_err_probe (dev,PTR_ERR (clk), +"%s: Couldn't find clock with name: %s\n" , + __func__, names[i]); + goto free_clks; + } + + opp_table->clks[i] = clk; } - opp_table->clk_configured = true; + opp_table->clk_count = count; + +/* Set generic single clk set here */ +if (count ==1 ) { + opp_table->config_clks = _opp_config_clk_single; + +/* +* We could have just dropped the "clk" field and used "clks" +* everywhere. Instead we kept the "clk" field around for +* following reasons: +* +* - avoiding clks[0] everywhere else. +* - not running single clk helpers for multiple clk usecase by +* mistake. +* +* Since this is single-clk case, just update the clk pointer +* too. +*/ + opp_table->clk = opp_table->clks[0 ]; + }else { + opp_table->config_clks = config_clks; + }return 0 ; + +free_clks: +_put_clks (opp_table, i); +return ret; }/**
@@ -2077,11 +2153,13 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,*/ static void _opp_put_clknames (struct opp_table *opp_table) { -if (opp_table->clk_configured) { -clk_put (opp_table->clk); - opp_table->clk =ERR_PTR (-EINVAL ); - opp_table->clk_configured = false; - } +if (!opp_table->clks) +return ; + + opp_table->config_clks =NULL ; + opp_table->clk =ERR_PTR (-ENODEV ); + +_put_clks (opp_table, opp_table->clk_count); }/**
@@ -2298,11 +2376,16 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)/* Configure clocks */ if (config->clk_names) { - ret =_opp_set_clknames (opp_table, dev, config->clk_names); + ret =_opp_set_clknames (opp_table, dev, config->clk_names, + config->config_clks);if (ret) goto err; data->flags |=OPP_CONFIG_CLK ; + }else if (config->config_clks) { +/* Don't allow config callback without clocks */ + ret = -EINVAL ; + goto err; }/* Configure property names */
@@ -2614,7 +2697,7 @@ static int _opp_set_availability(struct device *dev, unsigned long freq,/* Do we have the frequency? */ list_for_each_entry (tmp_opp, &opp_table->opp_list, node) { -if (tmp_opp->rate == freq) { +if (tmp_opp->rates[0 ] == freq) { opp = tmp_opp;break ; }
@@ -2685,7 +2768,7 @@ int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,/* Do we have the frequency? */ list_for_each_entry (tmp_opp, &opp_table->opp_list, node) { -if (tmp_opp->rate == freq) { +if (tmp_opp->rates[0 ] == freq) { opp = tmp_opp;break ; }
drivers/opp/debugfs.c +23 -4
@@ -74,6 +74,24 @@ static void opp_debug_create_bw(struct dev_pm_opp *opp, } } +static void opp_debug_create_clks (struct dev_pm_opp *opp, +struct opp_table *opp_table, +struct dentry *pdentry) +{ +char name[12 ]; +int i; + +if (opp_table->clk_count ==1 ) { +debugfs_create_ulong ("rate_hz" ,S_IRUGO , pdentry, &opp->rates[0 ]); +return ; + } + +for (i =0 ; i < opp_table->clk_count; i++) { +snprintf (name,sizeof (name),"rate_hz_%d" , i); +debugfs_create_ulong (name,S_IRUGO , pdentry, &opp->rates[i]); + } +} +static void opp_debug_create_supplies (struct dev_pm_opp *opp,struct opp_table *opp_table,struct dentry *pdentry)
@@ -117,10 +135,11 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)* Get directory name for OPP. * * - Normally rate is unique to each OPP, use it to get unique opp-name. -* - For some devices rate isn't available, use index instead. +* - For some devices rate isn't available or there are multiple, use +* index instead for them. */ -if (likely (opp->rate)) - id = opp->rate; +if (likely (opp_table->clk_count ==1 )) + id = opp->rates[0 ];else id =_get_opp_count (opp_table);
@@ -134,7 +153,6 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)debugfs_create_bool ("turbo" ,S_IRUGO , d, &opp->turbo);debugfs_create_bool ("suspend" ,S_IRUGO , d, &opp->suspend);debugfs_create_u32 ("performance_state" ,S_IRUGO , d, &opp->pstate); -debugfs_create_ulong ("rate_hz" ,S_IRUGO , d, &opp->rate);debugfs_create_u32 ("level" ,S_IRUGO , d, &opp->level);debugfs_create_ulong ("clock_latency_ns" ,S_IRUGO , d, &opp->clock_latency_ns);
@@ -142,6 +160,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) opp->of_name =of_node_full_name (opp->np);debugfs_create_str ("of_name" ,S_IRUGO , d, (char **)&opp->of_name); +opp_debug_create_clks (opp, opp_table, d);opp_debug_create_supplies (opp, opp_table, d);opp_debug_create_bw (opp, opp_table, d);
drivers/opp/of.c +55 -14
102 lines not on this page · raw message
drivers/opp/opp.h +10 -6
@@ -79,7 +79,7 @@ struct opp_config_data {* @suspend: true if suspend OPP * @removed: flag indicating that OPP's reference is dropped by OPP core. * @pstate: Device's power domain's performance state. -* @rate: Frequency in hertz +* @rates: Frequencies in hertz * @level: Performance level * @supplies: Power supplies voltage/current values * @bandwidth: Interconnect bandwidth values
@@ -102,7 +102,7 @@ struct dev_pm_opp {bool suspend;bool removed;unsigned int pstate; -unsigned long rate; +unsigned long *rates;unsigned int level;struct dev_pm_opp_supply *supplies;
@@ -170,8 +170,10 @@ enum opp_table_access {* @supported_hw: Array of version number to support. * @supported_hw_count: Number of elements in supported_hw array. * @prop_name: A name to postfix to many DT properties, while parsing them. -* @clk_configured: Clock name is configured by the platform. -* @clk: Device's clock handle +* @config_clks: Platform specific config_clks() callback. +* @clks: Device's clock handles, for multiple clocks. +* @clk: Device's clock handle, for single clock. +* @clk_count: Number of clocks. * @config_regulators: Platform specific config_regulators() callback. * @regulators: Supply regulators * @regulator_count: Number of power supply regulators. Its value can be -1
@@ -220,8 +222,10 @@ struct opp_table {unsigned int *supported_hw;unsigned int supported_hw_count;const char *prop_name; -bool clk_configured; +config_clks_t config_clks; +struct clk **clks;struct clk *clk; +int clk_count;config_regulators_t config_regulators;struct regulator **regulators;int regulator_count;
@@ -246,7 +250,7 @@ struct opp_table *_find_opp_table(struct device *dev);struct opp_device *_add_opp_dev (const struct device *dev,struct opp_table *opp_table);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_compare_key (struct opp_table *opp_table,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);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);
include/linux/pm_opp.h +6 -1
@@ -61,9 +61,13 @@ typedef int (*config_regulators_t)(struct device *dev, struct dev_pm_opp *old_opp, struct dev_pm_opp *new_opp, struct regulator **regulators, unsigned int count); +typedef int (*config_clks_t )(struct device *dev,struct opp_table *opp_table, +struct dev_pm_opp *opp,void *data,bool scaling_down); +/** * struct dev_pm_opp_config - Device OPP configuration values -* @clk_names: Clk names, NULL terminated array, max 1 clock for now. +* @clk_names: Clk names, NULL terminated array. +* @config_clks: Custom set clk helper. * @prop_name: Name to postfix to properties. * @config_regulators: Custom set regulator helper. * @supported_hw: Array of hierarchy of versions to match.
@@ -78,6 +82,7 @@ typedef int (*config_regulators_t)(struct device *dev,struct dev_pm_opp_config {/* NULL terminated */ const char *const *clk_names; +config_clks_t config_clks;const char *prop_name;config_regulators_t config_regulators;const unsigned int *supported_hw;
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 7/13 OPP: Compare bandwidths for all paths in _opp_compare_key()
In reply to Viresh KumarReplicate the same behavior as "rates" here and compare all values instead of relying on the first entry alone.
--- drivers/opp/core.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
drivers/opp/core.c +17 -3
@@ -1683,6 +1683,20 @@ static int _opp_compare_rate(struct opp_table *opp_table,return 0 ; } +static int _opp_compare_bw (struct opp_table *opp_table,struct dev_pm_opp *opp1, +struct dev_pm_opp *opp2) +{ +int i; + +for (i =0 ; i < opp_table->path_count; i++) { +if (opp1->bandwidth[i].peak != opp2->bandwidth[i].peak) +return opp1->bandwidth[i].peak < opp2->bandwidth[i].peak ?-1 :1 ; + } + +/* Same bw for both OPPs */ +return 0 ; +} +/* * Returns * 0: opp1 == opp2
@@ -1698,9 +1712,9 @@ int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1,if (ret)return ret; -if (opp1->bandwidth && opp2->bandwidth && - opp1->bandwidth[0 ].peak != opp2->bandwidth[0 ].peak) -return opp1->bandwidth[0 ].peak < opp2->bandwidth[0 ].peak ?-1 :1 ; + ret =_opp_compare_bw (opp_table, opp1, opp2); +if (ret) +return ret;if (opp1->level != opp2->level)return opp1->level < opp2->level ?-1 :1 ;
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 8/13 OPP: Add key specific assert() method to key finding helpers
In reply to Viresh KumarThe 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.
--- drivers/opp/core.c | 52 +++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 19 deletions(-)
drivers/opp/core.c +33 -19
@@ -481,10 +481,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) {
@@ -509,7 +514,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;
@@ -522,7 +528,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);
@@ -531,35 +537,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); }/**
@@ -588,14 +601,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 ); }/**
@@ -619,7 +633,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);
@@ -644,7 +658,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);
@@ -666,7 +680,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);
@@ -691,7 +705,7 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,unsigned long temp = *level;struct dev_pm_opp *opp; - opp =_find_key_ceil (dev, &temp,0 , true, _read_level); + opp =_find_key_ceil (dev, &temp,0 , true, _read_level,NULL ); *level = temp;return opp; }
@@ -722,7 +736,7 @@ struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev, unsigned int *bw,unsigned long temp = *bw;struct dev_pm_opp *opp; - opp =_find_key_ceil (dev, &temp, index, true, _read_bw); + opp =_find_key_ceil (dev, &temp, index, true, _read_bw,NULL ); *bw = temp;return opp; }
@@ -753,7 +767,7 @@ struct dev_pm_opp *dev_pm_opp_find_bw_floor(struct device *dev,unsigned long temp = *bw;struct dev_pm_opp *opp; - opp =_find_key_floor (dev, &temp, index, true, _read_bw); + opp =_find_key_floor (dev, &temp, index, true, _read_bw,NULL ); *bw = temp;return opp; }
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 9/13 OPP: Assert clk_count == 1 for single clk helpers
In reply to Viresh KumarMany helpers can be safely called only for devices that have a single clk associated with them. Assert the same for those routines.
--- drivers/opp/core.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)
drivers/opp/core.c +39 -6
@@ -97,6 +97,18 @@ struct opp_table *_find_opp_table(struct device *dev)return opp_table; } +/* +* Returns true if multiple clocks aren't there, else returns false with WARN. +* +* We don't force clk_count == 1 here as there are users who don't have a clock +* representation in the OPP table and manage the clock configuration themselves +* in an platform specific way. +*/ +static bool assert_single_clk (struct opp_table *opp_table) +{ +return !WARN_ON (opp_table->clk_count >1 ); +} +/** * dev_pm_opp_get_voltage() - Gets the voltage corresponding to an opp * @opp: opp for which voltage has to be returned for
@@ -181,6 +193,9 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)return 0 ; } +if (!assert_single_clk (opp->opp_table)) +return 0 ; +return opp->rates[0 ]; }EXPORT_SYMBOL_GPL (dev_pm_opp_get_freq);
@@ -601,7 +616,8 @@ 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,NULL ); +return _find_key_exact (dev, freq,0 , available, _read_freq, + assert_single_clk); }EXPORT_SYMBOL_GPL (dev_pm_opp_find_freq_exact);
@@ -609,7 +625,7 @@ 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, -NULL ); + assert_single_clk); }/**
@@ -633,7 +649,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,NULL ); +return _find_key_ceil (dev, freq,0 , true, _read_freq, assert_single_clk); }EXPORT_SYMBOL_GPL (dev_pm_opp_find_freq_ceil);
@@ -658,7 +674,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,NULL ); +return _find_key_floor (dev, freq,0 , true, _read_freq, assert_single_clk); }EXPORT_SYMBOL_GPL (dev_pm_opp_find_freq_floor);
@@ -1521,6 +1537,9 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)if (IS_ERR (opp_table))return ; +if (!assert_single_clk (opp_table)) + goto put_table; +mutex_lock (&opp_table->lock);list_for_each_entry (iter, &opp_table->opp_list, node) {
@@ -1542,6 +1561,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) __func__, freq); } +put_table:/* Drop the reference taken by _find_opp_table() */ dev_pm_opp_put_opp_table (opp_table); }
@@ -1868,6 +1888,9 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,unsigned long tol;int ret; +if (!assert_single_clk (opp_table)) +return -EINVAL ; + new_opp =_opp_allocate (opp_table);if (!new_opp)return -ENOMEM ;
@@ -2721,6 +2744,11 @@ static int _opp_set_availability(struct device *dev, unsigned long freq,return r; } +if (!assert_single_clk (opp_table)) { + r = -EINVAL ; + goto put_table; + } +mutex_lock (&opp_table->lock);/* Do we have the frequency? */
@@ -2792,6 +2820,11 @@ int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,return r; } +if (!assert_single_clk (opp_table)) { + r = -EINVAL ; + goto put_table; + } +mutex_lock (&opp_table->lock);/* Do we have the frequency? */
@@ -2823,11 +2856,11 @@ int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq, opp);dev_pm_opp_put (opp); - goto adjust_put_table; + goto put_table; adjust_unlock:mutex_unlock (&opp_table->lock); -adjust_put_table: +put_table:dev_pm_opp_put_opp_table (opp_table);return r; }
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH V2 09/13] OPP: Assert clk_count == 1 for single clk helpers
Quoted 3 lines · Viresh Kumar
Many helpers can be safely called only for devices that have a single clk associated with them. Assert the same for those routines.
Where is the safety problem with multiple-clocks case? And how users of PM OPP API are supposed to iterate/find OPPs they want if the API now throws WARN?
Best regards,
Krzysztof
Re: [PATCH V2 09/13] OPP: Assert clk_count == 1 for single clk helpers
Quoted 1 line · Krzysztof Kozlowski
Where is the safety problem with multiple-clocks case?
All these APIs, which I have added the assert to now, are designed with a single clk/freq in mind. They simply take a clock frequency value as input and do something based on it. It only works fine with single clk case, as more information is required for multiple clock case, a freq value isn't sufficient here. In order to avoid abuse or accidental use of the wrong API, these WARNs were required.
Quoted 2 lines
And how users of PM OPP API are supposed to iterate/find OPPs they want if the API now throws WARN?
We need to provide new APIs for that, as I mentioned in the cover letter and the other mail I sent you yesterday.
Specifically, if we want to have OPP finder API based on freq value, then it needs to either have index + freq as input, or an array of frequencies (one for each clk) as input.
Though I am not sure what you would need at the moment, as you can also use opp-level for OPPs as they match UFS gears, that was my understanding from earlier discussion.
Signature · 1 line
viresh
PATCH v2 10/13 OPP: Provide a simple implementation to configure multiple clocks
In reply to Viresh KumarThis provides a simple implementation to configure multiple clocks for a device.
--- drivers/opp/core.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 10 ++++++++++ 2 files changed, 44 insertions(+)
drivers/opp/core.c +34 -0
@@ -843,6 +843,40 @@ _opp_config_clk_single(struct device *dev, struct opp_table *opp_table,return ret; } +/* +* Simple implementation for configuring multiple clocks. Configure clocks in +* the order in which they are present in the array while scaling up. +*/ +int dev_pm_opp_config_clks_simple (struct device *dev, +struct opp_table *opp_table,struct dev_pm_opp *opp,void *data, +bool scaling_down) +{ +int ret, i; + +if (scaling_down) { +for (i = opp_table->clk_count -1 ; i >=0 ; i--) { + ret =clk_set_rate (opp_table->clks[i], opp->rates[i]); +if (ret) { +dev_err (dev,"%s: failed to set clock rate: %d\n" , __func__, + ret); +return ret; + } + } + }else { +for (i =0 ; i < opp_table->clk_count; i++) { + ret =clk_set_rate (opp_table->clks[i], opp->rates[i]); +if (ret) { +dev_err (dev,"%s: failed to set clock rate: %d\n" , __func__, + ret); +return ret; + } + } + } + +return ret; +} +EXPORT_SYMBOL_GPL (dev_pm_opp_config_clks_simple); +static int _opp_config_regulator_single (struct device *dev,struct dev_pm_opp *old_opp,struct dev_pm_opp *new_opp,struct regulator **regulators,unsigned int count)
include/linux/pm_opp.h +10 -0
@@ -159,6 +159,9 @@ int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nbint dev_pm_opp_set_config (struct device *dev,struct dev_pm_opp_config *config);int devm_pm_opp_set_config (struct device *dev,struct dev_pm_opp_config *config);void dev_pm_opp_clear_config (int token); +int dev_pm_opp_config_clks_simple (struct device *dev, +struct opp_table *opp_table,struct dev_pm_opp *opp,void *data, +bool scaling_down);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);
@@ -342,6 +345,13 @@ static inline int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_cstatic inline void dev_pm_opp_clear_config (int token) {} +static inline int dev_pm_opp_config_clks_simple (struct device *dev, +struct opp_table *opp_table,struct dev_pm_opp *opp,void *data, +bool scaling_down) +{ +return -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) {
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 11/13 OPP: Allow config_clks helper for single clk case
In reply to Viresh KumarThere is a corner case with Tegra30, where we want to skip clk configuration via dev_pm_opp_set_opp(), but still want the OPP core to read the "opp-hz" property so we can find the right OPP via freq finding helpers.
This is the easiest of the ways to make it work, without any special hacks in the OPP core. Allow config_clks to be passed for single clk case.
--- drivers/opp/core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/opp/core.c +4 -4
@@ -2177,7 +2177,7 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev, count =1 ;/* Fail early for invalid configurations */ -if (!count || (config_clks && count ==1 ) || (!config_clks && count >1 )) +if (!count || (!config_clks && count >1 ))return -EINVAL ;/* Another CPU that shares the OPP table has set the clkname ? */
@@ -2203,10 +2203,12 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev, } opp_table->clk_count = count; + opp_table->config_clks = config_clks;/* Set generic single clk set here */ if (count ==1 ) { - opp_table->config_clks = _opp_config_clk_single; +if (!opp_table->config_clks) + opp_table->config_clks = _opp_config_clk_single;/* * We could have just dropped the "clk" field and used "clks"
@@ -2221,8 +2223,6 @@ static int _opp_set_clknames(struct opp_table *opp_table, struct device *dev,* too. */ opp_table->clk = opp_table->clks[0 ]; - }else { - opp_table->config_clks = config_clks; }return 0 ;
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 12/13 PM / devfreq: tegra30: Register config_clks helper
In reply to Viresh KumarThere is a corner case with Tegra30, where we want to skip clk configuration that happens via dev_pm_opp_set_opp(), but still want the OPP core to read the "opp-hz" property so we can find the right OPP via freq finding helpers.
The OPP core provides support for the platforms to provide config_clks helpers now, lets use that instead of devm_pm_opp_of_add_table_noclk() to achieve the same result, as the OPP core won't parse the DT's "opp-hz" property anymore if the clock isn't provided.
--- drivers/devfreq/tegra30-devfreq.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
drivers/devfreq/tegra30-devfreq.c +19 -3
@@ -821,6 +821,15 @@ static int devm_tegra_devfreq_init_hw(struct device *dev,return err; } +static int tegra_devfreq_config_clks_nop (struct device *dev, +struct opp_table *opp_table, +struct dev_pm_opp *opp,void *data, +bool scaling_down) +{ +/* We want to skip clk configuration via dev_pm_opp_set_opp() */ +return 0 ; +} +static int tegra_devfreq_probe (struct platform_device *pdev) {u32 hw_version =BIT (tegra_sku_info.soc_speedo_id);
@@ -830,6 +839,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)unsigned int i;long rate;int err; +const char *clk_names[] = {"actmon" ,NULL }; +struct dev_pm_opp_config config = { + .supported_hw = &hw_version, + .supported_hw_count =1 , + .clk_names = clk_names, + .config_clks = tegra_devfreq_config_clks_nop, + }; tegra =devm_kzalloc (&pdev->dev,sizeof (*tegra),GFP_KERNEL );if (!tegra)
@@ -874,13 +890,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)return err; } - err =devm_pm_opp_set_supported_hw (&pdev->dev, &hw_version,1 ); + err =devm_pm_opp_set_config (&pdev->dev, &config);if (err) { -dev_err (&pdev->dev,"Failed to set supported HW: %d\n" , err); +dev_err (&pdev->dev,"Failed to set OPP config: %d\n" , err);return err; } - err =devm_pm_opp_of_add_table_noclk (&pdev->dev,0 ); + err =devm_pm_opp_of_add_table_indexed (&pdev->dev,0 );if (err) {dev_err (&pdev->dev,"Failed to add OPP table: %d\n" , err);return err;
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v2 13/13 OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
In reply to Viresh KumarRemove the now unused variants and the extra "getclk" parameter from few routines, which is always "true" now.
--- drivers/opp/core.c | 19 +++++++---------- drivers/opp/of.c | 48 +++++++----------------------------------- drivers/opp/opp.h | 2 +- include/linux/pm_opp.h | 12 ----------- 4 files changed, 17 insertions(+), 64 deletions(-)
drivers/opp/core.c +8 -11
@@ -1360,8 +1360,7 @@ void _get_opp_table_kref(struct opp_table *opp_table) }static struct opp_table *_update_opp_table_clk (struct device *dev, -struct opp_table *opp_table, -bool getclk) +struct opp_table *opp_table) {int ret;
@@ -1369,8 +1368,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,* Return early if we don't need to get clk or we have already done it * earlier. */ -if (!getclk ||IS_ERR (opp_table) || !IS_ERR (opp_table->clk) || - opp_table->clks) +if (IS_ERR (opp_table) || !IS_ERR (opp_table->clk) || opp_table->clks)return opp_table;/* Find clk for the device */
@@ -1409,8 +1407,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,* uses the opp_tables_busy flag to indicate if another creator is in the middle * of adding an OPP table and others should wait for it to finish. */ -struct opp_table *_add_opp_table_indexed (struct device *dev,int index, -bool getclk) +struct opp_table *_add_opp_table_indexed (struct device *dev,int index) {struct opp_table *opp_table;
@@ -1457,12 +1454,12 @@ struct opp_table *_add_opp_table_indexed(struct device *dev, int index, unlock:mutex_unlock (&opp_table_lock); -return _update_opp_table_clk (dev, opp_table, getclk); +return _update_opp_table_clk (dev, opp_table); } -static struct opp_table *_add_opp_table (struct device *dev,bool getclk) +static struct opp_table *_add_opp_table (struct device *dev) { -return _add_opp_table_indexed (dev,0 , getclk); +return _add_opp_table_indexed (dev,0 ); }struct opp_table *dev_pm_opp_get_opp_table (struct device *dev)
@@ -2444,7 +2441,7 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)if (!data)return -ENOMEM ; - opp_table =_add_opp_table (dev, false); + opp_table =_add_opp_table (dev);if (IS_ERR (opp_table)) {kfree (data);return PTR_ERR (opp_table);
@@ -2735,7 +2732,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)struct opp_table *opp_table;int ret; - opp_table =_add_opp_table (dev, true); + opp_table =_add_opp_table (dev);if (IS_ERR (opp_table))return PTR_ERR (opp_table);
drivers/opp/of.c +8 -40
96 lines not on this page · raw message
drivers/opp/opp.h +1 -1
@@ -254,7 +254,7 @@ int _opp_compare_key(struct opp_table *opp_table, struct dev_pm_opp *opp1, strucint _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); +struct opp_table *_add_opp_table_indexed (struct device *dev,int index);void _put_opp_list_kref (struct opp_table *opp_table);void _required_opps_available (struct dev_pm_opp *opp,int count);
include/linux/pm_opp.h +0 -12
@@ -402,8 +402,6 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev)int dev_pm_opp_of_add_table (struct device *dev);int dev_pm_opp_of_add_table_indexed (struct device *dev,int index);int devm_pm_opp_of_add_table_indexed (struct device *dev,int index); -int dev_pm_opp_of_add_table_noclk (struct device *dev,int index); -int devm_pm_opp_of_add_table_noclk (struct device *dev,int index);void dev_pm_opp_of_remove_table (struct device *dev);int devm_pm_opp_of_add_table (struct device *dev);int dev_pm_opp_of_cpumask_add_table (const struct cpumask *cpumask);
@@ -434,16 +432,6 @@ static inline int devm_pm_opp_of_add_table_indexed(struct device *dev, int indexreturn -EOPNOTSUPP ; } -static inline int dev_pm_opp_of_add_table_noclk (struct device *dev,int index) -{ -return -EOPNOTSUPP ; -} - -static inline int devm_pm_opp_of_add_table_noclk (struct device *dev,int index) -{ -return -EOPNOTSUPP ; -} -static inline void dev_pm_opp_of_remove_table (struct device *dev) { }
Signature · 1 line
2.31.1.272.g89b43f80a514
Quoted 42 lines · Viresh Kumar
Hello,
This patchset adds support for devices with multiple clocks. None of the clocks is considered primary in this case and all are handled equally.
The drivers, for multiple clock case, are expected to call
dev_pm_opp_set_opp()to set the specific OPP. Though how they find the target OPP is left for the users to handle. For some, we may have another unique OPP property, like level, which can be used to find the OPP. While in case of others, we may want to implement freq-based OPP finder APIs for multiple clock rates. I have decided not to implement them in advance, and add them only someone wants to use them.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.gitopp/linux-next
V1->V2:- Fix broken git bisect for:
OPP: Reuse_opp_compare_key()in_opp_add_static_v2()- Include binding changes written by Krzysztof earlier.
- Check
config_clksbefore calling it, it isn't always set.- Add
config_clksfor Tegra30's devfreq to handle its corner case.-
_opp_compare_key()supports multi-clk case now, earlier it skipped freq comparison for such a case.- New patch to compare all bandwidth values as well in
_opp_compare_key().- New patch to remove
*_noclk()interface.- Various other minor fixes.
--
VireshKrzysztof Kozlowski (1):
dt-bindings: opp: accept array of frequencies
Quoted 12 lines
Viresh Kumar (12):
OPP: Use consistent names for OPP table instances
OPP: Removerate_not_availableparameter to_opp_add()
OPP: Reuse_opp_compare_key()in_opp_add_static_v2()
OPP: Makedev_pm_opp_set_opp()independent of frequency
OPP: Allow multiple clocks for a device
OPP: Compare bandwidths for all paths in_opp_compare_key()
OPP: Add key specificassert()method to key finding helpers
OPP: Assertclk_count== 1 for single clk helpers
OPP: Provide a simple implementation to configure multiple clocks
OPP: Allowconfig_clkshelper for single clk case
PM / devfreq: tegra30: Registerconfig_clkshelper
Hello Viresh,
This patch breaks Tegra again, please take a look:
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
8<--- cut here ---
Unable to handle kernel paging request at virtual address ffffffff [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000 Internal error: Oops: 37 [#1] PREEMPT SMP ARM Modules linked in: CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.19.0-rc1-00040-g30b62d123f4f #82 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events_unbound deferred_probe_work_func PC is at _opp_compare_key+0x40/0xc4 LR is at 0xfffffffb pc : [<c0b91b54>] lr : [<fffffffb>] psr: 20000113 sp : df831b08 ip : c33cd4d0 fp : df831b24 r10: c2586078 r9 : c258606c r8 : 00000000 r7 : 00000000 r6 : 00000001 r5 : c33cd480 r4 : c2586000 r3 : 00000000 r2 : c33cd480 r1 : c258606c r0 : c2586000 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 8000404a DAC: 00000051 ... Backtrace: _opp_compare_key from _set_opp+0x80/0x408 r7:00000000 r6:c27c0010 r5:c33cd480 r4:c2586000 _set_opp from dev_pm_opp_set_opp+0x74/0xdc r10:00000001 r9:000f4240 r8:c33b6840 r7:c33b6840 r6:c33cd480 r5:c27c0010 r4:c2586000 dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x54/0xbc r6:c33b6840 r5:c1a21760 r4:c33cd480 tegra_pmc_core_pd_set_performance_state from _genpd_set_performance_state+0x1f0/0x280 r6:c33b6b58 r5:c33b6b48 r4:000f4240 _genpd_set_performance_state from _genpd_set_performance_state+0xb4/0x280 r10:00000001 r9:000f4240 r8:c33b9800 r7:c33b6840 r6:c33b9b18 r5:c33d0040 r4:000f4240 _genpd_set_performance_state from genpd_set_performance_state+0xb8/0xd4 r10:c33b9a98 r9:c33d0400 r8:00000000 r7:00000000 r6:c33b9800 r5:00000000 r4:c33d0400 genpd_set_performance_state from genpd_runtime_resume+0x22c/0x240 r5:00000000 r4:c27c0810 genpd_runtime_resume from __rpm_callback+0x4c/0x1ac r10:c27ba8bc r9:00000000 r8:c27c0960 r7:c27c08cc r6:c27ba800 r5:c09611b8 r4:c27c0810 __rpm_callback from rpm_callback+0x60/0x64 r9:df831ce4 r8:c27c0960 r7:00000004 r6:c27ba800 r5:c09611b8 r4:c27c0810 rpm_callback from rpm_resume+0x480/0x7e0 r7:00000004 r6:c27ba800 r5:c09611b8 r4:c27c0810 rpm_resume from __pm_runtime_resume+0x58/0xb0 r10:00000000 r9:c2587194 r8:c2587210 r7:c27c0810 r6:c27c08cc r5:60000113 r4:c27c0810 __pm_runtime_resume from host1x_probe+0x3d4/0x6d4 r7:c27c0810 r6:c27c0800 r5:00000000 r4:c2587040 host1x_probe from platform_probe+0x6c/0xc0 r10:c191438c r9:c1aa8e20 r8:0000000d r7:c27c0810 r6:c1a2d354 r5:c27c0810 r4:00000000 platform_probe from really_probe.part.0+0xac/0x2c0 r7:c27c0810 r6:c1a2d354 r5:c27c0810 r4:00000000 really_probe.part.0 from __driver_probe_device+0xb8/0x14c r7:c27c0810 r6:c1a2d354 r5:00000000 r4:c27c0810 __driver_probe_device from driver_probe_device+0x44/0x11c r7:c27c0810 r6:c27c0810 r5:c2137c58 r4:c2137c54 driver_probe_device from __device_attach_driver+0xc8/0x10c r9:c1aa8e20 r8:c242c000 r7:00000000 r6:c27c0810 r5:df831e6c r4:c1a2d354 __device_attach_driver from bus_for_each_drv+0x90/0xdc r7:00000000 r6:c09440f8 r5:df831e6c r4:00000000 bus_for_each_drv from __device_attach+0xbc/0x1d4 r6:c27c0854 r5:00000001 r4:c27c0810 __device_attach from device_initial_probe+0x1c/0x20 r6:c1a30df8 r5:c27c0810 r4:c27c0810 device_initial_probe from bus_probe_device+0x98/0xa0 bus_probe_device from deferred_probe_work_func+0x8c/0xbc r7:00000000 r6:c1a309e8 r5:c1a3099c r4:c27c0810 deferred_probe_work_func from process_one_work+0x2b8/0x774 r7:c25c8000 r6:c2407000 r5:c2557480 r4:c1a309f8 process_one_work from worker_thread+0x17c/0x56c r10:00000088 r9:c25c8000 r8:c1905d40 r7:c240703c r6:c2557498 r5:c2407000 r4:c2557480 worker_thread from kthread+0x108/0x13c r10:00000000 r9:df815e2c r8:c2557500 r7:c2557480 r6:c0151924 r5:c25c8000 r4:c2554540 kthread from ret_from_fork+0x14/0x28 Exception stack(0xdf831fb0 to 0xdf831ff8) 1fa0: 00000000 00000000 00000000
00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c015ba68 r4:c2554540
Code: e24cc004 ea000001 e1530006 0a000007 (e5be5004) ---[ end trace 0000000000000000 ]---
Signature · 2 lines
Best regards, Dmitry
Quoted 1 line · Dmitry Osipenko
This patch breaks Tegra again, please take a look:
Damn, not again :(
Quoted 1 line
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
Why did you mention this patch ? This just removed an unused API, Tegra should have broke because of something else, isn't it ?
Quoted 11 lines
8<--- cut here ---
Unable to handle kernel paging request at virtual address ffffffff [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000 Internal error: Oops: 37 [#1] PREEMPT SMP ARM Modules linked in: CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.19.0-rc1-00040-g30b62d123f4f #82 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events_unbound deferred_probe_work_func PC is at _opp_compare_key+0x40/0xc4 LR is at 0xfffffffb
How is LR set to such an address ?
Quoted 10 lines
pc : [<c0b91b54>] lr : [<fffffffb>] psr: 20000113 sp : df831b08 ip : c33cd4d0 fp : df831b24 r10: c2586078 r9 : c258606c r8 : 00000000 r7 : 00000000 r6 : 00000001 r5 : c33cd480 r4 : c2586000 r3 : 00000000 r2 : c33cd480 r1 : c258606c r0 : c2586000 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 8000404a DAC: 00000051 ... Backtrace: _opp_compare_key from _set_opp+0x80/0x408
Whatever happened, happened from _opp_compare_key() and I tried to look at it many times, couldn't figure out what's wrong there.
For the device in question, pmc I think, we don't have any "opp-hz" property in the DT, but still the OPP core will fetch its clock and set clk_count to 1. But this was working earlier too, we were comparing the rate anyways. I think one of _opp_compare_rate() or _opp_compare_bw() is broken here, but I just couldn't figure out. The rate one should run one loop and bw one should just return. I don't see why a crash should come out eventually.
Can you help debug this a bit ? Also see what are the values of opp_table->path_count and opp_table->clk_count, should be 0 and 1 AFAICT.
Sorry about this Dmitry, I think we are all settled and again went into crap.
Signature · 1 line
viresh
Quoted 9 lines · Viresh Kumar
Quoted 1 line · Dmitry Osipenko
This patch breaks Tegra again, please take a look:
Damn, not again :(
Quoted 1 line
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
Why did you mention this patch ? This just removed an unused API, Tegra should have broke because of something else, isn't it ?
This patch is the cause.
Quoted 36 lines
Quoted 11 lines
8<--- cut here ---
Unable to handle kernel paging request at virtual address ffffffff [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000 Internal error: Oops: 37 [#1] PREEMPT SMP ARM Modules linked in: CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.19.0-rc1-00040-g30b62d123f4f #82 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events_unbound deferred_probe_work_func PC is at _opp_compare_key+0x40/0xc4 LR is at 0xfffffffbHow is LR set to such an address ?
Quoted 10 lines
pc : [<c0b91b54>] lr : [<fffffffb>] psr: 20000113 sp : df831b08 ip : c33cd4d0 fp : df831b24 r10: c2586078 r9 : c258606c r8 : 00000000 r7 : 00000000 r6 : 00000001 r5 : c33cd480 r4 : c2586000 r3 : 00000000 r2 : c33cd480 r1 : c258606c r0 : c2586000 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 8000404a DAC: 00000051 ... Backtrace: _opp_compare_key from _set_opp+0x80/0x408Whatever happened, happened from
_opp_compare_key()and I tried to look at it many times, couldn't figure out what's wrong there.For the device in question, pmc I think, we don't have any "opp-hz" property in the DT, but still the OPP core will fetch its clock and set
clk_countto 1. But this was working earlier too, we were comparing the rate anyways. I think one of_opp_compare_rate()or_opp_compare_bw()is broken here, but I just couldn't figure out. The rate one should run one loop and bw one should just return. I don't see why a crash should come out eventually.
Quoted 3 lines
Can you help debug this a bit ? Also see what are the values of
opp_table->path_countandopp_table->clk_count, should be 0 and 1 AFAICT.
I see that previously dev_pm_opp_set_config() had "_add_opp_table(dev, false)", now it's "_add_opp_table(dev, true)".
Will take a closer look later on.
Quoted 2 lines
Sorry about this Dmitry, I think we are all settled and again went into crap.
No problems :)
Signature · 2 lines
Best regards, Dmitry
Quoted 56 lines · Dmitry Osipenko
Quoted 9 lines · Viresh Kumar
Quoted 1 line · Dmitry Osipenko
This patch breaks Tegra again, please take a look:
Damn, not again :(
Quoted 1 line
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
Why did you mention this patch ? This just removed an unused API, Tegra should have broke because of something else, isn't it ?
This patch is the cause.
Quoted 36 lines
Quoted 11 lines
8<--- cut here ---
Unable to handle kernel paging request at virtual address ffffffff [ffffffff] *pgd=9effd861, *pte=00000000, *ppte=00000000 Internal error: Oops: 37 [#1] PREEMPT SMP ARM Modules linked in: CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 5.19.0-rc1-00040-g30b62d123f4f #82 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events_unbound deferred_probe_work_func PC is at _opp_compare_key+0x40/0xc4 LR is at 0xfffffffbHow is LR set to such an address ?
Quoted 10 lines
pc : [<c0b91b54>] lr : [<fffffffb>] psr: 20000113 sp : df831b08 ip : c33cd4d0 fp : df831b24 r10: c2586078 r9 : c258606c r8 : 00000000 r7 : 00000000 r6 : 00000001 r5 : c33cd480 r4 : c2586000 r3 : 00000000 r2 : c33cd480 r1 : c258606c r0 : c2586000 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 8000404a DAC: 00000051 ... Backtrace: _opp_compare_key from _set_opp+0x80/0x408Whatever happened, happened from
_opp_compare_key()and I tried to look at it many times, couldn't figure out what's wrong there.For the device in question, pmc I think, we don't have any "opp-hz" property in the DT, but still the OPP core will fetch its clock and set
clk_countto 1. But this was working earlier too, we were comparing the rate anyways. I think one of_opp_compare_rate()or_opp_compare_bw()is broken here, but I just couldn't figure out. The rate one should run one loop and bw one should just return. I don't see why a crash should come out eventually.Quoted 3 lines
Can you help debug this a bit ? Also see what are the values of
opp_table->path_countandopp_table->clk_count, should be 0 and 1 AFAICT.I see that previously
dev_pm_opp_set_config()had "_add_opp_table(dev, false)", now it's "_add_opp_table(dev, true)".
Quoted 6 lines
Will take a closer look later on.
Quoted 2 lines
Sorry about this Dmitry, I think we are all settled and again went into crap.
No problems :)
BTW, maybe we should consider to start adding kselftests for OPP, like clk framework did. That will be handy to have given that it's not easy to test the whole OPP core without having specific devices.
Signature · 2 lines
Best regards, Dmitry
Quoted 3 lines · Dmitry Osipenko
BTW, maybe we should consider to start adding kselftests for OPP, like clk framework did. That will be handy to have given that it's not easy to test the whole OPP core without having specific devices.
After being regularly bitten by such issues, I added some for cpufreq earlier. Its time that I invest some time for OPP core too I think :)
I don't know though when I will be able to find time for that :(
Signature · 1 line
viresh
Quoted 4 lines · Dmitry Osipenko
Quoted 3 lines · Viresh Kumar
Quoted 1 line · Dmitry Osipenko
This patch breaks Tegra again, please take a look:
Quoted 8 lines
Quoted 6 lines
Damn, not again :(
Quoted 1 line
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
Why did you mention this patch ? This just removed an unused API, Tegra should have broke because of something else, isn't it ?
This patch is the cause.
I was tracking the crash too closely it seems. :(
Quoted 2 lines
I see that previously
dev_pm_opp_set_config()had "_add_opp_table(dev, false)", now it's "_add_opp_table(dev, true)".
That's definitely a mistake, I still don't understand though how it can lead to the crash we got.
I have fixed this in my tree now, can you check again please.
Signature · 1 line
viresh
Quoted 14 lines · Viresh Kumar
Quoted 4 lines · Dmitry Osipenko
Quoted 3 lines · Viresh Kumar
Quoted 1 line · Dmitry Osipenko
This patch breaks Tegra again, please take a look:
Quoted 8 lines
Quoted 6 lines
Damn, not again :(
Quoted 1 line
OPP: Remove dev{m}_pm_opp_of_add_table_noclk()
Why did you mention this patch ? This just removed an unused API, Tegra should have broke because of something else, isn't it ?
This patch is the cause.
Quoted 7 lines
I was tracking the crash too closely it seems. :(
Quoted 2 lines
I see that previously
dev_pm_opp_set_config()had "_add_opp_table(dev, false)", now it's "_add_opp_table(dev, true)".That's definitely a mistake, I still don't understand though how it can lead to the crash we got.
I'll investigate it.
Quoted 2 lines
I have fixed this in my tree now, can you check again please.
Yours tree works, thank you.
Signature · 2 lines
Best regards, Dmitry
Quoted 11 lines · Viresh Kumar
Hello,
This patchset adds support for devices with multiple clocks. None of the clocks is considered primary in this case and all are handled equally.
The drivers, for multiple clock case, are expected to call
dev_pm_opp_set_opp()to set the specific OPP. Though how they find the target OPP is left for the users to handle. For some, we may have another unique OPP property, like level, which can be used to find the OPP. While in case of others, we may want to implement freq-based OPP finder APIs for multiple clock rates. I have decided not to implement them in advance, and add them only someone wants to use them.
This break OPP parsing on SC8280XP and hence cpufreq and other things:
[ +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found [ +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19 [ +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1 [ +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1
...
I just did a rebase on next-20220708 and hit this.
I've narrowed it down to _read_rate() now returning -ENODEV since opp_table->clk_count is zero.
Similar to what was reported for tegra for v1:
https://lore.kernel.org/all/[email protected]/
I don't have time to look at this any more today, but it would we nice if you could unbreak linux-next.
Perhaps Bjorn or Mani can help with further details, but this doesn't look like something that is specific to SC8280XP.
Johan
Quoted 15 lines · Johan Hovold
This break OPP parsing on SC8280XP and hence cpufreq and other things:
[ +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found [ +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19 [ +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1 [ +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1...
I just did a rebase on next-20220708 and hit this.
I've narrowed it down to
_read_rate()now returning-ENODEVsinceopp_table->clk_countis zero.Similar to what was reported for tegra for v1:
Quoted 7 lines
https://lore.kernel.org/all/[email protected]/I don't have time to look at this any more today, but it would we nice if you could unbreak linux-next.
Perhaps Bjorn or Mani can help with further details, but this doesn't look like something that is specific to SC8280XP.
It is actually. This is yet another corner case, Tegra had one as well.
I have tried to understand the Qcom code / setup to best of my abilities, and the problem as per me is that qcom-cpufreq-hw doesn't provide a clk to the OPP core, which breaks it after the new updates to the OPP core. I believe following will solve it. Can someone please try this ? I will then merge it with the right commit.
drivers/opp/core.c +14 -0
@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev, }if (ret == -ENOENT ) { +/* +* There are few platforms which don't want the OPP core to +* manage device's clock settings. In such cases neither the +* platform provides the clks explicitly to us, nor the DT +* contains a valid clk entry. The OPP nodes in DT may still +* contain "opp-hz" property though, which we need to parse and +* allow the platform to find an OPP based on freq later on. +* +* This is a simple solution to take care of such corner cases, +* i.e. make the clk_count 1, which lets us allocate space for +* frequency in opp->rates and also parse the entries in DT. +*/ + opp_table->clk_count =1 ; +dev_dbg (dev,"%s: Couldn't find clock: %d\n" , __func__, ret);return opp_table; }
Signature · 1 line
viresh
Manivannan Sadhasivam
Tested-byQuoted 24 lines · Viresh Kumar
Quoted 15 lines · Johan Hovold
This break OPP parsing on SC8280XP and hence cpufreq and other things:
[ +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found [ +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19 [ +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1 [ +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1...
I just did a rebase on next-20220708 and hit this.
I've narrowed it down to
_read_rate()now returning-ENODEVsinceopp_table->clk_countis zero.Similar to what was reported for tegra for v1:
Quoted 7 lines
https://lore.kernel.org/all/[email protected]/I don't have time to look at this any more today, but it would we nice if you could unbreak linux-next.
Perhaps Bjorn or Mani can help with further details, but this doesn't look like something that is specific to SC8280XP.
Quoted 9 lines
It is actually. This is yet another corner case, Tegra had one as well.
I have tried to understand the Qcom code / setup to best of my abilities, and the problem as per me is that qcom-cpufreq-hw doesn't provide a clk to the OPP core, which breaks it after the new updates to the OPP core. I believe following will solve it. Can someone please try this ? I will then merge it with the right commit.
I gave it a shot on Lenovo X13s based on SC8280 SoC and it fixes the OPP issue. So you can add,
Thanks,
Mani
Quoted 28 lines
drivers/opp/core.c +14
-0@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev, }if (ret == -ENOENT ) { +/* +* There are few platforms which don't want the OPP core to +* manage device's clock settings. In such cases neither the +* platform provides the clks explicitly to us, nor the DT +* contains a valid clk entry. The OPP nodes in DT may still +* contain "opp-hz" property though, which we need to parse and +* allow the platform to find an OPP based on freq later on. +* +* This is a simple solution to take care of such corner cases, +* i.e. make the clk_count 1, which lets us allocate space for +* frequency in opp->rates and also parse the entries in DT. +*/ + opp_table->clk_count =1 ; +dev_dbg (dev,"%s: Couldn't find clock: %d\n" , __func__, ret);return opp_table; }Signature · 2 lines
-- viresh
Quoted 18 lines · Viresh Kumar
Quoted 17 lines · Johan Hovold
This break OPP parsing on SC8280XP and hence cpufreq and other things:
[ +0.010890] cpu cpu0: _opp_add_static_v2: opp key field not found [ +0.000019] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -19 [ +0.000060] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 403200000, volt: 576000, enabled: 1 [ +0.000030] cpu cpu0: _opp_is_duplicate: duplicate OPPs detected. Existing: freq: 300000000, volt: 576000, enabled: 1. New: freq: 499200000, volt: 576000, enabled: 1...
I just did a rebase on next-20220708 and hit this.
I've narrowed it down to
_read_rate()now returning-ENODEVsinceopp_table->clk_countis zero.Similar to what was reported for tegra for v1:
https://lore.kernel.org/all/[email protected]/
Quoted 8 lines
Quoted 5 lines
I don't have time to look at this any more today, but it would we nice if you could unbreak linux-next.
Perhaps Bjorn or Mani can help with further details, but this doesn't look like something that is specific to SC8280XP.
It is actually. This is yet another corner case, Tegra had one as well.
I literally meant that it does not appear to be SC8280XP specific. Bjorn reported seeing similar problems on multiple Qualcomm SoCs.
Quoted 21 lines
I have tried to understand the Qcom code / setup to best of my abilities, and the problem as per me is that qcom-cpufreq-hw doesn't provide a clk to the OPP core, which breaks it after the new updates to the OPP core. I believe following will solve it. Can someone please try this ? I will then merge it with the right commit.
drivers/opp/core.c +7
-0@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev, }if (ret == -ENOENT ) { +/* +* There are few platforms which don't want the OPP core to +* manage device's clock settings. In such cases neither the +* platform provides the clks explicitly to us, nor the DT +* contains a valid clk entry. The OPP nodes in DT may still +* contain "opp-hz" property though, which we need to parse and +* allow the platform to find an OPP based on freq later on.
drivers/opp/core.c static struct opp_table *_update_opp_table_clk(struct device *dev, · line 1403
Quoted 10 lines
drivers/opp/core.c +7
-0+* +* This is a simple solution to take care of such corner cases, +* i.e. make the clk_count 1, which lets us allocate space for +* frequency in opp->rates and also parse the entries in DT. +*/ + opp_table->clk_count =1 ; +dev_dbg (dev,"%s: Couldn't find clock: %d\n" , __func__, ret);return opp_table; }
This looks like a hack. And it also triggers a bunch of new warning when opp is trying to create debugfs entries for an entirely different table which now gets clk_count set to 1:
[ +0.000979] cx: _update_opp_table_clk: Couldn't find clock: -2 [ +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
This is for the rpmhpd whose opp table does not have either opp-hz or clocks (just opp-level).
The above unbreaks cpufreq though.
Johan
Quoted 20 lines · Johan Hovold
Quoted 19 lines · Viresh Kumar
drivers/opp/core.c +11
-0@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev, }if (ret == -ENOENT ) { +/* +* There are few platforms which don't want the OPP core to +* manage device's clock settings. In such cases neither the +* platform provides the clks explicitly to us, nor the DT +* contains a valid clk entry. The OPP nodes in DT may still +* contain "opp-hz" property though, which we need to parse and +* allow the platform to find an OPP based on freq later on. +* +* This is a simple solution to take care of such corner cases, +* i.e. make the clk_count 1, which lets us allocate space for +* frequency in opp->rates and also parse the entries in DT.
Quoted 8 lines
drivers/opp/core.c static struct opp_table *_update_opp_table_clk(struct device *dev, · line 1403
Quoted 6 lines
drivers/opp/core.c +3
-0+*/ + opp_table->clk_count =1 ; +dev_dbg (dev,"%s: Couldn't find clock: %d\n" , __func__, ret);return opp_table; }This looks like a hack.
Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it is done for Tegra, where you will pass the right clock name to the OPP core, so it can verify that the clk is there and parse the table. And then tell the OPP core not to configure the clk from dev_pm_opp_set_opp(), which is possible now. This would have done the things in the right way.
The problem with Qcom's DT is that the CPU node have the OPP table but doesn't contain the clocks, which are available with the qcom,cpufreq-hw node instead. Because of this, I couldn't pass the real clocks name to the OPP core, "xo", for the CPU device.
I really tried to avoid adding the above code for Tegra and found a better and cleaner way out. But I couldn't do the same here and figured it may be more generic of a problem, which is fine as well.
The OPP core does two things currently:
1) Parse the DT and provide helpers to find the right OPP, etc.
2) Provide generic helper to configure all resources related to the OPP.
It is fine if some platforms only want to have the first and not the second. To have the second though, you need to have the first as well.
The clk is required only for the second case, and the OPP core should parse the DT anyways, irrespective of the availability of the clock. Because of this reason, making the above change looked reasonable (this is what was happening before my new patches came in anyway). The clock isn't there, but there is "opp-hz" present in the DT, which needs to be parsed.
Quoted 15 lines
And it also triggers a bunch of new warning when
opp is trying to create debugfs entries for an entirely different table which now getsclk_countset to 1:[ +0.000979] cx: _update_opp_table_clk: Couldn't find clock: -2 [ +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present! [ +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
Quoted 2 lines
This is for the rpmhpd whose opp table does not have either opp-hz or clocks (just opp-level).
Ahh, I missed switching back to the earlier code here. i.e. not use the frequency for OPP directory's name, when it is 0.
This will fix it.
drivers/opp/debugfs.c +1 -1
@@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)* - For some devices rate isn't available or there are multiple, use * index instead for them. */ -if (likely (opp_table->clk_count ==1 )) +if (likely (opp_table->clk_count ==1 && opp->rates[0 ])) id = opp->rates[0 ];else id =_get_opp_count (opp_table);
I have merged this into:
commit 341df9889277 ("OPP: Allow multiple clocks for a device")
and pushed out for linux-next.
Bjorn, Mani,
It would be really good if we can find a way to make following work on Qcom:
clk_get(cpu_dev, NULL or "xo")
If that happens, we can handle the special case just at the consumer driver (qcom-cpufreq-hw) and not in the core.
Signature · 1 line
viresh
Quoted 56 lines · Viresh Kumar
Quoted 20 lines · Johan Hovold
Quoted 19 lines · Viresh Kumar
drivers/opp/core.c +11
-0@@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev, }if (ret == -ENOENT ) { +/* +* There are few platforms which don't want the OPP core to +* manage device's clock settings. In such cases neither the +* platform provides the clks explicitly to us, nor the DT +* contains a valid clk entry. The OPP nodes in DT may still +* contain "opp-hz" property though, which we need to parse and +* allow the platform to find an OPP based on freq later on. +* +* This is a simple solution to take care of such corner cases, +* i.e. make the clk_count 1, which lets us allocate space for +* frequency in opp->rates and also parse the entries in DT. Quoted 8 lines
drivers/opp/core.c static struct opp_table *_update_opp_table_clk(struct device *dev, · line 1403
Quoted 6 lines
drivers/opp/core.c +3
-0+*/ + opp_table->clk_count =1 ; +dev_dbg (dev,"%s: Couldn't find clock: %d\n" , __func__, ret);return opp_table; }This looks like a hack.
Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it is done for Tegra, where you will pass the right clock name to the OPP core, so it can verify that the clk is there and parse the table. And then tell the OPP core not to configure the clk from
dev_pm_opp_set_opp(), which is possible now. This would have done the things in the right way.The problem with Qcom's DT is that the CPU node have the OPP table but doesn't contain the clocks, which are available with the qcom,cpufreq-hw node instead. Because of this, I couldn't pass the real clocks name to the OPP core, "xo", for the CPU device.
I really tried to avoid adding the above code for Tegra and found a better and cleaner way out. But I couldn't do the same here and figured it may be more generic of a problem, which is fine as well.
The OPP core does two things currently:
1) Parse the DT and provide helpers to find the right OPP, etc.
2) Provide generic helper to configure all resources related to the OPP.
It is fine if some platforms only want to have the first and not the second. To have the second though, you need to have the first as well.
Quoted 6 lines
The clk is required only for the second case, and the OPP core should parse the DT anyways, irrespective of the availability of the clock. Because of this reason, making the above change looked reasonable (this is what was happening before my new patches came in anyway). The clock isn't there, but there is "opp-hz" present in the DT, which needs to be parsed.
Ok, thanks for the details. I'd still look into if there's some way to avoid setting clk_count when there are no clocks as it sounds like an anti-pattern that will just make the code harder to understand and maintain.
Quoted 20 lines
Quoted 9 lines
And it also triggers a bunch of new warning when
opp is trying to create debugfs entries for an entirely different table which now getsclk_countset to 1:[ +0.000979] cx:
_update_opp_table_clk: Couldn't find clock: -2 [ +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!This is for the rpmhpd whose opp table does not have either opp-hz or clocks (just opp-level).
Ahh, I missed switching back to the earlier code here. i.e. not use the frequency for OPP directory's name, when it is 0.
This will fix it.
drivers/opp/debugfs.c +0
-0
drivers/opp/debugfs.c void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) · line 144
Quoted 8 lines
drivers/opp/debugfs.c +1
-1@@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)* - For some devices rate isn't available or there are multiple, use * index instead for them. */ -if (likely (opp_table->clk_count ==1 )) +if (likely (opp_table->clk_count ==1 && opp->rates[0 ])) id = opp->rates[0 ];else id =_get_opp_count (opp_table);
Indeed it does, thanks.
Quoted 5 lines
I have merged this into:
commit
341df9889277("OPP: Allow multiple clocks for a device")and pushed out for linux-next.
Thanks for addressing this quickly. With the two patches above applied, the issues I noticed are gone.
Johan
Quoted 4 lines · Johan Hovold
Ok, thanks for the details. I'd still look into if there's some way to avoid setting
clk_countwhen there are no clocks as it sounds like an anti-pattern that will just make the code harder to understand and maintain.
Here is an attempt from me :)
https://lore.kernel.org/lkml/[email protected]/
Signature · 1 line
viresh