by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 1/8] cpufreq: arm_big_little: add cluster regulator support

On 21 April 2015 at 18:47, Bartlomiej Zolnierkiewicz
<[email protected]> wrote:
> Add cluster regulator support as a preparation to adding
> generic arm_big_little_dt cpufreq_dt driver support for
> ODROID-XU3 board. This allows arm_big_little[_dt] driver

This is irrelevant here, its not about XU3 but any board that
wants to use it..

> to set not only the frequency but also the voltage (which
> is obtained from operating point's voltage value) for CPU
> clusters.
>
> Cc: Kukjin Kim <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Andreas Faerber <[email protected]>
> Cc: Sachin Kamat <[email protected]>
> Cc: Thomas Abraham <[email protected]>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> ---
> .../bindings/cpufreq/arm_big_little_dt.txt | 4 +
> drivers/cpufreq/arm_big_little.c | 153 +++++++++++++++++---
> 2 files changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/arm_big_little_dt.txt b/Documentation/devicetree/bindings/cpufreq/arm_big_little_dt.txt
> index 0715695..8ca4a12 100644
> --- a/Documentation/devicetree/bindings/cpufreq/arm_big_little_dt.txt
> +++ b/Documentation/devicetree/bindings/cpufreq/arm_big_little_dt.txt
> @@ -18,6 +18,10 @@ Required properties:
> Optional properties:
> - clock-latency: Specify the possible maximum transition latency for clock,
> in unit of nanoseconds.
> +- cpu-cluster.0-supply: Provides the regulator node supplying voltage to CPU
> + cluster 0.
> +- cpu-cluster.1-supply: Provides the regulator node supplying voltage to CPU
> + cluster 1.

I don't think you need these..

http://permalink.gmane.org/gmane.linux.power-management.general/58548

> Examples:
>
> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index e1a6ba6..edb461b 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -31,6 +31,7 @@
> #include <linux/slab.h>
> #include <linux/topology.h>
> #include <linux/types.h>
> +#include <linux/regulator/consumer.h>
> #include <asm/bL_switcher.h>
>
> #include "arm_big_little.h"
> @@ -54,6 +55,9 @@ static bool bL_switching_enabled;
>
> static struct cpufreq_arm_bL_ops *arm_bL_ops;
> static struct clk *clk[MAX_CLUSTERS];
> +static struct regulator *reg[MAX_CLUSTERS];
> +static struct device *cpu_devs[MAX_CLUSTERS];
> +static int transition_latencies[MAX_CLUSTERS];
> static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
> static atomic_t cluster_usage[MAX_CLUSTERS + 1];
>
> @@ -122,7 +126,76 @@ static unsigned int bL_cpufreq_get_rate(unsigned int cpu)
> }
> }
>
> -static unsigned int
> +static int
> +bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate)
> +{
> + unsigned long volt = 0, volt_old = 0;
> + long freq_Hz;
> + u32 old_rate;
> + int ret;
> +
> + freq_Hz = new_rate * 1000;
> + old_rate = clk_get_rate(clk[cluster]) / 1000;
> +
> + if (!IS_ERR(reg[cluster])) {
> + struct dev_pm_opp *opp;
> + unsigned long opp_freq;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(cpu_devs[cluster], &freq_Hz);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + pr_err("%s: cpu %d, cluster: %d, failed to find OPP for %ld\n",
> + __func__, cpu, cluster, freq_Hz);
> + return PTR_ERR(opp);
> + }
> + volt = dev_pm_opp_get_voltage(opp);
> + opp_freq = dev_pm_opp_get_freq(opp);
> + rcu_read_unlock();
> + volt_old = regulator_get_voltage(reg[cluster]);
> + pr_debug("%s: cpu %d, cluster: %d, Found OPP: %ld kHz, %ld uV\n",
> + __func__, cpu, cluster, opp_freq / 1000, volt);
> + }
> +
> + pr_debug("%s: cpu %d, cluster: %d, %u MHz, %ld mV --> %u MHz, %ld mV\n",
> + __func__, cpu, cluster,
> + old_rate / 1000, (volt_old > 0) ? volt_old / 1000 : -1,
> + new_rate / 1000, volt ? volt / 1000 : -1);
> +
> + /* scaling up? scale voltage before frequency */
> + if (!IS_ERR(reg[cluster]) && new_rate > old_rate) {
> + ret = regulator_set_voltage_tol(reg[cluster], volt, 0);
> + if (ret) {
> + pr_err("%s: cpu: %d, cluster: %d, failed to scale voltage up: %d\n",
> + __func__, cpu, cluster, ret);
> + return ret;
> + }
> + }
> +
> + ret = clk_set_rate(clk[cluster], new_rate * 1000);
> + if (WARN_ON(ret)) {
> + pr_err("%s: clk_set_rate failed: %d, cluster: %d\n",
> + __func__, cluster, ret);
> + if (!IS_ERR(reg[cluster]) && volt_old > 0)
> + regulator_set_voltage_tol(reg[cluster], volt_old, 0);
> + return ret;
> + }
> +
> + /* scaling down? scale voltage after frequency */
> + if (!IS_ERR(reg[cluster]) && new_rate < old_rate) {
> + ret = regulator_set_voltage_tol(reg[cluster], volt, 0);
> + if (ret) {
> + pr_err("%s: cpu: %d, cluster: %d, failed to scale voltage down: %d\n",
> + __func__, cpu, cluster, ret);
> + clk_set_rate(clk[cluster], old_rate * 1000);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int

If you want to make such fixes, please add them separately.

> bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> {
> u32 new_rate, prev_rate;
> @@ -145,22 +218,17 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
> __func__, cpu, old_cluster, new_cluster, new_rate);
>
> - ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
> - if (WARN_ON(ret)) {
> - pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
> - new_cluster);
> - if (bLs) {
> - per_cpu(cpu_last_req_freq, cpu) = prev_rate;
> - per_cpu(physical_cluster, cpu) = old_cluster;
> - }
> -
> - mutex_unlock(&cluster_lock[new_cluster]);
> -
> - return ret;
> + ret = bL_cpufreq_set_rate_cluster(cpu, new_cluster, new_rate);
> + if (ret && bLs) {
> + per_cpu(cpu_last_req_freq, cpu) = prev_rate;
> + per_cpu(physical_cluster, cpu) = old_cluster;
> }
>
> mutex_unlock(&cluster_lock[new_cluster]);
>
> + if (ret)
> + return ret;
> +
> /* Recalc freq for old cluster when switching clusters */
> if (old_cluster != new_cluster) {
> pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n",
> @@ -174,14 +242,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> /* Set freq of old cluster if there are cpus left on it */
> new_rate = find_cluster_maxfreq(old_cluster);
> new_rate = ACTUAL_FREQ(old_cluster, new_rate);
> -

??

> if (new_rate) {
> pr_debug("%s: Updating rate of old cluster: %d, to freq: %d\n",
> __func__, old_cluster, new_rate);
>
> - if (clk_set_rate(clk[old_cluster], new_rate * 1000))
> - pr_err("%s: clk_set_rate failed: %d, old cluster: %d\n",
> - __func__, ret, old_cluster);
> + bL_cpufreq_set_rate_cluster(cpu, old_cluster, new_rate);
> }
> mutex_unlock(&cluster_lock[old_cluster]);
> }
> @@ -288,6 +353,8 @@ static void _put_cluster_clk_and_freq_table(struct device *cpu_dev)
> return;
>
> clk_put(clk[cluster]);
> + if (!IS_ERR(reg[cluster]))
> + regulator_put(reg[cluster]);
> dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table[cluster]);
> if (arm_bL_ops->free_opp_table)
> arm_bL_ops->free_opp_table(cpu_dev);
> @@ -321,6 +388,7 @@ static void put_cluster_clk_and_freq_table(struct device *cpu_dev)
>
> static int _get_cluster_clk_and_freq_table(struct device *cpu_dev)
> {
> + unsigned long min_uV = ~0, max_uV = 0;
> u32 cluster = raw_cpu_to_cluster(cpu_dev->id);
> char name[14] = "cpu-cluster.";
> int ret;
> @@ -335,6 +403,51 @@ static int _get_cluster_clk_and_freq_table(struct device *cpu_dev)
> goto out;
> }
>
> + name[12] = cluster + '0';
> + reg[cluster] = regulator_get_optional(cpu_dev, name);

Just pass NULL instead of name.

> + if (!IS_ERR(reg[cluster])) {
> + unsigned long opp_freq = 0;
> +
> + dev_dbg(cpu_dev, "%s: reg: %p, cluster: %d\n",
> + __func__, reg[cluster], cluster);
> + cpu_devs[cluster] = cpu_dev;

If you want to save cpu_dev for further use, save it in bL_cpufreq_init()
not here.

> + /*
> + * Disable any OPPs where the connected regulator isn't able to
> + * provide the specified voltage and record minimum and maximum
> + * voltage levels.
> + */
> + while (1) {
> + struct dev_pm_opp *opp;
> + unsigned long opp_uV;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &opp_freq);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + break;
> + }
> + opp_uV = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + if (regulator_is_supported_voltage(reg[cluster], opp_uV,
> + opp_uV)) {
> + if (opp_uV < min_uV)
> + min_uV = opp_uV;
> + if (opp_uV > max_uV)
> + max_uV = opp_uV;
> + } else {
> + dev_pm_opp_disable(cpu_dev, opp_freq);
> + }
> +
> + opp_freq++;
> + }
> +
> + ret = regulator_set_voltage_time(reg[cluster], min_uV, max_uV);
> + if (ret > 0)
> + transition_latencies[cluster] = ret * 1000;
> + }
> +
> ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table[cluster]);
> if (ret) {
> dev_err(cpu_dev, "%s: failed to init cpufreq table, cpu: %d, err: %d\n",
> @@ -342,7 +455,6 @@ static int _get_cluster_clk_and_freq_table(struct device *cpu_dev)
> goto free_opp_table;
> }
>
> - name[12] = cluster + '0';
> clk[cluster] = clk_get(cpu_dev, name);
> if (!IS_ERR(clk[cluster])) {
> dev_dbg(cpu_dev, "%s: clk: %p & freq table: %p, cluster: %d\n",
> @@ -469,6 +581,11 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
> else
> policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>
> + if (cur_cluster < MAX_CLUSTERS &&
> + policy->cpuinfo.transition_latency != CPUFREQ_ETERNAL)
> + policy->cpuinfo.transition_latency
> + += transition_latencies[cur_cluster];

Instead of the crap here + a global array for transition latencies,
pass 'policy' to _get_cluster_clk_and_freq_table() and add it
directly to policy.

> +
> if (is_bL_switching_enabled())
> per_cpu(cpu_last_req_freq, policy->cpu) = clk_get_cpu_rate(policy->cpu);
>
> --
> 1.7.9.5
>

2015-06-11 22:17:35

by Heiko Stübner

[permalink] [raw]

Subject: Re: [PATCH 1/8] cpufreq: arm_big_little: add cluster regulator support

Hi,

Am Dienstag, 21. April 2015, 15:17:51 schrieb Bartlomiej Zolnierkiewicz:
> Add cluster regulator support as a preparation to adding
> generic arm_big_little_dt cpufreq_dt driver support for
> ODROID-XU3 board. This allows arm_big_little[_dt] driver
> to set not only the frequency but also the voltage (which
> is obtained from operating point's voltage value) for CPU
> clusters.
>
> Cc: Kukjin Kim <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Andreas Faerber <[email protected]>
> Cc: Sachin Kamat <[email protected]>
> Cc: Thomas Abraham <[email protected]>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>

I gave this a spin on the rk3368 arm64 soc from Rockchip, mainly to check if
my armclk handling was correct.

Your patch here only supports individual supplies per cluster but my
current board shares the supplies over both cpu clusters, so I've cooked
up a patch to also try to support shared supplies
[0].

Nevertheless,
Tested-by: Heiko Stuebner <[email protected]>

Do you plan to continue working on this?

Thanks
Heiko

[0] ---------------- 8< -----------------------------
From: Heiko Stuebner <[email protected]>
Subject: [PATCH] cpufreq: arm_big_little: add support for shared cluster regulators

In some socs or board designs the supplying regulator is shared between
more than one cluster but the current regulator support for big_little
sets the target voltage without any tolerance.

So when cluster0 requests 0.9V and cluster1 1.3V no suitable frequency
span is available that fits both. To accomodate this, look for shared
regulators and calculate the maximum voltage necessary. If the regulator
of the remote cluster has a lower voltage, its maximum also gets increased.

If cluster supplies are not shared, the behaviour is the same as before
with one specific voltage being set instead of a voltage-range.

When adapting shared voltages the remote clusters need to be locked too,
because cpufreq can very well try to change more than one cluster at the
same time. While the used mutex_trylock prevents deadlocks reliably,
it might also prevent some (or a lot) frequency changes from succeeding:

lock cluster0
lock cluster1
trylock cluster1
trylock cluster0
both fail

I'm probably simply overlooking some better way currently.

Signed-off-by: Heiko Stuebner <[email protected]>
---
drivers/cpufreq/arm_big_little.c | 102 ++++++++++++++++++++++++++++++++++-----
1 file changed, 91 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index e04ca0c..c65b111 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -130,12 +130,78 @@ static unsigned int bL_cpufreq_get_rate(unsigned int cpu)
}

static int
+bL_adapt_shared_regulators(u32 cluster, unsigned long *volt_max)
+{
+ unsigned long other_volt;
+ int ret, i;
+
+ for (i = 0; i < MAX_CLUSTERS; i++) {
+ if (i == cluster || IS_ERR_OR_NULL(reg[i]))
+ continue;
+
+ if (regulator_is_match(reg[cluster], reg[i])) {
+ other_volt = regulator_get_voltage(reg[i]);
+ if (other_volt > *volt_max) {
+ *volt_max = other_volt;
+ } else {
+ pr_debug("%s: adapting shared regulator in cluster %d to %lu-%lu mV\n",
+ __func__, i, other_volt / 1000, *volt_max / 1000);
+ ret = regulator_set_voltage(reg[i], other_volt, *volt_max);
+ if (ret) {
+ pr_err("%s: shared-supply for cluster: %d, failed to scale voltage up: %d\n",
+ __func__, cluster, ret);
+ return ret;
+ }
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int
+bL_lock_shared_regulators(u32 cluster)
+{
+ int ret, i;
+
+ for (i = 0; i < MAX_CLUSTERS; i++) {
+ if (i == cluster || IS_ERR_OR_NULL(reg[i]))
+ continue;
+
+ if (regulator_is_match(reg[cluster], reg[i])) {
+ ret = mutex_trylock(&cluster_lock[i]);
+ if (!ret) {
+ for (i--; i >= 0; i--)
+ mutex_unlock(&cluster_lock[i]);
+ return -EBUSY;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static void
+bL_unlock_shared_regulators(u32 cluster)
+{
+ int i;
+
+ for (i = 0; i < MAX_CLUSTERS; i++) {
+ if (i == cluster || IS_ERR_OR_NULL(reg[i]))
+ continue;
+
+ if (regulator_is_match(reg[cluster], reg[i]))
+ mutex_unlock(&cluster_lock[i]);
+ }
+}
+
+static int
bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate)
{
- unsigned long volt = 0, volt_old = 0;
+ unsigned long volt = 0, volt_max = 0, volt_old = 0;
long freq_Hz;
u32 old_rate;
- int ret;
+ int ret = 0;

freq_Hz = new_rate * 1000;
old_rate = clk_get_rate(clk[cluster]) / 1000;
@@ -144,13 +210,18 @@ bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate)
struct dev_pm_opp *opp;
unsigned long opp_freq;

+ ret = bL_lock_shared_regulators(cluster);
+ if(ret)
+ return 0;
+
rcu_read_lock();
opp = dev_pm_opp_find_freq_ceil(cpu_devs[cluster], &freq_Hz);
if (IS_ERR(opp)) {
rcu_read_unlock();
pr_err("%s: cpu %d, cluster: %d, failed to find OPP for %ld\n",
__func__, cpu, cluster, freq_Hz);
- return PTR_ERR(opp);
+ ret = PTR_ERR(opp);
+ goto unlock;
}
volt = dev_pm_opp_get_voltage(opp);
opp_freq = dev_pm_opp_get_freq(opp);
@@ -158,20 +229,25 @@ bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate)
volt_old = regulator_get_voltage(reg[cluster]);
pr_debug("%s: cpu %d, cluster: %d, Found OPP: %ld kHz, %ld uV\n",
__func__, cpu, cluster, opp_freq / 1000, volt);
+
+ volt_max = volt;
+ ret = bL_adapt_shared_regulators(cluster, &volt_max);
+ if (ret)
+ goto unlock;
}

- pr_debug("%s: cpu %d, cluster: %d, %u MHz, %ld mV --> %u MHz, %ld mV\n",
+ pr_debug("%s: cpu %d, cluster: %d, %u MHz, %ld mV --> %u MHz, %ld-%ld mV\n",
__func__, cpu, cluster,
old_rate / 1000, (volt_old > 0) ? volt_old / 1000 : -1,
- new_rate / 1000, volt ? volt / 1000 : -1);
+ new_rate / 1000, volt ? volt / 1000 : -1, volt_max ? volt_max / 1000 : -1);

/* scaling up? scale voltage before frequency */
if (!IS_ERR(reg[cluster]) && new_rate > old_rate) {
- ret = regulator_set_voltage_tol(reg[cluster], volt, 0);
+ ret = regulator_set_voltage(reg[cluster], volt, volt_max);
if (ret) {
pr_err("%s: cpu: %d, cluster: %d, failed to scale voltage up: %d\n",
__func__, cpu, cluster, ret);
- return ret;
+ goto unlock;
}
}

@@ -181,21 +257,25 @@ bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate)
__func__, cluster, ret);
if (!IS_ERR(reg[cluster]) && volt_old > 0)
regulator_set_voltage_tol(reg[cluster], volt_old, 0);
- return ret;
+ goto unlock;
}

/* scaling down? scale voltage after frequency */
if (!IS_ERR(reg[cluster]) && new_rate < old_rate) {
- ret = regulator_set_voltage_tol(reg[cluster], volt, 0);
+ ret = regulator_set_voltage(reg[cluster], volt, volt_max);
if (ret) {
pr_err("%s: cpu: %d, cluster: %d, failed to scale voltage down: %d\n",
__func__, cpu, cluster, ret);
clk_set_rate(clk[cluster], old_rate * 1000);
- return ret;
+ goto unlock;
}
}

- return 0;
+unlock:
+ if (!IS_ERR(reg[cluster]))
+ bL_unlock_shared_regulators(cluster);
+
+ return ret;
}

static int
--
2.1.4

---------------- 8< ----------------
From: Heiko Stuebner <[email protected]>
Subject: [PATCH] regulator: add a regulator_is_match function

Another stolen concept from the common clock framework. At some points it
can be useful to check if two regulator structs are actually pointing to
the same regulator_dev.

The usecase in question was to check if the supplying regulators of two
cpu clusters are actually the same and the regulator is thus shared
between these cpu clusters.

Therefore add regulator_is_match() that compares the rdev pointers
of two regulators and emits a bool stating if they match.

Signed-off-by: Heiko Stuebner <[email protected]>
---
drivers/regulator/core.c | 23 +++++++++++++++++++++++
include/linux/regulator/consumer.h | 7 +++++++
2 files changed, 30 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 443eaab..6bb7e70 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1591,6 +1591,29 @@ void regulator_put(struct regulator *regulator)
EXPORT_SYMBOL_GPL(regulator_put);

/**
+ * regulator_is_match - check if two regulator's point to the same rdev
+ * @p: regulator compared against q
+ * @q: regulator compared against p
+ *
+ * Returns true if the two struct regulator pointers both point to the same rdev
+ * Returns false otherwise. Note that two NULL clks are treated as matching.
+ */
+bool regulator_is_match(const struct regulator *p, const struct regulator *q)
+{
+ /* trivial case: identical struct regulator's or both NULL */
+ if (p == q)
+ return true;
+
+ /* true if clk->core pointers match. Avoid derefing garbage */
+ if (!IS_ERR_OR_NULL(p) && !IS_ERR_OR_NULL(q))
+ if (p->rdev == q->rdev)
+ return true;
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(regulator_is_match);
+
+/**
* regulator_register_supply_alias - Provide device alias for supply lookup
*
* @dev: device that will be given as the regulator "consumer"
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index f8a689e..38ffaa0 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -172,6 +172,7 @@ struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
const char *id);
void regulator_put(struct regulator *regulator);
void devm_regulator_put(struct regulator *regulator);
+bool regulator_is_match(const struct regulator *p, const struct regulator *q);

int regulator_register_supply_alias(struct device *dev, const char *id,
struct device *alias_dev,
@@ -316,6 +317,12 @@ static inline void devm_regulator_put(struct regulator *regulator)
{
}

+static inline bool regulator_is_match(const struct regulator *p,
+ const struct regulator *q)
+{
+ return true;
+}
+
static inline int regulator_register_supply_alias(struct device *dev,
const char *id,
struct device *alias_dev,
--
2.1.4