| Message ID | 20200528131130.17984-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | regulator: do not balance regulators without constraints | expand |
On Thu, May 28, 2020 at 03:11:30PM +0200, Marek Szyprowski wrote: > Balancing coupled regulators must wait until the clients for all of the > coupled regualtors set their constraints, otherwise the balancing code > might change the voltage of the not-yet-constrained regulator to the > value below the bootloader-configured operation point, what might cause a > system crash. This forces every supply to have something which explicitly manages voltages which means that if one of the coupled supplies doesn't really care about the voltage (perhaps doesn't even have any explicit consumers) and just needs to be within a certain range of another supply then it'll end up restricting things needlessly. Saravana was trying to do some stuff with sync_state() which might be interesting here although I have concerns with that approach too: https://lore.kernel.org/lkml/20200527074057.246606-1-saravanak@google.com/
Hi Mark, On 28.05.2020 15:43, Mark Brown wrote: > On Thu, May 28, 2020 at 03:11:30PM +0200, Marek Szyprowski wrote: >> Balancing coupled regulators must wait until the clients for all of the >> coupled regualtors set their constraints, otherwise the balancing code >> might change the voltage of the not-yet-constrained regulator to the >> value below the bootloader-configured operation point, what might cause a >> system crash. > This forces every supply to have something which explicitly manages > voltages which means that if one of the coupled supplies doesn't really > care about the voltage (perhaps doesn't even have any explicit > consumers) and just needs to be within a certain range of another supply > then it'll end up restricting things needlessly. Frankly, that's exactly what we need for Exynos5422 case. If devfreq driver is not enabled/compiled, we want to keep the "vdd_int" volatage unchanged. This confirms me that we really need to have a custom coupler for Exynos5422 case. It will solve such issues without adding hacks to regulator core. > Saravana was trying to do some stuff with sync_state() which might be > interesting here although I have concerns with that approach too: > > https://lore.kernel.org/lkml/20200527074057.246606-1-saravanak@google.com/ This still doesn't solve the above mentioned case. Best regards
On Fri, May 29, 2020 at 07:45:06AM +0200, Marek Szyprowski wrote: > On 28.05.2020 15:43, Mark Brown wrote: > > This forces every supply to have something which explicitly manages > > voltages which means that if one of the coupled supplies doesn't really > > care about the voltage (perhaps doesn't even have any explicit > > consumers) and just needs to be within a certain range of another supply > > then it'll end up restricting things needlessly. > Frankly, that's exactly what we need for Exynos5422 case. If devfreq > driver is not enabled/compiled, we want to keep the "vdd_int" volatage > unchanged. This confirms me that we really need to have a custom coupler > for Exynos5422 case. It will solve such issues without adding hacks to > regulator core. It sounds like you need that or some form of cooperation between the devfreq and cpufreq drivers. > > Saravana was trying to do some stuff with sync_state() which might be > > interesting here although I have concerns with that approach too: > > https://lore.kernel.org/lkml/20200527074057.246606-1-saravanak@google.com/ > This still doesn't solve the above mentioned case. I didn't mean the particular patch, I meant something using the sync_state() callback.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 941783a14b45..c1d77d44186b 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3697,10 +3697,21 @@ static int regulator_balance_voltage(struct regulator_dev *rdev, * the coupled voltages. */ int optimal_uV = 0, optimal_max_uV = 0, current_uV = 0; + int cons_uV = 0, cons_max_uV = INT_MAX; if (test_bit(i, &c_rdev_done)) continue; + ret = regulator_check_consumers(c_rdevs[i], + &cons_uV, + &cons_max_uV, state); + if (ret < 0) + goto out; + + /* no constraints set - ignore */ + if (cons_uV == 0) + continue; + ret = regulator_get_optimal_voltage(c_rdevs[i], ¤t_uV, &optimal_uV,
Balancing coupled regulators must wait until the clients for all of the coupled regualtors set their constraints, otherwise the balancing code might change the voltage of the not-yet-constrained regulator to the value below the bootloader-configured operation point, what might cause a system crash. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- This is probably a generalization of the issue aleady observed and reported here: https://lore.kernel.org/linux-samsung-soc/20191008101709.qVNy8eijBi0LynOteWFMnTg4GUwKG599n6OyYoX1Abs@z/ https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/ https://lore.kernel.org/linux-pm/cover.1589528491.git.viresh.kumar@linaro.org/ The problem is with "vdd_int" regulator coupled with "vdd_arm" on Odroid XU3/XU4 boards family. "vdd_arm" is handled by CPUfreq. "vdd_int" is handled by devfreq. CPUfreq initialized quite early during boot and it starts changing OPPs and "vdd_arm" value. Sometimes CPU activity during boot goes down and some low-frequency OPPs are selected, what in turn causes lowering "vdd_arm". This happens before devfreq applies its requirements on "vdd_int". Regulator balancing code reduces "vdd_arm" voltage value, what in turn causes lowering "vdd_int" value to the lowest possible value. This is much below the operation point of the wcore bus, which still runs at the highest frequency. The issue was hard to notice because in the most cases the board managed to boot properly, even when the regulator was set to lowest value allowed by the regulator constraints. However, it caused some random issues, which can be observed as "Unhandled prefetch abort" or low USB stability. I know that adding more and more special cases to the generic code is not the best idea, but so far I see no other way to fix this issue. The only other solution that comes to my mind is admiting that it is not possible to have generic regulator coupler and this needs board-specific code in all cases. Such code might take care of those corner cases if they are critical. Best regards, Marek Szyprowski --- drivers/regulator/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)