[PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Sibi Sankar]

This series enables CPUFreq support on the X1E SoC using the SCMI perf
protocol. This was originally part of the RFC: firmware: arm_scmi:
Qualcomm Vendor Protocol [1]. I've split it up so that this part can
land earlier. Warnings Introduced by the series are fixed by [2]

V6:
* Drop patches that are already picked up.
* Rebase to the latest next-20241029.

V5:
* Fix build error reported by kernel test robot by adding 64BIT requirement
  to COMPILE_TEST
* Pick Rbs

V4:
* Move val, flag and chan to local loop variables. [Jassi]
* Add cpucp mailbox to the MAINTAINERS file. [Jassi]
* Move to core_initcall. [Konrad]
* Skip explicitly setting txdone_irq/txdone_poll to zero. [Konrad]

V3:
* Fix Maintainer info in cpucp mbox bindings. [Bjorn]
* Fix copyright info in cpucp driver. [Bjorn]
* Drop unused APSS_CPUCP_TX_MBOX_IDR, value init and drv_data. [Bjorn/Dmitry]
* Convert to lower case hex. [Bjorn]
* Convert irq and dev to local variables. [Bjorn]
* Replace for and if with for_each_set_bit. [Bjorn]
* Document the need for spinlock. [Bjorn]
* Add space after " for aesthetics. [Bjorn]
* Fix err in calc and add fixes tag. [Bjorn]
* Include io.h and re-order platform_device.h
* Use GENMASK_ULL to generate APSS_CPUCP_RX_MBOX_CMD_MASK.

V2:
* Fix series version number [Rob]
* Pickup Rbs from Dimitry and Rob.
* Use power-domain instead of clocks. [Sudeep/Ulf]
* Rename sram sub-nodes according to schema. [Dmitry]
* Use BIT() instead of manual shift. [Dmitry]
* Define RX_MBOX_CMD to account for chan calculation. [Dmitry]
* Clear the bit instead of the entire status within the spinlock. [Dmitry]
* Use dev_err_probe instead. [Dmitry]
* Drop superfluous error message while handling errors from get_irq. [Dmitry]
* Use devm_mbox_controller_register and drop remove path. [Dmitry]
* Define TX_MBOX_CMD to account for chan calculation.
* Use cpucp->dev in probe path for conformity.

RFC V1:
* Use x1e80100 as the fallback for future SoCs using the cpucp-mbox
  controller. [Krzysztoff/Konrad/Rob]
* Use chan->lock and chan->cl to detect if the channel is no longer
  Available. [Dmitry]
* Use BIT() instead of using manual shifts. [Dmitry]
* Don't use integer as a pointer value. [Dmitry]
* Allow it to default to of_mbox_index_xlate. [Dmitry]
* Use devm_of_iomap. [Dmitry]
* Use module_platform_driver instead of module init/exit. [Dmitry]
* Get channel number using mailbox core (like other drivers) and
  further simplify the driver by dropping setup_mbox func.

[1]: https://lore.kernel.org/lkml/20240117173458.2312669-1-quic_sibis@quicinc.com/#r
[2]: https://lore.kernel.org/lkml/20241030125512.2884761-1-quic_sibis@quicinc.com/

Other relevant Links:
https://lore.kernel.org/lkml/be2e475a-349f-4e98-b238-262dd7117a4e@linaro.org/

base: next-20241029

Sibi Sankar (2):
  arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
  arm64: dts: qcom: x1e80100: Enable cpufreq

 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 89 +++++++++++++++++++-------
 1 file changed, 65 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH V7 1/2] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes

[Sibi Sankar]

Add the cpucp mailbox and sram nodes required by SCMI perf protocol
on X1E80100 SoCs.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index f70a7e00ed50..be93e482bb28 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -5786,6 +5786,13 @@ gic_its: msi-controller@17040000 {
 			};
 		};
 
