[PATCH 0/3] EDAC: TI: add support for DRA7 and keystone EDAC

[PATCH 0/3] EDAC: TI: add support for DRA7 and keystone EDAC

[Tero Kristo]

Hi,

The EMIF controller on certain TI SoCs has ECC support capability.
This series adds support for this on dra7xx and keystone2 variants
of the SoCs. DTS nodes are only added for keystone2 devices, as these
can be tested with upstream right now. DRA7xx support will be added
once the specific boards that actually support ECC are posted upstream.

Tested the ECC support on keystone-k2g-evm against 4.14-rc1, injecting
1 and 2 bit errors from u-boot. Below log with 2 bit error injected at
the beginning of the SDRAM.

<snip>
root at k2g-evm:~# insmod /lib/modules/4.14.0-rc1-00003-gf5201bf/kernel/drivers/eda
c/ti_edac.ko
[   32.559025] EDAC MC0: Giving out device to module ti_edac controller ti,emif-
keystone: DEV 21010000.emif (INTERRUPT)
[   32.599088] EDAC MC0: 1 UE ti,emif-keystone on mc#0memory#0 (memory:0 page:0x
0 offset:0x0 grain:4 - 2B)
<snip>

Boot tested on keystone-k2e/l/hk also just to make sure there are no
issues (the driver is not enabled in keystone config by default either.)

-Tero

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/3] Documentation: dt: memory: ti-emif: add edac support under emif

[Tero Kristo]

Certain revisions of the TI EMIF IP contain ECC support in them. Reflect
this in the DT binding.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
---
 .../devicetree/bindings/memory-controllers/ti/emif.txt   | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt b/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
index 0db6047..f56a347 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
@@ -3,12 +3,16 @@
 EMIF - External Memory Interface - is an SDRAM controller used in
 TI SoCs. EMIF supports, based on the IP revision, one or more of
 DDR2/DDR3/LPDDR2 protocols. This binding describes a given instance
-of the EMIF IP and memory parts attached to it.
+of the EMIF IP and memory parts attached to it. Certain revisions
+of the EMIF IP controller also contain optional ECC support, which
+corrects one bit errors and detects two bit errors.
 
 Required properties:
 - compatible	: Should be of the form "ti,emif-<ip-rev>" where <ip-rev>
   is the IP revision of the specific EMIF instance.
 		  For am437x should be ti,emif-am4372.
+		  For dra7xx should be ti,emif-dra7xx.
+		  For k2x family, should be ti,emif-keystone.
 
 - phy-type	: <u32> indicating the DDR phy type. Following are the
   allowed values
@@ -42,6 +46,10 @@ Optional properties:
 - hw-caps-temp-alert	: Have this property if the controller
   has capability for generating SDRAM temperature alerts
 
+- interrupts		: A list of interrupt specifiers for memory
+  controller interrupts, if available. Required for EMIF instances
+  that support ECC.
+
 Example:
 
 emif1: emif at 0x4c000000 {
@@ -54,3 +62,9 @@ emif1: emif at 0x4c000000 {
 	hw-caps-ll-interface;
 	hw-caps-temp-alert;
 };
+
+emif1: emif at 4c000000 {
+	compatible = "ti,emif-dra7";
+	reg = <0x4c000000 0x200>;
+	interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+};
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
correct one bit errors and detect two bit errors. Add EDAC driver for this
feature which plugs into the generic kernel EDAC framework.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
v2:
 * fixed checkpatch errors
 * improved error cleanup for failed probe
 * converted to use edac_printk
 * added MAINTAINERS entry
 * dropped mutex and used atomic counter for ID
 * ... and some minor cosmetic changes

 MAINTAINERS            |   6 +
 drivers/edac/Kconfig   |   7 ++
 drivers/edac/Makefile  |   1 +
 drivers/edac/ti_edac.c | 314 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 328 insertions(+)
 create mode 100644 drivers/edac/ti_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2281af4..c46ed26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5028,6 +5028,12 @@ L:	linux-edac at vger.kernel.org
 S:	Maintained
 F:	drivers/edac/skx_edac.c
 
+EDAC-TI
+M:	Tero Kristo <t-kristo@ti.com>
+L:	linux-edac at vger.kernel.org
+S:	Maintained
+F:	drivers/edac/ti_edac.c
+
 EDIROL UA-101/UA-1000 DRIVER
 M:	Clemens Ladisch <clemens@ladisch.de>
 L:	alsa-devel at alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 96afb2a..3c40170 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -457,4 +457,11 @@ config EDAC_XGENE
 	  Support for error detection and correction on the
 	  APM X-Gene family of SOCs.
 
+config EDAC_TI
+	tristate "Texas Instruments DDR3 ECC Controller"
+	depends on ARCH_KEYSTONE || SOC_DRA7XX
+	help
+	  Support for error detection and correction on the
+          TI SoCs.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 0fd9ffa..b54912e 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_THUNDERX)		+= thunderx_edac.o
 obj-$(CONFIG_EDAC_ALTERA)		+= altera_edac.o
 obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
 obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
+obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
diff --git a/drivers/edac/ti_edac.c b/drivers/edac/ti_edac.c
new file mode 100644
index 0000000..c147853
--- /dev/null
+++ b/drivers/edac/ti_edac.c
@@ -0,0 +1,314 @@
+/*
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Texas Instruments DDR3 ECC error correction and detection driver
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/init.h>
+#include <linux/edac.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+
+#include "edac_module.h"
+
+/* EMIF controller registers */
+#define EMIF_SDRAM_CONFIG		0x008
+#define EMIF_IRQ_STATUS			0x0ac
+#define EMIF_IRQ_ENABLE_SET		0x0b4
+#define EMIF_ECC_CTRL			0x110
+#define EMIF_1B_ECC_ERR_CNT		0x130
+#define EMIF_1B_ECC_ERR_THRSH		0x134
+#define EMIF_1B_ECC_ERR_ADDR_LOG	0x13c
+#define EMIF_2B_ECC_ERR_ADDR_LOG	0x140
+
+/* Bit definitions for EMIF_SDRAM_CONFIG */
+#define SDRAM_TYPE_SHIFT		29
+#define SDRAM_TYPE_MASK			GENMASK(31, 29)
+#define SDRAM_TYPE_DDR3			(3 << SDRAM_TYPE_SHIFT)
+#define SDRAM_TYPE_DDR2			(2 << SDRAM_TYPE_SHIFT)
+#define SDRAM_NARROW_MODE_MASK		GENMASK(15, 14)
+#define SDRAM_K2_NARROW_MODE_SHIFT	12
+#define SDRAM_K2_NARROW_MODE_MASK	GENMASK(13, 12)
+#define SDRAM_ROWSIZE_SHIFT		7
+#define SDRAM_ROWSIZE_MASK		GENMASK(9, 7)
+#define SDRAM_IBANK_SHIFT		4
+#define SDRAM_IBANK_MASK		GENMASK(6, 4)
+#define SDRAM_K2_IBANK_SHIFT		5
+#define SDRAM_K2_IBANK_MASK		GENMASK(6, 5)
+#define SDRAM_K2_EBANK_SHIFT		3
+#define SDRAM_K2_EBANK_MASK		BIT(SDRAM_K2_EBANK_SHIFT)
+#define SDRAM_PAGESIZE_SHIFT		0
+#define SDRAM_PAGESIZE_MASK		GENMASK(2, 0)
+#define SDRAM_K2_PAGESIZE_SHIFT		0
+#define SDRAM_K2_PAGESIZE_MASK		GENMASK(1, 0)
+
+#define EMIF_1B_ECC_ERR_THRSH_SHIFT	24
+
+/* IRQ bit definitions */
+#define EMIF_1B_ECC_ERR			BIT(5)
+#define EMIF_2B_ECC_ERR			BIT(4)
+#define EMIF_WR_ECC_ERR			BIT(3)
+#define EMIF_SYS_ERR			BIT(0)
+/* Bit 31 enables ECC and 28 enables RMW */
+#define ECC_ENABLED			(BIT(31) | BIT(28))
+
+#define EDAC_MOD_NAME			"ti-emif-edac"
+
+enum {
+	EMIF_TYPE_DRA7,
+	EMIF_TYPE_K2
+};
+
+struct ti_edac {
+	void __iomem *reg;
+};
+
+static atomic_t edac_id = ATOMIC_INIT(0);
+
+static u32 ti_edac_readl(struct ti_edac *edac, u16 offset)
+{
+	return readl_relaxed(edac->reg + offset);
+}
+
+static void ti_edac_writel(struct ti_edac *edac, u32 val, u16 offset)
+{
+	writel_relaxed(val, edac->reg + offset);
+}
+
+static irqreturn_t ti_edac_isr(int irq, void *data)
+{
+	struct mem_ctl_info *mci = data;
+	struct ti_edac *edac = mci->pvt_info;
+	u32 irq_status;
+	u32 err_addr;
+	int err_count;
+
+	irq_status = ti_edac_readl(edac, EMIF_IRQ_STATUS);
+
+	if (irq_status & EMIF_1B_ECC_ERR) {
+		err_addr = ti_edac_readl(edac, EMIF_1B_ECC_ERR_ADDR_LOG);
+		err_count = ti_edac_readl(edac, EMIF_1B_ECC_ERR_CNT);
+		ti_edac_writel(edac, err_count, EMIF_1B_ECC_ERR_CNT);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, -1, 0, 0, 0,
+				     mci->ctl_name, "1B");
+	}
+
+	if (irq_status & EMIF_2B_ECC_ERR) {
+		err_addr = ti_edac_readl(edac, EMIF_2B_ECC_ERR_ADDR_LOG);
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, -1, 0, 0, 0,
+				     mci->ctl_name, "2B");
+	}
+
+	if (irq_status & EMIF_WR_ECC_ERR)
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     0, 0, -1, 0, 0, 0,
+				     mci->ctl_name, "WR");
+
+	ti_edac_writel(edac, irq_status, EMIF_IRQ_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type)
+{
+	struct dimm_info *dimm;
+	struct ti_edac *edac = mci->pvt_info;
+	int bits;
+	u32 val;
+	u32 memsize;
+
+	dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0);
+
+	val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG);
+
+	if (type == EMIF_TYPE_DRA7) {
+		bits = ((val & SDRAM_PAGESIZE_MASK) >> SDRAM_PAGESIZE_SHIFT) + 8;
+		bits += ((val & SDRAM_ROWSIZE_MASK) >> SDRAM_ROWSIZE_SHIFT) + 9;
+		bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT;
+
+		if (val & SDRAM_NARROW_MODE_MASK) {
+			bits++;
+			dimm->dtype = DEV_X16;
+		} else {
+			bits += 2;
+			dimm->dtype = DEV_X32;
+		}
+	} else {
+		bits = 16;
+		bits += ((val & SDRAM_K2_PAGESIZE_MASK) >>
+			SDRAM_K2_PAGESIZE_SHIFT) + 8;
+		bits += (val & SDRAM_K2_IBANK_MASK) >> SDRAM_K2_IBANK_SHIFT;
+		bits += (val & SDRAM_K2_EBANK_MASK) >> SDRAM_K2_EBANK_SHIFT;
+
+		val = (val & SDRAM_K2_NARROW_MODE_MASK) >>
+			SDRAM_K2_NARROW_MODE_SHIFT;
+		switch (val) {
+		case 0:
+			bits += 3;
+			dimm->dtype = DEV_X64;
+			break;
+		case 1:
+			bits += 2;
+			dimm->dtype = DEV_X32;
+			break;
+		case 2:
+			bits++;
+			dimm->dtype = DEV_X16;
+			break;
+		}
+	}
+
+	memsize = 1 << bits;
+
+	dimm->nr_pages = memsize >> PAGE_SHIFT;
+	dimm->grain = 4;
+	if ((val & SDRAM_TYPE_MASK) == SDRAM_TYPE_DDR2)
+		dimm->mtype = MEM_DDR2;
+	else
+		dimm->mtype = MEM_DDR3;
+
+	val = ti_edac_readl(edac, EMIF_ECC_CTRL);
+	if (val & ECC_ENABLED)
+		dimm->edac_mode = EDAC_SECDED;
+	else
+		dimm->edac_mode = EDAC_NONE;
+}
+
+static const struct of_device_id ti_edac_of_match[] = {
+	{ .compatible = "ti,emif-keystone", .data = (void *)EMIF_TYPE_K2 },
+	{ .compatible = "ti,emif-dra7xx", .data = (void *)EMIF_TYPE_DRA7 },
+	{},
+};
+
+static int ti_edac_probe(struct platform_device *pdev)
+{
+	int error_irq = 0, ret = -ENODEV;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *reg;
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[1];
+	const struct of_device_id *id;
+	struct ti_edac *edac;
+	int my_id;
+
+	id = of_match_device(ti_edac_of_match, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg)) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "EMIF controller regs not defined\n");
+		return PTR_ERR(reg);
+	}
+
+	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
+	layers[0].size = 1;
+
+	/* Allocate ID number for our EMIF controller */
+	my_id = atomic_inc_return(&edac_id) - 1;
+
+	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
+	if (!mci)
+		return -ENOMEM;
+
+	mci->pdev = &pdev->dev;
+	edac = mci->pvt_info;
+	edac->reg = reg;
+	platform_set_drvdata(pdev, mci);
+
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
+	mci->mod_name = EDAC_MOD_NAME;
+	mci->ctl_name = id->compatible;
+	mci->dev_name = dev_name(&pdev->dev);
+
+	/* Setup memory layout */
+	ti_edac_setup_dimm(mci, (u32)(id->data));
+
+	ret = edac_mc_add_mc(mci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "Failed to register mci: %d.\n", ret);
+		goto err;
+	}
+
+	/* add EMIF ECC error handler */
+	error_irq = platform_get_irq(pdev, 0);
+	if (!error_irq) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "EMIF irq number not defined.\n");
+		goto err2;
+	}
+
+	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
+			       "emif-edac-irq", mci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "request_irq fail for EMIF EDAC irq\n");
+		goto err2;
+	}
+
+	/* Generate an interrupt with each 1b error */
+	ti_edac_writel(edac, 1 << EMIF_1B_ECC_ERR_THRSH_SHIFT,
+		       EMIF_1B_ECC_ERR_THRSH);
+
+	/* Enable interrupts */
+	ti_edac_writel(edac,
+		       EMIF_1B_ECC_ERR | EMIF_2B_ECC_ERR | EMIF_WR_ECC_ERR,
+		       EMIF_IRQ_ENABLE_SET);
+
+	return 0;
+
+err2:
+	edac_mc_del_mc(&pdev->dev);
+err:
+	edac_mc_free(mci);
+	return ret;
+}
+
+static int ti_edac_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+static struct platform_driver ti_edac_driver = {
+	.probe = ti_edac_probe,
+	.remove = ti_edac_remove,
+	.driver = {
+		   .name = EDAC_MOD_NAME,
+		   .of_match_table = ti_edac_of_match,
+	},
+};
+
+module_platform_driver(ti_edac_driver);
+
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("EDAC Driver for Texas Instruments DDR3 MC");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Borislav Petkov]

On Fri, Nov 10, 2017 at 10:25:37AM +0200, Tero Kristo wrote:
> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
> correct one bit errors and detect two bit errors. Add EDAC driver for this
> feature which plugs into the generic kernel EDAC framework.

...

> +static int ti_edac_probe(struct platform_device *pdev)
> +{
> +	int error_irq = 0, ret = -ENODEV;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	void __iomem *reg;
> +	struct mem_ctl_info *mci;
> +	struct edac_mc_layer layers[1];
> +	const struct of_device_id *id;
> +	struct ti_edac *edac;
> +	int my_id;
> +
> +	id = of_match_device(ti_edac_of_match, &pdev->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	reg = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(reg)) {
> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
> +			    "EMIF controller regs not defined\n");
> +		return PTR_ERR(reg);
> +	}
> +
> +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
> +	layers[0].size = 1;
> +
> +	/* Allocate ID number for our EMIF controller */
> +	my_id = atomic_inc_return(&edac_id) - 1;

You can do ATOMIC_INIT(-1) and then you don't need that silly - 1.

But there's something else wrong with this ID generation:

> +
> +	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
> +	if (!mci)
> +		return -ENOMEM;

<--- if you return here due to error, the next one which succeeds will
get the next number and you'd have a hole in the numbering and wonder
where did instance 0 go.

So I'm wondering if instead you could derive the ID from some controller
registers which are specific to the instance. Or some hardware detail
which is instance-specific.

In my driver, for example, the 0th instance has PCI device functions
00:18.x which is node 0, then node 1 has 00:19.x and so on, and this is
a stable numbering. If you could do that for your hw you'll be able to
pinpoint which instance and which DIMMs we're talking about reliably.

> +
> +	mci->pdev = &pdev->dev;
> +	edac = mci->pvt_info;
> +	edac->reg = reg;
> +	platform_set_drvdata(pdev, mci);
> +
> +	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
> +	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
> +	mci->mod_name = EDAC_MOD_NAME;
> +	mci->ctl_name = id->compatible;
> +	mci->dev_name = dev_name(&pdev->dev);
> +
> +	/* Setup memory layout */
> +	ti_edac_setup_dimm(mci, (u32)(id->data));
> +
> +	ret = edac_mc_add_mc(mci);

Do that...

> +	if (ret) {
> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
> +			    "Failed to register mci: %d.\n", ret);
> +		goto err;
> +	}
> +
> +	/* add EMIF ECC error handler */
> +	error_irq = platform_get_irq(pdev, 0);
> +	if (!error_irq) {
> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
> +			    "EMIF irq number not defined.\n");
> +		goto err2;
> +	}
> +
> +	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
> +			       "emif-edac-irq", mci);
> +	if (ret) {
> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
> +			    "request_irq fail for EMIF EDAC irq\n");
> +		goto err2;
> +	}

... here, after you've requested IRQs successfully and you can save
yourself the err2 error path.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCHv2 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

On 11/11/17 12:46, Borislav Petkov wrote:
> On Fri, Nov 10, 2017 at 10:25:37AM +0200, Tero Kristo wrote:
>> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
>> correct one bit errors and detect two bit errors. Add EDAC driver for this
>> feature which plugs into the generic kernel EDAC framework.
> 
> ...
> 
>> +static int ti_edac_probe(struct platform_device *pdev)
>> +{
>> +	int error_irq = 0, ret = -ENODEV;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	void __iomem *reg;
>> +	struct mem_ctl_info *mci;
>> +	struct edac_mc_layer layers[1];
>> +	const struct of_device_id *id;
>> +	struct ti_edac *edac;
>> +	int my_id;
>> +
>> +	id = of_match_device(ti_edac_of_match, &pdev->dev);
>> +	if (!id)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	reg = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(reg)) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "EMIF controller regs not defined\n");
>> +		return PTR_ERR(reg);
>> +	}
>> +
>> +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
>> +	layers[0].size = 1;
>> +
>> +	/* Allocate ID number for our EMIF controller */
>> +	my_id = atomic_inc_return(&edac_id) - 1;
> 
> You can do ATOMIC_INIT(-1) and then you don't need that silly - 1.
> 
> But there's something else wrong with this ID generation:
> 
>> +
>> +	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
>> +	if (!mci)
>> +		return -ENOMEM;
> 
> <--- if you return here due to error, the next one which succeeds will
> get the next number and you'd have a hole in the numbering and wonder
> where did instance 0 go.

