Add EDAC support for Altera SoC SDRAM Controller

Add EDAC support for Altera SoC SDRAM Controller

[tthayer]

[PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM
[PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC
[PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM

[PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[tthayer]

From: Thor Thayer <tthayer@altera.com>

Addition of the Altera SDRAM controller bindings and device
tree changes to the Altera SoC project.

v2: Changes to SoC SDRAM EDAC code.

v3: Implement code suggestions for SDRAM EDAC code.

v4: Remove syscon from SDRAM controller bindings.

v5: No Change, bump version for consistency.

Signed-off-by: Thor Thayer <tthayer@altera.com>
---
 .../bindings/arm/altera/socfpga-sdram.txt          |   11 +++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
 2 files changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
new file mode 100644
index 0000000..8f8746b
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
@@ -0,0 +1,11 @@
+Altera SOCFPGA SDRAM Controller
+
+Required properties:
+- compatible : "altr,sdr-ctl";
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	sdrctl@ffc25000 {
+		compatible = "altr,sdr-ctl";
+		reg = <0xffc25000 0x1000>;
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index df43702..6ce912e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -676,6 +676,11 @@
 			clocks = <&l4_sp_clk>;
 		};
 
+		sdrctl@ffc25000 {
+			compatible = "altr,sdr-ctl", "syscon";
+			reg = <0xffc25000 0x1000>;
+		};
+
 		rstmgr@ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

[PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC

[tthayer]

From: Thor Thayer <tthayer@altera.com>

Addition of the Altera SDRAM EDAC bindings and device
tree changes to the Altera SoC project.

v2: Changes to SoC EDAC source code.

v3: Fix typo in device tree documentation.

v4,v5: No changes - bump version for consistency.

Signed-off-by: Thor Thayer <tthayer@altera.com>
---
 .../bindings/arm/altera/socfpga-sdram-edac.txt     |   12 ++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
 2 files changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 0000000..431e98b
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,12 @@
+Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
+
+Required properties:
+- compatible : should contain "altr,sdram-edac";
+- interrupts : Should contain the SDRAM ECC IRQ in the
+	appropriate format for the IRQ controller.
+
+Example:
+	sdramedac@0 {
+		compatible = "altr,sdram-edac";
+		interrupts = <0 39 4>;
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 6ce912e..a0ea69b 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -681,6 +681,11 @@
 			reg = <0xffc25000 0x1000>;
 		};
 
+		sdramedac@0 {
+			compatible = "altr,sdram-edac";
+			interrupts = <0 39 4>;
+		};
+
 		rstmgr@ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

[PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller

[tthayer]

From: Thor Thayer <tthayer@altera.com>

v2: Use the SDRAM controller registers to calculate memory size
    instead of the Device Tree. Update To & Cc list. Add maintainer
    information.

v3: EDAC driver cleanup based on comments from Mailing list.

v4: Panic on DBE. Add macro around inject-error reads to prevent
    them from being optimized out. Remove of_match_ptr since this
    will always use Device Tree.

v5: Addition of printk to trigger function to ensure read vars
    are not optimized out.

Signed-off-by: Thor Thayer <tthayer@altera.com>
---
 MAINTAINERS                |    5 +
 drivers/edac/Kconfig       |    9 +
 drivers/edac/Makefile      |    2 +
 drivers/edac/altera_edac.c |  419 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 435 insertions(+)
 create mode 100644 drivers/edac/altera_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e67ea24..ecd1277 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1290,6 +1290,11 @@ M:	Dinh Nguyen <dinguyen@altera.com>
 S:	Maintained
 F:	drivers/clk/socfpga/
 
+ARM/SOCFPGA SDRAM EDAC SUPPORT
+M:	Thor Thayer <tthayer@altera.com>
+S:	Maintained
+F:	drivers/edac/altera_edac.c
+
 ARM/STI ARCHITECTURE
 M:	Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
 M:	Maxime Coquelin <maxime.coquelin@st.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..4f4d379 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,13 @@ config EDAC_OCTEON_PCI
 	  Support for error detection and correction on the
 	  Cavium Octeon family of SOCs.
 
+config EDAC_ALTERA_MC
+	bool "Altera SDRAM Memory Controller EDAC"
+	depends on EDAC_MM_EDAC && ARCH_SOCFPGA
+	help
+	  Support for error detection and correction on the
+	  Altera SDRAM memory controller. Note that the
+	  preloader must initialize the SDRAM before loading
+	  the kernel.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6..9741336 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -64,3 +64,5 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
+
+obj-$(CONFIG_EDAC_ALTERA_MC)	        += altera_edac.o
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
new file mode 100644
index 0000000..39d3dd4
--- /dev/null
+++ b/drivers/edac/altera_edac.c
@@ -0,0 +1,419 @@
+/*
+ *  Copyright Altera Corporation (C) 2014. All rights reserved.
+ *  Copyright 2011-2012 Calxeda, Inc.
+ *
+ * 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.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+
+ *
+ * Adapted from the highbank_mc_edac driver
+ *
+ */
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/uaccess.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.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			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		0x2C
+
+/* SDRAM Controller Address Widths Field Register */
+#define DRAMADDRW_COLBIT_MASK	0x001F
+#define DRAMADDRW_COLBIT_LSB	0
+#define DRAMADDRW_ROWBIT_MASK	0x03E0
+#define DRAMADDRW_ROWBIT_LSB	5
+#define DRAMADDRW_BANKBIT_MASK	0x1C00
+#define DRAMADDRW_BANKBIT_LSB	10
+#define DRAMADDRW_CSBIT_MASK	0xE000
+#define DRAMADDRW_CSBIT_LSB	13
+
+/* SDRAM Controller Interface Data Width Register */
+#define DRAMIFWIDTH		0x30
+
+/* SDRAM Controller Interface Data Width Defines */
+#define DRAMIFWIDTH_16B_ECC	24
+#define DRAMIFWIDTH_32B_ECC	40
+
+/* SDRAM Controller DRAM Status Register */
+#define DRAMSTS		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		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		0x40
+
+/* SDRAM Controller Single Bit Error Count Register Bit Masks */
+#define SBECOUNT_MASK		0x0F
+
+/* SDRAM Controller Double Bit Error Count Register */
+#define DBECOUNT		0x44
+
+/* SDRAM Controller Double Bit Error Count Register Bit Masks */
+#define DBECOUNT_MASK		0x0F
+
+/* SDRAM Controller ECC Error Address Register */
+#define ERRADDR		0x48
+
+/* SDRAM Controller ECC Error Address Register Bit Masks */
+#define ERRADDR_MASK		0xFFFFFFFF
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register */
+#define DROPCOUNT		0x4C
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
+#define DROPCOUNT_MASK		0x0F
+
+/* SDRAM Controller ECC AutoCorrect Address Register */
+#define DROPADDR		0x50
+
+/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
+#define DROPADDR_MASK		0xFFFFFFFF
+
+/* Altera SDRAM Memory Controller data */
+struct altr_sdram_mc_data {
+	struct regmap *mc_vbase;
+};
+
+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;
+	u32 status, err_count, err_addr;
+
+	/* Error Address is shared by both SBE & DBE */
+	regmap_read(drvdata->mc_vbase, ERRADDR, &err_addr);
+
+	regmap_read(drvdata->mc_vbase, DRAMSTS, &status);
+
+	if (status & DRAMSTS_DBEERR) {
+		regmap_read(drvdata->mc_vbase, DBECOUNT, &err_count);
+		panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
+		      err_count, err_addr);
+	}
+	if (status & DRAMSTS_SBEERR) {
+		regmap_read(drvdata->mc_vbase, SBECOUNT, &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,
+		     (DRAMINTR_INTRCLR | DRAMINTR_INTREN));
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_EDAC_DEBUG
+static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
+					    const char __user *data,
+					    size_t count, loff_t *ppos)
+{
+	struct mem_ctl_info *mci = file->private_data;
+	struct altr_sdram_mc_data *drvdata = mci->pvt_info;
+	u32 *ptemp;
+	dma_addr_t dma_handle;
+	u32 reg, read_reg;
+
+	ptemp = dma_alloc_coherent(mci->pdev, 16, &dma_handle, GFP_KERNEL);
+	if (IS_ERR(ptemp)) {
+		dma_free_coherent(mci->pdev, 16, ptemp, dma_handle);
+		edac_printk(KERN_ERR, EDAC_MC,
+			    "Inject: Buffer Allocation error\n");
+		return -ENOMEM;
+	}
+
+	regmap_read(drvdata->mc_vbase, CTLCFG, &read_reg);
+	read_reg &= ~(CTLCFG_GEN_SB_ERR | CTLCFG_GEN_DB_ERR);
+
+	if (count == 3) {
+		edac_printk(KERN_ALERT, EDAC_MC,
+			    "Inject Double bit error\n");
+		regmap_write(drvdata->mc_vbase, CTLCFG,
+			     (read_reg | CTLCFG_GEN_DB_ERR));
+	} else {
+		edac_printk(KERN_ALERT, EDAC_MC,
+			    "Inject Single bit error\n");
+		regmap_write(drvdata->mc_vbase,	CTLCFG,
+			     (read_reg | CTLCFG_GEN_SB_ERR));
+	}
+
+	ptemp[0] = 0x5A5A5A5A;
+	ptemp[1] = 0xA5A5A5A5;
+	/* Clear the error injection bits */
+	regmap_write(drvdata->mc_vbase,	CTLCFG, read_reg);
+	/* Ensure it has been written out */
+	wmb();
+
+	/*
+	 * To trigger the error, we need to read the data back
+	 * (the data was written with errors above)
+	 * The ACCESS_ONCE macros are used to prevent the
+	 * compiler optimizing these reads out.
+	 */
+	reg = ACCESS_ONCE(ptemp[0]);
+	read_reg = ACCESS_ONCE(ptemp[1]);
+	/* Force Read */
+	rmb();
+
+	edac_printk(KERN_ALERT, EDAC_MC, "Read Data [0x%X, 0x%X]\n",
+		    reg, read_reg);
+
+	dma_free_coherent(mci->pdev, 16, ptemp, dma_handle);
+
+	return count;
+}
+
+static const struct file_operations altr_sdr_mc_debug_inject_fops = {
+	.open = simple_open,
+	.write = altr_sdr_mc_err_inject_write,
+	.llseek = generic_file_llseek,
+};
+
+static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
+{
+	if (mci->debugfs)
+		debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs, mci,
+				    &altr_sdr_mc_debug_inject_fops);
+}
+#else
+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)
+{
+	u32 size;
+	u32 read_reg, row, bank, col, cs, width;
+
+	if (regmap_read(mc_vbase, DRAMADDRW, &read_reg) < 0)
+		return 0;
+
+	if (regmap_read(mc_vbase, DRAMIFWIDTH, &width) < 0)
+		return 0;
+
+	col = (read_reg & DRAMADDRW_COLBIT_MASK) >>
+		DRAMADDRW_COLBIT_LSB;
+	row = (read_reg & DRAMADDRW_ROWBIT_MASK) >>
+		DRAMADDRW_ROWBIT_LSB;
+	bank = (read_reg & DRAMADDRW_BANKBIT_MASK) >>
+		DRAMADDRW_BANKBIT_LSB;
+	cs = (read_reg & DRAMADDRW_CSBIT_MASK) >>
+		DRAMADDRW_CSBIT_LSB;
+
+	/* 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;
+}
+
+static int altr_sdram_probe(struct platform_device *pdev)
+{
+	struct edac_mc_layer layers[2];
+	struct mem_ctl_info *mci;
+	struct altr_sdram_mc_data *drvdata;
+	struct regmap *mc_vbase;
+	struct dimm_info *dimm;
+	u32 read_reg, mem_size;
+	int irq;
+	int res = 0;
+
+	/* Validate the SDRAM controller has ECC enabled */
+	/* Grab the register values from the sdr-ctl in device tree */
+	mc_vbase = syscon_regmap_lookup_by_compatible("altr,sdr-ctl");
+	if (IS_ERR(mc_vbase)) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			    "regmap for altr,sdr-ctl lookup failed.\n");
+		return -ENODEV;
+	}
+
+	if (regmap_read(mc_vbase, CTLCFG, &read_reg) ||
+	    ((read_reg & CTLCFG_ECC_AUTO_EN) !=	CTLCFG_ECC_AUTO_EN)) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			    "No ECC/ECC disabled [0x%08X]\n", read_reg);
+		return -ENODEV;
+	}
+
+	/* Grab memory size from device tree. */
+	mem_size = altr_sdram_get_total_mem_size(mc_vbase);
+	if (mem_size <= 0) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			    "Unable to calculate memory size\n");
+		return -ENODEV;
+	}
+
+	/* Ensure the SDRAM Interrupt is disabled and cleared */
+	if (regmap_write(mc_vbase, DRAMINTR, DRAMINTR_INTRCLR)) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			    "Error clearing SDRAM ECC IRQ\n");
+		return -ENODEV;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			    "No irq %d in DT\n", irq);
+		return -ENODEV;
+	}
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = 1;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = 1;
+	layers[1].is_virt_csrow = false;
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+			    sizeof(struct altr_sdram_mc_data));
+	if (!mci)
+		return -ENOMEM;
+
+	mci->pdev = &pdev->dev;
+	drvdata = mci->pvt_info;
+	drvdata->mc_vbase = mc_vbase;
+	platform_set_drvdata(pdev, mci);
+
+	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
+		res = -ENOMEM;
+		goto free;
+	}
+
+	mci->mtype_cap = MEM_FLAG_DDR3;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->mod_name = EDAC_MOD_STR;
+	mci->mod_ver = EDAC_VERSION;
+	mci->ctl_name = dev_name(&pdev->dev);
+	mci->scrub_mode = SCRUB_SW_SRC;
+	mci->dev_name = dev_name(&pdev->dev);
+
+	dimm = *mci->dimms;
+	dimm->nr_pages = ((mem_size - 1) >> PAGE_SHIFT) + 1;
+	dimm->grain = 8;
+	dimm->dtype = DEV_X8;
+	dimm->mtype = MEM_DDR3;
+	dimm->edac_mode = EDAC_SECDED;
+
+	res = edac_mc_add_mc(mci);
+	if (res < 0)
+		goto err;
+
+	res = devm_request_irq(&pdev->dev, irq, altr_sdram_mc_err_handler,
+			       0, dev_name(&pdev->dev), mci);
+	if (res < 0) {
+		edac_mc_printk(mci, KERN_ERR,
+			       "Unable to request irq %d\n", irq);
+		res = -ENODEV;
+		goto err2;
+	}
+
+	if (regmap_write(drvdata->mc_vbase, DRAMINTR,
+			 (DRAMINTR_INTRCLR | DRAMINTR_INTREN))) {
+		edac_mc_printk(mci, KERN_ERR,
+			       "Error enabling SDRAM ECC IRQ\n");
+		res = -ENODEV;
+		goto err2;
+	}
+
+	altr_sdr_mc_create_debugfs_nodes(mci);
+
+	devres_close_group(&pdev->dev, NULL);
+
+	return 0;
+
+err2:
+	edac_mc_del_mc(&pdev->dev);
+err:
+	devres_release_group(&pdev->dev, NULL);
+free:
+	edac_mc_free(mci);
+	edac_printk(KERN_ERR, EDAC_MC,
+		    "EDAC Probe Failed; Error %d\n", res);
+
+	return res;
+}
+
+static int altr_sdram_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static 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,
+	.driver = {
+		.name = "altr_sdram_edac",
+		.of_match_table = altr_sdram_ctrl_of_match,
+	},
+};
+
+module_platform_driver(altr_sdram_edac_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Altera Corporation");
+MODULE_DESCRIPTION("EDAC Driver for Altera SDRAM Controller");
-- 
1.7.9.5

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

Hi!

On Thu, May 15, 2014 at 11:04:49AM -0500, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Addition of the Altera SDRAM controller bindings and device
> tree changes to the Altera SoC project.
> 
> v2: Changes to SoC SDRAM EDAC code.
> 
> v3: Implement code suggestions for SDRAM EDAC code.
> 
> v4: Remove syscon from SDRAM controller bindings.
> 
> v5: No Change, bump version for consistency.
> 
> Signed-off-by: Thor Thayer <tthayer@altera.com>
> ---
>  .../bindings/arm/altera/socfpga-sdram.txt          |   11 +++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
>  2 files changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> new file mode 100644
> index 0000000..8f8746b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> @@ -0,0 +1,11 @@
> +Altera SOCFPGA SDRAM Controller
> +
> +Required properties:
> +- compatible : "altr,sdr-ctl";
> +- reg : Should contain 1 register ranges(address and length)
> +
> +Example:
> +	sdrctl@ffc25000 {
> +		compatible = "altr,sdr-ctl";
> +		reg = <0xffc25000 0x1000>;
> +	};
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index df43702..6ce912e 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -676,6 +676,11 @@
>  			clocks = <&l4_sp_clk>;
>  		};
>  
> +		sdrctl@ffc25000 {
> +			compatible = "altr,sdr-ctl", "syscon";
                                                   ^^^^^^^^^^

Get rid of that, too, please.
> +			reg = <0xffc25000 0x1000>;
> +		};
> +

