Hi Russell,

On Sat, Jul 09, 2011 at 11:14:45AM +0100, Russell King - ARM Linux wrote:

On Thu, Jul 07, 2011 at 04:50:14PM +0100, Lorenzo Pieralisi wrote:

...

From: Will Deacon will.deacon@arm.com

This patch adds simple definitions of cpu_reset for ARMv6 and ARMv7 cores, which disable the MMU via the SCTLR.

This really needs fixing properly, so that we have this well defined across all supported ARM cores. Requiring ARMv6 and ARMv7 to have this code called with a flat mapping (which may overlap a section boundary) vs ARMv5 and lower code which doesn't is just silly.

With any API, we need consistency. So if ARMv6 and v7 require a flat mapping, we need to ensure that ARMv5 and lower is happy to deal with that code being also called with a flat mapping.

I've had a look at a bunch of the cpu_*_reset definitions and I can't see any reason why they wouldn't be callable with the flat mapping in place. In fact, there's a scary comment for xscale:

@ CAUTION: MMU turned off from this point. We count on the pipeline @ already containing those two last instructions to survive.

which I think would disappear if the code was called via the ID map.

At the moment, the only caller [1] of these functions is arch_reset which is called from arm_machine_restart after putting a flat mapping in place. The extra work is actually to call the reset code via that mapping. I've been working on this in my kexec series, which I'll continue with during 3.1.

Will

[1] plat-s3c24xx/cpu.c is an exception, but cpu_reset is only entered if the watchdog fails during hard reboot. The logic could be easily fixed up here so that arm_machine_restart is called instead.

On Sun, Jul 10, 2011 at 12:52:06PM +0100, Russell King - ARM Linux wrote:

On Sun, Jul 10, 2011 at 12:00:24PM +0100, Will Deacon wrote:

...

I've had a look at a bunch of the cpu_*_reset definitions and I can't see any reason why they wouldn't be callable with the flat mapping in place. In fact, there's a scary comment for xscale:

However, that flat mapping doesn't save us, because this only covers space below PAGE_OFFSET virtual. We're executing these generally from virtual space at addresses greater than this, which means when the MMU is turned off, the instruction stream disappears.

Yes, although I have fixed this in my kexec branch. It's still very much a WIP, so feel free to comment when I post the next revision of that patch series.

...

@ CAUTION: MMU turned off from this point. We count on the pipeline @ already containing those two last instructions to survive.

which I think would disappear if the code was called via the ID map.

Yes, and everything pre-ARMv6 fundamentally relies upon the instructions following the MCR to turn the MMU off already being in the CPUs pipeline. Pre-ARMv6 relies upon this kind of behaviour from the CPU:

fetch decode execute mcr mov pc mcr nop mov pc mcr nop nop mov pc <inst0> <--- flushed --->

where inst0 is the instruction at the target of the "mov pc" branch. May not be well documented in the architecture manual, but there have been documentation of this level against CPUs in the past. Maybe back in ARMv3 times, but the trick has proven to work all the way up to ARMv5, and these are of course CPU specific files rather than architecture specific.

With more recent (>= ARMv6) CPUs, this really is implementation-specific behaviour and, as such, is not documented by the architecture. Instead you get:

