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