Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Sudeep Holla]

On Tue, Apr 08, 2025 at 07:51:46AM +0900, Donghyeok Choe wrote:
> On Mon, Apr 07, 2025 at 10:17:43AM +0100, Sudeep Holla wrote:
> > On Fri, Apr 04, 2025 at 01:13:23PM +0900, Donghyeok Choe wrote:
> > > On Thu, Apr 03, 2025 at 10:18:54AM +0100, Sudeep Holla wrote:
> > > > /me more confused.
> > > > 
> > > > Are you saying you have some cpuidle platform specific logic inside
> > > > trace_android_vh_cpuidle_psci_enter(). I would assume it was just to
> > > > trace the entry into the state and nothing more.
> > > 
> > > If you have any further questions, feel free to reach out.
> > > 
> > 
> > I was trying to understand the difference in behaviour between normal
> > cpuidle entering the same deepest state that is entered in s2idle state.
> > I assume GIC doesn't loose power and no need for GIC ITS save/restore
> > in normal cpuidle path ?
> >
> > If so, what triggers the GIC suspend in s2idle path if syscore_ops is
> > not getting called ?
> >
> > Why would the firmware pull the plug on GIC ?
> 
> The GIC loses power. It is powered down to the same level as during suspend.
> Therefore, it became necessary to perform GIC ITS save/restore through
> a method other than the GIC ITS syscore path.
> To help with better understanding, I will write a pseudo code.
> 
> void mimic_syscore_suspend()
> {
> 	/* Perform the actions required to power off all cores. */
> 	...
> 	its_save_disable();
> }
> 
> void android_vh_cpuidle_psci_enter_handler(... bool s2idle)
> {
> 
> 	if (!s2idle)
> 		return;
> 
> 	set_cpu_powerdown_mark();
> 
> 	if (cpu != booting core)
> 		return;
> 
> 	/* only booting core here */
> 	mimic_syscore_suspend()
> }
> 
> void mimic_syscore_resume()
> {
> 	...
> 	its_restore_enable();
> }
> 
> void android_vh_cpuidle_psci_exit_handler(... bool s2idle)
> {
> 	if (!s2idle)
> 		return;
> 
> 	if (cpu == booting core)
> 		mimic_syscore_resume();
> 
> 	set_cpu_poweron_mark();
> }
> 
> All cores will be marked as powered down when the HVC/SMC call for
> CPU suspend is invoked. When all cores call the suspend function,
> the firmware will recognize the powerdown mark and transition
> the system into suspend. At this point, the entire GIC will also
> be powered off.
> In a cpuidle situation that is not s2idle, the cores do not mark
> CPU powerdown, so the GIC ITS save/restore operation is neither
> performed nor necessary.
> 

OK, I understood. In short, you create problems by hacking up or misusing
your trace handlers in ways it shouldn't be, and now you are t/crying to
solve those problems.

> > Do you use any suspend/resume logic in drivers/irqchip/irq-gic-pm.c ?
> No, there are parts of the GIC that require secure access, so the
> GIC save/restore is performed by the firmware.
> Since the GIC-ITS is entirely controlled as a non-secure IP,
> I think it is more efficient to perform save/restore in the kernel.
> 

I can understand that part, but my hacking up things the way you have
shown above, though you may think you have achieved some feature very
smartly, you have just dug up the hole with issues you are facing now.

The only reason IIUC s2idle info is used is to identify when the RPM
is disabled. You are using that info to manage GIC power state.

The CPU deepest idle states entered in the normal and s2idle must be
same. If you want to still achieve extra power save with GIC powerdown
make it completely transparent to the OS.

-- 
Regards,
Sudeep