[PATCH 0/8] GICv3 Save and Restore

[PATCH 0/8] GICv3 Save and Restore

[Derek Basehore]

This patch set adds saving and restoring the GIC on v3
implementations through a devicetree flag.

The new CPU_PM state was the cleanest way to synchronize the save and
restore (that I figured out) with other PM operations since this isn't
going into any PSCI implementations (at least not in
ARM-Trusted-Firmware).

I've verified these patches on a Rockchip RK3399 platform (some
devicetree changes needed). This included turning off the power rail
for the GIC during suspend.

Derek Basehore (8):
  cpu_pm: add syscore_suspend error handling
  lib/iomap_copy: add __ioread64_copy()
  cpu_pm: Add SYSTEM_PM state
  irqchip/gic-v3: add ability to save/restore GIC/ITS state
  DT/arm,gic-v3: add save-suspend-state property
  irqchip/gic-v3-its: add ability to resend MAPC on resume
  DT/arm,gic-v3: add resend-mapc-on-resume property
  irqchip/gic-v3: add power down/up sequence

 .../bindings/interrupt-controller/arm,gic-v3.txt   |  43 +++
 drivers/irqchip/irq-gic-v3-its.c                   | 136 +++++--
 drivers/irqchip/irq-gic-v3.c                       | 391 ++++++++++++++++++++-
 include/linux/cpu_pm.h                             |  14 +
 include/linux/io.h                                 |   1 +
 include/linux/irqchip/arm-gic-v3.h                 |  26 ++
 kernel/cpu_pm.c                                    |  75 ++--
 lib/iomap_copy.c                                   |  26 ++
 8 files changed, 640 insertions(+), 72 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 1/8] cpu_pm: add syscore_suspend error handling

[Derek Basehore]

If cpu_cluster_pm_enter() fails, cpu_pm_exit() should be called. This
will put the CPU in the correct state to resume from the failure.

Change-Id: I4e499d686ea6046103be355d3a92bb0d51525f53
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 kernel/cpu_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 67b02e138a47..03bcc0751a51 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -186,6 +186,9 @@ static int cpu_pm_suspend(void)
 		return ret;
 
 	ret = cpu_cluster_pm_enter();
+	if (ret)
+		cpu_pm_exit();
+
 	return ret;
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 1/8] cpu_pm: add syscore_suspend error handling

[Brian Norris]

On Fri, Jan 12, 2018 at 01:24:15PM -0800, Derek Basehore wrote:
> If cpu_cluster_pm_enter() fails, cpu_pm_exit() should be called. This
> will put the CPU in the correct state to resume from the failure.
> 
> Change-Id: I4e499d686ea6046103be355d3a92bb0d51525f53

If you ran checkpatch.pl, it would warn you not to include Gerrit IDs
here :)

Brian

> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  kernel/cpu_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index 67b02e138a47..03bcc0751a51 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -186,6 +186,9 @@ static int cpu_pm_suspend(void)
>  		return ret;
>  
>  	ret = cpu_cluster_pm_enter();
> +	if (ret)
> +		cpu_pm_exit();
> +
>  	return ret;
>  }
>  
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

[PATCH 2/8] lib/iomap_copy: add __ioread64_copy()

[Derek Basehore]

Some drivers will want to read IO memory 64 bits at a time. Add an API
for this.

The 64-bit vs. 32-bit behavior is the same as the existing
__iowrite64_copy(), where we don't expect to need to (or be able to)
write in 64-bit chunks, so we fall back to a 32-bit equivalent.

Change-Id: Ia681f60bb98a0c6107052bccbeb5032861a93a0b
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 include/linux/io.h |  1 +
 lib/iomap_copy.c   | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/io.h b/include/linux/io.h
index 32e30e8fb9db..53c34a0d65f7 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -31,6 +31,7 @@ struct resource;
 __visible void __iowrite32_copy(void __iomem *to, const void *from, size_t count);
 void __ioread32_copy(void *to, const void __iomem *from, size_t count);
 void __iowrite64_copy(void __iomem *to, const void *from, size_t count);
+void __ioread64_copy(void *to, const void __iomem *from, size_t count);
 
 #ifdef CONFIG_MMU
 int ioremap_page_range(unsigned long addr, unsigned long end,
diff --git a/lib/iomap_copy.c b/lib/iomap_copy.c
index b8f1d6cbb200..71c137ede5fb 100644
--- a/lib/iomap_copy.c
+++ b/lib/iomap_copy.c
@@ -89,3 +89,29 @@ void __attribute__((weak)) __iowrite64_copy(void __iomem *to,
 }
 
 EXPORT_SYMBOL_GPL(__iowrite64_copy);
+
+/**
+ * __ioread64_copy - copy data from MMIO space, in 64-bit units
+ * @to: destination (must be 64-bit aligned)
+ * @from: source, in MMIO space (must be 64-bit aligned)
+ * @count: number of 64-bit quantities to copy
+ *
+ * Copy data from MMIO space to kernel space, in units of 64 bits at a time if
+ * on a 64-bit system.  32-bit systems will fall back to 32 bits at a time.
+ * Order of access is not guaranteed, nor is a memory barrier performed
+ * afterwards.
+ */
+void __ioread64_copy(void *to, const void __iomem *from, size_t count)
+{
+#ifdef CONFIG_64BIT
+	u64 *dst = to;
+	const u64 __iomem *src = from;
+	const u64 __iomem *end = src + count;
+
+	while (src < end)
+		*dst++ = __raw_readq(src++);
+#else
+	__ioread32_copy(to, from, count * 2);
+#endif
+}
+EXPORT_SYMBOL_GPL(__ioread64_copy);
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 3/8] cpu_pm: Add SYSTEM_PM state

