[PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a resource
if no agent has requested to use that resource."

Linux Kernel should not rely on a state that it has not requested, so
make state as off during initialization.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pmdomain/arm/scmi_pm_domain.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index a7784a8bb5db..eadfc032867f 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -89,13 +89,6 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
 		return -ENOMEM;
 
 	for (i = 0; i < num_domains; i++, scmi_pd++) {
-		u32 state;
-
-		if (power_ops->state_get(ph, i, &state)) {
-			dev_warn(dev, "failed to get state for domain %d\n", i);
-			continue;
-		}
-
 		scmi_pd->domain = i;
 		scmi_pd->ph = ph;
 		scmi_pd->name = power_ops->name_get(ph, i);
@@ -104,8 +97,7 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
 		scmi_pd->genpd.power_on = scmi_pd_power_on;
 		scmi_pd->genpd.flags = GENPD_FLAG_ACTIVE_WAKEUP;
 
-		pm_genpd_init(&scmi_pd->genpd, NULL,
-			      state == SCMI_POWER_STATE_GENERIC_OFF);
+		pm_genpd_init(&scmi_pd->genpd, NULL, true);
 
 		domains[i] = &scmi_pd->genpd;
 	}
-- 
2.37.1

Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Sudeep Holla]

On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a resource
> if no agent has requested to use that resource."
>

True, but ...

> Linux Kernel should not rely on a state that it has not requested, so
> make state as off during initialization.
>

IIUC, this was done to avoid any transitions if the bootloader like U-Boot
has turned on the resource and OS can just rely on that stay. Anyways if
the resource is not used by any driver/device in the kernel, won't it be
turned off anyways ? What am I missing ?

I need to dig details, but I remember doing what this patch does and
reverting to what we have based on the feedback IIRC.

--
Regards,
Sudeep

RE: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Peng Fan]

> Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize
> state as off
> 
> On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a
> > resource if no agent has requested to use that resource."
> >
> 
> True, but ...
> 
> > Linux Kernel should not rely on a state that it has not requested, so
> > make state as off during initialization.
> >
> 
> IIUC, this was done to avoid any transitions if the bootloader like U-
> Boot has turned on the resource and OS can just rely on that stay.

But if it is not U-Boot turned it on? Or U-Boot is in a separate agent?

> Anyways if the resource is not used by any driver/device in the kernel,
> won't it be turned off anyways ? What am I missing ?

Because the power domain is ON, kernel will not issue SCMI
to platform to request it ON when kernel needs this power domain
on.

But in case when kernel is doing some jobs that needs the
power domain ON, SCMI platform might power down the
power domain because kernel agent not request that.

Thanks,
Peng.

> 
> I need to dig details, but I remember doing what this patch does and
> reverting to what we have based on the feedback IIRC.
> 
> --
> Regards,
> Sudeep

Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Sudeep Holla]

On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize
> > state as off
> >
> > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a
> > > resource if no agent has requested to use that resource."
> > >
> >
> > True, but ...
> >
> > > Linux Kernel should not rely on a state that it has not requested, so
> > > make state as off during initialization.
> > >
> >
> > IIUC, this was done to avoid any transitions if the bootloader like U-
> > Boot has turned on the resource and OS can just rely on that stay.
>
> But if it is not U-Boot turned it on?

Not sure if I understand what exactly you mean by that.

> Or U-Boot is in a separate agent?
>

No, it will be same as OS for the SCMI platform/agent as they use/share the
same transport. It is hard to distinguish between them.

> > Anyways if the resource is not used by any driver/device in the kernel,
> > won't it be turned off anyways ? What am I missing ?
>
> Because the power domain is ON, kernel will not issue SCMI
> to platform to request it ON when kernel needs this power domain
> on.
>

Yes, but the agent(via bootloader) has already requested the SCMI platform,
so it should be fine. No ?

> But in case when kernel is doing some jobs that needs the
> power domain ON, SCMI platform might power down the
> power domain because kernel agent not request that.
>

See my comment/question above.

--
Regards,
Sudeep

RE: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Ranjani Vaidyanathan]

Comments inline.

Regards,
Ranjani Vaidyanathan

-----Original Message-----
From: Sudeep Holla [mailto:sudeep.holla@arm.com] 
Sent: Monday, January 13, 2025 7:49 AM
To: Peng Fan <peng.fan@nxp.com>
Cc: Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
Subject: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state 
> > as off
> >
> > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a 
> > > resource if no agent has requested to use that resource."
> > >
> >
> > True, but ...
> >
> > > Linux Kernel should not rely on a state that it has not requested, 
> > > so make state as off during initialization.
> > >
> >
> > IIUC, this was done to avoid any transitions if the bootloader like 
> > U- Boot has turned on the resource and OS can just rely on that stay.
>
> But if it is not U-Boot turned it on?

Not sure if I understand what exactly you mean by that.
[RV] Its possible that some other agent (M33/M7 running OS) in the system turned on the power domain. Resources in the same power domain can shared across agents.  That being said, uboot provides mechanism to clean up any power domains/clocks that it enabled. And our implementation of uboot does disable any power domain it powered up for downloading of images or anything else (display is a unique case if splash screen is enabled).

> Or U-Boot is in a separate agent?
>

No, it will be same as OS for the SCMI platform/agent as they use/share the same transport. It is hard to distinguish between them.

> > Anyways if the resource is not used by any driver/device in the 
> > kernel, won't it be turned off anyways ? What am I missing ?
>
> Because the power domain is ON, kernel will not issue SCMI to platform 
> to request it ON when kernel needs this power domain on.
>

Yes, but the agent(via bootloader) has already requested the SCMI platform,
so it should be fine. No ?
[RV] As mentioned above, it need not be the bootloader. And secondly how to handle this power domain during suspend/resume? It's possible that the agent that turned on the power domain initially will have different wakeup requirements. IMO Linux should completely be responsible for the power domains that the drivers need. 

> But in case when kernel is doing some jobs that needs the
> power domain ON, SCMI platform might power down the
> power domain because kernel agent not request that.
>

See my comment/question above.

--
Regards,
Sudeep

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Sudeep Holla]