Yeah, that definitely can happen.

> 
> So I'm wondering if instead you could derive the ID from some controller
> registers which are specific to the instance. Or some hardware detail
> which is instance-specific.
> 
> In my driver, for example, the 0th instance has PCI device functions
> 00:18.x which is node 0, then node 1 has 00:19.x and so on, and this is
> a stable numbering. If you could do that for your hw you'll be able to
> pinpoint which instance and which DIMMs we're talking about reliably.

The only reliable way to determine the instance is to sort them by 
physical address space they occupy. This will require me to lookup all 
nodes that have same compatible string, and check their regs. This 
should probably be okay seeing we have only two EMIF instances at most.

I'll check how to do this most reliably.


> 
>> +
>> +	mci->pdev = &pdev->dev;
>> +	edac = mci->pvt_info;
>> +	edac->reg = reg;
>> +	platform_set_drvdata(pdev, mci);
>> +
>> +	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
>> +	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
>> +	mci->mod_name = EDAC_MOD_NAME;
>> +	mci->ctl_name = id->compatible;
>> +	mci->dev_name = dev_name(&pdev->dev);
>> +
>> +	/* Setup memory layout */
>> +	ti_edac_setup_dimm(mci, (u32)(id->data));
>> +
>> +	ret = edac_mc_add_mc(mci);
> 
> Do that...
> 
>> +	if (ret) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "Failed to register mci: %d.\n", ret);
>> +		goto err;
>> +	}
>> +
>> +	/* add EMIF ECC error handler */
>> +	error_irq = platform_get_irq(pdev, 0);
>> +	if (!error_irq) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "EMIF irq number not defined.\n");
>> +		goto err2;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
>> +			       "emif-edac-irq", mci);
>> +	if (ret) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "request_irq fail for EMIF EDAC irq\n");
>> +		goto err2;
>> +	}
> 
> ... here, after you've requested IRQs successfully and you can save
> yourself the err2 error path.

Ok.

> 
> Thx.
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/3] Documentation: dt: memory: ti-emif: add edac support under emif

[Tero Kristo]

Rob,

Any comments on this? I guess the series is only pending an ack on this one.

(Added devicetree ML to delivery, it seems I missed this on the original 
post.)

-Tero

On 07/11/17 22:38, Tero Kristo wrote:
> Certain revisions of the TI EMIF IP contain ECC support in them. Reflect
> this in the DT binding.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> ---
>   .../devicetree/bindings/memory-controllers/ti/emif.txt   | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt b/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
> index 0db6047..f56a347 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
> @@ -3,12 +3,16 @@
>   EMIF - External Memory Interface - is an SDRAM controller used in
>   TI SoCs. EMIF supports, based on the IP revision, one or more of
>   DDR2/DDR3/LPDDR2 protocols. This binding describes a given instance
> -of the EMIF IP and memory parts attached to it.
> +of the EMIF IP and memory parts attached to it. Certain revisions
> +of the EMIF IP controller also contain optional ECC support, which
> +corrects one bit errors and detects two bit errors.
>   
>   Required properties:
>   - compatible	: Should be of the form "ti,emif-<ip-rev>" where <ip-rev>
>     is the IP revision of the specific EMIF instance.
>   		  For am437x should be ti,emif-am4372.
> +		  For dra7xx should be ti,emif-dra7xx.
> +		  For k2x family, should be ti,emif-keystone.
>   
>   - phy-type	: <u32> indicating the DDR phy type. Following are the
>     allowed values
> @@ -42,6 +46,10 @@ Optional properties:
>   - hw-caps-temp-alert	: Have this property if the controller
>     has capability for generating SDRAM temperature alerts
>   
> +- interrupts		: A list of interrupt specifiers for memory
> +  controller interrupts, if available. Required for EMIF instances
> +  that support ECC.
> +
>   Example:
>   
>   emif1: emif at 0x4c000000 {
> @@ -54,3 +62,9 @@ emif1: emif at 0x4c000000 {
>   	hw-caps-ll-interface;
>   	hw-caps-temp-alert;
>   };
> +
> +emif1: emif at 4c000000 {
> +	compatible = "ti,emif-dra7";
> +	reg = <0x4c000000 0x200>;
> +	interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
> +};
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
correct one bit errors and detect two bit errors. Add EDAC driver for this
feature which plugs into the generic kernel EDAC framework.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Cc: Tony Lindgren <tony@atomide.com>
---
 drivers/edac/Kconfig   |   7 ++
 drivers/edac/Makefile  |   1 +
 drivers/edac/ti_edac.c | 306 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 314 insertions(+)
 create mode 100644 drivers/edac/ti_edac.c

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 96afb2a..54f0184 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -457,4 +457,11 @@ config EDAC_XGENE
 	  Support for error detection and correction on the
 	  APM X-Gene family of SOCs.
 
+config EDAC_TI
+	tristate "Texas Instruments DDR3 ECC handler"
+	depends on ARCH_KEYSTONE || SOC_DRA7XX
+	help
+	  Support for error detection and correction on the
+          TI SoCs.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 0fd9ffa..b54912e 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_THUNDERX)		+= thunderx_edac.o
 obj-$(CONFIG_EDAC_ALTERA)		+= altera_edac.o
 obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
 obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
+obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
diff --git a/drivers/edac/ti_edac.c b/drivers/edac/ti_edac.c
new file mode 100644
index 0000000..54bacf3
--- /dev/null
+++ b/drivers/edac/ti_edac.c
@@ -0,0 +1,306 @@
+/*
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Texas Instruments DDR3 ECC error correction and detection driver
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/init.h>
+#include <linux/edac.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+
+#include "edac_module.h"
+
+/* EMIF controller registers */
+#define EMIF_SDRAM_CONFIG		0x008
+#define EMIF_IRQ_STATUS			0x0ac
+#define EMIF_IRQ_ENABLE_SET		0x0b4
+#define EMIF_ECC_CTRL			0x110
+#define EMIF_1B_ECC_ERR_CNT		0x130
+#define EMIF_1B_ECC_ERR_THRSH		0x134
+#define EMIF_1B_ECC_ERR_ADDR_LOG	0x13c
+#define EMIF_2B_ECC_ERR_ADDR_LOG	0x140
+
+/* Bit definitions for EMIF_SDRAM_CONFIG */
+#define SDRAM_TYPE_SHIFT		29
+#define SDRAM_TYPE_MASK			GENMASK(31,29)
+#define SDRAM_TYPE_DDR3			(3 << SDRAM_TYPE_SHIFT)
+#define SDRAM_TYPE_DDR2			(2 << SDRAM_TYPE_SHIFT)
+#define SDRAM_NARROW_MODE_MASK		GENMASK(15,14)
+#define SDRAM_K2_NARROW_MODE_SHIFT	12
+#define SDRAM_K2_NARROW_MODE_MASK	GENMASK(13,12)
+#define SDRAM_ROWSIZE_SHIFT		7
+#define SDRAM_ROWSIZE_MASK		GENMASK(9,7)
+#define SDRAM_IBANK_SHIFT		4
+#define SDRAM_IBANK_MASK		GENMASK(6,4)
+#define SDRAM_K2_IBANK_SHIFT		5
+#define SDRAM_K2_IBANK_MASK		GENMASK(6,5)
+#define SDRAM_K2_EBANK_SHIFT		3
+#define SDRAM_K2_EBANK_MASK		BIT(SDRAM_K2_EBANK_SHIFT)
+#define SDRAM_PAGESIZE_SHIFT		0
+#define SDRAM_PAGESIZE_MASK		GENMASK(2,0)
+#define SDRAM_K2_PAGESIZE_SHIFT		0
+#define SDRAM_K2_PAGESIZE_MASK		GENMASK(1,0)
+
+#define EMIF_1B_ECC_ERR_THRSH_SHIFT	24
+
+/* IRQ bit definitions */
+#define EMIF_1B_ECC_ERR			BIT(5)
+#define EMIF_2B_ECC_ERR			BIT(4)
+#define EMIF_WR_ECC_ERR			BIT(3)
+#define EMIF_SYS_ERR			BIT(0)
+/* Bit 31 enables ECC and 28 enables RMW */
+#define ECC_ENABLED			(BIT(31) | BIT(28))
+
+enum {
+	EMIF_TYPE_DRA7,
+	EMIF_TYPE_K2
+};
+
+struct ti_edac {
+	void __iomem *reg;
+};
+
+static DEFINE_MUTEX(ti_edac_lock);
+
+static u32 ti_edac_readl(struct ti_edac *edac, u16 offset)
+{
+	return readl_relaxed(edac->reg + offset);
+}
+
+static void ti_edac_writel(struct ti_edac *edac, u32 val, u16 offset)
+{
+	writel_relaxed(val, edac->reg + offset);
+}
+
+static irqreturn_t ti_edac_isr(int irq, void *data)
+{
+	struct mem_ctl_info *mci = data;
+	struct ti_edac *edac = mci->pvt_info;
+	u32 irq_status;
+	u32 err_addr;
+	int err_count;
+
+	irq_status = ti_edac_readl(edac, EMIF_IRQ_STATUS);
+
+	if (irq_status & EMIF_1B_ECC_ERR) {
+		err_addr = ti_edac_readl(edac, EMIF_1B_ECC_ERR_ADDR_LOG);
+		err_count = ti_edac_readl(edac, EMIF_1B_ECC_ERR_CNT);
+		ti_edac_writel(edac, err_count, EMIF_1B_ECC_ERR_CNT);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, -1, 0, 0, 0,
+				     mci->ctl_name, "1B");
+	}
+
+	if (irq_status & EMIF_2B_ECC_ERR) {
+		err_addr = ti_edac_readl(edac, EMIF_2B_ECC_ERR_ADDR_LOG);
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, -1, 0, 0, 0,
+				     mci->ctl_name, "2B");
+	}
+
+	if (irq_status & EMIF_WR_ECC_ERR)
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     0, 0, -1, 0, 0, 0,
+				     mci->ctl_name, "WR");
+
+	ti_edac_writel(edac, irq_status, EMIF_IRQ_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type)
+{
+	struct dimm_info *dimm;
+	struct ti_edac *edac = mci->pvt_info;
+	int bits;
+	u32 val;
+	u32 memsize;
+
+	dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0);
+
+	val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG);
+
+	if (type == EMIF_TYPE_DRA7) {
+		bits = ((val & SDRAM_PAGESIZE_MASK) >>
+			SDRAM_PAGESIZE_SHIFT) + 8;
+		bits += ((val & SDRAM_ROWSIZE_MASK) >>
+			SDRAM_ROWSIZE_SHIFT) + 9;
+		bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT;
+
+		if (val & SDRAM_NARROW_MODE_MASK) {
+			bits++;
+			dimm->dtype = DEV_X16;
+		} else {
+			bits += 2;
+			dimm->dtype = DEV_X32;
+		}
+	} else {
+		bits = 16;
+		bits += ((val & SDRAM_K2_PAGESIZE_MASK) >>
+			SDRAM_K2_PAGESIZE_SHIFT) + 8;
+		bits += (val & SDRAM_K2_IBANK_MASK) >> SDRAM_K2_IBANK_SHIFT;
+		bits += (val & SDRAM_K2_EBANK_MASK) >> SDRAM_K2_EBANK_SHIFT;
+
+		val = (val & SDRAM_K2_NARROW_MODE_MASK) >>
+			SDRAM_K2_NARROW_MODE_SHIFT;
+		switch (val) {
+		case 0:
+			bits += 3;
+			dimm->dtype = DEV_X64;
+			break;
+		case 1:
+			bits += 2;
+			dimm->dtype = DEV_X32;
+			break;
+		case 2:
+			bits ++;
+			dimm->dtype = DEV_X16;
+			break;
+		}
+	}
+
+	memsize = 1 << bits;
+
+	dimm->nr_pages = memsize >> PAGE_SHIFT;
+	dimm->grain = 4;
+	if ((val & SDRAM_TYPE_MASK) == SDRAM_TYPE_DDR2)
+		dimm->mtype = MEM_DDR2;
+	else
+		dimm->mtype = MEM_DDR3;
+
+	val = ti_edac_readl(edac, EMIF_ECC_CTRL);
+	if (val & ECC_ENABLED)
+		dimm->edac_mode = EDAC_SECDED;
+	else
+		dimm->edac_mode = EDAC_NONE;
+}
+
+static const struct of_device_id ti_edac_of_match[] = {
+	{ .compatible = "ti,emif-keystone", .data = (void *)EMIF_TYPE_K2 },
+	{ .compatible = "ti,emif-dra7xx", .data = (void *)EMIF_TYPE_DRA7 },
+	{},
+};
+
+static int ti_edac_probe(struct platform_device *pdev)
+{
+	int error_irq = 0, ret = -ENODEV;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *reg;
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[1];
+	const struct of_device_id *id;
+	struct ti_edac *edac;
+	static int edac_id;
+	int my_id;
+
+	id = of_match_device(ti_edac_of_match, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg)) {
+		dev_err(dev, "DDR3 controller regs not defined\n");
+		return PTR_ERR(reg);
+	}
+
+	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
+	layers[0].size = 1;
+
+	/* Allocate ID number for our EMIF controller */
+	mutex_lock(&ti_edac_lock);
+	my_id = edac_id++;
+	mutex_unlock(&ti_edac_lock);
+
+	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
+	if (!mci)
+		return -ENOMEM;
+
+	mci->pdev = &pdev->dev;
+	edac = mci->pvt_info;
+	edac->reg = reg;
+	platform_set_drvdata(pdev, mci);
+
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
+	mci->mod_name = pdev->dev.driver->name;
+	mci->ctl_name = id->compatible;
+	mci->dev_name = dev_name(&pdev->dev);
+
+	/* Setup memory layout */
+	ti_edac_setup_dimm(mci, (u32)(id->data));
+
+	if (edac_mc_add_mc(mci)) {
+		pr_err("%s: Failed to register mci.\n", __func__);
+		return -ENOMEM;
+	}
+
+	/* add EMIF ECC error handler */
+	error_irq = platform_get_irq(pdev, 0);
+	if (!error_irq) {
+		dev_err(dev, "DDR3 EDAC irq number not defined\n");
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
+			       "ddr3-edac-irq", mci);
+	if (ret) {
+		dev_err(dev, "request_irq fail for DDR3 EDAC error irq\n");
+		return ret;
+	}
+
+	/* Generate an interrupt with each 1b error */
+	ti_edac_writel(edac, 1 << EMIF_1B_ECC_ERR_THRSH_SHIFT,
+		       EMIF_1B_ECC_ERR_THRSH);
+
+	/* Enable interrupts */
+	ti_edac_writel(edac,
+		       EMIF_1B_ECC_ERR | EMIF_2B_ECC_ERR | EMIF_WR_ECC_ERR,
+		       EMIF_IRQ_ENABLE_SET);
+
+	return ret;
+}
+
+static int ti_edac_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+static struct platform_driver ti_edac_driver = {
+	.probe = ti_edac_probe,
+	.remove = ti_edac_remove,
+	.driver = {
+		   .name = "ti_edac",
+		   .of_match_table = ti_edac_of_match,
+	},
+};
+
+module_platform_driver(ti_edac_driver);
+
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("EDAC Driver for Texas Instruments DDR3 MC");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Borislav Petkov]

On Tue, Nov 07, 2017 at 10:38:58PM +0200, Tero Kristo wrote:
> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
> correct one bit errors and detect two bit errors. Add EDAC driver for this
> feature which plugs into the generic kernel EDAC framework.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: Tony Lindgren <tony@atomide.com>

Please add yourself and whoever else wants to get CCed on bug reports
for this driver, to MAINTAINERS.

Also,

please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.

> ---
>  drivers/edac/Kconfig   |   7 ++
>  drivers/edac/Makefile  |   1 +
>  drivers/edac/ti_edac.c | 306 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 314 insertions(+)
>  create mode 100644 drivers/edac/ti_edac.c
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 96afb2a..54f0184 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -457,4 +457,11 @@ config EDAC_XGENE
>  	  Support for error detection and correction on the
>  	  APM X-Gene family of SOCs.
>  
> +config EDAC_TI
> +	tristate "Texas Instruments DDR3 ECC handler"

s/handler/Controller/

> +	depends on ARCH_KEYSTONE || SOC_DRA7XX
> +	help
> +	  Support for error detection and correction on the
> +          TI SoCs.
> +
>  endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 0fd9ffa..b54912e 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_THUNDERX)		+= thunderx_edac.o
>  obj-$(CONFIG_EDAC_ALTERA)		+= altera_edac.o
>  obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
>  obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
> +obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
> diff --git a/drivers/edac/ti_edac.c b/drivers/edac/ti_edac.c
> new file mode 100644
> index 0000000..54bacf3
> --- /dev/null
> +++ b/drivers/edac/ti_edac.c
> @@ -0,0 +1,306 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Texas Instruments DDR3 ECC error correction and detection driver
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/edac.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/module.h>
> +
> +#include "edac_module.h"
> +
> +/* EMIF controller registers */
> +#define EMIF_SDRAM_CONFIG		0x008
> +#define EMIF_IRQ_STATUS			0x0ac
> +#define EMIF_IRQ_ENABLE_SET		0x0b4
> +#define EMIF_ECC_CTRL			0x110
> +#define EMIF_1B_ECC_ERR_CNT		0x130
> +#define EMIF_1B_ECC_ERR_THRSH		0x134
> +#define EMIF_1B_ECC_ERR_ADDR_LOG	0x13c
> +#define EMIF_2B_ECC_ERR_ADDR_LOG	0x140
> +
> +/* Bit definitions for EMIF_SDRAM_CONFIG */
> +#define SDRAM_TYPE_SHIFT		29
> +#define SDRAM_TYPE_MASK			GENMASK(31,29)


ERROR: space required after that ',' (ctx:VxV)
#100: FILE: drivers/edac/ti_edac.c:41:
+#define SDRAM_TYPE_MASK                        GENMASK(31,29)
                                                          ^

ditto for the rest

