Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support

[Paul Kocialkowski]

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@xxxxxxxxxx>
> > ---
> > 
> > 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,

Attachment: signature.asc
Description: This is a digitally signed message part