From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754647AbaK0LAA (ORCPT ); Thu, 27 Nov 2014 06:00:00 -0500 Received: from mail-bn1bon0078.outbound.protection.outlook.com ([157.56.111.78]:25632 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753427AbaK0K75 (ORCPT ); Thu, 27 Nov 2014 05:59:57 -0500 Date: Thu, 27 Nov 2014 11:59:39 +0100 From: Michal Simek User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Punnaiah Choudary Kalluri , , , , , , , , , , , , , , CC: , , Subject: Re: [PATCH v4] edac: synps: Added EDAC support for zynq ddr ecc controller References: <1416932548-25094-1-git-send-email-punnaia@xilinx.com> In-Reply-To: <1416932548-25094-1-git-send-email-punnaia@xilinx.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-RCIS-Action: ALLOW X-TM-AS-Product-Ver: IMSS-7.1.0.1224-7.5.0.1018-21132.006 X-TM-AS-User-Approved-Sender: Yes Message-ID: <2a4244f4a9594432837db1c82e701a04@BY2FFO11FD013.protection.gbl> X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:62.221.5.235;CTRY:GB;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(438002)(377454003)(51444003)(51704005)(189002)(24454002)(479174003)(164054003)(199003)(22564002)(23746002)(120916001)(46102003)(62966003)(77156002)(108616004)(92566001)(86362001)(99396003)(95666004)(64706001)(83506001)(31966008)(50986999)(104016003)(4396001)(87936001)(76176999)(107046002)(65956001)(6806004)(106466001)(47776003)(64126003)(21056001)(102836001)(20776003)(74316001)(65806001)(50466002)(54356999)(19580405001)(44976005)(19580395003)(2201001)(65826006)(107986001)(2004002)(921003)(1121002)(24736002)(2101003)(83996005);DIR:OUT;SFP:1101;SCL:1;SRVR:BY2FFO11HUB057;H:xir-pvapsmtpgw01;FPR:;MLV:sfv;PTR:unknown-62-221-5-235.ipspace.xilinx.com;MX:1;A:1;LANG:en; X-Microsoft-Antispam: UriScan:; X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:;SRVR:BY2FFO11HUB057; X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:;SRVR:BY2FFO11HUB057; X-Forefront-PRVS: 040866B734 Authentication-Results: spf=pass (sender IP is 62.221.5.235) smtp.mailfrom=michal.simek@xilinx.com; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:;SRVR:BY2FFO11HUB057; X-OriginatorOrg: xilinx.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Punnaiah, On 11/25/2014 05:22 PM, Punnaiah Choudary Kalluri wrote: > Added EDAC support for reporting the ecc errors of synopsys ddr controller. > The ddr ecc controller corrects single bit errors and detects double bit > errors. > > Signed-off-by: Punnaiah Choudary Kalluri > --- > Changes for v4: > - Shorten the macro definitions > - Corrected the ddr ip version > - Reverted the file name change > Changes for v3: > - Updated maintainer information > - Driver cleanup as per the review comments > - Shortened the prefix "sysnopsys" to "synps" > Changes for v2: > - Updated the commit header and message > - Renamed the filenames to synopsys_edac > - Corrected the compatilble string, commnets > - Renamed the macros,fucntions and data structures > --- > .../devicetree/bindings/edac/synopsys_edac.txt | 18 + > MAINTAINERS | 1 + > drivers/edac/Kconfig | 7 + > drivers/edac/Makefile | 1 + > drivers/edac/synopsys_edac.c | 543 ++++++++++++++++++++ > 5 files changed, 570 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/edac/synopsys_edac.txt > create mode 100644 drivers/edac/synopsys_edac.c > > diff --git a/Documentation/devicetree/bindings/edac/synopsys_edac.txt b/Documentation/devicetree/bindings/edac/synopsys_edac.txt Binding is already here Documentation/devicetree/bindings/memory-controllers/synopsys.txt can you please just add there ECC information? > new file mode 100644 > index 0000000..a64497a > --- /dev/null > +++ b/Documentation/devicetree/bindings/edac/synopsys_edac.txt > @@ -0,0 +1,18 @@ > +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are > +corrected and double bit ecc errors that are detected by the DDR ECC controller. > +ECC support for DDR is available in half-bus width(16 bit) configuration only. > + > +Required properties: > +- compatible: Should be "xlnx,zynq-ddrc-A05" > +- reg: Should contain DDR controller registers location and length. > + > +Example: > +++++++++ > + > +ddrc0: ddrc@f8006000 { > + compatible = "xlnx,zynq-ddrc-A05"; here should be a05 not A05. > + reg = <0xf8006000 0x1000>; > +}; > + > +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate > +control register. > diff --git a/MAINTAINERS b/MAINTAINERS > index f10ed39..4a16f97 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1522,6 +1522,7 @@ N: xilinx > F: drivers/clocksource/cadence_ttc_timer.c > F: drivers/i2c/busses/i2c-cadence.c > F: drivers/mmc/host/sdhci-of-arasan.c > +F: drivers/edac/synopsys_edac.c > > ARM SMMU DRIVER > M: Will Deacon > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index 7072c28..c6d448e 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -385,4 +385,11 @@ config EDAC_ALTERA_MC > preloader must initialize the SDRAM before loading > the kernel. > > +config EDAC_SYNOPSYS > + tristate "Synopsys DDR Memory Controller" > + depends on EDAC_MM_EDAC && ARCH_ZYNQ > + help > + Support for EDAC on the ECC memory used with the Synopsys DDR > + memory controller. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile > index 359aa49..9b5a095 100644 > --- a/drivers/edac/Makefile > +++ b/drivers/edac/Makefile > @@ -67,3 +67,4 @@ obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o > obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o > > obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o > +obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o > diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c > new file mode 100644 > index 0000000..3522432 > --- /dev/null > +++ b/drivers/edac/synopsys_edac.c > @@ -0,0 +1,543 @@ > +/* > + * Synopsys DDR ECC Driver > + * This driver is based on ppc4xx_edac.c drivers > + * > + * Copyright (C) 2012 - 2014 Xilinx, Inc. > + * > + * 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. > + * > + * 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. > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details > + */ > + > +#include > +#include > +#include > + > +#include "edac_core.h" > + > +/* Number of cs_rows needed per memory controller */ > +#define SYNPS_EDAC_NR_CSROWS 1 > + > +/* Number of channels per memory controller */ > +#define SYNPS_EDAC_NR_CHANS 1 > + > +/* Granularity of reported error in bytes */ > +#define SYNPS_EDAC_ERR_GRAIN 1 > + > +#define SYNPS_EDAC_MSG_SIZE 256 > + > +#define SYNPS_EDAC_MOD_STRING "synps_edac" > +#define SYNPS_EDAC_MOD_VER "1" > + > +/* Synopsys DDR memory controller registers that are relevant to ECC */ > +#define CTRL_OFST 0x0 > +#define T_ZQ_OFST 0xA4 > + > +/* ECC control register */ > +#define ECC_CTRL_OFST 0xC4 > +/* ECC log register */ > +#define CE_LOG_OFST 0xC8 > +/* ECC address register */ > +#define CE_ADDR_OFST 0xCC > +/* ECC data[31:0] register */ > +#define CE_DATA_31_0_OFST 0xD0 > + > +/* Uncorrectable error info regsisters */ > +#define UE_LOG_OFST 0xDC > +#define UE_ADDR_OFST 0xE0 > +#define UE_DATA_31_0_OFST 0xE4 > + > +#define STAT_OFST 0xF0 > +#define SCRUB_OFST 0xF4 > + > +/* Control regsiter bitfield definitions */ > +#define CTRL_BW_MASK 0xC > +#define CTRL_BW_SHIFT 2 > + > +#define DDRCTL_WDTH_16 1 > +#define DDRCTL_WDTH_32 0 > + > +/* ZQ register bitfield definitions */ > +#define T_ZQ_DDRMODE_MASK 0x2 > + > +/* ECC control register bitfield definitions */ > +#define ECC_CTRL_CLR_CE_ERR 0x2 > +#define ECC_CTRL_CLR_UE_ERR 0x1 > + > +/* ECC correctable/uncorrectable error log register definitions */ > +#define CE_LOG_VALID 0x1 > +#define CE_LOG_BITPOS_MASK 0xFE > +#define CE_LOG_BITPOS_SHIFT 1 > + > +/* ECC correctable/uncorrectable error address register definitions */ > +#define ADDR_COL_MASK 0xFFF > +#define ADDR_ROW_MASK 0xFFFF000 > +#define ADDR_ROW_SHIFT 12 > +#define ADDR_BANK_MASK 0x70000000 > +#define ADDR_BANK_SHIFT 28 > + > +/* ECC statistic regsiter definitions */ > +#define STAT_UECNT_MASK 0xFF > +#define STAT_CECNT_MASK 0xFF00 > +#define STAT_CECNT_SHIFT 8 > + > +/* ECC scrub regsiter definitions */ > +#define SCRUB_MODE_MASK 0x7 > +#define SCRUB_MODE_SECDED 0x4 > + > +/** > + * struct ecc_error_info - ECC error log information > + * @row: Row number > + * @col: Column number > + * @bank: Bank number > + * @bitpos: Bit position > + * @data: Data causing the error > + */ > +struct ecc_error_info { > + u32 row; > + u32 col; > + u32 bank; > + u32 bitpos; > + u32 data; > +}; > + > +/** > + * struct synps_ecc_status - ECC status information to report > + * @ce_count: Correctable error count > + * @ue_count: Uncorrectable error count > + * @ceinfo: Correctable error log information > + * @ueinfo: Uncorrectable error log information > + */ > +struct synps_ecc_status { > + u32 ce_cnt; > + u32 ue_cnt; > + struct ecc_error_info ceinfo; > + struct ecc_error_info ueinfo; > +}; > + > +/** > + * struct synps_edac_priv - DDR memory controller private instance data > + * @baseaddr: Base address of the DDR controller > + * @ce_count: Correctable Error count > + * @ue_count: Uncorrectable Error count > + */ > +struct synps_edac_priv { > + void __iomem *baseaddr; > + u32 ce_cnt; > + u32 ue_cnt; > +}; > + > +/** > + * synps_edac_geterror_info - Get the current ecc error info > + * @base: Pointer to the base address of the ddr memory controller > + * @perrstatus: Pointer to the synopsys ecc status structure > + * > + * This routine determines there is any ecc error or not > + * > + * Return: zero if there is no error otherwise returns 1 > + */ > +static int synps_edac_geterror_info(void __iomem *base, > + struct synps_ecc_status *p) > +{ This is driver internal function. I think that will be just better to return 0 on success and different values otherwise. It should be easy to change it. > + u32 regval; > + u32 clearval = 0; > + > + regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK); > + > + if (!regval) > + return 0; > + > + memset(p, 0, sizeof(*p)); > + > + p->ce_cnt = (regval & STAT_CECNT_MASK) >> STAT_CECNT_SHIFT; > + p->ue_cnt = regval & STAT_UECNT_MASK; > + > + regval = readl(base + CE_LOG_OFST); > + if (!(p->ce_cnt && (regval & CE_LOG_VALID))) > + goto ue_err; > + > + p->ceinfo.bitpos = (regval & CE_LOG_BITPOS_MASK) >> CE_LOG_BITPOS_SHIFT; > + regval = readl(base + CE_ADDR_OFST); > + p->ceinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT; > + p->ceinfo.col = regval & ADDR_COL_MASK; > + p->ceinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT; > + p->ceinfo.data = readl(base + CE_DATA_31_0_OFST); > + edac_dbg(3, "ce bitposition: %d data: %d\n", p->ceinfo.bitpos, > + p->ceinfo.data); > + clearval = ECC_CTRL_CLR_CE_ERR; > + > +ue_err: > + regval = readl(base + UE_LOG_OFST); > + if (!(p->ue_cnt && (regval & CE_LOG_VALID))) > + goto out; > + > + regval = readl(base + UE_ADDR_OFST); > + p->ueinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT; > + p->ueinfo.col = regval & ADDR_COL_MASK; > + p->ueinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT; > + p->ueinfo.data = readl(base + UE_DATA_31_0_OFST); > + clearval |= ECC_CTRL_CLR_UE_ERR; > + > +out: > + writel(clearval, base + ECC_CTRL_OFST); > + writel(0x0, base + ECC_CTRL_OFST); > + > + return 1; > +} > + > +/** > + * synps_edac_handle_error - Handle controller error types CE and UE > + * @mci: Pointer to the edac memory controller instance > + * @perrstatus: Pointer to the synopsys ecc status structure > + * > + * This routine handles the controller ECC correctable and un correctable > + * error. > + */ > +static void synps_edac_handle_error(struct mem_ctl_info *mci, > + struct synps_ecc_status *p) > +{ > + char message[SYNPS_EDAC_MSG_SIZE]; > + struct ecc_error_info *pinf; > + > + if (p->ce_cnt) { > + pinf = &p->ceinfo; > + snprintf(message, SYNPS_EDAC_MSG_SIZE, > + "DDR ECC error type :%s Row %d Bank %d Col %d ", > + "CE", pinf->row, pinf->bank, pinf->col); > + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, > + p->ce_cnt, 0, 0, 0, 0, 0, -1, > + message, ""); > + } > + > + if (p->ue_cnt) { > + pinf = &p->ueinfo; > + snprintf(message, SYNPS_EDAC_MSG_SIZE, > + "DDR ECC error type :%s Row %d Bank %d Col %d ", > + "UE", pinf->row, pinf->bank, pinf->col); > + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, > + p->ue_cnt, 0, 0, 0, 0, 0, -1, > + message, ""); > + } > +} > + > +/** > + * synps_edac_check - Check controller for ECC errors > + * @mci: Pointer to the edac memory controller instance > + * > + * This routine is used to check and post ECC errors and is called by > + * the EDAC polling thread > + */ > +static void synps_edac_check(struct mem_ctl_info *mci) > +{ > + struct synps_edac_priv *priv = mci->pvt_info; > + struct synps_ecc_status stat; > + int status; > + > + status = synps_edac_geterror_info(priv->baseaddr, &stat); > + if (!status) > + return; > + > + priv->ce_cnt += stat.ce_cnt; > + priv->ue_cnt += stat.ue_cnt; > + synps_edac_handle_error(mci, &stat); > + > + edac_dbg(3, "Total error count ce %d ue %d\n", > + priv->ce_cnt, priv->ue_cnt); > +} > + > +/** > + * synps_edac_get_dtype - Return the controller memory width > + * @base: Pointer to the ddr memory contoller base address > + * > + * This routine returns the EDAC device type width appropriate for the > + * current controller configuration. > + * > + * Return: a device type width enumeration. > + */ > +static enum dev_type synps_edac_get_dtype(const void __iomem *base) > +{ > + enum dev_type dt; > + u32 width; > + > + width = readl(base + CTRL_OFST); > + width = (width & CTRL_BW_MASK) >> CTRL_BW_SHIFT; > + > + switch (width) { > + case DDRCTL_WDTH_16: > + dt = DEV_X2; > + break; > + case DDRCTL_WDTH_32: > + dt = DEV_X4; > + break; > + default: > + dt = DEV_UNKNOWN; > + } > + > + return dt; > +} > + > +/** > + * synps_edac_get_eccstate - Return the controller ecc enable/disable status > + * @base: Pointer to the ddr memory contoller base address > + * > + * This routine returns the ECC enable/diable status for the controller > + * > + * Return: a ecc status boolean i.e true/false - enabled/disabled. > + */ > +static bool synps_edac_get_eccstate(void __iomem *base) > +{ > + enum dev_type dt; > + u32 ecctype; > + bool state = false; > + > + dt = synps_edac_get_dtype(base); > + if (dt == DEV_UNKNOWN) > + return state; > + > + ecctype = readl(base + SCRUB_OFST) & SCRUB_MODE_MASK; > + > + if ((ecctype == SCRUB_MODE_SECDED) && (dt == DEV_X2)) { > + state = true; > + writel(0x0, base + ECC_CTRL_OFST); > + } > + > + return state; > +} > + > +/** > + * synps_edac_get_memsize - reads the size of the attached memory device > + * > + * Return: the memory size in bytes > + */ > +static u32 synps_edac_get_memsize(void) > +{ > + struct sysinfo inf; > + > + si_meminfo(&inf); > + > + return inf.totalram * inf.mem_unit; > +} > + > +/** > + * synps_edac_get_mtype - Returns controller memory type > + * @base: pointer to the synopsys ecc status structure > + * > + * This routine returns the EDAC memory type appropriate for the > + * current controller configuration. > + * > + * Return: a memory type enumeration. > + */ > +static enum mem_type synps_edac_get_mtype(const void __iomem *base) > +{ > + enum mem_type mt; > + u32 memtype; > + > + memtype = readl(base + T_ZQ_OFST); > + > + if (memtype & T_ZQ_DDRMODE_MASK) > + mt = MEM_DDR3; > + else > + mt = MEM_DDR2; > + > + return mt; > +} > + > +/** > + * synps_edac_init_csrows - Initialize the cs row data > + * @mci: Pointer to the edac memory controller instance > + * > + * This routine initializes the chip select rows associated > + * with the EDAC memory controller instance > + * > + * Return: Unconditionally 0. > + */ > +static int synps_edac_init_csrows(struct mem_ctl_info *mci) > +{ > + struct csrow_info *csi; > + struct dimm_info *dimm; > + struct synps_edac_priv *priv = mci->pvt_info; > + u32 size; > + int row, j; > + > + for (row = 0; row < mci->nr_csrows; row++) { > + csi = mci->csrows[row]; > + size = synps_edac_get_memsize(); > + > + for (j = 0; j < csi->nr_channels; j++) { > + dimm = csi->channels[j]->dimm; > + dimm->edac_mode = EDAC_FLAG_SECDED; > + dimm->mtype = synps_edac_get_mtype(priv->baseaddr); > + dimm->nr_pages = > + (size >> PAGE_SHIFT) / csi->nr_channels; > + dimm->grain = SYNPS_EDAC_ERR_GRAIN; > + dimm->dtype = synps_edac_get_dtype(priv->baseaddr); > + } > + } > + > + return 0; > +} > + > +/** > + * synps_edac_mc_init - Initialize driver instance > + * @mci: Pointer to the edac memory controller instance > + * @pdev: Pointer to the platform_device struct > + * > + * This routine performs initialization of the EDAC memory controller > + * instance and related driver-private data associated with the > + * memory controller the instance is bound to. > + * > + * Return: 0 if OK; otherwise, < 0 on error. > + */ > +static int synps_edac_mc_init(struct mem_ctl_info *mci, > + struct platform_device *pdev) > +{ > + int status; > + struct synps_edac_priv *priv; > + > + mci->pdev = &pdev->dev; > + priv = mci->pvt_info; > + platform_set_drvdata(pdev, mci); > + > + /* Initialize controller capabilities and configuration */ > + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2; > + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED; > + mci->scrub_cap = SCRUB_HW_SRC; > + mci->scrub_mode = SCRUB_NONE; > + > + mci->edac_cap = EDAC_FLAG_SECDED; > + mci->ctl_name = "synps_ddr_controller"; > + mci->dev_name = SYNPS_EDAC_MOD_STRING; > + mci->mod_name = SYNPS_EDAC_MOD_VER; > + mci->mod_ver = "1"; > + > + edac_op_state = EDAC_OPSTATE_POLL; > + mci->edac_check = synps_edac_check; > + mci->ctl_page_to_phys = NULL; > + > + /* > + * Initialize the MC control structure 'csrows' table > + * with the mapping and control information. > + */ > + status = synps_edac_init_csrows(mci); > + > + return status; > +} > + > +/** > + * synps_edac_mc_probe - Check controller and bind driver > + * @pdev: Pointer to the platform_device struct > + * > + * This routine probes a specific controller > + * instance for binding with the driver. > + * > + * Return: 0 if the controller instance was successfully bound to the > + * driver; otherwise, < 0 on error. > + */ > +static int synps_edac_mc_probe(struct platform_device *pdev) > +{ > + struct mem_ctl_info *mci; > + struct edac_mc_layer layers[2]; > + struct synps_edac_priv *priv; > + int rc; > + struct resource *res; > + void __iomem *baseaddr; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + baseaddr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(baseaddr)) > + return PTR_ERR(baseaddr); > + > + if (synps_edac_get_eccstate(baseaddr) == false) { > + edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n"); > + return -ENXIO; > + } > + > + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT; > + layers[0].size = SYNPS_EDAC_NR_CSROWS; > + layers[0].is_virt_csrow = true; > + layers[1].type = EDAC_MC_LAYER_CHANNEL; > + layers[1].size = SYNPS_EDAC_NR_CHANS; > + layers[1].is_virt_csrow = false; > + > + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, > + sizeof(struct synps_edac_priv)); > + if (!mci) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed memory allocation for mc instance\n"); > + return -ENOMEM; > + } > + > + priv = mci->pvt_info; > + priv->baseaddr = baseaddr; > + rc = synps_edac_mc_init(mci, pdev); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed to initialize instance\n"); > + goto free_edac_mc; > + } > + > + rc = edac_mc_add_mc(mci); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed to register with EDAC core\n"); > + goto del_edac_mc; > + } > + > + return rc; > + > +del_edac_mc: > + edac_mc_del_mc(&pdev->dev); > +free_edac_mc: > + edac_mc_free(mci); > + > + return rc; > +} > + > +/** > + * synps_edac_mc_remove - Unbind driver from controller > + * @pdev: Pointer to the platform_device struct > + * > + * Return: Unconditionally 0 > + */ > +static int synps_edac_mc_remove(struct platform_device *pdev) > +{ > + struct mem_ctl_info *mci = platform_get_drvdata(pdev); > + > + edac_mc_del_mc(&pdev->dev); > + edac_mc_free(mci); > + > + return 0; > +} > + > +static struct of_device_id synps_edac_match[] = { > + { .compatible = "xlnx,zynq-ddrc-A05", }, here should be a05 > + { /* end of table */ } > +}; > + > +MODULE_DEVICE_TABLE(of, synps_edac_match); > + > +static struct platform_driver synps_edac_mc_driver = { > + .driver = { > + .name = "synopsys-edac", > + .owner = THIS_MODULE, This line is not needed. There were some patches to remove this from all drivers. > + .of_match_table = synps_edac_match, > + }, > + .probe = synps_edac_mc_probe, > + .remove = synps_edac_mc_remove, > +}; > + > +module_platform_driver(synps_edac_mc_driver); > + > +MODULE_AUTHOR("Xilinx Inc"); > +MODULE_DESCRIPTION("Synopsys DDR ECC driver"); > +MODULE_LICENSE("GPL v2"); Thanks, Michal From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Subject: Re: [PATCH v4] edac: synps: Added EDAC support for zynq ddr ecc controller Date: Thu, 27 Nov 2014 11:59:39 +0100 Message-ID: <2a4244f4a9594432837db1c82e701a04@BY2FFO11FD013.protection.gbl> References: <1416932548-25094-1-git-send-email-punnaia@xilinx.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <1416932548-25094-1-git-send-email-punnaia-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Punnaiah Choudary Kalluri , dougthompson-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-edac-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, rob-VoJi6FS/r0vR7s880joybQ@public.gmane.org, bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org Cc: kpc528-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, kalluripunnaiahchoudary-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, punnaia-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Punnaiah, On 11/25/2014 05:22 PM, Punnaiah Choudary Kalluri wrote: > Added EDAC support for reporting the ecc errors of synopsys ddr controller. > The ddr ecc controller corrects single bit errors and detects double bit > errors. > > Signed-off-by: Punnaiah Choudary Kalluri > --- > Changes for v4: > - Shorten the macro definitions > - Corrected the ddr ip version > - Reverted the file name change > Changes for v3: > - Updated maintainer information > - Driver cleanup as per the review comments > - Shortened the prefix "sysnopsys" to "synps" > Changes for v2: > - Updated the commit header and message > - Renamed the filenames to synopsys_edac > - Corrected the compatilble string, commnets > - Renamed the macros,fucntions and data structures > --- > .../devicetree/bindings/edac/synopsys_edac.txt | 18 + > MAINTAINERS | 1 + > drivers/edac/Kconfig | 7 + > drivers/edac/Makefile | 1 + > drivers/edac/synopsys_edac.c | 543 ++++++++++++++++++++ > 5 files changed, 570 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/edac/synopsys_edac.txt > create mode 100644 drivers/edac/synopsys_edac.c > > diff --git a/Documentation/devicetree/bindings/edac/synopsys_edac.txt b/Documentation/devicetree/bindings/edac/synopsys_edac.txt Binding is already here Documentation/devicetree/bindings/memory-controllers/synopsys.txt can you please just add there ECC information? > new file mode 100644 > index 0000000..a64497a > --- /dev/null > +++ b/Documentation/devicetree/bindings/edac/synopsys_edac.txt > @@ -0,0 +1,18 @@ > +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are > +corrected and double bit ecc errors that are detected by the DDR ECC controller. > +ECC support for DDR is available in half-bus width(16 bit) configuration only. > + > +Required properties: > +- compatible: Should be "xlnx,zynq-ddrc-A05" > +- reg: Should contain DDR controller registers location and length. > + > +Example: > +++++++++ > + > +ddrc0: ddrc@f8006000 { > + compatible = "xlnx,zynq-ddrc-A05"; here should be a05 not A05. > + reg = <0xf8006000 0x1000>; > +}; > + > +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate > +control register. > diff --git a/MAINTAINERS b/MAINTAINERS > index f10ed39..4a16f97 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1522,6 +1522,7 @@ N: xilinx > F: drivers/clocksource/cadence_ttc_timer.c > F: drivers/i2c/busses/i2c-cadence.c > F: drivers/mmc/host/sdhci-of-arasan.c > +F: drivers/edac/synopsys_edac.c > > ARM SMMU DRIVER > M: Will Deacon > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index 7072c28..c6d448e 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -385,4 +385,11 @@ config EDAC_ALTERA_MC > preloader must initialize the SDRAM before loading > the kernel. > > +config EDAC_SYNOPSYS > + tristate "Synopsys DDR Memory Controller" > + depends on EDAC_MM_EDAC && ARCH_ZYNQ > + help > + Support for EDAC on the ECC memory used with the Synopsys DDR > + memory controller. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile > index 359aa49..9b5a095 100644 > --- a/drivers/edac/Makefile > +++ b/drivers/edac/Makefile > @@ -67,3 +67,4 @@ obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o > obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o > > obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o > +obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o > diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c > new file mode 100644 > index 0000000..3522432 > --- /dev/null > +++ b/drivers/edac/synopsys_edac.c > @@ -0,0 +1,543 @@ > +/* > + * Synopsys DDR ECC Driver > + * This driver is based on ppc4xx_edac.c drivers > + * > + * Copyright (C) 2012 - 2014 Xilinx, Inc. > + * > + * 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. > + * > + * 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. > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details > + */ > + > +#include > +#include > +#include > + > +#include "edac_core.h" > + > +/* Number of cs_rows needed per memory controller */ > +#define SYNPS_EDAC_NR_CSROWS 1 > + > +/* Number of channels per memory controller */ > +#define SYNPS_EDAC_NR_CHANS 1 > + > +/* Granularity of reported error in bytes */ > +#define SYNPS_EDAC_ERR_GRAIN 1 > + > +#define SYNPS_EDAC_MSG_SIZE 256 > + > +#define SYNPS_EDAC_MOD_STRING "synps_edac" > +#define SYNPS_EDAC_MOD_VER "1" > + > +/* Synopsys DDR memory controller registers that are relevant to ECC */ > +#define CTRL_OFST 0x0 > +#define T_ZQ_OFST 0xA4 > + > +/* ECC control register */ > +#define ECC_CTRL_OFST 0xC4 > +/* ECC log register */ > +#define CE_LOG_OFST 0xC8 > +/* ECC address register */ > +#define CE_ADDR_OFST 0xCC > +/* ECC data[31:0] register */ > +#define CE_DATA_31_0_OFST 0xD0 > + > +/* Uncorrectable error info regsisters */ > +#define UE_LOG_OFST 0xDC > +#define UE_ADDR_OFST 0xE0 > +#define UE_DATA_31_0_OFST 0xE4 > + > +#define STAT_OFST 0xF0 > +#define SCRUB_OFST 0xF4 > + > +/* Control regsiter bitfield definitions */ > +#define CTRL_BW_MASK 0xC > +#define CTRL_BW_SHIFT 2 > + > +#define DDRCTL_WDTH_16 1 > +#define DDRCTL_WDTH_32 0 > + > +/* ZQ register bitfield definitions */ > +#define T_ZQ_DDRMODE_MASK 0x2 > + > +/* ECC control register bitfield definitions */ > +#define ECC_CTRL_CLR_CE_ERR 0x2 > +#define ECC_CTRL_CLR_UE_ERR 0x1 > + > +/* ECC correctable/uncorrectable error log register definitions */ > +#define CE_LOG_VALID 0x1 > +#define CE_LOG_BITPOS_MASK 0xFE > +#define CE_LOG_BITPOS_SHIFT 1 > + > +/* ECC correctable/uncorrectable error address register definitions */ > +#define ADDR_COL_MASK 0xFFF > +#define ADDR_ROW_MASK 0xFFFF000 > +#define ADDR_ROW_SHIFT 12 > +#define ADDR_BANK_MASK 0x70000000 > +#define ADDR_BANK_SHIFT 28 > + > +/* ECC statistic regsiter definitions */ > +#define STAT_UECNT_MASK 0xFF > +#define STAT_CECNT_MASK 0xFF00 > +#define STAT_CECNT_SHIFT 8 > + > +/* ECC scrub regsiter definitions */ > +#define SCRUB_MODE_MASK 0x7 > +#define SCRUB_MODE_SECDED 0x4 > + > +/** > + * struct ecc_error_info - ECC error log information > + * @row: Row number > + * @col: Column number > + * @bank: Bank number > + * @bitpos: Bit position > + * @data: Data causing the error > + */ > +struct ecc_error_info { > + u32 row; > + u32 col; > + u32 bank; > + u32 bitpos; > + u32 data; > +}; > + > +/** > + * struct synps_ecc_status - ECC status information to report > + * @ce_count: Correctable error count > + * @ue_count: Uncorrectable error count > + * @ceinfo: Correctable error log information > + * @ueinfo: Uncorrectable error log information > + */ > +struct synps_ecc_status { > + u32 ce_cnt; > + u32 ue_cnt; > + struct ecc_error_info ceinfo; > + struct ecc_error_info ueinfo; > +}; > + > +/** > + * struct synps_edac_priv - DDR memory controller private instance data > + * @baseaddr: Base address of the DDR controller > + * @ce_count: Correctable Error count > + * @ue_count: Uncorrectable Error count > + */ > +struct synps_edac_priv { > + void __iomem *baseaddr; > + u32 ce_cnt; > + u32 ue_cnt; > +}; > + > +/** > + * synps_edac_geterror_info - Get the current ecc error info > + * @base: Pointer to the base address of the ddr memory controller > + * @perrstatus: Pointer to the synopsys ecc status structure > + * > + * This routine determines there is any ecc error or not > + * > + * Return: zero if there is no error otherwise returns 1 > + */ > +static int synps_edac_geterror_info(void __iomem *base, > + struct synps_ecc_status *p) > +{ This is driver internal function. I think that will be just better to return 0 on success and different values otherwise. It should be easy to change it. > + u32 regval; > + u32 clearval = 0; > + > + regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK); > + > + if (!regval) > + return 0; > + > + memset(p, 0, sizeof(*p)); > + > + p->ce_cnt = (regval & STAT_CECNT_MASK) >> STAT_CECNT_SHIFT; > + p->ue_cnt = regval & STAT_UECNT_MASK; > + > + regval = readl(base + CE_LOG_OFST); > + if (!(p->ce_cnt && (regval & CE_LOG_VALID))) > + goto ue_err; > + > + p->ceinfo.bitpos = (regval & CE_LOG_BITPOS_MASK) >> CE_LOG_BITPOS_SHIFT; > + regval = readl(base + CE_ADDR_OFST); > + p->ceinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT; > + p->ceinfo.col = regval & ADDR_COL_MASK; > + p->ceinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT; > + p->ceinfo.data = readl(base + CE_DATA_31_0_OFST); > + edac_dbg(3, "ce bitposition: %d data: %d\n", p->ceinfo.bitpos, > + p->ceinfo.data); > + clearval = ECC_CTRL_CLR_CE_ERR; > + > +ue_err: > + regval = readl(base + UE_LOG_OFST); > + if (!(p->ue_cnt && (regval & CE_LOG_VALID))) > + goto out; > + > + regval = readl(base + UE_ADDR_OFST); > + p->ueinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT; > + p->ueinfo.col = regval & ADDR_COL_MASK; > + p->ueinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT; > + p->ueinfo.data = readl(base + UE_DATA_31_0_OFST); > + clearval |= ECC_CTRL_CLR_UE_ERR; > + > +out: > + writel(clearval, base + ECC_CTRL_OFST); > + writel(0x0, base + ECC_CTRL_OFST); > + > + return 1; > +} > + > +/** > + * synps_edac_handle_error - Handle controller error types CE and UE > + * @mci: Pointer to the edac memory controller instance > + * @perrstatus: Pointer to the synopsys ecc status structure > + * > + * This routine handles the controller ECC correctable and un correctable > + * error. > + */ > +static void synps_edac_handle_error(struct mem_ctl_info *mci, > + struct synps_ecc_status *p) > +{ > + char message[SYNPS_EDAC_MSG_SIZE]; > + struct ecc_error_info *pinf; > + > + if (p->ce_cnt) { > + pinf = &p->ceinfo; > + snprintf(message, SYNPS_EDAC_MSG_SIZE, > + "DDR ECC error type :%s Row %d Bank %d Col %d ", > + "CE", pinf->row, pinf->bank, pinf->col); > + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, > + p->ce_cnt, 0, 0, 0, 0, 0, -1, > + message, ""); > + } > + > + if (p->ue_cnt) { > + pinf = &p->ueinfo; > + snprintf(message, SYNPS_EDAC_MSG_SIZE, > + "DDR ECC error type :%s Row %d Bank %d Col %d ", > + "UE", pinf->row, pinf->bank, pinf->col); > + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, > + p->ue_cnt, 0, 0, 0, 0, 0, -1, > + message, ""); > + } > +} > + > +/** > + * synps_edac_check - Check controller for ECC errors > + * @mci: Pointer to the edac memory controller instance > + * > + * This routine is used to check and post ECC errors and is called by > + * the EDAC polling thread > + */ > +static void synps_edac_check(struct mem_ctl_info *mci) > +{ > + struct synps_edac_priv *priv = mci->pvt_info; > + struct synps_ecc_status stat; > + int status; > + > + status = synps_edac_geterror_info(priv->baseaddr, &stat); > + if (!status) > + return; > + > + priv->ce_cnt += stat.ce_cnt; > + priv->ue_cnt += stat.ue_cnt; > + synps_edac_handle_error(mci, &stat); > + > + edac_dbg(3, "Total error count ce %d ue %d\n", > + priv->ce_cnt, priv->ue_cnt); > +} > + > +/** > + * synps_edac_get_dtype - Return the controller memory width > + * @base: Pointer to the ddr memory contoller base address > + * > + * This routine returns the EDAC device type width appropriate for the > + * current controller configuration. > + * > + * Return: a device type width enumeration. > + */ > +static enum dev_type synps_edac_get_dtype(const void __iomem *base) > +{ > + enum dev_type dt; > + u32 width; > + > + width = readl(base + CTRL_OFST); > + width = (width & CTRL_BW_MASK) >> CTRL_BW_SHIFT; > + > + switch (width) { > + case DDRCTL_WDTH_16: > + dt = DEV_X2; > + break; > + case DDRCTL_WDTH_32: > + dt = DEV_X4; > + break; > + default: > + dt = DEV_UNKNOWN; > + } > + > + return dt; > +} > + > +/** > + * synps_edac_get_eccstate - Return the controller ecc enable/disable status > + * @base: Pointer to the ddr memory contoller base address > + * > + * This routine returns the ECC enable/diable status for the controller > + * > + * Return: a ecc status boolean i.e true/false - enabled/disabled. > + */ > +static bool synps_edac_get_eccstate(void __iomem *base) > +{ > + enum dev_type dt; > + u32 ecctype; > + bool state = false; > + > + dt = synps_edac_get_dtype(base); > + if (dt == DEV_UNKNOWN) > + return state; > + > + ecctype = readl(base + SCRUB_OFST) & SCRUB_MODE_MASK; > + > + if ((ecctype == SCRUB_MODE_SECDED) && (dt == DEV_X2)) { > + state = true; > + writel(0x0, base + ECC_CTRL_OFST); > + } > + > + return state; > +} > + > +/** > + * synps_edac_get_memsize - reads the size of the attached memory device > + * > + * Return: the memory size in bytes > + */ > +static u32 synps_edac_get_memsize(void) > +{ > + struct sysinfo inf; > + > + si_meminfo(&inf); > + > + return inf.totalram * inf.mem_unit; > +} > + > +/** > + * synps_edac_get_mtype - Returns controller memory type > + * @base: pointer to the synopsys ecc status structure > + * > + * This routine returns the EDAC memory type appropriate for the > + * current controller configuration. > + * > + * Return: a memory type enumeration. > + */ > +static enum mem_type synps_edac_get_mtype(const void __iomem *base) > +{ > + enum mem_type mt; > + u32 memtype; > + > + memtype = readl(base + T_ZQ_OFST); > + > + if (memtype & T_ZQ_DDRMODE_MASK) > + mt = MEM_DDR3; > + else > + mt = MEM_DDR2; > + > + return mt; > +} > + > +/** > + * synps_edac_init_csrows - Initialize the cs row data > + * @mci: Pointer to the edac memory controller instance > + * > + * This routine initializes the chip select rows associated > + * with the EDAC memory controller instance > + * > + * Return: Unconditionally 0. > + */ > +static int synps_edac_init_csrows(struct mem_ctl_info *mci) > +{ > + struct csrow_info *csi; > + struct dimm_info *dimm; > + struct synps_edac_priv *priv = mci->pvt_info; > + u32 size; > + int row, j; > + > + for (row = 0; row < mci->nr_csrows; row++) { > + csi = mci->csrows[row]; > + size = synps_edac_get_memsize(); > + > + for (j = 0; j < csi->nr_channels; j++) { > + dimm = csi->channels[j]->dimm; > + dimm->edac_mode = EDAC_FLAG_SECDED; > + dimm->mtype = synps_edac_get_mtype(priv->baseaddr); > + dimm->nr_pages = > + (size >> PAGE_SHIFT) / csi->nr_channels; > + dimm->grain = SYNPS_EDAC_ERR_GRAIN; > + dimm->dtype = synps_edac_get_dtype(priv->baseaddr); > + } > + } > + > + return 0; > +} > + > +/** > + * synps_edac_mc_init - Initialize driver instance > + * @mci: Pointer to the edac memory controller instance > + * @pdev: Pointer to the platform_device struct > + * > + * This routine performs initialization of the EDAC memory controller > + * instance and related driver-private data associated with the > + * memory controller the instance is bound to. > + * > + * Return: 0 if OK; otherwise, < 0 on error. > + */ > +static int synps_edac_mc_init(struct mem_ctl_info *mci, > + struct platform_device *pdev) > +{ > + int status; > + struct synps_edac_priv *priv; > + > + mci->pdev = &pdev->dev; > + priv = mci->pvt_info; > + platform_set_drvdata(pdev, mci); > + > + /* Initialize controller capabilities and configuration */ > + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2; > + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED; > + mci->scrub_cap = SCRUB_HW_SRC; > + mci->scrub_mode = SCRUB_NONE; > + > + mci->edac_cap = EDAC_FLAG_SECDED; > + mci->ctl_name = "synps_ddr_controller"; > + mci->dev_name = SYNPS_EDAC_MOD_STRING; > + mci->mod_name = SYNPS_EDAC_MOD_VER; > + mci->mod_ver = "1"; > + > + edac_op_state = EDAC_OPSTATE_POLL; > + mci->edac_check = synps_edac_check; > + mci->ctl_page_to_phys = NULL; > + > + /* > + * Initialize the MC control structure 'csrows' table > + * with the mapping and control information. > + */ > + status = synps_edac_init_csrows(mci); > + > + return status; > +} > + > +/** > + * synps_edac_mc_probe - Check controller and bind driver > + * @pdev: Pointer to the platform_device struct > + * > + * This routine probes a specific controller > + * instance for binding with the driver. > + * > + * Return: 0 if the controller instance was successfully bound to the > + * driver; otherwise, < 0 on error. > + */ > +static int synps_edac_mc_probe(struct platform_device *pdev) > +{ > + struct mem_ctl_info *mci; > + struct edac_mc_layer layers[2]; > + struct synps_edac_priv *priv; > + int rc; > + struct resource *res; > + void __iomem *baseaddr; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + baseaddr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(baseaddr)) > + return PTR_ERR(baseaddr); > + > + if (synps_edac_get_eccstate(baseaddr) == false) { > + edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n"); > + return -ENXIO; > + } > + > + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT; > + layers[0].size = SYNPS_EDAC_NR_CSROWS; > + layers[0].is_virt_csrow = true; > + layers[1].type = EDAC_MC_LAYER_CHANNEL; > + layers[1].size = SYNPS_EDAC_NR_CHANS; > + layers[1].is_virt_csrow = false; > + > + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, > + sizeof(struct synps_edac_priv)); > + if (!mci) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed memory allocation for mc instance\n"); > + return -ENOMEM; > + } > + > + priv = mci->pvt_info; > + priv->baseaddr = baseaddr; > + rc = synps_edac_mc_init(mci, pdev); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed to initialize instance\n"); > + goto free_edac_mc; > + } > + > + rc = edac_mc_add_mc(mci); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed to register with EDAC core\n"); > + goto del_edac_mc; > + } > + > + return rc; > + > +del_edac_mc: > + edac_mc_del_mc(&pdev->dev); > +free_edac_mc: > + edac_mc_free(mci); > + > + return rc; > +} > + > +/** > + * synps_edac_mc_remove - Unbind driver from controller > + * @pdev: Pointer to the platform_device struct > + * > + * Return: Unconditionally 0 > + */ > +static int synps_edac_mc_remove(struct platform_device *pdev) > +{ > + struct mem_ctl_info *mci = platform_get_drvdata(pdev); > + > + edac_mc_del_mc(&pdev->dev); > + edac_mc_free(mci); > + > + return 0; > +} > + > +static struct of_device_id synps_edac_match[] = { > + { .compatible = "xlnx,zynq-ddrc-A05", }, here should be a05 > + { /* end of table */ } > +}; > + > +MODULE_DEVICE_TABLE(of, synps_edac_match); > + > +static struct platform_driver synps_edac_mc_driver = { > + .driver = { > + .name = "synopsys-edac", > + .owner = THIS_MODULE, This line is not needed. There were some patches to remove this from all drivers. > + .of_match_table = synps_edac_match, > + }, > + .probe = synps_edac_mc_probe, > + .remove = synps_edac_mc_remove, > +}; > + > +module_platform_driver(synps_edac_mc_driver); > + > +MODULE_AUTHOR("Xilinx Inc"); > +MODULE_DESCRIPTION("Synopsys DDR ECC driver"); > +MODULE_LICENSE("GPL v2"); Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: michal.simek@xilinx.com (Michal Simek) Date: Thu, 27 Nov 2014 11:59:39 +0100 Subject: [PATCH v4] edac: synps: Added EDAC support for zynq ddr ecc controller In-Reply-To: <1416932548-25094-1-git-send-email-punnaia@xilinx.com> References: <1416932548-25094-1-git-send-email-punnaia@xilinx.com> Message-ID: <2a4244f4a9594432837db1c82e701a04@BY2FFO11FD013.protection.gbl> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Punnaiah, On 11/25/2014 05:22 PM, Punnaiah Choudary Kalluri wrote: > Added EDAC support for reporting the ecc errors of synopsys ddr controller. > The ddr ecc controller corrects single bit errors and detects double bit > errors. > > Signed-off-by: Punnaiah Choudary Kalluri > --- > Changes for v4: > - Shorten the macro definitions > - Corrected the ddr ip version > - Reverted the file name change > Changes for v3: > - Updated maintainer information > - Driver cleanup as per the review comments > - Shortened the prefix "sysnopsys" to "synps" > Changes for v2: > - Updated the commit header and message > - Renamed the filenames to synopsys_edac > - Corrected the compatilble string, commnets > - Renamed the macros,fucntions and data structures > --- > .../devicetree/bindings/edac/synopsys_edac.txt | 18 + > MAINTAINERS | 1 + > drivers/edac/Kconfig | 7 + > drivers/edac/Makefile | 1 + > drivers/edac/synopsys_edac.c | 543 ++++++++++++++++++++ > 5 files changed, 570 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/edac/synopsys_edac.txt > create mode 100644 drivers/edac/synopsys_edac.c > > diff --git a/Documentation/devicetree/bindings/edac/synopsys_edac.txt b/Documentation/devicetree/bindings/edac/synopsys_edac.txt Binding is already here Documentation/devicetree/bindings/memory-controllers/synopsys.txt can you please just add there ECC information? > new file mode 100644 > index 0000000..a64497a > --- /dev/null > +++ b/Documentation/devicetree/bindings/edac/synopsys_edac.txt > @@ -0,0 +1,18 @@ > +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are > +corrected and double bit ecc errors that are detected by the DDR ECC controller. > +ECC support for DDR is available in half-bus width(16 bit) configuration only. > + > +Required properties: > +- compatible: Should be "xlnx,zynq-ddrc-A05" > +- reg: Should contain DDR controller registers location and length. > + > +Example: > +++++++++ > + > +ddrc0: ddrc at f8006000 { > + compatible = "xlnx,zynq-ddrc-A05"; here should be a05 not A05. > + reg = <0xf8006000 0x1000>; > +}; > + > +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate > +control register. > diff --git a/MAINTAINERS b/MAINTAINERS > index f10ed39..4a16f97 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1522,6 +1522,7 @@ N: xilinx > F: drivers/clocksource/cadence_ttc_timer.c > F: drivers/i2c/busses/i2c-cadence.c > F: drivers/mmc/host/sdhci-of-arasan.c > +F: drivers/edac/synopsys_edac.c > > ARM SMMU DRIVER > M: Will Deacon > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index 7072c28..c6d448e 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -385,4 +385,11 @@ config EDAC_ALTERA_MC > preloader must initialize the SDRAM before loading > the kernel. > > +config EDAC_SYNOPSYS > + tristate "Synopsys DDR Memory Controller" > + depends on EDAC_MM_EDAC && ARCH_ZYNQ > + help > + Support for EDAC on the ECC memory used with the Synopsys DDR > + memory controller. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile > index 359aa49..9b5a095 100644 > --- a/drivers/edac/Makefile > +++ b/drivers/edac/Makefile > @@ -67,3 +67,4 @@ obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o > obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o > > obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o > +obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o > diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c > new file mode 100644 > index 0000000..3522432 > --- /dev/null > +++ b/drivers/edac/synopsys_edac.c > @@ -0,0 +1,543 @@ > +/* > + * Synopsys DDR ECC Driver > + * This driver is based on ppc4xx_edac.c drivers > + * > + * Copyright (C) 2012 - 2014 Xilinx, Inc. > + * > + * 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. > + * > + * 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. > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details > + */ > + > +#include > +#include > +#include > + > +#include "edac_core.h" > + > +/* Number of cs_rows needed per memory controller */ > +#define SYNPS_EDAC_NR_CSROWS 1 > + > +/* Number of channels per memory controller */ > +#define SYNPS_EDAC_NR_CHANS 1 > + > +/* Granularity of reported error in bytes */ > +#define SYNPS_EDAC_ERR_GRAIN 1 > + > +#define SYNPS_EDAC_MSG_SIZE 256 > + > +#define SYNPS_EDAC_MOD_STRING "synps_edac" > +#define SYNPS_EDAC_MOD_VER "1" > + > +/* Synopsys DDR memory controller registers that are relevant to ECC */ > +#define CTRL_OFST 0x0 > +#define T_ZQ_OFST 0xA4 > + > +/* ECC control register */ > +#define ECC_CTRL_OFST 0xC4 > +/* ECC log register */ > +#define CE_LOG_OFST 0xC8 > +/* ECC address register */ > +#define CE_ADDR_OFST 0xCC > +/* ECC data[31:0] register */ > +#define CE_DATA_31_0_OFST 0xD0 > + > +/* Uncorrectable error info regsisters */ > +#define UE_LOG_OFST 0xDC > +#define UE_ADDR_OFST 0xE0 > +#define UE_DATA_31_0_OFST 0xE4 > + > +#define STAT_OFST 0xF0 > +#define SCRUB_OFST 0xF4 > + > +/* Control regsiter bitfield definitions */ > +#define CTRL_BW_MASK 0xC > +#define CTRL_BW_SHIFT 2 > + > +#define DDRCTL_WDTH_16 1 > +#define DDRCTL_WDTH_32 0 > + > +/* ZQ register bitfield definitions */ > +#define T_ZQ_DDRMODE_MASK 0x2 > + > +/* ECC control register bitfield definitions */ > +#define ECC_CTRL_CLR_CE_ERR 0x2 > +#define ECC_CTRL_CLR_UE_ERR 0x1 > + > +/* ECC correctable/uncorrectable error log register definitions */ > +#define CE_LOG_VALID 0x1 > +#define CE_LOG_BITPOS_MASK 0xFE > +#define CE_LOG_BITPOS_SHIFT 1 > + > +/* ECC correctable/uncorrectable error address register definitions */ > +#define ADDR_COL_MASK 0xFFF > +#define ADDR_ROW_MASK 0xFFFF000 > +#define ADDR_ROW_SHIFT 12 > +#define ADDR_BANK_MASK 0x70000000 > +#define ADDR_BANK_SHIFT 28 > + > +/* ECC statistic regsiter definitions */ > +#define STAT_UECNT_MASK 0xFF > +#define STAT_CECNT_MASK 0xFF00 > +#define STAT_CECNT_SHIFT 8 > + > +/* ECC scrub regsiter definitions */ > +#define SCRUB_MODE_MASK 0x7 > +#define SCRUB_MODE_SECDED 0x4 > + > +/** > + * struct ecc_error_info - ECC error log information > + * @row: Row number > + * @col: Column number > + * @bank: Bank number > + * @bitpos: Bit position > + * @data: Data causing the error > + */ > +struct ecc_error_info { > + u32 row; > + u32 col; > + u32 bank; > + u32 bitpos; > + u32 data; > +}; > + > +/** > + * struct synps_ecc_status - ECC status information to report > + * @ce_count: Correctable error count > + * @ue_count: Uncorrectable error count > + * @ceinfo: Correctable error log information > + * @ueinfo: Uncorrectable error log information > + */ > +struct synps_ecc_status { > + u32 ce_cnt; > + u32 ue_cnt; > + struct ecc_error_info ceinfo; > + struct ecc_error_info ueinfo; > +}; > + > +/** > + * struct synps_edac_priv - DDR memory controller private instance data > + * @baseaddr: Base address of the DDR controller > + * @ce_count: Correctable Error count > + * @ue_count: Uncorrectable Error count > + */ > +struct synps_edac_priv { > + void __iomem *baseaddr; > + u32 ce_cnt; > + u32 ue_cnt; > +}; > + > +/** > + * synps_edac_geterror_info - Get the current ecc error info > + * @base: Pointer to the base address of the ddr memory controller > + * @perrstatus: Pointer to the synopsys ecc status structure > + * > + * This routine determines there is any ecc error or not > + * > + * Return: zero if there is no error otherwise returns 1 > + */ > +static int synps_edac_geterror_info(void __iomem *base, > + struct synps_ecc_status *p) > +{ This is driver internal function. I think that will be just better to return 0 on success and different values otherwise. It should be easy to change it. > + u32 regval; > + u32 clearval = 0; > + > + regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK); > + > + if (!regval) > + return 0; > + > + memset(p, 0, sizeof(*p)); > + > + p->ce_cnt = (regval & STAT_CECNT_MASK) >> STAT_CECNT_SHIFT; > + p->ue_cnt = regval & STAT_UECNT_MASK; > + > + regval = readl(base + CE_LOG_OFST); > + if (!(p->ce_cnt && (regval & CE_LOG_VALID))) > + goto ue_err; > + > + p->ceinfo.bitpos = (regval & CE_LOG_BITPOS_MASK) >> CE_LOG_BITPOS_SHIFT; > + regval = readl(base + CE_ADDR_OFST); > + p->ceinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT; > + p->ceinfo.col = regval & ADDR_COL_MASK; > + p->ceinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT; > + p->ceinfo.data = readl(base + CE_DATA_31_0_OFST); > + edac_dbg(3, "ce bitposition: %d data: %d\n", p->ceinfo.bitpos, > + p->ceinfo.data); > + clearval = ECC_CTRL_CLR_CE_ERR; > + > +ue_err: > + regval = readl(base + UE_LOG_OFST); > + if (!(p->ue_cnt && (regval & CE_LOG_VALID))) > + goto out; > + > + regval = readl(base + UE_ADDR_OFST); > + p->ueinfo.row = (regval & ADDR_ROW_MASK) >> ADDR_ROW_SHIFT; > + p->ueinfo.col = regval & ADDR_COL_MASK; > + p->ueinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT; > + p->ueinfo.data = readl(base + UE_DATA_31_0_OFST); > + clearval |= ECC_CTRL_CLR_UE_ERR; > + > +out: > + writel(clearval, base + ECC_CTRL_OFST); > + writel(0x0, base + ECC_CTRL_OFST); > + > + return 1; > +} > + > +/** > + * synps_edac_handle_error - Handle controller error types CE and UE > + * @mci: Pointer to the edac memory controller instance > + * @perrstatus: Pointer to the synopsys ecc status structure > + * > + * This routine handles the controller ECC correctable and un correctable > + * error. > + */ > +static void synps_edac_handle_error(struct mem_ctl_info *mci, > + struct synps_ecc_status *p) > +{ > + char message[SYNPS_EDAC_MSG_SIZE]; > + struct ecc_error_info *pinf; > + > + if (p->ce_cnt) { > + pinf = &p->ceinfo; > + snprintf(message, SYNPS_EDAC_MSG_SIZE, > + "DDR ECC error type :%s Row %d Bank %d Col %d ", > + "CE", pinf->row, pinf->bank, pinf->col); > + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, > + p->ce_cnt, 0, 0, 0, 0, 0, -1, > + message, ""); > + } > + > + if (p->ue_cnt) { > + pinf = &p->ueinfo; > + snprintf(message, SYNPS_EDAC_MSG_SIZE, > + "DDR ECC error type :%s Row %d Bank %d Col %d ", > + "UE", pinf->row, pinf->bank, pinf->col); > + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, > + p->ue_cnt, 0, 0, 0, 0, 0, -1, > + message, ""); > + } > +} > + > +/** > + * synps_edac_check - Check controller for ECC errors > + * @mci: Pointer to the edac memory controller instance > + * > + * This routine is used to check and post ECC errors and is called by > + * the EDAC polling thread > + */ > +static void synps_edac_check(struct mem_ctl_info *mci) > +{ > + struct synps_edac_priv *priv = mci->pvt_info; > + struct synps_ecc_status stat; > + int status; > + > + status = synps_edac_geterror_info(priv->baseaddr, &stat); > + if (!status) > + return; > + > + priv->ce_cnt += stat.ce_cnt; > + priv->ue_cnt += stat.ue_cnt; > + synps_edac_handle_error(mci, &stat); > + > + edac_dbg(3, "Total error count ce %d ue %d\n", > + priv->ce_cnt, priv->ue_cnt); > +} > + > +/** > + * synps_edac_get_dtype - Return the controller memory width > + * @base: Pointer to the ddr memory contoller base address > + * > + * This routine returns the EDAC device type width appropriate for the > + * current controller configuration. > + * > + * Return: a device type width enumeration. > + */ > +static enum dev_type synps_edac_get_dtype(const void __iomem *base) > +{ > + enum dev_type dt; > + u32 width; > + > + width = readl(base + CTRL_OFST); > + width = (width & CTRL_BW_MASK) >> CTRL_BW_SHIFT; > + > + switch (width) { > + case DDRCTL_WDTH_16: > + dt = DEV_X2; > + break; > + case DDRCTL_WDTH_32: > + dt = DEV_X4; > + break; > + default: > + dt = DEV_UNKNOWN; > + } > + > + return dt; > +} > + > +/** > + * synps_edac_get_eccstate - Return the controller ecc enable/disable status > + * @base: Pointer to the ddr memory contoller base address > + * > + * This routine returns the ECC enable/diable status for the controller > + * > + * Return: a ecc status boolean i.e true/false - enabled/disabled. > + */ > +static bool synps_edac_get_eccstate(void __iomem *base) > +{ > + enum dev_type dt; > + u32 ecctype; > + bool state = false; > + > + dt = synps_edac_get_dtype(base); > + if (dt == DEV_UNKNOWN) > + return state; > + > + ecctype = readl(base + SCRUB_OFST) & SCRUB_MODE_MASK; > + > + if ((ecctype == SCRUB_MODE_SECDED) && (dt == DEV_X2)) { > + state = true; > + writel(0x0, base + ECC_CTRL_OFST); > + } > + > + return state; > +} > + > +/** > + * synps_edac_get_memsize - reads the size of the attached memory device > + * > + * Return: the memory size in bytes > + */ > +static u32 synps_edac_get_memsize(void) > +{ > + struct sysinfo inf; > + > + si_meminfo(&inf); > + > + return inf.totalram * inf.mem_unit; > +} > + > +/** > + * synps_edac_get_mtype - Returns controller memory type > + * @base: pointer to the synopsys ecc status structure > + * > + * This routine returns the EDAC memory type appropriate for the > + * current controller configuration. > + * > + * Return: a memory type enumeration. > + */ > +static enum mem_type synps_edac_get_mtype(const void __iomem *base) > +{ > + enum mem_type mt; > + u32 memtype; > + > + memtype = readl(base + T_ZQ_OFST); > + > + if (memtype & T_ZQ_DDRMODE_MASK) > + mt = MEM_DDR3; > + else > + mt = MEM_DDR2; > + > + return mt; > +} > + > +/** > + * synps_edac_init_csrows - Initialize the cs row data > + * @mci: Pointer to the edac memory controller instance > + * > + * This routine initializes the chip select rows associated > + * with the EDAC memory controller instance > + * > + * Return: Unconditionally 0. > + */ > +static int synps_edac_init_csrows(struct mem_ctl_info *mci) > +{ > + struct csrow_info *csi; > + struct dimm_info *dimm; > + struct synps_edac_priv *priv = mci->pvt_info; > + u32 size; > + int row, j; > + > + for (row = 0; row < mci->nr_csrows; row++) { > + csi = mci->csrows[row]; > + size = synps_edac_get_memsize(); > + > + for (j = 0; j < csi->nr_channels; j++) { > + dimm = csi->channels[j]->dimm; > + dimm->edac_mode = EDAC_FLAG_SECDED; > + dimm->mtype = synps_edac_get_mtype(priv->baseaddr); > + dimm->nr_pages = > + (size >> PAGE_SHIFT) / csi->nr_channels; > + dimm->grain = SYNPS_EDAC_ERR_GRAIN; > + dimm->dtype = synps_edac_get_dtype(priv->baseaddr); > + } > + } > + > + return 0; > +} > + > +/** > + * synps_edac_mc_init - Initialize driver instance > + * @mci: Pointer to the edac memory controller instance > + * @pdev: Pointer to the platform_device struct > + * > + * This routine performs initialization of the EDAC memory controller > + * instance and related driver-private data associated with the > + * memory controller the instance is bound to. > + * > + * Return: 0 if OK; otherwise, < 0 on error. > + */ > +static int synps_edac_mc_init(struct mem_ctl_info *mci, > + struct platform_device *pdev) > +{ > + int status; > + struct synps_edac_priv *priv; > + > + mci->pdev = &pdev->dev; > + priv = mci->pvt_info; > + platform_set_drvdata(pdev, mci); > + > + /* Initialize controller capabilities and configuration */ > + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2; > + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED; > + mci->scrub_cap = SCRUB_HW_SRC; > + mci->scrub_mode = SCRUB_NONE; > + > + mci->edac_cap = EDAC_FLAG_SECDED; > + mci->ctl_name = "synps_ddr_controller"; > + mci->dev_name = SYNPS_EDAC_MOD_STRING; > + mci->mod_name = SYNPS_EDAC_MOD_VER; > + mci->mod_ver = "1"; > + > + edac_op_state = EDAC_OPSTATE_POLL; > + mci->edac_check = synps_edac_check; > + mci->ctl_page_to_phys = NULL; > + > + /* > + * Initialize the MC control structure 'csrows' table > + * with the mapping and control information. > + */ > + status = synps_edac_init_csrows(mci); > + > + return status; > +} > + > +/** > + * synps_edac_mc_probe - Check controller and bind driver > + * @pdev: Pointer to the platform_device struct > + * > + * This routine probes a specific controller > + * instance for binding with the driver. > + * > + * Return: 0 if the controller instance was successfully bound to the > + * driver; otherwise, < 0 on error. > + */ > +static int synps_edac_mc_probe(struct platform_device *pdev) > +{ > + struct mem_ctl_info *mci; > + struct edac_mc_layer layers[2]; > + struct synps_edac_priv *priv; > + int rc; > + struct resource *res; > + void __iomem *baseaddr; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + baseaddr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(baseaddr)) > + return PTR_ERR(baseaddr); > + > + if (synps_edac_get_eccstate(baseaddr) == false) { > + edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n"); > + return -ENXIO; > + } > + > + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT; > + layers[0].size = SYNPS_EDAC_NR_CSROWS; > + layers[0].is_virt_csrow = true; > + layers[1].type = EDAC_MC_LAYER_CHANNEL; > + layers[1].size = SYNPS_EDAC_NR_CHANS; > + layers[1].is_virt_csrow = false; > + > + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, > + sizeof(struct synps_edac_priv)); > + if (!mci) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed memory allocation for mc instance\n"); > + return -ENOMEM; > + } > + > + priv = mci->pvt_info; > + priv->baseaddr = baseaddr; > + rc = synps_edac_mc_init(mci, pdev); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed to initialize instance\n"); > + goto free_edac_mc; > + } > + > + rc = edac_mc_add_mc(mci); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MC, > + "Failed to register with EDAC core\n"); > + goto del_edac_mc; > + } > + > + return rc; > + > +del_edac_mc: > + edac_mc_del_mc(&pdev->dev); > +free_edac_mc: > + edac_mc_free(mci); > + > + return rc; > +} > + > +/** > + * synps_edac_mc_remove - Unbind driver from controller > + * @pdev: Pointer to the platform_device struct > + * > + * Return: Unconditionally 0 > + */ > +static int synps_edac_mc_remove(struct platform_device *pdev) > +{ > + struct mem_ctl_info *mci = platform_get_drvdata(pdev); > + > + edac_mc_del_mc(&pdev->dev); > + edac_mc_free(mci); > + > + return 0; > +} > + > +static struct of_device_id synps_edac_match[] = { > + { .compatible = "xlnx,zynq-ddrc-A05", }, here should be a05 > + { /* end of table */ } > +}; > + > +MODULE_DEVICE_TABLE(of, synps_edac_match); > + > +static struct platform_driver synps_edac_mc_driver = { > + .driver = { > + .name = "synopsys-edac", > + .owner = THIS_MODULE, This line is not needed. There were some patches to remove this from all drivers. > + .of_match_table = synps_edac_match, > + }, > + .probe = synps_edac_mc_probe, > + .remove = synps_edac_mc_remove, > +}; > + > +module_platform_driver(synps_edac_mc_driver); > + > +MODULE_AUTHOR("Xilinx Inc"); > +MODULE_DESCRIPTION("Synopsys DDR ECC driver"); > +MODULE_LICENSE("GPL v2"); Thanks, Michal