| Message ID | 1461395466-14896-2-git-send-email-pingbo.wen@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sat, Apr 23, 2016 at 03:11:06PM +0800, WEN Pingbo wrote: > This patch try to add a boot_protection flag in regulator constraints. > So the regulator core will prevent the specified operation during kernel > booting. > The boot_protection flag only work before late_initicall. And as other > constraints liked, you can specify this flag in a board file, or in > dts file. By default, all operations of this regulator will be rejected > during kernel booting, if you add this flag in a regulator. But you > still have a chance to change this, by modifying boot_valid_ops_mask. This is still a complete hack which is going to break as soon as things are built modular, it's definitely *not* something that should ever appear in DT since it depends so heavily on implementation details. If you need some driver to start early work on getting that sorted. This is also going to interact badly with any other drivers that are trying to configure things at runtime, if they've done enables and disables (or especially an enable without a matching disable) their refcounts are going to be wrong and if they've tried to do anything with setting voltages we'll have completely ignored whatever they asked for or told them that they can't change voltages. If we were doing anything like this it would need to be a lot more transparent to other regulators sharing the supplies (which are presumably what's causing problems here). > [ This patch depends on regulator_ops_is_valid patch. And some document > need to add, but I want to hear some voice first. ] There is no need to say that patch 2 in a series depends on patch 1. > @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev) > rdev_dbg(rdev, "%s\n", buf); > > if ((constraints->min_uV != constraints->max_uV) && > - !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) > + !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) > rdev_warn(rdev, > "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n"); > } This appears to be unrelated? > + if (constraints->boot_protection) { > + if (of_property_read_bool(np, "boot-allow-set-voltage")) > + constraints->boot_valid_ops_mask |= > + REGULATOR_CHANGE_VOLTAGE; We were factoring things out a minute ago...
Hi, Mark > 在 2016年4月25日，06:51，Mark Brown <broonie@kernel.org> 写道： > > On Sat, Apr 23, 2016 at 03:11:06PM +0800, WEN Pingbo wrote: > >> This patch try to add a boot_protection flag in regulator constraints. >> So the regulator core will prevent the specified operation during kernel >> booting. > >> The boot_protection flag only work before late_initicall. And as other >> constraints liked, you can specify this flag in a board file, or in >> dts file. By default, all operations of this regulator will be rejected >> during kernel booting, if you add this flag in a regulator. But you >> still have a chance to change this, by modifying boot_valid_ops_mask. > > This is still a complete hack which is going to break as soon as things > are built modular, it's definitely *not* something that should ever > appear in DT since it depends so heavily on implementation details. If > you need some driver to start early work on getting that sorted. > I think this patch can handle the case you mentioned. I have add a regulator_has_booted flag, and it will set in regulator_init_complete() late_initcall hook. The regulator_ops_is_valid() will ignore boot protection if this flag is set. > This is also going to interact badly with any other drivers that are > trying to configure things at runtime, if they've done enables and > disables (or especially an enable without a matching disable) their > refcounts are going to be wrong and if they've tried to do anything with > setting voltages we'll have completely ignored whatever they asked for > or told them that they can't change voltages. If we were doing anything > like this it would need to be a lot more transparent to other > regulators sharing the supplies (which are presumably what's causing > problems here). Ok, I have to admit that the boot_protection didn’t cover this. If other consumer try to configure during booting, it will get some error code. And the consumers need to re-configure the regulator state after late_initcall. If we need to hold the state of other consumer, I prefer using a dummy-consumer to hold this. And this is my next try. > >> @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev) >> rdev_dbg(rdev, "%s\n", buf); >> >> if ((constraints->min_uV != constraints->max_uV) && >> - !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) >> + !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) >> rdev_warn(rdev, >> "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n"); >> } > > This appears to be unrelated? > >> + if (constraints->boot_protection) { >> + if (of_property_read_bool(np, "boot-allow-set-voltage")) >> + constraints->boot_valid_ops_mask |= >> + REGULATOR_CHANGE_VOLTAGE; > > We were factoring things out a minute ago… Actually, the two part are only want to check the regulator operation capacity, if we include the boot_protect checking, it will get error. Pingbo
On Tue, Apr 26, 2016 at 07:46:07PM +0800, Pingbo Wen wrote: > > This is still a complete hack which is going to break as soon as things > > are built modular, it's definitely *not* something that should ever > > appear in DT since it depends so heavily on implementation details. If > > you need some driver to start early work on getting that sorted. > I think this patch can handle the case you mentioned. I have add a > regulator_has_booted flag, and it will set in regulator_init_complete() > late_initcall hook. The regulator_ops_is_valid() will ignore boot > protection if this flag is set. That doesn't help after we get to userspace so doesn't work as soon as things are built as modules. That's a key issue with making something that's robust here. > > This is also going to interact badly with any other drivers that are > > trying to configure things at runtime, if they've done enables and > > disables (or especially an enable without a matching disable) their > Ok, I have to admit that the boot_protection didn’t cover this. If other > consumer try to configure during booting, it will get some error code. > And the consumers need to re-configure the regulator state after > late_initcall. The worst thing is where we silently accept but forget about things because the code currently translates them into valid noops then later start paying attention to the operations, error codes at least the other driver can handle. > If we need to hold the state of other consumer, I prefer using a > dummy-consumer to hold this. And this is my next try. Or possibly store the data on the consumer but don't act on it then go round actually acting on it when we decide to do things - that's more what I'd have expected and seems like it might be easier than creating a separate object.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index fe47d38..5b9dc22 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -55,6 +55,7 @@ static LIST_HEAD(regulator_map_list); static LIST_HEAD(regulator_ena_gpio_list); static LIST_HEAD(regulator_supply_alias_list); static bool has_full_constraints; +static bool regulator_has_booted; static struct dentry *debugfs_root; @@ -139,7 +140,15 @@ static bool regulator_ops_is_valid(struct regulator_dev *rdev, int ops) return false; } - if (rdev->constraints->valid_ops_mask & ops) + /* + * Ignore regulator boot-protection, after later_initcall. + */ + if (!regulator_has_booted && rdev->constraints->boot_protection) { + if (rdev->constraints->boot_valid_ops_mask & ops) + return true; + else + rdev_info(rdev, "rejected operation 0x%02x\n", ops); + } else if (rdev->constraints->valid_ops_mask & ops) return true; return false; @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev) rdev_dbg(rdev, "%s\n", buf); if ((constraints->min_uV != constraints->max_uV) && - !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) + !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) rdev_warn(rdev, "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n"); } @@ -1309,7 +1318,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev, * it is then we don't need to do nearly so much work for * enable/disable calls. */ - if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS) && + if (rdev->constraints && + !(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_STATUS) && _regulator_is_enabled(rdev)) regulator->always_on = true; @@ -4353,6 +4363,12 @@ static int __init regulator_late_cleanup(struct device *dev, void *data) struct regulation_constraints *c = rdev->constraints; int enabled, ret; + /* + * The kernel boot is finished, let's unset boot_protection + * Need a lock? + */ + c->boot_protection = 0; + if (c && c->always_on) return 0; @@ -4406,6 +4422,8 @@ static int __init regulator_init_complete(void) if (of_have_populated_dt()) has_full_constraints = true; + regulator_has_booted = true; + /* If we have a full configuration then disable any regulators * we have permission to change the status for and which are * not in use or always_on. This is effectively the default diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index 6b0aa80..bfec59c 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -78,6 +78,35 @@ static void of_get_regulation_constraints(struct device_node *np, if (of_property_read_bool(np, "regulator-allow-set-load")) constraints->valid_ops_mask |= REGULATOR_CHANGE_DRMS; + constraints->boot_protection = of_property_read_bool(np, + "regulator-boot-protection"); + + if (constraints->boot_protection) { + if (of_property_read_bool(np, "boot-allow-set-voltage")) + constraints->boot_valid_ops_mask |= + REGULATOR_CHANGE_VOLTAGE; + if (of_property_read_bool(np, "boot-allow-set-current")) + constraints->boot_valid_ops_mask |= + REGULATOR_CHANGE_CURRENT; + if (of_property_read_bool(np, "boot-allow-set-mode")) + constraints->boot_valid_ops_mask |= + REGULATOR_CHANGE_MODE; + if (of_property_read_bool(np, "boot-allow-set-status")) + constraints->boot_valid_ops_mask |= + REGULATOR_CHANGE_STATUS; + if (of_property_read_bool(np, "boot-allow-set-load")) + constraints->boot_valid_ops_mask |= + REGULATOR_CHANGE_DRMS; + if (of_property_read_bool(np, "boot-allow-bypass")) + constraints->boot_valid_ops_mask |= + REGULATOR_CHANGE_BYPASS; + + /* + * boot_valid_ops_mask is a subset of valid_ops_mask + */ + constraints->boot_valid_ops_mask &= constraints->valid_ops_mask; + } + ret = of_property_read_u32(np, "regulator-ramp-delay", &pval); if (!ret) { if (pval) diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h index 5d627c8..a4f5c0f 100644 --- a/include/linux/regulator/machine.h +++ b/include/linux/regulator/machine.h @@ -134,6 +134,7 @@ struct regulation_constraints { /* valid operations for regulator on this machine */ unsigned int valid_ops_mask; + unsigned int boot_valid_ops_mask; /* regulator input voltage - only if supply is another regulator */ int input_uV; @@ -155,6 +156,7 @@ struct regulation_constraints { /* constraint flags */ unsigned always_on:1; /* regulator never off when system is on */ unsigned boot_on:1; /* bootloader/firmware enabled regulator */ + unsigned boot_protection:1; unsigned apply_uV:1; /* apply uV constraint if min == max */ unsigned ramp_disable:1; /* disable ramp delay */ unsigned soft_start:1; /* ramp voltage slowly */
In some platform, some critical shared regulator is initialized before kernel loading. But in kernel booting, the driver probing order and conflict operation from other regulator consumer, may set the regulator in a undefined state, which will cause serious problem. This patch try to add a boot_protection flag in regulator constraints. So the regulator core will prevent the specified operation during kernel booting. The boot_protection flag only work before late_initicall. And as other constraints liked, you can specify this flag in a board file, or in dts file. By default, all operations of this regulator will be rejected during kernel booting, if you add this flag in a regulator. But you still have a chance to change this, by modifying boot_valid_ops_mask. [ This patch depends on regulator_ops_is_valid patch. And some document need to add, but I want to hear some voice first. ] Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org> --- drivers/regulator/core.c | 24 +++++++++++++++++++++--- drivers/regulator/of_regulator.c | 29 +++++++++++++++++++++++++++++ include/linux/regulator/machine.h | 2 ++ 3 files changed, 52 insertions(+), 3 deletions(-) -- 1.9.1