[PATCH v9 0/3] Add support for Amazon's Annapurna Labs EDAC for L1/L2

[PATCH v9 0/3] Add support for Amazon's Annapurna Labs EDAC for L1/L2

[Hanna Hawa]

This series adds L1 cache and L2 cache error detection and correction support
for Amazon's Annapurna Labs SoCs.

Alpine SoCs supports L1 and L2 single bit correction and two bits detection
capability based on ARM implementation.

The CPU cores in the SoC are the same and all of them support ECC.

Changes since v8:
-----------------
- Fix Kconfig dependency, to be depends ARM64 in any case (for compile
  test also), as the driver use arm64 sysreg functions.
  Reported-by: kbuild test robot <lkp@intel.com>

Changes since v7:
-----------------
- Add missing <linux/platform_device.h> include
- Use new APIs edac_device_handle_{ue/ce}_count
- Add lock around edac_device_handle_{ue/ce}_count
- Check retrun value of of_find_node_by_path
- Remove the goto with single usage and restructuring the code
- Add of_node_put for L2 cache of_node

Changes since v6:
-----------------
- Add ARM64 dependency
- Add COMPILE_TEST

Changes since v5:
-----------------
- Use top-level machine compatible to bind the EDAC device
- Remove DT bindings
- Add initcall to create platform device and register the edac driver
- follow 'next-level-cache' phandle to create CPU topology for L2 driver
- Change the driver to be tristate
- Move register read to function flow
- EXPORT_SYMBOL_GPL of_find_next_cache_node

Changes since v4:
-----------------
- Added include for cpumask.h in al_l2_edac.c
- Fix RAMID error print according to ARM TRM
- Use for_each_possible_cpu() to parse information for DT.
- Add missing of_node_put() call.

Changes since v3:
-----------------
- Added include for smp.h sysreg.h
- Use scnprintf instead of snprintf
- Move write_sysreg_s after valid check to minimize the window between
read/write.
- Use IS_ERR_OR_NULL instead of IS_ERR, because
edac_device_alloc_ctl_info may return NULL.

Changes since v2:
-----------------
- Use BIT for single bit instead of GENMASK
- Use BIT_ULL and GENMASK_ULL for 64bit vector
- Fix the mod_name/ctrl_name.

Changes since v1:
-----------------
- Split into two drivers
- Get cpu-mask according to l2-cache handler from devicetree
- Remove parameter casting
- Use GENMASK() in bit mask
- Use FIELD_GET()
- Update define description PLRU_RAM -> PF_RAM
- Use sys_reg() and read_sysreg_s()
- Remove all write/read wrappers
- Check fatal field to set if the error correctable or not
- Remove un-relevant information from error prints.
- Update smp_call_function_single() call function to wait
- remove usage of get_online_cpus/put_online_cpus
- Use on_each_cpu() and smp_call_function_any() instead of loop with for_each_cpu.
- use buffer for error prints and pass to edac API
- Remove edac_op_state set
- Add for loop to report on repeated errors of the same type
- Fix error name of the TLB to be L2_TLB as written in ARM TRM
- Minor change in Kconfig
- Minor changes in commit message

Hanna Hawa (3):
  edac: Add support for Amazon's Annapurna Labs L1 EDAC
  of: EXPORT_SYMBOL_GPL of_find_next_cache_node
  edac: Add support for Amazon's Annapurna Labs L2 EDAC

 MAINTAINERS               |  10 ++
 drivers/edac/Kconfig      |  16 +++
 drivers/edac/Makefile     |   2 +
 drivers/edac/al_l1_edac.c | 207 +++++++++++++++++++++++++++++
 drivers/edac/al_l2_edac.c | 270 ++++++++++++++++++++++++++++++++++++++
 drivers/of/base.c         |   1 +
 6 files changed, 506 insertions(+)
 create mode 100644 drivers/edac/al_l1_edac.c
 create mode 100644 drivers/edac/al_l2_edac.c

-- 
2.17.1

[PATCH v9 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

[Hanna Hawa]

Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
report L1 errors.

Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
 MAINTAINERS               |   5 +
 drivers/edac/Kconfig      |   8 ++
 drivers/edac/Makefile     |   1 +
 drivers/edac/al_l1_edac.c | 207 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 221 insertions(+)
 create mode 100644 drivers/edac/al_l1_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bd5847e802de..649ff0d314e5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -763,6 +763,11 @@ F:	drivers/tty/serial/altera_jtaguart.c
 F:	include/linux/altera_uart.h
 F:	include/linux/altera_jtaguart.h
 
+AMAZON ANNAPURNA LABS L1 EDAC
+M:	Hanna Hawa <hhhawa@amazon.com>
+S:	Maintained
+F:	drivers/edac/al_l1_edac.c
+
 AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER
 M:	Talel Shenhar <talel@amazon.com>
 S:	Maintained
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 37027c298323..0ea9439749ab 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -74,6 +74,14 @@ config EDAC_GHES
 
 	  In doubt, say 'Y'.
 
+config EDAC_AL_L1
+	tristate "Amazon's Annapurna Labs L1 EDAC"
+	depends on ARM64 && (ARCH_ALPINE || COMPILE_TEST)
+	help
+	  Support for L1 error detection and correction
+	  for Amazon's Annapurna Labs SoCs.
+	  This driver detects errors of L1 caches.
+
 config EDAC_AMD64
 	tristate "AMD64 (Opteron, Athlon64)"
 	depends on AMD_NB && EDAC_DECODE_MCE
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index d77200c9680b..7d67433b683a 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_EDAC_GHES)			+= ghes_edac.o
 edac_mce_amd-y				:= mce_amd.o
 obj-$(CONFIG_EDAC_DECODE_MCE)		+= edac_mce_amd.o
 
