[PATCH v3 0/4] edac: Add L3/SoC support to the APM X-Gene SoC EDAC driver

[PATCH v3 0/4] edac: Add L3/SoC support to the APM X-Gene SoC EDAC driver

[Loc Ho]

v3:
* Add an function to retrieve the EDAC debugfs node
* Move all debugfs node under EDAC code debugfs node
* Update L3 to check for v1 errata
* Rename error label for L3/SoC add routines
* Re-structure SoC EDAC functions for code readability
* Inline the function xgene_edac_soc_mem_data
* Remove un-necessary { }

v2:
* Update binding documentation accordingly
* Change all single bit defines to BIT(x)
* Add support for L3 version 1 and 2 HW's
* Change to use debug file system for error injection
* In L3/SoC instance add function, allocate EDAC context after all
  initalization successed
* Support raw or detail info for SoC EDAC error reporting

v1:
* Add L3/SoC support to the APM X-Gene SoC EDAC driver
---
Loc Ho (4):
  edac: Add an function to retrieve the EDAC debugfs node
  Documentation: Update the APM X-Gene SoC EDAC DTS binding for L3/SoC
    subnodes
  edac: Add L3/SoC EDAC support to the APM X-Gene SoC EDAC driver
  arm64: Add L3/SoC DT subnodes to the APM X-Gene SoC EDAC node

 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   23 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   10 +
 drivers/edac/edac_core.h                           |    1 +
 drivers/edac/edac_mc_sysfs.c                       |   10 +
 drivers/edac/xgene_edac.c                          |  827 +++++++++++++++++++-
 5 files changed, 859 insertions(+), 12 deletions(-)

--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Loc Ho]

This patch adds an function to retrieve the EDAC debugfs node. This allows
EDAC driver to create debugfs node under the EDAC debugfs node.

Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 drivers/edac/edac_core.h     |    1 +
 drivers/edac/edac_mc_sysfs.c |   10 ++++++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
index ad42587..6771e64 100644
--- a/drivers/edac/edac_core.h
+++ b/drivers/edac/edac_core.h
@@ -511,5 +511,6 @@ extern void edac_pci_remove_sysfs(struct edac_pci_ctl_info *pci);
  * edac misc APIs
  */
 extern char *edac_op_state_to_string(int op_state);
+extern struct dentry *edac_get_debugfs(void);
 
 #endif				/* _EDAC_CORE_H_ */
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 33df7d9..b76afe0 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -959,6 +959,16 @@ nomem:
 	debugfs_remove(mci->debugfs);
 	return -ENOMEM;
 }
+
+struct dentry *edac_get_debugfs(void)
+{
+	return edac_debugfs;
+}
+#else
+struct dentry *edac_get_debugfs(void)
+{
+	return NULL;
+}
 #endif
 
 /*
-- 
1.7.1

--
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 v3 2/4] Documentation: Update the APM X-Gene SoC EDAC DTS binding for L3/SoC subnodes

[Loc Ho]

This patch updates documentation for the APM X-Gene SoC EDAC DTS binding
for L3/SoC subnodes.

Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   23 ++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
index 78edb80..78e2a31 100644
--- a/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
+++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
@@ -5,6 +5,8 @@ The follow error types are supported:
 
   memory controller	- Memory controller
   PMD (L1/L2)		- Processor module unit (PMD) L1/L2 cache
+  L3			- L3 cache controller
+  SoC			- SoC IP's such as Ethernet, SATA, and etc
 
 The following section describes the EDAC DT node binding.
 
@@ -30,6 +32,17 @@ Required properties for PMD subnode:
 - reg			: First resource shall be the PMD resource.
 - pmd-controller	: Instance number of the PMD controller.
 
+Required properties for L3 subnode:
+- compatible		: Shall be "apm,xgene-edac-l3" or
+                          "apm,xgene-edac-l3-v2".
+- reg			: First resource shall be the L3 EDAC resource.
+
+Required properties for SoC subnode:
+- compatible		: Shall be "apm,xgene-edac-soc-v1" for revision 1 or
+                          "apm,xgene-edac-l3-soc" for general value reporting
+                          only.
+- reg			: First resource shall be the SoC EDAC resource.
+
 Example:
 	csw: csw@7e200000 {
 		compatible = "apm,xgene-csw", "syscon";
@@ -76,4 +89,14 @@ Example:
 			reg = <0x0 0x7c000000 0x0 0x200000>;
 			pmd-controller = <0>;
 		};
+
+		edacl3@7e600000 {
+			compatible = "apm,xgene-edac-l3";
+			reg = <0x0 0x7e600000 0x0 0x1000>;
+		};
+
+		edacsoc@7e930000 {
+			compatible = "apm,xgene-edac-soc-v1";
+			reg = <0x0 0x7e930000 0x0 0x1000>;
+		};
 	};
-- 
1.7.1

--
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 v3 3/4] edac: Add L3/SoC EDAC support to the APM X-Gene SoC EDAC driver

[Loc Ho]

This patch adds EDAC support for the L3 and SoC components.

Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 drivers/edac/xgene_edac.c |  827 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 815 insertions(+), 12 deletions(-)

diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
index ba06904..d5e33bf 100644
--- a/drivers/edac/xgene_edac.c
+++ b/drivers/edac/xgene_edac.c
@@ -66,6 +66,8 @@ struct xgene_edac {
 
 	struct list_head	mcus;
 	struct list_head	pmds;
+	struct list_head	l3s;
+	struct list_head	socs;
 
 	struct mutex		mc_lock;
 	int			mc_active_mask;
@@ -877,8 +879,8 @@ static const struct file_operations xgene_edac_pmd_debug_inject_fops[] = {
 	{ }
 };
 
-static void xgene_edac_pmd_create_debugfs_nodes(
-	struct edac_device_ctl_info *edac_dev)
+static void
+xgene_edac_pmd_create_debugfs_nodes(struct edac_device_ctl_info *edac_dev)
 {
 	struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info;
 	struct dentry *edac_debugfs;
@@ -887,10 +889,6 @@ static void xgene_edac_pmd_create_debugfs_nodes(
 	if (!IS_ENABLED(CONFIG_EDAC_DEBUG))
 		return;
 
-	/*
-	 * Todo: Switch to common EDAC debug file system for edac device
-	 *       when available.
-	 */
 	if (!ctx->edac->dfs) {
 		ctx->edac->dfs = debugfs_create_dir(edac_dev->dev->kobj.name,
 						    NULL);
@@ -1016,10 +1014,791 @@ static int xgene_edac_pmd_remove(struct xgene_edac_pmd_ctx *pmd)
 	return 0;
 }
 