[Derek Basehore]

This adds a new CPU PM state that is only called on system suspend.
Saving the GIC state needs to happen after the CPU_PM and
CPU_CLUSTER_PM transitions on suspend. The corresponding restore needs
to happen before these same transitions on resume. The CPU_CLUSTER_PM
transition is not valid since that can be called with normal idle
operations when powering down the GIC is not desired. Another set of
syscore ops would have to rely on the correct ordering of syscore ops,
so avoid that by adding a new CPU_PM state.

Change-Id: I484bafbad8e810540d54b7c73ef03e4adf7b7c9a
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 include/linux/cpu_pm.h | 14 ++++++++++
 kernel/cpu_pm.c        | 74 +++++++++++++++++++++++++++++++++-----------------
 2 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/include/linux/cpu_pm.h b/include/linux/cpu_pm.h
index 455b233dd3b1..28a462120cc5 100644
--- a/include/linux/cpu_pm.h
+++ b/include/linux/cpu_pm.h
@@ -41,6 +41,11 @@
  * are used to save any global context for affected blocks, and must be called
  * after all the CPUs in the power domain have been notified of the low power
  * state.
+ *
+ * CPU system notifications apply to all CPUs within the system. They are used
+ * for operations that must only happen on suspend and have an ordering
+ * requirement with the other CPU PM notifications. Must be called after all
+ * CPUs and clusters in the system have been notified of the low power state.
  */
 
 /*
@@ -64,6 +69,15 @@ enum cpu_pm_event {
 
 	/* A cpu power domain is exiting a low power state */
 	CPU_CLUSTER_PM_EXIT,
+
+	/* A cpu power domain is entering a system low power state (suspend) */
+	CPU_SYSTEM_PM_ENTER,
+
+	/* A cpu power domain failed to enter a system low power state */
+	CPU_SYSTEM_PM_ENTER_FAILED,
+
+	/* A cpu power domain is exiting a system low power state */
+	CPU_SYSTEM_PM_EXIT,
 };
 
 #ifdef CONFIG_CPU_PM
diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 03bcc0751a51..833357959bc5 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -72,6 +72,22 @@ int cpu_pm_unregister_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(cpu_pm_unregister_notifier);
 
+static int cpu_pm_notify_enter(enum cpu_pm_event enter, enum cpu_pm_event fail)
+{
+	int nr_calls;
+	int ret = 0;
+
+	ret = cpu_pm_notify(enter, -1, &nr_calls);
+	if (ret)
+		/*
+		 * Inform listeners (nr_calls - 1) about failure to enter CPU PM
+		 * state <enter> who were notified earlier to prepare for it.
+		 */
+		cpu_pm_notify(fail, nr_calls - 1, NULL);
+
+	return ret;
+}
+
 /**
  * cpu_pm_enter - CPU low power entry notifier
  *
@@ -89,18 +105,7 @@ EXPORT_SYMBOL_GPL(cpu_pm_unregister_notifier);
  */
 int cpu_pm_enter(void)
 {
-	int nr_calls;
-	int ret = 0;
-
-	ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
-	if (ret)
-		/*
-		 * Inform listeners (nr_calls - 1) about failure of CPU PM
-		 * PM entry who are notified earlier to prepare for it.
-		 */
-		cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
-
-	return ret;
+	return cpu_pm_notify_enter(CPU_PM_ENTER, CPU_PM_ENTER_FAILED);
 }
 EXPORT_SYMBOL_GPL(cpu_pm_enter);
 
@@ -140,18 +145,8 @@ EXPORT_SYMBOL_GPL(cpu_pm_exit);
  */
 int cpu_cluster_pm_enter(void)
 {
-	int nr_calls;
-	int ret = 0;
-
-	ret = cpu_pm_notify(CPU_CLUSTER_PM_ENTER, -1, &nr_calls);
-	if (ret)
-		/*
-		 * Inform listeners (nr_calls - 1) about failure of CPU cluster
-		 * PM entry who are notified earlier to prepare for it.
-		 */
-		cpu_pm_notify(CPU_CLUSTER_PM_ENTER_FAILED, nr_calls - 1, NULL);
-
-	return ret;
+	return cpu_pm_notify_enter(CPU_CLUSTER_PM_ENTER,
+		CPU_CLUSTER_PM_ENTER_FAILED);
 }
 EXPORT_SYMBOL_GPL(cpu_cluster_pm_enter);
 
@@ -176,6 +171,27 @@ int cpu_cluster_pm_exit(void)
 }
 EXPORT_SYMBOL_GPL(cpu_cluster_pm_exit);
 
