diff mbox

[4/8] irqchip/gic-v3: add ability to save/restore GIC/ITS state

Message ID 20180112212422.148625-5-dbasehore@chromium.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Derek Basehore Jan. 12, 2018, 9:24 p.m. UTC
Some platforms power off GIC logic in S3, so we need to save/restore
state. This adds a DT-binding to save/restore the GICD/GICR/GITS
states using the new CPU_PM_SYSTEM_ENTER/EXIT CPU PM states.

Change-Id: I1fb2117296373fa67397fdd4a8960077b241462e
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/irqchip/irq-gic-v3-its.c   |  51 ++++++
 drivers/irqchip/irq-gic-v3.c       | 333 +++++++++++++++++++++++++++++++++++--
 include/linux/irqchip/arm-gic-v3.h |  17 ++
 3 files changed, 391 insertions(+), 10 deletions(-)

Comments

Marc Zyngier Jan. 13, 2018, 6:10 p.m. UTC | #1
[I remember asking you to copy Sudeep Hola on this. Please do so the
next time around]

On Fri, 12 Jan 2018 21:24:18 +0000,
Derek Basehore wrote:
> 
> Some platforms power off GIC logic in S3, so we need to save/restore

S3 is a not a GIC concept, and is only vaguely mentioned in terms of
the rk3399 silicon, if grep serves me right. Please expand on what
state this is exactly.

> state. This adds a DT-binding to save/restore the GICD/GICR/GITS
> states using the new CPU_PM_SYSTEM_ENTER/EXIT CPU PM states.

DT binding? I can't see any in this patch.

> 
> Change-Id: I1fb2117296373fa67397fdd4a8960077b241462e

It's been mentioned somewhere else in the thread: these tags have no
purpose in the kernel. Please sanitise your patches before posting them.

> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Who is the author of this patch? If that's a joined authorship, please
use the Co-Developed-by: tag.

