[PATCH v2 0/2] Use PSCI OS initiated mode for sc7280

[PATCH v2 0/2] Use PSCI OS initiated mode for sc7280

[Maulik Shah]

Changes in v2:
- Add new change to Move enabling OSI mode after power domains creation
- Fix compatible string to domains-idle-states for cluster idle state.
- Update cover letter with some more details on OSI and PC mode comparision

The dependency [2] is now merged in trustedfirmware project.

Stats comparision between OSI and PC mode are captured at [3] with usecase
details, where during multiple CPUs online the residency in cluster idle
state is better with OSI and also inline with single CPU mode. In PC mode
with multiple CPUs cluster idle state residency is dropping compare to
single CPU mode.

Recording of this meeting is also available at [4].

This change adds power-domains for cpuidle states to use PSCI OS
initiated mode for sc7280.

This change depends on external project changes [1] & [2] which are
under review/discussion to add PSCI os-initiated support in Arm Trusted
Firmware.

I can update here once the dependency are in and change is ready to merge.

[1] https://review.trustedfirmware.org/q/topic:psci-osi
[2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487
[3] https://www.trustedfirmware.org/docs/PSCI-OS-initiated.pdf
[4] https://www.trustedfirmware.org/meetings/tf-a-technical-forum

Maulik Shah (2):
  cpuidle: psci: Move enabling OSI mode after power domains creation
  arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

 arch/arm64/boot/dts/qcom/sc7280.dtsi  | 98 ++++++++++++++++++++-------
 drivers/cpuidle/cpuidle-psci-domain.c | 29 ++------
 2 files changed, 80 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Maulik Shah]

A switch from OSI to PC mode is only possible if all CPUs other than the
calling one are OFF, either through a call to CPU_OFF or not yet booted.

Currently OSI mode is enabled before power domains are created. In cases
where CPUidle states are not using hierarchical CPU topology the bail out
path tries to switch back to PC mode which gets denied by firmware since
other CPUs are online at this point and creates inconsistent state as
firmware is in OSI mode and Linux in PC mode.

This change moves enabling OSI mode after power domains are created,
this would makes sure that hierarchical CPU topology is used before
switching firmware to OSI mode.

Fixes: 70c179b49870 ("cpuidle: psci: Allow PM domain to be initialized even if no OSI mode")
Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
---
 drivers/cpuidle/cpuidle-psci-domain.c | 29 +++++++--------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 11316c3b14ca..d81f6ae35002 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -120,20 +120,6 @@ static void psci_pd_remove(void)
 	}
 }
 
-static bool psci_pd_try_set_osi_mode(void)
-{
-	int ret;
-
-	if (!psci_has_osi_support())
-		return false;
-
-	ret = psci_set_osi_mode(true);
-	if (ret)
-		return false;
-
-	return true;
-}
-
 static void psci_cpuidle_domain_sync_state(struct device *dev)
 {
 	/*
@@ -152,15 +138,12 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *node;
-	bool use_osi;
+	bool use_osi = psci_has_osi_support();
 	int ret = 0, pd_count = 0;
 
 	if (!np)
 		return -ENODEV;
 
-	/* If OSI mode is supported, let's try to enable it. */
-	use_osi = psci_pd_try_set_osi_mode();
-
 	/*
 	 * Parse child nodes for the "#power-domain-cells" property and
 	 * initialize a genpd/genpd-of-provider pair when it's found.
@@ -178,13 +161,18 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
 
 	/* Bail out if not using the hierarchical CPU topology. */
 	if (!pd_count)
-		goto no_pd;
+		goto remove_pd;
 
 	/* Link genpd masters/subdomains to model the CPU topology. */
 	ret = dt_idle_pd_init_topology(np);
 	if (ret)
 		goto remove_pd;
 
+	/* let's try to enable OSI. */
+	ret = psci_set_osi_mode(use_osi);
+	if (ret)
+		goto remove_pd;
+
 	pr_info("Initialized CPU PM domain topology using %s mode\n",
 		use_osi ? "OSI" : "PC");
 	return 0;
@@ -194,9 +182,6 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
 remove_pd:
 	psci_pd_remove();
 	pr_err("failed to create CPU PM domains ret=%d\n", ret);
-no_pd:
-	if (use_osi)
-		psci_set_osi_mode(false);
 	return ret;
 }
 
-- 
2.17.1

[PATCH v2 2/2] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Maulik Shah]

Add power-domains for cpuidle states to use psci os-initiated idle states.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 98 +++++++++++++++++++++-------
 1 file changed, 73 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 5e6f9f441f1a..1a232eb4dde6 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -170,9 +170,8 @@
 			reg = <0x0 0x0>;
 			clocks = <&cpufreq_hw 0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD0>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_0>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -196,9 +195,8 @@
 			reg = <0x0 0x100>;
 			clocks = <&cpufreq_hw 0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD1>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_100>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -218,9 +216,8 @@
 			reg = <0x0 0x200>;
 			clocks = <&cpufreq_hw 0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD2>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_200>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -240,9 +237,8 @@
 			reg = <0x0 0x300>;
 			clocks = <&cpufreq_hw 0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD3>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_300>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -262,9 +258,8 @@
 			reg = <0x0 0x400>;
 			clocks = <&cpufreq_hw 1>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD4>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_400>;
 			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -284,9 +279,8 @@
 			reg = <0x0 0x500>;
 			clocks = <&cpufreq_hw 1>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD5>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_500>;
 			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -306,9 +300,8 @@
 			reg = <0x0 0x600>;
 			clocks = <&cpufreq_hw 1>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD6>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_600>;
 			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -328,9 +321,8 @@
 			reg = <0x0 0x700>;
 			clocks = <&cpufreq_hw 2>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD7>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_700>;
 			operating-points-v2 = <&cpu7_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -422,9 +414,11 @@
 				min-residency-us = <5555>;
 				local-timer-stop;
 			};
+		};
 
+		domain-idle-states {
 			CLUSTER_SLEEP_0: cluster-sleep-0 {
-				compatible = "arm,idle-state";
+				compatible = "domain-idle-state";
 				idle-state-name = "cluster-power-down";
 				arm,psci-suspend-param = <0x40003444>;
 				entry-latency-us = <3263>;
@@ -790,6 +784,59 @@
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD0: cpu0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD1: cpu1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD2: cpu2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD3: cpu3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD4: cpu4 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD5: cpu5 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD6: cpu6 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD7: cpu7 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CLUSTER_PD: cpu-cluster0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
+		};
 	};
 
 	qspi_opp_table: opp-table-qspi {
@@ -5280,6 +5327,7 @@
 					  <SLEEP_TCS   3>,
 					  <WAKE_TCS    3>,
 					  <CONTROL_TCS 1>;
+			power-domains = <&CLUSTER_PD>;
 
 			apps_bcm_voter: bcm-voter {
 				compatible = "qcom,bcm-voter";
-- 
2.17.1

Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Sudeep Holla]

On Thu, Mar 30, 2023 at 02:12:49PM +0530, Maulik Shah wrote:
> A switch from OSI to PC mode is only possible if all CPUs other than the
> calling one are OFF, either through a call to CPU_OFF or not yet booted.
>

As per the spec, all cores are in one of the following states:
 - Running
 - OFF, either through a call to CPU_OFF or not yet booted
 - Suspended, through a call to CPU_DEFAULT_SUSPEND

Better to provide full information.

> Currently OSI mode is enabled before power domains are created. In cases
> where CPUidle states are not using hierarchical CPU topology the bail out
> path tries to switch back to PC mode which gets denied by firmware since
> other CPUs are online at this point and creates inconsistent state as
> firmware is in OSI mode and Linux in PC mode.
>

OK what is the issue if the other cores are online ? As long as they are
running, it is allowed in the spec, so your statement is incorrect.

Is CPUidle enabled before setting the OSI mode. I see only that can cause
issue as we don't use CPU_DEFAULT_SUSPEND. If idle is not yet enabled, it
shouldn't be a problem.

-- 
Regards,
Sudeep

Re: [PATCH v2 0/2] Use PSCI OS initiated mode for sc7280

[Sudeep Holla]

On Thu, Mar 30, 2023 at 02:12:48PM +0530, Maulik Shah wrote:
> Changes in v2:
> - Add new change to Move enabling OSI mode after power domains creation
> - Fix compatible string to domains-idle-states for cluster idle state.
> - Update cover letter with some more details on OSI and PC mode comparision
> 
> The dependency [2] is now merged in trustedfirmware project.
> 
> Stats comparision between OSI and PC mode are captured at [3] with usecase
> details, where during multiple CPUs online the residency in cluster idle
> state is better with OSI and also inline with single CPU mode. In PC mode
> with multiple CPUs cluster idle state residency is dropping compare to
> single CPU mode.
> 
> Recording of this meeting is also available at [4].
> 
> This change adds power-domains for cpuidle states to use PSCI OS
> initiated mode for sc7280.
> 
> This change depends on external project changes [1] & [2] which are
> under review/discussion to add PSCI os-initiated support in Arm Trusted
> Firmware.
>

I will wait for your response to understand the issue in 1/2. If it is
firmware issue, I prefer we fix it in the firmware and no quirks or work
around for this in the kernel while it is still fresh out of oven.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Ulf Hansson]

On Thu, 30 Mar 2023 at 11:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Mar 30, 2023 at 02:12:49PM +0530, Maulik Shah wrote:
> > A switch from OSI to PC mode is only possible if all CPUs other than the
> > calling one are OFF, either through a call to CPU_OFF or not yet booted.
> >
>
> As per the spec, all cores are in one of the following states:
>  - Running
>  - OFF, either through a call to CPU_OFF or not yet booted
>  - Suspended, through a call to CPU_DEFAULT_SUSPEND
>
> Better to provide full information.
>
> > Currently OSI mode is enabled before power domains are created. In cases
> > where CPUidle states are not using hierarchical CPU topology the bail out
> > path tries to switch back to PC mode which gets denied by firmware since
> > other CPUs are online at this point and creates inconsistent state as
> > firmware is in OSI mode and Linux in PC mode.
> >
>
> OK what is the issue if the other cores are online ? As long as they are
> running, it is allowed in the spec, so your statement is incorrect.
>
> Is CPUidle enabled before setting the OSI mode. I see only that can cause
> issue as we don't use CPU_DEFAULT_SUSPEND. If idle is not yet enabled, it
> shouldn't be a problem.

Sudeep, you may very well be correct here. Nevertheless, it looks like
the current public TF-A implementation doesn't work exactly like this,
as it reports an error in Maulik's case. We should fix it too, I
think.

Although, to me it doesn't really matter as I think $subject patch
makes sense anyway. It's always nice to simplify code when it's
possible.

Note also that, before commit 70c179b49870 ("cpuidle: psci: Allow PM
domain to be initialized even if no OSI mode"), it made sense to call
psci_pd_try_set_osi_mode() before creating the genpds, but beyond that
it doesn't really matter anymore.

Kind regards
Uffe

Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Ulf Hansson]

On Thu, 30 Mar 2023 at 10:43, Maulik Shah <quic_mkshah@quicinc.com> wrote:
>
> A switch from OSI to PC mode is only possible if all CPUs other than the
> calling one are OFF, either through a call to CPU_OFF or not yet booted.
>
> Currently OSI mode is enabled before power domains are created. In cases
> where CPUidle states are not using hierarchical CPU topology the bail out
> path tries to switch back to PC mode which gets denied by firmware since
> other CPUs are online at this point and creates inconsistent state as
> firmware is in OSI mode and Linux in PC mode.
>
> This change moves enabling OSI mode after power domains are created,
> this would makes sure that hierarchical CPU topology is used before
> switching firmware to OSI mode.
>
> Fixes: 70c179b49870 ("cpuidle: psci: Allow PM domain to be initialized even if no OSI mode")
> Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 29 +++++++--------------------
>  1 file changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 11316c3b14ca..d81f6ae35002 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -120,20 +120,6 @@ static void psci_pd_remove(void)
>         }
>  }
>
> -static bool psci_pd_try_set_osi_mode(void)
> -{
> -       int ret;
> -
> -       if (!psci_has_osi_support())
> -               return false;
> -
> -       ret = psci_set_osi_mode(true);
> -       if (ret)
> -               return false;
> -
> -       return true;
> -}
> -
>  static void psci_cpuidle_domain_sync_state(struct device *dev)
>  {
>         /*
> @@ -152,15 +138,12 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
>  {
>         struct device_node *np = pdev->dev.of_node;
>         struct device_node *node;
> -       bool use_osi;
> +       bool use_osi = psci_has_osi_support();
>         int ret = 0, pd_count = 0;
>
>         if (!np)
>                 return -ENODEV;
>
> -       /* If OSI mode is supported, let's try to enable it. */
> -       use_osi = psci_pd_try_set_osi_mode();
> -
>         /*
>          * Parse child nodes for the "#power-domain-cells" property and
>          * initialize a genpd/genpd-of-provider pair when it's found.
> @@ -178,13 +161,18 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
>
>         /* Bail out if not using the hierarchical CPU topology. */
>         if (!pd_count)
> -               goto no_pd;
> +               goto remove_pd;
>
>         /* Link genpd masters/subdomains to model the CPU topology. */
>         ret = dt_idle_pd_init_topology(np);
>         if (ret)
>                 goto remove_pd;
>
> +       /* let's try to enable OSI. */
> +       ret = psci_set_osi_mode(use_osi);
> +       if (ret)
> +               goto remove_pd;

If this fails, subdomains that were added in
dt_idle_pd_init_topology() should be removed too. In other words, we
need a add a new helper function in drivers/cpuidle/dt_idle_genpd.c,
that we can call before calling psci_pd_remove() for this error path.

> +
>         pr_info("Initialized CPU PM domain topology using %s mode\n",
>                 use_osi ? "OSI" : "PC");
>         return 0;
> @@ -194,9 +182,6 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
>  remove_pd:
>         psci_pd_remove();
>         pr_err("failed to create CPU PM domains ret=%d\n", ret);
> -no_pd:
> -       if (use_osi)
> -               psci_set_osi_mode(false);
>         return ret;
>  }
>

Other than the above, this looks good to me.

Kind regards
Uffe

Re: [PATCH v2 2/2] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Ulf Hansson]

On Thu, 30 Mar 2023 at 10:43, Maulik Shah <quic_mkshah@quicinc.com> wrote:
>
> Add power-domains for cpuidle states to use psci os-initiated idle states.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 98 +++++++++++++++++++++-------
>  1 file changed, 73 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 5e6f9f441f1a..1a232eb4dde6 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -170,9 +170,8 @@
>                         reg = <0x0 0x0>;
>                         clocks = <&cpufreq_hw 0>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0
> -                                          &LITTLE_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD0>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_0>;
>                         operating-points-v2 = <&cpu0_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -196,9 +195,8 @@
>                         reg = <0x0 0x100>;
>                         clocks = <&cpufreq_hw 0>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0
> -                                          &LITTLE_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD1>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_100>;
>                         operating-points-v2 = <&cpu0_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -218,9 +216,8 @@
>                         reg = <0x0 0x200>;
>                         clocks = <&cpufreq_hw 0>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0
> -                                          &LITTLE_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD2>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_200>;
>                         operating-points-v2 = <&cpu0_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -240,9 +237,8 @@
>                         reg = <0x0 0x300>;
>                         clocks = <&cpufreq_hw 0>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0
> -                                          &LITTLE_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD3>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_300>;
>                         operating-points-v2 = <&cpu0_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -262,9 +258,8 @@
>                         reg = <0x0 0x400>;
>                         clocks = <&cpufreq_hw 1>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&BIG_CPU_SLEEP_0
> -                                          &BIG_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD4>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_400>;
>                         operating-points-v2 = <&cpu4_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -284,9 +279,8 @@
>                         reg = <0x0 0x500>;
>                         clocks = <&cpufreq_hw 1>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&BIG_CPU_SLEEP_0
> -                                          &BIG_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD5>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_500>;
>                         operating-points-v2 = <&cpu4_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -306,9 +300,8 @@
>                         reg = <0x0 0x600>;
>                         clocks = <&cpufreq_hw 1>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&BIG_CPU_SLEEP_0
> -                                          &BIG_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD6>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_600>;
>                         operating-points-v2 = <&cpu4_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -328,9 +321,8 @@
>                         reg = <0x0 0x700>;
>                         clocks = <&cpufreq_hw 2>;
>                         enable-method = "psci";
> -                       cpu-idle-states = <&BIG_CPU_SLEEP_0
> -                                          &BIG_CPU_SLEEP_1
> -                                          &CLUSTER_SLEEP_0>;
> +                       power-domains = <&CPU_PD7>;
> +                       power-domain-names = "psci";
>                         next-level-cache = <&L2_700>;
>                         operating-points-v2 = <&cpu7_opp_table>;
>                         interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
> @@ -422,9 +414,11 @@
>                                 min-residency-us = <5555>;
>                                 local-timer-stop;
>                         };
> +               };
>
> +               domain-idle-states {
>                         CLUSTER_SLEEP_0: cluster-sleep-0 {
> -                               compatible = "arm,idle-state";
> +                               compatible = "domain-idle-state";
>                                 idle-state-name = "cluster-power-down";
>                                 arm,psci-suspend-param = <0x40003444>;
>                                 entry-latency-us = <3263>;
> @@ -790,6 +784,59 @@
>         psci {
>                 compatible = "arm,psci-1.0";
>                 method = "smc";
> +
> +               CPU_PD0: cpu0 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD1: cpu1 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD2: cpu2 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD3: cpu3 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD4: cpu4 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD5: cpu5 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD6: cpu6 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
> +               };
> +
> +               CPU_PD7: cpu7 {
> +                       #power-domain-cells = <0>;
> +                       power-domains = <&CLUSTER_PD>;
> +                       domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
> +               };
> +
> +               CLUSTER_PD: cpu-cluster0 {
> +                       #power-domain-cells = <0>;
> +                       domain-idle-states = <&CLUSTER_SLEEP_0>;
> +               };
>         };
>
>         qspi_opp_table: opp-table-qspi {
> @@ -5280,6 +5327,7 @@
>                                           <SLEEP_TCS   3>,
>                                           <WAKE_TCS    3>,
>                                           <CONTROL_TCS 1>;
> +                       power-domains = <&CLUSTER_PD>;
>
>                         apps_bcm_voter: bcm-voter {
>                                 compatible = "qcom,bcm-voter";
> --
> 2.17.1
>

Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Sudeep Holla]

On Thu, Mar 30, 2023 at 02:06:19PM +0200, Ulf Hansson wrote:
> On Thu, 30 Mar 2023 at 11:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Mar 30, 2023 at 02:12:49PM +0530, Maulik Shah wrote:
> > > A switch from OSI to PC mode is only possible if all CPUs other than the
> > > calling one are OFF, either through a call to CPU_OFF or not yet booted.
> > >
> >
> > As per the spec, all cores are in one of the following states:
> >  - Running
> >  - OFF, either through a call to CPU_OFF or not yet booted
> >  - Suspended, through a call to CPU_DEFAULT_SUSPEND
> >
> > Better to provide full information.
> >
> > > Currently OSI mode is enabled before power domains are created. In cases
> > > where CPUidle states are not using hierarchical CPU topology the bail out
> > > path tries to switch back to PC mode which gets denied by firmware since
> > > other CPUs are online at this point and creates inconsistent state as
> > > firmware is in OSI mode and Linux in PC mode.
> > >
> >
> > OK what is the issue if the other cores are online ? As long as they are
> > running, it is allowed in the spec, so your statement is incorrect.
> >
> > Is CPUidle enabled before setting the OSI mode. I see only that can cause
> > issue as we don't use CPU_DEFAULT_SUSPEND. If idle is not yet enabled, it
> > shouldn't be a problem.
> 
> Sudeep, you may very well be correct here. Nevertheless, it looks like
> the current public TF-A implementation doesn't work exactly like this,
> as it reports an error in Maulik's case. We should fix it too, I
> think.
> 
> Although, to me it doesn't really matter as I think $subject patch
> makes sense anyway. It's always nice to simplify code when it's
> possible.
>

Agreed, I don't have any objection to the change. The wording the message
worried me and wanted to check if there are any other issues because of this.
As such it doesn't look like there are but the commit message needs to be
updated as it gives a different impression/understanding.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Wing Li]

Adding some clarifications.

On Thu, Mar 30, 2023 at 6:13 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Mar 30, 2023 at 02:06:19PM +0200, Ulf Hansson wrote:
> > On Thu, 30 Mar 2023 at 11:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Mar 30, 2023 at 02:12:49PM +0530, Maulik Shah wrote:
> > > > A switch from OSI to PC mode is only possible if all CPUs other than the
> > > > calling one are OFF, either through a call to CPU_OFF or not yet booted.
> > > >
> > >
> > > As per the spec, all cores are in one of the following states:
> > >  - Running
> > >  - OFF, either through a call to CPU_OFF or not yet booted
> > >  - Suspended, through a call to CPU_DEFAULT_SUSPEND
> > >
> > > Better to provide full information.

The spec quoted above only applies when switching from
platform-coordinated mode to OS-initiated mode.

For switching from OS-initiated to platform-coordinated, which is the
case Maulik is referring to, section 5.20.2 of the spec specifies:
"A switch from OS-initiated mode to platform-coordinated mode is only
possible if all cores other than the calling one are OFF, either
through a call to CPU_OFF or not yet booted."

> > >
> > > > Currently OSI mode is enabled before power domains are created. In cases
> > > > where CPUidle states are not using hierarchical CPU topology the bail out
> > > > path tries to switch back to PC mode which gets denied by firmware since
> > > > other CPUs are online at this point and creates inconsistent state as
> > > > firmware is in OSI mode and Linux in PC mode.
> > > >
> > >
> > > OK what is the issue if the other cores are online ? As long as they are
> > > running, it is allowed in the spec, so your statement is incorrect.

The issue here is that the kernel prematurely enabled OSI mode based
on the condition that OSI mode is supported by the firmware, and is
unable to switch back to PC mode in the bail out path if hierarchical
CPU topology isn't used because the other CPUs at this point are now
online.

> > >
> > > Is CPUidle enabled before setting the OSI mode. I see only that can cause
> > > issue as we don't use CPU_DEFAULT_SUSPEND. If idle is not yet enabled, it
> > > shouldn't be a problem.
> >
> > Sudeep, you may very well be correct here. Nevertheless, it looks like
> > the current public TF-A implementation doesn't work exactly like this,
> > as it reports an error in Maulik's case. We should fix it too, I
> > think.
> >
> > Although, to me it doesn't really matter as I think $subject patch
> > makes sense anyway. It's always nice to simplify code when it's
> > possible.
> >
>
> Agreed, I don't have any objection to the change. The wording the message
> worried me and wanted to check if there are any other issues because of this.
> As such it doesn't look like there are but the commit message needs to be
> updated as it gives a different impression/understanding.

I think the commit message is accurate and we can keep it as is.

Best regards,
Wing

>
> --
> Regards,
> Sudeep

Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power domains creation

[Sudeep Holla]

On Thu, Mar 30, 2023 at 06:40:03PM -0700, Wing Li wrote:
> Adding some clarifications.
> 
> On Thu, Mar 30, 2023 at 6:13 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> > On Thu, Mar 30, 2023 at 02:06:19PM +0200, Ulf Hansson wrote:
> > > On Thu, 30 Mar 2023 at 11:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Mar 30, 2023 at 02:12:49PM +0530, Maulik Shah wrote:
> > > > > A switch from OSI to PC mode is only possible if all CPUs other than
> > the
> > > > > calling one are OFF, either through a call to CPU_OFF or not yet
> > booted.
> > > > >
> > > >
> > > > As per the spec, all cores are in one of the following states:
> > > >  - Running
> > > >  - OFF, either through a call to CPU_OFF or not yet booted
> > > >  - Suspended, through a call to CPU_DEFAULT_SUSPEND
> > > >
> > > > Better to provide full information.
> >
> 
> The spec quoted above only applies when switching from platform-coordinated
> mode to OS-initiated mode.
> 
> For switching from OS-initiated to platform-coordinated, which is the case
> Maulik is referring to, section 5.20.2 of the spec specifies:
> "A switch from OS-initiated mode to platform-coordinated mode is only
> possible if all cores other than the calling one are OFF, either through a
> call to CPU_OFF or not yet booted."
> 
>

My bad, I read/imagined it as PC->OSI mode couple of times even though it
is pretty explicit. Sorry for that. And thanks a lot for pointing that out.

-- 
Regards,
Sudeep