[PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[Ulf Hansson]

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

Changes in v2: [By Ulf Hansson]
	- I have picked up the series from Geert [1] and converted it into use
	the WAKEUP_PATH driver PM flag. This includes some minor changes to each
	patch and updates to the changelogs.
	- An important note, the WAKEUP_PATH driver PM flag is introduced in a
	separate series [2], not yet applied, so @subject series depends on it.
	- One more note, two of the patches has a checkpatch error, however I
	did not fix them, becuase I think that should be done separate.

[1]
https://lkml.org/lkml/2017/11/9/382
[2]
https://marc.info/?l=linux-pm&m=151454744124661&w=2

More information below, picked from Geert's previous cover letter.

Kind regards
Uffe


Hi all,

If an interrupt controller in a Renesas ARM SoC is part of a Clock
Domain, and it is part of the wakeup path, it must be kept active during
system suspend.

Currently this is handled in all interrupt controller drivers by
explicitly increasing the use count of the module clock when the device
is part of the wakeup path.  However, this explicit clock handling is
merely a workaround for a failure to properly communicate wakeup
information to the device core.

Hence this series fixes the affected drivers by setting the devices'
power.wakeup_path fields instead, to indicate they are part of the
wakeup path.  Depending on the PM Domain's active_wakeup configuration,
the genpd core code will keep the device enabled (and the clock running)
during system suspend when needed.

Note that most of these patches depend on the series "[PATCH v2 0/3] PM
/ Domain: renesas: Fix active wakeup behavior", hence they should not be
applied yet.

This has been tested on r8a73a4/ape6evm, r8a7740/armadillo,
r8a7791/koelsch, r8a7795/salvator-x and -xs, r8a7796/salvator-x, and
sh73a0/kzm9g.

Thanks for your comments!

Geert Uytterhoeven (3):
  irqchip/renesas-intc-irqpin: Use WAKEUP_PATH driver PM flag
  irqchip/renesas-irqc: Use WAKEUP_PATH driver PM flag
  gpio: rcar: Use WAKEUP_PATH driver PM flag

 drivers/gpio/gpio-rcar.c                  | 40 +++++++++++------------------
 drivers/irqchip/irq-renesas-intc-irqpin.c | 42 +++++++++++--------------------
 drivers/irqchip/irq-renesas-irqc.c        | 32 +++++++++++------------
 3 files changed, 45 insertions(+), 69 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] irqchip/renesas-intc-irqpin: Use WAKEUP_PATH driver PM flag

[Ulf Hansson]

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

Since commit 705bc96c2c15313c ("irqchip: renesas-intc-irqpin: Add minimal
runtime PM support"), when an IRQ is used for wakeup, the INTC 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/irqchip/irq-renesas-intc-irqpin.c | 42 +++++++++++--------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
index 06f29cf..bfc2c5c 100644
--- a/drivers/irqchip/irq-renesas-intc-irqpin.c
+++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
@@ -17,7 +17,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -78,16 +77,14 @@ struct intc_irqpin_priv {
 	struct platform_device *pdev;
 	struct irq_chip irq_chip;
 	struct irq_domain *irq_domain;
-	struct clk *clk;
 	unsigned shared_irqs:1;
-	unsigned needs_clk:1;
+	unsigned wakeup_path:1;
 	u8 shared_irq_mask;
 };
 
 struct intc_irqpin_config {
 	unsigned int irlm_bit;
 	unsigned needs_irlm:1;
-	unsigned needs_clk:1;
 };
 
 static unsigned long intc_irqpin_read32(void __iomem *iomem)
@@ -287,15 +284,7 @@ static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on)
 	int hw_irq = irqd_to_hwirq(d);
 
 	irq_set_irq_wake(p->irq[hw_irq].requested_irq, on);
-
-	if (!p->clk)
-		return 0;
-
-	if (on)
-		clk_enable(p->clk);
-	else
-		clk_disable(p->clk);
-
+	p->wakeup_path = on;
 	return 0;
 }
 
@@ -365,12 +354,10 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
 static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
 	.irlm_bit = 23, /* ICR0.IRLM0 */
 	.needs_irlm = 1,
-	.needs_clk = 0,
 };
 
 static const struct intc_irqpin_config intc_irqpin_rmobile = {
 	.needs_irlm = 0,
-	.needs_clk = 1,
 };
 
 static const struct of_device_id intc_irqpin_dt_ids[] = {
@@ -422,18 +409,6 @@ static int intc_irqpin_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, p);
 
 	config = of_device_get_match_data(dev);
-	if (config)
-		p->needs_clk = config->needs_clk;
-
-	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);
 	pm_runtime_get_sync(dev);
