[PATCH v4 0/5] GICv3 Save and Restore

[PATCH v4 0/5] GICv3 Save and Restore

[Derek Basehore]

A lot of changes in v2. The distributor and redistributor saving and
restoring is left to the PSCI/firmware implementation after
discussions with ARM. This reduces the line changes by a lot and
removes now unneeded patches.

Patches are verified on an RK3399 platform with pending patches in the
ARM-Trusted-Firmware project.

Just a couple minor changes in v3 to formatting.

Fixed a false ITS wedged detection due to the cmd_write and creadr
offsets not matching up on reset in v4. Also minor formatting changes.

Derek Basehore (5):
  cpu_pm: add syscore_suspend error handling
  irqchip/gic-v3-its: add ability to save/restore ITS state
  DT/arm,gic-v3-its: add reset-on-suspend property
  irqchip/gic-v3-its: add ability to resend MAPC on resume
  DT/arm,gic-v3: add collections-reset-on-suspend property

 .../bindings/interrupt-controller/arm,gic-v3.txt   |   7 +
 arch/arm64/Kconfig                                 |  10 +
 drivers/irqchip/irq-gic-v3-its.c                   | 202 +++++++++++++++++----
 kernel/cpu_pm.c                                    |   3 +
 4 files changed, 184 insertions(+), 38 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH v4 1/5] 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.

Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[Derek Basehore]

Some platforms power off GIC logic in suspend, so we need to
save/restore state. The distributor and redistributor registers need
to be handled in platform code due to access permissions on those
registers, but the ITS registers can be restored in the kernel.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06f025fd5726..e13515cdb68f 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -33,6 +33,7 @@
 #include <linux/of_platform.h>
 #include <linux/percpu.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
 
 #include <linux/irqchip.h>
 #include <linux/irqchip/arm-gic-v3.h>
@@ -46,6 +47,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_SAVE_SUSPEND_STATE		(1ULL << 3)
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
 
@@ -83,6 +85,15 @@ struct its_baser {
 	u32		psz;
 };
 
+/*
+ * 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;
+	u32			ctlr;
+};
+
 struct its_device;
 
 /*
@@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
 	gic_enable_quirks(iidr, its_quirks, its);
 }
 
+static int its_save_disable(void)
+{
+	struct its_node *its;
+	int err = 0;
+
+	spin_lock(&its_lock);
+	list_for_each_entry(its, &its_nodes, entry) {
+		struct its_ctx *ctx;
+		void __iomem *base;
+
+		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
+			continue;
+
+		ctx = &its->its_ctx;
+		base = its->base;
+		ctx->ctlr = readl_relaxed(base + GITS_CTLR);
+		err = its_force_quiescent(base);
+		if (err) {
+			pr_err("ITS failed to quiesce\n");
+			writel_relaxed(ctx->ctlr, base + GITS_CTLR);
+			goto err;
+		}
+
+		ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
+	}
+
+err:
+	if (err) {
+		list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
+			if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
+				struct its_ctx *ctx = &its->its_ctx;
+				void __iomem *base = its->base;
+
+				writel_relaxed(ctx->ctlr, base + GITS_CTLR);
+			}
+		}
+	}
+
+	spin_unlock(&its_lock);
+
+	return err;
+}
+
+static void its_restore_enable(void)
+{
+	struct its_node *its;
+
+	spin_lock(&its_lock);
+	list_for_each_entry(its, &its_nodes, entry) {
+		if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
+			struct its_ctx *ctx = &its->its_ctx;
+			void __iomem *base = its->base;
+			/*
+			 * Only the lower 32 bits matter here since the upper 32
+			 * don't include any of the offset.
+			 */
+			u32 creader = readl_relaxed(base + GITS_CREADR);
+			int i;
+
+			/*
+			 * Reset the write location to where the ITS is
+			 * currently at.
+			 */
+			gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
+			gits_write_cwriter(creader, base + GITS_CWRITER);
+			its->cmd_write = &its->cmd_base[
+				creader / sizeof(struct its_cmd_block)];
+			/* Restore GITS_BASER from the value cache. */
+			for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+				struct its_baser *baser = &its->tables[i];
+
+				its_write_baser(its, baser, baser->val);
+			}
+			writel_relaxed(ctx->ctlr, base + GITS_CTLR);
+		}
+	}
+	spin_unlock(&its_lock);
+}
+
+static struct syscore_ops its_syscore_ops = {
+	.suspend = its_save_disable,
+	.resume = its_restore_enable,
+};
+
 static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 {
 	struct irq_domain *inner_domain;
@@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
+	if (fwnode_property_present(handle, "reset-on-suspend"))
+		its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
+
 	err = its_init_domain(handle, its);
 	if (err)
 		goto out_free_tables;
@@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 		}
 	}
 
+	register_syscore_ops(&its_syscore_ops);
+
 	return 0;
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH v4 3/5] DT/arm,gic-v3-its: add reset-on-suspend property

[Derek Basehore]

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

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt | 3 +++
 1 file changed, 3 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..a470147d4f14 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -78,6 +78,9 @@ 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.
+- reset-on-suspend: Boolean property. Indicates that the ITS state is
+  reset on suspend. The state is then saved on suspend and restored on
+  resume.
 
 The main GIC node must contain the appropriate #address-cells,
 #size-cells and ranges properties for the reg property of all ITS
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH v4 4/5] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Derek Basehore]

This adds functionality 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.

This feature is enabled via Kconfig and a device tree entry.

Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 arch/arm64/Kconfig               |  10 ++++
 drivers/irqchip/irq-gic-v3-its.c | 101 ++++++++++++++++++++++++---------------
 2 files changed, 73 insertions(+), 38 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 53612879fe56..f38f1a7b4266 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -571,6 +571,16 @@ config HISILICON_ERRATUM_161600802
 
 	  If unsure, say Y.
 
+config ARM_GIC500_COLLECTIONS_RESET
+	bool "GIC-500 Collections: Workaround for GIC-500 Collections on suspend reset"
+	default y
+	help
+	  The GIC-500 can store Collections state internally for the ITS. If
+	  the ITS is reset on suspend (ie from power getting disabled), the
+	  collections need to be reconfigured on resume.
+
+	  If unsure, say Y.
+
 config QCOM_FALKOR_ERRATUM_E1041
 	bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
 	default y
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e13515cdb68f..63764efa4dcc 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -48,6 +48,7 @@
 #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
 #define ITS_FLAGS_SAVE_SUSPEND_STATE		(1ULL << 3)
+#define ITS_FLAGS_WORKAROUND_GIC500_MAPC	(1ULL << 4)
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
 
@@ -1950,52 +1951,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);
 }
@@ -2997,6 +2999,18 @@ static bool __maybe_unused its_enable_quirk_hip07_161600802(void *data)
 	return true;
 }
 