+		cpucp_mbox: mailbox@17430000 {
+			compatible = "qcom,x1e80100-cpucp-mbox";
+			reg = <0 0x17430000 0 0x10000>, <0 0x18830000 0 0x10000>;
+			interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		apps_rsc: rsc@17500000 {
 			compatible = "qcom,rpmh-rsc";
 			reg = <0 0x17500000 0 0x10000>,
@@ -5969,6 +5976,25 @@ frame@1780d000 {
 			};
 		};
 
+		sram: sram@18b4e000 {
+			compatible = "mmio-sram";
+			reg = <0x0 0x18b4e000 0x0 0x400>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0x0 0x18b4e000 0x400>;
+
+			cpu_scp_lpri0: scp-sram-section@0 {
+				compatible = "arm,scmi-shmem";
+				reg = <0x0 0x200>;
+			};
+
+			cpu_scp_lpri1: scp-sram-section@200 {
+				compatible = "arm,scmi-shmem";
+				reg = <0x200 0x200>;
+			};
+		};
+
 		pmu@24091000 {
 			compatible = "qcom,x1e80100-llcc-bwmon", "qcom,sc7280-llcc-bwmon";
 			reg = <0 0x24091000 0 0x1000>;
-- 
2.34.1

[PATCH V7 2/2] arm64: dts: qcom: x1e80100: Enable cpufreq

[Sibi Sankar]

Enable cpufreq on X1E80100 SoCs through the SCMI perf protocol node.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 63 ++++++++++++++++----------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index be93e482bb28..9c6d223b1b60 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -71,8 +71,8 @@ cpu0: cpu@0 {
 			reg = <0x0 0x0>;
 			enable-method = "psci";
 			next-level-cache = <&l2_0>;
-			power-domains = <&cpu_pd0>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd0>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 
 			l2_0: l2-cache {
@@ -88,8 +88,8 @@ cpu1: cpu@100 {
 			reg = <0x0 0x100>;
 			enable-method = "psci";
 			next-level-cache = <&l2_0>;
-			power-domains = <&cpu_pd1>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd1>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -99,8 +99,8 @@ cpu2: cpu@200 {
 			reg = <0x0 0x200>;
 			enable-method = "psci";
 			next-level-cache = <&l2_0>;
-			power-domains = <&cpu_pd2>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd2>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -110,8 +110,8 @@ cpu3: cpu@300 {
 			reg = <0x0 0x300>;
 			enable-method = "psci";
 			next-level-cache = <&l2_0>;
-			power-domains = <&cpu_pd3>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd3>, <&scmi_dvfs 0>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -121,8 +121,8 @@ cpu4: cpu@10000 {
 			reg = <0x0 0x10000>;
 			enable-method = "psci";
 			next-level-cache = <&l2_1>;
-			power-domains = <&cpu_pd4>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd4>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 
 			l2_1: l2-cache {
@@ -138,8 +138,8 @@ cpu5: cpu@10100 {
 			reg = <0x0 0x10100>;
 			enable-method = "psci";
 			next-level-cache = <&l2_1>;
-			power-domains = <&cpu_pd5>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd5>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -149,8 +149,8 @@ cpu6: cpu@10200 {
 			reg = <0x0 0x10200>;
 			enable-method = "psci";
 			next-level-cache = <&l2_1>;
-			power-domains = <&cpu_pd6>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd6>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -160,8 +160,8 @@ cpu7: cpu@10300 {
 			reg = <0x0 0x10300>;
 			enable-method = "psci";
 			next-level-cache = <&l2_1>;
-			power-domains = <&cpu_pd7>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd7>, <&scmi_dvfs 1>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -171,8 +171,8 @@ cpu8: cpu@20000 {
 			reg = <0x0 0x20000>;
 			enable-method = "psci";
 			next-level-cache = <&l2_2>;
-			power-domains = <&cpu_pd8>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd8>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 
 			l2_2: l2-cache {
@@ -188,8 +188,8 @@ cpu9: cpu@20100 {
 			reg = <0x0 0x20100>;
 			enable-method = "psci";
 			next-level-cache = <&l2_2>;
-			power-domains = <&cpu_pd9>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd9>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -199,8 +199,8 @@ cpu10: cpu@20200 {
 			reg = <0x0 0x20200>;
 			enable-method = "psci";
 			next-level-cache = <&l2_2>;
-			power-domains = <&cpu_pd10>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd10>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -210,8 +210,8 @@ cpu11: cpu@20300 {
 			reg = <0x0 0x20300>;
 			enable-method = "psci";
 			next-level-cache = <&l2_2>;
-			power-domains = <&cpu_pd11>;
-			power-domain-names = "psci";
+			power-domains = <&cpu_pd11>, <&scmi_dvfs 2>;
+			power-domain-names = "psci", "perf";
 			cpu-idle-states = <&cluster_c4>;
 		};
 
@@ -310,6 +310,21 @@ scm: scm {
 					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
 			qcom,dload-mode = <&tcsr 0x19000>;
 		};
+
+		scmi {
+			compatible = "arm,scmi";
+			mboxes = <&cpucp_mbox 0>, <&cpucp_mbox 2>;
+			mbox-names = "tx", "rx";
+			shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			scmi_dvfs: protocol@13 {
+				reg = <0x13>;
+				#power-domain-cells = <1>;
+			};
+		};
 	};
 
 	clk_virt: interconnect-0 {
-- 
2.34.1

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Johan Hovold]

[ +CC: Marc, who I think I saw reporting something similar even if I can
  seem to find where right now ]

On Wed, Oct 30, 2024 at 06:38:38PM +0530, Sibi Sankar wrote:
> This series enables CPUFreq support on the X1E SoC using the SCMI perf
> protocol. This was originally part of the RFC: firmware: arm_scmi:
> Qualcomm Vendor Protocol [1]. I've split it up so that this part can
> land earlier. Warnings Introduced by the series are fixed by [2]

 Sibi Sankar (2):
>   arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
>   arm64: dts: qcom: x1e80100: Enable cpufreq

I've been running with v6 of these for a while now, without noticing any
issues, and just updated to v7 to be able to provide a Tested-by tag.

I wanted to run a compilation and see how the frequencies varied, but
before I got around to that I just grepped the cpufreq sysfs attributes
for CPU0 four times. And this triggered a reset of the machine (x1e80100
CRD).

The last values output were:

	affected_cpus:0 1 2 3
	cpuinfo_cur_freq:<unknown>
	cpuinfo_max_freq:3417600
	cpuinfo_min_freq:710400
	cpuinfo_transition_latency:30000
	related_cpus:0 1 2 3
	scaling_available_frequencies:710400 806400 998400 1190400 1440000 1670400 1920000 2188800 2515200 2707200 2976000 320
	scaling_available_governors:ondemand userspace performance schedutil
	scaling_cur_freq:806400
	scaling_driver:scmi
	scaling_governor:schedutil
	scaling_max_freq:3417600
	scaling_min_freq:710400
	scaling_setspeed:<unsupported>

Notice the <unknown> current frequency (the previous greps said 710400
and 2515200).

The last thing I see on the serial console, presumably just before
the reset, is:

	[  196.268025] arm-scmi arm-scmi.0.auto: timed out in resp(caller: do_xfer+0x164/0x564)

I just rebooted and grepped again and it triggered on the first attempt
(cur_freq also said '<unknown>'). Same error in the log, printed when
grepping.

Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Marc Zyngier]

On Fri, 01 Nov 2024 13:00:37 +0000,
Johan Hovold <johan@kernel.org> wrote:
> 
> [ +CC: Marc, who I think I saw reporting something similar even if I can
>   seem to find where right now ]

It was on IRC.

> 
> On Wed, Oct 30, 2024 at 06:38:38PM +0530, Sibi Sankar wrote:
> > This series enables CPUFreq support on the X1E SoC using the SCMI perf
> > protocol. This was originally part of the RFC: firmware: arm_scmi:
> > Qualcomm Vendor Protocol [1]. I've split it up so that this part can
> > land earlier. Warnings Introduced by the series are fixed by [2]
> 
>  Sibi Sankar (2):
> >   arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
> >   arm64: dts: qcom: x1e80100: Enable cpufreq
> 
> I've been running with v6 of these for a while now, without noticing any
> issues, and just updated to v7 to be able to provide a Tested-by tag.
> 
> I wanted to run a compilation and see how the frequencies varied, but
> before I got around to that I just grepped the cpufreq sysfs attributes
> for CPU0 four times. And this triggered a reset of the machine (x1e80100
> CRD).
> 
> The last values output were:
> 
> 	affected_cpus:0 1 2 3
> 	cpuinfo_cur_freq:<unknown>
> 	cpuinfo_max_freq:3417600
> 	cpuinfo_min_freq:710400
> 	cpuinfo_transition_latency:30000
> 	related_cpus:0 1 2 3
> 	scaling_available_frequencies:710400 806400 998400 1190400 1440000 1670400 1920000 2188800 2515200 2707200 2976000 320
> 	scaling_available_governors:ondemand userspace performance schedutil
> 	scaling_cur_freq:806400
> 	scaling_driver:scmi
> 	scaling_governor:schedutil
> 	scaling_max_freq:3417600
> 	scaling_min_freq:710400
> 	scaling_setspeed:<unsupported>
> 
> Notice the <unknown> current frequency (the previous greps said 710400
> and 2515200).
> 
> The last thing I see on the serial console, presumably just before
> the reset, is:
> 
> 	[  196.268025] arm-scmi arm-scmi.0.auto: timed out in resp(caller: do_xfer+0x164/0x564)
> 
> I just rebooted and grepped again and it triggered on the first attempt
> (cur_freq also said '<unknown>'). Same error in the log, printed when
> grepping.

I'm seeing similar things indeed. Randomly grepping in cpufreq/policy*
results in hard resets, although I don't get much on the serial
console when that happens. Interestingly, I also see some errors in
dmesg at boot time:

maz@semi-fraudulent:~$ dmesg| grep -i scmi
[    0.966175] scmi_core: SCMI protocol bus registered
[    7.929710] arm-scmi arm-scmi.2.auto: Using scmi_mailbox_transport
[    7.939059] arm-scmi arm-scmi.2.auto: SCMI max-rx-timeout: 30ms
[    7.945567] arm-scmi arm-scmi.2.auto: SCMI RAW Mode initialized for instance 0
[    7.958348] arm-scmi arm-scmi.2.auto: SCMI RAW Mode COEX enabled !
[    7.978303] arm-scmi arm-scmi.2.auto: SCMI Notifications - Core Enabled.
[    7.985351] arm-scmi arm-scmi.2.auto: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x20000
[    8.033774] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
[    8.033902] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
[    8.036528] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
[    8.036744] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
[    8.171232] scmi-perf-domain scmi_dev.4: Initialized 3 performance domains

All these "Failed" are a bit worrying. Happy to put any theory to the
test.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Johan Hovold]

On Fri, Nov 01, 2024 at 02:08:24PM +0000, Marc Zyngier wrote:

> I'm seeing similar things indeed. Randomly grepping in cpufreq/policy*
> results in hard resets, although I don't get much on the serial
> console when that happens. Interestingly, I also see some errors in
> dmesg at boot time:
> 
> maz@semi-fraudulent:~$ dmesg| grep -i scmi
> [    0.966175] scmi_core: SCMI protocol bus registered
> [    7.929710] arm-scmi arm-scmi.2.auto: Using scmi_mailbox_transport
> [    7.939059] arm-scmi arm-scmi.2.auto: SCMI max-rx-timeout: 30ms
> [    7.945567] arm-scmi arm-scmi.2.auto: SCMI RAW Mode initialized for instance 0
> [    7.958348] arm-scmi arm-scmi.2.auto: SCMI RAW Mode COEX enabled !
> [    7.978303] arm-scmi arm-scmi.2.auto: SCMI Notifications - Core Enabled.
> [    7.985351] arm-scmi arm-scmi.2.auto: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x20000
> [    8.033774] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> [    8.033902] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> [    8.036528] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> [    8.036744] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> [    8.171232] scmi-perf-domain scmi_dev.4: Initialized 3 performance domains
> 
> All these "Failed" are a bit worrying. Happy to put any theory to the
> test.

Yes, those warnings indeed look troubling. Fortunately they appear to be
mostly benign and only indicate that the firmware is reporting duplicate
OPPs, which the kernel is now ignoring without any other side effects
than the warnings.

The side-effects and these remaining warnings are addressed by this
series:

	https://lore.kernel.org/all/20241030125512.2884761-1-quic_sibis@quicinc.com/

but I think we should try to make the warnings a bit more informative
(and less scary) by printing something along the lines of:

	arm-scmi arm-scmi.0.auto: [Firmware Bug]: Ignoring duplicate OPP 3417600 for NCC

instead.

Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Marc Zyngier]

On Fri, 01 Nov 2024 14:19:54 +0000,
Johan Hovold <johan@kernel.org> wrote:
> 
> On Fri, Nov 01, 2024 at 02:08:24PM +0000, Marc Zyngier wrote:
> 
> > I'm seeing similar things indeed. Randomly grepping in cpufreq/policy*
> > results in hard resets, although I don't get much on the serial
> > console when that happens. Interestingly, I also see some errors in
> > dmesg at boot time:
> > 
> > maz@semi-fraudulent:~$ dmesg| grep -i scmi
> > [    0.966175] scmi_core: SCMI protocol bus registered
> > [    7.929710] arm-scmi arm-scmi.2.auto: Using scmi_mailbox_transport
> > [    7.939059] arm-scmi arm-scmi.2.auto: SCMI max-rx-timeout: 30ms
> > [    7.945567] arm-scmi arm-scmi.2.auto: SCMI RAW Mode initialized for instance 0
> > [    7.958348] arm-scmi arm-scmi.2.auto: SCMI RAW Mode COEX enabled !
> > [    7.978303] arm-scmi arm-scmi.2.auto: SCMI Notifications - Core Enabled.
> > [    7.985351] arm-scmi arm-scmi.2.auto: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x20000
> > [    8.033774] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> > [    8.033902] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> > [    8.036528] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> > [    8.036744] arm-scmi arm-scmi.2.auto: Failed to add opps_by_lvl at 3801600 for NCC - ret:-16
> > [    8.171232] scmi-perf-domain scmi_dev.4: Initialized 3 performance domains
> > 
> > All these "Failed" are a bit worrying. Happy to put any theory to the
> > test.
> 
> Yes, those warnings indeed look troubling. Fortunately they appear to be
> mostly benign and only indicate that the firmware is reporting duplicate
> OPPs, which the kernel is now ignoring without any other side effects
> than the warnings.

Right. Not something that would explain the hard reset behaviour then.

> 
> The side-effects and these remaining warnings are addressed by this
> series:
> 
> 	https://lore.kernel.org/all/20241030125512.2884761-1-quic_sibis@quicinc.com/
> 
> but I think we should try to make the warnings a bit more informative
> (and less scary) by printing something along the lines of:
> 
> 	arm-scmi arm-scmi.0.auto: [Firmware Bug]: Ignoring duplicate OPP 3417600 for NCC
> 
> instead.

Indeed. Seeing [Firmware Bug] has a comforting feeling of
familiarity... :)

I wonder whether the same sort of reset happen on more "commercial"
systems (such as some of the laptops). You expect that people look at
the cpufreq stuff closely, and don't see things exploding like we are.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Johan Hovold]

On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:
> On Fri, 01 Nov 2024 14:19:54 +0000,
> Johan Hovold <johan@kernel.org> wrote:

> > The side-effects and these remaining warnings are addressed by this
> > series:
> > 
> > 	https://lore.kernel.org/all/20241030125512.2884761-1-quic_sibis@quicinc.com/
> > 
> > but I think we should try to make the warnings a bit more informative
> > (and less scary) by printing something along the lines of:
> > 
> > 	arm-scmi arm-scmi.0.auto: [Firmware Bug]: Ignoring duplicate OPP 3417600 for NCC
> > 
> > instead.
> 
> Indeed. Seeing [Firmware Bug] has a comforting feeling of
> familiarity... :)
> 
> I wonder whether the same sort of reset happen on more "commercial"
> systems (such as some of the laptops). You expect that people look at
> the cpufreq stuff closely, and don't see things exploding like we are.

I finally got around to getting my Lenovo ThinkPad T14s to boot (it
refuses to start the kernel when using GRUB, and it's not due to the
known 64 GB memory issue as it only has 32 GB) and can confirm that it
hard resets when accessing the cpufreq sysfs attributes as well.

On the bright side, at least I don't see any warnings due to duplicate
OPPs on this machine (x1e78100, latest UEFI fw).

Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Marc Zyngier]

On Tue, 05 Nov 2024 16:57:07 +0000,
Johan Hovold <johan@kernel.org> wrote:
> 
> On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:
> > On Fri, 01 Nov 2024 14:19:54 +0000,
> > Johan Hovold <johan@kernel.org> wrote:
> 
> > > The side-effects and these remaining warnings are addressed by this
> > > series:
> > > 
> > > 	https://lore.kernel.org/all/20241030125512.2884761-1-quic_sibis@quicinc.com/
> > > 
> > > but I think we should try to make the warnings a bit more informative
> > > (and less scary) by printing something along the lines of:
> > > 
> > > 	arm-scmi arm-scmi.0.auto: [Firmware Bug]: Ignoring duplicate OPP 3417600 for NCC
> > > 
> > > instead.
> > 
> > Indeed. Seeing [Firmware Bug] has a comforting feeling of
> > familiarity... :)
> > 
> > I wonder whether the same sort of reset happen on more "commercial"
> > systems (such as some of the laptops). You expect that people look at
> > the cpufreq stuff closely, and don't see things exploding like we are.
> 
> I finally got around to getting my Lenovo ThinkPad T14s to boot (it
> refuses to start the kernel when using GRUB, and it's not due to the
> known 64 GB memory issue as it only has 32 GB)

<cry>
I know the feeling. My devkit can't use GRUB either, so I added a
hook to the GRUB config to generate EFI scripts that directly execute
the kernel with initrd, dtb, and command line.

This is probably the worse firmware I've seen in a very long while.
</cry>

> and can confirm that it
> hard resets when accessing the cpufreq sysfs attributes as well.

Right. So this also happens on non-abandonware machines.

> On the bright side, at least I don't see any warnings due to duplicate
> OPPs on this machine (x1e78100, latest UEFI fw).

One bug fixed...

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Sibi Sankar]

On 11/5/24 23:42, Marc Zyngier wrote:
> On Tue, 05 Nov 2024 16:57:07 +0000,
> Johan Hovold <johan@kernel.org> wrote:
>>
>> On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:
>>> On Fri, 01 Nov 2024 14:19:54 +0000,
>>> Johan Hovold <johan@kernel.org> wrote:
>>
>>>> The side-effects and these remaining warnings are addressed by this
>>>> series:
>>>>
>>>> 	https://lore.kernel.org/all/20241030125512.2884761-1-quic_sibis@quicinc.com/
>>>>
>>>> but I think we should try to make the warnings a bit more informative
>>>> (and less scary) by printing something along the lines of:
>>>>
>>>> 	arm-scmi arm-scmi.0.auto: [Firmware Bug]: Ignoring duplicate OPP 3417600 for NCC
>>>>
>>>> instead.
>>>
>>> Indeed. Seeing [Firmware Bug] has a comforting feeling of
>>> familiarity... :)
>>>
>>> I wonder whether the same sort of reset happen on more "commercial"
>>> systems (such as some of the laptops). You expect that people look at
>>> the cpufreq stuff closely, and don't see things exploding like we are.
>>
>> I finally got around to getting my Lenovo ThinkPad T14s to boot (it
>> refuses to start the kernel when using GRUB, and it's not due to the
>> known 64 GB memory issue as it only has 32 GB)
> 
> <cry>
> I know the feeling. My devkit can't use GRUB either, so I added a
> hook to the GRUB config to generate EFI scripts that directly execute
> the kernel with initrd, dtb, and command line.
> 
> This is probably the worse firmware I've seen in a very long while.

The PERF_LEVEL_GET implementation in the SCP firmware side
is the reason for the crash :|, currently there is a bug
in the kernel that picks up index that we set with LEVEL_SET
with fast channel and that masks the crash. I was told the
crash happens when idle states are enabled and a regular
LEVEL_GET message is triggered from the kernel. This was
fixed a while back but it will take a while to flow back
to all the devices. It should already be out CRD's.

Johan,
Now that you are aware of the the limitations can we make
a call on how to deal with this and land cpufreq?

-Sibi

> </cry>
> 
>> and can confirm that it
>> hard resets when accessing the cpufreq sysfs attributes as well.
> 
> Right. So this also happens on non-abandonware machines.
> 
>> On the bright side, at least I don't see any warnings due to duplicate
>> OPPs on this machine (x1e78100, latest UEFI fw).
> 
> One bug fixed...
> 
> 	M.
> 

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Marc Zyngier]

On Thu, 05 Dec 2024 11:23:05 +0000,
Sibi Sankar <quic_sibis@quicinc.com> wrote:
> 
> 
> 
> On 11/5/24 23:42, Marc Zyngier wrote:
> > On Tue, 05 Nov 2024 16:57:07 +0000,
> > Johan Hovold <johan@kernel.org> wrote:
> >> 
> >> On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:
> >>> On Fri, 01 Nov 2024 14:19:54 +0000,
> >>> Johan Hovold <johan@kernel.org> wrote:
> >> 
> >>>> The side-effects and these remaining warnings are addressed by this
> >>>> series:
> >>>> 
> >>>> 	https://lore.kernel.org/all/20241030125512.2884761-1-quic_sibis@quicinc.com/
> >>>> 
> >>>> but I think we should try to make the warnings a bit more informative
> >>>> (and less scary) by printing something along the lines of:
> >>>> 
> >>>> 	arm-scmi arm-scmi.0.auto: [Firmware Bug]: Ignoring duplicate OPP 3417600 for NCC
> >>>> 
> >>>> instead.
> >>> 
> >>> Indeed. Seeing [Firmware Bug] has a comforting feeling of
> >>> familiarity... :)
> >>> 
> >>> I wonder whether the same sort of reset happen on more "commercial"
> >>> systems (such as some of the laptops). You expect that people look at
> >>> the cpufreq stuff closely, and don't see things exploding like we are.
> >> 
> >> I finally got around to getting my Lenovo ThinkPad T14s to boot (it
> >> refuses to start the kernel when using GRUB, and it's not due to the
> >> known 64 GB memory issue as it only has 32 GB)
> > 
> > <cry>
> > I know the feeling. My devkit can't use GRUB either, so I added a
> > hook to the GRUB config to generate EFI scripts that directly execute
> > the kernel with initrd, dtb, and command line.
> > 
> > This is probably the worse firmware I've seen in a very long while.
> 
> The PERF_LEVEL_GET implementation in the SCP firmware side
> is the reason for the crash :|, currently there is a bug
> in the kernel that picks up index that we set with LEVEL_SET
> with fast channel and that masks the crash. I was told the
> crash happens when idle states are enabled and a regular
> LEVEL_GET message is triggered from the kernel. This was
> fixed a while back but it will take a while to flow back
> to all the devices. It should already be out CRD's.

But this fix will never reach some platforms, such as the devkit, and
we cannot mandate that people update to the latest anyway. How do we
distinguish between good and bad firmware versions?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Johan Hovold]

On Thu, Dec 05, 2024 at 04:53:05PM +0530, Sibi Sankar wrote:
> On 11/5/24 23:42, Marc Zyngier wrote:
> > On Tue, 05 Nov 2024 16:57:07 +0000,
> > Johan Hovold <johan@kernel.org> wrote:
> >> On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:

> >>> I wonder whether the same sort of reset happen on more "commercial"
> >>> systems (such as some of the laptops). You expect that people look at
> >>> the cpufreq stuff closely, and don't see things exploding like we are.
> >>
> >> I finally got around to getting my Lenovo ThinkPad T14s to boot (it
> >> refuses to start the kernel when using GRUB, and it's not due to the
> >> known 64 GB memory issue as it only has 32 GB)
> > 
> > <cry>
> > I know the feeling. My devkit can't use GRUB either, so I added a
> > hook to the GRUB config to generate EFI scripts that directly execute
> > the kernel with initrd, dtb, and command line.
> > 
> > This is probably the worse firmware I've seen in a very long while.
> 
> The PERF_LEVEL_GET implementation in the SCP firmware side
> is the reason for the crash :|, currently there is a bug
> in the kernel that picks up index that we set with LEVEL_SET
> with fast channel and that masks the crash. I was told the
> crash happens when idle states are enabled and a regular
> LEVEL_GET message is triggered from the kernel. This was
> fixed a while back but it will take a while to flow back
> to all the devices. It should already be out CRD's.
> 
> Johan,
> Now that you are aware of the the limitations can we make
> a call on how to deal with this and land cpufreq?

As Marc said, it seems you need to come up with a way to detect and work
around the broken firmware.

We want to get the fast channel issue fixed, but when we merge that fix
it will trigger these crashes if we also merge cpufreq support for x1e.

Can you expand the on the PERF_LEVEL_GET issue? Is it possible to
implement some workaround for the buggy firmware? Like returning a dummy
value? How exactly are things working today? Can't that be used a basis
for a quirk?

> > </cry>
> > 
> >> and can confirm that it
> >> hard resets when accessing the cpufreq sysfs attributes as well.

Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Sibi Sankar]

On 12/5/24 21:16, Johan Hovold wrote:
> On Thu, Dec 05, 2024 at 04:53:05PM +0530, Sibi Sankar wrote:
>> On 11/5/24 23:42, Marc Zyngier wrote:
>>> On Tue, 05 Nov 2024 16:57:07 +0000,
>>> Johan Hovold <johan@kernel.org> wrote:
>>>> On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:
> 
>>>>> I wonder whether the same sort of reset happen on more "commercial"
>>>>> systems (such as some of the laptops). You expect that people look at
>>>>> the cpufreq stuff closely, and don't see things exploding like we are.
>>>>
>>>> I finally got around to getting my Lenovo ThinkPad T14s to boot (it
>>>> refuses to start the kernel when using GRUB, and it's not due to the
>>>> known 64 GB memory issue as it only has 32 GB)
>>>
>>> <cry>
>>> I know the feeling. My devkit can't use GRUB either, so I added a
>>> hook to the GRUB config to generate EFI scripts that directly execute
>>> the kernel with initrd, dtb, and command line.
>>>
>>> This is probably the worse firmware I've seen in a very long while.
>>
>> The PERF_LEVEL_GET implementation in the SCP firmware side
>> is the reason for the crash :|, currently there is a bug
>> in the kernel that picks up index that we set with LEVEL_SET
>> with fast channel and that masks the crash. I was told the
>> crash happens when idle states are enabled and a regular
>> LEVEL_GET message is triggered from the kernel. This was
>> fixed a while back but it will take a while to flow back
>> to all the devices. It should already be out CRD's.
>>
>> Johan,
>> Now that you are aware of the the limitations can we make
>> a call on how to deal with this and land cpufreq?
> 
> As Marc said, it seems you need to come up with a way to detect and work
> around the broken firmware.

The perf protocol version won't have any changes so detecting
it isn't possible :(

> 
> We want to get the fast channel issue fixed, but when we merge that fix
> it will trigger these crashes if we also merge cpufreq support for x1e.
> 
> Can you expand the on the PERF_LEVEL_GET issue? Is it possible to
> implement some workaround for the buggy firmware? Like returning a dummy
> value? How exactly are things working today? Can't that be used a basis
> for a quirk?

The main problem is the X1E firmware supports fast channel level get
but when queried it says it doesn't support it :|. The PERF_LEVEL_GET
regular messaging which gets used as a fallback has a bug which causes
the device to crash. So we either enable cpufreq only on platforms
that has the fix in place or live with the warning that certain messages
don't support fast channel which I don't think will fly. I've also been
told the crash wouldn't show up if we have all sleep states disabled.

> 
>>> </cry>
>>>
>>>> and can confirm that it
>>>> hard resets when accessing the cpufreq sysfs attributes as well.
> 
> Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Marc Zyngier]

On Mon, 06 Jan 2025 12:22:48 +0000,
Sibi Sankar <quic_sibis@quicinc.com> wrote:
> 
> 
> 
> On 12/5/24 21:16, Johan Hovold wrote:
> > On Thu, Dec 05, 2024 at 04:53:05PM +0530, Sibi Sankar wrote:
> >> On 11/5/24 23:42, Marc Zyngier wrote:
> >>> On Tue, 05 Nov 2024 16:57:07 +0000,
> >>> Johan Hovold <johan@kernel.org> wrote:
> >>>> On Fri, Nov 01, 2024 at 02:43:57PM +0000, Marc Zyngier wrote:
> > 
> >>>>> I wonder whether the same sort of reset happen on more "commercial"
> >>>>> systems (such as some of the laptops). You expect that people look at
> >>>>> the cpufreq stuff closely, and don't see things exploding like we are.
> >>>> 
> >>>> I finally got around to getting my Lenovo ThinkPad T14s to boot (it
> >>>> refuses to start the kernel when using GRUB, and it's not due to the
> >>>> known 64 GB memory issue as it only has 32 GB)
> >>> 
> >>> <cry>
> >>> I know the feeling. My devkit can't use GRUB either, so I added a
> >>> hook to the GRUB config to generate EFI scripts that directly execute
> >>> the kernel with initrd, dtb, and command line.
> >>> 
> >>> This is probably the worse firmware I've seen in a very long while.
> >> 
> >> The PERF_LEVEL_GET implementation in the SCP firmware side
> >> is the reason for the crash :|, currently there is a bug
> >> in the kernel that picks up index that we set with LEVEL_SET
> >> with fast channel and that masks the crash. I was told the
> >> crash happens when idle states are enabled and a regular
> >> LEVEL_GET message is triggered from the kernel. This was
> >> fixed a while back but it will take a while to flow back
> >> to all the devices. It should already be out CRD's.
> >> 
> >> Johan,
> >> Now that you are aware of the the limitations can we make
> >> a call on how to deal with this and land cpufreq?
> > 
> > As Marc said, it seems you need to come up with a way to detect and work
> > around the broken firmware.
> 
> The perf protocol version won't have any changes so detecting
> it isn't possible :(

This is just... baffling. Can this be checked against one of the
strings contained in the DMI tables?

> 
> > 
> > We want to get the fast channel issue fixed, but when we merge that fix
> > it will trigger these crashes if we also merge cpufreq support for x1e.
> > 
> > Can you expand the on the PERF_LEVEL_GET issue? Is it possible to
> > implement some workaround for the buggy firmware? Like returning a dummy
> > value? How exactly are things working today? Can't that be used a basis
> > for a quirk?
> 
> The main problem is the X1E firmware supports fast channel level get
> but when queried it says it doesn't support it :|. The PERF_LEVEL_GET
> regular messaging which gets used as a fallback has a bug which causes
> the device to crash. So we either enable cpufreq only on platforms
> that has the fix in place

Again: how do we detect this?

> or live with the warning that certain messages
> don't support fast channel which I don't think will fly. I've also been
> told the crash wouldn't show up if we have all sleep states
> disabled.

So we have the choice between crashing quickly, or sucking power like
mad?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Johan Hovold]

