From: Wadim Egorov <w.egorov@phytec.de>
To: Miquel Raynal <miquel.raynal@bootlin.com>,
Simon Glass <sjg@chromium.org>, Tom Rini <trini@konsulko.com>,
Jaehoon Chung <jh80.chung@samsung.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
u-boot@lists.denx.de, Samuel Holland <samuel.holland@sifive.com>,
Neha Malcom Francis <n-francis@ti.com>,
Heiko Schocher <hs@denx.de>, Fabio Estevam <festevam@denx.de>
Subject: Re: [PATCH 1/2] power-domain: Add support for refcounting (again)
Date: Fri, 25 Apr 2025 11:57:12 +0300 [thread overview]
Message-ID: <f66ee984-8140-4f36-bd08-4c71e8a44256@phytec.de> (raw)
In-Reply-To: <20250425-ge-ian-display-support-v1-1-8a7a0fe75023@bootlin.com>
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));
>
>
next prev parent reply other threads:[~2025-04-25 8:57 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f66ee984-8140-4f36-bd08-4c71e8a44256@phytec.de \
--to=w.egorov@phytec.de \
--cc=festevam@denx.de \
--cc=hs@denx.de \
--cc=jh80.chung@samsung.com \
--cc=miquel.raynal@bootlin.com \
--cc=n-francis@ti.com \
--cc=samuel.holland@sifive.com \
--cc=sjg@chromium.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.