+static bool __maybe_unused its_enable_quirk_gic500_collections(void *data)
+{
+	struct its_node *its = data;
+
+	if (fwnode_property_present(its->fwnode_handle,
+				    "collections-reset-on-suspend")) {
+		its->flags |= ITS_FLAGS_WORKAROUND_GIC500_MAPC;
+		return true;
+	}
+	return false;
+}
+
 static const struct gic_quirk its_quirks[] = {
 #ifdef CONFIG_CAVIUM_ERRATUM_22375
 	{
@@ -3042,6 +3056,14 @@ static const struct gic_quirk its_quirks[] = {
 		.mask	= 0xffffffff,
 		.init	= its_enable_quirk_hip07_161600802,
 	},
+#endif
+#ifdef CONFIG_ARM_GIC500_COLLECTIONS_RESET
+	{
+		.desc	= "ITS: GIC-500 Collections Reset on Resume",
+		.iidr	= 0x00000000,
+		.mask	= 0xff000000,
+		.init	= its_enable_quirk_gic500_collections,
+	},
 #endif
 	{
 	}
@@ -3129,6 +3151,9 @@ static void its_restore_enable(void)
 			}
 			writel_relaxed(ctx->ctlr, base + GITS_CTLR);
 		}
+
+		if (its->flags & ITS_FLAGS_WORKAROUND_GIC500_MAPC)
+			its_cpu_init_collection(its);
 	}
 	spin_unlock(&its_lock);
 }
@@ -3395,7 +3420,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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 5/5] DT/arm,gic-v3: add collections-reset-on-suspend property

[Derek Basehore]

This boolean property for the GIC-V3-ITS enables resending the MAP
COLLECTIONS commands when resuming for when the state is reset on
suspend.

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

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index a470147d4f14..adb958e046d2 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -81,6 +81,10 @@ Optional:
 - reset-on-suspend: Boolean property. Indicates that the ITS state is
   reset on suspend. The state is then saved on suspend and restored on
   resume.
+- collections-reset-on-suspend : Boolean property. If the collections for the
+  ITS are stored internally instead of externally, 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 main GIC node must contain the appropriate #address-cells,
 #size-cells and ranges properties for the reg property of all ITS
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[Marc Zyngier]

On 03/02/18 01:24, Derek Basehore wrote:
> Some platforms power off GIC logic in suspend, so we need to
> save/restore state. The distributor and redistributor registers need
> to be handled in platform code due to access permissions on those
> registers, but the ITS registers can be restored in the kernel.
> 
> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..e13515cdb68f 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -33,6 +33,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/percpu.h>
>  #include <linux/slab.h>
> +#include <linux/syscore_ops.h>
>  
>  #include <linux/irqchip.h>
>  #include <linux/irqchip/arm-gic-v3.h>
> @@ -46,6 +47,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_SAVE_SUSPEND_STATE		(1ULL << 3)
>  
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  
> @@ -83,6 +85,15 @@ struct its_baser {
>  	u32		psz;
>  };
>  
> +/*
> + * 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;
> +	u32			ctlr;
> +};

Nit: This is pretty small for the ITS context. Given that its_node is
the context, you can safely expand this in the its_node structure.

> +
>  struct its_device;
>  
>  /*
> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>  	gic_enable_quirks(iidr, its_quirks, its);
>  }
>  
> +static int its_save_disable(void)
> +{
> +	struct its_node *its;
> +	int err = 0;
> +
> +	spin_lock(&its_lock);
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		struct its_ctx *ctx;
> +		void __iomem *base;
> +
> +		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> +			continue;
> +
> +		ctx = &its->its_ctx;
> +		base = its->base;
> +		ctx->ctlr = readl_relaxed(base + GITS_CTLR);
> +		err = its_force_quiescent(base);
> +		if (err) {
> +			pr_err("ITS failed to quiesce\n");
> +			writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> +			goto err;
> +		}
> +
> +		ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
> +	}
> +
> +err:
> +	if (err) {
> +		list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
> +			if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
> +				struct its_ctx *ctx = &its->its_ctx;
> +				void __iomem *base = its->base;
> +
> +				writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> +			}
> +		}
> +	}
> +
> +	spin_unlock(&its_lock);
> +
> +	return err;
> +}
> +
> +static void its_restore_enable(void)
> +{
> +	struct its_node *its;
> +
> +	spin_lock(&its_lock);
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
> +			struct its_ctx *ctx = &its->its_ctx;
> +			void __iomem *base = its->base;
> +			/*
> +			 * Only the lower 32 bits matter here since the upper 32
> +			 * don't include any of the offset.
> +			 */
> +			u32 creader = readl_relaxed(base + GITS_CREADR);

Accessor matching gits_write_cwriter and co?

> +			int i;
> +
> +			/*
> +			 * Reset the write location to where the ITS is
> +			 * currently at.
> +			 */
> +			gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
> +			gits_write_cwriter(creader, base + GITS_CWRITER);
> +			its->cmd_write = &its->cmd_base[
> +				creader / sizeof(struct its_cmd_block)];

Nit: please do not split lines like this, this is unreadable. We both
have screens that are wide enough for this to fit on a single line.

More importantly: Why isn't it sufficient to reset both CREADR and
CWRITER to zero? Is there any case where you can suspend whilst having
anything in flight?

> +			/* Restore GITS_BASER from the value cache. */
> +			for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> +				struct its_baser *baser = &its->tables[i];
> +
> +				its_write_baser(its, baser, baser->val);

You may want to first test that this BASER register is actually
requiring something before writing to it. Yes, this is normally safe.
But HW is also normally broken.

> +			}
> +			writel_relaxed(ctx->ctlr, base + GITS_CTLR);

Before restoring all of this, shouldn't you first test that the ITS is
actually in a disabled state?

> +		}
> +	}
> +	spin_unlock(&its_lock);
> +}
> +
> +static struct syscore_ops its_syscore_ops = {
> +	.suspend = its_save_disable,
> +	.resume = its_restore_enable,
> +};
> +
>  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>  {
>  	struct irq_domain *inner_domain;
> @@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> +	if (fwnode_property_present(handle, "reset-on-suspend"))
> +		its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
> +
>  	err = its_init_domain(handle, its);
>  	if (err)
>  		goto out_free_tables;
> @@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  		}
>  	}
>  
> +	register_syscore_ops(&its_syscore_ops);
> +
>  	return 0;
>  }
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[dbasehore .]

