| Message ID | 200903111932.16317.david-b@pacbell.net (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Wed, Mar 11, 2009 at 06:32:15PM -0800, David Brownell wrote: > Make the regulator setup code cope more consistently with > regulators undesirably left enabled by the bootloader. First up I'd just like to make it absolutely clear that I agree that this is a feature we should have - it's obviously useful. > * Unless the "boot_on" or "always_on" machine constraints > were set, disable() the regulator. This gives drivers > a clean start state: enable state matches usecount, > regardless of boot_on/always_on flag state. At the minute the regulator constraints have the property that if you pass a zero-initialised set of constraints the regulator API will not do anything other than allow you to read the state of the regulator - any action taken by the regulator API must be explicitly permitted by code. This change will mean that users will have to explicitly mark any regulators that are needed as enabled or we'll do unfortunate things. It is a particular problem for multi-function devices like pcf50633 which not only register all their regulators by default but also embed constraints within the general pcf50633 platform data. If the user simply turns on the regulator driver in their config they'll get this behavior if they don't edit the code. Even with regulator code I'd not be surprised if people were bitten by this for things like the memory or a CPU core without regulator based DVFS. You've addressed some of the use cases for this by providing devmode but it's still a big incompatible change, especially at this point in the development cycle where we've got at most a couple of weeks to the merge window. We could do this without introducing the incompatibility by adding a new flag (eg, boot_off) which machine constraints need to explicitly set to enforce this behaviour or by having something machine drivers can call to say "now power off any regulators that look idle". I'm not 100% agaist doing things the way you suggest since there is appeal in having boot_on be more of a boolean but I do feel that if we're going to go this way we should do it more gently. For example, we could merge a patch now which warns loudly that this will happen and then after the 2.6.30 merge window actually implement it. This would reduce merge issues for machine support coming in via other trees; we're rather late in the development cycle to be putting this in right now. Not everyone will read a warning but there is some chance they might and there's also a chance they'll test in -next. We should also provide a temporary Kconfig which actually enables the behaviour if people request it. > * To help make some integration stages easier, add a new > "devmode" machine constraint where state the bootloader > left isn't touched, but enable state and usecount may > not match. (System boots but some drivers act odd ... > debuggable. System dies part way through booting ... > often painful.) This also addresses things like the reverse engineering use case where people genuinely don't know what's going on and want to use the API to inspect the state of the regulators. I do wonder if we can't come up with a different way of expressing devmode - a different name like dont_disable that more directly expresses what the constraint does might be more obvious. Something that can enable this globally may also be convenient. The code itself is OK. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 12 March 2009, Mark Brown wrote: > On Wed, Mar 11, 2009 at 06:32:15PM -0800, David Brownell wrote: > > > Make the regulator setup code cope more consistently with > > regulators undesirably left enabled by the bootloader. > > First up I'd just like to make it absolutely clear that I agree that > this is a feature we should have - it's obviously useful. Feature being "consistency". I'd say "essential", not merely "useful" ... this is bugfix territory. There are things that just can't work with the current regulator framework, because its handling of this routine scenario is so inconsistent. It's not necessarily going to be safe to force every (!!) driver into that dance you had described (enable-it-then-disable-it), just to force all regulators into the kind of self-consistent state that framework users always expect to start with. Especially: RIGHT AFTER INITIALIZATION!!! There's no excluse to be self-inconsistent that early, even if there were an excuse later on. The "v4" patch I posted resolves that inconsistency in about the simplest way I can find; but unlike this patch it doesn't solve the "force this regulator off" problem. > It is a particular problem for multi-function devices like pcf50633 > which not only register all their regulators by default but also embed > constraints within the general pcf50633 platform data. The pcf50633 driver didn't *need* to register all regulators; and I don't see what the issue would be with platform_data. Its MFD core could easily check the regulator init data and skip regulators that didn't initialize a key field. > If the user > simply turns on the regulator driver in their config they'll get this > behavior if they don't edit the code. Even with regulator code I'd not > be surprised if people were bitten by this for things like the memory or > a CPU core without regulator based DVFS. That would be part of why the twl4030 regulators are only registered on request! Not only will a given board tend to not use all those regulators, but a number of them really aren't intended to be managed by Linux. (And then there are board options, like what various control signals do.) Similar things could happen with a system using pcf50633. There's no reason one of the LDOs or switching regulators shouldn't be managed exclusively by another I2C master on that bus ... they each have dedicated registers, and its not uncommon to dedicate a microcontroller to managing just one part of a system (and its resources). > You've addressed some of the use cases for this by providing devmode but I think it's pretty ugly myself, but you did say something about wanting explicit support for reverse engineering. The "v4" patch which I just sent uses a simpler approach; such support doesn't need to be explicit in order to work. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -799,18 +799,47 @@ static int set_machine_constraints(struc } } - /* If the constraints say the regulator should be on at this point - * and we have control then make sure it is enabled. + /* During integration, developers may need time to sort out what + * to do with this regulator; leave the bootloader's setting alone. + * Regulator consumers won't get consistent behavior. + * + * Else the constraints say whether it should be on or off; we + * don't leave it in an unknown state. */ - if ((constraints->always_on || constraints->boot_on) && ops->enable) { - ret = ops->enable(rdev); - if (ret < 0) { - printk(KERN_ERR "%s: failed to enable %s\n", - __func__, name); - rdev->constraints = NULL; - goto out; + if (constraints->devmode) { + char *label = "unknown"; + + if (ops->is_enabled) { + ret = ops->is_enabled(rdev); + if (ret == 0) + label = "disabled"; + else if (ret > 0) + label = "enabled"; + ret = 0; + } + pr_warning("%s: devmode regulator '%s' state is '%s'\n", + __func__, name, label); + } else if (constraints->always_on || constraints->boot_on) { + if (ops->enable) { + ret = ops->enable(rdev); + if (ret < 0) { + pr_err("%s: failed enabling %s\n", + __func__, name); + rdev->constraints = NULL; + goto out; + } } rdev->use_count = 1; + } else { + if (ops->disable) { + ret = ops->disable(rdev); + if (ret < 0) { + pr_err("%s: failed disabling %s\n", + __func__, name); + rdev->constraints = NULL; + goto out; + } + } } print_constraints(rdev); --- a/include/linux/regulator/machine.h +++ b/include/linux/regulator/machine.h @@ -117,7 +117,8 @@ struct regulation_constraints { /* mode to set on startup */ unsigned int initial_mode; - /* constriant flags */ + /* constraint flags */ + unsigned devmode:1; /* state after setup is indeterminate */ unsigned always_on:1; /* regulator never off when system is on */ unsigned boot_on:1; /* bootloader/firmware enabled regulator */ unsigned apply_uV:1; /* apply uV constraint iff min == max */