All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06  4:02 ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg
  Cc: linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y, Loc Ho

This patch adds support for the APM X-Gene SoC memory controller EDAC driver
for DT.

v8:
* Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
* Add WARN_ONCE in stub function atomic_scrub
* Update DTS binding documentation (with only memory controller node)
* Temporary remove L1/L2, L3, and SoC driver code and update memory driver
  code accordingly

v7:
* Update binding documentation for memory controller, PMD, L3, and SoC EDAC
* Change resource for PCP, CSW, MCBA, MCBB, and efuse to syscon type nodes
  and updae driver accordingly
* Fix clearing L2 RTO register properly
* Fix the MODULE_DEVICE_TABLE for OF
* Update DT accordingly to binding documentation

v6:
* Rebase to 4.0.0-rc3
* Add memory scrub stub function and enable ARM64 EDAC support patch
* Add bit definition defines for L2RTOS registers
* Remove un-necessary clearing of all L1/L2 software generated registers
* Remove wrong notification of LSU un-correctable error
* Change L2 reporting of un-correcable error to correctable error
* Change clearing of the L2C L2RTO registers
* Add support for L2 HW version 1 and version 2 or above
* Add support for L3 HW version 1 and version 2 or above

v5:
* Rebase to 3.17.rc1 (next)
* Update binding documentation for additional SoC node binding resource
* Enable MCU correctable and uncorrectable interrupts if not enabled by
  firmware
* Enable top level interrupt only after all MCU registered. Otherwise,
  error interrupt will never get cleared by the corresponding MCU.
* Remove clearing of L1 and L2 errors during initialization time. Otherwise,
  they will not be captured between firmware booting and error configuration.
* Add capture and clearing SoC register bus errors
* Add register bus resource to SoC DT node

v4:
* Fix PMD l1/l2 error reading address due to wrong variable type
* Fix clearing of software generated and HW errors for l1/l2

v3:
* Update binding documentation for PMD DT node and exampples
* Add binding documentation for SoC DT node
* Change MC, PMD, and L3C driver error injection to use debugfs
* Add missing IRQ for MC correctable error (code and DT)
* Use true/false where appropriate instead 1/0
* Add bit definition for L1 MMUESR register and fully decode this error
* Remove the un-necessary dev variable from xgene_edac_pmd_ctx structure
* Add check for disabled PMD (code and DT)
* Switch to edac_printk instead pr_err
* Some minor comments update

v2:
* Add EDAC entry in MAINTAINERS for APM EDAC driver
* Remove the MC scrub patch
* Remove the word 'Caches' from Kconfig
* Change all MASK defines to use BIT(x)
* Update comment or remove them
* Wrap error injection code around CONFIG_EDAC_DEBUG
* Change function name xgene_edac_mc_hw_init to xgene_edac_mc_irq_ctl
* Change all function XXX_hw_init to XXX_hw_ctl
* Fix typo 'activie'
* Move calling function edac_mc_alloc after resource retrieval
* Check for NULL on platform_get_resource return if reference directly
* Add documentation for struct xgene_edac_pmd_ctx
* Move L1 and L2 check out of function xgene_edac_pmd_check to its own
  functions
* Use for loop for configure each CPU of an PMD
* Replace /2 by >> 1
* Remove unnecessary comment on edac_device_add_device failure
* Make mem_err_ip static const
* Unwind EDAC register correctly if failed
---
Loc Ho (5):
  arm64: Enable EDAC on ARM64
  MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver
  Documentation: Add documentation for the APM X-Gene SoC EDAC DTS
    binding
  edac: Add APM X-Gene SoC EDAC driver
  arm64: Add APM X-Gene SoC EDAC DTS entries

 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   50 ++
 MAINTAINERS                                        |    8 +
 arch/arm64/Kconfig                                 |    1 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   64 +++
 arch/arm64/include/asm/edac.h                      |   28 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/xgene_edac_mc.c                       |  552 ++++++++++++++++++++
 8 files changed, 711 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
 create mode 100644 arch/arm64/include/asm/edac.h
 create mode 100644 drivers/edac/xgene_edac_mc.c

--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06  4:02 ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds support for the APM X-Gene SoC memory controller EDAC driver
for DT.

v8:
* Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
* Add WARN_ONCE in stub function atomic_scrub
* Update DTS binding documentation (with only memory controller node)
* Temporary remove L1/L2, L3, and SoC driver code and update memory driver
  code accordingly

v7:
* Update binding documentation for memory controller, PMD, L3, and SoC EDAC
* Change resource for PCP, CSW, MCBA, MCBB, and efuse to syscon type nodes
  and updae driver accordingly
* Fix clearing L2 RTO register properly
* Fix the MODULE_DEVICE_TABLE for OF
* Update DT accordingly to binding documentation

v6:
* Rebase to 4.0.0-rc3
* Add memory scrub stub function and enable ARM64 EDAC support patch
* Add bit definition defines for L2RTOS registers
* Remove un-necessary clearing of all L1/L2 software generated registers
* Remove wrong notification of LSU un-correctable error
* Change L2 reporting of un-correcable error to correctable error
* Change clearing of the L2C L2RTO registers
* Add support for L2 HW version 1 and version 2 or above
* Add support for L3 HW version 1 and version 2 or above

v5:
* Rebase to 3.17.rc1 (next)
* Update binding documentation for additional SoC node binding resource
* Enable MCU correctable and uncorrectable interrupts if not enabled by
  firmware
* Enable top level interrupt only after all MCU registered. Otherwise,
  error interrupt will never get cleared by the corresponding MCU.
* Remove clearing of L1 and L2 errors during initialization time. Otherwise,
  they will not be captured between firmware booting and error configuration.
* Add capture and clearing SoC register bus errors
* Add register bus resource to SoC DT node

v4:
* Fix PMD l1/l2 error reading address due to wrong variable type
* Fix clearing of software generated and HW errors for l1/l2

v3:
* Update binding documentation for PMD DT node and exampples
* Add binding documentation for SoC DT node
* Change MC, PMD, and L3C driver error injection to use debugfs
* Add missing IRQ for MC correctable error (code and DT)
* Use true/false where appropriate instead 1/0
* Add bit definition for L1 MMUESR register and fully decode this error
* Remove the un-necessary dev variable from xgene_edac_pmd_ctx structure
* Add check for disabled PMD (code and DT)
* Switch to edac_printk instead pr_err
* Some minor comments update

v2:
* Add EDAC entry in MAINTAINERS for APM EDAC driver
* Remove the MC scrub patch
* Remove the word 'Caches' from Kconfig
* Change all MASK defines to use BIT(x)
* Update comment or remove them
* Wrap error injection code around CONFIG_EDAC_DEBUG
* Change function name xgene_edac_mc_hw_init to xgene_edac_mc_irq_ctl
* Change all function XXX_hw_init to XXX_hw_ctl
* Fix typo 'activie'
* Move calling function edac_mc_alloc after resource retrieval
* Check for NULL on platform_get_resource return if reference directly
* Add documentation for struct xgene_edac_pmd_ctx
* Move L1 and L2 check out of function xgene_edac_pmd_check to its own
  functions
* Use for loop for configure each CPU of an PMD
* Replace /2 by >> 1
* Remove unnecessary comment on edac_device_add_device failure
* Make mem_err_ip static const
* Unwind EDAC register correctly if failed
---
Loc Ho (5):
  arm64: Enable EDAC on ARM64
  MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver
  Documentation: Add documentation for the APM X-Gene SoC EDAC DTS
    binding
  edac: Add APM X-Gene SoC EDAC driver
  arm64: Add APM X-Gene SoC EDAC DTS entries

 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   50 ++
 MAINTAINERS                                        |    8 +
 arch/arm64/Kconfig                                 |    1 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   64 +++
 arch/arm64/include/asm/edac.h                      |   28 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/xgene_edac_mc.c                       |  552 ++++++++++++++++++++
 8 files changed, 711 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
 create mode 100644 arch/arm64/include/asm/edac.h
 create mode 100644 drivers/edac/xgene_edac_mc.c

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v8 1/5] arm64: Enable EDAC on ARM64
  2015-05-06  4:02 ` Loc Ho
@ 2015-05-06  4:02     ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg
  Cc: linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y, Loc Ho

Add an stub atomic_scrub function and enable EDAC for arm64.

Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 arch/arm64/Kconfig            |    1 +
 arch/arm64/include/asm/edac.h |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm64/include/asm/edac.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4269dba..577078f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -22,6 +22,7 @@ config ARM64
 	select BUILDTIME_EXTABLE_SORT
 	select CLONE_BACKWARDS
 	select COMMON_CLK
+	select EDAC_SUPPORT
 	select CPU_PM if (SUSPEND || CPU_IDLE)
 	select DCACHE_WORD_ACCESS
 	select GENERIC_ALLOCATOR
diff --git a/arch/arm64/include/asm/edac.h b/arch/arm64/include/asm/edac.h
new file mode 100644
index 0000000..683495b
--- /dev/null
+++ b/arch/arm64/include/asm/edac.h
@@ -0,0 +1,28 @@
+/*
+ * ARM64 EDAC Header File
+ *
+ * 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 __ASM_EDAC_H
+#define __ASM_EDAC_H
+
+/*
+ * ECC atomic, DMA, SMP and interrupt safe scrub function.
+ */
+static inline void atomic_scrub(void *va, u32 size)
+{
+	/* Stub function for now until an ARM64 HW has a way to test it. */
+	WARN_ONCE(1, "not implemented");
+}
+
+#endif
-- 
1.7.1

--
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] 33+ messages in thread

* [PATCH v8 1/5] arm64: Enable EDAC on ARM64
@ 2015-05-06  4:02     ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: linux-arm-kernel

Add an stub atomic_scrub function and enable EDAC for arm64.

Signed-off-by: Loc Ho <lho@apm.com>
---
 arch/arm64/Kconfig            |    1 +
 arch/arm64/include/asm/edac.h |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm64/include/asm/edac.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4269dba..577078f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -22,6 +22,7 @@ config ARM64
 	select BUILDTIME_EXTABLE_SORT
 	select CLONE_BACKWARDS
 	select COMMON_CLK
+	select EDAC_SUPPORT
 	select CPU_PM if (SUSPEND || CPU_IDLE)
 	select DCACHE_WORD_ACCESS
 	select GENERIC_ALLOCATOR
diff --git a/arch/arm64/include/asm/edac.h b/arch/arm64/include/asm/edac.h
new file mode 100644
index 0000000..683495b
--- /dev/null
+++ b/arch/arm64/include/asm/edac.h
@@ -0,0 +1,28 @@
+/*
+ * ARM64 EDAC Header File
+ *
+ * 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 __ASM_EDAC_H
+#define __ASM_EDAC_H
+
+/*
+ * ECC atomic, DMA, SMP and interrupt safe scrub function.
+ */
+static inline void atomic_scrub(void *va, u32 size)
+{
+	/* Stub function for now until an ARM64 HW has a way to test it. */
+	WARN_ONCE(1, "not implemented");
+}
+
+#endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v8 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver
  2015-05-06  4:02     ` Loc Ho
@ 2015-05-06  4:02         ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg
  Cc: linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y, Loc Ho

This patch adds a MAINTAINERS entry for APM X-Gene SoC EDAC driver.

Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 MAINTAINERS |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 00a586b..06d3387 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3735,6 +3735,14 @@ W:	bluesmoke.sourceforge.net
 S:	Maintained
 F:	drivers/edac/sb_edac.c

+EDAC-XGENE
+APPLIED MICRO (APM) X-GENE SOC EDAC
+M:     Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
+M:     Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
+S:     Supported
+F:     drivers/edac/xgene_edac_mc.c
+F:     Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
+
 EDIROL UA-101/UA-1000 DRIVER
 M:	Clemens Ladisch <clemens-P6GI/4k7KOmELgA04lAiVw@public.gmane.org>
 L:	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (moderated for non-subscribers)
--
1.7.1

--
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] 33+ messages in thread

