[PATCH v7 0/5] i.MX6 PU power domain support

[PATCH v7 0/5] i.MX6 PU power domain support

[Philipp Zabel]

Hi,

this is a rebased and updated version of the series I sent previously:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/238377.html

It is based on the "PM / Domains: Generic OF-based support" series that Ulf
Hansson sent recently:

http://www.spinics.net/lists/arm-kernel/msg357003.html

Changes since v6:
 - imx6qdl.dtsi entry now uses clock name macros instead of index numbers

regards
Philipp

Philipp Zabel (5):
  Documentation: Add device tree bindings for Freescale i.MX GPC
  ARM: imx6: gpc: Add PU power domain for GPU/VPU
  ARM: dts: imx6qdl: Add power-domain information to gpc node
  ARM: dts: imx6sl: Add power-domain information to gpc node
  ARM: dts: imx6qdl: Allow disabling the PU regulator, add a enable ramp
    delay

 .../devicetree/bindings/power/fsl,imx-gpc.txt      |  54 ++++++
 arch/arm/boot/dts/imx6qdl.dtsi                     |  11 +-
 arch/arm/boot/dts/imx6sl.dtsi                      |   4 +
 arch/arm/mach-imx/Kconfig                          |   1 +
 arch/arm/mach-imx/gpc.c                            | 203 +++++++++++++++++++++
 5 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpc.txt

-- 
2.1.0.rc1

[PATCH v7 1/5] Documentation: Add device tree bindings for Freescale i.MX GPC

[Philipp Zabel]

The i.MX6 contains a power controller that controls power gating and
sequencing for the SoC's power domains.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/power/fsl,imx-gpc.txt      | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpc.txt

diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
new file mode 100644
index 0000000..eaa8c93
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
@@ -0,0 +1,54 @@
+Freescale i.MX General Power Controller
+=======================================
+
+The i.MX6Q General Power Control (GPC) block contains DVFS load tracking
+counters and Power Gating Control (PGC) for the CPU and PU (GPU/VPU) power
+domains.
+
+Required properties:
+- compatible: Should be "fsl,imx6q-gpc" or "fsl,imx6sl-gpc"
+- reg: should be register base and length as documented in the
+  datasheet
+- interrupts: Should contain GPC interrupt request 1
+- pu-supply: Link to the LDO regulator powering the PU power domain
+- clocks: Clock phandles to devices in the PU power domain that need
+	  to be enabled during domain power-up for reset propagation.
+- #power-domain-cells: Should be 1, see below:
+
+The gpc node is a power-controller as documented by the generic power domain
+bindings in Documentation/devicetree/bindings/power/power_domain.txt.
+
+Example:
+
+	gpc: gpc at 020dc000 {
+		compatible = "fsl,imx6q-gpc";
+		reg = <0x020dc000 0x4000>;
+		interrupts = <0 89 0x04 0 90 0x04>;
+		pu-supply = <&reg_pu>;
+		clocks = <&clks 122>, <&clks 74>, <&clks 121>,
+			 <&clks 26>, <&clks 143>, <&clks 168>;
+		#power-domain-cells = <1>;
+	};
+
+
+Specifying power domain for IP modules
+======================================
+
+IP cores belonging to a power domain should contain a 'power-domain' property
+that is a phandle pointing to the gpc device node and a DOMAIN_INDEX specifying
+the power domain the device belongs to.
+
+Example of a device that is part of the PU power domain:
+
+	vpu: vpu at 02040000 {
+		reg = <0x02040000 0x3c000>;
+		/* ... */
+		power-domain = <&gpc 1>;
+		/* ... */
+	};
+
+The following DOMAIN_INDEX values are valid for i.MX6Q:
+ARM_DOMAIN     0
+PU_DOMAIN      1
+The following additional DOMAIN_INDEX value is valid for i.MX6SL:
+DISPLAY_DOMAIN 2
-- 
2.1.0.rc1

[PATCH v7 1/5] Documentation: Add device tree bindings for Freescale i.MX GPC

[Geert Uytterhoeven]

Hi Philipp,

On Tue, Aug 26, 2014 at 3:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> @@ -0,0 +1,54 @@

> +IP cores belonging to a power domain should contain a 'power-domain' property

power-domains

> +that is a phandle pointing to the gpc device node and a DOMAIN_INDEX specifying
> +the power domain the device belongs to.
> +
> +Example of a device that is part of the PU power domain:
> +
> +       vpu: vpu at 02040000 {
> +               reg = <0x02040000 0x3c000>;
> +               /* ... */
> +               power-domain = <&gpc 1>;

power-domains

> +               /* ... */
> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH v7 1/5] Documentation: Add device tree bindings for Freescale i.MX GPC

