From: Dmitry Osipenko <digetx@gmail.com> To: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org Subject: Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev Date: Fri, 28 Sep 2018 23:09:17 +0300 [thread overview] Message-ID: <95655644-ef41-f5bd-7e8e-a257d48cd020@gmail.com> (raw) In-Reply-To: <20180702080505.GN112168@atomide.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Osipenko <digetx@gmail.com> To: Tony Lindgren <tony@atomide.com>, Maciej Purski <m.purski@samsung.com>, Mark Brown <broonie@kernel.org> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org, Carlos Hernandez <ceh@ti.com>, Marek Szyprowski <m.szyprowski@samsung.com> Subject: Re: [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev Date: Fri, 28 Sep 2018 23:09:17 +0300 [thread overview] Message-ID: <95655644-ef41-f5bd-7e8e-a257d48cd020@gmail.com> (raw) Message-ID: <20180928200917.7y7CZjkYadBgLvbQOo4hX9lmx8XXAQQMHkGSaJCP0S8@z> (raw) In-Reply-To: <20180702080505.GN112168@atomide.com> 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
next prev parent reply other threads:[~2018-09-28 20:09 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-29 22:15 Regression in Linux next again Tony Lindgren 2018-05-30 9:13 ` Mark Brown 2018-05-30 14:03 ` Maciej Purski 2018-05-30 14:33 ` Mark Brown 2018-05-30 14:45 ` Tony Lindgren [not found] ` <CGME20180604135952eucas1p292f7bcec405e6a1a6261031df36cad32@eucas1p2.samsung.com> 2018-06-04 13:59 ` Maciej Purski [not found] ` <CGME20180604135952eucas1p2d76b6aa5d8fc9912113d519b48f7e99a@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 1/7] regulator: core: Add debug messages Maciej Purski [not found] ` <CGME20180604135952eucas1p2e3fdb68bf31e32b7c9557051671885a9@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 2/7] regulator: core: Add regulator_set_voltage_rdev() Maciej Purski [not found] ` <CGME20180604135953eucas1p2f2c9dd9581cd114d323c3d64afe5c308@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 3/7] regulator: core: Use re-entrant locks Maciej Purski [not found] ` <CGME20180604135953eucas1p2ec281df0793bc73e79f3000837abcb04@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 4/7] regulator: core: Implement voltage balancing algorithm Maciej Purski [not found] ` <CGME20180604135954eucas1p2156fed3300b5514a4efa2baf9e7b9bc5@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 5/7] regulator: core: Lock dependent regulators Maciej Purski 2018-06-04 14:20 ` Lucas Stach 2018-06-18 12:37 ` Maciej Purski [not found] ` <CGME20180604135954eucas1p2eeb77ada3ca97fecc6caec20d7e8397a@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 6/7] regulator: core: Lock dependent regulators on regulator_enable() Maciej Purski [not found] ` <CGME20180604135954eucas1p2bebd1c4970401bb957da228056f9a662@eucas1p2.samsung.com> 2018-06-04 13:59 ` [PATCH 7/7] regulator: core: Enable voltage balancing Maciej Purski 2018-06-04 23:13 ` kbuild test robot 2018-06-04 23:54 ` kbuild test robot 2018-06-05 4:45 ` Regression in Linux next again Tony Lindgren [not found] ` <CGME20180613103622eucas1p1778ba2c2e5dd85ccb4c488bd0a38386d@eucas1p1.samsung.com> 2018-06-13 10:33 ` [PATCH v2] regulator: core: Enable voltage balancing Maciej Purski 2018-06-15 11:29 ` Tony Lindgren 2018-06-18 13:17 ` Maciej Purski [not found] ` <CGME20180618140856eucas1p281619f9bf003655a3c2eac356216ab25@eucas1p2.samsung.com> 2018-06-18 14:08 ` [PATCH] regulator: core: Pass max_uV value to regulator_set_voltage_rdev Maciej Purski 2018-07-02 8:05 ` Tony Lindgren 2018-09-28 20:09 ` Dmitry Osipenko [this message] 2018-09-28 20:09 ` Dmitry Osipenko 2018-09-28 20:22 ` Tony Lindgren 2018-09-28 20:36 ` Dmitry Osipenko 2018-09-28 22:26 ` Dmitry Osipenko 2018-09-28 22:41 ` Tony Lindgren 2018-09-28 23:17 ` Dmitry Osipenko 2018-09-28 23:51 ` Dmitry Osipenko 2018-09-29 0:27 ` Tony Lindgren 2018-09-29 0:44 ` Dmitry Osipenko 2018-10-01 7:25 ` Maciej Purski 2018-10-01 13:34 ` Dmitry Osipenko 2018-05-30 14:53 ` Regression in Linux next again Naresh Kamboju 2018-05-31 5:44 ` Naresh Kamboju
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=95655644-ef41-f5bd-7e8e-a257d48cd020@gmail.com \ --to=digetx@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).