How about

		sdrctl@ffc25000 {
			compatible = "altr,sdr-ctl";
			reg = <0xffc25000 0x1000>;
			ranges;

			edac@ffc2502c {
				compatible = "altr,sdram-edac";
				reg = <0xffc2502c 0x50>;
				interrupts = <0 39 4>;
			};
		};

Then we can later add:

			sdr-ports: ports@ffc2507c {
				#reset-cells = <1>;
				compatible = "altr,sdr-ports";
				reg = <0xffc2507c 0x10>;
				clocks = <&ddr_dqs_clk>;
				...
			};

to use the reset-controller framework for the port resets.

>  		rstmgr@ffd05000 {
>  			compatible = "altr,rst-mgr";
>  			reg = <0xffd05000 0x1000>;
> -- 

Regards,
Steffen

-- 
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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Hi!
>
> On Thu, May 15, 2014 at 11:04:49AM -0500, tthayer@altera.com wrote:
>> From: Thor Thayer <tthayer@altera.com>
>>
>> Addition of the Altera SDRAM controller bindings and device
>> tree changes to the Altera SoC project.
>>
>> v2: Changes to SoC SDRAM EDAC code.
>>
>> v3: Implement code suggestions for SDRAM EDAC code.
>>
>> v4: Remove syscon from SDRAM controller bindings.
>>
>> v5: No Change, bump version for consistency.
>>
>> Signed-off-by: Thor Thayer <tthayer@altera.com>
>> ---
>>  .../bindings/arm/altera/socfpga-sdram.txt          |   11 +++++++++++
>>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
>>  2 files changed, 16 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> new file mode 100644
>> index 0000000..8f8746b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> @@ -0,0 +1,11 @@
>> +Altera SOCFPGA SDRAM Controller
>> +
>> +Required properties:
>> +- compatible : "altr,sdr-ctl";
>> +- reg : Should contain 1 register ranges(address and length)
>> +
>> +Example:
>> +     sdrctl@ffc25000 {
>> +             compatible = "altr,sdr-ctl";
>> +             reg = <0xffc25000 0x1000>;
>> +     };
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index df43702..6ce912e 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -676,6 +676,11 @@
>>                       clocks = <&l4_sp_clk>;
>>               };
>>
>> +             sdrctl@ffc25000 {
>> +                     compatible = "altr,sdr-ctl", "syscon";
>                                                    ^^^^^^^^^^
>
> Get rid of that, too, please.

Hi Steffan,

I believe I need to keep the "syscon". The same register (ctrlcfg)
that has the ECC enable bitfield also includes the DRAM configuration
bitfields that other drivers will want to access (specifically the
FPGA bridge needs this information). Since this register will be
shared between drivers,  syscon seems like the best solution.

I may be misunderstanding something so feel free to elaborate. Thanks
for reviewing.

Thor


>> +                     reg = <0xffc25000 0x1000>;
>> +             };
>> +
>
> How about
>
>                 sdrctl@ffc25000 {
>                         compatible = "altr,sdr-ctl";
>                         reg = <0xffc25000 0x1000>;
>                         ranges;
>
>                         edac@ffc2502c {
>                                 compatible = "altr,sdram-edac";
>                                 reg = <0xffc2502c 0x50>;
>                                 interrupts = <0 39 4>;
>                         };
>                 };
>
> Then we can later add:
>
>                         sdr-ports: ports@ffc2507c {
>                                 #reset-cells = <1>;
>                                 compatible = "altr,sdr-ports";
>                                 reg = <0xffc2507c 0x10>;
>                                 clocks = <&ddr_dqs_clk>;
>                                 ...
>                         };
>
> to use the reset-controller framework for the port resets.
>
>>               rstmgr@ffd05000 {
>>                       compatible = "altr,rst-mgr";
>>                       reg = <0xffd05000 0x1000>;
>> --
>
> Regards,
> Steffen
>
> --
> 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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

Hi Thor!

On Mon, May 19, 2014 at 01:36:30PM -0500, Thor Thayer wrote:
> On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > Hi!
> >
> > On Thu, May 15, 2014 at 11:04:49AM -0500, tthayer@altera.com wrote:
> >> From: Thor Thayer <tthayer@altera.com>
> >>
> >> Addition of the Altera SDRAM controller bindings and device
> >> tree changes to the Altera SoC project.
> >>
> >> v2: Changes to SoC SDRAM EDAC code.
> >>
> >> v3: Implement code suggestions for SDRAM EDAC code.
> >>
> >> v4: Remove syscon from SDRAM controller bindings.
> >>
> >> v5: No Change, bump version for consistency.
> >>
> >> Signed-off-by: Thor Thayer <tthayer@altera.com>
> >> ---
> >>  .../bindings/arm/altera/socfpga-sdram.txt          |   11 +++++++++++
> >>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
> >>  2 files changed, 16 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> new file mode 100644
> >> index 0000000..8f8746b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> @@ -0,0 +1,11 @@
> >> +Altera SOCFPGA SDRAM Controller
> >> +
> >> +Required properties:
> >> +- compatible : "altr,sdr-ctl";
> >> +- reg : Should contain 1 register ranges(address and length)
> >> +
> >> +Example:
> >> +     sdrctl@ffc25000 {
> >> +             compatible = "altr,sdr-ctl";
> >> +             reg = <0xffc25000 0x1000>;
> >> +     };
> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> index df43702..6ce912e 100644
> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> @@ -676,6 +676,11 @@
> >>                       clocks = <&l4_sp_clk>;
> >>               };
> >>
> >> +             sdrctl@ffc25000 {
> >> +                     compatible = "altr,sdr-ctl", "syscon";
> >                                                    ^^^^^^^^^^
> >
> > Get rid of that, too, please.
> 
> Hi Steffan,
> 
> I believe I need to keep the "syscon". The same register (ctrlcfg)
> that has the ECC enable bitfield also includes the DRAM configuration
> bitfields that other drivers will want to access (specifically the
> FPGA bridge needs this information). Since this register will be
> shared between drivers,  syscon seems like the best solution.
> 

Hm, from looking at the documentation of the ctrlcfg I can't really
understand which bits you would need for the FPGA brigde and why.
That all sounds like stuff you would want to set for the specific
RAM you are dealing with on a specific board.
What bridge are you talking about? The SDRAM bridge?

I can see the problem with the ECC enable, though.

Regards,
Steffen

> >                 sdrctl@ffc25000 {
> >                         compatible = "altr,sdr-ctl";
> >                         reg = <0xffc25000 0x1000>;
> >                         ranges;
> >
> >                         edac@ffc2502c {
> >                                 compatible = "altr,sdram-edac";
> >                                 reg = <0xffc2502c 0x50>;
> >                                 interrupts = <0 39 4>;
> >                         };
> >                 };
> >
> > Then we can later add:
> >
> >                         sdr-ports: ports@ffc2507c {
> >                                 #reset-cells = <1>;
> >                                 compatible = "altr,sdr-ports";
> >                                 reg = <0xffc2507c 0x10>;
> >                                 clocks = <&ddr_dqs_clk>;
> >                                 ...
> >                         };

-- 
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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Mon, May 19, 2014 at 2:12 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Hi Thor!
>
> On Mon, May 19, 2014 at 01:36:30PM -0500, Thor Thayer wrote:
>> On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>> > Hi!
>> >
>> > On Thu, May 15, 2014 at 11:04:49AM -0500, tthayer@altera.com wrote:
>> >> From: Thor Thayer <tthayer@altera.com>
>> >>
>> >> Addition of the Altera SDRAM controller bindings and device
>> >> tree changes to the Altera SoC project.
>> >>
>> >> v2: Changes to SoC SDRAM EDAC code.
>> >>
>> >> v3: Implement code suggestions for SDRAM EDAC code.
>> >>
>> >> v4: Remove syscon from SDRAM controller bindings.
>> >>
>> >> v5: No Change, bump version for consistency.
>> >>
>> >> Signed-off-by: Thor Thayer <tthayer@altera.com>
>> >> ---
>> >>  .../bindings/arm/altera/socfpga-sdram.txt          |   11 +++++++++++
>> >>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
>> >>  2 files changed, 16 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> new file mode 100644
>> >> index 0000000..8f8746b
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> @@ -0,0 +1,11 @@
>> >> +Altera SOCFPGA SDRAM Controller
>> >> +
>> >> +Required properties:
>> >> +- compatible : "altr,sdr-ctl";
>> >> +- reg : Should contain 1 register ranges(address and length)
>> >> +
>> >> +Example:
>> >> +     sdrctl@ffc25000 {
>> >> +             compatible = "altr,sdr-ctl";
>> >> +             reg = <0xffc25000 0x1000>;
>> >> +     };
>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >> index df43702..6ce912e 100644
>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >> @@ -676,6 +676,11 @@
>> >>                       clocks = <&l4_sp_clk>;
>> >>               };
>> >>
>> >> +             sdrctl@ffc25000 {
>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>> >                                                    ^^^^^^^^^^
>> >
>> > Get rid of that, too, please.
>>
>> Hi Steffan,
>>
>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>> that has the ECC enable bitfield also includes the DRAM configuration
>> bitfields that other drivers will want to access (specifically the
>> FPGA bridge needs this information). Since this register will be
>> shared between drivers,  syscon seems like the best solution.
>>
>
> Hm, from looking at the documentation of the ctrlcfg I can't really
> understand which bits you would need for the FPGA brigde and why.
> That all sounds like stuff you would want to set for the specific
> RAM you are dealing with on a specific board.
> What bridge are you talking about? The SDRAM bridge?
>
> I can see the problem with the ECC enable, though.
>
> Regards,
> Steffen
>

Hi Steffen,

I'll get more details from the guy working on the FPGA bridge when he
gets back in the office. When I started working on EDAC, that register
had been allocated by the FPGA bridge driver so we decided to use the
syscon to allow sharing of the register.

My understanding was that the FPGA bridge as an SDRAM master would
allow FPGA devices to access the SDRAM.  As part of that process, they
may want to read the SDRAM configuration.

I'll need to get more details from the driver developer because the
FPGA driver is in flux while the appropriate driver architecture is
being discussed.

Thor


>> >                 sdrctl@ffc25000 {
>> >                         compatible = "altr,sdr-ctl";
>> >                         reg = <0xffc25000 0x1000>;
>> >                         ranges;
>> >
>> >                         edac@ffc2502c {
>> >                                 compatible = "altr,sdram-edac";
>> >                                 reg = <0xffc2502c 0x50>;
>> >                                 interrupts = <0 39 4>;
>> >                         };
>> >                 };
>> >
>> > Then we can later add:
>> >
>> >                         sdr-ports: ports@ffc2507c {
>> >                                 #reset-cells = <1>;
>> >                                 compatible = "altr,sdr-ports";
>> >                                 reg = <0xffc2507c 0x10>;
>> >                                 clocks = <&ddr_dqs_clk>;
>> >                                 ...
>> >                         };
>
> --
> 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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Alan Tull]

