[PATCH v5 0/7] Fix RK3588 GPU power domain

[PATCH v5 0/7] Fix RK3588 GPU power domain

[Sebastian Reichel]

Hi,

I got a report, that the Linux kernel crashes on Rock 5B when the panthor
driver is loaded late after booting. The crash starts with the following
shortened error print:

rockchip-pm-domain fd8d8000.power-management:power-controller: failed to set domain 'gpu', val=0
rockchip-pm-domain fd8d8000.power-management:power-controller: failed to get ack on domain 'gpu', val=0xa9fff
SError Interrupt on CPU4, code 0x00000000be000411 -- SError

This series first does some cleanups in the Rockchip power domain
driver and changes the driver, so that it no longer tries to continue
when it fails to enable a domain. This gets rid of the SError interrupt
and long backtraces. But the kernel still hangs when it fails to enable
a power domain. I have not done further analysis to check if that can
be avoided.

Last but not least this provides a fix for the GPU power domain failing
to get enabled - after some testing from my side it seems to require the
GPU voltage supply to be enabled.

This introduces devm_of_regulator_get without the _optional suffix, since
that is more sensible for the Rockchip usecase. Longer explanation can be
seen in patch 6, which adds the handling to the Rockchip driver. My merge
suggestion would be that Mark provides an immutable branch.

The last patch, which updates the RK3588 board files should cover all RK3588
boards that are currently in Heiko's for-next branch. Any board missing the
update will behave as before, so it is perfectly fine not to update all DT
files at once (in case I missed any).

Changes since PATCHv4:
 * https://lore.kernel.org/linux-rockchip/20241022154508.63563-1-sebastian.reichel@collabora.com/
 * Rebase to Heiko's for-next branch
   - update DT patch to handle new RK3588(s) boards
   - make sure to use a clean topic branch without HDMI-RX code (Heiko Stübner)
 * Add Tested-by from Heiko Stübner

Changes since PATCHv3:
 * accidently none, sorry!

Changes since PATCHv2:
 * https://lore.kernel.org/linux-rockchip/20240919091834.83572-1-sebastian.reichel@collabora.com/
 * Rebase to 6.12-rc1 + devm_of_regulator_get_optional branch (Ulf Hansson, Chen-Yu Tsai)
  - Introduce devm_of_regulator_get()
  - Add code to only request regulators for domains needing them
 * Mention other platforms in the DT binding patch (Rob Murphy)
 * Update more RK3588 DT files (Jonas Karlman)

Changes since PATCHv1:
 * https://lore.kernel.org/all/20240910180530.47194-1-sebastian.reichel@collabora.com/
 * Collect Reviewed-by/Acked-by/Tested-by
 * swap first and second patch to avoid introducing and directly removing a mutex_unlock
 * fix spelling of indentation
 * fix double empty line after rockchip_pd_regulator_disable()

Greetings,

-- Sebastian

Sebastian Reichel (7):
  regulator: Add (devm_)of_regulator_get()
  pmdomain: rockchip: cleanup mutex handling in rockchip_pd_power
  pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors
  pmdomain: rockchip: reduce indentation in rockchip_pd_power
  dt-bindings: power: rockchip: add regulator support
  pmdomain: rockchip: add regulator support
  arm64: dts: rockchip: Add GPU power domain regulator dependency for
    RK3588

 .../power/rockchip,power-controller.yaml      |   3 +
 .../boot/dts/rockchip/rk3588-armsom-sige7.dts |   4 +
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |   2 +-
 .../boot/dts/rockchip/rk3588-coolpi-cm5.dtsi  |   4 +
 .../rockchip/rk3588-edgeble-neu6a-common.dtsi |   4 +
 .../boot/dts/rockchip/rk3588-evb1-v10.dts     |   4 +
 .../boot/dts/rockchip/rk3588-fet3588-c.dtsi   |   4 +
 .../rockchip/rk3588-friendlyelec-cm3588.dtsi  |   4 +
 .../arm64/boot/dts/rockchip/rk3588-jaguar.dts |   4 +
 .../boot/dts/rockchip/rk3588-nanopc-t6.dtsi   |   4 +
 .../boot/dts/rockchip/rk3588-ok3588-c.dts     |   4 +
 .../dts/rockchip/rk3588-orangepi-5-plus.dts   |   4 +
 .../boot/dts/rockchip/rk3588-quartzpro64.dts  |   4 +
 .../boot/dts/rockchip/rk3588-rock-5-itx.dts   |   4 +
 .../boot/dts/rockchip/rk3588-rock-5b.dts      |   4 +
 .../arm64/boot/dts/rockchip/rk3588-tiger.dtsi |   4 +
 .../boot/dts/rockchip/rk3588-toybrick-x0.dts  |   4 +
 .../boot/dts/rockchip/rk3588-turing-rk1.dtsi  |   4 +
 .../boot/dts/rockchip/rk3588s-coolpi-4b.dts   |   4 +
 .../boot/dts/rockchip/rk3588s-evb1-v10.dts    |   4 +
 .../dts/rockchip/rk3588s-gameforce-ace.dts    |   4 +
 .../dts/rockchip/rk3588s-indiedroid-nova.dts  |   4 +
 .../dts/rockchip/rk3588s-khadas-edge2.dts     |   4 +
 .../boot/dts/rockchip/rk3588s-odroid-m2.dts   |   4 +
 .../boot/dts/rockchip/rk3588s-rock-5a.dts     |   4 +
 .../boot/dts/rockchip/rk3588s-rock-5c.dts     |   4 +
 drivers/pmdomain/rockchip/pm-domains.c        | 190 +++++++++++-------
 drivers/regulator/devres.c                    |  17 ++
 drivers/regulator/of_regulator.c              |  21 ++
 include/linux/regulator/consumer.h            |   6 +
 30 files changed, 266 insertions(+), 69 deletions(-)

-- 
2.45.2

[PATCH v5 1/7] regulator: Add (devm_)of_regulator_get()

[Sebastian Reichel]

The Rockchip power-domain controller also plans to make use of
per-domain regulators similar to the MediaTek power-domain controller.
Since existing DTs are missing the regulator information, the kernel
should fallback to the automatically created dummy regulator if
necessary. Thus the version without the _optional suffix is needed.

The Rockchip driver plans to use the managed version, but to be
consistent with existing code the unmanaged version is added at the
same time.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/regulator/devres.c         | 17 +++++++++++++++++
 drivers/regulator/of_regulator.c   | 21 +++++++++++++++++++++
 include/linux/regulator/consumer.h |  6 ++++++
 3 files changed, 44 insertions(+)

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 36164aec30e8..a3a3ccc711fc 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -771,6 +771,23 @@ static struct regulator *_devm_of_regulator_get(struct device *dev, struct devic
 	return regulator;
 }
 