On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 03/02/18 01:24, Derek Basehore wrote:
>> Some platforms power off GIC logic in suspend, so we need to
>> save/restore state. The distributor and redistributor registers need
>> to be handled in platform code due to access permissions on those
>> registers, but the ITS registers can be restored in the kernel.
>>
>> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..e13515cdb68f 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -33,6 +33,7 @@
>>  #include <linux/of_platform.h>
>>  #include <linux/percpu.h>
>>  #include <linux/slab.h>
>> +#include <linux/syscore_ops.h>
>>
>>  #include <linux/irqchip.h>
>>  #include <linux/irqchip/arm-gic-v3.h>
>> @@ -46,6 +47,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_SAVE_SUSPEND_STATE         (1ULL << 3)
>>
>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>
>> @@ -83,6 +85,15 @@ struct its_baser {
>>       u32             psz;
>>  };
>>
>> +/*
>> + * 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;
>> +     u32                     ctlr;
>> +};
>
> Nit: This is pretty small for the ITS context. Given that its_node is
> the context, you can safely expand this in the its_node structure.

Sounds reasonable. I think I just have it this way because I used to
have more in here.

>
>> +
>>  struct its_device;
>>
>>  /*
>> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>>       gic_enable_quirks(iidr, its_quirks, its);
>>  }
>>
>> +static int its_save_disable(void)
>> +{
>> +     struct its_node *its;
>> +     int err = 0;
>> +
>> +     spin_lock(&its_lock);
>> +     list_for_each_entry(its, &its_nodes, entry) {
>> +             struct its_ctx *ctx;
>> +             void __iomem *base;
>> +
>> +             if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> +                     continue;
>> +
>> +             ctx = &its->its_ctx;
>> +             base = its->base;
>> +             ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>> +             err = its_force_quiescent(base);
>> +             if (err) {
>> +                     pr_err("ITS failed to quiesce\n");
>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> +                     goto err;
>> +             }
>> +
>> +             ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>> +     }
>> +
>> +err:
>> +     if (err) {
>> +             list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>> +                     if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> +                             struct its_ctx *ctx = &its->its_ctx;
>> +                             void __iomem *base = its->base;
>> +
>> +                             writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> +                     }
>> +             }
>> +     }
>> +
>> +     spin_unlock(&its_lock);
>> +
>> +     return err;
>> +}
>> +
>> +static void its_restore_enable(void)
>> +{
>> +     struct its_node *its;
>> +
>> +     spin_lock(&its_lock);
>> +     list_for_each_entry(its, &its_nodes, entry) {
>> +             if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> +                     struct its_ctx *ctx = &its->its_ctx;
>> +                     void __iomem *base = its->base;
>> +                     /*
>> +                      * Only the lower 32 bits matter here since the upper 32
>> +                      * don't include any of the offset.
>> +                      */
>> +                     u32 creader = readl_relaxed(base + GITS_CREADR);
>
> Accessor matching gits_write_cwriter and co?
>
>> +                     int i;
>> +
>> +                     /*
>> +                      * Reset the write location to where the ITS is
>> +                      * currently at.
>> +                      */
>> +                     gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>> +                     gits_write_cwriter(creader, base + GITS_CWRITER);
>> +                     its->cmd_write = &its->cmd_base[
>> +                             creader / sizeof(struct its_cmd_block)];
>
> Nit: please do not split lines like this, this is unreadable. We both
> have screens that are wide enough for this to fit on a single line.
>
> More importantly: Why isn't it sufficient to reset both CREADR and
> CWRITER to zero? Is there any case where you can suspend whilst having
> anything in flight?

CREADR is RO and we need to handle the non-zero case. I was planning
on getting rid of the write to CWRITER since it shouldn't be needed.
Either CREADR and CWRITER have the prior values, or both are reset to
0.

>
>> +                     /* Restore GITS_BASER from the value cache. */
>> +                     for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>> +                             struct its_baser *baser = &its->tables[i];
>> +
>> +                             its_write_baser(its, baser, baser->val);
>
> You may want to first test that this BASER register is actually
> requiring something before writing to it. Yes, this is normally safe.
> But HW is also normally broken.
>
>> +                     }
>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>
> Before restoring all of this, shouldn't you first test that the ITS is
> actually in a disabled state?

The save_disable code put it in a disabled state. The reset state is
also disabled. I don't expect PSCI to change the GITS_CTLR register at
all. Are you expecting something on the other side of the PSCI layer
to poke this register? We'd probably want to force disable at the
start of restore_enable if so.

>
>> +             }
>> +     }
>> +     spin_unlock(&its_lock);
>> +}
>> +
>> +static struct syscore_ops its_syscore_ops = {
>> +     .suspend = its_save_disable,
>> +     .resume = its_restore_enable,
>> +};
>> +
>>  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>>  {
>>       struct irq_domain *inner_domain;
>> @@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *res,
>>               ctlr |= GITS_CTLR_ImDe;
>>       writel_relaxed(ctlr, its->base + GITS_CTLR);
>>
>> +     if (fwnode_property_present(handle, "reset-on-suspend"))
>> +             its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>> +
>>       err = its_init_domain(handle, its);
>>       if (err)
>>               goto out_free_tables;
>> @@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>               }
>>       }
>>
>> +     register_syscore_ops(&its_syscore_ops);
>> +
>>       return 0;
>>  }
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Thanks for the review.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[dbasehore .]

