[PATCH] EDAC support for SiFive SoCs

[PATCH] EDAC support for SiFive SoCs

[Yash Shah]

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

[PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs

[Yash Shah]

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

Re: [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs

[James Morse]

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

Re: [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs

[Paul Walmsley]

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

Re: [PATCH] edac: sifive: Add EDAC platform driver for SiFive SoCs

[Yash Shah]

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