+/**
+ * devm_of_regulator_get - Resource managed of_regulator_get()
+ * @dev: device used for dev_printk() messages and resource lifetime management
+ * @node: device node for regulator "consumer"
+ * @id:  supply name or regulator ID.
+ *
+ * Managed of_regulator_get(). Regulators returned from this
+ * function are automatically regulator_put() on driver detach. See
+ * of_regulator_get() for more information.
+ */
+struct regulator *devm_of_regulator_get(struct device *dev, struct device_node *node,
+						 const char *id)
+{
+	return _devm_of_regulator_get(dev, node, id, NORMAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_of_regulator_get);
+
 /**
  * devm_of_regulator_get_optional - Resource managed of_regulator_get_optional()
  * @dev: device used for dev_printk() messages and resource lifetime management
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 3d85762beda6..31a5bacd99b4 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -682,6 +682,27 @@ struct regulator *_of_regulator_get(struct device *dev, struct device_node *node
 	return _regulator_get_common(r, dev, id, get_type);
 }
 
+/**
+ * of_regulator_get - get regulator via device tree lookup
+ * @dev: device used for dev_printk() messages
+ * @node: device node for regulator "consumer"
+ * @id: Supply name
+ *
+ * Return: pointer to struct regulator corresponding to the regulator producer,
+ *	   or PTR_ERR() encoded error number.
+ *
+ * This is intended for use by consumers that want to get a regulator
+ * supply directly from a device node. This will _not_ consider supply
+ * aliases. See regulator_dev_lookup().
+ */
+struct regulator *of_regulator_get(struct device *dev,
+					    struct device_node *node,
+					    const char *id)
+{
+	return _of_regulator_get(dev, node, id, NORMAL_GET);
+}
+EXPORT_SYMBOL_GPL(of_regulator_get);
+
 /**
  * of_regulator_get_optional - get optional regulator via device tree lookup
  * @dev: device used for dev_printk() messages
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 8c3c372ad735..5903ae7444ae 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -169,6 +169,12 @@ void regulator_put(struct regulator *regulator);
 void devm_regulator_put(struct regulator *regulator);
 
 #if IS_ENABLED(CONFIG_OF)
+struct regulator *__must_check of_regulator_get(struct device *dev,
+						struct device_node *node,
+						const char *id);
+struct regulator *__must_check devm_of_regulator_get(struct device *dev,
+						     struct device_node *node,
+						     const char *id);
 struct regulator *__must_check of_regulator_get_optional(struct device *dev,
 							 struct device_node *node,
 							 const char *id);
-- 
2.45.2

[PATCH v5 2/7] pmdomain: rockchip: cleanup mutex handling in rockchip_pd_power

[Sebastian Reichel]

Use the cleanup infrastructure to handle the mutex, which
slightly improve code readability for this function.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index cb0f93800138..a161ee13c633 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -574,13 +574,12 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 	struct rockchip_pmu *pmu = pd->pmu;
 	int ret;
 
-	mutex_lock(&pmu->mutex);
+	guard(mutex)(&pmu->mutex);
 
 	if (rockchip_pmu_domain_is_on(pd) != power_on) {
 		ret = clk_bulk_enable(pd->num_clks, pd->clks);
 		if (ret < 0) {
 			dev_err(pmu->dev, "failed to enable clocks\n");
-			mutex_unlock(&pmu->mutex);
 			return ret;
 		}
 
@@ -606,7 +605,6 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 		clk_bulk_disable(pd->num_clks, pd->clks);
 	}
 
-	mutex_unlock(&pmu->mutex);
 	return 0;
 }
 
-- 
2.45.2

[PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Sebastian Reichel]

Currently rockchip_do_pmu_set_power_domain prints a warning if there
have been errors turning on the power domain, but it does not return
any errors and rockchip_pd_power() tries to continue setting up the
QOS registers. This usually results in accessing unpowered registers,
which triggers an SError and a full system hang.

This improves the error handling by forwarding the error to avoid
kernel panics.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index a161ee13c633..8f440f2883db 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
 	return ret;
 }
 
-static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
-					     bool on)
+static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
+					    bool on)
 {
 	struct rockchip_pmu *pmu = pd->pmu;
 	struct generic_pm_domain *genpd = &pd->genpd;
 	u32 pd_pwr_offset = pd->info->pwr_offset;
 	bool is_on, is_mem_on = false;
+	int ret;
 
 	if (pd->info->pwr_mask == 0)
-		return;
+		return 0;
 
 	if (on && pd->info->mem_status_mask)
 		is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
@@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
 
 	wmb();
 
-	if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
-		return;
+	if (is_mem_on) {
+		ret = rockchip_pmu_domain_mem_reset(pd);
+		if (ret)
+			return ret;
+	}
 
-	if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
-				      is_on == on, 0, 10000)) {
-		dev_err(pmu->dev,
-			"failed to set domain '%s', val=%d\n",
-			genpd->name, is_on);
-		return;
+	ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
+					is_on == on, 0, 10000);
+	if (ret) {
+		dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
+			genpd->name, on ? "on" : "off", is_on);
+		return ret;
 	}
+
+	return 0;
 }
 
 static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
@@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 			rockchip_pmu_set_idle_request(pd, true);
 		}
 
-		rockchip_do_pmu_set_power_domain(pd, power_on);
+		ret = rockchip_do_pmu_set_power_domain(pd, power_on);
+		if (ret < 0) {
+			clk_bulk_disable(pd->num_clks, pd->clks);
+			return ret;
+		}
 
 		if (power_on) {
 			/* if powering up, leave idle mode */
-- 
2.45.2

Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Peter Geis]

On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Currently rockchip_do_pmu_set_power_domain prints a warning if there
> have been errors turning on the power domain, but it does not return
> any errors and rockchip_pd_power() tries to continue setting up the
> QOS registers. This usually results in accessing unpowered registers,
> which triggers an SError and a full system hang.
>
> This improves the error handling by forwarding the error to avoid
> kernel panics.

Good Afternoon,

I think we should merge your patch here with my patch for returning
errors from rockchip_pmu_set_idle_request [1].

>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index a161ee13c633..8f440f2883db 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
>         return ret;
>  }
>
> -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> -                                            bool on)
> +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> +                                           bool on)
>  {
>         struct rockchip_pmu *pmu = pd->pmu;
>         struct generic_pm_domain *genpd = &pd->genpd;
>         u32 pd_pwr_offset = pd->info->pwr_offset;
>         bool is_on, is_mem_on = false;
> +       int ret;
>
>         if (pd->info->pwr_mask == 0)
> -               return;
> +               return 0;
>
>         if (on && pd->info->mem_status_mask)
>                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>
>         wmb();
>
> -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> -               return;
> +       if (is_mem_on) {
> +               ret = rockchip_pmu_domain_mem_reset(pd);
> +               if (ret)
> +                       return ret;
> +       }
>
> -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> -                                     is_on == on, 0, 10000)) {
> -               dev_err(pmu->dev,
> -                       "failed to set domain '%s', val=%d\n",
> -                       genpd->name, is_on);
> -               return;
> +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> +                                       is_on == on, 0, 10000);
> +       if (ret) {
> +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> +                       genpd->name, on ? "on" : "off", is_on);
> +               return ret;
>         }
> +
> +       return 0;
>  }
>
>  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>                         rockchip_pmu_set_idle_request(pd, true);
>                 }
>
> -               rockchip_do_pmu_set_power_domain(pd, power_on);
> +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> +               if (ret < 0) {
> +                       clk_bulk_disable(pd->num_clks, pd->clks);
> +                       return ret;

Looking at it, we shouldn't return directly from here because the
mutex never gets unlocked.
Instead of repeating clk_bulk_disable and return ret for each failure,
we can initialize ret = 0, have a goto: out pointing to
clk_bulk_disable, and change return 0 to return ret at the end.

What do you think?

Very Respectfully,
Peter Geis

[1] https://lore.kernel.org/linux-rockchip/20241210013010.81257-2-pgwipeout@gmail.com/

> +               }
>
>                 if (power_on) {
>                         /* if powering up, leave idle mode */
> --
> 2.45.2
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5184 bytes --]

Hello Peter,

On Wed, Dec 11, 2024 at 02:53:34PM -0500, Peter Geis wrote:
> On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > Currently rockchip_do_pmu_set_power_domain prints a warning if there
> > have been errors turning on the power domain, but it does not return
> > any errors and rockchip_pd_power() tries to continue setting up the
> > QOS registers. This usually results in accessing unpowered registers,
> > which triggers an SError and a full system hang.
> >
> > This improves the error handling by forwarding the error to avoid
> > kernel panics.
> 
> I think we should merge your patch here with my patch for returning
> errors from rockchip_pmu_set_idle_request [1].

I will have a look.

> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> > index a161ee13c633..8f440f2883db 100644
> > --- a/drivers/pmdomain/rockchip/pm-domains.c
> > +++ b/drivers/pmdomain/rockchip/pm-domains.c
> > @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
> >         return ret;
> >  }
> >
> > -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > -                                            bool on)
> > +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > +                                           bool on)
> >  {
> >         struct rockchip_pmu *pmu = pd->pmu;
> >         struct generic_pm_domain *genpd = &pd->genpd;
> >         u32 pd_pwr_offset = pd->info->pwr_offset;
> >         bool is_on, is_mem_on = false;
> > +       int ret;
> >
> >         if (pd->info->pwr_mask == 0)
> > -               return;
> > +               return 0;
> >
> >         if (on && pd->info->mem_status_mask)
> >                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> > @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> >
> >         wmb();
> >
> > -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> > -               return;
> > +       if (is_mem_on) {
> > +               ret = rockchip_pmu_domain_mem_reset(pd);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >
> > -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > -                                     is_on == on, 0, 10000)) {
> > -               dev_err(pmu->dev,
> > -                       "failed to set domain '%s', val=%d\n",
> > -                       genpd->name, is_on);
> > -               return;
> > +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > +                                       is_on == on, 0, 10000);
> > +       if (ret) {
> > +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> > +                       genpd->name, on ? "on" : "off", is_on);
> > +               return ret;
> >         }
> > +
> > +       return 0;
> >  }
> >
> >  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> >                         rockchip_pmu_set_idle_request(pd, true);
> >                 }
> >
> > -               rockchip_do_pmu_set_power_domain(pd, power_on);
> > +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> > +               if (ret < 0) {
> > +                       clk_bulk_disable(pd->num_clks, pd->clks);
> > +                       return ret;
> 
> Looking at it, we shouldn't return directly from here because the
> mutex never gets unlocked.

Yes, we should do that after patch 2/7 from this series :)

> Instead of repeating clk_bulk_disable and return ret for each failure,
> we can initialize ret = 0, have a goto: out pointing to
> clk_bulk_disable, and change return 0 to return ret at the end.

Right now there is only a single clk_bulk_disable() in an error
case, so I did not use the typical error goto chain. I suppose
it makes a lot more sense with proper error handling for the calls
to rockchip_pmu_set_idle_request().

Greetings,

-- Sebastian

> 
> What do you think?
> 
> Very Respectfully,
> Peter Geis
> 
> [1] https://lore.kernel.org/linux-rockchip/20241210013010.81257-2-pgwipeout@gmail.com/
> 
> > +               }
> >
> >                 if (power_on) {
> >                         /* if powering up, leave idle mode */
> > --
> > 2.45.2
> >
> >
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Peter Geis]

On Wed, Dec 11, 2024 at 3:46 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hello Peter,
>
> On Wed, Dec 11, 2024 at 02:53:34PM -0500, Peter Geis wrote:
> > On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > >
> > > Currently rockchip_do_pmu_set_power_domain prints a warning if there
> > > have been errors turning on the power domain, but it does not return
> > > any errors and rockchip_pd_power() tries to continue setting up the
> > > QOS registers. This usually results in accessing unpowered registers,
> > > which triggers an SError and a full system hang.
> > >
> > > This improves the error handling by forwarding the error to avoid
> > > kernel panics.
> >
> > I think we should merge your patch here with my patch for returning
> > errors from rockchip_pmu_set_idle_request [1].
>
> I will have a look.
>
> > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
> > >  1 file changed, 22 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> > > index a161ee13c633..8f440f2883db 100644
> > > --- a/drivers/pmdomain/rockchip/pm-domains.c
> > > +++ b/drivers/pmdomain/rockchip/pm-domains.c
> > > @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
> > >         return ret;
> > >  }
> > >
> > > -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > -                                            bool on)
> > > +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > +                                           bool on)
> > >  {
> > >         struct rockchip_pmu *pmu = pd->pmu;
> > >         struct generic_pm_domain *genpd = &pd->genpd;
> > >         u32 pd_pwr_offset = pd->info->pwr_offset;
> > >         bool is_on, is_mem_on = false;
> > > +       int ret;
> > >
> > >         if (pd->info->pwr_mask == 0)
> > > -               return;
> > > +               return 0;
> > >
> > >         if (on && pd->info->mem_status_mask)
> > >                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> > > @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > >
> > >         wmb();
> > >
> > > -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> > > -               return;
> > > +       if (is_mem_on) {
> > > +               ret = rockchip_pmu_domain_mem_reset(pd);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > >
> > > -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > -                                     is_on == on, 0, 10000)) {
> > > -               dev_err(pmu->dev,
> > > -                       "failed to set domain '%s', val=%d\n",
> > > -                       genpd->name, is_on);
> > > -               return;
> > > +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > +                                       is_on == on, 0, 10000);
> > > +       if (ret) {
> > > +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> > > +                       genpd->name, on ? "on" : "off", is_on);
> > > +               return ret;
> > >         }
> > > +
> > > +       return 0;
> > >  }
> > >
> > >  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > >                         rockchip_pmu_set_idle_request(pd, true);
> > >                 }
> > >
> > > -               rockchip_do_pmu_set_power_domain(pd, power_on);
> > > +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> > > +               if (ret < 0) {
> > > +                       clk_bulk_disable(pd->num_clks, pd->clks);
> > > +                       return ret;
> >
> > Looking at it, we shouldn't return directly from here because the
> > mutex never gets unlocked.
>
> Yes, we should do that after patch 2/7 from this series :)

That's excellent!

>
> > Instead of repeating clk_bulk_disable and return ret for each failure,
> > we can initialize ret = 0, have a goto: out pointing to
> > clk_bulk_disable, and change return 0 to return ret at the end.
>
> Right now there is only a single clk_bulk_disable() in an error
> case, so I did not use the typical error goto chain. I suppose
> it makes a lot more sense with proper error handling for the calls
> to rockchip_pmu_set_idle_request().

If you'd like, I can base my v2 on this patch series with the changes
I'm suggesting?

>
> Greetings,
>
> -- Sebastian
>
> >
> > What do you think?
> >
> > Very Respectfully,
> > Peter Geis
> >
> > [1] https://lore.kernel.org/linux-rockchip/20241210013010.81257-2-pgwipeout@gmail.com/
> >
> > > +               }
> > >
> > >                 if (power_on) {
> > >                         /* if powering up, leave idle mode */
> > > --
> > > 2.45.2
> > >
> > >
> > > _______________________________________________
> > > Linux-rockchip mailing list
> > > Linux-rockchip@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Ulf Hansson]

On Thu, 12 Dec 2024 at 00:11, Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Wed, Dec 11, 2024 at 3:46 PM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > Hello Peter,
> >
> > On Wed, Dec 11, 2024 at 02:53:34PM -0500, Peter Geis wrote:
> > > On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
> > > <sebastian.reichel@collabora.com> wrote:
> > > >
> > > > Currently rockchip_do_pmu_set_power_domain prints a warning if there
> > > > have been errors turning on the power domain, but it does not return
> > > > any errors and rockchip_pd_power() tries to continue setting up the
> > > > QOS registers. This usually results in accessing unpowered registers,
> > > > which triggers an SError and a full system hang.
> > > >
> > > > This improves the error handling by forwarding the error to avoid
> > > > kernel panics.
> > >
> > > I think we should merge your patch here with my patch for returning
> > > errors from rockchip_pmu_set_idle_request [1].
> >
> > I will have a look.
> >
> > > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > ---
> > > >  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
> > > >  1 file changed, 22 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> > > > index a161ee13c633..8f440f2883db 100644
> > > > --- a/drivers/pmdomain/rockchip/pm-domains.c
> > > > +++ b/drivers/pmdomain/rockchip/pm-domains.c
> > > > @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
> > > >         return ret;
> > > >  }
> > > >
> > > > -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > -                                            bool on)
> > > > +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > +                                           bool on)
> > > >  {
> > > >         struct rockchip_pmu *pmu = pd->pmu;
> > > >         struct generic_pm_domain *genpd = &pd->genpd;
> > > >         u32 pd_pwr_offset = pd->info->pwr_offset;
> > > >         bool is_on, is_mem_on = false;
> > > > +       int ret;
> > > >
> > > >         if (pd->info->pwr_mask == 0)
> > > > -               return;
> > > > +               return 0;
> > > >
> > > >         if (on && pd->info->mem_status_mask)
> > > >                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> > > > @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > >
> > > >         wmb();
> > > >
> > > > -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> > > > -               return;
> > > > +       if (is_mem_on) {
> > > > +               ret = rockchip_pmu_domain_mem_reset(pd);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > >
> > > > -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > > -                                     is_on == on, 0, 10000)) {
> > > > -               dev_err(pmu->dev,
> > > > -                       "failed to set domain '%s', val=%d\n",
> > > > -                       genpd->name, is_on);
> > > > -               return;
> > > > +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > > +                                       is_on == on, 0, 10000);
> > > > +       if (ret) {
> > > > +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> > > > +                       genpd->name, on ? "on" : "off", is_on);
> > > > +               return ret;
> > > >         }
> > > > +
> > > > +       return 0;
> > > >  }
> > > >
> > > >  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > > @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > >                         rockchip_pmu_set_idle_request(pd, true);
> > > >                 }
> > > >
> > > > -               rockchip_do_pmu_set_power_domain(pd, power_on);
> > > > +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> > > > +               if (ret < 0) {
> > > > +                       clk_bulk_disable(pd->num_clks, pd->clks);
> > > > +                       return ret;
> > >
> > > Looking at it, we shouldn't return directly from here because the
> > > mutex never gets unlocked.
> >
> > Yes, we should do that after patch 2/7 from this series :)
>
> That's excellent!
>
> >
> > > Instead of repeating clk_bulk_disable and return ret for each failure,
> > > we can initialize ret = 0, have a goto: out pointing to
> > > clk_bulk_disable, and change return 0 to return ret at the end.
> >
> > Right now there is only a single clk_bulk_disable() in an error
> > case, so I did not use the typical error goto chain. I suppose
> > it makes a lot more sense with proper error handling for the calls
> > to rockchip_pmu_set_idle_request().
>
> If you'd like, I can base my v2 on this patch series with the changes
> I'm suggesting?

