Hi!
This series targets the "automatic" state handling of voltage monitors
when the state of the monitored regulator is changed. This is e.g.
necessary for the da9063, which reaches an invalid state (!PWR_OK) if
the voltage monitor is not disabled before the regulator is disabled.
The problem could also be tackled inside of the driver's "state change
ops" (.enable(), .disable(), ...) but I thought it might be a good idea
to have a "common framework" independent of the driver's implementation.
For now, 1/2 adds "disable monitor properties" to the regulator's desc,
which indicate on which actions the monitors should be disabled.
2/2 depends on [1], which implements (static) voltage monitoring for the
da9063. It disables the monitors while the regulator is disabled.
The monitors are only enabled after the ramp-delay, to ensure that the
regulator is already in a valid state.
Possible next step:
"regulators-{uv,ov}-{warn,error,protection}-enable" dt property on chip
level, with either 1 or 0, to en-/disable the dynamic voltage monitoring
for every regulator of the chip. This would require the regulator's
set_{over,under}_voltage_protection() to work with limit = 1.
I added some TODOs (basically about error handling) and still have to do
some testing, but I wanted to share the "direction" of this series,
therefore the v2. Thank you, Mark and Matti, for the input!
Thanks & best regards,
Benjamin
[1] https://lore.kernel.org/all/[email protected]/
---
Changes in v2:
1/2:
- move from board-specific (machine.h) to driver-specific (driver.h)
- move from own struct to fields/properties in regulator_desc
- handle modes as one "unsupported modes" field
- factor out new monitors_set_state() to handle all (activated) monitors
- move re-enabling of monitor after ramp-delay
- add TODOs for error handling when the action fails (return error from
actual action instead, return state of monitoring to pre-action).
- reword commit message
2/2:
- adapting change to the properties approach
Link to v1: https://lore.kernel.org/r/[email protected]
---
Benjamin Bara (2):
regulator: add properties to disable monitoring on actions
regulator: da9063: disable monitoring while regulator is off
drivers/regulator/core.c | 64 ++++++++++++++++++++++++++++++++----
drivers/regulator/da9063-regulator.c | 2 ++
include/linux/regulator/driver.h | 10 ++++++
3 files changed, 70 insertions(+), 6 deletions(-)
---
base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
change-id: 20230419-dynamic-vmon-e08daa0ac7ad
Best regards,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
These are useful when the state of the regulator might change during
runtime, but the monitors state (in hardware) are not implicitly changed
with the change of the regulator state or mode (in hardware). Also, when
the monitors should be disabled while ramping after a set_value().
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/core.c | 64 ++++++++++++++++++++++++++++++++++++----
include/linux/regulator/driver.h | 10 +++++++
2 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4fcd36055b02..5052e1da85a7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1360,7 +1360,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;
@@ -1385,6 +1385,29 @@ 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 = 0;
+
+ /* 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 && reg_c->under_voltage_detection)
+ ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
+ enable ? ®_c->under_voltage_limits : &disable_limits);
+ return ret;
+}
+
/**
* set_machine_constraints - sets regulator constraints
* @rdev: regulator source
@@ -1512,7 +1535,7 @@ static int set_machine_constraints(struct regulator_dev *rdev)
"IC does not support requested over-current limits\n");
}
- if (rdev->constraints->over_voltage_detection)
+ if (rdev->constraints->over_voltage_detection && !rdev->desc->mon_disable_while_reg_off)
ret = handle_notify_limits(rdev,
ops->set_over_voltage_protection,
&rdev->constraints->over_voltage_limits);
@@ -1526,7 +1549,7 @@ static int set_machine_constraints(struct regulator_dev *rdev)
"IC does not support requested over voltage limits\n");
}
- if (rdev->constraints->under_voltage_detection)
+ if (rdev->constraints->under_voltage_detection && !rdev->desc->mon_disable_while_reg_off)
ret = handle_notify_limits(rdev,
ops->set_under_voltage_protection,
&rdev->constraints->under_voltage_limits);
@@ -2734,7 +2757,10 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
trace_regulator_enable_complete(rdev_get_name(rdev));
- return 0;
+ if (rdev->desc->mon_disable_while_reg_off)
+ ret = monitors_set_state(rdev, true);
+
+ return ret;
}
/**
@@ -2893,7 +2919,12 @@ EXPORT_SYMBOL_GPL(regulator_enable);
static int _regulator_do_disable(struct regulator_dev *rdev)
{
- int ret;
+ int ret = 0;
+
+ if (rdev->desc->mon_disable_while_reg_off)
+ ret = monitors_set_state(rdev, false);
+ if (ret)
+ return ret;
trace_regulator_disable(rdev_get_name(rdev));
@@ -3537,7 +3568,7 @@ static int _regulator_set_voltage_time(struct regulator_dev *rdev,
static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV)
{
- int ret;
+ int ret = 0;
int delay = 0;
int best_val = 0;
unsigned int selector;
@@ -3545,6 +3576,11 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
const struct regulator_ops *ops = rdev->desc->ops;
int old_uV = regulator_get_voltage_rdev(rdev);
+ if (rdev->desc->mon_disable_while_reg_set)
+ ret = monitors_set_state(rdev, false);
+ if (ret)
+ return ret;
+
trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
min_uV += rdev->constraints->uV_offset;
@@ -3636,6 +3672,10 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
out:
trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
+ if (rdev->desc->mon_disable_while_reg_set)
+ /* TODO: ignore return value here when ret already !0? */
+ ret = monitors_set_state(rdev, true);
+
return ret;
}
@@ -4545,7 +4585,19 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
if (ret < 0)
goto out;
+ if (mode & rdev->desc->mon_unsupported_reg_modes)
+ ret = monitors_set_state(rdev, false);
+ if (ret)
+ goto out;
+
ret = rdev->desc->ops->set_mode(rdev, mode);
+ if (ret)
+ goto out;
+
+ if (mode & ~rdev->desc->mon_unsupported_reg_modes)
+ /* TODO: if set_mode fails, we stay unmonitored */
+ ret = monitors_set_state(rdev, true);
+
out:
regulator_unlock(rdev);
return ret;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d3b4a3d4514a..2fdc2c78e4bd 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -357,6 +357,12 @@ enum regulator_type {
* the regulator was actually enabled. Max upto enable_time.
*
* @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
+ *
+ * @mon_disable_while_reg_off: Disables regulator's monitors while it is off.
+ * @mon_disable_while_reg_set: Disables regulator's monitors while it is changing its value.
+ * @mon_unsupported_reg_modes: Disables regulator's monitors before an unsupported mode is entered.
+ * Unsupported REGULATOR_MODE_* are OR'ed. REGULATOR_MODE_INVALID means
+ * all modes can be monitored.
*/
struct regulator_desc {
const char *name;
@@ -431,6 +437,10 @@ struct regulator_desc {
unsigned int poll_enabled_time;
unsigned int (*of_map_mode)(unsigned int mode);
+
+ unsigned int mon_disable_while_reg_off;
+ unsigned int mon_disable_while_reg_set;
+ unsigned int mon_unsupported_reg_modes;
};
/**
--
2.34.1
From: Benjamin Bara <[email protected]>
The PWR_OK state of the da9063 indicates whether all monitored voltages
are within the expected range. When a regulator is disabled without
disabling its voltage monitor before, the PWR_OK state becomes false. On
our board, this invalid state leads to a hard reset.
Therefore, prevent the invalid state by disabling the monitor before the
regulator is disabled.
This still requires to explicitly enable the voltage monitor in the
device tree and has no impact otherwise.
TODO: clarify if a MODE change has impact on the voltage monitor
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/da9063-regulator.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 82f52a2a031a..13a6b189f23a 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -99,6 +99,7 @@ struct da9063_regulator_info {
.desc.vsel_reg = DA9063_REG_V##regl_name##_A, \
.desc.vsel_mask = DA9063_V##regl_name##_MASK, \
.desc.linear_min_sel = DA9063_V##regl_name##_BIAS, \
+ .desc.mon_disable_while_reg_off = 1, \
.sleep = BFIELD(DA9063_REG_V##regl_name##_A, DA9063_LDO_SL), \
.suspend = BFIELD(DA9063_REG_##regl_name##_CONT, DA9063_LDO_CONF), \
.suspend_sleep = BFIELD(DA9063_REG_V##regl_name##_B, DA9063_LDO_SL), \
@@ -124,6 +125,7 @@ struct da9063_regulator_info {
.desc.vsel_reg = DA9063_REG_V##regl_name##_A, \
.desc.vsel_mask = DA9063_VBUCK_MASK, \
.desc.linear_min_sel = DA9063_VBUCK_BIAS, \
+ .desc.mon_disable_while_reg_off = 1, \
.sleep = BFIELD(DA9063_REG_V##regl_name##_A, DA9063_BUCK_SL), \
.suspend = BFIELD(DA9063_REG_##regl_name##_CONT, DA9063_BUCK_CONF), \
.suspend_sleep = BFIELD(DA9063_REG_V##regl_name##_B, DA9063_BUCK_SL), \
--
2.34.1
On 4/21/23 12:13, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> These are useful when the state of the regulator might change during
> runtime, but the monitors state (in hardware) are not implicitly changed
> with the change of the regulator state or mode (in hardware). Also, when
> the monitors should be disabled while ramping after a set_value().
>
> Signed-off-by: Benjamin Bara <[email protected]>
> ---
> drivers/regulator/core.c | 64 ++++++++++++++++++++++++++++++++++++----
> include/linux/regulator/driver.h | 10 +++++++
> 2 files changed, 68 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 4fcd36055b02..5052e1da85a7 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1360,7 +1360,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;
>
> @@ -1385,6 +1385,29 @@ 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 = 0;
> +
> + /* 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 && reg_c->under_voltage_detection)
> + ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> + enable ? ®_c->under_voltage_limits : &disable_limits);
I still think forcing the use of the set_over_voltage_protection() /
set_under_voltage_protection() (and omitting over-current protection)
instead of allowing the driver to populate potentially more suitable
callbacks is somewhat limiting.
For example, the only _in-tree_ regulator driver that I know is
disabling monitors at voltage change is the bd718x7. There we have extra
conditions for disabling - the power must be enabled (which could
probably be a generic condition for disabling monitoring at voltage
change), and also the new voltage must be greater than the old voltage.
This logic is naturally implemented in set_under_voltage_protection -
which should unconditionally disable the monitoring if it is requested
via device-tree.
After that being said - I am leaving weighing pros and cons to You and
Mark - I don't feel I am in a position where I should dictate the design
here. I'll just say that if the new generic disabling unconditionally
uses set_under_voltage_protection - then at least the bd718x7 can not
benefit from it w/o relaxing the monitoring.
Finally, thanks Benjamin for improving things here!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~