On Wed, 13 Aug 2025 at 13:58, Geert Uytterhoeven [off-list ref] wrote:

Hi Ulf,

On Tue, 12 Aug 2025 at 12:01, Ulf Hansson [off-list ref] wrote:

quoted

On Thu, 7 Aug 2025 at 11:38, Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, 30 Jul 2025 at 12:29, Ulf Hansson [off-list ref] wrote:

quoted

On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, 9 Jul 2025 at 13:31, Ulf Hansson [off-list ref] wrote:

quoted

On Tue, 1 Jul 2025 at 13:47, Ulf Hansson [off-list ref] wrote:

quoted

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.

OK, just like I already assumed...

quoted

quoted

quoted

quoted

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.

I was under the impression there is a time-out, after which the
.sync_state() callback would be called anyway, just like for probe
deferral due to missing optional providers like DMACs and IOMMUs.
Apparently that is not the case?

The behaviour is configurable, so it depends. The current default
behaviour does *not* enforce the ->sync_state() callbacks to be
called, even after a time-out.

You may set CONFIG_FW_DEVLINK_SYNC_STATE_TIMEOUT to achieve the above
behavior or use the fw_devlink command line parameters to change it.
Like setting "fw_devlink.sync_state=timeout".

I guess it can be debated what the default behaviour should be.
Perhaps we should even allow the default behaviour to be dynamically
tweaked on a per provider device/driver basis?

quoted

quoted

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.

Indeed.

quoted

In other words, the ->sync_state() callbacks are called even if the
corresponding consumer devices have not been probed yet.

Hence shouldn't there be a timeout, as the kernel may not even have
a driver for one or more consumer devices?

See above.

quoted

quoted

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 <redacted>
Date: Tue, 29 Jul 2025 14:27:22 +0200
Subject: [PATCH] pmdomain: core: Allow powered-on PM domains to be powered-off
 during boot

[...]

I gave this a try (i.e. "| GENPD_FLAG_NO_STAY_ON" in rcar-sysc.c), but
this doesn't make any difference.  I assume this would only work when
actively calling genpd_power_off() (i.e. not from of_genpd_sync_state()
or genpd_provider_sync_state())?

Right. Thanks for testing!

So, we may need to restore some part of the genpd_power_off_unused()
when CONFIG_PM_GENERIC_DOMAINS_OF is set. Without clearing
"genpd->stay_on".

I can extend the patch, if you think it would make sense for you?

Kind regards
Uffe