@@ -602,12 +577,25 @@ static int intc_irqpin_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int intc_irqpin_suspend(struct device *dev)
+{
+	struct intc_irqpin_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(intc_irqpin_pm_ops, intc_irqpin_suspend, NULL);
+
 static struct platform_driver intc_irqpin_device_driver = {
 	.probe		= intc_irqpin_probe,
 	.remove		= intc_irqpin_remove,
 	.driver		= {
 		.name	= "renesas_intc_irqpin",
 		.of_match_table = intc_irqpin_dt_ids,
+		.pm	= &intc_irqpin_pm_ops,
 	}
 };
 
-- 
2.7.4

[PATCH v2 2/3] irqchip/renesas-irqc: Use WAKEUP_PATH driver PM flag

[Ulf Hansson]

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

Since commit 6f46aedb9c85873b ("irqchip: renesas-irqc: Add wake-up
support"), when an IRQ is used for wakeup, the INTC block's module clock 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/irqchip/irq-renesas-irqc.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-irqc.c b/drivers/irqchip/irq-renesas-irqc.c
index 52304b1..70d1409 100644
--- a/drivers/irqchip/irq-renesas-irqc.c
+++ b/drivers/irqchip/irq-renesas-irqc.c
@@ -17,7 +17,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
@@ -64,7 +63,7 @@ struct irqc_priv {
 	struct platform_device *pdev;
 	struct irq_chip_generic *gc;
 	struct irq_domain *irq_domain;
-	struct clk *clk;
+	unsigned wakeup_path:1;
 };
 
 static struct irqc_priv *irq_data_to_priv(struct irq_data *data)
@@ -111,15 +110,7 @@ static int irqc_irq_set_wake(struct irq_data *d, unsigned int on)
 	int hw_irq = irqd_to_hwirq(d);
 
 	irq_set_irq_wake(p->irq[hw_irq].requested_irq, on);
-
-	if (!p->clk)
-		return 0;
-
-	if (on)
-		clk_enable(p->clk);
-	else
-		clk_disable(p->clk);
-
+	p->wakeup_path = on;
 	return 0;
 }
 
@@ -159,12 +150,6 @@ static int irqc_probe(struct platform_device *pdev)
 	p->pdev = pdev;
 	platform_set_drvdata(pdev, p);
 
-	p->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(p->clk)) {
-		dev_warn(&pdev->dev, "unable to get clock\n");
-		p->clk = NULL;
-	}
-
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
@@ -276,6 +261,18 @@ static int irqc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int irqc_suspend(struct device *dev)
+{
+	struct irqc_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(irqc_pm_ops, irqc_suspend, NULL);
+
 static const struct of_device_id irqc_dt_ids[] = {
 	{ .compatible = "renesas,irqc", },
 	{},
@@ -288,6 +285,7 @@ static struct platform_driver irqc_device_driver = {
 	.driver		= {
 		.name	= "renesas_irqc",
 		.of_match_table	= irqc_dt_ids,
+		.pm	= &irqc_pm_ops,
 	}
 };
 
-- 
2.7.4

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

[Ulf Hansson]

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

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),
 	}
 };
-- 
2.7.4