* [PATCH v8 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver
@ 2015-05-06  4:02         ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds a MAINTAINERS entry for APM X-Gene SoC EDAC driver.

Signed-off-by: Loc Ho <lho@apm.com>
---
 MAINTAINERS |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 00a586b..06d3387 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3735,6 +3735,14 @@ W:	bluesmoke.sourceforge.net
 S:	Maintained
 F:	drivers/edac/sb_edac.c

+EDAC-XGENE
+APPLIED MICRO (APM) X-GENE SOC EDAC
+M:     Loc Ho <lho@apm.com>
+M:     Feng Kan <fkan@apm.com>
+S:     Supported
+F:     drivers/edac/xgene_edac_mc.c
+F:     Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
+
 EDIROL UA-101/UA-1000 DRIVER
 M:	Clemens Ladisch <clemens@ladisch.de>
 L:	alsa-devel at alsa-project.org (moderated for non-subscribers)
--
1.7.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v8 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding
  2015-05-06  4:02         ` Loc Ho
@ 2015-05-06  4:02             ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg
  Cc: linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y, Loc Ho,
	Feng Kan

This patch adds documentation for the APM X-Gene SoC EDAC DTS binding.

Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   50 ++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt

diff --git a/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
new file mode 100644
index 0000000..b27c5a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
@@ -0,0 +1,50 @@
+* APM X-Gene SoC EDAC nodes
+
+EDAC nodes are defined to describe on-chip error detection and correction.
+The follow type is supported:
+
+  memory controller	- Memory controller
+
+The following section describes the memory controller DT node binding.
+
+Required properties:
+- compatible		: Shall be "apm,xgene-edac-mc".
+- regmap-pcp		: Regmap of the CPU bus (PCP) resource.
+- regmap-csw		: Regmap of the CPU switch fabric (CSW) resource.
+- regmap-mcba		: Regmap of the MCB-A (memory bridge) resource.
+- regmap-mcbb		: Regmap of the MCB-B (memory bridge) resource.
+- reg			: First resource shall be the memory controller unit
+                          (MCU) resource.
+- interrupts            : Interrupt-specifier for MCU error IRQ(s).
+
+Example:
+	pcp: pcp@78800000 {
+		compatible = "syscon";
+		reg = <0x0 0x78800000 0x0 0x100>;
+	};
+
+	csw: csw@7e200000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e200000 0x0 0x1000>;
+	};
+
+	mcba: mcba@7e700000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e700000 0x0 0x1000>;
+	};
+
+	mcbb: mcbb@7e720000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e720000 0x0 0x1000>;
+	};
+
+	edacmc0: edacmc0@7e800000 {
+		compatible = "apm,xgene-edac-mc";
+		regmap-pcp = <&pcp>;
+		regmap-csw = <&csw>;
+		regmap-mcba = <&mcba>;
+		regmap-mcbb = <&mcbb>;
+		reg = <0x0 0x7e800000 0x0 0x1000>;
+		interrupts = <0x0 0x20 0x4>,
+			     <0x0 0x21 0x4>;
+	};
-- 
1.7.1

--
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] 33+ messages in thread

* [PATCH v8 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding
@ 2015-05-06  4:02             ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds documentation for the APM X-Gene SoC EDAC DTS binding.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Loc Ho <lho@apm.com>
---
 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   50 ++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt

diff --git a/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
new file mode 100644
index 0000000..b27c5a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
@@ -0,0 +1,50 @@
+* APM X-Gene SoC EDAC nodes
+
+EDAC nodes are defined to describe on-chip error detection and correction.
+The follow type is supported:
+
+  memory controller	- Memory controller
+
+The following section describes the memory controller DT node binding.
+
+Required properties:
+- compatible		: Shall be "apm,xgene-edac-mc".
+- regmap-pcp		: Regmap of the CPU bus (PCP) resource.
+- regmap-csw		: Regmap of the CPU switch fabric (CSW) resource.
+- regmap-mcba		: Regmap of the MCB-A (memory bridge) resource.
+- regmap-mcbb		: Regmap of the MCB-B (memory bridge) resource.
+- reg			: First resource shall be the memory controller unit
+                          (MCU) resource.
+- interrupts            : Interrupt-specifier for MCU error IRQ(s).
+
+Example:
+	pcp: pcp at 78800000 {
+		compatible = "syscon";
+		reg = <0x0 0x78800000 0x0 0x100>;
+	};
+
+	csw: csw at 7e200000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e200000 0x0 0x1000>;
+	};
+
+	mcba: mcba at 7e700000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e700000 0x0 0x1000>;
+	};
+
+	mcbb: mcbb at 7e720000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e720000 0x0 0x1000>;
+	};
+
+	edacmc0: edacmc0 at 7e800000 {
+		compatible = "apm,xgene-edac-mc";
+		regmap-pcp = <&pcp>;
+		regmap-csw = <&csw>;
+		regmap-mcba = <&mcba>;
+		regmap-mcbb = <&mcbb>;
+		reg = <0x0 0x7e800000 0x0 0x1000>;
+		interrupts = <0x0 0x20 0x4>,
+			     <0x0 0x21 0x4>;
+	};
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v8 4/5] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06  4:02             ` Loc Ho
@ 2015-05-06  4:02                 ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg
  Cc: linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y, Loc Ho,
	Feng Kan

This patch adds support for the APM X-Gene SoC memory controller EDAC driver.

Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 drivers/edac/Kconfig         |    7 +
 drivers/edac/Makefile        |    1 +
 drivers/edac/xgene_edac_mc.c |  552 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+), 0 deletions(-)
 create mode 100644 drivers/edac/xgene_edac_mc.c

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index cb59619..8ab74cc 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -392,4 +392,11 @@ config EDAC_SYNOPSYS
 	  Support for error detection and correction on the Synopsys DDR
 	  memory controller.

+config EDAC_XGENE
+	tristate "APM X-Gene SoC"
+	depends on EDAC_MM_EDAC && ARM64
+	help
+	  Support for error detection and correction on the
+	  APM X-Gene family of SOCs.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index b255f36..ff5c316 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -68,3 +68,4 @@ obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o

 obj-$(CONFIG_EDAC_ALTERA_MC)		+= altera_edac.o
 obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
+obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac_mc.o
diff --git a/drivers/edac/xgene_edac_mc.c b/drivers/edac/xgene_edac_mc.c
new file mode 100644
index 0000000..81ebf1d
--- /dev/null
+++ b/drivers/edac/xgene_edac_mc.c
@@ -0,0 +1,552 @@
+/*
+ * APM X-Gene SoC Memory Controller EDAC (error detection and correction)
+ *
+ * Copyright (c) 2015, Applied Micro Circuits Corporation
+ * Author: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
+ *         Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * 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/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include <asm/cputype.h>
+
+#include "edac_core.h"
+
+#define EDAC_MOD_STR			"xgene_edac_mc"
+
+static int edac_mc_idx;
+static int edac_mc_active_mask;
+static int edac_mc_registered_mask;
+static DEFINE_MUTEX(xgene_edac_mc_lock);
+
+/* Global error configuration status registers (CSR) */
+#define PCPHPERRINTSTS			0x0000
+#define PCPHPERRINTMSK			0x0004
+#define  MCU_CTL_ERR_MASK		BIT(12)
+#define  IOB_PA_ERR_MASK		BIT(11)
+#define  IOB_BA_ERR_MASK		BIT(10)
+#define  IOB_XGIC_ERR_MASK		BIT(9)
+#define  IOB_RB_ERR_MASK		BIT(8)
+#define  L3C_UNCORR_ERR_MASK		BIT(5)
+#define  MCU_UNCORR_ERR_MASK		BIT(4)
+#define  PMD3_MERR_MASK			BIT(3)
+#define  PMD2_MERR_MASK			BIT(2)
+#define  PMD1_MERR_MASK			BIT(1)
+#define  PMD0_MERR_MASK			BIT(0)
+#define PCPLPERRINTSTS			0x0008
+#define PCPLPERRINTMSK			0x000C
+#define  CSW_SWITCH_TRACE_ERR_MASK	BIT(2)
+#define  L3C_CORR_ERR_MASK		BIT(1)
+#define  MCU_CORR_ERR_MASK		BIT(0)
+#define MEMERRINTSTS			0x0010
+#define MEMERRINTMSK			0x0014
+
+/* Memory controller error CSR */
+#define MCU_MAX_RANK			8
+#define MCU_RANK_STRIDE			0x40
+
+#define MCUGECR				0x0110
+#define  MCU_GECR_DEMANDUCINTREN_MASK	BIT(0)
+#define  MCU_GECR_BACKUCINTREN_MASK	BIT(1)
+#define  MCU_GECR_CINTREN_MASK		BIT(2)
+#define  MUC_GECR_MCUADDRERREN_MASK	BIT(9)
+#define MCUGESR				0x0114
+#define  MCU_GESR_ADDRNOMATCH_ERR_MASK	BIT(7)
+#define  MCU_GESR_ADDRMULTIMATCH_ERR_MASK	BIT(6)
+#define  MCU_GESR_PHYP_ERR_MASK		BIT(3)
+#define MCUESRR0			0x0314
+#define  MCU_ESRR_MULTUCERR_MASK	BIT(3)
+#define  MCU_ESRR_BACKUCERR_MASK	BIT(2)
+#define  MCU_ESRR_DEMANDUCERR_MASK	BIT(1)
+#define  MCU_ESRR_CERR_MASK		BIT(0)
+#define MCUESRRA0			0x0318
+#define MCUEBLRR0			0x031c
+#define  MCU_EBLRR_ERRBANK_RD(src)	(((src) & 0x00000007) >> 0)
+#define MCUERCRR0			0x0320
+#define  MCU_ERCRR_ERRROW_RD(src)	(((src) & 0xFFFF0000) >> 16)
+#define  MCU_ERCRR_ERRCOL_RD(src)	((src) & 0x00000FFF)
+#define MCUSBECNT0			0x0324
+#define MCU_SBECNT_COUNT(src)		((src) & 0xFFFF)
+
+#define CSW_CSWCR			0x0000
+#define  CSW_CSWCR_DUALMCB_MASK		BIT(0)
+
+#define MCBADDRMR			0x0000
+#define  MCBADDRMR_MCU_INTLV_MODE_MASK	BIT(3)
+#define  MCBADDRMR_DUALMCU_MODE_MASK	BIT(2)
+#define  MCBADDRMR_MCB_INTLV_MODE_MASK	BIT(1)
+#define  MCBADDRMR_ADDRESS_MODE_MASK	BIT(0)
+
+struct xgene_edac_mc_ctx {
+	char *name;
+	struct regmap *pcp_map;
+	struct regmap *csw_map;
+	struct regmap *mcba_map;
+	struct regmap *mcbb_map;
+	void __iomem *mcu_csr;
+	int mcu_id;
+};
+
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+#ifdef CONFIG_EDAC_DEBUG
+static ssize_t xgene_edac_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 xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	int i;
+
+	for (i = 0; i < MCU_MAX_RANK; i++) {
+		writel(MCU_ESRR_MULTUCERR_MASK | MCU_ESRR_BACKUCERR_MASK |
+		       MCU_ESRR_DEMANDUCERR_MASK | MCU_ESRR_CERR_MASK,
+		       ctx->mcu_csr + MCUESRRA0 + i * MCU_RANK_STRIDE);
+	}
+	return count;
+}
+
+static const struct file_operations xgene_edac_mc_debug_inject_fops = {
+	.open = simple_open,
+	.write = xgene_edac_mc_err_inject_write,
+	.llseek = generic_file_llseek,
+};
+
+static void xgene_edac_mc_create_debugfs_node(struct mem_ctl_info *mci)
+{
+	if (!mci->debugfs)
+		return;
+
+	debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs, mci,
+			    &xgene_edac_mc_debug_inject_fops);
+}
+#else
+static void xgene_edac_mc_create_debugfs_node(struct mem_ctl_info *mci)
+{
+}
+#endif
+
+static void xgene_edac_mc_check(struct mem_ctl_info *mci)
+{
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	unsigned int pcp_hp_stat;
+	unsigned int pcp_lp_stat;
+	u32 reg;
+	u32 rank;
+	u32 bank;
+	u32 count;
+	u32 col_row;
+
+	if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat))
+		return;
+	if (regmap_read(ctx->pcp_map, PCPLPERRINTSTS, &pcp_lp_stat))
+		return;
+	if (!((MCU_UNCORR_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CTL_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CORR_ERR_MASK & pcp_lp_stat)))
+		return;
+
+	for (rank = 0; rank < MCU_MAX_RANK; rank++) {
+		reg = readl(ctx->mcu_csr + MCUESRR0 + rank * MCU_RANK_STRIDE);
+
+		/* Detect uncorrectable memory error */
+		if (reg & (MCU_ESRR_DEMANDUCERR_MASK |
+			   MCU_ESRR_BACKUCERR_MASK)) {
+			/* Detected uncorrectable memory error */
+			edac_mc_chipset_printk(mci, KERN_ERR, "X-Gene",
+				"MCU uncorrectable error at rank %d\n", rank);
+
+			edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				1, 0, 0, 0, 0, 0, -1, mci->ctl_name, "");
+		}
+
+		/* Detect correctable memory error */
+		if (reg & MCU_ESRR_CERR_MASK) {
+			bank = readl(ctx->mcu_csr + MCUEBLRR0 +
+				     rank * MCU_RANK_STRIDE);
+			col_row = readl(ctx->mcu_csr + MCUERCRR0 +
+					rank * MCU_RANK_STRIDE);
+			count = readl(ctx->mcu_csr + MCUSBECNT0 +
+				      rank * MCU_RANK_STRIDE);
+			edac_mc_chipset_printk(mci, KERN_WARNING, "X-Gene",
+				"MCU correctable error at rank %d bank %d column %d row %d count %d\n",
+				rank, MCU_EBLRR_ERRBANK_RD(bank),
+				MCU_ERCRR_ERRCOL_RD(col_row),
+				MCU_ERCRR_ERRROW_RD(col_row),
+				MCU_SBECNT_COUNT(count));
+
+			edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				1, 0, 0, 0, 0, 0, -1, mci->ctl_name, "");
+		}
+
+		/* Clear all error registers */
+		writel(0x0, ctx->mcu_csr + MCUEBLRR0 + rank * MCU_RANK_STRIDE);
+		writel(0x0, ctx->mcu_csr + MCUERCRR0 + rank * MCU_RANK_STRIDE);
+		writel(0x0, ctx->mcu_csr + MCUSBECNT0 +
+		       rank * MCU_RANK_STRIDE);
+		writel(reg, ctx->mcu_csr + MCUESRR0 + rank * MCU_RANK_STRIDE);
+	}
+
+	/* Detect memory controller error */
+	reg = readl(ctx->mcu_csr + MCUGESR);
+	if (reg) {
+		if (reg & MCU_GESR_ADDRNOMATCH_ERR_MASK)
+			edac_mc_chipset_printk(mci, KERN_WARNING, "X-Gene",
+				"MCU address miss-match error\n");
+		if (reg & MCU_GESR_ADDRMULTIMATCH_ERR_MASK)
+			edac_mc_chipset_printk(mci, KERN_WARNING, "X-Gene",
+				"MCU address multi-match error\n");
+
+		writel(reg, ctx->mcu_csr + MCUGESR);
+	}
+}
+
+static irqreturn_t xgene_edac_mc_isr(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	unsigned int pcp_hp_stat;
+	unsigned int pcp_lp_stat;
+
+	if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat))
+		return IRQ_NONE;
+	if (regmap_read(ctx->pcp_map, PCPLPERRINTSTS, &pcp_lp_stat))
+		return IRQ_NONE;
+	if (!((MCU_UNCORR_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CTL_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CORR_ERR_MASK & pcp_lp_stat)))
+		return IRQ_NONE;
+
+	xgene_edac_mc_check(mci);
+
+	return IRQ_HANDLED;
+}
+
+static void xgene_edac_mc_irq_ctl(struct mem_ctl_info *mci, bool enable)
+{
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	unsigned int val;
+
+	if (edac_op_state != EDAC_OPSTATE_INT)
+		return;
+
+	mutex_lock(&xgene_edac_mc_lock);
+
+	/*
+	 * As there is only single bit for enable error and interrupt mask,
+	 * we must only enable top level interrupt after all MCUs are
+	 * registered. Otherwise, if there is an error and the corresponding
+	 * MCU has not registered, the interrupt will never get cleared. To
+	 * determine all MCU have registered, we will keep track of active
+	 * MCUs and registered MCUs.
+	 */
+	if (enable) {
+		/* Set registered MCU bit */
+		edac_mc_registered_mask |= 1 << ctx->mcu_id;
+
+		/* Enable interrupt after all active MCU registered */
+		if (edac_mc_registered_mask == edac_mc_active_mask) {
+			/* Enable memory controller top level interrupt */
+			if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val))
+				goto err;
+			val &= ~(MCU_UNCORR_ERR_MASK | MCU_CTL_ERR_MASK);
+			if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val))
+				goto err;
+			if (regmap_read(ctx->pcp_map, PCPLPERRINTMSK, &val))
+				goto err;
+			val &= ~MCU_CORR_ERR_MASK;
+			if (regmap_write(ctx->pcp_map, PCPLPERRINTMSK, val))
+				goto err;
+		}
+
+		/* Enable MCU interrupt and error reporting */
+		val = readl(ctx->mcu_csr + MCUGECR);
+		val |= MCU_GECR_DEMANDUCINTREN_MASK |
+		       MCU_GECR_BACKUCINTREN_MASK |
+		       MCU_GECR_CINTREN_MASK |
+		       MUC_GECR_MCUADDRERREN_MASK;
+		writel(val, ctx->mcu_csr + MCUGECR);
+	} else {
+		/* Disable MCU interrupt */
+		val = readl(ctx->mcu_csr + MCUGECR);
+		val &= ~(MCU_GECR_DEMANDUCINTREN_MASK |
+			 MCU_GECR_BACKUCINTREN_MASK |
+			 MCU_GECR_CINTREN_MASK |
+			 MUC_GECR_MCUADDRERREN_MASK);
+		writel(val, ctx->mcu_csr + MCUGECR);
+
+		/* Disable memory controller top level interrupt */
+		if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val))
+			goto err;
+		val |= MCU_UNCORR_ERR_MASK | MCU_CTL_ERR_MASK;
+		if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val))
+			goto err;
+		if (regmap_read(ctx->pcp_map, PCPLPERRINTMSK, &val))
+			goto err;
+		val |= MCU_CORR_ERR_MASK;
+		if (regmap_write(ctx->pcp_map, PCPLPERRINTMSK, val))
+			goto err;
+
+		/* Clear registered MCU bit */
+		edac_mc_registered_mask &= ~(1 << ctx->mcu_id);
+	}
+
+err:
+	mutex_unlock(&xgene_edac_mc_lock);
+}
+
+static int xgene_edac_mc_is_active(struct xgene_edac_mc_ctx *ctx, int mc_idx)
+{
+	unsigned int reg;
+	u32 mcu_mask;
+
+	if (regmap_read(ctx->csw_map, CSW_CSWCR, &reg))
+		return 0;
+
+	if (reg & CSW_CSWCR_DUALMCB_MASK) {
+		/*
+		 * Dual MCB active - Determine if all 4 active or just MCU0
+		 * and MCU2 active
+		 */
+		if (regmap_read(ctx->mcbb_map, MCBADDRMR, &reg))
+			return 0;
+		mcu_mask = (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0xF : 0x5;
+	} else {
+		/*
+		 * Single MCB active - Determine if MCU0/MCU1 or just MCU0
+		 * active
+		 */
+		if (regmap_read(ctx->mcba_map, MCBADDRMR, &reg))
+			return 0;
+		mcu_mask = (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0x3 : 0x1;
+	}
+
+	/* Save active MC mask if hasn't set already */
+	if (!edac_mc_active_mask)
+		edac_mc_active_mask = mcu_mask;
+
+	return (mcu_mask & (1 << mc_idx)) ? 1 : 0;
+}
+
+static int xgene_edac_mc_probe(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[2];
+	struct xgene_edac_mc_ctx tmp_ctx;
+	struct xgene_edac_mc_ctx *ctx;
+	struct resource *res;
+	int rc = 0;
+
+	if (!devres_open_group(&pdev->dev, xgene_edac_mc_probe, GFP_KERNEL))
+		return -ENOMEM;
+
+	tmp_ctx.pcp_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							  "regmap-pcp");
+	if (IS_ERR(tmp_ctx.pcp_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap pcp\n");
+		rc = PTR_ERR(tmp_ctx.pcp_map);
+		goto err_group;
+	}
+
+	tmp_ctx.csw_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							  "regmap-csw");
+	if (IS_ERR(tmp_ctx.csw_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap csw\n");
+		rc = PTR_ERR(tmp_ctx.csw_map);
+		goto err_group;
+	}
+
+	tmp_ctx.mcba_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							   "regmap-mcba");
+	if (IS_ERR(tmp_ctx.mcba_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap mcba\n");
+		rc = PTR_ERR(tmp_ctx.mcba_map);
+		goto err_group;
+	}
+
+	tmp_ctx.mcbb_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							   "regmap-mcbb");
+	if (IS_ERR(tmp_ctx.mcbb_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap mcbb\n");
+		rc = PTR_ERR(tmp_ctx.mcbb_map);
+		goto err_group;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tmp_ctx.mcu_csr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tmp_ctx.mcu_csr)) {
+		dev_err(&pdev->dev, "no MCU resource address\n");
+		rc = PTR_ERR(tmp_ctx.mcu_csr);
+		goto err_group;
+	}
+	/* Ignore non-active MCU */
+	tmp_ctx.mcu_id = ((res->start >> 16) & 0xF) / 4;
+	if (!xgene_edac_mc_is_active(&tmp_ctx, tmp_ctx.mcu_id)) {
+		rc = -ENODEV;
+		goto err_group;
+	}
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = 4;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = 2;
+	layers[1].is_virt_csrow = false;
+	mci = edac_mc_alloc(edac_mc_idx++, ARRAY_SIZE(layers), layers,
+			    sizeof(*ctx));
+	if (!mci) {
+		rc = -ENOMEM;
+		goto err_group;
+	}
+
+	ctx = mci->pvt_info;
+	*ctx = tmp_ctx;		/* Copy over resource value */
+	ctx->name = "xgene_edac_mc_err";
+	mci->pdev = &pdev->dev;
+	dev_set_drvdata(mci->pdev, mci);
+	mci->ctl_name = ctx->name;
+	mci->dev_name = ctx->name;
+
+	mci->mtype_cap = MEM_FLAG_RDDR | MEM_FLAG_RDDR2 | MEM_FLAG_RDDR3 |
+			 MEM_FLAG_DDR | MEM_FLAG_DDR2 | MEM_FLAG_DDR3;
+	mci->edac_ctl_cap = EDAC_FLAG_SECDED;
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->mod_name = EDAC_MOD_STR;
+	mci->mod_ver = "0.1";
+	mci->ctl_page_to_phys = NULL;
+	mci->scrub_cap = SCRUB_FLAG_HW_SRC;
+	mci->scrub_mode = SCRUB_HW_SRC;
+
+	if (edac_op_state == EDAC_OPSTATE_POLL)
+		mci->edac_check = xgene_edac_mc_check;
+
+	if (edac_mc_add_mc(mci)) {
+		dev_err(&pdev->dev, "edac_mc_add_mc failed\n");
+		rc = -EINVAL;
+		goto err_free;
+	}
+
+	xgene_edac_mc_create_debugfs_node(mci);
+
+	if (edac_op_state == EDAC_OPSTATE_INT) {
+		int irq;
+		int i;
+
+		for (i = 0; i < 2; i++) {
+			irq = platform_get_irq(pdev, i);
+			if (irq < 0) {
+				dev_err(&pdev->dev, "No IRQ resource\n");
+				rc = -EINVAL;
+				goto err_del;
+			}
+			rc = devm_request_irq(&pdev->dev, irq,
+					      xgene_edac_mc_isr, IRQF_SHARED,
+					      dev_name(&pdev->dev), mci);
+			if (rc) {
+				dev_err(&pdev->dev,
+					"Could not request IRQ %d\n", irq);
+				goto err_del;
+			}
+		}
+	}
+
+	xgene_edac_mc_irq_ctl(mci, true);
+
+	devres_remove_group(&pdev->dev, xgene_edac_mc_probe);
+
+	dev_info(&pdev->dev, "X-Gene EDAC MC registered\n");
+	return 0;
+
+err_del:
+	edac_mc_del_mc(&pdev->dev);
+err_free:
+	edac_mc_free(mci);
+err_group:
+	devres_release_group(&pdev->dev, xgene_edac_mc_probe);
+	return rc;
+}
+
+static int xgene_edac_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = dev_get_drvdata(&pdev->dev);
+
+	xgene_edac_mc_irq_ctl(mci, false);
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+	return 0;
+}
+
+static struct of_device_id xgene_edac_mc_of_match[] = {
+	{ .compatible = "apm,xgene-edac-mc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_edac_of_match);
+
+static struct platform_driver xgene_edac_mc_driver = {
+	.probe = xgene_edac_mc_probe,
+	.remove = xgene_edac_mc_remove,
+	.driver = {
+		.name = "xgene-edac-mc",
+		.owner = THIS_MODULE,
+		.of_match_table = xgene_edac_mc_of_match,
+	},
+};
+
+static int __init xgene_edac_init(void)
+{
+	int rc;
+
+	/* Make sure error reporting method is sane */
+	switch (edac_op_state) {
+	case EDAC_OPSTATE_POLL:
+	case EDAC_OPSTATE_INT:
+		break;
+	default:
+		edac_op_state = EDAC_OPSTATE_INT;
+		break;
+	}
+
+	rc = platform_driver_register(&xgene_edac_mc_driver);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MOD_STR, "MCU fails to register\n");
+		goto reg_mc_failed;
+	}
+
+	return 0;
+
+reg_mc_failed:
+	return rc;
+}
+module_init(xgene_edac_init);
+
+static void __exit xgene_edac_exit(void)
+{
+	platform_driver_unregister(&xgene_edac_mc_driver);
+}
+module_exit(xgene_edac_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>");
+MODULE_DESCRIPTION("APM X-Gene Memory Controller EDAC driver");
+module_param(edac_op_state, int, 0444);
+MODULE_PARM_DESC(edac_op_state,
+		 "EDAC Error Reporting state: 0=Poll, 2=Interrupt");
--
1.7.1

--
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] 33+ messages in thread