On Mon, Jan 13, 2025 at 03:30:58PM +0000, Ranjani Vaidyanathan wrote:
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Monday, January 13, 2025 7:49 AM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Subject: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off
>
> On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state
> > > as off
> > >
> > > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a
> > > > resource if no agent has requested to use that resource."
> > > >
> > >
> > > True, but ...
> > >
> > > > Linux Kernel should not rely on a state that it has not requested,
> > > > so make state as off during initialization.
> > > >
> > >
> > > IIUC, this was done to avoid any transitions if the bootloader like
> > > U- Boot has turned on the resource and OS can just rely on that stay.
> >
> > But if it is not U-Boot turned it on?
>
> Not sure if I understand what exactly you mean by that.
> [RV] Its possible that some other agent (M33/M7 running OS) in the system
> turned on the power domain. Resources in the same power domain can shared
> across agents.  That being said, uboot provides mechanism to clean up any
> power domains/clocks that it enabled. And our implementation of uboot does
> disable any power domain it powered up for downloading of images or anything
> else (display is a unique case if splash screen is enabled).
>

Right I was referring to the display as one of the example when I referred
to the case where bootloader turns on the resource.

> >
> > Because the power domain is ON, kernel will not issue SCMI to platform
> > to request it ON when kernel needs this power domain on.
> >
>
> Yes, but the agent(via bootloader) has already requested the SCMI platform,
> so it should be fine. No ?
> [RV] As mentioned above, it need not be the bootloader. And secondly how to
> handle this power domain during suspend/resume? It's possible that the agent
> that turned on the power domain initially will have different wakeup
> requirements. IMO Linux should completely be responsible for the power
> domains that the drivers need.
>

May be I am still missing something. The genpd framework does issue power
down of all the PD that are not used once we boot. Is that not sufficient.
We are just not changing the pd state when initialising the genpds.
Is that causing the issue ? I was under the impression that it shouldn't
matter if the driver manages the genpds they consume and all unused ones
get turned off eventually.

What exactly is the issue for which this patch is the solution ? I think
I might have not understood the issue properly here. Sorry if that is the
case.

--
Regards,
Sudeep

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Cristian Marussi]

On Mon, Jan 13, 2025 at 05:20:38PM +0000, Sudeep Holla wrote:
> On Mon, Jan 13, 2025 at 03:30:58PM +0000, Ranjani Vaidyanathan wrote:
> > -----Original Message-----
> > From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> > Sent: Monday, January 13, 2025 7:49 AM
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> > Subject: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off
> >
> > On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state
> > > > as off
> > > >
> > > > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > >
> > > > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a
> > > > > resource if no agent has requested to use that resource."
> > > > >
> > > >
> > > > True, but ...
> > > >
> > > > > Linux Kernel should not rely on a state that it has not requested,
> > > > > so make state as off during initialization.
> > > > >
> > > >
> > > > IIUC, this was done to avoid any transitions if the bootloader like
> > > > U- Boot has turned on the resource and OS can just rely on that stay.
> > >
> > > But if it is not U-Boot turned it on?

Hi Sudeep, Ranjani,

> >
> > Not sure if I understand what exactly you mean by that.
> > [RV] Its possible that some other agent (M33/M7 running OS) in the system
> > turned on the power domain. Resources in the same power domain can shared
> > across agents.  That being said, uboot provides mechanism to clean up any
> > power domains/clocks that it enabled. And our implementation of uboot does
> > disable any power domain it powered up for downloading of images or anything
> > else (display is a unique case if splash screen is enabled).
> >
> 

So, RV,  you are saying that the your UBoot is cleanly releasing/turning_OFF all
the resources that it claimed during its life before giving control to
Linux (sicne no more needed) BUT some other agent in the system has requested
that resource to be ON, so when Linux query the status of the resources it sees
it as ON since it sees the physical real status of the resource ?

If this is the case, I suspected this was the issue, and I will address these
scenario in a separate mail on this thread that I was already in the middle of
writing....

> Right I was referring to the display as one of the example when I referred
> to the case where bootloader turns on the resource.
> 
> > >
> > > Because the power domain is ON, kernel will not issue SCMI to platform
> > > to request it ON when kernel needs this power domain on.
> > >
> >
> > Yes, but the agent(via bootloader) has already requested the SCMI platform,
> > so it should be fine. No ?
> > [RV] As mentioned above, it need not be the bootloader. And secondly how to
> > handle this power domain during suspend/resume? It's possible that the agent
> > that turned on the power domain initially will have different wakeup
> > requirements. IMO Linux should completely be responsible for the power
> > domains that the drivers need.
> >
> 
> May be I am still missing something. The genpd framework does issue power
> down of all the PD that are not used once we boot. Is that not sufficient.
> We are just not changing the pd state when initialising the genpds.
> Is that causing the issue ? I was under the impression that it shouldn't
> matter if the driver manages the genpds they consume and all unused ones
> get turned off eventually.

I think GenPD turns off unused PD domains ONLY if pd_ignore_unused
kernel-param is false, BUT it is true by default AFAICS.

Thanks,
Cristian

RE: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Ranjani Vaidyanathan]

Hello Cristian,

Regards,
Ranjani Vaidyanathan

-----Original Message-----
From: Cristian Marussi [mailto:cristian.marussi@arm.com] 
Sent: Monday, January 13, 2025 11:54 AM
To: Sudeep Holla <sudeep.holla@arm.com>; Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


On Mon, Jan 13, 2025 at 05:20:38PM +0000, Sudeep Holla wrote:
> On Mon, Jan 13, 2025 at 03:30:58PM +0000, Ranjani Vaidyanathan wrote:
> > -----Original Message-----
> > From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> > Sent: Monday, January 13, 2025 7:49 AM
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; 
> > Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; 
> > arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; 
> > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Ranjani 
> > Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> > Subject: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize 
> > state as off
> >
> > On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize 
> > > > state as off
> > > >
> > > > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > >
> > > > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable 
> > > > > a resource if no agent has requested to use that resource."
> > > > >
> > > >
> > > > True, but ...
> > > >
> > > > > Linux Kernel should not rely on a state that it has not 
> > > > > requested, so make state as off during initialization.
> > > > >
> > > >
> > > > IIUC, this was done to avoid any transitions if the bootloader 
> > > > like
> > > > U- Boot has turned on the resource and OS can just rely on that stay.
> > >
> > > But if it is not U-Boot turned it on?

Hi Sudeep, Ranjani,

> >
> > Not sure if I understand what exactly you mean by that.
> > [RV] Its possible that some other agent (M33/M7 running OS) in the 
> > system turned on the power domain. Resources in the same power 
> > domain can shared across agents.  That being said, uboot provides 
> > mechanism to clean up any power domains/clocks that it enabled. And 
> > our implementation of uboot does disable any power domain it powered 
> > up for downloading of images or anything else (display is a unique case if splash screen is enabled).
> >
>

So, RV,  you are saying that the your UBoot is cleanly releasing/turning_OFF all the resources that it claimed during its life before giving control to Linux (sicne no more needed) BUT some other agent in the system has requested that resource to be ON, so when Linux query the status of the resources it sees it as ON since it sees the physical real status of the resource ?

