[PATCH v2 0/2] Enable CPU Idle for gs101

[PATCH v2 0/2] Enable CPU Idle for gs101

[Peter Griffin]

Hi folks,

This series adds support for CPU Idle on gs101. In particular it
achieves this by registerring a cpu pm notifier and programming
a ACPM hint to enter the c2 idle state. With the hint programmed
the system now enters c2 idle state.

Note: the driver patch has a runtime dependency on the device tree
change to add `local-timer-stop` DT property to the CPU nodes.
Without this DT patch the system will hang in early boot as the
local timer is shutdown. The DT patch was originally sent along
with Wills MCT series in [1] but it can be merged independently
of the rest of the MCT changes, so I've included it here to
(hopefully!) make things clearer and easier as it has a strong
dependency with this patch.

We can measure the impact of these changes upstream using the fuel
gauge series from Thomas Antoine [2]. With the ACPM hint now
programmed /sys/class/power_supply/max77759-fg/current_avg is a
postive number around 150000 microamps meaning we are charging the
battery (assuming it isn't already full).

Prior to programming the hint this would report a negative number
around -150000 microamps meaning the battery was discharing.

Thanks,

Peter

[1] https://lore.kernel.org/lkml/20250402233407.2452429-5-willmcvicker@google.com/
[2] https://lore.kernel.org/lkml/20250421-b4-gs101_max77759_fg-v3-0-50cd8caf9017@uclouvain.be/

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
Changes in v2:
 * rebase onto next-20250610
 * Add #ifdef CONFIG_PM_SLEEP to avoid
   Fix warning: unused variable 'cpupm_pm_ops' [-Wunused-const-variable] (0-day)
- Link to v1: https://lore.kernel.org/r/20250524-gs101-cpuidle-v1-0-aea77a7842a6@linaro.org

---
Peter Griffin (1):
      soc: samsung: exynos-pmu: Enable CPU Idle for gs101

Will Deacon (1):
      arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

 arch/arm64/boot/dts/exynos/google/gs101.dtsi |   3 +
 drivers/soc/samsung/exynos-pmu.c             | 137 ++++++++++++++++++++++++++-
 2 files changed, 136 insertions(+), 4 deletions(-)
---
base-commit: b27cc623e01be9de1580eaa913508b237a7a9673
change-id: 20250524-gs101-cpuidle-cafc96dd849d

Best regards,
-- 
Peter Griffin <peter.griffin@linaro.org>

[PATCH v2 1/2] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[Peter Griffin]

From: Will Deacon <willdeacon@google.com>

In preparation for switching to the architected timer as the primary
clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
property to indicate that an alternative clockevents device must be
used for waking up from the "c2" idle state.

Signed-off-by: Will Deacon <willdeacon@google.com>
[Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
Signed-off-by: Will McVicker <willmcvicker@google.com>
Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>
Tested-by: Youngmin Nam <youngmin.nam@samsung.com>
Fixes: ea89fdf24fd9 ("arm64: dts: exynos: google: Add initial Google gs101 SoC support")
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index 48c691fd0a3ae430b5d66b402610d23b72b144d7..94aa0ffb9a9760c58818c0417001fd187b048ea8 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
 				idle-state-name = "c2";
 				compatible = "arm,idle-state";
 				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
 				entry-latency-us = <70>;
 				exit-latency-us = <160>;
 				min-residency-us = <2000>;
@@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
 				idle-state-name = "c2";
 				compatible = "arm,idle-state";
 				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
 				entry-latency-us = <150>;
 				exit-latency-us = <190>;
 				min-residency-us = <2500>;
@@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
 				idle-state-name = "c2";
 				compatible = "arm,idle-state";
 				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
 				entry-latency-us = <235>;
 				exit-latency-us = <220>;
 				min-residency-us = <3500>;

-- 
2.50.0.rc1.591.g9c95f17f64-goog

[PATCH v2 2/2] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Peter Griffin]

Register cpu pm notifiers for gs101 which call the
gs101_cpu_pmu_online/offline callbacks which in turn
program the ACPM hint. This is required to actually
enter the idle state.

A couple of corner cases are handled, namely when the
system is rebooting or suspending we ignore the request.
Additionally the request is ignored if the CPU is in
CPU hot plug.

Note: this patch has a runtime dependency on adding
'local-timer-stop' dt property to the CPU nodes. This
informs the time framework to switch to a broadcast timer
as the local timer will be shutdown. Without that DT
property specified the system hangs in early boot with
this patch applied.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
Changes in v2
 * Add ifdef CONFIG_PM_SLEEP to avoid
   Fix warning: unused variable 'cpupm_pm_ops' [-Wunused-const-variable] (0-day)
---
 drivers/soc/samsung/exynos-pmu.c | 137 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 133 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index a77288f49d249f890060c595556708334383c910..7f72ecd60994f18bb639dd8e09e1c6ff6158066b 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -8,6 +8,7 @@
 #include <linux/array_size.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuhotplug.h>
+#include <linux/cpu_pm.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/mfd/core.h>
@@ -15,6 +16,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
+#include <linux/reboot.h>
 #include <linux/regmap.h>
 
 #include <linux/soc/samsung/exynos-regs-pmu.h>
@@ -35,6 +37,10 @@ struct exynos_pmu_context {
 	const struct exynos_pmu_data *pmu_data;
 	struct regmap *pmureg;
 	struct regmap *pmuintrgen;
+	spinlock_t cpupm_lock;	/* serialization lock */
+	bool __percpu *hotplug_ing;
+	atomic_t sys_suspended;
+	atomic_t sys_rebooting;
 };
 
 void __iomem *pmu_base_addr;
@@ -336,7 +342,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
 #define CPU_INFORM_CLEAR	0
 #define CPU_INFORM_C2		1
 
-static int gs101_cpuhp_pmu_online(unsigned int cpu)
+static int gs101_cpu_pmu_online(unsigned int cpu)
 {
 	unsigned int cpuhint = smp_processor_id();
 	u32 reg, mask;
@@ -358,10 +364,26 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
 	return 0;
 }
 
-static int gs101_cpuhp_pmu_offline(unsigned int cpu)
+static int gs101_cpuhp_pmu_online(unsigned int cpu)
+{
+	gs101_cpu_pmu_online(cpu);
+
+	/*
+	 * Mark this CPU as having finished the hotplug.
+	 * This means this CPU can now enter C2 idle state.
+	 */
+	*per_cpu_ptr(pmu_context->hotplug_ing, cpu) = false;
+
+	return 0;
+}
+
+static int gs101_cpu_pmu_offline(unsigned int cpu)
 {
 	u32 reg, mask;
-	unsigned int cpuhint = smp_processor_id();
+	unsigned int cpuhint;
+
+	spin_lock(&pmu_context->cpupm_lock);
+	cpuhint	= smp_processor_id();
 
 	/* set cpu inform hint */
 	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
@@ -379,16 +401,89 @@ static int gs101_cpuhp_pmu_offline(unsigned int cpu)
 	regmap_read(pmu_context->pmuintrgen, GS101_GRP1_INTR_BID_UPEND, &reg);
 	regmap_write(pmu_context->pmuintrgen, GS101_GRP1_INTR_BID_CLEAR,
 		     reg & mask);
+
+	spin_unlock(&pmu_context->cpupm_lock);
 	return 0;
 }
 
+static int gs101_cpuhp_pmu_offline(unsigned int cpu)
+{
+	/*
+	 * Mark this CPU as entering hotplug. So as not to confuse
+	 * ACPM the CPU entering hotplug should not enter C2 idle state.
+	 */
+	*per_cpu_ptr(pmu_context->hotplug_ing, cpu) = true;
+
+	gs101_cpu_pmu_offline(cpu);
+
+	return 0;
+}
+
+static int gs101_cpu_pm_notify_callback(struct notifier_block *self,
+					unsigned long action, void *v)
+{
+	int cpu = smp_processor_id();
+
+	switch (action) {
+	case CPU_PM_ENTER:
+		/*
+		 * Ignore CPU_PM_ENTER event in reboot or
+		 * suspend sequence.
+		 */
+
+		if (atomic_read(&pmu_context->sys_suspended) ||
+		    atomic_read(&pmu_context->sys_rebooting))
+			return NOTIFY_OK;
+
+		if (*per_cpu_ptr(pmu_context->hotplug_ing, cpu))
+			return NOTIFY_BAD;
+
+		gs101_cpu_pmu_offline(cpu);
+
+		break;
+	case CPU_PM_EXIT:
+
+		if (atomic_read(&pmu_context->sys_rebooting))
+			return NOTIFY_OK;
+
+		gs101_cpu_pmu_online(cpu);
+
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block gs101_cpu_pm_notifier = {
+	.notifier_call = gs101_cpu_pm_notify_callback,
+	.priority = INT_MAX	/* we want to be called first */
+};
+
+static int exynos_cpupm_reboot_notifier(struct notifier_block *nb,
+					unsigned long event, void *v)
+{
+	switch (event) {
+	case SYS_POWER_OFF:
+	case SYS_RESTART:
+		atomic_set(&pmu_context->sys_rebooting, 1);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block exynos_cpupm_reboot_nb = {
+	.priority = INT_MAX,
+	.notifier_call = exynos_cpupm_reboot_notifier,
+};
+
 static int exynos_pmu_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct regmap_config pmu_regmcfg;
 	struct regmap *regmap;
 	struct resource *res;
-	int ret;
+	int ret, cpu;
 
 	pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pmu_base_addr))
@@ -444,6 +539,12 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 			 */
 			dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
 		} else {
+			pmu_context->hotplug_ing = alloc_percpu(bool);
+
+			/* set PMU to power on */
+			for_each_online_cpu(cpu)
+				gs101_cpuhp_pmu_online(cpu);
+
 			cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
 					  "soc/exynos-pmu:prepare",
 					  gs101_cpuhp_pmu_online, NULL);
@@ -451,6 +552,12 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
 					  "soc/exynos-pmu:online",
 					  NULL, gs101_cpuhp_pmu_offline);
+
+			cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
+			spin_lock_init(&pmu_context->cpupm_lock);
+			atomic_set(&pmu_context->sys_rebooting, 0);
+			atomic_set(&pmu_context->sys_suspended, 0);
+			register_reboot_notifier(&exynos_cpupm_reboot_nb);
 		}
 	}
 
