2015-05-13 22:04:39

by Thor Thayer

[permalink] [raw]
Subject: [PATCH 0/4] Add Altera Arria10 EDAC Support

From: Thor Thayer <[email protected]>

This series of patches adds support for the Arria10 EDAC. The
SDRAM controller and ECC registers are significantly different
from the CycloneV/ArriaV but common areas could be abstracted.

Thor Thayer (4):
edac, altera: Generalize driver to use DT Memory size
edac, altera: Refactor EDAC for Altera CycloneV SoC.
edac, altera: Addition of Arria10 EDAC
dts, altera: Arria10 SDRAM EDAC DTS additions.

.../bindings/arm/altera/socfpga-sdram-edac.txt | 2 +-
arch/arm/boot/dts/socfpga_arria10.dtsi | 11 +
drivers/edac/altera_edac.c | 357 ++++++++++++--------
drivers/edac/altera_edac.h | 201 +++++++++++
4 files changed, 434 insertions(+), 137 deletions(-)
create mode 100644 drivers/edac/altera_edac.h

--
1.7.9.5


2015-05-13 22:04:56

by Thor Thayer

[permalink] [raw]
Subject: [PATCH 1/4] edac, altera: Generalize driver to use DT Memory size

From: Thor Thayer <[email protected]>

The Arria10 SOC uses a completely different SDRAM controller from the
earlier CycloneV and ArriaV SoCs. The memory size is calculated in
the bootloader and passed via the device tree. Using this device
tree size is more generic than using the register fields to
calculate the memory size for different SDRAM controllers.

Signed-off-by: Thor Thayer <[email protected]>
---
drivers/edac/altera_edac.c | 53 ++++++++++++++++++++++----------------------
1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 3c4929f..e18a205 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -219,36 +219,35 @@ static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
{}
#endif

-/* Get total memory size in bytes */
-static u32 altr_sdram_get_total_mem_size(struct regmap *mc_vbase)
+/* Get total memory size from Open Firmware DTB */
+static unsigned long get_total_mem(void)
{
- u32 size, read_reg, row, bank, col, cs, width;
+ struct device_node *np = NULL;
+ const unsigned int *reg, *reg_end;
+ int len, sw, aw;
+ unsigned long start, size, total_mem;

- if (regmap_read(mc_vbase, DRAMADDRW_OFST, &read_reg) < 0)
+ np = of_find_node_by_type(NULL, "memory");
+ if (!np)
return 0;

- if (regmap_read(mc_vbase, DRAMIFWIDTH_OFST, &width) < 0)
- return 0;
-
- col = (read_reg & DRAMADDRW_COLBIT_MASK) >>
- DRAMADDRW_COLBIT_SHIFT;
- row = (read_reg & DRAMADDRW_ROWBIT_MASK) >>
- DRAMADDRW_ROWBIT_SHIFT;
- bank = (read_reg & DRAMADDRW_BANKBIT_MASK) >>
- DRAMADDRW_BANKBIT_SHIFT;
- cs = (read_reg & DRAMADDRW_CSBIT_MASK) >>
- DRAMADDRW_CSBIT_SHIFT;
-
- /* Correct for ECC as its not addressible */
- if (width == DRAMIFWIDTH_32B_ECC)
- width = 32;
- if (width == DRAMIFWIDTH_16B_ECC)
- width = 16;
-
- /* calculate the SDRAM size base on this info */
- size = 1 << (row + bank + col);
- size = size * cs * (width / 8);
- return size;
+ aw = of_n_addr_cells(np);
+ sw = of_n_size_cells(np);
+ reg = (const unsigned int *)of_get_property(np, "reg", &len);
+ reg_end = reg + (len / sizeof(u32));
+
+ total_mem = 0;
+ do {
+ start = of_read_number(reg, aw);
+ reg += aw;
+ size = of_read_number(reg, sw);
+ reg += sw;
+ total_mem += size;
+ } while (reg < reg_end);
+
+ of_node_put(np);
+ edac_printk(KERN_ERR, EDAC_MC, "total_mem 0x%lx\n", total_mem);
+ return total_mem;
}

static int altr_sdram_probe(struct platform_device *pdev)
@@ -280,7 +279,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
}