* [PATCH v8 4/5] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06  4:02                 ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds support for the APM X-Gene SoC memory controller EDAC driver.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Loc Ho <lho@apm.com>
---
 drivers/edac/Kconfig         |    7 +
 drivers/edac/Makefile        |    1 +
 drivers/edac/xgene_edac_mc.c |  552 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+), 0 deletions(-)
 create mode 100644 drivers/edac/xgene_edac_mc.c

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index cb59619..8ab74cc 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -392,4 +392,11 @@ config EDAC_SYNOPSYS
 	  Support for error detection and correction on the Synopsys DDR
 	  memory controller.

+config EDAC_XGENE
+	tristate "APM X-Gene SoC"
+	depends on EDAC_MM_EDAC && ARM64
+	help
+	  Support for error detection and correction on the
+	  APM X-Gene family of SOCs.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index b255f36..ff5c316 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -68,3 +68,4 @@ obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o

 obj-$(CONFIG_EDAC_ALTERA_MC)		+= altera_edac.o
 obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
+obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac_mc.o
diff --git a/drivers/edac/xgene_edac_mc.c b/drivers/edac/xgene_edac_mc.c
new file mode 100644
index 0000000..81ebf1d
--- /dev/null
+++ b/drivers/edac/xgene_edac_mc.c
@@ -0,0 +1,552 @@
+/*
+ * APM X-Gene SoC Memory Controller EDAC (error detection and correction)
+ *
+ * Copyright (c) 2015, Applied Micro Circuits Corporation
+ * Author: Feng Kan <fkan@apm.com>
+ *         Loc Ho <lho@apm.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * 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/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include <asm/cputype.h>
+
+#include "edac_core.h"
+
+#define EDAC_MOD_STR			"xgene_edac_mc"
+
+static int edac_mc_idx;
+static int edac_mc_active_mask;
+static int edac_mc_registered_mask;
+static DEFINE_MUTEX(xgene_edac_mc_lock);
+
+/* Global error configuration status registers (CSR) */
+#define PCPHPERRINTSTS			0x0000
+#define PCPHPERRINTMSK			0x0004
+#define  MCU_CTL_ERR_MASK		BIT(12)
+#define  IOB_PA_ERR_MASK		BIT(11)
+#define  IOB_BA_ERR_MASK		BIT(10)
+#define  IOB_XGIC_ERR_MASK		BIT(9)
+#define  IOB_RB_ERR_MASK		BIT(8)
+#define  L3C_UNCORR_ERR_MASK		BIT(5)
+#define  MCU_UNCORR_ERR_MASK		BIT(4)
+#define  PMD3_MERR_MASK			BIT(3)
+#define  PMD2_MERR_MASK			BIT(2)
+#define  PMD1_MERR_MASK			BIT(1)
+#define  PMD0_MERR_MASK			BIT(0)
+#define PCPLPERRINTSTS			0x0008
+#define PCPLPERRINTMSK			0x000C
+#define  CSW_SWITCH_TRACE_ERR_MASK	BIT(2)
+#define  L3C_CORR_ERR_MASK		BIT(1)
+#define  MCU_CORR_ERR_MASK		BIT(0)
+#define MEMERRINTSTS			0x0010
+#define MEMERRINTMSK			0x0014
+
+/* Memory controller error CSR */
+#define MCU_MAX_RANK			8
+#define MCU_RANK_STRIDE			0x40
+
+#define MCUGECR				0x0110
+#define  MCU_GECR_DEMANDUCINTREN_MASK	BIT(0)
+#define  MCU_GECR_BACKUCINTREN_MASK	BIT(1)
+#define  MCU_GECR_CINTREN_MASK		BIT(2)
+#define  MUC_GECR_MCUADDRERREN_MASK	BIT(9)
+#define MCUGESR				0x0114
+#define  MCU_GESR_ADDRNOMATCH_ERR_MASK	BIT(7)
+#define  MCU_GESR_ADDRMULTIMATCH_ERR_MASK	BIT(6)
+#define  MCU_GESR_PHYP_ERR_MASK		BIT(3)
+#define MCUESRR0			0x0314
+#define  MCU_ESRR_MULTUCERR_MASK	BIT(3)
+#define  MCU_ESRR_BACKUCERR_MASK	BIT(2)
+#define  MCU_ESRR_DEMANDUCERR_MASK	BIT(1)
+#define  MCU_ESRR_CERR_MASK		BIT(0)
+#define MCUESRRA0			0x0318
+#define MCUEBLRR0			0x031c
+#define  MCU_EBLRR_ERRBANK_RD(src)	(((src) & 0x00000007) >> 0)
+#define MCUERCRR0			0x0320
+#define  MCU_ERCRR_ERRROW_RD(src)	(((src) & 0xFFFF0000) >> 16)
+#define  MCU_ERCRR_ERRCOL_RD(src)	((src) & 0x00000FFF)
+#define MCUSBECNT0			0x0324
+#define MCU_SBECNT_COUNT(src)		((src) & 0xFFFF)
+
+#define CSW_CSWCR			0x0000
+#define  CSW_CSWCR_DUALMCB_MASK		BIT(0)
+
+#define MCBADDRMR			0x0000
+#define  MCBADDRMR_MCU_INTLV_MODE_MASK	BIT(3)
+#define  MCBADDRMR_DUALMCU_MODE_MASK	BIT(2)
+#define  MCBADDRMR_MCB_INTLV_MODE_MASK	BIT(1)
+#define  MCBADDRMR_ADDRESS_MODE_MASK	BIT(0)
+
+struct xgene_edac_mc_ctx {
+	char *name;
+	struct regmap *pcp_map;
+	struct regmap *csw_map;
+	struct regmap *mcba_map;
+	struct regmap *mcbb_map;
+	void __iomem *mcu_csr;
+	int mcu_id;
+};
+
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+#ifdef CONFIG_EDAC_DEBUG
+static ssize_t xgene_edac_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 xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	int i;
+
+	for (i = 0; i < MCU_MAX_RANK; i++) {
+		writel(MCU_ESRR_MULTUCERR_MASK | MCU_ESRR_BACKUCERR_MASK |
+		       MCU_ESRR_DEMANDUCERR_MASK | MCU_ESRR_CERR_MASK,
+		       ctx->mcu_csr + MCUESRRA0 + i * MCU_RANK_STRIDE);
+	}
+	return count;
+}
+
+static const struct file_operations xgene_edac_mc_debug_inject_fops = {
+	.open = simple_open,
+	.write = xgene_edac_mc_err_inject_write,
+	.llseek = generic_file_llseek,
+};
+
+static void xgene_edac_mc_create_debugfs_node(struct mem_ctl_info *mci)
+{
+	if (!mci->debugfs)
+		return;
+
+	debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs, mci,
+			    &xgene_edac_mc_debug_inject_fops);
+}
+#else
+static void xgene_edac_mc_create_debugfs_node(struct mem_ctl_info *mci)
+{
+}
+#endif
+
+static void xgene_edac_mc_check(struct mem_ctl_info *mci)
+{
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	unsigned int pcp_hp_stat;
+	unsigned int pcp_lp_stat;
+	u32 reg;
+	u32 rank;
+	u32 bank;
+	u32 count;
+	u32 col_row;
+
+	if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat))
+		return;
+	if (regmap_read(ctx->pcp_map, PCPLPERRINTSTS, &pcp_lp_stat))
+		return;
+	if (!((MCU_UNCORR_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CTL_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CORR_ERR_MASK & pcp_lp_stat)))
+		return;
+
+	for (rank = 0; rank < MCU_MAX_RANK; rank++) {
+		reg = readl(ctx->mcu_csr + MCUESRR0 + rank * MCU_RANK_STRIDE);
+
+		/* Detect uncorrectable memory error */
+		if (reg & (MCU_ESRR_DEMANDUCERR_MASK |
+			   MCU_ESRR_BACKUCERR_MASK)) {
+			/* Detected uncorrectable memory error */
+			edac_mc_chipset_printk(mci, KERN_ERR, "X-Gene",
+				"MCU uncorrectable error at rank %d\n", rank);
+
+			edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				1, 0, 0, 0, 0, 0, -1, mci->ctl_name, "");
+		}
+
+		/* Detect correctable memory error */
+		if (reg & MCU_ESRR_CERR_MASK) {
+			bank = readl(ctx->mcu_csr + MCUEBLRR0 +
+				     rank * MCU_RANK_STRIDE);
+			col_row = readl(ctx->mcu_csr + MCUERCRR0 +
+					rank * MCU_RANK_STRIDE);
+			count = readl(ctx->mcu_csr + MCUSBECNT0 +
+				      rank * MCU_RANK_STRIDE);
+			edac_mc_chipset_printk(mci, KERN_WARNING, "X-Gene",
+				"MCU correctable error at rank %d bank %d column %d row %d count %d\n",
+				rank, MCU_EBLRR_ERRBANK_RD(bank),
+				MCU_ERCRR_ERRCOL_RD(col_row),
+				MCU_ERCRR_ERRROW_RD(col_row),
+				MCU_SBECNT_COUNT(count));
+
+			edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				1, 0, 0, 0, 0, 0, -1, mci->ctl_name, "");
+		}
+
+		/* Clear all error registers */
+		writel(0x0, ctx->mcu_csr + MCUEBLRR0 + rank * MCU_RANK_STRIDE);
+		writel(0x0, ctx->mcu_csr + MCUERCRR0 + rank * MCU_RANK_STRIDE);
+		writel(0x0, ctx->mcu_csr + MCUSBECNT0 +
+		       rank * MCU_RANK_STRIDE);
+		writel(reg, ctx->mcu_csr + MCUESRR0 + rank * MCU_RANK_STRIDE);
+	}
+
+	/* Detect memory controller error */
+	reg = readl(ctx->mcu_csr + MCUGESR);
+	if (reg) {
+		if (reg & MCU_GESR_ADDRNOMATCH_ERR_MASK)
+			edac_mc_chipset_printk(mci, KERN_WARNING, "X-Gene",
+				"MCU address miss-match error\n");
+		if (reg & MCU_GESR_ADDRMULTIMATCH_ERR_MASK)
+			edac_mc_chipset_printk(mci, KERN_WARNING, "X-Gene",
+				"MCU address multi-match error\n");
+
+		writel(reg, ctx->mcu_csr + MCUGESR);
+	}
+}
+
+static irqreturn_t xgene_edac_mc_isr(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	unsigned int pcp_hp_stat;
+	unsigned int pcp_lp_stat;
+
+	if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat))
+		return IRQ_NONE;
+	if (regmap_read(ctx->pcp_map, PCPLPERRINTSTS, &pcp_lp_stat))
+		return IRQ_NONE;
+	if (!((MCU_UNCORR_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CTL_ERR_MASK & pcp_hp_stat) ||
+	      (MCU_CORR_ERR_MASK & pcp_lp_stat)))
+		return IRQ_NONE;
+
+	xgene_edac_mc_check(mci);
+
+	return IRQ_HANDLED;
+}
+
+static void xgene_edac_mc_irq_ctl(struct mem_ctl_info *mci, bool enable)
+{
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	unsigned int val;
+
+	if (edac_op_state != EDAC_OPSTATE_INT)
+		return;
+
+	mutex_lock(&xgene_edac_mc_lock);
+
+	/*
+	 * As there is only single bit for enable error and interrupt mask,
+	 * we must only enable top level interrupt after all MCUs are
+	 * registered. Otherwise, if there is an error and the corresponding
+	 * MCU has not registered, the interrupt will never get cleared. To
+	 * determine all MCU have registered, we will keep track of active
+	 * MCUs and registered MCUs.
+	 */
+	if (enable) {
+		/* Set registered MCU bit */
+		edac_mc_registered_mask |= 1 << ctx->mcu_id;
+
+		/* Enable interrupt after all active MCU registered */
+		if (edac_mc_registered_mask == edac_mc_active_mask) {
+			/* Enable memory controller top level interrupt */
+			if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val))
+				goto err;
+			val &= ~(MCU_UNCORR_ERR_MASK | MCU_CTL_ERR_MASK);
+			if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val))
+				goto err;
+			if (regmap_read(ctx->pcp_map, PCPLPERRINTMSK, &val))
+				goto err;
+			val &= ~MCU_CORR_ERR_MASK;
+			if (regmap_write(ctx->pcp_map, PCPLPERRINTMSK, val))
+				goto err;
+		}
+
+		/* Enable MCU interrupt and error reporting */
+		val = readl(ctx->mcu_csr + MCUGECR);
+		val |= MCU_GECR_DEMANDUCINTREN_MASK |
+		       MCU_GECR_BACKUCINTREN_MASK |
+		       MCU_GECR_CINTREN_MASK |
+		       MUC_GECR_MCUADDRERREN_MASK;
+		writel(val, ctx->mcu_csr + MCUGECR);
+	} else {
+		/* Disable MCU interrupt */
+		val = readl(ctx->mcu_csr + MCUGECR);
+		val &= ~(MCU_GECR_DEMANDUCINTREN_MASK |
+			 MCU_GECR_BACKUCINTREN_MASK |
+			 MCU_GECR_CINTREN_MASK |
+			 MUC_GECR_MCUADDRERREN_MASK);
+		writel(val, ctx->mcu_csr + MCUGECR);
+
+		/* Disable memory controller top level interrupt */
+		if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val))
+			goto err;
+		val |= MCU_UNCORR_ERR_MASK | MCU_CTL_ERR_MASK;
+		if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val))
+			goto err;
+		if (regmap_read(ctx->pcp_map, PCPLPERRINTMSK, &val))
+			goto err;
+		val |= MCU_CORR_ERR_MASK;
+		if (regmap_write(ctx->pcp_map, PCPLPERRINTMSK, val))
+			goto err;
+
+		/* Clear registered MCU bit */
+		edac_mc_registered_mask &= ~(1 << ctx->mcu_id);
+	}
+
+err:
+	mutex_unlock(&xgene_edac_mc_lock);
+}
+
+static int xgene_edac_mc_is_active(struct xgene_edac_mc_ctx *ctx, int mc_idx)
+{
+	unsigned int reg;
+	u32 mcu_mask;
+
+	if (regmap_read(ctx->csw_map, CSW_CSWCR, &reg))
+		return 0;
+
+	if (reg & CSW_CSWCR_DUALMCB_MASK) {
+		/*
+		 * Dual MCB active - Determine if all 4 active or just MCU0
+		 * and MCU2 active
+		 */
+		if (regmap_read(ctx->mcbb_map, MCBADDRMR, &reg))
+			return 0;
+		mcu_mask = (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0xF : 0x5;
+	} else {
+		/*
+		 * Single MCB active - Determine if MCU0/MCU1 or just MCU0
+		 * active
+		 */
+		if (regmap_read(ctx->mcba_map, MCBADDRMR, &reg))
+			return 0;
+		mcu_mask = (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0x3 : 0x1;
+	}
+
+	/* Save active MC mask if hasn't set already */
+	if (!edac_mc_active_mask)
+		edac_mc_active_mask = mcu_mask;
+
+	return (mcu_mask & (1 << mc_idx)) ? 1 : 0;
+}
+
+static int xgene_edac_mc_probe(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[2];
+	struct xgene_edac_mc_ctx tmp_ctx;
+	struct xgene_edac_mc_ctx *ctx;
+	struct resource *res;
+	int rc = 0;
+
+	if (!devres_open_group(&pdev->dev, xgene_edac_mc_probe, GFP_KERNEL))
+		return -ENOMEM;
+
+	tmp_ctx.pcp_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							  "regmap-pcp");
+	if (IS_ERR(tmp_ctx.pcp_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap pcp\n");
+		rc = PTR_ERR(tmp_ctx.pcp_map);
+		goto err_group;
+	}
+
+	tmp_ctx.csw_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							  "regmap-csw");
+	if (IS_ERR(tmp_ctx.csw_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap csw\n");
+		rc = PTR_ERR(tmp_ctx.csw_map);
+		goto err_group;
+	}
+
+	tmp_ctx.mcba_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							   "regmap-mcba");
+	if (IS_ERR(tmp_ctx.mcba_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap mcba\n");
+		rc = PTR_ERR(tmp_ctx.mcba_map);
+		goto err_group;
+	}
+
+	tmp_ctx.mcbb_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							   "regmap-mcbb");
+	if (IS_ERR(tmp_ctx.mcbb_map)) {
+		dev_err(&pdev->dev, "unable to get syscon regmap mcbb\n");
+		rc = PTR_ERR(tmp_ctx.mcbb_map);
+		goto err_group;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tmp_ctx.mcu_csr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tmp_ctx.mcu_csr)) {
+		dev_err(&pdev->dev, "no MCU resource address\n");
+		rc = PTR_ERR(tmp_ctx.mcu_csr);
+		goto err_group;
+	}
+	/* Ignore non-active MCU */
+	tmp_ctx.mcu_id = ((res->start >> 16) & 0xF) / 4;
+	if (!xgene_edac_mc_is_active(&tmp_ctx, tmp_ctx.mcu_id)) {
+		rc = -ENODEV;
+		goto err_group;
+	}
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = 4;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = 2;
+	layers[1].is_virt_csrow = false;
+	mci = edac_mc_alloc(edac_mc_idx++, ARRAY_SIZE(layers), layers,
+			    sizeof(*ctx));
+	if (!mci) {
+		rc = -ENOMEM;
+		goto err_group;
+	}
+
+	ctx = mci->pvt_info;
+	*ctx = tmp_ctx;		/* Copy over resource value */
+	ctx->name = "xgene_edac_mc_err";
+	mci->pdev = &pdev->dev;
+	dev_set_drvdata(mci->pdev, mci);
+	mci->ctl_name = ctx->name;
+	mci->dev_name = ctx->name;
+
+	mci->mtype_cap = MEM_FLAG_RDDR | MEM_FLAG_RDDR2 | MEM_FLAG_RDDR3 |
+			 MEM_FLAG_DDR | MEM_FLAG_DDR2 | MEM_FLAG_DDR3;
+	mci->edac_ctl_cap = EDAC_FLAG_SECDED;
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->mod_name = EDAC_MOD_STR;
+	mci->mod_ver = "0.1";
+	mci->ctl_page_to_phys = NULL;
+	mci->scrub_cap = SCRUB_FLAG_HW_SRC;
+	mci->scrub_mode = SCRUB_HW_SRC;
+
+	if (edac_op_state == EDAC_OPSTATE_POLL)
+		mci->edac_check = xgene_edac_mc_check;
+
+	if (edac_mc_add_mc(mci)) {
+		dev_err(&pdev->dev, "edac_mc_add_mc failed\n");
+		rc = -EINVAL;
+		goto err_free;
+	}
+
+	xgene_edac_mc_create_debugfs_node(mci);
+
+	if (edac_op_state == EDAC_OPSTATE_INT) {
+		int irq;
+		int i;
+
+		for (i = 0; i < 2; i++) {
+			irq = platform_get_irq(pdev, i);
+			if (irq < 0) {
+				dev_err(&pdev->dev, "No IRQ resource\n");
+				rc = -EINVAL;
+				goto err_del;
+			}
+			rc = devm_request_irq(&pdev->dev, irq,
+					      xgene_edac_mc_isr, IRQF_SHARED,
+					      dev_name(&pdev->dev), mci);
+			if (rc) {
+				dev_err(&pdev->dev,
+					"Could not request IRQ %d\n", irq);
+				goto err_del;
+			}
+		}
+	}
+
+	xgene_edac_mc_irq_ctl(mci, true);
+
+	devres_remove_group(&pdev->dev, xgene_edac_mc_probe);
+
+	dev_info(&pdev->dev, "X-Gene EDAC MC registered\n");
+	return 0;
+
+err_del:
+	edac_mc_del_mc(&pdev->dev);
+err_free:
+	edac_mc_free(mci);
+err_group:
+	devres_release_group(&pdev->dev, xgene_edac_mc_probe);
+	return rc;
+}
+
+static int xgene_edac_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = dev_get_drvdata(&pdev->dev);
+
+	xgene_edac_mc_irq_ctl(mci, false);
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+	return 0;
+}
+
+static struct of_device_id xgene_edac_mc_of_match[] = {
+	{ .compatible = "apm,xgene-edac-mc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_edac_of_match);
+
+static struct platform_driver xgene_edac_mc_driver = {
+	.probe = xgene_edac_mc_probe,
+	.remove = xgene_edac_mc_remove,
+	.driver = {
+		.name = "xgene-edac-mc",
+		.owner = THIS_MODULE,
+		.of_match_table = xgene_edac_mc_of_match,
+	},
+};
+
+static int __init xgene_edac_init(void)
+{
+	int rc;
+
+	/* Make sure error reporting method is sane */
+	switch (edac_op_state) {
+	case EDAC_OPSTATE_POLL:
+	case EDAC_OPSTATE_INT:
+		break;
+	default:
+		edac_op_state = EDAC_OPSTATE_INT;
+		break;
+	}
+
+	rc = platform_driver_register(&xgene_edac_mc_driver);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MOD_STR, "MCU fails to register\n");
+		goto reg_mc_failed;
+	}
+
+	return 0;
+
+reg_mc_failed:
+	return rc;
+}
+module_init(xgene_edac_init);
+
+static void __exit xgene_edac_exit(void)
+{
+	platform_driver_unregister(&xgene_edac_mc_driver);
+}
+module_exit(xgene_edac_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Feng Kan <fkan@apm.com>");
+MODULE_DESCRIPTION("APM X-Gene Memory Controller EDAC driver");
+module_param(edac_op_state, int, 0444);
+MODULE_PARM_DESC(edac_op_state,
+		 "EDAC Error Reporting state: 0=Poll, 2=Interrupt");
--
1.7.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v8 5/5] arm64: Add APM X-Gene SoC memory controller EDAC DTS entries
  2015-05-06  4:02                 ` Loc Ho
@ 2015-05-06  4:02                     ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg
  Cc: linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y, Loc Ho,
	Feng Kan

This patch adds APM X-Gene SoC memory controller EDAC DTS entries.

Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>
---
 arch/arm64/boot/dts/apm/apm-storm.dtsi |   64 ++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index c8d3e0e..e0270a6 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -374,6 +374,70 @@
 			};
 		};

+		pcp: pcp@78800000 {
+			compatible = "syscon";
+			reg = <0x0 0x78800000 0x0 0x100>;
+		};
+
+		csw: csw@7e200000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e200000 0x0 0x1000>;
+		};
+
+		mcba: mcba@7e700000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e700000 0x0 0x1000>;
+		};
+
+		mcbb: mcbb@7e720000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e720000 0x0 0x1000>;
+		};
+
+		edacmc0: edacmc0@7e800000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e800000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc1: edacmc1@7e840000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e840000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc2: edacmc2@7e880000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e880000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc3: edacmc3@7e8c0000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e8c0000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
 		pcie0: pcie@1f2b0000 {
 			status = "disabled";
 			device_type = "pci";
--
1.7.1

--
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] 33+ messages in thread

* [PATCH v8 5/5] arm64: Add APM X-Gene SoC memory controller EDAC DTS entries
@ 2015-05-06  4:02                     ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06  4:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds APM X-Gene SoC memory controller EDAC DTS entries.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Loc Ho <lho@apm.com>
---
 arch/arm64/boot/dts/apm/apm-storm.dtsi |   64 ++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index c8d3e0e..e0270a6 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -374,6 +374,70 @@
 			};
 		};

+		pcp: pcp at 78800000 {
+			compatible = "syscon";
+			reg = <0x0 0x78800000 0x0 0x100>;
+		};
+
+		csw: csw at 7e200000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e200000 0x0 0x1000>;
+		};
+
+		mcba: mcba at 7e700000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e700000 0x0 0x1000>;
+		};
+
+		mcbb: mcbb at 7e720000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e720000 0x0 0x1000>;
+		};
+
+		edacmc0: edacmc0 at 7e800000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e800000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc1: edacmc1 at 7e840000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e840000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc2: edacmc2 at 7e880000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e880000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc3: edacmc3 at 7e8c0000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e8c0000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
 		pcie0: pcie at 1f2b0000 {
 			status = "disabled";
 			device_type = "pci";
--
1.7.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 1/5] arm64: Enable EDAC on ARM64
  2015-05-06  4:02     ` Loc Ho
