Message ID | 20191007151730.7705-1-hhhawa@amazon.com |
---|---|
Headers | show |
Series | Add support for Amazon's Annapurna Labs EDAC for L1/L2 | expand |
+Sudeep On Mon, Oct 7, 2019 at 10:18 AM Hanna Hawa <hhhawa@amazon.com> wrote: > > Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and > report L2 errors. I was curious why you needed a DT cache parsing function... [...] > +static int al_l2_edac_probe(struct platform_device *pdev) > +{ > + struct edac_device_ctl_info *edac_dev; > + struct al_l2_edac *al_l2; > + 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); > + > + INIT_LIST_HEAD(&al_l2->l2_caches); > + > + for_each_possible_cpu(i) { > + struct device_node *cpu; > + struct device_node *cpu_cache; > + struct al_l2_cache *l2_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); > + } 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); > + } We already have what's probably similar code to parse DT and populate cacheinfo data. Does that not work for you? If not, why not and can we extend it? Then your driver might work if the data comes from ACPI instead (or maybe that's all different, I don't know). Rob
On Mon, Oct 7, 2019 at 10:18 AM Hanna Hawa <hhhawa@amazon.com> 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 | 251 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 265 insertions(+) > create mode 100644 drivers/edac/al_l2_edac.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 7887a62dc843..0eabcfcf91a9 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -748,6 +748,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 e8161d7c7469..cb394aff1cab 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" I still think this should be an "A57 L2 ECC" driver, but if no one cares I'll shut up and the 2nd person can rename everything. > + depends on ARCH_ALPINE || COMPILE_TEST Maybe it needs an ARM64 dependency too in this case? > + help > + Support for L2 error detection and correction > + for Amazon's Annapurna Labs SoCs. > + This driver detects errors of L2 caches. > + > + > + 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 >
Hi, On 10/10/2019 2:19 AM, Rob Herring wrote: > On Mon, Oct 7, 2019 at 10:18 AM Hanna Hawa <hhhawa@amazon.com> 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 | 251 ++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 265 insertions(+) >> create mode 100644 drivers/edac/al_l2_edac.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 7887a62dc843..0eabcfcf91a9 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -748,6 +748,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 e8161d7c7469..cb394aff1cab 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" > > I still think this should be an "A57 L2 ECC" driver, but if no one > cares I'll shut up and the 2nd person can rename everything. > >> + depends on ARCH_ALPINE > > || COMPILE_TEST Will be add in next patchset. > > Maybe it needs an ARM64 dependency too in this case? Yes, it need ARM64 dependency, I'll add. Thanks, Hanna > >> + help >> + Support for L2 error detection and correction >> + for Amazon's Annapurna Labs SoCs. >> + This driver detects errors of L2 caches. >> + > >> + >> + 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 >>
On 10/10/2019 2:09 AM, Rob Herring wrote: > +Sudeep > > On Mon, Oct 7, 2019 at 10:18 AM Hanna Hawa <hhhawa@amazon.com> wrote: >> >> Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and >> report L2 errors. > > I was curious why you needed a DT cache parsing function... > > [...] > >> +static int al_l2_edac_probe(struct platform_device *pdev) >> +{ >> + struct edac_device_ctl_info *edac_dev; >> + struct al_l2_edac *al_l2; >> + 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); >> + >> + INIT_LIST_HEAD(&al_l2->l2_caches); >> + >> + for_each_possible_cpu(i) { >> + struct device_node *cpu; >> + struct device_node *cpu_cache; >> + struct al_l2_cache *l2_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); >> + } 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); >> + } > > We already have what's probably similar code to parse DT and populate > cacheinfo data. Does that not work for you? If not, why not and can we > extend it? As I saw in cacheinfo it will return the cacheinfo for the online CPUs only, correct me if I'm wrong.. Here I'm parsing all the L2 info for all CPUs depends on DT to get "cluster_cpus", and using smp_call_function_any() will call the online cpu to read the L2MERRSR status register. > > Then your driver might work if the data comes from ACPI instead (or > maybe that's all different, I don't know). No plan to get it work on ACPI, at least in the near future. > > Rob >