On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 03/02/18 01:24, Derek Basehore wrote:
>> Some platforms power off GIC logic in suspend, so we need to
>> save/restore state. The distributor and redistributor registers need
>> to be handled in platform code due to access permissions on those
>> registers, but the ITS registers can be restored in the kernel.
>>
>> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..e13515cdb68f 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -33,6 +33,7 @@
>>  #include <linux/of_platform.h>
>>  #include <linux/percpu.h>
>>  #include <linux/slab.h>
>> +#include <linux/syscore_ops.h>
>>
>>  #include <linux/irqchip.h>
>>  #include <linux/irqchip/arm-gic-v3.h>
>> @@ -46,6 +47,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_SAVE_SUSPEND_STATE         (1ULL << 3)
>>
>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>
>> @@ -83,6 +85,15 @@ struct its_baser {
>>       u32             psz;
>>  };
>>
>> +/*
>> + * 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;
>> +     u32                     ctlr;
>> +};
>
> Nit: This is pretty small for the ITS context. Given that its_node is
> the context, you can safely expand this in the its_node structure.
>
>> +
>>  struct its_device;
>>
>>  /*
>> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>>       gic_enable_quirks(iidr, its_quirks, its);
>>  }
>>
>> +static int its_save_disable(void)
>> +{
>> +     struct its_node *its;
>> +     int err = 0;
>> +
>> +     spin_lock(&its_lock);
>> +     list_for_each_entry(its, &its_nodes, entry) {
>> +             struct its_ctx *ctx;
>> +             void __iomem *base;
>> +
>> +             if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> +                     continue;
>> +
>> +             ctx = &its->its_ctx;
>> +             base = its->base;
>> +             ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>> +             err = its_force_quiescent(base);
>> +             if (err) {
>> +                     pr_err("ITS failed to quiesce\n");
>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> +                     goto err;
>> +             }
>> +
>> +             ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>> +     }
>> +
>> +err:
>> +     if (err) {
>> +             list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>> +                     if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> +                             struct its_ctx *ctx = &its->its_ctx;
>> +                             void __iomem *base = its->base;
>> +
>> +                             writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> +                     }
>> +             }
>> +     }
>> +
>> +     spin_unlock(&its_lock);
>> +
>> +     return err;
>> +}
>> +
>> +static void its_restore_enable(void)
>> +{
>> +     struct its_node *its;
>> +
>> +     spin_lock(&its_lock);
>> +     list_for_each_entry(its, &its_nodes, entry) {
>> +             if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> +                     struct its_ctx *ctx = &its->its_ctx;
>> +                     void __iomem *base = its->base;
>> +                     /*
>> +                      * Only the lower 32 bits matter here since the upper 32
>> +                      * don't include any of the offset.
>> +                      */
>> +                     u32 creader = readl_relaxed(base + GITS_CREADR);
>
> Accessor matching gits_write_cwriter and co?
>
>> +                     int i;
>> +
>> +                     /*
>> +                      * Reset the write location to where the ITS is
>> +                      * currently at.
>> +                      */
>> +                     gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>> +                     gits_write_cwriter(creader, base + GITS_CWRITER);
>> +                     its->cmd_write = &its->cmd_base[
>> +                             creader / sizeof(struct its_cmd_block)];
>
> Nit: please do not split lines like this, this is unreadable. We both
> have screens that are wide enough for this to fit on a single line.
>
> More importantly: Why isn't it sufficient to reset both CREADR and
> CWRITER to zero? Is there any case where you can suspend whilst having
> anything in flight?
>
>> +                     /* Restore GITS_BASER from the value cache. */
>> +                     for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>> +                             struct its_baser *baser = &its->tables[i];
>> +
>> +                             its_write_baser(its, baser, baser->val);
>
> You may want to first test that this BASER register is actually
> requiring something before writing to it. Yes, this is normally safe.
> But HW is also normally broken.
>

Anything specific to test? I see that I can check if the BASER
register has the valid bit set and also that the type is not none.

>> +                     }
>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>
> Before restoring all of this, shouldn't you first test that the ITS is
> actually in a disabled state?
>
>> +             }
>> +     }
>> +     spin_unlock(&its_lock);
>> +}
>> +
>> +static struct syscore_ops its_syscore_ops = {
>> +     .suspend = its_save_disable,
>> +     .resume = its_restore_enable,
>> +};
>> +
>>  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>>  {
>>       struct irq_domain *inner_domain;
>> @@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *res,
>>               ctlr |= GITS_CTLR_ImDe;
>>       writel_relaxed(ctlr, its->base + GITS_CTLR);
>>
>> +     if (fwnode_property_present(handle, "reset-on-suspend"))
>> +             its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>> +
>>       err = its_init_domain(handle, its);
>>       if (err)
>>               goto out_free_tables;
>> @@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>               }
>>       }
>>
>> +     register_syscore_ops(&its_syscore_ops);
>> +
>>       return 0;
>>  }
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[dbasehore .]

On Mon, Feb 5, 2018 at 4:33 PM, dbasehore . <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 03/02/18 01:24, Derek Basehore wrote:
>>> Some platforms power off GIC logic in suspend, so we need to
>>> save/restore state. The distributor and redistributor registers need
>>> to be handled in platform code due to access permissions on those
>>> registers, but the ITS registers can be restored in the kernel.
>>>
>>> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> ---
>>>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..e13515cdb68f 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -33,6 +33,7 @@
>>>  #include <linux/of_platform.h>
>>>  #include <linux/percpu.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/syscore_ops.h>
>>>
>>>  #include <linux/irqchip.h>
>>>  #include <linux/irqchip/arm-gic-v3.h>
>>> @@ -46,6 +47,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_SAVE_SUSPEND_STATE         (1ULL << 3)
>>>
>>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>>
>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>       u32             psz;
>>>  };
>>>
>>> +/*
>>> + * 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;
>>> +     u32                     ctlr;
>>> +};
>>
>> Nit: This is pretty small for the ITS context. Given that its_node is
>> the context, you can safely expand this in the its_node structure.
>>
>>> +
>>>  struct its_device;
>>>
>>>  /*
>>> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>>>       gic_enable_quirks(iidr, its_quirks, its);
>>>  }
>>>
>>> +static int its_save_disable(void)
>>> +{
>>> +     struct its_node *its;
>>> +     int err = 0;
>>> +
>>> +     spin_lock(&its_lock);
>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>> +             struct its_ctx *ctx;
>>> +             void __iomem *base;
>>> +
>>> +             if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>> +                     continue;
>>> +
>>> +             ctx = &its->its_ctx;
>>> +             base = its->base;
>>> +             ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>> +             err = its_force_quiescent(base);
>>> +             if (err) {
>>> +                     pr_err("ITS failed to quiesce\n");
>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> +                     goto err;
>>> +             }
>>> +
>>> +             ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>> +     }
>>> +
>>> +err:
>>> +     if (err) {
>>> +             list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>>> +                     if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> +                             struct its_ctx *ctx = &its->its_ctx;
>>> +                             void __iomem *base = its->base;
>>> +
>>> +                             writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> +                     }
>>> +             }
>>> +     }
>>> +
>>> +     spin_unlock(&its_lock);
>>> +
>>> +     return err;
>>> +}
>>> +
>>> +static void its_restore_enable(void)
>>> +{
>>> +     struct its_node *its;
>>> +
>>> +     spin_lock(&its_lock);
>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>> +             if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> +                     struct its_ctx *ctx = &its->its_ctx;
>>> +                     void __iomem *base = its->base;
>>> +                     /*
>>> +                      * Only the lower 32 bits matter here since the upper 32
>>> +                      * don't include any of the offset.
>>> +                      */
>>> +                     u32 creader = readl_relaxed(base + GITS_CREADR);
>>
>> Accessor matching gits_write_cwriter and co?
>>
>>> +                     int i;
>>> +
>>> +                     /*
>>> +                      * Reset the write location to where the ITS is
>>> +                      * currently at.
>>> +                      */
>>> +                     gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>> +                     gits_write_cwriter(creader, base + GITS_CWRITER);
>>> +                     its->cmd_write = &its->cmd_base[
>>> +                             creader / sizeof(struct its_cmd_block)];
>>
>> Nit: please do not split lines like this, this is unreadable. We both
>> have screens that are wide enough for this to fit on a single line.
>>
>> More importantly: Why isn't it sufficient to reset both CREADR and
>> CWRITER to zero? Is there any case where you can suspend whilst having
>> anything in flight?
>>
>>> +                     /* Restore GITS_BASER from the value cache. */
>>> +                     for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>>> +                             struct its_baser *baser = &its->tables[i];
>>> +
>>> +                             its_write_baser(its, baser, baser->val);
>>
>> You may want to first test that this BASER register is actually
>> requiring something before writing to it. Yes, this is normally safe.
>> But HW is also normally broken.
>>
>
> Anything specific to test? I see that I can check if the BASER
> register has the valid bit set and also that the type is not none.
>