If this is the case, I suspected this was the issue, and I will address these scenario in a separate mail on this thread that I was already in the middle of writing....

[RV] Yes, this is our case. The platform will return the physical view of the resource (as SCMI spec has left it to be implementation defined).  I will try to provide comments to your other long email. 

> Right I was referring to the display as one of the example when I 
> referred to the case where bootloader turns on the resource.
>
> > >
> > > Because the power domain is ON, kernel will not issue SCMI to 
> > > platform to request it ON when kernel needs this power domain on.
> > >
> >
> > Yes, but the agent(via bootloader) has already requested the SCMI 
> > platform, so it should be fine. No ?
> > [RV] As mentioned above, it need not be the bootloader. And secondly 
> > how to handle this power domain during suspend/resume? It's possible 
> > that the agent that turned on the power domain initially will have 
> > different wakeup requirements. IMO Linux should completely be 
> > responsible for the power domains that the drivers need.
> >
>
> May be I am still missing something. The genpd framework does issue 
> power down of all the PD that are not used once we boot. Is that not sufficient.
> We are just not changing the pd state when initialising the genpds.
> Is that causing the issue ? I was under the impression that it 
> shouldn't matter if the driver manages the genpds they consume and all 
> unused ones get turned off eventually.

I think GenPD turns off unused PD domains ONLY if pd_ignore_unused kernel-param is false, BUT it is true by default AFAICS.

Thanks,
Cristian

RE: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Ranjani Vaidyanathan]

Hello Sudeep,

Will try to explain the situation we are facing.
1. We have multiple agents running, Agent-A is booted up first before Linux is booted and powers up a shared power domain PD-X
2. Linux boots and gets the power state of PD-X. And its already ON. And then PD -X is initialized with a default ON state. 
3. When the driver that needs PD-X  is probed, Linux sees that the power domain status is ON and never makes an SCMI call to power up the PD-X for Linux Agent
4. Agent-A now is shutdown/suspends. Linux will crash because the platform disables PD-X because it has no other requests for PD-X. 


Regards,
Ranjani Vaidyanathan

-----Original Message-----
From: Sudeep Holla [mailto:sudeep.holla@arm.com] 
Sent: Monday, January 13, 2025 11:21 AM
To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; Sudeep Holla <sudeep.holla@arm.com>; cristian.marussi@arm.com; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


On Mon, Jan 13, 2025 at 03:30:58PM +0000, Ranjani Vaidyanathan wrote:
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Monday, January 13, 2025 7:49 AM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; 
> Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; 
> arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; 
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Ranjani 
> Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Subject: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize 
> state as off
>
> On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize 
> > > state as off
> > >
> > > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a 
> > > > resource if no agent has requested to use that resource."
> > > >
> > >
> > > True, but ...
> > >
> > > > Linux Kernel should not rely on a state that it has not 
> > > > requested, so make state as off during initialization.
> > > >
> > >
> > > IIUC, this was done to avoid any transitions if the bootloader 
> > > like
> > > U- Boot has turned on the resource and OS can just rely on that stay.
> >
> > But if it is not U-Boot turned it on?
>
> Not sure if I understand what exactly you mean by that.
> [RV] Its possible that some other agent (M33/M7 running OS) in the 
> system turned on the power domain. Resources in the same power domain 
> can shared across agents.  That being said, uboot provides mechanism 
> to clean up any power domains/clocks that it enabled. And our 
> implementation of uboot does disable any power domain it powered up 
> for downloading of images or anything else (display is a unique case if splash screen is enabled).
>

Right I was referring to the display as one of the example when I referred to the case where bootloader turns on the resource.

> >
> > Because the power domain is ON, kernel will not issue SCMI to 
> > platform to request it ON when kernel needs this power domain on.
> >
>
> Yes, but the agent(via bootloader) has already requested the SCMI 
> platform, so it should be fine. No ?
> [RV] As mentioned above, it need not be the bootloader. And secondly 
> how to handle this power domain during suspend/resume? It's possible 
> that the agent that turned on the power domain initially will have 
> different wakeup requirements. IMO Linux should completely be 
> responsible for the power domains that the drivers need.
>

May be I am still missing something. The genpd framework does issue power down of all the PD that are not used once we boot. Is that not sufficient.
We are just not changing the pd state when initialising the genpds.
Is that causing the issue ? I was under the impression that it shouldn't matter if the driver manages the genpds they consume and all unused ones get turned off eventually.

What exactly is the issue for which this patch is the solution ? I think I might have not understood the issue properly here. Sorry if that is the case.

--
Regards,
Sudeep

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Sudeep Holla]

Hi Ranjani,

On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan wrote:
> Hello Sudeep,
>
> Will try to explain the situation we are facing.
> 1. We have multiple agents running, Agent-A is booted up first before Linux
> is booted and powers up a shared power domain PD-X.
> 2. Linux boots and gets the power state of PD-X. And its already ON. And
> then PD -X is initialized with a default ON state.
> 3. When the driver that needs PD-X  is probed, Linux sees that the power
> domain status is ON and never makes an SCMI call to power up the PD-X for
> Linux Agent.
> 4. Agent-A now is shutdown/suspends. Linux will crash because the platform
> disables PD-X because it has no other requests for PD-X.
>

Thanks for the detailed explanation. I understand the issue now.

I would like to discuss if the below alternative approach works for you.
We can debate the pros and cons. I see with the approach in this patch
proposed by Peng we would avoid querying and setting genpd all together
during the genpd initialisation which is good. But if there are any genpd
left on by the platform or bootloader(same agent), it will not get turned
off when Linux tries to turn off the unused genpds(IIRC this could be the
reason for the current state of code). While your platform may find sending
those commands unnecessary, there was some usecase where SCMI platform kept
all resources ON by default for faster boot and expects OSPM to turn off
unused resources. So we need to support both the cases. I hope my below
patch should suffice.

Regards,
Sudeep

---->8

From dc755fa02d351e71c727da1c396e2d346b496096 Mon Sep 17 00:00:00 2001
From: Sudeep Holla <sudeep.holla@arm.com>
Date: Tue, 14 Jan 2025 15:08:44 +0000
Subject: [PATCH] pmdomain: arm: scmi_pm_domain: Send an explicit request to
 set the current state

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. 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 and expect the OSPM to turn off the unused
domains if power saving is required.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/pmdomain/arm/scmi_pm_domain.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index a7784a8bb5db..255c8c36a15c 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -96,6 +96,8 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
 			continue;
 		}

+		power_ops->state_set(ph, i, state);
+
 		scmi_pd->domain = i;
 		scmi_pd->ph = ph;
 		scmi_pd->name = power_ops->name_get(ph, i);