[Philipp Zabel]

Hi Geert,

Am Freitag, den 05.09.2014, 13:47 +0200 schrieb Geert Uytterhoeven:
> Hi Philipp,
> 
> On Tue, Aug 26, 2014 at 3:46 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> > @@ -0,0 +1,54 @@
> 
> > +IP cores belonging to a power domain should contain a 'power-domain' property
> 
> power-domains
> 
> > +that is a phandle pointing to the gpc device node and a DOMAIN_INDEX specifying
> > +the power domain the device belongs to.
> > +
> > +Example of a device that is part of the PU power domain:
> > +
> > +       vpu: vpu at 02040000 {
> > +               reg = <0x02040000 0x3c000>;
> > +               /* ... */
> > +               power-domain = <&gpc 1>;
> 
> power-domains

Thanks, I'll change those.

regards
Philipp

[PATCH v7 2/5] ARM: imx6: gpc: Add PU power domain for GPU/VPU

[Philipp Zabel]

When generic pm domain support is enabled, the PGC can be used
to completely gate power to the PU power domain containing GPU3D,
GPU2D, and VPU cores.
This code triggers the PGC powerdown sequence to disable the GPU/VPU
isolation cells and gate power and then disables the PU regulator.
To reenable, the reverse powerup sequence is triggered after the PU
regulator is enabled again.
The GPU and VPU devices in the PU power domain temporarily need
to be clocked during powerup, so that the reset machinery can work.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/mach-imx/Kconfig |   1 +
 arch/arm/mach-imx/gpc.c   | 203 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 204 insertions(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 9de84a2..78d69cf 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -50,6 +50,7 @@ config HAVE_IMX_ANATOP
 
 config HAVE_IMX_GPC
 	bool
+	select PM_GENERIC_DOMAINS if PM
 
 config HAVE_IMX_MMDC
 	bool
diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 82ea74e..5eec828 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -10,19 +10,41 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regulator/consumer.h>
 #include <linux/irqchip/arm-gic.h>
 #include "common.h"
+#include "hardware.h"
 
+#define GPC_CNTR		0x000
 #define GPC_IMR1		0x008
+#define GPC_PGC_GPU_PDN		0x260
+#define GPC_PGC_GPU_PUPSCR	0x264
+#define GPC_PGC_GPU_PDNSCR	0x268
 #define GPC_PGC_CPU_PDN		0x2a0
 
 #define IMR_NUM			4
 
+#define GPU_VPU_PUP_REQ		BIT(1)
+#define GPU_VPU_PDN_REQ		BIT(0)
+
+#define GPC_CLK_MAX		6
+
+struct pu_domain {
+	struct generic_pm_domain base;
+	struct regulator *reg;
+	struct clk *clk[GPC_CLK_MAX];
+	int num_clks;
+};
+
 static void __iomem *gpc_base;
 static u32 gpc_wake_irqs[IMR_NUM];
 static u32 gpc_saved_imrs[IMR_NUM];
@@ -139,3 +161,184 @@ void __init imx_gpc_init(void)
 	gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
 	gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
 }
+
+#ifdef CONFIG_PM
+
+static int imx6q_pm_pu_power_off(struct generic_pm_domain *genpd)
+{
+	struct pu_domain *pu = container_of(genpd, struct pu_domain, base);
+	int iso, iso2sw;
+	u32 val;
+
+	/* Read ISO and ISO2SW power down delays */
+	val = readl_relaxed(gpc_base + GPC_PGC_GPU_PDNSCR);
+	iso = val & 0x3f;
+	iso2sw = (val >> 8) & 0x3f;
+
+	/* Gate off PU domain when GPU/VPU when powered down */
+	writel_relaxed(0x1, gpc_base + GPC_PGC_GPU_PDN);
+
+	/* Request GPC to power down GPU/VPU */
+	val = readl_relaxed(gpc_base + GPC_CNTR);
+	val |= GPU_VPU_PDN_REQ;
+	writel_relaxed(val, gpc_base + GPC_CNTR);
+
+	/* Wait ISO + ISO2SW IPG clock cycles */
+	ndelay((iso + iso2sw) * 1000 / 66);
+
+	regulator_disable(pu->reg);
+
+	return 0;
+}
+
+static int imx6q_pm_pu_power_on(struct generic_pm_domain *genpd)
+{
+	struct pu_domain *pu = container_of(genpd, struct pu_domain, base);
+	int i, ret, sw, sw2iso;
+	u32 val;
+
+	ret = regulator_enable(pu->reg);
+	if (ret) {
+		pr_err("%s: failed to enable regulator: %d\n", __func__, ret);
+		return ret;
+	}
+
+	/* Enable reset clocks for all devices in the PU domain */
+	for (i = 0; i < pu->num_clks; i++)
+		clk_prepare_enable(pu->clk[i]);
+
+	/* Gate off PU domain when GPU/VPU when powered down */
+	writel_relaxed(0x1, gpc_base + GPC_PGC_GPU_PDN);
+
+	/* Read ISO and ISO2SW power down delays */
+	val = readl_relaxed(gpc_base + GPC_PGC_GPU_PUPSCR);
+	sw = val & 0x3f;
+	sw2iso = (val >> 8) & 0x3f;
+
+	/* Request GPC to power up GPU/VPU */
+	val = readl_relaxed(gpc_base + GPC_CNTR);
+	val |= GPU_VPU_PUP_REQ;
+	writel_relaxed(val, gpc_base + GPC_CNTR);
+
+	/* Wait ISO + ISO2SW IPG clock cycles */
+	ndelay((sw + sw2iso) * 1000 / 66);
+
+	/* Disable reset clocks for all devices in the PU domain */
+	for (i = 0; i < pu->num_clks; i++)
+		clk_disable_unprepare(pu->clk[i]);
+
+	return 0;
+}
+
+static struct generic_pm_domain imx6q_arm_domain = {
+	.name = "ARM",
+};
+
+static struct pu_domain imx6q_pu_domain = {
+	.base = {
+		.name = "PU",
+		.power_off = imx6q_pm_pu_power_off,
+		.power_on = imx6q_pm_pu_power_on,
+		.power_off_latency_ns = 25000,
+		.power_on_latency_ns = 2000000,
+	},
+};
+
+static struct generic_pm_domain imx6sl_display_domain = {
+	.name = "DISPLAY",
+};
+
+static struct generic_pm_domain *imx_gpc_domains[] = {
+	&imx6q_arm_domain,
+	&imx6q_pu_domain.base,
+	&imx6sl_display_domain,
+};
+
+static struct genpd_onecell_data imx_gpc_onecell_data = {
+	.domains = imx_gpc_domains,
+	.domain_num = ARRAY_SIZE(imx_gpc_domains),
+};
+
+static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
+{
+	struct clk *clk;
+	bool is_off;
+	int i;
+
+	imx6q_pu_domain.base.of_node = dev->of_node;
+	imx6q_pu_domain.reg = pu_reg;
+
+	for (i = 0; ; i++) {
+		clk = of_clk_get(dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+		if (i >= GPC_CLK_MAX) {
+			dev_err(dev, "more than %d clocks\n", GPC_CLK_MAX);
+			goto clk_err;
+		}
+		imx6q_pu_domain.clk[i] = clk;
+	}
+	imx6q_pu_domain.num_clks = i;
+
+	is_off = IS_ENABLED(CONFIG_PM_RUNTIME);
+	if (is_off)
+		imx6q_pm_pu_power_off(&imx6q_pu_domain.base);
+
+	pm_genpd_init(&imx6q_pu_domain.base, NULL, is_off);
+	return of_genpd_add_provider(dev->of_node, of_genpd_xlate_onecell,
+				     &imx_gpc_onecell_data);
+
+clk_err:
+	while (i--)
+		clk_put(imx6q_pu_domain.clk[i]);
+	return -EINVAL;
+}
+
+#else
+static inline int imx_gpc_genpd_init(struct device *dev, struct regulator *reg)
+{
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static int imx_gpc_probe(struct platform_device *pdev)
+{
+	struct regulator *pu_reg;
+	int ret;
+
+	pu_reg = devm_regulator_get(&pdev->dev, "pu");
+	if (IS_ERR(pu_reg)) {
+		ret = PTR_ERR(pu_reg);
+		dev_err(&pdev->dev, "failed to get pu regulator: %d\n", ret);
+		return ret;
+	}
+
+	/* The regulator is initially enabled */
+	ret = regulator_enable(pu_reg);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable pu regulator: %d\n", ret);
+		return ret;
+	}
+	return imx_gpc_genpd_init(&pdev->dev, pu_reg);
+}
+
+static struct of_device_id imx_gpc_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-gpc" },
+	{ .compatible = "fsl,imx6sl-gpc" },
+	{ }
+};
+
+static struct platform_driver imx_gpc_driver = {
+	.driver = {
+		.name = "imx-gpc",
+		.owner = THIS_MODULE,
+		.of_match_table = imx_gpc_dt_ids,
+	},
+	.probe = imx_gpc_probe,
+};
+
+static int __init imx_pgc_init(void)
+{
+	return platform_driver_register(&imx_gpc_driver);
+}
+subsys_initcall(imx_pgc_init);
-- 
2.1.0.rc1

[PATCH v7 2/5] ARM: imx6: gpc: Add PU power domain for GPU/VPU

[Ulf Hansson]

On 26 August 2014 15:46, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> When generic pm domain support is enabled, the PGC can be used
> to completely gate power to the PU power domain containing GPU3D,
> GPU2D, and VPU cores.
> This code triggers the PGC powerdown sequence to disable the GPU/VPU
> isolation cells and gate power and then disables the PU regulator.
> To reenable, the reverse powerup sequence is triggered after the PU
> regulator is enabled again.
> The GPU and VPU devices in the PU power domain temporarily need
> to be clocked during powerup, so that the reset machinery can work.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  arch/arm/mach-imx/Kconfig |   1 +
>  arch/arm/mach-imx/gpc.c   | 203 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 204 insertions(+)
>
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 9de84a2..78d69cf 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -50,6 +50,7 @@ config HAVE_IMX_ANATOP
>
>  config HAVE_IMX_GPC
>         bool
> +       select PM_GENERIC_DOMAINS if PM
>
>  config HAVE_IMX_MMDC
>         bool
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index 82ea74e..5eec828 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -10,19 +10,41 @@
>   * http://www.gnu.org/copyleft/gpl.html
>   */
>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/irqchip/arm-gic.h>
>  #include "common.h"
> +#include "hardware.h"
>
> +#define GPC_CNTR               0x000
>  #define GPC_IMR1               0x008
> +#define GPC_PGC_GPU_PDN                0x260
> +#define GPC_PGC_GPU_PUPSCR     0x264
> +#define GPC_PGC_GPU_PDNSCR     0x268
>  #define GPC_PGC_CPU_PDN                0x2a0
>
>  #define IMR_NUM                        4
>
> +#define GPU_VPU_PUP_REQ                BIT(1)
> +#define GPU_VPU_PDN_REQ                BIT(0)
> +
> +#define GPC_CLK_MAX            6
> +
> +struct pu_domain {
> +       struct generic_pm_domain base;
> +       struct regulator *reg;
> +       struct clk *clk[GPC_CLK_MAX];
> +       int num_clks;
> +};
> +
>  static void __iomem *gpc_base;
>  static u32 gpc_wake_irqs[IMR_NUM];
>  static u32 gpc_saved_imrs[IMR_NUM];
> @@ -139,3 +161,184 @@ void __init imx_gpc_init(void)
>         gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
>         gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
>  }
> +
> +#ifdef CONFIG_PM

