[PATCH v2 0/8] bus: brcmstb_gisb: add support for GISBv7 arbiter

[PATCH v2 0/8] bus: brcmstb_gisb: add support for GISBv7 arbiter

[Doug Berger]

This patch set contains changes to enable the GISB arbiter driver
on the latest ARM64 architecture Set-Top Box chips from Broadcom.

This driver relies on being able to hook the abort handlers of
the processor core that are triggered by bus error signals
generated by the GISB bus arbiter hardware found in BCM7XXX chips.
The first two patches are based on the arm64/for-next/core
branch to enable this functionality for the arm64 architecture.

The remaining patches correct some issues with the existing driver,
add the ARM64 architecture specific support to the driver, and
finally add the new register map for the GISBv7 hardware first
appearing in the BCM7278 device.

Changes since v1 at [1]:
 - Removed code associated with hooking SError handling in favor
   of a registered notifier (Thanks Mark!)
 - Removed an unnecessary explicit cast (Thanks Gregory!)

[1] https://lkml.org/lkml/2017/3/24/413

Doug Berger (6):
  arm64: mm: mark fault_info __ro_after_init
  bus: brcmstb_gisb: Use register offsets with writes too
  bus: brcmstb_gisb: Correct hooking of ARM aborts
  bus: brcmstb_gisb: correct support for 64-bit address output
  bus: brcmstb_gisb: add notifier handling
  bus: brcmstb_gisb: update to support new revision

Florian Fainelli (2):
  arm64: mm: Allow installation of memory abort handlers
  bus: brcmstb_gisb: Add ARM64 support

 .../devicetree/bindings/bus/brcm,gisb-arb.txt      |   3 +-
 arch/arm64/include/asm/system_misc.h               |   3 +
 arch/arm64/mm/fault.c                              |  17 +++-
 drivers/bus/Kconfig                                |   2 +-
 drivers/bus/brcmstb_gisb.c                         | 111 ++++++++++++++++-----
 5 files changed, 106 insertions(+), 30 deletions(-)

-- 
2.12.0

[PATCH v2 1/8] arm64: mm: Allow installation of memory abort handlers

[Doug Berger]

From: Florian Fainelli <f.fainelli@gmail.com>

Similarly to what the ARM/Linux kernel provides, add a hook_fault_code()
function which allows drivers or other parts of the kernel to install
custom memory abort handlers. This is useful when a given SoC's busing
does not propagate the exact faulting physical address, but there is a
way to read it through e.g: a special arbiter driver.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/include/asm/system_misc.h |  3 +++
 arch/arm64/mm/fault.c                | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index bc812435bc76..e05f5b8c7c1c 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -38,6 +38,9 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
 					     struct pt_regs *),
 			   int sig, int code, const char *name);
+void hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
+				       struct pt_regs *),
+		     int sig, int code, const char *name);
 
 struct mm_struct;
 extern void show_pte(struct mm_struct *mm, unsigned long addr);
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 4bf899fb451b..cdf1260f1005 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -488,7 +488,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	return 1;
 }
 
