[PATCH] Revert "power-domain: Add refcounting"

[PATCH] Revert "power-domain: Add refcounting"

[Wadim Egorov]

Unfortunately this change breaks boot on K3 platform.
U-Boot will hang after:

  U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
  SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')

This reverts commit 197376fbf300e92afa0a1583815d9c9eb52d613a as
suggested in [1].

[1] https://lists.denx.de/pipermail/u-boot/2025-April/587032.html

Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
---
 drivers/firmware/scmi/sandbox-scmi_devices.c  |  1 -
 drivers/power/domain/power-domain-uclass.c    | 40 ++-----------
 .../power/domain/sandbox-power-domain-test.c  |  1 -
 include/power-domain.h                        | 60 +++----------------
 test/dm/power-domain.c                        |  2 +-
 5 files changed, 13 insertions(+), 91 deletions(-)

diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
index 9f253b0fd40..96c2922b067 100644
--- a/drivers/firmware/scmi/sandbox-scmi_devices.c
+++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
@@ -163,5 +163,4 @@ U_BOOT_DRIVER(sandbox_scmi_devices) = {
 	.priv_auto = sizeof(struct sandbox_scmi_device_priv),
 	.remove = sandbox_scmi_devices_remove,
 	.probe = sandbox_scmi_devices_probe,
-	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
 };
diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
index a6e5f9ed036..938bd8cbc9f 100644
--- a/drivers/power/domain/power-domain-uclass.c
+++ b/drivers/power/domain/power-domain-uclass.c
@@ -12,10 +12,6 @@
 #include <power-domain-uclass.h>
 #include <dm/device-internal.h>
 
-struct power_domain_priv {
-	int on_count;
-};
-
 static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev)
 {
 	return (struct power_domain_ops *)dev->driver->ops;
@@ -111,49 +107,22 @@ int power_domain_free(struct power_domain *power_domain)
 	return ops->rfree ? ops->rfree(power_domain) : 0;
 }
 
-int power_domain_on_lowlevel(struct power_domain *power_domain)
+int power_domain_on(struct power_domain *power_domain)
 {
-	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
 	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
-	int ret;
 
 	debug("%s(power_domain=%p)\n", __func__, power_domain);
 
-	if (priv->on_count++ > 0)
-		return -EALREADY;
-
-	ret = ops->on ? ops->on(power_domain) : 0;
-	if (ret) {
-		priv->on_count--;
-		return ret;
-	}
-
-	return 0;
+	return ops->on ? ops->on(power_domain) : 0;
 }
 
-int power_domain_off_lowlevel(struct power_domain *power_domain)
+int power_domain_off(struct power_domain *power_domain)
 {
-	struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
 	struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
-	int ret;
 
 	debug("%s(power_domain=%p)\n", __func__, power_domain);
 
-	if (priv->on_count <= 0) {
-		debug("Power domain %s already off.\n", power_domain->dev->name);
-		return -EALREADY;
-	}
-
-	if (priv->on_count-- > 1)
-		return -EBUSY;
-
-	ret = ops->off ? ops->off(power_domain) : 0;
-	if (ret) {
-		priv->on_count++;
-		return ret;
-	}
-
-	return 0;
+	return ops->off ? ops->off(power_domain) : 0;
 }
 
 #if CONFIG_IS_ENABLED(OF_REAL)
@@ -211,5 +180,4 @@ int dev_power_domain_off(struct udevice *dev)
 UCLASS_DRIVER(power_domain) = {
 	.id		= UCLASS_POWER_DOMAIN,
 	.name		= "power_domain",
-	.per_device_auto = sizeof(struct power_domain_priv),
 };
diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c
index 5b530974e94..08c15ef342b 100644
--- a/drivers/power/domain/sandbox-power-domain-test.c
+++ b/drivers/power/domain/sandbox-power-domain-test.c
@@ -51,5 +51,4 @@ U_BOOT_DRIVER(sandbox_power_domain_test) = {
 	.id = UCLASS_MISC,
 	.of_match = sandbox_power_domain_test_ids,
 	.priv_auto	= sizeof(struct sandbox_power_domain_test),
-	.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
 };