On Mon, Jan 06, 2025 at 05:52:48PM +0530, Sibi Sankar wrote:
> On 12/5/24 21:16, Johan Hovold wrote:

> > As Marc said, it seems you need to come up with a way to detect and work
> > around the broken firmware.
> 
> The perf protocol version won't have any changes so detecting
> it isn't possible :(

But there could be other ways, see below.

> > We want to get the fast channel issue fixed, but when we merge that fix
> > it will trigger these crashes if we also merge cpufreq support for x1e.
> > 
> > Can you expand the on the PERF_LEVEL_GET issue? Is it possible to
> > implement some workaround for the buggy firmware? Like returning a dummy
> > value? How exactly are things working today? Can't that be used a basis
> > for a quirk?
> 
> The main problem is the X1E firmware supports fast channel level get
> but when queried it says it doesn't support it :|. The PERF_LEVEL_GET
> regular messaging which gets used as a fallback has a bug which causes
> the device to crash. So we either enable cpufreq only on platforms
> that has the fix in place or live with the warning that certain messages
> don't support fast channel which I don't think will fly. I've also been
> told the crash wouldn't show up if we have all sleep states disabled.

We certainly want cpufreq enabled also on the current/older firmware
which have these bugs.

Based on the above, it sounds like your fix:

	https://lore.kernel.org/lkml/20241030125512.2884761-2-quic_sibis@quicinc.com/

