[RFC,5/7] clk: mmp2: create a power domain for the GPU core
diff mbox series

Message ID 20190509211911.17998-6-lkundrak@v3.sk
State RFC, archived
Headers show
Series
  • [RFC,1/7] dt-bindings: clock: make marvell,mmp2 a power controller
Related show

Commit Message

Lubomir Rintel May 9, 2019, 9:19 p.m. UTC
The power management unit on MMP2 is able to gate clock for the GC860 GPU.

There's some special dance required to initialize the unit after the power
has been enabled. If not followed, either the GPU's memory interface or the
GPU core doesn't work or the SoC just hangs.

Once the power has been applied to the GPU block, it doesn't seem
possible to turn it off entirely and initialize again.

As the data sheet is missing, neither the details about initialization, nor
the reason why it can't be reinitialized are understood. The meaning of
most bits in the APMU_GPU register are partially described in [1].

[1] http://lists.laptop.org/pipermail/devel/2019-April/039053.html

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/arm/mach-mmp/Kconfig     |  2 +
 drivers/clk/mmp/clk-of-mmp2.c | 91 +++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

Comments

Ulf Hansson May 14, 2019, 8:55 a.m. UTC | #1
On Thu, 9 May 2019 at 23:19, Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The power management unit on MMP2 is able to gate clock for the GC860 GPU.
>
> There's some special dance required to initialize the unit after the power
> has been enabled. If not followed, either the GPU's memory interface or the
> GPU core doesn't work or the SoC just hangs.
>
> Once the power has been applied to the GPU block, it doesn't seem
> possible to turn it off entirely and initialize again.

I didn't quite see this constraint being managed in the code below.
Can you please elaborate on how you intend to deal with this?

We have a GENPD_FLAG_ALWAYS_ON, that might be useful here.

We also have new flag/behavior, likely to be merged soon [1], which
uses a flag called GENPD_FLAG_RPM_ALWAYS_ON....

>
> As the data sheet is missing, neither the details about initialization, nor
> the reason why it can't be reinitialized are understood. The meaning of
> most bits in the APMU_GPU register are partially described in [1].
>
> [1] http://lists.laptop.org/pipermail/devel/2019-April/039053.html
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  arch/arm/mach-mmp/Kconfig     |  2 +
>  drivers/clk/mmp/clk-of-mmp2.c | 91 +++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
>
> diff --git a/arch/arm/mach-mmp/Kconfig b/arch/arm/mach-mmp/Kconfig
> index 94500bed56ab..a0efaefbfc74 100644
> --- a/arch/arm/mach-mmp/Kconfig
> +++ b/arch/arm/mach-mmp/Kconfig
> @@ -124,6 +124,8 @@ config MACH_MMP2_DT
>         select PINCTRL_SINGLE
>         select ARCH_HAS_RESET_CONTROLLER
>         select CPU_PJ4
> +       select PM_GENERIC_DOMAINS if PM
> +       select PM_GENERIC_DOMAINS_OF if PM && OF

selecting PM_GENERIC_DOMAINS_OF isn't needed as it defaults to y, in
case PM_GENERIC_DOMAINS and OF.

See /kernel/power/Kconfig

