If a PM domain (genpd) is powered-on during boot, there is probably a good
reason for it. Therefore it's known to be a bad idea to allow such genpd to be
powered-off before all of its consumer devices have been probed. This series
intends to fix this problem.

We have been discussing these issues at LKML and at various Linux-conferences
in the past. I have therefore tried to include the people I can recall being
involved, but I may have forgotten some (my apologies), feel free to loop them
in.

A few notes:
*)
Even if this looks good, the last patch can't go in without some additional
changes to a couple of existing genpd provider drivers. Typically genpd provider
drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
will fix this asap, if we think the series makes sense.

*)
Patch 1 -> 3 are just preparatory cleanups. 

*)
I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
Let me know if you want me to share this code too.


Please help review and test!
Finally, a big thanks to Saravana for all the support!

Kind regards
Ulf Hansson


Saravana Kannan (1):
  driver core: Add dev_set_drv_sync_state()

Ulf Hansson (10):
  pmdomain: core: Convert genpd_power_off() to void
  pmdomain: core: Simplify return statement in genpd_power_off()
  pmdomain: core: Use genpd->opp_table to simplify error/remove path
  pmdomain: core: Add a bus and a driver for genpd providers
  pmdomain: core: Use device_set_node() to assign the fwnode too
  pmdomain: core: Add the genpd->dev to the genpd provider bus
  pmdomain: core: Export a common ->sync_state() helper for genpd
    providers
  pmdomain: core: Add internal ->sync_state() support for genpd
    providers
  pmdomain: core: Default to use of_genpd_sync_state() for genpd
    providers
  pmdomain: core: Leave powered-on genpds on until ->sync_state()

 drivers/pmdomain/core.c   | 273 +++++++++++++++++++++++++++++++-------
 include/linux/device.h    |  12 ++
 include/linux/pm_domain.h |  11 ++
 3 files changed, 249 insertions(+), 47 deletions(-)

-- 
2.43.0

On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> If a PM domain (genpd) is powered-on during boot, there is probably a good
> reason for it. Therefore it's known to be a bad idea to allow such genpd to be
> powered-off before all of its consumer devices have been probed. This series
> intends to fix this problem.
>
> We have been discussing these issues at LKML and at various Linux-conferences
> in the past. I have therefore tried to include the people I can recall being
> involved, but I may have forgotten some (my apologies), feel free to loop them
> in.
>
> A few notes:
> *)
> Even if this looks good, the last patch can't go in without some additional
> changes to a couple of existing genpd provider drivers. Typically genpd provider
> drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
> will fix this asap, if we think the series makes sense.
>
> *)
> Patch 1 -> 3 are just preparatory cleanups.
>
> *)
> I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
> Let me know if you want me to share this code too.
>
>
> Please help review and test!
> Finally, a big thanks to Saravana for all the support!

You are welcome! Thank you for getting this series done! Happy to see
sync_state() support being added to another framework.

I skimmed the series and at a high level it looks right. Not too
familiar with power domain code, so I didn't go deep. Just reviewed
the fw_devlink and driver core parts of it.

-Saravana

>
> Kind regards
> Ulf Hansson
>
>
> Saravana Kannan (1):
>   driver core: Add dev_set_drv_sync_state()
>
> Ulf Hansson (10):
>   pmdomain: core: Convert genpd_power_off() to void
>   pmdomain: core: Simplify return statement in genpd_power_off()
>   pmdomain: core: Use genpd->opp_table to simplify error/remove path
>   pmdomain: core: Add a bus and a driver for genpd providers
>   pmdomain: core: Use device_set_node() to assign the fwnode too
>   pmdomain: core: Add the genpd->dev to the genpd provider bus
>   pmdomain: core: Export a common ->sync_state() helper for genpd
>     providers
>   pmdomain: core: Add internal ->sync_state() support for genpd
>     providers
>   pmdomain: core: Default to use of_genpd_sync_state() for genpd
>     providers
>   pmdomain: core: Leave powered-on genpds on until ->sync_state()
>
>  drivers/pmdomain/core.c   | 273 +++++++++++++++++++++++++++++++-------
>  include/linux/device.h    |  12 ++
>  include/linux/pm_domain.h |  11 ++
>  3 files changed, 249 insertions(+), 47 deletions(-)
>
> --
> 2.43.0
>

Hi,

