On Thu, 7 Aug 2025 at 11:38, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Ulf,
>
> On Wed, 30 Jul 2025 at 12:29, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > > On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > > > Changes in v3:
> > > > > - Added a couple of patches to adress problems on some Renesas
> > > > > platforms. Thanks Geert and Tomi for helping out!
> > > > > - Adressed a few comments from Saravanna and Konrad.
> > > > > - Added some tested-by tags.
> > > >
> > > > I decided it was time to give this a try, so I have queued this up for
> > > > v6.17 via the next branch at my pmdomain tree.
> > > >
> > > > If you encounter any issues, please let me know so I can help to fix them.
> > >
> > > Thanks for your series! Due to holidays, I only managed to test
> > > this very recently.
> > >
> > > Unfortunately I have an issue with unused PM Domains no longer being
> > > disabled on R-Car:
> > > - On R-Car Gen1/2/3, using rcar-sysc.c, unused PM Domains are never
> > > disabled.
> > > - On R-Car Gen4, using rcar-gen4-sysc.c, unused PM Domains are
> > > sometimes not disabled.
> > > At first, I noticed the IOMMU driver was not enabled in my config,
> > > and enabling it did fix the issue. However, after that I still
> > > encountered the issue in a different config that does have the
> > > IOMMU driver enabled...
> > >
> > > FTR, unused PM Domains are still disabled correctly on R/SH-Mobile
> > > (using rmobile-sysc.c) and on BeagleBone Black. Note that these use
> > > of_genpd_add_provider_simple(), while all R-Car drivers use
> > > of_genpd_add_provider_onecell(). Perhaps there is an issue with
> > > the latter? If you don't have a clue, I plan to do some more
> > > investigation later...
>
> of_genpd_add_provider_onecell() has:
>
> if (!dev)
> sync_state = true;
> else
> dev_set_drv_sync_state(dev, genpd_sync_state);
>
> for (i = 0; i < data->num_domains; i++) {
> ...
> if (sync_state && !genpd_is_no_sync_state(genpd)) {
> genpd->sync_state = GENPD_SYNC_STATE_ONECELL;
> device_set_node(&genpd->dev, fwnode);
> sync_state = false;
> ^^^^^^^^^^^^^^^^^^^
> }
> ...
> }
>
> As the R-Car SYSC drivers are not platform drivers, dev is NULL, and
> genpd->sync_state is set to GENPD_SYNC_STATE_ONECELL for the first PM
> Domain only. All other domains have the default value of sync_state
> (0 = GENPD_SYNC_STATE_OFF). Hence when genpd_provider_sync_state()
> is called later, it ignores all but the first domain.
> Apparently this is intentional, as of_genpd_sync_state() tries to
> power off all domains handled by the same controller anyway (see below)?
Right, this is intentional and mainly because of how fw_devlink works.
fw_devlink is limited to use only the first device - if multiple
devices share the same fwnode. In principle, we could have picked any
of the devices in the array of genpds here - and reached the same
result.
>
> > > BTW, the "pending due to"-messages look weird to me.
> > > On R-Car M2-W (r8a7791.dtsi) I see e.g.:
> > >
> > > genpd_provider ca15-cpu0: sync_state() pending due to e6020000.watchdog
> > > renesas-cpg-mssr e6150000.clock-controller: sync_state() pending
> > > due to e6020000.watchdog
> > >
> > > ca15-cpu0 is the PM Domain holding the first CPU core, while
> > > the watchdog resides in the always-on Clock Domain, and uses the
> > > clock-controller for PM_CLK handling.
>
> Unfortunately the first PM Domain is "ca15-cpu0", which is blocked on
> these bogus pending states, and no PM Domain is powered off.
I see, thanks for the details. I am looking closer at this.
In any case, this is the main issue, as it prevents the ->sync_state()
callback to be called. Hence the "genpd->stay_on" will also *not* be
cleared for any of the genpd's for the genpd-provider.
>
> If I remove the "sync_state = false" above, genpd_provider_sync_state()
> considers all domains, and does power down all unused domains (even
> multiple times, as expected).
I think those are getting called because with the change above, there
is no device_link being tracked. As stated above, fw_devlink is
limited to use only one device - if multiple devices share the same
fwnode.
In other words, the ->sync_state() callbacks are called even if the
corresponding consumer devices have not been probed yet.
>
> Upon closer look, all "pending due to" messages I see claim that the
> first (index 0) PM Domain is pending on some devices, while all of
> these devices are part of a different domain (usually the always-on
> domain, which is always the last (32 or 64) on R-Car).
>
> So I think there are two issues:
> 1. Devices are not attributed to the correct PM Domain using
> fw_devlink sync_state,
> 2. One PM Domain of a multi-domain controller being blocked should
> not block all other domains handled by the same controller.
Right, that's a current limitation with fw_devlink. To cope with this,
it's possible to enforce the ->sync_state() callback to be invoked
from user-space (timeout or explicitly) for a device.
Another option would be to allow an opt-out behavior for some genpd's
that are powered-on at initialization. Something along the lines of
the below.
From: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
Date: Tue, 29 Jul 2025 14:27:22 +0200
Subject: [PATCH] pmdomain: core: Allow powered-on PM domains to be powered-off
during boot
Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
---
drivers/pmdomain/core.c | 3 ++-
include/linux/pm_domain.h | 7 +++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 0006ab3d0789..ef0760824c92 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -187,6 +187,7 @@ static const struct genpd_lock_ops genpd_raw_spin_ops = {
#define genpd_is_opp_table_fw(genpd) (genpd->flags & GENPD_FLAG_OPP_TABLE_FW)
#define genpd_is_dev_name_fw(genpd) (genpd->flags & GENPD_FLAG_DEV_NAME_FW)
#define genpd_is_no_sync_state(genpd) (genpd->flags &
GENPD_FLAG_NO_SYNC_STATE)
+#define genpd_is_no_stay_on(genpd) (genpd->flags & GENPD_FLAG_NO_STAY_ON)
static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
const struct generic_pm_domain *genpd)
@@ -2392,7 +2393,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
atomic_set(&genpd->sd_count, 0);
genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
- genpd->stay_on = !is_off;
+ genpd->stay_on = !genpd_is_no_stay_on(genpd) && !is_off;
genpd->sync_state = GENPD_SYNC_STATE_OFF;
genpd->device_count = 0;
genpd->provider = NULL;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b9d3c7d5c4f8..61b81574efc4 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -109,6 +109,12 @@ struct dev_pm_domain_list {
* genpd provider specific way, likely through a
* parent device node. This flag makes genpd to
* skip its internal support for this.
+ *
+ * GENPD_FLAG_NO_STAY_ON: A powered-on PM domain at initialization is
+ * prevented by genpd from being powered-off until
+ * we receive a ->sync_state() or runs the
+ * late_initcall_sync. Use this flag to allow
+ * power-off without waiting for these conditions.
*/
#define GENPD_FLAG_PM_CLK (1U << 0)
#define GENPD_FLAG_IRQ_SAFE (1U << 1)
@@ -120,6 +126,7 @@ struct dev_pm_domain_list {
#define GENPD_FLAG_OPP_TABLE_FW (1U << 7)
#define GENPD_FLAG_DEV_NAME_FW (1U << 8)
#define GENPD_FLAG_NO_SYNC_STATE (1U << 9)
+#define GENPD_FLAG_NO_STAY_ON (1U << 10)
enum gpd_status {
GENPD_STATE_ON = 0, /* PM domain is on */
--
2.43.0
Kind regards
Uffe
