[RFC] irqchip/gic-v3: Implement suspend and resume callbacks

[Marc Zyngier]

+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?

> +#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.

> +		/* 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.

> +
> +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.

> +
> +	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?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...