> +#define SDRAM_TYPE_DDR3			(3 << SDRAM_TYPE_SHIFT)
> +#define SDRAM_TYPE_DDR2			(2 << SDRAM_TYPE_SHIFT)
> +#define SDRAM_NARROW_MODE_MASK		GENMASK(15,14)
> +#define SDRAM_K2_NARROW_MODE_SHIFT	12
> +#define SDRAM_K2_NARROW_MODE_MASK	GENMASK(13,12)
> +#define SDRAM_ROWSIZE_SHIFT		7
> +#define SDRAM_ROWSIZE_MASK		GENMASK(9,7)
> +#define SDRAM_IBANK_SHIFT		4
> +#define SDRAM_IBANK_MASK		GENMASK(6,4)
> +#define SDRAM_K2_IBANK_SHIFT		5
> +#define SDRAM_K2_IBANK_MASK		GENMASK(6,5)
> +#define SDRAM_K2_EBANK_SHIFT		3
> +#define SDRAM_K2_EBANK_MASK		BIT(SDRAM_K2_EBANK_SHIFT)
> +#define SDRAM_PAGESIZE_SHIFT		0
> +#define SDRAM_PAGESIZE_MASK		GENMASK(2,0)
> +#define SDRAM_K2_PAGESIZE_SHIFT		0
> +#define SDRAM_K2_PAGESIZE_MASK		GENMASK(1,0)
> +
> +#define EMIF_1B_ECC_ERR_THRSH_SHIFT	24
> +
> +/* IRQ bit definitions */
> +#define EMIF_1B_ECC_ERR			BIT(5)
> +#define EMIF_2B_ECC_ERR			BIT(4)
> +#define EMIF_WR_ECC_ERR			BIT(3)
> +#define EMIF_SYS_ERR			BIT(0)
> +/* Bit 31 enables ECC and 28 enables RMW */
> +#define ECC_ENABLED			(BIT(31) | BIT(28))

...

> +static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type)
> +{
> +	struct dimm_info *dimm;
> +	struct ti_edac *edac = mci->pvt_info;
> +	int bits;
> +	u32 val;
> +	u32 memsize;
> +
> +	dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0);
> +
> +	val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG);
> +
> +	if (type == EMIF_TYPE_DRA7) {
> +		bits = ((val & SDRAM_PAGESIZE_MASK) >>
> +			SDRAM_PAGESIZE_SHIFT) + 8;
> +		bits += ((val & SDRAM_ROWSIZE_MASK) >>
> +			SDRAM_ROWSIZE_SHIFT) + 9;

Bah, let those stick out - it is more readable this way:

                bits = ((val & SDRAM_PAGESIZE_MASK) >> SDRAM_PAGESIZE_SHIFT) + 8;
                bits += ((val & SDRAM_ROWSIZE_MASK) >> SDRAM_ROWSIZE_SHIFT) + 9;
                bits += (val & SDRAM_IBANK_MASK)    >> SDRAM_IBANK_SHIFT;



> +		bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT;
> +
> +		if (val & SDRAM_NARROW_MODE_MASK) {
> +			bits++;
> +			dimm->dtype = DEV_X16;
> +		} else {
> +			bits += 2;
> +			dimm->dtype = DEV_X32;
> +		}
> +	} else {
> +		bits = 16;
> +		bits += ((val & SDRAM_K2_PAGESIZE_MASK) >>
> +			SDRAM_K2_PAGESIZE_SHIFT) + 8;
> +		bits += (val & SDRAM_K2_IBANK_MASK) >> SDRAM_K2_IBANK_SHIFT;
> +		bits += (val & SDRAM_K2_EBANK_MASK) >> SDRAM_K2_EBANK_SHIFT;
> +
> +		val = (val & SDRAM_K2_NARROW_MODE_MASK) >>
> +			SDRAM_K2_NARROW_MODE_SHIFT;
> +		switch (val) {
> +		case 0:
> +			bits += 3;
> +			dimm->dtype = DEV_X64;
> +			break;
> +		case 1:
> +			bits += 2;
> +			dimm->dtype = DEV_X32;
> +			break;
> +		case 2:
> +			bits ++;

ERROR: space prohibited before that '++' (ctx:WxO)
#233: FILE: drivers/edac/ti_edac.c:174:
+                       bits ++;
                             ^


> +			dimm->dtype = DEV_X16;
> +			break;
> +		}
> +	}
> +
> +	memsize = 1 << bits;
> +
> +	dimm->nr_pages = memsize >> PAGE_SHIFT;
> +	dimm->grain = 4;
> +	if ((val & SDRAM_TYPE_MASK) == SDRAM_TYPE_DDR2)
> +		dimm->mtype = MEM_DDR2;
> +	else
> +		dimm->mtype = MEM_DDR3;
> +
> +	val = ti_edac_readl(edac, EMIF_ECC_CTRL);
> +	if (val & ECC_ENABLED)
> +		dimm->edac_mode = EDAC_SECDED;
> +	else
> +		dimm->edac_mode = EDAC_NONE;

So if ECC is not enabled, why do you even need to continue loading the
driver here and not return a retval which ti_edac_probe() propagates?

Also, you want to call that function much earlier in ti_edac_probe() so
that you can save yourself all the setup.

Or can you have instances which have ECC disabled and others which have
ECC enabled, on the same platform?

> +}
> +
> +static const struct of_device_id ti_edac_of_match[] = {
> +	{ .compatible = "ti,emif-keystone", .data = (void *)EMIF_TYPE_K2 },
> +	{ .compatible = "ti,emif-dra7xx", .data = (void *)EMIF_TYPE_DRA7 },
> +	{},
> +};
> +
> +static int ti_edac_probe(struct platform_device *pdev)
> +{
> +	int error_irq = 0, ret = -ENODEV;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	void __iomem *reg;
> +	struct mem_ctl_info *mci;
> +	struct edac_mc_layer layers[1];
> +	const struct of_device_id *id;
> +	struct ti_edac *edac;
> +	static int edac_id;
> +	int my_id;
> +
> +	id = of_match_device(ti_edac_of_match, &pdev->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	reg = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(reg)) {
> +		dev_err(dev, "DDR3 controller regs not defined\n");

We have edac_printk() et al macros for printing. Ditto for the remaining
calls.

> +		return PTR_ERR(reg);
> +	}
> +
> +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
> +	layers[0].size = 1;
> +
> +	/* Allocate ID number for our EMIF controller */
> +	mutex_lock(&ti_edac_lock);
> +	my_id = edac_id++;
> +	mutex_unlock(&ti_edac_lock);

That looks silly. Why not atomic_inc_return()?

> +	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
> +	if (!mci)
> +		return -ENOMEM;
> +
> +	mci->pdev = &pdev->dev;
> +	edac = mci->pvt_info;
> +	edac->reg = reg;
> +	platform_set_drvdata(pdev, mci);

...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

On 09/11/17 12:14, Borislav Petkov wrote:
> On Tue, Nov 07, 2017 at 10:38:58PM +0200, Tero Kristo wrote:
>> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
>> correct one bit errors and detect two bit errors. Add EDAC driver for this
>> feature which plugs into the generic kernel EDAC framework.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> Cc: Santosh Shilimkar <ssantosh@kernel.org>
>> Cc: Tony Lindgren <tony@atomide.com>
> 
> Please add yourself and whoever else wants to get CCed on bug reports
> for this driver, to MAINTAINERS.

Ok, will do that.

> 
> Also,
> 
> please integrate scripts/checkpatch.pl into your patch creation
> workflow. Some of the warnings/errors *actually* make sense.

Oh wow, I actually do that but forgot to do that for the very latest 
version of this patch for some reason. >.<

Sorry about that. Will fix all.

> 
>> ---
>>   drivers/edac/Kconfig   |   7 ++
>>   drivers/edac/Makefile  |   1 +
>>   drivers/edac/ti_edac.c | 306 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 314 insertions(+)
>>   create mode 100644 drivers/edac/ti_edac.c
>>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 96afb2a..54f0184 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -457,4 +457,11 @@ config EDAC_XGENE
>>   	  Support for error detection and correction on the
>>   	  APM X-Gene family of SOCs.
>>   
>> +config EDAC_TI
>> +	tristate "Texas Instruments DDR3 ECC handler"
> 
> s/handler/Controller/

Ok.

> 
>> +	depends on ARCH_KEYSTONE || SOC_DRA7XX
>> +	help
>> +	  Support for error detection and correction on the
>> +          TI SoCs.
>> +
>>   endif # EDAC
>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>> index 0fd9ffa..b54912e 100644
>> --- a/drivers/edac/Makefile
>> +++ b/drivers/edac/Makefile
>> @@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_THUNDERX)		+= thunderx_edac.o
>>   obj-$(CONFIG_EDAC_ALTERA)		+= altera_edac.o
>>   obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
>>   obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
>> +obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
>> diff --git a/drivers/edac/ti_edac.c b/drivers/edac/ti_edac.c
>> new file mode 100644
>> index 0000000..54bacf3
>> --- /dev/null
>> +++ b/drivers/edac/ti_edac.c
>> @@ -0,0 +1,306 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * Texas Instruments DDR3 ECC error correction and detection driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/edac.h>
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/module.h>
>> +
>> +#include "edac_module.h"
>> +
>> +/* EMIF controller registers */
>> +#define EMIF_SDRAM_CONFIG		0x008
>> +#define EMIF_IRQ_STATUS			0x0ac
>> +#define EMIF_IRQ_ENABLE_SET		0x0b4
>> +#define EMIF_ECC_CTRL			0x110
>> +#define EMIF_1B_ECC_ERR_CNT		0x130
>> +#define EMIF_1B_ECC_ERR_THRSH		0x134
>> +#define EMIF_1B_ECC_ERR_ADDR_LOG	0x13c
>> +#define EMIF_2B_ECC_ERR_ADDR_LOG	0x140
>> +
>> +/* Bit definitions for EMIF_SDRAM_CONFIG */
>> +#define SDRAM_TYPE_SHIFT		29
>> +#define SDRAM_TYPE_MASK			GENMASK(31,29)
> 
> 
> ERROR: space required after that ',' (ctx:VxV)
> #100: FILE: drivers/edac/ti_edac.c:41:
> +#define SDRAM_TYPE_MASK                        GENMASK(31,29)
>                                                            ^
> 
> ditto for the rest

