[PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[PATCH v3] 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 C2 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. Some common code is refactored so that it can
be called from both the CPU hot plug callback and CPU PM notifier taking
into account that CPU PM notifiers are called with IRQs disabled whereas
CPU hotplug callbacks aren't.

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>
---
Hi folks,

This patch 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 `local-timer-stop` DT patch mentioned above is already queued.

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/20250421-b4-gs101_max77759_fg-v3-0-50cd8caf9017@uclouvain.be/
---
Changes in v3:
- Add more verbose comment regarding spinlock (Krzysztof)
- Remove per-cpu hotplug_ing bool to avoid highly discouraged remote writes
  (Krzysztof)
- Add extra comments for similarly named functions  (Krzysztof)
- Initialize lock before for_each_online_cpu() in probe() (Peter)
- Use spin_lock_irqsave in cpu hot plug callbacks (Peter/Krzysztof)
- Rebase on next-20250627
- Link to v2: https://lore.kernel.org/r/20250611-gs101-cpuidle-v2-0-4fa811ec404d@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
---
 drivers/soc/samsung/exynos-pmu.c | 187 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 183 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index a77288f49d249f890060c595556708334383c910..a455f4514a49455171ac599d8fb48b6b88b83d21 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,14 @@ struct exynos_pmu_context {
 	const struct exynos_pmu_data *pmu_data;
 	struct regmap *pmureg;
 	struct regmap *pmuintrgen;
+	/*
+	 * Serialization lock for CPU hot plug and cpuidle ACPM hint
+	 * programming. Also protects the hotplug_ing flag.
+	 */
+	spinlock_t cpupm_lock;
+	bool *hotplug_ing;
+	atomic_t sys_suspended;
+	atomic_t sys_rebooting;
 };
 
 void __iomem *pmu_base_addr;
@@ -336,7 +346,12 @@ 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)
+/*
+ * __gs101_cpu_pmu_ prefix functions are common code shared by CPU PM notifiers
+ * (CPUIdle) and CPU hotplug callbacks. Functions should be called with IRQs
+ * disabled and cpupm_lock held.
+ */
+static int __gs101_cpu_pmu_online(unsigned int cpu)
 {
 	unsigned int cpuhint = smp_processor_id();
 	u32 reg, mask;
@@ -358,10 +373,42 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
 	return 0;
 }
 
-static int gs101_cpuhp_pmu_offline(unsigned int cpu)
+/* Called from CPU PM notifier (CPUIdle code path) with IRQs disabled */
+static int gs101_cpu_pmu_online(void)
+{
+	int cpu;
+
+	spin_lock(&pmu_context->cpupm_lock);
+	cpu = smp_processor_id();
+	__gs101_cpu_pmu_online(cpu);
+	spin_unlock(&pmu_context->cpupm_lock);
+
+	return 0;
+}
+
+/* Called from CPU hot plug callback with IRQs enabled */
+static int gs101_cpuhp_pmu_online(unsigned int cpu)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pmu_context->cpupm_lock, flags);
+
+	__gs101_cpu_pmu_online(cpu);
+	/*
+	 * Mark this CPU as having finished the hotplug.
+	 * This means this CPU can now enter C2 idle state.
+	 */
+	pmu_context->hotplug_ing[cpu] = false;
+	spin_unlock_irqrestore(&pmu_context->cpupm_lock, flags);
+
+	return 0;
+}
+
+/* Common function shared by both CPU hot plug and CPUIdle */
+static int __gs101_cpu_pmu_offline(unsigned int cpu)
 {
-	u32 reg, mask;
 	unsigned int cpuhint = smp_processor_id();
+	u32 reg, mask;
 
 	/* set cpu inform hint */
 	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
@@ -379,16 +426,111 @@ 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);
+
+	return 0;
+}
+
+/* Called from CPU PM notifier (CPUIdle code path) with IRQs disabled */
+static int gs101_cpu_pmu_offline(void)
+{
+	int cpu;
+
+	spin_lock(&pmu_context->cpupm_lock);
+	cpu = smp_processor_id();
+
+	if (pmu_context->hotplug_ing[cpu]) {
+		spin_unlock(&pmu_context->cpupm_lock);
+		return NOTIFY_BAD;
+	}
+
+	__gs101_cpu_pmu_offline(cpu);
+	spin_unlock(&pmu_context->cpupm_lock);
+
+	return NOTIFY_OK;
+}
+
+/* Called from CPU hot plug callback with IRQs enabled */
+static int gs101_cpuhp_pmu_offline(unsigned int cpu)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pmu_context->cpupm_lock, flags);
+	/*
+	 * Mark this CPU as entering hotplug. So as not to confuse
+	 * ACPM the CPU entering hotplug should not enter C2 idle state.
+	 */
+	pmu_context->hotplug_ing[cpu] = true;
+	__gs101_cpu_pmu_offline(cpu);
+
+	spin_unlock_irqrestore(&pmu_context->cpupm_lock, flags);
+
 	return 0;
 }
 