> ---
>  drivers/irqchip/irq-gic-v3-its.c   |  51 ++++++
>  drivers/irqchip/irq-gic-v3.c       | 333 +++++++++++++++++++++++++++++++++++--
>  include/linux/irqchip/arm-gic-v3.h |  17 ++
>  3 files changed, 391 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..5cb808e3d0bf 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -85,6 +85,16 @@ struct its_baser {
>  
>  struct its_device;
>  
> +/*
> + * Saved ITS state - this is where saved state for the ITS is stored
> + * when it's disabled during system suspend.
> + */
> +struct its_ctx {
> +	u64			cbaser;
> +	u64			cwriter;

Why do you need to save cwriter? Do you expect to perform a
save/restore in the middle of a set of commands? I would really expect
the system to be in a quiescent state, and the command queue to be
reset to an empty state on resume. WHy isn't it so?

> +	u32			ctlr;
> +};
> +
>  /*
>   * The ITS structure - contains most of the infrastructure, with the
>   * top-level MSI domain, the command queue, the collections, and the
> @@ -101,6 +111,7 @@ struct its_node {
>  	struct its_collection	*collections;
>  	struct fwnode_handle	*fwnode_handle;
>  	u64			(*get_msi_base)(struct its_device *its_dev);
> +	struct its_ctx		its_ctx;
>  	struct list_head	its_device_list;
>  	u64			flags;
>  	unsigned long		list_nr;
> @@ -3042,6 +3053,46 @@ static void its_enable_quirks(struct its_node *its)
>  	gic_enable_quirks(iidr, its_quirks, its);
>  }
>  
> +void its_save_disable(void)
> +{
> +	struct its_node *its;
> +
> +	spin_lock(&its_lock);
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		struct its_ctx *ctx = &its->its_ctx;
> +		void __iomem *base = its->base;
> +		int i;
> +
> +		ctx->ctlr = readl_relaxed(base + GITS_CTLR);
> +		its_force_quiescent(base);

What if the ITS fails to become quiescent?

> +		ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
> +		ctx->cwriter = readq_relaxed(base + GITS_CWRITER);

How about those systems that do not have a readq (32bit)? Please make
sure this builds on 32bit too.

> +		for (i = 0; i < ARRAY_SIZE(its->tables); i++)
> +			its->tables[i].val = its_read_baser(its, &its->tables[i]);
> +	}
> +	spin_unlock(&its_lock);
> +}
> +
> +void its_restore_enable(void)
> +{
> +	struct its_node *its;
> +
> +	spin_lock(&its_lock);
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		struct its_ctx *ctx = &its->its_ctx;
> +		void __iomem *base = its->base;
> +		int i;
> +
> +		gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
> +		gits_write_cwriter(ctx->cwriter, base + GITS_CWRITER);
> +		for (i = 0; i < ARRAY_SIZE(its->tables); i++)
> +			its_write_baser(its, &its->tables[i],
> +					its->tables[i].val);
> +		writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> +	}
> +	spin_unlock(&its_lock);
> +}
> +
>  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>  {
>  	struct irq_domain *inner_domain;
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 9a7a15049903..95d37fb6f458 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -47,6 +47,36 @@ struct redist_region {
>  	bool			single_redist;
>  };
>  
> +struct gic_dist_ctx {
> +	u64			*irouter;
> +	u32			*igroupr;
> +	u32			*isenabler;
> +	u32			*ispendr;
> +	u32			*isactiver;
> +	u32			*ipriorityr;
> +	u32			*icfgr;
> +	u32			*igrpmodr;
> +	u32			*nsacr;
> +	u32			ctlr;
> +};
> +
> +struct gic_redist_ctx {
> +	u64			propbaser;
> +	u64			pendbaser;
> +	u32			ipriorityr[8];
> +	u32			ctlr;
> +	u32			igroupr0;
> +	u32			isenabler0;
> +	u32			ispendr0;
> +	u32			isactiver0;
> +	u32			icfgr0;
> +	u32			icfgr1;
> +	u32			igrpmodr0;
> +	u32			nsacr;
> +};
> +
> +#define GICD_NR_REGS(reg, nr_irq)	((nr_irq) >> GICD_## reg ##_SHIFT)
> +
>  struct gic_chip_data {
>  	struct fwnode_handle	*fwnode;
>  	void __iomem		*dist_base;
> @@ -58,6 +88,8 @@ struct gic_chip_data {
>  	bool			has_rss;
>  	unsigned int		irq_nr;
>  	struct partition_desc	*ppi_descs[16];
> +	struct gic_dist_ctx	*gicd_ctx;
> +	struct gic_redist_ctx	*gicr_ctx;
>  };
>  
>  static struct gic_chip_data gic_data __read_mostly;
> @@ -133,13 +165,13 @@ static u64 __maybe_unused gic_read_iar(void)
>  }
>  #endif
>  
> -static void gic_enable_redist(bool enable)
> +static void gic_enable_redist_base(int cpu, bool enable)

_base? What does _base mean here? Shouldn't it be something like
gic_enable_redist_on_cpu(), or something along these lines?

>  {
>  	void __iomem *rbase;
>  	u32 count = 1000000;	/* 1s! */
>  	u32 val;
>  
> -	rbase = gic_data_rdist_rd_base();
> +	rbase = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
>  
>  	val = readl_relaxed(rbase + GICR_WAKER);
>  	if (enable)
> @@ -167,6 +199,11 @@ static void gic_enable_redist(bool enable)
>  				   enable ? "wakeup" : "sleep");
>  }
>  
> +static void gic_enable_redist(bool enable)
> +{
> +	gic_enable_redist_base(smp_processor_id(), enable);
> +}
> +
>  /*
>   * Routines to disable, enable, EOI and route interrupts
>   */
> @@ -757,6 +794,155 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>  #define gic_smp_init()		do { } while(0)
>  #endif
>  
> +static void gic_redist_save(int cpu)
> +{
> +	struct gic_redist_ctx *ctx = &gic_data.gicr_ctx[cpu];
> +	void __iomem *base = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
> +
> +	gic_do_wait_for_rwp(base);
> +	ctx->ctlr = readl_relaxed(base + GICR_CTLR);
> +	ctx->propbaser = readq_relaxed(base + GICR_PROPBASER);

Why is this a per-redistributor data?

> +	ctx->pendbaser = readq_relaxed(base + GICR_PENDBASER);

Use the relevant accessors that will ensure proper compilation on 32bit.

> +
> +	base += GICR_SGI_BASE_OFFSET;
> +	ctx->igroupr0 = readl_relaxed(base + GICR_IGROUPR0);
> +	ctx->isenabler0 = readl_relaxed(base + GICR_ISENABLER0);
> +	ctx->ispendr0 = readl_relaxed(base + GICR_ISPENDR0);
> +	ctx->isactiver0 = readl_relaxed(base + GICR_ISACTIVER0);
> +	__ioread32_copy(ctx->ipriorityr, base + GICR_IPRIORITYR0, 8);

WHy d we need to save the priorities? Aren't they known to be fixed?

> +	ctx->icfgr0 = readl_relaxed(base + GICR_ICFGR0);
> +	ctx->icfgr1 = readl_relaxed(base + GICR_ICFGR0 + sizeof(u32));
> +	ctx->igrpmodr0 = readl_relaxed(base + GICR_IGRPMODR0);

What's the purpose of saving GICR_IGRPMODR0?

> +	ctx->nsacr = readl_relaxed(base + GICR_NSACR);

What is the purpose of saving NSACR?

> +}
> +
> +static void gic_dist_save(void)
> +{
> +	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
> +	void __iomem *base = gic_data.dist_base;
> +	int irq_nr = gic_data.irq_nr;
> +
> +	__ioread64_copy(ctx->irouter, base + GICD_IROUTER,
> +			GICD_NR_REGS(IROUTER, irq_nr));
> +	__ioread32_copy(ctx->igroupr, base + GICD_IGROUPR,
> +			GICD_NR_REGS(IGROUPR, irq_nr));
> +	__ioread32_copy(ctx->isenabler, base + GICD_ISENABLER,
> +			GICD_NR_REGS(ISENABLER, irq_nr));
> +	__ioread32_copy(ctx->ispendr, base + GICD_ISPENDR,
> +			GICD_NR_REGS(ISPENDR, irq_nr));
> +	__ioread32_copy(ctx->isactiver, base + GICD_ISACTIVER,
> +			GICD_NR_REGS(ISACTIVER, irq_nr));
> +	__ioread32_copy(ctx->icfgr, base + GICD_ICFGR,
> +			GICD_NR_REGS(ICFGR, irq_nr));
> +	__ioread32_copy(ctx->igrpmodr, base + GICD_IGRPMODR,
> +			GICD_NR_REGS(IGRPMODR, irq_nr));
> +	__ioread32_copy(ctx->nsacr, base + GICD_NSACR,
> +			GICD_NR_REGS(NSACR, irq_nr));
> +	ctx->ctlr = readl_relaxed(base + GICD_CTLR);

The GICv1/v2 driver already has code to that effect. Do we really need
to reinvent the wheel? You just need to special case the few GICv3
specific registers.

Also, this driver doesn't use the __ioread/__iowrite stuff. They hide
important information (relaxed/non-relaxed), and have very imprecise
semantics on 32bit systems when used with 64bit accessors. Please get
rid of them and be aware of the 32bit limitations.

> +}
> +
> +static int gic_suspend(void)
> +{
> +	void __iomem *rbase;
> +	int cpu;
> +
> +	if (!gic_data.gicd_ctx || !gic_data.gicr_ctx)
> +		return 0;
> +
> +	for_each_possible_cpu(cpu) {
> +		/*
> +		 * The current processor will have the redistributor disabled in
> +		 * the next step.
> +		 */
> +		if (cpu == smp_processor_id())
> +			continue;
> +
> +		/* Each redistributor must be disabled to save state. */
> +		rbase = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
> +		if (!(readl_relaxed(rbase + GICR_WAKER) &
> +		     GICR_WAKER_ProcessorSleep)) {
> +			pr_err("Redistributor for CPU: %d enabled \n", cpu);
> +			return -EPERM;
> +		}

How can this happen? Is this just to be on the safe side?

> +	}
> +
> +	/* Disable the redist in case it wasn't in CPU_PM_ENTER. */
> +	gic_enable_redist(false);
> +	for_each_possible_cpu(cpu)
> +		gic_redist_save(cpu);
> +
> +	its_save_disable();

Why do we need to tie up the core GIC and the ITS? Can't they have
independent save/restore entry points?

> +	gic_dist_save();
> +
> +	return 0;
> +}
> +
> +static void gic_rdist_restore(int cpu)
> +{
> +	struct gic_redist_ctx *ctx = &gic_data.gicr_ctx[cpu];
> +	void __iomem *base = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
> +	void __iomem *sgi_base = base + GICR_SGI_BASE_OFFSET;
> +
> +	writel_relaxed(ctx->ctlr & ~(GICR_CTLR_ENABLE_LPIS), base + GICR_CTLR);
> +	gic_do_wait_for_rwp(base);
> +	writeq_relaxed(ctx->propbaser, base + GICR_PROPBASER);
> +	writeq_relaxed(ctx->pendbaser, base + GICR_PENDBASER);

Without checking for GICR_CTLR.EnableLPIs first? You're heading
straight into UNPRED territory.

> +
> +	writel_relaxed(ctx->igroupr0, sgi_base + GICR_IGROUPR0);
> +	writel_relaxed(ctx->isenabler0, sgi_base + GICR_ISENABLER0);
> +	writel_relaxed(ctx->ispendr0, sgi_base + GICR_ISPENDR0);
> +	writel_relaxed(ctx->isactiver0, sgi_base + GICR_ISACTIVER0);
> +	__iowrite32_copy(sgi_base + GICR_IPRIORITYR0, ctx->ipriorityr, 8);
> +	writel_relaxed(ctx->icfgr0, sgi_base + GICR_ICFGR0);
> +	writel_relaxed(ctx->icfgr1, sgi_base + GICR_ICFGR0 + sizeof(u32));
> +	writel_relaxed(ctx->igrpmodr0, sgi_base + GICR_IGRPMODR0);
> +	writel_relaxed(ctx->nsacr, sgi_base + GICR_NSACR);
> +	writel_relaxed(ctx->ctlr, sgi_base + GICR_CTLR);
> +	gic_do_wait_for_rwp(base);

Most of the remarks I had for the save side apply here too.

> +}
> +
> +static void gic_dist_restore(void)
> +{
> +	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
> +	void __iomem *base = gic_data.dist_base;
> +	int irq_nr = gic_data.irq_nr;
> +
> +	gic_dist_wait_for_rwp();
> +	__iowrite64_copy(base + GICD_IROUTER, ctx->irouter,
> +			 GICD_NR_REGS(IROUTER, irq_nr));
> +	__iowrite32_copy(base + GICD_IGROUPR, ctx->igroupr,
> +			 GICD_NR_REGS(IGROUPR, irq_nr));
> +	__iowrite32_copy(base + GICD_ISENABLER, ctx->isenabler,
> +			 GICD_NR_REGS(ISENABLER, irq_nr));
> +	__iowrite32_copy(base + GICD_ISPENDR, ctx->ispendr,
> +			 GICD_NR_REGS(ISPENDR, irq_nr));
> +	__iowrite32_copy(base + GICD_ISACTIVER, ctx->isactiver,
> +			 GICD_NR_REGS(ISACTIVER, irq_nr));
> +	__iowrite32_copy(base + GICD_ICFGR, ctx->icfgr,
> +			 GICD_NR_REGS(ICFGR, irq_nr));
> +	__iowrite32_copy(base + GICD_IGRPMODR, ctx->igrpmodr,
> +			 GICD_NR_REGS(IGRPMODR, irq_nr));
> +	__iowrite32_copy(base + GICD_NSACR, ctx->nsacr,
> +			 GICD_NR_REGS(NSACR, irq_nr));
> +	writel_relaxed(ctx->ctlr, base + GICD_CTLR);
> +	gic_dist_wait_for_rwp();

Same thing.

> +}
> +
> +static void gic_resume(void)
> +{
> +	int cpu;
> +
> +	if (!gic_data.gicd_ctx || !gic_data.gicr_ctx)
> +		return;
> +
> +	gic_dist_restore();
> +	its_restore_enable();
> +	for_each_possible_cpu(cpu)
> +		gic_rdist_restore(cpu);
> +
> +	gic_enable_redist(true);
> +}
> +
>  #ifdef CONFIG_CPU_PM
>  /* Check whether it's single security state view */
>  static bool gic_dist_security_disabled(void)
> @@ -767,13 +953,24 @@ static bool gic_dist_security_disabled(void)
>  static int gic_cpu_pm_notifier(struct notifier_block *self,
>  			       unsigned long cmd, void *v)
>  {
> -	if (cmd == CPU_PM_EXIT) {
> +	switch (cmd) {
> +	case CPU_PM_EXIT:
>  		if (gic_dist_security_disabled())
>  			gic_enable_redist(true);
>  		gic_cpu_sys_reg_init();
> -	} else if (cmd == CPU_PM_ENTER && gic_dist_security_disabled()) {
> -		gic_write_grpen1(0);
> -		gic_enable_redist(false);
> +		break;
> +	case CPU_PM_ENTER:
> +		if (gic_dist_security_disabled()) {
> +			gic_write_grpen1(0);
> +			gic_enable_redist(false);
> +		}
> +		break;
> +	case CPU_SYSTEM_PM_EXIT:
> +		gic_resume();
> +		break;
> +	case CPU_SYSTEM_PM_ENTER:
> +		gic_suspend();
> +		break;
>  	}
>  	return NOTIFY_OK;
>  }
> @@ -991,11 +1188,99 @@ static const struct irq_domain_ops partition_domain_ops = {
>  	.select = gic_irq_domain_select,
>  };
>  
> +static int gic_populate_ctx(struct gic_dist_ctx *ctx, int irqs)

This isn't the GIC context. This is the save area. Please name the
function accordingly.

> +{
> +	int err;
> +
> +	ctx->irouter = kcalloc(GICD_NR_REGS(IROUTER, irqs),
> +			       sizeof(*ctx->irouter), GFP_KERNEL);
> +	if (IS_ERR(ctx->irouter))
> +		return PTR_ERR(ctx->irouter);
> +
> +	ctx->igroupr = kcalloc(GICD_NR_REGS(IGROUPR, irqs),
> +			       sizeof(*ctx->igroupr), GFP_KERNEL);
> +	if (IS_ERR(ctx->igroupr)) {
> +		err = PTR_ERR(ctx->igroupr);
> +		goto out_irouter;
> +	}
> +
> +	ctx->isenabler = kcalloc(GICD_NR_REGS(ISENABLER, irqs),
> +				 sizeof(*ctx->isenabler), GFP_KERNEL);
> +	if (IS_ERR(ctx->isenabler)) {
> +		err = PTR_ERR(ctx->isenabler);
> +		goto out_igroupr;
> +	}
> +
> +	ctx->ispendr = kcalloc(GICD_NR_REGS(ISPENDR, irqs),
> +			       sizeof(*ctx->ispendr), GFP_KERNEL);
> +	if (IS_ERR(ctx->ispendr)) {
> +		err = PTR_ERR(ctx->ispendr);
> +		goto out_isenabler;
> +	}
> +
> +	ctx->isactiver = kcalloc(GICD_NR_REGS(ISACTIVER, irqs),
> +				 sizeof(*ctx->isactiver), GFP_KERNEL);
> +	if (IS_ERR(ctx->isactiver)) {
> +		err = PTR_ERR(ctx->isactiver);
> +		goto out_ispendr;
> +	}
> +
> +	ctx->ipriorityr = kcalloc(GICD_NR_REGS(IPRIORITYR, irqs),
> +				  sizeof(*ctx->ipriorityr), GFP_KERNEL);
> +	if (IS_ERR(ctx->ipriorityr)) {
> +		err = PTR_ERR(ctx->ipriorityr);
> +		goto out_isactiver;
> +	}
> +
> +	ctx->icfgr = kcalloc(GICD_NR_REGS(ICFGR, irqs), sizeof(*ctx->icfgr),
> +			     GFP_KERNEL);
> +	if (IS_ERR(ctx->icfgr)) {
> +		err = PTR_ERR(ctx->icfgr);
> +		goto out_ipriorityr;
> +	}
> +
> +	ctx->igrpmodr = kcalloc(GICD_NR_REGS(IGRPMODR, irqs),
> +				sizeof(*ctx->igrpmodr), GFP_KERNEL);
> +	if (IS_ERR(ctx->igrpmodr)) {
> +		err = PTR_ERR(ctx->igrpmodr);
> +		goto out_icfgr;
> +	}
> +
> +	ctx->nsacr = kcalloc(GICD_NR_REGS(NSACR, irqs), sizeof(*ctx->nsacr),
> +			     GFP_KERNEL);
> +	if (IS_ERR(ctx->nsacr)) {
> +		err = PTR_ERR(ctx->nsacr);
> +		goto out_igrpmodr;
> +	}
> +
> +	return 0;
> +
> +out_irouter:
> +	kfree(ctx->irouter);
> +out_igroupr:
> +	kfree(ctx->igroupr);
> +out_isenabler:
> +	kfree(ctx->isenabler);
> +out_ispendr:
> +	kfree(ctx->ispendr);
> +out_isactiver:
> +	kfree(ctx->isactiver);
> +out_ipriorityr:
> +	kfree(ctx->ipriorityr);
> +out_icfgr:
> +	kfree(ctx->icfgr);
> +out_igrpmodr:
> +	kfree(ctx->igrpmodr);

WHy do you need all of these labels? Can't you just branch to an error
one and free them all in one go? In the end, what is the point of
keeping almost all of them if the last allocation fails?

> +	return err;
> +}
> +
>  static int __init gic_init_bases(void __iomem *dist_base,
>  				 struct redist_region *rdist_regs,
>  				 u32 nr_redist_regions,
>  				 u64 redist_stride,
> -				 struct fwnode_handle *handle)
> +				 struct fwnode_handle *handle,
> +				 struct gic_dist_ctx *gicd_ctx,
> +				 struct gic_redist_ctx *gicr_ctx)
>  {
>  	u32 typer;
>  	int gic_irqs;
> @@ -1012,6 +1297,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  	gic_data.redist_regions = rdist_regs;
>  	gic_data.nr_redist_regions = nr_redist_regions;
>  	gic_data.redist_stride = redist_stride;
> +	gic_data.gicd_ctx = gicd_ctx;
> +	gic_data.gicr_ctx = gicr_ctx;
>  
>  	/*
>  	 * Find out how many interrupts are supported.
> @@ -1035,6 +1322,12 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  		goto out_free;
>  	}
>  
> +	if (gicd_ctx) {
> +		err = gic_populate_ctx(gicd_ctx, gic_irqs);
> +		if (err)
> +			goto out_free;
> +	}
> +
>  	gic_data.has_rss = !!(typer & GICD_TYPER_RSS);
>  	pr_info("Distributor has %sRange Selector support\n",
>  		gic_data.has_rss ? "" : "no ");
> @@ -1190,6 +1483,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>  {
>  	void __iomem *dist_base;
>  	struct redist_region *rdist_regs;
> +	struct gic_dist_ctx *gicd_ctx = NULL;
> +	struct gic_redist_ctx *gicr_ctx = NULL;
>  	u64 redist_stride;
>  	u32 nr_redist_regions;
>  	int err, i;
> @@ -1232,17 +1527,35 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>  	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
>  		redist_stride = 0;
>  
> +	if (of_property_read_bool(node, "save-suspend-state")) {
> +		gicd_ctx = kzalloc(sizeof(*gicd_ctx), GFP_KERNEL);
> +		if (IS_ERR(gicd_ctx)) {
> +			err = PTR_ERR(gicd_ctx);
> +			goto out_unmap_rdist;
> +		}
> +
> +		gicr_ctx = kcalloc(num_possible_cpus(), sizeof(*gicr_ctx),
> +				   GFP_KERNEL);

Since this is a per-cpu allocation, why isn't this a per-cpu
allocation? In other words, why isn't the GICR save area allocated as
CPUs get matched against their redistributors instead?

> +		if (IS_ERR(gicr_ctx)) {
> +			err = PTR_ERR(gicr_ctx);
> +			goto out_free_gicd_ctx;
> +		}
> +	}

You really want to kill the box because something went wrong in your
save area allocation? It doesn't feel quite right.

> +
>  	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
> -			     redist_stride, &node->fwnode);
> +			     redist_stride, &node->fwnode, gicd_ctx, gicr_ctx);
>  	if (err)
> -		goto out_unmap_rdist;
> +		goto out_free_gicr_ctx;
>  
>  	gic_populate_ppi_partitions(node);
>  
>  	if (static_key_true(&supports_deactivate))
>  		gic_of_setup_kvm_info(node);
>  	return 0;
> -
> +out_free_gicr_ctx:
> +	kfree(gicr_ctx);
> +out_free_gicd_ctx:
> +	kfree(gicd_ctx);
>  out_unmap_rdist:
>  	for (i = 0; i < nr_redist_regions; i++)
>  		if (rdist_regs[i].redist_base)
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index c00c4c33e432..f086987e3cb4 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -46,6 +46,19 @@
>  #define GICD_IDREGS			0xFFD0
>  #define GICD_PIDR2			0xFFE8
>  
> +#define GICD_IGROUPR_SHIFT		5
> +#define GICD_ISENABLER_SHIFT		5
> +#define GICD_ICENABLER_SHIFT		GICD_ISENABLER_SHIFT
> +#define GICD_ISPENDR_SHIFT		5
> +#define GICD_ICPENDR_SHIFT		GICD_ISPENDR_SHIFT
> +#define GICD_ISACTIVER_SHIFT		5
> +#define GICD_ICACTIVER_SHIFT		GICD_ISACTIVER_SHIFT
> +#define GICD_IPRIORITYR_SHIFT		2
> +#define GICD_ICFGR_SHIFT		4
> +#define GICD_IGRPMODR_SHIFT		5
> +#define GICD_NSACR_SHIFT		4
> +#define GICD_IROUTER_SHIFT		0

No, please. We use the SHIFT/MASK suffixes to indicate bit fields. Do
not overload this term with other meanings in the context of this
driver.

> +
>  /*
>   * Those registers are actually from GICv2, but the spec demands that they
>   * are implemented as RES0 if ARE is 1 (which we do in KVM's emulated GICv3).
> @@ -188,6 +201,8 @@
>  
>  #define GICR_PENDBASER_PTZ				BIT_ULL(62)
>  
> +#define GICR_SGI_BASE_OFFSET		(1U << 16)
> +
>  /*
>   * Re-Distributor registers, offsets from SGI_base
>   */
> @@ -581,6 +596,8 @@ struct rdists {
>  
>  struct irq_domain;
>  struct fwnode_handle;
> +void its_save_disable(void);
> +void its_restore_enable(void);
>  int its_cpu_init(void);
>  int its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  	     struct irq_domain *domain);
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

Overall, this patch needs quite a bit of rework. You should take into
account *how* the GIC is used in Linux, and not blindly copy the whole
of the register state. You should be more careful with 32bit (at least
make sure it still compiles and works in a guest). Also, there is a
lot of scope to reuse existing code on the GICv1/2 side, so please
investigate this.

Finally, please split this patch so that the ITS and the core GICv3
code are patches separately.

Thanks,

	M.
Brian Norris Jan. 18, 2018, 11:33 p.m. UTC | #2
Hi,

On Sat, Jan 13, 2018 at 06:10:52PM +0000, Marc Zyngier wrote:
> On Fri, 12 Jan 2018 21:24:18 +0000,
> Derek Basehore wrote:
> > 
> > Some platforms power off GIC logic in S3, so we need to save/restore
> 
> S3 is a not a GIC concept, and is only vaguely mentioned in terms of
> the rk3399 silicon, if grep serves me right. Please expand on what
> state this is exactly.
> 
> > state. This adds a DT-binding to save/restore the GICD/GICR/GITS
> > states using the new CPU_PM_SYSTEM_ENTER/EXIT CPU PM states.
> 
> DT binding? I can't see any in this patch.
> 
> > 
> > Change-Id: I1fb2117296373fa67397fdd4a8960077b241462e
> 
> It's been mentioned somewhere else in the thread: these tags have no
> purpose in the kernel. Please sanitise your patches before posting them.
> 
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> 
> Who is the author of this patch? If that's a joined authorship, please
> use the Co-Developed-by: tag.

I only did some minimal code shuffling when rebasing and working with
this code in our downstream tree. I probably didn't actually need to
apply my Signed-off-by at the time, but Derek carried it along anyway.

Derek is the author, and I'd be perfectly fine dropping my S-o-b from
these patches.

> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 9a7a15049903..95d37fb6f458 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c

...

> > @@ -991,11 +1188,99 @@ static const struct irq_domain_ops partition_domain_ops = {
> >  	.select = gic_irq_domain_select,
> >  };
> >  
> > +static int gic_populate_ctx(struct gic_dist_ctx *ctx, int irqs)
> 
> This isn't the GIC context. This is the save area. Please name the
> function accordingly.
> 
> > +{
> > +	int err;
> > +
> > +	ctx->irouter = kcalloc(GICD_NR_REGS(IROUTER, irqs),
> > +			       sizeof(*ctx->irouter), GFP_KERNEL);
> > +	if (IS_ERR(ctx->irouter))
> > +		return PTR_ERR(ctx->irouter);
> > +
> > +	ctx->igroupr = kcalloc(GICD_NR_REGS(IGROUPR, irqs),
> > +			       sizeof(*ctx->igroupr), GFP_KERNEL);
> > +	if (IS_ERR(ctx->igroupr)) {
> > +		err = PTR_ERR(ctx->igroupr);
> > +		goto out_irouter;
> > +	}
> > +
> > +	ctx->isenabler = kcalloc(GICD_NR_REGS(ISENABLER, irqs),
> > +				 sizeof(*ctx->isenabler), GFP_KERNEL);
> > +	if (IS_ERR(ctx->isenabler)) {
> > +		err = PTR_ERR(ctx->isenabler);
> > +		goto out_igroupr;
> > +	}
> > +
> > +	ctx->ispendr = kcalloc(GICD_NR_REGS(ISPENDR, irqs),
> > +			       sizeof(*ctx->ispendr), GFP_KERNEL);
> > +	if (IS_ERR(ctx->ispendr)) {
> > +		err = PTR_ERR(ctx->ispendr);
> > +		goto out_isenabler;
> > +	}
> > +
> > +	ctx->isactiver = kcalloc(GICD_NR_REGS(ISACTIVER, irqs),
> > +				 sizeof(*ctx->isactiver), GFP_KERNEL);
> > +	if (IS_ERR(ctx->isactiver)) {
> > +		err = PTR_ERR(ctx->isactiver);
> > +		goto out_ispendr;
> > +	}
> > +
> > +	ctx->ipriorityr = kcalloc(GICD_NR_REGS(IPRIORITYR, irqs),
> > +				  sizeof(*ctx->ipriorityr), GFP_KERNEL);
> > +	if (IS_ERR(ctx->ipriorityr)) {
> > +		err = PTR_ERR(ctx->ipriorityr);
> > +		goto out_isactiver;
> > +	}
> > +
> > +	ctx->icfgr = kcalloc(GICD_NR_REGS(ICFGR, irqs), sizeof(*ctx->icfgr),
> > +			     GFP_KERNEL);
> > +	if (IS_ERR(ctx->icfgr)) {
> > +		err = PTR_ERR(ctx->icfgr);
> > +		goto out_ipriorityr;
> > +	}
> > +
> > +	ctx->igrpmodr = kcalloc(GICD_NR_REGS(IGRPMODR, irqs),
> > +				sizeof(*ctx->igrpmodr), GFP_KERNEL);
> > +	if (IS_ERR(ctx->igrpmodr)) {
> > +		err = PTR_ERR(ctx->igrpmodr);
> > +		goto out_icfgr;
> > +	}
> > +
> > +	ctx->nsacr = kcalloc(GICD_NR_REGS(NSACR, irqs), sizeof(*ctx->nsacr),
> > +			     GFP_KERNEL);
> > +	if (IS_ERR(ctx->nsacr)) {
> > +		err = PTR_ERR(ctx->nsacr);
> > +		goto out_igrpmodr;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_irouter:
> > +	kfree(ctx->irouter);
> > +out_igroupr:
> > +	kfree(ctx->igroupr);
> > +out_isenabler:
> > +	kfree(ctx->isenabler);
> > +out_ispendr:
> > +	kfree(ctx->ispendr);
> > +out_isactiver:
> > +	kfree(ctx->isactiver);
> > +out_ipriorityr:
> > +	kfree(ctx->ipriorityr);
> > +out_icfgr:
> > +	kfree(ctx->icfgr);
> > +out_igrpmodr:
> > +	kfree(ctx->igrpmodr);
> 
> WHy do you need all of these labels? Can't you just branch to an error
> one and free them all in one go?

If you assume that the memory was initially all zero (which it is --
kzalloc()), then you can get away with a single error label (kfree(0) is
a no-op). But otherwise, this is the right way to do error handling...

> In the end, what is the point of
> keeping almost all of them if the last allocation fails?

I didn't understand this question until I realized that the error
handling is all accidentally done in reverse. If we fail the last
allocation, we should be freeing all but the last one. But this code
actually only frees 1 bit of memory in that case (i.e., a memory leak).

> > +	return err;
> > +}
> > +
> >  static int __init gic_init_bases(void __iomem *dist_base,
> >  				 struct redist_region *rdist_regs,
> >  				 u32 nr_redist_regions,
> >  				 u64 redist_stride,
> > -				 struct fwnode_handle *handle)
> > +				 struct fwnode_handle *handle,
> > +				 struct gic_dist_ctx *gicd_ctx,
> > +				 struct gic_redist_ctx *gicr_ctx)
> >  {
> >  	u32 typer;
> >  	int gic_irqs;

...

> > @@ -1232,17 +1527,35 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
> >  	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
> >  		redist_stride = 0;
> >  
> > +	if (of_property_read_bool(node, "save-suspend-state")) {
> > +		gicd_ctx = kzalloc(sizeof(*gicd_ctx), GFP_KERNEL);
> > +		if (IS_ERR(gicd_ctx)) {
> > +			err = PTR_ERR(gicd_ctx);
> > +			goto out_unmap_rdist;
> > +		}
> > +
> > +		gicr_ctx = kcalloc(num_possible_cpus(), sizeof(*gicr_ctx),
> > +				   GFP_KERNEL);
> 
> Since this is a per-cpu allocation, why isn't this a per-cpu
> allocation? In other words, why isn't the GICR save area allocated as
> CPUs get matched against their redistributors instead?
> 
> > +		if (IS_ERR(gicr_ctx)) {
> > +			err = PTR_ERR(gicr_ctx);
> > +			goto out_free_gicd_ctx;
> > +		}
> > +	}
> 
> You really want to kill the box because something went wrong in your
> save area allocation? It doesn't feel quite right.

Isn't that what all drivers (including irqchip drivers) do on failed
allocations? What else would we do? Pretend that we can limp along and
just b0rk the system when it suspends?

Brian
Marc Zyngier Jan. 19, 2018, 9:22 a.m. UTC | #3
On 18/01/18 23:33, Brian Norris wrote:
> Hi,
> 
> On Sat, Jan 13, 2018 at 06:10:52PM +0000, Marc Zyngier wrote:
>> On Fri, 12 Jan 2018 21:24:18 +0000,
>> Derek Basehore wrote:
>>>
>>> Some platforms power off GIC logic in S3, so we need to save/restore
>>
>> S3 is a not a GIC concept, and is only vaguely mentioned in terms of
>> the rk3399 silicon, if grep serves me right. Please expand on what
>> state this is exactly.
>>
>>> state. This adds a DT-binding to save/restore the GICD/GICR/GITS
>>> states using the new CPU_PM_SYSTEM_ENTER/EXIT CPU PM states.
>>
>> DT binding? I can't see any in this patch.
>>
>>>
>>> Change-Id: I1fb2117296373fa67397fdd4a8960077b241462e
>>
>> It's been mentioned somewhere else in the thread: these tags have no
>> purpose in the kernel. Please sanitise your patches before posting them.
>>
>>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>
>> Who is the author of this patch? If that's a joined authorship, please
>> use the Co-Developed-by: tag.
> 
> I only did some minimal code shuffling when rebasing and working with
> this code in our downstream tree. I probably didn't actually need to
> apply my Signed-off-by at the time, but Derek carried it along anyway.
> 
> Derek is the author, and I'd be perfectly fine dropping my S-o-b from
> these patches.
> 
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index 9a7a15049903..95d37fb6f458 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c

[...]

>>> +		if (IS_ERR(gicr_ctx)) {
>>> +			err = PTR_ERR(gicr_ctx);
>>> +			goto out_free_gicd_ctx;
>>> +		}
>>> +	}
>>
>> You really want to kill the box because something went wrong in your
>> save area allocation? It doesn't feel quite right.
> 
> Isn't that what all drivers (including irqchip drivers) do on failed
> allocations? What else would we do? Pretend that we can limp along and
> just b0rk the system when it suspends?
It would certainly give the user a chance to diagnostic the problem
(which is otherwise pretty hard if the system doesn't boot). We kill the
system if we cannot continue. In this case, we can. So why not try it?

Thanks,

	M.
Derek Basehore Jan. 19, 2018, 10:58 p.m. UTC | #4
On Fri, Jan 19, 2018 at 1:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 18/01/18 23:33, Brian Norris wrote:
>> Hi,
>>
>> On Sat, Jan 13, 2018 at 06:10:52PM +0000, Marc Zyngier wrote:
>>> On Fri, 12 Jan 2018 21:24:18 +0000,
>>> Derek Basehore wrote:
>>>>
>>>> Some platforms power off GIC logic in S3, so we need to save/restore
>>>
>>> S3 is a not a GIC concept, and is only vaguely mentioned in terms of
>>> the rk3399 silicon, if grep serves me right. Please expand on what
>>> state this is exactly.
>>>
>>>> state. This adds a DT-binding to save/restore the GICD/GICR/GITS
>>>> states using the new CPU_PM_SYSTEM_ENTER/EXIT CPU PM states.
>>>
>>> DT binding? I can't see any in this patch.
>>>
>>>>
>>>> Change-Id: I1fb2117296373fa67397fdd4a8960077b241462e
>>>
>>> It's been mentioned somewhere else in the thread: these tags have no
>>> purpose in the kernel. Please sanitise your patches before posting them.
>>>
>>>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>>
>>> Who is the author of this patch? If that's a joined authorship, please
>>> use the Co-Developed-by: tag.
>>
>> I only did some minimal code shuffling when rebasing and working with
>> this code in our downstream tree. I probably didn't actually need to
>> apply my Signed-off-by at the time, but Derek carried it along anyway.
>>
>> Derek is the author, and I'd be perfectly fine dropping my S-o-b from
>> these patches.
>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index 9a7a15049903..95d37fb6f458 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>
> [...]
>
>>>> +           if (IS_ERR(gicr_ctx)) {
>>>> +                   err = PTR_ERR(gicr_ctx);
>>>> +                   goto out_free_gicd_ctx;
>>>> +           }
>>>> +   }
>>>
>>> You really want to kill the box because something went wrong in your
>>> save area allocation? It doesn't feel quite right.
>>
>> Isn't that what all drivers (including irqchip drivers) do on failed
>> allocations? What else would we do? Pretend that we can limp along and
>> just b0rk the system when it suspends?
> It would certainly give the user a chance to diagnostic the problem
> (which is otherwise pretty hard if the system doesn't boot). We kill the
> system if we cannot continue. In this case, we can. So why not try it?

I'm in the middle of a lot of refactoring that will make this
irrelevant, so I guess we can leave it at that. I'll disable the
feature and print an error in the case of allocation failures (in the
parts that remain) in the next version.

Still debugging the broken ATF code which is now going to be used, so
no ETA on that.

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
diff mbox

Patch

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06f025fd5726..5cb808e3d0bf 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -85,6 +85,16 @@  struct its_baser {
 
 struct its_device;
 
+/*
+ * Saved ITS state - this is where saved state for the ITS is stored
+ * when it's disabled during system suspend.
+ */
+struct its_ctx {
+	u64			cbaser;
+	u64			cwriter;
+	u32			ctlr;
+};
+
 /*
  * The ITS structure - contains most of the infrastructure, with the
  * top-level MSI domain, the command queue, the collections, and the
@@ -101,6 +111,7 @@  struct its_node {
 	struct its_collection	*collections;
 	struct fwnode_handle	*fwnode_handle;
 	u64			(*get_msi_base)(struct its_device *its_dev);
+	struct its_ctx		its_ctx;
 	struct list_head	its_device_list;
 	u64			flags;
 	unsigned long		list_nr;
@@ -3042,6 +3053,46 @@  static void its_enable_quirks(struct its_node *its)
 	gic_enable_quirks(iidr, its_quirks, its);
 }
 
+void its_save_disable(void)
+{
+	struct its_node *its;
+
+	spin_lock(&its_lock);
+	list_for_each_entry(its, &its_nodes, entry) {
+		struct its_ctx *ctx = &its->its_ctx;
+		void __iomem *base = its->base;
+		int i;
+
+		ctx->ctlr = readl_relaxed(base + GITS_CTLR);
+		its_force_quiescent(base);
+		ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
+		ctx->cwriter = readq_relaxed(base + GITS_CWRITER);
+		for (i = 0; i < ARRAY_SIZE(its->tables); i++)
+			its->tables[i].val = its_read_baser(its, &its->tables[i]);
+	}
+	spin_unlock(&its_lock);
+}
+
+void its_restore_enable(void)
+{
+	struct its_node *its;
+
+	spin_lock(&its_lock);
+	list_for_each_entry(its, &its_nodes, entry) {
+		struct its_ctx *ctx = &its->its_ctx;
+		void __iomem *base = its->base;
+		int i;
+
+		gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
+		gits_write_cwriter(ctx->cwriter, base + GITS_CWRITER);
+		for (i = 0; i < ARRAY_SIZE(its->tables); i++)
+			its_write_baser(its, &its->tables[i],
+					its->tables[i].val);
+		writel_relaxed(ctx->ctlr, base + GITS_CTLR);
+	}
+	spin_unlock(&its_lock);
+}
+
 static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 {
 	struct irq_domain *inner_domain;
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 9a7a15049903..95d37fb6f458 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -47,6 +47,36 @@  struct redist_region {
 	bool			single_redist;
 };
 
+struct gic_dist_ctx {
+	u64			*irouter;
+	u32			*igroupr;
+	u32			*isenabler;
+	u32			*ispendr;
+	u32			*isactiver;
+	u32			*ipriorityr;
+	u32			*icfgr;
+	u32			*igrpmodr;
+	u32			*nsacr;
+	u32			ctlr;
+};
+
+struct gic_redist_ctx {
+	u64			propbaser;
+	u64			pendbaser;
+	u32			ipriorityr[8];
+	u32			ctlr;
+	u32			igroupr0;
+	u32			isenabler0;
+	u32			ispendr0;
+	u32			isactiver0;
+	u32			icfgr0;
+	u32			icfgr1;
+	u32			igrpmodr0;
+	u32			nsacr;
+};
+
+#define GICD_NR_REGS(reg, nr_irq)	((nr_irq) >> GICD_## reg ##_SHIFT)
+
 struct gic_chip_data {
 	struct fwnode_handle	*fwnode;
 	void __iomem		*dist_base;
@@ -58,6 +88,8 @@  struct gic_chip_data {
 	bool			has_rss;
 	unsigned int		irq_nr;
 	struct partition_desc	*ppi_descs[16];
+	struct gic_dist_ctx	*gicd_ctx;
+	struct gic_redist_ctx	*gicr_ctx;
 };
 
 static struct gic_chip_data gic_data __read_mostly;
@@ -133,13 +165,13 @@  static u64 __maybe_unused gic_read_iar(void)
 }
 #endif
 
-static void gic_enable_redist(bool enable)
+static void gic_enable_redist_base(int cpu, bool enable)
 {
 	void __iomem *rbase;
 	u32 count = 1000000;	/* 1s! */
 	u32 val;
 
-	rbase = gic_data_rdist_rd_base();
+	rbase = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
 
 	val = readl_relaxed(rbase + GICR_WAKER);
 	if (enable)
@@ -167,6 +199,11 @@  static void gic_enable_redist(bool enable)
 				   enable ? "wakeup" : "sleep");
 }
 
+static void gic_enable_redist(bool enable)
+{
+	gic_enable_redist_base(smp_processor_id(), enable);
+}
+
 /*
  * Routines to disable, enable, EOI and route interrupts
  */
@@ -757,6 +794,155 @@  static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 #define gic_smp_init()		do { } while(0)
 #endif
 
+static void gic_redist_save(int cpu)
+{
+	struct gic_redist_ctx *ctx = &gic_data.gicr_ctx[cpu];
+	void __iomem *base = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
+
+	gic_do_wait_for_rwp(base);
+	ctx->ctlr = readl_relaxed(base + GICR_CTLR);
+	ctx->propbaser = readq_relaxed(base + GICR_PROPBASER);
+	ctx->pendbaser = readq_relaxed(base + GICR_PENDBASER);
+
+	base += GICR_SGI_BASE_OFFSET;
+	ctx->igroupr0 = readl_relaxed(base + GICR_IGROUPR0);
+	ctx->isenabler0 = readl_relaxed(base + GICR_ISENABLER0);
+	ctx->ispendr0 = readl_relaxed(base + GICR_ISPENDR0);
+	ctx->isactiver0 = readl_relaxed(base + GICR_ISACTIVER0);
+	__ioread32_copy(ctx->ipriorityr, base + GICR_IPRIORITYR0, 8);
+	ctx->icfgr0 = readl_relaxed(base + GICR_ICFGR0);
+	ctx->icfgr1 = readl_relaxed(base + GICR_ICFGR0 + sizeof(u32));
+	ctx->igrpmodr0 = readl_relaxed(base + GICR_IGRPMODR0);
+	ctx->nsacr = readl_relaxed(base + GICR_NSACR);
+}
+
+static void gic_dist_save(void)
+{
+	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
+	void __iomem *base = gic_data.dist_base;
+	int irq_nr = gic_data.irq_nr;
+
+	__ioread64_copy(ctx->irouter, base + GICD_IROUTER,
+			GICD_NR_REGS(IROUTER, irq_nr));
+	__ioread32_copy(ctx->igroupr, base + GICD_IGROUPR,
+			GICD_NR_REGS(IGROUPR, irq_nr));
+	__ioread32_copy(ctx->isenabler, base + GICD_ISENABLER,
+			GICD_NR_REGS(ISENABLER, irq_nr));
+	__ioread32_copy(ctx->ispendr, base + GICD_ISPENDR,
+			GICD_NR_REGS(ISPENDR, irq_nr));
+	__ioread32_copy(ctx->isactiver, base + GICD_ISACTIVER,
+			GICD_NR_REGS(ISACTIVER, irq_nr));
+	__ioread32_copy(ctx->icfgr, base + GICD_ICFGR,
+			GICD_NR_REGS(ICFGR, irq_nr));
+	__ioread32_copy(ctx->igrpmodr, base + GICD_IGRPMODR,
+			GICD_NR_REGS(IGRPMODR, irq_nr));
+	__ioread32_copy(ctx->nsacr, base + GICD_NSACR,
+			GICD_NR_REGS(NSACR, irq_nr));
+	ctx->ctlr = readl_relaxed(base + GICD_CTLR);
+}
+
+static int gic_suspend(void)
+{
+	void __iomem *rbase;
+	int cpu;
+
+	if (!gic_data.gicd_ctx || !gic_data.gicr_ctx)
+		return 0;
+
+	for_each_possible_cpu(cpu) {
+		/*
+		 * The current processor will have the redistributor disabled in
+		 * the next step.
+		 */
+		if (cpu == smp_processor_id())
+			continue;
+
+		/* Each redistributor must be disabled to save state. */
+		rbase = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
+		if (!(readl_relaxed(rbase + GICR_WAKER) &
+		     GICR_WAKER_ProcessorSleep)) {
+			pr_err("Redistributor for CPU: %d enabled \n", cpu);
+			return -EPERM;
+		}
+	}
+
+	/* Disable the redist in case it wasn't in CPU_PM_ENTER. */
+	gic_enable_redist(false);
+	for_each_possible_cpu(cpu)
+		gic_redist_save(cpu);
+
+	its_save_disable();
+	gic_dist_save();
+
+	return 0;
+}
+
+static void gic_rdist_restore(int cpu)
+{
+	struct gic_redist_ctx *ctx = &gic_data.gicr_ctx[cpu];
+	void __iomem *base = per_cpu_ptr(gic_data.rdists.rdist, cpu)->rd_base;
+	void __iomem *sgi_base = base + GICR_SGI_BASE_OFFSET;
+
+	writel_relaxed(ctx->ctlr & ~(GICR_CTLR_ENABLE_LPIS), base + GICR_CTLR);
+	gic_do_wait_for_rwp(base);
+	writeq_relaxed(ctx->propbaser, base + GICR_PROPBASER);
+	writeq_relaxed(ctx->pendbaser, base + GICR_PENDBASER);
+
+	writel_relaxed(ctx->igroupr0, sgi_base + GICR_IGROUPR0);
+	writel_relaxed(ctx->isenabler0, sgi_base + GICR_ISENABLER0);
+	writel_relaxed(ctx->ispendr0, sgi_base + GICR_ISPENDR0);
+	writel_relaxed(ctx->isactiver0, sgi_base + GICR_ISACTIVER0);
+	__iowrite32_copy(sgi_base + GICR_IPRIORITYR0, ctx->ipriorityr, 8);
+	writel_relaxed(ctx->icfgr0, sgi_base + GICR_ICFGR0);
+	writel_relaxed(ctx->icfgr1, sgi_base + GICR_ICFGR0 + sizeof(u32));
+	writel_relaxed(ctx->igrpmodr0, sgi_base + GICR_IGRPMODR0);
+	writel_relaxed(ctx->nsacr, sgi_base + GICR_NSACR);
+	writel_relaxed(ctx->ctlr, sgi_base + GICR_CTLR);
+	gic_do_wait_for_rwp(base);
+}
+
+static void gic_dist_restore(void)
+{
+	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
+	void __iomem *base = gic_data.dist_base;
+	int irq_nr = gic_data.irq_nr;
+
+	gic_dist_wait_for_rwp();
+	__iowrite64_copy(base + GICD_IROUTER, ctx->irouter,
+			 GICD_NR_REGS(IROUTER, irq_nr));
+	__iowrite32_copy(base + GICD_IGROUPR, ctx->igroupr,
+			 GICD_NR_REGS(IGROUPR, irq_nr));
+	__iowrite32_copy(base + GICD_ISENABLER, ctx->isenabler,
+			 GICD_NR_REGS(ISENABLER, irq_nr));
+	__iowrite32_copy(base + GICD_ISPENDR, ctx->ispendr,
+			 GICD_NR_REGS(ISPENDR, irq_nr));
+	__iowrite32_copy(base + GICD_ISACTIVER, ctx->isactiver,
+			 GICD_NR_REGS(ISACTIVER, irq_nr));
+	__iowrite32_copy(base + GICD_ICFGR, ctx->icfgr,
+			 GICD_NR_REGS(ICFGR, irq_nr));
+	__iowrite32_copy(base + GICD_IGRPMODR, ctx->igrpmodr,
+			 GICD_NR_REGS(IGRPMODR, irq_nr));
+	__iowrite32_copy(base + GICD_NSACR, ctx->nsacr,
+			 GICD_NR_REGS(NSACR, irq_nr));
+	writel_relaxed(ctx->ctlr, base + GICD_CTLR);
+	gic_dist_wait_for_rwp();
+}
+
+static void gic_resume(void)
+{
+	int cpu;
+
+	if (!gic_data.gicd_ctx || !gic_data.gicr_ctx)
+		return;
+
+	gic_dist_restore();
+	its_restore_enable();
+	for_each_possible_cpu(cpu)
+		gic_rdist_restore(cpu);
+
+	gic_enable_redist(true);
+}
+
 #ifdef CONFIG_CPU_PM
 /* Check whether it's single security state view */
 static bool gic_dist_security_disabled(void)
@@ -767,13 +953,24 @@  static bool gic_dist_security_disabled(void)
 static int gic_cpu_pm_notifier(struct notifier_block *self,
 			       unsigned long cmd, void *v)
 {
-	if (cmd == CPU_PM_EXIT) {
+	switch (cmd) {
+	case CPU_PM_EXIT:
 		if (gic_dist_security_disabled())
 			gic_enable_redist(true);
 		gic_cpu_sys_reg_init();
-	} else if (cmd == CPU_PM_ENTER && gic_dist_security_disabled()) {
-		gic_write_grpen1(0);
-		gic_enable_redist(false);
+		break;
+	case CPU_PM_ENTER:
+		if (gic_dist_security_disabled()) {
+			gic_write_grpen1(0);
+			gic_enable_redist(false);
+		}
+		break;
+	case CPU_SYSTEM_PM_EXIT:
+		gic_resume();
+		break;
+	case CPU_SYSTEM_PM_ENTER:
+		gic_suspend();
+		break;
 	}
 	return NOTIFY_OK;
 }
@@ -991,11 +1188,99 @@  static const struct irq_domain_ops partition_domain_ops = {
 	.select = gic_irq_domain_select,
 };
 
+static int gic_populate_ctx(struct gic_dist_ctx *ctx, int irqs)
+{
+	int err;
+
+	ctx->irouter = kcalloc(GICD_NR_REGS(IROUTER, irqs),
+			       sizeof(*ctx->irouter), GFP_KERNEL);
+	if (IS_ERR(ctx->irouter))
+		return PTR_ERR(ctx->irouter);
+
+	ctx->igroupr = kcalloc(GICD_NR_REGS(IGROUPR, irqs),
+			       sizeof(*ctx->igroupr), GFP_KERNEL);
+	if (IS_ERR(ctx->igroupr)) {
+		err = PTR_ERR(ctx->igroupr);
+		goto out_irouter;
+	}
+
+	ctx->isenabler = kcalloc(GICD_NR_REGS(ISENABLER, irqs),
+				 sizeof(*ctx->isenabler), GFP_KERNEL);
+	if (IS_ERR(ctx->isenabler)) {
+		err = PTR_ERR(ctx->isenabler);
+		goto out_igroupr;
+	}
+
+	ctx->ispendr = kcalloc(GICD_NR_REGS(ISPENDR, irqs),
+			       sizeof(*ctx->ispendr), GFP_KERNEL);
+	if (IS_ERR(ctx->ispendr)) {
+		err = PTR_ERR(ctx->ispendr);
+		goto out_isenabler;
+	}
+
+	ctx->isactiver = kcalloc(GICD_NR_REGS(ISACTIVER, irqs),
+				 sizeof(*ctx->isactiver), GFP_KERNEL);
+	if (IS_ERR(ctx->isactiver)) {
+		err = PTR_ERR(ctx->isactiver);
+		goto out_ispendr;
+	}
+
+	ctx->ipriorityr = kcalloc(GICD_NR_REGS(IPRIORITYR, irqs),
+				  sizeof(*ctx->ipriorityr), GFP_KERNEL);
+	if (IS_ERR(ctx->ipriorityr)) {
+		err = PTR_ERR(ctx->ipriorityr);
+		goto out_isactiver;
+	}
+
+	ctx->icfgr = kcalloc(GICD_NR_REGS(ICFGR, irqs), sizeof(*ctx->icfgr),
+			     GFP_KERNEL);
+	if (IS_ERR(ctx->icfgr)) {
+		err = PTR_ERR(ctx->icfgr);
+		goto out_ipriorityr;
+	}
+
+	ctx->igrpmodr = kcalloc(GICD_NR_REGS(IGRPMODR, irqs),
+				sizeof(*ctx->igrpmodr), GFP_KERNEL);
+	if (IS_ERR(ctx->igrpmodr)) {
+		err = PTR_ERR(ctx->igrpmodr);
+		goto out_icfgr;
+	}
+
+	ctx->nsacr = kcalloc(GICD_NR_REGS(NSACR, irqs), sizeof(*ctx->nsacr),
+			     GFP_KERNEL);
+	if (IS_ERR(ctx->nsacr)) {
+		err = PTR_ERR(ctx->nsacr);
+		goto out_igrpmodr;
+	}
+
+	return 0;
+
+out_irouter:
+	kfree(ctx->irouter);
+out_igroupr:
+	kfree(ctx->igroupr);
+out_isenabler:
+	kfree(ctx->isenabler);
+out_ispendr:
+	kfree(ctx->ispendr);
+out_isactiver:
+	kfree(ctx->isactiver);
+out_ipriorityr:
+	kfree(ctx->ipriorityr);
+out_icfgr:
+	kfree(ctx->icfgr);
+out_igrpmodr:
+	kfree(ctx->igrpmodr);
+	return err;
+}
+
 static int __init gic_init_bases(void __iomem *dist_base,
 				 struct redist_region *rdist_regs,
 				 u32 nr_redist_regions,
 				 u64 redist_stride,
-				 struct fwnode_handle *handle)
+				 struct fwnode_handle *handle,
+				 struct gic_dist_ctx *gicd_ctx,
+				 struct gic_redist_ctx *gicr_ctx)
 {
 	u32 typer;
 	int gic_irqs;
@@ -1012,6 +1297,8 @@  static int __init gic_init_bases(void __iomem *dist_base,
 	gic_data.redist_regions = rdist_regs;
 	gic_data.nr_redist_regions = nr_redist_regions;
 	gic_data.redist_stride = redist_stride;
+	gic_data.gicd_ctx = gicd_ctx;
+	gic_data.gicr_ctx = gicr_ctx;
 
 	/*
 	 * Find out how many interrupts are supported.
@@ -1035,6 +1322,12 @@  static int __init gic_init_bases(void __iomem *dist_base,
 		goto out_free;
 	}
 
+	if (gicd_ctx) {
+		err = gic_populate_ctx(gicd_ctx, gic_irqs);
+		if (err)
+			goto out_free;
+	}
+
 	gic_data.has_rss = !!(typer & GICD_TYPER_RSS);
 	pr_info("Distributor has %sRange Selector support\n",
 		gic_data.has_rss ? "" : "no ");
@@ -1190,6 +1483,8 @@  static int __init gic_of_init(struct device_node *node, struct device_node *pare
 {
 	void __iomem *dist_base;
 	struct redist_region *rdist_regs;
+	struct gic_dist_ctx *gicd_ctx = NULL;
+	struct gic_redist_ctx *gicr_ctx = NULL;
 	u64 redist_stride;
 	u32 nr_redist_regions;
 	int err, i;
@@ -1232,17 +1527,35 @@  static int __init gic_of_init(struct device_node *node, struct device_node *pare
 	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
 		redist_stride = 0;
 
+	if (of_property_read_bool(node, "save-suspend-state")) {
+		gicd_ctx = kzalloc(sizeof(*gicd_ctx), GFP_KERNEL);
+		if (IS_ERR(gicd_ctx)) {
+			err = PTR_ERR(gicd_ctx);
+			goto out_unmap_rdist;
+		}
+
+		gicr_ctx = kcalloc(num_possible_cpus(), sizeof(*gicr_ctx),
+				   GFP_KERNEL);
+		if (IS_ERR(gicr_ctx)) {
+			err = PTR_ERR(gicr_ctx);
+			goto out_free_gicd_ctx;
+		}
+	}
+
 	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
-			     redist_stride, &node->fwnode);
+			     redist_stride, &node->fwnode, gicd_ctx, gicr_ctx);
 	if (err)
-		goto out_unmap_rdist;
+		goto out_free_gicr_ctx;
 
 	gic_populate_ppi_partitions(node);
 
 	if (static_key_true(&supports_deactivate))
 		gic_of_setup_kvm_info(node);
 	return 0;
-
+out_free_gicr_ctx:
+	kfree(gicr_ctx);
+out_free_gicd_ctx:
+	kfree(gicd_ctx);
 out_unmap_rdist:
 	for (i = 0; i < nr_redist_regions; i++)
 		if (rdist_regs[i].redist_base)
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index c00c4c33e432..f086987e3cb4 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -46,6 +46,19 @@ 
 #define GICD_IDREGS			0xFFD0
 #define GICD_PIDR2			0xFFE8
 
+#define GICD_IGROUPR_SHIFT		5
+#define GICD_ISENABLER_SHIFT		5
+#define GICD_ICENABLER_SHIFT		GICD_ISENABLER_SHIFT
+#define GICD_ISPENDR_SHIFT		5
+#define GICD_ICPENDR_SHIFT		GICD_ISPENDR_SHIFT
+#define GICD_ISACTIVER_SHIFT		5
+#define GICD_ICACTIVER_SHIFT		GICD_ISACTIVER_SHIFT
+#define GICD_IPRIORITYR_SHIFT		2
+#define GICD_ICFGR_SHIFT		4
+#define GICD_IGRPMODR_SHIFT		5
+#define GICD_NSACR_SHIFT		4
+#define GICD_IROUTER_SHIFT		0
+
 /*
  * Those registers are actually from GICv2, but the spec demands that they
  * are implemented as RES0 if ARE is 1 (which we do in KVM's emulated GICv3).
@@ -188,6 +201,8 @@ 
 
 #define GICR_PENDBASER_PTZ				BIT_ULL(62)
 
+#define GICR_SGI_BASE_OFFSET		(1U << 16)
+
 /*
  * Re-Distributor registers, offsets from SGI_base
  */
@@ -581,6 +596,8 @@  struct rdists {
 
 struct irq_domain;
 struct fwnode_handle;
+void its_save_disable(void);
+void its_restore_enable(void);
 int its_cpu_init(void);
 int its_init(struct fwnode_handle *handle, struct rdists *rdists,
 	     struct irq_domain *domain);