/* Grab memory size from device tree. */
- mem_size = altr_sdram_get_total_mem_size(mc_vbase);
+ mem_size = get_total_mem();
if (!mem_size) {
edac_printk(KERN_ERR, EDAC_MC,
"Unable to calculate memory size\n");
--
1.7.9.5

2015-05-13 22:04:09

by Thor Thayer

[permalink] [raw]
Subject: [PATCH 2/4] edac, altera: Refactor EDAC for Altera CycloneV SoC.

From: Thor Thayer <[email protected]>

The Arria10 SOC uses a completely different SDRAM controller from the
earlier CycloneV and ArriaV SoCs. This patch abstracts the SDRAM bits
for the CycloneV/ArriaV SoCs in preparation for the Arria10 support.

Signed-off-by: Thor Thayer <[email protected]>
---
drivers/edac/altera_edac.c | 194 ++++++++++++++++++++------------------------
drivers/edac/altera_edac.h | 116 ++++++++++++++++++++++++++
2 files changed, 206 insertions(+), 104 deletions(-)
create mode 100644 drivers/edac/altera_edac.h

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index e18a205..204ad2d 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1,5 +1,5 @@
/*
- * Copyright Altera Corporation (C) 2014. All rights reserved.
+ * Copyright Altera Corporation (C) 2014-2015. All rights reserved.
* Copyright 2011-2012 Calxeda, Inc.
*
* This program is free software; you can redistribute it and/or modify it
@@ -28,111 +28,64 @@
#include <linux/types.h>
#include <linux/uaccess.h>

+#include "altera_edac.h"
#include "edac_core.h"
#include "edac_module.h"

#define EDAC_MOD_STR "altera_edac"
#define EDAC_VERSION "1"

-/* SDRAM Controller CtrlCfg Register */
-#define CTLCFG_OFST 0x00
-
-/* SDRAM Controller CtrlCfg Register Bit Masks */
-#define CTLCFG_ECC_EN 0x400
-#define CTLCFG_ECC_CORR_EN 0x800
-#define CTLCFG_GEN_SB_ERR 0x2000
-#define CTLCFG_GEN_DB_ERR 0x4000
-
-#define CTLCFG_ECC_AUTO_EN (CTLCFG_ECC_EN | \
- CTLCFG_ECC_CORR_EN)
-
-/* SDRAM Controller Address Width Register */
-#define DRAMADDRW_OFST 0x2C
-
-/* SDRAM Controller Address Widths Field Register */
-#define DRAMADDRW_COLBIT_MASK 0x001F
-#define DRAMADDRW_COLBIT_SHIFT 0
-#define DRAMADDRW_ROWBIT_MASK 0x03E0
-#define DRAMADDRW_ROWBIT_SHIFT 5
-#define DRAMADDRW_BANKBIT_MASK 0x1C00
-#define DRAMADDRW_BANKBIT_SHIFT 10
-#define DRAMADDRW_CSBIT_MASK 0xE000
-#define DRAMADDRW_CSBIT_SHIFT 13
-
-/* SDRAM Controller Interface Data Width Register */
-#define DRAMIFWIDTH_OFST 0x30
-
-/* SDRAM Controller Interface Data Width Defines */
-#define DRAMIFWIDTH_16B_ECC 24
-#define DRAMIFWIDTH_32B_ECC 40
-
-/* SDRAM Controller DRAM Status Register */
-#define DRAMSTS_OFST 0x38
-
-/* SDRAM Controller DRAM Status Register Bit Masks */
-#define DRAMSTS_SBEERR 0x04
-#define DRAMSTS_DBEERR 0x08
-#define DRAMSTS_CORR_DROP 0x10
-
-/* SDRAM Controller DRAM IRQ Register */
-#define DRAMINTR_OFST 0x3C
-
-/* SDRAM Controller DRAM IRQ Register Bit Masks */
-#define DRAMINTR_INTREN 0x01
-#define DRAMINTR_SBEMASK 0x02
-#define DRAMINTR_DBEMASK 0x04
-#define DRAMINTR_CORRDROPMASK 0x08
-#define DRAMINTR_INTRCLR 0x10
-
-/* SDRAM Controller Single Bit Error Count Register */
-#define SBECOUNT_OFST 0x40
-
-/* SDRAM Controller Single Bit Error Count Register Bit Masks */
-#define SBECOUNT_MASK 0x0F
-
-/* SDRAM Controller Double Bit Error Count Register */
-#define DBECOUNT_OFST 0x44
-
-/* SDRAM Controller Double Bit Error Count Register Bit Masks */
-#define DBECOUNT_MASK 0x0F
-
-/* SDRAM Controller ECC Error Address Register */
-#define ERRADDR_OFST 0x48
-
-/* SDRAM Controller ECC Error Address Register Bit Masks */
-#define ERRADDR_MASK 0xFFFFFFFF
-
-/* Altera SDRAM Memory Controller data */
-struct altr_sdram_mc_data {
- struct regmap *mc_vbase;
+const struct altr_sdram_prv_data c5_data = {
+ .ecc_ctrl_offset = CV_CTLCFG_OFST,
+ .ecc_ctl_en_mask = CV_CTLCFG_ECC_AUTO_EN,
+ .ecc_stat_offset = CV_DRAMSTS_OFST,
+ .ecc_stat_ce_mask = CV_DRAMSTS_SBEERR,
+ .ecc_stat_ue_mask = CV_DRAMSTS_DBEERR,
+ .ecc_saddr_offset = CV_ERRADDR_OFST,
+ .ecc_cecnt_offset = CV_SBECOUNT_OFST,
+ .ecc_uecnt_offset = CV_DBECOUNT_OFST,
+ .ecc_irq_en_offset = CV_DRAMINTR_OFST,
+ .ecc_irq_en_mask = CV_DRAMINTR_INTREN,
+ .ecc_irq_clr_offset = CV_DRAMINTR_OFST,
+ .ecc_irq_clr_mask = (CV_DRAMINTR_INTRCLR | CV_DRAMINTR_INTREN),
+ .ecc_cnt_rst_offset = CV_DRAMINTR_OFST,
+ .ecc_cnt_rst_mask = CV_DRAMINTR_INTRCLR,
+#ifdef CONFIG_EDAC_DEBUG
+ .ce_ue_trgr_offset = CV_CTLCFG_OFST,
+ .ce_set_mask = CV_CTLCFG_GEN_SB_ERR,
+ .ue_set_mask = CV_CTLCFG_GEN_DB_ERR,
+#endif
};

static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
{
struct mem_ctl_info *mci = dev_id;
struct altr_sdram_mc_data *drvdata = mci->pvt_info;
+ const struct altr_sdram_prv_data *priv = drvdata->data;
u32 status, err_count, err_addr;

/* Error Address is shared by both SBE & DBE */
- regmap_read(drvdata->mc_vbase, ERRADDR_OFST, &err_addr);
+ regmap_read(drvdata->mc_vbase, priv->ecc_saddr_offset, &err_addr);

- regmap_read(drvdata->mc_vbase, DRAMSTS_OFST, &status);
+ regmap_read(drvdata->mc_vbase, priv->ecc_stat_offset, &status);

- if (status & DRAMSTS_DBEERR) {
- regmap_read(drvdata->mc_vbase, DBECOUNT_OFST, &err_count);
+ if (status & priv->ecc_stat_ue_mask) {
+ regmap_read(drvdata->mc_vbase, priv->ecc_uecnt_offset,
+ &err_count);
panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
err_count, err_addr);
}
- if (status & DRAMSTS_SBEERR) {
- regmap_read(drvdata->mc_vbase, SBECOUNT_OFST, &err_count);
+ if (status & priv->ecc_stat_ce_mask) {
+ regmap_read(drvdata->mc_vbase, priv->ecc_cecnt_offset,
+ &err_count);
edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
err_addr >> PAGE_SHIFT,
err_addr & ~PAGE_MASK, 0,
0, 0, -1, mci->ctl_name, "");
}

- regmap_write(drvdata->mc_vbase, DRAMINTR_OFST,
- (DRAMINTR_INTRCLR | DRAMINTR_INTREN));
+ regmap_write(drvdata->mc_vbase, priv->ecc_irq_clr_offset,
+ priv->ecc_irq_clr_mask);

return IRQ_HANDLED;
}
@@ -144,6 +97,7 @@ static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
{
struct mem_ctl_info *mci = file->private_data;
struct altr_sdram_mc_data *drvdata = mci->pvt_info;
+ const struct altr_sdram_prv_data *priv = drvdata->data;
u32 *ptemp;
dma_addr_t dma_handle;
u32 reg, read_reg;
@@ -156,8 +110,9 @@ static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
return -ENOMEM;
}

- regmap_read(drvdata->mc_vbase, CTLCFG_OFST, &read_reg);
- read_reg &= ~(CTLCFG_GEN_SB_ERR | CTLCFG_GEN_DB_ERR);
+ regmap_read(drvdata->mc_vbase, priv->ce_ue_trgr_offset,
+ &read_reg);
+ read_reg &= ~(priv->ce_set_mask | priv->ue_set_mask);

/* Error are injected by writing a word while the SBE or DBE
* bit in the CTLCFG register is set. Reading the word will
@@ -166,20 +121,20 @@ static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
if (count == 3) {
edac_printk(KERN_ALERT, EDAC_MC,
"Inject Double bit error\n");
- regmap_write(drvdata->mc_vbase, CTLCFG_OFST,
- (read_reg | CTLCFG_GEN_DB_ERR));
+ regmap_write(drvdata->mc_vbase, priv->ce_ue_trgr_offset,
+ (read_reg | priv->ue_set_mask));
} else {
edac_printk(KERN_ALERT, EDAC_MC,
"Inject Single bit error\n");
- regmap_write(drvdata->mc_vbase, CTLCFG_OFST,
- (read_reg | CTLCFG_GEN_SB_ERR));
+ regmap_write(drvdata->mc_vbase, priv->ce_ue_trgr_offset,
+ (read_reg | priv->ce_set_mask));
}

ptemp[0] = 0x5A5A5A5A;
ptemp[1] = 0xA5A5A5A5;

/* Clear the error injection bits */
- regmap_write(drvdata->mc_vbase, CTLCFG_OFST, read_reg);
+ regmap_write(drvdata->mc_vbase, priv->ce_ue_trgr_offset, read_reg);
/* Ensure it has been written out */
wmb();

@@ -250,18 +205,29 @@ static unsigned long get_total_mem(void)
return total_mem;
}

