At the moment unused regulators managed by the RPM firmware on Qualcomm
platforms stay on forever if they are already on during boot. This is
because we have no way of checking if they are really on or not.
Fix this by sending an explicit disable request for all unused
regulators managed by the qcom_smd-regulator driver.
I'm sending this as RFC mainly for the change in the regulator core.
There is also a slight chance of breakage for incomplete device trees
that mistakenly rely on having unused regulators always-on.
Signed-off-by: Stephan Gerhold <[email protected]>
---
Stephan Gerhold (2):
regulator: core: Disable unused regulators with unknown status
regulator: qcom_smd: Disable unused regulators
drivers/regulator/core.c | 9 +++++++--
drivers/regulator/qcom_smd-regulator.c | 5 +++--
2 files changed, 10 insertions(+), 4 deletions(-)
---
base-commit: f9a1d31874c383f58bb4f89bfe79b764682cd026
change-id: 20231004-reg-smd-unused-95bcf0d586fb
Best regards,
--
Stephan Gerhold <[email protected]>
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth
The RPM firmware on Qualcomm platforms does not provide a way to check
if a regulator is on during boot using the SMD interface. If the
regulators are already on during boot and Linux does not make use of
them they will currently stay enabled forever. The regulator core does
not know these regulators are on and cannot clean them up together with
the other unused regulators.
Fix this by setting the initial enable state to -EINVAL similar to
qcom-rpmh-regulator.c. The regulator core will then also explicitly
disable all unused regulators with unknown status.
Signed-off-by: Stephan Gerhold <[email protected]>
---
NOTE: This has a slight potential of breaking boards that rely on having
unused regulators permanently enabled (without regulator-always-on).
However, this is always a mistake in the device tree so it's probably
better to risk some breakage now, add the missing regulators and avoid
this problem for all future boards.
---
drivers/regulator/qcom_smd-regulator.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index f53ada076252..0bbfba2e17ff 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
reqlen++;
}
- if (vreg->uv_updated && vreg->is_enabled) {
+ if (vreg->uv_updated && vreg->is_enabled > 0) {
req[reqlen].key = cpu_to_le32(RPM_KEY_UV);
req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
req[reqlen].value = cpu_to_le32(vreg->uV);
reqlen++;
}
- if (vreg->load_updated && vreg->is_enabled) {
+ if (vreg->load_updated && vreg->is_enabled > 0) {
req[reqlen].key = cpu_to_le32(RPM_KEY_MA);
req[reqlen].nbytes = cpu_to_le32(sizeof(u32));
req[reqlen].value = cpu_to_le32(vreg->load / 1000);
@@ -1377,6 +1377,7 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
vreg->rpm = rpm;
vreg->type = rpm_data->type;
vreg->id = rpm_data->id;
+ vreg->is_enabled = -EINVAL;
memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
vreg->desc.name = rpm_data->name;
--
2.39.2
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 <[email protected]>
---
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(-)
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()) {
--
2.39.2
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 <[email protected]>
> ---
> 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 4.10.2023 16:17, Stephan Gerhold wrote:
> The RPM firmware on Qualcomm platforms does not provide a way to check
> if a regulator is on during boot using the SMD interface. If the
> regulators are already on during boot and Linux does not make use of
> them they will currently stay enabled forever. The regulator core does
> not know these regulators are on and cannot clean them up together with
> the other unused regulators.
>
> Fix this by setting the initial enable state to -EINVAL similar to
> qcom-rpmh-regulator.c. The regulator core will then also explicitly
> disable all unused regulators with unknown status.
>
> Signed-off-by: Stephan Gerhold <[email protected]>
> ---
> NOTE: This has a slight potential of breaking boards that rely on having
> unused regulators permanently enabled (without regulator-always-on).
> However, this is always a mistake in the device tree so it's probably
> better to risk some breakage now, add the missing regulators and avoid
> this problem for all future boards.
> ---
> drivers/regulator/qcom_smd-regulator.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index f53ada076252..0bbfba2e17ff 100644
> --- a/drivers/regulator/qcom_smd-regulator.c
> +++ b/drivers/regulator/qcom_smd-regulator.c
> @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
> reqlen++;
> }
>
> - if (vreg->uv_updated && vreg->is_enabled) {
> + if (vreg->uv_updated && vreg->is_enabled > 0) {
At a quick glance, are there any states for this value, other
than 0 and 1? This is not the regulator_ops->is_enabled, but
qcom_rpm_reg->is_enabled.
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 <[email protected]>
> > ---
> > 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 Fri, Oct 06, 2023 at 11:15:40PM +0200, Konrad Dybcio wrote:
> On 4.10.2023 16:17, Stephan Gerhold wrote:
> > The RPM firmware on Qualcomm platforms does not provide a way to check
> > if a regulator is on during boot using the SMD interface. If the
> > regulators are already on during boot and Linux does not make use of
> > them they will currently stay enabled forever. The regulator core does
> > not know these regulators are on and cannot clean them up together with
> > the other unused regulators.
> >
> > Fix this by setting the initial enable state to -EINVAL similar to
> > qcom-rpmh-regulator.c. The regulator core will then also explicitly
> > disable all unused regulators with unknown status.
> >
> > Signed-off-by: Stephan Gerhold <[email protected]>
> > ---
> > NOTE: This has a slight potential of breaking boards that rely on having
> > unused regulators permanently enabled (without regulator-always-on).
> > However, this is always a mistake in the device tree so it's probably
> > better to risk some breakage now, add the missing regulators and avoid
> > this problem for all future boards.
> > ---
> > drivers/regulator/qcom_smd-regulator.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> > index f53ada076252..0bbfba2e17ff 100644
> > --- a/drivers/regulator/qcom_smd-regulator.c
> > +++ b/drivers/regulator/qcom_smd-regulator.c
> > @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
> > reqlen++;
> > }
> >
> > - if (vreg->uv_updated && vreg->is_enabled) {
> > + if (vreg->uv_updated && vreg->is_enabled > 0) {
> At a quick glance, are there any states for this value, other
> than 0 and 1? This is not the regulator_ops->is_enabled, but
> qcom_rpm_reg->is_enabled.
>
Yes, I initially assign vreg->is_enabled = -EINVAL (for use with PATCH
1/2). It's in the part of the patch that you trimmed in your reply. :D
Thanks,
Stephan
@@ -1377,6 +1377,7 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
vreg->rpm = rpm;
vreg->type = rpm_data->type;
vreg->id = rpm_data->id;
+ vreg->is_enabled = -EINVAL;
memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
vreg->desc.name = rpm_data->name;
On 10/9/23 22:23, Stephan Gerhold wrote:
> On Fri, Oct 06, 2023 at 11:15:40PM +0200, Konrad Dybcio wrote:
>> On 4.10.2023 16:17, Stephan Gerhold wrote:
>>> The RPM firmware on Qualcomm platforms does not provide a way to check
>>> if a regulator is on during boot using the SMD interface. If the
>>> regulators are already on during boot and Linux does not make use of
>>> them they will currently stay enabled forever. The regulator core does
>>> not know these regulators are on and cannot clean them up together with
>>> the other unused regulators.
>>>
>>> Fix this by setting the initial enable state to -EINVAL similar to
>>> qcom-rpmh-regulator.c. The regulator core will then also explicitly
>>> disable all unused regulators with unknown status.
>>>
>>> Signed-off-by: Stephan Gerhold <[email protected]>
>>> ---
>>> NOTE: This has a slight potential of breaking boards that rely on having
>>> unused regulators permanently enabled (without regulator-always-on).
>>> However, this is always a mistake in the device tree so it's probably
>>> better to risk some breakage now, add the missing regulators and avoid
>>> this problem for all future boards.
>>> ---
>>> drivers/regulator/qcom_smd-regulator.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
>>> index f53ada076252..0bbfba2e17ff 100644
>>> --- a/drivers/regulator/qcom_smd-regulator.c
>>> +++ b/drivers/regulator/qcom_smd-regulator.c
>>> @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
>>> reqlen++;
>>> }
>>>
>>> - if (vreg->uv_updated && vreg->is_enabled) {
>>> + if (vreg->uv_updated && vreg->is_enabled > 0) {
>> At a quick glance, are there any states for this value, other
>> than 0 and 1? This is not the regulator_ops->is_enabled, but
>> qcom_rpm_reg->is_enabled.
>>
>
> Yes, I initially assign vreg->is_enabled = -EINVAL (for use with PATCH
> 1/2). It's in the part of the patch that you trimmed in your reply. :D
>
> Thanks,
> Stephan
Oh, right ^^
Konrad
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 <[email protected]>
>>> ---
>>> 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,
--
Stephan Gerhold <[email protected]>
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth
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.