On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:

>>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>>> >> new file mode 100644
>>> >> index 0000000..8f8746b
>>> >> --- /dev/null
>>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>>> >> @@ -0,0 +1,11 @@
>>> >> +Altera SOCFPGA SDRAM Controller
>>> >> +
>>> >> +Required properties:
>>> >> +- compatible : "altr,sdr-ctl";
>>> >> +- reg : Should contain 1 register ranges(address and length)
>>> >> +
>>> >> +Example:
>>> >> +     sdrctl@ffc25000 {
>>> >> +             compatible = "altr,sdr-ctl";
>>> >> +             reg = <0xffc25000 0x1000>;
>>> >> +     };
>>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>> >> index df43702..6ce912e 100644
>>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>> >> @@ -676,6 +676,11 @@
>>> >>                       clocks = <&l4_sp_clk>;
>>> >>               };
>>> >>
>>> >> +             sdrctl@ffc25000 {
>>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>>> >                                                    ^^^^^^^^^^
>>> >
>>> > Get rid of that, too, please.
>>>
>>> Hi Steffan,
>>>
>>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>>> that has the ECC enable bitfield also includes the DRAM configuration
>>> bitfields that other drivers will want to access (specifically the
>>> FPGA bridge needs this information). Since this register will be
>>> shared between drivers,  syscon seems like the best solution.
>>>
>>
>> Hm, from looking at the documentation of the ctrlcfg I can't really
>> understand which bits you would need for the FPGA brigde and why.

Hi Steffen,

Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
allows an FPGA port to come out of reset (enables that port).  Has no
other effect on SDRAM configuration.

>> That all sounds like stuff you would want to set for the specific
>> RAM you are dealing with on a specific board.
>> What bridge are you talking about? The SDRAM bridge?

Yes, the port allows the FPGA a direct path to the SDRAM.  This one
register the only register in the sdr that the bridge driver needs.

>>
>> I can see the problem with the ECC enable, though.
>>
>> Regards,
>> Steffen
>>
>
> Hi Steffen,
>
> I'll get more details from the guy working on the FPGA bridge when he
> gets back in the office. When I started working on EDAC, that register
> had been allocated by the FPGA bridge driver so we decided to use the
> syscon to allow sharing of the register.
>
> My understanding was that the FPGA bridge as an SDRAM master would
> allow FPGA devices to access the SDRAM.  As part of that process, they
> may want to read the SDRAM configuration.

The FPGA bridge driver doesn't look at the SDRAM configuration.  It
just wants access to this one register so it can enable or disable the
FPGA path to SDRAM.  The only function of this bridge driver is to
enable/disable the bridge.

~Alan

>
> I'll need to get more details from the driver developer because the
> FPGA driver is in flux while the appropriate driver architecture is
> being discussed.
>
> Thor
>
>
>>> >                 sdrctl@ffc25000 {
>>> >                         compatible = "altr,sdr-ctl";
>>> >                         reg = <0xffc25000 0x1000>;
>>> >                         ranges;
>>> >
>>> >                         edac@ffc2502c {
>>> >                                 compatible = "altr,sdram-edac";
>>> >                                 reg = <0xffc2502c 0x50>;
>>> >                                 interrupts = <0 39 4>;
>>> >                         };
>>> >                 };
>>> >
>>> > Then we can later add:
>>> >
>>> >                         sdr-ports: ports@ffc2507c {
>>> >                                 #reset-cells = <1>;
>>> >                                 compatible = "altr,sdr-ports";
>>> >                                 reg = <0xffc2507c 0x10>;
>>> >                                 clocks = <&ddr_dqs_clk>;
>>> >                                 ...
>>> >                         };
>>
>> --
>> 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 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

Hi!

On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
> 
> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >>> >> new file mode 100644
> >>> >> index 0000000..8f8746b
> >>> >> --- /dev/null
> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >>> >> @@ -0,0 +1,11 @@
> >>> >> +Altera SOCFPGA SDRAM Controller
> >>> >> +
> >>> >> +Required properties:
> >>> >> +- compatible : "altr,sdr-ctl";
> >>> >> +- reg : Should contain 1 register ranges(address and length)
> >>> >> +
> >>> >> +Example:
> >>> >> +     sdrctl@ffc25000 {
> >>> >> +             compatible = "altr,sdr-ctl";
> >>> >> +             reg = <0xffc25000 0x1000>;
> >>> >> +     };
> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >>> >> index df43702..6ce912e 100644
> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >>> >> @@ -676,6 +676,11 @@
> >>> >>                       clocks = <&l4_sp_clk>;
> >>> >>               };
> >>> >>
> >>> >> +             sdrctl@ffc25000 {
> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
> >>> >                                                    ^^^^^^^^^^
> >>> >
> >>> > Get rid of that, too, please.
> >>>
> >>> Hi Steffan,
> >>>
> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
> >>> that has the ECC enable bitfield also includes the DRAM configuration
> >>> bitfields that other drivers will want to access (specifically the
> >>> FPGA bridge needs this information). Since this register will be
> >>> shared between drivers,  syscon seems like the best solution.
> >>>
> >>
> >> Hm, from looking at the documentation of the ctrlcfg I can't really
> >> understand which bits you would need for the FPGA brigde and why.
> 
> Hi Steffen,
> 
> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
> allows an FPGA port to come out of reset (enables that port).  Has no
> other effect on SDRAM configuration.
> 
> >> That all sounds like stuff you would want to set for the specific
> >> RAM you are dealing with on a specific board.
> >> What bridge are you talking about? The SDRAM bridge?
> 
> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
> register the only register in the sdr that the bridge driver needs.
> 

So, what I suggested down ...

> >>
> >> I can see the problem with the ECC enable, though.
> >>
> >> Regards,
> >> Steffen
> >>
> >
> >>> >                 sdrctl@ffc25000 {
> >>> >                         compatible = "altr,sdr-ctl";
> >>> >                         reg = <0xffc25000 0x1000>;
> >>> >                         ranges;
> >>> >
> >>> >                         edac@ffc2502c {
> >>> >                                 compatible = "altr,sdram-edac";
> >>> >                                 reg = <0xffc2502c 0x50>;
> >>> >                                 interrupts = <0 39 4>;
> >>> >                         };
> >>> >                 };
> >>> >
> >>> > Then we can later add:
> >>> >
> >>> >                         sdr-ports: ports@ffc2507c {
> >>> >                                 #reset-cells = <1>;
> >>> >                                 compatible = "altr,sdr-ports";
> >>> >                                 reg = <0xffc2507c 0x10>;
> >>> >                                 clocks = <&ddr_dqs_clk>;
> >>> >                                 ...
> >>> >                         };
>

... here should work. No?! Just a simple driver that registers with the
reset-framework. I would add 0x7c to that driver and than that driver could
"configure" the port and let it out of reset.

I have done the same thing for the other 3 bridges, but am not finished yet.
Especially the GPV-stuff needs to at least be able to be added later if not now.

Regard,
Steffen

-- 
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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Hi!
>
> On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
>> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
>>
>> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >>> >> new file mode 100644
>> >>> >> index 0000000..8f8746b
>> >>> >> --- /dev/null
>> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >>> >> @@ -0,0 +1,11 @@
>> >>> >> +Altera SOCFPGA SDRAM Controller
>> >>> >> +
>> >>> >> +Required properties:
>> >>> >> +- compatible : "altr,sdr-ctl";
>> >>> >> +- reg : Should contain 1 register ranges(address and length)
>> >>> >> +
>> >>> >> +Example:
>> >>> >> +     sdrctl@ffc25000 {
>> >>> >> +             compatible = "altr,sdr-ctl";
>> >>> >> +             reg = <0xffc25000 0x1000>;
>> >>> >> +     };
>> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >>> >> index df43702..6ce912e 100644
>> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >>> >> @@ -676,6 +676,11 @@
>> >>> >>                       clocks = <&l4_sp_clk>;
>> >>> >>               };
>> >>> >>
>> >>> >> +             sdrctl@ffc25000 {
>> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>> >>> >                                                    ^^^^^^^^^^
>> >>> >
>> >>> > Get rid of that, too, please.
>> >>>
>> >>> Hi Steffan,
>> >>>
>> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>> >>> that has the ECC enable bitfield also includes the DRAM configuration
>> >>> bitfields that other drivers will want to access (specifically the
>> >>> FPGA bridge needs this information). Since this register will be
>> >>> shared between drivers,  syscon seems like the best solution.
>> >>>
>> >>
>> >> Hm, from looking at the documentation of the ctrlcfg I can't really
>> >> understand which bits you would need for the FPGA brigde and why.
>>
>> Hi Steffen,
>>
>> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
>> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
>> allows an FPGA port to come out of reset (enables that port).  Has no
>> other effect on SDRAM configuration.
>>
>> >> That all sounds like stuff you would want to set for the specific
>> >> RAM you are dealing with on a specific board.
>> >> What bridge are you talking about? The SDRAM bridge?
>>
>> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
>> register the only register in the sdr that the bridge driver needs.
>>
>
> So, what I suggested down ...
>
>> >>
>> >> I can see the problem with the ECC enable, though.
>> >>
>> >> Regards,
>> >> Steffen
>> >>
>> >
>> >>> >                 sdrctl@ffc25000 {
>> >>> >                         compatible = "altr,sdr-ctl";
>> >>> >                         reg = <0xffc25000 0x1000>;
>> >>> >                         ranges;
>> >>> >
>> >>> >                         edac@ffc2502c {
>> >>> >                                 compatible = "altr,sdram-edac";
>> >>> >                                 reg = <0xffc2502c 0x50>;
>> >>> >                                 interrupts = <0 39 4>;
>> >>> >                         };
>> >>> >                 };
>> >>> >
>> >>> > Then we can later add:
>> >>> >
>> >>> >                         sdr-ports: ports@ffc2507c {
>> >>> >                                 #reset-cells = <1>;
>> >>> >                                 compatible = "altr,sdr-ports";
>> >>> >                                 reg = <0xffc2507c 0x10>;
>> >>> >                                 clocks = <&ddr_dqs_clk>;
>> >>> >                                 ...
>> >>> >                         };
>>
>
> ... here should work. No?! Just a simple driver that registers with the
> reset-framework. I would add 0x7c to that driver and than that driver could
> "configure" the port and let it out of reset.
>
> I have done the same thing for the other 3 bridges, but am not finished yet.
> Especially the GPV-stuff needs to at least be able to be added later if not now.
>
> Regard,
> Steffen
>
> --
> 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 |

Hi Steffen!

I'm not clear on how the EDAC driver will interact with the registers
allocated to the SDRAM controller. If the group of registers from
0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
controller, how does the EDAC driver cleanly access that single
register inside this range?

Is the solution that I don't use request_region() (and therefore not
request exclusive access) when setting up the SDRAM controller?