Re: [PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[Rafael J. Wysocki]

On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Changes in v2: [By Ulf Hansson]
>         - I have picked up the series from Geert [1] and converted it into use
>         the WAKEUP_PATH driver PM flag. This includes some minor changes to each
>         patch and updates to the changelogs.
>         - An important note, the WAKEUP_PATH driver PM flag is introduced in a
>         separate series [2], not yet applied, so @subject series depends on it.
>         - One more note, two of the patches has a checkpatch error, however I
>         did not fix them, becuase I think that should be done separate.
>
> [1]
> https://lkml.org/lkml/2017/11/9/382
> [2]
> https://marc.info/?l=linux-pm&m=151454744124661&w=2
>
> More information below, picked from Geert's previous cover letter.
>
> Kind regards
> Uffe
>
>
> Hi all,
>
> If an interrupt controller in a Renesas ARM SoC is part of a Clock
> Domain, and it is part of the wakeup path, it must be kept active during
> system suspend.
>
> Currently this is handled in all interrupt controller drivers by
> explicitly increasing the use count of the module clock when the device
> is part of the wakeup path.  However, this explicit clock handling is
> merely a workaround for a failure to properly communicate wakeup
> information to the device core.
>
> Hence this series fixes the affected drivers by setting the devices'
> power.wakeup_path fields instead, to indicate they are part of the
> wakeup path.  Depending on the PM Domain's active_wakeup configuration,
> the genpd core code will keep the device enabled (and the clock running)
> during system suspend when needed.

However, there is a convention, documented in the kerneldoc comment of
device_init_wakeup(), by which devices participating in system wakeup
"passively" (like USB controllers and hubs) are expected to have it
enabled by default.

If that convention was followed by the devices in question here, the
wakeup_path bit would be set for them and no other code changes would
be necessary.  So is there any reason for not following it?

Thanks,
Rafael

Re: [PATCH v2 1/3] irqchip/renesas-intc-irqpin: Use WAKEUP_PATH driver PM flag

[Rafael J. Wysocki]

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 705bc96c2c15313c ("irqchip: renesas-intc-irqpin: Add minimal
> runtime PM support"), when an IRQ is used for wakeup, the INTC 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/irqchip/irq-renesas-intc-irqpin.c | 42 +++++++++++--------------------
>  1 file changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
> index 06f29cf..bfc2c5c 100644
> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
> @@ -17,7 +17,6 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>
> -#include <linux/clk.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -78,16 +77,14 @@ struct intc_irqpin_priv {
>         struct platform_device *pdev;
>         struct irq_chip irq_chip;
>         struct irq_domain *irq_domain;
> -       struct clk *clk;
>         unsigned shared_irqs:1;
> -       unsigned needs_clk:1;
> +       unsigned wakeup_path:1;
>         u8 shared_irq_mask;
>  };
>
>  struct intc_irqpin_config {
>         unsigned int irlm_bit;
>         unsigned needs_irlm:1;
> -       unsigned needs_clk:1;
>  };
>
>  static unsigned long intc_irqpin_read32(void __iomem *iomem)
> @@ -287,15 +284,7 @@ static int intc_irqpin_irq_set_wake(struct irq_data *d, unsigned int on)
>         int hw_irq = irqd_to_hwirq(d);
>
>         irq_set_irq_wake(p->irq[hw_irq].requested_irq, on);
> -
> -       if (!p->clk)
> -               return 0;
> -
> -       if (on)
> -               clk_enable(p->clk);
> -       else
> -               clk_disable(p->clk);
> -
> +       p->wakeup_path = on;
>         return 0;
>  }
>
> @@ -365,12 +354,10 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
>  static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
>         .irlm_bit = 23, /* ICR0.IRLM0 */
>         .needs_irlm = 1,
> -       .needs_clk = 0,
>  };
>
>  static const struct intc_irqpin_config intc_irqpin_rmobile = {
>         .needs_irlm = 0,
> -       .needs_clk = 1,
>  };
>
>  static const struct of_device_id intc_irqpin_dt_ids[] = {
> @@ -422,18 +409,6 @@ static int intc_irqpin_probe(struct platform_device *pdev)
>         platform_set_drvdata(pdev, p);
>
>         config = of_device_get_match_data(dev);
> -       if (config)
> -               p->needs_clk = config->needs_clk;
> -
> -       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);
>         pm_runtime_get_sync(dev);
> @@ -602,12 +577,25 @@ static int intc_irqpin_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int intc_irqpin_suspend(struct device *dev)
> +{
> +       struct intc_irqpin_priv *p = dev_get_drvdata(dev);
> +
> +       dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0);

If you want that thing to be a DPM_FLAG_, then please follow the rule
that these flags are only set once at the probe time.

> +       return 0;
> +}
> +#endif

Thanks,
Rafael

