* [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() [not found] <CGME20191008101720eucas1p2e0d1bca6e696848bf689067e05620679@eucas1p2.samsung.com> @ 2019-10-08 10:17 ` Marek Szyprowski 2019-10-08 10:17 ` Marek Szyprowski 2019-10-08 11:50 ` Mark Brown 0 siblings, 2 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-08 10:17 UTC (permalink / raw) To: linux-kernel, Mark Brown, Dmitry Osipenko Cc: Marek Szyprowski, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking"), regardless of the subject, added additional call to regulator_balance_voltage() during regulator_enable(). This is basically a good idea, however it causes some issue for the regulators which are already enabled at boot and are critical for system operation (for example provides supply to the CPU). CPUfreq or other drivers typically call regulator_enable() on such regulators during their probe, although the regulators are already enabled by bootloader. The mentioned patch however added a call to regulator_balance_voltage(), what in case of system boot, where no additional requirements are set yet, typically causes to limit the voltage to the minimal value defined at regulator constraints. This causes a crash of the system when voltage on the CPU regulator is set to the lowest possible value without adjusting the operation frequency. Fix this by adding a check if regulator is already enabled - if so, then skip the balancing procedure. The voltage will be balanced later anyway once the required voltage value is requested. Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking") Reported-by: "kernelci.org bot" <bot@kernelci.org> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- This patch fixes enabling coupled requlators on Exynos5800-based Peach-Pi board done by the following patch: https://patchwork.kernel.org/patch/11172427/ Once it has been applied, the following issue has been reported: https://patchwork.kernel.org/patch/11176661/ --- drivers/regulator/core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index afe94470b67f..aca74b83f3bc 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2481,7 +2481,8 @@ static int _regulator_enable(struct regulator *regulator) } /* balance only if there are regulators coupled */ - if (rdev->coupling_desc.n_coupled > 1) { + if (rdev->coupling_desc.n_coupled > 1 && + !_regulator_is_enabled(rdev)) { ret = regulator_balance_voltage(rdev, PM_SUSPEND_ON); if (ret < 0) goto err_disable_supply; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 10:17 ` [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() Marek Szyprowski @ 2019-10-08 10:17 ` Marek Szyprowski 2019-10-08 11:50 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-08 10:17 UTC (permalink / raw) To: linux-kernel, Mark Brown, Dmitry Osipenko Cc: Marek Szyprowski, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking"), regardless of the subject, added additional call to regulator_balance_voltage() during regulator_enable(). This is basically a good idea, however it causes some issue for the regulators which are already enabled at boot and are critical for system operation (for example provides supply to the CPU). CPUfreq or other drivers typically call regulator_enable() on such regulators during their probe, although the regulators are already enabled by bootloader. The mentioned patch however added a call to regulator_balance_voltage(), what in case of system boot, where no additional requirements are set yet, typically causes to limit the voltage to the minimal value defined at regulator constraints. This causes a crash of the system when voltage on the CPU regulator is set to the lowest possible value without adjusting the operation frequency. Fix this by adding a check if regulator is already enabled - if so, then skip the balancing procedure. The voltage will be balanced later anyway once the required voltage value is requested. Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking") Reported-by: "kernelci.org bot" <bot@kernelci.org> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- This patch fixes enabling coupled requlators on Exynos5800-based Peach-Pi board done by the following patch: https://patchwork.kernel.org/patch/11172427/ Once it has been applied, the following issue has been reported: https://patchwork.kernel.org/patch/11176661/ --- drivers/regulator/core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index afe94470b67f..aca74b83f3bc 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -2481,7 +2481,8 @@ static int _regulator_enable(struct regulator *regulator) } /* balance only if there are regulators coupled */ - if (rdev->coupling_desc.n_coupled > 1) { + if (rdev->coupling_desc.n_coupled > 1 && + !_regulator_is_enabled(rdev)) { ret = regulator_balance_voltage(rdev, PM_SUSPEND_ON); if (ret < 0) goto err_disable_supply; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 10:17 ` [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() Marek Szyprowski 2019-10-08 10:17 ` Marek Szyprowski @ 2019-10-08 11:50 ` Mark Brown 2019-10-08 11:50 ` Mark Brown 2019-10-08 12:01 ` Marek Szyprowski 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 11:50 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc [-- Attachment #1: Type: text/plain, Size: 1894 bytes --] On Tue, Oct 08, 2019 at 12:17:09PM +0200, Marek Szyprowski wrote: > Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators > locking"), regardless of the subject, added additional call to > regulator_balance_voltage() during regulator_enable(). This is basically > a good idea, however it causes some issue for the regulators which are > already enabled at boot and are critical for system operation (for example > provides supply to the CPU). If regulators are essential to system operation they should be marked as always-on... > CPUfreq or other drivers typically call regulator_enable() on such > regulators during their probe, although the regulators are already enabled > by bootloader. The mentioned patch however added a call to > regulator_balance_voltage(), what in case of system boot, where no > additional requirements are set yet, typically causes to limit the voltage > to the minimal value defined at regulator constraints. This causes a crash > of the system when voltage on the CPU regulator is set to the lowest > possible value without adjusting the operation frequency. Fix this by > adding a check if regulator is already enabled - if so, then skip the > balancing procedure. The voltage will be balanced later anyway once the > required voltage value is requested. This then means that for users that might legitimately enable and disable regulators that need to be constrained are forced to change the voltage when they enable the regualtors in order to have their constraints take effect which seems bad. I'd rather change the the cpufreq consumers to either not do the enable (since there really should be an always-on constraint this should be redundant, we might need to fix the core to take account of their settings though I think we lost that) or to set the voltage to whatever they need prior to doing their first enable, that seems more robust. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 11:50 ` Mark Brown @ 2019-10-08 11:50 ` Mark Brown 2019-10-08 12:01 ` Marek Szyprowski 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 11:50 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc [-- Attachment #1: Type: text/plain, Size: 1894 bytes --] On Tue, Oct 08, 2019 at 12:17:09PM +0200, Marek Szyprowski wrote: > Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators > locking"), regardless of the subject, added additional call to > regulator_balance_voltage() during regulator_enable(). This is basically > a good idea, however it causes some issue for the regulators which are > already enabled at boot and are critical for system operation (for example > provides supply to the CPU). If regulators are essential to system operation they should be marked as always-on... > CPUfreq or other drivers typically call regulator_enable() on such > regulators during their probe, although the regulators are already enabled > by bootloader. The mentioned patch however added a call to > regulator_balance_voltage(), what in case of system boot, where no > additional requirements are set yet, typically causes to limit the voltage > to the minimal value defined at regulator constraints. This causes a crash > of the system when voltage on the CPU regulator is set to the lowest > possible value without adjusting the operation frequency. Fix this by > adding a check if regulator is already enabled - if so, then skip the > balancing procedure. The voltage will be balanced later anyway once the > required voltage value is requested. This then means that for users that might legitimately enable and disable regulators that need to be constrained are forced to change the voltage when they enable the regualtors in order to have their constraints take effect which seems bad. I'd rather change the the cpufreq consumers to either not do the enable (since there really should be an always-on constraint this should be redundant, we might need to fix the core to take account of their settings though I think we lost that) or to set the voltage to whatever they need prior to doing their first enable, that seems more robust. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 11:50 ` Mark Brown 2019-10-08 11:50 ` Mark Brown @ 2019-10-08 12:01 ` Marek Szyprowski 2019-10-08 12:01 ` Marek Szyprowski 2019-10-08 12:06 ` Mark Brown 1 sibling, 2 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-08 12:01 UTC (permalink / raw) To: Mark Brown Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc Hi Mark, On 08.10.2019 13:50, Mark Brown wrote: > On Tue, Oct 08, 2019 at 12:17:09PM +0200, Marek Szyprowski wrote: >> Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators >> locking"), regardless of the subject, added additional call to >> regulator_balance_voltage() during regulator_enable(). This is basically >> a good idea, however it causes some issue for the regulators which are >> already enabled at boot and are critical for system operation (for example >> provides supply to the CPU). > If regulators are essential to system operation they should be marked as > always-on... The are marked as always on: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/exynos5800-peach-pi.dts#n253 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/exynos5800-peach-pi.dts#n265 >> CPUfreq or other drivers typically call regulator_enable() on such >> regulators during their probe, although the regulators are already enabled >> by bootloader. The mentioned patch however added a call to >> regulator_balance_voltage(), what in case of system boot, where no >> additional requirements are set yet, typically causes to limit the voltage >> to the minimal value defined at regulator constraints. This causes a crash >> of the system when voltage on the CPU regulator is set to the lowest >> possible value without adjusting the operation frequency. Fix this by >> adding a check if regulator is already enabled - if so, then skip the >> balancing procedure. The voltage will be balanced later anyway once the >> required voltage value is requested. > This then means that for users that might legitimately enable and > disable regulators that need to be constrained are forced to change the > voltage when they enable the regualtors in order to have their > constraints take effect which seems bad. I'd rather change the the > cpufreq consumers to either not do the enable (since there really should > be an always-on constraint this should be redundant, we might need to > fix the core to take account of their settings though I think we lost > that) or to set the voltage to whatever they need prior to doing their > first enable, that seems more robust. Well, I'm open for other ways of fixing this issue. Calling enable on always-on regulator imho should not change its rate... Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:01 ` Marek Szyprowski @ 2019-10-08 12:01 ` Marek Szyprowski 2019-10-08 12:06 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-08 12:01 UTC (permalink / raw) To: Mark Brown Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc Hi Mark, On 08.10.2019 13:50, Mark Brown wrote: > On Tue, Oct 08, 2019 at 12:17:09PM +0200, Marek Szyprowski wrote: >> Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators >> locking"), regardless of the subject, added additional call to >> regulator_balance_voltage() during regulator_enable(). This is basically >> a good idea, however it causes some issue for the regulators which are >> already enabled at boot and are critical for system operation (for example >> provides supply to the CPU). > If regulators are essential to system operation they should be marked as > always-on... The are marked as always on: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/exynos5800-peach-pi.dts#n253 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/exynos5800-peach-pi.dts#n265 >> CPUfreq or other drivers typically call regulator_enable() on such >> regulators during their probe, although the regulators are already enabled >> by bootloader. The mentioned patch however added a call to >> regulator_balance_voltage(), what in case of system boot, where no >> additional requirements are set yet, typically causes to limit the voltage >> to the minimal value defined at regulator constraints. This causes a crash >> of the system when voltage on the CPU regulator is set to the lowest >> possible value without adjusting the operation frequency. Fix this by >> adding a check if regulator is already enabled - if so, then skip the >> balancing procedure. The voltage will be balanced later anyway once the >> required voltage value is requested. > This then means that for users that might legitimately enable and > disable regulators that need to be constrained are forced to change the > voltage when they enable the regualtors in order to have their > constraints take effect which seems bad. I'd rather change the the > cpufreq consumers to either not do the enable (since there really should > be an always-on constraint this should be redundant, we might need to > fix the core to take account of their settings though I think we lost > that) or to set the voltage to whatever they need prior to doing their > first enable, that seems more robust. Well, I'm open for other ways of fixing this issue. Calling enable on always-on regulator imho should not change its rate... Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:01 ` Marek Szyprowski 2019-10-08 12:01 ` Marek Szyprowski @ 2019-10-08 12:06 ` Mark Brown 2019-10-08 12:06 ` Mark Brown 2019-10-08 12:38 ` Marek Szyprowski 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 12:06 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc [-- Attachment #1: Type: text/plain, Size: 1224 bytes --] On Tue, Oct 08, 2019 at 02:01:15PM +0200, Marek Szyprowski wrote: > On 08.10.2019 13:50, Mark Brown wrote: > > This then means that for users that might legitimately enable and > > disable regulators that need to be constrained are forced to change the > > voltage when they enable the regualtors in order to have their > > constraints take effect which seems bad. I'd rather change the the > > cpufreq consumers to either not do the enable (since there really should > > be an always-on constraint this should be redundant, we might need to > > fix the core to take account of their settings though I think we lost > > that) or to set the voltage to whatever they need prior to doing their > > first enable, that seems more robust. > Well, I'm open for other ways of fixing this issue. Calling enable on > always-on regulator imho should not change its rate... Yes, although there is the whole "don't touch things until a consumer tells us to" thing going on. I had expected that this was kicking in because we weren't paying attention to the constraints of disabled regulators but I can't see the code implementing that any more so I guess we removed it at some point (it was always debatable). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:06 ` Mark Brown @ 2019-10-08 12:06 ` Mark Brown 2019-10-08 12:38 ` Marek Szyprowski 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 12:06 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc [-- Attachment #1: Type: text/plain, Size: 1224 bytes --] On Tue, Oct 08, 2019 at 02:01:15PM +0200, Marek Szyprowski wrote: > On 08.10.2019 13:50, Mark Brown wrote: > > This then means that for users that might legitimately enable and > > disable regulators that need to be constrained are forced to change the > > voltage when they enable the regualtors in order to have their > > constraints take effect which seems bad. I'd rather change the the > > cpufreq consumers to either not do the enable (since there really should > > be an always-on constraint this should be redundant, we might need to > > fix the core to take account of their settings though I think we lost > > that) or to set the voltage to whatever they need prior to doing their > > first enable, that seems more robust. > Well, I'm open for other ways of fixing this issue. Calling enable on > always-on regulator imho should not change its rate... Yes, although there is the whole "don't touch things until a consumer tells us to" thing going on. I had expected that this was kicking in because we weren't paying attention to the constraints of disabled regulators but I can't see the code implementing that any more so I guess we removed it at some point (it was always debatable). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:06 ` Mark Brown 2019-10-08 12:06 ` Mark Brown @ 2019-10-08 12:38 ` Marek Szyprowski 2019-10-08 12:38 ` Marek Szyprowski 2019-10-08 12:47 ` Mark Brown 1 sibling, 2 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-08 12:38 UTC (permalink / raw) To: Mark Brown Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 08.10.2019 14:06, Mark Brown wrote: > On Tue, Oct 08, 2019 at 02:01:15PM +0200, Marek Szyprowski wrote: >> On 08.10.2019 13:50, Mark Brown wrote: >>> This then means that for users that might legitimately enable and >>> disable regulators that need to be constrained are forced to change the >>> voltage when they enable the regualtors in order to have their >>> constraints take effect which seems bad. I'd rather change the the >>> cpufreq consumers to either not do the enable (since there really should >>> be an always-on constraint this should be redundant, we might need to >>> fix the core to take account of their settings though I think we lost >>> that) or to set the voltage to whatever they need prior to doing their >>> first enable, that seems more robust. >> Well, I'm open for other ways of fixing this issue. Calling enable on >> always-on regulator imho should not change its rate... > Yes, although there is the whole "don't touch things until a consumer > tells us to" thing going on. I had expected that this was kicking in > because we weren't paying attention to the constraints of disabled > regulators but I can't see the code implementing that any more so I > guess we removed it at some point (it was always debatable). Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable"). I've checked and indeed reverting it fixes Peach Pi to boot properly. The question is if this is desired behavior or not? I've CC: Viresh, Kamil and Bartlomiej, here is the link to the beginning of this thread: https://lkml.org/lkml/2019/10/8/265 Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:38 ` Marek Szyprowski @ 2019-10-08 12:38 ` Marek Szyprowski 2019-10-08 12:47 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-08 12:38 UTC (permalink / raw) To: Mark Brown Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 08.10.2019 14:06, Mark Brown wrote: > On Tue, Oct 08, 2019 at 02:01:15PM +0200, Marek Szyprowski wrote: >> On 08.10.2019 13:50, Mark Brown wrote: >>> This then means that for users that might legitimately enable and >>> disable regulators that need to be constrained are forced to change the >>> voltage when they enable the regualtors in order to have their >>> constraints take effect which seems bad. I'd rather change the the >>> cpufreq consumers to either not do the enable (since there really should >>> be an always-on constraint this should be redundant, we might need to >>> fix the core to take account of their settings though I think we lost >>> that) or to set the voltage to whatever they need prior to doing their >>> first enable, that seems more robust. >> Well, I'm open for other ways of fixing this issue. Calling enable on >> always-on regulator imho should not change its rate... > Yes, although there is the whole "don't touch things until a consumer > tells us to" thing going on. I had expected that this was kicking in > because we weren't paying attention to the constraints of disabled > regulators but I can't see the code implementing that any more so I > guess we removed it at some point (it was always debatable). Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable"). I've checked and indeed reverting it fixes Peach Pi to boot properly. The question is if this is desired behavior or not? I've CC: Viresh, Kamil and Bartlomiej, here is the link to the beginning of this thread: https://lkml.org/lkml/2019/10/8/265 Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:38 ` Marek Szyprowski 2019-10-08 12:38 ` Marek Szyprowski @ 2019-10-08 12:47 ` Mark Brown 2019-10-08 12:47 ` Mark Brown 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 12:47 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 651 bytes --] On Tue, Oct 08, 2019 at 02:38:55PM +0200, Marek Szyprowski wrote: > Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 > ("opp: core: add regulators enable and disable"). I've checked and > indeed reverting it fixes Peach Pi to boot properly. The question is if > this is desired behavior or not? That doesn't seem ideal - either it's redundant for regulators that need to be marked as always-on anyway or it's going to force the regulators on when a device could do runtime PM (eg, if the same code can run on something like a GPU which can be turned off while the screen is off or is displaying a static image). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:47 ` Mark Brown @ 2019-10-08 12:47 ` Mark Brown 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 12:47 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 651 bytes --] On Tue, Oct 08, 2019 at 02:38:55PM +0200, Marek Szyprowski wrote: > Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 > ("opp: core: add regulators enable and disable"). I've checked and > indeed reverting it fixes Peach Pi to boot properly. The question is if > this is desired behavior or not? That doesn't seem ideal - either it's redundant for regulators that need to be marked as always-on anyway or it's going to force the regulators on when a device could do runtime PM (eg, if the same code can run on something like a GPU which can be turned off while the screen is off or is displaying a static image). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 12:47 ` Mark Brown 2019-10-08 12:47 ` Mark Brown @ 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz ` (2 more replies) 1 sibling, 3 replies; 43+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2019-10-08 13:24 UTC (permalink / raw) To: Mark Brown, Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny On 10/8/19 2:47 PM, Mark Brown wrote: > On Tue, Oct 08, 2019 at 02:38:55PM +0200, Marek Szyprowski wrote: > >> Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 >> ("opp: core: add regulators enable and disable"). I've checked and >> indeed reverting it fixes Peach Pi to boot properly. The question is if >> this is desired behavior or not? > > That doesn't seem ideal - either it's redundant for regulators that need > to be marked as always-on anyway or it's going to force the regulators > on when a device could do runtime PM (eg, if the same code can run on > something like a GPU which can be turned off while the screen is off or > is displaying a static image). Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") currently can be safely reverted as all affected users use always-on regulators. However IMHO it should be possible to enable always-on regulator without side-effects. When it comes to setting regulator constraints before doing enable operation, it also seems to be possible solution but would require splitting regulator_set_voltage() operation on two functions: - one for setting constraints (before regulator_enable() operation) - the other one actually setting voltage (after enable operation) Unfortunately this is much bigger task and doesn't seem to be -rc time material so I'm in favor of just applying Marek's fix as it is for now. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz @ 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 15:48 ` Mark Brown 2 siblings, 0 replies; 43+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2019-10-08 13:24 UTC (permalink / raw) To: Mark Brown, Marek Szyprowski Cc: linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny On 10/8/19 2:47 PM, Mark Brown wrote: > On Tue, Oct 08, 2019 at 02:38:55PM +0200, Marek Szyprowski wrote: > >> Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 >> ("opp: core: add regulators enable and disable"). I've checked and >> indeed reverting it fixes Peach Pi to boot properly. The question is if >> this is desired behavior or not? > > That doesn't seem ideal - either it's redundant for regulators that need > to be marked as always-on anyway or it's going to force the regulators > on when a device could do runtime PM (eg, if the same code can run on > something like a GPU which can be turned off while the screen is off or > is displaying a static image). Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") currently can be safely reverted as all affected users use always-on regulators. However IMHO it should be possible to enable always-on regulator without side-effects. When it comes to setting regulator constraints before doing enable operation, it also seems to be possible solution but would require splitting regulator_set_voltage() operation on two functions: - one for setting constraints (before regulator_enable() operation) - the other one actually setting voltage (after enable operation) Unfortunately this is much bigger task and doesn't seem to be -rc time material so I'm in favor of just applying Marek's fix as it is for now. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz @ 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 16:15 ` Mark Brown 2019-10-08 15:48 ` Mark Brown 2 siblings, 2 replies; 43+ messages in thread From: Dmitry Osipenko @ 2019-10-08 15:02 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Mark Brown, Marek Szyprowski Cc: linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny 08.10.2019 16:24, Bartlomiej Zolnierkiewicz пишет: > > On 10/8/19 2:47 PM, Mark Brown wrote: >> On Tue, Oct 08, 2019 at 02:38:55PM +0200, Marek Szyprowski wrote: >> >>> Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 >>> ("opp: core: add regulators enable and disable"). I've checked and >>> indeed reverting it fixes Peach Pi to boot properly. Yes, please note that the "ww_mutex" patch didn't change the original logic and only rearranged the code a tad. The question is if >>> this is desired behavior or not? >> >> That doesn't seem ideal - either it's redundant for regulators that need >> to be marked as always-on anyway or it's going to force the regulators >> on when a device could do runtime PM (eg, if the same code can run on >> something like a GPU which can be turned off while the screen is off or >> is displaying a static image). > > Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") > currently can be safely reverted as all affected users use always-on > regulators. However IMHO it should be possible to enable always-on > regulator without side-effects. > > When it comes to setting regulator constraints before doing enable > operation, it also seems to be possible solution but would require > splitting regulator_set_voltage() operation on two functions: > > - one for setting constraints (before regulator_enable() operation) > > - the other one actually setting voltage (after enable operation) > > Unfortunately this is much bigger task and doesn't seem to be -rc > time material so I'm in favor of just applying Marek's fix as it is > for now. That OPP patch caused the same problem for the NVIDIA Tegra20 CPUFreq driver (in-progress) and I resolved it in the coupler's code [0]. Perhaps the generic coupler could do the same thing by assuming that min_uV=current_uV until any consumer sets the voltage, i.e. if regulator_check_consumers(min_uV=0) returns min_uV=0. [0] https://lkml.org/lkml/2019/7/25/892 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 15:02 ` Dmitry Osipenko @ 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 16:15 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Dmitry Osipenko @ 2019-10-08 15:02 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Mark Brown, Marek Szyprowski Cc: linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny 08.10.2019 16:24, Bartlomiej Zolnierkiewicz пишет: > > On 10/8/19 2:47 PM, Mark Brown wrote: >> On Tue, Oct 08, 2019 at 02:38:55PM +0200, Marek Szyprowski wrote: >> >>> Then if I get it right, the issue is caused by the commit 7f93ff73f7c8 >>> ("opp: core: add regulators enable and disable"). I've checked and >>> indeed reverting it fixes Peach Pi to boot properly. Yes, please note that the "ww_mutex" patch didn't change the original logic and only rearranged the code a tad. The question is if >>> this is desired behavior or not? >> >> That doesn't seem ideal - either it's redundant for regulators that need >> to be marked as always-on anyway or it's going to force the regulators >> on when a device could do runtime PM (eg, if the same code can run on >> something like a GPU which can be turned off while the screen is off or >> is displaying a static image). > > Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") > currently can be safely reverted as all affected users use always-on > regulators. However IMHO it should be possible to enable always-on > regulator without side-effects. > > When it comes to setting regulator constraints before doing enable > operation, it also seems to be possible solution but would require > splitting regulator_set_voltage() operation on two functions: > > - one for setting constraints (before regulator_enable() operation) > > - the other one actually setting voltage (after enable operation) > > Unfortunately this is much bigger task and doesn't seem to be -rc > time material so I'm in favor of just applying Marek's fix as it is > for now. That OPP patch caused the same problem for the NVIDIA Tegra20 CPUFreq driver (in-progress) and I resolved it in the coupler's code [0]. Perhaps the generic coupler could do the same thing by assuming that min_uV=current_uV until any consumer sets the voltage, i.e. if regulator_check_consumers(min_uV=0) returns min_uV=0. [0] https://lkml.org/lkml/2019/7/25/892 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 15:02 ` Dmitry Osipenko @ 2019-10-08 16:15 ` Mark Brown 2019-10-08 16:15 ` Mark Brown 2019-10-08 17:05 ` Dmitry Osipenko 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 16:15 UTC (permalink / raw) To: Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 947 bytes --] On Tue, Oct 08, 2019 at 06:02:36PM +0300, Dmitry Osipenko wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > That OPP patch caused the same problem for the NVIDIA Tegra20 CPUFreq > driver (in-progress) and I resolved it in the coupler's code [0]. > Perhaps the generic coupler could do the same thing by assuming that > min_uV=current_uV until any consumer sets the voltage, i.e. if > regulator_check_consumers(min_uV=0) returns min_uV=0. That sounds like it might just postpone the inevitable - if you set the wrong voltage first it might decide to drop down some voltage that wasn't expected. There's a bit of a bootstrapping issue. I think it would be safer to just say that anything that is within spec won't get changed any time we balance, we'd only change things if needed to bring them back into spec. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 16:15 ` Mark Brown @ 2019-10-08 16:15 ` Mark Brown 2019-10-08 17:05 ` Dmitry Osipenko 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 16:15 UTC (permalink / raw) To: Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 947 bytes --] On Tue, Oct 08, 2019 at 06:02:36PM +0300, Dmitry Osipenko wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > That OPP patch caused the same problem for the NVIDIA Tegra20 CPUFreq > driver (in-progress) and I resolved it in the coupler's code [0]. > Perhaps the generic coupler could do the same thing by assuming that > min_uV=current_uV until any consumer sets the voltage, i.e. if > regulator_check_consumers(min_uV=0) returns min_uV=0. That sounds like it might just postpone the inevitable - if you set the wrong voltage first it might decide to drop down some voltage that wasn't expected. There's a bit of a bootstrapping issue. I think it would be safer to just say that anything that is within spec won't get changed any time we balance, we'd only change things if needed to bring them back into spec. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 16:15 ` Mark Brown 2019-10-08 16:15 ` Mark Brown @ 2019-10-08 17:05 ` Dmitry Osipenko 2019-10-08 17:05 ` Dmitry Osipenko 2019-10-08 17:17 ` Mark Brown 1 sibling, 2 replies; 43+ messages in thread From: Dmitry Osipenko @ 2019-10-08 17:05 UTC (permalink / raw) To: Mark Brown Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny 08.10.2019 19:15, Mark Brown пишет: > On Tue, Oct 08, 2019 at 06:02:36PM +0300, Dmitry Osipenko wrote: > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to. Indeed, thanks! >> That OPP patch caused the same problem for the NVIDIA Tegra20 CPUFreq >> driver (in-progress) and I resolved it in the coupler's code [0]. >> Perhaps the generic coupler could do the same thing by assuming that >> min_uV=current_uV until any consumer sets the voltage, i.e. if >> regulator_check_consumers(min_uV=0) returns min_uV=0. > > That sounds like it might just postpone the inevitable - if you set the > wrong voltage first it might decide to drop down some voltage that > wasn't expected. There's a bit of a bootstrapping issue. I think it > would be safer to just say that anything that is within spec won't get > changed any time we balance, we'd only change things if needed to bring > them back into spec. Yes, the case of changing voltage before regulator is enabled seems won't work as expected. Maybe it won't hurt to disallow a non always-on regulators to be coupled until there will be a real user for that case. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 17:05 ` Dmitry Osipenko @ 2019-10-08 17:05 ` Dmitry Osipenko 2019-10-08 17:17 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Dmitry Osipenko @ 2019-10-08 17:05 UTC (permalink / raw) To: Mark Brown Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny 08.10.2019 19:15, Mark Brown пишет: > On Tue, Oct 08, 2019 at 06:02:36PM +0300, Dmitry Osipenko wrote: > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to. Indeed, thanks! >> That OPP patch caused the same problem for the NVIDIA Tegra20 CPUFreq >> driver (in-progress) and I resolved it in the coupler's code [0]. >> Perhaps the generic coupler could do the same thing by assuming that >> min_uV=current_uV until any consumer sets the voltage, i.e. if >> regulator_check_consumers(min_uV=0) returns min_uV=0. > > That sounds like it might just postpone the inevitable - if you set the > wrong voltage first it might decide to drop down some voltage that > wasn't expected. There's a bit of a bootstrapping issue. I think it > would be safer to just say that anything that is within spec won't get > changed any time we balance, we'd only change things if needed to bring > them back into spec. Yes, the case of changing voltage before regulator is enabled seems won't work as expected. Maybe it won't hurt to disallow a non always-on regulators to be coupled until there will be a real user for that case. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 17:05 ` Dmitry Osipenko 2019-10-08 17:05 ` Dmitry Osipenko @ 2019-10-08 17:17 ` Mark Brown 2019-10-08 17:17 ` Mark Brown 2019-10-08 18:00 ` Dmitry Osipenko 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 17:17 UTC (permalink / raw) To: Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 867 bytes --] On Tue, Oct 08, 2019 at 08:05:03PM +0300, Dmitry Osipenko wrote: > 08.10.2019 19:15, Mark Brown пишет: > > That sounds like it might just postpone the inevitable - if you set the > > wrong voltage first it might decide to drop down some voltage that > > wasn't expected. There's a bit of a bootstrapping issue. I think it > > would be safer to just say that anything that is within spec won't get > > changed any time we balance, we'd only change things if needed to bring > > them back into spec. > Yes, the case of changing voltage before regulator is enabled seems > won't work as expected. > Maybe it won't hurt to disallow a non always-on regulators to be coupled > until there will be a real user for that case. I thought that coupling with the CPU core and main SoC power regulators was one of the main use cases for this feature? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 17:17 ` Mark Brown @ 2019-10-08 17:17 ` Mark Brown 2019-10-08 18:00 ` Dmitry Osipenko 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 17:17 UTC (permalink / raw) To: Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 867 bytes --] On Tue, Oct 08, 2019 at 08:05:03PM +0300, Dmitry Osipenko wrote: > 08.10.2019 19:15, Mark Brown пишет: > > That sounds like it might just postpone the inevitable - if you set the > > wrong voltage first it might decide to drop down some voltage that > > wasn't expected. There's a bit of a bootstrapping issue. I think it > > would be safer to just say that anything that is within spec won't get > > changed any time we balance, we'd only change things if needed to bring > > them back into spec. > Yes, the case of changing voltage before regulator is enabled seems > won't work as expected. > Maybe it won't hurt to disallow a non always-on regulators to be coupled > until there will be a real user for that case. I thought that coupling with the CPU core and main SoC power regulators was one of the main use cases for this feature? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 17:17 ` Mark Brown 2019-10-08 17:17 ` Mark Brown @ 2019-10-08 18:00 ` Dmitry Osipenko 2019-10-08 18:00 ` Dmitry Osipenko 2019-10-08 18:07 ` Mark Brown 1 sibling, 2 replies; 43+ messages in thread From: Dmitry Osipenko @ 2019-10-08 18:00 UTC (permalink / raw) To: Mark Brown Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny 08.10.2019 20:17, Mark Brown пишет: > On Tue, Oct 08, 2019 at 08:05:03PM +0300, Dmitry Osipenko wrote: >> 08.10.2019 19:15, Mark Brown пишет: > >>> That sounds like it might just postpone the inevitable - if you set the >>> wrong voltage first it might decide to drop down some voltage that >>> wasn't expected. There's a bit of a bootstrapping issue. I think it >>> would be safer to just say that anything that is within spec won't get >>> changed any time we balance, we'd only change things if needed to bring >>> them back into spec. > >> Yes, the case of changing voltage before regulator is enabled seems >> won't work as expected. > >> Maybe it won't hurt to disallow a non always-on regulators to be coupled >> until there will be a real user for that case. > > I thought that coupling with the CPU core and main SoC power regulators > was one of the main use cases for this feature? > In a case of NVIDIA Tegra SoCs, it's coupling of CPU core *and* SoC core regulators. I see that Exynos also couples the always-on regulators. Properly handling case of a disabled coupled regulator certainly will be useful, but looks like there are no real users for that feature right now and thus no real testing is done. Keeping coupled regulators in s valid voltage range is the today's main feature. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 18:00 ` Dmitry Osipenko @ 2019-10-08 18:00 ` Dmitry Osipenko 2019-10-08 18:07 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Dmitry Osipenko @ 2019-10-08 18:00 UTC (permalink / raw) To: Mark Brown Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny 08.10.2019 20:17, Mark Brown пишет: > On Tue, Oct 08, 2019 at 08:05:03PM +0300, Dmitry Osipenko wrote: >> 08.10.2019 19:15, Mark Brown пишет: > >>> That sounds like it might just postpone the inevitable - if you set the >>> wrong voltage first it might decide to drop down some voltage that >>> wasn't expected. There's a bit of a bootstrapping issue. I think it >>> would be safer to just say that anything that is within spec won't get >>> changed any time we balance, we'd only change things if needed to bring >>> them back into spec. > >> Yes, the case of changing voltage before regulator is enabled seems >> won't work as expected. > >> Maybe it won't hurt to disallow a non always-on regulators to be coupled >> until there will be a real user for that case. > > I thought that coupling with the CPU core and main SoC power regulators > was one of the main use cases for this feature? > In a case of NVIDIA Tegra SoCs, it's coupling of CPU core *and* SoC core regulators. I see that Exynos also couples the always-on regulators. Properly handling case of a disabled coupled regulator certainly will be useful, but looks like there are no real users for that feature right now and thus no real testing is done. Keeping coupled regulators in s valid voltage range is the today's main feature. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 18:00 ` Dmitry Osipenko 2019-10-08 18:00 ` Dmitry Osipenko @ 2019-10-08 18:07 ` Mark Brown 2019-10-08 18:07 ` Mark Brown 2019-10-09 10:29 ` Marek Szyprowski 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 18:07 UTC (permalink / raw) To: Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 692 bytes --] On Tue, Oct 08, 2019 at 09:00:29PM +0300, Dmitry Osipenko wrote: > 08.10.2019 20:17, Mark Brown пишет: > > On Tue, Oct 08, 2019 at 08:05:03PM +0300, Dmitry Osipenko wrote: > >> Maybe it won't hurt to disallow a non always-on regulators to be coupled > >> until there will be a real user for that case. > > I thought that coupling with the CPU core and main SoC power regulators > > was one of the main use cases for this feature? > Properly handling case of a disabled coupled regulator certainly will be > useful, but looks like there are no real users for that feature right > now and thus no real testing is done. Right, sorry - I missed the double negative there. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 18:07 ` Mark Brown @ 2019-10-08 18:07 ` Mark Brown 2019-10-09 10:29 ` Marek Szyprowski 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 18:07 UTC (permalink / raw) To: Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 692 bytes --] On Tue, Oct 08, 2019 at 09:00:29PM +0300, Dmitry Osipenko wrote: > 08.10.2019 20:17, Mark Brown пишет: > > On Tue, Oct 08, 2019 at 08:05:03PM +0300, Dmitry Osipenko wrote: > >> Maybe it won't hurt to disallow a non always-on regulators to be coupled > >> until there will be a real user for that case. > > I thought that coupling with the CPU core and main SoC power regulators > > was one of the main use cases for this feature? > Properly handling case of a disabled coupled regulator certainly will be > useful, but looks like there are no real users for that feature right > now and thus no real testing is done. Right, sorry - I missed the double negative there. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 18:07 ` Mark Brown 2019-10-08 18:07 ` Mark Brown @ 2019-10-09 10:29 ` Marek Szyprowski 2019-10-09 10:29 ` Marek Szyprowski 2019-10-09 14:13 ` Mark Brown 1 sibling, 2 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-09 10:29 UTC (permalink / raw) To: Mark Brown, Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 08.10.2019 20:07, Mark Brown wrote: > On Tue, Oct 08, 2019 at 09:00:29PM +0300, Dmitry Osipenko wrote: >> Properly handling case of a disabled coupled regulator certainly will be >> useful, but looks like there are no real users for that feature right >> now and thus no real testing is done. > Right, sorry - I missed the double negative there. Okay, then what is the conclusion, as I got lost a bit? How do you want this issue to be fixed? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-09 10:29 ` Marek Szyprowski @ 2019-10-09 10:29 ` Marek Szyprowski 2019-10-09 14:13 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-09 10:29 UTC (permalink / raw) To: Mark Brown, Dmitry Osipenko Cc: Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 08.10.2019 20:07, Mark Brown wrote: > On Tue, Oct 08, 2019 at 09:00:29PM +0300, Dmitry Osipenko wrote: >> Properly handling case of a disabled coupled regulator certainly will be >> useful, but looks like there are no real users for that feature right >> now and thus no real testing is done. > Right, sorry - I missed the double negative there. Okay, then what is the conclusion, as I got lost a bit? How do you want this issue to be fixed? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-09 10:29 ` Marek Szyprowski 2019-10-09 10:29 ` Marek Szyprowski @ 2019-10-09 14:13 ` Mark Brown 2019-10-09 14:13 ` Mark Brown ` (2 more replies) 1 sibling, 3 replies; 43+ messages in thread From: Mark Brown @ 2019-10-09 14:13 UTC (permalink / raw) To: Marek Szyprowski Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 444 bytes --] On Wed, Oct 09, 2019 at 12:29:00PM +0200, Marek Szyprowski wrote: > Okay, then what is the conclusion, as I got lost a bit? How do you want > this issue to be fixed? We should revert the enable call, it shouldn't be required, and ideally the default balancer could be updated to only make configuration changes if they're actually required which would help avoid triggering any such things in future if we don't absolutely have to. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-09 14:13 ` Mark Brown @ 2019-10-09 14:13 ` Mark Brown 2019-10-10 7:29 ` Viresh Kumar 2019-10-10 10:19 ` Marek Szyprowski 2 siblings, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-09 14:13 UTC (permalink / raw) To: Marek Szyprowski Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 444 bytes --] On Wed, Oct 09, 2019 at 12:29:00PM +0200, Marek Szyprowski wrote: > Okay, then what is the conclusion, as I got lost a bit? How do you want > this issue to be fixed? We should revert the enable call, it shouldn't be required, and ideally the default balancer could be updated to only make configuration changes if they're actually required which would help avoid triggering any such things in future if we don't absolutely have to. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-09 14:13 ` Mark Brown 2019-10-09 14:13 ` Mark Brown @ 2019-10-10 7:29 ` Viresh Kumar 2019-10-10 7:29 ` Viresh Kumar 2019-10-10 10:19 ` Marek Szyprowski 2 siblings, 1 reply; 43+ messages in thread From: Viresh Kumar @ 2019-10-10 7:29 UTC (permalink / raw) To: Mark Brown Cc: Marek Szyprowski, Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Kamil Konieczny On 09-10-19, 15:13, Mark Brown wrote: > On Wed, Oct 09, 2019 at 12:29:00PM +0200, Marek Szyprowski wrote: > > > Okay, then what is the conclusion, as I got lost a bit? How do you want > > this issue to be fixed? > > We should revert the enable call, it shouldn't be required, and ideally > the default balancer could be updated to only make configuration changes > if they're actually required which would help avoid triggering any such > things in future if we don't absolutely have to. Sorry for the delay in responding, just came back after vacations. Should the OPP change be reverted ? Someone going to send that revert to me with the required explanation ? -- viresh ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-10 7:29 ` Viresh Kumar @ 2019-10-10 7:29 ` Viresh Kumar 0 siblings, 0 replies; 43+ messages in thread From: Viresh Kumar @ 2019-10-10 7:29 UTC (permalink / raw) To: Mark Brown Cc: Marek Szyprowski, Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Kamil Konieczny On 09-10-19, 15:13, Mark Brown wrote: > On Wed, Oct 09, 2019 at 12:29:00PM +0200, Marek Szyprowski wrote: > > > Okay, then what is the conclusion, as I got lost a bit? How do you want > > this issue to be fixed? > > We should revert the enable call, it shouldn't be required, and ideally > the default balancer could be updated to only make configuration changes > if they're actually required which would help avoid triggering any such > things in future if we don't absolutely have to. Sorry for the delay in responding, just came back after vacations. Should the OPP change be reverted ? Someone going to send that revert to me with the required explanation ? -- viresh ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-09 14:13 ` Mark Brown 2019-10-09 14:13 ` Mark Brown 2019-10-10 7:29 ` Viresh Kumar @ 2019-10-10 10:19 ` Marek Szyprowski 2019-10-10 10:19 ` Marek Szyprowski 2019-10-10 13:55 ` Mark Brown 2 siblings, 2 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-10 10:19 UTC (permalink / raw) To: Mark Brown Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 09.10.2019 16:13, Mark Brown wrote: > On Wed, Oct 09, 2019 at 12:29:00PM +0200, Marek Szyprowski wrote: > >> Okay, then what is the conclusion, as I got lost a bit? How do you want >> this issue to be fixed? > We should revert the enable call, it shouldn't be required, and ideally > the default balancer could be updated to only make configuration changes > if they're actually required which would help avoid triggering any such > things in future if we don't absolutely have to. Okay, Then in case of regulator core - do you accept the initial patch as it indeed forces the default balancer to avoid unnecessary changes, or do you want me to rewrite it to assume min_uV = current_uV for the already enabled regulators during the initial balancing, like suggested by Dmitry? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-10 10:19 ` Marek Szyprowski @ 2019-10-10 10:19 ` Marek Szyprowski 2019-10-10 13:55 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-10 10:19 UTC (permalink / raw) To: Mark Brown Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 09.10.2019 16:13, Mark Brown wrote: > On Wed, Oct 09, 2019 at 12:29:00PM +0200, Marek Szyprowski wrote: > >> Okay, then what is the conclusion, as I got lost a bit? How do you want >> this issue to be fixed? > We should revert the enable call, it shouldn't be required, and ideally > the default balancer could be updated to only make configuration changes > if they're actually required which would help avoid triggering any such > things in future if we don't absolutely have to. Okay, Then in case of regulator core - do you accept the initial patch as it indeed forces the default balancer to avoid unnecessary changes, or do you want me to rewrite it to assume min_uV = current_uV for the already enabled regulators during the initial balancing, like suggested by Dmitry? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-10 10:19 ` Marek Szyprowski 2019-10-10 10:19 ` Marek Szyprowski @ 2019-10-10 13:55 ` Mark Brown 2019-10-10 13:55 ` Mark Brown 2019-10-17 10:29 ` Marek Szyprowski 1 sibling, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-10 13:55 UTC (permalink / raw) To: Marek Szyprowski Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 767 bytes --] On Thu, Oct 10, 2019 at 12:19:55PM +0200, Marek Szyprowski wrote: > On 09.10.2019 16:13, Mark Brown wrote: > > We should revert the enable call, it shouldn't be required, and ideally > > the default balancer could be updated to only make configuration changes > > if they're actually required which would help avoid triggering any such > > things in future if we don't absolutely have to. > Okay, Then in case of regulator core - do you accept the initial patch > as it indeed forces the default balancer to avoid unnecessary changes, > or do you want me to rewrite it to assume min_uV = current_uV for the > already enabled regulators during the initial balancing, like suggested > by Dmitry? Neither, I'm suggesting you make the change above. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-10 13:55 ` Mark Brown @ 2019-10-10 13:55 ` Mark Brown 2019-10-17 10:29 ` Marek Szyprowski 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-10 13:55 UTC (permalink / raw) To: Marek Szyprowski Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 767 bytes --] On Thu, Oct 10, 2019 at 12:19:55PM +0200, Marek Szyprowski wrote: > On 09.10.2019 16:13, Mark Brown wrote: > > We should revert the enable call, it shouldn't be required, and ideally > > the default balancer could be updated to only make configuration changes > > if they're actually required which would help avoid triggering any such > > things in future if we don't absolutely have to. > Okay, Then in case of regulator core - do you accept the initial patch > as it indeed forces the default balancer to avoid unnecessary changes, > or do you want me to rewrite it to assume min_uV = current_uV for the > already enabled regulators during the initial balancing, like suggested > by Dmitry? Neither, I'm suggesting you make the change above. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-10 13:55 ` Mark Brown 2019-10-10 13:55 ` Mark Brown @ 2019-10-17 10:29 ` Marek Szyprowski 1 sibling, 0 replies; 43+ messages in thread From: Marek Szyprowski @ 2019-10-17 10:29 UTC (permalink / raw) To: Mark Brown Cc: Dmitry Osipenko, Bartlomiej Zolnierkiewicz, linux-kernel, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny Hi Mark, On 10.10.2019 15:55, Mark Brown wrote: > On Thu, Oct 10, 2019 at 12:19:55PM +0200, Marek Szyprowski wrote: >> On 09.10.2019 16:13, Mark Brown wrote: >>> We should revert the enable call, it shouldn't be required, and ideally >>> the default balancer could be updated to only make configuration changes >>> if they're actually required which would help avoid triggering any such >>> things in future if we don't absolutely have to. >> Okay, Then in case of regulator core - do you accept the initial patch >> as it indeed forces the default balancer to avoid unnecessary changes, >> or do you want me to rewrite it to assume min_uV = current_uV for the >> already enabled regulators during the initial balancing, like suggested >> by Dmitry? > Neither, I'm suggesting you make the change above. I've posted a revert: https://lkml.org/lkml/2019/10/17/267 Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 15:02 ` Dmitry Osipenko @ 2019-10-08 15:48 ` Mark Brown 2019-10-08 15:48 ` Mark Brown 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2 siblings, 2 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 15:48 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Marek Szyprowski, linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 1038 bytes --] On Tue, Oct 08, 2019 at 03:24:17PM +0200, Bartlomiej Zolnierkiewicz wrote: > Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") > currently can be safely reverted as all affected users use always-on > regulators. However IMHO it should be possible to enable always-on > regulator without side-effects. With coupled regulators you might have something kicking in because a change was made on a completely different regulator... If we don't take account of coupling requirements we'd doubtless have issues with that at some point. > When it comes to setting regulator constraints before doing enable > operation, it also seems to be possible solution but would require > splitting regulator_set_voltage() operation on two functions: > - one for setting constraints (before regulator_enable() operation) > - the other one actually setting voltage (after enable operation) I don't follow? What would a "constraint" be in this context and how would it be different to the voltage range you'd set in normal operation? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 15:48 ` Mark Brown @ 2019-10-08 15:48 ` Mark Brown 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 1 sibling, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 15:48 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Marek Szyprowski, linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 1038 bytes --] On Tue, Oct 08, 2019 at 03:24:17PM +0200, Bartlomiej Zolnierkiewicz wrote: > Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") > currently can be safely reverted as all affected users use always-on > regulators. However IMHO it should be possible to enable always-on > regulator without side-effects. With coupled regulators you might have something kicking in because a change was made on a completely different regulator... If we don't take account of coupling requirements we'd doubtless have issues with that at some point. > When it comes to setting regulator constraints before doing enable > operation, it also seems to be possible solution but would require > splitting regulator_set_voltage() operation on two functions: > - one for setting constraints (before regulator_enable() operation) > - the other one actually setting voltage (after enable operation) I don't follow? What would a "constraint" be in this context and how would it be different to the voltage range you'd set in normal operation? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 15:48 ` Mark Brown 2019-10-08 15:48 ` Mark Brown @ 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2019-10-08 16:21 ` Mark Brown 1 sibling, 2 replies; 43+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2019-10-08 16:02 UTC (permalink / raw) To: Mark Brown Cc: Marek Szyprowski, linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny On 10/8/19 5:48 PM, Mark Brown wrote: > On Tue, Oct 08, 2019 at 03:24:17PM +0200, Bartlomiej Zolnierkiewicz wrote: > >> Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") >> currently can be safely reverted as all affected users use always-on >> regulators. However IMHO it should be possible to enable always-on >> regulator without side-effects. > > With coupled regulators you might have something kicking in because a > change was made on a completely different regulator... If we don't take > account of coupling requirements we'd doubtless have issues with that at > some point. OK, I have not considered this. >> When it comes to setting regulator constraints before doing enable >> operation, it also seems to be possible solution but would require >> splitting regulator_set_voltage() operation on two functions: > >> - one for setting constraints (before regulator_enable() operation) > >> - the other one actually setting voltage (after enable operation) > > I don't follow? What would a "constraint" be in this context and how > would it be different to the voltage range you'd set in normal operation? The constraint here would be just the voltage range. I just wanted to point out that the actual voltage set operation (on the hardware itself not the internal subsystem bookkeeping) shouldn't be done before enable operation (especially in context of non-coupled regulators). Taking into account your remark about enable operation on coupled regulators and Dmitry's mail about cpufreq issue I think now that just dropping opp change is the most straightforward fix. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz @ 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2019-10-08 16:21 ` Mark Brown 1 sibling, 0 replies; 43+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2019-10-08 16:02 UTC (permalink / raw) To: Mark Brown Cc: Marek Szyprowski, linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny On 10/8/19 5:48 PM, Mark Brown wrote: > On Tue, Oct 08, 2019 at 03:24:17PM +0200, Bartlomiej Zolnierkiewicz wrote: > >> Commit 7f93ff73f7c8 ("opp: core: add regulators enable and disable") >> currently can be safely reverted as all affected users use always-on >> regulators. However IMHO it should be possible to enable always-on >> regulator without side-effects. > > With coupled regulators you might have something kicking in because a > change was made on a completely different regulator... If we don't take > account of coupling requirements we'd doubtless have issues with that at > some point. OK, I have not considered this. >> When it comes to setting regulator constraints before doing enable >> operation, it also seems to be possible solution but would require >> splitting regulator_set_voltage() operation on two functions: > >> - one for setting constraints (before regulator_enable() operation) > >> - the other one actually setting voltage (after enable operation) > > I don't follow? What would a "constraint" be in this context and how > would it be different to the voltage range you'd set in normal operation? The constraint here would be just the voltage range. I just wanted to point out that the actual voltage set operation (on the hardware itself not the internal subsystem bookkeeping) shouldn't be done before enable operation (especially in context of non-coupled regulators). Taking into account your remark about enable operation on coupled regulators and Dmitry's mail about cpufreq issue I think now that just dropping opp change is the most straightforward fix. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz @ 2019-10-08 16:21 ` Mark Brown 2019-10-08 16:21 ` Mark Brown 1 sibling, 1 reply; 43+ messages in thread From: Mark Brown @ 2019-10-08 16:21 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Marek Szyprowski, linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 478 bytes --] On Tue, Oct 08, 2019 at 06:02:08PM +0200, Bartlomiej Zolnierkiewicz wrote: > Taking into account your remark about enable operation on coupled > regulators and Dmitry's mail about cpufreq issue I think now that just > dropping opp change is the most straightforward fix. It's certainly the most straightforward thing for the immediate problem. I do think we probably need to improve how we're handling the coupling though, we've got some fragility here that needs addressing. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() 2019-10-08 16:21 ` Mark Brown @ 2019-10-08 16:21 ` Mark Brown 0 siblings, 0 replies; 43+ messages in thread From: Mark Brown @ 2019-10-08 16:21 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Marek Szyprowski, linux-kernel, Dmitry Osipenko, Liam Girdwood, Lucas Stach, Krzysztof Kozlowski, linux-samsung-soc, Viresh Kumar, Kamil Konieczny [-- Attachment #1: Type: text/plain, Size: 478 bytes --] On Tue, Oct 08, 2019 at 06:02:08PM +0200, Bartlomiej Zolnierkiewicz wrote: > Taking into account your remark about enable operation on coupled > regulators and Dmitry's mail about cpufreq issue I think now that just > dropping opp change is the most straightforward fix. It's certainly the most straightforward thing for the immediate problem. I do think we probably need to improve how we're handling the coupling though, we've got some fragility here that needs addressing. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2019-10-17 10:29 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20191008101720eucas1p2e0d1bca6e696848bf689067e05620679@eucas1p2.samsung.com> 2019-10-08 10:17 ` [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator_enable() Marek Szyprowski 2019-10-08 10:17 ` Marek Szyprowski 2019-10-08 11:50 ` Mark Brown 2019-10-08 11:50 ` Mark Brown 2019-10-08 12:01 ` Marek Szyprowski 2019-10-08 12:01 ` Marek Szyprowski 2019-10-08 12:06 ` Mark Brown 2019-10-08 12:06 ` Mark Brown 2019-10-08 12:38 ` Marek Szyprowski 2019-10-08 12:38 ` Marek Szyprowski 2019-10-08 12:47 ` Mark Brown 2019-10-08 12:47 ` Mark Brown 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 13:24 ` Bartlomiej Zolnierkiewicz 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 15:02 ` Dmitry Osipenko 2019-10-08 16:15 ` Mark Brown 2019-10-08 16:15 ` Mark Brown 2019-10-08 17:05 ` Dmitry Osipenko 2019-10-08 17:05 ` Dmitry Osipenko 2019-10-08 17:17 ` Mark Brown 2019-10-08 17:17 ` Mark Brown 2019-10-08 18:00 ` Dmitry Osipenko 2019-10-08 18:00 ` Dmitry Osipenko 2019-10-08 18:07 ` Mark Brown 2019-10-08 18:07 ` Mark Brown 2019-10-09 10:29 ` Marek Szyprowski 2019-10-09 10:29 ` Marek Szyprowski 2019-10-09 14:13 ` Mark Brown 2019-10-09 14:13 ` Mark Brown 2019-10-10 7:29 ` Viresh Kumar 2019-10-10 7:29 ` Viresh Kumar 2019-10-10 10:19 ` Marek Szyprowski 2019-10-10 10:19 ` Marek Szyprowski 2019-10-10 13:55 ` Mark Brown 2019-10-10 13:55 ` Mark Brown 2019-10-17 10:29 ` Marek Szyprowski 2019-10-08 15:48 ` Mark Brown 2019-10-08 15:48 ` Mark Brown 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2019-10-08 16:02 ` Bartlomiej Zolnierkiewicz 2019-10-08 16:21 ` Mark Brown 2019-10-08 16:21 ` Mark Brown
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).