If you could point me to your drivers for the other bridges that you
reference, your code may answer my question.

Thanks for your comments and insight.

Thor

Re: [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller

[Borislav Petkov]

On Thu, May 15, 2014 at 11:04:51AM -0500, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> v2: Use the SDRAM controller registers to calculate memory size
>     instead of the Device Tree. Update To & Cc list. Add maintainer
>     information.
> 
> v3: EDAC driver cleanup based on comments from Mailing list.
> 
> v4: Panic on DBE. Add macro around inject-error reads to prevent
>     them from being optimized out. Remove of_match_ptr since this
>     will always use Device Tree.
> 
> v5: Addition of printk to trigger function to ensure read vars
>     are not optimized out.

Yeah, you could turn those vX: messages into a real commit message -
much better than none at all :-)

Other than that, the edac bits look ok. I'll wait out until you guys've
sorted the devicetree issues so ping me when it is ready to pick up.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > Hi!
> >
> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
> >>
> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >>> >> new file mode 100644
> >> >>> >> index 0000000..8f8746b
> >> >>> >> --- /dev/null
> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >>> >> @@ -0,0 +1,11 @@
> >> >>> >> +Altera SOCFPGA SDRAM Controller
> >> >>> >> +
> >> >>> >> +Required properties:
> >> >>> >> +- compatible : "altr,sdr-ctl";
> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
> >> >>> >> +
> >> >>> >> +Example:
> >> >>> >> +     sdrctl@ffc25000 {
> >> >>> >> +             compatible = "altr,sdr-ctl";
> >> >>> >> +             reg = <0xffc25000 0x1000>;
> >> >>> >> +     };
> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> >>> >> index df43702..6ce912e 100644
> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> >>> >> @@ -676,6 +676,11 @@
> >> >>> >>                       clocks = <&l4_sp_clk>;
> >> >>> >>               };
> >> >>> >>
> >> >>> >> +             sdrctl@ffc25000 {
> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
> >> >>> >                                                    ^^^^^^^^^^
> >> >>> >
> >> >>> > Get rid of that, too, please.
> >> >>>
> >> >>> Hi Steffan,
> >> >>>
> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
> >> >>> bitfields that other drivers will want to access (specifically the
> >> >>> FPGA bridge needs this information). Since this register will be
> >> >>> shared between drivers,  syscon seems like the best solution.
> >> >>>
> >> >>
> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
> >> >> understand which bits you would need for the FPGA brigde and why.
> >>
> >> Hi Steffen,
> >>
> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
> >> allows an FPGA port to come out of reset (enables that port).  Has no
> >> other effect on SDRAM configuration.
> >>
> >> >> That all sounds like stuff you would want to set for the specific
> >> >> RAM you are dealing with on a specific board.
> >> >> What bridge are you talking about? The SDRAM bridge?
> >>
> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
> >> register the only register in the sdr that the bridge driver needs.
> >>
> >
> > So, what I suggested down ...
> >
> >> >>
> >> >> I can see the problem with the ECC enable, though.
> >> >>
> >> >> Regards,
> >> >> Steffen
> >> >>
> >> >
> >> >>> >                 sdrctl@ffc25000 {
> >> >>> >                         compatible = "altr,sdr-ctl";
> >> >>> >                         reg = <0xffc25000 0x1000>;
> >> >>> >                         ranges;
> >> >>> >
> >> >>> >                         edac@ffc2502c {
> >> >>> >                                 compatible = "altr,sdram-edac";
> >> >>> >                                 reg = <0xffc2502c 0x50>;
> >> >>> >                                 interrupts = <0 39 4>;
> >> >>> >                         };
> >> >>> >                 };
> >> >>> >
> >> >>> > Then we can later add:
> >> >>> >
> >> >>> >                         sdr-ports: ports@ffc2507c {
> >> >>> >                                 #reset-cells = <1>;
> >> >>> >                                 compatible = "altr,sdr-ports";
> >> >>> >                                 reg = <0xffc2507c 0x10>;
> >> >>> >                                 clocks = <&ddr_dqs_clk>;
> >> >>> >                                 ...
> >> >>> >                         };
> >>
> >
> > ... here should work. No?! Just a simple driver that registers with the
> > reset-framework. I would add 0x7c to that driver and than that driver could
> > "configure" the port and let it out of reset.
> >
> > I have done the same thing for the other 3 bridges, but am not finished yet.
> > Especially the GPV-stuff needs to at least be able to be added later if not now.
> >

Hi Thor!

> I'm not clear on how the EDAC driver will interact with the registers
> allocated to the SDRAM controller. If the group of registers from
> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
> controller, how does the EDAC driver cleanly access that single
> register inside this range?
> 

The compatible in the example is wrong. I didn't mean to map the whole address space
to some driver.
I think for the configuration register syscon is the right approach. It is a
bag of bits that don't necessitate an own driver, so syscon is perfect.

So, let me change my proposal to

	sdr-ctl: sdram@ffc25000 {
		compatible = "altr,sdr-ctl", "syscon";
		reg = <0xffc25000 0x1>;
	};

	edac: edac@ffc2502c {
		compatible = "altr,sdram-edac";
		reg = <0xffc2502c 0x50>;
		interrupts = <0 39 4>;
		config = <&sdr-ctl>;
		...
	};

	sdr-ports: ports@ffc2507c {
		compatible = "altr,sdr-ports";
		reg = <0xffc2507c 0x10>;
		clocks = <&ddr_dqs_clk>;
		...
	};

Maybe we can just skip the outer node that combines all the others.
So, if we do it like that, you can still use syscon, but only for the register
that needs it. And the EDAC definitely needs access to the config register, so
all is good.

> Is the solution that I don't use request_region() (and therefore not
> request exclusive access) when setting up the SDRAM controller?
> 
> If you could point me to your drivers for the other bridges that you
> reference, your code may answer my question.
>