Yea sorry about these, they slipped through.

> 
>> +#define SDRAM_TYPE_DDR3			(3 << SDRAM_TYPE_SHIFT)
>> +#define SDRAM_TYPE_DDR2			(2 << SDRAM_TYPE_SHIFT)
>> +#define SDRAM_NARROW_MODE_MASK		GENMASK(15,14)
>> +#define SDRAM_K2_NARROW_MODE_SHIFT	12
>> +#define SDRAM_K2_NARROW_MODE_MASK	GENMASK(13,12)
>> +#define SDRAM_ROWSIZE_SHIFT		7
>> +#define SDRAM_ROWSIZE_MASK		GENMASK(9,7)
>> +#define SDRAM_IBANK_SHIFT		4
>> +#define SDRAM_IBANK_MASK		GENMASK(6,4)
>> +#define SDRAM_K2_IBANK_SHIFT		5
>> +#define SDRAM_K2_IBANK_MASK		GENMASK(6,5)
>> +#define SDRAM_K2_EBANK_SHIFT		3
>> +#define SDRAM_K2_EBANK_MASK		BIT(SDRAM_K2_EBANK_SHIFT)
>> +#define SDRAM_PAGESIZE_SHIFT		0
>> +#define SDRAM_PAGESIZE_MASK		GENMASK(2,0)
>> +#define SDRAM_K2_PAGESIZE_SHIFT		0
>> +#define SDRAM_K2_PAGESIZE_MASK		GENMASK(1,0)
>> +
>> +#define EMIF_1B_ECC_ERR_THRSH_SHIFT	24
>> +
>> +/* IRQ bit definitions */
>> +#define EMIF_1B_ECC_ERR			BIT(5)
>> +#define EMIF_2B_ECC_ERR			BIT(4)
>> +#define EMIF_WR_ECC_ERR			BIT(3)
>> +#define EMIF_SYS_ERR			BIT(0)
>> +/* Bit 31 enables ECC and 28 enables RMW */
>> +#define ECC_ENABLED			(BIT(31) | BIT(28))
> 
> ...
> 
>> +static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type)
>> +{
>> +	struct dimm_info *dimm;
>> +	struct ti_edac *edac = mci->pvt_info;
>> +	int bits;
>> +	u32 val;
>> +	u32 memsize;
>> +
>> +	dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0);
>> +
>> +	val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG);
>> +
>> +	if (type == EMIF_TYPE_DRA7) {
>> +		bits = ((val & SDRAM_PAGESIZE_MASK) >>
>> +			SDRAM_PAGESIZE_SHIFT) + 8;
>> +		bits += ((val & SDRAM_ROWSIZE_MASK) >>
>> +			SDRAM_ROWSIZE_SHIFT) + 9;
> 
> Bah, let those stick out - it is more readable this way:
> 
>                  bits = ((val & SDRAM_PAGESIZE_MASK) >> SDRAM_PAGESIZE_SHIFT) + 8;
>                  bits += ((val & SDRAM_ROWSIZE_MASK) >> SDRAM_ROWSIZE_SHIFT) + 9;
>                  bits += (val & SDRAM_IBANK_MASK)    >> SDRAM_IBANK_SHIFT;

Ok.

> 
> 
> 
>> +		bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT;
>> +
>> +		if (val & SDRAM_NARROW_MODE_MASK) {
>> +			bits++;
>> +			dimm->dtype = DEV_X16;
>> +		} else {
>> +			bits += 2;
>> +			dimm->dtype = DEV_X32;
>> +		}
>> +	} else {
>> +		bits = 16;
>> +		bits += ((val & SDRAM_K2_PAGESIZE_MASK) >>
>> +			SDRAM_K2_PAGESIZE_SHIFT) + 8;
>> +		bits += (val & SDRAM_K2_IBANK_MASK) >> SDRAM_K2_IBANK_SHIFT;
>> +		bits += (val & SDRAM_K2_EBANK_MASK) >> SDRAM_K2_EBANK_SHIFT;
>> +
>> +		val = (val & SDRAM_K2_NARROW_MODE_MASK) >>
>> +			SDRAM_K2_NARROW_MODE_SHIFT;
>> +		switch (val) {
>> +		case 0:
>> +			bits += 3;
>> +			dimm->dtype = DEV_X64;
>> +			break;
>> +		case 1:
>> +			bits += 2;
>> +			dimm->dtype = DEV_X32;
>> +			break;
>> +		case 2:
>> +			bits ++;
> 
> ERROR: space prohibited before that '++' (ctx:WxO)
> #233: FILE: drivers/edac/ti_edac.c:174:
> +                       bits ++;
>                               ^
> 
> 
>> +			dimm->dtype = DEV_X16;
>> +			break;
>> +		}
>> +	}
>> +
>> +	memsize = 1 << bits;
>> +
>> +	dimm->nr_pages = memsize >> PAGE_SHIFT;
>> +	dimm->grain = 4;
>> +	if ((val & SDRAM_TYPE_MASK) == SDRAM_TYPE_DDR2)
>> +		dimm->mtype = MEM_DDR2;
>> +	else
>> +		dimm->mtype = MEM_DDR3;
>> +
>> +	val = ti_edac_readl(edac, EMIF_ECC_CTRL);
>> +	if (val & ECC_ENABLED)
>> +		dimm->edac_mode = EDAC_SECDED;
>> +	else
>> +		dimm->edac_mode = EDAC_NONE;
> 
> So if ECC is not enabled, why do you even need to continue loading the
> driver here and not return a retval which ti_edac_probe() propagates?
> 
> Also, you want to call that function much earlier in ti_edac_probe() so
> that you can save yourself all the setup.
> 
> Or can you have instances which have ECC disabled and others which have
> ECC enabled, on the same platform?

Yeah, the DRA7 SoCs have 2x emif instances, and ECC is only available on 
one (and it can be either enabled or disabled.) I thought it might be 
useful to have the dimm info populated for both in this case to avoid 
confusion and to be able to see the status from userspace.

It is also possible to ditch that and only have dimm info for instances 
which have ECC enabled. Which way would you prefer?

> 
>> +}
>> +
>> +static const struct of_device_id ti_edac_of_match[] = {
>> +	{ .compatible = "ti,emif-keystone", .data = (void *)EMIF_TYPE_K2 },
>> +	{ .compatible = "ti,emif-dra7xx", .data = (void *)EMIF_TYPE_DRA7 },
>> +	{},
>> +};
>> +
>> +static int ti_edac_probe(struct platform_device *pdev)
>> +{
>> +	int error_irq = 0, ret = -ENODEV;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	void __iomem *reg;
>> +	struct mem_ctl_info *mci;
>> +	struct edac_mc_layer layers[1];
>> +	const struct of_device_id *id;
>> +	struct ti_edac *edac;
>> +	static int edac_id;
>> +	int my_id;
>> +
>> +	id = of_match_device(ti_edac_of_match, &pdev->dev);
>> +	if (!id)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	reg = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(reg)) {
>> +		dev_err(dev, "DDR3 controller regs not defined\n");
> 
> We have edac_printk() et al macros for printing. Ditto for the remaining
> calls.

Ok, I can switch to use those.

> 
>> +		return PTR_ERR(reg);
>> +	}
>> +
>> +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
>> +	layers[0].size = 1;
>> +
>> +	/* Allocate ID number for our EMIF controller */
>> +	mutex_lock(&ti_edac_lock);
>> +	my_id = edac_id++;
>> +	mutex_unlock(&ti_edac_lock);
> 
> That looks silly. Why not atomic_inc_return()?

Oh didn't quite realize we had such an API also available. Will switch 
to that.

> 
>> +	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
>> +	if (!mci)
>> +		return -ENOMEM;
>> +
>> +	mci->pdev = &pdev->dev;
>> +	edac = mci->pvt_info;
>> +	edac->reg = reg;
>> +	platform_set_drvdata(pdev, mci);
> 
> ...
> 

Thanks for review, and sorry again for missing the checkpatch.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Borislav Petkov]

On Thu, Nov 09, 2017 at 12:38:02PM +0200, Tero Kristo wrote:
> Yeah, the DRA7 SoCs have 2x emif instances, and ECC is only available on one
> (and it can be either enabled or disabled.) I thought it might be useful to
> have the dimm info populated for both in this case to avoid confusion and to
> be able to see the status from userspace.
> 
> It is also possible to ditch that and only have dimm info for instances
> which have ECC enabled. Which way would you prefer?

Actually, having all DIMMs present, even if ECC is not enabled for a
subset of them, would make the picture more user-friendly. Yap, that's better.

> Thanks for review, and sorry again for missing the checkpatch.

No worries, that's what review is for. :)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Jan Lübbe]

