* [PATCH v7 0/4] edac: Add APM X-Gene SoC EDAC driver
@ 2015-04-28 22:10 Loc Ho
2015-04-28 22:10 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Loc Ho
0 siblings, 1 reply; 44+ messages in thread
From: Loc Ho @ 2015-04-28 22:10 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for the APM X-Gene SoC EDAC driver for DT.
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 | 107 +
MAINTAINERS | 8 +
arch/arm64/Kconfig | 1 +
arch/arm64/boot/dts/apm/apm-storm.dtsi | 123 ++
arch/arm64/include/asm/edac.h | 31 +
drivers/edac/Kconfig | 9 +-
drivers/edac/Makefile | 1 +
drivers/edac/xgene_edac.c | 2175 ++++++++++++++++++++
8 files changed, 2454 insertions(+), 1 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.c
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH v7 1/5] arm64: Enable EDAC on ARM64 2015-04-28 22:10 [PATCH v7 0/4] edac: Add APM X-Gene SoC EDAC driver Loc Ho @ 2015-04-28 22:10 ` Loc Ho 2015-04-28 22:10 ` [PATCH v7 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver Loc Ho 2015-04-29 14:40 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Catalin Marinas 0 siblings, 2 replies; 44+ messages in thread From: Loc Ho @ 2015-04-28 22:10 UTC (permalink / raw) To: linux-arm-kernel Add an stub atomic_scrub and enable EDAC for arm64. Signed-off-by: Loc Ho <lho@apm.com> --- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/edac.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 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..1cedba6 --- /dev/null +++ b/arch/arm64/include/asm/edac.h @@ -0,0 +1,31 @@ +/* + * 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. + * Implements the per arch atomic_scrub() that EDAC use for software + * ECC scrubbing. It reads memory and then writes back the original + * value, allowing the hardware to detect and correct memory errors. + */ +static inline void atomic_scrub(void *va, u32 size) +{ + /* Stub function for now until an ARM64 HW has a way to test it. */ +} + +#endif + -- 1.7.1 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v7 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver 2015-04-28 22:10 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Loc Ho @ 2015-04-28 22:10 ` Loc Ho 2015-04-28 22:10 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Loc Ho 2015-04-29 14:40 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Catalin Marinas 1 sibling, 1 reply; 44+ messages in thread From: Loc Ho @ 2015-04-28 22:10 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.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] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-28 22:10 ` [PATCH v7 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver Loc Ho @ 2015-04-28 22:10 ` Loc Ho 2015-04-28 22:10 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Loc Ho 2015-04-29 16:47 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Rob Herring 0 siblings, 2 replies; 44+ messages in thread From: Loc Ho @ 2015-04-28 22:10 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 | 107 ++++++++++++++++++++ 1 files changed, 107 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..548299a --- /dev/null +++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt @@ -0,0 +1,107 @@ +* APM X-Gene SoC EDAC nodes + +EDAC nodes are defined to describe on-chip error detection and correction. +There are four types of EDAC: + + memory controller - Memory controller + PMD (L1/L2) - Processor module unit (PMD) L1/L2 cache + L3 - CPU L3 cache + SoC - SoC IP such as SATA, Ethernet, and etc + +The following section describes the memory controller DT node binding. + +Required properties: +- compatible : Shall be "apm,xgene-edac-mc". +- regmap-pcp : Regmap of the PCP resource. +- regmap-csw : Regmap of the CSW resource. +- regmap-mcba : Regmap of the MCB-A resource. +- regmap-mcbb : Regmap of the MCB-B resource. +- reg : First resource shall be the MCU resource. +- interrupts : Interrupt-specifier for MCU error IRQ(s). + +The following section describes the L1/L2 DT node binding. + +- compatible : Shall be "apm,xgene-edac-pmd". +- regmap-pcp : Regmap of the PCP resource. +- regmap-efuse : Regmap of the PMD efuse resource. +- reg : First resource shall be the PMD resource. +- interrupts : Interrupt-specifier for PMD error IRQ(s). + +The following section describes the L3 DT node binding. + +- compatible : Shall be "apm,xgene-edac-l3". +- regmap-pcp : Regmap of the PCP resource. +- reg : First resource shall be the L3 resource. +- interrupts : Interrupt-specifier for L3 error IRQ(s). + +The following section describes the SoC DT node binding. + +- compatible : Shall be "apm,xgene-edac-soc"". +- regmap-pcp : Regmap of the PCP resource. +- reg : First resource shall be the SoC resource. + Second resource shall be the register bus resource. +- interrupts : Interrupt-specifier for SoC error IRQ(s). + +Example: + efuse: efuse at 1054a000 { + compatible = "syscon"; + reg = <0x0 0x1054a000 0x0 0x20>; + }; + + pcperror: pcperror 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 = <&pcperror>; + regmap-csw = <&csw>; + regmap-mcba = <&mcba>; + regmap-mcbb = <&mcbb>; + reg = <0x0 0x7e800000 0x0 0x1000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacpmd0: edacpmd0 at 7c000000 { + compatible = "apm,xgene-edac-pmd"; + regmap-pcp = <&pcperror>; + regmap-efuse = <&efuse>; + reg = <0x0 0x7c000000 0x0 0x200000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacl3: edacl3 at 7e600000 { + compatible = "apm,xgene-edac-l3"; + regmap-pcp = <&pcperror>; + reg = <0x0 0x7e600000 0x0 0x1000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacsoc: edacsoc at 7e930000 { + compatible = "apm,xgene-edac-soc"; + regmap-pcp = <&pcperror>; + reg = <0x0 0x7e930000 0x0 0x1000>, + <0x0 0x7e000000 0x0 0x1000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>, + <0x0 0x27 0x4>; + }; -- 1.7.1 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver 2015-04-28 22:10 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Loc Ho @ 2015-04-28 22:10 ` Loc Ho 2015-04-28 22:10 ` [PATCH 5/5] arm64: Add APM X-Gene SoC EDAC DTS entries Loc Ho ` (2 more replies) 2015-04-29 16:47 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Rob Herring 1 sibling, 3 replies; 44+ messages in thread From: Loc Ho @ 2015-04-28 22:10 UTC (permalink / raw) To: linux-arm-kernel This patch adds support for the APM X-Gene SoC EDAC driver. Signed-off-by: Feng Kan <fkan@apm.com> Signed-off-by: Loc Ho <lho@apm.com> --- drivers/edac/Kconfig | 9 +- drivers/edac/Makefile | 1 + drivers/edac/xgene_edac.c | 2175 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 2184 insertions(+), 1 deletions(-) create mode 100644 drivers/edac/xgene_edac.c diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index cb59619..6289dd9 100644 --- a/drivers/edac/Kconfig +++ b/drivers/edac/Kconfig @@ -10,7 +10,7 @@ config EDAC_SUPPORT menuconfig EDAC bool "EDAC (Error Detection And Correction) reporting" depends on HAS_IOMEM - depends on X86 || PPC || TILE || ARM || EDAC_SUPPORT + depends on X86 || PPC || TILE || ARM || ARM64 || EDAC_SUPPORT help EDAC is designed to report errors in the core system. These are low-level errors that are reported in the CPU or @@ -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..28ef2a5 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.o diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c new file mode 100644 index 0000000..00a2547 --- /dev/null +++ b/drivers/edac/xgene_edac.c @@ -0,0 +1,2175 @@ +/* + * APM X-Gene SoC EDAC (error detection and correction) Module + * + * 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" + +static int edac_mc_idx; +static int edac_mc_active_mask; +static int edac_mc_registered_mask; +static DEFINE_MUTEX(xgene_edac_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_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_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, ®)) + 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, ®)) + 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, ®)) + 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; +} + +#ifdef CONFIG_OF +static struct of_device_id xgene_edac_mc_of_match[] = { + { .compatible = "apm,xgene-edac-mc" }, + {}, +}; +MODULE_DEVICE_TABLE(of, xgene_edac_of_match); +#endif + +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 = of_match_ptr(xgene_edac_mc_of_match), + }, +}; + +/* CPU L1/L2 error device */ +#define MAX_CPU_PER_PMD 2 +#define CPU_CSR_STRIDE 0x00100000 +#define CPU_L2C_PAGE 0x000D0000 +#define CPU_MEMERR_L2C_PAGE 0x000E0000 +#define CPU_MEMERR_CPU_PAGE 0x000F0000 + +#define MEMERR_CPU_ICFECR_PAGE_OFFSET 0x0000 +#define MEMERR_CPU_ICFESR_PAGE_OFFSET 0x0004 +#define MEMERR_CPU_ICFESR_ERRWAY_RD(src) (((src) & 0xFF000000) >> 24) +#define MEMERR_CPU_ICFESR_ERRINDEX_RD(src) (((src) & 0x003F0000) >> 16) +#define MEMERR_CPU_ICFESR_ERRINFO_RD(src) (((src) & 0x0000FF00) >> 8) +#define MEMERR_CPU_ICFESR_ERRTYPE_RD(src) (((src) & 0x00000070) >> 4) +#define MEMERR_CPU_ICFESR_MULTCERR_MASK BIT(2) +#define MEMERR_CPU_ICFESR_CERR_MASK BIT(0) +#define MEMERR_CPU_LSUESR_PAGE_OFFSET 0x000c +#define MEMERR_CPU_LSUESR_ERRWAY_RD(src) (((src) & 0xFF000000) >> 24) +#define MEMERR_CPU_LSUESR_ERRINDEX_RD(src) (((src) & 0x003F0000) >> 16) +#define MEMERR_CPU_LSUESR_ERRINFO_RD(src) (((src) & 0x0000FF00) >> 8) +#define MEMERR_CPU_LSUESR_ERRTYPE_RD(src) (((src) & 0x00000070) >> 4) +#define MEMERR_CPU_LSUESR_MULTCERR_MASK BIT(2) +#define MEMERR_CPU_LSUESR_CERR_MASK BIT(0) +#define MEMERR_CPU_LSUECR_PAGE_OFFSET 0x0008 +#define MEMERR_CPU_MMUECR_PAGE_OFFSET 0x0010 +#define MEMERR_CPU_MMUESR_PAGE_OFFSET 0x0014 +#define MEMERR_CPU_MMUESR_ERRWAY_RD(src) (((src) & 0xFF000000) >> 24) +#define MEMERR_CPU_MMUESR_ERRINDEX_RD(src) (((src) & 0x007F0000) >> 16) +#define MEMERR_CPU_MMUESR_ERRINFO_RD(src) (((src) & 0x0000FF00) >> 8) +#define MEMERR_CPU_MMUESR_ERRREQSTR_LSU_MASK BIT(7) +#define MEMERR_CPU_MMUESR_ERRTYPE_RD(src) (((src) & 0x00000070) >> 4) +#define MEMERR_CPU_MMUESR_MULTCERR_MASK BIT(2) +#define MEMERR_CPU_MMUESR_CERR_MASK BIT(0) +#define MEMERR_CPU_ICFESRA_PAGE_OFFSET 0x0804 +#define MEMERR_CPU_LSUESRA_PAGE_OFFSET 0x080c +#define MEMERR_CPU_MMUESRA_PAGE_OFFSET 0x0814 + +#define MEMERR_L2C_L2ECR_PAGE_OFFSET 0x0000 +#define MEMERR_L2C_L2ESR_PAGE_OFFSET 0x0004 +#define MEMERR_L2C_L2ESR_ERRSYN_RD(src) (((src) & 0xFF000000) >> 24) +#define MEMERR_L2C_L2ESR_ERRWAY_RD(src) (((src) & 0x00FC0000) >> 18) +#define MEMERR_L2C_L2ESR_ERRCPU_RD(src) (((src) & 0x00020000) >> 17) +#define MEMERR_L2C_L2ESR_ERRGROUP_RD(src) (((src) & 0x0000E000) >> 13) +#define MEMERR_L2C_L2ESR_ERRACTION_RD(src) (((src) & 0x00001C00) >> 10) +#define MEMERR_L2C_L2ESR_ERRTYPE_RD(src) (((src) & 0x00000300) >> 8) +#define MEMERR_L2C_L2ESR_MULTUCERR_MASK BIT(3) +#define MEMERR_L2C_L2ESR_MULTICERR_MASK BIT(2) +#define MEMERR_L2C_L2ESR_UCERR_MASK BIT(1) +#define MEMERR_L2C_L2ESR_ERR_MASK BIT(0) +#define MEMERR_L2C_L2EALR_PAGE_OFFSET 0x0008 +#define CPUX_L2C_L2RTOCR_PAGE_OFFSET 0x0010 +#define MEMERR_L2C_L2EAHR_PAGE_OFFSET 0x000c +#define CPUX_L2C_L2RTOSR_PAGE_OFFSET 0x0014 +#define MEMERR_L2C_L2RTOSR_MULTERR_MASK BIT(1) +#define MEMERR_L2C_L2RTOSR_ERR_MASK BIT(0) +#define CPUX_L2C_L2RTOALR_PAGE_OFFSET 0x0018 +#define CPUX_L2C_L2RTOAHR_PAGE_OFFSET 0x001c +#define MEMERR_L2C_L2ESRA_PAGE_OFFSET 0x0804 + +/* + * Processor Module Domain (PMD) context - Context for a pair of processsors. + * Each PMD consists of 2 CPUs and a shared L2 cache. Each CPU consists of + * its own L1 cache. + */ +struct xgene_edac_pmd_ctx { + char *name; + struct regmap *pcp_map; /* PCP CSR for reading error interrupt reg */ + void __iomem *pmd_csr; /* PMD CSR for reading L1/L2 error reg */ + int pmd; /* Identify the register in pcp_csr */ +}; + +static void xgene_edac_pmd_l1_check(struct edac_device_ctl_info *edac_dev, + int cpu_idx) +{ + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + void __iomem *pg_f; + u32 val; + + pg_f = ctx->pmd_csr + cpu_idx * CPU_CSR_STRIDE + CPU_MEMERR_CPU_PAGE; + + val = readl(pg_f + MEMERR_CPU_ICFESR_PAGE_OFFSET); + if (val) { + dev_err(edac_dev->dev, + "CPU%d L1 memory error ICF 0x%08X Way 0x%02X Index 0x%02X Info 0x%02X\n", + ctx->pmd * MAX_CPU_PER_PMD + cpu_idx, val, + MEMERR_CPU_ICFESR_ERRWAY_RD(val), + MEMERR_CPU_ICFESR_ERRINDEX_RD(val), + MEMERR_CPU_ICFESR_ERRINFO_RD(val)); + if (val & MEMERR_CPU_ICFESR_CERR_MASK) + dev_err(edac_dev->dev, + "One or more correctable error\n"); + if (val & MEMERR_CPU_ICFESR_MULTCERR_MASK) + dev_err(edac_dev->dev, "Multiple correctable error\n"); + switch (MEMERR_CPU_ICFESR_ERRTYPE_RD(val)) { + case 1: + dev_err(edac_dev->dev, "L1 TLB multiple hit\n"); + break; + case 2: + dev_err(edac_dev->dev, "Way select multiple hit\n"); + break; + case 3: + dev_err(edac_dev->dev, "Physical tag parity error\n"); + break; + case 4: + case 5: + dev_err(edac_dev->dev, "L1 data parity error\n"); + break; + case 6: + dev_err(edac_dev->dev, "L1 pre-decode parity error\n"); + break; + } + + /* Clear any HW errors */ + writel(val, pg_f + MEMERR_CPU_ICFESR_PAGE_OFFSET); + + if (val & (MEMERR_CPU_ICFESR_CERR_MASK | + MEMERR_CPU_ICFESR_MULTCERR_MASK)) + edac_device_handle_ce(edac_dev, 0, 0, + edac_dev->ctl_name); + } + + val = readl(pg_f + MEMERR_CPU_LSUESR_PAGE_OFFSET); + if (val) { + dev_err(edac_dev->dev, + "CPU%d memory error LSU 0x%08X Way 0x%02X Index 0x%02X Info 0x%02X\n", + ctx->pmd * MAX_CPU_PER_PMD + cpu_idx, val, + MEMERR_CPU_LSUESR_ERRWAY_RD(val), + MEMERR_CPU_LSUESR_ERRINDEX_RD(val), + MEMERR_CPU_LSUESR_ERRINFO_RD(val)); + if (val & MEMERR_CPU_LSUESR_CERR_MASK) + dev_err(edac_dev->dev, + "One or more correctable error\n"); + if (val & MEMERR_CPU_LSUESR_MULTCERR_MASK) + dev_err(edac_dev->dev, "Multiple correctable error\n"); + switch (MEMERR_CPU_LSUESR_ERRTYPE_RD(val)) { + case 0: + dev_err(edac_dev->dev, "Load tag error\n"); + break; + case 1: + dev_err(edac_dev->dev, "Load data error\n"); + break; + case 2: + dev_err(edac_dev->dev, "WSL multihit error\n"); + break; + case 3: + dev_err(edac_dev->dev, "Store tag error\n"); + break; + case 4: + dev_err(edac_dev->dev, + "DTB multihit from load pipeline error\n"); + break; + case 5: + dev_err(edac_dev->dev, + "DTB multihit from store pipeline error\n"); + break; + } + + /* Clear any HW errors */ + writel(val, pg_f + MEMERR_CPU_LSUESR_PAGE_OFFSET); + + if (val & (MEMERR_CPU_LSUESR_CERR_MASK | + MEMERR_CPU_LSUESR_MULTCERR_MASK)) + edac_device_handle_ce(edac_dev, 0, 0, + edac_dev->ctl_name); + } + + val = readl(pg_f + MEMERR_CPU_MMUESR_PAGE_OFFSET); + if (val) { + dev_err(edac_dev->dev, + "CPU%d memory error MMU 0x%08X Way 0x%02X Index 0x%02X Info 0x%02X %s\n", + ctx->pmd * MAX_CPU_PER_PMD + cpu_idx, val, + MEMERR_CPU_MMUESR_ERRWAY_RD(val), + MEMERR_CPU_MMUESR_ERRINDEX_RD(val), + MEMERR_CPU_MMUESR_ERRINFO_RD(val), + val & MEMERR_CPU_MMUESR_ERRREQSTR_LSU_MASK ? "LSU" : + "ICF"); + if (val & MEMERR_CPU_MMUESR_CERR_MASK) + dev_err(edac_dev->dev, + "One or more correctable error\n"); + if (val & MEMERR_CPU_MMUESR_MULTCERR_MASK) + dev_err(edac_dev->dev, "Multiple correctable error\n"); + switch (MEMERR_CPU_MMUESR_ERRTYPE_RD(val)) { + case 0: + dev_err(edac_dev->dev, "Stage 1 UTB hit error\n"); + break; + case 1: + dev_err(edac_dev->dev, "Stage 1 UTB miss error\n"); + break; + case 2: + dev_err(edac_dev->dev, "Stage 1 UTB allocate error\n"); + break; + case 3: + dev_err(edac_dev->dev, + "TMO operation single bank error\n"); + break; + case 4: + dev_err(edac_dev->dev, "Stage 2 UTB error\n"); + break; + case 5: + dev_err(edac_dev->dev, "Stage 2 UTB miss error\n"); + break; + case 6: + dev_err(edac_dev->dev, "Stage 2 UTB allocate error\n"); + break; + case 7: + dev_err(edac_dev->dev, + "TMO operation multiple bank error\n"); + break; + } + + /* Clear any HW errors */ + writel(val, pg_f + MEMERR_CPU_MMUESR_PAGE_OFFSET); + + edac_device_handle_ce(edac_dev, 0, 0, edac_dev->ctl_name); + } +} + +static void xgene_edac_pmd_l2_check(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + void __iomem *pg_d; + void __iomem *pg_e; + u32 val_hi; + u32 val_lo; + u32 val; + + /* Check L2 */ + pg_e = ctx->pmd_csr + CPU_MEMERR_L2C_PAGE; + val = readl(pg_e + MEMERR_L2C_L2ESR_PAGE_OFFSET); + if (val) { + val_lo = readl(pg_e + MEMERR_L2C_L2EALR_PAGE_OFFSET); + val_hi = readl(pg_e + MEMERR_L2C_L2EAHR_PAGE_OFFSET); + dev_err(edac_dev->dev, + "PMD%d memory error L2C L2ESR 0x%08X @ 0x%08X.%08X\n", + ctx->pmd, val, val_hi, val_lo); + dev_err(edac_dev->dev, + "ErrSyndrome 0x%02X ErrWay 0x%02X ErrCpu %d ErrGroup 0x%02X ErrAction 0x%02X\n", + MEMERR_L2C_L2ESR_ERRSYN_RD(val), + MEMERR_L2C_L2ESR_ERRWAY_RD(val), + MEMERR_L2C_L2ESR_ERRCPU_RD(val), + MEMERR_L2C_L2ESR_ERRGROUP_RD(val), + MEMERR_L2C_L2ESR_ERRACTION_RD(val)); + + if (val & MEMERR_L2C_L2ESR_ERR_MASK) + dev_err(edac_dev->dev, + "One or more correctable error\n"); + if (val & MEMERR_L2C_L2ESR_MULTICERR_MASK) + dev_err(edac_dev->dev, "Multiple correctable error\n"); + if (val & MEMERR_L2C_L2ESR_UCERR_MASK) + dev_err(edac_dev->dev, + "One or more uncorrectable error\n"); + if (val & MEMERR_L2C_L2ESR_MULTUCERR_MASK) + dev_err(edac_dev->dev, + "Multiple uncorrectable error\n"); + + switch (MEMERR_L2C_L2ESR_ERRTYPE_RD(val)) { + case 0: + dev_err(edac_dev->dev, "Outbound SDB parity error\n"); + break; + case 1: + dev_err(edac_dev->dev, "Inbound SDB parity error\n"); + break; + case 2: + dev_err(edac_dev->dev, "Tag ECC error\n"); + break; + case 3: + dev_err(edac_dev->dev, "Data ECC error\n"); + break; + } + + /* Clear any HW errors */ + writel(val, pg_e + MEMERR_L2C_L2ESR_PAGE_OFFSET); + + if (val & (MEMERR_L2C_L2ESR_ERR_MASK | + MEMERR_L2C_L2ESR_MULTICERR_MASK)) + edac_device_handle_ce(edac_dev, 0, 0, + edac_dev->ctl_name); + if (val & (MEMERR_L2C_L2ESR_UCERR_MASK | + MEMERR_L2C_L2ESR_MULTUCERR_MASK)) + edac_device_handle_ue(edac_dev, 0, 0, + edac_dev->ctl_name); + } + + /* Check if any memory request timed out on L2 cache */ + pg_d = ctx->pmd_csr + CPU_L2C_PAGE; + val = readl(pg_d + CPUX_L2C_L2RTOSR_PAGE_OFFSET); + if (val) { + val_lo = readl(pg_d + CPUX_L2C_L2RTOALR_PAGE_OFFSET); + val_hi = readl(pg_d + CPUX_L2C_L2RTOAHR_PAGE_OFFSET); + dev_err(edac_dev->dev, + "PMD%d L2C error L2C RTOSR 0x%08X @ 0x%08X.%08X\n", + ctx->pmd, val, val_hi, val_lo); + writel(val, pg_d + CPUX_L2C_L2RTOSR_PAGE_OFFSET); + } +} + +static void xgene_edac_pmd_check(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + unsigned int pcp_hp_stat; + int i; + + if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat)) + return; + if (!((PMD0_MERR_MASK << ctx->pmd) & pcp_hp_stat)) + return; + + /* Check CPU L1 error */ + for (i = 0; i < MAX_CPU_PER_PMD; i++) + xgene_edac_pmd_l1_check(edac_dev, i); + + /* Check CPU L2 error */ + xgene_edac_pmd_l2_check(edac_dev); +} + +static irqreturn_t xgene_edac_pmd_isr(int irq, void *dev_id) +{ + struct edac_device_ctl_info *edac_dev = dev_id; + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + unsigned int pcp_hp_stat; + + if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat)) + return IRQ_NONE; + if (!((PMD0_MERR_MASK << ctx->pmd) & pcp_hp_stat)) + return IRQ_NONE; + + xgene_edac_pmd_check(edac_dev); + + return IRQ_HANDLED; +} + +static void xgene_edac_pmd_cpu_hw_cfg(struct edac_device_ctl_info *edac_dev, + int cpu) +{ + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + void __iomem *pg_f = ctx->pmd_csr + cpu * CPU_CSR_STRIDE + + CPU_MEMERR_CPU_PAGE; + + /* + * Enable CPU memory error: + * MEMERR_CPU_ICFESRA, MEMERR_CPU_LSUESRA, and MEMERR_CPU_MMUESRA + */ + writel(0x00000301, pg_f + MEMERR_CPU_ICFECR_PAGE_OFFSET); + writel(0x00000301, pg_f + MEMERR_CPU_LSUECR_PAGE_OFFSET); + writel(0x00000101, pg_f + MEMERR_CPU_MMUECR_PAGE_OFFSET); +} + +static bool xgene_edac_pmd_l2c_version1(void) +{ + /* Check all chips with PMD L2C version 1 HW */ + #define REVIDR_MINOR_REV(revidr) ((revidr) & 0x00000007) + + switch (MIDR_VARIANT(read_cpuid_id())) { + case 0: + switch (MIDR_REVISION(read_cpuid_id())) { + case 0: + + switch (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1))) { + case 1: + case 2: + return true; + }; + break; + case 1: + if (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1)) == 1) + return true; + break; + } + break; + case 1: + switch (MIDR_REVISION(read_cpuid_id())) { + case 0: + switch (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1))) { + case 1: + return true; + }; + break; + case 1: + switch (REVIDR_MINOR_REV(read_cpuid(REVIDR_EL1))) { + case 1: + case 0: + return true; + }; + break; + } + break; + } + + return false; +} + +static void xgene_edac_pmd_hw_cfg(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + void __iomem *pg_d = ctx->pmd_csr + CPU_L2C_PAGE; + void __iomem *pg_e = ctx->pmd_csr + CPU_MEMERR_L2C_PAGE; + + /* Enable PMD memory error - MEMERR_L2C_L2ECR and L2C_L2RTOCR */ + writel(0x00000703, pg_e + MEMERR_L2C_L2ECR_PAGE_OFFSET); + /* Configure L2C HW request time out feature if supported */ + if (!xgene_edac_pmd_l2c_version1()) + writel(0x00000119, pg_d + CPUX_L2C_L2RTOCR_PAGE_OFFSET); +} + +static void xgene_edac_pmd_hw_ctl(struct edac_device_ctl_info *edac_dev, + bool enable) +{ + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + unsigned int val; + int i; + + /* Enable PMD error interrupt */ + if (edac_dev->op_state == OP_RUNNING_INTERRUPT) { + mutex_lock(&xgene_edac_lock); + + if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val)) + goto err; + if (enable) + val &= ~(PMD0_MERR_MASK << ctx->pmd); + else + val |= PMD0_MERR_MASK << ctx->pmd; + if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val)) + goto err; + +err: + mutex_unlock(&xgene_edac_lock); + } + + if (enable) { + xgene_edac_pmd_hw_cfg(edac_dev); + + /* Two CPUs per a PMD */ + for (i = 0; i < MAX_CPU_PER_PMD; i++) + xgene_edac_pmd_cpu_hw_cfg(edac_dev, i); + } +} + +#ifdef CONFIG_EDAC_DEBUG +static ssize_t xgene_edac_pmd_l1_inject_ctrl_write(struct file *file, + const char __user *data, + size_t count, loff_t *ppos) +{ + struct edac_device_ctl_info *edac_dev = file->private_data; + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + void __iomem *cpux_pg_f; + int i; + + for (i = 0; i < MAX_CPU_PER_PMD; i++) { + cpux_pg_f = ctx->pmd_csr + i * CPU_CSR_STRIDE + + CPU_MEMERR_CPU_PAGE; + + writel(MEMERR_CPU_ICFESR_MULTCERR_MASK | + MEMERR_CPU_ICFESR_CERR_MASK, + cpux_pg_f + MEMERR_CPU_ICFESRA_PAGE_OFFSET); + writel(MEMERR_CPU_LSUESR_MULTCERR_MASK | + MEMERR_CPU_LSUESR_CERR_MASK, + cpux_pg_f + MEMERR_CPU_LSUESRA_PAGE_OFFSET); + writel(MEMERR_CPU_MMUESR_MULTCERR_MASK | + MEMERR_CPU_MMUESR_CERR_MASK, + cpux_pg_f + MEMERR_CPU_MMUESRA_PAGE_OFFSET); + } + return count; +} + +static ssize_t xgene_edac_pmd_l2_inject_ctrl_write(struct file *file, + const char __user *data, + size_t count, loff_t *ppos) +{ + struct edac_device_ctl_info *edac_dev = file->private_data; + struct xgene_edac_pmd_ctx *ctx = edac_dev->pvt_info; + void __iomem *pg_e = ctx->pmd_csr + CPU_MEMERR_L2C_PAGE; + + writel(MEMERR_L2C_L2ESR_MULTUCERR_MASK | + MEMERR_L2C_L2ESR_MULTICERR_MASK | + MEMERR_L2C_L2ESR_UCERR_MASK | + MEMERR_L2C_L2ESR_ERR_MASK, + pg_e + MEMERR_L2C_L2ESRA_PAGE_OFFSET); + return count; +} + +static const struct file_operations xgene_edac_pmd_debug_inject_fops[] = { + { + .open = simple_open, + .write = xgene_edac_pmd_l1_inject_ctrl_write, + .llseek = generic_file_llseek, }, + { + .open = simple_open, + .write = xgene_edac_pmd_l2_inject_ctrl_write, + .llseek = generic_file_llseek, }, + { } +}; + +static void xgene_edac_pmd_create_debugfs_nodes( + struct edac_device_ctl_info *edac_dev) +{ + struct dentry *edac_debugfs; + + /* + * Todo: Switch to common EDAC debug file system for edac device + * when available. + */ + edac_debugfs = debugfs_create_dir(edac_dev->dev->kobj.name, NULL); + if (!edac_debugfs) + return; + + debugfs_create_file("l1_inject_ctrl", S_IWUSR, edac_debugfs, edac_dev, + &xgene_edac_pmd_debug_inject_fops[0]); + debugfs_create_file("l2_inject_ctrl", S_IWUSR, edac_debugfs, edac_dev, + &xgene_edac_pmd_debug_inject_fops[1]); +} +#else +static void xgene_edac_pmd_create_debugfs_nodes( + struct edac_device_ctl_info *edac_dev) +{ +} +#endif + +static int xgene_edac_pmd_available(u32 efuse, int pmd) +{ + return (efuse & (1 << pmd)) ? 0 : 1; +} + +static int xgene_edac_pmd_probe(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edac_dev; + struct xgene_edac_pmd_ctx *ctx; + char edac_name[10]; + struct resource *res; + struct regmap *efuse_map; + int pmd; + int rc = 0; + u32 val; + + if (!devres_open_group(&pdev->dev, xgene_edac_pmd_probe, GFP_KERNEL)) + return -ENOMEM; + + /* Find the PMD number from its address */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res || resource_size(res) <= 0) { + rc = -ENODEV; + goto err_group; + } + pmd = ((res->start >> 20) & 0x1E) >> 1; + + /* Determine if this PMD is disabled */ + efuse_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, + "regmap-efuse"); + if (IS_ERR(efuse_map)) { + dev_err(&pdev->dev, "unable to get syscon regmap efuse\n"); + rc = PTR_ERR(efuse_map); + goto err_group; + } + if ((rc = regmap_read(efuse_map, 0, &val))) + goto err_group; + if (!xgene_edac_pmd_available(val, pmd)) { + rc = -ENODEV; + goto err_group; + } + + sprintf(edac_name, "l2c%d", pmd); + edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx), + edac_name, 1, "l2c", 1, 2, NULL, + 0, edac_device_alloc_index()); + if (!edac_dev) { + rc = -ENOMEM; + goto err_group; + } + + ctx = edac_dev->pvt_info; + ctx->name = "xgene_pmd_err"; + ctx->pmd = pmd; + edac_dev->dev = &pdev->dev; + dev_set_drvdata(edac_dev->dev, edac_dev); + edac_dev->ctl_name = ctx->name; + edac_dev->dev_name = ctx->name; + edac_dev->mod_name = EDAC_MOD_STR; + + ctx->pcp_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, + "regmap-pcp"); + if (IS_ERR(ctx->pcp_map)) { + dev_err(&pdev->dev, "unable to get syscon pcp map\n"); + rc = PTR_ERR(ctx->pcp_map); + goto err_free; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ctx->pmd_csr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ctx->pmd_csr)) { + dev_err(&pdev->dev, + "devm_ioremap failed for PMD resource address\n"); + rc = PTR_ERR(ctx->pmd_csr); + goto err_free; + } + + if (edac_op_state == EDAC_OPSTATE_POLL) + edac_dev->edac_check = xgene_edac_pmd_check; + + xgene_edac_pmd_create_debugfs_nodes(edac_dev); + + rc = edac_device_add_device(edac_dev); + if (rc > 0) { + dev_err(&pdev->dev, "edac_device_add_device failed\n"); + rc = -ENOMEM; + goto err_free; + } + + if (edac_op_state == EDAC_OPSTATE_INT) { + int irq = platform_get_irq(pdev, 0); + + 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_pmd_isr, IRQF_SHARED, + dev_name(&pdev->dev), edac_dev); + if (rc) { + dev_err(&pdev->dev, "Could not request IRQ %d\n", irq); + goto err_del; + } + edac_dev->op_state = OP_RUNNING_INTERRUPT; + } + + xgene_edac_pmd_hw_ctl(edac_dev, 1); + + devres_remove_group(&pdev->dev, xgene_edac_pmd_probe); + + dev_info(&pdev->dev, "X-Gene EDAC PMD registered\n"); + return 0; + +err_del: + edac_device_del_device(&pdev->dev); +err_free: + edac_device_free_ctl_info(edac_dev); +err_group: + devres_release_group(&pdev->dev, xgene_edac_pmd_probe); + return rc; +} + +static int xgene_edac_pmd_remove(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edac_dev = dev_get_drvdata(&pdev->dev); + + xgene_edac_pmd_hw_ctl(edac_dev, 0); + edac_device_del_device(&pdev->dev); + edac_device_free_ctl_info(edac_dev); + return 0; +} + +#ifdef CONFIG_OF +static struct of_device_id xgene_edac_pmd_of_match[] = { + { .compatible = "apm,xgene-edac-pmd" }, + {}, +}; +MODULE_DEVICE_TABLE(of, xgene_edac_pmd_of_match); +#endif + +static struct platform_driver xgene_edac_pmd_driver = { + .probe = xgene_edac_pmd_probe, + .remove = xgene_edac_pmd_remove, + .driver = { + .name = "xgene-edac-pmd", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(xgene_edac_pmd_of_match), + }, +}; + +/* L3 Error device */ +#define L3C_ESR (0x0A * 4) +#define L3C_ESR_DATATAG_MASK BIT(9) +#define L3C_ESR_MULTIHIT_MASK BIT(8) +#define L3C_ESR_UCEVICT_MASK BIT(6) +#define L3C_ESR_MULTIUCERR_MASK BIT(5) +#define L3C_ESR_MULTICERR_MASK BIT(4) +#define L3C_ESR_UCERR_MASK BIT(3) +#define L3C_ESR_CERR_MASK BIT(2) +#define L3C_ESR_UCERRINTR_MASK BIT(1) +#define L3C_ESR_CERRINTR_MASK BIT(0) +#define L3C_ECR (0x0B * 4) +#define L3C_ECR_UCINTREN BIT(3) +#define L3C_ECR_CINTREN BIT(2) +#define L3C_UCERREN BIT(1) +#define L3C_CERREN BIT(0) +#define L3C_ELR (0x0C * 4) +#define L3C_ELR_ERRSYN(src) ((src & 0xFF800000) >> 23) +#define L3C_ELR_ERRWAY(src) ((src & 0x007E0000) >> 17) +#define L3C_ELR_AGENTID(src) ((src & 0x0001E000) >> 13) +#define L3C_ELR_ERRGRP(src) ((src & 0x00000F00) >> 8) +#define L3C_ELR_OPTYPE(src) ((src & 0x000000F0) >> 4) +#define L3C_ELR_PADDRHIGH(src) (src & 0x0000000F) +#define L3C_AELR (0x0D * 4) +#define L3C_BELR (0x0E * 4) +#define L3C_BELR_BANK(src) (src & 0x0000000F) + +struct xgene_edac_dev_ctx { + char *name; + int edac_idx; + struct regmap *pcp_map; + void __iomem *dev_csr; + void __iomem *bus_csr; +}; + +static bool xgene_edac_l3_version1(void) +{ + return MIDR_VARIANT(read_cpuid_id()) == 0 ? true : false; +} + +static void xgene_edac_l3_check(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + u32 l3cesr; + u32 l3celr; + u32 l3caelr; + u32 l3cbelr; + + l3cesr = readl(ctx->dev_csr + L3C_ESR); + if (!(l3cesr & (L3C_ESR_UCERR_MASK | L3C_ESR_CERR_MASK))) + return; + + if (l3cesr & L3C_ESR_UCERR_MASK) + dev_err(edac_dev->dev, "L3C uncorrectable error\n"); + if (l3cesr & L3C_ESR_CERR_MASK) + dev_warn(edac_dev->dev, "L3C correctable error\n"); + + l3celr = readl(ctx->dev_csr + L3C_ELR); + l3caelr = readl(ctx->dev_csr + L3C_AELR); + l3cbelr = readl(ctx->dev_csr + L3C_BELR); + if (l3cesr & L3C_ESR_MULTIHIT_MASK) + dev_err(edac_dev->dev, "L3C multiple hit error\n"); + if (l3cesr & L3C_ESR_UCEVICT_MASK) + dev_err(edac_dev->dev, + "L3C dropped eviction of line with error\n"); + if (l3cesr & L3C_ESR_MULTIUCERR_MASK) + dev_err(edac_dev->dev, "L3C multiple uncorrectable error\n"); + if (l3cesr & L3C_ESR_DATATAG_MASK) { + if (!xgene_edac_l3_version1()) { + dev_err(edac_dev->dev, "L3C data error\n"); + goto no_tag_syndrome; + } + dev_err(edac_dev->dev, + "L3C data error syndrome 0x%X group 0x%X\n", + L3C_ELR_ERRSYN(l3celr), L3C_ELR_ERRGRP(l3celr)); + } else { + dev_err(edac_dev->dev, + "L3C tag error syndrome 0x%X Way of Tag 0x%X Agent ID 0x%X Operation type 0x%X\n", + L3C_ELR_ERRSYN(l3celr), L3C_ELR_ERRWAY(l3celr), + L3C_ELR_AGENTID(l3celr), L3C_ELR_OPTYPE(l3celr)); + } + + /* + * NOTE: Address [41:38] in L3C_ELR_PADDRHIGH(l3celr). + * Address [37:6] in l3caelr. Lower 6 bits are zero. + */ + dev_err(edac_dev->dev, "L3C error address 0x%08X.%08X bank %d\n", + L3C_ELR_PADDRHIGH(l3celr) << 6 | (l3caelr >> 26), + (l3caelr & 0x3FFFFFFF) << 6, L3C_BELR_BANK(l3cbelr)); +no_tag_syndrome: + dev_err(edac_dev->dev, + "L3C error status register value 0x%X\n", l3cesr); + + /* Clear L3C error interrupt */ + writel(0, ctx->dev_csr + L3C_ESR); + + if (l3cesr & L3C_ESR_CERR_MASK) + edac_device_handle_ce(edac_dev, 0, 0, edac_dev->ctl_name); + if (l3cesr & L3C_ESR_UCERR_MASK) + edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name); +} + +static irqreturn_t xgene_edac_l3_isr(int irq, void *dev_id) +{ + struct edac_device_ctl_info *edac_dev = dev_id; + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + u32 l3cesr; + + l3cesr = readl(ctx->dev_csr + L3C_ESR); + if (!(l3cesr & (L3C_ESR_UCERRINTR_MASK | L3C_ESR_CERRINTR_MASK))) + return IRQ_NONE; + + xgene_edac_l3_check(edac_dev); + + return IRQ_HANDLED; +} + +static void xgene_edac_l3_hw_ctl(struct edac_device_ctl_info *edac_dev, + bool enable) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + u32 val; + + val = readl(ctx->dev_csr + L3C_ECR); + val |= L3C_UCERREN | L3C_CERREN; + /* On disable, we just disable interrupt but keep error enabled */ + if (edac_dev->op_state == OP_RUNNING_INTERRUPT) { + if (enable) + val |= L3C_ECR_UCINTREN | L3C_ECR_CINTREN; + else + val &= ~(L3C_ECR_UCINTREN | L3C_ECR_CINTREN); + } + writel(val, ctx->dev_csr + L3C_ECR); + + mutex_lock(&xgene_edac_lock); + + if (edac_dev->op_state == OP_RUNNING_INTERRUPT) { + /* Enable L3C error top level interrupt */ + if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val)) + goto err; + if (enable) + val &= ~L3C_UNCORR_ERR_MASK; + else + val |= L3C_UNCORR_ERR_MASK; + if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val)) + goto err; + if (regmap_read(ctx->pcp_map, PCPLPERRINTMSK, &val)) + goto err; + if (enable) + val &= ~L3C_CORR_ERR_MASK; + else + val |= L3C_CORR_ERR_MASK; + if (regmap_write(ctx->pcp_map, PCPLPERRINTMSK, val)) + goto err; + } + +err: + mutex_unlock(&xgene_edac_lock); +} + +#ifdef CONFIG_EDAC_DEBUG +static ssize_t xgene_edac_l3_inject_ctrl_write(struct file *file, + const char __user *data, + size_t count, loff_t *ppos) +{ + struct edac_device_ctl_info *edac_dev = file->private_data; + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + + writel(L3C_ESR_UCERR_MASK | L3C_ESR_CERR_MASK | + L3C_ESR_MULTIUCERR_MASK | L3C_ESR_MULTICERR_MASK, + ctx->dev_csr + L3C_ESR); + return count; +} + +static const struct file_operations xgene_edac_l3_debug_inject_fops = { + .open = simple_open, + .write = xgene_edac_l3_inject_ctrl_write, + .llseek = generic_file_llseek, +}; + +static void xgene_edac_l3_create_debugfs_node( + struct edac_device_ctl_info *edac_dev) +{ + struct dentry *edac_debugfs; + + /* + * Todo: Switch to common EDAC debug file system for edac device + * when available. + */ + edac_debugfs = debugfs_create_dir(edac_dev->dev->kobj.name, NULL); + if (!edac_debugfs) + return; + + debugfs_create_file("inject_ctrl", S_IWUSR, edac_debugfs, edac_dev, + &xgene_edac_l3_debug_inject_fops); +} +#else +static void xgene_edac_l3_create_debugfs_node( + struct edac_device_ctl_info *edac_dev) +{ +} +#endif + +static int xgene_edac_l3_probe(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edac_dev; + struct xgene_edac_dev_ctx *ctx; + struct resource *res; + int rc = 0; + + if (!devres_open_group(&pdev->dev, xgene_edac_l3_probe, GFP_KERNEL)) + return -ENOMEM; + + edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx), + "l3c", 1, "l3c", 1, 0, NULL, 0, + edac_device_alloc_index()); + if (!edac_dev) { + rc = -ENOMEM; + goto err; + } + + ctx = edac_dev->pvt_info; + ctx->name = "xgene_l3_err"; + edac_dev->dev = &pdev->dev; + dev_set_drvdata(edac_dev->dev, edac_dev); + edac_dev->ctl_name = ctx->name; + edac_dev->dev_name = ctx->name; + edac_dev->mod_name = EDAC_MOD_STR; + + ctx->pcp_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, + "regmap-pcp"); + if (IS_ERR(ctx->pcp_map)) { + dev_err(&pdev->dev, "unable to get syscon regmap pcp\n"); + rc = PTR_ERR(ctx->pcp_map); + goto err1; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ctx->dev_csr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ctx->dev_csr)) { + dev_err(&pdev->dev, "no L3 resource address\n"); + rc = PTR_ERR(ctx->dev_csr); + goto err1; + } + + if (edac_op_state == EDAC_OPSTATE_POLL) + edac_dev->edac_check = xgene_edac_l3_check; + + xgene_edac_l3_create_debugfs_node(edac_dev); + + rc = edac_device_add_device(edac_dev); + if (rc > 0) { + dev_err(&pdev->dev, "edac_device_add_device failed\n"); + rc = -ENOMEM; + goto err1; + } + + 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 err2; + } + rc = devm_request_irq(&pdev->dev, irq, + xgene_edac_l3_isr, IRQF_SHARED, + dev_name(&pdev->dev), edac_dev); + if (rc) { + dev_err(&pdev->dev, + "Could not request IRQ %d\n", irq); + goto err2; + } + } + edac_dev->op_state = OP_RUNNING_INTERRUPT; + } + + xgene_edac_l3_hw_ctl(edac_dev, true); + + devres_remove_group(&pdev->dev, xgene_edac_l3_probe); + + dev_info(&pdev->dev, "X-Gene EDAC L3 registered\n"); + return 0; + +err2: + edac_device_del_device(&pdev->dev); +err1: + edac_device_free_ctl_info(edac_dev); +err: + devres_release_group(&pdev->dev, xgene_edac_l3_probe); + return rc; +} + +static int xgene_edac_l3_remove(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edac_dev = dev_get_drvdata(&pdev->dev); + + xgene_edac_l3_hw_ctl(edac_dev, false); + edac_device_del_device(&pdev->dev); + edac_device_free_ctl_info(edac_dev); + return 0; +} + +#ifdef CONFIG_OF +static struct of_device_id xgene_edac_l3_of_match[] = { + { .compatible = "apm,xgene-edac-l3" }, + {}, +}; +MODULE_DEVICE_TABLE(of, xgene_edac_l3_of_match); +#endif + +static struct platform_driver xgene_edac_l3_driver = { + .probe = xgene_edac_l3_probe, + .remove = xgene_edac_l3_remove, + .driver = { + .name = "xgene-edac-l3", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(xgene_edac_l3_of_match), + }, +}; + +/* SoC Error device */ +#define IOBAXIS0TRANSERRINTSTS 0x0000 +#define IOBAXIS0_M_ILLEGAL_ACCESS_MASK BIT(1) +#define IOBAXIS0_ILLEGAL_ACCESS_MASK BIT(0) +#define IOBAXIS0TRANSERRINTMSK 0x0004 +#define IOBAXIS0TRANSERRREQINFOL 0x0008 +#define IOBAXIS0TRANSERRREQINFOH 0x000c +#define REQTYPE_RD(src) (((src) & BIT(0))) +#define ERRADDRH_RD(src) (((src) & 0xffc00000) >> 22) +#define IOBAXIS1TRANSERRINTSTS 0x0010 +#define IOBAXIS1TRANSERRINTMSK 0x0014 +#define IOBAXIS1TRANSERRREQINFOL 0x0018 +#define IOBAXIS1TRANSERRREQINFOH 0x001c +#define IOBPATRANSERRINTSTS 0x0020 +#define IOBPA_M_REQIDRAM_CORRUPT_MASK BIT(7) +#define IOBPA_REQIDRAM_CORRUPT_MASK BIT(6) +#define IOBPA_M_TRANS_CORRUPT_MASK BIT(5) +#define IOBPA_TRANS_CORRUPT_MASK BIT(4) +#define IOBPA_M_WDATA_CORRUPT_MASK BIT(3) +#define IOBPA_WDATA_CORRUPT_MASK BIT(2) +#define IOBPA_M_RDATA_CORRUPT_MASK BIT(1) +#define IOBPA_RDATA_CORRUPT_MASK BIT(0) +#define IOBBATRANSERRINTSTS 0x0030 +#define M_ILLEGAL_ACCESS_MASK 0x00008000 +#define ILLEGAL_ACCESS_MASK 0x00004000 +#define M_WIDRAM_CORRUPT_MASK 0x00002000 +#define WIDRAM_CORRUPT_MASK BIT(12) +#define M_RIDRAM_CORRUPT_MASK BIT(11) +#define RIDRAM_CORRUPT_MASK BIT(10) +#define M_TRANS_CORRUPT_MASK BIT(9) +#define TRANS_CORRUPT_MASK BIT(8) +#define M_WDATA_CORRUPT_MASK BIT(7) +#define WDATA_CORRUPT_MASK BIT(6) +#define M_RBM_POISONED_REQ_MASK BIT(5) +#define RBM_POISONED_REQ_MASK BIT(4) +#define M_XGIC_POISONED_REQ_MASK BIT(3) +#define XGIC_POISONED_REQ_MASK BIT(2) +#define M_WRERR_RESP_MASK BIT(1) +#define WRERR_RESP_MASK BIT(0) +#define IOBBATRANSERRREQINFOL 0x0038 +#define IOBBATRANSERRREQINFOH 0x003c +#define REQTYPE_F2_RD(src) (((src) & BIT(0))) +#define ERRADDRH_F2_RD(src) (((src) & 0xffc00000) >> 22) +#define IOBBATRANSERRCSWREQID 0x0040 +#define XGICTRANSERRINTSTS 0x0050 +#define M_WR_ACCESS_ERR_MASK BIT(3) +#define WR_ACCESS_ERR_MASK BIT(2) +#define M_RD_ACCESS_ERR_MASK BIT(1) +#define RD_ACCESS_ERR_MASK BIT(0) +#define XGICTRANSERRINTMSK 0x0054 +#define XGICTRANSERRREQINFO 0x0058 +#define REQTYPE_MASK 0x04000000 +#define ERRADDR_RD(src) ((src) & 0x03ffffff) +#define GLBL_ERR_STS 0x0800 +#define MDED_ERR_MASK BIT(3) +#define DED_ERR_MASK BIT(2) +#define MSEC_ERR_MASK BIT(1) +#define SEC_ERR_MASK BIT(0) +#define GLBL_SEC_ERRL 0x0810 +#define GLBL_SEC_ERRH 0x0818 +#define GLBL_MSEC_ERRL 0x0820 +#define GLBL_MSEC_ERRH 0x0828 +#define GLBL_DED_ERRL 0x0830 +#define GLBL_DED_ERRLMASK 0x0834 +#define GLBL_DED_ERRH 0x0838 +#define GLBL_DED_ERRHMASK 0x083c +#define GLBL_MDED_ERRL 0x0840 +#define GLBL_MDED_ERRLMASK 0x0844 +#define GLBL_MDED_ERRH 0x0848 +#define GLBL_MDED_ERRHMASK 0x084c + +/* IO Bus Registers */ +#define RBCSR 0x0000 +#define STICKYERR_MASK BIT(0) +#define RBEIR 0x0008 +#define AGENT_OFFLINE_ERR_MASK BIT(30) +#define UNIMPL_RBPAGE_ERR_MASK BIT(29) +#define WORD_ALIGNED_ERR_MASK BIT(28) +#define PAGE_ACCESS_ERR_MASK BIT(27) +#define WRITE_ACCESS_MASK BIT(26) +#define RBERRADDR_RD(src) ((src) & 0x03FFFFFF) + +static void xgene_edac_iob_gic_report(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + u32 err_addr_lo; + u32 err_addr_hi; + u32 reg; + u32 info; + + /* GIC transaction error interrupt */ + reg = readl(ctx->dev_csr + XGICTRANSERRINTSTS); + if (reg) { + dev_err(edac_dev->dev, "XGIC transaction error\n"); + if (reg & RD_ACCESS_ERR_MASK) + dev_err(edac_dev->dev, "XGIC read size error\n"); + if (reg & M_RD_ACCESS_ERR_MASK) + dev_err(edac_dev->dev, + "Multiple XGIC read size error\n"); + if (reg & WR_ACCESS_ERR_MASK) + dev_err(edac_dev->dev, "XGIC write size error\n"); + if (reg & M_WR_ACCESS_ERR_MASK) + dev_err(edac_dev->dev, + "Multiple XGIC write size error\n"); + info = readl(ctx->dev_csr + XGICTRANSERRREQINFO); + dev_err(edac_dev->dev, "XGIC %s access @ 0x%08X (0x%08X)\n", + info & REQTYPE_MASK ? "read" : "write", + ERRADDR_RD(info), info); + writel(reg, ctx->dev_csr + XGICTRANSERRINTSTS); + } + + /* IOB memory error */ + reg = readl(ctx->dev_csr + GLBL_ERR_STS); + if (reg) { + if (reg & SEC_ERR_MASK) { + err_addr_lo = readl(ctx->dev_csr + GLBL_SEC_ERRL); + err_addr_hi = readl(ctx->dev_csr + GLBL_SEC_ERRH); + dev_err(edac_dev->dev, + "IOB single-bit correctable memory at 0x%08X.%08X error\n", + err_addr_lo, err_addr_hi); + writel(err_addr_lo, ctx->dev_csr + GLBL_SEC_ERRL); + writel(err_addr_hi, ctx->dev_csr + GLBL_SEC_ERRH); + } + if (reg & MSEC_ERR_MASK) { + err_addr_lo = readl(ctx->dev_csr + GLBL_MSEC_ERRL); + err_addr_hi = readl(ctx->dev_csr + GLBL_MSEC_ERRH); + dev_err(edac_dev->dev, + "IOB multiple single-bit correctable memory at 0x%08X.%08X error\n", + err_addr_lo, err_addr_hi); + writel(err_addr_lo, ctx->dev_csr + GLBL_MSEC_ERRL); + writel(err_addr_hi, ctx->dev_csr + GLBL_MSEC_ERRH); + } + if (reg & (SEC_ERR_MASK | MSEC_ERR_MASK)) + edac_device_handle_ce(edac_dev, 0, 0, + edac_dev->ctl_name); + + if (reg & DED_ERR_MASK) { + err_addr_lo = readl(ctx->dev_csr + GLBL_DED_ERRL); + err_addr_hi = readl(ctx->dev_csr + GLBL_DED_ERRH); + dev_err(edac_dev->dev, + "IOB double-bit uncorrectable memory at 0x%08X.%08X error\n", + err_addr_lo, err_addr_hi); + writel(err_addr_lo, ctx->dev_csr + GLBL_DED_ERRL); + writel(err_addr_hi, ctx->dev_csr + GLBL_DED_ERRH); + } + if (reg & MDED_ERR_MASK) { + err_addr_lo = readl(ctx->dev_csr + GLBL_MDED_ERRL); + err_addr_hi = readl(ctx->dev_csr + GLBL_MDED_ERRH); + dev_err(edac_dev->dev, + "Multiple IOB double-bit uncorrectable memory at 0x%08X.%08X error\n", + err_addr_lo, err_addr_hi); + writel(err_addr_lo, ctx->dev_csr + GLBL_MDED_ERRL); + writel(err_addr_hi, ctx->dev_csr + GLBL_MDED_ERRH); + } + if (reg & (DED_ERR_MASK | MDED_ERR_MASK)) + edac_device_handle_ue(edac_dev, 0, 0, + edac_dev->ctl_name); + } +} + +static void xgene_edac_rb_report(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + u32 err_addr_lo; + u32 err_addr_hi; + u32 reg; + + /* + * Check RB acess errors + * 1. Out of range + * 2. Un-implemented page + * 3. Un-aligned access + * 4. Offline slave IP + */ + reg = readl(ctx->bus_csr + RBCSR); + if (reg & STICKYERR_MASK) { + bool write; + u32 address; + + dev_err(edac_dev->dev, "IOB bus access error(s)\n"); + reg = readl(ctx->bus_csr + RBEIR); + write = reg & WRITE_ACCESS_MASK ? 1 : 0; + address = RBERRADDR_RD(reg); + if (reg & AGENT_OFFLINE_ERR_MASK) + dev_err(edac_dev->dev, + "IOB bus %s access to offline agent error\n", + write ? "write" : "read"); + if (reg & UNIMPL_RBPAGE_ERR_MASK) + dev_err(edac_dev->dev, + "IOB bus %s access to unimplemented page error\n", + write ? "write" : "read"); + if (reg & WORD_ALIGNED_ERR_MASK) + dev_err(edac_dev->dev, + "IOB bus %s word aligned access error\n", + write ? "write" : "read"); + if (reg & PAGE_ACCESS_ERR_MASK) + dev_err(edac_dev->dev, + "IOB bus %s to page out of range access error\n", + write ? "write" : "read"); + writel(0x0, ctx->bus_csr + RBEIR); + writel(0x0, ctx->bus_csr + RBCSR); + } + + /* IOB Bridge agent transaction error interrupt */ + reg = readl(ctx->dev_csr + IOBBATRANSERRINTSTS); + if (!reg) + return; + + dev_err(edac_dev->dev, "IOB bridge agent (BA) transaction error\n"); + if (reg & WRERR_RESP_MASK) + dev_err(edac_dev->dev, "IOB BA write response error\n"); + if (reg & M_WRERR_RESP_MASK) + dev_err(edac_dev->dev, + "Multiple IOB BA write response error\n"); + if (reg & XGIC_POISONED_REQ_MASK) + dev_err(edac_dev->dev, "IOB BA XGIC poisoned write error\n"); + if (reg & M_XGIC_POISONED_REQ_MASK) + dev_err(edac_dev->dev, + "Multiple IOB BA XGIC poisoned write error\n"); + if (reg & RBM_POISONED_REQ_MASK) + dev_err(edac_dev->dev, "IOB BA RBM poisoned write error\n"); + if (reg & M_RBM_POISONED_REQ_MASK) + dev_err(edac_dev->dev, + "Multiple IOB BA RBM poisoned write error\n"); + if (reg & WDATA_CORRUPT_MASK) + dev_err(edac_dev->dev, "IOB BA write error\n"); + if (reg & M_WDATA_CORRUPT_MASK) + dev_err(edac_dev->dev, "Multiple IOB BA write error\n"); + if (reg & TRANS_CORRUPT_MASK) + dev_err(edac_dev->dev, "IOB BA transaction error\n"); + if (reg & M_TRANS_CORRUPT_MASK) + dev_err(edac_dev->dev, "Multiple IOB BA transaction error\n"); + if (reg & RIDRAM_CORRUPT_MASK) + dev_err(edac_dev->dev, + "IOB BA RDIDRAM read transaction ID error\n"); + if (reg & M_RIDRAM_CORRUPT_MASK) + dev_err(edac_dev->dev, + "Multiple IOB BA RDIDRAM read transaction ID error\n"); + if (reg & WIDRAM_CORRUPT_MASK) + dev_err(edac_dev->dev, + "IOB BA RDIDRAM write transaction ID error\n"); + if (reg & M_WIDRAM_CORRUPT_MASK) + dev_err(edac_dev->dev, + "Multiple IOB BA RDIDRAM write transaction ID error\n"); + if (reg & ILLEGAL_ACCESS_MASK) + dev_err(edac_dev->dev, + "IOB BA XGIC/RB illegal access error\n"); + if (reg & M_ILLEGAL_ACCESS_MASK) + dev_err(edac_dev->dev, + "Multiple IOB BA XGIC/RB illegal access error\n"); + + err_addr_lo = readl(ctx->dev_csr + IOBBATRANSERRREQINFOL); + err_addr_hi = readl(ctx->dev_csr + IOBBATRANSERRREQINFOH); + dev_err(edac_dev->dev, "IOB BA %s access at 0x%02X.%08X (0x%08X)\n", + REQTYPE_F2_RD(err_addr_hi) ? "read" : "write", + ERRADDRH_F2_RD(err_addr_hi), err_addr_lo, err_addr_hi); + if (reg & WRERR_RESP_MASK) + dev_err(edac_dev->dev, "IOB BA requestor ID 0x%08X\n", + readl(ctx->dev_csr + IOBBATRANSERRCSWREQID)); + writel(reg, ctx->dev_csr + IOBBATRANSERRINTSTS); +} + +static void xgene_edac_pa_report(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + u32 err_addr_lo; + u32 err_addr_hi; + u32 reg; + + /* IOB Processing agent transaction error interrupt */ + reg = readl(ctx->dev_csr + IOBPATRANSERRINTSTS); + if (reg) { + dev_err(edac_dev->dev, + "IOB procesing agent (PA) transaction error\n"); + if (reg & IOBPA_RDATA_CORRUPT_MASK) + dev_err(edac_dev->dev, "IOB PA read data RAM error\n"); + if (reg & IOBPA_M_RDATA_CORRUPT_MASK) + dev_err(edac_dev->dev, + "Mutilple IOB PA read data RAM error\n"); + if (reg & IOBPA_WDATA_CORRUPT_MASK) + dev_err(edac_dev->dev, + "IOB PA write data RAM error\n"); + if (reg & IOBPA_M_WDATA_CORRUPT_MASK) + dev_err(edac_dev->dev, + "Mutilple IOB PA write data RAM error\n"); + if (reg & IOBPA_TRANS_CORRUPT_MASK) + dev_err(edac_dev->dev, "IOB PA transaction error\n"); + if (reg & IOBPA_M_TRANS_CORRUPT_MASK) + dev_err(edac_dev->dev, + "Mutilple IOB PA transaction error\n"); + if (reg & IOBPA_REQIDRAM_CORRUPT_MASK) + dev_err(edac_dev->dev, + "IOB PA transaction ID RAM error\n"); + if (reg & IOBPA_M_REQIDRAM_CORRUPT_MASK) + dev_err(edac_dev->dev, + "Multiple IOB PA transaction ID RAM error\n"); + writel(reg, ctx->dev_csr + IOBPATRANSERRINTSTS); + } + + /* IOB AXI0 Error */ + reg = readl(ctx->dev_csr + IOBAXIS0TRANSERRINTSTS); + if (reg) { + err_addr_lo = readl(ctx->dev_csr + IOBAXIS0TRANSERRREQINFOL); + err_addr_hi = readl(ctx->dev_csr + IOBAXIS0TRANSERRREQINFOH); + dev_err(edac_dev->dev, + "%sAXI slave 0 illegal %s access @ 0x%02X.%08X (0x%08X)\n", + reg & IOBAXIS0_M_ILLEGAL_ACCESS_MASK ? "Multiple " : "", + REQTYPE_RD(err_addr_hi) ? "read" : "write", + ERRADDRH_RD(err_addr_hi), err_addr_lo, err_addr_hi); + writel(reg, ctx->dev_csr + IOBAXIS0TRANSERRINTSTS); + } + + /* IOB AXI1 Error */ + reg = readl(ctx->dev_csr + IOBAXIS1TRANSERRINTSTS); + if (reg) { + err_addr_lo = readl(ctx->dev_csr + IOBAXIS1TRANSERRREQINFOL); + err_addr_hi = readl(ctx->dev_csr + IOBAXIS1TRANSERRREQINFOH); + dev_err(edac_dev->dev, + "%sAXI slave 1 illegal %s access @ 0x%02X.%08X (0x%08X)\n", + reg & IOBAXIS0_M_ILLEGAL_ACCESS_MASK ? "Multiple " : "", + REQTYPE_RD(err_addr_hi) ? "read" : "write", + ERRADDRH_RD(err_addr_hi), err_addr_lo, err_addr_hi); + writel(reg, ctx->dev_csr + IOBAXIS1TRANSERRINTSTS); + } +} + +static void xgene_edac_soc_check(struct edac_device_ctl_info *edac_dev) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + static const char * const mem_err_ip[] = { + "10GbE0", + "10GbE1", + "Security", + "SATA45", + "SATA23/ETH23", + "SATA01/ETH01", + "USB1", + "USB0", + "QML", + "QM0", + "QM1 (XGbE01)", + "PCIE4", + "PCIE3", + "PCIE2", + "PCIE1", + "PCIE0", + "CTX Manager", + "OCM", + "1GbE", + "CLE", + "AHBC", + "PktDMA", + "GFC", + "MSLIM", + "10GbE2", + "10GbE3", + "QM2 (XGbE23)", + "IOB", + "unknown", + "unknown", + "unknown", + "unknown", + }; + unsigned int pcp_hp_stat; + unsigned int pcp_lp_stat; + unsigned int reg; + int i; + + if (regmap_read(ctx->pcp_map, PCPHPERRINTSTS, &pcp_hp_stat)) + return; + if (regmap_read(ctx->pcp_map, PCPLPERRINTSTS, &pcp_lp_stat)) + return; + if (regmap_read(ctx->pcp_map, MEMERRINTSTS, ®)) + return; + if (!((pcp_hp_stat & (IOB_PA_ERR_MASK | IOB_BA_ERR_MASK | + IOB_XGIC_ERR_MASK | IOB_RB_ERR_MASK)) || + (pcp_lp_stat & CSW_SWITCH_TRACE_ERR_MASK) || reg)) + return; + + if (pcp_hp_stat & IOB_XGIC_ERR_MASK) + xgene_edac_iob_gic_report(edac_dev); + + if (pcp_hp_stat & (IOB_RB_ERR_MASK | IOB_BA_ERR_MASK)) + xgene_edac_rb_report(edac_dev); + + if (pcp_hp_stat & IOB_PA_ERR_MASK) + xgene_edac_pa_report(edac_dev); + + if (pcp_lp_stat & CSW_SWITCH_TRACE_ERR_MASK) { + dev_info(edac_dev->dev, + "CSW switch trace correctable memory parity error\n"); + edac_device_handle_ce(edac_dev, 0, 0, edac_dev->ctl_name); + } + + for (i = 0; i < 31; i++) { + if (reg & (1 << i)) { + dev_err(edac_dev->dev, "%s memory parity error\n", + mem_err_ip[i]); + edac_device_handle_ue(edac_dev, 0, 0, + edac_dev->ctl_name); + } + } +} + +static irqreturn_t xgene_edac_soc_isr(int irq, void *dev_id) +{ + struct edac_device_ctl_info *edac_dev = dev_id; + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + unsigned int pcp_hp_stat; + unsigned int pcp_lp_stat; + unsigned int reg; + + 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 (regmap_read(ctx->pcp_map, MEMERRINTSTS, ®)) + return IRQ_NONE; + if (!((pcp_hp_stat & (IOB_PA_ERR_MASK | IOB_BA_ERR_MASK | + IOB_XGIC_ERR_MASK | IOB_RB_ERR_MASK)) || + (pcp_lp_stat & CSW_SWITCH_TRACE_ERR_MASK) || reg)) + return IRQ_NONE; + + xgene_edac_soc_check(edac_dev); + + return IRQ_HANDLED; +} + +static void xgene_edac_soc_hw_ctl(struct edac_device_ctl_info *edac_dev, + bool enable) +{ + struct xgene_edac_dev_ctx *ctx = edac_dev->pvt_info; + unsigned int val; + + /* Enable SoC IP error interrupt */ + if (edac_dev->op_state == OP_RUNNING_INTERRUPT) { + mutex_lock(&xgene_edac_lock); + + if (regmap_read(ctx->pcp_map, PCPHPERRINTMSK, &val)) + goto err; + if (enable) + val &= ~(IOB_PA_ERR_MASK | IOB_BA_ERR_MASK | + IOB_XGIC_ERR_MASK | IOB_RB_ERR_MASK); + else + val |= IOB_PA_ERR_MASK | IOB_BA_ERR_MASK | + IOB_XGIC_ERR_MASK | IOB_RB_ERR_MASK; + if (regmap_write(ctx->pcp_map, PCPHPERRINTMSK, val)) + goto err; + if (regmap_read(ctx->pcp_map, PCPLPERRINTMSK, &val)) + goto err; + if (enable) + val &= ~CSW_SWITCH_TRACE_ERR_MASK; + else + val |= CSW_SWITCH_TRACE_ERR_MASK; + if (regmap_write(ctx->pcp_map, PCPLPERRINTMSK, val)) + goto err; + +err: + mutex_unlock(&xgene_edac_lock); + + writel(enable ? 0x0 : 0xFFFFFFFF, + ctx->dev_csr + IOBAXIS0TRANSERRINTMSK); + writel(enable ? 0x0 : 0xFFFFFFFF, + ctx->dev_csr + IOBAXIS1TRANSERRINTMSK); + writel(enable ? 0x0 : 0xFFFFFFFF, + ctx->dev_csr + XGICTRANSERRINTMSK); + + regmap_write(ctx->pcp_map, MEMERRINTMSK, + enable ? 0x0 : 0xFFFFFFFF); + } +} + +static int xgene_edac_soc_probe(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edac_dev; + struct xgene_edac_dev_ctx *ctx; + struct resource *res; + int rc = 0; + + if (!devres_open_group(&pdev->dev, xgene_edac_soc_probe, GFP_KERNEL)) + return -ENOMEM; + + edac_dev = edac_device_alloc_ctl_info(sizeof(*ctx), + "SOC", 1, "SOC", 1, 2, NULL, 0, + edac_device_alloc_index()); + if (!edac_dev) { + rc = -ENOMEM; + goto err; + } + + ctx = edac_dev->pvt_info; + ctx->name = "xgene_soc_err"; + edac_dev->dev = &pdev->dev; + dev_set_drvdata(edac_dev->dev, edac_dev); + edac_dev->ctl_name = ctx->name; + edac_dev->dev_name = ctx->name; + edac_dev->mod_name = EDAC_MOD_STR; + + ctx->pcp_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, + "regmap-pcp"); + if (IS_ERR(ctx->pcp_map)) { + dev_err(&pdev->dev, "unable to get syscon regmap pcp\n"); + rc = PTR_ERR(ctx->pcp_map); + goto err1; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ctx->dev_csr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ctx->dev_csr)) { + dev_err(&pdev->dev, "no SoC resource address\n"); + rc = PTR_ERR(ctx->dev_csr); + goto err1; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + ctx->bus_csr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ctx->bus_csr)) { + dev_err(&pdev->dev, "no SoC bus resource address\n"); + rc = PTR_ERR(ctx->bus_csr); + goto err1; + } + + if (edac_op_state == EDAC_OPSTATE_POLL) + edac_dev->edac_check = xgene_edac_soc_check; + + rc = edac_device_add_device(edac_dev); + if (rc > 0) { + dev_err(&pdev->dev, "edac_device_add_device failed\n"); + rc = -ENOMEM; + goto err1; + } + + if (edac_op_state == EDAC_OPSTATE_INT) { + int irq; + int i; + + /* + * Register for SoC un-correctable and correctable errors + */ + for (i = 0; i < 3; i++) { + irq = platform_get_irq(pdev, i); + if (irq < 0) { + dev_err(&pdev->dev, "No IRQ resource\n"); + rc = -EINVAL; + goto err2; + } + rc = devm_request_irq(&pdev->dev, irq, + xgene_edac_soc_isr, IRQF_SHARED, + dev_name(&pdev->dev), edac_dev); + if (rc) { + dev_err(&pdev->dev, + "Could not request IRQ %d\n", irq); + goto err2; + } + } + + edac_dev->op_state = OP_RUNNING_INTERRUPT; + } + + xgene_edac_soc_hw_ctl(edac_dev, true); + + devres_remove_group(&pdev->dev, xgene_edac_soc_probe); + + dev_info(&pdev->dev, "X-Gene EDAC SoC registered\n"); + return 0; + +err2: + edac_device_del_device(&pdev->dev); +err1: + edac_device_free_ctl_info(edac_dev); +err: + devres_release_group(&pdev->dev, xgene_edac_soc_probe); + return rc; +} + +static int xgene_edac_soc_remove(struct platform_device *pdev) +{ + struct edac_device_ctl_info *edac_dev = dev_get_drvdata(&pdev->dev); + + xgene_edac_soc_hw_ctl(edac_dev, false); + edac_device_del_device(&pdev->dev); + edac_device_free_ctl_info(edac_dev); + return 0; +} + +#ifdef CONFIG_OF +static struct of_device_id xgene_edac_soc_of_match[] = { + { .compatible = "apm,xgene-edac-soc" }, + {}, +}; +MODULE_DEVICE_TABLE(of, xgene_edac_soc_of_match); +#endif + +static struct platform_driver xgene_edac_soc_driver = { + .probe = xgene_edac_soc_probe, + .remove = xgene_edac_soc_remove, + .driver = { + .name = "xgene-edac-soc", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(xgene_edac_soc_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; + } + rc = platform_driver_register(&xgene_edac_pmd_driver); + if (rc) { + edac_printk(KERN_ERR, EDAC_MOD_STR, "PMD fails to register\n"); + goto reg_pmd_failed; + } + rc = platform_driver_register(&xgene_edac_l3_driver); + if (rc) { + edac_printk(KERN_ERR, EDAC_MOD_STR, "L3 fails to register\n"); + goto reg_l3_failed; + } + rc = platform_driver_register(&xgene_edac_soc_driver); + if (rc) { + edac_printk(KERN_ERR, EDAC_MOD_STR, "SoC fails to register\n"); + goto reg_soc_failed; + } + + return 0; + +reg_soc_failed: + platform_driver_unregister(&xgene_edac_l3_driver); + +reg_l3_failed: + platform_driver_unregister(&xgene_edac_pmd_driver); + +reg_pmd_failed: + platform_driver_unregister(&xgene_edac_mc_driver); + +reg_mc_failed: + return rc; +} +module_init(xgene_edac_init); + +static void __exit xgene_edac_exit(void) +{ + platform_driver_unregister(&xgene_edac_soc_driver); + platform_driver_unregister(&xgene_edac_l3_driver); + platform_driver_unregister(&xgene_edac_pmd_driver); + 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 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] 44+ messages in thread
* [PATCH 5/5] arm64: Add APM X-Gene SoC EDAC DTS entries 2015-04-28 22:10 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Loc Ho @ 2015-04-28 22:10 ` Loc Ho 2015-04-29 8:49 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Arnd Bergmann 2015-04-29 17:00 ` Rob Herring 2 siblings, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-04-28 22:10 UTC (permalink / raw) To: linux-arm-kernel This patch adds APM X-Gene SoC 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 | 123 ++++++++++++++++++++++++++++++++ 1 files changed, 123 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..7ccb289 100644 --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi @@ -374,6 +374,129 @@ }; }; + efuse: efuse at 1054a000 { + compatible = "syscon"; + reg = <0x0 0x1054a000 0x0 0x20>; + }; + + pcperror: pcperror 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 = <&pcperror>; + 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 = <&pcperror>; + 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 = <&pcperror>; + 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 = <&pcperror>; + regmap-csw = <&csw>; + regmap-mcba = <&mcba>; + regmap-mcbb = <&mcbb>; + reg = <0x0 0x7e8c0000 0x0 0x1000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacpmd0: edacpmd0 at 7c000000 { + compatible = "apm,xgene-edac-pmd"; + regmap-pcp = <&pcperror>; + regmap-efuse = <&efuse>; + reg = <0x0 0x7c000000 0x0 0x200000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacpmd1: edacpmd1 at 7c200000 { + compatible = "apm,xgene-edac-pmd"; + regmap-pcp = <&pcperror>; + regmap-efuse = <&efuse>; + reg = <0x0 0x7c200000 0x0 0x200000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacpmd2: edacpmd2 at 7c400000 { + compatible = "apm,xgene-edac-pmd"; + regmap-pcp = <&pcperror>; + regmap-efuse = <&efuse>; + reg = <0x0 0x7c400000 0x0 0x200000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacpmd3: edacpmd3 at 7c600000 { + compatible = "apm,xgene-edac-pmd"; + regmap-pcp = <&pcperror>; + regmap-efuse = <&efuse>; + reg = <0x0 0x7c600000 0x0 0x200000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacl3: edacl3 at 7e600000 { + compatible = "apm,xgene-edac-l3"; + regmap-pcp = <&pcperror>; + reg = <0x0 0x7e600000 0x0 0x1000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>; + }; + + edacsoc: edacsoc at 7e930000 { + compatible = "apm,xgene-edac-soc"; + regmap-pcp = <&pcperror>; + reg = <0x0 0x7e930000 0x0 0x1000>, + <0x0 0x7e000000 0x0 0x1000>; + interrupts = <0x0 0x20 0x4>, + <0x0 0x21 0x4>, + <0x0 0x27 0x4>; + }; + pcie0: pcie at 1f2b0000 { status = "disabled"; device_type = "pci"; -- 1.7.1 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver 2015-04-28 22:10 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Loc Ho 2015-04-28 22:10 ` [PATCH 5/5] arm64: Add APM X-Gene SoC EDAC DTS entries Loc Ho @ 2015-04-29 8:49 ` Arnd Bergmann 2015-04-29 16:57 ` Rob Herring 2015-04-29 17:00 ` Rob Herring 2 siblings, 1 reply; 44+ messages in thread From: Arnd Bergmann @ 2015-04-29 8:49 UTC (permalink / raw) To: linux-arm-kernel On Tuesday 28 April 2015 16:10:44 Loc Ho wrote: > + > + 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; > + } > + rc = platform_driver_register(&xgene_edac_pmd_driver); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MOD_STR, "PMD fails to register\n"); > + goto reg_pmd_failed; > + } > + rc = platform_driver_register(&xgene_edac_l3_driver); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MOD_STR, "L3 fails to register\n"); > + goto reg_l3_failed; > + } > + rc = platform_driver_register(&xgene_edac_soc_driver); > + if (rc) { > + edac_printk(KERN_ERR, EDAC_MOD_STR, "SoC fails to register\n"); > + goto reg_soc_failed; > + } > I had not looked at the driver before, but I have one comment now: I think this can be simplified to registering a single platform_driver that just matches all four compatible strings, with an appropriate data: +static struct of_device_id xgene_edac_of_match[] = { + { .compatible = "apm,xgene-edac-mcu", .data = &xgene_edac_mcu_data }, + { .compatible = "apm,xgene-edac-pmd", .data = &xgene_edac_pmd_data }, + { .compatible = "apm,xgene-edac-l3", .data = &xgene_edac_l3_data }, + { .compatible = "apm,xgene-edac-soc", .data = &xgene_edac_soc_data }, + {}, +}; +MODULE_DEVICE_TABLE(of, xgene_edac_soc_of_match); + +static struct platform_driver xgene_edac_soc_driver = { + .probe = xgene_edac_soc_probe, + .remove = xgene_edac_soc_remove, + .driver = { + .name = "xgene-edac-soc", + .owner = THIS_MODULE, + .of_match_table = xgene_edac_of_match, + }, +}; You can probably share most of the four probe and release functions as well, and put the differences into the data structure that is attached to the of_device_id entry. As a style-only comment, you can also remove the #ifdef CONFIG_OF and of_match_ptr(), because the driver depends on OF anyway. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver 2015-04-29 8:49 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Arnd Bergmann @ 2015-04-29 16:57 ` Rob Herring 2015-04-29 18:23 ` Arnd Bergmann 0 siblings, 1 reply; 44+ messages in thread From: Rob Herring @ 2015-04-29 16:57 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 3:49 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 28 April 2015 16:10:44 Loc Ho wrote: >> + >> + 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; >> + } >> + rc = platform_driver_register(&xgene_edac_pmd_driver); >> + if (rc) { >> + edac_printk(KERN_ERR, EDAC_MOD_STR, "PMD fails to register\n"); >> + goto reg_pmd_failed; >> + } >> + rc = platform_driver_register(&xgene_edac_l3_driver); >> + if (rc) { >> + edac_printk(KERN_ERR, EDAC_MOD_STR, "L3 fails to register\n"); >> + goto reg_l3_failed; >> + } >> + rc = platform_driver_register(&xgene_edac_soc_driver); >> + if (rc) { >> + edac_printk(KERN_ERR, EDAC_MOD_STR, "SoC fails to register\n"); >> + goto reg_soc_failed; >> + } >> > > I had not looked at the driver before, but I have one comment now: > > I think this can be simplified to registering a single platform_driver > that just matches all four compatible strings, with an appropriate data: > > +static struct of_device_id xgene_edac_of_match[] = { > + { .compatible = "apm,xgene-edac-mcu", .data = &xgene_edac_mcu_data }, > + { .compatible = "apm,xgene-edac-pmd", .data = &xgene_edac_pmd_data }, > + { .compatible = "apm,xgene-edac-l3", .data = &xgene_edac_l3_data }, > + { .compatible = "apm,xgene-edac-soc", .data = &xgene_edac_soc_data }, > + {}, > +}; What exactly is shared here to combine all this into one driver? I would argue these are all independent functions and should be separate drivers. Add some library functions if there are common parts. The bindings are still a bit broken IMO. Rob ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver 2015-04-29 16:57 ` Rob Herring @ 2015-04-29 18:23 ` Arnd Bergmann 0 siblings, 0 replies; 44+ messages in thread From: Arnd Bergmann @ 2015-04-29 18:23 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 29 April 2015 11:57:09 Rob Herring wrote: > >> > > > > I had not looked at the driver before, but I have one comment now: > > > > I think this can be simplified to registering a single platform_driver > > that just matches all four compatible strings, with an appropriate data: > > > > +static struct of_device_id xgene_edac_of_match[] = { > > + { .compatible = "apm,xgene-edac-mcu", .data = &xgene_edac_mcu_data }, > > + { .compatible = "apm,xgene-edac-pmd", .data = &xgene_edac_pmd_data }, > > + { .compatible = "apm,xgene-edac-l3", .data = &xgene_edac_l3_data }, > > + { .compatible = "apm,xgene-edac-soc", .data = &xgene_edac_soc_data }, > > + {}, > > +}; > > What exactly is shared here to combine all this into one driver? I > would argue these are all independent functions and should be separate > drivers. Add some library functions if there are common parts. Right, that is probably even better. I was mostly worried about having multiple platform_driver instances in a single module, which is a bit odd, and your approach would solve that in a simpler way. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver 2015-04-28 22:10 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Loc Ho 2015-04-28 22:10 ` [PATCH 5/5] arm64: Add APM X-Gene SoC EDAC DTS entries Loc Ho 2015-04-29 8:49 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Arnd Bergmann @ 2015-04-29 17:00 ` Rob Herring 2 siblings, 0 replies; 44+ messages in thread From: Rob Herring @ 2015-04-29 17:00 UTC (permalink / raw) To: linux-arm-kernel On Tue, Apr 28, 2015 at 5:10 PM, Loc Ho <lho@apm.com> wrote: > This patch adds support for the APM X-Gene SoC EDAC driver. > > Signed-off-by: Feng Kan <fkan@apm.com> > Signed-off-by: Loc Ho <lho@apm.com> > --- > drivers/edac/Kconfig | 9 +- > drivers/edac/Makefile | 1 + > drivers/edac/xgene_edac.c | 2175 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 2184 insertions(+), 1 deletions(-) > create mode 100644 drivers/edac/xgene_edac.c > > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index cb59619..6289dd9 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -10,7 +10,7 @@ config EDAC_SUPPORT > menuconfig EDAC > bool "EDAC (Error Detection And Correction) reporting" > depends on HAS_IOMEM > - depends on X86 || PPC || TILE || ARM || EDAC_SUPPORT > + depends on X86 || PPC || TILE || ARM || ARM64 || EDAC_SUPPORT ARM64 selects EDAC_SUPPORT, so you don't need this. Rob ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-28 22:10 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Loc Ho 2015-04-28 22:10 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Loc Ho @ 2015-04-29 16:47 ` Rob Herring 2015-04-29 21:33 ` Loc Ho 1 sibling, 1 reply; 44+ messages in thread From: Rob Herring @ 2015-04-29 16:47 UTC (permalink / raw) To: linux-arm-kernel On Tue, Apr 28, 2015 at 5:10 PM, Loc Ho <lho@apm.com> wrote: > 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 | 107 ++++++++++++++++++++ > 1 files changed, 107 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..548299a > --- /dev/null > +++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt > @@ -0,0 +1,107 @@ > +* APM X-Gene SoC EDAC nodes > + > +EDAC nodes are defined to describe on-chip error detection and correction. > +There are four types of EDAC: > + > + memory controller - Memory controller > + PMD (L1/L2) - Processor module unit (PMD) L1/L2 cache > + L3 - CPU L3 cache > + SoC - SoC IP such as SATA, Ethernet, and etc > + > +The following section describes the memory controller DT node binding. > + > +Required properties: > +- compatible : Shall be "apm,xgene-edac-mc". You are still describing what you want for EDAC driver not what the h/w looks like AFAICT. > +- regmap-pcp : Regmap of the PCP resource. > +- regmap-csw : Regmap of the CSW resource. > +- regmap-mcba : Regmap of the MCB-A resource. > +- regmap-mcbb : Regmap of the MCB-B resource. What are these blocks? Regardless, I doubt the linkage here belongs in DT. > +- reg : First resource shall be the MCU resource. > +- interrupts : Interrupt-specifier for MCU error IRQ(s). What is MCU? > + > +The following section describes the L1/L2 DT node binding. Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest. Rob > + > +- compatible : Shall be "apm,xgene-edac-pmd". > +- regmap-pcp : Regmap of the PCP resource. > +- regmap-efuse : Regmap of the PMD efuse resource. > +- reg : First resource shall be the PMD resource. > +- interrupts : Interrupt-specifier for PMD error IRQ(s). > + > +The following section describes the L3 DT node binding. > + > +- compatible : Shall be "apm,xgene-edac-l3". > +- regmap-pcp : Regmap of the PCP resource. > +- reg : First resource shall be the L3 resource. > +- interrupts : Interrupt-specifier for L3 error IRQ(s). > + > +The following section describes the SoC DT node binding. > + > +- compatible : Shall be "apm,xgene-edac-soc"". > +- regmap-pcp : Regmap of the PCP resource. > +- reg : First resource shall be the SoC resource. > + Second resource shall be the register bus resource. > +- interrupts : Interrupt-specifier for SoC error IRQ(s). > + > +Example: > + efuse: efuse at 1054a000 { > + compatible = "syscon"; > + reg = <0x0 0x1054a000 0x0 0x20>; > + }; > + > + pcperror: pcperror 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 = <&pcperror>; > + regmap-csw = <&csw>; > + regmap-mcba = <&mcba>; > + regmap-mcbb = <&mcbb>; > + reg = <0x0 0x7e800000 0x0 0x1000>; > + interrupts = <0x0 0x20 0x4>, > + <0x0 0x21 0x4>; > + }; > + > + edacpmd0: edacpmd0 at 7c000000 { > + compatible = "apm,xgene-edac-pmd"; > + regmap-pcp = <&pcperror>; > + regmap-efuse = <&efuse>; > + reg = <0x0 0x7c000000 0x0 0x200000>; > + interrupts = <0x0 0x20 0x4>, > + <0x0 0x21 0x4>; > + }; > + > + edacl3: edacl3 at 7e600000 { > + compatible = "apm,xgene-edac-l3"; > + regmap-pcp = <&pcperror>; > + reg = <0x0 0x7e600000 0x0 0x1000>; > + interrupts = <0x0 0x20 0x4>, > + <0x0 0x21 0x4>; > + }; > + > + edacsoc: edacsoc at 7e930000 { > + compatible = "apm,xgene-edac-soc"; > + regmap-pcp = <&pcperror>; > + reg = <0x0 0x7e930000 0x0 0x1000>, > + <0x0 0x7e000000 0x0 0x1000>; > + interrupts = <0x0 0x20 0x4>, > + <0x0 0x21 0x4>, > + <0x0 0x27 0x4>; > + }; > -- > 1.7.1 > ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 16:47 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Rob Herring @ 2015-04-29 21:33 ` Loc Ho 2015-04-29 21:49 ` Borislav Petkov 0 siblings, 1 reply; 44+ messages in thread From: Loc Ho @ 2015-04-29 21:33 UTC (permalink / raw) To: linux-arm-kernel Hi Rob, >> .../devicetree/bindings/edac/apm-xgene-edac.txt | 107 ++++++++++++++++++++ >> 1 files changed, 107 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..548299a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/edac/apm-xgene-edac.txt >> @@ -0,0 +1,107 @@ >> +* APM X-Gene SoC EDAC nodes >> + >> +EDAC nodes are defined to describe on-chip error detection and correction. >> +There are four types of EDAC: >> + >> + memory controller - Memory controller >> + PMD (L1/L2) - Processor module unit (PMD) L1/L2 cache >> + L3 - CPU L3 cache >> + SoC - SoC IP such as SATA, Ethernet, and etc >> + >> +The following section describes the memory controller DT node binding. >> + >> +Required properties: >> +- compatible : Shall be "apm,xgene-edac-mc". > > You are still describing what you want for EDAC driver not what the > h/w looks like AFAICT. > We have 4 memory controller and two memory bridges. Each memory bridges can handle two memory controllers. >> +- regmap-pcp : Regmap of the PCP resource. >> +- regmap-csw : Regmap of the CSW resource. >> +- regmap-mcba : Regmap of the MCB-A resource. >> +- regmap-mcbb : Regmap of the MCB-B resource. > > What are these blocks? Regardless, I doubt the linkage here belongs in DT. The PCP provide the top level interrupt for various status. From that, one can determine the type of error based on various status bits - memory error, CPU cache parity memory errors (L1, L2, and L3), and SoC memory parity errors. The CSW contains the status to indicate whether an memory bridge is active or not. The MCB A and MCB B are used to determine an memory controller is active or not. In summary, we have: CSW, MCB A, and MCB B are used to determine active memory controller and ignore in-active memory controller. PCP and the memory controller (reg property) are used to determine the memory error type and type of error (correctable or un-conrrectable). If one still needs more clarification, let me know. > >> +- reg : First resource shall be the MCU resource. >> +- interrupts : Interrupt-specifier for MCU error IRQ(s). > > What is MCU? MCU represents the memory controller unit. > >> + >> +The following section describes the L1/L2 DT node binding. > > Similar comments for the rest. I would define memory controller > bindings and EDAC driver, then worry about the rest. Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 21:33 ` Loc Ho @ 2015-04-29 21:49 ` Borislav Petkov 2015-04-29 21:56 ` Loc Ho 0 siblings, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-29 21:49 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 02:33:28PM -0700, Loc Ho wrote: > > Similar comments for the rest. I would define memory controller > > bindings and EDAC driver, then worry about the rest. > > Okay.. As comment in following emails, I will break up the driver into > multiple drivers and focus only on the memory controller driver first. Please no multiple EDAC drivers. Or do you mean something else here? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 21:49 ` Borislav Petkov @ 2015-04-29 21:56 ` Loc Ho 2015-04-29 22:08 ` Borislav Petkov 2015-04-29 22:43 ` Rob Herring 0 siblings, 2 replies; 44+ messages in thread From: Loc Ho @ 2015-04-29 21:56 UTC (permalink / raw) To: linux-arm-kernel Hi, >> > Similar comments for the rest. I would define memory controller >> > bindings and EDAC driver, then worry about the rest. >> >> Okay.. As comment in following emails, I will break up the driver into >> multiple drivers and focus only on the memory controller driver first. > > Please no multiple EDAC drivers. Or do you mean something else here? We will have the following: xgene-edac-mc.c xgene-edac-pmd.c xgene-edac-l3.c xgene-edac-soc.c Or what would you suggest. There are the following HW: 1. 4 DDR controller with one shared top level interrupt, two shared memory bridges 2. 4 CPU's domain with one shared top level interrupt, shared L2 for two CPU's, and individual L1 3. 1 L3 with one shared top level interrupt 4. One SoC memory parity block with one shared top level interupt -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 21:56 ` Loc Ho @ 2015-04-29 22:08 ` Borislav Petkov 2015-04-29 22:20 ` Loc Ho 2015-04-29 23:02 ` Rob Herring 2015-04-29 22:43 ` Rob Herring 1 sibling, 2 replies; 44+ messages in thread From: Borislav Petkov @ 2015-04-29 22:08 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 02:56:25PM -0700, Loc Ho wrote: > Hi, > > >> > Similar comments for the rest. I would define memory controller > >> > bindings and EDAC driver, then worry about the rest. > >> > >> Okay.. As comment in following emails, I will break up the driver into > >> multiple drivers and focus only on the memory controller driver first. > > > > Please no multiple EDAC drivers. Or do you mean something else here? > > We will have the following: > > xgene-edac-mc.c > xgene-edac-pmd.c > xgene-edac-l3.c > xgene-edac-soc.c > > Or what would you suggest. xgene-edac.c This granularity is insane. On x86 we have single EDAC drivers for multiple CPU families and platforms. You can start carving out stuff when this is absolutely needed, i.e. when an IP block is in multiple hw incarnations. And even then I'd be sceptical and would really want to know whether it is even worth it to have an EDAC module for it. So let's be conservative please. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 22:08 ` Borislav Petkov @ 2015-04-29 22:20 ` Loc Ho 2015-04-29 23:02 ` Rob Herring 1 sibling, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-04-29 22:20 UTC (permalink / raw) To: linux-arm-kernel Hi Boris, >> >> > Similar comments for the rest. I would define memory controller >> >> > bindings and EDAC driver, then worry about the rest. >> >> >> >> Okay.. As comment in following emails, I will break up the driver into >> >> multiple drivers and focus only on the memory controller driver first. >> > >> > Please no multiple EDAC drivers. Or do you mean something else here? >> >> We will have the following: >> >> xgene-edac-mc.c >> xgene-edac-pmd.c >> xgene-edac-l3.c >> xgene-edac-soc.c >> >> Or what would you suggest. > > xgene-edac.c > > This granularity is insane. On x86 we have single EDAC drivers for > multiple CPU families and platforms. > > You can start carving out stuff when this is absolutely needed, i.e. > when an IP block is in multiple hw incarnations. And even then I'd be > sceptical and would really want to know whether it is even worth it to > have an EDAC module for it. > > So let's be conservative please. For now, let me just deal with the DDR module. Rob, Did I answered all your questions regarding the EDAC DT binding for memory controller? I would like to get this sorted out before I post another version. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 22:08 ` Borislav Petkov 2015-04-29 22:20 ` Loc Ho @ 2015-04-29 23:02 ` Rob Herring 2015-04-30 8:20 ` Borislav Petkov 1 sibling, 1 reply; 44+ messages in thread From: Rob Herring @ 2015-04-29 23:02 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 5:08 PM, Borislav Petkov <bp@alien8.de> wrote: > On Wed, Apr 29, 2015 at 02:56:25PM -0700, Loc Ho wrote: >> Hi, >> >> >> > Similar comments for the rest. I would define memory controller >> >> > bindings and EDAC driver, then worry about the rest. >> >> >> >> Okay.. As comment in following emails, I will break up the driver into >> >> multiple drivers and focus only on the memory controller driver first. >> > >> > Please no multiple EDAC drivers. Or do you mean something else here? >> >> We will have the following: >> >> xgene-edac-mc.c >> xgene-edac-pmd.c >> xgene-edac-l3.c >> xgene-edac-soc.c >> >> Or what would you suggest. > > xgene-edac.c > > This granularity is insane. On x86 we have single EDAC drivers for > multiple CPU families and platforms. It may make sense on x86 as there are only 2 vendors, some level of architecture definition at least within a vendor, some commonality among generations, and some amount of abstraction by ACPI. For the most part none of that applies to ARM based systems (although that is changing). > You can start carving out stuff when this is absolutely needed, i.e. > when an IP block is in multiple hw incarnations. And even then I'd be > sceptical and would really want to know whether it is even worth it to > have an EDAC module for it. One platform device corresponds to 1 DT node which corresponds to 1 h/w block. This is a well defined pattern throughout the kernel. There are cases of grouping, but those are places where we need coordination between h/w blocks such as audio and display and that is still multiple platform devices with a single parent device. If you want this all in 1 file or 1 module that is fine. I think that it is silly to group otherwise independent things together and generally not what we do anywhere else in the kernel. They all likely have different capabilities and control mechanisms. Rob ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 23:02 ` Rob Herring @ 2015-04-30 8:20 ` Borislav Petkov 2015-04-30 8:31 ` Arnd Bergmann 0 siblings, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 8:20 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 06:02:14PM -0500, Rob Herring wrote: > I think that it is silly to group otherwise independent things > together and generally not what we do anywhere else in the kernel. > They all likely have different capabilities and control mechanisms. So let's look at the other extremity here: the moment someone releases yet another "independent" IP block with some RAS functionality, same someone will create yet another <vendor>_edac_<ip_block> driver. How many independent IP blocks are there with RAS functionality? 10, 20, 100...? That number is most likely growing, I'd bet. Oh, and then there'll probably be functionality which is needed by two IP blocks so it needs to be shared. So we either copy'paste stuff or create a lib only for that functionality... Even worse, what if two EDAC drivers for two IP blocks would need to talk to each other. That'll be fun. Or there'll be a v2 of the IP block which has almost the same functionality but no 100% - just a *little* different. So then we go create <vendor>_edac_<ip_block-v2> driver. Yuck! What I would prefer is to concentrate all vendor-specific RAS functionality in one single driver. Shared functionality is then taken care of automagically with all the synergies (I hate that word!) involved and no unnecessary too finer-grained splitting. In that case, we would only have to enable loading more than one EDAC drivers on a system which has different RAS IP blocks. Now *that* is a much cleaner solution IMO which will keep the sanity in EDAC-land above 0. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 8:20 ` Borislav Petkov @ 2015-04-30 8:31 ` Arnd Bergmann 2015-04-30 8:45 ` Borislav Petkov 0 siblings, 1 reply; 44+ messages in thread From: Arnd Bergmann @ 2015-04-30 8:31 UTC (permalink / raw) To: linux-arm-kernel On Thursday 30 April 2015 10:20:51 Borislav Petkov wrote: > On Wed, Apr 29, 2015 at 06:02:14PM -0500, Rob Herring wrote: > > I think that it is silly to group otherwise independent things > > together and generally not what we do anywhere else in the kernel. > > They all likely have different capabilities and control mechanisms. > > So let's look at the other extremity here: the moment someone releases > yet another "independent" IP block with some RAS functionality, same > someone will create yet another <vendor>_edac_<ip_block> driver. How > many independent IP blocks are there with RAS functionality? 10, 20, > 100...? That number is most likely growing, I'd bet. That should be a safe assumption. > Oh, and then there'll probably be functionality which is needed by two > IP blocks so it needs to be shared. So we either copy'paste stuff or > create a lib only for that functionality... > > Even worse, what if two EDAC drivers for two IP blocks would need to > talk to each other. That'll be fun. > > Or there'll be a v2 of the IP block which has almost the same > functionality but no 100% - just a *little* different. So then we go > create <vendor>_edac_<ip_block-v2> driver. Yuck! > > What I would prefer is to concentrate all vendor-specific RAS > functionality in one single driver. Shared functionality is then taken > care of automagically with all the synergies (I hate that word!) > involved and no unnecessary too finer-grained splitting. > > In that case, we would only have to enable loading more than one EDAC > drivers on a system which has different RAS IP blocks. Now *that* is a > much cleaner solution IMO which will keep the sanity in EDAC-land above > 0. The problem with your approach is that a lot of these blocks are not vendor specific. You will probably see a server chip that contains a memory controller from DesignWare, a cache controller from ARM, and some other device from the chip vendor, and each of them comes with EDAC support. Then you get three other vendors that have various combinations of the same memory controller, cache controller and other EDAC devices. Not all of these chips would have ARM CPU cores, some may be e.g. MIPS or PowerPC. This is very much what we see in all other subsystems (timers, irqchips, gpio, ...) and the answer is always to have one driver per device type, and have that driver handle all the variations of that device, but not group devices into one driver just because they happen to be on the same chip for one vendor who is merely integrating them. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 8:31 ` Arnd Bergmann @ 2015-04-30 8:45 ` Borislav Petkov 2015-04-30 9:01 ` Arnd Bergmann 0 siblings, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 8:45 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 10:31:12AM +0200, Arnd Bergmann wrote: > The problem with your approach is that a lot of these blocks are not > vendor specific. You will probably see a server chip that contains a > memory controller from DesignWare, a cache controller from ARM, and > some other device from the chip vendor, and each of them comes with > EDAC support. Then you get three other vendors that have various > combinations of the same memory controller, cache controller and > other EDAC devices. Not all of these chips would have ARM CPU cores, > some may be e.g. MIPS or PowerPC. I don't mean system-vendor specific but IP-block vendor specific. So doing a designware-edac, arm-edac, apm-edac and so on... Also, I really want for people thinking of writing EDAC drivers to think hard before doing so. Whether it is even worth to expose *every* RAS functionality in some driver. And to consider that exposing it might cause more trouble than benefit. We've had that experience on x86 with reporting innocuous DRAM ECC errors which were corrected by the hardware. People weren't even reading the error message and running around wanting to RMA their processors because something in dmesg screamed "Hardware Error". And now APEI does the firmware-first thing where it gets to look at the error first and then decide to report it up to the OS or not. Which is a good thing because in most cases it unnecessarily upsets the user. So to get back on track: I think having the IP-block vendor stuff in one EDAC module would make this whole heterogeneity much more manageable. On that system above, we will load - if I can count correctly - 6 EDAC drivers anyway. But the EDAC pile would remain sane. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 8:45 ` Borislav Petkov @ 2015-04-30 9:01 ` Arnd Bergmann 2015-04-30 9:41 ` Borislav Petkov 0 siblings, 1 reply; 44+ messages in thread From: Arnd Bergmann @ 2015-04-30 9:01 UTC (permalink / raw) To: linux-arm-kernel On Thursday 30 April 2015 10:45:50 Borislav Petkov wrote: > On Thu, Apr 30, 2015 at 10:31:12AM +0200, Arnd Bergmann wrote: > > The problem with your approach is that a lot of these blocks are not > > vendor specific. You will probably see a server chip that contains a > > memory controller from DesignWare, a cache controller from ARM, and > > some other device from the chip vendor, and each of them comes with > > EDAC support. Then you get three other vendors that have various > > combinations of the same memory controller, cache controller and > > other EDAC devices. Not all of these chips would have ARM CPU cores, > > some may be e.g. MIPS or PowerPC. > > I don't mean system-vendor specific but IP-block vendor specific. So > doing a designware-edac, arm-edac, apm-edac and so on... Ok, but I'd also do multiple drivers for one vendor if they have several blocks that are clearly unrelated. My understanding was that this is the case here, but I have not looked too closely. Note that a lot of vendors do not like to say who they licensed IP blocks from, or they are contractually required not to say it. > Also, I really want for people thinking of writing EDAC drivers to think > hard before doing so. Whether it is even worth to expose *every* RAS > functionality in some driver. And to consider that exposing it might > cause more trouble than benefit. Definitely agreed on this point. > We've had that experience on x86 with reporting innocuous DRAM ECC > errors which were corrected by the hardware. People weren't even reading > the error message and running around wanting to RMA their processors > because something in dmesg screamed "Hardware Error". > > And now APEI does the firmware-first thing where it gets to look at the > error first and then decide to report it up to the OS or not. > > Which is a good thing because in most cases it unnecessarily upsets the > user. Yes, that is helpful feedback, but seems unrelated to the discussion about how to split this driver or not. > So to get back on track: I think having the IP-block vendor stuff in one > EDAC module would make this whole heterogeneity much more manageable. > > On that system above, we will load - if I can count correctly - 6 EDAC > drivers anyway. But the EDAC pile would remain sane. Ok, let me try to state what I think we agree on: - For EDAC devices that are clearly unrelated (in particular, made by different vendors, but possibly part of the same chip), there should be one module per device type that registers one platform_driver. - For EDAC drivers that are variants of one another (e.g. different generations of the same memory controller), we want one module with one platform_driver that can handle all variants. Let me know if you disagree with the above. If that's fine, I think it leaves the case where we have one chip that has multiple EDAC blocks and we can't easily tell if those blocks are all inter-related or not. For this case, I would like to rely on your judgment as subsystem maintainer on whether the parts look related enough to have a single driver (with matching all device variants) or one driver for each class of device, as well as spotting cases where a part already has a driver from a different chip vendor that was licensing the same IP block. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 9:01 ` Arnd Bergmann @ 2015-04-30 9:41 ` Borislav Petkov 2015-04-30 10:21 ` Arnd Bergmann 2015-04-30 10:42 ` Arnd Bergmann 0 siblings, 2 replies; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 9:41 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 11:01:43AM +0200, Arnd Bergmann wrote: > Ok, but I'd also do multiple drivers for one vendor if they have several > blocks that are clearly unrelated. My understanding was that this is the > case here, but I have not looked too closely. AFAIU, this is a driver for a bunch of xgene IP blocks. Now, if someone integrates xgene memory controller in some other hw platform, we could still serve it with the same xgene-edac driver - it'll simply use only the memory controller part of the functionality. > Note that a lot of vendors do not like to say who they licensed IP > blocks from, or they are contractually required not to say it. Then there's that :-\ But the moment they supply an EDAC driver, one can recognize the IP block from the registers and functionality, no? Or do they change stuff a little bit so that it looks sufficiently different? > Yes, that is helpful feedback, but seems unrelated to the discussion > about how to split this driver or not. Yeah, I know. I just wanted to mention that in the discussion because I get the impression that people are throwing out EDAC drivers just because the hw functionality is there. So I wanted to remind people to step back and consider the usefullness of such a driver first. > Ok, let me try to state what I think we agree on: > > - For EDAC devices that are clearly unrelated (in particular, made by > different vendors, but possibly part of the same chip), there should > be one module per device type that registers one platform_driver. > > - For EDAC drivers that are variants of one another (e.g. different > generations of the same memory controller), we want one module with > one platform_driver that can handle all variants. > > Let me know if you disagree with the above. If that's fine, I think it > leaves the case where we have one chip that has multiple EDAC blocks > and we can't easily tell if those blocks are all inter-related or not. > For this case, I would like to rely on your judgment as subsystem > maintainer on whether the parts look related enough to have a single > driver (with matching all device variants) or one driver for each > class of device, as well as spotting cases where a part already has > a driver from a different chip vendor that was licensing the same IP > block. Well, so the way I see it is, if we could map the EDAC drivers to the IP blocks. I.e., I'd like to have EDAC drivers mapped to the *IP* *blocks* vendor *instead* of the platform vendor. We will have to be able to load multiple EDAC drivers anyway but it'll be much easier this way. So let's have an example: Let's say there's one xgene_edac driver which contains all the RAS functionality of xgene IP blocks. Memory controller, L3 cache, whatever... Now, three *platform* vendors license, say, the xgene memory controller. With my scheme, xgene_edac will be loaded on all those three vendors' Linux. Done. But if those three vendors went and created an EDAC module each for their system, it would be a lot of repeated copy'pasting and bloat. Now, the other dimension doesn't look better either: xgene_edac_mc xgene_edac_mc-v2 xgene_edac_l3 xgene_edac-l2 ... Also, in both cases, if any of those drivers need to talk to each other or know of one another in any way, the situation becomes really funny as they need to create something ad-hoc each time. And this all can be avoided by having a single IP-blocks RAS driver. Does that make more sense? Can it even be done with devicetree and all? Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 9:41 ` Borislav Petkov @ 2015-04-30 10:21 ` Arnd Bergmann 2015-04-30 12:33 ` Borislav Petkov 2015-04-30 10:42 ` Arnd Bergmann 1 sibling, 1 reply; 44+ messages in thread From: Arnd Bergmann @ 2015-04-30 10:21 UTC (permalink / raw) To: linux-arm-kernel On Thursday 30 April 2015 11:41:34 Borislav Petkov wrote: > On Thu, Apr 30, 2015 at 11:01:43AM +0200, Arnd Bergmann wrote: > > Ok, but I'd also do multiple drivers for one vendor if they have several > > blocks that are clearly unrelated. My understanding was that this is the > > case here, but I have not looked too closely. > > AFAIU, this is a driver for a bunch of xgene IP blocks. > > Now, if someone integrates xgene memory controller in some other hw > platform, we could still serve it with the same xgene-edac driver - > it'll simply use only the memory controller part of the functionality. > > > Note that a lot of vendors do not like to say who they licensed IP > > blocks from, or they are contractually required not to say it. > > Then there's that :-\ > > But the moment they supply an EDAC driver, one can recognize the IP > block from the registers and functionality, no? Or do they change stuff > a little bit so that it looks sufficiently different? They don't normally change stuff intentionally, but you can have drivers for the same hardware that are written so differently that it becomes hard to spot that they are doing the same thing. We have a bunch of XGMAC drivers for the same basic licensed IP block, each one configured a bit differently and extended with some soc-specific bits, and drivers written from scratch. I would also not assume that people are trying hard to hide something from you, it's more that some guy is given the task to write a driver for an IP block, but is not told who made that, so the binding will just come with the name of the company he's working for. > > Ok, let me try to state what I think we agree on: > > > > - For EDAC devices that are clearly unrelated (in particular, made by > > different vendors, but possibly part of the same chip), there should > > be one module per device type that registers one platform_driver. > > > > - For EDAC drivers that are variants of one another (e.g. different > > generations of the same memory controller), we want one module with > > one platform_driver that can handle all variants. > > > > Let me know if you disagree with the above. If that's fine, I think it > > leaves the case where we have one chip that has multiple EDAC blocks > > and we can't easily tell if those blocks are all inter-related or not. > > For this case, I would like to rely on your judgment as subsystem > > maintainer on whether the parts look related enough to have a single > > driver (with matching all device variants) or one driver for each > > class of device, as well as spotting cases where a part already has > > a driver from a different chip vendor that was licensing the same IP > > block. > > Well, so the way I see it is, if we could map the EDAC drivers to the IP > blocks. I.e., I'd like to have EDAC drivers mapped to the *IP* *blocks* > vendor *instead* of the platform vendor. > > We will have to be able to load multiple EDAC drivers anyway but it'll > be much easier this way. > > So let's have an example: > > Let's say there's one xgene_edac driver which contains all the RAS > functionality of xgene IP blocks. Memory controller, L3 cache, > whatever... > > Now, three *platform* vendors license, say, the xgene memory controller. > > With my scheme, xgene_edac will be loaded on all those three vendors' > Linux. Done. Right, but they would then also have to load the code for the other blocks they did not license. > But if those three vendors went and created an EDAC module each for > their system, it would be a lot of repeated copy'pasting and bloat. Yes, and I would expect you to reject any such driver submission. > Now, the other dimension doesn't look better either: > > xgene_edac_mc > xgene_edac_mc-v2 > xgene_edac_l3 > xgene_edac-l2 > ... Not following you here, what is the problem here? > Also, in both cases, if any of those drivers need to talk to each other > or know of one another in any way, the situation becomes really funny as > they need to create something ad-hoc each time. Agreed. If the drivers need to talk to one another, that is a clear sign that the devices are not actually independent and that they really belong into a single driver. That's not the case I'm thinking of here. > And this all can be avoided by having a single IP-blocks RAS driver. > > Does that make more sense? Can it even be done with devicetree and all? The devicetree just describes the available hardware blocks. Typically you have one node for one block (e.g. a memory controller), and then have one driver for that block. Things get a little funny if you want to have multiple drivers for the same device, e.g. you want a driver in drivers/memory/ to support autorefresh in sleep states and another driver in drivers/edac to support error handling. If you get something like this, you need some coordination between the two drivers, but that is unrelated to devicetree or not. A platform driver should match against a set of similar devices that are identified by a "compatible" string, and we try to list the name of the ip block here, which could be something like "arm,pl12345rev1.2" if it is licensed from another company, or if that is not known, describe it by using the product number of the first chip it appeared in, e.g. "amcc,405ez-memory-controller". APM does not use numbers for their chips, so "apm,xgene-memory-controller" would refer to the memory controller used in the "xgene" chip. If they come out with a second product that uses the same one, they would use the same string, or use "apm,ygene-memory-controller" for the one that is in "ygene" if that is slightly different. The driver can then match against both and handle the differences based on the compatible string. The point where it gets silly is when you try to handle multiple unrelated devices in the same driver and you get a probe function that just does: apm_edac_probe(struct platform_device *dev) { if (dev_is_xgene_mc(dev)) return apm_probe_xgene_mc(dev); if (dev_is_xgene_l2(dev)) return apm_probe_xgene_l2(dev); if (dev_is_ygene_mc(dev)) ... } with each of the called functions implemented in a separate file. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 10:21 ` Arnd Bergmann @ 2015-04-30 12:33 ` Borislav Petkov 2015-04-30 12:52 ` Arnd Bergmann 0 siblings, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 12:33 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 12:21:14PM +0200, Arnd Bergmann wrote: > They don't normally change stuff intentionally, but you can have > drivers for the same hardware that are written so differently that > it becomes hard to spot that they are doing the same thing. > > We have a bunch of XGMAC drivers for the same basic licensed IP block, > each one configured a bit differently and extended with some soc-specific > bits, and drivers written from scratch. So not nice. > I would also not assume that people are trying hard to hide something > from you, it's more that some guy is given the task to write a driver > for an IP block, but is not told who made that, so the binding will > just come with the name of the company he's working for. So I'd home some guy would go grep the sources and hopefully find that very similar functionality is already present. > Right, but they would then also have to load the code for the other > blocks they did not license. That's a problem? I mean, the other code is inactive anyway and is only a couple of KBytes. So not a big deal IMO. If that becomes a problem with embedded, we can always do ugly ifdeffery and Kconfig games if absolutely necessary. > > xgene_edac_mc > > xgene_edac_mc-v2 > > xgene_edac_l3 > > xgene_edac-l2 > > ... > > Not following you here, what is the problem here? The unnecessary fragmentation of xgene_edac functionality. I would like, if possible, to concentrate all xgene RAS stuff in one file, like x86 does. And then there's the talking between the two aspect below... > Agreed. If the drivers need to talk to one another, that is a clear > sign that the devices are not actually independent and that they > really belong into a single driver. That's not the case I'm thinking > of here. But if we split <vendor>_edac stuff per IP block, that will become a problem. So we better talk about it now. > The devicetree just describes the available hardware blocks. Typically > you have one node for one block (e.g. a memory controller), and then > have one driver for that block. > > Things get a little funny if you want to have multiple drivers for > the same device, e.g. you want a driver in drivers/memory/ to support > autorefresh in sleep states and another driver in drivers/edac to > support error handling. If you get something like this, you need > some coordination between the two drivers, but that is unrelated to > devicetree or not. > > A platform driver should match against a set of similar devices that are > identified by a "compatible" string, and we try to list the name of the > ip block here, which could be something like "arm,pl12345rev1.2" if it > is licensed from another company, or if that is not known, describe it > by using the product number of the first chip it appeared in, e.g. > "amcc,405ez-memory-controller". APM does not use numbers for their > chips, so "apm,xgene-memory-controller" would refer to the memory > controller used in the "xgene" chip. If they come out with a second > product that uses the same one, they would use the same string, or > use "apm,ygene-memory-controller" for the one that is in "ygene" if > that is slightly different. The driver can then match against both > and handle the differences based on the compatible string. Thanks for that background info, it helps a lot. > The point where it gets silly is when you try to handle multiple > unrelated devices in the same driver and you get a probe function > that just does: Ok, what if they're related through RAS? I.e., xgene_edac contains all RAS-specific functionality of the xgene IP blocks. This doesn't sound silly to me, frankly, and it concentrates it all in one place. Communication between blocks is trivial and there's no code duplication. Other vendors using those IP blocks would simply add new DT descriptions. > apm_edac_probe(struct platform_device *dev) > { > if (dev_is_xgene_mc(dev)) > return apm_probe_xgene_mc(dev); > if (dev_is_xgene_l2(dev)) > return apm_probe_xgene_l2(dev); > if (dev_is_ygene_mc(dev)) > ... So we do that on x86 - amd64_edac does AMD per-family setup and sb_edac too, for the last 4 Intel uarches. And it works just fine. So if "silly" is the only problem, I can certainly live with it. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 12:33 ` Borislav Petkov @ 2015-04-30 12:52 ` Arnd Bergmann 0 siblings, 0 replies; 44+ messages in thread From: Arnd Bergmann @ 2015-04-30 12:52 UTC (permalink / raw) To: linux-arm-kernel On Thursday 30 April 2015 14:33:00 Borislav Petkov wrote: > > > The point where it gets silly is when you try to handle multiple > > unrelated devices in the same driver and you get a probe function > > that just does: > > Ok, what if they're related through RAS? > > I.e., xgene_edac contains all RAS-specific functionality of the xgene IP > blocks. This doesn't sound silly to me, frankly, and it concentrates it > all in one place. Communication between blocks is trivial and there's no > code duplication. My point is that with the current implementation, there is no communication at all: you have four separate drivers that just live in the same file. > Other vendors using those IP blocks would simply add new DT descriptions. > > > apm_edac_probe(struct platform_device *dev) > > { > > if (dev_is_xgene_mc(dev)) > > return apm_probe_xgene_mc(dev); > > if (dev_is_xgene_l2(dev)) > > return apm_probe_xgene_l2(dev); > > if (dev_is_ygene_mc(dev)) > > ... > > So we do that on x86 - amd64_edac does AMD per-family setup and sb_edac > too, for the last 4 Intel uarches. And it works just fine. So if "silly" > is the only problem, I can certainly live with it. This is different, because in your examples, there is still some shared code between the CPU families, fact most of the driver looks like it's shared, and even the family-specific code is mostly shared. E.g. f1x_early_channel_count() is used on almost all families. What I mean with silly is the case where none of the code behind those is shared at all, and the only common part is the probe() function that contains the multiplexer. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 9:41 ` Borislav Petkov 2015-04-30 10:21 ` Arnd Bergmann @ 2015-04-30 10:42 ` Arnd Bergmann 2015-04-30 13:00 ` Borislav Petkov 1 sibling, 1 reply; 44+ messages in thread From: Arnd Bergmann @ 2015-04-30 10:42 UTC (permalink / raw) To: linux-arm-kernel On Thursday 30 April 2015 11:41:34 Borislav Petkov wrote: > But if those three vendors went and created an EDAC module each for > their system, it would be a lot of repeated copy'pasting and bloat. > > Now, the other dimension doesn't look better either: > > xgene_edac_mc > xgene_edac_mc-v2 > xgene_edac_l3 > xgene_edac-l2 > ... > > Also, in both cases, if any of those drivers need to talk to each other > or know of one another in any way, the situation becomes really funny as > they need to create something ad-hoc each time. > I've looked at the driver in a little more detail now, and found that it is a weird conglomerate at the moment. Basically you have four drivers in one file here, appended to one another, but not sharing any common functions except the module_init() that registers the four drivers. This is what Rob was referring to when he suggested splitting it up into four files, and these would in total be smaller than the common file (by being able to use module_platform_driver()). However, there is another dimension to this, which supports your point about making it one driver for the platform: The split into four drivers is completely artificial, because all four use the same IRQs and implement four separate interrupt handlers for it (using IRQF_SHARED), each handling only the events they are interested in. Similarly, they all access the same register set ("pcperror"), aside from having their own separate registers, and they use the "syscon" framework to get to the registers. This seems to be an inferior design, as the pcperror stuff apparently is the real EDAC block on the system and that should have a device driver for itself, with proper interfaces. syscon in contrast is designed for the case where you have a badly designed IP block that has lots of registers for functions all over the place for which no clear abstraction is possible. There is also the "efuse" that is listed as "syscon" here, but should really use a separate framework that has been under discussion for a while. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 10:42 ` Arnd Bergmann @ 2015-04-30 13:00 ` Borislav Petkov 2015-04-30 16:57 ` Loc Ho 0 siblings, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 13:00 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 12:42:48PM +0200, Arnd Bergmann wrote: > I've looked at the driver in a little more detail now, and found that > it is a weird conglomerate at the moment. Basically you have four > drivers in one file here, appended to one another, but not sharing > any common functions except the module_init() that registers the four > drivers. This is what Rob was referring to when he suggested splitting > it up into four files, and these would in total be smaller than the > common file (by being able to use module_platform_driver()). > > However, there is another dimension to this, which supports your point > about making it one driver for the platform: The split into four drivers > is completely artificial, because all four use the same IRQs and > implement four separate interrupt handlers for it (using IRQF_SHARED), > each handling only the events they are interested in. Similarly, they > all access the same register set ("pcperror"), aside from having their > own separate registers, and they use the "syscon" framework to get to > the registers. This seems to be an inferior design, as the pcperror Doh, now that you mention it... So why isn't this thing registering a single IRQ handler which multiplexes between the _check routines depending on the bits set in PCPHPERRINTSTS/MEMERRINTSTS? (L3 alternatively, ctx->dev_csr + L3C_ESR). This would really make it a single driver which acts according to the bits set in those error registers. It can't get any simpler than that. Loc? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 13:00 ` Borislav Petkov @ 2015-04-30 16:57 ` Loc Ho 2015-04-30 17:18 ` Borislav Petkov 0 siblings, 1 reply; 44+ messages in thread From: Loc Ho @ 2015-04-30 16:57 UTC (permalink / raw) To: linux-arm-kernel Hi, >> I've looked at the driver in a little more detail now, and found that >> it is a weird conglomerate at the moment. Basically you have four >> drivers in one file here, appended to one another, but not sharing >> any common functions except the module_init() that registers the four >> drivers. This is what Rob was referring to when he suggested splitting >> it up into four files, and these would in total be smaller than the >> common file (by being able to use module_platform_driver()). >> >> However, there is another dimension to this, which supports your point >> about making it one driver for the platform: The split into four drivers >> is completely artificial, because all four use the same IRQs and >> implement four separate interrupt handlers for it (using IRQF_SHARED), >> each handling only the events they are interested in. Similarly, they >> all access the same register set ("pcperror"), aside from having their >> own separate registers, and they use the "syscon" framework to get to >> the registers. This seems to be an inferior design, as the pcperror > > Doh, now that you mention it... > > So why isn't this thing registering a single IRQ handler which > multiplexes between the _check routines depending on the bits set in > PCPHPERRINTSTS/MEMERRINTSTS? (L3 alternatively, ctx->dev_csr + L3C_ESR). > > This would really make it a single driver which acts according to the > bits set in those error registers. It can't get any simpler than that. > > Loc? I had read all the emails interaction. Yes I can write a single EDAC driver. But I actually have multiple instances and single instance of the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, one L3 and one SoC. If you can suggest to me how this would work with devicetree, I might be able to make it works. But from HW point of view (take the memory controller as an example), there is really 4 hardware blocks and some shared IP's block for status. It is much simple representation with multiple instance of the driver. In addition, should one day we have 6 memory controllers, I would just add two more instances in the DT node. I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of: 1. One top level interrupt and status registers 2. CSW, MCB A and MCB B for detection of any active memory controllers 3. 4 individual memory controller instance 4. 4 CPU domain instance with its own individual L2 registers 5. Each CPU has instance own L1 6. One L3 instance 7. One SoC instance As for as what is useful, we find that the memory controller and the L1 and L2 are useful as it provides info for errors - for HW/system debugging as well as margin of DDR. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 16:57 ` Loc Ho @ 2015-04-30 17:18 ` Borislav Petkov 2015-04-30 21:19 ` Loc Ho 0 siblings, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 17:18 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 09:57:46AM -0700, Loc Ho wrote: > I had read all the emails interaction. Yes I can write a single EDAC > driver. But I actually have multiple instances and single instance of > the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, > one L3 and one SoC. If you can suggest to me how this would work with > devicetree, I might be able to make it works. But from HW point of > view (take the memory controller as an example), there is really 4 > hardware blocks and some shared IP's block for status. It is much > simple representation with multiple instance of the driver. Your whole error reporting is done roughly through the same registers, as Arnd pointed out. Why can't you build your single driver around that and based on the error type being reported, multiplex inside a *single* interrupt handler to the proper decoding routines. > I think we should first agreed on the DT binding and let's not worry > about APEI. Then, whether we have one file or multiple file. Again, > the HW consist of: > > 1. One top level interrupt and status registers > 2. CSW, MCB A and MCB B for detection of any active memory controllers > 3. 4 individual memory controller instance > 4. 4 CPU domain instance with its own individual L2 registers > 5. Each CPU has instance own L1 > 6. One L3 instance > 7. One SoC instance And all of those are separate IP blocks and they can be integrated with other, non APM hardware, or do those things all belong together? IOW, can we expect such IP blocks to appear in other hw platforms or are they all custom APM design which need to go together due to proprietary coherency protocol or whatever, for example? > As for as what is useful, we find that the memory controller and the > L1 and L2 are useful as it provides info for errors - for HW/system > debugging as well as margin of DDR. Sounds like all is useful. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 17:18 ` Borislav Petkov @ 2015-04-30 21:19 ` Loc Ho 2015-04-30 21:30 ` Borislav Petkov 2015-04-30 22:36 ` Rob Herring 0 siblings, 2 replies; 44+ messages in thread From: Loc Ho @ 2015-04-30 21:19 UTC (permalink / raw) To: linux-arm-kernel Hi, >> I had read all the emails interaction. Yes I can write a single EDAC >> driver. But I actually have multiple instances and single instance of >> the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, >> one L3 and one SoC. If you can suggest to me how this would work with >> devicetree, I might be able to make it works. But from HW point of >> view (take the memory controller as an example), there is really 4 >> hardware blocks and some shared IP's block for status. It is much >> simple representation with multiple instance of the driver. > > Your whole error reporting is done roughly through the same registers, > as Arnd pointed out. Why can't you build your single driver around that > and based on the error type being reported, multiplex inside a *single* > interrupt handler to the proper decoding routines. Yes... I can do this but I don't see how it will scale when some SoC in the future may have more or less instance of the memory controllers for example. I guess for now, I will just iteration 4 time with an single base address or four resources in a loop. Let's agreed on the DT layout and I will generate an new patch then. See below... > >> I think we should first agreed on the DT binding and let's not worry >> about APEI. Then, whether we have one file or multiple file. Again, >> the HW consist of: >> >> 1. One top level interrupt and status registers >> 2. CSW, MCB A and MCB B for detection of any active memory controllers >> 3. 4 individual memory controller instance >> 4. 4 CPU domain instance with its own individual L2 registers >> 5. Each CPU has instance own L1 >> 6. One L3 instance >> 7. One SoC instance > > And all of those are separate IP blocks and they can be integrated with > other, non APM hardware, or do those things all belong together? The top level interrupt may be different and APM specific unless other vendors adapt the same bit definitions. I highly doubt other vendor will use the same bit definitions. The CSW is APM only. The MCB A, MCB B, and memory controller are APM only. The L3, and SoC are APM specify only. For L1 and L2, I will need to check with the CPU designer - but likely APM specific. > IOW, > can we expect such IP blocks to appear in other hw platforms or are > they all custom APM design which need to go together due to proprietary > coherency protocol or whatever, for example? See above. > >> As for as what is useful, we find that the memory controller and the >> L1 and L2 are useful as it provides info for errors - for HW/system >> debugging as well as margin of DDR. > > Sounds like all is useful. > Now let us discuss the DT layout if it is an single driver: xgeneedac: xgeneedac at 7e800000 { compatible = "apm,xgene-edac"; regmap-efuse = <&efuse>; /* efuse */ reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt status resource */ <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */ <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */ <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */ <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */ <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */ <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */ <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */ <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */ <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */ <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */ <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */ <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */ <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */ interrupts = <0x0 0x20 0x4>, <0x0 0x21 0x4>, <0x0 0x27 0x4>; }; Arnd, Rob, and Borislav, Please comment and let me know. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 21:19 ` Loc Ho @ 2015-04-30 21:30 ` Borislav Petkov 2015-04-30 21:39 ` Loc Ho 2015-04-30 22:36 ` Rob Herring 1 sibling, 1 reply; 44+ messages in thread From: Borislav Petkov @ 2015-04-30 21:30 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 02:19:44PM -0700, Loc Ho wrote: > The top level interrupt may be different and APM specific unless other > vendors adapt the same bit definitions. I highly doubt other vendor > will use the same bit definitions. The CSW is APM only. The MCB A, MCB > B, and memory controller are APM only. The L3, and SoC are APM specify > only. For L1 and L2, I will need to check with the CPU designer - but > likely APM specific. So it sounds to me like this whole driver will control APM-specific hw so a single driver should be fine. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 21:30 ` Borislav Petkov @ 2015-04-30 21:39 ` Loc Ho 0 siblings, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-04-30 21:39 UTC (permalink / raw) To: linux-arm-kernel Hi, >> The top level interrupt may be different and APM specific unless other >> vendors adapt the same bit definitions. I highly doubt other vendor >> will use the same bit definitions. The CSW is APM only. The MCB A, MCB >> B, and memory controller are APM only. The L3, and SoC are APM specify >> only. For L1 and L2, I will need to check with the CPU designer - but >> likely APM specific. > > So it sounds to me like this whole driver will control APM-specific hw > so a single driver should be fine. Okay... But I would like Rob agrees on the DT binding before I generate this new patch. Rob, can you comment on the DT binding in previous email thread? -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 21:19 ` Loc Ho 2015-04-30 21:30 ` Borislav Petkov @ 2015-04-30 22:36 ` Rob Herring 2015-04-30 22:47 ` Arnd Bergmann ` (2 more replies) 1 sibling, 3 replies; 44+ messages in thread From: Rob Herring @ 2015-04-30 22:36 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 30, 2015 at 4:19 PM, Loc Ho <lho@apm.com> wrote: > Hi, > >>> I had read all the emails interaction. Yes I can write a single EDAC >>> driver. But I actually have multiple instances and single instance of >>> the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, >>> one L3 and one SoC. If you can suggest to me how this would work with >>> devicetree, I might be able to make it works. But from HW point of >>> view (take the memory controller as an example), there is really 4 >>> hardware blocks and some shared IP's block for status. It is much >>> simple representation with multiple instance of the driver. >> >> Your whole error reporting is done roughly through the same registers, >> as Arnd pointed out. Why can't you build your single driver around that >> and based on the error type being reported, multiplex inside a *single* >> interrupt handler to the proper decoding routines. > > Yes... I can do this but I don't see how it will scale when some SoC > in the future may have more or less instance of the memory controllers > for example. I guess for now, I will just iteration 4 time with an > single base address or four resources in a loop. Let's agreed on the > DT layout and I will generate an new patch then. See below... You are right, it does not scale. >>> I think we should first agreed on the DT binding and let's not worry >>> about APEI. Then, whether we have one file or multiple file. Again, >>> the HW consist of: >>> >>> 1. One top level interrupt and status registers For these registers, are there ECC specific functions here or just normal interrupt control/status bits (mask/unmask/status)? Assuming the later, then you should make this block an interrupt-controller. Then this is the interrupt-parent for the rest of the blocks. >>> 2. CSW, MCB A and MCB B for detection of any active memory controllers >>> 3. 4 individual memory controller instance >>> 4. 4 CPU domain instance with its own individual L2 registers >>> 5. Each CPU has instance own L1 >>> 6. One L3 instance >>> 7. One SoC instance >> >> And all of those are separate IP blocks and they can be integrated with >> other, non APM hardware, or do those things all belong together? > > The top level interrupt may be different and APM specific unless other > vendors adapt the same bit definitions. I highly doubt other vendor > will use the same bit definitions. The CSW is APM only. The MCB A, MCB > B, and memory controller are APM only. The L3, and SoC are APM specify > only. For L1 and L2, I will need to check with the CPU designer - but > likely APM specific. > >> IOW, >> can we expect such IP blocks to appear in other hw platforms or are >> they all custom APM design which need to go together due to proprietary >> coherency protocol or whatever, for example? > > See above. > >> >>> As for as what is useful, we find that the memory controller and the >>> L1 and L2 are useful as it provides info for errors - for HW/system >>> debugging as well as margin of DDR. >> >> Sounds like all is useful. >> > > Now let us discuss the DT layout if it is an single driver: > > xgeneedac: xgeneedac at 7e800000 { > compatible = "apm,xgene-edac"; > regmap-efuse = <&efuse>; /* efuse */ > reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt > status resource */ > <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource > <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */ > <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */ > <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */ > <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */ > <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */ > <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */ > <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */ > <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */ > <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */ > <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */ > <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */ > <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */ > <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */ Uggg, no. You were on the right track earlier in the thread. One node per instance of each block. > interrupts = <0x0 0x20 0x4>, > <0x0 0x21 0x4>, > <0x0 0x27 0x4>; Can you describe what each of these interrupts do? Rob ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 22:36 ` Rob Herring @ 2015-04-30 22:47 ` Arnd Bergmann 2015-05-01 6:44 ` Loc Ho 2015-04-30 22:59 ` Loc Ho 2015-05-01 19:59 ` Loc Ho 2 siblings, 1 reply; 44+ messages in thread From: Arnd Bergmann @ 2015-04-30 22:47 UTC (permalink / raw) To: linux-arm-kernel On Thursday 30 April 2015 17:36:16 Rob Herring wrote: > > Now let us discuss the DT layout if it is an single driver: > > > > xgeneedac: xgeneedac at 7e800000 { > > compatible = "apm,xgene-edac"; > > regmap-efuse = <&efuse>; /* efuse */ > > reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt > > status resource */ > > <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource > > <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */ > > <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */ > > <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */ > > <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */ > > <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */ > > <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */ > > <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */ > > <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */ > > <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */ > > <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */ > > <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */ > > <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */ > > <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */ > > Uggg, no. You were on the right track earlier in the thread. One node > per instance of each block. > I agree these should be separate nodes, but I also think that we want a separate node for the pcp/pcperror/xgeneedac device. The compatible string for that should match whatever the datasheet calls that block, no idea why we now have the third name for that. The specific parts could either be subnodes of the pcperror device, or they could be separate device nodes that reference the pcperror device through a phandle, so the driver can make the connection between them. This probably depends on what exactly all those nodes are: if the registers in there are all exclusively related to EDAC handling of the pcperror, subnodes would be best, but if the registers also contain functionality that is not related to EDAC handling, we probably want to have separate top-level nodes that a driver could bind to, e.g. for doing power management on the memory controller. Arnd ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 22:47 ` Arnd Bergmann @ 2015-05-01 6:44 ` Loc Ho 0 siblings, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-05-01 6:44 UTC (permalink / raw) To: linux-arm-kernel Hi Arnd, >> > Now let us discuss the DT layout if it is an single driver: >> > >> > xgeneedac: xgeneedac at 7e800000 { >> > compatible = "apm,xgene-edac"; >> > regmap-efuse = <&efuse>; /* efuse */ >> > reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt >> > status resource */ >> > <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource >> > <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */ >> > <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */ >> > <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */ >> > <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */ >> > <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */ >> > <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */ >> > <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */ >> > <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */ >> > <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */ >> > <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */ >> > <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */ >> > <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */ >> > <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */ >> >> Uggg, no. You were on the right track earlier in the thread. One node >> per instance of each block. >> > > I agree these should be separate nodes, but I also think that we want a > separate node for the pcp/pcperror/xgeneedac device. The compatible > string for that should match whatever the datasheet calls that block, > no idea why we now have the third name for that. Let me go back to the regmap in the previous version - one node for CPU domain bus (PCP), one node for CPU switch fabric (CSW), one node for the memory bridge A (MCB A), one node for the memory bridge B (MCB B). Writing an simple driver that all it will provide is an map of that those registers space doesn't make sense to me if once can use the regmap interface. There is really nothing in these IP's that the OS would care about. Anyone would care would be the FW. > > The specific parts could either be subnodes of the pcperror device, > or they could be separate device nodes that reference the pcperror > device through a phandle, so the driver can make the connection between > them. This probably depends on what exactly all those nodes are: > if the registers in there are all exclusively related to EDAC handling > of the pcperror, subnodes would be best, but if the registers also > contain functionality that is not related to EDAC handling, we > probably want to have separate top-level nodes that a driver could > bind to, e.g. for doing power management on the memory controller. Again... Each sub-block does have some other type of register besides the parity status registers and etc. But they are also divided according into block of registers.For power management with the memory controller, those will be handled by the FW. The only exception that I can see is the CPU clock divider for the P-state which will require an driver but that region can be mapped as a special area. Let's just focus on the memory controller first and not worry about the other just yet. So here is what I have: /* CPU bus node */ pcp: pcp at 78800000 { compatible = "syscon"; reg = <0x0 0x78800000 0x0 0x100>; }; /* CPU switch fabric node */ csw: csw at 7e200000 { compatible = "syscon"; reg = <0x0 0x7e200000 0x0 0x1000>; }; /* Memory Bridge A */ mcba: mcba at 7e700000 { compatible = "syscon"; reg = <0x0 0x7e700000 0x0 0x1000>; }; /* Memory Bridge B */ mcbb: mcbb at 7e720000 { compatible = "syscon"; reg = <0x0 0x7e720000 0x0 0x1000>; }; /* Memory Controller 0 */ 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>; }; Any comment on this before I generate another version? -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 22:36 ` Rob Herring 2015-04-30 22:47 ` Arnd Bergmann @ 2015-04-30 22:59 ` Loc Ho 2015-05-01 19:59 ` Loc Ho 2 siblings, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-04-30 22:59 UTC (permalink / raw) To: linux-arm-kernel Hi, >> >>>> I had read all the emails interaction. Yes I can write a single EDAC >>>> driver. But I actually have multiple instances and single instance of >>>> the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, >>>> one L3 and one SoC. If you can suggest to me how this would work with >>>> devicetree, I might be able to make it works. But from HW point of >>>> view (take the memory controller as an example), there is really 4 >>>> hardware blocks and some shared IP's block for status. It is much >>>> simple representation with multiple instance of the driver. >>> >>> Your whole error reporting is done roughly through the same registers, >>> as Arnd pointed out. Why can't you build your single driver around that >>> and based on the error type being reported, multiplex inside a *single* >>> interrupt handler to the proper decoding routines. >> >> Yes... I can do this but I don't see how it will scale when some SoC >> in the future may have more or less instance of the memory controllers >> for example. I guess for now, I will just iteration 4 time with an >> single base address or four resources in a loop. Let's agreed on the >> DT layout and I will generate an new patch then. See below... > > You are right, it does not scale. Glad that you agree and I would like to avoid. > >>>> I think we should first agreed on the DT binding and let's not worry >>>> about APEI. Then, whether we have one file or multiple file. Again, >>>> the HW consist of: >>>> >>>> 1. One top level interrupt and status registers > > For these registers, are there ECC specific functions here or just > normal interrupt control/status bits (mask/unmask/status)? It is just interrupt and status bit - a bit for an type of errors. > Assuming > the later, then you should make this block an interrupt-controller. > Then this is the interrupt-parent for the rest of the blocks. Okay... Let me see if I can find an example. > >>>> 2. CSW, MCB A and MCB B for detection of any active memory controllers >>>> 3. 4 individual memory controller instance >>>> 4. 4 CPU domain instance with its own individual L2 registers >>>> 5. Each CPU has instance own L1 >>>> 6. One L3 instance >>>> 7. One SoC instance >>> >>> And all of those are separate IP blocks and they can be integrated with >>> other, non APM hardware, or do those things all belong together? >> >> The top level interrupt may be different and APM specific unless other >> vendors adapt the same bit definitions. I highly doubt other vendor >> will use the same bit definitions. The CSW is APM only. The MCB A, MCB >> B, and memory controller are APM only. The L3, and SoC are APM specify >> only. For L1 and L2, I will need to check with the CPU designer - but >> likely APM specific. >> >>> IOW, >>> can we expect such IP blocks to appear in other hw platforms or are >>> they all custom APM design which need to go together due to proprietary >>> coherency protocol or whatever, for example? >> >> See above. >> >>> >>>> As for as what is useful, we find that the memory controller and the >>>> L1 and L2 are useful as it provides info for errors - for HW/system >>>> debugging as well as margin of DDR. >>> >>> Sounds like all is useful. >>> >> >> Now let us discuss the DT layout if it is an single driver: >> >> xgeneedac: xgeneedac at 7e800000 { >> compatible = "apm,xgene-edac"; >> regmap-efuse = <&efuse>; /* efuse */ >> reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt >> status resource */ >> <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource >> <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */ >> <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */ >> <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */ >> <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */ >> <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */ >> <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */ >> <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */ >> <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */ >> <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */ >> <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */ >> <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */ >> <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */ >> <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */ > > Uggg, no. You were on the right track earlier in the thread. One node > per instance of each block. Let me try just the memory controller. I will re-generate an patch for just the memory controller. > >> interrupts = <0x0 0x20 0x4>, >> <0x0 0x21 0x4>, >> <0x0 0x27 0x4>; > > Can you describe what each of these interrupts do? IRQ 0x20 is used for un-correctable error for memory controller, L1, L2 and some SoC IP parity errors. IRQ 0x21 is used for correctable error for memory controller, and some SoC IP parity errors. IRQ 0x27 is used for SoC parity error. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-30 22:36 ` Rob Herring 2015-04-30 22:47 ` Arnd Bergmann 2015-04-30 22:59 ` Loc Ho @ 2015-05-01 19:59 ` Loc Ho 2015-05-04 22:36 ` Rob Herring 2 siblings, 1 reply; 44+ messages in thread From: Loc Ho @ 2015-05-01 19:59 UTC (permalink / raw) To: linux-arm-kernel Hi Rob, >>>> I think we should first agreed on the DT binding and let's not worry >>>> about APEI. Then, whether we have one file or multiple file. Again, >>>> the HW consist of: >>>> >>>> 1. One top level interrupt and status registers > > For these registers, are there ECC specific functions here or just > normal interrupt control/status bits (mask/unmask/status)? Assuming > the later, then you should make this block an interrupt-controller. > Then this is the interrupt-parent for the rest of the blocks. > This is the only item remain before I generate an patch with just the memory controller. Most of the code that I see are actually an interrupt controller HW. As it is just an interrupt mask and status registers, is there an example in Linux that I can model after? Also, I am not quite convince as to why we can't just share the interrupt and request it by each memory controller? -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-05-01 19:59 ` Loc Ho @ 2015-05-04 22:36 ` Rob Herring 2015-05-04 23:39 ` Loc Ho 0 siblings, 1 reply; 44+ messages in thread From: Rob Herring @ 2015-05-04 22:36 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 1, 2015 at 2:59 PM, Loc Ho <lho@apm.com> wrote: > Hi Rob, > >>>>> I think we should first agreed on the DT binding and let's not worry >>>>> about APEI. Then, whether we have one file or multiple file. Again, >>>>> the HW consist of: >>>>> >>>>> 1. One top level interrupt and status registers >> >> For these registers, are there ECC specific functions here or just >> normal interrupt control/status bits (mask/unmask/status)? Assuming >> the later, then you should make this block an interrupt-controller. >> Then this is the interrupt-parent for the rest of the blocks. >> > > This is the only item remain before I generate an patch with just the > memory controller. Most of the code that I see are actually an > interrupt controller HW. As it is just an interrupt mask and status > registers, is there an example in Linux that I can model after? Also, > I am not quite convince as to why we can't just share the interrupt > and request it by each memory controller? Pretty much anything that calls irq_set_chained_handler. If you don't need to touch the shared registers and can set them up once at boot time, then you could just do shared irq handlers. But if you have to clear the interrupt within the PCP, you need a demuxer. Rob ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-05-04 22:36 ` Rob Herring @ 2015-05-04 23:39 ` Loc Ho 0 siblings, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-05-04 23:39 UTC (permalink / raw) To: linux-arm-kernel Hi, On Mon, May 4, 2015 at 3:36 PM, Rob Herring <robherring2@gmail.com> wrote: > On Fri, May 1, 2015 at 2:59 PM, Loc Ho <lho@apm.com> wrote: >> Hi Rob, >> >>>>>> I think we should first agreed on the DT binding and let's not worry >>>>>> about APEI. Then, whether we have one file or multiple file. Again, >>>>>> the HW consist of: >>>>>> >>>>>> 1. One top level interrupt and status registers >>> >>> For these registers, are there ECC specific functions here or just >>> normal interrupt control/status bits (mask/unmask/status)? Assuming >>> the later, then you should make this block an interrupt-controller. >>> Then this is the interrupt-parent for the rest of the blocks. >>> >> >> This is the only item remain before I generate an patch with just the >> memory controller. Most of the code that I see are actually an >> interrupt controller HW. As it is just an interrupt mask and status >> registers, is there an example in Linux that I can model after? Also, >> I am not quite convince as to why we can't just share the interrupt >> and request it by each memory controller? > > Pretty much anything that calls irq_set_chained_handler. If you don't > need to touch the shared registers and can set them up once at boot > time, then you could just do shared irq handlers. But if you have to > clear the interrupt within the PCP, you need a demuxer. > Thanks Rob. I look through the entire driver. Interrupts are cleared at the source. Therefore, the next version will just register for the shared interrupt as it is currently. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 21:56 ` Loc Ho 2015-04-29 22:08 ` Borislav Petkov @ 2015-04-29 22:43 ` Rob Herring 2015-04-30 0:47 ` Loc Ho 1 sibling, 1 reply; 44+ messages in thread From: Rob Herring @ 2015-04-29 22:43 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 4:56 PM, Loc Ho <lho@apm.com> wrote: > Hi, > >>> > Similar comments for the rest. I would define memory controller >>> > bindings and EDAC driver, then worry about the rest. >>> >>> Okay.. As comment in following emails, I will break up the driver into >>> multiple drivers and focus only on the memory controller driver first. >> >> Please no multiple EDAC drivers. Or do you mean something else here? > > We will have the following: > > xgene-edac-mc.c > xgene-edac-pmd.c > xgene-edac-l3.c > xgene-edac-soc.c > > Or what would you suggest. There are the following HW: > > 1. 4 DDR controller with one shared top level interrupt, two shared > memory bridges And ECC registers are in the DDR controllers? I would expect the DT to have 4 DDR controller nodes and probably 2 bridges. The memory bridges have control registers? > 2. 4 CPU's domain with one shared top level interrupt, shared L2 for > two CPU's, and individual L1 > 3. 1 L3 with one shared top level interrupt Presumably the registers for ECC are wired to some other block and not CPU registers? > 4. One SoC memory parity block with one shared top level interupt Shared interrupts are easily supported, but implicit in the DT. Otherwise, it seems this block is independent from the rest, correct? Rob ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding 2015-04-29 22:43 ` Rob Herring @ 2015-04-30 0:47 ` Loc Ho 0 siblings, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-04-30 0:47 UTC (permalink / raw) To: linux-arm-kernel Hi Rob, >> >>>> > Similar comments for the rest. I would define memory controller >>>> > bindings and EDAC driver, then worry about the rest. >>>> >>>> Okay.. As comment in following emails, I will break up the driver into >>>> multiple drivers and focus only on the memory controller driver first. >>> >>> Please no multiple EDAC drivers. Or do you mean something else here? >> >> We will have the following: >> >> xgene-edac-mc.c >> xgene-edac-pmd.c >> xgene-edac-l3.c >> xgene-edac-soc.c >> >> Or what would you suggest. There are the following HW: >> >> 1. 4 DDR controller with one shared top level interrupt, two shared >> memory bridges > > And ECC registers are in the DDR controllers? > Yes... ECC registers are with each DDR controller. That's why I leave the reg property. > I would expect the DT to have 4 DDR controller nodes and probably 2 > bridges. The memory bridges have control registers? Mustang DT does have 4 individual DT nodes for memory controllers. The 2 bridges are used to only determine if the memory controller is active or not. If not, it just skips the initialization during probe. > >> 2. 4 CPU's domain with one shared top level interrupt, shared L2 for >> two CPU's, and individual L1 >> 3. 1 L3 with one shared top level interrupt > > Presumably the registers for ECC are wired to some other block and not > CPU registers? Yes... The bus has dedicated 64K block for L2, L1, and L3. That's why the reg property still there and not shared. > >> 4. One SoC memory parity block with one shared top level interupt > > Shared interrupts are easily supported, but implicit in the DT. > Otherwise, it seems this block is independent from the rest, correct? Interrupts are shared and the status registers are also shared. Yes... the actual error module is independent. The shared blocks are: 1. Interrupt status (PCP block) - Needed for all subblock IP's 2. CSW, MCB A and MCB B - Needed for each individual memory controller to determine whether it is active 3. efuse block - used to determine if an CPU domain is disabled (non-existed in HW) Anything else need clarification? -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 1/5] arm64: Enable EDAC on ARM64 2015-04-28 22:10 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Loc Ho 2015-04-28 22:10 ` [PATCH v7 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver Loc Ho @ 2015-04-29 14:40 ` Catalin Marinas 2015-04-29 14:46 ` Jon Masters 2015-04-29 21:39 ` Loc Ho 1 sibling, 2 replies; 44+ messages in thread From: Catalin Marinas @ 2015-04-29 14:40 UTC (permalink / raw) To: linux-arm-kernel On Tue, Apr 28, 2015 at 04:10:41PM -0600, Loc Ho wrote: > --- /dev/null > +++ b/arch/arm64/include/asm/edac.h > @@ -0,0 +1,31 @@ > +/* > + * 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 For consistency, please use __ASM_EDAC_H. > +/* > + * ECC atomic, DMA, SMP and interrupt safe scrub function. > + * Implements the per arch atomic_scrub() that EDAC use for software > + * ECC scrubbing. It reads memory and then writes back the original > + * value, allowing the hardware to detect and correct memory errors. > + */ The comment here is misleading since the function doesn't do anything. > +static inline void atomic_scrub(void *va, u32 size) > +{ > + /* Stub function for now until an ARM64 HW has a way to test it. */ > +} Can we have a WARN_ONCE() here? We had some discussions last year, restarted this year and the conclusion was to leave this function as a no-op since it is not safe in the presence of KVM or non-coherent DMA: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html -- Catalin ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 1/5] arm64: Enable EDAC on ARM64 2015-04-29 14:40 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Catalin Marinas @ 2015-04-29 14:46 ` Jon Masters 2015-04-29 21:39 ` Loc Ho 1 sibling, 0 replies; 44+ messages in thread From: Jon Masters @ 2015-04-29 14:46 UTC (permalink / raw) To: linux-arm-kernel On 04/29/2015 10:40 AM, Catalin Marinas wrote: > On Tue, Apr 28, 2015 at 04:10:41PM -0600, Loc Ho wrote: >> +/* >> + * ECC atomic, DMA, SMP and interrupt safe scrub function. >> + * Implements the per arch atomic_scrub() that EDAC use for software >> + * ECC scrubbing. It reads memory and then writes back the original >> + * value, allowing the hardware to detect and correct memory errors. >> + */ > > The comment here is misleading since the function doesn't do anything. Well, on the other hand, it is "safe" (joke) ;) Suggest a documented few line summary of the status from earlier this year/last year be there instead. Essentially atomic scrubbing is only safe when you're dealing with a fully coherent system. We captured various examples of things that could go wrong in the thread link. > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html Thanks Loc and team. Jon. ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7 1/5] arm64: Enable EDAC on ARM64 2015-04-29 14:40 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Catalin Marinas 2015-04-29 14:46 ` Jon Masters @ 2015-04-29 21:39 ` Loc Ho 1 sibling, 0 replies; 44+ messages in thread From: Loc Ho @ 2015-04-29 21:39 UTC (permalink / raw) To: linux-arm-kernel Hi, >> +/* >> + * ECC atomic, DMA, SMP and interrupt safe scrub function. >> + * Implements the per arch atomic_scrub() that EDAC use for software >> + * ECC scrubbing. It reads memory and then writes back the original >> + * value, allowing the hardware to detect and correct memory errors. >> + */ > > The comment here is misleading since the function doesn't do anything. Okay.. I will leave only this line: 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. */ >> +} > > Can we have a WARN_ONCE() here? > > We had some discussions last year, restarted this year and the > conclusion was to leave this function as a no-op since it is not safe in > the presence of KVM or non-coherent DMA: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html I will leave WARN_ONCE. -Loc ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2015-05-04 23:39 UTC | newest] Thread overview: 44+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-04-28 22:10 [PATCH v7 0/4] edac: Add APM X-Gene SoC EDAC driver Loc Ho 2015-04-28 22:10 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Loc Ho 2015-04-28 22:10 ` [PATCH v7 2/5] MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver Loc Ho 2015-04-28 22:10 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Loc Ho 2015-04-28 22:10 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Loc Ho 2015-04-28 22:10 ` [PATCH 5/5] arm64: Add APM X-Gene SoC EDAC DTS entries Loc Ho 2015-04-29 8:49 ` [PATCH v7 4/5] edac: Add APM X-Gene SoC EDAC driver Arnd Bergmann 2015-04-29 16:57 ` Rob Herring 2015-04-29 18:23 ` Arnd Bergmann 2015-04-29 17:00 ` Rob Herring 2015-04-29 16:47 ` [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding Rob Herring 2015-04-29 21:33 ` Loc Ho 2015-04-29 21:49 ` Borislav Petkov 2015-04-29 21:56 ` Loc Ho 2015-04-29 22:08 ` Borislav Petkov 2015-04-29 22:20 ` Loc Ho 2015-04-29 23:02 ` Rob Herring 2015-04-30 8:20 ` Borislav Petkov 2015-04-30 8:31 ` Arnd Bergmann 2015-04-30 8:45 ` Borislav Petkov 2015-04-30 9:01 ` Arnd Bergmann 2015-04-30 9:41 ` Borislav Petkov 2015-04-30 10:21 ` Arnd Bergmann 2015-04-30 12:33 ` Borislav Petkov 2015-04-30 12:52 ` Arnd Bergmann 2015-04-30 10:42 ` Arnd Bergmann 2015-04-30 13:00 ` Borislav Petkov 2015-04-30 16:57 ` Loc Ho 2015-04-30 17:18 ` Borislav Petkov 2015-04-30 21:19 ` Loc Ho 2015-04-30 21:30 ` Borislav Petkov 2015-04-30 21:39 ` Loc Ho 2015-04-30 22:36 ` Rob Herring 2015-04-30 22:47 ` Arnd Bergmann 2015-05-01 6:44 ` Loc Ho 2015-04-30 22:59 ` Loc Ho 2015-05-01 19:59 ` Loc Ho 2015-05-04 22:36 ` Rob Herring 2015-05-04 23:39 ` Loc Ho 2015-04-29 22:43 ` Rob Herring 2015-04-30 0:47 ` Loc Ho 2015-04-29 14:40 ` [PATCH v7 1/5] arm64: Enable EDAC on ARM64 Catalin Marinas 2015-04-29 14:46 ` Jon Masters 2015-04-29 21:39 ` Loc Ho
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).