@ 2015-05-06  4:10       ` Jon Masters
  -1 siblings, 0 replies; 33+ messages in thread
From: Jon Masters @ 2015-05-06  4:10 UTC (permalink / raw)
  To: Loc Ho, dougthompson, bp, mchehab, robh+dt, mark.rutland, ijc+devicetree
  Cc: devicetree, patches, linux-arm-kernel, linux-edac

Hi Loc,

Great to see v8 of the patches :) A comment below.

On 05/06/2015 12:02 AM, Loc Ho wrote:

> Add an stub atomic_scrub function and enable EDAC for arm64.

> +/*
> + * ECC atomic, DMA, SMP and interrupt safe scrub function.
> + */

nit: I expect you'll still get a little push back on that (similar to
last time) because it doesn't actually implement anything, so while it
is "safe" it's only because it's empty :) Might be better to indicate
that those are the features it is supposed to have when implemented.

> +static inline void atomic_scrub(void *va, u32 size)
> +{
> +	/* Stub function for now until an ARM64 HW has a way to test it. */
> +	WARN_ONCE(1, "not implemented");
> +}

That's great and seems to address the previous comment from v7.

Thanks,

Jon.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v8 1/5] arm64: Enable EDAC on ARM64
@ 2015-05-06  4:10       ` Jon Masters
  0 siblings, 0 replies; 33+ messages in thread
