* Regression in Linux next again
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
this patchset contains previous two reverted patches. They were split
in order to make it easier to bisect, where the problem lies. It adds
also some simple debugs, which should help us track down the problem.
Tony, please apply this patchset and test it on your Beaglebone. It'd be
great if you could try to find out, which patch causes failure. They should
be appliable on the current next.
Thanks,
Maciej Purski
Maciej Purski (7):
regulator: core: Add debug messages
regulator: core: Add regulator_set_voltage_rdev()
regulator: core: Use re-entrant locks
regulator: core: Implement voltage balancing algorithm
regulator: core: Lock dependent regulators
regulator: core: Lock dependent regulators on regulator_enable()
regulator: core: Enable voltage balancing
drivers/regulator/core.c | 341 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 300 insertions(+), 41 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH 1/7] regulator: core: Add debug messages
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
Add debug messages on voltage setting and enabling path in order
to debug the coupled regulators problem.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ed568b..b740426 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1571,6 +1571,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
struct device *dev = rdev->dev.parent;
int ret;
+ pr_err("%s: %d\n", __func__, __LINE__);
/* No supply to resovle? */
if (!rdev->supply_name)
return 0;
@@ -2211,6 +2212,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
{
int ret;
+ pr_err("%s: %d\n", __func__, __LINE__);
lockdep_assert_held_once(&rdev->mutex);
/* check voltage and requested load before enabling */
@@ -2259,6 +2261,7 @@ int regulator_enable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
@@ -2275,6 +2278,7 @@ int regulator_enable(struct regulator *regulator)
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
+ pr_err("%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2373,6 +2377,7 @@ int regulator_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
@@ -2383,6 +2388,7 @@ int regulator_disable(struct regulator *regulator)
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
+ pr_err("%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_disable);
@@ -2858,6 +2864,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
const struct regulator_ops *ops = rdev->desc->ops;
int old_uV = _regulator_get_voltage(rdev);
+ pr_err("%s: %d\n", __func__, __LINE__);
trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
min_uV += rdev->constraints->uV_offset;
@@ -2992,6 +2999,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int best_supply_uV = 0;
int supply_change_uV = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
/* If we're setting the same range as last time the change
* should be a noop (some cpufreq implementations use the same
* voltage for multiple frequencies, for example).
@@ -3124,6 +3132,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
{
int ret = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
regulator_lock_supply(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 1/7] regulator: core: Add debug messages
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Add debug messages on voltage setting and enabling path in order
to debug the coupled regulators problem.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ed568b..b740426 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1571,6 +1571,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
struct device *dev = rdev->dev.parent;
int ret;
+ pr_err("%s: %d\n", __func__, __LINE__);
/* No supply to resovle? */
if (!rdev->supply_name)
return 0;
@@ -2211,6 +2212,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
{
int ret;
+ pr_err("%s: %d\n", __func__, __LINE__);
lockdep_assert_held_once(&rdev->mutex);
/* check voltage and requested load before enabling */
@@ -2259,6 +2261,7 @@ int regulator_enable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
@@ -2275,6 +2278,7 @@ int regulator_enable(struct regulator *regulator)
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
+ pr_err("%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2373,6 +2377,7 @@ int regulator_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
@@ -2383,6 +2388,7 @@ int regulator_disable(struct regulator *regulator)
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
+ pr_err("%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_disable);
@@ -2858,6 +2864,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
const struct regulator_ops *ops = rdev->desc->ops;
int old_uV = _regulator_get_voltage(rdev);
+ pr_err("%s: %d\n", __func__, __LINE__);
trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
min_uV += rdev->constraints->uV_offset;
@@ -2992,6 +2999,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int best_supply_uV = 0;
int supply_change_uV = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
/* If we're setting the same range as last time the change
* should be a noop (some cpufreq implementations use the same
* voltage for multiple frequencies, for example).
@@ -3124,6 +3132,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
{
int ret = 0;
+ pr_err("%s: %d\n", __func__, __LINE__);
regulator_lock_supply(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 2/7] regulator: core: Add regulator_set_voltage_rdev()
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
Refactor regulator_set_voltage_unlocked() by taking code related to
regulator_dev and creating a new function regulator_set_voltage_rdev(),
which operates only on struct regulator_dev.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b740426..413a824 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,9 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
+ int min_uV, int max_uV,
+ suspend_state_t state);
static struct regulator *create_regulator(struct regulator_dev *rdev,
struct device *dev,
const char *supply_name);
@@ -2996,8 +2999,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int ret = 0;
int old_min_uV, old_max_uV;
int current_uV;
- int best_supply_uV = 0;
- int supply_change_uV = 0;
pr_err("%s: %d\n", __func__, __LINE__);
/* If we're setting the same range as last time the change
@@ -3042,6 +3043,26 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret < 0)
goto out2;
+ ret = regulator_set_voltage_rdev(rdev, min_uV, max_uV, state);
+ if (ret < 0)
+ goto out2;
+
+out:
+ return 0;
+out2:
+ voltage->min_uV = old_min_uV;
+ voltage->max_uV = old_max_uV;
+
+ return ret;
+}
+
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
+ int max_uV, suspend_state_t state)
+{
+ int best_supply_uV = 0;
+ int supply_change_uV = 0;
+ int ret;
+
if (rdev->supply &&
regulator_ops_is_valid(rdev->supply->rdev,
REGULATOR_CHANGE_VOLTAGE) &&
@@ -3053,13 +3074,13 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
selector = regulator_map_voltage(rdev, min_uV, max_uV);
if (selector < 0) {
ret = selector;
- goto out2;
+ goto out;
}
best_supply_uV = _regulator_list_voltage(rdev, selector, 0);
if (best_supply_uV < 0) {
ret = best_supply_uV;
- goto out2;
+ goto out;
}
best_supply_uV += rdev->desc->min_dropout_uV;
@@ -3067,7 +3088,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
current_supply_uV = _regulator_get_voltage(rdev->supply->rdev);
if (current_supply_uV < 0) {
ret = current_supply_uV;
- goto out2;
+ goto out;
}
supply_change_uV = best_supply_uV - current_supply_uV;
@@ -3079,7 +3100,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret) {
dev_err(&rdev->dev, "Failed to increase supply voltage: %d\n",
ret);
- goto out2;
+ goto out;
}
}
@@ -3089,7 +3110,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
ret = _regulator_do_set_suspend_voltage(rdev, min_uV,
max_uV, state);
if (ret < 0)
- goto out2;
+ goto out;
if (supply_change_uV < 0) {
ret = regulator_set_voltage_unlocked(rdev->supply,
@@ -3103,11 +3124,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
out:
return ret;
-out2:
- voltage->min_uV = old_min_uV;
- voltage->max_uV = old_max_uV;
-
- return ret;
}
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 2/7] regulator: core: Add regulator_set_voltage_rdev()
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Refactor regulator_set_voltage_unlocked() by taking code related to
regulator_dev and creating a new function regulator_set_voltage_rdev(),
which operates only on struct regulator_dev.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b740426..413a824 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,9 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
+ int min_uV, int max_uV,
+ suspend_state_t state);
static struct regulator *create_regulator(struct regulator_dev *rdev,
struct device *dev,
const char *supply_name);
@@ -2996,8 +2999,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int ret = 0;
int old_min_uV, old_max_uV;
int current_uV;
- int best_supply_uV = 0;
- int supply_change_uV = 0;
pr_err("%s: %d\n", __func__, __LINE__);
/* If we're setting the same range as last time the change
@@ -3042,6 +3043,26 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret < 0)
goto out2;
+ ret = regulator_set_voltage_rdev(rdev, min_uV, max_uV, state);
+ if (ret < 0)
+ goto out2;
+
+out:
+ return 0;
+out2:
+ voltage->min_uV = old_min_uV;
+ voltage->max_uV = old_max_uV;
+
+ return ret;
+}
+
+static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
+ int max_uV, suspend_state_t state)
+{
+ int best_supply_uV = 0;
+ int supply_change_uV = 0;
+ int ret;
+
if (rdev->supply &&
regulator_ops_is_valid(rdev->supply->rdev,
REGULATOR_CHANGE_VOLTAGE) &&
@@ -3053,13 +3074,13 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
selector = regulator_map_voltage(rdev, min_uV, max_uV);
if (selector < 0) {
ret = selector;
- goto out2;
+ goto out;
}
best_supply_uV = _regulator_list_voltage(rdev, selector, 0);
if (best_supply_uV < 0) {
ret = best_supply_uV;
- goto out2;
+ goto out;
}
best_supply_uV += rdev->desc->min_dropout_uV;
@@ -3067,7 +3088,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
current_supply_uV = _regulator_get_voltage(rdev->supply->rdev);
if (current_supply_uV < 0) {
ret = current_supply_uV;
- goto out2;
+ goto out;
}
supply_change_uV = best_supply_uV - current_supply_uV;
@@ -3079,7 +3100,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret) {
dev_err(&rdev->dev, "Failed to increase supply voltage: %d\n",
ret);
- goto out2;
+ goto out;
}
}
@@ -3089,7 +3110,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
ret = _regulator_do_set_suspend_voltage(rdev, min_uV,
max_uV, state);
if (ret < 0)
- goto out2;
+ goto out;
if (supply_change_uV < 0) {
ret = regulator_set_voltage_unlocked(rdev->supply,
@@ -3103,11 +3124,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
out:
return ret;
-out2:
- voltage->min_uV = old_min_uV;
- voltage->max_uV = old_max_uV;
-
- return ret;
}
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 3/7] regulator: core: Use re-entrant locks
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
Re-entrant locks were implemented in previous patches. They should
substitute all mutex_lock() and mutex_unlock() calls on regulators'
mutexes.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 413a824..c5478d2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2274,9 +2274,9 @@ int regulator_enable(struct regulator *regulator)
return ret;
}
- mutex_lock(&rdev->mutex);
+ regulator_lock(rdev);
ret = _regulator_enable(rdev);
- mutex_unlock(&rdev->mutex);
+ regulator_unlock(rdev);
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2384,9 +2384,9 @@ int regulator_disable(struct regulator *regulator)
if (regulator->always_on)
return 0;
- mutex_lock(&rdev->mutex);
+ regulator_lock(rdev);
ret = _regulator_disable(rdev);
- mutex_unlock(&rdev->mutex);
+ regulator_unlock(rdev);
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2436,10 +2436,10 @@ int regulator_force_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret;
- mutex_lock(&rdev->mutex);
+ regulator_lock(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
- mutex_unlock(&rdev->mutex);
+ regulator_unlock(rdev);
if (rdev->supply)
while (rdev->open_count--)
@@ -2587,9 +2587,9 @@ int regulator_is_enabled(struct regulator *regulator)
if (regulator->always_on)
return 1;
- mutex_lock(®ulator->rdev->mutex);
+ regulator_lock(regulator->rdev);
ret = _regulator_is_enabled(regulator->rdev);
- mutex_unlock(®ulator->rdev->mutex);
+ regulator_unlock(regulator->rdev);
return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 3/7] regulator: core: Use re-entrant locks
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Re-entrant locks were implemented in previous patches. They should
substitute all mutex_lock() and mutex_unlock() calls on regulators'
mutexes.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 413a824..c5478d2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2274,9 +2274,9 @@ int regulator_enable(struct regulator *regulator)
return ret;
}
- mutex_lock(&rdev->mutex);
+ regulator_lock(rdev);
ret = _regulator_enable(rdev);
- mutex_unlock(&rdev->mutex);
+ regulator_unlock(rdev);
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2384,9 +2384,9 @@ int regulator_disable(struct regulator *regulator)
if (regulator->always_on)
return 0;
- mutex_lock(&rdev->mutex);
+ regulator_lock(rdev);
ret = _regulator_disable(rdev);
- mutex_unlock(&rdev->mutex);
+ regulator_unlock(rdev);
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2436,10 +2436,10 @@ int regulator_force_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret;
- mutex_lock(&rdev->mutex);
+ regulator_lock(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
- mutex_unlock(&rdev->mutex);
+ regulator_unlock(rdev);
if (rdev->supply)
while (rdev->open_count--)
@@ -2587,9 +2587,9 @@ int regulator_is_enabled(struct regulator *regulator)
if (regulator->always_on)
return 1;
- mutex_lock(®ulator->rdev->mutex);
+ regulator_lock(regulator->rdev);
ret = _regulator_is_enabled(regulator->rdev);
- mutex_unlock(®ulator->rdev->mutex);
+ regulator_unlock(regulator->rdev);
return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 4/7] regulator: core: Implement voltage balancing algorithm
2018-06-04 13:59 ` Maciej Purski
(?)
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, Carlos Hernandez, Maciej Purski, linux-omap,
linux-arm-kernel, Marek Szyprowski
On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
two devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.
Introduce new function regulator_balance_voltage(), which
keeps max_spread constraint fulfilled between a group of coupled
regulators. It should be called if a regulator changes its
voltage or after disabling or enabling. Disabled regulators should
follow changes of the enabled ones, but their consumers' demands
shouldn't be taken into account while calculating voltage of other
coupled regulators.
Find voltages, which are closest to suiting all the consumers' demands,
while fulfilling max_spread constraint, keeping the following rules:
- if one regulator is about to rise its voltage, rise others
voltages in order to keep the max_spread
- if a regulator, which has caused rising other regulators, is
lowered, lower other regulators if possible
- if one regulator is about to lower its voltage, but it hasn't caused
rising other regulators, don't change its voltage if it breaks the
max_spread
Change regulators' voltages step by step, keeping max_spread constraint
fulfilled all the time. Function regulator_get_optimal_voltage()
should find the best possible change for the regulator, which doesn't
break max_spread constraint. In function regulator_balance_voltage()
optimize number of steps by finding highest voltage difference on
each iteration.
If a regulator, which is about to change its voltage, is not coupled,
method regulator_get_optimal_voltage() should simply return the lowest
voltage fulfilling consumers' demands.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 183 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c5478d2..0b366c5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,8 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+ suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
suspend_state_t state);
@@ -3126,6 +3128,187 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+{
+ struct coupling_desc *c_desc = &rdev->coupling_desc;
+ struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
+ int max_spread = rdev->constraints->max_spread;
+ int n_coupled = c_desc->n_coupled;
+ int desired_min_uV, desired_max_uV, min_current_uV = INT_MAX;
+ int max_current_uV = 0, highest_min_uV = 0, target_uV, possible_uV;
+ int i, ret;
+
+ /* If consumers don't provide any demands, set voltage to min_uV */
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV, PM_SUSPEND_ON);
+ if (ret < 0)
+ goto out;
+
+ /*
+ * If there are no coupled regulators, simply set the voltage demanded
+ * by consumers.
+ */
+ if (n_coupled == 1) {
+ ret = desired_min_uV;
+ goto out;
+ }
+
+ /* Find highest min desired voltage */
+ for (i = 0; i < n_coupled; i++) {
+ int tmp_min = 0;
+ int tmp_max = INT_MAX;
+
+ if (!_regulator_is_enabled(c_rdevs[i]))
+ continue;
+
+ ret = regulator_check_consumers(c_rdevs[i],
+ &tmp_min,
+ &tmp_max, PM_SUSPEND_ON);
+ if (ret < 0)
+ goto out;
+
+ if (tmp_min > highest_min_uV)
+ highest_min_uV = tmp_min;
+ }
+
+ /*
+ * Let target_uV be equal to the desired one if possible.
+ * If not, set it to minimum voltage, allowed by other coupled
+ * regulators.
+ */
+ target_uV = max(desired_min_uV, highest_min_uV - max_spread);
+
+ /*
+ * Find min and max voltages, which currently aren't
+ * violating max_spread
+ */
+ for (i = 0; i < n_coupled; i++) {
+ int tmp_act;
+
+ /*
+ * Don't check the regulator, which is about
+ * to change voltage
+ */
+ if (c_rdevs[i] == rdev)
+ continue;
+ if (!_regulator_is_enabled(c_rdevs[i]))
+ continue;
+
+ tmp_act = _regulator_get_voltage(c_rdevs[i]);
+ if (tmp_act < 0) {
+ ret = tmp_act;
+ goto out;
+ }
+
+ if (tmp_act < min_current_uV)
+ min_current_uV = tmp_act;
+
+ if (tmp_act > max_current_uV)
+ max_current_uV = tmp_act;
+ }
+
+ /* There aren't any other regulators enabled */
+ if (max_current_uV == 0) {
+ possible_uV = target_uV;
+ } else {
+ /*
+ * Correct target voltage, so as it currently isn't
+ * violating max_spread
+ */
+ possible_uV = max(target_uV, max_current_uV - max_spread);
+ possible_uV = min(possible_uV, min_current_uV + max_spread);
+ }
+
+ if (possible_uV > desired_max_uV) {
+ ret = -EINVAL;
+ goto out;
+ }
+ ret = possible_uV;
+
+out:
+ return ret;
+}
+
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+ suspend_state_t state)
+{
+ struct regulator_dev **c_rdevs;
+ struct regulator_dev *best_rdev;
+ struct coupling_desc *c_desc = &rdev->coupling_desc;
+ int n_coupled;
+ int i, best_delta, best_uV, ret = 1;
+
+ c_rdevs = c_desc->coupled_rdevs;
+ n_coupled = c_desc->n_coupled;
+
+ /*
+ * if system is in a state other than PM_SUSPEND_ON, don't check
+ * other coupled regulators
+ */
+ if (state != PM_SUSPEND_ON)
+ n_coupled = 1;
+
+ /*
+ * Find the best possible voltage change on each loop. Leave the loop
+ * if there isn't any possible change.
+ */
+ while (1) {
+ best_delta = 0;
+ best_uV = 0;
+ best_rdev = NULL;
+
+ /*
+ * Find highest difference between optimal voltage
+ * and current voltage.
+ */
+ for (i = 0; i < n_coupled; i++) {
+ /*
+ * optimal_uV is the best voltage that can be set for
+ * i-th regulator at the moment without violating
+ * max_spread constraint in order to balance
+ * the coupled voltages.
+ */
+ int optimal_uV, current_uV;
+
+ optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
+ if (optimal_uV < 0) {
+ ret = optimal_uV;
+ goto out;
+ }
+
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = optimal_uV;
+ goto out;
+ }
+
+ if (abs(best_delta) < abs(optimal_uV - current_uV)) {
+ best_delta = optimal_uV - current_uV;
+ best_rdev = c_rdevs[i];
+ best_uV = optimal_uV;
+ }
+ }
+
+ /* Nothing to change, return successfully */
+ if (!best_rdev) {
+ ret = 0;
+ goto out;
+ }
+
+ ret = regulator_set_voltage_rdev(best_rdev, best_uV,
+ best_uV, state);
+
+ if (ret < 0)
+ goto out;
+ }
+
+out:
+ return ret;
+}
+
/**
* regulator_set_voltage - set regulator output voltage
* @regulator: regulator source
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 4/7] regulator: core: Implement voltage balancing algorithm
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
two devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.
Introduce new function regulator_balance_voltage(), which
keeps max_spread constraint fulfilled between a group of coupled
regulators. It should be called if a regulator changes its
voltage or after disabling or enabling. Disabled regulators should
follow changes of the enabled ones, but their consumers' demands
shouldn't be taken into account while calculating voltage of other
coupled regulators.
Find voltages, which are closest to suiting all the consumers' demands,
while fulfilling max_spread constraint, keeping the following rules:
- if one regulator is about to rise its voltage, rise others
voltages in order to keep the max_spread
- if a regulator, which has caused rising other regulators, is
lowered, lower other regulators if possible
- if one regulator is about to lower its voltage, but it hasn't caused
rising other regulators, don't change its voltage if it breaks the
max_spread
Change regulators' voltages step by step, keeping max_spread constraint
fulfilled all the time. Function regulator_get_optimal_voltage()
should find the best possible change for the regulator, which doesn't
break max_spread constraint. In function regulator_balance_voltage()
optimize number of steps by finding highest voltage difference on
each iteration.
If a regulator, which is about to change its voltage, is not coupled,
method regulator_get_optimal_voltage() should simply return the lowest
voltage fulfilling consumers' demands.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 183 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c5478d2..0b366c5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,8 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+ suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
suspend_state_t state);
@@ -3126,6 +3128,187 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+{
+ struct coupling_desc *c_desc = &rdev->coupling_desc;
+ struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
+ int max_spread = rdev->constraints->max_spread;
+ int n_coupled = c_desc->n_coupled;
+ int desired_min_uV, desired_max_uV, min_current_uV = INT_MAX;
+ int max_current_uV = 0, highest_min_uV = 0, target_uV, possible_uV;
+ int i, ret;
+
+ /* If consumers don't provide any demands, set voltage to min_uV */
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV, PM_SUSPEND_ON);
+ if (ret < 0)
+ goto out;
+
+ /*
+ * If there are no coupled regulators, simply set the voltage demanded
+ * by consumers.
+ */
+ if (n_coupled == 1) {
+ ret = desired_min_uV;
+ goto out;
+ }
+
+ /* Find highest min desired voltage */
+ for (i = 0; i < n_coupled; i++) {
+ int tmp_min = 0;
+ int tmp_max = INT_MAX;
+
+ if (!_regulator_is_enabled(c_rdevs[i]))
+ continue;
+
+ ret = regulator_check_consumers(c_rdevs[i],
+ &tmp_min,
+ &tmp_max, PM_SUSPEND_ON);
+ if (ret < 0)
+ goto out;
+
+ if (tmp_min > highest_min_uV)
+ highest_min_uV = tmp_min;
+ }
+
+ /*
+ * Let target_uV be equal to the desired one if possible.
+ * If not, set it to minimum voltage, allowed by other coupled
+ * regulators.
+ */
+ target_uV = max(desired_min_uV, highest_min_uV - max_spread);
+
+ /*
+ * Find min and max voltages, which currently aren't
+ * violating max_spread
+ */
+ for (i = 0; i < n_coupled; i++) {
+ int tmp_act;
+
+ /*
+ * Don't check the regulator, which is about
+ * to change voltage
+ */
+ if (c_rdevs[i] == rdev)
+ continue;
+ if (!_regulator_is_enabled(c_rdevs[i]))
+ continue;
+
+ tmp_act = _regulator_get_voltage(c_rdevs[i]);
+ if (tmp_act < 0) {
+ ret = tmp_act;
+ goto out;
+ }
+
+ if (tmp_act < min_current_uV)
+ min_current_uV = tmp_act;
+
+ if (tmp_act > max_current_uV)
+ max_current_uV = tmp_act;
+ }
+
+ /* There aren't any other regulators enabled */
+ if (max_current_uV == 0) {
+ possible_uV = target_uV;
+ } else {
+ /*
+ * Correct target voltage, so as it currently isn't
+ * violating max_spread
+ */
+ possible_uV = max(target_uV, max_current_uV - max_spread);
+ possible_uV = min(possible_uV, min_current_uV + max_spread);
+ }
+
+ if (possible_uV > desired_max_uV) {
+ ret = -EINVAL;
+ goto out;
+ }
+ ret = possible_uV;
+
+out:
+ return ret;
+}
+
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+ suspend_state_t state)
+{
+ struct regulator_dev **c_rdevs;
+ struct regulator_dev *best_rdev;
+ struct coupling_desc *c_desc = &rdev->coupling_desc;
+ int n_coupled;
+ int i, best_delta, best_uV, ret = 1;
+
+ c_rdevs = c_desc->coupled_rdevs;
+ n_coupled = c_desc->n_coupled;
+
+ /*
+ * if system is in a state other than PM_SUSPEND_ON, don't check
+ * other coupled regulators
+ */
+ if (state != PM_SUSPEND_ON)
+ n_coupled = 1;
+
+ /*
+ * Find the best possible voltage change on each loop. Leave the loop
+ * if there isn't any possible change.
+ */
+ while (1) {
+ best_delta = 0;
+ best_uV = 0;
+ best_rdev = NULL;
+
+ /*
+ * Find highest difference between optimal voltage
+ * and current voltage.
+ */
+ for (i = 0; i < n_coupled; i++) {
+ /*
+ * optimal_uV is the best voltage that can be set for
+ * i-th regulator at the moment without violating
+ * max_spread constraint in order to balance
+ * the coupled voltages.
+ */
+ int optimal_uV, current_uV;
+
+ optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
+ if (optimal_uV < 0) {
+ ret = optimal_uV;
+ goto out;
+ }
+
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = optimal_uV;
+ goto out;
+ }
+
+ if (abs(best_delta) < abs(optimal_uV - current_uV)) {
+ best_delta = optimal_uV - current_uV;
+ best_rdev = c_rdevs[i];
+ best_uV = optimal_uV;
+ }
+ }
+
+ /* Nothing to change, return successfully */
+ if (!best_rdev) {
+ ret = 0;
+ goto out;
+ }
+
+ ret = regulator_set_voltage_rdev(best_rdev, best_uV,
+ best_uV, state);
+
+ if (ret < 0)
+ goto out;
+ }
+
+out:
+ return ret;
+}
+
/**
* regulator_set_voltage - set regulator output voltage
* @regulator: regulator source
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 4/7] regulator: core: Implement voltage balancing algorithm
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires
higher voltage, there might occur a situation that the spread between
two devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators.
Introduce new function regulator_balance_voltage(), which
keeps max_spread constraint fulfilled between a group of coupled
regulators. It should be called if a regulator changes its
voltage or after disabling or enabling. Disabled regulators should
follow changes of the enabled ones, but their consumers' demands
shouldn't be taken into account while calculating voltage of other
coupled regulators.
Find voltages, which are closest to suiting all the consumers' demands,
while fulfilling max_spread constraint, keeping the following rules:
- if one regulator is about to rise its voltage, rise others
voltages in order to keep the max_spread
- if a regulator, which has caused rising other regulators, is
lowered, lower other regulators if possible
- if one regulator is about to lower its voltage, but it hasn't caused
rising other regulators, don't change its voltage if it breaks the
max_spread
Change regulators' voltages step by step, keeping max_spread constraint
fulfilled all the time. Function regulator_get_optimal_voltage()
should find the best possible change for the regulator, which doesn't
break max_spread constraint. In function regulator_balance_voltage()
optimize number of steps by finding highest voltage difference on
each iteration.
If a regulator, which is about to change its voltage, is not coupled,
method regulator_get_optimal_voltage() should simply return the lowest
voltage fulfilling consumers' demands.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 183 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c5478d2..0b366c5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,8 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+ suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
suspend_state_t state);
@@ -3126,6 +3128,187 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+{
+ struct coupling_desc *c_desc = &rdev->coupling_desc;
+ struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
+ int max_spread = rdev->constraints->max_spread;
+ int n_coupled = c_desc->n_coupled;
+ int desired_min_uV, desired_max_uV, min_current_uV = INT_MAX;
+ int max_current_uV = 0, highest_min_uV = 0, target_uV, possible_uV;
+ int i, ret;
+
+ /* If consumers don't provide any demands, set voltage to min_uV */
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV, PM_SUSPEND_ON);
+ if (ret < 0)
+ goto out;
+
+ /*
+ * If there are no coupled regulators, simply set the voltage demanded
+ * by consumers.
+ */
+ if (n_coupled == 1) {
+ ret = desired_min_uV;
+ goto out;
+ }
+
+ /* Find highest min desired voltage */
+ for (i = 0; i < n_coupled; i++) {
+ int tmp_min = 0;
+ int tmp_max = INT_MAX;
+
+ if (!_regulator_is_enabled(c_rdevs[i]))
+ continue;
+
+ ret = regulator_check_consumers(c_rdevs[i],
+ &tmp_min,
+ &tmp_max, PM_SUSPEND_ON);
+ if (ret < 0)
+ goto out;
+
+ if (tmp_min > highest_min_uV)
+ highest_min_uV = tmp_min;
+ }
+
+ /*
+ * Let target_uV be equal to the desired one if possible.
+ * If not, set it to minimum voltage, allowed by other coupled
+ * regulators.
+ */
+ target_uV = max(desired_min_uV, highest_min_uV - max_spread);
+
+ /*
+ * Find min and max voltages, which currently aren't
+ * violating max_spread
+ */
+ for (i = 0; i < n_coupled; i++) {
+ int tmp_act;
+
+ /*
+ * Don't check the regulator, which is about
+ * to change voltage
+ */
+ if (c_rdevs[i] == rdev)
+ continue;
+ if (!_regulator_is_enabled(c_rdevs[i]))
+ continue;
+
+ tmp_act = _regulator_get_voltage(c_rdevs[i]);
+ if (tmp_act < 0) {
+ ret = tmp_act;
+ goto out;
+ }
+
+ if (tmp_act < min_current_uV)
+ min_current_uV = tmp_act;
+
+ if (tmp_act > max_current_uV)
+ max_current_uV = tmp_act;
+ }
+
+ /* There aren't any other regulators enabled */
+ if (max_current_uV == 0) {
+ possible_uV = target_uV;
+ } else {
+ /*
+ * Correct target voltage, so as it currently isn't
+ * violating max_spread
+ */
+ possible_uV = max(target_uV, max_current_uV - max_spread);
+ possible_uV = min(possible_uV, min_current_uV + max_spread);
+ }
+
+ if (possible_uV > desired_max_uV) {
+ ret = -EINVAL;
+ goto out;
+ }
+ ret = possible_uV;
+
+out:
+ return ret;
+}
+
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+ suspend_state_t state)
+{
+ struct regulator_dev **c_rdevs;
+ struct regulator_dev *best_rdev;
+ struct coupling_desc *c_desc = &rdev->coupling_desc;
+ int n_coupled;
+ int i, best_delta, best_uV, ret = 1;
+
+ c_rdevs = c_desc->coupled_rdevs;
+ n_coupled = c_desc->n_coupled;
+
+ /*
+ * if system is in a state other than PM_SUSPEND_ON, don't check
+ * other coupled regulators
+ */
+ if (state != PM_SUSPEND_ON)
+ n_coupled = 1;
+
+ /*
+ * Find the best possible voltage change on each loop. Leave the loop
+ * if there isn't any possible change.
+ */
+ while (1) {
+ best_delta = 0;
+ best_uV = 0;
+ best_rdev = NULL;
+
+ /*
+ * Find highest difference between optimal voltage
+ * and current voltage.
+ */
+ for (i = 0; i < n_coupled; i++) {
+ /*
+ * optimal_uV is the best voltage that can be set for
+ * i-th regulator@the moment without violating
+ * max_spread constraint in order to balance
+ * the coupled voltages.
+ */
+ int optimal_uV, current_uV;
+
+ optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
+ if (optimal_uV < 0) {
+ ret = optimal_uV;
+ goto out;
+ }
+
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = optimal_uV;
+ goto out;
+ }
+
+ if (abs(best_delta) < abs(optimal_uV - current_uV)) {
+ best_delta = optimal_uV - current_uV;
+ best_rdev = c_rdevs[i];
+ best_uV = optimal_uV;
+ }
+ }
+
+ /* Nothing to change, return successfully */
+ if (!best_rdev) {
+ ret = 0;
+ goto out;
+ }
+
+ ret = regulator_set_voltage_rdev(best_rdev, best_uV,
+ best_uV, state);
+
+ if (ret < 0)
+ goto out;
+ }
+
+out:
+ return ret;
+}
+
/**
* regulator_set_voltage - set regulator output voltage
* @regulator: regulator source
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 5/7] regulator: core: Lock dependent regulators
2018-06-04 13:59 ` Maciej Purski
(?)
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, Carlos Hernandez, Maciej Purski, linux-omap,
linux-arm-kernel, Marek Szyprowski
Implementing coupled regulators adds a new dependency between
regulators. Therefore, the current locking model should be changed.
Coupled regulators should be locked with regulator's supplies at the
same time.
Add new function regulator_lock_dependent(), which locks all regulators
related with the one, that is being changed.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 23 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0b366c5..7c57268 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
}
}
-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+ unsigned int subclass)
{
+ struct regulator_dev *c_rdev;
int i;
- for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
- regulator_lock_nested(rdev, i);
+ for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+ c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+ if (!c_rdev)
+ continue;
+
+ regulator_lock_nested(c_rdev, subclass++);
+
+ if (c_rdev->supply)
+ subclass =
+ regulator_lock_recursive(c_rdev->supply->rdev,
+ subclass);
+ }
+
+ return subclass;
}
/**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev: regulator source
+ * regulator_unlock_dependent - unlock regulator's suppliers and coupled
+ * regulators
+ * @rdev: regulator source
+ *
+ * Unlock all regulators related with rdev by coupling or suppling.
*/
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
{
- struct regulator *supply;
+ struct regulator_dev *c_rdev;
+ int i;
- while (1) {
- regulator_unlock(rdev);
- supply = rdev->supply;
+ for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+ c_rdev = rdev->coupling_desc.coupled_rdevs[i];
- if (!rdev->supply)
- return;
+ if (!c_rdev)
+ continue;
+
+ regulator_unlock(c_rdev);
- rdev = supply->rdev;
+ if (c_rdev->supply)
+ regulator_unlock_dependent(c_rdev->supply->rdev);
}
}
/**
+ * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
+ * @rdev: regulator source
+ *
+ * This function as a wrapper on regulator_lock_recursive(), which locks
+ * all regulators related with rdev by coupling or suppling.
+ */
+static inline void regulator_lock_dependent(struct regulator_dev *rdev)
+{
+ regulator_lock_recursive(rdev, 0);
+}
+
+/**
* of_get_regulator - get a regulator device node based on supply name
* @dev: Device pointer for the consumer (of regulator) device
* @supply: regulator supply name
@@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
int ret = 0;
pr_err("%s: %d\n", __func__, __LINE__);
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
PM_SUSPEND_ON);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
@@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
if (regulator_check_states(state) || state == PM_SUSPEND_ON)
return -EINVAL;
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = _regulator_set_suspend_voltage(regulator, min_uV,
max_uV, state);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
@@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
{
int ret;
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = _regulator_get_voltage(regulator->rdev);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 5/7] regulator: core: Lock dependent regulators
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
Implementing coupled regulators adds a new dependency between
regulators. Therefore, the current locking model should be changed.
Coupled regulators should be locked with regulator's supplies at the
same time.
Add new function regulator_lock_dependent(), which locks all regulators
related with the one, that is being changed.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 23 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0b366c5..7c57268 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
}
}
-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+ unsigned int subclass)
{
+ struct regulator_dev *c_rdev;
int i;
- for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
- regulator_lock_nested(rdev, i);
+ for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+ c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+ if (!c_rdev)
+ continue;
+
+ regulator_lock_nested(c_rdev, subclass++);
+
+ if (c_rdev->supply)
+ subclass =
+ regulator_lock_recursive(c_rdev->supply->rdev,
+ subclass);
+ }
+
+ return subclass;
}
/**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev: regulator source
+ * regulator_unlock_dependent - unlock regulator's suppliers and coupled
+ * regulators
+ * @rdev: regulator source
+ *
+ * Unlock all regulators related with rdev by coupling or suppling.
*/
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
{
- struct regulator *supply;
+ struct regulator_dev *c_rdev;
+ int i;
- while (1) {
- regulator_unlock(rdev);
- supply = rdev->supply;
+ for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+ c_rdev = rdev->coupling_desc.coupled_rdevs[i];
- if (!rdev->supply)
- return;
+ if (!c_rdev)
+ continue;
+
+ regulator_unlock(c_rdev);
- rdev = supply->rdev;
+ if (c_rdev->supply)
+ regulator_unlock_dependent(c_rdev->supply->rdev);
}
}
/**
+ * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
+ * @rdev: regulator source
+ *
+ * This function as a wrapper on regulator_lock_recursive(), which locks
+ * all regulators related with rdev by coupling or suppling.
+ */
+static inline void regulator_lock_dependent(struct regulator_dev *rdev)
+{
+ regulator_lock_recursive(rdev, 0);
+}
+
+/**
* of_get_regulator - get a regulator device node based on supply name
* @dev: Device pointer for the consumer (of regulator) device
* @supply: regulator supply name
@@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
int ret = 0;
pr_err("%s: %d\n", __func__, __LINE__);
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
PM_SUSPEND_ON);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
@@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
if (regulator_check_states(state) || state == PM_SUSPEND_ON)
return -EINVAL;
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = _regulator_set_suspend_voltage(regulator, min_uV,
max_uV, state);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
@@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
{
int ret;
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = _regulator_get_voltage(regulator->rdev);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 5/7] regulator: core: Lock dependent regulators
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Implementing coupled regulators adds a new dependency between
regulators. Therefore, the current locking model should be changed.
Coupled regulators should be locked with regulator's supplies at the
same time.
Add new function regulator_lock_dependent(), which locks all regulators
related with the one, that is being changed.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 23 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0b366c5..7c57268 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
}
}
-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+ unsigned int subclass)
{
+ struct regulator_dev *c_rdev;
int i;
- for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
- regulator_lock_nested(rdev, i);
+ for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+ c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+ if (!c_rdev)
+ continue;
+
+ regulator_lock_nested(c_rdev, subclass++);
+
+ if (c_rdev->supply)
+ subclass =
+ regulator_lock_recursive(c_rdev->supply->rdev,
+ subclass);
+ }
+
+ return subclass;
}
/**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev: regulator source
+ * regulator_unlock_dependent - unlock regulator's suppliers and coupled
+ * regulators
+ * @rdev: regulator source
+ *
+ * Unlock all regulators related with rdev by coupling or suppling.
*/
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
{
- struct regulator *supply;
+ struct regulator_dev *c_rdev;
+ int i;
- while (1) {
- regulator_unlock(rdev);
- supply = rdev->supply;
+ for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+ c_rdev = rdev->coupling_desc.coupled_rdevs[i];
- if (!rdev->supply)
- return;
+ if (!c_rdev)
+ continue;
+
+ regulator_unlock(c_rdev);
- rdev = supply->rdev;
+ if (c_rdev->supply)
+ regulator_unlock_dependent(c_rdev->supply->rdev);
}
}
/**
+ * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
+ * @rdev: regulator source
+ *
+ * This function as a wrapper on regulator_lock_recursive(), which locks
+ * all regulators related with rdev by coupling or suppling.
+ */
+static inline void regulator_lock_dependent(struct regulator_dev *rdev)
+{
+ regulator_lock_recursive(rdev, 0);
+}
+
+/**
* of_get_regulator - get a regulator device node based on supply name
* @dev: Device pointer for the consumer (of regulator) device
* @supply: regulator supply name
@@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
int ret = 0;
pr_err("%s: %d\n", __func__, __LINE__);
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
PM_SUSPEND_ON);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
@@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
if (regulator_check_states(state) || state == PM_SUSPEND_ON)
return -EINVAL;
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = _regulator_set_suspend_voltage(regulator, min_uV,
max_uV, state);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
@@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
{
int ret;
- regulator_lock_supply(regulator->rdev);
+ regulator_lock_dependent(regulator->rdev);
ret = _regulator_get_voltage(regulator->rdev);
- regulator_unlock_supply(regulator->rdev);
+ regulator_unlock_dependent(regulator->rdev);
return ret;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH 5/7] regulator: core: Lock dependent regulators
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-04 14:20 ` Lucas Stach
-1 siblings, 0 replies; 81+ messages in thread
From: Lucas Stach @ 2018-06-04 14:20 UTC (permalink / raw)
To: Maciej Purski, Mark Brown, Tony Lindgren
Cc: linux-kernel, Carlos Hernandez, linux-omap, linux-arm-kernel,
Marek Szyprowski
Hi Maciej,
Am Montag, den 04.06.2018, 15:59 +0200 schrieb Maciej Purski:
> Implementing coupled regulators adds a new dependency between
> regulators. Therefore, the current locking model should be changed.
> Coupled regulators should be locked with regulator's supplies at the
> same time.
>
> Add new function regulator_lock_dependent(), which locks all regulators
> related with the one, that is being changed.
Sort of high level comment, but this doesn't look right: With dependent
regulators you don't strictly lock the regulators in the direction of
the tree root, but also siblings at the same level. This is prone with
deadlocks, as you can't control the order of the regulator locks being
taken by different tasks. This really needs a ww_mutex to be
implemented in a robust way.
Regards,
Lucas
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
> 1 file changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 0b366c5..7c57268 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
> > }
> }
>
> -/**
> - * regulator_lock_supply - lock a regulator and its supplies
> - * @rdev: regulator source
> - */
> -static void regulator_lock_supply(struct regulator_dev *rdev)
> +static int regulator_lock_recursive(struct regulator_dev *rdev,
> > + unsigned int subclass)
> {
> > + struct regulator_dev *c_rdev;
> > int i;
>
> > - for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
> > - regulator_lock_nested(rdev, i);
> > + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
> > + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
> +
> > + if (!c_rdev)
> > + continue;
> +
> > + regulator_lock_nested(c_rdev, subclass++);
> +
> > + if (c_rdev->supply)
> > + subclass =
> > + regulator_lock_recursive(c_rdev->supply->rdev,
> > + subclass);
> > + }
> +
> > + return subclass;
> }
>
> /**
> - * regulator_unlock_supply - unlock a regulator and its supplies
> - * @rdev: regulator source
> + * regulator_unlock_dependent - unlock regulator's suppliers and coupled
> > + * regulators
> > + * @rdev: regulator source
> + *
> + * Unlock all regulators related with rdev by coupling or suppling.
> */
> -static void regulator_unlock_supply(struct regulator_dev *rdev)
> +static void regulator_unlock_dependent(struct regulator_dev *rdev)
> {
> > - struct regulator *supply;
> > + struct regulator_dev *c_rdev;
> > + int i;
>
> > - while (1) {
> > - regulator_unlock(rdev);
> > - supply = rdev->supply;
> > + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
> > + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
>
> > - if (!rdev->supply)
> > - return;
> > + if (!c_rdev)
> > + continue;
> +
> > + regulator_unlock(c_rdev);
>
> > - rdev = supply->rdev;
> > + if (c_rdev->supply)
> > + regulator_unlock_dependent(c_rdev->supply->rdev);
> > }
> }
>
> /**
> + * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
> > + * @rdev: regulator source
> + *
> + * This function as a wrapper on regulator_lock_recursive(), which locks
> + * all regulators related with rdev by coupling or suppling.
> + */
> +static inline void regulator_lock_dependent(struct regulator_dev *rdev)
> +{
> > + regulator_lock_recursive(rdev, 0);
> +}
> +
> +/**
> * of_get_regulator - get a regulator device node based on supply name
> * @dev: Device pointer for the consumer (of regulator) device
> * @supply: regulator supply name
> @@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
> > int ret = 0;
>
> > pr_err("%s: %d\n", __func__, __LINE__);
> > - regulator_lock_supply(regulator->rdev);
> > + regulator_lock_dependent(regulator->rdev);
>
> > ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
> > PM_SUSPEND_ON);
>
> > - regulator_unlock_supply(regulator->rdev);
> > + regulator_unlock_dependent(regulator->rdev);
>
> > return ret;
> }
> @@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
> > if (regulator_check_states(state) || state == PM_SUSPEND_ON)
> > return -EINVAL;
>
> > - regulator_lock_supply(regulator->rdev);
> > + regulator_lock_dependent(regulator->rdev);
>
> > ret = _regulator_set_suspend_voltage(regulator, min_uV,
> > max_uV, state);
>
> > - regulator_unlock_supply(regulator->rdev);
> > + regulator_unlock_dependent(regulator->rdev);
>
> > return ret;
> }
> @@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
> {
> > int ret;
>
> > - regulator_lock_supply(regulator->rdev);
> > + regulator_lock_dependent(regulator->rdev);
>
> > ret = _regulator_get_voltage(regulator->rdev);
>
> > - regulator_unlock_supply(regulator->rdev);
> > + regulator_unlock_dependent(regulator->rdev);
>
> > return ret;
> }
^ permalink raw reply [flat|nested] 81+ messages in thread* [PATCH 5/7] regulator: core: Lock dependent regulators
@ 2018-06-04 14:20 ` Lucas Stach
0 siblings, 0 replies; 81+ messages in thread
From: Lucas Stach @ 2018-06-04 14:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maciej,
Am Montag, den 04.06.2018, 15:59 +0200 schrieb Maciej Purski:
> Implementing coupled regulators adds a new dependency between
> regulators. Therefore, the current locking model should be changed.
> Coupled regulators should be locked with regulator's supplies at the
> same time.
>
> Add new function regulator_lock_dependent(), which locks all regulators
> related with the one, that is being changed.
Sort of high level comment, but this doesn't look right: With dependent
regulators you don't strictly lock the regulators in the direction of
the tree root, but also siblings at the same level. This is prone with
deadlocks, as you can't control the order of the regulator locks being
taken by different tasks. This really needs a ww_mutex to be
implemented in a robust way.
Regards,
Lucas
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> ?drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
> ?1 file changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 0b366c5..7c57268 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
> > ? }
> ?}
> ?
> -/**
> - * regulator_lock_supply - lock a regulator and its supplies
> - * @rdev:?????????regulator source
> - */
> -static void regulator_lock_supply(struct regulator_dev *rdev)
> +static int regulator_lock_recursive(struct regulator_dev *rdev,
> > + ????unsigned int subclass)
> ?{
> > + struct regulator_dev *c_rdev;
> > ? int i;
> ?
> > - for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
> > - regulator_lock_nested(rdev, i);
> > + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
> > + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
> +
> > + if (!c_rdev)
> > + continue;
> +
> > + regulator_lock_nested(c_rdev, subclass++);
> +
> > + if (c_rdev->supply)
> > + subclass =
> > + regulator_lock_recursive(c_rdev->supply->rdev,
> > + ?subclass);
> > + }
> +
> > + return subclass;
> ?}
> ?
> ?/**
> - * regulator_unlock_supply - unlock a regulator and its supplies
> - * @rdev:?????????regulator source
> + * regulator_unlock_dependent - unlock regulator's suppliers and coupled
> > + * regulators
> > + * @rdev: regulator source
> + *
> + * Unlock all regulators related with rdev by coupling or suppling.
> ? */
> -static void regulator_unlock_supply(struct regulator_dev *rdev)
> +static void regulator_unlock_dependent(struct regulator_dev *rdev)
> ?{
> > - struct regulator *supply;
> > + struct regulator_dev *c_rdev;
> > + int i;
> ?
> > - while (1) {
> > - regulator_unlock(rdev);
> > - supply = rdev->supply;
> > + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
> > + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
> ?
> > - if (!rdev->supply)
> > - return;
> > + if (!c_rdev)
> > + continue;
> +
> > + regulator_unlock(c_rdev);
> ?
> > - rdev = supply->rdev;
> > + if (c_rdev->supply)
> > + regulator_unlock_dependent(c_rdev->supply->rdev);
> > ? }
> ?}
> ?
> ?/**
> + * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
> > + * @rdev: regulator source
> + *
> + * This function as a wrapper on regulator_lock_recursive(), which locks
> + * all regulators related with rdev by coupling or suppling.
> + */
> +static inline void regulator_lock_dependent(struct regulator_dev *rdev)
> +{
> > + regulator_lock_recursive(rdev, 0);
> +}
> +
> +/**
> ? * of_get_regulator - get a regulator device node based on supply name
> ? * @dev: Device pointer for the consumer (of regulator) device
> ? * @supply: regulator supply name
> @@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
> > ? int ret = 0;
> ?
> > ? pr_err("%s: %d\n", __func__, __LINE__);
> > - regulator_lock_supply(regulator->rdev);
> > + regulator_lock_dependent(regulator->rdev);
> ?
> > ? ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
> > ? ?????PM_SUSPEND_ON);
> ?
> > - regulator_unlock_supply(regulator->rdev);
> > + regulator_unlock_dependent(regulator->rdev);
> ?
> > ? return ret;
> ?}
> @@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
> > ? if (regulator_check_states(state) || state == PM_SUSPEND_ON)
> > ? return -EINVAL;
> ?
> > - regulator_lock_supply(regulator->rdev);
> > + regulator_lock_dependent(regulator->rdev);
> ?
> > ? ret = _regulator_set_suspend_voltage(regulator, min_uV,
> > ? ?????max_uV, state);
> ?
> > - regulator_unlock_supply(regulator->rdev);
> > + regulator_unlock_dependent(regulator->rdev);
> ?
> > ? return ret;
> ?}
> @@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
> ?{
> > ? int ret;
> ?
> > - regulator_lock_supply(regulator->rdev);
> > + regulator_lock_dependent(regulator->rdev);
> ?
> > ? ret = _regulator_get_voltage(regulator->rdev);
> ?
> > - regulator_unlock_supply(regulator->rdev);
> > + regulator_unlock_dependent(regulator->rdev);
> ?
> > ? return ret;
> ?}
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [PATCH 5/7] regulator: core: Lock dependent regulators
2018-06-04 14:20 ` Lucas Stach
@ 2018-06-18 12:37 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-18 12:37 UTC (permalink / raw)
To: Lucas Stach, Mark Brown, Tony Lindgren
Cc: linux-kernel, Carlos Hernandez, linux-omap, linux-arm-kernel,
Marek Szyprowski
On 06/04/2018 04:20 PM, Lucas Stach wrote:
> Hi Maciej,
>
> Am Montag, den 04.06.2018, 15:59 +0200 schrieb Maciej Purski:
>> Implementing coupled regulators adds a new dependency between
>> regulators. Therefore, the current locking model should be changed.
>> Coupled regulators should be locked with regulator's supplies at the
>> same time.
>>
>> Add new function regulator_lock_dependent(), which locks all regulators
>> related with the one, that is being changed.
>
> Sort of high level comment, but this doesn't look right: With dependent
> regulators you don't strictly lock the regulators in the direction of
> the tree root, but also siblings at the same level. This is prone with
> deadlocks, as you can't control the order of the regulator locks being
> taken by different tasks. This really needs a ww_mutex to be
> implemented in a robust way.
>
> Regards,
> Lucas
>
Thanks for that. You are right and it needs fixing. I'll consider it in my
next version. However, that error should arise only, when we have the actual
coupled regulators, so it shouldn't be a problem in Tony's case.
Best regards,
Maciej Purski
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>> drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index 0b366c5..7c57268 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
>>> }
>> }
>>
>> -/**
>> - * regulator_lock_supply - lock a regulator and its supplies
>> - * @rdev: regulator source
>> - */
>> -static void regulator_lock_supply(struct regulator_dev *rdev)
>> +static int regulator_lock_recursive(struct regulator_dev *rdev,
>>> + unsigned int subclass)
>> {
>>> + struct regulator_dev *c_rdev;
>>> int i;
>>
>>> - for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
>>> - regulator_lock_nested(rdev, i);
>>> + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
>>> + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
>> +
>>> + if (!c_rdev)
>>> + continue;
>> +
>>> + regulator_lock_nested(c_rdev, subclass++);
>> +
>>> + if (c_rdev->supply)
>>> + subclass =
>>> + regulator_lock_recursive(c_rdev->supply->rdev,
>>> + subclass);
>>> + }
>> +
>>> + return subclass;
>> }
>>
>> /**
>> - * regulator_unlock_supply - unlock a regulator and its supplies
>> - * @rdev: regulator source
>> + * regulator_unlock_dependent - unlock regulator's suppliers and coupled
>>> + * regulators
>>> + * @rdev: regulator source
>> + *
>> + * Unlock all regulators related with rdev by coupling or suppling.
>> */
>> -static void regulator_unlock_supply(struct regulator_dev *rdev)
>> +static void regulator_unlock_dependent(struct regulator_dev *rdev)
>> {
>>> - struct regulator *supply;
>>> + struct regulator_dev *c_rdev;
>>> + int i;
>>
>>> - while (1) {
>>> - regulator_unlock(rdev);
>>> - supply = rdev->supply;
>>> + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
>>> + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
>>
>>> - if (!rdev->supply)
>>> - return;
>>> + if (!c_rdev)
>>> + continue;
>> +
>>> + regulator_unlock(c_rdev);
>>
>>> - rdev = supply->rdev;
>>> + if (c_rdev->supply)
>>> + regulator_unlock_dependent(c_rdev->supply->rdev);
>>> }
>> }
>>
>> /**
>> + * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
>>> + * @rdev: regulator source
>> + *
>> + * This function as a wrapper on regulator_lock_recursive(), which locks
>> + * all regulators related with rdev by coupling or suppling.
>> + */
>> +static inline void regulator_lock_dependent(struct regulator_dev *rdev)
>> +{
>>> + regulator_lock_recursive(rdev, 0);
>> +}
>> +
>> +/**
>> * of_get_regulator - get a regulator device node based on supply name
>> * @dev: Device pointer for the consumer (of regulator) device
>> * @supply: regulator supply name
>> @@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
>>> int ret = 0;
>>
>>> pr_err("%s: %d\n", __func__, __LINE__);
>>> - regulator_lock_supply(regulator->rdev);
>>> + regulator_lock_dependent(regulator->rdev);
>>
>>> ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
>>> PM_SUSPEND_ON);
>>
>>> - regulator_unlock_supply(regulator->rdev);
>>> + regulator_unlock_dependent(regulator->rdev);
>>
>>> return ret;
>> }
>> @@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
>>> if (regulator_check_states(state) || state == PM_SUSPEND_ON)
>>> return -EINVAL;
>>
>>> - regulator_lock_supply(regulator->rdev);
>>> + regulator_lock_dependent(regulator->rdev);
>>
>>> ret = _regulator_set_suspend_voltage(regulator, min_uV,
>>> max_uV, state);
>>
>>> - regulator_unlock_supply(regulator->rdev);
>>> + regulator_unlock_dependent(regulator->rdev);
>>
>>> return ret;
>> }
>> @@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
>> {
>>> int ret;
>>
>>> - regulator_lock_supply(regulator->rdev);
>>> + regulator_lock_dependent(regulator->rdev);
>>
>>> ret = _regulator_get_voltage(regulator->rdev);
>>
>>> - regulator_unlock_supply(regulator->rdev);
>>> + regulator_unlock_dependent(regulator->rdev);
>>
>>> return ret;
>> }
>
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread* [PATCH 5/7] regulator: core: Lock dependent regulators
@ 2018-06-18 12:37 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-18 12:37 UTC (permalink / raw)
To: linux-arm-kernel
On 06/04/2018 04:20 PM, Lucas Stach wrote:
> Hi Maciej,
>
> Am Montag, den 04.06.2018, 15:59 +0200 schrieb Maciej Purski:
>> Implementing coupled regulators adds a new dependency between
>> regulators. Therefore, the current locking model should be changed.
>> Coupled regulators should be locked with regulator's supplies at the
>> same time.
>>
>> Add new function regulator_lock_dependent(), which locks all regulators
>> related with the one, that is being changed.
>
> Sort of high level comment, but this doesn't look right: With dependent
> regulators you don't strictly lock the regulators in the direction of
> the tree root, but also siblings at the same level. This is prone with
> deadlocks, as you can't control the order of the regulator locks being
> taken by different tasks. This really needs a ww_mutex to be
> implemented in a robust way.
>
> Regards,
> Lucas
>
Thanks for that. You are right and it needs fixing. I'll consider it in my
next version. However, that error should arise only, when we have the actual
coupled regulators, so it shouldn't be a problem in Tony's case.
Best regards,
Maciej Purski
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>> ?drivers/regulator/core.c | 75 +++++++++++++++++++++++++++++++++---------------
>> ?1 file changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index 0b366c5..7c57268 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -201,38 +201,67 @@ static void regulator_unlock(struct regulator_dev *rdev)
>>> ? }
>> ?}
>>
>> -/**
>> - * regulator_lock_supply - lock a regulator and its supplies
>> - * @rdev:?????????regulator source
>> - */
>> -static void regulator_lock_supply(struct regulator_dev *rdev)
>> +static int regulator_lock_recursive(struct regulator_dev *rdev,
>>> + ????unsigned int subclass)
>> ?{
>>> + struct regulator_dev *c_rdev;
>>> ? int i;
>>
>>> - for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
>>> - regulator_lock_nested(rdev, i);
>>> + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
>>> + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
>> +
>>> + if (!c_rdev)
>>> + continue;
>> +
>>> + regulator_lock_nested(c_rdev, subclass++);
>> +
>>> + if (c_rdev->supply)
>>> + subclass =
>>> + regulator_lock_recursive(c_rdev->supply->rdev,
>>> + ?subclass);
>>> + }
>> +
>>> + return subclass;
>> ?}
>>
>> ?/**
>> - * regulator_unlock_supply - unlock a regulator and its supplies
>> - * @rdev:?????????regulator source
>> + * regulator_unlock_dependent - unlock regulator's suppliers and coupled
>>> + * regulators
>>> + * @rdev: regulator source
>> + *
>> + * Unlock all regulators related with rdev by coupling or suppling.
>> ? */
>> -static void regulator_unlock_supply(struct regulator_dev *rdev)
>> +static void regulator_unlock_dependent(struct regulator_dev *rdev)
>> ?{
>>> - struct regulator *supply;
>>> + struct regulator_dev *c_rdev;
>>> + int i;
>>
>>> - while (1) {
>>> - regulator_unlock(rdev);
>>> - supply = rdev->supply;
>>> + for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
>>> + c_rdev = rdev->coupling_desc.coupled_rdevs[i];
>>
>>> - if (!rdev->supply)
>>> - return;
>>> + if (!c_rdev)
>>> + continue;
>> +
>>> + regulator_unlock(c_rdev);
>>
>>> - rdev = supply->rdev;
>>> + if (c_rdev->supply)
>>> + regulator_unlock_dependent(c_rdev->supply->rdev);
>>> ? }
>> ?}
>>
>> ?/**
>> + * regulator_lock_dependent - lock regulator's suppliers and coupled regulators
>>> + * @rdev: regulator source
>> + *
>> + * This function as a wrapper on regulator_lock_recursive(), which locks
>> + * all regulators related with rdev by coupling or suppling.
>> + */
>> +static inline void regulator_lock_dependent(struct regulator_dev *rdev)
>> +{
>>> + regulator_lock_recursive(rdev, 0);
>> +}
>> +
>> +/**
>> ? * of_get_regulator - get a regulator device node based on supply name
>> ? * @dev: Device pointer for the consumer (of regulator) device
>> ? * @supply: regulator supply name
>> @@ -3332,12 +3361,12 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
>>> ? int ret = 0;
>>
>>> ? pr_err("%s: %d\n", __func__, __LINE__);
>>> - regulator_lock_supply(regulator->rdev);
>>> + regulator_lock_dependent(regulator->rdev);
>>
>>> ? ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
>>> ? ?????PM_SUSPEND_ON);
>>
>>> - regulator_unlock_supply(regulator->rdev);
>>> + regulator_unlock_dependent(regulator->rdev);
>>
>>> ? return ret;
>> ?}
>> @@ -3415,12 +3444,12 @@ int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
>>> ? if (regulator_check_states(state) || state == PM_SUSPEND_ON)
>>> ? return -EINVAL;
>>
>>> - regulator_lock_supply(regulator->rdev);
>>> + regulator_lock_dependent(regulator->rdev);
>>
>>> ? ret = _regulator_set_suspend_voltage(regulator, min_uV,
>>> ? ?????max_uV, state);
>>
>>> - regulator_unlock_supply(regulator->rdev);
>>> + regulator_unlock_dependent(regulator->rdev);
>>
>>> ? return ret;
>> ?}
>> @@ -3612,11 +3641,11 @@ int regulator_get_voltage(struct regulator *regulator)
>> ?{
>>> ? int ret;
>>
>>> - regulator_lock_supply(regulator->rdev);
>>> + regulator_lock_dependent(regulator->rdev);
>>
>>> ? ret = _regulator_get_voltage(regulator->rdev);
>>
>>> - regulator_unlock_supply(regulator->rdev);
>>> + regulator_unlock_dependent(regulator->rdev);
>>
>>> ? return ret;
>> ?}
>
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH 6/7] regulator: core: Lock dependent regulators on regulator_enable()
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
Since regulator_enable() might now call regulator_balance_voltage(),
it should also lock its coupled regulators and suppliers.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7c57268..2a7ffb7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2305,9 +2305,9 @@ int regulator_enable(struct regulator *regulator)
return ret;
}
- regulator_lock(rdev);
+ regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
- regulator_unlock(rdev);
+ regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2415,9 +2415,9 @@ int regulator_disable(struct regulator *regulator)
if (regulator->always_on)
return 0;
- regulator_lock(rdev);
+ regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
- regulator_unlock(rdev);
+ regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2467,10 +2467,10 @@ int regulator_force_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret;
- regulator_lock(rdev);
+ regulator_lock_dependent(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
- regulator_unlock(rdev);
+ regulator_unlock_dependent(rdev);
if (rdev->supply)
while (rdev->open_count--)
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 6/7] regulator: core: Lock dependent regulators on regulator_enable()
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Since regulator_enable() might now call regulator_balance_voltage(),
it should also lock its coupled regulators and suppliers.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7c57268..2a7ffb7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2305,9 +2305,9 @@ int regulator_enable(struct regulator *regulator)
return ret;
}
- regulator_lock(rdev);
+ regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
- regulator_unlock(rdev);
+ regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2415,9 +2415,9 @@ int regulator_disable(struct regulator *regulator)
if (regulator->always_on)
return 0;
- regulator_lock(rdev);
+ regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
- regulator_unlock(rdev);
+ regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
@@ -2467,10 +2467,10 @@ int regulator_force_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret;
- regulator_lock(rdev);
+ regulator_lock_dependent(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
- regulator_unlock(rdev);
+ regulator_unlock_dependent(rdev);
if (rdev->supply)
while (rdev->open_count--)
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread
* [PATCH 7/7] regulator: core: Enable voltage balancing
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-04 13:59 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: Mark Brown, Tony Lindgren
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski, Maciej Purski
Call regulator_balance_voltage() instead of set_voltage_rdev()
in set_voltage_unlocked() and in enabling and disabling functions,
but only if the regulator is coupled.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2a7ffb7..2dd1f99 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2296,6 +2296,11 @@ int regulator_enable(struct regulator *regulator)
int ret = 0;
pr_err("%s: %d\n", __func__, __LINE__);
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ return -EPERM;
+ }
+
if (regulator->always_on)
return 0;
@@ -2307,6 +2312,9 @@ int regulator_enable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
+ /* balance only if there are regulators coupled */
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2417,6 +2425,8 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2470,6 +2480,8 @@ int regulator_force_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3031,7 +3043,16 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int old_min_uV, old_max_uV;
int current_uV;
+<<<<<<< HEAD
pr_err("%s: %d\n", __func__, __LINE__);
+=======
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ ret = -EPERM;
+ goto out;
+ }
+
+>>>>>>> fcbf6fa... regulator: core: Enable voltage balancing
/* If we're setting the same range as last time the change
* should be a noop (some cpufreq implementations use the same
* voltage for multiple frequencies, for example).
@@ -3074,7 +3095,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret < 0)
goto out2;
- ret = regulator_set_voltage_rdev(rdev, min_uV, max_uV, state);
+ /* for not coupled regulators this will just set the voltage */
+ ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
goto out2;
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH 7/7] regulator: core: Enable voltage balancing
@ 2018-06-04 13:59 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-04 13:59 UTC (permalink / raw)
To: linux-arm-kernel
Call regulator_balance_voltage() instead of set_voltage_rdev()
in set_voltage_unlocked() and in enabling and disabling functions,
but only if the regulator is coupled.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2a7ffb7..2dd1f99 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2296,6 +2296,11 @@ int regulator_enable(struct regulator *regulator)
int ret = 0;
pr_err("%s: %d\n", __func__, __LINE__);
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ return -EPERM;
+ }
+
if (regulator->always_on)
return 0;
@@ -2307,6 +2312,9 @@ int regulator_enable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
+ /* balance only if there are regulators coupled */
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2417,6 +2425,8 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2470,6 +2480,8 @@ int regulator_force_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3031,7 +3043,16 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int old_min_uV, old_max_uV;
int current_uV;
+<<<<<<< HEAD
pr_err("%s: %d\n", __func__, __LINE__);
+=======
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ ret = -EPERM;
+ goto out;
+ }
+
+>>>>>>> fcbf6fa... regulator: core: Enable voltage balancing
/* If we're setting the same range as last time the change
* should be a noop (some cpufreq implementations use the same
* voltage for multiple frequencies, for example).
@@ -3074,7 +3095,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret < 0)
goto out2;
- ret = regulator_set_voltage_rdev(rdev, min_uV, max_uV, state);
+ /* for not coupled regulators this will just set the voltage */
+ ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
goto out2;
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH 7/7] regulator: core: Enable voltage balancing
2018-06-04 13:59 ` Maciej Purski
(?)
@ 2018-06-04 23:13 ` kbuild test robot
-1 siblings, 0 replies; 81+ messages in thread
From: kbuild test robot @ 2018-06-04 23:13 UTC (permalink / raw)
Cc: kbuild-all, Mark Brown, Tony Lindgren, linux-kernel,
linux-arm-kernel, linux-omap, Carlos Hernandez, Marek Szyprowski,
Maciej Purski
[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regulator/for-next]
[also build test ERROR on next-20180604]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/regulator-core-Add-debug-messages/20180605-052333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-x011-201822 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers//regulator/core.c: In function 'regulator_set_voltage_unlocked':
>> drivers//regulator/core.c:3046:1: error: version control conflict marker in file
<<<<<<< HEAD
^~~~~~~
drivers//regulator/core.c:3048:1: error: version control conflict marker in file
=======
^~~~~~~
vim +3046 drivers//regulator/core.c
3035
3036 static int regulator_set_voltage_unlocked(struct regulator *regulator,
3037 int min_uV, int max_uV,
3038 suspend_state_t state)
3039 {
3040 struct regulator_dev *rdev = regulator->rdev;
3041 struct regulator_voltage *voltage = ®ulator->voltage[state];
3042 int ret = 0;
3043 int old_min_uV, old_max_uV;
3044 int current_uV;
3045
> 3046 <<<<<<< HEAD
3047 pr_err("%s: %d\n", __func__, __LINE__);
3048 =======
3049 if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
3050 rdev_err(rdev, "not all coupled regulators registered\n");
3051 ret = -EPERM;
3052 goto out;
3053 }
3054
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34005 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [PATCH 7/7] regulator: core: Enable voltage balancing
@ 2018-06-04 23:13 ` kbuild test robot
0 siblings, 0 replies; 81+ messages in thread
From: kbuild test robot @ 2018-06-04 23:13 UTC (permalink / raw)
To: Maciej Purski
Cc: kbuild-all, Mark Brown, Tony Lindgren, linux-kernel,
linux-arm-kernel, linux-omap, Carlos Hernandez, Marek Szyprowski,
Maciej Purski
[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regulator/for-next]
[also build test ERROR on next-20180604]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/regulator-core-Add-debug-messages/20180605-052333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-x011-201822 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers//regulator/core.c: In function 'regulator_set_voltage_unlocked':
>> drivers//regulator/core.c:3046:1: error: version control conflict marker in file
<<<<<<< HEAD
^~~~~~~
drivers//regulator/core.c:3048:1: error: version control conflict marker in file
=======
^~~~~~~
vim +3046 drivers//regulator/core.c
3035
3036 static int regulator_set_voltage_unlocked(struct regulator *regulator,
3037 int min_uV, int max_uV,
3038 suspend_state_t state)
3039 {
3040 struct regulator_dev *rdev = regulator->rdev;
3041 struct regulator_voltage *voltage = ®ulator->voltage[state];
3042 int ret = 0;
3043 int old_min_uV, old_max_uV;
3044 int current_uV;
3045
> 3046 <<<<<<< HEAD
3047 pr_err("%s: %d\n", __func__, __LINE__);
3048 =======
3049 if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
3050 rdev_err(rdev, "not all coupled regulators registered\n");
3051 ret = -EPERM;
3052 goto out;
3053 }
3054
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34005 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* [PATCH 7/7] regulator: core: Enable voltage balancing
@ 2018-06-04 23:13 ` kbuild test robot
0 siblings, 0 replies; 81+ messages in thread
From: kbuild test robot @ 2018-06-04 23:13 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regulator/for-next]
[also build test ERROR on next-20180604]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/regulator-core-Add-debug-messages/20180605-052333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-x011-201822 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers//regulator/core.c: In function 'regulator_set_voltage_unlocked':
>> drivers//regulator/core.c:3046:1: error: version control conflict marker in file
<<<<<<< HEAD
^~~~~~~
drivers//regulator/core.c:3048:1: error: version control conflict marker in file
=======
^~~~~~~
vim +3046 drivers//regulator/core.c
3035
3036 static int regulator_set_voltage_unlocked(struct regulator *regulator,
3037 int min_uV, int max_uV,
3038 suspend_state_t state)
3039 {
3040 struct regulator_dev *rdev = regulator->rdev;
3041 struct regulator_voltage *voltage = ®ulator->voltage[state];
3042 int ret = 0;
3043 int old_min_uV, old_max_uV;
3044 int current_uV;
3045
> 3046 <<<<<<< HEAD
3047 pr_err("%s: %d\n", __func__, __LINE__);
3048 =======
3049 if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
3050 rdev_err(rdev, "not all coupled regulators registered\n");
3051 ret = -EPERM;
3052 goto out;
3053 }
3054
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 34005 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180605/c5e61c9a/attachment-0001.gz>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH 7/7] regulator: core: Enable voltage balancing
2018-06-04 13:59 ` Maciej Purski
(?)
@ 2018-06-04 23:54 ` kbuild test robot
-1 siblings, 0 replies; 81+ messages in thread
From: kbuild test robot @ 2018-06-04 23:54 UTC (permalink / raw)
Cc: kbuild-all, Mark Brown, Tony Lindgren, linux-kernel,
linux-arm-kernel, linux-omap, Carlos Hernandez, Marek Szyprowski,
Maciej Purski
[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regulator/for-next]
[also build test ERROR on next-20180604]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/regulator-core-Add-debug-messages/20180605-052333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: i386-randconfig-a1-06041847 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/regulator/core.c: In function 'regulator_set_voltage_unlocked':
>> drivers/regulator/core.c:3046:1: error: expected expression before '<<' token
<<<<<<< HEAD
^
drivers/regulator/core.c:3048:1: error: expected expression before '==' token
=======
^
vim +3046 drivers/regulator/core.c
3035
3036 static int regulator_set_voltage_unlocked(struct regulator *regulator,
3037 int min_uV, int max_uV,
3038 suspend_state_t state)
3039 {
3040 struct regulator_dev *rdev = regulator->rdev;
3041 struct regulator_voltage *voltage = ®ulator->voltage[state];
3042 int ret = 0;
3043 int old_min_uV, old_max_uV;
3044 int current_uV;
3045
> 3046 <<<<<<< HEAD
3047 pr_err("%s: %d\n", __func__, __LINE__);
3048 =======
3049 if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
3050 rdev_err(rdev, "not all coupled regulators registered\n");
3051 ret = -EPERM;
3052 goto out;
3053 }
3054
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26101 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [PATCH 7/7] regulator: core: Enable voltage balancing
@ 2018-06-04 23:54 ` kbuild test robot
0 siblings, 0 replies; 81+ messages in thread
From: kbuild test robot @ 2018-06-04 23:54 UTC (permalink / raw)
To: Maciej Purski
Cc: kbuild-all, Mark Brown, Tony Lindgren, linux-kernel,
linux-arm-kernel, linux-omap, Carlos Hernandez, Marek Szyprowski,
Maciej Purski
[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regulator/for-next]
[also build test ERROR on next-20180604]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/regulator-core-Add-debug-messages/20180605-052333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: i386-randconfig-a1-06041847 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/regulator/core.c: In function 'regulator_set_voltage_unlocked':
>> drivers/regulator/core.c:3046:1: error: expected expression before '<<' token
<<<<<<< HEAD
^
drivers/regulator/core.c:3048:1: error: expected expression before '==' token
=======
^
vim +3046 drivers/regulator/core.c
3035
3036 static int regulator_set_voltage_unlocked(struct regulator *regulator,
3037 int min_uV, int max_uV,
3038 suspend_state_t state)
3039 {
3040 struct regulator_dev *rdev = regulator->rdev;
3041 struct regulator_voltage *voltage = ®ulator->voltage[state];
3042 int ret = 0;
3043 int old_min_uV, old_max_uV;
3044 int current_uV;
3045
> 3046 <<<<<<< HEAD
3047 pr_err("%s: %d\n", __func__, __LINE__);
3048 =======
3049 if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
3050 rdev_err(rdev, "not all coupled regulators registered\n");
3051 ret = -EPERM;
3052 goto out;
3053 }
3054
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26101 bytes --]
^ permalink raw reply [flat|nested] 81+ messages in thread* [PATCH 7/7] regulator: core: Enable voltage balancing
@ 2018-06-04 23:54 ` kbuild test robot
0 siblings, 0 replies; 81+ messages in thread
From: kbuild test robot @ 2018-06-04 23:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi Maciej,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on regulator/for-next]
[also build test ERROR on next-20180604]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maciej-Purski/regulator-core-Add-debug-messages/20180605-052333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: i386-randconfig-a1-06041847 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/regulator/core.c: In function 'regulator_set_voltage_unlocked':
>> drivers/regulator/core.c:3046:1: error: expected expression before '<<' token
<<<<<<< HEAD
^
drivers/regulator/core.c:3048:1: error: expected expression before '==' token
=======
^
vim +3046 drivers/regulator/core.c
3035
3036 static int regulator_set_voltage_unlocked(struct regulator *regulator,
3037 int min_uV, int max_uV,
3038 suspend_state_t state)
3039 {
3040 struct regulator_dev *rdev = regulator->rdev;
3041 struct regulator_voltage *voltage = ®ulator->voltage[state];
3042 int ret = 0;
3043 int old_min_uV, old_max_uV;
3044 int current_uV;
3045
> 3046 <<<<<<< HEAD
3047 pr_err("%s: %d\n", __func__, __LINE__);
3048 =======
3049 if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
3050 rdev_err(rdev, "not all coupled regulators registered\n");
3051 ret = -EPERM;
3052 goto out;
3053 }
3054
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 26101 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180605/699454b6/attachment-0001.gz>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: Regression in Linux next again
2018-06-04 13:59 ` Maciej Purski
@ 2018-06-05 4:45 ` Tony Lindgren
-1 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-06-05 4:45 UTC (permalink / raw)
To: Maciej Purski
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski
Hi,
* Maciej Purski <m.purski@samsung.com> [180604 14:02]:
> Tony, please apply this patchset and test it on your Beaglebone. It'd be
> great if you could try to find out, which patch causes failure. They should
> be appliable on the current next.
It seems beagle-x15 boots for me except with the last patch in the
series with compile issue fixed. With that applied after modules
get loaded it just hangs with:
[ 25.925607] regulator_resolve_supply: 1608
[ 25.930620] regulator_resolve_supply: 1608
[ 26.098449] lib80211: common routines for IEEE802.11 drivers
[ 26.446713] cfg80211: Loading compiled-in X.509 certificates for regulatory database
[ 26.514834] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[ 26.525311] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2
[ 26.534250] cfg80211: failed to load regulatory.db
[ 26.998736] regulator_set_voltage: 3381
[ 27.007662] _regulator_do_set_voltage: 2913
[ 27.012593] regulator_set_voltage: 3381
[ 27.016969] _regulator_do_set_voltage: 2913
[ 27.022847] regulator_set_voltage: 3381
Not sure what module gets loaded at that point, I'll test again
with initcall_debug once you post a compiling version of your last
patch.
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* Regression in Linux next again
@ 2018-06-05 4:45 ` Tony Lindgren
0 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-06-05 4:45 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
* Maciej Purski <m.purski@samsung.com> [180604 14:02]:
> Tony, please apply this patchset and test it on your Beaglebone. It'd be
> great if you could try to find out, which patch causes failure. They should
> be appliable on the current next.
It seems beagle-x15 boots for me except with the last patch in the
series with compile issue fixed. With that applied after modules
get loaded it just hangs with:
[ 25.925607] regulator_resolve_supply: 1608
[ 25.930620] regulator_resolve_supply: 1608
[ 26.098449] lib80211: common routines for IEEE802.11 drivers
[ 26.446713] cfg80211: Loading compiled-in X.509 certificates for regulatory database
[ 26.514834] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[ 26.525311] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2
[ 26.534250] cfg80211: failed to load regulatory.db
[ 26.998736] regulator_set_voltage: 3381
[ 27.007662] _regulator_do_set_voltage: 2913
[ 27.012593] regulator_set_voltage: 3381
[ 27.016969] _regulator_do_set_voltage: 2913
[ 27.022847] regulator_set_voltage: 3381
Not sure what module gets loaded at that point, I'll test again
with initcall_debug once you post a compiling version of your last
patch.
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH v2] regulator: core: Enable voltage balancing
2018-06-05 4:45 ` Tony Lindgren
@ 2018-06-13 10:33 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-13 10:33 UTC (permalink / raw)
To: Tony Lindgren
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski, Maciej Purski
Call regulator_balance_voltage() instead of set_voltage_rdev()
in set_voltage_unlocked() and in enabling and disabling functions,
but only if the regulator is coupled.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
Changes in v2:
- fix compile errors
- make debug messages more informative
---
drivers/regulator/core.c | 38 ++++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2a7ffb7..266f4eb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1605,7 +1605,6 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
struct device *dev = rdev->dev.parent;
int ret;
- pr_err("%s: %d\n", __func__, __LINE__);
/* No supply to resovle? */
if (!rdev->supply_name)
return 0;
@@ -2246,7 +2245,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
{
int ret;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
lockdep_assert_held_once(&rdev->mutex);
/* check voltage and requested load before enabling */
@@ -2295,7 +2294,12 @@ int regulator_enable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ return -EPERM;
+ }
+
if (regulator->always_on)
return 0;
@@ -2307,12 +2311,15 @@ int regulator_enable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
+ /* balance only if there are regulators coupled */
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2411,18 +2418,20 @@ int regulator_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_disable);
@@ -2470,6 +2479,8 @@ int regulator_force_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -2898,7 +2909,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
const struct regulator_ops *ops = rdev->desc->ops;
int old_uV = _regulator_get_voltage(rdev);
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
min_uV += rdev->constraints->uV_offset;
@@ -3031,7 +3042,13 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int old_min_uV, old_max_uV;
int current_uV;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ ret = -EPERM;
+ goto out;
+ }
+
/* If we're setting the same range as last time the change
* should be a noop (some cpufreq implementations use the same
* voltage for multiple frequencies, for example).
@@ -3074,7 +3091,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret < 0)
goto out2;
- ret = regulator_set_voltage_rdev(rdev, min_uV, max_uV, state);
+ /* for not coupled regulators this will just set the voltage */
+ ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
goto out2;
@@ -3360,7 +3378,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
{
int ret = 0;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(regulator->rdev, "%s: %d\n", __func__, __LINE__);
regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH v2] regulator: core: Enable voltage balancing
@ 2018-06-13 10:33 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-13 10:33 UTC (permalink / raw)
To: linux-arm-kernel
Call regulator_balance_voltage() instead of set_voltage_rdev()
in set_voltage_unlocked() and in enabling and disabling functions,
but only if the regulator is coupled.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
Changes in v2:
- fix compile errors
- make debug messages more informative
---
drivers/regulator/core.c | 38 ++++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2a7ffb7..266f4eb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1605,7 +1605,6 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
struct device *dev = rdev->dev.parent;
int ret;
- pr_err("%s: %d\n", __func__, __LINE__);
/* No supply to resovle? */
if (!rdev->supply_name)
return 0;
@@ -2246,7 +2245,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
{
int ret;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
lockdep_assert_held_once(&rdev->mutex);
/* check voltage and requested load before enabling */
@@ -2295,7 +2294,12 @@ int regulator_enable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ return -EPERM;
+ }
+
if (regulator->always_on)
return 0;
@@ -2307,12 +2311,15 @@ int regulator_enable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
+ /* balance only if there are regulators coupled */
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2411,18 +2418,20 @@ int regulator_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_disable);
@@ -2470,6 +2479,8 @@ int regulator_force_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
+ if (rdev->coupling_desc.n_coupled > 1)
+ regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -2898,7 +2909,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
const struct regulator_ops *ops = rdev->desc->ops;
int old_uV = _regulator_get_voltage(rdev);
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
min_uV += rdev->constraints->uV_offset;
@@ -3031,7 +3042,13 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
int old_min_uV, old_max_uV;
int current_uV;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
+ if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
+ rdev_err(rdev, "not all coupled regulators registered\n");
+ ret = -EPERM;
+ goto out;
+ }
+
/* If we're setting the same range as last time the change
* should be a noop (some cpufreq implementations use the same
* voltage for multiple frequencies, for example).
@@ -3074,7 +3091,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
if (ret < 0)
goto out2;
- ret = regulator_set_voltage_rdev(rdev, min_uV, max_uV, state);
+ /* for not coupled regulators this will just set the voltage */
+ ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
goto out2;
@@ -3360,7 +3378,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
{
int ret = 0;
- pr_err("%s: %d\n", __func__, __LINE__);
+ rdev_err(regulator->rdev, "%s: %d\n", __func__, __LINE__);
regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH v2] regulator: core: Enable voltage balancing
2018-06-13 10:33 ` Maciej Purski
@ 2018-06-15 11:29 ` Tony Lindgren
-1 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-06-15 11:29 UTC (permalink / raw)
To: Maciej Purski
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski
Hi,
* Maciej Purski <m.purski@samsung.com> [180613 10:39]:
> Call regulator_balance_voltage() instead of set_voltage_rdev()
> in set_voltage_unlocked() and in enabling and disabling functions,
> but only if the regulator is coupled.
>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>
> ---
> Changes in v2:
> - fix compile errors
> - make debug messages more informative
Thanks for updating it. This series still hangs after loading
modules on beagleboard-x15:
[ 26.679749] smps12: regulator_set_voltage: 3381
[ 26.684529] smps12: regulator_set_voltage_unlocked: 3045
[ 26.695616] smps12: _regulator_do_set_voltage: 2912
[ 26.701275] smps12: regulator_set_voltage: 3381
[ 26.706002] smps12: regulator_set_voltage_unlocked: 3045
[ 26.712349] smps12: _regulator_do_set_voltage: 2912
[ 26.719329] abb_mpu: regulator_set_voltage: 3381
[ 26.724105] abb_mpu: regulator_set_voltage_unlocked: 3045
So it seems to be the abb_mpu where it hangs?
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH v2] regulator: core: Enable voltage balancing
@ 2018-06-15 11:29 ` Tony Lindgren
0 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-06-15 11:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
* Maciej Purski <m.purski@samsung.com> [180613 10:39]:
> Call regulator_balance_voltage() instead of set_voltage_rdev()
> in set_voltage_unlocked() and in enabling and disabling functions,
> but only if the regulator is coupled.
>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>
> ---
> Changes in v2:
> - fix compile errors
> - make debug messages more informative
Thanks for updating it. This series still hangs after loading
modules on beagleboard-x15:
[ 26.679749] smps12: regulator_set_voltage: 3381
[ 26.684529] smps12: regulator_set_voltage_unlocked: 3045
[ 26.695616] smps12: _regulator_do_set_voltage: 2912
[ 26.701275] smps12: regulator_set_voltage: 3381
[ 26.706002] smps12: regulator_set_voltage_unlocked: 3045
[ 26.712349] smps12: _regulator_do_set_voltage: 2912
[ 26.719329] abb_mpu: regulator_set_voltage: 3381
[ 26.724105] abb_mpu: regulator_set_voltage_unlocked: 3045
So it seems to be the abb_mpu where it hangs?
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH v2] regulator: core: Enable voltage balancing
2018-06-15 11:29 ` Tony Lindgren
@ 2018-06-18 13:17 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-18 13:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski
On 06/15/2018 01:29 PM, Tony Lindgren wrote:
> Hi,
>
> * Maciej Purski <m.purski@samsung.com> [180613 10:39]:
>> Call regulator_balance_voltage() instead of set_voltage_rdev()
>> in set_voltage_unlocked() and in enabling and disabling functions,
>> but only if the regulator is coupled.
>>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>
>> ---
>> Changes in v2:
>> - fix compile errors
>> - make debug messages more informative
>
> Thanks for updating it. This series still hangs after loading
> modules on beagleboard-x15:
>
> [ 26.679749] smps12: regulator_set_voltage: 3381
> [ 26.684529] smps12: regulator_set_voltage_unlocked: 3045
> [ 26.695616] smps12: _regulator_do_set_voltage: 2912
> [ 26.701275] smps12: regulator_set_voltage: 3381
> [ 26.706002] smps12: regulator_set_voltage_unlocked: 3045
> [ 26.712349] smps12: _regulator_do_set_voltage: 2912
> [ 26.719329] abb_mpu: regulator_set_voltage: 3381
> [ 26.724105] abb_mpu: regulator_set_voltage_unlocked: 3045
>
> So it seems to be the abb_mpu where it hangs?
>
> Regards,
>
> Tony
>
Hi,
thanks for testing. Yes, it seems that it fails on abb_mpu. I don't know
yet, what is so special about that regulator.
We know at least, that it fails on voltage setting somewhere
between set_voltage_unlocked() and do_set_voltage()
and it does not look like any locking issue.
The most suspicious part in voltage balancing code is of course the
infinite loop. Soon I'll send a next patch on top of my latest compiling
path:
2ff49a6 regulator: core: Enable voltage balancing.
It should reveal, if it is indeed the loop.
As usual, I'd be grateful, if you gave it a try.
Best regards,
Maciej Purski
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH v2] regulator: core: Enable voltage balancing
@ 2018-06-18 13:17 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-18 13:17 UTC (permalink / raw)
To: linux-arm-kernel
On 06/15/2018 01:29 PM, Tony Lindgren wrote:
> Hi,
>
> * Maciej Purski <m.purski@samsung.com> [180613 10:39]:
>> Call regulator_balance_voltage() instead of set_voltage_rdev()
>> in set_voltage_unlocked() and in enabling and disabling functions,
>> but only if the regulator is coupled.
>>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>
>> ---
>> Changes in v2:
>> - fix compile errors
>> - make debug messages more informative
>
> Thanks for updating it. This series still hangs after loading
> modules on beagleboard-x15:
>
> [ 26.679749] smps12: regulator_set_voltage: 3381
> [ 26.684529] smps12: regulator_set_voltage_unlocked: 3045
> [ 26.695616] smps12: _regulator_do_set_voltage: 2912
> [ 26.701275] smps12: regulator_set_voltage: 3381
> [ 26.706002] smps12: regulator_set_voltage_unlocked: 3045
> [ 26.712349] smps12: _regulator_do_set_voltage: 2912
> [ 26.719329] abb_mpu: regulator_set_voltage: 3381
> [ 26.724105] abb_mpu: regulator_set_voltage_unlocked: 3045
>
> So it seems to be the abb_mpu where it hangs?
>
> Regards,
>
> Tony
>
Hi,
thanks for testing. Yes, it seems that it fails on abb_mpu. I don't know
yet, what is so special about that regulator.
We know at least, that it fails on voltage setting somewhere
between set_voltage_unlocked() and do_set_voltage()
and it does not look like any locking issue.
The most suspicious part in voltage balancing code is of course the
infinite loop. Soon I'll send a next patch on top of my latest compiling
path:
2ff49a6 regulator: core: Enable voltage balancing.
It should reveal, if it is indeed the loop.
As usual, I'd be grateful, if you gave it a try.
Best regards,
Maciej Purski
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-06-15 11:29 ` Tony Lindgren
@ 2018-06-18 14:08 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-18 14:08 UTC (permalink / raw)
To: Tony Lindgren
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski, Maciej Purski
If the regulator is not coupled, balance_voltage() should preserve
its desired max uV, instead of setting the exact value like in
coupled regulators case.
Remove debugs, which are not necessary for now.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 266f4eb..9894f4e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2245,7 +2245,6 @@ static int _regulator_enable(struct regulator_dev *rdev)
{
int ret;
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
lockdep_assert_held_once(&rdev->mutex);
/* check voltage and requested load before enabling */
@@ -2294,7 +2293,6 @@ int regulator_enable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
rdev_err(rdev, "not all coupled regulators registered\n");
return -EPERM;
@@ -2319,7 +2317,6 @@ int regulator_enable(struct regulator *regulator)
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2418,7 +2415,6 @@ int regulator_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
@@ -2431,7 +2427,6 @@ int regulator_disable(struct regulator *regulator)
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_disable);
@@ -3112,6 +3107,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
int supply_change_uV = 0;
int ret;
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
+
if (rdev->supply &&
regulator_ops_is_valid(rdev->supply->rdev,
REGULATOR_CHANGE_VOLTAGE) &&
@@ -3175,7 +3172,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int
+regulator_get_optimal_voltage(struct regulator_dev *rdev, int *max_uV)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3200,6 +3198,7 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
*/
if (n_coupled == 1) {
ret = desired_min_uV;
+ *max_uV = desired_max_uV;
goto out;
}
@@ -3274,6 +3273,7 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
goto out;
}
ret = possible_uV;
+ *max_uV = ret;
out:
return ret;
@@ -3303,6 +3303,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* if there isn't any possible change.
*/
while (1) {
+ int max_uV;
+
best_delta = 0;
best_uV = 0;
best_rdev = NULL;
@@ -3318,9 +3320,9 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, current_uV;;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
+ optimal_uV = regulator_get_optimal_voltage(c_rdevs[i], &max_uV);
if (optimal_uV < 0) {
ret = optimal_uV;
goto out;
@@ -3337,6 +3339,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
best_rdev = c_rdevs[i];
best_uV = optimal_uV;
}
+
+ rdev_err(rdev,
+ "optimal uV: %d current uV: %d, max uV: %d\n",
+ optimal_uV, current_uV, max_uV);
}
/* Nothing to change, return successfully */
@@ -3346,7 +3352,7 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
}
ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ max_uV, state);
if (ret < 0)
goto out;
@@ -3378,7 +3384,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
{
int ret = 0;
- rdev_err(regulator->rdev, "%s: %d\n", __func__, __LINE__);
+ dev_err(regulator->dev, "%s: %d\n", __func__, __LINE__);
regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-06-18 14:08 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-06-18 14:08 UTC (permalink / raw)
To: linux-arm-kernel
If the regulator is not coupled, balance_voltage() should preserve
its desired max uV, instead of setting the exact value like in
coupled regulators case.
Remove debugs, which are not necessary for now.
Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
drivers/regulator/core.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 266f4eb..9894f4e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2245,7 +2245,6 @@ static int _regulator_enable(struct regulator_dev *rdev)
{
int ret;
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
lockdep_assert_held_once(&rdev->mutex);
/* check voltage and requested load before enabling */
@@ -2294,7 +2293,6 @@ int regulator_enable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
rdev_err(rdev, "not all coupled regulators registered\n");
return -EPERM;
@@ -2319,7 +2317,6 @@ int regulator_enable(struct regulator *regulator)
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_enable);
@@ -2418,7 +2415,6 @@ int regulator_disable(struct regulator *regulator)
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
if (regulator->always_on)
return 0;
@@ -2431,7 +2427,6 @@ int regulator_disable(struct regulator *regulator)
if (ret == 0 && rdev->supply)
regulator_disable(rdev->supply);
- rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
return ret;
}
EXPORT_SYMBOL_GPL(regulator_disable);
@@ -3112,6 +3107,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
int supply_change_uV = 0;
int ret;
+ rdev_err(rdev, "%s: %d\n", __func__, __LINE__);
+
if (rdev->supply &&
regulator_ops_is_valid(rdev->supply->rdev,
REGULATOR_CHANGE_VOLTAGE) &&
@@ -3175,7 +3172,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int
+regulator_get_optimal_voltage(struct regulator_dev *rdev, int *max_uV)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3200,6 +3198,7 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
*/
if (n_coupled == 1) {
ret = desired_min_uV;
+ *max_uV = desired_max_uV;
goto out;
}
@@ -3274,6 +3273,7 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
goto out;
}
ret = possible_uV;
+ *max_uV = ret;
out:
return ret;
@@ -3303,6 +3303,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* if there isn't any possible change.
*/
while (1) {
+ int max_uV;
+
best_delta = 0;
best_uV = 0;
best_rdev = NULL;
@@ -3318,9 +3320,9 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, current_uV;;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
+ optimal_uV = regulator_get_optimal_voltage(c_rdevs[i], &max_uV);
if (optimal_uV < 0) {
ret = optimal_uV;
goto out;
@@ -3337,6 +3339,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
best_rdev = c_rdevs[i];
best_uV = optimal_uV;
}
+
+ rdev_err(rdev,
+ "optimal uV: %d current uV: %d, max uV: %d\n",
+ optimal_uV, current_uV, max_uV);
}
/* Nothing to change, return successfully */
@@ -3346,7 +3352,7 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
}
ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ max_uV, state);
if (ret < 0)
goto out;
@@ -3378,7 +3384,7 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
{
int ret = 0;
- rdev_err(regulator->rdev, "%s: %d\n", __func__, __LINE__);
+ dev_err(regulator->dev, "%s: %d\n", __func__, __LINE__);
regulator_lock_dependent(regulator->rdev);
ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
--
2.7.4
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-06-18 14:08 ` Maciej Purski
@ 2018-07-02 8:05 ` Tony Lindgren
-1 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-07-02 8:05 UTC (permalink / raw)
To: Maciej Purski
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski
* Maciej Purski <m.purski@samsung.com> [180618 14:11]:
> If the regulator is not coupled, balance_voltage() should preserve
> its desired max uV, instead of setting the exact value like in
> coupled regulators case.
>
> Remove debugs, which are not necessary for now.
Sorry for the delay in testing. I gave your series with this one
a quick boot test on beagleboard-x15 and now the output is
different. So instead of just hanging it seems to be stuck in
some eternal loop see below.
Regards,
Tony
8< -------
* Loading modules ...[ 14.490595] omap-mailbox 48840000.mailbox: omap mailbox rev 0x400
[ 14.498515] omap-mailbox 48842000.mailbox: omap mailbox rev 0x400
[ 14.535853] omap_wdt: OMAP Watchdog Timer Rev 0x01: initial timeout 60 sec
[ 14.937029] lib80211: common routines for IEEE802.11 drivers
[ 15.350565] cfg80211: Loading compiled-in X.509 certificates for regulatory database
[ 15.448875] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[ 15.460831] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2
[ 15.469916] cfg80211: failed to load regulatory.db
[ 16.092151] cpu cpu0: regulator_set_voltage: 3387
[ 16.097313] smps12: regulator_set_voltage_unlocked: 3040
[ 16.109880] smps12: optimal uV: 1154000 current uV: 970000, max uV: 1250000
[ 16.117085] smps12: regulator_set_voltage_rdev: 3110
[ 16.122696] smps12: _regulator_do_set_voltage: 2907
[ 16.130443] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.137824] smps12: regulator_set_voltage_rdev: 3110
[ 16.143334] smps12: _regulator_do_set_voltage: 2907
[ 16.149849] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.157174] smps12: regulator_set_voltage_rdev: 3110
[ 16.162683] smps12: _regulator_do_set_voltage: 2907
[ 16.169069] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.176391] smps12: regulator_set_voltage_rdev: 3110
[ 16.181932] smps12: _regulator_do_set_voltage: 2907
[ 16.188353] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.195614] smps12: regulator_set_voltage_rdev: 3110
[ 16.201199] smps12: _regulator_do_set_voltage: 2907
[ 16.207676] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.214931] smps12: regulator_set_voltage_rdev: 3110
[ 16.220614] smps12: _regulator_do_set_voltage: 2907
[ 16.227159] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.234379] smps12: regulator_set_voltage_rdev: 3110
[ 16.239961] smps12: _regulator_do_set_voltage: 2907
[ 16.246494] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.253746] smps12: regulator_set_voltage_rdev: 3110
[ 16.259890] smps12: _regulator_do_set_voltage: 2907
[ 16.267013] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.274114] smps12: regulator_set_voltage_rdev: 3110
[ 16.279565] smps12: _regulator_do_set_voltage: 2907
[ 16.288426] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.295562] smps12: regulator_set_voltage_rdev: 3110
[ 16.301580] smps12: _regulator_do_set_voltage: 2907
[ 16.308331] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.315655] smps12: regulator_set_voltage_rdev: 3110
[ 16.321328] smps12: _regulator_do_set_voltage: 2907
[ 16.327686] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.334945] smps12: regulator_set_voltage_rdev: 3110
[ 16.340662] smps12: _regulator_do_set_voltage: 2907
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-07-02 8:05 ` Tony Lindgren
0 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-07-02 8:05 UTC (permalink / raw)
To: linux-arm-kernel
* Maciej Purski <m.purski@samsung.com> [180618 14:11]:
> If the regulator is not coupled, balance_voltage() should preserve
> its desired max uV, instead of setting the exact value like in
> coupled regulators case.
>
> Remove debugs, which are not necessary for now.
Sorry for the delay in testing. I gave your series with this one
a quick boot test on beagleboard-x15 and now the output is
different. So instead of just hanging it seems to be stuck in
some eternal loop see below.
Regards,
Tony
8< -------
* Loading modules ...[ 14.490595] omap-mailbox 48840000.mailbox: omap mailbox rev 0x400
[ 14.498515] omap-mailbox 48842000.mailbox: omap mailbox rev 0x400
[ 14.535853] omap_wdt: OMAP Watchdog Timer Rev 0x01: initial timeout 60 sec
[ 14.937029] lib80211: common routines for IEEE802.11 drivers
[ 15.350565] cfg80211: Loading compiled-in X.509 certificates for regulatory database
[ 15.448875] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[ 15.460831] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2
[ 15.469916] cfg80211: failed to load regulatory.db
[ 16.092151] cpu cpu0: regulator_set_voltage: 3387
[ 16.097313] smps12: regulator_set_voltage_unlocked: 3040
[ 16.109880] smps12: optimal uV: 1154000 current uV: 970000, max uV: 1250000
[ 16.117085] smps12: regulator_set_voltage_rdev: 3110
[ 16.122696] smps12: _regulator_do_set_voltage: 2907
[ 16.130443] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.137824] smps12: regulator_set_voltage_rdev: 3110
[ 16.143334] smps12: _regulator_do_set_voltage: 2907
[ 16.149849] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.157174] smps12: regulator_set_voltage_rdev: 3110
[ 16.162683] smps12: _regulator_do_set_voltage: 2907
[ 16.169069] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.176391] smps12: regulator_set_voltage_rdev: 3110
[ 16.181932] smps12: _regulator_do_set_voltage: 2907
[ 16.188353] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.195614] smps12: regulator_set_voltage_rdev: 3110
[ 16.201199] smps12: _regulator_do_set_voltage: 2907
[ 16.207676] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.214931] smps12: regulator_set_voltage_rdev: 3110
[ 16.220614] smps12: _regulator_do_set_voltage: 2907
[ 16.227159] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.234379] smps12: regulator_set_voltage_rdev: 3110
[ 16.239961] smps12: _regulator_do_set_voltage: 2907
[ 16.246494] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.253746] smps12: regulator_set_voltage_rdev: 3110
[ 16.259890] smps12: _regulator_do_set_voltage: 2907
[ 16.267013] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.274114] smps12: regulator_set_voltage_rdev: 3110
[ 16.279565] smps12: _regulator_do_set_voltage: 2907
[ 16.288426] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.295562] smps12: regulator_set_voltage_rdev: 3110
[ 16.301580] smps12: _regulator_do_set_voltage: 2907
[ 16.308331] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.315655] smps12: regulator_set_voltage_rdev: 3110
[ 16.321328] smps12: _regulator_do_set_voltage: 2907
[ 16.327686] smps12: optimal uV: 1154000 current uV: 1160000, max uV: 1250000
[ 16.334945] smps12: regulator_set_voltage_rdev: 3110
[ 16.340662] smps12: _regulator_do_set_voltage: 2907
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-07-02 8:05 ` Tony Lindgren
(?)
(?)
@ 2018-09-28 20:09 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 20:09 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, linux-omap
On 7/2/18 11:05 AM, Tony Lindgren wrote:
> * Maciej Purski <m.purski@samsung.com> [180618 14:11]:
>> If the regulator is not coupled, balance_voltage() should preserve
>> its desired max uV, instead of setting the exact value like in
>> coupled regulators case.
>>
>> Remove debugs, which are not necessary for now.
>
> Sorry for the delay in testing. I gave your series with this one
> a quick boot test on beagleboard-x15 and now the output is
> different. So instead of just hanging it seems to be stuck in
> some eternal loop see below.
Hello guys,
I'm working on the CPUFreq driver for NVIDIA Tegra and it requires the coupled-regulators functionality, hence I'm interested in getting the coupled regulators series finalized ASAP and want to help with it.
It looks to me that the infinite-loop problem is caused by the voltage value that is getting rounded-up by the driver because it isn't aligned to the uV_step. IIUC, uV_step is relevant only for the "linear" regulators and hence we can't simply set the best_delta to uV_step to solve the problem. Other solution could be to bail out of the loop once regulator sets value equal to the "desired".
Tony, could you please give a try to the patch below?
Do the following:
1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
3) Apply this patch:
>From 7928aecb3af9d367dd3c085972349aaa16318c1b Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage()
---
drivers/regulator/core.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..4a386fe9f26a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3187,7 +3187,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3211,7 +3212,9 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* by consumers.
*/
if (n_coupled == 1) {
- ret = desired_min_uV;
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3288,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3303,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3320,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3337,26 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3366,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 20:09 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 20:09 UTC (permalink / raw)
To: Tony Lindgren, Maciej Purski, Mark Brown
Cc: linux-kernel, linux-arm-kernel, linux-omap, Carlos Hernandez,
Marek Szyprowski
On 7/2/18 11:05 AM, Tony Lindgren wrote:
> * Maciej Purski <m.purski@samsung.com> [180618 14:11]:
>> If the regulator is not coupled, balance_voltage() should preserve
>> its desired max uV, instead of setting the exact value like in
>> coupled regulators case.
>>
>> Remove debugs, which are not necessary for now.
>
> Sorry for the delay in testing. I gave your series with this one
> a quick boot test on beagleboard-x15 and now the output is
> different. So instead of just hanging it seems to be stuck in
> some eternal loop see below.
Hello guys,
I'm working on the CPUFreq driver for NVIDIA Tegra and it requires the coupled-regulators functionality, hence I'm interested in getting the coupled regulators series finalized ASAP and want to help with it.
It looks to me that the infinite-loop problem is caused by the voltage value that is getting rounded-up by the driver because it isn't aligned to the uV_step. IIUC, uV_step is relevant only for the "linear" regulators and hence we can't simply set the best_delta to uV_step to solve the problem. Other solution could be to bail out of the loop once regulator sets value equal to the "desired".
Tony, could you please give a try to the patch below?
Do the following:
1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
3) Apply this patch:
From 7928aecb3af9d367dd3c085972349aaa16318c1b Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage()
---
drivers/regulator/core.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..4a386fe9f26a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3187,7 +3187,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3211,7 +3212,9 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* by consumers.
*/
if (n_coupled == 1) {
- ret = desired_min_uV;
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3288,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3303,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3320,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3337,26 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3366,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 20:09 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 20:09 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, linux-omap
On 7/2/18 11:05 AM, Tony Lindgren wrote:
> * Maciej Purski <m.purski@samsung.com> [180618 14:11]:
>> If the regulator is not coupled, balance_voltage() should preserve
>> its desired max uV, instead of setting the exact value like in
>> coupled regulators case.
>>
>> Remove debugs, which are not necessary for now.
>
> Sorry for the delay in testing. I gave your series with this one
> a quick boot test on beagleboard-x15 and now the output is
> different. So instead of just hanging it seems to be stuck in
> some eternal loop see below.
Hello guys,
I'm working on the CPUFreq driver for NVIDIA Tegra and it requires the coupled-regulators functionality, hence I'm interested in getting the coupled regulators series finalized ASAP and want to help with it.
It looks to me that the infinite-loop problem is caused by the voltage value that is getting rounded-up by the driver because it isn't aligned to the uV_step. IIUC, uV_step is relevant only for the "linear" regulators and hence we can't simply set the best_delta to uV_step to solve the problem. Other solution could be to bail out of the loop once regulator sets value equal to the "desired".
Tony, could you please give a try to the patch below?
Do the following:
1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
3) Apply this patch:
From 7928aecb3af9d367dd3c085972349aaa16318c1b Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage()
---
drivers/regulator/core.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..4a386fe9f26a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3187,7 +3187,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3211,7 +3212,9 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* by consumers.
*/
if (n_coupled == 1) {
- ret = desired_min_uV;
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3288,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3303,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3320,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3337,26 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3366,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 20:09 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 20:09 UTC (permalink / raw)
To: linux-arm-kernel
On 7/2/18 11:05 AM, Tony Lindgren wrote:
> * Maciej Purski <m.purski@samsung.com> [180618 14:11]:
>> If the regulator is not coupled, balance_voltage() should preserve
>> its desired max uV, instead of setting the exact value like in
>> coupled regulators case.
>>
>> Remove debugs, which are not necessary for now.
>
> Sorry for the delay in testing. I gave your series with this one
> a quick boot test on beagleboard-x15 and now the output is
> different. So instead of just hanging it seems to be stuck in
> some eternal loop see below.
Hello guys,
I'm working on the CPUFreq driver for NVIDIA Tegra and it requires the coupled-regulators functionality, hence I'm interested in getting the coupled regulators series finalized ASAP and want to help with it.
It looks to me that the infinite-loop problem is caused by the voltage value that is getting rounded-up by the driver because it isn't aligned to the uV_step. IIUC, uV_step is relevant only for the "linear" regulators and hence we can't simply set the best_delta to uV_step to solve the problem. Other solution could be to bail out of the loop once regulator sets value equal to the "desired".
Tony, could you please give a try to the patch below?
Do the following:
1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
3) Apply this patch:
>From 7928aecb3af9d367dd3c085972349aaa16318c1b Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage()
---
drivers/regulator/core.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..4a386fe9f26a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3187,7 +3187,8 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3211,7 +3212,9 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* by consumers.
*/
if (n_coupled == 1) {
- ret = desired_min_uV;
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3288,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3303,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3320,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3337,26 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3366,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 20:09 ` Dmitry Osipenko
@ 2018-09-28 20:22 ` Tony Lindgren
-1 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-09-28 20:22 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
* Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
> Tony, could you please give a try to the patch below?
>
> Do the following:
>
> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
> 3) Apply this patch:
Seems to be getting closer, system boots up and starts
init, but then I start getting tons of this on beagle-x15:
[ 17.089059] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.096342] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.103275] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.118139] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.125078] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.132105] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.148187] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.155265] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.162355] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.171111] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.178156] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.185240] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.197636] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.204597] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.211621] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.227466] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.234428] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.241467] cpufreq: __target_index: Failed to change cpu frequency: -22
...
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 20:22 ` Tony Lindgren
0 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-09-28 20:22 UTC (permalink / raw)
To: linux-arm-kernel
* Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
> Tony, could you please give a try to the patch below?
>
> Do the following:
>
> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
> 3) Apply this patch:
Seems to be getting closer, system boots up and starts
init, but then I start getting tons of this on beagle-x15:
[ 17.089059] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.096342] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.103275] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.118139] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.125078] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.132105] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.148187] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.155265] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.162355] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.171111] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.178156] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.185240] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.197636] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.204597] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.211621] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 17.227466] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 17.234428] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 17.241467] cpufreq: __target_index: Failed to change cpu frequency: -22
...
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 20:22 ` Tony Lindgren
@ 2018-09-28 20:36 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 20:36 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/28/18 11:22 PM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>> Tony, could you please give a try to the patch below?
>>
>> Do the following:
>>
>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>> 3) Apply this patch:
>
> Seems to be getting closer, system boots up and starts
> init, but then I start getting tons of this on beagle-x15:
>
> [ 17.089059] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.096342] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.103275] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.118139] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.125078] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.132105] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.148187] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.155265] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.162355] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.171111] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.178156] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.185240] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.197636] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.204597] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.211621] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.227466] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.234428] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.241467] cpufreq: __target_index: Failed to change cpu frequency: -22
Thank you very much for testing and for the quick reply! That's an interesting result. I'll take a closer look at the patches and come back once there will be another patch to try.
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 20:36 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 20:36 UTC (permalink / raw)
To: linux-arm-kernel
On 9/28/18 11:22 PM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>> Tony, could you please give a try to the patch below?
>>
>> Do the following:
>>
>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>> 3) Apply this patch:
>
> Seems to be getting closer, system boots up and starts
> init, but then I start getting tons of this on beagle-x15:
>
> [ 17.089059] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.096342] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.103275] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.118139] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.125078] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.132105] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.148187] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.155265] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.162355] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.171111] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.178156] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.185240] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.197636] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.204597] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.211621] cpufreq: __target_index: Failed to change cpu frequency: -22
> [ 17.227466] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
> [ 17.234428] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
> [ 17.241467] cpufreq: __target_index: Failed to change cpu frequency: -22
Thank you very much for testing and for the quick reply! That's an interesting result. I'll take a closer look at the patches and come back once there will be another patch to try.
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 20:22 ` Tony Lindgren
(?)
@ 2018-09-28 22:26 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 22:26 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/28/18 11:22 PM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>> Tony, could you please give a try to the patch below?
>>
>> Do the following:
>>
>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>> 3) Apply this patch:
>
> Seems to be getting closer, system boots up and starts
> init, but then I start getting tons of this on beagle-x15:
Tony, could you please try this one? Fixed couple more bugs, should be good now.
>From 9c7382b1a28692576cbe00f57fdea0d25b27cc4c Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v2
---
drivers/regulator/core.c | 83 ++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 33 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..2f3b1cf19bd5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,7 +105,7 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
@@ -2330,7 +2330,7 @@ int regulator_enable(struct regulator *regulator)
ret = _regulator_enable(rdev);
/* balance only if there are regulators coupled */
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2440,7 +2440,7 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2494,7 +2494,7 @@ int regulator_force_disable(struct regulator *regulator)
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3099,12 +3099,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
- ret = regulator_balance_voltage(rdev, state);
+ ret = regulator_balance_voltage(regulator, state);
if (ret < 0)
goto out2;
@@ -3187,7 +3183,10 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator *regulator,
+ struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3198,20 +3197,29 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
int i, ret;
/* If consumers don't provide any demands, set voltage to min_uV */
- desired_min_uV = rdev->constraints->min_uV;
- desired_max_uV = rdev->constraints->max_uV;
- ret = regulator_check_consumers(rdev,
- &desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
- if (ret < 0)
- goto out;
+ if (regulator->rdev == rdev) {
+ desired_min_uV = regulator->voltage[state].min_uV;
+ desired_max_uV = regulator->voltage[state].max_uV;
+
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV,
+ state);
+ if (ret < 0)
+ goto out;
+ } else {
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ }
/*
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,20 +3293,24 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state)
{
+ struct regulator_dev *rdev = regulator->rdev;
struct regulator_dev **c_rdevs;
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3326,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3343,28 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(regulator,
+ c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3374,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 22:26 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 22:26 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/28/18 11:22 PM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>> Tony, could you please give a try to the patch below?
>>
>> Do the following:
>>
>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>> 3) Apply this patch:
>
> Seems to be getting closer, system boots up and starts
> init, but then I start getting tons of this on beagle-x15:
Tony, could you please try this one? Fixed couple more bugs, should be good now.
From 9c7382b1a28692576cbe00f57fdea0d25b27cc4c Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v2
---
drivers/regulator/core.c | 83 ++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 33 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..2f3b1cf19bd5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,7 +105,7 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
@@ -2330,7 +2330,7 @@ int regulator_enable(struct regulator *regulator)
ret = _regulator_enable(rdev);
/* balance only if there are regulators coupled */
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2440,7 +2440,7 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2494,7 +2494,7 @@ int regulator_force_disable(struct regulator *regulator)
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3099,12 +3099,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
- ret = regulator_balance_voltage(rdev, state);
+ ret = regulator_balance_voltage(regulator, state);
if (ret < 0)
goto out2;
@@ -3187,7 +3183,10 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator *regulator,
+ struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3198,20 +3197,29 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
int i, ret;
/* If consumers don't provide any demands, set voltage to min_uV */
- desired_min_uV = rdev->constraints->min_uV;
- desired_max_uV = rdev->constraints->max_uV;
- ret = regulator_check_consumers(rdev,
- &desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
- if (ret < 0)
- goto out;
+ if (regulator->rdev == rdev) {
+ desired_min_uV = regulator->voltage[state].min_uV;
+ desired_max_uV = regulator->voltage[state].max_uV;
+
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV,
+ state);
+ if (ret < 0)
+ goto out;
+ } else {
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ }
/*
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,20 +3293,24 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state)
{
+ struct regulator_dev *rdev = regulator->rdev;
struct regulator_dev **c_rdevs;
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3326,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3343,28 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(regulator,
+ c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3374,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 22:26 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 22:26 UTC (permalink / raw)
To: linux-arm-kernel
On 9/28/18 11:22 PM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>> Tony, could you please give a try to the patch below?
>>
>> Do the following:
>>
>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>> 3) Apply this patch:
>
> Seems to be getting closer, system boots up and starts
> init, but then I start getting tons of this on beagle-x15:
Tony, could you please try this one? Fixed couple more bugs, should be good now.
>From 9c7382b1a28692576cbe00f57fdea0d25b27cc4c Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v2
---
drivers/regulator/core.c | 83 ++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 33 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..2f3b1cf19bd5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,7 +105,7 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
@@ -2330,7 +2330,7 @@ int regulator_enable(struct regulator *regulator)
ret = _regulator_enable(rdev);
/* balance only if there are regulators coupled */
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2440,7 +2440,7 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2494,7 +2494,7 @@ int regulator_force_disable(struct regulator *regulator)
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3099,12 +3099,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
- ret = regulator_balance_voltage(rdev, state);
+ ret = regulator_balance_voltage(regulator, state);
if (ret < 0)
goto out2;
@@ -3187,7 +3183,10 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator *regulator,
+ struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3198,20 +3197,29 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
int i, ret;
/* If consumers don't provide any demands, set voltage to min_uV */
- desired_min_uV = rdev->constraints->min_uV;
- desired_max_uV = rdev->constraints->max_uV;
- ret = regulator_check_consumers(rdev,
- &desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
- if (ret < 0)
- goto out;
+ if (regulator->rdev == rdev) {
+ desired_min_uV = regulator->voltage[state].min_uV;
+ desired_max_uV = regulator->voltage[state].max_uV;
+
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV,
+ state);
+ if (ret < 0)
+ goto out;
+ } else {
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ }
/*
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,20 +3293,24 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state)
{
+ struct regulator_dev *rdev = regulator->rdev;
struct regulator_dev **c_rdevs;
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3326,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3343,28 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(regulator,
+ c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
current_uV = _regulator_get_voltage(c_rdevs[i]);
if (current_uV < 0) {
- ret = optimal_uV;
+ ret = current_uV;
goto out;
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3374,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 22:26 ` Dmitry Osipenko
@ 2018-09-28 22:41 ` Tony Lindgren
-1 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-09-28 22:41 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
* Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
> On 9/28/18 11:22 PM, Tony Lindgren wrote:
> > * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
> >> Tony, could you please give a try to the patch below?
> >>
> >> Do the following:
> >>
> >> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
> >> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
> >> 3) Apply this patch:
> >
> > Seems to be getting closer, system boots up and starts
> > init, but then I start getting tons of this on beagle-x15:
>
> Tony, could you please try this one? Fixed couple more bugs, should be good now.
I'm still getting these errors after init:
[ 29.008665] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.015987] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.022967] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.031993] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.038932] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.045962] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.055588] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.062639] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.069569] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.086215] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.093366] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.100295] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.110370] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.118402] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.125450] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.127884] palmas-usb 48070000.i2c:tps659038@58:tps659038_usb: GPIO lookup for consumer id
[ 29.136112] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.140658] palmas-usb 48070000.i2c:tps659038@58:tps659038_usb: using device tree for GPIO lookup
[ 29.147728] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
...
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 22:41 ` Tony Lindgren
0 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-09-28 22:41 UTC (permalink / raw)
To: linux-arm-kernel
* Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
> On 9/28/18 11:22 PM, Tony Lindgren wrote:
> > * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
> >> Tony, could you please give a try to the patch below?
> >>
> >> Do the following:
> >>
> >> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
> >> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
> >> 3) Apply this patch:
> >
> > Seems to be getting closer, system boots up and starts
> > init, but then I start getting tons of this on beagle-x15:
>
> Tony, could you please try this one? Fixed couple more bugs, should be good now.
I'm still getting these errors after init:
[ 29.008665] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.015987] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.022967] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.031993] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.038932] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.045962] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.055588] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.062639] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.069569] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.086215] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.093366] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.100295] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.110370] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.118402] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
[ 29.125450] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 29.127884] palmas-usb 48070000.i2c:tps659038 at 58:tps659038_usb: GPIO lookup for consumer id
[ 29.136112] cpu cpu0: vbb failed for 1210000uV[min 950000uV max 1500000uV]
[ 29.140658] palmas-usb 48070000.i2c:tps659038 at 58:tps659038_usb: using device tree for GPIO lookup
[ 29.147728] cpu cpu0: vbb failed for 1060000uV[min 850000uV max 1500000uV]
...
Regards,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 22:41 ` Tony Lindgren
(?)
@ 2018-09-28 23:17 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 23:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/29/18 1:41 AM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
>> On 9/28/18 11:22 PM, Tony Lindgren wrote:
>>> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>>>> Tony, could you please give a try to the patch below?
>>>>
>>>> Do the following:
>>>>
>>>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>>>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>>>> 3) Apply this patch:
>>>
>>> Seems to be getting closer, system boots up and starts
>>> init, but then I start getting tons of this on beagle-x15:
>>
>> Tony, could you please try this one? Fixed couple more bugs, should be good now.
>
> I'm still getting these errors after init:
Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
Please try this patch:
>From 2f10c29547778499f614b363a7756a40099bfa5a Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v3
---
drivers/regulator/core.c | 91 ++++++++++++++++++++++++----------------
1 file changed, 55 insertions(+), 36 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..d0edb66b37a2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,7 +105,7 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
@@ -2330,7 +2330,7 @@ int regulator_enable(struct regulator *regulator)
ret = _regulator_enable(rdev);
/* balance only if there are regulators coupled */
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2440,7 +2440,7 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2494,7 +2494,7 @@ int regulator_force_disable(struct regulator *regulator)
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3099,12 +3099,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
- ret = regulator_balance_voltage(rdev, state);
+ ret = regulator_balance_voltage(regulator, state);
if (ret < 0)
goto out2;
@@ -3187,7 +3183,10 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator *regulator,
+ struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3198,20 +3197,29 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
int i, ret;
/* If consumers don't provide any demands, set voltage to min_uV */
- desired_min_uV = rdev->constraints->min_uV;
- desired_max_uV = rdev->constraints->max_uV;
- ret = regulator_check_consumers(rdev,
- &desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
- if (ret < 0)
- goto out;
+ if (regulator->rdev == rdev) {
+ desired_min_uV = regulator->voltage[state].min_uV;
+ desired_max_uV = regulator->voltage[state].max_uV;
+
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV,
+ state);
+ if (ret < 0)
+ goto out;
+ } else {
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ }
/*
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,20 +3293,24 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state)
{
+ struct regulator_dev *rdev = regulator->rdev;
struct regulator_dev **c_rdevs;
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3326,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3343,30 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV = 0;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(regulator,
+ c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
- current_uV = _regulator_get_voltage(c_rdevs[i]);
- if (current_uV < 0) {
- ret = optimal_uV;
- goto out;
+ if (n_coupled > 1) {
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = current_uV;
+ goto out;
+ }
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3376,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 23:17 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 23:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/29/18 1:41 AM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
>> On 9/28/18 11:22 PM, Tony Lindgren wrote:
>>> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>>>> Tony, could you please give a try to the patch below?
>>>>
>>>> Do the following:
>>>>
>>>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>>>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>>>> 3) Apply this patch:
>>>
>>> Seems to be getting closer, system boots up and starts
>>> init, but then I start getting tons of this on beagle-x15:
>>
>> Tony, could you please try this one? Fixed couple more bugs, should be good now.
>
> I'm still getting these errors after init:
Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
Please try this patch:
From 2f10c29547778499f614b363a7756a40099bfa5a Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v3
---
drivers/regulator/core.c | 91 ++++++++++++++++++++++++----------------
1 file changed, 55 insertions(+), 36 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..d0edb66b37a2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,7 +105,7 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
@@ -2330,7 +2330,7 @@ int regulator_enable(struct regulator *regulator)
ret = _regulator_enable(rdev);
/* balance only if there are regulators coupled */
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2440,7 +2440,7 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2494,7 +2494,7 @@ int regulator_force_disable(struct regulator *regulator)
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3099,12 +3099,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
- ret = regulator_balance_voltage(rdev, state);
+ ret = regulator_balance_voltage(regulator, state);
if (ret < 0)
goto out2;
@@ -3187,7 +3183,10 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator *regulator,
+ struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3198,20 +3197,29 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
int i, ret;
/* If consumers don't provide any demands, set voltage to min_uV */
- desired_min_uV = rdev->constraints->min_uV;
- desired_max_uV = rdev->constraints->max_uV;
- ret = regulator_check_consumers(rdev,
- &desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
- if (ret < 0)
- goto out;
+ if (regulator->rdev == rdev) {
+ desired_min_uV = regulator->voltage[state].min_uV;
+ desired_max_uV = regulator->voltage[state].max_uV;
+
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV,
+ state);
+ if (ret < 0)
+ goto out;
+ } else {
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ }
/*
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,20 +3293,24 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state)
{
+ struct regulator_dev *rdev = regulator->rdev;
struct regulator_dev **c_rdevs;
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3326,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3343,30 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV = 0;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(regulator,
+ c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
- current_uV = _regulator_get_voltage(c_rdevs[i]);
- if (current_uV < 0) {
- ret = optimal_uV;
- goto out;
+ if (n_coupled > 1) {
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = current_uV;
+ goto out;
+ }
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3376,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 23:17 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 23:17 UTC (permalink / raw)
To: linux-arm-kernel
On 9/29/18 1:41 AM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
>> On 9/28/18 11:22 PM, Tony Lindgren wrote:
>>> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>>>> Tony, could you please give a try to the patch below?
>>>>
>>>> Do the following:
>>>>
>>>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>>>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>>>> 3) Apply this patch:
>>>
>>> Seems to be getting closer, system boots up and starts
>>> init, but then I start getting tons of this on beagle-x15:
>>
>> Tony, could you please try this one? Fixed couple more bugs, should be good now.
>
> I'm still getting these errors after init:
Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
Please try this patch:
>From 2f10c29547778499f614b363a7756a40099bfa5a Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v3
---
drivers/regulator/core.c | 91 ++++++++++++++++++++++++----------------
1 file changed, 55 insertions(+), 36 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..d0edb66b37a2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,7 +105,7 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
unsigned long event, void *data);
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV);
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state);
static int regulator_set_voltage_rdev(struct regulator_dev *rdev,
int min_uV, int max_uV,
@@ -2330,7 +2330,7 @@ int regulator_enable(struct regulator *regulator)
ret = _regulator_enable(rdev);
/* balance only if there are regulators coupled */
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret != 0 && rdev->supply)
@@ -2440,7 +2440,7 @@ int regulator_disable(struct regulator *regulator)
regulator_lock_dependent(rdev);
ret = _regulator_disable(rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (ret == 0 && rdev->supply)
@@ -2494,7 +2494,7 @@ int regulator_force_disable(struct regulator *regulator)
regulator->uA_load = 0;
ret = _regulator_force_disable(regulator->rdev);
if (rdev->coupling_desc.n_coupled > 1)
- regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+ regulator_balance_voltage(regulator, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);
if (rdev->supply)
@@ -3099,12 +3099,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
- ret = regulator_balance_voltage(rdev, state);
+ ret = regulator_balance_voltage(regulator, state);
if (ret < 0)
goto out2;
@@ -3187,7 +3183,10 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator *regulator,
+ struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3198,20 +3197,29 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
int i, ret;
/* If consumers don't provide any demands, set voltage to min_uV */
- desired_min_uV = rdev->constraints->min_uV;
- desired_max_uV = rdev->constraints->max_uV;
- ret = regulator_check_consumers(rdev,
- &desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
- if (ret < 0)
- goto out;
+ if (regulator->rdev == rdev) {
+ desired_min_uV = regulator->voltage[state].min_uV;
+ desired_max_uV = regulator->voltage[state].max_uV;
+
+ ret = regulator_check_consumers(rdev,
+ &desired_min_uV,
+ &desired_max_uV,
+ state);
+ if (ret < 0)
+ goto out;
+ } else {
+ desired_min_uV = rdev->constraints->min_uV;
+ desired_max_uV = rdev->constraints->max_uV;
+ }
/*
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,20 +3293,24 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
-static int regulator_balance_voltage(struct regulator_dev *rdev,
+static int regulator_balance_voltage(struct regulator *regulator,
suspend_state_t state)
{
+ struct regulator_dev *rdev = regulator->rdev;
struct regulator_dev **c_rdevs;
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3326,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3343,30 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV = 0;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(regulator,
+ c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
- current_uV = _regulator_get_voltage(c_rdevs[i]);
- if (current_uV < 0) {
- ret = optimal_uV;
- goto out;
+ if (n_coupled > 1) {
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = current_uV;
+ goto out;
+ }
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3376,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 23:17 ` Dmitry Osipenko
(?)
@ 2018-09-28 23:51 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 23:51 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>> * Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
>>> On 9/28/18 11:22 PM, Tony Lindgren wrote:
>>>> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>>>>> Tony, could you please give a try to the patch below?
>>>>>
>>>>> Do the following:
>>>>>
>>>>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>>>>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>>>>> 3) Apply this patch:
>>>>
>>>> Seems to be getting closer, system boots up and starts
>>>> init, but then I start getting tons of this on beagle-x15:
>>>
>>> Tony, could you please try this one? Fixed couple more bugs, should be good now.
>>
>> I'm still getting these errors after init:
>
> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>
> Please try this patch:
I've revised the patch and here is the current final version.
>From 0332e230568d5bba470f917f0b36dd9ef9e6e511 Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v4
---
drivers/regulator/core.c | 58 +++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..17dce370f236 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3099,10 +3099,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
@@ -3187,7 +3183,9 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3202,7 +3200,8 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
desired_max_uV = rdev->constraints->max_uV;
ret = regulator_check_consumers(rdev,
&desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
+ &desired_max_uV,
+ state);
if (ret < 0)
goto out;
@@ -3210,8 +3209,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3286,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3301,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3318,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3335,29 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV = 0;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
- current_uV = _regulator_get_voltage(c_rdevs[i]);
- if (current_uV < 0) {
- ret = optimal_uV;
- goto out;
+ if (n_coupled > 1) {
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = current_uV;
+ goto out;
+ }
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3367,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 23:51 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 23:51 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>> * Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
>>> On 9/28/18 11:22 PM, Tony Lindgren wrote:
>>>> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>>>>> Tony, could you please give a try to the patch below?
>>>>>
>>>>> Do the following:
>>>>>
>>>>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>>>>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>>>>> 3) Apply this patch:
>>>>
>>>> Seems to be getting closer, system boots up and starts
>>>> init, but then I start getting tons of this on beagle-x15:
>>>
>>> Tony, could you please try this one? Fixed couple more bugs, should be good now.
>>
>> I'm still getting these errors after init:
>
> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>
> Please try this patch:
I've revised the patch and here is the current final version.
From 0332e230568d5bba470f917f0b36dd9ef9e6e511 Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v4
---
drivers/regulator/core.c | 58 +++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..17dce370f236 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3099,10 +3099,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
@@ -3187,7 +3183,9 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3202,7 +3200,8 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
desired_max_uV = rdev->constraints->max_uV;
ret = regulator_check_consumers(rdev,
&desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
+ &desired_max_uV,
+ state);
if (ret < 0)
goto out;
@@ -3210,8 +3209,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3286,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3301,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3318,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3335,29 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV = 0;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
- current_uV = _regulator_get_voltage(c_rdevs[i]);
- if (current_uV < 0) {
- ret = optimal_uV;
- goto out;
+ if (n_coupled > 1) {
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = current_uV;
+ goto out;
+ }
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3367,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-28 23:51 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-28 23:51 UTC (permalink / raw)
To: linux-arm-kernel
On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>> * Dmitry Osipenko <digetx@gmail.com> [180928 22:31]:
>>> On 9/28/18 11:22 PM, Tony Lindgren wrote:
>>>> * Dmitry Osipenko <digetx@gmail.com> [180928 20:13]:
>>>>> Tony, could you please give a try to the patch below?
>>>>>
>>>>> Do the following:
>>>>>
>>>>> 1) git cherry-pick 696861761a58d8c93605b5663824929fb6540f16
>>>>> 2) git cherry-pick 456e7cdf3b1a14e2606b8b687385ab2e3f23a49a
>>>>> 3) Apply this patch:
>>>>
>>>> Seems to be getting closer, system boots up and starts
>>>> init, but then I start getting tons of this on beagle-x15:
>>>
>>> Tony, could you please try this one? Fixed couple more bugs, should be good now.
>>
>> I'm still getting these errors after init:
>
> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>
> Please try this patch:
I've revised the patch and here is the current final version.
>From 0332e230568d5bba470f917f0b36dd9ef9e6e511 Mon Sep 17 00:00:00 2001
From: Dmitry Osipenko <digetx@gmail.com>
Date: Fri, 28 Sep 2018 21:49:20 +0300
Subject: [PATCH] Fixup regulator_balance_voltage() v4
---
drivers/regulator/core.c | 58 +++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 282511508698..17dce370f236 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3099,10 +3099,6 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
voltage->min_uV = min_uV;
voltage->max_uV = max_uV;
- ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
- if (ret < 0)
- goto out2;
-
/* for not coupled regulators this will just set the voltage */
ret = regulator_balance_voltage(rdev, state);
if (ret < 0)
@@ -3187,7 +3183,9 @@ static int regulator_set_voltage_rdev(struct regulator_dev *rdev, int min_uV,
return ret;
}
-static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
+static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
+ int *min_uV, int *max_uV,
+ suspend_state_t state)
{
struct coupling_desc *c_desc = &rdev->coupling_desc;
struct regulator_dev **c_rdevs = c_desc->coupled_rdevs;
@@ -3202,7 +3200,8 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
desired_max_uV = rdev->constraints->max_uV;
ret = regulator_check_consumers(rdev,
&desired_min_uV,
- &desired_max_uV, PM_SUSPEND_ON);
+ &desired_max_uV,
+ state);
if (ret < 0)
goto out;
@@ -3210,8 +3209,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
* If there are no coupled regulators, simply set the voltage demanded
* by consumers.
*/
- if (n_coupled == 1) {
- ret = desired_min_uV;
+ if (n_coupled == 1 || state != PM_SUSPEND_ON) {
+ *min_uV = desired_min_uV;
+ *max_uV = desired_max_uV;
+ ret = 1;
goto out;
}
@@ -3285,8 +3286,10 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev)
ret = -EINVAL;
goto out;
}
- ret = possible_uV;
+ ret = (possible_uV == desired_min_uV);
+ *min_uV = possible_uV;
+ *max_uV = desired_max_uV;
out:
return ret;
}
@@ -3298,7 +3301,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
struct regulator_dev *best_rdev;
struct coupling_desc *c_desc = &rdev->coupling_desc;
int n_coupled;
- int i, best_delta, best_uV, ret = 1;
+ int i, best_delta, best_min_uV, best_max_uV, ret = 1;
+ bool last = false;
c_rdevs = c_desc->coupled_rdevs;
n_coupled = c_desc->n_coupled;
@@ -3314,9 +3318,10 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* Find the best possible voltage change on each loop. Leave the loop
* if there isn't any possible change.
*/
- while (1) {
+ while (!last) {
best_delta = 0;
- best_uV = 0;
+ best_min_uV = 0;
+ best_max_uV = 0;
best_rdev = NULL;
/*
@@ -3330,24 +3335,29 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
* max_spread constraint in order to balance
* the coupled voltages.
*/
- int optimal_uV, current_uV;
+ int optimal_uV, optimal_max_uV, current_uV = 0;
- optimal_uV = regulator_get_optimal_voltage(c_rdevs[i]);
- if (optimal_uV < 0) {
- ret = optimal_uV;
+ ret = regulator_get_optimal_voltage(c_rdevs[i],
+ &optimal_uV,
+ &optimal_max_uV,
+ state);
+ if (ret < 0)
goto out;
- }
- current_uV = _regulator_get_voltage(c_rdevs[i]);
- if (current_uV < 0) {
- ret = optimal_uV;
- goto out;
+ if (n_coupled > 1) {
+ current_uV = _regulator_get_voltage(c_rdevs[i]);
+ if (current_uV < 0) {
+ ret = current_uV;
+ goto out;
+ }
}
if (abs(best_delta) < abs(optimal_uV - current_uV)) {
best_delta = optimal_uV - current_uV;
best_rdev = c_rdevs[i];
- best_uV = optimal_uV;
+ best_min_uV = optimal_uV;
+ best_max_uV = optimal_max_uV;
+ last = (best_rdev == rdev && ret > 0);
}
}
@@ -3357,8 +3367,8 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
goto out;
}
- ret = regulator_set_voltage_rdev(best_rdev, best_uV,
- best_uV, state);
+ ret = regulator_set_voltage_rdev(best_rdev, best_min_uV,
+ best_max_uV, state);
if (ret < 0)
goto out;
--
2.19.0
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-28 23:51 ` Dmitry Osipenko
@ 2018-09-29 0:27 ` Tony Lindgren
-1 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-09-29 0:27 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
* Dmitry Osipenko <digetx@gmail.com> [180928 23:55]:
> On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
> > On 9/29/18 1:41 AM, Tony Lindgren wrote:
> >> I'm still getting these errors after init:
> >
> > Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
OK that's starting to make some sense now thanks.
> > Please try this patch:
>
> I've revised the patch and here is the current final version.
Hey cool this one works now :) I suggest you rework the whole series
with these fixes. I recall the series had a problem with git bisect
too between the patches. It will make life easier for other people
who may need to git bisect these patches.
Thanks,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-29 0:27 ` Tony Lindgren
0 siblings, 0 replies; 81+ messages in thread
From: Tony Lindgren @ 2018-09-29 0:27 UTC (permalink / raw)
To: linux-arm-kernel
* Dmitry Osipenko <digetx@gmail.com> [180928 23:55]:
> On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
> > On 9/29/18 1:41 AM, Tony Lindgren wrote:
> >> I'm still getting these errors after init:
> >
> > Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
OK that's starting to make some sense now thanks.
> > Please try this patch:
>
> I've revised the patch and here is the current final version.
Hey cool this one works now :) I suggest you rework the whole series
with these fixes. I recall the series had a problem with git bisect
too between the patches. It will make life easier for other people
who may need to git bisect these patches.
Thanks,
Tony
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-29 0:27 ` Tony Lindgren
@ 2018-09-29 0:44 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-29 0:44 UTC (permalink / raw)
To: Tony Lindgren
Cc: Maciej Purski, Mark Brown, linux-kernel, linux-arm-kernel,
linux-omap, Carlos Hernandez, Marek Szyprowski
On 9/29/18 3:27 AM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 23:55]:
>> On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
>>> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>>>> I'm still getting these errors after init:
>>>
>>> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>
> OK that's starting to make some sense now thanks.
>
>>> Please try this patch:
>>
>> I've revised the patch and here is the current final version.
>
> Hey cool this one works now :) I suggest you rework the whole series
> with these fixes. I recall the series had a problem with git bisect
> too between the patches. It will make life easier for other people
> who may need to git bisect these patches.
Awesome! There are few other things in this patchset that also need fixing. I've asked Maciej if he's going to continue working on the patches, waiting for the answer. I can pick up the patches and try to finish the work if necessary.
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-09-29 0:44 ` Dmitry Osipenko
0 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-09-29 0:44 UTC (permalink / raw)
To: linux-arm-kernel
On 9/29/18 3:27 AM, Tony Lindgren wrote:
> * Dmitry Osipenko <digetx@gmail.com> [180928 23:55]:
>> On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
>>> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>>>> I'm still getting these errors after init:
>>>
>>> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>
> OK that's starting to make some sense now thanks.
>
>>> Please try this patch:
>>
>> I've revised the patch and here is the current final version.
>
> Hey cool this one works now :) I suggest you rework the whole series
> with these fixes. I recall the series had a problem with git bisect
> too between the patches. It will make life easier for other people
> who may need to git bisect these patches.
Awesome! There are few other things in this patchset that also need fixing. I've asked Maciej if he's going to continue working on the patches, waiting for the answer. I can pick up the patches and try to finish the work if necessary.
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-09-29 0:44 ` Dmitry Osipenko
@ 2018-10-01 7:25 ` Maciej Purski
-1 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-10-01 7:25 UTC (permalink / raw)
To: Dmitry Osipenko, Tony Lindgren
Cc: Mark Brown, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski
Hi all,
unfortunately, I don't work at kernel anymore. That would be great, if you could take over those patches and finish the
work.
Best regards,
Maciej
On 09/29/2018 02:44 AM, Dmitry Osipenko wrote:
> On 9/29/18 3:27 AM, Tony Lindgren wrote:
>> * Dmitry Osipenko <digetx@gmail.com> [180928 23:55]:
>>> On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
>>>> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>>>>> I'm still getting these errors after init:
>>>>
>>>> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>>
>> OK that's starting to make some sense now thanks.
>>
>>>> Please try this patch:
>>>
>>> I've revised the patch and here is the current final version.
>>
>> Hey cool this one works now :) I suggest you rework the whole series
>> with these fixes. I recall the series had a problem with git bisect
>> too between the patches. It will make life easier for other people
>> who may need to git bisect these patches.
>
> Awesome! There are few other things in this patchset that also need fixing. I've asked Maciej if he's going to continue working on the patches, waiting for the answer. I can pick up the patches and try to finish the work if necessary.
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
@ 2018-10-01 7:25 ` Maciej Purski
0 siblings, 0 replies; 81+ messages in thread
From: Maciej Purski @ 2018-10-01 7:25 UTC (permalink / raw)
To: linux-arm-kernel
Hi all,
unfortunately, I don't work at kernel anymore. That would be great, if you could take over those patches and finish the
work.
Best regards,
Maciej
On 09/29/2018 02:44 AM, Dmitry Osipenko wrote:
> On 9/29/18 3:27 AM, Tony Lindgren wrote:
>> * Dmitry Osipenko <digetx@gmail.com> [180928 23:55]:
>>> On 9/29/18 2:17 AM, Dmitry Osipenko wrote:
>>>> On 9/29/18 1:41 AM, Tony Lindgren wrote:
>>>>> I'm still getting these errors after init:
>>>>
>>>> Thank you very much again, seems I got what's wrong with your case. The ti-abb-regulator driver sets the "abb->current_info_idx = -EINVAL" on probe and that value is getting updated only after the first voltage change, hence _regulator_get_voltage() returns -22.
>>
>> OK that's starting to make some sense now thanks.
>>
>>>> Please try this patch:
>>>
>>> I've revised the patch and here is the current final version.
>>
>> Hey cool this one works now :) I suggest you rework the whole series
>> with these fixes. I recall the series had a problem with git bisect
>> too between the patches. It will make life easier for other people
>> who may need to git bisect these patches.
>
> Awesome! There are few other things in this patchset that also need fixing. I've asked Maciej if he's going to continue working on the patches, waiting for the answer. I can pick up the patches and try to finish the work if necessary.
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev
2018-10-01 7:25 ` Maciej Purski
@ 2018-10-01 13:34 ` Dmitry Osipenko
-1 siblings, 0 replies; 81+ messages in thread
From: Dmitry Osipenko @ 2018-10-01 13:34 UTC (permalink / raw)
To: Maciej Purski, Mark Brown
Cc: Tony Lindgren, linux-kernel, linux-arm-kernel, linux-omap,
Carlos Hernandez, Marek Szyprowski
On 10/1/18 10:25 AM, Maciej Purski wrote:
> Hi all,
> unfortunately, I don't work at kernel anymore. That would be great, if you could take over those patches and finish the
> work.
Hello Maciej,
I'll take care of the patches and try to push them forward, thank you.
^ permalink raw reply [flat|nested] 81+ messages in thread