From: Benjamin Bara <[email protected]>
Similar to the existing implementation, the new function does not handle
EOPNOTSUPP as an error. The initial monitoring state is set to the
regulator state.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/core.c | 134 ++++++++++++++++++++++++++++-------------------
1 file changed, 80 insertions(+), 54 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index dc741ac156c3..76f112817f9d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1426,7 +1426,7 @@ static int notif_set_limit(struct regulator_dev *rdev,
static int handle_notify_limits(struct regulator_dev *rdev,
int (*set)(struct regulator_dev *, int, int, bool),
- struct notification_limit *limits)
+ const struct notification_limit *limits)
{
int ret = 0;
@@ -1451,6 +1451,80 @@ static int handle_notify_limits(struct regulator_dev *rdev,
return ret;
}
+
+static const struct notification_limit disable_limits = {
+ .prot = REGULATOR_NOTIF_LIMIT_DISABLE,
+ .err = REGULATOR_NOTIF_LIMIT_DISABLE,
+ .warn = REGULATOR_NOTIF_LIMIT_DISABLE,
+};
+
+static int monitors_set_state(struct regulator_dev *rdev, bool enable)
+{
+ const struct regulation_constraints *reg_c = rdev->constraints;
+ const struct regulator_ops *ops = rdev->desc->ops;
+ int ret;
+
+ /* only set the state if monitoring is activated in the device-tree. */
+ if (reg_c->over_voltage_detection) {
+ ret = handle_notify_limits(rdev, ops->set_over_voltage_protection,
+ enable ? ®_c->over_voltage_limits
+ : &disable_limits);
+ if (ret) {
+ if (ret != -EOPNOTSUPP) {
+ rdev_err(rdev, "failed to set over voltage limits %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+ rdev_warn(rdev,
+ "IC does not support requested over voltage limits\n");
+ }
+ }
+ if (reg_c->under_voltage_detection) {
+ ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
+ enable ? ®_c->under_voltage_limits
+ : &disable_limits);
+ if (ret) {
+ if (ret != -EOPNOTSUPP) {
+ rdev_err(rdev, "failed to set under voltage limits %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+ rdev_warn(rdev,
+ "IC does not support requested under voltage limits\n");
+ }
+ }
+ if (reg_c->over_current_detection) {
+ ret = handle_notify_limits(rdev, ops->set_over_current_protection,
+ enable ? ®_c->over_curr_limits
+ : &disable_limits);
+ if (ret) {
+ if (ret != -EOPNOTSUPP) {
+ rdev_err(rdev, "failed to set over current limits: %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+ rdev_warn(rdev,
+ "IC does not support requested over-current limits\n");
+ }
+ }
+ if (reg_c->over_temp_detection) {
+ ret = handle_notify_limits(rdev, ops->set_thermal_protection,
+ enable ? ®_c->temp_limits
+ : &disable_limits);
+ if (ret) {
+ if (ret != -EOPNOTSUPP) {
+ rdev_err(rdev, "failed to set temperature limits %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+ rdev_warn(rdev,
+ "IC does not support requested temperature limits\n");
+ }
+ }
+
+ return 0;
+}
+
/**
* set_machine_constraints - sets regulator constraints
* @rdev: regulator source
@@ -1564,60 +1638,12 @@ static int set_machine_constraints(struct regulator_dev *rdev)
}
}
- if (rdev->constraints->over_current_detection)
- ret = handle_notify_limits(rdev,
- ops->set_over_current_protection,
- &rdev->constraints->over_curr_limits);
- if (ret) {
- if (ret != -EOPNOTSUPP) {
- rdev_err(rdev, "failed to set over current limits: %pe\n",
- ERR_PTR(ret));
- return ret;
- }
- rdev_warn(rdev,
- "IC does not support requested over-current limits\n");
- }
-
- if (rdev->constraints->over_voltage_detection)
- ret = handle_notify_limits(rdev,
- ops->set_over_voltage_protection,
- &rdev->constraints->over_voltage_limits);
- if (ret) {
- if (ret != -EOPNOTSUPP) {
- rdev_err(rdev, "failed to set over voltage limits %pe\n",
- ERR_PTR(ret));
- return ret;
- }
- rdev_warn(rdev,
- "IC does not support requested over voltage limits\n");
- }
-
- if (rdev->constraints->under_voltage_detection)
- ret = handle_notify_limits(rdev,
- ops->set_under_voltage_protection,
- &rdev->constraints->under_voltage_limits);
- if (ret) {
- if (ret != -EOPNOTSUPP) {
- rdev_err(rdev, "failed to set under voltage limits %pe\n",
- ERR_PTR(ret));
- return ret;
- }
- rdev_warn(rdev,
- "IC does not support requested under voltage limits\n");
- }
-
- if (rdev->constraints->over_temp_detection)
- ret = handle_notify_limits(rdev,
- ops->set_thermal_protection,
- &rdev->constraints->temp_limits);
- if (ret) {
- if (ret != -EOPNOTSUPP) {
- rdev_err(rdev, "failed to set temperature limits %pe\n",
- ERR_PTR(ret));
+ /* set initial monitor state to current regulator state. */
+ ret = _regulator_is_enabled(rdev);
+ if (ret >= 0) {
+ ret = monitors_set_state(rdev, !!ret);
+ if (ret)
return ret;
- }
- rdev_warn(rdev,
- "IC does not support requested temperature limits\n");
}
if (rdev->constraints->active_discharge && ops->set_active_discharge) {
--
2.34.1
Hi Benjamin,
Thanks for working on this. :)
On 5/21/23 14:39, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Similar to the existing implementation, the new function does not handle
> EOPNOTSUPP as an error. The initial monitoring state is set to the
> regulator state.
As far as I see, this changes the existing logic. Previously the
monitoring was unconditionally enabled for all regulators, now it gets
only enabled for regulators which are marked as enabled.
Furthermore, if I am not reading this wrong, the code tries to disable
all protections if regulator is not enabled at startup(?)
I am not saying this is wrong. I am just saying that things will change
here and likely to break something.
There are PMICs like ROHM BD9576, where the protection can not be disabled.
For example, the bd9576_set_uvp() has:
if (severity == REGULATOR_SEVERITY_PROT) {
if (!enable || lim_uV)
return -EINVAL;
return 0;
}
I am unsure if we might also have cases where some regulator could
really be enabled w/o core knowing it. There can also be a problem if we
have hardware where monitoring is common for all regulators, eg either
globally enabled / disabled.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Hi Matti,
thanks for the feedback!
On Tue, 23 May 2023 at 11:46, Matti Vaittinen <[email protected]> wrote:
> As far as I see, this changes the existing logic. Previously the
> monitoring was unconditionally enabled for all regulators, now it gets
> only enabled for regulators which are marked as enabled.
>
> Furthermore, if I am not reading this wrong, the code tries to disable
> all protections if regulator is not enabled at startup(?)
>
> I am not saying this is wrong. I am just saying that things will
> change here and likely to break something.
>
> There are PMICs like ROHM BD9576, where the protection can not be
> disabled.
Thanks for letting me know! I dropped my initial "disable monitor while
disabling the regulator" property, and activated it per default instead.
But this basically means something like that will be required. I guess
it might make sense to have a property which is called something like
"monitor always on", to let the driver inform the core that the monitors
cannot or should not be disabled, instead.
Except if you think there is a general problem with keeping monitors
disabled while the regulator is disabled, then I might have to do it
differently.
> I am unsure if we might also have cases where some regulator could
> really be enabled w/o core knowing it.
Unfortunately, I am not 100% sure what you mean by that.
On the da9063, for example, it might be possible that a monitor is
activated by the OTP, without that the kernel actually activates it.
I think it is not recommended, but it is possible.
> There can also be a problem if we have hardware where monitoring is
> common for all regulators, eg either globally enabled / disabled.
Yes, but I think in this case it should be the responsibility of the
driver to ensure that either all or no regulator is monitored, because
the same requirement is valid for implementing the protection ops.
Best regards,
Benjamin
On 5/23/23 14:51, Benjamin Bara wrote:
> Hi Matti,
>
> thanks for the feedback!
>
> On Tue, 23 May 2023 at 11:46, Matti Vaittinen <[email protected]> wrote:
>> As far as I see, this changes the existing logic. Previously the
>> monitoring was unconditionally enabled for all regulators, now it gets
>> only enabled for regulators which are marked as enabled.
>>
>> Furthermore, if I am not reading this wrong, the code tries to disable
>> all protections if regulator is not enabled at startup(?)
>>
>> I am not saying this is wrong. I am just saying that things will
>> change here and likely to break something.
>>
>> There are PMICs like ROHM BD9576, where the protection can not be
>> disabled.
>
> Thanks for letting me know! I dropped my initial "disable monitor while
> disabling the regulator" property, and activated it per default instead.
> But this basically means something like that will be required. I guess
> it might make sense to have a property which is called something like
> "monitor always on", to let the driver inform the core that the monitors
> cannot or should not be disabled, instead. > Except if you think there is a general problem with keeping monitors
> disabled while the regulator is disabled, then I might have to do it
> differently.
I am thinking that maybe the default should still be to not touch the
monitoring unless explicitly requested. My thinking is that the hardware
should by default be able to handle the voltage change / enable /
disable etc while monitoring is enabled. Hardware which requires
explicit monitoring disabling sounds (to me) like a 'design problem' and
disabling the monitoring sounds (to me) like a workaround. I wouldn't
make this workaround default. Furthermore, monitoring is a safety
feature, and as such core should not autonomously disable it (unless
such behaviour is requested). Well, experience has proven that my
thinking is not _always_ right, so feel free to voice other opinions :)
>> I am unsure if we might also have cases where some regulator could
>> really be enabled w/o core knowing it.
>
> Unfortunately, I am not 100% sure what you mean by that.
I was thinking of a case where regulator state is not readable - I'm not
100% sure how core thinks of their state. Another case could be a
regulator which is not registered to the core but shares monitoring with
some other regulator. This falls under the common monitoring category
mentioned below.
> On the da9063, for example, it might be possible that a monitor is
> activated by the OTP, without that the kernel actually activates it.
> I think it is not recommended, but it is possible.
>
>
>> There can also be a problem if we have hardware where monitoring is
>> common for all regulators, eg either globally enabled / disabled.
>
> Yes, but I think in this case it should be the responsibility of the
> driver to ensure that either all or no regulator is monitored, because
> the same requirement is valid for implementing the protection ops.
If I didn't misread the code, the differences here are that existing
"ideology" is to a) only touch the monitoring (enable/disable) when
explicitly requested for a given level and b) knowing that all monitors
that are requested to be enabled are enabled at the end of the probe.
In my eyes change a) is problematic. For example, if a board using
BD9576 wants to have protection disabled via device-tree (let's assume
there is a board where we know that some disturbance to voltages will
occur under specific conditions) - it is very valid to complain
disabling protection is not supported. Go fix your board design message
needs to be given because protection can't be disabled. This is very
different from case where we just try disabling monitoring because
regulator is turned off. In latter case with BD9576 the failure to
disable protection should just be silently ignored. When we use same
callbacks for both the initial configuration and the runtime
enable/disable/voltage-change handling the driver has no way knowing if
this is an error or not. Writing this leads me back to thinking that the
monitor configuration for enable/disable/voltage-change should be done
via separate driver callback - that would allow driver to separate these
use-cases. If this was change I wrote, I might try creating separate
driver callbacks for
enable/disable/voltage_change_start/voltage_change_done which get the
initial monitor configuration (as was read from device-tree) as an
argument. Do you think that could give the flexibility to handle all
different hardware quirks?
The change b) does also have consequences. Some PMICs like the BD9576 do
use same IRQ for indicating either ERROR or WARNING level problem.
Whether to use WARNING or ERROR is selected at star-up when the
device-tree flags are read. Eg, the .set_<XXX>_protection callbacks
store the severity information (WARNING or ERROR) and complain if both
are tried to be used. With the current approach we know the validity of
this configuration is checked right when regulator is registered, not
later at runtime when regulator is enabled.
Another example regarding design that uses the knowledge that all
requested monitors are enabled when regulator is registered is BD96801 -
which is not upstream (although I've had patches in my outbox for an
year already waiting for permission from the HQ to actually send them...
Don't ask...). This PMIC can configure fatality of the fault monitoring.
This driver checks that all regulators did agree on whether to use
PROTECTION or ERROR/WARNING level monitoring at the end of the probe -
and toggles the IRQ fatality accordingly. I truly believe that
out-of-tree drivers must not mandate upstream design - but I equally
believe that we may see similar HW designs in upstream and considering
this now makes sense :) Yes, in order to paper over b) a driver can for
sure go and parse all the monitoring properties from device-tree itself
and decide things based on that - but it might be quite a lot of
duplicated code.
To sum up my view - I do definitely like the idea that core supports
toggling the monitors for duration of enable/disable/voltage-change as
this is needed by some real world ICs.
I, however, think drivers should be able to separate the "set the
default monitoring config" request from the "change config to something
we use for duration of this operation" - because the best monitoring
config that is required for an operation may not be a "disable all".
Hence, we should leave it for the driver to decide what config to set
for the duration of an enable/disable/voltage_set-operation.
Furthermore, I believe the default should be "don't touch the
monitoring" and not to try disable/enable it w/o explicit request.
Again, thank you for working on this and including me in the discussion :)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Wed, May 24, 2023 at 10:28:10AM +0300, Matti Vaittinen wrote:
> I am thinking that maybe the default should still be to not touch the
> monitoring unless explicitly requested. My thinking is that the hardware
This is the general approach of the regulator API, we require explicit
permission to change any hardware setting since that way anything we do
that's unsafe for the hardware was the result of explicit permissions
rather than a software decision.
> > > I am unsure if we might also have cases where some regulator could
> > > really be enabled w/o core knowing it.
> > Unfortunately, I am not 100% sure what you mean by that.
> I was thinking of a case where regulator state is not readable - I'm not
> 100% sure how core thinks of their state. Another case could be a regulator
> which is not registered to the core but shares monitoring with some other
> regulator. This falls under the common monitoring category mentioned below.
I'd expect that a regulator which supports monitoring will have at least
the requested state readable so it wouldn't come up.
Hi Matti!
On Wed, 24 May 2023 at 09:28, Matti Vaittinen <[email protected]> wrote:
> I am thinking that maybe the default should still be to not touch the
> monitoring unless explicitly requested.
Got it - I will bring back the "mon_disable_reg_disabled" property. With
this property, the current behaviour will be in-place.
> My thinking is that the hardware should by default be able to handle
> the voltage change / enable / disable etc while monitoring is enabled.
> Hardware which requires explicit monitoring disabling sounds (to me)
> like a 'design problem' and disabling the monitoring sounds (to me)
> like a workaround. I wouldn't make this workaround default.
> Furthermore, monitoring is a safety feature, and as such core should
> not autonomously disable it (unless such behaviour is requested).
I totally agree with you here. However, there are regulators that
require such workarounds (e.g. bd718x7 and da9063) and that's the reason
for this series.
> I was thinking of a case where regulator state is not readable - I'm
> not 100% sure how core thinks of their state.
AFAIK, they would be considered as always on. But as Mark said, we could
add this as a requirement for having protection.
> Another case could be a regulator which is not registered to the core
> but shares monitoring with some other regulator.
I think this case should be handled by the driver anyways. Activating a
shared protection on one regulator, without activating it on the other
regulator should be considered as an error in my opinion.
> If I didn't misread the code, the differences here are that existing
> "ideology" is to a) only touch the monitoring (enable/disable) when
> explicitly requested for a given level and b) knowing that all
> monitors that are requested to be enabled are enabled at the end of
> the probe.
>
> In my eyes change a) is problematic. For example, if a board using
> BD9576 wants to have protection disabled via device-tree (let's assume
> there is a board where we know that some disturbance to voltages will
> occur under specific conditions) - it is very valid to complain
> disabling protection is not supported.
Yes, I think so too. I would not give the BD9576 any new "workaround
property", which would lead to the behaviour which is currently
implemented, meaning the monitoring is not touched after initialization.
> Go fix your board design message needs to be given because protection
> can't be disabled. This is very different from case where we just try
> disabling monitoring because regulator is turned off. In latter case
> with BD9576 the failure to disable protection should just be silently
> ignored. When we use same callbacks for both the initial configuration
> and the runtime enable/disable/voltage-change handling the driver has
> no way knowing if this is an error or not.
Got it. I am aware now that there are PMICs which do not allow to turn
off the monitor, therefore the default behaviour will be the same as
now. For now, only the da9063 (invalid state when monitoring a disabled
monitor) and the bd718x7 (invalid state when monitoring an enabled
regulator that switches to a higher voltage) are affected by the new
properties. The others which currently have {O,U}VP (max597x, bd9576)
should stay the same as now.
> Therefore, I will switch back to only do it when the monitor
> configuration for enable/disable/voltage-change should be done via
> separate driver callback - that would allow driver to separate these
> use-cases. If this was change I wrote, I might try creating separate
> driver callbacks for
> enable/disable/voltage_change_start/voltage_change_done which get the
> initial monitor configuration (as was read from device-tree) as an
> argument. Do you think that could give the flexibility to handle all
> different hardware quirks?
I think it would, yes. But I also think that it will lead to a lot of
duplicate code. However, instead of a simple "enable/disable" property,
we could reuse the "type of protection" too, to create some kind of
matrix. Example: Instead of setting mon_disable_reg_set_higher to 1 for
the bd718x7, we could set it to REGULATOR_MONITOR_OVER_VOLTAGE, meaning
just this protection should be disabled while switching to the higher
voltage. What do you think about that?
> The change b) does also have consequences. Some PMICs like the BD9576
> do use same IRQ for indicating either ERROR or WARNING level problem.
> Whether to use WARNING or ERROR is selected at star-up when the
> device-tree flags are read. Eg, the .set_<XXX>_protection callbacks
> store the severity information (WARNING or ERROR) and complain if both
> are tried to be used. With the current approach we know the validity
> of this configuration is checked right when regulator is registered,
> not later at runtime when regulator is enabled.
Not sure about that, but I think it would fail to register the
regulator? In this case, later it would not be able to enable it because
it is not registered, right?
> Another example regarding design that uses the knowledge that all
> requested monitors are enabled when regulator is registered is BD96801
> - which is not upstream (although I've had patches in my outbox for an
> year already waiting for permission from the HQ to actually send
> them... Don't ask...). This PMIC can configure fatality of the fault
> monitoring. This driver checks that all regulators did agree on
> whether to use PROTECTION or ERROR/WARNING level monitoring at the end
> of the probe - and toggles the IRQ fatality accordingly. I truly
> believe that out-of-tree drivers must not mandate upstream design -
> but I equally believe that we may see similar HW designs in upstream
> and considering this now makes sense :) Yes, in order to paper over b)
> a driver can for sure go and parse all the monitoring properties from
> device-tree itself and decide things based on that - but it might be
> quite a lot of duplicated code.
From my point of view, the behaviour will stay exactly the same! If they
don't agree on the same level, the probe should actually fail and the
regulators should not be registered.
> To sum up my view - I do definitely like the idea that core supports
> toggling the monitors for duration of enable/disable/voltage-change as
> this is needed by some real world ICs.
>
> I, however, think drivers should be able to separate the "set the
> default monitoring config" request from the "change config to
> something we use for duration of this operation" - because the best
> monitoring config that is required for an operation may not be a
> "disable all". Hence, we should leave it for the driver to decide what
> config to set for the duration of an
> enable/disable/voltage_set-operation.
>
> Furthermore, I believe the default should be "don't touch the
> monitoring" and not to try disable/enable it w/o explicit request.
Yes, I will definitely keep that in mind and implement it like that in
the next version.
> Again, thank you for working on this and including me in the
> discussion :)
Thanks for your valuable feedback!
Best regards,
Benjamin