Hi Philipp,

Should this be CONFIG_PM_GENERIC_DOMAINS?

> +
> +static int imx6q_pm_pu_power_off(struct generic_pm_domain *genpd)
> +{
> +       struct pu_domain *pu = container_of(genpd, struct pu_domain, base);
> +       int iso, iso2sw;
> +       u32 val;
> +
> +       /* Read ISO and ISO2SW power down delays */
> +       val = readl_relaxed(gpc_base + GPC_PGC_GPU_PDNSCR);
> +       iso = val & 0x3f;
> +       iso2sw = (val >> 8) & 0x3f;
> +
> +       /* Gate off PU domain when GPU/VPU when powered down */
> +       writel_relaxed(0x1, gpc_base + GPC_PGC_GPU_PDN);
> +
> +       /* Request GPC to power down GPU/VPU */
> +       val = readl_relaxed(gpc_base + GPC_CNTR);
> +       val |= GPU_VPU_PDN_REQ;
> +       writel_relaxed(val, gpc_base + GPC_CNTR);
> +
> +       /* Wait ISO + ISO2SW IPG clock cycles */
> +       ndelay((iso + iso2sw) * 1000 / 66);
> +
> +       regulator_disable(pu->reg);
> +
> +       return 0;
> +}
> +
> +static int imx6q_pm_pu_power_on(struct generic_pm_domain *genpd)
> +{
> +       struct pu_domain *pu = container_of(genpd, struct pu_domain, base);
> +       int i, ret, sw, sw2iso;
> +       u32 val;
> +
> +       ret = regulator_enable(pu->reg);
> +       if (ret) {
> +               pr_err("%s: failed to enable regulator: %d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       /* Enable reset clocks for all devices in the PU domain */
> +       for (i = 0; i < pu->num_clks; i++)
> +               clk_prepare_enable(pu->clk[i]);
> +
> +       /* Gate off PU domain when GPU/VPU when powered down */
> +       writel_relaxed(0x1, gpc_base + GPC_PGC_GPU_PDN);
> +
> +       /* Read ISO and ISO2SW power down delays */
> +       val = readl_relaxed(gpc_base + GPC_PGC_GPU_PUPSCR);
> +       sw = val & 0x3f;
> +       sw2iso = (val >> 8) & 0x3f;
> +
> +       /* Request GPC to power up GPU/VPU */
> +       val = readl_relaxed(gpc_base + GPC_CNTR);
> +       val |= GPU_VPU_PUP_REQ;
> +       writel_relaxed(val, gpc_base + GPC_CNTR);
> +
> +       /* Wait ISO + ISO2SW IPG clock cycles */
> +       ndelay((sw + sw2iso) * 1000 / 66);
> +
> +       /* Disable reset clocks for all devices in the PU domain */
> +       for (i = 0; i < pu->num_clks; i++)
> +               clk_disable_unprepare(pu->clk[i]);
> +
> +       return 0;
> +}
> +
> +static struct generic_pm_domain imx6q_arm_domain = {
> +       .name = "ARM",
> +};
> +
> +static struct pu_domain imx6q_pu_domain = {
> +       .base = {
> +               .name = "PU",
> +               .power_off = imx6q_pm_pu_power_off,
> +               .power_on = imx6q_pm_pu_power_on,
> +               .power_off_latency_ns = 25000,
> +               .power_on_latency_ns = 2000000,
> +       },
> +};
> +
> +static struct generic_pm_domain imx6sl_display_domain = {
> +       .name = "DISPLAY",
> +};
> +
> +static struct generic_pm_domain *imx_gpc_domains[] = {
> +       &imx6q_arm_domain,
> +       &imx6q_pu_domain.base,
> +       &imx6sl_display_domain,
> +};
> +
> +static struct genpd_onecell_data imx_gpc_onecell_data = {
> +       .domains = imx_gpc_domains,
> +       .domain_num = ARRAY_SIZE(imx_gpc_domains),
> +};
> +
> +static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
> +{
> +       struct clk *clk;
> +       bool is_off;
> +       int i;
> +
> +       imx6q_pu_domain.base.of_node = dev->of_node;
> +       imx6q_pu_domain.reg = pu_reg;
> +
> +       for (i = 0; ; i++) {
> +               clk = of_clk_get(dev->of_node, i);
> +               if (IS_ERR(clk))
> +                       break;
> +               if (i >= GPC_CLK_MAX) {
> +                       dev_err(dev, "more than %d clocks\n", GPC_CLK_MAX);
> +                       goto clk_err;
> +               }
> +               imx6q_pu_domain.clk[i] = clk;
> +       }
> +       imx6q_pu_domain.num_clks = i;
> +
> +       is_off = IS_ENABLED(CONFIG_PM_RUNTIME);
> +       if (is_off)
> +               imx6q_pm_pu_power_off(&imx6q_pu_domain.base);

Could you elaborate why is_off depends on CONFIG_PM_RUNTIME? That
seems strange to me. :-)

Additionally, I think it would be better to leave the domain "on"
state. Wouldn't that even be required for some drivers to be able to
succeed probing of these devices?

> +
> +       pm_genpd_init(&imx6q_pu_domain.base, NULL, is_off);
> +       return of_genpd_add_provider(dev->of_node, of_genpd_xlate_onecell,
> +                                    &imx_gpc_onecell_data);
> +
> +clk_err:
> +       while (i--)
> +               clk_put(imx6q_pu_domain.clk[i]);
> +       return -EINVAL;
> +}
> +
> +#else
> +static inline int imx_gpc_genpd_init(struct device *dev, struct regulator *reg)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static int imx_gpc_probe(struct platform_device *pdev)
> +{
> +       struct regulator *pu_reg;
> +       int ret;
> +
> +       pu_reg = devm_regulator_get(&pdev->dev, "pu");
> +       if (IS_ERR(pu_reg)) {
> +               ret = PTR_ERR(pu_reg);
> +               dev_err(&pdev->dev, "failed to get pu regulator: %d\n", ret);
> +               return ret;
> +       }
> +
> +       /* The regulator is initially enabled */
> +       ret = regulator_enable(pu_reg);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to enable pu regulator: %d\n", ret);
> +               return ret;
> +       }
> +       return imx_gpc_genpd_init(&pdev->dev, pu_reg);
> +}
> +
> +static struct of_device_id imx_gpc_dt_ids[] = {
> +       { .compatible = "fsl,imx6q-gpc" },
> +       { .compatible = "fsl,imx6sl-gpc" },
> +       { }
> +};
> +
> +static struct platform_driver imx_gpc_driver = {
> +       .driver = {
> +               .name = "imx-gpc",
> +               .owner = THIS_MODULE,
> +               .of_match_table = imx_gpc_dt_ids,
> +       },
> +       .probe = imx_gpc_probe,
> +};
> +
> +static int __init imx_pgc_init(void)
> +{
> +       return platform_driver_register(&imx_gpc_driver);
> +}
> +subsys_initcall(imx_pgc_init);