-static const struct fault_info {
+static struct fault_info {
 	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
 	int	sig;
 	int	code;
@@ -560,6 +560,19 @@ static const struct fault_info {
 	{ do_bad,		SIGBUS,  0,		"unknown 63"			},
 };
 
+void __init hook_fault_code(int nr,
+			    int (*fn)(unsigned long, unsigned int, struct pt_regs *),
+			    int sig, int code, const char *name)
+{
+	BUG_ON(nr < 0 || nr >= ARRAY_SIZE(fault_info));
+
+	fault_info[nr].fn	= fn;
+	fault_info[nr].sig	= sig;
+	fault_info[nr].code	= code;
+	fault_info[nr].name	= name;
+}
+
+
 static const char *fault_name(unsigned int esr)
 {
 	const struct fault_info *inf = fault_info + (esr & 63);
-- 
2.12.0

Re: [PATCH v2 1/8] arm64: mm: Allow installation of memory abort handlers

[Mark Rutland]

Hi,

On Tue, Mar 28, 2017 at 02:34:24PM -0700, Doug Berger wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> Similarly to what the ARM/Linux kernel provides, add a hook_fault_code()
> function which allows drivers or other parts of the kernel to install
> custom memory abort handlers. This is useful when a given SoC's busing
> does not propagate the exact faulting physical address, but there is a
> way to read it through e.g: a special arbiter driver.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Personally, I do not think that it makes sense to allow arbitrary code
to hook such low-level fault handling.

IMO, if it is truly necessary to allow drivers to handle particular
faults, that should be driven by data associated with the relevant
mapping (e.g. the VMA), rather than allowing code to hook *all* faults.

>From my PoV, NAK to this interface to take over low-level fault
handling.

Catalin and Will have the final say here, as the arm64 maintainers.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/system_misc.h |  3 +++
>  arch/arm64/mm/fault.c                | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index bc812435bc76..e05f5b8c7c1c 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -38,6 +38,9 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
>  void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>  					     struct pt_regs *),
>  			   int sig, int code, const char *name);
> +void hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
> +				       struct pt_regs *),
> +		     int sig, int code, const char *name);
>  
>  struct mm_struct;
>  extern void show_pte(struct mm_struct *mm, unsigned long addr);
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 4bf899fb451b..cdf1260f1005 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -488,7 +488,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  	return 1;
>  }
>  
> -static const struct fault_info {
> +static struct fault_info {
>  	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
>  	int	sig;
>  	int	code;
> @@ -560,6 +560,19 @@ static const struct fault_info {
>  	{ do_bad,		SIGBUS,  0,		"unknown 63"			},
>  };
>  
> +void __init hook_fault_code(int nr,
> +			    int (*fn)(unsigned long, unsigned int, struct pt_regs *),
> +			    int sig, int code, const char *name)
> +{
> +	BUG_ON(nr < 0 || nr >= ARRAY_SIZE(fault_info));
> +
> +	fault_info[nr].fn	= fn;
> +	fault_info[nr].sig	= sig;
> +	fault_info[nr].code	= code;
> +	fault_info[nr].name	= name;
> +}
> +
> +
>  static const char *fault_name(unsigned int esr)
>  {
>  	const struct fault_info *inf = fault_info + (esr & 63);
> -- 
> 2.12.0
> 

[PATCH v2 2/8] arm64: mm: mark fault_info __ro_after_init

[Doug Berger]

The fault_info table must be made writeable to allow installation
of custom memory abort handlers, but it can be made read-only
after initialization to provide some protection.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 arch/arm64/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index cdf1260f1005..43319ed58a47 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -493,7 +493,7 @@ static struct fault_info {
 	int	sig;
 	int	code;
 	const char *name;
-} fault_info[] = {
+} fault_info[] __ro_after_init = {
 	{ do_bad,		SIGBUS,  0,		"ttbr address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 1 address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 2 address size fault"	},
-- 
2.12.0

Re: [PATCH v2 2/8] arm64: mm: mark fault_info __ro_after_init

[Mark Rutland]

On Tue, Mar 28, 2017 at 02:34:25PM -0700, Doug Berger wrote:
> The fault_info table must be made writeable to allow installation
> of custom memory abort handlers, but it can be made read-only
> after initialization to provide some protection.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Thanks for putting this together. I agree that making this RO is good.

However, I also think that we should not allow arbitrary hooking of
fault codes, and thus this can be const.

Thanks,
Mark.

> ---
>  arch/arm64/mm/fault.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index cdf1260f1005..43319ed58a47 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -493,7 +493,7 @@ static struct fault_info {
>  	int	sig;
>  	int	code;
>  	const char *name;
> -} fault_info[] = {
> +} fault_info[] __ro_after_init = {
>  	{ do_bad,		SIGBUS,  0,		"ttbr address size fault"	},
>  	{ do_bad,		SIGBUS,  0,		"level 1 address size fault"	},
>  	{ do_bad,		SIGBUS,  0,		"level 2 address size fault"	},
> -- 
> 2.12.0
> 

[PATCH v2 3/8] bus: brcmstb_gisb: Use register offsets with writes too

[Doug Berger]

This commit corrects the bug introduced in commit f80835875d3d
("bus: brcmstb_gisb: Look up register offsets in a table") such
that gisb_write() translates the register enumeration into an
offset from the base address for writes as well as reads.

Fixes: f80835875d3d ("bus: brcmstb_gisb: Look up register offsets in a table")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Acked-by: Gregory Fong <gregory.0xf0@gmail.com>
---
 drivers/bus/brcmstb_gisb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index 72fe0a5a8bf3..a94598d0945a 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Broadcom Corporation
+ * Copyright (C) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -127,9 +127,9 @@ static void gisb_write(struct brcmstb_gisb_arb_device *gdev, u32 val, int reg)
 		return;
 
 	if (gdev->big_endian)
-		iowrite32be(val, gdev->base + reg);
+		iowrite32be(val, gdev->base + offset);
 	else
-		iowrite32(val, gdev->base + reg);
+		iowrite32(val, gdev->base + offset);
 }
 
 static ssize_t gisb_arb_get_timeout(struct device *dev,
-- 
2.12.0

[PATCH v2 4/8] bus: brcmstb_gisb: Correct hooking of ARM aborts

[Doug Berger]

The fault status reporting in the FSR registers is different depending
on whether the Long Physical Address Extension (LPAE) is being used.

This commit corrects the registerring of fault handlers for arm
architecture kernels when the LPAE is enabled.  It also forces the
handler to report that the abort exception was unhandled so that the
appropriate signal is sent to the offending user process.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/bus/brcmstb_gisb.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index a94598d0945a..9eba0143f1a4 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -225,27 +225,29 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 				     struct pt_regs *regs)
 {
-	int ret = 0;
 	struct brcmstb_gisb_arb_device *gdev;
 
 	/* iterate over each GISB arb registered handlers */
 	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
-		ret |= brcmstb_gisb_arb_decode_addr(gdev, "bus error");
+		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
+
+#if !defined(CONFIG_ARM_LPAE)
 	/*
 	 * If it was an imprecise abort, then we need to correct the
 	 * return address to be _after_ the instruction.
 	*/
 	if (fsr & (1 << 10))
 		regs->ARM_pc += 4;
+#endif
 
-	return ret;
+	/* Always report unhandled exception */
+	return 1;
 }
 #endif
 
 #ifdef CONFIG_MIPS
 static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)
 {
-	int ret = 0;
 	struct brcmstb_gisb_arb_device *gdev;
 	u32 cap_status;
 
@@ -258,7 +260,7 @@ static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)
 			goto out;
 		}
 
-		ret |= brcmstb_gisb_arb_decode_addr(gdev, "bus error");
+		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
 	}
 out:
 	return is_fixup ? MIPS_BE_FIXUP : MIPS_BE_FATAL;
@@ -379,9 +381,16 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	list_add_tail(&gdev->next, &brcmstb_gisb_arb_device_list);
 
 #ifdef CONFIG_ARM
+#ifdef CONFIG_ARM_LPAE
+	hook_fault_code(16, brcmstb_bus_error_handler, SIGBUS, 0,
+			"synchronous external abort");
+	hook_fault_code(17, brcmstb_bus_error_handler, SIGBUS, 0,
+			"asynchronous external abort");
+#else
 	hook_fault_code(22, brcmstb_bus_error_handler, SIGBUS, 0,
 			"imprecise external abort");
 #endif
+#endif /* CONFIG_ARM */
 #ifdef CONFIG_MIPS
 	board_be_handler = brcmstb_bus_error_handler;
 #endif
-- 
2.12.0

[PATCH v2 5/8] bus: brcmstb_gisb: correct support for 64-bit address output

[Doug Berger]

The GISB bus can support addresses beyond 32-bits.  So this commit
corrects support for reading a captured 64-bit address into a 64-bit
variable by obtaining the high bits from the ARB_ERR_CAP_HI_ADDR
register (when present) and then outputting the full 64-bit value.

It also removes unused definitions.

Fixes: 44127b771d9c ("bus: add Broadcom GISB bus arbiter timeout/error handler")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Acked-by: Gregory Fong <gregory.0xf0@gmail.com>
---
 drivers/bus/brcmstb_gisb.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index 9eba0143f1a4..edf79432f899 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -37,8 +37,6 @@
 #define  ARB_ERR_CAP_CLEAR		(1 << 0)
 #define  ARB_ERR_CAP_STATUS_TIMEOUT	(1 << 12)
 #define  ARB_ERR_CAP_STATUS_TEA		(1 << 11)
-#define  ARB_ERR_CAP_STATUS_BS_SHIFT	(1 << 2)
-#define  ARB_ERR_CAP_STATUS_BS_MASK	0x3c
 #define  ARB_ERR_CAP_STATUS_WRITE	(1 << 1)
 #define  ARB_ERR_CAP_STATUS_VALID	(1 << 0)
 
@@ -47,7 +45,6 @@ enum {
 	ARB_ERR_CAP_CLR,
 	ARB_ERR_CAP_HI_ADDR,
 	ARB_ERR_CAP_ADDR,
-	ARB_ERR_CAP_DATA,
 	ARB_ERR_CAP_STATUS,
 	ARB_ERR_CAP_MASTER,
 };
@@ -57,7 +54,6 @@ static const int gisb_offsets_bcm7038[] = {
 	[ARB_ERR_CAP_CLR]	= 0x0c4,
 	[ARB_ERR_CAP_HI_ADDR]	= -1,
 	[ARB_ERR_CAP_ADDR]	= 0x0c8,
-	[ARB_ERR_CAP_DATA]	= 0x0cc,
 	[ARB_ERR_CAP_STATUS]	= 0x0d0,
 	[ARB_ERR_CAP_MASTER]	= -1,
 };
@@ -67,7 +63,6 @@ static const int gisb_offsets_bcm7400[] = {
 	[ARB_ERR_CAP_CLR]	= 0x0c8,
 	[ARB_ERR_CAP_HI_ADDR]	= -1,
 	[ARB_ERR_CAP_ADDR]	= 0x0cc,
-	[ARB_ERR_CAP_DATA]	= 0x0d0,
 	[ARB_ERR_CAP_STATUS]	= 0x0d4,
 	[ARB_ERR_CAP_MASTER]	= 0x0d8,
 };
@@ -77,7 +72,6 @@ static const int gisb_offsets_bcm7435[] = {
 	[ARB_ERR_CAP_CLR]	= 0x168,
 	[ARB_ERR_CAP_HI_ADDR]	= -1,
 	[ARB_ERR_CAP_ADDR]	= 0x16c,
-	[ARB_ERR_CAP_DATA]	= 0x170,
 	[ARB_ERR_CAP_STATUS]	= 0x174,
 	[ARB_ERR_CAP_MASTER]	= 0x178,
 };
@@ -87,7 +81,6 @@ static const int gisb_offsets_bcm7445[] = {
 	[ARB_ERR_CAP_CLR]	= 0x7e4,
 	[ARB_ERR_CAP_HI_ADDR]	= 0x7e8,
 	[ARB_ERR_CAP_ADDR]	= 0x7ec,
-	[ARB_ERR_CAP_DATA]	= 0x7f0,
 	[ARB_ERR_CAP_STATUS]	= 0x7f4,
 	[ARB_ERR_CAP_MASTER]	= 0x7f8,
 };
@@ -109,9 +102,13 @@ static u32 gisb_read(struct brcmstb_gisb_arb_device *gdev, int reg)
 {
 	int offset = gdev->gisb_offsets[reg];
 
-	/* return 1 if the hardware doesn't have ARB_ERR_CAP_MASTER */
-	if (offset == -1)
-		return 1;
+	if (offset < 0) {
+		/* return 1 if the hardware doesn't have ARB_ERR_CAP_MASTER */
+		if (reg == ARB_ERR_CAP_MASTER)
+			return 1;
+		else
+			return 0;
+	}
 
 	if (gdev->big_endian)
 		return ioread32be(gdev->base + offset);
@@ -119,6 +116,16 @@ static u32 gisb_read(struct brcmstb_gisb_arb_device *gdev, int reg)
 		return ioread32(gdev->base + offset);
 }
 
+static u64 gisb_read_address(struct brcmstb_gisb_arb_device *gdev)
+{
+	u64 value;
+
+	value = gisb_read(gdev, ARB_ERR_CAP_ADDR);
+	value |= (u64)gisb_read(gdev, ARB_ERR_CAP_HI_ADDR) << 32;
+
+	return value;
+}
+
 static void gisb_write(struct brcmstb_gisb_arb_device *gdev, u32 val, int reg)
 {
 	int offset = gdev->gisb_offsets[reg];
@@ -185,7 +192,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 					const char *reason)
 {
 	u32 cap_status;
-	unsigned long arb_addr;
+	u64 arb_addr;
 	u32 master;
 	const char *m_name;
 	char m_fmt[11];
@@ -197,10 +204,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 		return 1;
 
 	/* Read the address and master */
-	arb_addr = gisb_read(gdev, ARB_ERR_CAP_ADDR) & 0xffffffff;
-#if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
-	arb_addr |= (u64)gisb_read(gdev, ARB_ERR_CAP_HI_ADDR) << 32;
-#endif
+	arb_addr = gisb_read_address(gdev);
 	master = gisb_read(gdev, ARB_ERR_CAP_MASTER);
 
 	m_name = brcmstb_gisb_master_to_str(gdev, master);
@@ -209,7 +213,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 		m_name = m_fmt;
 	}
 
-	pr_crit("%s: %s at 0x%lx [%c %s], core: %s\n",
+	pr_crit("%s: %s at 0x%llx [%c %s], core: %s\n",
 		__func__, reason, arb_addr,
 		cap_status & ARB_ERR_CAP_STATUS_WRITE ? 'W' : 'R',
 		cap_status & ARB_ERR_CAP_STATUS_TIMEOUT ? "timeout" : "",
-- 
2.12.0

[PATCH v2 6/8] bus: brcmstb_gisb: Add ARM64 support

[Doug Berger]

From: Florian Fainelli <f.fainelli@gmail.com>

Hook to the ARM64 data abort exception #16: synchronous external
abort, which is how the GISB errors will be funneled back to the
ARM64 CPU in case of problems

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/bus/Kconfig        |  2 +-
 drivers/bus/brcmstb_gisb.c | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 0a52da439abf..d2a5f1184022 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -57,7 +57,7 @@ config ARM_CCN
 
 config BRCMSTB_GISB_ARB
 	bool "Broadcom STB GISB bus arbiter"
-	depends on ARM || MIPS
+	depends on ARM || ARM64 || MIPS
 	default ARCH_BRCMSTB || BMIPS_GENERIC
 	help
 	  Driver for the Broadcom Set Top Box System-on-a-chip internal bus
diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index edf79432f899..500b6bb5c739 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -30,6 +30,11 @@
 #include <asm/signal.h>
 #endif
 
+#ifdef CONFIG_ARM64
+#include <asm/signal.h>
+#include <asm/system_misc.h>
+#endif
+
 #ifdef CONFIG_MIPS
 #include <asm/traps.h>
 #endif
@@ -225,7 +230,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 	return 0;
 }
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 				     struct pt_regs *regs)
 {
@@ -235,7 +240,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
 		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
 
-#if !defined(CONFIG_ARM_LPAE)
+#if defined(CONFIG_ARM) && !defined(CONFIG_ARM_LPAE)
 	/*
 	 * If it was an imprecise abort, then we need to correct the
 	 * return address to be _after_ the instruction.
@@ -247,7 +252,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	/* Always report unhandled exception */
 	return 1;
 }
-#endif
+#endif /* CONFIG_ARM || CONFIG_ARM64 */
 
 #ifdef CONFIG_MIPS
 static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)
