| Message ID | 1357777251-13541-9-git-send-email-nicolas.pitre@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote: > Otherwise there might be some interrupts or IPIs becoming pending and the > CPU will not enter low power mode when doing a WFI. The effect of this > is a CPU that loops back into the kernel, go through the first man > election, signals itself as alive, and prevent the cluster from being > shut down. > > This could benefit from a better solution. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/common/bL_platsmp.c | 1 + > arch/arm/common/gic.c | 6 ++++++ > arch/arm/include/asm/hardware/gic.h | 2 ++ > 3 files changed, 9 insertions(+) > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > index 0ae44123bf..6a3b251b97 100644 > --- a/arch/arm/common/bL_platsmp.c > +++ b/arch/arm/common/bL_platsmp.c > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > pcpu = mpidr & 0xff; > pcluster = (mpidr >> 8) & 0xff; > bL_set_entry_vector(pcpu, pcluster, NULL); > + gic_cpu_if_down(); So for a case where CPU still don't power down for some reason even after CPU interface is disabled, can not listen to and SGI or PPI. Not sure if this happens on big.LITTLE but i have seen one such issue on Cortex-A9 based SOC. Regards Santosh
On Fri, 11 Jan 2013, Santosh Shilimkar wrote: > On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote: > > Otherwise there might be some interrupts or IPIs becoming pending and the > > CPU will not enter low power mode when doing a WFI. The effect of this > > is a CPU that loops back into the kernel, go through the first man > > election, signals itself as alive, and prevent the cluster from being > > shut down. > > > > This could benefit from a better solution. > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > --- > > arch/arm/common/bL_platsmp.c | 1 + > > arch/arm/common/gic.c | 6 ++++++ > > arch/arm/include/asm/hardware/gic.h | 2 ++ > > 3 files changed, 9 insertions(+) > > > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > > index 0ae44123bf..6a3b251b97 100644 > > --- a/arch/arm/common/bL_platsmp.c > > +++ b/arch/arm/common/bL_platsmp.c > > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > > pcpu = mpidr & 0xff; > > pcluster = (mpidr >> 8) & 0xff; > > bL_set_entry_vector(pcpu, pcluster, NULL); > > + gic_cpu_if_down(); > > So for a case where CPU still don't power down for some reason even > after CPU interface is disabled, can not listen to and SGI or PPI. > Not sure if this happens on big.LITTLE but i have seen one such issue > on Cortex-A9 based SOC. Here the problem was the reverse i.e. a CPU wouldn't go down because some pending SGIs prevented that. Nicolas
On Saturday 12 January 2013 12:37 AM, Nicolas Pitre wrote: > On Fri, 11 Jan 2013, Santosh Shilimkar wrote: > >> On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote: >>> Otherwise there might be some interrupts or IPIs becoming pending and the >>> CPU will not enter low power mode when doing a WFI. The effect of this >>> is a CPU that loops back into the kernel, go through the first man >>> election, signals itself as alive, and prevent the cluster from being >>> shut down. >>> >>> This could benefit from a better solution. >>> >>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>> --- >>> arch/arm/common/bL_platsmp.c | 1 + >>> arch/arm/common/gic.c | 6 ++++++ >>> arch/arm/include/asm/hardware/gic.h | 2 ++ >>> 3 files changed, 9 insertions(+) >>> >>> diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c >>> index 0ae44123bf..6a3b251b97 100644 >>> --- a/arch/arm/common/bL_platsmp.c >>> +++ b/arch/arm/common/bL_platsmp.c >>> @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) >>> pcpu = mpidr & 0xff; >>> pcluster = (mpidr >> 8) & 0xff; >>> bL_set_entry_vector(pcpu, pcluster, NULL); >>> + gic_cpu_if_down(); >> >> So for a case where CPU still don't power down for some reason even >> after CPU interface is disabled, can not listen to and SGI or PPI. >> Not sure if this happens on big.LITTLE but i have seen one such issue >> on Cortex-A9 based SOC. > > Here the problem was the reverse i.e. a CPU wouldn't go down because > some pending SGIs prevented that. > I understood that part. What I was saying is, with CPU IF disabled and if CPU doesn't enter into the intended low power state and if the wakeup mechanism on that CPU is SGI/SPI, CPU may never wakeup and can lead to dead lock. I have seen this scenario on OMAP especially in CPUidle path. It may not be relevant for switcher considering, you almost force CPU to enter to low power state :-) Regards, Santosh
On Sat, 12 Jan 2013, Santosh Shilimkar wrote: > On Saturday 12 January 2013 12:37 AM, Nicolas Pitre wrote: > > On Fri, 11 Jan 2013, Santosh Shilimkar wrote: > > > > > On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote: > > > > Otherwise there might be some interrupts or IPIs becoming pending and > > > > the > > > > CPU will not enter low power mode when doing a WFI. The effect of this > > > > is a CPU that loops back into the kernel, go through the first man > > > > election, signals itself as alive, and prevent the cluster from being > > > > shut down. > > > > > > > > This could benefit from a better solution. > > > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > > --- > > > > arch/arm/common/bL_platsmp.c | 1 + > > > > arch/arm/common/gic.c | 6 ++++++ > > > > arch/arm/include/asm/hardware/gic.h | 2 ++ > > > > 3 files changed, 9 insertions(+) > > > > > > > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > > > > index 0ae44123bf..6a3b251b97 100644 > > > > --- a/arch/arm/common/bL_platsmp.c > > > > +++ b/arch/arm/common/bL_platsmp.c > > > > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > > > > pcpu = mpidr & 0xff; > > > > pcluster = (mpidr >> 8) & 0xff; > > > > bL_set_entry_vector(pcpu, pcluster, NULL); > > > > + gic_cpu_if_down(); > > > > > > So for a case where CPU still don't power down for some reason even > > > after CPU interface is disabled, can not listen to and SGI or PPI. > > > Not sure if this happens on big.LITTLE but i have seen one such issue > > > on Cortex-A9 based SOC. > > > > Here the problem was the reverse i.e. a CPU wouldn't go down because > > some pending SGIs prevented that. > > > I understood that part. What I was saying is, with CPU IF disabled and > if CPU doesn't enter into the intended low power state and if the wakeup > mechanism on that CPU is SGI/SPI, CPU may never wakeup and can lead to > dead lock. I have seen this scenario on OMAP especially in CPUidle path. Obviously, on the CPU idle path, you should not turn off the GIC interface as this might lose the ability to wake the CPU up with a pending interrupt, if your system is so configured. Here this is the CPU hot unplug path and we don't want the CPU to be awaken at all until we explicitly do something to wake it back up. However, in theory, all interrupts should have been migrated away from this CPU, so there shouldn't be any need for this. I should revisit the test that led me to create this patch. > It may not be relevant for switcher considering, you almost force CPU to > enter to low power state :-) The switcher doesn't use cpu_die() but calls into bL_cpu_power_down() directly. Nicolas
On Saturday 12 January 2013 10:17 PM, Nicolas Pitre wrote: > On Sat, 12 Jan 2013, Santosh Shilimkar wrote: > >> On Saturday 12 January 2013 12:37 AM, Nicolas Pitre wrote: >>> On Fri, 11 Jan 2013, Santosh Shilimkar wrote: >>> >>>> On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote: >>>>> Otherwise there might be some interrupts or IPIs becoming pending and >>>>> the >>>>> CPU will not enter low power mode when doing a WFI. The effect of this >>>>> is a CPU that loops back into the kernel, go through the first man >>>>> election, signals itself as alive, and prevent the cluster from being >>>>> shut down. >>>>> >>>>> This could benefit from a better solution. >>>>> >>>>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>>>> --- >>>>> arch/arm/common/bL_platsmp.c | 1 + >>>>> arch/arm/common/gic.c | 6 ++++++ >>>>> arch/arm/include/asm/hardware/gic.h | 2 ++ >>>>> 3 files changed, 9 insertions(+) >>>>> >>>>> diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c >>>>> index 0ae44123bf..6a3b251b97 100644 >>>>> --- a/arch/arm/common/bL_platsmp.c >>>>> +++ b/arch/arm/common/bL_platsmp.c >>>>> @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) >>>>> pcpu = mpidr & 0xff; >>>>> pcluster = (mpidr >> 8) & 0xff; >>>>> bL_set_entry_vector(pcpu, pcluster, NULL); >>>>> + gic_cpu_if_down(); >>>> >>>> So for a case where CPU still don't power down for some reason even >>>> after CPU interface is disabled, can not listen to and SGI or PPI. >>>> Not sure if this happens on big.LITTLE but i have seen one such issue >>>> on Cortex-A9 based SOC. >>> >>> Here the problem was the reverse i.e. a CPU wouldn't go down because >>> some pending SGIs prevented that. >>> >> I understood that part. What I was saying is, with CPU IF disabled and >> if CPU doesn't enter into the intended low power state and if the wakeup >> mechanism on that CPU is SGI/SPI, CPU may never wakeup and can lead to >> dead lock. I have seen this scenario on OMAP especially in CPUidle path. > > Obviously, on the CPU idle path, you should not turn off the GIC > interface as this might lose the ability to wake the CPU up with a > pending interrupt, if your system is so configured. > > Here this is the CPU hot unplug path and we don't want the CPU to be > awaken at all until we explicitly do something to wake it back up. > I see. > However, in theory, all interrupts should have been migrated away from > this CPU, so there shouldn't be any need for this. I should revisit the > test that led me to create this patch. > Thats right from hot-plug path and SPI are concerned. But SGI/PPI can still wakeup CPU and there is no migration as such since they are local to that >> It may not be relevant for switcher considering, you almost force CPU to >> enter to low power state :-) > > The switcher doesn't use cpu_die() but calls into bL_cpu_power_down() > directly. > Good to know that. Regards, Santosh
On Thu, Jan 10, 2013 at 12:20:43AM +0000, Nicolas Pitre wrote: > Otherwise there might be some interrupts or IPIs becoming pending and the > CPU will not enter low power mode when doing a WFI. The effect of this > is a CPU that loops back into the kernel, go through the first man > election, signals itself as alive, and prevent the cluster from being > shut down. > > This could benefit from a better solution. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/common/bL_platsmp.c | 1 + > arch/arm/common/gic.c | 6 ++++++ > arch/arm/include/asm/hardware/gic.h | 2 ++ > 3 files changed, 9 insertions(+) > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > index 0ae44123bf..6a3b251b97 100644 > --- a/arch/arm/common/bL_platsmp.c > +++ b/arch/arm/common/bL_platsmp.c > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > pcpu = mpidr & 0xff; > pcluster = (mpidr >> 8) & 0xff; > bL_set_entry_vector(pcpu, pcluster, NULL); > + gic_cpu_if_down(); I'm starting to sound like a stuck record (and not a very tuneful one at that) but... I think you need a barrier here. > bL_cpu_power_down(); Will
On Mon, 14 Jan 2013, Will Deacon wrote: > On Thu, Jan 10, 2013 at 12:20:43AM +0000, Nicolas Pitre wrote: > > Otherwise there might be some interrupts or IPIs becoming pending and the > > CPU will not enter low power mode when doing a WFI. The effect of this > > is a CPU that loops back into the kernel, go through the first man > > election, signals itself as alive, and prevent the cluster from being > > shut down. > > > > This could benefit from a better solution. > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > --- > > arch/arm/common/bL_platsmp.c | 1 + > > arch/arm/common/gic.c | 6 ++++++ > > arch/arm/include/asm/hardware/gic.h | 2 ++ > > 3 files changed, 9 insertions(+) > > > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > > index 0ae44123bf..6a3b251b97 100644 > > --- a/arch/arm/common/bL_platsmp.c > > +++ b/arch/arm/common/bL_platsmp.c > > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > > pcpu = mpidr & 0xff; > > pcluster = (mpidr >> 8) & 0xff; > > bL_set_entry_vector(pcpu, pcluster, NULL); > > + gic_cpu_if_down(); > > I'm starting to sound like a stuck record (and not a very tuneful one at > that) but... I think you need a barrier here. And I'm getting puzzled at the repetition. ;-) Nicolas
On Mon, Jan 14, 2013 at 04:54:52PM +0000, Nicolas Pitre wrote: > On Mon, 14 Jan 2013, Will Deacon wrote: > > > On Thu, Jan 10, 2013 at 12:20:43AM +0000, Nicolas Pitre wrote: > > > Otherwise there might be some interrupts or IPIs becoming pending and the > > > CPU will not enter low power mode when doing a WFI. The effect of this > > > is a CPU that loops back into the kernel, go through the first man > > > election, signals itself as alive, and prevent the cluster from being > > > shut down. > > > > > > This could benefit from a better solution. > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > --- > > > arch/arm/common/bL_platsmp.c | 1 + > > > arch/arm/common/gic.c | 6 ++++++ > > > arch/arm/include/asm/hardware/gic.h | 2 ++ > > > 3 files changed, 9 insertions(+) > > > > > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > > > index 0ae44123bf..6a3b251b97 100644 > > > --- a/arch/arm/common/bL_platsmp.c > > > +++ b/arch/arm/common/bL_platsmp.c > > > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > > > pcpu = mpidr & 0xff; > > > pcluster = (mpidr >> 8) & 0xff; > > > bL_set_entry_vector(pcpu, pcluster, NULL); > > > + gic_cpu_if_down(); > > > > I'm starting to sound like a stuck record (and not a very tuneful one at > > that) but... I think you need a barrier here. > > And I'm getting puzzled at the repetition. ;-) Sorry! This case is more interesting though, because you also want to order the cpu_if_down GIC write so that it completes before we do the power_off. Will
On Mon, 14 Jan 2013, Will Deacon wrote: > On Mon, Jan 14, 2013 at 04:54:52PM +0000, Nicolas Pitre wrote: > > On Mon, 14 Jan 2013, Will Deacon wrote: > > > > > On Thu, Jan 10, 2013 at 12:20:43AM +0000, Nicolas Pitre wrote: > > > > Otherwise there might be some interrupts or IPIs becoming pending and the > > > > CPU will not enter low power mode when doing a WFI. The effect of this > > > > is a CPU that loops back into the kernel, go through the first man > > > > election, signals itself as alive, and prevent the cluster from being > > > > shut down. > > > > > > > > This could benefit from a better solution. > > > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > > --- > > > > arch/arm/common/bL_platsmp.c | 1 + > > > > arch/arm/common/gic.c | 6 ++++++ > > > > arch/arm/include/asm/hardware/gic.h | 2 ++ > > > > 3 files changed, 9 insertions(+) > > > > > > > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > > > > index 0ae44123bf..6a3b251b97 100644 > > > > --- a/arch/arm/common/bL_platsmp.c > > > > +++ b/arch/arm/common/bL_platsmp.c > > > > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > > > > pcpu = mpidr & 0xff; > > > > pcluster = (mpidr >> 8) & 0xff; > > > > bL_set_entry_vector(pcpu, pcluster, NULL); > > > > + gic_cpu_if_down(); > > > > > > I'm starting to sound like a stuck record (and not a very tuneful one at > > > that) but... I think you need a barrier here. > > > > And I'm getting puzzled at the repetition. ;-) > > Sorry! This case is more interesting though, because you also want to order > the cpu_if_down GIC write so that it completes before we do the power_off. In this case I'm leaning toward removing that gic_cpu_if_down() entirely. I'm not convinced it is necessary, and if it is then we probably have a bug somewhere else. Nicolas
On Mon, Jan 14, 2013 at 05:18:24PM +0000, Nicolas Pitre wrote: > On Mon, 14 Jan 2013, Will Deacon wrote: > > Sorry! This case is more interesting though, because you also want to order > > the cpu_if_down GIC write so that it completes before we do the power_off. > > In this case I'm leaning toward removing that gic_cpu_if_down() > entirely. I'm not convinced it is necessary, and if it is then we > probably have a bug somewhere else. Or that :) Will
On Sat, Jan 12, 2013 at 04:47:19PM +0000, Nicolas Pitre wrote: > On Sat, 12 Jan 2013, Santosh Shilimkar wrote: > > > On Saturday 12 January 2013 12:37 AM, Nicolas Pitre wrote: > > > On Fri, 11 Jan 2013, Santosh Shilimkar wrote: > > > > > > > On Thursday 10 January 2013 05:50 AM, Nicolas Pitre wrote: > > > > > Otherwise there might be some interrupts or IPIs becoming pending and > > > > > the > > > > > CPU will not enter low power mode when doing a WFI. The effect of this > > > > > is a CPU that loops back into the kernel, go through the first man > > > > > election, signals itself as alive, and prevent the cluster from being > > > > > shut down. > > > > > > > > > > This could benefit from a better solution. > > > > > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > > > --- > > > > > arch/arm/common/bL_platsmp.c | 1 + > > > > > arch/arm/common/gic.c | 6 ++++++ > > > > > arch/arm/include/asm/hardware/gic.h | 2 ++ > > > > > 3 files changed, 9 insertions(+) > > > > > > > > > > diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c > > > > > index 0ae44123bf..6a3b251b97 100644 > > > > > --- a/arch/arm/common/bL_platsmp.c > > > > > +++ b/arch/arm/common/bL_platsmp.c > > > > > @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) > > > > > pcpu = mpidr & 0xff; > > > > > pcluster = (mpidr >> 8) & 0xff; > > > > > bL_set_entry_vector(pcpu, pcluster, NULL); > > > > > + gic_cpu_if_down(); > > > > > > > > So for a case where CPU still don't power down for some reason even > > > > after CPU interface is disabled, can not listen to and SGI or PPI. > > > > Not sure if this happens on big.LITTLE but i have seen one such issue > > > > on Cortex-A9 based SOC. > > > > > > Here the problem was the reverse i.e. a CPU wouldn't go down because > > > some pending SGIs prevented that. > > > > > I understood that part. What I was saying is, with CPU IF disabled and > > if CPU doesn't enter into the intended low power state and if the wakeup > > mechanism on that CPU is SGI/SPI, CPU may never wakeup and can lead to > > dead lock. I have seen this scenario on OMAP especially in CPUidle path. > > Obviously, on the CPU idle path, you should not turn off the GIC > interface as this might lose the ability to wake the CPU up with a > pending interrupt, if your system is so configured. That's platform specific. On TC2 turning GIC CPU IF off is pivotal otherwise the CPU receiving an IRQ can complete wfi and be reset by firmware when executing in the middle of nowhere, leading to a system lock-up. Disabling the GIC CPU IF must not be added to the gic_cpu_save() code, but we do need a helper function to disable the CPU IF for platforms that need this to happen to function properly (eg TC2). Lorenzo
On Mon, Jan 14, 2013 at 05:24:01PM +0000, Will Deacon wrote: > On Mon, Jan 14, 2013 at 05:18:24PM +0000, Nicolas Pitre wrote: > > On Mon, 14 Jan 2013, Will Deacon wrote: > > > Sorry! This case is more interesting though, because you also want to order > > > the cpu_if_down GIC write so that it completes before we do the power_off. > > > > In this case I'm leaning toward removing that gic_cpu_if_down() > > entirely. I'm not convinced it is necessary, and if it is then we > > probably have a bug somewhere else. > > Or that :) In the CPU idle code path (cpu_suspend) we do need to turn off the GIC CPU IF, hence we will have to cross that bridge when we come to it. Very soon. Lorenzo
diff --git a/arch/arm/common/bL_platsmp.c b/arch/arm/common/bL_platsmp.c index 0ae44123bf..6a3b251b97 100644 --- a/arch/arm/common/bL_platsmp.c +++ b/arch/arm/common/bL_platsmp.c @@ -68,6 +68,7 @@ static void __ref bL_cpu_die(unsigned int cpu) pcpu = mpidr & 0xff; pcluster = (mpidr >> 8) & 0xff; bL_set_entry_vector(pcpu, pcluster, NULL); + gic_cpu_if_down(); bL_cpu_power_down(); } diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 36ae03a3f5..760e8f4ca1 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c @@ -428,6 +428,12 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic) writel_relaxed(1, base + GIC_CPU_CTRL); } +void gic_cpu_if_down(void) +{ + void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]); + writel_relaxed(0, cpu_base + GIC_CPU_CTRL); +} + #ifdef CONFIG_CPU_PM /* * Saves the GIC distributor registers during suspend or idle. Must be called diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h index 4b1ce6cd47..2a7605492d 100644 --- a/arch/arm/include/asm/hardware/gic.h +++ b/arch/arm/include/asm/hardware/gic.h @@ -46,6 +46,8 @@ void gic_handle_irq(struct pt_regs *regs); void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); void gic_raise_softirq(const struct cpumask *mask, unsigned int irq); +void gic_cpu_if_down(void); + static inline void gic_init(unsigned int nr, int start, void __iomem *dist , void __iomem *cpu) {
Otherwise there might be some interrupts or IPIs becoming pending and the CPU will not enter low power mode when doing a WFI. The effect of this is a CPU that loops back into the kernel, go through the first man election, signals itself as alive, and prevent the cluster from being shut down. This could benefit from a better solution. Signed-off-by: Nicolas Pitre <nico@linaro.org> --- arch/arm/common/bL_platsmp.c | 1 + arch/arm/common/gic.c | 6 ++++++ arch/arm/include/asm/hardware/gic.h | 2 ++ 3 files changed, 9 insertions(+)