Could initialization of the generic power domain at subsys_init level,
mean that some other drivers may already be probed?

Are you sure none of those drivers handles devices within your power
domain, thus causing attachment of the power domain to fail?

Kind regards
Uffe

> --
> 2.1.0.rc1
>

[PATCH v7 2/5] ARM: imx6: gpc: Add PU power domain for GPU/VPU

[Philipp Zabel]

Hi Ulf,

thank you for the comments.

Am Dienstag, den 26.08.2014, 21:48 +0200 schrieb Ulf Hansson:
[...]
> > @@ -139,3 +161,184 @@ void __init imx_gpc_init(void)
> >         gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
> >         gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
> >  }
> > +
> > +#ifdef CONFIG_PM
> 
> Hi Philipp,
> 
> Should this be CONFIG_PM_GENERIC_DOMAINS?

Yes, I'll change this.

[...]
> > +static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
> > +{
> > +       struct clk *clk;
> > +       bool is_off;
> > +       int i;
> > +
> > +       imx6q_pu_domain.base.of_node = dev->of_node;
> > +       imx6q_pu_domain.reg = pu_reg;
> > +
> > +       for (i = 0; ; i++) {
> > +               clk = of_clk_get(dev->of_node, i);
> > +               if (IS_ERR(clk))
> > +                       break;
> > +               if (i >= GPC_CLK_MAX) {
> > +                       dev_err(dev, "more than %d clocks\n", GPC_CLK_MAX);
> > +                       goto clk_err;
> > +               }
> > +               imx6q_pu_domain.clk[i] = clk;
> > +       }
> > +       imx6q_pu_domain.num_clks = i;
> > +
> > +       is_off = IS_ENABLED(CONFIG_PM_RUNTIME);
> > +       if (is_off)
> > +               imx6q_pm_pu_power_off(&imx6q_pu_domain.base);
> 
> Could you elaborate why is_off depends on CONFIG_PM_RUNTIME? That
> seems strange to me. :-)
> 
> Additionally, I think it would be better to leave the domain "on"
> state. Wouldn't that even be required for some drivers to be able to
> succeed probing of these devices?