@@ -395,6 +400,10 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 			"imprecise external abort");
 #endif
 #endif /* CONFIG_ARM */
+#ifdef CONFIG_ARM64
+	hook_fault_code(16, brcmstb_bus_error_handler, SIGBUS, 0,
+			"synchronous external abort");
+#endif
 #ifdef CONFIG_MIPS
 	board_be_handler = brcmstb_bus_error_handler;
 #endif
-- 
2.12.0

Re: [PATCH v2 6/8] bus: brcmstb_gisb: Add ARM64 support

[Mark Rutland]

Hi,

On Tue, Mar 28, 2017 at 02:34:29PM -0700, Doug Berger wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> Hook to the ARM64 data abort exception #16: synchronous external
> abort, which is how the GISB errors will be funneled back to the
> ARM64 CPU in case of problems

I believe that you can use a die notifier for this, and that you don't
need to hook the low-level architectural fault here.

I note that the code doesn't even look at the faulting address, and
likely doesn't have the information to do anything with it, so it make
no sense to me for this code to hook the low-level fault.

Further, as an aside, from digging into how we handle unexpected faults
it appears that we don't always treat some faults (e.g. TLB conflict,
faults on PTW) sufficiently fatally when they occur at EL0, so we likely
need to do so rework there.

Thanks,
Mark.

> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/bus/Kconfig        |  2 +-
>  drivers/bus/brcmstb_gisb.c | 15 ++++++++++++---
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 0a52da439abf..d2a5f1184022 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -57,7 +57,7 @@ config ARM_CCN
>  
>  config BRCMSTB_GISB_ARB
>  	bool "Broadcom STB GISB bus arbiter"
> -	depends on ARM || MIPS
> +	depends on ARM || ARM64 || MIPS
>  	default ARCH_BRCMSTB || BMIPS_GENERIC
>  	help
>  	  Driver for the Broadcom Set Top Box System-on-a-chip internal bus
> diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
> index edf79432f899..500b6bb5c739 100644
> --- a/drivers/bus/brcmstb_gisb.c
> +++ b/drivers/bus/brcmstb_gisb.c
> @@ -30,6 +30,11 @@
>  #include <asm/signal.h>
>  #endif
>  
> +#ifdef CONFIG_ARM64
> +#include <asm/signal.h>
> +#include <asm/system_misc.h>
> +#endif
> +
>  #ifdef CONFIG_MIPS
>  #include <asm/traps.h>
>  #endif
> @@ -225,7 +230,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
>  	return 0;
>  }
>  
> -#ifdef CONFIG_ARM
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>  static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
>  				     struct pt_regs *regs)
>  {
> @@ -235,7 +240,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
>  	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
>  		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
>  
> -#if !defined(CONFIG_ARM_LPAE)
> +#if defined(CONFIG_ARM) && !defined(CONFIG_ARM_LPAE)
>  	/*
>  	 * If it was an imprecise abort, then we need to correct the
>  	 * return address to be _after_ the instruction.
> @@ -247,7 +252,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
>  	/* Always report unhandled exception */
>  	return 1;
>  }
> -#endif
> +#endif /* CONFIG_ARM || CONFIG_ARM64 */
>  
>  #ifdef CONFIG_MIPS
>  static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)
> @@ -395,6 +400,10 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
>  			"imprecise external abort");
>  #endif
>  #endif /* CONFIG_ARM */
> +#ifdef CONFIG_ARM64
> +	hook_fault_code(16, brcmstb_bus_error_handler, SIGBUS, 0,
> +			"synchronous external abort");
> +#endif
>  #ifdef CONFIG_MIPS
>  	board_be_handler = brcmstb_bus_error_handler;
>  #endif
> -- 
> 2.12.0
> 

[PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

[Doug Berger]

Check for GISB arbitration errors through a chained notifier
when a process dies or a kernel panic occurs.  This allows a
meaningful diagnostic message to occur along with other
diagnostic information.

Notably writes to a bad GISB address on an ARM64 architecture
kernel cause unrecoverable SError aborts and this allows the
cause of the abort to be seen.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/bus/brcmstb_gisb.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index 500b6bb5c739..774729002b8c 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -24,6 +24,8 @@
 #include <linux/of.h>
 #include <linux/bitops.h>
 #include <linux/pm.h>
+#include <linux/kernel.h>
+#include <linux/kdebug.h>
 
 #ifdef CONFIG_ARM
 #include <asm/bug.h>
@@ -290,6 +292,25 @@ static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/*
+ * Dump out gisb errors on die or panic.
+ */
+static int dump_gisb_error(struct notifier_block *self, unsigned long v,
+			   void *p)
+{
+	struct brcmstb_gisb_arb_device *gdev;
+
+	/* iterate over each GISB arb registered handlers */
+	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
+		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
+
+	return 0;
+}
+
+static struct notifier_block gisb_error_notifier = {
+	.notifier_call = dump_gisb_error,
+};
+
 static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
 		gisb_arb_get_timeout, gisb_arb_set_timeout);
 