`In addition, if the physical address of the code that enables or disables the MMU differs from its MVA, instruction prefetching can cause complications. Therefore, ARM strongly recommends that any code that enables or disables the MMU has identical virtual and physical addresses.'

This has the advantage of working across all CPU implementations, which is what we need for having architectural proc-vN.S files.

It's curious that with ARMs move to a more relaxed model, that turning the MMU off has visibly changed from a fairly weak operation to an effective strong instruction barrier. These now have visibly this behaviour:

fetch decode execute mcr mov pc mcr <inst0> mov pc mcr <------ flushed ------> mov pc*

• • attempt to reload this instruction fails because MMU is now off.

I'm not saying there that the pipeline is flushed (it may be) - but this represents the _observed_ behaviour.

Sure. The new definitions for v6/v7 reset have an instruction barrier following the SCTLR write anyway since the architecture doesn't make any guarantees about immediacy of MMU disabling. The advantage is that it will work for all v6/v7 implementations. The disadvantage is that we're effectively coding for the worst-case scenario.

Will

On Sat, Jul 9, 2011 at 3:15 AM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:

On Thu, Jul 07, 2011 at 04:50:15PM +0100, Lorenzo Pieralisi wrote:

...

During some CPU power modes entered during idle, hotplug and suspend, peripherals located in the CPU power domain, such as the GIC and VFP, may be powered down.  Add a notifier chain that allows drivers for those peripherals to be notified before and after they may be reset.

I defer comment on this until I see what the CPU_COMPLEX_* stuff is used for.

Per previous discussions on this patch, CPU_COMPLEX_* will be renamed CPU_CLUSTER_*, and will refer to all the gunk around the CPU that may be shared by another CPU. In this series, it is only used to save and restore the GIC distributor registers, but it could be used for L2 as well, if any platforms can agree on what needs to happen to the L2, and when.

On 7/7/2011 8:50 AM, Lorenzo Pieralisi wrote:

From: Colin Crossccross@android.com

When the cpu is powered down in a low power mode, the gic cpu interface may be reset, and when the cpu complex is powered down, the gic distributor may also be reset.

This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the gic cpu interface registers, and the CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save and restore the gic distributor registers.

Signed-off-by: Colin Crossccross@android.com

arch/arm/common/gic.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 4ddd0a6..8d62e07 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c

[...]

+static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) +{

• int i;
• for (i = 0; i< MAX_GIC_NR; i++) {

• switch (cmd) {
  

• case CPU_PM_ENTER:
  

• 	gic_cpu_save(i);
  

• 	break;
  

• case CPU_PM_ENTER_FAILED:
  

• case CPU_PM_EXIT:
  

• 	gic_cpu_restore(i);
  

• 	break;
  

• case CPU_COMPLEX_PM_ENTER:
  

• 	gic_dist_save(i);
  

• 	break;
  

• case CPU_COMPLEX_PM_ENTER_FAILED:
  

• case CPU_COMPLEX_PM_EXIT:
  

• 	gic_dist_restore(i);
  

• 	break;
  

• }
  

• }
• return NOTIFY_OK;

+}

Just to put forth OMAP requirements for GIC and see how much we can leverage these for OMAP.

OMAP support GP(general) and HS(secure) devices and implements the trustzone on these devices. on Secure devices the GIC save and restore is completely done by secure ROM code. There are API's for save and restore is automatic on CPU reset based on the last CPU cluster state.

On GP devices too, very few GIC registers needs to be saved in a pre-defined memory/register layout and restore is again done by boot-ROM code.

OMAP need to enable/disable distributor and CPU interfaces based on CPU power states and that is something can be useful. Would be good if there is a provision to over-write the gic save/restore function using function pointers so that OMAP PM code can use the notifiers.

Any more thoughts how we can handle this? We would like to use common ARM code as much as possible.

Regards Santosh

On 7/7/2011 6:41 PM, Colin Cross wrote:

On Thu, Jul 7, 2011 at 6:35 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:

...

On 7/7/2011 8:50 AM, Lorenzo Pieralisi wrote:

...

From: Colin Crossccross@android.com

When the cpu is powered down in a low power mode, the gic cpu interface may be reset, and when the cpu complex is powered down, the gic distributor may also be reset.

This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the gic cpu interface registers, and the CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save and restore the gic distributor registers.

Signed-off-by: Colin Crossccross@android.com

arch/arm/common/gic.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 4ddd0a6..8d62e07 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c

[...]

...

+static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) +{

•   int i;
  

•   for (i = 0; i<    MAX_GIC_NR; i++) {
  

•           switch (cmd) {
  

•           case CPU_PM_ENTER:
  

•                   gic_cpu_save(i);
  

•                   break;
  

•           case CPU_PM_ENTER_FAILED:
  

•           case CPU_PM_EXIT:
  

•                   gic_cpu_restore(i);
  

•                   break;
  

•           case CPU_COMPLEX_PM_ENTER:
  

•                   gic_dist_save(i);
  

•                   break;
  

•           case CPU_COMPLEX_PM_ENTER_FAILED:
  

•           case CPU_COMPLEX_PM_EXIT:
  

•                   gic_dist_restore(i);
  

•                   break;
  

•           }
  

•   }
  

•   return NOTIFY_OK;
  

+}

Just to put forth OMAP requirements for GIC and see how much we can leverage these for OMAP.

OMAP support GP(general) and HS(secure) devices and implements the trustzone on these devices. on Secure devices the GIC save and restore is completely done by secure ROM code. There are API's for save and restore is automatic on CPU reset based on the last CPU cluster state.

On GP devices too, very few GIC registers needs to be saved in a pre-defined memory/register layout and restore is again done by boot-ROM code.

OMAP need to enable/disable distributor and CPU interfaces based on CPU power states and that is something can be useful. Would be good if there is a provision to over-write the gic save/restore function using function pointers so that OMAP PM code can use the notifiers.

Any more thoughts how we can handle this? We would like to use common ARM code as much as possible.

Is it strictly necessary to use the custom OMAP save and restore?

Yes. On secure devices there is no choice.

Anything that was modified by the kernel is obviously writable, and could be saved and restored using the common code. Anything that can only be modified by TrustZone already has to be restored by the custom OMAP code. There aren't many registers in the GIC, so it shouldn't be much of a performance difference.

In that case we will end up doing things two time un-necessary and that's not useful at all. You need to save all those extra cycles to have less latency on C-states. And otherside you can't skip the secure API save otherwise the boot-ROM code will end up re-initializing the GIC and all secure interrupt state will be lost.

From above code, today we use need dist/cpu interface disable/enable functions.

Regards Santosh

Regards Santosh

On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:

This is necessary for cpuidle states that lose the GIC registers, not just suspend, because the GIC is in the cpu's power domain. We could avoid saving and restoring all the GIC registers in suspend and idle by reusing the initialization functions, and then having the core irq code call the unmask, set_type, and set_affinity functions on each irq to reconfigure it, but that will be very inefficient - it will convert each register write in the restore functions to a read-modify-write per interrupt in that register. Santosh is already complaining that this commong GIC restore code will be slower than the automatic DMA to restore the GIC registers that OMAP4 supports.

Well, we need to come up with something sensible - a way of doing this which doesn't require every interrupt controller driver (of which we as an architecture have many) to have lots of support added.

If the current way is inefficient and is noticably so, then let's talk to Thomas about finding a way around that - maybe having the generic code make one suspend/resume callback per irq gc chip rather than doing it per-IRQ. We can then reuse the same paths for suspend/resume as for idle state saving.

On Sat, Jul 09, 2011 at 04:01:19PM -0700, Colin Cross wrote:

On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:

...

This is necessary for cpuidle states that lose the GIC registers, not just suspend, because the GIC is in the cpu's power domain.  We could avoid saving and restoring all the GIC registers in suspend and idle by reusing the initialization functions, and then having the core irq code call the unmask, set_type, and set_affinity functions on each irq to reconfigure it, but that will be very inefficient - it will convert each register write in the restore functions to a read-modify-write per interrupt in that register.  Santosh is already complaining that this commong GIC restore code will be slower than the automatic DMA to restore the GIC registers that OMAP4 supports.

Well, we need to come up with something sensible - a way of doing this which doesn't require every interrupt controller driver (of which we as an architecture have many) to have lots of support added.

If the current way is inefficient and is noticably so, then let's talk to Thomas about finding a way around that - maybe having the generic code make one suspend/resume callback per irq gc chip rather than doing it per-IRQ.  We can then reuse the same paths for suspend/resume as for idle state saving.

Are you referring to moving the gic driver to be gc chip? Otherwise, I don't understand your suggestion - how is callback per chip any different than what this patch implements? It just gets it's notification through a cpu_pm notifier, which works in idle and suspend, instead of a syscore op like the gc driver does.

This patch does save and restore some registers that are never modified after init, so they don't need to be saved.

The point is that we should aim to get to the point where, if an interrupt controller supports PM, then it supports _all_ PM out the box and doesn't require additional code for cpu idle PM vs system suspend PM.

In other words, all we should need to do is provide genirq with a couple of functions for 'save state' and 'restore state'.

On 7/9/2011 4:05 PM, Russell King - ARM Linux wrote:

On Sat, Jul 09, 2011 at 04:01:19PM -0700, Colin Cross wrote:

...

On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:

...

This is necessary for cpuidle states that lose the GIC registers, not just suspend, because the GIC is in the cpu's power domain. We could avoid saving and restoring all the GIC registers in suspend and idle by reusing the initialization functions, and then having the core irq code call the unmask, set_type, and set_affinity functions on each irq to reconfigure it, but that will be very inefficient - it will convert each register write in the restore functions to a read-modify-write per interrupt in that register. Santosh is already complaining that this commong GIC restore code will be slower than the automatic DMA to restore the GIC registers that OMAP4 supports.

Well, we need to come up with something sensible - a way of doing this which doesn't require every interrupt controller driver (of which we as an architecture have many) to have lots of support added.

If the current way is inefficient and is noticably so, then let's talk to Thomas about finding a way around that - maybe having the generic code make one suspend/resume callback per irq gc chip rather than doing it per-IRQ. We can then reuse the same paths for suspend/resume as for idle state saving.

Are you referring to moving the gic driver to be gc chip? Otherwise, I don't understand your suggestion - how is callback per chip any different than what this patch implements? It just gets it's notification through a cpu_pm notifier, which works in idle and suspend, instead of a syscore op like the gc driver does.

This patch does save and restore some registers that are never modified after init, so they don't need to be saved.

The point is that we should aim to get to the point where, if an interrupt controller supports PM, then it supports _all_ PM out the box and doesn't require additional code for cpu idle PM vs system suspend PM.

In other words, all we should need to do is provide genirq with a couple of functions for 'save state' and 'restore state'.

Agreed. But how generic irq code will know about when the interrupt controller looses it's context without notifiers. This still depend on the SOC power domain partitions and hence platform code At least for the GIC case, it seems depends on the CPU cluster low power state.

Regards Santosh

On Thu, Jul 21, 2011 at 09:32:12AM +0100, Santosh Shilimkar wrote:

Lorenzo, Colin,

On 7/7/2011 9:20 PM, Lorenzo Pieralisi wrote:

...

From: Colin Crossccross@android.com

When the cpu is powered down in a low power mode, the gic cpu interface may be reset, and when the cpu complex is powered down, the gic distributor may also be reset.

This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the gic cpu interface registers, and the CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save and restore the gic distributor registers.

Signed-off-by: Colin Crossccross@android.com

arch/arm/common/gic.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 4ddd0a6..8d62e07 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c

[...]

I missed one more comment in the last review.

...

+static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) +{

• int i;
• for (i = 0; i< MAX_GIC_NR; i++) {

• switch (cmd) {
  

• case CPU_PM_ENTER:
  

• 	gic_cpu_save(i);
  

On OMAP, GIC cpu interface context is lost only when CPU cluster is powered down.

Yes, it's true, but that's the only chance we have to save the GIC CPU IF state if the GIC context is lost, right ? It is a private memory map per processor; I agree, it might be useless if just one CPU is shutdown, but at that point in time you do not know the state of other CPUs. If the cluster moves to a state where GIC context is lost at least you had the GIC CPU IF state saved. If we do not save it, well, there is no way to do that anymore since the last CPU cannot access other CPUs GIC CPU IF registers (or better, banked GIC distributor registers). If you force hotplug on CPUs other than 0 (that's the way it is done on OMAP4 in cpuidle, right ?) to hit deep low-power states you reinit the GIC CPU IF state as per cold boot, so yes, it is useless there.

...

• 	break;
  

• case CPU_PM_ENTER_FAILED:
  

• case CPU_PM_EXIT:
  

• 	gic_cpu_restore(i);
  

• 	break;
  

• case CPU_COMPLEX_PM_ENTER:
  

• 	gic_dist_save(i);
  

• 	break;
  

• case CPU_COMPLEX_PM_ENTER_FAILED:
  

• case CPU_COMPLEX_PM_EXIT:
  

• 	gic_dist_restore(i);
  

• 	break;
  

• }
  

• }
• return NOTIFY_OK;

+}

Entire GIC is kept in CPU cluster power domain and hence GIC CPU interface context won't be lost whenever CPU alone enters in the deepest power state.

Already commented above, it is the same on the dev board I am using.

If it is different on other SOC's then the common notifiers won't match to all the SOC designs.

Ditto.

Looks like exporting these functions directly or adding them to gen irq and then invoking them from platform code based on power sequence need might be better.

I will have a look into that for the next version.

Thank you very much, Lorenzo

On Thu, Jul 21, 2011 at 3:46 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:

On 7/21/2011 3:57 PM, Lorenzo Pieralisi wrote:

...

On Thu, Jul 21, 2011 at 09:32:12AM +0100, Santosh Shilimkar wrote:

...

Lorenzo, Colin,

On 7/7/2011 9:20 PM, Lorenzo Pieralisi wrote:

...

From: Colin Crossccross@android.com

When the cpu is powered down in a low power mode, the gic cpu interface may be reset, and when the cpu complex is powered down, the gic distributor may also be reset.

This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the gic cpu interface registers, and the CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save and restore the gic distributor registers.

Signed-off-by: Colin Crossccross@android.com

arch/arm/common/gic.c |  212 +++++++++++++++++++++++++++++++++++++++++++++++++   1 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 4ddd0a6..8d62e07 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c

[...]

I missed one more comment in the last review.

...

+static int gic_notifier(struct notifier_block *self, unsigned long cmd,        void *v) +{

• int i;
• for (i = 0; i<   MAX_GIC_NR; i++) {
• switch (cmd) {
• case CPU_PM_ENTER:
• gic_cpu_save(i);

On OMAP, GIC cpu interface context is lost only when CPU cluster is powered down.

Yes, it's true, but that's the only chance we have to save the GIC CPU IF state if the GIC context is lost, right ? It is a private memory map per processor; I agree, it might be useless if just one CPU is shutdown, but at that point in time you do not know the state of other CPUs. If the cluster moves to a state where GIC context is lost at least you had the GIC CPU IF state saved. If we do not save it, well, there is no way to do that anymore since the last CPU cannot access other CPUs GIC CPU IF registers (or better, banked GIC distributor registers). If you force hotplug on CPUs other than 0 (that's the way it is done on OMAP4 in cpuidle, right ?) to hit deep low-power states you reinit the GIC CPU IF state as per cold boot, so yes, it is useless there.

Actually, on OMAP there is no need to save any CPU interface registers.

For my OMAP4 PM rebasing, for time-being I will go with exported GIC functions so that I don't have too many redundancies with GIC save/restore code.

I think you should try to balance cpu idle latency with reuse of common code. In this case, you are avoiding restoring 7 registers by reimplementing the bare minimum that is necessary for OMAP4, which is unlikely to make a measurable impact on wakeup latency. Can you try starting with reusing all the common code, and add some timestamps during wakeup to measure where the longest delays are, to determine where you should diverge from the common code and use omap-optimized code?

On Thu, Jul 21, 2011 at 10:10 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:

On 7/22/2011 12:36 AM, Colin Cross wrote:

...

On Thu, Jul 21, 2011 at 3:46 AM, Santosh Shilimkar santosh.shilimkar@ti.com  wrote:

...

On 7/21/2011 3:57 PM, Lorenzo Pieralisi wrote:

...

On Thu, Jul 21, 2011 at 09:32:12AM +0100, Santosh Shilimkar wrote:

...

Lorenzo, Colin,

On 7/7/2011 9:20 PM, Lorenzo Pieralisi wrote:

...

From: Colin Crossccross@android.com

When the cpu is powered down in a low power mode, the gic cpu interface may be reset, and when the cpu complex is powered down, the gic distributor may also be reset.

This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save and restore the gic cpu interface registers, and the CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save and restore the gic distributor registers.

Signed-off-by: Colin Crossccross@android.com

arch/arm/common/gic.c |  212 +++++++++++++++++++++++++++++++++++++++++++++++++   1 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 4ddd0a6..8d62e07 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c

[...]

I missed one more comment in the last review.

...

+static int gic_notifier(struct notifier_block *self, unsigned long cmd,        void *v) +{

• int i;
• for (i = 0; i<     MAX_GIC_NR; i++) {
• switch (cmd) {
• case CPU_PM_ENTER:
• gic_cpu_save(i);

On OMAP, GIC cpu interface context is lost only when CPU cluster is powered down.

Yes, it's true, but that's the only chance we have to save the GIC CPU IF state if the GIC context is lost, right ? It is a private memory map per processor; I agree, it might be useless if just one CPU is shutdown, but at that point in time you do not know the state of other CPUs. If the cluster moves to a state where GIC context is lost at least you had the GIC CPU IF state saved. If we do not save it, well, there is no way to do that anymore since the last CPU cannot access other CPUs GIC CPU IF registers (or better, banked GIC distributor registers). If you force hotplug on CPUs other than 0 (that's the way it is done on OMAP4 in cpuidle, right ?) to hit deep low-power states you reinit the GIC CPU IF state as per cold boot, so yes, it is useless there.

Actually, on OMAP there is no need to save any CPU interface registers.

For my OMAP4 PM rebasing, for time-being I will go with exported GIC functions so that I don't have too many redundancies with GIC save/restore code.

I think you should try to balance cpu idle latency with reuse of common code.  In this case, you are avoiding restoring 7 registers by reimplementing the bare minimum that is necessary for OMAP4, which is unlikely to make a measurable impact on wakeup latency.  Can you try starting with reusing all the common code, and add some timestamps during wakeup to measure where the longest delays are, to determine where you should diverge from the common code and use omap-optimized code?

I am going to use all the common code but having them exported functions gives more flexibility to call them in right and needed places. As discussed earlier, I plan to use the common GIC code wherever it's needed on OMAP.

My main point was we are saving and restoring GIC CPU interface registers for a case where they are actually not lost.

Yes, but you're still avoiding 7 registers, which is unlikely to be worth the complexity of calling these functions differently from every other platform.

On Sat, Jul 09, 2011 at 11:44:08AM +0100, Russell King - ARM Linux wrote:

We need to sort this out so we have a _sane_ approach to this, rather than inventing more and more creative ways to save VFP state and restore it later.

And here, let's prove that the current code is just soo bloody complex that it needs redoing. In the following code, 'last_VFP_context' is renamed to 'vfp_current_hw_state' for clarity.

void vfp_sync_hwstate(struct thread_info *thread) { unsigned int cpu = get_cpu();

/* * If the thread we're interested in is the current owner of the * hardware VFP state, then we need to save its state. */ if (vfp_current_hw_state[cpu] == &thread->vfpstate) { u32 fpexc = fmrx(FPEXC);

/* * Save the last VFP state on this CPU. */ fmxr(FPEXC, fpexc | FPEXC_EN); vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN); fmxr(FPEXC, fpexc); }

Here, 'thread' is the thread we're interested in ensuring that we have up to date context in thread->vfpstate. On entry to this function, we can be running on any CPU in the system, and 'thread' could have been running on any other CPU in the system.

What this code is saying is: if the currrent CPU's hardware VFP state was owned by this thread, then update the current VFP state. So far it looks sane.

Now, lets consider what with thread migration. First, lets define three threads.

Thread 1, we'll call 'interesting_thread' which is a thread which is running on CPU0, using VFP (so vfp_current_hw_state[0] = &interesting_thread->vfpstate) and gets migrated off to CPU1, where it continues execution of VFP instructions.

Thread 2, we'll call 'new_cpu0_thread' which is the thread which takes over on CPU0. This has also been using VFP, and last used VFP on CPU0, but doesn't use it again.

The following code will be executed twice:

cpu = thread->cpu;

/* * On SMP, if VFP is enabled, save the old state in * case the thread migrates to a different CPU. The * restoring is done lazily. */ if ((fpexc & FPEXC_EN) && vfp_current_hw_state[cpu]) { vfp_save_state(vfp_current_hw_state[cpu], fpexc); vfp_current_hw_state[cpu]->hard.cpu = cpu; } /* * Thread migration, just force the reloading of the * state on the new CPU in case the VFP registers * contain stale data. */ if (thread->vfpstate.hard.cpu != cpu) vfp_current_hw_state[cpu] = NULL;

The first execution will be on CPU0 to switch away from 'interesting_thread'. interesting_thread->cpu will be 0.

So, vfp_current_hw_state[0] points at interesting_thread->vfpstate. The hardware state will be saved, along with the CPU number (0) that it was executing on.

'thread' will be 'new_cpu0_thread' with new_cpu0_thread->cpu = 0. Also, because it was executing on CPU0, new_cpu0_thread->vfpstate.hard.cpu = 0, and so the thread migration check is not triggered.

This means that vfp_current_hw_state[0] remains pointing at interesting_thread.

The second execution will be on CPU1 to switch _to_ 'interesting_thread'. So, 'thread' will be 'interesting_thread' and interesting_thread->cpu now will be 1. The previous thread executing on CPU1 is not relevant to this so we shall ignore that.

We get to the thread migration check. Here, we discover that interesting_thread->vfpstate.hard.cpu = 0, yet interesting_thread->cpu is now 1, indicating thread migration. We set vfp_current_hw_state[1] to NULL.

So, at this point vfp_current_hw_state[] contains the following:

[0] = interesting_thread [1] = NULL

Our interesting thread now executes a VFP instruction, takes a fault which loads the state into the VFP hardware. Now, through the assembly we now have:

[0] = interesting_thread [1] = interesting_thread

CPU1 stops due to ptrace (and so saves its VFP state) using the thread switch code above), and CPU0 calls vfp_sync_hwstate().

if (vfp_current_hw_state[cpu] == &thread->vfpstate) { vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);

BANG, we corrupt interesting_thread's VFP state by overwriting the more up-to-date state saved by CPU1 with the old VFP state from CPU0.

I think this is not the only problem with this code, and it's in desperate need of being cleaned up. Until such time, adding more state saving code is just going to be extremely hairy.

Finally, as far as saving state for _idle_ goes (in other words, while the CPU's idle loop in cpu_idle() is running), take a moment to consider the following: the idle thread being a kernel thread does not use VFP. It has no useful VFP state. So:

1. On SMP, because we've switched away from any userland thread, we have already saved its state when we switched away.

If VFP hardware state is lost across an idle, the only thing that needs doing is that fact noted by setting vfp_current_hw_state[cpu] for the CPUs which lost VFP state to NULL. No state saving is required.

2. On UP, the VFP hardware may contain the current threads state, which, if state is lost, would need to be saved _IF_ vfp_current_hw_state[cpu] is non-NULL. Again, if state is lost, then vfp_current_hw_state[cpu] needs to be NULL'd.

These conditions won't change as a result of cleaning up the hairyness of the existing code.

On Mon, 11 Jul 2011, Lorenzo Pieralisi wrote:

On Fri, Jul 08, 2011 at 05:12:22PM +0100, Frank Hofmann wrote:

...

Hi Lorenzo,

only a few comments at this stage.

The sr_entry.S code is both exclusively .arm (using conditionals and long-distance adr, i.e. not Thumb2-clean), and it uses post-armv5 instructions (like wfi). Same for the other *.S code in the patch series. It's non-generic assembly within arch/arch/kernel/, wouldn't one better place this into arch/arm/mm/...-v[67].S ?

Yes, it is certainly something I should improve to make it more generic.

...

Then, sr_suspend/sr_resume; these functions are "C-exported" and are directly calling cpu_do_suspend/do_resume to pass a supplied buffer; I've done that for one iteration of the hibernation patch, yes, but that was a bit sneaky and Russell stated then the interface is cpu_suspend/cpu_resume not the proc funcs directly. Unless _those_ have been changed they're also unsafe to call from C funcs (clobber all regs). Couldn't you simply use cpu_suspend/resume directly ?

Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should. That's why I use cpu_do_{suspend,resume}, so that I can choose the memory used to save registers (uncached), but that's an abuse of API.

There's the option to use a switch_stack() as Will Deacon has provided via his kexec series. Agreed not mainline yet but good for ideas. Will's diff is here:

http://www.spinics.net/lists/arm-kernel/msg127951.html

and that one restores the original sp after the "stack switched" function returns.

If you use that to switch to a different (uncached) stack before doing cpu_suspend (and the 'idle' finisher through that), wouldn't that solve the problem ? When the suspend returns, the above would restore your cached stackpointer.

I agree this is sneaky, but I wanted to avoid duplicating code that just saves registers. Maybe moving to an uncached stack might solve this problem if we want to reuse cpu_suspend from cpu idle, which is still an open point.

(me speedtyping above ...) As you say ;-)

...

How much memory do all the pagedirs require that are being kept around ? Why does each core need a separate one, what would happen to just use a single "identity table" for all ? I understand you can't use swapper_pg_dir for idle, so a separate one has to be allocated, yet the question remains why per-cpu required ?

I just allocate a 1:1 mapping once cloned from init_mm, reused for all CPUs, with an additional mapping.

Have started to wonder whether this facility (a 1:1-mapped pagedir for kernel text/data, or maybe even "non-user text/data") could/should be made available on a global scale; after all, both kexec, reset, hibernate, some forms of idle/suspend do require "some sort of that" to go through MMU reinitialization. I'm unfortunately not deep enough in the VM subsystem to say how exactly this best would have to look like.

Merely mentioning this because it looks while everyone creates 1:1 mappings, there's ever so slight differences between how the 1:1 mapping(s) are created; we've seen:

* (re)using current->active_mm->pgd * (re)using swapper_pg_dir (different lengths for the 1:1 section) * using a separately-allocated/initialized pgdir

Such proliferation usually means there's a justified need to do that kind of thing. Just the gritty details haven't been sorted how everyone with that need could do _the same_ thing instead of reinventing various square wheels.

The main reason why I've used swapper_pg_dir in the hibernation patch is because it's the only static one in the system (the hibernation hooks are in irqs-off codepaths and pgd_alloc isn't a good idea then), and happens to have "clear" lower sections (in the user area) so that one's not actually substituting anything when creating 1:1 mappings (and getting rid of them restores the "pristine" state). But this assumption only holds as long as swapper_pg_dir's use isn't changed. So a little creepy feeling remains.

A generic "reset_mmu_pg_dir", wouldn't that be a good thing to have ?

The array of pointers is there to save pgdir on idle entry, one per-cpu.

If you're going through cpu_{do_}suspend/resume, the TTBRs are saved/restored anyway, what do you need to keep the virtual addresses around for ?

...

I'm currently transitioning between jobs; will re-subscribe to arm-kernel under a different email address soon, this one is likely to stop working in August. Sorry the inconvenience and high-latency responses till then :(

FrankH.

May I thank you very much for the review in the interim,

You're welcome. FrankH.

Lorenzo

On Mon, 11 Jul 2011, Lorenzo Pieralisi wrote:

[ ... ]

...

...

The array of pointers is there to save pgdir on idle entry, one per-cpu.

If you're going through cpu_{do_}suspend/resume, the TTBRs are saved/restored anyway, what do you need to keep the virtual addresses around for ?

Because I switch mm before calling suspend, which is called with a cloned pgdir. I am not sure I can avoid that.

On resume, you'll be restoring the same thread as was previously running, right ? If so, all you do there is copying current->active_mm->pgd to some other place ?

Also, if you'd be using cpu_suspend(), would there still be a need for cpu_switch_mm() before ? It'd rather be a case of possibly calling that before the MMU-off sequence / cpu_resume() ?

Or is it that you use the new pgdir to make a memory region uncacheable ?

FrankH.

On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote:

Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should.

Hang on. Please explain something to me here. You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache. Why is this the case?

OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues.

However, lets recap. What we do in cpu_suspend() in order is:

- Save the ABI registers onto the stack, and some private state - Save the CPU specific state onto the stack - Flush the L1 cache - Call the platform specific suspend finisher

On resume, with the MMU and caches off:

- Platform defined entry point is called, which _may_ be cpu_resume directly. - Platform initial code is executed to do whatever that requires - cpu_resume will be called - cpu_resume loads the previously saved private state - The CPU specific state is restored - Page table is modified to permit 1:1 mapping for MMU enable - MMU is enabled with caches disabled - Page table modification is undone - Caches are enabled in the main control register - CPU exception mode stacks are reinitialized - CPU specific init function is called - ABI registers are popped off and 'cpu_suspend' function returns

So, as far as L2 goes, in the suspend finisher:

- If L2 state is lost, the finisher needs to clean dirty data from L2 to ensure that it is preserved in RAM. Note: There is no need to disable or even invalidate the L2 cache as we should not be writing any data in the finisher function which we later need after resume.

- If L2 state is not lost, the finisher needs to clean the saved state as a minimum, to sure that this is visible when the main control register C bit is clear. The easiest way to do that is to find the top of stack via current_thread_info() - we have a macro for that, and then add THREAD_SIZE to find the top of stack. 'sp' will be the current bottom of stack.

In the resume initial code:

- If L2 state was lost, the L2 configuration needs to be restored. This generally needs to happen before cpu_resume is called: - there are CPUs which need L2 setup before the MMU is enabled. - OMAP3 currently does this in its assembly, which is convenient to allow it to make the SMI calls to the secure world. The same will be true of any CPU running in non-secure mode.

- If L2 state was not lost, and the platform choses not to clean and invalidate the ABI registers from the stack, and the platform restores the L2 configuration before calling cpu_resume, then the ABI registers will be read out of L2 on return if that's where they are - at that point everything will be setup correctly.

This will give the greatest performance, which is important for CPU idle use of these code paths.

Now, considering SMP, there's an issue here: do we know at the point where one CPU goes down whether L2 state will be lost?

If the answer is that state will not be lost, we can do the minimum. If all L2 state is lost, we need to do as above. If we don't know the answer, then we have to assume that L2 state will be lost.

But wait - L2 cache (or more accurately, the outer cache) is common between CPUs in a SMP system. So, if we're _not_ the last CPU to go down, then we assume that L2 state will not be lost. It is the last CPUs responsibility to deal with L2 state when it goes into a PM mode that results in L2 state being lost.

Lastly, should generic code deal with flushing L2 and setting it back up on resume? A couple of points there:

1. Will generic code know whether L2 state will be lost, or should it assume that L2 state is always lost and do a full L2 flush. That seems wasteful if we have another CPU still running (which would also have to flush L2.)

2. L2 configuration registers are not accessible to CPUs operating in non-secure mode like OMAPs. Generic code on these CPUs has _no_ way to restore and re-enable the L2 cache. It needs to make implementation specific SMI calls to achieve that.

So, I believe the answer to that is no. However, I think we can still do a change to improve the situation:

1. Pass in r2 and r3 to the suspend finisher the bottom and top of the stack which needs to be cleaned from L2. This covers the saved state but not the ABI registers.

2. Mandate that L2 configuration is to be restored by platforms in their pre-cpu_resume code so L2 is available when the C bit is set.

On Mon, Jul 11, 2011 at 11:51:00AM -0700, Colin Cross wrote:

On Mon, Jul 11, 2011 at 11:40 AM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote:

...

Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should.

Hang on.  Please explain something to me here.  You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache.  Why is this the case?

OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues.

OMAP is very different, because it doesn't use cpu_suspend. It saves it's state to SAR ram, which is mapped uncached, which avoids L2 problems.

I'm afraid your information is out of date. See:

http://ftp.arm.linux.org.uk/git/?p=linux-2.6-arm.git%3Ba=commitdiff%3Bh=076f...

arch/arm/mach-omap2/pm34xx.c | 47 +++---------- arch/arm/mach-omap2/sleep34xx.S | 143 +-------------------------------------- 2 files changed, 13 insertions(+), 177 deletions(-)

for the trivial conversion, and:

http://ftp.arm.linux.org.uk/git/?p=linux-2.6-arm.git%3Ba=commit%3Bh=46e130d2...

arch/arm/mach-omap2/pm.h | 20 ++- arch/arm/mach-omap2/pm34xx.c | 20 ++-- arch/arm/mach-omap2/sleep34xx.S | 303 ++++++++++++++++++++++---------------- arch/arm/plat-omap/sram.c | 15 +-- 4 files changed, 206 insertions(+), 152 deletions(-)

for the cleanup of the SRAM code, most of which was found not to need being in SRAM. That's all tested as working (see the tested-by's) on a range of OMAP3 platforms.

The whole series is at:

http://ftp.arm.linux.org.uk/git/?p=linux-2.6-arm.git%3Ba=shortlog%3Bh=refs/h...

There is only one currently merged SoC which hasn't been converted: shmobile. That's in progress, and there may already be patches for it. There's no blockers on that as far as I know other than availability of time.

The sleep_save_sp location also needs to be cleaned.

Right.

...

In the resume initial code:

• If L2 state was lost, the L2 configuration needs to be restored.

This generally needs to happen before cpu_resume is called:  - there are CPUs which need L2 setup before the MMU is enabled.  - OMAP3 currently does this in its assembly, which is convenient to    allow it to make the SMI calls to the secure world.  The same will    be true of any CPU running in non-secure mode.

• If L2 state was not lost, and the platform choses not to clean and

invalidate the ABI registers from the stack, and the platform restores  the L2 configuration before calling cpu_resume, then the ABI registers  will be read out of L2 on return if that's where they are - at that  point everything will be setup correctly.

This will give the greatest performance, which is important for CPU  idle use of these code paths.

Now, considering SMP, there's an issue here: do we know at the point where one CPU goes down whether L2 state will be lost?

CPU idle will need a voting mechanism between the two CPUs, and the second CPU to go down needs to determine the minimum state supported by both CPUs. Any time a CPU goes down, we should know for sure that either the L2 will not be lost, or the L2 might be lost if the other CPU goes idle.

No it doesn't. As I've said: L2 is shared between the two CPUs.

If L2 state is preserved, then the only thing that needs cleaning from L2 is the state which must be accessible to bring the system back up, which is the CPU specific state, the cpu_suspend state and the sleep_save_sp store.

If L2 state is lost, the _first_ CPU to go down in a two-CPU system must do as above. If nothing else happens L2 state will be preserved. So there's absolutely no point what so ever the first CPU cleaning the entire L2 state - the second CPU will still be scribbling into L2 at that point.

However, when the second CPU goes down, that's when the L2 state would need to be preserved if the L2.

So, let's recap. In a two CPU system, when precisely one CPU goes into sleep mode, only the minimum L2 state needs cleaning. Nothing else. Only when the second CPU goes down does it matter whether L2 state needs to be preserved by cleaning or not because that is when L2 data loss may occur.

In practice, I don't think many SoCs will support low power modes that lose the L2 during idle, only during suspend. Tegra and OMAP4 both don't support the modes where the L2 is powered down in idle.

That's good, that means less to worry about. For idle, all we need to care about is the CPU specific state, the cpu_suspend state and the sleep_save_sp store. Great, that means things can be even simpler - cpu_suspend() just needs to know whether we're calling it because of idle, or whether we're calling it because of system suspend.

On 7/11/2011 12:19 PM, Russell King - ARM Linux wrote:

On Mon, Jul 11, 2011 at 11:51:00AM -0700, Colin Cross wrote:

...

On Mon, Jul 11, 2011 at 11:40 AM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote:

...

Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should.

Hang on. Please explain something to me here. You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache. Why is this the case?

OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues.

OMAP is very different, because it doesn't use cpu_suspend. It saves it's state to SAR ram, which is mapped uncached, which avoids L2 problems.

I'm afraid your information is out of date. See:

I think the confusion is OMAP3 and OMAP4. Colin was talking about OMAP4 which isn't merged in mainline yet where as you were referring OMAP3 clean-ups happened recently.

Regards Santosh

On Mon, Jul 11, 2011 at 01:05:20PM -0700, Santosh Shilimkar wrote:

(Just to add few more points on top of what Colin already commented)

On 7/11/2011 11:40 AM, Russell King - ARM Linux wrote:

...

On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote:

...

Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should.

Hang on. Please explain something to me here. You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache. Why is this the case?

OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues.

This part is not done yet Russell. Infact the cpu_resume() function need an update to work with L2 enable configuration.

The code does deal with L2 cache enable in the resume path...

...

However, lets recap. What we do in cpu_suspend() in order is:

• Save the ABI registers onto the stack, and some private state
• Save the CPU specific state onto the stack

We need to disable C bit here to avoid any speculative artifacts during further operations before WFI.

Which you are doing.

...

• Flush the L1 cache
• Call the platform specific suspend finisher

Also finisher function should issue the WFI and just in case for some reason CPU doesn't hit the targeted low power state, finisher function takes care of things like enabling C bit, SMP bit etc.

You're restoring the C bit in the non-off paths already which follow a failed WFI. You're not touching the SMP bit there though - was it ever reset? I don't think so.

...

On resume, with the MMU and caches off:

• Platform defined entry point is called, which _may_ be cpu_resume directly.
• Platform initial code is executed to do whatever that requires
• cpu_resume will be called
• cpu_resume loads the previously saved private state
• The CPU specific state is restored
• Page table is modified to permit 1:1 mapping for MMU enable

1:1 idmap used here should be mapped as non-cached to avoid L2 related issues. I faced similar issue in OMAP sleep code earlier and later thanks to Colin, we got the fixed my making use of non-cached idmap.

It is specified that if the main control register C bit is clear, accesses will be uncached.

Whether you get cache hits or not is probably implementation dependent, and provided that the state information is cleaned from the L2 cache, it doesn't matter if we hit the L2 cached copy or the RAM copy. It's the same data.

Thank you very much Russell for this recap.

On Mon, Jul 11, 2011 at 07:40:10PM +0100, Russell King - ARM Linux wrote:

On Mon, Jul 11, 2011 at 03:00:47PM +0100, Lorenzo Pieralisi wrote:

...

Well, short answer is no. On SMP we do need to save CPU registers but if just one single cpu is shutdown L2 is still on. cpu_suspend saves regs on the stack that has to be cleaned from L2 before shutting a CPU down which make things more complicated than they should.

Hang on. Please explain something to me here. You've mentioned a few times that cpu_suspend() can't be used because of the L2 cache. Why is this the case?

OMAP appears to have code in its sleep path - which has been converted to cpu_suspend() support - to deal with the L2 issues.

OMAP4, it is SMP configs I am talking about.

However, lets recap. What we do in cpu_suspend() in order is:

• Save the ABI registers onto the stack, and some private state
• Save the CPU specific state onto the stack

As Santosh said, L1 should be cleaned with C bit cleared. We are still in coherency and if L1 D$ keeps allocating we might run into issues here when for instance a single CPU is going down. It is the stack, as usual. The finisher should be written in assembly (I think that's the case) and should not use the stack (eg thread_info). If I am not mistaken thread_info might be written by other CPUs and DDI might pull in it as a dirty line. We must avoid having dirty lines in L1 D$ when we pull the power.

On top of that, if the C bit is cleared I think we need to clean the L2 cache in assembly in the finisher, and avoid using spinlocks because this does not work when C bit is cleared. This means that this code will become racy by definition or I am missing something.

• Flush the L1 cache
• Call the platform specific suspend finisher

On resume, with the MMU and caches off:

• Platform defined entry point is called, which _may_ be cpu_resume directly.
• Platform initial code is executed to do whatever that requires
• cpu_resume will be called
• cpu_resume loads the previously saved private state
• The CPU specific state is restored
• Page table is modified to permit 1:1 mapping for MMU enable
• MMU is enabled with caches disabled
• Page table modification is undone
• Caches are enabled in the main control register
• CPU exception mode stacks are reinitialized
• CPU specific init function is called
• ABI registers are popped off and 'cpu_suspend' function returns

So, as far as L2 goes, in the suspend finisher:

• If L2 state is lost, the finisher needs to clean dirty data from L2 to ensure that it is preserved in RAM. Note: There is no need to disable or even invalidate the L2 cache as we should not be writing any data in the finisher function which we later need after resume.

I agree that "not writing" should be sufficient but want to raise a point anyway. If L2 is shutdown or put in L2 RAM retention on idle (I read the reply from Colin about idle support for L2 shutdown but I think we have to cater for it anyway) it has to be disabled before issuing wfi (we have to be in control of L2 to make sure it is not fetching lines behind our back). It is about avoiding transactions on the AXI bus when the power is yanked from L2. Also L2 prefetch bits should be cleared, I am checking with HW guys how and when this might create issues.

• If L2 state is not lost, the finisher needs to clean the saved state as a minimum, to sure that this is visible when the main control register C bit is clear. The easiest way to do that is to find the top of stack via current_thread_info() - we have a macro for that, and then add THREAD_SIZE to find the top of stack. 'sp' will be the current bottom of stack.

Spot-on.

In the resume initial code:

• If L2 state was lost, the L2 configuration needs to be restored. This generally needs to happen before cpu_resume is called:

  • there are CPUs which need L2 setup before the MMU is enabled.
  • OMAP3 currently does this in its assembly, which is convenient to allow it to make the SMI calls to the secure world. The same will be true of any CPU running in non-secure mode.

• If L2 state was not lost, and the platform choses not to clean and invalidate the ABI registers from the stack, and the platform restores the L2 configuration before calling cpu_resume, then the ABI registers will be read out of L2 on return if that's where they are - at that point everything will be setup correctly.

  This will give the greatest performance, which is important for CPU idle use of these code paths.

Now, considering SMP, there's an issue here: do we know at the point where one CPU goes down whether L2 state will be lost?

That is what I am tracking in the patch, meaning that we have to know when the executing CPU is the last running. If we mandate control within CPU idle to control that for all platforms I think we are all set.

If the answer is that state will not be lost, we can do the minimum. If all L2 state is lost, we need to do as above. If we don't know the answer, then we have to assume that L2 state will be lost.

I know I am a pain in the neck, but please consider L2 RAM retention in this picture where logic is lost but RAM is retained, so it should not be cleaned.

But wait - L2 cache (or more accurately, the outer cache) is common between CPUs in a SMP system. So, if we're _not_ the last CPU to go down, then we assume that L2 state will not be lost. It is the last CPUs responsibility to deal with L2 state when it goes into a PM mode that results in L2 state being lost.

That's correct.

Lastly, should generic code deal with flushing L2 and setting it back up on resume? A couple of points there:

1. Will generic code know whether L2 state will be lost, or should it assume that L2 state is always lost and do a full L2 flush. That seems wasteful if we have another CPU still running (which would also have to flush L2.)

on SMP, single CPU shutdown, as you stated, only the minimum should be cleaned from L2. The same goes for system shutdown (all CPUs down) L2 RAM retention.

2. L2 configuration registers are not accessible to CPUs operating in non-secure mode like OMAPs. Generic code on these CPUs has _no_ way to restore and re-enable the L2 cache. It needs to make implementation specific SMI calls to achieve that.

So, I believe the answer to that is no. However, I think we can still do a change to improve the situation:

1. Pass in r2 and r3 to the suspend finisher the bottom and top of the stack which needs to be cleaned from L2. This covers the saved state but not the ABI registers.

2. Mandate that L2 configuration is to be restored by platforms in their pre-cpu_resume code so L2 is available when the C bit is set.

On top of that, if we can also define and mandate a warm-boot protocol (eg CPU0 is always the one coming up from complete system shutdown) and the other(s) are put into wfi or a platform specific procedure that would be grand.

Lorenzo

On Thu, Jul 07, 2011 at 04:50:18PM +0100, Lorenzo Pieralisi wrote:

+static int late_init(void) +{

• int rc;
• struct sr_cluster *cluster;
• int cluster_index, cpu_index = sr_platform_get_cpu_index();

Stop this madness, and use the standard linux APIs like smp_processor_id here. It might actually help you find bugs, like the fact that you're in a preemptible context here, and so could be rescheduled onto any other CPU in the system _after_ you've read the MPIDR register.

• cluster_index = sr_platform_get_cluster_index();
• cluster = main_table.cluster_table + cluster_index;
• main_table.os_mmu_context[cluster_index][cpu_index] =

• 		current->active_mm->pgd;
  

• cpu_switch_mm(main_table.fw_mmu_context, current->active_mm);
• rc = sr_platform_init();
• cpu_switch_mm(main_table.os_mmu_context[cluster_index][cpu_index],

• 		current->active_mm);
  

CPU numbers are unique in the system, why do you need a 'cluster_index' to save this? In fact why do you even need to save it in a structure at all?

Plus, "cluster" is not used, please get rid of it.

Plus, here you just switch the page tables without a MMU flush. Further down in this file you call cpu_switch_mm() but also flush the TLB. Why that difference?

If this is the state of just the first bit of this I've looked at, plus the comments from Frank on your use of internal functions like cpu_do_suspend and cpu_do_resume, I don't want to look at it any further. Can you please clean it up as best you can first.

On Sat, Jul 09, 2011 at 09:45:08AM +0100, Russell King - ARM Linux wrote:

On Sat, Jul 09, 2011 at 09:38:15AM +0100, Russell King - ARM Linux wrote:

...

On Thu, Jul 07, 2011 at 04:50:18PM +0100, Lorenzo Pieralisi wrote:

...

+static int late_init(void) +{

• int rc;
• struct sr_cluster *cluster;
• int cluster_index, cpu_index = sr_platform_get_cpu_index();

Stop this madness, and use the standard linux APIs like smp_processor_id here. It might actually help you find bugs, like the fact that you're in a preemptible context here, and so could be rescheduled onto any other CPU in the system _after_ you've read the MPIDR register.

You are right, wanted to make all code uniform and rushed in a change which is bound to create issues.

...

...

• cluster_index = sr_platform_get_cluster_index();
• cluster = main_table.cluster_table + cluster_index;
• main_table.os_mmu_context[cluster_index][cpu_index] =

• 		current->active_mm->pgd;
  

• cpu_switch_mm(main_table.fw_mmu_context, current->active_mm);
• rc = sr_platform_init();
• cpu_switch_mm(main_table.os_mmu_context[cluster_index][cpu_index],

• 		current->active_mm);
  

CPU numbers are unique in the system, why do you need a 'cluster_index' to save this? In fact why do you even need to save it in a structure at all?

Plus, "cluster" is not used, please get rid of it.

Oh, that's another thing. This thread is about introducing idle support not cluster support. Cluster support is surely something different (esp. as it seems from the above code that you're trying to support several clusters of MPCore CPUs, each with physical 0-N CPU numbers.)

Cluster support should be an entirely separate patch series.

Yes it is cluster support, within idle though, that has to be said; I will introduce it in a different patch as suggested and this patch will have to rely on it.

[...]

In any case, smp_processor_id() will be (and must be) unique in any given running kernel across all CPUs, even if you have clusters of N CPUs all physically numbered 0-N.

Indeed.