On Tue, 2017-11-07 at 22:38 +0200, Tero Kristo wrote:
> +       if (edac_mc_add_mc(mci)) {
> +               pr_err("%s: Failed to register mci.\n", __func__);
> +               return -ENOMEM;
This leaks the allocated mci structure in the error case.

> +       }
> +
> +       /* add EMIF ECC error handler */
> +       error_irq = platform_get_irq(pdev, 0);
> +       if (!error_irq) {
> +               dev_err(dev, "DDR3 EDAC irq number not defined\n");
> +               return ret;
This leaves the mc registered in the error case.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

On 09/11/17 13:50, Jan L?bbe wrote:
> On Tue, 2017-11-07 at 22:38 +0200, Tero Kristo wrote:
>> +       if (edac_mc_add_mc(mci)) {
>> +               pr_err("%s: Failed to register mci.\n", __func__);
>> +               return -ENOMEM;
> This leaks the allocated mci structure in the error case.
> 
>> +       }
>> +
>> +       /* add EMIF ECC error handler */
>> +       error_irq = platform_get_irq(pdev, 0);
>> +       if (!error_irq) {
>> +               dev_err(dev, "DDR3 EDAC irq number not defined\n");
>> +               return ret;
> This leaves the mc registered in the error case.

I am too much used to devm_alloc nowadays I guess... I'll fix these for 
v2 and maybe re-arrange the irq alloc with rest of the code.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
correct one bit errors and detect two bit errors. Add EDAC driver for this
feature which plugs into the generic kernel EDAC framework.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
v3:
 * dropped the atomic edac_id counter
 * added _emif_get_id which calculates the emif ID based on the sorted
   address order of all emif instances
 * changed the order of irq allocation and edac_mc_add_mc() within probe

v2:
 * fixed checkpatch errors
 * improved error cleanup for failed probe
 * converted to use edac_printk
 * added MAINTAINERS entry
 * dropped mutex and used atomic counter for ID
 * ... and some minor cosmetic changes
 
 MAINTAINERS            |   6 +
 drivers/edac/Kconfig   |   7 +
 drivers/edac/Makefile  |   1 +
 drivers/edac/ti_edac.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 354 insertions(+)
 create mode 100644 drivers/edac/ti_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2281af4..c46ed26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5028,6 +5028,12 @@ L:	linux-edac at vger.kernel.org
 S:	Maintained
 F:	drivers/edac/skx_edac.c
 
+EDAC-TI
+M:	Tero Kristo <t-kristo@ti.com>
+L:	linux-edac at vger.kernel.org
+S:	Maintained
+F:	drivers/edac/ti_edac.c
+
 EDIROL UA-101/UA-1000 DRIVER
 M:	Clemens Ladisch <clemens@ladisch.de>
 L:	alsa-devel at alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 96afb2a..3c40170 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -457,4 +457,11 @@ config EDAC_XGENE
 	  Support for error detection and correction on the
 	  APM X-Gene family of SOCs.
 
+config EDAC_TI
+	tristate "Texas Instruments DDR3 ECC Controller"
+	depends on ARCH_KEYSTONE || SOC_DRA7XX
+	help
+	  Support for error detection and correction on the
+          TI SoCs.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 0fd9ffa..b54912e 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_THUNDERX)		+= thunderx_edac.o
 obj-$(CONFIG_EDAC_ALTERA)		+= altera_edac.o
 obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
 obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
+obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
diff --git a/drivers/edac/ti_edac.c b/drivers/edac/ti_edac.c
new file mode 100644
index 0000000..bd32b12
--- /dev/null
+++ b/drivers/edac/ti_edac.c
@@ -0,0 +1,340 @@
+/*
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Texas Instruments DDR3 ECC error correction and detection driver
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/init.h>
+#include <linux/edac.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+
+#include "edac_module.h"
+
+/* EMIF controller registers */
+#define EMIF_SDRAM_CONFIG		0x008
+#define EMIF_IRQ_STATUS			0x0ac
+#define EMIF_IRQ_ENABLE_SET		0x0b4
+#define EMIF_ECC_CTRL			0x110
+#define EMIF_1B_ECC_ERR_CNT		0x130
+#define EMIF_1B_ECC_ERR_THRSH		0x134
+#define EMIF_1B_ECC_ERR_ADDR_LOG	0x13c
+#define EMIF_2B_ECC_ERR_ADDR_LOG	0x140
+
+/* Bit definitions for EMIF_SDRAM_CONFIG */
+#define SDRAM_TYPE_SHIFT		29
+#define SDRAM_TYPE_MASK			GENMASK(31, 29)
+#define SDRAM_TYPE_DDR3			(3 << SDRAM_TYPE_SHIFT)
+#define SDRAM_TYPE_DDR2			(2 << SDRAM_TYPE_SHIFT)
+#define SDRAM_NARROW_MODE_MASK		GENMASK(15, 14)
+#define SDRAM_K2_NARROW_MODE_SHIFT	12
+#define SDRAM_K2_NARROW_MODE_MASK	GENMASK(13, 12)
+#define SDRAM_ROWSIZE_SHIFT		7
+#define SDRAM_ROWSIZE_MASK		GENMASK(9, 7)
+#define SDRAM_IBANK_SHIFT		4
+#define SDRAM_IBANK_MASK		GENMASK(6, 4)
+#define SDRAM_K2_IBANK_SHIFT		5
+#define SDRAM_K2_IBANK_MASK		GENMASK(6, 5)
+#define SDRAM_K2_EBANK_SHIFT		3
+#define SDRAM_K2_EBANK_MASK		BIT(SDRAM_K2_EBANK_SHIFT)
+#define SDRAM_PAGESIZE_SHIFT		0
+#define SDRAM_PAGESIZE_MASK		GENMASK(2, 0)
+#define SDRAM_K2_PAGESIZE_SHIFT		0
+#define SDRAM_K2_PAGESIZE_MASK		GENMASK(1, 0)
+
+#define EMIF_1B_ECC_ERR_THRSH_SHIFT	24
+
+/* IRQ bit definitions */
+#define EMIF_1B_ECC_ERR			BIT(5)
+#define EMIF_2B_ECC_ERR			BIT(4)
+#define EMIF_WR_ECC_ERR			BIT(3)
+#define EMIF_SYS_ERR			BIT(0)
+/* Bit 31 enables ECC and 28 enables RMW */
+#define ECC_ENABLED			(BIT(31) | BIT(28))
+
+#define EDAC_MOD_NAME			"ti-emif-edac"
+
+enum {
+	EMIF_TYPE_DRA7,
+	EMIF_TYPE_K2
+};
+
+struct ti_edac {
+	void __iomem *reg;
+};
+
+static u32 ti_edac_readl(struct ti_edac *edac, u16 offset)
+{
+	return readl_relaxed(edac->reg + offset);
+}
+
+static void ti_edac_writel(struct ti_edac *edac, u32 val, u16 offset)
+{
+	writel_relaxed(val, edac->reg + offset);
+}
+
+static irqreturn_t ti_edac_isr(int irq, void *data)
+{
+	struct mem_ctl_info *mci = data;
+	struct ti_edac *edac = mci->pvt_info;
+	u32 irq_status;
+	u32 err_addr;
+	int err_count;
+
+	irq_status = ti_edac_readl(edac, EMIF_IRQ_STATUS);
+
+	if (irq_status & EMIF_1B_ECC_ERR) {
+		err_addr = ti_edac_readl(edac, EMIF_1B_ECC_ERR_ADDR_LOG);
+		err_count = ti_edac_readl(edac, EMIF_1B_ECC_ERR_CNT);
+		ti_edac_writel(edac, err_count, EMIF_1B_ECC_ERR_CNT);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, -1, 0, 0, 0,
+				     mci->ctl_name, "1B");
+	}
+
+	if (irq_status & EMIF_2B_ECC_ERR) {
+		err_addr = ti_edac_readl(edac, EMIF_2B_ECC_ERR_ADDR_LOG);
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, -1, 0, 0, 0,
+				     mci->ctl_name, "2B");
+	}
+
+	if (irq_status & EMIF_WR_ECC_ERR)
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     0, 0, -1, 0, 0, 0,
+				     mci->ctl_name, "WR");
+
+	ti_edac_writel(edac, irq_status, EMIF_IRQ_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static void ti_edac_setup_dimm(struct mem_ctl_info *mci, u32 type)
+{
+	struct dimm_info *dimm;
+	struct ti_edac *edac = mci->pvt_info;
+	int bits;
+	u32 val;
+	u32 memsize;
+
+	dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers, 0, 0, 0);
+
+	val = ti_edac_readl(edac, EMIF_SDRAM_CONFIG);
+
+	if (type == EMIF_TYPE_DRA7) {
+		bits = ((val & SDRAM_PAGESIZE_MASK) >> SDRAM_PAGESIZE_SHIFT) + 8;
+		bits += ((val & SDRAM_ROWSIZE_MASK) >> SDRAM_ROWSIZE_SHIFT) + 9;
+		bits += (val & SDRAM_IBANK_MASK) >> SDRAM_IBANK_SHIFT;
+
+		if (val & SDRAM_NARROW_MODE_MASK) {
+			bits++;
+			dimm->dtype = DEV_X16;
+		} else {
+			bits += 2;
+			dimm->dtype = DEV_X32;
+		}
+	} else {
+		bits = 16;
+		bits += ((val & SDRAM_K2_PAGESIZE_MASK) >>
+			SDRAM_K2_PAGESIZE_SHIFT) + 8;
+		bits += (val & SDRAM_K2_IBANK_MASK) >> SDRAM_K2_IBANK_SHIFT;
+		bits += (val & SDRAM_K2_EBANK_MASK) >> SDRAM_K2_EBANK_SHIFT;
+
+		val = (val & SDRAM_K2_NARROW_MODE_MASK) >>
+			SDRAM_K2_NARROW_MODE_SHIFT;
+		switch (val) {
+		case 0:
+			bits += 3;
+			dimm->dtype = DEV_X64;
+			break;
+		case 1:
+			bits += 2;
+			dimm->dtype = DEV_X32;
+			break;
+		case 2:
+			bits++;
+			dimm->dtype = DEV_X16;
+			break;
+		}
+	}
+
+	memsize = 1 << bits;
+
+	dimm->nr_pages = memsize >> PAGE_SHIFT;
+	dimm->grain = 4;
+	if ((val & SDRAM_TYPE_MASK) == SDRAM_TYPE_DDR2)
+		dimm->mtype = MEM_DDR2;
+	else
+		dimm->mtype = MEM_DDR3;
+
+	val = ti_edac_readl(edac, EMIF_ECC_CTRL);
+	if (val & ECC_ENABLED)
+		dimm->edac_mode = EDAC_SECDED;
+	else
+		dimm->edac_mode = EDAC_NONE;
+}
+
+static const struct of_device_id ti_edac_of_match[] = {
+	{ .compatible = "ti,emif-keystone", .data = (void *)EMIF_TYPE_K2 },
+	{ .compatible = "ti,emif-dra7xx", .data = (void *)EMIF_TYPE_DRA7 },
+	{},
+};
+
+static int _emif_get_id(struct device_node *node)
+{
+	const __be32 *addrp;
+	u32 addr;
+	u32 my_addr;
+	int my_id = 0;
+	struct device_node *np;
+
+	addrp = of_get_address(node, 0, NULL, NULL);
+	my_addr = (u32)of_translate_address(node, addrp);
+
+	for_each_matching_node(np, ti_edac_of_match) {
+		if (np == node)
+			continue;
+
+		addrp = of_get_address(np, 0, NULL, NULL);
+		addr = (u32)of_translate_address(np, addrp);
+
+		pr_info("%s: addr=%x, my_addr=%x\n", __func__,
+			addr, my_addr);
+
+		if (addr < my_addr)
+			my_id++;
+	}
+
+	return my_id;
+}
+
+static int ti_edac_probe(struct platform_device *pdev)
+{
+	int error_irq = 0, ret = -ENODEV;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *reg;
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[1];
+	const struct of_device_id *id;
+	struct ti_edac *edac;
+	int emif_id;
+
+	id = of_match_device(ti_edac_of_match, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg)) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "EMIF controller regs not defined\n");
+		return PTR_ERR(reg);
+	}
+
+	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
+	layers[0].size = 1;
+
+	/* Allocate ID number for our EMIF controller */
+	emif_id = _emif_get_id(pdev->dev.of_node);
+	if (emif_id < 0)
+		return -EINVAL;
+
+	mci = edac_mc_alloc(emif_id, 1, layers, sizeof(*edac));
+	if (!mci)
+		return -ENOMEM;
+
+	mci->pdev = &pdev->dev;
+	edac = mci->pvt_info;
+	edac->reg = reg;
+	platform_set_drvdata(pdev, mci);
+
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
+	mci->mod_name = EDAC_MOD_NAME;
+	mci->ctl_name = id->compatible;
+	mci->dev_name = dev_name(&pdev->dev);
+
+	/* Setup memory layout */
+	ti_edac_setup_dimm(mci, (u32)(id->data));
+
+	/* add EMIF ECC error handler */
+	error_irq = platform_get_irq(pdev, 0);
+	if (!error_irq) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "EMIF irq number not defined.\n");
+		goto err;
+	}
+
+	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
+			       "emif-edac-irq", mci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "request_irq fail for EMIF EDAC irq\n");
+		goto err;
+	}
+
+	ret = edac_mc_add_mc(mci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "Failed to register mci: %d.\n", ret);
+		goto err;
+	}
+
+	/* Generate an interrupt with each 1b error */
+	ti_edac_writel(edac, 1 << EMIF_1B_ECC_ERR_THRSH_SHIFT,
+		       EMIF_1B_ECC_ERR_THRSH);
+
+	/* Enable interrupts */
+	ti_edac_writel(edac,
+		       EMIF_1B_ECC_ERR | EMIF_2B_ECC_ERR | EMIF_WR_ECC_ERR,
+		       EMIF_IRQ_ENABLE_SET);
+
+	return 0;
+
+err:
+	edac_mc_free(mci);
+	return ret;
+}
+
+static int ti_edac_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+static struct platform_driver ti_edac_driver = {
+	.probe = ti_edac_probe,
+	.remove = ti_edac_remove,
+	.driver = {
+		   .name = EDAC_MOD_NAME,
+		   .of_match_table = ti_edac_of_match,
+	},
+};
+
+module_platform_driver(ti_edac_driver);
+
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("EDAC Driver for Texas Instruments DDR3 MC");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Santosh Shilimkar]

