Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
From: Borislav Petkov
Date: Thu Jan 10 2019 - 04:51:03 EST
On Sun, Dec 16, 2018 at 10:01:56PM -0800, Stefan Schaeckeler wrote:
> From: Stefan M Schaeckeler <sschaeck@xxxxxxxxx>
>
> Add support for the Aspeed AST2500 SoC EDAC driver.
>
> Signed-off-by: Stefan M Schaeckeler <sschaeck@xxxxxxxxx>
> ---
> MAINTAINERS | 6 +
> arch/arm/boot/dts/aspeed-g5.dtsi | 7 +
> drivers/edac/Kconfig | 7 +
> drivers/edac/Makefile | 1 +
> drivers/edac/aspeed_edac.c | 457 +++++++++++++++++++++++++++++++
> 5 files changed, 478 insertions(+)
> create mode 100644 drivers/edac/aspeed_edac.c
I couldn't see anything out of the ordinary - only nitpicks below.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3318f30903b2..1feb92b14029 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5315,6 +5315,12 @@ L: linux-edac@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/edac/amd64_edac*
>
> +EDAC-AST2500
> +M: Stefan Schaeckeler <sschaeck@xxxxxxxxx>
> +S: Supported
> +F: drivers/edac/aspeed_edac.c
> +F: Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> +
> EDAC-CALXEDA
> M: Robert Richter <rric@xxxxxxxxxx>
> L: linux-edac@xxxxxxxxxxxxxxx
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index d107459fc0f8..b4e479ab5a2d 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -47,6 +47,13 @@
> reg = <0x80000000 0>;
> };
>
> + edac: sdram@1e6e0000 {
> + compatible = "aspeed,ast2500-sdram-edac";
> + reg = <0x1e6e0000 0x174>;
> + interrupts = <0>;
> + status = "disabled";
> + };
> +
> ahb {
> compatible = "simple-bus";
> #address-cells = <1>;
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 41c9ccdd20d6..67834430b0a1 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -475,4 +475,11 @@ config EDAC_QCOM
> For debugging issues having to do with stability and overall system
> health, you should probably say 'Y' here.
>
> +config EDAC_ASPEED
> + tristate "Aspeed AST 2500 SoC"
> + depends on MACH_ASPEED_G5
> + help
> + Support for error detection and correction on the
> + Aspeed AST 2500 SoC.
> +
> endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 716096d08ea0..e1f23d4ff860 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o
> obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o
> obj-$(CONFIG_EDAC_TI) += ti_edac.o
> obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o
> +obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o
> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> new file mode 100644
> index 000000000000..d6ed119909eb
> --- /dev/null
> +++ b/drivers/edac/aspeed_edac.c
> @@ -0,0 +1,457 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 Cisco Systems
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
You have the SPDX license identifier - no need for that text.
> + */
> +
> +#include <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/stop_machine.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <asm/page.h>
> +#include "edac_module.h"
> +
> +
> +#define DRV_NAME "aspeed-edac"
> +
> +
> +/* registers */
no need for that comment
> +#define ASPEED_MCR_PROT 0x00 /* protection key register */
> +#define ASPEED_MCR_CONF 0x04 /* configuration register */
> +#define ASPEED_MCR_INTR_CTRL 0x50 /* interrupt control/status register */
> +#define ASPEED_MCR_ADDR_UNREC 0x58 /* address of first un-recoverable error */
> +#define ASPEED_MCR_ADDR_REC 0x5c /* address of last recoverable error */
> +#define ASPEED_MCR_LAST ASPEED_MCR_ADDR_REC
> +
> +
> +/* bits and masks */
ditto
> +#define ASPEED_MCR_PROT_PASSWD 0xfc600309
> +#define ASPEED_MCR_CONF_DRAM_TYPE BIT(4)
> +#define ASPEED_MCR_CONF_ECC BIT(7)
> +#define ASPEED_MCR_INTR_CTRL_CLEAR BIT(31)
> +#define ASPEED_MCR_INTR_CTRL_CNT_REC GENMASK(23, 16)
> +#define ASPEED_MCR_INTR_CTRL_CNT_UNREC GENMASK(15, 12)
> +#define ASPEED_MCR_INTR_CTRL_ENABLE (BIT(0) | BIT(1))
> +
> +
> +
> +static int aspeed_edac_regmap_reg_write(void *context, unsigned int reg,
> + unsigned int val)
All the static functions don't need the "aspeed_edac" prefix.
> +{
> + void __iomem *regs = (void __iomem *)context;
> +
> + /* enable write to MCR register set */
> + writel(ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT);
> +
> + writel(val, regs + reg);
> +
> + /* disable write to MCR register set */
> + writel(~ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT);
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_regmap_reg_read(void *context, unsigned int reg,
> + unsigned int *val)
> +{
> + void __iomem *regs = (void __iomem *)context;
> +
> + *val = readl(regs + reg);
> +
> + return 0;
> +}
> +
> +static bool aspeed_edac_regmap_is_volatile(struct device *dev,
> + unsigned int reg)
> +{
> + switch (reg) {
> + case ASPEED_MCR_PROT:
> + case ASPEED_MCR_INTR_CTRL:
> + case ASPEED_MCR_ADDR_UNREC:
> + case ASPEED_MCR_ADDR_REC:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +
> +static const struct regmap_config aspeed_edac_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = ASPEED_MCR_LAST,
> + .reg_write = aspeed_edac_regmap_reg_write,
> + .reg_read = aspeed_edac_regmap_reg_read,
> + .volatile_reg = aspeed_edac_regmap_is_volatile,
> + .fast_io = true,
> +};
> +
> +
> +static struct regmap *aspeed_edac_regmap;
Put that definition at the top of the file, under the #defines.
> +
> +
> +static void aspeed_edac_count_rec(struct mem_ctl_info *mci,
> + u8 rec_cnt,
> + u32 rec_addr)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + u32 page, offset, syndrome;
> +
> + if (rec_cnt > 0) {
Save an indentation level:
if (!rec_cnt)
return;
> + /* report first few errors (if there are) */
> + /* note: no addresses are recorded */
> + if (rec_cnt > 1) {
> + page = 0; /* not available */
> + offset = 0; /* not available */
> + syndrome = 0; /* not available */
Do a single comment, over the lines, not sideways. Ditto for the rest of
the driver - side comments make the code harder to read.
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + rec_cnt-1, page, offset,
> + syndrome, 0, 0, -1,
> + "address(es) not available", "");
> + }
> +
> + /* report last error */
> + /* note: rec_addr is the last recoverable error addr */
> + page = rec_addr >> PAGE_SHIFT;
> + offset = rec_addr & ~PAGE_MASK;
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1,
> + csrow->first_page + page, offset, syndrome,
> + 0, 0, -1, "", "");
> + }
> +}
> +
> +
> +static void aspeed_edac_count_un_rec(struct mem_ctl_info *mci,
> + u8 un_rec_cnt,
> + u32 un_rec_addr)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + u32 page, offset, syndrome;
> +
> + if (un_rec_cnt > 0) {
As above: save an indentation level by flipping the check.
> + /* report 1. error */
> + /* note: un_rec_addr is the first unrecoverable error addr */
> + page = un_rec_addr >> PAGE_SHIFT;
> + offset = un_rec_addr & ~PAGE_MASK;
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
> + csrow->first_page + page, offset, syndrome,
> + 0, 0, -1, "", "");
> +
> + /* report further errors (if there are) */
> + /* note: no addresses are recorded */
> + if (un_rec_cnt > 1) {
> + page = 0; /* not available */
> + offset = 0; /* not available */
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + un_rec_cnt-1, page, offset,
> + syndrome, 0, 0, -1,
> + "address(es) not available", "");
> + }
> + }
> +}
> +
> +
> +static void aspeed_edac_enable_interrupts(void)
> +{
> +
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_ENABLE,
> + ASPEED_MCR_INTR_CTRL_ENABLE);
> +}
A function which has a single statement and is called only once, looks
a bit useless to me. Just put the regmap_update_bits() call at the call
site with a comment (which you already have).
> +
> +
> +static void aspeed_edac_disable_interrupts(void)
> +{
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_ENABLE, 0);
> +}
Ditto.
> +
> +
> +static void aspeed_edac_clear_interrupts(void)
> +{
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_CLEAR,
> + ASPEED_MCR_INTR_CTRL_CLEAR);
> +
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_CLEAR, 0);
> +}
Ditto.
> +
> +
> +static irqreturn_t aspeed_edac_isr(int irq, void *arg)
> +{
> + u8 rec_cnt, un_rec_cnt;
> + u32 rec_addr, un_rec_addr;
> + struct mem_ctl_info *mci = arg;
> + u32 reg50, reg5c, reg58;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL, ®50);
> + dev_dbg(mci->pdev, "received edac interrupt w/ mmc register 50: 0x%x\n",
> + reg50);
> +
> + /* collect data about recoverable and unrecoverable errors */
> + rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_REC) >> 16;
> + un_rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_UNREC) >> 12;
> +
> + dev_dbg(mci->pdev, "%d recoverable interrupts and %d unrecoverable interrupts\n",
> + rec_cnt, un_rec_cnt);
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_ADDR_UNREC, ®58);
> + un_rec_addr = reg58 >> 4;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_ADDR_REC, ®5c);
> + rec_addr = reg5c >> 4;
> +
> + /* clear interrupt flags and error counters: */
> + aspeed_edac_clear_interrupts();
> +
> + /* process recoverable and unrecoverable errors */
> + if (rec_cnt > 0)
You check rec_cnt here *and* in the function? One can never be sure huh? :-)
> + aspeed_edac_count_rec(mci, rec_cnt, rec_addr);
> +
> + if (un_rec_cnt > 0)
> + aspeed_edac_count_un_rec(mci, un_rec_cnt, un_rec_addr);
Ditto. Just do the checks in the respective functions.
> +
> + if ((rec_cnt == 0) && (un_rec_cnt == 0))
Do
if (!rec_cnt && !un_rec_cnt)
> + dev_dbg(mci->pdev, "received edac interrupt, but did not find any ecc counters\n");
s/ecc/ECC/g
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL, ®50);
> + dev_dbg(mci->pdev, "edac interrupt handled. mmc reg 50 is now: 0x%x\n",
> + reg50);
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> +static int aspeed_edac_config_irq(void *ctx,
> + struct platform_device *pdev)
Let that line stick out.
> +{
> + int irq;
> + int rc;
> +
> + /* register interrupt handler */
> +
> + irq = platform_get_irq(pdev, 0);
> + dev_dbg(&pdev->dev, "got irq %d\n", irq);
> + if (!irq)
> + return -ENODEV;
> +
> + rc = devm_request_irq(&pdev->dev, irq, aspeed_edac_isr,
> + IRQF_TRIGGER_HIGH, DRV_NAME, ctx);
> + if (rc) {
> + dev_err(&pdev->dev, "unable to request irq %d\n", irq);
> + return rc;
> + }
> +
> + /* enable interrupts */
> + aspeed_edac_enable_interrupts();
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_init_csrows(struct mem_ctl_info *mci)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + struct dimm_info *dimm;
> + struct device_node *np;
> + u32 nr_pages, dram_type;
> + struct resource r;
> + u32 reg04;
> + int rc;
> +
> + /* retrieve info about physical memory from device tree */
> + np = of_find_node_by_path("/memory");
> +
Superfluous new line.
> + if (!np) {
> + dev_err(mci->pdev, "dt: missing /memory node\n");
> + return -ENODEV;
> + };
> +
> + rc = of_address_to_resource(np, 0, &r);
> +
> + of_node_put(np);
> +
> + if (rc) {
> + dev_err(mci->pdev, "dt: failed requesting resource for /memory node\n");
> + return rc;
> + };
> +
> + dev_dbg(mci->pdev, "dt: /memory node resources: first page r.start=0x%x, resource_size=0x%x, PAGE_SHIFT macro=0x%x\n",
> + r.start, resource_size(&r), PAGE_SHIFT);
> +
> + csrow->first_page = r.start >> PAGE_SHIFT;
> + nr_pages = resource_size(&r) >> PAGE_SHIFT;
> + csrow->last_page = csrow->first_page + nr_pages - 1;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
> + dram_type = (reg04 & ASPEED_MCR_CONF_DRAM_TYPE) ? MEM_DDR4 : MEM_DDR3;
> +
> + dimm = csrow->channels[0]->dimm;
> + dimm->mtype = dram_type;
> + dimm->edac_mode = EDAC_SECDED;
> + dimm->nr_pages = nr_pages / csrow->nr_channels;
> +
> + dev_dbg(mci->pdev, "initialized dimm with first_page=0x%lx and nr_pages=0x%x\n",
> + csrow->first_page, nr_pages);
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *regs;
> + struct resource *res;
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[2];
> + struct device_node *np;
> + u32 reg04;
> + int rc;
Please sort function local variables declaration in a reverse christmas
tree order:
<type A> longest_variable_name;
<type B> shorter_var_name;
<type C> even_shorter;
<type D> i;
Do that in all functions.
> +
> + /* setup regmap */
> + np = dev->of_node;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOENT;
> +
> + regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + aspeed_edac_regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> + &aspeed_edac_regmap_config);
> + if (IS_ERR(aspeed_edac_regmap))
> + return PTR_ERR(aspeed_edac_regmap);
> +
> + /* bail out if ECC mode is not configured */
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
> + if (!(reg04 & ASPEED_MCR_CONF_ECC)) {
> + dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n");
> + return -EPERM;
> + }
> +
> + edac_op_state = EDAC_OPSTATE_INT;
> +
> + /* allocate & init EDAC MC data structure */
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
> +
> + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
> + if (mci == NULL)
if (!mci)
> + return -ENOMEM;
> +
> + mci->pdev = &pdev->dev;
> + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
> + mci->edac_ctl_cap = EDAC_FLAG_SECDED;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->scrub_cap = SCRUB_FLAG_HW_SRC;
> + mci->scrub_mode = SCRUB_HW_SRC;
> + mci->mod_name = DRV_NAME;
> + mci->ctl_name = "MIC";
> + mci->dev_name = dev_name(&pdev->dev);
> +
> + rc = aspeed_edac_init_csrows(mci);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to init csrows\n");
> + goto probe_exit02;
> + }
> +
> + platform_set_drvdata(pdev, mci);
> +
> + /* register with edac core */
> + rc = edac_mc_add_mc(mci);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to register with EDAC core\n");
> + goto probe_exit02;
> + }
> +
> + /* register interrupt handler and enable interrupts */
> + rc = aspeed_edac_config_irq(mci, pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "failed setting up irq\n");
> + goto probe_exit01;
> + }
> +
> + return 0;
> +
> +probe_exit01:
> + edac_mc_del_mc(&pdev->dev);
> +probe_exit02:
> + edac_mc_free(mci);
> + return rc;
> +}
...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.