[PATCH v3 7/7] EDAC: Add driver for the Marvell Armada XP SDRAM and L2 cache ECC
Borislav Petkov
bp at alien8.de
Thu Nov 16 11:31:45 PST 2017
On Fri, Nov 10, 2017 at 10:03:08AM +0100, Jan Luebbe wrote:
> Add support for the ECC functionality as found in the DDR RAM and L2
> cache controllers on the MV78230/MV78x60 SoCs. This driver has been
> tested on the MV78460 (on a custom board with a DDR3 ECC DIMM).
>
> Signed-off-by: Jan Luebbe <jlu at pengutronix.de>
...
> diff --git a/drivers/edac/armada_xp_edac.c b/drivers/edac/armada_xp_edac.c
> new file mode 100644
> index 000000000000..cb9173b30aa9
> --- /dev/null
> +++ b/drivers/edac/armada_xp_edac.c
> @@ -0,0 +1,658 @@
> +/*
> + * Copyright (C) 2017 Pengutronix, Jan Luebbe <kernel at pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/edac.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/hardware/cache-l2x0.h>
I get this when I cross-build it:
drivers/edac/armada_xp_edac.c:19:37: fatal error: asm/hardware/cache-l2x0.h: No such file or directory
#include <asm/hardware/cache-l2x0.h>
^
compilation terminated.
make[2]: *** [drivers/edac/armada_xp_edac.o] Error 1
make[1]: *** [drivers/edac] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2
and that driver depends on ARCH_MVEBU which is arm64, AFAICT.
That header is
arch/arm/include/asm/hardware/cache-l2x0.h
however and not in arch/arm64/.
I could very well be missing something though...
> +#include <asm/hardware/cache-aurora-l2.h>
> +
> +#include "edac_mc.h"
> +#include "edac_device.h"
> +#include "edac_module.h"
> +
> +/************************ EDAC MC (DDR RAM) ********************************/
> +
> +#define SDRAM_NUM_CS 4
> +
> +#define SDRAM_CONFIG_REG 0x0
> +#define SDRAM_CONFIG_ECC_MASK BIT(18)
> +#define SDRAM_CONFIG_REGISTERED_MASK BIT(17)
> +#define SDRAM_CONFIG_BUS_WIDTH_MASK BIT(15)
> +
> +#define SDRAM_ADDR_CTRL_REG 0x10
> +#define SDRAM_ADDR_CTRL_SIZE_HIGH_OFFSET(cs) (20+cs)
> +#define SDRAM_ADDR_CTRL_SIZE_HIGH_MASK(cs) \
> + (0x1 << SDRAM_ADDR_CTRL_SIZE_HIGH_OFFSET(cs))
> +#define SDRAM_ADDR_CTRL_ADDR_SEL_MASK(cs) BIT(16+cs)
> +#define SDRAM_ADDR_CTRL_SIZE_LOW_OFFSET(cs) (cs*4+2)
> +#define SDRAM_ADDR_CTRL_SIZE_LOW_MASK(cs) \
> + (0x3 << SDRAM_ADDR_CTRL_SIZE_LOW_OFFSET(cs))
> +#define SDRAM_ADDR_CTRL_STRUCT_OFFSET(cs) (cs*4)
> +#define SDRAM_ADDR_CTRL_STRUCT_MASK(cs) \
> + (0x3 << SDRAM_ADDR_CTRL_STRUCT_OFFSET(cs))
> +
> +#define SDRAM_ERR_DATA_H_REG 0x40
> +#define SDRAM_ERR_DATA_L_REG 0x44
> +
> +#define SDRAM_ERR_RECV_ECC_REG 0x48
> +#define SDRAM_ERR_RECV_ECC_VALUE_MASK 0xff
> +
> +#define SDRAM_ERR_CALC_ECC_REG 0x4c
> +#define SDRAM_ERR_CALC_ECC_ROW_OFFSET 8
> +#define SDRAM_ERR_CALC_ECC_ROW_MASK \
> + (0xffff << SDRAM_ERR_CALC_ECC_ROW_OFFSET)
> +#define SDRAM_ERR_CALC_ECC_VALUE_MASK 0xff
> +
> +#define SDRAM_ERR_ADDR_REG 0x50
> +#define SDRAM_ERR_ADDR_BANK_OFFSET 23
> +#define SDRAM_ERR_ADDR_BANK_MASK \
> + (0x7 << SDRAM_ERR_ADDR_BANK_OFFSET)
> +#define SDRAM_ERR_ADDR_COL_OFFSET 8
> +#define SDRAM_ERR_ADDR_COL_MASK \
> + (0x7fff << SDRAM_ERR_ADDR_COL_OFFSET)
> +#define SDRAM_ERR_ADDR_CS_OFFSET 1
> +#define SDRAM_ERR_ADDR_CS_MASK \
> + (0x3 << SDRAM_ERR_ADDR_CS_OFFSET)
> +#define SDRAM_ERR_ADDR_TYPE_MASK BIT(0)
> +
> +#define SDRAM_ERR_CTRL_REG 0x54
> +#define SDRAM_ERR_CTRL_ERR_THR_OFFSET 16
> +#define SDRAM_ERR_CTRL_ERR_THR_MASK \
> + (0xff << SDRAM_ERR_CTRL_ERR_THR_OFFSET)
> +#define SDRAM_ERR_CTRL_ERR_PROP_MASK BIT(9)
> +
> +#define SDRAM_ERR_SBE_COUNT_REG 0x58
> +#define SDRAM_ERR_DBE_COUNT_REG 0x5c
> +
> +#define SDRAM_ERR_CAUSE_ERR_REG 0xd0
> +#define SDRAM_ERR_CAUSE_MSG_REG 0xd8
> +#define SDRAM_ERR_CAUSE_DBE_MASK BIT(1)
> +#define SDRAM_ERR_CAUSE_SBE_MASK BIT(0)
> +
> +#define SDRAM_RANK_CTRL_REG 0x1e0
> +#define SDRAM_RANK_CTRL_EXIST_MASK(cs) BIT(cs)
Align those define names and values vertically. They can stick out and
overflow the 80 cols rule:
#define SDRAM_ERR_CTRL_ERR_THR_OFFSET 16
#define SDRAM_ERR_CTRL_ERR_THR_MASK (0xff << SDRAM_ERR_CTRL_ERR_THR_OFFSET)
#define SDRAM_ERR_CTRL_ERR_PROP_MASK BIT(9)
also, you can shorten those defines too - there's "ERR" twice in the
name.
> +
> +struct armada_xp_mc_edac_drvdata {
That struct name is too long. So long that it makes you break parameter
names below.
For example, you don't need that lenghty prefix "armada_xp_mc_edac"
prepended to static symbols. Same for the function names below.
> + void __iomem *base;
> +
> + unsigned int width; /* width in bytes */
> + bool cs_addr_sel[SDRAM_NUM_CS]; /* bank interleaving */
Put all side comments over the respective line they're commenting.
> +
> + char msg[128];
> +};
> +
> +/* derived from "DRAM Address Multiplexing" in the ARAMDA XP Functional Spec */
> +static uint32_t armada_xp_mc_edac_calc_address(struct armada_xp_mc_edac_drvdata
> + *drvdata, uint8_t cs,
> + uint8_t bank, uint16_t row,
> + uint16_t col)
> +{
> + if (drvdata->width == 8) { /* 64 bit */
> + if (drvdata->cs_addr_sel[cs]) /* bank interleaved */
> + return (((row & 0xfff8) << 16) |
> + ((bank & 0x7) << 16) |
> + ((row & 0x7) << 13) |
> + ((col & 0x3ff) << 3));
> + else
> + return (((row & 0xffff << 16) |
> + ((bank & 0x7) << 13) |
> + ((col & 0x3ff)) << 3));
> + } else if (drvdata->width == 4) { /* 32 bit */
> + if (drvdata->cs_addr_sel[cs]) /* bank interleaved */
> + return (((row & 0xfff0) << 15) |
> + ((bank & 0x7) << 16) |
> + ((row & 0xf) << 12) |
> + ((col & 0x3ff) << 2));
> + else
> + return (((row & 0xffff << 15) |
> + ((bank & 0x7) << 12) |
> + ((col & 0x3ff)) << 2));
> + } else { /* 16 bit */
> + if (drvdata->cs_addr_sel[cs]) /* bank interleaved */
> + return (((row & 0xffe0) << 14) |
> + ((bank & 0x7) << 16) |
> + ((row & 0x1f) << 11) |
> + ((col & 0x3ff) << 1));
> + else
> + return (((row & 0xffff << 14) |
> + ((bank & 0x7) << 11) |
> + ((col & 0x3ff)) << 1));
> + }
> +}
> +
> +static void armada_xp_mc_edac_check(struct mem_ctl_info *mci)
> +{
> + struct armada_xp_mc_edac_drvdata *drvdata = mci->pvt_info;
> + uint32_t data_h, data_l, recv_ecc, calc_ecc, addr;
> + uint32_t cnt_sbe, cnt_dbe, cause_err, cause_msg;
> + uint32_t row_val, col_val, bank_val, addr_val;
> + uint8_t syndrome_val, cs_val;
> + char *msg = drvdata->msg;
> +
> + data_h = readl(drvdata->base + SDRAM_ERR_DATA_H_REG);
> + data_l = readl(drvdata->base + SDRAM_ERR_DATA_L_REG);
> + recv_ecc = readl(drvdata->base + SDRAM_ERR_RECV_ECC_REG);
> + calc_ecc = readl(drvdata->base + SDRAM_ERR_CALC_ECC_REG);
> + addr = readl(drvdata->base + SDRAM_ERR_ADDR_REG);
> + cnt_sbe = readl(drvdata->base + SDRAM_ERR_SBE_COUNT_REG);
> + cnt_dbe = readl(drvdata->base + SDRAM_ERR_DBE_COUNT_REG);
> + cause_err = readl(drvdata->base + SDRAM_ERR_CAUSE_ERR_REG);
> + cause_msg = readl(drvdata->base + SDRAM_ERR_CAUSE_MSG_REG);
Align vertically for better readability:
data_h = readl(drvdata->base + SDRAM_ERR_DATA_H_REG);
data_l = readl(drvdata->base + SDRAM_ERR_DATA_L_REG);
recv_ecc = readl(drvdata->base + SDRAM_ERR_RECV_ECC_REG);
calc_ecc = readl(drvdata->base + SDRAM_ERR_CALC_ECC_REG);
...
> +
> + /* clear cause registers */
> + writel(~(SDRAM_ERR_CAUSE_DBE_MASK | SDRAM_ERR_CAUSE_SBE_MASK),
> + drvdata->base + SDRAM_ERR_CAUSE_ERR_REG);
> + writel(~(SDRAM_ERR_CAUSE_DBE_MASK | SDRAM_ERR_CAUSE_SBE_MASK),
> + drvdata->base + SDRAM_ERR_CAUSE_MSG_REG);
> +
> + /* clear error counter registers */
> + if (cnt_sbe)
> + writel(0, drvdata->base + SDRAM_ERR_SBE_COUNT_REG);
> + if (cnt_dbe)
> + writel(0, drvdata->base + SDRAM_ERR_DBE_COUNT_REG);
> +
> + if (!cnt_sbe && !cnt_dbe)
> + return;
> +
> + if ((addr & SDRAM_ERR_ADDR_TYPE_MASK) == 0) {
if (!(addr & SDRAM_ERR_ADDR_TYPE_MASK)) {
> + if (cnt_sbe)
> + cnt_sbe--;
> + else
> + dev_warn(mci->pdev, "inconsistent SBE count detected");
> + } else {
> + if (cnt_dbe)
> + cnt_dbe--;
> + else
> + dev_warn(mci->pdev, "inconsistent DBE count detected");
> + }
> +
> + /* report earlier errors */
> + if (cnt_sbe)
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + cnt_sbe, /* error count */
> + 0, 0, 0, /* pfn, offset, syndrome */
> + -1, -1, -1, /* top, mid, low layer */
> + mci->ctl_name,
> + "details unavailable (multiple errors)");
> + if (cnt_dbe)
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + cnt_sbe, /* error count */
> + 0, 0, 0, /* pfn, offset, syndrome */
> + -1, -1, -1, /* top, mid, low layer */
> + mci->ctl_name,
> + "details unavailable (multiple errors)");
> +
> + /* report details for most recent error */
> + cs_val = (addr & SDRAM_ERR_ADDR_CS_MASK)
> + >> SDRAM_ERR_ADDR_CS_OFFSET;
> + bank_val = (addr & SDRAM_ERR_ADDR_BANK_MASK)
> + >> SDRAM_ERR_ADDR_BANK_OFFSET;
> + row_val = (calc_ecc & SDRAM_ERR_CALC_ECC_ROW_MASK)
> + >> SDRAM_ERR_CALC_ECC_ROW_OFFSET;
> + col_val = (addr & SDRAM_ERR_ADDR_COL_MASK)
> + >> SDRAM_ERR_ADDR_COL_OFFSET;
Let those stick out and align vertically.
> + syndrome_val = (recv_ecc ^ calc_ecc) & 0xff;
> + addr_val = armada_xp_mc_edac_calc_address(drvdata, cs_val, bank_val,
> + row_val, col_val);
> + msg += sprintf(msg, "row=0x%04x ", row_val); /* 11 chars */
> + msg += sprintf(msg, "bank=0x%x ", bank_val); /* 9 chars */
> + msg += sprintf(msg, "col=0x%04x ", col_val); /* 11 chars */
> + msg += sprintf(msg, "cs=%d", cs_val); /* 4 chars */
> +
> + if ((addr & SDRAM_ERR_ADDR_TYPE_MASK) == 0) {
if (!(...
as above.
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + 1, /* error count */
> + addr_val >> PAGE_SHIFT,
> + addr_val & ~PAGE_MASK,
> + syndrome_val,
> + cs_val, -1, -1, /* top, mid, low layer */
> + mci->ctl_name, drvdata->msg);
> + } else {
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + 1, /* error count */
> + addr_val >> PAGE_SHIFT,
> + addr_val & ~PAGE_MASK,
> + syndrome_val,
> + cs_val, -1, -1, /* top, mid, low layer */
> + mci->ctl_name, drvdata->msg);
> + }
> +}
> +
> +static void armada_xp_mc_edac_read_config(struct mem_ctl_info *mci)
> +{
> + struct armada_xp_mc_edac_drvdata *drvdata = mci->pvt_info;
> + struct dimm_info *dimm;
> + unsigned int i, cs_struct, cs_size;
> + uint32_t config, addr_ctrl, rank_ctrl;
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
> +
> + config = readl(drvdata->base + SDRAM_CONFIG_REG);
> + if (config & SDRAM_CONFIG_BUS_WIDTH_MASK)
> + drvdata->width = 8; /* 64 bit */
> + else
> + drvdata->width = 4; /* 32 bit */
> +
> + addr_ctrl = readl(drvdata->base + SDRAM_ADDR_CTRL_REG);
> + rank_ctrl = readl(drvdata->base + SDRAM_RANK_CTRL_REG);
> + for (i = 0; i < SDRAM_NUM_CS; i++) {
> + dimm = mci->dimms[i];
> +
> + if (!(rank_ctrl & SDRAM_RANK_CTRL_EXIST_MASK(i)))
> + continue;
> +
> + drvdata->cs_addr_sel[i] =
> + !!(addr_ctrl & SDRAM_ADDR_CTRL_ADDR_SEL_MASK(i));
> +
> + cs_struct = (addr_ctrl & SDRAM_ADDR_CTRL_STRUCT_MASK(i)) >>
> + SDRAM_ADDR_CTRL_STRUCT_OFFSET(i);
> + cs_size = ((addr_ctrl & SDRAM_ADDR_CTRL_SIZE_HIGH_MASK(i)) >>
> + (SDRAM_ADDR_CTRL_SIZE_HIGH_OFFSET(i) - 2) |
> + ((addr_ctrl & SDRAM_ADDR_CTRL_SIZE_LOW_MASK(i)) >>
> + SDRAM_ADDR_CTRL_SIZE_LOW_OFFSET(i)));
Align vertically and let them stick out. As they are now, they're almost
unreadable.
> + switch (cs_size) {
> + case 0: /* 2GBit */
> + dimm->nr_pages = (0x80000000ULL >> PAGE_SHIFT);
> + break;
> + case 1: /* 256MBit */
> + dimm->nr_pages = (0x10000000ULL >> PAGE_SHIFT);
> + break;
> + case 2: /* 512MBit */
> + dimm->nr_pages = (0x20000000ULL >> PAGE_SHIFT);
> + break;
> + case 3: /* 1GBit */
> + dimm->nr_pages = (0x40000000ULL >> PAGE_SHIFT);
> + break;
> + case 4: /* 4GBit */
> + dimm->nr_pages = (0x100000000ULL >> PAGE_SHIFT);
> + break;
> + case 5: /* 8GBit */
> + dimm->nr_pages = (0x200000000ULL >> PAGE_SHIFT);
That's silly. Just write the pages in decimal numbers. You have the
comments explaining what those numbers are.
> + break;
> + }
> + dimm->grain = 8;
> + dimm->dtype = cs_struct ? DEV_X16 : DEV_X8;
> + dimm->mtype = (config & SDRAM_CONFIG_REGISTERED_MASK) ?
> + MEM_RDDR3 : MEM_DDR3;
> + dimm->edac_mode = EDAC_SECDED;
> + }
> +}
> +
> +static const struct of_device_id armada_xp_mc_edac_of_match[] = {
> + {.compatible = "marvell,armada-xp-sdram-controller",},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, armada_xp_mc_edac_of_match);
> +
> +static int armada_xp_mc_edac_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *id;
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[1];
> + struct armada_xp_mc_edac_drvdata *drvdata;
> + struct resource *r;
> + void __iomem *base;
> + uint32_t config;
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + dev_err(&pdev->dev, "Unable to get mem resource\n");
> + return -ENODEV;
> + }
> +
> + base = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev, "Unable to map regs\n");
> + return PTR_ERR(base);
> + }
> +
> + config = readl(base + SDRAM_CONFIG_REG);
> + if (!(config & SDRAM_CONFIG_ECC_MASK)) {
> + dev_warn(&pdev->dev, "SDRAM ECC is not enabled");
> + return -EINVAL;
> + }
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = SDRAM_NUM_CS;
> + layers[0].is_virt_csrow = true;
> +
> + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(*drvdata));
> + if (!mci)
> + return -ENOMEM;
> +
> + drvdata = mci->pvt_info;
> + drvdata->base = base;
> + mci->pdev = &pdev->dev;
> + platform_set_drvdata(pdev, mci);
> +
> + id = of_match_device(armada_xp_mc_edac_of_match, &pdev->dev);
> + mci->edac_check = armada_xp_mc_edac_check;
> + mci->mtype_cap = MEM_FLAG_DDR3;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->mod_name = pdev->dev.driver->name;
> + mci->ctl_name = id ? id->compatible : "unknown";
> + mci->dev_name = dev_name(&pdev->dev);
> + mci->scrub_mode = SCRUB_NONE;
> +
> + armada_xp_mc_edac_read_config(mci);
> +
> + /* configure SBE threshold */
> + /* it seems that SBEs are not captured otherwise */
> + writel(1 << SDRAM_ERR_CTRL_ERR_THR_OFFSET,
> + drvdata->base + SDRAM_ERR_CTRL_REG);
> +
> + /* clear cause registers */
> + writel(~(SDRAM_ERR_CAUSE_DBE_MASK | SDRAM_ERR_CAUSE_SBE_MASK),
> + drvdata->base + SDRAM_ERR_CAUSE_ERR_REG);
> + writel(~(SDRAM_ERR_CAUSE_DBE_MASK | SDRAM_ERR_CAUSE_SBE_MASK),
> + drvdata->base + SDRAM_ERR_CAUSE_MSG_REG);
Let those stick out and align vertically.
> +
> + /* clear counter registers */
> + writel(0, drvdata->base + SDRAM_ERR_SBE_COUNT_REG);
> + writel(0, drvdata->base + SDRAM_ERR_DBE_COUNT_REG);
> +
> + if (edac_mc_add_mc(mci)) {
> + edac_mc_free(mci);
> + return -EINVAL;
> + }
> + edac_op_state = EDAC_OPSTATE_POLL;
> +
> + return 0;
> +}
> +
> +static int armada_xp_mc_edac_remove(struct platform_device *pdev)
> +{
> + struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> +
> + edac_mc_del_mc(&pdev->dev);
> + edac_mc_free(mci);
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +static struct platform_driver armada_xp_mc_edac_driver = {
> + .probe = armada_xp_mc_edac_probe,
> + .remove = armada_xp_mc_edac_remove,
> + .driver = {
> + .name = "armada_xp_mc_edac",
> + .of_match_table = of_match_ptr(armada_xp_mc_edac_of_match),
> + },
> +};
> +
> +/************************ EDAC Device (L2 Cache) ***************************/
> +
> +struct aurora_l2_edac_drvdata {
> + void __iomem *base;
> +
> + char msg[128];
> +
> + /* error injection via debugfs */
> + uint32_t inject_addr;
> + uint32_t inject_mask;
> + uint8_t inject_ctl;
> +
> + struct dentry *debugfs;
> +};
> +
> +static void aurora_l2_edac_inject(struct aurora_l2_edac_drvdata *drvdata)
The whole EDAC error injection functionality should be behind
CONFIG_EDAC_DEBUG. You don't want error injection capabilities exposed
on a production system.
> +{
> + drvdata->inject_addr &= AURORA_ERR_INJECT_CTL_ADDR_MASK;
> + drvdata->inject_ctl &= AURORA_ERR_INJECT_CTL_EN_MASK;
> + writel(0, drvdata->base + AURORA_ERR_INJECT_CTL_REG);
> + writel(drvdata->inject_mask,
> + drvdata->base + AURORA_ERR_INJECT_MASK_REG);
> + writel(drvdata->inject_addr | drvdata->inject_ctl,
> + drvdata->base + AURORA_ERR_INJECT_CTL_REG);
Let those stick out and align vertically.
> +}
> +
> +static void aurora_l2_edac_check(struct edac_device_ctl_info *dci)
> +{
> + struct aurora_l2_edac_drvdata *drvdata = dci->pvt_info;
> + uint32_t cnt, attr_cap, addr_cap, way_cap;
> + unsigned int cnt_ce, cnt_ue;
> +
> + cnt = readl(drvdata->base + AURORA_ERR_CNT_REG);
> + attr_cap = readl(drvdata->base + AURORA_ERR_ATTR_CAP_REG);
> + addr_cap = readl(drvdata->base + AURORA_ERR_ADDR_CAP_REG);
> + way_cap = readl(drvdata->base + AURORA_ERR_WAY_CAP_REG);
> +
> + cnt_ce = (cnt & AURORA_ERR_CNT_CE_MASK) >> AURORA_ERR_CNT_CE_OFFSET;
> + cnt_ue = (cnt & AURORA_ERR_CNT_UE_MASK) >> AURORA_ERR_CNT_UE_OFFSET;
> + /* clear error counter registers */
> + if (cnt_ce || cnt_ue)
> + writel(AURORA_ERR_CNT_CLR, drvdata->base + AURORA_ERR_CNT_REG);
> +
> + if (attr_cap & AURORA_ERR_ATTR_CAP_VALID) {
if (!(attr_cap & AURORA_ERR_ATTR_CAP_VALID))
goto clear_remaining;
saves you an indentation level.
> + char *msg = drvdata->msg;
> + size_t size = sizeof(drvdata->msg);
> + size_t len = 0;
> +
> + switch ((attr_cap & AURORA_ERR_ATTR_CAP_ERR_SOURCE_MASK)
> + >> AURORA_ERR_ATTR_CAP_ERR_SOURCE_OFFSET) {
> + case 0:
> + len += snprintf(msg+len, size-len, "src=CPU0 ");
> + break;
> + case 1:
> + len += snprintf(msg+len, size-len, "src=CPU1 ");
> + break;
> + case 2:
> + len += snprintf(msg+len, size-len, "src=CPU2 ");
> + break;
> + case 3:
> + len += snprintf(msg+len, size-len, "src=CPU3 ");
> + break;
> + case 7:
> + len += snprintf(msg+len, size-len, "src=IO ");
> + break;
> + }
Lemme try to shorten that:
src = (attr_cap & AURORA_ERR_ATTR_SRC_MSK) >> AURORA_ERR_ATTR_SRC_OFF;
if (src <= 3)
len += snprintf(msg+len, size-len, "src=CPU%d ", src);
else
len += snprintf(msg+len, size-len, "src=IO ");
Yeah, those AURORA_* defines could be shorter.
> + switch ((attr_cap & AURORA_ERR_ATTR_CAP_TRANS_TYPE_MASK)
> + >> AURORA_ERR_ATTR_CAP_TRANS_TYPE_OFFSET) {
Ditto.
> + case 0:
> + len += snprintf(msg+len, size-len, "txn=Data-Read ");
> + break;
> + case 1:
> + len += snprintf(msg+len, size-len, "txn=Isn-Read ");
> + break;
> + case 2:
> + len += snprintf(msg+len, size-len, "txn=Clean-Flush ");
> + break;
> + case 3:
> + len += snprintf(msg+len, size-len, "txn=Eviction ");
> + break;
> + case 4:
> + len += snprintf(msg+len, size-len,
> + "txn=Read-Modify-Write ");
> + break;
> + }
> + switch ((attr_cap & AURORA_ERR_ATTR_CAP_ERR_TYPE_MASK)
> + >> AURORA_ERR_ATTR_CAP_ERR_TYPE_OFFSET) {
> + case 0:
> + len += snprintf(msg+len, size-len, "err=CorrECC ");
> + break;
> + case 1:
> + len += snprintf(msg+len, size-len, "err=UnCorrECC ");
> + break;
> + case 2:
> + len += snprintf(msg+len, size-len, "err=TagParity ");
> + break;
> + }
> + len += snprintf(msg+len, size-len, "addr=0x%x ",
> + addr_cap & AURORA_ERR_ADDR_CAP_ADDR_MASK);
> + len += snprintf(msg+len, size-len, "index=0x%x ",
> + (way_cap & AURORA_ERR_WAY_CAP_INDEX_MASK)
> + >> AURORA_ERR_WAY_CAP_INDEX_OFFSET);
> + len += snprintf(msg+len, size-len, "way=0x%x",
> + (way_cap & AURORA_ERR_WAY_CAP_WAY_MASK)
> + >> AURORA_ERR_WAY_CAP_WAY_OFFSET);
Let it stick out.
<---- newline here.
> + /* clear error capture registers */
> + writel(AURORA_ERR_ATTR_CAP_VALID,
> + drvdata->base + AURORA_ERR_ATTR_CAP_REG);
Let it stick out.
> + if ((attr_cap & AURORA_ERR_ATTR_CAP_ERR_TYPE_MASK)
> + >> AURORA_ERR_ATTR_CAP_ERR_TYPE_OFFSET) {
Ditto.
> + /* UnCorrECC or TagParity */
> + if (cnt_ue)
> + cnt_ue--;
> + edac_device_handle_ue(dci, 0, 0, drvdata->msg);
> + } else {
> + if (cnt_ce)
> + cnt_ce--;
> + edac_device_handle_ce(dci, 0, 0, drvdata->msg);
> + }
> + }
> +
> + /* report remaining errors */
> + while (cnt_ue--)
> + edac_device_handle_ue(dci, 0, 0,
> + "details unavailable (multiple errors)");
> + while (cnt_ce--)
> + edac_device_handle_ue(dci, 0, 0,
> + "details unavailable (multiple errors)");
Ditto. You get the idea...
> +}
> +
> +static void aurora_l2_edac_poll(struct edac_device_ctl_info *dci)
> +{
> + struct aurora_l2_edac_drvdata *drvdata = dci->pvt_info;
> +
> + aurora_l2_edac_check(dci);
> + aurora_l2_edac_inject(drvdata);
> +}
> +
> +static const struct of_device_id aurora_l2_edac_of_match[] = {
> + {.compatible = "marvell,aurora-system-cache",},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, aurora_l2_edac_of_match);
> +
> +static int aurora_l2_edac_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *id;
> + struct edac_device_ctl_info *dci;
> + struct aurora_l2_edac_drvdata *drvdata;
> + struct resource *r;
> + void __iomem *base;
> + uint32_t l2x0_aux_ctrl;
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + dev_err(&pdev->dev, "Unable to get mem resource\n");
> + return -ENODEV;
> + }
> +
> + base = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev, "Unable to map regs\n");
> + return PTR_ERR(base);
> + }
> +
> + l2x0_aux_ctrl = readl(base + L2X0_AUX_CTRL);
> + if (!(l2x0_aux_ctrl & AURORA_ACR_PARITY_EN))
> + dev_warn(&pdev->dev, "tag parity is not enabled");
> + if (!(l2x0_aux_ctrl & AURORA_ACR_ECC_EN))
> + dev_warn(&pdev->dev, "data ECC is not enabled");
> +
> + dci = edac_device_alloc_ctl_info(sizeof(*drvdata),
> + "cpu", 1, "L", 1, 2, NULL, 0, 0);
> + if (!dci)
> + return -ENOMEM;
> +
> + drvdata = dci->pvt_info;
> + drvdata->base = base;
> + dci->dev = &pdev->dev;
> + platform_set_drvdata(pdev, dci);
> +
> + id = of_match_device(aurora_l2_edac_of_match, &pdev->dev);
> + dci->edac_check = aurora_l2_edac_poll;
> + dci->mod_name = pdev->dev.driver->name;
> + dci->ctl_name = id ? id->compatible : "unknown";
> + dci->dev_name = dev_name(&pdev->dev);
> +
> + /* clear registers */
> + writel(AURORA_ERR_CNT_CLR, drvdata->base + AURORA_ERR_CNT_REG);
> + writel(AURORA_ERR_ATTR_CAP_VALID,
> + drvdata->base + AURORA_ERR_ATTR_CAP_REG);
> +
> + if (edac_device_add_device(dci)) {
> + edac_device_free_ctl_info(dci);
> + return -EINVAL;
> + }
> +
> + drvdata->debugfs = edac_debugfs_create_dir(dev_name(&pdev->dev));
> + if (drvdata->debugfs) {
> + edac_debugfs_create_x32("inject_addr", 0644,
> + drvdata->debugfs,
> + &drvdata->inject_addr);
> + edac_debugfs_create_x32("inject_mask", 0644,
> + drvdata->debugfs,
> + &drvdata->inject_mask);
> + edac_debugfs_create_x8("inject_ctl", 0644,
> + drvdata->debugfs, &drvdata->inject_ctl);
> + }
> +
> + return 0;
> +}
> +
> +static int aurora_l2_edac_remove(struct platform_device *pdev)
> +{
> + struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
> + struct aurora_l2_edac_drvdata *drvdata = dci->pvt_info;
> +
> + edac_debugfs_remove_recursive(drvdata->debugfs);
> + edac_device_del_device(&pdev->dev);
> + edac_device_free_ctl_info(dci);
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +static struct platform_driver aurora_l2_edac_driver = {
> + .probe = aurora_l2_edac_probe,
> + .remove = aurora_l2_edac_remove,
> + .driver = {
> + .name = "aurora_l2_edac",
> + .of_match_table = of_match_ptr(aurora_l2_edac_of_match),
> + },
> +};
> +
> +/************************ Driver registration ******************************/
> +
> +static struct platform_driver * const drivers[] = {
> + &armada_xp_mc_edac_driver,
> + &aurora_l2_edac_driver,
> +};
> +
> +static int __init armada_xp_edac_init(void)
> +{
> + int res = 0;
No need to zero it.
> +
> + /* only polling is supported */
> + edac_op_state = EDAC_OPSTATE_POLL;
> +
> + res = platform_register_drivers(drivers, ARRAY_SIZE(drivers));
> + if (res)
> + pr_warn("Aramda XP EDAC drivers fail to register\n");
> +
> + return 0;
> +}
> +module_init(armada_xp_edac_init);
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
More information about the linux-arm-kernel
mailing list