Regression in Linux next again

Regression in Linux next again

[Tony Lindgren]

Hi,

Linux next has a regression at least on beaglebone-x15 where
booting Linux hangs shortly after starting init when loading
modules with no output. I bisected it down to commit
456e7cdf3b1a ("regulator: core: Change voltage setting path")

I think I bisected this same issue for the second time now
but for a different merge window. What's up with that?

And then we also have commit 696861761a58 ("regulator: core: Add
voltage balancing mechanism") that fails to compile breaking
git bisect:

drivers/regulator/core.c: In function 'regulator_balance_voltage':
drivers/regulator/core.c:3284:9: error: implicit declaration of
function 'regulator_set_voltage'.

Reverting both patches fixes the issue for me. I could
not debug it further because of the compile error(s).

Regards,

Tony

Re: Regression in Linux next again

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

On Tue, May 29, 2018 at 03:15:01PM -0700, Tony Lindgren wrote:

> I think I bisected this same issue for the second time now
> but for a different merge window. What's up with that?

Last time we just reverted it as Maciej was unable to reproduce your
problem, he's tried again with some alterations.

> Reverting both patches fixes the issue for me. I could
> not debug it further because of the compile error(s).

OK, unless this gets fixed really quickly I'll revert again.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression in Linux next again

[Maciej Purski]

On 05/30/2018 11:13 AM, Mark Brown wrote:
> On Tue, May 29, 2018 at 03:15:01PM -0700, Tony Lindgren wrote:
> 
>> I think I bisected this same issue for the second time now
>> but for a different merge window. What's up with that?
> 
> Last time we just reverted it as Maciej was unable to reproduce your
> problem, he's tried again with some alterations.
> 
>> Reverting both patches fixes the issue for me. I could
>> not debug it further because of the compile error(s).
> 
> OK, unless this gets fixed really quickly I'll revert again.
> 

I'm afraid, I have no idea, how to fix it quickly. You can revert it and
in the next version I'll fix the build error and split the last patch even
more, so we could perform a more precise bisection. I'd be grateful if you
could push it on your test coupled branch and Tony could test it again before
merging it with next again.

Best regards,

Maciej Purski

Re: Regression in Linux next again

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On Wed, May 30, 2018 at 04:03:15PM +0200, Maciej Purski wrote:

> I'm afraid, I have no idea, how to fix it quickly. You can revert it and
> in the next version I'll fix the build error and split the last patch even
> more, so we could perform a more precise bisection. I'd be grateful if you
> could push it on your test coupled branch and Tony could test it again before
> merging it with next again.

Yeah, if we could get testing from Tony first that'd be ideal.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression in Linux next again

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [180530 14:36]:
> On Wed, May 30, 2018 at 04:03:15PM +0200, Maciej Purski wrote:
> 
> > I'm afraid, I have no idea, how to fix it quickly. You can revert it and
> > in the next version I'll fix the build error and split the last patch even
> > more, so we could perform a more precise bisection. I'd be grateful if you
> > could push it on your test coupled branch and Tony could test it again before
> > merging it with next again.
> 
> Yeah, if we could get testing from Tony first that'd be ideal.

I can boot test the patches easily no problem. Maciej, maybe email
me some debug version and I'll email you back the dmesg output.

Regards,

Tony

Re: Regression in Linux next again

[Naresh Kamboju]

On 30 May 2018 at 20:03, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 04:03:15PM +0200, Maciej Purski wrote:
>
>> I'm afraid, I have no idea, how to fix it quickly. You can revert it and
>> in the next version I'll fix the build error and split the last patch even
>> more, so we could perform a more precise bisection. I'd be grateful if you
>> could push it on your test coupled branch and Tony could test it again before
>> merging it with next again.
>
> Yeah, if we could get testing from Tony first that'd be ideal.

Linux next 4.17.0-rc7-next-20180529 boot failed on x15 device.
Manually reproduced boot failed problem on x15 device.
The qemu_arm32 boots successfully.

Ref bug:
LKFT: linux-next: Boot failed on beagle board x15
https://bugs.linaro.org/show_bug.cgi?id=3863

Re: Regression in Linux next again

[Naresh Kamboju]

On 30 May 2018 at 20:23, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
> On 30 May 2018 at 20:03, Mark Brown <broonie@kernel.org> wrote:
>> On Wed, May 30, 2018 at 04:03:15PM +0200, Maciej Purski wrote:
>>
>>> I'm afraid, I have no idea, how to fix it quickly. You can revert it and
>>> in the next version I'll fix the build error and split the last patch even
>>> more, so we could perform a more precise bisection. I'd be grateful if you
>>> could push it on your test coupled branch and Tony could test it again before
>>> merging it with next again.
>>
>> Yeah, if we could get testing from Tony first that'd be ideal.
>
> Linux next 4.17.0-rc7-next-20180529 boot failed on x15 device.
> Manually reproduced boot failed problem on x15 device.
> The qemu_arm32 boots successfully.
>
> Ref bug:
> LKFT: linux-next: Boot failed on beagle board x15
> https://bugs.linaro.org/show_bug.cgi?id=3863

This bug still happening on 4.17.0-rc7-next-20180530 on beagle board x15.

Linux version 4.17.0-rc7-next-20180530 (buildslave@x86-64-07) (gcc version
 6.2.1 20161016 (Linaro GCC 6.2-2016.11)) #1 SMP Wed May 30 12:38:23 UTC 2018

- Naresh

Re: Regression in Linux next again

[Maciej Purski]

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

[PATCH 1/7] regulator: core: Add debug messages

[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

[PATCH 2/7] regulator: core: Add regulator_set_voltage_rdev()

[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

[PATCH 3/7] regulator: core: Use re-entrant locks

[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(&regulator->rdev->mutex);
+	regulator_lock(regulator->rdev);
 	ret = _regulator_is_enabled(regulator->rdev);
-	mutex_unlock(&regulator->rdev->mutex);
+	regulator_unlock(regulator->rdev);
 
 	return ret;
 }
-- 
2.7.4

[PATCH 4/7] regulator: core: Implement voltage balancing algorithm

[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

[PATCH 5/7] regulator: core: Lock dependent regulators

[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

[PATCH 6/7] regulator: core: Lock dependent regulators on regulator_enable()

[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

[PATCH 7/7] regulator: core: Enable voltage balancing

[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

Re: [PATCH 5/7] regulator: core: Lock dependent regulators

[Lucas Stach]

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;
>  }

Re: [PATCH 7/7] regulator: core: Enable voltage balancing

[kbuild test robot]

[-- 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 = &regulator->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 --]

Re: [PATCH 7/7] regulator: core: Enable voltage balancing

[kbuild test robot]

[-- 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 = &regulator->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 --]

Re: Regression in Linux next again

[Tony Lindgren]

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

[PATCH v2] regulator: core: Enable voltage balancing

[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

Re: [PATCH v2] regulator: core: Enable voltage balancing

[Tony Lindgren]

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

Re: [PATCH 5/7] regulator: core: Lock dependent regulators

[Maciej Purski]

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;
>>   }
> 
> 
> 

Re: [PATCH v2] regulator: core: Enable voltage balancing

[Maciej Purski]

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

[PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Tony Lindgren]

* 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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Tony Lindgren]

* 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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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.

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Tony Lindgren]

* 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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Tony Lindgren]

* 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

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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.

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Maciej Purski]

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.
> 
> 

Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev

[Dmitry Osipenko]

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.