Message-ID: <8e748129-3348-4bf1-9fc8-fadc569fa48e@ideasonboard.com>
Date: Thu, 24 Apr 2025 13:59:40 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Ulf Hansson <ulf.hansson@...aro.org>,
 Saravana Kannan <saravanak@...gle.com>, Stephen Boyd <sboyd@...nel.org>,
 linux-pm@...r.kernel.org
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Michael Grzeschik <m.grzeschik@...gutronix.de>,
 Bjorn Andersson <andersson@...nel.org>, Abel Vesa <abel.vesa@...aro.org>,
 Devarsh Thakkar <devarsht@...v0571a.ent.ti.com>,
 Peng Fan <peng.fan@....nxp.com>, Johan Hovold <johan@...nel.org>,
 Maulik Shah <maulik.shah@....qualcomm.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Kevin Hilman <khilman@...libre.com>, Devarsh Thakkar <devarsht@...com>
Subject: Re: [PATCH 00/11] pmdomain: Add generic ->sync_state() support to
 genpd

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@...00/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

Powered by blists - more mailing lists