Messages in this thread | | | Subject | Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support | From | Paul Kocialkowski <> | Date | Fri, 12 Feb 2016 20:24:11 +0100 |
| |
Hi,
Le mardi 09 février 2016 à 21:51 +0100, Paul Kocialkowski a écrit : > Le jeudi 21 janvier 2016 à 20:24 +0000, Mark Brown a écrit : > > Provide a flag auto_runtime_pm in the regulator_desc which causes the > > regulator core to take a runtime PM reference to a regulator while it > > is enabled. This helps integration with chip wide power management for > > auxiliary PMICs, they may be able to implement chip wide power savings > > if nothing on the PMIC is in use. > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > --- > > > > Not tested at all yet, pushing out for testing by others who have > > devices that could benefit from this. > > Thanks for pushing this, I didn't think it would be that easy to > implement. > > I have tried using those patches on top of my Optimus Black support > series (as of v2), on top of 4.5.0-rc2. As-is, it fails at run-time, due > to badly-ordered calls to runtime pm functions. Here is the relevant > part of the kernel log: > > [ 1.943359] regulator disable, pm autosuspend > [ 1.949615] regulator enable, pm sync > [ 1.953491] lp872x_runtime_resume() 37 > [ 1.957519] lp872x_runtime_suspend() 37 > > Despite having the regulator disabled and enabled in that precise order, > the runtime functions are called in the opposite order, which causes the > enable pin (and thus the whole regulator chip) to be disabled. > > Is that behavior intended? I suppose this is not an issue at the > regulator core level.
I have added more tracing today. Here is some more context: [ 2.016510] omap_hsmmc 4809c000.mmc: Got CD GPIO [ 2.021697] omap_hsmmc 4809c000.mmc: omap_device: omap_device_enable() called from invalid state 1 [ 2.031616] omap_hsmmc_disable_boot_regulators() vmmc disable [ 2.038177] omap_hsmmc_disable_boot_regulator(): regulator enable [ 2.045104] omap_hsmmc_disable_boot_regulator(): regulator disable [ 2.052368] regulator disable, pm autosuspend [ 2.056976] omap_hsmmc_disable_boot_regulators() vqmmc disable [ 2.063110] omap_hsmmc_disable_boot_regulators() pbias disable [ 2.069244] omap_hsmmc_disable_boot_regulator(): regulator enable [ 2.075653] omap_hsmmc_disable_boot_regulator(): regulator disable [ 2.082397] omap_hsmmc_enable_supply(): vmmc regulator enable [ 2.088958] mmc_regulator_set_ocr(): regulator enable [ 2.095764] regulator enable, pm sync [ 2.099639] lp872x_runtime_resume() 37 [ 2.104309] lp872x_runtime_suspend() 37
This is mostly the omap_hsmmc driver enabling and disabling the regulator to sort out the boot state (in omap_hsmmc_disable_boot_regulators). There is a comment in that function that explains this behavior.
Apparently, the first enable is not acted upon by the regulator core as the regulator is enabled at boot.
So I think the question here is why runtime pm is calling the device functions in reverse order. Any clue? Is that intended behavior?
> The diff I used to produce this is as follows. Perhaps I'm doing > something wrong? > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index e39c75d..416701d 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -2061,6 +2061,7 @@ static int _regulator_do_enable(struct > regulator_dev *rdev) > } > > if (rdev->desc->auto_runtime_pm) { > + printk(KERN_ERR "regulator enable, pm sync\n"); > ret = pm_runtime_get_sync(rdev->dev.parent); > if (ret < 0) > goto err; > @@ -2190,8 +2191,10 @@ static int _regulator_do_disable(struct > regulator_dev *rdev) > return ret; > } > > - if (rdev->desc->auto_runtime_pm) > + if (rdev->desc->auto_runtime_pm) { > + printk(KERN_ERR "regulator disable, pm autosuspend\n"); > pm_runtime_put_autosuspend(rdev->dev.parent); > + } > > /* cares about last_off_jiffy only if off_on_delay is required > by > * device. > diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c > index 3e74861..5b99a34 100644 > --- a/drivers/regulator/lp872x.c > +++ b/drivers/regulator/lp872x.c > @@ -15,6 +15,9 @@ > #include <linux/regmap.h> > #include <linux/err.h> > #include <linux/gpio.h> > +#include <linux/delay.h> > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> > #include <linux/regulator/lp872x.h> > #include <linux/regulator/driver.h> > #include <linux/platform_device.h> > @@ -522,6 +525,7 @@ static struct regulator_desc lp8720_regulator_desc[] > = { > .of_match = of_match_ptr("ldo1"), > .id = LP8720_ID_LDO1, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -536,6 +540,7 @@ static struct regulator_desc lp8720_regulator_desc[] > = { > .of_match = of_match_ptr("ldo2"), > .id = LP8720_ID_LDO2, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -550,6 +555,7 @@ static struct regulator_desc lp8720_regulator_desc[] > = { > .of_match = of_match_ptr("ldo3"), > .id = LP8720_ID_LDO3, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -564,6 +570,7 @@ static struct regulator_desc lp8720_regulator_desc[] > = { > .of_match = of_match_ptr("ldo4"), > .id = LP8720_ID_LDO4, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl), > .volt_table = lp8720_ldo4_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -578,6 +585,7 @@ static struct regulator_desc lp8720_regulator_desc[] > = { > .of_match = of_match_ptr("ldo5"), > .id = LP8720_ID_LDO5, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -592,6 +600,7 @@ static struct regulator_desc lp8720_regulator_desc[] > = { > .of_match = of_match_ptr("buck"), > .id = LP8720_ID_BUCK, > .ops = &lp8720_buck_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp8720_buck_vtbl), > .volt_table = lp8720_buck_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -607,6 +616,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("ldo1"), > .id = LP8725_ID_LDO1, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -621,6 +631,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("ldo2"), > .id = LP8725_ID_LDO2, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -635,6 +646,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("ldo3"), > .id = LP8725_ID_LDO3, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -649,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("ldo4"), > .id = LP8725_ID_LDO4, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -663,6 +676,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("ldo5"), > .id = LP8725_ID_LDO5, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table = lp872x_ldo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -677,6 +691,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("lilo1"), > .id = LP8725_ID_LILO1, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl), > .volt_table = lp8725_lilo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -691,6 +706,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("lilo2"), > .id = LP8725_ID_LILO2, > .ops = &lp872x_ldo_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl), > .volt_table = lp8725_lilo_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -705,6 +721,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("buck1"), > .id = LP8725_ID_BUCK1, > .ops = &lp8725_buck_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl), > .volt_table = lp8725_buck_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -717,6 +734,7 @@ static struct regulator_desc lp8725_regulator_desc[] > = { > .of_match = of_match_ptr("buck2"), > .id = LP8725_ID_BUCK2, > .ops = &lp8725_buck_ops, > + .auto_runtime_pm = 1, > .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl), > .volt_table = lp8725_buck_vtbl, > .type = REGULATOR_VOLTAGE, > @@ -985,9 +1003,63 @@ static int lp872x_probe(struct i2c_client *cl, > const struct i2c_device_id *id) > if (ret) > return ret; > > - return lp872x_regulator_register(lp); > + pm_runtime_enable(&cl->dev); > + > + ret = lp872x_regulator_register(lp); > + if (ret) { > + pm_runtime_disable(&cl->dev); > + return ret; > + } > + > + return 0; > +} > + > +static int lp872x_runtime_suspend(struct device *dev) > +{ > + struct lp872x *lp = i2c_get_clientdata(to_i2c_client(dev)); > + struct gpio_desc *gpiod; > + int gpio; > + > + printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio); > + > + gpio = lp->pdata->enable_gpio; > + if (!gpio_is_valid(gpio)) > + return 0; > + > + gpiod = gpio_to_desc(gpio); > + gpiod_set_value(gpiod, 0); > + > + return 0; > +} > + > +static int lp872x_runtime_resume(struct device *dev) > +{ > + struct lp872x *lp = i2c_get_clientdata(to_i2c_client(dev)); > + struct gpio_desc *gpiod; > + int gpio; > + > + printk(KERN_ERR "%s() %d\n", __func__, lp->pdata->enable_gpio); > + > + gpio = lp->pdata->enable_gpio; > + if (!gpio_is_valid(gpio)) > + return 0; > + > + gpiod = gpio_to_desc(gpio); > + gpiod_set_value(gpiod, 0); > + > + /* Each chip has a different enable delay. */ > + if (lp->chipid == LP8720) > + usleep_range(LP8720_ENABLE_DELAY, 1.5 * > LP8720_ENABLE_DELAY); > + else > + usleep_range(LP8725_ENABLE_DELAY, 1.5 * > LP8725_ENABLE_DELAY); > + > + return 0; > } > > +static const struct dev_pm_ops lp872x_pm_ops = { > + SET_RUNTIME_PM_OPS(lp872x_runtime_suspend, > lp872x_runtime_resume, NULL) > +}; > + > static const struct of_device_id lp872x_dt_ids[] = { > { .compatible = "ti,lp8720", }, > { .compatible = "ti,lp8725", }, > @@ -1006,6 +1078,7 @@ static struct i2c_driver lp872x_driver = { > .driver = { > .name = "lp872x", > .of_match_table = of_match_ptr(lp872x_dt_ids), > + .pm = &lp872x_pm_ops, > }, > .probe = lp872x_probe, > .id_table = lp872x_ids,
[unhandled content-type:application/pgp-signature] | |