On 17/04/2025 17:24, Ulf Hansson wrote:
> If a PM domain (genpd) is powered-on during boot, there is probably a good
> reason for it. Therefore it's known to be a bad idea to allow such genpd to be
> powered-off before all of its consumer devices have been probed. This series
> intends to fix this problem.
> 
> We have been discussing these issues at LKML and at various Linux-conferences
> in the past. I have therefore tried to include the people I can recall being
> involved, but I may have forgotten some (my apologies), feel free to loop them
> in.
> 
> A few notes:
> *)
> Even if this looks good, the last patch can't go in without some additional
> changes to a couple of existing genpd provider drivers. Typically genpd provider
> drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
> will fix this asap, if we think the series makes sense.
> 
> *)
> Patch 1 -> 3 are just preparatory cleanups.
> 
> *)
> I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
> Let me know if you want me to share this code too.
> 
> 
> Please help review and test!
> Finally, a big thanks to Saravana for all the support!

I had a quick test with this on TI's AM62 board. A few observations.

With this series, all the individual PDs seem to get a state_synced file:

...
/sys/devices/genpd_provider/pd:143/state_synced
/sys/devices/genpd_provider/pd:54/state_synced
/sys/devices/genpd_provider/pd:105/state_synced
/sys/devices/genpd_provider/pd:62/state_synced
/sys/devices/genpd_provider/pd:141/state_synced
...

Is that on purpose? What do these files represent? They all seem to be "1".

When I boot up, I see the sync_state pending:

[   22.541292] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
2b10000.audio-contro
ller
[   22.554839] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e0f0000.watchdog
[   22.566550] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e030000.watchdog
[   22.577854] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e020000.watchdog
[   22.589239] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e010000.watchdog
[   22.600674] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
e000000.watchdog
[   22.611875] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
30200000.dss
[   22.622813] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
fd00000.gpu
[   22.633565] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
b00000.temperature-s
ensor
[   22.645540] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
2b300050.target-modu
le
[   22.657067] ti_sci_pm_domains 
44043000.system-controller:power-controller: sync_state() pending due to 
chosen:framebuffer@0

The "real" state_synced file on this platform is:

/sys/devices/platform/bus@f0000/44043000.system-controller/44043000.system-controller:power-controller/state_synced

In strict mode, this shows 0, and if I echo 1 (interestingly "echo 1 > 
/sys/..." doesn't work, I need "echo -n 1 > /sys/...), I see PDs getting 
powered off (added a debug print there):

[   87.335487] ti_sci_pd_power_off 88
[   87.342896] ti_sci_pd_power_off 87
[   87.347404] ti_sci_pd_power_off 86
[   87.356464] ti_sci_pd_power_off 128
[   87.361296] ti_sci_pd_power_off 127
[   87.368714] ti_sci_pd_power_off 126
[   87.373349] ti_sci_pd_power_off 125
[   87.378077] ti_sci_pd_power_off 62
[   87.382587] ti_sci_pd_power_off 60
[   87.387194] ti_sci_pd_power_off 59
[   87.391759] ti_sci_pd_power_off 53
[   87.396648] ti_sci_pd_power_off 52
[   87.400801] ti_sci_pd_power_off 51
[   87.405131] ti_sci_pd_power_off 75
[   87.409238] ti_sci_pd_power_off 143
[   87.413328] ti_sci_pd_power_off 142
[   87.417403] ti_sci_pd_power_off 141
[   87.421494] ti_sci_pd_power_off 105
[   87.425632] ti_sci_pd_power_off 104
[   87.429815] ti_sci_pd_power_off 103
[   87.433941] ti_sci_pd_power_off 102
[   87.438054] ti_sci_pd_power_off 158
[   87.442151] ti_sci_pd_power_off 156
[   87.446324] ti_sci_pd_power_off 155
[   87.450463] ti_sci_pd_power_off 154
[   87.454549] ti_sci_pd_power_off 153
[   87.458671] ti_sci_pd_power_off 152
[   87.462571] ti_sci_pd_power_off 43
[   87.466425] ti_sci_pd_power_off 42
[   87.470254] ti_sci_pd_power_off 41
[   87.474032] ti_sci_pd_power_off 40
[   87.477825] ti_sci_pd_power_off 39
[   87.481609] ti_sci_pd_power_off 38
[   87.485432] ti_sci_pd_power_off 37
[   87.489256] ti_sci_pd_power_off 36
[   87.493077] ti_sci_pd_power_off 95
[   87.496845] ti_sci_pd_power_off 132
[   87.500780] ti_sci_pd_power_off 107
[   87.504583] ti_sci_pd_power_off 114
[   87.508429] ti_sci_pd_power_off 79
[   87.512050] ti_sci_pd_power_off 148
[   87.515859] ti_sci_pd_power_off 147
[   87.519644] ti_sci_pd_power_off 106
[   87.523414] ti_sci_pd_power_off 149
[   87.527203] ti_sci_pd_power_off 50
[   87.530971] ti_sci_pd_power_off 49
[   87.534708] ti_sci_pd_power_off 48
[   87.538401] ti_sci_pd_power_off 35
[   87.542040] ti_sci_pd_power_off 186

