| Message ID | 1529330913-11152-1-git-send-email-m.purski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
* 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
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.
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;
* 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
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;
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;
* 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
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.
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. > >
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.
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,
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(-)