+/* L3 Error device */
+#define L3C_ESR				(0x0A * 4)
+#define  L3C_ESR_DATATAG_MASK		BIT(9)
+#define  L3C_ESR_MULTIHIT_MASK		BIT(8)
+#define  L3C_ESR_UCEVICT_MASK		BIT(6)
+#define  L3C_ESR_MULTIUCERR_MASK	BIT(5)
+#define  L3C_ESR_MULTICERR_MASK		BIT(4)
+#define  L3C_ESR_UCERR_MASK		BIT(3)
+#define  L3C_ESR_CERR_MASK		BIT(2)
+#define  L3C_ESR_UCERRINTR_MASK		BIT(1)
+#define  L3C_ESR_CERRINTR_MASK		BIT(0)
+#define L3C_ECR				(0x0B * 4)
+#define  L3C_ECR_UCINTREN		BIT(3)
+#define  L3C_ECR_CINTREN		BIT(2)
+#define  L3C_UCERREN			BIT(1)
+#define  L3C_CERREN			BIT(0)
+#define L3C_ELR				(0x0C * 4)
+#define  L3C_ELR_ERRSYN(src)		((src & 0xFF800000) >> 23)
+#define  L3C_ELR_ERRWAY(src)		((src & 0x007E0000) >> 17)
+#define  L3C_ELR_AGENTID(src)		((src & 0x0001E000) >> 13)
+#define  L3C_ELR_ERRGRP(src)		((src & 0x00000F00) >> 8)
+#define  L3C_ELR_OPTYPE(src)		((src & 0x000000F0) >> 4)
+#define  L3C_ELR_PADDRHIGH(src)		(src & 0x0000000F)
+#define L3C_AELR			(0x0D * 4)
+#define L3C_BELR			(0x0E * 4)
+#define  L3C_BELR_BANK(src)		(src & 0x0000000F)
+
+struct xgene_edac_dev_ctx {
+	struct list_head	next;
+	struct device		ddev;
+	char			*name;
+	struct xgene_edac	*edac;
+	struct edac_device_ctl_info *edac_dev;
+	int			edac_idx;
+	void __iomem		*dev_csr;
+	int			version;
+};
+
+static bool xgene_edac_l3_v1_errata_chk(u32 l3cesr, u32 l3celr)
+{
+	/*
+	 * L3 version 1 has certain conditions in which correctable error
+	 * needs to be flagged as un-correctable error. This function
+	 * check for such conditions.
+	 */
+	if (l3cesr & L3C_ESR_DATATAG_MASK) {
+		switch (L3C_ELR_ERRSYN(l3celr)) {
+		case 0x13C:
+		case 0x0B4:
+		case 0x007:
+		case 0x00D:
+		case 0x00E:
+		case 0x019:
+		case 0x01A:
+		case 0x01C:
+		case 0x04E:
+		case 0x041:
+			return true;
+		}
+	} else if (L3C_ELR_ERRSYN(l3celr) == 9) {
+		return true;
+	}
+
+	return false;
+}
+
+static void xgene_edac_l3_check(struct edac_device_ctl_info *edac_dev)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	u32 l3cesr;
+	u32 l3celr;
+	u32 l3caelr;
+	u32 l3cbelr;
+
+	l3cesr = readl(ctx->dev_csr + L3C_ESR);
+	if (!(l3cesr & (L3C_ESR_UCERR_MASK | L3C_ESR_CERR_MASK)))
+		return;
+
+	if (l3cesr & L3C_ESR_UCERR_MASK)
+		dev_err(edac_dev->dev, "L3C uncorrectable error\n");
+	if (l3cesr & L3C_ESR_CERR_MASK)
+		dev_warn(edac_dev->dev, "L3C correctable error\n");
+
+	l3celr = readl(ctx->dev_csr + L3C_ELR);
+	l3caelr = readl(ctx->dev_csr + L3C_AELR);
+	l3cbelr = readl(ctx->dev_csr + L3C_BELR);
+	if (l3cesr & L3C_ESR_MULTIHIT_MASK)
+		dev_err(edac_dev->dev, "L3C multiple hit error\n");
+	if (l3cesr & L3C_ESR_UCEVICT_MASK)
+		dev_err(edac_dev->dev,
+			"L3C dropped eviction of line with error\n");
+	if (l3cesr & L3C_ESR_MULTIUCERR_MASK)
+		dev_err(edac_dev->dev, "L3C multiple uncorrectable error\n");
+	if (l3cesr & L3C_ESR_DATATAG_MASK)
+		dev_err(edac_dev->dev,
+			"L3C data error syndrome 0x%X group 0x%X\n",
+			L3C_ELR_ERRSYN(l3celr), L3C_ELR_ERRGRP(l3celr));
+	else
+		dev_err(edac_dev->dev,
+			"L3C tag error syndrome 0x%X Way of Tag 0x%X Agent ID 0x%X Operation type 0x%X\n",
+			L3C_ELR_ERRSYN(l3celr), L3C_ELR_ERRWAY(l3celr),
+			L3C_ELR_AGENTID(l3celr), L3C_ELR_OPTYPE(l3celr));
+	/*
+	 * NOTE: Address [41:38] in L3C_ELR_PADDRHIGH(l3celr).
+	 *       Address [37:6] in l3caelr. Lower 6 bits are zero.
+	 */
+	dev_err(edac_dev->dev, "L3C error address 0x%08X.%08X bank %d\n",
+		L3C_ELR_PADDRHIGH(l3celr) << 6 | (l3caelr >> 26),
+		(l3caelr & 0x3FFFFFFF) << 6, L3C_BELR_BANK(l3cbelr));
+	dev_err(edac_dev->dev,
+		"L3C error status register value 0x%X\n", l3cesr);
+
+	/* Clear L3C error interrupt */
+	writel(0, ctx->dev_csr + L3C_ESR);
+
+	if (ctx->version <= 1 &&
+	    xgene_edac_l3_v1_errata_chk(l3cesr, l3celr)) {
+		/*
+		 * Version 1 of the L3C has broken single bit correctable
+		 * logic for certain condition. When such condition is
+		 * detected, map them as uncorrectable.
+		 */
+		edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
+		return;
+	}
+	if (l3cesr & L3C_ESR_CERR_MASK)
+		edac_device_handle_ce(edac_dev, 0, 0, edac_dev->ctl_name);
+	if (l3cesr & L3C_ESR_UCERR_MASK)
+		edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
+}
+
+static void xgene_edac_l3_hw_init(struct edac_device_ctl_info *edac_dev,
+				  bool enable)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	u32 val;
+
+	val = readl(ctx->dev_csr + L3C_ECR);
+	val |= L3C_UCERREN | L3C_CERREN;
+	/* On disable, we just disable interrupt but keep error enabled */
+	if (edac_dev->op_state == OP_RUNNING_INTERRUPT) {
+		if (enable)
+			val |= L3C_ECR_UCINTREN | L3C_ECR_CINTREN;
+		else
+			val &= ~(L3C_ECR_UCINTREN | L3C_ECR_CINTREN);
+	}
+	writel(val, ctx->dev_csr + L3C_ECR);
+
+	if (edac_dev->op_state == OP_RUNNING_INTERRUPT) {
+		/* Enable/disable L3 error top level interrupt */
+		if (enable) {
+			xgene_edac_pcp_clrbits(ctx->edac, PCPHPERRINTMSK,
+					       L3C_UNCORR_ERR_MASK);
+			xgene_edac_pcp_clrbits(ctx->edac, PCPLPERRINTMSK,
+					       L3C_CORR_ERR_MASK);
+		} else {
+			xgene_edac_pcp_setbits(ctx->edac, PCPHPERRINTMSK,
+					       L3C_UNCORR_ERR_MASK);
+			xgene_edac_pcp_setbits(ctx->edac, PCPLPERRINTMSK,
+					       L3C_CORR_ERR_MASK);
+		}
+	}
+}
+
+static ssize_t xgene_edac_l3_inject_ctrl_write(struct file *file,
+					       const char __user *data,
+					       size_t count, loff_t *ppos)
+{
+	struct edac_device_ctl_info *edac_dev = file->private_data;
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+
+	/* Generate all errors */
+	writel(0xFFFFFFFF, ctx->dev_csr + L3C_ESR);
+	return count;
+}
+
+static const struct file_operations xgene_edac_l3_debug_inject_fops = {
+	.open = simple_open,
+	.write = xgene_edac_l3_inject_ctrl_write,
+	.llseek = generic_file_llseek
+};
+
+static void xgene_edac_l3_create_debugfs_nodes(
+	struct edac_device_ctl_info *edac_dev)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	struct dentry *edac_debugfs;
+	char name[30];
+
+	if (!IS_ENABLED(CONFIG_EDAC_DEBUG))
+		return;
+
+	if (!ctx->edac->dfs) {
+		ctx->edac->dfs = debugfs_create_dir(edac_dev->dev->kobj.name,
+						    NULL);
+		if (!ctx->edac->dfs)
+			return;
+	}
+	sprintf(name, "l3c%d", ctx->edac_idx);
+	edac_debugfs = debugfs_create_dir(name, ctx->edac->dfs);
+	if (!edac_debugfs)
+		return;
+
+	debugfs_create_file("l3_inject_ctrl", S_IWUSR, edac_debugfs, edac_dev,
+			    &xgene_edac_l3_debug_inject_fops);
+}
+
+static int xgene_edac_l3_add(struct xgene_edac *edac, struct device_node *np,
+			     int version)
+{
+	struct edac_device_ctl_info *edac_dev;
+	struct xgene_edac_dev_ctx *ctx;
+	struct resource res;
+	void __iomem *dev_csr;
+	int edac_idx;
+	int rc = 0;
+
+	if (!devres_open_group(edac->dev, xgene_edac_l3_add, GFP_KERNEL))
+		return -ENOMEM;
+
+	rc = of_address_to_resource(np, 0, &res);
+	if (rc < 0) {
+		dev_err(edac->dev, "no L3 resource address\n");
+		goto err_release_group;
+	}
+	dev_csr = devm_ioremap_resource(edac->dev, &res);
+	if (IS_ERR(dev_csr)) {
+		dev_err(edac->dev,
+			"devm_ioremap_resource failed for L3 resource address\n");
+		rc = PTR_ERR(dev_csr);
+		goto err_release_group;
+	}
+
+	edac_idx = edac_device_alloc_index();
+	edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx),
+					      "l3c", 1, "l3c", 1, 0, NULL, 0,
+					      edac_idx);
+	if (!edac_dev) {
+		rc = -ENOMEM;
+		goto err_release_group;
+	}
+
+	ctx = edac_dev->pvt_info;
+	ctx->dev_csr = dev_csr;
+	ctx->name = "xgene_l3_err";
+	ctx->edac_idx = edac_idx;
+	ctx->edac = edac;
+	ctx->edac_dev = edac_dev;
+	ctx->ddev = *edac->dev;
+	ctx->version = version;
+	edac_dev->dev = &ctx->ddev;
+	edac_dev->ctl_name = ctx->name;
+	edac_dev->dev_name = ctx->name;
+	edac_dev->mod_name = EDAC_MOD_STR;
+
+	if (edac_op_state == EDAC_OPSTATE_POLL)
+		edac_dev->edac_check = xgene_edac_l3_check;
+
+	xgene_edac_l3_create_debugfs_nodes(edac_dev);
+
+	rc = edac_device_add_device(edac_dev);
+	if (rc > 0) {
+		dev_err(edac->dev, "failed edac_device_add_device()\n");
+		rc = -ENOMEM;
+		goto err_ctl_free;
+	}
+
+	if (edac_op_state == EDAC_OPSTATE_INT)
+		edac_dev->op_state = OP_RUNNING_INTERRUPT;
+
+	list_add(&ctx->next, &edac->l3s);
+
+	xgene_edac_l3_hw_init(edac_dev, 1);
+
+	devres_remove_group(edac->dev, xgene_edac_l3_add);
+
+	dev_info(edac->dev, "X-Gene EDAC L3 registered\n");
+	return 0;
+
+err_ctl_free:
+	edac_device_free_ctl_info(edac_dev);
+err_release_group:
+	devres_release_group(edac->dev, xgene_edac_l3_add);
+	return rc;
+}
+
+static int xgene_edac_l3_remove(struct xgene_edac_dev_ctx *l3)
+{
+	struct edac_device_ctl_info *edac_dev = l3->edac_dev;
+
+	xgene_edac_l3_hw_init(edac_dev, 0);
+	edac_device_del_device(l3->edac->dev);
+	edac_device_free_ctl_info(edac_dev);
+	return 0;
+}
+
+/* SoC Error device */
+#define IOBAXIS0TRANSERRINTSTS		0x0000
+#define  IOBAXIS0_M_ILLEGAL_ACCESS_MASK	BIT(1)
+#define  IOBAXIS0_ILLEGAL_ACCESS_MASK	BIT(0)
+#define IOBAXIS0TRANSERRINTMSK		0x0004
+#define IOBAXIS0TRANSERRREQINFOL	0x0008
+#define IOBAXIS0TRANSERRREQINFOH	0x000c
+#define  REQTYPE_RD(src)		(((src) & BIT(0)))
+#define  ERRADDRH_RD(src)		(((src) & 0xffc00000) >> 22)
+#define IOBAXIS1TRANSERRINTSTS		0x0010
+#define IOBAXIS1TRANSERRINTMSK		0x0014
+#define IOBAXIS1TRANSERRREQINFOL	0x0018
+#define IOBAXIS1TRANSERRREQINFOH	0x001c
+#define IOBPATRANSERRINTSTS		0x0020
+#define  IOBPA_M_REQIDRAM_CORRUPT_MASK	BIT(7)
+#define  IOBPA_REQIDRAM_CORRUPT_MASK	BIT(6)
+#define  IOBPA_M_TRANS_CORRUPT_MASK	BIT(5)
+#define  IOBPA_TRANS_CORRUPT_MASK	BIT(4)
+#define  IOBPA_M_WDATA_CORRUPT_MASK	BIT(3)
+#define  IOBPA_WDATA_CORRUPT_MASK	BIT(2)
+#define  IOBPA_M_RDATA_CORRUPT_MASK	BIT(1)
+#define  IOBPA_RDATA_CORRUPT_MASK	BIT(0)
+#define IOBBATRANSERRINTSTS		0x0030
+#define  M_ILLEGAL_ACCESS_MASK		BIT(15)
+#define  ILLEGAL_ACCESS_MASK		BIT(14)
+#define  M_WIDRAM_CORRUPT_MASK		BIT(13)
+#define  WIDRAM_CORRUPT_MASK		BIT(12)
+#define  M_RIDRAM_CORRUPT_MASK		BIT(11)
+#define  RIDRAM_CORRUPT_MASK		BIT(10)
+#define  M_TRANS_CORRUPT_MASK		BIT(9)
+#define  TRANS_CORRUPT_MASK		BIT(8)
+#define  M_WDATA_CORRUPT_MASK		BIT(7)
+#define  WDATA_CORRUPT_MASK		BIT(6)
+#define  M_RBM_POISONED_REQ_MASK	BIT(5)
+#define  RBM_POISONED_REQ_MASK		BIT(4)
+#define  M_XGIC_POISONED_REQ_MASK	BIT(3)
+#define  XGIC_POISONED_REQ_MASK		BIT(2)
+#define  M_WRERR_RESP_MASK		BIT(1)
+#define  WRERR_RESP_MASK		BIT(0)
+#define IOBBATRANSERRREQINFOL		0x0038
+#define IOBBATRANSERRREQINFOH		0x003c
+#define  REQTYPE_F2_RD(src)		((src) & BIT(0))
+#define  ERRADDRH_F2_RD(src)		(((src) & 0xffc00000) >> 22)
+#define IOBBATRANSERRCSWREQID		0x0040
+#define XGICTRANSERRINTSTS		0x0050
+#define  M_WR_ACCESS_ERR_MASK		BIT(3)
+#define  WR_ACCESS_ERR_MASK		BIT(2)
+#define  M_RD_ACCESS_ERR_MASK		BIT(1)
+#define  RD_ACCESS_ERR_MASK		BIT(0)
+#define XGICTRANSERRINTMSK		0x0054
+#define XGICTRANSERRREQINFO		0x0058
+#define  REQTYPE_MASK			BIT(26)
+#define  ERRADDR_RD(src)		((src) & 0x03ffffff)
+#define GLBL_ERR_STS			0x0800
+#define  MDED_ERR_MASK			BIT(3)
+#define  DED_ERR_MASK			BIT(2)
+#define  MSEC_ERR_MASK			BIT(1)
+#define  SEC_ERR_MASK			BIT(0)
+#define GLBL_SEC_ERRL			0x0810
+#define GLBL_SEC_ERRH			0x0818
+#define GLBL_MSEC_ERRL			0x0820
+#define GLBL_MSEC_ERRH			0x0828
+#define GLBL_DED_ERRL			0x0830
+#define GLBL_DED_ERRLMASK		0x0834
+#define GLBL_DED_ERRH			0x0838
+#define GLBL_DED_ERRHMASK		0x083c
+#define GLBL_MDED_ERRL			0x0840
+#define GLBL_MDED_ERRLMASK		0x0844
+#define GLBL_MDED_ERRH			0x0848
+#define GLBL_MDED_ERRHMASK		0x084c
+
+static const char * const soc_mem_err_v1[] = {
+	"10GbE0",
+	"10GbE1",
+	"Security",
+	"SATA45",
+	"SATA23/ETH23",
+	"SATA01/ETH01",
+	"USB1",
+	"USB0",
+	"QML",
+	"QM0",
+	"QM1 (XGbE01)",
+	"PCIE4",
+	"PCIE3",
+	"PCIE2",
+	"PCIE1",
+	"PCIE0",
+	"CTX Manager",
+	"OCM",
+	"1GbE",
+	"CLE",
+	"AHBC",
+	"PktDMA",
+	"GFC",
+	"MSLIM",
+	"10GbE2",
+	"10GbE3",
+	"QM2 (XGbE23)",
+	"IOB",
+	"unknown",
+	"unknown",
+	"unknown",
+	"unknown",
+};
+
+static void xgene_edac_iob_gic_report(struct edac_device_ctl_info *edac_dev)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	u32 err_addr_lo;
+	u32 err_addr_hi;
+	u32 reg;
+	u32 info;
+
+	/* GIC transaction error interrupt */
+	reg = readl(ctx->dev_csr + XGICTRANSERRINTSTS);
+	if (!reg)
+		goto chk_iob_err;
+	dev_err(edac_dev->dev, "XGIC transaction error\n");
+	if (reg & RD_ACCESS_ERR_MASK)
+		dev_err(edac_dev->dev, "XGIC read size error\n");
+	if (reg & M_RD_ACCESS_ERR_MASK)
+		dev_err(edac_dev->dev, "Multiple XGIC read size error\n");
+	if (reg & WR_ACCESS_ERR_MASK)
+		dev_err(edac_dev->dev, "XGIC write size error\n");
+	if (reg & M_WR_ACCESS_ERR_MASK)
+		dev_err(edac_dev->dev, "Multiple XGIC write size error\n");
+	info = readl(ctx->dev_csr + XGICTRANSERRREQINFO);
+	dev_err(edac_dev->dev, "XGIC %s access @ 0x%08X (0x%08X)\n",
+		info & REQTYPE_MASK ? "read" : "write", ERRADDR_RD(info),
+		info);
+	writel(reg, ctx->dev_csr + XGICTRANSERRINTSTS);
+
+chk_iob_err:
+	/* IOB memory error */
+	reg = readl(ctx->dev_csr + GLBL_ERR_STS);
+	if (!reg)
+		return;
+	if (reg & SEC_ERR_MASK) {
+		err_addr_lo = readl(ctx->dev_csr + GLBL_SEC_ERRL);
+		err_addr_hi = readl(ctx->dev_csr + GLBL_SEC_ERRH);
+		dev_err(edac_dev->dev,
+			"IOB single-bit correctable memory at 0x%08X.%08X error\n",
+			err_addr_lo, err_addr_hi);
+		writel(err_addr_lo, ctx->dev_csr + GLBL_SEC_ERRL);
+		writel(err_addr_hi, ctx->dev_csr + GLBL_SEC_ERRH);
+	}
+	if (reg & MSEC_ERR_MASK) {
+		err_addr_lo = readl(ctx->dev_csr + GLBL_MSEC_ERRL);
+		err_addr_hi = readl(ctx->dev_csr + GLBL_MSEC_ERRH);
+		dev_err(edac_dev->dev,
+			"IOB multiple single-bit correctable memory at 0x%08X.%08X error\n",
+			err_addr_lo, err_addr_hi);
+		writel(err_addr_lo, ctx->dev_csr + GLBL_MSEC_ERRL);
+		writel(err_addr_hi, ctx->dev_csr + GLBL_MSEC_ERRH);
+	}
+	if (reg & (SEC_ERR_MASK | MSEC_ERR_MASK))
+		edac_device_handle_ce(edac_dev, 0, 0, edac_dev->ctl_name);
+
+	if (reg & DED_ERR_MASK) {
+		err_addr_lo = readl(ctx->dev_csr + GLBL_DED_ERRL);
+		err_addr_hi = readl(ctx->dev_csr + GLBL_DED_ERRH);
+		dev_err(edac_dev->dev,
+			"IOB double-bit uncorrectable memory at 0x%08X.%08X error\n",
+			err_addr_lo, err_addr_hi);
+		writel(err_addr_lo, ctx->dev_csr + GLBL_DED_ERRL);
+		writel(err_addr_hi, ctx->dev_csr + GLBL_DED_ERRH);
+	}
+	if (reg & MDED_ERR_MASK) {
+		err_addr_lo = readl(ctx->dev_csr + GLBL_MDED_ERRL);
+		err_addr_hi = readl(ctx->dev_csr + GLBL_MDED_ERRH);
+		dev_err(edac_dev->dev,
+			"Multiple IOB double-bit uncorrectable memory at 0x%08X.%08X error\n",
+			err_addr_lo, err_addr_hi);
+		writel(err_addr_lo, ctx->dev_csr + GLBL_MDED_ERRL);
+		writel(err_addr_hi, ctx->dev_csr + GLBL_MDED_ERRH);
+	}
+	if (reg & (DED_ERR_MASK | MDED_ERR_MASK))
+		edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
+}
+
+static void xgene_edac_rb_report(struct edac_device_ctl_info *edac_dev)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	u32 err_addr_lo;
+	u32 err_addr_hi;
+	u32 reg;
+
+	/* IOB Bridge agent transaction error interrupt */
+	reg = readl(ctx->dev_csr + IOBBATRANSERRINTSTS);
+	if (!reg)
+		return;
+
+	dev_err(edac_dev->dev, "IOB bridge agent (BA) transaction error\n");
+	if (reg & WRERR_RESP_MASK)
+		dev_err(edac_dev->dev, "IOB BA write response error\n");
+	if (reg & M_WRERR_RESP_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB BA write response error\n");
+	if (reg & XGIC_POISONED_REQ_MASK)
+		dev_err(edac_dev->dev, "IOB BA XGIC poisoned write error\n");
+	if (reg & M_XGIC_POISONED_REQ_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB BA XGIC poisoned write error\n");
+	if (reg & RBM_POISONED_REQ_MASK)
+		dev_err(edac_dev->dev, "IOB BA RBM poisoned write error\n");
+	if (reg & M_RBM_POISONED_REQ_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB BA RBM poisoned write error\n");
+	if (reg & WDATA_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "IOB BA write error\n");
+	if (reg & M_WDATA_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "Multiple IOB BA write error\n");
+	if (reg & TRANS_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "IOB BA transaction error\n");
+	if (reg & M_TRANS_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "Multiple IOB BA transaction error\n");
+	if (reg & RIDRAM_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"IOB BA RDIDRAM read transaction ID error\n");
+	if (reg & M_RIDRAM_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB BA RDIDRAM read transaction ID error\n");
+	if (reg & WIDRAM_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"IOB BA RDIDRAM write transaction ID error\n");
+	if (reg & M_WIDRAM_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB BA RDIDRAM write transaction ID error\n");
+	if (reg & ILLEGAL_ACCESS_MASK)
+		dev_err(edac_dev->dev,
+			"IOB BA XGIC/RB illegal access error\n");
+	if (reg & M_ILLEGAL_ACCESS_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB BA XGIC/RB illegal access error\n");
+
+	err_addr_lo = readl(ctx->dev_csr + IOBBATRANSERRREQINFOL);
+	err_addr_hi = readl(ctx->dev_csr + IOBBATRANSERRREQINFOH);
+	dev_err(edac_dev->dev, "IOB BA %s access at 0x%02X.%08X (0x%08X)\n",
+		REQTYPE_F2_RD(err_addr_hi) ? "read" : "write",
+		ERRADDRH_F2_RD(err_addr_hi), err_addr_lo, err_addr_hi);
+	if (reg & WRERR_RESP_MASK)
+		dev_err(edac_dev->dev, "IOB BA requestor ID 0x%08X\n",
+			readl(ctx->dev_csr + IOBBATRANSERRCSWREQID));
+	writel(reg, ctx->dev_csr + IOBBATRANSERRINTSTS);
+}
+
+static void xgene_edac_pa_report(struct edac_device_ctl_info *edac_dev)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	u32 err_addr_lo;
+	u32 err_addr_hi;
+	u32 reg;
+
+	/* IOB Processing agent transaction error interrupt */
+	reg = readl(ctx->dev_csr + IOBPATRANSERRINTSTS);
+	if (!reg)
+		goto chk_iob_axi0;
+	dev_err(edac_dev->dev, "IOB procesing agent (PA) transaction error\n");
+	if (reg & IOBPA_RDATA_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "IOB PA read data RAM error\n");
+	if (reg & IOBPA_M_RDATA_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"Mutilple IOB PA read data RAM error\n");
+	if (reg & IOBPA_WDATA_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "IOB PA write data RAM error\n");
+	if (reg & IOBPA_M_WDATA_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"Mutilple IOB PA write data RAM error\n");
+	if (reg & IOBPA_TRANS_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "IOB PA transaction error\n");
+	if (reg & IOBPA_M_TRANS_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "Mutilple IOB PA transaction error\n");
+	if (reg & IOBPA_REQIDRAM_CORRUPT_MASK)
+		dev_err(edac_dev->dev, "IOB PA transaction ID RAM error\n");
+	if (reg & IOBPA_M_REQIDRAM_CORRUPT_MASK)
+		dev_err(edac_dev->dev,
+			"Multiple IOB PA transaction ID RAM error\n");
+	writel(reg, ctx->dev_csr + IOBPATRANSERRINTSTS);
+
+chk_iob_axi0:
+	/* IOB AXI0 Error */
+	reg = readl(ctx->dev_csr + IOBAXIS0TRANSERRINTSTS);
+	if (!reg)
+		goto chk_iob_axi1;
+	err_addr_lo = readl(ctx->dev_csr + IOBAXIS0TRANSERRREQINFOL);
+	err_addr_hi = readl(ctx->dev_csr + IOBAXIS0TRANSERRREQINFOH);
+	dev_err(edac_dev->dev,
+		"%sAXI slave 0 illegal %s access @ 0x%02X.%08X (0x%08X)\n",
+		reg & IOBAXIS0_M_ILLEGAL_ACCESS_MASK ? "Multiple " : "",
+		REQTYPE_RD(err_addr_hi) ? "read" : "write",
+		ERRADDRH_RD(err_addr_hi), err_addr_lo, err_addr_hi);
+	writel(reg, ctx->dev_csr + IOBAXIS0TRANSERRINTSTS);
+
+chk_iob_axi1:
+	/* IOB AXI1 Error */
+	reg = readl(ctx->dev_csr + IOBAXIS1TRANSERRINTSTS);
+	if (!reg)
+		return;
+	err_addr_lo = readl(ctx->dev_csr + IOBAXIS1TRANSERRREQINFOL);
+	err_addr_hi = readl(ctx->dev_csr + IOBAXIS1TRANSERRREQINFOH);
+	dev_err(edac_dev->dev,
+		"%sAXI slave 1 illegal %s access @ 0x%02X.%08X (0x%08X)\n",
+		reg & IOBAXIS0_M_ILLEGAL_ACCESS_MASK ? "Multiple " : "",
+		REQTYPE_RD(err_addr_hi) ? "read" : "write",
+		ERRADDRH_RD(err_addr_hi), err_addr_lo, err_addr_hi);
+	writel(reg, ctx->dev_csr + IOBAXIS1TRANSERRINTSTS);
+}
+
+static void xgene_edac_soc_check(struct edac_device_ctl_info *edac_dev)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+	const char * const *soc_mem_err = NULL;
+	u32 pcp_hp_stat;
+	u32 pcp_lp_stat;
+	u32 reg;
+	int i;
+
+	xgene_edac_pcp_rd(ctx->edac, PCPHPERRINTSTS, &pcp_hp_stat);
+	xgene_edac_pcp_rd(ctx->edac, PCPLPERRINTSTS, &pcp_lp_stat);
+	xgene_edac_pcp_rd(ctx->edac, MEMERRINTSTS, &reg);
+	if (!((pcp_hp_stat & (IOB_PA_ERR_MASK | IOB_BA_ERR_MASK |
+			     IOB_XGIC_ERR_MASK | IOB_RB_ERR_MASK)) ||
+	      (pcp_lp_stat & CSW_SWITCH_TRACE_ERR_MASK) || reg))
+		return;
+
+	if (pcp_hp_stat & IOB_XGIC_ERR_MASK)
+		xgene_edac_iob_gic_report(edac_dev);
+
+	if (pcp_hp_stat & (IOB_RB_ERR_MASK | IOB_BA_ERR_MASK))
+		xgene_edac_rb_report(edac_dev);
+
+	if (pcp_hp_stat & IOB_PA_ERR_MASK)
+		xgene_edac_pa_report(edac_dev);
+
+	if (pcp_lp_stat & CSW_SWITCH_TRACE_ERR_MASK) {
+		dev_info(edac_dev->dev,
+			 "CSW switch trace correctable memory parity error\n");
+		edac_device_handle_ce(edac_dev, 0, 0, edac_dev->ctl_name);
+	}
+
+	if (ctx->version == 1)
+		soc_mem_err = soc_mem_err_v1;
+	if (!soc_mem_err) {
+		dev_err(edac_dev->dev, "SoC memory parity error 0x%08X\n",
+			reg);
+		edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
+		return;
+	}
+	for (i = 0; i < 31; i++) {
+		if (reg & (1 << i)) {
+			dev_err(edac_dev->dev, "%s memory parity error\n",
+				soc_mem_err[i]);
+			edac_device_handle_ue(edac_dev, 0, 0,
+					      edac_dev->ctl_name);
+		}
+	}
+}
+
+static void xgene_edac_soc_hw_init(struct edac_device_ctl_info *edac_dev,
+				   bool enable)
+{
+	struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info;
+
+	/* Enable SoC IP error interrupt */
+	if (edac_dev->op_state == OP_RUNNING_INTERRUPT) {
+		if (enable) {
+			xgene_edac_pcp_clrbits(ctx->edac, PCPHPERRINTMSK,
+					       IOB_PA_ERR_MASK |
+					       IOB_BA_ERR_MASK |
+					       IOB_XGIC_ERR_MASK |
+					       IOB_RB_ERR_MASK);
+			xgene_edac_pcp_clrbits(ctx->edac, PCPLPERRINTMSK,
+					       CSW_SWITCH_TRACE_ERR_MASK);
+		} else {
+			xgene_edac_pcp_setbits(ctx->edac, PCPHPERRINTMSK,
+					       IOB_PA_ERR_MASK |
+					       IOB_BA_ERR_MASK |
+					       IOB_XGIC_ERR_MASK |
+					       IOB_RB_ERR_MASK);
+			xgene_edac_pcp_setbits(ctx->edac, PCPLPERRINTMSK,
+					       CSW_SWITCH_TRACE_ERR_MASK);
+		}
+
+		writel(enable ? 0x0 : 0xFFFFFFFF,
+		       ctx->dev_csr + IOBAXIS0TRANSERRINTMSK);
+		writel(enable ? 0x0 : 0xFFFFFFFF,
+		       ctx->dev_csr + IOBAXIS1TRANSERRINTMSK);
+		writel(enable ? 0x0 : 0xFFFFFFFF,
+		       ctx->dev_csr + XGICTRANSERRINTMSK);
+
+		xgene_edac_pcp_setbits(ctx->edac, MEMERRINTMSK,
+				       enable ? 0x0 : 0xFFFFFFFF);
+	}
+}
+
+static int xgene_edac_soc_add(struct xgene_edac *edac, struct device_node *np,
+			      int version)
+{
+	struct edac_device_ctl_info *edac_dev;
+	struct xgene_edac_dev_ctx *ctx;
+	void __iomem *dev_csr;
+	struct resource res;
+	int edac_idx;
+	int rc = 0;
+
+	if (!devres_open_group(edac->dev, xgene_edac_soc_add, GFP_KERNEL))
+		return -ENOMEM;
+
+	rc = of_address_to_resource(np, 0, &res);
+	if (rc < 0) {
+		dev_err(edac->dev, "no SoC resource address\n");
+		goto err_release_group;
+	}
+	dev_csr = devm_ioremap_resource(edac->dev, &res);
+	if (IS_ERR(dev_csr)) {
+		dev_err(edac->dev,
+			"devm_ioremap_resource failed for soc resource address\n");
+		rc = PTR_ERR(dev_csr);
+		goto err_release_group;
+	}
+
+	edac_idx = edac_device_alloc_index();
+	edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx),
+					      "SOC", 1, "SOC", 1, 2, NULL, 0,
+					      edac_idx);
+	if (!edac_dev) {
+		rc = -ENOMEM;
+		goto err_release_group;
+	}
+
+	ctx = edac_dev->pvt_info;
+	ctx->dev_csr = dev_csr;
+	ctx->name = "xgene_soc_err";
+	ctx->edac_idx = edac_idx;
+	ctx->edac = edac;
+	ctx->edac_dev = edac_dev;
+	ctx->ddev = *edac->dev;
+	ctx->version = version;
+	edac_dev->dev = &ctx->ddev;
+	edac_dev->ctl_name = ctx->name;
+	edac_dev->dev_name = ctx->name;
+	edac_dev->mod_name = EDAC_MOD_STR;
+
+	if (edac_op_state == EDAC_OPSTATE_POLL)
+		edac_dev->edac_check = xgene_edac_soc_check;
+
+	rc = edac_device_add_device(edac_dev);
+	if (rc > 0) {
+		dev_err(edac->dev, "failed edac_device_add_device()\n");
+		rc = -ENOMEM;
+		goto err_ctl_free;
+	}
+
+	if (edac_op_state == EDAC_OPSTATE_INT)
+		edac_dev->op_state = OP_RUNNING_INTERRUPT;
+
+	list_add(&ctx->next, &edac->socs);
+
+	xgene_edac_soc_hw_init(edac_dev, 1);
+
+	devres_remove_group(edac->dev, xgene_edac_soc_add);
+
+	dev_info(edac->dev, "X-Gene EDAC SoC registered\n");
+
+	return 0;
+
+err_ctl_free:
+	edac_device_free_ctl_info(edac_dev);
+err_release_group:
+	devres_release_group(edac->dev, xgene_edac_soc_add);
+	return rc;
+}
+
+static int xgene_edac_soc_remove(struct xgene_edac_dev_ctx *soc)
+{
+	struct edac_device_ctl_info *edac_dev = soc->edac_dev;
+
+	xgene_edac_soc_hw_init(edac_dev, 0);
+	edac_device_del_device(soc->edac->dev);
+	edac_device_free_ctl_info(edac_dev);
+	return 0;
+}
+
 static irqreturn_t xgene_edac_isr(int irq, void *dev_id)
 {
 	struct xgene_edac *ctx = dev_id;
 	struct xgene_edac_pmd_ctx *pmd;
+	struct xgene_edac_dev_ctx *node;
 	unsigned int pcp_hp_stat;
 	unsigned int pcp_lp_stat;
 
@@ -1030,9 +1809,8 @@ static irqreturn_t xgene_edac_isr(int irq, void *dev_id)
 	    (MCU_CORR_ERR_MASK & pcp_lp_stat)) {
 		struct xgene_edac_mc_ctx *mcu;
 
-		list_for_each_entry(mcu, &ctx->mcus, next) {
+		list_for_each_entry(mcu, &ctx->mcus, next)
 			xgene_edac_mc_check(mcu->mci);
-		}
 	}
 
 	list_for_each_entry(pmd, &ctx->pmds, next) {
@@ -1040,6 +1818,12 @@ static irqreturn_t xgene_edac_isr(int irq, void *dev_id)
 			xgene_edac_pmd_check(pmd->edac_dev);
 	}
 
+	list_for_each_entry(node, &ctx->l3s, next)
+		xgene_edac_l3_check(node->edac_dev);
+
+	list_for_each_entry(node, &ctx->socs, next)
+		xgene_edac_soc_check(node->edac_dev);
+
 	return IRQ_HANDLED;
 }
 
