Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
On Wed, 20 Mar 2019, Yash Shah wrote: > This EDAC driver supports: > - Initial configuration reporting on bootup via debug logs > - ECC event monitoring and reporting through the EDAC framework > - ECC event injection > > This driver is partially based on pnd2_edac.c and altera_edac.c > > Initially L2 Cache controller is added as a subcomponent to > this EDAC driver. > > Signed-off-by: Yash Shah Based on the discussion with Borislav, we're going to move the L2 driver elsewhere in the tree. We'll look into creating a platform-specific EDAC driver on top of that. - Paul
Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
On Mon, Mar 25, 2019 at 02:26:52PM -0700, Paul Walmsley wrote: > We'll definitely take the RAM savings that a few #ifdefs will deliver to > us. They add up. We're selling chips for embedded use cases, not just > big-iron x86 systems. Fair enough. Btw, while we're at it, this driver would need a MAINTAINERS entry so that you guys can get CCed on bugs. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
On Mon, 25 Mar 2019, Borislav Petkov wrote: > On Sun, Mar 24, 2019 at 05:23:27PM -0700, Paul Walmsley wrote: > > > + if (!IS_ENABLED(CONFIG_EDAC_DEBUG)) > > > + return; > > > > Can all of these debugfs functions be wrapped with an #if ... #endif such > > that, if CONFIG_EDAC_DEBUG is not set, they will all be stripped out by > > the preprocessor? > > Why would you make the code more ugly with ifdeffery? > > Do you have any serious code size constraints so that you absolutely > need to remove a couple of KBs? We'll definitely take the RAM savings that a few #ifdefs will deliver to us. They add up. We're selling chips for embedded use cases, not just big-iron x86 systems. Other EDAC drivers have far more #ifdef lines than the single set that I'm proposing, so I don't understand why you're singling this driver out for criticism. Consider: ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_SDRAM ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_L2C ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_OCRAM ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_OCRAM ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_L2C ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_ETHERNET ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_NAND ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_DMA ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_USB ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_QSPI ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_SDMMC ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_L2C ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_OCRAM ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_ETHERNET ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_NAND ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_DMA ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_USB ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_QSPI ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_SDMMC ./altera_edac.c:#ifdef CONFIG_EDAC_ALTERA_SDRAM ./synopsys_edac.c:#ifdef CONFIG_EDAC_DEBUG ./synopsys_edac.c:#ifdef CONFIG_EDAC_DEBUG ./synopsys_edac.c:#ifdef CONFIG_EDAC_DEBUG ./synopsys_edac.c:#ifdef CONFIG_EDAC_DEBUG ./synopsys_edac.c:#ifdef CONFIG_EDAC_DEBUG - Paul
Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
On Sun, Mar 24, 2019 at 05:23:27PM -0700, Paul Walmsley wrote: > > + if (!IS_ENABLED(CONFIG_EDAC_DEBUG)) > > + return; > > Can all of these debugfs functions be wrapped with an #if ... #endif such > that, if CONFIG_EDAC_DEBUG is not set, they will all be stripped out by > the preprocessor? Why would you make the code more ugly with ifdeffery? Do you have any serious code size constraints so that you absolutely need to remove a couple of KBs? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
Hi Yash,
Just a few brief comments here:
On Wed, 20 Mar 2019, Yash Shah wrote:
> This EDAC driver supports:
> - Initial configuration reporting on bootup via debug logs
> - ECC event monitoring and reporting through the EDAC framework
> - ECC event injection
>
It's probably worth mentioning here, as you did in the subject line, that
this is for the FU540-C000 platform.
> This driver is partially based on pnd2_edac.c and altera_edac.c
>
> Initially L2 Cache controller is added as a subcomponent to
> this EDAC driver.
>
> Signed-off-by: Yash Shah
> ---
> arch/riscv/Kconfig | 1 +
> drivers/edac/Kconfig | 13 ++
> drivers/edac/Makefile | 1 +
> drivers/edac/sifive_edac.c | 297
> +
> 4 files changed, 312 insertions(+)
> create mode 100644 drivers/edac/sifive_edac.c
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 515fc3c..fede4b6 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -49,6 +49,7 @@ config RISCV
> select RISCV_TIMER
> select GENERIC_IRQ_MULTI_HANDLER
> select ARCH_HAS_PTE_SPECIAL
> + select EDAC_SUPPORT
>
> config MMU
> def_bool y
This part of the patch either needs to get an ack from Palmer, or should
be split into a final, separate patch that Palmer can merge separately.
That way it will avoid unexpected merge conflicts if anything that Palmer
merges changes this file.
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index e286b5b..112d9d1 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -440,6 +440,19 @@ config EDAC_ALTERA_SDMMC
> Support for error detection and correction on the
> Altera SDMMC FIFO Memory for Altera SoCs.
>
> +config EDAC_SIFIVE
> + tristate "Sifive ECC"
> + depends on RISCV
> + help
> + Support for error detection and correction on the SiFive SoCs.
> +
> +config EDAC_SIFIVE_L2
> + bool "SiFive L2 Cache ECC"
> + depends on EDAC_SIFIVE=y
> + help
> + Support for error detection and correction of the L2 cache
> + memory on 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 716096d..b16dce8 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -74,6 +74,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 000..e11ae6b5
> --- /dev/null
> +++ b/drivers/edac/sifive_edac.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SiFive EDAC Driver
> + *
> + * Copyright (C) 2018-2019 SiFive, Inc.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "edac_module.h"
> +
> +#define SIFIVE_EDAC_DIRFIX_LOW 0x100
> +#define SIFIVE_EDAC_DIRFIX_HIGH 0x104
> +#define SIFIVE_EDAC_DIRFIX_COUNT 0x108
> +
> +#define SIFIVE_EDAC_DATFIX_LOW 0x140
> +#define SIFIVE_EDAC_DATFIX_HIGH 0x144
> +#define SIFIVE_EDAC_DATFIX_COUNT 0x148
> +
> +#define SIFIVE_EDAC_DATFAIL_LOW 0x160
> +#define SIFIVE_EDAC_DATFAIL_HIGH 0x164
> +#define SIFIVE_EDAC_DATFAIL_COUNT 0x168
> +
> +#define SIFIVE_EDAC_ECCINJECTERR 0x40
> +#define SIFIVE_EDAC_CONFIG 0x00
> +
> +#define SIFIVE_EDAC_MAX_INTR 3
> +
> +/* EDAC Parent Probe */
> +
> +static const struct of_device_id sifive_edac_device_of_match[];
> +
> +static const struct of_device_id sifive_edac_of_match[] = {
> + { .compatible = "sifive,ecc-manager" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sifive_edac_of_match);
> +
> +static int sifive_edac_probe(struct platform_device *pdev)
> +{
> + of_platform_populate(pdev->dev.of_node, sifive_edac_device_of_match,
> + NULL, >dev);
> + return 0;
> +}
> +
> +static struct platform_driver sifive_edac_driver = {
> + .probe = sifive_edac_probe,
> + .driver = {
> + .name = "ecc_manager",
As mentioned before we don't have an ECC manager IP block, so we probably
should figure out a different approach here, if possible.
> + .of_match_table = sifive_edac_of_match,
> + },
> +};
> +module_platform_driver(sifive_edac_driver);
> +
> +struct sifive_edac_device_prv {
> + void (*setup)(struct edac_device_ctl_info *dci);
> + irqreturn_t (*ecc_irq_handler)(int irq, void *dev_id);
> + const struct file_operations *inject_fops;
> +};
> +
> +struct sifive_edac_device_dev {
Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
On Thu, Mar 21, 2019 at 7:03 PM Borislav Petkov wrote:
>
> On Wed, Mar 20, 2019 at 05:22:08PM +0530, Yash Shah wrote:
> > This EDAC driver supports:
> > - Initial configuration reporting on bootup via debug logs
> > - ECC event monitoring and reporting through the EDAC framework
> > - ECC event injection
> >
> > This driver is partially based on pnd2_edac.c and altera_edac.c
> >
> > Initially L2 Cache controller is added as a subcomponent to
> > this EDAC driver.
> >
> > Signed-off-by: Yash Shah
>
> ...
>
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> > index e286b5b..112d9d1 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -440,6 +440,19 @@ config EDAC_ALTERA_SDMMC
> > Support for error detection and correction on the
> > Altera SDMMC FIFO Memory for Altera SoCs.
> >
> > +config EDAC_SIFIVE
> > + tristate "Sifive ECC"
> > + depends on RISCV
> > + help
> > + Support for error detection and correction on the SiFive SoCs.
> > +
> > +config EDAC_SIFIVE_L2
>
> That config item is unused. Drop it.
Sure, will drop it.
> > +static irqreturn_t sifive_edac_l2_int_handler(int irq, void *device)
> > +{
> > + struct edac_device_ctl_info *dci =
> > + (struct edac_device_ctl_info *)device;
>
> No ugly linebreaks like that - just let it stick out even if it is > 80
> cols long.
Ok. Will let it stick out.
> > +
> > +/*
> > + * sifive_edac_device_probe()
> > + * This is a generic EDAC device driver that will support
> > + * various SiFive memory devices as well as the memories
> > + * for other peripherals. Module specific initialization is
> > + * done by passing the function index in the device tree.
>
> This comment doesn't have anything to do with that function but rather
> belongs at the top of this file.
Will move it at the top.
>
> > + */
> > +static int sifive_edac_device_probe(struct platform_device *pdev)
> > +{
> > + struct edac_device_ctl_info *dci;
> > + struct sifive_edac_device_dev *drvdata;
> > + int rc, i;
> > + struct resource *res;
> > + void __iomem *baseaddr;
> > + struct device_node *np = pdev->dev.of_node;
> > + char *ecc_name = (char *)np->name;
> > + static int dev_instance;
>
> Please sort function local variables declaration in a reverse christmas
> tree order:
>
> longest_variable_name;
> shorter_var_name;
> even_shorter;
> i;
>
Sure, will be done.
> > +
> > + rc = edac_device_add_device(dci);
> > + if (rc) {
> > + dev_err(>dev, "failed to register with EDAC core\n");
> > + goto del_edac_device;
> > + }
>
> Call setup_sifive_debug() in the success case here so that you don't
> have to teardown below.
Ok.
>
> > +
> > + return rc;
> > +
> > +del_edac_device:
> > + teardown_sifive_debug();
> > + edac_device_del_device(>dev);
> > + edac_device_free_ctl_info(dci);
>
> Those three can be replaced by a call to sifive_edac_device_remove() ?
Since now there won't be a need to teardown, I will stick with this
(bottom two function calls).
>
> Btw, you have prefixed your static functions with "sifive_edac_" which
> doesn't make much sense and you have long names for no good reason.
Will remove the prefix "sifive_edac_" on static functions wherever feasible.
> > +static struct platform_driver sifive_edac_device_driver = {
> > + .driver = {
> > + .name = "sifive_edac_device",
>
> "device"? I think it is clear that it is a device and thus kinda
> tautological. "sifive_edac" should be enough...
Sure. Will keep it just "sifive_edac"
>
> Last but not least, building this driver with the riscv64 cross compilers from
> here:
>
> http://cdn.kernel.org/pub/tools/crosstool/files/bin/
>
> fails like below. With the riscv32 compiler it fails the same.
>
> Provided I'm not doing anything wrong, you should not be sending me
> half-baked stuff which doesn't even build.
Sorry about that. It fails if configured as 'module'.
I intended this driver to be built-in only hence I never came across
these errors while testing.
I somehow missed it in Kconfig file.
I will make the correction in Kconfig (change 'tristate' to 'bool')
and make sure everything builds fine.
> --
> Regards/Gruss,
> Boris.
Thanks for your comments.
- Yash
>
> ECO tip #101: Trim your mails when you reply.
> --
Re: [PATCH 2/2] edac: sifive: Add EDAC driver for SiFive FU540-C000 chip
On Wed, Mar 20, 2019 at 05:22:08PM +0530, Yash Shah wrote:
> This EDAC driver supports:
> - Initial configuration reporting on bootup via debug logs
> - ECC event monitoring and reporting through the EDAC framework
> - ECC event injection
>
> This driver is partially based on pnd2_edac.c and altera_edac.c
>
> Initially L2 Cache controller is added as a subcomponent to
> this EDAC driver.
>
> Signed-off-by: Yash Shah
...
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index e286b5b..112d9d1 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -440,6 +440,19 @@ config EDAC_ALTERA_SDMMC
> Support for error detection and correction on the
> Altera SDMMC FIFO Memory for Altera SoCs.
>
> +config EDAC_SIFIVE
> + tristate "Sifive ECC"
> + depends on RISCV
> + help
> + Support for error detection and correction on the SiFive SoCs.
> +
> +config EDAC_SIFIVE_L2
That config item is unused. Drop it.
> + bool "SiFive L2 Cache ECC"
> + depends on EDAC_SIFIVE=y
> + help
> + Support for error detection and correction of the L2 cache
> + memory on SiFive SoCs.
> +
> config EDAC_SYNOPSYS
> tristate "Synopsys DDR Memory Controller"
> depends on ARCH_ZYNQ || ARCH_ZYNQMP
...
> +/*
> + * sifive_edac_l2_int_handler - ISR function for l2 cache controller
> + * @irq: Irq Number
> + * @device: Pointer to the edac device controller instance
> + *
> + * This routine is triggered whenever there is ECC error detected
> + *
> + * Return: Always returns IRQ_HANDLED
> + */
> +static irqreturn_t sifive_edac_l2_int_handler(int irq, void *device)
> +{
> + struct edac_device_ctl_info *dci =
> + (struct edac_device_ctl_info *)device;
No ugly linebreaks like that - just let it stick out even if it is > 80
cols long.
> + struct sifive_edac_device_dev *drvdata = dci->pvt_info;
> + u32 regval, add_h, add_l;
> +
> + if (irq == drvdata->irq[dir_corr]) {
> + add_h = readl(drvdata->base + SIFIVE_EDAC_DIRFIX_HIGH);
> + add_l = readl(drvdata->base + SIFIVE_EDAC_DIRFIX_LOW);
> + dev_err(dci->dev,
> + "DirError at address 0x%08X.%08X\n", add_h, add_l);
Same here and below.
> + regval = readl(drvdata->base + SIFIVE_EDAC_DIRFIX_COUNT);
> + edac_device_handle_ce(dci, 0, 0, "DirECCFix");
> + }
> + if (irq == drvdata->irq[data_corr]) {
> + add_h = readl(drvdata->base + SIFIVE_EDAC_DATFIX_HIGH);
> + add_l = readl(drvdata->base + SIFIVE_EDAC_DATFIX_LOW);
> + dev_err(dci->dev,
> + "DataError at address 0x%08X.%08X\n", add_h, add_l);
> + regval = readl(drvdata->base + SIFIVE_EDAC_DATFIX_COUNT);
> + edac_device_handle_ce(dci, 0, 0, "DatECCFix");
> + }
> + if (irq == drvdata->irq[data_uncorr]) {
> + add_h = readl(drvdata->base + SIFIVE_EDAC_DATFAIL_HIGH);
> + add_l = readl(drvdata->base + SIFIVE_EDAC_DATFAIL_LOW);
> + dev_err(dci->dev,
> + "DataFail at address 0x%08X.%08X\n", add_h, add_l);
> + regval = readl(drvdata->base + SIFIVE_EDAC_DATFAIL_COUNT);
> + edac_device_handle_ue(dci, 0, 0, "DatECCFail");
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void sifive_edac_l2_config_read(struct edac_device_ctl_info *dci)
> +{
> + struct sifive_edac_device_dev *drvdata = dci->pvt_info;
> + u32 regval, val;
> +
> + regval = readl(drvdata->base + SIFIVE_EDAC_CONFIG);
> + val = regval & 0xFF;
> + dev_info(dci->dev, "No. of Banks in the cache: %d\n", val);
> + val = (regval & 0xFF00) >> 8;
> + dev_info(dci->dev, "No. of ways per bank: %d\n", val);
> + val = (regval & 0xFF) >> 16;
> + dev_info(dci->dev, "Sets per bank: %llu\n", (uint64_t)1 << val);
> + val = (regval & 0xFF00) >> 24;
> + dev_info(dci->dev,
> + "Bytes per cache block: %llu\n", (uint64_t)1 << val);
> +}
> +
> +static const struct sifive_edac_device_prv l2ecc_data = {
> + .setup = sifive_edac_l2_config_read,
> + .inject_fops = _edac_l2_fops,
> + .ecc_irq_handler = sifive_edac_l2_int_handler,
> +};
> +
> +/*
> + * sifive_edac_device_probe()
> + * This is a generic EDAC device driver that will support
> + * various SiFive memory devices as well as the memories
> + * for other peripherals. Module specific initialization is
> + * done by passing the function index in the device tree.
This comment doesn't have anything to do with that function but rather
belongs at the top of this file.
> + */
> +static int sifive_edac_device_probe(struct platform_device *pdev)
> +{
> + struct edac_device_ctl_info *dci;
> + struct sifive_edac_device_dev *drvdata;
> + int rc, i;
> + struct resource *res;
> + void __iomem *baseaddr;
> + struct device_node *np = pdev->dev.of_node;
> + char
