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

[PATCH v6] 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
C2 hint. This hint is required to actually enter the C2 idle state in
addition to the PSCI calls due to limitations in the firmare.

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 callbacks and CPU PM notifier taking
into account that CPU PM notifiers are called with IRQs disabled whereas
CPU hotplug callbacks are not.

Additionally due to CPU PM notifiers using raw_spinlock the locking is
updated to use raw_spinlock variants, this includes updating the pmu_regs
regmap to use .use_raw_spinlock = true and additionally creating and
registering a custom  pmu-intr-gen regmap instead of using the regmap
provided by syscon.

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. This is
required in addition to the PSCI calls to enter the c2 idle state due to
limitations in the el3mon/ACPM firmware.

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 v6:
- Add more verbose comment on why the hint values are required for gs101
  CPU hotplug & CPU Idle states in addition to standard PSCI calls (Sudeep) 
- Link to v5: https://lore.kernel.org/r/20250709-gs101-cpuidle-v5-1-b34d3210286d@linaro.org

Changes in v5:
- Rename hotplug_ing flag to in_hotplug to aid readability
- Use NOIRQ_SYSTEM_SLEEP_PM_OPS wrapper for dev_pm_ops (Krzysztof)
- Link to v4: https://lore.kernel.org/r/20250709-gs101-cpuidle-v4-1-56e21dd24963@linaro.org

Changes in v4:
- Avoid lockdep [ BUG: Invalid wait context ] on boot (André)
  - Updated callbacks to use raw_spinlock variants
  - Ensure pmu_regs regmap uses a raw_spinlock
  - Create pmu-intr-gen regmap that uses a raw_spinlock
- Use pm_sleep_ptr to avoid #ifdef (André)
- Refactor CPU hotplug and cpuidle parts into dedicated function
- Remove unnecessary break; statement (André)
- Link to v3: https://lore.kernel.org/r/20250627-gs101-cpuidle-v3-1-0200452dfe60@linaro.org

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 | 266 +++++++++++++++++++++++++++++++++++----
 1 file changed, 244 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index a77288f49d249f890060c595556708334383c910..88ed3f1744ab0a5e4b890a27ff1948a86c04fe3b 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 in_hotplug flag.
+	 */
+	raw_spinlock_t cpupm_lock;
+	bool *in_hotplug;
+	atomic_t sys_suspended;
+	atomic_t sys_rebooting;
 };
 
 void __iomem *pmu_base_addr;
@@ -221,6 +231,15 @@ static const struct regmap_config regmap_smccfg = {
 	.reg_read = tensor_sec_reg_read,
 	.reg_write = tensor_sec_reg_write,
 	.reg_update_bits = tensor_sec_update_bits,
+	.use_raw_spinlock = true,
+};
+
+static const struct regmap_config regmap_pmu_intr = {
+	.name = "pmu_intr_gen",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.use_raw_spinlock = true,
 };
 
 static const struct exynos_pmu_data gs101_pmu_data = {
@@ -330,13 +349,19 @@ struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
 EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
 
 /*
- * CPU_INFORM register hint values which are used by
- * EL3 firmware (el3mon).
+ * CPU_INFORM register "hint" values are required to be programmed in addition to
+ * the standard PSCI calls to have functional CPU hotplug and CPU idle states.
+ * This is required to workaround limitations in the el3mon/ACPM firmware.
  */
 #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 +383,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;
+
+	raw_spin_lock(&pmu_context->cpupm_lock);
+	cpu = smp_processor_id();
+	__gs101_cpu_pmu_online(cpu);
+	raw_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;
+
+	raw_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->in_hotplug[cpu] = false;
+	raw_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,6 +436,167 @@ 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;
+
+	raw_spin_lock(&pmu_context->cpupm_lock);
+	cpu = smp_processor_id();
+
+	if (pmu_context->in_hotplug[cpu]) {
+		raw_spin_unlock(&pmu_context->cpupm_lock);
+		return NOTIFY_BAD;
+	}
+
+	__gs101_cpu_pmu_offline(cpu);
+	raw_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;
+
+	raw_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->in_hotplug[cpu] = true;
+	__gs101_cpu_pmu_offline(cpu);
+
+	raw_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();
+
+	case CPU_PM_EXIT:
+
+		if (atomic_read(&pmu_context->sys_rebooting))
+			return NOTIFY_OK;
+
+		return gs101_cpu_pmu_online();
+	}
+
+	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 setup_cpuhp_and_cpuidle(struct device *dev)
+{
+	struct device_node *intr_gen_node;
+	struct resource intrgen_res;
+	void __iomem *virt_addr;
+	int ret, cpu;
+
+	intr_gen_node = of_parse_phandle(dev->of_node,
+					 "google,pmu-intr-gen-syscon", 0);
+	if (!intr_gen_node) {
+		/*
+		 * To maintain support for older DTs that didn't specify syscon
+		 * phandle just issue a warning rather than fail to probe.
+		 */
+		dev_warn(dev, "pmu-intr-gen syscon unavailable\n");
+		return 0;
+	}
+
+	/*
+	 * To avoid lockdep issues (CPU PM notifiers use raw spinlocks) create
+	 * a mmio regmap for pmu-intr-gen that uses raw spinlocks instead of
+	 * syscon provided regmap.
+	 */
+	ret = of_address_to_resource(intr_gen_node, 0, &intrgen_res);
+	of_node_put(intr_gen_node);
+
+	virt_addr = devm_ioremap(dev, intrgen_res.start,
+				 resource_size(&intrgen_res));
+	if (!virt_addr)
+		return -ENOMEM;
+
+	pmu_context->pmuintrgen = devm_regmap_init_mmio(dev, virt_addr,
+							&regmap_pmu_intr);
+	if (IS_ERR(pmu_context->pmuintrgen)) {
+		dev_err(dev, "failed to initialize pmu-intr-gen regmap\n");
+		return PTR_ERR(pmu_context->pmuintrgen);
+	}
+
+	/* register custom mmio regmap with syscon */
+	ret = of_syscon_register_regmap(intr_gen_node,
+					pmu_context->pmuintrgen);
+	if (ret)
+		return ret;
+
+	pmu_context->in_hotplug = devm_kmalloc_array(dev, num_possible_cpus(),
+						     sizeof(*pmu_context->in_hotplug),
+						     GFP_KERNEL);
+
+	raw_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);
+
+	/* register CPU hotplug callbacks */
+	cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,	"soc/exynos-pmu:prepare",
+			  gs101_cpuhp_pmu_online, NULL);
+
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
+			  NULL, gs101_cpuhp_pmu_offline);
+
+	/* register CPU PM notifiers for cpuidle */
+	cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
+	register_reboot_notifier(&exynos_cpupm_reboot_nb);
 	return 0;
 }
 