@@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	board_be_handler = brcmstb_bus_error_handler;
 #endif
 
+	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
+		register_die_notifier(&gisb_error_notifier);
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &gisb_error_notifier);
+	}
+
 	dev_info(&pdev->dev, "registered mem: %p, irqs: %d, %d\n",
 			gdev->base, timeout_irq, tea_irq);
 
-- 
2.12.0

Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

[Mark Rutland]

Hi Doug,

On Tue, Mar 28, 2017 at 02:34:30PM -0700, Doug Berger wrote:
> Check for GISB arbitration errors through a chained notifier
> when a process dies or a kernel panic occurs.  This allows a
> meaningful diagnostic message to occur along with other
> diagnostic information.
> 
> Notably writes to a bad GISB address on an ARM64 architecture
> kernel cause unrecoverable SError aborts and this allows the
> cause of the abort to be seen.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Thanks for changing this.

> ---
>  drivers/bus/brcmstb_gisb.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
> index 500b6bb5c739..774729002b8c 100644
> --- a/drivers/bus/brcmstb_gisb.c
> +++ b/drivers/bus/brcmstb_gisb.c
> @@ -24,6 +24,8 @@
>  #include <linux/of.h>
>  #include <linux/bitops.h>
>  #include <linux/pm.h>
> +#include <linux/kernel.h>
> +#include <linux/kdebug.h>
>  
>  #ifdef CONFIG_ARM
>  #include <asm/bug.h>
> @@ -290,6 +292,25 @@ static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * Dump out gisb errors on die or panic.
> + */
> +static int dump_gisb_error(struct notifier_block *self, unsigned long v,
> +			   void *p)
> +{
> +	struct brcmstb_gisb_arb_device *gdev;
> +
> +	/* iterate over each GISB arb registered handlers */
> +	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
> +		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
> +
> +	return 0;

I think this should be NOTIFY_OK.

> +}
> +
> +static struct notifier_block gisb_error_notifier = {
> +	.notifier_call = dump_gisb_error,
> +};
> +
>  static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
>  		gisb_arb_get_timeout, gisb_arb_set_timeout);
>  
> @@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
>  	board_be_handler = brcmstb_bus_error_handler;
>  #endif
>  
> +	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
> +		register_die_notifier(&gisb_error_notifier);
> +		atomic_notifier_chain_register(&panic_notifier_list,
> +					       &gisb_error_notifier);

