[v2,3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag

Message ID 1514554304-18989-4-git-send-email-ulf.hansson@linaro.org
State New
Headers show
Series
  • renesas: irqchip: Use WAKEUP_PATH driver PM flag
Related show

Commit Message

Ulf Hansson Dec. 29, 2017, 1:31 p.m.
From: Geert Uytterhoeven <geert+renesas@glider.be>


Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable
when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's
module clock (if exists) is manually kept running during system suspend, to
make sure the device stays active.

However, this explicit clock handling is merely a workaround for a failure
to properly communicate wakeup information to the PM core. Instead, set the
WAKEUP_PATH driver PM flag to indicate that the device is part of the
wakeup path, which further also enables middle-layers and PM domains (like
genpd) to act on this.

In case the device is attached to genpd and depending on if it has an
active wakeup configuration, genpd will keep the device active (the clock
running) during system suspend when needed. This enables us to remove all
explicit clock handling code from the driver, so let's do that as well.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

[Ulf: Converted to use the WAKEUP_PATH driver PM flag]
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/gpio/gpio-rcar.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

-- 
2.7.4

Comments

Linus Walleij Jan. 2, 2018, 9:27 a.m. | #1
On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> From: Geert Uytterhoeven <geert+renesas@glider.be>

>

> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable

> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's

> module clock (if exists) is manually kept running during system suspend, to

> make sure the device stays active.

>

> However, this explicit clock handling is merely a workaround for a failure

> to properly communicate wakeup information to the PM core. Instead, set the

> WAKEUP_PATH driver PM flag to indicate that the device is part of the

> wakeup path, which further also enables middle-layers and PM domains (like

> genpd) to act on this.

>

> In case the device is attached to genpd and depending on if it has an

> active wakeup configuration, genpd will keep the device active (the clock

> running) during system suspend when needed. This enables us to remove all

> explicit clock handling code from the driver, so let's do that as well.

>

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>


Acked-by: Linus Walleij <linus.walleij@linaro.org>


I guess it is dependent on the other patches?

If you want me to just apply it to the GPIO tree, tell me.

Yours,
Linus Walleij
Ulf Hansson Jan. 2, 2018, 12:53 p.m. | #2
On 2 January 2018 at 11:48, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven

> <geert@linux-m68k.org> wrote:

>> Hi Rafael,

>>

>> On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:

>>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>>>> From: Geert Uytterhoeven <geert+renesas@glider.be>

>>>>

>>>> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable

>>>> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's

>>>> module clock (if exists) is manually kept running during system suspend, to

>>>> make sure the device stays active.

>>>>

>>>> However, this explicit clock handling is merely a workaround for a failure

>>>> to properly communicate wakeup information to the PM core. Instead, set the

>>>> WAKEUP_PATH driver PM flag to indicate that the device is part of the

>>>> wakeup path, which further also enables middle-layers and PM domains (like

>>>> genpd) to act on this.

>>>>

>>>> In case the device is attached to genpd and depending on if it has an

>>>> active wakeup configuration, genpd will keep the device active (the clock

>>>> running) during system suspend when needed. This enables us to remove all

>>>> explicit clock handling code from the driver, so let's do that as well.

>>>>

>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

>>>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]

>>

>> Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n?

>>

>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

>>

>>>> --- a/drivers/gpio/gpio-rcar.c

>>>> +++ b/drivers/gpio/gpio-rcar.c

>>

>>>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)

>>>>         return 0;

>>>>  }

>>>>

>>>> +#ifdef CONFIG_PM_SLEEP

>>>> +static int gpio_rcar_suspend(struct device *dev)

>>>> +{

>>>> +       struct gpio_rcar_priv *p = dev_get_drvdata(dev);

>>>> +

>>>> +       dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0);

>>>

>>> Why don't you simply set dev->power.wakeup_path here?

>>

>> That's what my v1 did (https://patchwork.kernel.org/patch/10050995/).

>

> I very much prefer this one. :-)


Okay!

The reason why I suggested inventing a new driver PM flag, was because
I consider the ->dev.power.wakeup_path, being a status flag/bit, owned
by the PM core. In other words, consumers of the flag are allowed to
look at it, but not change it.

Anyway, I am perfectly fine to drop the DPM_FLAG_WAKEUP_PATH thingy.
However, perhaps we should still add a helper function
(device_set_wakeup_path() or similar), which users can call to set the
flag?

>

> What's wrong with it?


It works, although I would rather change the assignment of the flag to
respect if the current value is true, something like this:

dev->power.wakeup_path = dev->power.wakeup_path || p->wakeup_path;

Kind regards
Uffe
Geert Uytterhoeven Jan. 2, 2018, 1:57 p.m. | #3
Hi Ulf,

On Tue, Jan 2, 2018 at 2:02 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

>>>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag]

>>

>> Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n?

>

> Oh, yes - correct!

>

> The code looks nicer, with the penalty of one static struct declared

