[RESEND v4 0/3] Use PSCI OS initiated mode for sc7280

[RESEND v4 0/3] Use PSCI OS initiated mode for sc7280

[Maulik Shah]

This is resend of v4 with patch1 and patch2 Cced to stable kernel.

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

[RESEND v4 1/3] cpuidle: dt_idle_genpd: Add helper function to remove genpd topology

[Maulik Shah]

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.

Cc: stable@vger.kernel.org
Reviewed-by: Ulf Hanssson <ulf.hansson@linaro.org>
Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
---
 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

[RESEND v4 2/3] 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.

Cc: stable@vger.kernel.org
Fixes: 70c179b49870 ("cpuidle: psci: Allow PM domain to be initialized even if no OSI mode")
Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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

[RESEND v4 3/3] 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
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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 a0e8db8270e7..84208a6c673d 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>,
@@ -198,9 +197,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>,
@@ -221,9 +219,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>,
@@ -244,9 +241,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>,
@@ -267,9 +263,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>,
@@ -290,9 +285,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>,
@@ -313,9 +307,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>,
@@ -336,9 +329,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>,
@@ -431,9 +423,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>;
@@ -811,6 +805,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 {
@@ -5291,6 +5338,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: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Doug Anderson]

Hi,

On Mon, Jul 3, 2023 at 1:56 AM 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
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)