@@ -471,10 +578,32 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int exynos_cpupm_suspend_noirq(struct device *dev)
+{
+	atomic_set(&pmu_context->sys_suspended, 1);
+	return 0;
+}
+
+static int exynos_cpupm_resume_noirq(struct device *dev)
+{
+	atomic_set(&pmu_context->sys_suspended, 0);
+	return 0;
+}
+
+static const struct dev_pm_ops cpupm_pm_ops = {
+	.suspend_noirq = exynos_cpupm_suspend_noirq,
+	.resume_noirq = exynos_cpupm_resume_noirq,
+};
+#endif
+
 static struct platform_driver exynos_pmu_driver = {
 	.driver  = {
 		.name   = "exynos-pmu",
 		.of_match_table = exynos_pmu_of_device_ids,
+#ifdef CONFIG_PM_SLEEP
+		.pm = &cpupm_pm_ops,
+#endif
 	},
 	.probe = exynos_pmu_probe,
 };

-- 
2.50.0.rc1.591.g9c95f17f64-goog

Re: [PATCH v2 2/2] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Krzysztof Kozlowski]

On 11/06/2025 11:34, Peter Griffin wrote:
> Register cpu pm notifiers for gs101 which call the
> gs101_cpu_pmu_online/offline callbacks which in turn
> program the ACPM hint. This is required to actually
> enter the idle state.
> 
> A couple of corner cases are handled, namely when the
> system is rebooting or suspending we ignore the request.
> Additionally the request is ignored if the CPU is in
> CPU hot plug.
> 
> Note: this patch has a runtime dependency on adding
> 'local-timer-stop' dt property to the CPU nodes. This
> informs the time framework to switch to a broadcast timer
> as the local timer will be shutdown. Without that DT
> property specified the system hangs in early boot with
> this patch applied.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> Changes in v2
>  * Add ifdef CONFIG_PM_SLEEP to avoid
>    Fix warning: unused variable 'cpupm_pm_ops' [-Wunused-const-variable] (0-day)
> ---
>  drivers/soc/samsung/exynos-pmu.c | 137 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 133 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index a77288f49d249f890060c595556708334383c910..7f72ecd60994f18bb639dd8e09e1c6ff6158066b 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -8,6 +8,7 @@
>  #include <linux/array_size.h>
>  #include <linux/arm-smccc.h>
>  #include <linux/cpuhotplug.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/mfd/core.h>
> @@ -15,6 +16,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/delay.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  
>  #include <linux/soc/samsung/exynos-regs-pmu.h>
> @@ -35,6 +37,10 @@ struct exynos_pmu_context {
>  	const struct exynos_pmu_data *pmu_data;
>  	struct regmap *pmureg;
>  	struct regmap *pmuintrgen;
> +	spinlock_t cpupm_lock;	/* serialization lock */

serialization of what? Or rather, can it be not a serialization lock? Is
it possible? It's as useful as saying "protection against concurrent
accesses lock". No, you need to be explicit which members and/or code
are protected.

> +	bool __percpu *hotplug_ing;
> +	atomic_t sys_suspended;

Why re-implementing own refcnt of pm suspend status?
pm_runtime_suspended() and others?

> +	atomic_t sys_rebooting;
>  };
>  
>  void __iomem *pmu_base_addr;
> @@ -336,7 +342,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>  #define CPU_INFORM_CLEAR	0
>  #define CPU_INFORM_C2		1
>  
> -static int gs101_cpuhp_pmu_online(unsigned int cpu)
> +static int gs101_cpu_pmu_online(unsigned int cpu)
>  {
>  	unsigned int cpuhint = smp_processor_id();
>  	u32 reg, mask;
> @@ -358,10 +364,26 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
>  	return 0;
>  }
>  
> -static int gs101_cpuhp_pmu_offline(unsigned int cpu)
> +static int gs101_cpuhp_pmu_online(unsigned int cpu)