I don't think this is quite right. A notifier_block can only be
registered to one notifier chain at a time, and this has the potential
to corrupt both chains.

I also think you only need to register the panic notifier. An SError
should always result in a panic.

Thanks,
Mark.

Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

[Doug Berger]

On 03/29/2017 03:13 AM, Mark Rutland wrote:

snip

>> +/*
>> + * Dump out gisb errors on die or panic.
>> + */
>> +static int dump_gisb_error(struct notifier_block *self, unsigned long v,
>> +			   void *p)
>> +{
>> +	struct brcmstb_gisb_arb_device *gdev;
>> +
>> +	/* iterate over each GISB arb registered handlers */
>> +	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
>> +		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
>> +
>> +	return 0;
> 
> I think this should be NOTIFY_OK.
> 
I used dump_mem_limit() as a template and didn't catch this (work to
do...).  Upon review I think I would prefer NOTIFY_DONE since this call
is opportunistic (i.e. it is taking the opportunity to check whether
additional diagnostic data is available to display) and has no interest
in affecting the overall handling of the event.

>> +}
>> +
>> +static struct notifier_block gisb_error_notifier = {
>> +	.notifier_call = dump_gisb_error,
>> +};
>> +
>>  static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
>>  		gisb_arb_get_timeout, gisb_arb_set_timeout);
>>  
>> @@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
>>  	board_be_handler = brcmstb_bus_error_handler;
>>  #endif
>>  
>> +	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
>> +		register_die_notifier(&gisb_error_notifier);
>> +		atomic_notifier_chain_register(&panic_notifier_list,
>> +					       &gisb_error_notifier);
> 
> I don't think this is quite right. A notifier_block can only be
> registered to one notifier chain at a time, and this has the potential
> to corrupt both chains.
> 
A VERY good point thanks for pointing this out.

> I also think you only need to register the panic notifier. An SError
> should always result in a panic.
> 
That was my initial thought as well.  However, testing revealed that the
bad mode Oops actually exits the user space process and doesn't reach
the panic so there was no helpful diagnostic message.  This may be in
line with your comments about insufficient fatality of failures in PATCH
v2 6/8, but it actually is more in line with our desired behavior for
the aborted write.  Setting the notify on die gave us the result we are
looking for, but as noted above I should have created a separate notifier.

I had hoped that the same approach (i.e. die notifier) would remove the
need for PATCH v2 6/8 as well, but I found that the Unhandled fault
error didn't actually die from user mode.

> Thanks,
> Mark.
> 
Thank you for your help with this,
    Doug

Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

[Mark Rutland]

On Wed, Mar 29, 2017 at 10:39:11AM -0700, Doug Berger wrote:
> On 03/29/2017 03:13 AM, Mark Rutland wrote:

> >> +static int dump_gisb_error(struct notifier_block *self, unsigned long v,
> >> +			   void *p)

> >> +	return 0;
> > 
> > I think this should be NOTIFY_OK.
> > 
> I used dump_mem_limit() as a template and didn't catch this (work to
> do...).  Upon review I think I would prefer NOTIFY_DONE since this call
> is opportunistic (i.e. it is taking the opportunity to check whether
> additional diagnostic data is available to display) and has no interest
> in affecting the overall handling of the event.

That's fine by me.

Does the distinction matter here?

Most notifer users treat NOTIFY_OK and NOTIFY_DONE as equivalent, and
notifier_call_chain only terminates when it sees NOTIFY_STOP_MASK.

[...]