FWIW, I dug up an old sc7280-herobrine board to test some other change
and found it no longer booted. :( I bisected it and this is the change
that breaks it. Specifically, I can make mainline boot with:

git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
domain-idle-states for cluster sleep
git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
power-domains for cpuidle states

(I get an ath11k crash after that, but that's easy to hack out since I
don't need WiFi)

I suppose the two options here are to either:

1. Track the problem down and figure out why the breaks boot and then
fix it. I'm personally not going to track this down, but if someone
wants me to test a patch I can do that.

2. Delete all the herobrine dts files.

So far we've been keeping the herobrine dts files alive because I
thought some graphics folks (Rob, Abhinav, Jessica, for instance) were
still using it. ...but Rob says he hasn't booted his in a while. No
idea if Abhinav and Jessica are using theirs. Any opinions? Is
herobrine hardware support near and dear to anyone these days?


-Doug

Re: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Abhinav Kumar]

Hi Doug

On 3/14/2024 4:20 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jul 3, 2023 at 1:56 AM 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
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)
> 
> FWIW, I dug up an old sc7280-herobrine board to test some other change
> and found it no longer booted. :( I bisected it and this is the change
> that breaks it. Specifically, I can make mainline boot with:
> 
> git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
> domain-idle-states for cluster sleep
> git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
> power-domains for cpuidle states
> 

We noticed that some variants of sc7280 herobrine boards didnt boot but 
some did atleast till linux 6.8 rc-6. I have not tested linux 6.9 rc-1 yet.

We did not narrow down which change broke some of the boards, I can go 
back and confirm if its this one next week.

> (I get an ath11k crash after that, but that's easy to hack out since I
> don't need WiFi)
> 

hmm, wifi worked alright on 6.8 rc-6 for us.

> I suppose the two options here are to either:
> 
> 1. Track the problem down and figure out why the breaks boot and then
> fix it. I'm personally not going to track this down, but if someone
> wants me to test a patch I can do that.
> 

Can Maulik help us do that?

> 2. Delete all the herobrine dts files.
> 
> So far we've been keeping the herobrine dts files alive because I
> thought some graphics folks (Rob, Abhinav, Jessica, for instance) were
> still using it. ...but Rob says he hasn't booted his in a while. No
> idea if Abhinav and Jessica are using theirs. Any opinions? Is
> herobrine hardware support near and dear to anyone these days?
> 

Yes, so we have been using sc7280 herobrine devices even till the last 
cycle and quite a bit of feature development was actually done on that.

It was the only device having eDP other than sc8280xp till x1e80100 
landed last cycle.

I do want to start using sc8280xp as well because from the experience we 
got, it has more visibility in terms of users. So that will address my 
eDP concern.

But, the nice thing about chromebooks is we really like to use them for 
IGT development / CI debug as CrOS provides a nice environment to 
cros-deploy IGT.

We can continue to use sc7180 for IGT development but if we want to 
debug issues with eDP + IGT, sc7280 is a really useful platform for that.

sc8280xp or x1e80100 is not a CrOS supported device. So we will have to 
develop and test IGT directly on the device (which is a bit of a pain) 
unless someone has a better way of "cross-compilation" for IGT on 
non-CrOS images.


> 
> -Doug

Re: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Doug Anderson]

Hi,

On Thu, Mar 14, 2024 at 4:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> On 3/14/2024 4:20 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Jul 3, 2023 at 1:56 AM 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
> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)
> >
> > FWIW, I dug up an old sc7280-herobrine board to test some other change
> > and found it no longer booted. :( I bisected it and this is the change
> > that breaks it. Specifically, I can make mainline boot with:
> >
> > git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
> > domain-idle-states for cluster sleep
> > git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
> > power-domains for cpuidle states
> >
>
> We noticed that some variants of sc7280 herobrine boards didnt boot but
> some did atleast till linux 6.8 rc-6. I have not tested linux 6.9 rc-1 yet.

Wow, really? This doesn't seem like it would be related to the
variant. Maybe the firmware version? FWIW, the device I was having
problems with was a "villager-rev2" with FW 15368.0.0.

OK, so I just pulled out a `hoglin-rev5` with 15432.0.0 and v6.8-rc6
boots and WiFi comes up. However, when I move to full mainline
(b0546776ad3f (HEAD, linux/master) Merge tag 'printk-for-6.9' of
git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux) I get the
ath11k crash.

OK, so I updated my villager to 15432.0.0 and things work even without
reverting ${SUBJECT} patch. I guess that's the answer: this patch
broke things with some old firmwares but with the newer firmware it's
fixed. Hopefully that doesn't happen again since I don't think there
will ever be a newer firmware than 15432.0.0.


> > (I get an ath11k crash after that, but that's easy to hack out since I
> > don't need WiFi)
> >
>
> hmm, wifi worked alright on 6.8 rc-6 for us.

I guess I'll leave it to you to track down / report as needed.


> > I suppose the two options here are to either:
> >
> > 1. Track the problem down and figure out why the breaks boot and then
> > fix it. I'm personally not going to track this down, but if someone
> > wants me to test a patch I can do that.
> >
>
> Can Maulik help us do that?

OK, sounds like we don't need to, as long as everyone updates their
firmware. This should be OK.


> > 2. Delete all the herobrine dts files.
> >
> > So far we've been keeping the herobrine dts files alive because I
> > thought some graphics folks (Rob, Abhinav, Jessica, for instance) were
> > still using it. ...but Rob says he hasn't booted his in a while. No
> > idea if Abhinav and Jessica are using theirs. Any opinions? Is
> > herobrine hardware support near and dear to anyone these days?
> >
>
> Yes, so we have been using sc7280 herobrine devices even till the last
> cycle and quite a bit of feature development was actually done on that.
>
> It was the only device having eDP other than sc8280xp till x1e80100
> landed last cycle.

OK, thanks for confirming that they're still useful to you. When I got
the failures I feared that nobody was using them anymore.


> I do want to start using sc8280xp as well because from the experience we
> got, it has more visibility in terms of users. So that will address my
> eDP concern.
>
> But, the nice thing about chromebooks is we really like to use them for
> IGT development / CI debug as CrOS provides a nice environment to
> cros-deploy IGT.
>
> We can continue to use sc7180 for IGT development but if we want to
> debug issues with eDP + IGT, sc7280 is a really useful platform for that.
>
> sc8280xp or x1e80100 is not a CrOS supported device. So we will have to
> develop and test IGT directly on the device (which is a bit of a pain)
> unless someone has a better way of "cross-compilation" for IGT on
> non-CrOS images.

I'd have to let others comment on IGT.

-Doug

Re: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Sudeep Holla]

On Thu, Mar 14, 2024 at 04:20:59PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jul 3, 2023 at 1:56 AM 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
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)
> 
> FWIW, I dug up an old sc7280-herobrine board to test some other change
> and found it no longer booted. :( I bisected it and this is the change
> that breaks it. Specifically, I can make mainline boot with:
> 
> git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
> domain-idle-states for cluster sleep
> git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
> power-domains for cpuidle states
>

IIRC, this could be issue with psci firmware. There were some known
issues which were discussed few years back but I am not aware of the
details and if/how it is applicable here.

Not sure if you are getting any logs during the boot, if you do have
worth look at logs related to PSCI/OSI/Idle/...

-- 
Regards,
Sudeep

Re: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Doug Anderson]

Hi,

On Fri, Mar 15, 2024 at 8:24 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Mar 14, 2024 at 04:20:59PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Jul 3, 2023 at 1:56 AM 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
> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)
> >
> > FWIW, I dug up an old sc7280-herobrine board to test some other change
> > and found it no longer booted. :( I bisected it and this is the change
> > that breaks it. Specifically, I can make mainline boot with:
> >
> > git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
> > domain-idle-states for cluster sleep
> > git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
> > power-domains for cpuidle states
> >
>
> IIRC, this could be issue with psci firmware. There were some known
> issues which were discussed few years back but I am not aware of the
> details and if/how it is applicable here.
>
> Not sure if you are getting any logs during the boot, if you do have
> worth look at logs related to PSCI/OSI/Idle/...

Given that the new firmware fixes it I'm going to say it's not worth
looking into any longer.

-Doug

Re: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Sudeep Holla]