+static const struct of_device_id altr_sdram_ctrl_of_match[] = {
+ { .compatible = "altr,sdram-edac", .data = (void *)&c5_data},
+ {},
+};
+MODULE_DEVICE_TABLE(of, altr_sdram_ctrl_of_match);
+
static int altr_sdram_probe(struct platform_device *pdev)
{
+ const struct of_device_id *id;
struct edac_mc_layer layers[2];
struct mem_ctl_info *mci;
struct altr_sdram_mc_data *drvdata;
+ const struct altr_sdram_prv_data *priv;
struct regmap *mc_vbase;
struct dimm_info *dimm;
- u32 read_reg, mem_size;
- int irq;
- int res = 0;
+ u32 read_reg;
+ int irq, res = 0;
+ unsigned long mem_size;
+
+ id = of_match_device(altr_sdram_ctrl_of_match, &pdev->dev);
+ if (!id)
+ return -ENODEV;

- /* Validate the SDRAM controller has ECC enabled */
/* Grab the register range from the sdr controller in device tree */
mc_vbase = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
"altr,sdr-syscon");
@@ -271,8 +237,13 @@ static int altr_sdram_probe(struct platform_device *pdev)
return -ENODEV;
}

- if (regmap_read(mc_vbase, CTLCFG_OFST, &read_reg) ||
- ((read_reg & CTLCFG_ECC_AUTO_EN) != CTLCFG_ECC_AUTO_EN)) {
+ /* Check specific dependencies for the module */
+ priv = of_match_node(altr_sdram_ctrl_of_match,
+ pdev->dev.of_node)->data;
+
+ /* Validate the SDRAM controller has ECC enabled */
+ if (regmap_read(mc_vbase, priv->ecc_ctrl_offset, &read_reg) ||
+ ((read_reg & priv->ecc_ctl_en_mask) != priv->ecc_ctl_en_mask)) {
edac_printk(KERN_ERR, EDAC_MC,
"No ECC/ECC disabled [0x%08X]\n", read_reg);
return -ENODEV;
@@ -286,10 +257,27 @@ static int altr_sdram_probe(struct platform_device *pdev)
return -ENODEV;
}

- /* Ensure the SDRAM Interrupt is disabled and cleared */
- if (regmap_write(mc_vbase, DRAMINTR_OFST, DRAMINTR_INTRCLR)) {
+ /* Ensure the SDRAM Interrupt is disabled */
+ if (regmap_update_bits(mc_vbase, priv->ecc_irq_en_offset,
+ priv->ecc_irq_en_mask, 0)) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Error disabling SDRAM ECC IRQ\n");
+ return -ENODEV;
+ }
+
+ /* Toggle to clear the SDRAM Error count */
+ if (regmap_update_bits(mc_vbase, priv->ecc_cnt_rst_offset,
+ priv->ecc_cnt_rst_mask,
+ priv->ecc_cnt_rst_mask)) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Error clearing SDRAM ECC count\n");
+ return -ENODEV;
+ }
+
+ if (regmap_update_bits(mc_vbase, priv->ecc_cnt_rst_offset,
+ priv->ecc_cnt_rst_mask, 0)) {
edac_printk(KERN_ERR, EDAC_MC,
- "Error clearing SDRAM ECC IRQ\n");
+ "Error clearing SDRAM ECC count\n");
return -ENODEV;
}

@@ -314,9 +302,12 @@ static int altr_sdram_probe(struct platform_device *pdev)
mci->pdev = &pdev->dev;
drvdata = mci->pvt_info;
drvdata->mc_vbase = mc_vbase;
+ drvdata->data = priv;
platform_set_drvdata(pdev, mci);

if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Unable to get managed device resource\n");
res = -ENOMEM;
goto free;
}
@@ -350,8 +341,9 @@ static int altr_sdram_probe(struct platform_device *pdev)
goto err2;
}

- if (regmap_write(drvdata->mc_vbase, DRAMINTR_OFST,
- (DRAMINTR_INTRCLR | DRAMINTR_INTREN))) {
+ /* Infrastructure ready - enable the IRQ */
+ if (regmap_update_bits(drvdata->mc_vbase, priv->ecc_irq_en_offset,
+ priv->ecc_irq_en_mask, priv->ecc_irq_en_mask)) {
edac_mc_printk(mci, KERN_ERR,
"Error enabling SDRAM ECC IRQ\n");
res = -ENODEV;
@@ -387,12 +379,6 @@ static int altr_sdram_remove(struct platform_device *pdev)
return 0;
}

-static const struct of_device_id altr_sdram_ctrl_of_match[] = {
- { .compatible = "altr,sdram-edac", },
- {},
-};
-MODULE_DEVICE_TABLE(of, altr_sdram_ctrl_of_match);
-
static struct platform_driver altr_sdram_edac_driver = {
.probe = altr_sdram_probe,
.remove = altr_sdram_remove,
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
new file mode 100644
index 0000000..b744d91
--- /dev/null
+++ b/drivers/edac/altera_edac.h
@@ -0,0 +1,116 @@
+/*
+ *
+ * Copyright (C) 2015 Altera Corporation
+ *
+ * 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/>.
+ */
+
+#ifndef _ALTERA_EDAC_H
+#define _ALTERA_EDAC_H
+
+#include <linux/edac.h>
+#include <linux/types.h>
+
+/* SDRAM Controller CtrlCfg Register */
+#define CV_CTLCFG_OFST 0x00
+
+/* SDRAM Controller CtrlCfg Register Bit Masks */
+#define CV_CTLCFG_ECC_EN 0x400
+#define CV_CTLCFG_ECC_CORR_EN 0x800
+#define CV_CTLCFG_GEN_SB_ERR 0x2000
+#define CV_CTLCFG_GEN_DB_ERR 0x4000
+
+#define CV_CTLCFG_ECC_AUTO_EN (CV_CTLCFG_ECC_EN | \
+ CV_CTLCFG_ECC_CORR_EN)
+
+/* SDRAM Controller Address Width Register */
+#define CV_DRAMADDRW_OFST 0x2C
+
+/* SDRAM Controller Address Widths Field Register */
+#define DRAMADDRW_COLBIT_MASK 0x001F
+#define DRAMADDRW_COLBIT_SHIFT 0
+#define DRAMADDRW_ROWBIT_MASK 0x03E0
+#define DRAMADDRW_ROWBIT_SHIFT 5
+#define CV_DRAMADDRW_BANKBIT_MASK 0x1C00
+#define CV_DRAMADDRW_BANKBIT_SHIFT 10
+#define CV_DRAMADDRW_CSBIT_MASK 0xE000
+#define CV_DRAMADDRW_CSBIT_SHIFT 13
+
+/* SDRAM Controller Interface Data Width Register */
+#define CV_DRAMIFWIDTH_OFST 0x30
+
+/* SDRAM Controller Interface Data Width Defines */
+#define CV_DRAMIFWIDTH_16B_ECC 24
+#define CV_DRAMIFWIDTH_32B_ECC 40
+
+/* SDRAM Controller DRAM Status Register */
+#define CV_DRAMSTS_OFST 0x38
+
+/* SDRAM Controller DRAM Status Register Bit Masks */
+#define CV_DRAMSTS_SBEERR 0x04
+#define CV_DRAMSTS_DBEERR 0x08
+#define CV_DRAMSTS_CORR_DROP 0x10
+
+/* SDRAM Controller DRAM IRQ Register */
+#define CV_DRAMINTR_OFST 0x3C
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define CV_DRAMINTR_INTREN 0x01
+#define CV_DRAMINTR_SBEMASK 0x02
+#define CV_DRAMINTR_DBEMASK 0x04
+#define CV_DRAMINTR_CORRDROPMASK 0x08
+#define CV_DRAMINTR_INTRCLR 0x10
+
+/* SDRAM Controller Single Bit Error Count Register */
+#define CV_SBECOUNT_OFST 0x40
+
+/* SDRAM Controller Double Bit Error Count Register */
+#define CV_DBECOUNT_OFST 0x44
+
+/* SDRAM Controller ECC Error Address Register */
+#define CV_ERRADDR_OFST 0x48
+
+struct altr_sdram_prv_data {
+ int ecc_ctrl_offset;
+ int ecc_ctl_en_mask;
+ int ecc_cecnt_offset;
+ int ecc_uecnt_offset;
+ int ecc_stat_offset;
+ int ecc_stat_ce_mask;
+ int ecc_stat_ue_mask;
+ int ecc_saddr_offset;
+ int ecc_daddr_offset;
+ int ecc_irq_en_offset;
+ int ecc_irq_en_mask;
+ int ecc_irq_clr_offset;
+ int ecc_irq_clr_mask;
+ int ecc_cnt_rst_offset;
+ int ecc_cnt_rst_mask;
+#ifdef CONFIG_EDAC_DEBUG
+ struct edac_dev_sysfs_attribute *eccmgr_sysfs_attr;
+ int ecc_enable_mask;
+ int ce_set_mask;
+ int ue_set_mask;
+ int ce_ue_trgr_offset;
+#endif
+};
+
+/* Altera SDRAM Memory Controller data */
+struct altr_sdram_mc_data {
+ struct regmap *mc_vbase;
+ int sb_irq;
+ int db_irq;
+ const struct altr_sdram_prv_data *data;
+};
+
+#endif /* #ifndef _ALTERA_EDAC_H */
--
1.7.9.5

2015-05-13 22:03:26

by Thor Thayer

[permalink] [raw]
Subject: [PATCH 3/4] edac, altera: Addition of Arria10 EDAC

From: Thor Thayer <[email protected]>

The Arria10 SDRAM and ECC system differs significantly from the
Cyclone5 and Arria5 SoCs. This patch adds support for the Arria10
SoC.
1) IRQ handler needs to support SHARED IRQ
2) Support sberr and dberr address reporting.

