On Tue, Nov 07, 2017 at 10:38:58PM +0200, Tero Kristo wrote: > TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can > correct one bit errors and detect two bit errors. Add EDAC driver for this > feature which plugs into the generic kernel EDAC framework. > > Signed-off-by: Tero Kristo <t-kristo@xxxxxx> > Cc: Santosh Shilimkar <ssantosh@xxxxxxxxxx> > Cc: Tony Lindgren <tony@xxxxxxxxxxx> Please add yourself and whoever else wants to get CCed on bug reports for this driver, to MAINTAINERS. Also, please integrate scripts/checkpatch.pl into your patch creation workflow. Some of the warnings/errors *actually* make sense. > --- > drivers/edac/Kconfig | 7 ++ > drivers/edac/Makefile | 1 + > drivers/edac/ti_edac.c | 306 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 314 insertions(+) > create mode 100644 drivers/edac/ti_edac.c > > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index 96afb2a..54f0184 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -457,4 +457,11 @@ config EDAC_XGENE > Support for error detection and correction on the > APM X-Gene family of SOCs. > > +config EDAC_TI > + tristate "Texas Instruments DDR3 ECC handler" s/handler/Controller/ > + depends on ARCH_KEYSTONE || SOC_DRA7XX > + help > + Support for error detection and correction on the > + TI SoCs. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile > index 0fd9ffa..b54912e 100644 > --- a/drivers/edac/Makefile > +++ b/drivers/edac/Makefile > @@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_THUNDERX) += thunderx_edac.o > obj-$(CONFIG_EDAC_ALTERA) += altera_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/ti_edac.c b/drivers/edac/ti_edac.c > new file mode 100644 > index 0000000..54bacf3 > --- /dev/null > +++ b/drivers/edac/ti_edac.c > @@ -0,0 +1,306 @@ > +/* > + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/ > + * > + * Texas Instruments DDR3 ECC error correction and detection driver > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/init.h> > +#include <linux/edac.h> > +#include <linux/io.h> > +#include <linux/interrupt.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/module.h> > + > +#include "edac_module.h" > + > +/* EMIF controller registers */ > +#define EMIF_SDRAM_CONFIG 0x008 > +#define EMIF_IRQ_STATUS 0x0ac > +#define EMIF_IRQ_ENABLE_SET 0x0b4 > +#define EMIF_ECC_CTRL 0x110 > +#define EMIF_1B_ECC_ERR_CNT 0x130 > +#define EMIF_1B_ECC_ERR_THRSH 0x134 > +#define EMIF_1B_ECC_ERR_ADDR_LOG 0x13c > +#define EMIF_2B_ECC_ERR_ADDR_LOG 0x140 > + > +/* Bit definitions for EMIF_SDRAM_CONFIG */ > +#define SDRAM_TYPE_SHIFT 29 > +#define SDRAM_TYPE_MASK GENMASK(31,29) ERROR: space required after that ',' (ctx:VxV) #100: FILE: drivers/edac/ti_edac.c:41: +#define SDRAM_TYPE_MASK GENMASK(31,29) ^ ditto for the rest > +#define SDRAM_TYPE_DDR3 (3 << SDRAM_TYPE_SHIFT) > +#define SDRAM_TYPE_DDR2 (2 << SDRAM_TYPE_SHIFT) > +#define SDRAM_NARROW_MODE_MASK GENMASK(15,14) > +#define SDRAM_K2_NARROW_MODE_SHIFT 12 > +#define SDRAM_K2_NARROW_MODE_MASK GENMASK(13,12) > +#define SDRAM_ROWSIZE_SHIFT 7 > +#define SDRAM_ROWSIZE_MASK GENMASK(9,7) > +#define SDRAM_IBANK_SHIFT 4 > +#define SDRAM_IBANK_MASK GENMASK(6,4) > +#define SDRAM_K2_IBANK_SHIFT 5 > +#define SDRAM_K2_IBANK_MASK GENMASK(6,5) > +#define SDRAM_K2_EBANK_SHIFT 3 > +#define SDRAM_K2_EBANK_MASK BIT(SDRAM_K2_EBANK_SHIFT) > +#define SDRAM_PAGESIZE_SHIFT 0 > +#define SDRAM_PAGESIZE_MASK GENMASK(2,0) > +#define SDRAM_K2_PAGESIZE_SHIFT 0 > +#define SDRAM_K2_PAGESIZE_MASK GENMASK(1,0) > + > +#define EMIF_1B_ECC_ERR_THRSH_SHIFT 24 > + > +/* IRQ bit definitions */ > +#define EMIF_1B_ECC_ERR BIT(5) > +#define EMIF_2B_ECC_ERR BIT(4) > +#define EMIF_WR_ECC_ERR BIT(3) > +#define EMIF_SYS_ERR BIT(0) > +/* Bit 31 enables ECC and 28 enables RMW */ > +#define ECC_ENABLED (BIT(31) | BIT(28)) ... > +static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type) > +{ > + struct dimm_info *dimm; > + struct ti_edac *edac = mci->pvt_info; > + int bits; > + u32 val; > + u32 memsize; > + > + dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0); > + > + val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG); > + > + if (type == EMIF_TYPE_DRA7) { > + bits = ((val & SDRAM_PAGESIZE_MASK) >> > + SDRAM_PAGESIZE_SHIFT) + 8; > + bits += ((val & SDRAM_ROWSIZE_MASK) >> > + SDRAM_ROWSIZE_SHIFT) + 9; Bah, let those stick out - it is more readable this way: bits = ((val & SDRAM_PAGESIZE_MASK) >> SDRAM_PAGESIZE_SHIFT) + 8; bits += ((val & SDRAM_ROWSIZE_MASK) >> SDRAM_ROWSIZE_SHIFT) + 9; bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT; > + bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT; > + > + if (val & SDRAM_NARROW_MODE_MASK) { > + bits++; > + dimm->dtype = DEV_X16; > + } else { > + bits += 2; > + dimm->dtype = DEV_X32; > + } > + } else { > + bits = 16; > + bits += ((val & SDRAM_K2_PAGESIZE_MASK) >> > + SDRAM_K2_PAGESIZE_SHIFT) + 8; > + bits += (val & SDRAM_K2_IBANK_MASK) >> SDRAM_K2_IBANK_SHIFT; > + bits += (val & SDRAM_K2_EBANK_MASK) >> SDRAM_K2_EBANK_SHIFT; > + > + val = (val & SDRAM_K2_NARROW_MODE_MASK) >> > + SDRAM_K2_NARROW_MODE_SHIFT; > + switch (val) { > + case 0: > + bits += 3; > + dimm->dtype = DEV_X64; > + break; > + case 1: > + bits += 2; > + dimm->dtype = DEV_X32; > + break; > + case 2: > + bits ++; ERROR: space prohibited before that '++' (ctx:WxO) #233: FILE: drivers/edac/ti_edac.c:174: + bits ++; ^ > + dimm->dtype = DEV_X16; > + break; > + } > + } > + > + memsize = 1 << bits; > + > + dimm->nr_pages = memsize >> PAGE_SHIFT; > + dimm->grain = 4; > + if ((val & SDRAM_TYPE_MASK) == SDRAM_TYPE_DDR2) > + dimm->mtype = MEM_DDR2; > + else > + dimm->mtype = MEM_DDR3; > + > + val = ti_edac_readl(edac, EMIF_ECC_CTRL); > + if (val & ECC_ENABLED) > + dimm->edac_mode = EDAC_SECDED; > + else > + dimm->edac_mode = EDAC_NONE; So if ECC is not enabled, why do you even need to continue loading the driver here and not return a retval which ti_edac_probe() propagates? Also, you want to call that function much earlier in ti_edac_probe() so that you can save yourself all the setup. Or can you have instances which have ECC disabled and others which have ECC enabled, on the same platform? > +} > + > +static const struct of_device_id ti_edac_of_match[] = { > + { .compatible = "ti,emif-keystone", .data = (void *)EMIF_TYPE_K2 }, > + { .compatible = "ti,emif-dra7xx", .data = (void *)EMIF_TYPE_DRA7 }, > + {}, > +}; > + > +static int ti_edac_probe(struct platform_device *pdev) > +{ > + int error_irq = 0, ret = -ENODEV; > + struct device *dev = &pdev->dev; > + struct resource *res; > + void __iomem *reg; > + struct mem_ctl_info *mci; > + struct edac_mc_layer layers[1]; > + const struct of_device_id *id; > + struct ti_edac *edac; > + static int edac_id; > + int my_id; > + > + id = of_match_device(ti_edac_of_match, &pdev->dev); > + if (!id) > + return -ENODEV; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + reg = devm_ioremap_resource(dev, res); > + if (IS_ERR(reg)) { > + dev_err(dev, "DDR3 controller regs not defined\n"); We have edac_printk() et al macros for printing. Ditto for the remaining calls. > + return PTR_ERR(reg); > + } > + > + layers[0].type = EDAC_MC_LAYER_ALL_MEM; > + layers[0].size = 1; > + > + /* Allocate ID number for our EMIF controller */ > + mutex_lock(&ti_edac_lock); > + my_id = edac_id++; > + mutex_unlock(&ti_edac_lock); That looks silly. Why not atomic_inc_return()? > + mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac)); > + if (!mci) > + return -ENOMEM; > + > + mci->pdev = &pdev->dev; > + edac = mci->pvt_info; > + edac->reg = reg; > + platform_set_drvdata(pdev, mci); ... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