Re: [PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[Geert Uytterhoeven]

Hi Rafael,

On Sun, Dec 31, 2017 at 1:56 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>
>>
>> Changes in v2: [By Ulf Hansson]
>>         - I have picked up the series from Geert [1] and converted it into use
>>         the WAKEUP_PATH driver PM flag. This includes some minor changes to each
>>         patch and updates to the changelogs.
>>         - An important note, the WAKEUP_PATH driver PM flag is introduced in a
>>         separate series [2], not yet applied, so @subject series depends on it.
>>         - One more note, two of the patches has a checkpatch error, however I
>>         did not fix them, becuase I think that should be done separate.
>>
>> [1]
>> https://lkml.org/lkml/2017/11/9/382
>> [2]
>> https://marc.info/?l=linux-pm&m=151454744124661&w=2
>>
>> More information below, picked from Geert's previous cover letter.
>>
>> Kind regards
>> Uffe
>>
>>
>> Hi all,
>>
>> If an interrupt controller in a Renesas ARM SoC is part of a Clock
>> Domain, and it is part of the wakeup path, it must be kept active during
>> system suspend.
>>
>> Currently this is handled in all interrupt controller drivers by
>> explicitly increasing the use count of the module clock when the device
>> is part of the wakeup path.  However, this explicit clock handling is
>> merely a workaround for a failure to properly communicate wakeup
>> information to the device core.
>>
>> Hence this series fixes the affected drivers by setting the devices'
>> power.wakeup_path fields instead, to indicate they are part of the
>> wakeup path.  Depending on the PM Domain's active_wakeup configuration,
>> the genpd core code will keep the device enabled (and the clock running)
>> during system suspend when needed.
>
> However, there is a convention, documented in the kerneldoc comment of
> device_init_wakeup(), by which devices participating in system wakeup
> "passively" (like USB controllers and hubs) are expected to have it
> enabled by default.
>
> If that convention was followed by the devices in question here, the
> wakeup_path bit would be set for them and no other code changes would
> be necessary.  So is there any reason for not following it?

Yes there is.  The need to stay enabled during system suspend depends
on the consumer of the interrupt. It is controlled by the consumer using
the irq_chip.irq_set_wake() callback at runtime, and may change at runtime.

If the wakeup_path flag is always set, the interrupt controller will
never be suspended during system suspend, and thus waste power.

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 v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[Rafael J. Wysocki]

On Sun, Dec 31, 2017 at 10:22 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Rafael,
>
> On Sun, Dec 31, 2017 at 1:56 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>
>>>
>>> Changes in v2: [By Ulf Hansson]
>>>         - I have picked up the series from Geert [1] and converted it into use
>>>         the WAKEUP_PATH driver PM flag. This includes some minor changes to each
>>>         patch and updates to the changelogs.
>>>         - An important note, the WAKEUP_PATH driver PM flag is introduced in a
>>>         separate series [2], not yet applied, so @subject series depends on it.
>>>         - One more note, two of the patches has a checkpatch error, however I
>>>         did not fix them, becuase I think that should be done separate.
>>>
>>> [1]
>>> https://lkml.org/lkml/2017/11/9/382
>>> [2]
>>> https://marc.info/?l=linux-pm&m=151454744124661&w=2
>>>
>>> More information below, picked from Geert's previous cover letter.
>>>
>>> Kind regards
>>> Uffe
>>>
>>>
>>> Hi all,
>>>
>>> If an interrupt controller in a Renesas ARM SoC is part of a Clock
>>> Domain, and it is part of the wakeup path, it must be kept active during
>>> system suspend.
>>>
>>> Currently this is handled in all interrupt controller drivers by
>>> explicitly increasing the use count of the module clock when the device
>>> is part of the wakeup path.  However, this explicit clock handling is
>>> merely a workaround for a failure to properly communicate wakeup
>>> information to the device core.
>>>
>>> Hence this series fixes the affected drivers by setting the devices'
>>> power.wakeup_path fields instead, to indicate they are part of the
>>> wakeup path.  Depending on the PM Domain's active_wakeup configuration,
>>> the genpd core code will keep the device enabled (and the clock running)
>>> during system suspend when needed.
>>
>> However, there is a convention, documented in the kerneldoc comment of
>> device_init_wakeup(), by which devices participating in system wakeup
>> "passively" (like USB controllers and hubs) are expected to have it
>> enabled by default.
>>
>> If that convention was followed by the devices in question here, the
>> wakeup_path bit would be set for them and no other code changes would
>> be necessary.  So is there any reason for not following it?
>
> Yes there is.  The need to stay enabled during system suspend depends
> on the consumer of the interrupt. It is controlled by the consumer using
> the irq_chip.irq_set_wake() callback at runtime, and may change at runtime.
>
> If the wakeup_path flag is always set, the interrupt controller will
> never be suspended during system suspend, and thus waste power.

OK

For IRQ chips in particular, I think, you don't need add new fields to
struct dev_pm_info to make it work.

In ->suspend (or ->suspend_late, which may be better) you can check
the IRQD_WAKEUP_STATE flag of the irq_desc associated with the pin.
If that is set, you can simply set power.wakeup_path for the device
and that will make genpd skip it.  Wouldn't that work?

Thanks,
Rafael

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

[Linus Walleij]

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

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

[Geert Uytterhoeven]

Hi Linus,

On Tue, Jan 2, 2018 at 10:27 AM, Linus Walleij <linus.walleij@linaro.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]
>> 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?

Yes, it depends on (a) a clock patch queued in clk-next for v4.16, and (b) a PM
patch introducing WAKEUP_PATH.  Applying it prematurely will cause build
or runtime issues.

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 v2 3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 10:37 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Linus,
>
> On Tue, Jan 2, 2018 at 10:27 AM, Linus Walleij <linus.walleij@linaro.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]
>>> 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?
>
> Yes, it depends on (a) a clock patch queued in clk-next for v4.16, and (b) a PM
> patch introducing WAKEUP_PATH.  Applying it prematurely will cause build
> or runtime issues.