--
2.34.1

RE: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Ranjani Vaidyanathan]

Hello Sudeep,

Comments below.

Regards,
Ranjani Vaidyanathan

-----Original Message-----
From: Sudeep Holla [mailto:sudeep.holla@arm.com] 
Sent: Tuesday, January 14, 2025 9:24 AM
To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


Hi Ranjani,

On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan wrote:
> Hello Sudeep,
>
> Will try to explain the situation we are facing.
> 1. We have multiple agents running, Agent-A is booted up first before 
> Linux is booted and powers up a shared power domain PD-X.
> 2. Linux boots and gets the power state of PD-X. And its already ON. 
> And then PD -X is initialized with a default ON state.
> 3. When the driver that needs PD-X  is probed, Linux sees that the 
> power domain status is ON and never makes an SCMI call to power up the 
> PD-X for Linux Agent.
> 4. Agent-A now is shutdown/suspends. Linux will crash because the 
> platform disables PD-X because it has no other requests for PD-X.
>

Thanks for the detailed explanation. I understand the issue now.

I would like to discuss if the below alternative approach works for you.
We can debate the pros and cons. I see with the approach in this patch proposed by Peng we would avoid querying and setting genpd all together during the genpd initialisation which is good. But if there are any genpd left on by the platform or bootloader(same agent), it will not get turned off when Linux tries to turn off the unused genpds(IIRC this could be the reason for the current state of code). While your platform may find sending those commands unnecessary, there was some usecase where SCMI platform kept all resources ON by default for faster boot and expects OSPM to turn off unused resources. So we need to support both the cases. I hope my below patch should suffice.

[RV] Linux can still make the call to disable unused power domains, even if it never explicitly made a request to power it on. The platform will aggregate the request from all agents and will power off the resource if no other agent has enabled it. From Linux point of view it has disabled all unused power domains. 
Your patch below may also work, but feels like a workaround to artificially (for lack of a better word) enable a resource. And also makes unnecessary SCMI calls (expensive) for every resource immaterial of it power state (maybe can be improved by a conditional check). 

Regards,
Sudeep

---->8

From dc755fa02d351e71c727da1c396e2d346b496096 Mon Sep 17 00:00:00 2001
From: Sudeep Holla <sudeep.holla@arm.com>
Date: Tue, 14 Jan 2025 15:08:44 +0000
Subject: [PATCH] pmdomain: arm: scmi_pm_domain: Send an explicit request to  set the current state

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. 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 and expect the OSPM to turn off the unused domains if power saving is required.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/pmdomain/arm/scmi_pm_domain.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
index a7784a8bb5db..255c8c36a15c 100644
--- a/drivers/pmdomain/arm/scmi_pm_domain.c
+++ b/drivers/pmdomain/arm/scmi_pm_domain.c
@@ -96,6 +96,8 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
                        continue;
                }

+               power_ops->state_set(ph, i, state);
+
                scmi_pd->domain = i;
                scmi_pd->ph = ph;
                scmi_pd->name = power_ops->name_get(ph, i);
--
2.34.1

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Sudeep Holla]

On Tue, Jan 14, 2025 at 04:09:13PM +0000, Ranjani Vaidyanathan wrote:
> Hello Sudeep,
>
> Comments below.
>
> Regards,
> Ranjani Vaidyanathan
>
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Tuesday, January 14, 2025 9:24 AM
> To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off
>
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
>
>
> Hi Ranjani,
>
> On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan wrote:
> > Hello Sudeep,
> >
> > Will try to explain the situation we are facing.
> > 1. We have multiple agents running, Agent-A is booted up first before
> > Linux is booted and powers up a shared power domain PD-X.
> > 2. Linux boots and gets the power state of PD-X. And its already ON.
> > And then PD -X is initialized with a default ON state.
> > 3. When the driver that needs PD-X  is probed, Linux sees that the
> > power domain status is ON and never makes an SCMI call to power up the
> > PD-X for Linux Agent.
> > 4. Agent-A now is shutdown/suspends. Linux will crash because the
> > platform disables PD-X because it has no other requests for PD-X.
> >
>
> Thanks for the detailed explanation. I understand the issue now.
>
> I would like to discuss if the below alternative approach works for you.
> We can debate the pros and cons. I see with the approach in this patch
> proposed by Peng we would avoid querying and setting genpd all together
> during the genpd initialisation which is good. But if there are any genpd
> left on by the platform or bootloader(same agent), it will not get turned
> off when Linux tries to turn off the unused genpds(IIRC this could be the
> reason for the current state of code). While your platform may find sending
> those commands unnecessary, there was some usecase where SCMI platform kept
> all resources ON by default for faster boot and expects OSPM to turn off
> unused resources. So we need to support both the cases. I hope my below
> patch should suffice.
>
> [RV] Linux can still make the call to disable unused power domains, even if
> it never explicitly made a request to power it on. The platform will
> aggregate the request from all agents and will power off the resource if no
> other agent has enabled it. From Linux point of view it has disabled all
> unused power domains.

I need to dig into genpd to see if that is possible. IIUC, genpd tracks the
state and will call off only if it is turned on and is unused. If we
initialise to default off state, it may not issue the OFF call to the
firmware irrespective of the actual state(i.e. even if it left ON).

> Your patch below may also work, but feels like a workaround to artificially
> (for lack of a better word) enable a resource.

I tend to agree this might feel like workaround but I need to look and
refresh my genpd knowledge. Or we can check with Ulf.

> And also makes unnecessary SCMI calls (expensive) for every resource
> immaterial of it power state (maybe can be improved by a conditional check).

Yes we can make explicit call only for ON state.

--
Regards,
Sudeep

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Cristian Marussi]