@@ -435,23 +653,9 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 	pmu_context->dev = dev;
 
 	if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_cpuhp) {
-		pmu_context->pmuintrgen = syscon_regmap_lookup_by_phandle(dev->of_node,
-							"google,pmu-intr-gen-syscon");
-		if (IS_ERR(pmu_context->pmuintrgen)) {
-			/*
-			 * To maintain support for older DTs that didn't specify syscon phandle
-			 * just issue a warning rather than fail to probe.
-			 */
-			dev_warn(&pdev->dev, "pmu-intr-gen syscon unavailable\n");
-		} else {
-			cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
-					  "soc/exynos-pmu:prepare",
-					  gs101_cpuhp_pmu_online, NULL);
-
-			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
-					  "soc/exynos-pmu:online",
-					  NULL, gs101_cpuhp_pmu_offline);
-		}
+		ret = setup_cpuhp_and_cpuidle(dev);
+		if (ret)
+			return ret;
 	}
 
 	if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init)
@@ -471,10 +675,28 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 	return 0;
 }
 
+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 = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(exynos_cpupm_suspend_noirq,
+				  exynos_cpupm_resume_noirq)
+};
+
 static struct platform_driver exynos_pmu_driver = {
 	.driver  = {
 		.name   = "exynos-pmu",
 		.of_match_table = exynos_pmu_of_device_ids,
+		.pm = pm_sleep_ptr(&cpupm_pm_ops),
 	},
 	.probe = exynos_pmu_probe,
 };

---
base-commit: c44dd082b6696fc5bb3458ad28d1beb6153b8e6e
change-id: 20250524-gs101-cpuidle-cafc96dd849d

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

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

[Sudeep Holla]

On Fri, Jul 11, 2025 at 02:50:26PM +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
> C2 hint. This hint is required to actually enter the C2 idle state in
> addition to the PSCI calls due to limitations in the firmare.
> 
> 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 callbacks and CPU PM notifier taking
> into account that CPU PM notifiers are called with IRQs disabled whereas
> CPU hotplug callbacks are not.
> 
> Additionally due to CPU PM notifiers using raw_spinlock the locking is
> updated to use raw_spinlock variants, this includes updating the pmu_regs
> regmap to use .use_raw_spinlock = true and additionally creating and
> registering a custom  pmu-intr-gen regmap instead of using the regmap
> provided by syscon.
> 
> 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. This is
> required in addition to the PSCI calls to enter the c2 idle state due to
> limitations in the el3mon/ACPM firmware.
> 

