Hi André,
On Thu, 3 Jul 2025 at 11:12, André Draszik <andre.draszik@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
