Re: [PATCH] pmdomain: arm: scmi_pm_domain: Send an explicit request to set the current state

[Sudeep Holla <sudeep.holla@arm.com>]

On Fri, Jan 17, 2025 at 12:17:39PM +0100, Ulf Hansson wrote:
> On Thu, 16 Jan 2025 at 17:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jan 16, 2025 at 04:54:44PM +0100, Ulf Hansson wrote:
> > > On Wed, 15 Jan 2025 at 12:39, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On a system with multiple active SCMI agents, one agent(other than OSPM/
> > > > Linux or bootloader) would request to turn on a shared power domain
> > > > before the Linux boots/initialise the genpds. So when the Linux boots
> > > > and gets the power state as already ON, it just registers the genpd with
> > > > a default ON state.
> > > >
> > > > However, when the driver that needs this shared power domain is probed
> > > > genpd sees that the power domain status is ON and never makes any SCMI
> > > > call to power it up which is correct. But, since Linux didn't make an
> > > > explicit request to turn on the shared power domain, the SCMI platform
> > > > firmware will not know if the OSPM agent is actively using it.
> > > >
> > > > Suppose the other agent that requested the shared power domain to be
> > > > powered ON requests to power it OFF as it no longer needs it, the SCMI
> > > > platform firmware needs to turn it off if there are no active users of
> > > > it which in the above scenaro is the case.
> > > >
> > > > As a result of SCMI platform firmware turning off the resource, OSPM/
> > > > Linux will crash the moment as it expects the shared power domain to be
> > > > powered ON.
> > > >
> > > > Send an explicit request to set the current state when setting up the
> > > > genpd power domains so that OSPM registers its vote in the power domain
> > > > state with the SCMI platform firmware.
> > > >
> > > > The other option is to not read the state and set the genpds as default
> > > > OFF, but it can't handle the scenario on certain platforms where SCMI
> > > > platform keeps all the power domains turned ON by default for faster boot
> > > > (or any other such variations) and expect the OSPM to turn off the unused
> > > > domains if power saving is required.
> > > >
> > > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Link: https://lore.kernel.org/all/Z4aBkezSWOPCXcUh@bogus
> > > > Reported-by: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> > > > Reported-by: Peng Fan <peng.fan@nxp.com>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > >
> > > I read up on the discussion and it looks like there is not really a
> > > simple solution here.
> > >
> > > In principle if a boot-loader wants to do a handover and leave the
> > > power-domain powered-on for the kernel, the additional call to
> > > ->state_set() *could* bump the usagecount in the SCMI FW, forever
> > > leaving the power-domain on.
> > >
> >
> > IIUC, the refcount in firmware differs from the one in the kernel. It is
> > refcount per agent i.e. it is really just a kind of boolean to indicate if
> > the agent is active user of the resource. So if the bootloader and the Linux
> > being the same agent request to be turned on without a request to turn off
> > doesn't mean the refcount is set to 2 and Linux needs to turn off twice.
> > This is just my opinion and understanding.
> >
> > > I guess this problem only exists for power-domains being shared across
> > > scmi agents. Perhaps some kind of configuration flag can help us to
> > > determine what to do?
> > >
> >
> > While I can't disagree, there is also a thought that OS shouldn't be aware
> > of that detail for equally valid reasons. I am not sure if we can get that
> > added in the spec.
>
> Okay, it seems like $subject patch is the way forward at this moment.
>
> I have applied it for *next* to allow it to be a bit more tested
> before we decide if this is material for stable kernels too. That
> means we may have to send the patch to stable maintainers manually to
> get it applied.

Thanks. We should remember to drop this if we manage to get "unknown"
default/initial state supported in core genpd. I liked the proposal
you made, it is just that I feel we invariably end up sending the ON/OFF
request to the firmware anyways(can't think of any other way), it is just
deferred to a point where the domain is used and at late initcall when
unused genpds are powered off.

--
Regards,
Sudeep