| Message ID | 1450263756-12013-1-git-send-email-wsa@the-dreams.de |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 16 Dec 2015, Wolfram Sang wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > When we also are I2C slave, we need to disable runtime PM because the > address detection mechanism needs to be active all the time. However, we > can reenable runtime PM once the slave instance was unregistered. So, > use pm_runtime_disable/enable to achieve this, since it has proper > refcounting. pm_runtime_allow/forbid is like a global switch which is > unsuitable here. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > I'd be grateful to get an ACK from a runtime PM expert to verify that my > assumptions match reality :) Yes, disabling runtime PM will do what you want. However you might consider using pm_runtime_get_sync() and pm_runtime_put() instead, because pm_runtime_enable() does not check to see if the device is idle and can be suspended right away. Alternatively, you can call pm_runtime_idle() after pm_runtime_enable(). pm_runtime_allow/forbid is even more unsuitable than you said, because it can be overridden by the user. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan, > > When we also are I2C slave, we need to disable runtime PM because the > > address detection mechanism needs to be active all the time. However, we > > can reenable runtime PM once the slave instance was unregistered. So, > > use pm_runtime_disable/enable to achieve this, since it has proper > > refcounting. pm_runtime_allow/forbid is like a global switch which is > > unsuitable here. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > I'd be grateful to get an ACK from a runtime PM expert to verify that my > > assumptions match reality :) > > Yes, disabling runtime PM will do what you want. However you might > consider using pm_runtime_get_sync() and pm_runtime_put() instead, > because pm_runtime_enable() does not check to see if the device is idle > and can be suspended right away. Alternatively, you can call > pm_runtime_idle() after pm_runtime_enable(). Thank you for your answer! I think I'll go for the get/put solution here. I have another case, may I ask your advice about this, too? When an I2C bus is marked in DT as multi-master, then RuntimePM also needs to be disabled, because arbitration detection needs to stay awake. I am currently implementing this for the i2c-rcar driver: - pm_runtime_enable(dev); + /* No RuntimePM in multi-master to keep arbitration working */ + if (!of_get_property(dev->of_node, "multi-master", NULL)) { + pm_runtime_enable(dev); + priv->flags |= ID_P_PM; + } + pm_runtime_get_sync(dev); ... @@ -664,7 +673,8 @@ static int rcar_i2c_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; i2c_del_adapter(&priv->adap); - pm_runtime_disable(dev); + if (priv->flags & ID_P_PM) + pm_runtime_disable(dev); return 0; } Here, I'd tend to keep using enable/disable, although get/put would probably also work. What is the rule of thumb using this pattern or the other? > pm_runtime_allow/forbid is even more unsuitable than you said, because > it can be overridden by the user. Yeah, I realized his today, too. Thanks! Wolfram
Hi Wolfram, On Wed, Dec 16, 2015 at 4:43 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > I have another case, may I ask your advice about this, too? When an I2C > bus is marked in DT as multi-master, then RuntimePM also needs to be > disabled, because arbitration detection needs to stay awake. I am > currently implementing this for the i2c-rcar driver: > > - pm_runtime_enable(dev); > + /* No RuntimePM in multi-master to keep arbitration working */ > + if (!of_get_property(dev->of_node, "multi-master", NULL)) { > + pm_runtime_enable(dev); > + priv->flags |= ID_P_PM; > + } > + > pm_runtime_get_sync(dev); > ... > > @@ -664,7 +673,8 @@ static int rcar_i2c_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > > i2c_del_adapter(&priv->adap); > - pm_runtime_disable(dev); > + if (priv->flags & ID_P_PM) > + pm_runtime_disable(dev); > > return 0; > } > > Here, I'd tend to keep using enable/disable, although get/put would > probably also work. What is the rule of thumb using this pattern or the > other? Have you actually tried the above? All our drivers rely on Runtime PM for the power/clock domain handling. I believe the pm_runtime_get_sync() won't do anything if you haven't enabled Runtime PM for the device, so the device's module clock won't be enabled at all. Hence I think you should add just add additional pm_runtime_get_sync()/ pm_runtime_put() calls in the driver's probe() and remove() methods if multi-master mode is enabled. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 16, 2015 at 04:55:28PM +0100, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Wed, Dec 16, 2015 at 4:43 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > > I have another case, may I ask your advice about this, too? When an I2C > > bus is marked in DT as multi-master, then RuntimePM also needs to be > > disabled, because arbitration detection needs to stay awake. I am > > currently implementing this for the i2c-rcar driver: > > > > - pm_runtime_enable(dev); > > + /* No RuntimePM in multi-master to keep arbitration working */ > > + if (!of_get_property(dev->of_node, "multi-master", NULL)) { > > + pm_runtime_enable(dev); > > + priv->flags |= ID_P_PM; > > + } > > + > > pm_runtime_get_sync(dev); > > ... > > > > @@ -664,7 +673,8 @@ static int rcar_i2c_remove(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > > > i2c_del_adapter(&priv->adap); > > - pm_runtime_disable(dev); > > + if (priv->flags & ID_P_PM) > > + pm_runtime_disable(dev); > > > > return 0; > > } > > > > Here, I'd tend to keep using enable/disable, although get/put would > > probably also work. What is the rule of thumb using this pattern or the > > other? > > Have you actually tried the above? Nope, I just finished the sketching phase when Alan's mail came along. > All our drivers rely on Runtime PM for the power/clock domain handling. > I believe the pm_runtime_get_sync() won't do anything if you haven't enabled > Runtime PM for the device, so the device's module clock won't be enabled at > all. This was another question I had: Is this a bug or a feature :) But if you say it is policy, I will count this as "feature" and switch to get/put here as well. Thanks, Wolfram
Hi Wolfram, On Wed, Dec 16, 2015 at 5:06 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Wed, Dec 16, 2015 at 04:55:28PM +0100, Geert Uytterhoeven wrote: >> On Wed, Dec 16, 2015 at 4:43 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> > I have another case, may I ask your advice about this, too? When an I2C >> > bus is marked in DT as multi-master, then RuntimePM also needs to be >> > disabled, because arbitration detection needs to stay awake. I am >> > currently implementing this for the i2c-rcar driver: >> > >> > - pm_runtime_enable(dev); >> > + /* No RuntimePM in multi-master to keep arbitration working */ >> > + if (!of_get_property(dev->of_node, "multi-master", NULL)) { >> > + pm_runtime_enable(dev); >> > + priv->flags |= ID_P_PM; >> > + } >> > + >> > pm_runtime_get_sync(dev); >> > ... >> > >> > @@ -664,7 +673,8 @@ static int rcar_i2c_remove(struct platform_device *pdev) >> > struct device *dev = &pdev->dev; >> > >> > i2c_del_adapter(&priv->adap); >> > - pm_runtime_disable(dev); >> > + if (priv->flags & ID_P_PM) >> > + pm_runtime_disable(dev); >> > >> > return 0; >> > } >> > >> > Here, I'd tend to keep using enable/disable, although get/put would >> > probably also work. What is the rule of thumb using this pattern or the >> > other? >> >> Have you actually tried the above? > > Nope, I just finished the sketching phase when Alan's mail came along. > >> All our drivers rely on Runtime PM for the power/clock domain handling. >> I believe the pm_runtime_get_sync() won't do anything if you haven't enabled >> Runtime PM for the device, so the device's module clock won't be enabled at >> all. > > This was another question I had: Is this a bug or a feature :) But if > you say it is policy, I will count this as "feature" and switch to > get/put here as well. It's a feature ;-) Without using Runtime PM, your driver has to care about clocks and power domains, which may or may not be present, depending on the SoC. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index b2389c492579cf..dc3343291e7497 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -533,7 +533,7 @@ static int rcar_reg_slave(struct i2c_client *slave) if (slave->flags & I2C_CLIENT_TEN) return -EAFNOSUPPORT; - pm_runtime_forbid(rcar_i2c_priv_to_dev(priv)); + pm_runtime_disable(rcar_i2c_priv_to_dev(priv)); priv->slave = slave; rcar_i2c_write(priv, ICSAR, slave->addr); @@ -555,7 +555,7 @@ static int rcar_unreg_slave(struct i2c_client *slave) priv->slave = NULL; - pm_runtime_allow(rcar_i2c_priv_to_dev(priv)); + pm_runtime_enable(rcar_i2c_priv_to_dev(priv)); return 0; }