Signed-off-by: Thor Thayer <[email protected]>
---
drivers/edac/altera_edac.c | 132 ++++++++++++++++++++++++++++++++++++++------
drivers/edac/altera_edac.h | 85 ++++++++++++++++++++++++++++
2 files changed, 201 insertions(+), 16 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 204ad2d..735a180 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -42,6 +42,7 @@ const struct altr_sdram_prv_data c5_data = {
.ecc_stat_ce_mask = CV_DRAMSTS_SBEERR,
.ecc_stat_ue_mask = CV_DRAMSTS_DBEERR,
.ecc_saddr_offset = CV_ERRADDR_OFST,
+ .ecc_daddr_offset = CV_ERRADDR_OFST,
.ecc_cecnt_offset = CV_SBECOUNT_OFST,
.ecc_uecnt_offset = CV_DBECOUNT_OFST,
.ecc_irq_en_offset = CV_DRAMINTR_OFST,
@@ -57,37 +58,62 @@ const struct altr_sdram_prv_data c5_data = {
#endif
};

+const struct altr_sdram_prv_data a10_data = {
+ .ecc_ctrl_offset = A10_ECCCTRL1_OFST,
+ .ecc_ctl_en_mask = A10_ECCCTRL1_ECC_EN,
+ .ecc_stat_offset = A10_INTSTAT_OFST,
+ .ecc_stat_ce_mask = A10_INTSTAT_SBEERR,
+ .ecc_stat_ue_mask = A10_INTSTAT_DBEERR,
+ .ecc_saddr_offset = A10_SERRADDR_OFST,
+ .ecc_daddr_offset = A10_DERRADDR_OFST,
+ .ecc_irq_en_offset = A10_ERRINTEN_OFST,
+ .ecc_irq_en_mask = A10_ECC_IRQ_EN_MASK,
+ .ecc_irq_clr_offset = A10_INTSTAT_OFST,
+ .ecc_irq_clr_mask = (A10_INTSTAT_SBEERR | A10_INTSTAT_DBEERR),
+ .ecc_cnt_rst_offset = A10_ECCCTRL1_OFST,
+ .ecc_cnt_rst_mask = A10_ECC_CNT_RESET_MASK,
+#ifdef CONFIG_EDAC_DEBUG
+ .ce_ue_trgr_offset = A10_DIAGINTTEST_OFST,
+ .ce_set_mask = A10_DIAGINT_TSERRA_MASK,
+ .ue_set_mask = A10_DIAGINT_TDERRA_MASK,
+#endif
+};
+
static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
{
struct mem_ctl_info *mci = dev_id;
struct altr_sdram_mc_data *drvdata = mci->pvt_info;
const struct altr_sdram_prv_data *priv = drvdata->data;
- u32 status, err_count, err_addr;
-
- /* Error Address is shared by both SBE & DBE */
- regmap_read(drvdata->mc_vbase, priv->ecc_saddr_offset, &err_addr);
+ u32 status, err_count = 1, err_addr;

regmap_read(drvdata->mc_vbase, priv->ecc_stat_offset, &status);

if (status & priv->ecc_stat_ue_mask) {
- regmap_read(drvdata->mc_vbase, priv->ecc_uecnt_offset,
- &err_count);
+ regmap_read(drvdata->mc_vbase, priv->ecc_daddr_offset,
+ &err_addr);
+ if (priv->ecc_uecnt_offset)
+ regmap_read(drvdata->mc_vbase, priv->ecc_uecnt_offset,
+ &err_count);
panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
err_count, err_addr);
}
if (status & priv->ecc_stat_ce_mask) {
- regmap_read(drvdata->mc_vbase, priv->ecc_cecnt_offset,
- &err_count);
+ regmap_read(drvdata->mc_vbase, priv->ecc_saddr_offset,
+ &err_addr);
+ if (priv->ecc_uecnt_offset)
+ regmap_read(drvdata->mc_vbase, priv->ecc_cecnt_offset,
+ &err_count);
edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
err_addr >> PAGE_SHIFT,
err_addr & ~PAGE_MASK, 0,
0, 0, -1, mci->ctl_name, "");
- }
-
- regmap_write(drvdata->mc_vbase, priv->ecc_irq_clr_offset,
- priv->ecc_irq_clr_mask);
+ /* Clear IRQ to resume */
+ regmap_write(drvdata->mc_vbase, priv->ecc_irq_clr_offset,
+ priv->ecc_irq_clr_mask);

- return IRQ_HANDLED;
+ return IRQ_HANDLED;
+ }
+ return IRQ_NONE;
}

#ifdef CONFIG_EDAC_DEBUG
@@ -207,10 +233,58 @@ static unsigned long get_total_mem(void)

static const struct of_device_id altr_sdram_ctrl_of_match[] = {
{ .compatible = "altr,sdram-edac", .data = (void *)&c5_data},
+ { .compatible = "altr,sdram-edac-a10", .data = (void *)&a10_data},
{},
};
MODULE_DEVICE_TABLE(of, altr_sdram_ctrl_of_match);

+static int a10_init(struct regmap *mc_vbase)
+{
+ if (regmap_update_bits(mc_vbase, A10_INTMODE_OFST,
+ A10_INTMODE_SB_INT, A10_INTMODE_SB_INT)) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Error setting SB IRQ mode\n");
+ return -ENODEV;
+ }
+
+ if (regmap_write(mc_vbase, A10_SERRCNTREG_OFST, 1)) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Error setting trigger count\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static int a10_unmask_irq(struct platform_device *pdev, u32 mask)
+{
+ void __iomem *sm_base;
+
+ if (!devm_request_mem_region(&pdev->dev, A10_SYMAN_INTMASK_CLR,
+ sizeof(u32), dev_name(&pdev->dev))) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Unable to request mem region\n");
+ return -EBUSY;
+ }
+
+ sm_base = devm_ioremap(&pdev->dev, A10_SYMAN_INTMASK_CLR,
+ sizeof(u32));
+ if (!sm_base) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Unable to ioremap device\n");
+
+ return -ENOMEM;
+ }
+
+ iowrite32(mask, sm_base);
+
+ devm_iounmap(&pdev->dev, sm_base);
+ devm_release_mem_region(&pdev->dev, A10_SYMAN_INTMASK_CLR,
+ sizeof(u32));
+
+ return 0;
+}
+
static int altr_sdram_probe(struct platform_device *pdev)
{
const struct of_device_id *id;
@@ -221,8 +295,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
struct regmap *mc_vbase;
struct dimm_info *dimm;
u32 read_reg;
- int irq, res = 0;
- unsigned long mem_size;
+ int irq, irq2, res = 0;
+ unsigned long mem_size, irqflags;

id = of_match_device(altr_sdram_ctrl_of_match, &pdev->dev);
if (!id)
@@ -288,6 +362,9 @@ static int altr_sdram_probe(struct platform_device *pdev)
return -ENODEV;
}

+ /* Arria10 has a 2nd IRQ */
+ irq2 = platform_get_irq(pdev, 1);
+
layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
layers[0].size = 1;
layers[0].is_virt_csrow = true;
@@ -332,8 +409,31 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0)
goto err;

+ /* Only the Arria10 has separate IRQs */
+ if (irq2 > 0) {
+ /* Arria10 specific initialization */
+ res = a10_init(mc_vbase);
+ if (res < 0)
+ goto err2;
+
+ res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
+ if (res < 0)
+ goto err2;
+
+ res = devm_request_irq(&pdev->dev, irq2,
+ altr_sdram_mc_err_handler,
+ IRQF_SHARED, dev_name(&pdev->dev), mci);
+ if (res < 0) {
+ edac_mc_printk(mci, KERN_ERR,
+ "Unable to request irq %d\n", irq2);
+ res = -ENODEV;
+ goto err2;
+ }
+ irqflags = IRQF_SHARED;
+ }
+
res = devm_request_irq(&pdev->dev, irq, altr_sdram_mc_err_handler,
- 0, dev_name(&pdev->dev), mci);
+ irqflags, dev_name(&pdev->dev), mci);
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
"Unable to request irq %d\n", irq);
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index b744d91..7b64dc7 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -80,6 +80,91 @@
/* SDRAM Controller ECC Error Address Register */
#define CV_ERRADDR_OFST 0x48

+/*-----------------------------------------*/
+
+/* SDRAM Controller EccCtrl Register */
+#define A10_ECCCTRL1_OFST 0x00
+
+/* SDRAM Controller EccCtrl Register Bit Masks */
+#define A10_ECCCTRL1_ECC_EN 0x001
+#define A10_ECCCTRL1_CNT_RST 0x010
+#define A10_ECCCTRL1_AWB_CNT_RST 0x100
+#define A10_ECC_CNT_RESET_MASK (A10_ECCCTRL1_CNT_RST | \
+ A10_ECCCTRL1_AWB_CNT_RST)
+
+/* SDRAM Controller Address Width Register */
+#define CV_DRAMADDRW 0xFFC2502C
+#define A10_DRAMADDRW 0xFFCFA0A8
+
+/* SDRAM Controller Address Widths Field Register */
+#define DRAMADDRW_COLBIT_MASK 0x001F
+#define DRAMADDRW_COLBIT_SHIFT 0
+#define DRAMADDRW_ROWBIT_MASK 0x03E0
+#define DRAMADDRW_ROWBIT_SHIFT 5
+#define CV_DRAMADDRW_BANKBIT_MASK 0x1C00
+#define CV_DRAMADDRW_BANKBIT_SHIFT 10
+#define CV_DRAMADDRW_CSBIT_MASK 0xE000
+#define CV_DRAMADDRW_CSBIT_SHIFT 13
+
+#define A10_DRAMADDRW_BANKBIT_MASK 0x3C00
+#define A10_DRAMADDRW_BANKBIT_SHIFT 10
+#define A10_DRAMADDRW_GRPBIT_MASK 0xC000
+#define A10_DRAMADDRW_GRPBIT_SHIFT 14
+#define A10_DRAMADDRW_CSBIT_MASK 0x70000
+#define A10_DRAMADDRW_CSBIT_SHIFT 16
+
+/* SDRAM Controller Interface Data Width Register */
+#define CV_DRAMIFWIDTH 0xFFC25030
+#define A10_DRAMIFWIDTH 0xFFCFB008
+
+/* SDRAM Controller Interface Data Width Defines */
+#define CV_DRAMIFWIDTH_16B_ECC 24
+#define CV_DRAMIFWIDTH_32B_ECC 40
+
+#define A10_DRAMIFWIDTH_16B 0x0
+#define A10_DRAMIFWIDTH_32B 0x1
+#define A10_DRAMIFWIDTH_64B 0x2
+
+/* SDRAM Controller DRAM IRQ Register */
+#define A10_ERRINTEN_OFST 0x10
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define A10_ERRINTEN_SERRINTEN 0x01
+#define A10_ERRINTEN_DERRINTEN 0x02
+#define A10_ECC_IRQ_EN_MASK (A10_ERRINTEN_SERRINTEN | \
+ A10_ERRINTEN_DERRINTEN)
+
+/* SDRAM Interrupt Mode Register */
+#define A10_INTMODE_OFST 0x1C
+#define A10_INTMODE_SB_INT 1
+
+/* SDRAM Controller Error Status Register */
+#define A10_INTSTAT_OFST 0x20
+
+/* SDRAM Controller Error Status Register Bit Masks */
+#define A10_INTSTAT_SBEERR 0x01
+#define A10_INTSTAT_DBEERR 0x02
+
+/* SDRAM Controller ECC Error Address Register */
+#define A10_DERRADDR_OFST 0x2C
+#define A10_SERRADDR_OFST 0x30
+
+/* SDRAM Controller ECC Diagnostic Register */
+#define A10_DIAGINTTEST_OFST 0x24
+
+#define A10_DIAGINT_TSERRA_MASK 0x0001
+#define A10_DIAGINT_TDERRA_MASK 0x0100
+
+#define A10_SBERR_IRQ 34
+#define A10_DBERR_IRQ 32
+
+/* SDRAM Single Bit Error Count Compare Set Register */
+#define A10_SERRCNTREG_OFST 0x3C
+
+#define A10_SYMAN_INTMASK_CLR 0xFFD06098
+#define A10_INTMASK_CLR_OFST 0x10
+#define A10_DDR0_IRQ_MASK BIT(17)
+
struct altr_sdram_prv_data {
int ecc_ctrl_offset;
int ecc_ctl_en_mask;
--
1.7.9.5

2015-05-13 22:03:43

by Thor Thayer

[permalink] [raw]
Subject: [PATCH 4/4] dts, altera: Arria10 SDRAM EDAC DTS additions.

From: Thor Thayer <[email protected]>

Support for the Arria10 SDRAM EDAC is added to the device tree.
Update the bindings document for the new match string.

Signed-off-by: Thor Thayer <[email protected]>
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 2 +-
arch/arm/boot/dts/socfpga_arria10.dtsi | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
index d0ce01d..f5ad0ff 100644
--- a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -2,7 +2,7 @@ Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
The EDAC accesses a range of registers in the SDRAM controller.

Required properties:
-- compatible : should contain "altr,sdram-edac";
+- compatible : should contain "altr,sdram-edac" or "altr,sdram-edac-a10"
- altr,sdr-syscon : phandle of the sdr module
- interrupts : Should contain the SDRAM ECC IRQ in the
appropriate format for the IRQ controller.
diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi b/arch/arm/boot/dts/socfpga_arria10.dtsi
index e121661..70da147 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -524,6 +524,17 @@
status = "disabled";
};