> and not used, in case CONFIG_PM_SLEEP is unset.


At 23 pointers of 4 or 8 bytes each in 3 drivers, I don't consider this
insignificant.

Fortunately this driver is not used on RZ/A1, which you can run without
external RAM if you manage to fit everything in 10 MiB of SRAM...

> Should I revert back to your proposal, I am fine with whatever?


Yes please.

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
Ulf Hansson Jan. 2, 2018, 3:41 p.m. | #4
On 2 January 2018 at 14:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Jan 2, 2018 at 1:53 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> On 2 January 2018 at 11:48, Rafael J. Wysocki <rafael@kernel.org> wrote:

>>> On Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven

>>> <geert@linux-m68k.org> wrote:

>>>> On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:

>>>>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>>>>>> --- a/drivers/gpio/gpio-rcar.c

>>>>>> +++ b/drivers/gpio/gpio-rcar.c

>>>>

>>>>>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)

>>>>>>         return 0;

>>>>>>  }

>>>>>>

>>>>>> +#ifdef CONFIG_PM_SLEEP

>>>>>> +static int gpio_rcar_suspend(struct device *dev)

>>>>>> +{

>>>>>> +       struct gpio_rcar_priv *p = dev_get_drvdata(dev);

>>>>>> +

>>>>>> +       dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0);

>>>>>

>>>>> Why don't you simply set dev->power.wakeup_path here?

>>>>

>>>> That's what my v1 did (https://patchwork.kernel.org/patch/10050995/).

>>>

>>> I very much prefer this one. :-)

>>

>> Okay!

>

>>> What's wrong with it?

>>

>> It works, although I would rather change the assignment of the flag to

>> respect if the current value is true, something like this:

>>

>> dev->power.wakeup_path = dev->power.wakeup_path || p->wakeup_path;

>

> dev->power.wakeup_path |= p->wakeup_path?


Yeah, correct.

Br
Uffe

Patch

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index e76de57..d414511 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -14,7 +14,6 @@ 
  * GNU General Public License for more details.
  */
 
-#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/init.h>
@@ -37,10 +36,9 @@  struct gpio_rcar_priv {
 	struct platform_device *pdev;
 	struct gpio_chip gpio_chip;
 	struct irq_chip irq_chip;
-	struct clk *clk;
 	unsigned int irq_parent;
 	bool has_both_edge_trigger;
-	bool needs_clk;
+	bool wakeup_path;
 };
 
 #define IOINTSEL 0x00	/* General IO/Interrupt Switching Register */
@@ -186,14 +184,7 @@  static int gpio_rcar_irq_set_wake(struct irq_data *d, unsigned int on)
 		}
 	}
 
-	if (!p->clk)
-		return 0;
-
-	if (on)
-		clk_enable(p->clk);
-	else
-		clk_disable(p->clk);
-
+	p->wakeup_path = on;
 	return 0;
 }
 
@@ -330,17 +321,14 @@  static int gpio_rcar_direction_output(struct gpio_chip *chip, unsigned offset,
 
 struct gpio_rcar_info {
 	bool has_both_edge_trigger;
-	bool needs_clk;
 };
 
 static const struct gpio_rcar_info gpio_rcar_info_gen1 = {
 	.has_both_edge_trigger = false,
-	.needs_clk = false,
 };
 
 static const struct gpio_rcar_info gpio_rcar_info_gen2 = {
 	.has_both_edge_trigger = true,
-	.needs_clk = true,
 };
 
 static const struct of_device_id gpio_rcar_of_table[] = {
@@ -403,7 +391,6 @@  static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args);
 	*npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;
 	p->has_both_edge_trigger = info->has_both_edge_trigger;
-	p->needs_clk = info->needs_clk;
 
 	if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) {
 		dev_warn(&p->pdev->dev,
@@ -415,6 +402,18 @@  static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int gpio_rcar_suspend(struct device *dev)
+{
+	struct gpio_rcar_priv *p = dev_get_drvdata(dev);
+
+	dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0);
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, NULL);
+
 static int gpio_rcar_probe(struct platform_device *pdev)
 {
 	struct gpio_rcar_priv *p;
@@ -440,16 +439,6 @@  static int gpio_rcar_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, p);
 
-	p->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(p->clk)) {
-		if (p->needs_clk) {
-			dev_err(dev, "unable to get clock\n");
-			ret = PTR_ERR(p->clk);
-			goto err0;
-		}
-		p->clk = NULL;
-	}
-
 	pm_runtime_enable(dev);
 
 	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
@@ -536,6 +525,7 @@  static struct platform_driver gpio_rcar_device_driver = {
 	.remove		= gpio_rcar_remove,
 	.driver		= {
 		.name	= "gpio_rcar",
+		.pm     = &gpio_rcar_pm_ops,
 		.of_match_table = of_match_ptr(gpio_rcar_of_table),
 	}
 };