I leave you guys to decide the best way forward, but please keep in
mind that fixes/stable patches are easier managed if they are as
simple as possible and without relying on cleanup patches. Better fix
the problem first, then clean up the code.

Kind regards
Uffe

Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 6099 bytes --]

Hi,

On Thu, Dec 12, 2024 at 12:26:42PM +0100, Ulf Hansson wrote:
> On Thu, 12 Dec 2024 at 00:11, Peter Geis <pgwipeout@gmail.com> wrote:
> > On Wed, Dec 11, 2024 at 3:46 PM Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > > On Wed, Dec 11, 2024 at 02:53:34PM -0500, Peter Geis wrote:
> > > > On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
> > > > <sebastian.reichel@collabora.com> wrote:
> > > > >
> > > > > Currently rockchip_do_pmu_set_power_domain prints a warning if there
> > > > > have been errors turning on the power domain, but it does not return
> > > > > any errors and rockchip_pd_power() tries to continue setting up the
> > > > > QOS registers. This usually results in accessing unpowered registers,
> > > > > which triggers an SError and a full system hang.
> > > > >
> > > > > This improves the error handling by forwarding the error to avoid
> > > > > kernel panics.
> > > >
> > > > I think we should merge your patch here with my patch for returning
> > > > errors from rockchip_pmu_set_idle_request [1].
> > >
> > > I will have a look.
> > >
> > > > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > > Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
> > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > ---
> > > > >  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
> > > > >  1 file changed, 22 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> > > > > index a161ee13c633..8f440f2883db 100644
> > > > > --- a/drivers/pmdomain/rockchip/pm-domains.c
> > > > > +++ b/drivers/pmdomain/rockchip/pm-domains.c
> > > > > @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
> > > > >         return ret;
> > > > >  }
> > > > >
> > > > > -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > > -                                            bool on)
> > > > > +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > > +                                           bool on)
> > > > >  {
> > > > >         struct rockchip_pmu *pmu = pd->pmu;
> > > > >         struct generic_pm_domain *genpd = &pd->genpd;
> > > > >         u32 pd_pwr_offset = pd->info->pwr_offset;
> > > > >         bool is_on, is_mem_on = false;
> > > > > +       int ret;
> > > > >
> > > > >         if (pd->info->pwr_mask == 0)
> > > > > -               return;
> > > > > +               return 0;
> > > > >
> > > > >         if (on && pd->info->mem_status_mask)
> > > > >                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> > > > > @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > >
> > > > >         wmb();
> > > > >
> > > > > -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> > > > > -               return;
> > > > > +       if (is_mem_on) {
> > > > > +               ret = rockchip_pmu_domain_mem_reset(pd);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +       }
> > > > >
> > > > > -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > > > -                                     is_on == on, 0, 10000)) {
> > > > > -               dev_err(pmu->dev,
> > > > > -                       "failed to set domain '%s', val=%d\n",
> > > > > -                       genpd->name, is_on);
> > > > > -               return;
> > > > > +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > > > +                                       is_on == on, 0, 10000);
> > > > > +       if (ret) {
> > > > > +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> > > > > +                       genpd->name, on ? "on" : "off", is_on);
> > > > > +               return ret;
> > > > >         }
> > > > > +
> > > > > +       return 0;
> > > > >  }
> > > > >
> > > > >  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > > > @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > > >                         rockchip_pmu_set_idle_request(pd, true);
> > > > >                 }
> > > > >
> > > > > -               rockchip_do_pmu_set_power_domain(pd, power_on);
> > > > > +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> > > > > +               if (ret < 0) {
> > > > > +                       clk_bulk_disable(pd->num_clks, pd->clks);
> > > > > +                       return ret;
> > > >
> > > > Looking at it, we shouldn't return directly from here because the
> > > > mutex never gets unlocked.
> > >
> > > Yes, we should do that after patch 2/7 from this series :)
> >
> > That's excellent!
> >
> > >
> > > > Instead of repeating clk_bulk_disable and return ret for each failure,
> > > > we can initialize ret = 0, have a goto: out pointing to
> > > > clk_bulk_disable, and change return 0 to return ret at the end.
> > >
> > > Right now there is only a single clk_bulk_disable() in an error
> > > case, so I did not use the typical error goto chain. I suppose
> > > it makes a lot more sense with proper error handling for the calls
> > > to rockchip_pmu_set_idle_request().
> >
> > If you'd like, I can base my v2 on this patch series with the changes
> > I'm suggesting?
> 
> I leave you guys to decide the best way forward, but please keep in
> mind that fixes/stable patches are easier managed if they are as
> simple as possible and without relying on cleanup patches. Better fix
> the problem first, then clean up the code.

I had this ordered the other way around initially and as Heiko
pointed out that makes things more complicated overall:

https://lore.kernel.org/linux-rockchip/4864529.A9s0UXYOmP@diego/

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/7] pmdomain: rockchip: forward rockchip_do_pmu_set_power_domain errors

[Ulf Hansson]