This needs either renaming or comments. One is cpu_pmu_online other is
cpuhp_pmu_online. Sounds the same to me.


> +{
> +	gs101_cpu_pmu_online(cpu);
> +
> +	/*
> +	 * Mark this CPU as having finished the hotplug.
> +	 * This means this CPU can now enter C2 idle state.
> +	 */
> +	*per_cpu_ptr(pmu_context->hotplug_ing, cpu) = false;

Quoting docs: "Per cpu data structures are designed to be used by one
cpu exclusively".

... and further about write access. Adding standard driver code using
"highly discouraged" practice is not something expected.


> +
> +	return 0;
> +}
> +
> +static int gs101_cpu_pmu_offline(unsigned int cpu)
>  {
>  	u32 reg, mask;
> -	unsigned int cpuhint = smp_processor_id();
> +	unsigned int cpuhint;
> +
> +	spin_lock(&pmu_context->cpupm_lock);

This does not disable interrupts...

> +	cpuhint	= smp_processor_id();

... which is a requirement here, according to docs, no? Maybe the
original code had an issue, though.

>  
>  	/* set cpu inform hint */
>  	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
> @@ -379,16 +401,89 @@ static int gs101_cpuhp_pmu_offline(unsigned int cpu)
>  	regmap_read(pmu_context->pmuintrgen, GS101_GRP1_INTR_BID_UPEND, &reg);
>  	regmap_write(pmu_context->pmuintrgen, GS101_GRP1_INTR_BID_CLEAR,
>  		     reg & mask);
> +
> +	spin_unlock(&pmu_context->cpupm_lock);
>  	return 0;
>  }
>  
> +static int gs101_cpuhp_pmu_offline(unsigned int cpu)
> +{
> +	/*
> +	 * Mark this CPU as entering hotplug. So as not to confuse
> +	 * ACPM the CPU entering hotplug should not enter C2 idle state.
> +	 */
> +	*per_cpu_ptr(pmu_context->hotplug_ing, cpu) = true;
> +
> +	gs101_cpu_pmu_offline(cpu);
> +
> +	return 0;
> +}
> +
> +static int gs101_cpu_pm_notify_callback(struct notifier_block *self,
> +					unsigned long action, void *v)
> +{
> +	int cpu = smp_processor_id();
> +
> +	switch (action) {
> +	case CPU_PM_ENTER:
> +		/*
> +		 * Ignore CPU_PM_ENTER event in reboot or
> +		 * suspend sequence.
> +		 */
> +
> +		if (atomic_read(&pmu_context->sys_suspended) ||
> +		    atomic_read(&pmu_context->sys_rebooting))
> +			return NOTIFY_OK;
> +
> +		if (*per_cpu_ptr(pmu_context->hotplug_ing, cpu))
> +			return NOTIFY_BAD;
> +
> +		gs101_cpu_pmu_offline(cpu);
> +
> +		break;
> +	case CPU_PM_EXIT:
> +
> +		if (atomic_read(&pmu_context->sys_rebooting))
> +			return NOTIFY_OK;
> +
> +		gs101_cpu_pmu_online(cpu);
> +
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block gs101_cpu_pm_notifier = {
> +	.notifier_call = gs101_cpu_pm_notify_callback,
> +	.priority = INT_MAX	/* we want to be called first */

You should say why. Everyone wants to be the first.

> +};
> +
> +static int exynos_cpupm_reboot_notifier(struct notifier_block *nb,
> +					unsigned long event, void *v)
> +{
> +	switch (event) {
> +	case SYS_POWER_OFF:
> +	case SYS_RESTART:
> +		atomic_set(&pmu_context->sys_rebooting, 1);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block exynos_cpupm_reboot_nb = {
> +	.priority = INT_MAX,
> +	.notifier_call = exynos_cpupm_reboot_notifier,
> +};
> +
>  static int exynos_pmu_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct regmap_config pmu_regmcfg;
>  	struct regmap *regmap;
>  	struct resource *res;
> -	int ret;
> +	int ret, cpu;
>  
>  	pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pmu_base_addr))
> @@ -444,6 +539,12 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>  			 */
>  			dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
>  		} else {
> +			pmu_context->hotplug_ing = alloc_percpu(bool);
> +
> +			/* set PMU to power on */
> +			for_each_online_cpu(cpu)
> +				gs101_cpuhp_pmu_online(cpu);
> +
>  			cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
>  					  "soc/exynos-pmu:prepare",
>  					  gs101_cpuhp_pmu_online, NULL);
> @@ -451,6 +552,12 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>  			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>  					  "soc/exynos-pmu:online",
>  					  NULL, gs101_cpuhp_pmu_offline);
> +
> +			cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
> +			spin_lock_init(&pmu_context->cpupm_lock);
> +			atomic_set(&pmu_context->sys_rebooting, 0);
> +			atomic_set(&pmu_context->sys_suspended, 0);
> +			register_reboot_notifier(&exynos_cpupm_reboot_nb);
>  		}
>  	}
>  
> @@ -471,10 +578,32 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int exynos_cpupm_suspend_noirq(struct device *dev)
> +{
> +	atomic_set(&pmu_context->sys_suspended, 1);
> +	return 0;
> +}
> +
> +static int exynos_cpupm_resume_noirq(struct device *dev)
> +{
> +	atomic_set(&pmu_context->sys_suspended, 0);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cpupm_pm_ops = {
> +	.suspend_noirq = exynos_cpupm_suspend_noirq,
> +	.resume_noirq = exynos_cpupm_resume_noirq,

SET_LATE_SYSTEM_SLEEP_PM_OPS or one of other wrappers.

> +};
> +#endif
> +
>  static struct platform_driver exynos_pmu_driver = {
>  	.driver  = {
>  		.name   = "exynos-pmu",
>  		.of_match_table = exynos_pmu_of_device_ids,
> +#ifdef CONFIG_PM_SLEEP
> +		.pm = &cpupm_pm_ops,

pm_ptr
> +#endif
>  	},
>  	.probe = exynos_pmu_probe,
>  };
> 


Best regards,
Krzysztof

Re: (subset) [PATCH v2 1/2] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[Krzysztof Kozlowski]

On Wed, 11 Jun 2025 10:34:25 +0100, Peter Griffin wrote:
> In preparation for switching to the architected timer as the primary
> clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
> property to indicate that an alternative clockevents device must be
> used for waking up from the "c2" idle state.
> 
> 

Applied, thanks!

[1/2] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes
      https://git.kernel.org/krzk/linux/c/b649082312dd1a4c3989bbdb7c25eb711e9b1d94

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Re: [PATCH v2 2/2] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Peter Griffin]