The other bridges don't need access to any SDRAM controller registers and
I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(

Regards,
Steffen

-- 
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 |

Re: [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller

[Thor Thayer]

On Mon, May 26, 2014 at 4:57 AM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> On Thu, May 15, 2014 at 11:04:51AM -0500, tthayer-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org wrote:
>> From: Thor Thayer <tthayer-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
>>
>> v2: Use the SDRAM controller registers to calculate memory size
>>     instead of the Device Tree. Update To & Cc list. Add maintainer
>>     information.
>>
>> v3: EDAC driver cleanup based on comments from Mailing list.
>>
>> v4: Panic on DBE. Add macro around inject-error reads to prevent
>>     them from being optimized out. Remove of_match_ptr since this
>>     will always use Device Tree.
>>
>> v5: Addition of printk to trigger function to ensure read vars
>>     are not optimized out.
>
> Yeah, you could turn those vX: messages into a real commit message -
> much better than none at all :-)
>
> Other than that, the edac bits look ok. I'll wait out until you guys've
> sorted the devicetree issues so ping me when it is ready to pick up.
>
> Thanks.
>
> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --

Hi Boris,

Sorry about the commit message - I thought I'd fixed it. It will be
fixed in the next version.

I think Steffen and I have worked out how the devicetree should look.
I'll submit the changes soon.

Thanks,

Thor
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
>> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>> > Hi!
>> >
>> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
>> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
>> >>
>> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >>> >> new file mode 100644
>> >> >>> >> index 0000000..8f8746b
>> >> >>> >> --- /dev/null
>> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >>> >> @@ -0,0 +1,11 @@
>> >> >>> >> +Altera SOCFPGA SDRAM Controller
>> >> >>> >> +
>> >> >>> >> +Required properties:
>> >> >>> >> +- compatible : "altr,sdr-ctl";
>> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
>> >> >>> >> +
>> >> >>> >> +Example:
>> >> >>> >> +     sdrctl@ffc25000 {
>> >> >>> >> +             compatible = "altr,sdr-ctl";
>> >> >>> >> +             reg = <0xffc25000 0x1000>;
>> >> >>> >> +     };
>> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> >> index df43702..6ce912e 100644
>> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> >> @@ -676,6 +676,11 @@
>> >> >>> >>                       clocks = <&l4_sp_clk>;
>> >> >>> >>               };
>> >> >>> >>
>> >> >>> >> +             sdrctl@ffc25000 {
>> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>> >> >>> >                                                    ^^^^^^^^^^
>> >> >>> >
>> >> >>> > Get rid of that, too, please.
>> >> >>>
>> >> >>> Hi Steffan,
>> >> >>>
>> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
>> >> >>> bitfields that other drivers will want to access (specifically the
>> >> >>> FPGA bridge needs this information). Since this register will be
>> >> >>> shared between drivers,  syscon seems like the best solution.
>> >> >>>
>> >> >>
>> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
>> >> >> understand which bits you would need for the FPGA brigde and why.
>> >>
>> >> Hi Steffen,
>> >>
>> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
>> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
>> >> allows an FPGA port to come out of reset (enables that port).  Has no
>> >> other effect on SDRAM configuration.
>> >>
>> >> >> That all sounds like stuff you would want to set for the specific
>> >> >> RAM you are dealing with on a specific board.
>> >> >> What bridge are you talking about? The SDRAM bridge?
>> >>
>> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
>> >> register the only register in the sdr that the bridge driver needs.
>> >>
>> >
>> > So, what I suggested down ...
>> >
>> >> >>
>> >> >> I can see the problem with the ECC enable, though.
>> >> >>
>> >> >> Regards,
>> >> >> Steffen
>> >> >>
>> >> >
>> >> >>> >                 sdrctl@ffc25000 {
>> >> >>> >                         compatible = "altr,sdr-ctl";
>> >> >>> >                         reg = <0xffc25000 0x1000>;
>> >> >>> >                         ranges;
>> >> >>> >
>> >> >>> >                         edac@ffc2502c {
>> >> >>> >                                 compatible = "altr,sdram-edac";
>> >> >>> >                                 reg = <0xffc2502c 0x50>;
>> >> >>> >                                 interrupts = <0 39 4>;
>> >> >>> >                         };
>> >> >>> >                 };
>> >> >>> >
>> >> >>> > Then we can later add:
>> >> >>> >
>> >> >>> >                         sdr-ports: ports@ffc2507c {
>> >> >>> >                                 #reset-cells = <1>;
>> >> >>> >                                 compatible = "altr,sdr-ports";
>> >> >>> >                                 reg = <0xffc2507c 0x10>;
>> >> >>> >                                 clocks = <&ddr_dqs_clk>;
>> >> >>> >                                 ...
>> >> >>> >                         };
>> >>
>> >
>> > ... here should work. No?! Just a simple driver that registers with the
>> > reset-framework. I would add 0x7c to that driver and than that driver could
>> > "configure" the port and let it out of reset.
>> >
>> > I have done the same thing for the other 3 bridges, but am not finished yet.
>> > Especially the GPV-stuff needs to at least be able to be added later if not now.
>> >
>
> Hi Thor!
>
>> I'm not clear on how the EDAC driver will interact with the registers
>> allocated to the SDRAM controller. If the group of registers from
>> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
>> controller, how does the EDAC driver cleanly access that single
>> register inside this range?
>>
>
> The compatible in the example is wrong. I didn't mean to map the whole address space
> to some driver.
> I think for the configuration register syscon is the right approach. It is a
> bag of bits that don't necessitate an own driver, so syscon is perfect.
>
> So, let me change my proposal to
>
>         sdr-ctl: sdram@ffc25000 {
>                 compatible = "altr,sdr-ctl", "syscon";
>                 reg = <0xffc25000 0x1>;
>         };
>
>         edac: edac@ffc2502c {
>                 compatible = "altr,sdram-edac";
>                 reg = <0xffc2502c 0x50>;
>                 interrupts = <0 39 4>;
>                 config = <&sdr-ctl>;
>                 ...
>         };
>
>         sdr-ports: ports@ffc2507c {
>                 compatible = "altr,sdr-ports";
>                 reg = <0xffc2507c 0x10>;
>                 clocks = <&ddr_dqs_clk>;
>                 ...
>         };
>
> Maybe we can just skip the outer node that combines all the others.
> So, if we do it like that, you can still use syscon, but only for the register
> that needs it. And the EDAC definitely needs access to the config register, so
> all is good.
>
>> Is the solution that I don't use request_region() (and therefore not
>> request exclusive access) when setting up the SDRAM controller?
>>
>> If you could point me to your drivers for the other bridges that you
>> reference, your code may answer my question.
>>
>
> The other bridges don't need access to any SDRAM controller registers and
> I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
>
> Regards,
> Steffen
>
> --
> 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 |

Thank you for your help on this, Steffen. It is good to have your
insight. I'll implement the changes and resubmit shortly.

Thanks,

Thor

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Alan Tull]

On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
>> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>> > Hi!
>> >
>> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
>> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
>> >>
>> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >>> >> new file mode 100644
>> >> >>> >> index 0000000..8f8746b
>> >> >>> >> --- /dev/null
>> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >>> >> @@ -0,0 +1,11 @@
>> >> >>> >> +Altera SOCFPGA SDRAM Controller
>> >> >>> >> +
>> >> >>> >> +Required properties:
>> >> >>> >> +- compatible : "altr,sdr-ctl";
>> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
>> >> >>> >> +
>> >> >>> >> +Example:
>> >> >>> >> +     sdrctl@ffc25000 {
>> >> >>> >> +             compatible = "altr,sdr-ctl";
>> >> >>> >> +             reg = <0xffc25000 0x1000>;
>> >> >>> >> +     };
>> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> >> index df43702..6ce912e 100644
>> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >> >>> >> @@ -676,6 +676,11 @@
>> >> >>> >>                       clocks = <&l4_sp_clk>;
>> >> >>> >>               };
>> >> >>> >>
>> >> >>> >> +             sdrctl@ffc25000 {
>> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>> >> >>> >                                                    ^^^^^^^^^^
>> >> >>> >
>> >> >>> > Get rid of that, too, please.
>> >> >>>
>> >> >>> Hi Steffan,
>> >> >>>
>> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
>> >> >>> bitfields that other drivers will want to access (specifically the
>> >> >>> FPGA bridge needs this information). Since this register will be
>> >> >>> shared between drivers,  syscon seems like the best solution.
>> >> >>>
>> >> >>
>> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
>> >> >> understand which bits you would need for the FPGA brigde and why.
>> >>
>> >> Hi Steffen,
>> >>
>> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
>> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
>> >> allows an FPGA port to come out of reset (enables that port).  Has no
>> >> other effect on SDRAM configuration.
>> >>
>> >> >> That all sounds like stuff you would want to set for the specific
>> >> >> RAM you are dealing with on a specific board.
>> >> >> What bridge are you talking about? The SDRAM bridge?
>> >>
>> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
>> >> register the only register in the sdr that the bridge driver needs.
>> >>
>> >
>> > So, what I suggested down ...
>> >
>> >> >>
>> >> >> I can see the problem with the ECC enable, though.
>> >> >>
>> >> >> Regards,
>> >> >> Steffen
>> >> >>
>> >> >
>> >> >>> >                 sdrctl@ffc25000 {
>> >> >>> >                         compatible = "altr,sdr-ctl";
>> >> >>> >                         reg = <0xffc25000 0x1000>;
>> >> >>> >                         ranges;
>> >> >>> >
>> >> >>> >                         edac@ffc2502c {
>> >> >>> >                                 compatible = "altr,sdram-edac";
>> >> >>> >                                 reg = <0xffc2502c 0x50>;
>> >> >>> >                                 interrupts = <0 39 4>;
>> >> >>> >                         };
>> >> >>> >                 };
>> >> >>> >
>> >> >>> > Then we can later add:
>> >> >>> >
>> >> >>> >                         sdr-ports: ports@ffc2507c {
>> >> >>> >                                 #reset-cells = <1>;
>> >> >>> >                                 compatible = "altr,sdr-ports";
>> >> >>> >                                 reg = <0xffc2507c 0x10>;
>> >> >>> >                                 clocks = <&ddr_dqs_clk>;
>> >> >>> >                                 ...
>> >> >>> >                         };
>> >>
>> >
>> > ... here should work. No?! Just a simple driver that registers with the
>> > reset-framework. I would add 0x7c to that driver and than that driver could
>> > "configure" the port and let it out of reset.
>> >
>> > I have done the same thing for the other 3 bridges, but am not finished yet.
>> > Especially the GPV-stuff needs to at least be able to be added later if not now.
>> >
>
> Hi Thor!
>
>> I'm not clear on how the EDAC driver will interact with the registers
>> allocated to the SDRAM controller. If the group of registers from
>> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
>> controller, how does the EDAC driver cleanly access that single
>> register inside this range?
>>
>
> The compatible in the example is wrong. I didn't mean to map the whole address space
> to some driver.
> I think for the configuration register syscon is the right approach. It is a
> bag of bits that don't necessitate an own driver, so syscon is perfect.
>
> So, let me change my proposal to
>
>         sdr-ctl: sdram@ffc25000 {
>                 compatible = "altr,sdr-ctl", "syscon";
>                 reg = <0xffc25000 0x1>;
>         };
>
>         edac: edac@ffc2502c {
>                 compatible = "altr,sdram-edac";
>                 reg = <0xffc2502c 0x50>;
>                 interrupts = <0 39 4>;
>                 config = <&sdr-ctl>;
>                 ...
>         };
>
>         sdr-ports: ports@ffc2507c {
>                 compatible = "altr,sdr-ports";
>                 reg = <0xffc2507c 0x10>;
>                 clocks = <&ddr_dqs_clk>;
>                 ...
>         };
>
> Maybe we can just skip the outer node that combines all the others.
> So, if we do it like that, you can still use syscon, but only for the register
> that needs it. And the EDAC definitely needs access to the config register, so
> all is good.
>
>> Is the solution that I don't use request_region() (and therefore not
>> request exclusive access) when setting up the SDRAM controller?
>>
>> If you could point me to your drivers for the other bridges that you
>> reference, your code may answer my question.
>>
>
> The other bridges don't need access to any SDRAM controller registers and
> I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
>
> Regards,
> Steffen
>
> --
> 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 |

OK now I understand.  For register offset 0x0, that's all stuff set up
in the bootloader.  We won't be touching that register except for the
EDAC.  So syscon isn't needed here.

Each driver that uses some sdr registers can specify which registers
it uses in the device tree.  So far we don't have any cases of two
drivers that share a register.

The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50.
The fpga bridges just needs offset 0x80.

Thanks for your feedback!

