[RFC PATCH 0/1] sunxi: H616: add GPU power domain driver

[RFC PATCH 0/1] sunxi: H616: add GPU power domain driver

[Andre Przywara]

The Allwinner provided BSP code and experiments show that the Mali GPU
on the Allwinner H616 SoC needs to have a bit in one PRCM register
cleared to work. In the WIP graphics support patches, this was typically
covered by a U-Boot one-liner hack.

This patch here adds a proper power-domain driver for this one bit.
Since "power-domains" is a generic DT property, it already works on the
Panfrost driver side, without extra code. Said register is in the PRCM
MMIO range, which we already use in the R-CCU driver, so add the power
domain registration code there.
I mark this as RFC to get answers to some questions. Once people are
happy with the approach, I will send out a proper series with the
respective Kconfig and DT patches.

- Is modelling this as a power domain the right approach in the first
  place? Is that maybe just another reset bit? The logic seems backwards
  for that (bit set = disabled), but who knows?
- Is embedding this in the R-CCU driver the right way? I also have a
  version for a standalone driver in a separate file, but we have to
  artificially split the MMIO region to not conflict with the R-CCU
  driver, which looks arbitrary. If anyone has any information about
  the PRCM (register map), it would help to make an informed decision.
- Experiments in U-Boot identify bit 0 in 0x7010260 as behaving similar
  as our bit 0 in 0x7010254 here, so chances are there is another power
  domain for some other peripheral nearby. Also bits 0x7010250[9:0] are
  writable and stick, with setting bit 2 hanging the chip. So to allow
  further extensions without breaking compatibility, I made this a
  one-cell power domain ("power-domains = <&r_ccu 0>;"). Please let me
  know if this sounds overkill and a simple ("no cell") approach would be
  better.
- This patch makes the R-CCU driver dependent on CONFIG_PM. Shall we model
  this is a simple "select" or "depends on" in Kconfig, or shall the code
  be made conditional in the R-CCU driver, to allow compilation without
  PM support enabled?
- Shall any failure in registering the PPU driver also abort the R-CCU
  probe? Or do we treat this as optional?

I'd be grateful for any feedback.

Cheers,
Andre

Andre Przywara (1):
  clk: sunxi-ng: h6-r: add GPU power domain

 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 84 ++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

-- 
2.35.8


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 1/1] clk: sunxi-ng: h6-r: add GPU power domain

[Andre Przywara]

The Allwinner H616 features register 0x7010254 in the PRCM MMIO frame,
where bit 0 needs to be cleared to enable operation of the Mali GPU.
With this bit set (the reset default), any access to the Mali registers
hangs the bus and thus the whole system. The BSP code clears this bit
in U-Boot and their kernel never touches it again.

Register a power-domain device to control this bit. Since we claim this
MMIO region in the H6 R-CCU driver, add the code here, so that we don't
need to artificially split the MMIO range in the DT.
Since there seems to be at least one other register with similar behaviour
nearby (0x7010260), make the power domain take one cell, even though we
only support domain #0 for now.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 84 ++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
index 02b28cfc5525e..363fb7a71e9f5 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
@@ -4,9 +4,11 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 
 #include "ccu_common.h"
 #include "ccu_reset.h"
@@ -217,6 +219,86 @@ static const struct sunxi_ccu_desc sun50i_h616_r_ccu_desc = {
 	.num_resets	= ARRAY_SIZE(sun50i_h616_r_ccu_resets),
 };
 