+ sdr: sdr@ffc25000 {
+ compatible = "syscon";
+ reg = <0xffcfb100 0x80>;
+ };
+
+ sdramedac {
+ compatible = "altr,sdram-edac-a10";
+ altr,sdr-syscon = <&sdr>;
+ interrupts = <0 2 4>, <0 0 4>;
+ };
+
L2: l2-cache@fffff000 {
compatible = "arm,pl310-cache";
reg = <0xfffff000 0x1000>;
--
1.7.9.5

2015-05-14 03:25:51

by Dinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH 1/4] edac, altera: Generalize driver to use DT Memory size



On 5/13/15 4:49 PM, [email protected] wrote:
> From: Thor Thayer <[email protected]>
>
> The Arria10 SOC uses a completely different SDRAM controller from the
> earlier CycloneV and ArriaV SoCs. The memory size is calculated in
> the bootloader and passed via the device tree. Using this device
> tree size is more generic than using the register fields to
> calculate the memory size for different SDRAM controllers.
>
> Signed-off-by: Thor Thayer <[email protected]>
> ---
> drivers/edac/altera_edac.c | 53 ++++++++++++++++++++++----------------------
> 1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 3c4929f..e18a205 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -219,36 +219,35 @@ static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
> {}
> #endif
>
> -/* Get total memory size in bytes */
> -static u32 altr_sdram_get_total_mem_size(struct regmap *mc_vbase)
> +/* Get total memory size from Open Firmware DTB */
> +static unsigned long get_total_mem(void)
> {
> - u32 size, read_reg, row, bank, col, cs, width;
> + struct device_node *np = NULL;
> + const unsigned int *reg, *reg_end;
> + int len, sw, aw;
> + unsigned long start, size, total_mem;
>
> - if (regmap_read(mc_vbase, DRAMADDRW_OFST, &read_reg) < 0)
> + np = of_find_node_by_type(NULL, "memory");
> + if (!np)
> return 0;
>
> - if (regmap_read(mc_vbase, DRAMIFWIDTH_OFST, &width) < 0)
> - return 0;
> -
> - col = (read_reg & DRAMADDRW_COLBIT_MASK) >>
> - DRAMADDRW_COLBIT_SHIFT;
> - row = (read_reg & DRAMADDRW_ROWBIT_MASK) >>
> - DRAMADDRW_ROWBIT_SHIFT;
> - bank = (read_reg & DRAMADDRW_BANKBIT_MASK) >>
> - DRAMADDRW_BANKBIT_SHIFT;
> - cs = (read_reg & DRAMADDRW_CSBIT_MASK) >>
> - DRAMADDRW_CSBIT_SHIFT;
> -
> - /* Correct for ECC as its not addressible */
> - if (width == DRAMIFWIDTH_32B_ECC)
> - width = 32;
> - if (width == DRAMIFWIDTH_16B_ECC)
> - width = 16;
> -
> - /* calculate the SDRAM size base on this info */
> - size = 1 << (row + bank + col);
> - size = size * cs * (width / 8);
> - return size;
> + aw = of_n_addr_cells(np);
> + sw = of_n_size_cells(np);
> + reg = (const unsigned int *)of_get_property(np, "reg", &len);
> + reg_end = reg + (len / sizeof(u32));
> +
> + total_mem = 0;
> + do {
> + start = of_read_number(reg, aw);
> + reg += aw;
> + size = of_read_number(reg, sw);
> + reg += sw;
> + total_mem += size;
> + } while (reg < reg_end);
> +
> + of_node_put(np);
> + edac_printk(KERN_ERR, EDAC_MC, "total_mem 0x%lx\n", total_mem);

Use edac_dbg() here.

Dinh

2015-05-14 20:18:59

by Dinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH 2/4] edac, altera: Refactor EDAC for Altera CycloneV SoC.