+obj-$(CONFIG_EDAC_AL_L1)		+= al_l1_edac.o
 obj-$(CONFIG_EDAC_AMD76X)		+= amd76x_edac.o
 obj-$(CONFIG_EDAC_CPC925)		+= cpc925_edac.o
 obj-$(CONFIG_EDAC_I5000)		+= i5000_edac.o
diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
new file mode 100644
index 000000000000..723b35b18f5b
--- /dev/null
+++ b/drivers/edac/al_l1_edac.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ */
+
+#include <asm/sysreg.h>
+#include <linux/bitfield.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/smp.h>
+
+#include "edac_device.h"
+#include "edac_module.h"
+
+#define DRV_NAME				"al_l1_edac"
+
+/* Same bit assignments of CPUMERRSR_EL1 in ARM CA57/CA72 */
+#define ARM_CA57_CPUMERRSR_EL1			sys_reg(3, 1, 15, 2, 2)
+#define ARM_CA57_CPUMERRSR_RAM_ID		GENMASK(30, 24)
+#define  ARM_CA57_L1_I_TAG_RAM			0x00
+#define  ARM_CA57_L1_I_DATA_RAM			0x01
+#define  ARM_CA57_L1_D_TAG_RAM			0x08
+#define  ARM_CA57_L1_D_DATA_RAM			0x09
+#define  ARM_CA57_L2_TLB_RAM			0x18
+#define ARM_CA57_CPUMERRSR_VALID		BIT(31)
+#define ARM_CA57_CPUMERRSR_REPEAT		GENMASK_ULL(39, 32)
+#define ARM_CA57_CPUMERRSR_OTHER		GENMASK_ULL(47, 40)
+#define ARM_CA57_CPUMERRSR_FATAL		BIT_ULL(63)
+
+#define AL_L1_EDAC_MSG_MAX			256
+
+static void al_l1_edac_cpumerrsr_read_status(void *arg)
+{
+	struct edac_device_ctl_info *edac_dev = arg;
+	int cpu, space, count;
+	u32 ramid, repeat, other, fatal;
+	u64 val;
+	char msg[AL_L1_EDAC_MSG_MAX];
+	char *p;
+	spinlock_t *lock;
+
+	val = read_sysreg_s(ARM_CA57_CPUMERRSR_EL1);
+	if (!(FIELD_GET(ARM_CA57_CPUMERRSR_VALID, val)))
+		return;
+
+	write_sysreg_s(0, ARM_CA57_CPUMERRSR_EL1);
+
+	cpu = smp_processor_id();
+	ramid = FIELD_GET(ARM_CA57_CPUMERRSR_RAM_ID, val);
+	repeat = FIELD_GET(ARM_CA57_CPUMERRSR_REPEAT, val);
+	other = FIELD_GET(ARM_CA57_CPUMERRSR_OTHER, val);
+	fatal = FIELD_GET(ARM_CA57_CPUMERRSR_FATAL, val);
+
+	space = sizeof(msg);
+	p = msg;
+	count = scnprintf(p, space, "CPU%d L1 %serror detected", cpu,
+			  (fatal) ? "Fatal " : "");
+	p += count;
+	space -= count;
+
+	switch (ramid) {
+	case ARM_CA57_L1_I_TAG_RAM:
+		count = scnprintf(p, space, " RAMID='L1-I Tag RAM'");
+		break;
+	case ARM_CA57_L1_I_DATA_RAM:
+		count = scnprintf(p, space, " RAMID='L1-I Data RAM'");
+		break;
+	case ARM_CA57_L1_D_TAG_RAM:
+		count = scnprintf(p, space, " RAMID='L1-D Tag RAM'");
+		break;
+	case ARM_CA57_L1_D_DATA_RAM:
+		count = scnprintf(p, space, " RAMID='L1-D Data RAM'");
+		break;
+	case ARM_CA57_L2_TLB_RAM:
+		count = scnprintf(p, space, " RAMID='L2 TLB RAM'");
+		break;
+	default:
+		count = scnprintf(p, space, " RAMID='unknown'");
+		break;
+	}
+
+	p += count;
+	space -= count;
+	count = scnprintf(p, space,
+			  " repeat=%d, other=%d (CPUMERRSR_EL1=0x%llx)",
+			  repeat, other, val);
+
+	lock = edac_dev->pvt_info;
+	spin_lock(lock);
+	if (fatal)
+		edac_device_handle_ue_count(edac_dev, repeat, 0, 0, msg);
+	else
+		edac_device_handle_ce_count(edac_dev, repeat, 0, 0, msg);
+	spin_unlock(lock);
+}
+
+static void al_l1_edac_check(struct edac_device_ctl_info *edac_dev)
+{
+	on_each_cpu(al_l1_edac_cpumerrsr_read_status, edac_dev, 1);
+}
+
+static int al_l1_edac_probe(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *edac_dev;
+	struct device *dev = &pdev->dev;
+	spinlock_t *lock;
+	int ret;
+
+	edac_dev = edac_device_alloc_ctl_info(sizeof(*lock), DRV_NAME, 1, "L",
+					      1, 1, NULL, 0,
+					      edac_device_alloc_index());
+	if (!edac_dev)
+		return -ENOMEM;
+
+	edac_dev->edac_check = al_l1_edac_check;
+	edac_dev->dev = dev;
+	edac_dev->mod_name = DRV_NAME;
+	edac_dev->dev_name = dev_name(dev);
+	edac_dev->ctl_name = "L1_cache";
+	platform_set_drvdata(pdev, edac_dev);
+	lock = edac_dev->pvt_info;
+
+	spin_lock_init(lock);
+
+	ret = edac_device_add_device(edac_dev);
+	if (ret) {
+		dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
+		edac_device_free_ctl_info(edac_dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int al_l1_edac_remove(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
+
+	edac_device_del_device(edac_dev->dev);
+	edac_device_free_ctl_info(edac_dev);
+
+	return 0;
+}
+
+static const struct of_device_id al_l1_edac_of_match[] = {
+	/*
+	 * "al,alpine-v2", and "amazon,al-alpine-v3" are machine compatible
+	 * strings which have Cortex-A57/A72 configured with this support,
+	 * and access to CPUMERRSR_EL1 register is enabled in firmware.
+	 */
+	{ .compatible = "al,alpine-v2" },
+	{ .compatible = "amazon,al-alpine-v3" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);
+
+static struct platform_driver al_l1_edac_driver = {
+	.probe = al_l1_edac_probe,
+	.remove = al_l1_edac_remove,
+	.driver = {
+		.name = DRV_NAME,
+	},
+};
+
+static struct platform_device *edac_l1_device;
+
+static int __init al_l1_init(void)
+{
+	struct device_node *root;
+	int ret;
+
+	root = of_find_node_by_path("/");
+	if (!root) {
+		pr_debug("Can't find root node!\n");
+		return 0;
+	}
+
+	if (!of_match_node(al_l1_edac_of_match, root))
+		return 0;
+
+	ret = platform_driver_register(&al_l1_edac_driver);
+	if (ret) {
+		pr_err("Failed to register %s (%d)\n", DRV_NAME, ret);
+		return ret;
+	}
+
+	edac_l1_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
+	if (IS_ERR(edac_l1_device)) {
+		pr_err("Failed to register EDAC AL L1 platform device\n");
+		return PTR_ERR(edac_l1_device);
+	}
+
+	return 0;
+}
+
+static void __exit al_l1_exit(void)
+{
+	platform_device_unregister(edac_l1_device);
+	platform_driver_unregister(&al_l1_edac_driver);
+}
+
+late_initcall(al_l1_init);
+module_exit(al_l1_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Hanna Hawa <hhhawa@amazon.com>");
+MODULE_DESCRIPTION("Amazon's Annapurna Lab's L1 EDAC Driver");
-- 
2.17.1

[PATCH v9 2/3] of: EXPORT_SYMBOL_GPL of_find_next_cache_node

[Hanna Hawa]

Make of_find_next_cache_node() available for modules.

Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index db7fbc0c0893..416286582b9b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2224,6 +2224,7 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(of_find_next_cache_node);
 
 /**
  * of_find_last_cache_level - Find the level at which the last cache is
-- 
2.17.1

[PATCH v9 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC

[Hanna Hawa]

Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and
report L2 errors.

Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
---
 MAINTAINERS               |   5 +
 drivers/edac/Kconfig      |   8 ++
 drivers/edac/Makefile     |   1 +
 drivers/edac/al_l2_edac.c | 270 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 284 insertions(+)
 create mode 100644 drivers/edac/al_l2_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 649ff0d314e5..5dd8a14e2a30 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -768,6 +768,11 @@ M:	Hanna Hawa <hhhawa@amazon.com>
 S:	Maintained
 F:	drivers/edac/al_l1_edac.c
 
+AMAZON ANNAPURNA LABS L2 EDAC
+M:	Hanna Hawa <hhhawa@amazon.com>
+S:	Maintained
+F:	drivers/edac/al_l2_edac.c
+
 AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER
 M:	Talel Shenhar <talel@amazon.com>
 S:	Maintained
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 0ea9439749ab..f22c875f61bb 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -82,6 +82,14 @@ config EDAC_AL_L1
 	  for Amazon's Annapurna Labs SoCs.
 	  This driver detects errors of L1 caches.
 
+config EDAC_AL_L2
+	tristate "Amazon's Annapurna Labs L2 EDAC"
+	depends on ARM64 && (ARCH_ALPINE || COMPILE_TEST)
+	help
+	  Support for L2 error detection and correction
+	  for Amazon's Annapurna Labs SoCs.
+	  This driver detects errors of L2 caches.
+
 config EDAC_AMD64
 	tristate "AMD64 (Opteron, Athlon64)"
 	depends on AMD_NB && EDAC_DECODE_MCE
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 7d67433b683a..ae48f8fccec3 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -23,6 +23,7 @@ edac_mce_amd-y				:= mce_amd.o
 obj-$(CONFIG_EDAC_DECODE_MCE)		+= edac_mce_amd.o
 
 obj-$(CONFIG_EDAC_AL_L1)		+= al_l1_edac.o
+obj-$(CONFIG_EDAC_AL_L2)		+= al_l2_edac.o
 obj-$(CONFIG_EDAC_AMD76X)		+= amd76x_edac.o
 obj-$(CONFIG_EDAC_CPC925)		+= cpc925_edac.o
 obj-$(CONFIG_EDAC_I5000)		+= i5000_edac.o
diff --git a/drivers/edac/al_l2_edac.c b/drivers/edac/al_l2_edac.c
new file mode 100644
index 000000000000..147094efb6fd
--- /dev/null
+++ b/drivers/edac/al_l2_edac.c
@@ -0,0 +1,270 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ */
+
+#include <asm/sysreg.h>
+#include <linux/bitfield.h>
+#include <linux/cpumask.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/smp.h>
+
+#include "edac_device.h"
+#include "edac_module.h"
+
+#define DRV_NAME				"al_l2_edac"
+
+/* Same bit assignments of L2MERRSR_EL1 in ARM CA57/CA72 */
+#define ARM_CA57_L2MERRSR_EL1			sys_reg(3, 1, 15, 2, 3)
+#define ARM_CA57_L2MERRSR_RAMID			GENMASK(30, 24)
+#define  ARM_CA57_L2_TAG_RAM			0x10
+#define  ARM_CA57_L2_DATA_RAM			0x11
+#define  ARM_CA57_L2_SNOOP_RAM			0x12
+#define  ARM_CA57_L2_DIRTY_RAM			0x14
+#define  ARM_CA57_L2_INC_PF_RAM			0x18
+#define ARM_CA57_L2MERRSR_VALID			BIT(31)
+#define ARM_CA57_L2MERRSR_REPEAT		GENMASK_ULL(39, 32)
+#define ARM_CA57_L2MERRSR_OTHER			GENMASK_ULL(47, 40)
+#define ARM_CA57_L2MERRSR_FATAL			BIT_ULL(63)
+
+#define AL_L2_EDAC_MSG_MAX			256
+
+struct al_l2_cache {
+	cpumask_t cluster_cpus;
+	struct list_head list_node;
+	struct device_node *of_node;
+};
+
+struct al_l2_edac {
+	struct list_head l2_caches;
+	spinlock_t lock;
+};
+
+static void al_l2_edac_l2merrsr_read_status(void *arg)
+{
+	struct edac_device_ctl_info *edac_dev = arg;
+	struct al_l2_edac *al_l2 = edac_dev->pvt_info;
+	int cpu, space, count;
+	u32 ramid, repeat, other, fatal;
+	u64 val;
+	char msg[AL_L2_EDAC_MSG_MAX];
+	char *p;
+
+	val = read_sysreg_s(ARM_CA57_L2MERRSR_EL1);
+	if (!(FIELD_GET(ARM_CA57_L2MERRSR_VALID, val)))
+		return;
+
+	write_sysreg_s(0, ARM_CA57_L2MERRSR_EL1);
+
+	cpu = smp_processor_id();
+	ramid = FIELD_GET(ARM_CA57_L2MERRSR_RAMID, val);
+	repeat = FIELD_GET(ARM_CA57_L2MERRSR_REPEAT, val);
+	other = FIELD_GET(ARM_CA57_L2MERRSR_OTHER, val);
+	fatal = FIELD_GET(ARM_CA57_L2MERRSR_FATAL, val);
+
+	space = sizeof(msg);
+	p = msg;
+	count = scnprintf(p, space, "CPU%d L2 %serror detected", cpu,
+			  (fatal) ? "Fatal " : "");
+	p += count;
+	space -= count;
+
+	switch (ramid) {
+	case ARM_CA57_L2_TAG_RAM:
+		count = scnprintf(p, space, " RAMID='L2 Tag RAM'");
+		break;
+	case ARM_CA57_L2_DATA_RAM:
+		count = scnprintf(p, space, " RAMID='L2 Data RAM'");
+		break;
+	case ARM_CA57_L2_SNOOP_RAM:
+		count = scnprintf(p, space, " RAMID='L2 Snoop Tag RAM'");
+		break;
+	case ARM_CA57_L2_DIRTY_RAM:
+		count = scnprintf(p, space, " RAMID='L2 Dirty RAM'");
+		break;
+	case ARM_CA57_L2_INC_PF_RAM:
+		count = scnprintf(p, space, " RAMID='L2 internal metadata'");
+		break;
+	default:
+		count = scnprintf(p, space, " RAMID='unknown'");
+		break;
+	}
+
+	p += count;
+	space -= count;
+
+	count = scnprintf(p, space,
+			  " repeat=%d, other=%d (L2MERRSR_EL1=0x%llx)",
+			  repeat, other, val);
+
+	spin_lock(&al_l2->lock);
+	if (fatal)
+		edac_device_handle_ue_count(edac_dev, repeat, 0, 0, msg);
+	else
+		edac_device_handle_ce_count(edac_dev, repeat, 0, 0, msg);
+	spin_unlock(&al_l2->lock);
+}
+
+static void al_l2_edac_check(struct edac_device_ctl_info *edac_dev)
+{
+	struct al_l2_edac *al_l2 = edac_dev->pvt_info;
+	struct al_l2_cache *l2_cache;
+
+	list_for_each_entry(l2_cache, &al_l2->l2_caches, list_node)
+		smp_call_function_any(&l2_cache->cluster_cpus,
+				      al_l2_edac_l2merrsr_read_status,
+				      edac_dev, 1);
+}
+
+static int al_l2_edac_probe(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *edac_dev;
+	struct al_l2_edac *al_l2;
+	struct al_l2_cache *l2_cache;
+	struct device *dev = &pdev->dev;
+	int ret, i;
+
+	edac_dev = edac_device_alloc_ctl_info(sizeof(*al_l2), DRV_NAME, 1, "L",
+					      1, 2, NULL, 0,
+					      edac_device_alloc_index());
+	if (!edac_dev)
+		return -ENOMEM;
+
+	al_l2 = edac_dev->pvt_info;
+	edac_dev->edac_check = al_l2_edac_check;
+	edac_dev->dev = dev;
+	edac_dev->mod_name = DRV_NAME;
+	edac_dev->dev_name = dev_name(dev);
+	edac_dev->ctl_name = "L2_cache";
+	platform_set_drvdata(pdev, edac_dev);
+
+	spin_lock_init(&al_l2->lock);
+	INIT_LIST_HEAD(&al_l2->l2_caches);
+
+	for_each_possible_cpu(i) {
+		struct device_node *cpu;
+		struct device_node *cpu_cache;
+		bool found = false;
+
+		cpu = of_get_cpu_node(i, NULL);
+		if (!cpu)
+			continue;
+
+		cpu_cache = of_find_next_cache_node(cpu);
+		list_for_each_entry(l2_cache, &al_l2->l2_caches, list_node) {
+			if (l2_cache->of_node == cpu_cache) {
+				found = true;
+				break;
+			}
+		}
+
+		if (found) {
+			cpumask_set_cpu(i, &l2_cache->cluster_cpus);
+			of_node_put(cpu_cache);
+		} else {
+			l2_cache = devm_kzalloc(dev, sizeof(*l2_cache),
+						GFP_KERNEL);
+			l2_cache->of_node = cpu_cache;
+			list_add(&l2_cache->list_node, &al_l2->l2_caches);
+			cpumask_set_cpu(i, &l2_cache->cluster_cpus);
+		}
+
+		of_node_put(cpu);
+	}
+
+	list_for_each_entry(l2_cache, &al_l2->l2_caches, list_node)
+		of_node_put(l2_cache->of_node);
+
+	if (list_empty(&al_l2->l2_caches)) {
+		dev_err(dev, "L2 Cache list is empty for EDAC device\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = edac_device_add_device(edac_dev);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	dev_err(dev, "Failed to add L2 edac device (%d)\n", ret);
+	edac_device_free_ctl_info(edac_dev);
+
+	return ret;
+}
+
+static int al_l2_edac_remove(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
+
+	edac_device_del_device(edac_dev->dev);
+	edac_device_free_ctl_info(edac_dev);
+
+	return 0;
+}
+
+static const struct of_device_id al_l2_edac_of_match[] = {
+	/*
+	 * "al,alpine-v2", and "amazon,al-alpine-v3" are machine compatible
+	 * strings which have Cortex-A57/A72 configured with this support,
+	 * and access to L2MERRSR_EL1 register is enabled in firmware.
+	 */
+	{ .compatible = "al,alpine-v2" },
+	{ .compatible = "amazon,al-alpine-v3" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, al_l2_edac_of_match);
+
+static struct platform_driver al_l2_edac_driver = {
+	.probe = al_l2_edac_probe,
+	.remove = al_l2_edac_remove,
+	.driver = {
+		.name = DRV_NAME,
+	},
+};
+
+static struct platform_device *edac_l2_device;
+
+static int __init al_l2_init(void)
+{
+	struct device_node *root;
+	int ret;
+
+	root = of_find_node_by_path("/");
+	if (!root) {
+		pr_debug("Can't find root node!\n");
+		return 0;
+	}
+
+	if (!of_match_node(al_l2_edac_of_match, root))
+		return 0;
+
+	ret = platform_driver_register(&al_l2_edac_driver);
+	if (ret) {
+		pr_err("Failed to register %s (%d)\n", DRV_NAME, ret);
+		return ret;
+	}
+
+	edac_l2_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
+	if (IS_ERR(edac_l2_device)) {
+		pr_err("Failed to register EDAC AL L2 platform device\n");
+		return PTR_ERR(edac_l2_device);
+	}
+
+	return 0;
+}
+
+static void __exit al_l2_exit(void)
+{
+	platform_device_unregister(edac_l2_device);
+	platform_driver_unregister(&al_l2_edac_driver);
+}
+
+late_initcall(al_l2_init);
+module_exit(al_l2_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Hanna Hawa <hhhawa@amazon.com>");
+MODULE_DESCRIPTION("Amazon's Annapurna Lab's L2 EDAC Driver");
-- 
2.17.1

Re: [PATCH v9 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

[Borislav Petkov]

On Wed, Jan 29, 2020 at 09:50:14PM +0200, Hanna Hawa wrote:

> Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
> report L1 errors.

You don't add support for a driver - you either add a driver or you add
support for HW...

> diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
> new file mode 100644
> index 000000000000..723b35b18f5b
> --- /dev/null
> +++ b/drivers/edac/al_l1_edac.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + */
> +
> +#include <asm/sysreg.h>
> +#include <linux/bitfield.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/smp.h>

asm/ includes go after linux/ includes.

> +
> +#include "edac_device.h"
> +#include "edac_module.h"
> +
> +#define DRV_NAME				"al_l1_edac"
> +
> +/* Same bit assignments of CPUMERRSR_EL1 in ARM CA57/CA72 */
> +#define ARM_CA57_CPUMERRSR_EL1			sys_reg(3, 1, 15, 2, 2)
> +#define ARM_CA57_CPUMERRSR_RAM_ID		GENMASK(30, 24)
> +#define  ARM_CA57_L1_I_TAG_RAM			0x00
> +#define  ARM_CA57_L1_I_DATA_RAM			0x01
> +#define  ARM_CA57_L1_D_TAG_RAM			0x08
> +#define  ARM_CA57_L1_D_DATA_RAM			0x09
> +#define  ARM_CA57_L2_TLB_RAM			0x18
> +#define ARM_CA57_CPUMERRSR_VALID		BIT(31)
> +#define ARM_CA57_CPUMERRSR_REPEAT		GENMASK_ULL(39, 32)
> +#define ARM_CA57_CPUMERRSR_OTHER		GENMASK_ULL(47, 40)
> +#define ARM_CA57_CPUMERRSR_FATAL		BIT_ULL(63)
> +
> +#define AL_L1_EDAC_MSG_MAX			256
> +
> +static void al_l1_edac_cpumerrsr_read_status(void *arg)

This is a static function so you don't really need a function prefix of
"al_l1_edac_". Use those chars to make its name more descriptive. Ditto
for the rest of the static functions in that file.

> +{
> +	struct edac_device_ctl_info *edac_dev = arg;
> +	int cpu, space, count;
> +	u32 ramid, repeat, other, fatal;
> +	u64 val;
> +	char msg[AL_L1_EDAC_MSG_MAX];
> +	char *p;
> +	spinlock_t *lock;

Please sort function local variables declaration in a reverse christmas
tree order:

	<type A> longest_variable_name;
	<type B> shorter_var_name;
	<type C> even_shorter;
	<type D> i;

> +
> +	val = read_sysreg_s(ARM_CA57_CPUMERRSR_EL1);
> +	if (!(FIELD_GET(ARM_CA57_CPUMERRSR_VALID, val)))
> +		return;
> +
> +	write_sysreg_s(0, ARM_CA57_CPUMERRSR_EL1);
> +
> +	cpu = smp_processor_id();
> +	ramid = FIELD_GET(ARM_CA57_CPUMERRSR_RAM_ID, val);
> +	repeat = FIELD_GET(ARM_CA57_CPUMERRSR_REPEAT, val);
> +	other = FIELD_GET(ARM_CA57_CPUMERRSR_OTHER, val);
> +	fatal = FIELD_GET(ARM_CA57_CPUMERRSR_FATAL, val);
> +
> +	space = sizeof(msg);
> +	p = msg;
> +	count = scnprintf(p, space, "CPU%d L1 %serror detected", cpu,
> +			  (fatal) ? "Fatal " : "");
> +	p += count;
> +	space -= count;
> +
> +	switch (ramid) {
> +	case ARM_CA57_L1_I_TAG_RAM:
> +		count = scnprintf(p, space, " RAMID='L1-I Tag RAM'");
> +		break;
> +	case ARM_CA57_L1_I_DATA_RAM:
> +		count = scnprintf(p, space, " RAMID='L1-I Data RAM'");
> +		break;
> +	case ARM_CA57_L1_D_TAG_RAM:
> +		count = scnprintf(p, space, " RAMID='L1-D Tag RAM'");
> +		break;
> +	case ARM_CA57_L1_D_DATA_RAM:
> +		count = scnprintf(p, space, " RAMID='L1-D Data RAM'");
> +		break;
> +	case ARM_CA57_L2_TLB_RAM:
> +		count = scnprintf(p, space, " RAMID='L2 TLB RAM'");
> +		break;
> +	default:
> +		count = scnprintf(p, space, " RAMID='unknown'");
> +		break;
> +	}
> +
> +	p += count;
> +	space -= count;
> +	count = scnprintf(p, space,
> +			  " repeat=%d, other=%d (CPUMERRSR_EL1=0x%llx)",
> +			  repeat, other, val);
> +
> +	lock = edac_dev->pvt_info;

That sure looks funky - why not use a proper private struct like the
rest of the drivers?

> +	spin_lock(lock);
> +	if (fatal)
> +		edac_device_handle_ue_count(edac_dev, repeat, 0, 0, msg);
> +	else
> +		edac_device_handle_ce_count(edac_dev, repeat, 0, 0, msg);
> +	spin_unlock(lock);
> +}
> +
> +static void al_l1_edac_check(struct edac_device_ctl_info *edac_dev)
> +{
> +	on_each_cpu(al_l1_edac_cpumerrsr_read_status, edac_dev, 1);
> +}
> +
> +static int al_l1_edac_probe(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *edac_dev;
> +	struct device *dev = &pdev->dev;
> +	spinlock_t *lock;
> +	int ret;
> +
> +	edac_dev = edac_device_alloc_ctl_info(sizeof(*lock), DRV_NAME, 1, "L",
> +					      1, 1, NULL, 0,
> +					      edac_device_alloc_index());
> +	if (!edac_dev)
> +		return -ENOMEM;
> +
> +	edac_dev->edac_check = al_l1_edac_check;
> +	edac_dev->dev = dev;
> +	edac_dev->mod_name = DRV_NAME;
> +	edac_dev->dev_name = dev_name(dev);
> +	edac_dev->ctl_name = "L1_cache";
> +	platform_set_drvdata(pdev, edac_dev);
> +	lock = edac_dev->pvt_info;
> +
> +	spin_lock_init(lock);
> +
> +	ret = edac_device_add_device(edac_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
> +		edac_device_free_ctl_info(edac_dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int al_l1_edac_remove(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
> +
> +	edac_device_del_device(edac_dev->dev);
> +	edac_device_free_ctl_info(edac_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id al_l1_edac_of_match[] = {
> +	/*
> +	 * "al,alpine-v2", and "amazon,al-alpine-v3" are machine compatible
> +	 * strings which have Cortex-A57/A72 configured with this support,
> +	 * and access to CPUMERRSR_EL1 register is enabled in firmware.
> +	 */
> +	{ .compatible = "al,alpine-v2" },

Err, checkpatch says:

WARNING: DT compatible string "al,alpine-v2" appears un-documented -- check ./Documentation/devicetree/bindings/
#236: FILE: drivers/edac/al_l1_edac.c:151:
+       { .compatible = "al,alpine-v2" },


Do you need a devicetree addition? I usually get such with the ARM EDAC
drivers...

> +	{ .compatible = "amazon,al-alpine-v3" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);
> +
> +static struct platform_driver al_l1_edac_driver = {
> +	.probe = al_l1_edac_probe,
> +	.remove = al_l1_edac_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +};
> +
> +static struct platform_device *edac_l1_device;

Please move to the top of the file.

> +
> +static int __init al_l1_init(void)
> +{
> +	struct device_node *root;
> +	int ret;
> +
> +	root = of_find_node_by_path("/");
> +	if (!root) {
> +		pr_debug("Can't find root node!\n");
> +		return 0;
> +	}
> +
> +	if (!of_match_node(al_l1_edac_of_match, root))
> +		return 0;
> +
> +	ret = platform_driver_register(&al_l1_edac_driver);
> +	if (ret) {
> +		pr_err("Failed to register %s (%d)\n", DRV_NAME, ret);
> +		return ret;
> +	}
> +
> +	edac_l1_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
> +	if (IS_ERR(edac_l1_device)) {
> +		pr_err("Failed to register EDAC AL L1 platform device\n");

		platform_driver_unregister() ?

> +		return PTR_ERR(edac_l1_device);
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit al_l1_exit(void)
> +{
> +	platform_device_unregister(edac_l1_device);
> +	platform_driver_unregister(&al_l1_edac_driver);
> +}
> +
> +late_initcall(al_l1_init);

Why not module_init() ?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

[Hawa, Hanna]

On 2/25/2020 8:52 PM, Borislav Petkov wrote:
> On Wed, Jan 29, 2020 at 09:50:14PM +0200, Hanna Hawa wrote:
> 
>> Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
>> report L1 errors.
> 
> You don't add support for a driver - you either add a driver or you add
> support for HW...
> 
>> diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
>> new file mode 100644
>> index 000000000000..723b35b18f5b
>> --- /dev/null
>> +++ b/drivers/edac/al_l1_edac.c
>> @@ -0,0 +1,207 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
>> + */
>> +
>> +#include <asm/sysreg.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/smp.h>
> 
> asm/ includes go after linux/ includes.

Will be fixed

> 
>> +
>> +#include "edac_device.h"
>> +#include "edac_module.h"
>> +
>> +#define DRV_NAME				"al_l1_edac"
>> +
>> +/* Same bit assignments of CPUMERRSR_EL1 in ARM CA57/CA72 */
>> +#define ARM_CA57_CPUMERRSR_EL1			sys_reg(3, 1, 15, 2, 2)
>> +#define ARM_CA57_CPUMERRSR_RAM_ID		GENMASK(30, 24)
>> +#define  ARM_CA57_L1_I_TAG_RAM			0x00
>> +#define  ARM_CA57_L1_I_DATA_RAM			0x01
>> +#define  ARM_CA57_L1_D_TAG_RAM			0x08
>> +#define  ARM_CA57_L1_D_DATA_RAM			0x09
>> +#define  ARM_CA57_L2_TLB_RAM			0x18
>> +#define ARM_CA57_CPUMERRSR_VALID		BIT(31)
>> +#define ARM_CA57_CPUMERRSR_REPEAT		GENMASK_ULL(39, 32)
>> +#define ARM_CA57_CPUMERRSR_OTHER		GENMASK_ULL(47, 40)
>> +#define ARM_CA57_CPUMERRSR_FATAL		BIT_ULL(63)
>> +
>> +#define AL_L1_EDAC_MSG_MAX			256
>> +
>> +static void al_l1_edac_cpumerrsr_read_status(void *arg)
> 
> This is a static function so you don't really need a function prefix of
> "al_l1_edac_". Use those chars to make its name more descriptive. Ditto
> for the rest of the static functions in that file.

Will be fixed.

> 
>> +{
>> +	struct edac_device_ctl_info *edac_dev = arg;
>> +	int cpu, space, count;
>> +	u32 ramid, repeat, other, fatal;
>> +	u64 val;
>> +	char msg[AL_L1_EDAC_MSG_MAX];
>> +	char *p;
>> +	spinlock_t *lock;
> 
> Please sort function local variables declaration in a reverse christmas
> tree order:
> 
> 	<type A> longest_variable_name;
> 	<type B> shorter_var_name;
> 	<type C> even_shorter;
> 	<type D> i;

Will be fixed

> 
>> +
>> +	val = read_sysreg_s(ARM_CA57_CPUMERRSR_EL1);
>> +	if (!(FIELD_GET(ARM_CA57_CPUMERRSR_VALID, val)))
>> +		return;
>> +
>> +	write_sysreg_s(0, ARM_CA57_CPUMERRSR_EL1);
>> +
>> +	cpu = smp_processor_id();
>> +	ramid = FIELD_GET(ARM_CA57_CPUMERRSR_RAM_ID, val);
>> +	repeat = FIELD_GET(ARM_CA57_CPUMERRSR_REPEAT, val);
>> +	other = FIELD_GET(ARM_CA57_CPUMERRSR_OTHER, val);
>> +	fatal = FIELD_GET(ARM_CA57_CPUMERRSR_FATAL, val);
>> +
>> +	space = sizeof(msg);
>> +	p = msg;
>> +	count = scnprintf(p, space, "CPU%d L1 %serror detected", cpu,
>> +			  (fatal) ? "Fatal " : "");
>> +	p += count;
>> +	space -= count;
>> +
>> +	switch (ramid) {
>> +	case ARM_CA57_L1_I_TAG_RAM:
>> +		count = scnprintf(p, space, " RAMID='L1-I Tag RAM'");
>> +		break;
>> +	case ARM_CA57_L1_I_DATA_RAM:
>> +		count = scnprintf(p, space, " RAMID='L1-I Data RAM'");
>> +		break;
>> +	case ARM_CA57_L1_D_TAG_RAM:
>> +		count = scnprintf(p, space, " RAMID='L1-D Tag RAM'");
>> +		break;
>> +	case ARM_CA57_L1_D_DATA_RAM:
>> +		count = scnprintf(p, space, " RAMID='L1-D Data RAM'");
>> +		break;
>> +	case ARM_CA57_L2_TLB_RAM:
>> +		count = scnprintf(p, space, " RAMID='L2 TLB RAM'");
>> +		break;
>> +	default:
>> +		count = scnprintf(p, space, " RAMID='unknown'");
>> +		break;
>> +	}
>> +
>> +	p += count;
>> +	space -= count;
>> +	count = scnprintf(p, space,
>> +			  " repeat=%d, other=%d (CPUMERRSR_EL1=0x%llx)",
>> +			  repeat, other, val);
>> +
>> +	lock = edac_dev->pvt_info;
> 
> That sure looks funky - why not use a proper private struct like the
> rest of the drivers?

Then the structure will include single variable..

> 
>> +	spin_lock(lock);
>> +	if (fatal)
>> +		edac_device_handle_ue_count(edac_dev, repeat, 0, 0, msg);
>> +	else
>> +		edac_device_handle_ce_count(edac_dev, repeat, 0, 0, msg);
>> +	spin_unlock(lock);
>> +}
>> +
>> +static void al_l1_edac_check(struct edac_device_ctl_info *edac_dev)
>> +{
>> +	on_each_cpu(al_l1_edac_cpumerrsr_read_status, edac_dev, 1);
>> +}
>> +
>> +static int al_l1_edac_probe(struct platform_device *pdev)
>> +{
>> +	struct edac_device_ctl_info *edac_dev;
>> +	struct device *dev = &pdev->dev;
>> +	spinlock_t *lock;
>> +	int ret;
>> +
>> +	edac_dev = edac_device_alloc_ctl_info(sizeof(*lock), DRV_NAME, 1, "L",
>> +					      1, 1, NULL, 0,
>> +					      edac_device_alloc_index());
>> +	if (!edac_dev)
>> +		return -ENOMEM;
>> +
>> +	edac_dev->edac_check = al_l1_edac_check;
>> +	edac_dev->dev = dev;
>> +	edac_dev->mod_name = DRV_NAME;
>> +	edac_dev->dev_name = dev_name(dev);
>> +	edac_dev->ctl_name = "L1_cache";
>> +	platform_set_drvdata(pdev, edac_dev);
>> +	lock = edac_dev->pvt_info;
>> +
>> +	spin_lock_init(lock);
>> +
>> +	ret = edac_device_add_device(edac_dev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
>> +		edac_device_free_ctl_info(edac_dev);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int al_l1_edac_remove(struct platform_device *pdev)
>> +{
>> +	struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
>> +
>> +	edac_device_del_device(edac_dev->dev);
>> +	edac_device_free_ctl_info(edac_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id al_l1_edac_of_match[] = {
>> +	/*
>> +	 * "al,alpine-v2", and "amazon,al-alpine-v3" are machine compatible
>> +	 * strings which have Cortex-A57/A72 configured with this support,
>> +	 * and access to CPUMERRSR_EL1 register is enabled in firmware.
>> +	 */
>> +	{ .compatible = "al,alpine-v2" },
> 
> Err, checkpatch says:
> 
> WARNING: DT compatible string "al,alpine-v2" appears un-documented -- check ./Documentation/devicetree/bindings/
> #236: FILE: drivers/edac/al_l1_edac.c:151:
> +       { .compatible = "al,alpine-v2" },
> 
> 
> Do you need a devicetree addition? I usually get such with the ARM EDAC
> drivers...

There is missing documentation for alpine-v2, there is an open patches 
to add this missing doc. [1]

[1]: https://lkml.org/lkml/2020/2/25/362

> 
>> +	{ .compatible = "amazon,al-alpine-v3" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);
>> +
>> +static struct platform_driver al_l1_edac_driver = {
>> +	.probe = al_l1_edac_probe,
>> +	.remove = al_l1_edac_remove,
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +	},
>> +};
>> +
>> +static struct platform_device *edac_l1_device;
> 
> Please move to the top of the file.

Ack.

> 
>> +
>> +static int __init al_l1_init(void)
>> +{
>> +	struct device_node *root;
>> +	int ret;
>> +
>> +	root = of_find_node_by_path("/");
>> +	if (!root) {
>> +		pr_debug("Can't find root node!\n");
>> +		return 0;
>> +	}
>> +
>> +	if (!of_match_node(al_l1_edac_of_match, root))
>> +		return 0;
>> +
>> +	ret = platform_driver_register(&al_l1_edac_driver);
>> +	if (ret) {
>> +		pr_err("Failed to register %s (%d)\n", DRV_NAME, ret);
>> +		return ret;
>> +	}
>> +
>> +	edac_l1_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
>> +	if (IS_ERR(edac_l1_device)) {
>> +		pr_err("Failed to register EDAC AL L1 platform device\n");
> 
> 		platform_driver_unregister() ?

Ack

> 
>> +		return PTR_ERR(edac_l1_device);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit al_l1_exit(void)
>> +{
>> +	platform_device_unregister(edac_l1_device);
>> +	platform_driver_unregister(&al_l1_edac_driver);
>> +}
>> +
>> +late_initcall(al_l1_init);
> 
> Why not module_init() ?

Will be fixed.

Thanks,
Hanna
> 

Re: [PATCH v9 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC

[Robert Richter]

On 29.01.20 21:50:16, Hanna Hawa wrote:
> Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and
> report L2 errors.
> 
> Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
> ---
>  MAINTAINERS               |   5 +
>  drivers/edac/Kconfig      |   8 ++
>  drivers/edac/Makefile     |   1 +
>  drivers/edac/al_l2_edac.c | 270 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 284 insertions(+)
>  create mode 100644 drivers/edac/al_l2_edac.c

Hanna, most of the review comments by Boris for patch #1 (al_l1_edac)
apply here too. Please address them.

Thanks,

-Robert

Re: [PATCH v9 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC

[Hawa, Hanna]

On 3/10/2020 3:47 PM, Robert Richter wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 29.01.20 21:50:16, Hanna Hawa wrote:
>> Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and
>> report L2 errors.
>>
>> Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
>> ---
>>   MAINTAINERS               |   5 +
>>   drivers/edac/Kconfig      |   8 ++
>>   drivers/edac/Makefile     |   1 +
>>   drivers/edac/al_l2_edac.c | 270 ++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 284 insertions(+)
>>   create mode 100644 drivers/edac/al_l2_edac.c
> 
> Hanna, most of the review comments by Boris for patch #1 (al_l1_edac)
> apply here too. Please address them.

Hi Boris, Robert,

Sorry for not getting back to you sooner, will address comments on both 
files and upload new patchset ASAP.

Thanks,
Hanna

> 
> Thanks,
> 
> -Robert
>