+/*
+ * Notifies listeners that the system is entering suspend. Called after
+ * cpu_cluster_pm_enter() and only within the syscore suspend callback,
+ * cpu_pm_suspend.
+ */
+static int cpu_system_pm_enter(void)
+{
+	return cpu_pm_notify_enter(CPU_SYSTEM_PM_ENTER,
+		CPU_SYSTEM_PM_ENTER_FAILED);
+}
+
+/*
+ * Notifies listeners that the system is leaving suspend. Called before
+ * cpu_cluster_pm_exit() and only within the syscore resume callback,
+ * cpu_pm_resume.
+ */
+static int cpu_system_pm_exit(void)
+{
+	return cpu_pm_notify(CPU_SYSTEM_PM_EXIT, -1, NULL);
+}
+
 #ifdef CONFIG_PM
 static int cpu_pm_suspend(void)
 {
@@ -186,14 +202,22 @@ static int cpu_pm_suspend(void)
 		return ret;
 
 	ret = cpu_cluster_pm_enter();
-	if (ret)
+	if (ret) {
 		cpu_pm_exit();
+		return ret;
+	}
 
+	ret = cpu_system_pm_enter();
+	if (ret) {
+		cpu_cluster_pm_exit();
+		cpu_pm_exit();
+	}
 	return ret;
 }
 
 static void cpu_pm_resume(void)
 {
+	cpu_system_pm_exit();
 	cpu_cluster_pm_exit();
 	cpu_pm_exit();
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

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

[Derek Basehore]

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(-)

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);
-- 
2.16.0.rc1.238.g530d649a79-goog

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

[Marc Zyngier]

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

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

[Brian Norris]

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

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

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

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

[dbasehore .]

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

[PATCH 5/8] DT/arm,gic-v3: add save-suspend-state property

[Derek Basehore]

This adds documentation for the new save-suspend-state property. This
property enables saving and restoring the GIC for when it loses state
in system suspend.

Change-Id: I9ad34baed04475c6ef1562bc0f72ad5d348f9904
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 .../bindings/interrupt-controller/arm,gic-v3.txt   | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index 0a57f2f4167d..820556a8ffd2 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -57,6 +57,10 @@ Optional
   occupied by the redistributors. Required if more than one such
   region is present.
 
+- save-suspend-state : Bool property. Setting this has the kernel save
+  and restore the GIC state for suspend and resume respectively. For
+  when the GIC loses power during suspend.
+
 Sub-nodes:
 
 PPI affinity can be expressed as a single "ppi-partitions" node,
@@ -107,6 +111,38 @@ Examples:
 		};
 	};
 
+	gic: interrupt-controller@fee00000 {
+		compatible = "arm,gic-v3";
+		#interrupt-cells = <4>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		interrupt-controller;
+		save-suspend-state;
+
+		reg = <0x0 0xfee00000 0 0x10000>, /* GICD */
+		      <0x0 0xfef00000 0 0xc0000>, /* GICR */
+		      <0x0 0xfff00000 0 0x10000>, /* GICC */
+		      <0x0 0xfff10000 0 0x10000>, /* GICH */
+		      <0x0 0xfff20000 0 0x10000>; /* GICV */
+		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH 0>;
+		its: interrupt-controller@fee20000 {
+			compatible = "arm,gic-v3-its";
+			msi-controller;
+			reg = <0x0 0xfee20000 0x0 0x20000>;
+		};
+
+		ppi-partitions {
+			ppi_cluster0: interrupt-partition-0 {
+				affinity = <&cpu_l0 &cpu_l1 &cpu_l2 &cpu_l3>;
+			};
+
+			ppi_cluster1: interrupt-partition-1 {
+				affinity = <&cpu_b0 &cpu_b1>;
+			};
+		};
+	};
+
 	gic: interrupt-controller@2c010000 {
 		compatible = "arm,gic-v3";
 		#interrupt-cells = <4>;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 5/8] DT/arm,gic-v3: add save-suspend-state property

[Marc Zyngier]

On Fri, 12 Jan 2018 21:24:19 +0000,
Derek Basehore wrote:
> 
> This adds documentation for the new save-suspend-state property. This
> property enables saving and restoring the GIC for when it loses state
> in system suspend.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>

Please CC the DT maintainers for all DT binding changes.

> ---
>  .../bindings/interrupt-controller/arm,gic-v3.txt   | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> index 0a57f2f4167d..820556a8ffd2 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> @@ -57,6 +57,10 @@ Optional
>    occupied by the redistributors. Required if more than one such
>    region is present.
>  
> +- save-suspend-state : Bool property. Setting this has the kernel save
> +  and restore the GIC state for suspend and resume respectively. For
> +  when the GIC loses power during suspend.

The name is pretty backward. You're supposed to describe a property of
the system, not instruct what SW should do.

What about something like "powered-off-on-suspend"?

Also, does this apply to the GIC? The ITS? The whole thing? Does it
make sense to treat them independently? (hint: probably).

> +
>  Sub-nodes:
>  
>  PPI affinity can be expressed as a single "ppi-partitions" node,
> @@ -107,6 +111,38 @@ Examples:
>  		};
>  	};
>  
> +	gic: interrupt-controller@fee00000 {
> +		compatible = "arm,gic-v3";
> +		#interrupt-cells = <4>;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		interrupt-controller;
> +		save-suspend-state;
> +
> +		reg = <0x0 0xfee00000 0 0x10000>, /* GICD */
> +		      <0x0 0xfef00000 0 0xc0000>, /* GICR */
> +		      <0x0 0xfff00000 0 0x10000>, /* GICC */
> +		      <0x0 0xfff10000 0 0x10000>, /* GICH */
> +		      <0x0 0xfff20000 0 0x10000>; /* GICV */
> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH 0>;
> +		its: interrupt-controller@fee20000 {
> +			compatible = "arm,gic-v3-its";
> +			msi-controller;
> +			reg = <0x0 0xfee20000 0x0 0x20000>;
> +		};
> +
> +		ppi-partitions {
> +			ppi_cluster0: interrupt-partition-0 {
> +				affinity = <&cpu_l0 &cpu_l1 &cpu_l2 &cpu_l3>;
> +			};
> +
> +			ppi_cluster1: interrupt-partition-1 {
> +				affinity = <&cpu_b0 &cpu_b1>;
> +			};
> +		};
> +	};
> +

