* [PATCH 0/2] power-domain: Enable uclass refcounting
@ 2025-04-25 6:49 Miquel Raynal
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Miquel Raynal @ 2025-04-25 6:49 UTC (permalink / raw)
To: Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Heiko Schocher, Fabio Estevam, Miquel Raynal
On one side we have a power domain uclass which is mostly clueless about
the topology of the power domains it manages.
On the other side we have SoCs which are getting more and more complex,
ie. with several layers of power domains. Sometimes (eg. on TI K3
platforms) all power domains are defined by a single device tree node
and the "subdomains" are referenced using an identification
cell (#power-domain-cells = <1 or more>). Sometimes however (eg. on NXP
i.MX8 SoCs), the description uses many device tree nodes.
When a power domain is referenced several times, it is likely that the
power_domain_on() function will be called several times in a row, which
in some cases may lead to glitches or even breakages. This situation is
problematic on i.MX8MP and only with proper reference counting we can
safely support the video pipeline.
There was a first attempt to bring refcount support to the power domain
uclass, but it miserably failed because of the details mentioned
above:
https://lore.kernel.org/u-boot/20250403-ge-mainline-display-support-v6-5-478b5e3dd872@bootlin.com/
So here is a new version of it, which takes into consideration:
- the fact that a single power domain udev can target several power
domain IDs (referred as "subdomains")
- the fact that some platforms might count on a uneven count of on/off
calls to work, and "fixing" this may break the platforms.
So refcounting is an opt-in parameter, it is a matter of filling the
platform (public part of a per-uclass structure) subdomains number with
the number of subdomains this device nodes features (one per
power_domain->id) in the probe function of the power domain driver.
This series shows how it can be done by enabling refcounting on i.MX8MP.
Samuel, Neha, Wadim, Heiko, could you please confirm it works on your
side?
For once, CI is green :-)
https://github.com/u-boot/u-boot/pull/753
---
Miquel Raynal (2):
power-domain: Add support for refcounting (again)
imx: power-domain: Enable refcounting on imx8mp
arch/sandbox/include/asm/power-domain.h | 2 +
drivers/firmware/scmi/sandbox-scmi_devices.c | 1 +
drivers/power/domain/imx8m-power-domain.c | 4 ++
drivers/power/domain/imx8mp-hsiomix.c | 4 ++
drivers/power/domain/imx8mp-mediamix.c | 4 ++
drivers/power/domain/power-domain-uclass.c | 90 ++++++++++++++++++++++--
drivers/power/domain/sandbox-power-domain-test.c | 15 ++++
drivers/power/domain/sandbox-power-domain.c | 4 ++
include/power-domain.h | 69 +++++++++++++++---
test/dm/power-domain.c | 11 ++-
10 files changed, 189 insertions(+), 15 deletions(-)
---
base-commit: 233fda6af674736dbc6ff37a9ef003b9fa4b8074
change-id: 20250424-ge-ian-display-support-a305ddb47f2b
Best regards,
--
Miquel Raynal <miquel.raynal@bootlin.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] power-domain: Add support for refcounting (again)
2025-04-25 6:49 [PATCH 0/2] power-domain: Enable uclass refcounting Miquel Raynal
@ 2025-04-25 6:49 ` Miquel Raynal
2025-04-25 8:57 ` Wadim Egorov
` (2 more replies)
2025-04-25 6:49 ` [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp Miquel Raynal
` (2 subsequent siblings)
3 siblings, 3 replies; 13+ messages in thread
From: Miquel Raynal @ 2025-04-25 6:49 UTC (permalink / raw)
To: Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Heiko Schocher, Fabio Estevam, Miquel Raynal
It is very surprising that such an uclass, specifically designed to
handle resources that may be shared by different devices, is not keeping
the count of the number of times a power domain has been
enabled/disabled to avoid shutting it down unexpectedly or disabling it
several times.
Doing this causes troubles on eg. i.MX8MP because disabling power
domains can be done in recursive loops were the same power domain
disabled up to 4 times in a row. PGCs seem to have tight FSM internal
timings to respect and it is easy to produce a race condition that puts
the power domains in an unstable state, leading to ADB400 errors and
later crashes in Linux.
Some drivers implement their own mechanism for that, but it is probably
best to add this feature in the uclass and share the common code across
drivers. In order to avoid breaking existing drivers, refcounting is
only enabled if the number of subdomains a device node supports is
explicitly set in the probe function. ->xlate() callbacks will return
the power domain ID which is then being used as the array index to reach
the correct refcounter.
As we do not want to break existing users while stile getting
interesting error codes, the implementation is split between:
- a low-level helper reporting error codes if the requested transition
could not be operated,
- a higher-level helper ignoring the "non error" codes, like EALREADY and
EBUSY.
CI tests using power domains are slightly updated to make sure the count
of on/off calls is even and the results match what we *now* expect. They
are also extended to test the low-level functions.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
arch/sandbox/include/asm/power-domain.h | 2 +
drivers/firmware/scmi/sandbox-scmi_devices.c | 1 +
drivers/power/domain/power-domain-uclass.c | 90 ++++++++++++++++++++++--
drivers/power/domain/sandbox-power-domain-test.c | 15 ++++
drivers/power/domain/sandbox-power-domain.c | 4 ++
include/power-domain.h | 69 +++++++++++++++---
test/dm/power-domain.c | 11 ++-
7 files changed, 177 insertions(+), 15 deletions(-)
diff --git a/arch/sandbox/include/asm/power-domain.h b/arch/sandbox/include/asm/power-domain.h
index 4d5e861dbce2b6434ac9bcffe5fc8f704d32e62d..3b0717f8fa06f1c0493fe6ee758e2e72ff77141e 100644
--- a/arch/sandbox/include/asm/power-domain.h
+++ b/arch/sandbox/include/asm/power-domain.h
@@ -13,6 +13,8 @@ int sandbox_power_domain_query(struct udevice *dev, unsigned long id);
int sandbox_power_domain_test_get(struct udevice *dev);
int sandbox_power_domain_test_on(struct udevice *dev);
int sandbox_power_domain_test_off(struct udevice *dev);
+int sandbox_power_domain_test_on_ll(struct udevice *dev);
+int sandbox_power_domain_test_off_ll(struct udevice *dev);
int sandbox_power_domain_test_free(struct udevice *dev);
#endif
diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644
--- a/drivers/firmware/scmi/sandbox-scmi_devices.c
+++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
@@ -163,4 +163,5 @@ 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 938bd8cbc9ffd1ba2109d702f886b6a99288d063..d9fa8ad4bd2126ea564fd5be8124035946dbd432 100644
--- a/drivers/power/domain/power-domain-uclass.c
+++ b/drivers/power/domain/power-domain-uclass.c
@@ -12,6 +12,10 @@
#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;
@@ -107,22 +111,67 @@ int power_domain_free(struct power_domain *power_domain)
return ops->rfree ? ops->rfree(power_domain) : 0;
}
-int power_domain_on(struct power_domain *power_domain)
+int power_domain_on_lowlevel(struct power_domain *power_domain)
{
+ struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
+ struct power_domain_plat *plat = dev_get_uclass_plat(power_domain->dev);
struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
+ int *on_count = plat->subdomains ? &priv->on_count[power_domain->id] : NULL;
+ int ret;
- debug("%s(power_domain=%p)\n", __func__, power_domain);
+ /* Refcounting is not enabled on all drivers by default */
+ if (on_count) {
+ debug("Enable power domain %s.%ld: %d -> %d (%s)\n",
+ power_domain->dev->name, power_domain->id, *on_count, *on_count + 1,
+ (((*on_count + 1) > 1) ? "EALREADY" : "todo"));
- return ops->on ? ops->on(power_domain) : 0;
+ (*on_count)++;
+ if (*on_count > 1)
+ return -EALREADY;
+ }
+
+ ret = ops->on ? ops->on(power_domain) : 0;
+ if (ret) {
+ if (on_count)
+ (*on_count)--;
+ return ret;
+ }
+
+ return 0;
}
-int power_domain_off(struct power_domain *power_domain)
+int power_domain_off_lowlevel(struct power_domain *power_domain)
{
+ struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
+ struct power_domain_plat *plat = dev_get_uclass_plat(power_domain->dev);
struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
+ int *on_count = plat->subdomains ? &priv->on_count[power_domain->id] : NULL;
+ int ret;
- debug("%s(power_domain=%p)\n", __func__, power_domain);
+ /* Refcounting is not enabled on all drivers by default */
+ if (on_count) {
+ debug("Disable power domain %s.%ld: %d -> %d (%s%s)\n",
+ power_domain->dev->name, power_domain->id, *on_count, *on_count - 1,
+ (((*on_count) <= 0) ? "EALREADY" : ""),
+ (((*on_count - 1) > 0) ? "BUSY" : "todo"));
- return ops->off ? ops->off(power_domain) : 0;
+ if (*on_count <= 0)
+ return -EALREADY;
+
+ (*on_count)--;
+ if (*on_count > 0)
+ return -EBUSY;
+ }
+
+ ret = ops->off ? ops->off(power_domain) : 0;
+ if (ret) {
+ if (on_count)
+ (*on_count)++;
+
+ return ret;
+ }
+
+ return 0;
}
#if CONFIG_IS_ENABLED(OF_REAL)
@@ -177,7 +226,36 @@ int dev_power_domain_off(struct udevice *dev)
}
#endif /* OF_REAL */
+static int power_domain_post_probe(struct udevice *dev)
+{
+ struct power_domain_priv *priv = dev_get_uclass_priv(dev);
+ struct power_domain_plat *plat = dev_get_uclass_plat(dev);
+
+ if (plat->subdomains) {
+ priv->on_count = calloc(sizeof(int), plat->subdomains);
+ if (!priv->on_count)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int power_domain_pre_remove(struct udevice *dev)
+{
+ struct power_domain_priv *priv = dev_get_uclass_priv(dev);
+ struct power_domain_plat *plat = dev_get_uclass_plat(dev);
+
+ if (plat->subdomains)
+ free(priv->on_count);
+
+ return 0;
+}
+
UCLASS_DRIVER(power_domain) = {
.id = UCLASS_POWER_DOMAIN,
.name = "power_domain",
+ .post_probe = power_domain_post_probe,
+ .pre_remove = power_domain_pre_remove,
+ .per_device_auto = sizeof(struct power_domain_priv),
+ .per_device_plat_auto = sizeof(struct power_domain_plat),
};
diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c
index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..df063001f517cae92df6b04a213c81b0d5584d18 100644
--- a/drivers/power/domain/sandbox-power-domain-test.c
+++ b/drivers/power/domain/sandbox-power-domain-test.c
@@ -34,6 +34,20 @@ int sandbox_power_domain_test_off(struct udevice *dev)
return power_domain_off(&sbrt->pd);
}
+int sandbox_power_domain_test_on_ll(struct udevice *dev)
+{
+ struct sandbox_power_domain_test *sbrt = dev_get_priv(dev);
+
+ return power_domain_on_lowlevel(&sbrt->pd);
+}
+
+int sandbox_power_domain_test_off_ll(struct udevice *dev)
+{
+ struct sandbox_power_domain_test *sbrt = dev_get_priv(dev);
+
+ return power_domain_off_lowlevel(&sbrt->pd);
+}
+
int sandbox_power_domain_test_free(struct udevice *dev)
{
struct sandbox_power_domain_test *sbrt = dev_get_priv(dev);
@@ -51,4 +65,5 @@ 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/drivers/power/domain/sandbox-power-domain.c b/drivers/power/domain/sandbox-power-domain.c
index 9dd490b14a3f6e502baccd94d32704e4b6bd56ed..a80316576384b27dc8159e81b5c79fc355af2860 100644
--- a/drivers/power/domain/sandbox-power-domain.c
+++ b/drivers/power/domain/sandbox-power-domain.c
@@ -64,8 +64,12 @@ static int sandbox_power_domain_bind(struct udevice *dev)
static int sandbox_power_domain_probe(struct udevice *dev)
{
+ struct power_domain_plat *plat = dev_get_uclass_plat(dev);
+
debug("%s(dev=%p)\n", __func__, dev);
+ plat->subdomains = 1;
+
return 0;
}
diff --git a/include/power-domain.h b/include/power-domain.h
index 18525073e5e3534fcbac6fae4e18462f29a4dc49..7fd2c5e365b54889a156d0f0b969fae490ac41a7 100644
--- a/include/power-domain.h
+++ b/include/power-domain.h
@@ -65,6 +65,15 @@ struct power_domain {
void *priv;
};
+/**
+ * struct power_domain_plat - Per device accessible structure
+ * @subdomains: Number of subdomains covered by this device, required
+ * for refcounting
+ */
+struct power_domain_plat {
+ int subdomains;
+};
+
/**
* power_domain_get - Get/request the power domain for a device.
*
@@ -147,37 +156,81 @@ static inline int power_domain_free(struct power_domain *power_domain)
#endif
/**
- * power_domain_on - Enable power to a power domain.
+ * power_domain_on_lowlevel - Enable power to a power domain (with refcounting)
*
* @power_domain: A power domain struct that was previously successfully
* requested by power_domain_get().
- * Return: 0 if OK, or a negative error code.
+ * Return: 0 if the transition has been performed correctly,
+ * -EALREADY if the domain is already on,
+ * a negative error code otherwise.
*/
#if CONFIG_IS_ENABLED(POWER_DOMAIN)
-int power_domain_on(struct power_domain *power_domain);
+int power_domain_on_lowlevel(struct power_domain *power_domain);
#else
-static inline int power_domain_on(struct power_domain *power_domain)
+static inline int power_domain_on_lowlevel(struct power_domain *power_domain)
{
return -ENOSYS;
}
#endif
/**
- * power_domain_off - Disable power to a power domain.
+ * 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: 0 if OK, or a negative error code.
+ * 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: 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.
*/
#if CONFIG_IS_ENABLED(POWER_DOMAIN)
-int power_domain_off(struct power_domain *power_domain);
+int power_domain_off_lowlevel(struct power_domain *power_domain);
#else
-static inline int power_domain_off(struct power_domain *power_domain)
+static inline int power_domain_off_lowlevel(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 896cf5b2ae9d26701150fad70e888f8b135a22b0..1002d831764b5a62b05631aee0d70c5609b8574b 100644
--- a/test/dm/power-domain.c
+++ b/test/dm/power-domain.c
@@ -27,17 +27,26 @@ 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(1, sandbox_power_domain_query(dev_power_domain,
+ ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
TEST_POWER_DOMAIN));
ut_assertok(sandbox_power_domain_test_get(dev_test));
ut_assertok(sandbox_power_domain_test_on(dev_test));
ut_asserteq(0, sandbox_power_domain_query(dev_power_domain, 0));
+ ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
+ TEST_POWER_DOMAIN));
+ ut_asserteq(-EALREADY, sandbox_power_domain_test_on_ll(dev_test));
+ ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
+ TEST_POWER_DOMAIN));
+ ut_asserteq(-EBUSY, sandbox_power_domain_test_off_ll(dev_test));
ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
TEST_POWER_DOMAIN));
ut_assertok(sandbox_power_domain_test_off(dev_test));
ut_asserteq(0, sandbox_power_domain_query(dev_power_domain, 0));
+ ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
+ TEST_POWER_DOMAIN));
+ ut_asserteq(-EALREADY, sandbox_power_domain_test_off_ll(dev_test));
ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
TEST_POWER_DOMAIN));
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp
2025-04-25 6:49 [PATCH 0/2] power-domain: Enable uclass refcounting Miquel Raynal
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
@ 2025-04-25 6:49 ` Miquel Raynal
2025-04-25 11:13 ` Heiko Schocher
2025-07-21 16:01 ` Frieder Schrempf
2025-04-25 8:56 ` [PATCH 0/2] power-domain: Enable uclass refcounting Wadim Egorov
2025-04-28 16:11 ` Fabio Estevam
3 siblings, 2 replies; 13+ messages in thread
From: Miquel Raynal @ 2025-04-25 6:49 UTC (permalink / raw)
To: Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Heiko Schocher, Fabio Estevam, Miquel Raynal
Prevent enabling/disabling multiple times the same power domain to avoid
breakages due to the same power domains being referenced several times
by different device nodes.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/power/domain/imx8m-power-domain.c | 4 ++++
drivers/power/domain/imx8mp-hsiomix.c | 4 ++++
drivers/power/domain/imx8mp-mediamix.c | 4 ++++
3 files changed, 12 insertions(+)
diff --git a/drivers/power/domain/imx8m-power-domain.c b/drivers/power/domain/imx8m-power-domain.c
index e54ba5d9a5476f678fb48fb16e7217b031acd78a..b44aae78e6de9733e1e53bbec80f54562487a76b 100644
--- a/drivers/power/domain/imx8m-power-domain.c
+++ b/drivers/power/domain/imx8m-power-domain.c
@@ -506,8 +506,12 @@ static int imx8m_power_domain_bind(struct udevice *dev)
static int imx8m_power_domain_probe(struct udevice *dev)
{
struct imx8m_power_domain_plat *pdata = dev_get_plat(dev);
+ struct power_domain_plat *plat = dev_get_uclass_plat(dev);
int ret;
+ /* Every subdomain has its own device node */
+ plat->subdomains = 1;
+
/* Nothing to do for non-"power-domain" driver instances. */
if (!strstr(dev->name, "power-domain"))
return 0;
diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
index 455ad53ef525e18ae45068fa5d8c2be8a1b79335..1ca43880ef56a03ae10e70f7e260022acc447dbb 100644
--- a/drivers/power/domain/imx8mp-hsiomix.c
+++ b/drivers/power/domain/imx8mp-hsiomix.c
@@ -201,8 +201,12 @@ int imx8mp_hsiomix_bind(struct udevice *dev)
static int imx8mp_hsiomix_probe(struct udevice *dev)
{
struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
+ struct power_domain_plat *plat = dev_get_uclass_plat(dev);
int ret;
+ /* Definitions are in imx8mp-power.h */
+ plat->subdomains = 5;
+
priv->base = dev_read_addr_ptr(dev);
ret = clk_get_by_name(dev, "usb", &priv->clk_usb);
diff --git a/drivers/power/domain/imx8mp-mediamix.c b/drivers/power/domain/imx8mp-mediamix.c
index 78c32ca3d3a87febdefd5d128d39d817674b8d32..504c22f7d3631363d76eb21e43afd854258d4ea5 100644
--- a/drivers/power/domain/imx8mp-mediamix.c
+++ b/drivers/power/domain/imx8mp-mediamix.c
@@ -143,9 +143,13 @@ static int imx8mp_mediamix_bind(struct udevice *dev)
static int imx8mp_mediamix_probe(struct udevice *dev)
{
+ struct power_domain_plat *plat = dev_get_uclass_plat(dev);
struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
int ret;
+ /* Definitions are in imx8mp-power.h */
+ plat->subdomains = 9;
+
priv->base = dev_read_addr_ptr(dev);
ret = clk_get_by_name(dev, "apb", &priv->clk_apb);
--
2.48.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] power-domain: Enable uclass refcounting
2025-04-25 6:49 [PATCH 0/2] power-domain: Enable uclass refcounting Miquel Raynal
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
2025-04-25 6:49 ` [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp Miquel Raynal
@ 2025-04-25 8:56 ` Wadim Egorov
2025-04-28 16:11 ` Fabio Estevam
3 siblings, 0 replies; 13+ messages in thread
From: Wadim Egorov @ 2025-04-25 8:56 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Heiko Schocher, Fabio Estevam
Am 25.04.25 um 09:49 schrieb Miquel Raynal:
> On one side we have a power domain uclass which is mostly clueless about
> the topology of the power domains it manages.
>
> On the other side we have SoCs which are getting more and more complex,
> ie. with several layers of power domains. Sometimes (eg. on TI K3
> platforms) all power domains are defined by a single device tree node
> and the "subdomains" are referenced using an identification
> cell (#power-domain-cells = <1 or more>). Sometimes however (eg. on NXP
> i.MX8 SoCs), the description uses many device tree nodes.
>
> When a power domain is referenced several times, it is likely that the
> power_domain_on() function will be called several times in a row, which
> in some cases may lead to glitches or even breakages. This situation is
> problematic on i.MX8MP and only with proper reference counting we can
> safely support the video pipeline.
>
> There was a first attempt to bring refcount support to the power domain
> uclass, but it miserably failed because of the details mentioned
> above:
> https://lore.kernel.org/u-boot/20250403-ge-mainline-display-support-v6-5-478b5e3dd872@bootlin.com/
>
> So here is a new version of it, which takes into consideration:
> - the fact that a single power domain udev can target several power
> domain IDs (referred as "subdomains")
> - the fact that some platforms might count on a uneven count of on/off
> calls to work, and "fixing" this may break the platforms.
>
> So refcounting is an opt-in parameter, it is a matter of filling the
> platform (public part of a per-uclass structure) subdomains number with
> the number of subdomains this device nodes features (one per
> power_domain->id) in the probe function of the power domain driver.
>
> This series shows how it can be done by enabling refcounting on i.MX8MP.
>
> Samuel, Neha, Wadim, Heiko, could you please confirm it works on your
> side?
Booting works for me on the phycore-am62x.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] power-domain: Add support for refcounting (again)
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
@ 2025-04-25 8:57 ` Wadim Egorov
2025-04-25 9:49 ` Neha Malcom Francis
2025-04-25 11:12 ` Heiko Schocher
2 siblings, 0 replies; 13+ messages in thread
From: Wadim Egorov @ 2025-04-25 8:57 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Heiko Schocher, Fabio Estevam
Am 25.04.25 um 09:49 schrieb Miquel Raynal:
> It is very surprising that such an uclass, specifically designed to
> handle resources that may be shared by different devices, is not keeping
> the count of the number of times a power domain has been
> enabled/disabled to avoid shutting it down unexpectedly or disabling it
> several times.
>
> Doing this causes troubles on eg. i.MX8MP because disabling power
> domains can be done in recursive loops were the same power domain
> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> timings to respect and it is easy to produce a race condition that puts
> the power domains in an unstable state, leading to ADB400 errors and
> later crashes in Linux.
>
> Some drivers implement their own mechanism for that, but it is probably
> best to add this feature in the uclass and share the common code across
> drivers. In order to avoid breaking existing drivers, refcounting is
> only enabled if the number of subdomains a device node supports is
> explicitly set in the probe function. ->xlate() callbacks will return
> the power domain ID which is then being used as the array index to reach
> the correct refcounter.
>
> As we do not want to break existing users while stile getting
> interesting error codes, the implementation is split between:
> - a low-level helper reporting error codes if the requested transition
> could not be operated,
> - a higher-level helper ignoring the "non error" codes, like EALREADY and
> EBUSY.
>
> CI tests using power domains are slightly updated to make sure the count
> of on/off calls is even and the results match what we *now* expect. They
> are also extended to test the low-level functions.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Tested-by: Wadim Egorov <w.egorov@phytec.de> # On phycore-am62x
> ---
> arch/sandbox/include/asm/power-domain.h | 2 +
> drivers/firmware/scmi/sandbox-scmi_devices.c | 1 +
> drivers/power/domain/power-domain-uclass.c | 90 ++++++++++++++++++++++--
> drivers/power/domain/sandbox-power-domain-test.c | 15 ++++
> drivers/power/domain/sandbox-power-domain.c | 4 ++
> include/power-domain.h | 69 +++++++++++++++---
> test/dm/power-domain.c | 11 ++-
> 7 files changed, 177 insertions(+), 15 deletions(-)
>
> diff --git a/arch/sandbox/include/asm/power-domain.h b/arch/sandbox/include/asm/power-domain.h
> index 4d5e861dbce2b6434ac9bcffe5fc8f704d32e62d..3b0717f8fa06f1c0493fe6ee758e2e72ff77141e 100644
> --- a/arch/sandbox/include/asm/power-domain.h
> +++ b/arch/sandbox/include/asm/power-domain.h
> @@ -13,6 +13,8 @@ int sandbox_power_domain_query(struct udevice *dev, unsigned long id);
> int sandbox_power_domain_test_get(struct udevice *dev);
> int sandbox_power_domain_test_on(struct udevice *dev);
> int sandbox_power_domain_test_off(struct udevice *dev);
> +int sandbox_power_domain_test_on_ll(struct udevice *dev);
> +int sandbox_power_domain_test_off_ll(struct udevice *dev);
> int sandbox_power_domain_test_free(struct udevice *dev);
>
> #endif
> diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
> index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> @@ -163,4 +163,5 @@ 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 938bd8cbc9ffd1ba2109d702f886b6a99288d063..d9fa8ad4bd2126ea564fd5be8124035946dbd432 100644
> --- a/drivers/power/domain/power-domain-uclass.c
> +++ b/drivers/power/domain/power-domain-uclass.c
> @@ -12,6 +12,10 @@
> #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;
> @@ -107,22 +111,67 @@ int power_domain_free(struct power_domain *power_domain)
> return ops->rfree ? ops->rfree(power_domain) : 0;
> }
>
> -int power_domain_on(struct power_domain *power_domain)
> +int power_domain_on_lowlevel(struct power_domain *power_domain)
> {
> + struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
> + struct power_domain_plat *plat = dev_get_uclass_plat(power_domain->dev);
> struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
> + int *on_count = plat->subdomains ? &priv->on_count[power_domain->id] : NULL;
> + int ret;
>
> - debug("%s(power_domain=%p)\n", __func__, power_domain);
> + /* Refcounting is not enabled on all drivers by default */
> + if (on_count) {
> + debug("Enable power domain %s.%ld: %d -> %d (%s)\n",
> + power_domain->dev->name, power_domain->id, *on_count, *on_count + 1,
> + (((*on_count + 1) > 1) ? "EALREADY" : "todo"));
>
> - return ops->on ? ops->on(power_domain) : 0;
> + (*on_count)++;
> + if (*on_count > 1)
> + return -EALREADY;
> + }
> +
> + ret = ops->on ? ops->on(power_domain) : 0;
> + if (ret) {
> + if (on_count)
> + (*on_count)--;
> + return ret;
> + }
> +
> + return 0;
> }
>
> -int power_domain_off(struct power_domain *power_domain)
> +int power_domain_off_lowlevel(struct power_domain *power_domain)
> {
> + struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
> + struct power_domain_plat *plat = dev_get_uclass_plat(power_domain->dev);
> struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
> + int *on_count = plat->subdomains ? &priv->on_count[power_domain->id] : NULL;
> + int ret;
>
> - debug("%s(power_domain=%p)\n", __func__, power_domain);
> + /* Refcounting is not enabled on all drivers by default */
> + if (on_count) {
> + debug("Disable power domain %s.%ld: %d -> %d (%s%s)\n",
> + power_domain->dev->name, power_domain->id, *on_count, *on_count - 1,
> + (((*on_count) <= 0) ? "EALREADY" : ""),
> + (((*on_count - 1) > 0) ? "BUSY" : "todo"));
>
> - return ops->off ? ops->off(power_domain) : 0;
> + if (*on_count <= 0)
> + return -EALREADY;
> +
> + (*on_count)--;
> + if (*on_count > 0)
> + return -EBUSY;
> + }
> +
> + ret = ops->off ? ops->off(power_domain) : 0;
> + if (ret) {
> + if (on_count)
> + (*on_count)++;
> +
> + return ret;
> + }
> +
> + return 0;
> }
>
> #if CONFIG_IS_ENABLED(OF_REAL)
> @@ -177,7 +226,36 @@ int dev_power_domain_off(struct udevice *dev)
> }
> #endif /* OF_REAL */
>
> +static int power_domain_post_probe(struct udevice *dev)
> +{
> + struct power_domain_priv *priv = dev_get_uclass_priv(dev);
> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
> +
> + if (plat->subdomains) {
> + priv->on_count = calloc(sizeof(int), plat->subdomains);
> + if (!priv->on_count)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int power_domain_pre_remove(struct udevice *dev)
> +{
> + struct power_domain_priv *priv = dev_get_uclass_priv(dev);
> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
> +
> + if (plat->subdomains)
> + free(priv->on_count);
> +
> + return 0;
> +}
> +
> UCLASS_DRIVER(power_domain) = {
> .id = UCLASS_POWER_DOMAIN,
> .name = "power_domain",
> + .post_probe = power_domain_post_probe,
> + .pre_remove = power_domain_pre_remove,
> + .per_device_auto = sizeof(struct power_domain_priv),
> + .per_device_plat_auto = sizeof(struct power_domain_plat),
> };
> diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c
> index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..df063001f517cae92df6b04a213c81b0d5584d18 100644
> --- a/drivers/power/domain/sandbox-power-domain-test.c
> +++ b/drivers/power/domain/sandbox-power-domain-test.c
> @@ -34,6 +34,20 @@ int sandbox_power_domain_test_off(struct udevice *dev)
> return power_domain_off(&sbrt->pd);
> }
>
> +int sandbox_power_domain_test_on_ll(struct udevice *dev)
> +{
> + struct sandbox_power_domain_test *sbrt = dev_get_priv(dev);
> +
> + return power_domain_on_lowlevel(&sbrt->pd);
> +}
> +
> +int sandbox_power_domain_test_off_ll(struct udevice *dev)
> +{
> + struct sandbox_power_domain_test *sbrt = dev_get_priv(dev);
> +
> + return power_domain_off_lowlevel(&sbrt->pd);
> +}
> +
> int sandbox_power_domain_test_free(struct udevice *dev)
> {
> struct sandbox_power_domain_test *sbrt = dev_get_priv(dev);
> @@ -51,4 +65,5 @@ 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/drivers/power/domain/sandbox-power-domain.c b/drivers/power/domain/sandbox-power-domain.c
> index 9dd490b14a3f6e502baccd94d32704e4b6bd56ed..a80316576384b27dc8159e81b5c79fc355af2860 100644
> --- a/drivers/power/domain/sandbox-power-domain.c
> +++ b/drivers/power/domain/sandbox-power-domain.c
> @@ -64,8 +64,12 @@ static int sandbox_power_domain_bind(struct udevice *dev)
>
> static int sandbox_power_domain_probe(struct udevice *dev)
> {
> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
> +
> debug("%s(dev=%p)\n", __func__, dev);
>
> + plat->subdomains = 1;
> +
> return 0;
> }
>
> diff --git a/include/power-domain.h b/include/power-domain.h
> index 18525073e5e3534fcbac6fae4e18462f29a4dc49..7fd2c5e365b54889a156d0f0b969fae490ac41a7 100644
> --- a/include/power-domain.h
> +++ b/include/power-domain.h
> @@ -65,6 +65,15 @@ struct power_domain {
> void *priv;
> };
>
> +/**
> + * struct power_domain_plat - Per device accessible structure
> + * @subdomains: Number of subdomains covered by this device, required
> + * for refcounting
> + */
> +struct power_domain_plat {
> + int subdomains;
> +};
> +
> /**
> * power_domain_get - Get/request the power domain for a device.
> *
> @@ -147,37 +156,81 @@ static inline int power_domain_free(struct power_domain *power_domain)
> #endif
>
> /**
> - * power_domain_on - Enable power to a power domain.
> + * power_domain_on_lowlevel - Enable power to a power domain (with refcounting)
> *
> * @power_domain: A power domain struct that was previously successfully
> * requested by power_domain_get().
> - * Return: 0 if OK, or a negative error code.
> + * Return: 0 if the transition has been performed correctly,
> + * -EALREADY if the domain is already on,
> + * a negative error code otherwise.
> */
> #if CONFIG_IS_ENABLED(POWER_DOMAIN)
> -int power_domain_on(struct power_domain *power_domain);
> +int power_domain_on_lowlevel(struct power_domain *power_domain);
> #else
> -static inline int power_domain_on(struct power_domain *power_domain)
> +static inline int power_domain_on_lowlevel(struct power_domain *power_domain)
> {
> return -ENOSYS;
> }
> #endif
>
> /**
> - * power_domain_off - Disable power to a power domain.
> + * 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: 0 if OK, or a negative error code.
> + * 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: 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.
> */
> #if CONFIG_IS_ENABLED(POWER_DOMAIN)
> -int power_domain_off(struct power_domain *power_domain);
> +int power_domain_off_lowlevel(struct power_domain *power_domain);
> #else
> -static inline int power_domain_off(struct power_domain *power_domain)
> +static inline int power_domain_off_lowlevel(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 896cf5b2ae9d26701150fad70e888f8b135a22b0..1002d831764b5a62b05631aee0d70c5609b8574b 100644
> --- a/test/dm/power-domain.c
> +++ b/test/dm/power-domain.c
> @@ -27,17 +27,26 @@ 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(1, sandbox_power_domain_query(dev_power_domain,
> + ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
> TEST_POWER_DOMAIN));
> ut_assertok(sandbox_power_domain_test_get(dev_test));
>
> ut_assertok(sandbox_power_domain_test_on(dev_test));
> ut_asserteq(0, sandbox_power_domain_query(dev_power_domain, 0));
> + ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
> + TEST_POWER_DOMAIN));
> + ut_asserteq(-EALREADY, sandbox_power_domain_test_on_ll(dev_test));
> + ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
> + TEST_POWER_DOMAIN));
> + ut_asserteq(-EBUSY, sandbox_power_domain_test_off_ll(dev_test));
> ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
> TEST_POWER_DOMAIN));
>
> ut_assertok(sandbox_power_domain_test_off(dev_test));
> ut_asserteq(0, sandbox_power_domain_query(dev_power_domain, 0));
> + ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
> + TEST_POWER_DOMAIN));
> + ut_asserteq(-EALREADY, sandbox_power_domain_test_off_ll(dev_test));
> ut_asserteq(0, sandbox_power_domain_query(dev_power_domain,
> TEST_POWER_DOMAIN));
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] power-domain: Add support for refcounting (again)
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
2025-04-25 8:57 ` Wadim Egorov
@ 2025-04-25 9:49 ` Neha Malcom Francis
2025-04-25 11:12 ` Heiko Schocher
2 siblings, 0 replies; 13+ messages in thread
From: Neha Malcom Francis @ 2025-04-25 9:49 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Wadim Egorov,
Heiko Schocher, Fabio Estevam
Hi Miquel
On 25/04/25 12:19, Miquel Raynal wrote:
> It is very surprising that such an uclass, specifically designed to
> handle resources that may be shared by different devices, is not keeping
> the count of the number of times a power domain has been
> enabled/disabled to avoid shutting it down unexpectedly or disabling it
> several times.
>
> Doing this causes troubles on eg. i.MX8MP because disabling power
> domains can be done in recursive loops were the same power domain
> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> timings to respect and it is easy to produce a race condition that puts
> the power domains in an unstable state, leading to ADB400 errors and
> later crashes in Linux.
>
> Some drivers implement their own mechanism for that, but it is probably
> best to add this feature in the uclass and share the common code across
> drivers. In order to avoid breaking existing drivers, refcounting is
> only enabled if the number of subdomains a device node supports is
> explicitly set in the probe function. ->xlate() callbacks will return
> the power domain ID which is then being used as the array index to reach
> the correct refcounter.
>
> As we do not want to break existing users while stile getting
> interesting error codes, the implementation is split between:
> - a low-level helper reporting error codes if the requested transition
> could not be operated,
> - a higher-level helper ignoring the "non error" codes, like EALREADY and
> EBUSY.
>
> CI tests using power domains are slightly updated to make sure the count
> of on/off calls is even and the results match what we *now* expect. They
> are also extended to test the low-level functions.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> arch/sandbox/include/asm/power-domain.h | 2 +
> drivers/firmware/scmi/sandbox-scmi_devices.c | 1 +
> drivers/power/domain/power-domain-uclass.c | 90 ++++++++++++++++++++++--
> drivers/power/domain/sandbox-power-domain-test.c | 15 ++++
> drivers/power/domain/sandbox-power-domain.c | 4 ++
> include/power-domain.h | 69 +++++++++++++++---
> test/dm/power-domain.c | 11 ++-
> 7 files changed, 177 insertions(+), 15 deletions(-)
>
> diff --git a/arch/sandbox/include/asm/power-domain.h b/arch/sandbox/include/asm/power-domain.h
> index 4d5e861dbce2b6434ac9bcffe5fc8f704d32e62d..3b0717f8fa06f1c0493fe6ee758e2e72ff77141e 100644
> --- a/arch/sandbox/include/asm/power-domain.h
> +++ b/arch/sandbox/include/asm/power-domain.h
> @@ -13,6 +13,8 @@ int sandbox_power_domain_query(struct udevice *dev, unsigned long id);
> int sandbox_power_domain_test_get(struct udevice *dev);
> int sandbox_power_domain_test_on(struct udevice *dev);
> int sandbox_power_domain_test_off(struct udevice *dev);
> +int sandbox_power_domain_test_on_ll(struct udevice *dev);
> +int sandbox_power_domain_test_off_ll(struct udevice *dev);
> int sandbox_power_domain_test_free(struct udevice *dev);
>
> #endif
> diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c
> index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> @@ -163,4 +163,5 @@ 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,
> };
[...]
Thanks for working on this quickly, I've booted it on j784s4-evm and
works fine! And I am good with this approach using subdomains.
Tested-by: Neha Malcom Francis <n-francis@ti.com>
(on j784s4-evm)
--
Thanking You
Neha Malcom Francis
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] power-domain: Add support for refcounting (again)
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
2025-04-25 8:57 ` Wadim Egorov
2025-04-25 9:49 ` Neha Malcom Francis
@ 2025-04-25 11:12 ` Heiko Schocher
2 siblings, 0 replies; 13+ messages in thread
From: Heiko Schocher @ 2025-04-25 11:12 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Fabio Estevam
Hello Miquel,
Thanks for the fast fix, and sorry, that I did not found the
time to look into faster...
On 25.04.25 08:49, Miquel Raynal wrote:
> It is very surprising that such an uclass, specifically designed to
> handle resources that may be shared by different devices, is not keeping
> the count of the number of times a power domain has been
> enabled/disabled to avoid shutting it down unexpectedly or disabling it
> several times.
>
> Doing this causes troubles on eg. i.MX8MP because disabling power
> domains can be done in recursive loops were the same power domain
> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
> timings to respect and it is easy to produce a race condition that puts
> the power domains in an unstable state, leading to ADB400 errors and
> later crashes in Linux.
>
> Some drivers implement their own mechanism for that, but it is probably
> best to add this feature in the uclass and share the common code across
> drivers. In order to avoid breaking existing drivers, refcounting is
> only enabled if the number of subdomains a device node supports is
> explicitly set in the probe function. ->xlate() callbacks will return
> the power domain ID which is then being used as the array index to reach
> the correct refcounter.
>
> As we do not want to break existing users while stile getting
> interesting error codes, the implementation is split between:
> - a low-level helper reporting error codes if the requested transition
> could not be operated,
> - a higher-level helper ignoring the "non error" codes, like EALREADY and
> EBUSY.
>
> CI tests using power domains are slightly updated to make sure the count
> of on/off calls is even and the results match what we *now* expect. They
> are also extended to test the low-level functions.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> arch/sandbox/include/asm/power-domain.h | 2 +
> drivers/firmware/scmi/sandbox-scmi_devices.c | 1 +
> drivers/power/domain/power-domain-uclass.c | 90 ++++++++++++++++++++++--
> drivers/power/domain/sandbox-power-domain-test.c | 15 ++++
> drivers/power/domain/sandbox-power-domain.c | 4 ++
> include/power-domain.h | 69 +++++++++++++++---
> test/dm/power-domain.c | 11 ++-
> 7 files changed, 177 insertions(+), 15 deletions(-)
after reverting locally commit "197376fbf300e92afa0a1583815d9c9eb52d613a."
and applying this patch and patch 2/2 of this series, "pci enum"
works again on imx8mp based board:
u-boot=> pci enum
PCIE-0: Link up (Gen1-x1, Bus0)
u-boot=> print update_ub_emmc
update_ub_emmc=run prepemmc loadub updub cmpub
u-boot=> run loadub
e1000: 00:30:d6:39:7a:c1
e1000: 00:30:d6:39:7a:c2
Using e1000#1 device
TFTP from server 192.168.3.1; our IP address is 192.168.3.51
Filename 'lec/20250327/flash.bin.4g'.
Load address: 0x50000000
Loading: ################################################## 2 MiB
354.5 KiB/s
done
Bytes transferred = 2097152 (200000 hex)
u-boot=>
Thanks!
Reviewed-by: Heiko Schocher <hs@denx.de>
Tested-by: Heiko Schocher <hs@denx.de>
bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp
2025-04-25 6:49 ` [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp Miquel Raynal
@ 2025-04-25 11:13 ` Heiko Schocher
2025-07-21 16:01 ` Frieder Schrempf
1 sibling, 0 replies; 13+ messages in thread
From: Heiko Schocher @ 2025-04-25 11:13 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Fabio Estevam
Hello Miquel,
On 25.04.25 08:49, Miquel Raynal wrote:
> Prevent enabling/disabling multiple times the same power domain to avoid
> breakages due to the same power domains being referenced several times
> by different device nodes.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> drivers/power/domain/imx8m-power-domain.c | 4 ++++
> drivers/power/domain/imx8mp-hsiomix.c | 4 ++++
> drivers/power/domain/imx8mp-mediamix.c | 4 ++++
> 3 files changed, 12 insertions(+)
Thanks!
Reviewed-by: Heiko Schocher <hs@denx.de>
Tested-by: Heiko Schocher <hs@denx.de>
bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] power-domain: Enable uclass refcounting
2025-04-25 6:49 [PATCH 0/2] power-domain: Enable uclass refcounting Miquel Raynal
` (2 preceding siblings ...)
2025-04-25 8:56 ` [PATCH 0/2] power-domain: Enable uclass refcounting Wadim Egorov
@ 2025-04-28 16:11 ` Fabio Estevam
3 siblings, 0 replies; 13+ messages in thread
From: Fabio Estevam @ 2025-04-28 16:11 UTC (permalink / raw)
To: Miquel Raynal
Cc: Simon Glass, Tom Rini, Jaehoon Chung, Thomas Petazzoni, u-boot,
Samuel Holland, Neha Malcom Francis, Wadim Egorov, Heiko Schocher,
Fabio Estevam
On Fri, Apr 25, 2025 at 11:09 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> On one side we have a power domain uclass which is mostly clueless about
> the topology of the power domains it manages.
>
> On the other side we have SoCs which are getting more and more complex,
> ie. with several layers of power domains. Sometimes (eg. on TI K3
> platforms) all power domains are defined by a single device tree node
> and the "subdomains" are referenced using an identification
> cell (#power-domain-cells = <1 or more>). Sometimes however (eg. on NXP
> i.MX8 SoCs), the description uses many device tree nodes.
>
> When a power domain is referenced several times, it is likely that the
> power_domain_on() function will be called several times in a row, which
> in some cases may lead to glitches or even breakages. This situation is
> problematic on i.MX8MP and only with proper reference counting we can
> safely support the video pipeline.
>
> There was a first attempt to bring refcount support to the power domain
> uclass, but it miserably failed because of the details mentioned
> above:
> https://lore.kernel.org/u-boot/20250403-ge-mainline-display-support-v6-5-478b5e3dd872@bootlin.com/
>
> So here is a new version of it, which takes into consideration:
> - the fact that a single power domain udev can target several power
> domain IDs (referred as "subdomains")
> - the fact that some platforms might count on a uneven count of on/off
> calls to work, and "fixing" this may break the platforms.
>
> So refcounting is an opt-in parameter, it is a matter of filling the
> platform (public part of a per-uclass structure) subdomains number with
> the number of subdomains this device nodes features (one per
> power_domain->id) in the probe function of the power domain driver.
>
> This series shows how it can be done by enabling refcounting on i.MX8MP.
>
> Samuel, Neha, Wadim, Heiko, could you please confirm it works on your
> side?
>
> For once, CI is green :-)
> https://github.com/u-boot/u-boot/pull/753
Applied, thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp
2025-04-25 6:49 ` [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp Miquel Raynal
2025-04-25 11:13 ` Heiko Schocher
@ 2025-07-21 16:01 ` Frieder Schrempf
2025-07-22 4:43 ` Neha Malcom Francis
2025-07-22 7:26 ` Frieder Schrempf
1 sibling, 2 replies; 13+ messages in thread
From: Frieder Schrempf @ 2025-07-21 16:01 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Heiko Schocher, Fabio Estevam
Hi Miquel,
Am 25.04.25 um 08:49 schrieb Miquel Raynal:
> Prevent enabling/disabling multiple times the same power domain to avoid
> breakages due to the same power domains being referenced several times
> by different device nodes.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
I've stumbled upon some issue that seems to be related to this patch. I
saw you already had to bother with other breakages of existing
boards/platforms in the previous implementation. Unfortunately it looks
like this still causes some kind of regression in my case.
I'm working with an i.MX8MM board (kontron-sl-mx8mm_defconfig) and I see
a crash as soon as I do "usb start" and then "usb stop" (log for the
latter see below).
As soon as I revert this patch everything starts to work fine again. Do
you have any suggestions? The power-domains involved here are pgc_otg1
and pgc_hsiomix defined in imx8mm.dtsi.
Thanks
Frieder
=> usb stop
stopping USB..
Sending event 6/(unknown) to spy 'efi_disk del'
Sending event 6/(unknown) to spy 'efi_disk del'
Looking for power-domain@0
Looking for power-domain@0
- checking gpc@303a0000
- checking power-domain@0
- result for power-domain@0: power-domain@0 (ret=0)
- result for power-domain@0: power-domain@0 (ret=0)
"Synchronous Abort" handler, esr 0x96000005, far 0x4ebacdc50
elr: 0000000040249c94 lr : 0000000040249c6c (reloc)
elr: 00000000fff4ec94 lr : 00000000fff4ec6c
x0 : 0000000000000001 x1 : 00000000fbf22750
x2 : 00000003efbab500 x3 : 00000000fffcdfb0
x4 : 0000000000000020 x5 : 0000000000000020
x6 : 00000000fbeea10f x7 : 00000000fbeea3d7
x8 : 00000000fffffffe x9 : 00000000fbeea1ec
x10: 00000000fbeea280 x11: 00000000ffffffd8
x12: 00000000fbeeb1b0 x13: 00000000fbeeb1b0
x14: 0000000000000002 x15: 0000000000000000
x16: 00000000fff4edc4 x17: 0000000000000000
x18: 00000000fbef4db0 x19: 00000004ebacdc50
x20: 00000000fbeeace8 x21: 00000000fbf21ad0
x22: 0000000000000000 x23: 0000000000000001
x24: 0000000000000000 x25: 00000000fbf28f10
x26: 0000000000000000 x27: 00000000fbf2ad40
x28: 00000000fbf2ada0 x29: 00000000fbeeac70
Code: f9400693 d37ef662 ab130833 54000140 (b8626820)
Resetting CPU ...
> ---
> drivers/power/domain/imx8m-power-domain.c | 4 ++++
> drivers/power/domain/imx8mp-hsiomix.c | 4 ++++
> drivers/power/domain/imx8mp-mediamix.c | 4 ++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/drivers/power/domain/imx8m-power-domain.c b/drivers/power/domain/imx8m-power-domain.c
> index e54ba5d9a5476f678fb48fb16e7217b031acd78a..b44aae78e6de9733e1e53bbec80f54562487a76b 100644
> --- a/drivers/power/domain/imx8m-power-domain.c
> +++ b/drivers/power/domain/imx8m-power-domain.c
> @@ -506,8 +506,12 @@ static int imx8m_power_domain_bind(struct udevice *dev)
> static int imx8m_power_domain_probe(struct udevice *dev)
> {
> struct imx8m_power_domain_plat *pdata = dev_get_plat(dev);
> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
> int ret;
>
> + /* Every subdomain has its own device node */
> + plat->subdomains = 1;
> +
> /* Nothing to do for non-"power-domain" driver instances. */
> if (!strstr(dev->name, "power-domain"))
> return 0;
> diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
> index 455ad53ef525e18ae45068fa5d8c2be8a1b79335..1ca43880ef56a03ae10e70f7e260022acc447dbb 100644
> --- a/drivers/power/domain/imx8mp-hsiomix.c
> +++ b/drivers/power/domain/imx8mp-hsiomix.c
> @@ -201,8 +201,12 @@ int imx8mp_hsiomix_bind(struct udevice *dev)
> static int imx8mp_hsiomix_probe(struct udevice *dev)
> {
> struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
> int ret;
>
> + /* Definitions are in imx8mp-power.h */
> + plat->subdomains = 5;
> +
> priv->base = dev_read_addr_ptr(dev);
>
> ret = clk_get_by_name(dev, "usb", &priv->clk_usb);
> diff --git a/drivers/power/domain/imx8mp-mediamix.c b/drivers/power/domain/imx8mp-mediamix.c
> index 78c32ca3d3a87febdefd5d128d39d817674b8d32..504c22f7d3631363d76eb21e43afd854258d4ea5 100644
> --- a/drivers/power/domain/imx8mp-mediamix.c
> +++ b/drivers/power/domain/imx8mp-mediamix.c
> @@ -143,9 +143,13 @@ static int imx8mp_mediamix_bind(struct udevice *dev)
>
> static int imx8mp_mediamix_probe(struct udevice *dev)
> {
> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
> struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
> int ret;
>
> + /* Definitions are in imx8mp-power.h */
> + plat->subdomains = 9;
> +
> priv->base = dev_read_addr_ptr(dev);
>
> ret = clk_get_by_name(dev, "apb", &priv->clk_apb);
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp
2025-07-21 16:01 ` Frieder Schrempf
@ 2025-07-22 4:43 ` Neha Malcom Francis
2025-07-22 7:26 ` Frieder Schrempf
1 sibling, 0 replies; 13+ messages in thread
From: Neha Malcom Francis @ 2025-07-22 4:43 UTC (permalink / raw)
To: Frieder Schrempf, Miquel Raynal, Simon Glass, Tom Rini,
Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Wadim Egorov,
Heiko Schocher, Fabio Estevam
Hi Frieder
On 21/07/25 21:31, Frieder Schrempf wrote:
> Hi Miquel,
>
> Am 25.04.25 um 08:49 schrieb Miquel Raynal:
>> Prevent enabling/disabling multiple times the same power domain to avoid
>> breakages due to the same power domains being referenced several times
>> by different device nodes.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> I've stumbled upon some issue that seems to be related to this patch. I
> saw you already had to bother with other breakages of existing
> boards/platforms in the previous implementation. Unfortunately it looks
> like this still causes some kind of regression in my case.
>
> I'm working with an i.MX8MM board (kontron-sl-mx8mm_defconfig) and I see
> a crash as soon as I do "usb start" and then "usb stop" (log for the
> latter see below).
>
> As soon as I revert this patch everything starts to work fine again. Do
> you have any suggestions? The power-domains involved here are pgc_otg1
> and pgc_hsiomix defined in imx8mm.dtsi.
>
> Thanks
> Frieder
>
>
> => usb stop
> stopping USB..
> Sending event 6/(unknown) to spy 'efi_disk del'
> Sending event 6/(unknown) to spy 'efi_disk del'
> Looking for power-domain@0
> Looking for power-domain@0
> - checking gpc@303a0000
> - checking power-domain@0
> - result for power-domain@0: power-domain@0 (ret=0)
> - result for power-domain@0: power-domain@0 (ret=0)
> "Synchronous Abort" handler, esr 0x96000005, far 0x4ebacdc50
> elr: 0000000040249c94 lr : 0000000040249c6c (reloc)
> elr: 00000000fff4ec94 lr : 00000000fff4ec6c
> x0 : 0000000000000001 x1 : 00000000fbf22750
> x2 : 00000003efbab500 x3 : 00000000fffcdfb0
> x4 : 0000000000000020 x5 : 0000000000000020
> x6 : 00000000fbeea10f x7 : 00000000fbeea3d7
> x8 : 00000000fffffffe x9 : 00000000fbeea1ec
> x10: 00000000fbeea280 x11: 00000000ffffffd8
> x12: 00000000fbeeb1b0 x13: 00000000fbeeb1b0
> x14: 0000000000000002 x15: 0000000000000000
> x16: 00000000fff4edc4 x17: 0000000000000000
> x18: 00000000fbef4db0 x19: 00000004ebacdc50
> x20: 00000000fbeeace8 x21: 00000000fbf21ad0
> x22: 0000000000000000 x23: 0000000000000001
> x24: 0000000000000000 x25: 00000000fbf28f10
> x26: 0000000000000000 x27: 00000000fbf2ad40
> x28: 00000000fbf2ada0 x29: 00000000fbeeac70
>
> Code: f9400693 d37ef662 ab130833 54000140 (b8626820)
> Resetting CPU ...
>
>
>> ---
>> drivers/power/domain/imx8m-power-domain.c | 4 ++++
>> drivers/power/domain/imx8mp-hsiomix.c | 4 ++++
>> drivers/power/domain/imx8mp-mediamix.c | 4 ++++
>> 3 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/power/domain/imx8m-power-domain.c b/drivers/power/domain/imx8m-power-domain.c
>> index e54ba5d9a5476f678fb48fb16e7217b031acd78a..b44aae78e6de9733e1e53bbec80f54562487a76b 100644
>> --- a/drivers/power/domain/imx8m-power-domain.c
>> +++ b/drivers/power/domain/imx8m-power-domain.c
>> @@ -506,8 +506,12 @@ static int imx8m_power_domain_bind(struct udevice *dev)
>> static int imx8m_power_domain_probe(struct udevice *dev)
>> {
>> struct imx8m_power_domain_plat *pdata = dev_get_plat(dev);
>> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
>> int ret;
>>
>> + /* Every subdomain has its own device node */
>> + plat->subdomains = 1;
>> +
>> /* Nothing to do for non-"power-domain" driver instances. */
>> if (!strstr(dev->name, "power-domain"))
>> return 0;
>> diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
>> index 455ad53ef525e18ae45068fa5d8c2be8a1b79335..1ca43880ef56a03ae10e70f7e260022acc447dbb 100644
>> --- a/drivers/power/domain/imx8mp-hsiomix.c
>> +++ b/drivers/power/domain/imx8mp-hsiomix.c
>> @@ -201,8 +201,12 @@ int imx8mp_hsiomix_bind(struct udevice *dev)
>> static int imx8mp_hsiomix_probe(struct udevice *dev)
>> {
>> struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
>> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
>> int ret;
>>
>> + /* Definitions are in imx8mp-power.h */
>> + plat->subdomains = 5;
Enabling LOG_DEBUG on drivers/power/domain/power-domain-uclass.c may
help further
Looks like setting of plat->subdomains needs updation? PD of HSIOMIX is
17 (plat->subdomains is 5?)
Looking at the numerous imx* devicetrees; plat->subdomains looks to be
very different across each platform so hardcoding it in the probe of the
power driver may not be a good idea I think?
>> +
>> priv->base = dev_read_addr_ptr(dev);
>>
>> ret = clk_get_by_name(dev, "usb", &priv->clk_usb);
>> diff --git a/drivers/power/domain/imx8mp-mediamix.c b/drivers/power/domain/imx8mp-mediamix.c
>> index 78c32ca3d3a87febdefd5d128d39d817674b8d32..504c22f7d3631363d76eb21e43afd854258d4ea5 100644
>> --- a/drivers/power/domain/imx8mp-mediamix.c
>> +++ b/drivers/power/domain/imx8mp-mediamix.c
>> @@ -143,9 +143,13 @@ static int imx8mp_mediamix_bind(struct udevice *dev)
>>
>> static int imx8mp_mediamix_probe(struct udevice *dev)
>> {
>> + struct power_domain_plat *plat = dev_get_uclass_plat(dev);
>> struct imx8mp_mediamix_priv *priv = dev_get_priv(dev);
>> int ret;
>>
>> + /* Definitions are in imx8mp-power.h */
>> + plat->subdomains = 9;
>> +
>> priv->base = dev_read_addr_ptr(dev);
>>
>> ret = clk_get_by_name(dev, "apb", &priv->clk_apb);
>>
>
--
Thanking You
Neha Malcom Francis
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp
2025-07-21 16:01 ` Frieder Schrempf
2025-07-22 4:43 ` Neha Malcom Francis
@ 2025-07-22 7:26 ` Frieder Schrempf
2025-07-22 8:32 ` Frieder Schrempf
1 sibling, 1 reply; 13+ messages in thread
From: Frieder Schrempf @ 2025-07-22 7:26 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Heiko Schocher, Fabio Estevam
Am 21.07.25 um 18:01 schrieb Frieder Schrempf:
> Hi Miquel,
>
> Am 25.04.25 um 08:49 schrieb Miquel Raynal:
>> Prevent enabling/disabling multiple times the same power domain to avoid
>> breakages due to the same power domains being referenced several times
>> by different device nodes.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> I've stumbled upon some issue that seems to be related to this patch. I
> saw you already had to bother with other breakages of existing
> boards/platforms in the previous implementation. Unfortunately it looks
> like this still causes some kind of regression in my case.
>
> I'm working with an i.MX8MM board (kontron-sl-mx8mm_defconfig) and I see
> a crash as soon as I do "usb start" and then "usb stop" (log for the
> latter see below).
>
> As soon as I revert this patch everything starts to work fine again. Do
> you have any suggestions? The power-domains involved here are pgc_otg1
> and pgc_hsiomix defined in imx8mm.dtsi.
>
> Thanks
> Frieder
>
>
> => usb stop
> stopping USB..
> Sending event 6/(unknown) to spy 'efi_disk del'
> Sending event 6/(unknown) to spy 'efi_disk del'
> Looking for power-domain@0
> Looking for power-domain@0
> - checking gpc@303a0000
> - checking power-domain@0
> - result for power-domain@0: power-domain@0 (ret=0)
> - result for power-domain@0: power-domain@0 (ret=0)
> "Synchronous Abort" handler, esr 0x96000005, far 0x4ebacdc50
> elr: 0000000040249c94 lr : 0000000040249c6c (reloc)
> elr: 00000000fff4ec94 lr : 00000000fff4ec6c
> x0 : 0000000000000001 x1 : 00000000fbf22750
> x2 : 00000003efbab500 x3 : 00000000fffcdfb0
> x4 : 0000000000000020 x5 : 0000000000000020
> x6 : 00000000fbeea10f x7 : 00000000fbeea3d7
> x8 : 00000000fffffffe x9 : 00000000fbeea1ec
> x10: 00000000fbeea280 x11: 00000000ffffffd8
> x12: 00000000fbeeb1b0 x13: 00000000fbeeb1b0
> x14: 0000000000000002 x15: 0000000000000000
> x16: 00000000fff4edc4 x17: 0000000000000000
> x18: 00000000fbef4db0 x19: 00000004ebacdc50
> x20: 00000000fbeeace8 x21: 00000000fbf21ad0
> x22: 0000000000000000 x23: 0000000000000001
> x24: 0000000000000000 x25: 00000000fbf28f10
> x26: 0000000000000000 x27: 00000000fbf2ad40
> x28: 00000000fbf2ada0 x29: 00000000fbeeac70
>
> Code: f9400693 d37ef662 ab130833 54000140 (b8626820)
> Resetting CPU ...
>
>
FWIW, here is another log with only the debug output from
power-domain-uclass.c:
=> usb start
starting USB...
power_domain_get_by_index(dev=00000000fbefacd0,
power_domain=00000000fbeead28)
power_domain_get_by_index(dev=00000000fbef7450,
power_domain=00000000fbef74f0)
power_domain_get_by_index: dev_read_phandle_with_args failed: -2
power_domain_get_by_index(dev=00000000fbef7560,
power_domain=00000000fbef7600)
power_domain_get_by_index: dev_read_phandle_with_args failed: -2
Enable power domain power-domain@0.0: 0 -> 1 (todo)
power_domain_get_by_index(dev=00000000fbef58b0,
power_domain=00000000fbeeabf8)
power_domain_get_by_index(dev=00000000fbef7780,
power_domain=00000000fbef7820)
power_domain_get_by_index: dev_read_phandle_with_args failed: -2
Enable power domain power-domain@2.0: 0 -> 1 (todo)
power_domain_get_by_index(dev=00000000fbf239d0,
power_domain=00000000fbeea288)
Enable power domain power-domain@0.0: 1 -> 2 (EALREADY)
Bus usb@32e40000: 1 USB Device(s) found
=> <INTERRUPT>
=> usb stop
stopping USB..
power_domain_get_by_index(dev=00000000fbf239d0,
power_domain=00000000fbeeac88)
"Synchronous Abort" handler, esr 0x96000005, far 0x4ebaceb90
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp
2025-07-22 7:26 ` Frieder Schrempf
@ 2025-07-22 8:32 ` Frieder Schrempf
0 siblings, 0 replies; 13+ messages in thread
From: Frieder Schrempf @ 2025-07-22 8:32 UTC (permalink / raw)
To: Miquel Raynal, Simon Glass, Tom Rini, Jaehoon Chung
Cc: Thomas Petazzoni, u-boot, Samuel Holland, Neha Malcom Francis,
Wadim Egorov, Heiko Schocher, Fabio Estevam
Am 22.07.25 um 09:26 schrieb Frieder Schrempf:
> Am 21.07.25 um 18:01 schrieb Frieder Schrempf:
>> Hi Miquel,
>>
>> Am 25.04.25 um 08:49 schrieb Miquel Raynal:
>>> Prevent enabling/disabling multiple times the same power domain to avoid
>>> breakages due to the same power domains being referenced several times
>>> by different device nodes.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> I've stumbled upon some issue that seems to be related to this patch. I
>> saw you already had to bother with other breakages of existing
>> boards/platforms in the previous implementation. Unfortunately it looks
>> like this still causes some kind of regression in my case.
>>
>> I'm working with an i.MX8MM board (kontron-sl-mx8mm_defconfig) and I see
>> a crash as soon as I do "usb start" and then "usb stop" (log for the
>> latter see below).
>>
>> As soon as I revert this patch everything starts to work fine again. Do
>> you have any suggestions? The power-domains involved here are pgc_otg1
>> and pgc_hsiomix defined in imx8mm.dtsi.
>>
>> Thanks
>> Frieder
>>
>>
>> => usb stop
>> stopping USB..
>> Sending event 6/(unknown) to spy 'efi_disk del'
>> Sending event 6/(unknown) to spy 'efi_disk del'
>> Looking for power-domain@0
>> Looking for power-domain@0
>> - checking gpc@303a0000
>> - checking power-domain@0
>> - result for power-domain@0: power-domain@0 (ret=0)
>> - result for power-domain@0: power-domain@0 (ret=0)
>> "Synchronous Abort" handler, esr 0x96000005, far 0x4ebacdc50
>> elr: 0000000040249c94 lr : 0000000040249c6c (reloc)
>> elr: 00000000fff4ec94 lr : 00000000fff4ec6c
>> x0 : 0000000000000001 x1 : 00000000fbf22750
>> x2 : 00000003efbab500 x3 : 00000000fffcdfb0
>> x4 : 0000000000000020 x5 : 0000000000000020
>> x6 : 00000000fbeea10f x7 : 00000000fbeea3d7
>> x8 : 00000000fffffffe x9 : 00000000fbeea1ec
>> x10: 00000000fbeea280 x11: 00000000ffffffd8
>> x12: 00000000fbeeb1b0 x13: 00000000fbeeb1b0
>> x14: 0000000000000002 x15: 0000000000000000
>> x16: 00000000fff4edc4 x17: 0000000000000000
>> x18: 00000000fbef4db0 x19: 00000004ebacdc50
>> x20: 00000000fbeeace8 x21: 00000000fbf21ad0
>> x22: 0000000000000000 x23: 0000000000000001
>> x24: 0000000000000000 x25: 00000000fbf28f10
>> x26: 0000000000000000 x27: 00000000fbf2ad40
>> x28: 00000000fbf2ada0 x29: 00000000fbeeac70
>>
>> Code: f9400693 d37ef662 ab130833 54000140 (b8626820)
>> Resetting CPU ...
>>
>>
>
> FWIW, here is another log with only the debug output from
> power-domain-uclass.c:
>
> => usb start
> starting USB...
> power_domain_get_by_index(dev=00000000fbefacd0,
> power_domain=00000000fbeead28)
> power_domain_get_by_index(dev=00000000fbef7450,
> power_domain=00000000fbef74f0)
> power_domain_get_by_index: dev_read_phandle_with_args failed: -2
> power_domain_get_by_index(dev=00000000fbef7560,
> power_domain=00000000fbef7600)
> power_domain_get_by_index: dev_read_phandle_with_args failed: -2
> Enable power domain power-domain@0.0: 0 -> 1 (todo)
> power_domain_get_by_index(dev=00000000fbef58b0,
> power_domain=00000000fbeeabf8)
> power_domain_get_by_index(dev=00000000fbef7780,
> power_domain=00000000fbef7820)
> power_domain_get_by_index: dev_read_phandle_with_args failed: -2
> Enable power domain power-domain@2.0: 0 -> 1 (todo)
> power_domain_get_by_index(dev=00000000fbf239d0,
> power_domain=00000000fbeea288)
> Enable power domain power-domain@0.0: 1 -> 2 (EALREADY)
> Bus usb@32e40000: 1 USB Device(s) found
> => <INTERRUPT>
> => usb stop
> stopping USB..
> power_domain_get_by_index(dev=00000000fbf239d0,
> power_domain=00000000fbeeac88)
> "Synchronous Abort" handler, esr 0x96000005, far 0x4ebaceb90
This should do the trick:
https://patchwork.ozlabs.org/project/uboot/patch/20250722083119.64084-1-frieder@fris.de/
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-07-22 8:33 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-25 6:49 [PATCH 0/2] power-domain: Enable uclass refcounting Miquel Raynal
2025-04-25 6:49 ` [PATCH 1/2] power-domain: Add support for refcounting (again) Miquel Raynal
2025-04-25 8:57 ` Wadim Egorov
2025-04-25 9:49 ` Neha Malcom Francis
2025-04-25 11:12 ` Heiko Schocher
2025-04-25 6:49 ` [PATCH 2/2] imx: power-domain: Enable refcounting on imx8mp Miquel Raynal
2025-04-25 11:13 ` Heiko Schocher
2025-07-21 16:01 ` Frieder Schrempf
2025-07-22 4:43 ` Neha Malcom Francis
2025-07-22 7:26 ` Frieder Schrempf
2025-07-22 8:32 ` Frieder Schrempf
2025-04-25 8:56 ` [PATCH 0/2] power-domain: Enable uclass refcounting Wadim Egorov
2025-04-28 16:11 ` Fabio Estevam
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.