Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

[Geert Uytterhoeven]

Hi Ulf,

On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@linaro.org> 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...

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.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

[Ulf Hansson]

On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@linaro.org> 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...

Geert, thanks for reporting!

>
> 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.

I will have a closer look as soon as I can  to see if I can find some
potential problems.

Kind regards
Uffe

Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

[Geert Uytterhoeven]

Hi Ulf,

On Wed, 30 Jul 2025 at 12:29, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@linaro.org> 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)?

> > 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.

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).

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.

Does that make sense?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

[Ulf Hansson]

On Thu, 7 Aug 2025 at 11:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Wed, 30 Jul 2025 at 12:29, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org>
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@linaro.org>
---
 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

Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

[Geert Uytterhoeven]

Hi Ulf,

On Tue, 12 Aug 2025 at 12:01, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Thu, 7 Aug 2025 at 11:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, 30 Jul 2025 at 12:29, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@linaro.org> 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.

OK, just like I already assumed...

> > > > 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?

> > 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.

> 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?

> > 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@linaro.org>
> 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())?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 00/24] pmdomain: Add generic ->sync_state() support to genpd

[Ulf Hansson]

On Wed, 13 Aug 2025 at 13:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Tue, 12 Aug 2025 at 12:01, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Thu, 7 Aug 2025 at 11:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, 30 Jul 2025 at 12:29, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Wed, 30 Jul 2025 at 11:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, 9 Jul 2025 at 13:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > On Tue, 1 Jul 2025 at 13:47, Ulf Hansson <ulf.hansson@linaro.org> 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.
>
> OK, just like I already assumed...
>
> > > > > 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?

>
> > > 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.
>
> > 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.

>
> > > 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@linaro.org>
> > 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