diff --git a/include/power-domain.h b/include/power-domain.h
index ad33dea76ce..18525073e5e 100644
--- a/include/power-domain.h
+++ b/include/power-domain.h
@@ -147,81 +147,37 @@ static inline int power_domain_free(struct power_domain *power_domain)
 #endif
 
 /**
- * power_domain_on_lowlevel - Enable power to a power domain (with refcounting)
+ * power_domain_on - Enable power to a power domain.
  *
  * @power_domain:	A power domain struct that was previously successfully
  *		requested by power_domain_get().
- * Return: 0 if the transition has been performed correctly,
- *         -EALREADY if the domain is already on,
- *         a negative error code otherwise.
+ * Return: 0 if OK, or a negative error code.
  */
 #if CONFIG_IS_ENABLED(POWER_DOMAIN)
-int power_domain_on_lowlevel(struct power_domain *power_domain);
+int power_domain_on(struct power_domain *power_domain);
 #else
-static inline int power_domain_on_lowlevel(struct power_domain *power_domain)
+static inline int power_domain_on(struct power_domain *power_domain)
 {
 	return -ENOSYS;
 }
 #endif
 
 /**
- * power_domain_on - Enable power to a power domain (ignores the actual state
- *		      of the power domain)
- *
- * @power_domain:	A power domain struct that was previously successfully
- *		requested by power_domain_get().
- * Return: a negative error code upon error during the transition, 0 otherwise.
- */
-static inline int power_domain_on(struct power_domain *power_domain)
-{
-	int ret;
-
-	ret = power_domain_on_lowlevel(power_domain);
-	if (ret == -EALREADY)
-		ret = 0;
-
-	return ret;
-}
-
-/**
- * power_domain_off_lowlevel - Disable power to a power domain (with refcounting)
+ * power_domain_off - Disable power to a power domain.
  *
  * @power_domain:	A power domain struct that was previously successfully
  *		requested by power_domain_get().
- * Return: 0 if the transition has been performed correctly,
- *         -EALREADY if the domain is already off,
- *         -EBUSY if another device is keeping the domain on (but the refcounter
- *         is decremented),
- *         a negative error code otherwise.
+ * Return: 0 if OK, or a negative error code.
  */
 #if CONFIG_IS_ENABLED(POWER_DOMAIN)
-int power_domain_off_lowlevel(struct power_domain *power_domain);
+int power_domain_off(struct power_domain *power_domain);
 #else
-static inline int power_domain_off_lowlevel(struct power_domain *power_domain)
+static inline int power_domain_off(struct power_domain *power_domain)
 {
 	return -ENOSYS;
 }
 #endif
 
