* [PATCH] EDAC support for SiFive SoCs @ 2019-05-02 11:16 Yash Shah 2019-05-02 11:16 ` [PATCH] edac: sifive: Add EDAC platform driver " Yash Shah 0 siblings, 1 reply; 5+ messages in thread From: Yash Shah @ 2019-05-02 11:16 UTC (permalink / raw) To: linux-edac, linux-riscv, palmer, bp, james.morse Cc: aou, paulmck, gregkh, linux-kernel, nicolas.ferre, sachin.ghadi, Yash Shah, paul.walmsley, mchehab, davem Adds an EDAC platform driver for SiFive SoCs. This patch was earlier part of the patch series: 'L2 cache controller and EDAC support for SiFive SoCs' https://lkml.org/lkml/2019/4/15/320 In order to merge L2 cache controller driver without any dependency on EDAC, this EDAC patch is re-posted separately with updated MAINTAINERS entry. This patch depends on patch 'RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs' https://lkml.org/lkml/2019/5/2/309 The EDAC driver registers for notifier events from the L2 cache controller driver (arch/riscv/mm/sifive_l2_cache.c) for L2 ECC events The patch is based on Linux 5.1-rc2 and tested on HiFive Unleashed board with additional board related patches needed for testing can be found at dev/yashs/L2_cache_controller branch of: https://github.com/yashshah7/riscv-linux.git Yash Shah (1): edac: sifive: Add EDAC platform driver for SiFive SoCs MAINTAINERS | 6 +++ arch/riscv/Kconfig | 1 + drivers/edac/Kconfig | 6 +++ drivers/edac/Makefile | 1 + drivers/edac/sifive_edac.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+) create mode 100644 drivers/edac/sifive_edac.c -- 1.9.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs 2019-05-02 11:16 [PATCH] EDAC support for SiFive SoCs Yash Shah @ 2019-05-02 11:16 ` Yash Shah 2019-05-02 16:42 ` James Morse 0 siblings, 1 reply; 5+ messages in thread From: Yash Shah @ 2019-05-02 11:16 UTC (permalink / raw) To: linux-edac, linux-riscv, palmer, bp, james.morse Cc: aou, paulmck, gregkh, linux-kernel, nicolas.ferre, sachin.ghadi, Yash Shah, paul.walmsley, mchehab, davem The initial ver of EDAC driver supports: - ECC event monitoring and reporting through the EDAC framework for SiFive L2 cache controller. This patch depends on patch 'RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs' https://lkml.org/lkml/2019/5/2/309 The EDAC driver registers for notifier events from the L2 cache controller driver (arch/riscv/mm/sifive_l2_cache.c) for L2 ECC events Signed-off-by: Yash Shah <yash.shah@sifive.com> --- MAINTAINERS | 6 +++ arch/riscv/Kconfig | 1 + drivers/edac/Kconfig | 6 +++ drivers/edac/Makefile | 1 + drivers/edac/sifive_edac.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+) create mode 100644 drivers/edac/sifive_edac.c diff --git a/MAINTAINERS b/MAINTAINERS index ba4f104..6e433db 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5679,6 +5679,12 @@ L: linux-edac@vger.kernel.org S: Maintained F: drivers/edac/sb_edac.c +EDAC-SIFIVE +M: Yash Shah <yash.shah@sifive.com> +L: linux-edac@vger.kernel.org +S: Maintained +F: drivers/edac/sifive_edac.c + EDAC-SKYLAKE M: Tony Luck <tony.luck@intel.com> L: linux-edac@vger.kernel.org diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index eb56c82..31999a6 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -49,6 +49,7 @@ config RISCV select GENERIC_IRQ_MULTI_HANDLER select ARCH_HAS_PTE_SPECIAL select HAVE_EBPF_JIT if 64BIT + select EDAC_SUPPORT config MMU def_bool y diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index 47eb4d1..3e05228 100644 --- a/drivers/edac/Kconfig +++ b/drivers/edac/Kconfig @@ -460,6 +460,12 @@ config EDAC_ALTERA_SDMMC Support for error detection and correction on the Altera SDMMC FIFO Memory for Altera SoCs. +config EDAC_SIFIVE + bool "Sifive platform EDAC driver" + depends on EDAC=y && RISCV + help + Support for error detection and correction on the SiFive SoCs. + config EDAC_SYNOPSYS tristate "Synopsys DDR Memory Controller" depends on ARCH_ZYNQ || ARCH_ZYNQMP diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index 89ad4a84..165ca65e 100644 --- a/drivers/edac/Makefile +++ b/drivers/edac/Makefile @@ -79,6 +79,7 @@ obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o obj-$(CONFIG_EDAC_THUNDERX) += thunderx_edac.o obj-$(CONFIG_EDAC_ALTERA) += altera_edac.o +obj-$(CONFIG_EDAC_SIFIVE) += sifive_edac.o obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o obj-$(CONFIG_EDAC_TI) += ti_edac.o diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c new file mode 100644 index 0000000..eb7a9b9 --- /dev/null +++ b/drivers/edac/sifive_edac.c @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * SiFive Platform EDAC Driver + * + * Copyright (C) 2018-2019 SiFive, Inc. + * + * This driver is partially based on octeon_edac-pc.c + * + */ +#include <linux/edac.h> +#include <linux/platform_device.h> +#include "edac_module.h" + +#define DRVNAME "sifive_edac" + +extern int register_sifive_l2_error_notifier(struct notifier_block *nb); +extern int unregister_sifive_l2_error_notifier(struct notifier_block *nb); + +struct sifive_edac_priv { + struct notifier_block notifier; + struct edac_device_ctl_info *dci; +}; + +/** + * EDAC error callback + * + * @event: non-zero if unrecoverable. + */ +static +int ecc_err_event(struct notifier_block *this, unsigned long event, void *ptr) +{ + const char *msg = (char *)ptr; + struct sifive_edac_priv *p; + + p = container_of(this, struct sifive_edac_priv, notifier); + + if (event) + edac_device_handle_ue(p->dci, 0, 0, msg); + else + edac_device_handle_ce(p->dci, 0, 0, msg); + + return NOTIFY_STOP; +} + +static int ecc_register(struct platform_device *pdev) +{ + struct sifive_edac_priv *p; + + p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL); + if (!p) + return -ENOMEM; + + p->notifier.notifier_call = ecc_err_event; + platform_set_drvdata(pdev, p); + + p->dci = edac_device_alloc_ctl_info(sizeof(*p), "sifive_ecc", 1, + "sifive_ecc", 1, 1, NULL, 0, + edac_device_alloc_index()); + if (IS_ERR(p->dci)) + return PTR_ERR(p->dci); + + p->dci->dev = &pdev->dev; + p->dci->mod_name = "Sifive ECC Manager"; + p->dci->ctl_name = dev_name(&pdev->dev); + p->dci->dev_name = dev_name(&pdev->dev); + + if (edac_device_add_device(p->dci)) { + dev_err(p->dci->dev, "failed to register with EDAC core\n"); + goto err; + } + + register_sifive_l2_error_notifier(&p->notifier); + + return 0; + +err: + edac_device_free_ctl_info(p->dci); + + return -ENXIO; +} + +static int ecc_unregister(struct platform_device *pdev) +{ + struct sifive_edac_priv *p = platform_get_drvdata(pdev); + + unregister_sifive_l2_error_notifier(&p->notifier); + edac_device_del_device(&pdev->dev); + edac_device_free_ctl_info(p->dci); + + return 0; +} + +struct platform_device *sifive_pdev; + +static int __init sifive_edac_init(void) +{ + int ret; + + sifive_pdev = platform_device_register_simple(DRVNAME, 0, NULL, 0); + if (IS_ERR(sifive_pdev)) + return PTR_ERR(sifive_pdev); + + ret = ecc_register(sifive_pdev); + if (ret) + platform_device_unregister(sifive_pdev); + + return ret; +} + +static void __exit sifive_edac_exit(void) +{ + ecc_unregister(sifive_pdev); + platform_device_unregister(sifive_pdev); +} + +module_init(sifive_edac_init); +module_exit(sifive_edac_exit); + +MODULE_AUTHOR("SiFive Inc."); +MODULE_DESCRIPTION("SiFive platform EDAC driver"); +MODULE_LICENSE("GPL v2"); -- 1.9.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs 2019-05-02 11:16 ` [PATCH] edac: sifive: Add EDAC platform driver " Yash Shah @ 2019-05-02 16:42 ` James Morse 2019-05-03 19:25 ` Paul Walmsley 2019-05-06 9:50 ` Yash Shah 0 siblings, 2 replies; 5+ messages in thread From: James Morse @ 2019-05-02 16:42 UTC (permalink / raw) To: Yash Shah Cc: aou, paulmck, gregkh, palmer, linux-kernel, nicolas.ferre, sachin.ghadi, bp, paul.walmsley, linux-riscv, mchehab, davem, linux-edac Hi Yash, Sorry for the delay on the earlier version of this - I was trying to work out what happens when multiple edac drivers probe based on DT... On 02/05/2019 12:16, Yash Shah wrote: > The initial ver of EDAC driver supports: > - ECC event monitoring and reporting through the EDAC framework for SiFive > L2 cache controller. > You probably don't want this bit preserved in the kernel log: { > This patch depends on patch > 'RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs' > https://lkml.org/lkml/2019/5/2/309 } > The EDAC driver registers for notifier events from the L2 cache controller > driver (arch/riscv/mm/sifive_l2_cache.c) for L2 ECC events > > Signed-off-by: Yash Shah <yash.shah@sifive.com> > --- (if you put it here, it gets discarded when the patch is applied) Having an separately posted dependency like this is tricky, as this code can't be used/tested until the other bits are merged. > MAINTAINERS | 6 +++ > arch/riscv/Kconfig | 1 + > drivers/edac/Kconfig | 6 +++ > drivers/edac/Makefile | 1 + > drivers/edac/sifive_edac.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 135 insertions(+) > create mode 100644 drivers/edac/sifive_edac.c > diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c > new file mode 100644 > index 0000000..eb7a9b9 > --- /dev/null > +++ b/drivers/edac/sifive_edac.c > @@ -0,0 +1,121 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * SiFive Platform EDAC Driver > + * > + * Copyright (C) 2018-2019 SiFive, Inc. > + * > + * This driver is partially based on octeon_edac-pc.c > + * > + */ > +#include <linux/edac.h> > +#include <linux/platform_device.h> > +#include "edac_module.h" > + > +#define DRVNAME "sifive_edac" > + > +extern int register_sifive_l2_error_notifier(struct notifier_block *nb); > +extern int unregister_sifive_l2_error_notifier(struct notifier_block *nb); Ideally these would live in some header file. > +struct sifive_edac_priv { > + struct notifier_block notifier; > + struct edac_device_ctl_info *dci; > +}; > + > +/** > + * EDAC error callback > + * > + * @event: non-zero if unrecoverable. > + */ > +static > +int ecc_err_event(struct notifier_block *this, unsigned long event, void *ptr) > +{ > + const char *msg = (char *)ptr; > + struct sifive_edac_priv *p; > + > + p = container_of(this, struct sifive_edac_priv, notifier); > + > + if (event) > + edac_device_handle_ue(p->dci, 0, 0, msg); > + else > + edac_device_handle_ce(p->dci, 0, 0, msg); This would be easier to read if your SIFIVE_L2_ERR_TYPE_UE were exposed via some header file. > + > + return NOTIFY_STOP; Your notifier register calls are EXPORT_SYMBOL()d, but Kconfig forbids building this as a module, so its not for this driver. If there is another user of this notifier-chain, won't NOTIFY_STOP here break it? > +} > + > +static int ecc_register(struct platform_device *pdev) > +{ > + struct sifive_edac_priv *p; > + > + p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL); > + if (!p) > + return -ENOMEM; > + > + p->notifier.notifier_call = ecc_err_event; > + platform_set_drvdata(pdev, p); > + > + p->dci = edac_device_alloc_ctl_info(sizeof(*p), "sifive_ecc", 1, sizeof(*p) here is how much space in struct edac_device_ctl_info you need for private storage... but you never touch p->dci->pvt_info, so you aren't using it. 0? > + "sifive_ecc", 1, 1, NULL, 0, > + edac_device_alloc_index()); > + if (IS_ERR(p->dci)) > + return PTR_ERR(p->dci); > + > + p->dci->dev = &pdev->dev; > + p->dci->mod_name = "Sifive ECC Manager"; > + p->dci->ctl_name = dev_name(&pdev->dev); > + p->dci->dev_name = dev_name(&pdev->dev); > + > + if (edac_device_add_device(p->dci)) { > + dev_err(p->dci->dev, "failed to register with EDAC core\n"); > + goto err; > + } > + > + register_sifive_l2_error_notifier(&p->notifier); > + > + return 0; > + > +err: > + edac_device_free_ctl_info(p->dci); > + > + return -ENXIO; > +} > +struct platform_device *sifive_pdev; static? > +static int __init sifive_edac_init(void) > +{ > + int ret; > + > + sifive_pdev = platform_device_register_simple(DRVNAME, 0, NULL, 0); > + if (IS_ERR(sifive_pdev)) > + return PTR_ERR(sifive_pdev); > + > + ret = ecc_register(sifive_pdev); > + if (ret) > + platform_device_unregister(sifive_pdev); > + > + return ret; > +} > + > +static void __exit sifive_edac_exit(void) > +{ > + ecc_unregister(sifive_pdev); > + platform_device_unregister(sifive_pdev); > +} Looks good to me. I think this patch should go with its two dependencies, I'm not sure why it got split off... Reviewed-by: James Morse <james.morse@arm.com> Thanks, James _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs 2019-05-02 16:42 ` James Morse @ 2019-05-03 19:25 ` Paul Walmsley 2019-05-06 9:50 ` Yash Shah 1 sibling, 0 replies; 5+ messages in thread From: Paul Walmsley @ 2019-05-03 19:25 UTC (permalink / raw) To: James Morse Cc: aou, paulmck, gregkh, palmer, linux-kernel, nicolas.ferre, sachin.ghadi, Yash Shah, bp, paul.walmsley, linux-riscv, mchehab, davem, linux-edac Hi James, On Thu, 2 May 2019, James Morse wrote: > Having an separately posted dependency like this is tricky, as this code can't be > used/tested until the other bits are merged. ... > Looks good to me. I think this patch should go with its two dependencies, I'm not sure why > it got split off... The split was due to my suggestion to Yash, I think. The motivation was to decouple the L2 cache controller driver's journey upstream from the EDAC driver's upstream path. The patches will go up via separate trees, so the idea was to avoid blocking the L2 cache controller driver on the EDAC driver review path. Thanks for your review, - Paul _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs 2019-05-02 16:42 ` James Morse 2019-05-03 19:25 ` Paul Walmsley @ 2019-05-06 9:50 ` Yash Shah 1 sibling, 0 replies; 5+ messages in thread From: Yash Shah @ 2019-05-06 9:50 UTC (permalink / raw) To: James Morse Cc: aou, paulmck, gregkh, Palmer Dabbelt, linux-kernel, nicolas.ferre, Sachin Ghadi, Borislav Petkov, Paul Walmsley, linux-riscv, mchehab, davem, linux-edac Hi james, On Thu, May 2, 2019 at 10:12 PM James Morse <james.morse@arm.com> wrote: > > Hi Yash, > > Sorry for the delay on the earlier version of this - I was trying to work out what happens > when multiple edac drivers probe based on DT... > > > On 02/05/2019 12:16, Yash Shah wrote: > > The initial ver of EDAC driver supports: > > - ECC event monitoring and reporting through the EDAC framework for SiFive > > L2 cache controller. > > > > You probably don't want this bit preserved in the kernel log: > { > > > This patch depends on patch > > 'RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs' > > https://lkml.org/lkml/2019/5/2/309 > > } > > > The EDAC driver registers for notifier events from the L2 cache controller > > driver (arch/riscv/mm/sifive_l2_cache.c) for L2 ECC events > > > > Signed-off-by: Yash Shah <yash.shah@sifive.com> > > --- > > (if you put it here, it gets discarded when the patch is applied) Ok, will move it down here. > > Having an separately posted dependency like this is tricky, as this code can't be > used/tested until the other bits are merged. > > > > MAINTAINERS | 6 +++ > > arch/riscv/Kconfig | 1 + > > drivers/edac/Kconfig | 6 +++ > > drivers/edac/Makefile | 1 + > > drivers/edac/sifive_edac.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 135 insertions(+) > > create mode 100644 drivers/edac/sifive_edac.c > > > diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c > > new file mode 100644 > > index 0000000..eb7a9b9 > > --- /dev/null > > +++ b/drivers/edac/sifive_edac.c > > @@ -0,0 +1,121 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * SiFive Platform EDAC Driver > > + * > > + * Copyright (C) 2018-2019 SiFive, Inc. > > + * > > + * This driver is partially based on octeon_edac-pc.c > > + * > > + */ > > +#include <linux/edac.h> > > +#include <linux/platform_device.h> > > +#include "edac_module.h" > > + > > +#define DRVNAME "sifive_edac" > > + > > +extern int register_sifive_l2_error_notifier(struct notifier_block *nb); > > +extern int unregister_sifive_l2_error_notifier(struct notifier_block *nb); > > Ideally these would live in some header file. Will move the externs in sifive_l2_cache header file > > > > +struct sifive_edac_priv { > > + struct notifier_block notifier; > > + struct edac_device_ctl_info *dci; > > +}; > > + > > +/** > > + * EDAC error callback > > + * > > + * @event: non-zero if unrecoverable. > > + */ > > +static > > +int ecc_err_event(struct notifier_block *this, unsigned long event, void *ptr) > > +{ > > + const char *msg = (char *)ptr; > > + struct sifive_edac_priv *p; > > + > > + p = container_of(this, struct sifive_edac_priv, notifier); > > + > > + if (event) > > + edac_device_handle_ue(p->dci, 0, 0, msg); > > + else > > + edac_device_handle_ce(p->dci, 0, 0, msg); > > This would be easier to read if your SIFIVE_L2_ERR_TYPE_UE were exposed via some header file. sure. > > > > + > > + return NOTIFY_STOP; > > Your notifier register calls are EXPORT_SYMBOL()d, but Kconfig forbids building this as a > module, so its not for this driver. If there is another user of this notifier-chain, won't > NOTIFY_STOP here break it? > Yes, you are right. Will change it to NOTIFY_OK > > > +} > > + > > +static int ecc_register(struct platform_device *pdev) > > +{ > > + struct sifive_edac_priv *p; > > + > > + p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL); > > + if (!p) > > + return -ENOMEM; > > + > > + p->notifier.notifier_call = ecc_err_event; > > + platform_set_drvdata(pdev, p); > > + > > + p->dci = edac_device_alloc_ctl_info(sizeof(*p), "sifive_ecc", 1, > > sizeof(*p) here is how much space in struct edac_device_ctl_info you need for private > storage... but you never touch p->dci->pvt_info, so you aren't using it. > > 0? Yes, will change it. > > > > + "sifive_ecc", 1, 1, NULL, 0, > > + edac_device_alloc_index()); > > + if (IS_ERR(p->dci)) > > + return PTR_ERR(p->dci); > > + > > + p->dci->dev = &pdev->dev; > > + p->dci->mod_name = "Sifive ECC Manager"; > > + p->dci->ctl_name = dev_name(&pdev->dev); > > + p->dci->dev_name = dev_name(&pdev->dev); > > + > > + if (edac_device_add_device(p->dci)) { > > + dev_err(p->dci->dev, "failed to register with EDAC core\n"); > > + goto err; > > + } > > + > > + register_sifive_l2_error_notifier(&p->notifier); > > + > > + return 0; > > + > > +err: > > + edac_device_free_ctl_info(p->dci); > > + > > + return -ENXIO; > > +} > > > +struct platform_device *sifive_pdev; > > static? Yes, will change this too. > > > > +static int __init sifive_edac_init(void) > > +{ > > + int ret; > > + > > + sifive_pdev = platform_device_register_simple(DRVNAME, 0, NULL, 0); > > + if (IS_ERR(sifive_pdev)) > > + return PTR_ERR(sifive_pdev); > > + > > + ret = ecc_register(sifive_pdev); > > + if (ret) > > + platform_device_unregister(sifive_pdev); > > + > > + return ret; > > +} > > + > > +static void __exit sifive_edac_exit(void) > > +{ > > + ecc_unregister(sifive_pdev); > > + platform_device_unregister(sifive_pdev); > > +} > > Looks good to me. I think this patch should go with its two dependencies, I'm not sure why > it got split off... > > Reviewed-by: James Morse <james.morse@arm.com> > Thanks for your review. - Yash > > Thanks, > > James _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-06 9:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-02 11:16 [PATCH] EDAC support for SiFive SoCs Yash Shah 2019-05-02 11:16 ` [PATCH] edac: sifive: Add EDAC platform driver " Yash Shah 2019-05-02 16:42 ` James Morse 2019-05-03 19:25 ` Paul Walmsley 2019-05-06 9:50 ` Yash Shah
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).