| Message ID | 1462777508-24934-1-git-send-email-pingbo.wen@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote: > In some platforms, critical shared regulator is initialized in > bootloader. But during kernel booting, the driver probing order and > conflicting operations from other regulator consumers, 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. This still feels like a short term hack that doesn't belong in an ABI. It's all very implementation specific and not very robust, it's not describing the outcome we're looking for but rather a very specific behaviour which won't work outside of a fairly narrow system configuration. The difficulty in coherently explaining what the end of boot is and what protection means is a big warning sign here. I think you need to be looking at some combination of getting the devices you're interested in started up early and more precisely describing the end result you're trying to achieve. The issues with probe deferral do seem related here, it's another symptom of not really making any decisions about init ordering and so sometimes making bad ones. > And regulator core will postpone all operations until all consumers > have taked their place. It doesn't, it postpones them until late_initacall(). This is both after the consumers have loaded if they are built in and before any consumers built as modules come up. > 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. Anything added to the DT ABI needs a binding. > + /* constraints check has already done */ > + if (rdev->boot_mode) > + rdev->desc->ops->set_mode(rdev, rdev->boot_mode); This whole sequence of code ignores errors - that's not great. We should at least log them. > + mutex_unlock(&rdev->mutex); > + > + if (regulator) > + regulator_set_voltage(regulator, regulator->min_uV, > + regulator->max_uV); That's... exciting. There's a couple of issues here. One is that this is not operating on the rdev but rather on a consumer regulator device, the other is that we drop out of the lock before doing the update which tends to be a warning sign that something fun is going on and at least an internal function should be used. These two most likely come down to the same issue.
Hi, Mark On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote: > On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote: > >> And regulator core will postpone all operations until all consumers >> have taked their place. > > It doesn't, it postpones them until late_initacall(). This is both > after the consumers have loaded if they are built in and before any > consumers built as modules come up. Yes, this patch only protects a regulator from regulator registration(built in) to late_initcall(). But, IMO, if a regulator is critical, it's weird to build as a module. Maybe I was thoughtless here. If we take modules under consideration, and to make this patch more universal, I think what we really need is adding a flag to protect a regulator from registration to a specific consumer(not the first consumer). The regulator driver gives the initial state, and the specific consumer need to clear this flag while finishing regulator setting(by calling a function like regulator_clear_protect()). And what the regulator core need to do is staging all operations during protection. And that will cover all consumers probing order, whenever the regulator is registered. Any idea? > >> 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. > > Anything added to the DT ABI needs a binding. > I will add bindings in next version. >> + /* constraints check has already done */ >> + if (rdev->boot_mode) >> + rdev->desc->ops->set_mode(rdev, rdev->boot_mode); > > This whole sequence of code ignores errors - that's not great. We > should at least log them. > OK. >> + mutex_unlock(&rdev->mutex); >> + >> + if (regulator) >> + regulator_set_voltage(regulator, regulator->min_uV, >> + regulator->max_uV); > > That's... exciting. There's a couple of issues here. One is that > this is not operating on the rdev but rather on a consumer regulator > device, the other is that we drop out of the lock before doing the > update which tends to be a warning sign that something fun is going on > and at least an internal function should be used. These two most likely > come down to the same issue. > OK, some bugs here. I will use a unlock version. Pingbo
On Wed, Jun 15, 2016 at 08:05:13PM +0800, Pingbo Wen wrote: > On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote: > > It doesn't, it postpones them until late_initacall(). This is both > > after the consumers have loaded if they are built in and before any > > consumers built as modules come up. > Yes, this patch only protects a regulator from regulator > registration(built in) to late_initcall(). But, IMO, if a regulator is > critical, it's weird to build as a module. Maybe I was thoughtless here. Well, this comes back to the issue with it being very use case specific and not generally clear what critical regualtors are supposed to be. It really depends on what the system integrator wants, and there's things like the distro use case where the normal thing to do is to build as many drivers as possible as modules since you support such a huge range of hardware. > If we take modules under consideration, and to make this patch more > universal, I think what we really need is adding a flag to protect a > regulator from registration to a specific consumer(not the first > consumer). The regulator driver gives the initial state, and the > specific consumer need to clear this flag while finishing regulator > setting(by calling a function like regulator_clear_protect()). And what > the regulator core need to do is staging all operations during > protection. And that will cover all consumers probing order, whenever > the regulator is registered. Having the consumer driver know that it's "critical" seems wrong since different systems may have different ideas about that, it's probably better to hook this in with the device model so that when the device finishes probing that kicks things off. > Any idea? There were the other ideas I mentioned in my mail too.
On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote: > On Wed, Jun 15, 2016 at 08:05:13PM +0800, Pingbo Wen wrote: >> On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote: > >> If we take modules under consideration, and to make this patch more >> universal, I think what we really need is adding a flag to protect a >> regulator from registration to a specific consumer(not the first >> consumer). The regulator driver gives the initial state, and the >> specific consumer need to clear this flag while finishing regulator >> setting(by calling a function like regulator_clear_protect()). And what >> the regulator core need to do is staging all operations during >> protection. And that will cover all consumers probing order, whenever >> the regulator is registered. > > Having the consumer driver know that it's "critical" seems wrong since > different systems may have different ideas about that, it's probably > better to hook this in with the device model so that when the device > finishes probing that kicks things off. > That will imply the protection would be end when the specific device has probed, and consumers should take their place at the same time. But there have some other devices, which will set the consumer in a IRQ event, or after some other events, can't be covered. We can set the protection flag easily, but it's hard to tell whether a consumer is well initialized, the end of protection, since regulator consumer is not initialized within one call. Pingbo
On Fri, Jun 17, 2016 at 11:34:25AM +0800, Pingbo Wen wrote: > On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote: > > Having the consumer driver know that it's "critical" seems wrong since > > different systems may have different ideas about that, it's probably > > better to hook this in with the device model so that when the device > > finishes probing that kicks things off. > That will imply the protection would be end when the specific device has > probed, and consumers should take their place at the same time. But > there have some other devices, which will set the consumer in a IRQ > event, or after some other events, can't be covered. I don't understand what this means, sorry. > We can set the protection flag easily, but it's hard to tell whether a > consumer is well initialized, the end of protection, since regulator > consumer is not initialized within one call. If the driver is not initializing itself during probe the driver is doing something wrong and needs to be fixed anyway.
Hi, Mark On Friday, June 17, 2016 07:42 PM, Mark Brown wrote: > On Fri, Jun 17, 2016 at 11:34:25AM +0800, Pingbo Wen wrote: >> On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote: > >>> Having the consumer driver know that it's "critical" seems wrong since >>> different systems may have different ideas about that, it's probably >>> better to hook this in with the device model so that when the device >>> finishes probing that kicks things off. > >> That will imply the protection would be end when the specific device has >> probed, and consumers should take their place at the same time. But >> there have some other devices, which will set the consumer in a IRQ >> event, or after some other events, can't be covered. > > I don't understand what this means, sorry. > I mean maybe there's some consumer driver only do partial initialization during probing. >> We can set the protection flag easily, but it's hard to tell whether a >> consumer is well initialized, the end of protection, since regulator >> consumer is not initialized within one call. > > If the driver is not initializing itself during probe the driver is > doing something wrong and needs to be fixed anyway. > OK, if all driver have full initialized during probing, and we need insert a hook after driver probing. I think we can add a function in driver/base/dd.c:driver_probe_device() as this: ret = really_probe(dev, drv); ... if (!ret) regulator_clean_up(dev); And in regulator_clean_up(), we can iterate all regulator deivce, and call a regulator_clear_boot_protection function: if (!rdev->constraints->boot_protection) return 0; if (strcmp(rdev->constraints->critical_consumer, dev_name(dev))) return 0; rdev->constraints->boot_protection = 0; ... -real clean stuffs- the critical_consumer can be specified in devicetree. Add a callback in driver_probe_device() is not so good, but it's fine for me. Pingbo
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index fe47d38..f994a0f 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; @@ -1030,6 +1031,13 @@ static int set_machine_constraints(struct regulator_dev *rdev, if (!rdev->constraints) return -ENOMEM; + /* + * If a regulator driver is registered after late_initcall, the + * boot_protection should be ingnored. + */ + if (regulator_has_booted) + rdev->constraints->boot_protection = 0; + ret = machine_constraints_voltage(rdev, rdev->constraints); if (ret != 0) return ret; @@ -2195,8 +2203,14 @@ static int _regulator_disable(struct regulator_dev *rdev) if (rdev->use_count == 1 && (rdev->constraints && !rdev->constraints->always_on)) { - /* we are last user */ - if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS)) { + /* + * We are last user. + * + * If boot_protection is set, we only clear use_count, + * and regulator_init_complete() will disable it. + */ + if (!rdev->constraints->boot_protection && + regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS)) { ret = _notifier_call_chain(rdev, REGULATOR_EVENT_PRE_DISABLE, NULL); @@ -2297,6 +2311,10 @@ int regulator_force_disable(struct regulator *regulator) struct regulator_dev *rdev = regulator->rdev; int ret; + WARN(rdev->constraints->boot_protection, + "disable regulator %s with boot protection flag\n", + rdev->desc->name); + mutex_lock(&rdev->mutex); regulator->uA_load = 0; ret = _regulator_force_disable(regulator->rdev); @@ -2852,6 +2870,10 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator, if (ret < 0) goto out2; + /* We need to change voltage, but boot_protection is set. */ + if (rdev->constraints->boot_protection) + goto out; + if (rdev->supply && (rdev->desc->min_dropout_uV || !rdev->desc->ops->get_voltage)) { int current_supply_uV; @@ -3069,6 +3091,9 @@ int regulator_sync_voltage(struct regulator *regulator) if (ret < 0) goto out; + if (rdev->constraints->boot_protection) + goto out; + ret = _regulator_do_set_voltage(rdev, min_uV, max_uV); out: @@ -3161,6 +3186,15 @@ int regulator_set_current_limit(struct regulator *regulator, if (ret < 0) goto out; + /* + * Stage new current value, and applied it later. + */ + if (rdev->constraints->boot_protection) { + regulator->min_uA = min_uA; + regulator->max_uA = max_uA; + goto out; + } + ret = rdev->desc->ops->set_current_limit(rdev, min_uA, max_uA); out: mutex_unlock(&rdev->mutex); @@ -3240,6 +3274,11 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode) if (ret < 0) goto out; + if (rdev->constraints->boot_protection) { + rdev->boot_mode = mode; + goto out; + } + ret = rdev->desc->ops->set_mode(rdev, mode); out: mutex_unlock(&rdev->mutex); @@ -3306,11 +3345,14 @@ EXPORT_SYMBOL_GPL(regulator_get_mode); int regulator_set_load(struct regulator *regulator, int uA_load) { struct regulator_dev *rdev = regulator->rdev; - int ret; + int ret = 0; mutex_lock(&rdev->mutex); regulator->uA_load = uA_load; - ret = drms_uA_update(rdev); + + if (!rdev->constraints->boot_protection) + ret = drms_uA_update(rdev); + mutex_unlock(&rdev->mutex); return ret; @@ -3344,7 +3386,8 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable) if (enable && !regulator->bypass) { rdev->bypass_count++; - if (rdev->bypass_count == rdev->open_count) { + if (rdev->bypass_count == rdev->open_count && + !rdev->constraints->boot_protection) { ret = rdev->desc->ops->set_bypass(rdev, enable); if (ret != 0) rdev->bypass_count--; @@ -3353,7 +3396,8 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable) } else if (!enable && regulator->bypass) { rdev->bypass_count--; - if (rdev->bypass_count != rdev->open_count) { + if (rdev->bypass_count != rdev->open_count && + !rdev->constraints->boot_protection) { ret = rdev->desc->ops->set_bypass(rdev, enable); if (ret != 0) rdev->bypass_count++; @@ -4346,6 +4390,51 @@ static int __init regulator_init(void) /* init early to allow our consumers to complete system booting */ core_initcall(regulator_init); +static void __init regulator_clear_boot_protection(struct regulator_dev *rdev) +{ + struct regulator *regulator; + int min_uA = INT_MAX, max_uA = 0; + + mutex_lock(&rdev->mutex); + + rdev->constraints->boot_protection = 0; + + /* update current setting */ + list_for_each_entry(regulator, &rdev->consumer_list, list) { + if (regulator->min_uA < min_uA) + min_uA = regulator->min_uA; + if (regulator->max_uA > max_uA) + max_uA = regulator->max_uA; + } + + if (max_uA && !regulator_check_current_limit(rdev, &min_uA, &max_uA)) + rdev->desc->ops->set_current_limit(rdev, min_uA, max_uA); + + /* constraints check has already done */ + if (rdev->boot_mode) + rdev->desc->ops->set_mode(rdev, rdev->boot_mode); + + /* update regulator load */ + drms_uA_update(rdev); + + /* check if we need to set bypass mode */ + if (rdev->desc->ops->set_bypass && rdev->bypass_count && + regulator_ops_is_valid(rdev, REGULATOR_CHANGE_BYPASS)) { + if (rdev->bypass_count == rdev->open_count) + rdev->desc->ops->set_bypass(rdev, true); + else + rdev->desc->ops->set_bypass(rdev, false); + } + + regulator = list_first_entry_or_null(&rdev->consumer_list, + struct regulator, list); + mutex_unlock(&rdev->mutex); + + if (regulator) + regulator_set_voltage(regulator, regulator->min_uV, + regulator->max_uV); +} + static int __init regulator_late_cleanup(struct device *dev, void *data) { struct regulator_dev *rdev = dev_to_rdev(dev); @@ -4353,6 +4442,9 @@ static int __init regulator_late_cleanup(struct device *dev, void *data) struct regulation_constraints *c = rdev->constraints; int enabled, ret; + if (c->boot_protection) + regulator_clear_boot_protection(rdev); + if (c && c->always_on) return 0; @@ -4406,6 +4498,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/internal.h b/drivers/regulator/internal.h index c74ac87..ab81a71 100644 --- a/drivers/regulator/internal.h +++ b/drivers/regulator/internal.h @@ -29,6 +29,8 @@ struct regulator { int uA_load; int min_uV; int max_uV; + int min_uA; + int max_uA; char *supply_name; struct device_attribute dev_attr; struct regulator_dev *rdev; diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index 6b0aa80..95fd789 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -78,6 +78,9 @@ 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"); + ret = of_property_read_u32(np, "regulator-ramp-delay", &pval); if (!ret) { if (pval) diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index cd271e8..ddb80a3 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -389,6 +389,9 @@ struct regulator_dev { u32 open_count; u32 bypass_count; + /* save mode during boot protection */ + unsigned int boot_mode; + /* lists we belong to */ struct list_head list; /* list of all regulators */ diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h index 5d627c8..fd88a12 100644 --- a/include/linux/regulator/machine.h +++ b/include/linux/regulator/machine.h @@ -155,6 +155,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; /* protect regulator initialized by bootloader */ 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 platforms, critical shared regulator is initialized in bootloader. But during kernel booting, the driver probing order and conflicting operations from other regulator consumers, 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. And regulator core will postpone all operations until all consumers have taked their place. 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. Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org> --- drivers/regulator/core.c | 106 +++++++++++++++++++++++++++++++++++--- drivers/regulator/internal.h | 2 + drivers/regulator/of_regulator.c | 3 ++ include/linux/regulator/driver.h | 3 ++ include/linux/regulator/machine.h | 1 + 5 files changed, 109 insertions(+), 6 deletions(-) -- 1.9.1