We do have a lot of "extra" PDs enabled by the bootloader...

With the timeout mode, I see the sync_state() getting called some 
seconds after the boot has finished.

So... I think it all works as expected. You can take this as some kind 
of Tested-by, but it'd be good if someone from TI who knows more about 
PDs would test this too =).

Interestingly, I see a difference in behavior to the old patches from 
Abel: with the old patches, if I boot up with the DSS (display 
subsystem) enabled by the bootloader, it looks the same as with these 
patches. However, with the old patches, when I load the DSS driver, and 
it probes successfully, the DSS PD will get managed correctly, i.e. if I 
blank the screen, the DSS PD will go to off, even if the sync_state has 
not been called.

With these patches the DSS PD will stay on, no matter if I load the DSS 
driver or not, and will only go off after sync_state has been called.

I'm not quite sure here, but I think the behavior with the old patches 
makes sense: when the driver for a particular PD loads, the PD no longer 
needs to be kept on. Or... Is this about the case where a PD has 
multiple consumers? The PD provider cannot know how many consumers there 
are for a single PD, so it must keep all boot-time-enabled PDs on until 
sync_state() (i.e. all the consumer drivers have probed)?

  Tomi

On Thu, 24 Apr 2025 at 12:59, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> On 17/04/2025 17:24, Ulf Hansson wrote:
> > If a PM domain (genpd) is powered-on during boot, there is probably a good
> > reason for it. Therefore it's known to be a bad idea to allow such genpd to be
> > powered-off before all of its consumer devices have been probed. This series
> > intends to fix this problem.
> >
> > We have been discussing these issues at LKML and at various Linux-conferences
> > in the past. I have therefore tried to include the people I can recall being
> > involved, but I may have forgotten some (my apologies), feel free to loop them
> > in.
> >
> > A few notes:
> > *)
> > Even if this looks good, the last patch can't go in without some additional
> > changes to a couple of existing genpd provider drivers. Typically genpd provider
> > drivers that implements ->sync_state() need to call of_genpd_sync_state(), but I
> > will fix this asap, if we think the series makes sense.
> >
> > *)
> > Patch 1 -> 3 are just preparatory cleanups.
> >
> > *)
> > I have tested this with QEMU with a bunch of local test-drivers and DT nodes.
> > Let me know if you want me to share this code too.
> >
> >
> > Please help review and test!
> > Finally, a big thanks to Saravana for all the support!
>
> I had a quick test with this on TI's AM62 board. A few observations.
>
> With this series, all the individual PDs seem to get a state_synced file:
>
> ...
> /sys/devices/genpd_provider/pd:143/state_synced
> /sys/devices/genpd_provider/pd:54/state_synced
> /sys/devices/genpd_provider/pd:105/state_synced
> /sys/devices/genpd_provider/pd:62/state_synced
> /sys/devices/genpd_provider/pd:141/state_synced
> ...
>
> Is that on purpose? What do these files represent? They all seem to be "1".

It's on purpose, but in this case there are no fw_devlink tracking
them, but instead that's done via..

/sys/devices/platform/bus@f0000/44043000.system-controller/44043000.system-controller:power-controller/state_synced

..as you point out below.

Depending on the DT layout these nodes may be useful, but not in the
TI PM domain case.