The devices in the PU power domain are Vivante GPUs and the CODA Video
Processing Unit. These are platform devices that don't need to be active
to enumerate before being probed. The drivers support runtime PM, so if
CONFIG_PM_RUNTIME is enabled, it is safe to disable the PU power domain
on boot. The drivers will probe and request for the power domain to be
enabled via runtime PM before accessing the hardware.

If CONFIG_PM_RUNTIME is disabled, the power domain needs to stay enabled
at all times.

[...]
> > +static struct platform_driver imx_gpc_driver = {
> > +       .driver = {
> > +               .name = "imx-gpc",
> > +               .owner = THIS_MODULE,
> > +               .of_match_table = imx_gpc_dt_ids,
> > +       },
> > +       .probe = imx_gpc_probe,
> > +};
> > +
> > +static int __init imx_pgc_init(void)
> > +{
> > +       return platform_driver_register(&imx_gpc_driver);
> > +}
> > +subsys_initcall(imx_pgc_init);
> 
> Could initialization of the generic power domain at subsys_init level,
> mean that some other drivers may already be probed?
>
> Are you sure none of those drivers handles devices within your power
> domain, thus causing attachment of the power domain to fail?

The GPU and VPU drivers are standard module_platform_drivers, those will
be probed later at device_initcall times.