is correct even if it triggers the crash on machines with buggy firmware.

Why can't you add a quirk for x1e platforms that makes sure that the
driver always uses fastchannel level get?

You know it is supported (and as has to be used) even if the buggy
firmware says it's not. Just set the corresponding attribute bit
unconditionally based on the DT machine compatible (or fall back to the
current implementation which theoretically other broken fw
implementations may also be relying on), or similar.

Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Sibi Sankar]

On 1/10/25 15:22, Johan Hovold wrote:
> On Mon, Jan 06, 2025 at 05:52:48PM +0530, Sibi Sankar wrote:
>> On 12/5/24 21:16, Johan Hovold wrote:
> 
>>> As Marc said, it seems you need to come up with a way to detect and work
>>> around the broken firmware.
>>
>> The perf protocol version won't have any changes so detecting
>> it isn't possible :(
> 
> But there could be other ways, see below.
> 
>>> We want to get the fast channel issue fixed, but when we merge that fix
>>> it will trigger these crashes if we also merge cpufreq support for x1e.
>>>
>>> Can you expand the on the PERF_LEVEL_GET issue? Is it possible to
>>> implement some workaround for the buggy firmware? Like returning a dummy
>>> value? How exactly are things working today? Can't that be used a basis
>>> for a quirk?
>>
>> The main problem is the X1E firmware supports fast channel level get
>> but when queried it says it doesn't support it :|. The PERF_LEVEL_GET
>> regular messaging which gets used as a fallback has a bug which causes
>> the device to crash. So we either enable cpufreq only on platforms
>> that has the fix in place or live with the warning that certain messages
>> don't support fast channel which I don't think will fly. I've also been
>> told the crash wouldn't show up if we have all sleep states disabled.
> 
> We certainly want cpufreq enabled also on the current/older firmware
> which have these bugs.
> 
> Based on the above, it sounds like your fix:
> 
> 	https://lore.kernel.org/lkml/20241030125512.2884761-2-quic_sibis@quicinc.com/
> 
> is correct even if it triggers the crash on machines with buggy firmware.
> 
> Why can't you add a quirk for x1e platforms that makes sure that the
> driver always uses fastchannel level get?