On Thu, 12 Dec 2024 at 20:14, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Thu, Dec 12, 2024 at 12:26:42PM +0100, Ulf Hansson wrote:
> > On Thu, 12 Dec 2024 at 00:11, Peter Geis <pgwipeout@gmail.com> wrote:
> > > On Wed, Dec 11, 2024 at 3:46 PM Sebastian Reichel
> > > <sebastian.reichel@collabora.com> wrote:
> > > > On Wed, Dec 11, 2024 at 02:53:34PM -0500, Peter Geis wrote:
> > > > > On Wed, Dec 11, 2024 at 9:32 AM Sebastian Reichel
> > > > > <sebastian.reichel@collabora.com> wrote:
> > > > > >
> > > > > > Currently rockchip_do_pmu_set_power_domain prints a warning if there
> > > > > > have been errors turning on the power domain, but it does not return
> > > > > > any errors and rockchip_pd_power() tries to continue setting up the
> > > > > > QOS registers. This usually results in accessing unpowered registers,
> > > > > > which triggers an SError and a full system hang.
> > > > > >
> > > > > > This improves the error handling by forwarding the error to avoid
> > > > > > kernel panics.
> > > > >
> > > > > I think we should merge your patch here with my patch for returning
> > > > > errors from rockchip_pmu_set_idle_request [1].
> > > >
> > > > I will have a look.
> > > >
> > > > > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > > > > Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
> > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > ---
> > > > > >  drivers/pmdomain/rockchip/pm-domains.c | 34 +++++++++++++++++---------
> > > > > >  1 file changed, 22 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> > > > > > index a161ee13c633..8f440f2883db 100644
> > > > > > --- a/drivers/pmdomain/rockchip/pm-domains.c
> > > > > > +++ b/drivers/pmdomain/rockchip/pm-domains.c
> > > > > > @@ -533,16 +533,17 @@ static int rockchip_pmu_domain_mem_reset(struct rockchip_pm_domain *pd)
> > > > > >         return ret;
> > > > > >  }
> > > > > >
> > > > > > -static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > > > -                                            bool on)
> > > > > > +static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > > > +                                           bool on)
> > > > > >  {
> > > > > >         struct rockchip_pmu *pmu = pd->pmu;
> > > > > >         struct generic_pm_domain *genpd = &pd->genpd;
> > > > > >         u32 pd_pwr_offset = pd->info->pwr_offset;
> > > > > >         bool is_on, is_mem_on = false;
> > > > > > +       int ret;
> > > > > >
> > > > > >         if (pd->info->pwr_mask == 0)
> > > > > > -               return;
> > > > > > +               return 0;
> > > > > >
> > > > > >         if (on && pd->info->mem_status_mask)
> > > > > >                 is_mem_on = rockchip_pmu_domain_is_mem_on(pd);
> > > > > > @@ -557,16 +558,21 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> > > > > >
> > > > > >         wmb();
> > > > > >
> > > > > > -       if (is_mem_on && rockchip_pmu_domain_mem_reset(pd))
> > > > > > -               return;
> > > > > > +       if (is_mem_on) {
> > > > > > +               ret = rockchip_pmu_domain_mem_reset(pd);
> > > > > > +               if (ret)
> > > > > > +                       return ret;
> > > > > > +       }
> > > > > >
> > > > > > -       if (readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > > > > -                                     is_on == on, 0, 10000)) {
> > > > > > -               dev_err(pmu->dev,
> > > > > > -                       "failed to set domain '%s', val=%d\n",
> > > > > > -                       genpd->name, is_on);
> > > > > > -               return;
> > > > > > +       ret = readx_poll_timeout_atomic(rockchip_pmu_domain_is_on, pd, is_on,
> > > > > > +                                       is_on == on, 0, 10000);
> > > > > > +       if (ret) {
> > > > > > +               dev_err(pmu->dev, "failed to set domain '%s' %s, val=%d\n",
> > > > > > +                       genpd->name, on ? "on" : "off", is_on);
> > > > > > +               return ret;
> > > > > >         }
> > > > > > +
> > > > > > +       return 0;
> > > > > >  }
> > > > > >
> > > > > >  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > > > > @@ -592,7 +598,11 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> > > > > >                         rockchip_pmu_set_idle_request(pd, true);
> > > > > >                 }
> > > > > >
> > > > > > -               rockchip_do_pmu_set_power_domain(pd, power_on);
> > > > > > +               ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> > > > > > +               if (ret < 0) {
> > > > > > +                       clk_bulk_disable(pd->num_clks, pd->clks);
> > > > > > +                       return ret;
> > > > >
> > > > > Looking at it, we shouldn't return directly from here because the
> > > > > mutex never gets unlocked.
> > > >
> > > > Yes, we should do that after patch 2/7 from this series :)
> > >
> > > That's excellent!
> > >
> > > >
> > > > > Instead of repeating clk_bulk_disable and return ret for each failure,
> > > > > we can initialize ret = 0, have a goto: out pointing to
> > > > > clk_bulk_disable, and change return 0 to return ret at the end.
> > > >
> > > > Right now there is only a single clk_bulk_disable() in an error
> > > > case, so I did not use the typical error goto chain. I suppose
> > > > it makes a lot more sense with proper error handling for the calls
> > > > to rockchip_pmu_set_idle_request().
> > >
> > > If you'd like, I can base my v2 on this patch series with the changes
> > > I'm suggesting?
> >
> > I leave you guys to decide the best way forward, but please keep in
> > mind that fixes/stable patches are easier managed if they are as
> > simple as possible and without relying on cleanup patches. Better fix
> > the problem first, then clean up the code.
>
> I had this ordered the other way around initially and as Heiko
> pointed out that makes things more complicated overall:
>
> https://lore.kernel.org/linux-rockchip/4864529.A9s0UXYOmP@diego/

Right, I have no strong opinions, but leave the decision to you.

Still, we need confirmation on the regulator-patch (patch1) from Mark,
before I can apply any of this.

Kind regards
Uffe

[PATCH v5 4/7] pmdomain: rockchip: reduce indentation in rockchip_pd_power

[Sebastian Reichel]

Rework the logic, so that the function exits early when the
power domain state is already correct to reduce code indentation.

No functional change intended.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 49 +++++++++++++-------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index 8f440f2883db..f4e555dac20a 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -582,39 +582,40 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 
 	guard(mutex)(&pmu->mutex);
 
-	if (rockchip_pmu_domain_is_on(pd) != power_on) {
-		ret = clk_bulk_enable(pd->num_clks, pd->clks);
-		if (ret < 0) {
-			dev_err(pmu->dev, "failed to enable clocks\n");
-			return ret;
-		}
+	if (rockchip_pmu_domain_is_on(pd) == power_on)
+		return 0;
 
-		rockchip_pmu_ungate_clk(pd, true);
+	ret = clk_bulk_enable(pd->num_clks, pd->clks);
+	if (ret < 0) {
+		dev_err(pmu->dev, "failed to enable clocks\n");
+		return ret;
+	}
 
-		if (!power_on) {
-			rockchip_pmu_save_qos(pd);
+	rockchip_pmu_ungate_clk(pd, true);
 
-			/* if powering down, idle request to NIU first */
-			rockchip_pmu_set_idle_request(pd, true);
-		}
+	if (!power_on) {
+		rockchip_pmu_save_qos(pd);
 
-		ret = rockchip_do_pmu_set_power_domain(pd, power_on);
-		if (ret < 0) {
-			clk_bulk_disable(pd->num_clks, pd->clks);
-			return ret;
-		}
+		/* if powering down, idle request to NIU first */
+		rockchip_pmu_set_idle_request(pd, true);
+	}
 
-		if (power_on) {
-			/* if powering up, leave idle mode */
-			rockchip_pmu_set_idle_request(pd, false);
+	ret = rockchip_do_pmu_set_power_domain(pd, power_on);
+	if (ret < 0) {
+		clk_bulk_disable(pd->num_clks, pd->clks);
+		return ret;
+	}
 
-			rockchip_pmu_restore_qos(pd);
-		}
+	if (power_on) {
+		/* if powering up, leave idle mode */
+		rockchip_pmu_set_idle_request(pd, false);
 
-		rockchip_pmu_ungate_clk(pd, false);
-		clk_bulk_disable(pd->num_clks, pd->clks);
+		rockchip_pmu_restore_qos(pd);
 	}
 
+	rockchip_pmu_ungate_clk(pd, false);
+	clk_bulk_disable(pd->num_clks, pd->clks);
+
 	return 0;
 }
 
-- 
2.45.2

[PATCH v5 5/7] dt-bindings: power: rockchip: add regulator support

[Sebastian Reichel]

Add optional support for a voltage supply required to enable a
power domain. The binding follows the way it is handled by the
Mediatek binding to keep things consistent.

This will initially be used by the RK3588 GPU power domain, which
fails to be enabled when the GPU regulator is not enabled. It is
not limited to that platform, since older generations have similar
requirements. They worked around this by marking the regulators
as always-on instead of describing the dependency.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/power/rockchip,power-controller.yaml   | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml b/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
index 650dc0aae6f5..ebab98987e49 100644
--- a/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
@@ -132,6 +132,9 @@ $defs:
           A number of phandles to clocks that need to be enabled
           while power domain switches state.
 
+      domain-supply:
+        description: domain regulator supply.
+
       pm_qos:
         $ref: /schemas/types.yaml#/definitions/phandle-array
         items:
-- 
2.45.2

[PATCH v5 6/7] pmdomain: rockchip: add regulator support

[Sebastian Reichel]

Some power domains require extra voltages to be applied. For example
trying to enable the GPU power domain on RK3588 fails when the SoC
does not have VDD GPU enabled. The same is expected to happen for
the NPU, which also has a dedicated supply line.

We get the regulator using devm_of_regulator_get(), so a missing
dependency in the devicetree is handled gracefully by printing a warning
and creating a dummy regulator. This is necessary, since existing DTs do
not have the regulator described. They might still work if the regulator
is marked as always-on. It is also working if the regulator is enabled
at boot time and the GPU driver is probed before the kernel disables
unused regulators.