I would rather keep the above note as part of the commit message or the
code comment as this will get lost when the patch is applied which is not
something we want. I clearly want to loudly shout or shame the broken
firmware for not getting this right.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On Fri, Jul 11, 2025 at 03:53:17PM +0100, Sudeep Holla wrote:
> On Fri, Jul 11, 2025 at 02:50:26PM +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
> > C2 hint. This hint is required to actually enter the C2 idle state in
> > addition to the PSCI calls due to limitations in the firmare.
> > 
> > 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 callbacks and CPU PM notifier taking
> > into account that CPU PM notifiers are called with IRQs disabled whereas
> > CPU hotplug callbacks are not.
> > 
> > Additionally due to CPU PM notifiers using raw_spinlock the locking is
> > updated to use raw_spinlock variants, this includes updating the pmu_regs
> > regmap to use .use_raw_spinlock = true and additionally creating and
> > registering a custom  pmu-intr-gen regmap instead of using the regmap
> > provided by syscon.
> > 
> > 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. This is
> > required in addition to the PSCI calls to enter the c2 idle state due to
> > limitations in the el3mon/ACPM firmware.
> > 
> 
> I would rather keep the above note as part of the commit message or the
> code comment as this will get lost when the patch is applied which is not
> something we want. I clearly want to loudly shout or shame the broken
> firmware for not getting this right.
> 

I did see the comment but still thought it is worth adding the note in the
commit log too. Sorry for referring code comment above which is wrong as it
is already taken care.

-- 
Regards,
Sudeep

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

[Peter Griffin]

Hi Sudeep,

On Fri, 11 Jul 2025 at 15:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Jul 11, 2025 at 03:53:17PM +0100, Sudeep Holla wrote:
> > On Fri, Jul 11, 2025 at 02:50:26PM +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
> > > C2 hint. This hint is required to actually enter the C2 idle state in
> > > addition to the PSCI calls due to limitations in the firmare.
> > >
> > > 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 callbacks and CPU PM notifier taking
> > > into account that CPU PM notifiers are called with IRQs disabled whereas
> > > CPU hotplug callbacks are not.
> > >
> > > Additionally due to CPU PM notifiers using raw_spinlock the locking is
> > > updated to use raw_spinlock variants, this includes updating the pmu_regs
> > > regmap to use .use_raw_spinlock = true and additionally creating and
> > > registering a custom  pmu-intr-gen regmap instead of using the regmap
> > > provided by syscon.
> > >
> > > 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. This is
> > > required in addition to the PSCI calls to enter the c2 idle state due to
> > > limitations in the el3mon/ACPM firmware.
> > >
> >
> > I would rather keep the above note as part of the commit message or the
> > code comment as this will get lost when the patch is applied which is not
> > something we want. I clearly want to loudly shout or shame the broken
> > firmware for not getting this right.
> >
>
> I did see the comment but still thought it is worth adding the note in the
> commit log too. Sorry for referring code comment above which is wrong as it
> is already taken care.

I mentioned it in the commit log, a comment in the code and also the
cover letter so I think we should be all good? In the commit log it is
part of the first paragraph.

Thanks,

Peter

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

[Krzysztof Kozlowski]

On 11/07/2025 15:50, Peter Griffin wrote:
>  
>  #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 in_hotplug flag.
> +	 */
> +	raw_spinlock_t cpupm_lock;
> +	bool *in_hotplug;

This should be bitmap - more obvious code.

> +	atomic_t sys_suspended;
> +	atomic_t sys_rebooting;
>  };
>  
>  void __iomem *pmu_base_addr;
> @@ -221,6 +231,15 @@ static const struct regmap_config regmap_smccfg = {
>  	.reg_read = tensor_sec_reg_read,
>  	.reg_write = tensor_sec_reg_write,
>  	.reg_update_bits = tensor_sec_update_bits,
> +	.use_raw_spinlock = true,
> +};
> +
> +static const struct regmap_config regmap_pmu_intr = {
> +	.name = "pmu_intr_gen",
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.use_raw_spinlock = true,
>  };
>  
>  static const struct exynos_pmu_data gs101_pmu_data = {
> @@ -330,13 +349,19 @@ struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
>  EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>  


...

> +/* Called from CPU PM notifier (CPUIdle code path) with IRQs disabled */
> +static int gs101_cpu_pmu_offline(void)
> +{
> +	int cpu;
> +
> +	raw_spin_lock(&pmu_context->cpupm_lock);
> +	cpu = smp_processor_id();
> +
> +	if (pmu_context->in_hotplug[cpu]) {
> +		raw_spin_unlock(&pmu_context->cpupm_lock);
> +		return NOTIFY_BAD;
> +	}
> +
> +	__gs101_cpu_pmu_offline(cpu);
> +	raw_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;
> +
> +	raw_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->in_hotplug[cpu] = true;
> +	__gs101_cpu_pmu_offline(cpu);
> +
> +	raw_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))

