| Message ID | 20231004-reg-smd-unused-v1-1-5d682493d555@kernkonzept.com |
|---|---|
| State | New |
| Headers | show |
| Series | regulator: qcom_smd: Disable unused regulators | expand |
On Fri, Oct 06, 2023 at 11:11:48PM +0200, Konrad Dybcio wrote: > On 4.10.2023 16:17, Stephan Gerhold wrote: > > Some regulator drivers do not provide a way to check if the regulator is > > currently enabled or not. That does not necessarily mean that the > > regulator is always-on. For example, the regulators managed by the RPM > > firmware on Qualcomm platforms can be either on or off during boot but > > the initial state is not known. To sync the state the regulator should > > get either explicitly enabled or explicitly disabled. > > > > Enabling all regulators unconditionally is not safe, because we might > > not know which voltages are safe. The devices supplied by those > > regulators might also require a special power-up sequence where the > > regulators are turned on in a certain order or with specific delay. > > > > Disabling all unused regulators is safer. If the regulator is already > > off it will just stay that way. If the regulator is on, disabling it > > explicitly allows the firmware to turn it off for reduced power > > consumption. > > > > The regulator core already has functionality for disabling unused > > regulators. However, at the moment it assumes that all regulators where > > the .is_enabled() callback fails are actually off. There is no way to > > return a special value for the "unknown" state to explicitly ask for > > disabling those regulators. > > > > Some drivers (e.g. qcom-rpmh-regulator.c) return -EINVAL for the case > > where the initial status is unknown. Use that return code to assume the > > initial status is unknown and try to explicitly disable the regulator > > in that case. > > > > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > > --- > > Instead of -EINVAL we could also use a different return code to indicate > > the initial status is unknown. Or maybe there is some other option that > > would be easier? This is working for me but I'm sending it as RFC to get > > more feedback. :) > > -EOPNOTSUPP for "doesn't support getting is_enabled state"? > The way it is implemented right now the Qualcomm SMD RPM regulator does actually support getting the .is_enabled() state. It is only unable to determine the initial state during boot. Once the regulator has been enabled by some consumer for the first time the .is_enabled() callback starts returning the expected results. Typically -EOPNOTSUPP is used when the driver callback (or similar) is not implemented at all. I'm not sure if using -EOPNOTSUPP for the "temporarily unable to determine state" purpose would be misleading. Thanks, Stephan
On 10/9/23 22:21, Stephan Gerhold wrote: > On Fri, Oct 06, 2023 at 11:11:48PM +0200, Konrad Dybcio wrote: >> On 4.10.2023 16:17, Stephan Gerhold wrote: >>> Some regulator drivers do not provide a way to check if the regulator is >>> currently enabled or not. That does not necessarily mean that the >>> regulator is always-on. For example, the regulators managed by the RPM >>> firmware on Qualcomm platforms can be either on or off during boot but >>> the initial state is not known. To sync the state the regulator should >>> get either explicitly enabled or explicitly disabled. >>> >>> Enabling all regulators unconditionally is not safe, because we might >>> not know which voltages are safe. The devices supplied by those >>> regulators might also require a special power-up sequence where the >>> regulators are turned on in a certain order or with specific delay. >>> >>> Disabling all unused regulators is safer. If the regulator is already >>> off it will just stay that way. If the regulator is on, disabling it >>> explicitly allows the firmware to turn it off for reduced power >>> consumption. >>> >>> The regulator core already has functionality for disabling unused >>> regulators. However, at the moment it assumes that all regulators where >>> the .is_enabled() callback fails are actually off. There is no way to >>> return a special value for the "unknown" state to explicitly ask for >>> disabling those regulators. >>> >>> Some drivers (e.g. qcom-rpmh-regulator.c) return -EINVAL for the case >>> where the initial status is unknown. Use that return code to assume the >>> initial status is unknown and try to explicitly disable the regulator >>> in that case. >>> >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> >>> --- >>> Instead of -EINVAL we could also use a different return code to indicate >>> the initial status is unknown. Or maybe there is some other option that >>> would be easier? This is working for me but I'm sending it as RFC to get >>> more feedback. :) >> >> -EOPNOTSUPP for "doesn't support getting is_enabled state"? >> > > The way it is implemented right now the Qualcomm SMD RPM regulator does > actually support getting the .is_enabled() state. It is only unable to > determine the initial state during boot. Once the regulator has been > enabled by some consumer for the first time the .is_enabled() callback > starts returning the expected results. > > Typically -EOPNOTSUPP is used when the driver callback (or similar) is > not implemented at all. I'm not sure if using -EOPNOTSUPP for the > "temporarily unable to determine state" purpose would be misleading. I'd say EOPNOTSUPP is fair here because calling is_enabled in that context is not supported, but I guess it's up to Mark. Konrad
On Wed, Oct 04, 2023 at 04:17:17PM +0200, Stephan Gerhold wrote: > Instead of -EINVAL we could also use a different return code to indicate > the initial status is unknown. Or maybe there is some other option that > would be easier? This is working for me but I'm sending it as RFC to get > more feedback. :) The more normal thing here would be -EBUSY I think - -EINVAL kind of indicates that the operation will never work while in reality it could possibly work in future. Though for the RPMH it's not really the case that it ever supports readback, what it does is have it's own reference counting in the driver. Rather than doing this we should probably have logic in the core which sees that the driver has a write operation but no read operation and implements appropriate behaviour.
On Mon, Oct 23, 2023 at 01:09:11PM +0100, Mark Brown wrote: > On Wed, Oct 04, 2023 at 04:17:17PM +0200, Stephan Gerhold wrote: > > > Instead of -EINVAL we could also use a different return code to indicate > > the initial status is unknown. Or maybe there is some other option that > > would be easier? This is working for me but I'm sending it as RFC to get > > more feedback. :) > > The more normal thing here would be -EBUSY I think - -EINVAL kind of > indicates that the operation will never work while in reality it could > possibly work in future. Though for the RPMH it's not really the case > that it ever supports readback, what it does is have it's own reference > counting in the driver. Rather than doing this we should probably have > logic in the core which sees that the driver has a write operation but > no read operation and implements appropriate behaviour. I like the suggestion to not implement is_enabled, and handle that in the core instead, for all three generations of our rpm-based regulators. Regards, Bjorn
On Mon, Oct 23, 2023 at 01:09:11PM +0100, Mark Brown wrote: > On Wed, Oct 04, 2023 at 04:17:17PM +0200, Stephan Gerhold wrote: > > > Instead of -EINVAL we could also use a different return code to indicate > > the initial status is unknown. Or maybe there is some other option that > > would be easier? This is working for me but I'm sending it as RFC to get > > more feedback. :) > > The more normal thing here would be -EBUSY I think - -EINVAL kind of > indicates that the operation will never work while in reality it could > possibly work in future. Though for the RPMH it's not really the case > that it ever supports readback, what it does is have it's own reference > counting in the driver. Rather than doing this we should probably have > logic in the core which sees that the driver has a write operation but > no read operation and implements appropriate behaviour. Yep, I agree that it would be nicer to handle this case in the core, rather than duplicating the logic in all the RPM-related drivers. I think it does not change much for this patch, though. Even when implemented in the core we still need to represent this situation somehow for regulator_is_enabled(). Simply returning 0 (disabled) or 1 (enabled) would be wrong. Do you think returning -EBUSY would be appropriate for that? The second challenge I see on a quick look is that both qcom_smd-regulator.c and qcom-rpmh-regulator.c use their reference counter internally in other function (e.g. to decide if a voltage change should be sent, see "vreg->enabled" checks). I think we would also need to add some rdev_is_enabled() function that would expose the core reference counter to the driver? Tracking the enable state in the driver (the way it is right now) is not that much code, so I'm not entirely sure if we might actually end up with more code/complexity when moving this to the core. Thanks,
On Tue, Oct 24, 2023 at 10:57:38AM +0200, Stephan Gerhold wrote: > I think it does not change much for this patch, though. Even when > implemented in the core we still need to represent this situation > somehow for regulator_is_enabled(). Simply returning 0 (disabled) or > 1 (enabled) would be wrong. Do you think returning -EBUSY would be > appropriate for that? In these cases where we simply can't read the expectation is that we'll always be using the logical state - one way of thinking about it is that the operation is mostly a bootstrapping helper to figure out what the initial state is. A quick survey of users suggest they'll pretty much all be buggy if we start returning errors, and I frankly even if all the current users were fixed I'd expect that to continue to be a common error. I suppose that the effect of ignoring the possibility of error is like the current behaviour though. > The second challenge I see on a quick look is that both > qcom_smd-regulator.c and qcom-rpmh-regulator.c use their reference > counter internally in other function (e.g. to decide if a voltage change > should be sent, see "vreg->enabled" checks). I think we would also need > to add some rdev_is_enabled() function that would expose the core > reference counter to the driver? > Tracking the enable state in the driver (the way it is right now) is not > that much code, so I'm not entirely sure if we might actually end up > with more code/complexity when moving this to the core. We have to do the reference count in the core anyway since it's a reference count not just a simple on/off so it doesn't really cost us anything to make it available to drivers.
On Wed, Oct 25, 2023 at 06:49:47PM +0100, Mark Brown wrote: > On Tue, Oct 24, 2023 at 10:57:38AM +0200, Stephan Gerhold wrote: > > > I think it does not change much for this patch, though. Even when > > implemented in the core we still need to represent this situation > > somehow for regulator_is_enabled(). Simply returning 0 (disabled) or > > 1 (enabled) would be wrong. Do you think returning -EBUSY would be > > appropriate for that? > > In these cases where we simply can't read the expectation is that we'll > always be using the logical state - one way of thinking about it is that > the operation is mostly a bootstrapping helper to figure out what the > initial state is. A quick survey of users suggest they'll pretty much > all be buggy if we start returning errors, and I frankly even if all the > current users were fixed I'd expect that to continue to be a common > error. I suppose that the effect of ignoring the possibility of error > is like the current behaviour though. > regulator_is_enabled() already returns error codes in various cases, e.g. regulator_is_enabled_regmap() returns the original error code from the regmap_read() call if that fails. So if users ignore that and interpret the value as logical one they either don't care (which is probably fine in some cases?) or already use it wrong. Or am I missing something? > > qcom_smd-regulator.c and qcom-rpmh-regulator.c use their reference > > counter internally in other function (e.g. to decide if a voltage change > > should be sent, see "vreg->enabled" checks). I think we would also need > > to add some rdev_is_enabled() function that would expose the core > > reference counter to the driver? > > > Tracking the enable state in the driver (the way it is right now) is not > > that much code, so I'm not entirely sure if we might actually end up > > with more code/complexity when moving this to the core. > > We have to do the reference count in the core anyway since it's a > reference count not just a simple on/off so it doesn't really cost us > anything to make it available to drivers. I assume you're referring to "use_count" as the reference counter? On a closer look I think it cannot be used as-is for my purpose: 1. With "regulator-boot-on", set_machine_constraints() explicitly enables the regulator, but doesn't increase the use_count. In that case we should return true in ->is_enabled(). I'm not sure how we would know, just based on use_count = 0. 2. To cleanup unused regulators that may or may not be enabled we need to know if the regulator was ever explicitly enabled/disabled before. It's pointless to send a disable request for a regulator that we already disabled explicitly before (after a enable -> disable cycle). use_count just tells us if there is currently a user, but not if there was one before. I think I would literally need to move the existing "enabled" field from the RPM regulator drivers to the core and manage it similarly there based on ->enable() and ->disable() calls. Which would be a (slight) overhead for all regulators rather than being isolated for the few RPM regulator drivers. Thanks, Stephan
On Wed, Oct 25, 2023 at 09:51:51PM +0200, Stephan Gerhold wrote: > On Wed, Oct 25, 2023 at 06:49:47PM +0100, Mark Brown wrote: > > In these cases where we simply can't read the expectation is that we'll > > always be using the logical state - one way of thinking about it is that > > the operation is mostly a bootstrapping helper to figure out what the > > initial state is. A quick survey of users suggest they'll pretty much > > all be buggy if we start returning errors, and I frankly even if all the > > current users were fixed I'd expect that to continue to be a common > > error. I suppose that the effect of ignoring the possibility of error > > is like the current behaviour though. > regulator_is_enabled() already returns error codes in various cases, > e.g. regulator_is_enabled_regmap() returns the original error code from > the regmap_read() call if that fails. So if users ignore that and > interpret the value as logical one they either don't care (which is > probably fine in some cases?) or already use it wrong. Or am I missing > something? That's broadly what I just indicated. Expecting anybody to do anything useful with an error report is probably optimistic, but it's probably going to give the same behaviour as we have currently so it's probably fine. > > We have to do the reference count in the core anyway since it's a > > reference count not just a simple on/off so it doesn't really cost us > > anything to make it available to drivers. > I assume you're referring to "use_count" as the reference counter? Yes. > On a closer look I think it cannot be used as-is for my purpose: > 1. With "regulator-boot-on", set_machine_constraints() explicitly > enables the regulator, but doesn't increase the use_count. > In that case we should return true in ->is_enabled(). I'm not sure > how we would know, just based on use_count = 0. OK, so use_count plus other information we also already have to hand. Or OTOH it's not that much overhead to track the enable state explicitly for hardware without readback as you're suggesting below if it ends up being too much hassle. > 2. To cleanup unused regulators that may or may not be enabled we need > to know if the regulator was ever explicitly enabled/disabled before. > It's pointless to send a disable request for a regulator that we > already disabled explicitly before (after a enable -> disable cycle). > use_count just tells us if there is currently a user, but not if > there was one before. It's pointless, but equally well it's not huge overhead. > I think I would literally need to move the existing "enabled" field from > the RPM regulator drivers to the core and manage it similarly there > based on ->enable() and ->disable() calls. Which would be a (slight) > overhead for all regulators rather than being isolated for the few RPM > regulator drivers. These aren't the only regulators with this limitation, we've also got similar open coding for GPIO controlled regulators like the fixed regualtor for example.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 3137e40fcd3e..182e3727651a 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -6207,8 +6207,13 @@ static int regulator_late_cleanup(struct device *dev, void *data) if (rdev->use_count) goto unlock; - /* If reading the status failed, assume that it's off. */ - if (_regulator_is_enabled(rdev) <= 0) + /* + * If reading the status failed, assume that it's off. + * If the current status is unknown (-EINVAL), assume that the + * regulator might be on and try to explicitly disable it. + */ + ret = _regulator_is_enabled(rdev); + if (ret <= 0 && ret != -EINVAL) goto unlock; if (have_full_constraints()) {
Some regulator drivers do not provide a way to check if the regulator is currently enabled or not. That does not necessarily mean that the regulator is always-on. For example, the regulators managed by the RPM firmware on Qualcomm platforms can be either on or off during boot but the initial state is not known. To sync the state the regulator should get either explicitly enabled or explicitly disabled. Enabling all regulators unconditionally is not safe, because we might not know which voltages are safe. The devices supplied by those regulators might also require a special power-up sequence where the regulators are turned on in a certain order or with specific delay. Disabling all unused regulators is safer. If the regulator is already off it will just stay that way. If the regulator is on, disabling it explicitly allows the firmware to turn it off for reduced power consumption. The regulator core already has functionality for disabling unused regulators. However, at the moment it assumes that all regulators where the .is_enabled() callback fails are actually off. There is no way to return a special value for the "unknown" state to explicitly ask for disabling those regulators. Some drivers (e.g. qcom-rpmh-regulator.c) return -EINVAL for the case where the initial status is unknown. Use that return code to assume the initial status is unknown and try to explicitly disable the regulator in that case. Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> --- Instead of -EINVAL we could also use a different return code to indicate the initial status is unknown. Or maybe there is some other option that would be easier? This is working for me but I'm sending it as RFC to get more feedback. :) --- drivers/regulator/core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)