The regulator itself is not acquired at driver probe time, since that
creates an unsolvable circular dependency. The power domain driver must
be probed early, since SoC peripherals need it. Regulators on the other
hand depend on SoC peripherals like SPI, I2C or GPIO. MediaTek does not
run into this, since they have two power domain drivers.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 113 +++++++++++++++++--------
 1 file changed, 79 insertions(+), 34 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index f4e555dac20a..31c71b6fddf1 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -18,6 +18,7 @@
 #include <linux/of_clk.h>
 #include <linux/clk.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/mfd/syscon.h>
 #include <soc/rockchip/pm_domains.h>
 #include <dt-bindings/power/px30-power.h>
@@ -44,6 +45,7 @@ struct rockchip_domain_info {
 	int idle_mask;
 	int ack_mask;
 	bool active_wakeup;
+	bool need_regulator;
 	int pwr_w_mask;
 	int req_w_mask;
 	int clk_ungate_mask;
@@ -92,6 +94,8 @@ struct rockchip_pm_domain {
 	u32 *qos_save_regs[MAX_QOS_REGS_NUM];
 	int num_clks;
 	struct clk_bulk_data *clks;
+	struct device_node *node;
+	struct regulator *supply;
 };
 
 struct rockchip_pmu {
@@ -129,7 +133,7 @@ struct rockchip_pmu {
 	.active_wakeup = wakeup,			\
 }
 
-#define DOMAIN_M_O_R(_name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, ack, wakeup)	\
+#define DOMAIN_M_O_R(_name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, ack, wakeup, regulator)	\
 {							\
 	.name = _name,					\
 	.pwr_offset = p_offset,				\
@@ -145,6 +149,7 @@ struct rockchip_pmu {
 	.idle_mask = (idle),				\
 	.ack_mask = (ack),				\
 	.active_wakeup = wakeup,			\
+	.need_regulator = regulator,			\
 }
 
 #define DOMAIN_M_O_R_G(_name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, ack, g_mask, wakeup)	\
@@ -303,8 +308,8 @@ void rockchip_pmu_unblock(void)
 }
 EXPORT_SYMBOL_GPL(rockchip_pmu_unblock);
 
-#define DOMAIN_RK3588(name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, wakeup)	\
-	DOMAIN_M_O_R(name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, idle, wakeup)
+#define DOMAIN_RK3588(name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, wakeup, regulator)	\
+	DOMAIN_M_O_R(name, p_offset, pwr, status, m_offset, m_status, r_status, r_offset, req, idle, idle, wakeup, regulator)
 
 static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
 {
@@ -619,18 +624,57 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 	return 0;
 }
 
+static int rockchip_pd_regulator_disable(struct rockchip_pm_domain *pd)
+{
+	return IS_ERR_OR_NULL(pd->supply) ? 0 : regulator_disable(pd->supply);
+}
+
+static int rockchip_pd_regulator_enable(struct rockchip_pm_domain *pd)
+{
+	struct rockchip_pmu *pmu = pd->pmu;
+
+	if (!pd->info->need_regulator)
+		return 0;
+
+	if (IS_ERR_OR_NULL(pd->supply)) {
+		pd->supply = devm_of_regulator_get(pmu->dev, pd->node, "domain");
+
+		if (IS_ERR(pd->supply))
+			return PTR_ERR(pd->supply);
+	}
+
+	return regulator_enable(pd->supply);
+}
+
 static int rockchip_pd_power_on(struct generic_pm_domain *domain)
 {
 	struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
+	int ret;
+
+	ret = rockchip_pd_regulator_enable(pd);
+	if (ret) {
+		dev_err(pd->pmu->dev, "Failed to enable supply: %d\n", ret);
+		return ret;
+	}
 
-	return rockchip_pd_power(pd, true);
+	ret = rockchip_pd_power(pd, true);
+	if (ret)
+		rockchip_pd_regulator_disable(pd);
+
+	return ret;
 }
 
 static int rockchip_pd_power_off(struct generic_pm_domain *domain)
 {
 	struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
+	int ret;
 
-	return rockchip_pd_power(pd, false);
+	ret = rockchip_pd_power(pd, false);
+	if (ret)
+		return ret;
+
+	rockchip_pd_regulator_disable(pd);
+	return ret;
 }
 
 static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
@@ -711,6 +755,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 
 	pd->info = pd_info;
 	pd->pmu = pmu;
+	pd->node = node;
 
 	pd->num_clks = of_clk_get_parent_count(node);
 	if (pd->num_clks > 0) {
@@ -1174,35 +1219,35 @@ static const struct rockchip_domain_info rk3576_pm_domains[] = {
 };
 
 static const struct rockchip_domain_info rk3588_pm_domains[] = {
-	[RK3588_PD_GPU]		= DOMAIN_RK3588("gpu",     0x0, BIT(0),  0,       0x0, 0,       BIT(1),  0x0, BIT(0),  BIT(0),  false),
-	[RK3588_PD_NPU]		= DOMAIN_RK3588("npu",     0x0, BIT(1),  BIT(1),  0x0, 0,       0,       0x0, 0,       0,       false),
-	[RK3588_PD_VCODEC]	= DOMAIN_RK3588("vcodec",  0x0, BIT(2),  BIT(2),  0x0, 0,       0,       0x0, 0,       0,       false),
-	[RK3588_PD_NPUTOP]	= DOMAIN_RK3588("nputop",  0x0, BIT(3),  0,       0x0, BIT(11), BIT(2),  0x0, BIT(1),  BIT(1),  false),
-	[RK3588_PD_NPU1]	= DOMAIN_RK3588("npu1",    0x0, BIT(4),  0,       0x0, BIT(12), BIT(3),  0x0, BIT(2),  BIT(2),  false),
-	[RK3588_PD_NPU2]	= DOMAIN_RK3588("npu2",    0x0, BIT(5),  0,       0x0, BIT(13), BIT(4),  0x0, BIT(3),  BIT(3),  false),
-	[RK3588_PD_VENC0]	= DOMAIN_RK3588("venc0",   0x0, BIT(6),  0,       0x0, BIT(14), BIT(5),  0x0, BIT(4),  BIT(4),  false),
-	[RK3588_PD_VENC1]	= DOMAIN_RK3588("venc1",   0x0, BIT(7),  0,       0x0, BIT(15), BIT(6),  0x0, BIT(5),  BIT(5),  false),
-	[RK3588_PD_RKVDEC0]	= DOMAIN_RK3588("rkvdec0", 0x0, BIT(8),  0,       0x0, BIT(16), BIT(7),  0x0, BIT(6),  BIT(6),  false),
-	[RK3588_PD_RKVDEC1]	= DOMAIN_RK3588("rkvdec1", 0x0, BIT(9),  0,       0x0, BIT(17), BIT(8),  0x0, BIT(7),  BIT(7),  false),
-	[RK3588_PD_VDPU]	= DOMAIN_RK3588("vdpu",    0x0, BIT(10), 0,       0x0, BIT(18), BIT(9),  0x0, BIT(8),  BIT(8),  false),
-	[RK3588_PD_RGA30]	= DOMAIN_RK3588("rga30",   0x0, BIT(11), 0,       0x0, BIT(19), BIT(10), 0x0, 0,       0,       false),
-	[RK3588_PD_AV1]		= DOMAIN_RK3588("av1",     0x0, BIT(12), 0,       0x0, BIT(20), BIT(11), 0x0, BIT(9),  BIT(9),  false),
-	[RK3588_PD_VI]		= DOMAIN_RK3588("vi",      0x0, BIT(13), 0,       0x0, BIT(21), BIT(12), 0x0, BIT(10), BIT(10), false),
-	[RK3588_PD_FEC]		= DOMAIN_RK3588("fec",     0x0, BIT(14), 0,       0x0, BIT(22), BIT(13), 0x0, 0,       0,       false),
-	[RK3588_PD_ISP1]	= DOMAIN_RK3588("isp1",    0x0, BIT(15), 0,       0x0, BIT(23), BIT(14), 0x0, BIT(11), BIT(11), false),
-	[RK3588_PD_RGA31]	= DOMAIN_RK3588("rga31",   0x4, BIT(0),  0,       0x0, BIT(24), BIT(15), 0x0, BIT(12), BIT(12), false),
-	[RK3588_PD_VOP]		= DOMAIN_RK3588("vop",     0x4, BIT(1),  0,       0x0, BIT(25), BIT(16), 0x0, BIT(13) | BIT(14), BIT(13) | BIT(14), false),
-	[RK3588_PD_VO0]		= DOMAIN_RK3588("vo0",     0x4, BIT(2),  0,       0x0, BIT(26), BIT(17), 0x0, BIT(15), BIT(15), false),
-	[RK3588_PD_VO1]		= DOMAIN_RK3588("vo1",     0x4, BIT(3),  0,       0x0, BIT(27), BIT(18), 0x4, BIT(0),  BIT(16), false),
-	[RK3588_PD_AUDIO]	= DOMAIN_RK3588("audio",   0x4, BIT(4),  0,       0x0, BIT(28), BIT(19), 0x4, BIT(1),  BIT(17), false),
-	[RK3588_PD_PHP]		= DOMAIN_RK3588("php",     0x4, BIT(5),  0,       0x0, BIT(29), BIT(20), 0x4, BIT(5),  BIT(21), false),
-	[RK3588_PD_GMAC]	= DOMAIN_RK3588("gmac",    0x4, BIT(6),  0,       0x0, BIT(30), BIT(21), 0x0, 0,       0,       false),
-	[RK3588_PD_PCIE]	= DOMAIN_RK3588("pcie",    0x4, BIT(7),  0,       0x0, BIT(31), BIT(22), 0x0, 0,       0,       true),
-	[RK3588_PD_NVM]		= DOMAIN_RK3588("nvm",     0x4, BIT(8),  BIT(24), 0x4, 0,       0,       0x4, BIT(2),  BIT(18), false),
-	[RK3588_PD_NVM0]	= DOMAIN_RK3588("nvm0",    0x4, BIT(9),  0,       0x4, BIT(1),  BIT(23), 0x0, 0,       0,       false),
-	[RK3588_PD_SDIO]	= DOMAIN_RK3588("sdio",    0x4, BIT(10), 0,       0x4, BIT(2),  BIT(24), 0x4, BIT(3),  BIT(19), false),
-	[RK3588_PD_USB]		= DOMAIN_RK3588("usb",     0x4, BIT(11), 0,       0x4, BIT(3),  BIT(25), 0x4, BIT(4),  BIT(20), true),
-	[RK3588_PD_SDMMC]	= DOMAIN_RK3588("sdmmc",   0x4, BIT(13), 0,       0x4, BIT(5),  BIT(26), 0x0, 0,       0,       false),
+	[RK3588_PD_GPU]		= DOMAIN_RK3588("gpu",     0x0, BIT(0),  0,       0x0, 0,       BIT(1),  0x0, BIT(0),  BIT(0),  false, true),
+	[RK3588_PD_NPU]		= DOMAIN_RK3588("npu",     0x0, BIT(1),  BIT(1),  0x0, 0,       0,       0x0, 0,       0,       false, true),
+	[RK3588_PD_VCODEC]	= DOMAIN_RK3588("vcodec",  0x0, BIT(2),  BIT(2),  0x0, 0,       0,       0x0, 0,       0,       false, false),
+	[RK3588_PD_NPUTOP]	= DOMAIN_RK3588("nputop",  0x0, BIT(3),  0,       0x0, BIT(11), BIT(2),  0x0, BIT(1),  BIT(1),  false, false),
+	[RK3588_PD_NPU1]	= DOMAIN_RK3588("npu1",    0x0, BIT(4),  0,       0x0, BIT(12), BIT(3),  0x0, BIT(2),  BIT(2),  false, false),
+	[RK3588_PD_NPU2]	= DOMAIN_RK3588("npu2",    0x0, BIT(5),  0,       0x0, BIT(13), BIT(4),  0x0, BIT(3),  BIT(3),  false, false),
+	[RK3588_PD_VENC0]	= DOMAIN_RK3588("venc0",   0x0, BIT(6),  0,       0x0, BIT(14), BIT(5),  0x0, BIT(4),  BIT(4),  false, false),
+	[RK3588_PD_VENC1]	= DOMAIN_RK3588("venc1",   0x0, BIT(7),  0,       0x0, BIT(15), BIT(6),  0x0, BIT(5),  BIT(5),  false, false),
+	[RK3588_PD_RKVDEC0]	= DOMAIN_RK3588("rkvdec0", 0x0, BIT(8),  0,       0x0, BIT(16), BIT(7),  0x0, BIT(6),  BIT(6),  false, false),
+	[RK3588_PD_RKVDEC1]	= DOMAIN_RK3588("rkvdec1", 0x0, BIT(9),  0,       0x0, BIT(17), BIT(8),  0x0, BIT(7),  BIT(7),  false, false),
+	[RK3588_PD_VDPU]	= DOMAIN_RK3588("vdpu",    0x0, BIT(10), 0,       0x0, BIT(18), BIT(9),  0x0, BIT(8),  BIT(8),  false, false),
+	[RK3588_PD_RGA30]	= DOMAIN_RK3588("rga30",   0x0, BIT(11), 0,       0x0, BIT(19), BIT(10), 0x0, 0,       0,       false, false),
+	[RK3588_PD_AV1]		= DOMAIN_RK3588("av1",     0x0, BIT(12), 0,       0x0, BIT(20), BIT(11), 0x0, BIT(9),  BIT(9),  false, false),
+	[RK3588_PD_VI]		= DOMAIN_RK3588("vi",      0x0, BIT(13), 0,       0x0, BIT(21), BIT(12), 0x0, BIT(10), BIT(10), false, false),
+	[RK3588_PD_FEC]		= DOMAIN_RK3588("fec",     0x0, BIT(14), 0,       0x0, BIT(22), BIT(13), 0x0, 0,       0,       false, false),
+	[RK3588_PD_ISP1]	= DOMAIN_RK3588("isp1",    0x0, BIT(15), 0,       0x0, BIT(23), BIT(14), 0x0, BIT(11), BIT(11), false, false),
+	[RK3588_PD_RGA31]	= DOMAIN_RK3588("rga31",   0x4, BIT(0),  0,       0x0, BIT(24), BIT(15), 0x0, BIT(12), BIT(12), false, false),
+	[RK3588_PD_VOP]		= DOMAIN_RK3588("vop",     0x4, BIT(1),  0,       0x0, BIT(25), BIT(16), 0x0, BIT(13) | BIT(14), BIT(13) | BIT(14), false, false),
+	[RK3588_PD_VO0]		= DOMAIN_RK3588("vo0",     0x4, BIT(2),  0,       0x0, BIT(26), BIT(17), 0x0, BIT(15), BIT(15), false, false),
+	[RK3588_PD_VO1]		= DOMAIN_RK3588("vo1",     0x4, BIT(3),  0,       0x0, BIT(27), BIT(18), 0x4, BIT(0),  BIT(16), false, false),
+	[RK3588_PD_AUDIO]	= DOMAIN_RK3588("audio",   0x4, BIT(4),  0,       0x0, BIT(28), BIT(19), 0x4, BIT(1),  BIT(17), false, false),
+	[RK3588_PD_PHP]		= DOMAIN_RK3588("php",     0x4, BIT(5),  0,       0x0, BIT(29), BIT(20), 0x4, BIT(5),  BIT(21), false, false),
+	[RK3588_PD_GMAC]	= DOMAIN_RK3588("gmac",    0x4, BIT(6),  0,       0x0, BIT(30), BIT(21), 0x0, 0,       0,       false, false),
+	[RK3588_PD_PCIE]	= DOMAIN_RK3588("pcie",    0x4, BIT(7),  0,       0x0, BIT(31), BIT(22), 0x0, 0,       0,       true, false),
+	[RK3588_PD_NVM]		= DOMAIN_RK3588("nvm",     0x4, BIT(8),  BIT(24), 0x4, 0,       0,       0x4, BIT(2),  BIT(18), false, false),
+	[RK3588_PD_NVM0]	= DOMAIN_RK3588("nvm0",    0x4, BIT(9),  0,       0x4, BIT(1),  BIT(23), 0x0, 0,       0,       false, false),
+	[RK3588_PD_SDIO]	= DOMAIN_RK3588("sdio",    0x4, BIT(10), 0,       0x4, BIT(2),  BIT(24), 0x4, BIT(3),  BIT(19), false, false),
+	[RK3588_PD_USB]		= DOMAIN_RK3588("usb",     0x4, BIT(11), 0,       0x4, BIT(3),  BIT(25), 0x4, BIT(4),  BIT(20), true, false),
+	[RK3588_PD_SDMMC]	= DOMAIN_RK3588("sdmmc",   0x4, BIT(13), 0,       0x4, BIT(5),  BIT(26), 0x0, 0,       0,       false, false),
 };
 
 static const struct rockchip_pmu_info px30_pmu = {
-- 
2.45.2

Re: [PATCH v5 6/7] pmdomain: rockchip: add regulator support

[Heiko Stübner]

Am Mittwoch, 11. Dezember 2024, 15:26:51 CET schrieb Sebastian Reichel:
> Some power domains require extra voltages to be applied. For example
> trying to enable the GPU power domain on RK3588 fails when the SoC
> does not have VDD GPU enabled. The same is expected to happen for
> the NPU, which also has a dedicated supply line.
> 
> We get the regulator using devm_of_regulator_get(), so a missing
> dependency in the devicetree is handled gracefully by printing a warning
> and creating a dummy regulator. This is necessary, since existing DTs do
> not have the regulator described. They might still work if the regulator
> is marked as always-on. It is also working if the regulator is enabled
> at boot time and the GPU driver is probed before the kernel disables
> unused regulators.
> 
> The regulator itself is not acquired at driver probe time, since that
> creates an unsolvable circular dependency. The power domain driver must
> be probed early, since SoC peripherals need it. Regulators on the other
> hand depend on SoC peripherals like SPI, I2C or GPIO. MediaTek does not
> run into this, since they have two power domain drivers.
> 
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

[PATCH v5 7/7] arm64: dts: rockchip: Add GPU power domain regulator dependency for RK3588

[Sebastian Reichel]

Enabling the GPU power domain requires that the GPU regulator is
enabled. The regulator is enabled at boot time, but automatically
gets disabled when there are no users.

If the GPU driver is not probed at boot time or rebound while
the system is running the system will try to enable the power
domain before the regulator is enabled resulting in a failure
hanging the whole system. Avoid this by adding an explicit
dependency.

Reported-by: Adrián Martínez Larumbe <adrian.larumbe@collabora.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Adrian Larumbe <adrian.larumbe@collabora.com> # On Rock 5B
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts          | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi                 | 2 +-
 arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5.dtsi           | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts              | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-fet3588-c.dtsi            | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588.dtsi  | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts                | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dtsi            | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts              | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts       | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts           | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts            | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi                | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts           | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi           | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts            | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-evb1-v10.dts             | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts        | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-indiedroid-nova.dts      | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts         | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-odroid-m2.dts            | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts              | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts              | 4 ++++
 25 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts b/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts
index 08f09053a066..676f3e71dbe2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts
@@ -286,6 +286,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		hym8563_int: hym8563-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index 8cfa30837ce7..ef16e1fab4f3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -864,7 +864,7 @@ power-domain@RK3588_PD_NPU2 {
 				};
 			};
 			/* These power domains are grouped by VD_GPU */
-			power-domain@RK3588_PD_GPU {
+			pd_gpu: power-domain@RK3588_PD_GPU {
 				reg = <RK3588_PD_GPU>;
 				clocks = <&cru CLK_GPU>,
 					 <&cru CLK_GPU_COREGROUP>,
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5.dtsi
index 71ed680621b8..cc37f082adea 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5.dtsi
@@ -277,6 +277,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		hym8563_int: hym8563-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi
index 5e72d0eff0e0..8a783dc64c0e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi
@@ -126,6 +126,10 @@ regulator-state-mem {
 	};
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	leds {
 		led_user_en: led_user_en {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
index d6e464cdc536..12c72c0592d0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
@@ -459,6 +459,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	audio {
 		hp_detect: headphone-detect {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-fet3588-c.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-fet3588-c.dtsi
index 390051317389..4331cdc70f97 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-fet3588-c.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-fet3588-c.dtsi
@@ -205,6 +205,10 @@ regulator-state-mem {
 	};
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	leds {
 		led_rgb_b: led-rgb-b {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588.dtsi
index e3a9598b99fc..1af0a30866f6 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588.dtsi
@@ -256,6 +256,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	gpio-leds {
 		led_sys_pin: led-sys-pin {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
index 90f823b2c219..f9fdb5fc6e4a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
@@ -451,6 +451,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	emmc {
 		emmc_reset: emmc-reset {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dtsi
index cb350727d116..a0f57cc1e7fc 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-nanopc-t6.dtsi
@@ -565,6 +565,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	gpio-leds {
 		sys_led_pin: sys-led-pin {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts b/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts
index 1c0851b45eb8..fbe1d5c06d90 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts
@@ -312,6 +312,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	pcie2 {
 		pcie2_0_rst: pcie2-0-rst {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
index 9f5a38b290bf..b83ea0db4fba 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
@@ -455,6 +455,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		hym8563_int: hym8563-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts b/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
index 088cfade6f6f..b46abdc27c71 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-quartzpro64.dts
@@ -347,6 +347,10 @@ rgmii_phy: ethernet-phy@1 {
 	};
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		hym8563_int: hym8563-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
index 6d68f70284e4..eaa2e527a918 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
@@ -564,6 +564,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		rtc_int: rtc-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index d597112f1d5b..12474098acf6 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -376,6 +376,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		hym8563_int: hym8563-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
index 81a6a05ce13b..92682f269eef 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
@@ -336,6 +336,10 @@ &pcie3x4 {
 	reset-gpios = <&gpio3 RK_PB6 GPIO_ACTIVE_HIGH>;
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	emmc {
 		emmc_reset: emmc-reset {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts b/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts
index 3cbee5b97470..5a428e00ab93 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts
@@ -289,6 +289,10 @@ rgmii_phy: ethernet-phy@1 {
 	};
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	rtl8211f {
 		rtl8211f_rst: rtl8211f-rst {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
index 6bc46734cc14..711ac4f2c7cb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
@@ -287,6 +287,10 @@ &pcie3x4 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	fan {
 		fan_int: fan-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts b/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts
index 9c394f733bbf..e2ba35299f83 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts
@@ -361,6 +361,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	hym8563 {
 		hym8563_int: hym8563-int {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588s-evb1-v10.dts
index bc4077575beb..5b3de53630c6 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-evb1-v10.dts
@@ -340,6 +340,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	audio {
 		hp_detect: headphone-detect {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts b/arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts
index 812bba0aef1a..8f70c00a46d3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts
@@ -675,6 +675,10 @@ &pcie2x1l1 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	audio-amplifier {
 		headphone_amplifier_en: headphone-amplifier-en {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-indiedroid-nova.dts b/arch/arm64/boot/dts/rockchip/rk3588s-indiedroid-nova.dts
index 4a3aa80f2226..887ea6ace971 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-indiedroid-nova.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-indiedroid-nova.dts
@@ -455,6 +455,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	bluetooth-pins {
 		bt_reset: bt-reset {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
index ac48e7fd3923..88a5e822ed17 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
@@ -233,6 +233,10 @@ hym8563: rtc@51 {
 	};
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	vdd_sd {
 		vdd_sd_en: vdd-sd-en {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-odroid-m2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-odroid-m2.dts
index 8f034c6d494c..729f187c9f10 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-odroid-m2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-odroid-m2.dts
@@ -433,6 +433,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	lcd {
 		lcd_pwren: lcd-pwren {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
index 70a43432bdc5..19141b576e5f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
@@ -359,6 +359,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	leds {
 		io_led: io-led {
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 9b14d5383cdc..b336b0ee4e57 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -425,6 +425,10 @@ &pcie2x1l2 {
 	status = "okay";
 };
 
+&pd_gpu {
+	domain-supply = <&vdd_gpu_s0>;
+};
+
 &pinctrl {
 	leds {
 		led_pins: led-pins {
-- 
2.45.2