| Message ID | 1445347435-2333-4-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thomas, On mar., oct. 20 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed > the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to > irq_set_probe(). > > While the commit log seems to imply that there are no functional > changes, there are indeed functional changes introduced by this > commit: the IRQ_NOAUTOEN flag is no longer cleared. This functional > change causes a regression on Armada XP, which no longer works > properly after suspend/resume because per-CPU interrupts remain > disabled. > > Due to how the hardware registers work, the irq-armada-370-xp cannot > simply save/restore a bunch of registers at suspend/resume to make > sure that the interrupts remain in the same state after > resuming. Therefore, it relies on the kernel to say whether the > interrupt is disabled or not, using the irqd_irq_disabled() > function. This was all working fine while the IRQ_NOAUTOEN flag was > cleared. > > With the change introduced by Rob Herring in d17cab4451df1, the > IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled() > returns false for per-CPU interrupts, and therefore our per-CPU > interrupts are no longer re-enabled after resume. > > This commit fixes that by using irqd_irq_disabled() only for global > interrupts, and using the newly introduced is_enabled_percpu_irq() for > per-CPU interrupts. > > Also, it fixes a related problems that per-CPU interrupts were only > re-enabled on the boot CPU and not other CPUs. Until now this wasn't a > problem since on this platform, only the local timers are using > per-CPU interrupts and the local timers of secondary CPUs are turned > off/on during CPU hotplug before suspend, after after resume. However, > in Linux 4.4, we will also be using per-CPU interrupts for the network > controller, so we need to properly restore the per-CPU interrupts on > secondary CPUs as well. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/irqchip/irq-armada-370-xp.c | 45 ++++++++++++++++++++++++++++++++----- > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c > index f5afe81..106ac4c 100644 > --- a/drivers/irqchip/irq-armada-370-xp.c > +++ b/drivers/irqchip/irq-armada-370-xp.c > @@ -311,7 +311,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h, > irq_set_percpu_devid(virq); > irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip, > handle_percpu_devid_irq); > - > } else { > irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip, > handle_level_irq); > @@ -377,12 +376,35 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask, > static int armada_xp_mpic_secondary_init(struct notifier_block *nfb, > unsigned long action, void *hcpu) > { > + unsigned int nirqs, irq; > + > if (action != CPU_STARTING && action != CPU_STARTING_FROZEN) > return NOTIFY_OK; > > armada_xp_mpic_perf_init(); > armada_xp_mpic_smp_cpu_init(); > > + /* Re-enable per-CPU interrupts that were enabled before suspend */ > + nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff; > + for (irq = 0; irq < nirqs; irq++) { Actually we could reduce this loop by using ARMADA_370_XP_MAX_PER_CPU_IRQS, as we know that we can't have more per cpu irq. > + struct irq_data *data; > + int virq; > + > + virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq); > + if (virq == 0) > + continue; > + > + data = irq_get_irq_data(virq); > + > + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > + continue; So eventually you only manage the timer IRQs? If it is intentional you could it differently, but I wonder why you don't enable again the other percpu IRQ. > + > + if (!is_enabled_percpu_irq(virq)) > + continue; > + > + armada_370_xp_irq_unmask(data); > + } > + > return NOTIFY_OK; > } > The following chunk will conflict with "irqchip: armada-370-xp: Rework per-cpu interrupts handling" which is in Linux next. But as this patch is for 4.3, you can't do anything... > @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void) > if (virq == 0) > continue; > > - if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > + data = irq_get_irq_data(virq); > + > + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) { > + /* Non per-CPU interrupts */ > writel(irq, per_cpu_int_base + > ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > - else > + if (!irqd_irq_disabled(data)) > + armada_370_xp_irq_unmask(data); > + } else { > + /* Per-CPU interrupts */ > writel(irq, main_int_base + > ARMADA_370_XP_INT_SET_ENABLE_OFFS); > > - data = irq_get_irq_data(virq); > - if (!irqd_irq_disabled(data)) > - armada_370_xp_irq_unmask(data); > + /* > + * Re-enable on the current CPU, > + * armada_xp_mpic_secondary_init() will take > + * care of secondary CPUs when they come up. > + */ > + if (is_enabled_percpu_irq(virq)) > + armada_370_xp_irq_unmask(data); > + } > } > > /* Reconfigure doorbells for IPIs and MSIs */ > -- > 2.6.2 >
Hello, On Tue, 20 Oct 2015 15:46:00 +0200, Gregory CLEMENT wrote: > > + /* Re-enable per-CPU interrupts that were enabled before suspend */ > > + nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff; > > + for (irq = 0; irq < nirqs; irq++) { > > Actually we could reduce this loop by using > ARMADA_370_XP_MAX_PER_CPU_IRQS, as we know that we can't have more per > cpu irq. Indeed. I can fix that up in the next version. > > + struct irq_data *data; > > + int virq; > > + > > + virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq); > > + if (virq == 0) > > + continue; > > + > > + data = irq_get_irq_data(virq); > > + > > + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > > + continue; > > So eventually you only manage the timer IRQs? > > If it is intentional you could it differently, but I wonder why you don't > enable again the other percpu IRQ. The idea is to have the same condition at the one used in the ->resume() hook. In the end, it should be using the is_percpu_irq() function as is done in linux-next in the ->resume() function. But since this patch is (hopefully) aimed at 4.3, I've for now kept the same logic as the current ->resume() function. > The following chunk will conflict with "irqchip: armada-370-xp: Rework > per-cpu interrupts handling" which is in Linux next. But as this patch > is for 4.3, you can't do anything... Indeed. I intentionally based this series on 4.3-rc, because it's fixing a regression introduced between 4.2 and 4.3-rc, and therefore as such should be fixed before 4.3 is released if possible (though I understand it's already -rc6 time so maybe a bit late). Thomas
Hi Thomas, > > @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void) > if (virq == 0) > continue; > > - if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > + data = irq_get_irq_data(virq); > + > + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) { > + /* Non per-CPU interrupts */ > writel(irq, per_cpu_int_base + For "Non per-CPU interrupts" per_cpu_int_base is used - is it intentional? In armada_370_xp_irq_mask/unmask the condition looks exactly opposite... > ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > - else > + if (!irqd_irq_disabled(data)) > + armada_370_xp_irq_unmask(data); > + } else { > + /* Per-CPU interrupts */ > writel(irq, main_int_base + > ARMADA_370_XP_INT_SET_ENABLE_OFFS); > Best regards, Marcin
Marcin, On Sun, 25 Oct 2015 22:22:37 +0100, Marcin Wojtas wrote: > > @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void) > > if (virq == 0) > > continue; > > > > - if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > > + data = irq_get_irq_data(virq); > > + > > + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) { > > + /* Non per-CPU interrupts */ > > writel(irq, per_cpu_int_base + > > For "Non per-CPU interrupts" per_cpu_int_base is used - is it > intentional? In armada_370_xp_irq_mask/unmask the condition looks > exactly opposite... Yes, this is normal. Carefully read PATCH 5/5, which adds a big comment, which explains the logic of the HW and how the irq-armada-370-xp driver copes with it. Each interrupt can be masked at two levels. One level is enabled when the interrupted is mapped, the other upon ->mask()/->unmask(). So when we're resuming, we need to re-enable the interrupt at the level it was enabled in ->map(), and have ->mask()/->unmask() continue to mask/unmask the interrupt at the other level. For per-CPU interrupts, ->map() and ->resume() enable the interrupt at the global level, and leave ->mask()/->unmask() enable/disable at the per-CPU level. For global interrupts, ->map() and ->resume() enable the interrupt at the per-CPU level, and leave ->mask()/->unmask() enable/disable at the global level. Again, see PATCH 5/5, and let me know if there are still some unclear aspects. Thanks! Thomas
Thomas, 2015-10-26 1:10 GMT+01:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Marcin, > > On Sun, 25 Oct 2015 22:22:37 +0100, Marcin Wojtas wrote: > >> > @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void) >> > if (virq == 0) >> > continue; >> > >> > - if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) >> > + data = irq_get_irq_data(virq); >> > + >> > + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) { >> > + /* Non per-CPU interrupts */ >> > writel(irq, per_cpu_int_base + >> >> For "Non per-CPU interrupts" per_cpu_int_base is used - is it >> intentional? In armada_370_xp_irq_mask/unmask the condition looks >> exactly opposite... > > Yes, this is normal. Carefully read PATCH 5/5, which adds a big > comment, which explains the logic of the HW and how the > irq-armada-370-xp driver copes with it. > > Each interrupt can be masked at two levels. One level is enabled when > the interrupted is mapped, the other upon ->mask()/->unmask(). So > when we're resuming, we need to re-enable the interrupt at the level it > was enabled in ->map(), and have ->mask()/->unmask() continue to > mask/unmask the interrupt at the other level. > > For per-CPU interrupts, ->map() and ->resume() enable the interrupt at > the global level, and leave ->mask()/->unmask() enable/disable at the > per-CPU level. > > For global interrupts, ->map() and ->resume() enable the interrupt at > the per-CPU level, and leave ->mask()/->unmask() enable/disable at the > global level. > > Again, see PATCH 5/5, and let me know if there are still some unclear > aspects. > Thanks for the explanation - now it's clear. Btw, I checked the patches with mvneta in both 'standby' and 'mem' modes on A38x (with not-yet-submitted support for PM in mvneta and pinctrl) and everything works properly. Hence: Tested-by: Marcin Wojtas <mw@semihalf.com> Best regards, Marcin
Marcin, On Mon, 26 Oct 2015 05:35:46 +0100, Marcin Wojtas wrote: > Thanks for the explanation - now it's clear. Good :-) Hopefully the explanation in PATCH 5/5 is also clear enough. > Btw, I checked the patches with mvneta in both 'standby' and 'mem' > modes on A38x (with not-yet-submitted support for PM in mvneta and > pinctrl) and everything works properly. Hence: Thanks for the testing. However, I wonder why you think those changes are need to get mvneta to work fine with the 'standby' mode ? While I do agree that they are need for the 'mem' mode, they shouldn't be needed for the 'standby' mode. For now, the standby mode only puts the CPU into deep-idle, and that's all: all devices remain powered on, and they don't lose their state. Thomas
Thomas, 2015-10-26 6:09 GMT+01:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Marcin, > > On Mon, 26 Oct 2015 05:35:46 +0100, Marcin Wojtas wrote: > >> Thanks for the explanation - now it's clear. > > Good :-) Hopefully the explanation in PATCH 5/5 is also clear enough. The Ascii-art is beutiful, indeed:) > >> Btw, I checked the patches with mvneta in both 'standby' and 'mem' >> modes on A38x (with not-yet-submitted support for PM in mvneta and >> pinctrl) and everything works properly. Hence: > > Thanks for the testing. However, I wonder why you think those changes > are need to get mvneta to work fine with the 'standby' mode ? While I > do agree that they are need for the 'mem' mode, they shouldn't be > needed for the 'standby' mode. For now, the standby mode only puts the > CPU into deep-idle, and that's all: all devices remain powered on, and > they don't lose their state. > Yes, you are right - without any pm_ops the driver works well after suspend/resume in standby. However in the linux mem and standby is treated exactly the same as pm sleep, so the same routines are executed in both modes. Hence the s2ram support cannot spoil standby. Best regards, Marcin
Dear Marcin Wojtas, On Mon, 26 Oct 2015 08:06:18 +0100, Marcin Wojtas wrote: > > Good :-) Hopefully the explanation in PATCH 5/5 is also clear enough. > > The Ascii-art is beutiful, indeed:) Glad to hear that my artistic skills are appreciated :) > Yes, you are right - without any pm_ops the driver works well after > suspend/resume in standby. However in the linux mem and standby is > treated exactly the same as pm sleep, so the same routines are > executed in both modes. Hence the s2ram support cannot spoil standby. Absolutely. It would be nicer if we knew in the ->suspend() and ->resume() hooks if we're doing a standby suspend, or a mem suspend, but IIRC, we don't have this information available. Thomas
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c index f5afe81..106ac4c 100644 --- a/drivers/irqchip/irq-armada-370-xp.c +++ b/drivers/irqchip/irq-armada-370-xp.c @@ -311,7 +311,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h, irq_set_percpu_devid(virq); irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip, handle_percpu_devid_irq); - } else { irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip, handle_level_irq); @@ -377,12 +376,35 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask, static int armada_xp_mpic_secondary_init(struct notifier_block *nfb, unsigned long action, void *hcpu) { + unsigned int nirqs, irq; + if (action != CPU_STARTING && action != CPU_STARTING_FROZEN) return NOTIFY_OK; armada_xp_mpic_perf_init(); armada_xp_mpic_smp_cpu_init(); + /* Re-enable per-CPU interrupts that were enabled before suspend */ + nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff; + for (irq = 0; irq < nirqs; irq++) { + struct irq_data *data; + int virq; + + virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq); + if (virq == 0) + continue; + + data = irq_get_irq_data(virq); + + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) + continue; + + if (!is_enabled_percpu_irq(virq)) + continue; + + armada_370_xp_irq_unmask(data); + } + return NOTIFY_OK; } @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void) if (virq == 0) continue; - if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) + data = irq_get_irq_data(virq); + + if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) { + /* Non per-CPU interrupts */ writel(irq, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); - else + if (!irqd_irq_disabled(data)) + armada_370_xp_irq_unmask(data); + } else { + /* Per-CPU interrupts */ writel(irq, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS); - data = irq_get_irq_data(virq); - if (!irqd_irq_disabled(data)) - armada_370_xp_irq_unmask(data); + /* + * Re-enable on the current CPU, + * armada_xp_mpic_secondary_init() will take + * care of secondary CPUs when they come up. + */ + if (is_enabled_percpu_irq(virq)) + armada_370_xp_irq_unmask(data); + } } /* Reconfigure doorbells for IPIs and MSIs */
Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to irq_set_probe(). While the commit log seems to imply that there are no functional changes, there are indeed functional changes introduced by this commit: the IRQ_NOAUTOEN flag is no longer cleared. This functional change causes a regression on Armada XP, which no longer works properly after suspend/resume because per-CPU interrupts remain disabled. Due to how the hardware registers work, the irq-armada-370-xp cannot simply save/restore a bunch of registers at suspend/resume to make sure that the interrupts remain in the same state after resuming. Therefore, it relies on the kernel to say whether the interrupt is disabled or not, using the irqd_irq_disabled() function. This was all working fine while the IRQ_NOAUTOEN flag was cleared. With the change introduced by Rob Herring in d17cab4451df1, the IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled() returns false for per-CPU interrupts, and therefore our per-CPU interrupts are no longer re-enabled after resume. This commit fixes that by using irqd_irq_disabled() only for global interrupts, and using the newly introduced is_enabled_percpu_irq() for per-CPU interrupts. Also, it fixes a related problems that per-CPU interrupts were only re-enabled on the boot CPU and not other CPUs. Until now this wasn't a problem since on this platform, only the local timers are using per-CPU interrupts and the local timers of secondary CPUs are turned off/on during CPU hotplug before suspend, after after resume. However, in Linux 4.4, we will also be using per-CPU interrupts for the network controller, so we need to properly restore the per-CPU interrupts on secondary CPUs as well. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/irqchip/irq-armada-370-xp.c | 45 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 6 deletions(-)