[RFC] irqchip/gic-v3: Implement suspend and resume callbacks
Chandra Sekhar Lingutla
clingutla at codeaurora.org
Tue Sep 27 05:35:00 PDT 2016
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 at codeaurora.org>
>>
>> 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
More information about the linux-arm-kernel
mailing list