| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1455305051.2760.8.camel@collins>
Date: Fri, 12 Feb 2016 20:24:11 +0100
From: Paul Kocialkowski <contact@...lk.fr>
To: Mark Brown <broonie@...nel.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, linux-kernel@...r.kernel.org,
"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM
support
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@...nel.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,
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists