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

Tue Sep 9 00:19:42 PDT 2025

Hi Ulf,

On Fri, 5 Sept 2025 at 13:09, Ulf Hansson <ulf.hansson at linaro.org> wrote:
> > > > > > > > > > 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?
> > > >
> > > > The domains are indeed powered off like before when passing
> > > > "fw_devlink.sync_state=timeout", so that fixes the regression.
> > > > But it was not needed before...
> > >
> > > Right, the default behavior in genpd has changed. The approach was
> > > taken as we believed that original behavior was not correct.
> > >
> > > Although, the current situation isn't good as it causes lot of churns for us.
> >
> > I did some more testing, and there seems to be a similar issue with
> > DMA controllers, which I hadn't really noticed before, as I have
> > DMAC drivers built-in in all my kernel configs.  If the (optional)
> > DMAC driver is not available, you need "fw_devlink=permissive" to make
> > drivers probe devices that have (optional) DMA support.
>
> Well, after the deferred probe timeout, the missing supplier depency
> should be ignored even if you don't have "permissive".
>
> Typically we get a print in the log that could say:
> "pm-test-drv pm_test24: deferred probe timeout, ignoring dependency"
>
> ... and then the driver-core tries probing the device anyway. It's
> then up to the consumer driver to allow probing to succeed, even if
> the DMA setup procedure fails.

Well, that didn't seem to work. Perhaps we still have some drivers that
do not handle -EPROBE_DEFER correctly. Will check...

> > > > > > > > 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.
> > > >
> > > > That needs explicit handling, which was not needed before.
> > > >
> > > > Perhaps the fw_devlink creation should be removed again from
> > > > of_genpd_add_provider_onecell(), as it is not correct, except for
> > > > the first domain?
> > >
> > > There is nothing wrong with it, the behavior is correct, unless I
> > > failed to understand your problem. To me the problem is that it is
> >
> > It does print wrong and confusing warnings indicating the problem:
> >
> >     genpd_provider ca57-cpu0: sync_state() pending due to fe000000.pcie
> >
> > (no, the PCIe controller is not part of the CPU PM Domain)
>
> That's a good point. We should fix this to avoid confusion.
>
> > > just less fine grained, compared to using
> > > of_genpd_add_provider_simple(). All PM domains that is provided by a
> > > single power-domain controller that uses #power-domain-cells = <1>,
> > > requires all consumers of them to be probed, to allow the sync_state
> > > callback to be invoked.
> >
> > if it would be just coarse-grained, the link should be created to a
> > device representing the controller, not to the device representing
> > the first PM domain.
> >
> > > If we could teach fw_devlink to take into account the
> > > power-domain-*id* that is specified by the consumer node, when the
> > > provider is using #power-domain-cells = <1>, this could potentially
> > > become as fine-grained as of_genpd_add_provider_simple().
> > >
> > > Saravana, do think we can extend fw_devlink to take this into account somehow?
> >
> > Doesn't the pmdomain core create a device for each PM domain, so the
> > index could be used to link to the correct domain?
>
> Correct, we have a device associated for each PM domain - and when
> of_genpd_add_provider_onecell() is called, we get the index associated
> for each of them.
>
> If I understand your proposal, you are suggesting that genpd itself
> creates the device_link between its corresponding device (supplier)
> and the upcoming consumer device(s), at the point when
> of_genpd_add_provider_onecell() is called, right?
>
> It's an interesting idea, but I am not sure it will work. To create
> the device_link at this point, assumes that all the consumer OF-nodes
> have been populated (to have devices available for them). Maybe there
> is an intermediate step we can take to instruct fw_devlink to treat
> these "links" differently?

I am not sure where these links are created. In the example above
"ca57-cpu0" is the device name of the supplier, but that is not the name
of a platform device.  Doesn't the core create devlinks between platform
devices? If yes, who creates this link to a non-platform device,
and can that code be updated easily to take into account the index cell?

> > I also had a closer look at the few pending sync_state() pending
> > warnings I do see on SH/R-Mobile SoCs.  Their PM Domain driver uses
> > of_genpd_add_provider_simple() instead of of_genpd_add_provider_onecell().
> > E.g. on R-Mobile A1:
> >
> >     genpd_provider a3sm: sync_state() pending due to f0100000.cache-controller
> >     genpd_provider a4s: sync_state() pending due to fe400000.memory-controller
> >     genpd_provider d4: sync_state() pending due to ptm
> >
> > All three domains were already handled specially in the PM Domain
> > driver, as they must not be turned off, so these platforms didn't
> > regress due to this series.
>
> Okay!
>
> Again the prints are confusing and annoying, but in principle we are
> fine. Thanks for clarifying!
>
> This also makes me wonder if we should skip enabling the fw_devlink
> support for those genpds with "simple" providers that are special
> (GENPD_FLAG_ALWAYS_ON and GENPD_FLAG_RPM_ALWAYS_ON). It doesn't make
> sense to track consumers for them, I think.

Makes sense.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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