On 05/13/2015 04:49 PM, [email protected] wrote:
> From: Thor Thayer <[email protected]>
>
> The Arria10 SOC uses a completely different SDRAM controller from the
> earlier CycloneV and ArriaV SoCs. This patch abstracts the SDRAM bits
> for the CycloneV/ArriaV SoCs in preparation for the Arria10 support.
>
> Signed-off-by: Thor Thayer <[email protected]>
> ---
> drivers/edac/altera_edac.c | 194 ++++++++++++++++++++------------------------
> drivers/edac/altera_edac.h | 116 ++++++++++++++++++++++++++
> 2 files changed, 206 insertions(+), 104 deletions(-)
> create mode 100644 drivers/edac/altera_edac.h
>
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index e18a205..204ad2d 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright Altera Corporation (C) 2014. All rights reserved.
> + * Copyright Altera Corporation (C) 2014-2015. All rights reserved.
> * Copyright 2011-2012 Calxeda, Inc.
> *
> * This program is free software; you can redistribute it and/or modify it
> @@ -28,111 +28,64 @@
> #include <linux/types.h>
> #include <linux/uaccess.h>
>
[...]
> -
> -/* Altera SDRAM Memory Controller data */
> -struct altr_sdram_mc_data {
> - struct regmap *mc_vbase;
> +const struct altr_sdram_prv_data c5_data = {

This should be static.

Dinh

2015-05-14 20:26:38

by Dinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH 3/4] edac, altera: Addition of Arria10 EDAC

On 05/13/2015 04:49 PM, [email protected] wrote:
> From: Thor Thayer <[email protected]>
>
> The Arria10 SDRAM and ECC system differs significantly from the
> Cyclone5 and Arria5 SoCs. This patch adds support for the Arria10
> SoC.
> 1) IRQ handler needs to support SHARED IRQ
> 2) Support sberr and dberr address reporting.
>
> Signed-off-by: Thor Thayer <[email protected]>
> ---
> drivers/edac/altera_edac.c | 132 ++++++++++++++++++++++++++++++++++++++------
> drivers/edac/altera_edac.h | 85 ++++++++++++++++++++++++++++
> 2 files changed, 201 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 204ad2d..735a180 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -42,6 +42,7 @@ const struct altr_sdram_prv_data c5_data = {
> .ecc_stat_ce_mask = CV_DRAMSTS_SBEERR,
> .ecc_stat_ue_mask = CV_DRAMSTS_DBEERR,
> .ecc_saddr_offset = CV_ERRADDR_OFST,
> + .ecc_daddr_offset = CV_ERRADDR_OFST,
> .ecc_cecnt_offset = CV_SBECOUNT_OFST,
> .ecc_uecnt_offset = CV_DBECOUNT_OFST,
> .ecc_irq_en_offset = CV_DRAMINTR_OFST,
> @@ -57,37 +58,62 @@ const struct altr_sdram_prv_data c5_data = {
> #endif
> };
>
> +const struct altr_sdram_prv_data a10_data = {\

This should be static.

> + .ecc_ctrl_offset = A10_ECCCTRL1_OFST,
> + .ecc_ctl_en_mask = A10_ECCCTRL1_ECC_EN,
> + .ecc_stat_offset = A10_INTSTAT_OFST,
> + .ecc_stat_ce_mask = A10_INTSTAT_SBEERR,
> + .ecc_stat_ue_mask = A10_INTSTAT_DBEERR,
> + .ecc_saddr_offset = A10_SERRADDR_OFST,
> + .ecc_daddr_offset = A10_DERRADDR_OFST,
> + .ecc_irq_en_offset = A10_ERRINTEN_OFST,
> + .ecc_irq_en_mask = A10_ECC_IRQ_EN_MASK,
> + .ecc_irq_clr_offset = A10_INTSTAT_OFST,
> + .ecc_irq_clr_mask = (A10_INTSTAT_SBEERR | A10_INTSTAT_DBEERR),
> + .ecc_cnt_rst_offset = A10_ECCCTRL1_OFST,
> + .ecc_cnt_rst_mask = A10_ECC_CNT_RESET_MASK,
> +#ifdef CONFIG_EDAC_DEBUG
> + .ce_ue_trgr_offset = A10_DIAGINTTEST_OFST,
> + .ce_set_mask = A10_DIAGINT_TSERRA_MASK,
> + .ue_set_mask = A10_DIAGINT_TDERRA_MASK,
> +#endif
> +};
> +
>
<snip>

> +
> static int altr_sdram_probe(struct platform_device *pdev)
> {
> const struct of_device_id *id;
> @@ -221,8 +295,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
> struct regmap *mc_vbase;
> struct dimm_info *dimm;
> u32 read_reg;
> - int irq, res = 0;
> - unsigned long mem_size;
> + int irq, irq2, res = 0;
> + unsigned long mem_size, irqflags;
>
> id = of_match_device(altr_sdram_ctrl_of_match, &pdev->dev);
> if (!id)
> @@ -288,6 +362,9 @@ static int altr_sdram_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> + /* Arria10 has a 2nd IRQ */
> + irq2 = platform_get_irq(pdev, 1);
> +
> layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> layers[0].size = 1;
> layers[0].is_virt_csrow = true;
> @@ -332,8 +409,31 @@ static int altr_sdram_probe(struct platform_device *pdev)
> if (res < 0)
> goto err;
>
> + /* Only the Arria10 has separate IRQs */
> + if (irq2 > 0) {
> + /* Arria10 specific initialization */
> + res = a10_init(mc_vbase);
> + if (res < 0)
> + goto err2;
> +
> + res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
> + if (res < 0)
> + goto err2;
> +
> + res = devm_request_irq(&pdev->dev, irq2,
> + altr_sdram_mc_err_handler,
> + IRQF_SHARED, dev_name(&pdev->dev), mci);
> + if (res < 0) {
> + edac_mc_printk(mci, KERN_ERR,
> + "Unable to request irq %d\n", irq2);
> + res = -ENODEV;
> + goto err2;
> + }
> + irqflags = IRQF_SHARED;
> + }
> +
> res = devm_request_irq(&pdev->dev, irq, altr_sdram_mc_err_handler,
> - 0, dev_name(&pdev->dev), mci);
> + irqflags, dev_name(&pdev->dev), mci);

irqflags was never set for the case of !(irq2 > 0).

Dinh

2015-05-14 20:37:32

by Thor Thayer

[permalink] [raw]
Subject: Re: [PATCH 3/4] edac, altera: Addition of Arria10 EDAC



On 05/14/2015 03:20 PM, Dinh Nguyen wrote:
> On 05/13/2015 04:49 PM, [email protected] wrote:
>> From: Thor Thayer <[email protected]>
>>
>> The Arria10 SDRAM and ECC system differs significantly from the
>> Cyclone5 and Arria5 SoCs. This patch adds support for the Arria10
>> SoC.
>> 1) IRQ handler needs to support SHARED IRQ
>> 2) Support sberr and dberr address reporting.
>>
>> Signed-off-by: Thor Thayer <[email protected]>
>> ---
>> drivers/edac/altera_edac.c | 132 ++++++++++++++++++++++++++++++++++++++------
>> drivers/edac/altera_edac.h | 85 ++++++++++++++++++++++++++++
>> 2 files changed, 201 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index 204ad2d..735a180 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -42,6 +42,7 @@ const struct altr_sdram_prv_data c5_data = {
>> .ecc_stat_ce_mask = CV_DRAMSTS_SBEERR,
>> .ecc_stat_ue_mask = CV_DRAMSTS_DBEERR,
>> .ecc_saddr_offset = CV_ERRADDR_OFST,
>> + .ecc_daddr_offset = CV_ERRADDR_OFST,
>> .ecc_cecnt_offset = CV_SBECOUNT_OFST,
>> .ecc_uecnt_offset = CV_DBECOUNT_OFST,
>> .ecc_irq_en_offset = CV_DRAMINTR_OFST,
>> @@ -57,37 +58,62 @@ const struct altr_sdram_prv_data c5_data = {
>> #endif
>> };
>>
>> +const struct altr_sdram_prv_data a10_data = {\
>
> This should be static.
>
>> + .ecc_ctrl_offset = A10_ECCCTRL1_OFST,
>> + .ecc_ctl_en_mask = A10_ECCCTRL1_ECC_EN,
>> + .ecc_stat_offset = A10_INTSTAT_OFST,
>> + .ecc_stat_ce_mask = A10_INTSTAT_SBEERR,
>> + .ecc_stat_ue_mask = A10_INTSTAT_DBEERR,
>> + .ecc_saddr_offset = A10_SERRADDR_OFST,
>> + .ecc_daddr_offset = A10_DERRADDR_OFST,
>> + .ecc_irq_en_offset = A10_ERRINTEN_OFST,
>> + .ecc_irq_en_mask = A10_ECC_IRQ_EN_MASK,
>> + .ecc_irq_clr_offset = A10_INTSTAT_OFST,
>> + .ecc_irq_clr_mask = (A10_INTSTAT_SBEERR | A10_INTSTAT_DBEERR),
>> + .ecc_cnt_rst_offset = A10_ECCCTRL1_OFST,
>> + .ecc_cnt_rst_mask = A10_ECC_CNT_RESET_MASK,
>> +#ifdef CONFIG_EDAC_DEBUG
>> + .ce_ue_trgr_offset = A10_DIAGINTTEST_OFST,
>> + .ce_set_mask = A10_DIAGINT_TSERRA_MASK,
>> + .ue_set_mask = A10_DIAGINT_TDERRA_MASK,
>> +#endif
>> +};
>> +
>>
> <snip>
>
>> +
>> static int altr_sdram_probe(struct platform_device *pdev)
>> {
>> const struct of_device_id *id;
>> @@ -221,8 +295,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
>> struct regmap *mc_vbase;
>> struct dimm_info *dimm;
>> u32 read_reg;
>> - int irq, res = 0;
>> - unsigned long mem_size;
>> + int irq, irq2, res = 0;
>> + unsigned long mem_size, irqflags;
>>
>> id = of_match_device(altr_sdram_ctrl_of_match, &pdev->dev);
>> if (!id)
>> @@ -288,6 +362,9 @@ static int altr_sdram_probe(struct platform_device *pdev)
>> return -ENODEV;
>> }
>>
>> + /* Arria10 has a 2nd IRQ */
>> + irq2 = platform_get_irq(pdev, 1);
>> +
>> layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
>> layers[0].size = 1;
>> layers[0].is_virt_csrow = true;
>> @@ -332,8 +409,31 @@ static int altr_sdram_probe(struct platform_device *pdev)
>> if (res < 0)
>> goto err;
>>
>> + /* Only the Arria10 has separate IRQs */
>> + if (irq2 > 0) {
>> + /* Arria10 specific initialization */
>> + res = a10_init(mc_vbase);
>> + if (res < 0)
>> + goto err2;
>> +
>> + res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
>> + if (res < 0)
>> + goto err2;
>> +
>> + res = devm_request_irq(&pdev->dev, irq2,
>> + altr_sdram_mc_err_handler,
>> + IRQF_SHARED, dev_name(&pdev->dev), mci);
>> + if (res < 0) {
>> + edac_mc_printk(mci, KERN_ERR,
>> + "Unable to request irq %d\n", irq2);
>> + res = -ENODEV;
>> + goto err2;
>> + }
>> + irqflags = IRQF_SHARED;
>> + }
>> +
>> res = devm_request_irq(&pdev->dev, irq, altr_sdram_mc_err_handler,
>> - 0, dev_name(&pdev->dev), mci);
>> + irqflags, dev_name(&pdev->dev), mci);
>
> irqflags was never set for the case of !(irq2 > 0).
>
> Dinh
>

