Message-ID: <CAPDyKFqEgJPn-e-FooG_3h=Eqfw511c9_b+ywPcrfao8_p=u+Q@mail.gmail.com>
Date: Fri, 25 Apr 2025 14:17:01 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Saravana Kannan <saravanak@...gle.com>, Stephen Boyd <sboyd@...nel.org>, linux-pm@...r.kernel.org, 
	"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

On Thu, 24 Apr 2025 at 12:59, Tomi Valkeinen
<tomi.valkeinen@...asonboard.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@...00/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@...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 =).

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

Powered by blists - more mailing lists