Alan

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote:
> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
> >> <s.trumtrar@pengutronix.de> wrote:
> >> > Hi!
> >> >
> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
> >> >>
> >> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >>> >> new file mode 100644
> >> >> >>> >> index 0000000..8f8746b
> >> >> >>> >> --- /dev/null
> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >>> >> @@ -0,0 +1,11 @@
> >> >> >>> >> +Altera SOCFPGA SDRAM Controller
> >> >> >>> >> +
> >> >> >>> >> +Required properties:
> >> >> >>> >> +- compatible : "altr,sdr-ctl";
> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
> >> >> >>> >> +
> >> >> >>> >> +Example:
> >> >> >>> >> +     sdrctl@ffc25000 {
> >> >> >>> >> +             compatible = "altr,sdr-ctl";
> >> >> >>> >> +             reg = <0xffc25000 0x1000>;
> >> >> >>> >> +     };
> >> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> index df43702..6ce912e 100644
> >> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> @@ -676,6 +676,11 @@
> >> >> >>> >>                       clocks = <&l4_sp_clk>;
> >> >> >>> >>               };
> >> >> >>> >>
> >> >> >>> >> +             sdrctl@ffc25000 {
> >> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
> >> >> >>> >                                                    ^^^^^^^^^^
> >> >> >>> >
> >> >> >>> > Get rid of that, too, please.
> >> >> >>>
> >> >> >>> Hi Steffan,
> >> >> >>>
> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
> >> >> >>> bitfields that other drivers will want to access (specifically the
> >> >> >>> FPGA bridge needs this information). Since this register will be
> >> >> >>> shared between drivers,  syscon seems like the best solution.
> >> >> >>>
> >> >> >>
> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
> >> >> >> understand which bits you would need for the FPGA brigde and why.
> >> >>
> >> >> Hi Steffen,
> >> >>
> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
> >> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
> >> >> allows an FPGA port to come out of reset (enables that port).  Has no
> >> >> other effect on SDRAM configuration.
> >> >>
> >> >> >> That all sounds like stuff you would want to set for the specific
> >> >> >> RAM you are dealing with on a specific board.
> >> >> >> What bridge are you talking about? The SDRAM bridge?
> >> >>
> >> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
> >> >> register the only register in the sdr that the bridge driver needs.
> >> >>
> >> >
> >> > So, what I suggested down ...
> >> >
> >> >> >>
> >> >> >> I can see the problem with the ECC enable, though.
> >> >> >>
> >> >> >> Regards,
> >> >> >> Steffen
> >> >> >>
> >> >> >
> >> >> >>> >                 sdrctl@ffc25000 {
> >> >> >>> >                         compatible = "altr,sdr-ctl";
> >> >> >>> >                         reg = <0xffc25000 0x1000>;
> >> >> >>> >                         ranges;
> >> >> >>> >
> >> >> >>> >                         edac@ffc2502c {
> >> >> >>> >                                 compatible = "altr,sdram-edac";
> >> >> >>> >                                 reg = <0xffc2502c 0x50>;
> >> >> >>> >                                 interrupts = <0 39 4>;
> >> >> >>> >                         };
> >> >> >>> >                 };
> >> >> >>> >
> >> >> >>> > Then we can later add:
> >> >> >>> >
> >> >> >>> >                         sdr-ports: ports@ffc2507c {
> >> >> >>> >                                 #reset-cells = <1>;
> >> >> >>> >                                 compatible = "altr,sdr-ports";
> >> >> >>> >                                 reg = <0xffc2507c 0x10>;
> >> >> >>> >                                 clocks = <&ddr_dqs_clk>;
> >> >> >>> >                                 ...
> >> >> >>> >                         };
> >> >>
> >> >
> >> > ... here should work. No?! Just a simple driver that registers with the
> >> > reset-framework. I would add 0x7c to that driver and than that driver could
> >> > "configure" the port and let it out of reset.
> >> >
> >> > I have done the same thing for the other 3 bridges, but am not finished yet.
> >> > Especially the GPV-stuff needs to at least be able to be added later if not now.
> >> >
> >
> > Hi Thor!
> >
> >> I'm not clear on how the EDAC driver will interact with the registers
> >> allocated to the SDRAM controller. If the group of registers from
> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
> >> controller, how does the EDAC driver cleanly access that single
> >> register inside this range?
> >>
> >
> > The compatible in the example is wrong. I didn't mean to map the whole address space
> > to some driver.
> > I think for the configuration register syscon is the right approach. It is a
> > bag of bits that don't necessitate an own driver, so syscon is perfect.
> >
> > So, let me change my proposal to
> >
> >         sdr-ctl: sdram@ffc25000 {
> >                 compatible = "altr,sdr-ctl", "syscon";
> >                 reg = <0xffc25000 0x1>;
> >         };
> >
> >         edac: edac@ffc2502c {
> >                 compatible = "altr,sdram-edac";
> >                 reg = <0xffc2502c 0x50>;
> >                 interrupts = <0 39 4>;
> >                 config = <&sdr-ctl>;
> >                 ...
> >         };
> >
> >         sdr-ports: ports@ffc2507c {
> >                 compatible = "altr,sdr-ports";
> >                 reg = <0xffc2507c 0x10>;
> >                 clocks = <&ddr_dqs_clk>;
> >                 ...
> >         };
> >
> > Maybe we can just skip the outer node that combines all the others.
> > So, if we do it like that, you can still use syscon, but only for the register
> > that needs it. And the EDAC definitely needs access to the config register, so
> > all is good.
> >
> >> Is the solution that I don't use request_region() (and therefore not
> >> request exclusive access) when setting up the SDRAM controller?
> >>
> >> If you could point me to your drivers for the other bridges that you
> >> reference, your code may answer my question.
> >>
> >
> > The other bridges don't need access to any SDRAM controller registers and
> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
> >
> > Regards,
> > Steffen
> >
> > --
> > 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 |
> 
> OK now I understand.  For register offset 0x0, that's all stuff set up
> in the bootloader.  We won't be touching that register except for the
> EDAC.  So syscon isn't needed here.
> 
> Each driver that uses some sdr registers can specify which registers
> it uses in the device tree.  So far we don't have any cases of two
> drivers that share a register.
> 
> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50.
> The fpga bridges just needs offset 0x80.
> 

Well, almost ;-)
Use syscon for 0x0 and reference that in the ECC driver, which only is
responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle.
Then a bootloader can probe with the same dts and have a driver match on the
config-register and one for all the DRAM setup (< 0x38), which we don't need in
the kernel. On the other hand the EDAC seems unneccessary for the bootloader.
And this all matches the partitioning of the SDR register space, so I think it
is also a correct hardware description.

Regards,
Steffen

-- 
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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

On Tue, May 27, 2014 at 01:00:32PM -0500, Thor Thayer wrote:
> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
> >> <s.trumtrar@pengutronix.de> wrote:
> >> > Hi!
> >> >
> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
> >> >>
> >> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >>> >> new file mode 100644
> >> >> >>> >> index 0000000..8f8746b
> >> >> >>> >> --- /dev/null
> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >>> >> @@ -0,0 +1,11 @@
> >> >> >>> >> +Altera SOCFPGA SDRAM Controller
> >> >> >>> >> +
> >> >> >>> >> +Required properties:
> >> >> >>> >> +- compatible : "altr,sdr-ctl";
> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
> >> >> >>> >> +
> >> >> >>> >> +Example:
> >> >> >>> >> +     sdrctl@ffc25000 {
> >> >> >>> >> +             compatible = "altr,sdr-ctl";
> >> >> >>> >> +             reg = <0xffc25000 0x1000>;
> >> >> >>> >> +     };
> >> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> index df43702..6ce912e 100644
> >> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> @@ -676,6 +676,11 @@
> >> >> >>> >>                       clocks = <&l4_sp_clk>;
> >> >> >>> >>               };
> >> >> >>> >>
> >> >> >>> >> +             sdrctl@ffc25000 {
> >> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
> >> >> >>> >                                                    ^^^^^^^^^^
> >> >> >>> >
> >> >> >>> > Get rid of that, too, please.
> >> >> >>>
> >> >> >>> Hi Steffan,
> >> >> >>>
> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
> >> >> >>> bitfields that other drivers will want to access (specifically the
> >> >> >>> FPGA bridge needs this information). Since this register will be
> >> >> >>> shared between drivers,  syscon seems like the best solution.
> >> >> >>>
> >> >> >>
> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
> >> >> >> understand which bits you would need for the FPGA brigde and why.
> >> >>
> >> >> Hi Steffen,
> >> >>
> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
> >> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
> >> >> allows an FPGA port to come out of reset (enables that port).  Has no
> >> >> other effect on SDRAM configuration.
> >> >>
> >> >> >> That all sounds like stuff you would want to set for the specific
> >> >> >> RAM you are dealing with on a specific board.
> >> >> >> What bridge are you talking about? The SDRAM bridge?
> >> >>
> >> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
> >> >> register the only register in the sdr that the bridge driver needs.
> >> >>
> >> >
> >> > So, what I suggested down ...
> >> >
> >> >> >>
> >> >> >> I can see the problem with the ECC enable, though.
> >> >> >>
> >> >> >> Regards,
> >> >> >> Steffen
> >> >> >>
> >> >> >
> >> >> >>> >                 sdrctl@ffc25000 {
> >> >> >>> >                         compatible = "altr,sdr-ctl";
> >> >> >>> >                         reg = <0xffc25000 0x1000>;
> >> >> >>> >                         ranges;
> >> >> >>> >
> >> >> >>> >                         edac@ffc2502c {
> >> >> >>> >                                 compatible = "altr,sdram-edac";
> >> >> >>> >                                 reg = <0xffc2502c 0x50>;
> >> >> >>> >                                 interrupts = <0 39 4>;
> >> >> >>> >                         };
> >> >> >>> >                 };
> >> >> >>> >
> >> >> >>> > Then we can later add:
> >> >> >>> >
> >> >> >>> >                         sdr-ports: ports@ffc2507c {
> >> >> >>> >                                 #reset-cells = <1>;
> >> >> >>> >                                 compatible = "altr,sdr-ports";
> >> >> >>> >                                 reg = <0xffc2507c 0x10>;
> >> >> >>> >                                 clocks = <&ddr_dqs_clk>;
> >> >> >>> >                                 ...
> >> >> >>> >                         };
> >> >>
> >> >
> >> > ... here should work. No?! Just a simple driver that registers with the
> >> > reset-framework. I would add 0x7c to that driver and than that driver could
> >> > "configure" the port and let it out of reset.
> >> >
> >> > I have done the same thing for the other 3 bridges, but am not finished yet.
> >> > Especially the GPV-stuff needs to at least be able to be added later if not now.
> >> >
> >
> > Hi Thor!
> >
> >> I'm not clear on how the EDAC driver will interact with the registers
> >> allocated to the SDRAM controller. If the group of registers from
> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
> >> controller, how does the EDAC driver cleanly access that single
> >> register inside this range?
> >>
> >
> > The compatible in the example is wrong. I didn't mean to map the whole address space
> > to some driver.
> > I think for the configuration register syscon is the right approach. It is a
> > bag of bits that don't necessitate an own driver, so syscon is perfect.
> >
> > So, let me change my proposal to
> >
> >         sdr-ctl: sdram@ffc25000 {
> >                 compatible = "altr,sdr-ctl", "syscon";
> >                 reg = <0xffc25000 0x1>;
> >         };
> >
> >         edac: edac@ffc2502c {
> >                 compatible = "altr,sdram-edac";
> >                 reg = <0xffc2502c 0x50>;
> >                 interrupts = <0 39 4>;
> >                 config = <&sdr-ctl>;
> >                 ...
> >         };
> >
> >         sdr-ports: ports@ffc2507c {
> >                 compatible = "altr,sdr-ports";
> >                 reg = <0xffc2507c 0x10>;
> >                 clocks = <&ddr_dqs_clk>;
> >                 ...
> >         };
> >
> > Maybe we can just skip the outer node that combines all the others.
> > So, if we do it like that, you can still use syscon, but only for the register
> > that needs it. And the EDAC definitely needs access to the config register, so
> > all is good.
> >
> >> Is the solution that I don't use request_region() (and therefore not
> >> request exclusive access) when setting up the SDRAM controller?
> >>
> >> If you could point me to your drivers for the other bridges that you
> >> reference, your code may answer my question.
> >>
> >
> > The other bridges don't need access to any SDRAM controller registers and
> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
> >
> > Regards,
> > Steffen
> >
> > --
> > 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 |
> 
> Thank you for your help on this, Steffen. It is good to have your
> insight. I'll implement the changes and resubmit shortly.
> 

Very good. I'm happy that we could come to a conclusion we are all okay with.
(Unless someone else shows up and rains on your parade ;-))

Regards,
Steffen

-- 
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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Alan Tull]