+static int gs101_cpu_pm_notify_callback(struct notifier_block *self,
+					unsigned long action, void *v)
+{
+	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;
+
+		return gs101_cpu_pmu_offline();
+
+		break;
+	case CPU_PM_EXIT:
+
+		if (atomic_read(&pmu_context->sys_rebooting))
+			return NOTIFY_OK;
+
+		return gs101_cpu_pmu_online();
+
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block gs101_cpu_pm_notifier = {
+	.notifier_call = gs101_cpu_pm_notify_callback,
+	/*
+	 * We want to be called first, as the ACPM hint and handshake is what
+	 * puts the CPU into C2.
+	 */
+	.priority = INT_MAX
+};
+
+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 +586,18 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 			 */
 			dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
 		} else {
+			pmu_context->hotplug_ing =
+				devm_kmalloc_array(dev, num_possible_cpus(),
+						   sizeof(bool), GFP_KERNEL);
+
+			spin_lock_init(&pmu_context->cpupm_lock);
+			atomic_set(&pmu_context->sys_rebooting, 0);
+			atomic_set(&pmu_context->sys_suspended, 0);
+
+			/* 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 +605,9 @@ 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);
+			register_reboot_notifier(&exynos_cpupm_reboot_nb);
 		}
 	}
 
@@ -471,10 +628,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,
 };

---
base-commit: 2aeda9592360c200085898a258c4754bfe879921
change-id: 20250524-gs101-cpuidle-cafc96dd849d

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

Re: [PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[André Draszik]

Thanks Pete for your patch!

On Fri, 2025-06-27 at 14:08 +0100, 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 C2 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. Some common code is refactored so that it can
> be called from both the CPU hot plug callback and CPU PM notifier taking
> into account that CPU PM notifiers are called with IRQs disabled whereas
> CPU hotplug callbacks aren't.
> 
> 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>

With this applied, I see the following during boot:

[    1.841304][    T0] =============================
[    1.841422][    T0] [ BUG: Invalid wait context ]
[    1.841550][    T0] 6.16.0-rc4-next-20250702+ #54 Tainted: G     U          T  
[    1.841727][    T0] -----------------------------
[    1.841844][    T0] swapper/0/0 is trying to lock:
[    1.841965][    T0] ffff000800ee84b8 (&pmu_context->cpupm_lock){....}-{3:3}
[    1.842001][    T0] , at: gs101_cpu_pm_notify_callback+0x48/0x100
[    1.842309][    T0] other info that might help us debug this:
[    1.842613][    T0] context-{5:5}
[    1.842987][    T0] 1 lock held by swapper/0/0:
[    1.843442][    T0]  #0: 
[    1.843859][    T0] ffffafe9d8f1f100
[    1.844282][    T0]  (
[    1.844618][    T0] cpu_pm_notifier.lock
[    1.844980][    T0] ){....}-{2:2}, at: cpu_pm_enter+0x30/0x88
[    1.845340][    T0] stack backtrace:
[    1.845855][    T0] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G     U          T   6.16.0-rc4-next-20250702+ #54 PREEMPT 
[    1.845878][    T0] Tainted: [U]=USER, [T]=RANDSTRUCT
[    1.845884][    T0] Hardware name: Oriole (DT)
[    1.845897][    T0] Call trace:
[    1.845909][    T0]  show_stack+0x24/0x38 (C)
[    1.845934][    T0]  dump_stack_lvl+0x40/0xc0
[    1.845949][    T0]  dump_stack+0x18/0x24
[    1.845956][    T0]  __lock_acquire+0xc68/0xd90
[    1.845976][    T0]  lock_acquire+0x14c/0x2b8
[    1.845985][    T0]  _raw_spin_lock+0x54/0x78
[    1.846011][    T0]  gs101_cpu_pm_notify_callback+0x48/0x100
[    1.846021][    T0]  notifier_call_chain+0xb0/0x198
[    1.846046][    T0]  raw_notifier_call_chain_robust+0x50/0xb0
[    1.846053][    T0]  cpu_pm_enter+0x4c/0x88
[    1.846063][    T0]  psci_enter_idle_state+0x2c/0x70
[    1.846078][    T0]  cpuidle_enter_state+0x14c/0x4c0
[    1.846097][    T0]  cpuidle_enter+0x44/0x68
[    1.846121][    T0]  do_idle+0x1f0/0x2a8
[    1.846142][    T0]  cpu_startup_entry+0x40/0x50
[    1.846152][    T0]  rest_init+0x1c4/0x1d0
[    1.846161][    T0]  start_kernel+0x358/0x438
[    1.846181][    T0]  __primary_switched+0x88/0x98

> 
> [...]
> 
> @@ -444,6 +586,18 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>  			 */
>  			dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
>  		} else {
> +			pmu_context->hotplug_ing =
> +				devm_kmalloc_array(dev, num_possible_cpus(),
> +						   sizeof(bool), GFP_KERNEL);

I haven't done a full review, but sizeof(bool) should be rewritten as
sizeof(*pmu_context->hotplug_ing)

> [...]
>  
> @@ -471,10 +628,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

This can use pm_sleep_ptr() instead to avoid the ifdef.

Cheers,
Andre'

Re: [PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Krzysztof Kozlowski]

On 03/07/2025 12:12, André Draszik wrote:
> Thanks Pete for your patch!
> 
> On Fri, 2025-06-27 at 14:08 +0100, 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 C2 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. Some common code is refactored so that it can
>> be called from both the CPU hot plug callback and CPU PM notifier taking
>> into account that CPU PM notifiers are called with IRQs disabled whereas
>> CPU hotplug callbacks aren't.
>>
>> 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>
> 
> With this applied, I see the following during boot:
> 
> [    1.841304][    T0] =============================
> [    1.841422][    T0] [ BUG: Invalid wait context ]
> [    1.841550][    T0] 6.16.0-rc4-next-20250702+ #54 Tainted: G     U          T  
> [    1.841727][    T0] -----------------------------
> [    1.841844][    T0] swapper/0/0 is trying to lock:
> [    1.841965][    T0] ffff000800ee84b8 (&pmu_context->cpupm_lock){....}-{3:3}
> [    1.842001][    T0] , at: gs101_cpu_pm_notify_callback+0x48/0x100
> [    1.842309][    T0] other info that might help us debug this:
> [    1.842613][    T0] context-{5:5}
> [    1.842987][    T0] 1 lock held by swapper/0/0:
> [    1.843442][    T0]  #0: 
> [    1.843859][    T0] ffffafe9d8f1f100
> [    1.844282][    T0]  (
> [    1.844618][    T0] cpu_pm_notifier.lock
> [    1.844980][    T0] ){....}-{2:2}, at: cpu_pm_enter+0x30/0x88
> [    1.845340][    T0] stack backtrace:
> [    1.845855][    T0] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G     U          T   6.16.0-rc4-next-20250702+ #54 PREEMPT 
> [    1.845878][    T0] Tainted: [U]=USER, [T]=RANDSTRUCT
> [    1.845884][    T0] Hardware name: Oriole (DT)
> [    1.845897][    T0] Call trace:
> [    1.845909][    T0]  show_stack+0x24/0x38 (C)
> [    1.845934][    T0]  dump_stack_lvl+0x40/0xc0
> [    1.845949][    T0]  dump_stack+0x18/0x24
> [    1.845956][    T0]  __lock_acquire+0xc68/0xd90
> [    1.845976][    T0]  lock_acquire+0x14c/0x2b8
> [    1.845985][    T0]  _raw_spin_lock+0x54/0x78
> [    1.846011][    T0]  gs101_cpu_pm_notify_callback+0x48/0x100
> [    1.846021][    T0]  notifier_call_chain+0xb0/0x198
> [    1.846046][    T0]  raw_notifier_call_chain_robust+0x50/0xb0
> [    1.846053][    T0]  cpu_pm_enter+0x4c/0x88
> [    1.846063][    T0]  psci_enter_idle_state+0x2c/0x70
> [    1.846078][    T0]  cpuidle_enter_state+0x14c/0x4c0
> [    1.846097][    T0]  cpuidle_enter+0x44/0x68
> [    1.846121][    T0]  do_idle+0x1f0/0x2a8
> [    1.846142][    T0]  cpu_startup_entry+0x40/0x50
> [    1.846152][    T0]  rest_init+0x1c4/0x1d0
> [    1.846161][    T0]  start_kernel+0x358/0x438
> [    1.846181][    T0]  __primary_switched+0x88/0x98

Thanks for the report. Probably such code should be always tested with
lock debugging options enabled, e.g.:
https://github.com/krzk/tools/blob/master/linux/build.sh#L845

Best regards,
Krzysztof

Re: [PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[André Draszik]

More small comments. Sorry for the drip feed, just trying to understand
things...

On Fri, 2025-06-27 at 14:08 +0100, Peter Griffin wrote:
> [...]
>  
> +static int gs101_cpu_pm_notify_callback(struct notifier_block *self,
> +					unsigned long action, void *v)
> +{
> +	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;
> +
> +		return gs101_cpu_pmu_offline();
> +
> +		break;

break is not needed after return, and generally there should be an empty
line before the next case statement.

> +	case CPU_PM_EXIT:

Should this also handle CPU_PM_ENTER_FAILED in the same way to bring
the CPU back up in case of failures?

> +
> +		if (atomic_read(&pmu_context->sys_rebooting))
> +			return NOTIFY_OK;
> +
> +		return gs101_cpu_pmu_online();
> +
> +		break;

No break needed.

Cheers,
Andre'

Re: [PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Peter Griffin]

Hi André,

On Thu, 3 Jul 2025 at 11:12, André Draszik <andre.draszik@linaro.org> wrote:
>
> Thanks Pete for your patch!
>
> On Fri, 2025-06-27 at 14:08 +0100, 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 C2 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. Some common code is refactored so that it can
> > be called from both the CPU hot plug callback and CPU PM notifier taking
> > into account that CPU PM notifiers are called with IRQs disabled whereas
> > CPU hotplug callbacks aren't.
> >
> > 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>
>
> With this applied, I see the following during boot:

Thanks for the lockdep report. It has turned out to be a bit of a
lockdep rabbit hole!
To fix this I've had to: -
1) Update to use raw_spin_lock() variants from the callbacks
2) Set .use_raw_spinlock = true  in regmap_smccfg
3) Create a new custom regmap_pmu_intr (instead of relying on the
syscon generated mmio regmap) for the pmu_intr_gen region that sets
.use_raw_spinlock = true