Okay, I found this snippet in the GICv3 Specification:
No memory is allocated for the translation table. The ITS discards any
writes to the
interrupt translation page when either:
• GITS_BASER<n>.Type specifies any valid table entry type other than interrupt
collections, that is, any value other than 100.
• GITS_BASER<n>.Type specifies an interrupt collection and
GITS_TYPER.HCC == 0.

I'll turn this into a test in code and only write the BASER if that passes.

>>> +                     }
>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>
>> Before restoring all of this, shouldn't you first test that the ITS is
>> actually in a disabled state?
>>
>>> +             }
>>> +     }
>>> +     spin_unlock(&its_lock);
>>> +}
>>> +
>>> +static struct syscore_ops its_syscore_ops = {
>>> +     .suspend = its_save_disable,
>>> +     .resume = its_restore_enable,
>>> +};
>>> +
>>>  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>>>  {
>>>       struct irq_domain *inner_domain;
>>> @@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *res,
>>>               ctlr |= GITS_CTLR_ImDe;
>>>       writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>
>>> +     if (fwnode_property_present(handle, "reset-on-suspend"))
>>> +             its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>> +
>>>       err = its_init_domain(handle, its);
>>>       if (err)
>>>               goto out_free_tables;
>>> @@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>>               }
>>>       }
>>>
>>> +     register_syscore_ops(&its_syscore_ops);
>>> +
>>>       return 0;
>>>  }
>>>
>>
>> Thanks,
>>
>>         M.
>> --
>> Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[dbasehore .]

On Mon, Feb 5, 2018 at 5:01 PM, dbasehore . <dbasehore@chromium.org> wrote:
> On Mon, Feb 5, 2018 at 4:33 PM, dbasehore . <dbasehore@chromium.org> wrote:
>> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 03/02/18 01:24, Derek Basehore wrote:
>>>> Some platforms power off GIC logic in suspend, so we need to
>>>> save/restore state. The distributor and redistributor registers need
>>>> to be handled in platform code due to access permissions on those
>>>> registers, but the ITS registers can be restored in the kernel.
>>>>
>>>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 101 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>> index 06f025fd5726..e13515cdb68f 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -33,6 +33,7 @@
>>>>  #include <linux/of_platform.h>
>>>>  #include <linux/percpu.h>
>>>>  #include <linux/slab.h>
>>>> +#include <linux/syscore_ops.h>
>>>>
>>>>  #include <linux/irqchip.h>
>>>>  #include <linux/irqchip/arm-gic-v3.h>
>>>> @@ -46,6 +47,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_SAVE_SUSPEND_STATE         (1ULL << 3)
>>>>
>>>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>>>
>>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>>       u32             psz;
>>>>  };
>>>>
>>>> +/*
>>>> + * 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;
>>>> +     u32                     ctlr;
>>>> +};
>>>
>>> Nit: This is pretty small for the ITS context. Given that its_node is
>>> the context, you can safely expand this in the its_node structure.
>>>
>>>> +
>>>>  struct its_device;
>>>>
>>>>  /*
>>>> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>>>>       gic_enable_quirks(iidr, its_quirks, its);
>>>>  }
>>>>
>>>> +static int its_save_disable(void)
>>>> +{
>>>> +     struct its_node *its;
>>>> +     int err = 0;
>>>> +
>>>> +     spin_lock(&its_lock);
>>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>>> +             struct its_ctx *ctx;
>>>> +             void __iomem *base;
>>>> +
>>>> +             if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>>> +                     continue;
>>>> +
>>>> +             ctx = &its->its_ctx;
>>>> +             base = its->base;
>>>> +             ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>>> +             err = its_force_quiescent(base);
>>>> +             if (err) {
>>>> +                     pr_err("ITS failed to quiesce\n");
>>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>>> +                     goto err;
>>>> +             }
>>>> +
>>>> +             ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>>> +     }
>>>> +
>>>> +err:
>>>> +     if (err) {
>>>> +             list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>>>> +                     if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>>> +                             struct its_ctx *ctx = &its->its_ctx;
>>>> +                             void __iomem *base = its->base;
>>>> +
>>>> +                             writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>>> +                     }
>>>> +             }
>>>> +     }
>>>> +
>>>> +     spin_unlock(&its_lock);
>>>> +
>>>> +     return err;
>>>> +}
>>>> +
>>>> +static void its_restore_enable(void)
>>>> +{
>>>> +     struct its_node *its;
>>>> +
>>>> +     spin_lock(&its_lock);
>>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>>> +             if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>>> +                     struct its_ctx *ctx = &its->its_ctx;
>>>> +                     void __iomem *base = its->base;
>>>> +                     /*
>>>> +                      * Only the lower 32 bits matter here since the upper 32
>>>> +                      * don't include any of the offset.
>>>> +                      */
>>>> +                     u32 creader = readl_relaxed(base + GITS_CREADR);
>>>
>>> Accessor matching gits_write_cwriter and co?
>>>
>>>> +                     int i;
>>>> +
>>>> +                     /*
>>>> +                      * Reset the write location to where the ITS is
>>>> +                      * currently at.
>>>> +                      */
>>>> +                     gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>>> +                     gits_write_cwriter(creader, base + GITS_CWRITER);
>>>> +                     its->cmd_write = &its->cmd_base[
>>>> +                             creader / sizeof(struct its_cmd_block)];
>>>
>>> Nit: please do not split lines like this, this is unreadable. We both
>>> have screens that are wide enough for this to fit on a single line.
>>>
>>> More importantly: Why isn't it sufficient to reset both CREADR and
>>> CWRITER to zero? Is there any case where you can suspend whilst having
>>> anything in flight?
>>>
>>>> +                     /* Restore GITS_BASER from the value cache. */
>>>> +                     for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>>>> +                             struct its_baser *baser = &its->tables[i];
>>>> +
>>>> +                             its_write_baser(its, baser, baser->val);
>>>
>>> You may want to first test that this BASER register is actually
>>> requiring something before writing to it. Yes, this is normally safe.
>>> But HW is also normally broken.
>>>
>>
>> Anything specific to test? I see that I can check if the BASER
>> register has the valid bit set and also that the type is not none.
>>
>
> Okay, I found this snippet in the GICv3 Specification:
> No memory is allocated for the translation table. The ITS discards any
> writes to the
> interrupt translation page when either:
> • GITS_BASER<n>.Type specifies any valid table entry type other than interrupt
> collections, that is, any value other than 100.
> • GITS_BASER<n>.Type specifies an interrupt collection and
> GITS_TYPER.HCC == 0.