On Tue, May 27, 2014 at 2:42 PM, Steffen Trumtrar
<s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote:
>> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
>> <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
>> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
>> >> <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> >> > Hi!
>> >> >
>> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
>> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >>
>> >> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >> >>> >> new file mode 100644
>> >> >> >>> >> index 0000000..8f8746b
>> >> >> >>> >> --- /dev/null
>> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >> >>> >> @@ -0,0 +1,11 @@
>> >> >> >>> >> +Altera SOCFPGA SDRAM Controller
>> >> >> >>> >> +
>> >> >> >>> >> +Required properties:
>> >> >> >>> >> +- compatible : "altr,sdr-ctl";
>> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
>> >> >> >>> >> +
>> >> >> >>> >> +Example:
>> >> >> >>> >> +     sdrctl@ffc25000 {
>> >> >> >>> >> +             compatible = "altr,sdr-ctl";
>> >> >> >>> >> +             reg = <0xffc25000 0x1000>;
>> >> >> >>> >> +     };
>> >> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >> >> >>> >> index df43702..6ce912e 100644
>> >> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >> >> >>> >> @@ -676,6 +676,11 @@
>> >> >> >>> >>                       clocks = <&l4_sp_clk>;
>> >> >> >>> >>               };
>> >> >> >>> >>
>> >> >> >>> >> +             sdrctl@ffc25000 {
>> >> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>> >> >> >>> >                                                    ^^^^^^^^^^
>> >> >> >>> >
>> >> >> >>> > Get rid of that, too, please.
>> >> >> >>>
>> >> >> >>> Hi Steffan,
>> >> >> >>>
>> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
>> >> >> >>> bitfields that other drivers will want to access (specifically the
>> >> >> >>> FPGA bridge needs this information). Since this register will be
>> >> >> >>> shared between drivers,  syscon seems like the best solution.
>> >> >> >>>
>> >> >> >>
>> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
>> >> >> >> understand which bits you would need for the FPGA brigde and why.
>> >> >>
>> >> >> Hi Steffen,
>> >> >>
>> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
>> >> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
>> >> >> allows an FPGA port to come out of reset (enables that port).  Has no
>> >> >> other effect on SDRAM configuration.
>> >> >>
>> >> >> >> That all sounds like stuff you would want to set for the specific
>> >> >> >> RAM you are dealing with on a specific board.
>> >> >> >> What bridge are you talking about? The SDRAM bridge?
>> >> >>
>> >> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
>> >> >> register the only register in the sdr that the bridge driver needs.
>> >> >>
>> >> >
>> >> > So, what I suggested down ...
>> >> >
>> >> >> >>
>> >> >> >> I can see the problem with the ECC enable, though.
>> >> >> >>
>> >> >> >> Regards,
>> >> >> >> Steffen
>> >> >> >>
>> >> >> >
>> >> >> >>> >                 sdrctl@ffc25000 {
>> >> >> >>> >                         compatible = "altr,sdr-ctl";
>> >> >> >>> >                         reg = <0xffc25000 0x1000>;
>> >> >> >>> >                         ranges;
>> >> >> >>> >
>> >> >> >>> >                         edac@ffc2502c {
>> >> >> >>> >                                 compatible = "altr,sdram-edac";
>> >> >> >>> >                                 reg = <0xffc2502c 0x50>;
>> >> >> >>> >                                 interrupts = <0 39 4>;
>> >> >> >>> >                         };
>> >> >> >>> >                 };
>> >> >> >>> >
>> >> >> >>> > Then we can later add:
>> >> >> >>> >
>> >> >> >>> >                         sdr-ports: ports@ffc2507c {
>> >> >> >>> >                                 #reset-cells = <1>;
>> >> >> >>> >                                 compatible = "altr,sdr-ports";
>> >> >> >>> >                                 reg = <0xffc2507c 0x10>;
>> >> >> >>> >                                 clocks = <&ddr_dqs_clk>;
>> >> >> >>> >                                 ...
>> >> >> >>> >                         };
>> >> >>
>> >> >
>> >> > ... here should work. No?! Just a simple driver that registers with the
>> >> > reset-framework. I would add 0x7c to that driver and than that driver could
>> >> > "configure" the port and let it out of reset.
>> >> >
>> >> > I have done the same thing for the other 3 bridges, but am not finished yet.
>> >> > Especially the GPV-stuff needs to at least be able to be added later if not now.
>> >> >
>> >
>> > Hi Thor!
>> >
>> >> I'm not clear on how the EDAC driver will interact with the registers
>> >> allocated to the SDRAM controller. If the group of registers from
>> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
>> >> controller, how does the EDAC driver cleanly access that single
>> >> register inside this range?
>> >>
>> >
>> > The compatible in the example is wrong. I didn't mean to map the whole address space
>> > to some driver.
>> > I think for the configuration register syscon is the right approach. It is a
>> > bag of bits that don't necessitate an own driver, so syscon is perfect.
>> >
>> > So, let me change my proposal to
>> >
>> >         sdr-ctl: sdram@ffc25000 {
>> >                 compatible = "altr,sdr-ctl", "syscon";
>> >                 reg = <0xffc25000 0x1>;
>> >         };
>> >
>> >         edac: edac@ffc2502c {
>> >                 compatible = "altr,sdram-edac";
>> >                 reg = <0xffc2502c 0x50>;
>> >                 interrupts = <0 39 4>;
>> >                 config = <&sdr-ctl>;
>> >                 ...
>> >         };
>> >
>> >         sdr-ports: ports@ffc2507c {
>> >                 compatible = "altr,sdr-ports";
>> >                 reg = <0xffc2507c 0x10>;
>> >                 clocks = <&ddr_dqs_clk>;
>> >                 ...
>> >         };
>> >
>> > Maybe we can just skip the outer node that combines all the others.
>> > So, if we do it like that, you can still use syscon, but only for the register
>> > that needs it. And the EDAC definitely needs access to the config register, so
>> > all is good.
>> >
>> >> Is the solution that I don't use request_region() (and therefore not
>> >> request exclusive access) when setting up the SDRAM controller?
>> >>
>> >> If you could point me to your drivers for the other bridges that you
>> >> reference, your code may answer my question.
>> >>
>> >
>> > The other bridges don't need access to any SDRAM controller registers and
>> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
>> >
>> > Regards,
>> > Steffen
>> >
>> > --
>> > 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 |
>>
>> OK now I understand.  For register offset 0x0, that's all stuff set up
>> in the bootloader.  We won't be touching that register except for the
>> EDAC.  So syscon isn't needed here.
>>
>> Each driver that uses some sdr registers can specify which registers
>> it uses in the device tree.  So far we don't have any cases of two
>> drivers that share a register.
>>
>> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50.
>> The fpga bridges just needs offset 0x80.
>>
>
> Well, almost ;-)
> Use syscon for 0x0 and reference that in the ECC driver, which only is
> responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle.
> Then a bootloader can probe with the same dts and have a driver match on the
> config-register and one for all the DRAM setup (< 0x38), which we don't need in
> the kernel. On the other hand the EDAC seems unneccessary for the bootloader.
> And this all matches the partitioning of the SDR register space, so I think it
> is also a correct hardware description.
>
> Regards,
> Steffen
>
> --
> 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 |

We could do that.  But is syscon a concept the bootloader will know about?

Actually it is a bit messier than what I originally said:
 ECC driver needs two ranges: offset 0x00 and 0x38 through 0x50.
 Machine layer needs control of 0x10, 0x14, 0x54, and 0x58 for putting
SDRAM into self-refresh.
 fpga bridges needs offset 0x80.

Alan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

On Tue, May 27, 2014 at 03:57:47PM -0500, Alan Tull wrote:
> On Tue, May 27, 2014 at 2:42 PM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote:
> >> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
> >> <s.trumtrar@pengutronix.de> wrote:
> >> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
> >> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
> >> >> <s.trumtrar@pengutronix.de> wrote:
> >> >> > Hi!
> >> >> >
> >> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
> >> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
> >> >> >>
> >> >> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >> >>> >> new file mode 100644
> >> >> >> >>> >> index 0000000..8f8746b
> >> >> >> >>> >> --- /dev/null
> >> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >> >>> >> @@ -0,0 +1,11 @@
> >> >> >> >>> >> +Altera SOCFPGA SDRAM Controller
> >> >> >> >>> >> +
> >> >> >> >>> >> +Required properties:
> >> >> >> >>> >> +- compatible : "altr,sdr-ctl";
> >> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
> >> >> >> >>> >> +
> >> >> >> >>> >> +Example:
> >> >> >> >>> >> +     sdrctl@ffc25000 {
> >> >> >> >>> >> +             compatible = "altr,sdr-ctl";
> >> >> >> >>> >> +             reg = <0xffc25000 0x1000>;
> >> >> >> >>> >> +     };
> >> >> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >> >>> >> index df43702..6ce912e 100644
> >> >> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> >> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >> >>> >> @@ -676,6 +676,11 @@
> >> >> >> >>> >>                       clocks = <&l4_sp_clk>;
> >> >> >> >>> >>               };
> >> >> >> >>> >>
> >> >> >> >>> >> +             sdrctl@ffc25000 {
> >> >> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
> >> >> >> >>> >                                                    ^^^^^^^^^^
> >> >> >> >>> >
> >> >> >> >>> > Get rid of that, too, please.
> >> >> >> >>>
> >> >> >> >>> Hi Steffan,
> >> >> >> >>>
> >> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
> >> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
> >> >> >> >>> bitfields that other drivers will want to access (specifically the
> >> >> >> >>> FPGA bridge needs this information). Since this register will be
> >> >> >> >>> shared between drivers,  syscon seems like the best solution.
> >> >> >> >>>
> >> >> >> >>
> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
> >> >> >> >> understand which bits you would need for the FPGA brigde and why.
> >> >> >>
> >> >> >> Hi Steffen,
> >> >> >>
> >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
> >> >> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
> >> >> >> allows an FPGA port to come out of reset (enables that port).  Has no
> >> >> >> other effect on SDRAM configuration.
> >> >> >>
> >> >> >> >> That all sounds like stuff you would want to set for the specific
> >> >> >> >> RAM you are dealing with on a specific board.
> >> >> >> >> What bridge are you talking about? The SDRAM bridge?
> >> >> >>
> >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
> >> >> >> register the only register in the sdr that the bridge driver needs.
> >> >> >>
> >> >> >
> >> >> > So, what I suggested down ...
> >> >> >
> >> >> >> >>
> >> >> >> >> I can see the problem with the ECC enable, though.
> >> >> >> >>
> >> >> >> >> Regards,
> >> >> >> >> Steffen
> >> >> >> >>
> >> >> >> >
> >> >> >> >>> >                 sdrctl@ffc25000 {
> >> >> >> >>> >                         compatible = "altr,sdr-ctl";
> >> >> >> >>> >                         reg = <0xffc25000 0x1000>;
> >> >> >> >>> >                         ranges;
> >> >> >> >>> >
> >> >> >> >>> >                         edac@ffc2502c {
> >> >> >> >>> >                                 compatible = "altr,sdram-edac";
> >> >> >> >>> >                                 reg = <0xffc2502c 0x50>;
> >> >> >> >>> >                                 interrupts = <0 39 4>;
> >> >> >> >>> >                         };
> >> >> >> >>> >                 };
> >> >> >> >>> >
> >> >> >> >>> > Then we can later add:
> >> >> >> >>> >
> >> >> >> >>> >                         sdr-ports: ports@ffc2507c {
> >> >> >> >>> >                                 #reset-cells = <1>;
> >> >> >> >>> >                                 compatible = "altr,sdr-ports";
> >> >> >> >>> >                                 reg = <0xffc2507c 0x10>;
> >> >> >> >>> >                                 clocks = <&ddr_dqs_clk>;
> >> >> >> >>> >                                 ...
> >> >> >> >>> >                         };
> >> >> >>
> >> >> >
> >> >> > ... here should work. No?! Just a simple driver that registers with the
> >> >> > reset-framework. I would add 0x7c to that driver and than that driver could
> >> >> > "configure" the port and let it out of reset.
> >> >> >
> >> >> > I have done the same thing for the other 3 bridges, but am not finished yet.
> >> >> > Especially the GPV-stuff needs to at least be able to be added later if not now.
> >> >> >
> >> >
> >> > Hi Thor!
> >> >
> >> >> I'm not clear on how the EDAC driver will interact with the registers
> >> >> allocated to the SDRAM controller. If the group of registers from
> >> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
> >> >> controller, how does the EDAC driver cleanly access that single
> >> >> register inside this range?
> >> >>
> >> >
> >> > The compatible in the example is wrong. I didn't mean to map the whole address space
> >> > to some driver.
> >> > I think for the configuration register syscon is the right approach. It is a
> >> > bag of bits that don't necessitate an own driver, so syscon is perfect.
> >> >
> >> > So, let me change my proposal to
> >> >
> >> >         sdr-ctl: sdram@ffc25000 {
> >> >                 compatible = "altr,sdr-ctl", "syscon";
> >> >                 reg = <0xffc25000 0x1>;
> >> >         };
> >> >
> >> >         edac: edac@ffc2502c {
> >> >                 compatible = "altr,sdram-edac";
> >> >                 reg = <0xffc2502c 0x50>;
> >> >                 interrupts = <0 39 4>;
> >> >                 config = <&sdr-ctl>;
> >> >                 ...
> >> >         };
> >> >
> >> >         sdr-ports: ports@ffc2507c {
> >> >                 compatible = "altr,sdr-ports";
> >> >                 reg = <0xffc2507c 0x10>;
> >> >                 clocks = <&ddr_dqs_clk>;
> >> >                 ...
> >> >         };
> >> >
> >> > Maybe we can just skip the outer node that combines all the others.
> >> > So, if we do it like that, you can still use syscon, but only for the register
> >> > that needs it. And the EDAC definitely needs access to the config register, so
> >> > all is good.
> >> >
> >> >> Is the solution that I don't use request_region() (and therefore not
> >> >> request exclusive access) when setting up the SDRAM controller?
> >> >>
> >> >> If you could point me to your drivers for the other bridges that you
> >> >> reference, your code may answer my question.
> >> >>
> >> >
> >> > The other bridges don't need access to any SDRAM controller registers and
> >> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
> >> >
> >> > Regards,
> >> > Steffen
> >> >
> >> > --
> >> > 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 |
> >>
> >> OK now I understand.  For register offset 0x0, that's all stuff set up
> >> in the bootloader.  We won't be touching that register except for the
> >> EDAC.  So syscon isn't needed here.
> >>
> >> Each driver that uses some sdr registers can specify which registers
> >> it uses in the device tree.  So far we don't have any cases of two
> >> drivers that share a register.
> >>
> >> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50.
> >> The fpga bridges just needs offset 0x80.
> >>
> >
> > Well, almost ;-)
> > Use syscon for 0x0 and reference that in the ECC driver, which only is
> > responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle.
> > Then a bootloader can probe with the same dts and have a driver match on the
> > config-register and one for all the DRAM setup (< 0x38), which we don't need in
> > the kernel. On the other hand the EDAC seems unneccessary for the bootloader.
> > And this all matches the partitioning of the SDR register space, so I think it
> > is also a correct hardware description.
> >
> > Regards,
> > Steffen
> >
> > --
> > 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 |
> 
> We could do that.  But is syscon a concept the bootloader will know about?
> 

Depends on the bootloader. I guess u-boot doesn't, but barebox does.
Alternative would be to have the bootloader know that it has to look for an
ECC node when it instead is looking for DRAM timing/width/... setup.
Sounds wrong to me.

> Actually it is a bit messier than what I originally said:
>  ECC driver needs two ranges: offset 0x00 and 0x38 through 0x50.
>  Machine layer needs control of 0x10, 0x14, 0x54, and 0x58 for putting
> SDRAM into self-refresh.

I didn't get that. Does the ECC driver need access to this "machine layer"?

Regards,
Steffen


-- 
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 |

Re: [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Alan Tull]

On Wed, May 28, 2014 at 2:01 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> On Tue, May 27, 2014 at 03:57:47PM -0500, Alan Tull wrote:
>> On Tue, May 27, 2014 at 2:42 PM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>> > On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote:
>> >> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar
>> >> <s.trumtrar@pengutronix.de> wrote:
>> >> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote:
>> >> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar
>> >> >> <s.trumtrar@pengutronix.de> wrote:
>> >> >> > Hi!
>> >> >> >
>> >> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote:
>> >> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote:
>> >> >> >>
>> >> >> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >> >> >>> >> new file mode 100644
>> >> >> >> >>> >> index 0000000..8f8746b
>> >> >> >> >>> >> --- /dev/null
>> >> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
>> >> >> >> >>> >> @@ -0,0 +1,11 @@
>> >> >> >> >>> >> +Altera SOCFPGA SDRAM Controller
>> >> >> >> >>> >> +
>> >> >> >> >>> >> +Required properties:
>> >> >> >> >>> >> +- compatible : "altr,sdr-ctl";
>> >> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
>> >> >> >> >>> >> +
>> >> >> >> >>> >> +Example:
>> >> >> >> >>> >> +     sdrctl@ffc25000 {
>> >> >> >> >>> >> +             compatible = "altr,sdr-ctl";
>> >> >> >> >>> >> +             reg = <0xffc25000 0x1000>;
>> >> >> >> >>> >> +     };
>> >> >> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> >> >> >> >>> >> index df43702..6ce912e 100644
>> >> >> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
>> >> >> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> >> >> >> >>> >> @@ -676,6 +676,11 @@
>> >> >> >> >>> >>                       clocks = <&l4_sp_clk>;
>> >> >> >> >>> >>               };
>> >> >> >> >>> >>
>> >> >> >> >>> >> +             sdrctl@ffc25000 {
>> >> >> >> >>> >> +                     compatible = "altr,sdr-ctl", "syscon";
>> >> >> >> >>> >                                                    ^^^^^^^^^^
>> >> >> >> >>> >
>> >> >> >> >>> > Get rid of that, too, please.
>> >> >> >> >>>
>> >> >> >> >>> Hi Steffan,
>> >> >> >> >>>
>> >> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
>> >> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
>> >> >> >> >>> bitfields that other drivers will want to access (specifically the
>> >> >> >> >>> FPGA bridge needs this information). Since this register will be
>> >> >> >> >>> shared between drivers,  syscon seems like the best solution.
>> >> >> >> >>>
>> >> >> >> >>
>> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
>> >> >> >> >> understand which bits you would need for the FPGA brigde and why.
>> >> >> >>
>> >> >> >> Hi Steffen,
>> >> >> >>
>> >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register.  14 bits
>> >> >> >> wide, defaults to 0.  When appropriate bits set to 1 in that reg, it
>> >> >> >> allows an FPGA port to come out of reset (enables that port).  Has no
>> >> >> >> other effect on SDRAM configuration.
>> >> >> >>
>> >> >> >> >> That all sounds like stuff you would want to set for the specific
>> >> >> >> >> RAM you are dealing with on a specific board.
>> >> >> >> >> What bridge are you talking about? The SDRAM bridge?
>> >> >> >>
>> >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM.  This one
>> >> >> >> register the only register in the sdr that the bridge driver needs.
>> >> >> >>
>> >> >> >
>> >> >> > So, what I suggested down ...
>> >> >> >
>> >> >> >> >>
>> >> >> >> >> I can see the problem with the ECC enable, though.
>> >> >> >> >>
>> >> >> >> >> Regards,
>> >> >> >> >> Steffen
>> >> >> >> >>
>> >> >> >> >
>> >> >> >> >>> >                 sdrctl@ffc25000 {
>> >> >> >> >>> >                         compatible = "altr,sdr-ctl";
>> >> >> >> >>> >                         reg = <0xffc25000 0x1000>;
>> >> >> >> >>> >                         ranges;
>> >> >> >> >>> >
>> >> >> >> >>> >                         edac@ffc2502c {
>> >> >> >> >>> >                                 compatible = "altr,sdram-edac";
>> >> >> >> >>> >                                 reg = <0xffc2502c 0x50>;
>> >> >> >> >>> >                                 interrupts = <0 39 4>;
>> >> >> >> >>> >                         };
>> >> >> >> >>> >                 };
>> >> >> >> >>> >
>> >> >> >> >>> > Then we can later add:
>> >> >> >> >>> >
>> >> >> >> >>> >                         sdr-ports: ports@ffc2507c {
>> >> >> >> >>> >                                 #reset-cells = <1>;
>> >> >> >> >>> >                                 compatible = "altr,sdr-ports";
>> >> >> >> >>> >                                 reg = <0xffc2507c 0x10>;
>> >> >> >> >>> >                                 clocks = <&ddr_dqs_clk>;
>> >> >> >> >>> >                                 ...
>> >> >> >> >>> >                         };
>> >> >> >>
>> >> >> >
>> >> >> > ... here should work. No?! Just a simple driver that registers with the
>> >> >> > reset-framework. I would add 0x7c to that driver and than that driver could
>> >> >> > "configure" the port and let it out of reset.
>> >> >> >
>> >> >> > I have done the same thing for the other 3 bridges, but am not finished yet.
>> >> >> > Especially the GPV-stuff needs to at least be able to be added later if not now.
>> >> >> >
>> >> >
>> >> > Hi Thor!
>> >> >
>> >> >> I'm not clear on how the EDAC driver will interact with the registers
>> >> >> allocated to the SDRAM controller. If the group of registers from
>> >> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
>> >> >> controller, how does the EDAC driver cleanly access that single
>> >> >> register inside this range?
>> >> >>
>> >> >
>> >> > The compatible in the example is wrong. I didn't mean to map the whole address space
>> >> > to some driver.
>> >> > I think for the configuration register syscon is the right approach. It is a
>> >> > bag of bits that don't necessitate an own driver, so syscon is perfect.
>> >> >
>> >> > So, let me change my proposal to
>> >> >
>> >> >         sdr-ctl: sdram@ffc25000 {
>> >> >                 compatible = "altr,sdr-ctl", "syscon";
>> >> >                 reg = <0xffc25000 0x1>;
>> >> >         };
>> >> >
>> >> >         edac: edac@ffc2502c {
>> >> >                 compatible = "altr,sdram-edac";
>> >> >                 reg = <0xffc2502c 0x50>;
>> >> >                 interrupts = <0 39 4>;
>> >> >                 config = <&sdr-ctl>;
>> >> >                 ...
>> >> >         };
>> >> >
>> >> >         sdr-ports: ports@ffc2507c {
>> >> >                 compatible = "altr,sdr-ports";
>> >> >                 reg = <0xffc2507c 0x10>;
>> >> >                 clocks = <&ddr_dqs_clk>;
>> >> >                 ...
>> >> >         };
>> >> >
>> >> > Maybe we can just skip the outer node that combines all the others.
>> >> > So, if we do it like that, you can still use syscon, but only for the register
>> >> > that needs it. And the EDAC definitely needs access to the config register, so
>> >> > all is good.
>> >> >
>> >> >> Is the solution that I don't use request_region() (and therefore not
>> >> >> request exclusive access) when setting up the SDRAM controller?
>> >> >>
>> >> >> If you could point me to your drivers for the other bridges that you
>> >> >> reference, your code may answer my question.
>> >> >>
>> >> >
>> >> > The other bridges don't need access to any SDRAM controller registers and
>> >> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
>> >> >
>> >> > Regards,
>> >> > Steffen
>> >> >
>> >> > --
>> >> > 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 |
>> >>
>> >> OK now I understand.  For register offset 0x0, that's all stuff set up
>> >> in the bootloader.  We won't be touching that register except for the
>> >> EDAC.  So syscon isn't needed here.
>> >>
>> >> Each driver that uses some sdr registers can specify which registers
>> >> it uses in the device tree.  So far we don't have any cases of two
>> >> drivers that share a register.
>> >>
>> >> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50.
>> >> The fpga bridges just needs offset 0x80.
>> >>
>> >
>> > Well, almost ;-)
>> > Use syscon for 0x0 and reference that in the ECC driver, which only is
>> > responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle.
>> > Then a bootloader can probe with the same dts and have a driver match on the
>> > config-register and one for all the DRAM setup (< 0x38), which we don't need in
>> > the kernel. On the other hand the EDAC seems unneccessary for the bootloader.
>> > And this all matches the partitioning of the SDR register space, so I think it
>> > is also a correct hardware description.
>> >
>> > Regards,
>> > Steffen
>> >
>> > --
>> > 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 |
>>
>> We could do that.  But is syscon a concept the bootloader will know about?
>>
>
> Depends on the bootloader. I guess u-boot doesn't, but barebox does.

OK then we will want offset 0 to be 'syscon'.

> Alternative would be to have the bootloader know that it has to look for an
> ECC node when it instead is looking for DRAM timing/width/... setup.
> Sounds wrong to me.
>
>> Actually it is a bit messier than what I originally said:
>>  ECC driver needs two ranges: offset 0x00 and 0x38 through 0x50.
>>  Machine layer needs control of 0x10, 0x14, 0x54, and 0x58 for putting
>> SDRAM into self-refresh.
>
> I didn't get that. Does the ECC driver need access to this "machine layer"?

Nope.  Now that I think about it this machine layer will only need
access to 0x54, 0x58 and nobody else will need them.

So I agree with your suggestion.  I think that besides offset 0, these
registers will be not be shared between drivers.

Thanks!
Alan Tull

>
> Regards,
> Steffen
>
>
> --
> 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 |