I don't think we need another full example of the binding.

>  	gic: interrupt-controller@2c010000 {
>  		compatible = "arm,gic-v3";
>  		#interrupt-cells = <4>;
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

Thanks,

	M.

[PATCH 6/8] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Derek Basehore]

This adds a DT-binding to resend the MAPC command to an ITS node on
resume. If the ITS is powered down during suspend and the collections
are not backed by memory, the ITS will lose that state. This just sets
up the known state for the collections after the ITS is restored.

Change-Id: I4fe7f3f16500e1d3b14368262f03a43509c0049b
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 85 ++++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 38 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 5cb808e3d0bf..6d2688a2ee67 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -46,6 +46,7 @@
 #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
+#define ITS_FLAGS_RESEND_MAPC_ON_RESUME		(1ULL << 3)
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
 
@@ -1949,52 +1950,53 @@ static void its_cpu_init_lpis(void)
 	dsb(sy);
 }
 
-static void its_cpu_init_collection(void)
+static void its_cpu_init_collection(struct its_node *its)
 {
-	struct its_node *its;
-	int cpu;
-
-	spin_lock(&its_lock);
-	cpu = smp_processor_id();
-
-	list_for_each_entry(its, &its_nodes, entry) {
-		u64 target;
+	int cpu = smp_processor_id();
+	u64 target;
 
-		/* avoid cross node collections and its mapping */
-		if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
-			struct device_node *cpu_node;
+	/* avoid cross node collections and its mapping */
+	if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
+		struct device_node *cpu_node;
 
-			cpu_node = of_get_cpu_node(cpu, NULL);
-			if (its->numa_node != NUMA_NO_NODE &&
-				its->numa_node != of_node_to_nid(cpu_node))
-				continue;
-		}
+		cpu_node = of_get_cpu_node(cpu, NULL);
+		if (its->numa_node != NUMA_NO_NODE &&
+			its->numa_node != of_node_to_nid(cpu_node))
+			return;
+	}
 
+	/*
+	 * We now have to bind each collection to its target
+	 * redistributor.
+	 */
+	if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
 		/*
-		 * We now have to bind each collection to its target
+		 * This ITS wants the physical address of the
 		 * redistributor.
 		 */
-		if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
-			/*
-			 * This ITS wants the physical address of the
-			 * redistributor.
-			 */
-			target = gic_data_rdist()->phys_base;
-		} else {
-			/*
-			 * This ITS wants a linear CPU number.
-			 */
-			target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
-			target = GICR_TYPER_CPU_NUMBER(target) << 16;
-		}
+		target = gic_data_rdist()->phys_base;
+	} else {
+		/* This ITS wants a linear CPU number. */
+		target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
+		target = GICR_TYPER_CPU_NUMBER(target) << 16;
+	}
 
-		/* Perform collection mapping */
-		its->collections[cpu].target_address = target;
-		its->collections[cpu].col_id = cpu;
+	/* Perform collection mapping */
+	its->collections[cpu].target_address = target;
+	its->collections[cpu].col_id = cpu;
 
-		its_send_mapc(its, &its->collections[cpu], 1);
-		its_send_invall(its, &its->collections[cpu]);
-	}
+	its_send_mapc(its, &its->collections[cpu], 1);
+	its_send_invall(its, &its->collections[cpu]);
+}
+
+static void its_cpu_init_collections(void)
+{
+	struct its_node *its;
+
+	spin_lock(&its_lock);
+
+	list_for_each_entry(its, &its_nodes, entry)
+		its_cpu_init_collection(its);
 
 	spin_unlock(&its_lock);
 }
@@ -3089,6 +3091,9 @@ void its_restore_enable(void)
 			its_write_baser(its, &its->tables[i],
 					its->tables[i].val);
 		writel_relaxed(ctx->ctlr, base + GITS_CTLR);
+
+		if (its->flags & ITS_FLAGS_RESEND_MAPC_ON_RESUME)
+			its_cpu_init_collection(its);
 	}
 	spin_unlock(&its_lock);
 }
@@ -3312,6 +3317,10 @@ static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
+	if (fwnode_property_present(its->fwnode_handle,
+				    "resend-mapc-on-resume"))
+		its->flags |= ITS_FLAGS_RESEND_MAPC_ON_RESUME;
+
 	err = its_init_domain(handle, its);
 	if (err)
 		goto out_free_tables;
@@ -3347,7 +3356,7 @@ int its_cpu_init(void)
 			return -ENXIO;
 		}
 		its_cpu_init_lpis();
-		its_cpu_init_collection();
+		its_cpu_init_collections();
 	}
 
 	return 0;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 6/8] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Marc Zyngier]

On Fri, 12 Jan 2018 21:24:20 +0000,
Derek Basehore wrote:
> 
> This adds a DT-binding to resend the MAPC command to an ITS node on

This isn't a DT binding. That's the driver implementation. The binding
is what you put in Documentation/device-tree...

> resume. If the ITS is powered down during suspend and the collections
> are not backed by memory, the ITS will lose that state. This just sets
> up the known state for the collections after the ITS is restored.
> 
> Change-Id: I4fe7f3f16500e1d3b14368262f03a43509c0049b
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 85 ++++++++++++++++++++++------------------
>  1 file changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 5cb808e3d0bf..6d2688a2ee67 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -46,6 +46,7 @@
>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
> +#define ITS_FLAGS_RESEND_MAPC_ON_RESUME		(1ULL << 3)
>  
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  
> @@ -1949,52 +1950,53 @@ static void its_cpu_init_lpis(void)
>  	dsb(sy);
>  }
>  
> -static void its_cpu_init_collection(void)
> +static void its_cpu_init_collection(struct its_node *its)
>  {
> -	struct its_node *its;
> -	int cpu;
> -
> -	spin_lock(&its_lock);
> -	cpu = smp_processor_id();
> -
> -	list_for_each_entry(its, &its_nodes, entry) {
> -		u64 target;
> +	int cpu = smp_processor_id();
> +	u64 target;
>  
> -		/* avoid cross node collections and its mapping */
> -		if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
> -			struct device_node *cpu_node;
> +	/* avoid cross node collections and its mapping */
> +	if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
> +		struct device_node *cpu_node;
>  
> -			cpu_node = of_get_cpu_node(cpu, NULL);
> -			if (its->numa_node != NUMA_NO_NODE &&
> -				its->numa_node != of_node_to_nid(cpu_node))
> -				continue;
> -		}
> +		cpu_node = of_get_cpu_node(cpu, NULL);
> +		if (its->numa_node != NUMA_NO_NODE &&
> +			its->numa_node != of_node_to_nid(cpu_node))
> +			return;
> +	}
>  
> +	/*
> +	 * We now have to bind each collection to its target
> +	 * redistributor.
> +	 */
> +	if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
>  		/*
> -		 * We now have to bind each collection to its target
> +		 * This ITS wants the physical address of the
>  		 * redistributor.
>  		 */
> -		if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
> -			/*
> -			 * This ITS wants the physical address of the
> -			 * redistributor.
> -			 */
> -			target = gic_data_rdist()->phys_base;
> -		} else {
> -			/*
> -			 * This ITS wants a linear CPU number.
> -			 */
> -			target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
> -			target = GICR_TYPER_CPU_NUMBER(target) << 16;
> -		}
> +		target = gic_data_rdist()->phys_base;
> +	} else {
> +		/* This ITS wants a linear CPU number. */
> +		target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
> +		target = GICR_TYPER_CPU_NUMBER(target) << 16;
> +	}
>  
> -		/* Perform collection mapping */
> -		its->collections[cpu].target_address = target;
> -		its->collections[cpu].col_id = cpu;
> +	/* Perform collection mapping */
> +	its->collections[cpu].target_address = target;
> +	its->collections[cpu].col_id = cpu;
>  
> -		its_send_mapc(its, &its->collections[cpu], 1);
> -		its_send_invall(its, &its->collections[cpu]);
> -	}
> +	its_send_mapc(its, &its->collections[cpu], 1);
> +	its_send_invall(its, &its->collections[cpu]);
> +}
> +
> +static void its_cpu_init_collections(void)
> +{
> +	struct its_node *its;
> +
> +	spin_lock(&its_lock);
> +
> +	list_for_each_entry(its, &its_nodes, entry)
> +		its_cpu_init_collection(its);
>  
>  	spin_unlock(&its_lock);
>  }
> @@ -3089,6 +3091,9 @@ void its_restore_enable(void)
>  			its_write_baser(its, &its->tables[i],
>  					its->tables[i].val);
>  		writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> +
> +		if (its->flags & ITS_FLAGS_RESEND_MAPC_ON_RESUME)
> +			its_cpu_init_collection(its);
>  	}
>  	spin_unlock(&its_lock);
>  }
> @@ -3312,6 +3317,10 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> +	if (fwnode_property_present(its->fwnode_handle,
> +				    "resend-mapc-on-resume"))
> +		its->flags |= ITS_FLAGS_RESEND_MAPC_ON_RESUME;

This function is firmware agnostic, and I really don't want to make
this thing a general purpose property.

Please set this as a quirk when you detect some particular combination
of HW and firmware (in your case, GIC500 + this property). And again,
the property name is totally backward. IT should be something like
"collection-state-lost-on-suspend".

> +
>  	err = its_init_domain(handle, its);
>  	if (err)
>  		goto out_free_tables;
> @@ -3347,7 +3356,7 @@ int its_cpu_init(void)
>  			return -ENXIO;
>  		}
>  		its_cpu_init_lpis();
> -		its_cpu_init_collection();
> +		its_cpu_init_collections();
>  	}
>  
>  	return 0;
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

Thanks,

	M.

Re: [PATCH 6/8] irqchip/gic-v3-its: add ability to resend MAPC on resume

[dbasehore .]

On Sun, Jan 14, 2018 at 2:56 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Fri, 12 Jan 2018 21:24:20 +0000,
> Derek Basehore wrote:
>>
>> This adds a DT-binding to resend the MAPC command to an ITS node on
>
> This isn't a DT binding. That's the driver implementation. The binding
> is what you put in Documentation/device-tree...
>
>> resume. If the ITS is powered down during suspend and the collections
>> are not backed by memory, the ITS will lose that state. This just sets
>> up the known state for the collections after the ITS is restored.
>>
>> Change-Id: I4fe7f3f16500e1d3b14368262f03a43509c0049b
>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 85 ++++++++++++++++++++++------------------
>>  1 file changed, 47 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 5cb808e3d0bf..6d2688a2ee67 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -46,6 +46,7 @@
>>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING                (1ULL << 0)
>>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375    (1ULL << 1)
>>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144    (1ULL << 2)
>> +#define ITS_FLAGS_RESEND_MAPC_ON_RESUME              (1ULL << 3)
>>
>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>
>> @@ -1949,52 +1950,53 @@ static void its_cpu_init_lpis(void)
>>       dsb(sy);
>>  }
>>
>> -static void its_cpu_init_collection(void)
>> +static void its_cpu_init_collection(struct its_node *its)
>>  {
>> -     struct its_node *its;
>> -     int cpu;
>> -
>> -     spin_lock(&its_lock);
>> -     cpu = smp_processor_id();
>> -
>> -     list_for_each_entry(its, &its_nodes, entry) {
>> -             u64 target;
>> +     int cpu = smp_processor_id();
>> +     u64 target;
>>
>> -             /* avoid cross node collections and its mapping */
>> -             if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
>> -                     struct device_node *cpu_node;
>> +     /* avoid cross node collections and its mapping */
>> +     if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
>> +             struct device_node *cpu_node;
>>
>> -                     cpu_node = of_get_cpu_node(cpu, NULL);
>> -                     if (its->numa_node != NUMA_NO_NODE &&
>> -                             its->numa_node != of_node_to_nid(cpu_node))
>> -                             continue;
>> -             }
>> +             cpu_node = of_get_cpu_node(cpu, NULL);
>> +             if (its->numa_node != NUMA_NO_NODE &&
>> +                     its->numa_node != of_node_to_nid(cpu_node))
>> +                     return;
>> +     }
>>
>> +     /*
>> +      * We now have to bind each collection to its target
>> +      * redistributor.
>> +      */
>> +     if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
>>               /*
>> -              * We now have to bind each collection to its target
>> +              * This ITS wants the physical address of the
>>                * redistributor.
>>                */
>> -             if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
>> -                     /*
>> -                      * This ITS wants the physical address of the
>> -                      * redistributor.
>> -                      */
>> -                     target = gic_data_rdist()->phys_base;
>> -             } else {
>> -                     /*
>> -                      * This ITS wants a linear CPU number.
>> -                      */
>> -                     target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
>> -                     target = GICR_TYPER_CPU_NUMBER(target) << 16;
>> -             }
>> +             target = gic_data_rdist()->phys_base;
>> +     } else {
>> +             /* This ITS wants a linear CPU number. */
>> +             target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
>> +             target = GICR_TYPER_CPU_NUMBER(target) << 16;
>> +     }
>>
>> -             /* Perform collection mapping */
>> -             its->collections[cpu].target_address = target;
>> -             its->collections[cpu].col_id = cpu;
>> +     /* Perform collection mapping */
>> +     its->collections[cpu].target_address = target;
>> +     its->collections[cpu].col_id = cpu;
>>
>> -             its_send_mapc(its, &its->collections[cpu], 1);
>> -             its_send_invall(its, &its->collections[cpu]);
>> -     }
>> +     its_send_mapc(its, &its->collections[cpu], 1);
>> +     its_send_invall(its, &its->collections[cpu]);
>> +}
>> +
>> +static void its_cpu_init_collections(void)
>> +{
>> +     struct its_node *its;
>> +
>> +     spin_lock(&its_lock);
>> +
>> +     list_for_each_entry(its, &its_nodes, entry)
>> +             its_cpu_init_collection(its);
>>
>>       spin_unlock(&its_lock);
>>  }
>> @@ -3089,6 +3091,9 @@ void its_restore_enable(void)
>>                       its_write_baser(its, &its->tables[i],
>>                                       its->tables[i].val);
>>               writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> +
>> +             if (its->flags & ITS_FLAGS_RESEND_MAPC_ON_RESUME)
>> +                     its_cpu_init_collection(its);
>>       }
>>       spin_unlock(&its_lock);
>>  }
>> @@ -3312,6 +3317,10 @@ static int __init its_probe_one(struct resource *res,
>>               ctlr |= GITS_CTLR_ImDe;
>>       writel_relaxed(ctlr, its->base + GITS_CTLR);
>>
>> +     if (fwnode_property_present(its->fwnode_handle,
>> +                                 "resend-mapc-on-resume"))
>> +             its->flags |= ITS_FLAGS_RESEND_MAPC_ON_RESUME;
>
> This function is firmware agnostic, and I really don't want to make
> this thing a general purpose property.
>
> Please set this as a quirk when you detect some particular combination
> of HW and firmware (in your case, GIC500 + this property). And again,
> the property name is totally backward. IT should be something like
> "collection-state-lost-on-suspend".

So name it ITS_FLAGS_WORKAROUND_GIC500_MAPC or should there be a quirk
number for it?

>
>> +
>>       err = its_init_domain(handle, its);
>>       if (err)
>>               goto out_free_tables;
>> @@ -3347,7 +3356,7 @@ int its_cpu_init(void)
>>                       return -ENXIO;
>>               }
>>               its_cpu_init_lpis();
>> -             its_cpu_init_collection();
>> +             its_cpu_init_collections();
>>       }
>>
>>       return 0;
>> --
>> 2.16.0.rc1.238.g530d649a79-goog
>>
>
> Thanks,
>
>         M.

[PATCH 7/8] DT/arm,gic-v3: add resend-mapc-on-resume property

[Derek Basehore]

This boolean property for the GIC-V3-ITS enables resending the MAP
COLLECTIONS commands when restoring the GIC on resume.

Change-Id: I03ab8689dace109851a4d2be3abf5fbafb090d27
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt        | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index 820556a8ffd2..1f0d691df9b3 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -82,6 +82,12 @@ These nodes must have the following properties:
 Optional:
 - socionext,synquacer-pre-its: (u32, u32) tuple describing the untranslated
   address and size of the pre-ITS window.
+- resend-mapc-on-resume : Bool property. If the collections for the ITS
+  are stored in the ITS instead of external memory, the state will be
+  lost if the GIC loses power. Setting this enables the kernel to
+  reset the collections state on resume for this ITS node. The state
+  will only be reset if the save-suspend-state boolean is set for the
+  gic.
 
 The main GIC node must contain the appropriate #address-cells,
 #size-cells and ranges properties for the reg property of all ITS
@@ -129,6 +135,7 @@ Examples:
 		its: interrupt-controller@fee20000 {
 			compatible = "arm,gic-v3-its";
 			msi-controller;
+			resend-mapc-on-resume;
 			reg = <0x0 0xfee20000 0x0 0x20000>;
 		};
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 8/8] irqchip/gic-v3: add power down/up sequence

[Derek Basehore]

This adds the implementation specific power down/up sequence for the
GIC-500 and the GIC-600 (which are implementations of the GIC-v3
specification). This allows the LPI pending information to be properly
flushed on suspend if the GIC is disabled.

Change-Id: Iad2135b5f5a57f7dc0c15d05e4b9a06e1b4c24d1
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/irqchip/irq-gic-v3.c       | 58 ++++++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-v3.h |  9 ++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 95d37fb6f458..5286757dd413 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -816,6 +816,35 @@ static void gic_redist_save(int cpu)
 	ctx->nsacr = readl_relaxed(base + GICR_NSACR);
 }
 
+static void gic_power_down(void)
+{
+	void __iomem *base = gic_data.dist_base;
+	u32 product_id = readl_relaxed(base + GICD_IIDR) >>
+			 GICD_IIDR_PRODUCTID_SHIFT;
+
+	/*
+	 * This power down sequence is implementation defined. It's the same for
+	 * the GIC-500 and GIC-600.
+	 */
+	if ((product_id & GIC500_IIDR_PRODUCTID) ||
+	    (product_id & GIC600_IIDR_PRODUCTID)) {
+		u32 val;
+
+		/*
+		 * There's only one instance of the GICR_WAKER register which
+		 * each redistributor maps to. So this just needs to be set for
+		 * the current CPU.
+		 */
+		base = gic_data_rdist_rd_base();
+		val = readl_relaxed(base + GICR_WAKER);
+		writel_relaxed(val | GICR_WAKER_Sleep, base + GICR_WAKER);
+		while (!(readl_relaxed(base + GICR_WAKER) &
+			 GICR_WAKER_Quiescent))
+			;
+
+	}
+}
+
 static void gic_dist_save(void)
 {
 	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
@@ -871,6 +900,7 @@ static int gic_suspend(void)
 	for_each_possible_cpu(cpu)
 		gic_redist_save(cpu);
 
+	gic_power_down();
 	its_save_disable();
 	gic_dist_save();
 
@@ -901,6 +931,33 @@ static void gic_rdist_restore(int cpu)
 	gic_do_wait_for_rwp(base);
 }
 
+static void gic_power_up(void)
+{
+	void __iomem *base = gic_data.dist_base;
+	u32 product_id = readl_relaxed(base + GICD_IIDR) >>
+			 GICD_IIDR_PRODUCTID_SHIFT;
+
+	/*
+	 * Same as the power down sequence, this part of the power up sequence
+	 * is implementation defined.
+	 */
+	if ((product_id & GIC500_IIDR_PRODUCTID) ||
+	    (product_id & GIC600_IIDR_PRODUCTID)) {
+		u32 val;
+
+		/*
+		 * Need to turn the GIC back on in-case suspend is cancelled.
+		 * The GIC hardware reset state or the platform layer should
+		 * handle this otherwise.
+		 */
+		base = gic_data_rdist_rd_base();
+		val = readl_relaxed(base + GICR_WAKER);
+		writel_relaxed(val & ~GICR_WAKER_Sleep, base + GICR_WAKER);
+		while (readl_relaxed(base + GICR_WAKER) & GICR_WAKER_Quiescent)
+			;
+	}
+}
+
 static void gic_dist_restore(void)
 {
 	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
@@ -937,6 +994,7 @@ static void gic_resume(void)
 
 	gic_dist_restore();
 	its_restore_enable();
+	gic_power_up();
 	for_each_possible_cpu(cpu)
 		gic_rdist_restore(cpu);
 
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index f086987e3cb4..22ced72be1c5 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -59,6 +59,10 @@
 #define GICD_NSACR_SHIFT		4
 #define GICD_IROUTER_SHIFT		0
 
+#define GICD_IIDR_PRODUCTID_SHIFT	24
+#define GIC500_IIDR_PRODUCTID		0x00
+#define GIC600_IIDR_PRODUCTID		0x02
+
 /*
  * 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).
@@ -122,8 +126,13 @@
 
 #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)
 
+/*
+ * Sleep and Quiescent are implementation specific for the GIC-500 and GIC-600.
+ */
+#define GICR_WAKER_Sleep		(1U << 0)
 #define GICR_WAKER_ProcessorSleep	(1U << 1)
 #define GICR_WAKER_ChildrenAsleep	(1U << 2)
+#define GICR_WAKER_Quiescent		(1U << 31)
 
 #define GIC_BASER_CACHE_nCnB		0ULL
 #define GIC_BASER_CACHE_SameAsInner	0ULL
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 8/8] irqchip/gic-v3: add power down/up sequence