Makes sense but introducing anything like ^^ needs consensus from
the scmi maintainers but for obvious they won't comment on this
thread :| . I'll repeat the same question on another thread.

-Sibi

> 
> You know it is supported (and as has to be used) even if the buggy
> firmware says it's not. Just set the corresponding attribute bit
> unconditionally based on the DT machine compatible (or fall back to the
> current implementation which theoretically other broken fw
> implementations may also be relying on), or similar.
> 
> Johan

Re: [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Johan Hovold]

Hi Sibi and Bjorn,

On Wed, Oct 30, 2024 at 06:38:38PM +0530, Sibi Sankar wrote:
> This series enables CPUFreq support on the X1E SoC using the SCMI perf
> protocol. This was originally part of the RFC: firmware: arm_scmi:
> Qualcomm Vendor Protocol [1]. I've split it up so that this part can
> land earlier. Warnings Introduced by the series are fixed by [2]

With the SCMI perf quirk for the Qualcomm firmware merged for 6.16, we
can now finally get this one merged.

There appears to be a trivial conflict due to a context change (the new
watchdog node) however.

Can you fix that up when applying, Bjorn, or do you want Sibi to send a
v8?

Johan

Re: (subset) [PATCH V7 0/2] qcom: x1e80100: Enable CPUFreq

[Bjorn Andersson]

On Wed, 30 Oct 2024 18:38:38 +0530, Sibi Sankar wrote:
> This series enables CPUFreq support on the X1E SoC using the SCMI perf
> protocol. This was originally part of the RFC: firmware: arm_scmi:
> Qualcomm Vendor Protocol [1]. I've split it up so that this part can
> land earlier. Warnings Introduced by the series are fixed by [2]
> 
> V6:
> * Drop patches that are already picked up.
> * Rebase to the latest next-20241029.
> 
> [...]

Applied, thanks!

[1/2] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes
      commit: 06e3c7ec804524767f529404d023993fbca9ea43
[2/2] arm64: dts: qcom: x1e80100: Enable cpufreq
      commit: 892c83aa39cb4c1a4c63fbc51d6f6bd76bfa1ec9

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