On 11/13/2017 5:08 AM, Tero Kristo wrote:
> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
> correct one bit errors and detect two bit errors. Add EDAC driver for this
> feature which plugs into the generic kernel EDAC framework.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
Once you have driver queued up, please post DTS patches
separately. Will queue them up.

Regards,
Santosh

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Borislav Petkov]

On Mon, Nov 13, 2017 at 09:10:09AM -0800, Santosh Shilimkar wrote:
> Once you have driver queued up, please post DTS patches
> separately. Will queue them up.

It would be easier if you ACK the DTS patches and I can pick all three
up after the merge window is over. This way they go together and there's
no inter-tree dependency.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Santosh Shilimkar]

On 11/13/2017 9:58 AM, Borislav Petkov wrote:
> On Mon, Nov 13, 2017 at 09:10:09AM -0800, Santosh Shilimkar wrote:
>> Once you have driver queued up, please post DTS patches
>> separately. Will queue them up.
> 
> It would be easier if you ACK the DTS patches and I can pick all three
> up after the merge window is over. This way they go together and there's
> no inter-tree dependency.
> 
That doesn't help and not an usual workflow.
DTS patches actually conflict and they better go through arch port.
Let the driver get merged which has no dependency with DTS patches
instead of waiting for merge window being over. DTS patches can be
merged anytime once its respective driver are in tree.

Regards,
Santosh

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Borislav Petkov]

On Mon, Nov 13, 2017 at 10:04:09AM -0800, Santosh Shilimkar wrote:
> DTS patches can be merged anytime once its respective driver are in
> tree.

Works for me too.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo]

On 13/11/17 20:08, Borislav Petkov wrote:
> On Mon, Nov 13, 2017 at 10:04:09AM -0800, Santosh Shilimkar wrote:
>> DTS patches can be merged anytime once its respective driver are in
>> tree.
> 
> Works for me too.
> 

I guess you can just merge the dts patch also whenever you like, as 
there is currently no support around for ti,emif-keystone. It just won't 
do anything.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Santosh Shilimkar]

On 11/13/2017 10:49 AM, Tero Kristo wrote:
> On 13/11/17 20:08, Borislav Petkov wrote:
>> On Mon, Nov 13, 2017 at 10:04:09AM -0800, Santosh Shilimkar wrote:
>>> DTS patches can be merged anytime once its respective driver are in
>>> tree.
>>
>> Works for me too.
>>
> 
> I guess you can just merge the dts patch also whenever you like, as 
> there is currently no support around for ti,emif-keystone. It just won't 
> do anything.
> 
Its not followed practice Tero and I don't want to get flamed for
it. ;-) I will apply the patches once the driver is in.

Regards,
Santosh

[PATCHv3 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Borislav Petkov]

On Mon, Nov 13, 2017 at 03:08:10PM +0200, Tero Kristo wrote:
> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
> correct one bit errors and detect two bit errors. Add EDAC driver for this
> feature which plugs into the generic kernel EDAC framework.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
> v3:
>  * dropped the atomic edac_id counter
>  * added _emif_get_id which calculates the emif ID based on the sorted
>    address order of all emif instances
>  * changed the order of irq allocation and edac_mc_add_mc() within probe
> 
> v2:
>  * fixed checkpatch errors
>  * improved error cleanup for failed probe
>  * converted to use edac_printk
>  * added MAINTAINERS entry
>  * dropped mutex and used atomic counter for ID
>  * ... and some minor cosmetic changes
>  
>  MAINTAINERS            |   6 +
>  drivers/edac/Kconfig   |   7 +
>  drivers/edac/Makefile  |   1 +
>  drivers/edac/ti_edac.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 354 insertions(+)
>  create mode 100644 drivers/edac/ti_edac.c

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 3/3] ARM: dts: Keystone: add ECC error handler support

[Tero Kristo]

From: Murali Karicheri <m-karicheri2@ti.com>

Add emif node for keystone2 devices, which is used for ECC support.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
[t-kristo at ti.com: made emif enabled by default for all keystone2 devices]
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
---
 arch/arm/boot/dts/keystone-k2g.dtsi | 6 ++++++
 arch/arm/boot/dts/keystone.dtsi     | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi b/arch/arm/boot/dts/keystone-k2g.dtsi
index 826b286..33e12c1 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -343,5 +343,11 @@
 			clock-names = "fck", "mmchsdb_fck";
 			status = "disabled";
 		};
+
+		emif: emif at 21010000 {
+			compatible = "ti,emif-keystone";
+			reg = <0x21010000 0x200>;
+			interrupts = <GIC_SPI 123 IRQ_TYPE_EDGE_RISING>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index 8dd74f4..5f4e38f 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -340,5 +340,11 @@
 					<GIC_SPI 29 IRQ_TYPE_EDGE_RISING>;
 			};
 		};
+
+		emif: emif at 21010000 {
+			compatible = "ti,emif-keystone";
+			reg = <0x21010000 0x200>;
+			interrupts = <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>;
+		};
 	};
 };
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki