2019-10-15 12:28:51

by Hanna Hawa

[permalink] [raw]
Subject: [PATCH v7 0/3] Add support for Amazon's Annapurna Labs EDAC for L1/L2

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 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 | 190 +++++++++++++++++++++++++++++
drivers/edac/al_l2_edac.c | 251 ++++++++++++++++++++++++++++++++++++++
drivers/of/base.c | 1 +
6 files changed, 470 insertions(+)
create mode 100644 drivers/edac/al_l1_edac.c
create mode 100644 drivers/edac/al_l2_edac.c

--
2.17.1


2019-10-15 12:31:27

by Hanna Hawa

[permalink] [raw]
Subject: [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

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

Signed-off-by: Hanna Hawa <[email protected]>
---
MAINTAINERS | 5 +
drivers/edac/Kconfig | 8 ++
drivers/edac/Makefile | 1 +
drivers/edac/al_l1_edac.c | 190 ++++++++++++++++++++++++++++++++++++++
4 files changed, 204 insertions(+)
create mode 100644 drivers/edac/al_l1_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 77eae44bf5de..7887a62dc843 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -743,6 +743,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 <[email protected]>
+S: Maintained
+F: drivers/edac/al_l1_edac.c
+
AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER
M: Talel Shenhar <[email protected]>
S: Maintained
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 200c04ce5b0e..e26c54216519 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 165ca65e1a3a..caa2dc91e8a0 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..e363a80b4d13
--- /dev/null
+++ b/drivers/edac/al_l1_edac.c
@@ -0,0 +1,190 @@
+// 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/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, i, space, count;
+ u32 ramid, repeat, other, fatal;
+ u64 val;
+ char msg[AL_L1_EDAC_MSG_MAX];
+ char *p;
+
+ 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);
+
+ for (i = 0; i < repeat; i++) {
+ if (fatal)
+ edac_device_handle_ue(edac_dev, 0, 0, msg);
+ else
+ edac_device_handle_ce(edac_dev, 0, 0, msg);
+ }
+}
+
+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;
+ int ret;
+
+ edac_dev = edac_device_alloc_ctl_info(0, 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);
+
+ ret = edac_device_add_device(edac_dev);
+ if (ret)
+ goto err;
+
+ return 0;
+err:
+ dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
+ edac_device_free_ctl_info(edac_dev);
+
+ return ret;
+}
+
+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[] = {
+ { .compatible = "al,alpine-v2" },
+ { .compatible = "amazon,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 = of_find_node_by_path("/");
+ int ret;
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("Amazon's Annapurna Lab's L1 EDAC Driver");
--
2.17.1

2019-10-15 12:31:31

by Hanna Hawa

[permalink] [raw]
Subject: [PATCH v7 2/3] of: EXPORT_SYMBOL_GPL of_find_next_cache_node

Make of_find_next_cache_node() available for modules.

Signed-off-by: Hanna Hawa <[email protected]>
---
drivers/of/base.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20e0e7ee4edf..fe22c7428958 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2172,6 +2172,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

2019-10-18 16:15:31

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v7 2/3] of: EXPORT_SYMBOL_GPL of_find_next_cache_node

On Tue, 15 Oct 2019 13:09:26 +0100, Hanna Hawa wrote:
> Make of_find_next_cache_node() available for modules.
>
> Signed-off-by: Hanna Hawa <[email protected]>
> ---
> drivers/of/base.c | 1 +
> 1 file changed, 1 insertion(+)
>

Acked-by: Rob Herring <[email protected]>

2020-01-15 18:51:26

by James Morse

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

Hi Hanna,

(This was still on my list. I've not seen a newer version, its not in next, and it still
applies, so:)

On 15/10/2019 13:09, Hanna Hawa wrote:
> Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
> report L1 errors.

> diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
> new file mode 100644
> index 000000000000..e363a80b4d13
> --- /dev/null
> +++ b/drivers/edac/al_l1_edac.c
> @@ -0,0 +1,190 @@

> +#include <asm/sysreg.h>
> +#include <linux/bitfield.h>
> +#include <linux/of.h>
> +#include <linux/smp.h>

You need <linux/platform_device.h> for platform_device_register_simple().

[...]

> +static void al_l1_edac_cpumerrsr_read_status(void *arg)
> +{

> + for (i = 0; i < repeat; i++) {
> + if (fatal)
> + edac_device_handle_ue(edac_dev, 0, 0, msg);
> + else
> + edac_device_handle_ce(edac_dev, 0, 0, msg);
> + }

What serialises these? You kick this off from on_each_cpu(), what stops two CPUs calling
this at the same time? 'edac_dev->counters.ce_count += count;' will go wrong in this case.

I think you need a spinlock around the edac_device_* calls that take edac_dev so that only
one occurs at a time.


> +}
> +
> +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;
> + int ret;
> +
> + edac_dev = edac_device_alloc_ctl_info(0, 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);
> +
> + ret = edac_device_add_device(edac_dev);
> + if (ret)
> + goto err;
> +
> + return 0;
> +err:

(this goto has one user, meaning you can remove it by restructuring the code)


> + dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
> + edac_device_free_ctl_info(edac_dev);
> +
> + return ret;
> +}
> +

> +static const struct of_device_id al_l1_edac_of_match[] = {
> + { .compatible = "al,alpine-v2" },
> + { .compatible = "amazon,alpine-v3" },
> + {}
> +};

Unusually these are machine compatibles. It may be worth a comment that these are the
platforms which are known to have Cortex-A57/A72 configured with this support, and access
to the registers enabled by firmware.


> +MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);

[..]

> +static int __init al_l1_init(void)
> +{
> + struct device_node *root = of_find_node_by_path("/");
> + int ret;

root could be NULL here.


> + 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;
> +}

With the edac_device_handle_ce() race fixed:
Reviewed-by: James Morse <[email protected]>


Thanks,

James

2020-01-20 14:44:22

by Hanna Hawa

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC



On 1/15/2020 8:49 PM, James Morse wrote:
> Hi Hanna,
>
> (This was still on my list. I've not seen a newer version, its not in next, and it still
> applies, so:)

Thank you.

>
> On 15/10/2019 13:09, Hanna Hawa wrote:
>> Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
>> report L1 errors.
>
>> diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
>> new file mode 100644
>> index 000000000000..e363a80b4d13
>> --- /dev/null
>> +++ b/drivers/edac/al_l1_edac.c
>> @@ -0,0 +1,190 @@
>
>> +#include <asm/sysreg.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/of.h>
>> +#include <linux/smp.h>
>
> You need <linux/platform_device.h> for platform_device_register_simple().

Will be added in next PS.

>
> [...]
>
>> +static void al_l1_edac_cpumerrsr_read_status(void *arg)
>> +{
>
>> + for (i = 0; i < repeat; i++) {
>> + if (fatal)
>> + edac_device_handle_ue(edac_dev, 0, 0, msg);
>> + else
>> + edac_device_handle_ce(edac_dev, 0, 0, msg);
>> + }
>
> What serialises these? You kick this off from on_each_cpu(), what stops two CPUs calling
> this at the same time? 'edac_dev->counters.ce_count += count;' will go wrong in this case.
>
> I think you need a spinlock around the edac_device_* calls that take edac_dev so that only
> one occurs at a time.

Agree with you, will add spinlock in next PS.

>
>
>> +}
>> +
>> +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;
>> + int ret;
>> +
>> + edac_dev = edac_device_alloc_ctl_info(0, 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);
>> +
>> + ret = edac_device_add_device(edac_dev);
>> + if (ret)
>> + goto err;
>> +
>> + return 0;
>> +err:
>
> (this goto has one user, meaning you can remove it by restructuring the code)

Will be fixed in next PS.

>
>
>> + dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
>> + edac_device_free_ctl_info(edac_dev);
>> +
>> + return ret;
>> +}
>> +
>
>> +static const struct of_device_id al_l1_edac_of_match[] = {
>> + { .compatible = "al,alpine-v2" },
>> + { .compatible = "amazon,alpine-v3" },
>> + {}
>> +};
>
> Unusually these are machine compatibles. It may be worth a comment that these are the
> platforms which are known to have Cortex-A57/A72 configured with this support, and access
> to the registers enabled by firmware.

Will be added.

>
>
>> +MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);
>
> [..]
>
>> +static int __init al_l1_init(void)
>> +{
>> + struct device_node *root = of_find_node_by_path("/");
>> + int ret;
>
> root could be NULL here.

Will be fixed.

>
>
>> + 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;
>> +}
>
> With the edac_device_handle_ce() race fixed:
> Reviewed-by: James Morse <[email protected]>

Thanks,
Hanna

>
>
> Thanks,
>
> James
>