This is when GITS_BASER.Valid == 0 by the way.

>
> I'll turn this into a test in code and only write the BASER if that passes.
>
>>>> +                     }
>>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>>
>>> Before restoring all of this, shouldn't you first test that the ITS is
>>> actually in a disabled state?
>>>
>>>> +             }
>>>> +     }
>>>> +     spin_unlock(&its_lock);
>>>> +}
>>>> +
>>>> +static struct syscore_ops its_syscore_ops = {
>>>> +     .suspend = its_save_disable,
>>>> +     .resume = its_restore_enable,
>>>> +};
>>>> +
>>>>  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>>>>  {
>>>>       struct irq_domain *inner_domain;
>>>> @@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *res,
>>>>               ctlr |= GITS_CTLR_ImDe;
>>>>       writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>>
>>>> +     if (fwnode_property_present(handle, "reset-on-suspend"))
>>>> +             its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>>> +
>>>>       err = its_init_domain(handle, its);
>>>>       if (err)
>>>>               goto out_free_tables;
>>>> @@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>>>               }
>>>>       }
>>>>
>>>> +     register_syscore_ops(&its_syscore_ops);
>>>> +
>>>>       return 0;
>>>>  }
>>>>
>>>
>>> Thanks,
>>>
>>>         M.
>>> --
>>> Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[Marc Zyngier]

On 05/02/18 21:33, dbasehore . wrote:
> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 03/02/18 01:24, Derek Basehore wrote:
>>> Some platforms power off GIC logic in suspend, so we need to
>>> save/restore state. The distributor and redistributor registers need
>>> to be handled in platform code due to access permissions on those
>>> registers, but the ITS registers can be restored in the kernel.
>>>
>>> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> ---
>>>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..e13515cdb68f 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -33,6 +33,7 @@
>>>  #include <linux/of_platform.h>
>>>  #include <linux/percpu.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/syscore_ops.h>
>>>
>>>  #include <linux/irqchip.h>
>>>  #include <linux/irqchip/arm-gic-v3.h>
>>> @@ -46,6 +47,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_SAVE_SUSPEND_STATE         (1ULL << 3)
>>>
>>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>>
>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>       u32             psz;
>>>  };
>>>
>>> +/*
>>> + * 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;
>>> +     u32                     ctlr;
>>> +};
>>
>> Nit: This is pretty small for the ITS context. Given that its_node is
>> the context, you can safely expand this in the its_node structure.
> 
> Sounds reasonable. I think I just have it this way because I used to
> have more in here.
> 
>>
>>> +
>>>  struct its_device;
>>>
>>>  /*
>>> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>>>       gic_enable_quirks(iidr, its_quirks, its);
>>>  }
>>>
>>> +static int its_save_disable(void)
>>> +{
>>> +     struct its_node *its;
>>> +     int err = 0;
>>> +
>>> +     spin_lock(&its_lock);
>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>> +             struct its_ctx *ctx;
>>> +             void __iomem *base;
>>> +
>>> +             if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>> +                     continue;
>>> +
>>> +             ctx = &its->its_ctx;
>>> +             base = its->base;
>>> +             ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>> +             err = its_force_quiescent(base);
>>> +             if (err) {
>>> +                     pr_err("ITS failed to quiesce\n");
>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> +                     goto err;
>>> +             }
>>> +
>>> +             ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>> +     }
>>> +
>>> +err:
>>> +     if (err) {
>>> +             list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>>> +                     if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> +                             struct its_ctx *ctx = &its->its_ctx;
>>> +                             void __iomem *base = its->base;
>>> +
>>> +                             writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> +                     }
>>> +             }
>>> +     }
>>> +
>>> +     spin_unlock(&its_lock);
>>> +
>>> +     return err;
>>> +}
>>> +
>>> +static void its_restore_enable(void)
>>> +{
>>> +     struct its_node *its;
>>> +
>>> +     spin_lock(&its_lock);
>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>> +             if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> +                     struct its_ctx *ctx = &its->its_ctx;
>>> +                     void __iomem *base = its->base;
>>> +                     /*
>>> +                      * Only the lower 32 bits matter here since the upper 32
>>> +                      * don't include any of the offset.
>>> +                      */
>>> +                     u32 creader = readl_relaxed(base + GITS_CREADR);
>>
>> Accessor matching gits_write_cwriter and co?
>>
>>> +                     int i;
>>> +
>>> +                     /*
>>> +                      * Reset the write location to where the ITS is
>>> +                      * currently at.
>>> +                      */
>>> +                     gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>> +                     gits_write_cwriter(creader, base + GITS_CWRITER);
>>> +                     its->cmd_write = &its->cmd_base[
>>> +                             creader / sizeof(struct its_cmd_block)];
>>
>> Nit: please do not split lines like this, this is unreadable. We both
>> have screens that are wide enough for this to fit on a single line.
>>
>> More importantly: Why isn't it sufficient to reset both CREADR and
>> CWRITER to zero? Is there any case where you can suspend whilst having
>> anything in flight?
> 
> CREADR is RO and we need to handle the non-zero case. I was planning
> on getting rid of the write to CWRITER since it shouldn't be needed.
> Either CREADR and CWRITER have the prior values, or both are reset to
> 0.

You're writing GITS_CBASER, which has for consequence: "When this
register is successfully written, the value of GITS_CREADR is set to
zero.". Ergo, none of that is necessary and you *must* set CWRITER to 0.

> 
>>
>>> +                     /* Restore GITS_BASER from the value cache. */
>>> +                     for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>>> +                             struct its_baser *baser = &its->tables[i];
>>> +
>>> +                             its_write_baser(its, baser, baser->val);
>>
>> You may want to first test that this BASER register is actually
>> requiring something before writing to it. Yes, this is normally safe.
>> But HW is also normally broken.
>>
>>> +                     }
>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>
>> Before restoring all of this, shouldn't you first test that the ITS is
>> actually in a disabled state?
> 
> The save_disable code put it in a disabled state. The reset state is> also disabled. I don't expect PSCI to change the GITS_CTLR register at
> all. Are you expecting something on the other side of the PSCI layer
> to poke this register? We'd probably want to force disable at the
> start of restore_enable if so.

I expect firmware to do its worse, because even if mainline ATF is close
to perfection, it will be hacked to death by platform people who usually
do not have a clue (that's definitely my experience). So I expect this
code to be written defensively.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/5] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Marc Zyngier]

On 03/02/18 01:24, Derek Basehore wrote:
> This adds functionality 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.
> 
> This feature is enabled via Kconfig and a device tree entry.
> 
> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  arch/arm64/Kconfig               |  10 ++++
>  drivers/irqchip/irq-gic-v3-its.c | 101 ++++++++++++++++++++++++---------------
>  2 files changed, 73 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 53612879fe56..f38f1a7b4266 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -571,6 +571,16 @@ config HISILICON_ERRATUM_161600802
>  
>  	  If unsure, say Y.
>  
> +config ARM_GIC500_COLLECTIONS_RESET
> +	bool "GIC-500 Collections: Workaround for GIC-500 Collections on suspend reset"
> +	default y
> +	help
> +	  The GIC-500 can store Collections state internally for the ITS. If
> +	  the ITS is reset on suspend (ie from power getting disabled), the
> +	  collections need to be reconfigured on resume.
> +
> +	  If unsure, say Y.
> +
>  config QCOM_FALKOR_ERRATUM_E1041
>  	bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
>  	default y
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e13515cdb68f..63764efa4dcc 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -48,6 +48,7 @@
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
>  #define ITS_FLAGS_SAVE_SUSPEND_STATE		(1ULL << 3)
> +#define ITS_FLAGS_WORKAROUND_GIC500_MAPC	(1ULL << 4)
>  
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  
> @@ -1950,52 +1951,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);
>  }
> @@ -2997,6 +2999,18 @@ static bool __maybe_unused its_enable_quirk_hip07_161600802(void *data)
>  	return true;
>  }
>  
> +static bool __maybe_unused its_enable_quirk_gic500_collections(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	if (fwnode_property_present(its->fwnode_handle,
> +				    "collections-reset-on-suspend")) {
> +		its->flags |= ITS_FLAGS_WORKAROUND_GIC500_MAPC;
> +		return true;
> +	}
> +	return false;
> +}
> +
>  static const struct gic_quirk its_quirks[] = {
>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>  	{
> @@ -3042,6 +3056,14 @@ static const struct gic_quirk its_quirks[] = {
>  		.mask	= 0xffffffff,
>  		.init	= its_enable_quirk_hip07_161600802,
>  	},
> +#endif
> +#ifdef CONFIG_ARM_GIC500_COLLECTIONS_RESET
> +	{
> +		.desc	= "ITS: GIC-500 Collections Reset on Resume",
> +		.iidr	= 0x00000000,
> +		.mask	= 0xff000000,
> +		.init	= its_enable_quirk_gic500_collections,
> +	},
>  #endif
>  	{
>  	}
> @@ -3129,6 +3151,9 @@ static void its_restore_enable(void)
>  			}
>  			writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>  		}
> +
> +		if (its->flags & ITS_FLAGS_WORKAROUND_GIC500_MAPC)
> +			its_cpu_init_collection(its);
>  	}
>  	spin_unlock(&its_lock);
>  }
> @@ -3395,7 +3420,7 @@ int its_cpu_init(void)
>  			return -ENXIO;
>  		}
>  		its_cpu_init_lpis();
> -		its_cpu_init_collection();
> +		its_cpu_init_collections();
>  	}
>  
>  	return 0;
> 

I wonder if a better way to implement this is simply to bite the bullet
and implement the letter of the architecture:

If GITS_TYPER.HCC != 0, then collections 0...HCC-1 are held in HW, and
the rest in a SW-managed table. That would allow us to deal with this
without a quirk (which I asked for initially, apologies for changing my
mind).

We still the same DT flag to tell us we're going to be reset on suspend.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/5] irqchip/gic-v3-its: add ability to resend MAPC on resume

[dbasehore .]

On Tue, Feb 6, 2018 at 8:40 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 03/02/18 01:24, Derek Basehore wrote:
>> This adds functionality 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.
>>
>> This feature is enabled via Kconfig and a device tree entry.
>>
>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> ---
>>  arch/arm64/Kconfig               |  10 ++++
>>  drivers/irqchip/irq-gic-v3-its.c | 101 ++++++++++++++++++++++++---------------
>>  2 files changed, 73 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 53612879fe56..f38f1a7b4266 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -571,6 +571,16 @@ config HISILICON_ERRATUM_161600802
>>
>>         If unsure, say Y.
>>
>> +config ARM_GIC500_COLLECTIONS_RESET
>> +     bool "GIC-500 Collections: Workaround for GIC-500 Collections on suspend reset"
>> +     default y
>> +     help
>> +       The GIC-500 can store Collections state internally for the ITS. If
>> +       the ITS is reset on suspend (ie from power getting disabled), the
>> +       collections need to be reconfigured on resume.
>> +
>> +       If unsure, say Y.
>> +
>>  config QCOM_FALKOR_ERRATUM_E1041
>>       bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
>>       default y
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index e13515cdb68f..63764efa4dcc 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -48,6 +48,7 @@
>>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375    (1ULL << 1)
>>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144    (1ULL << 2)
>>  #define ITS_FLAGS_SAVE_SUSPEND_STATE         (1ULL << 3)
>> +#define ITS_FLAGS_WORKAROUND_GIC500_MAPC     (1ULL << 4)
>>
>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>
>> @@ -1950,52 +1951,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);
>>  }
>> @@ -2997,6 +2999,18 @@ static bool __maybe_unused its_enable_quirk_hip07_161600802(void *data)
>>       return true;
>>  }
>>
>> +static bool __maybe_unused its_enable_quirk_gic500_collections(void *data)
>> +{
>> +     struct its_node *its = data;
>> +
>> +     if (fwnode_property_present(its->fwnode_handle,
>> +                                 "collections-reset-on-suspend")) {
>> +             its->flags |= ITS_FLAGS_WORKAROUND_GIC500_MAPC;
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  static const struct gic_quirk its_quirks[] = {
>>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>>       {
>> @@ -3042,6 +3056,14 @@ static const struct gic_quirk its_quirks[] = {
>>               .mask   = 0xffffffff,
>>               .init   = its_enable_quirk_hip07_161600802,
>>       },
>> +#endif
>> +#ifdef CONFIG_ARM_GIC500_COLLECTIONS_RESET
>> +     {
>> +             .desc   = "ITS: GIC-500 Collections Reset on Resume",
>> +             .iidr   = 0x00000000,
>> +             .mask   = 0xff000000,
>> +             .init   = its_enable_quirk_gic500_collections,
>> +     },
>>  #endif
>>       {
>>       }
>> @@ -3129,6 +3151,9 @@ static void its_restore_enable(void)
>>                       }
>>                       writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>               }
>> +
>> +             if (its->flags & ITS_FLAGS_WORKAROUND_GIC500_MAPC)
>> +                     its_cpu_init_collection(its);
>>       }
>>       spin_unlock(&its_lock);
>>  }
>> @@ -3395,7 +3420,7 @@ int its_cpu_init(void)
>>                       return -ENXIO;
>>               }
>>               its_cpu_init_lpis();
>> -             its_cpu_init_collection();
>> +             its_cpu_init_collections();
>>       }
>>
>>       return 0;
>>
>
> I wonder if a better way to implement this is simply to bite the bullet
> and implement the letter of the architecture:
>
> If GITS_TYPER.HCC != 0, then collections 0...HCC-1 are held in HW, and
> the rest in a SW-managed table. That would allow us to deal with this
> without a quirk (which I asked for initially, apologies for changing my
> mind).
>
> We still the same DT flag to tell us we're going to be reset on suspend.

