[PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Wolfram Sang]

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 :)

 drivers/i2c/busses/i2c-rcar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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;
 }
-- 
2.1.4

Re: [PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Alan Stern]

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

Re: [PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Geert Uytterhoeven]

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

Re: [PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Geert Uytterhoeven]

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