Plus, I'm not going to apply the WAKEUP_PATH thing just yet.

At least not in its current version.

Thanks,
Rafael

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

[Rafael J. Wysocki]

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>
> ---
>  drivers/gpio/gpio-rcar.c | 40 +++++++++++++++-------------------------
>  1 file changed, 15 insertions(+), 25 deletions(-)
>
> 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);

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

> +       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;

Thanks,
Rafael

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

[Geert Uytterhoeven]

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/).

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 v2 3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag

[Rafael J. Wysocki]

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

What's wrong with it?

Thanks,
Rafael

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

[Ulf Hansson]

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

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

[Ulf Hansson]

>>> 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.

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

[...]

Kind regards
Uffe

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

[Geert Uytterhoeven]

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?

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 v2 3/3] gpio: rcar: Use WAKEUP_PATH driver PM flag

[Geert Uytterhoeven]

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

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

[Ulf Hansson]

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

Re: [PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[Geert Uytterhoeven]

Hi Rafael,

On Sun, Dec 31, 2017 at 12:47 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Sun, Dec 31, 2017 at 10:22 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Sun, Dec 31, 2017 at 1:56 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>
>>>>
>>>> Changes in v2: [By Ulf Hansson]
>>>>         - I have picked up the series from Geert [1] and converted it into use
>>>>         the WAKEUP_PATH driver PM flag. This includes some minor changes to each
>>>>         patch and updates to the changelogs.
>>>>         - An important note, the WAKEUP_PATH driver PM flag is introduced in a
>>>>         separate series [2], not yet applied, so @subject series depends on it.
>>>>         - One more note, two of the patches has a checkpatch error, however I
>>>>         did not fix them, becuase I think that should be done separate.
>>>>
>>>> [1]
>>>> https://lkml.org/lkml/2017/11/9/382
>>>> [2]
>>>> https://marc.info/?l=linux-pm&m=151454744124661&w=2
>>>>
>>>> More information below, picked from Geert's previous cover letter.
>>>>
>>>> Kind regards
>>>> Uffe
>>>>
>>>>
>>>> Hi all,
>>>>
>>>> If an interrupt controller in a Renesas ARM SoC is part of a Clock
>>>> Domain, and it is part of the wakeup path, it must be kept active during
>>>> system suspend.
>>>>
>>>> Currently this is handled in all interrupt controller drivers by
>>>> explicitly increasing the use count of the module clock when the device
>>>> is part of the wakeup path.  However, this explicit clock handling is
>>>> merely a workaround for a failure to properly communicate wakeup
>>>> information to the device core.
>>>>
>>>> Hence this series fixes the affected drivers by setting the devices'
>>>> power.wakeup_path fields instead, to indicate they are part of the
>>>> wakeup path.  Depending on the PM Domain's active_wakeup configuration,
>>>> the genpd core code will keep the device enabled (and the clock running)
>>>> during system suspend when needed.
>>>
>>> However, there is a convention, documented in the kerneldoc comment of
>>> device_init_wakeup(), by which devices participating in system wakeup
>>> "passively" (like USB controllers and hubs) are expected to have it
>>> enabled by default.
>>>
>>> If that convention was followed by the devices in question here, the
>>> wakeup_path bit would be set for them and no other code changes would
>>> be necessary.  So is there any reason for not following it?
>>
>> Yes there is.  The need to stay enabled during system suspend depends
>> on the consumer of the interrupt. It is controlled by the consumer using
>> the irq_chip.irq_set_wake() callback at runtime, and may change at runtime.
>>
>> If the wakeup_path flag is always set, the interrupt controller will
>> never be suspended during system suspend, and thus waste power.
>
> OK
>
> For IRQ chips in particular, I think, you don't need add new fields to
> struct dev_pm_info to make it work.
>
> In ->suspend (or ->suspend_late, which may be better) you can check
> the IRQD_WAKEUP_STATE flag of the irq_desc associated with the pin.
> If that is set, you can simply set power.wakeup_path for the device
> and that will make genpd skip it.  Wouldn't that work?

The irq_desc is per pin, while suspend is called per platform device, which
contains one or more irq_chips, each serving one or more pins.
So checking for IRQD_WAKEUP_STATE means looping over all irq_descs
associated to the platform device.

Which made me realize {gpio_rcar,irqc,intc_irqpin}_priv.wakeup_path should
not be a flag, but a counter. Currently it works by luck, as we never have
two independent wake-up sources being routed through the same irqchip.

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