RE: [PATCH 3/3] edac: sifive: Add EDAC support for Memory Controller in SiFive SoCs
> -Original Message-
> From: Borislav Petkov
> Sent: 31 August 2020 14:22
> To: Yash Shah
> Cc: robh...@kernel.org; pal...@dabbelt.com; Paul Walmsley ( Sifive)
> ; mche...@kernel.org; tony.l...@intel.com;
> a...@eecs.berkeley.edu; james.mo...@arm.com; rrich...@marvell.com;
> devicet...@vger.kernel.org; linux-ri...@lists.infradead.org; linux-
> ker...@vger.kernel.org; linux-e...@vger.kernel.org; Sachin Ghadi
>
> Subject: Re: [PATCH 3/3] edac: sifive: Add EDAC support for Memory
> Controller in SiFive SoCs
>
> [External Email] Do not click links or attachments unless you recognize the
> sender and know the content is safe
>
> > Subject: Re: [PATCH 3/3] edac: sifive: Add EDAC support for Memory
> > Controller in SiFive SoCs
>
> Fix subject prefix: "EDAC/sifive: ..."
>
> On Tue, Aug 25, 2020 at 05:36:22PM +0530, Yash Shah wrote:
> > Add Memory controller EDAC support in exisiting SiFive platform EDAC
>
> s/in exisiting/to the/
>
> > driver. It registers for notifier events from the SiFive DDR
> > controller driver for DDR ECC events.
>
> Simplify:
>
> "It registers for ECC notifier events from the memory controller."
>
> > Signed-off-by: Yash Shah
> > ---
> > drivers/edac/Kconfig | 2 +-
> > drivers/edac/sifive_edac.c | 117
> > +
> > 2 files changed, 118 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> > 7b6ec30..f8b3b53 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -462,7 +462,7 @@ config EDAC_ALTERA_SDMMC
> >
> > config EDAC_SIFIVE
> > bool "Sifive platform EDAC driver"
> > - depends on EDAC=y && SIFIVE_L2
> > + depends on EDAC=y && (SIFIVE_L2 || SIFIVE_DDR)
> > help
> > Support for error detection and correction on the SiFive SoCs.
> >
> > diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c
> > index 3a3dcb1..cf032685 100644
> > --- a/drivers/edac/sifive_edac.c
> > +++ b/drivers/edac/sifive_edac.c
> > @@ -11,14 +11,120 @@
> > #include
> > #include "edac_module.h"
> > #include
> > +#include
> >
> > #define DRVNAME "sifive_edac"
> > +#define SIFIVE_EDAC_MOD_NAME "Sifive ECC Manager"
>
> s/SIFIVE_EDAC_MOD_NAME/EDAC_MOD_NAME/g
>
> like the other EDAC drivers.
>
Sure, will make all the above suggested textual changes in v2.
> ...
>
> > +static int ecc_mc_register(struct platform_device *pdev) {
> > + struct sifive_edac_mc_priv *p;
> > + struct edac_mc_layer layers[1];
> > + int ret;
> > +
> > + p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL);
> > + if (!p)
> > + return -ENOMEM;
> > +
> > + p->notifier.notifier_call = ecc_mc_err_event;
> > + platform_set_drvdata(pdev, p);
> > +
> > + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> > + layers[0].size = 1;
> > + layers[0].is_virt_csrow = true;
> > +
> > + p->mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
> > + if (!p->mci) {
> > + dev_err(>dev, "Failed mem allocation for mc
> > instance\n");
> > + return -ENOMEM;
> > + }
> > +
> > + p->mci->pdev = >dev;
> > + /* Initialize controller capabilities */
> > + p->mci->mtype_cap = MEM_FLAG_DDR4;
> > + p->mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
> > + p->mci->edac_cap = EDAC_FLAG_SECDED;
> > + p->mci->scrub_cap = SCRUB_UNKNOWN;
> > + p->mci->scrub_mode = SCRUB_HW_PROG;
> > + p->mci->ctl_name = dev_name(>dev);
> > + p->mci->dev_name = dev_name(>dev);
> > + p->mci->mod_name = SIFIVE_EDAC_MOD_NAME;
> > + p->mci->ctl_page_to_phys = NULL;
> > +
> > + /* Interrupt feature is supported by cadence mc */
> > + edac_op_state = EDAC_OPSTATE_INT;
> > +
> > + ret = edac_mc_add_mc(p->mci);
> > + if (ret) {
> > + edac_printk(KERN_ERR, SIFIVE_EDAC_MOD_NAME,
> > + "Failed to register with EDAC core\n");
> > + goto err;
> > + }
> > +
> > +#ifdef CONFIG_SIFIVE_DDR
>
> It seems all that ifdeffery can be replaced with
>
> if (IS_ENABLED(CONFIG_...))
Yes, will replace all the ifdeffery in v2
Thanks for the review.
- Yash
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 3/3] edac: sifive: Add EDAC support for Memory Controller in SiFive SoCs
> Subject: Re: [PATCH 3/3] edac: sifive: Add EDAC support for Memory Controller
> in SiFive SoCs
Fix subject prefix: "EDAC/sifive: ..."
On Tue, Aug 25, 2020 at 05:36:22PM +0530, Yash Shah wrote:
> Add Memory controller EDAC support in exisiting SiFive platform EDAC
s/in exisiting/to the/
> driver. It registers for notifier events from the SiFive DDR controller
> driver for DDR ECC events.
Simplify:
"It registers for ECC notifier events from the memory controller."
> Signed-off-by: Yash Shah
> ---
> drivers/edac/Kconfig | 2 +-
> drivers/edac/sifive_edac.c | 117
> +
> 2 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 7b6ec30..f8b3b53 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -462,7 +462,7 @@ config EDAC_ALTERA_SDMMC
>
> config EDAC_SIFIVE
> bool "Sifive platform EDAC driver"
> - depends on EDAC=y && SIFIVE_L2
> + depends on EDAC=y && (SIFIVE_L2 || SIFIVE_DDR)
> help
> Support for error detection and correction on the SiFive SoCs.
>
> diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c
> index 3a3dcb1..cf032685 100644
> --- a/drivers/edac/sifive_edac.c
> +++ b/drivers/edac/sifive_edac.c
> @@ -11,14 +11,120 @@
> #include
> #include "edac_module.h"
> #include
> +#include
>
> #define DRVNAME "sifive_edac"
> +#define SIFIVE_EDAC_MOD_NAME "Sifive ECC Manager"
s/SIFIVE_EDAC_MOD_NAME/EDAC_MOD_NAME/g
like the other EDAC drivers.
...
> +static int ecc_mc_register(struct platform_device *pdev)
> +{
> + struct sifive_edac_mc_priv *p;
> + struct edac_mc_layer layers[1];
> + int ret;
> +
> + p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> +
> + p->notifier.notifier_call = ecc_mc_err_event;
> + platform_set_drvdata(pdev, p);
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> +
> + p->mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
> + if (!p->mci) {
> + dev_err(>dev, "Failed mem allocation for mc instance\n");
> + return -ENOMEM;
> + }
> +
> + p->mci->pdev = >dev;
> + /* Initialize controller capabilities */
> + p->mci->mtype_cap = MEM_FLAG_DDR4;
> + p->mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
> + p->mci->edac_cap = EDAC_FLAG_SECDED;
> + p->mci->scrub_cap = SCRUB_UNKNOWN;
> + p->mci->scrub_mode = SCRUB_HW_PROG;
> + p->mci->ctl_name = dev_name(>dev);
> + p->mci->dev_name = dev_name(>dev);
> + p->mci->mod_name = SIFIVE_EDAC_MOD_NAME;
> + p->mci->ctl_page_to_phys = NULL;
> +
> + /* Interrupt feature is supported by cadence mc */
> + edac_op_state = EDAC_OPSTATE_INT;
> +
> + ret = edac_mc_add_mc(p->mci);
> + if (ret) {
> + edac_printk(KERN_ERR, SIFIVE_EDAC_MOD_NAME,
> + "Failed to register with EDAC core\n");
> + goto err;
> + }
> +
> +#ifdef CONFIG_SIFIVE_DDR
It seems all that ifdeffery can be replaced with
if (IS_ENABLED(CONFIG_...))
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 3/3] edac: sifive: Add EDAC support for Memory Controller in SiFive SoCs
On Tue, 25 Aug 2020 05:06:22 PDT (-0700), yash.s...@sifive.com wrote:
Add Memory controller EDAC support in exisiting SiFive platform EDAC
driver. It registers for notifier events from the SiFive DDR controller
driver for DDR ECC events.
Signed-off-by: Yash Shah
---
drivers/edac/Kconfig | 2 +-
drivers/edac/sifive_edac.c | 117 +
2 files changed, 118 insertions(+), 1 deletion(-)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 7b6ec30..f8b3b53 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -462,7 +462,7 @@ config EDAC_ALTERA_SDMMC
config EDAC_SIFIVE
bool "Sifive platform EDAC driver"
- depends on EDAC=y && SIFIVE_L2
+ depends on EDAC=y && (SIFIVE_L2 || SIFIVE_DDR)
help
Support for error detection and correction on the SiFive SoCs.
diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c
index 3a3dcb1..cf032685 100644
--- a/drivers/edac/sifive_edac.c
+++ b/drivers/edac/sifive_edac.c
@@ -11,14 +11,120 @@
#include
#include "edac_module.h"
#include
+#include
#define DRVNAME "sifive_edac"
+#define SIFIVE_EDAC_MOD_NAME "Sifive ECC Manager"
struct sifive_edac_priv {
struct notifier_block notifier;
struct edac_device_ctl_info *dci;
};
+struct sifive_edac_mc_priv {
+ struct notifier_block notifier;
+ struct mem_ctl_info *mci;
+};
+
+/**
+ * EDAC MC error callback
+ *
+ * @event: non-zero if unrecoverable.
+ */
+static
+int ecc_mc_err_event(struct notifier_block *this, unsigned long event, void
*ptr)
+{
+ struct sifive_ddr_priv *priv = ptr;
+ struct sifive_edac_mc_priv *p;
+
+ p = container_of(this, struct sifive_edac_mc_priv, notifier);
+ if (event == SIFIVE_DDR_ERR_TYPE_UE) {
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, p->mci,
+priv->error_count, priv->page_frame_number,
+priv->offset_in_page, priv->syndrome,
+priv->top_layer, priv->mid_layer,
+priv->low_layer, p->mci->ctl_name, "");
+ } else if (event == SIFIVE_DDR_ERR_TYPE_CE) {
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, p->mci,
+priv->error_count, priv->page_frame_number,
+priv->offset_in_page, priv->syndrome,
+priv->top_layer, priv->mid_layer,
+priv->low_layer, p->mci->ctl_name, "");
+ }
+
+ return NOTIFY_OK;
+}
+
+static int ecc_mc_register(struct platform_device *pdev)
+{
+ struct sifive_edac_mc_priv *p;
+ struct edac_mc_layer layers[1];
+ int ret;
+
+ p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+
+ p->notifier.notifier_call = ecc_mc_err_event;
+ platform_set_drvdata(pdev, p);
+
+ layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+ layers[0].size = 1;
+ layers[0].is_virt_csrow = true;
+
+ p->mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
+ if (!p->mci) {
+ dev_err(>dev, "Failed mem allocation for mc instance\n");
+ return -ENOMEM;
+ }
+
+ p->mci->pdev = >dev;
+ /* Initialize controller capabilities */
+ p->mci->mtype_cap = MEM_FLAG_DDR4;
+ p->mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+ p->mci->edac_cap = EDAC_FLAG_SECDED;
+ p->mci->scrub_cap = SCRUB_UNKNOWN;
+ p->mci->scrub_mode = SCRUB_HW_PROG;
+ p->mci->ctl_name = dev_name(>dev);
+ p->mci->dev_name = dev_name(>dev);
+ p->mci->mod_name = SIFIVE_EDAC_MOD_NAME;
+ p->mci->ctl_page_to_phys = NULL;
+
+ /* Interrupt feature is supported by cadence mc */
+ edac_op_state = EDAC_OPSTATE_INT;
+
+ ret = edac_mc_add_mc(p->mci);
+ if (ret) {
+ edac_printk(KERN_ERR, SIFIVE_EDAC_MOD_NAME,
+ "Failed to register with EDAC core\n");
+ goto err;
+ }
+
+#ifdef CONFIG_SIFIVE_DDR
+ register_sifive_ddr_error_notifier(>notifier);
+#endif
+
+ return 0;
+
+err:
+ edac_mc_free(p->mci);
+
+ return -ENXIO;
+}
+
+static int ecc_mc_unregister(struct platform_device *pdev)
+{
+ struct sifive_edac_mc_priv *p = platform_get_drvdata(pdev);
+
+#ifdef CONFIG_SIFIVE_DDR
+ unregister_sifive_ddr_error_notifier(>notifier);
+#endif
+ edac_mc_del_mc(>dev);
+ edac_mc_free(p->mci);
+
+ return 0;
+}
+
/**
* EDAC error callback
*
@@ -67,7 +173,9 @@ static int ecc_register(struct platform_device *pdev)
goto err;
}
+#ifdef CONFIG_SIFIVE_L2
register_sifive_l2_error_notifier(>notifier);
+#endif
return 0;
@@ -81,7 +189,9 @@ static int ecc_unregister(struct platform_device *pdev)
{
struct
[PATCH 3/3] edac: sifive: Add EDAC support for Memory Controller in SiFive SoCs
Add Memory controller EDAC support in exisiting SiFive platform EDAC
driver. It registers for notifier events from the SiFive DDR controller
driver for DDR ECC events.
Signed-off-by: Yash Shah
---
drivers/edac/Kconfig | 2 +-
drivers/edac/sifive_edac.c | 117 +
2 files changed, 118 insertions(+), 1 deletion(-)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 7b6ec30..f8b3b53 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -462,7 +462,7 @@ config EDAC_ALTERA_SDMMC
config EDAC_SIFIVE
bool "Sifive platform EDAC driver"
- depends on EDAC=y && SIFIVE_L2
+ depends on EDAC=y && (SIFIVE_L2 || SIFIVE_DDR)
help
Support for error detection and correction on the SiFive SoCs.
diff --git a/drivers/edac/sifive_edac.c b/drivers/edac/sifive_edac.c
index 3a3dcb1..cf032685 100644
--- a/drivers/edac/sifive_edac.c
+++ b/drivers/edac/sifive_edac.c
@@ -11,14 +11,120 @@
#include
#include "edac_module.h"
#include
+#include
#define DRVNAME "sifive_edac"
+#define SIFIVE_EDAC_MOD_NAME "Sifive ECC Manager"
struct sifive_edac_priv {
struct notifier_block notifier;
struct edac_device_ctl_info *dci;
};
+struct sifive_edac_mc_priv {
+ struct notifier_block notifier;
+ struct mem_ctl_info *mci;
+};
+
+/**
+ * EDAC MC error callback
+ *
+ * @event: non-zero if unrecoverable.
+ */
+static
+int ecc_mc_err_event(struct notifier_block *this, unsigned long event, void
*ptr)
+{
+ struct sifive_ddr_priv *priv = ptr;
+ struct sifive_edac_mc_priv *p;
+
+ p = container_of(this, struct sifive_edac_mc_priv, notifier);
+ if (event == SIFIVE_DDR_ERR_TYPE_UE) {
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, p->mci,
+priv->error_count, priv->page_frame_number,
+priv->offset_in_page, priv->syndrome,
+priv->top_layer, priv->mid_layer,
+priv->low_layer, p->mci->ctl_name, "");
+ } else if (event == SIFIVE_DDR_ERR_TYPE_CE) {
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, p->mci,
+priv->error_count, priv->page_frame_number,
+priv->offset_in_page, priv->syndrome,
+priv->top_layer, priv->mid_layer,
+priv->low_layer, p->mci->ctl_name, "");
+ }
+
+ return NOTIFY_OK;
+}
+
+static int ecc_mc_register(struct platform_device *pdev)
+{
+ struct sifive_edac_mc_priv *p;
+ struct edac_mc_layer layers[1];
+ int ret;
+
+ p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+
+ p->notifier.notifier_call = ecc_mc_err_event;
+ platform_set_drvdata(pdev, p);
+
+ layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+ layers[0].size = 1;
+ layers[0].is_virt_csrow = true;
+
+ p->mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
+ if (!p->mci) {
+ dev_err(>dev, "Failed mem allocation for mc instance\n");
+ return -ENOMEM;
+ }
+
+ p->mci->pdev = >dev;
+ /* Initialize controller capabilities */
+ p->mci->mtype_cap = MEM_FLAG_DDR4;
+ p->mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+ p->mci->edac_cap = EDAC_FLAG_SECDED;
+ p->mci->scrub_cap = SCRUB_UNKNOWN;
+ p->mci->scrub_mode = SCRUB_HW_PROG;
+ p->mci->ctl_name = dev_name(>dev);
+ p->mci->dev_name = dev_name(>dev);
+ p->mci->mod_name = SIFIVE_EDAC_MOD_NAME;
+ p->mci->ctl_page_to_phys = NULL;
+
+ /* Interrupt feature is supported by cadence mc */
+ edac_op_state = EDAC_OPSTATE_INT;
+
+ ret = edac_mc_add_mc(p->mci);
+ if (ret) {
+ edac_printk(KERN_ERR, SIFIVE_EDAC_MOD_NAME,
+ "Failed to register with EDAC core\n");
+ goto err;
+ }
+
+#ifdef CONFIG_SIFIVE_DDR
+ register_sifive_ddr_error_notifier(>notifier);
+#endif
+
+ return 0;
+
+err:
+ edac_mc_free(p->mci);
+
+ return -ENXIO;
+}
+
+static int ecc_mc_unregister(struct platform_device *pdev)
+{
+ struct sifive_edac_mc_priv *p = platform_get_drvdata(pdev);
+
+#ifdef CONFIG_SIFIVE_DDR
+ unregister_sifive_ddr_error_notifier(>notifier);
+#endif
+ edac_mc_del_mc(>dev);
+ edac_mc_free(p->mci);
+
+ return 0;
+}
+
/**
* EDAC error callback
*
@@ -67,7 +173,9 @@ static int ecc_register(struct platform_device *pdev)
goto err;
}
+#ifdef CONFIG_SIFIVE_L2
register_sifive_l2_error_notifier(>notifier);
+#endif
return 0;
@@ -81,7 +189,9 @@ static int ecc_unregister(struct platform_device *pdev)
{
struct sifive_edac_priv *p = platform_get_drvdata(pdev);
+#ifdef