regards
Philipp

[PATCH v7 2/5] ARM: imx6: gpc: Add PU power domain for GPU/VPU

[Ulf Hansson]

[...]

>> > +       is_off = IS_ENABLED(CONFIG_PM_RUNTIME);
>> > +       if (is_off)
>> > +               imx6q_pm_pu_power_off(&imx6q_pu_domain.base);
>>
>> Could you elaborate why is_off depends on CONFIG_PM_RUNTIME? That
>> seems strange to me. :-)
>>
>> Additionally, I think it would be better to leave the domain "on"
>> state. Wouldn't that even be required for some drivers to be able to
>> succeed probing of these devices?
>
> The devices in the PU power domain are Vivante GPUs and the CODA Video
> Processing Unit. These are platform devices that don't need to be active
> to enumerate before being probed. The drivers support runtime PM, so if
> CONFIG_PM_RUNTIME is enabled, it is safe to disable the PU power domain
> on boot. The drivers will probe and request for the power domain to be
> enabled via runtime PM before accessing the hardware.
>
> If CONFIG_PM_RUNTIME is disabled, the power domain needs to stay enabled
> at all times.

Hi Philipp,

Thanks for your reply, that certainly made it clear on what you would
like to accomplish.

Before discussing further consequences in genpd of this approach
(using IS_ENABLED(CONFIG_PM_RUNTIME)), I would be interested to see
how the runtime PM support is implemented for any of the drivers you
refer to. Do you mind pointing me to any of them?

