[RFC,1/2] regulator: core: Disable unused regulators with unknown status

  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(-)
  

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"?

I think this looks really good.. And will definitely help finding
power hogs!

At the cost of breaking booting with broken DTs. But as the name by
which I referred to them suggests, this was never really destined to
work..

Konrad
  

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.