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

Tomi Valkeinen
2025-04-24T10:59:46Z

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