>         help
>           Include support for Marvell MMP2 based platforms using
>           the device tree.
> diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
> index 45f94c89cdc1..7bbf70b2ccd2 100644
> --- a/drivers/clk/mmp/clk-of-mmp2.c
> +++ b/drivers/clk/mmp/clk-of-mmp2.c
> @@ -16,8 +16,11 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/of_address.h>
> +#include <linux/pm_domain.h>
> +#include <linux/clk.h>
>
>  #include <dt-bindings/clock/marvell,mmp2.h>
> +#include <dt-bindings/power/marvell,mmp2.h>
>
>  #include "clk.h"
>  #include "reset.h"
> @@ -58,6 +61,8 @@
>
>  struct mmp2_clk_unit {
>         struct mmp_clk_unit unit;
> +       struct genpd_onecell_data pd_data;
> +       struct generic_pm_domain gpu_pm_domain;
>         void __iomem *mpmu_base;
>         void __iomem *apmu_base;
>         void __iomem *apbc_base;
> @@ -325,6 +330,90 @@ static void mmp2_clk_reset_init(struct device_node *np,
>         mmp_clk_reset_register(np, cells, nr_resets);
>  }
>
> +static int mmp2_gpu_pm_domain_power_on(struct generic_pm_domain *genpd)
> +{
> +       struct mmp2_clk_unit *pxa_unit = container_of(genpd,
> +                       struct mmp2_clk_unit, gpu_pm_domain);
> +       struct mmp_clk_unit *unit = &pxa_unit->unit;
> +       void __iomem *reg = pxa_unit->apmu_base + APMU_GPU;
> +       unsigned long flags = 0;
> +       u32 tmp;
> +       int ret;
> +
> +       spin_lock_irqsave(&gpu_lock, flags);
> +
> +       /* Power up the module. */
> +       tmp = readl(reg);
> +       tmp &= ~0x8700;
> +       tmp |= 0x8600;
> +       writel(tmp, reg);
> +
> +       ret = clk_prepare_enable(unit->clk_table[MMP2_CLK_GPU_GC]);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare_enable(unit->clk_table[MMP2_CLK_GPU_BUS]);
> +       if (ret) {
> +               clk_disable_unprepare(unit->clk_table[MMP2_CLK_GPU_GC]);
> +               return ret;
> +       }
> +
> +       /* Disable isolation now that clocks are running. */
> +       tmp = readl(reg);
> +       tmp |= 0x100;
> +       writel(tmp, reg);
> +       udelay(1);
> +
> +       spin_unlock_irqrestore(&gpu_lock, flags);
> +
> +       return 0;
> +}
> +
> +static int mmp2_gpu_pm_domain_power_off(struct generic_pm_domain *genpd)
> +{
> +       struct mmp2_clk_unit *pxa_unit = container_of(genpd,
> +                       struct mmp2_clk_unit, gpu_pm_domain);
> +       struct mmp_clk_unit *unit = &pxa_unit->unit;
> +       void __iomem *reg = pxa_unit->apmu_base + APMU_GPU;
> +       unsigned long flags = 0;
> +       u32 tmp;
> +
> +       spin_lock_irqsave(&gpu_lock, flags);

Why is this lock needed?

> +
> +       /*
> +        * Re-enable isolation. We must not touch the other bits,
> +        * otherwise the * GPU hangs without a known way to recover.
> +        */
> +       tmp = readl(reg);
> +       tmp &= ~0x100;
> +       writel(tmp, reg);
> +       udelay(1);
> +
> +       clk_disable_unprepare(unit->clk_table[MMP2_CLK_GPU_BUS]);
> +       clk_disable_unprepare(unit->clk_table[MMP2_CLK_GPU_GC]);
> +
> +       spin_unlock_irqrestore(&gpu_lock, flags);
> +
> +       return 0;
> +}
> +
> +static struct generic_pm_domain *mmp2_pm_onecell_domains[MMP2_NR_POWER_DOMAINS];
> +
> +static void mmp2_pm_domain_init(struct device_node *np,
> +                       struct mmp2_clk_unit *pxa_unit)
> +{
> +       pm_genpd_init(&pxa_unit->gpu_pm_domain, NULL, true);

pm_genpd_init(), should be called after the below initialization has
been done. Please re-order this.

> +       pxa_unit->gpu_pm_domain.name = "GPU";
> +       pxa_unit->gpu_pm_domain.power_on = mmp2_gpu_pm_domain_power_on;
> +       pxa_unit->gpu_pm_domain.power_off = mmp2_gpu_pm_domain_power_off;
> +       mmp2_pm_onecell_domains[MMP2_POWER_DOMAIN_GPU]
> +                               = &pxa_unit->gpu_pm_domain;
> +
> +       pxa_unit->pd_data.domains = mmp2_pm_onecell_domains;
> +       pxa_unit->pd_data.num_domains = ARRAY_SIZE(mmp2_pm_onecell_domains);
> +       of_genpd_add_provider_onecell(np, &pxa_unit->pd_data);
> +}
> +
>  static void __init mmp2_clk_init(struct device_node *np)
>  {
>         struct mmp2_clk_unit *pxa_unit;
> @@ -351,6 +440,8 @@ static void __init mmp2_clk_init(struct device_node *np)
>                 goto unmap_apmu_region;
>         }
>
> +       mmp2_pm_domain_init(np, pxa_unit);
> +
>         mmp_clk_init(np, &pxa_unit->unit, MMP2_NR_CLKS);
>
>         mmp2_pll_init(pxa_unit);
> --
> 2.21.0
>

Kind regards
Uffe

[1]
https://www.spinics.net/lists/arm-kernel/msg724822.html

Patch
diff mbox series

diff --git a/arch/arm/mach-mmp/Kconfig b/arch/arm/mach-mmp/Kconfig
index 94500bed56ab..a0efaefbfc74 100644
--- a/arch/arm/mach-mmp/Kconfig
+++ b/arch/arm/mach-mmp/Kconfig
@@ -124,6 +124,8 @@  config MACH_MMP2_DT
 	select PINCTRL_SINGLE
 	select ARCH_HAS_RESET_CONTROLLER
 	select CPU_PJ4
+	select PM_GENERIC_DOMAINS if PM
+	select PM_GENERIC_DOMAINS_OF if PM && OF
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree.
diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
index 45f94c89cdc1..7bbf70b2ccd2 100644
--- a/drivers/clk/mmp/clk-of-mmp2.c
+++ b/drivers/clk/mmp/clk-of-mmp2.c
@@ -16,8 +16,11 @@ 
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/of_address.h>
+#include <linux/pm_domain.h>
+#include <linux/clk.h>
 
 #include <dt-bindings/clock/marvell,mmp2.h>
+#include <dt-bindings/power/marvell,mmp2.h>
 
 #include "clk.h"
 #include "reset.h"
@@ -58,6 +61,8 @@ 
 
 struct mmp2_clk_unit {
 	struct mmp_clk_unit unit;
+	struct genpd_onecell_data pd_data;
+	struct generic_pm_domain gpu_pm_domain;
 	void __iomem *mpmu_base;
 	void __iomem *apmu_base;
 	void __iomem *apbc_base;
@@ -325,6 +330,90 @@  static void mmp2_clk_reset_init(struct device_node *np,
 	mmp_clk_reset_register(np, cells, nr_resets);
 }
 
+static int mmp2_gpu_pm_domain_power_on(struct generic_pm_domain *genpd)
+{
+	struct mmp2_clk_unit *pxa_unit = container_of(genpd,
+			struct mmp2_clk_unit, gpu_pm_domain);
+	struct mmp_clk_unit *unit = &pxa_unit->unit;
+	void __iomem *reg = pxa_unit->apmu_base + APMU_GPU;
+	unsigned long flags = 0;
+	u32 tmp;
+	int ret;
+
+	spin_lock_irqsave(&gpu_lock, flags);
+
+	/* Power up the module. */
+	tmp = readl(reg);
+	tmp &= ~0x8700;
+	tmp |= 0x8600;
+	writel(tmp, reg);
+
+	ret = clk_prepare_enable(unit->clk_table[MMP2_CLK_GPU_GC]);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(unit->clk_table[MMP2_CLK_GPU_BUS]);
+	if (ret) {
+		clk_disable_unprepare(unit->clk_table[MMP2_CLK_GPU_GC]);
+		return ret;
+	}
+
+	/* Disable isolation now that clocks are running. */
+	tmp = readl(reg);
+	tmp |= 0x100;
+	writel(tmp, reg);
+	udelay(1);
+
+	spin_unlock_irqrestore(&gpu_lock, flags);
+
+	return 0;
+}
+
+static int mmp2_gpu_pm_domain_power_off(struct generic_pm_domain *genpd)
+{
+	struct mmp2_clk_unit *pxa_unit = container_of(genpd,
+			struct mmp2_clk_unit, gpu_pm_domain);
+	struct mmp_clk_unit *unit = &pxa_unit->unit;
+	void __iomem *reg = pxa_unit->apmu_base + APMU_GPU;
+	unsigned long flags = 0;
+	u32 tmp;
+
+	spin_lock_irqsave(&gpu_lock, flags);
+
+	/*
+	 * Re-enable isolation. We must not touch the other bits,
+	 * otherwise the * GPU hangs without a known way to recover.
+	 */
+	tmp = readl(reg);
+	tmp &= ~0x100;
+	writel(tmp, reg);
+	udelay(1);
+
+	clk_disable_unprepare(unit->clk_table[MMP2_CLK_GPU_BUS]);
+	clk_disable_unprepare(unit->clk_table[MMP2_CLK_GPU_GC]);
+
+	spin_unlock_irqrestore(&gpu_lock, flags);
+
+	return 0;
+}
+
+static struct generic_pm_domain *mmp2_pm_onecell_domains[MMP2_NR_POWER_DOMAINS];
+
+static void mmp2_pm_domain_init(struct device_node *np,
+			struct mmp2_clk_unit *pxa_unit)
+{
+	pm_genpd_init(&pxa_unit->gpu_pm_domain, NULL, true);
+	pxa_unit->gpu_pm_domain.name = "GPU";
+	pxa_unit->gpu_pm_domain.power_on = mmp2_gpu_pm_domain_power_on;
+	pxa_unit->gpu_pm_domain.power_off = mmp2_gpu_pm_domain_power_off;
+	mmp2_pm_onecell_domains[MMP2_POWER_DOMAIN_GPU]
+				= &pxa_unit->gpu_pm_domain;
+
+	pxa_unit->pd_data.domains = mmp2_pm_onecell_domains;
+	pxa_unit->pd_data.num_domains = ARRAY_SIZE(mmp2_pm_onecell_domains);
+	of_genpd_add_provider_onecell(np, &pxa_unit->pd_data);
+}
+
 static void __init mmp2_clk_init(struct device_node *np)
 {
 	struct mmp2_clk_unit *pxa_unit;
@@ -351,6 +440,8 @@  static void __init mmp2_clk_init(struct device_node *np)
 		goto unmap_apmu_region;
 	}
 
+	mmp2_pm_domain_init(np, pxa_unit);
+
 	mmp_clk_init(np, &pxa_unit->unit, MMP2_NR_CLKS);
 
 	mmp2_pll_init(pxa_unit);