Hi Krzysztof,

Thanks a lot for your review feedback!

On Wed, 18 Jun 2025 at 11:22, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 11/06/2025 11:34, Peter Griffin wrote:
> > Register cpu pm notifiers for gs101 which call the
> > gs101_cpu_pmu_online/offline callbacks which in turn
> > program the ACPM hint. This is required to actually
> > enter the idle state.
> >
> > A couple of corner cases are handled, namely when the
> > system is rebooting or suspending we ignore the request.
> > Additionally the request is ignored if the CPU is in
> > CPU hot plug.
> >
> > Note: this patch has a runtime dependency on adding
> > 'local-timer-stop' dt property to the CPU nodes. This
> > informs the time framework to switch to a broadcast timer
> > as the local timer will be shutdown. Without that DT
> > property specified the system hangs in early boot with
> > this patch applied.
>
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Noted, will fix

>
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> > Changes in v2
> >  * Add ifdef CONFIG_PM_SLEEP to avoid
> >    Fix warning: unused variable 'cpupm_pm_ops' [-Wunused-const-variable] (0-day)
> > ---
> >  drivers/soc/samsung/exynos-pmu.c | 137 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 133 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> > index a77288f49d249f890060c595556708334383c910..7f72ecd60994f18bb639dd8e09e1c6ff6158066b 100644
> > --- a/drivers/soc/samsung/exynos-pmu.c
> > +++ b/drivers/soc/samsung/exynos-pmu.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/array_size.h>
> >  #include <linux/arm-smccc.h>
> >  #include <linux/cpuhotplug.h>
> > +#include <linux/cpu_pm.h>
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> >  #include <linux/mfd/core.h>
> > @@ -15,6 +16,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/delay.h>
> > +#include <linux/reboot.h>
> >  #include <linux/regmap.h>
> >
> >  #include <linux/soc/samsung/exynos-regs-pmu.h>
> > @@ -35,6 +37,10 @@ struct exynos_pmu_context {
> >       const struct exynos_pmu_data *pmu_data;
> >       struct regmap *pmureg;
> >       struct regmap *pmuintrgen;
> > +     spinlock_t cpupm_lock;  /* serialization lock */
>
> serialization of what? Or rather, can it be not a serialization lock? Is
> it possible? It's as useful as saying "protection against concurrent
> accesses lock". No, you need to be explicit which members and/or code
> are protected.

I can update the comment to be more verbose, but the lock is used to
ensure the cpu online/offline sequence called from CPU hotplug
callbacks and cpu pm notifiers are serialized.

>
> > +     bool __percpu *hotplug_ing;
> > +     atomic_t sys_suspended;
>
> Why re-implementing own refcnt of pm suspend status?
> pm_runtime_suspended() and others?

sys_suspended is being used to detect whether a *system* wide sleep
state is happening. I see a bunch of different drivers using a similar
approach in the kernel to set a flag from their suspend/resume
callback. Grep for things like system_suspending, is_suspending etc.
An alternative approach could be to use register_pm_notifier() and set
the flag from the callback there.

pm_runtime_suspended() tells me the runtime pm status, which is not
what I want here.

> > +     atomic_t sys_rebooting;
> >  };
> >
> >  void __iomem *pmu_base_addr;
> > @@ -336,7 +342,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
> >  #define CPU_INFORM_CLEAR     0
> >  #define CPU_INFORM_C2                1
> >
> > -static int gs101_cpuhp_pmu_online(unsigned int cpu)
> > +static int gs101_cpu_pmu_online(unsigned int cpu)
> >  {
> >       unsigned int cpuhint = smp_processor_id();
> >       u32 reg, mask;
> > @@ -358,10 +364,26 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
> >       return 0;
> >  }
> >
> > -static int gs101_cpuhp_pmu_offline(unsigned int cpu)
> > +static int gs101_cpuhp_pmu_online(unsigned int cpu)
>
> This needs either renaming or comments. One is cpu_pmu_online other is
> cpuhp_pmu_online. Sounds the same to me.

I can add some comments, but one function is specifically for CPU Hot
Plug, which is what the 'cpuhp' part was trying to convey.

>
>
> > +{
> > +     gs101_cpu_pmu_online(cpu);
> > +
> > +     /*
> > +      * Mark this CPU as having finished the hotplug.
> > +      * This means this CPU can now enter C2 idle state.
> > +      */
> > +     *per_cpu_ptr(pmu_context->hotplug_ing, cpu) = false;
>
> Quoting docs: "Per cpu data structures are designed to be used by one
> cpu exclusively".
>
> ... and further about write access. Adding standard driver code using
> "highly discouraged" practice is not something expected.

I'll update this to dynamically allocate based on num_possible_cpus()
and then read/write the flag with cpupm lock held. I didn't realize
the docs described the per_cpu remote writes as "highly discouraged
unless absolutely necessary", so thanks for highlighting that. The
per_cpu variables with remote writes seem quite widely used in the
downstream exynos-cpupm driver, but then it takes all sorts of locks
through all the different cal layers.

>
>
> > +
> > +     return 0;
> > +}
> > +
> > +static int gs101_cpu_pmu_offline(unsigned int cpu)
> >  {
> >       u32 reg, mask;
> > -     unsigned int cpuhint = smp_processor_id();
> > +     unsigned int cpuhint;
> > +
> > +     spin_lock(&pmu_context->cpupm_lock);
>
> This does not disable interrupts...
>
> > +     cpuhint = smp_processor_id();
>
> ... which is a requirement here, according to docs, no? Maybe the
> original code had an issue, though.

CPU notifiers are called with interrupts disabled. We do use a similar
pattern in the CPU hot plug path which isn't called with IRQs disabled
though, so I'll add some locking there in the next version.

Thanks,

Peter



>
> >
> >       /* set cpu inform hint */
> >       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
> > @@ -379,16 +401,89 @@ static int gs101_cpuhp_pmu_offline(unsigned int cpu)
> >       regmap_read(pmu_context->pmuintrgen, GS101_GRP1_INTR_BID_UPEND, &reg);
> >       regmap_write(pmu_context->pmuintrgen, GS101_GRP1_INTR_BID_CLEAR,
> >                    reg & mask);
> > +
> > +     spin_unlock(&pmu_context->cpupm_lock);
> >       return 0;
> >  }
> >
> > +static int gs101_cpuhp_pmu_offline(unsigned int cpu)
> > +{
> > +     /*
> > +      * Mark this CPU as entering hotplug. So as not to confuse
> > +      * ACPM the CPU entering hotplug should not enter C2 idle state.
> > +      */
> > +     *per_cpu_ptr(pmu_context->hotplug_ing, cpu) = true;
> > +
> > +     gs101_cpu_pmu_offline(cpu);
> > +
> > +     return 0;
> > +}
> > +
> > +static int gs101_cpu_pm_notify_callback(struct notifier_block *self,
> > +                                     unsigned long action, void *v)
> > +{
> > +     int cpu = smp_processor_id();
> > +
> > +     switch (action) {
> > +     case CPU_PM_ENTER:
> > +             /*
> > +              * Ignore CPU_PM_ENTER event in reboot or
> > +              * suspend sequence.
> > +              */
> > +
> > +             if (atomic_read(&pmu_context->sys_suspended) ||
> > +                 atomic_read(&pmu_context->sys_rebooting))
> > +                     return NOTIFY_OK;
> > +
> > +             if (*per_cpu_ptr(pmu_context->hotplug_ing, cpu))
> > +                     return NOTIFY_BAD;
> > +
> > +             gs101_cpu_pmu_offline(cpu);
> > +
> > +             break;
> > +     case CPU_PM_EXIT:
> > +
> > +             if (atomic_read(&pmu_context->sys_rebooting))
> > +                     return NOTIFY_OK;
> > +
> > +             gs101_cpu_pmu_online(cpu);
> > +
> > +             break;
> > +     }
> > +
> > +     return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block gs101_cpu_pm_notifier = {
> > +     .notifier_call = gs101_cpu_pm_notify_callback,
> > +     .priority = INT_MAX     /* we want to be called first */
>
> You should say why. Everyone wants to be the first.
>
> > +};
> > +
> > +static int exynos_cpupm_reboot_notifier(struct notifier_block *nb,
> > +                                     unsigned long event, void *v)
> > +{
> > +     switch (event) {
> > +     case SYS_POWER_OFF:
> > +     case SYS_RESTART:
> > +             atomic_set(&pmu_context->sys_rebooting, 1);
> > +             break;
> > +     }
> > +
> > +     return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block exynos_cpupm_reboot_nb = {
> > +     .priority = INT_MAX,
> > +     .notifier_call = exynos_cpupm_reboot_notifier,
> > +};
> > +
> >  static int exynos_pmu_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct regmap_config pmu_regmcfg;
> >       struct regmap *regmap;
> >       struct resource *res;
> > -     int ret;
> > +     int ret, cpu;
> >
> >       pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> >       if (IS_ERR(pmu_base_addr))
> > @@ -444,6 +539,12 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> >                        */
> >                       dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
> >               } else {
> > +                     pmu_context->hotplug_ing = alloc_percpu(bool);
> > +
> > +                     /* set PMU to power on */
> > +                     for_each_online_cpu(cpu)
> > +                             gs101_cpuhp_pmu_online(cpu);
> > +
> >                       cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
> >                                         "soc/exynos-pmu:prepare",
> >                                         gs101_cpuhp_pmu_online, NULL);
> > @@ -451,6 +552,12 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> >                       cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> >                                         "soc/exynos-pmu:online",
> >                                         NULL, gs101_cpuhp_pmu_offline);
> > +
> > +                     cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
> > +                     spin_lock_init(&pmu_context->cpupm_lock);
> > +                     atomic_set(&pmu_context->sys_rebooting, 0);
> > +                     atomic_set(&pmu_context->sys_suspended, 0);
> > +                     register_reboot_notifier(&exynos_cpupm_reboot_nb);
> >               }
> >       }
> >
> > @@ -471,10 +578,32 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int exynos_cpupm_suspend_noirq(struct device *dev)
> > +{
> > +     atomic_set(&pmu_context->sys_suspended, 1);
> > +     return 0;
> > +}
> > +
> > +static int exynos_cpupm_resume_noirq(struct device *dev)
> > +{
> > +     atomic_set(&pmu_context->sys_suspended, 0);
> > +     return 0;
> > +}
> > +
> > +static const struct dev_pm_ops cpupm_pm_ops = {
> > +     .suspend_noirq = exynos_cpupm_suspend_noirq,
> > +     .resume_noirq = exynos_cpupm_resume_noirq,
>
> SET_LATE_SYSTEM_SLEEP_PM_OPS or one of other wrappers.
>
> > +};
> > +#endif
> > +
> >  static struct platform_driver exynos_pmu_driver = {
> >       .driver  = {
> >               .name   = "exynos-pmu",
> >               .of_match_table = exynos_pmu_of_device_ids,
> > +#ifdef CONFIG_PM_SLEEP
> > +             .pm = &cpupm_pm_ops,
>
> pm_ptr
> > +#endif
> >       },
> >       .probe = exynos_pmu_probe,
> >  };
> >
>
>
> Best regards,
> Krzysztof