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

[Krzysztof Kozlowski <krzk@kernel.org>]

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