-/**
- * power_domain_off - Disable power to a power domain (ignores the actual state
- *		      of the power domain)
- *
- * @power_domain:	A power domain struct that was previously successfully
- *		requested by power_domain_get().
- * Return: a negative error code upon error during the transition, 0 otherwise.
- */
-static inline int power_domain_off(struct power_domain *power_domain)
-{
-	int ret;
-
-	ret = power_domain_off_lowlevel(power_domain);
-	if (ret == -EALREADY || ret == -EBUSY)
-		ret = 0;
-
-	return ret;
-}
-
 /**
  * dev_power_domain_on - Enable power domains for a device .
  *
diff --git a/test/dm/power-domain.c b/test/dm/power-domain.c
index 8a95f6bdb90..896cf5b2ae9 100644
--- a/test/dm/power-domain.c
+++ b/test/dm/power-domain.c
@@ -27,7 +27,7 @@ static int dm_test_power_domain(struct unit_test_state *uts)
 
 	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "power-domain-test",
 					      &dev_test));
-	ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
+	ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
 						  TEST_POWER_DOMAIN));
 	ut_assertok(sandbox_power_domain_test_get(dev_test));
 
-- 
2.34.1

Re: [PATCH] Revert "power-domain: Add refcounting"

[Miquel Raynal]

Hi Tom,

On 17/04/2025 at 13:53:11 +02, Wadim Egorov <w.egorov@phytec.de> wrote:

> Unfortunately this change breaks boot on K3 platform.
> U-Boot will hang after:
>
>   U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
>   SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')
>
> This reverts commit 197376fbf300e92afa0a1583815d9c9eb52d613a as
> suggested in [1].
>
> [1] https://lists.denx.de/pipermail/u-boot/2025-April/587032.html
>
> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>

This sadly needs to get in, there have been 3 or 4 reports already, as
many boards are broken. We need to find another way to add refcounting
(otherwise some imx8* features will stay broken), but it requires more
thinking time.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH] Revert "power-domain: Add refcounting"

[Tom Rini]

On Thu, 17 Apr 2025 13:53:11 +0200, Wadim Egorov wrote:

> Unfortunately this change breaks boot on K3 platform.
> U-Boot will hang after:
> 
>   U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
>   SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')
> 
> This reverts commit 197376fbf300e92afa0a1583815d9c9eb52d613a as
> suggested in [1].
> 
> [...]

Applied to u-boot/master, thanks!

[1/1] Revert "power-domain: Add refcounting"
      commit: 71f497a6d3908574a1b1c3813c2a1385910f76bd
-- 
Tom

Re: [PATCH] Revert "power-domain: Add refcounting"

[Simon Glass]

Hi Miquel,

On Fri, 18 Apr 2025 at 01:10, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Tom,
>
> On 17/04/2025 at 13:53:11 +02, Wadim Egorov <w.egorov@phytec.de> wrote:
>
> > Unfortunately this change breaks boot on K3 platform.
> > U-Boot will hang after:
> >
> >   U-Boot SPL 2025.04-01050-ga40fc5afaec0 (Apr 14 2025 - 07:31:32 +0000)
> >   SYSFW ABI: 3.1 (firmware rev 0x0009 '9.2.7--v09.02.07 (Kool Koala)')
> >
> > This reverts commit 197376fbf300e92afa0a1583815d9c9eb52d613a as
> > suggested in [1].
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2025-April/587032.html
> >
> > Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>
> This sadly needs to get in, there have been 3 or 4 reports already, as
> many boards are broken. We need to find another way to add refcounting
> (otherwise some imx8* features will stay broken), but it requires more
> thinking time.
>
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

We had this discussion on your patch and you ended up with a
'low-level' function to provide the base functionality. But it would
be easier if you retained the current behaviour and then added
ref-counting to two new functions. Then people can migrate.

Of course, people perhaps won't migrate. This sort of thing is quite
tricky. But I still think this would be an easier route. You could
perhaps even add a Kconfig option like POWER_DOMAIN_NO_REF_COUNT which
uses the current behaviour.

Regards,
Simon

Re: [PATCH] Revert "power-domain: Add refcounting"

[Miquel Raynal]

Hi Simon,

>> This sadly needs to get in, there have been 3 or 4 reports already, as
>> many boards are broken. We need to find another way to add refcounting
>> (otherwise some imx8* features will stay broken), but it requires more
>> thinking time.
>>
>> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> We had this discussion on your patch and you ended up with a
> 'low-level' function to provide the base functionality. But it would
> be easier if you retained the current behaviour and then added
> ref-counting to two new functions. Then people can migrate.

It's not the fact that people are using refcounting which breaks. It is
the fact that the device model does not support well the fact that one
DT node might be used to picture several devices. The clock subsystem
handles that by registering all clocks manually, and at some point
abuses one of the uclass internal structures (there is a FIXME about
that). It's not even the drivers that break.

> Of course, people perhaps won't migrate. This sort of thing is quite
> tricky. But I still think this would be an easier route. You could
> perhaps even add a Kconfig option like POWER_DOMAIN_NO_REF_COUNT which
> uses the current behaviour.

I now have an idea how to improve the power domain uclass to make it
fit, let's see if that works, otherwise I might fallback to this
solution indeed.

Thanks,
Miquèl