From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751252AbcBLTYY (ORCPT ); Fri, 12 Feb 2016 14:24:24 -0500 Received: from gagarine.paulk.fr ([109.190.93.129]:57873 "EHLO gagarine.paulk.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751164AbcBLTYW (ORCPT ); Fri, 12 Feb 2016 14:24:22 -0500 Message-ID: <1455305051.2760.8.camel@collins> Subject: Re: [PATCH 2/2] regulator: core: Provide per-regulator runtime PM support From: Paul Kocialkowski To: Mark Brown Cc: Liam Girdwood , linux-kernel@vger.kernel.org, "Rafael J. Wysocki" Date: Fri, 12 Feb 2016 20:24:11 +0100 In-Reply-To: <1455051116.2757.15.camel@collins> References: <1453407851-8039-1-git-send-email-broonie@kernel.org> <1453407851-8039-2-git-send-email-broonie@kernel.org> <1455051116.2757.15.camel@collins> Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-LzhrybvjaG5f5r0299ZO" X-Mailer: Evolution 3.10.4-0ubuntu2+7.0trisquel1 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-LzhrybvjaG5f5r0299ZO Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Le mardi 09 f=C3=A9vrier 2016 =C3=A0 21:51 +0100, Paul Kocialkowski a =C3= =A9crit : > Le jeudi 21 janvier 2016 =C3=A0 20:24 +0000, Mark Brown a =C3=A9crit : > > 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. > >=20 > > Signed-off-by: Mark Brown > > --- > >=20 > > Not tested at all yet, pushing out for testing by others who have > > devices that could benefit from this. >=20 > Thanks for pushing this, I didn't think it would be that easy to > implement. >=20 > 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: >=20 > [ 1.943359] regulator disable, pm autosuspend > [ 1.949615] regulator enable, pm sync > [ 1.953491] lp872x_runtime_resume() 37 > [ 1.957519] lp872x_runtime_suspend() 37 >=20 > 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. >=20 > 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? >=20 > 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) > } > =20 > if (rdev->desc->auto_runtime_pm) { > + printk(KERN_ERR "regulator enable, pm sync\n"); > ret =3D 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; > } > =20 > - 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); > + } > =20 > /* 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 > #include > #include > +#include > +#include > +#include > #include > #include > #include > @@ -522,6 +525,7 @@ static struct regulator_desc lp8720_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo1"), > .id =3D LP8720_ID_LDO1, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -536,6 +540,7 @@ static struct regulator_desc lp8720_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo2"), > .id =3D LP8720_ID_LDO2, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -550,6 +555,7 @@ static struct regulator_desc lp8720_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo3"), > .id =3D LP8720_ID_LDO3, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -564,6 +570,7 @@ static struct regulator_desc lp8720_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo4"), > .id =3D LP8720_ID_LDO4, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp8720_ldo4_vtbl), > .volt_table =3D lp8720_ldo4_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -578,6 +585,7 @@ static struct regulator_desc lp8720_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo5"), > .id =3D LP8720_ID_LDO5, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -592,6 +600,7 @@ static struct regulator_desc lp8720_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("buck"), > .id =3D LP8720_ID_BUCK, > .ops =3D &lp8720_buck_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp8720_buck_vtbl), > .volt_table =3D lp8720_buck_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -607,6 +616,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo1"), > .id =3D LP8725_ID_LDO1, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -621,6 +631,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo2"), > .id =3D LP8725_ID_LDO2, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -635,6 +646,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo3"), > .id =3D LP8725_ID_LDO3, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -649,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo4"), > .id =3D LP8725_ID_LDO4, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -663,6 +676,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("ldo5"), > .id =3D LP8725_ID_LDO5, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp872x_ldo_vtbl), > .volt_table =3D lp872x_ldo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -677,6 +691,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("lilo1"), > .id =3D LP8725_ID_LILO1, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp8725_lilo_vtbl), > .volt_table =3D lp8725_lilo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -691,6 +706,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("lilo2"), > .id =3D LP8725_ID_LILO2, > .ops =3D &lp872x_ldo_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp8725_lilo_vtbl), > .volt_table =3D lp8725_lilo_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -705,6 +721,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("buck1"), > .id =3D LP8725_ID_BUCK1, > .ops =3D &lp8725_buck_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp8725_buck_vtbl), > .volt_table =3D lp8725_buck_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -717,6 +734,7 @@ static struct regulator_desc lp8725_regulator_desc[] > =3D { > .of_match =3D of_match_ptr("buck2"), > .id =3D LP8725_ID_BUCK2, > .ops =3D &lp8725_buck_ops, > + .auto_runtime_pm =3D 1, > .n_voltages =3D ARRAY_SIZE(lp8725_buck_vtbl), > .volt_table =3D lp8725_buck_vtbl, > .type =3D REGULATOR_VOLTAGE, > @@ -985,9 +1003,63 @@ static int lp872x_probe(struct i2c_client *cl, > const struct i2c_device_id *id) > if (ret) > return ret; > =20 > - return lp872x_regulator_register(lp); > + pm_runtime_enable(&cl->dev); > + > + ret =3D 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 =3D 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 =3D lp->pdata->enable_gpio; > + if (!gpio_is_valid(gpio)) > + return 0; > + > + gpiod =3D gpio_to_desc(gpio); > + gpiod_set_value(gpiod, 0); > + > + return 0; > +} > + > +static int lp872x_runtime_resume(struct device *dev) > +{ > + struct lp872x *lp =3D 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 =3D lp->pdata->enable_gpio; > + if (!gpio_is_valid(gpio)) > + return 0; > + > + gpiod =3D gpio_to_desc(gpio); > + gpiod_set_value(gpiod, 0); > + > + /* Each chip has a different enable delay. */ > + if (lp->chipid =3D=3D LP8720) > + usleep_range(LP8720_ENABLE_DELAY, 1.5 * > LP8720_ENABLE_DELAY); > + else > + usleep_range(LP8725_ENABLE_DELAY, 1.5 * > LP8725_ENABLE_DELAY); > + > + return 0; > } > =20 > +static const struct dev_pm_ops lp872x_pm_ops =3D { > + SET_RUNTIME_PM_OPS(lp872x_runtime_suspend, > lp872x_runtime_resume, NULL) > +}; > + > static const struct of_device_id lp872x_dt_ids[] =3D { > { .compatible =3D "ti,lp8720", }, > { .compatible =3D "ti,lp8725", }, > @@ -1006,6 +1078,7 @@ static struct i2c_driver lp872x_driver =3D { > .driver =3D { > .name =3D "lp872x", > .of_match_table =3D of_match_ptr(lp872x_dt_ids), > + .pm =3D &lp872x_pm_ops, > }, > .probe =3D lp872x_probe, > .id_table =3D lp872x_ids, --=-LzhrybvjaG5f5r0299ZO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAABAgAGBQJWvjFbAAoJEIT9weqP7pUMyZ4P/AxgiYx7kANaIwOvwWGdy8K8 1csEyWOrWiSYlAZH8rshvpG0benSeEpD2KbMD5LypaIjgXHEm5EqX94ElFO6hSqB yCzjnpbCEOYyYgOpANotS8hy0dB2EmiMi9+w09luq17kj9mKrE4cBEOOwznA9zjx SjCZXMWM+iFDgd/Z2AUtlM8KunlGGmrTmC5dbPxXQX1zSAzmCpoSW0xTznLDrHAN KKD515l/4+bk8BYioqGWWYT0WILTw2ViZ9eNS76HQESD26nk2Kdd6Q3MJewBFHTz AkxVqXnijid5DTN79kqy5MWOpzbBmzfNNc/8JfB1ytroquoT3x1i5A5Hy3j6eKwD Oxrd8NqiYeVHLyG3y8wW30cAgvLq/DJSGdDolV4XYHF2GH+coj+JzpmoRhlxxWLu Q8a70XD83TYy7oe5NmoLXDGu6KKKIw2bVZhWJMqjLWP1UC+aPQCrKSE/PsQGs6hw hykikEp6K9Q8Z/aldaf0kJEbtqPx8FG02OAMA2CHysA45SyKHDTFiWnrdlQ4rEUs kDGtGSaNu2XcC51zSekRn5LrqoPvI7/d8udDZXRJzk/zdYbQK6xaDudlKSe54oRM 8H5ntDRp1Q1DS4Nc6QrOIv8X225M06hvatznRrEMJAg28HsiPeoDQfXovKWvBg8T BFtjy/pafmViD1cVyUaC =/L6L -----END PGP SIGNATURE----- --=-LzhrybvjaG5f5r0299ZO--