I don't get exactly why you need here atomics. You don't have here
barriers, so ordering is not kept (non-RMW atomics are unordered), so
maybe ordering was not the problem to be solved here. But then you don't
use these at all as RMW and this is even explicitly described in atomic doc!

"Therefore, if you find yourself only using the Non-RMW operations of
atomic_t, you do not in fact need atomic_t at all and are doing it wrong."

And it is right. READ/WRITE_ONCE gives you the same.

The question is whether you need ordering or barriers in general
(atomics don't give you these) - you have here control dependency
if-else, however it is immediately followed with gs101_cpu_pmu_offline()
which will use spin-lock (so memory barrier).

Basically you should have here comment explaining why there is no
barrier - you rely on barrier from spin lock in next calls.

And if my reasoning is correct, then you should use just READ/WRITE_ONCE.


> +			return NOTIFY_OK;
> +
> +		return gs101_cpu_pmu_offline();
> +
> +	case CPU_PM_EXIT:
> +
> +		if (atomic_read(&pmu_context->sys_rebooting))
> +			return NOTIFY_OK;
> +
> +		return gs101_cpu_pmu_online();
> +	}
> +
> +	return NOTIFY_OK;
> +}

The rest looked fine.


Best regards,
Krzysztof

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

[Peter Griffin]

Hi Krzysztof,

Thanks, as always, for your review feedback.

On Sat, 12 Jul 2025 at 18:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 11/07/2025 15:50, Peter Griffin wrote:
> >
> >  #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 in_hotplug flag.
> > +      */
> > +     raw_spinlock_t cpupm_lock;
> > +     bool *in_hotplug;
>
> This should be bitmap - more obvious code.

I've done this in v7 which I just sent.

>
> > +     atomic_t sys_suspended;
> > +     atomic_t sys_rebooting;
> >  };
> >
> >  void __iomem *pmu_base_addr;
> > @@ -221,6 +231,15 @@ static const struct regmap_config regmap_smccfg = {
> >       .reg_read = tensor_sec_reg_read,
> >       .reg_write = tensor_sec_reg_write,
> >       .reg_update_bits = tensor_sec_update_bits,
> > +     .use_raw_spinlock = true,
> > +};
> > +
> > +static const struct regmap_config regmap_pmu_intr = {
> > +     .name = "pmu_intr_gen",
> > +     .reg_bits = 32,
> > +     .reg_stride = 4,
> > +     .val_bits = 32,
> > +     .use_raw_spinlock = true,
> >  };
> >
> >  static const struct exynos_pmu_data gs101_pmu_data = {
> > @@ -330,13 +349,19 @@ struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> >  EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
> >
>
>
> ...
>
> > +/* Called from CPU PM notifier (CPUIdle code path) with IRQs disabled */
> > +static int gs101_cpu_pmu_offline(void)
> > +{
> > +     int cpu;
> > +
> > +     raw_spin_lock(&pmu_context->cpupm_lock);
> > +     cpu = smp_processor_id();
> > +
> > +     if (pmu_context->in_hotplug[cpu]) {
> > +             raw_spin_unlock(&pmu_context->cpupm_lock);
> > +             return NOTIFY_BAD;
> > +     }
> > +
> > +     __gs101_cpu_pmu_offline(cpu);
> > +     raw_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;
> > +
> > +     raw_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->in_hotplug[cpu] = true;
> > +     __gs101_cpu_pmu_offline(cpu);
> > +
> > +     raw_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))
>
> I don't get exactly why you need here atomics. You don't have here
> barriers, so ordering is not kept (non-RMW atomics are unordered), so
> maybe ordering was not the problem to be solved here. But then you don't
> use these at all as RMW and this is even explicitly described in atomic doc!
>
> "Therefore, if you find yourself only using the Non-RMW operations of
> atomic_t, you do not in fact need atomic_t at all and are doing it wrong."
>
> And it is right. READ/WRITE_ONCE gives you the same.
>
> The question is whether you need ordering or barriers in general
> (atomics don't give you these) - you have here control dependency
> if-else, however it is immediately followed with gs101_cpu_pmu_offline()
> which will use spin-lock (so memory barrier).
>
> Basically you should have here comment explaining why there is no
> barrier - you rely on barrier from spin lock in next calls.
>
> And if my reasoning is correct, then you should use just READ/WRITE_ONCE.

There was a bad assumption on my part that atomic_read etc would have
barriers. After looking at this some more in v7 I decided to protect
the reboot/suspend flags with the raw spinlock. I think that makes it
much easier to reason about the code.

Thanks,

Peter