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@xxxxxxx> 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