>
> When I boot up, I see the sync_state pending:
>
> [   22.541292] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> 2b10000.audio-contro
> ller
> [   22.554839] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e0f0000.watchdog
> [   22.566550] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e030000.watchdog
> [   22.577854] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e020000.watchdog
> [   22.589239] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e010000.watchdog
> [   22.600674] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> e000000.watchdog
> [   22.611875] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> 30200000.dss
> [   22.622813] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> fd00000.gpu
> [   22.633565] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> b00000.temperature-s
> ensor
> [   22.645540] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> 2b300050.target-modu
> le
> [   22.657067] ti_sci_pm_domains
> 44043000.system-controller:power-controller: sync_state() pending due to
> chosen:framebuffer@0
>
> The "real" state_synced file on this platform is:
>
> /sys/devices/platform/bus@f0000/44043000.system-controller/44043000.system-controller:power-controller/state_synced
>
> In strict mode, this shows 0, and if I echo 1 (interestingly "echo 1 >
> /sys/..." doesn't work, I need "echo -n 1 > /sys/...), I see PDs getting
> powered off (added a debug print there):
>
> [   87.335487] ti_sci_pd_power_off 88
> [   87.342896] ti_sci_pd_power_off 87
> [   87.347404] ti_sci_pd_power_off 86
> [   87.356464] ti_sci_pd_power_off 128
> [   87.361296] ti_sci_pd_power_off 127
> [   87.368714] ti_sci_pd_power_off 126
> [   87.373349] ti_sci_pd_power_off 125
> [   87.378077] ti_sci_pd_power_off 62
> [   87.382587] ti_sci_pd_power_off 60
> [   87.387194] ti_sci_pd_power_off 59
> [   87.391759] ti_sci_pd_power_off 53
> [   87.396648] ti_sci_pd_power_off 52
> [   87.400801] ti_sci_pd_power_off 51
> [   87.405131] ti_sci_pd_power_off 75
> [   87.409238] ti_sci_pd_power_off 143
> [   87.413328] ti_sci_pd_power_off 142
> [   87.417403] ti_sci_pd_power_off 141
> [   87.421494] ti_sci_pd_power_off 105
> [   87.425632] ti_sci_pd_power_off 104
> [   87.429815] ti_sci_pd_power_off 103
> [   87.433941] ti_sci_pd_power_off 102
> [   87.438054] ti_sci_pd_power_off 158
> [   87.442151] ti_sci_pd_power_off 156
> [   87.446324] ti_sci_pd_power_off 155
> [   87.450463] ti_sci_pd_power_off 154
> [   87.454549] ti_sci_pd_power_off 153
> [   87.458671] ti_sci_pd_power_off 152
> [   87.462571] ti_sci_pd_power_off 43
> [   87.466425] ti_sci_pd_power_off 42
> [   87.470254] ti_sci_pd_power_off 41
> [   87.474032] ti_sci_pd_power_off 40
> [   87.477825] ti_sci_pd_power_off 39
> [   87.481609] ti_sci_pd_power_off 38
> [   87.485432] ti_sci_pd_power_off 37
> [   87.489256] ti_sci_pd_power_off 36
> [   87.493077] ti_sci_pd_power_off 95
> [   87.496845] ti_sci_pd_power_off 132
> [   87.500780] ti_sci_pd_power_off 107
> [   87.504583] ti_sci_pd_power_off 114
> [   87.508429] ti_sci_pd_power_off 79
> [   87.512050] ti_sci_pd_power_off 148
> [   87.515859] ti_sci_pd_power_off 147
> [   87.519644] ti_sci_pd_power_off 106
> [   87.523414] ti_sci_pd_power_off 149
> [   87.527203] ti_sci_pd_power_off 50
> [   87.530971] ti_sci_pd_power_off 49
> [   87.534708] ti_sci_pd_power_off 48
> [   87.538401] ti_sci_pd_power_off 35
> [   87.542040] ti_sci_pd_power_off 186
>
> We do have a lot of "extra" PDs enabled by the bootloader...
>
> With the timeout mode, I see the sync_state() getting called some
> seconds after the boot has finished.
>
> So... I think it all works as expected. You can take this as some kind
> of Tested-by, but it'd be good if someone from TI who knows more about
> PDs would test this too =).

Thanks a lot for testing and sharing your information!

>
> Interestingly, I see a difference in behavior to the old patches from
> Abel: with the old patches, if I boot up with the DSS (display
> subsystem) enabled by the bootloader, it looks the same as with these
> patches. However, with the old patches, when I load the DSS driver, and
> it probes successfully, the DSS PD will get managed correctly, i.e. if I
> blank the screen, the DSS PD will go to off, even if the sync_state has
> not been called.
>
> With these patches the DSS PD will stay on, no matter if I load the DSS
> driver or not, and will only go off after sync_state has been called.
>
> I'm not quite sure here, but I think the behavior with the old patches
> makes sense: when the driver for a particular PD loads, the PD no longer
> needs to be kept on. Or... Is this about the case where a PD has
> multiple consumers? The PD provider cannot know how many consumers there
> are for a single PD, so it must keep all boot-time-enabled PDs on until
> sync_state() (i.e. all the consumer drivers have probed)?

You are correct!

ti_sci_pm_domains are modelled in DT by using:
#power-domain-cells = <1>;
or
#power-domain-cells = <2>;

fw_devlink doesn't look at those additional specifiers in DT. For
example, if a consumer has "power-domains = <&k2g_pds 5>;" the '5'
will not be considered as a separate domain, but instead all consumers
of &k2g_pds needs to be probed, before the ->sync_state() gets called.

Theoretically, if we could treat the specifier ('5' in this case) as
being a separate domain, that should would for most cases. The
question is, how difficult it would be to extend fw_devlink to support
this, so that when all consumers that has  "power-domains = <&k2g_pds
5>" has probed, the ->sync_state() get's invoked for the corresponding
genpd->dev.

If Saravanna want to comment on this, that would be nice, otherwise I
will chat with him offlist about this.

That said, it seems like this is working fine for the TI platforms,
which is great!

Kind regards
Uffe