Kind regards
Uffe

[PATCH v7 2/5] ARM: imx6: gpc: Add PU power domain for GPU/VPU

[Philipp Zabel]

On Tue, Sep 09, 2014 at 09:01:08PM +0200, Ulf Hansson wrote:
> [...]
> 
> >> > +       is_off = IS_ENABLED(CONFIG_PM_RUNTIME);
> >> > +       if (is_off)
> >> > +               imx6q_pm_pu_power_off(&imx6q_pu_domain.base);
> >>
> >> Could you elaborate why is_off depends on CONFIG_PM_RUNTIME? That
> >> seems strange to me. :-)
> >>
> >> Additionally, I think it would be better to leave the domain "on"
> >> state. Wouldn't that even be required for some drivers to be able to
> >> succeed probing of these devices?
> >
> > The devices in the PU power domain are Vivante GPUs and the CODA Video
> > Processing Unit. These are platform devices that don't need to be active
> > to enumerate before being probed. The drivers support runtime PM, so if
> > CONFIG_PM_RUNTIME is enabled, it is safe to disable the PU power domain
> > on boot. The drivers will probe and request for the power domain to be
> > enabled via runtime PM before accessing the hardware.
> >
> > If CONFIG_PM_RUNTIME is disabled, the power domain needs to stay enabled
> > at all times.
> 
> Hi Philipp,
> 
> Thanks for your reply, that certainly made it clear on what you would
> like to accomplish.
> 
> Before discussing further consequences in genpd of this approach
> (using IS_ENABLED(CONFIG_PM_RUNTIME)), I would be interested to see
> how the runtime PM support is implemented for any of the drivers you
> refer to. Do you mind pointing me to any of them?

Of course, the CODA driver is at drivers/media/platform/coda.c in mainline
kernels (moved to drivers/media/platform/coda-common.c in linux-next).

regards
Philipp

[PATCH v7 2/5] ARM: imx6: gpc: Add PU power domain for GPU/VPU

[Ulf Hansson]

On 9 September 2014 22:49, Philipp Zabel <pza@pengutronix.de> wrote:
> On Tue, Sep 09, 2014 at 09:01:08PM +0200, Ulf Hansson wrote:
>> [...]
>>
>> >> > +       is_off = IS_ENABLED(CONFIG_PM_RUNTIME);
>> >> > +       if (is_off)
>> >> > +               imx6q_pm_pu_power_off(&imx6q_pu_domain.base);
>> >>
>> >> Could you elaborate why is_off depends on CONFIG_PM_RUNTIME? That
>> >> seems strange to me. :-)
>> >>
>> >> Additionally, I think it would be better to leave the domain "on"
>> >> state. Wouldn't that even be required for some drivers to be able to
>> >> succeed probing of these devices?
>> >
>> > The devices in the PU power domain are Vivante GPUs and the CODA Video
>> > Processing Unit. These are platform devices that don't need to be active
>> > to enumerate before being probed. The drivers support runtime PM, so if
>> > CONFIG_PM_RUNTIME is enabled, it is safe to disable the PU power domain
>> > on boot. The drivers will probe and request for the power domain to be
>> > enabled via runtime PM before accessing the hardware.
>> >
>> > If CONFIG_PM_RUNTIME is disabled, the power domain needs to stay enabled
>> > at all times.
>>
>> Hi Philipp,
>>
>> Thanks for your reply, that certainly made it clear on what you would
>> like to accomplish.
>>
>> Before discussing further consequences in genpd of this approach
>> (using IS_ENABLED(CONFIG_PM_RUNTIME)), I would be interested to see
>> how the runtime PM support is implemented for any of the drivers you
>> refer to. Do you mind pointing me to any of them?
>
> Of course, the CODA driver is at drivers/media/platform/coda.c in mainline
> kernels (moved to drivers/media/platform/coda-common.c in linux-next).

