* [PATCHv9 0/3] Addition of Altera EDAC support.
@ 2014-07-30 18:22 ` tthayer
0 siblings, 0 replies; 34+ messages in thread
From: tthayer @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux, tthayer
From: Thor Thayer <tthayer@opensource.altera.com>
The Altera SDRAM controller and EDAC support are added in this
patch series. The SDRAM controller is an MFD so that multiple
drivers can access it's registers.
Thor Thayer (3):
mfd: altera: Add Altera SDRAM Controller
edac: altera: Add Altera EDAC support.
arm: dts: Add Altera SDRAM controller bindings
.../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +
MAINTAINERS | 6 +
arch/arm/boot/dts/socfpga.dtsi | 10 +
drivers/edac/Kconfig | 10 +
drivers/edac/Makefile | 2 +
drivers/edac/altera_edac.c | 293 ++++++++++++++++++++
drivers/mfd/Kconfig | 7 +
drivers/mfd/Makefile | 1 +
drivers/mfd/altera-sdr.c | 162 +++++++++++
include/linux/mfd/altera-sdr.h | 102 +++++++
10 files changed, 606 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
create mode 100644 drivers/edac/altera_edac.c
create mode 100644 drivers/mfd/altera-sdr.c
create mode 100644 include/linux/mfd/altera-sdr.h
--
1.7.9.5
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 0/3] Addition of Altera EDAC support.
@ 2014-07-30 18:22 ` tthayer
0 siblings, 0 replies; 34+ messages in thread
From: tthayer @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux, tthayer
From: Thor Thayer <tthayer@opensource.altera.com>
The Altera SDRAM controller and EDAC support are added in this
patch series. The SDRAM controller is an MFD so that multiple
drivers can access it's registers.
Thor Thayer (3):
mfd: altera: Add Altera SDRAM Controller
edac: altera: Add Altera EDAC support.
arm: dts: Add Altera SDRAM controller bindings
.../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +
MAINTAINERS | 6 +
arch/arm/boot/dts/socfpga.dtsi | 10 +
drivers/edac/Kconfig | 10 +
drivers/edac/Makefile | 2 +
drivers/edac/altera_edac.c | 293 ++++++++++++++++++++
drivers/mfd/Kconfig | 7 +
drivers/mfd/Makefile | 1 +
drivers/mfd/altera-sdr.c | 162 +++++++++++
include/linux/mfd/altera-sdr.h | 102 +++++++
10 files changed, 606 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
create mode 100644 drivers/edac/altera_edac.c
create mode 100644 drivers/mfd/altera-sdr.c
create mode 100644 include/linux/mfd/altera-sdr.h
--
1.7.9.5
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 0/3] Addition of Altera EDAC support.
@ 2014-07-30 18:22 ` tthayer
0 siblings, 0 replies; 34+ messages in thread
From: tthayer at opensource.altera.com @ 2014-07-30 18:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Thor Thayer <tthayer@opensource.altera.com>
The Altera SDRAM controller and EDAC support are added in this
patch series. The SDRAM controller is an MFD so that multiple
drivers can access it's registers.
Thor Thayer (3):
mfd: altera: Add Altera SDRAM Controller
edac: altera: Add Altera EDAC support.
arm: dts: Add Altera SDRAM controller bindings
.../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +
MAINTAINERS | 6 +
arch/arm/boot/dts/socfpga.dtsi | 10 +
drivers/edac/Kconfig | 10 +
drivers/edac/Makefile | 2 +
drivers/edac/altera_edac.c | 293 ++++++++++++++++++++
drivers/mfd/Kconfig | 7 +
drivers/mfd/Makefile | 1 +
drivers/mfd/altera-sdr.c | 162 +++++++++++
include/linux/mfd/altera-sdr.h | 102 +++++++
10 files changed, 606 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
create mode 100644 drivers/edac/altera_edac.c
create mode 100644 drivers/mfd/altera-sdr.c
create mode 100644 include/linux/mfd/altera-sdr.h
--
1.7.9.5
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
0 siblings, 0 replies; 34+ messages in thread
From: tthayer @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux, tthayer, Alan Tull
From: Thor Thayer <tthayer@opensource.altera.com>
Add a simple MFD for the Altera SDRAM Controller.
Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v1-8: The MFD implementation was not included in the original series.
v9: New MFD implementation.
---
MAINTAINERS | 5 ++
drivers/mfd/Kconfig | 7 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
5 files changed, 277 insertions(+)
create mode 100644 drivers/mfd/altera-sdr.c
create mode 100644 include/linux/mfd/altera-sdr.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 86efa7e..48a8923 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen@altera.com>
S: Maintained
F: drivers/clk/socfpga/
+ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
+M: Thor Thayer <tthayer@altera.com>
+S: Maintained
+F: drivers/mfd/altera-sdr.c
+
ARM/STI ARCHITECTURE
M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
M: Maxime Coquelin <maxime.coquelin@st.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6cc4b6a..8ce4961 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -719,6 +719,13 @@ config MFD_STMPE
Keypad: stmpe-keypad
Touchscreen: stmpe-ts
+config MFD_ALTERA_SDR
+ bool "Altera SDRAM Controller MFD"
+ depends on ARCH_SOCFPGA
+ select MFD_CORE
+ help
+ Support for Altera SDRAM Controller (SDR) MFD.
+
menu "STMicroelectronics STMPE Interface Drivers"
depends on MFD_STMPE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8afedba..24cc2b7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
obj-$(CONFIG_MFD_AS3722) += as3722.o
obj-$(CONFIG_MFD_STW481X) += stw481x.o
obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
+obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
new file mode 100644
index 0000000..b5c6646
--- /dev/null
+++ b/drivers/mfd/altera-sdr.c
@@ -0,0 +1,162 @@
+/*
+ * SDRAM Controller (SDR) MFD
+ *
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static const struct mfd_cell altera_sdr_devs[] = {
+#if defined(CONFIG_EDAC_ALTERA_MC)
+ {
+ .name = "altr_sdram_edac",
+ .of_compatible = "altr,sdram-edac",
+ },
+#endif
+};
+
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+ return readl(sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+ writel(value, sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);
+
+/* Get total memory size in bytes */
+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
+{
+ u32 size;
+ u32 read_reg, row, bank, col, cs, width;
+
+ read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
+ if (read_reg < 0)
+ return 0;
+
+ width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
+ if (width < 0)
+ return 0;
+
+ col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
+ SDR_DRAMADDRW_COLBITS_LSB;
+ row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
+ SDR_DRAMADDRW_ROWBITS_LSB;
+ bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
+ SDR_DRAMADDRW_BANKBITS_LSB;
+ cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
+ SDR_DRAMADDRW_CSBITS_LSB;
+
+ /* Correct for ECC as its not addressible */
+ if (width == SDR_DRAMIFWIDTH_32B_ECC)
+ width = 32;
+ if (width == SDR_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;
+}
+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
+
+static int altera_sdr_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct altera_sdr *sdr;
+ struct resource *res;
+ void __iomem *base;
+ int ret;
+
+ sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
+ if (!sdr)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENOENT;
+
+ base = devm_ioremap(dev, res->start, resource_size(res));
+ if (!base)
+ return -ENOMEM;
+
+ sdr->dev = &pdev->dev;
+ sdr->reg_base = base;
+
+ ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
+ ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
+ if (ret)
+ dev_err(sdr->dev, "error adding devices");
+
+ platform_set_drvdata(pdev, sdr);
+
+ dev_dbg(dev, "Altera SDR MFD registered\n");
+
+ return 0;
+}
+
+static int altera_sdr_remove(struct platform_device *pdev)
+{
+ struct altera_sdr *sdr = platform_get_drvdata(pdev);
+
+ mfd_remove_devices(sdr->dev);
+
+ return 0;
+}
+
+static const struct of_device_id of_altera_sdr_match[] = {
+ { .compatible = "altr,sdr", },
+ { },
+};
+
+static const struct platform_device_id altera_sdr_ids[] = {
+ { "altera_sdr", },
+ { }
+};
+
+static struct platform_driver altera_sdr_driver = {
+ .driver = {
+ .name = "altera_sdr",
+ .owner = THIS_MODULE,
+ .of_match_table = of_altera_sdr_match,
+ },
+ .probe = altera_sdr_probe,
+ .remove = altera_sdr_remove,
+ .id_table = altera_sdr_ids,
+};
+
+static int __init altera_sdr_init(void)
+{
+ return platform_driver_register(&altera_sdr_driver);
+}
+postcore_initcall(altera_sdr_init);
+
+static void __exit altera_sdr_exit(void)
+{
+ platform_driver_unregister(&altera_sdr_driver);
+}
+module_exit(altera_sdr_exit);
+
+MODULE_AUTHOR("Alan Tull <atull@altera.com>");
+MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
new file mode 100644
index 0000000..a5f5c39
--- /dev/null
+++ b/include/linux/mfd/altera-sdr.h
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __LINUX_MFD_ALTERA_SDR_H
+#define __LINUX_MFD_ALTERA_SDR_H
+
+/* SDRAM Controller register offsets */
+#define SDR_CTLCFG_OFST 0x00
+#define SDR_DRAMADDRW_OFST 0x2C
+#define SDR_DRAMIFWIDTH_OFST 0x30
+#define SDR_DRAMSTS_OFST 0x38
+#define SDR_DRAMINTR_OFST 0x3C
+#define SDR_SBECOUNT_OFST 0x40
+#define SDR_DBECOUNT_OFST 0x44
+#define SDR_ERRADDR_OFST 0x48
+#define SDR_DROPCOUNT_OFST 0x4C
+#define SDR_DROPADDR_OFST 0x50
+#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
+#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
+
+/* SDRAM Controller CtrlCfg Register Bit Masks */
+#define SDR_CTLCFG_ECC_EN 0x400
+#define SDR_CTLCFG_ECC_CORR_EN 0x800
+#define SDR_CTLCFG_GEN_SB_ERR 0x2000
+#define SDR_CTLCFG_GEN_DB_ERR 0x4000
+
+#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
+ SDR_CTLCFG_ECC_CORR_EN)
+
+/* SDRAM Controller Address Widths Field Register */
+#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
+#define SDR_DRAMADDRW_COLBITS_LSB 0
+#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
+#define SDR_DRAMADDRW_ROWBITS_LSB 5
+#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
+#define SDR_DRAMADDRW_BANKBITS_LSB 10
+#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
+#define SDR_DRAMADDRW_CSBITS_LSB 13
+
+/* SDRAM Controller Interface Data Width Defines */
+#define SDR_DRAMIFWIDTH_16B_ECC 24
+#define SDR_DRAMIFWIDTH_32B_ECC 40
+
+/* SDRAM Controller DRAM Status Register Bit Masks */
+#define SDR_DRAMSTS_SBEERR 0x04
+#define SDR_DRAMSTS_DBEERR 0x08
+#define SDR_DRAMSTS_CORR_DROP 0x10
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define SDR_DRAMINTR_INTREN 0x01
+#define SDR_DRAMINTR_SBEMASK 0x02
+#define SDR_DRAMINTR_DBEMASK 0x04
+#define SDR_DRAMINTR_CORRDROPMASK 0x08
+#define SDR_DRAMINTR_INTRCLR 0x10
+
+/* SDRAM Controller Single Bit Error Count Register Bit Masks */
+#define SDR_SBECOUNT_COUNT_MASK 0x0F
+
+/* SDRAM Controller Double Bit Error Count Register Bit Masks */
+#define SDR_DBECOUNT_COUNT_MASK 0x0F
+
+/* SDRAM Controller ECC Error Address Register Bit Masks */
+#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
+#define SDR_DROPCOUNT_CORRMASK 0x0F
+
+/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
+#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
+
+#define SELFRSHREQ_POS 3
+#define SELFRSHREQ_MASK 0x8
+
+#define SELFRFSHMASK_POS 4
+#define SELFRFSHMASK_MASK 0x30
+
+#define SELFRFSHACK_POS 1
+#define SELFRFSHACK_MASK 0x2
+
+struct altera_sdr {
+ struct device *dev;
+ void __iomem *reg_base;
+};
+
+/* Register access API */
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
+u32 altera_sdr_mem_size(struct altera_sdr *sdr);
+
+#endif /* __LINUX_MFD_ALTERA_SDR_H */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
0 siblings, 0 replies; 34+ messages in thread
From: tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2-Re5JQEeQqe8AvxtiuMwx3w, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, rob-VoJi6FS/r0vR7s880joybQ,
linux-lFZ/pmaqli7XmaaqVzeoHQ, atull-EIB2kfCEclfQT0dZR+AlfA,
delicious.quinoa-Re5JQEeQqe8AvxtiuMwx3w,
dinguyen-EIB2kfCEclfQT0dZR+AlfA,
dougthompson-aS9lmoZGLiVWk0Htik3J/w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A, bp-Gina5bIWoIWzQB+pC5nmwQ,
sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-edac-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w,
tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx, Alan Tull
From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Add a simple MFD for the Altera SDRAM Controller.
Signed-off-by: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
---
v1-8: The MFD implementation was not included in the original series.
v9: New MFD implementation.
---
MAINTAINERS | 5 ++
drivers/mfd/Kconfig | 7 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
5 files changed, 277 insertions(+)
create mode 100644 drivers/mfd/altera-sdr.c
create mode 100644 include/linux/mfd/altera-sdr.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 86efa7e..48a8923 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
S: Maintained
F: drivers/clk/socfpga/
+ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
+M: Thor Thayer <tthayer-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
+S: Maintained
+F: drivers/mfd/altera-sdr.c
+
ARM/STI ARCHITECTURE
M: Srinivas Kandagatla <srinivas.kandagatla-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
M: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6cc4b6a..8ce4961 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -719,6 +719,13 @@ config MFD_STMPE
Keypad: stmpe-keypad
Touchscreen: stmpe-ts
+config MFD_ALTERA_SDR
+ bool "Altera SDRAM Controller MFD"
+ depends on ARCH_SOCFPGA
+ select MFD_CORE
+ help
+ Support for Altera SDRAM Controller (SDR) MFD.
+
menu "STMicroelectronics STMPE Interface Drivers"
depends on MFD_STMPE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8afedba..24cc2b7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
obj-$(CONFIG_MFD_AS3722) += as3722.o
obj-$(CONFIG_MFD_STW481X) += stw481x.o
obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
+obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
new file mode 100644
index 0000000..b5c6646
--- /dev/null
+++ b/drivers/mfd/altera-sdr.c
@@ -0,0 +1,162 @@
+/*
+ * SDRAM Controller (SDR) MFD
+ *
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static const struct mfd_cell altera_sdr_devs[] = {
+#if defined(CONFIG_EDAC_ALTERA_MC)
+ {
+ .name = "altr_sdram_edac",
+ .of_compatible = "altr,sdram-edac",
+ },
+#endif
+};
+
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+ return readl(sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+ writel(value, sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);
+
+/* Get total memory size in bytes */
+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
+{
+ u32 size;
+ u32 read_reg, row, bank, col, cs, width;
+
+ read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
+ if (read_reg < 0)
+ return 0;
+
+ width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
+ if (width < 0)
+ return 0;
+
+ col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
+ SDR_DRAMADDRW_COLBITS_LSB;
+ row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
+ SDR_DRAMADDRW_ROWBITS_LSB;
+ bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
+ SDR_DRAMADDRW_BANKBITS_LSB;
+ cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
+ SDR_DRAMADDRW_CSBITS_LSB;
+
+ /* Correct for ECC as its not addressible */
+ if (width == SDR_DRAMIFWIDTH_32B_ECC)
+ width = 32;
+ if (width == SDR_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;
+}
+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
+
+static int altera_sdr_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct altera_sdr *sdr;
+ struct resource *res;
+ void __iomem *base;
+ int ret;
+
+ sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
+ if (!sdr)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENOENT;
+
+ base = devm_ioremap(dev, res->start, resource_size(res));
+ if (!base)
+ return -ENOMEM;
+
+ sdr->dev = &pdev->dev;
+ sdr->reg_base = base;
+
+ ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
+ ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
+ if (ret)
+ dev_err(sdr->dev, "error adding devices");
+
+ platform_set_drvdata(pdev, sdr);
+
+ dev_dbg(dev, "Altera SDR MFD registered\n");
+
+ return 0;
+}
+
+static int altera_sdr_remove(struct platform_device *pdev)
+{
+ struct altera_sdr *sdr = platform_get_drvdata(pdev);
+
+ mfd_remove_devices(sdr->dev);
+
+ return 0;
+}
+
+static const struct of_device_id of_altera_sdr_match[] = {
+ { .compatible = "altr,sdr", },
+ { },
+};
+
+static const struct platform_device_id altera_sdr_ids[] = {
+ { "altera_sdr", },
+ { }
+};
+
+static struct platform_driver altera_sdr_driver = {
+ .driver = {
+ .name = "altera_sdr",
+ .owner = THIS_MODULE,
+ .of_match_table = of_altera_sdr_match,
+ },
+ .probe = altera_sdr_probe,
+ .remove = altera_sdr_remove,
+ .id_table = altera_sdr_ids,
+};
+
+static int __init altera_sdr_init(void)
+{
+ return platform_driver_register(&altera_sdr_driver);
+}
+postcore_initcall(altera_sdr_init);
+
+static void __exit altera_sdr_exit(void)
+{
+ platform_driver_unregister(&altera_sdr_driver);
+}
+module_exit(altera_sdr_exit);
+
+MODULE_AUTHOR("Alan Tull <atull-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
new file mode 100644
index 0000000..a5f5c39
--- /dev/null
+++ b/include/linux/mfd/altera-sdr.h
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __LINUX_MFD_ALTERA_SDR_H
+#define __LINUX_MFD_ALTERA_SDR_H
+
+/* SDRAM Controller register offsets */
+#define SDR_CTLCFG_OFST 0x00
+#define SDR_DRAMADDRW_OFST 0x2C
+#define SDR_DRAMIFWIDTH_OFST 0x30
+#define SDR_DRAMSTS_OFST 0x38
+#define SDR_DRAMINTR_OFST 0x3C
+#define SDR_SBECOUNT_OFST 0x40
+#define SDR_DBECOUNT_OFST 0x44
+#define SDR_ERRADDR_OFST 0x48
+#define SDR_DROPCOUNT_OFST 0x4C
+#define SDR_DROPADDR_OFST 0x50
+#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
+#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
+
+/* SDRAM Controller CtrlCfg Register Bit Masks */
+#define SDR_CTLCFG_ECC_EN 0x400
+#define SDR_CTLCFG_ECC_CORR_EN 0x800
+#define SDR_CTLCFG_GEN_SB_ERR 0x2000
+#define SDR_CTLCFG_GEN_DB_ERR 0x4000
+
+#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
+ SDR_CTLCFG_ECC_CORR_EN)
+
+/* SDRAM Controller Address Widths Field Register */
+#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
+#define SDR_DRAMADDRW_COLBITS_LSB 0
+#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
+#define SDR_DRAMADDRW_ROWBITS_LSB 5
+#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
+#define SDR_DRAMADDRW_BANKBITS_LSB 10
+#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
+#define SDR_DRAMADDRW_CSBITS_LSB 13
+
+/* SDRAM Controller Interface Data Width Defines */
+#define SDR_DRAMIFWIDTH_16B_ECC 24
+#define SDR_DRAMIFWIDTH_32B_ECC 40
+
+/* SDRAM Controller DRAM Status Register Bit Masks */
+#define SDR_DRAMSTS_SBEERR 0x04
+#define SDR_DRAMSTS_DBEERR 0x08
+#define SDR_DRAMSTS_CORR_DROP 0x10
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define SDR_DRAMINTR_INTREN 0x01
+#define SDR_DRAMINTR_SBEMASK 0x02
+#define SDR_DRAMINTR_DBEMASK 0x04
+#define SDR_DRAMINTR_CORRDROPMASK 0x08
+#define SDR_DRAMINTR_INTRCLR 0x10
+
+/* SDRAM Controller Single Bit Error Count Register Bit Masks */
+#define SDR_SBECOUNT_COUNT_MASK 0x0F
+
+/* SDRAM Controller Double Bit Error Count Register Bit Masks */
+#define SDR_DBECOUNT_COUNT_MASK 0x0F
+
+/* SDRAM Controller ECC Error Address Register Bit Masks */
+#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
+#define SDR_DROPCOUNT_CORRMASK 0x0F
+
+/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
+#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
+
+#define SELFRSHREQ_POS 3
+#define SELFRSHREQ_MASK 0x8
+
+#define SELFRFSHMASK_POS 4
+#define SELFRFSHMASK_MASK 0x30
+
+#define SELFRFSHACK_POS 1
+#define SELFRFSHACK_MASK 0x2
+
+struct altera_sdr {
+ struct device *dev;
+ void __iomem *reg_base;
+};
+
+/* Register access API */
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
+u32 altera_sdr_mem_size(struct altera_sdr *sdr);
+
+#endif /* __LINUX_MFD_ALTERA_SDR_H */
--
1.7.9.5
--
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
0 siblings, 0 replies; 34+ messages in thread
From: tthayer at opensource.altera.com @ 2014-07-30 18:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Thor Thayer <tthayer@opensource.altera.com>
Add a simple MFD for the Altera SDRAM Controller.
Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v1-8: The MFD implementation was not included in the original series.
v9: New MFD implementation.
---
MAINTAINERS | 5 ++
drivers/mfd/Kconfig | 7 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
5 files changed, 277 insertions(+)
create mode 100644 drivers/mfd/altera-sdr.c
create mode 100644 include/linux/mfd/altera-sdr.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 86efa7e..48a8923 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen@altera.com>
S: Maintained
F: drivers/clk/socfpga/
+ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
+M: Thor Thayer <tthayer@altera.com>
+S: Maintained
+F: drivers/mfd/altera-sdr.c
+
ARM/STI ARCHITECTURE
M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
M: Maxime Coquelin <maxime.coquelin@st.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6cc4b6a..8ce4961 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -719,6 +719,13 @@ config MFD_STMPE
Keypad: stmpe-keypad
Touchscreen: stmpe-ts
+config MFD_ALTERA_SDR
+ bool "Altera SDRAM Controller MFD"
+ depends on ARCH_SOCFPGA
+ select MFD_CORE
+ help
+ Support for Altera SDRAM Controller (SDR) MFD.
+
menu "STMicroelectronics STMPE Interface Drivers"
depends on MFD_STMPE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8afedba..24cc2b7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
obj-$(CONFIG_MFD_AS3722) += as3722.o
obj-$(CONFIG_MFD_STW481X) += stw481x.o
obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
+obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
new file mode 100644
index 0000000..b5c6646
--- /dev/null
+++ b/drivers/mfd/altera-sdr.c
@@ -0,0 +1,162 @@
+/*
+ * SDRAM Controller (SDR) MFD
+ *
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static const struct mfd_cell altera_sdr_devs[] = {
+#if defined(CONFIG_EDAC_ALTERA_MC)
+ {
+ .name = "altr_sdram_edac",
+ .of_compatible = "altr,sdram-edac",
+ },
+#endif
+};
+
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+ return readl(sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+ writel(value, sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);
+
+/* Get total memory size in bytes */
+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
+{
+ u32 size;
+ u32 read_reg, row, bank, col, cs, width;
+
+ read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
+ if (read_reg < 0)
+ return 0;
+
+ width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
+ if (width < 0)
+ return 0;
+
+ col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
+ SDR_DRAMADDRW_COLBITS_LSB;
+ row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
+ SDR_DRAMADDRW_ROWBITS_LSB;
+ bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
+ SDR_DRAMADDRW_BANKBITS_LSB;
+ cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
+ SDR_DRAMADDRW_CSBITS_LSB;
+
+ /* Correct for ECC as its not addressible */
+ if (width == SDR_DRAMIFWIDTH_32B_ECC)
+ width = 32;
+ if (width == SDR_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;
+}
+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
+
+static int altera_sdr_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct altera_sdr *sdr;
+ struct resource *res;
+ void __iomem *base;
+ int ret;
+
+ sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
+ if (!sdr)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENOENT;
+
+ base = devm_ioremap(dev, res->start, resource_size(res));
+ if (!base)
+ return -ENOMEM;
+
+ sdr->dev = &pdev->dev;
+ sdr->reg_base = base;
+
+ ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
+ ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
+ if (ret)
+ dev_err(sdr->dev, "error adding devices");
+
+ platform_set_drvdata(pdev, sdr);
+
+ dev_dbg(dev, "Altera SDR MFD registered\n");
+
+ return 0;
+}
+
+static int altera_sdr_remove(struct platform_device *pdev)
+{
+ struct altera_sdr *sdr = platform_get_drvdata(pdev);
+
+ mfd_remove_devices(sdr->dev);
+
+ return 0;
+}
+
+static const struct of_device_id of_altera_sdr_match[] = {
+ { .compatible = "altr,sdr", },
+ { },
+};
+
+static const struct platform_device_id altera_sdr_ids[] = {
+ { "altera_sdr", },
+ { }
+};
+
+static struct platform_driver altera_sdr_driver = {
+ .driver = {
+ .name = "altera_sdr",
+ .owner = THIS_MODULE,
+ .of_match_table = of_altera_sdr_match,
+ },
+ .probe = altera_sdr_probe,
+ .remove = altera_sdr_remove,
+ .id_table = altera_sdr_ids,
+};
+
+static int __init altera_sdr_init(void)
+{
+ return platform_driver_register(&altera_sdr_driver);
+}
+postcore_initcall(altera_sdr_init);
+
+static void __exit altera_sdr_exit(void)
+{
+ platform_driver_unregister(&altera_sdr_driver);
+}
+module_exit(altera_sdr_exit);
+
+MODULE_AUTHOR("Alan Tull <atull@altera.com>");
+MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
new file mode 100644
index 0000000..a5f5c39
--- /dev/null
+++ b/include/linux/mfd/altera-sdr.h
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __LINUX_MFD_ALTERA_SDR_H
+#define __LINUX_MFD_ALTERA_SDR_H
+
+/* SDRAM Controller register offsets */
+#define SDR_CTLCFG_OFST 0x00
+#define SDR_DRAMADDRW_OFST 0x2C
+#define SDR_DRAMIFWIDTH_OFST 0x30
+#define SDR_DRAMSTS_OFST 0x38
+#define SDR_DRAMINTR_OFST 0x3C
+#define SDR_SBECOUNT_OFST 0x40
+#define SDR_DBECOUNT_OFST 0x44
+#define SDR_ERRADDR_OFST 0x48
+#define SDR_DROPCOUNT_OFST 0x4C
+#define SDR_DROPADDR_OFST 0x50
+#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
+#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
+
+/* SDRAM Controller CtrlCfg Register Bit Masks */
+#define SDR_CTLCFG_ECC_EN 0x400
+#define SDR_CTLCFG_ECC_CORR_EN 0x800
+#define SDR_CTLCFG_GEN_SB_ERR 0x2000
+#define SDR_CTLCFG_GEN_DB_ERR 0x4000
+
+#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
+ SDR_CTLCFG_ECC_CORR_EN)
+
+/* SDRAM Controller Address Widths Field Register */
+#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
+#define SDR_DRAMADDRW_COLBITS_LSB 0
+#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
+#define SDR_DRAMADDRW_ROWBITS_LSB 5
+#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
+#define SDR_DRAMADDRW_BANKBITS_LSB 10
+#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
+#define SDR_DRAMADDRW_CSBITS_LSB 13
+
+/* SDRAM Controller Interface Data Width Defines */
+#define SDR_DRAMIFWIDTH_16B_ECC 24
+#define SDR_DRAMIFWIDTH_32B_ECC 40
+
+/* SDRAM Controller DRAM Status Register Bit Masks */
+#define SDR_DRAMSTS_SBEERR 0x04
+#define SDR_DRAMSTS_DBEERR 0x08
+#define SDR_DRAMSTS_CORR_DROP 0x10
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define SDR_DRAMINTR_INTREN 0x01
+#define SDR_DRAMINTR_SBEMASK 0x02
+#define SDR_DRAMINTR_DBEMASK 0x04
+#define SDR_DRAMINTR_CORRDROPMASK 0x08
+#define SDR_DRAMINTR_INTRCLR 0x10
+
+/* SDRAM Controller Single Bit Error Count Register Bit Masks */
+#define SDR_SBECOUNT_COUNT_MASK 0x0F
+
+/* SDRAM Controller Double Bit Error Count Register Bit Masks */
+#define SDR_DBECOUNT_COUNT_MASK 0x0F
+
+/* SDRAM Controller ECC Error Address Register Bit Masks */
+#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
+#define SDR_DROPCOUNT_CORRMASK 0x0F
+
+/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
+#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
+
+#define SELFRSHREQ_POS 3
+#define SELFRSHREQ_MASK 0x8
+
+#define SELFRFSHMASK_POS 4
+#define SELFRFSHMASK_MASK 0x30
+
+#define SELFRFSHACK_POS 1
+#define SELFRFSHACK_MASK 0x2
+
+struct altera_sdr {
+ struct device *dev;
+ void __iomem *reg_base;
+};
+
+/* Register access API */
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
+u32 altera_sdr_mem_size(struct altera_sdr *sdr);
+
+#endif /* __LINUX_MFD_ALTERA_SDR_H */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 2/3] edac: altera: Add Altera EDAC support.
@ 2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
0 siblings, 0 replies; 34+ messages in thread
From: tthayer @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux, tthayer
From: Thor Thayer <tthayer@opensource.altera.com>
This patch adds support for the CycloneV and ArriaV SDRAM controllers.
Correction and reporting of SBEs, Panic on DBEs.
Signed-off-by: Thor Thayer <tthayer@opensource.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.
v6: Changes to split out shared SDRAM controller reg (offset 0x00)
as a syscon device and allocate ECC specific SDRAM registers
to EDAC.
v7: No changes. Bump for consistency.
v8: Alphabetize headers.
v9: Move Altera EDAC driver to use SDRAM MFD device since controller
registers are shared between different drivers.
---
MAINTAINERS | 1 +
drivers/edac/Kconfig | 10 ++
drivers/edac/Makefile | 2 +
drivers/edac/altera_edac.c | 293 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 306 insertions(+)
create mode 100644 drivers/edac/altera_edac.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 48a8923..7fde28b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1344,6 +1344,7 @@ ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
M: Thor Thayer <tthayer@altera.com>
S: Maintained
F: drivers/mfd/altera-sdr.c
+F: drivers/edac/altera_edac.c
ARM/STI ARCHITECTURE
M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..429e244 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,14 @@ 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
+ select MFD_ALTERA_SDR
+ 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..70845c4 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..602ae62
--- /dev/null
+++ b/drivers/edac/altera_edac.c
@@ -0,0 +1,293 @@
+/*
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Adapted from the highbank_mc_edac driver.
+ */
+
+#include <linux/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include "edac_core.h"
+#include "edac_module.h"
+
+#define EDAC_MOD_STR "altera_edac"
+#define EDAC_VERSION "1"
+
+/* Altera SDRAM Memory Controller data */
+struct altr_sdram_mc_data {
+ struct altera_sdr *sdr;
+};
+
+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 */
+ err_addr = altera_sdr_readl(drvdata->sdr, SDR_ERRADDR_OFST);
+ status = altera_sdr_readl(drvdata->sdr, SDR_DRAMSTS_OFST);
+
+ if (status & SDR_DRAMSTS_DBEERR) {
+ err_count = altera_sdr_readl(drvdata->sdr, SDR_DBECOUNT_OFST);
+ panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
+ err_count, err_addr);
+ }
+ if (status & SDR_DRAMSTS_SBEERR) {
+ err_count = altera_sdr_readl(drvdata->sdr, SDR_SBECOUNT_OFST);
+ 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, "");
+ }
+
+ altera_sdr_writel(drvdata->sdr,
+ SDR_DRAMINTR_OFST,
+ SDR_DRAMINTR_INTRCLR | SDR_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;
+
+ mci->pdev->coherent_dma_mask = ~0;
+ ptemp = dma_alloc_coherent(mci->pdev, 16, &dma_handle, GFP_KERNEL);
+
+ if (!ptemp) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Inject: Buffer Allocation error\n");
+ return -ENOMEM;
+ }
+
+ read_reg = altera_sdr_readl(drvdata->sdr, SDR_CTLCFG_OFST);
+ read_reg &= ~(SDR_CTLCFG_GEN_SB_ERR | SDR_CTLCFG_GEN_DB_ERR);
+
+ /* Error are injected by writing a word while the SBE or DBE
+ * bit in the CTLCFG register is set. Reading the word will
+ * trigger the SBE or DBE error and the corresponding IRQ.
+ */
+ if (count == 3) {
+ edac_printk(KERN_ALERT, EDAC_MC,
+ "Inject Double bit error\n");
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST,
+ (read_reg | SDR_CTLCFG_GEN_DB_ERR));
+ } else {
+ edac_printk(KERN_ALERT, EDAC_MC,
+ "Inject Single bit error\n");
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST,
+ (read_reg | SDR_CTLCFG_GEN_SB_ERR));
+ }
+
+ ptemp[0] = 0x5A5A5A5A;
+ ptemp[1] = 0xA5A5A5A5;
+
+ /* Clear the error injection bits */
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST, 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 and printk are used to prevent the
+ * 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
+
+static int altr_sdram_probe(struct platform_device *pdev)
+{
+ struct altera_sdr *sdr = dev_get_drvdata(pdev->dev.parent);
+ struct edac_mc_layer layers[2];
+ struct mem_ctl_info *mci;
+ struct altr_sdram_mc_data *drvdata;
+ struct dimm_info *dimm;
+ u32 read_reg, mem_size;
+ int irq;
+ int res = 0;
+
+ /* Validate the SDRAM controller has ECC enabled */
+ read_reg = altera_sdr_readl(sdr, SDR_CTLCFG_OFST);
+ if ((read_reg & SDR_CTLCFG_ECC_AUTO_EN) != SDR_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 = altera_sdr_mem_size(sdr);
+ edac_printk(KERN_DEBUG, EDAC_MC, "Memory Size = 0x%08x\n", mem_size);
+ 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 */
+ altera_sdr_writel(sdr, SDR_DRAMINTR_OFST, SDR_DRAMINTR_INTRCLR);
+
+ 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->sdr = sdr;
+ 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;
+ }
+
+ altera_sdr_writel(drvdata->sdr,
+ SDR_DRAMINTR_OFST,
+ SDR_DRAMINTR_INTRCLR | SDR_DRAMINTR_INTREN);
+
+ 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 2/3] edac: altera: Add Altera EDAC support.
@ 2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
0 siblings, 0 replies; 34+ messages in thread
From: tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2-Re5JQEeQqe8AvxtiuMwx3w, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, rob-VoJi6FS/r0vR7s880joybQ,
linux-lFZ/pmaqli7XmaaqVzeoHQ, atull-EIB2kfCEclfQT0dZR+AlfA,
delicious.quinoa-Re5JQEeQqe8AvxtiuMwx3w,
dinguyen-EIB2kfCEclfQT0dZR+AlfA,
dougthompson-aS9lmoZGLiVWk0Htik3J/w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A, bp-Gina5bIWoIWzQB+pC5nmwQ,
sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-edac-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w,
tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
This patch adds support for the CycloneV and ArriaV SDRAM controllers.
Correction and reporting of SBEs, Panic on DBEs.
Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@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.
v6: Changes to split out shared SDRAM controller reg (offset 0x00)
as a syscon device and allocate ECC specific SDRAM registers
to EDAC.
v7: No changes. Bump for consistency.
v8: Alphabetize headers.
v9: Move Altera EDAC driver to use SDRAM MFD device since controller
registers are shared between different drivers.
---
MAINTAINERS | 1 +
drivers/edac/Kconfig | 10 ++
drivers/edac/Makefile | 2 +
drivers/edac/altera_edac.c | 293 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 306 insertions(+)
create mode 100644 drivers/edac/altera_edac.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 48a8923..7fde28b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1344,6 +1344,7 @@ ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
M: Thor Thayer <tthayer-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
S: Maintained
F: drivers/mfd/altera-sdr.c
+F: drivers/edac/altera_edac.c
ARM/STI ARCHITECTURE
M: Srinivas Kandagatla <srinivas.kandagatla-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..429e244 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,14 @@ 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
+ select MFD_ALTERA_SDR
+ 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..70845c4 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..602ae62
--- /dev/null
+++ b/drivers/edac/altera_edac.c
@@ -0,0 +1,293 @@
+/*
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Adapted from the highbank_mc_edac driver.
+ */
+
+#include <linux/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include "edac_core.h"
+#include "edac_module.h"
+
+#define EDAC_MOD_STR "altera_edac"
+#define EDAC_VERSION "1"
+
+/* Altera SDRAM Memory Controller data */
+struct altr_sdram_mc_data {
+ struct altera_sdr *sdr;
+};
+
+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 */
+ err_addr = altera_sdr_readl(drvdata->sdr, SDR_ERRADDR_OFST);
+ status = altera_sdr_readl(drvdata->sdr, SDR_DRAMSTS_OFST);
+
+ if (status & SDR_DRAMSTS_DBEERR) {
+ err_count = altera_sdr_readl(drvdata->sdr, SDR_DBECOUNT_OFST);
+ panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
+ err_count, err_addr);
+ }
+ if (status & SDR_DRAMSTS_SBEERR) {
+ err_count = altera_sdr_readl(drvdata->sdr, SDR_SBECOUNT_OFST);
+ 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, "");
+ }
+
+ altera_sdr_writel(drvdata->sdr,
+ SDR_DRAMINTR_OFST,
+ SDR_DRAMINTR_INTRCLR | SDR_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;
+
+ mci->pdev->coherent_dma_mask = ~0;
+ ptemp = dma_alloc_coherent(mci->pdev, 16, &dma_handle, GFP_KERNEL);
+
+ if (!ptemp) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Inject: Buffer Allocation error\n");
+ return -ENOMEM;
+ }
+
+ read_reg = altera_sdr_readl(drvdata->sdr, SDR_CTLCFG_OFST);
+ read_reg &= ~(SDR_CTLCFG_GEN_SB_ERR | SDR_CTLCFG_GEN_DB_ERR);
+
+ /* Error are injected by writing a word while the SBE or DBE
+ * bit in the CTLCFG register is set. Reading the word will
+ * trigger the SBE or DBE error and the corresponding IRQ.
+ */
+ if (count == 3) {
+ edac_printk(KERN_ALERT, EDAC_MC,
+ "Inject Double bit error\n");
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST,
+ (read_reg | SDR_CTLCFG_GEN_DB_ERR));
+ } else {
+ edac_printk(KERN_ALERT, EDAC_MC,
+ "Inject Single bit error\n");
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST,
+ (read_reg | SDR_CTLCFG_GEN_SB_ERR));
+ }
+
+ ptemp[0] = 0x5A5A5A5A;
+ ptemp[1] = 0xA5A5A5A5;
+
+ /* Clear the error injection bits */
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST, 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 and printk are used to prevent the
+ * 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
+
+static int altr_sdram_probe(struct platform_device *pdev)
+{
+ struct altera_sdr *sdr = dev_get_drvdata(pdev->dev.parent);
+ struct edac_mc_layer layers[2];
+ struct mem_ctl_info *mci;
+ struct altr_sdram_mc_data *drvdata;
+ struct dimm_info *dimm;
+ u32 read_reg, mem_size;
+ int irq;
+ int res = 0;
+
+ /* Validate the SDRAM controller has ECC enabled */
+ read_reg = altera_sdr_readl(sdr, SDR_CTLCFG_OFST);
+ if ((read_reg & SDR_CTLCFG_ECC_AUTO_EN) != SDR_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 = altera_sdr_mem_size(sdr);
+ edac_printk(KERN_DEBUG, EDAC_MC, "Memory Size = 0x%08x\n", mem_size);
+ 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 */
+ altera_sdr_writel(sdr, SDR_DRAMINTR_OFST, SDR_DRAMINTR_INTRCLR);
+
+ 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->sdr = sdr;
+ 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;
+ }
+
+ altera_sdr_writel(drvdata->sdr,
+ SDR_DRAMINTR_OFST,
+ SDR_DRAMINTR_INTRCLR | SDR_DRAMINTR_INTREN);
+
+ 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
--
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 2/3] edac: altera: Add Altera EDAC support.
@ 2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
0 siblings, 0 replies; 34+ messages in thread
From: tthayer at opensource.altera.com @ 2014-07-30 18:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Thor Thayer <tthayer@opensource.altera.com>
This patch adds support for the CycloneV and ArriaV SDRAM controllers.
Correction and reporting of SBEs, Panic on DBEs.
Signed-off-by: Thor Thayer <tthayer@opensource.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.
v6: Changes to split out shared SDRAM controller reg (offset 0x00)
as a syscon device and allocate ECC specific SDRAM registers
to EDAC.
v7: No changes. Bump for consistency.
v8: Alphabetize headers.
v9: Move Altera EDAC driver to use SDRAM MFD device since controller
registers are shared between different drivers.
---
MAINTAINERS | 1 +
drivers/edac/Kconfig | 10 ++
drivers/edac/Makefile | 2 +
drivers/edac/altera_edac.c | 293 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 306 insertions(+)
create mode 100644 drivers/edac/altera_edac.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 48a8923..7fde28b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1344,6 +1344,7 @@ ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
M: Thor Thayer <tthayer@altera.com>
S: Maintained
F: drivers/mfd/altera-sdr.c
+F: drivers/edac/altera_edac.c
ARM/STI ARCHITECTURE
M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..429e244 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,14 @@ 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
+ select MFD_ALTERA_SDR
+ 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..70845c4 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..602ae62
--- /dev/null
+++ b/drivers/edac/altera_edac.c
@@ -0,0 +1,293 @@
+/*
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Adapted from the highbank_mc_edac driver.
+ */
+
+#include <linux/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include "edac_core.h"
+#include "edac_module.h"
+
+#define EDAC_MOD_STR "altera_edac"
+#define EDAC_VERSION "1"
+
+/* Altera SDRAM Memory Controller data */
+struct altr_sdram_mc_data {
+ struct altera_sdr *sdr;
+};
+
+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 */
+ err_addr = altera_sdr_readl(drvdata->sdr, SDR_ERRADDR_OFST);
+ status = altera_sdr_readl(drvdata->sdr, SDR_DRAMSTS_OFST);
+
+ if (status & SDR_DRAMSTS_DBEERR) {
+ err_count = altera_sdr_readl(drvdata->sdr, SDR_DBECOUNT_OFST);
+ panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
+ err_count, err_addr);
+ }
+ if (status & SDR_DRAMSTS_SBEERR) {
+ err_count = altera_sdr_readl(drvdata->sdr, SDR_SBECOUNT_OFST);
+ 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, "");
+ }
+
+ altera_sdr_writel(drvdata->sdr,
+ SDR_DRAMINTR_OFST,
+ SDR_DRAMINTR_INTRCLR | SDR_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;
+
+ mci->pdev->coherent_dma_mask = ~0;
+ ptemp = dma_alloc_coherent(mci->pdev, 16, &dma_handle, GFP_KERNEL);
+
+ if (!ptemp) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Inject: Buffer Allocation error\n");
+ return -ENOMEM;
+ }
+
+ read_reg = altera_sdr_readl(drvdata->sdr, SDR_CTLCFG_OFST);
+ read_reg &= ~(SDR_CTLCFG_GEN_SB_ERR | SDR_CTLCFG_GEN_DB_ERR);
+
+ /* Error are injected by writing a word while the SBE or DBE
+ * bit in the CTLCFG register is set. Reading the word will
+ * trigger the SBE or DBE error and the corresponding IRQ.
+ */
+ if (count == 3) {
+ edac_printk(KERN_ALERT, EDAC_MC,
+ "Inject Double bit error\n");
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST,
+ (read_reg | SDR_CTLCFG_GEN_DB_ERR));
+ } else {
+ edac_printk(KERN_ALERT, EDAC_MC,
+ "Inject Single bit error\n");
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST,
+ (read_reg | SDR_CTLCFG_GEN_SB_ERR));
+ }
+
+ ptemp[0] = 0x5A5A5A5A;
+ ptemp[1] = 0xA5A5A5A5;
+
+ /* Clear the error injection bits */
+ altera_sdr_writel(drvdata->sdr, SDR_CTLCFG_OFST, 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 and printk are used to prevent the
+ * 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
+
+static int altr_sdram_probe(struct platform_device *pdev)
+{
+ struct altera_sdr *sdr = dev_get_drvdata(pdev->dev.parent);
+ struct edac_mc_layer layers[2];
+ struct mem_ctl_info *mci;
+ struct altr_sdram_mc_data *drvdata;
+ struct dimm_info *dimm;
+ u32 read_reg, mem_size;
+ int irq;
+ int res = 0;
+
+ /* Validate the SDRAM controller has ECC enabled */
+ read_reg = altera_sdr_readl(sdr, SDR_CTLCFG_OFST);
+ if ((read_reg & SDR_CTLCFG_ECC_AUTO_EN) != SDR_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 = altera_sdr_mem_size(sdr);
+ edac_printk(KERN_DEBUG, EDAC_MC, "Memory Size = 0x%08x\n", mem_size);
+ 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 */
+ altera_sdr_writel(sdr, SDR_DRAMINTR_OFST, SDR_DRAMINTR_INTRCLR);
+
+ 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->sdr = sdr;
+ 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;
+ }
+
+ altera_sdr_writel(drvdata->sdr,
+ SDR_DRAMINTR_OFST,
+ SDR_DRAMINTR_INTRCLR | SDR_DRAMINTR_INTREN);
+
+ 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
2014-07-30 18:22 ` tthayer
(?)
@ 2014-07-30 18:22 ` tthayer
-1 siblings, 0 replies; 34+ messages in thread
From: tthayer @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux, tthayer
From: Thor Thayer <tthayer@opensource.altera.com>
Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
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.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
---
.../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
2 files changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
new file mode 100644
index 0000000..2bb1ddf
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
@@ -0,0 +1,13 @@
+Altera SOCFPGA SDRAM Controller
+The SDRAM controller is implemented as a MFD so various drivers may
+nest under this main SDRAM controller binding.
+
+Required properties:
+- compatible : "altr,sdr";
+- reg : Should contain 1 register range(address and length)
+
+Example:
+ sdr@0xffc25000 {
+ compatible = "altr,sdr";
+ reg = <0xffc25000 0x1000>;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..ecb306d 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,16 @@
};
};
+ sdr@0xffc25000 {
+ compatible = "altr,sdr";
+ reg = <0xffc25000 0x1000>;
+
+ sdramedac@0 {
+ compatible = "altr,sdram-edac";
+ interrupts = <0 39 4>;
+ };
+ };
+
L2: l2-cache@fffef000 {
compatible = "arm,pl310-cache";
reg = <0xfffef000 0x1000>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
@ 2014-07-30 18:22 ` tthayer
0 siblings, 0 replies; 34+ messages in thread
From: tthayer @ 2014-07-30 18:22 UTC (permalink / raw)
To: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux, tthayer
From: Thor Thayer <tthayer@opensource.altera.com>
Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
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.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
---
.../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
2 files changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
new file mode 100644
index 0000000..2bb1ddf
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
@@ -0,0 +1,13 @@
+Altera SOCFPGA SDRAM Controller
+The SDRAM controller is implemented as a MFD so various drivers may
+nest under this main SDRAM controller binding.
+
+Required properties:
+- compatible : "altr,sdr";
+- reg : Should contain 1 register range(address and length)
+
+Example:
+ sdr@0xffc25000 {
+ compatible = "altr,sdr";
+ reg = <0xffc25000 0x1000>;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..ecb306d 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,16 @@
};
};
+ sdr@0xffc25000 {
+ compatible = "altr,sdr";
+ reg = <0xffc25000 0x1000>;
+
+ sdramedac@0 {
+ compatible = "altr,sdram-edac";
+ interrupts = <0 39 4>;
+ };
+ };
+
L2: l2-cache@fffef000 {
compatible = "arm,pl310-cache";
reg = <0xfffef000 0x1000>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
@ 2014-07-30 18:22 ` tthayer
0 siblings, 0 replies; 34+ messages in thread
From: tthayer at opensource.altera.com @ 2014-07-30 18:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Thor Thayer <tthayer@opensource.altera.com>
Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
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.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
---
.../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
2 files changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
new file mode 100644
index 0000000..2bb1ddf
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
@@ -0,0 +1,13 @@
+Altera SOCFPGA SDRAM Controller
+The SDRAM controller is implemented as a MFD so various drivers may
+nest under this main SDRAM controller binding.
+
+Required properties:
+- compatible : "altr,sdr";
+- reg : Should contain 1 register range(address and length)
+
+Example:
+ sdr at 0xffc25000 {
+ compatible = "altr,sdr";
+ reg = <0xffc25000 0x1000>;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..ecb306d 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,16 @@
};
};
+ sdr at 0xffc25000 {
+ compatible = "altr,sdr";
+ reg = <0xffc25000 0x1000>;
+
+ sdramedac at 0 {
+ compatible = "altr,sdram-edac";
+ interrupts = <0 39 4>;
+ };
+ };
+
L2: l2-cache at fffef000 {
compatible = "arm,pl310-cache";
reg = <0xfffef000 0x1000>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
@ 2014-07-31 8:26 ` Lee Jones
-1 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2014-07-31 8:26 UTC (permalink / raw)
To: tthayer
Cc: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, devicetree, linux-doc, linux-edac,
linux-kernel, linux-arm-kernel, tthayer.linux, Alan Tull
On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Add a simple MFD for the Altera SDRAM Controller.
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v1-8: The MFD implementation was not included in the original series.
>
> v9: New MFD implementation.
> ---
> MAINTAINERS | 5 ++
> drivers/mfd/Kconfig | 7 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> 5 files changed, 277 insertions(+)
> create mode 100644 drivers/mfd/altera-sdr.c
> create mode 100644 include/linux/mfd/altera-sdr.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 86efa7e..48a8923 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen@altera.com>
> S: Maintained
> F: drivers/clk/socfpga/
>
> +ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
> +M: Thor Thayer <tthayer@altera.com>
> +S: Maintained
> +F: drivers/mfd/altera-sdr.c
> +
> ARM/STI ARCHITECTURE
> M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
> M: Maxime Coquelin <maxime.coquelin@st.com>
This should be in a separate patch.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6cc4b6a..8ce4961 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -719,6 +719,13 @@ config MFD_STMPE
> Keypad: stmpe-keypad
> Touchscreen: stmpe-ts
>
> +config MFD_ALTERA_SDR
> + bool "Altera SDRAM Controller MFD"
> + depends on ARCH_SOCFPGA
> + select MFD_CORE
> + help
> + Support for Altera SDRAM Controller (SDR) MFD.
> +
> menu "STMicroelectronics STMPE Interface Drivers"
> depends on MFD_STMPE
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8afedba..24cc2b7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
> obj-$(CONFIG_MFD_AS3722) += as3722.o
> obj-$(CONFIG_MFD_STW481X) += stw481x.o
> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
> +obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
> diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
> new file mode 100644
> index 0000000..b5c6646
> --- /dev/null
> +++ b/drivers/mfd/altera-sdr.c
> @@ -0,0 +1,162 @@
> +/*
> + * SDRAM Controller (SDR) MFD
> + *
> + * Copyright (C) 2014 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
Can you use the shorter version of the licence?
> + */
'\n' here.
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/altera-sdr.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static const struct mfd_cell altera_sdr_devs[] = {
> +#if defined(CONFIG_EDAC_ALTERA_MC)
No need to do this, as it will only be matched if the driver is
enabled. Please remove the #iffery.
> + {
> + .name = "altr_sdram_edac",
> + .of_compatible = "altr,sdram-edac",
What other devices will there be?
> + },
> +#endif
> +};
> +
> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
> +{
> + return readl(sdr->reg_base + reg_offset);
> +}
> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
> +
> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> +{
> + writel(value, sdr->reg_base + reg_offset);
> +}
> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
Why are you abstracting these?
Might be better to use Regmap even.
> +/* Get total memory size in bytes */
> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
> +{
> + u32 size;
> + u32 read_reg, row, bank, col, cs, width;
Weird that size is on its own. Either place on a single line or
separate them all.
> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
> + if (read_reg < 0)
> + return 0;
> +
> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
> + if (width < 0)
> + return 0;
> +
> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
> + SDR_DRAMADDRW_COLBITS_LSB;
> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
> + SDR_DRAMADDRW_ROWBITS_LSB;
> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
> + SDR_DRAMADDRW_BANKBITS_LSB;
> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
> + SDR_DRAMADDRW_CSBITS_LSB;
These would probably be better as macros.
> + /* Correct for ECC as its not addressible */
> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
> + width = 32;
> + if (width == SDR_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;
> +}
> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
Should this really be done in here? Isn't this an SDRAM function?
> +static int altera_sdr_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct altera_sdr *sdr;
> + struct resource *res;
> + void __iomem *base;
> + int ret;
> +
> + sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
> + if (!sdr)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOENT;
> +
> + base = devm_ioremap(dev, res->start, resource_size(res));
Instead use devm_ioremap_resource(), then you can omit the error
checking of platform_get_resource().
> + if (!base)
> + return -ENOMEM;
> +
> + sdr->dev = &pdev->dev;
Either use 'dev' here, or remove the top line in this function and use
&pdev->dev everywhere. I personally prefer the latter.
> + sdr->reg_base = base;
> +
> + ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
> + ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
> + if (ret)
> + dev_err(sdr->dev, "error adding devices");
> +
> + platform_set_drvdata(pdev, sdr);
> +
> + dev_dbg(dev, "Altera SDR MFD registered\n");
> +
> + return 0;
> +}
> +
> +static int altera_sdr_remove(struct platform_device *pdev)
> +{
> + struct altera_sdr *sdr = platform_get_drvdata(pdev);
No need for this, just use &pdev->dev.
> + mfd_remove_devices(sdr->dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_altera_sdr_match[] = {
> + { .compatible = "altr,sdr", },
> + { },
> +};
> +
> +static const struct platform_device_id altera_sdr_ids[] = {
> + { "altera_sdr", },
> + { }
> +};
What's this for?
> +static struct platform_driver altera_sdr_driver = {
> + .driver = {
> + .name = "altera_sdr",
> + .owner = THIS_MODULE,
You can remove this line, it's taken care of for you.
> + .of_match_table = of_altera_sdr_match,
> + },
> + .probe = altera_sdr_probe,
> + .remove = altera_sdr_remove,
> + .id_table = altera_sdr_ids,
> +};
> +
> +static int __init altera_sdr_init(void)
> +{
> + return platform_driver_register(&altera_sdr_driver);
> +}
> +postcore_initcall(altera_sdr_init);
Why was this chosen?
> +static void __exit altera_sdr_exit(void)
> +{
> + platform_driver_unregister(&altera_sdr_driver);
> +}
> +module_exit(altera_sdr_exit);
> +
> +MODULE_AUTHOR("Alan Tull <atull@altera.com>");
> +MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
> new file mode 100644
> index 0000000..a5f5c39
> --- /dev/null
> +++ b/include/linux/mfd/altera-sdr.h
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright (C) 2014 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
Use the short version.
> + */
'\n' here.
> +#ifndef __LINUX_MFD_ALTERA_SDR_H
> +#define __LINUX_MFD_ALTERA_SDR_H
> +
> +/* SDRAM Controller register offsets */
> +#define SDR_CTLCFG_OFST 0x00
> +#define SDR_DRAMADDRW_OFST 0x2C
> +#define SDR_DRAMIFWIDTH_OFST 0x30
> +#define SDR_DRAMSTS_OFST 0x38
> +#define SDR_DRAMINTR_OFST 0x3C
> +#define SDR_SBECOUNT_OFST 0x40
> +#define SDR_DBECOUNT_OFST 0x44
> +#define SDR_ERRADDR_OFST 0x48
> +#define SDR_DROPCOUNT_OFST 0x4C
> +#define SDR_DROPADDR_OFST 0x50
> +#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
> +#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
> +
> +/* SDRAM Controller CtrlCfg Register Bit Masks */
> +#define SDR_CTLCFG_ECC_EN 0x400
> +#define SDR_CTLCFG_ECC_CORR_EN 0x800
> +#define SDR_CTLCFG_GEN_SB_ERR 0x2000
> +#define SDR_CTLCFG_GEN_DB_ERR 0x4000
> +
> +#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
> + SDR_CTLCFG_ECC_CORR_EN)
> +
> +/* SDRAM Controller Address Widths Field Register */
> +#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
> +#define SDR_DRAMADDRW_COLBITS_LSB 0
> +#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
> +#define SDR_DRAMADDRW_ROWBITS_LSB 5
> +#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
> +#define SDR_DRAMADDRW_BANKBITS_LSB 10
> +#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
> +#define SDR_DRAMADDRW_CSBITS_LSB 13
We normally call _LSB, _SHIFT.
> +/* SDRAM Controller Interface Data Width Defines */
> +#define SDR_DRAMIFWIDTH_16B_ECC 24
> +#define SDR_DRAMIFWIDTH_32B_ECC 40
> +
> +/* SDRAM Controller DRAM Status Register Bit Masks */
> +#define SDR_DRAMSTS_SBEERR 0x04
> +#define SDR_DRAMSTS_DBEERR 0x08
> +#define SDR_DRAMSTS_CORR_DROP 0x10
> +
> +/* SDRAM Controller DRAM IRQ Register Bit Masks */
> +#define SDR_DRAMINTR_INTREN 0x01
> +#define SDR_DRAMINTR_SBEMASK 0x02
> +#define SDR_DRAMINTR_DBEMASK 0x04
> +#define SDR_DRAMINTR_CORRDROPMASK 0x08
> +#define SDR_DRAMINTR_INTRCLR 0x10
> +
> +/* SDRAM Controller Single Bit Error Count Register Bit Masks */
> +#define SDR_SBECOUNT_COUNT_MASK 0x0F
> +
> +/* SDRAM Controller Double Bit Error Count Register Bit Masks */
> +#define SDR_DBECOUNT_COUNT_MASK 0x0F
> +
> +/* SDRAM Controller ECC Error Address Register Bit Masks */
> +#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
> +
> +/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
> +#define SDR_DROPCOUNT_CORRMASK 0x0F
> +
> +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
> +#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
> +
> +#define SELFRSHREQ_POS 3
> +#define SELFRSHREQ_MASK 0x8
> +
> +#define SELFRFSHMASK_POS 4
> +#define SELFRFSHMASK_MASK 0x30
> +
> +#define SELFRFSHACK_POS 1
> +#define SELFRFSHACK_MASK 0x2
> +
> +struct altera_sdr {
> + struct device *dev;
> + void __iomem *reg_base;
> +};
> +
> +/* Register access API */
> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
Regmap?
> +u32 altera_sdr_mem_size(struct altera_sdr *sdr);
> +
> +#endif /* __LINUX_MFD_ALTERA_SDR_H */
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-07-31 8:26 ` Lee Jones
0 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2014-07-31 8:26 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Add a simple MFD for the Altera SDRAM Controller.
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v1-8: The MFD implementation was not included in the original series.
>
> v9: New MFD implementation.
> ---
> MAINTAINERS | 5 ++
> drivers/mfd/Kconfig | 7 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> 5 files changed, 277 insertions(+)
> create mode 100644 drivers/mfd/altera-sdr.c
> create mode 100644 include/linux/mfd/altera-sdr.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 86efa7e..48a8923 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen@altera.com>
> S: Maintained
> F: drivers/clk/socfpga/
>
> +ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
> +M: Thor Thayer <tthayer@altera.com>
> +S: Maintained
> +F: drivers/mfd/altera-sdr.c
> +
> ARM/STI ARCHITECTURE
> M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
> M: Maxime Coquelin <maxime.coquelin@st.com>
This should be in a separate patch.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6cc4b6a..8ce4961 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -719,6 +719,13 @@ config MFD_STMPE
> Keypad: stmpe-keypad
> Touchscreen: stmpe-ts
>
> +config MFD_ALTERA_SDR
> + bool "Altera SDRAM Controller MFD"
> + depends on ARCH_SOCFPGA
> + select MFD_CORE
> + help
> + Support for Altera SDRAM Controller (SDR) MFD.
> +
> menu "STMicroelectronics STMPE Interface Drivers"
> depends on MFD_STMPE
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8afedba..24cc2b7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
> obj-$(CONFIG_MFD_AS3722) += as3722.o
> obj-$(CONFIG_MFD_STW481X) += stw481x.o
> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
> +obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
> diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
> new file mode 100644
> index 0000000..b5c6646
> --- /dev/null
> +++ b/drivers/mfd/altera-sdr.c
> @@ -0,0 +1,162 @@
> +/*
> + * SDRAM Controller (SDR) MFD
> + *
> + * Copyright (C) 2014 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
Can you use the shorter version of the licence?
> + */
'\n' here.
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/altera-sdr.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static const struct mfd_cell altera_sdr_devs[] = {
> +#if defined(CONFIG_EDAC_ALTERA_MC)
No need to do this, as it will only be matched if the driver is
enabled. Please remove the #iffery.
> + {
> + .name = "altr_sdram_edac",
> + .of_compatible = "altr,sdram-edac",
What other devices will there be?
> + },
> +#endif
> +};
> +
> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
> +{
> + return readl(sdr->reg_base + reg_offset);
> +}
> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
> +
> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> +{
> + writel(value, sdr->reg_base + reg_offset);
> +}
> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
Why are you abstracting these?
Might be better to use Regmap even.
> +/* Get total memory size in bytes */
> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
> +{
> + u32 size;
> + u32 read_reg, row, bank, col, cs, width;
Weird that size is on its own. Either place on a single line or
separate them all.
> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
> + if (read_reg < 0)
> + return 0;
> +
> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
> + if (width < 0)
> + return 0;
> +
> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
> + SDR_DRAMADDRW_COLBITS_LSB;
> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
> + SDR_DRAMADDRW_ROWBITS_LSB;
> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
> + SDR_DRAMADDRW_BANKBITS_LSB;
> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
> + SDR_DRAMADDRW_CSBITS_LSB;
These would probably be better as macros.
> + /* Correct for ECC as its not addressible */
> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
> + width = 32;
> + if (width == SDR_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;
> +}
> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
Should this really be done in here? Isn't this an SDRAM function?
> +static int altera_sdr_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct altera_sdr *sdr;
> + struct resource *res;
> + void __iomem *base;
> + int ret;
> +
> + sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
> + if (!sdr)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOENT;
> +
> + base = devm_ioremap(dev, res->start, resource_size(res));
Instead use devm_ioremap_resource(), then you can omit the error
checking of platform_get_resource().
> + if (!base)
> + return -ENOMEM;
> +
> + sdr->dev = &pdev->dev;
Either use 'dev' here, or remove the top line in this function and use
&pdev->dev everywhere. I personally prefer the latter.
> + sdr->reg_base = base;
> +
> + ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
> + ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
> + if (ret)
> + dev_err(sdr->dev, "error adding devices");
> +
> + platform_set_drvdata(pdev, sdr);
> +
> + dev_dbg(dev, "Altera SDR MFD registered\n");
> +
> + return 0;
> +}
> +
> +static int altera_sdr_remove(struct platform_device *pdev)
> +{
> + struct altera_sdr *sdr = platform_get_drvdata(pdev);
No need for this, just use &pdev->dev.
> + mfd_remove_devices(sdr->dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_altera_sdr_match[] = {
> + { .compatible = "altr,sdr", },
> + { },
> +};
> +
> +static const struct platform_device_id altera_sdr_ids[] = {
> + { "altera_sdr", },
> + { }
> +};
What's this for?
> +static struct platform_driver altera_sdr_driver = {
> + .driver = {
> + .name = "altera_sdr",
> + .owner = THIS_MODULE,
You can remove this line, it's taken care of for you.
> + .of_match_table = of_altera_sdr_match,
> + },
> + .probe = altera_sdr_probe,
> + .remove = altera_sdr_remove,
> + .id_table = altera_sdr_ids,
> +};
> +
> +static int __init altera_sdr_init(void)
> +{
> + return platform_driver_register(&altera_sdr_driver);
> +}
> +postcore_initcall(altera_sdr_init);
Why was this chosen?
> +static void __exit altera_sdr_exit(void)
> +{
> + platform_driver_unregister(&altera_sdr_driver);
> +}
> +module_exit(altera_sdr_exit);
> +
> +MODULE_AUTHOR("Alan Tull <atull@altera.com>");
> +MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
> new file mode 100644
> index 0000000..a5f5c39
> --- /dev/null
> +++ b/include/linux/mfd/altera-sdr.h
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright (C) 2014 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
Use the short version.
> + */
'\n' here.
> +#ifndef __LINUX_MFD_ALTERA_SDR_H
> +#define __LINUX_MFD_ALTERA_SDR_H
> +
> +/* SDRAM Controller register offsets */
> +#define SDR_CTLCFG_OFST 0x00
> +#define SDR_DRAMADDRW_OFST 0x2C
> +#define SDR_DRAMIFWIDTH_OFST 0x30
> +#define SDR_DRAMSTS_OFST 0x38
> +#define SDR_DRAMINTR_OFST 0x3C
> +#define SDR_SBECOUNT_OFST 0x40
> +#define SDR_DBECOUNT_OFST 0x44
> +#define SDR_ERRADDR_OFST 0x48
> +#define SDR_DROPCOUNT_OFST 0x4C
> +#define SDR_DROPADDR_OFST 0x50
> +#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
> +#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
> +
> +/* SDRAM Controller CtrlCfg Register Bit Masks */
> +#define SDR_CTLCFG_ECC_EN 0x400
> +#define SDR_CTLCFG_ECC_CORR_EN 0x800
> +#define SDR_CTLCFG_GEN_SB_ERR 0x2000
> +#define SDR_CTLCFG_GEN_DB_ERR 0x4000
> +
> +#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
> + SDR_CTLCFG_ECC_CORR_EN)
> +
> +/* SDRAM Controller Address Widths Field Register */
> +#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
> +#define SDR_DRAMADDRW_COLBITS_LSB 0
> +#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
> +#define SDR_DRAMADDRW_ROWBITS_LSB 5
> +#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
> +#define SDR_DRAMADDRW_BANKBITS_LSB 10
> +#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
> +#define SDR_DRAMADDRW_CSBITS_LSB 13
We normally call _LSB, _SHIFT.
> +/* SDRAM Controller Interface Data Width Defines */
> +#define SDR_DRAMIFWIDTH_16B_ECC 24
> +#define SDR_DRAMIFWIDTH_32B_ECC 40
> +
> +/* SDRAM Controller DRAM Status Register Bit Masks */
> +#define SDR_DRAMSTS_SBEERR 0x04
> +#define SDR_DRAMSTS_DBEERR 0x08
> +#define SDR_DRAMSTS_CORR_DROP 0x10
> +
> +/* SDRAM Controller DRAM IRQ Register Bit Masks */
> +#define SDR_DRAMINTR_INTREN 0x01
> +#define SDR_DRAMINTR_SBEMASK 0x02
> +#define SDR_DRAMINTR_DBEMASK 0x04
> +#define SDR_DRAMINTR_CORRDROPMASK 0x08
> +#define SDR_DRAMINTR_INTRCLR 0x10
> +
> +/* SDRAM Controller Single Bit Error Count Register Bit Masks */
> +#define SDR_SBECOUNT_COUNT_MASK 0x0F
> +
> +/* SDRAM Controller Double Bit Error Count Register Bit Masks */
> +#define SDR_DBECOUNT_COUNT_MASK 0x0F
> +
> +/* SDRAM Controller ECC Error Address Register Bit Masks */
> +#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
> +
> +/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
> +#define SDR_DROPCOUNT_CORRMASK 0x0F
> +
> +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
> +#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
> +
> +#define SELFRSHREQ_POS 3
> +#define SELFRSHREQ_MASK 0x8
> +
> +#define SELFRFSHMASK_POS 4
> +#define SELFRFSHMASK_MASK 0x30
> +
> +#define SELFRFSHACK_POS 1
> +#define SELFRFSHACK_MASK 0x2
> +
> +struct altera_sdr {
> + struct device *dev;
> + void __iomem *reg_base;
> +};
> +
> +/* Register access API */
> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
Regmap?
> +u32 altera_sdr_mem_size(struct altera_sdr *sdr);
> +
> +#endif /* __LINUX_MFD_ALTERA_SDR_H */
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-07-31 8:26 ` Lee Jones
(?)
@ 2014-07-31 20:00 ` Thor Thayer
-1 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-07-31 20:00 UTC (permalink / raw)
To: Lee Jones
Cc: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, devicetree, linux-doc, linux-edac,
linux-kernel, linux-arm-kernel, tthayer.linux, Alan Tull
On 07/31/2014 03:26 AM, Lee Jones wrote:
> On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
>
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add a simple MFD for the Altera SDRAM Controller.
>>
>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v1-8: The MFD implementation was not included in the original series.
>>
>> v9: New MFD implementation.
>> ---
>> MAINTAINERS | 5 ++
>> drivers/mfd/Kconfig | 7 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
>> 5 files changed, 277 insertions(+)
>> create mode 100644 drivers/mfd/altera-sdr.c
>> create mode 100644 include/linux/mfd/altera-sdr.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 86efa7e..48a8923 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen@altera.com>
>> S: Maintained
>> F: drivers/clk/socfpga/
>>
>> +ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
>> +M: Thor Thayer <tthayer@altera.com>
>> +S: Maintained
>> +F: drivers/mfd/altera-sdr.c
>> +
>> ARM/STI ARCHITECTURE
>> M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
>> M: Maxime Coquelin <maxime.coquelin@st.com>
> This should be in a separate patch.
OK. Thanks for your comments and for reviewing. I will move this into a
separate patch.
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 6cc4b6a..8ce4961 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -719,6 +719,13 @@ config MFD_STMPE
>> Keypad: stmpe-keypad
>> Touchscreen: stmpe-ts
>>
>> +config MFD_ALTERA_SDR
>> + bool "Altera SDRAM Controller MFD"
>> + depends on ARCH_SOCFPGA
>> + select MFD_CORE
>> + help
>> + Support for Altera SDRAM Controller (SDR) MFD.
>> +
>> menu "STMicroelectronics STMPE Interface Drivers"
>> depends on MFD_STMPE
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 8afedba..24cc2b7 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
>> obj-$(CONFIG_MFD_AS3722) += as3722.o
>> obj-$(CONFIG_MFD_STW481X) += stw481x.o
>> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
>> +obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
>> diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
>> new file mode 100644
>> index 0000000..b5c6646
>> --- /dev/null
>> +++ b/drivers/mfd/altera-sdr.c
>> @@ -0,0 +1,162 @@
>> +/*
>> + * SDRAM Controller (SDR) MFD
>> + *
>> + * Copyright (C) 2014 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
> Can you use the shorter version of the licence?
Hi. This seems to be the shorter version of the license agreement and is
fairly common in the kernel, right?
>> + */
> '\n' here.
OK.
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/altera-sdr.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +static const struct mfd_cell altera_sdr_devs[] = {
>> +#if defined(CONFIG_EDAC_ALTERA_MC)
> No need to do this, as it will only be matched if the driver is
> enabled. Please remove the #iffery.
OK. Will remove.
>> + {
>> + .name = "altr_sdram_edac",
>> + .of_compatible = "altr,sdram-edac",
> What other devices will there be?
>
There will be an FPGA bridge and a power control driver that will need
access to the SDR Controller registers.
>> + },
>> +#endif
>> +};
>> +
>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>> +{
>> + return readl(sdr->reg_base + reg_offset);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>> +
>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>> +{
>> + writel(value, sdr->reg_base + reg_offset);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
> Why are you abstracting these?
>
> Might be better to use Regmap even.
regmap seems unnecessarily complex for what we're doing which is why
this method was chosen.
Future drivers will access different sets of registers in the device.
These drivers won't share bitfields in the same register so the MFD
seemed like the best solution. Originally we implemented this using
syscon but that seems to be frowned upon so we changed to using a MFD.
>> +/* Get total memory size in bytes */
>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
>> +{
>> + u32 size;
>> + u32 read_reg, row, bank, col, cs, width;
> Weird that size is on its own. Either place on a single line or
> separate them all.
OK. I will change this.
>> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
>> + if (read_reg < 0)
>> + return 0;
>> +
>> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
>> + if (width < 0)
>> + return 0;
>> +
>> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
>> + SDR_DRAMADDRW_COLBITS_LSB;
>> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
>> + SDR_DRAMADDRW_ROWBITS_LSB;
>> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
>> + SDR_DRAMADDRW_BANKBITS_LSB;
>> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
>> + SDR_DRAMADDRW_CSBITS_LSB;
> These would probably be better as macros.
>
OK. I will fix this.
>> + /* Correct for ECC as its not addressible */
>> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
>> + width = 32;
>> + if (width == SDR_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;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> Should this really be done in here? Isn't this an SDRAM function?
>
This register is part of the SDRAM controller and size information may
be required by the other drivers that share this memory area/need SDRAM
information.
>> +static int altera_sdr_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct altera_sdr *sdr;
>> + struct resource *res;
>> + void __iomem *base;
>> + int ret;
>> +
>> + sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
>> + if (!sdr)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENOENT;
>> +
>> + base = devm_ioremap(dev, res->start, resource_size(res));
> Instead use devm_ioremap_resource(), then you can omit the error
> checking of platform_get_resource().
OK. Thanks. I will make the change.
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + sdr->dev = &pdev->dev;
> Either use 'dev' here, or remove the top line in this function and use
> &pdev->dev everywhere. I personally prefer the latter.
OK. I will use the latter.
>> + sdr->reg_base = base;
>> +
>> + ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
>> + ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
>> + if (ret)
>> + dev_err(sdr->dev, "error adding devices");
>> +
>> + platform_set_drvdata(pdev, sdr);
>> +
>> + dev_dbg(dev, "Altera SDR MFD registered\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_sdr_remove(struct platform_device *pdev)
>> +{
>> + struct altera_sdr *sdr = platform_get_drvdata(pdev);
> No need for this, just use &pdev->dev.
OK. I will make the change.
>> + mfd_remove_devices(sdr->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id of_altera_sdr_match[] = {
>> + { .compatible = "altr,sdr", },
>> + { },
>> +};
>> +
>> +static const struct platform_device_id altera_sdr_ids[] = {
>> + { "altera_sdr", },
>> + { }
>> +};
> What's this for?
We don't strictly need it because we are driven by the device tree. It
can be removed it if is a problem but I'm not clear why it is a problem.
>> +static struct platform_driver altera_sdr_driver = {
>> + .driver = {
>> + .name = "altera_sdr",
>> + .owner = THIS_MODULE,
> You can remove this line, it's taken care of for you.
I will remove the .owner line.
>> + .of_match_table = of_altera_sdr_match,
>> + },
>> + .probe = altera_sdr_probe,
>> + .remove = altera_sdr_remove,
>> + .id_table = altera_sdr_ids,
>> +};
>> +
>> +static int __init altera_sdr_init(void)
>> +{
>> + return platform_driver_register(&altera_sdr_driver);
>> +}
>> +postcore_initcall(altera_sdr_init);
> Why was this chosen?
We want this to happen pretty early.
>> +static void __exit altera_sdr_exit(void)
>> +{
>> + platform_driver_unregister(&altera_sdr_driver);
>> +}
>> +module_exit(altera_sdr_exit);
>> +
>> +MODULE_AUTHOR("Alan Tull <atull@altera.com>");
>> +MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
>> new file mode 100644
>> index 0000000..a5f5c39
>> --- /dev/null
>> +++ b/include/linux/mfd/altera-sdr.h
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Copyright (C) 2014 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
> Use the short version.
Same as above. This is the shorter version that we've been using at Altera.
>> + */
> '\n' here.
OK.
>> +#ifndef __LINUX_MFD_ALTERA_SDR_H
>> +#define __LINUX_MFD_ALTERA_SDR_H
>> +
>> +/* SDRAM Controller register offsets */
>> +#define SDR_CTLCFG_OFST 0x00
>> +#define SDR_DRAMADDRW_OFST 0x2C
>> +#define SDR_DRAMIFWIDTH_OFST 0x30
>> +#define SDR_DRAMSTS_OFST 0x38
>> +#define SDR_DRAMINTR_OFST 0x3C
>> +#define SDR_SBECOUNT_OFST 0x40
>> +#define SDR_DBECOUNT_OFST 0x44
>> +#define SDR_ERRADDR_OFST 0x48
>> +#define SDR_DROPCOUNT_OFST 0x4C
>> +#define SDR_DROPADDR_OFST 0x50
>> +#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
>> +#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
>> +
>> +/* SDRAM Controller CtrlCfg Register Bit Masks */
>> +#define SDR_CTLCFG_ECC_EN 0x400
>> +#define SDR_CTLCFG_ECC_CORR_EN 0x800
>> +#define SDR_CTLCFG_GEN_SB_ERR 0x2000
>> +#define SDR_CTLCFG_GEN_DB_ERR 0x4000
>> +
>> +#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
>> + SDR_CTLCFG_ECC_CORR_EN)
>> +
>> +/* SDRAM Controller Address Widths Field Register */
>> +#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
>> +#define SDR_DRAMADDRW_COLBITS_LSB 0
>> +#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
>> +#define SDR_DRAMADDRW_ROWBITS_LSB 5
>> +#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
>> +#define SDR_DRAMADDRW_BANKBITS_LSB 10
>> +#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
>> +#define SDR_DRAMADDRW_CSBITS_LSB 13
> We normally call _LSB, _SHIFT.
OK. I will change.
>> +/* SDRAM Controller Interface Data Width Defines */
>> +#define SDR_DRAMIFWIDTH_16B_ECC 24
>> +#define SDR_DRAMIFWIDTH_32B_ECC 40
>> +
>> +/* SDRAM Controller DRAM Status Register Bit Masks */
>> +#define SDR_DRAMSTS_SBEERR 0x04
>> +#define SDR_DRAMSTS_DBEERR 0x08
>> +#define SDR_DRAMSTS_CORR_DROP 0x10
>> +
>> +/* SDRAM Controller DRAM IRQ Register Bit Masks */
>> +#define SDR_DRAMINTR_INTREN 0x01
>> +#define SDR_DRAMINTR_SBEMASK 0x02
>> +#define SDR_DRAMINTR_DBEMASK 0x04
>> +#define SDR_DRAMINTR_CORRDROPMASK 0x08
>> +#define SDR_DRAMINTR_INTRCLR 0x10
>> +
>> +/* SDRAM Controller Single Bit Error Count Register Bit Masks */
>> +#define SDR_SBECOUNT_COUNT_MASK 0x0F
>> +
>> +/* SDRAM Controller Double Bit Error Count Register Bit Masks */
>> +#define SDR_DBECOUNT_COUNT_MASK 0x0F
>> +
>> +/* SDRAM Controller ECC Error Address Register Bit Masks */
>> +#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
>> +
>> +/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
>> +#define SDR_DROPCOUNT_CORRMASK 0x0F
>> +
>> +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
>> +#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
>> +
>> +#define SELFRSHREQ_POS 3
>> +#define SELFRSHREQ_MASK 0x8
>> +
>> +#define SELFRFSHMASK_POS 4
>> +#define SELFRFSHMASK_MASK 0x30
>> +
>> +#define SELFRFSHACK_POS 1
>> +#define SELFRFSHACK_MASK 0x2
>> +
>> +struct altera_sdr {
>> + struct device *dev;
>> + void __iomem *reg_base;
>> +};
>> +
>> +/* Register access API */
>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
> Regmap?
Same as above. This seems to be more complex than we need.
Thanks for your comments and for reviewing!
Thor
>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr);
>> +
>> +#endif /* __LINUX_MFD_ALTERA_SDR_H */
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-07-31 20:00 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-07-31 20:00 UTC (permalink / raw)
To: Lee Jones
Cc: robherring2-Re5JQEeQqe8AvxtiuMwx3w, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, rob-VoJi6FS/r0vR7s880joybQ,
linux-lFZ/pmaqli7XmaaqVzeoHQ, atull-EIB2kfCEclfQT0dZR+AlfA,
delicious.quinoa-Re5JQEeQqe8AvxtiuMwx3w,
dinguyen-EIB2kfCEclfQT0dZR+AlfA,
dougthompson-aS9lmoZGLiVWk0Htik3J/w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A, bp-Gina5bIWoIWzQB+pC5nmwQ,
sameo-VuQAYsv1563Yd54FQh9/CA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-edac-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w, Alan Tull
On 07/31/2014 03:26 AM, Lee Jones wrote:
> On Wed, 30 Jul 2014, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>
>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>
>> Add a simple MFD for the Altera SDRAM Controller.
>>
>> Signed-off-by: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> ---
>> v1-8: The MFD implementation was not included in the original series.
>>
>> v9: New MFD implementation.
>> ---
>> MAINTAINERS | 5 ++
>> drivers/mfd/Kconfig | 7 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
>> 5 files changed, 277 insertions(+)
>> create mode 100644 drivers/mfd/altera-sdr.c
>> create mode 100644 include/linux/mfd/altera-sdr.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 86efa7e..48a8923 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
>> S: Maintained
>> F: drivers/clk/socfpga/
>>
>> +ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
>> +M: Thor Thayer <tthayer-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
>> +S: Maintained
>> +F: drivers/mfd/altera-sdr.c
>> +
>> ARM/STI ARCHITECTURE
>> M: Srinivas Kandagatla <srinivas.kandagatla-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> M: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
> This should be in a separate patch.
OK. Thanks for your comments and for reviewing. I will move this into a
separate patch.
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 6cc4b6a..8ce4961 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -719,6 +719,13 @@ config MFD_STMPE
>> Keypad: stmpe-keypad
>> Touchscreen: stmpe-ts
>>
>> +config MFD_ALTERA_SDR
>> + bool "Altera SDRAM Controller MFD"
>> + depends on ARCH_SOCFPGA
>> + select MFD_CORE
>> + help
>> + Support for Altera SDRAM Controller (SDR) MFD.
>> +
>> menu "STMicroelectronics STMPE Interface Drivers"
>> depends on MFD_STMPE
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 8afedba..24cc2b7 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
>> obj-$(CONFIG_MFD_AS3722) += as3722.o
>> obj-$(CONFIG_MFD_STW481X) += stw481x.o
>> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
>> +obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
>> diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
>> new file mode 100644
>> index 0000000..b5c6646
>> --- /dev/null
>> +++ b/drivers/mfd/altera-sdr.c
>> @@ -0,0 +1,162 @@
>> +/*
>> + * SDRAM Controller (SDR) MFD
>> + *
>> + * Copyright (C) 2014 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
> Can you use the shorter version of the licence?
Hi. This seems to be the shorter version of the license agreement and is
fairly common in the kernel, right?
>> + */
> '\n' here.
OK.
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/altera-sdr.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +static const struct mfd_cell altera_sdr_devs[] = {
>> +#if defined(CONFIG_EDAC_ALTERA_MC)
> No need to do this, as it will only be matched if the driver is
> enabled. Please remove the #iffery.
OK. Will remove.
>> + {
>> + .name = "altr_sdram_edac",
>> + .of_compatible = "altr,sdram-edac",
> What other devices will there be?
>
There will be an FPGA bridge and a power control driver that will need
access to the SDR Controller registers.
>> + },
>> +#endif
>> +};
>> +
>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>> +{
>> + return readl(sdr->reg_base + reg_offset);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>> +
>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>> +{
>> + writel(value, sdr->reg_base + reg_offset);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
> Why are you abstracting these?
>
> Might be better to use Regmap even.
regmap seems unnecessarily complex for what we're doing which is why
this method was chosen.
Future drivers will access different sets of registers in the device.
These drivers won't share bitfields in the same register so the MFD
seemed like the best solution. Originally we implemented this using
syscon but that seems to be frowned upon so we changed to using a MFD.
>> +/* Get total memory size in bytes */
>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
>> +{
>> + u32 size;
>> + u32 read_reg, row, bank, col, cs, width;
> Weird that size is on its own. Either place on a single line or
> separate them all.
OK. I will change this.
>> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
>> + if (read_reg < 0)
>> + return 0;
>> +
>> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
>> + if (width < 0)
>> + return 0;
>> +
>> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
>> + SDR_DRAMADDRW_COLBITS_LSB;
>> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
>> + SDR_DRAMADDRW_ROWBITS_LSB;
>> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
>> + SDR_DRAMADDRW_BANKBITS_LSB;
>> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
>> + SDR_DRAMADDRW_CSBITS_LSB;
> These would probably be better as macros.
>
OK. I will fix this.
>> + /* Correct for ECC as its not addressible */
>> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
>> + width = 32;
>> + if (width == SDR_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;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> Should this really be done in here? Isn't this an SDRAM function?
>
This register is part of the SDRAM controller and size information may
be required by the other drivers that share this memory area/need SDRAM
information.
>> +static int altera_sdr_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct altera_sdr *sdr;
>> + struct resource *res;
>> + void __iomem *base;
>> + int ret;
>> +
>> + sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
>> + if (!sdr)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENOENT;
>> +
>> + base = devm_ioremap(dev, res->start, resource_size(res));
> Instead use devm_ioremap_resource(), then you can omit the error
> checking of platform_get_resource().
OK. Thanks. I will make the change.
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + sdr->dev = &pdev->dev;
> Either use 'dev' here, or remove the top line in this function and use
> &pdev->dev everywhere. I personally prefer the latter.
OK. I will use the latter.
>> + sdr->reg_base = base;
>> +
>> + ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
>> + ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
>> + if (ret)
>> + dev_err(sdr->dev, "error adding devices");
>> +
>> + platform_set_drvdata(pdev, sdr);
>> +
>> + dev_dbg(dev, "Altera SDR MFD registered\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_sdr_remove(struct platform_device *pdev)
>> +{
>> + struct altera_sdr *sdr = platform_get_drvdata(pdev);
> No need for this, just use &pdev->dev.
OK. I will make the change.
>> + mfd_remove_devices(sdr->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id of_altera_sdr_match[] = {
>> + { .compatible = "altr,sdr", },
>> + { },
>> +};
>> +
>> +static const struct platform_device_id altera_sdr_ids[] = {
>> + { "altera_sdr", },
>> + { }
>> +};
> What's this for?
We don't strictly need it because we are driven by the device tree. It
can be removed it if is a problem but I'm not clear why it is a problem.
>> +static struct platform_driver altera_sdr_driver = {
>> + .driver = {
>> + .name = "altera_sdr",
>> + .owner = THIS_MODULE,
> You can remove this line, it's taken care of for you.
I will remove the .owner line.
>> + .of_match_table = of_altera_sdr_match,
>> + },
>> + .probe = altera_sdr_probe,
>> + .remove = altera_sdr_remove,
>> + .id_table = altera_sdr_ids,
>> +};
>> +
>> +static int __init altera_sdr_init(void)
>> +{
>> + return platform_driver_register(&altera_sdr_driver);
>> +}
>> +postcore_initcall(altera_sdr_init);
> Why was this chosen?
We want this to happen pretty early.
>> +static void __exit altera_sdr_exit(void)
>> +{
>> + platform_driver_unregister(&altera_sdr_driver);
>> +}
>> +module_exit(altera_sdr_exit);
>> +
>> +MODULE_AUTHOR("Alan Tull <atull-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>");
>> +MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
>> new file mode 100644
>> index 0000000..a5f5c39
>> --- /dev/null
>> +++ b/include/linux/mfd/altera-sdr.h
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Copyright (C) 2014 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
> Use the short version.
Same as above. This is the shorter version that we've been using at Altera.
>> + */
> '\n' here.
OK.
>> +#ifndef __LINUX_MFD_ALTERA_SDR_H
>> +#define __LINUX_MFD_ALTERA_SDR_H
>> +
>> +/* SDRAM Controller register offsets */
>> +#define SDR_CTLCFG_OFST 0x00
>> +#define SDR_DRAMADDRW_OFST 0x2C
>> +#define SDR_DRAMIFWIDTH_OFST 0x30
>> +#define SDR_DRAMSTS_OFST 0x38
>> +#define SDR_DRAMINTR_OFST 0x3C
>> +#define SDR_SBECOUNT_OFST 0x40
>> +#define SDR_DBECOUNT_OFST 0x44
>> +#define SDR_ERRADDR_OFST 0x48
>> +#define SDR_DROPCOUNT_OFST 0x4C
>> +#define SDR_DROPADDR_OFST 0x50
>> +#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
>> +#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
>> +
>> +/* SDRAM Controller CtrlCfg Register Bit Masks */
>> +#define SDR_CTLCFG_ECC_EN 0x400
>> +#define SDR_CTLCFG_ECC_CORR_EN 0x800
>> +#define SDR_CTLCFG_GEN_SB_ERR 0x2000
>> +#define SDR_CTLCFG_GEN_DB_ERR 0x4000
>> +
>> +#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
>> + SDR_CTLCFG_ECC_CORR_EN)
>> +
>> +/* SDRAM Controller Address Widths Field Register */
>> +#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
>> +#define SDR_DRAMADDRW_COLBITS_LSB 0
>> +#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
>> +#define SDR_DRAMADDRW_ROWBITS_LSB 5
>> +#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
>> +#define SDR_DRAMADDRW_BANKBITS_LSB 10
>> +#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
>> +#define SDR_DRAMADDRW_CSBITS_LSB 13
> We normally call _LSB, _SHIFT.
OK. I will change.
>> +/* SDRAM Controller Interface Data Width Defines */
>> +#define SDR_DRAMIFWIDTH_16B_ECC 24
>> +#define SDR_DRAMIFWIDTH_32B_ECC 40
>> +
>> +/* SDRAM Controller DRAM Status Register Bit Masks */
>> +#define SDR_DRAMSTS_SBEERR 0x04
>> +#define SDR_DRAMSTS_DBEERR 0x08
>> +#define SDR_DRAMSTS_CORR_DROP 0x10
>> +
>> +/* SDRAM Controller DRAM IRQ Register Bit Masks */
>> +#define SDR_DRAMINTR_INTREN 0x01
>> +#define SDR_DRAMINTR_SBEMASK 0x02
>> +#define SDR_DRAMINTR_DBEMASK 0x04
>> +#define SDR_DRAMINTR_CORRDROPMASK 0x08
>> +#define SDR_DRAMINTR_INTRCLR 0x10
>> +
>> +/* SDRAM Controller Single Bit Error Count Register Bit Masks */
>> +#define SDR_SBECOUNT_COUNT_MASK 0x0F
>> +
>> +/* SDRAM Controller Double Bit Error Count Register Bit Masks */
>> +#define SDR_DBECOUNT_COUNT_MASK 0x0F
>> +
>> +/* SDRAM Controller ECC Error Address Register Bit Masks */
>> +#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
>> +
>> +/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
>> +#define SDR_DROPCOUNT_CORRMASK 0x0F
>> +
>> +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
>> +#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
>> +
>> +#define SELFRSHREQ_POS 3
>> +#define SELFRSHREQ_MASK 0x8
>> +
>> +#define SELFRFSHMASK_POS 4
>> +#define SELFRFSHMASK_MASK 0x30
>> +
>> +#define SELFRFSHACK_POS 1
>> +#define SELFRFSHACK_MASK 0x2
>> +
>> +struct altera_sdr {
>> + struct device *dev;
>> + void __iomem *reg_base;
>> +};
>> +
>> +/* Register access API */
>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
> Regmap?
Same as above. This seems to be more complex than we need.
Thanks for your comments and for reviewing!
Thor
>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr);
>> +
>> +#endif /* __LINUX_MFD_ALTERA_SDR_H */
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-07-31 20:00 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-07-31 20:00 UTC (permalink / raw)
To: linux-arm-kernel
On 07/31/2014 03:26 AM, Lee Jones wrote:
> On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
>
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add a simple MFD for the Altera SDRAM Controller.
>>
>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v1-8: The MFD implementation was not included in the original series.
>>
>> v9: New MFD implementation.
>> ---
>> MAINTAINERS | 5 ++
>> drivers/mfd/Kconfig | 7 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
>> 5 files changed, 277 insertions(+)
>> create mode 100644 drivers/mfd/altera-sdr.c
>> create mode 100644 include/linux/mfd/altera-sdr.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 86efa7e..48a8923 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1340,6 +1340,11 @@ M: Dinh Nguyen <dinguyen@altera.com>
>> S: Maintained
>> F: drivers/clk/socfpga/
>>
>> +ARM/SOCFPGA SDRAM CONTROLLER SUPPORT
>> +M: Thor Thayer <tthayer@altera.com>
>> +S: Maintained
>> +F: drivers/mfd/altera-sdr.c
>> +
>> ARM/STI ARCHITECTURE
>> M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
>> M: Maxime Coquelin <maxime.coquelin@st.com>
> This should be in a separate patch.
OK. Thanks for your comments and for reviewing. I will move this into a
separate patch.
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 6cc4b6a..8ce4961 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -719,6 +719,13 @@ config MFD_STMPE
>> Keypad: stmpe-keypad
>> Touchscreen: stmpe-ts
>>
>> +config MFD_ALTERA_SDR
>> + bool "Altera SDRAM Controller MFD"
>> + depends on ARCH_SOCFPGA
>> + select MFD_CORE
>> + help
>> + Support for Altera SDRAM Controller (SDR) MFD.
>> +
>> menu "STMicroelectronics STMPE Interface Drivers"
>> depends on MFD_STMPE
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 8afedba..24cc2b7 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711) += as3711.o
>> obj-$(CONFIG_MFD_AS3722) += as3722.o
>> obj-$(CONFIG_MFD_STW481X) += stw481x.o
>> obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
>> +obj-$(CONFIG_MFD_ALTERA_SDR) += altera-sdr.o
>> diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
>> new file mode 100644
>> index 0000000..b5c6646
>> --- /dev/null
>> +++ b/drivers/mfd/altera-sdr.c
>> @@ -0,0 +1,162 @@
>> +/*
>> + * SDRAM Controller (SDR) MFD
>> + *
>> + * Copyright (C) 2014 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
> Can you use the shorter version of the licence?
Hi. This seems to be the shorter version of the license agreement and is
fairly common in the kernel, right?
>> + */
> '\n' here.
OK.
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/altera-sdr.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +static const struct mfd_cell altera_sdr_devs[] = {
>> +#if defined(CONFIG_EDAC_ALTERA_MC)
> No need to do this, as it will only be matched if the driver is
> enabled. Please remove the #iffery.
OK. Will remove.
>> + {
>> + .name = "altr_sdram_edac",
>> + .of_compatible = "altr,sdram-edac",
> What other devices will there be?
>
There will be an FPGA bridge and a power control driver that will need
access to the SDR Controller registers.
>> + },
>> +#endif
>> +};
>> +
>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>> +{
>> + return readl(sdr->reg_base + reg_offset);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>> +
>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>> +{
>> + writel(value, sdr->reg_base + reg_offset);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
> Why are you abstracting these?
>
> Might be better to use Regmap even.
regmap seems unnecessarily complex for what we're doing which is why
this method was chosen.
Future drivers will access different sets of registers in the device.
These drivers won't share bitfields in the same register so the MFD
seemed like the best solution. Originally we implemented this using
syscon but that seems to be frowned upon so we changed to using a MFD.
>> +/* Get total memory size in bytes */
>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
>> +{
>> + u32 size;
>> + u32 read_reg, row, bank, col, cs, width;
> Weird that size is on its own. Either place on a single line or
> separate them all.
OK. I will change this.
>> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
>> + if (read_reg < 0)
>> + return 0;
>> +
>> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
>> + if (width < 0)
>> + return 0;
>> +
>> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
>> + SDR_DRAMADDRW_COLBITS_LSB;
>> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
>> + SDR_DRAMADDRW_ROWBITS_LSB;
>> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
>> + SDR_DRAMADDRW_BANKBITS_LSB;
>> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
>> + SDR_DRAMADDRW_CSBITS_LSB;
> These would probably be better as macros.
>
OK. I will fix this.
>> + /* Correct for ECC as its not addressible */
>> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
>> + width = 32;
>> + if (width == SDR_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;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> Should this really be done in here? Isn't this an SDRAM function?
>
This register is part of the SDRAM controller and size information may
be required by the other drivers that share this memory area/need SDRAM
information.
>> +static int altera_sdr_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct altera_sdr *sdr;
>> + struct resource *res;
>> + void __iomem *base;
>> + int ret;
>> +
>> + sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
>> + if (!sdr)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENOENT;
>> +
>> + base = devm_ioremap(dev, res->start, resource_size(res));
> Instead use devm_ioremap_resource(), then you can omit the error
> checking of platform_get_resource().
OK. Thanks. I will make the change.
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + sdr->dev = &pdev->dev;
> Either use 'dev' here, or remove the top line in this function and use
> &pdev->dev everywhere. I personally prefer the latter.
OK. I will use the latter.
>> + sdr->reg_base = base;
>> +
>> + ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
>> + ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
>> + if (ret)
>> + dev_err(sdr->dev, "error adding devices");
>> +
>> + platform_set_drvdata(pdev, sdr);
>> +
>> + dev_dbg(dev, "Altera SDR MFD registered\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_sdr_remove(struct platform_device *pdev)
>> +{
>> + struct altera_sdr *sdr = platform_get_drvdata(pdev);
> No need for this, just use &pdev->dev.
OK. I will make the change.
>> + mfd_remove_devices(sdr->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id of_altera_sdr_match[] = {
>> + { .compatible = "altr,sdr", },
>> + { },
>> +};
>> +
>> +static const struct platform_device_id altera_sdr_ids[] = {
>> + { "altera_sdr", },
>> + { }
>> +};
> What's this for?
We don't strictly need it because we are driven by the device tree. It
can be removed it if is a problem but I'm not clear why it is a problem.
>> +static struct platform_driver altera_sdr_driver = {
>> + .driver = {
>> + .name = "altera_sdr",
>> + .owner = THIS_MODULE,
> You can remove this line, it's taken care of for you.
I will remove the .owner line.
>> + .of_match_table = of_altera_sdr_match,
>> + },
>> + .probe = altera_sdr_probe,
>> + .remove = altera_sdr_remove,
>> + .id_table = altera_sdr_ids,
>> +};
>> +
>> +static int __init altera_sdr_init(void)
>> +{
>> + return platform_driver_register(&altera_sdr_driver);
>> +}
>> +postcore_initcall(altera_sdr_init);
> Why was this chosen?
We want this to happen pretty early.
>> +static void __exit altera_sdr_exit(void)
>> +{
>> + platform_driver_unregister(&altera_sdr_driver);
>> +}
>> +module_exit(altera_sdr_exit);
>> +
>> +MODULE_AUTHOR("Alan Tull <atull@altera.com>");
>> +MODULE_DESCRIPTION("Altera SDRAM Controller (SDR) MFD");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/altera-sdr.h b/include/linux/mfd/altera-sdr.h
>> new file mode 100644
>> index 0000000..a5f5c39
>> --- /dev/null
>> +++ b/include/linux/mfd/altera-sdr.h
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Copyright (C) 2014 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
> Use the short version.
Same as above. This is the shorter version that we've been using at Altera.
>> + */
> '\n' here.
OK.
>> +#ifndef __LINUX_MFD_ALTERA_SDR_H
>> +#define __LINUX_MFD_ALTERA_SDR_H
>> +
>> +/* SDRAM Controller register offsets */
>> +#define SDR_CTLCFG_OFST 0x00
>> +#define SDR_DRAMADDRW_OFST 0x2C
>> +#define SDR_DRAMIFWIDTH_OFST 0x30
>> +#define SDR_DRAMSTS_OFST 0x38
>> +#define SDR_DRAMINTR_OFST 0x3C
>> +#define SDR_SBECOUNT_OFST 0x40
>> +#define SDR_DBECOUNT_OFST 0x44
>> +#define SDR_ERRADDR_OFST 0x48
>> +#define SDR_DROPCOUNT_OFST 0x4C
>> +#define SDR_DROPADDR_OFST 0x50
>> +#define SDR_CTRLGRP_LOWPWREQ_OFST 0x54
>> +#define SDR_CTRLGRP_LOWPWRACK_OFST 0x58
>> +
>> +/* SDRAM Controller CtrlCfg Register Bit Masks */
>> +#define SDR_CTLCFG_ECC_EN 0x400
>> +#define SDR_CTLCFG_ECC_CORR_EN 0x800
>> +#define SDR_CTLCFG_GEN_SB_ERR 0x2000
>> +#define SDR_CTLCFG_GEN_DB_ERR 0x4000
>> +
>> +#define SDR_CTLCFG_ECC_AUTO_EN (SDR_CTLCFG_ECC_EN | \
>> + SDR_CTLCFG_ECC_CORR_EN)
>> +
>> +/* SDRAM Controller Address Widths Field Register */
>> +#define SDR_DRAMADDRW_COLBITS_MASK 0x001F
>> +#define SDR_DRAMADDRW_COLBITS_LSB 0
>> +#define SDR_DRAMADDRW_ROWBITS_MASK 0x03E0
>> +#define SDR_DRAMADDRW_ROWBITS_LSB 5
>> +#define SDR_DRAMADDRW_BANKBITS_MASK 0x1C00
>> +#define SDR_DRAMADDRW_BANKBITS_LSB 10
>> +#define SDR_DRAMADDRW_CSBITS_MASK 0xE000
>> +#define SDR_DRAMADDRW_CSBITS_LSB 13
> We normally call _LSB, _SHIFT.
OK. I will change.
>> +/* SDRAM Controller Interface Data Width Defines */
>> +#define SDR_DRAMIFWIDTH_16B_ECC 24
>> +#define SDR_DRAMIFWIDTH_32B_ECC 40
>> +
>> +/* SDRAM Controller DRAM Status Register Bit Masks */
>> +#define SDR_DRAMSTS_SBEERR 0x04
>> +#define SDR_DRAMSTS_DBEERR 0x08
>> +#define SDR_DRAMSTS_CORR_DROP 0x10
>> +
>> +/* SDRAM Controller DRAM IRQ Register Bit Masks */
>> +#define SDR_DRAMINTR_INTREN 0x01
>> +#define SDR_DRAMINTR_SBEMASK 0x02
>> +#define SDR_DRAMINTR_DBEMASK 0x04
>> +#define SDR_DRAMINTR_CORRDROPMASK 0x08
>> +#define SDR_DRAMINTR_INTRCLR 0x10
>> +
>> +/* SDRAM Controller Single Bit Error Count Register Bit Masks */
>> +#define SDR_SBECOUNT_COUNT_MASK 0x0F
>> +
>> +/* SDRAM Controller Double Bit Error Count Register Bit Masks */
>> +#define SDR_DBECOUNT_COUNT_MASK 0x0F
>> +
>> +/* SDRAM Controller ECC Error Address Register Bit Masks */
>> +#define SDR_ERRADDR_ADDR_MASK 0xFFFFFFFF
>> +
>> +/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
>> +#define SDR_DROPCOUNT_CORRMASK 0x0F
>> +
>> +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
>> +#define SDR_DROPADDR_ADDR_MASK 0xFFFFFFFF
>> +
>> +#define SELFRSHREQ_POS 3
>> +#define SELFRSHREQ_MASK 0x8
>> +
>> +#define SELFRFSHMASK_POS 4
>> +#define SELFRFSHMASK_MASK 0x30
>> +
>> +#define SELFRFSHACK_POS 1
>> +#define SELFRFSHACK_MASK 0x2
>> +
>> +struct altera_sdr {
>> + struct device *dev;
>> + void __iomem *reg_base;
>> +};
>> +
>> +/* Register access API */
>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
> Regmap?
Same as above. This seems to be more complex than we need.
Thanks for your comments and for reviewing!
Thor
>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr);
>> +
>> +#endif /* __LINUX_MFD_ALTERA_SDR_H */
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-07-31 20:00 ` Thor Thayer
@ 2014-08-01 8:13 ` Lee Jones
-1 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2014-08-01 8:13 UTC (permalink / raw)
To: Thor Thayer
Cc: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, devicetree, linux-doc, linux-edac,
linux-kernel, linux-arm-kernel, tthayer.linux, Alan Tull
On Thu, 31 Jul 2014, Thor Thayer wrote:
> On 07/31/2014 03:26 AM, Lee Jones wrote:
> >On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
> >
> >>From: Thor Thayer <tthayer@opensource.altera.com>
> >>
> >>Add a simple MFD for the Altera SDRAM Controller.
> >>
> >>Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >>Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> >>---
> >>v1-8: The MFD implementation was not included in the original series.
> >>
> >>v9: New MFD implementation.
> >>---
> >> MAINTAINERS | 5 ++
> >> drivers/mfd/Kconfig | 7 ++
> >> drivers/mfd/Makefile | 1 +
> >> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> >> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> >> 5 files changed, 277 insertions(+)
> >> create mode 100644 drivers/mfd/altera-sdr.c
> >> create mode 100644 include/linux/mfd/altera-sdr.h
[...]
> >>+++ b/drivers/mfd/altera-sdr.c
> >>@@ -0,0 +1,162 @@
> >>+/*
> >>+ * SDRAM Controller (SDR) MFD
> >>+ *
> >>+ * Copyright (C) 2014 Altera Corporation
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms and conditions of the GNU General Public License,
> >>+ * version 2, as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> >>+ * more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License along with
> >>+ * this program. If not, see <http://www.gnu.org/licenses/>.
> >Can you use the shorter version of the licence?
> Hi. This seems to be the shorter version of the license agreement
> and is fairly common in the kernel, right?
This is the long(ish) version. Many programs have the Copyright line
and the "This program is free software;" line. It'll also be a good
idea to keep the link to the full licence.
[...]
> >>+ {
> >>+ .name = "altr_sdram_edac",
> >>+ .of_compatible = "altr,sdram-edac",
> >What other devices will there be?
> >
> There will be an FPGA bridge and a power control driver that will
> need access to the SDR Controller registers.
Okay. Do you know when they'll be upstreamed?
> >>+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
> >>+{
> >>+ return readl(sdr->reg_base + reg_offset);
> >>+}
> >>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>+
> >>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>+{
> >>+ writel(value, sdr->reg_base + reg_offset);
> >>+}
> >>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> >Why are you abstracting these?
> >
> >Might be better to use Regmap even.
> regmap seems unnecessarily complex for what we're doing which is why
> this method was chosen.
>
> Future drivers will access different sets of registers in the
> device. These drivers won't share bitfields in the same register so
> the MFD seemed like the best solution. Originally we implemented
> this using syscon but that seems to be frowned upon so we changed to
> using a MFD.
Why was the use of syscon frowned upon? Can you link me to the
thread? Writing directly to the registers sounds to me a lot worse
than using infrastructure which was designed for these kinds of
accesses.
If you do choose to fiddle with the registers in this manner, is there
any reason why you're calling back into here, rather than using
readl() and writel() directly?
> >>+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
> >>+{
> >>+ u32 size;
> >>+ u32 read_reg, row, bank, col, cs, width;
> >Weird that size is on its own. Either place on a single line or
> >separate them all.
> OK. I will change this.
> >>+ read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
> >>+ if (read_reg < 0)
> >>+ return 0;
> >>+
> >>+ width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
> >>+ if (width < 0)
> >>+ return 0;
u32s cant be < 0. The 'u' means 'unsigned'.
> >>+ col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
> >>+ SDR_DRAMADDRW_COLBITS_LSB;
> >>+ row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
> >>+ SDR_DRAMADDRW_ROWBITS_LSB;
> >>+ bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
> >>+ SDR_DRAMADDRW_BANKBITS_LSB;
> >>+ cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
> >>+ SDR_DRAMADDRW_CSBITS_LSB;
> >These would probably be better as macros.
> >
> OK. I will fix this.
> >>+ /* Correct for ECC as its not addressible */
> >>+ if (width == SDR_DRAMIFWIDTH_32B_ECC)
> >>+ width = 32;
> >>+ if (width == SDR_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;
> >>+}
> >>+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> >Should this really be done in here? Isn't this an SDRAM function?
> >
> This register is part of the SDRAM controller and size information
> may be required by the other drivers that share this memory
> area/need SDRAM information.
Then export a function from the SDRAM driver, not from here.
[...]
> >>+static const struct platform_device_id altera_sdr_ids[] = {
> >>+ { "altera_sdr", },
> >>+ { }
> >>+};
> >What's this for?
> We don't strictly need it because we are driven by the device tree.
> It can be removed it if is a problem but I'm not clear why it is a
> problem.
It's a problem because it's unused, superfluous bumph. :)
[...]
> >>+static int __init altera_sdr_init(void)
> >>+{
> >>+ return platform_driver_register(&altera_sdr_driver);
> >>+}
> >>+postcore_initcall(altera_sdr_init);
> >Why was this chosen?
> We want this to happen pretty early.
If you _need_ this is happen early, core_initcall() is more commonly
used, but _why_ do you need it to happen this early?
> >>+/*
> >>+ * Copyright (C) 2014 Altera Corporation
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms and conditions of the GNU General Public License,
> >>+ * version 2, as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> >>+ * more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License along with
> >>+ * this program. If not, see <http://www.gnu.org/licenses/>.
> >Use the short version.
> Same as above. This is the shorter version that we've been using at Altera.
If you read COPYING, you only require the Copyright line and a link to
the full licence. In this case I'll be happy if you left in the "This
program is free software;" line as well, but remove the paragraph
below it.
[...]
> >>+/* Register access API */
> >>+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
> >>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
> >Regmap?
> Same as above. This seems to be more complex than we need.
I'm not sure I believe that. Regmap is as complex or as simple as you
need.
> Thanks for your comments and for reviewing!
>
> Thor
You're welcome. OOI, do you own a hammer? ;)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-01 8:13 ` Lee Jones
0 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2014-08-01 8:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 31 Jul 2014, Thor Thayer wrote:
> On 07/31/2014 03:26 AM, Lee Jones wrote:
> >On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
> >
> >>From: Thor Thayer <tthayer@opensource.altera.com>
> >>
> >>Add a simple MFD for the Altera SDRAM Controller.
> >>
> >>Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >>Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> >>---
> >>v1-8: The MFD implementation was not included in the original series.
> >>
> >>v9: New MFD implementation.
> >>---
> >> MAINTAINERS | 5 ++
> >> drivers/mfd/Kconfig | 7 ++
> >> drivers/mfd/Makefile | 1 +
> >> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> >> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> >> 5 files changed, 277 insertions(+)
> >> create mode 100644 drivers/mfd/altera-sdr.c
> >> create mode 100644 include/linux/mfd/altera-sdr.h
[...]
> >>+++ b/drivers/mfd/altera-sdr.c
> >>@@ -0,0 +1,162 @@
> >>+/*
> >>+ * SDRAM Controller (SDR) MFD
> >>+ *
> >>+ * Copyright (C) 2014 Altera Corporation
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms and conditions of the GNU General Public License,
> >>+ * version 2, as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> >>+ * more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License along with
> >>+ * this program. If not, see <http://www.gnu.org/licenses/>.
> >Can you use the shorter version of the licence?
> Hi. This seems to be the shorter version of the license agreement
> and is fairly common in the kernel, right?
This is the long(ish) version. Many programs have the Copyright line
and the "This program is free software;" line. It'll also be a good
idea to keep the link to the full licence.
[...]
> >>+ {
> >>+ .name = "altr_sdram_edac",
> >>+ .of_compatible = "altr,sdram-edac",
> >What other devices will there be?
> >
> There will be an FPGA bridge and a power control driver that will
> need access to the SDR Controller registers.
Okay. Do you know when they'll be upstreamed?
> >>+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
> >>+{
> >>+ return readl(sdr->reg_base + reg_offset);
> >>+}
> >>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>+
> >>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>+{
> >>+ writel(value, sdr->reg_base + reg_offset);
> >>+}
> >>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> >Why are you abstracting these?
> >
> >Might be better to use Regmap even.
> regmap seems unnecessarily complex for what we're doing which is why
> this method was chosen.
>
> Future drivers will access different sets of registers in the
> device. These drivers won't share bitfields in the same register so
> the MFD seemed like the best solution. Originally we implemented
> this using syscon but that seems to be frowned upon so we changed to
> using a MFD.
Why was the use of syscon frowned upon? Can you link me to the
thread? Writing directly to the registers sounds to me a lot worse
than using infrastructure which was designed for these kinds of
accesses.
If you do choose to fiddle with the registers in this manner, is there
any reason why you're calling back into here, rather than using
readl() and writel() directly?
> >>+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
> >>+{
> >>+ u32 size;
> >>+ u32 read_reg, row, bank, col, cs, width;
> >Weird that size is on its own. Either place on a single line or
> >separate them all.
> OK. I will change this.
> >>+ read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
> >>+ if (read_reg < 0)
> >>+ return 0;
> >>+
> >>+ width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
> >>+ if (width < 0)
> >>+ return 0;
u32s cant be < 0. The 'u' means 'unsigned'.
> >>+ col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
> >>+ SDR_DRAMADDRW_COLBITS_LSB;
> >>+ row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
> >>+ SDR_DRAMADDRW_ROWBITS_LSB;
> >>+ bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
> >>+ SDR_DRAMADDRW_BANKBITS_LSB;
> >>+ cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
> >>+ SDR_DRAMADDRW_CSBITS_LSB;
> >These would probably be better as macros.
> >
> OK. I will fix this.
> >>+ /* Correct for ECC as its not addressible */
> >>+ if (width == SDR_DRAMIFWIDTH_32B_ECC)
> >>+ width = 32;
> >>+ if (width == SDR_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;
> >>+}
> >>+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> >Should this really be done in here? Isn't this an SDRAM function?
> >
> This register is part of the SDRAM controller and size information
> may be required by the other drivers that share this memory
> area/need SDRAM information.
Then export a function from the SDRAM driver, not from here.
[...]
> >>+static const struct platform_device_id altera_sdr_ids[] = {
> >>+ { "altera_sdr", },
> >>+ { }
> >>+};
> >What's this for?
> We don't strictly need it because we are driven by the device tree.
> It can be removed it if is a problem but I'm not clear why it is a
> problem.
It's a problem because it's unused, superfluous bumph. :)
[...]
> >>+static int __init altera_sdr_init(void)
> >>+{
> >>+ return platform_driver_register(&altera_sdr_driver);
> >>+}
> >>+postcore_initcall(altera_sdr_init);
> >Why was this chosen?
> We want this to happen pretty early.
If you _need_ this is happen early, core_initcall() is more commonly
used, but _why_ do you need it to happen this early?
> >>+/*
> >>+ * Copyright (C) 2014 Altera Corporation
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms and conditions of the GNU General Public License,
> >>+ * version 2, as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> >>+ * more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License along with
> >>+ * this program. If not, see <http://www.gnu.org/licenses/>.
> >Use the short version.
> Same as above. This is the shorter version that we've been using at Altera.
If you read COPYING, you only require the Copyright line and a link to
the full licence. In this case I'll be happy if you left in the "This
program is free software;" line as well, but remove the paragraph
below it.
[...]
> >>+/* Register access API */
> >>+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
> >>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
> >Regmap?
> Same as above. This seems to be more complex than we need.
I'm not sure I believe that. Regmap is as complex or as simple as you
need.
> Thanks for your comments and for reviewing!
>
> Thor
You're welcome. OOI, do you own a hammer? ;)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-08-01 8:13 ` Lee Jones
(?)
@ 2014-08-01 22:27 ` Thor Thayer
-1 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-01 22:27 UTC (permalink / raw)
To: Lee Jones
Cc: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, devicetree, linux-doc, linux-edac,
linux-kernel, linux-arm-kernel, tthayer.linux, Alan Tull
On 08/01/2014 03:13 AM, Lee Jones wrote:
> On Thu, 31 Jul 2014, Thor Thayer wrote:
>> On 07/31/2014 03:26 AM, Lee Jones wrote:
>>> On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
>>>
>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>
>>>> Add a simple MFD for the Altera SDRAM Controller.
>>>>
>>>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>>>> ---
>>>> v1-8: The MFD implementation was not included in the original series.
>>>>
>>>> v9: New MFD implementation.
>>>> ---
>>>> MAINTAINERS | 5 ++
>>>> drivers/mfd/Kconfig | 7 ++
>>>> drivers/mfd/Makefile | 1 +
>>>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
>>>> 5 files changed, 277 insertions(+)
>>>> create mode 100644 drivers/mfd/altera-sdr.c
>>>> create mode 100644 include/linux/mfd/altera-sdr.h
> [...]
>
>>>> +++ b/drivers/mfd/altera-sdr.c
>>>> @@ -0,0 +1,162 @@
>>>> +/*
>>>> + * SDRAM Controller (SDR) MFD
>>>> + *
>>>> + * Copyright (C) 2014 Altera Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> Can you use the shorter version of the licence?
>> Hi. This seems to be the shorter version of the license agreement
>> and is fairly common in the kernel, right?
> This is the long(ish) version. Many programs have the Copyright line
> and the "This program is free software;" line. It'll also be a good
> idea to keep the link to the full licence.
>
> [...]
Hi Lee.
Our legal department requires this header. Their reasoning is that they
want to retain the rights and warranty language with the file (just in
case the COPYING file changes).
>>>> + {
>>>> + .name = "altr_sdram_edac",
>>>> + .of_compatible = "altr,sdram-edac",
>>> What other devices will there be?
>>>
>> There will be an FPGA bridge and a power control driver that will
>> need access to the SDR Controller registers.
> Okay. Do you know when they'll be upstreamed?
The other drivers are waiting on this file as a pre-requisite.
>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>>>> +{
>>>> + return readl(sdr->reg_base + reg_offset);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>>>> +
>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>>>> +{
>>>> + writel(value, sdr->reg_base + reg_offset);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
>>> Why are you abstracting these?
>>>
>>> Might be better to use Regmap even.
>> regmap seems unnecessarily complex for what we're doing which is why
>> this method was chosen.
>>
>> Future drivers will access different sets of registers in the
>> device. These drivers won't share bitfields in the same register so
>> the MFD seemed like the best solution. Originally we implemented
>> this using syscon but that seems to be frowned upon so we changed to
>> using a MFD.
> Why was the use of syscon frowned upon? Can you link me to the
> thread? Writing directly to the registers sounds to me a lot worse
> than using infrastructure which was designed for these kinds of
> accesses.
>
> If you do choose to fiddle with the registers in this manner, is there
> any reason why you're calling back into here, rather than using
> readl() and writel() directly?
>
We'd prefer to use syscon and that is what we started with. If you'd
like to be our advocate, I will return to that because it was pretty
clean. My primary concern is to get it upstreamed and if it is MFD then
I'll make the changes.
Here are the threads.
http://marc.info/?l=linux-kernel&m=140128791902800&w=2
and
http://article.gmane.org/gmane.linux.kernel/1679601
>>>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
>>>> +{
>>>> + u32 size;
>>>> + u32 read_reg, row, bank, col, cs, width;
>>> Weird that size is on its own. Either place on a single line or
>>> separate them all.
>> OK. I will change this.
>>>> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
>>>> + if (read_reg < 0)
>>>> + return 0;
>>>> +
>>>> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
>>>> + if (width < 0)
>>>> + return 0;
> u32s cant be < 0. The 'u' means 'unsigned'.
Whoops. Good catch.
>>>> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
>>>> + SDR_DRAMADDRW_COLBITS_LSB;
>>>> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
>>>> + SDR_DRAMADDRW_ROWBITS_LSB;
>>>> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
>>>> + SDR_DRAMADDRW_BANKBITS_LSB;
>>>> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
>>>> + SDR_DRAMADDRW_CSBITS_LSB;
>>> These would probably be better as macros.
>>>
>> OK. I will fix this.
>>>> + /* Correct for ECC as its not addressible */
>>>> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
>>>> + width = 32;
>>>> + if (width == SDR_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;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
>>> Should this really be done in here? Isn't this an SDRAM function?
>>>
>> This register is part of the SDRAM controller and size information
>> may be required by the other drivers that share this memory
>> area/need SDRAM information.
> Then export a function from the SDRAM driver, not from here.
We don't have an SDRAM driver. Although I could put this in the EDAC
driver it would be lost to anyone else wanting this functionality so
this seemed to be the logical place.
>
> [...]
>
>>>> +static const struct platform_device_id altera_sdr_ids[] = {
>>>> + { "altera_sdr", },
>>>> + { }
>>>> +};
>>> What's this for?
>> We don't strictly need it because we are driven by the device tree.
>> It can be removed it if is a problem but I'm not clear why it is a
>> problem.
> It's a problem because it's unused, superfluous bumph. :)
OK. I will remove it.
>
> [...]
>
>>>> +static int __init altera_sdr_init(void)
>>>> +{
>>>> + return platform_driver_register(&altera_sdr_driver);
>>>> +}
>>>> +postcore_initcall(altera_sdr_init);
>>> Why was this chosen?
>> We want this to happen pretty early.
> If you _need_ this is happen early, core_initcall() is more commonly
> used, but _why_ do you need it to happen this early?
The syscon driver used this designation. After talking with Alan, this
could be changed to a core_initcall(). However, it could also be a
subsys_initcall which seems to be more common in the MFD drivers.
>>>> +/*
>>>> + * Copyright (C) 2014 Altera Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> Use the short version.
>> Same as above. This is the shorter version that we've been using at Altera.
> If you read COPYING, you only require the Copyright line and a link to
> the full licence. In this case I'll be happy if you left in the "This
> program is free software;" line as well, but remove the paragraph
> below it.
>
> [...]
>
>>>> +/* Register access API */
>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
>>> Regmap?
>> Same as above. This seems to be more complex than we need.
> I'm not sure I believe that. Regmap is as complex or as simple as you
> need.
OK. This is a moot point if we decide to go back to using syscon.
If we stick to using an MFD driver, then we can look at regmap. If we
stick with MFD, the code really starts to look like syscon.
Thanks!
>> Thanks for your comments and for reviewing!
>>
>> Thor
> You're welcome. OOI, do you own a hammer? ;)
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-01 22:27 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-01 22:27 UTC (permalink / raw)
To: Lee Jones
Cc: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, devicetree, linux-doc, linux-edac,
linux-kernel, linux-arm-kernel, tthayer.linux, Alan Tull
On 08/01/2014 03:13 AM, Lee Jones wrote:
> On Thu, 31 Jul 2014, Thor Thayer wrote:
>> On 07/31/2014 03:26 AM, Lee Jones wrote:
>>> On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
>>>
>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>
>>>> Add a simple MFD for the Altera SDRAM Controller.
>>>>
>>>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>>>> ---
>>>> v1-8: The MFD implementation was not included in the original series.
>>>>
>>>> v9: New MFD implementation.
>>>> ---
>>>> MAINTAINERS | 5 ++
>>>> drivers/mfd/Kconfig | 7 ++
>>>> drivers/mfd/Makefile | 1 +
>>>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
>>>> 5 files changed, 277 insertions(+)
>>>> create mode 100644 drivers/mfd/altera-sdr.c
>>>> create mode 100644 include/linux/mfd/altera-sdr.h
> [...]
>
>>>> +++ b/drivers/mfd/altera-sdr.c
>>>> @@ -0,0 +1,162 @@
>>>> +/*
>>>> + * SDRAM Controller (SDR) MFD
>>>> + *
>>>> + * Copyright (C) 2014 Altera Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> Can you use the shorter version of the licence?
>> Hi. This seems to be the shorter version of the license agreement
>> and is fairly common in the kernel, right?
> This is the long(ish) version. Many programs have the Copyright line
> and the "This program is free software;" line. It'll also be a good
> idea to keep the link to the full licence.
>
> [...]
Hi Lee.
Our legal department requires this header. Their reasoning is that they
want to retain the rights and warranty language with the file (just in
case the COPYING file changes).
>>>> + {
>>>> + .name = "altr_sdram_edac",
>>>> + .of_compatible = "altr,sdram-edac",
>>> What other devices will there be?
>>>
>> There will be an FPGA bridge and a power control driver that will
>> need access to the SDR Controller registers.
> Okay. Do you know when they'll be upstreamed?
The other drivers are waiting on this file as a pre-requisite.
>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>>>> +{
>>>> + return readl(sdr->reg_base + reg_offset);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>>>> +
>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>>>> +{
>>>> + writel(value, sdr->reg_base + reg_offset);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
>>> Why are you abstracting these?
>>>
>>> Might be better to use Regmap even.
>> regmap seems unnecessarily complex for what we're doing which is why
>> this method was chosen.
>>
>> Future drivers will access different sets of registers in the
>> device. These drivers won't share bitfields in the same register so
>> the MFD seemed like the best solution. Originally we implemented
>> this using syscon but that seems to be frowned upon so we changed to
>> using a MFD.
> Why was the use of syscon frowned upon? Can you link me to the
> thread? Writing directly to the registers sounds to me a lot worse
> than using infrastructure which was designed for these kinds of
> accesses.
>
> If you do choose to fiddle with the registers in this manner, is there
> any reason why you're calling back into here, rather than using
> readl() and writel() directly?
>
We'd prefer to use syscon and that is what we started with. If you'd
like to be our advocate, I will return to that because it was pretty
clean. My primary concern is to get it upstreamed and if it is MFD then
I'll make the changes.
Here are the threads.
http://marc.info/?l=linux-kernel&m=140128791902800&w=2
and
http://article.gmane.org/gmane.linux.kernel/1679601
>>>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
>>>> +{
>>>> + u32 size;
>>>> + u32 read_reg, row, bank, col, cs, width;
>>> Weird that size is on its own. Either place on a single line or
>>> separate them all.
>> OK. I will change this.
>>>> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
>>>> + if (read_reg < 0)
>>>> + return 0;
>>>> +
>>>> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
>>>> + if (width < 0)
>>>> + return 0;
> u32s cant be < 0. The 'u' means 'unsigned'.
Whoops. Good catch.
>>>> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
>>>> + SDR_DRAMADDRW_COLBITS_LSB;
>>>> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
>>>> + SDR_DRAMADDRW_ROWBITS_LSB;
>>>> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
>>>> + SDR_DRAMADDRW_BANKBITS_LSB;
>>>> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
>>>> + SDR_DRAMADDRW_CSBITS_LSB;
>>> These would probably be better as macros.
>>>
>> OK. I will fix this.
>>>> + /* Correct for ECC as its not addressible */
>>>> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
>>>> + width = 32;
>>>> + if (width == SDR_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;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
>>> Should this really be done in here? Isn't this an SDRAM function?
>>>
>> This register is part of the SDRAM controller and size information
>> may be required by the other drivers that share this memory
>> area/need SDRAM information.
> Then export a function from the SDRAM driver, not from here.
We don't have an SDRAM driver. Although I could put this in the EDAC
driver it would be lost to anyone else wanting this functionality so
this seemed to be the logical place.
>
> [...]
>
>>>> +static const struct platform_device_id altera_sdr_ids[] = {
>>>> + { "altera_sdr", },
>>>> + { }
>>>> +};
>>> What's this for?
>> We don't strictly need it because we are driven by the device tree.
>> It can be removed it if is a problem but I'm not clear why it is a
>> problem.
> It's a problem because it's unused, superfluous bumph. :)
OK. I will remove it.
>
> [...]
>
>>>> +static int __init altera_sdr_init(void)
>>>> +{
>>>> + return platform_driver_register(&altera_sdr_driver);
>>>> +}
>>>> +postcore_initcall(altera_sdr_init);
>>> Why was this chosen?
>> We want this to happen pretty early.
> If you _need_ this is happen early, core_initcall() is more commonly
> used, but _why_ do you need it to happen this early?
The syscon driver used this designation. After talking with Alan, this
could be changed to a core_initcall(). However, it could also be a
subsys_initcall which seems to be more common in the MFD drivers.
>>>> +/*
>>>> + * Copyright (C) 2014 Altera Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> Use the short version.
>> Same as above. This is the shorter version that we've been using at Altera.
> If you read COPYING, you only require the Copyright line and a link to
> the full licence. In this case I'll be happy if you left in the "This
> program is free software;" line as well, but remove the paragraph
> below it.
>
> [...]
>
>>>> +/* Register access API */
>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
>>> Regmap?
>> Same as above. This seems to be more complex than we need.
> I'm not sure I believe that. Regmap is as complex or as simple as you
> need.
OK. This is a moot point if we decide to go back to using syscon.
If we stick to using an MFD driver, then we can look at regmap. If we
stick with MFD, the code really starts to look like syscon.
Thanks!
>> Thanks for your comments and for reviewing!
>>
>> Thor
> You're welcome. OOI, do you own a hammer? ;)
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-01 22:27 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-01 22:27 UTC (permalink / raw)
To: linux-arm-kernel
On 08/01/2014 03:13 AM, Lee Jones wrote:
> On Thu, 31 Jul 2014, Thor Thayer wrote:
>> On 07/31/2014 03:26 AM, Lee Jones wrote:
>>> On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
>>>
>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>
>>>> Add a simple MFD for the Altera SDRAM Controller.
>>>>
>>>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>>>> ---
>>>> v1-8: The MFD implementation was not included in the original series.
>>>>
>>>> v9: New MFD implementation.
>>>> ---
>>>> MAINTAINERS | 5 ++
>>>> drivers/mfd/Kconfig | 7 ++
>>>> drivers/mfd/Makefile | 1 +
>>>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
>>>> 5 files changed, 277 insertions(+)
>>>> create mode 100644 drivers/mfd/altera-sdr.c
>>>> create mode 100644 include/linux/mfd/altera-sdr.h
> [...]
>
>>>> +++ b/drivers/mfd/altera-sdr.c
>>>> @@ -0,0 +1,162 @@
>>>> +/*
>>>> + * SDRAM Controller (SDR) MFD
>>>> + *
>>>> + * Copyright (C) 2014 Altera Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> Can you use the shorter version of the licence?
>> Hi. This seems to be the shorter version of the license agreement
>> and is fairly common in the kernel, right?
> This is the long(ish) version. Many programs have the Copyright line
> and the "This program is free software;" line. It'll also be a good
> idea to keep the link to the full licence.
>
> [...]
Hi Lee.
Our legal department requires this header. Their reasoning is that they
want to retain the rights and warranty language with the file (just in
case the COPYING file changes).
>>>> + {
>>>> + .name = "altr_sdram_edac",
>>>> + .of_compatible = "altr,sdram-edac",
>>> What other devices will there be?
>>>
>> There will be an FPGA bridge and a power control driver that will
>> need access to the SDR Controller registers.
> Okay. Do you know when they'll be upstreamed?
The other drivers are waiting on this file as a pre-requisite.
>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>>>> +{
>>>> + return readl(sdr->reg_base + reg_offset);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>>>> +
>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>>>> +{
>>>> + writel(value, sdr->reg_base + reg_offset);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
>>> Why are you abstracting these?
>>>
>>> Might be better to use Regmap even.
>> regmap seems unnecessarily complex for what we're doing which is why
>> this method was chosen.
>>
>> Future drivers will access different sets of registers in the
>> device. These drivers won't share bitfields in the same register so
>> the MFD seemed like the best solution. Originally we implemented
>> this using syscon but that seems to be frowned upon so we changed to
>> using a MFD.
> Why was the use of syscon frowned upon? Can you link me to the
> thread? Writing directly to the registers sounds to me a lot worse
> than using infrastructure which was designed for these kinds of
> accesses.
>
> If you do choose to fiddle with the registers in this manner, is there
> any reason why you're calling back into here, rather than using
> readl() and writel() directly?
>
We'd prefer to use syscon and that is what we started with. If you'd
like to be our advocate, I will return to that because it was pretty
clean. My primary concern is to get it upstreamed and if it is MFD then
I'll make the changes.
Here are the threads.
http://marc.info/?l=linux-kernel&m=140128791902800&w=2
and
http://article.gmane.org/gmane.linux.kernel/1679601
>>>> +u32 altera_sdr_mem_size(struct altera_sdr *sdr)
>>>> +{
>>>> + u32 size;
>>>> + u32 read_reg, row, bank, col, cs, width;
>>> Weird that size is on its own. Either place on a single line or
>>> separate them all.
>> OK. I will change this.
>>>> + read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
>>>> + if (read_reg < 0)
>>>> + return 0;
>>>> +
>>>> + width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
>>>> + if (width < 0)
>>>> + return 0;
> u32s cant be < 0. The 'u' means 'unsigned'.
Whoops. Good catch.
>>>> + col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
>>>> + SDR_DRAMADDRW_COLBITS_LSB;
>>>> + row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
>>>> + SDR_DRAMADDRW_ROWBITS_LSB;
>>>> + bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
>>>> + SDR_DRAMADDRW_BANKBITS_LSB;
>>>> + cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
>>>> + SDR_DRAMADDRW_CSBITS_LSB;
>>> These would probably be better as macros.
>>>
>> OK. I will fix this.
>>>> + /* Correct for ECC as its not addressible */
>>>> + if (width == SDR_DRAMIFWIDTH_32B_ECC)
>>>> + width = 32;
>>>> + if (width == SDR_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;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
>>> Should this really be done in here? Isn't this an SDRAM function?
>>>
>> This register is part of the SDRAM controller and size information
>> may be required by the other drivers that share this memory
>> area/need SDRAM information.
> Then export a function from the SDRAM driver, not from here.
We don't have an SDRAM driver. Although I could put this in the EDAC
driver it would be lost to anyone else wanting this functionality so
this seemed to be the logical place.
>
> [...]
>
>>>> +static const struct platform_device_id altera_sdr_ids[] = {
>>>> + { "altera_sdr", },
>>>> + { }
>>>> +};
>>> What's this for?
>> We don't strictly need it because we are driven by the device tree.
>> It can be removed it if is a problem but I'm not clear why it is a
>> problem.
> It's a problem because it's unused, superfluous bumph. :)
OK. I will remove it.
>
> [...]
>
>>>> +static int __init altera_sdr_init(void)
>>>> +{
>>>> + return platform_driver_register(&altera_sdr_driver);
>>>> +}
>>>> +postcore_initcall(altera_sdr_init);
>>> Why was this chosen?
>> We want this to happen pretty early.
> If you _need_ this is happen early, core_initcall() is more commonly
> used, but _why_ do you need it to happen this early?
The syscon driver used this designation. After talking with Alan, this
could be changed to a core_initcall(). However, it could also be a
subsys_initcall which seems to be more common in the MFD drivers.
>>>> +/*
>>>> + * Copyright (C) 2014 Altera Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>>> Use the short version.
>> Same as above. This is the shorter version that we've been using at Altera.
> If you read COPYING, you only require the Copyright line and a link to
> the full licence. In this case I'll be happy if you left in the "This
> program is free software;" line as well, but remove the paragraph
> below it.
>
> [...]
>
>>>> +/* Register access API */
>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
>>> Regmap?
>> Same as above. This seems to be more complex than we need.
> I'm not sure I believe that. Regmap is as complex or as simple as you
> need.
OK. This is a moot point if we decide to go back to using syscon.
If we stick to using an MFD driver, then we can look at regmap. If we
stick with MFD, the code really starts to look like syscon.
Thanks!
>> Thanks for your comments and for reviewing!
>>
>> Thor
> You're welcome. OOI, do you own a hammer? ;)
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-08-01 22:27 ` Thor Thayer
@ 2014-08-02 17:08 ` Steffen Trumtrar
-1 siblings, 0 replies; 34+ messages in thread
From: Steffen Trumtrar @ 2014-08-02 17:08 UTC (permalink / raw)
To: Thor Thayer
Cc: Lee Jones, robherring2, pawel.moll, mark.rutland, ijc+devicetree,
galak, rob, linux, atull, delicious.quinoa, dinguyen,
dougthompson, grant.likely, bp, sameo, devicetree, linux-doc,
linux-edac, linux-kernel, linux-arm-kernel, tthayer.linux,
Alan Tull
Hi!
On Fri, Aug 01, 2014 at 05:27:57PM -0500, Thor Thayer wrote:
>
> On 08/01/2014 03:13 AM, Lee Jones wrote:
> >On Thu, 31 Jul 2014, Thor Thayer wrote:
> >>On 07/31/2014 03:26 AM, Lee Jones wrote:
> >>>On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
[...]
> >>>>+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
> >>>>+{
> >>>>+ return readl(sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>>>+
> >>>>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>>>+{
> >>>>+ writel(value, sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> We'd prefer to use syscon and that is what we started with. If you'd
> like to be our advocate, I will return to that because it was pretty
> clean. My primary concern is to get it upstreamed and if it is MFD
> then I'll make the changes.
>
> Here are the threads.
> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
The conclusion of this thread was syscon for offset 0x0, no ?!
And if you decide to have new writel/readl functions, I'd prefer if you don't
change the order of parameters just because. That always weirds me out, when
there are vendorname_writel functions, that only change the API of writel and
nothing else (not exactly the case here).
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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-02 17:08 ` Steffen Trumtrar
0 siblings, 0 replies; 34+ messages in thread
From: Steffen Trumtrar @ 2014-08-02 17:08 UTC (permalink / raw)
To: linux-arm-kernel
Hi!
On Fri, Aug 01, 2014 at 05:27:57PM -0500, Thor Thayer wrote:
>
> On 08/01/2014 03:13 AM, Lee Jones wrote:
> >On Thu, 31 Jul 2014, Thor Thayer wrote:
> >>On 07/31/2014 03:26 AM, Lee Jones wrote:
> >>>On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
[...]
> >>>>+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
> >>>>+{
> >>>>+ return readl(sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>>>+
> >>>>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>>>+{
> >>>>+ writel(value, sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> We'd prefer to use syscon and that is what we started with. If you'd
> like to be our advocate, I will return to that because it was pretty
> clean. My primary concern is to get it upstreamed and if it is MFD
> then I'll make the changes.
>
> Here are the threads.
> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
The conclusion of this thread was syscon for offset 0x0, no ?!
And if you decide to have new writel/readl functions, I'd prefer if you don't
change the order of parameters just because. That always weirds me out, when
there are vendorname_writel functions, that only change the API of writel and
nothing else (not exactly the case here).
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 |
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-08-01 22:27 ` Thor Thayer
@ 2014-08-04 8:41 ` Lee Jones
-1 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2014-08-04 8:41 UTC (permalink / raw)
To: Thor Thayer
Cc: robherring2, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, devicetree, linux-doc, linux-edac,
linux-kernel, linux-arm-kernel, tthayer.linux, Alan Tull
On Fri, 01 Aug 2014, Thor Thayer wrote:
> On 08/01/2014 03:13 AM, Lee Jones wrote:
> >On Thu, 31 Jul 2014, Thor Thayer wrote:
> >>On 07/31/2014 03:26 AM, Lee Jones wrote:
> >>>On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
> >>>
> >>>>From: Thor Thayer <tthayer@opensource.altera.com>
> >>>>
> >>>>Add a simple MFD for the Altera SDRAM Controller.
> >>>>
> >>>>Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >>>>Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> >>>>---
> >>>>v1-8: The MFD implementation was not included in the original series.
> >>>>
> >>>>v9: New MFD implementation.
> >>>>---
> >>>> MAINTAINERS | 5 ++
> >>>> drivers/mfd/Kconfig | 7 ++
> >>>> drivers/mfd/Makefile | 1 +
> >>>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> >>>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> >>>> 5 files changed, 277 insertions(+)
> >>>> create mode 100644 drivers/mfd/altera-sdr.c
> >>>> create mode 100644 include/linux/mfd/altera-sdr.h
[...]
> >>>>+ return readl(sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>>>+
> >>>>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>>>+{
> >>>>+ writel(value, sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> >>>Why are you abstracting these?
You still didn't answer this?
> >>>Might be better to use Regmap even.
> >>regmap seems unnecessarily complex for what we're doing which is why
> >>this method was chosen.
> >>
> >>Future drivers will access different sets of registers in the
> >>device. These drivers won't share bitfields in the same register so
> >>the MFD seemed like the best solution. Originally we implemented
> >>this using syscon but that seems to be frowned upon so we changed to
> >>using a MFD.
> >Why was the use of syscon frowned upon? Can you link me to the
> >thread? Writing directly to the registers sounds to me a lot worse
> >than using infrastructure which was designed for these kinds of
> >accesses.
> >
> >If you do choose to fiddle with the registers in this manner, is there
> >any reason why you're calling back into here, rather than using
> >readl() and writel() directly?
> >
> We'd prefer to use syscon and that is what we started with. If you'd
> like to be our advocate, I will return to that because it was pretty
> clean. My primary concern is to get it upstreamed and if it is MFD
> then I'll make the changes.
>
> Here are the threads.
> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
> and
> http://article.gmane.org/gmane.linux.kernel/1679601
Syscon looks the most appropriate to me.
[...]
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> >>>Should this really be done in here? Isn't this an SDRAM function?
> >>>
> >>This register is part of the SDRAM controller and size information
> >>may be required by the other drivers that share this memory
> >>area/need SDRAM information.
> >Then export a function from the SDRAM driver, not from here.
> We don't have an SDRAM driver. Although I could put this in the
> EDAC driver it would be lost to anyone else wanting this
> functionality so this seemed to be the logical place.
Why can't you export it from the EDAC driver?
[...]
> >>>>+static int __init altera_sdr_init(void)
> >>>>+{
> >>>>+ return platform_driver_register(&altera_sdr_driver);
> >>>>+}
> >>>>+postcore_initcall(altera_sdr_init);
> >>>Why was this chosen?
> >>We want this to happen pretty early.
> >If you _need_ this is happen early, core_initcall() is more commonly
> >used, but _why_ do you need it to happen this early?
> The syscon driver used this designation. After talking with Alan,
> this could be changed to a core_initcall(). However, it could also
> be a subsys_initcall which seems to be more common in the MFD
> drivers.
That doesn't answer my question still.
What is the reason, requirement, need for this driver to be probed so
early during the boot process?
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-04 8:41 ` Lee Jones
0 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2014-08-04 8:41 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 01 Aug 2014, Thor Thayer wrote:
> On 08/01/2014 03:13 AM, Lee Jones wrote:
> >On Thu, 31 Jul 2014, Thor Thayer wrote:
> >>On 07/31/2014 03:26 AM, Lee Jones wrote:
> >>>On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
> >>>
> >>>>From: Thor Thayer <tthayer@opensource.altera.com>
> >>>>
> >>>>Add a simple MFD for the Altera SDRAM Controller.
> >>>>
> >>>>Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >>>>Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> >>>>---
> >>>>v1-8: The MFD implementation was not included in the original series.
> >>>>
> >>>>v9: New MFD implementation.
> >>>>---
> >>>> MAINTAINERS | 5 ++
> >>>> drivers/mfd/Kconfig | 7 ++
> >>>> drivers/mfd/Makefile | 1 +
> >>>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> >>>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> >>>> 5 files changed, 277 insertions(+)
> >>>> create mode 100644 drivers/mfd/altera-sdr.c
> >>>> create mode 100644 include/linux/mfd/altera-sdr.h
[...]
> >>>>+ return readl(sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>>>+
> >>>>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>>>+{
> >>>>+ writel(value, sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> >>>Why are you abstracting these?
You still didn't answer this?
> >>>Might be better to use Regmap even.
> >>regmap seems unnecessarily complex for what we're doing which is why
> >>this method was chosen.
> >>
> >>Future drivers will access different sets of registers in the
> >>device. These drivers won't share bitfields in the same register so
> >>the MFD seemed like the best solution. Originally we implemented
> >>this using syscon but that seems to be frowned upon so we changed to
> >>using a MFD.
> >Why was the use of syscon frowned upon? Can you link me to the
> >thread? Writing directly to the registers sounds to me a lot worse
> >than using infrastructure which was designed for these kinds of
> >accesses.
> >
> >If you do choose to fiddle with the registers in this manner, is there
> >any reason why you're calling back into here, rather than using
> >readl() and writel() directly?
> >
> We'd prefer to use syscon and that is what we started with. If you'd
> like to be our advocate, I will return to that because it was pretty
> clean. My primary concern is to get it upstreamed and if it is MFD
> then I'll make the changes.
>
> Here are the threads.
> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
> and
> http://article.gmane.org/gmane.linux.kernel/1679601
Syscon looks the most appropriate to me.
[...]
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> >>>Should this really be done in here? Isn't this an SDRAM function?
> >>>
> >>This register is part of the SDRAM controller and size information
> >>may be required by the other drivers that share this memory
> >>area/need SDRAM information.
> >Then export a function from the SDRAM driver, not from here.
> We don't have an SDRAM driver. Although I could put this in the
> EDAC driver it would be lost to anyone else wanting this
> functionality so this seemed to be the logical place.
Why can't you export it from the EDAC driver?
[...]
> >>>>+static int __init altera_sdr_init(void)
> >>>>+{
> >>>>+ return platform_driver_register(&altera_sdr_driver);
> >>>>+}
> >>>>+postcore_initcall(altera_sdr_init);
> >>>Why was this chosen?
> >>We want this to happen pretty early.
> >If you _need_ this is happen early, core_initcall() is more commonly
> >used, but _why_ do you need it to happen this early?
> The syscon driver used this designation. After talking with Alan,
> this could be changed to a core_initcall(). However, it could also
> be a subsys_initcall which seems to be more common in the MFD
> drivers.
That doesn't answer my question still.
What is the reason, requirement, need for this driver to be probed so
early during the boot process?
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
2014-08-02 17:08 ` Steffen Trumtrar
(?)
@ 2014-08-04 16:09 ` Thor Thayer
-1 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-04 16:09 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: Lee Jones, robherring2, pawel.moll, mark.rutland, ijc+devicetree,
galak, rob, linux, atull, delicious.quinoa, dinguyen,
dougthompson, grant.likely, bp, sameo, devicetree, linux-doc,
linux-edac, linux-kernel, linux-arm-kernel, tthayer.linux,
Alan Tull
On 08/02/2014 12:08 PM, Steffen Trumtrar wrote:
> Hi!
>
> On Fri, Aug 01, 2014 at 05:27:57PM -0500, Thor Thayer wrote:
>> On 08/01/2014 03:13 AM, Lee Jones wrote:
>>> On Thu, 31 Jul 2014, Thor Thayer wrote:
>>>> On 07/31/2014 03:26 AM, Lee Jones wrote:
>>>>> On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
> [...]
>
>>>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>>>>>> +{
>>>>>> + return readl(sdr->reg_base + reg_offset);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>>>>>> +
>>>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>>>>>> +{
>>>>>> + writel(value, sdr->reg_base + reg_offset);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
>> We'd prefer to use syscon and that is what we started with. If you'd
>> like to be our advocate, I will return to that because it was pretty
>> clean. My primary concern is to get it upstreamed and if it is MFD
>> then I'll make the changes.
>>
>> Here are the threads.
>> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
> The conclusion of this thread was syscon for offset 0x0, no ?!
> And if you decide to have new writel/readl functions, I'd prefer if you don't
> change the order of parameters just because. That always weirds me out, when
> there are vendorname_writel functions, that only change the API of writel and
> nothing else (not exactly the case here).
Hi Steffen,
Yes, I see your point on the order of parameters. i will change it.
As for the syscon only at offset 0, I submitted that in version 7 & 8.
It wasn't as clean as the version using syscon for the entire range. It
could also require changes to the SDRAM controller binding as things are
added in the future. My understanding is the bindings should not change
significantly and changes are frowned upon.
After going through this exercise in a couple of different ways, I'm
leaning toward using syscon for the entire SDRAM controller register range.
Is there a good reason not to use syscon on the entire register range?
Thanks,
Thor
>
> Regards,
> Steffen
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-04 16:09 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-04 16:09 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: Lee Jones, robherring2, pawel.moll, mark.rutland, ijc+devicetree,
galak, rob, linux, atull, delicious.quinoa, dinguyen,
dougthompson, grant.likely, bp, sameo, devicetree, linux-doc,
linux-edac, linux-kernel, linux-arm-kernel, tthayer.linux,
Alan Tull
On 08/02/2014 12:08 PM, Steffen Trumtrar wrote:
> Hi!
>
> On Fri, Aug 01, 2014 at 05:27:57PM -0500, Thor Thayer wrote:
>> On 08/01/2014 03:13 AM, Lee Jones wrote:
>>> On Thu, 31 Jul 2014, Thor Thayer wrote:
>>>> On 07/31/2014 03:26 AM, Lee Jones wrote:
>>>>> On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:
> [...]
>
>>>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>>>>>> +{
>>>>>> + return readl(sdr->reg_base + reg_offset);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>>>>>> +
>>>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>>>>>> +{
>>>>>> + writel(value, sdr->reg_base + reg_offset);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
>> We'd prefer to use syscon and that is what we started with. If you'd
>> like to be our advocate, I will return to that because it was pretty
>> clean. My primary concern is to get it upstreamed and if it is MFD
>> then I'll make the changes.
>>
>> Here are the threads.
>> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
> The conclusion of this thread was syscon for offset 0x0, no ?!
> And if you decide to have new writel/readl functions, I'd prefer if you don't
> change the order of parameters just because. That always weirds me out, when
> there are vendorname_writel functions, that only change the API of writel and
> nothing else (not exactly the case here).
Hi Steffen,
Yes, I see your point on the order of parameters. i will change it.
As for the syscon only at offset 0, I submitted that in version 7 & 8.
It wasn't as clean as the version using syscon for the entire range. It
could also require changes to the SDRAM controller binding as things are
added in the future. My understanding is the bindings should not change
significantly and changes are frowned upon.
After going through this exercise in a couple of different ways, I'm
leaning toward using syscon for the entire SDRAM controller register range.
Is there a good reason not to use syscon on the entire register range?
Thanks,
Thor
>
> Regards,
> Steffen
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
@ 2014-08-04 16:09 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-04 16:09 UTC (permalink / raw)
To: linux-arm-kernel
On 08/02/2014 12:08 PM, Steffen Trumtrar wrote:
> Hi!
>
> On Fri, Aug 01, 2014 at 05:27:57PM -0500, Thor Thayer wrote:
>> On 08/01/2014 03:13 AM, Lee Jones wrote:
>>> On Thu, 31 Jul 2014, Thor Thayer wrote:
>>>> On 07/31/2014 03:26 AM, Lee Jones wrote:
>>>>> On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:
> [...]
>
>>>>>> +u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
>>>>>> +{
>>>>>> + return readl(sdr->reg_base + reg_offset);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(altera_sdr_readl);
>>>>>> +
>>>>>> +void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
>>>>>> +{
>>>>>> + writel(value, sdr->reg_base + reg_offset);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(altera_sdr_writel);
>> We'd prefer to use syscon and that is what we started with. If you'd
>> like to be our advocate, I will return to that because it was pretty
>> clean. My primary concern is to get it upstreamed and if it is MFD
>> then I'll make the changes.
>>
>> Here are the threads.
>> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
> The conclusion of this thread was syscon for offset 0x0, no ?!
> And if you decide to have new writel/readl functions, I'd prefer if you don't
> change the order of parameters just because. That always weirds me out, when
> there are vendorname_writel functions, that only change the API of writel and
> nothing else (not exactly the case here).
Hi Steffen,
Yes, I see your point on the order of parameters. i will change it.
As for the syscon only at offset 0, I submitted that in version 7 & 8.
It wasn't as clean as the version using syscon for the entire range. It
could also require changes to the SDRAM controller binding as things are
added in the future. My understanding is the bindings should not change
significantly and changes are frowned upon.
After going through this exercise in a couple of different ways, I'm
leaning toward using syscon for the entire SDRAM controller register range.
Is there a good reason not to use syscon on the entire register range?
Thanks,
Thor
>
> Regards,
> Steffen
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
2014-07-30 18:22 ` tthayer
@ 2014-08-18 0:50 ` Rob Herring
-1 siblings, 0 replies; 34+ messages in thread
From: Rob Herring @ 2014-08-18 0:50 UTC (permalink / raw)
To: tthayer, pawel.moll, mark.rutland, ijc+devicetree, galak, rob,
linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux
On 07/30/2014 01:22 PM, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> 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.
>
> v6: Only map the ctrlcfg register as syscon.
>
> v7: No change. Bump for consistency.
>
> v8: No change. Bump for consistency.
>
> v9: Changes to support a MFD SDRAM controller with nested EDAC.
> ---
> .../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
> 2 files changed, 23 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
> new file mode 100644
> index 0000000..2bb1ddf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
> @@ -0,0 +1,13 @@
> +Altera SOCFPGA SDRAM Controller
> +The SDRAM controller is implemented as a MFD so various drivers may
> +nest under this main SDRAM controller binding.
> +
> +Required properties:
> +- compatible : "altr,sdr";
> +- reg : Should contain 1 register range(address and length)
> +
> +Example:
> + sdr@0xffc25000 {
> + compatible = "altr,sdr";
> + reg = <0xffc25000 0x1000>;
> + };
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 4676f25..ecb306d 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -603,6 +603,16 @@
> };
> };
>
> + sdr@0xffc25000 {
> + compatible = "altr,sdr";
> + reg = <0xffc25000 0x1000>;
> +
> + sdramedac@0 {
> + compatible = "altr,sdram-edac";
> + interrupts = <0 39 4>;
> + };
This doesn't match the documentation, but I don't think this is a move
in the right direction anyway. Because Linux has/wants an MFD driver is
not a reason to add a sub node. It is a single h/w block and DT should
reflect that.
Rob
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
@ 2014-08-18 0:50 ` Rob Herring
0 siblings, 0 replies; 34+ messages in thread
From: Rob Herring @ 2014-08-18 0:50 UTC (permalink / raw)
To: linux-arm-kernel
On 07/30/2014 01:22 PM, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> 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.
>
> v6: Only map the ctrlcfg register as syscon.
>
> v7: No change. Bump for consistency.
>
> v8: No change. Bump for consistency.
>
> v9: Changes to support a MFD SDRAM controller with nested EDAC.
> ---
> .../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
> 2 files changed, 23 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
> new file mode 100644
> index 0000000..2bb1ddf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
> @@ -0,0 +1,13 @@
> +Altera SOCFPGA SDRAM Controller
> +The SDRAM controller is implemented as a MFD so various drivers may
> +nest under this main SDRAM controller binding.
> +
> +Required properties:
> +- compatible : "altr,sdr";
> +- reg : Should contain 1 register range(address and length)
> +
> +Example:
> + sdr at 0xffc25000 {
> + compatible = "altr,sdr";
> + reg = <0xffc25000 0x1000>;
> + };
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 4676f25..ecb306d 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -603,6 +603,16 @@
> };
> };
>
> + sdr at 0xffc25000 {
> + compatible = "altr,sdr";
> + reg = <0xffc25000 0x1000>;
> +
> + sdramedac at 0 {
> + compatible = "altr,sdram-edac";
> + interrupts = <0 39 4>;
> + };
This doesn't match the documentation, but I don't think this is a move
in the right direction anyway. Because Linux has/wants an MFD driver is
not a reason to add a sub node. It is a single h/w block and DT should
reflect that.
Rob
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
2014-08-18 0:50 ` Rob Herring
(?)
@ 2014-08-18 14:44 ` Thor Thayer
-1 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-18 14:44 UTC (permalink / raw)
To: Rob Herring, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux
On 08/17/2014 07:50 PM, Rob Herring wrote:
> On 07/30/2014 01:22 PM, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> 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.
>>
>> v6: Only map the ctrlcfg register as syscon.
>>
>> v7: No change. Bump for consistency.
>>
>> v8: No change. Bump for consistency.
>>
>> v9: Changes to support a MFD SDRAM controller with nested EDAC.
>> ---
>> .../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
>> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
>> 2 files changed, 23 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>> new file mode 100644
>> index 0000000..2bb1ddf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>> @@ -0,0 +1,13 @@
>> +Altera SOCFPGA SDRAM Controller
>> +The SDRAM controller is implemented as a MFD so various drivers may
>> +nest under this main SDRAM controller binding.
>> +
>> +Required properties:
>> +- compatible : "altr,sdr";
>> +- reg : Should contain 1 register range(address and length)
>> +
>> +Example:
>> + sdr@0xffc25000 {
>> + compatible = "altr,sdr";
>> + reg = <0xffc25000 0x1000>;
>> + };
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 4676f25..ecb306d 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -603,6 +603,16 @@
>> };
>> };
>>
>> + sdr@0xffc25000 {
>> + compatible = "altr,sdr";
>> + reg = <0xffc25000 0x1000>;
>> +
>> + sdramedac@0 {
>> + compatible = "altr,sdram-edac";
>> + interrupts = <0 39 4>;
>> + };
> This doesn't match the documentation, but I don't think this is a move
> in the right direction anyway. Because Linux has/wants an MFD driver is
> not a reason to add a sub node. It is a single h/w block and DT should
> reflect that.
>
> Rob
Hi Rob,
Thanks for reviewing. After discussions with the community and
internally, I reverted to using the syscon case in revision 10. I
apologize for the confusion but the syscon method seems to be a cleaner
solution. I submitted the sycon version on 8/11/14.
Thanks,
Thor
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
@ 2014-08-18 14:44 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-18 14:44 UTC (permalink / raw)
To: Rob Herring, pawel.moll, mark.rutland, ijc+devicetree, galak,
rob, linux, atull, delicious.quinoa, dinguyen, dougthompson,
grant.likely, bp, sameo, lee.jones
Cc: devicetree, linux-doc, linux-edac, linux-kernel,
linux-arm-kernel, tthayer.linux
On 08/17/2014 07:50 PM, Rob Herring wrote:
> On 07/30/2014 01:22 PM, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> 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.
>>
>> v6: Only map the ctrlcfg register as syscon.
>>
>> v7: No change. Bump for consistency.
>>
>> v8: No change. Bump for consistency.
>>
>> v9: Changes to support a MFD SDRAM controller with nested EDAC.
>> ---
>> .../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
>> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
>> 2 files changed, 23 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>> new file mode 100644
>> index 0000000..2bb1ddf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>> @@ -0,0 +1,13 @@
>> +Altera SOCFPGA SDRAM Controller
>> +The SDRAM controller is implemented as a MFD so various drivers may
>> +nest under this main SDRAM controller binding.
>> +
>> +Required properties:
>> +- compatible : "altr,sdr";
>> +- reg : Should contain 1 register range(address and length)
>> +
>> +Example:
>> + sdr@0xffc25000 {
>> + compatible = "altr,sdr";
>> + reg = <0xffc25000 0x1000>;
>> + };
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 4676f25..ecb306d 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -603,6 +603,16 @@
>> };
>> };
>>
>> + sdr@0xffc25000 {
>> + compatible = "altr,sdr";
>> + reg = <0xffc25000 0x1000>;
>> +
>> + sdramedac@0 {
>> + compatible = "altr,sdram-edac";
>> + interrupts = <0 39 4>;
>> + };
> This doesn't match the documentation, but I don't think this is a move
> in the right direction anyway. Because Linux has/wants an MFD driver is
> not a reason to add a sub node. It is a single h/w block and DT should
> reflect that.
>
> Rob
Hi Rob,
Thanks for reviewing. After discussions with the community and
internally, I reverted to using the syscon case in revision 10. I
apologize for the confusion but the syscon method seems to be a cleaner
solution. I submitted the sycon version on 8/11/14.
Thanks,
Thor
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings
@ 2014-08-18 14:44 ` Thor Thayer
0 siblings, 0 replies; 34+ messages in thread
From: Thor Thayer @ 2014-08-18 14:44 UTC (permalink / raw)
To: linux-arm-kernel
On 08/17/2014 07:50 PM, Rob Herring wrote:
> On 07/30/2014 01:22 PM, tthayer at opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Add the Altera SDRAM controller bindings and device tree changes to the Altera SoC project.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> 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.
>>
>> v6: Only map the ctrlcfg register as syscon.
>>
>> v7: No change. Bump for consistency.
>>
>> v8: No change. Bump for consistency.
>>
>> v9: Changes to support a MFD SDRAM controller with nested EDAC.
>> ---
>> .../devicetree/bindings/arm/altera/socfpga-sdr.txt | 13 +++++++++++++
>> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
>> 2 files changed, 23 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>> new file mode 100644
>> index 0000000..2bb1ddf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
>> @@ -0,0 +1,13 @@
>> +Altera SOCFPGA SDRAM Controller
>> +The SDRAM controller is implemented as a MFD so various drivers may
>> +nest under this main SDRAM controller binding.
>> +
>> +Required properties:
>> +- compatible : "altr,sdr";
>> +- reg : Should contain 1 register range(address and length)
>> +
>> +Example:
>> + sdr at 0xffc25000 {
>> + compatible = "altr,sdr";
>> + reg = <0xffc25000 0x1000>;
>> + };
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 4676f25..ecb306d 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -603,6 +603,16 @@
>> };
>> };
>>
>> + sdr at 0xffc25000 {
>> + compatible = "altr,sdr";
>> + reg = <0xffc25000 0x1000>;
>> +
>> + sdramedac at 0 {
>> + compatible = "altr,sdram-edac";
>> + interrupts = <0 39 4>;
>> + };
> This doesn't match the documentation, but I don't think this is a move
> in the right direction anyway. Because Linux has/wants an MFD driver is
> not a reason to add a sub node. It is a single h/w block and DT should
> reflect that.
>
> Rob
Hi Rob,
Thanks for reviewing. After discussions with the community and
internally, I reverted to using the syscon case in revision 10. I
apologize for the confusion but the syscon method seems to be a cleaner
solution. I submitted the sycon version on 8/11/14.
Thanks,
Thor
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2014-08-18 14:44 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-30 18:22 [PATCHv9 0/3] Addition of Altera EDAC support tthayer
2014-07-30 18:22 ` tthayer at opensource.altera.com
2014-07-30 18:22 ` tthayer
2014-07-30 18:22 ` [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller tthayer
2014-07-30 18:22 ` tthayer at opensource.altera.com
2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
2014-07-31 8:26 ` Lee Jones
2014-07-31 8:26 ` Lee Jones
2014-07-31 20:00 ` Thor Thayer
2014-07-31 20:00 ` Thor Thayer
2014-07-31 20:00 ` Thor Thayer
2014-08-01 8:13 ` Lee Jones
2014-08-01 8:13 ` Lee Jones
2014-08-01 22:27 ` Thor Thayer
2014-08-01 22:27 ` Thor Thayer
2014-08-01 22:27 ` Thor Thayer
2014-08-02 17:08 ` Steffen Trumtrar
2014-08-02 17:08 ` Steffen Trumtrar
2014-08-04 16:09 ` Thor Thayer
2014-08-04 16:09 ` Thor Thayer
2014-08-04 16:09 ` Thor Thayer
2014-08-04 8:41 ` Lee Jones
2014-08-04 8:41 ` Lee Jones
2014-07-30 18:22 ` [PATCHv9 2/3] edac: altera: Add Altera EDAC support tthayer
2014-07-30 18:22 ` tthayer at opensource.altera.com
2014-07-30 18:22 ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
2014-07-30 18:22 ` [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings tthayer
2014-07-30 18:22 ` tthayer at opensource.altera.com
2014-07-30 18:22 ` tthayer
2014-08-18 0:50 ` Rob Herring
2014-08-18 0:50 ` Rob Herring
2014-08-18 14:44 ` Thor Thayer
2014-08-18 14:44 ` Thor Thayer
2014-08-18 14:44 ` Thor Thayer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.