So this would just be under the generic reset-on-suspend flag that I
added in patch 2/5?

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

Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state

[dbasehore .]

On Tue, Feb 6, 2018 at 8:23 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 05/02/18 21:33, dbasehore . wrote:
>> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 03/02/18 01:24, Derek Basehore wrote:
>>>> Some platforms power off GIC logic in suspend, so we need to
>>>> save/restore state. The distributor and redistributor registers need
>>>> to be handled in platform code due to access permissions on those
>>>> registers, but the ITS registers can be restored in the kernel.
>>>>
>>>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 101 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>> index 06f025fd5726..e13515cdb68f 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -33,6 +33,7 @@
>>>>  #include <linux/of_platform.h>
>>>>  #include <linux/percpu.h>
>>>>  #include <linux/slab.h>
>>>> +#include <linux/syscore_ops.h>
>>>>
>>>>  #include <linux/irqchip.h>
>>>>  #include <linux/irqchip/arm-gic-v3.h>
>>>> @@ -46,6 +47,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_SAVE_SUSPEND_STATE         (1ULL << 3)
>>>>
>>>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)
>>>>
>>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>>       u32             psz;
>>>>  };
>>>>
>>>> +/*
>>>> + * 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;
>>>> +     u32                     ctlr;
>>>> +};
>>>
>>> Nit: This is pretty small for the ITS context. Given that its_node is
>>> the context, you can safely expand this in the its_node structure.
>>
>> Sounds reasonable. I think I just have it this way because I used to
>> have more in here.
>>
>>>
>>>> +
>>>>  struct its_device;
>>>>
>>>>  /*
>>>> @@ -101,6 +112,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 +3054,90 @@ static void its_enable_quirks(struct its_node *its)
>>>>       gic_enable_quirks(iidr, its_quirks, its);
>>>>  }
>>>>
>>>> +static int its_save_disable(void)
>>>> +{
>>>> +     struct its_node *its;
>>>> +     int err = 0;
>>>> +
>>>> +     spin_lock(&its_lock);
>>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>>> +             struct its_ctx *ctx;
>>>> +             void __iomem *base;
>>>> +
>>>> +             if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>>> +                     continue;
>>>> +
>>>> +             ctx = &its->its_ctx;
>>>> +             base = its->base;
>>>> +             ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>>> +             err = its_force_quiescent(base);
>>>> +             if (err) {
>>>> +                     pr_err("ITS failed to quiesce\n");
>>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>>> +                     goto err;
>>>> +             }
>>>> +
>>>> +             ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>>> +     }
>>>> +
>>>> +err:
>>>> +     if (err) {
>>>> +             list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>>>> +                     if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>>> +                             struct its_ctx *ctx = &its->its_ctx;
>>>> +                             void __iomem *base = its->base;
>>>> +
>>>> +                             writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>>> +                     }
>>>> +             }
>>>> +     }
>>>> +
>>>> +     spin_unlock(&its_lock);
>>>> +
>>>> +     return err;
>>>> +}
>>>> +
>>>> +static void its_restore_enable(void)
>>>> +{
>>>> +     struct its_node *its;
>>>> +
>>>> +     spin_lock(&its_lock);
>>>> +     list_for_each_entry(its, &its_nodes, entry) {
>>>> +             if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>>> +                     struct its_ctx *ctx = &its->its_ctx;
>>>> +                     void __iomem *base = its->base;
>>>> +                     /*
>>>> +                      * Only the lower 32 bits matter here since the upper 32
>>>> +                      * don't include any of the offset.
>>>> +                      */
>>>> +                     u32 creader = readl_relaxed(base + GITS_CREADR);
>>>
>>> Accessor matching gits_write_cwriter and co?
>>>
>>>> +                     int i;
>>>> +
>>>> +                     /*
>>>> +                      * Reset the write location to where the ITS is
>>>> +                      * currently at.
>>>> +                      */
>>>> +                     gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>>> +                     gits_write_cwriter(creader, base + GITS_CWRITER);
>>>> +                     its->cmd_write = &its->cmd_base[
>>>> +                             creader / sizeof(struct its_cmd_block)];
>>>
>>> Nit: please do not split lines like this, this is unreadable. We both
>>> have screens that are wide enough for this to fit on a single line.
>>>
>>> More importantly: Why isn't it sufficient to reset both CREADR and
>>> CWRITER to zero? Is there any case where you can suspend whilst having
>>> anything in flight?
>>
>> CREADR is RO and we need to handle the non-zero case. I was planning
>> on getting rid of the write to CWRITER since it shouldn't be needed.
>> Either CREADR and CWRITER have the prior values, or both are reset to
>> 0.
>
> You're writing GITS_CBASER, which has for consequence: "When this
> register is successfully written, the value of GITS_CREADR is set to
> zero.". Ergo, none of that is necessary and you *must* set CWRITER to 0.

Huh, I missed that. It at least makes this a lot simpler. That also
means that the current code has a potential bug if suspend is
preempted.

>
>>
>>>
>>>> +                     /* Restore GITS_BASER from the value cache. */
>>>> +                     for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>>>> +                             struct its_baser *baser = &its->tables[i];
>>>> +
>>>> +                             its_write_baser(its, baser, baser->val);
>>>
>>> You may want to first test that this BASER register is actually
>>> requiring something before writing to it. Yes, this is normally safe.
>>> But HW is also normally broken.
>>>
>>>> +                     }
>>>> +                     writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>>
>>> Before restoring all of this, shouldn't you first test that the ITS is
>>> actually in a disabled state?
>>
>> The save_disable code put it in a disabled state. The reset state is> also disabled. I don't expect PSCI to change the GITS_CTLR register at
>> all. Are you expecting something on the other side of the PSCI layer
>> to poke this register? We'd probably want to force disable at the
>> start of restore_enable if so.
>
> I expect firmware to do its worse, because even if mainline ATF is close
> to perfection, it will be hacked to death by platform people who usually
> do not have a clue (that's definitely my experience). So I expect this
> code to be written defensively.

Fair enough. I'll disable the ITS at the start of restore_enable.

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