>
> [    1.841304][    T0] =============================
> [    1.841422][    T0] [ BUG: Invalid wait context ]
> [    1.841550][    T0] 6.16.0-rc4-next-20250702+ #54 Tainted: G     U          T
> [    1.841727][    T0] -----------------------------
> [    1.841844][    T0] swapper/0/0 is trying to lock:
> [    1.841965][    T0] ffff000800ee84b8 (&pmu_context->cpupm_lock){....}-{3:3}
> [    1.842001][    T0] , at: gs101_cpu_pm_notify_callback+0x48/0x100
> [    1.842309][    T0] other info that might help us debug this:
> [    1.842613][    T0] context-{5:5}
> [    1.842987][    T0] 1 lock held by swapper/0/0:
> [    1.843442][    T0]  #0:
> [    1.843859][    T0] ffffafe9d8f1f100
> [    1.844282][    T0]  (
> [    1.844618][    T0] cpu_pm_notifier.lock
> [    1.844980][    T0] ){....}-{2:2}, at: cpu_pm_enter+0x30/0x88
> [    1.845340][    T0] stack backtrace:
> [    1.845855][    T0] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G     U          T   6.16.0-rc4-next-20250702+ #54 PREEMPT
> [    1.845878][    T0] Tainted: [U]=USER, [T]=RANDSTRUCT
> [    1.845884][    T0] Hardware name: Oriole (DT)
> [    1.845897][    T0] Call trace:
> [    1.845909][    T0]  show_stack+0x24/0x38 (C)
> [    1.845934][    T0]  dump_stack_lvl+0x40/0xc0
> [    1.845949][    T0]  dump_stack+0x18/0x24
> [    1.845956][    T0]  __lock_acquire+0xc68/0xd90
> [    1.845976][    T0]  lock_acquire+0x14c/0x2b8
> [    1.845985][    T0]  _raw_spin_lock+0x54/0x78
> [    1.846011][    T0]  gs101_cpu_pm_notify_callback+0x48/0x100
> [    1.846021][    T0]  notifier_call_chain+0xb0/0x198
> [    1.846046][    T0]  raw_notifier_call_chain_robust+0x50/0xb0
> [    1.846053][    T0]  cpu_pm_enter+0x4c/0x88
> [    1.846063][    T0]  psci_enter_idle_state+0x2c/0x70
> [    1.846078][    T0]  cpuidle_enter_state+0x14c/0x4c0
> [    1.846097][    T0]  cpuidle_enter+0x44/0x68
> [    1.846121][    T0]  do_idle+0x1f0/0x2a8
> [    1.846142][    T0]  cpu_startup_entry+0x40/0x50
> [    1.846152][    T0]  rest_init+0x1c4/0x1d0
> [    1.846161][    T0]  start_kernel+0x358/0x438
> [    1.846181][    T0]  __primary_switched+0x88/0x98
>
> >
> > [...]
> >
> > @@ -444,6 +586,18 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> >                        */
> >                       dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
> >               } else {
> > +                     pmu_context->hotplug_ing =
> > +                             devm_kmalloc_array(dev, num_possible_cpus(),
> > +                                                sizeof(bool), GFP_KERNEL);
>
> I haven't done a full review, but sizeof(bool) should be rewritten as
> sizeof(*pmu_context->hotplug_ing)

Will fix

>
> > [...]
> >
> > @@ -471,10 +628,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
>
> This can use pm_sleep_ptr() instead to avoid the ifdef.

Will fix.

Thanks,

Peter

Re: [PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Peter Griffin]

Hi Krzysztof,

On Thu, 3 Jul 2025 at 11:19, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/07/2025 12:12, André Draszik wrote:
> > Thanks Pete for your patch!
> >
> > On Fri, 2025-06-27 at 14:08 +0100, 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 C2 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. Some common code is refactored so that it can
> >> be called from both the CPU hot plug callback and CPU PM notifier taking
> >> into account that CPU PM notifiers are called with IRQs disabled whereas
> >> CPU hotplug callbacks aren't.
> >>
> >> 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>
> >
> > With this applied, I see the following during boot:
> >
> > [    1.841304][    T0] =============================
> > [    1.841422][    T0] [ BUG: Invalid wait context ]
> > [    1.841550][    T0] 6.16.0-rc4-next-20250702+ #54 Tainted: G     U          T
> > [    1.841727][    T0] -----------------------------
> > [    1.841844][    T0] swapper/0/0 is trying to lock:
> > [    1.841965][    T0] ffff000800ee84b8 (&pmu_context->cpupm_lock){....}-{3:3}
> > [    1.842001][    T0] , at: gs101_cpu_pm_notify_callback+0x48/0x100
> > [    1.842309][    T0] other info that might help us debug this:
> > [    1.842613][    T0] context-{5:5}
> > [    1.842987][    T0] 1 lock held by swapper/0/0:
> > [    1.843442][    T0]  #0:
> > [    1.843859][    T0] ffffafe9d8f1f100
> > [    1.844282][    T0]  (
> > [    1.844618][    T0] cpu_pm_notifier.lock
> > [    1.844980][    T0] ){....}-{2:2}, at: cpu_pm_enter+0x30/0x88
> > [    1.845340][    T0] stack backtrace:
> > [    1.845855][    T0] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G     U          T   6.16.0-rc4-next-20250702+ #54 PREEMPT
> > [    1.845878][    T0] Tainted: [U]=USER, [T]=RANDSTRUCT
> > [    1.845884][    T0] Hardware name: Oriole (DT)
> > [    1.845897][    T0] Call trace:
> > [    1.845909][    T0]  show_stack+0x24/0x38 (C)
> > [    1.845934][    T0]  dump_stack_lvl+0x40/0xc0
> > [    1.845949][    T0]  dump_stack+0x18/0x24
> > [    1.845956][    T0]  __lock_acquire+0xc68/0xd90
> > [    1.845976][    T0]  lock_acquire+0x14c/0x2b8
> > [    1.845985][    T0]  _raw_spin_lock+0x54/0x78
> > [    1.846011][    T0]  gs101_cpu_pm_notify_callback+0x48/0x100
> > [    1.846021][    T0]  notifier_call_chain+0xb0/0x198
> > [    1.846046][    T0]  raw_notifier_call_chain_robust+0x50/0xb0
> > [    1.846053][    T0]  cpu_pm_enter+0x4c/0x88
> > [    1.846063][    T0]  psci_enter_idle_state+0x2c/0x70
> > [    1.846078][    T0]  cpuidle_enter_state+0x14c/0x4c0
> > [    1.846097][    T0]  cpuidle_enter+0x44/0x68
> > [    1.846121][    T0]  do_idle+0x1f0/0x2a8
> > [    1.846142][    T0]  cpu_startup_entry+0x40/0x50
> > [    1.846152][    T0]  rest_init+0x1c4/0x1d0
> > [    1.846161][    T0]  start_kernel+0x358/0x438
> > [    1.846181][    T0]  __primary_switched+0x88/0x98
>
> Thanks for the report. Probably such code should be always tested with
> lock debugging options enabled, e.g.:
> https://github.com/krzk/tools/blob/master/linux/build.sh#L845

Thanks for the tip, I will do it for future versions.

Peter

Re: [PATCH v3] soc: samsung: exynos-pmu: Enable CPU Idle for gs101

[Peter Griffin]

Hi André,

Thanks for the review feedback!

On Thu, 3 Jul 2025 at 12:01, André Draszik <andre.draszik@linaro.org> wrote:
>
> More small comments. Sorry for the drip feed, just trying to understand
> things...
>
> On Fri, 2025-06-27 at 14:08 +0100, Peter Griffin wrote:
> > [...]
> >
> > +static int gs101_cpu_pm_notify_callback(struct notifier_block *self,
> > +                                     unsigned long action, void *v)
> > +{
> > +     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;
> > +
> > +             return gs101_cpu_pmu_offline();
> > +
> > +             break;
>
> break is not needed after return, and generally there should be an empty
> line before the next case statement.

Will fix
>
> > +     case CPU_PM_EXIT:
>
> Should this also handle CPU_PM_ENTER_FAILED in the same way to bring
> the CPU back up in case of failures?

I choose not to do that, mainly because the downstream production
drivers don't handle CPU_PM_ENTER_FAILED, and without access to the
firmware code it is hard to reason about.

Logically it seems like we would want to do the same code as
CPU_PM_EXIT with a CPU_PM_ENTER_FAILED, but I've never seen
CPU_PM_FAILED so far in my debugging.

>
> > +
> > +             if (atomic_read(&pmu_context->sys_rebooting))
> > +                     return NOTIFY_OK;
> > +
> > +             return gs101_cpu_pmu_online();
> > +
> > +             break;
>
> No break needed.

Will fix

Thanks.

Peter