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

[Sudeep Holla <sudeep.holla@arm.com>]

On Wed, Jul 09, 2025 at 10:36:59PM +0100, Peter Griffin wrote:
> Hi Sudeep,
> 
> Thanks for your review feedback!
> 
> On Wed, 9 Jul 2025 at 17:10, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Jul 09, 2025 at 02:26:27PM +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.
> > >
> > > 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.
> > >
> >
> > Assuming this is arm64 platform and using PSCI for all the power management,
> > can you please briefly explain why all these dance is absolutely necessary
> > when PSCI calls can be the clue for the EL3 firmware. I am basing my question
> > on this information in the file:
> 
> Yes, you're correct it is an arm64 platform using PSCI. Unfortunately
> I don't have access to the el3mon firmware code to speak super
> authoritatively about it, but you're correct that it is essentially
> working around a firmware limitation.
> 
> What I initially observed whilst working on suspend to RAM, when
> hotplugging CPU's with just the PSCI calls the system hangs. Debugging
> this and tracing versus the downstream production drivers the missing
> piece was programming the "ACPM hint" to the CPU_INFORM registers.
> Further debugging and power measurements also showed that the ACPM
> hint is also required in addition to PSCI calls for the cpuidle states
> to function correctly.
> 

It is definitely worth adding all the above info and shaming the firmware
for not taking care of this. I still don't like this as PSCI is there
for nearly a decade now and still we see such limitations that needs to
be fixed in the firmware and now workaround in the kernel.

> > /*
> >  * CPU_INFORM register hint values which are used by
> >  * EL3 firmware (el3mon).
> >  */
> >
> > This clearly sounds like workaround for the firmware limitations. That
> > needs to be clearly documented IMO.
> 
> Sure I can add a more verbose comment, that this is required to work
> around firmware limitations in the PSCI implementation.
> 

Thanks that's what we need to fully understand the need of otherwise
useless dance around hotplug and idle state machinery in the kernel.

-- 
Regards,
Sudeep