From: Jon Masters @ 2015-05-06  4:10 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Loc,

Great to see v8 of the patches :) A comment below.

On 05/06/2015 12:02 AM, Loc Ho wrote:

> Add an stub atomic_scrub function and enable EDAC for arm64.

> +/*
> + * ECC atomic, DMA, SMP and interrupt safe scrub function.
> + */

nit: I expect you'll still get a little push back on that (similar to
last time) because it doesn't actually implement anything, so while it
is "safe" it's only because it's empty :) Might be better to indicate
that those are the features it is supposed to have when implemented.

> +static inline void atomic_scrub(void *va, u32 size)
> +{
> +	/* Stub function for now until an ARM64 HW has a way to test it. */
> +	WARN_ONCE(1, "not implemented");
> +}

That's great and seems to address the previous comment from v7.

Thanks,

Jon.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06  4:02 ` Loc Ho
@ 2015-05-06  8:41     ` Borislav Petkov
  -1 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2015-05-06  8:41 UTC (permalink / raw)
  To: Loc Ho
  Cc: dougthompson-aS9lmoZGLiVWk0Htik3J/w,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y

On Tue, May 05, 2015 at 10:02:22PM -0600, Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC memory controller EDAC driver
> for DT.
> 
> v8:
> * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> * Add WARN_ONCE in stub function atomic_scrub
> * Update DTS binding documentation (with only memory controller node)
> * Temporary remove L1/L2, L3, and SoC driver code and update memory driver
>   code accordingly

What does that mean exactly? They'll get added later?

It is called now xgene_edac_mc.c. Am I to expect more
xgene_edac_<functional_unit>.c submissions? What happened to building
everything around the shared IRQ handler?

Also, I see this SOB chain in some of the patches:

Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
Signed-off-by: Loc Ho <lho-qTEPVZfXA3Y@public.gmane.org>

and it is not clear who did what, from looking at the chain.

If you both authored the patches, you can write

Originally-by: Feng
[ Loc: did this and that]
Signed-off-by: Loc

for example.

See Documentation/SubmittingPatches for more info.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06  8:41     ` Borislav Petkov
  0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2015-05-06  8:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, May 05, 2015 at 10:02:22PM -0600, Loc Ho wrote:
> This patch adds support for the APM X-Gene SoC memory controller EDAC driver
> for DT.
> 
> v8:
> * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> * Add WARN_ONCE in stub function atomic_scrub
> * Update DTS binding documentation (with only memory controller node)
> * Temporary remove L1/L2, L3, and SoC driver code and update memory driver
>   code accordingly

What does that mean exactly? They'll get added later?

It is called now xgene_edac_mc.c. Am I to expect more
xgene_edac_<functional_unit>.c submissions? What happened to building
everything around the shared IRQ handler?

Also, I see this SOB chain in some of the patches:

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Loc Ho <lho@apm.com>

and it is not clear who did what, from looking at the chain.

If you both authored the patches, you can write

Originally-by: Feng
[ Loc: did this and that]
Signed-off-by: Loc

for example.

See Documentation/SubmittingPatches for more info.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06  4:02 ` Loc Ho
@ 2015-05-06  8:52     ` Arnd Bergmann
  -1 siblings, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2015-05-06  8:52 UTC (permalink / raw)
  To: Loc Ho
  Cc: dougthompson-aS9lmoZGLiVWk0Htik3J/w, bp-Gina5bIWoIWzQB+pC5nmwQ,
	mchehab-JPH+aEBZ4P+UEJcrhfAQsw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	jcm-H+wXaHxf7aLQT0dZR+AlfA, patches-qTEPVZfXA3Y

On Tuesday 05 May 2015 22:02:22 Loc Ho wrote:
> 
> v8:
> * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> * Add WARN_ONCE in stub function atomic_scrub
> * Update DTS binding documentation (with only memory controller node)
> * Temporary remove L1/L2, L3, and SoC driver code and update memory driver
>   code accordingly
> 
> 

I don't see how this is helping. You still use a syscon reference for the
pcp node after I told you not to, and you are completely leaving out the
other nodes, which makes it impossible to tell what your plan is for those.

Please come up with a plan that makes it possible to have proper support
for all the devices in the future. Leaving out bits because you know that
adding them later will be hard is not a good solution: if you screw up
the design now, adding them later will be even harder.

	Arnd
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06  8:52     ` Arnd Bergmann
  0 siblings, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2015-05-06  8:52 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday 05 May 2015 22:02:22 Loc Ho wrote:
> 
> v8:
> * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> * Add WARN_ONCE in stub function atomic_scrub
> * Update DTS binding documentation (with only memory controller node)
> * Temporary remove L1/L2, L3, and SoC driver code and update memory driver
>   code accordingly
> 
> 

I don't see how this is helping. You still use a syscon reference for the
pcp node after I told you not to, and you are completely leaving out the
other nodes, which makes it impossible to tell what your plan is for those.

Please come up with a plan that makes it possible to have proper support
for all the devices in the future. Leaving out bits because you know that
adding them later will be hard is not a good solution: if you screw up
the design now, adding them later will be even harder.

	Arnd

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06  8:41     ` Borislav Petkov
  (?)
@ 2015-05-06 17:00     ` Loc Ho
       [not found]       ` <CAPw-ZT=LepJr2Smjy81yhcTANdMRw99x1vR9rMoYsnHW_P3HPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  -1 siblings, 1 reply; 33+ messages in thread
From: Loc Ho @ 2015-05-06 17:00 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Doug Thompson, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
	Ian Campbell, linux-edac, devicetree, linux-arm-kernel,
	Jon Masters, patches

[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]

Hi,

> This patch adds support for the APM X-Gene SoC memory controller EDAC
> driver
> > for DT.
> >
> > v8:
> > * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> > * Add WARN_ONCE in stub function atomic_scrub
> > * Update DTS binding documentation (with only memory controller node)
> > * Temporary remove L1/L2, L3, and SoC driver code and update memory
> driver
> >   code accordingly
>
> What does that mean exactly? They'll get added later?
>

It is not include in this first patch set. Yes, it will be added later. I
will response along with Arnd subsequent email.​



> It is called now xgene_edac_mc.c. Am I to expect more
> xgene_edac_<functional_unit>.c submissions?


​Yes​



> What happened to building
> everything around the shared IRQ handler?
>

​It was discussed with Rob already in previous email. As we don't touch the
shared status registers besides reading status, it will just register as
shared interrupt.​



>
> Also, I see this SOB chain in some of the patches:
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Loc Ho <lho@apm.com>
>
> and it is not clear who did what, from looking at the chain.
>
> If you both authored the patches, you can write
>
> Originally-by: Feng
> [ Loc: did this and that]
> Signed-off-by: Loc
>
> for example.
>
> See Documentation/SubmittingPatches for more info.
>

​Okay... Feng was the originally author.

-Loc

[-- Attachment #2: Type: text/html, Size: 2858 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06 17:00     ` Loc Ho
@ 2015-05-06 17:12           ` Borislav Petkov
  0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2015-05-06 17:12 UTC (permalink / raw)
  To: Loc Ho
  Cc: Doug Thompson, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
	Ian Campbell, linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Masters,
	patches-qTEPVZfXA3Y

On Wed, May 06, 2015 at 10:00:45AM -0700, Loc Ho wrote:
> It was discussed with Rob already in previous email. As we don't touch
> the shared status registers besides reading status, it will just
> register as shared interrupt.

I don't see why that justifies the addition of an
edac_xgene-<functional_unit> compilation unit and thus an .ko module per
shared interrupt handler.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06 17:12           ` Borislav Petkov
  0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2015-05-06 17:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, May 06, 2015 at 10:00:45AM -0700, Loc Ho wrote:
> It was discussed with Rob already in previous email. As we don't touch
> the shared status registers besides reading status, it will just
> register as shared interrupt.

I don't see why that justifies the addition of an
edac_xgene-<functional_unit> compilation unit and thus an .ko module per
shared interrupt handler.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06  8:52     ` Arnd Bergmann
@ 2015-05-06 18:12       ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06 18:12 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Doug Thompson, Borislav Petkov, Mauro Carvalho Chehab,
	Rob Herring, Mark Rutland, Ian Campbell,
	linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Masters,
	patches-qTEPVZfXA3Y

Hi Arnd/Borislav/Rob,

> > v8:
> > * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> > * Add WARN_ONCE in stub function atomic_scrub
> > * Update DTS binding documentation (with only memory controller node)
> > * Temporary remove L1/L2, L3, and SoC driver code and update memory driver
> >   code accordingly
> >
> >
>
> I don't see how this is helping. You still use a syscon reference for the
> pcp node after I told you not to, and you are completely leaving out the
> other nodes, which makes it impossible to tell what your plan is for those.
>
>
> Please come up with a plan that makes it possible to have proper support
> for all the devices in the future. Leaving out bits because you know that
> adding them later will be hard is not a good solution: if you screw up
> the design now, adding them later will be even harder.


Okay... Let me summary the issue at hand and let us all agree:

1. Whether to have an single driver for APM EDAC or multiple instance
of 4 different drivers. With single driver, it does not scale in the
future when we add/remove memory controllers and CPU domains. This is
also agreed by Rob Herring from review of the DTS nodes. For L3 and
SoC EDAC, they are less of an issue as I don't see a situation that we
would have multiple instances.

2. With regard to the top level PCP interrupt, they are just for
status and once configured, it will not be touch. Therefore, I keep
the current implementation. With an single driver, there is no need to
worry about read/modify/write as it will be guarded with an lock. For
multiple instance, I am thinking that the xgene_edac_mc module will
provide exported lock functions for the other drivers.

If I am missing anything, let me know.

-Loc
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06 18:12       ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06 18:12 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Arnd/Borislav/Rob,

> > v8:
> > * Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
> > * Add WARN_ONCE in stub function atomic_scrub
> > * Update DTS binding documentation (with only memory controller node)
> > * Temporary remove L1/L2, L3, and SoC driver code and update memory driver
> >   code accordingly
> >
> >
>
> I don't see how this is helping. You still use a syscon reference for the
> pcp node after I told you not to, and you are completely leaving out the
> other nodes, which makes it impossible to tell what your plan is for those.
>
>
> Please come up with a plan that makes it possible to have proper support
> for all the devices in the future. Leaving out bits because you know that
> adding them later will be hard is not a good solution: if you screw up
> the design now, adding them later will be even harder.


Okay... Let me summary the issue at hand and let us all agree:

1. Whether to have an single driver for APM EDAC or multiple instance
of 4 different drivers. With single driver, it does not scale in the
future when we add/remove memory controllers and CPU domains. This is
also agreed by Rob Herring from review of the DTS nodes. For L3 and
SoC EDAC, they are less of an issue as I don't see a situation that we
would have multiple instances.

2. With regard to the top level PCP interrupt, they are just for
status and once configured, it will not be touch. Therefore, I keep
the current implementation. With an single driver, there is no need to
worry about read/modify/write as it will be guarded with an lock. For
multiple instance, I am thinking that the xgene_edac_mc module will
provide exported lock functions for the other drivers.

If I am missing anything, let me know.

-Loc

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06 17:12           ` Borislav Petkov
@ 2015-05-06 18:17               ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06 18:17 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Doug Thompson, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
	Ian Campbell, linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Masters,
	patches-qTEPVZfXA3Y

Hi,

>> It was discussed with Rob already in previous email. As we don't touch
>> the shared status registers besides reading status, it will just
>> register as shared interrupt.
>
> I don't see why that justifies the addition of an
> edac_xgene-<functional_unit> compilation unit and thus an .ko module per
> shared interrupt handler.

If you are concern about multiple module files, I can either combine
them as previous version or combine the multiple ko to an single ko
file.

-Loc
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06 18:17               ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06 18:17 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

>> It was discussed with Rob already in previous email. As we don't touch
>> the shared status registers besides reading status, it will just
>> register as shared interrupt.
>
> I don't see why that justifies the addition of an
> edac_xgene-<functional_unit> compilation unit and thus an .ko module per
> shared interrupt handler.

If you are concern about multiple module files, I can either combine
them as previous version or combine the multiple ko to an single ko
file.

-Loc

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06 18:12       ` Loc Ho
@ 2015-05-06 18:29           ` Borislav Petkov
  -1 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2015-05-06 18:29 UTC (permalink / raw)
  To: Loc Ho
  Cc: Arnd Bergmann, Doug Thompson, Mauro Carvalho Chehab, Rob Herring,
	Mark Rutland, Ian Campbell, linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Masters,
	patches-qTEPVZfXA3Y

On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
> 1. Whether to have an single driver for APM EDAC or multiple instance
> of 4 different drivers. With single driver, it does not scale in the
> future when we add/remove memory controllers and CPU domains. This is

Why doesn't it scale? Please explain this to me more verbosely.

> also agreed by Rob Herring from review of the DTS nodes. For L3 and
> SoC EDAC, they are less of an issue as I don't see a situation that we
> would have multiple instances.
> 
> 2. With regard to the top level PCP interrupt, they are just for
> status and once configured, it will not be touch. Therefore, I keep
> the current implementation. With an single driver, there is no need to
> worry about read/modify/write as it will be guarded with an lock. For
> multiple instance, I am thinking that the xgene_edac_mc module will
> provide exported lock functions for the other drivers.

Doing this would be a very bad design and it would be a homegrown case
only for this driver. Then other ARM drivers will appear which will do
their own locking too. No no no. No ad-hoc hackery.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06 18:29           ` Borislav Petkov
  0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2015-05-06 18:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
> 1. Whether to have an single driver for APM EDAC or multiple instance
> of 4 different drivers. With single driver, it does not scale in the
> future when we add/remove memory controllers and CPU domains. This is

Why doesn't it scale? Please explain this to me more verbosely.

> also agreed by Rob Herring from review of the DTS nodes. For L3 and
> SoC EDAC, they are less of an issue as I don't see a situation that we
> would have multiple instances.
> 
> 2. With regard to the top level PCP interrupt, they are just for
> status and once configured, it will not be touch. Therefore, I keep
> the current implementation. With an single driver, there is no need to
> worry about read/modify/write as it will be guarded with an lock. For
> multiple instance, I am thinking that the xgene_edac_mc module will
> provide exported lock functions for the other drivers.

Doing this would be a very bad design and it would be a homegrown case
only for this driver. Then other ARM drivers will appear which will do
their own locking too. No no no. No ad-hoc hackery.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06 18:29           ` Borislav Petkov
@ 2015-05-06 18:43               ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06 18:43 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arnd Bergmann, Doug Thompson, Mauro Carvalho Chehab, Rob Herring,
	Mark Rutland, Ian Campbell, linux-edac-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jon Masters,
	patches-qTEPVZfXA3Y

Hi Borislav,

On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
>> 1. Whether to have an single driver for APM EDAC or multiple instance
>> of 4 different drivers. With single driver, it does not scale in the
>> future when we add/remove memory controllers and CPU domains. This is
>
> Why doesn't it scale? Please explain this to me more verbosely.

Let me explain a bit more. We have four memory controller today.
Therefore, I would like to have 4 DTS node and the same driver probe
function called 4 times. If there is only one driver for the entire
APM EDAC, I would have to merge all the resource registers into an
single DTS node and its probe function called one time. In this one
driver design, what would I do in future chip or variant of the chip
in which it remove or add an addition memory controller? I would have
to change the driver as well as the DTS node. In the four instance
probe design, all I need is to add an additional DTS node.


>
>> also agreed by Rob Herring from review of the DTS nodes. For L3 and
>> SoC EDAC, they are less of an issue as I don't see a situation that we
>> would have multiple instances.
>>
>> 2. With regard to the top level PCP interrupt, they are just for
>> status and once configured, it will not be touch. Therefore, I keep
>> the current implementation. With an single driver, there is no need to
>> worry about read/modify/write as it will be guarded with an lock. For
>> multiple instance, I am thinking that the xgene_edac_mc module will
>> provide exported lock functions for the other drivers.
>
> Doing this would be a very bad design and it would be a homegrown case
> only for this driver. Then other ARM drivers will appear which will do
> their own locking too. No no no. No ad-hoc hackery.
>

What if I move the locking function into a common module to be
included by the EDAC framework? Or would you prefer that I go and
write an common driver that all it does is provide locking?

Any suggestion as all I need is an way to share access to an CSR which
can't use atomic operations? It isn't an actual interrupt controller
either.

-Loc
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06 18:43               ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-06 18:43 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Borislav,

On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
>> 1. Whether to have an single driver for APM EDAC or multiple instance
>> of 4 different drivers. With single driver, it does not scale in the
>> future when we add/remove memory controllers and CPU domains. This is
>
> Why doesn't it scale? Please explain this to me more verbosely.

Let me explain a bit more. We have four memory controller today.
Therefore, I would like to have 4 DTS node and the same driver probe
function called 4 times. If there is only one driver for the entire
APM EDAC, I would have to merge all the resource registers into an
single DTS node and its probe function called one time. In this one
driver design, what would I do in future chip or variant of the chip
in which it remove or add an addition memory controller? I would have
to change the driver as well as the DTS node. In the four instance
probe design, all I need is to add an additional DTS node.


>
>> also agreed by Rob Herring from review of the DTS nodes. For L3 and
>> SoC EDAC, they are less of an issue as I don't see a situation that we
>> would have multiple instances.
>>
>> 2. With regard to the top level PCP interrupt, they are just for
>> status and once configured, it will not be touch. Therefore, I keep
>> the current implementation. With an single driver, there is no need to
>> worry about read/modify/write as it will be guarded with an lock. For
>> multiple instance, I am thinking that the xgene_edac_mc module will
>> provide exported lock functions for the other drivers.
>
> Doing this would be a very bad design and it would be a homegrown case
> only for this driver. Then other ARM drivers will appear which will do
> their own locking too. No no no. No ad-hoc hackery.
>

What if I move the locking function into a common module to be
included by the EDAC framework? Or would you prefer that I go and
write an common driver that all it does is provide locking?

Any suggestion as all I need is an way to share access to an CSR which
can't use atomic operations? It isn't an actual interrupt controller
either.

-Loc

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06 18:43               ` Loc Ho
@ 2015-05-06 19:50                   ` Arnd Bergmann
  -1 siblings, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2015-05-06 19:50 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Loc Ho, Borislav Petkov, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Ian Campbell, Jon Masters,
	Mauro Carvalho Chehab, patches-qTEPVZfXA3Y, Rob Herring,
	Doug Thompson, linux-edac-u79uwXL29TY76Z2rM5mHXA

On Wednesday 06 May 2015 11:43:36 Loc Ho wrote:
> Hi Borislav,
> 
> On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> > On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
> >> 1. Whether to have an single driver for APM EDAC or multiple instance
> >> of 4 different drivers. With single driver, it does not scale in the
> >> future when we add/remove memory controllers and CPU domains. This is
> >
> > Why doesn't it scale? Please explain this to me more verbosely.
> 
> Let me explain a bit more. We have four memory controller today.
> Therefore, I would like to have 4 DTS node and the same driver probe
> function called 4 times. If there is only one driver for the entire
> APM EDAC, I would have to merge all the resource registers into an
> single DTS node and its probe function called one time. In this one
> driver design, what would I do in future chip or variant of the chip
> in which it remove or add an addition memory controller? I would have
> to change the driver as well as the DTS node. In the four instance
> probe design, all I need is to add an additional DTS node.

There are lots of possible representations in DT that would solve this.

First of all, a device node can be turned into a platform_device but
does not have to, so you you could have one DT node for the common
parts (the pcp), and then subnodes underneath it, and have the driver
attach to the main device but parse the subnodes manually to see
what registers are there. There is no magic involved here, and this
would address the concerns that Boris and I have.