On Tue, Jan 14, 2025 at 04:09:13PM +0000, Ranjani Vaidyanathan wrote:
> Hello Sudeep,
> 
> Comments below.
> 
> Regards,
> Ranjani Vaidyanathan
> 
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com] 
> Sent: Tuesday, January 14, 2025 9:24 AM
> To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla <sudeep.holla@arm.com>; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off
> 
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Hi Ranjani,
> 
> On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan wrote:
> > Hello Sudeep,
> >
> > Will try to explain the situation we are facing.
> > 1. We have multiple agents running, Agent-A is booted up first before 
> > Linux is booted and powers up a shared power domain PD-X.
> > 2. Linux boots and gets the power state of PD-X. And its already ON. 
> > And then PD -X is initialized with a default ON state.
> > 3. When the driver that needs PD-X  is probed, Linux sees that the 
> > power domain status is ON and never makes an SCMI call to power up the 
> > PD-X for Linux Agent.
> > 4. Agent-A now is shutdown/suspends. Linux will crash because the 
> > platform disables PD-X because it has no other requests for PD-X.
> >
> 
> Thanks for the detailed explanation. I understand the issue now.
> 
> I would like to discuss if the below alternative approach works for you.
> We can debate the pros and cons. I see with the approach in this patch proposed by Peng we would avoid querying and setting genpd all together during the genpd initialisation which is good. But if there are any genpd left on by the platform or bootloader(same agent), it will not get turned off when Linux tries to turn off the unused genpds(IIRC this could be the reason for the current state of code). While your platform may find sending those commands unnecessary, there was some usecase where SCMI platform kept all resources ON by default for faster boot and expects OSPM to turn off unused resources. So we need to support both the cases. I hope my below patch should suffice.
> 
> [RV] Linux can still make the call to disable unused power domains, even if it never explicitly made a request to power it on. The platform will aggregate the request from all agents and will power off the resource if no other agent has enabled it. From Linux point of view it has disabled all unused power domains. 
> Your patch below may also work, but feels like a workaround to artificially (for lack of a better word) enable a resource. And also makes unnecessary SCMI calls (expensive) for every resource immaterial of it power state (maybe can be improved by a conditional check). 
> 

...sincerely, both of these solutions seem to me hacks/workarounds to
counteract the fundamental issue that derives from having allowed (IMPDEF)
to implement the get operations to return the real physical state of a
resource instead of its virtual per-agent state as maintained by the platform,
while, at the same time, having allowed to implement the set-operations to
operate in a 'virtual-fashion'...

...so, when Peng's patch forcibly set the state to OFF on genpd init, you
are indeed artificially forcing the kernel internal state to align with
what would have been the virtual-per-agent state of the resource in your
specific particular configuration....

...on the other side Sudeep's proposed patch tries really to play the same
trick, just on the other way around, by instead forcibly/artificially aligning
the state on the platform side by issuing a redundant ON request to bump the
refcount and take hold of that resource from the Kernel agent point of view...

... but Peng's proposed patch will broke immediately the moment you have
instead a system with an SCMI-capable bootloader that instead left the
resource ON for the Kernel to inherit, since the kernel will now forcibly
see this anyway as OFF, and so you wont be ever be able to switch that resource
REALLY OFF in the future, if ever needed, because the bootloader/Kernel agent
will never see it as ON in genPD, since, at least in the genPD case, AFAICS
correct me if wrong, there is no callback to peek at the real state later on:
so, after the initialization value has been chosen at genpd_init time, genPd
subsystem maintains the PD state on its own based on the issued ON/OFF genPD
requests, so your forced-initial-OFF-state will be, in this specific alternative
scenario, wrong and forever.

...I think at least Sudeep's solution could survive this scenario
because it is more general and can cope with both scenarios

I know this phys-vs-virtual state is a lost battle at this point, but the
IMPDEF possible scenarios that derive from this choice now have to be supported
kernel-side as best as we can both ways...with the additional headache that,
from the Kernel agent perspective, we cannot infer what kind of IMPDEF get_state
is implemented by the platform we are talking to AND various different
subsystem in the Linux kernel handle this "initial-state-matter" in
different ways (e.g. see  clocks subsystem)

Thanks
Cristian

RE: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Ranjani Vaidyanathan]

Hi Cristian,

Regards,
Ranjani Vaidyanathan

-----Original Message-----
From: Cristian Marussi [mailto:cristian.marussi@arm.com] 
Sent: Wednesday, January 15, 2025 3:15 AM
To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>; Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


On Tue, Jan 14, 2025 at 04:09:13PM +0000, Ranjani Vaidyanathan wrote:
> Hello Sudeep,
>
> Comments below.
>
> Regards,
> Ranjani Vaidyanathan
>
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Tuesday, January 14, 2025 9:24 AM
> To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) 
> <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla 
> <sudeep.holla@arm.com>; ulf.hansson@linaro.org; 
> arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; 
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: 
> Initialize state as off
>
> Caution: This is an external email. Please take care when clicking 
> links or opening attachments. When in doubt, report the message using 
> the 'Report this email' button
>
>
> Hi Ranjani,
>
> On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan wrote:
> > Hello Sudeep,
> >
> > Will try to explain the situation we are facing.
> > 1. We have multiple agents running, Agent-A is booted up first 
> > before Linux is booted and powers up a shared power domain PD-X.
> > 2. Linux boots and gets the power state of PD-X. And its already ON.
> > And then PD -X is initialized with a default ON state.
> > 3. When the driver that needs PD-X  is probed, Linux sees that the 
> > power domain status is ON and never makes an SCMI call to power up 
> > the PD-X for Linux Agent.
> > 4. Agent-A now is shutdown/suspends. Linux will crash because the 
> > platform disables PD-X because it has no other requests for PD-X.
> >
>
> Thanks for the detailed explanation. I understand the issue now.
>
> I would like to discuss if the below alternative approach works for you.
> We can debate the pros and cons. I see with the approach in this patch proposed by Peng we would avoid querying and setting genpd all together during the genpd initialisation which is good. But if there are any genpd left on by the platform or bootloader(same agent), it will not get turned off when Linux tries to turn off the unused genpds(IIRC this could be the reason for the current state of code). While your platform may find sending those commands unnecessary, there was some usecase where SCMI platform kept all resources ON by default for faster boot and expects OSPM to turn off unused resources. So we need to support both the cases. I hope my below patch should suffice.
>
> [RV] Linux can still make the call to disable unused power domains, even if it never explicitly made a request to power it on. The platform will aggregate the request from all agents and will power off the resource if no other agent has enabled it. From Linux point of view it has disabled all unused power domains.
> Your patch below may also work, but feels like a workaround to artificially (for lack of a better word) enable a resource. And also makes unnecessary SCMI calls (expensive) for every resource immaterial of it power state (maybe can be improved by a conditional check).
>

...sincerely, both of these solutions seem to me hacks/workarounds to counteract the fundamental issue that derives from having allowed (IMPDEF) to implement the get operations to return the real physical state of a resource instead of its virtual per-agent state as maintained by the platform, while, at the same time, having allowed to implement the set-operations to operate in a 'virtual-fashion'...

...so, when Peng's patch forcibly set the state to OFF on genpd init, you are indeed artificially forcing the kernel internal state to align with what would have been the virtual-per-agent state of the resource in your specific particular configuration....
[RV] Perhaps it's a hack. But at boot the state should look like OFF, the agent should explicitly request those it needs to be ON. 