@@ -1058,6 +1842,8 @@ static int xgene_edac_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, edac);
 	INIT_LIST_HEAD(&edac->mcus);
 	INIT_LIST_HEAD(&edac->pmds);
+	INIT_LIST_HEAD(&edac->l3s);
+	INIT_LIST_HEAD(&edac->socs);
 	spin_lock_init(&edac->lock);
 	mutex_init(&edac->mc_lock);
 
@@ -1122,6 +1908,8 @@ static int xgene_edac_probe(struct platform_device *pdev)
 		}
 	}
 
+	edac->dfs = edac_get_debugfs();
+
 	for_each_child_of_node(pdev->dev.of_node, child) {
 		if (!of_device_is_available(child))
 			continue;
@@ -1131,6 +1919,14 @@ static int xgene_edac_probe(struct platform_device *pdev)
 			xgene_edac_pmd_add(edac, child, 1);
 		if (of_device_is_compatible(child, "apm,xgene-edac-pmd-v2"))
 			xgene_edac_pmd_add(edac, child, 2);
+		if (of_device_is_compatible(child, "apm,xgene-edac-l3"))
+			xgene_edac_l3_add(edac, child, 1);
+		if (of_device_is_compatible(child, "apm,xgene-edac-l3-v2"))
+			xgene_edac_l3_add(edac, child, 2);
+		if (of_device_is_compatible(child, "apm,xgene-edac-soc"))
+			xgene_edac_soc_add(edac, child, 0);
+		if (of_device_is_compatible(child, "apm,xgene-edac-soc-v1"))
+			xgene_edac_soc_add(edac, child, 1);
 	}
 
 	return 0;
@@ -1146,14 +1942,21 @@ static int xgene_edac_remove(struct platform_device *pdev)
 	struct xgene_edac_mc_ctx *temp_mcu;
 	struct xgene_edac_pmd_ctx *pmd;
 	struct xgene_edac_pmd_ctx *temp_pmd;
+	struct xgene_edac_dev_ctx *node;
+	struct xgene_edac_dev_ctx *temp_node;
 
-	list_for_each_entry_safe(mcu, temp_mcu, &edac->mcus, next) {
+	list_for_each_entry_safe(mcu, temp_mcu, &edac->mcus, next)
 		xgene_edac_mc_remove(mcu);
-	}
 
-	list_for_each_entry_safe(pmd, temp_pmd, &edac->pmds, next) {
+	list_for_each_entry_safe(pmd, temp_pmd, &edac->pmds, next)
 		xgene_edac_pmd_remove(pmd);
-	}
+
+	list_for_each_entry_safe(node, temp_node, &edac->l3s, next)
+		xgene_edac_l3_remove(node);
+
+	list_for_each_entry_safe(node, temp_node, &edac->socs, next)
+		xgene_edac_soc_remove(node);
+
 	return 0;
 }
 
-- 
1.7.1

--
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 v3 4/4] arm64: Add L3/SoC DT subnodes to the APM X-Gene SoC EDAC node

[Loc Ho]

This patch adds L3/SoC DT subnodes to the APM X-Gene SoC EDAC node.

Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 arch/arm64/boot/dts/apm/apm-storm.dtsi |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index 58093ed..8ab6b0e 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -477,6 +477,16 @@
 				reg = <0x0 0x7c600000 0x0 0x200000>;
 				pmd-controller = <3>;
 			};
+
+			edacl3@7e600000 {
+				compatible = "apm,xgene-edac-l3";
+				reg = <0x0 0x7e600000 0x0 0x1000>;
+			};
+
+			edacsoc@7e930000 {
+				compatible = "apm,xgene-edac-soc-v1";
+				reg = <0x0 0x7e930000 0x0 0x1000>;
+			};
 		};
 
 		pcie0: pcie@1f2b0000 {
-- 
1.7.1

--
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 v3 3/4] edac: Add L3/SoC EDAC support to the APM X-Gene SoC EDAC driver

[Borislav Petkov]

On Tue, Aug 11, 2015 at 02:47:56PM -0600, Loc Ho wrote:
> This patch adds EDAC support for the L3 and SoC components.
> 
> Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
> ---
>  drivers/edac/xgene_edac.c |  827 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 815 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
> index ba06904..d5e33bf 100644
> --- a/drivers/edac/xgene_edac.c
> +++ b/drivers/edac/xgene_edac.c
> @@ -66,6 +66,8 @@ struct xgene_edac {
>  
>  	struct list_head	mcus;
>  	struct list_head	pmds;
> +	struct list_head	l3s;
> +	struct list_head	socs;
>  
>  	struct mutex		mc_lock;
>  	int			mc_active_mask;
> @@ -877,8 +879,8 @@ static const struct file_operations xgene_edac_pmd_debug_inject_fops[] = {
>  	{ }
>  };
>  
> -static void xgene_edac_pmd_create_debugfs_nodes(
> -	struct edac_device_ctl_info *edac_dev)
> +static void
> +xgene_edac_pmd_create_debugfs_nodes(struct edac_device_ctl_info *edac_dev)
>  {
>  	struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info;
>  	struct dentry *edac_debugfs;
> @@ -887,10 +889,6 @@ static void xgene_edac_pmd_create_debugfs_nodes(
>  	if (!IS_ENABLED(CONFIG_EDAC_DEBUG))
>  		return;
>  
> -	/*
> -	 * Todo: Switch to common EDAC debug file system for edac device
> -	 *       when available.
> -	 */
>  	if (!ctx->edac->dfs) {
>  		ctx->edac->dfs = debugfs_create_dir(edac_dev->dev->kobj.name,
>  						    NULL);

Why is this removing only the comment and not the debugfs_create_dir()
and the rest of the gunk?

xgene_edac_l3_create_debugfs_nodes() does call debugfs_create_dir()
creating a top-level edac debugfs node too. Why?

Please go through the previous comments I had and make sure you've
incorporated them all. I'm not wasting time reviewing half-baked stuff.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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 v3 3/4] edac: Add L3/SoC EDAC support to the APM X-Gene SoC EDAC driver

[Loc Ho]

Hi Borislav,

>> ---
>>  drivers/edac/xgene_edac.c |  827 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 815 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
>> index ba06904..d5e33bf 100644
>> --- a/drivers/edac/xgene_edac.c
>> +++ b/drivers/edac/xgene_edac.c
>> @@ -66,6 +66,8 @@ struct xgene_edac {
>>
>>       struct list_head        mcus;
>>       struct list_head        pmds;
>> +     struct list_head        l3s;
>> +     struct list_head        socs;
>>
>>       struct mutex            mc_lock;
>>       int                     mc_active_mask;
>> @@ -877,8 +879,8 @@ static const struct file_operations xgene_edac_pmd_debug_inject_fops[] = {
>>       { }
>>  };
>>
>> -static void xgene_edac_pmd_create_debugfs_nodes(
>> -     struct edac_device_ctl_info *edac_dev)
>> +static void
>> +xgene_edac_pmd_create_debugfs_nodes(struct edac_device_ctl_info *edac_dev)
>>  {
>>       struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info;
>>       struct dentry *edac_debugfs;
>> @@ -887,10 +889,6 @@ static void xgene_edac_pmd_create_debugfs_nodes(
>>       if (!IS_ENABLED(CONFIG_EDAC_DEBUG))
>>               return;
>>
>> -     /*
>> -      * Todo: Switch to common EDAC debug file system for edac device
>> -      *       when available.
>> -      */
>>       if (!ctx->edac->dfs) {
>>               ctx->edac->dfs = debugfs_create_dir(edac_dev->dev->kobj.name,
>>                                                   NULL);
>
> Why is this removing only the comment and not the debugfs_create_dir()
> and the rest of the gunk?

I intentional left this code as is so that the driver will still works
with older version of the EDAC code. Since you don't want this, I will
get rip of this.

>
> xgene_edac_l3_create_debugfs_nodes() does call debugfs_create_dir()
> creating a top-level edac debugfs node too. Why?

Each subnode will create an folder under the edac debug fs. It is no
different than the EDAC MC debug file system. You will see:

/sys/kernel/debug/edac/mc0, ..
/sys/kernel/debug/edac/mc2
/sys/kernel/debug/edac/pmd0, ...
/sys/kernel/debug/edac/pmd3
/sys/kernel/debug/edac/l3c4

>
> Please go through the previous comments I had and make sure you've
> incorporated them all. I'm not wasting time reviewing half-baked stuff.

As for your comments from previous v2 email:

>> +static void xgene_edac_l3_hw_init(struct edac_device_ctl_info *edac_dev,
>> +                               bool enable)
>
>arg alignment. It should be
>
>function name(arg1, arg2,
>              arg3, arg4,
>              arg5...
>
> Check your other functions too.

[Loc Ho]
I looked over the entire v3 driver and they are all aligned correct.

>> +static void xgene_edac_l3_create_debugfs_nodes(
>> +     struct edac_device_ctl_info *edac_dev)
>
>This "(" brace at the end looks ugly. Simply leave it on the same line,
>even if if it is longer than 80-cols.
>
>You can do:
>
>static void xgene_edac_l3_create_debugfs_nodes(struct edac_device_ctl_info *edac_dev)
>{
>
>or:
>
>static void
>xgene_edac_l3_create_debugfs_nodes(struct edac_device_ctl_info *edac_dev)
>{
>
>which is more readable.

[Loc Ho]
I looked over the entire v3 driver and they are all aligned correct.

>> +     /*
>> +      * Todo: Switch to common EDAC debug file system for edac device
>> +      *       when available.
>> +      */
>
>What is that?

[Loc Ho]
This one was change in v3.

>> +
>> +     if (edac_op_state == EDAC_OPSTATE_POLL)
>> +             edac_dev->edac_check = xgene_edac_l3_check;
>
>So depending on what SoC elements you detect, the last one's check
>function will win?
>
>If xgene_edac loads on multiple SoC elements, you need a global xgene
>check function for which l3, soc, mc, etc register...

[Loc Ho]
I already commented on this one. It is per subnode type.

>> +     dev_info(edac->dev, "X-Gene EDAC L3 registered\n");
>> +     return 0;
>> +
>> +err1:
>> +     edac_device_free_ctl_info(edac_dev);
>> +err:
>
>Those labels need to be better named.

[Loc Ho]
I looked over the entire v3 driver and they are with proper naming.

>> +             if (reg & (DED_ERR_MASK | MDED_ERR_MASK))
>> +                     edac_device_handle_ue(edac_dev, 0, 0,
>> +                                           edac_dev->ctl_name);
>> +     }
>
>You can flip the logic in that function:
>
>        if (!reg)
>
>and use goto labels and thus save an indentation level. Which looks like
>you could use it.

[Loc Ho]

I had already reversed the logic for the L3 and SoC. Are you asking
for the MC/PMD (earlier posting)? The MC/PMD routine isn't that big.
Functions in general fit in an screen of 80 column?

>> +static const char * const *xgene_edac_soc_mem_data(
>> +     struct xgene_edac_dev_ctx *ctx)
>> +{
>> +     switch (ctx->version) {
>> +     case 1:
>> +             return soc_mem_err_v1;
>> +     default:
>> +             return NULL;
>> +     }
>> +}
>
>That's one badly formatted function used only once. Just move the logic
>there and kill it.

[Loc Ho]
I already killed this function.

>> +     list_for_each_entry(node, &ctx->socs, next) {
>> +             xgene_edac_soc_check(node->edac_dev);
>> +     }
>
>Loops with single statements don't need {}

[Loc Ho]
I already got rip of un-necessary { }

So, is there something that I am missing?

-Loc
--
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 v3 3/4] edac: Add L3/SoC EDAC support to the APM X-Gene SoC EDAC driver

[Borislav Petkov]

On Wed, Aug 12, 2015 at 10:50:52AM -0700, Loc Ho wrote:
> I intentional left this code as is so that the driver will still works
> with older version of the EDAC code.

Huh, what? I don't think this is how upstream development works.

> I had already reversed the logic for the L3 and SoC. Are you asking
> for the MC/PMD (earlier posting)? The MC/PMD routine isn't that big.
> Functions in general fit in an screen of 80 column?

Only for the ones where it becomes clumsy and flipping it would make the
code more readable. I'm not a hard 80 cols follower anyway.

> So, is there something that I am missing?

I hope you're not. But seeing the custom debugfs stuff left in the new
submission without any comment as to *why* you've left it, made me think
you didn't incorporate all feedback.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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 v3 3/4] edac: Add L3/SoC EDAC support to the APM X-Gene SoC EDAC driver

[Loc Ho]

Hi,

>> I intentional left this code as is so that the driver will still works
>> with older version of the EDAC code.
>
> Huh, what? I don't think this is how upstream development works.

I will fix this in next version.

>
>> I had already reversed the logic for the L3 and SoC. Are you asking
>> for the MC/PMD (earlier posting)? The MC/PMD routine isn't that big.
>> Functions in general fit in an screen of 80 column?
>
> Only for the ones where it becomes clumsy and flipping it would make the
> code more readable. I'm not a hard 80 cols follower anyway.

I will look at the code one more time and see if the code would be
better with logic flipped for L2/L1 and MC.

>
>> So, is there something that I am missing?
>
> I hope you're not. But seeing the custom debugfs stuff left in the new
> submission without any comment as to *why* you've left it, made me think
> you didn't incorporate all feedback.

-Loc
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Borislav Petkov]

On Tue, Aug 11, 2015 at 02:47:54PM -0600, Loc Ho wrote:
> This patch adds an function to retrieve the EDAC debugfs node. This allows
> EDAC driver to create debugfs node under the EDAC debugfs node.
> 
> Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
> ---
>  drivers/edac/edac_core.h     |    1 +
>  drivers/edac/edac_mc_sysfs.c |   10 ++++++++++
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
> index ad42587..6771e64 100644
> --- a/drivers/edac/edac_core.h
> +++ b/drivers/edac/edac_core.h
> @@ -511,5 +511,6 @@ extern void edac_pci_remove_sysfs(struct edac_pci_ctl_info *pci);
>   * edac misc APIs
>   */
>  extern char *edac_op_state_to_string(int op_state);
> +extern struct dentry *edac_get_debugfs(void);
>  
>  #endif				/* _EDAC_CORE_H_ */
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 33df7d9..b76afe0 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -959,6 +959,16 @@ nomem:
>  	debugfs_remove(mci->debugfs);
>  	return -ENOMEM;
>  }
> +
> +struct dentry *edac_get_debugfs(void)
> +{
> +	return edac_debugfs;
> +}

EXPORT_SYMBOL_GPL() if it is going to be used by modules.

> +#else
> +struct dentry *edac_get_debugfs(void)

static inline struct ...

> +{
> +	return NULL;
> +}
>  #endif
>  
>  /*
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Borislav Petkov]

On Wed, Aug 12, 2015 at 12:12:19PM +0200, Borislav Petkov wrote:
> EXPORT_SYMBOL_GPL() if it is going to be used by modules.

Better yet, move it to the header.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Loc Ho]

Hi Borislav,

>> EXPORT_SYMBOL_GPL() if it is going to be used by modules.
>
> Better yet, move it to the header.

I can't move it completely to the header and make it static. It will
generate a lot of compiler warning as it isn't used by other modules.
I will add EXPORT_SYMBOL_GPL() in the next version.

-Loc
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Borislav Petkov]

On Wed, Aug 12, 2015 at 10:23:11AM -0700, Loc Ho wrote:
> I can't move it completely to the header and make it static. It will
> generate a lot of compiler warning as it isn't used by other modules.

Come again?

I'm talking about static inline functions in headers. Grep the kernel
sources for examples.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Loc Ho]

Hi,

>
>>> EXPORT_SYMBOL_GPL() if it is going to be used by modules.
>>
>> Better yet, move it to the header.
>
> I can't move it completely to the header and make it static. It will
> generate a lot of compiler warning as it isn't used by other modules.
> I will add EXPORT_SYMBOL_GPL() in the next version.

Okay... The 'inline' helps with the compiler warning. If I move all to
header file with 'static inline' and wrapper around CONFIG_EDAC_DEBUG,
don't I need to export and declare the variable 'edac_debugfs'? Are
you okay with export this variable? Or, it is static inline if not
defined CONFIG_EDAC_DEBUG. Otherwise, just  an extern header function
and export it as well?

-Loc
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Borislav Petkov]

On Wed, Aug 12, 2015 at 09:30:58PM -0700, Loc Ho wrote:
> Okay... The 'inline' helps with the compiler warning. If I move all to
> header file with 'static inline' and wrapper around CONFIG_EDAC_DEBUG,
> don't I need to export and declare the variable 'edac_debugfs'? Are
> you okay with export this variable?

You need to make edac_debugfs extern in the header and define it in
edac_mc_sysfs.c.

But that's ok because edac_core.h will be included only by EDAC drivers
and they're supposed to see edac_debugfs anyway...

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Lothar Waßmann]

Hi,

> On Wed, Aug 12, 2015 at 09:30:58PM -0700, Loc Ho wrote:
> > Okay... The 'inline' helps with the compiler warning. If I move all to
> > header file with 'static inline' and wrapper around CONFIG_EDAC_DEBUG,
> > don't I need to export and declare the variable 'edac_debugfs'? Are
> > you okay with export this variable?
> 
> You need to make edac_debugfs extern in the header and define it in
> edac_mc_sysfs.c.
> 
> But that's ok because edac_core.h will be included only by EDAC drivers
> and they're supposed to see edac_debugfs anyway...
> 
What's the point in having an accessor function that returns a driver
internal variable when this variable is exported anyway?
This will only further inconsistencies because half of the users will
use the variable directly and the other half will use the accessor
function.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info-AvR2QvxeiV7DiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org
___________________________________________________________
--
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 v3 1/4] edac: Add an function to retrieve the EDAC debugfs node

[Borislav Petkov]

On Thu, Aug 13, 2015 at 10:41:50AM +0200, Lothar Waßmann wrote:
> What's the point in having an accessor function that returns a driver
> internal variable when this variable is exported anyway?

Not driver-internal but EDAC-core internal.

> This will only further inconsistencies because half of the users will
> use the variable directly and the other half will use the accessor
> function.

The idea is that it is not visible outside of EDAC. The EDAC drivers
themselves should see it. And yes, I admit that the accessors are maybe
redundant then since EDAC drivers can use edac_debugfs ptr directly.

Hmm, and that's ok too because my other concern about future refactoring
is taken care of too because as long as all users of edac_debugfs are
confined to drivers/edac/, changing stuff there is easy.

Ok, I'm convinced, Loc, feel free to drop the accessors in your next
version.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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