On Fri, Mar 15, 2024 at 10:12:12AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Mar 15, 2024 at 8:24 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 04:20:59PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Jul 3, 2023 at 1:56 AM 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
> > > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)
> > >
> > > FWIW, I dug up an old sc7280-herobrine board to test some other change
> > > and found it no longer booted. :( I bisected it and this is the change
> > > that breaks it. Specifically, I can make mainline boot with:
> > >
> > > git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
> > > domain-idle-states for cluster sleep
> > > git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
> > > power-domains for cpuidle states
> > >
> >
> > IIRC, this could be issue with psci firmware. There were some known
> > issues which were discussed few years back but I am not aware of the
> > details and if/how it is applicable here.
> >
> > Not sure if you are getting any logs during the boot, if you do have
> > worth look at logs related to PSCI/OSI/Idle/...
> 
> Given that the new firmware fixes it I'm going to say it's not worth
> looking into any longer.
> 

But it would be good to know if that is the exact reason, see if it can
be upgraded, or else we can disable them by default. The bootloader or
something else can detect the f/w version and enable them so that the
board with old f/w(if can't be upgraded) can still be used.

Otherwise it is a regression IMO.

-- 
Regards,
Sudeep

Re: [RESEND v4 3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Doug Anderson]

Hi,

On Fri, Mar 15, 2024 at 10:26 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Mar 15, 2024 at 10:12:12AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Mar 15, 2024 at 8:24 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Mar 14, 2024 at 04:20:59PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Mon, Jul 3, 2023 at 1:56 AM 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
> > > > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.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(-)
> > > >
> > > > FWIW, I dug up an old sc7280-herobrine board to test some other change
> > > > and found it no longer booted. :( I bisected it and this is the change
> > > > that breaks it. Specifically, I can make mainline boot with:
> > > >
> > > > git revert --no-edit db5d137e81bc # arm64: dts: qcom: sc7280: Update
> > > > domain-idle-states for cluster sleep
> > > > git revert --no-edit 7925ca85e956 # arm64: dts: qcom: sc7280: Add
> > > > power-domains for cpuidle states
> > > >
> > >
> > > IIRC, this could be issue with psci firmware. There were some known
> > > issues which were discussed few years back but I am not aware of the
> > > details and if/how it is applicable here.
> > >
> > > Not sure if you are getting any logs during the boot, if you do have
> > > worth look at logs related to PSCI/OSI/Idle/...
> >
> > Given that the new firmware fixes it I'm going to say it's not worth
> > looking into any longer.
> >
>
> But it would be good to know if that is the exact reason, see if it can
> be upgraded, or else we can disable them by default. The bootloader or
> something else can detect the f/w version and enable them so that the
> board with old f/w(if can't be upgraded) can still be used.
>
> Otherwise it is a regression IMO.

I think it only would really matter if the problematic firmware
actually made it out into the real world. In this case the only people
who run into this are developers at Google and Qualcomm who had early
versions of hardware and had old firmware sitting around on them. I
can count the number of folks affected on one hand, and that's even if
one of my fingers gets cut off. All of those folks can just upgrade
their firmware since there is no downside in doing so.

-Doug

Re: [RESEND v4 0/3] Use PSCI OS initiated mode for sc7280

[Ulf Hansson]

Bjorn, Rafael,

On Mon, 3 Jul 2023 at 10:56, Maulik Shah <quic_mkshah@quicinc.com> wrote:
>
> This is resend of v4 with patch1 and patch2 Cced to stable kernel.
>
> 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

This has been ready to be queued for quite a while and I have been
nagging you about it too. Sorry about that. :-)

To help out, I have queued up patch 1 and patch 2 for fixes, through
my new genpd tree [1].

Björn, please pick patch3 for v6.6.

Kind regards
Uffe

[1]
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/linux-pm.git fixes

>
> 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
>

Re: (subset) [RESEND v4 0/3] Use PSCI OS initiated mode for sc7280

[Bjorn Andersson]

On Mon, 03 Jul 2023 14:25:52 +0530, Maulik Shah wrote:
> This is resend of v4 with patch1 and patch2 Cced to stable kernel.
> 
> 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
> 
> [...]

Applied, thanks!

[3/3] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states
      commit: 7925ca85e956191a6a522e0a31a877b98cb5096c

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>