...on the other side Sudeep's proposed patch tries really to play the same trick, just on the other way around, by instead forcibly/artificially aligning the state on the platform side by issuing a redundant ON request to bump the refcount and take hold of that resource from the Kernel agent point of view...

... but Peng's proposed patch will broke immediately the moment you have instead a system with an SCMI-capable bootloader that instead left the resource ON for the Kernel to inherit, since the kernel will now forcibly see this anyway as OFF, and so you wont be ever be able to switch that resource REALLY OFF in the future, if ever needed, because the bootloader/Kernel agent will never see it as ON in genPD, since, at least in the genPD case, AFAICS correct me if wrong, there is no callback to peek at the real state later on:
so, after the initialization value has been chosen at genpd_init time, genPd subsystem maintains the PD state on its own based on the issued ON/OFF genPD requests, so your forced-initial-OFF-state will be, in this specific alternative scenario, wrong and forever.
[
[RV] SCMI-capable bootloader and Linux should be the same logical machine (different agents). And the platform maintains the state per logical machine. So if Linux tries to power off a state that was left powered ON by the bootloader it should bbe able to.

...I think at least Sudeep's solution could survive this scenario because it is more general and can cope with both scenarios

I know this phys-vs-virtual state is a lost battle at this point, but the IMPDEF possible scenarios that derive from this choice now have to be supported kernel-side as best as we can both ways...with the additional headache that, from the Kernel agent perspective, we cannot infer what kind of IMPDEF get_state is implemented by the platform we are talking to AND various different subsystem in the Linux kernel handle this "initial-state-matter" in different ways (e.g. see  clocks subsystem)
[RV] True agree. It would have been good if we had an attribute(s) to define what state the platform returns as part of the SCMI API. Perhaps the platform can return both the agent and physical state. 

Thanks
Cristian

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Cristian Marussi]

On Wed, Jan 15, 2025 at 06:42:46PM +0000, Ranjani Vaidyanathan wrote:
> Hi Cristian,
> 
> Regards,
> Ranjani Vaidyanathan
> 
> -----Original Message-----
> From: Cristian Marussi [mailto:cristian.marussi@arm.com] 
> Sent: Wednesday, January 15, 2025 3:15 AM
> To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>; Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; ulf.hansson@linaro.org; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off
> 
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> On Tue, Jan 14, 2025 at 04:09:13PM +0000, Ranjani Vaidyanathan wrote:
> > Hello Sudeep,
> >
> > Comments below.
> >
> > Regards,
> > Ranjani Vaidyanathan
> >
> > -----Original Message-----
> > From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> > Sent: Tuesday, January 14, 2025 9:24 AM
> > To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> > Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS) 
> > <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep Holla 
> > <sudeep.holla@arm.com>; ulf.hansson@linaro.org; 
> > arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; 
> > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: 
> > Initialize state as off
> >
> > Caution: This is an external email. Please take care when clicking 
> > links or opening attachments. When in doubt, report the message using 
> > the 'Report this email' button
> >
> >
> > Hi Ranjani,
> >
> > On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan wrote:
> > > Hello Sudeep,
> > >
> > > Will try to explain the situation we are facing.
> > > 1. We have multiple agents running, Agent-A is booted up first 
> > > before Linux is booted and powers up a shared power domain PD-X.
> > > 2. Linux boots and gets the power state of PD-X. And its already ON.
> > > And then PD -X is initialized with a default ON state.
> > > 3. When the driver that needs PD-X  is probed, Linux sees that the 
> > > power domain status is ON and never makes an SCMI call to power up 
> > > the PD-X for Linux Agent.
> > > 4. Agent-A now is shutdown/suspends. Linux will crash because the 
> > > platform disables PD-X because it has no other requests for PD-X.
> > >
> >
> > Thanks for the detailed explanation. I understand the issue now.
> >
> > I would like to discuss if the below alternative approach works for you.
> > We can debate the pros and cons. I see with the approach in this patch proposed by Peng we would avoid querying and setting genpd all together during the genpd initialisation which is good. But if there are any genpd left on by the platform or bootloader(same agent), it will not get turned off when Linux tries to turn off the unused genpds(IIRC this could be the reason for the current state of code). While your platform may find sending those commands unnecessary, there was some usecase where SCMI platform kept all resources ON by default for faster boot and expects OSPM to turn off unused resources. So we need to support both the cases. I hope my below patch should suffice.
> >
> > [RV] Linux can still make the call to disable unused power domains, even if it never explicitly made a request to power it on. The platform will aggregate the request from all agents and will power off the resource if no other agent has enabled it. From Linux point of view it has disabled all unused power domains.
> > Your patch below may also work, but feels like a workaround to artificially (for lack of a better word) enable a resource. And also makes unnecessary SCMI calls (expensive) for every resource immaterial of it power state (maybe can be improved by a conditional check).
> >
> 
> ...sincerely, both of these solutions seem to me hacks/workarounds to counteract the fundamental issue that derives from having allowed (IMPDEF) to implement the get operations to return the real physical state of a resource instead of its virtual per-agent state as maintained by the platform, while, at the same time, having allowed to implement the set-operations to operate in a 'virtual-fashion'...
> 
> ...so, when Peng's patch forcibly set the state to OFF on genpd init, you are indeed artificially forcing the kernel internal state to align with what would have been the virtual-per-agent state of the resource in your specific particular configuration....
> [RV] Perhaps it's a hack. But at boot the state should look like OFF, the agent should explicitly request those it needs to be ON. 
> 

Yes it is what I am saying, it should see it OFF in this system config, and
that would be the case on a platform that returns virtual per-agent states:
forcing GENPD to see as it as off just mimics the same, but breaks other
cases as I mentioned.