Another option would be to have multiple drivers: have one driver
that handles the common parts here, and make that export a shared
interface to which the other drivers can register, then create
five drivers that each do one thing. In my opinion that would work
just as well, and be a nice abstraction, but I suspect that Boris
would prefer doing it the other way.


> >> also agreed by Rob Herring from review of the DTS nodes. For L3 and
> >> SoC EDAC, they are less of an issue as I don't see a situation that we
> >> would have multiple instances.
> >>
> >> 2. With regard to the top level PCP interrupt, they are just for
> >> status and once configured, it will not be touch. Therefore, I keep
> >> the current implementation. With an single driver, there is no need to
> >> worry about read/modify/write as it will be guarded with an lock. For
> >> multiple instance, I am thinking that the xgene_edac_mc module will
> >> provide exported lock functions for the other drivers.
> >
> > Doing this would be a very bad design and it would be a homegrown case
> > only for this driver. Then other ARM drivers will appear which will do
> > their own locking too. No no no. No ad-hoc hackery.

I don't think there is a huge risk that other ARM drivers would copy a bad
design, when it's obviously bad. In particular, other chips will most
likely have other requirements in this area.

> What if I move the locking function into a common module to be
> included by the EDAC framework? Or would you prefer that I go and
> write an common driver that all it does is provide locking?
> 
> Any suggestion as all I need is an way to share access to an CSR which
> can't use atomic operations? It isn't an actual interrupt controller
> either.

If this is something that does not fit into existing categories, you
probably want to create your own high-level abstraction between the
accesses to the shared parts of the driver and the parts that can
exist in multiple instances. This will work for either of the approaches
I've described above.

	Arnd
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-06 19:50                   ` Arnd Bergmann
  0 siblings, 0 replies; 33+ messages in thread
From: Arnd Bergmann @ 2015-05-06 19:50 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 06 May 2015 11:43:36 Loc Ho wrote:
> Hi Borislav,
> 
> On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp@alien8.de> wrote:
> > On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
> >> 1. Whether to have an single driver for APM EDAC or multiple instance
> >> of 4 different drivers. With single driver, it does not scale in the
> >> future when we add/remove memory controllers and CPU domains. This is
> >
> > Why doesn't it scale? Please explain this to me more verbosely.
> 
> Let me explain a bit more. We have four memory controller today.
> Therefore, I would like to have 4 DTS node and the same driver probe
> function called 4 times. If there is only one driver for the entire
> APM EDAC, I would have to merge all the resource registers into an
> single DTS node and its probe function called one time. In this one
> driver design, what would I do in future chip or variant of the chip
> in which it remove or add an addition memory controller? I would have
> to change the driver as well as the DTS node. In the four instance
> probe design, all I need is to add an additional DTS node.

There are lots of possible representations in DT that would solve this.

First of all, a device node can be turned into a platform_device but
does not have to, so you you could have one DT node for the common
parts (the pcp), and then subnodes underneath it, and have the driver
attach to the main device but parse the subnodes manually to see
what registers are there. There is no magic involved here, and this
would address the concerns that Boris and I have.

Another option would be to have multiple drivers: have one driver
that handles the common parts here, and make that export a shared
interface to which the other drivers can register, then create
five drivers that each do one thing. In my opinion that would work
just as well, and be a nice abstraction, but I suspect that Boris
would prefer doing it the other way.


> >> also agreed by Rob Herring from review of the DTS nodes. For L3 and
> >> SoC EDAC, they are less of an issue as I don't see a situation that we
> >> would have multiple instances.
> >>
> >> 2. With regard to the top level PCP interrupt, they are just for
> >> status and once configured, it will not be touch. Therefore, I keep
> >> the current implementation. With an single driver, there is no need to
> >> worry about read/modify/write as it will be guarded with an lock. For
> >> multiple instance, I am thinking that the xgene_edac_mc module will
> >> provide exported lock functions for the other drivers.
> >
> > Doing this would be a very bad design and it would be a homegrown case
> > only for this driver. Then other ARM drivers will appear which will do
> > their own locking too. No no no. No ad-hoc hackery.

I don't think there is a huge risk that other ARM drivers would copy a bad
design, when it's obviously bad. In particular, other chips will most
likely have other requirements in this area.

> What if I move the locking function into a common module to be
> included by the EDAC framework? Or would you prefer that I go and
> write an common driver that all it does is provide locking?
> 
> Any suggestion as all I need is an way to share access to an CSR which
> can't use atomic operations? It isn't an actual interrupt controller
> either.

If this is something that does not fit into existing categories, you
probably want to create your own high-level abstraction between the
accesses to the shared parts of the driver and the parts that can
exist in multiple instances. This will work for either of the approaches
I've described above.

	Arnd

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
  2015-05-06 19:50                   ` Arnd Bergmann
@ 2015-05-11 22:29                     ` Loc Ho
  -1 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-11 22:29 UTC (permalink / raw)
  To: Arnd Bergmann, Borislav Petkov
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Ian Campbell, Jon Masters,
	Mauro Carvalho Chehab, patches-qTEPVZfXA3Y, Rob Herring,
	Doug Thompson, linux-edac-u79uwXL29TY76Z2rM5mHXA

Hi,

>>
>> On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
>> > On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
>> >> 1. Whether to have an single driver for APM EDAC or multiple instance
>> >> of 4 different drivers. With single driver, it does not scale in the
>> >> future when we add/remove memory controllers and CPU domains. This is
>> >
>> > Why doesn't it scale? Please explain this to me more verbosely.
>>
>> Let me explain a bit more. We have four memory controller today.
>> Therefore, I would like to have 4 DTS node and the same driver probe
>> function called 4 times. If there is only one driver for the entire
>> APM EDAC, I would have to merge all the resource registers into an
>> single DTS node and its probe function called one time. In this one
>> driver design, what would I do in future chip or variant of the chip
>> in which it remove or add an addition memory controller? I would have
>> to change the driver as well as the DTS node. In the four instance
>> probe design, all I need is to add an additional DTS node.
>
> There are lots of possible representations in DT that would solve this.
>
> First of all, a device node can be turned into a platform_device but
> does not have to, so you you could have one DT node for the common
> parts (the pcp), and then subnodes underneath it, and have the driver
> attach to the main device but parse the subnodes manually to see
> what registers are there. There is no magic involved here, and this
> would address the concerns that Boris and I have.
>
> Another option would be to have multiple drivers: have one driver
> that handles the common parts here, and make that export a shared
> interface to which the other drivers can register, then create
> five drivers that each do one thing. In my opinion that would work
> just as well, and be a nice abstraction, but I suspect that Boris
> would prefer doing it the other way.

Thanks all for the comment. I will follow the design as with
gpio-dwapb driver. This would take care of Borislav concern as
mentioned by Arnd.

-Loc
--
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] 33+ messages in thread

* [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver
@ 2015-05-11 22:29                     ` Loc Ho
  0 siblings, 0 replies; 33+ messages in thread
From: Loc Ho @ 2015-05-11 22:29 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

>>
>> On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp@alien8.de> wrote:
>> > On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
>> >> 1. Whether to have an single driver for APM EDAC or multiple instance
>> >> of 4 different drivers. With single driver, it does not scale in the
>> >> future when we add/remove memory controllers and CPU domains. This is
>> >
>> > Why doesn't it scale? Please explain this to me more verbosely.
>>
>> Let me explain a bit more. We have four memory controller today.
>> Therefore, I would like to have 4 DTS node and the same driver probe
>> function called 4 times. If there is only one driver for the entire
>> APM EDAC, I would have to merge all the resource registers into an
>> single DTS node and its probe function called one time. In this one
>> driver design, what would I do in future chip or variant of the chip
>> in which it remove or add an addition memory controller? I would have
>> to change the driver as well as the DTS node. In the four instance
>> probe design, all I need is to add an additional DTS node.
>
> There are lots of possible representations in DT that would solve this.
>
> First of all, a device node can be turned into a platform_device but
> does not have to, so you you could have one DT node for the common
> parts (the pcp), and then subnodes underneath it, and have the driver
> attach to the main device but parse the subnodes manually to see
> what registers are there. There is no magic involved here, and this
> would address the concerns that Boris and I have.
>
> Another option would be to have multiple drivers: have one driver
> that handles the common parts here, and make that export a shared
> interface to which the other drivers can register, then create
> five drivers that each do one thing. In my opinion that would work
> just as well, and be a nice abstraction, but I suspect that Boris
> would prefer doing it the other way.

Thanks all for the comment. I will follow the design as with
gpio-dwapb driver. This would take care of Borislav concern as
mentioned by Arnd.

-Loc

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2015-05-11 22:29 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-06  4:02 [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver Loc Ho
2015-05-06  4:02 ` Loc Ho
     [not found] ` <1430884947-16787-1-git-send-email-lho-qTEPVZfXA3Y@public.gmane.org>
2015-05-06  4:02   ` [PATCH v8 1/5] arm64: Enable EDAC on ARM64 Loc Ho
2015-05-06  4:02     ` Loc Ho
     [not found]     ` <1430884947-16787-2-git-send-email-lho-qTEPVZfXA3Y@public.gmane.org>
2015-05-06  4:02       ` [PATCH v8 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver Loc Ho
2015-05-06  4:02         ` Loc Ho
     [not found]         ` <1430884947-16787-3-git-send-email-lho-qTEPVZfXA3Y@public.gmane.org>
2015-05-06  4:02           ` [PATCH v8 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Loc Ho
2015-05-06  4:02             ` Loc Ho
     [not found]             ` <1430884947-16787-4-git-send-email-lho-qTEPVZfXA3Y@public.gmane.org>
2015-05-06  4:02               ` [PATCH v8 4/5] edac: Add APM X-Gene SoC memory controller EDAC driver Loc Ho
2015-05-06  4:02                 ` Loc Ho
     [not found]                 ` <1430884947-16787-5-git-send-email-lho-qTEPVZfXA3Y@public.gmane.org>
2015-05-06  4:02                   ` [PATCH v8 5/5] arm64: Add APM X-Gene SoC memory controller EDAC DTS entries Loc Ho
2015-05-06  4:02                     ` Loc Ho
2015-05-06  4:10     ` [PATCH v8 1/5] arm64: Enable EDAC on ARM64 Jon Masters
2015-05-06  4:10       ` Jon Masters
2015-05-06  8:41   ` [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver Borislav Petkov
2015-05-06  8:41     ` Borislav Petkov
2015-05-06 17:00     ` Loc Ho
     [not found]       ` <CAPw-ZT=LepJr2Smjy81yhcTANdMRw99x1vR9rMoYsnHW_P3HPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-06 17:12         ` Borislav Petkov
2015-05-06 17:12           ` Borislav Petkov
     [not found]           ` <20150506171242.GG22949-fF5Pk5pvG8Y@public.gmane.org>
2015-05-06 18:17             ` Loc Ho
2015-05-06 18:17               ` Loc Ho
2015-05-06  8:52   ` Arnd Bergmann
2015-05-06  8:52     ` Arnd Bergmann
2015-05-06 18:12     ` Loc Ho
2015-05-06 18:12       ` Loc Ho
     [not found]       ` <CAPw-ZTkuZWNM9D_wJRfQq009KeL1coyYKXhqhWV+FWW6C=xRiA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-06 18:29         ` Borislav Petkov
2015-05-06 18:29           ` Borislav Petkov
     [not found]           ` <20150506182900.GI22949-fF5Pk5pvG8Y@public.gmane.org>
2015-05-06 18:43             ` Loc Ho
2015-05-06 18:43               ` Loc Ho
     [not found]               ` <CAPw-ZTmJ8C3KmujZJN1z21S3LFQ-TRouUeAjN0Y02HOTKAG1_g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-06 19:50                 ` Arnd Bergmann
2015-05-06 19:50                   ` Arnd Bergmann
2015-05-11 22:29                   ` Loc Ho
2015-05-11 22:29                     ` Loc Ho

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.