| Message ID | 20250110061346.2440772-1-peng.fan@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | pmdomain: arm: scmi_pm_domain: Initialize state as off | expand |
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
> 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
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
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
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
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
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
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
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
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
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
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
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
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
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
> 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
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
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; }