> ...on the other side Sudeep's proposed patch tries really to play the same trick, just on the other way around, by instead forcibly/artificially aligning the state on the platform side by issuing a redundant ON request to bump the refcount and take hold of that resource from the Kernel agent point of view...
> 
> ... but Peng's proposed patch will broke immediately the moment you have instead a system with an SCMI-capable bootloader that instead left the resource ON for the Kernel to inherit, since the kernel will now forcibly see this anyway as OFF, and so you wont be ever be able to switch that resource REALLY OFF in the future, if ever needed, because the bootloader/Kernel agent will never see it as ON in genPD, since, at least in the genPD case, AFAICS correct me if wrong, there is no callback to peek at the real state later on:
> so, after the initialization value has been chosen at genpd_init time, genPd subsystem maintains the PD state on its own based on the issued ON/OFF genPD requests, so your forced-initial-OFF-state will be, in this specific alternative scenario, wrong and forever.
> [
> [RV] SCMI-capable bootloader and Linux should be the same logical machine (different agents). And the platform maintains the state per logical machine. So if Linux tries to power off a state that was left powered ON by the bootloader it should bbe able to.

In the SCMI world there are agents, i.e. clients issuing requests to the
platform AND the platform identifies such agents from the channel they
speak from. Not sure what you mean by logical machine.

In the case of an SCMI-aware bootloader like UBoot, that dies and relinquishes
all the resources to the Kernel during the boot, that means that the Kernel
should be configured to simply re-use the same SCMI channels as UBoot, so that
it will be seen as the same agent (transparently) from the platform: in such a
scenario the Kernel will transparently inherit all the per-agent current
interrnal state...

...IOW if an SCMI/Uboot was holding res_X, the Kernel will result as holding the
same res_X "by inheritance" from the platform point of view, since the Kernel
would have assumed the role of the UBoot agent from the platfom point of
view, since it is using the same channels as UBoot.

Why you should consider Uboot a diffrent agent, if it runs in NS-world
too and does not survive the Kernel boot ?

This, at least, is my understannding after bunch of past talk with ATG.

And this is the scenario that I fear would fail with Peng's patch.

Thanks,
Cristian

RE: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Peng Fan]

> Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain:
> Initialize state as off
> 
> On Wed, Jan 15, 2025 at 06:42:46PM +0000, Ranjani Vaidyanathan
> wrote:
> > Hi Cristian,
> >
> > Regards,
> > Ranjani Vaidyanathan
> >
> > -----Original Message-----
> > From: Cristian Marussi [mailto:cristian.marussi@arm.com]
> > Sent: Wednesday, January 15, 2025 3:15 AM
> > To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>; Peng Fan
> <peng.fan@nxp.com>;
> > Peng Fan (OSS) <peng.fan@oss.nxp.com>; cristian.marussi@arm.com;
> > ulf.hansson@linaro.org; arm-scmi@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain:
> > Initialize state as off
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message
> using
> > the 'Report this email' button
> >
> >
> > On Tue, Jan 14, 2025 at 04:09:13PM +0000, Ranjani Vaidyanathan
> wrote:
> > > Hello Sudeep,
> > >
> > > Comments below.
> > >
> > > Regards,
> > > Ranjani Vaidyanathan
> > >
> > > -----Original Message-----
> > > From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> > > Sent: Tuesday, January 14, 2025 9:24 AM
> > > To: Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
> > > Cc: Peng Fan <peng.fan@nxp.com>; Peng Fan (OSS)
> > > <peng.fan@oss.nxp.com>; cristian.marussi@arm.com; Sudeep
> Holla
> > > <sudeep.holla@arm.com>; ulf.hansson@linaro.org;
> > > arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain:
> > > Initialize state as off
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments. When in doubt, report the message
> > > using the 'Report this email' button
> > >
> > >
> > > Hi Ranjani,
> > >
> > > On Mon, Jan 13, 2025 at 07:54:06PM +0000, Ranjani Vaidyanathan
> wrote:
> > > > Hello Sudeep,
> > > >
> > > > Will try to explain the situation we are facing.
> > > > 1. We have multiple agents running, Agent-A is booted up first
> > > > before Linux is booted and powers up a shared power domain PD-
> X.
> > > > 2. Linux boots and gets the power state of PD-X. And its already
> ON.
> > > > And then PD -X is initialized with a default ON state.
> > > > 3. When the driver that needs PD-X  is probed, Linux sees that the
> > > > power domain status is ON and never makes an SCMI call to
> power up
> > > > the PD-X for Linux Agent.
> > > > 4. Agent-A now is shutdown/suspends. Linux will crash because
> the
> > > > platform disables PD-X because it has no other requests for PD-X.
> > > >
> > >
> > > Thanks for the detailed explanation. I understand the issue now.
> > >
> > > I would like to discuss if the below alternative approach works for
> you.
> > > We can debate the pros and cons. I see with the approach in this
> patch proposed by Peng we would avoid querying and setting genpd all
> together during the genpd initialisation which is good. But if there are
> any genpd left on by the platform or bootloader(same agent), it will
> not get turned off when Linux tries to turn off the unused genpds(IIRC
> this could be the reason for the current state of code). While your
> platform may find sending those commands unnecessary, there was
> some usecase where SCMI platform kept all resources ON by default
> for faster boot and expects OSPM to turn off unused resources. So we
> need to support both the cases. I hope my below patch should suffice.
> > >
> > > [RV] Linux can still make the call to disable unused power domains,
> even if it never explicitly made a request to power it on. The platform
> will aggregate the request from all agents and will power off the
> resource if no other agent has enabled it. From Linux point of view it
> has disabled all unused power domains.
> > > Your patch below may also work, but feels like a workaround to
> artificially (for lack of a better word) enable a resource. And also makes
> unnecessary SCMI calls (expensive) for every resource immaterial of it
> power state (maybe can be improved by a conditional check).
> > >
> >
> > ...sincerely, both of these solutions seem to me hacks/workarounds
> to counteract the fundamental issue that derives from having allowed
> (IMPDEF) to implement the get operations to return the real physical
> state of a resource instead of its virtual per-agent state as maintained
> by the platform, while, at the same time, having allowed to implement
> the set-operations to operate in a 'virtual-fashion'...
> >
> > ...so, when Peng's patch forcibly set the state to OFF on genpd init,
> you are indeed artificially forcing the kernel internal state to align with
> what would have been the virtual-per-agent state of the resource in
> your specific particular configuration....
> > [RV] Perhaps it's a hack. But at boot the state should look like OFF,
> the agent should explicitly request those it needs to be ON.
> >
> 
> Yes it is what I am saying, it should see it OFF in this system config, and
> that would be the case on a platform that returns virtual per-agent
> states:
> forcing GENPD to see as it as off just mimics the same, but breaks other
> cases as I mentioned.
> 
> > ...on the other side Sudeep's proposed patch tries really to play the
> same trick, just on the other way around, by instead forcibly/artificially
> aligning the state on the platform side by issuing a redundant ON
> request to bump the refcount and take hold of that resource from the
> Kernel agent point of view...
> >
> > ... but Peng's proposed patch will broke immediately the moment
> you have instead a system with an SCMI-capable bootloader that
> instead left the resource ON for the Kernel to inherit, since the kernel
> will now forcibly see this anyway as OFF, and so you wont be ever be
> able to switch that resource REALLY OFF in the future, if ever needed,
> because the bootloader/Kernel agent will never see it as ON in genPD,
> since, at least in the genPD case, AFAICS correct me if wrong, there is
> no callback to peek at the real state later on:
> > so, after the initialization value has been chosen at genpd_init time,
> genPd subsystem maintains the PD state on its own based on the
> issued ON/OFF genPD requests, so your forced-initial-OFF-state will be,
> in this specific alternative scenario, wrong and forever.
> > [
> > [RV] SCMI-capable bootloader and Linux should be the same logical
> machine (different agents). And the platform maintains the state per
> logical machine. So if Linux tries to power off a state that was left
> powered ON by the bootloader it should bbe able to.
> 
> In the SCMI world there are agents, i.e. clients issuing requests to the
> platform AND the platform identifies such agents from the channel
> they speak from. Not sure what you mean by logical machine.
> 
> In the case of an SCMI-aware bootloader like UBoot, that dies and
> relinquishes all the resources to the Kernel during the boot, that means
> that the Kernel should be configured to simply re-use the same SCMI
> channels as UBoot, so that it will be seen as the same agent
> (transparently) from the platform: in such a scenario the Kernel will
> transparently inherit all the per-agent current interrnal state...
> 
> ...IOW if an SCMI/Uboot was holding res_X, the Kernel will result as
> holding the same res_X "by inheritance" from the platform point of
> view, since the Kernel would have assumed the role of the UBoot agent
> from the platfom point of view, since it is using the same channels as
> UBoot.
> 
> Why you should consider Uboot a diffrent agent, if it runs in NS-world
> too and does not survive the Kernel boot ?

Per my understanding, Kernel itself should not count on bootloader
settings, kernel should try to configure all it could do as much as
possible, kernel is not aware what bootloader might be used or
what settings bootloader could set or run in what state.

Regards,
Peng.

> 
> This, at least, is my understannding after bunch of past talk with ATG.
> 
> And this is the scenario that I fear would fail with Peng's patch.
> 
> Thanks,
> Cristian

Re: [EXT] Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Dan Carpenter]