Correct. So irqflags will be 0 for the CycloneV case.

2015-05-15 10:56:21

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 4/4] dts, altera: Arria10 SDRAM EDAC DTS additions.

On Wednesday 13 May 2015 16:49:47 [email protected] wrote:
> + sdr: sdr@ffc25000 {
> + compatible = "syscon";
> + reg = <0xffcfb100 0x80>;
> + };
> +
>

A syscon node with just 128 bytes seems very odd. Can you check the
hardware manual to see if this is part of some bigger unit?

Arnd

2015-05-15 10:58:11

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 3/4] edac, altera: Addition of Arria10 EDAC

On Wednesday 13 May 2015 16:49:46 [email protected] wrote:
> +static int a10_unmask_irq(struct platform_device *pdev, u32 mask)
> +{
> + void __iomem *sm_base;
> +
> + if (!devm_request_mem_region(&pdev->dev, A10_SYMAN_INTMASK_CLR,
> + sizeof(u32), dev_name(&pdev->dev))) {
> + edac_printk(KERN_ERR, EDAC_MC,
> + "Unable to request mem region\n");
> + return -EBUSY;
> + }
> +
> + sm_base = devm_ioremap(&pdev->dev, A10_SYMAN_INTMASK_CLR,
> + sizeof(u32));
> + if (!sm_base) {
> + edac_printk(KERN_ERR, EDAC_MC,
> + "Unable to ioremap device\n");
> +
> + return -ENOMEM;
> + }
> +
> + iowrite32(mask, sm_base);
> +
> + devm_iounmap(&pdev->dev, sm_base);
> + devm_release_mem_region(&pdev->dev, A10_SYMAN_INTMASK_CLR,
> + sizeof(u32));
> +
> + return 0;
> +}

If you always unmap right away, better use the normal request_mem_region
and ioremap functions rather than their devm counterparts.


>
> + /* Only the Arria10 has separate IRQs */
> + if (irq2 > 0) {
> + /* Arria10 specific initialization */
> + res = a10_init(mc_vbase);
> + if (res < 0)
> + goto err2;
> +
> + res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
> + if (res < 0)
> + goto err2;
> +
> + res = devm_request_irq(&pdev->dev, irq2,
> + altr_sdram_mc_err_handler,
> + IRQF_SHARED, dev_name(&pdev->dev), mci);
> + if (res < 0) {
> + edac_mc_printk(mci, KERN_ERR,
> + "Unable to request irq %d\n", irq2);
> + res = -ENODEV;
> + goto err2;
> + }
> + irqflags = IRQF_SHARED;
> + }
> +
>

Should the unmask be done after the request?

Arnd

2015-05-15 11:01:46

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 1/4] edac, altera: Generalize driver to use DT Memory size

On Wednesday 13 May 2015 16:49:44 [email protected] wrote:
> -static u32 altr_sdram_get_total_mem_size(struct regmap *mc_vbase)
> +/* Get total memory size from Open Firmware DTB */
> +static unsigned long get_total_mem(void)
> {
> - u32 size, read_reg, row, bank, col, cs, width;
> + struct device_node *np = NULL;
> + const unsigned int *reg, *reg_end;
> + int len, sw, aw;
> + unsigned long start, size, total_mem;
>
> - if (regmap_read(mc_vbase, DRAMADDRW_OFST, &read_reg) < 0)
> + np = of_find_node_by_type(NULL, "memory");
> + if (!np)
> return 0;

There can be multiple memory nodes, I think you need to have a loop
using for_each_node_by_type.


Arnd

2015-05-15 21:00:22

by Thor Thayer

[permalink] [raw]
Subject: Re: [PATCH 4/4] dts, altera: Arria10 SDRAM EDAC DTS additions.

Hi Arnd,

On 05/15/2015 05:55 AM, Arnd Bergmann wrote:
> On Wednesday 13 May 2015 16:49:47 [email protected] wrote:
>> + sdr: sdr@ffc25000 {
>> + compatible = "syscon";
>> + reg = <0xffcfb100 0x80>;
>> + };
>> +
>>
>
> A syscon node with just 128 bytes seems very odd. Can you check the
> hardware manual to see if this is part of some bigger unit?
>
> Arnd
>

This is an unfortunate legacy of our previous SDRAM controller (in the
CycloneV) which had ECC registers interspersed with registers other
drivers needed - thus the use of syscon.

In the Arria10 chip, the ECC registers are in their own partitioned
group but I kept the syscon to remain consistent with the Device Tree
bindings from the CycloneV family.

I've implemented your other suggestions. Thank you for reviewing!

Thor