+#define	PD_H616_GPU_REG			0x254
+
+struct sun50i_h616_ppu_pd {
+	struct generic_pm_domain	genpd;
+	void __iomem			*base;
+};
+
+#define to_sun50i_h616_ppu_pd(_genpd) \
+	container_of(_genpd, struct sun50i_h616_ppu_pd, genpd)
+
+static bool sun50i_h616_ppu_power_status(const struct sun50i_h616_ppu_pd *pd)
+{
+	return !readl(pd->base + PD_H616_GPU_REG);
+}
+
+static int sun50i_h616_ppu_pd_set_power(const struct sun50i_h616_ppu_pd *pd,
+					bool power_on)
+{
+	if (power_on)
+		writel(0, pd->base + PD_H616_GPU_REG);
+	else
+		writel(1, pd->base + PD_H616_GPU_REG);
+
+	return 0;
+}
+
+static int sun50i_h616_ppu_pd_power_on(struct generic_pm_domain *genpd)
+{
+	const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
+
+	return sun50i_h616_ppu_pd_set_power(pd, true);
+}
+
+static int sun50i_h616_ppu_pd_power_off(struct generic_pm_domain *genpd)
+{
+	const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
+
+	return sun50i_h616_ppu_pd_set_power(pd, false);
+}
+
+static int sun50i_h616_register_ppu(struct platform_device *pdev,
+				    void __iomem *base)
+{
+	struct device *dev = &pdev->dev;
+	struct genpd_onecell_data *ppu;
+	struct sun50i_h616_ppu_pd *pd;
+	int ret;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	ppu = devm_kzalloc(dev, sizeof(*ppu), GFP_KERNEL);
+	if (!ppu)
+		return -ENOMEM;
+
+	ppu->domains = devm_kzalloc(dev, sizeof(*ppu->domains), GFP_KERNEL);
+	if (!ppu->domains)
+		return -ENOMEM;
+
+	ppu->num_domains = 1;
+	pd->genpd.name		= "GPU";
+	pd->genpd.power_off	= sun50i_h616_ppu_pd_power_off;
+	pd->genpd.power_on	= sun50i_h616_ppu_pd_power_on;
+	pd->base		= base;
+
+	ret = pm_genpd_init(&pd->genpd, NULL, !sun50i_h616_ppu_power_status(pd));
+	if (ret) {
+		dev_warn(dev, "Failed to add GPU power domain: %d\n", ret);
+		return ret;
+	}
+
+	ppu->domains[0] = &pd->genpd;
+	ret = of_genpd_add_provider_onecell(dev->of_node, ppu);
+	if (ret)
+		dev_warn(dev, "Failed to add provider: %d\n", ret);
+
+	return 0;
+}
+
 static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
 {
 	const struct sunxi_ccu_desc *desc;
@@ -230,6 +312,8 @@ static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
 	if (IS_ERR(reg))
 		return PTR_ERR(reg);
 
+	sun50i_h616_register_ppu(pdev, reg);
+
 	return devm_sunxi_ccu_probe(&pdev->dev, reg, desc);
 }
 
-- 
2.35.8


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/1] clk: sunxi-ng: h6-r: add GPU power domain

[Stephen Boyd]