> >> +	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
> >> +		register_die_notifier(&gisb_error_notifier);
> >> +		atomic_notifier_chain_register(&panic_notifier_list,
> >> +					       &gisb_error_notifier);
> > 
> > I don't think this is quite right. A notifier_block can only be
> > registered to one notifier chain at a time, and this has the potential
> > to corrupt both chains.
> > 
> A VERY good point thanks for pointing this out.
> 
> > I also think you only need to register the panic notifier. An SError
> > should always result in a panic.
> > 
> That was my initial thought as well.  However, testing revealed that the
> bad mode Oops actually exits the user space process and doesn't reach
> the panic so there was no helpful diagnostic message.  This may be in
> line with your comments about insufficient fatality of failures in PATCH
> v2 6/8, but it actually is more in line with our desired behavior for
> the aborted write.  Setting the notify on die gave us the result we are
> looking for, but as noted above I should have created a separate notifier.
> 
> I had hoped that the same approach (i.e. die notifier) would remove the
> need for PATCH v2 6/8 as well, but I found that the Unhandled fault
> error didn't actually die from user mode.

In my mind it's a bug that we don't treat those errors more fatally.

I'll try to dig into that.

Thanks,
Mark.

[PATCH v2 8/8] bus: brcmstb_gisb: update to support new revision

[Doug Berger]

The 7278 introduces a new version of this core.  This
commit adds support for that revision.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt |  3 ++-
 drivers/bus/brcmstb_gisb.c                              | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt b/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt
index 1eceefb20f01..8a6c3c2e58fe 100644
--- a/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt
+++ b/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt
@@ -3,7 +3,8 @@ Broadcom GISB bus Arbiter controller
 Required properties:
 
 - compatible:
-    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for 28nm chips
+    "brcm,bcm7278-gisb-arb" for V7 28nm chips
+    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for other 28nm chips
     "brcm,bcm7435-gisb-arb" for newer 40nm chips
     "brcm,bcm7400-gisb-arb" for older 40nm chips and all 65nm chips
     "brcm,bcm7038-gisb-arb" for 130nm chips
diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index 774729002b8c..6c3b0dda75f2 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -65,6 +65,15 @@ static const int gisb_offsets_bcm7038[] = {
 	[ARB_ERR_CAP_MASTER]	= -1,
 };
 
+static const int gisb_offsets_bcm7278[] = {
+	[ARB_TIMER]		= 0x008,
+	[ARB_ERR_CAP_CLR]	= 0x7f8,
+	[ARB_ERR_CAP_HI_ADDR]	= -1,
+	[ARB_ERR_CAP_ADDR]	= 0x7e0,
+	[ARB_ERR_CAP_STATUS]	= 0x7f0,
+	[ARB_ERR_CAP_MASTER]	= 0x7f4,
+};
+
 static const int gisb_offsets_bcm7400[] = {
 	[ARB_TIMER]		= 0x00c,
 	[ARB_ERR_CAP_CLR]	= 0x0c8,
@@ -328,6 +337,7 @@ static const struct of_device_id brcmstb_gisb_arb_of_match[] = {
 	{ .compatible = "brcm,bcm7445-gisb-arb", .data = gisb_offsets_bcm7445 },
 	{ .compatible = "brcm,bcm7435-gisb-arb", .data = gisb_offsets_bcm7435 },
 	{ .compatible = "brcm,bcm7400-gisb-arb", .data = gisb_offsets_bcm7400 },
+	{ .compatible = "brcm,bcm7278-gisb-arb", .data = gisb_offsets_bcm7278 },
 	{ .compatible = "brcm,bcm7038-gisb-arb", .data = gisb_offsets_bcm7038 },
 	{ },
 };
-- 
2.12.0

Re: [PATCH v2 8/8] bus: brcmstb_gisb: update to support new revision

[Mark Rutland]

On Tue, Mar 28, 2017 at 02:34:31PM -0700, Doug Berger wrote:
> The 7278 introduces a new version of this core.  This
> commit adds support for that revision.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>  Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt |  3 ++-
>  drivers/bus/brcmstb_gisb.c                              | 10 ++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt b/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt
> index 1eceefb20f01..8a6c3c2e58fe 100644
> --- a/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt
> +++ b/Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt
> @@ -3,7 +3,8 @@ Broadcom GISB bus Arbiter controller
>  Required properties:
>  
>  - compatible:
> -    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for 28nm chips
> +    "brcm,bcm7278-gisb-arb" for V7 28nm chips
> +    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for other 28nm chips
>      "brcm,bcm7435-gisb-arb" for newer 40nm chips
>      "brcm,bcm7400-gisb-arb" for older 40nm chips and all 65nm chips
>      "brcm,bcm7038-gisb-arb" for 130nm chips

For the binding:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.