[Marc Zyngier]

On Fri, 12 Jan 2018 21:24:22 +0000,
Derek Basehore wrote:
> 
> This adds the implementation specific power down/up sequence for the
> GIC-500 and the GIC-600 (which are implementations of the GIC-v3
> specification). This allows the LPI pending information to be properly
> flushed on suspend if the GIC is disabled.

Please add references to the TRMs.

> 
> Change-Id: Iad2135b5f5a57f7dc0c15d05e4b9a06e1b4c24d1
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3.c       | 58 ++++++++++++++++++++++++++++++++++++++
>  include/linux/irqchip/arm-gic-v3.h |  9 ++++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 95d37fb6f458..5286757dd413 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -816,6 +816,35 @@ static void gic_redist_save(int cpu)
>  	ctx->nsacr = readl_relaxed(base + GICR_NSACR);
>  }
>  
> +static void gic_power_down(void)
> +{
> +	void __iomem *base = gic_data.dist_base;
> +	u32 product_id = readl_relaxed(base + GICD_IIDR) >>
> +			 GICD_IIDR_PRODUCTID_SHIFT;
> +
> +	/*
> +	 * This power down sequence is implementation defined. It's the same for
> +	 * the GIC-500 and GIC-600.
> +	 */
> +	if ((product_id & GIC500_IIDR_PRODUCTID) ||
> +	    (product_id & GIC600_IIDR_PRODUCTID)) {
> +		u32 val;
> +
> +		/*
> +		 * There's only one instance of the GICR_WAKER register which
> +		 * each redistributor maps to. So this just needs to be set for
> +		 * the current CPU.
> +		 */
> +		base = gic_data_rdist_rd_base();
> +		val = readl_relaxed(base + GICR_WAKER);
> +		writel_relaxed(val | GICR_WAKER_Sleep, base + GICR_WAKER);
> +		while (!(readl_relaxed(base + GICR_WAKER) &
> +			 GICR_WAKER_Quiescent))

GICR_WAKER is secure only when GICD_CTLR.DS=0. How do you suggest this
works in the general case? Do you know of a single firmware
implementation that sets DS=1 for GIC500 or GIC600?

This feels like code that has been lifted verbatim from a firmware
implementation without even thinking of the consequences...

> +			;
> +
> +	}
> +}
> +
>  static void gic_dist_save(void)
>  {
>  	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
> @@ -871,6 +900,7 @@ static int gic_suspend(void)
>  	for_each_possible_cpu(cpu)
>  		gic_redist_save(cpu);
>  
> +	gic_power_down();
>  	its_save_disable();
>  	gic_dist_save();
>  
> @@ -901,6 +931,33 @@ static void gic_rdist_restore(int cpu)
>  	gic_do_wait_for_rwp(base);
>  }
>  
> +static void gic_power_up(void)
> +{
> +	void __iomem *base = gic_data.dist_base;
> +	u32 product_id = readl_relaxed(base + GICD_IIDR) >>
> +			 GICD_IIDR_PRODUCTID_SHIFT;
> +
> +	/*
> +	 * Same as the power down sequence, this part of the power up sequence
> +	 * is implementation defined.
> +	 */
> +	if ((product_id & GIC500_IIDR_PRODUCTID) ||
> +	    (product_id & GIC600_IIDR_PRODUCTID)) {
> +		u32 val;
> +
> +		/*
> +		 * Need to turn the GIC back on in-case suspend is cancelled.
> +		 * The GIC hardware reset state or the platform layer should
> +		 * handle this otherwise.
> +		 */
> +		base = gic_data_rdist_rd_base();
> +		val = readl_relaxed(base + GICR_WAKER);
> +		writel_relaxed(val & ~GICR_WAKER_Sleep, base + GICR_WAKER);
> +		while (readl_relaxed(base + GICR_WAKER) & GICR_WAKER_Quiescent)
> +			;
> +	}
> +}

Same here.

> +
>  static void gic_dist_restore(void)
>  {
>  	struct gic_dist_ctx *ctx = gic_data.gicd_ctx;
> @@ -937,6 +994,7 @@ static void gic_resume(void)
>  
>  	gic_dist_restore();
>  	its_restore_enable();
> +	gic_power_up();
>  	for_each_possible_cpu(cpu)
>  		gic_rdist_restore(cpu);
>  
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index f086987e3cb4..22ced72be1c5 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -59,6 +59,10 @@
>  #define GICD_NSACR_SHIFT		4
>  #define GICD_IROUTER_SHIFT		0
>  
> +#define GICD_IIDR_PRODUCTID_SHIFT	24
> +#define GIC500_IIDR_PRODUCTID		0x00
> +#define GIC600_IIDR_PRODUCTID		0x02
> +
>  /*
>   * 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).
> @@ -122,8 +126,13 @@
>  
>  #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)
>  
> +/*
> + * Sleep and Quiescent are implementation specific for the GIC-500 and GIC-600.
> + */
> +#define GICR_WAKER_Sleep		(1U << 0)
>  #define GICR_WAKER_ProcessorSleep	(1U << 1)
>  #define GICR_WAKER_ChildrenAsleep	(1U << 2)
> +#define GICR_WAKER_Quiescent		(1U << 31)
>  
>  #define GIC_BASER_CACHE_nCnB		0ULL
>  #define GIC_BASER_CACHE_SameAsInner	0ULL
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

I really don't see how this patch can have any effect on any known
implementation. Care to shed some light?

Thanks,

	M.