Thanks! I went for a look and found some parts that concerns me. :-)

In principle, the coda platform driver will have a close dependency to
its corresponding PM domain state (powered or not powered) while
probing, which seems fragile.

In the case of CONFIG_PM_RUNTIME, the driver will not bring the coda
device into active state (runtime PM resumed) during probing. As you
also stated above, but this have consequences. Let me elaborate.

When the device gets attached to its PM domain, which is prior the
coda driver is probed - the device specific flag in genpd,
"need_restore", get assigned to a value depending on what status the
PM domain currently has. The "need_restore" flag is used internally in
genpd to understand when the device's runtime PM callbacks shall or
shall not be invoked.

As long as the state of the PM domain is powered off, when it gets
attached to the coda device - this works. Can you guarantee that's
true for all SoCs and PM domains that uses the coda driver?

It won't work if the PM domain is powered on while attaching occurs.
That's because the device's runtime PM state will not be in sync with
the "need_restore" flag in genpd. In other words, genpd will prevent
the coda driver's runtime PM callbacks from being invoked properly.

So this is a generic problem while using genpd and there are similar
issues for the opposite situation. I am already working on a fix for
it and will keep you posted. Let's see if that fix may effect this
patch as well.

Kind regards
Uffe

[PATCH v7 3/5] ARM: dts: imx6qdl: Add power-domain information to gpc node

[Philipp Zabel]

The PGC that is part of GPC controls isolation and power sequencing of the
power domains. The PU power domain will be handled by the generic pm domain
framework. It needs a phandle to the PU regulator to turn off power when
the domain is disabled, and a list of phandles to all clocks that must be
enabled during powerup for reset propagation.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v5:
 - Replaced clock indices with name macros
---
 arch/arm/boot/dts/imx6qdl.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index c701af9..0b1384c 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -670,6 +670,14 @@
 				reg = <0x020dc000 0x4000>;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>,
 					     <0 90 IRQ_TYPE_LEVEL_HIGH>;
+				pu-supply = <&reg_pu>;
+				clocks = <&clks IMX6QDL_CLK_GPU3D_CORE>,
+					 <&clks IMX6QDL_CLK_GPU3D_SHADER>,
+					 <&clks IMX6QDL_CLK_GPU2D_CORE>,
+					 <&clks IMX6QDL_CLK_GPU2D_AXI>,
+					 <&clks IMX6QDL_CLK_OPENVG_AXI>,
+					 <&clks IMX6QDL_CLK_VPU_AXI>;
+				#power-domain-cells = <1>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {
-- 
2.1.0.rc1

[PATCH v7 4/5] ARM: dts: imx6sl: Add power-domain information to gpc node

[Philipp Zabel]

The PGC that is part of GPC controls isolation and power sequencing of the
power domains. The PU power domain will be handled by the generic pm domain
framework. It needs a phandle to the PU regulator to turn off power when
the domain is disabled and a list of clocks to be enabled during powerup
for reset propagation.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/boot/dts/imx6sl.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index c75800c..7ae4847 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -581,6 +581,10 @@
 				compatible = "fsl,imx6sl-gpc", "fsl,imx6q-gpc";
 				reg = <0x020dc000 0x4000>;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
+				pu-supply = <&reg_pu>;
+				clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
+					 <&clks IMX6SL_CLK_GPU2D_PODF>;
+				#power-domain-cells = <1>;
 			};
 
 			gpr: iomuxc-gpr at 020e0000 {
-- 
2.1.0.rc1

[PATCH v7 5/5] ARM: dts: imx6qdl: Allow disabling the PU regulator, add a enable ramp delay

[Philipp Zabel]

The PU regulator is enabled during boot, but not necessarily always-on.
It can be disabled by the generic pm domain framework when the PU power
domain is shut down. The ramp delay of 150 us might be a bit conservative,
the value is taken from the Freescale kernel.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 0b1384c..d8d3d93 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -579,7 +579,8 @@
 					regulator-name = "vddpu";
 					regulator-min-microvolt = <725000>;
 					regulator-max-microvolt = <1450000>;
-					regulator-always-on;
+					regulator-enable-ramp-delay = <150>;
+					regulator-boot-on;
 					anatop-reg-offset = <0x140>;
 					anatop-vol-bit-shift = <9>;
 					anatop-vol-bit-width = <5>;
-- 
2.1.0.rc1