Quoting Andre Przywara (2024-02-25 08:06:16)
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> index 02b28cfc5525e..363fb7a71e9f5 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> @@ -217,6 +219,86 @@ static const struct sunxi_ccu_desc sun50i_h616_r_ccu_desc = {
[...]
> +static int sun50i_h616_register_ppu(struct platform_device *pdev,
> +                                   void __iomem *base)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct genpd_onecell_data *ppu;
> +       struct sun50i_h616_ppu_pd *pd;
> +       int ret;
> +
> +       pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +
> +       ppu = devm_kzalloc(dev, sizeof(*ppu), GFP_KERNEL);
> +       if (!ppu)
> +               return -ENOMEM;
> +
> +       ppu->domains = devm_kzalloc(dev, sizeof(*ppu->domains), GFP_KERNEL);
> +       if (!ppu->domains)
> +               return -ENOMEM;
> +
> +       ppu->num_domains = 1;
> +       pd->genpd.name          = "GPU";
> +       pd->genpd.power_off     = sun50i_h616_ppu_pd_power_off;
> +       pd->genpd.power_on      = sun50i_h616_ppu_pd_power_on;
> +       pd->base                = base;
> +
> +       ret = pm_genpd_init(&pd->genpd, NULL, !sun50i_h616_ppu_power_status(pd));
> +       if (ret) {
> +               dev_warn(dev, "Failed to add GPU power domain: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ppu->domains[0] = &pd->genpd;
> +       ret = of_genpd_add_provider_onecell(dev->of_node, ppu);

Is this provider removed somewhere when probe fails or the driver is
removed? It looks like the rest of the driver uses devm during probe.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/1] clk: sunxi-ng: h6-r: add GPU power domain

[Ulf Hansson]

On Sun, 25 Feb 2024 at 17:08, Andre Przywara <andre.przywara@arm.com> wrote:
>
> The Allwinner H616 features register 0x7010254 in the PRCM MMIO frame,
> where bit 0 needs to be cleared to enable operation of the Mali GPU.
> With this bit set (the reset default), any access to the Mali registers
> hangs the bus and thus the whole system. The BSP code clears this bit
> in U-Boot and their kernel never touches it again.

Is the bit representing a reset or power-rail? If it's a reset, it's
probably better to model it like that.

>
> Register a power-domain device to control this bit. Since we claim this
> MMIO region in the H6 R-CCU driver, add the code here, so that we don't
> need to artificially split the MMIO range in the DT.
> Since there seems to be at least one other register with similar behaviour
> nearby (0x7010260), make the power domain take one cell, even though we
> only support domain #0 for now.

Seems like we need some updated DT bindings too to cover this?

>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 84 ++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> index 02b28cfc5525e..363fb7a71e9f5 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> @@ -4,9 +4,11 @@
>   */
>
>  #include <linux/clk-provider.h>
> +#include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
>
>  #include "ccu_common.h"
>  #include "ccu_reset.h"
> @@ -217,6 +219,86 @@ static const struct sunxi_ccu_desc sun50i_h616_r_ccu_desc = {
>         .num_resets     = ARRAY_SIZE(sun50i_h616_r_ccu_resets),
>  };
>
> +#define        PD_H616_GPU_REG                 0x254
> +
> +struct sun50i_h616_ppu_pd {
> +       struct generic_pm_domain        genpd;
> +       void __iomem                    *base;
> +};
> +
> +#define to_sun50i_h616_ppu_pd(_genpd) \
> +       container_of(_genpd, struct sun50i_h616_ppu_pd, genpd)
> +
> +static bool sun50i_h616_ppu_power_status(const struct sun50i_h616_ppu_pd *pd)
> +{
> +       return !readl(pd->base + PD_H616_GPU_REG);
> +}
> +
> +static int sun50i_h616_ppu_pd_set_power(const struct sun50i_h616_ppu_pd *pd,
> +                                       bool power_on)
> +{
> +       if (power_on)
> +               writel(0, pd->base + PD_H616_GPU_REG);
> +       else
> +               writel(1, pd->base + PD_H616_GPU_REG);
> +
> +       return 0;
> +}
> +
> +static int sun50i_h616_ppu_pd_power_on(struct generic_pm_domain *genpd)
> +{
> +       const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
> +
> +       return sun50i_h616_ppu_pd_set_power(pd, true);
> +}
> +
> +static int sun50i_h616_ppu_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +       const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
> +
> +       return sun50i_h616_ppu_pd_set_power(pd, false);
> +}
> +
> +static int sun50i_h616_register_ppu(struct platform_device *pdev,
> +                                   void __iomem *base)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct genpd_onecell_data *ppu;
> +       struct sun50i_h616_ppu_pd *pd;
> +       int ret;
> +
> +       pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +
> +       ppu = devm_kzalloc(dev, sizeof(*ppu), GFP_KERNEL);
> +       if (!ppu)
> +               return -ENOMEM;
> +
> +       ppu->domains = devm_kzalloc(dev, sizeof(*ppu->domains), GFP_KERNEL);
> +       if (!ppu->domains)
> +               return -ENOMEM;
> +
> +       ppu->num_domains = 1;
> +       pd->genpd.name          = "GPU";
> +       pd->genpd.power_off     = sun50i_h616_ppu_pd_power_off;
> +       pd->genpd.power_on      = sun50i_h616_ppu_pd_power_on;
> +       pd->base                = base;
> +
> +       ret = pm_genpd_init(&pd->genpd, NULL, !sun50i_h616_ppu_power_status(pd));
> +       if (ret) {
> +               dev_warn(dev, "Failed to add GPU power domain: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ppu->domains[0] = &pd->genpd;
> +       ret = of_genpd_add_provider_onecell(dev->of_node, ppu);
> +       if (ret)
> +               dev_warn(dev, "Failed to add provider: %d\n", ret);
> +
> +       return 0;
> +}
> +
>  static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
>  {
>         const struct sunxi_ccu_desc *desc;
> @@ -230,6 +312,8 @@ static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
>         if (IS_ERR(reg))
>                 return PTR_ERR(reg);
>
> +       sun50i_h616_register_ppu(pdev, reg);
> +
>         return devm_sunxi_ccu_probe(&pdev->dev, reg, desc);
>  }
>

In general (for maintenance reasons) it's a good idea to put genpd
providers under drivers/pmdomain/*. It looks like that should work
this case too, right?

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/1] clk: sunxi-ng: h6-r: add GPU power domain

[Andre Przywara]

On Mon, 8 Apr 2024 14:55:04 +0200
Ulf Hansson <ulf.hansson@linaro.org> wrote:

Hi Ulf,

many thanks for having a look!

> On Sun, 25 Feb 2024 at 17:08, Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > The Allwinner H616 features register 0x7010254 in the PRCM MMIO frame,
> > where bit 0 needs to be cleared to enable operation of the Mali GPU.
> > With this bit set (the reset default), any access to the Mali registers
> > hangs the bus and thus the whole system. The BSP code clears this bit
> > in U-Boot and their kernel never touches it again.  
> 
> Is the bit representing a reset or power-rail? If it's a reset, it's
> probably better to model it like that.

So there is no real documentation about this bit (or the whole "PRCM"
device), but we are pretty sure it's a power rail and not a reset. There
are other similar rails in neighbouring registers, and some BSP code
hints at those being power gates. Samuel once condensed this to:
https://github.com/crust-firmware/crust/blob/master/platform/h6/include/platform/prcm.h

> > Register a power-domain device to control this bit. Since we claim this
> > MMIO region in the H6 R-CCU driver, add the code here, so that we don't
> > need to artificially split the MMIO range in the DT.
> > Since there seems to be at least one other register with similar behaviour
> > nearby (0x7010260), make the power domain take one cell, even though we
> > only support domain #0 for now.  
> 
> Seems like we need some updated DT bindings too to cover this?

That's right (and I mentioned this in the cover letter), but I omitted
this from this first RFC post. I will include binding patches in the v1
post.

> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 84 ++++++++++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> > index 02b28cfc5525e..363fb7a71e9f5 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> > @@ -4,9 +4,11 @@
> >   */
> >
> >  #include <linux/clk-provider.h>
> > +#include <linux/io.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> >
> >  #include "ccu_common.h"
> >  #include "ccu_reset.h"
> > @@ -217,6 +219,86 @@ static const struct sunxi_ccu_desc sun50i_h616_r_ccu_desc = {
> >         .num_resets     = ARRAY_SIZE(sun50i_h616_r_ccu_resets),
> >  };
> >
> > +#define        PD_H616_GPU_REG                 0x254
> > +
> > +struct sun50i_h616_ppu_pd {
> > +       struct generic_pm_domain        genpd;
> > +       void __iomem                    *base;
> > +};
> > +
> > +#define to_sun50i_h616_ppu_pd(_genpd) \
> > +       container_of(_genpd, struct sun50i_h616_ppu_pd, genpd)
> > +
> > +static bool sun50i_h616_ppu_power_status(const struct sun50i_h616_ppu_pd *pd)
> > +{
> > +       return !readl(pd->base + PD_H616_GPU_REG);
> > +}
> > +
> > +static int sun50i_h616_ppu_pd_set_power(const struct sun50i_h616_ppu_pd *pd,
> > +                                       bool power_on)
> > +{
> > +       if (power_on)
> > +               writel(0, pd->base + PD_H616_GPU_REG);
> > +       else
> > +               writel(1, pd->base + PD_H616_GPU_REG);
> > +
> > +       return 0;
> > +}
> > +
> > +static int sun50i_h616_ppu_pd_power_on(struct generic_pm_domain *genpd)
> > +{
> > +       const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
> > +
> > +       return sun50i_h616_ppu_pd_set_power(pd, true);
> > +}
> > +
> > +static int sun50i_h616_ppu_pd_power_off(struct generic_pm_domain *genpd)
> > +{
> > +       const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
> > +
> > +       return sun50i_h616_ppu_pd_set_power(pd, false);
> > +}
> > +
> > +static int sun50i_h616_register_ppu(struct platform_device *pdev,
> > +                                   void __iomem *base)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct genpd_onecell_data *ppu;
> > +       struct sun50i_h616_ppu_pd *pd;
> > +       int ret;
> > +
> > +       pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > +       if (!pd)
> > +               return -ENOMEM;
> > +
> > +       ppu = devm_kzalloc(dev, sizeof(*ppu), GFP_KERNEL);
> > +       if (!ppu)
> > +               return -ENOMEM;
> > +
> > +       ppu->domains = devm_kzalloc(dev, sizeof(*ppu->domains), GFP_KERNEL);
> > +       if (!ppu->domains)
> > +               return -ENOMEM;
> > +
> > +       ppu->num_domains = 1;
> > +       pd->genpd.name          = "GPU";
> > +       pd->genpd.power_off     = sun50i_h616_ppu_pd_power_off;
> > +       pd->genpd.power_on      = sun50i_h616_ppu_pd_power_on;
> > +       pd->base                = base;
> > +
> > +       ret = pm_genpd_init(&pd->genpd, NULL, !sun50i_h616_ppu_power_status(pd));
> > +       if (ret) {
> > +               dev_warn(dev, "Failed to add GPU power domain: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ppu->domains[0] = &pd->genpd;
> > +       ret = of_genpd_add_provider_onecell(dev->of_node, ppu);
> > +       if (ret)
> > +               dev_warn(dev, "Failed to add provider: %d\n", ret);
> > +
> > +       return 0;
> > +}
> > +
> >  static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
> >  {
> >         const struct sunxi_ccu_desc *desc;
> > @@ -230,6 +312,8 @@ static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
> >         if (IS_ERR(reg))
> >                 return PTR_ERR(reg);
> >
> > +       sun50i_h616_register_ppu(pdev, reg);
> > +
> >         return devm_sunxi_ccu_probe(&pdev->dev, reg, desc);
> >  }
> >  
> 
> In general (for maintenance reasons) it's a good idea to put genpd
> providers under drivers/pmdomain/*. It looks like that should work
> this case too, right?

Well, this question was the main reason to mark this as an RFC.
This patch here lives in the R-CCU driver, since this device is using
the same MMIO frame. The manual calls this device "PRCM" (Power Reset
Clock Management), so this covers more than just those essential
clocks, but those were all we needed and knew about until now.

For simplicity (since this is indeed just a single bit to flip), I
put the code into this existing R-CCU driver. This avoids splitting up
the MMIO frame (at some potentially arbitrary point), and avoids the
churn for an extra driver and DT node.

Now just last week we found another separate component in this PRCM: an
NMI interrupt controller[1], for which we already have a separate
binding (and driver) in the tree, so we split this MMIO range
up anyway.
So that means at this point we might as well make this a separate
driver, tied to a separate DT compatible string. I originally made a
version of this driver as a separate file in drivers/pmdomain, so will
just dig this out and post it.

Unless people disagree, then please let me know.

Thanks for the comments!

Cheers,
Andre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/1] clk: sunxi-ng: h6-r: add GPU power domain

[Ulf Hansson]

On Tue, 16 Apr 2024 at 01:00, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Mon, 8 Apr 2024 14:55:04 +0200
> Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Hi Ulf,
>
> many thanks for having a look!
>
> > On Sun, 25 Feb 2024 at 17:08, Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > The Allwinner H616 features register 0x7010254 in the PRCM MMIO frame,
> > > where bit 0 needs to be cleared to enable operation of the Mali GPU.
> > > With this bit set (the reset default), any access to the Mali registers
> > > hangs the bus and thus the whole system. The BSP code clears this bit
> > > in U-Boot and their kernel never touches it again.
> >
> > Is the bit representing a reset or power-rail? If it's a reset, it's
> > probably better to model it like that.
>
> So there is no real documentation about this bit (or the whole "PRCM"
> device), but we are pretty sure it's a power rail and not a reset. There
> are other similar rails in neighbouring registers, and some BSP code
> hints at those being power gates. Samuel once condensed this to:
> https://github.com/crust-firmware/crust/blob/master/platform/h6/include/platform/prcm.h

Alright, I guess modeling this as a power-domain should be fine then.

>
> > > Register a power-domain device to control this bit. Since we claim this
> > > MMIO region in the H6 R-CCU driver, add the code here, so that we don't
> > > need to artificially split the MMIO range in the DT.
> > > Since there seems to be at least one other register with similar behaviour
> > > nearby (0x7010260), make the power domain take one cell, even though we
> > > only support domain #0 for now.
> >
> > Seems like we need some updated DT bindings too to cover this?
>
> That's right (and I mentioned this in the cover letter), but I omitted
> this from this first RFC post. I will include binding patches in the v1
> post.
>
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 84 ++++++++++++++++++++++++++
> > >  1 file changed, 84 insertions(+)
> > >
> > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> > > index 02b28cfc5525e..363fb7a71e9f5 100644
> > > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> > > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
> > > @@ -4,9 +4,11 @@
> > >   */
> > >
> > >  #include <linux/clk-provider.h>
> > > +#include <linux/io.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/pm_domain.h>
> > >
> > >  #include "ccu_common.h"
> > >  #include "ccu_reset.h"
> > > @@ -217,6 +219,86 @@ static const struct sunxi_ccu_desc sun50i_h616_r_ccu_desc = {
> > >         .num_resets     = ARRAY_SIZE(sun50i_h616_r_ccu_resets),
> > >  };
> > >
> > > +#define        PD_H616_GPU_REG                 0x254
> > > +
> > > +struct sun50i_h616_ppu_pd {
> > > +       struct generic_pm_domain        genpd;
> > > +       void __iomem                    *base;
> > > +};
> > > +
> > > +#define to_sun50i_h616_ppu_pd(_genpd) \
> > > +       container_of(_genpd, struct sun50i_h616_ppu_pd, genpd)
> > > +
> > > +static bool sun50i_h616_ppu_power_status(const struct sun50i_h616_ppu_pd *pd)
> > > +{
> > > +       return !readl(pd->base + PD_H616_GPU_REG);
> > > +}
> > > +
> > > +static int sun50i_h616_ppu_pd_set_power(const struct sun50i_h616_ppu_pd *pd,
> > > +                                       bool power_on)
> > > +{
> > > +       if (power_on)
> > > +               writel(0, pd->base + PD_H616_GPU_REG);
> > > +       else
> > > +               writel(1, pd->base + PD_H616_GPU_REG);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int sun50i_h616_ppu_pd_power_on(struct generic_pm_domain *genpd)
> > > +{
> > > +       const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
> > > +
> > > +       return sun50i_h616_ppu_pd_set_power(pd, true);
> > > +}
> > > +
> > > +static int sun50i_h616_ppu_pd_power_off(struct generic_pm_domain *genpd)
> > > +{
> > > +       const struct sun50i_h616_ppu_pd *pd = to_sun50i_h616_ppu_pd(genpd);
> > > +
> > > +       return sun50i_h616_ppu_pd_set_power(pd, false);
> > > +}
> > > +
> > > +static int sun50i_h616_register_ppu(struct platform_device *pdev,
> > > +                                   void __iomem *base)
> > > +{
> > > +       struct device *dev = &pdev->dev;
> > > +       struct genpd_onecell_data *ppu;
> > > +       struct sun50i_h616_ppu_pd *pd;
> > > +       int ret;
> > > +
> > > +       pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > > +       if (!pd)
> > > +               return -ENOMEM;
> > > +
> > > +       ppu = devm_kzalloc(dev, sizeof(*ppu), GFP_KERNEL);
> > > +       if (!ppu)
> > > +               return -ENOMEM;
> > > +
> > > +       ppu->domains = devm_kzalloc(dev, sizeof(*ppu->domains), GFP_KERNEL);
> > > +       if (!ppu->domains)
> > > +               return -ENOMEM;
> > > +
> > > +       ppu->num_domains = 1;
> > > +       pd->genpd.name          = "GPU";
> > > +       pd->genpd.power_off     = sun50i_h616_ppu_pd_power_off;
> > > +       pd->genpd.power_on      = sun50i_h616_ppu_pd_power_on;
> > > +       pd->base                = base;
> > > +
> > > +       ret = pm_genpd_init(&pd->genpd, NULL, !sun50i_h616_ppu_power_status(pd));
> > > +       if (ret) {
> > > +               dev_warn(dev, "Failed to add GPU power domain: %d\n", ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       ppu->domains[0] = &pd->genpd;
> > > +       ret = of_genpd_add_provider_onecell(dev->of_node, ppu);
> > > +       if (ret)
> > > +               dev_warn(dev, "Failed to add provider: %d\n", ret);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
> > >  {
> > >         const struct sunxi_ccu_desc *desc;
> > > @@ -230,6 +312,8 @@ static int sun50i_h6_r_ccu_probe(struct platform_device *pdev)
> > >         if (IS_ERR(reg))
> > >                 return PTR_ERR(reg);
> > >
> > > +       sun50i_h616_register_ppu(pdev, reg);
> > > +
> > >         return devm_sunxi_ccu_probe(&pdev->dev, reg, desc);
> > >  }
> > >
> >
> > In general (for maintenance reasons) it's a good idea to put genpd
> > providers under drivers/pmdomain/*. It looks like that should work
> > this case too, right?
>
> Well, this question was the main reason to mark this as an RFC.
> This patch here lives in the R-CCU driver, since this device is using
> the same MMIO frame. The manual calls this device "PRCM" (Power Reset
> Clock Management), so this covers more than just those essential
> clocks, but those were all we needed and knew about until now.
>
> For simplicity (since this is indeed just a single bit to flip), I
> put the code into this existing R-CCU driver. This avoids splitting up
> the MMIO frame (at some potentially arbitrary point), and avoids the
> churn for an extra driver and DT node.

If this isn't a separate register range, we shouldn't have a separate
driver, but rather just to keep the genpd provider specific parts in
drivers/pmdomain/*.

Of course, it means that we need to share a couple of functions
between the subsystems, but that looks quite okay, I think.

>
> Now just last week we found another separate component in this PRCM: an
> NMI interrupt controller[1], for which we already have a separate
> binding (and driver) in the tree, so we split this MMIO range
> up anyway.
> So that means at this point we might as well make this a separate
> driver, tied to a separate DT compatible string. I originally made a
> version of this driver as a separate file in drivers/pmdomain, so will
> just dig this out and post it.

Seems reasonable to me and looks like the preferred option, assuming
the MMIO range can be split up independently from each other.

>
> Unless people disagree, then please let me know.
>
> Thanks for the comments!
>
> Cheers,
> Andre

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel