Changes in v4:
- Add missing s-o-b line and reviewed by in patch 1
- Address ulf's comments for error handling in patch 2
Changes in v3:
- Add new change to provide helper function dt_idle_pd_remove_topology()
- Address ulf's comments for error handling
- Add reviewed by ulf for devicetree change
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 (3):
cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
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 | 39 ++++-------
drivers/cpuidle/dt_idle_genpd.c | 24 +++++++
drivers/cpuidle/dt_idle_genpd.h | 7 ++
4 files changed, 117 insertions(+), 51 deletions(-)
--
2.17.1
Genpd parent and child domain topology created using dt_idle_pd_init_topology()
needs to be removed during error cases.
Add new helper function dt_idle_pd_remove_topology() for same.
Reviewed-by: Ulf Hanssson <[email protected]>
Signed-off-by: Maulik Shah <[email protected]>
---
drivers/cpuidle/dt_idle_genpd.c | 24 ++++++++++++++++++++++++
drivers/cpuidle/dt_idle_genpd.h | 7 +++++++
2 files changed, 31 insertions(+)
diff --git a/drivers/cpuidle/dt_idle_genpd.c b/drivers/cpuidle/dt_idle_genpd.c
index b37165514d4e..1af63c189039 100644
--- a/drivers/cpuidle/dt_idle_genpd.c
+++ b/drivers/cpuidle/dt_idle_genpd.c
@@ -152,6 +152,30 @@ int dt_idle_pd_init_topology(struct device_node *np)
return 0;
}
+int dt_idle_pd_remove_topology(struct device_node *np)
+{
+ struct device_node *node;
+ struct of_phandle_args child, parent;
+ int ret;
+
+ for_each_child_of_node(np, node) {
+ if (of_parse_phandle_with_args(node, "power-domains",
+ "#power-domain-cells", 0, &parent))
+ continue;
+
+ child.np = node;
+ child.args_count = 0;
+ ret = of_genpd_remove_subdomain(&parent, &child);
+ of_node_put(parent.np);
+ if (ret) {
+ of_node_put(node);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
struct device *dt_idle_attach_cpu(int cpu, const char *name)
{
struct device *dev;
diff --git a/drivers/cpuidle/dt_idle_genpd.h b/drivers/cpuidle/dt_idle_genpd.h
index a95483d08a02..3be1f70f55b5 100644
--- a/drivers/cpuidle/dt_idle_genpd.h
+++ b/drivers/cpuidle/dt_idle_genpd.h
@@ -14,6 +14,8 @@ struct generic_pm_domain *dt_idle_pd_alloc(struct device_node *np,
int dt_idle_pd_init_topology(struct device_node *np);
+int dt_idle_pd_remove_topology(struct device_node *np);
+
struct device *dt_idle_attach_cpu(int cpu, const char *name);
void dt_idle_detach_cpu(struct device *dev);
@@ -36,6 +38,11 @@ static inline int dt_idle_pd_init_topology(struct device_node *np)
return 0;
}
+static inline int dt_idle_pd_remove_topology(struct device_node *np)
+{
+ return 0;
+}
+
static inline struct device *dt_idle_attach_cpu(int cpu, const char *name)
{
return NULL;
--
2.17.1
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 <[email protected]>
---
drivers/cpuidle/cpuidle-psci-domain.c | 39 +++++++++------------------
1 file changed, 13 insertions(+), 26 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index c2d6d9c3c930..b88af1262f1a 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.
@@ -170,33 +153,37 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
continue;
ret = psci_pd_init(node, use_osi);
- if (ret)
- goto put_node;
+ if (ret) {
+ of_node_put(node);
+ goto exit;
+ }
pd_count++;
}
/* Bail out if not using the hierarchical CPU topology. */
if (!pd_count)
- goto no_pd;
+ return 0;
/* 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;
-put_node:
- of_node_put(node);
remove_pd:
+ dt_idle_pd_remove_topology(np);
psci_pd_remove();
+exit:
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
Add power-domains for cpuidle states to use psci os-initiated idle states.
Cc: [email protected]
Reviewed-by: Ulf Hansson <[email protected]>
Signed-off-by: Maulik Shah <[email protected]>
---
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 31728f461422..88e234b4dee3 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 {
@@ -5283,6 +5330,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
On Mon, 24 Apr 2023 at 13:10, Maulik Shah <[email protected]> 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 <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
Bjorn, can you pick up this series for 6.4-rc[n]? Moreover, I think it
would be a good idea to tag both patch 1 and patch 2 for stable
kernels.
Kind regards
Uffe
> ---
> drivers/cpuidle/cpuidle-psci-domain.c | 39 +++++++++------------------
> 1 file changed, 13 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index c2d6d9c3c930..b88af1262f1a 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.
> @@ -170,33 +153,37 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
> continue;
>
> ret = psci_pd_init(node, use_osi);
> - if (ret)
> - goto put_node;
> + if (ret) {
> + of_node_put(node);
> + goto exit;
> + }
>
> pd_count++;
> }
>
> /* Bail out if not using the hierarchical CPU topology. */
> if (!pd_count)
> - goto no_pd;
> + return 0;
>
> /* 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;
>
> -put_node:
> - of_node_put(node);
> remove_pd:
> + dt_idle_pd_remove_topology(np);
> psci_pd_remove();
> +exit:
> 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
>
On Mon, 24 Apr 2023 at 13:09, Maulik Shah <[email protected]> wrote:
>
> Changes in v4:
> - Add missing s-o-b line and reviewed by in patch 1
> - Address ulf's comments for error handling in patch 2
>
> Changes in v3:
> - Add new change to provide helper function dt_idle_pd_remove_topology()
> - Address ulf's comments for error handling
> - Add reviewed by ulf for devicetree change
>
> 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 (3):
> cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> 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 | 39 ++++-------
> drivers/cpuidle/dt_idle_genpd.c | 24 +++++++
> drivers/cpuidle/dt_idle_genpd.h | 7 ++
> 4 files changed, 117 insertions(+), 51 deletions(-)
>
Looks like this series has not been queued up yet. Note that patch1
and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
DTS change) is dependent on patch 1 and patch2.
Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
Bjorn, is that okay for you?
Kind regards
Uffe
On Wed, May 24, 2023 at 11:56:28AM +0200, Ulf Hansson wrote:
> On Mon, 24 Apr 2023 at 13:09, Maulik Shah <[email protected]> wrote:
> >
> > Changes in v4:
> > - Add missing s-o-b line and reviewed by in patch 1
> > - Address ulf's comments for error handling in patch 2
> >
> > Changes in v3:
> > - Add new change to provide helper function dt_idle_pd_remove_topology()
> > - Address ulf's comments for error handling
> > - Add reviewed by ulf for devicetree change
> >
> > 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 (3):
> > cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> > 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 | 39 ++++-------
> > drivers/cpuidle/dt_idle_genpd.c | 24 +++++++
> > drivers/cpuidle/dt_idle_genpd.h | 7 ++
> > 4 files changed, 117 insertions(+), 51 deletions(-)
> >
>
> Looks like this series has not been queued up yet. Note that patch1
> and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> DTS change) is dependent on patch 1 and patch2.
>
> Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> Bjorn, is that okay for you?
>
Sorry, this fell between the chairs after you pointed me to it...
I can certainly pick the 3 patches through my tree, but are they fixing
any current regressions, or is it just that we need the first two
patches to land before the 3rd patch?
I also presume the 3rd patch is only needed when paired with the new
ATF?
Regards,
Bjorn
On Thu, 25 May 2023 at 04:41, Bjorn Andersson <[email protected]> wrote:
>
> On Wed, May 24, 2023 at 11:56:28AM +0200, Ulf Hansson wrote:
> > On Mon, 24 Apr 2023 at 13:09, Maulik Shah <[email protected]> wrote:
> > >
> > > Changes in v4:
> > > - Add missing s-o-b line and reviewed by in patch 1
> > > - Address ulf's comments for error handling in patch 2
> > >
> > > Changes in v3:
> > > - Add new change to provide helper function dt_idle_pd_remove_topology()
> > > - Address ulf's comments for error handling
> > > - Add reviewed by ulf for devicetree change
> > >
> > > 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 (3):
> > > cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> > > 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 | 39 ++++-------
> > > drivers/cpuidle/dt_idle_genpd.c | 24 +++++++
> > > drivers/cpuidle/dt_idle_genpd.h | 7 ++
> > > 4 files changed, 117 insertions(+), 51 deletions(-)
> > >
> >
> > Looks like this series has not been queued up yet. Note that patch1
> > and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> > DTS change) is dependent on patch 1 and patch2.
> >
> > Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> > Bjorn, is that okay for you?
> >
>
> Sorry, this fell between the chairs after you pointed me to it...
>
> I can certainly pick the 3 patches through my tree, but are they fixing
> any current regressions, or is it just that we need the first two
> patches to land before the 3rd patch?
I am not aware of any current regressions.
>
> I also presume the 3rd patch is only needed when paired with the new
> ATF?
Patch3 is beneficial to use with a new TF-A, but works with an old
TF-A too. Anyway, forget what I said about patch3 earlier, as that was
just not the complete information.
The problem is that we can't be using a new TF-A (supporting both PSCI
OSI and PC mode) without patch1 and patch2, unless we are using
patch3.
Thus, I strongly suggest we tag patch1 and patch2 for stable kernels,
to avoid any potential conflicts of TF-A versions that may be used.
>
> Regards,
> Bjorn
Did that make more sense?
Br
Uffe
On Mon, May 29, 2023 at 10:58:23AM +0200, Ulf Hansson wrote:
> On Thu, 25 May 2023 at 04:41, Bjorn Andersson <[email protected]> wrote:
> >
> > On Wed, May 24, 2023 at 11:56:28AM +0200, Ulf Hansson wrote:
> > > On Mon, 24 Apr 2023 at 13:09, Maulik Shah <[email protected]> wrote:
> > > >
> > > > Changes in v4:
> > > > - Add missing s-o-b line and reviewed by in patch 1
> > > > - Address ulf's comments for error handling in patch 2
> > > >
> > > > Changes in v3:
> > > > - Add new change to provide helper function dt_idle_pd_remove_topology()
> > > > - Address ulf's comments for error handling
> > > > - Add reviewed by ulf for devicetree change
> > > >
> > > > 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 (3):
> > > > cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> > > > 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 | 39 ++++-------
> > > > drivers/cpuidle/dt_idle_genpd.c | 24 +++++++
> > > > drivers/cpuidle/dt_idle_genpd.h | 7 ++
> > > > 4 files changed, 117 insertions(+), 51 deletions(-)
> > > >
> > >
> > > Looks like this series has not been queued up yet. Note that patch1
> > > and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> > > DTS change) is dependent on patch 1 and patch2.
> > >
> > > Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> > > Bjorn, is that okay for you?
> > >
> >
> > Sorry, this fell between the chairs after you pointed me to it...
> >
> > I can certainly pick the 3 patches through my tree, but are they fixing
> > any current regressions, or is it just that we need the first two
> > patches to land before the 3rd patch?
>
> I am not aware of any current regressions.
>
Okay, that confirms my understanding. So not -rc material.
> >
> > I also presume the 3rd patch is only needed when paired with the new
> > ATF?
>
> Patch3 is beneficial to use with a new TF-A, but works with an old
> TF-A too. Anyway, forget what I said about patch3 earlier, as that was
> just not the complete information.
>
> The problem is that we can't be using a new TF-A (supporting both PSCI
> OSI and PC mode) without patch1 and patch2, unless we are using
> patch3.
>
> Thus, I strongly suggest we tag patch1 and patch2 for stable kernels,
> to avoid any potential conflicts of TF-A versions that may be used.
>
So you're suggesting that I pick them for v6.5 and add a Cc: stable?
An alternative would be that you take the cpuidle patches for v6.4-rc
and I pick the dt for v6.5 - given that the cpuidle patches actually
resolves a problem, while the dts just introduces "new functionality".
> >
> > Regards,
> > Bjorn
>
> Did that make more sense?
>
Yes, it's getting there :)
Thanks,
Bjorn
> Br
> Uffe
+ Rafael
On Mon, 29 May 2023 at 18:05, Bjorn Andersson <[email protected]> wrote:
>
> On Mon, May 29, 2023 at 10:58:23AM +0200, Ulf Hansson wrote:
> > On Thu, 25 May 2023 at 04:41, Bjorn Andersson <[email protected]> wrote:
> > >
> > > On Wed, May 24, 2023 at 11:56:28AM +0200, Ulf Hansson wrote:
> > > > On Mon, 24 Apr 2023 at 13:09, Maulik Shah <[email protected]> wrote:
> > > > >
> > > > > Changes in v4:
> > > > > - Add missing s-o-b line and reviewed by in patch 1
> > > > > - Address ulf's comments for error handling in patch 2
> > > > >
> > > > > Changes in v3:
> > > > > - Add new change to provide helper function dt_idle_pd_remove_topology()
> > > > > - Address ulf's comments for error handling
> > > > > - Add reviewed by ulf for devicetree change
> > > > >
> > > > > 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 (3):
> > > > > cpuidle: dt_idle_genpd: Add helper function to remove genpd topology
> > > > > 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 | 39 ++++-------
> > > > > drivers/cpuidle/dt_idle_genpd.c | 24 +++++++
> > > > > drivers/cpuidle/dt_idle_genpd.h | 7 ++
> > > > > 4 files changed, 117 insertions(+), 51 deletions(-)
> > > > >
> > > >
> > > > Looks like this series has not been queued up yet. Note that patch1
> > > > and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> > > > DTS change) is dependent on patch 1 and patch2.
> > > >
> > > > Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> > > > Bjorn, is that okay for you?
> > > >
> > >
> > > Sorry, this fell between the chairs after you pointed me to it...
> > >
> > > I can certainly pick the 3 patches through my tree, but are they fixing
> > > any current regressions, or is it just that we need the first two
> > > patches to land before the 3rd patch?
> >
> > I am not aware of any current regressions.
> >
>
> Okay, that confirms my understanding. So not -rc material.
>
> > >
> > > I also presume the 3rd patch is only needed when paired with the new
> > > ATF?
> >
> > Patch3 is beneficial to use with a new TF-A, but works with an old
> > TF-A too. Anyway, forget what I said about patch3 earlier, as that was
> > just not the complete information.
> >
> > The problem is that we can't be using a new TF-A (supporting both PSCI
> > OSI and PC mode) without patch1 and patch2, unless we are using
> > patch3.
> >
> > Thus, I strongly suggest we tag patch1 and patch2 for stable kernels,
> > to avoid any potential conflicts of TF-A versions that may be used.
> >
>
> So you're suggesting that I pick them for v6.5 and add a Cc: stable?
>
> An alternative would be that you take the cpuidle patches for v6.4-rc
> and I pick the dt for v6.5 - given that the cpuidle patches actually
> resolves a problem, while the dts just introduces "new functionality".
Right, that's probably the best option. Although I don't have a tree
to take these patches through, let's ask Rafael if he can help with
this.
Rafael, can you pick patch 1 and patch 2 from $subject series for
v6.4-rc and tag them for stable? Then Bjorn can pick patch3 for v6.5.
Kind regards
Uffe
[...]
> > > > >
> > > > > Looks like this series has not been queued up yet. Note that patch1
> > > > > and patch2 are needed for stable kernels too. Moreover, patch3 (Qcom
> > > > > DTS change) is dependent on patch 1 and patch2.
> > > > >
> > > > > Therefore I suggest Bjorn to pick this up via the Qcom SoC tree.
> > > > > Bjorn, is that okay for you?
> > > > >
> > > >
> > > > Sorry, this fell between the chairs after you pointed me to it...
> > > >
> > > > I can certainly pick the 3 patches through my tree, but are they fixing
> > > > any current regressions, or is it just that we need the first two
> > > > patches to land before the 3rd patch?
> > >
> > > I am not aware of any current regressions.
> > >
> >
> > Okay, that confirms my understanding. So not -rc material.
> >
> > > >
> > > > I also presume the 3rd patch is only needed when paired with the new
> > > > ATF?
> > >
> > > Patch3 is beneficial to use with a new TF-A, but works with an old
> > > TF-A too. Anyway, forget what I said about patch3 earlier, as that was
> > > just not the complete information.
> > >
> > > The problem is that we can't be using a new TF-A (supporting both PSCI
> > > OSI and PC mode) without patch1 and patch2, unless we are using
> > > patch3.
> > >
> > > Thus, I strongly suggest we tag patch1 and patch2 for stable kernels,
> > > to avoid any potential conflicts of TF-A versions that may be used.
> > >
> >
> > So you're suggesting that I pick them for v6.5 and add a Cc: stable?
> >
> > An alternative would be that you take the cpuidle patches for v6.4-rc
> > and I pick the dt for v6.5 - given that the cpuidle patches actually
> > resolves a problem, while the dts just introduces "new functionality".
>
> Right, that's probably the best option. Although I don't have a tree
> to take these patches through, let's ask Rafael if he can help with
> this.
>
> Rafael, can you pick patch 1 and patch 2 from $subject series for
> v6.4-rc and tag them for stable? Then Bjorn can pick patch3 for v6.5.
Rafael, Bjorn, sorry for nagging about this series. Can you please
help to pick it up?
Kind regards
Uffe