Hi Ranjani,

Your email is pretty hard to read when we're looking at the email
archive or on email clients like mutt also.
https://lore.kernel.org/all/PA4PR04MB9485CC9D9925BB5D23EA629092192@PA4PR04MB9485.eurprd04.prod.outlook.com/

regards,
dan carpenter

Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize state as off

[Cristian Marussi]

On Mon, Jan 13, 2025 at 01:49:12PM +0000, Sudeep Holla wrote:
> On Mon, Jan 13, 2025 at 11:37:23AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH] pmdomain: arm: scmi_pm_domain: Initialize
> > > state as off
> > >
> > > On Fri, Jan 10, 2025 at 02:13:46PM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > Per ARM SCMI Spec DEN0056E, page 16, "The platform may disable a
> > > > resource if no agent has requested to use that resource."
> > > >

Hi,

not really a short mail this one...

> > >
> > > True, but ...
> > >
> > > > Linux Kernel should not rely on a state that it has not requested, so
> > > > make state as off during initialization.
> > > >
> > >
> > > IIUC, this was done to avoid any transitions if the bootloader like U-
> > > Boot has turned on the resource and OS can just rely on that stay.
> >
> > But if it is not U-Boot turned it on?
> 
> Not sure if I understand what exactly you mean by that.
> 
> > Or U-Boot is in a separate agent?
> >
> 
> No, it will be same as OS for the SCMI platform/agent as they use/share the
> same transport. It is hard to distinguish between them.
> 

Exactly, if U-Boot is SCMI capable, its requests will have been accounted,
lets say, as Agent_X, but when the Kernel springs to life it will take over
the role and the SAME channel used by the defunct UBoot, so as to appear as
being effectively the same Agent_X to the platform...

...the tricky part is what happens when, instead, UBoot is NOT an SCMI agent
and it does leave some resources ON (say some regulator needed while setting up
the screen...) with the intent of NOT having those turned off by the Kernel ?
(BUT with such actions not tracked by the platform, not being an SCMI agent)

To make the matter worse, all of this sort of "resource-state-alignment-issue"
between bootloader, Kernel or other independent agents, greatly depends on how
the state-get operations were implemented.

IOW, since some resources could be shared between agents (clocks, PDs), a
set-state on such resources is supposed to be "harmonized" by the platform,
so that, say, res_X will be enabled physically only when the first agent
acquires it, and res_X will be finally disabled only when the last agent
relinquishes it...

....you certainly dont want that after agent_A have issued a set_ON(res_X),
another agent_B issuing a set_OFF(res_X) can cause that same shared res_X to
be effectively switched off while still used by agent_A...in such a case agent_B
should receive an OK but res_X should anyway effectively stay ON until also
agent_A has relinquished it too...

...now the question is...what state would you expect to read-back from a
get_state operation on such shared resources ? the real/physical one OR the
virtual/per_agent state ?

...IOW in the above example, after agent_B successfully issued a set_OFF(res_X),
on the shared res_X (which indeed did NOT really caused sharted res_X to be
turned OFF) what will agent_B should read back from a get_state(res_X):

	OFF (virtual-view) ? ... or ON (physical view) ?

This platform "states-view" behaviour was long debated and at the end
AFAICR it has been left by the spec as IMPDEF, so each platform can
decide what to return...physical vs virtual state...and that means that,
whatever we do, Kernel side we should cope with 4 possible scenarios

- Uboot NON-SCMI / physical GET_STATE platform-impl
- Uboot NON-SCMI / virtual GET_STATE platform-impl
- Uboot SCMI / physical GET_STATE platform-impl
- Uboot SCMI / virtual GET_STATE platform-impl

...and in all of these scenarios we should additionally consider what
happens when the kernel wants or want-not to keep a resource in the state,
that it has found it (as queried with a get_state (physical or virtual))

IOW, as an example, what if:

I have an UBOOT/NON_SCMI that leaves res_X OFF from its point-of-view at
the end of its life, BUT res_X is also claimed as ON by another SCMI agent_Z
at the same time, and the platform implements physical-real-views, so that
Kernel will see anyway res_X as ON....what should happen, in this context,
if the Kernel wants to keep res_X ON ?

OR what if:

I have an UBOOT/SCMI agent_X that leaves res_X ON from its point-of-view at
the end of its life, BUT res_X is also claimed as ON by another SCMI agent_Z
at the same time, and the platform implements virtual-per-agent-views,
so that Kernel will see res_X as ON....what will happen, in this
context, if the Kernel wants to turn res_X OFF ? AND what happens
instead if, in this scenario, we have additionally the currently proposed patch
applied (that forcibly would makke the Kernel think that res_X is OFF) ?

All of these scenario should be considered, and I have indeed an even
longer (:P) mail currently brewing on my side to try to layout all
possibilities...

...next days once it is more refined I will post it.... you've been warned ! :P

Thanks
Cristian