On 09/21/2016 03:40 PM, Marc Zyngier wrote:
+Sudeep, Lorenzo,
On 21/09/16 09:42, Lingutla Chandrasekhar wrote:
Implement suspend and resume syscore_ops to disable and
enable non wake up capable interrupts.
When system enters suspend, enable only wakeup capable
interrupts. While resuming, enable previously enabled interrupts
and show triggered/pending interrupts.
The fundamental problem (which you're not mentioning at all) is that the
GICv3 architecture doesn't mention wake-up interrupts at all, so this
has to be entirely handled in firmware. Is that what is happening?
Signed-off-by: Lingutla Chandrasekhar <clingutla@xxxxxxxxxxxxxx>
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index ede5672..511a5a1 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -33,6 +33,7 @@
#include <linux/irqchip/arm-gic-common.h>
#include <linux/irqchip/arm-gic-v3.h>
#include <linux/irqchip/irq-partition-percpu.h>
+#include <linux/syscore_ops.h>
#include <asm/cputype.h>
#include <asm/exception.h>
@@ -57,6 +58,10 @@ struct gic_chip_data {
u32 nr_redist_regions;
unsigned int irq_nr;
struct partition_desc *ppi_descs[16];
+#ifdef CONFIG_PM
+ unsigned int wakeup_irqs[32];
+ unsigned int enabled_irqs[32];
Do not use ambiguous types for something that comes from the HW. Where
does this '32' comes from?
Expecting wakeup capable irq can be any of maximum 1024 interrupt lines, so
used array of size 32 (32 * 32 bits).
+#endif
};
static struct gic_chip_data gic_data __read_mostly;
@@ -330,6 +335,81 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
return 0;
}
+#ifdef CONFIG_PM
+static int gic_suspend(void)
+{
+ unsigned int i;
+ void __iomem *base = gic_data.dist_base;
+
+ for (i = 0; i * 32 < gic->irq_nr; i++) {
+ gic->enabled_irqs[i]
+ = readl_relaxed(base + GICD_ISENABLER + i * 4);
Do you realize that GICD_ISENABLER0 is always zero? What do you do for
PPIs? Please keep the assignment on a single line.
Agreed, will update.
+ /* disable all of them */
+ writel_relaxed(0xffffffff, base + GICD_ICENABLER + i * 4);
+ /* enable the wakeup set */
+ writel_relaxed(gic->wakeup_irqs[i],
+ base + GICD_ISENABLER + i * 4);
On a single line as well.
+ }
+ return 0;
+}
+
+static void gic_show_pending(void)
+{
+ unsigned int i;
+ u32 enabled;
+ u32 pending[32];
+ void __iomem *base = gic_data.dist_base;
+
+ for (i = 0; i * 32 < gic->irq_nr; i++) {
+ enabled = readl_relaxed(base + GICD_ICENABLER + i * 4);
+ pending[i] = readl_relaxed(base + GICD_ISPENDR + i * 4);
+ pending[i] &= enabled;
+ }
+
+ for_each_set_bit(i, (unsigned long *)pending, gic->irq_nr) {
+ unsigned int irq = irq_find_mapping(gic->domain, i);
+ struct irq_desc *desc = irq_to_desc(irq);
+ const char *name = "null";
+
+ if (desc == NULL)
+ name = "stray irq";
+ else if (desc->action && desc->action->name)
+ name = desc->action->name;
+
+ pr_debug("Pending IRQ: %d [%s]\n", __func__, irq, name);
+ }
+}
Please drop this function from this patch, it doesn't serve any purpose
other than your own debugging.
I think, this function is useful for debugging to know wakeup reason.
Can we move this function under PM_DEBUG flag or debugfs entry ?
+
+static void gic_resume(void)
+{
+ unsigned int i;
+ void __iomem *base = gic_data.dist_base;
+
+ gic_show_pending();
+
+ for (i = 0; i * 32 < gic->irq_nr; i++) {
+ /* disable all of them */
+ writel_relaxed(0xffffffff, base + GICD_ICENABLER + i * 4);
+ /* enable the enabled set */
+ writel_relaxed(gic->enabled_irqs[i],
+ base + GICD_ISENABLER + i * 4);
Same remarks as the suspend side.
+ }
+}
+
+static struct syscore_ops gic_syscore_ops = {
+ .suspend = gic_suspend,
+ .resume = gic_resume,
+};
+
+static int __init gic_init_sys(void)
+{
+ register_syscore_ops(&gic_syscore_ops);
+ return 0;
+}
+device_initcall(gic_init_sys);
+
+#endif
+
static u64 gic_mpidr_to_affinity(unsigned long mpidr)
{
u64 aff;
@@ -666,6 +746,32 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
#define gic_smp_init() do { } while(0)
#endif
+#ifdef CONFIG_PM
+int gic_set_wake(struct irq_data *d, unsigned int on)
+{
+ int ret = -ENXIO;
+ unsigned int reg_offset, bit_offset;
+ unsigned int gicirq = gic_irq(d);
+ struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+
+ /* per-cpu interrupts cannot be wakeup interrupts */
+ WARN_ON(gicirq < 32);
How did you decide that? There is no such specification anywhere.
I am basically looking at system suspend, where cores would be in power collapse.
+
+ reg_offset = gicirq / 32;
+ bit_offset = gicirq % 32;
+
+ if (on)
+ gic_data->wakeup_irqs[reg_offset] |= 1 << bit_offset;
+ else
+ gic_data->wakeup_irqs[reg_offset] &= ~(1 << bit_offset);
+
+ return ret;
+}
+
+#else
+#define gic_set_wake NULL
+#endif
+
#ifdef CONFIG_CPU_PM
/* Check whether it's single security state view */
static bool gic_dist_security_disabled(void)
@@ -707,6 +813,7 @@ static struct irq_chip gic_chip = {
.irq_eoi = gic_eoi_irq,
.irq_set_type = gic_set_type,
.irq_set_affinity = gic_set_affinity,
+ .irq_set_wake = gic_set_wake,
.irq_get_irqchip_state = gic_irq_get_irqchip_state,
.irq_set_irqchip_state = gic_irq_set_irqchip_state,
.flags = IRQCHIP_SET_TYPE_MASKED,
@@ -723,6 +830,7 @@ static struct irq_chip gic_eoimode1_chip = {
.irq_set_irqchip_state = gic_irq_set_irqchip_state,
.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity,
.flags = IRQCHIP_SET_TYPE_MASKED,
+ .irq_set_wake = gic_set_wake,
Keep the fields in the same order.
};
#define GIC_ID_NR (1U << gic_data.rdists.id_bits)
But here's my fundamental objection: None of that should be required and
setting (IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND) as part of the
irqchip flags should be enough.
Can you explain why this doesn't work for you?
These flags work for me, I will remove suspend/resume functions, but i think
it is very useful to know the wakeup source for debugging purposes.
Please let me know, if you have any better way to achieve this.
Thanks,
M.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
